[RFC v7 0/6] PolarFire SoC GPIO support

[RFC v7 0/6] PolarFire SoC GPIO support

[Conor Dooley]

Hey all,

Lewis is no longer at Microchip, so I've taken over the GPIO controller
patchset that he had been working on prior to that:
https://lore.kernel.org/linux-gpio/20220815120834.1562544-1-lewis.hanly@microchip.com/

However, you'll note that I've got more than a GPIO controller patch in
this series now. One thing that was wrong with Lewis' series was that it
could only, depending on the iteration of the series, support GPIOs that
had their interrupts muxed or GPIOs that had dedicated interrupts at the
parent interrupt controller. I found that to be problematic, because the
hardware itself always has a mix of muxed and dedicated interrupts and
so there was always a controller rendered unusable for interrupts.

I've attempted to fix this by remodelling how the interrupt hierarchy in
the devicetree is described, with a mux added between the GPIO
controllers and the platform's interrupt controller. This is a better
match for the hardware, or more accurately *is* a match, and avoids the
issues I pointed out here:
https://lore.kernel.org/linux-gpio/23a69be6-96d3-1c28-f1aa-555e38ff991e@microchip.com/

Stealing from the irqchip driver patch, here's some more information on
the interrupt configuration on this SoC, for the 3 GPIO controllers:
- GPIO controller 0 has 14 GPIOs
- GPIO controller 1 has 24 GPIOs
- GPIO controller 2 has 32 GPIOs

All GPIOs are capable of generating interrupts, for a total of 70.
There are only 41 interrupts available however, so a configurable mux is
used to ensure all GPIOs can be used for interrupt generation. 38 of the
41 interrupts are in what the documentation calls "direct mode", as they
provide an exclusive connection from a GPIO to the PLIC. The 3 remaining
interrupts are used to mux the interrupts which do not have an exclusive
connection, one for each GPIO controller. A register is used to set this
configuration of this mux, depending on how the "MSS Configurator"
(FPGA configuration tool) has set things up. This is done by the
platform's firmware, so access from Linux is read-only.

The mux has a single register, where bits 0 to 13 mux between GPIO
controller 1's 14 GPIOs and GPIO controller 2's first 14 GPIOs. The
remaining bits mux between the first 18 GPIOs of controller 1 and the
last 18 GPIOS of controller 2. If a bit in the mux's control register is
set, the corresponding interrupt line for GPIO controller 0 or 1 will be
put in "non-direct" mode. If cleared, GPIO controller 2's will.

I'm downgrading the series to RFC status, and have not implemented
feedback received on the GPIO controller itself (which was mostly about
using regmap), because I would really like to hear from people on the
interrupt side of things, in case a significant re-write of the driver
is required. I've split the changes I made to the interrupt handling
portions of the GPIO driver into a patch of their own to make that more
obvious. Clearly that would need to be squashed, and any feedback
implemented, before the driver would be acceptable.

I previously enquired, a year ago when I actually wrote the irqchip
driver, about how to implement this mux and tried to follow Marc's
suggestions about how I should ago about doing so:
https://lore.kernel.org/all/87wn11oo5o.wl-maz@kernel.org/

Marc, I know you're no longer maintaining irqchip drivers, but I'd
appreciate if you in particular could take a look and see whether I
implemented your suggestions correctly - in particular the
irq_domain_disconnect_hierarchy() stuff.

My main issue with what I've conjured up here are that the hwirq number
that I get when calling irqd_to_hwirq() in the mask/unmask callbacks
in the gpio driver runs as far as 95, but each GPIO controller only can
have up 32 interrupts, so there are % 32 operations done in those
drivers to make the number a valid GPIO. In my naivety, I had expected
that the hwirqs seen by the irq_chip in the GPIO controller driver would
run from 0 to 31, but instead the hwirq numbers are that of the GPIO
controller irq_chip's parent (so my new mux).

I'm not sure if this sort of knowledge about how the parent works is
acceptable to have in the GPIO controller driver, but more pertinently
it screams "fundamental mistake" to me, and that my implementation of
the alloc irq_domain_ops callback is just broken. Unfortunately, I have
been unable to figure out a way to avoid that, while also (what I think
is) correctly using the irq_domain_disconnect_hierarchy() stuff in
the alloc callback.

So yeah, is doing something along these lines acceptable, or if not,
pointers as to how to resolve the problem would be highly appreciated.

And there's also a nastly looking of_iomap() in the irqchip driver. I
know this is unacceptable, some regions were described entirely
incorrectly in the original devicetree for this SoC, and I'm trying
to work on my problems one by one! Please ignore that for now.

Cheers,
Conor.

CC: Marc Zyngier <maz@kernel.org>
CC: Conor Dooley <conor.dooley@microchip.com>
CC: Daire McNamara <daire.mcnamara@microchip.com>
CC: Linus Walleij <linus.walleij@linaro.org>
CC: Bartosz Golaszewski <brgl@bgdev.pl>
CC: Rob Herring <robh@kernel.org>
CC: Krzysztof Kozlowski <krzk+dt@kernel.org>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Paul Walmsley <paul.walmsley@sifive.com>
CC: Palmer Dabbelt <palmer@dabbelt.com>
CC: linux-riscv@lists.infradead.org
CC: linux-gpio@vger.kernel.org
CC: devicetree@vger.kernel.org
CC: linux-kernel@vger.kernel.org

Conor Dooley (5):
  dt-bindings: gpio: fix microchip,mpfs-gpio interrupt descriptions
  dt-bindings: interrupt-controller: document PolarFire SoC's gpio
    interrupt mux
  irqchip: add mpfs gpio interrupt mux
  gpio: mpfs: pass gpio line number as irq data
  riscv: dts: microchip: update gpio interrupts to better match the SoC

Lewis Hanly (1):
  gpio: mpfs: add polarfire soc gpio support

 .../bindings/gpio/microchip,mpfs-gpio.yaml    |  28 +-
 .../microchip,mpfs-gpio-irq-mux.yaml          |  79 ++++
 .../boot/dts/microchip/mpfs-icicle-kit.dts    |   8 -
 arch/riscv/boot/dts/microchip/mpfs.dtsi       |  50 ++-
 drivers/gpio/Kconfig                          |   7 +
 drivers/gpio/Makefile                         |   1 +
 drivers/gpio/gpio-mpfs.c                      | 349 ++++++++++++++++++
 drivers/irqchip/Kconfig                       |  11 +
 drivers/irqchip/Makefile                      |   1 +
 drivers/irqchip/irq-mpfs-mux.c                | 333 +++++++++++++++++
 10 files changed, 840 insertions(+), 27 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/microchip,mpfs-gpio-irq-mux.yaml
 create mode 100644 drivers/gpio/gpio-mpfs.c
 create mode 100644 drivers/irqchip/irq-mpfs-mux.c

-- 
2.43.2

[RFC v7 1/6] dt-bindings: gpio: fix microchip,mpfs-gpio interrupt descriptions

[Conor Dooley]

The microchip,mpfs-gpio binding suffered greatly due to being written
with a narrow minded view of the controller, and the interrupt bits
ended up incorrect. It was mistakenly assumed that the interrupt
configuration was set by platform firmware, based on the FPGA
configuration, and that the GPIO DT nodes were the only way to really
communicate interrupt configuration to software.

Instead, the mux should be a device in its own right, and the GPIO
controllers should be connected to it, rather than to the PLIC.
Now that a binding exists for that mux, try to fix the misconceptions
in the GPIO controller binding.

Firstly, it's not possible for this controller to have fewer than 14
GPIOs, and thus 14 interrupts also. There are three controllers, with
14, 24 & 32 GPIOs each.

The example is wacky too - it follows from the incorrect understanding
that the GPIO controllers are connected to the PLIC directly. They are
not however, with a mux sitting in between. Update the example to use
the mux as a parent, and the interrupt numbers at the mux for GPIO2 as
the example - rather than the strange looking, repeated <53>.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../bindings/gpio/microchip,mpfs-gpio.yaml    | 28 +++++++++----------
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/Documentation/devicetree/bindings/gpio/microchip,mpfs-gpio.yaml b/Documentation/devicetree/bindings/gpio/microchip,mpfs-gpio.yaml
index d61569b3f15b2..eb7dbf1668285 100644
--- a/Documentation/devicetree/bindings/gpio/microchip,mpfs-gpio.yaml
+++ b/Documentation/devicetree/bindings/gpio/microchip,mpfs-gpio.yaml
@@ -22,7 +22,7 @@ properties:
   interrupts:
     description:
       Interrupt mapping, one per GPIO. Maximum 32 GPIOs.
-    minItems: 1
+    minItems: 14
     maxItems: 32
 
   interrupt-controller: true
@@ -39,9 +39,7 @@ properties:
   ngpios:
     description:
       The number of GPIOs available.
-    minimum: 1
-    maximum: 32
-    default: 32
+    enum: [14, 24, 32]
 
   gpio-controller: true
   gpio-line-names: true
@@ -81,6 +79,7 @@ required:
   - reg
   - "#gpio-cells"
   - gpio-controller
+  - ngpios
   - clocks
 
 additionalProperties: false
@@ -91,18 +90,19 @@ examples:
         compatible = "microchip,mpfs-gpio";
         reg = <0x20122000 0x1000>;
         clocks = <&clkcfg 25>;
-        interrupt-parent = <&plic>;
+        interrupt-parent = <&irqmux>;
         gpio-controller;
         #gpio-cells = <2>;
+        ngpios = <32>;
         interrupt-controller;
-        #interrupt-cells = <1>;
-        interrupts = <53>, <53>, <53>, <53>,
-                     <53>, <53>, <53>, <53>,
-                     <53>, <53>, <53>, <53>,
-                     <53>, <53>, <53>, <53>,
-                     <53>, <53>, <53>, <53>,
-                     <53>, <53>, <53>, <53>,
-                     <53>, <53>, <53>, <53>,
-                     <53>, <53>, <53>, <53>;
+        #interrupt-cells = <2>;
+        interrupts = <64>, <65>, <66>, <67>,
+                     <68>, <69>, <70>, <71>,
+                     <72>, <73>, <74>, <75>,
+                     <76>, <77>, <78>, <79>,
+                     <80>, <81>, <82>, <83>,
+                     <84>, <85>, <86>, <87>,
+                     <88>, <89>, <90>, <91>,
+                     <92>, <93>, <94>, <95>;
     };
 ...
-- 
2.43.2

[RFC v7 2/6] dt-bindings: interrupt-controller: document PolarFire SoC's gpio interrupt mux

[Conor Dooley]

On PolarFire SoC there are more GPIO interrupts than there are interrupt
lines available on the PLIC, and a runtime configurable mux is used to
decide which interrupts are assigned direct connections to the PLIC &
which are relegated to sharing a line.
This mux is, in our reference configuration, written by platform
firmware during boot based on the FPGA's configuration.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../microchip,mpfs-gpio-irq-mux.yaml          | 79 +++++++++++++++++++
 1 file changed, 79 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/interrupt-controller/microchip,mpfs-gpio-irq-mux.yaml

