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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 21 Oct 2021 19:06:27 -0700
From:   Drew Fustini <dfustini@...libre.com>
To:     Emil Renner Berthing <kernel@...il.dk>
Cc:     linux-riscv <linux-riscv@...ts.infradead.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        linux-clk <linux-clk@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        "open list:SERIAL DRIVERS" <linux-serial@...r.kernel.org>,
        Palmer Dabbelt <palmer@...belt.com>,
        Paul Walmsley <paul.walmsley@...ive.com>,
        Rob Herring <robh+dt@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Marc Zyngier <maz@...nel.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Linus Walleij <linus.walleij@...aro.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Daniel Lezcano <daniel.lezcano@...aro.org>,
        Andy Shevchenko <andriy.shevchenko@...ux.intel.com>,
        Jiri Slaby <jirislaby@...nel.org>,
        Maximilian Luz <luzmaximilian@...il.com>,
        Sagar Kadam <sagar.kadam@...ive.com>,
        Drew Fustini <drew@...gleboard.org>,
        Geert Uytterhoeven <geert@...ux-m68k.org>,
        Michael Zhu <michael.zhu@...rfivetech.com>,
        Fu Wei <tekkamanninja@...il.com>,
        Anup Patel <anup.patel@....com>,
        Atish Patra <atish.patra@....com>,
        Matteo Croce <mcroce@...rosoft.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Huan Feng <huan.feng@...rfivetech.com>
Subject: Re: [PATCH v2 12/16] pinctrl: starfive: Add pinctrl driver for
 StarFive SoCs

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@...libre.com> 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