Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20180918153621.71984-1-mika.westerberg@linux.intel.com>
In-Reply-To: <20180918153621.71984-1-mika.westerberg@linux.intel.com>
From:   Rajat Jain <rajatja@google.com>
Date:   Tue, 18 Sep 2018 15:04:23 -0700
Message-ID: <CACK8Z6F4+N1paQTSWcomnwLpLEHoospSOT3M9bqFQZHHfWmahA@mail.gmail.com>
Subject: Re: [PATCH] pinctrl: intel: Do pin translation in other GPIO
 operations as well
To:     Mika Westerberg <mika.westerberg@linux.intel.com>
Cc:     Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        casey.g.bowman@intel.com,
        "Atwood, Matthew S" <matthew.s.atwood@intel.com>,
        linux-gpio@vger.kernel.org,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Sep 18, 2018 at 8:36 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> For some reason I thought GPIOLIB handles translation from GPIO ranges
> to pinctrl pins but it turns out not to be the case. This means that
> when GPIOs operations are performed for a pin controller having a custom
> GPIO base such as Cannon Lake and Ice Lake incorrect pin number gets
> used internally.
>
> Fix this in the same way we did for lock/unlock IRQ operations and
> translate the GPIO number to pin before using it.
>
> Fixes: a60eac3239f0 ("pinctrl: intel: Allow custom GPIO base for pad groups")
> Reported-by: Rajat Jain <rajatja@google.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Tested-by: Rajat Jain <rajatja@google.com>

This has fixed the issue for me.

One question, may not be related: I see this line in my logs everytime
I export a pin (GPIO40 = pin 16 in this case). Is that an indication
of a problem?

"gpio gpiochip0: Persistence not supported for GPIO 40"

Thanks,

Rajat