diff --git a/Documentation/devicetree/bindings/interrupt-controller/microchip,mpfs-gpio-irq-mux.yaml b/Documentation/devicetree/bindings/interrupt-controller/microchip,mpfs-gpio-irq-mux.yaml
new file mode 100644
index 0000000000000..89ed3a630eef3
--- /dev/null
+++ b/Documentation/devicetree/bindings/interrupt-controller/microchip,mpfs-gpio-irq-mux.yaml
@@ -0,0 +1,79 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/interrupt-controller/microchip,mpfs-gpio-irq-mux.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Microchip Polarfire SoC GPIO Interrupt Mux
+
+maintainers:
+  - Conor Dooley <conor.dooley@microchip.com>
+
+description: |
+  There are 3 GPIO controllers on this SoC, of which:
+  - GPIO controller 0 has 14 GPIOs
+  - GPIO controller 1 has 24 GPIOs
+  - GPIO controller 2 has 32 GPIOs
+
+  All GPIOs are capable of generating interrupts, for a total of 70.
+  There are only 41 IRQs available however, so a configurable mux is used to
+  ensure all GPIOs can be used for interrupt generation.
+  38 of the 41 interrupts are in what the documentation calls "direct mode",
+  as they provide an exclusive connection from a GPIO to the PLIC.
+  The 3 remaining interrupts are used to mux the interrupts which do not have
+  a exclusive connection, one for each GPIO controller.
+
+properties:
+  compatible:
+    const: microchip,mpfs-gpio-irq-mux
+
+  reg:
+    maxItems: 1
+
+  interrupt-controller: true
+
+  "#interrupt-cells":
+    const: 1
+
+  interrupts:
+    description:
+      The first 38 entries must be the "direct" interrupts, for exclusive
+      connections to the PLIC. The final 3 entries must be the
+      "non-direct"/muxed connections for each of GPIO controller 0, 1 & 2
+      respectively.
+    maxItems: 41
+
+allOf:
+  - $ref: /schemas/interrupt-controller.yaml#
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - "#interrupt-cells"
+  - interrupt-controller
+
+additionalProperties: false
+
+examples:
+  - |
+    irqmux: interrupt-controller@20002054 {
+        compatible = "microchip,mpfs-gpio-irq-mux";
+        reg = <0x20002054 0x4>;
+        interrupt-parent = <&plic>;
+        interrupt-controller;
+        #interrupt-cells = <1>;
+        status = "okay";
+        interrupts = <13>, <14>, <15>, <16>,
+                     <17>, <18>, <19>, <20>,
+                     <21>, <22>, <23>, <24>,
+                     <25>, <26>, <27>, <28>,
+                     <29>, <30>, <31>, <32>,
+                     <33>, <34>, <35>, <36>,
+                     <37>, <38>, <39>, <40>,
+                     <41>, <42>, <43>, <44>,
+                     <45>, <46>, <47>, <48>,
+                     <49>, <50>, <51>, <52>,
+                     <53>;
+    };
+...
-- 
2.43.2

[RFC v7 3/6] irqchip: add mpfs gpio interrupt mux

[Conor Dooley]

On PolarFire SoC there are more GPIO interrupts than there are interrupt
lines available on the PLIC, and a runtime configurable mux is used to
decide which interrupts are assigned direct connections to the PLIC &
which are relegated to sharing a line.
This mux is, in our reference configuration, written by platform
firmware during boot based on the FPGA's configuration.

Add a driver so that Linux can figure out this mapping and correctly map
the GPIO interrupts to their parents on the PLIC.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
Please read my cover letter for the things I'm unsure of on the
interrupt side.
---
 drivers/irqchip/Kconfig        |  11 ++
 drivers/irqchip/Makefile       |   1 +
 drivers/irqchip/irq-mpfs-mux.c | 333 +++++++++++++++++++++++++++++++++
 3 files changed, 345 insertions(+)
 create mode 100644 drivers/irqchip/irq-mpfs-mux.c

diff --git a/drivers/irqchip/Kconfig b/drivers/irqchip/Kconfig
index 14464716bacbb..2b951dbd559c7 100644
--- a/drivers/irqchip/Kconfig
+++ b/drivers/irqchip/Kconfig
@@ -706,6 +706,17 @@ config MCHP_EIC
 	help
 	  Support for Microchip External Interrupt Controller.
 
+config POLARFIRE_SOC_IRQ_MUX
+	bool "Microchip PolarFire SoC's GPIO IRQ Mux"
+	depends on ARCH_MICROCHIP_POLARFIRE
+	default y
+	help
+	  Support for the interrupt mux on Polarfire SoC. It sits between
+	  the GPIO controllers and the PLIC, as only 35 interrupts are shared
+	  between 3 GPIO controllers with 32 interrupts each. Configuration of
+	  the mux is done by the platform firmware, this driver is responsible
+	  for reading that configuration and setting up correct routing.
+
 config SUNPLUS_SP7021_INTC
 	bool "Sunplus SP7021 interrupt controller" if COMPILE_TEST
 	default SOC_SP7021
diff --git a/drivers/irqchip/Makefile b/drivers/irqchip/Makefile
index d9dc3d99aaa86..cf2f417c8d7fc 100644
--- a/drivers/irqchip/Makefile
+++ b/drivers/irqchip/Makefile
@@ -123,4 +123,5 @@ obj-$(CONFIG_WPCM450_AIC)		+= irq-wpcm450-aic.o
 obj-$(CONFIG_IRQ_IDT3243X)		+= irq-idt3243x.o
 obj-$(CONFIG_APPLE_AIC)			+= irq-apple-aic.o
 obj-$(CONFIG_MCHP_EIC)			+= irq-mchp-eic.o
+obj-$(CONFIG_POLARFIRE_SOC_IRQ_MUX)	+= irq-mpfs-mux.o
 obj-$(CONFIG_SUNPLUS_SP7021_INTC)	+= irq-sp7021-intc.o
diff --git a/drivers/irqchip/irq-mpfs-mux.c b/drivers/irqchip/irq-mpfs-mux.c
new file mode 100644
index 0000000000000..b093cae399700
--- /dev/null
+++ b/drivers/irqchip/irq-mpfs-mux.c
@@ -0,0 +1,333 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Microchip PolarFire SoC (MPFS) GPIO IRQ MUX
+ *
+ * Author: Conor Dooley <conor.dooley@microchip.com>
+ */
+
+#define pr_fmt(fmt) "mpfs-irq-mux: " fmt
+
+#include <linux/bits.h>
+#include <linux/interrupt.h>
+#include <linux/irqchip.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/irqdomain.h>
+#include <linux/of_address.h>
+#include <linux/of_irq.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+
+#define MPFS_MUX_NUM_IRQS		41
+#define MPFS_MUX_NUM_DIRECT_IRQS	38
+#define MPFS_MUX_NUM_NON_DIRECT_IRQS	3
+#define MPFS_MAX_IRQS_PER_GPIO		32
+#define MPFS_NUM_IRQS_GPIO0		14
+#define MPFS_NUM_IRQS_GPIO1		24
+#define MPFS_NUM_IRQS_GPIO2		32
+#define MPFS_NUM_IRQS_GPIO02_SHIFT	0
+#define MPFS_NUM_IRQS_GPIO1_SHIFT	14
+
+/*
+ * There are 3 GPIO controllers on this SoC, of which:
+ * - GPIO controller 0 has 14 GPIOs
+ * - GPIO controller 1 has 24 GPIOs
+ * - GPIO controller 2 has 32 GPIOs
+ *
+ * All GPIOs are capable of generating interrupts, for a total of 70.
+ * There are only 41 IRQs available however, so a configurable mux is used to
+ * ensure all GPIOs can be used for interrupt generation.
+ * 38 of the 41 interrupts are in what the documentation calls "direct mode",
+ * as they provide an exclusive connection from a GPIO to the PLIC.
+ * The 3 remaining interrupts are used to mux the interrupts which do not have
+ * a exclusive connection, one for each GPIO controller.
+ * A register is used to set this configuration of this mux, depending on how
+ * the "MSS Configurator" (FPGA configuration tool) has set things up.
+ * This is done by the platform's firmware, so access from Linux is read-only.
+ *
+ * Documentation also refers to GPIO controller 0 & 1 as "pad" GPIO controllers
+ * and GPIO controller 2 as the "fabric" GPIO controller. Despite that wording,
+ * all 3 are "hard" peripherals.
+ *
+ * The mux has a single register, where bits 0 to 13 mux between GPIO controller
+ * 1's 14 GPIOs and GPIO controller 2's first 14 GPIOs. The remaining bits mux
+ * between the first 18 GPIOs of controller 1 and the last 18 GPIOS of
+ * controller 2. If a bit in the mux's control register is set, the
+ * corresponding interrupt line for GPIO controller 0 or 1 will be put in
+ * "non-direct" mode. If cleared, the "fabric" controller's will.
+ *
+ * Register layout:
+ *    GPIO 1 interrupt line 17 | mux bit 31 | GPIO 2 interrupt line 31
+ *    ...                      | ...        | ...
+ *    ...                      | ...        | ...
+ *    GPIO 1 interrupt line  0 | mux bit 14 | GPIO 2 interrupt line 14
+ *    GPIO 0 interrupt line 13 | mux bit 13 | GPIO 2 interrupt line 13
+ *    ...                      | ...        | ...
+ *    ...                      | ...        | ...
+ *    GPIO 0 interrupt line  0 | mux bit  0 | GPIO 2 interrupt line  0
+ *
+ * That leaves 6 exclusive, or "direct", interrupts remaining. These are
+ * used by GPIO controller 1's lines 18 to 23.
+ */
+
+struct mpfs_irq_mux_bank_config {
+	u32 mask;
+	u8 shift;
+};
+
+static const struct mpfs_irq_mux_bank_config mpfs_irq_mux_bank_configs[3] = {
+	{GENMASK(MPFS_NUM_IRQS_GPIO0 - 1, 0), MPFS_NUM_IRQS_GPIO02_SHIFT},
+	{GENMASK(MPFS_NUM_IRQS_GPIO1 - 1, 0), MPFS_NUM_IRQS_GPIO1_SHIFT},
+	{GENMASK(MPFS_NUM_IRQS_GPIO2 - 1, 0), MPFS_NUM_IRQS_GPIO02_SHIFT},
+};
+
+struct mpfs_irq_mux_irqchip {
+	struct irq_domain *domain;
+	int bank;
+	int irq;
+	u8 offset;
+};
+
+struct mpfs_irq_mux {
+	void __iomem *reg;
+	u32 mux_config;
+	struct mpfs_irq_mux_irqchip nondirect_irqchips[MPFS_MUX_NUM_NON_DIRECT_IRQS];
+	int parent_irqs[MPFS_MUX_NUM_DIRECT_IRQS];
+};
+
+/*
+ * There is no "control" hardware in this mux, and as such there is no ability
+ * to mask at this level. As the irq has been disconnected from the hierarchy,
+ * there's no parent irqchip from which to use mask functions either.
+ */
+static void mpfs_irq_mux_irq_mask(struct irq_data *d) {}
+static void mpfs_irq_mux_irq_unmask(struct irq_data *d) {}
+
+static struct irq_chip mpfs_irq_mux_nondirect_irq_chip = {
+	.name = "MPFS GPIO Interrupt Mux",
+	.irq_mask = mpfs_irq_mux_irq_mask,
+	.irq_unmask = mpfs_irq_mux_irq_unmask,
+	.flags = IRQCHIP_IMMUTABLE,
+};
+
+static struct irq_chip mpfs_irq_mux_irq_chip = {
+	.name = "MPFS GPIO Interrupt Mux",
+	.irq_mask = irq_chip_mask_parent,
+	.irq_unmask = irq_chip_unmask_parent,
+	.irq_eoi = irq_chip_eoi_parent,
+	.irq_set_type = irq_chip_set_type_parent,
+	.irq_set_affinity = irq_chip_set_affinity_parent,
+	.flags = IRQCHIP_IMMUTABLE,
+};
+
+/*
+ * Returns an unsigned long, where a set bit indicates the corresponding
+ * interrupt is in non-direct/muxed mode for that bank/GPIO controller.
+ */
+static inline unsigned long mpfs_irq_mux_get_muxed_irqs(struct mpfs_irq_mux *priv,
+							unsigned int bank)
+{
+	unsigned long mux_config = priv->mux_config, muxed_irqs = -1;
+	struct mpfs_irq_mux_bank_config bank_config = mpfs_irq_mux_bank_configs[bank];
+
+	/*
+	 * If a bit is set in the mux, GPIO the corresponding interrupt from
+	 * controller 2 is direct and that controllers 0 or 1 is muxed.
+	 * Invert the bits in the configuration register, so that set bits
+	 * equate to non-direct mode, for GPIO controller 2.
+	 */
+	if (bank == 2u)
+		mux_config = ~mux_config;
+
+	muxed_irqs &= bank_config.mask;
+	muxed_irqs &= mux_config >> bank_config.shift;
+
+	return muxed_irqs;
+}
+
+static void mpfs_irq_mux_nondirect_handler(struct irq_desc *desc)
+{
+	struct mpfs_irq_mux_irqchip *irqchip_data = irq_desc_get_handler_data(desc);
+	struct mpfs_irq_mux *priv = container_of(irqchip_data, struct mpfs_irq_mux,
+						 nondirect_irqchips[irqchip_data->bank]);
+	unsigned long muxed_irqs;
+	int pos;
+
+	chained_irq_enter(irq_desc_get_chip(desc), desc);
+
+	muxed_irqs = mpfs_irq_mux_get_muxed_irqs(priv, irqchip_data->bank);
+
+	for_each_set_bit(pos, &muxed_irqs, MPFS_MAX_IRQS_PER_GPIO)
+		generic_handle_domain_irq(irqchip_data->domain, irqchip_data->offset + pos);
+
+	chained_irq_exit(irq_desc_get_chip(desc), desc);
+}
+
+static bool mpfs_irq_mux_is_direct(struct mpfs_irq_mux *priv, struct irq_fwspec *fwspec)
+{
+	unsigned int bank, line;
+	u32 muxed_irqs;
+
+	bank = fwspec->param[0] / MPFS_MAX_IRQS_PER_GPIO;
+	line = fwspec->param[0] % MPFS_MAX_IRQS_PER_GPIO;
+
+	muxed_irqs = mpfs_irq_mux_get_muxed_irqs(priv, bank);
+
+	if (BIT(line) & muxed_irqs)
+		return false;
+
+	return true;
+}
+
+static int mpfs_irq_mux_translate(struct irq_domain *d, struct irq_fwspec *fwspec,
+				  unsigned long *out_hwirq, unsigned int *out_type)
+{
+	if (!is_of_node(fwspec->fwnode))
+		return -EINVAL;
+
+	return irq_domain_translate_onecell(d, fwspec, out_hwirq, out_type);
+}
+
+static int mpfs_irq_mux_nondirect_alloc(struct irq_domain *d, unsigned int virq,
+					struct irq_fwspec *fwspec, struct mpfs_irq_mux *priv)
+{
+	unsigned int bank = fwspec->param[0] / MPFS_MAX_IRQS_PER_GPIO;
+
+	if (bank > 2)
+		return -EINVAL;
+
+	priv->nondirect_irqchips[bank].domain = d;
+
+	irq_domain_set_hwirq_and_chip(d, virq, fwspec->param[0],
+				      &mpfs_irq_mux_nondirect_irq_chip, priv);
+	irq_set_chained_handler_and_data(virq, handle_untracked_irq,
+					 &priv->nondirect_irqchips[bank]);
+
+	return irq_domain_disconnect_hierarchy(d->parent, virq);
+}
+
+static int mpfs_irq_mux_alloc(struct irq_domain *d, unsigned int virq,
+			      unsigned int nr_irqs, void *arg)
+{
+	struct mpfs_irq_mux *priv = d->host_data;
+	struct irq_fwspec *fwspec = arg;
+	struct irq_fwspec parent_fwspec;
+	unsigned int bank, line, irq;
+
+	if (!mpfs_irq_mux_is_direct(priv, fwspec))
+		return mpfs_irq_mux_nondirect_alloc(d, virq, fwspec, priv);
+
+	bank = fwspec->param[0] / MPFS_MAX_IRQS_PER_GPIO;
+	line = fwspec->param[0] % MPFS_MAX_IRQS_PER_GPIO;
+	irq = line + mpfs_irq_mux_bank_configs[bank].shift;
+
+	parent_fwspec.fwnode = d->parent->fwnode;
+	parent_fwspec.param[0] = priv->parent_irqs[irq];
+	parent_fwspec.param_count = 1;
+
+	irq_domain_set_hwirq_and_chip(d, virq, fwspec->param[0], &mpfs_irq_mux_irq_chip, priv);
+
+	return irq_domain_alloc_irqs_parent(d, virq, 1, &parent_fwspec);
+}
+
+static const struct irq_domain_ops mpfs_irq_mux_domain_ops = {
+	.translate = mpfs_irq_mux_translate,
+	.alloc = mpfs_irq_mux_alloc,
+	.free = irq_domain_free_irqs_common,
+};
+
+static int mpfs_irq_mux_probe(struct platform_device *pdev)
+{
+	struct device_node *node, *parent;
+	struct device *dev = &pdev->dev;
+	struct mpfs_irq_mux *priv;
+	struct irq_domain *hier_domain, *parent_domain;
+	int i, ret = 0;
+
+	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	/*
+	 * This is clearly unacceptable - there's a significant re-write of
+	 * the devicetree handling for this device required however to get
+	 * rid of this. It'll be fixed for !RFC.
+	 */
+	node = to_of_node(dev->fwnode);
+	priv->reg = of_iomap(node, 0);
+	if (!priv->reg) {
+		ret = -ENODEV;
+		goto out_free_priv;
+	}
+
+	priv->mux_config = readl(priv->reg);
+
+	for (i = 0; i < MPFS_MUX_NUM_DIRECT_IRQS; i++) {
+		struct of_phandle_args parent_irq;
+		int ret;
+
+		ret = of_irq_parse_one(node, i, &parent_irq);
+		if (ret) {
+			ret = -ENODEV;
+			goto out_unmap;
+		}
+
+		/*
+		 * The parent irqs are saved off for the first 38 interrupts
+		 * from the devicetree entry so that they can be used in the
+		 * domains alloc callback to allocate irqs from the parent irq
+		 * chip directly.
+		 */
+		priv->parent_irqs[i] = parent_irq.args[0];
+	}
+
+	parent = of_irq_find_parent(node);
+	parent_domain = irq_find_host(parent);
+	hier_domain = irq_domain_add_hierarchy(parent_domain, 0, MPFS_MAX_IRQS_PER_GPIO * 3,
+					       node, &mpfs_irq_mux_domain_ops, priv);
+	if (!hier_domain) {
+		dev_err(dev, "failed to add hierarchical domain\n");
+		ret = -ENODEV;
+		goto out_unmap;
+	}
+
+	/*
+	 * The last 3 interrupts must be the non-direct/muxed ones, per
+	 * the dt-binding.
+	 */
+	for (i = 0; i < MPFS_MUX_NUM_NON_DIRECT_IRQS; i++) {
+		int irq_index = i + MPFS_MUX_NUM_DIRECT_IRQS;
+
+		priv->nondirect_irqchips[i].bank = i;
+		priv->nondirect_irqchips[i].irq = irq_of_parse_and_map(node, irq_index);
+		priv->nondirect_irqchips[i].offset = i * MPFS_MAX_IRQS_PER_GPIO;
+		irq_set_chained_handler_and_data(priv->nondirect_irqchips[i].irq,
+						 mpfs_irq_mux_nondirect_handler,
+						 &priv->nondirect_irqchips[i]);
+	}
+
+	dev_dbg(dev, "mux configuration %x\n", priv->mux_config);
+
+	return 0;
+
+out_unmap:
+	iounmap(priv->reg);
+
+out_free_priv:
+	kfree(priv);
+
+	return ret;
+}
+
+static const struct of_device_id mpfs_irq_mux_match[] = {
+	{ .compatible = "microchip,mpfs-gpio-irq-mux" },
+
+};
+
+static struct platform_driver mpfs_irq_mux_driver = {
+	.driver = {
+		.name = "mpfs_irq_mux",
+		.of_match_table	= mpfs_irq_mux_match,
+	},
+	.probe = mpfs_irq_mux_probe,
+};
+builtin_platform_driver(mpfs_irq_mux_driver);
-- 
2.43.2

