[U-Boot] Fixed clock driver (clk: convert API to match reset/mailbox style)

[Michal Simek]

On 18.11.2016 02:14, Simon Glass wrote:
> Hi Michal,
> 
> On 16 November 2016 at 00:15, Michal Simek <michal.simek at 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 at 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