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

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.

Signed-off-by: Kever Yang <kever.yang@rock-chips.com>
---

 drivers/core/device.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

> On 25 Feb 2018, at 11:24, Kever Yang <kever.yang@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.

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.

> 
> Signed-off-by: Kever Yang <kever.yang@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
>

On 02/25/2018 06:52 PM, Dr. Philipp Tomsich wrote:
>> On 25 Feb 2018, at 11:24, Kever Yang <kever.yang@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@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
>>
>

Hi,

On 26 February 2018 at 10:01, Kever Yang <kever.yang@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@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@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