[RFC v7 4/6] gpio: mpfs: add polarfire soc gpio support

[Conor Dooley]

From: Lewis Hanly <lewis.hanly@microchip.com>

Add a driver to support the Polarfire SoC gpio controller

Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com>
Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
Unchanged from last list submission, I'm looking for comments on the
other patches in the series at the moment.
---
 drivers/gpio/Kconfig     |   7 +
 drivers/gpio/Makefile    |   1 +
 drivers/gpio/gpio-mpfs.c | 320 +++++++++++++++++++++++++++++++++++++++
 3 files changed, 328 insertions(+)
 create mode 100644 drivers/gpio/gpio-mpfs.c

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 3dbddec070281..78fe494e3722c 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -549,6 +549,13 @@ config GPIO_PL061
 	help
 	  Say yes here to support the PrimeCell PL061 GPIO device.
 
+config GPIO_POLARFIRE_SOC
+	bool "Microchip FPGA GPIO support"
+	depends on OF_GPIO
+	select GPIOLIB_IRQCHIP
+	help
+	  Say yes here to support the GPIO device on Microchip FPGAs.
+
 config GPIO_PXA
 	bool "PXA GPIO support"
 	depends on ARCH_PXA || ARCH_MMP || COMPILE_TEST
diff --git a/drivers/gpio/Makefile b/drivers/gpio/Makefile
index e2a53013780e5..dd6ba21bce76e 100644
--- a/drivers/gpio/Makefile
+++ b/drivers/gpio/Makefile
@@ -132,6 +132,7 @@ obj-$(CONFIG_GPIO_PCI_IDIO_16)		+= gpio-pci-idio-16.o
 obj-$(CONFIG_GPIO_PISOSR)		+= gpio-pisosr.o
 obj-$(CONFIG_GPIO_PL061)		+= gpio-pl061.o
 obj-$(CONFIG_GPIO_PMIC_EIC_SPRD)	+= gpio-pmic-eic-sprd.o
+obj-$(CONFIG_GPIO_POLARFIRE_SOC)	+= gpio-mpfs.o
 obj-$(CONFIG_GPIO_PXA)			+= gpio-pxa.o
 obj-$(CONFIG_GPIO_RASPBERRYPI_EXP)	+= gpio-raspberrypi-exp.o
 obj-$(CONFIG_GPIO_RC5T583)		+= gpio-rc5t583.o
