Patchwork [RESEND,5/5] ARM: dts: berlin: add the pinctrl node and muxing setup for uarts

login
register
mail settings
Submitter Antoine Ténart
Date April 10, 2014, 1:07 p.m.
Message ID <1397135274-10764-6-git-send-email-antoine.tenart@free-electrons.com>
Download mbox | patch
Permalink /patch/338277/
State New
Headers show

Comments

Antoine Ténart - April 10, 2014, 1:07 p.m.
The uart0 pinmux configuration is in the dtsi because uart0 will always
use uart0-pmux to work, no other possibility. Same thing for uart1.

Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com>
---
 arch/arm/boot/dts/berlin2.dtsi   | 20 ++++++++++++++++++++
 arch/arm/boot/dts/berlin2cd.dtsi | 13 +++++++++++++
 arch/arm/boot/dts/berlin2q.dtsi  | 20 ++++++++++++++++++++
 3 files changed, 53 insertions(+)
Andrew Lunn - April 11, 2014, 8:18 a.m.
On Thu, Apr 10, 2014 at 03:07:54PM +0200, Antoine Ténart wrote:
> The uart0 pinmux configuration is in the dtsi because uart0 will always
> use uart0-pmux to work, no other possibility. Same thing for uart1.
> 
> Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com>
> ---
>  arch/arm/boot/dts/berlin2.dtsi   | 20 ++++++++++++++++++++
>  arch/arm/boot/dts/berlin2cd.dtsi | 13 +++++++++++++
>  arch/arm/boot/dts/berlin2q.dtsi  | 20 ++++++++++++++++++++
>  3 files changed, 53 insertions(+)
> 
> diff --git a/arch/arm/boot/dts/berlin2.dtsi b/arch/arm/boot/dts/berlin2.dtsi
> index 56a1af2f1052..43eb90c36050 100644
> --- a/arch/arm/boot/dts/berlin2.dtsi
> +++ b/arch/arm/boot/dts/berlin2.dtsi
> @@ -176,6 +176,22 @@
>  			};
>  		};
>  
> +		pinctrl: pinctrl@0 {
> +			compatible = "marvell,berlin2-pinctrl";
> +			reg = <0xea0000 0x08>, <0xfc0000 0x44>;
> +			reg-names = "global_base", "apb_base";
> +
> +			uart0_pmux: uart0-pmux {
> +				berlin,group = "GSM4";
> +				berlin,function = <0>;
> +			};
> +
> +			uart1_pmux: uart1-pmux {
> +				berlin,group = "GSM5";
> +				berlin,function = <1>;

Hi Antoine

This very much looks like black magic.

I assume the data sheet is not available? So i think you need to
document all possible combinations of values of group and function.
Maybe you can add a file in

arch/arm/boot/dts/include/dt-bindings/pinctrl/

with something like

#define GSM4_UART0	0
#define GSM5_UART1	1
#define GSM12_UART0	1
#define GSM12_IrDA0	1
#define GSM12_GPIO	2

	Andrew
Antoine Ténart - April 11, 2014, 9:09 a.m.
Hi Andrew,

On Fri, Apr 11, 2014 at 10:18:59AM +0200, Andrew Lunn wrote:
> On Thu, Apr 10, 2014 at 03:07:54PM +0200, Antoine Ténart wrote:
> > The uart0 pinmux configuration is in the dtsi because uart0 will always
> > use uart0-pmux to work, no other possibility. Same thing for uart1.
> > 
> > Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com>
> > ---
> >  arch/arm/boot/dts/berlin2.dtsi   | 20 ++++++++++++++++++++
> >  arch/arm/boot/dts/berlin2cd.dtsi | 13 +++++++++++++
> >  arch/arm/boot/dts/berlin2q.dtsi  | 20 ++++++++++++++++++++
> >  3 files changed, 53 insertions(+)
> > 
> > diff --git a/arch/arm/boot/dts/berlin2.dtsi b/arch/arm/boot/dts/berlin2.dtsi
> > index 56a1af2f1052..43eb90c36050 100644
> > --- a/arch/arm/boot/dts/berlin2.dtsi
> > +++ b/arch/arm/boot/dts/berlin2.dtsi
> > @@ -176,6 +176,22 @@
> >  			};
> >  		};
> >  
> > +		pinctrl: pinctrl@0 {
> > +			compatible = "marvell,berlin2-pinctrl";
> > +			reg = <0xea0000 0x08>, <0xfc0000 0x44>;
> > +			reg-names = "global_base", "apb_base";
> > +
> > +			uart0_pmux: uart0-pmux {
> > +				berlin,group = "GSM4";
> > +				berlin,function = <0>;
> > +			};
> > +
> > +			uart1_pmux: uart1-pmux {
> > +				berlin,group = "GSM5";
> > +				berlin,function = <1>;
> 
> Hi Antoine
> 
> This very much looks like black magic.
> 
> I assume the data sheet is not available? So i think you need to
> document all possible combinations of values of group and function.
> Maybe you can add a file in
> 
> arch/arm/boot/dts/include/dt-bindings/pinctrl/
> 
> with something like
> 
> #define GSM4_UART0	0
> #define GSM5_UART1	1
> #define GSM12_UART0	1
> #define GSM12_IrDA0	1
> #define GSM12_GPIO	2

Groups' functions are not the same between BG2/BG2CD and BG2Q so multiple
headers would be needed which may not be a good thing. Some platforms use black
magic, and we can find a pinmux configuration for OMAP looking like:

0x128 (PIN_INPUT_PULLUP | MUX_MODE0)

Weel, we could explicitly define the functions in the driver itself and use in
the dt something like:

berlin,group = "GSM4";
berlin,function = "uart0";

This is what's done on the sunxi's pinctrl.

Antoine
Sebastian Hesselbarth - April 11, 2014, 9:19 a.m.
On 04/11/2014 11:09 AM, Antoine Ténart wrote:
> Hi Andrew,
>
> On Fri, Apr 11, 2014 at 10:18:59AM +0200, Andrew Lunn wrote:
>> On Thu, Apr 10, 2014 at 03:07:54PM +0200, Antoine Ténart wrote:
>>> The uart0 pinmux configuration is in the dtsi because uart0 will always
>>> use uart0-pmux to work, no other possibility. Same thing for uart1.
>>>
>>> Signed-off-by: Antoine Ténart <antoine.tenart@free-electrons.com>
>>> ---
>>>   arch/arm/boot/dts/berlin2.dtsi   | 20 ++++++++++++++++++++
>>>   arch/arm/boot/dts/berlin2cd.dtsi | 13 +++++++++++++
>>>   arch/arm/boot/dts/berlin2q.dtsi  | 20 ++++++++++++++++++++
>>>   3 files changed, 53 insertions(+)
>>>
>>> diff --git a/arch/arm/boot/dts/berlin2.dtsi b/arch/arm/boot/dts/berlin2.dtsi
>>> index 56a1af2f1052..43eb90c36050 100644
>>> --- a/arch/arm/boot/dts/berlin2.dtsi
>>> +++ b/arch/arm/boot/dts/berlin2.dtsi
>>> @@ -176,6 +176,22 @@
>>>   			};
>>>   		};
>>>
>>> +		pinctrl: pinctrl@0 {
>>> +			compatible = "marvell,berlin2-pinctrl";
>>> +			reg = <0xea0000 0x08>, <0xfc0000 0x44>;
>>> +			reg-names = "global_base", "apb_base";
>>> +
>>> +			uart0_pmux: uart0-pmux {
>>> +				berlin,group = "GSM4";
>>> +				berlin,function = <0>;
>>> +			};
>>> +
>>> +			uart1_pmux: uart1-pmux {
>>> +				berlin,group = "GSM5";
>>> +				berlin,function = <1>;
>>
>> This very much looks like black magic.
>>
>> I assume the data sheet is not available? So i think you need to
>> document all possible combinations of values of group and function.
>> Maybe you can add a file in
>>
>> arch/arm/boot/dts/include/dt-bindings/pinctrl/
>>
>> with something like
>>
>> #define GSM4_UART0	0
>> #define GSM5_UART1	1
>> #define GSM12_UART0	1
>> #define GSM12_IrDA0	1
>> #define GSM12_GPIO	2
>
> Groups' functions are not the same between BG2/BG2CD and BG2Q so multiple
> headers would be needed which may not be a good thing. Some platforms use black
> magic, and we can find a pinmux configuration for OMAP looking like:
>
> 0x128 (PIN_INPUT_PULLUP | MUX_MODE0)

And it is a constant pain-in-the-ass to understand it at all
without looking it up in different places.

For Berlin, the only publically available option is BSP code,
and you don't want to look it up in there even once more than
necessary. ;)

I don't see a problem in having one binding for each (different)
SoC at all.

> Weel, we could explicitly define the functions in the driver itself and use in
> the dt something like:
>
> berlin,group = "GSM4";
> berlin,function = "uart0";
>
> This is what's done on the sunxi's pinctrl.

We also have function strings in mvebu pinctrl. The name is very
useful for /sys/kernel/debug/pinctrl/ but nothing more. Matching
numbers is easier than matching string, so I suggest to stick with
binding includes as Andrew suggested.

Sebastian

Patch

diff --git a/arch/arm/boot/dts/berlin2.dtsi b/arch/arm/boot/dts/berlin2.dtsi
index 56a1af2f1052..43eb90c36050 100644
--- a/arch/arm/boot/dts/berlin2.dtsi
+++ b/arch/arm/boot/dts/berlin2.dtsi
@@ -176,6 +176,22 @@ 
 			};
 		};
 
