[RFC v1 0/4] Microchip mpfs/pic64gx pinctrl part 2

[RFC v1 0/4] Microchip mpfs/pic64gx pinctrl part 2

[Conor Dooley]

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

Hey Linus,

Got the other driver that I was talking about here for you...
It's in RFC state because I'd like to get some feedback on the approach
while I try to figure out a) what ibufmd is and b) how the bank voltage
interacts with the schmitt trigger setting. Although, I am pretty sure
for the latter that it is not forced on for low voltages and that the
commented code should be deleted.

There's some specific @Linus questions in the driver, mostly where I was
unsure about how config bits should be handled and looking around at
other drivers wasn't sufficient because they did different things.

Finally, on the dt side, this was using the pinmux property before the
other drivers were submitted, but I didn't really like it to begin with
(shoving two things into entries of the same property gives me the ick).
I moved to using pins + function which at least looks prettier in the
devicetree. I had been hoping that I could move to some sort of generic
dt_node_to_map function, but I couldn't figure out one that'd work
without creating groups in the driver. If there is, I'd love to get rid
of the custom dt_node_to_map stuff.
I want to avoid doing having set groups, because of the number of
possible configurations in the MSS Configurator FPGA tool is fairly
large, and I believe that MSS Configurator actually undersells the
number of possible combinations for ease of use. I haven't tested that
and the driver technically doesn't support it, but I feel like not trying
to define groups statically and using pins instead would permit those
combos in the future should that use case ever materialise.

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
CC: Valentina.FernandezAlanis@microchip.com

Conor Dooley (4):
  dt-bindings: pinctrl: document polarfire soc mssio pin controller
  pinctrl: add polarfire soc mssio pinctrl driver
  MAINTAINERS: add Microchip mpfs mssio driver/bindings to entry
  riscv: dts: microchip: add pinctrl nodes for mpfs/icicle kit

 .../pinctrl/microchip,mpfs-pinctrl-mssio.yaml | 108 +++
 .../microchip,mpfs-mss-top-sysreg.yaml        |   4 +
 MAINTAINERS                                   |   2 +
 .../dts/microchip/mpfs-icicle-kit-common.dtsi |   1 -
 .../dts/microchip/mpfs-icicle-kit-fabric.dtsi |  63 ++
 .../boot/dts/microchip/mpfs-pinctrl.dtsi      | 165 ++++
 arch/riscv/boot/dts/microchip/mpfs.dtsi       |  16 +
 drivers/pinctrl/Kconfig                       |   5 +-
 drivers/pinctrl/Makefile                      |   1 +
 drivers/pinctrl/pinctrl-mpfs-mssio.c          | 798 ++++++++++++++++++
 10 files changed, 1161 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/pinctrl/microchip,mpfs-pinctrl-mssio.yaml
 create mode 100644 arch/riscv/boot/dts/microchip/mpfs-pinctrl.dtsi
 create mode 100644 drivers/pinctrl/pinctrl-mpfs-mssio.c

-- 
2.51.0

[RFC v1 1/4] dt-bindings: pinctrl: document polarfire soc mssio pin controller

[Conor Dooley]

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

On Polarfire SoC, the Bank 2 and Bank 4 IOs connected to the
Multiprocessor Subsystem (MSS) are controlled by IOMUX_CRs 1 through 6,
which determine what function in routed to them, and
MSSIO_BANK#_IO_CFG_CRs, which determine the configuration of each pin.

Document it, including several custom configuration options that stem
from MSS Configurator options (the MSS Configurator is part of the FPGA
tooling for this device). "ibufmd" unfortunately is not a 1:1 mapping
with an MSS Configurator option, unlike clamp-diode or lockdown, and I
do not know the effect of any bits in the field. I have no been able to
find an explanation for these bits in documentation.

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

diff --git a/Documentation/devicetree/bindings/pinctrl/microchip,mpfs-pinctrl-mssio.yaml b/Documentation/devicetree/bindings/pinctrl/microchip,mpfs-pinctrl-mssio.yaml
new file mode 100644
index 000000000000..32d7a31d669f
--- /dev/null
+++ b/Documentation/devicetree/bindings/pinctrl/microchip,mpfs-pinctrl-mssio.yaml
@@ -0,0 +1,108 @@
+# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/pinctrl/microchip,mpfs-pinctrl-mssio.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip Polarfire SoC MSSIO pinctrl
+
+maintainers:
+  - Conor Dooley <conor.dooley@microchip.com>
+
+properties:
+  compatible:
+    const: microchip,mpfs-pinctrl-mssio
+
+  reg:
+    maxItems: 1
+
+  pinctrl-use-default: true
+
+patternProperties:
+  '-cfg$':
+    type: object
+    additionalProperties: false
+
+    patternProperties:
+      '-pins$':
+        type: object
+        additionalProperties: false
+
+        allOf:
+          - $ref: pincfg-node.yaml#
+          - $ref: pinmux-node.yaml#
+
+        properties:
+          pins:
+            description: |
+              The list of IOs that properties in the pincfg node apply to.
+
+          function:
+            description:
+              A string containing the name of the function to mux for these
+              pins. The "reserved" function tristates a pin.
+            enum: [ sd, emmc, qspi, spi, usb, uart, i2c, can, mdio, misc
+                    reserved, gpio, fabric-test, tied-low, tied-high, tristate ]
+
+          bias-bus-hold: true
+          bias-disable: true
+          bias-pull-down: true
+          bias-pull-up: true
+          input-schmitt-enable: true
+          low-power-enable: true
+
+          drive-strength:
+            enum: [ 2, 4, 6, 8, 10, 12, 16, 20 ]
+
+          microchip,clamp-diode:
+            $ref: /schemas/types.yaml#/definitions/flag
+            description: |
+              Reflects the "Clamp Diode" setting in the MSS Configurator for
+              this pin.
+
+          microchip,bank-lockdown:
+            $ref: /schemas/types.yaml#/definitions/flag
+            description: |
+              Reflects the "Lock Down Bank{2,4} I/Os" setting in the MSS
+              Configurator for this pin.
+
+          microchip,ibufmd:
+            $ref: /schemas/types.yaml#/definitions/uint32
+            default: 0
+            description: |
+              Reflects the "IBUFMD" bits in the MSS Configurator output files
+              for this pin.
+
+        required:
+          - pins
+          - function
+
+required:
+  - compatible
+  - reg
+
+additionalProperties: false
+
+examples:
+  - |
+    pinctrl@204 {
+      compatible = "microchip,mpfs-pinctrl-mssio";
+      reg = <0x204 0x7c>;
+
+      irkd_sd_cfg: irkd-sd-cfg {
+        sd-10ma-pins {
+          pins = <0>, <1>, <2>, <3>, <4>, <5>, <8>, <9>, <10>, <11>, <12>, <13>;
+          function = "sd";
+          bias-pull-up;
+          drive-strength = <10>;
+        };
+
+        sd-8ma-pins {
+          pins = <6>, <7>;
+          function = "sd";
+          bias-pull-up;
+          drive-strength = <8>;
+        };
+      };
+    };
+...
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 39987f722411..44e4a50c3155 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
@@ -42,6 +42,10 @@ properties:
     type: object
     $ref: /schemas/pinctrl/microchip,mpfs-pinctrl-iomux0.yaml
 
+  pinctrl@204:
+    type: object
+    $ref: /schemas/pinctrl/microchip,mpfs-pinctrl-mssio.yaml
+
 required:
   - compatible
   - reg
-- 
2.51.0

Re: [RFC v1 1/4] dt-bindings: pinctrl: document polarfire soc mssio pin controller

[Linus Walleij]

Hi Conor,

On Wed, Nov 12, 2025 at 3:33 PM Conor Dooley <conor@kernel.org> wrote:

> +examples:
> +  - |
> +    pinctrl@204 {
> +      compatible = "microchip,mpfs-pinctrl-mssio";
> +      reg = <0x204 0x7c>;
> +
> +      irkd_sd_cfg: irkd-sd-cfg {
> +        sd-10ma-pins {
> +          pins = <0>, <1>, <2>, <3>, <4>, <5>, <8>, <9>, <10>, <11>, <12>, <13>;
> +          function = "sd";
> +          bias-pull-up;
> +          drive-strength = <10>;
> +        };
> +
> +        sd-8ma-pins {
> +          pins = <6>, <7>;
> +          function = "sd";
> +          bias-pull-up;
> +          drive-strength = <8>;
> +        };
> +      };
> +    };

This looks good to me and you sure know how to do syntax
so I don't even need to check that part.

These bindings are fine.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

[RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Conor Dooley]

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

On Polarfire SoC, the Bank 2 and Bank 4 IOs connected to the
Multiprocessor Subsystem (MSS) are controlled by IOMUX_CRs 1 through 6,
which determine what function in routed to them, and
MSSIO_BANK#_IO_CFG_CRs, which determine the configuration of each pin.

Add a driver for this pin controller, including several custom
properties that reflect aspects of the MSS's configuration.

Reuse the Kconfig option for iomux0, since controlling MSSIOs without
iomux0 routing a function to the MSSIOs in question is pointless, and
routing a function to the MSSIOs is equally unhelpful if none of them
are configured to make use of that function.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 drivers/pinctrl/Kconfig              |   5 +-
 drivers/pinctrl/Makefile             |   1 +
 drivers/pinctrl/pinctrl-mpfs-mssio.c | 798 +++++++++++++++++++++++++++
 3 files changed, 803 insertions(+), 1 deletion(-)
 create mode 100644 drivers/pinctrl/pinctrl-mpfs-mssio.c

diff --git a/drivers/pinctrl/Kconfig b/drivers/pinctrl/Kconfig
index 4ec2bb7f67cf..437616e5a6d5 100644
--- a/drivers/pinctrl/Kconfig
+++ b/drivers/pinctrl/Kconfig
@@ -506,9 +506,12 @@ config PINCTRL_PISTACHIO
 	  This support pinctrl and GPIO driver for IMG Pistachio SoC.
 
 config PINCTRL_POLARFIRE_SOC
-	bool "Polarfire SoC pinctrl driver"
+	bool "Polarfire SoC pinctrl drivers"
 	depends on ARCH_MICROCHIP || COMPILE_TEST
+	select PINMUX
 	select GENERIC_PINCONF
+	select GENERIC_PINCTRL_GROUPS
+	select GENERIC_PINMUX_FUNCTIONS
 	default y
 	help
 	  This selects the pinctrl driver for Microchip Polarfire SoC.
