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

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

[Chris Chiu]

The touchpad in the Asus laptop model X540NA is unresponsive
after suspend/resume. The following error appears during resume:

i2c_hid i2c-ELAN1200:00: failed to reset device.

The problem here is that i2c_hid does not notice the interrupt
being generated at this point. Because the GPIO which connects
to touchpad interrupt is in ACPI mode after resume and no longer
work as IRQ.

Fix this by saving HOSTSW_OWN register during suspend and restore
them at resume.

Signed-off-by: Chris Chiu <chiu@endlessm.com>
---
 drivers/pinctrl/intel/pinctrl-intel.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c
index 6dc1096d3d34..eaafa0e534f3 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -83,6 +83,7 @@ struct intel_pad_context {
 
 struct intel_community_context {
 	u32 *intmask;
+	u32 *modemask;
 };
 
 struct intel_pinctrl_context {
@@ -1158,6 +1159,7 @@ static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
 	for (i = 0; i < pctrl->ncommunities; i++) {
 		struct intel_community *community = &pctrl->communities[i];
 		u32 *intmask;
+		u32 *modemask;
 
 		intmask = devm_kcalloc(pctrl->dev, community->ngpps,
 				       sizeof(*intmask), GFP_KERNEL);
@@ -1165,6 +1167,13 @@ static int intel_pinctrl_pm_init(struct intel_pinctrl *pctrl)
 			return -ENOMEM;
 
 		communities[i].intmask = intmask;
+
+		modemask = devm_kcalloc(pctrl->dev, community->ngpps,
+				       sizeof(*modemask), GFP_KERNEL);
+		if (!modemask)
+			return -ENOMEM;
+
+		communities[i].modemask = modemask;
 	}
 
 	pctrl->context.pads = pads;
@@ -1329,6 +1338,10 @@ int intel_pinctrl_suspend(struct device *dev)
 		base = community->regs + community->ie_offset;
 		for (gpp = 0; gpp < community->ngpps; gpp++)
 			communities[i].intmask[gpp] = readl(base + gpp * 4);
+
+		base = community->regs + community->hostown_offset;
+		for (gpp = 0; gpp < community->ngpps; gpp++)
+			communities[i].modemask[gpp] = readl(base + gpp * 4);
 	}
 
 	return 0;
@@ -1411,7 +1424,14 @@ int intel_pinctrl_resume(struct device *dev)
 		base = community->regs + community->ie_offset;
 		for (gpp = 0; gpp < community->ngpps; gpp++) {
 			writel(communities[i].intmask[gpp], base + gpp * 4);
-			dev_dbg(dev, "restored mask %d/%u %#08x\n", i, gpp,
+			dev_dbg(dev, "restored intmask %d/%u %#08x\n", i, gpp,
+				readl(base + gpp * 4));
+		}
+
+		base = community->regs + community->hostown_offset;
+		for (gpp = 0; gpp < community->ngpps; gpp++) {
+			writel(communities[i].modemask[gpp], base + gpp * 4);
+			dev_dbg(dev, "restored modemask %d/%u %#08x\n", i, gpp,
 				readl(base + gpp * 4));
 		}
 	}
-- 
2.11.0

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

[Chris Chiu]

Hi Mika,

    Yes, that’s the most weird part.

Chris

從我的 iPhone 傳送

> Mika Westerberg <mika.westerberg@linux.intel.com> 於 2017年11月15日 下午4:04 寫道：
> 
>> On Tue, Nov 14, 2017 at 06:41:36PM +0800, Chris Chiu wrote:
>> The touchpad in the Asus laptop model X540NA is unresponsive
>> after suspend/resume. The following error appears during resume:
>> 
>> i2c_hid i2c-ELAN1200:00: failed to reset device.
>> 
>> The problem here is that i2c_hid does not notice the interrupt
>> being generated at this point. Because the GPIO which connects
>> to touchpad interrupt is in ACPI mode after resume and no longer
>> work as IRQ.
> 
> Hmm, are you saying it is initially not in ACPI mode but after resume it
> magically is?

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

[Mika Westerberg]

On Wed, Nov 15, 2017 at 04:08:32PM +0800, Chris Chiu wrote:
>     Yes, that’s the most weird part.

Sounds pretty much like a BIOS issue. Do you know if there is a newer
BIOS and have you tried that already?

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

[Chris Chiu]

Hi Mika,
    I've confirmed with Asus and they said it's the latest BIOS for
shipment and verified OK on Windows. So their BIOS team will not do
anything for this.

Chris

On Wed, Nov 15, 2017 at 6:13 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Wed, Nov 15, 2017 at 04:08:32PM +0800, Chris Chiu wrote:
>>     Yes, that’s the most weird part.
>
> Sounds pretty much like a BIOS issue. Do you know if there is a newer
> BIOS and have you tried that already?

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

[Mika Westerberg]

On Wed, Nov 15, 2017 at 06:19:56PM +0800, Chris Chiu wrote:
> Hi Mika,
>     I've confirmed with Asus and they said it's the latest BIOS for
> shipment and verified OK on Windows. So their BIOS team will not do
> anything for this.

I'll ask around if our Windows people know anything about this. My gut
feeling is that the Windows driver does not touch HOSTSW_OWN either.

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

[Chris Chiu]

On Thu, Nov 16, 2017 at 8:44 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Wed, Nov 15, 2017 at 06:19:56PM +0800, Chris Chiu wrote:
>> Hi Mika,
>>     I've confirmed with Asus and they said it's the latest BIOS for
>> shipment and verified OK on Windows. So their BIOS team will not do
>> anything for this.
>
> I'll ask around if our Windows people know anything about this. My gut
> feeling is that the Windows driver does not touch HOSTSW_OWN either.

Thanks. Please let me know if you need any information. I still keep
the machine.

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

[Mika Westerberg]

On Thu, Nov 16, 2017 at 09:27:51PM +0800, Chris Chiu wrote:
> On Thu, Nov 16, 2017 at 8:44 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Wed, Nov 15, 2017 at 06:19:56PM +0800, Chris Chiu wrote:
> >> Hi Mika,
> >>     I've confirmed with Asus and they said it's the latest BIOS for
> >> shipment and verified OK on Windows. So their BIOS team will not do
> >> anything for this.
> >
> > I'll ask around if our Windows people know anything about this. My gut
> > feeling is that the Windows driver does not touch HOSTSW_OWN either.
> 
> Thanks. Please let me know if you need any information. I still keep
> the machine.

Got confirmation from Windows people. So Windows pretty much saves and
restores the same registers than we do (padcfg + ie).

Have you tried whether s2idle works instead of S3 suspend? You can try
it like

  # echo freeze > /sys/power/state

If that works, I'm guessing that this system uses s2idle and that's also
what Windows uses and could explain why it works in Windows.

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

[Chris Chiu]

On Fri, Nov 17, 2017 at 2:49 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Thu, Nov 16, 2017 at 09:27:51PM +0800, Chris Chiu wrote:
>> On Thu, Nov 16, 2017 at 8:44 PM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>> > On Wed, Nov 15, 2017 at 06:19:56PM +0800, Chris Chiu wrote:
>> >> Hi Mika,
>> >>     I've confirmed with Asus and they said it's the latest BIOS for
>> >> shipment and verified OK on Windows. So their BIOS team will not do
>> >> anything for this.
>> >
>> > I'll ask around if our Windows people know anything about this. My gut
>> > feeling is that the Windows driver does not touch HOSTSW_OWN either.
>>
>> Thanks. Please let me know if you need any information. I still keep
>> the machine.
>
> Got confirmation from Windows people. So Windows pretty much saves and
> restores the same registers than we do (padcfg + ie).
>
> Have you tried whether s2idle works instead of S3 suspend? You can try
> it like
>
>   # echo freeze > /sys/power/state
>
> If that works, I'm guessing that this system uses s2idle and that's also
> what Windows uses and could explain why it works in Windows.

Unfortunately, I cant wake it up from neither power button nor any key
event after  "# echo freeze > /sys/power/state"....

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

[Mika Westerberg]

On Fri, Nov 17, 2017 at 04:11:31PM +0800, Chris Chiu wrote:
> On Fri, Nov 17, 2017 at 2:49 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Thu, Nov 16, 2017 at 09:27:51PM +0800, Chris Chiu wrote:
> >> On Thu, Nov 16, 2017 at 8:44 PM, Mika Westerberg
> >> <mika.westerberg@linux.intel.com> wrote:
> >> > On Wed, Nov 15, 2017 at 06:19:56PM +0800, Chris Chiu wrote:
> >> >> Hi Mika,
> >> >>     I've confirmed with Asus and they said it's the latest BIOS for
> >> >> shipment and verified OK on Windows. So their BIOS team will not do
> >> >> anything for this.
> >> >
> >> > I'll ask around if our Windows people know anything about this. My gut
> >> > feeling is that the Windows driver does not touch HOSTSW_OWN either.
> >>
> >> Thanks. Please let me know if you need any information. I still keep
> >> the machine.
> >
> > Got confirmation from Windows people. So Windows pretty much saves and
> > restores the same registers than we do (padcfg + ie).
> >
> > Have you tried whether s2idle works instead of S3 suspend? You can try
> > it like
> >
> >   # echo freeze > /sys/power/state
> >
> > If that works, I'm guessing that this system uses s2idle and that's also
> > what Windows uses and could explain why it works in Windows.
> 
> Unfortunately, I cant wake it up from neither power button nor any key
> event after  "# echo freeze > /sys/power/state"....

Yeah, it is S3 platform based on the acpidump you shared. Although
freeze should still work.

Have you dumped the pin config registers through debugfs before and
after suspend? Are there any other differences except the HOSTSW_OWN
thing?

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

[Chris Chiu]

On Tue, Nov 21, 2017 at 6:52 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Fri, Nov 17, 2017 at 04:11:31PM +0800, Chris Chiu wrote:
>> On Fri, Nov 17, 2017 at 2:49 PM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>> > On Thu, Nov 16, 2017 at 09:27:51PM +0800, Chris Chiu wrote:
>> >> On Thu, Nov 16, 2017 at 8:44 PM, Mika Westerberg
>> >> <mika.westerberg@linux.intel.com> wrote:
>> >> > On Wed, Nov 15, 2017 at 06:19:56PM +0800, Chris Chiu wrote:
>> >> >> Hi Mika,
>> >> >>     I've confirmed with Asus and they said it's the latest BIOS for
>> >> >> shipment and verified OK on Windows. So their BIOS team will not do
>> >> >> anything for this.
>> >> >
>> >> > I'll ask around if our Windows people know anything about this. My gut
>> >> > feeling is that the Windows driver does not touch HOSTSW_OWN either.
>> >>
>> >> Thanks. Please let me know if you need any information. I still keep
>> >> the machine.
>> >
>> > Got confirmation from Windows people. So Windows pretty much saves and
>> > restores the same registers than we do (padcfg + ie).
>> >
>> > Have you tried whether s2idle works instead of S3 suspend? You can try
>> > it like
>> >
>> >   # echo freeze > /sys/power/state
>> >
>> > If that works, I'm guessing that this system uses s2idle and that's also
>> > what Windows uses and could explain why it works in Windows.
>>
>> Unfortunately, I cant wake it up from neither power button nor any key
>> event after  "# echo freeze > /sys/power/state"....
>
> Yeah, it is S3 platform based on the acpidump you shared. Although
> freeze should still work.
>
> Have you dumped the pin config registers through debugfs before and
> after suspend? Are there any other differences except the HOSTSW_OWN
> thing?

Yup, I checked the value of the corresponded pin. It shows following before
suspend
pin 18 (GPIO_18) GPIO 0x40800102 0x00024075

Then after resume
pin 18 (GPIO_18) GPIO 0x40800102 0x00024075 [ACPI]

What else register do you suggest me to compare? The PADCFG2 is invalid

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

[Mika Westerberg]

On Tue, Nov 21, 2017 at 07:54:26PM +0800, Chris Chiu wrote:
> Yup, I checked the value of the corresponded pin. It shows following before
> suspend
> pin 18 (GPIO_18) GPIO 0x40800102 0x00024075
> 
> Then after resume
> pin 18 (GPIO_18) GPIO 0x40800102 0x00024075 [ACPI]

OK, so ownership is changed to ACPI.

> What else register do you suggest me to compare? The PADCFG2 is invalid

It's fine APL does not have PADCFG2.

Hmm, I don't understand how this can work in Windows either. The Windows
people told me that they don't save and restore anything else than
padcfg registers + ie. If the ownership is changed to ACPI it means you
don't get interrupts anymore (only GPEs) and that applies to Windows as
well.

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

[Chris Chiu]

On Tue, Nov 21, 2017 at 8:04 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Tue, Nov 21, 2017 at 07:54:26PM +0800, Chris Chiu wrote:
>> Yup, I checked the value of the corresponded pin. It shows following before
>> suspend
>> pin 18 (GPIO_18) GPIO 0x40800102 0x00024075
>>
>> Then after resume
>> pin 18 (GPIO_18) GPIO 0x40800102 0x00024075 [ACPI]
>
> OK, so ownership is changed to ACPI.
>
>> What else register do you suggest me to compare? The PADCFG2 is invalid
>
> It's fine APL does not have PADCFG2.
>
> Hmm, I don't understand how this can work in Windows either. The Windows
> people told me that they don't save and restore anything else than
> padcfg registers + ie. If the ownership is changed to ACPI it means you
> don't get interrupts anymore (only GPEs) and that applies to Windows as
> well.

Thanks for your confirmation. I've pushed this back to ASUS. They told me
there's a new official BIOS released, of course they don't know
whether if it helps
or not. I then upgraded it and the problem is gone. So it's truly a
BIOS but as you
said. Thanks for your help.

Chris

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

[Mika Westerberg]

On Thu, Nov 23, 2017 at 08:24:05PM +0800, Chris Chiu wrote:
> Thanks for your confirmation. I've pushed this back to ASUS. They told me
> there's a new official BIOS released, of course they don't know
> whether if it helps
> or not. I then upgraded it and the problem is gone. So it's truly a
> BIOS but as you
> said. Thanks for your help.

Hehe, good that it solved without quirking the driver :)

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

