[PATCH 1/2] pwm: sun4i: Add Allwinner H3 support

[PATCH 1/2] pwm: sun4i: Add Allwinner H3 support

[Milo Kim]

According to the latest datasheet (v1.2), H3 has single PWM channel.
H3 PWM controller has same register layout as sun4i driver, so it works 
by adding H3 specific data.
And the second PWM channel is not supported, so the pinctrl function is removed.

Datasheet:
  http://linux-sunxi.org/File:Allwinner_H3_Datasheet_V1.2.pdf

Test environment:
  Tested on Nano Pi M1 board, but PA5 pin is assigned for UART0.
  So the debug console should be changed to other port like UART1.

  Ex)
	aliases {
		serial0 = &uart1;
	};

	&uart1 {
        	pinctrl-names = "default";
	        pinctrl-0 = <&uart1_pins_a>;
        	status = "okay";
	};

Cc: Chen-Yu Tsai <wens@csie.org>
Cc: Maxime Ripard <maxime.ripard@free-electrons.com>
Cc: Rob Herring <robh+dt@kernel.org>
Cc: Thierry Reding <thierry.reding@gmail.com>
Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
---
 Documentation/devicetree/bindings/pwm/pwm-sun4i.txt |  1 +
 arch/arm/boot/dts/sun8i-h3.dtsi                     | 15 +++++++++++++++
 drivers/pinctrl/sunxi/pinctrl-sun8i-h3.c            |  1 -
 drivers/pwm/pwm-sun4i.c                             |  9 +++++++++
 4 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/pwm/pwm-sun4i.txt b/Documentation/devicetree/bindings/pwm/pwm-sun4i.txt
index cf6068b..f1cbeef 100644
--- a/Documentation/devicetree/bindings/pwm/pwm-sun4i.txt
+++ b/Documentation/devicetree/bindings/pwm/pwm-sun4i.txt
@@ -6,6 +6,7 @@ Required properties:
     - "allwinner,sun5i-a10s-pwm"
     - "allwinner,sun5i-a13-pwm"
     - "allwinner,sun7i-a20-pwm"
+    - "allwinner,sun8i-h3-pwm"
   - reg: physical base address and length of the controller's registers
   - #pwm-cells: should be 3. See pwm.txt in this directory for a description of
     the cells format.
diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
index b44bc48..3a965fb 100644
--- a/arch/arm/boot/dts/sun8i-h3.dtsi
+++ b/arch/arm/boot/dts/sun8i-h3.dtsi
@@ -512,6 +512,13 @@
 				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
 			};
 
+			pwm0_pin_a: pwm0@0 {
+				allwinner,pins = "PA5";
+				allwinner,function = "pwm0";
+				allwinner,drive = <SUN4I_PINCTRL_10_MA>;
+				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
+			};
+
 			uart0_pins_a: uart0@0 {
 				allwinner,pins = "PA4", "PA5";
 				allwinner,function = "uart0";
@@ -585,6 +592,14 @@
 			interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
 		};
 