diff --git a/drivers/pinctrl/Makefile b/drivers/pinctrl/Makefile
index ea4e890766e1..bf181654fe7f 100644
--- a/drivers/pinctrl/Makefile
+++ b/drivers/pinctrl/Makefile
@@ -50,6 +50,7 @@ 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-mssio.o
 obj-$(CONFIG_PINCTRL_POLARFIRE_SOC)	+= pinctrl-mpfs-iomux0.o
 obj-$(CONFIG_PINCTRL_RK805)	+= pinctrl-rk805.o
 obj-$(CONFIG_PINCTRL_ROCKCHIP)	+= pinctrl-rockchip.o
diff --git a/drivers/pinctrl/pinctrl-mpfs-mssio.c b/drivers/pinctrl/pinctrl-mpfs-mssio.c
new file mode 100644
index 000000000000..e1b963350016
--- /dev/null
+++ b/drivers/pinctrl/pinctrl-mpfs-mssio.c
@@ -0,0 +1,798 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include "linux/dev_printk.h"
+#include <linux/bitfield.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_PAD_MUX_MASK GENMASK(3, 0)
+
+#define MPFS_PINCTRL_IOCFG_MASK GENMASK(14, 0)
+#define MPFS_PINCTRL_IBUFMD_MASK GENMASK(2, 0)
+#define MPFS_PINCTRL_DRV_MASK GENMASK(6, 3)
+#define MPFS_PINCTRL_CLAMP BIT(7)
+#define MPFS_PINCTRL_ENHYST BIT(8)
+#define MPFS_PINCTRL_LOCKDN BIT(9)
+#define MPFS_PINCTRL_WPD BIT(10)
+#define MPFS_PINCTRL_WPU BIT(11)
+#define MPFS_PINCTRL_PULL_MASK GENMASK(11, 10)
+#define MPFS_PINCTRL_LP_PERSIST_EN BIT(12)
+#define MPFS_PINCTRL_LP_BYPASS_EN BIT(13)
+
+#define MPFS_PINCTRL_LVCMOS25 0x6
+#define MPFS_PINCTRL_BANK_VOLTAGE_MASK GENMASK(19, 16)
+
+#define MPFS_PINCTRL_BANK2_CFG_CR 0x1c4
+#define MPFS_PINCTRL_BANK4_CFG_CR 0x1c8
+#define MPFS_PINCTRL_IOCFG01_REG 0x234
+
+#define MPFS_PINCTRL_INTER_BANK_GAP 0x4
+
+#define MPFS_PINCTRL_BANK2_START 14
+
+#define MPFS_PINCTRL_LOCKDOWN (PIN_CONFIG_END + 1)
+#define MPFS_PINCTRL_CLAMP_DIODE (PIN_CONFIG_END + 2)
+#define MPFS_PINCTRL_IBUFMD (PIN_CONFIG_END + 3)
+
+struct mpfs_pinctrl_mux_config {
+	u8 pin;
+	u8 function;
+};
+
+struct mpfs_pinctrl {
+	struct pinctrl_dev *pctrl;
+	struct device *dev;
+	struct regmap *regmap;
+	struct mutex mutex;
+	struct pinctrl_desc desc;
+	u32 bank2_voltage;
+	u32 bank4_voltage;
+};
+
+struct mpfs_pinctrl_drive_strength {
+	u8 ma;
+	u8 val;
+};
+
+static struct mpfs_pinctrl_drive_strength mpfs_pinctrl_drive_strengths[8] = {
+	{ 2,   2 },
+	{ 4,   3 },
+	{ 6,   4 },
+	{ 8,   5 },
+	{ 10,  6 },
+	{ 12,  7 },
+	{ 16, 10 },
+	{ 20, 12 },
+};
+
+static char *mpfs_pinctrl_function_names[] = {
+	"sd",
+	"emmc",
+	"qspi",
+	"spi",
+	"usb",
+	"uart",
+	"i2c",
+	"can",
+	"mdio",
+	"misc",
+	"reserved",
+	"gpio",
+	"fabric test",
+	"tied-low",
+	"tied-high",
+	"tristate"
+};
+
+static int mpfs_pinctrl_function_map(const char *function)
+{
+	size_t num = ARRAY_SIZE(mpfs_pinctrl_function_names);
+
+	for (int i = 0; i < num; i++)
+		if (!strcmp(function, mpfs_pinctrl_function_names[i]))
+			return i;
+
+	return -EINVAL;
+}
+
+static const struct pinconf_generic_params mpfs_pinctrl_custom_bindings[] = {
+	{ "microchip,bank-lockdown", MPFS_PINCTRL_LOCKDOWN, 1 },
+	{ "microchip,clamp-diode", MPFS_PINCTRL_CLAMP_DIODE, 1 },
+	{ "microchip,ibufmd", MPFS_PINCTRL_IBUFMD, 0x0 },
+};
+
+static int mpfs_pinctrl_pin_to_iomux_offset(unsigned int pin)
+{
+	int offset;
+
+	switch (pin) {
+	case 0 ... 7:
+		offset = pin * 4;
+		break;
+	case 8 ... 13:
+		offset = (pin - 8) * 4;
+		break;
+	case 14 ... 21:
+		offset = (pin - 14) * 4;
+		break;
+	case 22 ... 29:
+		offset = (pin - 22) * 4;
+		break;
+	case 30 ... 37:
+		offset = (pin - 30) * 4;
+		break;
+	default:
+		offset = -EINVAL;
+	}
+
+	return offset;
+}
+
+static int mpfs_pinctrl_pin_to_iomux_reg(unsigned int pin)
+{
+	int reg;
+
+	switch (pin) {
+	case 0 ... 7:
+		reg = 0x204;
+		break;
+	case 8 ... 13:
+		reg = 0x208;
+		break;
+	case 14 ... 21:
+		reg = 0x20c;
+		break;
+	case 22 ... 29:
+		reg = 0x210;
+		break;
+	case 30 ... 37:
+		reg = 0x214;
+		break;
+	default:
+		reg = -EINVAL;
+	}
+
+	return reg;
+}
+
+static int mpfs_pinctrl_pin_to_iocfg_reg(unsigned int pin)
+{
+	u32 reg = MPFS_PINCTRL_IOCFG01_REG;
+
+	if (pin >= MPFS_PINCTRL_BANK2_START)
+		reg += MPFS_PINCTRL_INTER_BANK_GAP;
+
+	// 2 pins per 32-bit register
+	reg += (pin / 2) * 0x4;
+
+	return reg;
+}
+
+static int mpfs_pinctrl_pin_to_iocfg_offset(unsigned int pin)
+{
+	return 16 * (pin % 2);
+}
+
+static u32 mpfs_pinctrl_pin_to_bank_voltage(struct mpfs_pinctrl *pctrl, unsigned int pin)
+{
+	u32 bank_voltage;
+
+	if (pin < MPFS_PINCTRL_BANK2_START)
+		bank_voltage = pctrl->bank4_voltage;
+	else
+		bank_voltage = pctrl->bank2_voltage;
+
+	return FIELD_GET(MPFS_PINCTRL_BANK_VOLTAGE_MASK, bank_voltage);
+}
+
+static void mpfs_pinctrl_dbg_show(struct pinctrl_dev *pctrl_dev, struct seq_file *seq,
+				  unsigned int pin)
+{
+	struct mpfs_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
+	u32 func;
+	int reg, offset;
+
+	reg = mpfs_pinctrl_pin_to_iomux_reg(pin);
+	offset = mpfs_pinctrl_pin_to_iomux_offset(pin);
+
+	seq_printf(seq, "reg: %x, offset: %u ", reg, offset);
+	seq_printf(seq, "pin: %u ", pin);
+
+	if (reg < 0 || offset < 0)
+		return;
+
+	regmap_read(pctrl->regmap, reg, &func);
+	func = (func >> offset) & MPFS_PINCTRL_PAD_MUX_MASK;
+	seq_printf(seq, "func: %s (%x)\n", mpfs_pinctrl_function_names[func], func);
+}
+
+static int mpfs_pinctrl_dt_node_to_map(struct pinctrl_dev *pctrl_dev, struct device_node *np,
+				       struct pinctrl_map **maps, unsigned int *num_maps)
+{
+	struct mpfs_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
+	struct device *dev = pctrl->dev;
+	struct device_node *child;
+	struct pinctrl_map *map;
+	const char **group_names;
+	const char *group_name;
+	int ngroups = 0;
+	int nmaps = 0;
+	int ret;
+
+	for_each_available_child_of_node(np, child)
+		ngroups += 1;
+
+	group_names = devm_kcalloc(dev, ngroups, sizeof(*group_names), GFP_KERNEL);
+	if (!group_names)
+		return -ENOMEM;
+
+	map = kcalloc(ngroups * 2, sizeof(*map), GFP_KERNEL);
+	if (!map)
+		return -ENOMEM;
+
+	ngroups = 0;
+	guard(mutex)(&pctrl->mutex);
+	for_each_available_child_of_node_scoped(np, child) {
+		struct mpfs_pinctrl_mux_config *pinmuxs;
+		const char *function_name;
+		unsigned int pin, *pins;
+		int function, npins;
+
+		npins = of_property_count_u32_elems(child, "pins");
+
+		if (npins < 1) {
+			dev_err(dev, "invalid pinctrl group %pOFn.%pOFn %d\n",
+				np, child, npins);
+			return npins;
+		}
+
+		group_name = devm_kasprintf(dev, GFP_KERNEL, "%pOFn.%pOFn",
+					 np, child);
+		if (!group_name)
+			return -ENOMEM;
+
+		group_names[ngroups++] = group_name;
+
+		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 (int i = 0; i < npins; i++) {
+			ret = of_property_read_u32_index(child, "pins", i, &pin);
+			if (ret)
+				return ret;
+
+			pins[i] = pin;
+			pinmuxs[i].pin = pin;
+
+			ret = of_property_read_string(child, "function", &function_name);
+			function = mpfs_pinctrl_function_map(function_name);
+			if (function < 0)
+				return dev_err_probe(dev, function, "invalid function %s\n",
+						     function_name);
+			pinmuxs[i].function = function;
+		}
+
+		map[nmaps].type = PIN_MAP_TYPE_MUX_GROUP;
+		map[nmaps].data.mux.function = np->name;
+		map[nmaps].data.mux.group = group_name;
+		nmaps += 1;
+
+		ret = pinctrl_generic_add_group(pctrl_dev, group_name, pins, npins, pinmuxs);
+		if (ret < 0)
+			return dev_err_probe(dev, ret, "failed to add group %s: %d\n",
+					     group_name, ret);
+
+		ret = pinconf_generic_parse_dt_config(child, pctrl_dev,
+						      &map[nmaps].data.configs.configs,
+						      &map[nmaps].data.configs.num_configs);
+		if (ret)
+			return dev_err_probe(dev, ret, "failed to parse pin config of group %s\n",
+				group_name);
+
+		if (map[nmaps].data.configs.num_configs == 0)
+			continue;
+
+		map[nmaps].type = PIN_MAP_TYPE_CONFIGS_GROUP;
+		map[nmaps].data.configs.group_or_pin = group_name;
+		nmaps += 1;
+	}
+
+	ret = pinmux_generic_add_function(pctrl_dev, np->name, group_names, ngroups, NULL);
+	if (ret < 0) {
+		pinctrl_utils_free_map(pctrl_dev, map, nmaps);
+		return dev_err_probe(dev, ret, "error adding function %s\n", np->name);
+	}
+
+	*maps = map;
+	*num_maps = nmaps;
+
+	return 0;
+};
+
+static const struct pinctrl_ops mpfs_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_pinctrl_dbg_show,
+	.dt_node_to_map = mpfs_pinctrl_dt_node_to_map,
+	.dt_free_map = pinctrl_utils_free_map,
+};
+
+static int mpfs_pinctrl_set_pin_func(struct mpfs_pinctrl *pctrl, u8 pin, u8 function)
+{
+	struct device *dev = pctrl->dev;
+	u32 reg, func, mask, offset;
+
+	reg = mpfs_pinctrl_pin_to_iomux_reg(pin);
+	offset = mpfs_pinctrl_pin_to_iomux_offset(pin);
+
+	func = function << offset;
+	mask = MPFS_PINCTRL_PAD_MUX_MASK << offset;
+
+	dev_dbg(dev, "Setting pin %u. reg: %x offset %u func %x\n", pin, reg, offset, func);
+
+	if (reg < 0 || offset < 0)
+		return -EINVAL;
+
+	regmap_update_bits(pctrl->regmap, reg, mask, func);
+
+	return 0;
+}
+
+static int mpfs_pinctrl_set_mux(struct pinctrl_dev *pctrl_dev, unsigned int fsel,
+				unsigned int gsel)
+{
+	struct mpfs_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
+	const struct group_desc *group;
+	struct mpfs_pinctrl_mux_config *configs;
+
+	group = pinctrl_generic_get_group(pctrl_dev, gsel);
+	if (!group)
+		return -EINVAL;
+
+	configs = group->data;
+
+	for (int i = 0; i < group->grp.npins; i++)
+		mpfs_pinctrl_set_pin_func(pctrl, configs[i].pin, configs[i].function);
+
+	return 0;
+}
+
+static const struct pinmux_ops mpfs_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_pinctrl_set_mux,
+};
+
+static int mpfs_pinctrl_get_drive_strength_ma(u32 drive_strength)
+{
+	size_t num = ARRAY_SIZE(mpfs_pinctrl_drive_strengths);
+
+	for (int i = 0; i < num; i++)
+		if (drive_strength == mpfs_pinctrl_drive_strengths[i].val)
+			return mpfs_pinctrl_drive_strengths[i].ma;
+
+	return -EINVAL;
+}
+
+static int mpfs_pinctrl_get_drive_strength_val(u32 drive_strength_ma)
+{
+	size_t num = ARRAY_SIZE(mpfs_pinctrl_drive_strengths);
+
+	if (!drive_strength_ma)
+		return -EINVAL;
+
+	for (int i = 0; i < num; i++)
+		if (drive_strength_ma <= mpfs_pinctrl_drive_strengths[i].ma)
+			return mpfs_pinctrl_drive_strengths[i].val;
+
+	return mpfs_pinctrl_drive_strengths[num - 1].val;
+}
+
+static int mpfs_pinctrl_pinconf_get(struct pinctrl_dev *pctrl_dev, unsigned int pin,
+				    unsigned long *config)
+{
+	struct mpfs_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
+	int param = pinconf_to_config_param(*config);
+	int reg = mpfs_pinctrl_pin_to_iocfg_reg(pin);
+	int val;
+	u32 arg;
+	//TODO bank_voltage;
+	u8 str;
+
+	regmap_read(pctrl->regmap, reg, &val);
+
+	val = val >> mpfs_pinctrl_pin_to_iocfg_offset(pin);
+	val = val & MPFS_PINCTRL_IOCFG_MASK;
+
+	switch (param) {
+	case PIN_CONFIG_BIAS_BUS_HOLD:
+		if (!(val & MPFS_PINCTRL_WPD))
+			return -EINVAL;
+
+		if (!(val & MPFS_PINCTRL_WPU))
+			return -EINVAL;
+
+		arg = 1;
+		break;
+	case PIN_CONFIG_BIAS_PULL_DOWN:
+		if (!(val & MPFS_PINCTRL_WPD))
+			return -EINVAL;
+
+		if (val & MPFS_PINCTRL_WPU)
+			return -EINVAL;
+
+		arg = 1;
+		break;
+	case PIN_CONFIG_BIAS_PULL_UP:
+		if (!(val & MPFS_PINCTRL_WPU))
+			return -EINVAL;
+
+		if (val & MPFS_PINCTRL_WPD)
+			return -EINVAL;
+
+		arg = 1;
+		break;
+	case PIN_CONFIG_BIAS_DISABLE:
+		if (val & MPFS_PINCTRL_PULL_MASK)
+			return -EINVAL;
+
+		arg = 1;
+		break;
+	case PIN_CONFIG_DRIVE_STRENGTH:
+		str = FIELD_GET(MPFS_PINCTRL_DRV_MASK, val);
+		if (!str)
+			return -EINVAL;
+
+		arg = mpfs_pinctrl_get_drive_strength_ma(str);
+		break;
+	//TODO @Linus, it correct to group these 3? There's no control over voltage.
+	case PIN_CONFIG_INPUT_SCHMITT:
+	case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
+	case PIN_CONFIG_INPUT_SCHMITT_UV:
+		//TODO Is it enabled regardless of register setting, or must it
+		// be set for lower voltage IO? Docs are missing, MSS Configurator
+		// is not clear. Leaning towards the latter.
+		//bank_voltage = mpfs_pinctrl_pin_to_bank_voltage(pctrl, pin);
+		//if (bank_voltage < MPFS_PINCTRL_LVCMOS25) {
+		//	arg = 1;
+		//	break;
+		//}
+
+		if (!FIELD_GET(MPFS_PINCTRL_ENHYST, val))
+			return -EINVAL;
+
+		arg = 1;
+		break;
+	case PIN_CONFIG_PERSIST_STATE:
+		if (!FIELD_GET(MPFS_PINCTRL_LP_PERSIST_EN, val))
+			return -EINVAL;
+
+		arg = 1;
+		break;
+	case PIN_CONFIG_MODE_LOW_POWER:
+		if (!FIELD_GET(MPFS_PINCTRL_LP_BYPASS_EN, val))
+			return -EINVAL;
+
+		arg = 1;
+		break;
+	case MPFS_PINCTRL_CLAMP_DIODE:
+		if (!FIELD_GET(MPFS_PINCTRL_CLAMP, val))
+			return -EINVAL;
+
+		arg = 1;
+		break;
+	case MPFS_PINCTRL_LOCKDOWN:
+		if (!FIELD_GET(MPFS_PINCTRL_LOCKDN, val))
+			return -EINVAL;
+
+		arg = 1;
+		break;
+	case MPFS_PINCTRL_IBUFMD:
+		arg = FIELD_GET(MPFS_PINCTRL_IBUFMD_MASK, val);
+		break;
+	default:
+		return -ENOTSUPP;
+	}
+
+	*config = pinconf_to_config_packed(param, arg);
+
+	return 0;
+}
+
+static int mpfs_pinctrl_pinconf_generate_config(struct mpfs_pinctrl *pctrl, unsigned int pin,
+						unsigned long *configs, unsigned int num_configs,
+						u32 *value)
+{
+	u32 val = 0;
+
+	for (int i = 0; i < num_configs; i++) {
+		int param, tmp;
+		u32 arg;
+		//TODO bank_voltage;
+
+		param = pinconf_to_config_param(configs[i]);
+		arg = pinconf_to_config_argument(configs[i]);
+
+		switch (param) {
+		case PIN_CONFIG_BIAS_BUS_HOLD:
+			val |= MPFS_PINCTRL_PULL_MASK;
+			break;
+		case PIN_CONFIG_BIAS_PULL_DOWN:
+			//TODO always start from val == 0, there's no reason to ever actually
+			// clear anything AFAICT. @Linus, does the driver need to check mutual
+			// exclusion on these, or can I drop the clearing?
+			val &= ~MPFS_PINCTRL_PULL_MASK;
+			val |= MPFS_PINCTRL_WPD;
+			break;
+		case PIN_CONFIG_BIAS_PULL_UP:
+			val &= ~MPFS_PINCTRL_PULL_MASK;
+			val |= MPFS_PINCTRL_WPU;
+			break;
+		case PIN_CONFIG_BIAS_DISABLE:
+			val &= ~MPFS_PINCTRL_PULL_MASK;
+			break;
+		case PIN_CONFIG_DRIVE_STRENGTH:
+			tmp = mpfs_pinctrl_get_drive_strength_val(arg);
+			if (tmp < 0)
+				return tmp;
+
+			val |= FIELD_PREP(MPFS_PINCTRL_DRV_MASK, tmp);
+			break;
+		case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
+			if (!arg)
+				break;
+			fallthrough;
+		case PIN_CONFIG_INPUT_SCHMITT:
+		case PIN_CONFIG_INPUT_SCHMITT_UV:
+			//TODO Is it enabled regardless of register setting, or must it
+			// be set for lower voltage IO? Docs are missing, MSS Configurator
+			// is not clear. Leaning towards the latter.
+			//bank_voltage = mpfs_pinctrl_pin_to_bank_voltage(pctrl, pin);
+			//if (bank_voltage < MPFS_PINCTRL_LVCMOS25 && !arg) {
+			//	dev_err(pctrl->dev,
+			//		"schmitt always enabled for 1.2, 1.5 and 1.8 volt io\n");
+			//	return -EINVAL;
+			//}
+			val |= MPFS_PINCTRL_ENHYST;
+			break;
+		case PIN_CONFIG_PERSIST_STATE:
+			val |= MPFS_PINCTRL_LP_PERSIST_EN;
+			break;
+		case PIN_CONFIG_MODE_LOW_POWER:
+			if (arg)
+				val |= MPFS_PINCTRL_LP_BYPASS_EN;
+			break;
+		//TODO @Linus, do I have to document these custom controls other than in the binding?
+		case MPFS_PINCTRL_CLAMP_DIODE:
+			val |= MPFS_PINCTRL_CLAMP;
+			break;
+		case MPFS_PINCTRL_LOCKDOWN:
+			val &= MPFS_PINCTRL_LOCKDN;
+			break;
+		case MPFS_PINCTRL_IBUFMD:
+			val |= FIELD_PREP(MPFS_PINCTRL_IBUFMD_MASK, arg);
+			break;
+		default:
+			dev_err(pctrl->dev, "config %u not supported\n", param);
+			return -ENOTSUPP;
+		}
+	}
+
+	*value = val;
+	return 0;
+}
+
+static int mpfs_pinctrl_pin_set_config(struct mpfs_pinctrl *pctrl, unsigned int pin, u32 config)
+{
+	int reg = mpfs_pinctrl_pin_to_iocfg_reg(pin);
+	int offset = mpfs_pinctrl_pin_to_iocfg_offset(pin);
+	u32 val, mask;
+
+	mask = MPFS_PINCTRL_IOCFG_MASK << offset;
+	val = config << offset;
+
+	regmap_update_bits(pctrl->regmap, reg, mask, val);
+
+	return 0;
+}
+
+static int mpfs_pinctrl_pinconf_set(struct pinctrl_dev *pctrl_dev, unsigned int pin,
+				    unsigned long *configs, unsigned int num_configs)
+{
+	struct mpfs_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
+	u32 val;
+	int ret;
+
+	ret = mpfs_pinctrl_pinconf_generate_config(pctrl, pin, configs, num_configs, &val);
+	if (ret)
+		return ret;
+
+	return mpfs_pinctrl_pin_set_config(pctrl, pin, val);
+}
+
+static int mpfs_pinctrl_pinconf_group_set(struct pinctrl_dev *pctrl_dev, unsigned int gsel,
+					  unsigned long *configs, unsigned int num_configs)
+{
+	struct mpfs_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
+	const struct group_desc *group;
+	unsigned int pin;
+	u32 val;
+	int ret;
+
+	group = pinctrl_generic_get_group(pctrl_dev, gsel);
+	if (!group)
+		return -EINVAL;
+
+	/*
+	 * Assume that the first pin in a group is representative, as the mss
+	 * configurator doesn't allow splitting a function between two banks.
+	 */
+	pin = group->grp.pins[0];
+
+	ret = mpfs_pinctrl_pinconf_generate_config(pctrl, pin, configs, num_configs, &val);
+	if (ret)
+		return ret;
+
+	for (int i = 0; i < group->grp.npins; i++)
+		mpfs_pinctrl_pin_set_config(pctrl, group->grp.pins[i], val);
+
+	return 0;
+}
+
+static void mpfs_pinctrl_pinconf_dbg_show(struct pinctrl_dev *pctrl_dev, struct seq_file *seq,
+					  unsigned int pin)
+{
+	struct mpfs_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctrl_dev);
+	u32 val;
+	int reg, offset;
+
+	seq_printf(seq, ", bank voltage: 0x%x, ", mpfs_pinctrl_pin_to_bank_voltage(pctrl, pin));
+
+	reg = mpfs_pinctrl_pin_to_iocfg_reg(pin);
+	offset = mpfs_pinctrl_pin_to_iocfg_offset(pin);
+
+	seq_printf(seq, "pin: %u ", pin);
+	seq_printf(seq, "reg: %x offset: %u ", reg, offset);
+
+	if (reg < 0 || offset < 0)
+		return;
+
+	regmap_read(pctrl->regmap, reg, &val);
+	val = (val & (MPFS_PINCTRL_IOCFG_MASK << offset)) >> offset;
+	seq_printf(seq, "val: %x\n", val);
+}
+
+static const struct pinconf_ops mpfs_pinctrl_pinconf_ops = {
+	.pin_config_get			= mpfs_pinctrl_pinconf_get,
+	.pin_config_set			= mpfs_pinctrl_pinconf_set,
+	.pin_config_group_set		= mpfs_pinctrl_pinconf_group_set,
+	.pin_config_dbg_show		= mpfs_pinctrl_pinconf_dbg_show,
+	.is_generic			= true,
+};
+
+static const struct pinctrl_pin_desc mpfs_pinctrl_pins[] = {
+	PINCTRL_PIN(0,  "bank 4 0"),
+	PINCTRL_PIN(1,  "bank 4 1"),
+	PINCTRL_PIN(2,  "bank 4 2"),
+	PINCTRL_PIN(3,  "bank 4 3"),
+	PINCTRL_PIN(4,  "bank 4 4"),
+	PINCTRL_PIN(5,  "bank 4 5"),
+	PINCTRL_PIN(6,  "bank 4 6"),
+	PINCTRL_PIN(7,  "bank 4 7"),
+	PINCTRL_PIN(8,  "bank 4 8"),
+	PINCTRL_PIN(9,  "bank 4 9"),
+	PINCTRL_PIN(10, "bank 4 10"),
+	PINCTRL_PIN(11, "bank 4 11"),
+	PINCTRL_PIN(12, "bank 4 12"),
+	PINCTRL_PIN(13, "bank 4 13"),
+
+	PINCTRL_PIN(14, "bank 2 0"),
+	PINCTRL_PIN(15, "bank 2 1"),
+	PINCTRL_PIN(16, "bank 2 2"),
+	PINCTRL_PIN(17, "bank 2 3"),
+	PINCTRL_PIN(18, "bank 2 4"),
+	PINCTRL_PIN(19, "bank 2 5"),
+	PINCTRL_PIN(20, "bank 2 6"),
+	PINCTRL_PIN(21, "bank 2 7"),
+	PINCTRL_PIN(22, "bank 2 8"),
+	PINCTRL_PIN(23, "bank 2 9"),
+	PINCTRL_PIN(24, "bank 2 10"),
+	PINCTRL_PIN(25, "bank 2 11"),
+	PINCTRL_PIN(26, "bank 2 12"),
+	PINCTRL_PIN(27, "bank 2 13"),
+	PINCTRL_PIN(28, "bank 2 14"),
+	PINCTRL_PIN(29, "bank 2 15"),
+	PINCTRL_PIN(30, "bank 2 16"),
+	PINCTRL_PIN(31, "bank 2 17"),
+	PINCTRL_PIN(32, "bank 2 18"),
+	PINCTRL_PIN(33, "bank 2 19"),
+	PINCTRL_PIN(34, "bank 2 20"),
+	PINCTRL_PIN(35, "bank 2 21"),
+	PINCTRL_PIN(36, "bank 2 22"),
+	PINCTRL_PIN(37, "bank 2 23"),
+};
+
+static int mpfs_pinctrl_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct mpfs_pinctrl *pctrl;
+	struct regmap *sysreg_scb;
+	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");
+
+	sysreg_scb = syscon_regmap_lookup_by_compatible("microchip,mpfs-sysreg-scb");
+	if (IS_ERR(sysreg_scb))
+		return PTR_ERR(sysreg_scb);
+
+	pctrl->desc.name = dev_name(dev);
+	pctrl->desc.pins = mpfs_pinctrl_pins;
+	pctrl->desc.npins = ARRAY_SIZE(mpfs_pinctrl_pins);
+	pctrl->desc.pctlops = &mpfs_pinctrl_ops;
+	pctrl->desc.pmxops = &mpfs_pinctrl_pinmux_ops;
+	pctrl->desc.confops = &mpfs_pinctrl_pinconf_ops;
+	pctrl->desc.owner = THIS_MODULE;
+	pctrl->desc.num_custom_params = ARRAY_SIZE(mpfs_pinctrl_custom_bindings);
+	pctrl->desc.custom_params = mpfs_pinctrl_custom_bindings;
+
+	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_pinctrl_of_match[] = {
+	{ .compatible = "microchip,mpfs-pinctrl-mssio" },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, mpfs_pinctrl_of_match);
+
+static struct platform_driver mpfs_pinctrl_driver = {
+	.driver = {
+		.name = "mpfs-pinctrl",
+		.of_match_table = mpfs_pinctrl_of_match,
+	},
+	.probe = mpfs_pinctrl_probe,
+};
+module_platform_driver(mpfs_pinctrl_driver);
+
+MODULE_AUTHOR("Conor Dooley <conor.dooley@microchip.com>");
+MODULE_DESCRIPTION("Polarfire SoC mssio pinctrl driver");
+MODULE_LICENSE("GPL");
-- 
2.51.0

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Linus Walleij]

