| 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 |
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.
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] [1]: ba8c90c61847 ("gpio: pca953x: Override IRQ for one of the expanders on Galileo Gen 2") [2]: https://lore.kernel.org/linux-gpio/20200520211916.25727-1-andriy.shevchenko@linux.intel.com/T/#u [3]: https://lore.kernel.org/linux-gpio/20210225163320.71267-1-andriy.shevchenko@linux.intel.com/T/#u -- With Best Regards, Andy Shevchenko
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). -- With Best Regards, Andy Shevchenko
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
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
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 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)? > [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 -- With Best Regards, Andy Shevchenko
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. -- With Best Regards, Andy Shevchenko
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
+ 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
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.
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
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 > -- Jeffrey Hugo Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
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
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
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.
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
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. -- With Best Regards, Andy Shevchenko
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 >
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
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
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! -- With Best Regards, Andy Shevchenko
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
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
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).
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 > >
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; -- 2.17.1
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 --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)
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(-) -- 2.17.1