[RFC,v7,0/6] PolarFire SoC GPIO support

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

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

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.