[RFC PATCH 0/3] pinctrl: sunxi: Add DT-based generic pinctrl driver

[RFC PATCH 0/3] pinctrl: sunxi: Add DT-based generic pinctrl driver

[Andre Przywara]

Hi,

so far the pinctrl driver for supporting a particular Allwinner SoC requires
a hardcoded table in the kernel code. This table (for instance [1]) lists
all pins and puts names to each special function each pin can have, along
with the respective mux value to put into the hardware registers. That looks
like:
	SUNXI_PIN(SUNXI_PINCTRL_PIN(G, 1),
		SUNXI_FUNCTION(0x0, "gpio_in"),
		SUNXI_FUNCTION(0x1, "gpio_out"),
		SUNXI_FUNCTION(0x2, "mmc1"),          /* CMD */
		SUNXI_FUNCTION_IRQ_BANK(0x6, 1, 1)),  /* EINT1 */
	SUNXI_PIN(SUNXI_PINCTRL_PIN(G, 2),
		....

On top of that we have the DT, which groups the pins and refers to the
function to use *by name*:
			mmc1_pins: mmc1-pins {
				pins = "PG0", "PG1", "PG2", "PG3",
				       "PG4", "PG5";
				function = "mmc1";
				drive-strength = <30>;
				bias-pull-up;
			};

This series here moves the data encoded in the table so far into the DT itself,
removing the need for a hardcoded table in the kernel.

The approach taken here is to parse the DT and generate the table with
the help of additional properties, then hand this table over to the existing
driver. This is covered by three basic extensions to the DT binding:
- allwinner,gpio-pins = <[nr of PA pins] [nr of PB pins] ...>;
  This tells the driver how many pins each port has (0 is possible as
  well). The sum of all of them is used to size the array. Also the pin
  names are deduced from it and generated. Each pin gets an entry for
  GPIO in and out, using mux 0 and 1, respectively.
- allwinner,irq-pin-map = 
    <[IRQ port] [1st IRQ pin] [GPIO port] [1st GPIO pin] [mux value] [length]>
  Maps IRQ pins to their associated GPIO pins, describing the pins that can
  actually trigger interrupts. There can be multiple of these maps, each
  consisting of those six values.
  Every IRQ capable pin in those ports gets assigned an additional function
  "irq" (see the SUNXI_FUNCTION_IRQ_BANK line above).
- pinmux = <[mux value] ...>;
  This property (in the pin group subnodes) tells the driver which mux value
  to actually write into the hardware registers upon selecting this function.
  For almost every group this mux value is the same for every pin, so we fill
  remaining entries with the last entry in that property, if this property
  has less members than the number of pins in this group.
For the A64, for instance, this looks like this:
  pio: pinctrl@1c20800 {
	compatible = "allwinner,sun50i-a64-pinctrl",
		     "allwinner,sunxi-pinctrl";
	reg = <0x01c20800 0x400>;
	...
	/* No Port A, PB0..PB9, PC0..PC16, PD0..PD24, ... */
	allwinner,gpio-pins = <0 10 17 25 18 7 14 12>;
	/* The three ports B, G and H can trigger interrupts. */
	allwinner,irq-ports = <0 0 1 0 6 10>,
			      <1 0 6 0 6 14>,
			      <2 0 7 0 6 12>;
	...
	mmc1_pins: mmc1-pins {
		pins = "PG0", "PG1", "PG2", "PG3", "PG4", "PG5";
		function = "mmc1";
		pinmux = <2>;
		drive-strength = <30>;
		bias-pull-up;
	};
	...

The benefit of this series is two-fold:
- Future SoCs don't need an in-kernel table anymore. They can describe
  everything in the DT, and use the generic compatible as a fallback:
	compatible = "allwinner,sun50i-h6-pinctrl", "allwinner,sunxi-pinctrl";
  So this driver should be the last file added to this directory ever.
  Of course we can't remove the existing tables, to keep compatiblity with
  older DTs, but at least we don't need to add more tables in the future.
  The Allwinner H6 will be the first user of this driver.
- New DT consumers (other than the Linux kernel) could force usage of the
  new binding, if that's feasible for them. They would not need to add
  any SoC specific data into their code, instead have a generic driver that
  reads all information from the DT.
  A prominent example is U-Boot, which at the moment has *some* pinctrl data
  hardcoded, but wants to move to at DT based driver. Instead of copying the
  kernel tables and blowing up the code, we can add the new properties to
  U-Boot's DT copy and keep the code clean.

Please note that the new binding is indeed just an extension, the existing
driver just ignores those new properties and uses the in-kernel table, thus
working as before, without any restrictions.
So we can as well add those new properties to the kernel DT copy, which makes
it easier for other users to re-use the DT files.
The extended DT would add the generic compatible as a fallback, but keep the
existing compatible name in the first place. So existing kernel drivers
would match on the first string and use the table. U-Boot on the other hand
would not match on the specific string, but recognize the generic name and
pull the information from the DT.
This allows the very same DT to be used by both drivers (or both bindings),
triggered by the generic compatible.


Please let me know your opinion on this. The approach to generate the table
from the DT and re-using the existing driver leaves some room for optimization.
But I found it far easier to not touch the actual driver, since we need it
to stay around to support the old binding anyway.
If it seems worth the come up with a separate pinctrl driver which just
supports the new binding and makes the DT a first class citizen, let me
know - and give me some time to fully understand the inner workings of the
pinctrl and GPIO subsystem then.

Cheers,
Andre.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pinctrl/sunxi/pinctrl-sun50i-a64.c

Andre Przywara (3):
  dt-bindings: pinctrl: sunxi: document new generic binding
  pinctrl: sunxi: introduce DT-based generic driver
  arm64: dts: allwinner: enhance A64 .dtsi with new pinctrl binding

 .../bindings/pinctrl/allwinner,sunxi-pinctrl.txt   |  58 +++
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi      |  27 +-
 drivers/pinctrl/sunxi/Makefile                     |   1 +
 drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c           | 412 +++++++++++++++++++++
 4 files changed, 496 insertions(+), 2 deletions(-)
 create mode 100644 drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c

-- 
2.14.1

[RFC PATCH 0/3] pinctrl: sunxi: Add DT-based generic pinctrl driver

[Andre Przywara]

Hi,

so far the pinctrl driver for supporting a particular Allwinner SoC requires
a hardcoded table in the kernel code. This table (for instance [1]) lists
all pins and puts names to each special function each pin can have, along
with the respective mux value to put into the hardware registers. That looks
like:
	SUNXI_PIN(SUNXI_PINCTRL_PIN(G, 1),
		SUNXI_FUNCTION(0x0, "gpio_in"),
		SUNXI_FUNCTION(0x1, "gpio_out"),
		SUNXI_FUNCTION(0x2, "mmc1"),          /* CMD */
		SUNXI_FUNCTION_IRQ_BANK(0x6, 1, 1)),  /* EINT1 */
	SUNXI_PIN(SUNXI_PINCTRL_PIN(G, 2),
		....

On top of that we have the DT, which groups the pins and refers to the
function to use *by name*:
			mmc1_pins: mmc1-pins {
				pins = "PG0", "PG1", "PG2", "PG3",
				       "PG4", "PG5";
				function = "mmc1";
				drive-strength = <30>;
				bias-pull-up;
			};

This series here moves the data encoded in the table so far into the DT itself,
removing the need for a hardcoded table in the kernel.

The approach taken here is to parse the DT and generate the table with
the help of additional properties, then hand this table over to the existing
driver. This is covered by three basic extensions to the DT binding:
- allwinner,gpio-pins = <[nr of PA pins] [nr of PB pins] ...>;
  This tells the driver how many pins each port has (0 is possible as
  well). The sum of all of them is used to size the array. Also the pin
  names are deduced from it and generated. Each pin gets an entry for
  GPIO in and out, using mux 0 and 1, respectively.
- allwinner,irq-pin-map = 
    <[IRQ port] [1st IRQ pin] [GPIO port] [1st GPIO pin] [mux value] [length]>
  Maps IRQ pins to their associated GPIO pins, describing the pins that can
  actually trigger interrupts. There can be multiple of these maps, each
  consisting of those six values.
  Every IRQ capable pin in those ports gets assigned an additional function
  "irq" (see the SUNXI_FUNCTION_IRQ_BANK line above).
- pinmux = <[mux value] ...>;
  This property (in the pin group subnodes) tells the driver which mux value
  to actually write into the hardware registers upon selecting this function.
  For almost every group this mux value is the same for every pin, so we fill
  remaining entries with the last entry in that property, if this property
  has less members than the number of pins in this group.
For the A64, for instance, this looks like this:
  pio: pinctrl at 1c20800 {
	compatible = "allwinner,sun50i-a64-pinctrl",
		     "allwinner,sunxi-pinctrl";
	reg = <0x01c20800 0x400>;
	...
	/* No Port A, PB0..PB9, PC0..PC16, PD0..PD24, ... */
	allwinner,gpio-pins = <0 10 17 25 18 7 14 12>;
	/* The three ports B, G and H can trigger interrupts. */
	allwinner,irq-ports = <0 0 1 0 6 10>,
			      <1 0 6 0 6 14>,
			      <2 0 7 0 6 12>;
	...
	mmc1_pins: mmc1-pins {
		pins = "PG0", "PG1", "PG2", "PG3", "PG4", "PG5";
		function = "mmc1";
		pinmux = <2>;
		drive-strength = <30>;
		bias-pull-up;
	};
	...

The benefit of this series is two-fold:
- Future SoCs don't need an in-kernel table anymore. They can describe
  everything in the DT, and use the generic compatible as a fallback:
	compatible = "allwinner,sun50i-h6-pinctrl", "allwinner,sunxi-pinctrl";
  So this driver should be the last file added to this directory ever.
  Of course we can't remove the existing tables, to keep compatiblity with
  older DTs, but at least we don't need to add more tables in the future.
  The Allwinner H6 will be the first user of this driver.
- New DT consumers (other than the Linux kernel) could force usage of the
  new binding, if that's feasible for them. They would not need to add
  any SoC specific data into their code, instead have a generic driver that
  reads all information from the DT.
  A prominent example is U-Boot, which at the moment has *some* pinctrl data
  hardcoded, but wants to move to at DT based driver. Instead of copying the
  kernel tables and blowing up the code, we can add the new properties to
  U-Boot's DT copy and keep the code clean.

Please note that the new binding is indeed just an extension, the existing
driver just ignores those new properties and uses the in-kernel table, thus
working as before, without any restrictions.
So we can as well add those new properties to the kernel DT copy, which makes
it easier for other users to re-use the DT files.
The extended DT would add the generic compatible as a fallback, but keep the
existing compatible name in the first place. So existing kernel drivers
would match on the first string and use the table. U-Boot on the other hand
would not match on the specific string, but recognize the generic name and
pull the information from the DT.
This allows the very same DT to be used by both drivers (or both bindings),
triggered by the generic compatible.


Please let me know your opinion on this. The approach to generate the table
from the DT and re-using the existing driver leaves some room for optimization.
But I found it far easier to not touch the actual driver, since we need it
to stay around to support the old binding anyway.
If it seems worth the come up with a separate pinctrl driver which just
supports the new binding and makes the DT a first class citizen, let me
know - and give me some time to fully understand the inner workings of the
pinctrl and GPIO subsystem then.

Cheers,
Andre.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pinctrl/sunxi/pinctrl-sun50i-a64.c

Andre Przywara (3):
  dt-bindings: pinctrl: sunxi: document new generic binding
  pinctrl: sunxi: introduce DT-based generic driver
  arm64: dts: allwinner: enhance A64 .dtsi with new pinctrl binding

 .../bindings/pinctrl/allwinner,sunxi-pinctrl.txt   |  58 +++
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi      |  27 +-
 drivers/pinctrl/sunxi/Makefile                     |   1 +
 drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c           | 412 +++++++++++++++++++++
 4 files changed, 496 insertions(+), 2 deletions(-)
 create mode 100644 drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c

-- 
2.14.1

[RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Andre Przywara]

So far all the Allwinner pinctrl drivers provided a table in the
kernel to describe all the pins and the link between the pinctrl functions
names (strings) and their respective mux values (register values).

Extend the binding to put those mappings in the DT, so that any SoC can
describe its pinctrl and GPIO data fully there instead of relying on
tables.
This uses a generic compatible name, to be prepended with an SoC
specific name in the node.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 .../bindings/pinctrl/allwinner,sunxi-pinctrl.txt   | 58 ++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/allwinner,sunxi-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/allwinner,sunxi-pinctrl.txt
index 6f2ec9af0de2..c1ea755229da 100644
--- a/Documentation/devicetree/bindings/pinctrl/allwinner,sunxi-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/allwinner,sunxi-pinctrl.txt
@@ -28,6 +28,7 @@ Required properties:
   "allwinner,sun50i-a64-r-pinctrl"
   "allwinner,sun50i-h5-pinctrl"
   "nextthing,gr8-pinctrl"
+  "allwinner,sunxi-pinctrl"	(see below)
 
 - reg: Should contain the register physical address and length for the
   pin controller.
@@ -69,6 +70,63 @@ Optional sub-node properties:
   - bias-pull-down
   - drive-strength
 
+** Generic pinctrl binding
+The above binding requires knowledge of the actual mux setting values for
+each supported SoC in the code parsing the DT (for instance the kernel).
+The generic binding puts this information in the DT. It uses the
+"allwinner,sunxi-pinctrl" compatible, in addition to some SoC specific string.
+It extends the above described binding as follows:
+Required properties:
+- allwinner,gpio-pins: An array of 32-bit numbers to denote the number of
+  implemented pins per pin controller port. Non-implemented ports can specify
+  0 here. There will be as many ports as this array has elements.
+- allwinner,irq-pin-map: Contains a number of IRQ port maps, describing the
+  relationship between interrupt banks and GPIO pins. Each map has six 32-bit
+  members:
+  <[IRQ port] [1st IRQ pin] [GPIO port] [1st GPIO pin] [mux value] [length]>
+  This maps the first [length] IRQ pins starting with [IRQ port]:[1st IRQ pin]
+  to [GPIO port]:[1st GPIO pin], all using [mux value] to select the IRQ
+  functionality.
+
+Optional properties:
+- allwinner,port-base: The number of GPIO ports to skip at the beginning.
+- allwinner,irq-bank-base: The number of IRQ banks to skip at the beginning.
+- allwinner,irq-read-needs-mux: Specifies that reading the line level of
+  a pin configured as an IRQ pin is not possible. A driver needs to switch
+  to the GPIO-in function to be able to read the level.
+
+Required properties for subnodes:
+- pinmux: An array of mux values to write into the respective MMIO register
+  bits for this pin when selecting the function. If this array has less
+  elements than pins, the *last* value will be used for all pins beyond that.
+  This allows to use a single element for the (likely) case all pins use the
+  same mux value.
+
+The binding described above can be extended in this manner to be supported
+by *both* an existing driver and some generic driver. Existing drivers will
+ignore the new properties and revert to their internal table instead.
+
+Example:
+  pinctrl@1c20800 {
+	compatible = "allwinner,sun50i-a64-pinctrl",
+		     "allwinner,sunxi-pinctrl";
+	reg = <0x01c20800 0x400>;
+	clocks = <&ccu 58>, <&hosc>, <&losc>;
+	/* No PortA, PB0-PB9, PC0-PC16, PD0-PD24, ... */
+	allwinner,gpio-pins = <0 10 17 25 18 7 14 12>;
+	/* banks B, G and H can trigger interrupts, using mux value 6 */
+	allwinner,irq-pin-map= <0 0 1 0 6 10>,
+			       <1 0 6 0 6 14>,
+			       <2 0 7 0 6 12>;
+	i2c1_pins: i2c1_pins {
+		pins = "PH2", "PH3";
+		function = "i2c1";
+		/* Both pins use a mux value of 2 to select this function. */
+		pinmux = <2>;
+	};
+	...
+  };
+
 *** Deprecated pin configuration and multiplexing binding
 
 Required subnode-properties:
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Andre Przywara]

So far all the Allwinner pinctrl drivers provided a table in the
kernel to describe all the pins and the link between the pinctrl functions
names (strings) and their respective mux values (register values).

Extend the binding to put those mappings in the DT, so that any SoC can
describe its pinctrl and GPIO data fully there instead of relying on
tables.
This uses a generic compatible name, to be prepended with an SoC
specific name in the node.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 .../bindings/pinctrl/allwinner,sunxi-pinctrl.txt   | 58 ++++++++++++++++++++++
 1 file changed, 58 insertions(+)

diff --git a/Documentation/devicetree/bindings/pinctrl/allwinner,sunxi-pinctrl.txt b/Documentation/devicetree/bindings/pinctrl/allwinner,sunxi-pinctrl.txt
index 6f2ec9af0de2..c1ea755229da 100644
--- a/Documentation/devicetree/bindings/pinctrl/allwinner,sunxi-pinctrl.txt
+++ b/Documentation/devicetree/bindings/pinctrl/allwinner,sunxi-pinctrl.txt
@@ -28,6 +28,7 @@ Required properties:
   "allwinner,sun50i-a64-r-pinctrl"
   "allwinner,sun50i-h5-pinctrl"
   "nextthing,gr8-pinctrl"
+  "allwinner,sunxi-pinctrl"	(see below)
 
 - reg: Should contain the register physical address and length for the
   pin controller.
@@ -69,6 +70,63 @@ Optional sub-node properties:
   - bias-pull-down
   - drive-strength
 
+** Generic pinctrl binding
+The above binding requires knowledge of the actual mux setting values for
+each supported SoC in the code parsing the DT (for instance the kernel).
+The generic binding puts this information in the DT. It uses the
+"allwinner,sunxi-pinctrl" compatible, in addition to some SoC specific string.
+It extends the above described binding as follows:
+Required properties:
+- allwinner,gpio-pins: An array of 32-bit numbers to denote the number of
+  implemented pins per pin controller port. Non-implemented ports can specify
+  0 here. There will be as many ports as this array has elements.
+- allwinner,irq-pin-map: Contains a number of IRQ port maps, describing the
+  relationship between interrupt banks and GPIO pins. Each map has six 32-bit
+  members:
+  <[IRQ port] [1st IRQ pin] [GPIO port] [1st GPIO pin] [mux value] [length]>
+  This maps the first [length] IRQ pins starting with [IRQ port]:[1st IRQ pin]
+  to [GPIO port]:[1st GPIO pin], all using [mux value] to select the IRQ
+  functionality.
+
+Optional properties:
+- allwinner,port-base: The number of GPIO ports to skip at the beginning.
+- allwinner,irq-bank-base: The number of IRQ banks to skip at the beginning.
+- allwinner,irq-read-needs-mux: Specifies that reading the line level of
+  a pin configured as an IRQ pin is not possible. A driver needs to switch
+  to the GPIO-in function to be able to read the level.
+
+Required properties for subnodes:
+- pinmux: An array of mux values to write into the respective MMIO register
+  bits for this pin when selecting the function. If this array has less
+  elements than pins, the *last* value will be used for all pins beyond that.
+  This allows to use a single element for the (likely) case all pins use the
+  same mux value.
+
+The binding described above can be extended in this manner to be supported
+by *both* an existing driver and some generic driver. Existing drivers will
+ignore the new properties and revert to their internal table instead.
+
+Example:
+  pinctrl at 1c20800 {
+	compatible = "allwinner,sun50i-a64-pinctrl",
+		     "allwinner,sunxi-pinctrl";
+	reg = <0x01c20800 0x400>;
+	clocks = <&ccu 58>, <&hosc>, <&losc>;
+	/* No PortA, PB0-PB9, PC0-PC16, PD0-PD24, ... */
+	allwinner,gpio-pins = <0 10 17 25 18 7 14 12>;
+	/* banks B, G and H can trigger interrupts, using mux value 6 */
+	allwinner,irq-pin-map= <0 0 1 0 6 10>,
+			       <1 0 6 0 6 14>,
+			       <2 0 7 0 6 12>;
+	i2c1_pins: i2c1_pins {
+		pins = "PH2", "PH3";
+		function = "i2c1";
+		/* Both pins use a mux value of 2 to select this function. */
+		pinmux = <2>;
+	};
+	...
+  };
+
 *** Deprecated pin configuration and multiplexing binding
 
 Required subnode-properties:
-- 
2.14.1

Re: [RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Rob Herring]

On Mon, Nov 13, 2017 at 01:25:21AM +0000, Andre Przywara wrote:
> So far all the Allwinner pinctrl drivers provided a table in the
> kernel to describe all the pins and the link between the pinctrl functions
> names (strings) and their respective mux values (register values).
> 
> Extend the binding to put those mappings in the DT, so that any SoC can
> describe its pinctrl and GPIO data fully there instead of relying on
> tables.
> This uses a generic compatible name, to be prepended with an SoC
> specific name in the node.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  .../bindings/pinctrl/allwinner,sunxi-pinctrl.txt   | 58 ++++++++++++++++++++++
>  1 file changed, 58 insertions(+)

I'm leaving this one to Linus...

Generally, this has not been the direction we've wanted to go. What 
makes this unique to Allwinner?

Rob

[RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Rob Herring]

On Mon, Nov 13, 2017 at 01:25:21AM +0000, Andre Przywara wrote:
> So far all the Allwinner pinctrl drivers provided a table in the
> kernel to describe all the pins and the link between the pinctrl functions
> names (strings) and their respective mux values (register values).
> 
> Extend the binding to put those mappings in the DT, so that any SoC can
> describe its pinctrl and GPIO data fully there instead of relying on
> tables.
> This uses a generic compatible name, to be prepended with an SoC
> specific name in the node.
> 
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> ---
>  .../bindings/pinctrl/allwinner,sunxi-pinctrl.txt   | 58 ++++++++++++++++++++++
>  1 file changed, 58 insertions(+)

I'm leaving this one to Linus...

Generally, this has not been the direction we've wanted to go. What 
makes this unique to Allwinner?

Rob

Re: [RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Linus Walleij]

On Mon, Nov 13, 2017 at 2:25 AM, Andre Przywara <andre.przywara@arm.com> wrote:

> So far all the Allwinner pinctrl drivers provided a table in the
> kernel to describe all the pins and the link between the pinctrl functions
> names (strings) and their respective mux values (register values).
>
> Extend the binding to put those mappings in the DT, so that any SoC can
> describe its pinctrl and GPIO data fully there instead of relying on
> tables.
> This uses a generic compatible name, to be prepended with an SoC
> specific name in the node.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
(...)

I definately want feedback from Maxime before I do anything with
this patch series.

> +** Generic pinctrl binding
> +The above binding requires knowledge of the actual mux setting values for
> +each supported SoC in the code parsing the DT (for instance the kernel).
> +The generic binding puts this information in the DT. It uses the
> +"allwinner,sunxi-pinctrl" compatible, in addition to some SoC specific string.
> +It extends the above described binding as follows:
> +Required properties:
> +- allwinner,gpio-pins: An array of 32-bit numbers to denote the number of
> +  implemented pins per pin controller port. Non-implemented ports can specify
> +  0 here. There will be as many ports as this array has elements.

I don't understand what this adds that gpio-ranges does not already
provide.
Documentation/devicetree/bindings/gpio/gpio.txt
at the end of the file.

These ranges exist exactly to map pin controller pins in a pin controller
to GPIO lines in a gpiochip.

> +- allwinner,irq-pin-map: Contains a number of IRQ port maps, describing the
> +  relationship between interrupt banks and GPIO pins. Each map has six 32-bit
> +  members:
> +  <[IRQ port] [1st IRQ pin] [GPIO port] [1st GPIO pin] [mux value] [length]>
> +  This maps the first [length] IRQ pins starting with [IRQ port]:[1st IRQ pin]
> +  to [GPIO port]:[1st GPIO pin], all using [mux value] to select the IRQ
> +  functionality.

We have recently added GPIO "bank" awareness into gpiolib
via Thierry Reding's patches, so a gpiochip can use the generic
GPIOLIB_IRQCHIP and specify multiple IRQ parents with a map.

Please see if you can use this instead.

If any of this should be expressed in the DT bindings it should
be genericized and not use any "allwinner,*" prefixes, and it
should preferably just be hard-coded in the driver and switched
in from the compatible string IMO.

> +Optional properties:
> +- allwinner,port-base: The number of GPIO ports to skip at the beginning.
> +- allwinner,irq-bank-base: The number of IRQ banks to skip at the beginning.

I don't understand this. It looks hacky. Can you elaborate why this
is needed?

> +- allwinner,irq-read-needs-mux: Specifies that reading the line level of
> +  a pin configured as an IRQ pin is not possible. A driver needs to switch
> +  to the GPIO-in function to be able to read the level.

I guess it is a bool flag?

> +Required properties for subnodes:
> +- pinmux: An array of mux values to write into the respective MMIO register
> +  bits for this pin when selecting the function. If this array has less
> +  elements than pins, the *last* value will be used for all pins beyond that.
> +  This allows to use a single element for the (likely) case all pins use the
> +  same mux value.

This is a standard bindings so I don't have much against it. But I
need Maxime's input here, I think we should keep Allwinner consistent
across SoCs.

Yours,
Linus Walleij

[RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Linus Walleij]

On Mon, Nov 13, 2017 at 2:25 AM, Andre Przywara <andre.przywara@arm.com> wrote:

> So far all the Allwinner pinctrl drivers provided a table in the
> kernel to describe all the pins and the link between the pinctrl functions
> names (strings) and their respective mux values (register values).
>
> Extend the binding to put those mappings in the DT, so that any SoC can
> describe its pinctrl and GPIO data fully there instead of relying on
> tables.
> This uses a generic compatible name, to be prepended with an SoC
> specific name in the node.
>
> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
(...)

I definately want feedback from Maxime before I do anything with
this patch series.

> +** Generic pinctrl binding
> +The above binding requires knowledge of the actual mux setting values for
> +each supported SoC in the code parsing the DT (for instance the kernel).
> +The generic binding puts this information in the DT. It uses the
> +"allwinner,sunxi-pinctrl" compatible, in addition to some SoC specific string.
> +It extends the above described binding as follows:
> +Required properties:
> +- allwinner,gpio-pins: An array of 32-bit numbers to denote the number of
> +  implemented pins per pin controller port. Non-implemented ports can specify
> +  0 here. There will be as many ports as this array has elements.

I don't understand what this adds that gpio-ranges does not already
provide.
Documentation/devicetree/bindings/gpio/gpio.txt
at the end of the file.

These ranges exist exactly to map pin controller pins in a pin controller
to GPIO lines in a gpiochip.

> +- allwinner,irq-pin-map: Contains a number of IRQ port maps, describing the
> +  relationship between interrupt banks and GPIO pins. Each map has six 32-bit
> +  members:
> +  <[IRQ port] [1st IRQ pin] [GPIO port] [1st GPIO pin] [mux value] [length]>
> +  This maps the first [length] IRQ pins starting with [IRQ port]:[1st IRQ pin]
> +  to [GPIO port]:[1st GPIO pin], all using [mux value] to select the IRQ
> +  functionality.

We have recently added GPIO "bank" awareness into gpiolib
via Thierry Reding's patches, so a gpiochip can use the generic
GPIOLIB_IRQCHIP and specify multiple IRQ parents with a map.

Please see if you can use this instead.

If any of this should be expressed in the DT bindings it should
be genericized and not use any "allwinner,*" prefixes, and it
should preferably just be hard-coded in the driver and switched
in from the compatible string IMO.

> +Optional properties:
> +- allwinner,port-base: The number of GPIO ports to skip at the beginning.
> +- allwinner,irq-bank-base: The number of IRQ banks to skip at the beginning.

I don't understand this. It looks hacky. Can you elaborate why this
is needed?

> +- allwinner,irq-read-needs-mux: Specifies that reading the line level of
> +  a pin configured as an IRQ pin is not possible. A driver needs to switch
> +  to the GPIO-in function to be able to read the level.

I guess it is a bool flag?

> +Required properties for subnodes:
> +- pinmux: An array of mux values to write into the respective MMIO register
> +  bits for this pin when selecting the function. If this array has less
> +  elements than pins, the *last* value will be used for all pins beyond that.
> +  This allows to use a single element for the (likely) case all pins use the
> +  same mux value.

This is a standard bindings so I don't have much against it. But I
need Maxime's input here, I think we should keep Allwinner consistent
across SoCs.

Yours,
Linus Walleij

Re: [RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Thierry Reding]

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

On Fri, Nov 24, 2017 at 11:19:52AM +0100, Linus Walleij wrote:
> On Mon, Nov 13, 2017 at 2:25 AM, Andre Przywara <andre.przywara@arm.com> wrote:
> 
> > So far all the Allwinner pinctrl drivers provided a table in the
> > kernel to describe all the pins and the link between the pinctrl functions
> > names (strings) and their respective mux values (register values).
> >
> > Extend the binding to put those mappings in the DT, so that any SoC can
> > describe its pinctrl and GPIO data fully there instead of relying on
> > tables.
> > This uses a generic compatible name, to be prepended with an SoC
> > specific name in the node.

This seems backwards to me. I'm not sure if Rob has any hard rules on
this, but in the past I've seen a lot of drivers stick this kind of data
into drivers. I personally also prefer that approach because the data is
completely static and there's no way for any specific board to customize
it. So the tables are in fact implied completely by the SoC compatible
string.

Moving all of this data into device tree has a number of disadvantages:

  * Existing boards already use the static tables in the driver, and the
    device trees don't contain any data, so you can't get rid of any of
    the existing tables because it would break ABI.

  * Moving the table into the DT doesn't actually solve anything because
    the driver would have to validate the DT description to make sure it
    contains valid data. And in order to validate DT content, the driver
    would need a copy of the table anyway.

I don't think you're going to do yourself any favours by pushing this. I
also don't see the commit description give any reason why you want to
move the table into device tree. Do you see any advantages in doing so?

Thierry

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

[RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Thierry Reding]

On Fri, Nov 24, 2017 at 11:19:52AM +0100, Linus Walleij wrote:
> On Mon, Nov 13, 2017 at 2:25 AM, Andre Przywara <andre.przywara@arm.com> wrote:
> 
> > So far all the Allwinner pinctrl drivers provided a table in the
> > kernel to describe all the pins and the link between the pinctrl functions
> > names (strings) and their respective mux values (register values).
> >
> > Extend the binding to put those mappings in the DT, so that any SoC can
> > describe its pinctrl and GPIO data fully there instead of relying on
> > tables.
> > This uses a generic compatible name, to be prepended with an SoC
> > specific name in the node.

This seems backwards to me. I'm not sure if Rob has any hard rules on
this, but in the past I've seen a lot of drivers stick this kind of data
into drivers. I personally also prefer that approach because the data is
completely static and there's no way for any specific board to customize
it. So the tables are in fact implied completely by the SoC compatible
string.

Moving all of this data into device tree has a number of disadvantages:

  * Existing boards already use the static tables in the driver, and the
    device trees don't contain any data, so you can't get rid of any of
    the existing tables because it would break ABI.

  * Moving the table into the DT doesn't actually solve anything because
    the driver would have to validate the DT description to make sure it
    contains valid data. And in order to validate DT content, the driver
    would need a copy of the table anyway.

I don't think you're going to do yourself any favours by pushing this. I
also don't see the commit description give any reason why you want to
move the table into device tree. Do you see any advantages in doing so?

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20171124/70f02bb4/attachment.sig>

Re: [RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Andre Przywara]

Hi,

(adding linux-sunxi, which I forgot at the initial post).

On 24/11/17 10:52, Thierry Reding wrote:
> On Fri, Nov 24, 2017 at 11:19:52AM +0100, Linus Walleij wrote:
>> On Mon, Nov 13, 2017 at 2:25 AM, Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org> wrote:
>>
>>> So far all the Allwinner pinctrl drivers provided a table in the
>>> kernel to describe all the pins and the link between the pinctrl functions
>>> names (strings) and their respective mux values (register values).
>>>
>>> Extend the binding to put those mappings in the DT, so that any SoC can
>>> describe its pinctrl and GPIO data fully there instead of relying on
>>> tables.
>>> This uses a generic compatible name, to be prepended with an SoC
>>> specific name in the node.
> 
> This seems backwards to me. I'm not sure if Rob has any hard rules on
> this, but in the past I've seen a lot of drivers stick this kind of data
> into drivers. I personally also prefer that approach because the data is
> completely static and there's no way for any specific board to customize
> it. So the tables are in fact implied completely by the SoC compatible
> string.

But this is just *data*, and I believe it is actually package specific.
We need the DT to describe the relation between devices and pins anyway,
it's just that we use arbitrary strings today instead of the actual
register value. This is what the generic pinmux property fixes.

> Moving all of this data into device tree has a number of disadvantages:
> 
>   * Existing boards already use the static tables in the driver, and the
>     device trees don't contain any data, so you can't get rid of any of
>     the existing tables because it would break ABI.

Yes, my DeLorean is in the garage, so I can't really change this anymore
;-) But that doesn't mean we have to go on with this forever, I think.

>   * Moving the table into the DT doesn't actually solve anything because
>     the driver would have to validate the DT description to make sure it
>     contains valid data. And in order to validate DT content, the driver
>     would need a copy of the table anyway.

I don't get what the driver would need to validate? We rely on DT
information to be correct anyway, otherwise your board just won't work.
If the DT is wrong, you have much bigger problems.

Actually we gain something, because we only commit information that can
actually be tested. Right now we have a lot of information which is
copied from the manual, and nobody knows if pin H24 on the A10 is really
PATA-CS1 or not. Plus we have bugs when creating the table, plus
copy&paste bugs. I found some while grep-ing for patterns - will send
fixes ASAP.

In the moment all the table gives us is a mapping between a *string* and
the respective mux register value (per pin), plus the number of pins in
each bank. This can *easily* be put in the DT and should belong there.

Actually I believe that the current binding is not correct, because it
makes those mux strings a part of the binding, though this is not
documented anywhere. A developer cannot take the binding and write a
working driver or even a DT without looking at the code.
Plus we already changed those names in the past (for instance commit
bc0f566a98c4), basically breaking compatibility.

> I don't think you're going to do yourself any favours by pushing this. I
> also don't see the commit description give any reason why you want to
> move the table into device tree. Do you see any advantages in doing so?

We stop adding tables with SoC specific *data* in the kernel *code*
base. With being boolean Kconfig options, this gets added to every
single-image kernel.

More important: those tables help Linux, but other DT consumers (*BSD,
U-Boot) have to replicate them, which is just wrong, IMHO.

I believe the kernel is a nice collection of really good code for
complicated file systems, high performance network protocols and
sophisticated memory management, among others. It shouldn't be a dumping
ground for arbitrary, very SoC specific information. Cf. LinusT 2011.
DT is out there to fix this, so we should do so.

Cheers,
Andre.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Andre Przywara]

Hi,

(adding linux-sunxi, which I forgot at the initial post).

On 24/11/17 10:52, Thierry Reding wrote:
> On Fri, Nov 24, 2017 at 11:19:52AM +0100, Linus Walleij wrote:
>> On Mon, Nov 13, 2017 at 2:25 AM, Andre Przywara <andre.przywara@arm.com> wrote:
>>
>>> So far all the Allwinner pinctrl drivers provided a table in the
>>> kernel to describe all the pins and the link between the pinctrl functions
>>> names (strings) and their respective mux values (register values).
>>>
>>> Extend the binding to put those mappings in the DT, so that any SoC can
>>> describe its pinctrl and GPIO data fully there instead of relying on
>>> tables.
>>> This uses a generic compatible name, to be prepended with an SoC
>>> specific name in the node.
> 
> This seems backwards to me. I'm not sure if Rob has any hard rules on
> this, but in the past I've seen a lot of drivers stick this kind of data
> into drivers. I personally also prefer that approach because the data is
> completely static and there's no way for any specific board to customize
> it. So the tables are in fact implied completely by the SoC compatible
> string.

But this is just *data*, and I believe it is actually package specific.
We need the DT to describe the relation between devices and pins anyway,
it's just that we use arbitrary strings today instead of the actual
register value. This is what the generic pinmux property fixes.

> Moving all of this data into device tree has a number of disadvantages:
> 
>   * Existing boards already use the static tables in the driver, and the
>     device trees don't contain any data, so you can't get rid of any of
>     the existing tables because it would break ABI.

Yes, my DeLorean is in the garage, so I can't really change this anymore
;-) But that doesn't mean we have to go on with this forever, I think.

