Change in ...coreboot[master]: mb/lenovo/x1_carbon_gen1: Swap Fn-F2 and Fn-F3

[Nicola Corna (Code Review)]

Nicola Corna has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31470

Change subject: mb/lenovo/x1_carbon_gen1: Swap Fn-F2 and Fn-F3
......................................................................

mb/lenovo/x1_carbon_gen1: Swap Fn-F2 and Fn-F3

thinkpad_acpi expects the battery hotkey (KEY_BATTERY) on scancode 0x01
(Fn-F2) and the lock hotkey (KEY_COFFEE) on scancode 0x02 (Fn-F3).
This is true for most of the Thinkpads, however on the X1 Carbon Gen1
(and possibly others), the battery hotkey is not present and the lock
one is instead on Fn-F3.

Swap the RHK calls in _Q11 (Fn-F2) and _Q12 (Fn-F3) to fix the issue, so
that the lock hotkey is on F3 and the battery one is on F2 (even if it's
not marked so).

Change-Id: Ib2d96be1a7815d7d03e6e8c6d300fd671c8598ca
Signed-off-by: Nicola Corna 
---
M src/ec/lenovo/h8/acpi/ec.asl
M src/mainboard/lenovo/x1_carbon_gen1/dsdt.asl
2 files changed, 9 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/31470/1

diff --git a/src/ec/lenovo/h8/acpi/ec.asl b/src/ec/lenovo/h8/acpi/ec.asl
index 327a2cf..6ffba5e 100644
--- a/src/ec/lenovo/h8/acpi/ec.asl
+++ b/src/ec/lenovo/h8/acpi/ec.asl
@@ -197,12 +197,20 @@

Method (_Q11, 0, NotSerialized)
    {
+#if IS_ENABLED(EC_LENOVO_H8_SWAP_FNF2_FNF3)
+		^HKEY.RHK (0x03)
+#else
    	^HKEY.RHK (0x02)
+#endif
    }

Method (_Q12, 0, NotSerialized)
    {
+#if IS_ENABLED(EC_LENOVO_H8_SWAP_FNF2_FNF3)
+		^HKEY.RHK (0x02)
+#else
    	^HKEY.RHK (0x03)
+#endif
    }

Method (_Q64, 0, NotSerialized)
diff --git a/src/mainboard/lenovo/x1_carbon_gen1/dsdt.asl b/src/mainboard/lenovo/x1_carbon_gen1/dsdt.asl
index 8c9bd5a..2696607 100644
--- a/src/mainboard/lenovo/x1_carbon_gen1/dsdt.asl
+++ b/src/mainboard/lenovo/x1_carbon_gen1/dsdt.asl
@@ -20,6 +20,7 @@
 #define BRIGHTNESS_DOWN _SB.PCI0.GFX0.DECB
 #define ACPI_VIDEO_DEVICE _SB.PCI0.GFX0
 #define EC_LENOVO_H8_ME_WORKAROUND 1
+#define EC_LENOVO_H8_SWAP_FNF2_FNF3 1

#include <arch/acpi.h>
 DefinitionBlock(

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31470 )

Change subject: mb/lenovo/x1_carbon_gen1: Swap Fn-F2 and Fn-F3
......................................................................

Patch Set 1:

https://review.coreboot.org/c/coreboot/+/31067 also has issues with event mapping.
I guess the best would be to move all of the _Q* methods to shared files and include those based on the EC revision / mainboard variant.

[Nicola Corna (Code Review)]

Nicola Corna has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31470 )

Change subject: mb/lenovo/x1_carbon_gen1: Swap Fn-F2 and Fn-F3
......................................................................

Patch Set 1:
> Patch Set 1:
>
> https://review.coreboot.org/c/coreboot/+/31067 also has issues with event mapping.
> I guess the best would be to move all of the _Q* methods to shared files and include those based on the EC revision / mainboard variant.

Thanks Patrick, I will have a look on that.

Change in ...coreboot[master]: ec/lenovo/h8/acpi: Add alternative Fn-F2 and Fn-F3 layout

[Nicola Corna (Code Review)]

Hello Alexander Couzens, Patrick Rudolph, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31470

