[U-Boot] [PATCH] core: do not fail in device_probe() when clk set default fail

[Simon Glass]

Hi,

On 26 February 2018 at 10:01, Kever Yang <kever.yang at rock-chips.com> wrote:
>
>
> On 02/25/2018 06:52 PM, Dr. Philipp Tomsich wrote:
>>> On 25 Feb 2018, at 11:24, Kever Yang <kever.yang at rock-chips.com> wrote:
>>>
>>> Assigned clocks are widely used in kernel, but not in U-Boot yet,
>>> many U-Boot clock driver do not have the API while dts port from kernel
>>> have "assigned-clocks" node.
>>>
>>> Just give a warning now instead of a device probe fail.
>> I strongly disagree on this one: the only reason this can fail is if assigned-clock-rates
>> can not be set (e.g. because a clock that should be assigned to is not known by the
>> clock driver).  However, this should never be ignored silently.
>
> Whe we search "assigned-clocks" in dts, you can see a lot of result in
> different platform,
> just as I mentioned in commit message because this is widely used in kernel.
> In other hand, when we search "set_parent" in clock driver, only part of
> Rockchip SoCs
> support it, and only GMAC support this, everything else will get an
> error return and
> failed in device_probe() which means driver been broken.
>
> I think it's more reasonable to like pinctrl, we can try to set it, but
> not fail on error return,
> or else too many modules been break before they are ready to support the
> new feature.
>
> Or maybe we should not return error if ops->set_parent is empty.
>
>>
>> If the clock subsystem does not know all clocks that are being attempted to set, then
>> commits to shared drivers will eventually break: e.g. commit ba1f96672522
>> ("net: designware: add clock support”) recently broke the GMAC for the RK3368 and
>> RK3399 because it iterated over all clocks defined in the “clocks” property of the
>> GMAC node.
>>
>> As long as various drivers perform an unconditional clk_enable and/or an unconditional
>> clk_set_rate, we should enforce this in the core already.  In consequence, dedicated
>> code from the drivers should now start to slowly disappear, as clock-rates can now be
>> set via the DTS.
>
> This is an option in kernel for a long time, but not mandatory, so we should
> make both(the new feature and drivers already there) work, or clean and
> replace
> everything in driver already there before enforce to use the new feature
> in core.
>
> Thanks,
> - Kever
>>
>>> Signed-off-by: Kever Yang <kever.yang at rock-chips.com>
>>> ---
>>>
>>> drivers/core/device.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/core/device.c b/drivers/core/device.c
>>> index 9d58f44..71872e9 100644
>>> --- a/drivers/core/device.c
>>> +++ b/drivers/core/device.c
>>> @@ -407,7 +407,7 @@ int device_probe(struct udevice *dev)
>>>      /* Process 'assigned-{clocks/clock-parents/clock-rates}' properties */
>>>      ret = clk_set_defaults(dev);
>>>      if (ret)
>>> -            goto fail;
>>> +            debug("%s clk_set_defaults failed %d\n", ret);
>> This probably shouldn’t have been a silent failure.
>> A pr_err() may be more appropriate… I would recommend this to continue failing
>> though, as it is simple enough to handle the additional clocks in the clock drivers
>> and create a permanent record of “things not needing additional changes, due to
>> the BROM already having set up things” by returning success.
>>
>>>      if (drv->probe) {
>>>              ret = drv->probe(dev);
>>> --
>>> 1.9.1
>>>

What is the resolution here? Can we use the error code to tell us
whether the error should be ignored or not? I am not keen on ignoring
all errors, which this code seems to do.

Also the debug() call introduces a compile error.

Regards,
Simon