[RFC 0/5] microchip mpfs/pic64gx pinctrl questions

[RFC 0/5] microchip mpfs/pic64gx pinctrl questions

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

Hey Linus, or whoever else,

Working on some pinctrl drivers for my devices, and I had two questions,
so I am sending this as an RFC in the hopes of getting an answer before
progressing further with the third pin controller on the platform.

Firstly, both of the drivers I have produced so far have only pinmuxing
functions and no pincfg role - they only determine internal routing in
the SoC. I've got an identical dt_node_to_map implementation in both
drivers, as all they are doing is populating the pin and mux bit setting
from dt. I used what the recently added spacemit k1 was doing as a
guide, but removed the loop since there's no pincfg stuff that can
differ between pins. I notice there are generic implementations of
dt_node_to_map but I didn't get them to work. Have I missed a trick here
(either on the dt side, or in the driver) that would let me use a generic
function instead of having my own implementation, either in the driver
or in how I've set up the dt side?
I got to this point by partially writing the third driver first, based
on that spacemit k1 driver's approach, so I might've blinded myself to
the correct/simple approach to things as a result of having to handle
the pincfg stuff etc in dt_node_to_map there.

Secondly, particularly if there's not some neat way to simplify things
in dt_node_to_map, should I merge the two drivers, at least partially?
They're effectively only different in their pinctrl_pin_desc and how the
regmap is generated in probe (from a syscon regmap versus
regmap_init_mmio). I've kept the bindings apart, as despite similarity,
they're really only similar due to simplicity.

None of this is in any sort of final state, it's either WIP in and off
itself or depends on other stuff that's not yet accepted, so please
excuse any build warnings etc.

Cheers,
Conor.

CC: Linus Walleij <linus.walleij@linaro.org>
CC: Rob Herring <robh@kernel.org>
CC: Krzysztof Kozlowski <krzk+dt@kernel.org>
CC: linux-kernel@vger.kernel.org
CC: linux-gpio@vger.kernel.org
CC: devicetree@vger.kernel.org

Conor Dooley (5):
  dt-bindings: pinctrl: add polarfire soc iomux0 pinmux
  dt-bindings: pinctrl: add pic64gx "gpio2" pinmux
  pinctrl: add polarfire soc iomux0 pinmux driver
  pinctrl: add pic64gx "gpio2" pinmux driver
  riscv: dts: microchip: add pinctrl nodes for iomux0

 .../microchip,mpfs-pinctrl-iomux0.yaml        |  77 +++++
 .../microchip,pic64gx-pinctrl-gpio2.yaml      |  74 +++++
 .../microchip,mpfs-mss-top-sysreg.yaml        |  17 +-
 .../dts/microchip/mpfs-icicle-kit-fabric.dtsi |  56 ++++
 .../boot/dts/microchip/mpfs-icicle-kit.dts    |   1 -
 .../boot/dts/microchip/mpfs-pinctrl.dtsi      | 117 ++++++++
 arch/riscv/boot/dts/microchip/mpfs.dtsi       |   9 +
 drivers/pinctrl/Kconfig                       |  14 +
 drivers/pinctrl/Makefile                      |   2 +
 drivers/pinctrl/pinctrl-mpfs-iomux0.c         | 252 ++++++++++++++++
 drivers/pinctrl/pinctrl-pic64gx-gpio2.c       | 283 ++++++++++++++++++
 11 files changed, 900 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,mpfs-pinctrl-iomux0.yaml
 create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,pic64gx-pinctrl-gpio2.yaml
 create mode 100644 arch/riscv/boot/dts/microchip/mpfs-pinctrl.dtsi
 create mode 100644 drivers/pinctrl/pinctrl-mpfs-iomux0.c
 create mode 100644 drivers/pinctrl/pinctrl-pic64gx-gpio2.c

-- 
2.47.3

[RFC 1/5] dt-bindings: pinctrl: add polarfire soc iomux0 pinmux

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

On Polarfire SoC, iomux0 is responsible for routing functions to either
MSS (multiprocessor subsystem) IOs or to the FPGA fabric, where they
can either interface with custom RTL or be routed to the FPGA fabric's
IOs. Document it.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../microchip,mpfs-pinctrl-iomux0.yaml        | 77 +++++++++++++++++++
 .../microchip,mpfs-mss-top-sysreg.yaml        | 15 ++++
 2 files changed, 92 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,mpfs-pinctrl-iomux0.yaml

diff --git a/Documentation/devicetree/bindings/pinctrl/microchip,mpfs-pinctrl-iomux0.yaml b/Documentation/devicetree/bindings/pinctrl/microchip,mpfs-pinctrl-iomux0.yaml
new file mode 100644
index 000000000000..779348304956
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/microchip,mpfs-pinctrl-iomux0.yaml
@@ -0,0 +1,77 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/microchip,mpfs-pinctrl-iomux0.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip PolarFire SoC iomux0
+
+maintainers:
+  - Conor Dooley <conor.dooley@microchip.com>
+
+description:
+  iomux0 is responsible for routing some functions to either the FPGA fabric,
+  or to MSSIOs. It only performs muxing, and has no IO configuration role, as
+  fabric IOs are configured separately and just routing a function to MSSIOs is
+  not sufficient for it to actually get mapped to an MSSIO, just makes it
+  possible.
+
+properties:
+  compatible:
+    oneOf:
+      - const: microchip,mpfs-pinctrl-iomux0
+      - items:
+          - const: microchip,pic64gx-pinctrl-iomux0
+          - const: microchip,mpfs-pinctrl-iomux0
+
+  reg:
+    maxItems: 1
+
+  pinctrl-use-default: true
+
+patternProperties:
+  '-pins$':
+    type: object
+    additionalProperties: false
+
+    allOf:
+      - $ref: pinmux-node.yaml#
+
+    properties:
+      pinmux:
+        description: |
+          The list of GPIOs and their mux settings that properties in the
+          node apply to. The upper 16 bits of the value represent the function
+          and the lower 16 bits where it is routed - 0 is to an MSSIO and 1 to
+          the fabric. Which bit controls which function is described in the
+          register map in section MSS/pfsoc_mss_top_sysreg.htm#IOMUX0_CR.
+
+    required:
+      - pinmux
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #define MPFS_PINFUNC(pin, func) (((pin) << 16) | (func))
+
+    soc {
+      pinctrl@200 {
+        compatible = "microchip,mpfs-pinctrl-iomux0";
+        reg = <0x200 0x4>;
+
+        spi0_mssio: spi0-mssio-pins {
+          pinmux = <MPFS_PINFUNC(0, 0)>;
+        };
+
+        spi0_fabric: spi0-fabric-pins {
+          pinmux = <MPFS_PINFUNC(0, 1)>;
+        };
+      };
+    };
+
+...
diff --git a/Documentation/devicetree/bindings/soc/microchip/microchip,mpfs-mss-top-sysreg.yaml b/Documentation/devicetree/bindings/soc/microchip/microchip,mpfs-mss-top-sysreg.yaml
index 1ab691db8795..1b737a3fcd33 100644
--- a/Documentation/devicetree/bindings/soc/microchip/microchip,mpfs-mss-top-sysreg.yaml
+++ b/Documentation/devicetree/bindings/soc/microchip/microchip,mpfs-mss-top-sysreg.yaml
@@ -18,10 +18,17 @@ properties:
     items:
       - const: microchip,mpfs-mss-top-sysreg
       - const: syscon
+      - const: simple-mfd
 
   reg:
     maxItems: 1
 
+  '#address-cells':
+    const: 1
+
+  '#size-cells':
+    const: 1
+
   '#reset-cells':
     description:
       The AHB/AXI peripherals on the PolarFire SoC have reset support, so
@@ -31,6 +38,14 @@ properties:
       of PolarFire clock/reset IDs.
     const: 1
 
+  pinctrl@200:
+    type: object
+    $ref: /schemas/pinctrl/microchip,mpfs-pinctrl-iomux0.yaml
+
+  pinctrl@204:
+    type: object
+    $ref: /schemas/pinctrl/microchip,mpfs-pinctrl-iomux0.yaml
+
 required:
   - compatible
   - reg
-- 
2.47.3

[RFC 2/5] dt-bindings: pinctrl: add pic64gx "gpio2" pinmux

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

The pic64gx has a second pinmux "downstream" of the iomux0 pinmux. The
documentation for the SoC provides no name for this device, but it is
used to swap pins between either GPIO controller #2 or select other
functions, hence the "gpio2" name. Currently there is no documentation
about what each bit actually does that is publicly available, nor (I
believe) what pins are affected. That info is as follows:

pin     role (1/0)
---     ----------
E14	MAC_0_MDC/GPIO_2_0
E15	MAC_0_MDIO/GPIO_2_1
F16	MAC_1_MDC/GPIO_2_2
F17	MAC_1_MDIO/GPIO_2_3
D19	SPI_0_CLK/GPIO_2_4
B18	SPI_0_SS0/GPIO_2_5
B10	CAN_0_RXBUS/GPIO_2_6
C14	PCIE_PERST_2#/GPIO_2_7
E18	PCIE_WAKE#/GPIO_2_8
D18	PCIE_PERST_1#/GPIO_2_9
E19	SPI_0_DO/GPIO_2_10
C7	SPI_0_DI/GPIO_2_11
D6	QSPI_SS0/GPIO_2_12
D7	QSPI_CLK (B)/GPIO_2_13
C9	QSPI_DATA0/GPIO_2_14
C10	QSPI_DATA1/GPIO_2_15
A5	QSPI_DATA2/GPIO_2_16
A6	QSPI_DATA3/GPIO_2_17
D8	MMUART_3_RXD/GPIO_2_18
D9	MMUART_3_TXD/GPIO_2_19
B8	MMUART_4_RXD/GPIO_2_20
A8	MMUART_4_TXD/GPIO_2_21
C12	CAN_1_TXBUS/GPIO_2_22
B12	CAN_1_RXBUS/GPIO_2_23
A11	CAN_0_TX_EBL_N/GPIO_2_24
A10	CAN_1_TX_EBL_N/GPIO_2_25
D11	MMUART_2_RXD/GPIO_2_26
C11	MMUART_2_TXD/GPIO_2_27
B9	CAN_0_TXBUS/GPIO_2_28

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../microchip,pic64gx-pinctrl-gpio2.yaml      | 74 +++++++++++++++++++
 1 file changed, 74 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,pic64gx-pinctrl-gpio2.yaml

