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

[Sean Anderson <seanga2@gmail.com>]

On 10/25/21 9:23 PM, Sean Anderson wrote:
> On 10/25/21 11:18 AM, Simon Glass wrote:
>> Hi Sean,
>>
>> On Sun, 24 Oct 2021 at 18:13, Sean Anderson <seanga2@gmail.com> wrote:
>>>
>>> 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?
>>>
>>
>> That is how it works today, as I understand it. But some boards want
>> the defaults to be there but not to implement them in U-Boot. This
>> seems fair enough to me. Perhaps we could add something to each node
>> instead, to disable it?
> 
> u-boot,assigned-clock-status = "disabled";

Actually, I think that was Peng's idea in the first place :)

--Sean