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()`.