Re: [PATCH v2 1/2] dt-bindings: clock: Add Qualcomm SC8280XP GCC bindings

[Dmitry Baryshkov]

On 26/04/2022 01:34, Stephen Boyd wrote:
> Quoting Bjorn Andersson (2022-04-22 20:43:18)
> > On Fri 22 Apr 20:13 PDT 2022, Stephen Boyd wrote:
> > > I'd really rather not have clock-names at all because we spend a bunch
> > > of time comparing strings with them when we could just as easily use
> > > a number.
> >
> > I know that you would like to get rid of the clock-names for the clock
> > controllers. I've looked at it since and while it will be faster to
> > execute I still feel that it's going to be harder to write and maintain.
> >
> > E.g. look at gcc_pcie_4_pipe_clk_src, its parents today are
> > pcie_4_pipe_clk and bi_tcxo. Something I can reason about being correct
> > or not.
> >
> > If we ditch the clock-names I will have:
> >
> > static const struct clk_parent_data gcc_parent_data_14[] = {
> >          { .index = 30 },
> >          { .index = 0 },
>
> Those numbers could have some #define.
>
> 	{ .index = PCIE_4_PIPE_CLK_DT }
> 	{ .index = BI_TCXO_DT }
>
> > };
> >
> > Generally we would perhaps use some compile time constant, but that
> > won't work here because we're talking about the index in the clocks
> > array in the yaml.
> >
> >
> > But perhaps I'm missing something that would make this manageable?
>
> I dunno. Maybe a macro in the dt-binding header could be used to specify
> the 'clocks' property of the DT node that is providing the other side?
> The idea is to make a bunch of macros that insert the arguments of the
> macro in the right place for the clocks property and then define the
> order of arguments otherwise. It would be similar to how
> CREATE_TRACE_POINTS is used in include/trace/define_trace.h
>
> In the dt-bindings/qcom,gcc-soc.h file:
>
> 	#ifdef IN_DTSI
>
> 	#undef GCC_DT_NODE_CLOCKS
> 	#define GCC_DT_NODE_CLOCKS
> 		clocks = <BI_TCXO_DT>,
> 			 <SLEEP_CLK_DT>;
>
> 	#endif /* IN_DTSI */
>
> 	#define BI_TCXO_DT 0
> 	#define SLEEP_CLK_DT 1

Isn't this being an overkill, to define exact properties in the bindings 
header? Also this would mean that we'd have to add dt-binding headers 
for all _consumers_ of clocks. And to make things more complex, e.g. for 
PCIe devices different instances of the device would use different 
amount of clocks. This would mean that we'd have to define 
SM8250_PCI0_CLOCKS, SM8250_PCIE1_CLOCKS and SM8250_PCIE2_CLOCKS.


If we were to switch to this fragile path of using indices (yes I 
consider it to be very fragile), I'd consider something like the 
following to work in the platform dtsi file:

clocks =
BEGIN_CLOCK
CLOCK(BI_TCXO_DT, &bi_tcxo)
CLOCK(SLEEP_CLK_DT, &sleep_clk)
END_CLOCK;

While the following should give an error:
clocks =
BEGIN_CLOCK
CLOCK(SLEEP_CLK_DT, &sleep_clk)
CLOCK(BI_TCXO_DT, &bi_tcxo)
END_CLOCK;

I think we can make this error out by using some additional tool (or 
additional preprocessor pass over the sources)

> And then in the SoC.dtsi file have
>
> 	#define IN_DTSI
> 	#include <dt-bindings/qcom,gcc-soc.h>
>
> 	#define BI_TCXO_DT	&xo_board
> 	#define SLEEP_CLK_DT	&sleep_clk
>
> 	...
>
> 	clock-controller@a000000 {
> 		compatible = "qcom,gcc-soc";
> 		reg = <0xa000000 0x10000>;
> 		GCC_DT_NODE_CLOCKS
> 	};
>
>
> and then in drivers/clk/qcom/gcc-soc.c file:
>
> 	#include <dt-bindings/qcom,gcc-soc.h>
>
> 	static const struct clk_parent_data gcc_parent_data_14[] = {
> 		{ .index = PCIE_4_PIPE_CLK_DT },
> 		{ .index = BI_TCXO_DT },
> 	};
>
> The benefit I see to this is that the index for each clock is in the
> header file (BI_TCXO_DT is 0) and it's next to the clocks property.
> Someone could still mess up the index based on where the macro is used
> in the clocks property though.

And actually might I suggest an alternative approach to manually using 
indices everywhere? What about spending the time once during the boot to 
convert .fw_name and clock_names to parent indices during clock 
registration and then using them for all the further operations?

--
With best wishes
Dmitry