| Message ID | 20210308152505.3762055-1-luzmaximilian@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | Revert "pinctrl: intel: Split intel_pinctrl_add_padgroups() for better maintenance" | expand |
On Mon, Mar 08, 2021 at 04:25:05PM +0100, Maximilian Luz wrote: > Following commit 036e126c72eb ("pinctrl: intel: Split > intel_pinctrl_add_padgroups() for better maintenance"), > gpiochip_get_desc() is broken on some Kaby Lake R devices (specifically > a Microsoft Surface Book 2), returning -EINVAL for GPIOs that in reality > should be there (they are defined in ACPI and have been accessible > previously). Due to this, gpiod_get() fails with -ENOENT. > > Reverting this commit fixes that issue and the GPIOs in question are > accessible again. I would like to have more information. Can you enable PINCTRL and GPIO debug options in the kernel, and show dmesg output (when kernel command line has 'ignore_loglevel' option) for both working and non-working cases? Also if it's possible to have DSDT.dsl of the device in question along with output of `grep -H 15 /sys/bus/acpi/devices/*/status`. > There is probably a better option than straight up reverting this, so > consider this more of a bug-report. Indeed.
On Mon, Mar 08, 2021 at 05:35:59PM +0200, Andy Shevchenko wrote: > On Mon, Mar 08, 2021 at 04:25:05PM +0100, Maximilian Luz wrote: > > Following commit 036e126c72eb ("pinctrl: intel: Split > > intel_pinctrl_add_padgroups() for better maintenance"), > > gpiochip_get_desc() is broken on some Kaby Lake R devices (specifically > > a Microsoft Surface Book 2), returning -EINVAL for GPIOs that in reality > > should be there (they are defined in ACPI and have been accessible > > previously). Due to this, gpiod_get() fails with -ENOENT. > > > > Reverting this commit fixes that issue and the GPIOs in question are > > accessible again. > > I would like to have more information. > Can you enable PINCTRL and GPIO debug options in the kernel, and show dmesg > output (when kernel command line has 'ignore_loglevel' option) for both working > and non-working cases? > > Also if it's possible to have DSDT.dsl of the device in question along with > output of `grep -H 15 /sys/bus/acpi/devices/*/status`. > > > There is probably a better option than straight up reverting this, so > > consider this more of a bug-report. > > Indeed. Can you test if the below helps (probably you have to apply it by editing the file manually): --- a/drivers/pinctrl/intel/pinctrl-intel.c +++ b/drivers/pinctrl/intel/pinctrl-intel.c @@ -1392,6 +1392,7 @@ static int intel_pinctrl_add_padgroups_by_size(struct intel_pinctrl *pctrl, gpps[i].size = min(gpp_size, npins); npins -= gpps[i].size; + gpps[i].gpio_base = gpps[i].base; gpps[i].padown_num = padown_num;
On 3/8/21 4:35 PM, Andy Shevchenko wrote: > On Mon, Mar 08, 2021 at 04:25:05PM +0100, Maximilian Luz wrote: >> Following commit 036e126c72eb ("pinctrl: intel: Split >> intel_pinctrl_add_padgroups() for better maintenance"), >> gpiochip_get_desc() is broken on some Kaby Lake R devices (specifically >> a Microsoft Surface Book 2), returning -EINVAL for GPIOs that in reality >> should be there (they are defined in ACPI and have been accessible >> previously). Due to this, gpiod_get() fails with -ENOENT. >> >> Reverting this commit fixes that issue and the GPIOs in question are >> accessible again. > > I would like to have more information. > Can you enable PINCTRL and GPIO debug options in the kernel, and show dmesg > output (when kernel command line has 'ignore_loglevel' option) for both working > and non-working cases? Sure. Here are dmesg logs for: - Kernel v5.12-rc2 (not working): https://paste.ubuntu.com/p/HVZybcvQDH/ - Kernel v5.12-rc2 with 036e126c72eb reverted: https://paste.ubuntu.com/p/hwcXFvhcBd/ > Also if it's possible to have DSDT.dsl of the device in question along with > output of `grep -H 15 /sys/bus/acpi/devices/*/status`. You can find the DSDT and a full ACPI dump at [1] and GPIOs that fail at [2] and [3]. [1]: https://github.com/linux-surface/acpidumps/tree/master/surface_book_2 [2]: https://github.com/linux-surface/acpidumps/blob/62972f0d806cef45ca01341e3cfbabc04c6dd583/surface_book_2/dsdt.dsl#L15274-L15285 [3]: https://github.com/linux-surface/acpidumps/blob/62972f0d806cef45ca01341e3cfbabc04c6dd583/surface_book_2/dsdt.dsl#L17947-L17982 `grep -H 15 /sys/bus/acpi/devices/*/status` yields /sys/bus/acpi/devices/ACPI0003:00/status:15 /sys/bus/acpi/devices/ACPI000C:00/status:15 /sys/bus/acpi/devices/ACPI000E:00/status:15 /sys/bus/acpi/devices/device:16/status:15 /sys/bus/acpi/devices/device:17/status:15 /sys/bus/acpi/devices/device:31/status:15 /sys/bus/acpi/devices/device:71/status:15 /sys/bus/acpi/devices/INT33A1:00/status:15 /sys/bus/acpi/devices/INT33BE:00/status:15 /sys/bus/acpi/devices/INT3400:00/status:15 /sys/bus/acpi/devices/INT3403:01/status:15 /sys/bus/acpi/devices/INT3403:02/status:15 /sys/bus/acpi/devices/INT3403:06/status:15 /sys/bus/acpi/devices/INT3403:07/status:15 /sys/bus/acpi/devices/INT3403:08/status:15 /sys/bus/acpi/devices/INT3403:09/status:15 /sys/bus/acpi/devices/INT3403:11/status:15 /sys/bus/acpi/devices/INT3407:00/status:15 /sys/bus/acpi/devices/INT344B:00/status:15 /sys/bus/acpi/devices/INT3472:00/status:15 /sys/bus/acpi/devices/INT3472:01/status:15 /sys/bus/acpi/devices/INT3472:02/status:15 /sys/bus/acpi/devices/INT347A:00/status:15 /sys/bus/acpi/devices/INT347E:00/status:15 /sys/bus/acpi/devices/INT3F0D:00/status:15 /sys/bus/acpi/devices/LNXPOWER:07/status:15 /sys/bus/acpi/devices/MSHW0005:00/status:15 /sys/bus/acpi/devices/MSHW0029:00/status:15 /sys/bus/acpi/devices/MSHW0036:00/status:15 /sys/bus/acpi/devices/MSHW0040:00/status:15 /sys/bus/acpi/devices/MSHW0042:00/status:15 /sys/bus/acpi/devices/MSHW0045:00/status:15 /sys/bus/acpi/devices/MSHW0084:00/status:15 /sys/bus/acpi/devices/MSHW0091:00/status:15 /sys/bus/acpi/devices/MSHW0107:00/status:15 /sys/bus/acpi/devices/MSHW0133:00/status:15 /sys/bus/acpi/devices/MSHW0153:00/status:15 /sys/bus/acpi/devices/NTC0103:00/status:15 /sys/bus/acpi/devices/PNP0103:00/status:15 /sys/bus/acpi/devices/PNP0C0D:00/status:15 This output is the same for both versions. >> There is probably a better option than straight up reverting this, so >> consider this more of a bug-report. > > Indeed.
On Mon, Mar 8, 2021 at 6:34 PM Maximilian Luz <luzmaximilian@gmail.com> wrote: > > On 3/8/21 4:35 PM, Andy Shevchenko wrote: > > On Mon, Mar 08, 2021 at 04:25:05PM +0100, Maximilian Luz wrote: > >> Following commit 036e126c72eb ("pinctrl: intel: Split > >> intel_pinctrl_add_padgroups() for better maintenance"), > >> gpiochip_get_desc() is broken on some Kaby Lake R devices (specifically > >> a Microsoft Surface Book 2), returning -EINVAL for GPIOs that in reality > >> should be there (they are defined in ACPI and have been accessible > >> previously). Due to this, gpiod_get() fails with -ENOENT. > >> > >> Reverting this commit fixes that issue and the GPIOs in question are > >> accessible again. > > > > I would like to have more information. > > Can you enable PINCTRL and GPIO debug options in the kernel, and show dmesg > > output (when kernel command line has 'ignore_loglevel' option) for both working > > and non-working cases? > > Sure. > > Here are dmesg logs for: > > - Kernel v5.12-rc2 (not working): https://paste.ubuntu.com/p/HVZybcvQDH/ Thanks! Yeah, yeah... Please, test my patch, I am quite sure it will fix the issue. [ 0.335705] gpio gpiochip0: (INT344B:00): created GPIO range 0->23 ==> INT344B:00 PIN 0->23 [ 0.335711] gpio gpiochip0: (INT344B:00): created GPIO range 0->23 ==> INT344B:00 PIN 24->47 [ 0.335716] gpio gpiochip0: (INT344B:00): created GPIO range 0->23 ==> INT344B:00 PIN 48->71 [ 0.335721] gpio gpiochip0: (INT344B:00): created GPIO range 0->23 ==> INT344B:00 PIN 72->95 [ 0.335725] gpio gpiochip0: (INT344B:00): created GPIO range 0->23 ==> INT344B:00 PIN 96->119 [ 0.335730] gpio gpiochip0: (INT344B:00): created GPIO range 0->23 ==> INT344B:00 PIN 120->143 [ 0.335734] gpio gpiochip0: (INT344B:00): created GPIO range 0->7 ==> INT344B:00 PIN 144->151 > - Kernel v5.12-rc2 with 036e126c72eb reverted: https://paste.ubuntu.com/p/hwcXFvhcBd/ > > > Also if it's possible to have DSDT.dsl of the device in question along with > > output of `grep -H 15 /sys/bus/acpi/devices/*/status`. > > You can find the DSDT and a full ACPI dump at [1] and GPIOs that fail at > [2] and [3]. > > [1]: https://github.com/linux-surface/acpidumps/tree/master/surface_book_2 > [2]: https://github.com/linux-surface/acpidumps/blob/62972f0d806cef45ca01341e3cfbabc04c6dd583/surface_book_2/dsdt.dsl#L15274-L15285 > [3]: https://github.com/linux-surface/acpidumps/blob/62972f0d806cef45ca01341e3cfbabc04c6dd583/surface_book_2/dsdt.dsl#L17947-L17982 > > `grep -H 15 /sys/bus/acpi/devices/*/status` yields > > /sys/bus/acpi/devices/ACPI0003:00/status:15 > /sys/bus/acpi/devices/ACPI000C:00/status:15 > /sys/bus/acpi/devices/ACPI000E:00/status:15 > /sys/bus/acpi/devices/device:16/status:15 > /sys/bus/acpi/devices/device:17/status:15 > /sys/bus/acpi/devices/device:31/status:15 > /sys/bus/acpi/devices/device:71/status:15 > /sys/bus/acpi/devices/INT33A1:00/status:15 > /sys/bus/acpi/devices/INT33BE:00/status:15 > /sys/bus/acpi/devices/INT3400:00/status:15 > /sys/bus/acpi/devices/INT3403:01/status:15 > /sys/bus/acpi/devices/INT3403:02/status:15 > /sys/bus/acpi/devices/INT3403:06/status:15 > /sys/bus/acpi/devices/INT3403:07/status:15 > /sys/bus/acpi/devices/INT3403:08/status:15 > /sys/bus/acpi/devices/INT3403:09/status:15 > /sys/bus/acpi/devices/INT3403:11/status:15 > /sys/bus/acpi/devices/INT3407:00/status:15 > /sys/bus/acpi/devices/INT344B:00/status:15 > /sys/bus/acpi/devices/INT3472:00/status:15 > /sys/bus/acpi/devices/INT3472:01/status:15 > /sys/bus/acpi/devices/INT3472:02/status:15 > /sys/bus/acpi/devices/INT347A:00/status:15 > /sys/bus/acpi/devices/INT347E:00/status:15 > /sys/bus/acpi/devices/INT3F0D:00/status:15 > /sys/bus/acpi/devices/LNXPOWER:07/status:15 > /sys/bus/acpi/devices/MSHW0005:00/status:15 > /sys/bus/acpi/devices/MSHW0029:00/status:15 > /sys/bus/acpi/devices/MSHW0036:00/status:15 > /sys/bus/acpi/devices/MSHW0040:00/status:15 > /sys/bus/acpi/devices/MSHW0042:00/status:15 > /sys/bus/acpi/devices/MSHW0045:00/status:15 > /sys/bus/acpi/devices/MSHW0084:00/status:15 > /sys/bus/acpi/devices/MSHW0091:00/status:15 > /sys/bus/acpi/devices/MSHW0107:00/status:15 > /sys/bus/acpi/devices/MSHW0133:00/status:15 > /sys/bus/acpi/devices/MSHW0153:00/status:15 > /sys/bus/acpi/devices/NTC0103:00/status:15 > /sys/bus/acpi/devices/PNP0103:00/status:15 > /sys/bus/acpi/devices/PNP0C0D:00/status:15 > > This output is the same for both versions. It was expected.
On 3/8/21 5:31 PM, Andy Shevchenko wrote: > On Mon, Mar 08, 2021 at 05:35:59PM +0200, Andy Shevchenko wrote: >> On Mon, Mar 08, 2021 at 04:25:05PM +0100, Maximilian Luz wrote: >>> Following commit 036e126c72eb ("pinctrl: intel: Split >>> intel_pinctrl_add_padgroups() for better maintenance"), >>> gpiochip_get_desc() is broken on some Kaby Lake R devices (specifically >>> a Microsoft Surface Book 2), returning -EINVAL for GPIOs that in reality >>> should be there (they are defined in ACPI and have been accessible >>> previously). Due to this, gpiod_get() fails with -ENOENT. >>> >>> Reverting this commit fixes that issue and the GPIOs in question are >>> accessible again. >> >> I would like to have more information. >> Can you enable PINCTRL and GPIO debug options in the kernel, and show dmesg >> output (when kernel command line has 'ignore_loglevel' option) for both working >> and non-working cases? >> >> Also if it's possible to have DSDT.dsl of the device in question along with >> output of `grep -H 15 /sys/bus/acpi/devices/*/status`. >> >>> There is probably a better option than straight up reverting this, so >>> consider this more of a bug-report. >> >> Indeed. > > > Can you test if the below helps (probably you have to apply it by editing > the file manually): > > --- a/drivers/pinctrl/intel/pinctrl-intel.c > +++ b/drivers/pinctrl/intel/pinctrl-intel.c > @@ -1392,6 +1392,7 @@ static int intel_pinctrl_add_padgroups_by_size(struct intel_pinctrl *pctrl, > gpps[i].size = min(gpp_size, npins); > npins -= gpps[i].size; > > + gpps[i].gpio_base = gpps[i].base; > gpps[i].padown_num = padown_num; > > That does fix the issue! Thanks for the fast response and fix! Regards, Max
On 3/8/21 5:42 PM, Andy Shevchenko wrote: > On Mon, Mar 8, 2021 at 6:34 PM Maximilian Luz <luzmaximilian@gmail.com> wrote: >> >> On 3/8/21 4:35 PM, Andy Shevchenko wrote: >>> On Mon, Mar 08, 2021 at 04:25:05PM +0100, Maximilian Luz wrote: >>>> Following commit 036e126c72eb ("pinctrl: intel: Split >>>> intel_pinctrl_add_padgroups() for better maintenance"), >>>> gpiochip_get_desc() is broken on some Kaby Lake R devices (specifically >>>> a Microsoft Surface Book 2), returning -EINVAL for GPIOs that in reality >>>> should be there (they are defined in ACPI and have been accessible >>>> previously). Due to this, gpiod_get() fails with -ENOENT. >>>> >>>> Reverting this commit fixes that issue and the GPIOs in question are >>>> accessible again. >>> >>> I would like to have more information. >>> Can you enable PINCTRL and GPIO debug options in the kernel, and show dmesg >>> output (when kernel command line has 'ignore_loglevel' option) for both working >>> and non-working cases? >> >> Sure. >> >> Here are dmesg logs for: >> >> - Kernel v5.12-rc2 (not working): https://paste.ubuntu.com/p/HVZybcvQDH/ > > Thanks! > > Yeah, yeah... Please, test my patch, I am quite sure it will fix the issue. It does indeed, thanks again! Regards, Max
On Mon, Mar 8, 2021 at 6:44 PM Maximilian Luz <luzmaximilian@gmail.com> wrote: > On 3/8/21 5:31 PM, Andy Shevchenko wrote: > > On Mon, Mar 08, 2021 at 05:35:59PM +0200, Andy Shevchenko wrote: > >> On Mon, Mar 08, 2021 at 04:25:05PM +0100, Maximilian Luz wrote: > >>> Following commit 036e126c72eb ("pinctrl: intel: Split > >>> intel_pinctrl_add_padgroups() for better maintenance"), > >>> gpiochip_get_desc() is broken on some Kaby Lake R devices (specifically > >>> a Microsoft Surface Book 2), returning -EINVAL for GPIOs that in reality > >>> should be there (they are defined in ACPI and have been accessible > >>> previously). Due to this, gpiod_get() fails with -ENOENT. > >>> > >>> Reverting this commit fixes that issue and the GPIOs in question are > >>> accessible again. > >> > >> I would like to have more information. > >> Can you enable PINCTRL and GPIO debug options in the kernel, and show dmesg > >> output (when kernel command line has 'ignore_loglevel' option) for both working > >> and non-working cases? > >> > >> Also if it's possible to have DSDT.dsl of the device in question along with > >> output of `grep -H 15 /sys/bus/acpi/devices/*/status`. > >> > >>> There is probably a better option than straight up reverting this, so > >>> consider this more of a bug-report. > >> > >> Indeed. > > > > > > Can you test if the below helps (probably you have to apply it by editing > > the file manually): > > > > --- a/drivers/pinctrl/intel/pinctrl-intel.c > > +++ b/drivers/pinctrl/intel/pinctrl-intel.c > > @@ -1392,6 +1392,7 @@ static int intel_pinctrl_add_padgroups_by_size(struct intel_pinctrl *pctrl, > > gpps[i].size = min(gpp_size, npins); > > npins -= gpps[i].size; > > > > + gpps[i].gpio_base = gpps[i].base; > > gpps[i].padown_num = padown_num; > > > > > > That does fix the issue! Thanks for the fast response and fix! I'm about to send the formal patch. I will add the Reported-and-tested-by tag if you are okay with it.
On 3/8/21 5:46 PM, Andy Shevchenko wrote: > On Mon, Mar 8, 2021 at 6:44 PM Maximilian Luz <luzmaximilian@gmail.com> wrote: >> On 3/8/21 5:31 PM, Andy Shevchenko wrote: >>> On Mon, Mar 08, 2021 at 05:35:59PM +0200, Andy Shevchenko wrote: >>>> On Mon, Mar 08, 2021 at 04:25:05PM +0100, Maximilian Luz wrote: >>>>> Following commit 036e126c72eb ("pinctrl: intel: Split >>>>> intel_pinctrl_add_padgroups() for better maintenance"), >>>>> gpiochip_get_desc() is broken on some Kaby Lake R devices (specifically >>>>> a Microsoft Surface Book 2), returning -EINVAL for GPIOs that in reality >>>>> should be there (they are defined in ACPI and have been accessible >>>>> previously). Due to this, gpiod_get() fails with -ENOENT. >>>>> >>>>> Reverting this commit fixes that issue and the GPIOs in question are >>>>> accessible again. >>>> >>>> I would like to have more information. >>>> Can you enable PINCTRL and GPIO debug options in the kernel, and show dmesg >>>> output (when kernel command line has 'ignore_loglevel' option) for both working >>>> and non-working cases? >>>> >>>> Also if it's possible to have DSDT.dsl of the device in question along with >>>> output of `grep -H 15 /sys/bus/acpi/devices/*/status`. >>>> >>>>> There is probably a better option than straight up reverting this, so >>>>> consider this more of a bug-report. >>>> >>>> Indeed. >>> >>> >>> Can you test if the below helps (probably you have to apply it by editing >>> the file manually): >>> >>> --- a/drivers/pinctrl/intel/pinctrl-intel.c >>> +++ b/drivers/pinctrl/intel/pinctrl-intel.c >>> @@ -1392,6 +1392,7 @@ static int intel_pinctrl_add_padgroups_by_size(struct intel_pinctrl *pctrl, >>> gpps[i].size = min(gpp_size, npins); >>> npins -= gpps[i].size; >>> >>> + gpps[i].gpio_base = gpps[i].base; >>> gpps[i].padown_num = padown_num; >>> >>> >> >> That does fix the issue! Thanks for the fast response and fix! > > I'm about to send the formal patch. I will add the > Reported-and-tested-by tag if you are okay with it. > Sure thing. Thanks, Max
diff --git a/drivers/pinctrl/intel/pinctrl-intel.c b/drivers/pinctrl/intel/pinctrl-intel.c index 8085782cd8f9..0fe6caf98a8a 100644 --- a/drivers/pinctrl/intel/pinctrl-intel.c +++ b/drivers/pinctrl/intel/pinctrl-intel.c @@ -1331,19 +1331,34 @@ static int intel_gpio_probe(struct intel_pinctrl *pctrl, int irq) return 0; } -static int intel_pinctrl_add_padgroups_by_gpps(struct intel_pinctrl *pctrl, - struct intel_community *community) +static int intel_pinctrl_add_padgroups(struct intel_pinctrl *pctrl, + struct intel_community *community) { struct intel_padgroup *gpps; + unsigned int npins = community->npins; unsigned int padown_num = 0; - size_t i, ngpps = community->ngpps; + size_t ngpps, i; + + if (community->gpps) + ngpps = community->ngpps; + else + ngpps = DIV_ROUND_UP(community->npins, community->gpp_size); gpps = devm_kcalloc(pctrl->dev, ngpps, sizeof(*gpps), GFP_KERNEL); if (!gpps) return -ENOMEM; for (i = 0; i < ngpps; i++) { - gpps[i] = community->gpps[i]; + if (community->gpps) { + gpps[i] = community->gpps[i]; + } else { + unsigned int gpp_size = community->gpp_size; + + gpps[i].reg_num = i; + gpps[i].base = community->pin_base + i * gpp_size; + gpps[i].size = min(gpp_size, npins); + npins -= gpps[i].size; + } if (gpps[i].size > 32) return -EINVAL; @@ -1361,38 +1376,6 @@ static int intel_pinctrl_add_padgroups_by_gpps(struct intel_pinctrl *pctrl, break; } - gpps[i].padown_num = padown_num; - padown_num += DIV_ROUND_UP(gpps[i].size * 4, 32); - } - - community->gpps = gpps; - - return 0; -} - -static int intel_pinctrl_add_padgroups_by_size(struct intel_pinctrl *pctrl, - struct intel_community *community) -{ - struct intel_padgroup *gpps; - unsigned int npins = community->npins; - unsigned int padown_num = 0; - size_t i, ngpps = DIV_ROUND_UP(npins, community->gpp_size); - - if (community->gpp_size > 32) - return -EINVAL; - - gpps = devm_kcalloc(pctrl->dev, ngpps, sizeof(*gpps), GFP_KERNEL); - if (!gpps) - return -ENOMEM; - - for (i = 0; i < ngpps; i++) { - unsigned int gpp_size = community->gpp_size; - - gpps[i].reg_num = i; - gpps[i].base = community->pin_base + i * gpp_size; - gpps[i].size = min(gpp_size, npins); - npins -= gpps[i].size; - gpps[i].padown_num = padown_num; /* @@ -1529,10 +1512,7 @@ static int intel_pinctrl_probe(struct platform_device *pdev, community->regs = regs; community->pad_regs = regs + offset; - if (community->gpps) - ret = intel_pinctrl_add_padgroups_by_gpps(pctrl, community); - else - ret = intel_pinctrl_add_padgroups_by_size(pctrl, community); + ret = intel_pinctrl_add_padgroups(pctrl, community); if (ret) return ret; }
Following commit 036e126c72eb ("pinctrl: intel: Split intel_pinctrl_add_padgroups() for better maintenance"), gpiochip_get_desc() is broken on some Kaby Lake R devices (specifically a Microsoft Surface Book 2), returning -EINVAL for GPIOs that in reality should be there (they are defined in ACPI and have been accessible previously). Due to this, gpiod_get() fails with -ENOENT. Reverting this commit fixes that issue and the GPIOs in question are accessible again. Signed-off-by: Maximilian Luz <luzmaximilian@gmail.com> --- There is probably a better option than straight up reverting this, so consider this more of a bug-report. Regards, Max --- drivers/pinctrl/intel/pinctrl-intel.c | 60 +++++++++------------------ 1 file changed, 20 insertions(+), 40 deletions(-)