+		pinctrl: pinctrl@0 {
+			compatible = "marvell,berlin2-pinctrl";
+			reg = <0xea0000 0x08>, <0xfc0000 0x44>;
+			reg-names = "global_base", "apb_base";
+
+			uart0_pmux: uart0-pmux {
+				berlin,group = "GSM4";
+				berlin,function = <0>;
+			};
+
+			uart1_pmux: uart1-pmux {
+				berlin,group = "GSM5";
+				berlin,function = <1>;
+			};
+		};
+
 		apb@fc0000 {
 			compatible = "simple-bus";
 			#address-cells = <1>;
@@ -191,6 +207,8 @@ 
 				reg-io-width = <1>;
 				interrupts = <8>;
 				clocks = <&smclk>;
+				pinctrl-0 = <&uart0_pmux>;
+				pinctrl-names = "default";
 				status = "disabled";
 			};
 
@@ -201,6 +219,8 @@ 
 				reg-io-width = <1>;
 				interrupts = <9>;
 				clocks = <&smclk>;
+				pinctrl-0 = <&uart1_pmux>;
+				pinctrl-names = "default";
 				status = "disabled";
 			};
 
diff --git a/arch/arm/boot/dts/berlin2cd.dtsi b/arch/arm/boot/dts/berlin2cd.dtsi
index 094968c27533..4c04cff89226 100644
--- a/arch/arm/boot/dts/berlin2cd.dtsi
+++ b/arch/arm/boot/dts/berlin2cd.dtsi
@@ -169,6 +169,17 @@ 
 			};
 		};
 
