[RFC PATCH 0/2] pinctrl: sunxi: Introduce DT-based pinctrl builder

[RFC PATCH 0/2] pinctrl: sunxi: Introduce DT-based pinctrl builder

[Andre Przywara]

Hi,

since the dawn of time every Allwinner SoC dumped a rather large table
of data into the kernel, to describe the mapping between the pinctrl
function name and its mux value, for each pin.

This series introduces code that avoids that (for new SoCs), by instead
reading that information directly from the devicetree. We have per-pin
group nodes there anyway, and were just missing the mux value.

Compared to my previous effort almost exactly five years ago [1], this
new version drops the idea of describing the pinctrl data entirely in
the DT, instead it still relies on driver provided information for that.
That is more flexible, since it allows to introduce quirks and special
handling more cleanly, at the cost of still requiring a separate driver
file for each SoC. However this file is now very small, and can be
easily written and reviewed. All that is needed is the number of pins
per bank, plus information about each bank's IRQ capability.
Patch 2/2 shows an example, for the yet unsupported Allwinner V5 SoC.

On the DT side all that would be needed is *one* extra property per
pin group to announce the mux value:

	uart0_pb_pins: uart0-pb-pins {
		pins = "PB9", "PB10";
		function = "uart0";
		pinmux = <2>;
	};

The new code works by providing a function that builds the former
mapping table *at runtime*, by using both the driver provided
information, plus traversing all children of the pinctrl DT node, to
find all pin groups needed. This table looks the same as what we
hardcoded so far, so can easily be digested by the existing sunxi
pinctrl driver.

Please have a look and tell me whether this new approach has a better
future than my previous attempt.

Cheers,
Andre

[1] https://patchwork.ozlabs.org/project/linux-gpio/cover/20171113012523.2328-1-andre.przywara@arm.com/


Andre Przywara (2):
  pinctrl: sunxi: allow reading mux values from DT
  pinctrl: sunxi: Add support for the Allwinner V5 pin controller

 drivers/pinctrl/sunxi/Kconfig            |   5 +
 drivers/pinctrl/sunxi/Makefile           |   2 +
 drivers/pinctrl/sunxi/pinctrl-sun8i-v5.c |  52 ++++
 drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c | 355 +++++++++++++++++++++++
 drivers/pinctrl/sunxi/pinctrl-sunxi.h    |   8 +
 5 files changed, 422 insertions(+)
 create mode 100644 drivers/pinctrl/sunxi/pinctrl-sun8i-v5.c
 create mode 100644 drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c

-- 
2.35.5

[RFC PATCH 1/2] pinctrl: sunxi: allow reading mux values from DT

[Andre Przywara]

So far every Allwinner SoC needs a large table in the kernel code, to
describe the mapping between the pinctrl function names ("uart") and
the actual pincontroller mux value to be written into the registers.
This adds a lot of data into a single image kernel, and also looks
somewhat weird, as the DT can easily store the mux value.

Add some code that allows to avoid that table: the struct that describes
the existing pins will be build at *runtime*, based on very basic
information provided by the respective SoC's pinctrl driver. This
consists of the number of pins per bank, plus information which bank
provides IRQ support, along with the mux value to use for that.
The code will then iterate over all children of the pincontroller DT
node (which describe each pin group), and populate that struct with the
mapping between function names and mux values. The only thing that needs
adding in the DT is a property with that value, per pin group.

When this table is built, it will be handed over to the existing sunxi
pinctrl driver, which cannot tell a difference between a hardcoded
struct and this new one built at runtime. It will take care of
registering the pinctrl device with the pinctrl subsystem.

All a new SoC driver would need to do is to provide two arrays, and then
call the sunxi_pinctrl_dt_table_init() function.

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

diff --git a/drivers/pinctrl/sunxi/Makefile b/drivers/pinctrl/sunxi/Makefile
index 2ff5a55927ad..f5bad7a52951 100644
--- a/drivers/pinctrl/sunxi/Makefile
+++ b/drivers/pinctrl/sunxi/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 # Core
 obj-y					+= pinctrl-sunxi.o
+obj-y					+= pinctrl-sunxi-dt.o
 
 # SoC Drivers
 obj-$(CONFIG_PINCTRL_SUNIV_F1C100S)	+= pinctrl-suniv-f1c100s.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..a0e1e0ba5447