Hi Conor,

took a quick look at this!

On Wed, Nov 12, 2025 at 3:33 PM Conor Dooley <conor@kernel.org> wrote:

> +#include "linux/dev_printk.h"

Weird but it's RFC so OK :)

> +#define MPFS_PINCTRL_LOCKDOWN (PIN_CONFIG_END + 1)
> +#define MPFS_PINCTRL_CLAMP_DIODE (PIN_CONFIG_END + 2)
> +#define MPFS_PINCTRL_IBUFMD (PIN_CONFIG_END + 3)

Yeah this should work for custom props.

> +struct mpfs_pinctrl_drive_strength {
> +       u8 ma;
> +       u8 val;
> +};
> +
> +static struct mpfs_pinctrl_drive_strength mpfs_pinctrl_drive_strengths[8] = {
> +       { 2,   2 },
> +       { 4,   3 },
> +       { 6,   4 },
> +       { 8,   5 },
> +       { 10,  6 },
> +       { 12,  7 },
> +       { 16, 10 },
> +       { 20, 12 },
> +};

I would probably assign field explicitly with C99 syntax, but no
hard requirement.

{ .ma = 2, .val = 2 }

BTW you can see clearly that each setting activates
another driver stage in the silicon, each totempole giving
2 mA.

> +static const struct pinconf_generic_params mpfs_pinctrl_custom_bindings[] = {
> +       { "microchip,bank-lockdown", MPFS_PINCTRL_LOCKDOWN, 1 },
> +       { "microchip,clamp-diode", MPFS_PINCTRL_CLAMP_DIODE, 1 },
> +       { "microchip,ibufmd", MPFS_PINCTRL_IBUFMD, 0x0 },
> +};