>   * Moving the table into the DT doesn't actually solve anything because
>     the driver would have to validate the DT description to make sure it
>     contains valid data. And in order to validate DT content, the driver
>     would need a copy of the table anyway.

I don't get what the driver would need to validate? We rely on DT
information to be correct anyway, otherwise your board just won't work.
If the DT is wrong, you have much bigger problems.

Actually we gain something, because we only commit information that can
actually be tested. Right now we have a lot of information which is
copied from the manual, and nobody knows if pin H24 on the A10 is really
PATA-CS1 or not. Plus we have bugs when creating the table, plus
copy&paste bugs. I found some while grep-ing for patterns - will send
fixes ASAP.

In the moment all the table gives us is a mapping between a *string* and
the respective mux register value (per pin), plus the number of pins in
each bank. This can *easily* be put in the DT and should belong there.

Actually I believe that the current binding is not correct, because it
makes those mux strings a part of the binding, though this is not
documented anywhere. A developer cannot take the binding and write a
working driver or even a DT without looking at the code.
Plus we already changed those names in the past (for instance commit
bc0f566a98c4), basically breaking compatibility.

> I don't think you're going to do yourself any favours by pushing this. I
> also don't see the commit description give any reason why you want to
> move the table into device tree. Do you see any advantages in doing so?

We stop adding tables with SoC specific *data* in the kernel *code*
base. With being boolean Kconfig options, this gets added to every
single-image kernel.

More important: those tables help Linux, but other DT consumers (*BSD,
U-Boot) have to replicate them, which is just wrong, IMHO.

I believe the kernel is a nice collection of really good code for
complicated file systems, high performance network protocols and
sophisticated memory management, among others. It shouldn't be a dumping
ground for arbitrary, very SoC specific information. Cf. LinusT 2011.
DT is out there to fix this, so we should do so.

Cheers,
Andre.

Re: [RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Thierry Reding]

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

On Fri, Nov 24, 2017 at 12:04:12PM +0000, Andre Przywara wrote:
> Hi,
> 
> (adding linux-sunxi, which I forgot at the initial post).
> 
> On 24/11/17 10:52, Thierry Reding wrote:
> > On Fri, Nov 24, 2017 at 11:19:52AM +0100, Linus Walleij wrote:
> >> On Mon, Nov 13, 2017 at 2:25 AM, Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org> wrote:
> >>
> >>> So far all the Allwinner pinctrl drivers provided a table in the
> >>> kernel to describe all the pins and the link between the pinctrl functions
> >>> names (strings) and their respective mux values (register values).
> >>>
> >>> Extend the binding to put those mappings in the DT, so that any SoC can
> >>> describe its pinctrl and GPIO data fully there instead of relying on
> >>> tables.
> >>> This uses a generic compatible name, to be prepended with an SoC
> >>> specific name in the node.
> > 
> > This seems backwards to me. I'm not sure if Rob has any hard rules on
> > this, but in the past I've seen a lot of drivers stick this kind of data
> > into drivers. I personally also prefer that approach because the data is
> > completely static and there's no way for any specific board to customize
> > it. So the tables are in fact implied completely by the SoC compatible
> > string.
> 
> But this is just *data*, and I believe it is actually package specific.
> We need the DT to describe the relation between devices and pins anyway,
> it's just that we use arbitrary strings today instead of the actual
> register value. This is what the generic pinmux property fixes.

Register values don't belong in a device tree. And it's totally fine to
have data in the kernel, too. We do it all the time.

> > Moving all of this data into device tree has a number of disadvantages:
> > 
> >   * Existing boards already use the static tables in the driver, and the
> >     device trees don't contain any data, so you can't get rid of any of
> >     the existing tables because it would break ABI.
> 
> Yes, my DeLorean is in the garage, so I can't really change this anymore
> ;-) But that doesn't mean we have to go on with this forever, I think.

ABI stability means that, yes, you have to keep maintaining the existing
tables forever, else old DTBs will stop working if you rip out the
static tables that drivers depend on.

> >   * Moving the table into the DT doesn't actually solve anything because
> >     the driver would have to validate the DT description to make sure it
> >     contains valid data. And in order to validate DT content, the driver
> >     would need a copy of the table anyway.
> 
> I don't get what the driver would need to validate? We rely on DT
> information to be correct anyway, otherwise your board just won't work.
> If the DT is wrong, you have much bigger problems.

Given that DT is an ABI you should treat it the same way as other ABIs.
You can't rely on the DT being correct. Rather you have to make sure to
validate it before you hand the content to hardware. If you allow direct
register access to your hardware via DT and don't validate, it becomes
really easy for people to exploit it.

This is not the same as saying we need to be able to fully validate all
aspects of device tree. We can't, because some information simply does
not exist outside of DT. However, I think it's a big mistake to trust a
user to fully know about all intricacies of a pinmux and not make any
mistake when writing the device tree. What if one of the settings causes
the board to go up in flames?

> Actually we gain something, because we only commit information that can
> actually be tested. Right now we have a lot of information which is
> copied from the manual, and nobody knows if pin H24 on the A10 is really
> PATA-CS1 or not. Plus we have bugs when creating the table, plus
> copy&paste bugs. I found some while grep-ing for patterns - will send
> fixes ASAP.

That's a different matter. If you've got bugs in the tables, then go fix
the tables. However the assumption here is that you've done at least a
minimum of testing and your driver didn't cause your board to go up in
flames. When patches were posted, people had the opportunity to review
the tables for correctness. However, if you put all of the flexibility
into DT, you also put all of the risk there. People may just make some
stupid mistake and cause physical damage to their hardware.

> In the moment all the table gives us is a mapping between a *string* and
> the respective mux register value (per pin), plus the number of pins in
> each bank. This can *easily* be put in the DT and should belong there.

Why? This is data that is implied by the compatible string and static
per SoC. There is no way you can change the mapping in DT. What does
need to go into DT is the configuration of the pinmux, that is, what
function is used for each pin on a given board.

> Actually I believe that the current binding is not correct, because it
> makes those mux strings a part of the binding, though this is not
> documented anywhere. A developer cannot take the binding and write a
> working driver or even a DT without looking at the code.
> Plus we already changed those names in the past (for instance commit
> bc0f566a98c4), basically breaking compatibility.