diff --git a/Documentation/devicetree/bindings/pinctrl/microchip,pic64gx-pinctrl-gpio2.yaml b/Documentation/devicetree/bindings/pinctrl/microchip,pic64gx-pinctrl-gpio2.yaml
new file mode 100644
index 000000000000..be7d4b1948dc
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/microchip,pic64gx-pinctrl-gpio2.yaml
@@ -0,0 +1,74 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/microchip,pic64gx-pinctrl-gpio2.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip PIC64GX GPIO2 Mux
+
+maintainers:
+  - Conor Dooley <conor.dooley@microchip.com>
+
+description:
+  The "GPIO2 Mux" determines whether GPIO2 or select other functions are
+  available on package pins on PIC64GX. Some of these functions must be
+  mapped to this mux 
+
+properties:
+  compatible:
+     const: microchip,pic64gx-pinctrl-gpio2
+
+  reg:
+    maxItems: 1
+
+  pinctrl-use-default: true
+
+patternProperties:
+  '-pins$':
+    type: object
+    additionalProperties: false
+
+    allOf:
+      - $ref: pinmux-node.yaml#
+
+    properties:
+      pinmux:
+        description: |
+          The list of GPIOs and their mux settings that properties in the
+          node apply to. The upper 16 bits of the value represent the pin
+          and the lower 16 bits determine which function is routed there.
+
+    required:
+      - pinmux
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    #define PIC64GX_PINFUNC(pin, func) (((pin) << 16) | (func))
+
+    soc {
+      pinctrl@f00 {
+      compatible = "microchip,pic64gx-pinctrl-gpio2";
+      reg = <0xf00 0x4>;
+      pinctrl-use-default;
+      pinctrl-names = "default";
+      pinctrl-0 = <&mdio0_gpio2>, <&mdio1_gpio2>, <&spi0_gpio2>, <&qspi_gpio2>,
+            <&mmuart3_gpio2>, <&mmuart4_gpio2>, <&can1_gpio2>, <&can0_gpio2>,
+            <&mmuart2_gpio2>;
+
+        mdio0_gpio: gpio0-gpio-pins {
+          pinmux = <PIC64GX_PINFUNC(0, 0)>, <PIC64GX_PINFUNC(1, 0)>;
+        };
+
+        mdio0_mdio: mdio0-mdio-pins {
+          pinmux = <PIC64GX_PINFUNC(0, 1)>, <PIC64GX_PINFUNC(1, 1)>;
+        };
+      };
+    };
+
+...
-- 
2.47.3

[RFC 3/5] pinctrl: add polarfire soc iomux0 pinmux driver

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

On Polarfire SoC, iomux0 is responsible for routing functions to either
MSS (multiprocessor subsystem) IOs or to the FPGA fabric, where they
can either interface with custom RTL or be routed to the FPGA fabric's
IOs. Add a driver for it.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../microchip,mpfs-mss-top-sysreg.yaml        |   2 +-
 drivers/pinctrl/Kconfig                       |   7 +
 drivers/pinctrl/Makefile                      |   1 +
 drivers/pinctrl/pinctrl-mpfs-iomux0.c         | 252 ++++++++++++++++++
 4 files changed, 261 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pinctrl/pinctrl-mpfs-iomux0.c

diff --git a/Documentation/devicetree/bindings/soc/microchip/microchip,mpfs-mss-top-sysreg.yaml b/Documentation/devicetree/bindings/soc/microchip/microchip,mpfs-mss-top-sysreg.yaml
index 1b737a3fcd33..cb5784ec5ac5 100644
--- a/Documentation/devicetree/bindings/soc/microchip/microchip,mpfs-mss-top-sysreg.yaml
+++ b/Documentation/devicetree/bindings/soc/microchip/microchip,mpfs-mss-top-sysreg.yaml
@@ -55,7 +55,7 @@ additionalProperties: false
 examples:
   - |
     syscon@20002000 {
-      compatible = "microchip,mpfs-mss-top-sysreg", "syscon";
+      compatible = "microchip,mpfs-mss-top-sysreg", "syscon", "simple-mfd";
       reg = <0x20002000 0x1000>;
       #reset-cells = <1>;
     };
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 33db9104df17..f85ccbc2a0e2 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -472,6 +472,13 @@ config PINCTRL_PISTACHIO
 	help
 	  This support pinctrl and GPIO driver for IMG Pistachio SoC.
 
+config PINCTRL_POLARFIRE_SOC
+	bool "Polarfire SoC pinctrl driver"
+	depends on ARCH_MICROCHIP
+	default y
+	help
+	  This selects the pinctrl driver for Microchip Polarfire SoC.
+
 config PINCTRL_RK805
 	tristate "Pinctrl and GPIO driver for RK805 PMIC"
 	depends on MFD_RK8XX
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index ac27e88677d1..8af119804f77 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -47,6 +47,7 @@ obj-$(CONFIG_PINCTRL_PALMAS)	+= pinctrl-palmas.o
 obj-$(CONFIG_PINCTRL_PEF2256)	+= pinctrl-pef2256.o
 obj-$(CONFIG_PINCTRL_PIC32)	+= pinctrl-pic32.o
 obj-$(CONFIG_PINCTRL_PISTACHIO)	+= pinctrl-pistachio.o
+obj-$(CONFIG_PINCTRL_POLARFIRE_SOC)	+= pinctrl-mpfs-iomux0.o
 obj-$(CONFIG_PINCTRL_RK805)	+= pinctrl-rk805.o
 obj-$(CONFIG_PINCTRL_ROCKCHIP)	+= pinctrl-rockchip.o
 obj-$(CONFIG_PINCTRL_SCMI)	+= pinctrl-scmi.o