I take it these have proper documentation in the DT bindings, so users know
exactly what they do.

> +static int mpfs_pinctrl_pin_to_iocfg_reg(unsigned int pin)
> +{
> +       u32 reg = MPFS_PINCTRL_IOCFG01_REG;
> +
> +       if (pin >= MPFS_PINCTRL_BANK2_START)
> +               reg += MPFS_PINCTRL_INTER_BANK_GAP;
> +
> +       // 2 pins per 32-bit register
> +       reg += (pin / 2) * 0x4;

This is a nice comment, easy to follow the code with small helpful
things like this.

> +static int mpfs_pinctrl_dt_node_to_map(struct pinctrl_dev *pctrl_dev, struct device_node *np,
> +                                      struct pinctrl_map **maps, unsigned int *num_maps)

I saw in the cover letter that you wanted this to use more generic helpers.

If you see room for improvement of the generic code, do not hesitate.
Doing a new driver is the only time when you actually have all these
details in your head and can create good helpers.

> +       //TODO @Linus, it correct to group these 3? There's no control over voltage.
> +       case PIN_CONFIG_INPUT_SCHMITT:
> +       case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> +       case PIN_CONFIG_INPUT_SCHMITT_UV:

Consider not supporting some like PIN_CONFIG_INPUT_SCHMITT_UV
in the bindings if they don't make any sense, and let it just return error
if someone tries to do that.

Isn't PIN_CONFIG_INPUT_SCHMITT_ENABLE the only one that
makes sense for this hardware?

> +static int mpfs_pinctrl_pinconf_generate_config(struct mpfs_pinctrl *pctrl, unsigned int pin,
> +                                               unsigned long *configs, unsigned int num_configs,
> +                                               u32 *value)
(...)
> +               case PIN_CONFIG_BIAS_PULL_DOWN:
> +                       //TODO always start from val == 0, there's no reason to ever actually
> +                       // clear anything AFAICT. @Linus, does the driver need to check mutual
> +                       // exclusion on these, or can I drop the clearing?
> +                       val &= ~MPFS_PINCTRL_PULL_MASK;
> +                       val |= MPFS_PINCTRL_WPD;
> +                       break;

I was about to say that the core checks that you don't enable pull up
and pull down
at the same time, but apparently that was just a dream I had.

The gpiolib however contains this in gpiod_configure_flags():

        if (((lflags & GPIO_PULL_UP) && (lflags & GPIO_PULL_DOWN)) ||
            ((lflags & GPIO_PULL_UP) && (lflags & GPIO_PULL_DISABLE)) ||
            ((lflags & GPIO_PULL_DOWN) && (lflags & GPIO_PULL_DISABLE))) {
                gpiod_err(desc,
                          "multiple pull-up, pull-down or pull-disable
enabled, invalid configuration\n");
                return -EINVAL;
        }

So there is a precedent for checking this.

So if you patch pinconf-generic.c to disallow this that'd be great, I think
it makes most sense to do this in the core.

> +               case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> +                       if (!arg)
> +                               break;
> +                       fallthrough;
> +               case PIN_CONFIG_INPUT_SCHMITT:
> +               case PIN_CONFIG_INPUT_SCHMITT_UV:
> +                       //TODO Is it enabled regardless of register setting, or must it
> +                       // be set for lower voltage IO? Docs are missing, MSS Configurator
> +                       // is not clear. Leaning towards the latter.
> +                       //bank_voltage = mpfs_pinctrl_pin_to_bank_voltage(pctrl, pin);
> +                       //if (bank_voltage < MPFS_PINCTRL_LVCMOS25 && !arg) {
> +                       //      dev_err(pctrl->dev,
> +                       //              "schmitt always enabled for 1.2, 1.5 and 1.8 volt io\n");
> +                       //      return -EINVAL;
> +                       //}
> +                       val |= MPFS_PINCTRL_ENHYST;
> +                       break;

See above.

I hope this helps!

Yours,
Linus Walleij

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Conor Dooley]

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

On Wed, Nov 19, 2025 at 01:08:26PM +0100, Linus Walleij wrote:
> Hi Conor,
> 
> took a quick look at this!
> 
> On Wed, Nov 12, 2025 at 3:33 PM Conor Dooley <conor@kernel.org> wrote:
> 
> > +#include "linux/dev_printk.h"
> 
> Weird but it's RFC so OK :)
> 
> > +#define MPFS_PINCTRL_LOCKDOWN (PIN_CONFIG_END + 1)
> > +#define MPFS_PINCTRL_CLAMP_DIODE (PIN_CONFIG_END + 2)
> > +#define MPFS_PINCTRL_IBUFMD (PIN_CONFIG_END + 3)
> 
> Yeah this should work for custom props.
> 
> > +struct mpfs_pinctrl_drive_strength {
> > +       u8 ma;
> > +       u8 val;
> > +};
> > +
> > +static struct mpfs_pinctrl_drive_strength mpfs_pinctrl_drive_strengths[8] = {
> > +       { 2,   2 },
> > +       { 4,   3 },
> > +       { 6,   4 },
> > +       { 8,   5 },
> > +       { 10,  6 },
> > +       { 12,  7 },
> > +       { 16, 10 },
> > +       { 20, 12 },
> > +};
> 
> I would probably assign field explicitly with C99 syntax, but no
> hard requirement.
> 
> { .ma = 2, .val = 2 }
> 
> BTW you can see clearly that each setting activates
> another driver stage in the silicon, each totempole giving
> 2 mA.
> 
> > +static const struct pinconf_generic_params mpfs_pinctrl_custom_bindings[] = {
> > +       { "microchip,bank-lockdown", MPFS_PINCTRL_LOCKDOWN, 1 },
> > +       { "microchip,clamp-diode", MPFS_PINCTRL_CLAMP_DIODE, 1 },
> > +       { "microchip,ibufmd", MPFS_PINCTRL_IBUFMD, 0x0 },
> > +};
> 
> I take it these have proper documentation in the DT bindings, so users know
> exactly what they do.

Honestly, even if users don't know what they do, they should just be
making this stuff 1:1 match some MSS configurator output, where it's
very clear if you select lockdown or turn on the clamp diode. The clamp
diode is almost certainly input voltage clamping (although the
documentation on the mssio stuff doesn't say anything about it) and the
lockdwn thing is some security feature that doesn't actually do anything
on its own, but will disable the bank in question if the tamper
detection hardware gets upset.
I'm not really all that sure why this is a per-pin setting, because the
documentation about this (and the MSS configurator) treat it as being
bank wide. There's some other part of this in another register that is
actually bank wide that I only discovered yesterday, and I dunno how
they interact.
The binding doesn't really explain this stuff, other than saying what
field in the configurator to get it from. I'll add something for !RFC.

ibufmd is a different story, the only source of it I could find was
in the configurator output files. I suspect that it can be just computed
from the other dt properties and the bank voltage, but I need to find
the answer in the configurator source code I think.

> > +static int mpfs_pinctrl_pin_to_iocfg_reg(unsigned int pin)
> > +{
> > +       u32 reg = MPFS_PINCTRL_IOCFG01_REG;
> > +
> > +       if (pin >= MPFS_PINCTRL_BANK2_START)
> > +               reg += MPFS_PINCTRL_INTER_BANK_GAP;
> > +
> > +       // 2 pins per 32-bit register
> > +       reg += (pin / 2) * 0x4;
> 
> This is a nice comment, easy to follow the code with small helpful
> things like this.
> 
> > +static int mpfs_pinctrl_dt_node_to_map(struct pinctrl_dev *pctrl_dev, struct device_node *np,
> > +                                      struct pinctrl_map **maps, unsigned int *num_maps)
> 
> I saw in the cover letter that you wanted this to use more generic helpers.
> 
> If you see room for improvement of the generic code, do not hesitate.
> Doing a new driver is the only time when you actually have all these
> details in your head and can create good helpers.
> 
> > +       //TODO @Linus, it correct to group these 3? There's no control over voltage.
> > +       case PIN_CONFIG_INPUT_SCHMITT:
> > +       case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> > +       case PIN_CONFIG_INPUT_SCHMITT_UV:
> 
> Consider not supporting some like PIN_CONFIG_INPUT_SCHMITT_UV
> in the bindings if they don't make any sense, and let it just return error
> if someone tries to do that.
> 
> Isn't PIN_CONFIG_INPUT_SCHMITT_ENABLE the only one that
> makes sense for this hardware?

Yeah, I just didn't know if having the others was helpful, since they
say things like:
 * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in
 *	schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis,
 *	the threshold value is given on a custom format as argument when
 *	setting pins to this mode.
which implies they should be implemented for systen not permitting
hysteresis adjustment.

> > +static int mpfs_pinctrl_pinconf_generate_config(struct mpfs_pinctrl *pctrl, unsigned int pin,
> > +                                               unsigned long *configs, unsigned int num_configs,
> > +                                               u32 *value)
> (...)
> > +               case PIN_CONFIG_BIAS_PULL_DOWN:
> > +                       //TODO always start from val == 0, there's no reason to ever actually
> > +                       // clear anything AFAICT. @Linus, does the driver need to check mutual
> > +                       // exclusion on these, or can I drop the clearing?
> > +                       val &= ~MPFS_PINCTRL_PULL_MASK;
> > +                       val |= MPFS_PINCTRL_WPD;
> > +                       break;
> 
> I was about to say that the core checks that you don't enable pull up
> and pull down
> at the same time, but apparently that was just a dream I had.
> 
> The gpiolib however contains this in gpiod_configure_flags():
> 
>         if (((lflags & GPIO_PULL_UP) && (lflags & GPIO_PULL_DOWN)) ||
>             ((lflags & GPIO_PULL_UP) && (lflags & GPIO_PULL_DISABLE)) ||
>             ((lflags & GPIO_PULL_DOWN) && (lflags & GPIO_PULL_DISABLE))) {
>                 gpiod_err(desc,
>                           "multiple pull-up, pull-down or pull-disable
> enabled, invalid configuration\n");
>                 return -EINVAL;
>         }
> 
> So there is a precedent for checking this.
> 
> So if you patch pinconf-generic.c to disallow this that'd be great, I think
> it makes most sense to do this in the core.

Yeah, seems better than doing it in my driver..

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

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Linus Walleij]

On Wed, Nov 19, 2025 at 7:23 PM Conor Dooley <conor@kernel.org> wrote:
> On Wed, Nov 19, 2025 at 01:08:26PM +0100, Linus Walleij wrote:
> > On Wed, Nov 12, 2025 at 3:33 PM Conor Dooley <conor@kernel.org> wrote:

> > > +       //TODO @Linus, it correct to group these 3? There's no control over voltage.
> > > +       case PIN_CONFIG_INPUT_SCHMITT:
> > > +       case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> > > +       case PIN_CONFIG_INPUT_SCHMITT_UV:
> >
> > Consider not supporting some like PIN_CONFIG_INPUT_SCHMITT_UV
> > in the bindings if they don't make any sense, and let it just return error
> > if someone tries to do that.
> >
> > Isn't PIN_CONFIG_INPUT_SCHMITT_ENABLE the only one that
> > makes sense for this hardware?
>
> Yeah, I just didn't know if having the others was helpful, since they
> say things like:
>  * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in
>  *      schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis,
>  *      the threshold value is given on a custom format as argument when
>  *      setting pins to this mode.
> which implies they should be implemented for systen not permitting
> hysteresis adjustment.

I see.

I looked at the bindings that look like this and are not 1:1 to the
in-kernel configs:

  input-schmitt-enable:
    type: boolean
    description: enable schmitt-trigger mode

  input-schmitt-disable:
    type: boolean
    description: disable schmitt-trigger mode

  input-schmitt-microvolt:
    description: threshold strength for schmitt-trigger

1. input-schmitt is missing! But it is right there in
drivers/pinctrl/pinconf-generic.c ... All DTS files appear to be
using input-schmitt-enable/disable and -microvolt.

2. input-schmitt-microvolt should probably be used separately
to set the voltage threshold and can be used in conjunction
with input-schmitt-enable in the same node. In your case
you probably don't want to use it at all and disallow it.

They are all treated individually in the parser.

Maybe we could patch the docs in pinconf-generic.h to make it clear that
they are all mutually exclusive.

The DT parser is a bit primitive for these.
For example right now it is fine with the schema
to set input-schmitt-enable and input-schmitt-disable at the same time, and
the result will be enabled because of parse order :/

The real trick would be to also make the
schema in Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
make them at least mutually exclusive and deprecate the
input-schmitt that noone is using, maybe that is simpler than I think?

Yours,
Linus Walleij

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Conor Dooley]

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

On Wed, Nov 19, 2025 at 10:48:07PM +0100, Linus Walleij wrote:
> On Wed, Nov 19, 2025 at 7:23 PM Conor Dooley <conor@kernel.org> wrote:
> > On Wed, Nov 19, 2025 at 01:08:26PM +0100, Linus Walleij wrote:
> > > On Wed, Nov 12, 2025 at 3:33 PM Conor Dooley <conor@kernel.org> wrote:
> 
> > > > +       //TODO @Linus, it correct to group these 3? There's no control over voltage.
> > > > +       case PIN_CONFIG_INPUT_SCHMITT:
> > > > +       case PIN_CONFIG_INPUT_SCHMITT_ENABLE:
> > > > +       case PIN_CONFIG_INPUT_SCHMITT_UV:
> > >
> > > Consider not supporting some like PIN_CONFIG_INPUT_SCHMITT_UV
> > > in the bindings if they don't make any sense, and let it just return error
> > > if someone tries to do that.
> > >
> > > Isn't PIN_CONFIG_INPUT_SCHMITT_ENABLE the only one that
> > > makes sense for this hardware?
> >
> > Yeah, I just didn't know if having the others was helpful, since they
> > say things like:
> >  * @PIN_CONFIG_INPUT_SCHMITT: this will configure an input pin to run in
> >  *      schmitt-trigger mode. If the schmitt-trigger has adjustable hysteresis,
> >  *      the threshold value is given on a custom format as argument when
> >  *      setting pins to this mode.
> > which implies they should be implemented for systen not permitting
> > hysteresis adjustment.
> 
> I see.
> 
> I looked at the bindings that look like this and are not 1:1 to the
> in-kernel configs:
> 
>   input-schmitt-enable:
>     type: boolean
>     description: enable schmitt-trigger mode
> 
>   input-schmitt-disable:
>     type: boolean
>     description: disable schmitt-trigger mode
> 
>   input-schmitt-microvolt:
>     description: threshold strength for schmitt-trigger
> 
> 1. input-schmitt is missing! But it is right there in
> drivers/pinctrl/pinconf-generic.c ... All DTS files appear to be
> using input-schmitt-enable/disable and -microvolt.
> 
> 2. input-schmitt-microvolt should probably be used separately
> to set the voltage threshold and can be used in conjunction
> with input-schmitt-enable in the same node. In your case
> you probably don't want to use it at all and disallow it.
> 
> They are all treated individually in the parser.
> 
> Maybe we could patch the docs in pinconf-generic.h to make it clear that
> they are all mutually exclusive.
> 
> The DT parser is a bit primitive for these.
> For example right now it is fine with the schema
> to set input-schmitt-enable and input-schmitt-disable at the same time, and
> the result will be enabled because of parse order :/

> The real trick would be to also make the
> schema in Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> make them at least mutually exclusive and deprecate the
> input-schmitt that noone is using, maybe that is simpler than I think?

I think that this is probably what to do. Mutual exclusion isn't
difficult to set up there and if there's no property for "input-schmitt"
then deprecating it sounds pretty reasonable?

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

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Linus Walleij]

On Thu, Nov 20, 2025 at 1:26 AM Conor Dooley <conor@kernel.org> wrote:
> On Wed, Nov 19, 2025 at 10:48:07PM +0100, Linus Walleij wrote:
> > On Wed, Nov 19, 2025 at 7:23 PM Conor Dooley <conor@kernel.org> wrote:

> > I looked at the bindings that look like this and are not 1:1 to the
> > in-kernel configs:
> >
> >   input-schmitt-enable:
> >     type: boolean
> >     description: enable schmitt-trigger mode
> >
> >   input-schmitt-disable:
> >     type: boolean
> >     description: disable schmitt-trigger mode
> >
> >   input-schmitt-microvolt:
> >     description: threshold strength for schmitt-trigger
> >
> > 1. input-schmitt is missing! But it is right there in
> > drivers/pinctrl/pinconf-generic.c ... All DTS files appear to be
> > using input-schmitt-enable/disable and -microvolt.
> >
> > 2. input-schmitt-microvolt should probably be used separately
> > to set the voltage threshold and can be used in conjunction
> > with input-schmitt-enable in the same node. In your case
> > you probably don't want to use it at all and disallow it.
> >
> > They are all treated individually in the parser.
> >
> > Maybe we could patch the docs in pinconf-generic.h to make it clear that
> > they are all mutually exclusive.
> >
> > The DT parser is a bit primitive for these.
> > For example right now it is fine with the schema
> > to set input-schmitt-enable and input-schmitt-disable at the same time, and
> > the result will be enabled because of parse order :/
>
> > The real trick would be to also make the
> > schema in Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> > make them at least mutually exclusive and deprecate the
> > input-schmitt that noone is using, maybe that is simpler than I think?
>
> I think that this is probably what to do. Mutual exclusion isn't
> difficult to set up there and if there's no property for "input-schmitt"
> then deprecating it sounds pretty reasonable?

Yeah I agree.

Do you want to look into it?

Otherwise it becomes my problem now that I've noticed it :D

Yours,
Linus Walleij

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Conor Dooley]

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

