[lkml]   [2018]   [Jul]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
Patch in this message
SubjectRe: [PATCH v2 2/2] pinctrl: nuvoton: add NPCM7xx pinctrl and GPIO driver
<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:&quot;Courier New&quot;"> ret = <span style="font-weight:bolder">bgpio_init</span>(&amp;pctrl-&gt;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-&gt;dev, <span style="color:rgb(0,0,128)">4</span>,
pctrl-&gt;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 +
pctrl-&gt;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 +
<span style="color:rgb(192,64,0)">NULL</span>,
<span style="color:rgb(192,64,0)">NULL</span>,
pctrl-&gt;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 +
</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-&gt;bgpio_dir<font face="monospace, monospace"> </font><span style="font-family:arial,sans-serif">&amp;= ~</span><span style="font-family:&quot;Courier New&quot;;font-size:11pt;font-weight:bolder">bgpio_line2mask</span><span style="font-family:&quot;Courier New&quot;;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-&gt;bgpio_dir |= <span style="font-family:&quot;Courier New&quot;;font-size:11pt;font-weight:bolder">bgpio_line2mask</span><span style="font-family:&quot;Courier New&quot;;font-size:11pt">(gc, gpio)</span><span style="font-family:&quot;Courier New&quot;;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-&gt;bgpio_dir &amp; pinmask)
return !!(gc-&gt;read_reg(gc-&gt;reg_set) &amp; pinmask);
return !!(gc-&gt;read_reg(gc-&gt;reg_dat) &amp; 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 face="arial, helvetica, sans-serif"><font style="color:rgb(0,0,0)"><br>


</pre><pre><font style=""><font face="monospace, monospace" style="color:rgb(0,0,0)">


</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">&lt;<a href="" target="_blank"></a>&gt;</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 &lt;<a href=""></a>&gt; wrote:<br>
&gt; Add Nuvoton BMC NPCM750/730/715/705 Pinmux and<br>
&gt; GPIO controller driver.<br>
&gt; Signed-off-by: Tomer Maimon &lt;<a href=""></a>&gt;<br>
<span class="">&gt; +++ b/drivers/pinctrl/nuvoton/<wbr>pinctrl-npcm7xx.c<br>
&gt; @@ -0,0 +1,2089 @@<br>
&gt; +// SPDX-License-Identifier: GPL-2.0<br>
&gt; +// Copyright (c) 2016-2018 Nuvoton Technology corporation.<br>
&gt; +// Copyright (c) 2016, Dell Inc<br>
&gt; +<br>
&gt; +#include &lt;linux/device.h&gt;<br>
&gt; +#include &lt;linux/gpio.h&gt;<br>
</span>As this is a driver you should only include &lt;linux/gpio/driver.h&gt;<br>
<span class=""><br>
&gt; +#include &lt;linux/interrupt.h&gt;<br>
&gt; +#include &lt;linux/irq.h&gt;<br>
&gt; +#include &lt;linux/mfd/syscon.h&gt;<br>
</span>If you need syscon then the driver should select or depend<br>
on MFD_SYSCON in Kconfig.<br>
<span class=""><br>
&gt; +#include &lt;linux/module.h&gt;<br>
&gt; +#include &lt;linux/of.h&gt;<br>
&gt; +#include &lt;linux/of_address.h&gt;<br>
&gt; +#include &lt;linux/of_irq.h&gt;<br>
</span>Do you really need this include?<br>
<span class=""><br>
&gt; +/* Structure for register banks */<br>
&gt; +struct NPCM7XX_GPIO {<br>
</span>Can we have this lowercase? Please?<br>
<span class=""><br>
&gt; +       void __iomem            *base;<br>
&gt; +       struct gpio_chip        gc;<br>
&gt; +       int                     irqbase;<br>
&gt; +       int                     irq;<br>
&gt; +       spinlock_t              lock;<br>
&gt; +       void                    *priv;<br>
&gt; +       struct irq_chip         irq_chip;<br>
&gt; +       u32                     pinctrl_id;<br>
&gt; +};<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&#39;s look into that<br>
<span class=""><br>
&gt; +struct NPCM7xx_pinctrl {<br>
&gt; +       struct pinctrl_dev      *pctldev;<br>
&gt; +       struct device           *dev;<br>
&gt; +       struct NPCM7XX_GPIO     gpio_bank[NPCM7XX_GPIO_BANK_<wbr>NUM];<br>
&gt; +       struct irq_domain       *domain;<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>
&gt; +enum operand {<br>
&gt; +       op_set,<br>
&gt; +       op_getbit,<br>
&gt; +       op_setbit,<br>
&gt; +       op_clrbit,<br>
&gt; +};<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>
&gt; +/* Perform locked bit operations on GPIO registers */<br>
&gt; +static int gpio_bitop(struct NPCM7XX_GPIO *bank, int op, unsigned int offset,<br>
&gt; +                     int reg)<br>
&gt; +{<br>
&gt; +       unsigned long flags;<br>
&gt; +       u32 mask, val;<br>
&gt; +<br>
&gt; +       mask = (1L &lt;&lt; offset);<br>
&gt; +       spin_lock_irqsave(&amp;bank-&gt;lock, flags);<br>
&gt; +       switch (op) {<br>
&gt; +       case op_set:<br>
&gt; +               iowrite32(mask, bank-&gt;base + reg);<br>
&gt; +               break;<br>
&gt; +       case op_getbit:<br>
&gt; +               mask &amp;= ioread32(bank-&gt;base + reg);<br>
&gt; +               break;<br>
&gt; +       case op_setbit:<br>
&gt; +               val = ioread32(bank-&gt;base + reg);<br>
&gt; +               iowrite32(val | mask, bank-&gt;base + reg);<br>
&gt; +               break;<br>
&gt; +       case op_clrbit:<br>
&gt; +               val = ioread32(bank-&gt;base + reg);<br>
&gt; +               iowrite32(val &amp; (~mask), bank-&gt;base + reg);<br>
&gt; +               break;<br>
&gt; +       }<br>
&gt; +       spin_unlock_irqrestore(&amp;bank-&gt;<wbr>lock, flags);<br>
&gt; +       return !!mask;<br>
&gt; +}<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&#39;s use that instead :)<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>
&gt; +static int npcmgpio_get_direction(struct gpio_chip *chip, unsigned int offset)<br>
&gt; +{<br>
&gt; +       struct NPCM7XX_GPIO *bank = gpiochip_get_data(chip);<br>
&gt; +       u32 oe, ie;<br>
&gt; +<br>
&gt; +       /* Get Input &amp; Output state */<br>
&gt; +       ie = gpio_bitop(bank, op_getbit, offset, NPCM7XX_GP_N_IEM);<br>
&gt; +       oe = gpio_bitop(bank, op_getbit, offset, NPCM7XX_GP_N_OE);<br>
&gt; +       if (ie &amp;&amp; !oe)<br>
&gt; +               return GPIOF_DIR_IN;<br>
&gt; +       else if (oe &amp;&amp; !ie)<br>
&gt; +               return GPIOF_DIR_OUT;<br>
</span>These are consumer flags and should not be used in drivers.<br>
Use plain 0/1 instead.<br>
Anyways the problem goes away with GPIO_GENERIC.<br>
<span class=""><br>
&gt; +static int npcmgpio_direction_input(<wbr>struct gpio_chip *chip, unsigned int offset)<br>
&gt; +{<br>
</span>&gt; +       return pinctrl_gpio_direction_input(<wbr>offset + chip-&gt;base);<br>
&gt; +}<br>
It&#39;s a bit tricksy to get this to work with GPIO_GENERIC.<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>
Something like this:<br>
1. Add two indirection pointers to the npcm7xx_gpio state container:<br>
struct npcm7xx_gpio {<br>
      int (*direction_input)(struct gpio_chip *chip, unsigned offset);<br>
      int (*direction_output)(struct gpio_chip *chip, unsigned offset,<br>
int value);<br>
2. Save the pointers<br>
struct npcm7xx_gpio *npcm;<br>
bgpio_init( ... register setup ...)<br>
npcm-&gt;direction_input = npcm-&gt;gc.direction_input;<br>
npcm-&gt;direction_output = npcm-&gt;gc.direction_output;<br>
npcm-&gt;gc.direction_input = npcmgpio_direction_input;<br>
npcm-&gt;gc.direction_output = npcmgpio_direction_output;<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>
    struct npcm7xx_gpio *npcm = gpiochip_get_data(chip);<br>
    int ret;<br>
    ret = pinctrl_gpio_direction_input(<wbr>offset + chip-&gt;base);<br>
    if (ret)<br>
        return ret;<br>
    return npcm-&gt;direction_input(chip);<br>
I&#39;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>
&gt; +/* Set GPIO to Output with initial value */<br>
&gt; +static int npcmgpio_direction_output(<wbr>struct gpio_chip *chip,<br>
&gt; +                                    unsigned int offset, int value)<br>
&gt; +{<br>
&gt; +       struct NPCM7XX_GPIO *bank = gpiochip_get_data(chip);<br>
&gt; +<br>
&gt; +       dev_dbg(chip-&gt;parent, &quot;gpio_direction_output: offset%d = %x\n&quot;, offset,<br>
&gt; +               value);<br>
&gt; +<br>
&gt; +       /* Check if we&#39;re enabled as an interrupt.. */<br>
&gt; +       if (gpio_bitop(bank, op_getbit, offset, NPCM7XX_GP_N_EVEN) &amp;&amp;<br>
&gt; +           gpio_bitop(bank, op_getbit, offset, NPCM7XX_GP_N_IEM)) {<br>
&gt; +               dev_dbg(chip-&gt;parent,<br>
&gt; +                       &quot;gpio_direction_output: IRQ enabled on offset%d\n&quot;,<br>
&gt; +                       offset);<br>
&gt; +               return -EINVAL;<br>
&gt; +       }<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>
&gt; +static int npcmgpio_gpio_request(struct gpio_chip *chip, unsigned int offset)<br>
&gt; +{<br>
&gt; +       dev_dbg(chip-&gt;parent, &quot;gpio_request: offset%d\n&quot;, offset);<br>
&gt; +       return pinctrl_gpio_request(offset + chip-&gt;base);<br>
&gt; +}<br>
&gt; +<br>
&gt; +static void npcmgpio_gpio_free(struct gpio_chip *chip, unsigned int offset)<br>
&gt; +{<br>
&gt; +       dev_dbg(chip-&gt;parent, &quot;gpio_free: offset%d\n&quot;, offset);<br>
&gt; +       pinctrl_gpio_free(offset + chip-&gt;base);<br>
&gt; +}<br>
</span>This needs the same pattern as the direction functions above, then<br>
you can use GPIO_GENERIC (mmio).<br>
<span class=""><br>
&gt; +static unsigned int npcmgpio_irq_startup(struct irq_data *d)<br>
&gt; +{<br>
&gt; +       struct gpio_chip *gc = irq_data_get_irq_chip_data(d);<br>
&gt; +       unsigned int gpio = d-&gt;hwirq;<br>
&gt; +<br>
&gt; +       /* active-high, input, clear interrupt, enable interrupt */<br>
&gt; +       dev_dbg(d-&gt;chip-&gt;parent_<wbr>device, &quot;startup: %u.%u\n&quot;, gpio, d-&gt;irq);<br>
&gt; +       npcmgpio_direction_output(gc, gpio, 1);<br>
&gt; +       npcmgpio_direction_input(gc, gpio);<br>
</span>Interesting dance. So it is required to set the line to<br>
1 and then switch to input?<br>
<span class=""><br>
&gt; +static struct irq_chip npcmgpio_irqchip = {<br>
&gt; +       .name = &quot;NPCM7XX-GPIO-IRQ&quot;,<br>
&gt; +       .irq_ack = npcmgpio_irq_ack,<br>
&gt; +       .irq_unmask = npcmgpio_irq_unmask,<br>
&gt; +       .irq_mask = npcmgpio_irq_mask,<br>
&gt; +       .irq_set_type = npcmgpio_set_irq_type,<br>
&gt; +       .irq_startup = npcmgpio_irq_startup,<br>
&gt; +};<br>
</span>This code is looking good BTW.<br>
The patch in my inbox just ends in the middle of everything, I wonder<br>
why :( suspect the new gmail interface I&#39;m using.<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>
Linus Walleij<br>
 \ /
  Last update: 2018-07-26 02:03    [W:0.071 / U:1.984 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site