| Message ID | 7105ae382d7b328102f66b39ffd7c94998e85265.1649533972.git.sander@svanheule.net |
|---|---|
| State | Accepted |
| Commit | 512c5be35223d9baa2629efa1084cf5210eaee80 |
| Headers | show |
| Series | Support for RTL930x/RTL931x GPIOs | expand |
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
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, 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
diff --git a/drivers/gpio/gpio-realtek-otto.c b/drivers/gpio/gpio-realtek-otto.c index bd75401b549d..c838ad8ce55f 100644 --- a/drivers/gpio/gpio-realtek-otto.c +++ b/drivers/gpio/gpio-realtek-otto.c @@ -58,6 +58,8 @@ struct realtek_gpio_ctrl { raw_spinlock_t lock; u16 intr_mask[REALTEK_GPIO_PORTS_PER_BANK]; u16 intr_type[REALTEK_GPIO_PORTS_PER_BANK]; + unsigned int (*port_offset_u8)(unsigned int port); + unsigned int (*port_offset_u16)(unsigned int port); }; /* Expand with more flags as devices with other quirks are added */ @@ -69,6 +71,11 @@ enum realtek_gpio_flags { * line the IRQ handler was assigned to, causing uncaught interrupts. */ GPIO_INTERRUPTS_DISABLED = BIT(0), + /* + * Port order is reversed, meaning DCBA register layout for 1-bit + * fields, and [BA, DC] for 2-bit fields. + */ + GPIO_PORTS_REVERSED = BIT(1), }; static struct realtek_gpio_ctrl *irq_data_to_ctrl(struct irq_data *data) @@ -86,21 +93,50 @@ static struct realtek_gpio_ctrl *irq_data_to_ctrl(struct irq_data *data) * port. The two interrupt mask registers store two bits per GPIO, so use u16 * values. */ +static unsigned int realtek_gpio_port_offset_u8(unsigned int port) +{ + return port; +} + +static unsigned int realtek_gpio_port_offset_u16(unsigned int port) +{ + return 2 * port; +} + +/* + * Reversed port order register access + * + * For registers with one bit per GPIO, all ports are stored as u8-s in one + * register in reversed order. The two interrupt mask registers store two bits + * per GPIO, so use u16 values. The first register contains ports 1 and 0, the + * second ports 3 and 2. + */ +static unsigned int realtek_gpio_port_offset_u8_rev(unsigned int port) +{ + return 3 - port; +} + +static unsigned int realtek_gpio_port_offset_u16_rev(unsigned int port) +{ + return 2 * (port ^ 1); +} + static void realtek_gpio_write_imr(struct realtek_gpio_ctrl *ctrl, unsigned int port, u16 irq_type, u16 irq_mask) { - iowrite16(irq_type & irq_mask, ctrl->base + REALTEK_GPIO_REG_IMR + 2 * port); + iowrite16(irq_type & irq_mask, + ctrl->base + REALTEK_GPIO_REG_IMR + ctrl->port_offset_u16(port)); } static void realtek_gpio_clear_isr(struct realtek_gpio_ctrl *ctrl, unsigned int port, u8 mask) { - iowrite8(mask, ctrl->base + REALTEK_GPIO_REG_ISR + port); + iowrite8(mask, ctrl->base + REALTEK_GPIO_REG_ISR + ctrl->port_offset_u8(port)); } static u8 realtek_gpio_read_isr(struct realtek_gpio_ctrl *ctrl, unsigned int port) { - return ioread8(ctrl->base + REALTEK_GPIO_REG_ISR + port); + return ioread8(ctrl->base + REALTEK_GPIO_REG_ISR + ctrl->port_offset_u8(port)); } /* Set the rising and falling edge mask bits for a GPIO port pin */ @@ -250,6 +286,7 @@ MODULE_DEVICE_TABLE(of, realtek_gpio_of_match); static int realtek_gpio_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; + unsigned long bgpio_flags; unsigned int dev_flags; struct gpio_irq_chip *girq; struct realtek_gpio_ctrl *ctrl; @@ -277,10 +314,20 @@ static int realtek_gpio_probe(struct platform_device *pdev) raw_spin_lock_init(&ctrl->lock); + 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; + } + err = bgpio_init(&ctrl->gc, dev, 4, ctrl->base + REALTEK_GPIO_REG_DATA, NULL, NULL, ctrl->base + REALTEK_GPIO_REG_DIR, NULL, - BGPIOF_BIG_ENDIAN_BYTE_ORDER); + bgpio_flags); if (err) { dev_err(dev, "unable to init generic GPIO"); return err;
The GPIO port layout on the RTL930x SoC series is reversed compared to the RTL838x and RTL839x SoC series. Add new port offset calculator functions to ensure the correct order is used when reading port IRQ data, and ensure bgpio uses the right byte ordering. Signed-off-by: Sander Vanheule <sander@svanheule.net> --- drivers/gpio/gpio-realtek-otto.c | 55 +++++++++++++++++++++++++++++--- 1 file changed, 51 insertions(+), 4 deletions(-)