Re: [PATCH] pinctrl: intel: save HOSTSW_OWN register over suspend/resume

[Andy Shevchenko <andriy.shevchenko@xxxxxxxxx>]

On Tue, Apr 02, 2019 at 02:16:19PM +0800, Chris Chiu wrote:
> On Mon, Apr 1, 2019 at 8:23 PM Andy Shevchenko
> <andriy.shevchenko@xxxxxxxxx> wrote:
> > On Mon, Apr 01, 2019 at 06:41:57PM +0800, Chris Chiu wrote:

> Thanks for the comment. My first version did mimic the logic of the interrupt
> mask restore but it was based on the DMI quirk. It saves HOSTSW_OWN
> for each padgroup and restores them all after resume if DMI info matched.
> 
> What really confused me is how to do this specifically for a requested GPIO
> pin. So here's my new proposed patch. Please suggests if there's any better
> idea. Thanks.

>  struct intel_community_context {
>         u32 *intmask;
> +       u32 *hostown;

This is okay.

>  };

> +#ifdef CONFIG_PM_SLEEP
> +static void intel_save_hostown(struct intel_pinctrl *pctrl, unsigned int pin);
> +#endif
> +

No need for this...

>         /* Disable TX buffer and enable RX (this will be input) */
>         __intel_gpio_set_direction(padcfg0, true);
> +#ifdef CONFIG_PM_SLEEP
> +       intel_save_hostown(pctrl, pin);
> +#endif

...and for this.
Just save all of them at ->suspend()

>         for (i = 0; i < pctrl->ncommunities; i++) {
>                 struct intel_community *community = &pctrl->communities[i];
> -               u32 *intmask;
> +               u32 *intmask, *hostown;
> 
>                 intmask = devm_kcalloc(pctrl->dev, community->ngpps,
>                                        sizeof(*intmask), GFP_KERNEL);
> @@ -1292,6 +1299,13 @@ static int intel_pinctrl_pm_init(struct
> intel_pinctrl *pctrl)
>                         return -ENOMEM;
> 
>                 communities[i].intmask = intmask;
> +
> +               hostown = devm_kcalloc(pctrl->dev, community->ngpps,
> +                                      sizeof(*hostown), GFP_KERNEL);
> +               if (!hostown)
> +                       return -ENOMEM;
> +
> +               communities[i].hostown= hostown;

This is good.

>         }

> +static void intel_save_hostown(struct intel_pinctrl *pctrl, unsigned int pin)
> +{
> +       const struct intel_community *community;
> +       const struct intel_padgroup *padgrp;
> +       int i;
> +
> +       community = intel_get_community(pctrl, pin);
> +       if (!community)
> +               return;
> +       if (!community->hostown_offset)
> +               return;
> +
> +       padgrp = intel_community_get_padgroup(community, pin);
> +       if (!padgrp)
> +               return;
> +
> +       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 (padgrp == pgrp) {
> +                               struct intel_community_context *communities;
> +                               void __iomem *base;
> +
> +                               communities = pctrl->context.communities;
> +                               base = community->regs +
> community->hostown_offset;
> +                               communities[i].hostown[j] = readl(base + j * 4);
> +                               break;
> +                       }
> +               }
> +       }

> +       return;

Useless.

> +}

This is too complicated. Just add

base = community->regs + community->hostown_offset;
for (gpp = 0; gpp < community->ngpps; gpp++)
	communities[i].hostown[gpp] = readl(base + gpp * 4);

into ->suspend() loop.

> +               base = community->regs + community->hostown_offset;
> +               for (gpp = 0; gpp < community->ngpps; gpp++) {
> +                       if (communities[i].hostown[gpp] &&
> +                           communities[i].hostown[gpp] != readl(base
> + gpp * 4)) {
> +                               writel(communities[i].hostown[gpp],
> base + gpp * 4);
> +                               dev_warn(dev, "hostown changed after resume\n");
> +                               dev_dbg(dev, "restored hostown %d/%u
> %#08x\n", i, gpp,
> +                                       readl(base + gpp * 4));
> +                       }
> +               }

Instead you may need to loop over each pin in the part of the group related to
one 32-bit HOSTSW_OWN register (i.e. 8, see PADOWN_*() macros in the driver),
check if it's requested and break a loop. If loop index is off-by-one a limit,
nothing to do, otherwise restore hostown register.

More pedantic approach is to collect the mask inside the loop and apply it.

The check function name is gpiochip_is_requested().

(One of Intel's drivers which is using that at ->resume() is
 drivers/gpio/gpio-lynxpoint.c)

P.S. I prefer pedantic approach. The simplification one is showed in order to
give you an idea.

-- 
With Best Regards,
Andy Shevchenko