Re: [PATCH v2 2/4] doc: dt: add documentation for lpc1850-cgu clk driver

Michael Turquette
2015-05-21T21:17:27Z

Quoting Joachim Eastwood (2015-05-13 00:57:18)
> On 13 May 2015 at 00:12, Michael Turquette <mturquette@linaro.org> wrote:
> > Quoting Joachim Eastwood (2015-04-27 14:38:11)
> >> Add DT binding documentation for lpc1850-cgu driver.
> >>
> >> Signed-off-by: Joachim Eastwood <manabian@gmail.com>
> >> ---
> >> .../devicetree/bindings/clock/lpc1850-cgu.txt | 138 ++++++++++++=
+++++++++
> >> 1 file changed, 138 insertions(+)
> >> create mode 100644 Documentation/devicetree/bindings/clock/lpc1850-cg=
u.txt
> >>
> >> diff --git a/Documentation/devicetree/bindings/clock/lpc1850-cgu.txt b=
/Documentation/devicetree/bindings/clock/lpc1850-cgu.txt
> >> new file mode 100644
> >> index 000000000000..0b278ca6aee7
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/clock/lpc1850-cgu.txt
> >> @@ -0,0 +1,138 @@
> >> +* NXP LPC1850 Clock Generation Unit (CGU)
> >> +
> >> +The CGU generates multiple independent clocks for the core and the
> >> +peripheral blocks of the LPC18xx. Each independent clock is called
> >> +a base clock and itself is one of the inputs to the two Clock
> >> +Control Units (CCUs) which control the branch clocks to the
> >> +individual peripherals.
> >> +
> >> +The CGU selects the inputs to the clock generators from multiple
> >> +clock sources, controls the clock generation, and routes the outputs
> >> +of the clock generators through the clock source bus to the output
> >> +stages. Each output stage provides an independent clock source and
> >> +corresponds to one of the base clocks for the LPC18xx.
> >> +
> >> + - Above text taken from NXP LPC1850 User Manual.
> >> +
> >> +
> >> +This binding uses the common clock binding:
> >> + Documentation/devicetree/bindings/clock/clock-bindings.txt
> >> +
> >> +Required properties:
> >> +- compatible:
> >> + Should be "nxp,lpc1850-cgu"
> >> +- reg:
> >> + Shall define the base and range of the address space
> >> + containing clock control registers
> >> +- #clock-cells:
> >> + Shall have value <1>. The permitted clock-specifier values
> >> + are the base clock numbers defined below.
> >> +- clocks:
> >> + Shall contain a list of phandles for the external input
> >> + sources to the CGU. The list shall be in the following
> >> + order: xtal, 32khz, enet_rx_clk, enet_tx_clk, gp_clkin.
> >> +- clock-indices:
> >> + Shall be an ordered list of numbers defining the base clock
> >> + number provided by the CGU.
> >> +- clock-output-names:
> >> + Shall be an ordered list of strings defining the names of
> >> + the clocks provided by the CGU.
> >> +
> >> +Which base clocks that are available on the CGU depends on the
> >> +specific LPC part. Base cloks are numbered from 0 to 27.
> >
> > s/cloks/clocks/
> >
> >> +
> >> +Number: Name: Description:
> >> + 0 BASE_SAFE_CLK Base safe clock (always on) fo=
r WWDT
> >> + 1 BASE_USB0_CLK Base clock for USB0
> >> + 2 BASE_PERIPH_CLK Base clock for Cortex-M0SUB su=
bsystem,
> >> + SPI, and SGPIO
> >> + 3 BASE_USB1_CLK Base clock for USB1
> >> + 4 BASE_CPU_CLK System base clock for ARM Cort=
ex-M core
> >> + and APB peripheral blocks #0 a=
nd #2
> >> + 5 BASE_SPIFI_CLK Base clock for SPIFI
> >> + 6 BASE_SPI_CLK Base clock for SPI
> >> + 7 BASE_PHY_RX_CLK Base clock for Ethernet PHY Re=
ceive clock
> >> + 8 BASE_PHY_TX_CLK Base clock for Ethernet PHY Tr=
ansmit clock
> >> + 9 BASE_APB1_CLK Base clock for APB peripheral =
block # 1
> >> +10 BASE_APB3_CLK Base clock for APB peripheral =
block # 3
> >> +11 BASE_LCD_CLK Base clock for LCD
> >> +12 BASE_ADCHS_CLK Base clock for ADCHS
> >> +13 BASE_SDIO_CLK Base clock for SD/MMC
> >> +14 BASE_SSP0_CLK Base clock for SSP0
> >> +15 BASE_SSP1_CLK Base clock for SSP1
> >> +16 BASE_UART0_CLK Base clock for UART0
> >> +17 BASE_UART1_CLK Base clock for UART1
> >> +18 BASE_UART2_CLK Base clock for UART2
> >> +19 BASE_UART3_CLK Base clock for UART3
> >> +20 BASE_OUT_CLK Base clock for CLKOUT pin
> >> +24-21 - Reserved
> >> +25 BASE_AUDIO_CLK Base clock for audio system (I=
2S)
> >> +26 BASE_CGU_OUT0_CLK Base clock for CGU_OUT0 clock =
output
> >> +27 BASE_CGU_OUT1_CLK Base clock for CGU_OUT1 clock =
output
> >> +
> >> +BASE_PERIPH_CLK and BASE_SPI_CLK is only available on LPC43xx.
> >> +BASE_ADCHS_CLK is only available on LPC4370.
> >> +
> >> +
> >> +Example board file:
> >> +
> >> +/ {
> >> + clocks {
> >> + xtal: xtal {
> >> + compatible =3D "fixed-clock";
> >> + #clock-cells =3D <0>;
> >> + clock-frequency =3D <12000000>;
> >> + };
> >> +
> >> + xtal32: xtal32 {
> >> + compatible =3D "fixed-clock";
> >> + #clock-cells =3D <0>;
> >> + clock-frequency =3D <32768>;
> >> + };
> >> +
> >> + enet_rx_clk: enet_rx_clk {
> >> + compatible =3D "fixed-clock";
> >> + #clock-cells =3D <0>;
> >> + clock-frequency =3D <0>;
> >> + clock-output-names =3D "enet_rx_clk";
> >> + };
> >> +
> >> + enet_tx_clk: enet_tx_clk {
> >> + compatible =3D "fixed-clock";
> >> + #clock-cells =3D <0>;
> >> + clock-frequency =3D <0>;
> >> + clock-output-names =3D "enet_tx_clk";
> >> + };
> >> +
> >> + gp_clkin: gp_clkin {
> >> + compatible =3D "fixed-clock";
> >> + #clock-cells =3D <0>;
> >> + clock-frequency =3D <0>;
> >> + clock-output-names =3D "gp_clkin";
> >> + };
> >> + };
> >> +
> >> + soc {
> >> + cgu: cgu@40050000 {
> >> + compatible =3D "nxp,lpc1850-cgu";
> >> + reg =3D <0x40050000 0x1000>;
> >> + #clock-cells =3D <1>;
> >> + clocks =3D <&xtal>, <&creg_clk 1>, <&enet_rx_c=
lk>, <&enet_tx_clk>, <&gp_clkin>;
> >> + clock-indices =3D <0>, <1>, <2>, <3>, <4>=
, <5>, <6>, <7>,
> >> + <8>, <9>, <10>, <11>, <12>, =
<13>, <14>, <15>,
> >> + <16>, <17>, <18>, <19>, <20>, =
<25>, <26>, <27>;
> >> + clock-output-names =3D "base_safe_clk", "ba=
se_usb0_clk",
> >> + "base_periph_clk", "base=
_usb1_clk",
> >> + "base_cpu_clk", "base=
_spifi_clk",
> >> + "base_spi_clk", "base=
_phy_rx_clk",
> >> + "base_phy_tx_clk", "base=
_apb1_clk",
> >> + "base_apb3_clk", "base=
_lcd_clk",
> >> + "base_adchs_clk", "base=
_sdio_clk",
> >> + "base_ssp0_clk", "base=
_ssp1_clk",
> >> + "base_uart0_clk", "base=
_uart1_clk",
> >> + "base_uart2_clk", "base=
_uart3_clk",
> >> + "base_out_clk", "base=
_audio_clk",
> >> + "base_cgu_out0_clk","base=
_cgu_out1_clk";
> >
> > Why do you need to use clock-indices?
> =

> Since the CGU can have up to 27 clock lines, but in this device not
> all are used. So I use clock-indices so I don't need to have empty
> entries in the clock-output-names array. I thought this was the
> intended usage(?)
> =

> > Why do you need to use clock-output-names? If all of your clock
> > consumers have nodes in DT then you can skip this and name them on the
> > consuming side. See a further explanation here:
> > http://lkml.kernel.org/r/<20150416192014.19585.9663@quantum>
> =

> Most of the clocks from the CGU goes to a CCU, but the CCU isn't the
> real consumer. It's more like a clock router with a bunch of gates.
> =

> If you take a close look at the CCU driver you will notice that it
> doesn't do clk_get on any of the clocks. My reason for doing this is
> to not increase the usage counter on the CGU clock so that if there
> are no consumer after a CCU the entire clock line up to the CGU can be
> disabled. So this is the reason that the CCU driver uses a "special"
> way to grab the clock output names from the CGU.

A call to clk_get will not enable/ungate the clock signal. It seems that
there might be some confusion around that. It is always OK to get the
clk. This will not increase the enable_count or prepare_count values.

If the CCU is a separate IP block then it might make sense for it to
call clk_get on the CGU clocks. On the other hand it might make more
sense for the CCU clocks to simply use the CGU clocks as parent clock
signal inputs.

None of the points above have anything to do with my original comment.
The use of the clock-output-names property is unnecessary if all of the
consumer devices have DT nodes of their own. Please look at the thread I
linked to and let me know what you think about getting rid of the
clock-output-names property.

> =

> Hope this makes sense.
> =

> > Also note that providing a clock consumer node in the examples section
> > of the binding is helpful when reviewing.
> =

> I'll add a consumer example in the next version.

Great.

Thanks,
Mike

> =

> regards,
> Joachim Eastwood