[Daniel Drake]

Hi Mika,

Digging up this old thread again...

On Tue, Nov 21, 2017 at 8:13 PM Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
>
> On Tue, Nov 21, 2017 at 07:54:26PM +0800, Chris Chiu wrote:
> > Yup, I checked the value of the corresponded pin. It shows following before
> > suspend
> > pin 18 (GPIO_18) GPIO 0x40800102 0x00024075
> >
> > Then after resume
> > pin 18 (GPIO_18) GPIO 0x40800102 0x00024075 [ACPI]
>
> OK, so ownership is changed to ACPI.
>
> > What else register do you suggest me to compare? The PADCFG2 is invalid
>
> It's fine APL does not have PADCFG2.
>
> Hmm, I don't understand how this can work in Windows either. The Windows
> people told me that they don't save and restore anything else than
> padcfg registers + ie. If the ownership is changed to ACPI it means you
> don't get interrupts anymore (only GPEs) and that applies to Windows as
> well.

In the mails after the one quoted above, we reported back to you that
the new BIOS from Asus solved the issue.

However, during the time that has followed, we have had numerous user
reports from Asus E403NA, X540NA, and X541NA laptops (basically the
same models that we originally discussed) where the touchpad stops
working after suspend/resume, even with the latest BIOS. We managed to
get an affected E403NA unit in-hands again, and confirmed that
HOSTSW_OWN was being lost like we had observed before.

