Hi,
On 26 February 2018 at 10:01, Kever Yang <kever.y...@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.y...@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.y...@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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
https://lists.denx.de/listinfo/u-boot