If you haven't documented the strings your binding is not complete.
That's a bug and should be fixed. Also, it is occasionally acceptable to
break compatibility (it's technically only breaking if somebody notices)
and fixing bugs in bindings has in the past been one of the exceptions
where breaking ABI was specifically allowed.

However, the kind of breakage we're talking about here is total. If you
rip out the static tables from your driver, you don't have any data to
replace the missing information and none of the driver will work. This
is different from the driver erroring out trying to configure a pin for
the NAND function because it couldn't match the name.

Also, device tree bindings are not documentation for how to write a
driver. They are not a replacement for hardware documentation. Nobody
should be expected to be able to write an OS driver solely based on a
device tree binding. Device tree bindings are more of a configuration
interface specification for OS drivers.

> > I don't think you're going to do yourself any favours by pushing this. I
> > also don't see the commit description give any reason why you want to
> > move the table into device tree. Do you see any advantages in doing so?
> 
> We stop adding tables with SoC specific *data* in the kernel *code*
> base. With being boolean Kconfig options, this gets added to every
> single-image kernel.

The kernel is full of data:

	$ objdump -h build/arm64/vmlinux
	[...]
	  1 .text         009c99c0  ffff000008081000  ffff000008081000  00011000  2**11
	                  CONTENTS, ALLOC, LOAD, READONLY, CODE
	  2 .rodata       00403bf8  ffff000008a50000  ffff000008a50000  009e0000  2**12
	                  CONTENTS, ALLOC, LOAD, DATA
	[...]

So that's about 40% of the kernel image. Code really is no good without
data to process.

> More important: those tables help Linux, but other DT consumers (*BSD,
> U-Boot) have to replicate them, which is just wrong, IMHO.

Yeah, I've heard this before. To be honest, I think these tables are the
kind of data that you should generate, and once you do that it becomes
extremely cheap to add the data to other DT consumers.

And let's face it: the really difficult part of adding pinmux support is
to write the driver (or subsystem if you don't have one yet). Adding the
data is really the easy part.

> I believe the kernel is a nice collection of really good code for
> complicated file systems, high performance network protocols and
> sophisticated memory management, among others. It shouldn't be a dumping
> ground for arbitrary, very SoC specific information. Cf. LinusT 2011.
> DT is out there to fix this, so we should do so.

Every driver is very SoC specific information. There's never been an
objection to having SoC specific drivers in the kernel. And back at the
time the discussion was as much about the development process and code
structure than it was about board files.

The majority of the improvements over the years have been achieved by
moving drivers out of arch/arm and moving board files to DT. The goal
was never to get rid of all data.

Thierry

[RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Thierry Reding]

On Fri, Nov 24, 2017 at 12:04:12PM +0000, Andre Przywara wrote:
> Hi,
> 
> (adding linux-sunxi, which I forgot at the initial post).
> 
> On 24/11/17 10:52, Thierry Reding wrote:
> > On Fri, Nov 24, 2017 at 11:19:52AM +0100, Linus Walleij wrote:
> >> On Mon, Nov 13, 2017 at 2:25 AM, Andre Przywara <andre.przywara@arm.com> wrote:
> >>
> >>> So far all the Allwinner pinctrl drivers provided a table in the
> >>> kernel to describe all the pins and the link between the pinctrl functions
> >>> names (strings) and their respective mux values (register values).
> >>>
> >>> Extend the binding to put those mappings in the DT, so that any SoC can
> >>> describe its pinctrl and GPIO data fully there instead of relying on
> >>> tables.
> >>> This uses a generic compatible name, to be prepended with an SoC
> >>> specific name in the node.
> > 
> > This seems backwards to me. I'm not sure if Rob has any hard rules on
> > this, but in the past I've seen a lot of drivers stick this kind of data
> > into drivers. I personally also prefer that approach because the data is
> > completely static and there's no way for any specific board to customize
> > it. So the tables are in fact implied completely by the SoC compatible
> > string.
> 
> But this is just *data*, and I believe it is actually package specific.
> We need the DT to describe the relation between devices and pins anyway,
> it's just that we use arbitrary strings today instead of the actual
> register value. This is what the generic pinmux property fixes.

Register values don't belong in a device tree. And it's totally fine to
have data in the kernel, too. We do it all the time.

> > Moving all of this data into device tree has a number of disadvantages:
> > 
> >   * Existing boards already use the static tables in the driver, and the
> >     device trees don't contain any data, so you can't get rid of any of
> >     the existing tables because it would break ABI.
> 
> Yes, my DeLorean is in the garage, so I can't really change this anymore
> ;-) But that doesn't mean we have to go on with this forever, I think.

ABI stability means that, yes, you have to keep maintaining the existing
tables forever, else old DTBs will stop working if you rip out the
static tables that drivers depend on.

> >   * Moving the table into the DT doesn't actually solve anything because
> >     the driver would have to validate the DT description to make sure it
> >     contains valid data. And in order to validate DT content, the driver
> >     would need a copy of the table anyway.
> 
> I don't get what the driver would need to validate? We rely on DT
> information to be correct anyway, otherwise your board just won't work.
> If the DT is wrong, you have much bigger problems.

Given that DT is an ABI you should treat it the same way as other ABIs.
You can't rely on the DT being correct. Rather you have to make sure to
validate it before you hand the content to hardware. If you allow direct
register access to your hardware via DT and don't validate, it becomes
really easy for people to exploit it.

This is not the same as saying we need to be able to fully validate all
aspects of device tree. We can't, because some information simply does
not exist outside of DT. However, I think it's a big mistake to trust a
user to fully know about all intricacies of a pinmux and not make any
mistake when writing the device tree. What if one of the settings causes
the board to go up in flames?

> Actually we gain something, because we only commit information that can
> actually be tested. Right now we have a lot of information which is
> copied from the manual, and nobody knows if pin H24 on the A10 is really
> PATA-CS1 or not. Plus we have bugs when creating the table, plus
> copy&paste bugs. I found some while grep-ing for patterns - will send
> fixes ASAP.

That's a different matter. If you've got bugs in the tables, then go fix
the tables. However the assumption here is that you've done at least a
minimum of testing and your driver didn't cause your board to go up in
flames. When patches were posted, people had the opportunity to review
the tables for correctness. However, if you put all of the flexibility
into DT, you also put all of the risk there. People may just make some
stupid mistake and cause physical damage to their hardware.

> In the moment all the table gives us is a mapping between a *string* and
> the respective mux register value (per pin), plus the number of pins in
> each bank. This can *easily* be put in the DT and should belong there.

Why? This is data that is implied by the compatible string and static
per SoC. There is no way you can change the mapping in DT. What does
need to go into DT is the configuration of the pinmux, that is, what
function is used for each pin on a given board.

> Actually I believe that the current binding is not correct, because it
> makes those mux strings a part of the binding, though this is not
> documented anywhere. A developer cannot take the binding and write a
> working driver or even a DT without looking at the code.
> Plus we already changed those names in the past (for instance commit
> bc0f566a98c4), basically breaking compatibility.

If you haven't documented the strings your binding is not complete.
That's a bug and should be fixed. Also, it is occasionally acceptable to
break compatibility (it's technically only breaking if somebody notices)
and fixing bugs in bindings has in the past been one of the exceptions
where breaking ABI was specifically allowed.

However, the kind of breakage we're talking about here is total. If you
rip out the static tables from your driver, you don't have any data to
replace the missing information and none of the driver will work. This
is different from the driver erroring out trying to configure a pin for
the NAND function because it couldn't match the name.

Also, device tree bindings are not documentation for how to write a
driver. They are not a replacement for hardware documentation. Nobody
should be expected to be able to write an OS driver solely based on a
device tree binding. Device tree bindings are more of a configuration
interface specification for OS drivers.

> > I don't think you're going to do yourself any favours by pushing this. I
> > also don't see the commit description give any reason why you want to
> > move the table into device tree. Do you see any advantages in doing so?
> 
> We stop adding tables with SoC specific *data* in the kernel *code*
> base. With being boolean Kconfig options, this gets added to every
> single-image kernel.

The kernel is full of data:

	$ objdump -h build/arm64/vmlinux
	[...]
	  1 .text         009c99c0  ffff000008081000  ffff000008081000  00011000  2**11
	                  CONTENTS, ALLOC, LOAD, READONLY, CODE
	  2 .rodata       00403bf8  ffff000008a50000  ffff000008a50000  009e0000  2**12
	                  CONTENTS, ALLOC, LOAD, DATA
	[...]

So that's about 40% of the kernel image. Code really is no good without
data to process.

> More important: those tables help Linux, but other DT consumers (*BSD,
> U-Boot) have to replicate them, which is just wrong, IMHO.

Yeah, I've heard this before. To be honest, I think these tables are the
kind of data that you should generate, and once you do that it becomes
extremely cheap to add the data to other DT consumers.

And let's face it: the really difficult part of adding pinmux support is
to write the driver (or subsystem if you don't have one yet). Adding the
data is really the easy part.

> I believe the kernel is a nice collection of really good code for
> complicated file systems, high performance network protocols and
> sophisticated memory management, among others. It shouldn't be a dumping
> ground for arbitrary, very SoC specific information. Cf. LinusT 2011.
> DT is out there to fix this, so we should do so.

Every driver is very SoC specific information. There's never been an
objection to having SoC specific drivers in the kernel. And back at the
time the discussion was as much about the development process and code
structure than it was about board files.

The majority of the improvements over the years have been achieved by
moving drivers out of arch/arm and moving board files to DT. The goal
was never to get rid of all data.

Thierry
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20171124/cf82009f/attachment.sig>

Re: [RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Andre Przywara]

Hi Thierry,

thanks for the reply and having this discussion!

On 24/11/17 13:31, Thierry Reding wrote:
> On Fri, Nov 24, 2017 at 12:04:12PM +0000, Andre Przywara wrote:
>> Hi,
>>
>> (adding linux-sunxi, which I forgot at the initial post).
>>
>> On 24/11/17 10:52, Thierry Reding wrote:
>>> On Fri, Nov 24, 2017 at 11:19:52AM +0100, Linus Walleij wrote:
>>>> On Mon, Nov 13, 2017 at 2:25 AM, Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org> wrote:
>>>>
>>>>> So far all the Allwinner pinctrl drivers provided a table in the
>>>>> kernel to describe all the pins and the link between the pinctrl functions
>>>>> names (strings) and their respective mux values (register values).
>>>>>
>>>>> Extend the binding to put those mappings in the DT, so that any SoC can
>>>>> describe its pinctrl and GPIO data fully there instead of relying on
>>>>> tables.
>>>>> This uses a generic compatible name, to be prepended with an SoC
>>>>> specific name in the node.
>>>
>>> This seems backwards to me. I'm not sure if Rob has any hard rules on
>>> this, but in the past I've seen a lot of drivers stick this kind of data
>>> into drivers. I personally also prefer that approach because the data is
>>> completely static and there's no way for any specific board to customize
>>> it. So the tables are in fact implied completely by the SoC compatible
>>> string.
>>
>> But this is just *data*, and I believe it is actually package specific.
>> We need the DT to describe the relation between devices and pins anyway,
>> it's just that we use arbitrary strings today instead of the actual
>> register value. This is what the generic pinmux property fixes.
> 
> Register values don't belong in a device tree.

I don't think that's true in a general way. This "pinmux" is already an
accepted property and used for exactly that purpose in other pinctrl
drivers:
$ git grep -l '[^,]pinmux = ' arch/arm{,64}/boot/dts
Plus the fsl,pinmux-ids property, which seems to serve the same purpose.

> And it's totally fine to
> have data in the kernel, too. We do it all the time.

I know, but that does not mean that I need to like it or should avoid
finding better solutions.

>>> Moving all of this data into device tree has a number of disadvantages:
>>>
>>>   * Existing boards already use the static tables in the driver, and the
>>>     device trees don't contain any data, so you can't get rid of any of
>>>     the existing tables because it would break ABI.
>>
>> Yes, my DeLorean is in the garage, so I can't really change this anymore
>> ;-) But that doesn't mean we have to go on with this forever, I think.
> 
> ABI stability means that, yes, you have to keep maintaining the existing
> tables forever, else old DTBs will stop working if you rip out the
> static tables that drivers depend on.

I think you got me wrong, I am totally not arguing ABI stability and its
consequences. That's why this binding is mostly for newer SoCs, but also
designed to stay compatible with the old binding - that's for other DT
users. Of course we need to keep the tables in the kernel.

>>>   * Moving the table into the DT doesn't actually solve anything because
>>>     the driver would have to validate the DT description to make sure it
>>>     contains valid data. And in order to validate DT content, the driver
>>>     would need a copy of the table anyway.
>>
>> I don't get what the driver would need to validate? We rely on DT
>> information to be correct anyway, otherwise your board just won't work.
>> If the DT is wrong, you have much bigger problems.
> 
> Given that DT is an ABI you should treat it the same way as other ABIs.
> You can't rely on the DT being correct.

But it's not a *user* ABI, where unprivileged users can supply input data.
What else can you rely on then if not on the DT?
By definition you can't be able to validate every bit of it. How would
you know that the reg address of that SATA chip is correct and not
actually pointing to the "set-board-on-fire" device?
And what about those regulators, where it gives you the allowed voltage
range?

> Rather you have to make sure to
> validate it before you hand the content to hardware. If you allow direct
> register access to your hardware via DT and don't validate, it becomes
> really easy for people to exploit it.

If you can change the DT, you already have full control over the system.
Ideally the DT comes as part of the firmware, so if you control this,
there are far less complicated ways to burn down the system. And even if
you supply the DT with your kernel, you must have control and could
trigger the set-fire device via /dev/mem or the like.

> This is not the same as saying we need to be able to fully validate all
> aspects of device tree. We can't, because some information simply does
> not exist outside of DT. However, I think it's a big mistake to trust a
> user to fully know about all intricacies of a pinmux and not make any
> mistake when writing the device tree.

When does a *user* write a device tree? What would a clueless Joe
Average expect from doing so? The device tree is written by either the
board/SoC vendor or by some kernel developers. It is not meant to be
changed by the user, apart from carefully crafted overlays, maybe. You
can have the same control by just changing the kernel (binary patching
the image file or re-compiling with
writel(MATCHES, base_addr + SET_FIRE)).

> What if one of the settings causes the board to go up in flames?

Then you better not play with it. But I don't think this is a valid
argument, really. What if gravity reverses tomorrow?
Are there any known issues with writing mux values to pinctrl registers?
I don't think the consequences would be different from putting low or
high to a GPIO, which you can do already from userland.

>> Actually we gain something, because we only commit information that can
>> actually be tested. Right now we have a lot of information which is
>> copied from the manual, and nobody knows if pin H24 on the A10 is really
>> PATA-CS1 or not. Plus we have bugs when creating the table, plus
>> copy&paste bugs. I found some while grep-ing for patterns - will send
>> fixes ASAP.
> 
> That's a different matter. If you've got bugs in the tables, then go fix
> the tables. However the assumption here is that you've done at least a
> minimum of testing and your driver didn't cause your board to go up in
> flames.

Yes, but at the moment we can't test all of the settings we put in the
kernel table. Some pins are for devices for which we don't have drivers
in the kernel (yet), for some we probably never will (JTAG).
>From writing this table for the A64 before I know it's tedious and even
with review there are bugs left. If we can decrease this risk, I am all
for it.

> When patches were posted, people had the opportunity to review
> the tables for correctness. However, if you put all of the flexibility
> into DT, you also put all of the risk there. People may just make some
> stupid mistake and cause physical damage to their hardware.

I don't see how a DT is more risky. Conceptually I consider the DT being
part of the firmware, so one trust level above the kernel. I understand
that some people have different experiences with "firmware", but frankly
I believe this discussion is pointless. If you get the DT wrong, your
board won't work. If the system easily allows you to destroy it by means
of software, you might want to fix it in general, for instance by
telling firmware to not allow access to it from the kernel. Or by
designing hardware in a way that you don't need to expose critical
settings to software. That how x86 solves most of it and what makes it
hard to physically kill a rented root server.
Hiding stuff from the DT and putting molly guards in the kernel does not
really solve this problem.

>> In the moment all the table gives us is a mapping between a *string* and
>> the respective mux register value (per pin), plus the number of pins in
>> each bank. This can *easily* be put in the DT and should belong there.
> 
> Why? This is data that is implied by the compatible string and static
> per SoC. There is no way you can change the mapping in DT. What does
> need to go into DT is the configuration of the pinmux, that is, what
> function is used for each pin on a given board.

Exactly, and that does not change on the *board* level, because there we
just refer to the pinctrl subnode phandles, defined in <soc>.dtsi.
And it's just there that I want to add the right pinmux value.

>> Actually I believe that the current binding is not correct, because it
>> makes those mux strings a part of the binding, though this is not
>> documented anywhere. A developer cannot take the binding and write a
>> working driver or even a DT without looking at the code.
>> Plus we already changed those names in the past (for instance commit
>> bc0f566a98c4), basically breaking compatibility.
> 
> If you haven't documented the strings your binding is not complete.
> That's a bug and should be fixed. Also, it is occasionally acceptable to
> break compatibility (it's technically only breaking if somebody notices)
> and fixing bugs in bindings has in the past been one of the exceptions
> where breaking ABI was specifically allowed.
> 
> However, the kind of breakage we're talking about here is total. If you
> rip out the static tables from your driver, you don't have any data to
> replace the missing information and none of the driver will work. This
> is different from the driver erroring out trying to configure a pin for
> the NAND function because it couldn't match the name.

I think this is a misunderstanding. As mentioned above I never proposed
to remove the tables from the kernel. I think I said this explicitly in
several places:
PATCH 0/3:
> Of course we can't remove the existing tables, to keep compatiblity
> with older DTs, but at least we don't need to add more tables in the
> future.
PATCH 1/3:
> +The binding described above can be extended in this manner to be
supported
> +by *both* an existing driver and some generic driver. Existing
drivers will
> +ignore the new properties and revert to their internal table instead.

I totally understand the concept of compatibility, ask Maxime, he hates
me for that ;-)

> Also, device tree bindings are not documentation for how to write a
> driver. They are not a replacement for hardware documentation. Nobody
> should be expected to be able to write an OS driver solely based on a
> device tree binding. Device tree bindings are more of a configuration
> interface specification for OS drivers.

Yes, but together with the hardware docs you should be able to write a
driver. And here you can't, because you are missing the strings. So a
BSD developer has to look at Linux code.
Sure you can go ahead and add those strings to the binding, but I wonder
how complicated it has to get until we just go with the obviously
simplest way and just add this innocent mux value to the DT.

>>> I don't think you're going to do yourself any favours by pushing this. I
>>> also don't see the commit description give any reason why you want to
>>> move the table into device tree. Do you see any advantages in doing so?
>>
>> We stop adding tables with SoC specific *data* in the kernel *code*
>> base. With being boolean Kconfig options, this gets added to every
>> single-image kernel.
> 
> The kernel is full of data:
> 
> 	$ objdump -h build/arm64/vmlinux
> 	[...]
> 	  1 .text         009c99c0  ffff000008081000  ffff000008081000  00011000  2**11
> 	                  CONTENTS, ALLOC, LOAD, READONLY, CODE
> 	  2 .rodata       00403bf8  ffff000008a50000  ffff000008a50000  009e0000  2**12
> 	                  CONTENTS, ALLOC, LOAD, DATA
> 	[...]
> 
> So that's about 40% of the kernel image. Code really is no good without
> data to process.

But how much of this is SoC specific configuration data? How much is it
in x86? Yes, historically we had and have a lot of configuration data in
ARM kernels. But that doesn't mean that we have to continue with this or
even increase the share.

>> More important: those tables help Linux, but other DT consumers (*BSD,
>> U-Boot) have to replicate them, which is just wrong, IMHO.
> 
> Yeah, I've heard this before. To be honest, I think these tables are the
> kind of data that you should generate, and once you do that it becomes
> extremely cheap to add the data to other DT consumers.

I don't get what's the point of duplicating this information everywhere.
We already have places in the DT to put this information:
	function = "uart1";
So we add "pinmux = <2>" *once* to *the* DT, and everyone gets this for
free. If U-Boot wants to enable MMC0, it just follows the phandle from
pinctrl-0 and writes the value(s) from "pinmux" to the registers derived
from the member of the "pins" string list. No SoC knowledge required.

> And let's face it: the really difficult part of adding pinmux support is
> to write the driver (or subsystem if you don't have one yet). Adding the
> data is really the easy part.

I know, I did this before. But currently you have to write both the DT
part *and* the kernel table. And check patch 3/3, that's what the table
gets reduced to. So you avoid writing 601 lines, instead add 23 lines to
the DT.

>> I believe the kernel is a nice collection of really good code for
>> complicated file systems, high performance network protocols and
>> sophisticated memory management, among others. It shouldn't be a dumping
>> ground for arbitrary, very SoC specific information. Cf. LinusT 2011.
>> DT is out there to fix this, so we should do so.
> 
> Every driver is very SoC specific information. There's never been an
> objection to having SoC specific drivers in the kernel. And back at the
> time the discussion was as much about the development process and code
> structure than it was about board files.
> 
> The majority of the improvements over the years have been achieved by
> moving drivers out of arch/arm and moving board files to DT. The goal
> was never to get rid of all data.

Sure, not all data. But if we have the relatively easy opportunity to
avoid further addition of data, we should do it, I believe.
This significantly reduces the amount of kernel code we need to add to
support new SoCs.

Cheers,
Andre.

[RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Andre Przywara]

Hi Thierry,

thanks for the reply and having this discussion!

On 24/11/17 13:31, Thierry Reding wrote:
> On Fri, Nov 24, 2017 at 12:04:12PM +0000, Andre Przywara wrote:
>> Hi,
>>
>> (adding linux-sunxi, which I forgot at the initial post).
>>
>> On 24/11/17 10:52, Thierry Reding wrote:
>>> On Fri, Nov 24, 2017 at 11:19:52AM +0100, Linus Walleij wrote:
>>>> On Mon, Nov 13, 2017 at 2:25 AM, Andre Przywara <andre.przywara@arm.com> wrote:
>>>>
>>>>> So far all the Allwinner pinctrl drivers provided a table in the
>>>>> kernel to describe all the pins and the link between the pinctrl functions
>>>>> names (strings) and their respective mux values (register values).
>>>>>
>>>>> Extend the binding to put those mappings in the DT, so that any SoC can
>>>>> describe its pinctrl and GPIO data fully there instead of relying on
>>>>> tables.
>>>>> This uses a generic compatible name, to be prepended with an SoC
>>>>> specific name in the node.
>>>
>>> This seems backwards to me. I'm not sure if Rob has any hard rules on
>>> this, but in the past I've seen a lot of drivers stick this kind of data
>>> into drivers. I personally also prefer that approach because the data is
>>> completely static and there's no way for any specific board to customize
>>> it. So the tables are in fact implied completely by the SoC compatible
>>> string.
>>
>> But this is just *data*, and I believe it is actually package specific.
>> We need the DT to describe the relation between devices and pins anyway,
>> it's just that we use arbitrary strings today instead of the actual
>> register value. This is what the generic pinmux property fixes.
> 
> Register values don't belong in a device tree.

I don't think that's true in a general way. This "pinmux" is already an
accepted property and used for exactly that purpose in other pinctrl
drivers:
$ git grep -l '[^,]pinmux = ' arch/arm{,64}/boot/dts
Plus the fsl,pinmux-ids property, which seems to serve the same purpose.

> And it's totally fine to
> have data in the kernel, too. We do it all the time.

I know, but that does not mean that I need to like it or should avoid
finding better solutions.

>>> Moving all of this data into device tree has a number of disadvantages:
>>>
>>>   * Existing boards already use the static tables in the driver, and the
>>>     device trees don't contain any data, so you can't get rid of any of
>>>     the existing tables because it would break ABI.
>>
>> Yes, my DeLorean is in the garage, so I can't really change this anymore
>> ;-) But that doesn't mean we have to go on with this forever, I think.
> 
> ABI stability means that, yes, you have to keep maintaining the existing
> tables forever, else old DTBs will stop working if you rip out the
> static tables that drivers depend on.

I think you got me wrong, I am totally not arguing ABI stability and its
consequences. That's why this binding is mostly for newer SoCs, but also
designed to stay compatible with the old binding - that's for other DT
users. Of course we need to keep the tables in the kernel.

>>>   * Moving the table into the DT doesn't actually solve anything because
>>>     the driver would have to validate the DT description to make sure it
>>>     contains valid data. And in order to validate DT content, the driver
>>>     would need a copy of the table anyway.
>>
>> I don't get what the driver would need to validate? We rely on DT
>> information to be correct anyway, otherwise your board just won't work.
>> If the DT is wrong, you have much bigger problems.
> 
> Given that DT is an ABI you should treat it the same way as other ABIs.
> You can't rely on the DT being correct.

But it's not a *user* ABI, where unprivileged users can supply input data.
What else can you rely on then if not on the DT?
By definition you can't be able to validate every bit of it. How would
you know that the reg address of that SATA chip is correct and not
actually pointing to the "set-board-on-fire" device?
And what about those regulators, where it gives you the allowed voltage
range?

> Rather you have to make sure to
> validate it before you hand the content to hardware. If you allow direct
> register access to your hardware via DT and don't validate, it becomes
> really easy for people to exploit it.

If you can change the DT, you already have full control over the system.
Ideally the DT comes as part of the firmware, so if you control this,
there are far less complicated ways to burn down the system. And even if
you supply the DT with your kernel, you must have control and could
trigger the set-fire device via /dev/mem or the like.

> This is not the same as saying we need to be able to fully validate all
> aspects of device tree. We can't, because some information simply does
> not exist outside of DT. However, I think it's a big mistake to trust a
> user to fully know about all intricacies of a pinmux and not make any
> mistake when writing the device tree.

When does a *user* write a device tree? What would a clueless Joe
Average expect from doing so? The device tree is written by either the
board/SoC vendor or by some kernel developers. It is not meant to be
changed by the user, apart from carefully crafted overlays, maybe. You
can have the same control by just changing the kernel (binary patching
the image file or re-compiling with
writel(MATCHES, base_addr + SET_FIRE)).

> What if one of the settings causes the board to go up in flames?

Then you better not play with it. But I don't think this is a valid
argument, really. What if gravity reverses tomorrow?
Are there any known issues with writing mux values to pinctrl registers?
I don't think the consequences would be different from putting low or
high to a GPIO, which you can do already from userland.

>> Actually we gain something, because we only commit information that can
>> actually be tested. Right now we have a lot of information which is
>> copied from the manual, and nobody knows if pin H24 on the A10 is really
>> PATA-CS1 or not. Plus we have bugs when creating the table, plus
>> copy&paste bugs. I found some while grep-ing for patterns - will send
>> fixes ASAP.
> 
> That's a different matter. If you've got bugs in the tables, then go fix
> the tables. However the assumption here is that you've done at least a
> minimum of testing and your driver didn't cause your board to go up in
> flames.

Yes, but at the moment we can't test all of the settings we put in the
kernel table. Some pins are for devices for which we don't have drivers
in the kernel (yet), for some we probably never will (JTAG).
>From writing this table for the A64 before I know it's tedious and even
with review there are bugs left. If we can decrease this risk, I am all
for it.

> When patches were posted, people had the opportunity to review
> the tables for correctness. However, if you put all of the flexibility
> into DT, you also put all of the risk there. People may just make some
> stupid mistake and cause physical damage to their hardware.

I don't see how a DT is more risky. Conceptually I consider the DT being
part of the firmware, so one trust level above the kernel. I understand
that some people have different experiences with "firmware", but frankly
I believe this discussion is pointless. If you get the DT wrong, your
board won't work. If the system easily allows you to destroy it by means
of software, you might want to fix it in general, for instance by
telling firmware to not allow access to it from the kernel. Or by
designing hardware in a way that you don't need to expose critical
settings to software. That how x86 solves most of it and what makes it
hard to physically kill a rented root server.
Hiding stuff from the DT and putting molly guards in the kernel does not
really solve this problem.

>> In the moment all the table gives us is a mapping between a *string* and
>> the respective mux register value (per pin), plus the number of pins in
>> each bank. This can *easily* be put in the DT and should belong there.
> 
> Why? This is data that is implied by the compatible string and static
> per SoC. There is no way you can change the mapping in DT. What does
> need to go into DT is the configuration of the pinmux, that is, what
> function is used for each pin on a given board.

Exactly, and that does not change on the *board* level, because there we
just refer to the pinctrl subnode phandles, defined in <soc>.dtsi.
And it's just there that I want to add the right pinmux value.

>> Actually I believe that the current binding is not correct, because it
>> makes those mux strings a part of the binding, though this is not
>> documented anywhere. A developer cannot take the binding and write a
>> working driver or even a DT without looking at the code.
>> Plus we already changed those names in the past (for instance commit
>> bc0f566a98c4), basically breaking compatibility.
> 
> If you haven't documented the strings your binding is not complete.
> That's a bug and should be fixed. Also, it is occasionally acceptable to
> break compatibility (it's technically only breaking if somebody notices)
> and fixing bugs in bindings has in the past been one of the exceptions
> where breaking ABI was specifically allowed.
> 
> However, the kind of breakage we're talking about here is total. If you
> rip out the static tables from your driver, you don't have any data to
> replace the missing information and none of the driver will work. This
> is different from the driver erroring out trying to configure a pin for
> the NAND function because it couldn't match the name.

I think this is a misunderstanding. As mentioned above I never proposed
to remove the tables from the kernel. I think I said this explicitly in
several places:
PATCH 0/3:
> Of course we can't remove the existing tables, to keep compatiblity
> with older DTs, but at least we don't need to add more tables in the
> future.
PATCH 1/3:
> +The binding described above can be extended in this manner to be
supported
> +by *both* an existing driver and some generic driver. Existing
drivers will
> +ignore the new properties and revert to their internal table instead.

I totally understand the concept of compatibility, ask Maxime, he hates
me for that ;-)

> Also, device tree bindings are not documentation for how to write a
> driver. They are not a replacement for hardware documentation. Nobody
> should be expected to be able to write an OS driver solely based on a
> device tree binding. Device tree bindings are more of a configuration
> interface specification for OS drivers.

Yes, but together with the hardware docs you should be able to write a
driver. And here you can't, because you are missing the strings. So a
BSD developer has to look at Linux code.
Sure you can go ahead and add those strings to the binding, but I wonder
how complicated it has to get until we just go with the obviously
simplest way and just add this innocent mux value to the DT.

>>> I don't think you're going to do yourself any favours by pushing this. I
>>> also don't see the commit description give any reason why you want to
>>> move the table into device tree. Do you see any advantages in doing so?
>>
>> We stop adding tables with SoC specific *data* in the kernel *code*
>> base. With being boolean Kconfig options, this gets added to every
>> single-image kernel.
> 
> The kernel is full of data:
> 
> 	$ objdump -h build/arm64/vmlinux
> 	[...]
> 	  1 .text         009c99c0  ffff000008081000  ffff000008081000  00011000  2**11
> 	                  CONTENTS, ALLOC, LOAD, READONLY, CODE
> 	  2 .rodata       00403bf8  ffff000008a50000  ffff000008a50000  009e0000  2**12
> 	                  CONTENTS, ALLOC, LOAD, DATA
> 	[...]
> 
> So that's about 40% of the kernel image. Code really is no good without
> data to process.

But how much of this is SoC specific configuration data? How much is it
in x86? Yes, historically we had and have a lot of configuration data in
ARM kernels. But that doesn't mean that we have to continue with this or
even increase the share.

>> More important: those tables help Linux, but other DT consumers (*BSD,
>> U-Boot) have to replicate them, which is just wrong, IMHO.
> 
> Yeah, I've heard this before. To be honest, I think these tables are the
> kind of data that you should generate, and once you do that it becomes
> extremely cheap to add the data to other DT consumers.

I don't get what's the point of duplicating this information everywhere.
We already have places in the DT to put this information:
	function = "uart1";
So we add "pinmux = <2>" *once* to *the* DT, and everyone gets this for
free. If U-Boot wants to enable MMC0, it just follows the phandle from
pinctrl-0 and writes the value(s) from "pinmux" to the registers derived
from the member of the "pins" string list. No SoC knowledge required.

> And let's face it: the really difficult part of adding pinmux support is
> to write the driver (or subsystem if you don't have one yet). Adding the
> data is really the easy part.

I know, I did this before. But currently you have to write both the DT
part *and* the kernel table. And check patch 3/3, that's what the table
gets reduced to. So you avoid writing 601 lines, instead add 23 lines to
the DT.

>> I believe the kernel is a nice collection of really good code for
>> complicated file systems, high performance network protocols and
>> sophisticated memory management, among others. It shouldn't be a dumping
>> ground for arbitrary, very SoC specific information. Cf. LinusT 2011.
>> DT is out there to fix this, so we should do so.
> 
> Every driver is very SoC specific information. There's never been an
> objection to having SoC specific drivers in the kernel. And back at the
> time the discussion was as much about the development process and code
> structure than it was about board files.
> 
> The majority of the improvements over the years have been achieved by
> moving drivers out of arch/arm and moving board files to DT. The goal
> was never to get rid of all data.

Sure, not all data. But if we have the relatively easy opportunity to
avoid further addition of data, we should do it, I believe.
This significantly reduces the amount of kernel code we need to add to
support new SoCs.

Cheers,
Andre.

Re: [RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Maxime Ripard]

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

On Fri, Nov 24, 2017 at 05:19:24PM +0000, Andre Przywara wrote:
> > This is not the same as saying we need to be able to fully validate all
> > aspects of device tree. We can't, because some information simply does
> > not exist outside of DT. However, I think it's a big mistake to trust a
> > user to fully know about all intricacies of a pinmux and not make any
> > mistake when writing the device tree.
> 
> When does a *user* write a device tree? What would a clueless Joe
> Average expect from doing so? The device tree is written by either the
> board/SoC vendor or by some kernel developers. It is not meant to be
> changed by the user, apart from carefully crafted overlays, maybe. You
> can have the same control by just changing the kernel (binary patching
> the image file or re-compiling with
> writel(MATCHES, base_addr + SET_FIRE)).

I guess the expectation should be that it's a Joe Average user in the
Allwinner case at least. It's far from the exception that someone that
never got any kernel (or electronics) experience before jumps in,
creates a DT for the shiny new board they just received and then
submits it.

I guess it's more the case in the Allwinner case than for any other
SoCs I've worked on at least, probably because of its widespread use
in the low-end consumer market and their reputation amongst hackers,
but still, this is basically our default.

> > What if one of the settings causes the board to go up in flames?
> 
> Then you better not play with it. But I don't think this is a valid
> argument, really. What if gravity reverses tomorrow?
> Are there any known issues with writing mux values to pinctrl registers?
> I don't think the consequences would be different from putting low or
> high to a GPIO, which you can do already from userland.

I guess the difference would be that's it's active in your example,
while the pinmux will be set even if a user doesn't do anything. And I
can testify that you can very well permanently fry a board passively
using pinctrl :)

> > And let's face it: the really difficult part of adding pinmux support is
> > to write the driver (or subsystem if you don't have one yet). Adding the
> > data is really the easy part.
> 
> I know, I did this before. But currently you have to write both the DT
> part *and* the kernel table. And check patch 3/3, that's what the table
> gets reduced to. So you avoid writing 601 lines, instead add 23 lines to
> the DT.

And I'll just add here that if the data size is of concern, as it can
be in a bootloader, you can very well have partial tables. There's
plenty of devices that will be of no use in a bootloader.

Maxime

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

[RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Maxime Ripard]

On Fri, Nov 24, 2017 at 05:19:24PM +0000, Andre Przywara wrote:
> > This is not the same as saying we need to be able to fully validate all
> > aspects of device tree. We can't, because some information simply does
> > not exist outside of DT. However, I think it's a big mistake to trust a
> > user to fully know about all intricacies of a pinmux and not make any
> > mistake when writing the device tree.
> 
> When does a *user* write a device tree? What would a clueless Joe
> Average expect from doing so? The device tree is written by either the
> board/SoC vendor or by some kernel developers. It is not meant to be
> changed by the user, apart from carefully crafted overlays, maybe. You
> can have the same control by just changing the kernel (binary patching
> the image file or re-compiling with
> writel(MATCHES, base_addr + SET_FIRE)).

I guess the expectation should be that it's a Joe Average user in the
Allwinner case at least. It's far from the exception that someone that
never got any kernel (or electronics) experience before jumps in,
creates a DT for the shiny new board they just received and then
submits it.

I guess it's more the case in the Allwinner case than for any other
SoCs I've worked on at least, probably because of its widespread use
in the low-end consumer market and their reputation amongst hackers,
but still, this is basically our default.

> > What if one of the settings causes the board to go up in flames?
> 
> Then you better not play with it. But I don't think this is a valid
> argument, really. What if gravity reverses tomorrow?
> Are there any known issues with writing mux values to pinctrl registers?
> I don't think the consequences would be different from putting low or
> high to a GPIO, which you can do already from userland.

I guess the difference would be that's it's active in your example,
while the pinmux will be set even if a user doesn't do anything. And I
can testify that you can very well permanently fry a board passively
using pinctrl :)

> > And let's face it: the really difficult part of adding pinmux support is
> > to write the driver (or subsystem if you don't have one yet). Adding the
> > data is really the easy part.
> 
> I know, I did this before. But currently you have to write both the DT
> part *and* the kernel table. And check patch 3/3, that's what the table
> gets reduced to. So you avoid writing 601 lines, instead add 23 lines to
> the DT.

And I'll just add here that if the data size is of concern, as it can
be in a bootloader, you can very well have partial tables. There's
plenty of devices that will be of no use in a bootloader.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20171127/9b0e9e22/attachment.sig>

Re: [RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Linus Walleij]

On Fri, Nov 24, 2017 at 6:19 PM, Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org> wrote:
> On 24/11/17 13:31, Thierry Reding wrote:

>> Register values don't belong in a device tree.
>
> I don't think that's true in a general way. This "pinmux" is already an
> accepted property and used for exactly that purpose in other pinctrl
> drivers:
> $ git grep -l '[^,]pinmux = ' arch/arm{,64}/boot/dts
> Plus the fsl,pinmux-ids property, which seems to serve the same purpose.

There are several examples of register values (and register
numbers even!) being encoded in the kernel.

What we need to ask is whether that is good in the general case
or bad. I don't even think that has a clear answer, it's a grey area.

So we need to avoid "arguments from consistency" which reads
something like "you allowed this thing A so now you must allow
this thing B which is similar". It is not a helpful approach to the
problem.

Some drivers encode a bunch of data into the device tree,
pinctrl-single is the most extreme. This conflict between in-DT
and in-driver data storage has been there since pinctrl was created
and was the result of a compromise between OMAPs needs
and everyone else, especially Tegra.

The opinions on this - and it is really opinions, not facts - differ
between people and over time.

Resolving the conflicts is more about classical diplomacy than
science unfortunately. I used to think the christian trinity was
amusing and inconsistent, but nowadays I understand exactly how
the people who came up with the Nicean creed were reasoning.

What is paramount for me as subsystem maintainer is the fact
that this driver has an active maintainer. And maintainers
is what makes things manageable for me. It would be much easier
for you to have your way if you were submitting an entirely
new driver. Like this pinmux property, it was submitted by the
mediatek people because it fits their usecase/hardware especially
well.

Yours,
Linus Walleij

[RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Linus Walleij]

On Fri, Nov 24, 2017 at 6:19 PM, Andre Przywara <andre.przywara@arm.com> wrote:
> On 24/11/17 13:31, Thierry Reding wrote:

>> Register values don't belong in a device tree.
>
> I don't think that's true in a general way. This "pinmux" is already an
> accepted property and used for exactly that purpose in other pinctrl
> drivers:
> $ git grep -l '[^,]pinmux = ' arch/arm{,64}/boot/dts
> Plus the fsl,pinmux-ids property, which seems to serve the same purpose.

There are several examples of register values (and register
numbers even!) being encoded in the kernel.

What we need to ask is whether that is good in the general case
or bad. I don't even think that has a clear answer, it's a grey area.

So we need to avoid "arguments from consistency" which reads
something like "you allowed this thing A so now you must allow
this thing B which is similar". It is not a helpful approach to the
problem.

Some drivers encode a bunch of data into the device tree,
pinctrl-single is the most extreme. This conflict between in-DT
and in-driver data storage has been there since pinctrl was created
and was the result of a compromise between OMAPs needs
and everyone else, especially Tegra.

The opinions on this - and it is really opinions, not facts - differ
between people and over time.

Resolving the conflicts is more about classical diplomacy than
science unfortunately. I used to think the christian trinity was
amusing and inconsistent, but nowadays I understand exactly how
the people who came up with the Nicean creed were reasoning.

What is paramount for me as subsystem maintainer is the fact
that this driver has an active maintainer. And maintainers
is what makes things manageable for me. It would be much easier
for you to have your way if you were submitting an entirely
new driver. Like this pinmux property, it was submitted by the
mediatek people because it fits their usecase/hardware especially
well.

Yours,
Linus Walleij

Re: [RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Linus Walleij]

On Fri, Nov 24, 2017 at 6:19 PM, Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org> wrote:

> Conceptually I consider the DT being
> part of the firmware,

As it is a subset of open firmware it is obviously firmware.

> so one trust level above the kernel.

We are several kernel developers who don't trust firmware one
bit. Several disasters in ACPI has made me ever more convinced
that firmware should be trusted less than kernel code.

But this is all very academic.

>> Also, device tree bindings are not documentation for how to write a
>> driver. They are not a replacement for hardware documentation. Nobody
>> should be expected to be able to write an OS driver solely based on a
>> device tree binding. Device tree bindings are more of a configuration
>> interface specification for OS drivers.
>
> Yes, but together with the hardware docs you should be able to write a
> driver. And here you can't, because you are missing the strings. So a
> BSD developer has to look at Linux code.

This is a fair point. It appears in several drivers.

BSD or even Windows (would they use DT) would have to sit in the
back seat just like Linux has been doing for years when it comes
to the hopeless Windowsisms in the x86 BIOSes. I suspect some
Windows on ARM is already experiencing this, but in the ACPI world,
where, incidentally, the servers were being deployed for Linux first
and Windows had to follow their example. I bet they have been
swearing a lot in Redmond about that.

In general it's one of these areas where we can not be utopian about the
hardware descriptions, just fail gracefully in different ways.

I usually try to keep the IETF motto "rough consensus and running
code" in mind. I don't know if it helps in this discussion though.

>> So that's about 40% of the kernel image. Code really is no good without
>> data to process.
>
> But how much of this is SoC specific configuration data? How much is it
> in x86? Yes, historically we had and have a lot of configuration data in
> ARM kernels. But that doesn't mean that we have to continue with this or
> even increase the share.

What people have been doing is trying to have better Kconfig setups
and compile it out by doing kernel modules. It is a bit hopeless with
pin controllers: almost all of them have to be built in. And if they come
with a lot of data, yeah there you have a real good point.

It would be sad if the ARMv7 multiboot or Aarch64 kernel just grows
so that we can't use it but have to go back to shipping board-specific
kernels with a huge bunch of stuff compiled out.

I was hoping Moore's law would save us here :/

An option that has been discussed is better used of  __initdata
and similar tags, especially with built-in drivers. Sadly, this is
hurt by another snag: the compiler or linker file or whatever it is,
is preventing us from discarding any strings from the kernel.
And pin controllers tend to stack up a lot of these.

This is really sucky and something we should solve in general.
I'm not smart enough to tackle any of these problems myself, just
to see them and "Oh that's bad. Very bad."

>> The majority of the improvements over the years have been achieved by
>> moving drivers out of arch/arm and moving board files to DT. The goal
>> was never to get rid of all data.
>
> Sure, not all data. But if we have the relatively easy opportunity to
> avoid further addition of data, we should do it, I believe.
> This significantly reduces the amount of kernel code we need to add to
> support new SoCs.

This is the core of your argument as I perceive it: get rid of data
from the kernel, because it is growing wild. It is a valid cause. Just
has to be weighed with other stuff, like maintainability, debuggability,
maintainers viewpoint. ...

Yours,
Linus Walleij

[RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Linus Walleij]

On Fri, Nov 24, 2017 at 6:19 PM, Andre Przywara <andre.przywara@arm.com> wrote:

> Conceptually I consider the DT being
> part of the firmware,

As it is a subset of open firmware it is obviously firmware.

> so one trust level above the kernel.

We are several kernel developers who don't trust firmware one
bit. Several disasters in ACPI has made me ever more convinced
that firmware should be trusted less than kernel code.

But this is all very academic.

>> Also, device tree bindings are not documentation for how to write a
>> driver. They are not a replacement for hardware documentation. Nobody
>> should be expected to be able to write an OS driver solely based on a
>> device tree binding. Device tree bindings are more of a configuration
>> interface specification for OS drivers.
>
> Yes, but together with the hardware docs you should be able to write a
> driver. And here you can't, because you are missing the strings. So a
> BSD developer has to look at Linux code.

This is a fair point. It appears in several drivers.

BSD or even Windows (would they use DT) would have to sit in the
back seat just like Linux has been doing for years when it comes
to the hopeless Windowsisms in the x86 BIOSes. I suspect some
Windows on ARM is already experiencing this, but in the ACPI world,
where, incidentally, the servers were being deployed for Linux first
and Windows had to follow their example. I bet they have been
swearing a lot in Redmond about that.

In general it's one of these areas where we can not be utopian about the
hardware descriptions, just fail gracefully in different ways.

I usually try to keep the IETF motto "rough consensus and running
code" in mind. I don't know if it helps in this discussion though.

>> So that's about 40% of the kernel image. Code really is no good without
>> data to process.
>
> But how much of this is SoC specific configuration data? How much is it
> in x86? Yes, historically we had and have a lot of configuration data in
> ARM kernels. But that doesn't mean that we have to continue with this or
> even increase the share.

What people have been doing is trying to have better Kconfig setups
and compile it out by doing kernel modules. It is a bit hopeless with
pin controllers: almost all of them have to be built in. And if they come
with a lot of data, yeah there you have a real good point.

It would be sad if the ARMv7 multiboot or Aarch64 kernel just grows
so that we can't use it but have to go back to shipping board-specific
kernels with a huge bunch of stuff compiled out.

I was hoping Moore's law would save us here :/

An option that has been discussed is better used of  __initdata
and similar tags, especially with built-in drivers. Sadly, this is
hurt by another snag: the compiler or linker file or whatever it is,
is preventing us from discarding any strings from the kernel.
And pin controllers tend to stack up a lot of these.

This is really sucky and something we should solve in general.
I'm not smart enough to tackle any of these problems myself, just
to see them and "Oh that's bad. Very bad."

>> The majority of the improvements over the years have been achieved by
>> moving drivers out of arch/arm and moving board files to DT. The goal
>> was never to get rid of all data.
>
> Sure, not all data. But if we have the relatively easy opportunity to
> avoid further addition of data, we should do it, I believe.
> This significantly reduces the amount of kernel code we need to add to
> support new SoCs.

This is the core of your argument as I perceive it: get rid of data
from the kernel, because it is growing wild. It is a valid cause. Just
has to be weighed with other stuff, like maintainability, debuggability,
maintainers viewpoint. ...

Yours,
Linus Walleij

Re: [RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[André Przywara]

Hi Linus,

thanks for the reply!

On 01/12/17 09:56, Linus Walleij wrote:
> On Fri, Nov 24, 2017 at 6:19 PM, Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org> wrote:
> 
>> Conceptually I consider the DT being
>> part of the firmware,
> 
> As it is a subset of open firmware it is obviously firmware.
> 
>> so one trust level above the kernel.
> 
> We are several kernel developers who don't trust firmware one
> bit. Several disasters in ACPI has made me ever more convinced
> that firmware should be trusted less than kernel code.

I know what you mean, check commit acd316248205 ;-)

But in the Allwinner case all the firmware is open source, and even
controlled by the community (ATF & U-Boot). This is even more true for
the DT. Allwinner came up with their own DT, but since it uses
completely non-standard bindings, we don't use it at all.

So I don't consider firmware this dark, evil and unhackable blob that
many people see in it. Instead it is a good opportunity to separate very
tedious machine-specific parts from the kernel (think of PSCI).

> But this is all very academic.

I agree!

>>> Also, device tree bindings are not documentation for how to write a
>>> driver. They are not a replacement for hardware documentation. Nobody
>>> should be expected to be able to write an OS driver solely based on a
>>> device tree binding. Device tree bindings are more of a configuration
>>> interface specification for OS drivers.
>>
>> Yes, but together with the hardware docs you should be able to write a
>> driver. And here you can't, because you are missing the strings. So a
>> BSD developer has to look at Linux code.
> 
> This is a fair point. It appears in several drivers.
> 
> BSD or even Windows (would they use DT) would have to sit in the
> back seat just like Linux has been doing for years when it comes
> to the hopeless Windowsisms in the x86 BIOSes. I suspect some
> Windows on ARM is already experiencing this, but in the ACPI world,
> where, incidentally, the servers were being deployed for Linux first
> and Windows had to follow their example. I bet they have been
> swearing a lot in Redmond about that.

On the other hand a lot of the ACPI tables have been either taken from
x86 or designed also with Windows in mind. And frankly, my sympathy for
MS is quite limited in this respect ;-)

> In general it's one of these areas where we can not be utopian about the
> hardware descriptions, just fail gracefully in different ways.
> 
> I usually try to keep the IETF motto "rough consensus and running
> code" in mind. I don't know if it helps in this discussion though.
> 
>>> So that's about 40% of the kernel image. Code really is no good without
>>> data to process.
>>
>> But how much of this is SoC specific configuration data? How much is it
>> in x86? Yes, historically we had and have a lot of configuration data in
>> ARM kernels. But that doesn't mean that we have to continue with this or
>> even increase the share.
> 
> What people have been doing is trying to have better Kconfig setups
> and compile it out by doing kernel modules. It is a bit hopeless with
> pin controllers: almost all of them have to be built in. And if they come
> with a lot of data, yeah there you have a real good point.
> 
> It would be sad if the ARMv7 multiboot or Aarch64 kernel just grows
> so that we can't use it but have to go back to shipping board-specific
> kernels with a huge bunch of stuff compiled out.
> 
> I was hoping Moore's law would save us here :/
> 
> An option that has been discussed is better used of  __initdata
> and similar tags, especially with built-in drivers. Sadly, this is
> hurt by another snag: the compiler or linker file or whatever it is,
> is preventing us from discarding any strings from the kernel.
> And pin controllers tend to stack up a lot of these.
> 
> This is really sucky and something we should solve in general.
> I'm not smart enough to tackle any of these problems myself, just
> to see them and "Oh that's bad. Very bad."

I am sure one can find clever hac^Wsolutions for this problem, but I was
wondering if a new approach - like putting the per machine data actually
in DT - isn't the smarter way.
Hence my suggestion with this driver: we *need* the DT anyway to assign
pins to devices, so why not just add the few bytes *there*, which allows
us to have a more or less "data-less" DT driver?

>From what I learned the pinctrl subsystem seems to predates the ARM DT
endeavor, so many design decisions probably didn't consider DT as a
possible solution in the first way. And this is fine, because this is
how it was back then. But it doesn't mean it has to stay this way. And I
carefully thought of solving this problem without alienating existing
users and developers - by keeping the driver and staying compatible with
the existing binding and the DTs.

>>> The majority of the improvements over the years have been achieved by
>>> moving drivers out of arch/arm and moving board files to DT. The goal
>>> was never to get rid of all data.
>>
>> Sure, not all data. But if we have the relatively easy opportunity to
>> avoid further addition of data, we should do it, I believe.
>> This significantly reduces the amount of kernel code we need to add to
>> support new SoCs.
> 
> This is the core of your argument as I perceive it: get rid of data
> from the kernel, because it is growing wild.

Yes, this, but also to avoid replicating data in other DT consumers. I
very much like the idea of the DT describing the hardware, but I am a
bit afraid we loose many good opportunities by treating the DT more like
a "Linux config file" or an external Linux board file.
So I wanted to propose a way where we simplify DT usage in other
systems, while still keeping the Linux driver intact.

> It is a valid cause. Just
> has to be weighed with other stuff, like maintainability, debuggability,
> maintainers viewpoint. ...

So to keep Maxime happy I actually designed this "driver" more like a
shim: to generate the table the current driver expects from the DT, and
actually not touching the existing driver at all.
So maintainability should actually be less of a concern: the driver will
just work with whatever one throws at it from the DT side, without
requiring frequent changes or additions.
In the moment we still need to write, review and merge *data* files for
each new SoC. And as I mentioned before, Allwinner decided to push for
new, slightly different chips every few months, so there will be more to
come. With at least the pinctrl driver out of the way we have one
problem less to worry about.

Cheers,
Andre.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[André Przywara]

Hi Linus,

thanks for the reply!

On 01/12/17 09:56, Linus Walleij wrote:
> On Fri, Nov 24, 2017 at 6:19 PM, Andre Przywara <andre.przywara@arm.com> wrote:
> 
>> Conceptually I consider the DT being
>> part of the firmware,
> 
> As it is a subset of open firmware it is obviously firmware.
> 
>> so one trust level above the kernel.
> 
> We are several kernel developers who don't trust firmware one
> bit. Several disasters in ACPI has made me ever more convinced
> that firmware should be trusted less than kernel code.

I know what you mean, check commit acd316248205 ;-)

But in the Allwinner case all the firmware is open source, and even
controlled by the community (ATF & U-Boot). This is even more true for
the DT. Allwinner came up with their own DT, but since it uses
completely non-standard bindings, we don't use it at all.

So I don't consider firmware this dark, evil and unhackable blob that
many people see in it. Instead it is a good opportunity to separate very
tedious machine-specific parts from the kernel (think of PSCI).

> But this is all very academic.

I agree!

>>> Also, device tree bindings are not documentation for how to write a
>>> driver. They are not a replacement for hardware documentation. Nobody
>>> should be expected to be able to write an OS driver solely based on a
>>> device tree binding. Device tree bindings are more of a configuration
>>> interface specification for OS drivers.
>>
>> Yes, but together with the hardware docs you should be able to write a
>> driver. And here you can't, because you are missing the strings. So a
>> BSD developer has to look at Linux code.
> 
> This is a fair point. It appears in several drivers.
> 
> BSD or even Windows (would they use DT) would have to sit in the
> back seat just like Linux has been doing for years when it comes
> to the hopeless Windowsisms in the x86 BIOSes. I suspect some
> Windows on ARM is already experiencing this, but in the ACPI world,
> where, incidentally, the servers were being deployed for Linux first
> and Windows had to follow their example. I bet they have been
> swearing a lot in Redmond about that.

On the other hand a lot of the ACPI tables have been either taken from
x86 or designed also with Windows in mind. And frankly, my sympathy for
MS is quite limited in this respect ;-)

