Message-ID: <CAP6Zq1h5TXBcfvxDA5Ae9XuW65s_zQd_JrM3vRWR6KBRYCHRrg@mail.gmail.com>
Date:   Thu, 26 Jul 2018 03:01:52 +0300
From:   Tomer Maimon <tmaimon77@...il.com>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        Avi Fishman <avifishman70@...il.com>,
        Nancy Yuen <yuenn@...gle.com>,
        Brendan Higgins <brendanhiggins@...gle.com>,
        Patrick Venture <venture@...gle.com>,
        Joel Stanley <joel@....id.au>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "open list:GPIO SUBSYSTEM" <linux-gpio@...r.kernel.org>,
        OpenBMC Maillist <openbmc@...ts.ozlabs.org>
Subject: Re: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver

Hi Linus,

Thanks a lot for your comments.

Sorry for my late reply, I was on vacation.

The last days I have been working to move NPCM pinctrl GPIO to GENERIC
GPIO, most of the work have been done but I had the some issue.

I initialize bgpio as follow:

                        ret = bgpio_init(&pctrl->gpio_bank[id].gc,
                                         pctrl->dev, 4,
                                         pctrl->gpio_bank[id].base +
                                         NPCM7XX_GP_N_DIN,
                                         pctrl->gpio_bank[id].base +
                                         NPCM7XX_GP_N_DOUT,
                                         NULL,
                                         NULL,
                                         pctrl->gpio_bank[id].base +
                                         NPCM7XX_GP_N_IEM,
                                         BGPIOF_READ_OUTPUT_REG_SET);

After doing it, the directions functions I used are:
bgpio_dir_out_inv, bgpio_dir_in_inv, bgpio_get_dir_inv

and the I/O get function is bgpio_get_set

By using inv directions:

direction out = 0 (gc->bgpio_dir &= ~bgpio_line2mask(gc, gpio))

direction in = 1 (gc->bgpio_dir |= bgpio_line2mask(gc, gpio))

The problem occur when reading the GPIO value from bgpio_get_set
function, because the directions value are inverse it reading the
wrong I/O registers

For direction out it reading dat register (instead of set register)

For direction in it calling set register (instead of dat register)

	if (gc->bgpio_dir & pinmask)
		return !!(gc->read_reg(gc->reg_set) & pinmask);
	else
		return !!(gc->read_reg(gc->reg_dat) & pinmask);
the same issue occur at bgpio_get_set_multiple function.

Maybe in bgpio_dir parameter direction out should be in both cases 1
and direction in = 0.

for now i did a local fix in the npcm pinctrl driver by setting
bgpio_dir parameters as direction out = 1 and direction in = 0.

Thanks a lot,

Tomer









On 13 July 2018 at 11:51, Linus Walleij <linus.walleij@...aro.org> wrote:

