All of lore.kernel.org
 help / color / mirror / Atom feed
From: emilio@elopez.com.ar (Emilio López)
To: linux-arm-kernel@lists.infradead.org
Subject: [PATCH 4/4] ARM: sun6i: Enable clock support in the DTSI
Date: Tue, 30 Jul 2013 22:36:46 -0300	[thread overview]
Message-ID: <51F86A2E.10505@elopez.com.ar> (raw)
In-Reply-To: <1375195462-19566-5-git-send-email-maxime.ripard@free-electrons.com>

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@free-electrons.com>
> ---
>  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 at 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 at 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 at 01c20050 {
> +			#clock-cells = <0>;
> +			compatible = "allwinner,sun4i-axi-clk";
> +			reg = <0x01c20050 0x4>;
> +			clocks = <&cpu>;
> +		};
> +
> +		ahb1_mux: ahb1_mux at 01c20054 {
> +			#clock-cells = <0>;
> +			compatible = "allwinner,sun6i-ahb1-mux-clk";
> +			reg = <0x01c20054 0x4>;
> +			clocks = <&osc32k>, <&osc24M>, <&axi>, <&pll6>;
> +		};
> +
> +		ahb1: ahb1 at 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 at 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 at 01c20054 {
> +			#clock-cells = <0>;
> +			compatible = "allwinner,sun4i-apb0-clk";
> +			reg = <0x01c20054 0x4>;
> +			clocks = <&ahb1>;
> +		};
> +
> +		apb1_gates: apb1_gates at 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 at 01c20058 {
> +			#clock-cells = <0>;
> +			compatible = "allwinner,sun4i-apb1-mux-clk";
> +			reg = <0x01c20058 0x4>;
> +			clocks = <&osc32k>, <&osc24M>, <&pll6>, <&pll6>;
> +		};
> +
> +		apb2: apb2 at 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 at 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 at 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 at 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

WARNING: multiple messages have this Message-ID (diff)
From: "Emilio López" <emilio@elopez.com.ar>
To: Maxime Ripard <maxime.ripard@free-electrons.com>
Cc: Mike Turquette <mturquette@linaro.org>,
	kevin.z.m.zh@gmail.com, sunny@allwinnertech.com,
	shuge@allwinnertech.com, linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 4/4] ARM: sun6i: Enable clock support in the DTSI
Date: Tue, 30 Jul 2013 22:36:46 -0300	[thread overview]
Message-ID: <51F86A2E.10505@elopez.com.ar> (raw)
In-Reply-To: <1375195462-19566-5-git-send-email-maxime.ripard@free-electrons.com>

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@free-electrons.com>
> ---
>  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

  reply	other threads:[~2013-07-31  1:36 UTC|newest]

Thread overview: 39+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-07-30 14:44 [PATCH 0/4] Add support for the Allwinner A31 clocks Maxime Ripard
2013-07-30 14:44 ` Maxime Ripard
2013-07-30 14:44 ` [PATCH 1/4] clk: sunxi: Rename the structure to prepare the addition of sun6i Maxime Ripard
2013-07-30 14:44   ` Maxime Ripard
2013-07-31  0:14   ` Emilio López
2013-07-31  0:14     ` Emilio López
2013-07-31  9:20     ` Maxime Ripard
2013-07-31  9:20       ` Maxime Ripard
2013-07-30 14:44 ` [PATCH 2/4] clk: sunxi: Allow to specify the divider width from the dividers data Maxime Ripard
2013-07-30 14:44   ` Maxime Ripard
2013-07-31  0:27   ` Emilio López
2013-07-31  0:27     ` Emilio López
2013-07-30 14:44 ` [PATCH 3/4] clk: sunxi: Add A31 clocks support Maxime Ripard
2013-07-30 14:44   ` Maxime Ripard
2013-07-31  1:01   ` Emilio López
2013-07-31  1:01     ` Emilio López
2013-07-31 10:14     ` Maxime Ripard
2013-07-31 10:14       ` Maxime Ripard
2013-08-12 12:53   ` Mark Rutland
2013-08-12 12:53     ` Mark Rutland
2013-08-12 13:01     ` Emilio López
2013-08-12 13:01       ` Emilio López
2013-08-12 13:54       ` Mark Rutland
2013-08-12 13:54         ` Mark Rutland
2013-07-30 14:44 ` [PATCH 4/4] ARM: sun6i: Enable clock support in the DTSI Maxime Ripard
2013-07-30 14:44   ` Maxime Ripard
2013-07-31  1:36   ` Emilio López [this message]
2013-07-31  1:36     ` Emilio López
2013-07-31  7:37     ` Maxime Ripard
2013-07-31  7:37       ` Maxime Ripard
     [not found]       ` <2013073116110750016327@gmail.com>
2013-07-31 11:37         ` Emilio López
2013-07-31 11:37           ` Emilio López
2013-07-31 11:49         ` maxime.ripard
2013-07-31 11:49           ` maxime.ripard
2013-07-31 12:10           ` kevin.z.m
2013-07-31 15:49             ` maxime.ripard
2013-07-31 15:49               ` maxime.ripard
     [not found]               ` <2013080108343040654547@gmail.com>
2013-08-01  9:53                 ` maxime.ripard
2013-08-01  9:53                   ` maxime.ripard

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=51F86A2E.10505@elopez.com.ar \
    --to=emilio@elopez.com.ar \
    --cc=linux-arm-kernel@lists.infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.