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

[Bjorn Andersson]

On Thu 28 Apr 09:24 PDT 2022, Dmitry Baryshkov wrote:

> On Thu, 28 Apr 2022 at 18:59, Bjorn Andersson
> <bjorn.andersson@xxxxxxxxxx> wrote:
> >
> > 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.
> 
> In this case I think we should go for clock-name in the DT and
> auto-flled indices inside. Stephen, WDYT? Would that fix your concern
> for comparing strings each and every time?
> 

You mean, just continue doing what we've been doing lately with fw_name
etc?

That lookup is the one that Stephen wants to avoid.

Regards,
Bjorn