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
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)
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?
--
With best wishes
Dmitry
