[PATCH V2] drivers: clk: zynqmp: remove clock name dependency

[Trivedi Manojbhai, Naman]

Hi Stephen,

While debugging further, I found that, CCF registers the clock by their node name if the clock is present in DT. This was causing issue when the clock node name is changed. By adding clock-output-names property in the clock provider nodes in DT, it ensures output clock name is not changed. This DT change is sufficient to fix the issue.

I have submitted a patch - https://lkml.org/lkml/2024/11/21/412 to fix the same in DT.  So, the driver changes which are part of this thread is not required. 

Thanks for review!

Regards,
Naman


>-----Original Message-----
>From: Stephen Boyd <sboyd at kernel.org>
>Sent: 30 August 2024 00:33
>To: Simek, Michal <michal.simek at amd.com>; Thangaraj, Senthil Nathan
><SenthilNathan.Thangaraj at amd.com>; Trivedi Manojbhai, Naman
><Naman.TrivediManojbhai at amd.com>; linux-arm-kernel at lists.infradead.org;
>linux-clk at vger.kernel.org; mturquette at baylibre.com
>Cc: linux-kernel@ <"vger.kernel.org linux-kernel"@vger.kernel.org>
>Subject: RE: [PATCH V2] drivers: clk: zynqmp: remove clock name dependency
>
>Caution: This message originated from an External Source. Use proper caution
>when opening attachments, clicking links, or responding.
>
>
>Quoting Trivedi Manojbhai, Naman (2024-08-28 22:54:16)
>> >> >> +566,30 @@ static int zynqmp_get_parent_list(struct device_node
>> >> >> +*np,
>> >> >> u32 clk_id,
>> >> >>
>> >> >>         for (i = 0; i < total_parents; i++) {
>> >> >>                 if (!parents[i].flag) {
>> >> >> -                       parent_list[i] = parents[i].name;
>> >> >> +                       ret = of_property_match_string(np,
>> >> >> + "clock-names",
>> >> >> +
>> >> >> + parents[i].name);
>> >> >
>> >> >You shouldn't need to match 'clock-names'. The order of that
>> >> >property is fixed in the binding, which means you can simply use
>> >> >the index that the name is at in the binding in 'struct parent_data'.
>> >>
>> >> This driver is common across multiple device families, and each
>> >> device has
>> >different set of clock names in device tree/binding.  This
>> >implementation seemed to be generic for all devices.
>> >> To use index directly, I have to add if..else for matching
>> >> compatible strings
>> >and more if..else inside each of them for matching clock names to find
>index.
>> >Please let me know if this is preferred approach.
>> >
>> >It is preferred to not use clock-names and use the index directly.
>> >This avoids a bunch of string comparisons and makes for smaller and faster
>code.
>>
>> Agreed, using the "clock-names" adds string comparisons, however, two
>reasons why I think comparing with 'clock-names' is necessary.
>>
>> First, the clock driver is common for multiple platforms. And all platforms
>have their unique DT binding.
>> So, clock name to DT index mapping is platform specific. Which means the
>driver will have to hardcode the clock name to index mapping for each
>platform.
>
>The clock name to DT index mapping must be described in the binding.
>
>>
>> Second, clock parents information is received from firmware. The parents of a
>clock may or may not be present in the DT node 'clock-controller' as many
>clocks have "intermediate" clocks as parent.
>> We don't have DT index for intermediate clocks so need to register by
>fw_name.
>
>If there isn't a DT index for those intermediate clks then they should be using a
>clk_hw pointer directly. The fw_name member matches an index in the clock-
>names property, which has a 1:1 relationship to the DT index.
>
>> For example, below debug prints show parents of "spi1_ref" clock. It doesn't
>have any parent which is in DT.
>> clkname: spi1_ref : parent 0: ppll_to_xpd
>> clkname: spi1_ref : parent 1: npll_to_xpd
>> clkname: spi1_ref : parent 2: flxpll
>> clkname: spi1_ref : parent 3: rpll
>> So, here we need to check if the parent is in the DT, and if not, register by
>fw_name.
>
>If the parent isn't in DT then it can't use fw_name. There seems to be some
>misunderstanding.
>
>>
>> The zynqmp_get_parent_list function iterates over the parent list for each
>clock, check if the parent clock is present in the DT node under 'clock-names'
>property. If so, the driver registers clock by index, else register by fw_name.
>>
>> Because of this reason I believe the comparison with "clock-names" is
>unavoidable. Please share your inputs.
>>
>
>I think what you're saying is that zynqmp_get_clock_info() is building a
>topology description for clks that the driver registers. But some of those clks
>have parents that aren't registered by this driver and are external to the device.
>The zynqmp firmware interface is string based, not number based, so you have
>to keep "clock-names" DT property.
>
>That's all OK.
>
>You don't need to parse clock-names for the DT index if you take the string
>from the zynqmp firmware and jam it into the fw_name when the parent isn't
>a clk registered by this device. When the parent _is_ a clk registered by this
>device, you should use a clk_hw pointer for that other clk to indicate the
>parent. When it's a mixture of internal and external you use struct
>clk_parent_data. When it's only internal, use struct clk_init_data::clk_hws to
>avoid even considering having to parse DT.
>
>If you want to skip the string comparisons in the clk core, and I suggest you do
>so, you can parse clock-names once, find the index for each external clk, and
>then use that info when building the topology returned from the zynqmp
>firmware to mark the parent as "external". Then when you go to register all the
>clks you can use that info to build the parent_data or parent_hws structures.
>It's unfortunate that the zynqmp firmware interface is string based, but I
>understand it can't be changed.
>
>I looked at zynqmp_pm_clock_get_parents() and it looks like it's just a bunch of
>numbers for the parent descriptions. If that's true, then there aren't any
>external clks at all, and so clock-names and clocks shouldn't be in the DT
>binding and you should just use clk_hw pointers everywhere.
>Did I miss something?