On Tue, Sep 18, 2018 at 8:36 AM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> For some reason I thought GPIOLIB handles translation from GPIO ranges
> to pinctrl pins but it turns out not to be the case. This means that
> when GPIOs operations are performed for a pin controller having a custom
> GPIO base such as Cannon Lake and Ice Lake incorrect pin number gets
> used internally.
>
> Fix this in the same way we did for lock/unlock IRQ operations and
> translate the GPIO number to pin before using it.
>
> Fixes: a60eac3239f0 ("pinctrl: intel: Allow custom GPIO base for pad groups")
> Reported-by: Rajat Jain <rajatja@google.com>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/pinctrl/intel/pinctrl-intel.c | 111 +++++++++++++++-----------
>  1 file changed, 63 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
> index 62b009b27eda..ec8dafc94694 100644
> --- a/drivers/pinctrl/intel/pinctrl-intel.c
> +++ b/drivers/pinctrl/intel/pinctrl-intel.c
> @@ -747,13 +747,63 @@ static const struct pinctrl_desc intel_pinctrl_desc = {
>         .owner = THIS_MODULE,
>  };
>
> +/**
> + * intel_gpio_to_pin() - Translate from GPIO offset to pin number
> + * @pctrl: Pinctrl structure
> + * @offset: GPIO offset from gpiolib
> + * @commmunity: Community is filled here if not %NULL
> + * @padgrp: Pad group is filled here if not %NULL
> + *
> + * When coming through gpiolib irqchip, the GPIO offset is not
> + * automatically translated to pinctrl pin number. This function can be
> + * used to find out the corresponding pinctrl pin.
> + */
> +static int intel_gpio_to_pin(struct intel_pinctrl *pctrl, unsigned offset,
> +                            const struct intel_community **community,
> +                            const struct intel_padgroup **padgrp)
> +{
> +       int i;
> +
> +       for (i = 0; i < pctrl->ncommunities; i++) {
> +               const struct intel_community *comm = &pctrl->communities[i];
> +               int j;
> +
> +               for (j = 0; j < comm->ngpps; j++) {
> +                       const struct intel_padgroup *pgrp = &comm->gpps[j];
> +
> +                       if (pgrp->gpio_base < 0)
> +                               continue;
> +
> +                       if (offset >= pgrp->gpio_base &&
> +                           offset < pgrp->gpio_base + pgrp->size) {
> +                               int pin;
> +
> +                               pin = pgrp->base + offset - pgrp->gpio_base;
> +                               if (community)
> +                                       *community = comm;
> +                               if (padgrp)
> +                                       *padgrp = pgrp;
> +
> +                               return pin;
> +                       }
> +               }
> +       }
> +
> +       return -EINVAL;
> +}
> +
>  static int intel_gpio_get(struct gpio_chip *chip, unsigned offset)
>  {
>         struct intel_pinctrl *pctrl = gpiochip_get_data(chip);
>         void __iomem *reg;
>         u32 padcfg0;
> +       int pin;
> +
> +       pin = intel_gpio_to_pin(pctrl, offset, NULL, NULL);
> +       if (pin < 0)
> +               return -EINVAL;
>
> -       reg = intel_get_padcfg(pctrl, offset, PADCFG0);
> +       reg = intel_get_padcfg(pctrl, pin, PADCFG0);
>         if (!reg)
>                 return -EINVAL;
>
> @@ -770,8 +820,13 @@ static void intel_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
>         unsigned long flags;
>         void __iomem *reg;
>         u32 padcfg0;
> +       int pin;
> +
> +       pin = intel_gpio_to_pin(pctrl, offset, NULL, NULL);
> +       if (pin < 0)
> +               return;
>
> -       reg = intel_get_padcfg(pctrl, offset, PADCFG0);
> +       reg = intel_get_padcfg(pctrl, pin, PADCFG0);
>         if (!reg)
>                 return;
>
> @@ -790,8 +845,13 @@ static int intel_gpio_get_direction(struct gpio_chip *chip, unsigned int offset)
>         struct intel_pinctrl *pctrl = gpiochip_get_data(chip);
>         void __iomem *reg;
>         u32 padcfg0;
> +       int pin;
>
> -       reg = intel_get_padcfg(pctrl, offset, PADCFG0);
> +       pin = intel_gpio_to_pin(pctrl, offset, NULL, NULL);
> +       if (pin < 0)
> +               return -EINVAL;
> +
> +       reg = intel_get_padcfg(pctrl, pin, PADCFG0);
>         if (!reg)
>                 return -EINVAL;
>
> @@ -827,51 +887,6 @@ static const struct gpio_chip intel_gpio_chip = {
>         .set_config = gpiochip_generic_config,
>  };
>
> -/**
> - * intel_gpio_to_pin() - Translate from GPIO offset to pin number
> - * @pctrl: Pinctrl structure
> - * @offset: GPIO offset from gpiolib
> - * @commmunity: Community is filled here if not %NULL
> - * @padgrp: Pad group is filled here if not %NULL
> - *
> - * When coming through gpiolib irqchip, the GPIO offset is not
> - * automatically translated to pinctrl pin number. This function can be
> - * used to find out the corresponding pinctrl pin.
> - */
> -static int intel_gpio_to_pin(struct intel_pinctrl *pctrl, unsigned offset,
> -                            const struct intel_community **community,
> -                            const struct intel_padgroup **padgrp)
> -{
> -       int i;
> -
> -       for (i = 0; i < pctrl->ncommunities; i++) {
> -               const struct intel_community *comm = &pctrl->communities[i];
> -               int j;
> -
> -               for (j = 0; j < comm->ngpps; j++) {
> -                       const struct intel_padgroup *pgrp = &comm->gpps[j];
> -
> -                       if (pgrp->gpio_base < 0)
> -                               continue;
> -
> -                       if (offset >= pgrp->gpio_base &&
> -                           offset < pgrp->gpio_base + pgrp->size) {
> -                               int pin;
> -
> -                               pin = pgrp->base + offset - pgrp->gpio_base;
> -                               if (community)
> -                                       *community = comm;
> -                               if (padgrp)
> -                                       *padgrp = pgrp;
> -
> -                               return pin;
> -                       }
> -               }
> -       }
> -
> -       return -EINVAL;
> -}
> -
>  static int intel_gpio_irq_reqres(struct irq_data *d)
>  {
>         struct gpio_chip *gc = irq_data_get_irq_chip_data(d);
> --
> 2.18.0
>