Re: [PATCH v1] gpio: realtek-otto: switch to 32-bit I/O

[Sander Vanheule]

Hi Andy,

Looks like you sent your reply as HTML, so replying back in plaintext.

On Sat, 2022-07-23 at 18:07 +0200, Andy Shevchenko wrote:
> 
> 
> On Saturday, July 23, 2022, Sander Vanheule <sander@xxxxxxxxxxxxx> wrote:
> > By using 16-bit I/O on the GPIO peripheral, which is apparently not safe
> > on MIPS, the IMR can end up containing garbage. This then results in
> > interrupt triggers for lines that don't have an interrupt source
> > associated. The irq_desc lookup fails, and the ISR will not be cleared,
> > keeping the CPU busy until reboot, or until another IMR operation
> > restores the correct value. This situation appears to happen very
> > rarely, in < 0.5% of IMR writes.
> > 
> > Instead of using 8-bit or 16-bit I/O operations on the 32-bit memory
> > mapped peripheral registers, switch to using 32-bit I/O only. This
> > allows to put all the GPIO lines in-order for 8-bit port values. For
> > 16-bit values, stick to manual (un)packing of per-port values.
> > 
> > Cc: Paul Cercueil <paul@xxxxxxxxxxxxxxx>
> > Signed-off-by: Sander Vanheule <sander@xxxxxxxxxxxxx>
> > ---
> >  drivers/gpio/gpio-realtek-otto.c | 122 ++++++++++++++++++-------------
> >  1 file changed, 73 insertions(+), 49 deletions(-)

[..]

> > +static void realtek_gpio_port_write16be(void __iomem *reg, unsigned int
> > port, u16 value)
> > +{
> > +       unsigned int shift = (port % 2) * 16;
> > +       u32 reg_val;
> > +
> > +       reg += 4 * (port / 2);
> > +       reg_val = ioread32be(reg) & ~(GENMASK(15, 0) << shift);
> > +       reg_val |= swab16(value) << shift;
> > +       iowrite32be(reg_val, reg);
> >  }

[..]

> > +static void realtek_gpio_port_write16(void __iomem *reg, unsigned int port,
> > u16 value)
> > +{
> > +       unsigned int shift = (port % 2) * 16;
> > +       u32 reg_val;
> > +
> > +       reg += 4 * (port / 2);
> > +       reg_val = ioread32(reg) & ~(GENMASK(15, 0) << shift);
> > +       reg_val |= value << shift;
> > +       iowrite32(reg_val, reg);
> >  }
> > 
> > +
> >  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 + ctrl-
> > >port_offset_u16(port));
> > +       ctrl->port_write16(ctrl->base + REALTEK_GPIO_REG_IMR, port, irq_type
> > & irq_mask);
> >  }

[..]

> > @@ -307,16 +328,17 @@ static int realtek_gpio_irq_set_affinity(struct
> > irq_data *data,
> >  static int realtek_gpio_irq_init(struct gpio_chip *gc)
> >  {
> >         struct realtek_gpio_ctrl *ctrl = gpiochip_get_data(gc);
> > +       u32 mask_all = GENMASK(gc->ngpio, 0);
> >         unsigned int port;
> >         int cpu;
> > 
> > -       for (port = 0; (port * 8) < gc->ngpio; port++) {
> > +       for (port = 0; (port * 8) < gc->ngpio; port++)
> >                 realtek_gpio_write_imr(ctrl, port, 0, 0);
> > 
> 
> 
> port++ ??? Is it correct code? Logically it seems you do 8 writes to the same
> port. Maybe this is the real issue?

port will go up to at most 3, since there is a limit on ngpio of 32. For the
initialisation, this means the driver will do at most 4 RMW operations instead
of 2 writes, but this way I can use realtek_gpio_write_imr() consistently for
all IMR changes.

The issue really is trying to use 16-bit (and 8-bit) writes on 32-bit MMIO
registers. iowrite16() works most of the time though, which is why I didn't spot
this issue initially. I've tested this by writing and reading back the IMR
values many times. With 16-bit I/O, it fails for ~35 iterations out of 10000.
With 32-bit I/O the IMR port-value comes back as what was written to it every
single time. ioread16() seems to give correct results, but iowrite16() may cause
the other half of the register to turn into garbage.

I've changed the IRQ operations on the four 8-bit port values into a single 32-
bit operation, which is also what happens inside gpio-mmio for the gpio part.
The IMR registers contain two bits per GPIO line, which why I've kept this
different implementation, manipulating the 8-pin ports instead of the entire 32-
pin bank in one go.

We can discuss the way 32-bit I/O is implemented, but AFAICT the driver simply
cannot use iowrite16() reliably.

Best,
Sander