[PATCH 1/1] clk: introduce of_clk_detect_critical helper

[Quentin Schulz]

Hi Maxim,

On 8/12/25 8:56 PM, Maxim Kochetkov wrote:
> [You don't often get email from fido_max at inbox.ru. Learn why this is 
> important at https://aka.ms/LearnAboutSenderIdentification ]
> 
> 12.08.2025 19:41, Christian Marangi wrote:
>> On Tue, Aug 12, 2025 at 07:29:46PM +0300, Maxim Kochetkov wrote:
>>> This helper find clock-critiacl property in dts and assign
>>
>> clock-critiacl -> typo
> 
> 
> Thanks. Will fix it in v2
> 
>>
>>> CLK_IS_CRITICAL flag to the clock.
>>>
>>> Signed-off-by: Maxim Kochetkov <fido_max at inbox.ru>
>>> ---
>>>   drivers/clk/clk-uclass.c | 17 +++++++++++++++++
>>>   drivers/core/of_access.c | 22 ++++++++++++++++++++++
>>>   include/clk.h            | 28 ++++++++++++++++++++++++++++
>>>   include/dm/of_access.h   | 17 +++++++++++++++++
>>>   4 files changed, 84 insertions(+)
>>>
>>> diff --git a/drivers/clk/clk-uclass.c b/drivers/clk/clk-uclass.c
>>> index 7262e89b512..4f35b90382f 100644
>>> --- a/drivers/clk/clk-uclass.c
>>> +++ b/drivers/clk/clk-uclass.c
>>> @@ -835,6 +835,23 @@ struct clk *devm_clk_get(struct udevice *dev, 
>>> const char *id)
>>>      return clk;
>>>   }
>>>
>>> +int of_clk_detect_critical(struct device_node *np, int index,
>>> +                       unsigned long *flags)
>>> +{
>>> +    struct property *prop;
>>> +    const __be32 *cur;
>>> +    uint32_t idx;
>>> +
>>> +    if (!np || !flags)
>>> +            return -EINVAL;
>>> +
>>> +    of_property_for_each_u32(np, "clock-critical", prop, cur, idx)
>>> +            if (index == idx)
>>> +                    *flags |= CLK_IS_CRITICAL;
>>> +
>>> +    return 0;
>>> +}
>>> +
>>>   int clk_uclass_post_probe(struct udevice *dev)
>>>   {
>>>      /*
>>> diff --git a/drivers/core/of_access.c b/drivers/core/of_access.c
>>> index b11e36202c1..59dda3ae152 100644
>>> --- a/drivers/core/of_access.c
>>> +++ b/drivers/core/of_access.c
>>> @@ -666,6 +666,28 @@ int of_property_read_string_helper(const struct 
>>> device_node *np,
>>>      return i <= 0 ? -ENODATA : i;
>>>   }
>>>
>>> +const __be32 *of_prop_next_u32(struct property *prop, const __be32 
>>> *cur,
>>> +                           u32 *pu)
>>> +{
>>> +    const void *curv = cur;
>>> +
>>> +    if (!prop)
>>> +            return NULL;
>>> +
>>> +    if (!cur) {
>>> +            curv = prop->value;
>>> +            goto out_val;
>>> +    }
>>> +
>>> +    curv += sizeof(*cur);
>>> +    if (curv >= prop->value + prop->length)
>>> +            return NULL;
>>> +
>>> +out_val:
>>> +    *pu = be32_to_cpup(curv);
>>> +    return curv;
>>> +}
>>> +
>>>   static int __of_root_parse_phandle_with_args(struct device_node *root,
>>>                                           const struct device_node *np,
>>>                                           const char *list_name,
>>> diff --git a/include/clk.h b/include/clk.h
>>> index f94135ff778..ea708f54fd5 100644
>>> --- a/include/clk.h
>>> +++ b/include/clk.h
>>> @@ -589,6 +589,27 @@ bool clk_dev_binded(struct clk *clk);
>>>    */
>>>   ulong clk_get_id(const struct clk *clk);
>>>
>>> +/**
>>> + * of_clk_detect_critical() - set CLK_IS_CRITICAL flag from Device Tree
>>> + * @np: Device node pointer associated with clock provider
>>> + * @index: clock index
>>> + * @flags: pointer to top-level framework flags
>>> + *
>>> + * Detects if the clock-critical property exists and, if so, sets the
>>> + * corresponding CLK_IS_CRITICAL flag.
>>> + *
>>> + * Do not use this function. It exists only for legacy Device Tree
>>> + * bindings, such as the one-clock-per-node style that are outdated.
>>> + * Those bindings typically put all clock data into .dts and the Linux
>>> + * driver has no clock data, thus making it impossible to set this flag
>>> + * correctly from the driver. Only those drivers may call
>>> + * of_clk_detect_critical from their setup functions.
>>> + *
>>> + * Return: error code or zero on success
>>> + */
>>> +int of_clk_detect_critical(struct device_node *np, int index,
>>> +                       unsigned long *flags);
>>> +
>>>   #else /* CONFIG_IS_ENABLED(CLK) */
>>>
>>>   static inline int clk_request(struct udevice *dev, struct clk *clk)
>>> @@ -665,6 +686,13 @@ static inline ulong clk_get_id(const struct clk 
>>> *clk)
>>>   {
>>>      return 0;
>>>   }
>>> +
>>> +int of_clk_detect_critical(struct device_node *np, int index,
>>> +                       unsigned long *flags)
>>> +{
>>> +    return 0;
>>> +}
>>> +
>>>   #endif /* CONFIG_IS_ENABLED(CLK) */
>>>
>>>   /**
>>> diff --git a/include/dm/of_access.h b/include/dm/of_access.h
>>> index 44143a5a391..dba725498f5 100644
>>> --- a/include/dm/of_access.h
>>> +++ b/include/dm/of_access.h
>>> @@ -405,6 +405,17 @@ int of_property_read_string_helper(const struct 
>>> device_node *np,
>>>                                 const char *propname, const char 
>>> **out_strs,
>>>                                 size_t sz, int index);
>>>
>>> +/*
>>> + * struct property *prop;
>>> + * const __be32 *p;
>>> + * u32 u;
>>> + *
>>> + * of_property_for_each_u32(np, "propname", prop, p, u)
>>> + *         printk("U32 value: %x\n", u);
>>> + */
>>> +const __be32 *of_prop_next_u32(struct property *prop, const __be32 
>>> *cur,
>>> +                           u32 *pu);
>>> +
>>>   /**
>>>    * of_property_read_string_index() - Find and read a string from a 
>>> multiple
>>>    * strings property.
>>> @@ -701,4 +712,10 @@ int of_remove_property(struct device_node *np, 
>>> struct property *prop);
>>>    */
>>>   int of_remove_node(struct device_node *to_remove);
>>>
>>> +#define of_property_for_each_u32(np, propname, prop, p, u)  \
>>> +    for (prop = of_find_property(np, propname, NULL),       \
>>> +            p = of_prop_next_u32(prop, NULL, &u);           \
>>> +            p;                                              \
>>> +            p = of_prop_next_u32(prop, p, &u))
>>> +
>>>   #endif
>>> -- 
>>
>> I would split these of additional function to a separate commit also
>> where they come from? They are ported from Linux Kernel?
> 
> Ok, I will split it.
> Yes these function are ported from Linux Kernel.
> 
>>
>> Also would be ideal to have some scenario where it's needed to declare a
>> clock in DT that is critical. Isn't that handled directly by the clock
>> driver internally?
>>
> 
> Yes. Now we can use this property from DT by custom drivers to mark
> critical clocks.
> 

How this is typically handled today is by hardcoding the clock that is 
critical in the SoC clock controller driver directly, c.f. 
https://lore.kernel.org/linux-rockchip/20241214224820.200665-1-heiko@sntech.de/

This is of course then forced-enabled for all boards based on this SoC. 
If that is something that isn't desirable, then we need to properly need 
to represent this clock in the Device Tree. You haven't really explained 
the real life example that would benefit from this. Is there one? What 
is it?

Finally, we do not accept Device Tree properties anymore if they aren't 
part of the Device Tree binding coming from the kernel, the Device Tree 
specification or the dt-schema. So it needs to go through those channels 
first before we can make use of this property here in U-Boot. Our 
ultimate goal is to have the exact same Device Tree for U-Boot than in 
the Linux kernel (and I assume even further than that, anything that 
requires DT use the same one across all SW stack).

Cheers,
Quentin