On Fri, Nov 21, 2025 at 12:13:21AM +0100, Linus Walleij wrote:
> On Thu, Nov 20, 2025 at 1:26 AM Conor Dooley <conor@kernel.org> wrote:
> > On Wed, Nov 19, 2025 at 10:48:07PM +0100, Linus Walleij wrote:
> > > On Wed, Nov 19, 2025 at 7:23 PM Conor Dooley <conor@kernel.org> wrote:
> 
> > > I looked at the bindings that look like this and are not 1:1 to the
> > > in-kernel configs:
> > >
> > >   input-schmitt-enable:
> > >     type: boolean
> > >     description: enable schmitt-trigger mode
> > >
> > >   input-schmitt-disable:
> > >     type: boolean
> > >     description: disable schmitt-trigger mode
> > >
> > >   input-schmitt-microvolt:
> > >     description: threshold strength for schmitt-trigger
> > >
> > > 1. input-schmitt is missing! But it is right there in
> > > drivers/pinctrl/pinconf-generic.c ... All DTS files appear to be
> > > using input-schmitt-enable/disable and -microvolt.
> > >
> > > 2. input-schmitt-microvolt should probably be used separately
> > > to set the voltage threshold and can be used in conjunction
> > > with input-schmitt-enable in the same node. In your case
> > > you probably don't want to use it at all and disallow it.
> > >
> > > They are all treated individually in the parser.
> > >
> > > Maybe we could patch the docs in pinconf-generic.h to make it clear that
> > > they are all mutually exclusive.
> > >
> > > The DT parser is a bit primitive for these.
> > > For example right now it is fine with the schema
> > > to set input-schmitt-enable and input-schmitt-disable at the same time, and
> > > the result will be enabled because of parse order :/
> >
> > > The real trick would be to also make the
> > > schema in Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> > > make them at least mutually exclusive and deprecate the
> > > input-schmitt that noone is using, maybe that is simpler than I think?
> >
> > I think that this is probably what to do. Mutual exclusion isn't
> > difficult to set up there and if there's no property for "input-schmitt"
> > then deprecating it sounds pretty reasonable?
> 
> Yeah I agree.
> 
> Do you want to look into it?
> 
> Otherwise it becomes my problem now that I've noticed it :D

