drivers: wifi: brcmfmac: address review (chip-ID constants, CLM Kconfig)

Address @tchebb's second review pass:

  * Named constants for chip IDs (BRCM_CC_43430_CHIP_ID, _43439_, _4345_)
    matching Linux brcm_hw_ids.h; replace bare 43430 / 43439 / 0xa9a6
    / 0x4345 throughout. This also fixes the brcmfmac_init() CLM gate
    and brcmfmac_chip_cm3_set_passive() bank-3 gate, both of which
    were checking the same chip ID twice (43430 == 0xa9a6) and so
    never fired on CYW43439.

  * Drop the chip-ID gate on the CLM blob upload. Linux brcmfmac
    doesn't gate this -- it just uploads if a blob is provided,
    silently skips if not. Mirror that: skip when brcmfmac_clm_blob_len
    is zero. Removes the hallucinated "firmware refuses the iovar"
    rationale.

  * Wire the CLM blob into the build system the same way as firmware
    and NVRAM: new WIFI_BRCMFMAC_CLM_FILE Kconfig string (default
    empty), CMake generates brcmfmac_clm_blob.inc from the named
    blob, brcmfmac_fw_blob.c #include's it. Empty string -> empty
    .inc -> zero-length array -> upload skipped. Also fixes a latent
    link error: brcmfmac_clm_blob[] was declared extern but never
    defined.

  * Named constants for the WL_REG_ON pulse delays (was bare 10/250),
    matching WHD's 20/150 values for this chip family.

  * Named constants for the dload header flags (BRCMF_DL_BEGIN,
    _DL_END, _DL_CRC_IN_HDR, _DL_TYPE_CLM) and the CLM upload chunk
    size, replacing magic 2/4/(1<<12)/512 ints in brcmfmac_process_clm_blob.

  * Bump the NVRAM scratch buffer to 4 KiB so it covers the largest
    NVRAM currently in linux-firmware (3099 bytes after the footer).

Signed-off-by: Jonathan E. Peace <jep@alphabetiq.com>

Author: jetpax

drivers/wifi/brcmfmac/CMakeLists.txt

@@ -23,6 +23,7 @@ if(CONFIG_BUILD_ONLY_NO_BLOBS)
   # which is the expected failure mode for a no-blobs build.
   file(WRITE ${brcmfmac_gen_dir}/brcmfmac_fw.inc "")
   file(WRITE ${brcmfmac_gen_dir}/brcmfmac_nvram.inc "")
+  file(WRITE ${brcmfmac_gen_dir}/brcmfmac_clm_blob.inc "")
 else()
   # Pull the firmware and per-board NVRAM from hal_broadcom (fetched
   # via `west blobs fetch hal_broadcom`) and convert each into a
@@ -37,6 +38,17 @@ else()
   generate_inc_file_for_target(${ZEPHYR_CURRENT_LIBRARY}
     ${brcmfmac_blob_dir}/${CONFIG_WIFI_BRCMFMAC_NVRAM_FILE}
     ${brcmfmac_gen_dir}/brcmfmac_nvram.inc)
+
+  # CLM blob is optional: a board that needs it sets
+  # CONFIG_WIFI_BRCMFMAC_CLM_FILE in its defconfig; an unset/empty
+  # string links a zero-length array and the driver skips upload.
+  if(CONFIG_WIFI_BRCMFMAC_CLM_FILE STREQUAL "")
+    file(WRITE ${brcmfmac_gen_dir}/brcmfmac_clm_blob.inc "")
+  else()
+    generate_inc_file_for_target(${ZEPHYR_CURRENT_LIBRARY}
+      ${brcmfmac_blob_dir}/${CONFIG_WIFI_BRCMFMAC_CLM_FILE}
+      ${brcmfmac_gen_dir}/brcmfmac_clm_blob.inc)
+  endif()
 endif()
 
 endif()

drivers/wifi/brcmfmac/Kconfig.brcmfmac

@@ -41,6 +41,17 @@ config WIFI_BRCMFMAC_NVRAM_FILE
 	  ${ZEPHYR_HAL_BROADCOM_MODULE_DIR}/zephyr/blobs/img/brcmfmac/.
 	  Encodes board-specific antenna, calibration and PA settings.
 
