Change in coreboot[master]: drivers/wifi/generic: Drop the dependency on HAVE_ACPI_TABLES
Furquan Shaikh has uploaded this change for review. ( https://review.coreboot.org/c/coreboot/+/46037 )
Change subject: drivers/wifi/generic: Drop the dependency on HAVE_ACPI_TABLES ......................................................................
drivers/wifi/generic: Drop the dependency on HAVE_ACPI_TABLES
This change drops the dependency of DRIVERS_WIFI_GENERIC on HAVE_ACPI_TABLES as the driver provides operations other than the ACPI support for WiFi devices. Since the dependency is now dropped, ACPI operations in generic.c are guarded by CONFIG(HAVE_ACPI_TABLES).
BUG=b:169802515 BRANCH=zork
Change-Id: I16444a9d842a6742e3c97ef04c4f18e93e6cdaa9 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/drivers/wifi/generic/Kconfig M src/drivers/wifi/generic/generic.c 2 files changed, 4 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/46037/1
diff --git a/src/drivers/wifi/generic/Kconfig b/src/drivers/wifi/generic/Kconfig index 049a952..ddd2be9 100644 --- a/src/drivers/wifi/generic/Kconfig +++ b/src/drivers/wifi/generic/Kconfig @@ -1,7 +1,6 @@ config DRIVERS_WIFI_GENERIC bool default n - depends on HAVE_ACPI_TABLES help When enabled, add identifiers in ACPI tables that are common to WiFi chipsets from multiple vendors. diff --git a/src/drivers/wifi/generic/generic.c b/src/drivers/wifi/generic/generic.c index b515f3c..97f5bda 100644 --- a/src/drivers/wifi/generic/generic.c +++ b/src/drivers/wifi/generic/generic.c @@ -37,6 +37,7 @@ */ #define WIFI_ACPI_NAME_MAX_LEN 5
+#if CONFIG(HAVE_ACPI_TABLES) __weak int get_wifi_sar_limits(struct wifi_sar_limits *sar_limits) { @@ -239,6 +240,7 @@ { wifi_generic_fill_ssdt(dev, dev->chip_info); } +#endif
static void wifi_pci_dev_init(struct device *dev) { @@ -287,8 +289,10 @@ .enable_resources = pci_dev_enable_resources, .init = wifi_pci_dev_init, .ops_pci = &pci_dev_ops_pci, +#if CONFIG(HAVE_ACPI_TABLES) .acpi_name = wifi_generic_acpi_name, .acpi_fill_ssdt = wifi_generic_fill_ssdt_generator, +#endif #if CONFIG(GENERATE_SMBIOS_TABLES) .get_smbios_data = smbios_write_wifi, #endif
Rob Barnes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46037 )
Change subject: drivers/wifi/generic: Drop the dependency on HAVE_ACPI_TABLES ......................................................................
Patch Set 1: Code-Review+1
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46037 )
Change subject: drivers/wifi/generic: Drop the dependency on HAVE_ACPI_TABLES ......................................................................
Patch Set 1: Code-Review+2
Hello build bot (Jenkins), Duncan Laurie, Michael Niewöhner, Rob Barnes, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46037
to look at the new patch set (#2).
Change subject: drivers/{wifi/generic,intel/wifi}: Drop the dependency on HAVE_ACPI_TABLES ......................................................................
drivers/{wifi/generic,intel/wifi}: Drop the dependency on HAVE_ACPI_TABLES
This change drops the dependency of DRIVERS_WIFI_GENERIC on HAVE_ACPI_TABLES as the driver provides operations other than the ACPI support for WiFi devices. Since the dependency is now dropped, ACPI operations in generic.c are guarded by CONFIG(HAVE_ACPI_TABLES).
BUG=b:169802515 BRANCH=zork
Change-Id: I16444a9d842a6742e3c97ef04c4f18e93e6cdaa9 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/drivers/intel/wifi/Kconfig M src/drivers/wifi/generic/Kconfig M src/drivers/wifi/generic/generic.c 3 files changed, 5 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/46037/2
Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46037 )
Change subject: drivers/{wifi/generic,intel/wifi}: Drop the dependency on HAVE_ACPI_TABLES ......................................................................
Patch Set 3: Code-Review+1
(1 comment)
https://review.coreboot.org/c/coreboot/+/46037/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46037/3//COMMIT_MSG@7 PS3, Line 7: drivers/{wifi/generic,intel/wifi}: Drop the dependency on HAVE_ACPI_TABLES keep to 72 chars, maybe drop 'the' ?
Hello build bot (Jenkins), Tim Wawrzynczak, Duncan Laurie, Michael Niewöhner, Rob Barnes, Patrick Rudolph, Karthik Ramasubramanian,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/coreboot/+/46037
to look at the new patch set (#4).
Change subject: drivers/{wifi/generic,intel/wifi}: Drop dependency on HAVE_ACPI_TABLES ......................................................................
drivers/{wifi/generic,intel/wifi}: Drop dependency on HAVE_ACPI_TABLES
This change drops the dependency of DRIVERS_WIFI_GENERIC on HAVE_ACPI_TABLES as the driver provides operations other than the ACPI support for WiFi devices. Since the dependency is now dropped, ACPI operations in generic.c are guarded by CONFIG(HAVE_ACPI_TABLES).
BUG=b:169802515 BRANCH=zork
Change-Id: I16444a9d842a6742e3c97ef04c4f18e93e6cdaa9 Signed-off-by: Furquan Shaikh furquan@google.com --- M src/drivers/intel/wifi/Kconfig M src/drivers/wifi/generic/Kconfig M src/drivers/wifi/generic/generic.c 3 files changed, 5 insertions(+), 2 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/37/46037/4
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46037 )
Change subject: drivers/{wifi/generic,intel/wifi}: Drop dependency on HAVE_ACPI_TABLES ......................................................................
Patch Set 5:
(1 comment)
https://review.coreboot.org/c/coreboot/+/46037/3//COMMIT_MSG Commit Message:
https://review.coreboot.org/c/coreboot/+/46037/3//COMMIT_MSG@7 PS3, Line 7: drivers/{wifi/generic,intel/wifi}: Drop the dependency on HAVE_ACPI_TABLES
keep to 72 chars, maybe drop 'the' ?
Done
Duncan Laurie has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46037 )
Change subject: drivers/{wifi/generic,intel/wifi}: Drop dependency on HAVE_ACPI_TABLES ......................................................................
Patch Set 6: Code-Review+2
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46037 )
Change subject: drivers/{wifi/generic,intel/wifi}: Drop dependency on HAVE_ACPI_TABLES ......................................................................
Patch Set 6: Code-Review+1
(3 comments)
https://review.coreboot.org/c/coreboot/+/46037/6/src/drivers/wifi/generic/ge... File src/drivers/wifi/generic/generic.c:
https://review.coreboot.org/c/coreboot/+/46037/6/src/drivers/wifi/generic/ge... PS6, Line 40: #if CONFIG(HAVE_ACPI_TABLES) : __weak : int get_wifi_sar_limits(struct wifi_sar_limits *sar_limits) : { : return -1; : } : : static void emit_sar_acpi_structures(void) : { : int i, j, package_size; : struct wifi_sar_limits sar_limits; : struct wifi_sar_delta_table *wgds; : : /* Retrieve the sar limits data */ : if (get_wifi_sar_limits(&sar_limits) < 0) { : printk(BIOS_ERR, "Error: failed from getting SAR limits!\n"); : return; : } : : /* : * Name ("WRDS", Package () { : * Revision, : * Package () { : * Domain Type, // 0x7:WiFi : * WiFi SAR BIOS, // BIOS SAR Enable/disable : * SAR Table Set // Set#1 of SAR Table (10 bytes) : * } : * }) : */ : acpigen_write_name("WRDS"); : acpigen_write_package(2); : acpigen_write_dword(WRDS_REVISION); : /* Emit 'Domain Type' + 'WiFi SAR BIOS' + 10 bytes for Set#1 */ : package_size = 1 + 1 + BYTES_PER_SAR_LIMIT; : acpigen_write_package(package_size); : acpigen_write_dword(WRDS_DOMAIN_TYPE_WIFI); : acpigen_write_dword(CONFIG(SAR_ENABLE)); : for (i = 0; i < BYTES_PER_SAR_LIMIT; i++) : acpigen_write_byte(sar_limits.sar_limit[0][i]); : acpigen_pop_len(); : acpigen_pop_len(); : : /* : * Name ("EWRD", Package () { : * Revision, : * Package () { : * Domain Type, // 0x7:WiFi : * Dynamic SAR Enable, // Dynamic SAR Enable/disable : * Extended SAR sets, // Number of optional SAR table sets : * SAR Table Set, // Set#2 of SAR Table (10 bytes) : * SAR Table Set, // Set#3 of SAR Table (10 bytes) : * SAR Table Set // Set#4 of SAR Table (10 bytes) : * } : * }) : */ : acpigen_write_name("EWRD"); : acpigen_write_package(2); : acpigen_write_dword(EWRD_REVISION); : /* : * Emit 'Domain Type' + "Dynamic SAR Enable' + 'Extended SAR sets' : * + number of bytes for Set#2 & 3 & 4 : */ : package_size = 1 + 1 + 1 + (NUM_SAR_LIMITS - 1) * BYTES_PER_SAR_LIMIT; : acpigen_write_package(package_size); : acpigen_write_dword(EWRD_DOMAIN_TYPE_WIFI); : acpigen_write_dword(CONFIG(DSAR_ENABLE)); : acpigen_write_dword(CONFIG_DSAR_SET_NUM); : for (i = 1; i < NUM_SAR_LIMITS; i++) : for (j = 0; j < BYTES_PER_SAR_LIMIT; j++) : acpigen_write_byte(sar_limits.sar_limit[i][j]); : acpigen_pop_len(); : acpigen_pop_len(); : : if (!CONFIG(GEO_SAR_ENABLE)) : return; : : /* : * Name ("WGDS", Package() { : * Revision, : * Package() { : * DomainType, // 0x7:WiFi : * WgdsWiFiSarDeltaGroup1PowerMax1, // Group 1 FCC 2400 Max : * WgdsWiFiSarDeltaGroup1PowerChainA1, // Group 1 FCC 2400 A Offset : * WgdsWiFiSarDeltaGroup1PowerChainB1, // Group 1 FCC 2400 B Offset : * WgdsWiFiSarDeltaGroup1PowerMax2, // Group 1 FCC 5200 Max : * WgdsWiFiSarDeltaGroup1PowerChainA2, // Group 1 FCC 5200 A Offset : * WgdsWiFiSarDeltaGroup1PowerChainB2, // Group 1 FCC 5200 B Offset : * WgdsWiFiSarDeltaGroup2PowerMax1, // Group 2 EC Jap 2400 Max : * WgdsWiFiSarDeltaGroup2PowerChainA1, // Group 2 EC Jap 2400 A Offset : * WgdsWiFiSarDeltaGroup2PowerChainB1, // Group 2 EC Jap 2400 B Offset : * WgdsWiFiSarDeltaGroup2PowerMax2, // Group 2 EC Jap 5200 Max : * WgdsWiFiSarDeltaGroup2PowerChainA2, // Group 2 EC Jap 5200 A Offset : * WgdsWiFiSarDeltaGroup2PowerChainB2, // Group 2 EC Jap 5200 B Offset : * WgdsWiFiSarDeltaGroup3PowerMax1, // Group 3 ROW 2400 Max : * WgdsWiFiSarDeltaGroup3PowerChainA1, // Group 3 ROW 2400 A Offset : * WgdsWiFiSarDeltaGroup3PowerChainB1, // Group 3 ROW 2400 B Offset : * WgdsWiFiSarDeltaGroup3PowerMax2, // Group 3 ROW 5200 Max : * WgdsWiFiSarDeltaGroup3PowerChainA2, // Group 3 ROW 5200 A Offset : * WgdsWiFiSarDeltaGroup3PowerChainB2, // Group 3 ROW 5200 B Offset : * } : * }) : */ : : wgds = &sar_limits.wgds; : acpigen_write_name("WGDS"); : acpigen_write_package(2); : acpigen_write_dword(wgds->version); : /* Emit 'Domain Type' + : * Group specific delta of power (6 bytes * NUM_WGDS_SAR_GROUPS) : */ : package_size = sizeof(sar_limits.wgds.group) + 1; : acpigen_write_package(package_size); : acpigen_write_dword(WGDS_DOMAIN_TYPE_WIFI); : for (i = 0; i < SAR_NUM_WGDS_GROUPS; i++) { : acpigen_write_byte(wgds->group[i].power_max_2400mhz); : acpigen_write_byte(wgds->group[i].power_chain_a_2400mhz); : acpigen_write_byte(wgds->group[i].power_chain_b_2400mhz); : acpigen_write_byte(wgds->group[i].power_max_5200mhz); : acpigen_write_byte(wgds->group[i].power_chain_a_5200mhz); : acpigen_write_byte(wgds->group[i].power_chain_b_5200mhz); : } : : acpigen_pop_len(); : acpigen_pop_len(); : } : : void wifi_generic_fill_ssdt(const struct device *dev, : const struct drivers_wifi_generic_config *config) : { : const char *path; : u32 address; : : 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)); : acpi_device_write_uid(dev); : : if (dev->chip_ops) : acpigen_write_name_string("_DDN", dev->chip_ops->name); : : /* Address */ : address = PCI_SLOT(dev->path.pci.devfn) & 0xffff; : address <<= 16; : address |= PCI_FUNC(dev->path.pci.devfn) & 0xffff; : acpigen_write_name_dword("_ADR", address); : : /* Wake capabilities */ : if (config) : acpigen_write_PRW(config->wake, ACPI_S3); : : /* Fill regulatory domain structure */ : if (CONFIG(HAVE_REGULATORY_DOMAIN)) { : /* : * Name ("WRDD", Package () { : * WRDD_REVISION, // Revision : * Package () { : * WRDD_DOMAIN_TYPE_WIFI, // Domain Type, 7:WiFi : * wifi_regulatory_domain() // Country Identifier : * } : * }) : */ : acpigen_write_name("WRDD"); : acpigen_write_package(2); : acpigen_write_integer(WRDD_REVISION); : acpigen_write_package(2); : acpigen_write_dword(WRDD_DOMAIN_TYPE_WIFI); : acpigen_write_dword(wifi_regulatory_domain()); : acpigen_pop_len(); : acpigen_pop_len(); : } : : /* Fill Wifi sar related ACPI structures */ : if (CONFIG(USE_SAR)) : emit_sar_acpi_structures(); : : 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)); : } : : const char *wifi_generic_acpi_name(const struct device *dev) : { : static char wifi_acpi_name[WIFI_ACPI_NAME_MAX_LEN]; : : /* ACPI 6.3, ASL 20.2.2: (Name Objects Encoding). */ : snprintf(wifi_acpi_name, sizeof(wifi_acpi_name), "WF%02X", : (dev_path_encode(dev) & 0xff)); : return wifi_acpi_name; : } : : static void wifi_generic_fill_ssdt_generator(const struct device *dev) : { : wifi_generic_fill_ssdt(dev, dev->chip_info); : } : #endif could be moved to drivers/wifi/generic/acpi.c
https://review.coreboot.org/c/coreboot/+/46037/6/src/drivers/wifi/generic/ge... PS6, Line 252: static int smbios_write_intel_wifi(struct device *dev, int *handle, unsigned long *current) : { : struct smbios_type_intel_wifi { : u8 type; : u8 length; : u16 handle; : u8 str; : u8 eos[2]; : } __packed; : : struct smbios_type_intel_wifi *t = (struct smbios_type_intel_wifi *)*current; : int len = sizeof(struct smbios_type_intel_wifi); : : memset(t, 0, sizeof(struct smbios_type_intel_wifi)); : t->type = 0x85; : t->length = len - 2; : t->handle = *handle; : /* Intel wifi driver expects this string to be in the table 0x85. */ : t->str = smbios_add_string(t->eos, "KHOIHGIUCCHHII"); : : len = t->length + smbios_string_table_len(t->eos); : *current += len; : *handle += 1; : return len; : } : : static int smbios_write_wifi(struct device *dev, int *handle, unsigned long *current) : { : if (dev->vendor == PCI_VENDOR_ID_INTEL) : return smbios_write_intel_wifi(dev, handle, current); : : return 0; : } maybe move to src/drivers/wifi/generic/smbios.c, and do like acpi below?
https://review.coreboot.org/c/coreboot/+/46037/6/src/drivers/wifi/generic/ge... PS6, Line 293: ONFIG(HAVE_ACPI_TABLES) instead of that, what about this in wifi_generic_enable()?
if(CONFIG(HAVE_ACPI_TABLES)) { wifi_generic_ops.acpi_name = .. ... }
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46037 )
Change subject: drivers/{wifi/generic,intel/wifi}: Drop dependency on HAVE_ACPI_TABLES ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/c/coreboot/+/46037/6/src/drivers/wifi/generic/ge... File src/drivers/wifi/generic/generic.c:
https://review.coreboot.org/c/coreboot/+/46037/6/src/drivers/wifi/generic/ge... PS6, Line 40: #if CONFIG(HAVE_ACPI_TABLES) : __weak : int get_wifi_sar_limits(struct wifi_sar_limits *sar_limits) : { : return -1; : } : : static void emit_sar_acpi_structures(void) : { : int i, j, package_size; : struct wifi_sar_limits sar_limits; : struct wifi_sar_delta_table *wgds; : : /* Retrieve the sar limits data */ : if (get_wifi_sar_limits(&sar_limits) < 0) { : printk(BIOS_ERR, "Error: failed from getting SAR limits!\n"); : return; : } : : /* : * Name ("WRDS", Package () { : * Revision, : * Package () { : * Domain Type, // 0x7:WiFi : * WiFi SAR BIOS, // BIOS SAR Enable/disable : * SAR Table Set // Set#1 of SAR Table (10 bytes) : * } : * }) : */ : acpigen_write_name("WRDS"); : acpigen_write_package(2); : acpigen_write_dword(WRDS_REVISION); : /* Emit 'Domain Type' + 'WiFi SAR BIOS' + 10 bytes for Set#1 */ : package_size = 1 + 1 + BYTES_PER_SAR_LIMIT; : acpigen_write_package(package_size); : acpigen_write_dword(WRDS_DOMAIN_TYPE_WIFI); : acpigen_write_dword(CONFIG(SAR_ENABLE)); : for (i = 0; i < BYTES_PER_SAR_LIMIT; i++) : acpigen_write_byte(sar_limits.sar_limit[0][i]); : acpigen_pop_len(); : acpigen_pop_len(); : : /* : * Name ("EWRD", Package () { : * Revision, : * Package () { : * Domain Type, // 0x7:WiFi : * Dynamic SAR Enable, // Dynamic SAR Enable/disable : * Extended SAR sets, // Number of optional SAR table sets : * SAR Table Set, // Set#2 of SAR Table (10 bytes) : * SAR Table Set, // Set#3 of SAR Table (10 bytes) : * SAR Table Set // Set#4 of SAR Table (10 bytes) : * } : * }) : */ : acpigen_write_name("EWRD"); : acpigen_write_package(2); : acpigen_write_dword(EWRD_REVISION); : /* : * Emit 'Domain Type' + "Dynamic SAR Enable' + 'Extended SAR sets' : * + number of bytes for Set#2 & 3 & 4 : */ : package_size = 1 + 1 + 1 + (NUM_SAR_LIMITS - 1) * BYTES_PER_SAR_LIMIT; : acpigen_write_package(package_size); : acpigen_write_dword(EWRD_DOMAIN_TYPE_WIFI); : acpigen_write_dword(CONFIG(DSAR_ENABLE)); : acpigen_write_dword(CONFIG_DSAR_SET_NUM); : for (i = 1; i < NUM_SAR_LIMITS; i++) : for (j = 0; j < BYTES_PER_SAR_LIMIT; j++) : acpigen_write_byte(sar_limits.sar_limit[i][j]); : acpigen_pop_len(); : acpigen_pop_len(); : : if (!CONFIG(GEO_SAR_ENABLE)) : return; : : /* : * Name ("WGDS", Package() { : * Revision, : * Package() { : * DomainType, // 0x7:WiFi : * WgdsWiFiSarDeltaGroup1PowerMax1, // Group 1 FCC 2400 Max : * WgdsWiFiSarDeltaGroup1PowerChainA1, // Group 1 FCC 2400 A Offset : * WgdsWiFiSarDeltaGroup1PowerChainB1, // Group 1 FCC 2400 B Offset : * WgdsWiFiSarDeltaGroup1PowerMax2, // Group 1 FCC 5200 Max : * WgdsWiFiSarDeltaGroup1PowerChainA2, // Group 1 FCC 5200 A Offset : * WgdsWiFiSarDeltaGroup1PowerChainB2, // Group 1 FCC 5200 B Offset : * WgdsWiFiSarDeltaGroup2PowerMax1, // Group 2 EC Jap 2400 Max : * WgdsWiFiSarDeltaGroup2PowerChainA1, // Group 2 EC Jap 2400 A Offset : * WgdsWiFiSarDeltaGroup2PowerChainB1, // Group 2 EC Jap 2400 B Offset : * WgdsWiFiSarDeltaGroup2PowerMax2, // Group 2 EC Jap 5200 Max : * WgdsWiFiSarDeltaGroup2PowerChainA2, // Group 2 EC Jap 5200 A Offset : * WgdsWiFiSarDeltaGroup2PowerChainB2, // Group 2 EC Jap 5200 B Offset : * WgdsWiFiSarDeltaGroup3PowerMax1, // Group 3 ROW 2400 Max : * WgdsWiFiSarDeltaGroup3PowerChainA1, // Group 3 ROW 2400 A Offset : * WgdsWiFiSarDeltaGroup3PowerChainB1, // Group 3 ROW 2400 B Offset : * WgdsWiFiSarDeltaGroup3PowerMax2, // Group 3 ROW 5200 Max : * WgdsWiFiSarDeltaGroup3PowerChainA2, // Group 3 ROW 5200 A Offset : * WgdsWiFiSarDeltaGroup3PowerChainB2, // Group 3 ROW 5200 B Offset : * } : * }) : */ : : wgds = &sar_limits.wgds; : acpigen_write_name("WGDS"); : acpigen_write_package(2); : acpigen_write_dword(wgds->version); : /* Emit 'Domain Type' + : * Group specific delta of power (6 bytes * NUM_WGDS_SAR_GROUPS) : */ : package_size = sizeof(sar_limits.wgds.group) + 1; : acpigen_write_package(package_size); : acpigen_write_dword(WGDS_DOMAIN_TYPE_WIFI); : for (i = 0; i < SAR_NUM_WGDS_GROUPS; i++) { : acpigen_write_byte(wgds->group[i].power_max_2400mhz); : acpigen_write_byte(wgds->group[i].power_chain_a_2400mhz); : acpigen_write_byte(wgds->group[i].power_chain_b_2400mhz); : acpigen_write_byte(wgds->group[i].power_max_5200mhz); : acpigen_write_byte(wgds->group[i].power_chain_a_5200mhz); : acpigen_write_byte(wgds->group[i].power_chain_b_5200mhz); : } : : acpigen_pop_len(); : acpigen_pop_len(); : } : : void wifi_generic_fill_ssdt(const struct device *dev, : const struct drivers_wifi_generic_config *config) : { : const char *path; : u32 address; : : 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)); : acpi_device_write_uid(dev); : : if (dev->chip_ops) : acpigen_write_name_string("_DDN", dev->chip_ops->name); : : /* Address */ : address = PCI_SLOT(dev->path.pci.devfn) & 0xffff; : address <<= 16; : address |= PCI_FUNC(dev->path.pci.devfn) & 0xffff; : acpigen_write_name_dword("_ADR", address); : : /* Wake capabilities */ : if (config) : acpigen_write_PRW(config->wake, ACPI_S3); : : /* Fill regulatory domain structure */ : if (CONFIG(HAVE_REGULATORY_DOMAIN)) { : /* : * Name ("WRDD", Package () { : * WRDD_REVISION, // Revision : * Package () { : * WRDD_DOMAIN_TYPE_WIFI, // Domain Type, 7:WiFi : * wifi_regulatory_domain() // Country Identifier : * } : * }) : */ : acpigen_write_name("WRDD"); : acpigen_write_package(2); : acpigen_write_integer(WRDD_REVISION); : acpigen_write_package(2); : acpigen_write_dword(WRDD_DOMAIN_TYPE_WIFI); : acpigen_write_dword(wifi_regulatory_domain()); : acpigen_pop_len(); : acpigen_pop_len(); : } : : /* Fill Wifi sar related ACPI structures */ : if (CONFIG(USE_SAR)) : emit_sar_acpi_structures(); : : 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)); : } : : const char *wifi_generic_acpi_name(const struct device *dev) : { : static char wifi_acpi_name[WIFI_ACPI_NAME_MAX_LEN]; : : /* ACPI 6.3, ASL 20.2.2: (Name Objects Encoding). */ : snprintf(wifi_acpi_name, sizeof(wifi_acpi_name), "WF%02X", : (dev_path_encode(dev) & 0xff)); : return wifi_acpi_name; : } : : static void wifi_generic_fill_ssdt_generator(const struct device *dev) : { : wifi_generic_fill_ssdt(dev, dev->chip_info); : } : #endif
could be moved to drivers/wifi/generic/acpi. […]
Yes, we can do that, but it still doesn't help with the suggestion below. This file is primarily to help generate ACPI and SMBIOS tables. So, I think we should keep these functions in this file like the other coreboot drivers.
https://review.coreboot.org/c/coreboot/+/46037/6/src/drivers/wifi/generic/ge... PS6, Line 252: static int smbios_write_intel_wifi(struct device *dev, int *handle, unsigned long *current) : { : struct smbios_type_intel_wifi { : u8 type; : u8 length; : u16 handle; : u8 str; : u8 eos[2]; : } __packed; : : struct smbios_type_intel_wifi *t = (struct smbios_type_intel_wifi *)*current; : int len = sizeof(struct smbios_type_intel_wifi); : : memset(t, 0, sizeof(struct smbios_type_intel_wifi)); : t->type = 0x85; : t->length = len - 2; : t->handle = *handle; : /* Intel wifi driver expects this string to be in the table 0x85. */ : t->str = smbios_add_string(t->eos, "KHOIHGIUCCHHII"); : : len = t->length + smbios_string_table_len(t->eos); : *current += len; : *handle += 1; : return len; : } : : static int smbios_write_wifi(struct device *dev, int *handle, unsigned long *current) : { : if (dev->vendor == PCI_VENDOR_ID_INTEL) : return smbios_write_intel_wifi(dev, handle, current); : : return 0; : }
maybe move to src/drivers/wifi/generic/smbios. […]
See comment below about ACPI tables.
https://review.coreboot.org/c/coreboot/+/46037/6/src/drivers/wifi/generic/ge... PS6, Line 293: ONFIG(HAVE_ACPI_TABLES)
instead of that, what about this in wifi_generic_enable()? […]
I don't think that will work. The compiler would complain that there is no structure member named `acpi_name`. So, you would still need #if in `wifi_generic_enable()`. Given that I don't think it helps much to move the #if from `wifi_generic_ops` to `wifi_generic_enable()`.
Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46037 )
Change subject: drivers/{wifi/generic,intel/wifi}: Drop dependency on HAVE_ACPI_TABLES ......................................................................
Patch Set 7:
(3 comments)
https://review.coreboot.org/c/coreboot/+/46037/6/src/drivers/wifi/generic/ge... File src/drivers/wifi/generic/generic.c:
https://review.coreboot.org/c/coreboot/+/46037/6/src/drivers/wifi/generic/ge... PS6, Line 40: #if CONFIG(HAVE_ACPI_TABLES) : __weak : int get_wifi_sar_limits(struct wifi_sar_limits *sar_limits) : { : return -1; : } : : static void emit_sar_acpi_structures(void) : { : int i, j, package_size; : struct wifi_sar_limits sar_limits; : struct wifi_sar_delta_table *wgds; : : /* Retrieve the sar limits data */ : if (get_wifi_sar_limits(&sar_limits) < 0) { : printk(BIOS_ERR, "Error: failed from getting SAR limits!\n"); : return; : } : : /* : * Name ("WRDS", Package () { : * Revision, : * Package () { : * Domain Type, // 0x7:WiFi : * WiFi SAR BIOS, // BIOS SAR Enable/disable : * SAR Table Set // Set#1 of SAR Table (10 bytes) : * } : * }) : */ : acpigen_write_name("WRDS"); : acpigen_write_package(2); : acpigen_write_dword(WRDS_REVISION); : /* Emit 'Domain Type' + 'WiFi SAR BIOS' + 10 bytes for Set#1 */ : package_size = 1 + 1 + BYTES_PER_SAR_LIMIT; : acpigen_write_package(package_size); : acpigen_write_dword(WRDS_DOMAIN_TYPE_WIFI); : acpigen_write_dword(CONFIG(SAR_ENABLE)); : for (i = 0; i < BYTES_PER_SAR_LIMIT; i++) : acpigen_write_byte(sar_limits.sar_limit[0][i]); : acpigen_pop_len(); : acpigen_pop_len(); : : /* : * Name ("EWRD", Package () { : * Revision, : * Package () { : * Domain Type, // 0x7:WiFi : * Dynamic SAR Enable, // Dynamic SAR Enable/disable : * Extended SAR sets, // Number of optional SAR table sets : * SAR Table Set, // Set#2 of SAR Table (10 bytes) : * SAR Table Set, // Set#3 of SAR Table (10 bytes) : * SAR Table Set // Set#4 of SAR Table (10 bytes) : * } : * }) : */ : acpigen_write_name("EWRD"); : acpigen_write_package(2); : acpigen_write_dword(EWRD_REVISION); : /* : * Emit 'Domain Type' + "Dynamic SAR Enable' + 'Extended SAR sets' : * + number of bytes for Set#2 & 3 & 4 : */ : package_size = 1 + 1 + 1 + (NUM_SAR_LIMITS - 1) * BYTES_PER_SAR_LIMIT; : acpigen_write_package(package_size); : acpigen_write_dword(EWRD_DOMAIN_TYPE_WIFI); : acpigen_write_dword(CONFIG(DSAR_ENABLE)); : acpigen_write_dword(CONFIG_DSAR_SET_NUM); : for (i = 1; i < NUM_SAR_LIMITS; i++) : for (j = 0; j < BYTES_PER_SAR_LIMIT; j++) : acpigen_write_byte(sar_limits.sar_limit[i][j]); : acpigen_pop_len(); : acpigen_pop_len(); : : if (!CONFIG(GEO_SAR_ENABLE)) : return; : : /* : * Name ("WGDS", Package() { : * Revision, : * Package() { : * DomainType, // 0x7:WiFi : * WgdsWiFiSarDeltaGroup1PowerMax1, // Group 1 FCC 2400 Max : * WgdsWiFiSarDeltaGroup1PowerChainA1, // Group 1 FCC 2400 A Offset : * WgdsWiFiSarDeltaGroup1PowerChainB1, // Group 1 FCC 2400 B Offset : * WgdsWiFiSarDeltaGroup1PowerMax2, // Group 1 FCC 5200 Max : * WgdsWiFiSarDeltaGroup1PowerChainA2, // Group 1 FCC 5200 A Offset : * WgdsWiFiSarDeltaGroup1PowerChainB2, // Group 1 FCC 5200 B Offset : * WgdsWiFiSarDeltaGroup2PowerMax1, // Group 2 EC Jap 2400 Max : * WgdsWiFiSarDeltaGroup2PowerChainA1, // Group 2 EC Jap 2400 A Offset : * WgdsWiFiSarDeltaGroup2PowerChainB1, // Group 2 EC Jap 2400 B Offset : * WgdsWiFiSarDeltaGroup2PowerMax2, // Group 2 EC Jap 5200 Max : * WgdsWiFiSarDeltaGroup2PowerChainA2, // Group 2 EC Jap 5200 A Offset : * WgdsWiFiSarDeltaGroup2PowerChainB2, // Group 2 EC Jap 5200 B Offset : * WgdsWiFiSarDeltaGroup3PowerMax1, // Group 3 ROW 2400 Max : * WgdsWiFiSarDeltaGroup3PowerChainA1, // Group 3 ROW 2400 A Offset : * WgdsWiFiSarDeltaGroup3PowerChainB1, // Group 3 ROW 2400 B Offset : * WgdsWiFiSarDeltaGroup3PowerMax2, // Group 3 ROW 5200 Max : * WgdsWiFiSarDeltaGroup3PowerChainA2, // Group 3 ROW 5200 A Offset : * WgdsWiFiSarDeltaGroup3PowerChainB2, // Group 3 ROW 5200 B Offset : * } : * }) : */ : : wgds = &sar_limits.wgds; : acpigen_write_name("WGDS"); : acpigen_write_package(2); : acpigen_write_dword(wgds->version); : /* Emit 'Domain Type' + : * Group specific delta of power (6 bytes * NUM_WGDS_SAR_GROUPS) : */ : package_size = sizeof(sar_limits.wgds.group) + 1; : acpigen_write_package(package_size); : acpigen_write_dword(WGDS_DOMAIN_TYPE_WIFI); : for (i = 0; i < SAR_NUM_WGDS_GROUPS; i++) { : acpigen_write_byte(wgds->group[i].power_max_2400mhz); : acpigen_write_byte(wgds->group[i].power_chain_a_2400mhz); : acpigen_write_byte(wgds->group[i].power_chain_b_2400mhz); : acpigen_write_byte(wgds->group[i].power_max_5200mhz); : acpigen_write_byte(wgds->group[i].power_chain_a_5200mhz); : acpigen_write_byte(wgds->group[i].power_chain_b_5200mhz); : } : : acpigen_pop_len(); : acpigen_pop_len(); : } : : void wifi_generic_fill_ssdt(const struct device *dev, : const struct drivers_wifi_generic_config *config) : { : const char *path; : u32 address; : : 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)); : acpi_device_write_uid(dev); : : if (dev->chip_ops) : acpigen_write_name_string("_DDN", dev->chip_ops->name); : : /* Address */ : address = PCI_SLOT(dev->path.pci.devfn) & 0xffff; : address <<= 16; : address |= PCI_FUNC(dev->path.pci.devfn) & 0xffff; : acpigen_write_name_dword("_ADR", address); : : /* Wake capabilities */ : if (config) : acpigen_write_PRW(config->wake, ACPI_S3); : : /* Fill regulatory domain structure */ : if (CONFIG(HAVE_REGULATORY_DOMAIN)) { : /* : * Name ("WRDD", Package () { : * WRDD_REVISION, // Revision : * Package () { : * WRDD_DOMAIN_TYPE_WIFI, // Domain Type, 7:WiFi : * wifi_regulatory_domain() // Country Identifier : * } : * }) : */ : acpigen_write_name("WRDD"); : acpigen_write_package(2); : acpigen_write_integer(WRDD_REVISION); : acpigen_write_package(2); : acpigen_write_dword(WRDD_DOMAIN_TYPE_WIFI); : acpigen_write_dword(wifi_regulatory_domain()); : acpigen_pop_len(); : acpigen_pop_len(); : } : : /* Fill Wifi sar related ACPI structures */ : if (CONFIG(USE_SAR)) : emit_sar_acpi_structures(); : : 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)); : } : : const char *wifi_generic_acpi_name(const struct device *dev) : { : static char wifi_acpi_name[WIFI_ACPI_NAME_MAX_LEN]; : : /* ACPI 6.3, ASL 20.2.2: (Name Objects Encoding). */ : snprintf(wifi_acpi_name, sizeof(wifi_acpi_name), "WF%02X", : (dev_path_encode(dev) & 0xff)); : return wifi_acpi_name; : } : : static void wifi_generic_fill_ssdt_generator(const struct device *dev) : { : wifi_generic_fill_ssdt(dev, dev->chip_info); : } : #endif
Yes, we can do that, but it still doesn't help with the suggestion below. […]
Done
https://review.coreboot.org/c/coreboot/+/46037/6/src/drivers/wifi/generic/ge... PS6, Line 252: static int smbios_write_intel_wifi(struct device *dev, int *handle, unsigned long *current) : { : struct smbios_type_intel_wifi { : u8 type; : u8 length; : u16 handle; : u8 str; : u8 eos[2]; : } __packed; : : struct smbios_type_intel_wifi *t = (struct smbios_type_intel_wifi *)*current; : int len = sizeof(struct smbios_type_intel_wifi); : : memset(t, 0, sizeof(struct smbios_type_intel_wifi)); : t->type = 0x85; : t->length = len - 2; : t->handle = *handle; : /* Intel wifi driver expects this string to be in the table 0x85. */ : t->str = smbios_add_string(t->eos, "KHOIHGIUCCHHII"); : : len = t->length + smbios_string_table_len(t->eos); : *current += len; : *handle += 1; : return len; : } : : static int smbios_write_wifi(struct device *dev, int *handle, unsigned long *current) : { : if (dev->vendor == PCI_VENDOR_ID_INTEL) : return smbios_write_intel_wifi(dev, handle, current); : : return 0; : }
See comment below about ACPI tables.
Done
https://review.coreboot.org/c/coreboot/+/46037/6/src/drivers/wifi/generic/ge... PS6, Line 293: ONFIG(HAVE_ACPI_TABLES)
I don't think that will work. […]
Done
Michael Niewöhner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/46037 )
Change subject: drivers/{wifi/generic,intel/wifi}: Drop dependency on HAVE_ACPI_TABLES ......................................................................
Patch Set 7: Code-Review+2
Furquan Shaikh has submitted this change. ( https://review.coreboot.org/c/coreboot/+/46037 )
Change subject: drivers/{wifi/generic,intel/wifi}: Drop dependency on HAVE_ACPI_TABLES ......................................................................
drivers/{wifi/generic,intel/wifi}: Drop dependency on HAVE_ACPI_TABLES
This change drops the dependency of DRIVERS_WIFI_GENERIC on HAVE_ACPI_TABLES as the driver provides operations other than the ACPI support for WiFi devices. Since the dependency is now dropped, ACPI operations in generic.c are guarded by CONFIG(HAVE_ACPI_TABLES).
BUG=b:169802515 BRANCH=zork
Change-Id: I16444a9d842a6742e3c97ef04c4f18e93e6cdaa9 Signed-off-by: Furquan Shaikh furquan@google.com Reviewed-on: https://review.coreboot.org/c/coreboot/+/46037 Reviewed-by: Michael Niewöhner foss@mniewoehner.de Reviewed-by: Duncan Laurie dlaurie@chromium.org Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M src/drivers/intel/wifi/Kconfig M src/drivers/wifi/generic/Kconfig M src/drivers/wifi/generic/generic.c 3 files changed, 5 insertions(+), 2 deletions(-)
Approvals: build bot (Jenkins): Verified Duncan Laurie: Looks good to me, approved Michael Niewöhner: Looks good to me, approved
diff --git a/src/drivers/intel/wifi/Kconfig b/src/drivers/intel/wifi/Kconfig index 83df822..5454e97 100644 --- a/src/drivers/intel/wifi/Kconfig +++ b/src/drivers/intel/wifi/Kconfig @@ -2,7 +2,7 @@ bool "Support Intel PCI-e WiFi adapters" depends on PCI default y if PCIEXP_PLUGIN_SUPPORT - select DRIVERS_WIFI_GENERIC if HAVE_ACPI_TABLES + select DRIVERS_WIFI_GENERIC help When enabled, add identifiers in ACPI and SMBIOS tables to make OS drivers work with certain Intel PCI-e WiFi chipsets. diff --git a/src/drivers/wifi/generic/Kconfig b/src/drivers/wifi/generic/Kconfig index 049a952..ddd2be9 100644 --- a/src/drivers/wifi/generic/Kconfig +++ b/src/drivers/wifi/generic/Kconfig @@ -1,7 +1,6 @@ config DRIVERS_WIFI_GENERIC bool default n - depends on HAVE_ACPI_TABLES help When enabled, add identifiers in ACPI tables that are common to WiFi chipsets from multiple vendors. diff --git a/src/drivers/wifi/generic/generic.c b/src/drivers/wifi/generic/generic.c index e551bf3..bb36861 100644 --- a/src/drivers/wifi/generic/generic.c +++ b/src/drivers/wifi/generic/generic.c @@ -37,6 +37,7 @@ */ #define WIFI_ACPI_NAME_MAX_LEN 5
+#if CONFIG(HAVE_ACPI_TABLES) __weak int get_wifi_sar_limits(struct wifi_sar_limits *sar_limits) { @@ -239,6 +240,7 @@ { wifi_generic_fill_ssdt(dev, dev->chip_info); } +#endif
static void wifi_pci_dev_init(struct device *dev) { @@ -288,8 +290,10 @@ .enable_resources = pci_dev_enable_resources, .init = wifi_pci_dev_init, .ops_pci = &pci_dev_ops_pci, +#if CONFIG(HAVE_ACPI_TABLES) .acpi_name = wifi_generic_acpi_name, .acpi_fill_ssdt = wifi_generic_fill_ssdt_generator, +#endif #if CONFIG(GENERATE_SMBIOS_TABLES) .get_smbios_data = smbios_write_wifi, #endif
-
Duncan Laurie (Code Review) -
Furquan Shaikh (Code Review)
-
Michael Niewöhner (Code Review)
-
Rob Barnes (Code Review)
-
Tim Wawrzynczak (Code Review)