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

[Bjorn Andersson]

On Thu 28 Apr 08:44 PDT 2022, Dmitry Baryshkov wrote:

> 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

BI_TCXO_DT is not the value, its the index of the entry in the clocks
array. And the actual values of the clock controller's clocks
property is not a property of the clock controller, but the system
definition.

I.e. that should be clear and explicitly expressed in the dts.

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

Let's not invent some magical syntax for describing the clocks in the
DT.

These macros can't expand to sparse arrays anyways, so iiuc this would
give a sense that the ordering might not be significant, when it really
is.

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

I'm pretty sure that's what clk_core_fill_parent_index() already does.

Regards,
Bjorn