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

[Dmitry Baryshkov]

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?


-- 
With best wishes
Dmitry