+config WIFI_BRCMFMAC_CLM_FILE
+	string "CLM blob file name (under hal_broadcom blobs)"
+	default ""
+	help
+	  Optional Country Locale Matrix blob under
+	  ${ZEPHYR_HAL_BROADCOM_MODULE_DIR}/zephyr/blobs/img/brcmfmac/.
+	  Required by chips whose firmware ships without built-in regulatory
+	  tables (e.g. BCM43458F); chips that have them (e.g. BCM43430A1)
+	  still accept the upload harmlessly. Leave empty to skip CLM upload
+	  entirely -- the driver links a zero-length blob in that case.
+
 config WIFI_BRCMFMAC_RX_THREAD_STACK_SIZE
 	int "RX thread stack size"
 	default 4096

drivers/wifi/brcmfmac/brcmfmac_chip.c

@@ -350,10 +350,11 @@ int brcmfmac_chip_read_id(struct brcmfmac_data *data)
 int brcmfmac_chip_ram_data(struct brcmfmac_data *data)
 {
 	/* TODO: Read ram_size from core registers like in Linux. */
-	if (data->chip_id == 43430 || data->chip_id == 43439) {
+	if (data->chip_id == BRCM_CC_43430_CHIP_ID ||
+	    data->chip_id == BRCM_CC_43439_CHIP_ID) {
 		data->ram_base = 0u;
 		data->ram_size = 0x80000u;
-	} else if (data->chip_id == 0x4345) {
+	} else if (data->chip_id == BRCM_CC_4345_CHIP_ID) {
 		data->ram_base = 0x198000u;
 		data->ram_size = 0xC8000u;
 	} else {
@@ -501,7 +502,8 @@ static int brcmfmac_chip_cm3_set_passive(struct brcmfmac_data *data)
 	 * present (BCM4329, BCM43236) or has a different PDA layout
 	 * (BCM43340, BCM4334) and the write would misconfigure RAM.
 	 */
-	if (data->chip_id == 43430 || data->chip_id == 0xa9a6) {
+	if (data->chip_id == BRCM_CC_43430_CHIP_ID ||
+	    data->chip_id == BRCM_CC_43439_CHIP_ID) {
 		(void)brcmfmac_sdio_backplane_write32(data,
 				sr->base + SOCRAM_BANKIDX_OFFSET, 3);
 		(void)brcmfmac_sdio_backplane_write32(data,

drivers/wifi/brcmfmac/brcmfmac_core.c

@@ -27,14 +27,28 @@
 
 LOG_MODULE_REGISTER(brcmfmac, CONFIG_WIFI_LOG_LEVEL);
 
+/* WL_REG_ON pulse timing. The chip's CBUCK regulator needs a brief window
+ * to fully discharge before re-power, then ~150 ms for the chip's clocks
+ * to come up and the bootloader to reach a state where F0/F1 enumerate.
+ * Matches the values WHD (Cypress's HAL) uses for the same family.
+ */
+#define BRCMFMAC_REG_ON_LOW_DELAY_MS     20
+#define BRCMFMAC_REG_ON_HIGH_DELAY_MS    150
+
+/* CLM blob upload chunking. Linux uses 1400 bytes/chunk; we cap at 512
+ * because brcmfmac_bcdc_iovar_set caps the payload at 1024 bytes total
+ * and the dload header eats 12 of those.
+ */
+#define BRCMFMAC_CLM_CHUNK_SIZE          512
+
 static int brcmfmac_initialization(struct brcmfmac_data *data)
 {
 	int ret = brcmfmac_chip_read_id(data);
 
 	if (ret != 0) {
 		return ret;
 	}
-	if (data->chip_id == 43430 && data->chip_rev == 1) {
+	if (data->chip_id == BRCM_CC_43430_CHIP_ID && data->chip_rev == 1) {
 		LOG_INF("  -> matches BCM43430A1");
 	}
 
@@ -98,15 +112,15 @@ static int brcmfmac_power_on(const struct device *dev)
 	}
 
 	/* Allow CBUCK regulator to discharge */
-	k_msleep(10);
+	k_msleep(BRCMFMAC_REG_ON_LOW_DELAY_MS);
 
 	/* WIFI power on */
 	ret = gpio_pin_set_dt(&cfg->reg_on, 1);
 	if (ret) {
 		return ret;
 	}
 
-	k_msleep(250); /* TODO: Make constant */
+	k_msleep(BRCMFMAC_REG_ON_HIGH_DELAY_MS);
 #else
 	(void)dev;
 #endif /* DT_INST_NODE_HAS_PROP(0, reg_on_gpios) */
@@ -159,24 +173,23 @@ static int brcmfmac_probe_sdio(const struct device *dev)
 
 static int brcmfmac_process_clm_blob(struct brcmfmac_data *data)
 {
-	const size_t chunk_size = 512;
 	struct brcmf_dload_data_le *hdr;
-	uint8_t buf[sizeof(*hdr) + chunk_size];
+	uint8_t buf[sizeof(*hdr) + BRCMFMAC_CLM_CHUNK_SIZE];
 	const uint8_t *current = brcmfmac_clm_blob;
 	size_t remaining = brcmfmac_clm_blob_len;
 
 	hdr = (struct brcmf_dload_data_le *)buf;
-	hdr->flag = (1 << 12) | 2; /* DL_BEGIN */
-	hdr->dload_type = 2;       /* DL_TYPE_CLM */
+	hdr->flag = BRCMF_DL_CRC_IN_HDR | BRCMF_DL_BEGIN;
+	hdr->dload_type = BRCMF_DL_TYPE_CLM;
 	hdr->crc = 0;
 
 	while (remaining > 0) {
-		size_t transfer = chunk_size;
+		size_t transfer = BRCMFMAC_CLM_CHUNK_SIZE;
 		int ret;
 
 		if (remaining <= transfer) {
 			transfer = remaining;
-			hdr->flag |= 4; /* DL_END */
+			hdr->flag |= BRCMF_DL_END;
 		}
 
 		memcpy(hdr->data, current, transfer);
@@ -191,7 +204,7 @@ static int brcmfmac_process_clm_blob(struct brcmfmac_data *data)
 
 		current += transfer;
 		remaining -= transfer;
-		hdr->flag &= ~2u;
+		hdr->flag &= ~BRCMF_DL_BEGIN;
 	}
 
 	return 0;
@@ -227,23 +240,20 @@ static int brcmfmac_init(const struct device *dev)
 	}
 
 	/*
-	 * CLM blob upload is required by chips whose firmware ships with no
-	 * built-in regulatory tables. Linux brcmfmac gates this on chip ID;
-	 * we mirror that gate so chips that don't need it (firmware refuses
-	 * the DL_TYPE_CLM iovar) don't fail init. Extend the list as new
-	 * chips are validated.
+	 * CLM blob upload. Chips that ship without built-in regulatory tables
+	 * (e.g. BCM43458F) require this; chips that have them (e.g. BCM43430A1)
+	 * still accept the iovar harmlessly. We always attempt the upload when
+	 * a blob is configured in the build via CONFIG_WIFI_BRCMFMAC_CLM_FILE;
+	 * an unconfigured build links an empty array (len==0) and we skip.
 	 */
-	bool needs_clm = (data->chip_id == 43430) ||         /* BCM43430A1 */
-			 (data->chip_id == 0xa9a6);          /* CYW43439   */
-	if (needs_clm) {
+	if (brcmfmac_clm_blob_len > 0) {
 		ret = brcmfmac_process_clm_blob(data);
 		if (ret != 0) {
 			LOG_ERR("CLM upload failed: %d", ret);
 			return ret;
 		}
 	} else {
-		LOG_DBG("CLM blob upload skipped for chip 0x%x rev %u",
-			data->chip_id, data->chip_rev);
+		LOG_DBG("CLM blob upload skipped (no blob configured)");
 	}
 
 	int got = brcmfmac_bcdc_iovar_get(data, "cur_etheraddr",

drivers/wifi/brcmfmac/brcmfmac_fw_blob.c

@@ -3,9 +3,11 @@
  *
  * SPDX-License-Identifier: Apache-2.0
  *
- * brcmfmac firmware and NVRAM blob payload. The actual bytes are
- * generated by CMake from the corresponding files in hal_broadcom
- * (fetched via `west blobs fetch hal_broadcom`) and #include'd here.
+ * brcmfmac firmware, NVRAM and (optional) CLM blob payloads. The actual
+ * bytes are generated by CMake from the corresponding files in
+ * hal_broadcom (fetched via `west blobs fetch hal_broadcom`) and
+ * #include'd here. An unset CONFIG_WIFI_BRCMFMAC_CLM_FILE links an
+ * empty CLM array and the driver skips the upload at runtime.
  */
 
 const unsigned char brcmfmac_fw[] = {
@@ -17,3 +19,8 @@ const unsigned char brcmfmac_nvram[] = {
 #include "brcmfmac_nvram.inc"
 };
 const unsigned int brcmfmac_nvram_len = sizeof(brcmfmac_nvram);
+
+const unsigned char brcmfmac_clm_blob[] = {
+#include "brcmfmac_clm_blob.inc"
+};
+const unsigned int brcmfmac_clm_blob_len = sizeof(brcmfmac_clm_blob);

drivers/wifi/brcmfmac/brcmfmac_priv.h

@@ -53,6 +53,15 @@
 #define CID_TYPE_MASK                   0xF0000000
 #define CID_TYPE_SHIFT                  28
 
+/* Broadcom/Cypress chipcommon[0] chip-ID values. The chip reports these
+ * as the low 16 bits of CHIPID; brcmfmac uses them to select chip-specific
+ * code paths (firmware blob, RAM layout, SOCRAM bank-3 quirk). Mirrors
+ * Linux brcm_hw_ids.h.
+ */
+#define BRCM_CC_43430_CHIP_ID           0xa9a6   /* BCM43430 / BCM43436 */
+#define BRCM_CC_43439_CHIP_ID           0xa9af   /* CYW43439 */
+#define BRCM_CC_4345_CHIP_ID            0x4345   /* BCM43458F */
+
 /* BCMA core IDs (subset; full list in Linux include/linux/bcma/bcma.h). */
 #define BCMA_CORE_INTERNAL_MEM          0x80E    /* SOCRAM */
 #define BCMA_CORE_80211                 0x812    /* D11 MAC */
@@ -261,6 +270,18 @@ struct brcmf_event_msg_be {
 #define BRCMF_E_STATUS_NO_NETWORKS 3
 #define BRCMF_E_STATUS_PARTIAL   8
 
+/* Flags for struct brcmf_dload_data_le (Linux brcmfmac/fwil_types.h
+ * enum brcmf_dload_data_flag). The chip expects DL_BEGIN on the first
+ * chunk and DL_END on the last; bit 12 (CRC-supplied) is set unconditionally
+ * since the header carries a (currently zero) CRC field.
+ */
+#define BRCMF_DL_BEGIN          (1u << 1)
+#define BRCMF_DL_END            (1u << 2)
+#define BRCMF_DL_CRC_IN_HDR     (1u << 12)
+
+/* dload_type values (Linux brcmfmac/fwil_types.h enum brcmf_dload_type). */
+#define BRCMF_DL_TYPE_CLM       2
+
 struct brcmf_dload_data_le {
 	uint16_t flag;
 	uint16_t dload_type;

drivers/wifi/brcmfmac/brcmfmac_sdio.c

@@ -331,7 +331,10 @@ int brcmfmac_sdio_fw_upload(struct brcmfmac_data *data)
 
 int brcmfmac_sdio_nvram_upload(struct brcmfmac_data *data)
 {
-	static uint8_t nvram_buf[2048] __aligned(CONFIG_DCACHE_LINE_SIZE);
+	/* Sized to fit the largest NVRAM currently shipped in linux-firmware
+	 * (3099 bytes after the footer); rounds up to 4 KiB.
+	 */
+	static uint8_t nvram_buf[4096] __aligned(CONFIG_DCACHE_LINE_SIZE);
 	int stripped = brcmfmac_sdio_nvram_strip(brcmfmac_nvram, brcmfmac_nvram_len,
 						nvram_buf, sizeof(nvram_buf));
 	if (stripped < 0) {