Re: [PATCH v2 12/16] pinctrl: starfive: Add pinctrl driver for StarFive SoCs

[Emil Renner Berthing <kernel@xxxxxxxx>]

On Thu, 21 Oct 2021 at 21:01, Drew Fustini <dfustini@xxxxxxxxxxxx> wrote:
> On Thu, Oct 21, 2021 at 07:42:19PM +0200, Emil Renner Berthing wrote:
> > +/*
> > + * Interrupt Trigger Polarity. If set to 1 the interrupt is triggered on a
> > + * rising edge (edge-triggered) or high level (level-triggered). If set to 0 the
> > + * interrupt is triggered on a falling edge (edge-triggered) or low level
> > + * (level-triggered).
> > + */
> > +#define GPIOIEV              0x020
> > +
> > +/*
> > + * Interrupt Mask. If set to 1 the interrupt is disabled (masked). If set to 0
> > + * the interrupt is enabled (unmasked).
> > + */
> > +#define GPIOIE               0x028
>
> It bothered me that the datasheet used the term GPIOIE for the interrupt
> mask register. I had used a more verbose #define name because I worried
> someone reading GPIOIE in functions might mistake it for an interrupt
> enable register. This happened to me when I was originally working with
> the gpio driver.
>
> However I suppose the best solution would have been to get the datasheet
> updated as I can see how it is best to have #define names in the driver
> match the datasheet.
>
> > +static void starfive_irq_mask(struct irq_data *d)
> > +{
> > +     struct starfive_pinctrl *sfp = starfive_from_irq_data(d);
> > +     irq_hw_number_t gpio = irqd_to_hwirq(d);
> > +     void __iomem *ie = sfp->base + GPIOIE + 4 * (gpio / 32);
> > +     u32 mask = BIT(gpio % 32);
> > +     unsigned long flags;
> > +     u32 value;
> > +
> > +     raw_spin_lock_irqsave(&sfp->lock, flags);
> > +     value = readl_relaxed(ie) & ~mask;
> > +     writel_relaxed(value, ie);
> > +     raw_spin_unlock_irqrestore(&sfp->lock, flags);
> > +}
> > +
> > +static void starfive_irq_mask_ack(struct irq_data *d)
> > +{
> > +     struct starfive_pinctrl *sfp = starfive_from_irq_data(d);
> > +     irq_hw_number_t gpio = irqd_to_hwirq(d);
> > +     void __iomem *ie = sfp->base + GPIOIE + 4 * (gpio / 32);
> > +     void __iomem *ic = sfp->base + GPIOIC + 4 * (gpio / 32);
> > +     u32 mask = BIT(gpio % 32);
> > +     unsigned long flags;
> > +     u32 value;
> > +
> > +     raw_spin_lock_irqsave(&sfp->lock, flags);
> > +     value = readl_relaxed(ie) & ~mask;
> > +     writel_relaxed(value, ie);
> > +     writel_relaxed(mask, ic);
> > +     raw_spin_unlock_irqrestore(&sfp->lock, flags);
> > +}
> > +
> > +static void starfive_irq_unmask(struct irq_data *d)
> > +{
> > +     struct starfive_pinctrl *sfp = starfive_from_irq_data(d);
> > +     irq_hw_number_t gpio = irqd_to_hwirq(d);
> > +     void __iomem *ie = sfp->base + GPIOIE + 4 * (gpio / 32);
> > +     u32 mask = BIT(gpio % 32);
> > +     unsigned long flags;
> > +     u32 value;
> > +
> > +     raw_spin_lock_irqsave(&sfp->lock, flags);
> > +     value = readl_relaxed(ie) | mask;
> > +     writel_relaxed(value, ie);
> > +     raw_spin_unlock_irqrestore(&sfp->lock, flags);
> > +}
> > +

...

> > +static int starfive_gpio_init_hw(struct gpio_chip *gc)
> > +{
> > +     struct starfive_pinctrl *sfp = starfive_from_gc(gc);
> > +
> > +     /* mask all GPIO interrupts */
> > +     writel(0, sfp->base + GPIOIE + 0);
> > +     writel(0, sfp->base + GPIOIE + 4);
>
> Woudln't 0 in GPIOIE mean mask is disabled for all interrupts?
>
> In other words, wouldn't this enable all the interrupts?

Heh, you're right. The code does the exact opposite of what the
documentation says it should be doing. However I just tried and with
the code as it is now GPIO interrupts work fine, but with the logic
flipped the kernel fails to boot. I'm guessing because an interrupt
storm. So it seems to me the documentation might be wrong and GPIOIE
is actually a good name.

Michael Zhu: Can you confirm if a 1 or 0 enables the interrupt in the
GPIOIE registers?

/Emil