Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46033 )
Change subject: drivers/wifi: Drop maxsleep parameter from chip config ......................................................................
drivers/wifi: Drop maxsleep parameter from chip config
This change drops maxsleep parameter from chip config and instead hardcodes the deepest sleep state from which the WiFi device can wake the system up from to SLP_TYP_S3. This is similar to how other device drivers in coreboot report _PRW property in ACPI. It relieves the users from adding another register attribute to devicetree since all mainboards configure the same value. If this changes in the future, it should be easy to bring the maxsleep config parameter back.
BUG=b:169802515 BRANCH=zork
Change-Id: I42131fced008da0d51f0f777b7f2d99deaf68827 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/drivers/intel/wifi/wifi.c M src/drivers/wifi/generic/chip.h M src/drivers/wifi/generic/generic.c M src/mainboard/google/dedede/variants/madoo/overridetree.cb M src/mainboard/google/dedede/variants/magolor/overridetree.cb M src/mainboard/google/dedede/variants/waddledee/overridetree.cb 6 files changed, 3 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/46033/1
diff --git a/src/drivers/intel/wifi/wifi.c b/src/drivers/intel/wifi/wifi.c index f768b04..6010673 100644 --- a/src/drivers/intel/wifi/wifi.c +++ b/src/drivers/intel/wifi/wifi.c @@ -49,11 +49,9 @@ struct drivers_intel_wifi_config *config = dev->chip_info; struct drivers_wifi_generic_config generic_config;
- if (config) { + if (config) generic_config.wake = config->wake; - /* By default, all intel wifi chips wake from S3 */ - generic_config.maxsleep = 3; - } + wifi_generic_fill_ssdt(dev, config ? &generic_config : NULL); } #endif diff --git a/src/drivers/wifi/generic/chip.h b/src/drivers/wifi/generic/chip.h index fe3a1d1..02ab504 100644 --- a/src/drivers/wifi/generic/chip.h +++ b/src/drivers/wifi/generic/chip.h @@ -6,11 +6,9 @@ /** * struct drivers_wifi_generic_config - Data structure to contain generic wifi config * @wake: Wake pin for ACPI _PRW - * @maxsleep: Maximum sleep state to wake from */ struct drivers_wifi_generic_config { unsigned int wake; - unsigned int maxsleep; };
/** diff --git a/src/drivers/wifi/generic/generic.c b/src/drivers/wifi/generic/generic.c index 059185d..903afdc 100644 --- a/src/drivers/wifi/generic/generic.c +++ b/src/drivers/wifi/generic/generic.c @@ -189,7 +189,7 @@
/* Wake capabilities */ if (config) - acpigen_write_PRW(config->wake, config->maxsleep); + acpigen_write_PRW(config->wake, SLP_TYP_S3);
/* Fill regulatory domain structure */ if (CONFIG(HAVE_REGULATORY_DOMAIN)) { diff --git a/src/mainboard/google/dedede/variants/madoo/overridetree.cb b/src/mainboard/google/dedede/variants/madoo/overridetree.cb index 039fd10..257f917 100644 --- a/src/mainboard/google/dedede/variants/madoo/overridetree.cb +++ b/src/mainboard/google/dedede/variants/madoo/overridetree.cb @@ -103,7 +103,6 @@ device pci 1c.7 on chip drivers/wifi/generic register "wake" = "GPE0_DW2_03" - register "maxsleep" = "3" device pci 00.0 on end end end # PCI Express Root Port 8 - WLAN diff --git a/src/mainboard/google/dedede/variants/magolor/overridetree.cb b/src/mainboard/google/dedede/variants/magolor/overridetree.cb index 8a83b83..f41e9fa 100644 --- a/src/mainboard/google/dedede/variants/magolor/overridetree.cb +++ b/src/mainboard/google/dedede/variants/magolor/overridetree.cb @@ -287,7 +287,6 @@ device pci 1c.7 on chip drivers/wifi/generic register "wake" = "GPE0_DW2_03" - register "maxsleep" = "3" device pci 00.0 on end end end # PCI Express Root Port 8 - WLAN diff --git a/src/mainboard/google/dedede/variants/waddledee/overridetree.cb b/src/mainboard/google/dedede/variants/waddledee/overridetree.cb index a525a0f..22a08a0 100644 --- a/src/mainboard/google/dedede/variants/waddledee/overridetree.cb +++ b/src/mainboard/google/dedede/variants/waddledee/overridetree.cb @@ -147,7 +147,6 @@ device pci 1c.7 on chip drivers/wifi/generic register "wake" = "GPE0_DW2_03" - register "maxsleep" = "3" device pci 00.0 on end end end # PCI Express Root Port 8 - WLAN
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46033 )
Change subject: drivers/wifi: Drop maxsleep parameter from chip config ......................................................................
Patch Set 1: Code-Review+1
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46033 )
Change subject: drivers/wifi: Drop maxsleep parameter from chip config ......................................................................
Patch Set 1: Code-Review+2
in theory with vpro you can wake from S5 but I don't think the wake source is the wifi device in that case anyway.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46033 )
Change subject: drivers/wifi: Drop maxsleep parameter from chip config ......................................................................
Patch Set 1:
(1 comment)
Patch Set 1: Code-Review+2
in theory with vpro you can wake from S5 but I don't think the wake source is the wifi device in that case anyway.
That's right.
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/wifi/generic/ge... File src/drivers/wifi/generic/generic.c:
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/wifi/generic/ge... PS1, Line 192: SLP_TYP_S3 Looks like this definition is not provided by all platforms using this driver. I will have to add a change prior to this to ensure that SLP_TYP_S3 is defined always.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46033 )
Change subject: drivers/wifi: Drop maxsleep parameter from chip config ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/intel/wifi/wifi... File src/drivers/intel/wifi/wifi.c:
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/intel/wifi/wifi... PS1, Line 54: By default, all intel wifi chips wake from S3 What about CNVi? I just checked the vendor ACPI of clevo/l140cu, where this is 0x04.
Scope (_SB.PCI0) { Device (CNVW) { ... Method (_PRW, 0, NotSerialized) { Return (GPRW (0x6D, 0x04)) } ...
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46033 )
Change subject: drivers/wifi: Drop maxsleep parameter from chip config ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/intel/wifi/wifi... File src/drivers/intel/wifi/wifi.c:
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/intel/wifi/wifi... PS1, Line 54: By default, all intel wifi chips wake from S3
What about CNVi? I just checked the vendor ACPI of clevo/l140cu, where this is 0x04. […]
SLP_TYP_S3 is 5 for Intel hardware. I don't think 4 is a valid value. Even for CNVi, this should still be 5 i.e. SLP_TYP_S3. That is what the boards in coreboot currently use. So, this change does not really change the logic.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46033 )
Change subject: drivers/wifi: Drop maxsleep parameter from chip config ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/intel/wifi/wifi... File src/drivers/intel/wifi/wifi.c:
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/intel/wifi/wifi... PS1, Line 54: By default, all intel wifi chips wake from S3
SLP_TYP_S3 is 5 for Intel hardware. I don't think 4 is a valid value. […]
My bad. I mixed up the use of PM1_CNT SLP_TYP and ACPI Sx. _PRW only cares about ACPI Sx (we actually have some usages in coreboot that are using SLP_TYP_S3 instead of ACPI_S3 which need to be fixed as follow-ups). Anyways, if a mainboard shows up in coreboot using CNVi/PCIe Intel WiFi and requires maxsleep as S5, it should be easy to bring this maxsleep parameter back. For now, I will keep this as is and can revisit later when required.
Hello build bot (Jenkins), Duncan Laurie, Rob Barnes, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46033
to look at the new patch set (#2).
Change subject: drivers/wifi: Drop maxsleep parameter from chip config ......................................................................
drivers/wifi: Drop maxsleep parameter from chip config
This change drops maxsleep parameter from chip config and instead hardcodes the deepest sleep state from which the WiFi device can wake the system up from to SLP_TYP_S3. This is similar to how other device drivers in coreboot report _PRW property in ACPI. It relieves the users from adding another register attribute to devicetree since all mainboards configure the same value. If this changes in the future, it should be easy to bring the maxsleep config parameter back.
BUG=b:169802515 BRANCH=zork
Change-Id: I42131fced008da0d51f0f777b7f2d99deaf68827 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/drivers/intel/wifi/wifi.c M src/drivers/wifi/generic/chip.h M src/drivers/wifi/generic/generic.c M src/mainboard/google/dedede/variants/madoo/overridetree.cb M src/mainboard/google/dedede/variants/magolor/overridetree.cb M src/mainboard/google/dedede/variants/waddledee/overridetree.cb 6 files changed, 3 insertions(+), 10 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/33/46033/2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46033 )
Change subject: drivers/wifi: Drop maxsleep parameter from chip config ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/wifi/generic/ge... File src/drivers/wifi/generic/generic.c:
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/wifi/generic/ge... PS1, Line 192: SLP_TYP_S3
Looks like this definition is not provided by all platforms using this driver. […]
Done
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46033 )
Change subject: drivers/wifi: Drop maxsleep parameter from chip config ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/intel/wifi/wifi... File src/drivers/intel/wifi/wifi.c:
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/intel/wifi/wifi... PS1, Line 54: By default, all intel wifi chips wake from S3
if a mainboard shows up in coreboot using CNVi/PCIe Intel WiFi
That's why I mentioned clevo/l140, which is in the tree already.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46033 )
Change subject: drivers/wifi: Drop maxsleep parameter from chip config ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/intel/wifi/wifi... File src/drivers/intel/wifi/wifi.c:
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/intel/wifi/wifi... PS1, Line 54: By default, all intel wifi chips wake from S3
I don't think 4 is a valid value
4 is mentioned for S4 in this intel doc on page 83: https://www.intel.com/content/dam/www/public/us/en/documents/articles/acpi-c...
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46033 )
Change subject: drivers/wifi: Drop maxsleep parameter from chip config ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/intel/wifi/wifi... File src/drivers/intel/wifi/wifi.c:
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/intel/wifi/wifi... PS1, Line 54: By default, all intel wifi chips wake from S3
I don't think 4 is a valid value […]
Hmm, the PCH datasheets (100, 300 series, cml-u) don't mention 4 as a valid value... strange
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46033 )
Change subject: drivers/wifi: Drop maxsleep parameter from chip config ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/intel/wifi/wifi... File src/drivers/intel/wifi/wifi.c:
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/intel/wifi/wifi... PS1, Line 54: By default, all intel wifi chips wake from S3
Hmm, the PCH datasheets (100, 300 series, cml-u) don't mention 4 as a valid value... […]
If you are looking at the PM1_CNT register, then 4 is not a valid value.
Also, do you know if the mainboard is designed to support wake from wifi(cnvi) in S5?
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46033 )
Change subject: drivers/wifi: Drop maxsleep parameter from chip config ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/intel/wifi/wifi... File src/drivers/intel/wifi/wifi.c:
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/intel/wifi/wifi... PS1, Line 54: By default, all intel wifi chips wake from S3
If you are looking at the PM1_CNT register, then 4 is not a valid value.
Yeah, however, Intel datasheets tend to be very wrong sometimes
Also, do you know if the mainboard is designed to support wake from wifi(cnvi) in S5?
I'm not sure about the requirements
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46033 )
Change subject: drivers/wifi: Drop maxsleep parameter from chip config ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/intel/wifi/wifi... File src/drivers/intel/wifi/wifi.c:
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/intel/wifi/wifi... PS1, Line 54: By default, all intel wifi chips wake from S3
I'm not sure about the requirements
It depends on the hardware board design.
In my opinion, let's take this as a follow-up. The current Intel driver was assuming maxsleep as S3. If we have to support this in the generic driver, we can always add it back - default as S3 unless provided by mainboard.
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46033 )
Change subject: drivers/wifi: Drop maxsleep parameter from chip config ......................................................................
Patch Set 2: Code-Review+2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46033 )
Change subject: drivers/wifi: Drop maxsleep parameter from chip config ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/intel/wifi/wifi... File src/drivers/intel/wifi/wifi.c:
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/intel/wifi/wifi... PS1, Line 54: By default, all intel wifi chips wake from S3
I'm not sure about the requirements […]
Redefining some of these as enums instead of a collection of #defines may help with that.
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46033 )
Change subject: drivers/wifi: Drop maxsleep parameter from chip config ......................................................................
Patch Set 2: Code-Review+2
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46033 )
Change subject: drivers/wifi: Drop maxsleep parameter from chip config ......................................................................
Patch Set 2:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/intel/wifi/wifi... File src/drivers/intel/wifi/wifi.c:
https://review.coreboot.org/c/coreboot/+/46033/1/src/drivers/intel/wifi/wifi... PS1, Line 54: By default, all intel wifi chips wake from S3
Redefining some of these as enums instead of a collection of #defines may help with that.
Agreed. That would help make things less confusing.
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46033 )
Change subject: drivers/wifi: Drop maxsleep parameter from chip config ......................................................................
drivers/wifi: Drop maxsleep parameter from chip config
This change drops maxsleep parameter from chip config and instead hardcodes the deepest sleep state from which the WiFi device can wake the system up from to SLP_TYP_S3. This is similar to how other device drivers in coreboot report _PRW property in ACPI. It relieves the users from adding another register attribute to devicetree since all mainboards configure the same value. If this changes in the future, it should be easy to bring the maxsleep config parameter back.
BUG=b:169802515 BRANCH=zork
Change-Id: I42131fced008da0d51f0f777b7f2d99deaf68827 Signed-off-by: Furquan Shaikh furquan@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/46033 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Michael Niewöhner foss@mniewoehner.de Reviewed-by: Tim Wawrzynczak twawrzynczak@chromium.org --- M src/drivers/intel/wifi/wifi.c M src/drivers/wifi/generic/chip.h M src/drivers/wifi/generic/generic.c M src/mainboard/google/dedede/variants/madoo/overridetree.cb M src/mainboard/google/dedede/variants/magolor/overridetree.cb M src/mainboard/google/dedede/variants/waddledee/overridetree.cb 6 files changed, 3 insertions(+), 10 deletions(-)
Approvals: build bot (Jenkins): Verified Tim Wawrzynczak: Looks good to me, approved Michael Niewöhner: Looks good to me, approved
diff --git a/src/drivers/intel/wifi/wifi.c b/src/drivers/intel/wifi/wifi.c index cc58ac6..3c90dde 100644 --- a/src/drivers/intel/wifi/wifi.c +++ b/src/drivers/intel/wifi/wifi.c @@ -49,11 +49,9 @@ struct drivers_intel_wifi_config *config = dev->chip_info; struct drivers_wifi_generic_config generic_config;
- if (config) { + if (config) generic_config.wake = config->wake; - /* By default, all intel wifi chips wake from S3 */ - generic_config.maxsleep = 3; - } + wifi_generic_fill_ssdt(dev, config ? &generic_config : NULL); } #endif diff --git a/src/drivers/wifi/generic/chip.h b/src/drivers/wifi/generic/chip.h index fe3a1d1..02ab504 100644 --- a/src/drivers/wifi/generic/chip.h +++ b/src/drivers/wifi/generic/chip.h @@ -6,11 +6,9 @@ /** * struct drivers_wifi_generic_config - Data structure to contain generic wifi config * @wake: Wake pin for ACPI _PRW - * @maxsleep: Maximum sleep state to wake from */ struct drivers_wifi_generic_config { unsigned int wake; - unsigned int maxsleep; };
/** diff --git a/src/drivers/wifi/generic/generic.c b/src/drivers/wifi/generic/generic.c index 2ecaadc..0705731 100644 --- a/src/drivers/wifi/generic/generic.c +++ b/src/drivers/wifi/generic/generic.c @@ -189,7 +189,7 @@
/* Wake capabilities */ if (config) - acpigen_write_PRW(config->wake, config->maxsleep); + acpigen_write_PRW(config->wake, ACPI_S3);
/* Fill regulatory domain structure */ if (CONFIG(HAVE_REGULATORY_DOMAIN)) { diff --git a/src/mainboard/google/dedede/variants/madoo/overridetree.cb b/src/mainboard/google/dedede/variants/madoo/overridetree.cb index 039fd10..257f917 100644 --- a/src/mainboard/google/dedede/variants/madoo/overridetree.cb +++ b/src/mainboard/google/dedede/variants/madoo/overridetree.cb @@ -103,7 +103,6 @@ device pci 1c.7 on chip drivers/wifi/generic register "wake" = "GPE0_DW2_03" - register "maxsleep" = "3" device pci 00.0 on end end end # PCI Express Root Port 8 - WLAN diff --git a/src/mainboard/google/dedede/variants/magolor/overridetree.cb b/src/mainboard/google/dedede/variants/magolor/overridetree.cb index 8a83b83..f41e9fa 100644 --- a/src/mainboard/google/dedede/variants/magolor/overridetree.cb +++ b/src/mainboard/google/dedede/variants/magolor/overridetree.cb @@ -287,7 +287,6 @@ device pci 1c.7 on chip drivers/wifi/generic register "wake" = "GPE0_DW2_03" - register "maxsleep" = "3" device pci 00.0 on end end end # PCI Express Root Port 8 - WLAN diff --git a/src/mainboard/google/dedede/variants/waddledee/overridetree.cb b/src/mainboard/google/dedede/variants/waddledee/overridetree.cb index a525a0f..22a08a0 100644 --- a/src/mainboard/google/dedede/variants/waddledee/overridetree.cb +++ b/src/mainboard/google/dedede/variants/waddledee/overridetree.cb @@ -147,7 +147,6 @@ device pci 1c.7 on chip drivers/wifi/generic register "wake" = "GPE0_DW2_03" - register "maxsleep" = "3" device pci 00.0 on end end end # PCI Express Root Port 8 - WLAN
-
Duncan Laurie (Code Review) -
Furquan Shaikh (Code Review)
-
Michael Niewöhner (Code Review)
-
Rob Barnes (Code Review)
-
Tim Wawrzynczak (Code Review)