| Message ID | 20190405000129.19331-3-drvlabo@gmail.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | MIPS: ralink: peripheral clock gating driver | expand |
Quoting NOGUCHI Hiroshi (2019-04-04 17:01:26) > Signed-off-by: NOGUCHI Hiroshi <drvlabo@gmail.com> > --- > .../bindings/clock/ralink,rt2880-clock.txt | 58 +++++++++++++++++++ > 1 file changed, 58 insertions(+) > create mode 100644 Documentation/devicetree/bindings/clock/ralink,rt2880-clock.txt > > diff --git a/Documentation/devicetree/bindings/clock/ralink,rt2880-clock.txt b/Documentation/devicetree/bindings/clock/ralink,rt2880-clock.txt > new file mode 100644 > index 000000000000..2fc0d622e01e > --- /dev/null > +++ b/Documentation/devicetree/bindings/clock/ralink,rt2880-clock.txt > @@ -0,0 +1,58 @@ > +* Clock bindings for Ralink/Mediatek MIPS based SoCs > + > +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? > + 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. > + > +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"; > +
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. >> + >> +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.
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.
diff --git a/Documentation/devicetree/bindings/clock/ralink,rt2880-clock.txt b/Documentation/devicetree/bindings/clock/ralink,rt2880-clock.txt new file mode 100644 index 000000000000..2fc0d622e01e --- /dev/null +++ b/Documentation/devicetree/bindings/clock/ralink,rt2880-clock.txt @@ -0,0 +1,58 @@ +* Clock bindings for Ralink/Mediatek MIPS based SoCs + +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. + 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>; +}; + +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>; + clock-names = "uart0"; + + ... +};
Signed-off-by: NOGUCHI Hiroshi <drvlabo@gmail.com> --- .../bindings/clock/ralink,rt2880-clock.txt | 58 +++++++++++++++++++ 1 file changed, 58 insertions(+) create mode 100644 Documentation/devicetree/bindings/clock/ralink,rt2880-clock.txt