Change in ...coreboot[master]: drivers/wifi: Add generic WiFi driver

[Karthik Ramasubramanian (Code Review)]

Hello Karthikeyan Ramasubramanian,

I'd like you to do a code review. Please visit

https://review.coreboot.org/c/coreboot/+/33155

to review the following change.

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

drivers/wifi: Add generic WiFi driver

Add generic WiFi driver to support common device operations across
multiple types of WiFi controller.

BUG=None
BRANCH=None
TEST=Boot to ChromeOS. Ensure that the SSDT table contains SAR tables
and wakeup GPE information. Ensure that the SSDT table is same after the
change.

Change-Id: Ica5edf95a37c8ed60f7e159d94fd58af5d41c0ef
Signed-off-by: Karthikeyan Ramasubramanian 
---
M src/drivers/intel/wifi/Kconfig
M src/drivers/intel/wifi/chip.h
M src/drivers/intel/wifi/wifi.c
A src/drivers/wifi/Kconfig
A src/drivers/wifi/Makefile.inc
A src/drivers/wifi/generic.c
A src/drivers/wifi/generic_wifi.h
7 files changed, 295 insertions(+), 206 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/33155/1

diff --git a/src/drivers/intel/wifi/Kconfig b/src/drivers/intel/wifi/Kconfig
index 4dc4d7f..7cf9391 100644
--- a/src/drivers/intel/wifi/Kconfig
+++ b/src/drivers/intel/wifi/Kconfig
@@ -2,6 +2,7 @@
    bool "Support Intel PCI-e WiFi adapters"
    depends on ARCH_X86
    default y if PCIEXP_PLUGIN_SUPPORT
+	select DRIVERS_GENERIC_WIFI if HAVE_ACPI_TABLES
    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/intel/wifi/chip.h b/src/drivers/intel/wifi/chip.h
index 1b3c2a5..72ff951 100644
--- a/src/drivers/intel/wifi/chip.h
+++ b/src/drivers/intel/wifi/chip.h
@@ -13,26 +13,13 @@
  * GNU General Public License for more details.
  */

-#ifndef _WIFI_CHIP_H_
-#define _WIFI_CHIP_H_
+#ifndef _INTEL_WIFI_CHIP_H_
+#define _INTEL_WIFI_CHIP_H_

-/* WRDS Spec Revision */
-#define WRDS_REVISION 0x0
-
-/* EWRD Spec Revision */
-#define EWRD_REVISION 0x0
-
-/* WRDS Domain type */
-#define WRDS_DOMAIN_TYPE_WIFI 0x7
-
-/* EWRD Domain type */
-#define EWRD_DOMAIN_TYPE_WIFI 0x7
-
-/* WGDS Domain type */
-#define WGDS_DOMAIN_TYPE_WIFI 0x7
+#include "drivers/wifi/generic_wifi.h"

struct drivers_intel_wifi_config {
-	unsigned wake; /* Wake pin for ACPI _PRW */
+	unsigned int wake; /* Wake pin for ACPI _PRW */
 };

-#endif /* _WIFI_CHIP_H_ */
+#endif /* _INTEL_WIFI_CHIP_H_ */
diff --git a/src/drivers/intel/wifi/wifi.c b/src/drivers/intel/wifi/wifi.c
index 83cfaa9..546c925 100644
--- a/src/drivers/intel/wifi/wifi.c
+++ b/src/drivers/intel/wifi/wifi.c
@@ -15,19 +15,16 @@
  * GNU General Public License for more details.
  */

-#include <arch/acpi_device.h>
-#include <arch/acpigen.h>
 #include <console/console.h>
 #include <device/device.h>
 #include <device/pci.h>
 #include <device/pci_ops.h>
 #include <device/pci_ids.h>
 #include <elog.h>
-#include <sar.h>
 #include <smbios.h>
 #include <string.h>
-#include <wrdd.h>
 #include "chip.h"
+#include "drivers/wifi/generic_wifi.h"

#define PMCS_DR 0xcc
 #define PME_STS (1 << 15)
@@ -65,194 +62,15 @@
 }
 #endif

-__weak
-int get_wifi_sar_limits(struct wifi_sar_limits *sar_limits)
-{
-	return -1;
-}
-
 #if CONFIG(HAVE_ACPI_TABLES)
