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
