<div dir="ltr">Hi Linus,<div><br></div><div>Thanks a lot for your comments.</div><div><br></div><div>Sorry for my late reply, I was on vacation.</div><div><br></div><div>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.</div><div><br></div><div>I initialize bgpio as follow:</div><div>
<div style="background-color:rgb(255,255,255)">
<pre style="color:rgb(0,0,0);font-weight:normal;text-decoration-line:none;font-size:11pt;font-family:"Courier New""> ret = <span style="font-weight:bolder">bgpio_init</span>(&pctrl->gpio_bank<span style="color:rgb(128,0,0)">[</span><span style="color:rgb(128,0,128)">id</span><span style="color:rgb(128,0,0)">]</span>.gc,
pctrl->dev, <span style="color:rgb(0,0,128)">4</span>,
pctrl->gpio_bank<span style="color:rgb(128,0,0)">[</span><span style="color:rgb(128,0,128)">id</span><span style="color:rgb(128,0,0)">]</span>.base +
NPCM7XX_GP_N_DIN,
pctrl->gpio_bank<span style="color:rgb(128,0,0)">[</span><span style="color:rgb(128,0,128)">id</span><span style="color:rgb(128,0,0)">]</span>.base +
NPCM7XX_GP_N_DOUT,
<span style="color:rgb(192,64,0)">NULL</span>,
<span style="color:rgb(192,64,0)">NULL</span>,
pctrl->gpio_bank<span style="color:rgb(128,0,0)">[</span><span style="color:rgb(128,0,128)">id</span><span style="color:rgb(128,0,0)">]</span>.base +
NPCM7XX_GP_N_IEM,
BGPIOF_READ_OUTPUT_REG_SET);
</pre><pre style="color:rgb(0,0,0);font-weight:normal;text-decoration-line:none"><font face="arial, helvetica, sans-serif">After doing it, the directions functions I used are: bgpio_dir_out_inv, bgpio_dir_in_inv, bgpio_get_dir_inv </font></pre><pre style="color:rgb(0,0,0);font-weight:normal;text-decoration-line:none"><font face="arial, helvetica, sans-serif"><pre style="text-decoration-style:initial;text-decoration-color:initial"><font face="arial, helvetica, sans-serif">and the I/O get function is bgpio_get_set </font></pre><pre style="text-decoration-style:initial;text-decoration-color:initial"><font face="arial, helvetica, sans-serif">By using inv directions:</font></pre></font></pre><pre style="color:rgb(0,0,0);font-weight:normal;text-decoration-line:none"><font face="arial, helvetica, sans-serif">direction out = 0 (</font>gc->bgpio_dir<font face="monospace, monospace"> </font><span style="font-family:arial,sans-serif">&= ~</span><span style="font-family:"Courier New";font-size:11pt;font-weight:bolder">bgpio_line2mask</span><span style="font-family:"Courier New";font-size:11pt">(gc, gpio))</span><br></pre><pre style="color:rgb(0,0,0);font-weight:normal;text-decoration-line:none"><pre style="text-decoration-style:initial;text-decoration-color:initial"><font face="arial, helvetica, sans-serif">direction in = 1 (</font>gc->bgpio_dir |= <span style="font-family:"Courier New";font-size:11pt;font-weight:bolder">bgpio_line2mask</span><span style="font-family:"Courier New";font-size:11pt">(gc, gpio)</span><span style="font-family:"Courier New";font-size:11pt">)</span><font face="arial, helvetica, sans-serif"><br></font></pre></pre><pre style="color:rgb(0,0,0);font-weight:normal;text-decoration-line:none"><font face="arial, helvetica, sans-serif">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</font></pre><pre style="color:rgb(0,0,0);font-weight:normal;text-decoration-line:none"><font face="arial, helvetica, sans-serif">For direction out it reading dat register (instead of set register)</font></pre><pre style="color:rgb(0,0,0);font-weight:normal;text-decoration-line:none"><font face="arial, helvetica, sans-serif">For direction in it calling set register (instead of dat register)</font></pre><pre><font color="#000000"> if (gc->bgpio_dir & pinmask)
return !!(gc->read_reg(gc->reg_set) & pinmask);
else
return !!(gc->read_reg(gc->reg_dat) & pinmask);
<font face="arial, helvetica, sans-serif">the same issue occur at </font></font><font face="arial, helvetica, sans-serif">bgpio_get_set_multiple function.</font></pre><pre><font face="arial, helvetica, sans-serif">Maybe in bgpio_dir parameter<font style="color:rgb(0,0,0)"> </font>direction out should be in both cases 1 and direction in = 0.</font></pre><pre><font face="arial, helvetica, sans-serif">for now i did a local fix in the npcm pinctrl driver by setting bgpio_dir<font style="color:rgb(0,0,0)"> </font></font><font style="font-family:arial,helvetica,sans-serif;color:rgb(0,0,0)">parameters as </font><span style="font-family:arial,helvetica,sans-serif">direction out = 1 and direction in = 0.</span></pre><pre><font face="arial, helvetica, sans-serif">Thanks a lot,</font></pre><pre><font face="arial, helvetica, sans-serif">Tomer</font></pre><pre><pre style="text-decoration-style:initial;text-decoration-color:initial"><font face="arial, helvetica, sans-serif">
</font>
<font face="arial, helvetica, sans-serif"><font style="color:rgb(0,0,0)"><br>
</font></font></pre>
</pre><pre><font style=""><font face="monospace, monospace" style="color:rgb(0,0,0)">
</font></font><br>
</pre><pre><font color="#000000">
</font></pre></div></div></div><div class="gmail_extra"><br><div class="gmail_quote">On 13 July 2018 at 11:51, Linus Walleij <span dir="ltr"><<a href="mailto:linus.walleij@linaro.org" target="_blank">linus.walleij@linaro.org</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On Thu, Jul 12, 2018 at 11:42 PM Tomer Maimon <<a href="mailto:tmaimon77@gmail.com">tmaimon77@gmail.com</a>> wrote:<br>
<br>
> Add Nuvoton BMC NPCM750/730/715/705 Pinmux and<br>
> GPIO controller driver.<br>
><br>
> Signed-off-by: Tomer Maimon <<a href="mailto:tmaimon77@gmail.com">tmaimon77@gmail.com</a>><br>
<br>
</span>(...)<br>
<span class="">> +++ b/drivers/pinctrl/nuvoton/<wbr>pinctrl-npcm7xx.c<br>
> @@ -0,0 +1,2089 @@<br>
> +// SPDX-License-Identifier: GPL-2.0<br>
> +// Copyright (c) 2016-2018 Nuvoton Technology corporation.<br>
> +// Copyright (c) 2016, Dell Inc<br>
> +<br>
> +#include <linux/device.h><br>
> +#include <linux/gpio.h><br>
<br>
</span>As this is a driver you should only include <linux/gpio/driver.h><br>
<span class=""><br>
> +#include <linux/interrupt.h><br>
> +#include <linux/irq.h><br>
> +#include <linux/mfd/syscon.h><br>
<br>
</span>If you need syscon then the driver should select or depend<br>
on MFD_SYSCON in Kconfig.<br>
<span class=""><br>
> +#include <linux/module.h><br>
> +#include <linux/of.h><br>
> +#include <linux/of_address.h><br>
> +#include <linux/of_irq.h><br>
<br>
</span>Do you really need this include?<br>
<span class=""><br>
> +/* Structure for register banks */<br>
> +struct NPCM7XX_GPIO {<br>
<br>
</span>Can we have this lowercase? Please?<br>
<span class=""><br>
> + void __iomem *base;<br>
> + struct gpio_chip gc;<br>
> + int irqbase;<br>
> + int irq;<br>
> + spinlock_t lock;<br>
> + void *priv;<br>
> + struct irq_chip irq_chip;<br>
> + u32 pinctrl_id;<br>
> +};<br>
<br>
</span>So each GPIO bank has its own gpio chip and register<br>
base, that is NICE! Because then it looks like you can<br>
select GPIO_GENERIC and use the MMIO GPIO helper<br>
library to handle the registers. Let's look into that<br>
option!<br>
<span class=""><br>
> +struct NPCM7xx_pinctrl {<br>
> + struct pinctrl_dev *pctldev;<br>
> + struct device *dev;<br>
> + struct NPCM7XX_GPIO gpio_bank[NPCM7XX_GPIO_BANK_<wbr>NUM];<br>
> + struct irq_domain *domain;<br>
<br>
</span>I wonder why the pin controller needs and IRQ domain but<br>
I keep reading the code and I might find out...<br>
<span class=""><br>
> +enum operand {<br>
> + op_set,<br>
> + op_getbit,<br>
> + op_setbit,<br>
> + op_clrbit,<br>
> +};<br>
<br>
</span>This looks complicated. I suspect you can use GPIO_GENERIC<br>
to set/get and clear bits in the register(s).<br>
<div><div class="h5"><br>
> +/* Perform locked bit operations on GPIO registers */<br>
> +static int gpio_bitop(struct NPCM7XX_GPIO *bank, int op, unsigned int offset,<br>
> + int reg)<br>
> +{<br>
> + unsigned long flags;<br>
> + u32 mask, val;<br>
> +<br>
> + mask = (1L << offset);<br>
> + spin_lock_irqsave(&bank->lock, flags);<br>
> + switch (op) {<br>
> + case op_set:<br>
> + iowrite32(mask, bank->base + reg);<br>
> + break;<br>
> + case op_getbit:<br>
> + mask &= ioread32(bank->base + reg);<br>
> + break;<br>
> + case op_setbit:<br>
> + val = ioread32(bank->base + reg);<br>
> + iowrite32(val | mask, bank->base + reg);<br>
> + break;<br>
> + case op_clrbit:<br>
> + val = ioread32(bank->base + reg);<br>
> + iowrite32(val & (~mask), bank->base + reg);<br>
> + break;<br>
> + }<br>
> + spin_unlock_irqrestore(&bank-><wbr>lock, flags);<br>
> + return !!mask;<br>
> +}<br>
<br>
</div></div>This is essentially a reimplementation of drivers/gpio/gpio-mmio.c<br>
(GPIO_GENERIC, also using a spinlock to protect the registers)<br>
so let's use that instead :)<br>
<br>
There are drivers already that reuse the spinlock inside the<br>
generic GPIO chip to protect their other registers like for<br>
IRQ registers.<br>
<span class=""><br>
> +static int npcmgpio_get_direction(struct gpio_chip *chip, unsigned int offset)<br>
> +{<br>
> + struct NPCM7XX_GPIO *bank = gpiochip_get_data(chip);<br>
> + u32 oe, ie;<br>
> +<br>
> + /* Get Input & Output state */<br>
> + ie = gpio_bitop(bank, op_getbit, offset, NPCM7XX_GP_N_IEM);<br>
> + oe = gpio_bitop(bank, op_getbit, offset, NPCM7XX_GP_N_OE);<br>
> + if (ie && !oe)<br>
> + return GPIOF_DIR_IN;<br>
> + else if (oe && !ie)<br>
> + return GPIOF_DIR_OUT;<br>
<br>
</span>These are consumer flags and should not be used in drivers.<br>
Use plain 0/1 instead.<br>
<br>
Anyways the problem goes away with GPIO_GENERIC.<br>
<span class=""><br>
> +static int npcmgpio_direction_input(<wbr>struct gpio_chip *chip, unsigned int offset)<br>
> +{<br>
</span>> + return pinctrl_gpio_direction_input(<wbr>offset + chip->base);<br>
> +}<br>
<br>
It's a bit tricksy to get this to work with GPIO_GENERIC.<br>
<br>
After calling bgpio_init() you need to overwrite the assigned<br>
.direction_input handler with this and then direct back to the<br>
one assigned by GPIO_GENERIC.<br>
<br>
Something like this:<br>
<br>
1. Add two indirection pointers to the npcm7xx_gpio state container:<br>
<br>
struct npcm7xx_gpio {<br>
(...)<br>
int (*direction_input)(struct gpio_chip *chip, unsigned offset);<br>
int (*direction_output)(struct gpio_chip *chip, unsigned offset,<br>
int value);<br>
(...)<br>
};<br>
<br>
2. Save the pointers<br>
<br>
struct npcm7xx_gpio *npcm;<br>
<br>
bgpio_init( ... register setup ...)<br>
npcm->direction_input = npcm->gc.direction_input;<br>
npcm->direction_output = npcm->gc.direction_output;<br>
npcm->gc.direction_input = npcmgpio_direction_input;<br>
npcm->gc.direction_output = npcmgpio_direction_output;<br>
<br>
3. Modify the functions like that:<br>
<span class=""><br>
static int npcmgpio_direction_input(<wbr>struct gpio_chip *chip, unsigned int offset)<br>
</span>{<br>
struct npcm7xx_gpio *npcm = gpiochip_get_data(chip);<br>
int ret;<br>
<br>
ret = pinctrl_gpio_direction_input(<wbr>offset + chip->base);<br>
if (ret)<br>
return ret;<br>
return npcm->direction_input(chip);<br>
}<br>
<br>
I'm sure you get the idea... if you think we can modify gpio-mmio<br>
to be more helpful with this, suggestions are welcome!<br>
<span class=""><br>
> +/* Set GPIO to Output with initial value */<br>
> +static int npcmgpio_direction_output(<wbr>struct gpio_chip *chip,<br>
> + unsigned int offset, int value)<br>
> +{<br>
> + struct NPCM7XX_GPIO *bank = gpiochip_get_data(chip);<br>
> +<br>
> + dev_dbg(chip->parent, "gpio_direction_output: offset%d = %x\n", offset,<br>
> + value);<br>
> +<br>
> + /* Check if we're enabled as an interrupt.. */<br>
> + if (gpio_bitop(bank, op_getbit, offset, NPCM7XX_GP_N_EVEN) &&<br>
> + gpio_bitop(bank, op_getbit, offset, NPCM7XX_GP_N_IEM)) {<br>
> + dev_dbg(chip->parent,<br>
> + "gpio_direction_output: IRQ enabled on offset%d\n",<br>
> + offset);<br>
> + return -EINVAL;<br>
> + }<br>
<br>
</span>This should not be necessary as you are using GPIOLIB_IRQCHIP,<br>
which locks the GPIO for interrupt and disallows this to happen.<br>
<span class=""><br>
> +static int npcmgpio_gpio_request(struct gpio_chip *chip, unsigned int offset)<br>
> +{<br>
> + dev_dbg(chip->parent, "gpio_request: offset%d\n", offset);<br>
> + return pinctrl_gpio_request(offset + chip->base);<br>
> +}<br>
> +<br>
> +static void npcmgpio_gpio_free(struct gpio_chip *chip, unsigned int offset)<br>
> +{<br>
> + dev_dbg(chip->parent, "gpio_free: offset%d\n", offset);<br>
> + pinctrl_gpio_free(offset + chip->base);<br>
> +}<br>
<br>
</span>This needs the same pattern as the direction functions above, then<br>
you can use GPIO_GENERIC (mmio).<br>
<span class=""><br>
> +static unsigned int npcmgpio_irq_startup(struct irq_data *d)<br>
> +{<br>
> + struct gpio_chip *gc = irq_data_get_irq_chip_data(d);<br>
> + unsigned int gpio = d->hwirq;<br>
> +<br>
> + /* active-high, input, clear interrupt, enable interrupt */<br>
> + dev_dbg(d->chip->parent_<wbr>device, "startup: %u.%u\n", gpio, d->irq);<br>
> + npcmgpio_direction_output(gc, gpio, 1);<br>
> + npcmgpio_direction_input(gc, gpio);<br>
<br>
</span>Interesting dance. So it is required to set the line to<br>
1 and then switch to input?<br>
<span class=""><br>
> +static struct irq_chip npcmgpio_irqchip = {<br>
> + .name = "NPCM7XX-GPIO-IRQ",<br>
> + .irq_ack = npcmgpio_irq_ack,<br>
> + .irq_unmask = npcmgpio_irq_unmask,<br>
> + .irq_mask = npcmgpio_irq_mask,<br>
> + .irq_set_type = npcmgpio_set_irq_type,<br>
> + .irq_startup = npcmgpio_irq_startup,<br>
> +};<br>
<br>
</span>This code is looking good BTW.<br>
<br>
The patch in my inbox just ends in the middle of everything, I wonder<br>
why :( suspect the new gmail interface I'm using.<br>
<br>
Anyways: the pointers above should keep you busy for the next<br>
iteration of the patch, the pin control part seems pretty straight-forward.<br>
<br>
Yours,<br>
Linus Walleij<br>
</blockquote></div><br></div>