-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();
-}
-
 static void intel_wifi_fill_ssdt(struct device *dev)
 {
    struct drivers_intel_wifi_config *config = dev->chip_info;
-	const char *path = acpi_device_path(dev->bus->dev);
-	u32 address;
+	struct generic_wifi_config generic_config = {
+		.wake = config->wake,
+	};

-	if (!path || !dev->enabled)
-		return;
-
-	/* Device */
-	acpigen_write_scope(path);
-	acpigen_write_device(acpi_device_name(dev));
-	acpigen_write_name_integer("_UID", 0);
-	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 && config->wake)
-		acpigen_write_PRW(config->wake, 3);
-
-	/* 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));
-}
-
-static const char *intel_wifi_acpi_name(const struct device *dev)
-{
-	return "WIFI";
+	generic_wifi_fill_ssdt(dev, &generic_config);
 }
 #endif

@@ -282,7 +100,7 @@
 #endif
    .ops_pci                  = &pci_ops,
 #if CONFIG(HAVE_ACPI_TABLES)
-	.acpi_name                = intel_wifi_acpi_name,
+	.acpi_name                = generic_wifi_acpi_name,
    .acpi_fill_ssdt_generator = intel_wifi_fill_ssdt,
 #endif
 };
diff --git a/src/drivers/wifi/Kconfig b/src/drivers/wifi/Kconfig
new file mode 100644
index 0000000..966dc55
--- /dev/null
+++ b/src/drivers/wifi/Kconfig
@@ -0,0 +1,4 @@
+config DRIVERS_GENERIC_WIFI
+	bool
+	default n
+	depends on HAVE_ACPI_TABLES
diff --git a/src/drivers/wifi/Makefile.inc b/src/drivers/wifi/Makefile.inc
new file mode 100644
index 0000000..d37015c
--- /dev/null
+++ b/src/drivers/wifi/Makefile.inc
@@ -0,0 +1 @@
+ramstage-$(CONFIG_DRIVERS_GENERIC_WIFI) += generic.c
diff --git a/src/drivers/wifi/generic.c b/src/drivers/wifi/generic.c
new file mode 100644
index 0000000..acf1e45
--- /dev/null
+++ b/src/drivers/wifi/generic.c
@@ -0,0 +1,229 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * Copyright 2019 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 or (at your option)
+ * any later version of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <arch/acpi_device.h>
+#include <arch/acpigen.h>
+#include <console/console.h>
+#include <device/device.h>
+#include <device/pci_def.h>
+#include <sar.h>
+#include <string.h>
+#include <wrdd.h>
+#include "generic_wifi.h"
+
+/* WRDS Spec Revision */
+#define WRDS_REVISION 0x0
+
+/* EWRD Spec Revision */
+#define EWRD_REVISION 0x0
+
+/* WRDS Domain type */
+#define WRDS_DOMAIN_TYPE_WIFI 0x7
+
+/* EWRD Domain type */
+#define EWRD_DOMAIN_TYPE_WIFI 0x7
+
+/* WGDS Domain type */
+#define WGDS_DOMAIN_TYPE_WIFI 0x7
+
+__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 generic_wifi_fill_ssdt(struct device *dev,
+			    struct generic_wifi_config *config)
+{
+	const char *path = acpi_device_path(dev->bus->dev);
+	u32 address;
+
+	if (!path || !dev->enabled)
+		return;
+
+	/* Device */
+	acpigen_write_scope(path);
+	acpigen_write_device(acpi_device_name(dev));
+	acpigen_write_name_integer("_UID", 0);
+	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 && config->wake)
+		acpigen_write_PRW(config->wake, 3);
+
+	/* 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 *generic_wifi_acpi_name(const struct device *dev)
+{
+	return "WIFI";
+}
diff --git a/src/drivers/wifi/generic_wifi.h b/src/drivers/wifi/generic_wifi.h
new file mode 100644
index 0000000..fdcf66a
--- /dev/null
+++ b/src/drivers/wifi/generic_wifi.h
@@ -0,0 +1,49 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * Copyright 2019 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _GENERIC_WIFI_H_
+#define _GENERIC_WIFI_H_
+
+/**
+ * struct generic_wifi_config - Data structure to contain common wifi config
+ * @wake: Wake pin for ACPI _PRW
+ */
+struct generic_wifi_config {
+	unsigned int wake;
+};
+
+/**
+ * wifi_fill_ssdt() - Fill ACPI SSDT table for WiFi controller
+ * @dev: Device structure corresponding to WiFi controller.
+ * @config: Common wifi config required to fill ACPI SSDT table.
+ *
+ * This function implements common device operation to help fill ACPI SSDT
+ * table for WiFi controller.
+ */
+void generic_wifi_fill_ssdt(struct device *dev,
+			    struct generic_wifi_config *config);
+
+/**
+ * wifi_acpi_name() - Get ACPI name for WiFi controller
+ * @dev: Device structure corresponding to WiFi controller.
+ *
+ * This function implements common device operation to get the ACPI name for
+ * WiFi controller.
+ *
+ * Return: string representing the ACPI name for WiFi controller.
+ */
+const char *generic_wifi_acpi_name(const struct device *dev);
+
+#endif /* _GENERIC_WIFI_H_ */

[Justin TerAvest (Code Review)]

Justin TerAvest has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33155 )

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/33155/1/src/drivers/wifi/Kconfig 
File src/drivers/wifi/Kconfig:

https://review.coreboot.org/#/c/33155/1/src/drivers/wifi/Kconfig@5 
PS1, Line 5: 
Can you add some help text?

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33155 )

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

Patch Set 1:

(1 comment)

https://review.coreboot.org/#/c/33155/1/src/drivers/wifi/Kconfig 
File src/drivers/wifi/Kconfig:

https://review.coreboot.org/#/c/33155/1/src/drivers/wifi/Kconfig@5 
PS1, Line 5:
> Can you add some help text?

Sure, will do.

[Karthik Ramasubramanian (Code Review)]

Hello Karthikeyan Ramasubramanian, Justin TerAvest, build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/33155