diff --git a/drivers/pinctrl/pinctrl-mpfs-iomux0.c b/drivers/pinctrl/pinctrl-mpfs-iomux0.c
new file mode 100644
index 000000000000..93a17c9c299d
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-mpfs-iomux0.c
@@ -0,0 +1,252 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bitfield.h>
+#include <linux/cleanup.h>
+#include <linux/module.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mod_devicetable.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/seq_file.h>
+
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+
+#include "core.h"
+#include "pinctrl-utils.h"
+#include "pinconf.h"
+#include "pinmux.h"
+
+#define MPFS_PINCTRL_DT_FUNC_MASK GENMASK(3, 0);
+#define MPFS_PINCTRL_DT_PIN_OFFSET 16
+
+#define MPFS_PINCTRL_IOMUX0_REG 0x200
+
+struct mpfs_iomux0_pinctrl_mux_config {
+	u8 pin;
+	u32 config;
+};
+
+struct mpfs_iomux0_pinctrl {
+	struct pinctrl_dev *pctrl;
+	struct device *dev;
+	struct regmap *regmap;
+	struct mutex mutex;
+	struct pinctrl_desc desc;
+};
+
+static void mpfs_iomux0_pinctrl_dbg_show(struct pinctrl_dev *pctrl_dev, struct seq_file *seq,
+				  unsigned int pin)
+{
+	struct mpfs_iomux0_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
+	u32 val;
+
+	seq_printf(seq, "reg: %x, pin: %u ", MPFS_PINCTRL_IOMUX0_REG, pin);
+
+	regmap_read(pctrl->regmap, MPFS_PINCTRL_IOMUX0_REG, &val);
+	val = (val & BIT(pin)) >> pin;
+
+	seq_printf(seq, "val: %x\n", val);
+}
+
+static int mpfs_iomux0_pinctrl_dt_node_to_map(struct pinctrl_dev *pctrl_dev, struct device_node *np,
+					 struct pinctrl_map **maps, unsigned int *num_maps)
+{
+	struct mpfs_iomux0_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
+	struct device *dev = pctrl->dev;
+	struct mpfs_iomux0_pinctrl_mux_config *pinmuxs;
+	struct pinctrl_map *map;
+	const char **grpnames;
+	const char *grpname;
+	int ret, i, npins;
+	unsigned int config, *pins;
+
+	map = kcalloc(1, sizeof(*map), GFP_KERNEL);
+	if (!map)
+		return -ENOMEM;
+
+	guard(mutex)(&pctrl->mutex);
+
+	npins = of_property_count_u32_elems(np, "pinmux");
+	if (npins < 1) {
+		dev_err(dev, "invalid pinctrl group %pOFn\n", np);
+		return -EINVAL;
+	}
+
+	grpnames = devm_kmalloc(dev, sizeof(*grpnames), GFP_KERNEL);
+	if (!grpnames)
+		return -ENOMEM;
+
+	grpname = devm_kasprintf(dev, GFP_KERNEL, "%pOFn", np);
+	if (!grpname)
+		return -ENOMEM;
+
+	*grpnames = grpname;
+
+	pins = devm_kcalloc(dev, npins, sizeof(*pins), GFP_KERNEL);
+	if (!pins)
+		return -ENOMEM;
+
+	pinmuxs = devm_kcalloc(dev, npins, sizeof(*pinmuxs), GFP_KERNEL);
+	if (!pinmuxs)
+		return -ENOMEM;
+
+	for (i = 0; i < npins; i++) {
+		ret = of_property_read_u32_index(np, "pinmux", i, &config);
+		if (ret)
+			return -EINVAL;
+
+		pins[i] = config >> MPFS_PINCTRL_DT_PIN_OFFSET;
+		pinmuxs[i].config = config & MPFS_PINCTRL_DT_FUNC_MASK;
+		pinmuxs[i].pin = config >> MPFS_PINCTRL_DT_PIN_OFFSET;
+	}
+
+	map->type = PIN_MAP_TYPE_MUX_GROUP;
+	map->data.mux.function = np->name;
+	map->data.mux.group = grpname;
+
+	ret = pinctrl_generic_add_group(pctrl_dev, grpname, pins, npins, pinmuxs);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to add group %s: %d\n", grpname, ret);
+
+	ret = pinmux_generic_add_function(pctrl_dev, np->name, grpnames, 1, NULL);
+	if (ret < 0) {
+		pinctrl_utils_free_map(pctrl_dev, map, 1);
+		return dev_err_probe(dev, ret, "error adding function %s\n", np->name);
+	}
+
+	*maps = map;
+	*num_maps = 1;
+
+	return 0;
+};
+
+static struct pinctrl_ops mpfs_iomux0_pinctrl_ops = {
+	.get_groups_count = pinctrl_generic_get_group_count,
+	.get_group_name	= pinctrl_generic_get_group_name,
+	.get_group_pins	= pinctrl_generic_get_group_pins,
+	.pin_dbg_show = mpfs_iomux0_pinctrl_dbg_show,
+	.dt_node_to_map = mpfs_iomux0_pinctrl_dt_node_to_map,
+	.dt_free_map = pinctrl_utils_free_map,
+};
+
+static int mpfs_iomux0_pinctrl_set_pin_func(struct mpfs_iomux0_pinctrl *pctrl, u8 pin, u32 config)
+{
+	struct device *dev = pctrl->dev;
+	u32 state;
+
+	state = config & MPFS_PINCTRL_DT_FUNC_MASK;
+	state <<= pin;
+
+	dev_dbg(dev, "Setting pin %u reg %x offset %u func %x\n", pin, MPFS_PINCTRL_IOMUX0_REG, pin, state);
+
+	regmap_set_bits(pctrl->regmap, MPFS_PINCTRL_IOMUX0_REG, state);
+
+	return 0;
+}
+
+static int mpfs_iomux0_pinctrl_set_mux(struct pinctrl_dev *pctrl_dev, unsigned int fsel, unsigned int gsel)
+{
+	struct mpfs_iomux0_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
+	const struct group_desc *group;
+	struct mpfs_iomux0_pinctrl_mux_config *configs;
+	int ret = -EINVAL;
+
+	group = pinctrl_generic_get_group(pctrl_dev, gsel);
+	if (!group)
+		return -EINVAL;
+
+	configs = group->data;
+
+	for (int i = 0; i < group->grp.npins; i++) {
+		u8 pin = configs[i].pin;
+		u32 config = configs[i].config;
+
+		ret = mpfs_iomux0_pinctrl_set_pin_func(pctrl, pin, config);
+	}
+
+	return ret;
+}
+
+static const struct pinmux_ops mpfs_iomux0_pinctrl_pinmux_ops = {
+	.get_functions_count = pinmux_generic_get_function_count,
+	.get_function_name = pinmux_generic_get_function_name,
+	.get_function_groups = pinmux_generic_get_function_groups,
+	.set_mux = mpfs_iomux0_pinctrl_set_mux,
+};
+
+static const struct pinctrl_pin_desc mpfs_iomux0_pinctrl_pins[] = {
+	PINCTRL_PIN(0, "spi0"),
+	PINCTRL_PIN(1, "spi1"),
+	PINCTRL_PIN(2, "i2c0"),
+	PINCTRL_PIN(3, "i2c1"),
+	PINCTRL_PIN(4, "can0"),
+	PINCTRL_PIN(5, "can1"),
+	PINCTRL_PIN(6, "qspi"),
+	PINCTRL_PIN(7, "uart0"),
+	PINCTRL_PIN(8, "uart1"),
+	PINCTRL_PIN(9, "uart2"),
+	PINCTRL_PIN(10, "uart3"),
+	PINCTRL_PIN(11, "uart4"),
+	PINCTRL_PIN(12, "mdio0"),
+	PINCTRL_PIN(13, "mdio1"),
+
+};
+
+static int mpfs_iomux0_pinctrl_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mpfs_iomux0_pinctrl *pctrl;
+	int ret;
+
+	pctrl = devm_kzalloc(dev, sizeof(*pctrl), GFP_KERNEL);
+	if (!pctrl)
+		return -ENOMEM;
+
+	pctrl->regmap = device_node_to_regmap(pdev->dev.parent->of_node);
+	if (IS_ERR(pctrl->regmap))
+		dev_err_probe(dev, PTR_ERR(pctrl->regmap), "Failed to find syscon regmap\n");
+
+	pctrl->desc.name = dev_name(dev);
+	pctrl->desc.pins = mpfs_iomux0_pinctrl_pins;
+	pctrl->desc.npins = ARRAY_SIZE(mpfs_iomux0_pinctrl_pins);
+	pctrl->desc.pctlops = &mpfs_iomux0_pinctrl_ops;
+	pctrl->desc.pmxops = &mpfs_iomux0_pinctrl_pinmux_ops;
+	pctrl->desc.owner = THIS_MODULE;
+
+	pctrl->dev = dev;
+
+	ret = devm_mutex_init(dev, &pctrl->mutex);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, pctrl);
+
+	pctrl->pctrl = devm_pinctrl_register(&pdev->dev, &pctrl->desc, pctrl);
+	if (IS_ERR(pctrl->pctrl))
+		return PTR_ERR(pctrl->pctrl);
+
+	return 0;
+}
+
+static const struct of_device_id mpfs_iomux0_pinctrl_of_match[] = {
+	{ .compatible = "microchip,mpfs-pinctrl-iomux0" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, mpfs_iomux0_pinctrl_of_match);
+
+static struct platform_driver mpfs_iomux0_pinctrl_driver = {
+	.driver = {
+		.name = "mpfs-pinctrl-iomux0",
+		.of_match_table = mpfs_iomux0_pinctrl_of_match,
+	},
+	.probe = mpfs_iomux0_pinctrl_probe,
+};
+module_platform_driver(mpfs_iomux0_pinctrl_driver);
+
+MODULE_AUTHOR("Conor Dooley <conor.dooley@microchip.com>");
+MODULE_DESCRIPTION("Polarfire SoC iomux0 pinctrl driver");
+MODULE_LICENSE("GPL");
-- 
2.47.3

[RFC 4/5] pinctrl: add pic64gx "gpio2" pinmux driver

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

The pic64gx has a second pinmux "downstream" of the iomux0 pinmux. The
documentation for the SoC provides no name for this device, but it is
used to swap pins between either GPIO controller #2 or select other
functions, hence the "gpio2" name. Add a driver for it.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../microchip,pic64gx-pinctrl-gpio2.yaml      |   2 +-
 drivers/pinctrl/Kconfig                       |   7 +
 drivers/pinctrl/Makefile                      |   1 +
 drivers/pinctrl/pinctrl-pic64gx-gpio2.c       | 283 ++++++++++++++++++
 4 files changed, 292 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pinctrl/pinctrl-pic64gx-gpio2.c

diff --git a/Documentation/devicetree/bindings/pinctrl/microchip,pic64gx-pinctrl-gpio2.yaml b/Documentation/devicetree/bindings/pinctrl/microchip,pic64gx-pinctrl-gpio2.yaml
index be7d4b1948dc..6af7b67731d6 100644
--- a/Documentation/devicetree/bindings/pinctrl/microchip,pic64gx-pinctrl-gpio2.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/microchip,pic64gx-pinctrl-gpio2.yaml
@@ -16,7 +16,7 @@ description:
 
 properties:
   compatible:
-     const: microchip,pic64gx-pinctrl-gpio2
+    const: microchip,pic64gx-pinctrl-gpio2
 
   reg:
     maxItems: 1
diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index f85ccbc2a0e2..692eb577ed74 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -461,6 +461,13 @@ config PINCTRL_PIC32MZDA
 	def_bool y if PIC32MZDA
 	select PINCTRL_PIC32
 
+config PINCTRL_PIC64GX
+	bool "pic64gx gpio2 pinctrl driver"
+	depends on ARCH_MICROCHIP
+	default y
+	help
+	  This selects the pinctrl driver for gpio2 on pic64gx.
+
 config PINCTRL_PISTACHIO
 	bool "IMG Pistachio SoC pinctrl driver"
 	depends on OF && (MIPS || COMPILE_TEST)
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index 8af119804f77..5493628a9a47 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -46,6 +46,7 @@ obj-$(CONFIG_PINCTRL_OCELOT)	+= pinctrl-ocelot.o
 obj-$(CONFIG_PINCTRL_PALMAS)	+= pinctrl-palmas.o
 obj-$(CONFIG_PINCTRL_PEF2256)	+= pinctrl-pef2256.o
 obj-$(CONFIG_PINCTRL_PIC32)	+= pinctrl-pic32.o
+obj-$(CONFIG_PINCTRL_PIC64GX)	+= pinctrl-pic64gx-gpio2.o
 obj-$(CONFIG_PINCTRL_PISTACHIO)	+= pinctrl-pistachio.o
 obj-$(CONFIG_PINCTRL_POLARFIRE_SOC)	+= pinctrl-mpfs-iomux0.o
 obj-$(CONFIG_PINCTRL_RK805)	+= pinctrl-rk805.o
diff --git a/drivers/pinctrl/pinctrl-pic64gx-gpio2.c b/drivers/pinctrl/pinctrl-pic64gx-gpio2.c
new file mode 100644
index 000000000000..b0607aad934c
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-pic64gx-gpio2.c
@@ -0,0 +1,283 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/bitfield.h>
+#include <linux/cleanup.h>
+#include <linux/module.h>
+#include <linux/mfd/syscon.h>
+#include <linux/mod_devicetable.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/regmap.h>
+#include <linux/seq_file.h>
+
+#include <linux/pinctrl/pinconf-generic.h>
+#include <linux/pinctrl/pinconf.h>
+#include <linux/pinctrl/pinctrl.h>
+#include <linux/pinctrl/pinmux.h>
+
+#include "core.h"
+#include "pinctrl-utils.h"
+#include "pinconf.h"
+#include "pinmux.h"
+
+#define PIC64GX_PINCTRL_DT_FUNC_MASK GENMASK(3, 0);
+#define PIC64GX_PINCTRL_DT_PIN_OFFSET 16
+
+#define PIC64GX_PINMUX_REG 0x0
+
+static const struct regmap_config pic64gx_gpio2_pinctrl_regmap_config = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.val_format_endian = REGMAP_ENDIAN_LITTLE,
+	.max_register = 0x0,
+};
+
+struct pic64gx_gpio2_pinctrl_mux_config {
+	u8 pin;
+	u32 config;
+};
+
+struct pic64gx_gpio2_pinctrl {
+	struct pinctrl_dev *pctrl;
+	struct device *dev;
+	struct regmap *regmap;
+	struct mutex mutex;
+	struct pinctrl_desc desc;
+};
+
+static void pic64gx_gpio2_pinctrl_dbg_show(struct pinctrl_dev *pctrl_dev, struct seq_file *seq,
+				  unsigned int pin)
+{
+	struct pic64gx_gpio2_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
+	u32 val;
+
+	seq_printf(seq, "reg: %x, pin: %u ", PIC64GX_PINMUX_REG, pin);
+
+	regmap_read(pctrl->regmap, PIC64GX_PINMUX_REG, &val);
+	val = (val & BIT(pin)) >> pin;
+	seq_printf(seq, "val: %x\n", val);
+}
+
+static int pic64gx_gpio2_pinctrl_dt_node_to_map(struct pinctrl_dev *pctrl_dev, struct device_node *np,
+					 struct pinctrl_map **maps, unsigned int *num_maps)
+{
+	struct pic64gx_gpio2_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
+	struct device *dev = pctrl->dev;
+	struct pic64gx_gpio2_pinctrl_mux_config *pinmuxs;
+	struct pinctrl_map *map;
+	const char **grpnames;
+	const char *grpname;
+	int ret, i, npins;
+	unsigned int config, *pins;
+
+	map = kcalloc(1, sizeof(*map), GFP_KERNEL);
+	if (!map)
+		return -ENOMEM;
+
+	guard(mutex)(&pctrl->mutex);
+
+	npins = of_property_count_u32_elems(np, "pinmux");
+	if (npins < 1) {
+		dev_err(dev, "invalid pinctrl group %pOFn\n", np);
+		return -EINVAL;
+	}
+
+	grpnames = devm_kmalloc(dev, sizeof(*grpnames), GFP_KERNEL);
+	if (!grpnames)
+		return -ENOMEM;
+
+	grpname = devm_kasprintf(dev, GFP_KERNEL, "%pOFn", np);
+	if (!grpname)
+		return -ENOMEM;
+
+	*grpnames = grpname;
+
+	pins = devm_kcalloc(dev, npins, sizeof(*pins), GFP_KERNEL);
+	if (!pins)
+		return -ENOMEM;
+
+	pinmuxs = devm_kcalloc(dev, npins, sizeof(*pinmuxs), GFP_KERNEL);
+	if (!pinmuxs)
+		return -ENOMEM;
+
+	for (i = 0; i < npins; i++) {
+		ret = of_property_read_u32_index(np, "pinmux", i, &config);
+		if (ret)
+			return -EINVAL;
+
+		pins[i] = config >> PIC64GX_PINCTRL_DT_PIN_OFFSET;
+		pinmuxs[i].config = config & PIC64GX_PINCTRL_DT_FUNC_MASK;
+		pinmuxs[i].pin = config >> PIC64GX_PINCTRL_DT_PIN_OFFSET;
+	}
+
+	map->type = PIN_MAP_TYPE_MUX_GROUP;
+	map->data.mux.function = np->name;
+	map->data.mux.group = grpname;
+
+	ret = pinctrl_generic_add_group(pctrl_dev, grpname, pins, npins, pinmuxs);
+	if (ret < 0)
+		return dev_err_probe(dev, ret, "failed to add group %s: %d\n", grpname, ret);
+
+	ret = pinmux_generic_add_function(pctrl_dev, np->name, grpnames, 1, NULL);
+	if (ret < 0) {
+		pinctrl_utils_free_map(pctrl_dev, map, 1);
+		return dev_err_probe(dev, ret, "error adding function %s\n", np->name);
+	}
+
+	*maps = map;
+	*num_maps = 1;
+
+	return 0;
+};
+
+static struct pinctrl_ops pic64gx_gpio2_pinctrl_ops = {
+	.get_groups_count = pinctrl_generic_get_group_count,
+	.get_group_name	= pinctrl_generic_get_group_name,
+	.get_group_pins	= pinctrl_generic_get_group_pins,
+	.pin_dbg_show = pic64gx_gpio2_pinctrl_dbg_show,
+	.dt_node_to_map = pic64gx_gpio2_pinctrl_dt_node_to_map,
+	.dt_free_map = pinctrl_utils_free_map,
+};
+
+static int pic64gx_gpio2_pinctrl_set_pin_func(struct pic64gx_gpio2_pinctrl *pctrl, u8 pin, u32 config)
+{
+	struct device *dev = pctrl->dev;
+	u32 func;
+
+	func = config & PIC64GX_PINCTRL_DT_FUNC_MASK;
+	func <<= pin;
+
+	dev_dbg(dev, "Setting pin %u reg: %x pin %u func %x\n", pin, PIC64GX_PINMUX_REG, pin, func);
+
+	regmap_set_bits(pctrl->regmap, PIC64GX_PINMUX_REG, func);
+
+	return 0;
+}
+
+static int pic64gx_gpio2_pinctrl_set_mux(struct pinctrl_dev *pctrl_dev, unsigned int fsel, unsigned int gsel)
+{
+	struct pic64gx_gpio2_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
+	const struct group_desc *group;
+	struct pic64gx_gpio2_pinctrl_mux_config *configs;
+	int ret = -EINVAL;
+
+	group = pinctrl_generic_get_group(pctrl_dev, gsel);
+	if (!group)
+		return -EINVAL;
+
+	configs = group->data;
+
+	for (int i = 0; i < group->grp.npins; i++) {
+		u8 pin = configs[i].pin;
+		u32 config = configs[i].config;
+
+		ret = pic64gx_gpio2_pinctrl_set_pin_func(pctrl, pin, config);
+	}
+
+	return ret;
+}
+
+static const struct pinmux_ops pic64gx_gpio2_pinctrl_pinmux_ops = {
+	.get_functions_count = pinmux_generic_get_function_count,
+	.get_function_name = pinmux_generic_get_function_name,
+	.get_function_groups = pinmux_generic_get_function_groups,
+	.set_mux = pic64gx_gpio2_pinctrl_set_mux,
+};
+
+static const struct pinctrl_pin_desc pic64gx_gpio2_pinctrl_pins[] = {
+	PINCTRL_PIN(0, "gpio2 0"),
+	PINCTRL_PIN(1, "gpio2 1"),
+	PINCTRL_PIN(2, "gpio2 2"),
+	PINCTRL_PIN(3, "gpio2 3"),
+	PINCTRL_PIN(4, "gpio2 4"),
+	PINCTRL_PIN(5, "gpio2 5"),
+	PINCTRL_PIN(6, "gpio2 6"),
+	PINCTRL_PIN(7, "gpio2 7"),
+	PINCTRL_PIN(8, "gpio2 8"),
+	PINCTRL_PIN(9, "gpio2 9"),
+	PINCTRL_PIN(10, "gpio2 10"),
+	PINCTRL_PIN(11, "gpio2 11"),
+	PINCTRL_PIN(12, "gpio2 12"),
+	PINCTRL_PIN(13, "gpio2 13"),
+	PINCTRL_PIN(14, "gpio2 14"),
+	PINCTRL_PIN(15, "gpio2 15"),
+	PINCTRL_PIN(16, "gpio2 16"),
+	PINCTRL_PIN(17, "gpio2 17"),
+	PINCTRL_PIN(18, "gpio2 18"),
+	PINCTRL_PIN(19, "gpio2 19"),
+	PINCTRL_PIN(20, "gpio2 20"),
+	PINCTRL_PIN(21, "gpio2 21"),
+	PINCTRL_PIN(22, "gpio2 22"),
+	PINCTRL_PIN(23, "gpio2 23"),
+	PINCTRL_PIN(24, "gpio2 24"),
+	PINCTRL_PIN(25, "gpio2 25"),
+	PINCTRL_PIN(26, "gpio2 26"),
+	PINCTRL_PIN(27, "gpio2 27"),
+	PINCTRL_PIN(28, "gpio2 28"),
+
+};
+
+static int pic64gx_gpio2_pinctrl_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct pic64gx_gpio2_pinctrl *pctrl;
+	void __iomem *base;
+	int ret;
+
+	pctrl = devm_kzalloc(dev, sizeof(*pctrl), GFP_KERNEL);
+	if (!pctrl)
+		return -ENOMEM;
+
+	base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(base)) {
+		dev_err(dev, "Failed get resource\n");
+		return PTR_ERR(base);
+	}
+
+	pctrl->regmap = devm_regmap_init_mmio(dev, base, &pic64gx_gpio2_pinctrl_regmap_config);
+	if (IS_ERR(pctrl->regmap)) {
+		dev_err(dev, "Failed to map regmap\n");
+		return PTR_ERR(pctrl->regmap);
+	}
+
+	pctrl->desc.name = dev_name(dev);
+	pctrl->desc.pins = pic64gx_gpio2_pinctrl_pins;
+	pctrl->desc.npins = ARRAY_SIZE(pic64gx_gpio2_pinctrl_pins);
+	pctrl->desc.pctlops = &pic64gx_gpio2_pinctrl_ops;
+	pctrl->desc.pmxops = &pic64gx_gpio2_pinctrl_pinmux_ops;
+	pctrl->desc.owner = THIS_MODULE;
+
+	pctrl->dev = dev;
+
+	ret = devm_mutex_init(dev, &pctrl->mutex);
+	if (ret)
+		return ret;
+
+	platform_set_drvdata(pdev, pctrl);
+
+	pctrl->pctrl = devm_pinctrl_register(&pdev->dev, &pctrl->desc, pctrl);
+	if (IS_ERR(pctrl->pctrl))
+		return PTR_ERR(pctrl->pctrl);
+
+	return 0;
+}
+
+static const struct of_device_id pic64gx_gpio2_pinctrl_of_match[] = {
+	{ .compatible = "microchip,pic64gx-pinctrl-gpio2" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, pic64gx_gpio2_pinctrl_of_match);
+
+static struct platform_driver pic64gx_gpio2_pinctrl_driver = {
+	.driver = {
+		.name = "pic64gx-pinctrl-gpio2",
+		.of_match_table = pic64gx_gpio2_pinctrl_of_match,
+	},
+	.probe = pic64gx_gpio2_pinctrl_probe,
+};
+module_platform_driver(pic64gx_gpio2_pinctrl_driver);
+
+MODULE_AUTHOR("Conor Dooley <conor.dooley@microchip.com>");
+MODULE_DESCRIPTION("pic64gx gpio2 pinctrl driver");
+MODULE_LICENSE("GPL");
-- 
2.47.3

[RFC 5/5] riscv: dts: microchip: add pinctrl nodes for iomux0

[Conor Dooley]

From: Conor Dooley <conor.dooley@microchip.com>

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
This is in RFC state, the commented out perpiherals use a child to
iomux0, and the patch ultimately adding the pinctrl dts nodes will add
both iomux0 and its child. Please ignore them for now. That's also the
reason for no commit message yet, since the final thing will be
different.
---
 .../dts/microchip/mpfs-icicle-kit-fabric.dtsi |  56 +++++++++
 .../boot/dts/microchip/mpfs-icicle-kit.dts    |   1 -
 .../boot/dts/microchip/mpfs-pinctrl.dtsi      | 117 ++++++++++++++++++
 arch/riscv/boot/dts/microchip/mpfs.dtsi       |   9 ++
 4 files changed, 182 insertions(+), 1 deletion(-)
 create mode 100644 arch/riscv/boot/dts/microchip/mpfs-pinctrl.dtsi

diff --git a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi
index a6dda55a2d1d..4cf8fd1dd24d 100644
--- a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi
+++ b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi
@@ -1,6 +1,9 @@
 // SPDX-License-Identifier: (GPL-2.0 OR MIT)
 /* Copyright (c) 2020-2021 Microchip Technology Inc */
 
+#include "mpfs.dtsi"
+#include "mpfs-pinctrl.dtsi"
+
 / {
 	compatible = "microchip,mpfs-icicle-reference-rtlv2210", "microchip,mpfs-icicle-kit",
 		     "microchip,mpfs";
@@ -63,6 +66,15 @@ refclk_ccc: cccrefclk {
 	};
 };
 
+&can0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&can0_fabric>;
+};
+
+&can1 {
+//	pinctrl-names = "default";
+};
+
 &ccc_nw {
 	clocks = <&refclk_ccc>, <&refclk_ccc>, <&refclk_ccc>, <&refclk_ccc>,
 		 <&refclk_ccc>, <&refclk_ccc>;
@@ -70,3 +82,47 @@ &ccc_nw {
 		      "dll0_ref", "dll1_ref";
 	status = "okay";
 };
+
+&i2c0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&i2c0_fabric>;
+};
+
+&i2c1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&i2c1_fabric>;
+};
+
+&mmuart1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&mmuart1_fabric>;
+};
+
+&mmuart2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&mmuart2_fabric>;
+};
+
+&mmuart3 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&mmuart3_fabric>;
+};
+
+&mmuart4 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&mmuart4_fabric>;
+};
+
+&qspi {
+	pinctrl-names = "default";
+	pinctrl-0 = <&qspi_fabric>;
+};
+
+&spi0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&spi0_fabric>;
+};
+
+&spi1 {
+//	pinctrl-names = "default";
+};
diff --git a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts
index f80df225f72b..3c4d5f576e86 100644
--- a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts
+++ b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts
@@ -3,7 +3,6 @@
 
 /dts-v1/;
 