Yeah, it's just a binding patch here I think, so yeah I'll do it.

> 
> Yours,
> Linus Walleij

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

Re: [RFC v1 2/4] pinctrl: add polarfire soc mssio pinctrl driver

[Conor Dooley]

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

On Fri, Nov 21, 2025 at 10:46:54AM +0000, Conor Dooley wrote:
> On Fri, Nov 21, 2025 at 12:13:21AM +0100, Linus Walleij wrote:
> > On Thu, Nov 20, 2025 at 1:26 AM Conor Dooley <conor@kernel.org> wrote:
> > > On Wed, Nov 19, 2025 at 10:48:07PM +0100, Linus Walleij wrote:
> > > > On Wed, Nov 19, 2025 at 7:23 PM Conor Dooley <conor@kernel.org> wrote:
> > 
> > > > I looked at the bindings that look like this and are not 1:1 to the
> > > > in-kernel configs:
> > > >
> > > >   input-schmitt-enable:
> > > >     type: boolean
> > > >     description: enable schmitt-trigger mode
> > > >
> > > >   input-schmitt-disable:
> > > >     type: boolean
> > > >     description: disable schmitt-trigger mode
> > > >
> > > >   input-schmitt-microvolt:
> > > >     description: threshold strength for schmitt-trigger
> > > >
> > > > 1. input-schmitt is missing! But it is right there in
> > > > drivers/pinctrl/pinconf-generic.c ... All DTS files appear to be
> > > > using input-schmitt-enable/disable and -microvolt.
> > > >
> > > > 2. input-schmitt-microvolt should probably be used separately
> > > > to set the voltage threshold and can be used in conjunction
> > > > with input-schmitt-enable in the same node. In your case
> > > > you probably don't want to use it at all and disallow it.
> > > >
> > > > They are all treated individually in the parser.
> > > >
> > > > Maybe we could patch the docs in pinconf-generic.h to make it clear that
> > > > they are all mutually exclusive.
> > > >
> > > > The DT parser is a bit primitive for these.
> > > > For example right now it is fine with the schema
> > > > to set input-schmitt-enable and input-schmitt-disable at the same time, and
> > > > the result will be enabled because of parse order :/
> > >
> > > > The real trick would be to also make the
> > > > schema in Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
> > > > make them at least mutually exclusive and deprecate the
> > > > input-schmitt that noone is using, maybe that is simpler than I think?
> > >
> > > I think that this is probably what to do. Mutual exclusion isn't
> > > difficult to set up there and if there's no property for "input-schmitt"
> > > then deprecating it sounds pretty reasonable?
> > 
> > Yeah I agree.
> > 
> > Do you want to look into it?
> > 
> > Otherwise it becomes my problem now that I've noticed it :D
> 
> Yeah, it's just a binding patch here I think, so yeah I'll do it.

