Re: [RFC v2 2/5] dt-bindings: clk: add document for ralink clock driver

[Stephen Boyd <sboyd@kernel.org>]

Quoting NOGUCHI Hiroshi (2019-05-01 04:33:24)
> 
> 
> On 2019/04/26 4:29, Stephen Boyd wrote:
> >> +Required properties:
> >> + - compatible: must be "ralink,rt2880-clock"
> >> + - #clock-cells: must be 1
> >> + - ralink,sysctl: a phandle to a ralink syscon register region
> >> + - clock-indices: identifying number.
> >> +       These must correspond to the bit number in CLKCFG1.
> > 
> > These look like driver level details that we're putting in the DT so we
> > can compress the number space that consumers use. Is that right? If so,
> > I don't get it. Can we not use this property?
> 
> I understand that the bit numbers in clock gating register are hardware 
> resource informations.
> Therefore, it is not strange that they are described in DT, I think.
> 
> 
> >> +       Clock consumers use one of them as #clock-cells index.
> >> + - clock-output-names: array of gating clocks' names
> >> + - clocks: array of phandles which points the parent clock
> >> +       for gating clocks.
> >> +       If gating clock does not need parent clock linkage,
> >> +       we bind to dummy clock whose frequency is zero.
> >> +
> >> +
> >> +Example:
> >> +
> >> +/* dummy parent clock node */
> >> +dummy_ck: dummy_ck {
> >> +       #clock-cells = <0>;
> >> +       compatible = "fixed-clock";
> >> +       clock-frequency = <0>;
> >> +};
> > 
> > Would this ever exist in practice? If not, please remove from the
> > example so we don't get the wrong idea.
> 
> I referred to arch/arm/boot/dts/.
> omap24xx-clocks.dtsi : defines dummy_ck
> omap2420-clocks.dtsi : refers dummy_ck
> 
> 
> In practice, There is no problem in specifying another existing clock,
> eg MT7620_CLK_PERIPH which is always active.

Ok. Please don't add things that don't exist into the example like dummy
clks. Sometimes people copy the examples directly and this can lead to
errors in the resulting DTBs.

> 
> 
> >> +
> >> +clkctrl: clkctrl {
> >> +       compatible = "ralink,rt2880-clock";
> >> +       #clock-cells = <1>;
> >> +       ralink,sysctl = <&sysc>;
> >> +
> >> +       clock-indices =
> >> +                       <12>,
> >> +                       <16>, <17>, <18>, <19>,
> >> +                       <20>,
> >> +                       <26>;
> >> +       clock-output-names =
> >> +                       "uart0",
> >> +                       "i2c", "i2s", "spi", "uart1",
> >> +                       "uart2",
> >> +                       "pcie0";
> >> +       clocks =
> >> +                       <&pll MT7620_CLK_PERIPH>,
> >> +                       <&pll MT7620_CLK_PERIPH>, <&pll MT7620_CLK_PCMI2S>, <&pll MT7620_CLK_SYS>, <&pll MT7620_CLK_PERIPH>,
> >> +                       <&pll MT7620_CLK_PERIPH>,
> >> +                       <&dummy_ck>;
> >> +       };
> >> +};
> >> +
> >> +/* consumer which refers "uart0" clock */
> >> +uart0: uartlite@c00 {
> >> +       compatible = "ns16550a";
> >> +       reg = <0xc00 0x100>;
> >> +
> >> +       clocks = <&clkctrl 12>;
> > 
> > So 12 matches in indices and then that is really "uart0" clk?
> > 
> >> +       clock-names = "uart0";
> >> +
> 
> That is right.
> rt2880-clock driver is implemented to let clock cell indices match 
> indcies in "clock-indices" property.

Usually the binding has a bunch of #defines for the clks, instead of
using raw integers to indicate which clock it is. Then consumers point
to that clk via <&phandle DEFINE>, similar to your 'clocks' property
above in the clkctrl node. Then a clk provider driver will remap that
DEFINE to a clk_hw structure and the driver contains the register
offsets and bits to twiddle to control the clk.

It looks like here we put those register offsets and bits to twiddle in
DT as clock-indices, and then consumers are supposed to know what
clock-indices to match based on the register bits of the clk? That's a
novel approach that may work here but doesn't really scale. I guess it's
OK, but I'd prefer to see #defines even for the clock-indices like
RT2800_UART0 or RT2800_I2C.

After that, it seems risky to put the details of what bits to twiddle in
DT because it expresses driver level hardware details in a place where
we might not be able to as easily modify or fix those bits if certain
clks don't get tested. If we had only specified the provider/consumer
part of the binding (i.e. the numbers in the clk specifier) we wouldn't
need to worry as much because we could fix those driver details in the
driver.