to look at the new patch set (#2).

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

drivers/wifi: Add generic WiFi driver

Add generic WiFi driver to support common device operations across
multiple types of WiFi controller.

BUG=None
BRANCH=None
TEST=Boot to ChromeOS. Ensure that the SSDT table contains SAR tables
and wakeup GPE information. Ensure that the SSDT table is same after the
change.

Change-Id: Ica5edf95a37c8ed60f7e159d94fd58af5d41c0ef
Signed-off-by: Karthikeyan Ramasubramanian 
---
M src/drivers/intel/wifi/Kconfig
M src/drivers/intel/wifi/chip.h
M src/drivers/intel/wifi/wifi.c
A src/drivers/wifi/Kconfig
A src/drivers/wifi/Makefile.inc
A src/drivers/wifi/generic.c
A src/drivers/wifi/generic_wifi.h
7 files changed, 298 insertions(+), 206 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/33155/2

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33155 )

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

Patch Set 2: Code-Review+1

(2 comments)

https://review.coreboot.org/#/c/33155/2/src/drivers/intel/wifi/Kconfig 
File src/drivers/intel/wifi/Kconfig:

https://review.coreboot.org/#/c/33155/2/src/drivers/intel/wifi/Kconfig@10 
PS2, Line 10: config USE_SAR
Just curious: Do the SAR configs need to be moved to the common Kconfig eventually?

https://review.coreboot.org/#/c/33155/2/src/drivers/intel/wifi/wifi.c 
File src/drivers/intel/wifi/wifi.c:

https://review.coreboot.org/#/c/33155/2/src/drivers/intel/wifi/wifi.c@69 
PS2, Line 69: struct
const?

[Karthik Ramasubramanian (Code Review)]

Hello Karthikeyan Ramasubramanian, Justin TerAvest, build bot (Jenkins), Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/33155

to look at the new patch set (#3).

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

drivers/wifi: Add generic WiFi driver

Add generic WiFi driver to support common device operations across
multiple types of WiFi controller.

BUG=None
BRANCH=None
TEST=Boot to ChromeOS. Ensure that the SSDT table contains SAR tables
and wakeup GPE information. Ensure that the SSDT table is same after the
change.

Change-Id: Ica5edf95a37c8ed60f7e159d94fd58af5d41c0ef
Signed-off-by: Karthikeyan Ramasubramanian 
---
M src/drivers/intel/wifi/Kconfig
M src/drivers/intel/wifi/chip.h
M src/drivers/intel/wifi/wifi.c
A src/drivers/wifi/Kconfig
A src/drivers/wifi/Makefile.inc
A src/drivers/wifi/generic.c
A src/drivers/wifi/generic_wifi.h
7 files changed, 344 insertions(+), 252 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/33155/3

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33155 )

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

Patch Set 3:

(2 comments)

https://review.coreboot.org/#/c/33155/2/src/drivers/intel/wifi/Kconfig 
File src/drivers/intel/wifi/Kconfig:

https://review.coreboot.org/#/c/33155/2/src/drivers/intel/wifi/Kconfig@10 
PS2, Line 10: config USE_SAR
> Just curious: Do the SAR configs need to be moved to the common Kconfig eventually?

Done

https://review.coreboot.org/#/c/33155/2/src/drivers/intel/wifi/wifi.c 
File src/drivers/intel/wifi/wifi.c:

https://review.coreboot.org/#/c/33155/2/src/drivers/intel/wifi/wifi.c@69 
PS2, Line 69: struct
> const?

Done

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33155 )

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

Patch Set 3:

(1 comment)

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 !!!

[Kyösti Mälkki (Code Review)]

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)

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33155 )

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

Patch Set 3:

(7 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@3 
PS3, Line 3: 	depends on ARCH_X86
> Followup, try replace this with 'depends on PCI'. […]

I will upload a follow-up patch on that one.

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'.

Ok.

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 _PR […]

Ok.

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 p […]

I need to check on this further and update on this comment later.

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.

Ok.

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' refer […]

Ok.

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. […]

Ok.

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33155 )

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

Patch Set 3: Code-Review+1

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has removed a vote on this change.

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

Removed Code-Review+1 by HAOUAS Elyes

[HAOUAS Elyes (Code Review)]

HAOUAS Elyes has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33155 )

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

Patch Set 3: Code-Review+1

[Karthik Ramasubramanian (Code Review)]

Hello Karthikeyan Ramasubramanian, HAOUAS Elyes, Justin TerAvest, build bot (Jenkins), Nico Huber, Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/33155

to look at the new patch set (#4).

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

drivers/wifi: Add generic WiFi driver

Add generic WiFi driver to support common device operations across
multiple types of WiFi controller.

BUG=None
BRANCH=None
TEST=Boot to ChromeOS. Ensure that the SSDT table contains SAR tables
and wakeup GPE information. Ensure that the SSDT table is same after the
change.

Change-Id: Ica5edf95a37c8ed60f7e159d94fd58af5d41c0ef
Signed-off-by: Karthikeyan Ramasubramanian 
---
M src/drivers/intel/wifi/Kconfig
M src/drivers/intel/wifi/chip.h
M src/drivers/intel/wifi/wifi.c
A src/drivers/wifi/Kconfig
A src/drivers/wifi/Makefile.inc
A src/drivers/wifi/generic.c
A src/drivers/wifi/generic_wifi.h
7 files changed, 356 insertions(+), 251 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/33155/4

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33155 )

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