> In general it's one of these areas where we can not be utopian about the
> hardware descriptions, just fail gracefully in different ways.
> 
> I usually try to keep the IETF motto "rough consensus and running
> code" in mind. I don't know if it helps in this discussion though.
> 
>>> So that's about 40% of the kernel image. Code really is no good without
>>> data to process.
>>
>> But how much of this is SoC specific configuration data? How much is it
>> in x86? Yes, historically we had and have a lot of configuration data in
>> ARM kernels. But that doesn't mean that we have to continue with this or
>> even increase the share.
> 
> What people have been doing is trying to have better Kconfig setups
> and compile it out by doing kernel modules. It is a bit hopeless with
> pin controllers: almost all of them have to be built in. And if they come
> with a lot of data, yeah there you have a real good point.
> 
> It would be sad if the ARMv7 multiboot or Aarch64 kernel just grows
> so that we can't use it but have to go back to shipping board-specific
> kernels with a huge bunch of stuff compiled out.
> 
> I was hoping Moore's law would save us here :/
> 
> An option that has been discussed is better used of  __initdata
> and similar tags, especially with built-in drivers. Sadly, this is
> hurt by another snag: the compiler or linker file or whatever it is,
> is preventing us from discarding any strings from the kernel.
> And pin controllers tend to stack up a lot of these.
> 
> This is really sucky and something we should solve in general.
> I'm not smart enough to tackle any of these problems myself, just
> to see them and "Oh that's bad. Very bad."

