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

[Sean Anderson]

On 10/25/21 11:18 AM, Simon Glass wrote:
> Hi Sean,
> 
> On Sun, 24 Oct 2021 at 18:13, Sean Anderson <seanga2 at 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 at 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 at 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 at 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";

?

--Sean