to look at the new patch set (#2).

Change subject: ec/lenovo/h8/acpi: Add alternative Fn-F2 and Fn-F3 layout
......................................................................

ec/lenovo/h8/acpi: Add alternative Fn-F2 and Fn-F3 layout

thinkpad_acpi maps the battery hotkey (KEY_BATTERY) on scancode 0x01 and
the lock hotkey (KEY_COFFEE) on scancode 0x02.

On the Thinkpad X1 Carbon (and possibly others), the hotkeys for Fn-F2
and Fn-F3 are different from the default one so a new layout has to be
defined.

Change-Id: Ib2d96be1a7815d7d03e6e8c6d300fd671c8598ca
Signed-off-by: Nicola Corna 
---
M src/ec/lenovo/h8/acpi/ec.asl
M src/mainboard/lenovo/x1_carbon_gen1/dsdt.asl
2 files changed, 22 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/31470/2

Change in ...coreboot[master]: ec/lenovo/h8/acpi: Add alternative Fn-F2 and Fn-F3 layout

[Nicola Corna (Code Review)]

Nicola Corna has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31470 )

Change subject: ec/lenovo/h8/acpi: Add alternative Fn-F2 and Fn-F3 layout
......................................................................

Patch Set 2:
> Patch Set 1:
>
> https://review.coreboot.org/c/coreboot/+/31067 also has issues with event mapping.
> I guess the best would be to move all of the _Q* methods to shared files and include those based on the EC revision / mainboard variant.

The changes of this patch and of 31067 on ec.asl are quite minimal, I'm not sure if duplicating the whole _Qxx mapping on separate files is the right solution.

Change in coreboot[master]: ec/lenovo/h8/acpi: Add alternative Fn-F2 and Fn-F3 layout

[Nicola Corna (Code Review)]

Nicola Corna has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31470 )

Change subject: ec/lenovo/h8/acpi: Add alternative Fn-F2 and Fn-F3 layout
......................................................................

Patch Set 2:

Any update on this? How should I handle the different _Qxx mapping?

Change in coreboot[master]: ec/lenovo/h8/acpi: Add alternative Fn-F2 and Fn-F3 layout

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31470 )

Change subject: ec/lenovo/h8/acpi: Add alternative Fn-F2 and Fn-F3 layout
......................................................................

Patch Set 2: Code-Review+1

(1 comment)

https://review.coreboot.org/c/coreboot/+/31470/2/src/ec/lenovo/h8/acpi/ec.as... 
File src/ec/lenovo/h8/acpi/ec.asl:

https://review.coreboot.org/c/coreboot/+/31470/2/src/ec/lenovo/h8/acpi/ec.as... 
PS2, Line 207: IS_ENABLED(EC_LENOVO_H8_ALT_FN_F2F3_LAYOUT)
#ifdef

Change in coreboot[master]: ec/lenovo/h8/acpi: Add alternative Fn-F2 and Fn-F3 layout

[Nicola Corna (Code Review)]

Hello Alexander Couzens, Patrick Rudolph, build bot (Jenkins),

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/31470

to look at the new patch set (#3).

Change subject: ec/lenovo/h8/acpi: Add alternative Fn-F2 and Fn-F3 layout
......................................................................

ec/lenovo/h8/acpi: Add alternative Fn-F2 and Fn-F3 layout

thinkpad_acpi maps the battery hotkey (KEY_BATTERY) on scancode 0x01 and
the lock hotkey (KEY_COFFEE) on scancode 0x02.

On the Thinkpad X1 Carbon (and possibly others), the hotkeys for Fn-F2
and Fn-F3 are different from the default one so a new layout has to be
defined.

Change-Id: Ib2d96be1a7815d7d03e6e8c6d300fd671c8598ca
Signed-off-by: Nicola Corna 
---
M src/ec/lenovo/h8/acpi/ec.asl
M src/mainboard/lenovo/x1_carbon_gen1/dsdt.asl
2 files changed, 22 insertions(+), 0 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/70/31470/3

Change in coreboot[master]: ec/lenovo/h8/acpi: Add alternative Fn-F2 and Fn-F3 layout

[Patrick Rudolph (Code Review)]

Patrick Rudolph has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31470 )

Change subject: ec/lenovo/h8/acpi: Add alternative Fn-F2 and Fn-F3 layout
......................................................................

Patch Set 3: Code-Review+2

Change in coreboot[master]: ec/lenovo/h8/acpi: Add alternative Fn-F2 and Fn-F3 layout

[Patrick Georgi (Code Review)]

Patrick Georgi has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31470 )

Change subject: ec/lenovo/h8/acpi: Add alternative Fn-F2 and Fn-F3 layout
......................................................................

Patch Set 3:

(1 comment)

https://review.coreboot.org/c/coreboot/+/31470/2/src/ec/lenovo/h8/acpi/ec.as... 
File src/ec/lenovo/h8/acpi/ec.asl:

