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

[Stephen Boyd]

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


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.