Re: [PATCH 4/4] ARM: sun6i: Enable clock support in the DTSI

From: Emilio López
Date: Tue Jul 30 2013 - 21:37:06 EST

Hi Maxime,

Overall this looks good to me, but I have some small comments:

El 30/07/13 11:44, Maxime Ripard escribió:
> Now that the clock driver has support for the A31 clocks, we can add
> them to the DTSI and start using them in the relevant hardware blocks.
>
> Signed-off-by: Maxime Ripard <maxime.ripard@xxxxxxxxxxxxxxxxxx>
> ---
> arch/arm/boot/dts/sun6i-a31.dtsi | 137 ++++++++++++++++++++++++++++++++++++---
> 1 file changed, 127 insertions(+), 10 deletions(-)
>
> diff --git a/arch/arm/boot/dts/sun6i-a31.dtsi b/arch/arm/boot/dts/sun6i-a31.dtsi
> index 0d13dc6..c6a3a91 100644
> --- a/arch/arm/boot/dts/sun6i-a31.dtsi
> +++ b/arch/arm/boot/dts/sun6i-a31.dtsi
> @@ -51,13 +51,130 @@
>
> clocks {
> #address-cells = <1>;
> - #size-cells = <0>;
> + #size-cells = <1>;
> + ranges;
>
> - osc: oscillator {
> + osc24M: hosc {

Please use osc24M and osc32k instead of hosc and losc, respectively.

> #clock-cells = <0>;
> compatible = "fixed-clock";
> clock-frequency = <24000000>;

Is osc24M not gatable on A31?

> };
> +
> + osc32k: losc {
> + #clock-cells = <0>;
> + compatible = "fixed-clock";
> + clock-frequency = <32768>;
> + };
> +
> + pll1: pll1@01c20000 {
> + #clock-cells = <0>;
> + compatible = "allwinner,sun6i-pll1-clk";
> + reg = <0x01c20000 0x4>;
> + clocks = <&osc24M>;
> + };
> +
> + /*
> + * This is a dummy clock, to be used as placeholder on
> + * other mux clocks when a specific parent clock is not
> + * yet implemented. It should be dropped when the driver
> + * is complete.
> + */
> + pll6: pll6 {
> + #clock-cells = <0>;
> + compatible = "fixed-clock";
> + clock-frequency = <0>;
> + };
> +
> + cpu: cpu@01c20050 {
> + #clock-cells = <0>;
> + compatible = "allwinner,sun4i-cpu-clk";
> + reg = <0x01c20050 0x4>;
> + clocks = <&osc32k>, <&osc24M>, <&pll1>, <&pll1>;

Listing pll1 twice doesn't sound correct, but vendor code seems to
indicate so. A comment to clarify it's not a typo would be good I think.

> + };
> +
> + axi: axi@01c20050 {
> + #clock-cells = <0>;
> + compatible = "allwinner,sun4i-axi-clk";
> + reg = <0x01c20050 0x4>;
> + clocks = <&cpu>;
> + };
> +
> + ahb1_mux: ahb1_mux@01c20054 {
> + #clock-cells = <0>;
> + compatible = "allwinner,sun6i-ahb1-mux-clk";
> + reg = <0x01c20054 0x4>;
> + clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6>;
> + };
> +
> + ahb1: ahb1@01c20054 {
> + #clock-cells = <0>;
> + compatible = "allwinner,sun4i-ahb-clk";
> + reg = <0x01c20054 0x4>;
> + clocks = <&ahb1_mux>;
> + };

Depending on when this lands, I believe these two above could be merged
into one with the refactoring introduced on my patchset.

> +
> + ahb1_gates: ahb1_gates@01c20060 {
> + #clock-cells = <1>;
> + compatible = "allwinner,sun6i-a31-ahb1-gates-clk";
> + reg = <0x01c20060 0x8>;
> + clocks = <&ahb1>;
> + clock-output-names = "ahb1_mipidsi", "ahb1_ss",
> + "ahb1_dma", "ahb1_mmc0", "ahb1_mmc1",
> + "ahb1_mmc2", "ahb1_mmc3", "ahb1_nand1",
> + "ahb1_nand0", "ahb1_sdram",
> + "ahb1_gmac", "ahb1_ts", "ahb1_hstimer",
> + "ahb1_spi0", "ahb1_spi1", "ahb1_spi2",
> + "ahb1_spi3", "ahb1_otg", "ahb1_ehci0",
> + "ahb1_ehci1", "ahb1_ohci0",
> + "ahb1_ohci1", "ahb1_ohci2", "ahb1_ve",
> + "ahb1_lcd0", "ahb1_lcd1", "ahb1_csi",
> + "ahb1_hdmi", "ahb1_de0", "ahb1_de1",
> + "ahb1_fe0", "ahb1_fe1", "ahb1_mp",
> + "ahb1_gpu", "ahb1_deu0", "ahb1_deu1",
> + "ahb1_drc0", "ahb1_drc1";
> + };
> +
> + apb1: apb1@01c20054 {
> + #clock-cells = <0>;
> + compatible = "allwinner,sun4i-apb0-clk";
> + reg = <0x01c20054 0x4>;
> + clocks = <&ahb1>;
> + };
> +
> + apb1_gates: apb1_gates@01c20060 {
> + #clock-cells = <1>;
> + compatible = "allwinner,sun6i-a31-apb1-gates-clk";
> + reg = <0x01c20068 0x4>;
> + clocks = <&apb1>;
> + clock-output-names = "apb1_codec", "apb1_digital_mic",
> + "apb1_pio", "apb1_daudio0",
> + "apb1_daudio1";
> + };
> +
> + apb2_mux: apb2_mux@01c20058 {
> + #clock-cells = <0>;
> + compatible = "allwinner,sun4i-apb1-mux-clk";
> + reg = <0x01c20058 0x4>;
> + clocks = <&osc32k>, <&osc24M>, <&pll6>, <&pll6>;
> + };
> +
> + apb2: apb2@01c20058 {
> + #clock-cells = <0>;
> + compatible = "allwinner,sun6i-apb2-div-clk";
> + reg = <0x01c20058 0x4>;
> + clocks = <&apb2_mux>;
> + };

Ditto for apb2_mux and apb2.

> +
> + apb2_gates: apb2_gates@01c2006c {
> + #clock-cells = <1>;
> + compatible = "allwinner,sun6i-a31-apb2-gates-clk";
> + reg = <0x01c2006c 0x8>;
> + clocks = <&apb2>;
> + clock-output-names = "apb2_i2c0", "apb2_i2c1",
> + "apb2_i2c2", "apb2_i2c3", "apb2_uart0",
> + "apb2_uart1", "apb2_uart2", "apb2_uart3",
> + "apb2_uart4", "apb2_uart5";
> + };
> };
>
> soc@01c20000 {
> @@ -69,7 +186,7 @@
> compatible = "allwinner,sun6i-a31-pinctrl";
> reg = <0x01c20800 0x400>;
> interrupts = <0 11 1>, <0 15 1>, <0 16 1>, <0 17 1>;
> - clocks = <&osc>;
> + clocks = <&apb1_gates 5>;
> gpio-controller;
> interrupt-controller;
> #address-cells = <1>;
> @@ -92,7 +209,7 @@
> <0 20 1>,
> <0 21 1>,
> <0 22 1>;
> - clocks = <&osc>;
> + clocks = <&osc24M>;
> };
>
> wdt1: watchdog@01c20ca0 {
> @@ -106,7 +223,7 @@
> interrupts = <0 0 1>;
> reg-shift = <2>;
> reg-io-width = <4>;
> - clocks = <&osc>;
> + clocks = <&apb2_gates 16>;
> status = "disabled";
> };
>
> @@ -116,7 +233,7 @@
> interrupts = <0 1 1>;
> reg-shift = <2>;
> reg-io-width = <4>;
> - clocks = <&osc>;
> + clocks = <&apb2_gates 17>;
> status = "disabled";
> };
>
> @@ -126,7 +243,7 @@
> interrupts = <0 2 1>;
> reg-shift = <2>;
> reg-io-width = <4>;
> - clocks = <&osc>;
> + clocks = <&apb2_gates 18>;
> status = "disabled";
> };
>
> @@ -136,7 +253,7 @@
> interrupts = <0 3 1>;
> reg-shift = <2>;
> reg-io-width = <4>;
> - clocks = <&osc>;
> + clocks = <&apb2_gates 19>;
> status = "disabled";
> };
>
> @@ -146,7 +263,7 @@
> interrupts = <0 4 1>;
> reg-shift = <2>;
> reg-io-width = <4>;
> - clocks = <&osc>;
> + clocks = <&apb2_gates 20>;
> status = "disabled";
> };
>
> @@ -156,7 +273,7 @@
> interrupts = <0 5 1>;
> reg-shift = <2>;
> reg-io-width = <4>;
> - clocks = <&osc>;
> + clocks = <&apb2_gates 21>;
> status = "disabled";
> };
>
>

Thanks for working on this!

Emilio
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/