https://review.coreboot.org/c/coreboot/+/31470/2/src/ec/lenovo/h8/acpi/ec.as... 
PS2, Line 207: IS_ENABLED(EC_LENOVO_H8_ALT_FN_F2F3_LAYOUT)
> #ifdef

Done

Change in coreboot[master]: ec/lenovo/h8/acpi: Add alternative Fn-F2 and Fn-F3 layout

[Patrick Georgi (Code Review)]

Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/31470 )

Change subject: ec/lenovo/h8/acpi: Add alternative Fn-F2 and Fn-F3 layout
......................................................................

ec/lenovo/h8/acpi: Add alternative Fn-F2 and Fn-F3 layout

thinkpad_acpi maps the battery hotkey (KEY_BATTERY) on scancode 0x01 and
the lock hotkey (KEY_COFFEE) on scancode 0x02.

On the Thinkpad X1 Carbon (and possibly others), the hotkeys for Fn-F2
and Fn-F3 are different from the default one so a new layout has to be
defined.

Change-Id: Ib2d96be1a7815d7d03e6e8c6d300fd671c8598ca
Signed-off-by: Nicola Corna 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/31470
Tested-by: build bot (Jenkins) 
Reviewed-by: Patrick Rudolph 
---
M src/ec/lenovo/h8/acpi/ec.asl
M src/mainboard/lenovo/x1_carbon_gen1/dsdt.asl
2 files changed, 22 insertions(+), 0 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Patrick Rudolph: Looks good to me, approved

diff --git a/src/ec/lenovo/h8/acpi/ec.asl b/src/ec/lenovo/h8/acpi/ec.asl
index 327a2cf..5a11681 100644
--- a/src/ec/lenovo/h8/acpi/ec.asl
+++ b/src/ec/lenovo/h8/acpi/ec.asl
@@ -195,6 +195,26 @@
    	^HKEY.RHK (0x01)
    }

+	/*
+	 * Alternative layout (like in the Thinkpad X1 Carbon 1st generation):
+	 *  * Fn-F2 (_Q11) -> not mapped
+	 *  * Fn-F3 (_Q12) -> scancode 0x01 (KEY_COFFEE)
+	 *
+	 * Default layout (like in the Thinkpad X220):
+	 *  * Fn-F2 (_Q11) -> scancode 0x01 (KEY_COFFEE)
+	 *  * Fn-F3 (_Q12) -> scancode 0x02 (KEY_BATTERY)
+	 */
+#ifdef EC_LENOVO_H8_ALT_FN_F2F3_LAYOUT
+	Method (_Q11, 0, NotSerialized)
+	{
+		// Not mapped
+	}
+
+	Method (_Q12, 0, NotSerialized)
+	{
+		^HKEY.RHK (0x02)
+	}
+#else
    Method (_Q11, 0, NotSerialized)
    {
    	^HKEY.RHK (0x02)
@@ -204,6 +224,7 @@
    {
    	^HKEY.RHK (0x03)
    }
+#endif

Method (_Q64, 0, NotSerialized)
    {
diff --git a/src/mainboard/lenovo/x1_carbon_gen1/dsdt.asl b/src/mainboard/lenovo/x1_carbon_gen1/dsdt.asl
index de6866d..29235d7 100644
--- a/src/mainboard/lenovo/x1_carbon_gen1/dsdt.asl
+++ b/src/mainboard/lenovo/x1_carbon_gen1/dsdt.asl
@@ -20,6 +20,7 @@
 #define BRIGHTNESS_DOWN _SB.PCI0.GFX0.DECB
 #define ACPI_VIDEO_DEVICE _SB.PCI0.GFX0
 #define EC_LENOVO_H8_ME_WORKAROUND 1
+#define EC_LENOVO_H8_ALT_FN_F2F3_LAYOUT 1

#include <arch/acpi.h>
 DefinitionBlock(

Change in coreboot[master]: ec/lenovo/h8/acpi: Add alternative Fn-F2 and Fn-F3 layout

[9elements QA (Code Review)]

9elements QA has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31470 )

Change subject: ec/lenovo/h8/acpi: Add alternative Fn-F2 and Fn-F3 layout
......................................................................

Patch Set 4:

Automatic boot test returned (PASS/FAIL/TOTAL): 3/0/3
Emulation targets:
EMULATION_QEMU_X86_Q35 using payload TianoCore : SUCCESS : https://lava.9esec.io/r/664
EMULATION_QEMU_X86_Q35 using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/663
EMULATION_QEMU_X86_I440FX using payload SeaBIOS : SUCCESS : https://lava.9esec.io/r/662

Please note: This test is under development and might not be accurate at all!