| Message ID | cover.1649533972.git.sander@svanheule.net |
|---|---|
| Headers | show |
| Series | Support for RTL930x/RTL931x GPIOs | expand |
On Sat, Apr 9, 2022 at 9:56 PM Sander Vanheule <sander@svanheule.net> wrote: > > This patch series adds support for the GPIO controllers as found on the > RTL930x and RTL931x SoC families of MIPS CPUs, used in managed NBase-T > ethernet switches. > > The RTL931x's GPIO controller behaves the same as the one in the older > RTL838x and RTL839x series. This controller is trivially supported. > > The RTL930x's controller has a reversed port order, and supports CPU > affinity settings for individual GPIO line IRQs, thus requiring two > additional changes to implement these features. > > Sander Vanheule (6): > dt-bindings: gpio: realtek-otto: Add rtl9300 compatible > gpio: realtek-otto: Support reversed port layouts > gpio: realtek-otto: Support per-cpu interrupts > gpio: realtek-otto: Add RTL930x support > dt-bindings: gpio: realtek-otto: Add rtl9310 compatible > gpio: realtek-otto: Add RTL931x support > > .../bindings/gpio/realtek,otto-gpio.yaml | 34 ++++- > drivers/gpio/gpio-realtek-otto.c | 137 +++++++++++++++++- > 2 files changed, 164 insertions(+), 7 deletions(-) > > -- > 2.35.1 > Queued the entire series, thanks! Bart
On Sat, Apr 9, 2022 at 9:56 PM Sander Vanheule <sander@svanheule.net> wrote: > + if (dev_flags & GPIO_PORTS_REVERSED) { > + bgpio_flags = 0; > + ctrl->port_offset_u8 = realtek_gpio_port_offset_u8_rev; > + ctrl->port_offset_u16 = realtek_gpio_port_offset_u16_rev; > + } else { > + bgpio_flags = BGPIOF_BIG_ENDIAN_BYTE_ORDER; > + ctrl->port_offset_u8 = realtek_gpio_port_offset_u8; > + ctrl->port_offset_u16 = realtek_gpio_port_offset_u16; > + } Just checking: is this really a different silicon block, or is this GPIO_PORTS_REVERSED flag passed around just a way of saying: if (!IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) ...? Yours, Linus Walleij
On Sat, Apr 9, 2022 at 9:56 PM Sander Vanheule <sander@svanheule.net> wrote: > On SoCs with multiple cores, it is possible that the GPIO interrupt > controller supports assigning specific pins to one or more cores. > > IRQ balancing can be performed on a line-by-line basis if the parent > interrupt is routed to all available cores, which is the default upon > initialisation. > > Signed-off-by: Sander Vanheule <sander@svanheule.net> That sounds complicated. Sounds like something the IRQ maintainer (Marc Z) should have a quick look at. Yours, Linus Walleij
Hi Linus, On Thu, 2022-04-21 at 01:01 +0200, Linus Walleij wrote: > On Sat, Apr 9, 2022 at 9:56 PM Sander Vanheule <sander@svanheule.net> wrote: > > > + if (dev_flags & GPIO_PORTS_REVERSED) { > > + bgpio_flags = 0; > > + ctrl->port_offset_u8 = realtek_gpio_port_offset_u8_rev; > > + ctrl->port_offset_u16 = realtek_gpio_port_offset_u16_rev; > > + } else { > > + bgpio_flags = BGPIOF_BIG_ENDIAN_BYTE_ORDER; > > + ctrl->port_offset_u8 = realtek_gpio_port_offset_u8; > > + ctrl->port_offset_u16 = realtek_gpio_port_offset_u16; > > + } > > Just checking: is this really a different silicon block, or is this > GPIO_PORTS_REVERSED flag passed around just a way of saying: > > if (!IS_ENABLED(CONFIG_CPU_BIG_ENDIAN)) ...? The kernel for RTL930x SoC is built with CONFIG_CPU_BIG_ENDIAN=y, just like the older SoCs that were previously supported. The SoC's IRQ controller is also the same across RTL930x/RTL839x/RTL838x, even though 32-bit registers are used there. On RTL838x/RTL839x the GPIO IRQ control registers have byte layout: [H1] [L1] [H2] [L2] [H3] [L3] [H4] [L4] On RTL930x, the GPIO IRQ control registers are: [H2] [L2] [H1] [L1] [H4] [L4] [H3] [L3] which is the reverse of: [L1] [H1] [L2] [H2] [L3] [H3] [L4] [H4] Same for the GPIO registers: On RTL83xx: [P1] [P2] [P3] [P4] (four 8b ports) On RTL930x: [P4] [P3] [P2] [P1] (one BE32 port) It looks like the RTL930x could use a little-endian interpretation of the 32b registers, followed by a little-endian interpretation of the contained port values. That would mean two reorderings for every 16b read or write operation, and manual manipulation of the register values. Although I have to say that the current offset calculation is not too pretty either. We also discussed this with Andy with the original submission of the driver: https://lore.kernel.org/linux-gpio/CAHp75VdrqE0kBwzK9Jk7pZGjyfFnhatfa8UY0z-3T1w1PrbAbw@mail.gmail.com/ Best, Sander
On Thu, 21 Apr 2022 00:04:16 +0100, Linus Walleij <linus.walleij@linaro.org> wrote: > > On Sat, Apr 9, 2022 at 9:56 PM Sander Vanheule <sander@svanheule.net> wrote: > > > On SoCs with multiple cores, it is possible that the GPIO interrupt > > controller supports assigning specific pins to one or more cores. > > > > IRQ balancing can be performed on a line-by-line basis if the parent > > interrupt is routed to all available cores, which is the default upon > > initialisation. > > > > Signed-off-by: Sander Vanheule <sander@svanheule.net> > > That sounds complicated. > > Sounds like something the IRQ maintainer (Marc Z) should > have a quick look at. This is pretty odd indeed. There seem to be a direct mapping between the GPIOs and the CPU it interrupts (or at least that's what the code seem to express). However, I don't see a direct relation between the CPUs and the chained interrupt. It isn't even clear if this interrupt itself is per-CPU. So this begs a few questions: - is the affinity actually affecting the target CPU? or is it affecting the target mux? - how is the affinity of the mux interrupt actually enforced? M.
Hi Linus, Marc, On Thu, 2022-04-21 at 10:48 +0100, Marc Zyngier wrote: > On Thu, 21 Apr 2022 00:04:16 +0100, > Linus Walleij <linus.walleij@linaro.org> wrote: > > > > On Sat, Apr 9, 2022 at 9:56 PM Sander Vanheule <sander@svanheule.net> wrote: > > > > > On SoCs with multiple cores, it is possible that the GPIO interrupt > > > controller supports assigning specific pins to one or more cores. > > > > > > IRQ balancing can be performed on a line-by-line basis if the parent > > > interrupt is routed to all available cores, which is the default upon > > > initialisation. > > > > > > Signed-off-by: Sander Vanheule <sander@svanheule.net> > > > > That sounds complicated. > > > > Sounds like something the IRQ maintainer (Marc Z) should > > have a quick look at. > > This is pretty odd indeed. There seem to be a direct mapping between > the GPIOs and the CPU it interrupts (or at least that's what the code > seem to express). However, I don't see a direct relation between the > CPUs and the chained interrupt. It isn't even clear if this interrupt > itself is per-CPU. > > So this begs a few questions: > > - is the affinity actually affecting the target CPU? or is it > affecting the target mux? > > - how is the affinity of the mux interrupt actually enforced? There are three interrupt controllers at play here: 1. MIPS CPU interrupt controller: drivers/irqchip/irq-mips-cpu.c One interrupt controller per VPE, so in this case there are two. Provides per-CPU interrupts. 2. SoC interrupt controller: drivers/irqchip/irq-realtek-rtl.c Also one interrupt controller per VPE. I suppose these will also be per- CPU, although this isn't implemented in the driver yet, and I don't think I yet fully understand how should work in the kernel. 3. GPIO interrupt controller: drivers/gpio/gpio-realtek-otto.c One interrupt controller for the entire GPIO bank, with optional configurable affinity (this patch) for the different VPEs. For the RTL839x series of SoCs, this results in the following: GPIO LINES SOC IRQ MIPS +--------+ +-----------+ HW IRQ +--------+ --->| GPIO | | SOC IRQ | LINES | IRQ | --->| BANK |-----o-->| VPE0 CTRL |=========>| VPE0 | . | | | +-----------+ +--------+ . +--------+ | . | | +-----------+ +--------+ \-->| SOC IRQ | | IRQ | | VPE1 CTRL |=========>| VPE1 | +-----------+ +--------+ For RTL930x, where GPIO IRQ affinity is configurable: GPIO LINES SOC IRQ MIPS +--------+ +-----------+ HW IRQ +--------+ --->| GPIO |-------->| SOC IRQ | LINES | IRQ | --->| BANK | | VPE0 CTRL |=========>| VPE0 | . | |-----\ +-----------+ +--------+ . +--------+ | . | | +-----------+ +--------+ \-->| SOC IRQ | | IRQ | | VPE1 CTRL |=========>| VPE1 | +-----------+ +--------+ The interrupt for the GPIO controller can be muxed to any of the MIPS HW interrupts on any (or all) of the VPEs, and these muxes (SoC IRQ controllers) can be configured independently per CPU. The SoC IRQ line index is fixed, and consistent for both VPEs. Only in the second diagram can individual GPIO interrupts be muxed to any of the VPEs, but there is still only one IRQ line per VPE for all selected GPIO lines. I hopes this helps to clarify the situation. We don't have any real documentation, so this is basically derived from registers descriptions in SDK headers and testing the interrupt behaviour. Best, Sander
On Fri, 2022-04-22 at 09:04 +0200, Sander Vanheule wrote: > Hi Linus, Marc, > > On Thu, 2022-04-21 at 10:48 +0100, Marc Zyngier wrote: > > On Thu, 21 Apr 2022 00:04:16 +0100, > > Linus Walleij <linus.walleij@linaro.org> wrote: > > > > > > On Sat, Apr 9, 2022 at 9:56 PM Sander Vanheule <sander@svanheule.net> wrote: > > > > > > > On SoCs with multiple cores, it is possible that the GPIO interrupt > > > > controller supports assigning specific pins to one or more cores. > > > > > > > > IRQ balancing can be performed on a line-by-line basis if the parent > > > > interrupt is routed to all available cores, which is the default upon > > > > initialisation. > > > > > > > > Signed-off-by: Sander Vanheule <sander@svanheule.net> > > > > > > That sounds complicated. > > > > > > Sounds like something the IRQ maintainer (Marc Z) should > > > have a quick look at. > > > > This is pretty odd indeed. There seem to be a direct mapping between > > the GPIOs and the CPU it interrupts (or at least that's what the code > > seem to express). However, I don't see a direct relation between the > > CPUs and the chained interrupt. It isn't even clear if this interrupt > > itself is per-CPU. > > > > So this begs a few questions: > > > > - is the affinity actually affecting the target CPU? or is it > > affecting the target mux? > > > > - how is the affinity of the mux interrupt actually enforced? > > There are three interrupt controllers at play here: > 1. MIPS CPU interrupt controller: drivers/irqchip/irq-mips-cpu.c > One interrupt controller per VPE, so in this case there are two. Provides > per-CPU interrupts. > 2. SoC interrupt controller: drivers/irqchip/irq-realtek-rtl.c > Also one interrupt controller per VPE. I suppose these will also be per- > CPU, although this isn't implemented in the driver yet, and I don't think > I yet fully understand how should work in the kernel. > 3. GPIO interrupt controller: drivers/gpio/gpio-realtek-otto.c > One interrupt controller for the entire GPIO bank, with optional > configurable affinity (this patch) for the different VPEs. > > For the RTL839x series of SoCs, this results in the following: Sorry for the messed up formattng, let me try that again. GPIO LINES SOC IRQ MIPS +--------+ LINE +-----------+ HW IRQ +--------+ --->| GPIO | | SOC IRQ | LINES | IRQ | --->| BANK |-----o-->| VPE0 CTRL |=========>| VPE0 | . | | | +-----------+ +--------+ . +--------+ | . | | +-----------+ +--------+ \-->| SOC IRQ | | IRQ | | VPE1 CTRL |=========>| VPE1 | +-----------+ +--------+ > For RTL930x, where GPIO IRQ affinity is configurable: GPIO LINES SOC IRQ MIPS +--------+ LINE +-----------+ HW IRQ +--------+ --->| GPIO |-------->| SOC IRQ | LINES | IRQ | --->| BANK | | VPE0 CTRL |=========>| VPE0 | . | |-----\ +-----------+ +--------+ . +--------+ | . | | +-----------+ +--------+ \-->| SOC IRQ | | IRQ | | VPE1 CTRL |=========>| VPE1 | +-----------+ +--------+ > The interrupt for the GPIO controller can be muxed to any of the MIPS HW > interrupts on any (or all) of the VPEs, and these muxes (SoC IRQ controllers) > can be configured independently per CPU. The SoC IRQ line index is fixed, and > consistent for both VPEs. > Only in the second diagram can individual GPIO interrupts be muxed to any of the > VPEs, but there is still only one IRQ line per VPE for all selected GPIO lines. > > I hopes this helps to clarify the situation. We don't have any real > documentation, so this is basically derived from registers descriptions in SDK > headers and testing the interrupt behaviour. > > Best, > Sander
On Thu, Apr 21, 2022 at 9:55 AM Sander Vanheule <sander@svanheule.net> wrote: > The kernel for RTL930x SoC is built with CONFIG_CPU_BIG_ENDIAN=y, just like the > older SoCs that were previously supported. The SoC's IRQ controller is also the > same across RTL930x/RTL839x/RTL838x, even though 32-bit registers are used > there. > > On RTL838x/RTL839x the GPIO IRQ control registers have byte layout: > [H1] [L1] [H2] [L2] > [H3] [L3] [H4] [L4] > > On RTL930x, the GPIO IRQ control registers are: > [H2] [L2] [H1] [L1] > [H4] [L4] [H3] [L3] > which is the reverse of: > [L1] [H1] [L2] [H2] > [L3] [H3] [L4] [H4] > > > Same for the GPIO registers: > On RTL83xx: [P1] [P2] [P3] [P4] (four 8b ports) > On RTL930x: [P4] [P3] [P2] [P1] (one BE32 port) > > It looks like the RTL930x could use a little-endian interpretation of the 32b > registers, followed by a little-endian interpretation of the contained port > values. That would mean two reorderings for every 16b read or write operation, > and manual manipulation of the register values. Although I have to say that the > current offset calculation is not too pretty either. I'm happy. It's not very invasive and the bulk of the problem is addressed by simply using the GPIO MMIO library, so: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> If someone knows a more elegant way, they can send a patch, this works so we should merge it. Yours, Linus Walleij