Unfortunately as this was a customer laptop we had to return it
immediately, before we could investigate further. We don't have access
to any more units since they are old models now.

However I'm wondering if you have any other ideas or if you think
something like our workaround patch might be acceptable under these
circumstances:
https://github.com/endlessm/linux/commit/f391452299f62a3d0cbe5333be90f69e9895d8ff

Thanks
Daniel

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

[Mika Westerberg]

+Andy

On Wed, Mar 27, 2019 at 04:22:04PM +0800, Daniel Drake wrote:
> Hi Mika,
> 
> Digging up this old thread again...
> 
> On Tue, Nov 21, 2017 at 8:13 PM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> >
> > On Tue, Nov 21, 2017 at 07:54:26PM +0800, Chris Chiu wrote:
> > > Yup, I checked the value of the corresponded pin. It shows following before
> > > suspend
> > > pin 18 (GPIO_18) GPIO 0x40800102 0x00024075
> > >
> > > Then after resume
> > > pin 18 (GPIO_18) GPIO 0x40800102 0x00024075 [ACPI]
> >
> > OK, so ownership is changed to ACPI.
> >
> > > What else register do you suggest me to compare? The PADCFG2 is invalid
> >
> > It's fine APL does not have PADCFG2.
> >
> > Hmm, I don't understand how this can work in Windows either. The Windows
> > people told me that they don't save and restore anything else than
> > padcfg registers + ie. If the ownership is changed to ACPI it means you
> > don't get interrupts anymore (only GPEs) and that applies to Windows as
> > well.
> 
> In the mails after the one quoted above, we reported back to you that
> the new BIOS from Asus solved the issue.
> 
> However, during the time that has followed, we have had numerous user
> reports from Asus E403NA, X540NA, and X541NA laptops (basically the
> same models that we originally discussed) where the touchpad stops
> working after suspend/resume, even with the latest BIOS. We managed to
> get an affected E403NA unit in-hands again, and confirmed that
> HOSTSW_OWN was being lost like we had observed before.
> 
> Unfortunately as this was a customer laptop we had to return it
> immediately, before we could investigate further. We don't have access
> to any more units since they are old models now.
> 
> However I'm wondering if you have any other ideas or if you think
> something like our workaround patch might be acceptable under these
> circumstances:
> https://github.com/endlessm/linux/commit/f391452299f62a3d0cbe5333be90f69e9895d8ff