ngl, I forget if there's a shorthand for the bias part, so I just want
to know if is this an accurate summary of what's exclusive?

diff --git a/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml b/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
index cbfcf215e571..6865472ac124 100644
--- a/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
+++ b/Documentation/devicetree/bindings/pinctrl/pincfg-node.yaml
@@ -153,4 +153,66 @@ properties:
       pin. Typically indicates how many double-inverters are
       used to delay the signal.
 
+allOf:
+  - if:
+      required:
+        - output-disable
+    then:
+      properties:
+        output-enable: false
+        output-impedance-ohms: false
+  - if:
+      required:
+        - output-low
+    then:
+      properties:
+        output-high: false
+  - if:
+      required:
+        - low-power-enable
+    then:
+      properties:
+        low-power-disable: false
+  - if:
+      required:
+        - input-schmitt-disable
+    then:
+      properties:
+        input-schmitt-enable: false
+        input-schmitt-microvolt: false
+  - if:
+      required:
+        - drive-open-source
+    then:
+      properties:
+        drive-open-drain: false
+  - if:
+      anyOf:
+        - required:
+            - bias-disable
+        - required:
+            - bias-high-impedance
+        - required:
+            - bias-hold
+        - required:
+            - bias-up
+        - required:
+            - bias-down
+        - required:
+            - bias-pull-pin-default
+    then:
+      oneOf:
+        - required:
+            - bias-disable
+        - required:
+            - bias-high-impedance
+        - required:
+            - bias-hold
+        - required:
+            - bias-up
+        - required:
+            - bias-down
+        - required:
+            - bias-pull-pin-default
+
 additionalProperties: true



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

[RFC v1 3/4] MAINTAINERS: add Microchip mpfs mssio driver/bindings to entry

[Conor Dooley]

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

Add the new mssio driver and bindings to the existing entry
for Microchip RISC-V devices.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 MAINTAINERS | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 5d4825073fcd..380970935407 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -22090,6 +22090,7 @@ F:	Documentation/devicetree/bindings/i2c/microchip,corei2c.yaml
 F:	Documentation/devicetree/bindings/mailbox/microchip,mpfs-mailbox.yaml
 F:	Documentation/devicetree/bindings/net/can/microchip,mpfs-can.yaml
 F:	Documentation/devicetree/bindings/pinctrl/microchip,mpfs-pinctrl-iomux0.yaml
+F:	Documentation/devicetree/bindings/pinctrl/microchip,mpfs-pinctrl-mssio.yaml
 F:	Documentation/devicetree/bindings/pinctrl/microchip,pic64gx-pinctrl-gpio2.yaml
 F:	Documentation/devicetree/bindings/pwm/microchip,corepwm.yaml
 F:	Documentation/devicetree/bindings/riscv/microchip.yaml
@@ -22105,6 +22106,7 @@ F:	drivers/i2c/busses/i2c-microchip-corei2c.c
 F:	drivers/mailbox/mailbox-mpfs.c
 F:	drivers/pci/controller/plda/pcie-microchip-host.c
 F:	drivers/pinctrl/pinctrl-mpfs-iomux0.c
+F:	drivers/pinctrl/pinctrl-mpfs-mssio.c
 F:	drivers/pinctrl/pinctrl-pic64gx-gpio2.c
 F:	drivers/pwm/pwm-microchip-core.c
 F:	drivers/reset/reset-mpfs.c
-- 
2.51.0

[RFC v1 4/4] riscv: dts: microchip: add pinctrl nodes for mpfs/icicle kit

[Conor Dooley]

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

Add pinctrl nodes to PolarFire to demonstrate their use, matching the
default configuration set by the HSS firmware for the Icicle kit's
reference design, as a demonstration of use.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../dts/microchip/mpfs-icicle-kit-common.dtsi |   1 -
 .../dts/microchip/mpfs-icicle-kit-fabric.dtsi |  63 +++++++
 .../boot/dts/microchip/mpfs-pinctrl.dtsi      | 165 ++++++++++++++++++
 arch/riscv/boot/dts/microchip/mpfs.dtsi       |  16 ++
 4 files changed, 244 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-common.dtsi b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-common.dtsi
index b3f61c58e57c..5667805b4b14 100644
--- a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-common.dtsi
+++ b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-common.dtsi
@@ -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-icicle-kit-fabric.dtsi b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit-fabric.dtsi
index 71f724325578..785176dabcf1 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"
+
 / {
 	core_pwm0: pwm@40000000 {
 		compatible = "microchip,corepwm-rtl-v4";
@@ -80,6 +83,16 @@ refclk_ccc: clock-cccref {
 	};
 };
 
+&can0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&can0_fabric>;
+};
+
+&can1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&ikrd_can1_cfg>;
+};
+
 &ccc_nw {
 	clocks = <&refclk_ccc>, <&refclk_ccc>, <&refclk_ccc>, <&refclk_ccc>,
 		 <&refclk_ccc>, <&refclk_ccc>;
@@ -87,3 +100,53 @@ &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 = <&uart1_fabric>;
+};
+
+&mmuart2 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart2_fabric>;
+};
+
+&mmuart3 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart3_fabric>;
+};
+
+&mmuart4 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&uart4_fabric>;
+};
+
+&mssio {
+	pinctrl-names = "default";
+	pinctrl-0 = <&spi1_mssio>, <&can1_mssio>, <&mdio0_mssio>, <&mdio1_mssio>;
+};
+
+&qspi {
+	pinctrl-names = "default";
+	pinctrl-0 = <&qspi_fabric>;
+};
+
+&spi0 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&spi0_fabric>;
+};
+
+&spi1 {
+	pinctrl-names = "default";
+	pinctrl-0 = <&ikrd_spi1_cfg>;
+};
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..47fc4a523c33
--- /dev/null
+++ b/arch/riscv/boot/dts/microchip/mpfs-pinctrl.dtsi
@@ -0,0 +1,165 @@
+// SPDX-License-Identifier: (GPL-2.0 OR MIT)
+
+&iomux0 {
+	spi0_fabric: mux-spi0-fabric {
+		function = "spi0";
+		groups = "spi0_fabric";
+	};
+
+	spi0_mssio: mux-spi0-mssio {
+		function = "spi0";
+		groups = "spi0_mssio";
+	};
+
+	spi1_fabric: mux-spi1-fabric {
+		function = "spi1";
+		groups = "spi1_fabric";
+	};
+
+	spi1_mssio: mux-spi1-mssio {
+		function = "spi1";
+		groups = "spi1_mssio";
+	};
+
+	i2c0_fabric: mux-i2c0-fabric {
+		function = "i2c0";
+		groups = "i2c0_fabric";
+	};
+
+	i2c0_mssio: mux-i2c0-mssio {
+		function = "i2c0";
+		groups = "i2c0_mssio";
+	};
+
+	i2c1_fabric: mux-i2c1-fabric {
+		function = "i2c1";
+		groups = "i2c1_fabric";
+	};
+
+	i2c1_mssio: mux-i2c1-mssio {
+		function = "i2c1";
+		groups = "i2c1_mssio";
+	};
+
+	can0_fabric: mux-can0-fabric {
+		function = "can0";
+		groups = "can0_fabric";
+	};
+
+	can0_mssio: mux-can0-mssio {
+		function = "can0";
+		groups = "can0_mssio";
+	};
+
+	can1_fabric: mux-can1-fabric {
+		function = "can1";
+		groups = "can1_fabric";
+	};
+
+	can1_mssio: mux-can1-mssio {
+		function = "can1";
+		groups = "can1_mssio";
+	};
+
+	qspi_fabric: mux-qspi-fabric {
+		function = "qspi";
+		groups = "qspi_fabric";
+	};
+
+	qspi_mssio: mux-qspi-mssio {
+		function = "qspi";
+		groups = "qspi_mssio";
+	};
+
+	uart0_fabric: mux-uart0-fabric {
+		function = "uart0";
+		groups = "uart0_fabric";
+	};
+
+	uart0_mssio: mux-uart0-mssio {
+		function = "uart0";
+		groups = "uart0_mssio";
+	};
+
+	uart1_fabric: mux-uart1-fabric {
+		function = "uart1";
+		groups = "uart1_fabric";
+	};
+
+	uart1_mssio: mux-uart1-mssio {
+		function = "uart1";
+		groups = "uart1_mssio";
+	};
+
+	uart2_fabric: mux-uart2-fabric {
+		function = "uart2";
+		groups = "uart2_fabric";
+	};
+
+	uart2_mssio: mux-uart2-mssio {
+		function = "uart2";
+		groups = "uart2_mssio";
+	};
+
+	uart3_fabric: mux-uart3-fabric {
+		function = "uart3";
+		groups = "uart3_fabric";
+	};
+
+	uart3_mssio: mux-uart3-mssio {
+		function = "uart3";
+		groups = "uart3_mssio";
+	};
+
+	uart4_fabric: mux-uart4-fabric {
+		function = "uart4";
+		groups = "uart4_fabric";
+	};
+
+	uart4_mssio: mux-uart4-mssio {
+		function = "uart4";
+		groups = "uart4_mssio";
+	};
+
+	mdio0_fabric: mux-mdio0-fabric {
+		function = "mdio0";
+		groups = "mdio0_fabric";
+	};
+
+	mdio0_mssio: mux-mdio0-mssio {
+		function = "mdio0";
+		groups = "mdio0_mssio";
+	};
+
+	mdio1_fabric: mux-mdio1-fabric {
+		function = "mdio1";
+		groups = "mdio1_fabric";
+	};
+
+	mdio1_mssio: mux-mdio1-mssio {
+		function = "mdio1";
+		groups = "mdio1_mssio";
+	};
+};
+
+&mssio {
+	ikrd_can1_cfg: ikrd-can1-cfg {
+		can1-pins {
+			pins = <34>, <35>, <36>;
+			function = "spi";
+			bias-pull-up;
+			drive-strength = <8>;
+			microchip,ibufmd = <0x1>;
+		};
+	};
+
+	ikrd_spi1_cfg: ikrd-spi1-cfg {
+		spi1-pins {
+			pins = <30>, <31>, <32>, <33>;
+			function = "spi";
+			bias-pull-up;
+			drive-strength = <8>;
+			microchip,ibufmd = <0x1>;
+		};
+	};
+};
diff --git a/arch/riscv/boot/dts/microchip/mpfs.dtsi b/arch/riscv/boot/dts/microchip/mpfs.dtsi
index 5c2963e269b8..0fb94581b6cb 100644
--- a/arch/riscv/boot/dts/microchip/mpfs.dtsi
+++ b/arch/riscv/boot/dts/microchip/mpfs.dtsi
@@ -254,7 +254,23 @@ 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;
+
+			};
+
+			mssio: pinctrl@204 {
+				compatible = "microchip,mpfs-pinctrl-mssio";
+				reg = <0x204 0x7c>;
+				/* on icicle ref design at least */
+				pinctrl-use-default;
+			};
 		};
 
 		sysreg_scb: syscon@20003000 {
-- 
2.51.0

Re: [RFC v1 0/4] Microchip mpfs/pic64gx pinctrl part 2

[Linus Walleij]

On Wed, Nov 12, 2025 at 3:33 PM Conor Dooley <conor@kernel.org> wrote:

> Got the other driver that I was talking about here for you...
> It's in RFC state because I'd like to get some feedback on the approach
> while I try to figure out a) what ibufmd is

