Kyösti Mälkki has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33155 )
Change subject: drivers/wifi: Add generic WiFi driver ......................................................................
Patch Set 3:
(8 comments)
https://review.coreboot.org/#/c/33155/3/src/drivers/intel/wifi/Kconfig File src/drivers/intel/wifi/Kconfig:
https://review.coreboot.org/#/c/33155/3/src/drivers/intel/wifi/Kconfig@a4 PS3, Line 4:
(out of topic): NO SENSE !!!
We've had discussions about this before, e.g. CB:31225 and decided this is the most compatible and convenient default value.
https://review.coreboot.org/#/c/33155/3/src/drivers/intel/wifi/Kconfig@3 PS3, Line 3: depends on ARCH_X86 Followup, try replace this with 'depends on PCI'. We should try to make PCI drivers to be arch-agnostic.
https://review.coreboot.org/#/c/33155/3/src/drivers/intel/wifi/wifi.c File src/drivers/intel/wifi/wifi.c:
https://review.coreboot.org/#/c/33155/3/src/drivers/intel/wifi/wifi.c@68 PS3, Line 68: struct drivers_intel_wifi_config *config = dev->chip_info; Need to test 'dev->chip != NULL' before 'config->wake'.
https://review.coreboot.org/#/c/33155/3/src/drivers/intel/wifi/wifi.c@73 PS3, Line 73: generic_wifi_fill_ssdt(dev, &generic_config); Consider passing NULL as a second argument for case without chip_info, fill_ssdt() can then skip _PRW creation.
https://review.coreboot.org/#/c/33155/3/src/drivers/intel/wifi/wifi.c@83 PS3, Line 83: val = pci_read_config16(dev, PMCS_DR); Not the scope, but is this a standard Power Management capability block? It would be better to use pci_find_capability() instead of hardcoding the register offset as it's easy to miss if the register moves from one model to another.
Logging the wake sources could/should be done inside PCI subsystem.
https://review.coreboot.org/#/c/33155/3/src/drivers/wifi/Kconfig File src/drivers/wifi/Kconfig:
https://review.coreboot.org/#/c/33155/3/src/drivers/wifi/Kconfig@9 PS3, Line 9: config USE_SAR Rest of file should go inside 'if DRIVERS_GENERIC_WIFI' block.
https://review.coreboot.org/#/c/33155/3/src/drivers/wifi/generic.c File src/drivers/wifi/generic.c:
https://review.coreboot.org/#/c/33155/3/src/drivers/wifi/generic.c@174 PS3, Line 174: if (!path || !dev->enabled) Maybe test 'dev->enabled' before you even call this function? I am not 100% sure if 'dev->bus' reference above remains valid for the '!dev->enabled' case.
https://review.coreboot.org/#/c/33155/3/src/drivers/wifi/generic.c@191 PS3, Line 191: if (config && config->wake) To use GPE0 (config->wake == 0) should be allowed. To wake from deeper sleep than S3 should be allowed too, I believe mini-PCIe specs have separate Vcc/Vstb/Vaux supply pins and could wake from S5/G2.
Maybe something like this instead:
if (config) acpigen_write_PRW(config->gpe, config->maxsleep)