diff mbox series

gpiolib: acpi: support override broken GPIO number in ACPI table

Message ID 20210226033919.8871-1-shawn.guo@linaro.org
State New
Headers show
Series gpiolib: acpi: support override broken GPIO number in ACPI table | expand

Commit Message

Shawn Guo Feb. 26, 2021, 3:39 a.m. UTC 
Running kernel with ACPI on Lenovo Flex 5G laptop, touchpad is just
not working.  That's because the GpioInt number of TSC2 node in ACPI
table is simply wrong, and the number even exceeds the maximum GPIO
lines.  As the touchpad works fine with Windows on the same machine,
presumably this is something Windows-ism.  Although it's obviously
a specification violation, believe of that Microsoft will fix this in
the near future is not really realistic.

It adds the support of overriding broken GPIO number in ACPI table
on particular machines, which are matched using DMI info.  Such
mechanism for fixing up broken firmware and ACPI table is not uncommon
in kernel.  And hopefully it can be useful for other machines that get
broken GPIO number coded in ACPI table.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/gpio/gpiolib-acpi.c | 63 ++++++++++++++++++++++++++++++++++++-
 1 file changed, 62 insertions(+), 1 deletion(-)

Comments

Andy Shevchenko Feb. 26, 2021, 9:12 a.m. UTC | #1 
On Fri, Feb 26, 2021 at 5:42 AM Shawn Guo <shawn.guo@linaro.org> wrote:
>
> Running kernel with ACPI on Lenovo Flex 5G laptop, touchpad is just
> not working.  That's because the GpioInt number of TSC2 node in ACPI
> table is simply wrong, and the number even exceeds the maximum GPIO
> lines.  As the touchpad works fine with Windows on the same machine,
> presumably this is something Windows-ism.  Although it's obviously
> a specification violation, believe of that Microsoft will fix this in
> the near future is not really realistic.
>
> It adds the support of overriding broken GPIO number in ACPI table
> on particular machines, which are matched using DMI info.  Such
> mechanism for fixing up broken firmware and ACPI table is not uncommon
> in kernel.  And hopefully it can be useful for other machines that get
> broken GPIO number coded in ACPI table.

Thanks for the report and patch.

First of all, have you reported the issue to Lenovo? At least they
will know that they did wrong.
Second, is it possible to have somewhere output of `acpidump -o
flex5g.dat` (the flex5g.dat file)?

And as Mika said once to one of mine patches "since you know the
number ahead there is no need to pollute GPIO ACPI library core with
this quirk". But in any case I would like to see the ACPI tables
first.
Andy Shevchenko Feb. 26, 2021, 11:19 a.m. UTC | #2 
On Fri, Feb 26, 2021 at 12:57:37PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 26, 2021 at 11:39 AM Shawn Guo <shawn.guo@linaro.org> wrote:
> > On Fri, Feb 26, 2021 at 11:12:07AM +0200, Andy Shevchenko wrote:
> > > On Fri, Feb 26, 2021 at 5:42 AM Shawn Guo <shawn.guo@linaro.org> wrote:
> > > > Running kernel with ACPI on Lenovo Flex 5G laptop, touchpad is just
> > > > not working.  That's because the GpioInt number of TSC2 node in ACPI
> > > > table is simply wrong, and the number even exceeds the maximum GPIO
> > > > lines.  As the touchpad works fine with Windows on the same machine,
> > > > presumably this is something Windows-ism.  Although it's obviously
> > > > a specification violation, believe of that Microsoft will fix this in
> > > > the near future is not really realistic.
> > > >
> > > > It adds the support of overriding broken GPIO number in ACPI table
> > > > on particular machines, which are matched using DMI info.  Such
> > > > mechanism for fixing up broken firmware and ACPI table is not uncommon
> > > > in kernel.  And hopefully it can be useful for other machines that get
> > > > broken GPIO number coded in ACPI table.
> > >
> > > Thanks for the report and patch.
> > >
> > > First of all, have you reported the issue to Lenovo? At least they
> > > will know that they did wrong.
> >
> > Yes, we are reporting this to Lenovo, but to be honest, we are not sure
> > how much they will care about it, as they are shipping the laptop with
> > Windows only.
> >
> > > Second, is it possible to have somewhere output of `acpidump -o
> > > flex5g.dat` (the flex5g.dat file)?
> >
> > https://raw.githubusercontent.com/aarch64-laptops/build/master/misc/lenovo-flex-5g/dsdt.dsl