I am sure one can find clever hac^Wsolutions for this problem, but I was
wondering if a new approach - like putting the per machine data actually
in DT - isn't the smarter way.
Hence my suggestion with this driver: we *need* the DT anyway to assign
pins to devices, so why not just add the few bytes *there*, which allows
us to have a more or less "data-less" DT driver?

>From what I learned the pinctrl subsystem seems to predates the ARM DT
endeavor, so many design decisions probably didn't consider DT as a
possible solution in the first way. And this is fine, because this is
how it was back then. But it doesn't mean it has to stay this way. And I
carefully thought of solving this problem without alienating existing
users and developers - by keeping the driver and staying compatible with
the existing binding and the DTs.

>>> The majority of the improvements over the years have been achieved by
>>> moving drivers out of arch/arm and moving board files to DT. The goal
>>> was never to get rid of all data.
>>
>> Sure, not all data. But if we have the relatively easy opportunity to
>> avoid further addition of data, we should do it, I believe.
>> This significantly reduces the amount of kernel code we need to add to
>> support new SoCs.
> 
> This is the core of your argument as I perceive it: get rid of data
> from the kernel, because it is growing wild.

Yes, this, but also to avoid replicating data in other DT consumers. I
very much like the idea of the DT describing the hardware, but I am a
bit afraid we loose many good opportunities by treating the DT more like
a "Linux config file" or an external Linux board file.
So I wanted to propose a way where we simplify DT usage in other
systems, while still keeping the Linux driver intact.

> It is a valid cause. Just
> has to be weighed with other stuff, like maintainability, debuggability,
> maintainers viewpoint. ...

So to keep Maxime happy I actually designed this "driver" more like a
shim: to generate the table the current driver expects from the DT, and
actually not touching the existing driver at all.
So maintainability should actually be less of a concern: the driver will
just work with whatever one throws at it from the DT side, without
requiring frequent changes or additions.
In the moment we still need to write, review and merge *data* files for
each new SoC. And as I mentioned before, Allwinner decided to push for
new, slightly different chips every few months, so there will be more to
come. With at least the pinctrl driver out of the way we have one
problem less to worry about.

Cheers,
Andre.

Re: [RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Linus Walleij]

On Wed, Dec 6, 2017 at 1:55 AM, André Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org> wrote:
> On 01/12/17 09:56, Linus Walleij wrote:

>> It is a valid cause. Just
>> has to be weighed with other stuff, like maintainability, debuggability,
>> maintainers viewpoint. ...
>
> So to keep Maxime happy I actually designed this "driver" more like a
> shim: to generate the table the current driver expects from the DT, and
> actually not touching the existing driver at all.
> So maintainability should actually be less of a concern: the driver will
> just work with whatever one throws at it from the DT side, without
> requiring frequent changes or additions.
> In the moment we still need to write, review and merge *data* files for
> each new SoC. And as I mentioned before, Allwinner decided to push for
> new, slightly different chips every few months, so there will be more to
> come. With at least the pinctrl driver out of the way we have one
> problem less to worry about.

I think you need mainly to convince Maxime that this is something that
he wants to maintain, going forward.

I am as subsystem maintainer pretty pleased as long as standard
properties etc are used to encode the data into the devicetree, and
DT maintrainers are not actively vetoing what you do.

If it leads to a conflict between Allwinner maintainers it is not worth the
effort for reasons that are social rather than technical. To me it is a very
nice but as with all volunteer communities also very vulnerable
endavour.

Please make sure not to push your point so hard that it hurts your
our your colleagues feelings.

I know people are passionate about their ideas, which is usally good
but also scare me sometimes because they sometimes become so
passionate that it makes them bad team players.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Linus Walleij]

On Wed, Dec 6, 2017 at 1:55 AM, Andr? Przywara <andre.przywara@arm.com> wrote:
> On 01/12/17 09:56, Linus Walleij wrote:

>> It is a valid cause. Just
>> has to be weighed with other stuff, like maintainability, debuggability,
>> maintainers viewpoint. ...
>
> So to keep Maxime happy I actually designed this "driver" more like a
> shim: to generate the table the current driver expects from the DT, and
> actually not touching the existing driver at all.
> So maintainability should actually be less of a concern: the driver will
> just work with whatever one throws at it from the DT side, without
> requiring frequent changes or additions.
> In the moment we still need to write, review and merge *data* files for
> each new SoC. And as I mentioned before, Allwinner decided to push for
> new, slightly different chips every few months, so there will be more to
> come. With at least the pinctrl driver out of the way we have one
> problem less to worry about.

I think you need mainly to convince Maxime that this is something that
he wants to maintain, going forward.

I am as subsystem maintainer pretty pleased as long as standard
properties etc are used to encode the data into the devicetree, and
DT maintrainers are not actively vetoing what you do.

If it leads to a conflict between Allwinner maintainers it is not worth the
effort for reasons that are social rather than technical. To me it is a very
nice but as with all volunteer communities also very vulnerable
endavour.

Please make sure not to push your point so hard that it hurts your
our your colleagues feelings.

I know people are passionate about their ideas, which is usally good
but also scare me sometimes because they sometimes become so
passionate that it makes them bad team players.

Yours,
Linus Walleij

Re: [RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Andre Przywara]

Hi Linus,

thanks for having a look!

On 24/11/17 10:19, Linus Walleij wrote:
> On Mon, Nov 13, 2017 at 2:25 AM, Andre Przywara <andre.przywara@arm.com> wrote:
> 
>> So far all the Allwinner pinctrl drivers provided a table in the
>> kernel to describe all the pins and the link between the pinctrl functions
>> names (strings) and their respective mux values (register values).
>>
>> Extend the binding to put those mappings in the DT, so that any SoC can
>> describe its pinctrl and GPIO data fully there instead of relying on
>> tables.
>> This uses a generic compatible name, to be prepended with an SoC
>> specific name in the node.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> (...)
> 
> I definately want feedback from Maxime before I do anything with
> this patch series.
> 
>> +** Generic pinctrl binding
>> +The above binding requires knowledge of the actual mux setting values for
>> +each supported SoC in the code parsing the DT (for instance the kernel).
>> +The generic binding puts this information in the DT. It uses the
>> +"allwinner,sunxi-pinctrl" compatible, in addition to some SoC specific string.
>> +It extends the above described binding as follows:
>> +Required properties:
>> +- allwinner,gpio-pins: An array of 32-bit numbers to denote the number of
>> +  implemented pins per pin controller port. Non-implemented ports can specify
>> +  0 here. There will be as many ports as this array has elements.
> 
> I don't understand what this adds that gpio-ranges does not already
> provide.
> Documentation/devicetree/bindings/gpio/gpio.txt
> at the end of the file.

Ah, thanks for the pointer, will try to see if that fits in here.
Although here (despite my clumsy naming) I think this is more about the
"pinmux" pins than the GPIOs. I am not sure if it's just my
understanding or if on Allwinner we just happen to have every pinmux pin
being a GPIO pin as well, so the distinction between the two isn't so clear.

> These ranges exist exactly to map pin controller pins in a pin controller
> to GPIO lines in a gpiochip.

Which makes it a bit more confusing because we have the pinctrl and GPIO
controller drivers combined in one file.

>> +- allwinner,irq-pin-map: Contains a number of IRQ port maps, describing the
>> +  relationship between interrupt banks and GPIO pins. Each map has six 32-bit
>> +  members:
>> +  <[IRQ port] [1st IRQ pin] [GPIO port] [1st GPIO pin] [mux value] [length]>
>> +  This maps the first [length] IRQ pins starting with [IRQ port]:[1st IRQ pin]
>> +  to [GPIO port]:[1st GPIO pin], all using [mux value] to select the IRQ
>> +  functionality.
> 
> We have recently added GPIO "bank" awareness into gpiolib
> via Thierry Reding's patches, so a gpiochip can use the generic
> GPIOLIB_IRQCHIP and specify multiple IRQ parents with a map.
> 
> Please see if you can use this instead.

Thanks for the hint, will have a look.

> If any of this should be expressed in the DT bindings it should
> be genericized and not use any "allwinner,*" prefixes, and it
> should preferably just be hard-coded in the driver and switched
> in from the compatible string IMO.

I agree, this allwinner prefix was just a first shot for the RFC.

The reason for this map is that older Allwinner SoCs have *one* IRQ pin
bank, which contains pins from *multiple* GPIO banks. The A10/A20 for
instance maps the first 22 IRQ pins to Port H0-H21, the remaining 10 IRQ
pins are on port I10-I19. This irq-pin-map is an admittedly
over-engineered attempt to express this is a generic way, trying to
embrace the DT way of mapping things, like we do with the "ranges"
property, for instance.

Recent SoCs just have some GPIO banks which map directly to IRQ pin
banks (on the A64 ports B, G and H map to IRQ bank 0, 1 and 2), so this
is not really needed here. My first version indeed just had a simpler
list to express this.

This series aims to build on top of the existing driver as much as
possible, so some things are not as elegant as they could be.

>> +Optional properties:
>> +- allwinner,port-base: The number of GPIO ports to skip at the beginning.
>> +- allwinner,irq-bank-base: The number of IRQ banks to skip at the beginning.
> 
> I don't understand this. It looks hacky. Can you elaborate why this
> is needed?

It is indeed hacky, and just maps the irq_bank_base member of struct
sunxi_pinctrl_desc into the DT. My understanding is that the A33 skips
the first interrupt bank in the register map, for whatever reason. Might
just be an implementation oddity.
I am just wondering if this could be expressed with the irq-pin-map
above. Or if it's more or less an A33 bug, we could derive this from the
compatible string.

>> +- allwinner,irq-read-needs-mux: Specifies that reading the line level of
>> +  a pin configured as an IRQ pin is not possible. A driver needs to switch
>> +  to the GPIO-in function to be able to read the level.
> 
> I guess it is a bool flag?

Right, should mention that.

>> +Required properties for subnodes:
>> +- pinmux: An array of mux values to write into the respective MMIO register
>> +  bits for this pin when selecting the function. If this array has less
>> +  elements than pins, the *last* value will be used for all pins beyond that.
>> +  This allows to use a single element for the (likely) case all pins use the
>> +  same mux value.
> 
> This is a standard bindings so I don't have much against it. But I
> need Maxime's input here, I think we should keep Allwinner consistent
> across SoCs.

I can understand that, and that's why I choose the new binding to just
be an extension of what we have already.

I am happy to have a discussion on that in general - actually this was
one aim of this series. See the other thread.

I might even write some summary on how it works today and why I believe
it should be improved, I just wasn't sure that was actually needed.

Cheers,
Andre.

[RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Andre Przywara]

Hi Linus,

thanks for having a look!

On 24/11/17 10:19, Linus Walleij wrote:
> On Mon, Nov 13, 2017 at 2:25 AM, Andre Przywara <andre.przywara@arm.com> wrote:
> 
>> So far all the Allwinner pinctrl drivers provided a table in the
>> kernel to describe all the pins and the link between the pinctrl functions
>> names (strings) and their respective mux values (register values).
>>
>> Extend the binding to put those mappings in the DT, so that any SoC can
>> describe its pinctrl and GPIO data fully there instead of relying on
>> tables.
>> This uses a generic compatible name, to be prepended with an SoC
>> specific name in the node.
>>
>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> (...)
> 
> I definately want feedback from Maxime before I do anything with
> this patch series.
> 
>> +** Generic pinctrl binding
>> +The above binding requires knowledge of the actual mux setting values for
>> +each supported SoC in the code parsing the DT (for instance the kernel).
>> +The generic binding puts this information in the DT. It uses the
>> +"allwinner,sunxi-pinctrl" compatible, in addition to some SoC specific string.
>> +It extends the above described binding as follows:
>> +Required properties:
>> +- allwinner,gpio-pins: An array of 32-bit numbers to denote the number of
>> +  implemented pins per pin controller port. Non-implemented ports can specify
>> +  0 here. There will be as many ports as this array has elements.
> 
> I don't understand what this adds that gpio-ranges does not already
> provide.
> Documentation/devicetree/bindings/gpio/gpio.txt
> at the end of the file.

Ah, thanks for the pointer, will try to see if that fits in here.
Although here (despite my clumsy naming) I think this is more about the
"pinmux" pins than the GPIOs. I am not sure if it's just my
understanding or if on Allwinner we just happen to have every pinmux pin
being a GPIO pin as well, so the distinction between the two isn't so clear.

> These ranges exist exactly to map pin controller pins in a pin controller
> to GPIO lines in a gpiochip.

Which makes it a bit more confusing because we have the pinctrl and GPIO
controller drivers combined in one file.

>> +- allwinner,irq-pin-map: Contains a number of IRQ port maps, describing the
>> +  relationship between interrupt banks and GPIO pins. Each map has six 32-bit
>> +  members:
>> +  <[IRQ port] [1st IRQ pin] [GPIO port] [1st GPIO pin] [mux value] [length]>
>> +  This maps the first [length] IRQ pins starting with [IRQ port]:[1st IRQ pin]
>> +  to [GPIO port]:[1st GPIO pin], all using [mux value] to select the IRQ
>> +  functionality.
> 
> We have recently added GPIO "bank" awareness into gpiolib
> via Thierry Reding's patches, so a gpiochip can use the generic
> GPIOLIB_IRQCHIP and specify multiple IRQ parents with a map.
> 
> Please see if you can use this instead.

