Re: [PATCH v2 5/5] clk: implement sync_state support

[Konrad Dybcio]

On 6/18/26 11:12 PM, Brian Masney wrote:
> Hi Ulf,
> 
> On Wed, Jun 17, 2026 at 05:29:51PM +0200, Ulf Hansson wrote:
>> On Wed, Jun 17, 2026 at 5:02 PM Brian Masney <bmasney@xxxxxxxxxx> wrote:
>>> On Wed, Jun 17, 2026 at 04:24:05PM +0200, Ulf Hansson wrote:
>>>> On Tue, Jun 16, 2026 at 11:09 PM Brian Masney <bmasney@xxxxxxxxxx> wrote:
>>>>> @@ -4345,8 +4382,17 @@ __clk_register(struct device *dev, struct device_node *np, struct clk_hw *hw)
>>>>>         core->dev = dev;
>>>>>         clk_pm_runtime_init(core);
>>>>>         core->of_node = np;
>>>>> -       if (dev && dev->driver)
>>>>> +       if (dev && dev->driver) {
>>>>>                 core->owner = dev->driver->owner;
>>>>> +
>>>>> +               /*
>>>>> +                * If a clk provider sets their own sync_state, then it needs to
>>>>> +                * also call clk_sync_state(). dev_set_drv_sync_state() won't
>>>>> +                * overwrite the sync_state callback, and this call will fail
>>>>> +                * with -EBUSY.
>>>>> +                */
>>>>> +               dev_set_drv_sync_state(dev, clk_sync_state);
>>>>
>>>> We have cases where a device node represents a provider for multiple
>>>> types of resources, like clocks, power-domains (genpds), resets, etc,
>>>> as in the qcom case, for example.
>>>>
>>>> For power-domain provider drivers (genpd) we also try to assign the
>>>> ->sync_state() callback, see of_genpd_add_provider_simple() and
>>>> of_genpd_add_provider_simple(). This means the above doesn't play well
>>>> with how genpd behaves, so we need to figure out a way to manage these
>>>> cases.
>>>>
>>>> In this regard, we also have of_genpd_sync_state(), which allows a
>>>> genpd provider driver to explicitly call genpd's sync state function,
>>>> if/when needed.
>>>>
>>>> Unfortunately I am not able to suggest a detailed solution for how to
>>>> move this forward at this point, as it requires some more thinking and
>>>> I am heading for some vacation very soon.
>>>
>>> One approach I initially considered was to make it so that we can have a
>>> list of sync_state callbacks that can be added to. I already did some
>>> work on this, but I didn't think it was worth it for just the QC clk
>>> drivers in isolation, but it would address the concern here.
>>
>> Right, maybe that would work.
>>
>> One more thing to somewhat consider, is the problem I have been trying
>> to address for power-domains providers [1]. At least for genpd
>> providers, we need a more fine grained sync state solution, which also
>> must be able to co-exists with other subsystems sync state support. I
>> am not sure if something like this is needed for clocks too?
>>
>> [1]
>> https://lore.kernel.org/all/20260508123910.114273-1-ulf.hansson@xxxxxxxxxx/
> 
> I am not aware of any clks setup that way but that's not to say they don't
> exist. Stephen, and/or some of the folks from QC would know better.

I'm not sure I'm following, but we do have this bit

qcom_cc_probe()
 -> qcom_cc_really_probe()
    -> gdsc_register()
      -> of_genpd_add_provider_onecell()
        -> dev_set_drv_sync_state()

and this patch calls dev_set_drv_sync_state() again in __clk_register()

Konrad