[U-Boot] Fixed clock driver (clk: convert API to match reset/mailbox style)
Hi guys,
I just found today with playing with clock drivers that the patch clk: convert API to match reset/mailbox style (sha1: 135aa95002646c46e89de93fa36adad1b010548f)
added this part of code to fixed clock driver -static ulong clk_fixed_rate_get_rate(struct udevice *dev) +static ulong clk_fixed_rate_get_rate(struct clk *clk) { - return to_clk_fixed_rate(dev)->fixed_rate; -} + if (clk->id != 0) + return -EINVAL;
which is returning -EINVAL when ulong should be returned.
The next thing I have found is that fixed clock driver has no set_rate function which is fine but when I was testing one driver which tries to set rate then error code was generated but without any useful information what happened. Are you ok with adding empty set_rate function with returning error message that set rate is not supported for fixed clocks?
Thanks, Michal
Hi Michal,
On 15 November 2016 at 12:07, Michal Simek monstr@monstr.eu wrote:
Hi guys,
I just found today with playing with clock drivers that the patch clk: convert API to match reset/mailbox style (sha1: 135aa95002646c46e89de93fa36adad1b010548f)
added this part of code to fixed clock driver -static ulong clk_fixed_rate_get_rate(struct udevice *dev) +static ulong clk_fixed_rate_get_rate(struct clk *clk) {
return to_clk_fixed_rate(dev)->fixed_rate;-}
if (clk->id != 0)return -EINVAL;which is returning -EINVAL when ulong should be returned.
This is intended - you can use IS_ERR_VALUE() to check if it is an error.
The next thing I have found is that fixed clock driver has no set_rate function which is fine but when I was testing one driver which tries to set rate then error code was generated but without any useful information what happened.
It should return -ENOSYS, right?
Are you ok with adding empty set_rate function with returning error message that set rate is not supported for fixed clocks?
What would it return that is different? If you are asking for a printed error message, that would bloat the code. So long as the caller checks the error we should be OK.
Regards, Simon
Hi Simon,
On 15.11.2016 20:23, Simon Glass wrote:
Hi Michal,
On 15 November 2016 at 12:07, Michal Simek monstr@monstr.eu wrote:
Hi guys,
I just found today with playing with clock drivers that the patch clk: convert API to match reset/mailbox style (sha1: 135aa95002646c46e89de93fa36adad1b010548f)
added this part of code to fixed clock driver -static ulong clk_fixed_rate_get_rate(struct udevice *dev) +static ulong clk_fixed_rate_get_rate(struct clk *clk) {
return to_clk_fixed_rate(dev)->fixed_rate;-}
if (clk->id != 0)return -EINVAL;which is returning -EINVAL when ulong should be returned.
This is intended - you can use IS_ERR_VALUE() to check if it is an error.
ok then some drivers needs to be fixed.
drivers/mmc/atmel_sdhci.c:90: ret = clk_set_rate(&clk, gck_rate); drivers/mmc/atmel_sdhci.c-91- if (ret) drivers/mmc/atmel_sdhci.c-92- return ret;
drivers/mmc/msm_sdhci.c:79: ret = clk_set_rate(&clk, clk_rate); drivers/mmc/msm_sdhci.c-80- clk_free(&clk); drivers/mmc/msm_sdhci.c-81- if (ret < 0)
drivers/net/dwc_eth_qos.c:484: ret = clk_set_rate(&eqos->clk_ptp_ref, 125 * 1000 * 1000); drivers/net/dwc_eth_qos.c-485- if (ret < 0) { drivers/net/dwc_eth_qos.c:486: error("clk_set_rate(clk_ptp_ref) failed: %d", ret); drivers/net/dwc_eth_qos.c-487- goto err_disable_clk_ptp_ref; drivers/net/dwc_eth_qos.c-488- }
drivers/serial/serial_msm.c:175: ret = clk_set_rate(&clk, clk_rate); drivers/serial/serial_msm.c-176- clk_free(&clk); drivers/serial/serial_msm.c-177- if (ret < 0)
drivers/spi/rk_spi.c:176: ret = clk_set_rate(&priv->clk, 99000000); drivers/spi/rk_spi.c-177- if (ret < 0) { drivers/spi/rk_spi.c-178- debug("%s: Failed to set clock: %d\n", __func__, ret);
The next thing I have found is that fixed clock driver has no set_rate function which is fine but when I was testing one driver which tries to set rate then error code was generated but without any useful information what happened.
It should return -ENOSYS, right?
I think -EINVAL in driver as reaction for incorrect id looks good. It was more about returning minus value where you should return unsigned.
Are you ok with adding empty set_rate function with returning error message that set rate is not supported for fixed clocks?
What would it return that is different? If you are asking for a printed error message, that would bloat the code. So long as the caller checks the error we should be OK.
Caller checks the return code for sure but it is a question if this is enough for people to know what's wrong. When this happen you have no clue that this problem is coming from clock subsystem. I can add one print message to the driver but the same message will end up in all these drivers.
Thanks, Michal
Hi Michal,
On 16 November 2016 at 00:15, Michal Simek michal.simek@xilinx.com wrote:
Hi Simon,
On 15.11.2016 20:23, Simon Glass wrote:
Hi Michal,
On 15 November 2016 at 12:07, Michal Simek monstr@monstr.eu wrote:
Hi guys,
I just found today with playing with clock drivers that the patch clk: convert API to match reset/mailbox style (sha1: 135aa95002646c46e89de93fa36adad1b010548f)
added this part of code to fixed clock driver -static ulong clk_fixed_rate_get_rate(struct udevice *dev) +static ulong clk_fixed_rate_get_rate(struct clk *clk) {
return to_clk_fixed_rate(dev)->fixed_rate;-}
if (clk->id != 0)return -EINVAL;which is returning -EINVAL when ulong should be returned.
This is intended - you can use IS_ERR_VALUE() to check if it is an error.
ok then some drivers needs to be fixed.
drivers/mmc/atmel_sdhci.c:90: ret = clk_set_rate(&clk, gck_rate); drivers/mmc/atmel_sdhci.c-91- if (ret) drivers/mmc/atmel_sdhci.c-92- return ret;
drivers/mmc/msm_sdhci.c:79: ret = clk_set_rate(&clk, clk_rate); drivers/mmc/msm_sdhci.c-80- clk_free(&clk); drivers/mmc/msm_sdhci.c-81- if (ret < 0)
drivers/net/dwc_eth_qos.c:484: ret = clk_set_rate(&eqos->clk_ptp_ref, 125 * 1000 * 1000); drivers/net/dwc_eth_qos.c-485- if (ret < 0) { drivers/net/dwc_eth_qos.c:486: error("clk_set_rate(clk_ptp_ref) failed: %d", ret); drivers/net/dwc_eth_qos.c-487- goto err_disable_clk_ptp_ref; drivers/net/dwc_eth_qos.c-488- }
drivers/serial/serial_msm.c:175: ret = clk_set_rate(&clk, clk_rate); drivers/serial/serial_msm.c-176- clk_free(&clk); drivers/serial/serial_msm.c-177- if (ret < 0)
drivers/spi/rk_spi.c:176: ret = clk_set_rate(&priv->clk, 99000000); drivers/spi/rk_spi.c-177- if (ret < 0) { drivers/spi/rk_spi.c-178- debug("%s: Failed to set clock: %d\n", __func__, ret);
Yes indeed.
The next thing I have found is that fixed clock driver has no set_rate function which is fine but when I was testing one driver which tries to set rate then error code was generated but without any useful information what happened.
It should return -ENOSYS, right?
I think -EINVAL in driver as reaction for incorrect id looks good. It was more about returning minus value where you should return unsigned.
Yes that's right. I meant -ENOSYS when the driver does not implement the method.
But consider -ENOENT when there is no such thing, as we tend to use -EINVAL for invalid data (e.g. in device tree)
Are you ok with adding empty set_rate function with returning error message that set rate is not supported for fixed clocks?
What would it return that is different? If you are asking for a printed error message, that would bloat the code. So long as the caller checks the error we should be OK.
Caller checks the return code for sure but it is a question if this is enough for people to know what's wrong. When this happen you have no clue that this problem is coming from clock subsystem. I can add one print message to the driver but the same message will end up in all these drivers.
IMO a nice solution to this sort of thing is to improve U-Boot's debug facilities, so you can turn on debugging without having to add #define DEBUG in each file. The problem is that you bloat the code for a case that (once development is done) never happens. In particular a great feature would be something that prints out an error trace, showing where the error is created. E.g.
#ifdef GLOBAL_DEBUG #define ERR_RET(val) ({ printf("%s: %d: Returning error %d\n", __func__, _LINE__, val); return (val); }, ret) #else #define ERR_RET(val) (val) #endif
Then:
if (ret) return ERR_RET(ret)
If we start adding printf() to drivers we carry that load into production code. I think it is easy to print the error in the board and then you can start digging.
Regards, Simon
On 18.11.2016 02:14, Simon Glass wrote:
Hi Michal,
On 16 November 2016 at 00:15, Michal Simek michal.simek@xilinx.com wrote:
Hi Simon,
On 15.11.2016 20:23, Simon Glass wrote:
Hi Michal,
On 15 November 2016 at 12:07, Michal Simek monstr@monstr.eu wrote:
Hi guys,
I just found today with playing with clock drivers that the patch clk: convert API to match reset/mailbox style (sha1: 135aa95002646c46e89de93fa36adad1b010548f)
added this part of code to fixed clock driver -static ulong clk_fixed_rate_get_rate(struct udevice *dev) +static ulong clk_fixed_rate_get_rate(struct clk *clk) {
return to_clk_fixed_rate(dev)->fixed_rate;-}
if (clk->id != 0)return -EINVAL;which is returning -EINVAL when ulong should be returned.
This is intended - you can use IS_ERR_VALUE() to check if it is an error.
ok then some drivers needs to be fixed.
drivers/mmc/atmel_sdhci.c:90: ret = clk_set_rate(&clk, gck_rate); drivers/mmc/atmel_sdhci.c-91- if (ret) drivers/mmc/atmel_sdhci.c-92- return ret;
drivers/mmc/msm_sdhci.c:79: ret = clk_set_rate(&clk, clk_rate); drivers/mmc/msm_sdhci.c-80- clk_free(&clk); drivers/mmc/msm_sdhci.c-81- if (ret < 0)
drivers/net/dwc_eth_qos.c:484: ret = clk_set_rate(&eqos->clk_ptp_ref, 125 * 1000 * 1000); drivers/net/dwc_eth_qos.c-485- if (ret < 0) { drivers/net/dwc_eth_qos.c:486: error("clk_set_rate(clk_ptp_ref) failed: %d", ret); drivers/net/dwc_eth_qos.c-487- goto err_disable_clk_ptp_ref; drivers/net/dwc_eth_qos.c-488- }
drivers/serial/serial_msm.c:175: ret = clk_set_rate(&clk, clk_rate); drivers/serial/serial_msm.c-176- clk_free(&clk); drivers/serial/serial_msm.c-177- if (ret < 0)
drivers/spi/rk_spi.c:176: ret = clk_set_rate(&priv->clk, 99000000); drivers/spi/rk_spi.c-177- if (ret < 0) { drivers/spi/rk_spi.c-178- debug("%s: Failed to set clock: %d\n", __func__, ret);
Yes indeed.
The next thing I have found is that fixed clock driver has no set_rate function which is fine but when I was testing one driver which tries to set rate then error code was generated but without any useful information what happened.
It should return -ENOSYS, right?
I think -EINVAL in driver as reaction for incorrect id looks good. It was more about returning minus value where you should return unsigned.
Yes that's right. I meant -ENOSYS when the driver does not implement the method.
But consider -ENOENT when there is no such thing, as we tend to use -EINVAL for invalid data (e.g. in device tree)
13 #define ENOENT 2 /* No such file or directory */ 33 #define EINVAL 22 /* Invalid argument */ 49 #define ENOSYS 38 /* Function not implemented */
I think ENOSYS is file based on errno.h. ENOENT is not aligned with it based on comment.
Are you ok with adding empty set_rate function with returning error message that set rate is not supported for fixed clocks?
What would it return that is different? If you are asking for a printed error message, that would bloat the code. So long as the caller checks the error we should be OK.
Caller checks the return code for sure but it is a question if this is enough for people to know what's wrong. When this happen you have no clue that this problem is coming from clock subsystem. I can add one print message to the driver but the same message will end up in all these drivers.
IMO a nice solution to this sort of thing is to improve U-Boot's debug facilities, so you can turn on debugging without having to add #define DEBUG in each file. The problem is that you bloat the code for a case that (once development is done) never happens. In particular a great feature would be something that prints out an error trace, showing where the error is created. E.g.
#ifdef GLOBAL_DEBUG #define ERR_RET(val) ({ printf("%s: %d: Returning error %d\n", __func__, _LINE__, val); return (val); }, ret) #else #define ERR_RET(val) (val) #endif
Then:
if (ret) return ERR_RET(ret)
If we start adding printf() to drivers we carry that load into production code. I think it is easy to print the error in the board and then you can start digging.
Probably this message should be enough to identify a place where error starts.
Maybe that macro should be extended with reading variable from environment and then print to have more dynamic case. Because I expect that there is a code which should return error but it is not a problem.
Thanks, Michal
participants (3)
-
Michal Simek -
Michal Simek -
Simon Glass