Thanks for the hint, will have a look.

> If any of this should be expressed in the DT bindings it should
> be genericized and not use any "allwinner,*" prefixes, and it
> should preferably just be hard-coded in the driver and switched
> in from the compatible string IMO.

I agree, this allwinner prefix was just a first shot for the RFC.

The reason for this map is that older Allwinner SoCs have *one* IRQ pin
bank, which contains pins from *multiple* GPIO banks. The A10/A20 for
instance maps the first 22 IRQ pins to Port H0-H21, the remaining 10 IRQ
pins are on port I10-I19. This irq-pin-map is an admittedly
over-engineered attempt to express this is a generic way, trying to
embrace the DT way of mapping things, like we do with the "ranges"
property, for instance.

Recent SoCs just have some GPIO banks which map directly to IRQ pin
banks (on the A64 ports B, G and H map to IRQ bank 0, 1 and 2), so this
is not really needed here. My first version indeed just had a simpler
list to express this.

This series aims to build on top of the existing driver as much as
possible, so some things are not as elegant as they could be.

>> +Optional properties:
>> +- allwinner,port-base: The number of GPIO ports to skip at the beginning.
>> +- allwinner,irq-bank-base: The number of IRQ banks to skip at the beginning.
> 
> I don't understand this. It looks hacky. Can you elaborate why this
> is needed?

It is indeed hacky, and just maps the irq_bank_base member of struct
sunxi_pinctrl_desc into the DT. My understanding is that the A33 skips
the first interrupt bank in the register map, for whatever reason. Might
just be an implementation oddity.
I am just wondering if this could be expressed with the irq-pin-map
above. Or if it's more or less an A33 bug, we could derive this from the
compatible string.

>> +- allwinner,irq-read-needs-mux: Specifies that reading the line level of
>> +  a pin configured as an IRQ pin is not possible. A driver needs to switch
>> +  to the GPIO-in function to be able to read the level.
> 
> I guess it is a bool flag?

Right, should mention that.

>> +Required properties for subnodes:
>> +- pinmux: An array of mux values to write into the respective MMIO register
>> +  bits for this pin when selecting the function. If this array has less
>> +  elements than pins, the *last* value will be used for all pins beyond that.
>> +  This allows to use a single element for the (likely) case all pins use the
>> +  same mux value.
> 
> This is a standard bindings so I don't have much against it. But I
> need Maxime's input here, I think we should keep Allwinner consistent
> across SoCs.

I can understand that, and that's why I choose the new binding to just
be an extension of what we have already.

I am happy to have a discussion on that in general - actually this was
one aim of this series. See the other thread.

I might even write some summary on how it works today and why I believe
it should be improved, I just wasn't sure that was actually needed.

Cheers,
Andre.

Re: [RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Maxime Ripard]

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

Hi Linus,

On Fri, Nov 24, 2017 at 11:19:52AM +0100, Linus Walleij wrote:
> On Mon, Nov 13, 2017 at 2:25 AM, Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org> wrote:
> 
> > So far all the Allwinner pinctrl drivers provided a table in the
> > kernel to describe all the pins and the link between the pinctrl functions
> > names (strings) and their respective mux values (register values).
> >
> > Extend the binding to put those mappings in the DT, so that any SoC can
> > describe its pinctrl and GPIO data fully there instead of relying on
> > tables.
> > This uses a generic compatible name, to be prepended with an SoC
> > specific name in the node.
> >
> > Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
> (...)
> 
> I definately want feedback from Maxime before I do anything with
> this patch series.

So I've been opposed to it as well, for the reasons Rob, Thierry and
you pointed out already in that thread

I just wasn't sure what your opinion on it was, so I didn't answer
just so that I could see if I was the only one pushing back.

So I guess we all agree here.

Maxime

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

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

[RFC PATCH 1/3] dt-bindings: pinctrl: sunxi: document new generic binding

[Maxime Ripard]

Hi Linus,

On Fri, Nov 24, 2017 at 11:19:52AM +0100, Linus Walleij wrote:
> On Mon, Nov 13, 2017 at 2:25 AM, Andre Przywara <andre.przywara@arm.com> wrote:
> 
> > So far all the Allwinner pinctrl drivers provided a table in the
> > kernel to describe all the pins and the link between the pinctrl functions
> > names (strings) and their respective mux values (register values).
> >
> > Extend the binding to put those mappings in the DT, so that any SoC can
> > describe its pinctrl and GPIO data fully there instead of relying on
> > tables.
> > This uses a generic compatible name, to be prepended with an SoC
> > specific name in the node.
> >
> > Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> (...)
> 
> I definately want feedback from Maxime before I do anything with
> this patch series.

So I've been opposed to it as well, for the reasons Rob, Thierry and
you pointed out already in that thread

I just wasn't sure what your opinion on it was, so I didn't answer
just so that I could see if I was the only one pushing back.

So I guess we all agree here.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20171124/ed86aab8/attachment-0001.sig>

[RFC PATCH 2/3] pinctrl: sunxi: introduce DT-based generic driver

[Andre Przywara]

This driver (shim) allows to fully describe an Allwinner pin controller
and its GPIO ports in device tree nodes.
It will read some newly introduced properties to build a table
describing the pins and their routing. This table matches those that we
have hardcoded for various SoCs in the kernel so far.
After this table has been created, it will be handed over to the actual
pinctrl driver, which registers it with the pinctrl subsystem.

Signed-off-by: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
---
 drivers/pinctrl/sunxi/Makefile           |   1 +
 drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c | 412 +++++++++++++++++++++++++++++++
 2 files changed, 413 insertions(+)
 create mode 100644 drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c

diff --git a/drivers/pinctrl/sunxi/Makefile b/drivers/pinctrl/sunxi/Makefile
index 12a752e836ef..06f9d924b9cf 100644
--- a/drivers/pinctrl/sunxi/Makefile
+++ b/drivers/pinctrl/sunxi/Makefile
@@ -20,3 +20,4 @@ obj-$(CONFIG_PINCTRL_SUN8I_V3S)		+= pinctrl-sun8i-v3s.o
 obj-$(CONFIG_PINCTRL_SUN50I_H5)		+= pinctrl-sun50i-h5.o
 obj-$(CONFIG_PINCTRL_SUN9I_A80)		+= pinctrl-sun9i-a80.o
 obj-$(CONFIG_PINCTRL_SUN9I_A80_R)	+= pinctrl-sun9i-a80-r.o
+obj-y					+= pinctrl-sunxi-dt.o
diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c b/drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c
new file mode 100644
index 000000000000..46a7bfbefee1
--- /dev/null
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c
@@ -0,0 +1,412 @@
+/*
+ * Generic Allwinner pinctrl driver DT shim
+ * Read all the information required by the pinctrl subsystem from the DT
+ * and build a table describing each pin. Hand this table over to the actual
+ * pinctrl driver.
+ *
+ * Copyright (C) 2017 ARM Ltd.
+ * Author: Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/export.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include "pinctrl-sunxi.h"
+
+#define INVALID_MUX	0xff
+
+/*
+ * Return the "index"th element from the "pinmux" property. If the property
+ * does not hold enough entries, return the last one instead.
+ * For almost every group the pinmux value is actually the same, so this
+ * allows to just list it once in the property.
+ */
+static u8 sunxi_pinctrl_dt_read_pinmux(const struct device_node *node,
+				       int index)
+{
+	int ret, num_elems;
+	u32 value;
+
+	ret = of_property_read_u32_index(node, "pinmux", index, &value);
+	if (!ret)
+		return value;
+	if (ret != -EOVERFLOW)
+		return INVALID_MUX;
+
+	num_elems = of_property_count_u32_elems(node, "pinmux");
+	if (num_elems <= 0)
+		return INVALID_MUX;
+
+	ret = of_property_read_u32_index(node, "pinmux", num_elems - 1, &value);
+	if (ret)
+		return INVALID_MUX;
+
+	return value;
+}
+
+/*
+ * Allocate a table with a sunxi_desc_pin structure for every pin needed.
+ * Fills in the respective pin names ("PA0") and their pin numbers.
+ * Returns the number of pins allocated.
+ */
+static int build_pins_table(struct device *dev, struct device_node *node,
+			    int port_base, struct sunxi_desc_pin **table)
+{
+	struct sunxi_desc_pin *pins, *cur_pin;
+	struct property *prop;
+	const __be32 *cur;
+	u32 pin_count;
+	int name_size = 0, npins = 0, nports = 0;
+	char *pin_names, *cur_name;
+	int i, j;
+
+	/*
+	 * Find the total number of pins.
+	 * Also work out how much memory we need to store all the pin names.
+	 */
+	of_property_for_each_u32(node, "allwinner,gpio-pins", prop,
+				 cur, pin_count) {
+		npins += pin_count;
+		if (pin_count < 10) {
+			name_size += pin_count * 4; /* 4 bytes for "PXy\0" */
+		} else {
+			/* 4 bytes for each "PXy\0" */
+			name_size += 10 * 4;
+
+			/* 5 bytes for each "PXyy\0" */
+			name_size += (pin_count - 10) * 5;
+		}
+		nports++;
+	}
+
+	if (!nports || !npins) {
+		dev_warn(dev, "%s: no pins defined\n", of_node_full_name(node));
+		return -EINVAL;
+	}
+
+	pins = devm_kzalloc(dev, npins * sizeof(*pins), GFP_KERNEL);
+	if (!pins)
+		return -ENOMEM;
+
+	/* Allocate memory to store the name for every pin. */
+	pin_names = devm_kmalloc(dev, name_size, GFP_KERNEL);
+	if (!pin_names)
+		return -ENOMEM;
+
+	/* Fill the pins array with the name and the number for each pin. */
+	cur_name = pin_names;
+	cur_pin = pins;
+	for (i = 0; i < nports; i++) {
+		of_property_read_u32_index(node, "allwinner,gpio-pins", i,
+					   &pin_count);
+		for (j = 0; j < pin_count; j++, cur_pin++) {
+			int nchars = sprintf(cur_name, "P%c%d",
+					     port_base + 'A' + i, j);
+
+			cur_pin->pin.number = (port_base + i) * 32 + j;
+			cur_pin->pin.name = cur_name;
+			cur_name += nchars + 1;
+		}
+	}
+
+	*table = pins;
+
+	return npins;
+}
+
+/* Check whether this pin can trigger interrupts. */
+static bool get_irq_pin(struct device_node *pnode,
+			struct pinctrl_pin_desc *pin,
+			u8 *irq_port, u8 *irq_pin, u8 *muxval)
+{
+	u32 gpio_firstpin, length, reg;
+	int i;
+
+	for (i = 0; ; i++) {
+		if (of_property_read_u32_index(pnode, "allwinner,irq-pin-map",
+					       i * 6 + 2, &reg))
+			break;
+
+		if (reg != pin->number / 32)
+			continue;
+
+		of_property_read_u32_index(pnode, "allwinner,irq-pin-map",
+					   i * 6 + 3, &gpio_firstpin);
+		of_property_read_u32_index(pnode, "allwinner,irq-pin-map",
+					   i * 6 + 5, &length);
+		if ((gpio_firstpin > pin->number % 32) ||
+		    (gpio_firstpin + length <= pin->number % 32))
+			continue;
+
+		if (irq_port) {
+			of_property_read_u32_index(pnode,
+						   "allwinner,irq-pin-map",
+						   i * 6 + 0, &reg);
+			*irq_port = reg;
+		}
+		if (irq_pin) {
+			of_property_read_u32_index(pnode,
+						   "allwinner,irq-pin-map",
+						   i * 6 + 1, &reg);
+			*irq_pin = reg + (pin->number % 32) - gpio_firstpin;
+		}
+		if (muxval) {
+			of_property_read_u32_index(pnode,
+						   "allwinner,irq-pin-map",
+						   i * 6 + 4, &reg);
+			*muxval = reg;
+		}
+
+
+		return true;
+	}
+
+	return false;
+}
+
+/*
+ * Work out the number of functions each pin has. Provide memory to hold
+ * the per-function information and assign it to the pin table.
+ * Fill in the GPIO in/out functions every pin has, also add an "irq"
+ * function for those pins in IRQ-capable ports.
+ */
+static int prepare_function_table(struct device *dev, struct device_node *pnode,
+				  struct sunxi_desc_pin *pins, int npins)
+{
+	struct device_node *node;
+	struct property *prop;
+	struct sunxi_desc_function *func;
+	int num_funcs, i;
+
+	/*
+	 * We need at least three functions per pin:
+	 * - one for GPIO in
+	 * - one for GPIO out
+	 * - one for the sentinel signalling the end of the list
+	 */
+	num_funcs = 3 * npins;
+
+	/* Add a function for each pin in a port supporting interrupts. */
+	for (i = 0; i < npins; i++) {
+		if (get_irq_pin(pnode, &pins[i].pin, NULL, NULL, NULL)) {
+			pins[i].variant++;
+			num_funcs++;
+		}
+	}
+
+	/*
+	 * Go over each pin group (every child of the pinctrl DT node) and
+	 * add the number of special functions each pins has. Also update the
+	 * total number of functions required.
+	 * We might slightly overshoot here in case of double definitions.
+	 */
+	for_each_child_of_node(pnode, node) {
+		const char *name;
+
+		of_property_for_each_string(node, "pins", prop, name) {
+			for (i = 0; i < npins; i++) {
+				if (strcmp(pins[i].pin.name, name))
+					continue;
+
+				pins[i].variant++;
+				num_funcs++;
+				break;
+			}
+		}
+	}
+
+	/*
+	 * Allocate the memory needed for the functions in one table.
+	 * We later use pointers into this table to mark each pin.
+	 */
+	func = devm_kzalloc(dev, num_funcs * sizeof(*func), GFP_KERNEL);
+	if (!func)
+		return -ENOMEM;
+
+	/* Assign the functions memory and fill in GPIOs, IRQ and a sentinel. */
+	for (i = 0; i < npins; i++) {
+		int lastfunc = pins[i].variant + 1;
+
+		func[0].name = "gpio_in";
+		func[0].muxval = 0;
+		func[1].name = "gpio_out";
+		func[1].muxval = 1;
+
+		if (get_irq_pin(pnode, &pins[i].pin,
+				&func[lastfunc].irqbank,
+				&func[lastfunc].irqnum,
+				&func[lastfunc].muxval))
+			func[lastfunc].name = "irq";
+
+		pins[i].functions = func;
+
+		/* Skip over the other needed functions and the sentinel. */
+		func += pins[i].variant + 3;
+
+		/*
+		 * Reset the value for filling in the remaining functions
+		 * behind the GPIOs later.
+		 */
+		pins[i].variant = 2;
+	}
+
+	return 0;
+}
+
+/*
+ * Iterate over all pins in this group and add the function name and its
+ * mux value to the respective pin.
+ */
+static void fill_pin_function(struct device *dev, struct device_node *node,
+			      struct sunxi_desc_pin *pins, int npins)
+{
+	const char *name, *funcname;
+	struct sunxi_desc_function *func;
+	struct property *prop;
+	int pin, i;
+	u8 muxval;
+
+	if (of_property_read_string(node, "function", &funcname)) {
+		dev_warn(dev, "missing \"function\" property\n");
+		return;
+	}
+
+	of_property_for_each_string(node, "pins", prop, name) {
+		/* Find the index of this pin in our table. */
+		for (pin = 0; pin < npins; pin++)
+			if (!strcmp(pins[pin].pin.name, name))
+				break;
+		if (pin == npins) {
+			dev_warn(dev, "%s: cannot find pin %s\n",
+				 of_node_full_name(node), name);
+			continue;
+		}
+
+		/* Read the associated mux value. */
+		muxval = sunxi_pinctrl_dt_read_pinmux(node, pin);
+		if (muxval == INVALID_MUX) {
+			dev_warn(dev, "%s: invalid mux value for pin %s\n",
+				 of_node_full_name(node), name);
+			continue;
+		}
+
+		/*
+		 * Check for double definitions by comparing the to-be-added
+		 * function with already assigned ones.
+		 * Ignore identical pairs (function name and mux value the
+		 * same), but warn about conflicting assignments.
+		 */
+		for (i = 2; i < pins[pin].variant; i++) {
+			func = &pins[pin].functions[i];
+
+			/* Skip over totally unrelated functions. */
+			if (strcmp(func->name, funcname) &&
+			    func->muxval != muxval)
+				continue;
+
+			/* Ignore (but skip below) any identical functions. */
+			if (!strcmp(func->name, funcname) &&
+			    muxval == func->muxval)
+				break;
+
+			dev_warn(dev,
+				 "pin %s: function %s redefined to mux %d\n",
+				 name, funcname, muxval);
+			break;
+		}
+
+		/* Skip any pins with that function already assigned. */
+		if (i < pins[pin].variant)
+			continue;
+
+		/* Assign function and muxval to the next free slot. */
+		func = &pins[pin].functions[pins[pin].variant];
+		func->muxval = muxval;
+		func->name = funcname;
+
+		pins[pin].variant++;
+	}
+}
+
+/* Iterate over all IRQ pin maps and find out the number of distinct ports. */
+static int count_nr_irq_banks(struct device_node *node)
+{
+	u32 port_mask = 0, irq_port;
+	int i = 0;
+
+	for (i = 0;
+	     !of_property_read_u32_index(node, "allwinner,irq-pin-map",
+					 i * 6 + 0, &irq_port);
+	     i++)
+		port_mask |= BIT(irq_port);
+
+	return hweight32(port_mask);
+}
+
+static int sunxi_generic_pinctrl_probe(struct platform_device *pdev)
+{
+	struct device_node *pnode = pdev->dev.of_node, *node;
+	struct sunxi_pinctrl_desc *desc;
+	struct sunxi_desc_pin *pins;
+	u32 port_base = 0;
+	int npins, ret, i;
+
+	of_property_read_u32(pnode, "allwinner,port-base", &port_base);
+	npins = build_pins_table(&pdev->dev, pnode, port_base, &pins);
+	if (npins < 0)
+		return npins;
+
+	ret = prepare_function_table(&pdev->dev, pnode, pins, npins);
+	if (ret)
+		return ret;
+
+	/*
+	 * Now iterate over all groups and add the respective function name
+	 * and mux values to each pin listed within.
+	 */
+	for_each_child_of_node(pnode, node)
+		fill_pin_function(&pdev->dev, node, pins, npins);
+
+	/* Clear the temporary storage. */
+	for (i = 0; i < npins; i++)
+		pins[i].variant = 0;
+
+	desc = devm_kzalloc(&pdev->dev, sizeof(*desc), GFP_KERNEL);
+	if (!desc)
+		return -ENOMEM;
+
+	desc->pins = pins;
+	desc->npins = npins;
+	desc->pin_base = port_base * 32;
+
+	desc->irq_banks = count_nr_irq_banks(pnode);
+	of_property_read_u32(pnode, "allwinner,irq-bank-base",
+			     &desc->irq_bank_base);
+	if (of_property_read_bool(pnode, "allwinner,irq_read_needs_mux"))
+		desc->irq_read_needs_mux = true;
+
+	return sunxi_pinctrl_init_with_variant(pdev, desc, 0);
+}
+
+static const struct of_device_id sunxi_pinctrl_match[] = {
+	{ .compatible = "allwinner,sunxi-pinctrl", },
+	{ },
+};
+
+static struct platform_driver sunxi_pinctrl_driver = {
+	.probe  = sunxi_generic_pinctrl_probe,
+	.driver = {
+		.name           = "sunxi-pinctrl",
+		.of_match_table = sunxi_pinctrl_match,
+	},
+};
+builtin_platform_driver(sunxi_pinctrl_driver);
-- 
2.14.1

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 2/3] pinctrl: sunxi: introduce DT-based generic driver

[Andre Przywara]

This driver (shim) allows to fully describe an Allwinner pin controller
and its GPIO ports in device tree nodes.
It will read some newly introduced properties to build a table
describing the pins and their routing. This table matches those that we
have hardcoded for various SoCs in the kernel so far.
After this table has been created, it will be handed over to the actual
pinctrl driver, which registers it with the pinctrl subsystem.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/pinctrl/sunxi/Makefile           |   1 +
 drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c | 412 +++++++++++++++++++++++++++++++
 2 files changed, 413 insertions(+)
 create mode 100644 drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c

diff --git a/drivers/pinctrl/sunxi/Makefile b/drivers/pinctrl/sunxi/Makefile
index 12a752e836ef..06f9d924b9cf 100644
--- a/drivers/pinctrl/sunxi/Makefile
+++ b/drivers/pinctrl/sunxi/Makefile
@@ -20,3 +20,4 @@ obj-$(CONFIG_PINCTRL_SUN8I_V3S)		+= pinctrl-sun8i-v3s.o
 obj-$(CONFIG_PINCTRL_SUN50I_H5)		+= pinctrl-sun50i-h5.o
 obj-$(CONFIG_PINCTRL_SUN9I_A80)		+= pinctrl-sun9i-a80.o
 obj-$(CONFIG_PINCTRL_SUN9I_A80_R)	+= pinctrl-sun9i-a80-r.o