diff --git a/drivers/gpio/gpio-mpfs.c b/drivers/gpio/gpio-mpfs.c
new file mode 100644
index 0000000000000..1ac0526ba1029
--- /dev/null
+++ b/drivers/gpio/gpio-mpfs.c
@@ -0,0 +1,320 @@
+// SPDX-License-Identifier: (GPL-2.0)
+/*
+ * Microchip PolarFire SoC (MPFS) GPIO controller driver
+ *
+ * Copyright (c) 2018-2022 Microchip Technology Inc. and its subsidiaries
+ *
+ * Author: Lewis Hanly <lewis.hanly@microchip.com>
+ */
+
+#include <linux/bitops.h>
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/errno.h>
+#include <linux/gpio/driver.h>
+#include <linux/init.h>
+#include <linux/irq.h>
+#include <linux/irqchip/chained_irq.h>
+#include <linux/of.h>
+#include <linux/of_irq.h>
+#include <linux/mod_devicetable.h>
+#include <linux/platform_device.h>
+#include <linux/spinlock.h>
+
+#define MPFS_GPIO_CTRL(i)		(0x4 * (i))
+#define MAX_NUM_GPIO			32
+#define MPFS_GPIO_EN_INT		3
+#define MPFS_GPIO_EN_OUT_BUF		BIT(2)
+#define MPFS_GPIO_EN_IN			BIT(1)
+#define MPFS_GPIO_EN_OUT		BIT(0)
+
+#define MPFS_GPIO_TYPE_INT_EDGE_BOTH	0x80
+#define MPFS_GPIO_TYPE_INT_EDGE_NEG	0x60
+#define MPFS_GPIO_TYPE_INT_EDGE_POS	0x40
+#define MPFS_GPIO_TYPE_INT_LEVEL_LOW	0x20
+#define MPFS_GPIO_TYPE_INT_LEVEL_HIGH	0x00
+#define MPFS_GPIO_TYPE_INT_MASK		GENMASK(7, 5)
+#define MPFS_IRQ_REG			0x80
+#define MPFS_INP_REG			0x84
+#define MPFS_OUTP_REG			0x88
+
+struct mpfs_gpio_chip {
+	void __iomem *base;
+	struct clk *clk;
+	raw_spinlock_t	lock;
+	struct gpio_chip gc;
+};
+
+static void mpfs_gpio_assign_bit(void __iomem *addr, unsigned int bit_offset, bool value)
+{
+	unsigned long reg = readl(addr);
+
+	__assign_bit(bit_offset, &reg, value);
+	writel(reg, addr);
+}
+
+static int mpfs_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio_index)
+{
+	struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+	u32 gpio_cfg;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
+
+	gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
+	gpio_cfg |= MPFS_GPIO_EN_IN;
+	gpio_cfg &= ~(MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF);
+	writel(gpio_cfg, mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
+
+	raw_spin_unlock_irqrestore(&mpfs_gpio->lock, flags);
+
+	return 0;
+}
+
+static int mpfs_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio_index, int value)
+{
+	struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+	u32 gpio_cfg;
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
+
+	gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
+	gpio_cfg |= MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF;
+	gpio_cfg &= ~MPFS_GPIO_EN_IN;
+	writel(gpio_cfg, mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
+
+	mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_OUTP_REG, gpio_index, value);
+
+	raw_spin_unlock_irqrestore(&mpfs_gpio->lock, flags);
+
+	return 0;
+}
+
+static int mpfs_gpio_get_direction(struct gpio_chip *gc,
+				   unsigned int gpio_index)
+{
+	struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+	u32 gpio_cfg;
+
+	gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
+	if (gpio_cfg & MPFS_GPIO_EN_IN)
+		return GPIO_LINE_DIRECTION_IN;
+
+	return GPIO_LINE_DIRECTION_OUT;
+}
+
+static int mpfs_gpio_get(struct gpio_chip *gc,
+			 unsigned int gpio_index)
+{
+	struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+
+	return !!(readl(mpfs_gpio->base + MPFS_INP_REG) & BIT(gpio_index));
+}
+
+static void mpfs_gpio_set(struct gpio_chip *gc, unsigned int gpio_index, int value)
+{
+	struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+	unsigned long flags;
+
+	raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
+
+	mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_OUTP_REG,
+			     gpio_index, value);
+
+	raw_spin_unlock_irqrestore(&mpfs_gpio->lock, flags);
+}
+
+static int mpfs_gpio_irq_set_type(struct irq_data *data, unsigned int type)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+	int gpio_index = irqd_to_hwirq(data);
+	u32 interrupt_type;
+	u32 gpio_cfg;
+	unsigned long flags;
+
+	switch (type) {
+	case IRQ_TYPE_EDGE_BOTH:
+		interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_BOTH;
+		break;
+	case IRQ_TYPE_EDGE_FALLING:
+		interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_NEG;
+		break;
+	case IRQ_TYPE_EDGE_RISING:
+		interrupt_type = MPFS_GPIO_TYPE_INT_EDGE_POS;
+		break;
+	case IRQ_TYPE_LEVEL_HIGH:
+		interrupt_type = MPFS_GPIO_TYPE_INT_LEVEL_HIGH;
+		break;
+	case IRQ_TYPE_LEVEL_LOW:
+		interrupt_type = MPFS_GPIO_TYPE_INT_LEVEL_LOW;
+		break;
+	}
+
+	raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
+
+	gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
+	gpio_cfg &= ~MPFS_GPIO_TYPE_INT_MASK;
+	gpio_cfg |= interrupt_type;
+	writel(gpio_cfg, mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
+
+	raw_spin_unlock_irqrestore(&mpfs_gpio->lock, flags);
+
+	return 0;
+}
+
+static void mpfs_gpio_irq_unmask(struct irq_data *data)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+	int gpio_index = irqd_to_hwirq(data);
+
+	gpiochip_enable_irq(gc, gpio_index);
+	mpfs_gpio_direction_input(gc, gpio_index);
+	mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, gpio_index, 1);
+	mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index),
+			     MPFS_GPIO_EN_INT, 1);
+}
+
+static void mpfs_gpio_irq_mask(struct irq_data *data)
+{
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
+	struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+	int gpio_index = irqd_to_hwirq(data);
+
+	mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, gpio_index, 1);
+	mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index),
+			     MPFS_GPIO_EN_INT, 0);
+	gpiochip_disable_irq(gc, gpio_index);
+}
+
+static const struct irq_chip mpfs_gpio_irqchip = {
+	.name = "mpfs",
+	.irq_set_type = mpfs_gpio_irq_set_type,
+	.irq_mask	= mpfs_gpio_irq_mask,
+	.irq_unmask	= mpfs_gpio_irq_unmask,
+	.flags = IRQCHIP_IMMUTABLE | IRQCHIP_MASK_ON_SUSPEND,
+	GPIOCHIP_IRQ_RESOURCE_HELPERS,
+};
+
+static void mpfs_gpio_irq_handler(struct irq_desc *desc)
+{
+	struct irq_chip *irqchip = irq_desc_get_chip(desc);
+	struct mpfs_gpio_chip *mpfs_gpio =
+		gpiochip_get_data(irq_desc_get_handler_data(desc));
+	unsigned long status;
+	int offset;
+
+	chained_irq_enter(irqchip, desc);
+
+	status = readl(mpfs_gpio->base + MPFS_IRQ_REG);
+	for_each_set_bit(offset, &status, mpfs_gpio->gc.ngpio) {
+		mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, offset, 1);
+		generic_handle_irq(irq_find_mapping(mpfs_gpio->gc.irq.domain, offset));
+	}
+
+	chained_irq_exit(irqchip, desc);
+}
+
+static int mpfs_gpio_probe(struct platform_device *pdev)
+{
+	struct clk *clk;
+	struct device *dev = &pdev->dev;
+	struct device_node *node = pdev->dev.of_node;
+	struct mpfs_gpio_chip *mpfs_gpio;
+	struct gpio_irq_chip *girq;
+	int i, ret, ngpios, nirqs;
+
+	mpfs_gpio = devm_kzalloc(dev, sizeof(*mpfs_gpio), GFP_KERNEL);
+	if (!mpfs_gpio)
+		return -ENOMEM;
+
+	mpfs_gpio->base = devm_platform_ioremap_resource(pdev, 0);
+	if (IS_ERR(mpfs_gpio->base))
+		return dev_err_probe(dev, PTR_ERR(mpfs_gpio->base), "failed to ioremap memory resource\n");
+
+	clk = devm_clk_get(dev, NULL);
+	if (IS_ERR(clk))
+		return dev_err_probe(dev, PTR_ERR(clk), "devm_clk_get failed\n");
+
+	ret = clk_prepare_enable(clk);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed to enable clock\n");
+
+	mpfs_gpio->clk = clk;
+
+	ngpios = MAX_NUM_GPIO;
+	device_property_read_u32(dev, "ngpios", &ngpios);
+	if (ngpios > MAX_NUM_GPIO)
+		ngpios = MAX_NUM_GPIO;
+
+	raw_spin_lock_init(&mpfs_gpio->lock);
+	mpfs_gpio->gc.direction_input = mpfs_gpio_direction_input;
+	mpfs_gpio->gc.direction_output = mpfs_gpio_direction_output;
+	mpfs_gpio->gc.get_direction = mpfs_gpio_get_direction;
+	mpfs_gpio->gc.get = mpfs_gpio_get;
+	mpfs_gpio->gc.set = mpfs_gpio_set;
+	mpfs_gpio->gc.base = -1;
+	mpfs_gpio->gc.ngpio = ngpios;
+	mpfs_gpio->gc.label = dev_name(dev);
+	mpfs_gpio->gc.parent = dev;
+	mpfs_gpio->gc.owner = THIS_MODULE;
+
+	nirqs = of_irq_count(node);
+	if (nirqs > MAX_NUM_GPIO) {
+		ret = -ENXIO;
+		goto cleanup_clock;
+	}
+	girq = &mpfs_gpio->gc.irq;
+	gpio_irq_chip_set_chip(girq, &mpfs_gpio_irqchip);
+	girq->handler = handle_simple_irq;
+	girq->parent_handler = mpfs_gpio_irq_handler;
+	girq->default_type = IRQ_TYPE_NONE;
+	girq->num_parents = nirqs;
+	girq->parents = devm_kcalloc(&pdev->dev, nirqs,
+				     sizeof(*girq->parents), GFP_KERNEL);
+	if (!girq->parents) {
+		ret = -ENOMEM;
+		goto cleanup_clock;
+	}
+	for (i = 0; i < nirqs; i++)
+		girq->parents[i] = platform_get_irq(pdev, i);
+
+	ret = gpiochip_add_data(&mpfs_gpio->gc, mpfs_gpio);
+	if (ret)
+		goto cleanup_clock;
+
+	platform_set_drvdata(pdev, mpfs_gpio);
+
+	return 0;
+
+cleanup_clock:
+	clk_disable_unprepare(mpfs_gpio->clk);
+	return ret;
+}
+
+static int mpfs_gpio_remove(struct platform_device *pdev)
+{
+	struct mpfs_gpio_chip *mpfs_gpio = platform_get_drvdata(pdev);
+
+	gpiochip_remove(&mpfs_gpio->gc);
+	clk_disable_unprepare(mpfs_gpio->clk);
+
+	return 0;
+}
+
+static const struct of_device_id mpfs_gpio_of_ids[] = {
+	{ .compatible = "microchip,mpfs-gpio", },
+	{ /* end of list */ }
+};
+
+static struct platform_driver mpfs_gpio_driver = {
+	.probe = mpfs_gpio_probe,
+	.driver = {
+		.name = "microchip,mpfs-gpio",
+		.of_match_table = mpfs_gpio_of_ids,
+	},
+	.remove = mpfs_gpio_remove,
+};
+builtin_platform_driver(mpfs_gpio_driver);
-- 
2.43.2

[RFC v7 5/6] gpio: mpfs: pass gpio line number as irq data

[Conor Dooley]

Since the interrupt mux is going to provide us a 1:1 mapping for
interrupts, and it is no longer correct to hit all of the set bits in
the interrupt handler, store the GPIO that "owns" an interrupt in its
data pointer, so that we can determine which bit to clear.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
This patch will need to be squashed, I've kept it apart for illustrative
purposes.
---
 drivers/gpio/gpio-mpfs.c | 85 +++++++++++++++++++++++++++-------------
 1 file changed, 57 insertions(+), 28 deletions(-)

diff --git a/drivers/gpio/gpio-mpfs.c b/drivers/gpio/gpio-mpfs.c
index 1ac0526ba1029..b92f094964822 100644
--- a/drivers/gpio/gpio-mpfs.c
+++ b/drivers/gpio/gpio-mpfs.c
@@ -43,6 +43,7 @@ struct mpfs_gpio_chip {
 	struct clk *clk;
 	raw_spinlock_t	lock;
 	struct gpio_chip gc;
+	u8 irq_data[MAX_NUM_GPIO];
 };
 
 static void mpfs_gpio_assign_bit(void __iomem *addr, unsigned int bit_offset, bool value)
@@ -129,7 +130,7 @@ static int mpfs_gpio_irq_set_type(struct irq_data *data, unsigned int type)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
 	struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
-	int gpio_index = irqd_to_hwirq(data);
+	int gpio_index = irqd_to_hwirq(data) % 32;
 	u32 interrupt_type;
 	u32 gpio_cfg;
 	unsigned long flags;
@@ -168,11 +169,10 @@ static void mpfs_gpio_irq_unmask(struct irq_data *data)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
 	struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
-	int gpio_index = irqd_to_hwirq(data);
+	int gpio_index = irqd_to_hwirq(data) % 32;
 
 	gpiochip_enable_irq(gc, gpio_index);
 	mpfs_gpio_direction_input(gc, gpio_index);
-	mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, gpio_index, 1);
 	mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index),
 			     MPFS_GPIO_EN_INT, 1);
 }
@@ -181,19 +181,18 @@ static void mpfs_gpio_irq_mask(struct irq_data *data)
 {
 	struct gpio_chip *gc = irq_data_get_irq_chip_data(data);
 	struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
-	int gpio_index = irqd_to_hwirq(data);
+	int gpio_index = irqd_to_hwirq(data) % 32;
 
-	mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, gpio_index, 1);
 	mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index),
 			     MPFS_GPIO_EN_INT, 0);
 	gpiochip_disable_irq(gc, gpio_index);
 }
 
 static const struct irq_chip mpfs_gpio_irqchip = {
-	.name = "mpfs",
+	.name = "MPFS GPIO",
 	.irq_set_type = mpfs_gpio_irq_set_type,
-	.irq_mask	= mpfs_gpio_irq_mask,
-	.irq_unmask	= mpfs_gpio_irq_unmask,
+	.irq_mask = mpfs_gpio_irq_mask,
+	.irq_unmask = mpfs_gpio_irq_unmask,
 	.flags = IRQCHIP_IMMUTABLE | IRQCHIP_MASK_ON_SUSPEND,
 	GPIOCHIP_IRQ_RESOURCE_HELPERS,
 };
