Change in ...coreboot[master]: Kconfig: Add system type entries for convertible and tablet
Duncan Laurie has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/31206
Change subject: Kconfig: Add system type entries for convertible and tablet ......................................................................
Kconfig: Add system type entries for convertible and tablet
These are two common system types and in some cases it is important to know when a device is a convertible or a tablet instead of just a laptop.
This change will select the appropriate SMBIOS enclosure type based on the selected system type.
This is important for the Intel Virtual Button driver as it does a check on the SMBIOS enclosure type and only enables the tablet mode events if it is set to convertible: https://patchwork.kernel.org/patch/10236253/
Change-Id: I148ec2329a1dd38ad55c60ba277a514c66376fcc Signed-off-by: Duncan Laurie dlaurie@google.com --- M src/Kconfig 1 file changed, 13 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/31206/1
diff --git a/src/Kconfig b/src/Kconfig index a069f63..dc960eb 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -335,6 +335,14 @@ default n bool
+config SYSTEM_TYPE_TABLET + default n + bool + +config SYSTEM_TYPE_CONVERTIBLE + default n + bool + config CBFS_AUTOGEN_ATTRIBUTES default n bool @@ -654,11 +662,14 @@ hex depends on GENERATE_SMBIOS_TABLES default 0x09 if SYSTEM_TYPE_LAPTOP + default 0x1e if SYSTEM_TYPE_TABLET + default 0x1f if SYSTEM_TYPE_CONVERTIBLE default 0x03 help System Enclosure or Chassis Types as defined in SMBIOS specification. - The default value is SMBIOS_ENCLOSURE_DESKTOP (0x03) or - SMBIOS_ENCLOSURE_LAPTOP (0x09) if SYSTEM_TYPE_LAPTOP is set. + The default value is SMBIOS_ENCLOSURE_DESKTOP (0x03) but laptop, + convertible, or tablet enclosure will be used if the appropriate + system type is selected.
endmenu
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31206 )
Change subject: Kconfig: Add system type entries for convertible and tablet ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/#/c/31206/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/31206/1/src/Kconfig@334 PS1, Line 334: config SYSTEM_TYPE_LAPTOP Should this be a 'choice' block (with a fourth SYSTEM_TYPE_DESKTOP added as the default)?
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31206 )
Change subject: Kconfig: Add system type entries for convertible and tablet ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31206/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/31206/1/src/Kconfig@334 PS1, Line 334: config SYSTEM_TYPE_LAPTOP
Should this be a 'choice' block (with a fourth SYSTEM_TYPE_DESKTOP added as the default)?
I thought about that but is choice meant to have user-visible options?
I also ran into issues trying to 'select' something in the board Kconfig. There has to be an example of this somewhere..
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31206 )
Change subject: Kconfig: Add system type entries for convertible and tablet ......................................................................
Patch Set 2: Code-Review+2
(1 comment)
https://review.coreboot.org/#/c/31206/1/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/31206/1/src/Kconfig@334 PS1, Line 334: config SYSTEM_TYPE_LAPTOP
I thought about that but is choice meant to have user-visible options? […]
Okay, good point. IIRC there's no real way to put any restrictions on Kconfig 'select', so making this a choice probably wouldn't help either. So it's probably best to do it like you have here.
A way we've guaranteed mutual exclusion between 'select'able options in the past is with _Static_assert()s in C code, like in src/security/vboot/vboot_loader.c. Could do that here, although I'm not sure if it's important enough to care.
Matt Delco has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31206 )
Change subject: Kconfig: Add system type entries for convertible and tablet ......................................................................
Patch Set 2:
It might be worth also applying a change like this (though now with an additional check for tablet):
https://review.coreboot.org/c/coreboot/+/28105/1/src/arch/x86/acpi.c
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31206 )
Change subject: Kconfig: Add system type entries for convertible and tablet ......................................................................
Patch Set 2:
Patch Set 2:
It might be worth also applying a change like this (though now with an additional check for tablet):
https://review.coreboot.org/c/coreboot/+/28105/1/src/arch/x86/acpi.c
Oh I forgot you had a patch series for this.. it does seem like that check should be updated.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31206 )
Change subject: Kconfig: Add system type entries for convertible and tablet ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/#/c/31206/2/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/31206/2/src/Kconfig@665 PS2, Line 665: SYSTEM_TYPE_TABLET smbios actually seems to distinguish detachable from tablet
Hello Julius Werner, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31206
to look at the new patch set (#3).
Change subject: Kconfig: Add system type entries for common enclosures ......................................................................
Kconfig: Add system type entries for common enclosures
These are more common system types and in some cases it is important to know when a device is a convertible or a tablet or detachable instead of just a laptop. This also adds SYSTEM_TYPE_DESKTOP so it can be an explicit choice instead of just having no other type selected.
This change will select the appropriate SMBIOS enclosure type based on the selected system type.
This is important for the Intel Virtual Button driver as it does a check on the SMBIOS enclosure type and only enables the tablet mode events if it is set to convertible: https://patchwork.kernel.org/patch/10236253/
Change-Id: I148ec2329a1dd38ad55c60ba277a514c66376fcc Signed-off-by: Duncan Laurie dlaurie@google.com --- M src/Kconfig M src/arch/x86/acpi.c 2 files changed, 26 insertions(+), 5 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/31206/3
Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31206 )
Change subject: Kconfig: Add system type entries for common enclosures ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/31206/3/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/31206/3/src/Kconfig@335 PS3, Line 335: default n Shouldn't this be
default y depends on !(SYSTEM_TYPE_LAPTOP || SYSTEM_TYPE_TABLET || SYSTEM_TYPE_DETACHABLE)
because otherwise no board is selecting it?
Matt Delco has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31206 )
Change subject: Kconfig: Add system type entries for common enclosures ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/#/c/31206/3/src/arch/x86/acpi.c File src/arch/x86/acpi.c:
https://review.coreboot.org/#/c/31206/3/src/arch/x86/acpi.c@1069 PS3, Line 1069: if (IS_ENABLED(CONFIG_SYSTEM_TYPE_DESKTOP)) Afaik this approach will cause the builds for configs that don't specify a type to switch from desktop to mobile. For some configs that don't specify (e.g., eve [and any poppy0]) this is an improvement, but I'm guessing there's other configs where this wouldn't be a desirable change.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31206 )
Change subject: Kconfig: Add system type entries for common enclosures ......................................................................
Patch Set 3:
(2 comments)
https://review.coreboot.org/#/c/31206/3/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/31206/3/src/Kconfig@335 PS3, Line 335: default n
Shouldn't this be […]
I was worried that it would make some boards that are laptop end up reporting desktop (in their resulting .config) which were before in some "unknown" state by not reporting anything.
But smbios was defaulting to desktop anyway, and I suppose it is good incentive to select the system type in the board kconfig.
https://review.coreboot.org/#/c/31206/3/src/arch/x86/acpi.c File src/arch/x86/acpi.c:
https://review.coreboot.org/#/c/31206/3/src/arch/x86/acpi.c@1069 PS3, Line 1069: if (IS_ENABLED(CONFIG_SYSTEM_TYPE_DESKTOP))
Afaik this approach will cause the builds for configs that don't specify a type to switch from deskt […]
I was hoping to avoid a lot of checks but you are right...
Hello Julius Werner, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31206
to look at the new patch set (#4).
Change subject: Kconfig: Add system type entries for common enclosures ......................................................................
Kconfig: Add system type entries for common enclosures
These are more common system types and in some cases it is important to know when a device is a convertible or a tablet or detachable instead of just a laptop. This also adds SYSTEM_TYPE_DESKTOP so it can be an explicit choice instead of just having no other type selected.
This change will select the appropriate SMBIOS enclosure type based on the selected system type.
This is important for the Intel Virtual Button driver as it does a check on the SMBIOS enclosure type and only enables the tablet mode events if it is set to convertible: https://patchwork.kernel.org/patch/10236253/
Change-Id: I148ec2329a1dd38ad55c60ba277a514c66376fcc Signed-off-by: Duncan Laurie dlaurie@google.com --- M src/Kconfig M src/arch/x86/acpi.c 2 files changed, 29 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/31206/4
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31206 )
Change subject: Kconfig: Add system type entries for common enclosures ......................................................................
Patch Set 4:
(1 comment)
https://review.coreboot.org/#/c/31206/4/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/31206/4/src/Kconfig@337 PS4, Line 337: SYSTEM_TYPE_LAPTOP || SYSTEM_TYPE_TABLET) How about leaving this as `default n` like the others?
Boards that don't select anything wouldn't have a spurious setting in their .config then. And the `default 0x03` would still trigger for SMBIOS.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31206 )
Change subject: Kconfig: Add system type entries for common enclosures ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/#/c/31206/4/src/Kconfig File src/Kconfig:
https://review.coreboot.org/#/c/31206/4/src/Kconfig@337 PS4, Line 337: SYSTEM_TYPE_LAPTOP || SYSTEM_TYPE_TABLET)
How about leaving this as `default n` like the others? […]
That is where it was with patchset 3 before being changed on request.
I don't particularly care, and it might be less effort to remove the SYSTEM_TYPE_DESKTOP and go back to it being the silent default.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31206 )
Change subject: Kconfig: Add system type entries for common enclosures ......................................................................
Patch Set 5: Code-Review+2
Hello Julius Werner, build bot (Jenkins), Furquan Shaikh,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/31206
to look at the new patch set (#6).
Change subject: Kconfig: Add system type entries for common enclosures ......................................................................
Kconfig: Add system type entries for common enclosures
These are more common system types and in some cases it is important to know when a device is a convertible or a tablet or detachable instead of just a laptop.
This change will select the appropriate SMBIOS enclosure type based on the selected system type.
This is important for the Intel Virtual Button driver as it does a check on the SMBIOS enclosure type and only enables the tablet mode events if it is set to convertible: https://patchwork.kernel.org/patch/10236253/
Change-Id: I148ec2329a1dd38ad55c60ba277a514c66376fcc Signed-off-by: Duncan Laurie dlaurie@google.com --- M src/Kconfig M src/arch/x86/acpi.c 2 files changed, 22 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/06/31206/6
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31206 )
Change subject: Kconfig: Add system type entries for common enclosures ......................................................................
Patch Set 6: Code-Review+2
Paul Menzel has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31206 )
Change subject: Kconfig: Add system type entries for common enclosures ......................................................................
Patch Set 6: Code-Review+1
Duncan Laurie has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/31206 )
Change subject: Kconfig: Add system type entries for common enclosures ......................................................................
Kconfig: Add system type entries for common enclosures
These are more common system types and in some cases it is important to know when a device is a convertible or a tablet or detachable instead of just a laptop.
This change will select the appropriate SMBIOS enclosure type based on the selected system type.
This is important for the Intel Virtual Button driver as it does a check on the SMBIOS enclosure type and only enables the tablet mode events if it is set to convertible: https://patchwork.kernel.org/patch/10236253/
Change-Id: I148ec2329a1dd38ad55c60ba277a514c66376fcc Signed-off-by: Duncan Laurie dlaurie@google.com Reviewed-on: https://review.coreboot.org/c/31206 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Nico Huber nico.h@gmx.de Reviewed-by: Paul Menzel paulepanter@users.sourceforge.net --- M src/Kconfig M src/arch/x86/acpi.c 2 files changed, 22 insertions(+), 3 deletions(-)
Approvals: build bot (Jenkins): Verified Nico Huber: Looks good to me, approved Paul Menzel: Looks good to me, but someone else must approve
diff --git a/src/Kconfig b/src/Kconfig index a069f63..e527751 100644 --- a/src/Kconfig +++ b/src/Kconfig @@ -335,6 +335,18 @@ default n bool
+config SYSTEM_TYPE_TABLET + default n + bool + +config SYSTEM_TYPE_DETACHABLE + default n + bool + +config SYSTEM_TYPE_CONVERTIBLE + default n + bool + config CBFS_AUTOGEN_ATTRIBUTES default n bool @@ -654,11 +666,15 @@ hex depends on GENERATE_SMBIOS_TABLES default 0x09 if SYSTEM_TYPE_LAPTOP + default 0x1e if SYSTEM_TYPE_TABLET + default 0x1f if SYSTEM_TYPE_CONVERTIBLE + default 0x20 if SYSTEM_TYPE_DETACHABLE default 0x03 help System Enclosure or Chassis Types as defined in SMBIOS specification. - The default value is SMBIOS_ENCLOSURE_DESKTOP (0x03) or - SMBIOS_ENCLOSURE_LAPTOP (0x09) if SYSTEM_TYPE_LAPTOP is set. + The default value is SMBIOS_ENCLOSURE_DESKTOP (0x03) but laptop, + convertible, or tablet enclosure will be used if the appropriate + system type is selected.
endmenu
diff --git a/src/arch/x86/acpi.c b/src/arch/x86/acpi.c index 3b33f1b..a89b871 100644 --- a/src/arch/x86/acpi.c +++ b/src/arch/x86/acpi.c @@ -1066,7 +1066,10 @@ fadt->x_dsdt_l = (unsigned long)dsdt; fadt->x_dsdt_h = 0;
- if (IS_ENABLED(CONFIG_SYSTEM_TYPE_LAPTOP)) + if (IS_ENABLED(CONFIG_SYSTEM_TYPE_CONVERTIBLE) || + IS_ENABLED(CONFIG_SYSTEM_TYPE_DETACHABLE) || + IS_ENABLED(CONFIG_SYSTEM_TYPE_LAPTOP) || + IS_ENABLED(CONFIG_SYSTEM_TYPE_TABLET)) fadt->preferred_pm_profile = PM_MOBILE; else fadt->preferred_pm_profile = PM_DESKTOP;
HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31206 )
Change subject: Kconfig: Add system type entries for common enclosures ......................................................................
Patch Set 7:
(1 comment)
https://review.coreboot.org/#/c/31206/7/src/arch/x86/acpi.c File src/arch/x86/acpi.c:
https://review.coreboot.org/#/c/31206/7/src/arch/x86/acpi.c@1072 PS7, Line 1072: CONFIG_SYSTEM_TYPE_TABLET we have "PM_TABLET", why don't you use it ?
-
Duncan Laurie (Code Review) -
Furquan Shaikh (Code Review)
-
HAOUAS Elyes (Code Review)
-
Julius Werner (Code Review)
-
Matt Delco (Code Review)
-
Nico Huber (Code Review)
-
Paul Menzel (Code Review)