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

[Trivedi Manojbhai, Naman]

Hi Stephen,

Thanks for detailed explanation. I am trying to understand your suggestions and have queries for a few comments. Please find inline response.

>-----Original Message-----
>From: Stephen Boyd <sboyd@xxxxxxxxxx>
>Sent: Friday, August 30, 2024 12:33 AM
>To: Simek, Michal <michal.simek@xxxxxxx>; Thangaraj, Senthil Nathan
><SenthilNathan.Thangaraj@xxxxxxx>; Trivedi Manojbhai, Naman
><Naman.TrivediManojbhai@xxxxxxx>; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx;
>linux-clk@xxxxxxxxxxxxxxx; mturquette@xxxxxxxxxxxx
>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.

I am having some confusion here to understand your suggestion. In the above comment, you mentioned 

"If the parent isn't in DT then it can't use fw_name"

And here in this suggestion, you mentioned

"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"

These two statements seem to be conflicting to me. Are you suggesting that if the parent clock is not registered by this device, then I can use fw_name directly to register it, no matter if the parent clock is present in the DT? 

In this case I don't need to parse DT at all if I understood correctly.

>
>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.

Ok, if I understood correctly, I could parse clock-names once and build a table which has DT index to clock name mapping. And then use this information while registering the clocks. 
In this approach too, for each parent I need to compare parent name with the string from table. So, string comparison can't be avoided completely but DT parsing need to be done only once. 
Is this correct understanding?

Also, I presume this is alternative approach to the above suggestion.

>
>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?

There are reference clocks in the DT under 'clock-names' property and these clocks can be a parent of some other clock. The current implementation is using clk_hw pointers for all clocks, and the parents are registered by their name. That is causing problem when the clock's node name in the DT is changed.