Looking into DSDT I think the problem is much worse. First of all there are
many cases where pins like 0x140, 0x1c0, etc are being used. On top of that
there is no GPIO driver in the upstream (as far as I can see by HID, perhaps
there is a driver but for different HID. And I see that GPIO device consumes a
lot of Interrupts from GIC as well (it's ARM platfrom as far as I understand).

Looking at the Microsoft brain damaged way of understanding GPIOs and hardware
[1], I am afraid you really want to have a specific GPIO driver for this. So,
for now until we have better picture of what's going on, NAK to this patch.

[1]: https://docs.microsoft.com/en-us/windows-hardware/drivers/bringup/general-purpose-i-o--gpio-

  "...All banks have the same number of pins, except for the last bank, which
   might have fewer."

They added completely unnecessary mapping layer and brought a lot of confusion
to everybody (developers, users, etc).
Shawn Guo Feb. 27, 2021, 3:19 a.m. UTC | #3 
On Fri, Feb 26, 2021 at 01:19:21PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 26, 2021 at 12:57:37PM +0200, Andy Shevchenko wrote:
> > On Fri, Feb 26, 2021 at 11:39 AM Shawn Guo <shawn.guo@linaro.org> wrote:
> > > On Fri, Feb 26, 2021 at 11:12:07AM +0200, Andy Shevchenko wrote:
> > > > On Fri, Feb 26, 2021 at 5:42 AM Shawn Guo <shawn.guo@linaro.org> wrote:
> > > > > Running kernel with ACPI on Lenovo Flex 5G laptop, touchpad is just
> > > > > not working.  That's because the GpioInt number of TSC2 node in ACPI
> > > > > table is simply wrong, and the number even exceeds the maximum GPIO
> > > > > lines.  As the touchpad works fine with Windows on the same machine,
> > > > > presumably this is something Windows-ism.  Although it's obviously
> > > > > a specification violation, believe of that Microsoft will fix this in
> > > > > the near future is not really realistic.
> > > > >
> > > > > It adds the support of overriding broken GPIO number in ACPI table
> > > > > on particular machines, which are matched using DMI info.  Such
> > > > > mechanism for fixing up broken firmware and ACPI table is not uncommon
> > > > > in kernel.  And hopefully it can be useful for other machines that get
> > > > > broken GPIO number coded in ACPI table.
> > > >
> > > > Thanks for the report and patch.
> > > >
> > > > First of all, have you reported the issue to Lenovo? At least they
> > > > will know that they did wrong.
> > >
> > > Yes, we are reporting this to Lenovo, but to be honest, we are not sure
> > > how much they will care about it, as they are shipping the laptop with
> > > Windows only.
> > >
> > > > Second, is it possible to have somewhere output of `acpidump -o
> > > > flex5g.dat` (the flex5g.dat file)?
> > >
> > > https://raw.githubusercontent.com/aarch64-laptops/build/master/misc/lenovo-flex-5g/dsdt.dsl
> 
> Looking into DSDT I think the problem is much worse. First of all there are
> many cases where pins like 0x140, 0x1c0, etc are being used. On top of that
> there is no GPIO driver in the upstream (as far as I can see by HID, perhaps
> there is a driver but for different HID. And I see that GPIO device consumes a
> lot of Interrupts from GIC as well (it's ARM platfrom as far as I understand).

Yes, it's a laptop built on Qualcomm Snapdragon SC8180X SoC.  The GPIO
driver is generic for all Snapdragon SoCs, and has been available in
upstream for many years (for DT though). It can be found as the gpio_chip
implementation in MSM pinctrl driver [1].  The SC8180X specific part can
be found as pinctrl-sc8180x.c [2], and it's already working for DT boot.
The only missing piece is to add "QCOM040D" as the acpi_device_id to
support ACPI boot, and it will be submitted after 5.12-rc1 comes out.

> Looking at the Microsoft brain damaged way of understanding GPIOs and hardware
> [1], I am afraid you really want to have a specific GPIO driver for this. So,
> for now until we have better picture of what's going on, NAK to this patch.

Thanks for the pointer to Microsoft document.  On Snapdragon, we have
only one GPIO instance that accommodates all GPIO pins, so I'm not sure
that Microsoft GPIOs mapping layer is relevant here at all.

Please take a look at the GPIO driver, and feel free to let me know if
you need any further information to understand what's going on.

Shawn


[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pinctrl/qcom/pinctrl-msm.c#n713
[2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pinctrl/qcom/pinctrl-sc8180x.c
Shawn Guo Feb. 27, 2021, 3:46 a.m. UTC | #4 
On Fri, Feb 26, 2021 at 12:57:37PM +0200, Andy Shevchenko wrote:
> On Fri, Feb 26, 2021 at 11:39 AM Shawn Guo <shawn.guo@linaro.org> wrote:
> >
> > On Fri, Feb 26, 2021 at 11:12:07AM +0200, Andy Shevchenko wrote:
> > > On Fri, Feb 26, 2021 at 5:42 AM Shawn Guo <shawn.guo@linaro.org> wrote:
> > > >
> > > > Running kernel with ACPI on Lenovo Flex 5G laptop, touchpad is just
> > > > not working.  That's because the GpioInt number of TSC2 node in ACPI
> > > > table is simply wrong, and the number even exceeds the maximum GPIO
> > > > lines.  As the touchpad works fine with Windows on the same machine,
> > > > presumably this is something Windows-ism.  Although it's obviously
> > > > a specification violation, believe of that Microsoft will fix this in
> > > > the near future is not really realistic.
> > > >
> > > > It adds the support of overriding broken GPIO number in ACPI table
> > > > on particular machines, which are matched using DMI info.  Such
> > > > mechanism for fixing up broken firmware and ACPI table is not uncommon
> > > > in kernel.  And hopefully it can be useful for other machines that get
> > > > broken GPIO number coded in ACPI table.
> > >
> > > Thanks for the report and patch.
> > >
> > > First of all, have you reported the issue to Lenovo? At least they
> > > will know that they did wrong.
> >
> > Yes, we are reporting this to Lenovo, but to be honest, we are not sure
> > how much they will care about it, as they are shipping the laptop with
> > Windows only.
> >
> > > Second, is it possible to have somewhere output of `acpidump -o
> > > flex5g.dat` (the flex5g.dat file)?
> >
> > https://raw.githubusercontent.com/aarch64-laptops/build/master/misc/lenovo-flex-5g/dsdt.dsl
> >
> > > And as Mika said once to one of mine patches "since you know the
> > > number ahead there is no need to pollute GPIO ACPI library core with
> > > this quirk". But in any case I would like to see the ACPI tables
> > > first.
> >
> > Oh, so you had something similar already?  Could you point me to the
> > patch and discussion?
> 
> Similar, but might be not the same:
>  - patches in the upstream [1] (v3 applied), discussion [2]
>  - new version with some additional fixes [3]

Thanks for all the pointers.  It looks to me that it's the same problem
- the GPIO number in ACPI table is broken and needs an override from
kernel.  So I think what we need is a generic solution to a problem
not uncommon.  Rather than asking all different drivers to resolve the
same problem all over the kernel, I believe GPIO ACPI library is just
the right place.

Looking at your platform and problem, I realise that to be a generic
solution, my patch needs an additional device identification matching,
as one GPIO number that is broken for one device could be correct for
another.  I will improve it, so that your problem can be resolved by
simply adding a new entry to acpi_gpio_pin_override_table[].

Shawn
Andy Shevchenko March 1, 2021, 12:19 p.m. UTC | #5 
On Sat, Feb 27, 2021 at 11:46:42AM +0800, Shawn Guo wrote:
> On Fri, Feb 26, 2021 at 12:57:37PM +0200, Andy Shevchenko wrote:
> > On Fri, Feb 26, 2021 at 11:39 AM Shawn Guo <shawn.guo@linaro.org> wrote:
> > >
> > > On Fri, Feb 26, 2021 at 11:12:07AM +0200, Andy Shevchenko wrote:
> > > > On Fri, Feb 26, 2021 at 5:42 AM Shawn Guo <shawn.guo@linaro.org> wrote:
> > > > >
> > > > > Running kernel with ACPI on Lenovo Flex 5G laptop, touchpad is just
> > > > > not working.  That's because the GpioInt number of TSC2 node in ACPI
> > > > > table is simply wrong, and the number even exceeds the maximum GPIO
> > > > > lines.  As the touchpad works fine with Windows on the same machine,
> > > > > presumably this is something Windows-ism.  Although it's obviously
> > > > > a specification violation, believe of that Microsoft will fix this in
> > > > > the near future is not really realistic.
> > > > >
> > > > > It adds the support of overriding broken GPIO number in ACPI table
> > > > > on particular machines, which are matched using DMI info.  Such
> > > > > mechanism for fixing up broken firmware and ACPI table is not uncommon
> > > > > in kernel.  And hopefully it can be useful for other machines that get
> > > > > broken GPIO number coded in ACPI table.
> > > >
> > > > Thanks for the report and patch.
> > > >
> > > > First of all, have you reported the issue to Lenovo? At least they
> > > > will know that they did wrong.
> > >
> > > Yes, we are reporting this to Lenovo, but to be honest, we are not sure
> > > how much they will care about it, as they are shipping the laptop with
> > > Windows only.
> > >
> > > > Second, is it possible to have somewhere output of `acpidump -o
> > > > flex5g.dat` (the flex5g.dat file)?
> > >
> > > https://raw.githubusercontent.com/aarch64-laptops/build/master/misc/lenovo-flex-5g/dsdt.dsl
> > >
> > > > And as Mika said once to one of mine patches "since you know the
> > > > number ahead there is no need to pollute GPIO ACPI library core with
> > > > this quirk". But in any case I would like to see the ACPI tables
> > > > first.
> > >
> > > Oh, so you had something similar already?  Could you point me to the
> > > patch and discussion?
> > 
> > Similar, but might be not the same:
> >  - patches in the upstream [1] (v3 applied), discussion [2]
> >  - new version with some additional fixes [3]
> 
> Thanks for all the pointers.  It looks to me that it's the same problem
> - the GPIO number in ACPI table is broken and needs an override from
> kernel.

Not exactly. On Galileo Gen 2 platform it's broken in understandable way.
In your case it's different and I'm not sure at all that's considered "broken"
in the MS' eyes.

> So I think what we need is a generic solution to a problem
> not uncommon.  Rather than asking all different drivers to resolve the
> same problem all over the kernel, I believe GPIO ACPI library is just
> the right place.
> 
> Looking at your platform and problem, I realise that to be a generic
> solution, my patch needs an additional device identification matching,
> as one GPIO number that is broken for one device could be correct for
> another.  I will improve it, so that your problem can be resolved by
> simply adding a new entry to acpi_gpio_pin_override_table[].

Before any steps further I really want to see more information about that IP
and how firmware applied the numbering scheme.

If it's confidential, you may sent any insights privately.
Shawn Guo March 2, 2021, 12:27 a.m. UTC | #6 
On Mon, Mar 01, 2021 at 02:17:06PM +0200, Andy Shevchenko wrote:
> On Sat, Feb 27, 2021 at 11:19:45AM +0800, Shawn Guo wrote:
> > On Fri, Feb 26, 2021 at 01:19:21PM +0200, Andy Shevchenko wrote:
> > > On Fri, Feb 26, 2021 at 12:57:37PM +0200, Andy Shevchenko wrote:
> > > > On Fri, Feb 26, 2021 at 11:39 AM Shawn Guo <shawn.guo@linaro.org> wrote:
> > > > > On Fri, Feb 26, 2021 at 11:12:07AM +0200, Andy Shevchenko wrote:
> > > > > > On Fri, Feb 26, 2021 at 5:42 AM Shawn Guo <shawn.guo@linaro.org> wrote:
> > > > > > > Running kernel with ACPI on Lenovo Flex 5G laptop, touchpad is just
> > > > > > > not working.  That's because the GpioInt number of TSC2 node in ACPI
> > > > > > > table is simply wrong, and the number even exceeds the maximum GPIO
> > > > > > > lines.  As the touchpad works fine with Windows on the same machine,
> > > > > > > presumably this is something Windows-ism.  Although it's obviously
> > > > > > > a specification violation, believe of that Microsoft will fix this in
> > > > > > > the near future is not really realistic.
> > > > > > >
> > > > > > > It adds the support of overriding broken GPIO number in ACPI table
> > > > > > > on particular machines, which are matched using DMI info.  Such
> > > > > > > mechanism for fixing up broken firmware and ACPI table is not uncommon
> > > > > > > in kernel.  And hopefully it can be useful for other machines that get
> > > > > > > broken GPIO number coded in ACPI table.
> > > > > >
> > > > > > Thanks for the report and patch.
> > > > > >
> > > > > > First of all, have you reported the issue to Lenovo? At least they
> > > > > > will know that they did wrong.
> > > > >
> > > > > Yes, we are reporting this to Lenovo, but to be honest, we are not sure
> > > > > how much they will care about it, as they are shipping the laptop with
> > > > > Windows only.
> > > > >
> > > > > > Second, is it possible to have somewhere output of `acpidump -o
> > > > > > flex5g.dat` (the flex5g.dat file)?
> > > > >
> > > > > https://raw.githubusercontent.com/aarch64-laptops/build/master/misc/lenovo-flex-5g/dsdt.dsl
> > > 
> > > Looking into DSDT I think the problem is much worse. First of all there are
> > > many cases where pins like 0x140, 0x1c0, etc are being used. On top of that
> > > there is no GPIO driver in the upstream (as far as I can see by HID, perhaps
> > > there is a driver but for different HID. And I see that GPIO device consumes a
> > > lot of Interrupts from GIC as well (it's ARM platfrom as far as I understand).
> > 
> > Yes, it's a laptop built on Qualcomm Snapdragon SC8180X SoC.  The GPIO
> > driver is generic for all Snapdragon SoCs, and has been available in
> > upstream for many years (for DT though). It can be found as the gpio_chip
> > implementation in MSM pinctrl driver [1].  The SC8180X specific part can
> > be found as pinctrl-sc8180x.c [2], and it's already working for DT boot.
> > The only missing piece is to add "QCOM040D" as the acpi_device_id to
> > support ACPI boot, and it will be submitted after 5.12-rc1 comes out.
> > 
> > > Looking at the Microsoft brain damaged way of understanding GPIOs and hardware
> > > [1], I am afraid you really want to have a specific GPIO driver for this. So,
> > > for now until we have better picture of what's going on, NAK to this patch.
> > 
> > Thanks for the pointer to Microsoft document.  On Snapdragon, we have
> > only one GPIO instance that accommodates all GPIO pins, so I'm not sure
> > that Microsoft GPIOs mapping layer is relevant here at all.
> > 
> > Please take a look at the GPIO driver, and feel free to let me know if
> > you need any further information to understand what's going on.
> 
> Yes, I looked into the driver and see that it has 3 blocks of GPIOs (we call
> them communities, but in the driver the term 'tiles' is used) AFAIU (correct me
> if I'm wrong). And who knows how many banks in each of them.

I'm not sure that the 3 'tiles' means 3 blocks of GPIOs.  Maybe, @Bjorn
can help clarify.  But the ACPI table shows that there is only 'GIO0'
with 'QCOM040D' HID.

> 
> I'm afraid that MS does on his way and not yours.
> 
> Can we have TRM for GPIO IP used there and any evidence / document from
> firmware team about the implementation of the GPIO numbering in the ACPI
> (at Intel we have so called BIOS Writers Guide that is given to the customers
> where such info can be found)?

Unfortunately, I do not have the access to any sort of these documents.
But I looped in Jeffrey who is part of Qualcomm kernel/firmware team,
and should be able to help clarify GPIO numbering in the ACPI table.

Shawn

> > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pinctrl/qcom/pinctrl-msm.c#n713
> > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pinctrl/qcom/pinctrl-sc8180x.c
Shawn Guo March 2, 2021, 12:44 a.m. UTC | #7 
+ Jeffrey

On Mon, Mar 01, 2021 at 02:19:50PM +0200, Andy Shevchenko wrote:
> On Sat, Feb 27, 2021 at 11:46:42AM +0800, Shawn Guo wrote:

> > On Fri, Feb 26, 2021 at 12:57:37PM +0200, Andy Shevchenko wrote:

> > > On Fri, Feb 26, 2021 at 11:39 AM Shawn Guo <shawn.guo@linaro.org> wrote:

> > > >

> > > > On Fri, Feb 26, 2021 at 11:12:07AM +0200, Andy Shevchenko wrote:

> > > > > On Fri, Feb 26, 2021 at 5:42 AM Shawn Guo <shawn.guo@linaro.org> wrote:

> > > > > >

> > > > > > Running kernel with ACPI on Lenovo Flex 5G laptop, touchpad is just

> > > > > > not working.  That's because the GpioInt number of TSC2 node in ACPI

> > > > > > table is simply wrong, and the number even exceeds the maximum GPIO

> > > > > > lines.  As the touchpad works fine with Windows on the same machine,

> > > > > > presumably this is something Windows-ism.  Although it's obviously

> > > > > > a specification violation, believe of that Microsoft will fix this in

> > > > > > the near future is not really realistic.

> > > > > >

> > > > > > It adds the support of overriding broken GPIO number in ACPI table

> > > > > > on particular machines, which are matched using DMI info.  Such

> > > > > > mechanism for fixing up broken firmware and ACPI table is not uncommon

> > > > > > in kernel.  And hopefully it can be useful for other machines that get

> > > > > > broken GPIO number coded in ACPI table.

> > > > >

> > > > > Thanks for the report and patch.

> > > > >

> > > > > First of all, have you reported the issue to Lenovo? At least they

> > > > > will know that they did wrong.

> > > >

> > > > Yes, we are reporting this to Lenovo, but to be honest, we are not sure

> > > > how much they will care about it, as they are shipping the laptop with

> > > > Windows only.

> > > >

> > > > > Second, is it possible to have somewhere output of `acpidump -o

> > > > > flex5g.dat` (the flex5g.dat file)?

> > > >

> > > > https://raw.githubusercontent.com/aarch64-laptops/build/master/misc/lenovo-flex-5g/dsdt.dsl

> > > >

> > > > > And as Mika said once to one of mine patches "since you know the

> > > > > number ahead there is no need to pollute GPIO ACPI library core with

> > > > > this quirk". But in any case I would like to see the ACPI tables

> > > > > first.

> > > >

> > > > Oh, so you had something similar already?  Could you point me to the

> > > > patch and discussion?

> > > 

> > > Similar, but might be not the same:

> > >  - patches in the upstream [1] (v3 applied), discussion [2]

> > >  - new version with some additional fixes [3]

> > 

> > Thanks for all the pointers.  It looks to me that it's the same problem

> > - the GPIO number in ACPI table is broken and needs an override from

> > kernel.

> 

> Not exactly. On Galileo Gen 2 platform it's broken in understandable way.

> In your case it's different and I'm not sure at all that's considered "broken"

> in the MS' eyes.


At least, I was told by Jeffrey that MS admits this is something needs
to be fixed in the future.

> 

> > So I think what we need is a generic solution to a problem

> > not uncommon.  Rather than asking all different drivers to resolve the

> > same problem all over the kernel, I believe GPIO ACPI library is just

> > the right place.

> > 

> > Looking at your platform and problem, I realise that to be a generic

> > solution, my patch needs an additional device identification matching,

> > as one GPIO number that is broken for one device could be correct for

> > another.  I will improve it, so that your problem can be resolved by

> > simply adding a new entry to acpi_gpio_pin_override_table[].

> 

> Before any steps further I really want to see more information about that IP

> and how firmware applied the numbering scheme.


Deduced by those working GPIO numbers in ACPI table and how Linux kernel
is working, I think the GPIO is numbered without any bank thing.  All
available pins are just numbered linearly, and every pin can be
configured in GPIO mode.

> 

> If it's confidential, you may sent any insights privately.


Unfortunately, all those documents are confidential to me as well.

Shawn
Andy Shevchenko March 2, 2021, 10:36 a.m. UTC | #8 
On Tue, Mar 2, 2021 at 2:44 AM Shawn Guo <shawn.guo@linaro.org> wrote:
> On Mon, Mar 01, 2021 at 02:19:50PM +0200, Andy Shevchenko wrote:
> > On Sat, Feb 27, 2021 at 11:46:42AM +0800, Shawn Guo wrote:
> > > On Fri, Feb 26, 2021 at 12:57:37PM +0200, Andy Shevchenko wrote:
> > > > On Fri, Feb 26, 2021 at 11:39 AM Shawn Guo <shawn.guo@linaro.org> wrote:
> > > > > On Fri, Feb 26, 2021 at 11:12:07AM +0200, Andy Shevchenko wrote:
> > > > > > On Fri, Feb 26, 2021 at 5:42 AM Shawn Guo <shawn.guo@linaro.org> wrote:
> > > > > > >
> > > > > > > Running kernel with ACPI on Lenovo Flex 5G laptop, touchpad is just
> > > > > > > not working.  That's because the GpioInt number of TSC2 node in ACPI
> > > > > > > table is simply wrong, and the number even exceeds the maximum GPIO
> > > > > > > lines.  As the touchpad works fine with Windows on the same machine,
> > > > > > > presumably this is something Windows-ism.  Although it's obviously
> > > > > > > a specification violation, believe of that Microsoft will fix this in
> > > > > > > the near future is not really realistic.
> > > > > > >
> > > > > > > It adds the support of overriding broken GPIO number in ACPI table
> > > > > > > on particular machines, which are matched using DMI info.  Such
> > > > > > > mechanism for fixing up broken firmware and ACPI table is not uncommon
> > > > > > > in kernel.  And hopefully it can be useful for other machines that get
> > > > > > > broken GPIO number coded in ACPI table.
> > > > > >
> > > > > > Thanks for the report and patch.
> > > > > >
> > > > > > First of all, have you reported the issue to Lenovo? At least they
> > > > > > will know that they did wrong.
> > > > >
> > > > > Yes, we are reporting this to Lenovo, but to be honest, we are not sure
> > > > > how much they will care about it, as they are shipping the laptop with
> > > > > Windows only.
> > > > >
> > > > > > Second, is it possible to have somewhere output of `acpidump -o
> > > > > > flex5g.dat` (the flex5g.dat file)?
> > > > >
> > > > > https://raw.githubusercontent.com/aarch64-laptops/build/master/misc/lenovo-flex-5g/dsdt.dsl
> > > > >
> > > > > > And as Mika said once to one of mine patches "since you know the
> > > > > > number ahead there is no need to pollute GPIO ACPI library core with
> > > > > > this quirk". But in any case I would like to see the ACPI tables
> > > > > > first.
> > > > >
> > > > > Oh, so you had something similar already?  Could you point me to the
> > > > > patch and discussion?
> > > >
> > > > Similar, but might be not the same:
> > > >  - patches in the upstream [1] (v3 applied), discussion [2]
> > > >  - new version with some additional fixes [3]
> > >
> > > Thanks for all the pointers.  It looks to me that it's the same problem
> > > - the GPIO number in ACPI table is broken and needs an override from
> > > kernel.
> >
> > Not exactly. On Galileo Gen 2 platform it's broken in understandable way.
> > In your case it's different and I'm not sure at all that's considered "broken"
> > in the MS' eyes.
>
> At least, I was told by Jeffrey that MS admits this is something needs
> to be fixed in the future.

Yes, but as far as I understand we have already this firmware in the
wild, so we will have users stuck with it...

> > > So I think what we need is a generic solution to a problem
> > > not uncommon.  Rather than asking all different drivers to resolve the
> > > same problem all over the kernel, I believe GPIO ACPI library is just
> > > the right place.
> > >
> > > Looking at your platform and problem, I realise that to be a generic
> > > solution, my patch needs an additional device identification matching,
> > > as one GPIO number that is broken for one device could be correct for
> > > another.  I will improve it, so that your problem can be resolved by
> > > simply adding a new entry to acpi_gpio_pin_override_table[].
> >
> > Before any steps further I really want to see more information about that IP
> > and how firmware applied the numbering scheme.
>
> Deduced by those working GPIO numbers in ACPI table and how Linux kernel
> is working, I think the GPIO is numbered without any bank thing.  All
> available pins are just numbered linearly, and every pin can be
> configured in GPIO mode.

Can you be absolutely sure? If you have banks (MS core code since the
laptop runs Windows uses what is described on their site for GPIOs)
linear and then you test pins at the beginning of the region, you
won't have issues, you need to find a pin in each tile wishfully at
the end of it and test.

Better and easier way of course to ask MS to provide an insight on this.

> > If it's confidential, you may sent any insights privately.
>
> Unfortunately, all those documents are confidential to me as well.

Okay, since we have no solid proof of what's going on there, I prefer
for now not to touch GPIO ACPI core and do this quirk in the driver
(using standard methods: DMI strings / ACPI IDs / etc).

Otherwise I'm not convinced that we need a global quirk for that and
it might be needed in the future.
Andy Shevchenko March 2, 2021, 12:21 p.m. UTC | #9 
On Tue, Mar 02, 2021 at 08:27:26AM +0800, Shawn Guo wrote:
> On Mon, Mar 01, 2021 at 02:17:06PM +0200, Andy Shevchenko wrote:
> > On Sat, Feb 27, 2021 at 11:19:45AM +0800, Shawn Guo wrote:
> > > On Fri, Feb 26, 2021 at 01:19:21PM +0200, Andy Shevchenko wrote:
> > > > On Fri, Feb 26, 2021 at 12:57:37PM +0200, Andy Shevchenko wrote:
> > > > > On Fri, Feb 26, 2021 at 11:39 AM Shawn Guo <shawn.guo@linaro.org> wrote:
> > > > > > On Fri, Feb 26, 2021 at 11:12:07AM +0200, Andy Shevchenko wrote:
> > > > > > > On Fri, Feb 26, 2021 at 5:42 AM Shawn Guo <shawn.guo@linaro.org> wrote:
> > > > > > > > Running kernel with ACPI on Lenovo Flex 5G laptop, touchpad is just
> > > > > > > > not working.  That's because the GpioInt number of TSC2 node in ACPI
> > > > > > > > table is simply wrong, and the number even exceeds the maximum GPIO
> > > > > > > > lines.  As the touchpad works fine with Windows on the same machine,
> > > > > > > > presumably this is something Windows-ism.  Although it's obviously
> > > > > > > > a specification violation, believe of that Microsoft will fix this in
> > > > > > > > the near future is not really realistic.
> > > > > > > >
> > > > > > > > It adds the support of overriding broken GPIO number in ACPI table
> > > > > > > > on particular machines, which are matched using DMI info.  Such
> > > > > > > > mechanism for fixing up broken firmware and ACPI table is not uncommon
> > > > > > > > in kernel.  And hopefully it can be useful for other machines that get
> > > > > > > > broken GPIO number coded in ACPI table.
> > > > > > >
> > > > > > > Thanks for the report and patch.
> > > > > > >
> > > > > > > First of all, have you reported the issue to Lenovo? At least they
> > > > > > > will know that they did wrong.
> > > > > >
> > > > > > Yes, we are reporting this to Lenovo, but to be honest, we are not sure
> > > > > > how much they will care about it, as they are shipping the laptop with
> > > > > > Windows only.
> > > > > >
> > > > > > > Second, is it possible to have somewhere output of `acpidump -o
> > > > > > > flex5g.dat` (the flex5g.dat file)?
> > > > > >
> > > > > > https://raw.githubusercontent.com/aarch64-laptops/build/master/misc/lenovo-flex-5g/dsdt.dsl
> > > > 
> > > > Looking into DSDT I think the problem is much worse. First of all there are
> > > > many cases where pins like 0x140, 0x1c0, etc are being used. On top of that
> > > > there is no GPIO driver in the upstream (as far as I can see by HID, perhaps
> > > > there is a driver but for different HID. And I see that GPIO device consumes a
> > > > lot of Interrupts from GIC as well (it's ARM platfrom as far as I understand).
> > > 
> > > Yes, it's a laptop built on Qualcomm Snapdragon SC8180X SoC.  The GPIO
> > > driver is generic for all Snapdragon SoCs, and has been available in
> > > upstream for many years (for DT though). It can be found as the gpio_chip
> > > implementation in MSM pinctrl driver [1].  The SC8180X specific part can
> > > be found as pinctrl-sc8180x.c [2], and it's already working for DT boot.
> > > The only missing piece is to add "QCOM040D" as the acpi_device_id to
> > > support ACPI boot, and it will be submitted after 5.12-rc1 comes out.
> > > 
> > > > Looking at the Microsoft brain damaged way of understanding GPIOs and hardware
> > > > [1], I am afraid you really want to have a specific GPIO driver for this. So,
> > > > for now until we have better picture of what's going on, NAK to this patch.
> > > 
> > > Thanks for the pointer to Microsoft document.  On Snapdragon, we have
> > > only one GPIO instance that accommodates all GPIO pins, so I'm not sure
> > > that Microsoft GPIOs mapping layer is relevant here at all.
> > > 
> > > Please take a look at the GPIO driver, and feel free to let me know if
> > > you need any further information to understand what's going on.
> > 
> > Yes, I looked into the driver and see that it has 3 blocks of GPIOs (we call
> > them communities, but in the driver the term 'tiles' is used) AFAIU (correct me
> > if I'm wrong). And who knows how many banks in each of them.
> 
> I'm not sure that the 3 'tiles' means 3 blocks of GPIOs.  Maybe, @Bjorn
> can help clarify.  But the ACPI table shows that there is only 'GIO0'
> with 'QCOM040D' HID.

Yeah, I already got that ACPI there is screwed up...

> > I'm afraid that MS does on his way and not yours.
> > 
> > Can we have TRM for GPIO IP used there and any evidence / document from
> > firmware team about the implementation of the GPIO numbering in the ACPI
> > (at Intel we have so called BIOS Writers Guide that is given to the customers
> > where such info can be found)?
> 
> Unfortunately, I do not have the access to any sort of these documents.
> But I looped in Jeffrey who is part of Qualcomm kernel/firmware team,
> and should be able to help clarify GPIO numbering in the ACPI table.

Thanks! Will wait for new information then.

> > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pinctrl/qcom/pinctrl-msm.c#n713
> > > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pinctrl/qcom/pinctrl-sc8180x.c
Jeffrey Hugo March 3, 2021, 5:02 a.m. UTC | #10 
On 3/2/2021 5:21 AM, Andy Shevchenko wrote:
> On Tue, Mar 02, 2021 at 08:27:26AM +0800, Shawn Guo wrote:
>> On Mon, Mar 01, 2021 at 02:17:06PM +0200, Andy Shevchenko wrote:
>>> On Sat, Feb 27, 2021 at 11:19:45AM +0800, Shawn Guo wrote:
>>>> On Fri, Feb 26, 2021 at 01:19:21PM +0200, Andy Shevchenko wrote:
>>>>> On Fri, Feb 26, 2021 at 12:57:37PM +0200, Andy Shevchenko wrote:
>>>>>> On Fri, Feb 26, 2021 at 11:39 AM Shawn Guo <shawn.guo@linaro.org> wrote:
>>>>>>> On Fri, Feb 26, 2021 at 11:12:07AM +0200, Andy Shevchenko wrote:
>>>>>>>> On Fri, Feb 26, 2021 at 5:42 AM Shawn Guo <shawn.guo@linaro.org> wrote:
>>>>>>>>> Running kernel with ACPI on Lenovo Flex 5G laptop, touchpad is just
>>>>>>>>> not working.  That's because the GpioInt number of TSC2 node in ACPI
>>>>>>>>> table is simply wrong, and the number even exceeds the maximum GPIO
>>>>>>>>> lines.  As the touchpad works fine with Windows on the same machine,
>>>>>>>>> presumably this is something Windows-ism.  Although it's obviously
>>>>>>>>> a specification violation, believe of that Microsoft will fix this in
>>>>>>>>> the near future is not really realistic.
>>>>>>>>>
>>>>>>>>> It adds the support of overriding broken GPIO number in ACPI table
>>>>>>>>> on particular machines, which are matched using DMI info.  Such
>>>>>>>>> mechanism for fixing up broken firmware and ACPI table is not uncommon
>>>>>>>>> in kernel.  And hopefully it can be useful for other machines that get
>>>>>>>>> broken GPIO number coded in ACPI table.
>>>>>>>>
>>>>>>>> Thanks for the report and patch.
>>>>>>>>
>>>>>>>> First of all, have you reported the issue to Lenovo? At least they
>>>>>>>> will know that they did wrong.
>>>>>>>
>>>>>>> Yes, we are reporting this to Lenovo, but to be honest, we are not sure
>>>>>>> how much they will care about it, as they are shipping the laptop with
>>>>>>> Windows only.
>>>>>>>
>>>>>>>> Second, is it possible to have somewhere output of `acpidump -o
>>>>>>>> flex5g.dat` (the flex5g.dat file)?
>>>>>>>
>>>>>>> https://raw.githubusercontent.com/aarch64-laptops/build/master/misc/lenovo-flex-5g/dsdt.dsl
>>>>>
>>>>> Looking into DSDT I think the problem is much worse. First of all there are
>>>>> many cases where pins like 0x140, 0x1c0, etc are being used. On top of that
>>>>> there is no GPIO driver in the upstream (as far as I can see by HID, perhaps
>>>>> there is a driver but for different HID. And I see that GPIO device consumes a
>>>>> lot of Interrupts from GIC as well (it's ARM platfrom as far as I understand).
>>>>
>>>> Yes, it's a laptop built on Qualcomm Snapdragon SC8180X SoC.  The GPIO
>>>> driver is generic for all Snapdragon SoCs, and has been available in
>>>> upstream for many years (for DT though). It can be found as the gpio_chip
>>>> implementation in MSM pinctrl driver [1].  The SC8180X specific part can
>>>> be found as pinctrl-sc8180x.c [2], and it's already working for DT boot.
>>>> The only missing piece is to add "QCOM040D" as the acpi_device_id to
>>>> support ACPI boot, and it will be submitted after 5.12-rc1 comes out.
>>>>
>>>>> Looking at the Microsoft brain damaged way of understanding GPIOs and hardware
>>>>> [1], I am afraid you really want to have a specific GPIO driver for this. So,
>>>>> for now until we have better picture of what's going on, NAK to this patch.
>>>>
>>>> Thanks for the pointer to Microsoft document.  On Snapdragon, we have
>>>> only one GPIO instance that accommodates all GPIO pins, so I'm not sure
>>>> that Microsoft GPIOs mapping layer is relevant here at all.
>>>>
>>>> Please take a look at the GPIO driver, and feel free to let me know if
>>>> you need any further information to understand what's going on.
>>>
>>> Yes, I looked into the driver and see that it has 3 blocks of GPIOs (we call
>>> them communities, but in the driver the term 'tiles' is used) AFAIU (correct me
>>> if I'm wrong). And who knows how many banks in each of them.
>>
>> I'm not sure that the 3 'tiles' means 3 blocks of GPIOs.  Maybe, @Bjorn
>> can help clarify.  But the ACPI table shows that there is only 'GIO0'
>> with 'QCOM040D' HID.
> 
> Yeah, I already got that ACPI there is screwed up...
> 
>>> I'm afraid that MS does on his way and not yours.
>>>
>>> Can we have TRM for GPIO IP used there and any evidence / document from
>>> firmware team about the implementation of the GPIO numbering in the ACPI
>>> (at Intel we have so called BIOS Writers Guide that is given to the customers
>>> where such info can be found)?
>>
>> Unfortunately, I do not have the access to any sort of these documents.
>> But I looped in Jeffrey who is part of Qualcomm kernel/firmware team,
>> and should be able to help clarify GPIO numbering in the ACPI table.
> 
> Thanks! Will wait for new information then.

Sorry, just joining the thread now.  Hopefully I'm addressing everything 
targeted at me.

I used to do kernel work on MSMs, then kernel work on server CPUs, but 
now I do kernel work on AI accelerators.  Never was on the firmware 
team, but I have a lot of contacts in those areas.  On my own time, I 
support Linux on the Qualcomm laptops.

Its not MS that needs to fix things (although there is plenty of things 
I could point to that MS could fix), its the Qualcomm Windows FW folks. 
  They have told me a while ago they were planning on fixing this issue 
on some future chipset, but apparently that hasn't happened yet.  Sadly, 
once these laptops ship, they are in a frozen maintenance mode.

In my opinion, MS has allowed Qualcomm to get away with doing bad things 
in ACPI on the Qualcomm laptops.  The ACPI is not a true hardware 
description that is OS agnostic as it should be, and probably violates 
the spec in many ways.  Instead, the ACPI is written against the Windows 
drivers, and has a lot of OS driver crap pushed into it.

The GPIO description is one such thing.

As I understand it, any particular SoC will have a number of GPIOs 
supported by the TLMM.  0 - N.  Linux understands this.  However, in the 
ACPI of the Qualcomm Windows laptops, you will likely find atleast one 
GPIO number which exceeds this N.  These are "virtual" GPIOs, and are a 
construct of the Windows Qualcomm TLMM driver and how it interfaces with 
the frameworks within Windows.

Some GPIO lines can be configured as wakeup sources by routing them to a 
specific hardware block in the SoC (which block it is varies from SoC to 
SoC).  Windows has a specific weird way of handling this which requires 
a unique "GPIO chip" to handle.  GPIO chips in Windows contain 32 GPIOs, 
so for each wakeup GPIO, the TLMM driver creates a GPIO chip 
(essentially creating 32 GPIOs), and assigns the added GPIOs numbers 
which exceed N.  The TLMM driver has an internal mapping of which 
virtual GPIO number corresponds to which real GPIO.

So, ACPI says that some peripheral has GPIO N+X, which is not a real 
GPIO.  That peripheral goes and requests that GPIO, which gets routed to 
the TLMM driver, and the TLMM driver translates that number to the real 
GPIO, and provides the reference back to the peripheral, while also 
setting up the special wakeup hardware.

So, N+1 is the first supported wakup GPIO, N+1+32 is the next one, then 
N+1+32+32, and so on.

I see how this creates a nice mess for running Linux on these laptops, 
but I don't have a good idea how to work around it.  Per SoC, you'd need 
to know the mapping and translate it for ACPI when running the Windows 
version of the FW (yes most Qualcomm MSMs have OS specific firmware), 
but reject such gpio numbers when running other firmware, or I guess on 
different targets.

> 
>>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pinctrl/qcom/pinctrl-msm.c#n713
>>>> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pinctrl/qcom/pinctrl-sc8180x.c
>
Andy Shevchenko March 3, 2021, 8:06 a.m. UTC | #11 
On Tue, Mar 02, 2021 at 10:02:49PM -0700, Jeffrey Hugo wrote:
> On 3/2/2021 5:21 AM, Andy Shevchenko wrote:
> > On Tue, Mar 02, 2021 at 08:27:26AM +0800, Shawn Guo wrote:
> > > On Mon, Mar 01, 2021 at 02:17:06PM +0200, Andy Shevchenko wrote:
> > > > On Sat, Feb 27, 2021 at 11:19:45AM +0800, Shawn Guo wrote:
> > > > > On Fri, Feb 26, 2021 at 01:19:21PM +0200, Andy Shevchenko wrote:
> > > > > > On Fri, Feb 26, 2021 at 12:57:37PM +0200, Andy Shevchenko wrote:
> > > > > > > On Fri, Feb 26, 2021 at 11:39 AM Shawn Guo <shawn.guo@linaro.org> wrote:
> > > > > > > > On Fri, Feb 26, 2021 at 11:12:07AM +0200, Andy Shevchenko wrote:
> > > > > > > > > On Fri, Feb 26, 2021 at 5:42 AM Shawn Guo <shawn.guo@linaro.org> wrote:
> > > > > > > > > > Running kernel with ACPI on Lenovo Flex 5G laptop, touchpad is just
> > > > > > > > > > not working.  That's because the GpioInt number of TSC2 node in ACPI
> > > > > > > > > > table is simply wrong, and the number even exceeds the maximum GPIO
> > > > > > > > > > lines.  As the touchpad works fine with Windows on the same machine,
> > > > > > > > > > presumably this is something Windows-ism.  Although it's obviously
> > > > > > > > > > a specification violation, believe of that Microsoft will fix this in
> > > > > > > > > > the near future is not really realistic.
> > > > > > > > > > 
> > > > > > > > > > It adds the support of overriding broken GPIO number in ACPI table
> > > > > > > > > > on particular machines, which are matched using DMI info.  Such
> > > > > > > > > > mechanism for fixing up broken firmware and ACPI table is not uncommon
> > > > > > > > > > in kernel.  And hopefully it can be useful for other machines that get
> > > > > > > > > > broken GPIO number coded in ACPI table.
> > > > > > > > > 
> > > > > > > > > Thanks for the report and patch.
> > > > > > > > > 
> > > > > > > > > First of all, have you reported the issue to Lenovo? At least they
> > > > > > > > > will know that they did wrong.
> > > > > > > > 
> > > > > > > > Yes, we are reporting this to Lenovo, but to be honest, we are not sure
> > > > > > > > how much they will care about it, as they are shipping the laptop with
> > > > > > > > Windows only.
> > > > > > > > 
> > > > > > > > > Second, is it possible to have somewhere output of `acpidump -o
> > > > > > > > > flex5g.dat` (the flex5g.dat file)?
> > > > > > > > 
> > > > > > > > https://raw.githubusercontent.com/aarch64-laptops/build/master/misc/lenovo-flex-5g/dsdt.dsl
> > > > > > 
> > > > > > Looking into DSDT I think the problem is much worse. First of all there are
> > > > > > many cases where pins like 0x140, 0x1c0, etc are being used. On top of that
> > > > > > there is no GPIO driver in the upstream (as far as I can see by HID, perhaps
> > > > > > there is a driver but for different HID. And I see that GPIO device consumes a
> > > > > > lot of Interrupts from GIC as well (it's ARM platfrom as far as I understand).
> > > > > 
> > > > > Yes, it's a laptop built on Qualcomm Snapdragon SC8180X SoC.  The GPIO
> > > > > driver is generic for all Snapdragon SoCs, and has been available in
> > > > > upstream for many years (for DT though). It can be found as the gpio_chip
> > > > > implementation in MSM pinctrl driver [1].  The SC8180X specific part can
> > > > > be found as pinctrl-sc8180x.c [2], and it's already working for DT boot.
> > > > > The only missing piece is to add "QCOM040D" as the acpi_device_id to
> > > > > support ACPI boot, and it will be submitted after 5.12-rc1 comes out.
> > > > > 
> > > > > > Looking at the Microsoft brain damaged way of understanding GPIOs and hardware
> > > > > > [1], I am afraid you really want to have a specific GPIO driver for this. So,
> > > > > > for now until we have better picture of what's going on, NAK to this patch.
> > > > > 
> > > > > Thanks for the pointer to Microsoft document.  On Snapdragon, we have
> > > > > only one GPIO instance that accommodates all GPIO pins, so I'm not sure
> > > > > that Microsoft GPIOs mapping layer is relevant here at all.
> > > > > 
> > > > > Please take a look at the GPIO driver, and feel free to let me know if
> > > > > you need any further information to understand what's going on.
> > > > 
> > > > Yes, I looked into the driver and see that it has 3 blocks of GPIOs (we call
> > > > them communities, but in the driver the term 'tiles' is used) AFAIU (correct me
> > > > if I'm wrong). And who knows how many banks in each of them.
> > > 
> > > I'm not sure that the 3 'tiles' means 3 blocks of GPIOs.  Maybe, @Bjorn
> > > can help clarify.  But the ACPI table shows that there is only 'GIO0'
> > > with 'QCOM040D' HID.
> > 
> > Yeah, I already got that ACPI there is screwed up...
> > 
> > > > I'm afraid that MS does on his way and not yours.
> > > > 
> > > > Can we have TRM for GPIO IP used there and any evidence / document from
> > > > firmware team about the implementation of the GPIO numbering in the ACPI
> > > > (at Intel we have so called BIOS Writers Guide that is given to the customers
> > > > where such info can be found)?
> > > 
> > > Unfortunately, I do not have the access to any sort of these documents.
> > > But I looped in Jeffrey who is part of Qualcomm kernel/firmware team,
> > > and should be able to help clarify GPIO numbering in the ACPI table.
> > 
> > Thanks! Will wait for new information then.
> 
> Sorry, just joining the thread now.  Hopefully I'm addressing everything
> targeted at me.
> 
> I used to do kernel work on MSMs, then kernel work on server CPUs, but now I
> do kernel work on AI accelerators.  Never was on the firmware team, but I
> have a lot of contacts in those areas.  On my own time, I support Linux on
> the Qualcomm laptops.
> 
> Its not MS that needs to fix things (although there is plenty of things I
> could point to that MS could fix), its the Qualcomm Windows FW folks.  They
> have told me a while ago they were planning on fixing this issue on some
> future chipset, but apparently that hasn't happened yet.  Sadly, once these
> laptops ship, they are in a frozen maintenance mode.

I see. MS indeed loves Linux then :-)

> In my opinion, MS has allowed Qualcomm to get away with doing bad things in
> ACPI on the Qualcomm laptops.  The ACPI is not a true hardware description
> that is OS agnostic as it should be, and probably violates the spec in many
> ways.  Instead, the ACPI is written against the Windows drivers, and has a
> lot of OS driver crap pushed into it.

You meant "ACPI" -> "DSDT on the certain platform" I hope.

> The GPIO description is one such thing.
> 
> As I understand it, any particular SoC will have a number of GPIOs supported
> by the TLMM.  0 - N.  Linux understands this.  However, in the ACPI of the
> Qualcomm Windows laptops, you will likely find atleast one GPIO number which
> exceeds this N.  These are "virtual" GPIOs, and are a construct of the
> Windows Qualcomm TLMM driver and how it interfaces with the frameworks
> within Windows.
> 
> Some GPIO lines can be configured as wakeup sources by routing them to a
> specific hardware block in the SoC (which block it is varies from SoC to
> SoC).  Windows has a specific weird way of handling this which requires a
> unique "GPIO chip" to handle.  GPIO chips in Windows contain 32 GPIOs, so
> for each wakeup GPIO, the TLMM driver creates a GPIO chip (essentially
> creating 32 GPIOs), and assigns the added GPIOs numbers which exceed N.  The
> TLMM driver has an internal mapping of which virtual GPIO number corresponds
> to which real GPIO.
> 
> So, ACPI says that some peripheral has GPIO N+X, which is not a real GPIO.
> That peripheral goes and requests that GPIO, which gets routed to the TLMM
> driver, and the TLMM driver translates that number to the real GPIO, and
> provides the reference back to the peripheral, while also setting up the
> special wakeup hardware.
> 
> So, N+1 is the first supported wakup GPIO, N+1+32 is the next one, then
> N+1+32+32, and so on.
> 
> I see how this creates a nice mess for running Linux on these laptops, but I
> don't have a good idea how to work around it.  Per SoC, you'd need to know
> the mapping and translate it for ACPI when running the Windows version of
> the FW (yes most Qualcomm MSMs have OS specific firmware), but reject such
> gpio numbers when running other firmware, or I guess on different targets.

Thank, this makes a lot of sense to me and (unfortunately) I'm familiar with
this concept on some of x86 cheap tablets.

Since the mapping of those wake IRQs is totally platform specific, it needs a
platform driver. On above mentioned x86 platforms we have a one you may take as
an example (good or bad it's another story):
drivers/platform/x86/intel_int0002_vgpio.c.

I think you will need something like this somewhere in ARM platform
infrastructure in the Linux kernel.

That said, I don't see that those numbers are "broken", they have their own
meaning and specific mapping to the real GPIOs and it's so platform specific,
that we can't treat it as a quirk.

Thanks, Jeffrey, it is helpful!

> > > > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pinctrl/qcom/pinctrl-msm.c#n713
> > > > > [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pinctrl/qcom/pinctrl-sc8180x.c
Shawn Guo March 3, 2021, 8:45 a.m. UTC | #12 
On Wed, Mar 03, 2021 at 10:06:53AM +0200, Andy Shevchenko wrote:
> Since the mapping of those wake IRQs is totally platform specific, it needs a
> platform driver. On above mentioned x86 platforms we have a one you may take as
> an example (good or bad it's another story):
> drivers/platform/x86/intel_int0002_vgpio.c.
> 
> I think you will need something like this somewhere in ARM platform
> infrastructure in the Linux kernel.

Well, you have the Virtual GPIO controller defined in ACPI as device
"INT0002", but we do not have such a thing.  I'm not sure it makes much
sense to create a baseless driver.

> 
> That said, I don't see that those numbers are "broken", they have their own
> meaning and specific mapping to the real GPIOs and it's so platform specific,
> that we can't treat it as a quirk.

Those numbers have their own meaning only for Windows.  It's OS specific
rather than platform specific.  Snapdragon platform manual has explicit
numbering of every single GPIO pin.  Those broken numbers in ACPI table
violate the hardware specification and are *broken* to Linux which
implements GPIO driver properly.

Shawn
Andy Shevchenko March 3, 2021, 9:42 a.m. UTC | #13 
On Wed, Mar 03, 2021 at 04:45:09PM +0800, Shawn Guo wrote:
> On Wed, Mar 03, 2021 at 10:06:53AM +0200, Andy Shevchenko wrote:
> > Since the mapping of those wake IRQs is totally platform specific, it needs a
> > platform driver. On above mentioned x86 platforms we have a one you may take as
> > an example (good or bad it's another story):
> > drivers/platform/x86/intel_int0002_vgpio.c.
> > 
> > I think you will need something like this somewhere in ARM platform
> > infrastructure in the Linux kernel.
> 
> Well, you have the Virtual GPIO controller defined in ACPI as device
> "INT0002", but we do not have such a thing.  I'm not sure it makes much
> sense to create a baseless driver.

It has similarities and differences. In your case you need to have somewhere
some piece of the code that will do proper things, but it shouldn't be GPIO
ACPI layer.

> > That said, I don't see that those numbers are "broken", they have their own
> > meaning and specific mapping to the real GPIOs and it's so platform specific,
> > that we can't treat it as a quirk.
> 
> Those numbers have their own meaning only for Windows.  It's OS specific
> rather than platform specific.

Platform is a combination of hardware, PCB and uC firmwares. AFAIU that
platform was never designed for use in Linux, correct? So, it is not the same
as any other platform on the same SoC.

> Snapdragon platform manual has explicit
> numbering of every single GPIO pin.  Those broken numbers in ACPI table
> violate the hardware specification and are *broken* to Linux which
> implements GPIO driver properly.

No, they are not broken. They have a specific (Windows way) meaning. There is
no quirk implied.
Shawn Guo March 3, 2021, 9:43 a.m. UTC | #14 
On Tue, Mar 02, 2021 at 10:02:49PM -0700, Jeffrey Hugo wrote:
> Sorry, just joining the thread now.  Hopefully I'm addressing everything
> targeted at me.
> 
> I used to do kernel work on MSMs, then kernel work on server CPUs, but now I
> do kernel work on AI accelerators.  Never was on the firmware team, but I
> have a lot of contacts in those areas.  On my own time, I support Linux on
> the Qualcomm laptops.
> 
> Its not MS that needs to fix things (although there is plenty of things I
> could point to that MS could fix), its the Qualcomm Windows FW folks.  They
> have told me a while ago they were planning on fixing this issue on some
> future chipset, but apparently that hasn't happened yet.  Sadly, once these
> laptops ship, they are in a frozen maintenance mode.
> 
> In my opinion, MS has allowed Qualcomm to get away with doing bad things in
> ACPI on the Qualcomm laptops.  The ACPI is not a true hardware description
> that is OS agnostic as it should be, and probably violates the spec in many
> ways.  Instead, the ACPI is written against the Windows drivers, and has a
> lot of OS driver crap pushed into it.
> 
> The GPIO description is one such thing.
> 
> As I understand it, any particular SoC will have a number of GPIOs supported
> by the TLMM.  0 - N.  Linux understands this.  However, in the ACPI of the
> Qualcomm Windows laptops, you will likely find atleast one GPIO number which
> exceeds this N.  These are "virtual" GPIOs, and are a construct of the
> Windows Qualcomm TLMM driver and how it interfaces with the frameworks
> within Windows.
> 
> Some GPIO lines can be configured as wakeup sources by routing them to a
> specific hardware block in the SoC (which block it is varies from SoC to
> SoC).  Windows has a specific weird way of handling this which requires a
> unique "GPIO chip" to handle.  GPIO chips in Windows contain 32 GPIOs, so
> for each wakeup GPIO, the TLMM driver creates a GPIO chip (essentially
> creating 32 GPIOs), and assigns the added GPIOs numbers which exceed N.  The
> TLMM driver has an internal mapping of which virtual GPIO number corresponds
> to which real GPIO.
> 
> So, ACPI says that some peripheral has GPIO N+X, which is not a real GPIO.
> That peripheral goes and requests that GPIO, which gets routed to the TLMM
> driver, and the TLMM driver translates that number to the real GPIO, and
> provides the reference back to the peripheral, while also setting up the
> special wakeup hardware.
> 
> So, N+1 is the first supported wakup GPIO, N+1+32 is the next one, then
> N+1+32+32, and so on.

Jeffrey,

Thanks so much for these great information!

May I ask a bit more about how the virtual number N+1+32*n maps back to
the real number (R)?  For example of touchpad GPIO on Flex 5G, I think
we have:

  N+1+32*n = 0x0280
  N = 191
  R = 24

If my math not bad, n = 14.  How does 14 map to 24?

Shawn
Andy Shevchenko March 3, 2021, 9:47 a.m. UTC | #15 
On Fri, Feb 26, 2021 at 11:39:19AM +0800, Shawn Guo wrote:
> Running kernel with ACPI on Lenovo Flex 5G laptop, touchpad is just
> not working.  That's because the GpioInt number of TSC2 node in ACPI
> table is simply wrong, and the number even exceeds the maximum GPIO
> lines.  As the touchpad works fine with Windows on the same machine,
> presumably this is something Windows-ism.  Although it's obviously
> a specification violation, believe of that Microsoft will fix this in
> the near future is not really realistic.
> 
> It adds the support of overriding broken GPIO number in ACPI table
> on particular machines, which are matched using DMI info.  Such
> mechanism for fixing up broken firmware and ACPI table is not uncommon
> in kernel.  And hopefully it can be useful for other machines that get
> broken GPIO number coded in ACPI table.


+Cc: Hans.

Hans, would appreciate your opinion on this thread. Maybe I'm mistaken in my
conclusions.
Jeffrey Hugo March 3, 2021, 3:10 p.m. UTC | #16 
On 3/3/2021 2:43 AM, Shawn Guo wrote:
> On Tue, Mar 02, 2021 at 10:02:49PM -0700, Jeffrey Hugo wrote:
>> Sorry, just joining the thread now.  Hopefully I'm addressing everything
>> targeted at me.
>>
>> I used to do kernel work on MSMs, then kernel work on server CPUs, but now I
>> do kernel work on AI accelerators.  Never was on the firmware team, but I
>> have a lot of contacts in those areas.  On my own time, I support Linux on
>> the Qualcomm laptops.
>>
>> Its not MS that needs to fix things (although there is plenty of things I
>> could point to that MS could fix), its the Qualcomm Windows FW folks.  They
>> have told me a while ago they were planning on fixing this issue on some
>> future chipset, but apparently that hasn't happened yet.  Sadly, once these
>> laptops ship, they are in a frozen maintenance mode.
>>
>> In my opinion, MS has allowed Qualcomm to get away with doing bad things in
>> ACPI on the Qualcomm laptops.  The ACPI is not a true hardware description
>> that is OS agnostic as it should be, and probably violates the spec in many
>> ways.  Instead, the ACPI is written against the Windows drivers, and has a
>> lot of OS driver crap pushed into it.
>>
>> The GPIO description is one such thing.
>>
>> As I understand it, any particular SoC will have a number of GPIOs supported
>> by the TLMM.  0 - N.  Linux understands this.  However, in the ACPI of the
>> Qualcomm Windows laptops, you will likely find atleast one GPIO number which
>> exceeds this N.  These are "virtual" GPIOs, and are a construct of the
>> Windows Qualcomm TLMM driver and how it interfaces with the frameworks
>> within Windows.
>>
>> Some GPIO lines can be configured as wakeup sources by routing them to a
>> specific hardware block in the SoC (which block it is varies from SoC to
>> SoC).  Windows has a specific weird way of handling this which requires a
>> unique "GPIO chip" to handle.  GPIO chips in Windows contain 32 GPIOs, so
>> for each wakeup GPIO, the TLMM driver creates a GPIO chip (essentially
>> creating 32 GPIOs), and assigns the added GPIOs numbers which exceed N.  The
>> TLMM driver has an internal mapping of which virtual GPIO number corresponds
>> to which real GPIO.
>>
>> So, ACPI says that some peripheral has GPIO N+X, which is not a real GPIO.
>> That peripheral goes and requests that GPIO, which gets routed to the TLMM
>> driver, and the TLMM driver translates that number to the real GPIO, and
>> provides the reference back to the peripheral, while also setting up the
>> special wakeup hardware.
>>
>> So, N+1 is the first supported wakup GPIO, N+1+32 is the next one, then
>> N+1+32+32, and so on.
> 
> Jeffrey,
> 
> Thanks so much for these great information!
> 
> May I ask a bit more about how the virtual number N+1+32*n maps back to
> the real number (R)?  For example of touchpad GPIO on Flex 5G, I think
> we have:
> 
>    N+1+32*n = 0x0280
>    N = 191
>    R = 24
> 
> If my math not bad, n = 14.  How does 14 map to 24?


So, if this was 845, the wakeup hardware would be the PDC.  Only a 
specific number of GPIOs are routed to the PDC.  When the TLMM is 
powered off in suspend, the PDC pays attention to the GPIOs that are 
routed to it, and are configured in the PDC as wakeup sources.  When the 
GPIO is asserted, the signal to the TLMM gets lost, but the PDC catches 
it.  The PDC will kick the CPU/SoC out of suspend, and then once the 
wakup process is complete, replay the GPIO so that the TLMM has the signal.

In your example, 14 would be the 14th GPIO that is routed to the PDC. 
You would need SoC hardware documentation to know the mapping from PDC 
line 14 to GPIO line X.  This is going to be SoC specific, so 845 
documentation is not going to help you for SC8XXX.

Chances are, you are going to need to get this documentation from 
Qualcomm (I don't know if its in IPCatalog or not), and put SoC specific 
lookup tables in the TLMM driver.

Does that make sense, or did I not answer the question you were actually 
asking?
Bjorn Andersson March 3, 2021, 3:57 p.m. UTC | #17 
On Wed 03 Mar 09:10 CST 2021, Jeffrey Hugo wrote:

> On 3/3/2021 2:43 AM, Shawn Guo wrote:
> > On Tue, Mar 02, 2021 at 10:02:49PM -0700, Jeffrey Hugo wrote:
> > > Sorry, just joining the thread now.  Hopefully I'm addressing everything
> > > targeted at me.
> > > 
> > > I used to do kernel work on MSMs, then kernel work on server CPUs, but now I
> > > do kernel work on AI accelerators.  Never was on the firmware team, but I
> > > have a lot of contacts in those areas.  On my own time, I support Linux on
> > > the Qualcomm laptops.
> > > 
> > > Its not MS that needs to fix things (although there is plenty of things I
> > > could point to that MS could fix), its the Qualcomm Windows FW folks.  They
> > > have told me a while ago they were planning on fixing this issue on some
> > > future chipset, but apparently that hasn't happened yet.  Sadly, once these
> > > laptops ship, they are in a frozen maintenance mode.
> > > 
> > > In my opinion, MS has allowed Qualcomm to get away with doing bad things in
> > > ACPI on the Qualcomm laptops.  The ACPI is not a true hardware description
> > > that is OS agnostic as it should be, and probably violates the spec in many
> > > ways.  Instead, the ACPI is written against the Windows drivers, and has a
> > > lot of OS driver crap pushed into it.
> > > 
> > > The GPIO description is one such thing.
> > > 
> > > As I understand it, any particular SoC will have a number of GPIOs supported
> > > by the TLMM.  0 - N.  Linux understands this.  However, in the ACPI of the
> > > Qualcomm Windows laptops, you will likely find atleast one GPIO number which
> > > exceeds this N.  These are "virtual" GPIOs, and are a construct of the
> > > Windows Qualcomm TLMM driver and how it interfaces with the frameworks
> > > within Windows.
> > > 
> > > Some GPIO lines can be configured as wakeup sources by routing them to a
> > > specific hardware block in the SoC (which block it is varies from SoC to
> > > SoC).  Windows has a specific weird way of handling this which requires a
> > > unique "GPIO chip" to handle.  GPIO chips in Windows contain 32 GPIOs, so
> > > for each wakeup GPIO, the TLMM driver creates a GPIO chip (essentially
> > > creating 32 GPIOs), and assigns the added GPIOs numbers which exceed N.  The
> > > TLMM driver has an internal mapping of which virtual GPIO number corresponds
> > > to which real GPIO.
> > > 
> > > So, ACPI says that some peripheral has GPIO N+X, which is not a real GPIO.
> > > That peripheral goes and requests that GPIO, which gets routed to the TLMM
> > > driver, and the TLMM driver translates that number to the real GPIO, and
> > > provides the reference back to the peripheral, while also setting up the
> > > special wakeup hardware.
> > > 
> > > So, N+1 is the first supported wakup GPIO, N+1+32 is the next one, then
> > > N+1+32+32, and so on.
> > 
> > Jeffrey,
> > 
> > Thanks so much for these great information!
> > 
> > May I ask a bit more about how the virtual number N+1+32*n maps back to
> > the real number (R)?  For example of touchpad GPIO on Flex 5G, I think
> > we have:
> > 
> >    N+1+32*n = 0x0280
> >    N = 191

There's 190 GPIOs on SC8180x, but then the math doesn't add up to a
whole number...

> >    R = 24
> > 
> > If my math not bad, n = 14.  How does 14 map to 24?
> 
> 
> So, if this was 845, the wakeup hardware would be the PDC.  Only a specific
> number of GPIOs are routed to the PDC.  When the TLMM is powered off in
> suspend, the PDC pays attention to the GPIOs that are routed to it, and are
> configured in the PDC as wakeup sources.  When the GPIO is asserted, the
> signal to the TLMM gets lost, but the PDC catches it.  The PDC will kick the
> CPU/SoC out of suspend, and then once the wakup process is complete, replay
> the GPIO so that the TLMM has the signal.
> 

SC8180x has the same hardware design.

> In your example, 14 would be the 14th GPIO that is routed to the PDC. You
> would need SoC hardware documentation to know the mapping from PDC line 14
> to GPIO line X.  This is going to be SoC specific, so 845 documentation is
> not going to help you for SC8XXX.
> 
> Chances are, you are going to need to get this documentation from Qualcomm
> (I don't know if its in IPCatalog or not), and put SoC specific lookup
> tables in the TLMM driver.
> 

I added the table in the driver, see sc8180x_pdc_map[], and it has gpio
14 at position 7, with the 14th entry being gpio 38 - which seems like
an unlikely change from the reference schematics.

> Does that make sense, or did I not answer the question you were actually
> asking?
> 

It does.

Regards,
Bjorn
Jeffrey Hugo March 3, 2021, 5:08 p.m. UTC | #18 
On 3/3/2021 1:06 AM, Andy Shevchenko wrote:
> On Tue, Mar 02, 2021 at 10:02:49PM -0700, Jeffrey Hugo wrote:
>> On 3/2/2021 5:21 AM, Andy Shevchenko wrote:
>>> On Tue, Mar 02, 2021 at 08:27:26AM +0800, Shawn Guo wrote:
>>>> On Mon, Mar 01, 2021 at 02:17:06PM +0200, Andy Shevchenko wrote:
>>>>> On Sat, Feb 27, 2021 at 11:19:45AM +0800, Shawn Guo wrote:
>>>>>> On Fri, Feb 26, 2021 at 01:19:21PM +0200, Andy Shevchenko wrote:
>>>>>>> On Fri, Feb 26, 2021 at 12:57:37PM +0200, Andy Shevchenko wrote:
>>>>>>>> On Fri, Feb 26, 2021 at 11:39 AM Shawn Guo <shawn.guo@linaro.org> wrote:
>>>>>>>>> On Fri, Feb 26, 2021 at 11:12:07AM +0200, Andy Shevchenko wrote:
>>>>>>>>>> On Fri, Feb 26, 2021 at 5:42 AM Shawn Guo <shawn.guo@linaro.org> wrote:
>>>>>>>>>>> Running kernel with ACPI on Lenovo Flex 5G laptop, touchpad is just
>>>>>>>>>>> not working.  That's because the GpioInt number of TSC2 node in ACPI
>>>>>>>>>>> table is simply wrong, and the number even exceeds the maximum GPIO
>>>>>>>>>>> lines.  As the touchpad works fine with Windows on the same machine,
>>>>>>>>>>> presumably this is something Windows-ism.  Although it's obviously
>>>>>>>>>>> a specification violation, believe of that Microsoft will fix this in
>>>>>>>>>>> the near future is not really realistic.
>>>>>>>>>>>
>>>>>>>>>>> It adds the support of overriding broken GPIO number in ACPI table
>>>>>>>>>>> on particular machines, which are matched using DMI info.  Such
>>>>>>>>>>> mechanism for fixing up broken firmware and ACPI table is not uncommon
>>>>>>>>>>> in kernel.  And hopefully it can be useful for other machines that get
>>>>>>>>>>> broken GPIO number coded in ACPI table.
>>>>>>>>>>
>>>>>>>>>> Thanks for the report and patch.
>>>>>>>>>>
>>>>>>>>>> First of all, have you reported the issue to Lenovo? At least they
>>>>>>>>>> will know that they did wrong.
>>>>>>>>>
>>>>>>>>> Yes, we are reporting this to Lenovo, but to be honest, we are not sure
>>>>>>>>> how much they will care about it, as they are shipping the laptop with
>>>>>>>>> Windows only.
>>>>>>>>>
>>>>>>>>>> Second, is it possible to have somewhere output of `acpidump -o
>>>>>>>>>> flex5g.dat` (the flex5g.dat file)?
>>>>>>>>>
>>>>>>>>> https://raw.githubusercontent.com/aarch64-laptops/build/master/misc/lenovo-flex-5g/dsdt.dsl
>>>>>>>
>>>>>>> Looking into DSDT I think the problem is much worse. First of all there are
>>>>>>> many cases where pins like 0x140, 0x1c0, etc are being used. On top of that
>>>>>>> there is no GPIO driver in the upstream (as far as I can see by HID, perhaps
>>>>>>> there is a driver but for different HID. And I see that GPIO device consumes a
>>>>>>> lot of Interrupts from GIC as well (it's ARM platfrom as far as I understand).
>>>>>>
>>>>>> Yes, it's a laptop built on Qualcomm Snapdragon SC8180X SoC.  The GPIO
>>>>>> driver is generic for all Snapdragon SoCs, and has been available in
>>>>>> upstream for many years (for DT though). It can be found as the gpio_chip
>>>>>> implementation in MSM pinctrl driver [1].  The SC8180X specific part can
>>>>>> be found as pinctrl-sc8180x.c [2], and it's already working for DT boot.
>>>>>> The only missing piece is to add "QCOM040D" as the acpi_device_id to
>>>>>> support ACPI boot, and it will be submitted after 5.12-rc1 comes out.
>>>>>>
>>>>>>> Looking at the Microsoft brain damaged way of understanding GPIOs and hardware
>>>>>>> [1], I am afraid you really want to have a specific GPIO driver for this. So,
>>>>>>> for now until we have better picture of what's going on, NAK to this patch.
>>>>>>
>>>>>> Thanks for the pointer to Microsoft document.  On Snapdragon, we have
>>>>>> only one GPIO instance that accommodates all GPIO pins, so I'm not sure
>>>>>> that Microsoft GPIOs mapping layer is relevant here at all.
>>>>>>
>>>>>> Please take a look at the GPIO driver, and feel free to let me know if
>>>>>> you need any further information to understand what's going on.
>>>>>
>>>>> Yes, I looked into the driver and see that it has 3 blocks of GPIOs (we call
>>>>> them communities, but in the driver the term 'tiles' is used) AFAIU (correct me
>>>>> if I'm wrong). And who knows how many banks in each of them.
>>>>
>>>> I'm not sure that the 3 'tiles' means 3 blocks of GPIOs.  Maybe, @Bjorn
>>>> can help clarify.  But the ACPI table shows that there is only 'GIO0'
>>>> with 'QCOM040D' HID.
>>>
>>> Yeah, I already got that ACPI there is screwed up...
>>>
>>>>> I'm afraid that MS does on his way and not yours.
>>>>>
>>>>> Can we have TRM for GPIO IP used there and any evidence / document from
>>>>> firmware team about the implementation of the GPIO numbering in the ACPI
>>>>> (at Intel we have so called BIOS Writers Guide that is given to the customers
>>>>> where such info can be found)?
>>>>
>>>> Unfortunately, I do not have the access to any sort of these documents.
>>>> But I looped in Jeffrey who is part of Qualcomm kernel/firmware team,
>>>> and should be able to help clarify GPIO numbering in the ACPI table.
>>>
>>> Thanks! Will wait for new information then.
>>
>> Sorry, just joining the thread now.  Hopefully I'm addressing everything
>> targeted at me.
>>
>> I used to do kernel work on MSMs, then kernel work on server CPUs, but now I
>> do kernel work on AI accelerators.  Never was on the firmware team, but I
>> have a lot of contacts in those areas.  On my own time, I support Linux on
>> the Qualcomm laptops.
>>
>> Its not MS that needs to fix things (although there is plenty of things I
>> could point to that MS could fix), its the Qualcomm Windows FW folks.  They
>> have told me a while ago they were planning on fixing this issue on some
>> future chipset, but apparently that hasn't happened yet.  Sadly, once these
>> laptops ship, they are in a frozen maintenance mode.
> 
> I see. MS indeed loves Linux then :-)
> 
>> In my opinion, MS has allowed Qualcomm to get away with doing bad things in
>> ACPI on the Qualcomm laptops.  The ACPI is not a true hardware description
>> that is OS agnostic as it should be, and probably violates the spec in many
>> ways.  Instead, the ACPI is written against the Windows drivers, and has a
>> lot of OS driver crap pushed into it.
> 
> You meant "ACPI" -> "DSDT on the certain platform" I hope.

Sorry for the ambiguity.  Yes, I was referring to the ACPI tables 
written for the specific platform, of which the DSDT is relevant to this 
discussion.  I did not mean to imply that ACPI as a Spec or concept 
itself was Windows specific.  I used to be on the ASWG (ACPI Spec 
Working Group) and have personal knowledge that no OS or architecture is 
a "second class citizen" as far as the ACPI spec is concerned.

> 
>> The GPIO description is one such thing.
>>
>> As I understand it, any particular SoC will have a number of GPIOs supported
>> by the TLMM.  0 - N.  Linux understands this.  However, in the ACPI of the
>> Qualcomm Windows laptops, you will likely find atleast one GPIO number which
>> exceeds this N.  These are "virtual" GPIOs, and are a construct of the
>> Windows Qualcomm TLMM driver and how it interfaces with the frameworks
>> within Windows.
>>
>> Some GPIO lines can be configured as wakeup sources by routing them to a
>> specific hardware block in the SoC (which block it is varies from SoC to
>> SoC).  Windows has a specific weird way of handling this which requires a
>> unique "GPIO chip" to handle.  GPIO chips in Windows contain 32 GPIOs, so
>> for each wakeup GPIO, the TLMM driver creates a GPIO chip (essentially
>> creating 32 GPIOs), and assigns the added GPIOs numbers which exceed N.  The
>> TLMM driver has an internal mapping of which virtual GPIO number corresponds
>> to which real GPIO.
>>
>> So, ACPI says that some peripheral has GPIO N+X, which is not a real GPIO.
>> That peripheral goes and requests that GPIO, which gets routed to the TLMM
>> driver, and the TLMM driver translates that number to the real GPIO, and
>> provides the reference back to the peripheral, while also setting up the
>> special wakeup hardware.
>>
>> So, N+1 is the first supported wakup GPIO, N+1+32 is the next one, then
>> N+1+32+32, and so on.
>>
>> I see how this creates a nice mess for running Linux on these laptops, but I
>> don't have a good idea how to work around it.  Per SoC, you'd need to know
>> the mapping and translate it for ACPI when running the Windows version of
>> the FW (yes most Qualcomm MSMs have OS specific firmware), but reject such
>> gpio numbers when running other firmware, or I guess on different targets.
> 
> Thank, this makes a lot of sense to me and (unfortunately) I'm familiar with
> this concept on some of x86 cheap tablets.
> 
> Since the mapping of those wake IRQs is totally platform specific, it needs a
> platform driver. On above mentioned x86 platforms we have a one you may take as
> an example (good or bad it's another story):
> drivers/platform/x86/intel_int0002_vgpio.c.
> 
> I think you will need something like this somewhere in ARM platform
> infrastructure in the Linux kernel.
> 
> That said, I don't see that those numbers are "broken", they have their own
> meaning and specific mapping to the real GPIOs and it's so platform specific,
> that we can't treat it as a quirk.
> 
> Thanks, Jeffrey, it is helpful!
> 
>>>>>> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pinctrl/qcom/pinctrl-msm.c#n713
>>>>>> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/pinctrl/qcom/pinctrl-sc8180x.c
>
Andy Shevchenko March 3, 2021, 5:32 p.m. UTC | #19 
On Wed, Mar 03, 2021 at 09:57:58AM -0600, Bjorn Andersson wrote:
> On Wed 03 Mar 09:10 CST 2021, Jeffrey Hugo wrote:


...

> I added the table in the driver, see sc8180x_pdc_map[], and it has gpio

> 14 at position 7, with the 14th entry being gpio 38 - which seems like

> an unlikely change from the reference schematics.


It means for the certain platform this table should be altered (perhaps simply
another table based on IDs or so can be provided).

-- 
With Best Regards,
Andy Shevchenko
Shawn Guo March 4, 2021, 6:37 a.m. UTC | #20 
On Wed, Mar 03, 2021 at 09:57:58AM -0600, Bjorn Andersson wrote:
> On Wed 03 Mar 09:10 CST 2021, Jeffrey Hugo wrote:
> 
> > On 3/3/2021 2:43 AM, Shawn Guo wrote:
> > > On Tue, Mar 02, 2021 at 10:02:49PM -0700, Jeffrey Hugo wrote:
> > > > Sorry, just joining the thread now.  Hopefully I'm addressing everything
> > > > targeted at me.
> > > > 
> > > > I used to do kernel work on MSMs, then kernel work on server CPUs, but now I
> > > > do kernel work on AI accelerators.  Never was on the firmware team, but I
> > > > have a lot of contacts in those areas.  On my own time, I support Linux on
> > > > the Qualcomm laptops.
> > > > 
> > > > Its not MS that needs to fix things (although there is plenty of things I
> > > > could point to that MS could fix), its the Qualcomm Windows FW folks.  They
> > > > have told me a while ago they were planning on fixing this issue on some
> > > > future chipset, but apparently that hasn't happened yet.  Sadly, once these
> > > > laptops ship, they are in a frozen maintenance mode.
> > > > 
> > > > In my opinion, MS has allowed Qualcomm to get away with doing bad things in
> > > > ACPI on the Qualcomm laptops.  The ACPI is not a true hardware description
> > > > that is OS agnostic as it should be, and probably violates the spec in many
> > > > ways.  Instead, the ACPI is written against the Windows drivers, and has a
> > > > lot of OS driver crap pushed into it.
> > > > 
> > > > The GPIO description is one such thing.
> > > > 
> > > > As I understand it, any particular SoC will have a number of GPIOs supported
> > > > by the TLMM.  0 - N.  Linux understands this.  However, in the ACPI of the
> > > > Qualcomm Windows laptops, you will likely find atleast one GPIO number which
> > > > exceeds this N.  These are "virtual" GPIOs, and are a construct of the
> > > > Windows Qualcomm TLMM driver and how it interfaces with the frameworks
> > > > within Windows.
> > > > 
> > > > Some GPIO lines can be configured as wakeup sources by routing them to a
> > > > specific hardware block in the SoC (which block it is varies from SoC to
> > > > SoC).  Windows has a specific weird way of handling this which requires a
> > > > unique "GPIO chip" to handle.  GPIO chips in Windows contain 32 GPIOs, so
> > > > for each wakeup GPIO, the TLMM driver creates a GPIO chip (essentially
> > > > creating 32 GPIOs), and assigns the added GPIOs numbers which exceed N.  The
> > > > TLMM driver has an internal mapping of which virtual GPIO number corresponds
> > > > to which real GPIO.
> > > > 
> > > > So, ACPI says that some peripheral has GPIO N+X, which is not a real GPIO.
> > > > That peripheral goes and requests that GPIO, which gets routed to the TLMM
> > > > driver, and the TLMM driver translates that number to the real GPIO, and
> > > > provides the reference back to the peripheral, while also setting up the
> > > > special wakeup hardware.
> > > > 
> > > > So, N+1 is the first supported wakup GPIO, N+1+32 is the next one, then
> > > > N+1+32+32, and so on.
> > > 
> > > Jeffrey,
> > > 
> > > Thanks so much for these great information!
> > > 
> > > May I ask a bit more about how the virtual number N+1+32*n maps back to
> > > the real number (R)?  For example of touchpad GPIO on Flex 5G, I think
> > > we have:
> > > 
> > >    N+1+32*n = 0x0280
> > >    N = 191
> 
> There's 190 GPIOs on SC8180x, but then the math doesn't add up to a
> whole number...

In pinctrl-sc8180x driver you wrote, it has sc8180x_pinctrl.ngpios = 191.
Which one of you should I listen to :)

BTW, if you read this number from DTS, I already sent you a series to
fix them.

https://lore.kernel.org/linux-gpio/20210303033106.549-1-shawn.guo@linaro.org/

> 
> > >    R = 24
> > > 
> > > If my math not bad, n = 14.  How does 14 map to 24?
> > 
> > 
> > So, if this was 845, the wakeup hardware would be the PDC.  Only a specific
> > number of GPIOs are routed to the PDC.  When the TLMM is powered off in
> > suspend, the PDC pays attention to the GPIOs that are routed to it, and are
> > configured in the PDC as wakeup sources.  When the GPIO is asserted, the
> > signal to the TLMM gets lost, but the PDC catches it.  The PDC will kick the
> > CPU/SoC out of suspend, and then once the wakup process is complete, replay
> > the GPIO so that the TLMM has the signal.
> > 
> 
> SC8180x has the same hardware design.
> 
> > In your example, 14 would be the 14th GPIO that is routed to the PDC. You
> > would need SoC hardware documentation to know the mapping from PDC line 14
> > to GPIO line X.  This is going to be SoC specific, so 845 documentation is
> > not going to help you for SC8XXX.
> > 
> > Chances are, you are going to need to get this documentation from Qualcomm
> > (I don't know if its in IPCatalog or not), and put SoC specific lookup
> > tables in the TLMM driver.
> > 
> 
> I added the table in the driver, see sc8180x_pdc_map[], and it has gpio
> 14 at position 7, with the 14th entry being gpio 38 - which seems like
> an unlikely change from the reference schematics.

As it's clear that the real GPIO number is 24, and the only possible map
in sc8180x_pdc_map[] is:

	{ .gpio = 24, wakeirq = 37 }

So we need to understand how 14 turns to 37.

Shawn
Shawn Guo March 4, 2021, 6:59 a.m. UTC | #21 
On Thu, Mar 04, 2021 at 02:37:12PM +0800, Shawn Guo wrote:
> > > > May I ask a bit more about how the virtual number N+1+32*n maps back to
> > > > the real number (R)?  For example of touchpad GPIO on Flex 5G, I think
> > > > we have:
> > > > 
> > > >    N+1+32*n = 0x0280
> > > >    N = 191
...
> > > >    R = 24
> > > > 
> > > > If my math not bad, n = 14.  How does 14 map to 24?
...
> > > In your example, 14 would be the 14th GPIO that is routed to the PDC. You
> > > would need SoC hardware documentation to know the mapping from PDC line 14
> > > to GPIO line X.  This is going to be SoC specific, so 845 documentation is
> > > not going to help you for SC8XXX.
> > > 
> > > Chances are, you are going to need to get this documentation from Qualcomm
> > > (I don't know if its in IPCatalog or not), and put SoC specific lookup
> > > tables in the TLMM driver.
> > > 
> > 
> > I added the table in the driver, see sc8180x_pdc_map[], and it has gpio
> > 14 at position 7, with the 14th entry being gpio 38 - which seems like
> > an unlikely change from the reference schematics.
> 
> As it's clear that the real GPIO number is 24, and the only possible map
> in sc8180x_pdc_map[] is:
> 
> 	{ .gpio = 24, wakeirq = 37 }
> 
> So we need to understand how 14 turns to 37.

Oh, if I should look at the index in sc8180x_pdc_map[], { .gpio = 24, .wakeirq = 37 }
sits on 6 or 7 depending on indexing starts from 0 or 1.  Then question
becomes how 14 turns to 6 or 7.

Shawn
Hans de Goede March 4, 2021, 7:32 p.m. UTC | #22 
Hi,

On 3/3/21 10:47 AM, Andy Shevchenko wrote:
> On Fri, Feb 26, 2021 at 11:39:19AM +0800, Shawn Guo wrote:

>> Running kernel with ACPI on Lenovo Flex 5G laptop, touchpad is just

>> not working.  That's because the GpioInt number of TSC2 node in ACPI

>> table is simply wrong, and the number even exceeds the maximum GPIO

>> lines.  As the touchpad works fine with Windows on the same machine,

>> presumably this is something Windows-ism.  Although it's obviously

>> a specification violation, believe of that Microsoft will fix this in

>> the near future is not really realistic.

>>

>> It adds the support of overriding broken GPIO number in ACPI table

>> on particular machines, which are matched using DMI info.  Such

>> mechanism for fixing up broken firmware and ACPI table is not uncommon

>> in kernel.  And hopefully it can be useful for other machines that get

>> broken GPIO number coded in ACPI table.

> 

> 

> +Cc: Hans.

> 

> Hans, would appreciate your opinion on this thread. Maybe I'm mistaken in my

> conclusions.


So I've read the entire thread here:
https://lore.kernel.org/linux-gpio/20210226033919.8871-1-shawn.guo@linaro.org/T/#u

And I agree wih Andy, this is not something which should be fixed up in the
generic gpiolib-acpi code.

Note that we have similar things going on on x86 platforms. There are cases
there where there are e.g. holes in the GPIO ranges advertised by the Intel
pinctrl drivers. And in the beginning as i2c (and thus GpioIRQ) HID devices
started to become more common there were also several rounds of work to make
sure that the GPIO numbering (per ACPI-device / island) exported to the rest
of the kernel (and thus to gpiolib-acpi) matched with the numbering which
the ACPI tables expected (so the numbering which the Windows driver use).

It seems to me, esp. in the light that there are a lot of "crazy high" GPIO
indexes in the DSDT of the Lenovo Flex 5G, that the right thing to do here
is to fix the qualcom pinctrl/GPIO driver to number its GPIOs in the way
expected by these ACPI tables. This will break use of existing devicetrees,
so it will likely need to detect if the main firmware of the system is ACPI
or DT based and then use 2 different numbering schemes depending on the
outcome of that check.

Please also do not try ti fix this with some quirks in e.g. the i2c-hid driver,
I will definitely NACK such attempts. From what we can see now any fix clearly
should be done inside the qualcom GPIO driver.

Regards,

Hans
Andy Shevchenko March 4, 2021, 8:16 p.m. UTC | #23 
On Thu, Mar 04, 2021 at 08:32:14PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 3/3/21 10:47 AM, Andy Shevchenko wrote:
> > On Fri, Feb 26, 2021 at 11:39:19AM +0800, Shawn Guo wrote:
> >> Running kernel with ACPI on Lenovo Flex 5G laptop, touchpad is just
> >> not working.  That's because the GpioInt number of TSC2 node in ACPI
> >> table is simply wrong, and the number even exceeds the maximum GPIO
> >> lines.  As the touchpad works fine with Windows on the same machine,
> >> presumably this is something Windows-ism.  Although it's obviously
> >> a specification violation, believe of that Microsoft will fix this in
> >> the near future is not really realistic.
> >>
> >> It adds the support of overriding broken GPIO number in ACPI table
> >> on particular machines, which are matched using DMI info.  Such
> >> mechanism for fixing up broken firmware and ACPI table is not uncommon
> >> in kernel.  And hopefully it can be useful for other machines that get
> >> broken GPIO number coded in ACPI table.
> > 
> > 
> > +Cc: Hans.
> > 
> > Hans, would appreciate your opinion on this thread. Maybe I'm mistaken in my
> > conclusions.
> 
> So I've read the entire thread here:
> https://lore.kernel.org/linux-gpio/20210226033919.8871-1-shawn.guo@linaro.org/T/#u
> 
> And I agree wih Andy, this is not something which should be fixed up in the
> generic gpiolib-acpi code.
> 
> Note that we have similar things going on on x86 platforms. There are cases
> there where there are e.g. holes in the GPIO ranges advertised by the Intel
> pinctrl drivers. And in the beginning as i2c (and thus GpioIRQ) HID devices
> started to become more common there were also several rounds of work to make
> sure that the GPIO numbering (per ACPI-device / island) exported to the rest
> of the kernel (and thus to gpiolib-acpi) matched with the numbering which
> the ACPI tables expected (so the numbering which the Windows driver use).
> 
> It seems to me, esp. in the light that there are a lot of "crazy high" GPIO
> indexes in the DSDT of the Lenovo Flex 5G, that the right thing to do here
> is to fix the qualcom pinctrl/GPIO driver to number its GPIOs in the way
> expected by these ACPI tables. This will break use of existing devicetrees,
> so it will likely need to detect if the main firmware of the system is ACPI
> or DT based and then use 2 different numbering schemes depending on the
> outcome of that check.
> 
> Please also do not try ti fix this with some quirks in e.g. the i2c-hid driver,
> I will definitely NACK such attempts. From what we can see now any fix clearly
> should be done inside the qualcom GPIO driver.

Hans, thank you very much!
Shawn Guo March 5, 2021, 1:14 a.m. UTC | #24 
On Thu, Mar 04, 2021 at 08:32:14PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 3/3/21 10:47 AM, Andy Shevchenko wrote:
> > On Fri, Feb 26, 2021 at 11:39:19AM +0800, Shawn Guo wrote:
> >> Running kernel with ACPI on Lenovo Flex 5G laptop, touchpad is just
> >> not working.  That's because the GpioInt number of TSC2 node in ACPI
> >> table is simply wrong, and the number even exceeds the maximum GPIO
> >> lines.  As the touchpad works fine with Windows on the same machine,
> >> presumably this is something Windows-ism.  Although it's obviously
> >> a specification violation, believe of that Microsoft will fix this in
> >> the near future is not really realistic.
> >>
> >> It adds the support of overriding broken GPIO number in ACPI table
> >> on particular machines, which are matched using DMI info.  Such
> >> mechanism for fixing up broken firmware and ACPI table is not uncommon
> >> in kernel.  And hopefully it can be useful for other machines that get
> >> broken GPIO number coded in ACPI table.
> > 
> > 
> > +Cc: Hans.
> > 
> > Hans, would appreciate your opinion on this thread. Maybe I'm mistaken in my
> > conclusions.
> 
> So I've read the entire thread here:
> https://lore.kernel.org/linux-gpio/20210226033919.8871-1-shawn.guo@linaro.org/T/#u
> 
> And I agree wih Andy, this is not something which should be fixed up in the
> generic gpiolib-acpi code.
> 
> Note that we have similar things going on on x86 platforms. There are cases
> there where there are e.g. holes in the GPIO ranges advertised by the Intel
> pinctrl drivers. And in the beginning as i2c (and thus GpioIRQ) HID devices
> started to become more common there were also several rounds of work to make
> sure that the GPIO numbering (per ACPI-device / island) exported to the rest
> of the kernel (and thus to gpiolib-acpi) matched with the numbering which
> the ACPI tables expected (so the numbering which the Windows driver use).
> 
> It seems to me, esp. in the light that there are a lot of "crazy high" GPIO
> indexes in the DSDT of the Lenovo Flex 5G, that the right thing to do here
> is to fix the qualcom pinctrl/GPIO driver to number its GPIOs in the way
> expected by these ACPI tables. This will break use of existing devicetrees,
> so it will likely need to detect if the main firmware of the system is ACPI
> or DT based and then use 2 different numbering schemes depending on the
> outcome of that check.
> 
> Please also do not try ti fix this with some quirks in e.g. the i2c-hid driver,
> I will definitely NACK such attempts. From what we can see now any fix clearly
> should be done inside the qualcom GPIO driver.

Thanks for your opinion on this, Hans.  Yeah, with the information from
Jeffrey, I now agree with Andy that these high GPIO numbers are not
broken but have some meaning, and we should map them back to real GPIO
number in Qualcomm GPIO driver.

So we reach a consensus that this is not the right solution for Lenovo
Flex 5G. But what about for Andy's Galileo Gen 2 case, where the GPIO
number in ACPI is truly broken?

  ba8c90c61847 ("gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2")

Shawn
Hans de Goede March 5, 2021, 9:10 a.m. UTC | #25 
Hi,

On 3/5/21 2:14 AM, Shawn Guo wrote:
> On Thu, Mar 04, 2021 at 08:32:14PM +0100, Hans de Goede wrote:
>> Hi,
>>
>> On 3/3/21 10:47 AM, Andy Shevchenko wrote:
>>> On Fri, Feb 26, 2021 at 11:39:19AM +0800, Shawn Guo wrote:
>>>> Running kernel with ACPI on Lenovo Flex 5G laptop, touchpad is just
>>>> not working.  That's because the GpioInt number of TSC2 node in ACPI
>>>> table is simply wrong, and the number even exceeds the maximum GPIO
>>>> lines.  As the touchpad works fine with Windows on the same machine,
>>>> presumably this is something Windows-ism.  Although it's obviously
>>>> a specification violation, believe of that Microsoft will fix this in
>>>> the near future is not really realistic.
>>>>
>>>> It adds the support of overriding broken GPIO number in ACPI table
>>>> on particular machines, which are matched using DMI info.  Such
>>>> mechanism for fixing up broken firmware and ACPI table is not uncommon
>>>> in kernel.  And hopefully it can be useful for other machines that get
>>>> broken GPIO number coded in ACPI table.
>>>
>>>
>>> +Cc: Hans.
>>>
>>> Hans, would appreciate your opinion on this thread. Maybe I'm mistaken in my
>>> conclusions.
>>
>> So I've read the entire thread here:
>> https://lore.kernel.org/linux-gpio/20210226033919.8871-1-shawn.guo@linaro.org/T/#u
>>
>> And I agree wih Andy, this is not something which should be fixed up in the
>> generic gpiolib-acpi code.
>>
>> Note that we have similar things going on on x86 platforms. There are cases
>> there where there are e.g. holes in the GPIO ranges advertised by the Intel
>> pinctrl drivers. And in the beginning as i2c (and thus GpioIRQ) HID devices
>> started to become more common there were also several rounds of work to make
>> sure that the GPIO numbering (per ACPI-device / island) exported to the rest
>> of the kernel (and thus to gpiolib-acpi) matched with the numbering which
>> the ACPI tables expected (so the numbering which the Windows driver use).
>>
>> It seems to me, esp. in the light that there are a lot of "crazy high" GPIO
>> indexes in the DSDT of the Lenovo Flex 5G, that the right thing to do here
>> is to fix the qualcom pinctrl/GPIO driver to number its GPIOs in the way
>> expected by these ACPI tables. This will break use of existing devicetrees,
>> so it will likely need to detect if the main firmware of the system is ACPI
>> or DT based and then use 2 different numbering schemes depending on the
>> outcome of that check.
>>
>> Please also do not try ti fix this with some quirks in e.g. the i2c-hid driver,
>> I will definitely NACK such attempts. From what we can see now any fix clearly
>> should be done inside the qualcom GPIO driver.
> 
> Thanks for your opinion on this, Hans.  Yeah, with the information from
> Jeffrey, I now agree with Andy that these high GPIO numbers are not
> broken but have some meaning, and we should map them back to real GPIO
> number in Qualcomm GPIO driver.
> 
> So we reach a consensus that this is not the right solution for Lenovo
> Flex 5G. But what about for Andy's Galileo Gen 2 case, where the GPIO
> number in ACPI is truly broken?

Well if the ACPI table truely simply has a wrong number in it, like in
this case, then we clearly need a workaround.

>   ba8c90c61847 ("gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2")

And we have one in place, so I'm not sure what the question is?

I guess the question is of your generic GPIO renumber patch would not
be a better answer to that ?

IMHO no, we want to keep quirks out of the core as much as possible,
for example the code which Andy added a quirk to is build as a module
in the generic Fedora distro kernel, so for most users the code will
not be loaded into memory. Where as if we add it to the core it would
use up extra memory for everyone.

Also if, in the future, we were to ever add a generic GPIO renumber quirk
mechanism to the core, then your code would need more work. Because to be
truely generic it would need to remap one gpiochip-name:pin-number on
another gpiochip-name:pin-number pair. There might very well be a case
with multiple gpiochips with pin number 32 being referenced in the DSDT
and where we need to remap one of those 32-s to a different number
(or possibly even to a different chip + number).

Regards,

Hans
Andy Shevchenko March 5, 2021, 10:08 a.m. UTC | #26 
On Fri, Mar 05, 2021 at 10:10:50AM +0100, Hans de Goede wrote:
> On 3/5/21 2:14 AM, Shawn Guo wrote:

> > On Thu, Mar 04, 2021 at 08:32:14PM +0100, Hans de Goede wrote:

> >> On 3/3/21 10:47 AM, Andy Shevchenko wrote:


...

> > So we reach a consensus that this is not the right solution for Lenovo

> > Flex 5G. But what about for Andy's Galileo Gen 2 case, where the GPIO

> > number in ACPI is truly broken?

> 

> Well if the ACPI table truely simply has a wrong number in it, like in

> this case, then we clearly need a workaround.

> 

> >   ba8c90c61847 ("gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2")

> 

> And we have one in place, so I'm not sure what the question is?

> 

> I guess the question is of your generic GPIO renumber patch would not

> be a better answer to that ?

> 

> IMHO no, we want to keep quirks out of the core as much as possible,

> for example the code which Andy added a quirk to is build as a module

> in the generic Fedora distro kernel, so for most users the code will

> not be loaded into memory. Where as if we add it to the core it would

> use up extra memory for everyone.


I guess Shawn is referring to my rework of that quirk [1] due to found a flaw
in the upstreamed variant. I agree, that this is not ideal, but TL;DR: it less
invasive even to the upstreamed approach and it has no use of any hard coded
numbering schemes. The Galileo Gen 2 is "broken" in an *understandable* way,
i.e. ACPI designers put an absolute GPIO numbers (there are two SoC based GPIO
controllers: SCH and DesignWare which numbers starts at 0) instead of be
relative. For the time being only one device has a driver that needs such GPIO
number, but as I explained in the cover letter, to support it as a quirk I have
to copy ~10% of the existing (in gpiolib-acpi.c) code.

I'm all ears for better approach!

[1]: https://lore.kernel.org/linux-gpio/20210225163320.71267-1-andriy.shevchenko@linux.intel.com/T/#u

> Also if, in the future, we were to ever add a generic GPIO renumber quirk

> mechanism to the core, then your code would need more work. Because to be

> truely generic it would need to remap one gpiochip-name:pin-number on

> another gpiochip-name:pin-number pair. There might very well be a case

> with multiple gpiochips with pin number 32 being referenced in the DSDT

> and where we need to remap one of those 32-s to a different number

> (or possibly even to a different chip + number).


-- 
With Best Regards,
Andy Shevchenko
Andy Shevchenko March 5, 2021, 10:10 a.m. UTC | #27 
On Fri, Mar 05, 2021 at 12:08:52PM +0200, Andy Shevchenko wrote:
> On Fri, Mar 05, 2021 at 10:10:50AM +0100, Hans de Goede wrote:
> > On 3/5/21 2:14 AM, Shawn Guo wrote:
> > > On Thu, Mar 04, 2021 at 08:32:14PM +0100, Hans de Goede wrote:
> > >> On 3/3/21 10:47 AM, Andy Shevchenko wrote:
> 
> ...
> 
> > > So we reach a consensus that this is not the right solution for Lenovo
> > > Flex 5G. But what about for Andy's Galileo Gen 2 case, where the GPIO
> > > number in ACPI is truly broken?
> > 
> > Well if the ACPI table truely simply has a wrong number in it, like in
> > this case, then we clearly need a workaround.
> > 
> > >   ba8c90c61847 ("gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2")
> > 
> > And we have one in place, so I'm not sure what the question is?
> > 
> > I guess the question is of your generic GPIO renumber patch would not
> > be a better answer to that ?
> > 
> > IMHO no, we want to keep quirks out of the core as much as possible,
> > for example the code which Andy added a quirk to is build as a module
> > in the generic Fedora distro kernel, so for most users the code will
> > not be loaded into memory. Where as if we add it to the core it would
> > use up extra memory for everyone.
> 
> I guess Shawn is referring to my rework of that quirk [1] due to found a flaw
> in the upstreamed variant. I agree, that this is not ideal, but TL;DR: it less
> invasive even to the upstreamed approach and it has no use of any hard coded
> numbering schemes. The Galileo Gen 2 is "broken" in an *understandable* way,
> i.e. ACPI designers put an absolute GPIO numbers (there are two SoC based GPIO
> controllers: SCH and DesignWare which numbers starts at 0) instead of be

"...at 0 and 8 respectively)"

> relative. For the time being only one device has a driver that needs such GPIO
> number, but as I explained in the cover letter, to support it as a quirk I have
> to copy ~10% of the existing (in gpiolib-acpi.c) code.
> 
> I'm all ears for better approach!
> 
> [1]: https://lore.kernel.org/linux-gpio/20210225163320.71267-1-andriy.shevchenko@linux.intel.com/T/#u
> 
> > Also if, in the future, we were to ever add a generic GPIO renumber quirk
> > mechanism to the core, then your code would need more work. Because to be
> > truely generic it would need to remap one gpiochip-name:pin-number on
> > another gpiochip-name:pin-number pair. There might very well be a case
> > with multiple gpiochips with pin number 32 being referenced in the DSDT
> > and where we need to remap one of those 32-s to a different number
> > (or possibly even to a different chip + number).
> 
> -- 
> With Best Regards,
> Andy Shevchenko
> 
>
Shawn Guo March 5, 2021, 11:26 a.m. UTC | #28 
On Fri, Mar 05, 2021 at 10:10:50AM +0100, Hans de Goede wrote:
> > So we reach a consensus that this is not the right solution for Lenovo
> > Flex 5G. But what about for Andy's Galileo Gen 2 case, where the GPIO
> > number in ACPI is truly broken?
> 
> Well if the ACPI table truely simply has a wrong number in it, like in
> this case, then we clearly need a workaround.
> 
> >   ba8c90c61847 ("gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2")
> 
> And we have one in place, so I'm not sure what the question is?
> 
> I guess the question is of your generic GPIO renumber patch would not
> be a better answer to that ?
> 
> IMHO no, we want to keep quirks out of the core as much as possible,
> for example the code which Andy added a quirk to is build as a module
> in the generic Fedora distro kernel, so for most users the code will
> not be loaded into memory. Where as if we add it to the core it would
> use up extra memory for everyone.

Fair point. I did not really think of it, because there is already
gpiolib_acpi_quirks[] in core code.  And on the other side, if there are
more drivers need such workaround, having each of these drivers copy the
same code is not ideal either.

> 
> Also if, in the future, we were to ever add a generic GPIO renumber quirk
> mechanism to the core, then your code would need more work. Because to be
> truely generic it would need to remap one gpiochip-name:pin-number on
> another gpiochip-name:pin-number pair. There might very well be a case
> with multiple gpiochips with pin number 32 being referenced in the DSDT
> and where we need to remap one of those 32-s to a different number
> (or possibly even to a different chip + number).

Yeah, I had already have v2 of my patch, just did not post it as the
overall direction is not agreed on.  I attach it here for discussion.
I think with the GPIO consumer specified, it should be good enough to
locate the broken GPIO number that needs override.  If gpiochip is
wrong, that means "\\_SB.GIO0" of GpioInt needs an override.  That's
a different issue.

GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
    "\\_SB.GIO0", 0x00, ResourceConsumer, ,
    )
    {   // Pin list
	0x0280
    }

Shawn


[PATCH v2] gpiolib: acpi: support override broken GPIO number in ACPI

Running kernel with ACPI on Lenovo Flex 5G laptop, touchpad is just
not working.  That's because the GpioInt number of TCPD node in ACPI
table is simply wrong, and the number even exceeds the maximum GPIO
lines.  As the touchpad works fine with Windows on the same machine,
presumably this is something Windows-ism.  Although it's obviously
a specification violation, believe of that Microsoft will fix this in
the near future is not really realistic.

It adds the support of overriding broken GPIO number in ACPI table
on particular machines, which are matched using DMI info.  Such
mechanism for fixing up broken firmware and ACPI table is not uncommon
in kernel.  And hopefully it can be useful for other machines that get
broken GPIO number coded in ACPI table.

The signature of acpi_get_gpiod() gets updated to pass over acpi_device
pointer of consumer device, so that the broken pin can be matched
precisely with consumer fwnode name.

Changes for v2:
- Match broken pin with additional consumer fwnode name comparison.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 drivers/gpio/gpiolib-acpi.c | 79 +++++++++++++++++++++++++++++++++++--
 1 file changed, 76 insertions(+), 3 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index e37a57d0a2f0..fed045d64a26 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -93,6 +93,72 @@ static DEFINE_MUTEX(acpi_gpio_deferred_req_irqs_lock);
 static LIST_HEAD(acpi_gpio_deferred_req_irqs_list);
 static bool acpi_gpio_deferred_req_irqs_done;
 
+struct acpi_gpio_pin_fixup {
+	const char *consumer;
+	int pin_broken;
+	int pin_correct;
+};
+
+struct acpi_gpio_pin_override {
+	const struct acpi_gpio_pin_fixup *fixups;
+	int num;
+};
+
+static const struct acpi_gpio_pin_fixup lenovo_flex_5g_fixups[] = {
+	{
+		/* GpioInt of Touchpad */
+		.consumer = "\\_SB.I2C8.TCPD",
+		.pin_broken = 0x0280,
+		.pin_correct = 0x0018,
+	},
+};
+
+static const struct acpi_gpio_pin_override lenovo_flex_5g_override = {
+	.fixups = lenovo_flex_5g_fixups,
+	.num = ARRAY_SIZE(lenovo_flex_5g_fixups),
+};
+
+static const struct dmi_system_id acpi_gpio_pin_override_table[] = {
+	{
+		.ident = "Lenovo Flex 5G",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_FAMILY, "Flex 5G 14Q8CX05"),
+		},
+		.driver_data = (void *)&lenovo_flex_5g_override,
+	},
+	{ } /* terminator */
+};
+
+static int acpi_gpio_pin_override(struct acpi_device *adev, int pin)
+{
+	const struct acpi_gpio_pin_override *override;
+	const struct dmi_system_id *system_id;
+	char *fwname;
+	int ret = pin;
+	int i;
+
+	system_id = dmi_first_match(acpi_gpio_pin_override_table);
+	if (!system_id)
+		return ret;
+
+	fwname = kasprintf(GFP_KERNEL, "%pfwf", &adev->fwnode);
+	override = system_id->driver_data;
+
+	for (i = 0; i < override->num; i++) {
+		const struct acpi_gpio_pin_fixup *f = &override->fixups[i];
+
+		if (!strcmp(f->consumer, fwname) && pin == f->pin_broken) {
+			ret = f->pin_correct;
+			goto done;
+		}
+	}
+
+done:
+	kfree(fwname);
+	return ret;
+}
+
 static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
 {
 	if (!gc->parent)
@@ -103,6 +169,7 @@ static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
 
 /**
  * acpi_get_gpiod() - Translate ACPI GPIO pin to GPIO descriptor usable with GPIO API
+ * @adev:	ACPI device that consumes the GPIO
  * @path:	ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1")
  * @pin:	ACPI GPIO pin number (0-based, controller-relative)
  *
@@ -111,7 +178,8 @@ static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
  * controller does not have GPIO chip registered at the moment. This is to
  * support probe deferral.
  */
-static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
+static struct gpio_desc *acpi_get_gpiod(struct acpi_device *adev,
+					char *path, int pin)
 {
 	struct gpio_chip *chip;
 	acpi_handle handle;
@@ -125,7 +193,11 @@ static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
 	if (!chip)
 		return ERR_PTR(-EPROBE_DEFER);
 
-	return gpiochip_get_desc(chip, pin);
+	/*
+	 * Give it a chance to correct the broken GPIO pin number in ACPI
+	 * table of particular machines.
+	 */
+	return gpiochip_get_desc(chip, acpi_gpio_pin_override(adev, pin));
 }
 
 static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
@@ -689,7 +761,8 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
 		if (pin_index >= agpio->pin_table_length)
 			return 1;
 
-		lookup->desc = acpi_get_gpiod(agpio->resource_source.string_ptr,
+		lookup->desc = acpi_get_gpiod(lookup->info.adev,
+					      agpio->resource_source.string_ptr,
 					      agpio->pin_table[pin_index]);
 		lookup->info.pin_config = agpio->pin_config;
 		lookup->info.debounce = agpio->debounce_timeout;
Hans de Goede March 5, 2021, 12:12 p.m. UTC | #29 
Hi,

On 3/5/21 12:26 PM, Shawn Guo wrote:
> On Fri, Mar 05, 2021 at 10:10:50AM +0100, Hans de Goede wrote:
>>> So we reach a consensus that this is not the right solution for Lenovo
>>> Flex 5G. But what about for Andy's Galileo Gen 2 case, where the GPIO
>>> number in ACPI is truly broken?
>>
>> Well if the ACPI table truely simply has a wrong number in it, like in
>> this case, then we clearly need a workaround.
>>
>>>   ba8c90c61847 ("gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2")
>>
>> And we have one in place, so I'm not sure what the question is?
>>
>> I guess the question is of your generic GPIO renumber patch would not
>> be a better answer to that ?
>>
>> IMHO no, we want to keep quirks out of the core as much as possible,
>> for example the code which Andy added a quirk to is build as a module
>> in the generic Fedora distro kernel, so for most users the code will
>> not be loaded into memory. Where as if we add it to the core it would
>> use up extra memory for everyone.
> 
> Fair point. I did not really think of it, because there is already
> gpiolib_acpi_quirks[] in core code.  And on the other side, if there are
> more drivers need such workaround, having each of these drivers copy the
> same code is not ideal either.
> 
>>
>> Also if, in the future, we were to ever add a generic GPIO renumber quirk
>> mechanism to the core, then your code would need more work. Because to be
>> truely generic it would need to remap one gpiochip-name:pin-number on
>> another gpiochip-name:pin-number pair. There might very well be a case
>> with multiple gpiochips with pin number 32 being referenced in the DSDT
>> and where we need to remap one of those 32-s to a different number
>> (or possibly even to a different chip + number).
> 
> Yeah, I had already have v2 of my patch, just did not post it as the
> overall direction is not agreed on.  I attach it here for discussion.
> I think with the GPIO consumer specified, it should be good enough to
> locate the broken GPIO number that needs override.  If gpiochip is
> wrong, that means "\\_SB.GIO0" of GpioInt needs an override.  That's
> a different issue.

Not really it is still pointing to a wrong pin, the full identifier
for a pin in linux (see the userspace iocontrol interface also) is
<gpio-chip-name> + <index> so if we add support for remapping we should
add support for specifying the full pair. But your new approach with
the consumer is also interesting.

Maybe a generic override mechanism should use the following
inputs (indexing into the override map) and outputs:

In: consumer-dev-name + consumer-gpio-pin-name (e.g. "power", etc.)
Out: gpio-chip-name> + index + flags

That would also work in cases where GPIOs are just completely missing
from the DSDTs.

And to circle back to Andy's point about the current fix for his
case requiring duplicating lots of gpiolib-acpi code.

Maybe we need a gpiod_get_with_lookup_table() or some such
where the driver doing the gpiod_get call can specify a lookup
table to do the overriding of the broken DSDT.

I think we should even be able to make this non ACPI specific
this way.

This will allow drivers to avoid having to code all
the lookup code themselves, while still pushing the
responsibility of actually maintaining the lookup-overrides
inside the individual drivers.

Regards,

Hans





> 
> GpioInt (Level, ActiveLow, ExclusiveAndWake, PullUp, 0x0000,
>     "\\_SB.GIO0", 0x00, ResourceConsumer, ,
>     )
>     {   // Pin list
> 	0x0280
>     }
> 
> Shawn
> 
> 
> [PATCH v2] gpiolib: acpi: support override broken GPIO number in ACPI
> 
> Running kernel with ACPI on Lenovo Flex 5G laptop, touchpad is just
> not working.  That's because the GpioInt number of TCPD node in ACPI
> table is simply wrong, and the number even exceeds the maximum GPIO
> lines.  As the touchpad works fine with Windows on the same machine,
> presumably this is something Windows-ism.  Although it's obviously
> a specification violation, believe of that Microsoft will fix this in
> the near future is not really realistic.
> 
> It adds the support of overriding broken GPIO number in ACPI table
> on particular machines, which are matched using DMI info.  Such
> mechanism for fixing up broken firmware and ACPI table is not uncommon
> in kernel.  And hopefully it can be useful for other machines that get
> broken GPIO number coded in ACPI table.
> 
> The signature of acpi_get_gpiod() gets updated to pass over acpi_device
> pointer of consumer device, so that the broken pin can be matched
> precisely with consumer fwnode name.
> 
> Changes for v2:
> - Match broken pin with additional consumer fwnode name comparison.
> 
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  drivers/gpio/gpiolib-acpi.c | 79 +++++++++++++++++++++++++++++++++++--
>  1 file changed, 76 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index e37a57d0a2f0..fed045d64a26 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -93,6 +93,72 @@ static DEFINE_MUTEX(acpi_gpio_deferred_req_irqs_lock);
>  static LIST_HEAD(acpi_gpio_deferred_req_irqs_list);
>  static bool acpi_gpio_deferred_req_irqs_done;
>  
> +struct acpi_gpio_pin_fixup {
> +	const char *consumer;
> +	int pin_broken;
> +	int pin_correct;
> +};
> +
> +struct acpi_gpio_pin_override {
> +	const struct acpi_gpio_pin_fixup *fixups;
> +	int num;
> +};
> +
> +static const struct acpi_gpio_pin_fixup lenovo_flex_5g_fixups[] = {
> +	{
> +		/* GpioInt of Touchpad */
> +		.consumer = "\\_SB.I2C8.TCPD",
> +		.pin_broken = 0x0280,
> +		.pin_correct = 0x0018,
> +	},
> +};
> +
> +static const struct acpi_gpio_pin_override lenovo_flex_5g_override = {
> +	.fixups = lenovo_flex_5g_fixups,
> +	.num = ARRAY_SIZE(lenovo_flex_5g_fixups),
> +};
> +
> +static const struct dmi_system_id acpi_gpio_pin_override_table[] = {
> +	{
> +		.ident = "Lenovo Flex 5G",
> +		.matches = {
> +			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "LENOVO"),
> +			DMI_EXACT_MATCH(DMI_PRODUCT_FAMILY, "Flex 5G 14Q8CX05"),
> +		},
> +		.driver_data = (void *)&lenovo_flex_5g_override,
> +	},
> +	{ } /* terminator */
> +};
> +
> +static int acpi_gpio_pin_override(struct acpi_device *adev, int pin)
> +{
> +	const struct acpi_gpio_pin_override *override;
> +	const struct dmi_system_id *system_id;
> +	char *fwname;
> +	int ret = pin;
> +	int i;
> +
> +	system_id = dmi_first_match(acpi_gpio_pin_override_table);
> +	if (!system_id)
> +		return ret;
> +
> +	fwname = kasprintf(GFP_KERNEL, "%pfwf", &adev->fwnode);
> +	override = system_id->driver_data;
> +
> +	for (i = 0; i < override->num; i++) {
> +		const struct acpi_gpio_pin_fixup *f = &override->fixups[i];
> +
> +		if (!strcmp(f->consumer, fwname) && pin == f->pin_broken) {
> +			ret = f->pin_correct;
> +			goto done;
> +		}
> +	}
> +
> +done:
> +	kfree(fwname);
> +	return ret;
> +}
> +
>  static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
>  {
>  	if (!gc->parent)
> @@ -103,6 +169,7 @@ static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
>  
>  /**
>   * acpi_get_gpiod() - Translate ACPI GPIO pin to GPIO descriptor usable with GPIO API
> + * @adev:	ACPI device that consumes the GPIO
>   * @path:	ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1")
>   * @pin:	ACPI GPIO pin number (0-based, controller-relative)
>   *
> @@ -111,7 +178,8 @@ static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
>   * controller does not have GPIO chip registered at the moment. This is to
>   * support probe deferral.
>   */
> -static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
> +static struct gpio_desc *acpi_get_gpiod(struct acpi_device *adev,
> +					char *path, int pin)
>  {
>  	struct gpio_chip *chip;
>  	acpi_handle handle;
> @@ -125,7 +193,11 @@ static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
>  	if (!chip)
>  		return ERR_PTR(-EPROBE_DEFER);
>  
> -	return gpiochip_get_desc(chip, pin);
> +	/*
> +	 * Give it a chance to correct the broken GPIO pin number in ACPI
> +	 * table of particular machines.
> +	 */
> +	return gpiochip_get_desc(chip, acpi_gpio_pin_override(adev, pin));
>  }
>  
>  static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
> @@ -689,7 +761,8 @@ static int acpi_populate_gpio_lookup(struct acpi_resource *ares, void *data)
>  		if (pin_index >= agpio->pin_table_length)
>  			return 1;
>  
> -		lookup->desc = acpi_get_gpiod(agpio->resource_source.string_ptr,
> +		lookup->desc = acpi_get_gpiod(lookup->info.adev,
> +					      agpio->resource_source.string_ptr,
>  					      agpio->pin_table[pin_index]);
>  		lookup->info.pin_config = agpio->pin_config;
>  		lookup->info.debounce = agpio->debounce_timeout;
>
diff mbox series

Patch

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index e37a57d0a2f0..30a5c5a954fa 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -93,6 +93,63 @@  static DEFINE_MUTEX(acpi_gpio_deferred_req_irqs_lock);
 static LIST_HEAD(acpi_gpio_deferred_req_irqs_list);
 static bool acpi_gpio_deferred_req_irqs_done;
 
+struct acpi_gpio_pin_fixup {
+	int pin_broken;
+	int pin_correct;
+};
+
+struct acpi_gpio_pin_override {
+	const struct acpi_gpio_pin_fixup *fixups;
+	int num;
+};
+
+static const struct acpi_gpio_pin_fixup lenovo_flex_5g_fixups[] = {
+	{
+		/* GpioInt of Touchpad (TSC2) */
+		.pin_broken = 0x0280,
+		.pin_correct = 0x0018,
+	},
+};
+
+static const struct acpi_gpio_pin_override lenovo_flex_5g_override = {
+	.fixups = lenovo_flex_5g_fixups,
+	.num = ARRAY_SIZE(lenovo_flex_5g_fixups),
+};
+
+static const struct dmi_system_id acpi_gpio_pin_override_table[] = {
+	{
+		.ident = "Lenovo Flex 5G",
+		.matches = {
+			DMI_EXACT_MATCH(DMI_SYS_VENDOR, "LENOVO"),
+			DMI_EXACT_MATCH(DMI_PRODUCT_FAMILY, "Flex 5G 14Q8CX05"),
+		},
+		.driver_data = (void *)&lenovo_flex_5g_override,
+	},
+	{ } /* terminator */
+};
+
+static int acpi_gpio_pin_override(int pin)
+{
+	const struct acpi_gpio_pin_override *override;
+	const struct acpi_gpio_pin_fixup *fixup;
+	const struct dmi_system_id *system_id;
+	int i;
+
+	system_id = dmi_first_match(acpi_gpio_pin_override_table);
+	if (!system_id)
+		return pin;
+
+	override = system_id->driver_data;
+
+	for (i = 0; i < override->num; i++) {
+		fixup = &override->fixups[i];
+		if (pin == fixup->pin_broken)
+			return fixup->pin_correct;
+	}
+
+	return pin;
+}
+
 static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
 {
 	if (!gc->parent)
@@ -125,7 +182,11 @@  static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
 	if (!chip)
 		return ERR_PTR(-EPROBE_DEFER);
 
-	return gpiochip_get_desc(chip, pin);
+	/*
+	 * Give it a chance to correct the broken GPIO pin number in ACPI
+	 * table of particular machines.
+	 */
+	return gpiochip_get_desc(chip, acpi_gpio_pin_override(pin));
 }
 
 static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)