@@ -201,18 +200,24 @@ static const struct irq_chip mpfs_gpio_irqchip = {
 static void mpfs_gpio_irq_handler(struct irq_desc *desc)
 {
 	struct irq_chip *irqchip = irq_desc_get_chip(desc);
-	struct mpfs_gpio_chip *mpfs_gpio =
-		gpiochip_get_data(irq_desc_get_handler_data(desc));
+	void *handler_data = irq_desc_get_handler_data(desc);
+	struct gpio_chip *gc = irq_data_get_irq_chip_data(&desc->irq_data);
+	struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
+	u8 gpio_index = *((u8 *)handler_data);
 	unsigned long status;
-	int offset;
+
+	/*
+	 * Since the parent may be a muxed/"non-direct" interrupt, this
+	 * interrupt may not be for us.
+	 */
+	status = readl(mpfs_gpio->base + MPFS_IRQ_REG);
+	if (!(status & BIT(gpio_index)))
+		return;
 
 	chained_irq_enter(irqchip, desc);
 
-	status = readl(mpfs_gpio->base + MPFS_IRQ_REG);
-	for_each_set_bit(offset, &status, mpfs_gpio->gc.ngpio) {
-		mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, offset, 1);
-		generic_handle_irq(irq_find_mapping(mpfs_gpio->gc.irq.domain, offset));
-	}
+	generic_handle_irq(irq_find_mapping(mpfs_gpio->gc.irq.domain, gpio_index));
+	mpfs_gpio_assign_bit(mpfs_gpio->base + MPFS_IRQ_REG, gpio_index, 1);
 
 	chained_irq_exit(irqchip, desc);
 }
@@ -222,6 +227,7 @@ static int mpfs_gpio_probe(struct platform_device *pdev)
 	struct clk *clk;
 	struct device *dev = &pdev->dev;
 	struct device_node *node = pdev->dev.of_node;
+	void **irq_data = NULL;
 	struct mpfs_gpio_chip *mpfs_gpio;
 	struct gpio_irq_chip *girq;
 	int i, ret, ngpios, nirqs;
@@ -232,7 +238,8 @@ static int mpfs_gpio_probe(struct platform_device *pdev)
 
 	mpfs_gpio->base = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(mpfs_gpio->base))
-		return dev_err_probe(dev, PTR_ERR(mpfs_gpio->base), "failed to ioremap memory resource\n");
+		return dev_err_probe(dev, PTR_ERR(mpfs_gpio->base),
+				     "failed to ioremap memory resource\n");
 
 	clk = devm_clk_get(dev, NULL);
 	if (IS_ERR(clk))
@@ -266,20 +273,42 @@ static int mpfs_gpio_probe(struct platform_device *pdev)
 		ret = -ENXIO;
 		goto cleanup_clock;
 	}
+
 	girq = &mpfs_gpio->gc.irq;
-	gpio_irq_chip_set_chip(girq, &mpfs_gpio_irqchip);
-	girq->handler = handle_simple_irq;
-	girq->parent_handler = mpfs_gpio_irq_handler;
-	girq->default_type = IRQ_TYPE_NONE;
 	girq->num_parents = nirqs;