--- /dev/null
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi-dt.c
@@ -0,0 +1,355 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2017-2022 ARM Ltd.
+ *
+ * Generic DT driven Allwinner pinctrl driver routines.
+ * Builds the pin tables from minimal driver information and pin groups
+ * described in the DT. Then hands those tables of to the traditional
+ * sunxi pinctrl driver.
+ */
+
+#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 one value in the property.
+ */
+static u8 sunxi_pinctrl_dt_read_pinmux(const struct device_node *node,
+				       int index)
+{
+	int ret, num_elems;
+	u32 value;
+
+	num_elems = of_property_count_u32_elems(node, "pinmux");
+	if (num_elems <= 0)
+		return INVALID_MUX;
+
+	if (index >= num_elems)
+		index = num_elems - 1;
+
+	ret = of_property_read_u32_index(node, "pinmux", index, &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 pins array. We cannot use the member in *desc yet, as this
+ * is marked as const, and we will need to change the array still.
+ */
+static struct sunxi_desc_pin *init_pins_table(struct device *dev,
+					      const u8 *pins_per_bank,
+					      struct sunxi_pinctrl_desc *desc)
+{
+	struct sunxi_desc_pin *pins, *cur_pin;
+	int name_size = 0;
+	int port_base = desc->pin_base / PINS_PER_BANK;
+	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.
+	 */
+	for (i = 0; i < SUNXI_PINCTRL_MAX_BANKS; i++) {
+		desc->npins += pins_per_bank[i];
+		if (pins_per_bank[i] < 10) {
+			/* 4 bytes for "PXy\0" */
+			name_size += pins_per_bank[i] * 4;
+		} else {
+			/* 4 bytes for each "PXy\0" */
+			name_size += 10 * 4;
+
+			/* 5 bytes for each "PXyy\0" */
+			name_size += (pins_per_bank[i] - 10) * 5;
+		}
+	}
+
+	if (desc->npins == 0) {
+		dev_err(dev, "no ports defined\n");
+		return ERR_PTR(-EINVAL);
+	}
+
+	pins = devm_kzalloc(dev, desc->npins * sizeof(*pins), GFP_KERNEL);
+	if (!pins)
+		return ERR_PTR(-ENOMEM);
+
+	/* Allocate memory to store the name for every pin. */
+	pin_names = devm_kmalloc(dev, name_size, GFP_KERNEL);
+	if (!pin_names)
+		return ERR_PTR(-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 < SUNXI_PINCTRL_MAX_BANKS; i++) {
+		for (j = 0; j < pins_per_bank[i]; j++, cur_pin++) {
+			int nchars = sprintf(cur_name, "P%c%d",
+					     port_base + 'A' + i, j);
+
+			cur_pin->pin.number = (port_base + i) * PINS_PER_BANK + j;
+			cur_pin->pin.name = cur_name;
+			cur_name += nchars + 1;
+		}
+	}
+
+	return pins;
+}
+
+/*
+ * Work out the number of functions for each pin. This will visit every
+ * child node of the pinctrl DT node to find all advertised functions.
+ * Provide memory to hold the per-function information and assign it to
+ * the pin table.
+ * Fill in the GPIO in/out functions already (that every pin has), also add
+ * an "irq" function at the end, for those pins in IRQ-capable ports.
+ * We do not fill in the extra functions (those describe in DT nodes) yet.
+ * We (ab)use the "variant" member in each pin to keep track of the number of
+ * extra functions needed. At the end this will get reset to 2, so that we
+ * can add extra function later, after the two GPIO functions.
+ */
+static int prepare_function_table(struct device *dev, struct device_node *pnode,
+				  struct sunxi_desc_pin *pins, int npins,
+				  const u8 *irq_bank_muxes)
+{
+	struct device_node *node;
+	struct property *prop;
+	struct sunxi_desc_function *func;
+	int num_funcs, irq_bank, last_bank, 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 bank supporting interrupts.
+	 * We temporarily (ab)use the variant field to store the number of
+	 * functions per pin. This will be cleaned back to 0 before we hand
+	 * over the whole structure to the generic sunxi pinctrl setup code.
+	 */
+	for (i = 0; i < npins; i++) {
+		struct sunxi_desc_pin *pin = &pins[i];
+		int bank = pin->pin.number / PINS_PER_BANK;
+
+		if (irq_bank_muxes[bank]) {
+			pin->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 function's memory and fill in GPIOs, IRQ and a sentinel.
+	 * The extra functions will be filled in later.
+	 */
+	irq_bank = 0;
+	last_bank = 0;
+	for (i = 0; i < npins; i++) {
+		struct sunxi_desc_pin *pin = &pins[i];
+		int bank = pin->pin.number / PINS_PER_BANK;
+		int lastfunc = pin->variant + 1;
+		int irq_mux = irq_bank_muxes[bank];
+
+		func[0].name = "gpio_in";
+		func[0].muxval = 0;
+		func[1].name = "gpio_out";
+		func[1].muxval = 1;
+
+		if (irq_mux) {
+			if (bank > last_bank)
+				irq_bank++;
+			func[lastfunc].muxval = irq_mux;
+			func[lastfunc].irqbank = irq_bank;
+			func[lastfunc].irqnum = pin->pin.number % PINS_PER_BANK;
+			func[lastfunc].name = "irq";
+		}
+
+		if (bank > last_bank)
+			last_bank = bank;
+
+		pin->functions = func;
+
+		/* Skip over the other needed functions and the sentinel. */
+		func += pin->variant + 3;
+
+		/*
+		 * Reset the value for filling in the remaining functions
+		 * behind the GPIOs later.
+		 */
+		pin->variant = 2;
+	}
+
+	return 0;
+}
+
+/*
+ * Iterate over all pins in a single group and add the function name and its
+ * mux value to the respective pin.
+ * The "variant" member is again used to temporarily track the number of
+ * already added functions.
+ */
+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, index;
+	u8 muxval;
+
+	if (of_property_read_string(node, "function", &funcname)) {
+		dev_warn(dev, "missing \"function\" property\n");
+		return;
+	}
+
+	index = 0;
+	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);
+			index++;
+			continue;
+		}
+
+		/* Read the associated mux value. */
+		muxval = sunxi_pinctrl_dt_read_pinmux(node, index);
+		if (muxval == INVALID_MUX) {
+			dev_warn(dev, "%s: invalid mux value for pin %s\n",
+				 of_node_full_name(node), name);
+			index++;
+			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) {
+			index++;
+			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++;
+		index++;
+	}
+}
+
+/*
+ * Initialise the pinctrl table, by building it from driver provided
+ * information: the number of pins per bank, the IRQ capable banks and their
+ * IRQ mux value.
+ * Then iterate over all pinctrl DT node children to enter the function name
+ * and mux values for each mentioned pin.
+ * At the end hand over this structure to the actual sunxi pinctrl driver.
+ */
+int sunxi_pinctrl_dt_table_init(struct platform_device *pdev,
+				const u8 *pins_per_bank,
+				const u8 *irq_bank_muxes,
+				struct sunxi_pinctrl_desc *desc)
+{
+	struct device_node *pnode = pdev->dev.of_node, *node;
+	struct sunxi_desc_pin *pins;
+	int ret, i;
+
+	pins = init_pins_table(&pdev->dev, pins_per_bank, desc);
+	if (IS_ERR(pins))
+		return PTR_ERR(pins);
+
+	ret = prepare_function_table(&pdev->dev, pnode, pins, desc->npins,
+				     irq_bank_muxes);
+	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, desc->npins);
+
+	/* Clear the temporary storage. */
+	for (i = 0; i < desc->npins; i++)
+		pins[i].variant = 0;
+
+	desc->pins = pins;
+
+	return sunxi_pinctrl_init_with_variant(pdev, desc, 0);
+}
diff --git a/drivers/pinctrl/sunxi/pinctrl-sunxi.h b/drivers/pinctrl/sunxi/pinctrl-sunxi.h
index a87a2f944d60..2305d05c70f8 100644
--- a/drivers/pinctrl/sunxi/pinctrl-sunxi.h
+++ b/drivers/pinctrl/sunxi/pinctrl-sunxi.h
@@ -29,6 +29,9 @@
 #define PM_BASE	384
 #define PN_BASE	416
 