Patch Set 4:

(6 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@3 
PS3, Line 3: 	depends on ARCH_X86
> I will upload a follow-up patch on that one.

Done

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;
> Ok.

Done

https://review.coreboot.org/#/c/33155/3/src/drivers/intel/wifi/wifi.c@73 
PS3, Line 73: 	generic_wifi_fill_ssdt(dev, &generic_config);
> Ok.

Done

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
> Ok.

Done

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)
> Ok.

Added the check here so that we don't have to add it redundantly every place where this function is invoked.

https://review.coreboot.org/#/c/33155/3/src/drivers/wifi/generic.c@191 
PS3, Line 191: 	if (config && config->wake)
> Ok.

Done

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33155 )

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

Patch Set 4:

(1 comment)

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)
> Added the check here so that we don't have to add it redundantly every place where this function is  […]

Done

[Kyösti Mälkki (Code Review)]

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 4:

(3 comments)

https://review.coreboot.org/#/c/33155/4/src/drivers/intel/wifi/chip.h 
File src/drivers/intel/wifi/chip.h:

https://review.coreboot.org/#/c/33155/4/src/drivers/intel/wifi/chip.h@19 
PS4, Line 19: #include "drivers/wifi/generic_wifi.h"
Why?

https://review.coreboot.org/#/c/33155/4/src/drivers/wifi/generic.c 
File src/drivers/wifi/generic.c:

https://review.coreboot.org/#/c/33155/4/src/drivers/wifi/generic.c@230 
PS4, Line 230: const char *generic_wifi_acpi_name(const struct device *dev)
No we need unique names in case of multiple devices present?

https://review.coreboot.org/#/c/33155/4/src/drivers/wifi/generic_wifi.h 
File src/drivers/wifi/generic_wifi.h:

https://review.coreboot.org/#/c/33155/4/src/drivers/wifi/generic_wifi.h@25 
PS4, Line 25: 	unsigned int wake;
The name could be better than 'wake' but that is easy fix for followup if we decide so.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33155 )

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

Patch Set 4:

(3 comments)

https://review.coreboot.org/#/c/33155/4/src/drivers/intel/wifi/chip.h 
File src/drivers/intel/wifi/chip.h:

https://review.coreboot.org/#/c/33155/4/src/drivers/intel/wifi/chip.h@19 
PS4, Line 19: #include "drivers/wifi/generic_wifi.h"
> Why?

+1. This should not be required.

https://review.coreboot.org/#/c/33155/4/src/drivers/wifi/generic.c 
File src/drivers/wifi/generic.c:

https://review.coreboot.org/#/c/33155/4/src/drivers/wifi/generic.c@195 
PS4, Line 195: )
Was the check for !config->wake dropped intentionally?

https://review.coreboot.org/#/c/33155/4/src/drivers/wifi/generic.c@230 
PS4, Line 230: const char *generic_wifi_acpi_name(const struct device *dev)
> No we need unique names in case of multiple devices present?

That's an interesting thought. We haven't seen multiple devices for WiFi, but that doesn't mean it will remain the same in the future. Should this be addressed when there is a need for it?

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33155 )

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/#/c/33155/4/src/drivers/intel/wifi/chip.h 
File src/drivers/intel/wifi/chip.h:

https://review.coreboot.org/#/c/33155/4/src/drivers/intel/wifi/chip.h@19 
PS4, Line 19: #include "drivers/wifi/generic_wifi.h"
> +1. This should not be required.

Sure will remove it.

https://review.coreboot.org/#/c/33155/4/src/drivers/wifi/generic.c 
File src/drivers/wifi/generic.c:

https://review.coreboot.org/#/c/33155/4/src/drivers/wifi/generic.c@195 
PS4, Line 195: )
> Was the check for !config->wake dropped intentionally?

In one of the previous comments, GPE0 can be a wake up source which can take the value of 0. Hence it was dropped.

[Kyösti Mälkki (Code Review)]

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 4:

(2 comments)

https://review.coreboot.org/#/c/33155/4/src/drivers/wifi/generic.c 
File src/drivers/wifi/generic.c:

https://review.coreboot.org/#/c/33155/4/src/drivers/wifi/generic.c@195 
PS4, Line 195: )
> Was the check for !config->wake dropped intentionally?

See patchset #3.

https://review.coreboot.org/#/c/33155/4/src/drivers/wifi/generic.c@230 
PS4, Line 230: const char *generic_wifi_acpi_name(const struct device *dev)
> That's an interesting thought. […]

I guess the worst thing that happens is ACPI interpreter throws more runtime errors in kernel logs or the choice of OS you have made gives you cryptic BSOD...

If duplicate name is ACPI violation, we should fix it here and now. Not my thing, ASL, so I don't know what relevance this has.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33155 )

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

Patch Set 4:

(2 comments)

https://review.coreboot.org/#/c/33155/4/src/drivers/wifi/generic.c 
File src/drivers/wifi/generic.c:

https://review.coreboot.org/#/c/33155/4/src/drivers/wifi/generic.c@195 
PS4, Line 195: )
> See patchset #3.