+		pwm: pwm@01c21400 {
+			compatible = "allwinner,sun8i-h3-pwm";
+			reg = <0x01c21400 0x8>;
+			clocks = <&osc24M>;
+			#pwm-cells = <3>;
+			status = "disabled";
+		};
+
 		uart0: serial@01c28000 {
 			compatible = "snps,dw-apb-uart";
 			reg = <0x01c28000 0x400>;
diff --git a/drivers/pinctrl/sunxi/pinctrl-sun8i-h3.c b/drivers/pinctrl/sunxi/pinctrl-sun8i-h3.c
index 11760bb..087f231 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sun8i-h3.c
+++ b/drivers/pinctrl/sunxi/pinctrl-sun8i-h3.c
@@ -60,7 +60,6 @@ static const struct sunxi_desc_pin sun8i_h3_pins[] = {
 		  SUNXI_FUNCTION(0x0, "gpio_in"),
 		  SUNXI_FUNCTION(0x1, "gpio_out"),
 		  SUNXI_FUNCTION(0x2, "sim"),		/* PWREN */
-		  SUNXI_FUNCTION(0x3, "pwm1"),
 		  SUNXI_FUNCTION_IRQ_BANK(0x6, 0, 6)),	/* PA_EINT6 */
 	SUNXI_PIN(SUNXI_PINCTRL_PIN(A, 7),
 		  SUNXI_FUNCTION(0x0, "gpio_in"),
diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
index 03a99a5..b0803f6 100644
--- a/drivers/pwm/pwm-sun4i.c
+++ b/drivers/pwm/pwm-sun4i.c
@@ -284,6 +284,12 @@ static const struct sun4i_pwm_data sun4i_pwm_data_a20 = {
 	.npwm = 2,
 };
 
+static const struct sun4i_pwm_data sun4i_pwm_data_h3 = {
+	.has_prescaler_bypass = true,
+	.has_rdy = true,
+	.npwm = 1,
+};
+
 static const struct of_device_id sun4i_pwm_dt_ids[] = {
 	{
 		.compatible = "allwinner,sun4i-a10-pwm",
@@ -298,6 +304,9 @@ static const struct of_device_id sun4i_pwm_dt_ids[] = {
 		.compatible = "allwinner,sun7i-a20-pwm",
 		.data = &sun4i_pwm_data_a20,
 	}, {
+		.compatible = "allwinner,sun8i-h3-pwm",
+		.data = &sun4i_pwm_data_h3,
+	}, {
 		/* sentinel */
 	},
 };
-- 
1.9.1

[PATCH 2/2] ARM: dts: sun8i-h3: Add UART1 pinctrl

[Milo Kim]

In H3, PA5 can be used as PWM and UART0. If the PWM is used, the console
UART should be moved to other port.
This patch enables UART1 pinctrl to support this case.

	PA5: PWM
	PG6, PG7: debug console

Cc: Chen-Yu Tsai <wens@csie.org>
Cc: Maxime Ripard <maxime.ripard@free-electrons.com>
Cc: Rob Herring <robh+dt@kernel.org>
Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
---
 arch/arm/boot/dts/sun8i-h3.dtsi | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
index 3a965fb..fdb6801 100644
--- a/arch/arm/boot/dts/sun8i-h3.dtsi
+++ b/arch/arm/boot/dts/sun8i-h3.dtsi
@@ -526,6 +526,13 @@
 				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
 			};
 
+			uart1_pins_a: uart1@0 {
+				allwinner,pins = "PG6", "PG7";
+				allwinner,function = "uart1";
+				allwinner,drive = <SUN4I_PINCTRL_10_MA>;
+				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
+			};
+
 			mmc0_pins_a: mmc0@0 {
 				allwinner,pins = "PF0", "PF1", "PF2", "PF3",
 						 "PF4", "PF5";
-- 
1.9.1

Re: [PATCH 2/2] ARM: dts: sun8i-h3: Add UART1 pinctrl

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1414 bytes --]

Hi Milo,

On Thu, Aug 25, 2016 at 03:44:54PM +0900, Milo Kim wrote:
> In H3, PA5 can be used as PWM and UART0. If the PWM is used, the console
> UART should be moved to other port.
> This patch enables UART1 pinctrl to support this case.
> 
> 	PA5: PWM
> 	PG6, PG7: debug console
> 
> Cc: Chen-Yu Tsai <wens@csie.org>
> Cc: Maxime Ripard <maxime.ripard@free-electrons.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
> ---
>  arch/arm/boot/dts/sun8i-h3.dtsi | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
> index 3a965fb..fdb6801 100644
> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
> @@ -526,6 +526,13 @@
>  				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
>  			};
>  
> +			uart1_pins_a: uart1@0 {
> +				allwinner,pins = "PG6", "PG7";
> +				allwinner,function = "uart1";
> +				allwinner,drive = <SUN4I_PINCTRL_10_MA>;
> +				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
> +			};
> +

Our policy is that we don't introduce new pinctrl nodes that are not
used by any board to avoid bloating the DT too much with unused nodes.

If you have a board using it, please submit that change as well.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] ARM: dts: sun8i-h3: Add UART1 pinctrl

[Milo Kim]

On 08/26/2016 04:10 PM, Maxime Ripard wrote:
>> +			uart1_pins_a: uart1@0 {
>> > +				allwinner,pins = "PG6", "PG7";
>> > +				allwinner,function = "uart1";
>> > +				allwinner,drive = <SUN4I_PINCTRL_10_MA>;
>> > +				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
>> > +			};
>> > +
> Our policy is that we don't introduce new pinctrl nodes that are not
> used by any board to avoid bloating the DT too much with unused nodes.
>
> If you have a board using it, please submit that change as well.

OK, I agree.

However, this pinmux is only useful in case PA5 is assigned for PWM.
PA5 is commonly used for UART0_RX, so I'd like to add UART1 node into 
*.dts and disable it (status = "disabled"). Does it make sense?

Best regards,
Milo

Re: [PATCH 2/2] ARM: dts: sun8i-h3: Add UART1 pinctrl

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1087 bytes --]

Hi Milo,

On Fri, Aug 26, 2016 at 05:00:37PM +0900, Milo Kim wrote:
> On 08/26/2016 04:10 PM, Maxime Ripard wrote:
> >>+			uart1_pins_a: uart1@0 {
> >>> +				allwinner,pins = "PG6", "PG7";
> >>> +				allwinner,function = "uart1";
> >>> +				allwinner,drive = <SUN4I_PINCTRL_10_MA>;
> >>> +				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
> >>> +			};
> >>> +
> >Our policy is that we don't introduce new pinctrl nodes that are not
> >used by any board to avoid bloating the DT too much with unused nodes.
> >
> >If you have a board using it, please submit that change as well.
> 
> OK, I agree.
> 
> However, this pinmux is only useful in case PA5 is assigned for PWM.
> PA5 is commonly used for UART0_RX, so I'd like to add UART1 node into *.dts
> and disable it (status = "disabled"). Does it make sense?

