| Message ID | 20230210164636.628462-1-wse@tuxedocomputers.com (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | gpiolib: acpi: Add a ignore wakeup quirk for Clevo NH5xAx | expand |
On Fri, Feb 10, 2023 at 05:46:36PM +0100, Werner Sembach wrote: > commit 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable") > changed the policy such that I2C touchpads may be able to wake up the > system by default if the system is configured as such. > > However on Clevo NH5xAx/TUXEDO XA15 Gen10 there is a mistake in the ACPI > tables that the TP_ATTN# signal connected to GPIO 10 is configured as > ActiveLow and level triggered but connected to a pull up. I'm not sure I understand the issue here. From what you say here it seems correct ACPI description. > As soon as the > system suspends the touchpad loses power and then the system wakes up. > > To avoid this problem, introduce a quirk for this model that will prevent > the wakeup capability for being set for GPIO 10. I'm not against fixing this, but wouldn't be better to actually target the root cause and have a different quirk? Or is it me who didn't get what is the root cause?
Am 10.02.23 um 18:04 schrieb Andy Shevchenko: > On Fri, Feb 10, 2023 at 05:46:36PM +0100, Werner Sembach wrote: >> commit 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable") >> changed the policy such that I2C touchpads may be able to wake up the >> system by default if the system is configured as such. >> >> However on Clevo NH5xAx/TUXEDO XA15 Gen10 there is a mistake in the ACPI >> tables that the TP_ATTN# signal connected to GPIO 10 is configured as >> ActiveLow and level triggered but connected to a pull up. > I'm not sure I understand the issue here. From what you say here it seems > correct ACPI description. TBH I copied the commit description from https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4cb786180dfb5258ff3111181b5e4ecb1d4a297b which is for a different device having the exact same problem. > >> As soon as the >> system suspends the touchpad loses power and then the system wakes up. >> >> To avoid this problem, introduce a quirk for this model that will prevent >> the wakeup capability for being set for GPIO 10. > I'm not against fixing this, but wouldn't be better to actually target the root > cause and have a different quirk? Or is it me who didn't get what is the root > cause? > I missed to reference the original discussion while copying the description: https://gitlab.freedesktop.org/drm/amd/-/issues/1722#note_1720627 (Note that it's a somewhat convoluted issue spanning multiple bugs when you scroll up from that particular linked comment, which are however irrelevant for this patch) I'm not deep into how ACPI defined IRQ work so maybe not a good idea for me summing it up, as I might have misunderstood parts of it ^^ I added the other ones from there to the cc.
On Mon, Feb 13, 2023 at 12:30:08PM +0100, Werner Sembach wrote: > Am 10.02.23 um 18:04 schrieb Andy Shevchenko: > > On Fri, Feb 10, 2023 at 05:46:36PM +0100, Werner Sembach wrote: > > > commit 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable") > > > changed the policy such that I2C touchpads may be able to wake up the > > > system by default if the system is configured as such. > > > > > > However on Clevo NH5xAx/TUXEDO XA15 Gen10 there is a mistake in the ACPI > > > tables that the TP_ATTN# signal connected to GPIO 10 is configured as > > > ActiveLow and level triggered but connected to a pull up. > > I'm not sure I understand the issue here. From what you say here it seems > > correct ACPI description. > TBH I copied the commit description from https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4cb786180dfb5258ff3111181b5e4ecb1d4a297b > which is for a different device having the exact same problem. Yeah, and I reviewed that and seems paid no attention to this detail. So, ActiveLow + PullUp is the _right_ thing to do in ACPI. The problem seems somewhere else. Mario, can we have an access to the schematics of the affected pin to understand better what's going on? Or is that description missing some crucial detail? > > > As soon as the > > > system suspends the touchpad loses power and then the system wakes up. > > > > > > To avoid this problem, introduce a quirk for this model that will prevent > > > the wakeup capability for being set for GPIO 10. > > I'm not against fixing this, but wouldn't be better to actually target the root > > cause and have a different quirk? Or is it me who didn't get what is the root > > cause? > > > I missed to reference the original discussion while copying the description: > https://gitlab.freedesktop.org/drm/amd/-/issues/1722#note_1720627 (Note that > it's a somewhat convoluted issue spanning multiple bugs when you scroll up > from that particular linked comment, which are however irrelevant for this > patch) > > I'm not deep into how ACPI defined IRQ work so maybe not a good idea for me > summing it up, as I might have misunderstood parts of it ^^ The GpioIo() and GpioInt() resources have gaps in them, due to this some additional information is required or some heuristics is used to deduct the settings. All this is described in https://www.kernel.org/doc/html/latest/firmware-guide/acpi/gpio-properties.html > I added the other ones from there to the cc. Thank you.
On 2/13/23 06:41, Andy Shevchenko wrote: > On Mon, Feb 13, 2023 at 12:30:08PM +0100, Werner Sembach wrote: >> Am 10.02.23 um 18:04 schrieb Andy Shevchenko: >>> On Fri, Feb 10, 2023 at 05:46:36PM +0100, Werner Sembach wrote: >>>> commit 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable") >>>> changed the policy such that I2C touchpads may be able to wake up the >>>> system by default if the system is configured as such. >>>> >>>> However on Clevo NH5xAx/TUXEDO XA15 Gen10 there is a mistake in the ACPI >>>> tables that the TP_ATTN# signal connected to GPIO 10 is configured as >>>> ActiveLow and level triggered but connected to a pull up. >>> I'm not sure I understand the issue here. From what you say here it seems >>> correct ACPI description. >> TBH I copied the commit description from https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4cb786180dfb5258ff3111181b5e4ecb1d4a297b >> which is for a different device having the exact same problem. > Yeah, and I reviewed that and seems paid no attention to this detail. > > So, ActiveLow + PullUp is the _right_ thing to do in ACPI. > The problem seems somewhere else. > > Mario, can we have an access to the schematics of the affected pin to > understand better what's going on? > > Or is that description missing some crucial detail? The schematics were shared by the reporter for the original issue which is how we reached the conclusion there was a mistake. As they're both Clevo designs it's certainly possible they have the same mistake in two systems. >>>> As soon as the >>>> system suspends the touchpad loses power and then the system wakes up. >>>> >>>> To avoid this problem, introduce a quirk for this model that will prevent >>>> the wakeup capability for being set for GPIO 10. >>> I'm not against fixing this, but wouldn't be better to actually target the root >>> cause and have a different quirk? Or is it me who didn't get what is the root >>> cause? >>> >> I missed to reference the original discussion while copying the description: >> https://gitlab.freedesktop.org/drm/amd/-/issues/1722#note_1720627 (Note that >> it's a somewhat convoluted issue spanning multiple bugs when you scroll up >> from that particular linked comment, which are however irrelevant for this >> patch) >> >> I'm not deep into how ACPI defined IRQ work so maybe not a good idea for me >> summing it up, as I might have misunderstood parts of it ^^ > The GpioIo() and GpioInt() resources have gaps in them, due to this some > additional information is required or some heuristics is used to deduct > the settings. > > All this is described in > https://www.kernel.org/doc/html/latest/firmware-guide/acpi/gpio-properties.html > >> I added the other ones from there to the cc. > Thank you. >
On Mon, Feb 13, 2023 at 07:20:48AM -0600, Mario Limonciello wrote: > On 2/13/23 06:41, Andy Shevchenko wrote: > > On Mon, Feb 13, 2023 at 12:30:08PM +0100, Werner Sembach wrote: > > > Am 10.02.23 um 18:04 schrieb Andy Shevchenko: > > > > On Fri, Feb 10, 2023 at 05:46:36PM +0100, Werner Sembach wrote: > > > > > commit 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable") > > > > > changed the policy such that I2C touchpads may be able to wake up the > > > > > system by default if the system is configured as such. > > > > > > > > > > However on Clevo NH5xAx/TUXEDO XA15 Gen10 there is a mistake in the ACPI > > > > > tables that the TP_ATTN# signal connected to GPIO 10 is configured as > > > > > ActiveLow and level triggered but connected to a pull up. > > > > I'm not sure I understand the issue here. From what you say here it seems > > > > correct ACPI description. > > > TBH I copied the commit description from https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4cb786180dfb5258ff3111181b5e4ecb1d4a297b > > > which is for a different device having the exact same problem. > > Yeah, and I reviewed that and seems paid no attention to this detail. > > > > So, ActiveLow + PullUp is the _right_ thing to do in ACPI. > > The problem seems somewhere else. > > > > Mario, can we have an access to the schematics of the affected pin to > > understand better what's going on? > > > > Or is that description missing some crucial detail? > > The schematics were shared by the reporter for the original issue which is > how we reached the conclusion there was a mistake. > > As they're both Clevo designs it's certainly possible they have the same > mistake in two systems. Thank you! I have looked at the schematics and read discussion. So, the conclusion that this is a BIOS bug is incorrect in my opinion. The problem is either in the PMIC/EC firmware that shouldn't shut down 3.3VS signal for a while or on the PCB level, so that pull up should be connected to another power source that stays on. This means the description on the initial patch with the same issue is incorrect. Do we know the power sequence on the suspend to see which and how on the time line the power sources are off/on? > > > > > As soon as the > > > > > system suspends the touchpad loses power and then the system wakes up. > > > > > > > > > > To avoid this problem, introduce a quirk for this model that will prevent > > > > > the wakeup capability for being set for GPIO 10. > > > > I'm not against fixing this, but wouldn't be better to actually target the root > > > > cause and have a different quirk? Or is it me who didn't get what is the root > > > > cause? > > > > > > > I missed to reference the original discussion while copying the description: > > > https://gitlab.freedesktop.org/drm/amd/-/issues/1722#note_1720627 (Note that > > > it's a somewhat convoluted issue spanning multiple bugs when you scroll up > > > from that particular linked comment, which are however irrelevant for this > > > patch) > > > > > > I'm not deep into how ACPI defined IRQ work so maybe not a good idea for me > > > summing it up, as I might have misunderstood parts of it ^^ > > The GpioIo() and GpioInt() resources have gaps in them, due to this some > > additional information is required or some heuristics is used to deduct > > the settings. > > > > All this is described in > > https://www.kernel.org/doc/html/latest/firmware-guide/acpi/gpio-properties.html > > > > > I added the other ones from there to the cc. > > Thank you.
Am 13.02.23 um 15:37 schrieb Andy Shevchenko: > On Mon, Feb 13, 2023 at 07:20:48AM -0600, Mario Limonciello wrote: >> On 2/13/23 06:41, Andy Shevchenko wrote: >>> On Mon, Feb 13, 2023 at 12:30:08PM +0100, Werner Sembach wrote: >>>> Am 10.02.23 um 18:04 schrieb Andy Shevchenko: >>>>> On Fri, Feb 10, 2023 at 05:46:36PM +0100, Werner Sembach wrote: >>>>>> commit 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable") >>>>>> changed the policy such that I2C touchpads may be able to wake up the >>>>>> system by default if the system is configured as such. >>>>>> >>>>>> However on Clevo NH5xAx/TUXEDO XA15 Gen10 there is a mistake in the ACPI >>>>>> tables that the TP_ATTN# signal connected to GPIO 10 is configured as >>>>>> ActiveLow and level triggered but connected to a pull up. >>>>> I'm not sure I understand the issue here. From what you say here it seems >>>>> correct ACPI description. >>>> TBH I copied the commit description from https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4cb786180dfb5258ff3111181b5e4ecb1d4a297b >>>> which is for a different device having the exact same problem. >>> Yeah, and I reviewed that and seems paid no attention to this detail. >>> >>> So, ActiveLow + PullUp is the _right_ thing to do in ACPI. >>> The problem seems somewhere else. >>> >>> Mario, can we have an access to the schematics of the affected pin to >>> understand better what's going on? >>> >>> Or is that description missing some crucial detail? Schematics for the NH5xAx can also be found on this unofficial clevo mirror (service manuals, scroll to end for schematics): http://repo.palkeo.com/clevo-mirror/NH5xACx_AFx_ADx/NH50AC.zip http://repo.palkeo.com/clevo-mirror/NH5xACx_AFx_ADx/NH50AF1.zip User: repo PW: repo >> The schematics were shared by the reporter for the original issue which is >> how we reached the conclusion there was a mistake. >> >> As they're both Clevo designs it's certainly possible they have the same >> mistake in two systems. > Thank you! > I have looked at the schematics and read discussion. > > So, the conclusion that this is a BIOS bug is incorrect in my opinion. > The problem is either in the PMIC/EC firmware that shouldn't shut down 3.3VS > signal for a while or on the PCB level, so that pull up should be connected > to another power source that stays on. > > This means the description on the initial patch with the same issue is > incorrect. > > Do we know the power sequence on the suspend to see which and how on the > time line the power sources are off/on? > >>>>>> As soon as the >>>>>> system suspends the touchpad loses power and then the system wakes up. >>>>>> >>>>>> To avoid this problem, introduce a quirk for this model that will prevent >>>>>> the wakeup capability for being set for GPIO 10. >>>>> I'm not against fixing this, but wouldn't be better to actually target the root >>>>> cause and have a different quirk? Or is it me who didn't get what is the root >>>>> cause? >>>>> >>>> I missed to reference the original discussion while copying the description: >>>> https://gitlab.freedesktop.org/drm/amd/-/issues/1722#note_1720627 (Note that >>>> it's a somewhat convoluted issue spanning multiple bugs when you scroll up >>>> from that particular linked comment, which are however irrelevant for this >>>> patch) >>>> >>>> I'm not deep into how ACPI defined IRQ work so maybe not a good idea for me >>>> summing it up, as I might have misunderstood parts of it ^^ >>> The GpioIo() and GpioInt() resources have gaps in them, due to this some >>> additional information is required or some heuristics is used to deduct >>> the settings. >>> >>> All this is described in >>> https://www.kernel.org/doc/html/latest/firmware-guide/acpi/gpio-properties.html >>> >>>> I added the other ones from there to the cc. >>> Thank you.
SUSB#_EN On Mon, Feb 13, 2023 at 7:47 AM Werner Sembach <wse@tuxedocomputers.com> wrote: > > Am 13.02.23 um 15:37 schrieb Andy Shevchenko: > > On Mon, Feb 13, 2023 at 07:20:48AM -0600, Mario Limonciello wrote: > >> On 2/13/23 06:41, Andy Shevchenko wrote: > >>> On Mon, Feb 13, 2023 at 12:30:08PM +0100, Werner Sembach wrote: > >>>> Am 10.02.23 um 18:04 schrieb Andy Shevchenko: > >>>>> On Fri, Feb 10, 2023 at 05:46:36PM +0100, Werner Sembach wrote: > >>>>>> commit 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable") > >>>>>> changed the policy such that I2C touchpads may be able to wake up the > >>>>>> system by default if the system is configured as such. > >>>>>> > >>>>>> However on Clevo NH5xAx/TUXEDO XA15 Gen10 there is a mistake in the ACPI > >>>>>> tables that the TP_ATTN# signal connected to GPIO 10 is configured as > >>>>>> ActiveLow and level triggered but connected to a pull up. > >>>>> I'm not sure I understand the issue here. From what you say here it seems > >>>>> correct ACPI description. > >>>> TBH I copied the commit description from https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4cb786180dfb5258ff3111181b5e4ecb1d4a297b > >>>> which is for a different device having the exact same problem. > >>> Yeah, and I reviewed that and seems paid no attention to this detail. > >>> > >>> So, ActiveLow + PullUp is the _right_ thing to do in ACPI. > >>> The problem seems somewhere else. > >>> > >>> Mario, can we have an access to the schematics of the affected pin to > >>> understand better what's going on? > >>> > >>> Or is that description missing some crucial detail? > > Schematics for the NH5xAx can also be found on this unofficial clevo mirror > (service manuals, scroll to end for schematics): > > http://repo.palkeo.com/clevo-mirror/NH5xACx_AFx_ADx/NH50AC.zip > > http://repo.palkeo.com/clevo-mirror/NH5xACx_AFx_ADx/NH50AF1.zip > > User: repo > > PW: repo > > >> The schematics were shared by the reporter for the original issue which is > >> how we reached the conclusion there was a mistake. > >> > >> As they're both Clevo designs it's certainly possible they have the same > >> mistake in two systems. > > Thank you! > > I have looked at the schematics and read discussion. > > > > So, the conclusion that this is a BIOS bug is incorrect in my opinion. > > The problem is either in the PMIC/EC firmware that shouldn't shut down 3.3VS > > signal for a while or on the PCB level, so that pull up should be connected > > to another power source that stays on. > > > > This means the description on the initial patch with the same issue is > > incorrect. > > > > Do we know the power sequence on the suspend to see which and how on the > > time line the power sources are off/on? > > If you look at the load switch for 3.3VS, its EN2 pin is connected to SUSB#_EN which is connected to SUSB# which is connected to AND(SUSB#_PCH -> SLP_S3_L, PM_SLP_S0 -> S0A3_GPIO). So there is no PMIC/EC firmware that is incharge of this. I guess I'm not quite sure how they have S0A3_GPIO configured, so maybe I have an invert wrong. The EC does control DD_ON which controls the 3.3V and 5V rails. > >>>>>> As soon as the > >>>>>> system suspends the touchpad loses power and then the system wakes up. > >>>>>> > >>>>>> To avoid this problem, introduce a quirk for this model that will prevent > >>>>>> the wakeup capability for being set for GPIO 10. > >>>>> I'm not against fixing this, but wouldn't be better to actually target the root > >>>>> cause and have a different quirk? Or is it me who didn't get what is the root > >>>>> cause? > >>>>> > >>>> I missed to reference the original discussion while copying the description: > >>>> https://gitlab.freedesktop.org/drm/amd/-/issues/1722#note_1720627 (Note that > >>>> it's a somewhat convoluted issue spanning multiple bugs when you scroll up > >>>> from that particular linked comment, which are however irrelevant for this > >>>> patch) > >>>> > >>>> I'm not deep into how ACPI defined IRQ work so maybe not a good idea for me > >>>> summing it up, as I might have misunderstood parts of it ^^ > >>> The GpioIo() and GpioInt() resources have gaps in them, due to this some > >>> additional information is required or some heuristics is used to deduct > >>> the settings. > >>> > >>> All this is described in > >>> https://www.kernel.org/doc/html/latest/firmware-guide/acpi/gpio-properties.html > >>> > >>>> I added the other ones from there to the cc. > >>> Thank you.
On Mon, Feb 13, 2023 at 10:20:41AM -0700, Raul Rangel wrote: > On Mon, Feb 13, 2023 at 7:47 AM Werner Sembach <wse@tuxedocomputers.com> wrote: > > Am 13.02.23 um 15:37 schrieb Andy Shevchenko: > > > On Mon, Feb 13, 2023 at 07:20:48AM -0600, Mario Limonciello wrote: > > >> On 2/13/23 06:41, Andy Shevchenko wrote: > > >>> On Mon, Feb 13, 2023 at 12:30:08PM +0100, Werner Sembach wrote: > > >>>> Am 10.02.23 um 18:04 schrieb Andy Shevchenko: > > >>>>> On Fri, Feb 10, 2023 at 05:46:36PM +0100, Werner Sembach wrote: > > >>>>>> commit 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable") > > >>>>>> changed the policy such that I2C touchpads may be able to wake up the > > >>>>>> system by default if the system is configured as such. > > >>>>>> > > >>>>>> However on Clevo NH5xAx/TUXEDO XA15 Gen10 there is a mistake in the ACPI > > >>>>>> tables that the TP_ATTN# signal connected to GPIO 10 is configured as > > >>>>>> ActiveLow and level triggered but connected to a pull up. > > >>>>> I'm not sure I understand the issue here. From what you say here it seems > > >>>>> correct ACPI description. > > >>>> TBH I copied the commit description from https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4cb786180dfb5258ff3111181b5e4ecb1d4a297b > > >>>> which is for a different device having the exact same problem. > > >>> Yeah, and I reviewed that and seems paid no attention to this detail. > > >>> > > >>> So, ActiveLow + PullUp is the _right_ thing to do in ACPI. > > >>> The problem seems somewhere else. > > >>> > > >>> Mario, can we have an access to the schematics of the affected pin to > > >>> understand better what's going on? > > >>> > > >>> Or is that description missing some crucial detail? > > > > Schematics for the NH5xAx can also be found on this unofficial clevo mirror > > (service manuals, scroll to end for schematics): > > > > http://repo.palkeo.com/clevo-mirror/NH5xACx_AFx_ADx/NH50AC.zip > > > > http://repo.palkeo.com/clevo-mirror/NH5xACx_AFx_ADx/NH50AF1.zip > > > > User: repo > > > > PW: repo > > > > >> The schematics were shared by the reporter for the original issue which is > > >> how we reached the conclusion there was a mistake. > > >> > > >> As they're both Clevo designs it's certainly possible they have the same > > >> mistake in two systems. > > > > Thank you! > > > I have looked at the schematics and read discussion. > > > > > > So, the conclusion that this is a BIOS bug is incorrect in my opinion. > > > The problem is either in the PMIC/EC firmware that shouldn't shut down 3.3VS > > > signal for a while or on the PCB level, so that pull up should be connected > > > to another power source that stays on. > > > > > > This means the description on the initial patch with the same issue is > > > incorrect. > > > > > > Do we know the power sequence on the suspend to see which and how on the > > > time line the power sources are off/on? > > If you look at the load switch for 3.3VS, its EN2 pin is connected to > SUSB#_EN which is connected to SUSB# which is connected to > AND(SUSB#_PCH -> SLP_S3_L, PM_SLP_S0 -> S0A3_GPIO). So there is no > PMIC/EC firmware that is incharge of this. I guess I'm not quite sure > how they have S0A3_GPIO configured, so maybe I have an invert wrong. > > The EC does control DD_ON which controls the 3.3V and 5V rails. On page 6 of the schematics I see the U7 that forms SUSB# from SUSB#_APU (which corresponds to what you said) _and_ EC_EN, which is GPIO from IT5570, which is EC. Are you using different schematics? I'm using the one from FDO bug report. > > >>>>>> As soon as the > > >>>>>> system suspends the touchpad loses power and then the system wakes up. > > >>>>>> > > >>>>>> To avoid this problem, introduce a quirk for this model that will prevent > > >>>>>> the wakeup capability for being set for GPIO 10. > > >>>>> I'm not against fixing this, but wouldn't be better to actually target the root > > >>>>> cause and have a different quirk? Or is it me who didn't get what is the root > > >>>>> cause? > > >>>>> > > >>>> I missed to reference the original discussion while copying the description: > > >>>> https://gitlab.freedesktop.org/drm/amd/-/issues/1722#note_1720627 (Note that > > >>>> it's a somewhat convoluted issue spanning multiple bugs when you scroll up > > >>>> from that particular linked comment, which are however irrelevant for this > > >>>> patch) > > >>>> > > >>>> I'm not deep into how ACPI defined IRQ work so maybe not a good idea for me > > >>>> summing it up, as I might have misunderstood parts of it ^^ > > >>> The GpioIo() and GpioInt() resources have gaps in them, due to this some > > >>> additional information is required or some heuristics is used to deduct > > >>> the settings. > > >>> > > >>> All this is described in > > >>> https://www.kernel.org/doc/html/latest/firmware-guide/acpi/gpio-properties.html > > >>> > > >>>> I added the other ones from there to the cc. > > >>> Thank you.
On Mon, Feb 13, 2023 at 07:50:02PM +0200, Andy Shevchenko wrote: > On Mon, Feb 13, 2023 at 10:20:41AM -0700, Raul Rangel wrote: > > On Mon, Feb 13, 2023 at 7:47 AM Werner Sembach <wse@tuxedocomputers.com> wrote: > > > Am 13.02.23 um 15:37 schrieb Andy Shevchenko: > > > > On Mon, Feb 13, 2023 at 07:20:48AM -0600, Mario Limonciello wrote: > > > >> On 2/13/23 06:41, Andy Shevchenko wrote: > > > >>> On Mon, Feb 13, 2023 at 12:30:08PM +0100, Werner Sembach wrote: > > > >>>> Am 10.02.23 um 18:04 schrieb Andy Shevchenko: > > > >>>>> On Fri, Feb 10, 2023 at 05:46:36PM +0100, Werner Sembach wrote: > > > >>>>>> commit 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable") > > > >>>>>> changed the policy such that I2C touchpads may be able to wake up the > > > >>>>>> system by default if the system is configured as such. > > > >>>>>> > > > >>>>>> However on Clevo NH5xAx/TUXEDO XA15 Gen10 there is a mistake in the ACPI > > > >>>>>> tables that the TP_ATTN# signal connected to GPIO 10 is configured as > > > >>>>>> ActiveLow and level triggered but connected to a pull up. > > > >>>>> I'm not sure I understand the issue here. From what you say here it seems > > > >>>>> correct ACPI description. > > > >>>> TBH I copied the commit description from https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=4cb786180dfb5258ff3111181b5e4ecb1d4a297b > > > >>>> which is for a different device having the exact same problem. > > > >>> Yeah, and I reviewed that and seems paid no attention to this detail. > > > >>> > > > >>> So, ActiveLow + PullUp is the _right_ thing to do in ACPI. > > > >>> The problem seems somewhere else. > > > >>> > > > >>> Mario, can we have an access to the schematics of the affected pin to > > > >>> understand better what's going on? > > > >>> > > > >>> Or is that description missing some crucial detail? > > > > > > Schematics for the NH5xAx can also be found on this unofficial clevo mirror > > > (service manuals, scroll to end for schematics): > > > > > > http://repo.palkeo.com/clevo-mirror/NH5xACx_AFx_ADx/NH50AC.zip > > > > > > http://repo.palkeo.com/clevo-mirror/NH5xACx_AFx_ADx/NH50AF1.zip > > > > > > User: repo > > > > > > PW: repo > > > > > > >> The schematics were shared by the reporter for the original issue which is > > > >> how we reached the conclusion there was a mistake. > > > >> > > > >> As they're both Clevo designs it's certainly possible they have the same > > > >> mistake in two systems. > > > > > > Thank you! > > > > I have looked at the schematics and read discussion. > > > > > > > > So, the conclusion that this is a BIOS bug is incorrect in my opinion. > > > > The problem is either in the PMIC/EC firmware that shouldn't shut down 3.3VS > > > > signal for a while or on the PCB level, so that pull up should be connected > > > > to another power source that stays on. > > > > > > > > This means the description on the initial patch with the same issue is > > > > incorrect. > > > > > > > > Do we know the power sequence on the suspend to see which and how on the > > > > time line the power sources are off/on? > > > > If you look at the load switch for 3.3VS, its EN2 pin is connected to > > SUSB#_EN which is connected to SUSB# which is connected to > > AND(SUSB#_PCH -> SLP_S3_L, PM_SLP_S0 -> S0A3_GPIO). So there is no > > PMIC/EC firmware that is incharge of this. I guess I'm not quite sure > > how they have S0A3_GPIO configured, so maybe I have an invert wrong. > > > > The EC does control DD_ON which controls the 3.3V and 5V rails. > > On page 6 of the schematics I see the U7 that forms SUSB# from SUSB#_APU > (which corresponds to what you said) _and_ EC_EN, which is GPIO from IT5570, > which is EC. > > Are you using different schematics? I'm using the one from FDO bug report. Just checked this one: http://repo.palkeo.com/clevo-mirror/NH5xACx_AFx_ADx/NH50AC.zip Also uses EC (SUSB_EC#). > > > >>>>>> As soon as the > > > >>>>>> system suspends the touchpad loses power and then the system wakes up. > > > >>>>>> > > > >>>>>> To avoid this problem, introduce a quirk for this model that will prevent > > > >>>>>> the wakeup capability for being set for GPIO 10. > > > >>>>> I'm not against fixing this, but wouldn't be better to actually target the root > > > >>>>> cause and have a different quirk? Or is it me who didn't get what is the root > > > >>>>> cause? > > > >>>>> > > > >>>> I missed to reference the original discussion while copying the description: > > > >>>> https://gitlab.freedesktop.org/drm/amd/-/issues/1722#note_1720627 (Note that > > > >>>> it's a somewhat convoluted issue spanning multiple bugs when you scroll up > > > >>>> from that particular linked comment, which are however irrelevant for this > > > >>>> patch) > > > >>>> > > > >>>> I'm not deep into how ACPI defined IRQ work so maybe not a good idea for me > > > >>>> summing it up, as I might have misunderstood parts of it ^^ > > > >>> The GpioIo() and GpioInt() resources have gaps in them, due to this some > > > >>> additional information is required or some heuristics is used to deduct > > > >>> the settings. > > > >>> > > > >>> All this is described in > > > >>> https://www.kernel.org/doc/html/latest/firmware-guide/acpi/gpio-properties.html > > > >>> > > > >>>> I added the other ones from there to the cc. > > > >>> Thank you.
On Mon, Feb 13, 2023 at 07:57:55PM +0200, Andy Shevchenko wrote: > On Mon, Feb 13, 2023 at 07:50:02PM +0200, Andy Shevchenko wrote: > > On Mon, Feb 13, 2023 at 10:20:41AM -0700, Raul Rangel wrote: > > > On Mon, Feb 13, 2023 at 7:47 AM Werner Sembach <wse@tuxedocomputers.com> wrote: > > > > Am 13.02.23 um 15:37 schrieb Andy Shevchenko: ... > > > > Schematics for the NH5xAx can also be found on this unofficial clevo mirror > > > > (service manuals, scroll to end for schematics): > > > > > > > > http://repo.palkeo.com/clevo-mirror/NH5xACx_AFx_ADx/NH50AC.zip > > > > > > > > http://repo.palkeo.com/clevo-mirror/NH5xACx_AFx_ADx/NH50AF1.zip > > > > > > > > User: repo > > > > > > > > PW: repo > > > > > > > > >> The schematics were shared by the reporter for the original issue which is > > > > >> how we reached the conclusion there was a mistake. > > > > >> > > > > >> As they're both Clevo designs it's certainly possible they have the same > > > > >> mistake in two systems. > > > > > > > > Thank you! > > > > > I have looked at the schematics and read discussion. > > > > > > > > > > So, the conclusion that this is a BIOS bug is incorrect in my opinion. > > > > > The problem is either in the PMIC/EC firmware that shouldn't shut down 3.3VS > > > > > signal for a while or on the PCB level, so that pull up should be connected > > > > > to another power source that stays on. > > > > > > > > > > This means the description on the initial patch with the same issue is > > > > > incorrect. > > > > > > > > > > Do we know the power sequence on the suspend to see which and how on the > > > > > time line the power sources are off/on? > > > > > > If you look at the load switch for 3.3VS, its EN2 pin is connected to > > > SUSB#_EN which is connected to SUSB# which is connected to > > > AND(SUSB#_PCH -> SLP_S3_L, PM_SLP_S0 -> S0A3_GPIO). So there is no > > > PMIC/EC firmware that is incharge of this. I guess I'm not quite sure > > > how they have S0A3_GPIO configured, so maybe I have an invert wrong. > > > > > > The EC does control DD_ON which controls the 3.3V and 5V rails. > > > > On page 6 of the schematics I see the U7 that forms SUSB# from SUSB#_APU > > (which corresponds to what you said) _and_ EC_EN, which is GPIO from IT5570, > > which is EC. > > > > Are you using different schematics? I'm using the one from FDO bug report. > > Just checked this one: > http://repo.palkeo.com/clevo-mirror/NH5xACx_AFx_ADx/NH50AC.zip > > Also uses EC (SUSB_EC#). So this all makes me thing that either EC firmware is buggy or we have ACPI EC code in the kernel to fix.
On Mon, Feb 13, 2023 at 07:59:53PM +0200, Andy Shevchenko wrote: > On Mon, Feb 13, 2023 at 07:57:55PM +0200, Andy Shevchenko wrote: > > On Mon, Feb 13, 2023 at 07:50:02PM +0200, Andy Shevchenko wrote: > > > On Mon, Feb 13, 2023 at 10:20:41AM -0700, Raul Rangel wrote: > > > > On Mon, Feb 13, 2023 at 7:47 AM Werner Sembach <wse@tuxedocomputers.com> wrote: > > > > > Am 13.02.23 um 15:37 schrieb Andy Shevchenko: ... > > > > > Schematics for the NH5xAx can also be found on this unofficial clevo mirror > > > > > (service manuals, scroll to end for schematics): > > > > > > > > > > http://repo.palkeo.com/clevo-mirror/NH5xACx_AFx_ADx/NH50AC.zip > > > > > > > > > > http://repo.palkeo.com/clevo-mirror/NH5xACx_AFx_ADx/NH50AF1.zip > > > > > > > > > > User: repo > > > > > > > > > > PW: repo > > > > > > > > > > >> The schematics were shared by the reporter for the original issue which is > > > > > >> how we reached the conclusion there was a mistake. > > > > > >> > > > > > >> As they're both Clevo designs it's certainly possible they have the same > > > > > >> mistake in two systems. > > > > > > > > > > Thank you! > > > > > > I have looked at the schematics and read discussion. > > > > > > > > > > > > So, the conclusion that this is a BIOS bug is incorrect in my opinion. > > > > > > The problem is either in the PMIC/EC firmware that shouldn't shut down 3.3VS > > > > > > signal for a while or on the PCB level, so that pull up should be connected > > > > > > to another power source that stays on. > > > > > > > > > > > > This means the description on the initial patch with the same issue is > > > > > > incorrect. > > > > > > > > > > > > Do we know the power sequence on the suspend to see which and how on the > > > > > > time line the power sources are off/on? > > > > > > > > If you look at the load switch for 3.3VS, its EN2 pin is connected to > > > > SUSB#_EN which is connected to SUSB# which is connected to > > > > AND(SUSB#_PCH -> SLP_S3_L, PM_SLP_S0 -> S0A3_GPIO). So there is no > > > > PMIC/EC firmware that is incharge of this. I guess I'm not quite sure > > > > how they have S0A3_GPIO configured, so maybe I have an invert wrong. > > > > > > > > The EC does control DD_ON which controls the 3.3V and 5V rails. > > > > > > On page 6 of the schematics I see the U7 that forms SUSB# from SUSB#_APU > > > (which corresponds to what you said) _and_ EC_EN, which is GPIO from IT5570, > > > which is EC. > > > > > > Are you using different schematics? I'm using the one from FDO bug report. > > > > Just checked this one: > > http://repo.palkeo.com/clevo-mirror/NH5xACx_AFx_ADx/NH50AC.zip > > > > Also uses EC (SUSB_EC#). Sorry, this has to be read as SUSBC_EC#. > So this all makes me thing that either EC firmware is buggy or we have ACPI EC > code in the kernel to fix.
On Mon, Feb 13, 2023 at 11:01 AM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, Feb 13, 2023 at 07:59:53PM +0200, Andy Shevchenko wrote: > > On Mon, Feb 13, 2023 at 07:57:55PM +0200, Andy Shevchenko wrote: > > > On Mon, Feb 13, 2023 at 07:50:02PM +0200, Andy Shevchenko wrote: > > > > On Mon, Feb 13, 2023 at 10:20:41AM -0700, Raul Rangel wrote: > > > > > On Mon, Feb 13, 2023 at 7:47 AM Werner Sembach <wse@tuxedocomputers.com> wrote: > > > > > > Am 13.02.23 um 15:37 schrieb Andy Shevchenko: > > ... > > > > > > > Schematics for the NH5xAx can also be found on this unofficial clevo mirror > > > > > > (service manuals, scroll to end for schematics): > > > > > > > > > > > > http://repo.palkeo.com/clevo-mirror/NH5xACx_AFx_ADx/NH50AC.zip > > > > > > > > > > > > http://repo.palkeo.com/clevo-mirror/NH5xACx_AFx_ADx/NH50AF1.zip > > > > > > > > > > > > User: repo > > > > > > > > > > > > PW: repo > > > > > > > > > > > > >> The schematics were shared by the reporter for the original issue which is > > > > > > >> how we reached the conclusion there was a mistake. > > > > > > >> > > > > > > >> As they're both Clevo designs it's certainly possible they have the same > > > > > > >> mistake in two systems. > > > > > > > > > > > > Thank you! > > > > > > > I have looked at the schematics and read discussion. > > > > > > > > > > > > > > So, the conclusion that this is a BIOS bug is incorrect in my opinion. > > > > > > > The problem is either in the PMIC/EC firmware that shouldn't shut down 3.3VS > > > > > > > signal for a while or on the PCB level, so that pull up should be connected > > > > > > > to another power source that stays on. > > > > > > > > > > > > > > This means the description on the initial patch with the same issue is > > > > > > > incorrect. > > > > > > > > > > > > > > Do we know the power sequence on the suspend to see which and how on the > > > > > > > time line the power sources are off/on? > > > > > > > > > > If you look at the load switch for 3.3VS, its EN2 pin is connected to > > > > > SUSB#_EN which is connected to SUSB# which is connected to > > > > > AND(SUSB#_PCH -> SLP_S3_L, PM_SLP_S0 -> S0A3_GPIO). So there is no > > > > > PMIC/EC firmware that is incharge of this. I guess I'm not quite sure > > > > > how they have S0A3_GPIO configured, so maybe I have an invert wrong. > > > > > > > > > > The EC does control DD_ON which controls the 3.3V and 5V rails. > > > > > > > > On page 6 of the schematics I see the U7 that forms SUSB# from SUSB#_APU > > > > (which corresponds to what you said) _and_ EC_EN, which is GPIO from IT5570, > > > > which is EC. > > > > > > > > Are you using different schematics? I'm using the one from FDO bug report. > > > > > > Just checked this one: > > > http://repo.palkeo.com/clevo-mirror/NH5xACx_AFx_ADx/NH50AC.zip > > > > > > Also uses EC (SUSB_EC#). > > Sorry, this has to be read as SUSBC_EC#. It looks like SUSBC_EC# has to stay high during S3/S0i3 otherwise it's going to shut down the S5 power domain. So I'm guessing U7 is there to prevent the S3 domain from being powered on while the S5 domain is powered off. Sheet 59 of 73 VDDCR_SOC_S5, VDDCR_ALW seems to have a helpful table that describes all the power states. I'm confused where SLP_SUS# comes from though. I'm also not sure about S5_MUX_CTRL since that seems to be connected to a testpoint. > > > So this all makes me thing that either EC firmware is buggy or we have ACPI EC > > code in the kernel to fix. > > -- > With Best Regards, > Andy Shevchenko > >
On Mon, Feb 13, 2023 at 11:20:40AM -0700, Raul Rangel wrote: > On Mon, Feb 13, 2023 at 11:01 AM Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > > > On Mon, Feb 13, 2023 at 07:59:53PM +0200, Andy Shevchenko wrote: > > > On Mon, Feb 13, 2023 at 07:57:55PM +0200, Andy Shevchenko wrote: > > > > On Mon, Feb 13, 2023 at 07:50:02PM +0200, Andy Shevchenko wrote: > > > > > On Mon, Feb 13, 2023 at 10:20:41AM -0700, Raul Rangel wrote: > > > > > > On Mon, Feb 13, 2023 at 7:47 AM Werner Sembach <wse@tuxedocomputers.com> wrote: > > > > > > > Am 13.02.23 um 15:37 schrieb Andy Shevchenko: ... > > > > > > > Schematics for the NH5xAx can also be found on this unofficial clevo mirror > > > > > > > (service manuals, scroll to end for schematics): > > > > > > > > > > > > > > http://repo.palkeo.com/clevo-mirror/NH5xACx_AFx_ADx/NH50AC.zip > > > > > > > > > > > > > > http://repo.palkeo.com/clevo-mirror/NH5xACx_AFx_ADx/NH50AF1.zip > > > > > > > > > > > > > > User: repo > > > > > > > > > > > > > > PW: repo > > > > > > > > > > > > > > >> The schematics were shared by the reporter for the original issue which is > > > > > > > >> how we reached the conclusion there was a mistake. > > > > > > > >> > > > > > > > >> As they're both Clevo designs it's certainly possible they have the same > > > > > > > >> mistake in two systems. > > > > > > > > > > > > > > Thank you! > > > > > > > > I have looked at the schematics and read discussion. > > > > > > > > > > > > > > > > So, the conclusion that this is a BIOS bug is incorrect in my opinion. > > > > > > > > The problem is either in the PMIC/EC firmware that shouldn't shut down 3.3VS > > > > > > > > signal for a while or on the PCB level, so that pull up should be connected > > > > > > > > to another power source that stays on. > > > > > > > > > > > > > > > > This means the description on the initial patch with the same issue is > > > > > > > > incorrect. > > > > > > > > > > > > > > > > Do we know the power sequence on the suspend to see which and how on the > > > > > > > > time line the power sources are off/on? > > > > > > > > > > > > If you look at the load switch for 3.3VS, its EN2 pin is connected to > > > > > > SUSB#_EN which is connected to SUSB# which is connected to > > > > > > AND(SUSB#_PCH -> SLP_S3_L, PM_SLP_S0 -> S0A3_GPIO). So there is no > > > > > > PMIC/EC firmware that is incharge of this. I guess I'm not quite sure > > > > > > how they have S0A3_GPIO configured, so maybe I have an invert wrong. > > > > > > > > > > > > The EC does control DD_ON which controls the 3.3V and 5V rails. > > > > > > > > > > On page 6 of the schematics I see the U7 that forms SUSB# from SUSB#_APU > > > > > (which corresponds to what you said) _and_ EC_EN, which is GPIO from IT5570, > > > > > which is EC. > > > > > > > > > > Are you using different schematics? I'm using the one from FDO bug report. > > > > > > > > Just checked this one: > > > > http://repo.palkeo.com/clevo-mirror/NH5xACx_AFx_ADx/NH50AC.zip > > > > > > > > Also uses EC (SUSB_EC#). > > > > Sorry, this has to be read as SUSBC_EC#. > > It looks like SUSBC_EC# has to stay high during S3/S0i3 otherwise it's > going to shut down the S5 power domain. So I'm guessing U7 is there to > prevent the S3 domain from being powered on while the S5 domain is > powered off. > > Sheet 59 of 73 VDDCR_SOC_S5, VDDCR_ALW seems to have a helpful table > that describes all the power states. I'm confused where SLP_SUS# comes > from though. I'm also not sure about S5_MUX_CTRL since that seems to > be connected to a testpoint. I see, thanks for looking into this. So, if EC has no business with this, whose responsibility to assert that signal? I'm trying to get if it's PCB issue (wrong power rail for pull up) or something else. Btw, is anybody can actually boot Windows and check the signals of the pin on a time line in comparison to Linux to see if the behaviour of them is the same? (If Windows and Linux got the same timeline of the signals it would mean that the issue in the Linux kernel.) > > > So this all makes me thing that either EC firmware is buggy or we have ACPI EC > > > code in the kernel to fix.
Fri, Feb 10, 2023 at 05:46:36PM +0100, Werner Sembach kirjoitti: > commit 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable") > changed the policy such that I2C touchpads may be able to wake up the > system by default if the system is configured as such. > > However on Clevo NH5xAx/TUXEDO XA15 Gen10 there is a mistake in the ACPI > tables that the TP_ATTN# signal connected to GPIO 10 is configured as > ActiveLow and level triggered but connected to a pull up. As soon as the > system suspends the touchpad loses power and then the system wakes up. > > To avoid this problem, introduce a quirk for this model that will prevent > the wakeup capability for being set for GPIO 10. Please, update the commit message and send v2. We would like to have this fix nevertheless, because users will suffer w/o it.
Am 13.02.23 um 22:56 schrieb andy.shevchenko@gmail.com: > Fri, Feb 10, 2023 at 05:46:36PM +0100, Werner Sembach kirjoitti: >> commit 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable") >> changed the policy such that I2C touchpads may be able to wake up the >> system by default if the system is configured as such. >> >> However on Clevo NH5xAx/TUXEDO XA15 Gen10 there is a mistake in the ACPI >> tables that the TP_ATTN# signal connected to GPIO 10 is configured as >> ActiveLow and level triggered but connected to a pull up. As soon as the >> system suspends the touchpad loses power and then the system wakes up. >> >> To avoid this problem, introduce a quirk for this model that will prevent >> the wakeup capability for being set for GPIO 10. > Please, update the commit message and send v2. > > We would like to have this fix nevertheless, because users will suffer w/o it. > OK I'm on it.
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c index e2ab4d5253bea..82e8e43582eba 100644 --- a/drivers/gpio/gpiolib-acpi.c +++ b/drivers/gpio/gpiolib-acpi.c @@ -1612,6 +1612,18 @@ static const struct dmi_system_id gpiolib_acpi_quirks[] __initconst = { .ignore_wake = "ELAN0415:00@9", }, }, + { + /* + * Spurious wakeups from TP_ATTN# pin + * Found in BIOS 1.7.7 + */ + .matches = { + DMI_MATCH(DMI_BOARD_NAME, "NH5xAx"), + }, + .driver_data = &(struct acpi_gpiolib_dmi_quirk) { + .ignore_wake = "SYNA1202:00@16", + }, + }, {} /* Terminating entry */ };
commit 1796f808e4bb ("HID: i2c-hid: acpi: Stop setting wakeup_capable") changed the policy such that I2C touchpads may be able to wake up the system by default if the system is configured as such. However on Clevo NH5xAx/TUXEDO XA15 Gen10 there is a mistake in the ACPI tables that the TP_ATTN# signal connected to GPIO 10 is configured as ActiveLow and level triggered but connected to a pull up. As soon as the system suspends the touchpad loses power and then the system wakes up. To avoid this problem, introduce a quirk for this model that will prevent the wakeup capability for being set for GPIO 10. Signed-off-by: Werner Sembach <wse@tuxedocomputers.com> Cc: stable@vger.kernel.org # v6.1+ --- drivers/gpio/gpiolib-acpi.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)