I wonder if it would be simpler to save it always and then upon resume
compare them and if changed, log this in dmesg and restore the saved
one.

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

[Mika Westerberg]

On Wed, Mar 27, 2019 at 07:29:40PM +0200, Mika Westerberg wrote:
> I wonder if it would be simpler to save it always and then upon resume
> compare them and if changed, log this in dmesg and restore the saved
> one.

Actually I think better is to restore hostsw_own only for GPIOs that are
already requested by us. The BIOS should have no business messing those
anyway once they are owned by the GPIO driver.

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

[Andy Shevchenko]

On Wed, Mar 27, 2019 at 07:29:40PM +0200, Mika Westerberg wrote:
> On Wed, Mar 27, 2019 at 04:22:04PM +0800, Daniel Drake wrote:
> > On Tue, Nov 21, 2017 at 8:13 PM Mika Westerberg
> > <mika.westerberg@linux.intel.com> wrote:
> > > On Tue, Nov 21, 2017 at 07:54:26PM +0800, Chris Chiu wrote:
> > > > Yup, I checked the value of the corresponded pin. It shows following before
> > > > suspend
> > > > pin 18 (GPIO_18) GPIO 0x40800102 0x00024075
> > > >
> > > > Then after resume
> > > > pin 18 (GPIO_18) GPIO 0x40800102 0x00024075 [ACPI]
> > >
> > > OK, so ownership is changed to ACPI.
> > >
> > > > What else register do you suggest me to compare? The PADCFG2 is invalid
> > >
> > > It's fine APL does not have PADCFG2.
> > >
> > > Hmm, I don't understand how this can work in Windows either. The Windows
> > > people told me that they don't save and restore anything else than
> > > padcfg registers + ie. If the ownership is changed to ACPI it means you
> > > don't get interrupts anymore (only GPEs) and that applies to Windows as
> > > well.
> > 
> > In the mails after the one quoted above, we reported back to you that
> > the new BIOS from Asus solved the issue.
> > 
> > However, during the time that has followed, we have had numerous user
> > reports from Asus E403NA, X540NA, and X541NA laptops (basically the
> > same models that we originally discussed) where the touchpad stops
> > working after suspend/resume, even with the latest BIOS. We managed to
> > get an affected E403NA unit in-hands again, and confirmed that
> > HOSTSW_OWN was being lost like we had observed before.

Hmm... Can you confirm that laptop you declared as a fixed case and the
mentioned here is the same one?

If they are different, I have a theory that PCBs of those two are not the same
and used GPIO pin can be also not the same, therefore the BIOS fixes only one
revision of the model, but didn't consider the rest.

If it's the case, I recommend to ping Asus again and make them check and fix.

Meanwhile, Mika's proposal sounds feasible and not so intrusive. We may
implement this later on.