Okay. Makes sense. I was just worried that there might be some device which uses wake = 0 to indicate no wake from wifi. But from a quick grep, it doesn't look like there is any such device in coreboot.

https://review.coreboot.org/#/c/33155/4/src/drivers/wifi/generic.c@230 
PS4, Line 230: const char *generic_wifi_acpi_name(const struct device *dev)
> I guess the worst thing that happens is ACPI interpreter throws more runtime errors in kernel logs o […]

It would be a violation only if the same name appears under the same namescope and path. I am fine either ways -- doing it now or later when there really is a need for it.

[Kyösti Mälkki (Code Review)]

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 4:

(1 comment)

https://review.coreboot.org/#/c/33155/4/src/drivers/wifi/generic.c 
File src/drivers/wifi/generic.c:

https://review.coreboot.org/#/c/33155/4/src/drivers/wifi/generic.c@195 
PS4, Line 195: )
> Okay. Makes sense. […]

One can then expose .maxsleep in intel/wifi/chip.h to have this no-wake feature (explicitly set maxsleep=3 to enable). Currently fine as-is.

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33155 )

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

Patch Set 4:

(1 comment)

https://review.coreboot.org/#/c/33155/4/src/drivers/wifi/generic.c 
File src/drivers/wifi/generic.c:

https://review.coreboot.org/#/c/33155/4/src/drivers/wifi/generic.c@195 
PS4, Line 195: )
> One can then expose .maxsleep in intel/wifi/chip. […]

SG.

Change in coreboot[master]: drivers/wifi: Add generic WiFi driver

[Karthik Ramasubramanian (Code Review)]

Hello Karthikeyan Ramasubramanian, HAOUAS Elyes, Justin TerAvest, build bot (Jenkins), Nico Huber, Furquan Shaikh,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/33155

to look at the new patch set (#5).

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

drivers/wifi: Add generic WiFi driver

Add generic WiFi driver to support common device operations across
multiple types of WiFi controller.

BUG=None
BRANCH=None
TEST=Boot to ChromeOS. Ensure that the SSDT table contains SAR tables
and wakeup GPE information. Ensure that the SSDT table is same after the
change.

Change-Id: Ica5edf95a37c8ed60f7e159d94fd58af5d41c0ef
Signed-off-by: Karthikeyan Ramasubramanian 
---
M src/drivers/intel/wifi/Kconfig
M src/drivers/intel/wifi/chip.h
M src/drivers/intel/wifi/wifi.c
A src/drivers/wifi/Kconfig
A src/drivers/wifi/Makefile.inc
A src/drivers/wifi/generic.c
A src/drivers/wifi/generic_wifi.h
7 files changed, 366 insertions(+), 252 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/33155/5

Change in coreboot[master]: drivers/wifi: Add generic WiFi driver

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33155 )

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

Patch Set 5:

(2 comments)

https://review.coreboot.org/c/coreboot/+/33155/4/src/drivers/intel/wifi/chip... 
File src/drivers/intel/wifi/chip.h:

https://review.coreboot.org/c/coreboot/+/33155/4/src/drivers/intel/wifi/chip... 
PS4, Line 19: #include "drivers/wifi/generic_wifi.h"
> Sure will remove it.

Done

https://review.coreboot.org/c/coreboot/+/33155/4/src/drivers/wifi/generic.c 
File src/drivers/wifi/generic.c:

https://review.coreboot.org/c/coreboot/+/33155/4/src/drivers/wifi/generic.c@... 
PS4, Line 230: const char *generic_wifi_acpi_name(const struct device *dev)
> It would be a violation only if the same name appears under the same namescope and path. […]

Done

Change in coreboot[master]: drivers/wifi: Add generic WiFi driver

[Kyösti Mälkki (Code Review)]

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 5: Code-Review+1

Change in coreboot[master]: drivers/wifi: Add generic WiFi driver

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33155 )

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

Patch Set 5:

(2 comments)

https://review.coreboot.org/c/coreboot/+/33155/5/src/drivers/wifi/generic.c 
File src/drivers/wifi/generic.c:

https://review.coreboot.org/c/coreboot/+/33155/5/src/drivers/wifi/generic.c@... 
PS5, Line 49: __weak
            : int get_wifi_sar_limits(struct wifi_sar_limits *sar_limits)
            : {
            : 	return -1;
            : }
Not for this change, but I am wondering if we should get rid of this weak definition. If USE_SAR is selected by a board, it should ensure that get_wifi_sar_limits is provided as well.

https://review.coreboot.org/c/coreboot/+/33155/5/src/drivers/wifi/generic.c@... 
PS5, Line 241: "WIFI%08x",
Device name can be max 4 characters long. Even if you pass in bigger string here, it will get clipped as part of acpigen_emit_simple_namestring

Change in coreboot[master]: drivers/wifi: Add generic WiFi driver

[Karthik Ramasubramanian (Code Review)]

Hello Kyösti Mälkki, Patrick Rudolph, Karthikeyan Ramasubramanian, HAOUAS Elyes, Justin TerAvest, build bot (Jenkins), Nico Huber, Furquan Shaikh, Patrick Georgi, Martin Roth,

I'd like you to reexamine a change. Please visit

https://review.coreboot.org/c/coreboot/+/33155