+		pinctrl: pinctrl@0 {
+			compatible = "marvell,berlin2cd-pinctrl";
+			reg = <0xea0000 0x08>, <0xfc0000 0x44>;
+			reg-names = "global_base", "apb_base";
+
+			uart0_pmux: uart0-pmux {
+				berlin,group = "G6";
+				berlin,function = <0>;
+			};
+		};
+
 		apb@fc0000 {
 			compatible = "simple-bus";
 			#address-cells = <1>;
@@ -184,6 +195,8 @@ 
 				reg-io-width = <1>;
 				interrupts = <8>;
 				clocks = <&smclk>;
+				pinctrl-0 = <&uart0_pmux>;
+				pinctrl-names = "default";
 				status = "disabled";
 			};
 
diff --git a/arch/arm/boot/dts/berlin2q.dtsi b/arch/arm/boot/dts/berlin2q.dtsi
index 07452a7483fa..e6e556055dfc 100644
--- a/arch/arm/boot/dts/berlin2q.dtsi
+++ b/arch/arm/boot/dts/berlin2q.dtsi
@@ -183,6 +183,22 @@ 
 			};
 		};
 
+		pinctrl: pinctrl@0 {
+			compatible = "marvell,berlin2q-pinctrl";
+			reg = <0xea0000 0x4c>, <0xfcd000 0x44>;
+			reg-names = "global_base", "sm_base";
+
+			uart0_pmux: uart0-pmux {
+				berlin,group = "GSM12";
+				berlin,function = <0>;
+			};
+
+			uart1_pmux: uart1-pmux {
+				berlin,group = "GSM14";
+				berlin,function = <1>;
+			};
+		};
+
 		apb@fc0000 {
 			compatible = "simple-bus";
 			#address-cells = <1>;
@@ -198,6 +214,8 @@ 
 				interrupts = <8>;
 				clocks = <&smclk>;
 				reg-shift = <2>;
+				pinctrl-0 = <&uart0_pmux>;
+				pinctrl-names = "default";
 				status = "disabled";
 			};
 
@@ -208,6 +226,8 @@ 
 				interrupts = <9>;
 				clocks = <&smclk>;
 				reg-shift = <2>;
+				pinctrl-0 = <&uart1_pmux>;
+				pinctrl-names = "default";
 				status = "disabled";
 			};