> On Thu, Jul 12, 2018 at 11:42 PM Tomer Maimon <tmaimon77@...il.com> wrote:
>
> > Add Nuvoton BMC NPCM750/730/715/705 Pinmux and
> > GPIO controller driver.
> >
> > Signed-off-by: Tomer Maimon <tmaimon77@...il.com>
>
> (...)
> > +++ b/drivers/pinctrl/nuvoton/pinctrl-npcm7xx.c
> > @@ -0,0 +1,2089 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +// Copyright (c) 2016-2018 Nuvoton Technology corporation.
> > +// Copyright (c) 2016, Dell Inc
> > +
> > +#include <linux/device.h>
> > +#include <linux/gpio.h>
>
> As this is a driver you should only include <linux/gpio/driver.h>
>
> > +#include <linux/interrupt.h>
> > +#include <linux/irq.h>
> > +#include <linux/mfd/syscon.h>
>
> If you need syscon then the driver should select or depend
> on MFD_SYSCON in Kconfig.
>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_irq.h>
>
> Do you really need this include?
>
> > +/* Structure for register banks */
> > +struct NPCM7XX_GPIO {
>
> Can we have this lowercase? Please?
>
> > +       void __iomem            *base;
> > +       struct gpio_chip        gc;
> > +       int                     irqbase;
> > +       int                     irq;
> > +       spinlock_t              lock;
> > +       void                    *priv;
> > +       struct irq_chip         irq_chip;
> > +       u32                     pinctrl_id;
> > +};
>
> So each GPIO bank has its own gpio chip and register
> base, that is NICE! Because then it looks like you can
> select GPIO_GENERIC and use the MMIO GPIO helper
> library to handle the registers. Let's look into that
> option!
>
> > +struct NPCM7xx_pinctrl {
> > +       struct pinctrl_dev      *pctldev;
> > +       struct device           *dev;
> > +       struct NPCM7XX_GPIO     gpio_bank[NPCM7XX_GPIO_BANK_NUM];
> > +       struct irq_domain       *domain;
>
> I wonder why the pin controller needs and IRQ domain but
> I keep reading the code and I might find out...
>
> > +enum operand {
> > +       op_set,
> > +       op_getbit,
> > +       op_setbit,
> > +       op_clrbit,
> > +};
>
> This looks complicated. I suspect you can use GPIO_GENERIC
> to set/get and clear bits in the register(s).
>
> > +/* Perform locked bit operations on GPIO registers */
> > +static int gpio_bitop(struct NPCM7XX_GPIO *bank, int op, unsigned int
> offset,
> > +                     int reg)
> > +{
> > +       unsigned long flags;
> > +       u32 mask, val;
> > +
> > +       mask = (1L << offset);
> > +       spin_lock_irqsave(&bank->lock, flags);
> > +       switch (op) {
> > +       case op_set:
> > +               iowrite32(mask, bank->base + reg);
> > +               break;
> > +       case op_getbit:
> > +               mask &= ioread32(bank->base + reg);
> > +               break;
> > +       case op_setbit:
> > +               val = ioread32(bank->base + reg);
> > +               iowrite32(val | mask, bank->base + reg);
> > +               break;
> > +       case op_clrbit:
> > +               val = ioread32(bank->base + reg);
> > +               iowrite32(val & (~mask), bank->base + reg);
> > +               break;
> > +       }
> > +       spin_unlock_irqrestore(&bank->lock, flags);
> > +       return !!mask;
> > +}
>
> This is essentially a reimplementation of drivers/gpio/gpio-mmio.c
> (GPIO_GENERIC, also using a spinlock to protect the registers)
> so let's use that instead :)
>
> There are drivers already that reuse the spinlock inside the
> generic GPIO chip to protect their other registers like for
> IRQ registers.
>
> > +static int npcmgpio_get_direction(struct gpio_chip *chip, unsigned int
> offset)
> > +{
> > +       struct NPCM7XX_GPIO *bank = gpiochip_get_data(chip);
> > +       u32 oe, ie;
> > +
> > +       /* Get Input & Output state */
> > +       ie = gpio_bitop(bank, op_getbit, offset, NPCM7XX_GP_N_IEM);
> > +       oe = gpio_bitop(bank, op_getbit, offset, NPCM7XX_GP_N_OE);
> > +       if (ie && !oe)
> > +               return GPIOF_DIR_IN;
> > +       else if (oe && !ie)
> > +               return GPIOF_DIR_OUT;
>
> These are consumer flags and should not be used in drivers.
> Use plain 0/1 instead.
>
> Anyways the problem goes away with GPIO_GENERIC.
>
> > +static int npcmgpio_direction_input(struct gpio_chip *chip, unsigned
> int offset)
> > +{
> > +       return pinctrl_gpio_direction_input(offset + chip->base);
> > +}
>
> It's a bit tricksy to get this to work with GPIO_GENERIC.
>
> After calling bgpio_init() you need to overwrite the assigned
> .direction_input handler with this and then direct back to the
> one assigned by GPIO_GENERIC.
>
> Something like this:
>
> 1. Add two indirection pointers to the npcm7xx_gpio state container:
>
> struct npcm7xx_gpio {
>      (...)
>       int (*direction_input)(struct gpio_chip *chip, unsigned offset);
>       int (*direction_output)(struct gpio_chip *chip, unsigned offset,
> int value);
>      (...)
> };
>
> 2. Save the pointers
>
> struct npcm7xx_gpio *npcm;
>
> bgpio_init( ... register setup ...)
> npcm->direction_input = npcm->gc.direction_input;
> npcm->direction_output = npcm->gc.direction_output;
> npcm->gc.direction_input = npcmgpio_direction_input;
> npcm->gc.direction_output = npcmgpio_direction_output;
>
> 3. Modify the functions like that:
>
> static int npcmgpio_direction_input(struct gpio_chip *chip, unsigned int
> offset)
> {
>     struct npcm7xx_gpio *npcm = gpiochip_get_data(chip);
>     int ret;
>
>     ret = pinctrl_gpio_direction_input(offset + chip->base);
>     if (ret)
>         return ret;
>     return npcm->direction_input(chip);
> }
>
> I'm sure you get the idea... if you think we can modify gpio-mmio
> to be more helpful with this, suggestions are welcome!
>
> > +/* Set GPIO to Output with initial value */
> > +static int npcmgpio_direction_output(struct gpio_chip *chip,
> > +                                    unsigned int offset, int value)
> > +{
> > +       struct NPCM7XX_GPIO *bank = gpiochip_get_data(chip);
> > +
> > +       dev_dbg(chip->parent, "gpio_direction_output: offset%d = %x\n",
> offset,
> > +               value);
> > +
> > +       /* Check if we're enabled as an interrupt.. */
> > +       if (gpio_bitop(bank, op_getbit, offset, NPCM7XX_GP_N_EVEN) &&
> > +           gpio_bitop(bank, op_getbit, offset, NPCM7XX_GP_N_IEM)) {
> > +               dev_dbg(chip->parent,
> > +                       "gpio_direction_output: IRQ enabled on
> offset%d\n",
> > +                       offset);
> > +               return -EINVAL;
> > +       }
>
> This should not be necessary as you are using GPIOLIB_IRQCHIP,
> which locks the GPIO for interrupt and disallows this to happen.
>
> > +static int npcmgpio_gpio_request(struct gpio_chip *chip, unsigned int
> offset)
> > +{
> > +       dev_dbg(chip->parent, "gpio_request: offset%d\n", offset);
> > +       return pinctrl_gpio_request(offset + chip->base);
> > +}
> > +
> > +static void npcmgpio_gpio_free(struct gpio_chip *chip, unsigned int
> offset)
> > +{
> > +       dev_dbg(chip->parent, "gpio_free: offset%d\n", offset);
> > +       pinctrl_gpio_free(offset + chip->base);
> > +}
>
> This needs the same pattern as the direction functions above, then
> you can use GPIO_GENERIC (mmio).
>
> > +static unsigned int npcmgpio_irq_startup(struct irq_data *d)
> > +{
> > +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> > +       unsigned int gpio = d->hwirq;
> > +
> > +       /* active-high, input, clear interrupt, enable interrupt */
> > +       dev_dbg(d->chip->parent_device, "startup: %u.%u\n", gpio,
> d->irq);
> > +       npcmgpio_direction_output(gc, gpio, 1);
> > +       npcmgpio_direction_input(gc, gpio);
>
> Interesting dance. So it is required to set the line to
> 1 and then switch to input?
>
> > +static struct irq_chip npcmgpio_irqchip = {
> > +       .name = "NPCM7XX-GPIO-IRQ",
> > +       .irq_ack = npcmgpio_irq_ack,
> > +       .irq_unmask = npcmgpio_irq_unmask,
> > +       .irq_mask = npcmgpio_irq_mask,
> > +       .irq_set_type = npcmgpio_set_irq_type,
> > +       .irq_startup = npcmgpio_irq_startup,
> > +};
>
> This code is looking good BTW.
>
> The patch in my inbox just ends in the middle of everything, I wonder
> why :( suspect the new gmail interface I'm using.
>
> Anyways: the pointers above should keep you busy for the next
> iteration of the patch, the pin control part seems pretty straight-forward.
>
> Yours,
> Linus Walleij
>

Content of type "text/html" skipped

Powered by blists - more mailing lists