> > 
> > Unfortunately as this was a customer laptop we had to return it
> > immediately, before we could investigate further. We don't have access
> > to any more units since they are old models now.
> > 
> > However I'm wondering if you have any other ideas or if you think
> > something like our workaround patch might be acceptable under these
> > circumstances:
> > https://github.com/endlessm/linux/commit/f391452299f62a3d0cbe5333be90f69e9895d8ff
> 
> I wonder if it would be simpler to save it always and then upon resume
> compare them and if changed, log this in dmesg and restore the saved
> one.

-- 
With Best Regards,
Andy Shevchenko

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

[Daniel Drake]

On Thu, Mar 28, 2019 at 5:17 PM Andy Shevchenko
<andriy.shevchenko@intel.com> 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

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

[Chris Chiu]

On Thu, Mar 28, 2019 at 5:38 PM Daniel Drake <drake@endlessm.com> wrote:
>
> On Thu, Mar 28, 2019 at 5:17 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> 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.

---
 drivers/pinctrl/intel/pinctrl-intel.c | 67 +++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/drivers/pinctrl/intel/pinctrl-intel.c
b/drivers/pinctrl/intel/pinctrl-intel.c
index 8cda7b535b02..994abc5ecd32 100644
--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -19,6 +19,7 @@
 #include <linux/pinctrl/pinmux.h>
 #include <linux/pinctrl/pinconf.h>
 #include <linux/pinctrl/pinconf-generic.h>
+#include <linux/dmi.h>

 #include "../core.h"
 #include "pinctrl-intel.h"
@@ -73,6 +74,8 @@

 #define DEBOUNCE_PERIOD                        31250 /* ns */