+obj-y					+= pinctrl-sunxi-dt.o
diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c b/drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c
new file mode 100644
index 000000000000..46a7bfbefee1
--- /dev/null
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c
@@ -0,0 +1,412 @@
+/*
+ * Generic Allwinner pinctrl driver DT shim
+ * Read all the information required by the pinctrl subsystem from the DT
+ * and build a table describing each pin. Hand this table over to the actual
+ * pinctrl driver.
+ *
+ * Copyright (C) 2017 ARM Ltd.
+ * Author: Andre Przywara <andre.przywara@arm.com>
+ *
+ * This file is licensed under the terms of the GNU General Public
+ * License version 2.  This program is licensed "as is" without any
+ * warranty of any kind, whether express or implied.
+ */
+
+#include <linux/export.h>
+#include <linux/of.h>
+#include <linux/of_address.h>
+#include <linux/of_device.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#include "pinctrl-sunxi.h"
+
+#define INVALID_MUX	0xff
+
+/*
+ * Return the "index"th element from the "pinmux" property. If the property
+ * does not hold enough entries, return the last one instead.
+ * For almost every group the pinmux value is actually the same, so this
+ * allows to just list it once in the property.
+ */
+static u8 sunxi_pinctrl_dt_read_pinmux(const struct device_node *node,
+				       int index)
+{
+	int ret, num_elems;
+	u32 value;
+
+	ret = of_property_read_u32_index(node, "pinmux", index, &value);
+	if (!ret)
+		return value;
+	if (ret != -EOVERFLOW)
+		return INVALID_MUX;
+
+	num_elems = of_property_count_u32_elems(node, "pinmux");
+	if (num_elems <= 0)
+		return INVALID_MUX;
+
+	ret = of_property_read_u32_index(node, "pinmux", num_elems - 1, &value);
+	if (ret)
+		return INVALID_MUX;
+
+	return value;
+}
+
+/*
+ * Allocate a table with a sunxi_desc_pin structure for every pin needed.
+ * Fills in the respective pin names ("PA0") and their pin numbers.
+ * Returns the number of pins allocated.
+ */
+static int build_pins_table(struct device *dev, struct device_node *node,
+			    int port_base, struct sunxi_desc_pin **table)
+{
+	struct sunxi_desc_pin *pins, *cur_pin;
+	struct property *prop;
+	const __be32 *cur;
+	u32 pin_count;
+	int name_size = 0, npins = 0, nports = 0;
+	char *pin_names, *cur_name;
+	int i, j;
+
+	/*
+	 * Find the total number of pins.
+	 * Also work out how much memory we need to store all the pin names.
+	 */
+	of_property_for_each_u32(node, "allwinner,gpio-pins", prop,
+				 cur, pin_count) {
+		npins += pin_count;
+		if (pin_count < 10) {
+			name_size += pin_count * 4; /* 4 bytes for "PXy\0" */
+		} else {
+			/* 4 bytes for each "PXy\0" */
+			name_size += 10 * 4;
+
+			/* 5 bytes for each "PXyy\0" */
+			name_size += (pin_count - 10) * 5;
+		}
+		nports++;
+	}
+
+	if (!nports || !npins) {
+		dev_warn(dev, "%s: no pins defined\n", of_node_full_name(node));
+		return -EINVAL;
+	}
+
+	pins = devm_kzalloc(dev, npins * sizeof(*pins), GFP_KERNEL);
+	if (!pins)
+		return -ENOMEM;
+
+	/* Allocate memory to store the name for every pin. */
+	pin_names = devm_kmalloc(dev, name_size, GFP_KERNEL);
+	if (!pin_names)
+		return -ENOMEM;
+
+	/* Fill the pins array with the name and the number for each pin. */
+	cur_name = pin_names;
+	cur_pin = pins;
+	for (i = 0; i < nports; i++) {
+		of_property_read_u32_index(node, "allwinner,gpio-pins", i,
+					   &pin_count);
+		for (j = 0; j < pin_count; j++, cur_pin++) {
+			int nchars = sprintf(cur_name, "P%c%d",
+					     port_base + 'A' + i, j);
+
+			cur_pin->pin.number = (port_base + i) * 32 + j;
+			cur_pin->pin.name = cur_name;
+			cur_name += nchars + 1;
+		}
+	}
+
+	*table = pins;
+
+	return npins;
+}
+
+/* Check whether this pin can trigger interrupts. */
+static bool get_irq_pin(struct device_node *pnode,
+			struct pinctrl_pin_desc *pin,
+			u8 *irq_port, u8 *irq_pin, u8 *muxval)
+{
+	u32 gpio_firstpin, length, reg;
+	int i;
+
+	for (i = 0; ; i++) {
+		if (of_property_read_u32_index(pnode, "allwinner,irq-pin-map",
+					       i * 6 + 2, &reg))
+			break;
+
+		if (reg != pin->number / 32)
+			continue;
+
+		of_property_read_u32_index(pnode, "allwinner,irq-pin-map",
+					   i * 6 + 3, &gpio_firstpin);
+		of_property_read_u32_index(pnode, "allwinner,irq-pin-map",
+					   i * 6 + 5, &length);
+		if ((gpio_firstpin > pin->number % 32) ||
+		    (gpio_firstpin + length <= pin->number % 32))
+			continue;
+
+		if (irq_port) {
+			of_property_read_u32_index(pnode,
+						   "allwinner,irq-pin-map",
+						   i * 6 + 0, &reg);
+			*irq_port = reg;
+		}
+		if (irq_pin) {
+			of_property_read_u32_index(pnode,
+						   "allwinner,irq-pin-map",
+						   i * 6 + 1, &reg);
+			*irq_pin = reg + (pin->number % 32) - gpio_firstpin;
+		}
+		if (muxval) {
+			of_property_read_u32_index(pnode,
+						   "allwinner,irq-pin-map",
+						   i * 6 + 4, &reg);
+			*muxval = reg;
+		}
+
+
+		return true;
+	}
+
+	return false;
+}
+
+/*
+ * Work out the number of functions each pin has. Provide memory to hold
+ * the per-function information and assign it to the pin table.
+ * Fill in the GPIO in/out functions every pin has, also add an "irq"
+ * function for those pins in IRQ-capable ports.
+ */
+static int prepare_function_table(struct device *dev, struct device_node *pnode,
+				  struct sunxi_desc_pin *pins, int npins)
+{
+	struct device_node *node;
+	struct property *prop;
+	struct sunxi_desc_function *func;
+	int num_funcs, i;
+
+	/*
+	 * We need at least three functions per pin:
+	 * - one for GPIO in
+	 * - one for GPIO out
+	 * - one for the sentinel signalling the end of the list
+	 */
+	num_funcs = 3 * npins;
+
+	/* Add a function for each pin in a port supporting interrupts. */
+	for (i = 0; i < npins; i++) {
+		if (get_irq_pin(pnode, &pins[i].pin, NULL, NULL, NULL)) {
+			pins[i].variant++;
+			num_funcs++;
+		}
+	}
+
+	/*
+	 * Go over each pin group (every child of the pinctrl DT node) and
+	 * add the number of special functions each pins has. Also update the
+	 * total number of functions required.
+	 * We might slightly overshoot here in case of double definitions.
+	 */
+	for_each_child_of_node(pnode, node) {
+		const char *name;
+
+		of_property_for_each_string(node, "pins", prop, name) {
+			for (i = 0; i < npins; i++) {
+				if (strcmp(pins[i].pin.name, name))
+					continue;
+
+				pins[i].variant++;
+				num_funcs++;
+				break;
+			}
+		}
+	}
+
+	/*
+	 * Allocate the memory needed for the functions in one table.
+	 * We later use pointers into this table to mark each pin.
+	 */
+	func = devm_kzalloc(dev, num_funcs * sizeof(*func), GFP_KERNEL);
+	if (!func)
+		return -ENOMEM;
+
+	/* Assign the functions memory and fill in GPIOs, IRQ and a sentinel. */
+	for (i = 0; i < npins; i++) {
+		int lastfunc = pins[i].variant + 1;
+
+		func[0].name = "gpio_in";
+		func[0].muxval = 0;
+		func[1].name = "gpio_out";
+		func[1].muxval = 1;
+
+		if (get_irq_pin(pnode, &pins[i].pin,
+				&func[lastfunc].irqbank,
+				&func[lastfunc].irqnum,
+				&func[lastfunc].muxval))
+			func[lastfunc].name = "irq";
+
+		pins[i].functions = func;
+
+		/* Skip over the other needed functions and the sentinel. */
+		func += pins[i].variant + 3;
+
+		/*
+		 * Reset the value for filling in the remaining functions
+		 * behind the GPIOs later.
+		 */
+		pins[i].variant = 2;
+	}
+
+	return 0;
+}
+
+/*
+ * Iterate over all pins in this group and add the function name and its
+ * mux value to the respective pin.
+ */
+static void fill_pin_function(struct device *dev, struct device_node *node,
+			      struct sunxi_desc_pin *pins, int npins)
+{
+	const char *name, *funcname;
+	struct sunxi_desc_function *func;
+	struct property *prop;
+	int pin, i;
+	u8 muxval;
+
+	if (of_property_read_string(node, "function", &funcname)) {
+		dev_warn(dev, "missing \"function\" property\n");
+		return;
+	}
+
+	of_property_for_each_string(node, "pins", prop, name) {
+		/* Find the index of this pin in our table. */
+		for (pin = 0; pin < npins; pin++)
+			if (!strcmp(pins[pin].pin.name, name))
+				break;
+		if (pin == npins) {
+			dev_warn(dev, "%s: cannot find pin %s\n",
+				 of_node_full_name(node), name);
+			continue;
+		}
+
+		/* Read the associated mux value. */
+		muxval = sunxi_pinctrl_dt_read_pinmux(node, pin);
+		if (muxval == INVALID_MUX) {
+			dev_warn(dev, "%s: invalid mux value for pin %s\n",
+				 of_node_full_name(node), name);
+			continue;
+		}
+
+		/*
+		 * Check for double definitions by comparing the to-be-added
+		 * function with already assigned ones.
+		 * Ignore identical pairs (function name and mux value the
+		 * same), but warn about conflicting assignments.
+		 */
+		for (i = 2; i < pins[pin].variant; i++) {
+			func = &pins[pin].functions[i];
+
+			/* Skip over totally unrelated functions. */
+			if (strcmp(func->name, funcname) &&
+			    func->muxval != muxval)
+				continue;
+
+			/* Ignore (but skip below) any identical functions. */
+			if (!strcmp(func->name, funcname) &&
+			    muxval == func->muxval)
+				break;
+
+			dev_warn(dev,
+				 "pin %s: function %s redefined to mux %d\n",
+				 name, funcname, muxval);
+			break;
+		}
+
+		/* Skip any pins with that function already assigned. */
+		if (i < pins[pin].variant)
+			continue;
+
+		/* Assign function and muxval to the next free slot. */
+		func = &pins[pin].functions[pins[pin].variant];
+		func->muxval = muxval;
+		func->name = funcname;
+
+		pins[pin].variant++;
+	}
+}
+
+/* Iterate over all IRQ pin maps and find out the number of distinct ports. */
+static int count_nr_irq_banks(struct device_node *node)
+{
+	u32 port_mask = 0, irq_port;
+	int i = 0;
+
+	for (i = 0;
+	     !of_property_read_u32_index(node, "allwinner,irq-pin-map",
+					 i * 6 + 0, &irq_port);
+	     i++)
+		port_mask |= BIT(irq_port);
+
+	return hweight32(port_mask);
+}
+
+static int sunxi_generic_pinctrl_probe(struct platform_device *pdev)
+{
+	struct device_node *pnode = pdev->dev.of_node, *node;
+	struct sunxi_pinctrl_desc *desc;
+	struct sunxi_desc_pin *pins;
+	u32 port_base = 0;
+	int npins, ret, i;
+
+	of_property_read_u32(pnode, "allwinner,port-base", &port_base);
+	npins = build_pins_table(&pdev->dev, pnode, port_base, &pins);
+	if (npins < 0)
+		return npins;
+
+	ret = prepare_function_table(&pdev->dev, pnode, pins, npins);
+	if (ret)
+		return ret;
+
+	/*
+	 * Now iterate over all groups and add the respective function name
+	 * and mux values to each pin listed within.
+	 */
+	for_each_child_of_node(pnode, node)
+		fill_pin_function(&pdev->dev, node, pins, npins);
+
+	/* Clear the temporary storage. */
+	for (i = 0; i < npins; i++)
+		pins[i].variant = 0;
+
+	desc = devm_kzalloc(&pdev->dev, sizeof(*desc), GFP_KERNEL);
+	if (!desc)
+		return -ENOMEM;
+
+	desc->pins = pins;
+	desc->npins = npins;
+	desc->pin_base = port_base * 32;
+
+	desc->irq_banks = count_nr_irq_banks(pnode);
+	of_property_read_u32(pnode, "allwinner,irq-bank-base",
+			     &desc->irq_bank_base);
+	if (of_property_read_bool(pnode, "allwinner,irq_read_needs_mux"))
+		desc->irq_read_needs_mux = true;
+
+	return sunxi_pinctrl_init_with_variant(pdev, desc, 0);
+}
+
+static const struct of_device_id sunxi_pinctrl_match[] = {
+	{ .compatible = "allwinner,sunxi-pinctrl", },
+	{ },
+};
+
+static struct platform_driver sunxi_pinctrl_driver = {
+	.probe  = sunxi_generic_pinctrl_probe,
+	.driver = {
+		.name           = "sunxi-pinctrl",
+		.of_match_table = sunxi_pinctrl_match,
+	},
+};
+builtin_platform_driver(sunxi_pinctrl_driver);
-- 
2.14.1

Re: [RFC PATCH 2/3] pinctrl: sunxi: introduce DT-based generic driver

[Tony Lindgren]

* Andre Przywara <andre.przywara@arm.com> [171113 01:28]:
> This driver (shim) allows to fully describe an Allwinner pin controller
> and its GPIO ports in device tree nodes.
> It will read some newly introduced properties to build a table
> describing the pins and their routing. This table matches those that we
> have hardcoded for various SoCs in the kernel so far.
> After this table has been created, it will be handed over to the actual
> pinctrl driver, which registers it with the pinctrl subsystem.

Hmm so how come you're not using GENERIC_PINCTRL_GROUPS and
GENERIC_PINMUX_FUNCTIONS that we now have?

These together with the #pinctrl-cells property should make things
quite easy and should also simplify your binding hopefully.

Regards,

Tony

[RFC PATCH 2/3] pinctrl: sunxi: introduce DT-based generic driver

[Tony Lindgren]

* Andre Przywara <andre.przywara@arm.com> [171113 01:28]:
> This driver (shim) allows to fully describe an Allwinner pin controller
> and its GPIO ports in device tree nodes.
> It will read some newly introduced properties to build a table
> describing the pins and their routing. This table matches those that we
> have hardcoded for various SoCs in the kernel so far.
> After this table has been created, it will be handed over to the actual
> pinctrl driver, which registers it with the pinctrl subsystem.

Hmm so how come you're not using GENERIC_PINCTRL_GROUPS and
GENERIC_PINMUX_FUNCTIONS that we now have?

These together with the #pinctrl-cells property should make things
quite easy and should also simplify your binding hopefully.

Regards,

Tony

[RFC PATCH 3/3] arm64: dts: allwinner: enhance A64 .dtsi with new pinctrl binding

[Andre Przywara]

Enhance the existing pinctrl DT nodes for the Allwinner A64 SoC to
include the new properties the generic, DT-based binding introduced.
This allows any generic driver to support this SoC as well.
The DT nodes stay fully compatible with the old binding, so existing
drivers continue to work without restrictions.
But on top of that new DT users can directly use the information here
and can do without a hardcoded table.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 8c8db1b057df..58fdae32240a 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -269,7 +269,8 @@
 		};
 
 		pio: pinctrl@1c20800 {
-			compatible = "allwinner,sun50i-a64-pinctrl";
+			compatible = "allwinner,sun50i-a64-pinctrl",
+				     "allwinner,sunxi-pinctrl";
 			reg = <0x01c20800 0x400>;
 			interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>,
 				     <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>,
@@ -279,16 +280,22 @@
 			#gpio-cells = <3>;
 			interrupt-controller;
 			#interrupt-cells = <3>;
+			allwinner,gpio-pins = <0 10 17 25 18 7 14 12>;
+			allwinner,irq-pin-map = <0 0 1 0 6 10>,
+						<1 0 6 0 6 14>,
+						<2 0 7 0 6 12>;
 
 			i2c1_pins: i2c1_pins {
 				pins = "PH2", "PH3";
 				function = "i2c1";
+				pinmux = <2>;
 			};
 
 			mmc0_pins: mmc0-pins {
 				pins = "PF0", "PF1", "PF2", "PF3",
 				       "PF4", "PF5";
 				function = "mmc0";
+				pinmux = <2>;
 				drive-strength = <30>;
 				bias-pull-up;
 			};
@@ -297,6 +304,7 @@
 				pins = "PG0", "PG1", "PG2", "PG3",
 				       "PG4", "PG5";
 				function = "mmc1";
+				pinmux = <2>;
 				drive-strength = <30>;
 				bias-pull-up;
 			};
@@ -306,6 +314,7 @@
 				       "PC10","PC11", "PC12", "PC13",
 				       "PC14", "PC15", "PC16";
 				function = "mmc2";
+				pinmux = <3>;
 				drive-strength = <30>;
 				bias-pull-up;
 			};
@@ -314,6 +323,7 @@
 				pins = "PD10", "PD11", "PD13", "PD14", "PD17",
 				       "PD18", "PD19", "PD20", "PD22", "PD23";
 				function = "emac";
+				pinmux = <4>;
 				drive-strength = <40>;
 			};
 
@@ -322,42 +332,50 @@
 				       "PD13", "PD15", "PD16", "PD17", "PD18",
 				       "PD19", "PD20", "PD21", "PD22", "PD23";
 				function = "emac";
+				pinmux = <4>;
 				drive-strength = <40>;
 			};
 
 			uart0_pins_a: uart0@0 {
 				pins = "PB8", "PB9";
 				function = "uart0";
+				pinmux = <4>;
 			};
 
 			uart1_pins: uart1_pins {
 				pins = "PG6", "PG7";
 				function = "uart1";
+				pinmux = <2>;
 			};
 
 			uart1_rts_cts_pins: uart1_rts_cts_pins {
 				pins = "PG8", "PG9";
 				function = "uart1";
+				pinmux = <2>;
 			};
 
 			uart2_pins: uart2-pins {
 				pins = "PB0", "PB1";
 				function = "uart2";
+				pinmux = <2>;
 			};
 
 			uart3_pins: uart3-pins {
 				pins = "PD0", "PD1";
 				function = "uart3";
+				pinmux = <3>;
 			};
 
 			uart4_pins: uart4-pins {
 				pins = "PD2", "PD3";
 				function = "uart4";
+				pinmux = <3>;
 			};
 
 			uart4_rts_cts_pins: uart4-rts-cts-pins {
 				pins = "PD4", "PD5";
 				function = "uart4";
+				pinmux = <3>;
 			};
 		};
 
@@ -487,7 +505,8 @@
 		};
 
 		r_pio: pinctrl@01f02c00 {
-			compatible = "allwinner,sun50i-a64-r-pinctrl";
+			compatible = "allwinner,sun50i-a64-r-pinctrl",
+				     "allwinner,sunxi-pinctrl";
 			reg = <0x01f02c00 0x400>;
 			interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&r_ccu CLK_APB0_PIO>, <&osc24M>, <&osc32k>;
@@ -496,10 +515,14 @@
 			#gpio-cells = <3>;
 			interrupt-controller;
 			#interrupt-cells = <3>;
+			allwinner,gpio-pins = <13>;
+			allwinner,port-base = <11>;
+			allwinner,irq-pin-map = <0 0 11 0 6 13>;
 
 			r_rsb_pins: rsb@0 {
 				pins = "PL0", "PL1";
 				function = "s_rsb";
+				pinmux = <2>;
 			};
 		};
 
-- 
2.14.1

[RFC PATCH 3/3] arm64: dts: allwinner: enhance A64 .dtsi with new pinctrl binding

[Andre Przywara]

Enhance the existing pinctrl DT nodes for the Allwinner A64 SoC to
include the new properties the generic, DT-based binding introduced.
This allows any generic driver to support this SoC as well.
The DT nodes stay fully compatible with the old binding, so existing
drivers continue to work without restrictions.
But on top of that new DT users can directly use the information here
and can do without a hardcoded table.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi | 27 +++++++++++++++++++++++++--
 1 file changed, 25 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
index 8c8db1b057df..58fdae32240a 100644
--- a/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
+++ b/arch/arm64/boot/dts/allwinner/sun50i-a64.dtsi
@@ -269,7 +269,8 @@
 		};
 
 		pio: pinctrl at 1c20800 {
-			compatible = "allwinner,sun50i-a64-pinctrl";
+			compatible = "allwinner,sun50i-a64-pinctrl",
+				     "allwinner,sunxi-pinctrl";
 			reg = <0x01c20800 0x400>;
 			interrupts = <GIC_SPI 11 IRQ_TYPE_LEVEL_HIGH>,
 				     <GIC_SPI 17 IRQ_TYPE_LEVEL_HIGH>,
@@ -279,16 +280,22 @@
 			#gpio-cells = <3>;
 			interrupt-controller;
 			#interrupt-cells = <3>;
+			allwinner,gpio-pins = <0 10 17 25 18 7 14 12>;
+			allwinner,irq-pin-map = <0 0 1 0 6 10>,
+						<1 0 6 0 6 14>,
+						<2 0 7 0 6 12>;
 
 			i2c1_pins: i2c1_pins {
 				pins = "PH2", "PH3";
 				function = "i2c1";
+				pinmux = <2>;
 			};
 
 			mmc0_pins: mmc0-pins {
 				pins = "PF0", "PF1", "PF2", "PF3",
 				       "PF4", "PF5";
 				function = "mmc0";
+				pinmux = <2>;
 				drive-strength = <30>;
 				bias-pull-up;
 			};
@@ -297,6 +304,7 @@
 				pins = "PG0", "PG1", "PG2", "PG3",
 				       "PG4", "PG5";
 				function = "mmc1";
+				pinmux = <2>;
 				drive-strength = <30>;
 				bias-pull-up;
 			};
@@ -306,6 +314,7 @@
 				       "PC10","PC11", "PC12", "PC13",
 				       "PC14", "PC15", "PC16";
 				function = "mmc2";
+				pinmux = <3>;
 				drive-strength = <30>;
 				bias-pull-up;
 			};
@@ -314,6 +323,7 @@
 				pins = "PD10", "PD11", "PD13", "PD14", "PD17",
 				       "PD18", "PD19", "PD20", "PD22", "PD23";
 				function = "emac";
+				pinmux = <4>;
 				drive-strength = <40>;
 			};
 
@@ -322,42 +332,50 @@
 				       "PD13", "PD15", "PD16", "PD17", "PD18",
 				       "PD19", "PD20", "PD21", "PD22", "PD23";
 				function = "emac";