-	girq->parents = devm_kcalloc(&pdev->dev, nirqs,
-				     sizeof(*girq->parents), GFP_KERNEL);
-	if (!girq->parents) {
-		ret = -ENOMEM;
-		goto cleanup_clock;
+
+	if (girq->num_parents) {
+		gpio_irq_chip_set_chip(girq, &mpfs_gpio_irqchip);
+		girq->parent_handler = mpfs_gpio_irq_handler;
+
+		girq->parents = devm_kcalloc(&pdev->dev, girq->num_parents,
+					     sizeof(*girq->parents), GFP_KERNEL);
+		irq_data = devm_kmalloc_array(&pdev->dev, girq->num_parents,
+					      sizeof(*irq_data), GFP_KERNEL);
+
+		if (!girq->parents || !irq_data) {
+			ret = -ENOMEM;
+			goto cleanup_clock;
+		}
+
+		for (i = 0; i < girq->num_parents; i++) {
+			ret = platform_get_irq(pdev, i);
+			if (ret < 0)
+				goto cleanup_clock;
+
+			girq->parents[i] = ret;
+			mpfs_gpio->irq_data[i] = i;
+			irq_data[i] = &mpfs_gpio->irq_data[i];
+
+			irq_set_chip_data(ret, &mpfs_gpio->gc);
+			irq_set_handler(ret, handle_simple_irq);
+		}
+
+		girq->parent_handler_data_array = irq_data;
+		girq->per_parent_data = true;
+		girq->handler = handle_simple_irq;
+		girq->default_type = IRQ_TYPE_NONE;
 	}
-	for (i = 0; i < nirqs; i++)
-		girq->parents[i] = platform_get_irq(pdev, i);
 
 	ret = gpiochip_add_data(&mpfs_gpio->gc, mpfs_gpio);
 	if (ret)
-- 
2.43.2

[RFC v7 6/6] riscv: dts: microchip: update gpio interrupts to better match the SoC

[Conor Dooley]

There are 3 GPIO controllers on this SoC, of which:
- GPIO controller 0 has 14 GPIOs
- GPIO controller 1 has 24 GPIOs
- GPIO controller 2 has 32 GPIOs

All GPIOs are capable of generating interrupts, for a total of 70.
There are only 41 IRQs available however, so a configurable mux is used to
ensure all GPIOs can be used for interrupt generation.
38 of the 41 interrupts are in what the documentation calls "direct mode",
as they provide an exclusive connection from a GPIO to the PLIC.
The 3 remaining interrupts are used to mux the interrupts which do not have
a exclusive connection, one for each GPIO controller.
Setting of the mux should be done by the platform's firmware at boot, based
on the output of the "MSS Configurator" (FPGA configuration tool).

The microchip,mpfs-gpio binding suffered greatly due to being written
with a narrow minded view of the controller, and the interrupt bits
ended up incorrect. It was mistakenly assumed that the interrupt
configuration was set by platform firmware, based on the FPGA
configuration, and that the GPIO DT nodes were the only way to really
communicate interrupt configuration to software.

Instead, the mux should be a device in its own right, and the GPIO
controllers should be connected to it, rather than to the PLIC.
Now that a binding exists for that mux, fix the inaccurate description
of the interrupt controller hierarchy.

Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
---
 .../boot/dts/microchip/mpfs-icicle-kit.dts    |  8 ---
 arch/riscv/boot/dts/microchip/mpfs.dtsi       | 50 +++++++++++++++++--
 2 files changed, 45 insertions(+), 13 deletions(-)

diff --git a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts
index f80df225f72b4..7a9822d2a8819 100644
--- a/arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts
+++ b/arch/riscv/boot/dts/microchip/mpfs-icicle-kit.dts
@@ -83,14 +83,6 @@ &core_pwm0 {
 };
 
 &gpio2 {
-	interrupts = <53>, <53>, <53>, <53>,
-		     <53>, <53>, <53>, <53>,
-		     <53>, <53>, <53>, <53>,
-		     <53>, <53>, <53>, <53>,
-		     <53>, <53>, <53>, <53>,
-		     <53>, <53>, <53>, <53>,
-		     <53>, <53>, <53>, <53>,
-		     <53>, <53>, <53>, <53>;
 	status = "okay";
 };
 
diff --git a/arch/riscv/boot/dts/microchip/mpfs.dtsi b/arch/riscv/boot/dts/microchip/mpfs.dtsi
index 9883ca3554c50..e31e0aacb943b 100644
--- a/arch/riscv/boot/dts/microchip/mpfs.dtsi
+++ b/arch/riscv/boot/dts/microchip/mpfs.dtsi
@@ -465,39 +465,79 @@ mac1: ethernet@20112000 {
 			status = "disabled";
 		};
 
-		gpio0: gpio@20120000 {
-			compatible = "microchip,mpfs-gpio";
-			reg = <0x0 0x20120000 0x0 0x1000>;
+		irqmux: interrupt-controller@20002054 {
+			compatible = "microchip,mpfs-gpio-irq-mux";
+			reg = <0x0 0x20002054 0x0 0x4>;
 			interrupt-parent = <&plic>;
 			interrupt-controller;
 			#interrupt-cells = <1>;
+			interrupts = <13>, <14>, <15>, <16>,
+				     <17>, <18>, <19>, <20>,
+				     <21>, <22>, <23>, <24>,
+				     <25>, <26>, <27>, <28>,
+				     <29>, <30>, <31>, <32>,
+				     <33>, <34>, <35>, <36>,
+				     <37>, <38>, <39>, <40>,
+				     <41>, <42>, <43>, <44>,
+				     <45>, <46>, <47>, <48>,
+				     <49>, <50>, <51>, <52>,
+				     <53>;
+		};
+
+		gpio0: gpio@20120000 {
+			compatible = "microchip,mpfs-gpio";
+			reg = <0x0 0x20120000 0x0 0x1000>;
+			interrupt-parent = <&irqmux>;
+			interrupt-controller;
+			#interrupt-cells = <1>;
+			interrupts = <0>, <1>, <2>, <3>,
+				     <4>, <5>, <6>, <7>,
+				     <8>, <9>, <10>, <11>,
+				     <12>, <13>;
 			clocks = <&clkcfg CLK_GPIO0>;
 			gpio-controller;
 			#gpio-cells = <2>;
+			ngpios = <14>;
 			status = "disabled";
 		};
 
 		gpio1: gpio@20121000 {
 			compatible = "microchip,mpfs-gpio";
 			reg = <0x0 0x20121000 0x0 0x1000>;
-			interrupt-parent = <&plic>;
+			interrupt-parent = <&irqmux>;
 			interrupt-controller;
 			#interrupt-cells = <1>;
+			interrupts = <32>, <33>, <34>, <35>,
+				     <36>, <37>, <38>, <39>,
+				     <40>, <41>, <42>, <43>,
+				     <44>, <45>, <46>, <47>,
+				     <48>, <49>, <50>, <51>,
+				     <52>, <53>, <54>, <55>;
 			clocks = <&clkcfg CLK_GPIO1>;
 			gpio-controller;
 			#gpio-cells = <2>;
+			ngpios = <24>;
 			status = "disabled";
 		};
 
 		gpio2: gpio@20122000 {
 			compatible = "microchip,mpfs-gpio";
 			reg = <0x0 0x20122000 0x0 0x1000>;
-			interrupt-parent = <&plic>;
+			interrupt-parent = <&irqmux>;
 			interrupt-controller;
 			#interrupt-cells = <1>;
+			interrupts = <64>, <65>, <66>, <67>,
+				     <68>, <69>, <70>, <71>,
+				     <72>, <73>, <74>, <75>,
+				     <76>, <77>, <78>, <79>,
+				     <80>, <81>, <82>, <83>,
+				     <84>, <85>, <86>, <87>,
+				     <88>, <89>, <90>, <91>,
+				     <92>, <93>, <94>, <95>;
 			clocks = <&clkcfg CLK_GPIO2>;
 			gpio-controller;
 			#gpio-cells = <2>;
+			ngpios = <32>;
 			status = "disabled";
 		};
 
-- 
2.43.2

Re: [RFC v7 1/6] dt-bindings: gpio: fix microchip,mpfs-gpio interrupt descriptions

[Krzysztof Kozlowski]

On 23/07/2024 13:27, Conor Dooley wrote:
> The microchip,mpfs-gpio binding suffered greatly due to being written
> with a narrow minded view of the controller, and the interrupt bits
> ended up incorrect. It was mistakenly assumed that the interrupt
> configuration was set by platform firmware, based on the FPGA
> configuration, and that the GPIO DT nodes were the only way to really
> communicate interrupt configuration to software.
> 
> Instead, the mux should be a device in its own right, and the GPIO
> controllers should be connected to it, rather than to the PLIC.
> Now that a binding exists for that mux, try to fix the misconceptions
> in the GPIO controller binding.
> 
> Firstly, it's not possible for this controller to have fewer than 14
> GPIOs, and thus 14 interrupts also. There are three controllers, with
> 14, 24 & 32 GPIOs each.
> 
> The example is wacky too - it follows from the incorrect understanding
> that the GPIO controllers are connected to the PLIC directly. They are
> not however, with a mux sitting in between. Update the example to use
> the mux as a parent, and the interrupt numbers at the mux for GPIO2 as
> the example - rather than the strange looking, repeated <53>.
> 

You make ngpios required, which could be an ABI break except that there
is no Linux user for this, so there is no ABI break, right? If so, would
be nice to mention it. Rest looks good:

Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>

Best regards,
Krzysztof

Re: [RFC v7 2/6] dt-bindings: interrupt-controller: document PolarFire SoC's gpio interrupt mux

[Krzysztof Kozlowski]

On 23/07/2024 13:27, Conor Dooley wrote:
> On PolarFire SoC there are more GPIO interrupts than there are interrupt
> lines available on the PLIC, and a runtime configurable mux is used to
> decide which interrupts are assigned direct connections to the PLIC &
> which are relegated to sharing a line.
> This mux is, in our reference configuration, written by platform
> firmware during boot based on the FPGA's configuration.
> 
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> ---
>  .../microchip,mpfs-gpio-irq-mux.yaml          | 79 +++++++++++++++++++
>  1 file changed, 79 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/microchip,mpfs-gpio-irq-mux.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/microchip,mpfs-gpio-irq-mux.yaml b/Documentation/devicetree/bindings/interrupt-controller/microchip,mpfs-gpio-irq-mux.yaml
> new file mode 100644
> index 0000000000000..89ed3a630eef3
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/microchip,mpfs-gpio-irq-mux.yaml
> @@ -0,0 +1,79 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/microchip,mpfs-gpio-irq-mux.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Microchip Polarfire SoC GPIO Interrupt Mux
> +
> +maintainers:
> +  - Conor Dooley <conor.dooley@microchip.com>
> +
> +description: |
> +  There are 3 GPIO controllers on this SoC, of which:
> +  - GPIO controller 0 has 14 GPIOs
> +  - GPIO controller 1 has 24 GPIOs
> +  - GPIO controller 2 has 32 GPIOs
> +
> +  All GPIOs are capable of generating interrupts, for a total of 70.
> +  There are only 41 IRQs available however, so a configurable mux is used to
> +  ensure all GPIOs can be used for interrupt generation.
> +  38 of the 41 interrupts are in what the documentation calls "direct mode",
> +  as they provide an exclusive connection from a GPIO to the PLIC.
> +  The 3 remaining interrupts are used to mux the interrupts which do not have
> +  a exclusive connection, one for each GPIO controller.
> +
> +properties:
> +  compatible:
> +    const: microchip,mpfs-gpio-irq-mux
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 1
> +
> +  interrupts:
> +    description:
> +      The first 38 entries must be the "direct" interrupts, for exclusive
> +      connections to the PLIC. The final 3 entries must be the
> +      "non-direct"/muxed connections for each of GPIO controller 0, 1 & 2
> +      respectively.
> +    maxItems: 41
> +
> +allOf:
> +  - $ref: /schemas/interrupt-controller.yaml#

Please put allOf: after required:.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - "#interrupt-cells"
> +  - interrupt-controller

and here keep the same order as in properties, so
controller+cells+interrupts.

> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    irqmux: interrupt-controller@20002054 {
> +        compatible = "microchip,mpfs-gpio-irq-mux";
> +        reg = <0x20002054 0x4>;
> +        interrupt-parent = <&plic>;
> +        interrupt-controller;
> +        #interrupt-cells = <1>;
> +        status = "okay";

Drop status


Best regards,
Krzysztof

Re: [RFC v7 2/6] dt-bindings: interrupt-controller: document PolarFire SoC's gpio interrupt mux

[Conor Dooley]

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

On Wed, Jul 24, 2024 at 03:27:21PM +0200, Krzysztof Kozlowski wrote:

> > +examples:
> > +  - |
> > +    irqmux: interrupt-controller@20002054 {
> > +        compatible = "microchip,mpfs-gpio-irq-mux";
> > +        reg = <0x20002054 0x4>;
> > +        interrupt-parent = <&plic>;
> > +        interrupt-controller;
> > +        #interrupt-cells = <1>;
> > +        status = "okay";
> 
> Drop status

And the label too.

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

Re: [RFC v7 1/6] dt-bindings: gpio: fix microchip,mpfs-gpio interrupt descriptions

[Conor Dooley]

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

On Wed, Jul 24, 2024 at 03:25:38PM +0200, Krzysztof Kozlowski wrote:
> On 23/07/2024 13:27, Conor Dooley wrote:
> > The microchip,mpfs-gpio binding suffered greatly due to being written
> > with a narrow minded view of the controller, and the interrupt bits
> > ended up incorrect. It was mistakenly assumed that the interrupt
> > configuration was set by platform firmware, based on the FPGA
> > configuration, and that the GPIO DT nodes were the only way to really
> > communicate interrupt configuration to software.
> > 
> > Instead, the mux should be a device in its own right, and the GPIO
> > controllers should be connected to it, rather than to the PLIC.
> > Now that a binding exists for that mux, try to fix the misconceptions
> > in the GPIO controller binding.
> > 
> > Firstly, it's not possible for this controller to have fewer than 14
> > GPIOs, and thus 14 interrupts also. There are three controllers, with
> > 14, 24 & 32 GPIOs each.
> > 
> > The example is wacky too - it follows from the incorrect understanding
> > that the GPIO controllers are connected to the PLIC directly. They are
> > not however, with a mux sitting in between. Update the example to use
> > the mux as a parent, and the interrupt numbers at the mux for GPIO2 as
> > the example - rather than the strange looking, repeated <53>.
> > 
> 
> You make ngpios required, which could be an ABI break except that there
> is no Linux user for this, so there is no ABI break, right? If so, would
> be nice to mention it. Rest looks good:

No upstream user at least, and I don't believe that there are any
non-linux projects using the GPIO controllers via DT. I could, I
suppose, not make it required and use 32 as a default - but that could
cause problems with existing devicetrees where all 3 controllers omitted
the property, despite having differing numbers of GPIOs.

Now that I look again, the driver actually doesn't enforce the presence
of the property and I think it should fail to probe if not present.

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

Re: [RFC v7 3/6] irqchip: add mpfs gpio interrupt mux

[Thomas Gleixner]

On Tue, Jul 23 2024 at 12:27, Conor Dooley wrote:
> +
> +struct mpfs_irq_mux_bank_config {
> +	u32 mask;
> +	u8 shift;
> +};

Please see:

  https://www.kernel.org/doc/html/latest/process/maintainer-tip.html

vs. coding style.

> +/*
> + * Returns an unsigned long, where a set bit indicates the corresponding
> + * interrupt is in non-direct/muxed mode for that bank/GPIO controller.
> + */
> +static inline unsigned long mpfs_irq_mux_get_muxed_irqs(struct mpfs_irq_mux *priv,
> +							unsigned int bank)
> +{
> +	unsigned long mux_config = priv->mux_config, muxed_irqs = -1;
> +	struct mpfs_irq_mux_bank_config bank_config = mpfs_irq_mux_bank_configs[bank];
> +
> +	/*
> +	 * If a bit is set in the mux, GPIO the corresponding interrupt from
> +	 * controller 2 is direct and that controllers 0 or 1 is muxed.

This is not a coherent sentence.

> +	 * Invert the bits in the configuration register, so that set bits
> +	 * equate to non-direct mode, for GPIO controller 2.
> +	 */
> +	if (bank == 2u)
> +		mux_config = ~mux_config;
> +

> +static int mpfs_irq_mux_nondirect_alloc(struct irq_domain *d, unsigned int virq,
> +					struct irq_fwspec *fwspec, struct mpfs_irq_mux *priv)
> +{
> +	unsigned int bank = fwspec->param[0] / MPFS_MAX_IRQS_PER_GPIO;
> +
> +	if (bank > 2)
> +		return -EINVAL;
> +
> +	priv->nondirect_irqchips[bank].domain = d;
> +
> +	irq_domain_set_hwirq_and_chip(d, virq, fwspec->param[0],
> +				      &mpfs_irq_mux_nondirect_irq_chip, priv);
> +	irq_set_chained_handler_and_data(virq, handle_untracked_irq,

Why does this use handle_untracked_irq()? This sets up a chained handler
but handle_untracked_irq() is a regular interrupt handler.

> +					 &priv->nondirect_irqchips[bank]);

Thanks,

        tglx

Re: [RFC v7 3/6] irqchip: add mpfs gpio interrupt mux

[Conor Dooley]

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

On Mon, Jul 29, 2024 at 12:41:25PM +0200, Thomas Gleixner wrote:
> On Tue, Jul 23 2024 at 12:27, Conor Dooley wrote:
> > +
> > +struct mpfs_irq_mux_bank_config {
> > +	u32 mask;
> > +	u8 shift;
> > +};
> 
> Please see:
> 
>   https://www.kernel.org/doc/html/latest/process/maintainer-tip.html
> 
> vs. coding style.
> 
> > +/*
> > + * Returns an unsigned long, where a set bit indicates the corresponding
> > + * interrupt is in non-direct/muxed mode for that bank/GPIO controller.
> > + */
> > +static inline unsigned long mpfs_irq_mux_get_muxed_irqs(struct mpfs_irq_mux *priv,
> > +							unsigned int bank)
> > +{
> > +	unsigned long mux_config = priv->mux_config, muxed_irqs = -1;
> > +	struct mpfs_irq_mux_bank_config bank_config = mpfs_irq_mux_bank_configs[bank];
> > +
> > +	/*
> > +	 * If a bit is set in the mux, GPIO the corresponding interrupt from
> > +	 * controller 2 is direct and that controllers 0 or 1 is muxed.
> 
> This is not a coherent sentence.

It should read "controller 0 or 1;s interrupt is muxed". Does that make
more sense to you?

> > +	 * Invert the bits in the configuration register, so that set bits
> > +	 * equate to non-direct mode, for GPIO controller 2.
> > +	 */
> > +	if (bank == 2u)
> > +		mux_config = ~mux_config;
> > +
> 
> > +static int mpfs_irq_mux_nondirect_alloc(struct irq_domain *d, unsigned int virq,
> > +					struct irq_fwspec *fwspec, struct mpfs_irq_mux *priv)
> > +{
> > +	unsigned int bank = fwspec->param[0] / MPFS_MAX_IRQS_PER_GPIO;
> > +
> > +	if (bank > 2)
> > +		return -EINVAL;
> > +
> > +	priv->nondirect_irqchips[bank].domain = d;
> > +
> > +	irq_domain_set_hwirq_and_chip(d, virq, fwspec->param[0],
> > +				      &mpfs_irq_mux_nondirect_irq_chip, priv);
> > +	irq_set_chained_handler_and_data(virq, handle_untracked_irq,
> 
> Why does this use handle_untracked_irq()?

I'll have to go and dig back in my notes as to why it is untracked. It
was probably something like irqd_set() in handle_irq_event() blowing up
on the irq_data being invalid (which I figure could relate back to my
questions in the cover letter about issues with irqd_to_hwirq()) - but
I'll double check what exactly prompted it when I get back from my
holidays, but...

> This sets up a chained handler
> but handle_untracked_irq() is a regular interrupt handler.

...what I was likely using before was handle_simple_irq() which isn't
chained either. You're expecting to see mpfs_irq_mux_nondirect_handler()
here I suppose?

>+static void mpfs_irq_mux_nondirect_handler(struct irq_desc *desc)
>+{
>+	struct mpfs_irq_mux_irqchip *irqchip_data = irq_desc_get_handler_data(desc);
>+	struct mpfs_irq_mux *priv = container_of(irqchip_data, struct mpfs_irq_mux,
>+						 nondirect_irqchips[irqchip_data->bank]);
>+	unsigned long muxed_irqs;
>+	int pos;
>+
>+	chained_irq_enter(irq_desc_get_chip(desc), desc);
>+
>+	muxed_irqs = mpfs_irq_mux_get_muxed_irqs(priv, irqchip_data->bank);
>+
>+	for_each_set_bit(pos, &muxed_irqs, MPFS_MAX_IRQS_PER_GPIO)
>+		generic_handle_domain_irq(irqchip_data->domain, irqchip_data->offset + pos);
>+
>+	chained_irq_exit(irq_desc_get_chip(desc), desc);
>+}

Given you've only commented on one significant issue and two minor items,
is it safe to conclude that the overall approach doesn't have you
screaming and running for the hills?

Cheers,
Conor.

> > +					 &priv->nondirect_irqchips[bank]);

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

Re: [RFC v7 3/6] irqchip: add mpfs gpio interrupt mux

[Thomas Gleixner]

On Thu, Aug 01 2024 at 16:09, Conor Dooley wrote:
> On Mon, Jul 29, 2024 at 12:41:25PM +0200, Thomas Gleixner wrote:
>> > +	/*
>> > +	 * If a bit is set in the mux, GPIO the corresponding interrupt from
>> > +	 * controller 2 is direct and that controllers 0 or 1 is muxed.
>> 
>> This is not a coherent sentence.
>
> It should read "controller 0 or 1;s interrupt is muxed". Does that make
> more sense to you?

No: If a bit is set in the mux, GPIO the corresponding...

I'm already failing at 'GPIO'. My parser expects a verb there :)

>> > +	irq_set_chained_handler_and_data(virq, handle_untracked_irq,
>> 
>> Why does this use handle_untracked_irq()?
>
> I'll have to go and dig back in my notes as to why it is untracked. It
> was probably something like irqd_set() in handle_irq_event() blowing up
> on the irq_data being invalid (which I figure could relate back to my
> questions in the cover letter about issues with irqd_to_hwirq()) - but
> I'll double check what exactly prompted it when I get back from my
> holidays, but...
>
>> This sets up a chained handler
>> but handle_untracked_irq() is a regular interrupt handler.
>
> ...what I was likely using before was handle_simple_irq() which isn't
> chained either. You're expecting to see mpfs_irq_mux_nondirect_handler()
> here I suppose?

Yes or some other proper chained handler.

> Given you've only commented on one significant issue and two minor items,
> is it safe to conclude that the overall approach doesn't have you
> screaming and running for the hills?

I don't love it, but I don't have a better approach to deal with this.

Thanks,

        tglx

Re: [RFC v7 3/6] irqchip: add mpfs gpio interrupt mux

[Conor Dooley]

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

On Thu, Aug 01, 2024 at 08:49:08PM +0200, Thomas Gleixner wrote:
> On Thu, Aug 01 2024 at 16:09, Conor Dooley wrote:
> > On Mon, Jul 29, 2024 at 12:41:25PM +0200, Thomas Gleixner wrote:
> >> > +	/*
> >> > +	 * If a bit is set in the mux, GPIO the corresponding interrupt from
> >> > +	 * controller 2 is direct and that controllers 0 or 1 is muxed.
> >> 
> >> This is not a coherent sentence.
> >
> > It should read "controller 0 or 1;s interrupt is muxed". Does that make

Heh, I even typoed here, the ; should be a '.

> > more sense to you?
> 
> No: If a bit is set in the mux, GPIO the corresponding...
> 
> I'm already failing at 'GPIO'. My parser expects a verb there :)

Ah, so double mistake in the sentence. s/GPIO// I suppose. An updated
comment could be:

"If a bit is set in the mux, the corresponding interrupt from GPIO
controller 2 is direct and controller 0 or 1's interrupt is muxed"

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

Re: [RFC v7 3/6] irqchip: add mpfs gpio interrupt mux

[Thomas Gleixner]

On Fri, Aug 02 2024 at 09:08, Conor Dooley wrote:
> On Thu, Aug 01, 2024 at 08:49:08PM +0200, Thomas Gleixner wrote:
>> On Thu, Aug 01 2024 at 16:09, Conor Dooley wrote:
>> > On Mon, Jul 29, 2024 at 12:41:25PM +0200, Thomas Gleixner wrote:
>> >> > +	/*
>> >> > +	 * If a bit is set in the mux, GPIO the corresponding interrupt from
>> >> > +	 * controller 2 is direct and that controllers 0 or 1 is muxed.
>> >> 
>> >> This is not a coherent sentence.
>> >
>> > It should read "controller 0 or 1;s interrupt is muxed". Does that make
>
> Heh, I even typoed here, the ; should be a '.
>
>> > more sense to you?
>> 
>> No: If a bit is set in the mux, GPIO the corresponding...
>> 
>> I'm already failing at 'GPIO'. My parser expects a verb there :)
>
> Ah, so double mistake in the sentence. s/GPIO// I suppose. An updated
> comment could be:
>
> "If a bit is set in the mux, the corresponding interrupt from GPIO
> controller 2 is direct and controller 0 or 1's interrupt is muxed"

That's actually understandable for mere mortals :)

Re: [RFC v7 4/6] gpio: mpfs: add polarfire soc gpio support

[Linus Walleij]

On Tue, Jul 23, 2024 at 1:28 PM Conor Dooley <conor.dooley@microchip.com> wrote:

> From: Lewis Hanly <lewis.hanly@microchip.com>
>
> Add a driver to support the Polarfire SoC gpio controller
>
> Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>

Looks good to me.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [RFC v7 4/6] gpio: mpfs: add polarfire soc gpio support

[Linus Walleij]

On Tue, Jul 23, 2024 at 1:28 PM Conor Dooley <conor.dooley@microchip.com> wrote:


> From: Lewis Hanly <lewis.hanly@microchip.com>
>
> Add a driver to support the Polarfire SoC gpio controller
>
> Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>

Just a comment on second thought:

> +config GPIO_POLARFIRE_SOC
> +       bool "Microchip FPGA GPIO support"
> +       depends on OF_GPIO
> +       select GPIOLIB_IRQCHIP

select GPIO_GENERIC?

> +static int mpfs_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio_index)
> +{
> +       struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
> +       u32 gpio_cfg;
> +       unsigned long flags;
> +
> +       raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
> +
> +       gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
> +       gpio_cfg |= MPFS_GPIO_EN_IN;
> +       gpio_cfg &= ~(MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF);

OK this part is unique...

> +static int mpfs_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio_index, int value)
> +{
> +       struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
> +       u32 gpio_cfg;
> +       unsigned long flags;
> +
> +       raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
> +
> +       gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
> +       gpio_cfg |= MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF;

Also here

> +static int mpfs_gpio_get_direction(struct gpio_chip *gc,
> +                                  unsigned int gpio_index)
> +static int mpfs_gpio_get(struct gpio_chip *gc,
> +                        unsigned int gpio_index)
> +static void mpfs_gpio_set(struct gpio_chip *gc, unsigned int gpio_index, int value)

But these are just MMIO functions.

Is it possible to use augmented generic MMIO, i.e just override these
two functions that
need special handling?

Yours,
Linus Walleij

Re: [RFC v7 5/6] gpio: mpfs: pass gpio line number as irq data

[Linus Walleij]

Hi Conor,

thanks for your patch!

On Tue, Jul 23, 2024 at 1:28 PM Conor Dooley <conor.dooley@microchip.com> wrote:

> Since the interrupt mux is going to provide us a 1:1 mapping for
> interrupts, and it is no longer correct to hit all of the set bits in
> the interrupt handler, store the GPIO that "owns" an interrupt in its
> data pointer, so that we can determine which bit to clear.
>
> Signed-off-by: Conor Dooley <conor.dooley@microchip.com>

I don't quite get this, the irqchip of the GPIO is clearly hard-coded
hierarchical, then why don't you:

select IRQ_DOMAIN_HIERARCHY

And use e.g. girq->child_to_parent_hwirq() to handle the
hierarchy?

See drivers/gpio/gpio-ixp4xx.c for a simple example of a hierarchical
GPIO interrupt controller.

Yours,
Linus Walleij

Re: [RFC v7 4/6] gpio: mpfs: add polarfire soc gpio support

[Conor Dooley]

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

On Mon, Aug 05, 2024 at 10:04:53AM +0200, Linus Walleij wrote:
> On Tue, Jul 23, 2024 at 1:28 PM Conor Dooley <conor.dooley@microchip.com> wrote:
> 
> 
> > From: Lewis Hanly <lewis.hanly@microchip.com>
> >
> > Add a driver to support the Polarfire SoC gpio controller
> >
> > Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com>
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> 
> Just a comment on second thought:
> 
> > +config GPIO_POLARFIRE_SOC
> > +       bool "Microchip FPGA GPIO support"
> > +       depends on OF_GPIO
> > +       select GPIOLIB_IRQCHIP
> 
> select GPIO_GENERIC?
> 
> > +static int mpfs_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio_index)
> > +{
> > +       struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
> > +       u32 gpio_cfg;
> > +       unsigned long flags;
> > +
> > +       raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
> > +
> > +       gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
> > +       gpio_cfg |= MPFS_GPIO_EN_IN;
> > +       gpio_cfg &= ~(MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF);
> 
> OK this part is unique...
> 
> > +static int mpfs_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio_index, int value)
> > +{
> > +       struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
> > +       u32 gpio_cfg;
> > +       unsigned long flags;
> > +
> > +       raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
> > +
> > +       gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
> > +       gpio_cfg |= MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF;
> 
> Also here
> 
> > +static int mpfs_gpio_get_direction(struct gpio_chip *gc,
> > +                                  unsigned int gpio_index)
> > +static int mpfs_gpio_get(struct gpio_chip *gc,
> > +                        unsigned int gpio_index)
> > +static void mpfs_gpio_set(struct gpio_chip *gc, unsigned int gpio_index, int value)
> 
> But these are just MMIO functions.
> 
> Is it possible to use augmented generic MMIO, i.e just override these
> two functions that
> need special handling?

I'll look into it - as I mentioned under the --- line, I really didn't
touch most of the driver and there's comments from Lewis' submission
that still apply.

Cheers,
Conor.

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

Re: [RFC v7 5/6] gpio: mpfs: pass gpio line number as irq data

[Conor Dooley]

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

On Mon, Aug 05, 2024 at 10:11:09AM +0200, Linus Walleij wrote:
> Hi Conor,
> 
> thanks for your patch!
> 
> On Tue, Jul 23, 2024 at 1:28 PM Conor Dooley <conor.dooley@microchip.com> wrote:
> 
> > Since the interrupt mux is going to provide us a 1:1 mapping for
> > interrupts, and it is no longer correct to hit all of the set bits in
> > the interrupt handler, store the GPIO that "owns" an interrupt in its
> > data pointer, so that we can determine which bit to clear.
> >
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> 
> I don't quite get this, the irqchip of the GPIO is clearly hard-coded
> hierarchical, then why don't you:
> 
> select IRQ_DOMAIN_HIERARCHY
> 
> And use e.g. girq->child_to_parent_hwirq() to handle the
> hierarchy?
> 
> See drivers/gpio/gpio-ixp4xx.c for a simple example of a hierarchical
> GPIO interrupt controller.

Cool, I'll check that out. I've got some re-figuring out of the
interrupt controller to do given Thomas' comment there. Maybe the
combination will solve the horrible % 32 hack...

Cheers,
Conor.

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

Re: [RFC v7 4/6] gpio: mpfs: add polarfire soc gpio support

[Linus Walleij]

On Tue, Aug 6, 2024 at 7:18 PM Conor Dooley <conor@kernel.org> wrote:
> On Mon, Aug 05, 2024 at 10:04:53AM +0200, Linus Walleij wrote:

> > Is it possible to use augmented generic MMIO, i.e just override these
> > two functions that
> > need special handling?
>
> I'll look into it - as I mentioned under the --- line, I really didn't
> touch most of the driver and there's comments from Lewis' submission
> that still apply.

Thanks Conor, thanks for taking this over, too many patch sets fall
on the floor. I'm mostly fine merging it like this and then improving
it in-tree as well, I'm not as insistent on things being perfect before
merging as long as they are maintained.

Yours,
Linus Walleij

Re: [RFC v7 4/6] gpio: mpfs: add polarfire soc gpio support

[Conor Dooley]

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

On Wed, Aug 07, 2024 at 06:55:58PM +0200, Linus Walleij wrote:
> On Tue, Aug 6, 2024 at 7:18 PM Conor Dooley <conor@kernel.org> wrote:
> > On Mon, Aug 05, 2024 at 10:04:53AM +0200, Linus Walleij wrote:
> 
> > > Is it possible to use augmented generic MMIO, i.e just override these
> > > two functions that
> > > need special handling?
> >
> > I'll look into it - as I mentioned under the --- line, I really didn't
> > touch most of the driver and there's comments from Lewis' submission
> > that still apply.
> 
> Thanks Conor, thanks for taking this over, too many patch sets fall
> on the floor.

Meh, it always bugged me that upstreaming driver was given up on because
fixing the interrupts was "hard". It'd be a poor example to others if I
le the driver sit downstream as a result.

> I'm mostly fine merging it like this and then improving
> it in-tree as well, I'm not as insistent on things being perfect before
> merging as long as they are maintained.

The gpio side of things might not be too bad, but the irqchip side is
crap (it has an of_iomap() in it for example*), and if the irqchip driver
needs work it feels sensible to improve on both before merging anything.
Unless of course, you think it would be reasonable to rip the interrupt
support out of the gpio driver, merge that and incrementally add it
while also improving the things you and the earlier review mentioned
w.r.t. regmap?

Cheers,
Conor.

* Getting rid of the of_iomap() needs me to rewrite parts of the clock
driver, which seemed overkill until I knew whether or not the approach
was permitted.

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

Re: [RFC v7 4/6] gpio: mpfs: add polarfire soc gpio support

[Conor Dooley]

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

On Mon, Aug 05, 2024 at 10:04:53AM +0200, Linus Walleij wrote:
> On Tue, Jul 23, 2024 at 1:28 PM Conor Dooley <conor.dooley@microchip.com> wrote:
> 
> 
> > From: Lewis Hanly <lewis.hanly@microchip.com>
> >
> > Add a driver to support the Polarfire SoC gpio controller
> >
> > Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com>
> > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> 
> Just a comment on second thought:
> 
> > +config GPIO_POLARFIRE_SOC
> > +       bool "Microchip FPGA GPIO support"
> > +       depends on OF_GPIO
> > +       select GPIOLIB_IRQCHIP
> 
> select GPIO_GENERIC?
> 
> > +static int mpfs_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio_index)
> > +{
> > +       struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
> > +       u32 gpio_cfg;
> > +       unsigned long flags;
> > +
> > +       raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
> > +
> > +       gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
> > +       gpio_cfg |= MPFS_GPIO_EN_IN;
> > +       gpio_cfg &= ~(MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF);
> 
> OK this part is unique...
> 
> > +static int mpfs_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio_index, int value)
> > +{
> > +       struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
> > +       u32 gpio_cfg;
> > +       unsigned long flags;
> > +
> > +       raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
> > +
> > +       gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
> > +       gpio_cfg |= MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF;
> 
> Also here
> 
> > +static int mpfs_gpio_get_direction(struct gpio_chip *gc,
> > +                                  unsigned int gpio_index)
> > +static int mpfs_gpio_get(struct gpio_chip *gc,
> > +                        unsigned int gpio_index)
> > +static void mpfs_gpio_set(struct gpio_chip *gc, unsigned int gpio_index, int value)
> 
> But these are just MMIO functions.
> 
> Is it possible to use augmented generic MMIO, i.e just override these
> two functions that
> need special handling?

So, I've been looking into this again (finally), with an eye to stripping
the interrupt handling bits out, and trying to upstream this in pieces.
I dunno if I'm making a mistake here, but I don't know if there's much
value in implementing this suggestion - as far as I can tell only the
get()/set() functions can be replaced by what's provided by gpio-mmio.c.
There are no controller wide registers that control direction and so
bgpio_get_dir() can't be used - direction is read from the same
mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index) registers that it is set
using. Adding bgpio stuff, to just go ahead and overwrite it, to save on
trivial get()/set() implementations seems to me like adding complication
rather than removing it. What am I missing here?

Cheers,
Conor.

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

Re: [RFC v7 4/6] gpio: mpfs: add polarfire soc gpio support

[Conor Dooley]

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

On Wed, Oct 16, 2024 at 10:56:32AM +0100, Conor Dooley wrote:
> On Mon, Aug 05, 2024 at 10:04:53AM +0200, Linus Walleij wrote:
> > On Tue, Jul 23, 2024 at 1:28 PM Conor Dooley <conor.dooley@microchip.com> wrote:
> > 
> > 
> > > From: Lewis Hanly <lewis.hanly@microchip.com>
> > >
> > > Add a driver to support the Polarfire SoC gpio controller
> > >
> > > Signed-off-by: Lewis Hanly <lewis.hanly@microchip.com>
> > > Signed-off-by: Conor Dooley <conor.dooley@microchip.com>
> > 
> > Just a comment on second thought:
> > 
> > > +config GPIO_POLARFIRE_SOC
> > > +       bool "Microchip FPGA GPIO support"
> > > +       depends on OF_GPIO
> > > +       select GPIOLIB_IRQCHIP
> > 
> > select GPIO_GENERIC?
> > 
> > > +static int mpfs_gpio_direction_input(struct gpio_chip *gc, unsigned int gpio_index)
> > > +{
> > > +       struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
> > > +       u32 gpio_cfg;
> > > +       unsigned long flags;
> > > +
> > > +       raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
> > > +
> > > +       gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
> > > +       gpio_cfg |= MPFS_GPIO_EN_IN;
> > > +       gpio_cfg &= ~(MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF);
> > 
> > OK this part is unique...
> > 
> > > +static int mpfs_gpio_direction_output(struct gpio_chip *gc, unsigned int gpio_index, int value)
> > > +{
> > > +       struct mpfs_gpio_chip *mpfs_gpio = gpiochip_get_data(gc);
> > > +       u32 gpio_cfg;
> > > +       unsigned long flags;
> > > +
> > > +       raw_spin_lock_irqsave(&mpfs_gpio->lock, flags);
> > > +
> > > +       gpio_cfg = readl(mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index));
> > > +       gpio_cfg |= MPFS_GPIO_EN_OUT | MPFS_GPIO_EN_OUT_BUF;
> > 
> > Also here
> > 
> > > +static int mpfs_gpio_get_direction(struct gpio_chip *gc,
> > > +                                  unsigned int gpio_index)
> > > +static int mpfs_gpio_get(struct gpio_chip *gc,
> > > +                        unsigned int gpio_index)
> > > +static void mpfs_gpio_set(struct gpio_chip *gc, unsigned int gpio_index, int value)
> > 
> > But these are just MMIO functions.
> > 
> > Is it possible to use augmented generic MMIO, i.e just override these
> > two functions that
> > need special handling?
> 
> So, I've been looking into this again (finally), with an eye to stripping
> the interrupt handling bits out, and trying to upstream this in pieces.
> I dunno if I'm making a mistake here, but I don't know if there's much
> value in implementing this suggestion - as far as I can tell only the
> get()/set() functions can be replaced by what's provided by gpio-mmio.c.
> There are no controller wide registers that control direction and so
> bgpio_get_dir() can't be used - direction is read from the same
> mpfs_gpio->base + MPFS_GPIO_CTRL(gpio_index) registers that it is set
> using. Adding bgpio stuff, to just go ahead and overwrite it, to save on
> trivial get()/set() implementations seems to me like adding complication
> rather than removing it. What am I missing here?

What does bring a nice simplification though, IMO, is regmap. I am
pretty sure that using it was one of the suggestions made last time
Lewis submitted this - so I think I'm going to do that instead.

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

Re: [RFC v7 4/6] gpio: mpfs: add polarfire soc gpio support

[Linus Walleij]

On Wed, Oct 16, 2024 at 11:56 AM Conor Dooley <conor@kernel.org> wrote:

> I dunno if I'm making a mistake here, but I don't know if there's much
> value in implementing this suggestion - as far as I can tell only the
> get()/set() functions can be replaced by what's provided by gpio-mmio.c.

You're not making any mistake, you know the driver and hw better
than me.

Just skip the GPIO_MMIO if you think the result would look ugly.

You can add my:
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: [RFC v7 4/6] gpio: mpfs: add polarfire soc gpio support

[Linus Walleij]

On Wed, Oct 16, 2024 at 12:29 PM Conor Dooley <conor@kernel.org> wrote:

> What does bring a nice simplification though, IMO, is regmap. I am
> pretty sure that using it was one of the suggestions made last time
> Lewis submitted this - so I think I'm going to do that instead.

If you have the time. Using GPIO_REGMAP for MMIO is not that
common and I think the driver is pretty neat as it stands.

Yours,
Linus Walleij

Re: [RFC v7 4/6] gpio: mpfs: add polarfire soc gpio support

[Conor Dooley]

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

On Wed, Oct 16, 2024 at 09:26:13PM +0200, Linus Walleij wrote:
> On Wed, Oct 16, 2024 at 12:29 PM Conor Dooley <conor@kernel.org> wrote:
> 
> > What does bring a nice simplification though, IMO, is regmap. I am
> > pretty sure that using it was one of the suggestions made last time
> > Lewis submitted this - so I think I'm going to do that instead.
> 
> If you have the time. Using GPIO_REGMAP for MMIO is not that
> common and I think the driver is pretty neat as it stands.

As with using the common MMIO stuff, I don't think GPIO_REGMAP provides
that much value as I cannot use the direction stuff from it. I was
thinking of using regmap directly, like:
https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=gpio-no-irq&id=c8933e1e3600e3fa29efe28fbb2e343e133f9d67
which I think reduces how ugly the two direction functions look.

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

Re: [RFC v7 4/6] gpio: mpfs: add polarfire soc gpio support

[Conor Dooley]

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

On Wed, Oct 16, 2024 at 08:42:51PM +0100, Conor Dooley wrote:
> On Wed, Oct 16, 2024 at 09:26:13PM +0200, Linus Walleij wrote:
> > On Wed, Oct 16, 2024 at 12:29 PM Conor Dooley <conor@kernel.org> wrote:
> > 
> > > What does bring a nice simplification though, IMO, is regmap. I am
> > > pretty sure that using it was one of the suggestions made last time
> > > Lewis submitted this - so I think I'm going to do that instead.
> > 
> > If you have the time. Using GPIO_REGMAP for MMIO is not that
> > common and I think the driver is pretty neat as it stands.
> 
> As with using the common MMIO stuff, I don't think GPIO_REGMAP provides
> that much value as I cannot use the direction stuff from it. I was
> thinking of using regmap directly, like:
> https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=gpio-no-irq&id=c8933e1e3600e3fa29efe28fbb2e343e133f9d67
> which I think reduces how ugly the two direction functions look.

Sorry to bother you Linus, but I was hoping to see some sort of comment
here before I squash this stuff and submit a new version. Is something
like what I linked above acceptable?

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

Re: [RFC v7 4/6] gpio: mpfs: add polarfire soc gpio support

[Linus Walleij]

On Tue, Oct 22, 2024 at 6:28 PM Conor Dooley <conor@kernel.org> wrote:
> On Wed, Oct 16, 2024 at 08:42:51PM +0100, Conor Dooley wrote:
> > On Wed, Oct 16, 2024 at 09:26:13PM +0200, Linus Walleij wrote:
> > > On Wed, Oct 16, 2024 at 12:29 PM Conor Dooley <conor@kernel.org> wrote:
> > >
> > > > What does bring a nice simplification though, IMO, is regmap. I am
> > > > pretty sure that using it was one of the suggestions made last time
> > > > Lewis submitted this - so I think I'm going to do that instead.
> > >
> > > If you have the time. Using GPIO_REGMAP for MMIO is not that
> > > common and I think the driver is pretty neat as it stands.
> >
> > As with using the common MMIO stuff, I don't think GPIO_REGMAP provides
> > that much value as I cannot use the direction stuff from it. I was
> > thinking of using regmap directly, like:
> > https://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git/commit/?h=gpio-no-irq&id=c8933e1e3600e3fa29efe28fbb2e343e133f9d67
> > which I think reduces how ugly the two direction functions look.
>
> Sorry to bother you Linus, but I was hoping to see some sort of comment
> here before I squash this stuff and submit a new version. Is something
> like what I linked above acceptable?

I pretty much trust your judgement on this, I'm fine with either solution,
the patch is also perfectly fine already as it is unless you want to polish it
further.

Yours,
Linus Walleij