I guess UART1 is either used or not used on that board. In the former
case, it should be enabled, in the latter, left out of the DTS
entirely.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] ARM: dts: sun8i-h3: Add UART1 pinctrl

[Milo Kim]

Hi Maxime,

On 08/26/2016 04:10 PM, Maxime Ripard wrote:
> Hi Milo,
>
> On Thu, Aug 25, 2016 at 03:44:54PM +0900, Milo Kim wrote:
>> In H3, PA5 can be used as PWM and UART0. If the PWM is used, the console
>> UART should be moved to other port.
>> This patch enables UART1 pinctrl to support this case.
>>
>> 	PA5: PWM
>> 	PG6, PG7: debug console
>>
>> Cc: Chen-Yu Tsai <wens@csie.org>
>> Cc: Maxime Ripard <maxime.ripard@free-electrons.com>
>> Cc: Rob Herring <robh+dt@kernel.org>
>> Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
>> ---
>>  arch/arm/boot/dts/sun8i-h3.dtsi | 7 +++++++
>>  1 file changed, 7 insertions(+)
>>
>> diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
>> index 3a965fb..fdb6801 100644
>> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
>> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
>> @@ -526,6 +526,13 @@
>>  				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
>>  			};
>>
>> +			uart1_pins_a: uart1@0 {
>> +				allwinner,pins = "PG6", "PG7";
>> +				allwinner,function = "uart1";
>> +				allwinner,drive = <SUN4I_PINCTRL_10_MA>;
>> +				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
>> +			};
>> +
>
> Our policy is that we don't introduce new pinctrl nodes that are not
> used by any board to avoid bloating the DT too much with unused nodes.
>
> If you have a board using it, please submit that change as well.

UART1 was already added in sun8i-h3.dtsi, so I drop this change in v2.

Commit-ID: 966c11a3b5e4

Best regards,
Milo

Re: [PATCH 1/2] pwm: sun4i: Add Allwinner H3 support

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5201 bytes --]

Hi Milo,

Remember to use get_maintainers to send your patches, you missed a few
mailing lists (linux-arm-kernel, for example), developpers (Alexandre
Belloni, the original author of the driver) and maintainers (Linus
Walleij for the pinctrl changes).

On Thu, Aug 25, 2016 at 03:44:53PM +0900, Milo Kim wrote:
> According to the latest datasheet (v1.2), H3 has single PWM channel.
> H3 PWM controller has same register layout as sun4i driver, so it works 
> by adding H3 specific data.
> And the second PWM channel is not supported, so the pinctrl function is removed.
> 
> Datasheet:
>   http://linux-sunxi.org/File:Allwinner_H3_Datasheet_V1.2.pdf

That should be in your cover letter, but not in your commit log. A
commit log is here forever, this link will very likely not.

> 
> Test environment:
>   Tested on Nano Pi M1 board, but PA5 pin is assigned for UART0.
>   So the debug console should be changed to other port like UART1.
> 
>   Ex)
> 	aliases {
> 		serial0 = &uart1;
> 	};
> 
> 	&uart1 {
>         	pinctrl-names = "default";
> 	        pinctrl-0 = <&uart1_pins_a>;
>         	status = "okay";
> 	};

That should be part of your cover letter too.

> Cc: Chen-Yu Tsai <wens@csie.org>
> Cc: Maxime Ripard <maxime.ripard@free-electrons.com>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Thierry Reding <thierry.reding@gmail.com>
> Signed-off-by: Milo Kim <woogyom.kim@gmail.com>
> ---
>  Documentation/devicetree/bindings/pwm/pwm-sun4i.txt |  1 +
>  arch/arm/boot/dts/sun8i-h3.dtsi                     | 15 +++++++++++++++
>  drivers/pinctrl/sunxi/pinctrl-sun8i-h3.c            |  1 -
>  drivers/pwm/pwm-sun4i.c                             |  9 +++++++++

Please split these changes into several patches:

