Re: [PATCH] clk: introduce u-boot,ignore-clk-defaults

[Sean Anderson <seanga2@gmail.com>]

On 10/14/21 10:19 PM, Simon Glass wrote:
> Hi Peng, Sean,
> 
> On Thu, 14 Oct 2021 at 19:17, Peng Fan <peng.fan@nxp.com> wrote:
>>
>>> Subject: Re: [PATCH] clk: introduce u-boot,ignore-clk-defaults
>>>
>>>
>>> On 10/13/21 5:37 AM, Peng Fan (OSS) wrote:
>>>> From: Peng Fan <peng.fan@nxp.com>
>>>>
>>>> Current code has a force clk_set_defaults in multiple stages, U-Boot
>>>> reuse the same device tree and Linux Kernel device tree, but we not
>>>> register all the clks as Linux Kernel, so clk_set_defaults will fail
>>>> and cause the clk provider registeration fail.
>>>>
>>>> So introduce a new property to ignore the default settings which could
>>>> be used by any node that wanna ignore default settings.
>>>>
>>>> Signed-off-by: Peng Fan <peng.fan@nxp.com>
>>>> ---
>>>>    doc/device-tree-bindings/device.txt | 3 +++
>>>>    drivers/clk/clk-uclass.c            | 3 +++
>>>>    2 files changed, 6 insertions(+)
>>>>
>>>> diff --git a/doc/device-tree-bindings/device.txt
>>>> b/doc/device-tree-bindings/device.txt
>>>> index 73ce2a3b5b..fe34ced268 100644
>>>> --- a/doc/device-tree-bindings/device.txt
>>>> +++ b/doc/device-tree-bindings/device.txt
>>>> @@ -28,6 +28,9 @@ the acpi,compatible property.
>>>>        Linux will only load the driver if the device can be detected (e.g. on
>>> I2C
>>>>        bus). Note that this is an out-of-tree Linux feature.
>>>>
>>>> +Common device bindings that could be shared listed below:
>>>> + - u-boot,ignore-clk-defaults : ignore the assigned-clock-parents
>>>> +   and assigned-clock-rates for a device that has the property.
>>>>
>>>>    Example
>>>>    -------
>>>> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c index
>>>> 493018b33e..6bf3179e7b 100644
>>>> --- a/drivers/clk/clk-uclass.c
>>>> +++ b/drivers/clk/clk-uclass.c
>>>> @@ -376,6 +376,9 @@ int clk_set_defaults(struct udevice *dev, enum
>>> clk_defaults_stage stage)
>>>>      if (!dev_has_ofnode(dev))
>>>>              return 0;
>>>>
>>>> +   if (ofnode_get_property(dev_ofnode(dev), "u-boot,ignore-clk-defaults",
>>> NULL))
>>>> +           return 0;
>>>> +
>>>>      /*
>>>>       * To avoid setting defaults twice, don't set them before relocation.
>>>>       * However, still set them for SPL. And still set them if
>>>> explicitly
>>>>
>>>
>>> Why not just have the property ignore errors?
>>
>> I think the force err return was done by Simon?
>>
>>>
>>> In the long term, it may be better to standardize that e.g. ENOENT means that
>>> the clock doesn't exist. That way we can skip setting the defaults.
>>> ENOSYS should probably be treated the same way (warn, but don't fail).
>>
>> I am not sure whether people expect force error for ENOENT/ENOSYS in U-Boot.
>> For i.MX, I not expect force error.
> 
> Yes that is me, indeed. It's just that we should not silently ignore
> errors. If we know the clock is optional, then the driver that knows
> that can handle it. But if we start having things quietly fail,
> debugging becomes a pain.

Can't we have them loudly fail instead?

--Sean