pinctrl: samsung: Remove static platform-specific data

arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi line 17

Quoted 11 lines · Tomasz Figa

The patch "pinctrl: samsung: Parse pin banks from DT" introduced platform-specific data parsing from DT.

This patch adds all necessary nodes and properties to exynos4210 device tree sources.

+++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi

arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi new file +3 -0

+/ {
+	pinctrl at 11400000 {
+		gpa0: pin-bank at 0 {

If you're going to put a unit address ("@0")into the DT node name, the node should have a reg property containing the same value, and the parent node should have #address-cells and #size-cells properties.

However, I believe you can actually get unique node names without using a unit address; instead name the nodes after the object they represent, so e.g. s/pin-bank at 0/gpa0/ above.

Review

Quoted 2 lines

(quoted diff) +1 -0

+			gpio-controller;

You need a #gpio-cells property too.

arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi line 26

Quoted 8 lines

arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi new file +8 -0

+			samsung,pctl-offset = <0x000>;
+			samsung,pin-bank = "gpa0";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;

The properties above represent the width of the fields. Must all fields always be packed together into say the LSB of the registers? What if there are gaps between the fields in a future SoC variant, or the order of the fields in the register changes? I think you want to add either a samsung,func-bit/samsung,func-position property for each of the fields, or change from samsung,func-width=<4> to samsung,field-mask=<0xf>. IIRC, the generic pinctrl binding used a mask for this purpose.

What if a future SoC variant adds more fields to the register? At that point, you'd need to edit the driver anyway in order to define a new DT property to represent the new field. Perhaps instead of having an explicit named property per field in the register, you want a simple list of fields:

samsung,pin-property-names = "func", "pud", "drv", "conpdn", "pudpdn"; samsung,pin-propert-masks = <0xf 0x30 0xc0 0x300 0xc00>;

That would allow a completely arbitrary number of fields and layouts to be described.

I wonder if for absolute generality you want a samsung,pin-stride property to represent the difference in register address per pin. I assume that's hard-coded as 4 right now.

Review

Quoted 1 line

(quoted diff) +1 -0

+			interrupt-controller;

You need a #interrupt-cells property too.

arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi line 94

Quoted 3 lines

arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi new file +3 -0

+		gpd0: pin-bank at 5 {
+			gpio-controller;
+			samsung,pctl-offset = <0x0A0>;

I think hex number are usually lower-case in DT, but I may be extrapolating a generality from a limited set of examples.

arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi line 529

Quoted 1 line

arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi new file +1 -0

+		gpy5: pin-bank at 19{

Missing a space before the { there.

Quoted 10 lines

(quoted diff) +0 -0

b/arch/arm/boot/dts/exynos4210.dtsi index ecbc707..0e93717 100644

arch/arm/boot/dts/exynos4210.dtsi +0 -0

@@ -59,6 +59,10 @@
 
 		reg = <0x11400000 0x1000>;
 		interrupts = <0 47 0>;
 		interrupt-controller;
 

arch/arm/boot/dts/exynos4210.dtsi line 65

Quoted 4 lines

arch/arm/boot/dts/exynos4210.dtsi +4 -0

+		samsung,geint-con = <0x700>;
+		samsung,geint-mask = <0x900>;
+		samsung,geint-pend = <0xA00>;
+		samsung,svc = <0xB08>;

I assume those new properties represent register addresses within the block. If not, I don't understand what they are.

It's unclear to me why those properties aren't all part of exynos4210-pinctrl-banks.dtsi. Do you really have multiple SoCs where the register addresses for the pinctrl registers are the same (hence can be in a shared exynos4210-pinctrl-banks.dtsi), yet the register addresses for the geint registers are different (hence must be in non-shared exynos4210.dtsi)?

Do these properties interact with samsung,eint-offset at all? Oh, perhaps these properties are defining a top-level interrupt controller that aggregates all the banks together, whereas samsung,eint-offset is per-bank?

Quoted 35 lines · On Friday 21 of September 2012 12:56:41 Stephen Warren wrote:

Quoted 8 lines · Tomasz Figa

The patch "pinctrl: samsung: Parse pin banks from DT" introduced platform-specific data parsing from DT.

This patch adds all necessary nodes and properties to exynos4210 device tree sources.

+++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi

arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi line 26

Quoted 8 lines

arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi new file +8 -0

+			samsung,pctl-offset = <0x000>;
+			samsung,pin-bank = "gpa0";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;

The properties above represent the width of the fields. Must all fields always be packed together into say the LSB of the registers? What if there are gaps between the fields in a future SoC variant, or the order of the fields in the register changes? I think you want to add either a samsung,func-bit/samsung,func-position property for each of the fields, or change from samsung,func-width=<4> to samsung,field-mask=<0xf>. IIRC, the generic pinctrl binding used a mask for this purpose.

What if a future SoC variant adds more fields to the register? At that point, you'd need to edit the driver anyway in order to define a new DT property to represent the new field. Perhaps instead of having an explicit named property per field in the register, you want a simple list of fields:

samsung,pin-property-names = "func", "pud", "drv", "conpdn", "pudpdn"; samsung,pin-propert-masks = <0xf 0x30 0xc0 0x300 0xc00>;

Quoted 6 lines

That would allow a completely arbitrary number of fields and layouts to be described.

I wonder if for absolute generality you want a samsung,pin-stride property to represent the difference in register address per pin. I assume that's hard-coded as 4 right now.

Hmm, considering so many different possible changes, maybe a more conservative solution would be better, like reducing the amount of information held in DT to bank type, e.g.

	samsung,bank-type = "exynos4";

or maybe

	compatible = "samsung,exynos4-pin-bank*;

and then define supported bank types in the driver. SoC-specific data would remain in DT, i.e. pctl-offset, pin-bank, pin-count, eint-offset, etc.

Quoted 10 lines

Quoted 10 lines

(quoted diff) +0 -0

b/arch/arm/boot/dts/exynos4210.dtsi index ecbc707..0e93717 100644

arch/arm/boot/dts/exynos4210.dtsi +0 -0

@@ -59,6 +59,10 @@
 
 		reg = <0x11400000 0x1000>;
 		interrupts = <0 47 0>;
 		interrupt-controller;
 

Quoted 7 lines

arch/arm/boot/dts/exynos4210.dtsi line 65

Quoted 4 lines

arch/arm/boot/dts/exynos4210.dtsi +4 -0

+		samsung,geint-con = <0x700>;
+		samsung,geint-mask = <0x900>;
+		samsung,geint-pend = <0xA00>;
+		samsung,svc = <0xB08>;

I assume those new properties represent register addresses within the block. If not, I don't understand what they are.

Yes, they do.

Quoted 6 lines

It's unclear to me why those properties aren't all part of exynos4210-pinctrl-banks.dtsi. Do you really have multiple SoCs where the register addresses for the pinctrl registers are the same (hence can be in a shared exynos4210-pinctrl-banks.dtsi), yet the register addresses for the geint registers are different (hence must be in non-shared exynos4210.dtsi)?

Exynos4210-pincstrl-banks.dtsi isn't shared, it's specific to Exynos4210. Other SoCs are going to have their own whatever-pinctrl-banks.dtsi.

Quoted 59 lines · Tomasz Figa

Quoted 42 lines · On Friday 21 of September 2012 12:56:41 Stephen Warren wrote:

Quoted 9 lines · Tomasz Figa

The patch "pinctrl: samsung: Parse pin banks from DT" introduced platform-specific data parsing from DT.

This patch adds all necessary nodes and properties to exynos4210 device
tree sources.

+++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi

arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi line 26

Quoted 8 lines

arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi new file +8 -0

+			samsung,pctl-offset = <0x000>;
+			samsung,pin-bank = "gpa0";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;

The properties above represent the width of the fields. Must all fields
always be packed together into say the LSB of the registers? What if there are gaps between the fields in a future SoC variant, or the order
of the fields in the register changes? I think you want to add either a
samsung,func-bit/samsung,func-position property for each of the fields,
or change from samsung,func-width=<4> to samsung,field-mask=<0xf>. IIRC,
the generic pinctrl binding used a mask for this purpose.

What if a future SoC variant adds more fields to the register? At that point, you'd need to edit the driver anyway in order to define a new DT
property to represent the new field. Perhaps instead of having an explicit named property per field in the register, you want a simple list of fields:

samsung,pin-property-names = "func", "pud", "drv", "conpdn", "pudpdn"; samsung,pin-propert-masks = <0xf 0x30 0xc0 0x300 0xc00>;

Quoted 7 lines

That would allow a completely arbitrary number of fields and layouts to
be described.

I wonder if for absolute generality you want a samsung,pin-stride property to represent the difference in register address per pin. I assume that's hard-coded as 4 right now.

Hmm, considering so many different possible changes, maybe a more conservative solution would be better, like reducing the amount of information held in DT to bank type, e.g.

	samsung,bank-type = "exynos4";

or maybe

Quoted 5 lines

	compatible = "samsung,exynos4-pin-bank*;

and then define supported bank types in the driver. SoC-specific data would remain in DT, i.e. pctl-offset, pin-bank, pin-count, eint-offset, etc.

Yes, removing much of the data from DT and putting it into a tiny table in the driver makes sense to me in this case.

Quoted 28 lines

Quoted 10 lines

Quoted 10 lines

(quoted diff) +0 -0

b/arch/arm/boot/dts/exynos4210.dtsi index ecbc707..0e93717 100644

arch/arm/boot/dts/exynos4210.dtsi +0 -0

@@ -59,6 +59,10 @@
 
 		reg = <0x11400000 0x1000>;
 		interrupts = <0 47 0>;
 		interrupt-controller;
 

Quoted 7 lines

arch/arm/boot/dts/exynos4210.dtsi line 65

Quoted 4 lines

arch/arm/boot/dts/exynos4210.dtsi +4 -0

+		samsung,geint-con = <0x700>;
+		samsung,geint-mask = <0x900>;
+		samsung,geint-pend = <0xA00>;
+		samsung,svc = <0xB08>;

I assume those new properties represent register addresses within the block. If not, I don't understand what they are.

Yes, they do.

Quoted 7 lines

It's unclear to me why those properties aren't all part of exynos4210-pinctrl-banks.dtsi. Do you really have multiple SoCs where the register addresses for the pinctrl registers are the same (hence can
be in a shared exynos4210-pinctrl-banks.dtsi), yet the register addresses for the geint registers are different (hence must be in non-shared exynos4210.dtsi)?

Quoted 3 lines

Exynos4210-pincstrl-banks.dtsi isn't shared, it's specific to Exynos4210. Other SoCs are going to have their own whatever-pinctrl-banks.dtsi.

OK, so my point here is: why not put all the pinctrl-related properties into a single file, rather than spreading them across different files. Having separate files makes sense if they can be re-used in different places, but not if they're single-use.

Quoted 60 lines · On Monday 24 of September 2012 11:42:15 Stephen Warren wrote:

Quoted 59 lines · Tomasz Figa

Quoted 42 lines · On Friday 21 of September 2012 12:56:41 Stephen Warren wrote:

Quoted 9 lines · Tomasz Figa

The patch "pinctrl: samsung: Parse pin banks from DT" introduced platform-specific data parsing from DT.

This patch adds all necessary nodes and properties to exynos4210 device
tree sources.

+++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi

arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi line 26

Quoted 8 lines

arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi new file +8 -0

+			samsung,pctl-offset = <0x000>;
+			samsung,pin-bank = "gpa0";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;

The properties above represent the width of the fields. Must all fields
always be packed together into say the LSB of the registers? What if there are gaps between the fields in a future SoC variant, or the order
of the fields in the register changes? I think you want to add either a
samsung,func-bit/samsung,func-position property for each of the fields,
or change from samsung,func-width=<4> to samsung,field-mask=<0xf>. IIRC,
the generic pinctrl binding used a mask for this purpose.

What if a future SoC variant adds more fields to the register? At that point, you'd need to edit the driver anyway in order to define a new DT
property to represent the new field. Perhaps instead of having an explicit named property per field in the register, you want a simple list of fields:

samsung,pin-property-names = "func", "pud", "drv", "conpdn", "pudpdn"; samsung,pin-propert-masks = <0xf 0x30 0xc0 0x300 0xc00>;

Quoted 7 lines

That would allow a completely arbitrary number of fields and layouts to
be described.

I wonder if for absolute generality you want a samsung,pin-stride property to represent the difference in register address per pin. I assume that's hard-coded as 4 right now.

Hmm, considering so many different possible changes, maybe a more conservative solution would be better, like reducing the amount of information held in DT to bank type, e.g.

	samsung,bank-type = "exynos4";

or maybe

Quoted 7 lines

Quoted 5 lines

	compatible = "samsung,exynos4-pin-bank*;

and then define supported bank types in the driver. SoC-specific data would remain in DT, i.e. pctl-offset, pin-bank, pin-count, eint-offset, etc.

Yes, removing much of the data from DT and putting it into a tiny table in the driver makes sense to me in this case.

A hybrid solution came to my mind, define bank types in device tree once and then reference them in banks. Something like:

	pinctrl-bank-types {
		bank_off: bank-off {
			samsung,func-width = <4>;
			samsung,pud-width = <2>;
			samsung,drv-width = <2>;
			samsung,conpdn-width = <2>;
			samsung,pudpdn-width = <2>;
		};

		bank_alive: bank-alive {
			samsung,func-width = <4>;
			samsung,pud-width = <2>;
			samsung,drv-width = <2>;
		};
	};

	/* ... */

	pinctrl at 12345678 {
		/* ... */
		gpa0: gpa0 {
			/* ... */
			samsung,bank-type = <&bank_off>;
			/* ... */
		};
		/* ... */
	};

This would add the possibility to define new banks types quickly, but would not add too much overhead.

What do you think?

Quoted 116 lines · Tomasz Figa

Quoted 65 lines · On Monday 24 of September 2012 11:42:15 Stephen Warren wrote:

Quoted 64 lines · Tomasz Figa

Quoted 45 lines · On Friday 21 of September 2012 12:56:41 Stephen Warren wrote:

Quoted 9 lines · Tomasz Figa

The patch "pinctrl: samsung: Parse pin banks from DT" introduced platform-specific data parsing from DT.

This patch adds all necessary nodes and properties to exynos4210 device
tree sources.

+++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi

arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi line 26

Quoted 8 lines

arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi new file +8 -0

+			samsung,pctl-offset = <0x000>;
+			samsung,pin-bank = "gpa0";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;

The properties above represent the width of the fields. Must all fields
always be packed together into say the LSB of the registers? What if there are gaps between the fields in a future SoC variant, or the order
of the fields in the register changes? I think you want to add either
a
samsung,func-bit/samsung,func-position property for each of the fields,
or change from samsung,func-width=<4> to samsung,field-mask=<0xf>. IIRC,
the generic pinctrl binding used a mask for this purpose.

What if a future SoC variant adds more fields to the register? At that
point, you'd need to edit the driver anyway in order to define a new DT
property to represent the new field. Perhaps instead of having an explicit named property per field in the register, you want a simple list of fields:

samsung,pin-property-names = "func", "pud", "drv", "conpdn", "pudpdn";
samsung,pin-propert-masks = <0xf 0x30 0xc0 0x300 0xc00>;

Quoted 7 lines

That would allow a completely arbitrary number of fields and layouts to
be described.

I wonder if for absolute generality you want a samsung,pin-stride property to represent the difference in register address per pin. I assume that's hard-coded as 4 right now.

Hmm, considering so many different possible changes, maybe a more conservative solution would be better, like reducing the amount of information held in DT to bank type, e.g.

	samsung,bank-type = "exynos4";

or maybe

	compatible = "samsung,exynos4-pin-bank*;

Quoted 7 lines

Quoted 3 lines

and then define supported bank types in the driver. SoC-specific data would remain in DT, i.e. pctl-offset, pin-bank, pin-count, eint-offset, etc.

Yes, removing much of the data from DT and putting it into a tiny table
in the driver makes sense to me in this case.

A hybrid solution came to my mind, define bank types in device tree once

and then reference them in banks. Something like:

	pinctrl-bank-types {
	
		bank_off: bank-off {
		
			samsung,func-width = <4>;
			samsung,pud-width = <2>;
			samsung,drv-width = <2>;
			samsung,conpdn-width = <2>;
			samsung,pudpdn-width = <2>;
		
		};
		
		bank_alive: bank-alive {
		
			samsung,func-width = <4>;
			samsung,pud-width = <2>;
			samsung,drv-width = <2>;
		
		};
	
	};
	
	/* ... */
	
	pinctrl at 12345678 {
	
		/* ... */
		gpa0: gpa0 {
		
			/* ... */
			samsung,bank-type = <&bank_off>;
			/* ... */
		
		};
		/* ... */
	
	};

Quoted 4 lines

This would add the possibility to define new banks types quickly, but would not add too much overhead.

What do you think?

That does solve the issue of parsing those same values over and over, but at the cost of making the binding a lot more conceptually complex.

However, it doesn't solve any of the extensibility issues I raised such as whether pos+size or mask would be a better representation, what about if the fields end up being in different (separate) registers on newer SoCs, etc.

I forget, do you actually have multiple different SoCs right now (or in the near future where the HW design is known now for certain even if the chip isn't available) that have different values for all these *-width properties and hence can be represented just using this binding and without altering the driver at all? If so, I suppose the original binding is at least useful (although I would certainly still request to use *-mask instead of *-width properties).

Hi Stephen,

Quoted 22 lines · On Monday 24 of September 2012 17:14:38 Stephen Warren wrote:

Quoted 21 lines · Tomasz Figa

Quoted 20 lines · On Monday 24 of September 2012 11:42:15 Stephen Warren wrote:

Quoted 19 lines · Tomasz Figa

Quoted 18 lines · On Friday 21 of September 2012 12:56:41 Stephen Warren wrote:

Quoted 17 lines · Tomasz Figa

The patch "pinctrl: samsung: Parse pin banks from DT" introduced platform-specific data parsing from DT.

This patch adds all necessary nodes and properties to exynos4210 device
tree sources.

+++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi

(quoted diff) +8 -0

+			samsung,pctl-offset = <0x000>;
+			samsung,pin-bank = "gpa0";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;

Hmm, could you elaborate on the idea of using mask instead of field widths?

I don't see how this could be better and there is an additional drawback of having to calculate width and pos from every mask.

Anyway, back to your concern, the values that are written to the bit fields specified by those bindings are arbitrary SoC-specific values anyway, so if, for example, we get a SoC with following register layout:

bits:    7 | 6 - 4  | 3 | 2 - 0
meaning: 0 | func 1 | 0 | func 0

or

bits:    7 - 5  | 4 | 3 - 1  | 0
meaning: func 1 | 0 | func 0 | 0

we can easily define the width as 4 and use appropriate 4-bit function values with zeroes on reserved positions.

Quoted 7 lines

I forget, do you actually have multiple different SoCs right now (or in the near future where the HW design is known now for certain even if the chip isn't available) that have different values for all these *-width properties and hence can be represented just using this binding and without altering the driver at all? If so, I suppose the original binding is at least useful (although I would certainly still request to use *-mask instead of *-width properties).

The binding I proposed covers all Samsung SoCs currently available, starting from s3c24xx, through s3c64xx (except 4bit2 bank type, with two function registers), to the whole Exynos series, including latest Exynos5.

Quoted 25 lines · Tomasz Figa

Hi Stephen,

Quoted 22 lines · On Monday 24 of September 2012 17:14:38 Stephen Warren wrote:

Quoted 21 lines · Tomasz Figa

Quoted 20 lines · On Monday 24 of September 2012 11:42:15 Stephen Warren wrote:

Quoted 19 lines · Tomasz Figa

Quoted 18 lines · On Friday 21 of September 2012 12:56:41 Stephen Warren wrote:

Quoted 17 lines · Tomasz Figa

The patch "pinctrl: samsung: Parse pin banks from DT" introduced platform-specific data parsing from DT.

This patch adds all necessary nodes and properties to exynos4210 device
tree sources.

+++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi

(quoted diff) +8 -0

+			samsung,pctl-offset = <0x000>;
+			samsung,pin-bank = "gpa0";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;

...

Quoted 2 lines

Hmm, could you elaborate on the idea of using mask instead of field widths?

For background: With e.g.:

samsung,func-width = <4>;
samsung,pud-width = <2>;
samsung,drv-width = <2>;

How do you know if the layout is:

bits:    7-4  | 3-2 | 1-0
meaning: func | pud | drv

or:

bits:    7-6 | 5-4 | 3-0  |
meaning: drv | pud | func |

or:

bits:    15-12 | 13-8   | 7-6 | 5-3    | 2-1 | 0
meaning: func  | unused | pud | unused | drv | unused

I suppose what you're saying is that for all currently extant Samsung SoCs, there's some rule that defines this; perhaps the fields are always in order MSB to LSB func, pud, drv, and there are never any unused bits between the fields? If so, I suppose that's reasonable, even if it does restrict the binding's ability to support any unanticipated future SoC register layout changes.

Quoted 2 lines

I don't see how this could be better and there is an additional drawback of having to calculate width and pos from every mask.

With the DT properties just defining "width", the driver still has to calculate the pos from every width by adding up the widths of all fields lower in the register, right? Or, does each field always start at a hard-coded bit position?

Anyway, you could completely avoid this question by using masks instead:

samsung,func-mask = <0xf0>;
samsung,pud-mask = <0xc>;
samsung,drv-mask = <0x3>;

The mask defines exactly which bits are included in the register field, so it implicitly defines both the position and width of the field.

Finding the shift/size is very easy. I believe Tony Lindgren's generic pinctrl already does this along these lines. Very roughly:

func_pos = ffs(func_mask);
func_width = ffs(~(func_mask >> func_pos));

Quoted 8 lines

Anyway, back to your concern, the values that are written to the bit fields specified by those bindings are arbitrary SoC-specific values anyway, so if, for example, we get a SoC with following register layout:

bits:    7 | 6 - 4  | 3 | 2 - 0
meaning: 0 | func 1 | 0 | func 0

Quoted 7 lines

or

bits:    7 - 5  | 4 | 3 - 1  | 0
meaning: func 1 | 0 | func 0 | 0

we can easily define the width as 4 and use appropriate 4-bit function values with zeroes on reserved positions.

The problem with that is that if the datasheet documents "func" values of 0, 1, 2, 3, whereas your driver expects values that are shifted left one bit in order to fit into the field, the DT would need to contain 0, 2, 4, 6. So, the DT values then don't match the documentation, which would end up being confusing.

Quoted 11 lines

Quoted 10 lines

I forget, do you actually have multiple different SoCs right now (or in
the near future where the HW design is known now for certain even if the
chip isn't available) that have different values for all these *-width properties and hence can be represented just using this binding and without altering the driver at all? If so, I suppose the original binding is at least useful (although I would certainly still request to
use *-mask instead of *-width properties).

Quoted 5 lines

The binding I proposed covers all Samsung SoCs currently available, starting from s3c24xx, through s3c64xx (except 4bit2 bank type, with two
function registers), to the whole Exynos series, including latest Exynos5.

OK, the HW is nice and consistent then. In that case, the binding is probably reasonable. Hopefully the HW designers are aware they shouldn't randomly break the uniformity!

Quoted 52 lines · On Tuesday 25 of September 2012 10:49:11 Stephen Warren wrote:

Quoted 25 lines · Tomasz Figa

Hi Stephen,

Quoted 22 lines · On Monday 24 of September 2012 17:14:38 Stephen Warren wrote:

Quoted 21 lines · Tomasz Figa

Quoted 20 lines · On Monday 24 of September 2012 11:42:15 Stephen Warren wrote:

Quoted 19 lines · Tomasz Figa

Quoted 18 lines · On Friday 21 of September 2012 12:56:41 Stephen Warren wrote:

Quoted 17 lines · Tomasz Figa

The patch "pinctrl: samsung: Parse pin banks from DT" introduced platform-specific data parsing from DT.

This patch adds all necessary nodes and properties to exynos4210 device
tree sources.

+++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi

(quoted diff) +8 -0

+			samsung,pctl-offset = <0x000>;
+			samsung,pin-bank = "gpa0";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;

...

Quoted 2 lines

Hmm, could you elaborate on the idea of using mask instead of field widths?

For background: With e.g.:

samsung,func-width = <4>;
samsung,pud-width = <2>;
samsung,drv-width = <2>;

How do you know if the layout is:

bits:    7-4  | 3-2 | 1-0
meaning: func | pud | drv

or:

bits:    7-6 | 5-4 | 3-0  |
meaning: drv | pud | func |

or:

bits:    15-12 | 13-8   | 7-6 | 5-3    | 2-1 | 0
meaning: func  | unused | pud | unused | drv | unused

Quoted 6 lines

I suppose what you're saying is that for all currently extant Samsung SoCs, there's some rule that defines this; perhaps the fields are always in order MSB to LSB func, pud, drv, and there are never any unused bits between the fields? If so, I suppose that's reasonable, even if it does restrict the binding's ability to support any unanticipated future SoC register layout changes.

I think we have a little misunderstanding here.

All the Samsung SoCs currently available have separate registers for particular configuration types. Each register is used to configure all pins in a bank. The width field specifies how many bits are used per pin, not per configuration type.

Quoted 66 lines · Tomasz Figa

Quoted 55 lines · On Tuesday 25 of September 2012 10:49:11 Stephen Warren wrote:

Quoted 27 lines · Tomasz Figa

Hi Stephen,

Quoted 24 lines · On Monday 24 of September 2012 17:14:38 Stephen Warren wrote:

Quoted 23 lines · Tomasz Figa

Quoted 22 lines · On Monday 24 of September 2012 11:42:15 Stephen Warren wrote:

Quoted 21 lines · Tomasz Figa

Quoted 20 lines · On Friday 21 of September 2012 12:56:41 Stephen Warren wrote:

Quoted 19 lines · Tomasz Figa

The patch "pinctrl: samsung: Parse pin banks from DT" introduced
platform-specific data parsing from DT.

This patch adds all necessary nodes and properties to exynos4210
device
tree sources.

+++ b/arch/arm/boot/dts/exynos4210-pinctrl-banks.dtsi

(quoted diff) +8 -0

+			samsung,pctl-offset = <0x000>;
+			samsung,pin-bank = "gpa0";
+			samsung,pin-count = <8>;
+			samsung,func-width = <4>;
+			samsung,pud-width = <2>;
+			samsung,drv-width = <2>;
+			samsung,conpdn-width = <2>;
+			samsung,pudpdn-width = <2>;

...

Quoted 2 lines

Hmm, could you elaborate on the idea of using mask instead of field widths?

For background: With e.g.:

samsung,func-width = <4>;
samsung,pud-width = <2>;
samsung,drv-width = <2>;

How do you know if the layout is:

bits:    7-4  | 3-2 | 1-0
meaning: func | pud | drv

or:

bits:    7-6 | 5-4 | 3-0  |
meaning: drv | pud | func |

or:

bits:    15-12 | 13-8   | 7-6 | 5-3    | 2-1 | 0
meaning: func  | unused | pud | unused | drv | unused

Quoted 9 lines

I suppose what you're saying is that for all currently extant Samsung SoCs, there's some rule that defines this; perhaps the fields are always
in order MSB to LSB func, pud, drv, and there are never any unused bits
between the fields? If so, I suppose that's reasonable, even if it does
restrict the binding's ability to support any unanticipated future SoC register layout changes.

Quoted 6 lines

I think we have a little misunderstanding here.

All the Samsung SoCs currently available have separate registers for particular configuration types. Each register is used to configure all pins in a bank. The width field specifies how many bits are used per pin, not per configuration type.

Oh I see. In that case, I guess just having "width" properties is fine, and I can see how it's much more likely this scheme would be extensible to any future SoC that sticks with the same overall kind of register structure.

It'd be a good idea to describe this explicitly in the binding documentation.

BTW, how does the driver know what register addresses to use; I can see the base for each pin controller bank is in samsung,pctl-offset, but what describes the offset for each of the func, pud, drv, ... registers from there? Are the offsets the same for all current Samsung SoCs?

On Tuesday 25 of September 2012 12:22:03 Stephen Warren wrote:

Quoted 4 lines

BTW, how does the driver know what register addresses to use; I can see the base for each pin controller bank is in samsung,pctl-offset, but what describes the offset for each of the func, pud, drv, ... registers from there? Are the offsets the same for all current Samsung SoCs?

The offsets are defined as constants in the driver.

They are the same in all cases, but the "4bit2" bank type of S3C64xx, which can have up to 16 pins with 4-bit function specifiers, so two registers are required for function configuration. In this case all the remaining registers are offset by 0x04.

I couldn't think about any good solution for this special case, but still, I haven't been thinking a lot about it, as the driver is targetted at current Exynos SoCs primarily.

This RFC series is a work on replacing static platform-specific data in pinctrl-samsung driver with data dynamically parsed from device tree.

This RFC series is a work on replacing static platform-specific data in pinctrl-samsung driver with data dynamically parsed from device tree.

It aims at reducing the SoC-specific part of the driver and thus the amount of modifications to driver sources when adding support for next SoCs (like Exynos4x12).

Furthermore, moving definitions of pin banks to device tree will allow to simplify GPIO and GEINT specification to a format similar to used previously by gpiolib-based implementation, using a phandle to the bank and pin index inside the bank, e.g.

	gpios = <&gpa1 4 0>;
	interrupt-parent = <&gpa1>;
	interrupts = <4 0>;

Any comments are welcome.

TODO:
- bindings documentation

Quoted 2 lines · Tomasz Figa

This RFC series is a work on replacing static platform-specific data in pinctrl-samsung driver with data dynamically parsed from device tree.

Hmm. I tend to think this is exactly the opposite of the correct direction; you end up wasting a whole ton of time during the boot process parsing data out of the device tree only to end up with exactly the same tables that you would have just put into the kernel anyway.

Is
it really likely that future SoCs will change information such as the width of the pullup/pulldown bitfield, but not change anything else that's not already in this binding. If that isn't the case, the binding won't be complete enough to describe any new features on future SoCs anyway.

Quoted 9 lines

It aims at reducing the SoC-specific part of the driver and thus the amount of modifications to driver sources when adding support for next SoCs (like Exynos4x12).

Furthermore, moving definitions of pin banks to device tree will allow to simplify GPIO and GEINT specification to a format similar to used previously by gpiolib-based implementation, using a phandle to the bank and pin index inside the bank, e.g.

Quoted 3 lines

	gpios = <&gpa1 4 0>;
	interrupt-parent = <&gpa1>;
	interrupts = <4 0>;

I don't think those two are correlated; the GPIO specifier format could just as easily be <bank pin> irrespective of whether the pinctrl driver contains SoC-specific tables or not.

Quoted 4 lines

Any comments are welcome.

TODO:
- bindings documentation

That's unfortunate; it would be the most interesting part to review. I guess I'll try to work out the binding from the examples in patch 6.

Quoted 3 lines · On Friday 21 of September 2012 12:40:35 Stephen Warren wrote:

Quoted 2 lines · Tomasz Figa

This RFC series is a work on replacing static platform-specific data in pinctrl-samsung driver with data dynamically parsed from device tree.

Quoted 9 lines

Quoted 9 lines

It aims at reducing the SoC-specific part of the driver and thus the amount of modifications to driver sources when adding support for next SoCs (like Exynos4x12).

Furthermore, moving definitions of pin banks to device tree will allow to simplify GPIO and GEINT specification to a format similar to used previously by gpiolib-based implementation, using a phandle to the bank and pin index inside the bank, e.g.

Quoted 7 lines

Quoted 3 lines

	gpios = <&gpa1 4 0>;
	interrupt-parent = <&gpa1>;
	interrupts = <4 0>;

I don't think those two are correlated; the GPIO specifier format could just as easily be <bank pin> irrespective of whether the pinctrl driver contains SoC-specific tables or not.

Correct me if I'm wrong, but each bank needs to have its own subnode to be able to address pins like this. That was the starting point of the whole series and the idea that if all the banks (which are SoC-specific) have to be defined anyway, maybe it wouldn't be too bad to put all the SoC-specific parameters there too.