diff mbox series

[v2,07/35] brcmfmac: pcie: Read Apple OTP information

Message ID 20220104072658.69756-8-marcan@marcan.st (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show
Series brcmfmac: Support Apple T2 and M1 platforms | expand

Commit Message

Hector Martin Jan. 4, 2022, 7:26 a.m. UTC
On Apple platforms, the One Time Programmable ROM in the Broadcom chips
contains information about the specific board design (module, vendor,
version) that is required to select the correct NVRAM file. Parse this
OTP ROM and extract the required strings.

Note that the user OTP offset/size is per-chip. This patch does not add
any chips yet.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 .../broadcom/brcm80211/brcmfmac/pcie.c        | 219 ++++++++++++++++++
 include/linux/bcma/bcma_driver_chipcommon.h   |   1 +
 2 files changed, 220 insertions(+)

Comments

Andy Shevchenko Jan. 4, 2022, 11:26 a.m. UTC | #1
On Tue, Jan 4, 2022 at 9:28 AM Hector Martin <marcan@marcan.st> wrote:
>
> On Apple platforms, the One Time Programmable ROM in the Broadcom chips
> contains information about the specific board design (module, vendor,
> version) that is required to select the correct NVRAM file. Parse this
> OTP ROM and extract the required strings.
>
> Note that the user OTP offset/size is per-chip. This patch does not add
> any chips yet.

...

> +static int
> +brcmf_pcie_parse_otp_sys_vendor(struct brcmf_pciedev_info *devinfo,
> +                               u8 *data, size_t size)
> +{
> +       int idx = 4;

Can you rather have a structure

struct my_cool_and_strange_blob {
__le32 hdr;
const char ...[];
...
}

and then cast your data to this struct?

> +       const char *chip_params;
> +       const char *board_params;
> +       const char *p;
> +
> +       /* 4-byte header and two empty strings */
> +       if (size < 6)
> +               return -EINVAL;
> +
> +       if (get_unaligned_le32(data) != BRCMF_OTP_VENDOR_HDR)
> +               return -EINVAL;
> +
> +       chip_params = &data[idx];

> +       /* Skip first string, including terminator */
> +       idx += strnlen(chip_params, size - idx) + 1;

strsep() ?

> +       if (idx >= size)
> +               return -EINVAL;
> +
> +       board_params = &data[idx];
> +
> +       /* Skip to terminator of second string */
> +       idx += strnlen(board_params, size - idx);
> +       if (idx >= size)
> +               return -EINVAL;
> +
> +       /* At this point both strings are guaranteed NUL-terminated */
> +       brcmf_dbg(PCIE, "OTP: chip_params='%s' board_params='%s'\n",
> +                 chip_params, board_params);
> +
> +       p = board_params;
> +       while (*p) {
> +               char tag = *p++;
> +               const char *end;
> +               size_t len;
> +
> +               if (tag == ' ') /* Skip extra spaces */
> +                       continue;

skip_spaces()

> +
> +               if (*p++ != '=') /* implicit NUL check */
> +                       return -EINVAL;

Have you checked the next_arg() implementation?

> +               /* *p might be NUL here, if so end == p and len == 0 */
> +               end = strchrnul(p, ' ');
> +               len = end - p;
> +
> +               /* leave 1 byte for NUL in destination string */
> +               if (len > (BRCMF_OTP_MAX_PARAM_LEN - 1))
> +                       return -EINVAL;
> +
> +               /* Copy len characters plus a NUL terminator */
> +               switch (tag) {
> +               case 'M':
> +                       strscpy(devinfo->otp.module, p, len + 1);
> +                       break;
> +               case 'V':
> +                       strscpy(devinfo->otp.vendor, p, len + 1);
> +                       break;
> +               case 'm':
> +                       strscpy(devinfo->otp.version, p, len + 1);
> +                       break;
> +               }
> +
> +               /* Skip to space separator or NUL */
> +               p = end;
> +       }
> +
> +       brcmf_dbg(PCIE, "OTP: module=%s vendor=%s version=%s\n",
> +                 devinfo->otp.module, devinfo->otp.vendor,
> +                 devinfo->otp.version);
> +
> +       if (!devinfo->otp.module ||
> +           !devinfo->otp.vendor ||
> +           !devinfo->otp.version)
> +               return -EINVAL;
> +
> +       devinfo->otp.valid = true;
> +       return 0;
> +}
> +
> +static int
> +brcmf_pcie_parse_otp(struct brcmf_pciedev_info *devinfo, u8 *otp, size_t size)
> +{
> +       int p = 0;

> +       int ret = -1;

Use proper error codes.

> +       brcmf_dbg(PCIE, "parse_otp size=%ld\n", size);
> +
> +       while (p < (size - 1)) {

too many parentheses

> +               u8 type = otp[p];
> +               u8 length = otp[p + 1];
> +
> +               if (type == 0)
> +                       break;
> +
> +               if ((p + 2 + length) > size)
> +                       break;
> +
> +               switch (type) {
> +               case BRCMF_OTP_SYS_VENDOR:
> +                       brcmf_dbg(PCIE, "OTP @ 0x%x (0x%x): SYS_VENDOR\n",

length as hex a bit harder to parse

> +                                 p, length);
> +                       ret = brcmf_pcie_parse_otp_sys_vendor(devinfo,
> +                                                             &otp[p + 2],
> +                                                             length);
> +                       break;
> +               case BRCMF_OTP_BRCM_CIS:
> +                       brcmf_dbg(PCIE, "OTP @ 0x%x (0x%x): BRCM_CIS\n",
> +                                 p, length);
> +                       break;
> +               default:
> +                       brcmf_dbg(PCIE, "OTP @ 0x%x (0x%x): Unknown type 0x%x\n",
> +                                 p, length, type);
> +                       break;
> +               }

> +               p += 2 + length;


length + 2 is easier to read.

> +       }
> +
> +       return ret;
> +}

...

> +               /* Map OTP to shadow area */
> +               WRITECC32(devinfo, sromcontrol,
> +                         sromctl | BCMA_CC_SROM_CONTROL_OTPSEL);

One line?

...

> +       otp = kzalloc(sizeof(u16) * words, GFP_KERNEL);

No check, why? I see in many places you forgot to check for NULL from
allocator functions.
Moreover here you should use kcalloc() which does overflow protection.
Arend van Spriel Jan. 6, 2022, 12:37 p.m. UTC | #2
On 1/4/2022 8:26 AM, Hector Martin wrote:
> On Apple platforms, the One Time Programmable ROM in the Broadcom chips
> contains information about the specific board design (module, vendor,
> version) that is required to select the correct NVRAM file. Parse this
> OTP ROM and extract the required strings.
> 
> Note that the user OTP offset/size is per-chip. This patch does not add
> any chips yet.

Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>
> Signed-off-by: Hector Martin <marcan@marcan.st>
> ---
>   .../broadcom/brcm80211/brcmfmac/pcie.c        | 219 ++++++++++++++++++
>   include/linux/bcma/bcma_driver_chipcommon.h   |   1 +
>   2 files changed, 220 insertions(+)
> 
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> index a52a6f8081eb..74c9a4f74813 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c

[...]

> +static int brcmf_pcie_read_otp(struct brcmf_pciedev_info *devinfo)
> +{
> +	const struct pci_dev *pdev = devinfo->pdev;
> +	struct brcmf_bus *bus = dev_get_drvdata(&pdev->dev);
> +	u32 coreid, base, words, idx, sromctl;
> +	u16 *otp;
> +	struct brcmf_core *core;
> +	int ret;
> +
> +	switch (devinfo->ci->chip) {
> +	default:
> +		/* OTP not supported on this chip */
> +		return 0;
> +	}

Does not seem this code is put to work yet. Will dive into it later on.

> +	core = brcmf_chip_get_core(devinfo->ci, coreid);
> +	if (!core) {
> +		brcmf_err(bus, "No OTP core\n");
> +		return -ENODEV;
> +	}
> +
> +	if (coreid == BCMA_CORE_CHIPCOMMON) {
> +		/* Chips with OTP accessed via ChipCommon need additional
> +		 * handling to access the OTP
> +		 */
> +		brcmf_pcie_select_core(devinfo, coreid);
> +		sromctl = READCC32(devinfo, sromcontrol);
> +
> +		if (!(sromctl & BCMA_CC_SROM_CONTROL_OTP_PRESENT)) {
> +			/* Chip lacks OTP, try without it... */
> +			brcmf_err(bus,
> +				  "OTP unavailable, using default firmware\n");
> +			return 0;
> +		}
> +
> +		/* Map OTP to shadow area */
> +		WRITECC32(devinfo, sromcontrol,
> +			  sromctl | BCMA_CC_SROM_CONTROL_OTPSEL);
> +	}
> +
> +	otp = kzalloc(sizeof(u16) * words, GFP_KERNEL);
> +
> +	/* Map bus window to SROM/OTP shadow area in core */
> +	base = brcmf_pcie_buscore_prep_addr(devinfo->pdev, base + core->base);

I guess this changes the bar window...

> +	brcmf_dbg(PCIE, "OTP data:\n");
> +	for (idx = 0; idx < words; idx++) {
> +		otp[idx] = brcmf_pcie_read_reg16(devinfo, base + 2 * idx);
> +		brcmf_dbg(PCIE, "[%8x] 0x%04x\n", base + 2 * idx, otp[idx]);
> +	}
> +
> +	if (coreid == BCMA_CORE_CHIPCOMMON) {
> +		brcmf_pcie_select_core(devinfo, coreid);

... which is why you need to reselect the core. Otherwise it makes no 
sense to me.

> +		WRITECC32(devinfo, sromcontrol, sromctl);
> +	}
> +
> +	ret = brcmf_pcie_parse_otp(devinfo, (u8 *)otp, 2 * words);
> +	kfree(otp);
> +
> +	return ret;
> +}
Hector Martin Jan. 6, 2022, 1:08 p.m. UTC | #3
On 06/01/2022 21.37, Arend van Spriel wrote:
> On 1/4/2022 8:26 AM, Hector Martin wrote:
>> +static int brcmf_pcie_read_otp(struct brcmf_pciedev_info *devinfo)
>> +{
>> +	const struct pci_dev *pdev = devinfo->pdev;
>> +	struct brcmf_bus *bus = dev_get_drvdata(&pdev->dev);
>> +	u32 coreid, base, words, idx, sromctl;
>> +	u16 *otp;
>> +	struct brcmf_core *core;
>> +	int ret;
>> +
>> +	switch (devinfo->ci->chip) {
>> +	default:
>> +		/* OTP not supported on this chip */
>> +		return 0;
>> +	}
> 
> Does not seem this code is put to work yet. Will dive into it later on.

The specific OTP ranges and cores are added by the subsequent patches
that add support for individual chips, once all the scaffolding is in place.

> 
>> +	core = brcmf_chip_get_core(devinfo->ci, coreid);
>> +	if (!core) {
>> +		brcmf_err(bus, "No OTP core\n");
>> +		return -ENODEV;
>> +	}
>> +
>> +	if (coreid == BCMA_CORE_CHIPCOMMON) {
>> +		/* Chips with OTP accessed via ChipCommon need additional
>> +		 * handling to access the OTP
>> +		 */
>> +		brcmf_pcie_select_core(devinfo, coreid);
>> +		sromctl = READCC32(devinfo, sromcontrol);
>> +
>> +		if (!(sromctl & BCMA_CC_SROM_CONTROL_OTP_PRESENT)) {
>> +			/* Chip lacks OTP, try without it... */
>> +			brcmf_err(bus,
>> +				  "OTP unavailable, using default firmware\n");
>> +			return 0;
>> +		}
>> +
>> +		/* Map OTP to shadow area */
>> +		WRITECC32(devinfo, sromcontrol,
>> +			  sromctl | BCMA_CC_SROM_CONTROL_OTPSEL);
>> +	}
>> +
>> +	otp = kzalloc(sizeof(u16) * words, GFP_KERNEL);
>> +
>> +	/* Map bus window to SROM/OTP shadow area in core */
>> +	base = brcmf_pcie_buscore_prep_addr(devinfo->pdev, base + core->base);
> 
> I guess this changes the bar window...
> 
>> +	brcmf_dbg(PCIE, "OTP data:\n");
>> +	for (idx = 0; idx < words; idx++) {
>> +		otp[idx] = brcmf_pcie_read_reg16(devinfo, base + 2 * idx);
>> +		brcmf_dbg(PCIE, "[%8x] 0x%04x\n", base + 2 * idx, otp[idx]);
>> +	}
>> +
>> +	if (coreid == BCMA_CORE_CHIPCOMMON) {
>> +		brcmf_pcie_select_core(devinfo, coreid);
> 
> ... which is why you need to reselect the core. Otherwise it makes no 
> sense to me.

Yes; *technically* with the BCMA_CORE_CHIPCOMMON core the OTP is always
within the first 0x1000 and so I wouldn't have to reselect it, since
it'd end up with the same window, but that is not the case with
BCMA_CORE_GCI used on other chips (where the OTP offset is >0x1000),
although those don't hit this code path. So while this line could be
removed without causing any issues, I find it more orthogonal and safer
to keep the pattern where I select the core before accessing
core-relative fixed registers, and treat brcmf_pcie_buscore_prep_addr as
invalidating the BAR window for all intents and purposes.
Hector Martin Jan. 7, 2022, 3:53 a.m. UTC | #4
On 2022/01/04 20:26, Andy Shevchenko wrote:
> On Tue, Jan 4, 2022 at 9:28 AM Hector Martin <marcan@marcan.st> wrote:
>>
>> On Apple platforms, the One Time Programmable ROM in the Broadcom chips
>> contains information about the specific board design (module, vendor,
>> version) that is required to select the correct NVRAM file. Parse this
>> OTP ROM and extract the required strings.
>>
>> Note that the user OTP offset/size is per-chip. This patch does not add
>> any chips yet.
> 
> ...
> 
>> +static int
>> +brcmf_pcie_parse_otp_sys_vendor(struct brcmf_pciedev_info *devinfo,
>> +                               u8 *data, size_t size)
>> +{
>> +       int idx = 4;
> 
> Can you rather have a structure
> 
> struct my_cool_and_strange_blob {
> __le32 hdr;
> const char ...[];
> ...
> }
> 
> and then cast your data to this struct?

That would mean I need to copy, since the original data is not aligned.
Since it's just one u32 header (which we don't even truly know the
interpretation of), it seems get_unaligned_le32 is easier.

> 
>> +       const char *chip_params;
>> +       const char *board_params;
>> +       const char *p;
>> +
>> +       /* 4-byte header and two empty strings */
>> +       if (size < 6)
>> +               return -EINVAL;
>> +
>> +       if (get_unaligned_le32(data) != BRCMF_OTP_VENDOR_HDR)
>> +               return -EINVAL;
>> +
>> +       chip_params = &data[idx];
> 
>> +       /* Skip first string, including terminator */
>> +       idx += strnlen(chip_params, size - idx) + 1;
> 
> strsep() ?

We're splitting on \0 here, so that won't work.

> 
>> +       if (idx >= size)
>> +               return -EINVAL;
>> +
>> +       board_params = &data[idx];
>> +
>> +       /* Skip to terminator of second string */
>> +       idx += strnlen(board_params, size - idx);
>> +       if (idx >= size)
>> +               return -EINVAL;
>> +
>> +       /* At this point both strings are guaranteed NUL-terminated */
>> +       brcmf_dbg(PCIE, "OTP: chip_params='%s' board_params='%s'\n",
>> +                 chip_params, board_params);
>> +
>> +       p = board_params;
>> +       while (*p) {
>> +               char tag = *p++;
>> +               const char *end;
>> +               size_t len;
>> +
>> +               if (tag == ' ') /* Skip extra spaces */
>> +                       continue;
> 
> skip_spaces()

Sure.

> 
>> +
>> +               if (*p++ != '=') /* implicit NUL check */
>> +                       return -EINVAL;
> 
> Have you checked the next_arg() implementation?

That function has a lot more logic (handling quotes, etc) and no other
hardware drivers use it. I'm not sure I feel comfortable using it to
parse untrusted data from a potentially compromised device. The parsing
we need to do here is much simpler.

> 
>> +               /* *p might be NUL here, if so end == p and len == 0 */
>> +               end = strchrnul(p, ' ');
>> +               len = end - p;
>> +
>> +               /* leave 1 byte for NUL in destination string */
>> +               if (len > (BRCMF_OTP_MAX_PARAM_LEN - 1))
>> +                       return -EINVAL;
>> +
>> +               /* Copy len characters plus a NUL terminator */
>> +               switch (tag) {
>> +               case 'M':
>> +                       strscpy(devinfo->otp.module, p, len + 1);
>> +                       break;
>> +               case 'V':
>> +                       strscpy(devinfo->otp.vendor, p, len + 1);
>> +                       break;
>> +               case 'm':
>> +                       strscpy(devinfo->otp.version, p, len + 1);
>> +                       break;
>> +               }
>> +
>> +               /* Skip to space separator or NUL */
>> +               p = end;
>> +       }
>> +
>> +       brcmf_dbg(PCIE, "OTP: module=%s vendor=%s version=%s\n",
>> +                 devinfo->otp.module, devinfo->otp.vendor,
>> +                 devinfo->otp.version);
>> +
>> +       if (!devinfo->otp.module ||
>> +           !devinfo->otp.vendor ||
>> +           !devinfo->otp.version)
>> +               return -EINVAL;
>> +
>> +       devinfo->otp.valid = true;
>> +       return 0;
>> +}
>> +
>> +static int
>> +brcmf_pcie_parse_otp(struct brcmf_pciedev_info *devinfo, u8 *otp, size_t size)
>> +{
>> +       int p = 0;
> 
>> +       int ret = -1;
> 
> Use proper error codes.

Ack.

> 
>> +       brcmf_dbg(PCIE, "parse_otp size=%ld\n", size);
>> +
>> +       while (p < (size - 1)) {
> 
> too many parentheses

Really? I see this is all over kernel code. I know it's redundant, but I
find parentheses around expressions used for one side of a comparison to
be a lot more readable since you don't have to start doubting whether
that particular operator has higher precedence than the comparison (+
does but & does not).

> 
>> +               u8 type = otp[p];
>> +               u8 length = otp[p + 1];
>> +
>> +               if (type == 0)
>> +                       break;
>> +
>> +               if ((p + 2 + length) > size)
>> +                       break;
>> +
>> +               switch (type) {
>> +               case BRCMF_OTP_SYS_VENDOR:
>> +                       brcmf_dbg(PCIE, "OTP @ 0x%x (0x%x): SYS_VENDOR\n",
> 
> length as hex a bit harder to parse

Not so sure about that, especially if you're trying to mentally add it
to offsets... but sure, I can make it decimal.

> 
>> +                                 p, length);
>> +                       ret = brcmf_pcie_parse_otp_sys_vendor(devinfo,
>> +                                                             &otp[p + 2],
>> +                                                             length);
>> +                       break;
>> +               case BRCMF_OTP_BRCM_CIS:
>> +                       brcmf_dbg(PCIE, "OTP @ 0x%x (0x%x): BRCM_CIS\n",
>> +                                 p, length);
>> +                       break;
>> +               default:
>> +                       brcmf_dbg(PCIE, "OTP @ 0x%x (0x%x): Unknown type 0x%x\n",
>> +                                 p, length, type);
>> +                       break;
>> +               }
> 
>> +               p += 2 + length;
> 
> 
> length + 2 is easier to read.

I was following the data order here; 2 header bytes and then length
payload bytes. Same reason I used p + 2 + length above.

> 
>> +       }
>> +
>> +       return ret;
>> +}
> 
> ...
> 
>> +               /* Map OTP to shadow area */
>> +               WRITECC32(devinfo, sromcontrol,
>> +                         sromctl | BCMA_CC_SROM_CONTROL_OTPSEL);
> 
> One line?

That exceeds 80 chars, which seems to be the standard in this file which
I'm trying to stick to. If people are okay with pushing to 100 lines,
there are lots of other places I could unwrap lines in this series.

> 
> ...
> 
>> +       otp = kzalloc(sizeof(u16) * words, GFP_KERNEL);
> 
> No check, why? I see in many places you forgot to check for NULL from
> allocator functions.

I think at some point something convinced me that kzalloc and friends
don't fail with GFP_KERNEL... which they rarely do, but they do. I'll
fix it, and add a few missing checks to the existing code while I'm at it.

> Moreover here you should use kzalloc() which does overflow protection.
words is a constant from the switch statement so this could never
overflow anyway, but sure.
diff mbox series

Patch

diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
index a52a6f8081eb..74c9a4f74813 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
@@ -255,6 +255,15 @@  struct brcmf_pcie_core_info {
 	u32 wrapbase;
 };
 
+#define BRCMF_OTP_MAX_PARAM_LEN 16
+
+struct brcmf_otp_params {
+	char module[BRCMF_OTP_MAX_PARAM_LEN];
+	char vendor[BRCMF_OTP_MAX_PARAM_LEN];
+	char version[BRCMF_OTP_MAX_PARAM_LEN];
+	bool valid;
+};
+
 struct brcmf_pciedev_info {
 	enum brcmf_pcie_state state;
 	bool in_irq;
@@ -282,6 +291,7 @@  struct brcmf_pciedev_info {
 	void (*write_ptr)(struct brcmf_pciedev_info *devinfo, u32 mem_offset,
 			  u16 value);
 	struct brcmf_mp_device *settings;
+	struct brcmf_otp_params otp;
 };
 
 struct brcmf_pcie_ringbuf {
@@ -353,6 +363,14 @@  static void brcmf_pcie_setup(struct device *dev, int ret,
 static struct brcmf_fw_request *
 brcmf_pcie_prepare_fw_request(struct brcmf_pciedev_info *devinfo);
 
+static u16
+brcmf_pcie_read_reg16(struct brcmf_pciedev_info *devinfo, u32 reg_offset)
+{
+	void __iomem *address = devinfo->regs + reg_offset;
+
+	return ioread16(address);
+}
+
 static u32
 brcmf_pcie_read_reg32(struct brcmf_pciedev_info *devinfo, u32 reg_offset)
 {
@@ -539,6 +557,8 @@  brcmf_pcie_copy_dev_tomem(struct brcmf_pciedev_info *devinfo, u32 mem_offset,
 }
 
 
+#define READCC32(devinfo, reg) brcmf_pcie_read_reg32(devinfo, \
+		CHIPCREGOFFS(reg))
 #define WRITECC32(devinfo, reg, value) brcmf_pcie_write_reg32(devinfo, \
 		CHIPCREGOFFS(reg), value)
 
@@ -1758,6 +1778,199 @@  static const struct brcmf_buscore_ops brcmf_pcie_buscore_ops = {
 	.write32 = brcmf_pcie_buscore_write32,
 };
 
+#define BRCMF_OTP_SYS_VENDOR	0x15
+#define BRCMF_OTP_BRCM_CIS	0x80
+
+#define BRCMF_OTP_VENDOR_HDR	0x00000008
+
+static int
+brcmf_pcie_parse_otp_sys_vendor(struct brcmf_pciedev_info *devinfo,
+				u8 *data, size_t size)
+{
+	int idx = 4;
+	const char *chip_params;
+	const char *board_params;
+	const char *p;
+
+	/* 4-byte header and two empty strings */
+	if (size < 6)
+		return -EINVAL;
+
+	if (get_unaligned_le32(data) != BRCMF_OTP_VENDOR_HDR)
+		return -EINVAL;
+
+	chip_params = &data[idx];
+
+	/* Skip first string, including terminator */
+	idx += strnlen(chip_params, size - idx) + 1;
+	if (idx >= size)
+		return -EINVAL;
+
+	board_params = &data[idx];
+
+	/* Skip to terminator of second string */
+	idx += strnlen(board_params, size - idx);
+	if (idx >= size)
+		return -EINVAL;
+
+	/* At this point both strings are guaranteed NUL-terminated */
+	brcmf_dbg(PCIE, "OTP: chip_params='%s' board_params='%s'\n",
+		  chip_params, board_params);
+
+	p = board_params;
+	while (*p) {
+		char tag = *p++;
+		const char *end;
+		size_t len;
+
+		if (tag == ' ') /* Skip extra spaces */
+			continue;
+
+		if (*p++ != '=') /* implicit NUL check */
+			return -EINVAL;
+
+		/* *p might be NUL here, if so end == p and len == 0 */
+		end = strchrnul(p, ' ');
+		len = end - p;
+
+		/* leave 1 byte for NUL in destination string */
+		if (len > (BRCMF_OTP_MAX_PARAM_LEN - 1))
+			return -EINVAL;
+
+		/* Copy len characters plus a NUL terminator */
+		switch (tag) {
+		case 'M':
+			strscpy(devinfo->otp.module, p, len + 1);
+			break;
+		case 'V':
+			strscpy(devinfo->otp.vendor, p, len + 1);
+			break;
+		case 'm':
+			strscpy(devinfo->otp.version, p, len + 1);
+			break;
+		}
+
+		/* Skip to space separator or NUL */
+		p = end;
+	}
+
+	brcmf_dbg(PCIE, "OTP: module=%s vendor=%s version=%s\n",
+		  devinfo->otp.module, devinfo->otp.vendor,
+		  devinfo->otp.version);
+
+	if (!devinfo->otp.module ||
+	    !devinfo->otp.vendor ||
+	    !devinfo->otp.version)
+		return -EINVAL;
+
+	devinfo->otp.valid = true;
+	return 0;
+}
+
+static int
+brcmf_pcie_parse_otp(struct brcmf_pciedev_info *devinfo, u8 *otp, size_t size)
+{
+	int p = 0;
+	int ret = -1;
+
+	brcmf_dbg(PCIE, "parse_otp size=%ld\n", size);
+
+	while (p < (size - 1)) {
+		u8 type = otp[p];
+		u8 length = otp[p + 1];
+
+		if (type == 0)
+			break;
+
+		if ((p + 2 + length) > size)
+			break;
+
+		switch (type) {
+		case BRCMF_OTP_SYS_VENDOR:
+			brcmf_dbg(PCIE, "OTP @ 0x%x (0x%x): SYS_VENDOR\n",
+				  p, length);
+			ret = brcmf_pcie_parse_otp_sys_vendor(devinfo,
+							      &otp[p + 2],
+							      length);
+			break;
+		case BRCMF_OTP_BRCM_CIS:
+			brcmf_dbg(PCIE, "OTP @ 0x%x (0x%x): BRCM_CIS\n",
+				  p, length);
+			break;
+		default:
+			brcmf_dbg(PCIE, "OTP @ 0x%x (0x%x): Unknown type 0x%x\n",
+				  p, length, type);
+			break;
+		}
+
+		p += 2 + length;
+	}
+
+	return ret;
+}
+
+static int brcmf_pcie_read_otp(struct brcmf_pciedev_info *devinfo)
+{
+	const struct pci_dev *pdev = devinfo->pdev;
+	struct brcmf_bus *bus = dev_get_drvdata(&pdev->dev);
+	u32 coreid, base, words, idx, sromctl;
+	u16 *otp;
+	struct brcmf_core *core;
+	int ret;
+
+	switch (devinfo->ci->chip) {
+	default:
+		/* OTP not supported on this chip */
+		return 0;
+	}
+
+	core = brcmf_chip_get_core(devinfo->ci, coreid);
+	if (!core) {
+		brcmf_err(bus, "No OTP core\n");
+		return -ENODEV;
+	}
+
+	if (coreid == BCMA_CORE_CHIPCOMMON) {
+		/* Chips with OTP accessed via ChipCommon need additional
+		 * handling to access the OTP
+		 */
+		brcmf_pcie_select_core(devinfo, coreid);
+		sromctl = READCC32(devinfo, sromcontrol);
+
+		if (!(sromctl & BCMA_CC_SROM_CONTROL_OTP_PRESENT)) {
+			/* Chip lacks OTP, try without it... */
+			brcmf_err(bus,
+				  "OTP unavailable, using default firmware\n");
+			return 0;
+		}
+
+		/* Map OTP to shadow area */
+		WRITECC32(devinfo, sromcontrol,
+			  sromctl | BCMA_CC_SROM_CONTROL_OTPSEL);
+	}
+
+	otp = kzalloc(sizeof(u16) * words, GFP_KERNEL);
+
+	/* Map bus window to SROM/OTP shadow area in core */
+	base = brcmf_pcie_buscore_prep_addr(devinfo->pdev, base + core->base);
+
+	brcmf_dbg(PCIE, "OTP data:\n");
+	for (idx = 0; idx < words; idx++) {
+		otp[idx] = brcmf_pcie_read_reg16(devinfo, base + 2 * idx);
+		brcmf_dbg(PCIE, "[%8x] 0x%04x\n", base + 2 * idx, otp[idx]);
+	}
+
+	if (coreid == BCMA_CORE_CHIPCOMMON) {
+		brcmf_pcie_select_core(devinfo, coreid);
+		WRITECC32(devinfo, sromcontrol, sromctl);
+	}
+
+	ret = brcmf_pcie_parse_otp(devinfo, (u8 *)otp, 2 * words);
+	kfree(otp);
+
+	return ret;
+}
+
 #define BRCMF_PCIE_FW_CODE	0
 #define BRCMF_PCIE_FW_NVRAM	1
 #define BRCMF_PCIE_FW_CLM	2
@@ -1955,6 +2168,12 @@  brcmf_pcie_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (ret)
 		goto fail_bus;
 
+	ret = brcmf_pcie_read_otp(devinfo);
+	if (ret) {
+		brcmf_err(bus, "failed to parse OTP\n");
+		goto fail_brcmf;
+	}
+
 	fwreq = brcmf_pcie_prepare_fw_request(devinfo);
 	if (!fwreq) {
 		ret = -ENOMEM;
diff --git a/include/linux/bcma/bcma_driver_chipcommon.h b/include/linux/bcma/bcma_driver_chipcommon.h
index d35b9206096d..c91db7460190 100644
--- a/include/linux/bcma/bcma_driver_chipcommon.h
+++ b/include/linux/bcma/bcma_driver_chipcommon.h
@@ -270,6 +270,7 @@ 
 #define  BCMA_CC_SROM_CONTROL_OP_WRDIS	0x40000000
 #define  BCMA_CC_SROM_CONTROL_OP_WREN	0x60000000
 #define  BCMA_CC_SROM_CONTROL_OTPSEL	0x00000010
+#define  BCMA_CC_SROM_CONTROL_OTP_PRESENT	0x00000020
 #define  BCMA_CC_SROM_CONTROL_LOCK	0x00000008
 #define  BCMA_CC_SROM_CONTROL_SIZE_MASK	0x00000006
 #define  BCMA_CC_SROM_CONTROL_SIZE_1K	0x00000000