+/* maximum number of banks per controller (PA -> PK) */
+#define SUNXI_PINCTRL_MAX_BANKS	11
+
 #define SUNXI_PINCTRL_PIN(bank, pin)		\
 	PINCTRL_PIN(P ## bank ## _BASE + (pin), "P" #bank #pin)
 
@@ -306,4 +309,9 @@ int sunxi_pinctrl_init_with_variant(struct platform_device *pdev,
 #define sunxi_pinctrl_init(_dev, _desc) \
 	sunxi_pinctrl_init_with_variant(_dev, _desc, 0)
 
+int sunxi_pinctrl_dt_table_init(struct platform_device *pdev,
+				const u8 *pins_per_bank,
+				const u8 *irq_bank_muxes,
+				struct sunxi_pinctrl_desc *desc);
+
 #endif /* __PINCTRL_SUNXI_H */
-- 
2.35.5

[RFC PATCH 2/2] pinctrl: sunxi: Add support for the Allwinner V5 pin controller

[Andre Przywara]

The Allwinner V5 contains pins in all 10 possible pin banks.
Use the newly introduced DT based pinctrl driver to describe just the
generic pinctrl properties, so advertise the number of pins per bank
and the interrupt capabilities. The actual function/mux assignment is
taken from the devicetree.

Signed-off-by: Andre Przywara <andre.przywara@arm.com>
---
 drivers/pinctrl/sunxi/Kconfig            |  5 +++
 drivers/pinctrl/sunxi/Makefile           |  1 +
 drivers/pinctrl/sunxi/pinctrl-sun8i-v5.c | 52 ++++++++++++++++++++++++
 3 files changed, 58 insertions(+)
 create mode 100644 drivers/pinctrl/sunxi/pinctrl-sun8i-v5.c

diff --git a/drivers/pinctrl/sunxi/Kconfig b/drivers/pinctrl/sunxi/Kconfig
index a78fdbbdfc0c..6b8ea56c08fd 100644
--- a/drivers/pinctrl/sunxi/Kconfig
+++ b/drivers/pinctrl/sunxi/Kconfig
@@ -131,4 +131,9 @@ config PINCTRL_SUN50I_H616_R
 	default ARM64 && ARCH_SUNXI
 	select PINCTRL_SUNXI
 
+config PINCTRL_SUN8I_V5
+	bool "Support for the Allwinner V5 PIO"
+	default MACH_SUN8I
+	select PINCTRL_SUNXI
+
 endif
diff --git a/drivers/pinctrl/sunxi/Makefile b/drivers/pinctrl/sunxi/Makefile
index f5bad7a52951..5b289e18bd5a 100644
--- a/drivers/pinctrl/sunxi/Makefile
+++ b/drivers/pinctrl/sunxi/Makefile
@@ -21,6 +21,7 @@ obj-$(CONFIG_PINCTRL_SUN8I_A83T_R)	+= pinctrl-sun8i-a83t-r.o
 obj-$(CONFIG_PINCTRL_SUN8I_H3)		+= pinctrl-sun8i-h3.o
 obj-$(CONFIG_PINCTRL_SUN8I_H3_R)	+= pinctrl-sun8i-h3-r.o
 obj-$(CONFIG_PINCTRL_SUN8I_V3S)		+= pinctrl-sun8i-v3s.o
+obj-$(CONFIG_PINCTRL_SUN8I_V5)		+= pinctrl-sun8i-v5.o
 obj-$(CONFIG_PINCTRL_SUN20I_D1)		+= pinctrl-sun20i-d1.o
 obj-$(CONFIG_PINCTRL_SUN50I_H5)		+= pinctrl-sun50i-h5.o
 obj-$(CONFIG_PINCTRL_SUN50I_H6)		+= pinctrl-sun50i-h6.o
diff --git a/drivers/pinctrl/sunxi/pinctrl-sun8i-v5.c b/drivers/pinctrl/sunxi/pinctrl-sun8i-v5.c
new file mode 100644
index 000000000000..402b1c915e04
--- /dev/null
+++ b/drivers/pinctrl/sunxi/pinctrl-sun8i-v5.c
@@ -0,0 +1,52 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Allwinner V5 SoC pinctrl driver.
+ *
+ * Copyright (C) 2022 Arm Ltd.
+ */
+
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/pinctrl/pinctrl.h>
+
+#include "pinctrl-sunxi.h"
+
+static const unsigned int v5_irq_bank_map[] = { 0, 1, 5, 6, 7 };
+
+static const u8 v5_irq_bank_muxes[SUNXI_PINCTRL_MAX_BANKS] =
+/*       PA PB PC PD PE PF PG PH */
+        { 6, 6, 0, 0, 0, 6, 6, 6 };
+
+static const u8 v5_nr_bank_pins[SUNXI_PINCTRL_MAX_BANKS] =
+/*        PA  PB  PC  PD  PE  PF  PG  PH  PI  PJ */
+        { 16, 11, 17, 23, 18,  7, 14, 16, 17, 18 };
+
+static struct sunxi_pinctrl_desc v5_pinctrl_data = {
+	.irq_banks = ARRAY_SIZE(v5_irq_bank_map),
+	.irq_bank_map = v5_irq_bank_map,
+	.irq_read_needs_mux = true,
+	.io_bias_cfg_variant = BIAS_VOLTAGE_PIO_POW_MODE_SEL,
+};
+
+static int v5_pinctrl_probe(struct platform_device *pdev)
+{
+	return sunxi_pinctrl_dt_table_init(pdev, v5_nr_bank_pins,
+					   v5_irq_bank_muxes,
+					   &v5_pinctrl_data);
+}
+
+static const struct of_device_id v5_pinctrl_match[] = {
+	{ .compatible = "allwinner,sun8i-v5-pinctrl", },
+	{}
+};
+
+static struct platform_driver v5_pinctrl_driver = {
+	.probe	= v5_pinctrl_probe,
+	.driver	= {
+		.name		= "sun8i-v5-pinctrl",
+		.of_match_table	= v5_pinctrl_match,
+	},
+};
+builtin_platform_driver(v5_pinctrl_driver);
-- 
2.35.5

Re: [RFC PATCH 0/2] pinctrl: sunxi: Introduce DT-based pinctrl builder

[Linus Walleij]

On Thu, Nov 10, 2022 at 2:44 AM Andre Przywara <andre.przywara@arm.com> wrote:

> Compared to my previous effort almost exactly five years ago [1], this
> new version drops the idea of describing the pinctrl data entirely in
> the DT, instead it still relies on driver provided information for that.
(...)
> On the DT side all that would be needed is *one* extra property per
> pin group to announce the mux value:
>
>         uart0_pb_pins: uart0-pb-pins {
>                 pins = "PB9", "PB10";
>                 function = "uart0";
>                 pinmux = <2>;
>         };

So what you need to do is to convince the device tree people that this
is a good idea.

For me as linux maintainer it's no big deal, it's fine either way. The new
code looks elegant.

But from a DT point of view this needs to make sense also for Windows
and BSD, so that is who you have to convince. If it is possible to derive
the same information from the compatible string (like today) that will
need an extended argument why all operating systems will benefit from
this.

Yours,
Linus Walleij

Re: [RFC PATCH 0/2] pinctrl: sunxi: Introduce DT-based pinctrl builder

[Andre Przywara]

On Thu, 10 Nov 2022 11:21:02 +0100
Linus Walleij <linus.walleij@linaro.org> wrote:

Hi Linus,

thanks for having a look!

> On Thu, Nov 10, 2022 at 2:44 AM Andre Przywara <andre.przywara@arm.com> wrote:
> 
> > Compared to my previous effort almost exactly five years ago [1], this
> > new version drops the idea of describing the pinctrl data entirely in
> > the DT, instead it still relies on driver provided information for that.  
> (...)
> > On the DT side all that would be needed is *one* extra property per
> > pin group to announce the mux value:
> >
> >         uart0_pb_pins: uart0-pb-pins {
> >                 pins = "PB9", "PB10";
> >                 function = "uart0";
> >                 pinmux = <2>;
> >         };  
> 
> So what you need to do is to convince the device tree people that this
> is a good idea.
> 
> For me as linux maintainer it's no big deal, it's fine either way. The new
> code looks elegant.
> 
> But from a DT point of view this needs to make sense also for Windows
> and BSD, so that is who you have to convince. If it is possible to derive
> the same information from the compatible string (like today) that will
> need an extended argument why all operating systems will benefit from
> this.

This is actually an argument in favour of it: at the moment *every* OS
(or DT user) has to carry some form of this table[1]. For U-Boot this is a
major pain, for instance, and we came up with some minimal and
simplified version of that (assuming one pinmux per function name,
ignoring different mappings in different ports: [2]), but we are already
touching its limits.
And I don't think this DT argument counts anyway: we already store a much
bigger chunk of "information" in the DT, namely the function name. This has
no technical meaning, really, other than to map this to a 4-bit value
internally. I don't know why we have an information like "UART0 is using
the 'uart0' pin group" in the DT, but refuse to put the actual
hardware information in there. We could possibly even get rid of the
string, and derive this from the node name, if we need some human readable
identifier.

And just to make sure: I don't propose to change this for existing DTs,
it's just for new SoCs going forward. Allwinner at the moment spins out
many SoCs with only little differences, but all require this largish
table, since the pin assignments are the ones that differ.

Cheers,
Andre

[1]
https://github.com/freebsd/freebsd-src/blob/main/sys/arm/allwinner/a64/a64_padconf.c
[2]
https://source.denx.de/u-boot/u-boot/-/blob/master/drivers/pinctrl/sunxi/pinctrl-sunxi.c#L587-605

Re: [RFC PATCH 0/2] pinctrl: sunxi: Introduce DT-based pinctrl builder

[Linus Walleij]

On Thu, Nov 10, 2022 at 12:33 PM Andre Przywara <andre.przywara@arm.com> wrote:

> This is actually an argument in favour of it: at the moment *every* OS
> (or DT user) has to carry some form of this table[1]. For U-Boot this is a
> major pain, for instance, and we came up with some minimal and
> simplified version of that (assuming one pinmux per function name,
> ignoring different mappings in different ports: [2]), but we are already
> touching its limits.

That's a compelling argument.

I don't know about reusing the existing pinmux property in a new way
which differs from the standard one:

  pinmux:
    description:
      The list of numeric pin ids and their mux settings that properties in the
      node apply to (either this, "pins" or "groups" have to be specified)
    $ref: /schemas/types.yaml#/definitions/uint32-array

You should rather invent something like "sunxi,pin-mux-val" or so.

That makes me happy to merge it at least, I don't see any problem
with it.

> And I don't think this DT argument counts anyway: we already store a much
> bigger chunk of "information" in the DT, namely the function name. This has
> no technical meaning, really, other than to map this to a 4-bit value
> internally. I don't know why we have an information like "UART0 is using
> the 'uart0' pin group" in the DT, but refuse to put the actual
> hardware information in there. We could possibly even get rid of the
> string, and derive this from the node name, if we need some human readable
> identifier.

These exist for consistency and maintenance, they are established
standard bindings:
Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml

Associating a function "uart0" with something like
[ "uart0-txrx", uart0-rtscts" ] by a line such as:

groups = "uart0-txrx", "uart0-rtscts";

is simple to understand, and makes it easier for maintainers who have
to look at a lot of different platforms with different muxes. So these
are there for human readability.

I.e. the goal of standard properties is not to minimize amount of
information (which is your goal here) but to structure things in a way that
makes maintenance easier by being similar on several platforms.

Some systems deviate from standard properties, and admittedly
this way of structuring things with strings is maybe not the ultimate.
The most deviating one is:
Documentation/devicetree/bindings/pinctrl/pinctrl-single.txt
which is used by OMAP and HiSilicon.
Some feel this should never have been merged.
But it was merged.
By me.

There is also the pinmux property above which some systems use for
putting in enumerated magic. That's part of the standard but
makes the standard somewhat incoherent.

This difference in pin control DT patterns is because we could
not come up with something that was acceptable for all and the
result of some diplomacy when the subsystem was created around
2011-2012.

The thing is that I am not a very consistent and stubborn person
and I think more along the lines of the IETF motto "rough consensus
and running code". The DT people were different back in 2011,
and also softer around the edges, not as strict and not insisting on
things being done one way for all systems. Their ambitions have
stepped up since. So these are the people you need to convince.

I suggest you propose the bindings with the patch set by Cc
devicetree@vger.kernel.org using the custom "sunxi,pin" or whatever
name you want and see if the DT reviewers agree with this solution.

If they say nothing in like 2 weeks I usually merge things that I
find non-objectionable anyways.

Yours,
Linus Walleij