pinctrl: intel: save HOSTSW_OWN register over suspend/resume

Mika Westerberg <[email protected]> 於 2017年11月15日 下午4:04 寫道：

Quoted 10 lines · 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.

Hmm, are you saying it is initially not in ACPI mode but after resume it magically is?

Yes, that’s the most weird part.

Quoted 1 line · Chris Chiu

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?

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.

Quoted 4 lines · 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.

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.

Quoted 6 lines · Mika Westerberg

Quoted 4 lines · 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.

Quoted 2 lines

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.

Quoted 8 lines · Chris Chiu

Quoted 6 lines · Mika Westerberg

Quoted 4 lines · 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.

Quoted 5 lines

Quoted 2 lines

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.

Quoted 21 lines · Mika Westerberg

Quoted 8 lines · Chris Chiu

Quoted 6 lines · Mika Westerberg

Quoted 4 lines · 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.

Quoted 5 lines

Quoted 2 lines

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

Quoted 4 lines

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

Quoted 23 lines · Chris Chiu

Quoted 21 lines · Mika Westerberg

Quoted 8 lines · Chris Chiu

Quoted 6 lines · Mika Westerberg

Quoted 4 lines · 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.

Quoted 5 lines

Quoted 2 lines

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

Quoted 7 lines

Quoted 4 lines

  # 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

Quoted 6 lines · Chris Chiu

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.

Quoted 1 line

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.

Quoted 6 lines · Chris Chiu

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.

Quoted 1 line

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.

Hi Mika,

Digging up this old thread again...

Quoted 13 lines · Mika Westerberg

Quoted 6 lines · Chris Chiu

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.

Quoted 1 line

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

Quoted 7 lines

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.

Quoted 25 lines · Daniel Drake

Quoted 12 lines · Mika Westerberg

Quoted 6 lines · Chris Chiu

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.

Quoted 1 line

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

Quoted 7 lines

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.

Quoted 6 lines

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.

Quoted 9 lines

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.

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

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.

Quoted 2 lines · Andy Shevchenko

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.

Quoted 1 line

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.

Quoted 2 lines

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

Quoted 22 lines · Daniel Drake

Quoted 2 lines · Andy Shevchenko

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.

Quoted 1 line

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.

Quoted 2 lines

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

Quoted 4 lines

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.

Quoted 28 lines · Chris Chiu

Quoted 26 lines · Daniel Drake

Quoted 2 lines · Andy Shevchenko

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.

Quoted 1 line

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.

Quoted 2 lines

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

Quoted 3 lines

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

Quoted 30 lines · Mika Westerberg

Quoted 28 lines · Chris Chiu

Quoted 26 lines · Daniel Drake

Quoted 2 lines · Andy Shevchenko

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.

Quoted 1 line

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.

Quoted 2 lines

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

Quoted 8 lines

Quoted 3 lines

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.

Quoted 41 lines · Chris Chiu

Quoted 30 lines · Mika Westerberg

Quoted 28 lines · Chris Chiu

Quoted 26 lines · Daniel Drake

Quoted 2 lines · Andy Shevchenko

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.

Quoted 1 line

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.

Quoted 2 lines

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

Quoted 8 lines

Quoted 3 lines

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

Quoted 8 lines

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.

Quoted 3 lines

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.

Quoted 4 lines · Mika Westerberg

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

On Mon, Apr 01, 2019 at 06:41:57PM +0800, Chris Chiu wrote:

Thanks for the patch.
My comments below.

drivers/pinctrl/intel/pinctrl-intel.c struct intel_pad_context { · line 81

Quoted 11 lines

drivers/pinctrl/intel/pinctrl-intel.c +1 -0

index 8cda7b535b02..d1cfa5adef9b 100644
@@ -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.

drivers/pinctrl/intel/pinctrl-intel.c struct intel_pad_context { · line 82

Quoted 1 line

drivers/pinctrl/intel/pinctrl-intel.c +0 -0

        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

Quoted 1 line · Andy Shevchenko

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;

        for (i = 0; i < pctrl->ncommunities; i++) {
                struct intel_community *community = &pctrl->communities[i];

(quoted diff) +1 -1

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

(quoted diff) +29 -0

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

+}

Review

Quoted 13 lines

(quoted diff) +6 -0

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

(quoted diff) +2 -0

+                               dev_warn(dev, "hostown changed after resume\n");
+                               dev_dbg(dev, "restored hostown %d/%u

%#08x\n", i, gpp,

(quoted diff) +3 -0

+                                       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

Quoted 7 lines · Andy Shevchenko

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.

Quoted 7 lines

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.

(quoted diff) +3 -0

+               base = community->regs + community->hostown_offset;
+               for (gpp = 0; gpp < community->ngpps; gpp++) {
+                       const struct intel_padgroup *padgrp =

&community->gpps[i];

(quoted diff) +8 -0

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

(quoted diff) +2 -0

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

(quoted diff) +4 -0

+                                               readl(base + gpp * 4));
+                               }
+                       }
+               }

Quoted 15 lines · Chris Chiu

Quoted 12 lines · Andy Shevchenko

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)

Quoted 6 lines

Quoted 2 lines

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)

Quoted 5 lines

(quoted diff) +2 -0

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

Quoted 18 lines · Andy Shevchenko

Quoted 2 lines · Chris Chiu

On Tue, Apr 2, 2019 at 7:58 PM Andy Shevchenko
<[email protected]> 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;
}

Quoted 2 lines

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

(quoted diff) +16 -0

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

Quoted 7 lines

Quoted 6 lines

(quoted diff) +2 -0

+                       if (requested) {
+                               if (communities[i].hostown[gpp] !=

readl(base + gpp * 4)) {
+
writel(communities[i].hostown[gpp], base + gpp * 4);

Quoted 9 lines

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.

(quoted diff) +4 -0

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

(quoted diff) +3 -0

+                                       readl(base + gpp * 4));
+                       }
+               }