-#include "mpfs.dtsi"
 #include "mpfs-icicle-kit-fabric.dtsi"
 #include <dt-bindings/gpio/gpio.h>
 #include <dt-bindings/leds/common.h>
diff --git a/arch/riscv/boot/dts/microchip/mpfs-pinctrl.dtsi b/arch/riscv/boot/dts/microchip/mpfs-pinctrl.dtsi
new file mode 100644
index 000000000000..1e4d55bd786f
--- /dev/null
+++ b/arch/riscv/boot/dts/microchip/mpfs-pinctrl.dtsi
@@ -0,0 +1,117 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+
+#define MPFS_PINFUNC(pin, func) (((pin) << 16) | (func))
+
+&iomux0 {
+	spi0_mssio: spi0-mssio-pins {
+		pinmux = <MPFS_PINFUNC(0, 0)>;
+	};
+
+	spi0_fabric: spi0-fabric-pins {
+		pinmux = <MPFS_PINFUNC(0, 1)>;
+	};
+
+	spi1_mssio: spi1-mssio-pins {
+		pinmux = <MPFS_PINFUNC(1, 0)>;
+	};
+
+	spi1_fabric: spi1-fabric-pins {
+		pinmux = <MPFS_PINFUNC(1, 1)>;
+	};
+
+	i2c0_mssio: i2c0-mssio-pins {
+		pinmux = <MPFS_PINFUNC(2, 0)>;
+	};
+
+	i2c0_fabric: i2c0-fabric-pins {
+		pinmux = <MPFS_PINFUNC(2, 1)>;
+	};
+
+	i2c1_mssio: i2c1-mssio-pins {
+		pinmux = <MPFS_PINFUNC(3, 0)>;
+	};
+
+	i2c1_fabric: i2c1-fabric-pins {
+		pinmux = <MPFS_PINFUNC(3, 1)>;
+	};
+
+	can0_mssio: can0-mssio-pins {
+		pinmux = <MPFS_PINFUNC(4, 0)>;
+	};
+
+	can0_fabric: can0-fabric-pins {
+		pinmux = <MPFS_PINFUNC(4, 1)>;
+	};
+
+	can1_mssio: can1-mssio-pins {
+		pinmux = <MPFS_PINFUNC(5, 0)>;
+	};
+
+	can1_fabric: can1-fabric-pins {
+		pinmux = <MPFS_PINFUNC(5, 1)>;
+	};
+
+	qspi_mssio: qspi-mssio-pins {
+		pinmux = <MPFS_PINFUNC(6, 0)>;
+	};
+
+	qspi_fabric: qspi-fabric-pins {
+		pinmux = <MPFS_PINFUNC(6, 1)>;
+	};
+
+	mmuart0_mssio: mmuart0-mssio-pins {
+		pinmux = <MPFS_PINFUNC(7, 0)>;
+	};
+
+	mmuart0_fabric: mmuart0-fabric-pins {
+		pinmux = <MPFS_PINFUNC(7, 1)>;
+	};
+
+	mmuart1_mssio: mmuart1-mssio-pins {
+		pinmux = <MPFS_PINFUNC(8, 0)>;
+	};
+
+	mmuart1_fabric: mmuart1-fabric-pins {
+		pinmux = <MPFS_PINFUNC(8, 1)>;
+	};
+
+	mmuart2_mssio: mmuart2-mssio-pins {
+		pinmux = <MPFS_PINFUNC(9, 0)>;
+	};
+
+	mmuart2_fabric: mmuart2-fabric-pins {
+		pinmux = <MPFS_PINFUNC(9, 1)>;
+	};
+
+	mmuart3_mssio: mmuart3-mssio-pins {
+		pinmux = <MPFS_PINFUNC(10, 0)>;
+	};
+
+	mmuart3_fabric: mmuart3-fabric-pins {
+		pinmux = <MPFS_PINFUNC(10, 1)>;
+	};
+
+	mmuart4_mssio: mmuart4-mssio-pins {
+		pinmux = <MPFS_PINFUNC(11, 0)>;
+	};
+
+	mmuart4_fabric: mmuart4-fabric-pins {
+		pinmux = <MPFS_PINFUNC(11, 1)>;
+	};
+
+	mdio0_mssio: mdio0-mssio-pins {
+		pinmux = <MPFS_PINFUNC(12, 0)>;
+	};
+
+	mdio0_fabric: mdio0-fabric-pins {
+		pinmux = <MPFS_PINFUNC(12, 1)>;
+	};
+
+	mdio1_mssio: mdio1-mssio-pins {
+		pinmux = <MPFS_PINFUNC(13, 0)>;
+	};
+
+	mdio1_fabric: mdio1-fabric-pins {
+		pinmux = <MPFS_PINFUNC(13, 1)>;
+	};
+};
diff --git a/arch/riscv/boot/dts/microchip/mpfs.dtsi b/arch/riscv/boot/dts/microchip/mpfs.dtsi
index 5c2963e269b8..0a0cfd3d3054 100644
--- a/arch/riscv/boot/dts/microchip/mpfs.dtsi
+++ b/arch/riscv/boot/dts/microchip/mpfs.dtsi
@@ -254,7 +254,16 @@ pdma: dma-controller@3000000 {
 		mss_top_sysreg: syscon@20002000 {
 			compatible = "microchip,mpfs-mss-top-sysreg", "syscon", "simple-mfd";
 			reg = <0x0 0x20002000 0x0 0x1000>;
+			#address-cells = <1>;
+			#size-cells = <1>;
 			#reset-cells = <1>;
+
+			iomux0: pinctrl@200 {
+				compatible = "microchip,mpfs-pinctrl-iomux0";
+				reg = <0x200 0x4>;
+				pinctrl-use-default;
+
+			};
 		};
 
 		sysreg_scb: syscon@20003000 {
-- 
2.47.3

Re: [RFC 0/5] microchip mpfs/pic64gx pinctrl questions

[Linus Walleij]

Hi Conor,

thanks for your patches!

looking at the drivers it appears to be trying extensively to make use
of the pinmux = <>; property to mux entire groups of pins.

pinmux = <nn>; is supposed to mux *one* pin per group, not entire
groups of pins from one property. See
Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml:

  The pinmux property accepts an array of pinmux groups, each of them describing
  a single pin multiplexing configuration.

  pincontroller {
    state_0_node_a {
      pinmux = <PINMUX_GROUP>, <PINMUX_GROUP>, ...;
    };
  };

So e.g. when you do this:

       spi0_mssio: spi0-mssio-pins {
         pinmux = <MPFS_PINFUNC(0, 0)>;
       };

We all know SPI uses more than one pin so this is clearly abusing
the pinmux property.

It is unfortunate that so many drivers now use this "mux one pin
individually" concept that we cannot see the diversity of pin
controllers.

I cannot recommend using the pinmux property for this SoC.

What you need to do is to define the actual pins and groups
that you have.

Look for example at
Documentation/devicetree/bindings/pinctrl/cortina,gemini-pinctrl.txt
drivers/pinctrl/pinctrl-gemini.c
arch/arm/boot/dts/gemini/gemini.dtsi

This is another SoC that muxes pins in groups, not in single per-pin
settings.

Notice that the driver in this case enumerates and registers all 323
pins on the package! This is done because some of the groups
are mutually exclusive and this way the pin control framework
will do its job to detect collisions between pin groups and disallow
this, and that is what pin control is supposed to be doing.

I.e. do not orient your design around which registers and settings
you have, and do not model your driver around that, instead
model the driver around which actual pins exist on the physical
component, how these are sorted into groups, how the groups
are related to function (such as the group of SPI pins being
related to the spi function) and define these pins, groups
and functions in your driver.

Yours,
Linus Walleij

Re: [RFC 2/5] dt-bindings: pinctrl: add pic64gx "gpio2" pinmux

[Linus Walleij]

On Fri, Sep 26, 2025 at 4:33 PM Conor Dooley <conor@kernel.org> wrote:

> pin     role (1/0)
> ---     ----------
> E14     MAC_0_MDC/GPIO_2_0
> E15     MAC_0_MDIO/GPIO_2_1
> F16     MAC_1_MDC/GPIO_2_2
> F17     MAC_1_MDIO/GPIO_2_3

So this is a group you can name "mac_grp" and a function
you can name "mac".

> D19     SPI_0_CLK/GPIO_2_4
> B18     SPI_0_SS0/GPIO_2_5
(...)
> E19     SPI_0_DO/GPIO_2_10
> C7      SPI_0_DI/GPIO_2_11

These pins would be "spi0_grp", function "spi0".

etc. No need for "pinmux" properties just use classix
group and function strings.

Yours,
Linus Walleij

Re: [RFC 3/5] pinctrl: add polarfire soc iomux0 pinmux driver

[Linus Walleij]

On Fri, Sep 26, 2025 at 4:33 PM Conor Dooley <conor@kernel.org> wrote:

> +static const struct pinctrl_pin_desc mpfs_iomux0_pinctrl_pins[] = {
> +       PINCTRL_PIN(0, "spi0"),
> +       PINCTRL_PIN(1, "spi1"),
> +       PINCTRL_PIN(2, "i2c0"),
> +       PINCTRL_PIN(3, "i2c1"),
> +       PINCTRL_PIN(4, "can0"),
> +       PINCTRL_PIN(5, "can1"),
> +       PINCTRL_PIN(6, "qspi"),
> +       PINCTRL_PIN(7, "uart0"),
> +       PINCTRL_PIN(8, "uart1"),
> +       PINCTRL_PIN(9, "uart2"),
> +       PINCTRL_PIN(10, "uart3"),
> +       PINCTRL_PIN(11, "uart4"),
> +       PINCTRL_PIN(12, "mdio0"),
> +       PINCTRL_PIN(13, "mdio1"),

This looks like it is abusing the API. These things do not look like
"pins" at all, rather these are all groups, right?

Yours,
Linus Walleij

Re: [RFC 3/5] pinctrl: add polarfire soc iomux0 pinmux driver

[Linus Walleij]

On Wed, Oct 1, 2025 at 1:34 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Sep 26, 2025 at 4:33 PM Conor Dooley <conor@kernel.org> wrote:
>
> > +static const struct pinctrl_pin_desc mpfs_iomux0_pinctrl_pins[] = {
> > +       PINCTRL_PIN(0, "spi0"),
> > +       PINCTRL_PIN(1, "spi1"),
> > +       PINCTRL_PIN(2, "i2c0"),
> > +       PINCTRL_PIN(3, "i2c1"),
> > +       PINCTRL_PIN(4, "can0"),
> > +       PINCTRL_PIN(5, "can1"),
> > +       PINCTRL_PIN(6, "qspi"),
> > +       PINCTRL_PIN(7, "uart0"),
> > +       PINCTRL_PIN(8, "uart1"),
> > +       PINCTRL_PIN(9, "uart2"),
> > +       PINCTRL_PIN(10, "uart3"),
> > +       PINCTRL_PIN(11, "uart4"),
> > +       PINCTRL_PIN(12, "mdio0"),
> > +       PINCTRL_PIN(13, "mdio1"),
>
> This looks like it is abusing the API. These things do not look like
> "pins" at all, rather these are all groups, right?

Or maybe they are rather functions. Like there is a function spi0
that can be mapped to a set of pins such as spi0_grp = <1, 2, 3, 4...>

I recommend a refresher with
https://docs.kernel.org/driver-api/pin-control.html
and work from there, and avoid looking too much at other
drivers that don't necessarily do the right thing.

Yours,
Linus Walleij

Re: [RFC 3/5] pinctrl: add polarfire soc iomux0 pinmux driver

[Conor Dooley]

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

On Wed, Oct 01, 2025 at 01:36:49PM +0200, Linus Walleij wrote:
> On Wed, Oct 1, 2025 at 1:34 PM Linus Walleij <linus.walleij@linaro.org> wrote:
> > On Fri, Sep 26, 2025 at 4:33 PM Conor Dooley <conor@kernel.org> wrote:
> >
> > > +static const struct pinctrl_pin_desc mpfs_iomux0_pinctrl_pins[] = {
> > > +       PINCTRL_PIN(0, "spi0"),
> > > +       PINCTRL_PIN(1, "spi1"),
> > > +       PINCTRL_PIN(2, "i2c0"),
> > > +       PINCTRL_PIN(3, "i2c1"),
> > > +       PINCTRL_PIN(4, "can0"),
> > > +       PINCTRL_PIN(5, "can1"),
> > > +       PINCTRL_PIN(6, "qspi"),
> > > +       PINCTRL_PIN(7, "uart0"),
> > > +       PINCTRL_PIN(8, "uart1"),
> > > +       PINCTRL_PIN(9, "uart2"),
> > > +       PINCTRL_PIN(10, "uart3"),
> > > +       PINCTRL_PIN(11, "uart4"),
> > > +       PINCTRL_PIN(12, "mdio0"),
> > > +       PINCTRL_PIN(13, "mdio1"),
> >
> > This looks like it is abusing the API. These things do not look like
> > "pins" at all, rather these are all groups, right?
> 
> Or maybe they are rather functions. Like there is a function spi0
> that can be mapped to a set of pins such as spi0_grp = <1, 2, 3, 4...>

They're not actually package pins at all that are being configured here,
it's internal routing inside the FPGA. It does operate on a function
level, but I don't think there's a neat mapping to the pinctrl subsystem
which (AFAIU) considers functions to contain groups, which in turn
contain pins. I suppose it could be thought of that, for example, spi0
is actually a function containing 4 (or 5, don't ask - or do if you want
to read a rant about pointlessly confusing design) "pins" in 1 group.

If I could just work in terms of functions only, and avoid groups or
pins at all, feels (to me ofc) like it'd maybe match the purpose of this
aspect of the hardware better.

> I recommend a refresher with
> https://docs.kernel.org/driver-api/pin-control.html
> and work from there, and avoid looking too much at other
> drivers that don't necessarily do the right thing.


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

Re: [RFC 2/5] dt-bindings: pinctrl: add pic64gx "gpio2" pinmux

[Conor Dooley]

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

On Wed, Oct 01, 2025 at 01:32:37PM +0200, Linus Walleij wrote:
> On Fri, Sep 26, 2025 at 4:33 PM Conor Dooley <conor@kernel.org> wrote:
> 
> > pin     role (1/0)
> > ---     ----------
> > E14     MAC_0_MDC/GPIO_2_0
> > E15     MAC_0_MDIO/GPIO_2_1
> > F16     MAC_1_MDC/GPIO_2_2
> > F17     MAC_1_MDIO/GPIO_2_3
> 
> So this is a group you can name "mac_grp" and a function
> you can name "mac".
> 
> > D19     SPI_0_CLK/GPIO_2_4
> > B18     SPI_0_SS0/GPIO_2_5
> (...)
> > E19     SPI_0_DO/GPIO_2_10
> > C7      SPI_0_DI/GPIO_2_11
> 
> These pins would be "spi0_grp", function "spi0".
> 
> etc. No need for "pinmux" properties just use classix
> group and function strings.

tbh, I found it hard to understand the "line" between using a pinmux
property and where stuff should be described in groups or functions in a
driver. What is that line?

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

Re: [RFC 2/5] dt-bindings: pinctrl: add pic64gx "gpio2" pinmux

[Conor Dooley]

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

On Wed, Oct 01, 2025 at 04:47:12PM +0100, Conor Dooley wrote:
> On Wed, Oct 01, 2025 at 01:32:37PM +0200, Linus Walleij wrote:
> > On Fri, Sep 26, 2025 at 4:33 PM Conor Dooley <conor@kernel.org> wrote:
> > 
> > > pin     role (1/0)
> > > ---     ----------
> > > E14     MAC_0_MDC/GPIO_2_0
> > > E15     MAC_0_MDIO/GPIO_2_1
> > > F16     MAC_1_MDC/GPIO_2_2
> > > F17     MAC_1_MDIO/GPIO_2_3
> > 
> > So this is a group you can name "mac_grp" and a function
> > you can name "mac".
> > 
> > > D19     SPI_0_CLK/GPIO_2_4
> > > B18     SPI_0_SS0/GPIO_2_5
> > (...)
> > > E19     SPI_0_DO/GPIO_2_10
> > > C7      SPI_0_DI/GPIO_2_11
> > 
> > These pins would be "spi0_grp", function "spi0".
> > 
> > etc. No need for "pinmux" properties just use classix
> > group and function strings.
> 
> tbh, I found it hard to understand the "line" between using a pinmux
> property and where stuff should be described in groups or functions in a
> driver. What is that line?

You may have noticed that I never really review pinctrl bindings, and it
is the lack of experience with this kind of decision that's why I never
do it.

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

Re: [RFC 0/5] microchip mpfs/pic64gx pinctrl questions

[Conor Dooley]

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

On Wed, Oct 01, 2025 at 01:29:01PM +0200, Linus Walleij wrote:
> Hi Conor,
> 
> thanks for your patches!
> 
> looking at the drivers it appears to be trying extensively to make use
> of the pinmux = <>; property to mux entire groups of pins.
> 
> pinmux = <nn>; is supposed to mux *one* pin per group, not entire
> groups of pins from one property. See
> Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml:
> 
>   The pinmux property accepts an array of pinmux groups, each of them describing
>   a single pin multiplexing configuration.
> 
>   pincontroller {
>     state_0_node_a {
>       pinmux = <PINMUX_GROUP>, <PINMUX_GROUP>, ...;
>     };
>   };
> 
> So e.g. when you do this:
> 
>        spi0_mssio: spi0-mssio-pins {
>          pinmux = <MPFS_PINFUNC(0, 0)>;
>        };
> 
> We all know SPI uses more than one pin so this is clearly abusing
> the pinmux property.
> 
> It is unfortunate that so many drivers now use this "mux one pin
> individually" concept that we cannot see the diversity of pin
> controllers.
> 
> I cannot recommend using the pinmux property for this SoC.
> 
> What you need to do is to define the actual pins and groups
> that you have.
> 
> Look for example at
> Documentation/devicetree/bindings/pinctrl/cortina,gemini-pinctrl.txt
> drivers/pinctrl/pinctrl-gemini.c

Does this driver have a mistake in L2074, using ARRAY_SIZE(idegrps) for
dram?

> arch/arm/boot/dts/gemini/gemini.dtsi
> 
> This is another SoC that muxes pins in groups, not in single per-pin
> settings.
> 
> Notice that the driver in this case enumerates and registers all 323
> pins on the package! This is done because some of the groups
> are mutually exclusive and this way the pin control framework
> will do its job to detect collisions between pin groups and disallow
> this, and that is what pin control is supposed to be doing.
> 
> I.e. do not orient your design around which registers and settings
> you have, and do not model your driver around that, instead
> model the driver around which actual pins exist on the physical
> component, how these are sorted into groups, how the groups
> are related to function (such as the group of SPI pins being
> related to the spi function) and define these pins, groups
> and functions in your driver.
> 
> Yours,
> Linus Walleij

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

Re: [RFC 0/5] microchip mpfs/pic64gx pinctrl questions

[Conor Dooley]

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

On Wed, Oct 01, 2025 at 01:29:01PM +0200, Linus Walleij wrote:
> Hi Conor,
> 
> thanks for your patches!
> 
> looking at the drivers it appears to be trying extensively to make use
> of the pinmux = <>; property to mux entire groups of pins.
> 
> pinmux = <nn>; is supposed to mux *one* pin per group, not entire
> groups of pins from one property. See
> Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml:
> 
>   The pinmux property accepts an array of pinmux groups, each of them describing
>   a single pin multiplexing configuration.
> 
>   pincontroller {
>     state_0_node_a {
>       pinmux = <PINMUX_GROUP>, <PINMUX_GROUP>, ...;
>     };
>   };
> 
> So e.g. when you do this:
> 
>        spi0_mssio: spi0-mssio-pins {
>          pinmux = <MPFS_PINFUNC(0, 0)>;
>        };
> 
> We all know SPI uses more than one pin so this is clearly abusing
> the pinmux property.
> 
> It is unfortunate that so many drivers now use this "mux one pin
> individually" concept that we cannot see the diversity of pin
> controllers.
> 
> I cannot recommend using the pinmux property for this SoC.
> 
> What you need to do is to define the actual pins and groups
> that you have.
> 
> Look for example at
> Documentation/devicetree/bindings/pinctrl/cortina,gemini-pinctrl.txt
> drivers/pinctrl/pinctrl-gemini.c
> arch/arm/boot/dts/gemini/gemini.dtsi
> 
> This is another SoC that muxes pins in groups, not in single per-pin
> settings.

This looks like something that the "gpio2" stuff could definitely go to,
since it covers multiple functions trying to access the same pin. Do you
have an "approved" example for a more demultiplexer case, where the
contention is about which of multiple possible pins (or pin analogues)
an IO from a particular block must be routed to?

> Notice that the driver in this case enumerates and registers all 323
> pins on the package! This is done because some of the groups
> are mutually exclusive and this way the pin control framework
> will do its job to detect collisions between pin groups and disallow
> this, and that is what pin control is supposed to be doing.

In that case, the mutual exclusion would be that a function can only be
routed to one "pin", but there's no concern about multiple functions
being routed to any given "pin".

> 
> I.e. do not orient your design around which registers and settings
> you have, and do not model your driver around that, instead
> model the driver around which actual pins exist on the physical
> component, how these are sorted into groups, how the groups
> are related to function (such as the group of SPI pins being
> related to the spi function) and define these pins, groups
> and functions in your driver.
> 
> Yours,
> Linus Walleij

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

Re: [RFC 0/5] microchip mpfs/pic64gx pinctrl questions

[Conor Dooley]

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

On Wed, Oct 01, 2025 at 05:15:07PM +0100, Conor Dooley wrote:
> On Wed, Oct 01, 2025 at 01:29:01PM +0200, Linus Walleij wrote:
> > Hi Conor,
> > 
> > thanks for your patches!
> > 
> > looking at the drivers it appears to be trying extensively to make use
> > of the pinmux = <>; property to mux entire groups of pins.
> > 
> > pinmux = <nn>; is supposed to mux *one* pin per group, not entire
> > groups of pins from one property. See
> > Documentation/devicetree/bindings/pinctrl/pinmux-node.yaml:
> > 
> >   The pinmux property accepts an array of pinmux groups, each of them describing
> >   a single pin multiplexing configuration.
> > 
> >   pincontroller {
> >     state_0_node_a {
> >       pinmux = <PINMUX_GROUP>, <PINMUX_GROUP>, ...;
> >     };
> >   };
> > 
> > So e.g. when you do this:
> > 
> >        spi0_mssio: spi0-mssio-pins {
> >          pinmux = <MPFS_PINFUNC(0, 0)>;
> >        };
> > 
> > We all know SPI uses more than one pin so this is clearly abusing
> > the pinmux property.
> > 
> > It is unfortunate that so many drivers now use this "mux one pin
> > individually" concept that we cannot see the diversity of pin
> > controllers.
> > 
> > I cannot recommend using the pinmux property for this SoC.
> > 
> > What you need to do is to define the actual pins and groups
> > that you have.
> > 
> > Look for example at
> > Documentation/devicetree/bindings/pinctrl/cortina,gemini-pinctrl.txt
> > drivers/pinctrl/pinctrl-gemini.c
> > arch/arm/boot/dts/gemini/gemini.dtsi
> > 
> > This is another SoC that muxes pins in groups, not in single per-pin
> > settings.
> 
> This looks like something that the "gpio2" stuff could definitely go to,
> since it covers multiple functions trying to access the same pin. Do you
> have an "approved" example for a more demultiplexer case, where the
> contention is about which of multiple possible pins (or pin analogues)
> an IO from a particular block must be routed to?
> 
> > Notice that the driver in this case enumerates and registers all 323
> > pins on the package! This is done because some of the groups
> > are mutually exclusive and this way the pin control framework
> > will do its job to detect collisions between pin groups and disallow
> > this, and that is what pin control is supposed to be doing.
> 
> In that case, the mutual exclusion would be that a function can only be
> routed to one "pin", but there's no concern about multiple functions
> being routed to any given "pin".

So, what I ended up doing is moving the "gpio2" stuff to use
functions/groups as your gemini stuff does, so each function contains
one group containing all the pins it needs - except for the gpio
function which contains analogues for each of the function's groups.
I'll send a patchset next week when the merge window is closed, but for
now that driver is here:
https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=pinctrl&id=8c0fdf0c76fe99549d293894121d64300dc4057f
Unlike "gpio2", where each pin supports two mutually exclusive
functions and so fits nicely into the pins/groups/functions hierarchy,
for iomux0 each function has two mutually exclusive routings but there's
no contention over the "pins", the corresponding function is either
routed there or it is entirely unused. I implemented this by co-opting
the pin structure to really contain functions, with each appearing in
two groups, one per routing. Each function then contains those two
groups - I think that then takes advantage of the framework's collision
detection like you requested? It's here in case you care enough to take
a look before I send a proper patchset next week:
https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=pinctrl&id=5aec232c11bd45262fb2cfaf7994e3030fd6d947

Cheers,
Conor.

> 
> > 
> > I.e. do not orient your design around which registers and settings
> > you have, and do not model your driver around that, instead
> > model the driver around which actual pins exist on the physical
> > component, how these are sorted into groups, how the groups
> > are related to function (such as the group of SPI pins being
> > related to the spi function) and define these pins, groups
> > and functions in your driver.
> > 
> > Yours,
> > Linus Walleij



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

Re: [RFC 2/5] dt-bindings: pinctrl: add pic64gx "gpio2" pinmux

[Linus Walleij]

On Wed, Oct 1, 2025 at 5:47 PM Conor Dooley <conor@kernel.org> wrote:

> tbh, I found it hard to understand the "line" between using a pinmux
> property and where stuff should be described in groups or functions in a
> driver. What is that line?

There is no such line, basically what I like as pin control maintainer
is what exists in the documentation with groups and functions.

Then various driver maintainers have pushed me around since
day 1 because they think it is much more convenient to just
have some single value to poke into a register.

I have come to accept both because the discussions just
go on forever. I'm not a very stern person, "those are my
principles, if you don't like them, I have others".

Essentially it is a question about what the device tree is for:
is it just for (outline) description and configuration of hardware
for a specific system, i.e. where everything that is not
system-specific should be encoded into the driver, or is it
for dumping all kind of various SoC-specific stuff into, without
abstraction. There is no clear line there either, and that is
part of the problem here.

Yours,
Linus Walleij

Re: [RFC 3/5] pinctrl: add polarfire soc iomux0 pinmux driver

[Linus Walleij]

On Wed, Oct 1, 2025 at 5:45 PM Conor Dooley <conor@kernel.org> wrote:

> They're not actually package pins at all that are being configured here,
> it's internal routing inside the FPGA. It does operate on a function
> level, but I don't think there's a neat mapping to the pinctrl subsystem
> which (AFAIU) considers functions to contain groups, which in turn
> contain pins. I suppose it could be thought of that, for example, spi0
> is actually a function containing 4 (or 5, don't ask - or do if you want
> to read a rant about pointlessly confusing design) "pins" in 1 group.
>
> If I could just work in terms of functions only, and avoid groups or
> pins at all, feels (to me ofc) like it'd maybe match the purpose of this
> aspect of the hardware better.

What I would ask myself is whether the abstraction fits the bill,
like if there is a natural set of functions in the silicon, then the code
should reflect that.

When it comes to those:

+static const struct pinctrl_pin_desc mpfs_iomux0_pinctrl_pins[] = {
+       PINCTRL_PIN(0, "spi0"),
+       PINCTRL_PIN(1, "spi1"),

At least be careful with the nouns used: are they really "pins"?
Should they be described as "pins"?

Maybe it is best to come up with some custom struct if not?

Yours,
Linus Walleij

Re: [RFC 2/5] dt-bindings: pinctrl: add pic64gx "gpio2" pinmux

[Conor Dooley]

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

On Mon, Oct 13, 2025 at 12:56:42PM +0200, Linus Walleij wrote:
> On Wed, Oct 1, 2025 at 5:47 PM Conor Dooley <conor@kernel.org> wrote:
> 
> > tbh, I found it hard to understand the "line" between using a pinmux
> > property and where stuff should be described in groups or functions in a
> > driver. What is that line?
> 
> There is no such line, basically what I like as pin control maintainer
> is what exists in the documentation with groups and functions.
> 
> Then various driver maintainers have pushed me around since
> day 1 because they think it is much more convenient to just
> have some single value to poke into a register.
> 
> I have come to accept both because the discussions just
> go on forever. I'm not a very stern person, "those are my
> principles, if you don't like them, I have others".

Right, I see. Currently I have 3 drivers, two being what are here. Both
of those I have converted to use functions and groups. The third retains
the pinmux property, mostly because of the number of functions that each
pin can be routed due to being an FPGA. I'll send the first two in the
coming day or two and see what to do about the third. It's got much more
going on and no internal pressure to get it working, unlike these the
first two that folks have expressed a need for.

> Essentially it is a question about what the device tree is for:
> is it just for (outline) description and configuration of hardware
> for a specific system, i.e. where everything that is not
> system-specific should be encoded into the driver, or is it
> for dumping all kind of various SoC-specific stuff into, without
> abstraction. There is no clear line there either, and that is
> part of the problem here.

Now that I have some understanding about how this stuff works, I can
start whinging about people using these pinmux properties if you want.
I'm probably biased by my own laziness and not wanting to write out
dozens and dozens of groups etc, but in cases where there's only two or
three functions per pin, using functions/groups seems like the way to
go..

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

Re: [RFC 3/5] pinctrl: add polarfire soc iomux0 pinmux driver

[Conor Dooley]

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

On Mon, Oct 13, 2025 at 01:02:35PM +0200, Linus Walleij wrote:
> On Wed, Oct 1, 2025 at 5:45 PM Conor Dooley <conor@kernel.org> wrote:
> 
> > They're not actually package pins at all that are being configured here,
> > it's internal routing inside the FPGA. It does operate on a function
> > level, but I don't think there's a neat mapping to the pinctrl subsystem
> > which (AFAIU) considers functions to contain groups, which in turn
> > contain pins. I suppose it could be thought of that, for example, spi0
> > is actually a function containing 4 (or 5, don't ask - or do if you want
> > to read a rant about pointlessly confusing design) "pins" in 1 group.
> >
> > If I could just work in terms of functions only, and avoid groups or
> > pins at all, feels (to me ofc) like it'd maybe match the purpose of this
> > aspect of the hardware better.
> 
> What I would ask myself is whether the abstraction fits the bill,
> like if there is a natural set of functions in the silicon, then the code
> should reflect that.
> 
> When it comes to those:
> 
> +static const struct pinctrl_pin_desc mpfs_iomux0_pinctrl_pins[] = {
> +       PINCTRL_PIN(0, "spi0"),
> +       PINCTRL_PIN(1, "spi1"),
> 
> At least be careful with the nouns used: are they really "pins"?
> Should they be described as "pins"?

I think that my choices if talking to someone outside of the context of
the structure of the pinctrl subsystem would be functions (for what's in
the driver as pins) and routings (instead of groups). pinctrl's function
doesn't really do very much in this context, although the devicetree
function and groups I think actually work fairly well: "function ABC is
assigned to pin 1".
Regarding nouns, I think it depends on how far you go with that...

> Maybe it is best to come up with some custom struct if not?

...because taking that to an extreme means forsaking a lot (all) of the
common pinctrl infrastructure, no? If it's just choosing more apt names
for variables/functions in the driver or redefining PINCTRL_PIN to be
something more suitably named, then sure. But if I have to avoid using
pinctrl_pin_desc et al, am I going to lose out on a bunch of useful
common code?

I'll send my v2 on tomorrow probably, and we can talk about what exact
changes should be made there?

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

Re: [RFC 0/5] microchip mpfs/pic64gx pinctrl questions

[Linus Walleij]

On Thu, Oct 9, 2025 at 5:55 PM Conor Dooley <conor@kernel.org> wrote:

> So, what I ended up doing is moving the "gpio2" stuff to use
> functions/groups as your gemini stuff does, so each function contains
> one group containing all the pins it needs - except for the gpio
> function which contains analogues for each of the function's groups.

I don't know exactly what you mean by this, but if it entails any
entanglement of the GPIO function with another function, then
there is the recent patch from Bartosz in commit
11aa02d6a9c222260490f952d041dec6d7f16a92
which makes it possible to give the pin control framework
an awareness of what a GPIO function is by reading hardware
properties, and that it is sometimes separate from other functions.

Yours,
Linus Walleij

Re: [RFC 0/5] microchip mpfs/pic64gx pinctrl questions

[Conor Dooley]

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

On Mon, Oct 13, 2025 at 03:27:57PM +0200, Linus Walleij wrote:
> On Thu, Oct 9, 2025 at 5:55 PM Conor Dooley <conor@kernel.org> wrote:
> 
> > So, what I ended up doing is moving the "gpio2" stuff to use
> > functions/groups as your gemini stuff does, so each function contains
> > one group containing all the pins it needs - except for the gpio
> > function which contains analogues for each of the function's groups.
> 
> I don't know exactly what you mean by this, but if it entails any

All I meant is that the functions for non-gpio things contain a group
with the pins they need, up to 10 groups for 10 non-gpio functions, and
that the gpio function, since each pin can do gpio and exactly one other
function, contains 10 groups, all of which are identical to a group
already defined for the non-gpio function. That's instead of having one
huge group with all 32 pins.

> entanglement of the GPIO function with another function, then
> there is the recent patch from Bartosz in commit
> 11aa02d6a9c222260490f952d041dec6d7f16a92
> which makes it possible to give the pin control framework
> an awareness of what a GPIO function is by reading hardware
> properties, and that it is sometimes separate from other functions.

That is unrelated, but interesting. What I don't really understand from
the commit message itself is whether this is useful if the pinctrl
driver is not also acting as a gpiochip driver. In my case, the pinctrl
hardware is not capable of doing anything more than muxing functions,
and the gpio function I talk about means routing a "real" gpio
controller's IO to the pins controlled by the driver I am talking about.
The 2 in "gpio 2" refers to the specific controller.
The rest of that thread makes it seem like this is intended for some
qcom devices where the pinctrl hardware is also a gpiochip.

Cheers,
Conor.

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

Re: [RFC 3/5] pinctrl: add polarfire soc iomux0 pinmux driver

[Linus Walleij]

On Mon, Oct 13, 2025 at 1:42 PM Conor Dooley <conor@kernel.org> wrote:

> I'll send my v2 on tomorrow probably, and we can talk about what exact
> changes should be made there?

Sounds like a deal, I try to go with the IETF motto "rough consensus and
running code" when it comes to pin control.

Yours,
Linus Walleij

Re: [RFC 0/5] microchip mpfs/pic64gx pinctrl questions

[Linus Walleij]

On Mon, Oct 13, 2025 at 3:55 PM Conor Dooley <conor@kernel.org> wrote:

[me]
> > entanglement of the GPIO function with another function, then
> > there is the recent patch from Bartosz in commit
> > 11aa02d6a9c222260490f952d041dec6d7f16a92
> > which makes it possible to give the pin control framework
> > an awareness of what a GPIO function is by reading hardware
> > properties, and that it is sometimes separate from other functions.
>
> That is unrelated, but interesting. What I don't really understand from
> the commit message itself is whether this is useful if the pinctrl
> driver is not also acting as a gpiochip driver. In my case, the pinctrl
> hardware is not capable of doing anything more than muxing functions,
> and the gpio function I talk about means routing a "real" gpio
> controller's IO to the pins controlled by the driver I am talking about.
> The 2 in "gpio 2" refers to the specific controller.
> The rest of that thread makes it seem like this is intended for some
> qcom devices where the pinctrl hardware is also a gpiochip.

It's useful if you want to use the .strict setting on the pin
controller and implement the shortcut GPIO enablement functions
such as .gpio_request_enable, .gpio_disable_free
and .gpio_set_direction.

These are often preferred when using the pin control driver
as a "back-end" for a GPIO "front-end".

Yours,
Linus Walleij