+				pinmux = <4>;
 				drive-strength = <40>;
 			};
 
 			uart0_pins_a: uart0 at 0 {
 				pins = "PB8", "PB9";
 				function = "uart0";
+				pinmux = <4>;
 			};
 
 			uart1_pins: uart1_pins {
 				pins = "PG6", "PG7";
 				function = "uart1";
+				pinmux = <2>;
 			};
 
 			uart1_rts_cts_pins: uart1_rts_cts_pins {
 				pins = "PG8", "PG9";
 				function = "uart1";
+				pinmux = <2>;
 			};
 
 			uart2_pins: uart2-pins {
 				pins = "PB0", "PB1";
 				function = "uart2";
+				pinmux = <2>;
 			};
 
 			uart3_pins: uart3-pins {
 				pins = "PD0", "PD1";
 				function = "uart3";
+				pinmux = <3>;
 			};
 
 			uart4_pins: uart4-pins {
 				pins = "PD2", "PD3";
 				function = "uart4";
+				pinmux = <3>;
 			};
 
 			uart4_rts_cts_pins: uart4-rts-cts-pins {
 				pins = "PD4", "PD5";
 				function = "uart4";
+				pinmux = <3>;
 			};
 		};
 
@@ -487,7 +505,8 @@
 		};
 
 		r_pio: pinctrl at 01f02c00 {
-			compatible = "allwinner,sun50i-a64-r-pinctrl";
+			compatible = "allwinner,sun50i-a64-r-pinctrl",
+				     "allwinner,sunxi-pinctrl";
 			reg = <0x01f02c00 0x400>;
 			interrupts = <GIC_SPI 45 IRQ_TYPE_LEVEL_HIGH>;
 			clocks = <&r_ccu CLK_APB0_PIO>, <&osc24M>, <&osc32k>;
@@ -496,10 +515,14 @@
 			#gpio-cells = <3>;
 			interrupt-controller;
 			#interrupt-cells = <3>;
+			allwinner,gpio-pins = <13>;
+			allwinner,port-base = <11>;
+			allwinner,irq-pin-map = <0 0 11 0 6 13>;
 
 			r_rsb_pins: rsb at 0 {
 				pins = "PL0", "PL1";
 				function = "s_rsb";
+				pinmux = <2>;
 			};
 		};
 
-- 
2.14.1

Re: [RFC PATCH 0/3] pinctrl: sunxi: Add DT-based generic pinctrl driver

[Linus Walleij]

On Mon, Nov 13, 2017 at 2:25 AM, Andre Przywara <andre.przywara@arm.com> wrote:

> so far the pinctrl driver for supporting a particular Allwinner SoC requires
> a hardcoded table in the kernel code.
(...)
> This series here moves the data encoded in the table so far into the DT itself,
> removing the need for a hardcoded table in the kernel.
(...)

The DT maintainers have been pretty clear on that they don't like
using the the DT as a generic fit-all information dump. They
prefer to look up hardware data from per-soc compatible strings.

I have been sceptic about it too, on the grounds that I think it
make configuration and multiplatform kernels easy, while making
debugging and reading code+device tree hard, also affecting
maintenance cost.

I'd like to have Maxime's buy-in before we progress and also some
discussion on these themes in general.

> The approach taken here is to parse the DT and generate the table with
> the help of additional properties, then hand this table over to the existing
> driver. This is covered by three basic extensions to the DT binding:

I adressed this in the bindings patch.

> The benefit of this series is two-fold:
> - Future SoCs don't need an in-kernel table anymore. They can describe
>   everything in the DT,

It can be debated whether that is really a good thing or actually
a bad thing for the reasons stated above.

Also an additional bad thing is inconsistency between different
SoCs.

What we have in the kernel for all-DT is pinctrl-single.c.

This exists for the case where there is exactly one register per
pin and all you have is strange macro files from the ASIC people
that noone understands. OMAP and HiSilicon is using this.
It's a compromise, I'm not super-happy with that driver at all times
but it is for a very strongly specified case.

Would it be possible for you guys to simply use/embrace/extend
pinctrl-single.c if you want to go this route?

Any higher order of complexity than "one register per pin" I think
is better handled by open coding it than trying to push things into
the device tree, because of readability, debuggability and maintenance
issues.

Yours,
Linus Walleij

[RFC PATCH 0/3] pinctrl: sunxi: Add DT-based generic pinctrl driver

[Linus Walleij]

On Mon, Nov 13, 2017 at 2:25 AM, Andre Przywara <andre.przywara@arm.com> wrote:

> so far the pinctrl driver for supporting a particular Allwinner SoC requires
> a hardcoded table in the kernel code.
(...)
> This series here moves the data encoded in the table so far into the DT itself,
> removing the need for a hardcoded table in the kernel.
(...)

The DT maintainers have been pretty clear on that they don't like
using the the DT as a generic fit-all information dump. They
prefer to look up hardware data from per-soc compatible strings.

I have been sceptic about it too, on the grounds that I think it
make configuration and multiplatform kernels easy, while making
debugging and reading code+device tree hard, also affecting
maintenance cost.

I'd like to have Maxime's buy-in before we progress and also some
discussion on these themes in general.

> The approach taken here is to parse the DT and generate the table with
> the help of additional properties, then hand this table over to the existing
> driver. This is covered by three basic extensions to the DT binding:

I adressed this in the bindings patch.

> The benefit of this series is two-fold:
> - Future SoCs don't need an in-kernel table anymore. They can describe
>   everything in the DT,

It can be debated whether that is really a good thing or actually
a bad thing for the reasons stated above.

Also an additional bad thing is inconsistency between different
SoCs.

What we have in the kernel for all-DT is pinctrl-single.c.

This exists for the case where there is exactly one register per
pin and all you have is strange macro files from the ASIC people
that noone understands. OMAP and HiSilicon is using this.
It's a compromise, I'm not super-happy with that driver at all times
but it is for a very strongly specified case.

Would it be possible for you guys to simply use/embrace/extend
pinctrl-single.c if you want to go this route?

Any higher order of complexity than "one register per pin" I think
is better handled by open coding it than trying to push things into
the device tree, because of readability, debuggability and maintenance
issues.

Yours,
Linus Walleij

Re: [RFC PATCH 0/3] pinctrl: sunxi: Add DT-based generic pinctrl driver

[Andre Przywara]

Hi,

On 24/11/17 10:28, Linus Walleij wrote:
> On Mon, Nov 13, 2017 at 2:25 AM, Andre Przywara <andre.przywara@arm.com> wrote:
> 
>> so far the pinctrl driver for supporting a particular Allwinner SoC requires
>> a hardcoded table in the kernel code.
> (...)
>> This series here moves the data encoded in the table so far into the DT itself,
>> removing the need for a hardcoded table in the kernel.
> (...)
> 
> The DT maintainers have been pretty clear on that they don't like
> using the the DT as a generic fit-all information dump. They
> prefer to look up hardware data from per-soc compatible strings.
> 
> I have been sceptic about it too, on the grounds that I think it
> make configuration and multiplatform kernels easy, while making
> debugging and reading code+device tree hard, also affecting
> maintenance cost.
> 
> I'd like to have Maxime's buy-in before we progress and also some
> discussion on these themes in general.

Indeed this was one of the goals of this series. I can understand were
we came from and that we had those in-kernel tables in the past.
I am just a bit worried that with Allwinner recently playing the SKU
game we end up with tons of tables for only slightly different SoCs (see
the H3 and H5, for instance). And with single image kernels we pile up
quite some *data* in each kernel, which is of little interest for
everyone else.

Also my understanding is that the actual Allwinner pin controller IP
(register map) is very much the same across all SoCs. Mostly the only
difference is the mapping between pins and mux functions, which we
express in the DT already anyway (in the subnodes). And this is really a
poster book example of what DT should be doing: express the specific
mappings of a particular implementation. I don't see why this would need
to be per-board only, if we can pull this up to the SoC level.

>> The approach taken here is to parse the DT and generate the table with
>> the help of additional properties, then hand this table over to the existing
>> driver. This is covered by three basic extensions to the DT binding:
> 
> I adressed this in the bindings patch.
> 
>> The benefit of this series is two-fold:
>> - Future SoCs don't need an in-kernel table anymore. They can describe
>>   everything in the DT,
> 
> It can be debated whether that is really a good thing or actually
> a bad thing for the reasons stated above.

The point is that we already rely on the DT anyway, just that we use a
*string* to specify a certain mux function. The in-kernel table just
maps this to an actual SoC-specific register value. I think the GPIO
subsystem needs the name, hence I am just adding the pinmux property.

> Also an additional bad thing is inconsistency between different
> SoCs.

Well, does that mean that we should never change anything? Consistency
is nice, but should not be an excuse for not improving things. And I
tried to embrace the existing driver by designing this on top of it.

> What we have in the kernel for all-DT is pinctrl-single.c.
> 
> This exists for the case where there is exactly one register per
> pin and all you have is strange macro files from the ASIC people
> that noone understands. OMAP and HiSilicon is using this.
> It's a compromise, I'm not super-happy with that driver at all times
> but it is for a very strongly specified case.
> 
> Would it be possible for you guys to simply use/embrace/extend
> pinctrl-single.c if you want to go this route?

Thanks for the pointer, I will have a look.

> Any higher order of complexity than "one register per pin" I think
> is better handled by open coding it than trying to push things into
> the device tree, because of readability, debuggability and maintenance
> issues.

I don't see how this binding would make this worse.

Cheers,
Andre.

[RFC PATCH 0/3] pinctrl: sunxi: Add DT-based generic pinctrl driver

[Andre Przywara]

Hi,

On 24/11/17 10:28, Linus Walleij wrote:
> On Mon, Nov 13, 2017 at 2:25 AM, Andre Przywara <andre.przywara@arm.com> wrote:
> 
>> so far the pinctrl driver for supporting a particular Allwinner SoC requires
>> a hardcoded table in the kernel code.
> (...)
>> This series here moves the data encoded in the table so far into the DT itself,
>> removing the need for a hardcoded table in the kernel.
> (...)
> 
> The DT maintainers have been pretty clear on that they don't like
> using the the DT as a generic fit-all information dump. They
> prefer to look up hardware data from per-soc compatible strings.
> 
> I have been sceptic about it too, on the grounds that I think it
> make configuration and multiplatform kernels easy, while making
> debugging and reading code+device tree hard, also affecting
> maintenance cost.
> 
> I'd like to have Maxime's buy-in before we progress and also some
> discussion on these themes in general.

Indeed this was one of the goals of this series. I can understand were
we came from and that we had those in-kernel tables in the past.
I am just a bit worried that with Allwinner recently playing the SKU
game we end up with tons of tables for only slightly different SoCs (see
the H3 and H5, for instance). And with single image kernels we pile up
quite some *data* in each kernel, which is of little interest for
everyone else.

Also my understanding is that the actual Allwinner pin controller IP
(register map) is very much the same across all SoCs. Mostly the only
difference is the mapping between pins and mux functions, which we
express in the DT already anyway (in the subnodes). And this is really a
poster book example of what DT should be doing: express the specific
mappings of a particular implementation. I don't see why this would need
to be per-board only, if we can pull this up to the SoC level.

>> The approach taken here is to parse the DT and generate the table with
>> the help of additional properties, then hand this table over to the existing
>> driver. This is covered by three basic extensions to the DT binding:
> 
> I adressed this in the bindings patch.
> 
>> The benefit of this series is two-fold:
>> - Future SoCs don't need an in-kernel table anymore. They can describe
>>   everything in the DT,
> 
> It can be debated whether that is really a good thing or actually
> a bad thing for the reasons stated above.

The point is that we already rely on the DT anyway, just that we use a
*string* to specify a certain mux function. The in-kernel table just
maps this to an actual SoC-specific register value. I think the GPIO
subsystem needs the name, hence I am just adding the pinmux property.

> Also an additional bad thing is inconsistency between different
> SoCs.

Well, does that mean that we should never change anything? Consistency
is nice, but should not be an excuse for not improving things. And I
tried to embrace the existing driver by designing this on top of it.

> What we have in the kernel for all-DT is pinctrl-single.c.
> 
> This exists for the case where there is exactly one register per
> pin and all you have is strange macro files from the ASIC people
> that noone understands. OMAP and HiSilicon is using this.
> It's a compromise, I'm not super-happy with that driver at all times
> but it is for a very strongly specified case.
> 
> Would it be possible for you guys to simply use/embrace/extend
> pinctrl-single.c if you want to go this route?

Thanks for the pointer, I will have a look.

> Any higher order of complexity than "one register per pin" I think
> is better handled by open coding it than trying to push things into
> the device tree, because of readability, debuggability and maintenance
> issues.

I don't see how this binding would make this worse.

Cheers,
Andre.

Re: [RFC PATCH 0/3] pinctrl: sunxi: Add DT-based generic pinctrl driver

[Linus Walleij]

On Fri, Nov 24, 2017 at 1:05 PM, Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org> wrote:
> On 24/11/17 10:28, Linus Walleij wrote:

>> The DT maintainers have been pretty clear on that they don't like
>> using the the DT as a generic fit-all information dump. They
>> prefer to look up hardware data from per-soc compatible strings.
(...)
> I am just a bit worried that with Allwinner recently playing the SKU
> game we end up with tons of tables for only slightly different SoCs (see
> the H3 and H5, for instance). And with single image kernels we pile up
> quite some *data* in each kernel, which is of little interest for
> everyone else.

So what you are saying is that you want to use the DTS for
data dumping and what I'm saying is that the DT maintainers
do not like that stance.

They will have to speak on the issue directly before we continue
I think.

I have been getting a *LOT* of pushback to putting large amounts
of data and configuration in the DTS recently, so IIUC that is something
they simply don't like, probably for good reasons.

C.f:
https://www.spinics.net/lists/dri-devel/msg150321.html

> Also my understanding is that the actual Allwinner pin controller IP
> (register map) is very much the same across all SoCs. Mostly the only
> difference is the mapping between pins and mux functions, which we
> express in the DT already anyway (in the subnodes). And this is really a
> poster book example of what DT should be doing: express the specific
> mappings of a particular implementation. I don't see why this would need
> to be per-board only, if we can pull this up to the SoC level.

It's not me you need to sell this point.

You need to sell it to the DT maintainers.

Yours,
Linus Walleij

[RFC PATCH 0/3] pinctrl: sunxi: Add DT-based generic pinctrl driver

[Linus Walleij]

On Fri, Nov 24, 2017 at 1:05 PM, Andre Przywara <andre.przywara@arm.com> wrote:
> On 24/11/17 10:28, Linus Walleij wrote:

>> The DT maintainers have been pretty clear on that they don't like
>> using the the DT as a generic fit-all information dump. They
>> prefer to look up hardware data from per-soc compatible strings.
(...)
> I am just a bit worried that with Allwinner recently playing the SKU
> game we end up with tons of tables for only slightly different SoCs (see
> the H3 and H5, for instance). And with single image kernels we pile up
> quite some *data* in each kernel, which is of little interest for
> everyone else.

So what you are saying is that you want to use the DTS for
data dumping and what I'm saying is that the DT maintainers
do not like that stance.

They will have to speak on the issue directly before we continue
I think.

I have been getting a *LOT* of pushback to putting large amounts
of data and configuration in the DTS recently, so IIUC that is something
they simply don't like, probably for good reasons.

C.f:
https://www.spinics.net/lists/dri-devel/msg150321.html

> Also my understanding is that the actual Allwinner pin controller IP
> (register map) is very much the same across all SoCs. Mostly the only
> difference is the mapping between pins and mux functions, which we
> express in the DT already anyway (in the subnodes). And this is really a
> poster book example of what DT should be doing: express the specific
> mappings of a particular implementation. I don't see why this would need
> to be per-board only, if we can pull this up to the SoC level.

It's not me you need to sell this point.

You need to sell it to the DT maintainers.

Yours,
Linus Walleij

Re: [RFC PATCH 0/3] pinctrl: sunxi: Add DT-based generic pinctrl driver

[Andre Przywara]

Hi,

On 30/11/17 15:20, Linus Walleij wrote:
> On Fri, Nov 24, 2017 at 1:05 PM, Andre Przywara <andre.przywara-5wv7dgnIgG8@public.gmane.org> wrote:
>> On 24/11/17 10:28, Linus Walleij wrote:
> 
>>> The DT maintainers have been pretty clear on that they don't like
>>> using the the DT as a generic fit-all information dump. They
>>> prefer to look up hardware data from per-soc compatible strings.
> (...)
>> I am just a bit worried that with Allwinner recently playing the SKU
>> game we end up with tons of tables for only slightly different SoCs (see
>> the H3 and H5, for instance). And with single image kernels we pile up
>> quite some *data* in each kernel, which is of little interest for
>> everyone else.
> 
> So what you are saying is that you want to use the DTS for
> data dumping

Well, that's what the DT is for, right? As opposed to dump the data in
*every* kernel (Linux, U-Boot, BSD) for *every* SoC.
And, following this argument, why do we have reg and interrupt
properties if we could derive them from the compatible string as well?
Those are SoC specific and immutable by a board as well.

It seems the discussion went slightly heated and astray, but basically I
am after two things, maybe we should separate them:

1) Put the actual mux value into the DT, next to the already existing
function property. That is a small addition, but has the great effect of
avoiding to hard code this information for *each* supported SoC in
*every* kernel. Doing so just sounds like bonkers to me, sorry.

If there is really so much resistance against also *using* this
information from Linux, I would be happy to at least add the already
existing and generic "pinmux" property to the binding. This way we could
add those values to the .dtsi(s), and other OSes could use them. No need
for a driver change in Linux, then. I am not buying this molly guard
argument at all, but this way we could keep this protection in the kernel.
This would immediately allow to get an easy DT based pinctrl driver on
the road for U-Boot.

2) Add the (very few!) properties to the pinctrl root node that we need
to size and enumerate the pins. This would allow generic pinctrl driver
support, so one thing less to worry when bringing up a new SoC (variant).

So my main goal is more about the binding and the DTs.
Linux could just ignore them, though I don't see a good reason for us to
not make use of those for good.

I believe that both are not really a big issue and I don't really
understand why there is so much opposition towards it.

> and what I'm saying is that the DT maintainers
> do not like that stance.
> 
> They will have to speak on the issue directly before we continue
> I think.

Fair enough.

> I have been getting a *LOT* of pushback to putting large amounts
> of data and configuration in the DTS recently, so IIUC that is something
> they simply don't like, probably for good reasons.

That's a shame, maybe it's to limit the amount of review and maintenance
needed? And it's a safe bet to have a simple binding, where nothing can
go wrong? And since we can't predict the future?

I don't know, but in this case we have quite a sample of already
existing controllers and can make an much more educated guess for a
better and compatible binding, kind of in hindsight.

> C.f:
> https://www.spinics.net/lists/dri-devel/msg150321.html
> 
>> Also my understanding is that the actual Allwinner pin controller IP
>> (register map) is very much the same across all SoCs. Mostly the only
>> difference is the mapping between pins and mux functions, which we
>> express in the DT already anyway (in the subnodes). And this is really a
>> poster book example of what DT should be doing: express the specific
>> mappings of a particular implementation. I don't see why this would need
>> to be per-board only, if we can pull this up to the SoC level.
> 
> It's not me you need to sell this point.
> 
> You need to sell it to the DT maintainers.

Well, so far I only got some push back from Thierry, Maxime and you. My
understanding of the DT maintainers' position is to leave those details
to the subsystem maintainers.

Cheers,
Andre.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC PATCH 0/3] pinctrl: sunxi: Add DT-based generic pinctrl driver

[Andre Przywara]

Hi,

On 30/11/17 15:20, Linus Walleij wrote:
> On Fri, Nov 24, 2017 at 1:05 PM, Andre Przywara <andre.przywara@arm.com> wrote:
>> On 24/11/17 10:28, Linus Walleij wrote:
> 
>>> The DT maintainers have been pretty clear on that they don't like
>>> using the the DT as a generic fit-all information dump. They
>>> prefer to look up hardware data from per-soc compatible strings.
> (...)
>> I am just a bit worried that with Allwinner recently playing the SKU
>> game we end up with tons of tables for only slightly different SoCs (see
>> the H3 and H5, for instance). And with single image kernels we pile up
>> quite some *data* in each kernel, which is of little interest for
>> everyone else.
> 
> So what you are saying is that you want to use the DTS for
> data dumping

Well, that's what the DT is for, right? As opposed to dump the data in
*every* kernel (Linux, U-Boot, BSD) for *every* SoC.
And, following this argument, why do we have reg and interrupt
properties if we could derive them from the compatible string as well?
Those are SoC specific and immutable by a board as well.

It seems the discussion went slightly heated and astray, but basically I
am after two things, maybe we should separate them:

1) Put the actual mux value into the DT, next to the already existing
function property. That is a small addition, but has the great effect of
avoiding to hard code this information for *each* supported SoC in
*every* kernel. Doing so just sounds like bonkers to me, sorry.

If there is really so much resistance against also *using* this
information from Linux, I would be happy to at least add the already
existing and generic "pinmux" property to the binding. This way we could
add those values to the .dtsi(s), and other OSes could use them. No need
for a driver change in Linux, then. I am not buying this molly guard
argument at all, but this way we could keep this protection in the kernel.
This would immediately allow to get an easy DT based pinctrl driver on
the road for U-Boot.

2) Add the (very few!) properties to the pinctrl root node that we need
to size and enumerate the pins. This would allow generic pinctrl driver
support, so one thing less to worry when bringing up a new SoC (variant).

So my main goal is more about the binding and the DTs.
Linux could just ignore them, though I don't see a good reason for us to
not make use of those for good.

I believe that both are not really a big issue and I don't really
understand why there is so much opposition towards it.

> and what I'm saying is that the DT maintainers
> do not like that stance.
> 
> They will have to speak on the issue directly before we continue
> I think.

Fair enough.

> I have been getting a *LOT* of pushback to putting large amounts
> of data and configuration in the DTS recently, so IIUC that is something
> they simply don't like, probably for good reasons.

That's a shame, maybe it's to limit the amount of review and maintenance
needed? And it's a safe bet to have a simple binding, where nothing can
go wrong? And since we can't predict the future?

I don't know, but in this case we have quite a sample of already
existing controllers and can make an much more educated guess for a
better and compatible binding, kind of in hindsight.

> C.f:
> https://www.spinics.net/lists/dri-devel/msg150321.html
> 
>> Also my understanding is that the actual Allwinner pin controller IP
>> (register map) is very much the same across all SoCs. Mostly the only
>> difference is the mapping between pins and mux functions, which we
>> express in the DT already anyway (in the subnodes). And this is really a
>> poster book example of what DT should be doing: express the specific
>> mappings of a particular implementation. I don't see why this would need
>> to be per-board only, if we can pull this up to the SoC level.
> 
> It's not me you need to sell this point.
> 
> You need to sell it to the DT maintainers.

Well, so far I only got some push back from Thierry, Maxime and you. My
understanding of the DT maintainers' position is to leave those details
to the subsystem maintainers.

Cheers,
Andre.