to look at the new patch set (#6).

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

drivers/wifi: Add generic WiFi driver

Add generic WiFi driver to support common device operations across
multiple types of WiFi controller.

BUG=None
BRANCH=None
TEST=Boot to ChromeOS. Ensure that the SSDT table contains SAR tables
and wakeup GPE information. Ensure that the SSDT table is same after the
change.

Change-Id: Ica5edf95a37c8ed60f7e159d94fd58af5d41c0ef
Signed-off-by: Karthikeyan Ramasubramanian 
---
M src/drivers/intel/wifi/Kconfig
M src/drivers/intel/wifi/chip.h
M src/drivers/intel/wifi/wifi.c
A src/drivers/wifi/Kconfig
A src/drivers/wifi/Makefile.inc
A src/drivers/wifi/generic.c
A src/drivers/wifi/generic_wifi.h
7 files changed, 367 insertions(+), 252 deletions(-)

git pull ssh://review.coreboot.org:29418/coreboot refs/changes/55/33155/6

Change in coreboot[master]: drivers/wifi: Add generic WiFi driver

[Karthik Ramasubramanian (Code Review)]

Karthik Ramasubramanian has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33155 )

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

Patch Set 6:

(2 comments)

https://review.coreboot.org/c/coreboot/+/33155/5/src/drivers/wifi/generic.c 
File src/drivers/wifi/generic.c:

https://review.coreboot.org/c/coreboot/+/33155/5/src/drivers/wifi/generic.c@... 
PS5, Line 49: __weak
            : int get_wifi_sar_limits(struct wifi_sar_limits *sar_limits)
            : {
            : 	return -1;
            : }
> Not for this change, but I am wondering if we should get rid of this weak definition. […]

Sure, will look into it as a follow-up to this change.

https://review.coreboot.org/c/coreboot/+/33155/5/src/drivers/wifi/generic.c@... 
PS5, Line 241: "WIFI%08x",
> Device name can be max 4 characters long. […]

Done

Change in coreboot[master]: drivers/wifi: Add generic WiFi driver

[Furquan Shaikh (Code Review)]

Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/33155 )

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

Patch Set 6: Code-Review+2

Change in coreboot[master]: drivers/wifi: Add generic WiFi driver

[Martin Roth (Code Review)]

Martin Roth has submitted this change and it was merged. ( https://review.coreboot.org/c/coreboot/+/33155 )

Change subject: drivers/wifi: Add generic WiFi driver
......................................................................

drivers/wifi: Add generic WiFi driver

Add generic WiFi driver to support common device operations across
multiple types of WiFi controller.

BUG=None
BRANCH=None
TEST=Boot to ChromeOS. Ensure that the SSDT table contains SAR tables
and wakeup GPE information. Ensure that the SSDT table is same after the
change.

Change-Id: Ica5edf95a37c8ed60f7e159d94fd58af5d41c0ef
Signed-off-by: Karthikeyan Ramasubramanian 
Reviewed-on: https://review.coreboot.org/c/coreboot/+/33155
Tested-by: build bot (Jenkins) 
Reviewed-by: Furquan Shaikh 
---
M src/drivers/intel/wifi/Kconfig
M src/drivers/intel/wifi/chip.h
M src/drivers/intel/wifi/wifi.c
A src/drivers/wifi/Kconfig
A src/drivers/wifi/Makefile.inc
A src/drivers/wifi/generic.c
A src/drivers/wifi/generic_wifi.h
7 files changed, 367 insertions(+), 252 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Furquan Shaikh: Looks good to me, approved

diff --git a/src/drivers/intel/wifi/Kconfig b/src/drivers/intel/wifi/Kconfig
index 4dc4d7f..fc8700f 100644
--- a/src/drivers/intel/wifi/Kconfig
+++ b/src/drivers/intel/wifi/Kconfig
@@ -2,52 +2,7 @@
    bool "Support Intel PCI-e WiFi adapters"
    depends on ARCH_X86
    default y if PCIEXP_PLUGIN_SUPPORT
+	select DRIVERS_GENERIC_WIFI if HAVE_ACPI_TABLES
    help
       When enabled, add identifiers in ACPI and SMBIOS tables to
       make OS drivers work with certain Intel PCI-e WiFi chipsets.
-
-config USE_SAR
-	bool
-	default n
-	help
-	  Enable it when wifi driver uses SAR configuration feature.
-	  VPD entry "wifi_sar" is read to get SAR settings, if its
-	  not found driver may look into CBFS for default settigs.
-	  WIFI_SAR_CBFS is option to enable CBFS lookup.
-
-config SAR_ENABLE
-	bool
-	default n
-	depends on USE_SAR
-
-config DSAR_ENABLE
-	bool
-	default n
-	depends on USE_SAR
-
-config GEO_SAR_ENABLE
-	bool
-	default n
-	depends on USE_SAR
-
-config WIFI_SAR_CBFS
-	bool "Enable SAR table addition to CBFS"
-	default n
-	depends on USE_SAR
-	help
-	 wifi driver would look for "wifi_sar" vpd key and load SAR settings from
-	 it, if the vpd key is not found then the driver tries to look for sar
-	 settings from CBFS with file name wifi_sar_defaults.hex.
-	 So OEM/ODM can override wifi sar with VPD.
-
-config WIFI_SAR_CBFS_FILEPATH
-	string "The cbfs file which has WIFI SAR defaults"
-	depends on WIFI_SAR_CBFS
-	default "src/mainboard/$(MAINBOARDDIR)/wifi_sar_defaults.hex"
-
-config DSAR_SET_NUM
-	hex "Number of SAR sets when D-SAR is enabled"
-	default 0x3
-	depends on USE_SAR
-	help
-	  There can be up to 3 optional SAR table sets.
diff --git a/src/drivers/intel/wifi/chip.h b/src/drivers/intel/wifi/chip.h
index 1b3c2a5..2180310 100644
--- a/src/drivers/intel/wifi/chip.h
+++ b/src/drivers/intel/wifi/chip.h
@@ -13,26 +13,11 @@
  * GNU General Public License for more details.
  */

-#ifndef _WIFI_CHIP_H_
-#define _WIFI_CHIP_H_
-
-/* WRDS Spec Revision */
-#define WRDS_REVISION 0x0
-
-/* EWRD Spec Revision */
-#define EWRD_REVISION 0x0
-
-/* WRDS Domain type */
-#define WRDS_DOMAIN_TYPE_WIFI 0x7
-
-/* EWRD Domain type */
-#define EWRD_DOMAIN_TYPE_WIFI 0x7
-
-/* WGDS Domain type */
-#define WGDS_DOMAIN_TYPE_WIFI 0x7
+#ifndef _INTEL_WIFI_CHIP_H_
+#define _INTEL_WIFI_CHIP_H_

struct drivers_intel_wifi_config {
-	unsigned wake; /* Wake pin for ACPI _PRW */
+	unsigned int wake; /* Wake pin for ACPI _PRW */
 };

-#endif /* _WIFI_CHIP_H_ */
+#endif /* _INTEL_WIFI_CHIP_H_ */
diff --git a/src/drivers/intel/wifi/wifi.c b/src/drivers/intel/wifi/wifi.c
index 83cfaa9..926905c 100644
--- a/src/drivers/intel/wifi/wifi.c
+++ b/src/drivers/intel/wifi/wifi.c
@@ -15,19 +15,16 @@
  * GNU General Public License for more details.
  */

-#include <arch/acpi_device.h>
-#include <arch/acpigen.h>
 #include <console/console.h>
 #include <device/device.h>
 #include <device/pci.h>
 #include <device/pci_ops.h>
 #include <device/pci_ids.h>
 #include <elog.h>
-#include <sar.h>
 #include <smbios.h>
 #include <string.h>
-#include <wrdd.h>
 #include "chip.h"
+#include "drivers/wifi/generic_wifi.h"

#define PMCS_DR 0xcc
 #define PME_STS (1 << 15)
@@ -65,194 +62,18 @@
 }
 #endif