I was going to ask about that :D

> and b) how the bank voltage
> interacts with the schmitt trigger setting.

Please check if "bank voltage" is somewhat analogous to
this generic config:

* @PIN_CONFIG_POWER_SOURCE: if the pin can select between different power
 *      supplies, the argument to this parameter (on a custom format) tells
 *      the driver which alternative power source to use.

> There's some specific @Linus questions in the driver, mostly where I was
> unsure about how config bits should be handled and looking around at
> other drivers wasn't sufficient because they did different things.

I tried to answer as best I could.

> Finally, on the dt side, this was using the pinmux property before the
> other drivers were submitted, but I didn't really like it to begin with
> (shoving two things into entries of the same property gives me the ick).
> I moved to using pins + function which at least looks prettier in the
> devicetree.

I think this looks way better than any pinmux properties.

> I had been hoping that I could move to some sort of generic
> dt_node_to_map function, but I couldn't figure out one that'd work
> without creating groups in the driver. If there is, I'd love to get rid
> of the custom dt_node_to_map stuff.

It seems like something that could be added to the core
(drivers/pinctrl/devicetree.c), if you feel like and have time for going
the extra mile. Maybe it would be simple to move some drivers
over to using it if done right.

> I want to avoid doing having set groups, because of the number of
> possible configurations in the MSS Configurator FPGA tool is fairly
> large, and I believe that MSS Configurator actually undersells the
> number of possible combinations for ease of use.

FPGA:s often have this "phone exchange" property.

> I haven't tested that
> and the driver technically doesn't support it, but I feel like not trying
> to define groups statically and using pins instead would permit those
> combos in the future should that use case ever materialise.

It makes sense for a driver for this type of very flexible hardware.

Yours,
Linus Walleij

Re: [RFC v1 0/4] Microchip mpfs/pic64gx pinctrl part 2

[Conor Dooley]

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

On Wed, Nov 19, 2025 at 01:16:16PM +0100, Linus Walleij wrote:
> On Wed, Nov 12, 2025 at 3:33 PM Conor Dooley <conor@kernel.org> wrote:
> 
> > Got the other driver that I was talking about here for you...
> > It's in RFC state because I'd like to get some feedback on the approach
> > while I try to figure out a) what ibufmd is
> 
> I was going to ask about that :D
> 
> > and b) how the bank voltage
> > interacts with the schmitt trigger setting.
> 
> Please check if "bank voltage" is somewhat analogous to
> this generic config:
> 
> * @PIN_CONFIG_POWER_SOURCE: if the pin can select between different power
>  *      supplies, the argument to this parameter (on a custom format) tells
>  *      the driver which alternative power source to use.

It's not pin based, the whole bank it is connected to has to be changed.
I'm not entirely sure if I want the driver to be able to control this at
all (at least not right now) because I don't know exactly what the
ramifications of changing it away from what the configuration tool
decided that it was are. Probably it could be done, but I don't know how
safe it is to change, as I think most of these kinds of settings were
never really seen as something "application" software would change
(linux is an application from the perspective of the design folks),
since it was all modifiable from the MSS configuration tool.

> > There's some specific @Linus questions in the driver, mostly where I was
> > unsure about how config bits should be handled and looking around at
> > other drivers wasn't sufficient because they did different things.
> 
> I tried to answer as best I could.

Cool, thanks.

> > Finally, on the dt side, this was using the pinmux property before the
> > other drivers were submitted, but I didn't really like it to begin with
> > (shoving two things into entries of the same property gives me the ick).
> > I moved to using pins + function which at least looks prettier in the
> > devicetree.
> 
> I think this looks way better than any pinmux properties.
> 
> > I had been hoping that I could move to some sort of generic
> > dt_node_to_map function, but I couldn't figure out one that'd work
> > without creating groups in the driver. If there is, I'd love to get rid
> > of the custom dt_node_to_map stuff.
> 
> It seems like something that could be added to the core
> (drivers/pinctrl/devicetree.c), if you feel like and have time for going
> the extra mile. Maybe it would be simple to move some drivers
> over to using it if done right.

Yeah I might, especially if it pushes people away from these pinmux
properties! Might also just submit this as-is and do it on top, depends
on what stuff I have going on.

> 
> > I want to avoid doing having set groups, because of the number of
> > possible configurations in the MSS Configurator FPGA tool is fairly
> > large, and I believe that MSS Configurator actually undersells the
> > number of possible combinations for ease of use.
> 
> FPGA:s often have this "phone exchange" property.
> 
> > I haven't tested that
> > and the driver technically doesn't support it, but I feel like not trying
> > to define groups statically and using pins instead would permit those
> > combos in the future should that use case ever materialise.
> 
> It makes sense for a driver for this type of very flexible hardware.


Thanks for taking a look.

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

Re: [RFC v1 0/4] Microchip mpfs/pic64gx pinctrl part 2

[Linus Walleij]

On Wed, Nov 19, 2025 at 7:06 PM Conor Dooley <conor@kernel.org> wrote:
> On Wed, Nov 19, 2025 at 01:16:16PM +0100, Linus Walleij wrote:
> > On Wed, Nov 12, 2025 at 3:33 PM Conor Dooley <conor@kernel.org> wrote:

> > > and b) how the bank voltage
> > > interacts with the schmitt trigger setting.
> >
> > Please check if "bank voltage" is somewhat analogous to
> > this generic config:
> >
> > * @PIN_CONFIG_POWER_SOURCE: if the pin can select between different power
> >  *      supplies, the argument to this parameter (on a custom format) tells
> >  *      the driver which alternative power source to use.
>
> It's not pin based, the whole bank it is connected to has to be changed.

So there *is* such a thing as a group pin config setting for a
whole group of pins. Groups are not just for functions...

And I don't know what is meant by a bank here, but it seems
to be exactly a group of pins.

From arch/arm/boot/dts/gemini/gemini-sq201.dts:

 /* Set up drive strength on GMAC0 and GMAC1 to 16 mA */
conf9 {
    groups = "gmii_gmac0_grp", "gmii_gmac1_grp";
    drive-strength = <16>;
};

If you look in driver/pinctrl/pinctrl-gemini.c you find:
gemini_pinconf_group_set()

static const struct pinconf_ops gemini_pinconf_ops = {
        .pin_config_get = gemini_pinconf_get,
        .pin_config_set = gemini_pinconf_set,
        .pin_config_group_set = gemini_pinconf_group_set,
        .is_generic = true,
};

OTOMH it's actually *fine* to *just* use groups for pin config like this
and *not* use it for muxing, i.e. have this group correspond to
a bank and not use that group for anything else than to set this
or any other per-bank property. But have a look!

Yours,
Linus Walleij

Re: [RFC v1 0/4] Microchip mpfs/pic64gx pinctrl part 2

[Conor Dooley]

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

On Wed, Nov 19, 2025 at 10:31:47PM +0100, Linus Walleij wrote:
> On Wed, Nov 19, 2025 at 7:06 PM Conor Dooley <conor@kernel.org> wrote:
> > On Wed, Nov 19, 2025 at 01:16:16PM +0100, Linus Walleij wrote:
> > > On Wed, Nov 12, 2025 at 3:33 PM Conor Dooley <conor@kernel.org> wrote:
> 
> > > > and b) how the bank voltage
> > > > interacts with the schmitt trigger setting.
> > >
> > > Please check if "bank voltage" is somewhat analogous to
> > > this generic config:
> > >
> > > * @PIN_CONFIG_POWER_SOURCE: if the pin can select between different power
> > >  *      supplies, the argument to this parameter (on a custom format) tells
> > >  *      the driver which alternative power source to use.
> >
> > It's not pin based, the whole bank it is connected to has to be changed.
> 
> So there *is* such a thing as a group pin config setting for a
> whole group of pins. Groups are not just for functions...
> 
> And I don't know what is meant by a bank here, but it seems
> to be exactly a group of pins.

Yeah, it's a whole group of pins. There's two banks that the IOs this
driver controls are split between, they don't really neatly correspond
to a particular function (although the configurator doesn't permit a
function belong to two banks, but it is technically possible for them
to).
> 
> From arch/arm/boot/dts/gemini/gemini-sq201.dts:
> 
>  /* Set up drive strength on GMAC0 and GMAC1 to 16 mA */
> conf9 {
>     groups = "gmii_gmac0_grp", "gmii_gmac1_grp";
>     drive-strength = <16>;
> };
> 
> If you look in driver/pinctrl/pinctrl-gemini.c you find:
> gemini_pinconf_group_set()
> 
> static const struct pinconf_ops gemini_pinconf_ops = {
>         .pin_config_get = gemini_pinconf_get,
>         .pin_config_set = gemini_pinconf_set,
>         .pin_config_group_set = gemini_pinconf_group_set,
>         .is_generic = true,
> };
> 
> OTOMH it's actually *fine* to *just* use groups for pin config like this
> and *not* use it for muxing, i.e. have this group correspond to
> a bank and not use that group for anything else than to set this
> or any other per-bank property. But have a look!

I could just put it in as a per-pin thing, even if the control isn't
actually that granular. The bank lockdown has a per-pin bit, but is
actually a bank wide toggle in the configurator, it wouldn't be too
different.

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