Change in coreboot[master]: drivers/wifi/generic: Split wifi_generic_fill_ssdt into two functions
Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46861 )
Change subject: drivers/wifi/generic: Split wifi_generic_fill_ssdt into two functions ......................................................................
drivers/wifi/generic: Split wifi_generic_fill_ssdt into two functions
This change splits `wifi_generic_fill_ssdt()` into following two functions: 1. `wifi_ssdt_write_device()`: This function writes the device, its address, _UID and _DDN.
2. `wifi_ssdt_write_properties()`: This function writes the properties for WiFi device like _PRW, regulatory domain and SAR.
This split is done so that the device write can be skipped for CNVi devices in follow-up CLs. It will allow the SoC controller representation for CNVi PCI device to be consistent with other internal PCI devices in the device tree i.e. not requiring a chip driver for the PCI device.
Because of this change, _PRW and SAR will be seen in a separate block in SSDT disassembly, but it does not result in any functional change.
Observed difference: Before: Scope (_SB.PCI0.PBR1) { Device (WF00) { Name (_UID, 0xAA6343DC) Name (_DDN, "WIFI Device") Name (_ADR, 0x0000000000000000)
Name (_PRW, Package() { 0x08, 0x03 }) } }
After: Device (_SB.PCI0.PBR1.WF00) { Name (_UID, 0xAA6343DC) Name (_DDN, "WIFI Device") Name (_ADR, 0x0000000000000000) }
Scope (_SB.PCI0.PBR1.WF00) { Name (_PRW, Package() { 0x08, 0x03 }) }
Change-Id: I8ab5e4684492ea3b1cf749e5b9e2008e7ec8fa28 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/drivers/wifi/generic/acpi.c 1 file changed, 29 insertions(+), 16 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/61/46861/1
diff --git a/src/drivers/wifi/generic/acpi.c b/src/drivers/wifi/generic/acpi.c index f83d04c..2afd99e 100644 --- a/src/drivers/wifi/generic/acpi.c +++ b/src/drivers/wifi/generic/acpi.c @@ -161,21 +161,10 @@ acpigen_pop_len(); }
-void wifi_generic_fill_ssdt(const struct device *dev) +static void wifi_ssdt_write_device(const struct device *dev, const char *path) { - const char *path; - const struct drivers_wifi_generic_config *config = dev->chip_info; - - if (!dev->enabled) - return; - - path = acpi_device_path(dev->bus->dev); - if (!path) - return; - /* Device */ - acpigen_write_scope(path); - acpigen_write_device(acpi_device_name(dev)); + acpigen_write_device(path); acpi_device_write_uid(dev);
if (dev->chip_ops) @@ -184,6 +173,16 @@ /* Address */ acpigen_write_ADR_pci_device(dev);
+ acpigen_pop_len(); /* Device */ +} + +static void wifi_ssdt_write_properties(const struct device *dev, const char *scope) +{ + const struct drivers_wifi_generic_config *config = dev->chip_info; + + /* Scope */ + acpigen_write_scope(scope); + /* Wake capabilities */ if (config) acpigen_write_PRW(config->wake, ACPI_S3); @@ -213,11 +212,25 @@ if (CONFIG(USE_SAR)) emit_sar_acpi_structures(dev);
- acpigen_pop_len(); /* Device */ acpigen_pop_len(); /* Scope */
- printk(BIOS_INFO, "%s.%s: %s %s\n", path, acpi_device_name(dev), - dev->chip_ops ? dev->chip_ops->name : "", dev_path(dev)); + printk(BIOS_INFO, "%s: %s %s\n", scope, dev->chip_ops ? dev->chip_ops->name : "", + dev_path(dev)); +} + +void wifi_generic_fill_ssdt(const struct device *dev) +{ + const char *path; + + if (!dev->enabled) + return; + + path = acpi_device_path(dev); + if (!path) + return; + + wifi_ssdt_write_device(dev, path); + wifi_ssdt_write_properties(dev, path); }
const char *wifi_generic_acpi_name(const struct device *dev)
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46861 )
Change subject: drivers/wifi/generic: Split wifi_generic_fill_ssdt into two functions ......................................................................
Patch Set 1: Code-Review+2
(1 comment)
https://review.coreboot.org/c/coreboot/+/46861/1/src/drivers/wifi/generic/ac... File src/drivers/wifi/generic/acpi.c:
https://review.coreboot.org/c/coreboot/+/46861/1/src/drivers/wifi/generic/ac... PS1, Line 181: dev->chip_info could use config_of, but this is just reformatting so not a big deal.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46861 )
Change subject: drivers/wifi/generic: Split wifi_generic_fill_ssdt into two functions ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46861/1/src/drivers/wifi/generic/ac... File src/drivers/wifi/generic/acpi.c:
https://review.coreboot.org/c/coreboot/+/46861/1/src/drivers/wifi/generic/ac... PS1, Line 181: dev->chip_info
could use config_of, but this is just reformatting so not a big deal.
Actually, we cannot use `config_of()` because it dies if there is no config. Since this driver is traditionally used both like a chip driver and a PCI driver, we will have to keep the dev->chip_info so that we can support the platforms using the PCI driver.
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46861 )
Change subject: drivers/wifi/generic: Split wifi_generic_fill_ssdt into two functions ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46861/1/src/drivers/wifi/generic/ac... File src/drivers/wifi/generic/acpi.c:
https://review.coreboot.org/c/coreboot/+/46861/1/src/drivers/wifi/generic/ac... PS1, Line 181: dev->chip_info
Actually, we cannot use `config_of()` because it dies if there is no config. […]
That was the part I deleted because I wasn't sure.. "unless it can't handle the lack of chip_info..." as I dislike the hidden die() behavior.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46861 )
Change subject: drivers/wifi/generic: Split wifi_generic_fill_ssdt into two functions ......................................................................
Patch Set 1:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46861/1/src/drivers/wifi/generic/ac... File src/drivers/wifi/generic/acpi.c:
https://review.coreboot.org/c/coreboot/+/46861/1/src/drivers/wifi/generic/ac... PS1, Line 181: dev->chip_info
That was the part I deleted because I wasn't sure.. "unless it can't handle the lack of chip_info... […]
Yeah, I think it was done to ensure config is not NULL. But, I agree the hidden `die()` gets missed. Wonder if we need a different helper so that the callers don't need to care about accessing the internals of the device structure, but still can do their own checks.
Patrick Georgi has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46861 )
Change subject: drivers/wifi/generic: Split wifi_generic_fill_ssdt into two functions ......................................................................
drivers/wifi/generic: Split wifi_generic_fill_ssdt into two functions
This change splits `wifi_generic_fill_ssdt()` into following two functions: 1. `wifi_ssdt_write_device()`: This function writes the device, its address, _UID and _DDN.
2. `wifi_ssdt_write_properties()`: This function writes the properties for WiFi device like _PRW, regulatory domain and SAR.
This split is done so that the device write can be skipped for CNVi devices in follow-up CLs. It will allow the SoC controller representation for CNVi PCI device to be consistent with other internal PCI devices in the device tree i.e. not requiring a chip driver for the PCI device.
Because of this change, _PRW and SAR will be seen in a separate block in SSDT disassembly, but it does not result in any functional change.
Observed difference: Before: Scope (_SB.PCI0.PBR1) { Device (WF00) { Name (_UID, 0xAA6343DC) Name (_DDN, "WIFI Device") Name (_ADR, 0x0000000000000000)
Name (_PRW, Package() { 0x08, 0x03 }) } }
After: Device (_SB.PCI0.PBR1.WF00) { Name (_UID, 0xAA6343DC) Name (_DDN, "WIFI Device") Name (_ADR, 0x0000000000000000) }
Scope (_SB.PCI0.PBR1.WF00) { Name (_PRW, Package() { 0x08, 0x03 }) }
Change-Id: I8ab5e4684492ea3b1cf749e5b9e2008e7ec8fa28 Signed-off-by: Furquan Shaikh furquan@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/46861 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Duncan Laurie dlaurie@chromium.org --- M src/drivers/wifi/generic/acpi.c 1 file changed, 29 insertions(+), 16 deletions(-)
Approvals: build bot (Jenkins): Verified Duncan Laurie: Looks good to me, approved
diff --git a/src/drivers/wifi/generic/acpi.c b/src/drivers/wifi/generic/acpi.c index f83d04c..2afd99e 100644 --- a/src/drivers/wifi/generic/acpi.c +++ b/src/drivers/wifi/generic/acpi.c @@ -161,21 +161,10 @@ acpigen_pop_len(); }
-void wifi_generic_fill_ssdt(const struct device *dev) +static void wifi_ssdt_write_device(const struct device *dev, const char *path) { - const char *path; - const struct drivers_wifi_generic_config *config = dev->chip_info; - - if (!dev->enabled) - return; - - path = acpi_device_path(dev->bus->dev); - if (!path) - return; - /* Device */ - acpigen_write_scope(path); - acpigen_write_device(acpi_device_name(dev)); + acpigen_write_device(path); acpi_device_write_uid(dev);
if (dev->chip_ops) @@ -184,6 +173,16 @@ /* Address */ acpigen_write_ADR_pci_device(dev);
+ acpigen_pop_len(); /* Device */ +} + +static void wifi_ssdt_write_properties(const struct device *dev, const char *scope) +{ + const struct drivers_wifi_generic_config *config = dev->chip_info; + + /* Scope */ + acpigen_write_scope(scope); + /* Wake capabilities */ if (config) acpigen_write_PRW(config->wake, ACPI_S3); @@ -213,11 +212,25 @@ if (CONFIG(USE_SAR)) emit_sar_acpi_structures(dev);
- acpigen_pop_len(); /* Device */ acpigen_pop_len(); /* Scope */
- printk(BIOS_INFO, "%s.%s: %s %s\n", path, acpi_device_name(dev), - dev->chip_ops ? dev->chip_ops->name : "", dev_path(dev)); + printk(BIOS_INFO, "%s: %s %s\n", scope, dev->chip_ops ? dev->chip_ops->name : "", + dev_path(dev)); +} + +void wifi_generic_fill_ssdt(const struct device *dev) +{ + const char *path; + + if (!dev->enabled) + return; + + path = acpi_device_path(dev); + if (!path) + return; + + wifi_ssdt_write_device(dev, path); + wifi_ssdt_write_properties(dev, path); }
const char *wifi_generic_acpi_name(const struct device *dev)
-
Duncan Laurie (Code Review) -
Furquan Shaikh (Code Review)
-
Patrick Georgi (Code Review)