-__weak
-int get_wifi_sar_limits(struct wifi_sar_limits *sar_limits)
-{
-	return -1;
-}
-
 #if CONFIG(HAVE_ACPI_TABLES)
-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();
-}
-
 static void intel_wifi_fill_ssdt(struct device *dev)
 {
    struct drivers_intel_wifi_config *config = dev->chip_info;
-	const char *path = acpi_device_path(dev->bus->dev);
-	u32 address;
+	struct generic_wifi_config generic_config;

-	if (!path || !dev->enabled)
-		return;
-
-	/* Device */
-	acpigen_write_scope(path);
-	acpigen_write_device(acpi_device_name(dev));
-	acpigen_write_name_integer("_UID", 0);
-	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 && config->wake)
-		acpigen_write_PRW(config->wake, 3);
-
-	/* 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();
+	if (config) {
+		generic_config.wake = config->wake;
+		/* By default, all intel wifi chips wake from S3 */
+		generic_config.maxsleep = 3;
    }
-
-	/* 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));
-}
-
-static const char *intel_wifi_acpi_name(const struct device *dev)
-{
-	return "WIFI";
+	generic_wifi_fill_ssdt(dev, config ? &generic_config : NULL);
 }
 #endif

@@ -282,7 +103,7 @@
 #endif
    .ops_pci                  = &pci_ops,
 #if CONFIG(HAVE_ACPI_TABLES)
-	.acpi_name                = intel_wifi_acpi_name,
+	.acpi_name                = generic_wifi_acpi_name,
    .acpi_fill_ssdt_generator = intel_wifi_fill_ssdt,
 #endif
 };
diff --git a/src/drivers/wifi/Kconfig b/src/drivers/wifi/Kconfig
new file mode 100644
index 0000000..9b87f84
--- /dev/null
+++ b/src/drivers/wifi/Kconfig
@@ -0,0 +1,57 @@
+config DRIVERS_GENERIC_WIFI
+	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.
+
+if DRIVERS_GENERIC_WIFI
+
+config USE_SAR
+	bool
+	default n
+	help
+	  Enable it when wifi driver uses SAR configuration feature.
+	  VPD entry "wifi_sar" is read to get SAR settings, if its
+	  not found driver may look into CBFS for default settigs.
+	  WIFI_SAR_CBFS is option to enable CBFS lookup.
+
+config SAR_ENABLE
+	bool
+	default n
+	depends on USE_SAR
+
+config DSAR_ENABLE
+	bool
+	default n
+	depends on USE_SAR
+
+config GEO_SAR_ENABLE
+	bool
+	default n
+	depends on USE_SAR
+
+config WIFI_SAR_CBFS
+	bool "Enable SAR table addition to CBFS"
+	default n
+	depends on USE_SAR
+	help
+	 wifi driver would look for "wifi_sar" vpd key and load SAR settings from
+	 it, if the vpd key is not found then the driver tries to look for sar
+	 settings from CBFS with file name wifi_sar_defaults.hex.
+	 So OEM/ODM can override wifi sar with VPD.
+
+config WIFI_SAR_CBFS_FILEPATH
+	string "The cbfs file which has WIFI SAR defaults"
+	depends on WIFI_SAR_CBFS
+	default "src/mainboard/$(MAINBOARDDIR)/wifi_sar_defaults.hex"
+
+config DSAR_SET_NUM
+	hex "Number of SAR sets when D-SAR is enabled"
+	default 0x3
+	depends on USE_SAR
+	help
+	  There can be up to 3 optional SAR table sets.
+
+endif # DRIVERS_GENERIC_WIFI
diff --git a/src/drivers/wifi/Makefile.inc b/src/drivers/wifi/Makefile.inc
new file mode 100644
index 0000000..d37015c
--- /dev/null
+++ b/src/drivers/wifi/Makefile.inc
@@ -0,0 +1 @@
+ramstage-$(CONFIG_DRIVERS_GENERIC_WIFI) += generic.c
diff --git a/src/drivers/wifi/generic.c b/src/drivers/wifi/generic.c
new file mode 100644
index 0000000..b593ffe
--- /dev/null
+++ b/src/drivers/wifi/generic.c
@@ -0,0 +1,245 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * Copyright 2019 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 or (at your option)
+ * any later version of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <arch/acpi_device.h>
+#include <arch/acpigen.h>
+#include <console/console.h>
+#include <device/device.h>
+#include <device/pci_def.h>
+#include <sar.h>
+#include <string.h>
+#include <wrdd.h>
+#include "generic_wifi.h"
+
+/* WRDS Spec Revision */
+#define WRDS_REVISION 0x0
+
+/* EWRD Spec Revision */
+#define EWRD_REVISION 0x0
+
+/* WRDS Domain type */
+#define WRDS_DOMAIN_TYPE_WIFI 0x7
+
+/* EWRD Domain type */
+#define EWRD_DOMAIN_TYPE_WIFI 0x7
+
+/* WGDS Domain type */
+#define WGDS_DOMAIN_TYPE_WIFI 0x7
+
+/*
+ * WIFI ACPI NAME = "WF" + hex value of last 8 bits of dev_path_encode + '\0'
+ * The above representation returns unique and consistent name every time
+ * generate_wifi_acpi_name is invoked. The last 8 bits of dev_path_encode is
+ * chosen since it contains the bus address of the device.
+ */
+#define WIFI_ACPI_NAME_MAX_LEN 5
+
+__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 generic_wifi_fill_ssdt(struct device *dev,
+			    const struct generic_wifi_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));
+	acpigen_write_name_integer("_UID", 0);
+	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, config->maxsleep);
+
+	/* 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 *generic_wifi_acpi_name(const struct device *dev)
+{
+	static char wifi_acpi_name[WIFI_ACPI_NAME_MAX_LEN];
+
+	snprintf(wifi_acpi_name, sizeof(wifi_acpi_name), "WF%02x",
+		 (dev_path_encode(dev) & 0xff));
+	return wifi_acpi_name;
+}
diff --git a/src/drivers/wifi/generic_wifi.h b/src/drivers/wifi/generic_wifi.h
new file mode 100644
index 0000000..5f17c3e
--- /dev/null
+++ b/src/drivers/wifi/generic_wifi.h
@@ -0,0 +1,51 @@
+/*
+ * This file is part of the coreboot project.
+ *
+ * Copyright 2019 Google LLC
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; version 2 of the License.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _GENERIC_WIFI_H_
+#define _GENERIC_WIFI_H_
+
+/**
+ * struct generic_wifi_config - Data structure to contain common wifi config
+ * @wake: Wake pin for ACPI _PRW
+ * @maxsleep: Maximum sleep state to wake from
+ */
+struct generic_wifi_config {
+	unsigned int wake;
+	unsigned int maxsleep;
+};
+
+/**
+ * wifi_fill_ssdt() - Fill ACPI SSDT table for WiFi controller
+ * @dev: Device structure corresponding to WiFi controller.
+ * @config: Common wifi config required to fill ACPI SSDT table.
+ *
+ * This function implements common device operation to help fill ACPI SSDT
+ * table for WiFi controller.
+ */
+void generic_wifi_fill_ssdt(struct device *dev,
+			    const struct generic_wifi_config *config);
+
+/**
+ * wifi_acpi_name() - Get ACPI name for WiFi controller
+ * @dev: Device structure corresponding to WiFi controller.
+ *
+ * This function implements common device operation to get the ACPI name for
+ * WiFi controller.
+ *
+ * Return: string representing the ACPI name for WiFi controller.
+ */
+const char *generic_wifi_acpi_name(const struct device *dev);
+
+#endif /* _GENERIC_WIFI_H_ */