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

[Drew Fustini <dfustini@xxxxxxxxxxxx>]

On Thu, Oct 21, 2021 at 09:50:42PM +0200, Emil Renner Berthing wrote:
> 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.

Ah, it seems I once knew this back in July [1] but never got the
documentation changed:

NOTE: Table 12-9 in the JH7100 datasheet is incorrect regarding fields
GPIOIE_0 and GPIOIE_1. An interrupt is enabled (unmasked) when the bit
is   set to 1 and it is disabled (masked) when set to 0. The datasheet
incorrectly states the opposite. I think this is due to the datasheet
author thinking of it as mask field which it is not, it is an enable
field. I will raise an issue on the documentation repo.


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

[1] https://github.com/esmil/linux/pull/34/commits/e247a259e40312d0202cdbdd686dbba09afc7813