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

[Chris Chiu <chiu@xxxxxxxxxxxx>]

On Mon, Apr 1, 2019 at 3:49 PM Mika Westerberg
<mika.westerberg@xxxxxxxxxxxxxxx> wrote:
>
> On Fri, Mar 29, 2019 at 04:38:20PM +0800, Chris Chiu wrote:
> > On Thu, Mar 28, 2019 at 8:34 PM Mika Westerberg
> > <mika.westerberg@xxxxxxxxxxxxxxx> wrote:
> > >
> > > On Thu, Mar 28, 2019 at 08:19:59PM +0800, Chris Chiu wrote:
> > > > On Thu, Mar 28, 2019 at 5:38 PM Daniel Drake <drake@xxxxxxxxxxxx> wrote:
> > > > >
> > > > > On Thu, Mar 28, 2019 at 5:17 PM Andy Shevchenko
> > > > > <andriy.shevchenko@xxxxxxxxx> wrote:
> > > > > > Hmm... Can you confirm that laptop you declared as a fixed case and the
> > > > > > mentioned here is the same one?
> > > > >
> > > > > They are definitely not the same exact unit - originally we had a
> > > > > pre-production sample, and now we briefly diagnosed a real production
> > > > > unit that was sold to a customer. There could be subtle motherboard
> > > > > variations as you mention.
> > > > >
> > > > > > If it's the case, I recommend to ping Asus again and make them check and fix.
> > > > >
> > > > > We'll keep an eye open for any opportunities to go deeper here.
> > > > > However further investigation on both our side and theirs is blocked
> > > > > by not having any of the affected hardware (since the models are now
> > > > > so old), so I'm not very optimistic that we'll be able to make
> > > > > progress there.
> > > > >
> > > > > > Meanwhile, Mika's proposal sounds feasible and not so intrusive. We may
> > > > > > implement this later on.
> > > > >
> > > > > Chris will work on implementing this for your consideration.
> > > > >
> > > > > Thanks for the quick feedback!
> > > > > Daniel
> > > >
> > > > What if I modify the patch as follows? It doesn't save HOSTSW_OWN register.
> > > > It just toggles the bit specifically for the IRQ GPIO pin after resume when DMI
> > > > matches.
> > >
> > > I don't really like having quirks like this if we can avoid it and in
> > > this case I think we can. Just always save HOSTSW_OWN and then restore
> > > it if there is a GPIO requested and the value differs (and log a warning
> > > or something like that).
> >
> > You mean save the content of hostsw_own register on padgroup based ex.
> >     communities[i].hostown[gpp] = readl(base + gpp * 4);
> >
> > And then check the hostown bit for the GPIO requested pin in
> > intel_pinctrl_resume(),
> > differs the hostsw_own bit on pin base (like padcfg), then restore the
> > hostsw_own
> > value of the padgroug which the GPIO pin is belonging to?
>
> Yes.
>
> > I think what you mean should be a much more straightforward solution
> > for this. Could
> > you implement this in your way and we can try to help verification. Thanks.
>
> Sure I can but it probably does not happen until end of the week because
> I'm currently busy with something else.

Thanks for your attention. I don't want to distract you so I'll try to
refine the
patch. It would be a great help if you can help review and give comments.

Don't know whether if the following patch still get the wrong idea about
your thought.  It saves the hostsw_own when GPIO requested, check
if the value differs in resume() and restore if necessary. Please kindly
correct me if any. Thanks

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c
b/drivers/pinctrl/intel/pinctrl-intel.c
index 8cda7b535b02..d1cfa5adef9b 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -77,6 +77,7 @@ struct intel_pad_context {
        u32 padcfg0;
        u32 padcfg1;
        u32 padcfg2;
+       u32 hostown;
 };

 struct intel_community_context {
@@ -219,6 +220,24 @@ static bool intel_pad_acpi_mode(struct
intel_pinctrl *pctrl, unsigned int pin)
        return !(readl(hostown) & BIT(gpp_offset));
 }

+static void __iomem *intel_get_hostown(struct intel_pinctrl *pctrl,
unsigned int pin)
+{
+       const struct intel_community *community;
+       const struct intel_padgroup *padgrp;
+
+       community = intel_get_community(pctrl, pin);
+       if (!community)
+               return NULL;
+       if (!community->hostown_offset)
+               return NULL;
+
+       padgrp = intel_community_get_padgroup(community, pin);
+       if (!padgrp)
+               return NULL;
+
+       return community->regs + community->hostown_offset +
padgrp->reg_num * 4;
+}
+
 static bool intel_pad_locked(struct intel_pinctrl *pctrl, unsigned int pin)
 {
        struct intel_community *community;
@@ -442,7 +461,7 @@ static int intel_gpio_request_enable(struct
pinctrl_dev *pctldev,
                                     unsigned int pin)
 {
        struct intel_pinctrl *pctrl = pinctrl_dev_get_drvdata(pctldev);
-       void __iomem *padcfg0;
+       void __iomem *padcfg0, *hostown;
        unsigned long flags;

        raw_spin_lock_irqsave(&pctrl->lock, flags);
@@ -457,6 +476,10 @@ static int intel_gpio_request_enable(struct
pinctrl_dev *pctldev,
        /* Disable TX buffer and enable RX (this will be input) */
        __intel_gpio_set_direction(padcfg0, true);

+       /* Save HOSTSW_OWN */
+       hostown = intel_get_hostown(pctrl, pin);
+       if (!pctrl->context.pads[pin].hostown)
+               pctrl->context.pads[pin].hostown = readl(hostown);
        raw_spin_unlock_irqrestore(&pctrl->lock, flags);

        return 0;
@@ -1543,12 +1566,22 @@ int intel_pinctrl_resume(struct device *dev)
        pads = pctrl->context.pads;
        for (i = 0; i < pctrl->soc->npins; i++) {
                const struct pinctrl_pin_desc *desc = &pctrl->soc->pins[i];
-               void __iomem *padcfg;
+               void __iomem *padcfg, *hostown;
                u32 val;

                if (!intel_pinctrl_should_save(pctrl, desc->number))
                        continue;

+               hostown = intel_get_hostown(pctrl, desc->number);
+               val = readl(hostown);
+               if (!pads[i].hostown && val != pads[i].hostown) {
+                       writel(pads[i].hostown, hostown);
+                       dev_warn(dev, "pin %u not owned by host\n",
+                               desc->number);
+                       dev_dbg(dev, "restored pin %u hostsw_own %#08x\n",
+                               desc->number, readl(hostown));
+               }
+
                padcfg = intel_get_padcfg(pctrl, desc->number, PADCFG0);
                val = readl(padcfg) & ~PADCFG0_GPIORXSTATE;
                if (val != pads[i].padcfg0) {