+#define PINCTRL_QUIRK_KEEP_HOSTOWN     BIT(0)
+
 struct intel_pad_context {
        u32 padcfg0;
        u32 padcfg1;
@@ -112,11 +115,37 @@ struct intel_pinctrl {
        size_t ncommunities;
        struct intel_pinctrl_context context;
        int irq;
+       u32 quirks;
 };

 #define pin_to_padno(c, p)     ((p) - (c)->pin_base)
 #define padgroup_offset(g, p)  ((p) - (g)->base)

+static const struct dmi_system_id dmi_retain_hostown_table[] = {
+       {
+               .ident = "ASUSTeK COMPUTER INC. ASUS E403NA",
+               .matches = {
+                       DMI_MATCH(DMI_BOARD_VENDOR, "ASUSTeK COMPUTER INC."),
+                       DMI_MATCH(DMI_BOARD_NAME, "E403NA"),
+               },
+       },
+       {
+               .ident = "ASUSTeK COMPUTER INC. ASUS X540NA",
+               .matches = {
+                       DMI_MATCH(DMI_BOARD_VENDOR, "ASUSTeK COMPUTER INC."),
+                       DMI_MATCH(DMI_BOARD_NAME, "X540NA"),
+               },
+       },
+       {
+               .ident = "ASUSTeK COMPUTER INC. ASUS X541NA",
+               .matches = {
+                       DMI_MATCH(DMI_BOARD_VENDOR, "ASUSTeK COMPUTER INC."),
+                       DMI_MATCH(DMI_BOARD_NAME, "X541NA"),
+               },
+       },
+       { }
+};
+
 static struct intel_community *intel_get_community(struct intel_pinctrl *pctrl,
                                                   unsigned int pin)
 {
@@ -219,6 +248,32 @@ static bool intel_pad_acpi_mode(struct
intel_pinctrl *pctrl, unsigned int pin)
        return !(readl(hostown) & BIT(gpp_offset));
 }

+static void intel_pad_force_hostown(struct intel_pinctrl *pctrl,
unsigned int pin)
+{
+       const struct intel_community *community;
+       const struct intel_padgroup *padgrp;
+       unsigned int offset, gpp_offset;
+       void __iomem *hostown;
+
+       community = intel_get_community(pctrl, pin);
+       if (!community)
+               return;
+       if (!community->hostown_offset)
+               return;
+
+       padgrp = intel_community_get_padgroup(community, pin);
+       if (!padgrp)
+               return;
+
+       gpp_offset = padgroup_offset(padgrp, pin);
+       offset = community->hostown_offset + padgrp->reg_num * 4;
+       hostown = community->regs + offset;
+
+       writel(readl(hostown) | BIT(gpp_offset), hostown);
+
+       return;
+}
+
 static bool intel_pad_locked(struct intel_pinctrl *pctrl, unsigned int pin)
 {
        struct intel_community *community;
@@ -1318,6 +1373,11 @@ int intel_pinctrl_probe(struct platform_device *pdev,
        pctrl->soc = soc_data;
        raw_spin_lock_init(&pctrl->lock);

+       if (dmi_first_match(dmi_retain_hostown_table)) {
+               pctrl->quirks |= PINCTRL_QUIRK_KEEP_HOSTOWN;
+               dev_info(&pdev->dev, "enabling KEEP_HOSTOWN quirk on
this hw\n");
+       }
+
        /*
         * Make a copy of the communities which we can use to hold pointers
         * to the registers.
@@ -1549,6 +1609,13 @@ int intel_pinctrl_resume(struct device *dev)
                if (!intel_pinctrl_should_save(pctrl, desc->number))
                        continue;

+               if ((pctrl->quirks & PINCTRL_QUIRK_KEEP_HOSTOWN) &&
+                   intel_pad_acpi_mode(pctrl, desc->number)) {
+                       intel_pad_force_hostown(pctrl, desc->number);
+                       dev_dbg(dev, "restored hostown for pin %d\n",
+                               desc->number);
+               }
+
                padcfg = intel_get_padcfg(pctrl, desc->number, PADCFG0);
                val = readl(padcfg) & ~PADCFG0_GPIORXSTATE;
                if (val != pads[i].padcfg0) {
--
2.21.0

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

[Mika Westerberg]

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@endlessm.com> wrote:
> >
> > On Thu, Mar 28, 2019 at 5:17 PM Andy Shevchenko
> > <andriy.shevchenko@intel.com> 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).

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

[Chris Chiu]

On Thu, Mar 28, 2019 at 8:34 PM Mika Westerberg
<mika.westerberg@linux.intel.com> 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@endlessm.com> wrote:
> > >
> > > On Thu, Mar 28, 2019 at 5:17 PM Andy Shevchenko
> > > <andriy.shevchenko@intel.com> 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?

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.


Chris

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

[Mika Westerberg]

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@linux.intel.com> 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@endlessm.com> wrote:
> > > >
> > > > On Thu, Mar 28, 2019 at 5:17 PM Andy Shevchenko
> > > > <andriy.shevchenko@intel.com> 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.

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

[Chris Chiu]

On Mon, Apr 1, 2019 at 3:49 PM Mika Westerberg
<mika.westerberg@linux.intel.com> 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@linux.intel.com> 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@endlessm.com> wrote:
> > > > >
> > > > > On Thu, Mar 28, 2019 at 5:17 PM Andy Shevchenko
> > > > > <andriy.shevchenko@intel.com> 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) {

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

[Andy Shevchenko]

On Mon, Apr 01, 2019 at 06:41:57PM +0800, Chris Chiu wrote:
> On Mon, Apr 1, 2019 at 3:49 PM Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Fri, Mar 29, 2019 at 04:38:20PM +0800, Chris Chiu wrote:

> > 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

Thanks for the patch.
My comments below.

> 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;

This is wrong. We have one register per entire (*) group of pins to keep host
ownership. Basically it's a mask.

*) if it's <= 32, otherwise there are more registers. But in any case it's 1
bit per pin, and not 32 bits.

>         for (i = 0; i < pctrl->soc->npins; i++)

Thus, the actual actions should mimic what we do for interrupt mask.

-- 
With Best Regards,
Andy Shevchenko

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

[Chris Chiu]

On Mon, Apr 1, 2019 at 8:23 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Mon, Apr 01, 2019 at 06:41:57PM +0800, Chris Chiu wrote:
>
> Thanks for the patch.
> My comments below.
>
> > 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;
>
> This is wrong. We have one register per entire (*) group of pins to keep host
> ownership. Basically it's a mask.
>
> *) if it's <= 32, otherwise there are more registers. But in any case it's 1
> bit per pin, and not 32 bits.
>
> >         for (i = 0; i < pctrl->soc->npins; i++)
>
> Thus, the actual actions should mimic what we do for interrupt mask.
>
> --
> With Best Regards,
> Andy Shevchenko
>

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.

--- a/drivers/pinctrl/intel/pinctrl-intel.c
+++ b/drivers/pinctrl/intel/pinctrl-intel.c
@@ -81,6 +81,7 @@ struct intel_pad_context {

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

 struct intel_pinctrl_context {
@@ -117,6 +118,10 @@ struct intel_pinctrl {
 #define pin_to_padno(c, p)     ((p) - (c)->pin_base)
 #define padgroup_offset(g, p)  ((p) - (g)->base)

+#ifdef CONFIG_PM_SLEEP
+static void intel_save_hostown(struct intel_pinctrl *pctrl, unsigned int pin);
+#endif
+
 static struct intel_community *intel_get_community(struct intel_pinctrl *pctrl,
                                                   unsigned int pin)
 {
@@ -456,7 +461,9 @@ static int intel_gpio_request_enable(struct
pinctrl_dev *pctldev,
        intel_gpio_set_gpio_mode(padcfg0);
        /* 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
        raw_spin_unlock_irqrestore(&pctrl->lock, flags);

        return 0;
@@ -1284,7 +1291,7 @@ static int intel_pinctrl_pm_init(struct
intel_pinctrl *pctrl)

        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;
        }

        pctrl->context.pads = pads;
@@ -1447,6 +1461,43 @@ int intel_pinctrl_probe_by_uid(struct
platform_device *pdev)
 EXPORT_SYMBOL_GPL(intel_pinctrl_probe_by_uid);

 #ifdef CONFIG_PM_SLEEP
+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;
+}
+
 static bool intel_pinctrl_should_save(struct intel_pinctrl *pctrl,
unsigned int pin)
 {
        const struct pin_desc *pd = pin_desc_get(pctrl->pctldev, pin);
@@ -1588,6 +1639,17 @@ int intel_pinctrl_resume(struct device *dev)
                        dev_dbg(dev, "restored mask %d/%u %#08x\n", i, gpp,
                                readl(base + gpp * 4));
                }
+
+               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));
+                       }
+               }
        }

        return 0;

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

[Andy Shevchenko]

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@intel.com> 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

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

[Chris Chiu]

On Tue, Apr 2, 2019 at 7:58 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
> > +               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
>
>

Thanks for your great comment. I remove the useless hostown save function
and make the following change in ->resume() to detect and restore the abnormal
HOSTSW_OWN. Please help comment if there're still problems. Thanks.

@@ -1588,6 +1600,29 @@ int intel_pinctrl_resume(struct device *dev)
                        dev_dbg(dev, "restored mask %d/%u %#08x\n", i, gpp,
                                readl(base + gpp * 4));
                }
+
+               base = community->regs + community->hostown_offset;
+               for (gpp = 0; gpp < community->ngpps; gpp++) {
+                       const struct intel_padgroup *padgrp =
&community->gpps[i];
+                       unsigned int requested = 0;
+                       int j;
+
+                       if (padgrp->gpio_base < 0)
+                               continue;
+
+                       for (j = 0; j < padgrp->size; j++)
+                               if
(gpiochip_is_requested(&pctrl->chip, padgrp->gpio_base + j))
+                                       requested |= BIT(j);
+
+                       if (requested) {
+                               if (communities[i].hostown[gpp] !=
readl(base + gpp * 4)) {
+
writel(communities[i].hostown[gpp], base + gpp * 4);
+                                       dev_warn(dev, "hostown been
changed during resume\n");
+                                       dev_dbg(dev, "restored hostown
%d/%u %#08x\n", i, gpp,
+                                               readl(base + gpp * 4));
+                               }
+                       }
+               }
        }

        return 0;

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

[Andy Shevchenko]

On Wed, Apr 03, 2019 at 03:06:43PM +0800, Chris Chiu wrote:
> On Tue, Apr 2, 2019 at 7:58 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:

> > 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.

> Thanks for your great comment. I remove the useless hostown save function
> and make the following change in ->resume() to detect and restore the abnormal
> HOSTSW_OWN. Please help comment if there're still problems. Thanks.

> +               base = community->regs + community->hostown_offset;
> +               for (gpp = 0; gpp < community->ngpps; gpp++) {
> +                       const struct intel_padgroup *padgrp =
> &community->gpps[i];
> +                       unsigned int requested = 0;
> +                       int j;
> +
> +                       if (padgrp->gpio_base < 0)
> +                               continue;

> +
> +                       for (j = 0; j < padgrp->size; j++)
> +                               if
> (gpiochip_is_requested(&pctrl->chip, padgrp->gpio_base + j))
> +                                       requested |= BIT(j);
> +

This better to make as a separate helper function

static u32 intel_gpio_is_requested(chip, base, size)
{
	u32 requested = 0;
	unsigned int i;

	for () {
		if ()
			requested |= BIT();
	}
	return requested;
}

(Note u32 as a type)

> +                       if (requested) {
> +                               if (communities[i].hostown[gpp] !=
> readl(base + gpp * 4)) {
> +
> writel(communities[i].hostown[gpp], base + gpp * 4);

The idea here not to check this at all, but rather apply a mask.

u32 value;

...
value = readl();
value = (value & ~requested) | (hostown[gpp] & requested);
writel(value);

> +                                       dev_warn(dev, "hostown been
> changed during resume\n");
> +                                       dev_dbg(dev, "restored hostown
> %d/%u %#08x\n", i, gpp,
> +                                               readl(base + gpp * 4));
> +                               }
> +                       }
> +               }

-- 
With Best Regards,
Andy Shevchenko

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

[Chris Chiu]

On Wed, Apr 3, 2019 at 9:06 PM Andy Shevchenko
<andriy.shevchenko@intel.com> wrote:
>
> On Wed, Apr 03, 2019 at 03:06:43PM +0800, Chris Chiu wrote:
> > On Tue, Apr 2, 2019 at 7:58 PM Andy Shevchenko
> > <andriy.shevchenko@intel.com> wrote:
>
> > > 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.
>
> > Thanks for your great comment. I remove the useless hostown save function
> > and make the following change in ->resume() to detect and restore the abnormal
> > HOSTSW_OWN. Please help comment if there're still problems. Thanks.
>
>
> This better to make as a separate helper function
>
> static u32 intel_gpio_is_requested(chip, base, size)
> {
>         u32 requested = 0;
>         unsigned int i;
>
>         for () {
>                 if ()
>                         requested |= BIT();
>         }
>         return requested;
> }
>
> (Note u32 as a type)
>

Thanks. I made a minor modification for the check function. I think to
pass a padgroup
as the argument would be better instead of base, size which I may need
to check if
the size > 32 (of course it shouldn't happen) or not.

+intel_padgroup_has_gpio_requested(struct gpio_chip *chip,  const
struct intel_padgroup *gpp)
+{
+       u32 requested = 0;
+       int i;
+
+       if (gpp == NULL)
+               return 0;
+
+       if (gpp->gpio_base < 0)
+               return 0;
+
+       for (i = 0; i < gpp->size; i++)
+               if (gpiochip_is_requested(chip, gpp->gpio_base + i))
+                       requested |= BIT(i);
+
+       return requested;
+}
+
 int intel_pinctrl_resume(struct device *dev)
 {
        struct platform_device *pdev = to_platform_device(dev);

> > +                       if (requested) {
> > +                               if (communities[i].hostown[gpp] !=
> > readl(base + gpp * 4)) {
> > +
> > writel(communities[i].hostown[gpp], base + gpp * 4);
>
> The idea here not to check this at all, but rather apply a mask.
>
> u32 value;
>
> ...
> value = readl();
> value = (value & ~requested) | (hostown[gpp] & requested);
> writel(value);
>

I made the following per your suggestion. So basically I don't need to show a
warning for the abnormal HOSTSW_OWN value change? I will submit a formal
patch for review if there's no big problem for these code logic. Please advise
if any. Thanks.

@@ -1588,6 +1619,22 @@ int intel_pinctrl_resume(struct device *dev)
                        dev_dbg(dev, "restored mask %d/%u %#08x\n", i, gpp,
                                readl(base + gpp * 4));
                }
+
+               base = community->regs + community->hostown_offset;
+               for (gpp = 0; gpp < community->ngpps; gpp++) {
+                       const struct intel_padgroup *padgrp =
&community->gpps[i];
+                       u32 requested =
intel_padgroup_has_gpio_requested(&pctrl->chip, padgrp);
+
+                       if (requested) {
+                               u32 value = readl(base + gpp * 4);
+                               u32 saved = communities[i].hostown[gpp];
+
+                               value = (value & ~requested) | (saved
& requested);
+                               writel(value, base + gpp * 4);
+                               dev_dbg(dev, "restored hostown %d/%u
%#08x\n", i, gpp,
+                                       readl(base + gpp * 4));
+                       }
+               }

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

[Andy Shevchenko]

On Thu, Apr 04, 2019 at 09:06:04PM +0800, Chris Chiu wrote:
> On Wed, Apr 3, 2019 at 9:06 PM Andy Shevchenko
> <andriy.shevchenko@intel.com> wrote:
> > On Wed, Apr 03, 2019 at 03:06:43PM +0800, Chris Chiu wrote:
> > > On Tue, Apr 2, 2019 at 7:58 PM Andy Shevchenko
> > > <andriy.shevchenko@intel.com> wrote:

> > This better to make as a separate helper function
> >
> > static u32 intel_gpio_is_requested(chip, base, size)
> > {
> >         u32 requested = 0;
> >         unsigned int i;
> >
> >         for () {
> >                 if ()
> >                         requested |= BIT();
> >         }
> >         return requested;
> > }
> >
> > (Note u32 as a type)
> >
> 
> Thanks. I made a minor modification for the check function. I think to
> pass a padgroup
> as the argument would be better instead of base, size which I may need
> to check if
> the size > 32 (of course it shouldn't happen) or not.

Group size is never bigger than 32 pins.

The helper should be pure GPIO, that's why I still would like to see base
there. Otherwise it would be layering violation.

> +intel_padgroup_has_gpio_requested(struct gpio_chip *chip,  const
> struct intel_padgroup *gpp)

Namespace is intel_gpio_ here.

> +{
> +       u32 requested = 0;
> +       int i;
> +
> +       if (gpp == NULL)
> +               return 0;
> +
> +       if (gpp->gpio_base < 0)
> +               return 0;
> +
> +       for (i = 0; i < gpp->size; i++)
> +               if (gpiochip_is_requested(chip, gpp->gpio_base + i))
> +                       requested |= BIT(i);
> +
> +       return requested;
> +}

> > > +                       if (requested) {
> > > +                               if (communities[i].hostown[gpp] !=
> > > readl(base + gpp * 4)) {
> > > +
> > > writel(communities[i].hostown[gpp], base + gpp * 4);
> >
> > The idea here not to check this at all, but rather apply a mask.
> >
> > u32 value;
> >
> > ...
> > value = readl();
> > value = (value & ~requested) | (hostown[gpp] & requested);
> > writel(value);
> >
> 
> I made the following per your suggestion. So basically I don't need to show a
> warning for the abnormal HOSTSW_OWN value change? I will submit a formal
> patch for review if there's no big problem for these code logic. Please advise
> if any. Thanks.

You still have all data to produce a warning if it's needed.

((value ^ hostown[gpp]) & requested) will return the changed bits.


> +               base = community->regs + community->hostown_offset;
> +               for (gpp = 0; gpp < community->ngpps; gpp++) {
> +                       const struct intel_padgroup *padgrp =
> &community->gpps[i];
> +                       u32 requested =
> intel_padgroup_has_gpio_requested(&pctrl->chip, padgrp);
> +

> +                       if (requested) {

You may not need this check at all.

> +                               u32 value = readl(base + gpp * 4);
> +                               u32 saved = communities[i].hostown[gpp];
> +
> +                               value = (value & ~requested) | (saved
> & requested);
> +                               writel(value, base + gpp * 4);

It's possible to split this as well to another helper function.

static void intel_gpio_update_pad_mode(void __iomem *hostown, u32 mask, u32 value)
{
	...
}

> +                               dev_dbg(dev, "restored hostown %d/%u
> %#08x\n", i, gpp,
> +                                       readl(base + gpp * 4));
> +                       }
> +               }

-- 
With Best Regards,
Andy Shevchenko