>  4 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/pwm/pwm-sun4i.txt b/Documentation/devicetree/bindings/pwm/pwm-sun4i.txt
> index cf6068b..f1cbeef 100644
> --- a/Documentation/devicetree/bindings/pwm/pwm-sun4i.txt
> +++ b/Documentation/devicetree/bindings/pwm/pwm-sun4i.txt
> @@ -6,6 +6,7 @@ Required properties:
>      - "allwinner,sun5i-a10s-pwm"
>      - "allwinner,sun5i-a13-pwm"
>      - "allwinner,sun7i-a20-pwm"
> +    - "allwinner,sun8i-h3-pwm"
>    - reg: physical base address and length of the controller's registers
>    - #pwm-cells: should be 3. See pwm.txt in this directory for a description of
>      the cells format.
> diff --git a/arch/arm/boot/dts/sun8i-h3.dtsi b/arch/arm/boot/dts/sun8i-h3.dtsi
> index b44bc48..3a965fb 100644
> --- a/arch/arm/boot/dts/sun8i-h3.dtsi
> +++ b/arch/arm/boot/dts/sun8i-h3.dtsi
> @@ -512,6 +512,13 @@
>  				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
>  			};
>  
> +			pwm0_pin_a: pwm0@0 {
> +				allwinner,pins = "PA5";
> +				allwinner,function = "pwm0";
> +				allwinner,drive = <SUN4I_PINCTRL_10_MA>;
> +				allwinner,pull = <SUN4I_PINCTRL_NO_PULL>;
> +			};
> +

One to add the new pin...

>  			uart0_pins_a: uart0@0 {
>  				allwinner,pins = "PA4", "PA5";
>  				allwinner,function = "uart0";
> @@ -585,6 +592,14 @@
>  			interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
>  		};
>  
> +		pwm: pwm@01c21400 {
> +			compatible = "allwinner,sun8i-h3-pwm";
> +			reg = <0x01c21400 0x8>;
> +			clocks = <&osc24M>;
> +			#pwm-cells = <3>;
> +			status = "disabled";
> +		};
> +

... one to add the controller node ...

>  		uart0: serial@01c28000 {
>  			compatible = "snps,dw-apb-uart";
>  			reg = <0x01c28000 0x400>;
> diff --git a/drivers/pinctrl/sunxi/pinctrl-sun8i-h3.c b/drivers/pinctrl/sunxi/pinctrl-sun8i-h3.c
> index 11760bb..087f231 100644
> --- a/drivers/pinctrl/sunxi/pinctrl-sun8i-h3.c
> +++ b/drivers/pinctrl/sunxi/pinctrl-sun8i-h3.c
> @@ -60,7 +60,6 @@ static const struct sunxi_desc_pin sun8i_h3_pins[] = {
>  		  SUNXI_FUNCTION(0x0, "gpio_in"),
>  		  SUNXI_FUNCTION(0x1, "gpio_out"),
>  		  SUNXI_FUNCTION(0x2, "sim"),		/* PWREN */
> -		  SUNXI_FUNCTION(0x3, "pwm1"),

... one to remove the pinctrl pin ...

>  		  SUNXI_FUNCTION_IRQ_BANK(0x6, 0, 6)),	/* PA_EINT6 */
>  	SUNXI_PIN(SUNXI_PINCTRL_PIN(A, 7),
>  		  SUNXI_FUNCTION(0x0, "gpio_in"),
> diff --git a/drivers/pwm/pwm-sun4i.c b/drivers/pwm/pwm-sun4i.c
> index 03a99a5..b0803f6 100644
> --- a/drivers/pwm/pwm-sun4i.c
> +++ b/drivers/pwm/pwm-sun4i.c
> @@ -284,6 +284,12 @@ static const struct sun4i_pwm_data sun4i_pwm_data_a20 = {
>  	.npwm = 2,
>  };
>  
> +static const struct sun4i_pwm_data sun4i_pwm_data_h3 = {
> +	.has_prescaler_bypass = true,
> +	.has_rdy = true,
> +	.npwm = 1,
> +};
> +
>  static const struct of_device_id sun4i_pwm_dt_ids[] = {
>  	{
>  		.compatible = "allwinner,sun4i-a10-pwm",
> @@ -298,6 +304,9 @@ static const struct of_device_id sun4i_pwm_dt_ids[] = {
>  		.compatible = "allwinner,sun7i-a20-pwm",
>  		.data = &sun4i_pwm_data_a20,
>  	}, {
> +		.compatible = "allwinner,sun8i-h3-pwm",
> +		.data = &sun4i_pwm_data_h3,
> +	}, {

... and one to add the H3 support in the driver.

It looks good otherwise, thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 1/2] pwm: sun4i: Add Allwinner H3 support

[Milo Kim]

Hi Maxime,

On 08/26/2016 05:51 AM, Maxime Ripard wrote:
> Remember to use get_maintainers to send your patches, you missed a few
> mailing lists (linux-arm-kernel, for example), developpers (Alexandre
> Belloni, the original author of the driver) and maintainers (Linus
> Walleij for the pinctrl changes).

Thanks for this tip and your review regarding splitting pinctrl patch. 
Let me prepare the 2nd patch-set.

Best regards,
Milo