Re: [PATCH wireless-next v2 6/8] firmware: bcm47xx_sprom: read ledbh4..ledbh15 from NVRAM

[vit9696]

This change looks confusing to me.

While I believe it not clearly documented in public, from my knowledge the general approach is as follows:
- Broadcom supplies SPROM with configuration defaults.
- Vendor overrides these defaults via NVRAM where SDK allows it.

I believe all currently known SPROMs have configuration for [0, 3] GPIO pins whereas NVRAM overrides cover the whole [0, 15] GPIO range.

What I entirely miss is why this patch modifies bcma_sprom_get and not just bcm47xx_sprom_fill_auto (called from bcm47xx_fill_sprom ← bcm47xx_get_sprom_bcma). I believe you can safely process the remaining overrides there next to currently processed [0, 3] overrides.

In my opinion, it also makes sense convert gpio0/gpio1/gpio2/gpio3/gpio_ext[] to a unified array, because this is actually a 16-byte or a 32-byte array depending on the chip.

Best,
Vitaly

> On 27 Oct 2026, at 02:53, Alessio Ferri <alessio.ferri@xxxxxxxxxxx> wrote:
> 
> Add gpio_ext[] for the LED behaviours of GPIO 4-15 and fill it from
> NVRAM ledbh4..ledbh15, both for a SPROM built from NVRAM and, through
> the new bcm47xx_sprom_fill_bcma_nvram(), on top of a SPROM read from a
> bcma device. The bcma prefix lookup moves into a helper shared by both
> paths.
> 
> Signed-off-by: Alessio Ferri <alessio.ferri@xxxxxxxxxxx>
> ---
> drivers/bcma/sprom.c                      |  3 ++
> drivers/firmware/broadcom/bcm47xx_sprom.c | 86 +++++++++++++++++++++++--------
> include/linux/bcm47xx_sprom.h             | 15 ++++++
> include/linux/ssb/ssb.h                   |  1 +
> 4 files changed, 83 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/bcma/sprom.c b/drivers/bcma/sprom.c
> index c98b6fb66..ac7622512 100644
> --- a/drivers/bcma/sprom.c
> +++ b/drivers/bcma/sprom.c
> @@ -11,6 +11,7 @@
> 
> #include <linux/bcma/bcma.h>
> #include <linux/bcma/bcma_regs.h>
> +#include <linux/bcm47xx_sprom.h>
> #include <linux/pci.h>
> #include <linux/io.h>
> #include <linux/dma-mapping.h>
> @@ -841,6 +842,8 @@ int bcma_sprom_get(struct bcma_bus *bus)
> else
> bcma_sprom_extract_r8(bus, sprom);
> kfree(sprom);
> + /* Most hosts have no NVRAM: ignore the result. */
> + bcm47xx_sprom_fill_bcma_nvram(bus, &bus->sprom);
> }
> 
> return err;
> diff --git a/drivers/firmware/broadcom/bcm47xx_sprom.c b/drivers/firmware/broadcom/bcm47xx_sprom.c
> index bca03fd85..75604cd70 100644
> --- a/drivers/firmware/broadcom/bcm47xx_sprom.c
> +++ b/drivers/firmware/broadcom/bcm47xx_sprom.c
> @@ -29,6 +29,7 @@
> #include <linux/bcm47xx_nvram.h>
> #include <linux/bcm47xx_sprom.h>
> #include <linux/bcma/bcma.h>
> +#include <linux/err.h>
> #include <linux/etherdevice.h>
> #include <linux/hex.h>
> #include <linux/if_ether.h>
> @@ -172,6 +173,20 @@ static void nvram_read_alpha2(const char *prefix, const char *name,
> memcpy(val, buf, 2);
> }
> 
> +/* Variables read from NVRAM whether or not the SPROM itself comes from it. */
> +static void bcm47xx_sprom_fill_nvram_only(struct ssb_sprom *sprom,
> +  const char *prefix, bool fallback)
> +{
> + char name[sizeof("ledbh15")];
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(sprom->gpio_ext); i++) {
> + snprintf(name, sizeof(name), "ledbh%d", i + 4);
> + nvram_read_u8(prefix, NULL, name, &sprom->gpio_ext[i], 0xff,
> +      fallback);
> + }
> +}
> +
> /* This is one-function-only macro, it uses local "sprom" variable! */
> #define ENTRY(_revmask, _type, _prefix, _name, _val, _allset, _fallback) \
> if (_revmask & BIT(sprom->revision)) \
> @@ -209,6 +224,8 @@ static void bcm47xx_sprom_fill_auto(struct ssb_sprom *sprom,
> ENTRY(0xfffffffe, u8, pre, "ledbh1", gpio1, 0xff, fb);
> ENTRY(0xfffffffe, u8, pre, "ledbh2", gpio2, 0xff, fb);
> ENTRY(0xfffffffe, u8, pre, "ledbh3", gpio3, 0xff, fb);
> + if (sprom->revision)
> + bcm47xx_sprom_fill_nvram_only(sprom, pre, fb);
> 
> ENTRY(0x0000070e, u16, pre, "pa0b0", pa0b0, 0, fb);
> ENTRY(0x0000070e, u16, pre, "pa0b1", pa0b1, 0, fb);
> @@ -647,45 +664,70 @@ static void bcm47xx_sprom_apply_prefix_alias(char *prefix, size_t prefix_size)
> }
> }
> 
> -static int bcm47xx_get_sprom_bcma(struct bcma_bus *bus, struct ssb_sprom *out)
> +/*
> + * The NVRAM prefix of a bcma bus's variables: pci/<bus>/<slot>/ or its
> + * devpathN alias for a PCI device, sb/<core>/ for the SoC's own core.
> + * Returns the prefix, NULL for a SoC bus without an 802.11 core, or an
> + * error pointer for a host type NVRAM knows nothing about. *fallback is
> + * set when the caller may fall back to unprefixed variables.
> + */
> +static char *bcm47xx_sprom_bcma_prefix(struct bcma_bus *bus, char *buf,
> +       size_t size, bool *fallback)
> {
> - struct bcma_boardinfo *binfo = &bus->boardinfo;
> struct bcma_device *core;
> - char buf[10];
> - char *prefix;
> - bool fallback = false;
> 
> + *fallback = false;
> switch (bus->hosttype) {
> case BCMA_HOSTTYPE_PCI:
> - memset(out, 0, sizeof(struct ssb_sprom));
> /* On BCM47XX all PCI buses share the same domain */
> if (IS_ENABLED(CONFIG_BCM47XX))
> - snprintf(buf, sizeof(buf), "pci/%u/%u/",
> + snprintf(buf, size, "pci/%u/%u/",
> bus->host_pci->bus->number + 1,
> PCI_SLOT(bus->host_pci->devfn));
> else
> - snprintf(buf, sizeof(buf), "pci/%u/%u/",
> + snprintf(buf, size, "pci/%u/%u/",
> pci_domain_nr(bus->host_pci->bus) + 1,
> bus->host_pci->bus->number);
> - bcm47xx_sprom_apply_prefix_alias(buf, sizeof(buf));
> - prefix = buf;
> - break;
> + bcm47xx_sprom_apply_prefix_alias(buf, size);
> + return buf;
> case BCMA_HOSTTYPE_SOC:
> - memset(out, 0, sizeof(struct ssb_sprom));
> core = bcma_find_core(bus, BCMA_CORE_80211);
> - if (core) {
> - snprintf(buf, sizeof(buf), "sb/%u/",
> - core->core_index);
> - prefix = buf;
> - fallback = true;
> - } else {
> - prefix = NULL;
> - }
> - break;
> + if (!core)
> + return NULL;
> + snprintf(buf, size, "sb/%u/", core->core_index);
> + *fallback = true;
> + return buf;
> default:
> + return ERR_PTR(-EINVAL);
> + }
> +}
> +
> +int bcm47xx_sprom_fill_bcma_nvram(struct bcma_bus *bus, struct ssb_sprom *out)
> +{
> + char buf[10];
> + char *prefix;
> + bool fallback;
> +
> + prefix = bcm47xx_sprom_bcma_prefix(bus, buf, sizeof(buf), &fallback);
> + if (IS_ERR(prefix))
> + return PTR_ERR(prefix);
> + bcm47xx_sprom_fill_nvram_only(out, prefix, fallback);
> + return 0;
> +}
> +
> +static int bcm47xx_get_sprom_bcma(struct bcma_bus *bus, struct ssb_sprom *out)
> +{
> + struct bcma_boardinfo *binfo = &bus->boardinfo;
> + char buf[10];
> + char *prefix;
> + bool fallback;
> +
> + prefix = bcm47xx_sprom_bcma_prefix(bus, buf, sizeof(buf), &fallback);
> + if (IS_ERR(prefix)) {
> pr_warn("Unable to fill SPROM for given bustype.\n");
> - return -EINVAL;
> + return PTR_ERR(prefix);
> }
> + memset(out, 0, sizeof(struct ssb_sprom));
> 
> nvram_read_u16(prefix, NULL, "boardvendor", &binfo->vendor, 0, true);
> if (!binfo->vendor)
> diff --git a/include/linux/bcm47xx_sprom.h b/include/linux/bcm47xx_sprom.h
> index 40a7da3ef..47e9922ec 100644
> --- a/include/linux/bcm47xx_sprom.h
> +++ b/include/linux/bcm47xx_sprom.h
> @@ -9,6 +9,7 @@
> #include <linux/types.h>
> #include <linux/vmalloc.h>
> 
> +struct bcma_bus;
> struct ssb_sprom;
> 
> #ifdef CONFIG_BCM47XX_SPROM
> @@ -28,4 +29,18 @@ static inline int bcm47xx_sprom_register_fallbacks(void)
> };
> #endif
> 
> +/*
> + * Fill the NVRAM-only variables of a bcma bus whose SPROM was read from the
> + * device. Like the SPROM fallback, only with bcma built in.
> + */
> +#if defined(CONFIG_BCM47XX_SPROM) && IS_BUILTIN(CONFIG_BCMA)
> +int bcm47xx_sprom_fill_bcma_nvram(struct bcma_bus *bus, struct ssb_sprom *out);
> +#else
> +static inline int bcm47xx_sprom_fill_bcma_nvram(struct bcma_bus *bus,
> + struct ssb_sprom *out)
> +{
> + return -EOPNOTSUPP;
> +}
> +#endif
> +
> #endif /* __BCM47XX_SPROM_H */
> diff --git a/include/linux/ssb/ssb.h b/include/linux/ssb/ssb.h
> index 0548af664..02e6c9054 100644
> --- a/include/linux/ssb/ssb.h
> +++ b/include/linux/ssb/ssb.h
> @@ -76,6 +76,7 @@ struct ssb_sprom {
> u8 gpio1; /* GPIO pin 1 */
> u8 gpio2; /* GPIO pin 2 */
> u8 gpio3; /* GPIO pin 3 */
> + u8 gpio_ext[12]; /* GPIO pins 4-15, NVRAM ledbh4-15 only */
> u8 maxpwr_bg; /* 2.4GHz Amplifier Max Power (in dBm Q5.2) */
> u8 maxpwr_al; /* 5.2GHz Amplifier Max Power (in dBm Q5.2) */
> u8 maxpwr_a; /* 5.3GHz Amplifier Max Power (in dBm Q5.2) */
> 
> --
> 2.34.1
>

Attachment: signature.asc
Description: OpenPGP digital signature