Message ID | 20220104072658.69756-1-marcan@marcan.st |
---|---|
Headers | show |
Series | brcmfmac: Support Apple T2 and M1 platforms | expand |
04.01.2022 10:26, Hector Martin пишет: > Apple platforms have firmware and config files identified with multiple > dimensions. We want to be able to find the most specific firmware > available for any given platform, progressively trying more general > firmwares. > > First, add support for having multiple alternate firmware paths. > > Acked-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/firmware.c | 75 ++++++++++++++----- > .../broadcom/brcm80211/brcmfmac/firmware.h | 2 + > 2 files changed, 59 insertions(+), 18 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > index 0497b721136a..7570dbf22cdd 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > @@ -427,6 +427,8 @@ void brcmf_fw_nvram_free(void *nvram) > struct brcmf_fw { > struct device *dev; > struct brcmf_fw_request *req; > + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]; > + int alt_index; unsigned int > u32 curpos; > void (*done)(struct device *dev, int err, struct brcmf_fw_request *req); > }; > @@ -592,14 +594,18 @@ static int brcmf_fw_complete_request(const struct firmware *fw, > return (cur->flags & BRCMF_FW_REQF_OPTIONAL) ? 0 : ret; > } > > -static char *brcm_alt_fw_path(const char *path, const char *board_type) > +static int brcm_alt_fw_paths(const char *path, const char *board_type, > + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS])> { > char alt_path[BRCMF_FW_NAME_LEN]; > const char *suffix; > > + memset(alt_paths, 0, array_size(sizeof(*alt_paths), > + BRCMF_FW_MAX_ALT_PATHS)); You don't need to use array_size() since size of a fixed array is already known. memset(alt_paths, 0, sizeof(alt_paths)); ... > +static void > +brcm_free_alt_fw_paths(const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) > +{ > + unsigned int i; > + > + for (i = 0; alt_paths[i]; i++) What if array is fully populated and there is no null in the end? Please don't do this, use BRCMF_FW_MAX_ALT_PATHS or ARRAY_SIZE(). > + kfree(alt_paths[i]); > } > > static int brcmf_fw_request_firmware(const struct firmware **fw, > @@ -617,19 +634,25 @@ static int brcmf_fw_request_firmware(const struct firmware **fw, > { > struct brcmf_fw_item *cur = &fwctx->req->items[fwctx->curpos]; > int ret; > + unsigned int i; Keep reverse Xmas tree coding style. ... > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h > @@ -11,6 +11,8 @@ > > #define BRCMF_FW_DEFAULT_PATH "brcm/" > > +#define BRCMF_FW_MAX_ALT_PATHS 8 Two tabs are needed here.
On 2022/01/04 17:26, Dmitry Osipenko wrote: > 04.01.2022 10:26, Hector Martin пишет: >> Apple platforms have firmware and config files identified with multiple >> dimensions. We want to be able to find the most specific firmware >> available for any given platform, progressively trying more general >> firmwares. >> >> First, add support for having multiple alternate firmware paths. >> >> Acked-by: Linus Walleij <linus.walleij@linaro.org> >> Signed-off-by: Hector Martin <marcan@marcan.st> >> --- >> .../broadcom/brcm80211/brcmfmac/firmware.c | 75 ++++++++++++++----- >> .../broadcom/brcm80211/brcmfmac/firmware.h | 2 + >> 2 files changed, 59 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >> index 0497b721136a..7570dbf22cdd 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >> @@ -427,6 +427,8 @@ void brcmf_fw_nvram_free(void *nvram) >> struct brcmf_fw { >> struct device *dev; >> struct brcmf_fw_request *req; >> + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]; >> + int alt_index; > > unsigned int Ack. > >> u32 curpos; >> void (*done)(struct device *dev, int err, struct brcmf_fw_request *req); >> }; >> @@ -592,14 +594,18 @@ static int brcmf_fw_complete_request(const struct firmware *fw, >> return (cur->flags & BRCMF_FW_REQF_OPTIONAL) ? 0 : ret; >> } >> >> -static char *brcm_alt_fw_path(const char *path, const char *board_type) >> +static int brcm_alt_fw_paths(const char *path, const char *board_type, >> + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS])> { >> char alt_path[BRCMF_FW_NAME_LEN]; >> const char *suffix; >> >> + memset(alt_paths, 0, array_size(sizeof(*alt_paths), >> + BRCMF_FW_MAX_ALT_PATHS)); > You don't need to use array_size() since size of a fixed array is > already known. > > memset(alt_paths, 0, sizeof(alt_paths)); It's a function argument, so that doesn't work and actually throws a warning. Array function argument notation is informative only; they behave strictly equivalent to pointers. Try it: $ cat test.c #include <stdio.h> void foo(char x[42]) { printf("%ld\n", sizeof(x)); } int main() { char x[42]; foo(x); } $ gcc test.c test.c: In function ‘foo’: test.c:5:31: warning: ‘sizeof’ on array function parameter ‘x’ will return size of ‘char *’ [-Wsizeof-array-argument] 5 | printf("%ld\n", sizeof(x)); | ^ test.c:3:15: note: declared here 3 | void foo(char x[42]) | ~~~~~^~~~~ $ ./a.out 8 > > ... >> +static void >> +brcm_free_alt_fw_paths(const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) >> +{ >> + unsigned int i; >> + >> + for (i = 0; alt_paths[i]; i++) > > What if array is fully populated and there is no null in the end? Please > don't do this, use BRCMF_FW_MAX_ALT_PATHS or ARRAY_SIZE(). Argh, forgot to change this one. I used BRCMF_FW_MAX_ALT_PATHS elsewhere; ARRAY_SIZE won't work as I explained above. > >> + kfree(alt_paths[i]); >> } >> >> static int brcmf_fw_request_firmware(const struct firmware **fw, >> @@ -617,19 +634,25 @@ static int brcmf_fw_request_firmware(const struct firmware **fw, >> { >> struct brcmf_fw_item *cur = &fwctx->req->items[fwctx->curpos]; >> int ret; >> + unsigned int i; > > Keep reverse Xmas tree coding style. First time I hear this one, heh. Sure. > > ... >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h >> @@ -11,6 +11,8 @@ >> >> #define BRCMF_FW_DEFAULT_PATH "brcm/" >> >> +#define BRCMF_FW_MAX_ALT_PATHS 8 > > Two tabs are needed here. Will do.
On 1/4/2022 8:26 AM, Hector Martin wrote: > On DT platforms, the module-instance and antenna-sku-info properties > are passed in the DT. On ACPI platforms, module-instance is passed via > the analogous Apple device property mechanism, while the antenna SKU > info is instead obtained via an ACPI method that grabs it from > non-volatile storage. > > Add support for this, to allow proper firmware selection on Apple > platforms. > > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/Makefile | 2 + > .../broadcom/brcm80211/brcmfmac/acpi.c | 47 +++++++++++++++++++ > .../broadcom/brcm80211/brcmfmac/common.c | 1 + > .../broadcom/brcm80211/brcmfmac/common.h | 9 ++++ > 4 files changed, 59 insertions(+) > create mode 100644 drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile > index 13c13504a6e8..19009eb9db93 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile > @@ -47,3 +47,5 @@ brcmfmac-$(CONFIG_OF) += \ > of.o > brcmfmac-$(CONFIG_DMI) += \ > dmi.o > +brcmfmac-$(CONFIG_ACPI) += \ > + acpi.o > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c > new file mode 100644 > index 000000000000..2b1a4448b291 > --- /dev/null > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c > @@ -0,0 +1,47 @@ > +// SPDX-License-Identifier: ISC > +/* > + * Copyright The Asahi Linux Contributors > + */ Common format for copyright statement (in this folder) seems to be: Copyright (c) <YEAR> <COPYRIGHT_HOLDER> Regards, Arend
On 1/4/2022 8:26 AM, Hector Martin wrote: > In order to make use of the multiple alt_path functionality, change > board_type to an array. Bus drivers can pass in a NULL-terminated list > of board type strings to try for the firmware fetch. > > Acked-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/firmware.c | 35 ++++++++++++------- > .../broadcom/brcm80211/brcmfmac/firmware.h | 2 +- > .../broadcom/brcm80211/brcmfmac/pcie.c | 4 ++- > .../broadcom/brcm80211/brcmfmac/sdio.c | 2 +- > 4 files changed, 27 insertions(+), 16 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > index 7570dbf22cdd..054ea3ed133e 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > @@ -594,28 +594,39 @@ static int brcmf_fw_complete_request(const struct firmware *fw, > return (cur->flags & BRCMF_FW_REQF_OPTIONAL) ? 0 : ret; > } > > -static int brcm_alt_fw_paths(const char *path, const char *board_type, > +static int brcm_alt_fw_paths(const char *path, struct brcmf_fw *fwctx, > const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) > { > + const char **board_types = fwctx->req->board_types; > + unsigned int i; > char alt_path[BRCMF_FW_NAME_LEN]; > const char *suffix; > > memset(alt_paths, 0, array_size(sizeof(*alt_paths), > BRCMF_FW_MAX_ALT_PATHS)); > > + if (!board_types[0]) > + return -ENOENT; > + > suffix = strrchr(path, '.'); > if (!suffix || suffix == path) > return -EINVAL; > > - /* strip extension at the end */ > - strscpy(alt_path, path, BRCMF_FW_NAME_LEN); > - alt_path[suffix - path] = 0; > + for (i = 0; i < BRCMF_FW_MAX_ALT_PATHS; i++) { > + if (!board_types[i]) > + break; Indentation error
On 2022/01/04 19:22, Arend van Spriel wrote: > On 1/4/2022 8:26 AM, Hector Martin wrote: >> In order to make use of the multiple alt_path functionality, change >> board_type to an array. Bus drivers can pass in a NULL-terminated list >> of board type strings to try for the firmware fetch. >> >> Acked-by: Linus Walleij <linus.walleij@linaro.org> >> Signed-off-by: Hector Martin <marcan@marcan.st> >> --- >> .../broadcom/brcm80211/brcmfmac/firmware.c | 35 ++++++++++++------- >> .../broadcom/brcm80211/brcmfmac/firmware.h | 2 +- >> .../broadcom/brcm80211/brcmfmac/pcie.c | 4 ++- >> .../broadcom/brcm80211/brcmfmac/sdio.c | 2 +- >> 4 files changed, 27 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >> index 7570dbf22cdd..054ea3ed133e 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >> @@ -594,28 +594,39 @@ static int brcmf_fw_complete_request(const struct firmware *fw, >> return (cur->flags & BRCMF_FW_REQF_OPTIONAL) ? 0 : ret; >> } >> >> -static int brcm_alt_fw_paths(const char *path, const char *board_type, >> +static int brcm_alt_fw_paths(const char *path, struct brcmf_fw *fwctx, >> const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) >> { >> + const char **board_types = fwctx->req->board_types; >> + unsigned int i; >> char alt_path[BRCMF_FW_NAME_LEN]; >> const char *suffix; >> >> memset(alt_paths, 0, array_size(sizeof(*alt_paths), >> BRCMF_FW_MAX_ALT_PATHS)); >> >> + if (!board_types[0]) >> + return -ENOENT; >> + >> suffix = strrchr(path, '.'); >> if (!suffix || suffix == path) >> return -EINVAL; >> >> - /* strip extension at the end */ >> - strscpy(alt_path, path, BRCMF_FW_NAME_LEN); >> - alt_path[suffix - path] = 0; >> + for (i = 0; i < BRCMF_FW_MAX_ALT_PATHS; i++) { >> + if (!board_types[i]) >> + break; > > Indentation error I knew I had a feeling I was forgetting to do something... that was running v2 through checkpatch. Sigh. Thanks for catching that, fixed :)
On 2022/01/04 19:21, Arend van Spriel wrote: > On 1/4/2022 8:26 AM, Hector Martin wrote: >> On DT platforms, the module-instance and antenna-sku-info properties >> are passed in the DT. On ACPI platforms, module-instance is passed via >> the analogous Apple device property mechanism, while the antenna SKU >> info is instead obtained via an ACPI method that grabs it from >> non-volatile storage. >> >> Add support for this, to allow proper firmware selection on Apple >> platforms. >> >> Signed-off-by: Hector Martin <marcan@marcan.st> >> --- >> .../broadcom/brcm80211/brcmfmac/Makefile | 2 + >> .../broadcom/brcm80211/brcmfmac/acpi.c | 47 +++++++++++++++++++ >> .../broadcom/brcm80211/brcmfmac/common.c | 1 + >> .../broadcom/brcm80211/brcmfmac/common.h | 9 ++++ >> 4 files changed, 59 insertions(+) >> create mode 100644 drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c >> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile >> index 13c13504a6e8..19009eb9db93 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile >> @@ -47,3 +47,5 @@ brcmfmac-$(CONFIG_OF) += \ >> of.o >> brcmfmac-$(CONFIG_DMI) += \ >> dmi.o >> +brcmfmac-$(CONFIG_ACPI) += \ >> + acpi.o >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c >> new file mode 100644 >> index 000000000000..2b1a4448b291 >> --- /dev/null >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c >> @@ -0,0 +1,47 @@ >> +// SPDX-License-Identifier: ISC >> +/* >> + * Copyright The Asahi Linux Contributors >> + */ > > Common format for copyright statement (in this folder) seems to be: > > Copyright (c) <YEAR> <COPYRIGHT_HOLDER> > > Regards, > Arend I get this every time I submit a patch to a new subsystem :-) This is based on this best practice: https://www.linuxfoundation.org/blog/copyright-notices-in-open-source-software-projects/ Basically, the year format quickly becomes outdated and is rather useless, and listing specific names also ends up missing every subsequent contributor, so more general copyright statements work better for this kind of use case. In the end we all know git history is the proper record of copyright status. I don't have a super strong opinion here, but we've been trying to standardize on this format for contributions coming from our subproject, and it feels more useful than a random contributor's name with a date 5 years in the past :)
On Tue, Jan 4, 2022 at 9:28 AM Hector Martin <marcan@marcan.st> wrote: > > On Apple ARM64 platforms, firmware selection requires two properties > that come from system firmware: the module-instance (aka "island", a > codename representing a given hardware platform) and the antenna-sku. > We map Apple's module codenames to board_types in the form > "apple,<module-instance>". > > The mapped board_type is added to the DTS file in that form, while the > antenna-sku is forwarded by our bootloader from the Apple Device Tree > into the FDT. Grab them from the DT so firmware selection can use > them. > + /* Apple ARM64 platforms have their own idea of board type, passed in > + * via the device tree. They also have an antenna SKU parameter > + */ > + if (!of_property_read_string(np, "brcm,board-type", &prop)) > + settings->board_type = devm_kstrdup(dev, prop, GFP_KERNEL); > + > + if (!of_property_read_string(np, "apple,antenna-sku", &prop)) > + settings->antenna_sku = devm_kstrdup(dev, prop, GFP_KERNEL); No error checks? But hold on, why do you need to copy them? Are you expecting this to be in DTO?
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.
On Tue, Jan 4, 2022 at 9:28 AM Hector Martin <marcan@marcan.st> wrote: > > In order to make use of the multiple alt_path functionality, change > board_type to an array. Bus drivers can pass in a NULL-terminated list > of board type strings to try for the firmware fetch. > + /* strip extension at the end */ > + strscpy(alt_path, path, BRCMF_FW_NAME_LEN); > + alt_path[suffix - path] = 0; > > - alt_paths[0] = kstrdup(alt_path, GFP_KERNEL); > + strlcat(alt_path, ".", BRCMF_FW_NAME_LEN); > + strlcat(alt_path, board_types[i], BRCMF_FW_NAME_LEN); > + strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); > + > + alt_paths[i] = kstrdup(alt_path, GFP_KERNEL); > + brcmf_dbg(TRACE, "FW alt path: %s\n", alt_paths[i]); Consider replacing these string manipulations with kasprintf().
On Tue, Jan 4, 2022 at 9:29 AM Hector Martin <marcan@marcan.st> wrote: > > The driver was enabling IRQs before the message processing was > initialized. This could cause IRQs to come in too early and crash the > driver. Instead, move the IRQ enable and hostready to a bus preinit > function, at which point everything is properly initialized. > > Fixes: 9e37f045d5e7 ("brcmfmac: Adding PCIe bus layer support.") You should gather fixes at the beginning of the series, and even possible to send them as a separate series. In the current state it's unclear if there are dependencies on your new feature (must not be for fixes that meant to be backported).
On Tue, Jan 4, 2022 at 9:29 AM Hector Martin <marcan@marcan.st> wrote: > > On Device Tree platforms, it is customary to be able to set the MAC > address via the Device Tree, as it is often stored in system firmware. > This is particularly relevant for Apple ARM64 platforms, where this > information comes from system configuration and passed through by the > bootloader into the DT. > > Implement support for this by fetching the platform MAC address and > adding or replacing the macaddr= property in nvram. This becomes the > dongle's default MAC address. > > On platforms with an SROM MAC address, this overrides it. On platforms > without one, such as Apple ARM64 devices, this is required for the > firmware to boot (it will fail if it does not have a valid MAC at all). ... > +#define BRCMF_FW_MACADDR_FMT "macaddr=%pM" > +#define BRCMF_FW_MACADDR_LEN (7 + ETH_ALEN * 3) ... > if (strncmp(&nvp->data[nvp->entry], "boardrev", 8) == 0) > nvp->boardrev_found = true; > + /* strip macaddr if platform MAC overrides */ > + if (nvp->strip_mac && > + strncmp(&nvp->data[nvp->entry], "macaddr", 7) == 0) If it has no side effects, I would rather swap the operands of && so you match string first (it will be in align with above code at least, although I haven't checked bigger context). .... > +static void brcmf_fw_add_macaddr(struct nvram_parser *nvp, u8 *mac) > +{ > + snprintf(&nvp->nvram[nvp->nvram_len], BRCMF_FW_MACADDR_LEN + 1, > + BRCMF_FW_MACADDR_FMT, mac); Please, avoid using implict format string, it's dangerous from security p.o.v. > + nvp->nvram_len += BRCMF_FW_MACADDR_LEN + 1; Also, with temporary variable the code can be better to read size_t mac_len = ...; > +}
On Tue, Jan 4, 2022 at 9:28 AM Hector Martin <marcan@marcan.st> wrote: > > On Apple platforms, firmware selection uses the following elements: > > Property Example Source > ============== ======= ======================== > * Chip name 4378 Device ID > * Chip revision B1 OTP > * Platform shikoku DT (ARM64) or ACPI (x86) > * Module type RASP OTP > * Module vendor m OTP > * Module version 6.11 OTP > * Antenna SKU X3 DT (ARM64) or ACPI (x86) > > In macOS, these firmwares are stored using filenames in this format > under /usr/share/firmware/wifi: > > C-4378__s-B1/P-shikoku-X3_M-RASP_V-m__m-6.11.txt > > To prepare firmwares for Linux, we rename these to a scheme following > the existing brcmfmac convention: > > brcmfmac<chip><lower(rev)>-pcie.apple,<platform>-<mod_type>-\ > <mod_vendor>-<mod_version>-<antenna_sku>.txt > > The NVRAM uses all the components, while the firmware and CLM blob only > use the chip/revision/platform/antenna_sku: > > brcmfmac<chip><lower(rev)>-pcie.apple,<platform>-<antenna_sku>.bin > > e.g. > > brcm/brcmfmac4378b1-pcie.apple,shikoku-RASP-m-6.11-X3.txt > brcm/brcmfmac4378b1-pcie.apple,shikoku-X3.bin > > In addition, since there are over 1000 files in total, many of which are > symlinks or outright duplicates, we deduplicate and prune the firmware > tree to reduce firmware filenames to fewer dimensions. For example, the > shikoku platform (MacBook Air M1 2020) simplifies to just 4 files: > > brcm/brcmfmac4378b1-pcie.apple,shikoku.clm_blob > brcm/brcmfmac4378b1-pcie.apple,shikoku.bin > brcm/brcmfmac4378b1-pcie.apple,shikoku-RASP-m.txt > brcm/brcmfmac4378b1-pcie.apple,shikoku-RASP-u.txt > > This reduces the total file count to around 170, of which 75 are > symlinks and 95 are regular files: 7 firmware blobs, 27 CLM blobs, and > 61 NVRAM config files. We also slightly process NVRAM files to correct > some formatting issues. > > To handle this, the driver must try the following path formats when > looking for firmware files: > > brcm/brcmfmac4378b1-pcie.apple,shikoku-RASP-m-6.11-X3.txt > brcm/brcmfmac4378b1-pcie.apple,shikoku-RASP-m-6.11.txt > brcm/brcmfmac4378b1-pcie.apple,shikoku-RASP-m.txt > brcm/brcmfmac4378b1-pcie.apple,shikoku-RASP.txt > brcm/brcmfmac4378b1-pcie.apple,shikoku-X3.txt * > brcm/brcmfmac4378b1-pcie.apple,shikoku.txt > > * Not relevant for NVRAM, only for firmware/CLM. > > The chip revision nominally comes from OTP on Apple platforms, but it > can be mapped to the PCI revision number, so we ignore the OTP revision > and continue to use the existing PCI revision mechanism to identify chip > revisions, as the driver already does for other chips. Unfortunately, > the mapping is not consistent between different chip types, so this has > to be determined experimentally. ... > + /* Apple platforms with fancy firmware/NVRAM selection */ > + if (devinfo->settings->board_type && > + devinfo->settings->antenna_sku && > + devinfo->otp.valid) { > + char *buf; > + int len; > + > + brcmf_dbg(PCIE, "Apple board: %s\n", > + devinfo->settings->board_type); > + > + /* Example: apple,shikoku-RASP-m-6.11-X3 */ > + len = (strlen(devinfo->settings->board_type) + 1 + > + strlen(devinfo->otp.module) + 1 + > + strlen(devinfo->otp.vendor) + 1 + > + strlen(devinfo->otp.version) + 1 + > + strlen(devinfo->settings->antenna_sku) + 1); NIH devm_kasprrintf() ? > + /* apple,shikoku */ > + fwreq->board_types[5] = devinfo->settings->board_type; > + > + buf = devm_kzalloc(&devinfo->pdev->dev, len, GFP_KERNEL); > + > + strscpy(buf, devinfo->settings->board_type, len); > + strlcat(buf, "-", len); > + strlcat(buf, devinfo->settings->antenna_sku, len); > + /* apple,shikoku-X3 */ > + fwreq->board_types[4] = devm_kstrdup(&devinfo->pdev->dev, buf, > + GFP_KERNEL); > + > + strscpy(buf, devinfo->settings->board_type, len); > + strlcat(buf, "-", len); > + strlcat(buf, devinfo->otp.module, len); > + /* apple,shikoku-RASP */ > + fwreq->board_types[3] = devm_kstrdup(&devinfo->pdev->dev, buf, > + GFP_KERNEL); > + > + strlcat(buf, "-", len); > + strlcat(buf, devinfo->otp.vendor, len); > + /* apple,shikoku-RASP-m */ > + fwreq->board_types[2] = devm_kstrdup(&devinfo->pdev->dev, buf, > + GFP_KERNEL); > + > + strlcat(buf, "-", len); > + strlcat(buf, devinfo->otp.version, len); > + /* apple,shikoku-RASP-m-6.11 */ > + fwreq->board_types[1] = devm_kstrdup(&devinfo->pdev->dev, buf, > + GFP_KERNEL); > + > + strlcat(buf, "-", len); > + strlcat(buf, devinfo->settings->antenna_sku, len); > + /* apple,shikoku-RASP-m-6.11-X3 */ > + fwreq->board_types[0] = buf;
On Tue, Jan 4, 2022 at 9:27 AM Hector Martin <marcan@marcan.st> wrote: > > Hi everyone, > > Happy new year! This 35-patch series adds proper support for the > Broadcom FullMAC chips used on Apple T2 and M1 platforms: > > - BCM4355C1 > - BCM4364B2/B3 > - BCM4377B3 > - BCM4378B1 > - BCM4387C2 > > As usual for Apple, things are ever so slightly different on these > machines from every other Broadcom platform. In particular, besides > the normal device/firmware support changes, a large fraction of this > series deals with selecting and loading the correct firmware. These > platforms use multiple dimensions for firmware selection, and the values > for these dimensions are variously sourced from DT or OTP (see the > commit message for #9 for the gory details). > > This is what is included: > > # 01: DT bindings (M1 platforms) > > On M1 platforms, we use the device tree to provide properties for PCIe > devices like these cards. This patch re-uses the existing SDIO binding > and adds the compatibles for these PCIe chips, plus the properties we > need to correctly instantiate them: > > - brcm,board-type: Overrides the board-type which is used for firmware > selection on all platforms, which normally comes from the DMI device > name or the root node compatible. Apple have their own > mapping/identifier here ("island" name), so we prefix it with "apple," > and use it as the board-type override. > > - apple,antenna-sku: Specifies the specific antenna configuration in a > produt. This would normally be filled in by the bootloader from > device-specific configuration data. On ACPI platforms, this is > provided via ACPI instead. This is used to build the funky Apple > firmware filenames. Note: it seems the antenna doesn't actually matter > for any of the above platforms (they are all aliases to the same files > and our firmware copier collapses down this dimension), but since > Apple do support having different firmware or NVRAM depending on > antenna SKU, we ough to support it in case it starts mattering on a > future platform. > > - brcm,cal-blob: A calibration blob for the Wi-Fi module, specific to a > given unit. On most platforms, this is stored in SROM on the module, > and does not need to be provided externally, but Apple instead stores > this in platform configuration for M1 machines and the driver needs to > upload it to the device after initializing the firmware. This has a > generic brcm name, since a priori this mechanism shouldn't be > Apple-specific, although chances are only Apple do it like this so far. > > # 02~09: Apple firmware selection (M1 platforms) > > These patches add support for choosing firmwares (binaries, CLM blobs, > and NVRAM configs alike) using all the dimensions that Apple uses. The > firmware files are renamed to conform to the existing brcmfmac > convention. See the commit message for #9 for the gory details as to how > these filenames are constructed. The data to make the firmware selection > comes from the above DT properties and from an OTP ROM on the chips on > M1 platforms. > > # 10~14: BCM4378 support (M1 T8103 platforms) > > These patches make changes required to support the BCM4378 chip present > in Apple M1 (T8103) platforms. This includes adding support for passing > in the MAC address via the DT (this is standard on DT platforms) since > the chip does not have a burned-in MAC; adding support for PCIe core > revs >64 (which moved around some registers); tweaking ring buffer > sizes; and fixing a bug. > > # 15~20: BCM4355/4364/4377 support (T2 platforms) > > These patches add support for the chips found across T2 Mac platforms. > This includes ACPI support for fetching properties instead of using DT, > providing a buffer of entropy to the devices (required for some of the > firmwares), and adding the required IDs. This also fixes the BCM4364 > firmware naming; it was added without consideration that there are two > incompatible chip revisions. To avoid this ambiguity in the future, all > the chips added by this series use firmware names ending in the revision > (apple/brcm style, that is letter+number), so that future revisions can > be added without creating confusion. > > # 21~27: BCM4387 support (M1 Pro/Max T600x platforms) > > These patches add support for the newer BCM4387 present in the recently > launched M1 Pro/Max platforms. This chip requires a few changes to D11 > reset behavior and TCM size calculation to work properly, and it uses > newer firmware which needs support for newer firmware interfaces > in the cfg80211 support. Backwards compatibility is maintained via > feature flags discovered at runtime from information provided by the > firmware. > > A note on #26: it seems this chip broke the old hack of passing the PMK > in hexdump form as a PSK, but it seems brcmfmac chips supported passing > it in binary all along. I'm not sure why it was done this way in the > Linux driver, but it seems OpenBSD always did it in binary and works > with older chips, so this should be reasonably well tested. Any further > insight as to why this was done this way would be appreciated. > > # 28~32: Fixes > > These are just random things I came across while developing this series. > #31 is required to avoid a compile warning in subsequent patches. None > of these are strictly required to support these chips/platforms. > > # 33-35: TxCap and calibration blobs > > These patches add support for uploading TxCap blobs, which are another > kind of firmware blob that Apple platforms use (T2 and M1), as well as > providing Wi-Fi calibration data from the device tree (M1). > > I'm not sure what the TxCap blobs do. Given the stray function > prototype at [5], it would seem the Broadcom folks in charge of Linux > drivers also know all about Apple's fancy OTP for firmware selection > and the existence of TxCap blobs, so it would be great if you could > share any insight here ;-) > > These patches are not required for the chips to function, but presumably > having proper per-device calibration data available matters, and I > assume the TxCap blobs aren't just for show either. > > # On firmware > > As you might expect, the firmware for these machines is not available > under a redistributable license; however, every owner of one of these > machines *is* implicitly licensed to posess the firmware, and the OS > packages containing it are available under well-known URLs on Apple's > CDN with no authentication. > > Our plan to support this is to propose a platform firmware mechanism, > where platforms can provide a firmware package in the EFI system > partition along with a manifest, and distros will extract it to > /lib/firmware on boot or otherwise make it available to the kernel. > > Then, on M1 platforms, our install script, which performs all the > bootloader installation steps required to run Linux on these machines in > the first place, will also take care of copying the firmware from the > base macOS image to the EFI partition. On T2 platforms, we'll provide an > analogous script that users can manually run prior to a normal EFI Linux > installation to just grab the firmware from /usr/share/firmware/wifi in > the running macOS. > > There is an example firmware manifest at [1] which details the files > copied by our firmware rename script [2], as of macOS 12.0.1. > > To test this series on a supported Mac today (T2 or M1), boot into macOS > and run: > > $ git clone https://github.com/AsahiLinux/asahi-installer > $ cd asahi-installer/src > $ python -m firmware.wifi /usr/share/firmware/wifi firmware.tar > > Then copy firmware.tar to Linux and extract it into /lib/firmware. I looked into the ~half of the series and basically common mistakes you have are (but not limited to): - missed checks for error from allocator calls - NIH devm_kasprintf() - quite possible reinveting a wheel of many functions we have already implemented in the kernel. Suggestion for the last one is to use `git grep ...`, which is very powerful instrument, and just always questioning yourself "I'm doing XYZ and my gut feeling is that XYZ is (so) generic I can't believe there is no implementation of it already exists in the kernel". This is how I come up with a lot of cleanups done in the past.
On January 4, 2022 8:30:51 AM Hector Martin <marcan@marcan.st> wrote: > This new API version is required for at least the BCM4387 firmware. Add > support for it, with a fallback to the v1 API. > > Acked-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/cfg80211.c | 113 ++++++++++++++---- > .../broadcom/brcm80211/brcmfmac/feature.c | 1 + > .../broadcom/brcm80211/brcmfmac/feature.h | 4 +- > .../broadcom/brcm80211/brcmfmac/fwil_types.h | 49 +++++++- > 4 files changed, 145 insertions(+), 22 deletions(-) Compiling this patch with C=2 gives following warnings: drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:1086:28: warning: incorrect type in assignment (different base types) drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:1086:28: expected restricted __le16 [usertype] version drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:1086:28: got int drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:1148:38: warning: incorrect type in assignment (different base types) drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:1148:38: expected restricted __le32 [usertype] scan_type drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:1148:38: got int drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:789:30: warning: incorrect type in assignment (different base types) drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:789:30: expected unsigned char [usertype] scan_type drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:789:30: got restricted __le32 [usertype] scan_type Will check if this is a valid warning. Regards, Arend
04.01.2022 11:43, Hector Martin пишет: >>> +static int brcm_alt_fw_paths(const char *path, const char *board_type, >>> + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS])> { >>> char alt_path[BRCMF_FW_NAME_LEN]; >>> const char *suffix; >>> >>> + memset(alt_paths, 0, array_size(sizeof(*alt_paths), >>> + BRCMF_FW_MAX_ALT_PATHS)); >> You don't need to use array_size() since size of a fixed array is >> already known. >> >> memset(alt_paths, 0, sizeof(alt_paths)); > It's a function argument, so that doesn't work and actually throws a > warning. Array function argument notation is informative only; they > behave strictly equivalent to pointers. Try it: > > $ cat test.c > #include <stdio.h> > > void foo(char x[42]) > { > printf("%ld\n", sizeof(x)); > } > > int main() { > char x[42]; > > foo(x); > } > $ gcc test.c > test.c: In function ‘foo’: > test.c:5:31: warning: ‘sizeof’ on array function parameter ‘x’ will > return size of ‘char *’ [-Wsizeof-array-argument] > 5 | printf("%ld\n", sizeof(x)); > | ^ > test.c:3:15: note: declared here > 3 | void foo(char x[42]) > | ~~~~~^~~~~ > $ ./a.out > 8 Then please use "const char **alt_paths" for the function argument to make code cleaner and add another argument to pass the number of array elements. static int brcm_alt_fw_paths(const char *path, const char *board_type, const char **alt_paths, unsigned int num_paths) { size_t alt_paths_size = array_size(sizeof(*alt_paths), num_paths); memset(alt_paths, 0, alt_paths_size); } ... Maybe even better create a dedicated struct for the alt_paths: struct brcmf_fw_alt_paths { const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]; unsigned int index; }; and then use the ".index" in the brcm_free_alt_fw_paths(). I suppose this will make code a bit nicer and easier to follow.
04.01.2022 11:43, Hector Martin пишет: >>> @@ -427,6 +427,8 @@ void brcmf_fw_nvram_free(void *nvram) >>> struct brcmf_fw { >>> struct device *dev; >>> struct brcmf_fw_request *req; >>> + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]; >>> + int alt_index; >> unsigned int > Ack. > The same applies to the rest of the patches. If value can't be negative, then please use unsigned type. This makes code more consistent.
04.01.2022 10:26, Hector Martin пишет: > +static int brcm_alt_fw_paths(const char *path, const char *board_type, > + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) > { ... > +static void > +brcm_free_alt_fw_paths(const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) > +{ I'd rename this funcs to brcm_init/deinit_alt_fw_paths(), for consistency and clarity.
On 05/01/2022 07.09, Dmitry Osipenko wrote: > 04.01.2022 11:43, Hector Martin пишет: >>>> +static int brcm_alt_fw_paths(const char *path, const char *board_type, >>>> + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS])> { >>>> char alt_path[BRCMF_FW_NAME_LEN]; >>>> const char *suffix; >>>> >>>> + memset(alt_paths, 0, array_size(sizeof(*alt_paths), >>>> + BRCMF_FW_MAX_ALT_PATHS)); >>> You don't need to use array_size() since size of a fixed array is >>> already known. >>> >>> memset(alt_paths, 0, sizeof(alt_paths)); >> It's a function argument, so that doesn't work and actually throws a >> warning. Array function argument notation is informative only; they >> behave strictly equivalent to pointers. Try it: >> >> $ cat test.c >> #include <stdio.h> >> >> void foo(char x[42]) >> { >> printf("%ld\n", sizeof(x)); >> } >> >> int main() { >> char x[42]; >> >> foo(x); >> } >> $ gcc test.c >> test.c: In function ‘foo’: >> test.c:5:31: warning: ‘sizeof’ on array function parameter ‘x’ will >> return size of ‘char *’ [-Wsizeof-array-argument] >> 5 | printf("%ld\n", sizeof(x)); >> | ^ >> test.c:3:15: note: declared here >> 3 | void foo(char x[42]) >> | ~~~~~^~~~~ >> $ ./a.out >> 8 > > Then please use "const char **alt_paths" for the function argument to > make code cleaner and add another argument to pass the number of array > elements. So you want me to do the ARRAY_SIZE at the caller side then? > > static int brcm_alt_fw_paths(const char *path, const char *board_type, > const char **alt_paths, unsigned int num_paths) > { > size_t alt_paths_size = array_size(sizeof(*alt_paths), num_paths); > > memset(alt_paths, 0, alt_paths_size); > } > > ... > > Maybe even better create a dedicated struct for the alt_paths: > > struct brcmf_fw_alt_paths { > const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]; > unsigned int index; > }; > > and then use the ".index" in the brcm_free_alt_fw_paths(). I suppose > this will make code a bit nicer and easier to follow. > I'm confused; the array size is constant. What would index contain and why would would brcm_free_alt_fw_paths use it? Just as an iterator variable instead of using a local variable? Or do you mean count? Though, to be honest, at this point I'm considering rethinking the whole patch for this mechanism because I'm not terribly happy with the current approach and clearly you aren't either :-) Maybe it makes more sense to stop trying to compute all the alt_paths ahead of time, and just have the function compute a single one to be used just-in-time at firmware request time, and just iterate over board_types.
On 04/01/2022 23.23, Andy Shevchenko wrote: > On Tue, Jan 4, 2022 at 9:29 AM Hector Martin <marcan@marcan.st> wrote: >> >> On Device Tree platforms, it is customary to be able to set the MAC >> address via the Device Tree, as it is often stored in system firmware. >> This is particularly relevant for Apple ARM64 platforms, where this >> information comes from system configuration and passed through by the >> bootloader into the DT. >> >> Implement support for this by fetching the platform MAC address and >> adding or replacing the macaddr= property in nvram. This becomes the >> dongle's default MAC address. >> >> On platforms with an SROM MAC address, this overrides it. On platforms >> without one, such as Apple ARM64 devices, this is required for the >> firmware to boot (it will fail if it does not have a valid MAC at all). > > ... > >> +#define BRCMF_FW_MACADDR_FMT "macaddr=%pM" >> +#define BRCMF_FW_MACADDR_LEN (7 + ETH_ALEN * 3) > > ... > >> if (strncmp(&nvp->data[nvp->entry], "boardrev", 8) == 0) >> nvp->boardrev_found = true; >> + /* strip macaddr if platform MAC overrides */ >> + if (nvp->strip_mac && >> + strncmp(&nvp->data[nvp->entry], "macaddr", 7) == 0) > > If it has no side effects, I would rather swap the operands of && so > you match string first (it will be in align with above code at least, > although I haven't checked bigger context). I usually check for trivial flags before calling more expensive functions because it's more efficient in the common case. Obviously here performance doesn't matter though. > > .... > >> +static void brcmf_fw_add_macaddr(struct nvram_parser *nvp, u8 *mac) >> +{ >> + snprintf(&nvp->nvram[nvp->nvram_len], BRCMF_FW_MACADDR_LEN + 1, >> + BRCMF_FW_MACADDR_FMT, mac); > > Please, avoid using implict format string, it's dangerous from security p.o.v. What do you mean by implicit format string? The format string is at the top of the file and its length is right next to it, which makes it harder for them to accidentally fall out of sync. +#define BRCMF_FW_MACADDR_FMT "macaddr=%pM" +#define BRCMF_FW_MACADDR_LEN (7 + ETH_ALEN * 3) > >> + nvp->nvram_len += BRCMF_FW_MACADDR_LEN + 1; > > Also, with temporary variable the code can be better to read > > size_t mac_len = ...; > Sure.
On 1/4/2022 8:26 AM, Hector Martin wrote: > Move one of the declarations from sdio.c to pcie.c, since it makes no > sense in the former (SDIO support is optional), and add missing ones. Actually, any bus is optional so each bus should indeed declare the applicable firmware names/patterns. > Fixes: 75729e110e68 ("brcmfmac: expose firmware config files through modinfo") > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 7 +++++++ > drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 1 - > 2 files changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > index 8b149996fc00..aed49416c434 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > @@ -59,6 +59,13 @@ BRCMF_FW_DEF(4366B, "brcmfmac4366b-pcie"); > BRCMF_FW_DEF(4366C, "brcmfmac4366c-pcie"); > BRCMF_FW_DEF(4371, "brcmfmac4371-pcie"); > > +/* firmware config files */ > +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-pcie.txt"); what is this one for? Those would be covered by the specific BRCMF_FW_DEF() macro instances, no? > +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-pcie.*.txt"); > + > +/* per-board firmware binaries */ > +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-pcie.*.bin"); > + > static const struct brcmf_firmware_mapping brcmf_pcie_fwnames[] = { > BRCMF_FW_ENTRY(BRCM_CC_43602_CHIP_ID, 0xFFFFFFFF, 43602), > BRCMF_FW_ENTRY(BRCM_CC_43465_CHIP_ID, 0xFFFFFFF0, 4366C), > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > index 8effeb7a7269..5d156e591b35 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c > @@ -629,7 +629,6 @@ BRCMF_FW_CLM_DEF(43752, "brcmfmac43752-sdio"); > > /* firmware config files */ > MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-sdio.*.txt"); > -MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-pcie.*.txt"); > > /* per-board firmware binaries */ > MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-sdio.*.bin");
On 1/4/2022 8:26 AM, Hector Martin wrote: > Teach brcm_alt_fw_paths to correctly split off variable length > extensions, and enable alt firmware lookups for the CLM blob firmware > requests. > > Apple platforms have per-board CLM blob files. Are you sure? I am not involved in development for Apple platforms, but in general we build a CLM blob specific for a chip revision. As always with the blobs they are created at a certain point in time and that is mostly why you need another one for a newer platform. Apple tends to do things a bit different so you could be right though. Anyway, despite my doubts on this it does not change the need for per-board firmware files. Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Acked-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/firmware.c | 16 ++++++++-------- > 1 file changed, 8 insertions(+), 8 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > index 0eb13e5df517..0497b721136a 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > @@ -595,16 +595,16 @@ static int brcmf_fw_complete_request(const struct firmware *fw, > static char *brcm_alt_fw_path(const char *path, const char *board_type) > { > char alt_path[BRCMF_FW_NAME_LEN]; > - char suffix[5]; > + const char *suffix; > > - strscpy(alt_path, path, BRCMF_FW_NAME_LEN); > - /* At least one character + suffix */ > - if (strlen(alt_path) < 5) > + suffix = strrchr(path, '.'); > + if (!suffix || suffix == path) > return NULL; > > - /* strip .txt or .bin at the end */ > - strscpy(suffix, alt_path + strlen(alt_path) - 4, 5); > - alt_path[strlen(alt_path) - 4] = 0; > + /* strip extension at the end */ > + strscpy(alt_path, path, BRCMF_FW_NAME_LEN); > + alt_path[suffix - path] = 0; > + > strlcat(alt_path, ".", BRCMF_FW_NAME_LEN); > strlcat(alt_path, board_type, BRCMF_FW_NAME_LEN); > strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); > @@ -619,7 +619,7 @@ static int brcmf_fw_request_firmware(const struct firmware **fw, > int ret; > > /* Files can be board-specific, first try a board-specific path */ > - if (cur->type == BRCMF_FW_TYPE_NVRAM && fwctx->req->board_type) { > + if (fwctx->req->board_type) { > char *alt_path; > > alt_path = brcm_alt_fw_path(cur->path, fwctx->req->board_type); So all firmware files are attempted with board-specific path now.
On 1/4/2022 8:26 AM, Hector Martin wrote: > Apple platforms have firmware and config files identified with multiple > dimensions. We want to be able to find the most specific firmware > available for any given platform, progressively trying more general > firmwares. > > First, add support for having multiple alternate firmware paths. Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Acked-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/firmware.c | 75 ++++++++++++++----- > .../broadcom/brcm80211/brcmfmac/firmware.h | 2 + > 2 files changed, 59 insertions(+), 18 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > index 0497b721136a..7570dbf22cdd 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > @@ -427,6 +427,8 @@ void brcmf_fw_nvram_free(void *nvram) > struct brcmf_fw { > struct device *dev; > struct brcmf_fw_request *req; > + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]; > + int alt_index; > u32 curpos; > void (*done)(struct device *dev, int err, struct brcmf_fw_request *req); > }; > @@ -592,14 +594,18 @@ static int brcmf_fw_complete_request(const struct firmware *fw, > return (cur->flags & BRCMF_FW_REQF_OPTIONAL) ? 0 : ret; > } > > -static char *brcm_alt_fw_path(const char *path, const char *board_type) > +static int brcm_alt_fw_paths(const char *path, const char *board_type, > + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) > { > char alt_path[BRCMF_FW_NAME_LEN]; > const char *suffix; > > + memset(alt_paths, 0, array_size(sizeof(*alt_paths), > + BRCMF_FW_MAX_ALT_PATHS)); > + > suffix = strrchr(path, '.'); > if (!suffix || suffix == path) > - return NULL; > + return -EINVAL; > > /* strip extension at the end */ > strscpy(alt_path, path, BRCMF_FW_NAME_LEN); > @@ -609,7 +615,18 @@ static char *brcm_alt_fw_path(const char *path, const char *board_type) > strlcat(alt_path, board_type, BRCMF_FW_NAME_LEN); > strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); > > - return kstrdup(alt_path, GFP_KERNEL); > + alt_paths[0] = kstrdup(alt_path, GFP_KERNEL); > + > + return 0; > +} > + > +static void > +brcm_free_alt_fw_paths(const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) > +{ > + unsigned int i; > + > + for (i = 0; alt_paths[i]; i++) > + kfree(alt_paths[i]); > } > > static int brcmf_fw_request_firmware(const struct firmware **fw, > @@ -617,19 +634,25 @@ static int brcmf_fw_request_firmware(const struct firmware **fw, > { > struct brcmf_fw_item *cur = &fwctx->req->items[fwctx->curpos]; > int ret; > + unsigned int i; > > /* Files can be board-specific, first try a board-specific path */ > if (fwctx->req->board_type) { > - char *alt_path; > + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]; > > - alt_path = brcm_alt_fw_path(cur->path, fwctx->req->board_type); > - if (!alt_path) > + if (brcm_alt_fw_paths(cur->path, fwctx->req->board_type, > + alt_paths) != 0) > goto fallback; > > - ret = request_firmware(fw, alt_path, fwctx->dev); > - kfree(alt_path); > - if (ret == 0) > - return ret; > + for (i = 0; i < BRCMF_FW_MAX_ALT_PATHS && alt_paths[i]; i++) { > + ret = firmware_request_nowarn(fw, alt_paths[i], > + fwctx->dev); > + if (ret == 0) { > + brcm_free_alt_fw_paths(alt_paths); > + return ret; > + } > + } > + brcm_free_alt_fw_paths(alt_paths); > } > > fallback: > @@ -641,6 +664,8 @@ static void brcmf_fw_request_done(const struct firmware *fw, void *ctx) > struct brcmf_fw *fwctx = ctx; > int ret; > > + brcm_free_alt_fw_paths(fwctx->alt_paths); > + > ret = brcmf_fw_complete_request(fw, fwctx); > > while (ret == 0 && ++fwctx->curpos < fwctx->req->n_items) { > @@ -662,13 +687,27 @@ static void brcmf_fw_request_done_alt_path(const struct firmware *fw, void *ctx) > struct brcmf_fw_item *first = &fwctx->req->items[0]; > int ret = 0; > > - /* Fall back to canonical path if board firmware not found */ > - if (!fw) > + if (fw) { > + brcmf_fw_request_done(fw, ctx); > + return; > + } > + > + fwctx->alt_index++; > + if (fwctx->alt_index < BRCMF_FW_MAX_ALT_PATHS && > + fwctx->alt_paths[fwctx->alt_index]) { > + /* Try the next alt firmware */ > + ret = request_firmware_nowait(THIS_MODULE, true, > + fwctx->alt_paths[fwctx->alt_index], > + fwctx->dev, GFP_KERNEL, fwctx, > + brcmf_fw_request_done_alt_path); > + } else { > + /* Fall back to canonical path if board firmware not found */ > ret = request_firmware_nowait(THIS_MODULE, true, first->path, > fwctx->dev, GFP_KERNEL, fwctx, > brcmf_fw_request_done); > + } > > - if (fw || ret < 0) > + if (ret < 0) > brcmf_fw_request_done(fw, ctx); > } > > @@ -693,7 +732,6 @@ int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req, > { > struct brcmf_fw_item *first = &req->items[0]; > struct brcmf_fw *fwctx; > - char *alt_path; > int ret; > > brcmf_dbg(TRACE, "enter: dev=%s\n", dev_name(dev)); > @@ -712,12 +750,13 @@ int brcmf_fw_get_firmwares(struct device *dev, struct brcmf_fw_request *req, > fwctx->done = fw_cb; > > /* First try alternative board-specific path if any */ > - alt_path = brcm_alt_fw_path(first->path, fwctx->req->board_type); > - if (alt_path) { > - ret = request_firmware_nowait(THIS_MODULE, true, alt_path, > + if (brcm_alt_fw_paths(first->path, req->board_type, > + fwctx->alt_paths) == 0) { > + fwctx->alt_index = 0; > + ret = request_firmware_nowait(THIS_MODULE, true, > + fwctx->alt_paths[0], > fwctx->dev, GFP_KERNEL, fwctx, > brcmf_fw_request_done_alt_path); > - kfree(alt_path); > } else { > ret = request_firmware_nowait(THIS_MODULE, true, first->path, > fwctx->dev, GFP_KERNEL, fwctx, > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h > index e290dec9c53d..7f4e6e359c82 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h > @@ -11,6 +11,8 @@ > > #define BRCMF_FW_DEFAULT_PATH "brcm/" > > +#define BRCMF_FW_MAX_ALT_PATHS 8 > + Any motivation to have 8 here today? In patch #9 I see a list of 6 paths in the commit message so you need 6 and rounded up here to power of 2? > /** > * struct brcmf_firmware_mapping - Used to map chipid/revmask to firmware > * filename and nvram filename. Each bus type implementation should create
On 1/4/2022 8:26 AM, Hector Martin wrote: > Now that the firmware fetcher can handle per-board CLM files, load the > CLM blob alongside the other firmware files and change the bus API to > just return the existing blob, instead of fetching the filename. > > This enables per-board CLM blobs, which are required on Apple platforms. Looks good to me. Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Acked-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/bus.h | 19 ++++++--- > .../broadcom/brcm80211/brcmfmac/common.c | 12 +----- > .../broadcom/brcm80211/brcmfmac/pcie.c | 39 ++++++++++++------- > .../broadcom/brcm80211/brcmfmac/sdio.c | 36 ++++++++++------- > .../broadcom/brcm80211/brcmfmac/sdio.h | 2 + > .../broadcom/brcm80211/brcmfmac/usb.c | 23 +++-------- > 6 files changed, 69 insertions(+), 62 deletions(-)
On 2022/01/06 19:19, Arend van Spriel wrote: > On 1/4/2022 8:26 AM, Hector Martin wrote: >> Teach brcm_alt_fw_paths to correctly split off variable length >> extensions, and enable alt firmware lookups for the CLM blob firmware >> requests. >> >> Apple platforms have per-board CLM blob files. > > Are you sure? I am not involved in development for Apple platforms, but > in general we build a CLM blob specific for a chip revision. As always > with the blobs they are created at a certain point in time and that is > mostly why you need another one for a newer platform. Apple tends to do > things a bit different so you could be right though. Anyway, despite my > doubts on this it does not change the need for per-board firmware files. Yup, I'm sure. The 2021 MacBook Pro 14" and the MacBook Pro 16", both using BCM4387 and both released simultaneously, have different CLM blobs; they're even a significantly different size. Running `strings` on the files yields: CLM DATA Oly.Maldives 1.61.4 ClmImport: 1.63.1 v3 Final 210923 CLM DATA Oly.Madagascar 1.61.4 ClmImport: 1.63.1 v4 Final 210923 The data shows significant differences and since the file format is opaque I can't know what's going on. Even if it's safe to use one file for both, unless there is some way for me to programmatically identify this fact so I can incorporate that logic into my firmware copier, I would much rather just keep them separate like Apple does. > So all firmware files are attempted with board-specific path now. Yes, I figured I'd keep things uniform. Technically for Apple platforms the CLM blob and firmware are only per-board and possibly per-antenna (they don't need the module variants, those are for nvram only), but there's no real harm in unifying it and using the same firmware naming alt path list for everything.
On 2022/01/06 18:56, Arend van Spriel wrote: > On 1/4/2022 8:26 AM, Hector Martin wrote: >> Move one of the declarations from sdio.c to pcie.c, since it makes no >> sense in the former (SDIO support is optional), and add missing ones. > > Actually, any bus is optional so each bus should indeed declare the > applicable firmware names/patterns. Of course; I didn't mean *only* SDIO support is optional :) >> +/* firmware config files */ >> +MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-pcie.txt"); > > what is this one for? Those would be covered by the specific > BRCMF_FW_DEF() macro instances, no? The BRCMF_FW_DEF() macro only declares the .bin file; BRCMF_FW_CLM_DEF declares that and the .clm_blob. Neither declare the NVRAM .txt.
On 2022/01/06 19:43, Arend van Spriel wrote: >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h >> index e290dec9c53d..7f4e6e359c82 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.h >> @@ -11,6 +11,8 @@ >> >> #define BRCMF_FW_DEFAULT_PATH "brcm/" >> >> +#define BRCMF_FW_MAX_ALT_PATHS 8 >> + > > Any motivation to have 8 here today? In patch #9 I see a list of 6 paths > in the commit message so you need 6 and rounded up here to power of 2? > Heh, yeah, that's just my powers-of-two-are-nice-numbers habit. I can drop it down to 6 if you prefer.
On 1/4/2022 8:26 AM, Hector Martin wrote: > In order to make use of the multiple alt_path functionality, change > board_type to an array. Bus drivers can pass in a NULL-terminated list > of board type strings to try for the firmware fetch. Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Acked-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/firmware.c | 35 ++++++++++++------- > .../broadcom/brcm80211/brcmfmac/firmware.h | 2 +- > .../broadcom/brcm80211/brcmfmac/pcie.c | 4 ++- > .../broadcom/brcm80211/brcmfmac/sdio.c | 2 +- > 4 files changed, 27 insertions(+), 16 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > index 7570dbf22cdd..054ea3ed133e 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c > @@ -594,28 +594,39 @@ static int brcmf_fw_complete_request(const struct firmware *fw, > return (cur->flags & BRCMF_FW_REQF_OPTIONAL) ? 0 : ret; > } > > -static int brcm_alt_fw_paths(const char *path, const char *board_type, > +static int brcm_alt_fw_paths(const char *path, struct brcmf_fw *fwctx, > const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) > { > + const char **board_types = fwctx->req->board_types; > + unsigned int i; > char alt_path[BRCMF_FW_NAME_LEN]; > const char *suffix; [...] > + for (i = 0; i < BRCMF_FW_MAX_ALT_PATHS; i++) { > + if (!board_types[i]) > + break; > > - strlcat(alt_path, ".", BRCMF_FW_NAME_LEN); > - strlcat(alt_path, board_type, BRCMF_FW_NAME_LEN); > - strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); > + /* strip extension at the end */ > + strscpy(alt_path, path, BRCMF_FW_NAME_LEN); > + alt_path[suffix - path] = 0; > > - alt_paths[0] = kstrdup(alt_path, GFP_KERNEL); > + strlcat(alt_path, ".", BRCMF_FW_NAME_LEN); > + strlcat(alt_path, board_types[i], BRCMF_FW_NAME_LEN); > + strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); > + > + alt_paths[i] = kstrdup(alt_path, GFP_KERNEL); > + brcmf_dbg(TRACE, "FW alt path: %s\n", alt_paths[i]); Could use alt_path in the debug print thus avoiding additional array access (working hard to find those nits to pick ;-) ). > + }
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; > +}
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.
On 04/01/2022 23.12, Andy Shevchenko wrote: > On Tue, Jan 4, 2022 at 9:29 AM Hector Martin <marcan@marcan.st> wrote: >> >> The driver was enabling IRQs before the message processing was >> initialized. This could cause IRQs to come in too early and crash the >> driver. Instead, move the IRQ enable and hostready to a bus preinit >> function, at which point everything is properly initialized. >> >> Fixes: 9e37f045d5e7 ("brcmfmac: Adding PCIe bus layer support.") > > You should gather fixes at the beginning of the series, and even > possible to send them as a separate series. In the current state it's > unclear if there are dependencies on your new feature (must not be for > fixes that meant to be backported). > Thanks, I wasn't sure what order you wanted those in. I'll put them at the top for v3. I think none of those should have any dependencies on the rest of the patches, modulo some trivial rebase wrangling.
On 04/01/2022 23.24, Andy Shevchenko wrote: > On Tue, Jan 4, 2022 at 9:28 AM Hector Martin <marcan@marcan.st> wrote: >> + /* Example: apple,shikoku-RASP-m-6.11-X3 */ >> + len = (strlen(devinfo->settings->board_type) + 1 + >> + strlen(devinfo->otp.module) + 1 + >> + strlen(devinfo->otp.vendor) + 1 + >> + strlen(devinfo->otp.version) + 1 + >> + strlen(devinfo->settings->antenna_sku) + 1); > > NIH devm_kasprrintf() ? This one builds it incrementally, but you're right, kasprintf is probably more readable here and fewer lines even though it'll duplicate all the previous argument references for each pattern. I'll redo it with devm_kasprintf(). > >> + /* apple,shikoku */ >> + fwreq->board_types[5] = devinfo->settings->board_type; >> + >> + buf = devm_kzalloc(&devinfo->pdev->dev, len, GFP_KERNEL); >> + >> + strscpy(buf, devinfo->settings->board_type, len); >> + strlcat(buf, "-", len); >> + strlcat(buf, devinfo->settings->antenna_sku, len); >> + /* apple,shikoku-X3 */ >> + fwreq->board_types[4] = devm_kstrdup(&devinfo->pdev->dev, buf, >> + GFP_KERNEL); >> + >> + strscpy(buf, devinfo->settings->board_type, len); >> + strlcat(buf, "-", len); >> + strlcat(buf, devinfo->otp.module, len); >> + /* apple,shikoku-RASP */ >> + fwreq->board_types[3] = devm_kstrdup(&devinfo->pdev->dev, buf, >> + GFP_KERNEL); >> + >> + strlcat(buf, "-", len); >> + strlcat(buf, devinfo->otp.vendor, len); >> + /* apple,shikoku-RASP-m */ >> + fwreq->board_types[2] = devm_kstrdup(&devinfo->pdev->dev, buf, >> + GFP_KERNEL); >> + >> + strlcat(buf, "-", len); >> + strlcat(buf, devinfo->otp.version, len); >> + /* apple,shikoku-RASP-m-6.11 */ >> + fwreq->board_types[1] = devm_kstrdup(&devinfo->pdev->dev, buf, >> + GFP_KERNEL); >> + >> + strlcat(buf, "-", len); >> + strlcat(buf, devinfo->settings->antenna_sku, len); >> + /* apple,shikoku-RASP-m-6.11-X3 */ >> + fwreq->board_types[0] = buf; >
On Wed, Jan 5, 2022 at 3:26 PM Hector Martin <marcan@marcan.st> wrote: > On 04/01/2022 23.23, Andy Shevchenko wrote: > > On Tue, Jan 4, 2022 at 9:29 AM Hector Martin <marcan@marcan.st> wrote: ... > >> +#define BRCMF_FW_MACADDR_FMT "macaddr=%pM" > >> + snprintf(&nvp->nvram[nvp->nvram_len], BRCMF_FW_MACADDR_LEN + 1, > >> + BRCMF_FW_MACADDR_FMT, mac); > > > > Please, avoid using implict format string, it's dangerous from security p.o.v. > > What do you mean by implicit format string? When I read the above code I feel uncomfortable because no-one can see (without additional action and more reading and checking) if it's correct or not. This is potential to be error prone. > The format string is at the > top of the file and its length is right next to it, which makes it > harder for them to accidentally fall out of sync. It is not an argument. Just you may do the same in the code directly and more explicitly: Also you don't check the return code of snprintf which means that you don't care about the result, which seems to me wrong approach. If you don't care about the result, so it means it's not very important, right? > +#define BRCMF_FW_MACADDR_FMT "macaddr=%pM" > +#define BRCMF_FW_MACADDR_LEN (7 + ETH_ALEN * 3)
05.01.2022 16:22, Hector Martin пишет: > On 05/01/2022 07.09, Dmitry Osipenko wrote: >> 04.01.2022 11:43, Hector Martin пишет: >>>>> +static int brcm_alt_fw_paths(const char *path, const char *board_type, >>>>> + const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS])> { >>>>> char alt_path[BRCMF_FW_NAME_LEN]; >>>>> const char *suffix; >>>>> >>>>> + memset(alt_paths, 0, array_size(sizeof(*alt_paths), >>>>> + BRCMF_FW_MAX_ALT_PATHS)); >>>> You don't need to use array_size() since size of a fixed array is >>>> already known. >>>> >>>> memset(alt_paths, 0, sizeof(alt_paths)); >>> It's a function argument, so that doesn't work and actually throws a >>> warning. Array function argument notation is informative only; they >>> behave strictly equivalent to pointers. Try it: >>> >>> $ cat test.c >>> #include <stdio.h> >>> >>> void foo(char x[42]) >>> { >>> printf("%ld\n", sizeof(x)); >>> } >>> >>> int main() { >>> char x[42]; >>> >>> foo(x); >>> } >>> $ gcc test.c >>> test.c: In function ‘foo’: >>> test.c:5:31: warning: ‘sizeof’ on array function parameter ‘x’ will >>> return size of ‘char *’ [-Wsizeof-array-argument] >>> 5 | printf("%ld\n", sizeof(x)); >>> | ^ >>> test.c:3:15: note: declared here >>> 3 | void foo(char x[42]) >>> | ~~~~~^~~~~ >>> $ ./a.out >>> 8 >> >> Then please use "const char **alt_paths" for the function argument to >> make code cleaner and add another argument to pass the number of array >> elements. > > So you want me to do the ARRAY_SIZE at the caller side then? > >> >> static int brcm_alt_fw_paths(const char *path, const char *board_type, >> const char **alt_paths, unsigned int num_paths) >> { >> size_t alt_paths_size = array_size(sizeof(*alt_paths), num_paths); >> >> memset(alt_paths, 0, alt_paths_size); >> } >> >> ... >> >> Maybe even better create a dedicated struct for the alt_paths: >> >> struct brcmf_fw_alt_paths { >> const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]; >> unsigned int index; >> }; >> >> and then use the ".index" in the brcm_free_alt_fw_paths(). I suppose >> this will make code a bit nicer and easier to follow. >> > > I'm confused; the array size is constant. What would index contain and > why would would brcm_free_alt_fw_paths use it? Just as an iterator > variable instead of using a local variable? Or do you mean count? Yes, use index for the count of active entries in the alt_paths[]. for (i = 0; i < alt_paths.index; i++) kfree(alt_paths.path[i]); alt_paths.index = 0; or while (alt_paths.index) kfree(alt_paths.path[--alt_paths.index]); > Though, to be honest, at this point I'm considering rethinking the whole > patch for this mechanism because I'm not terribly happy with the current > approach and clearly you aren't either :-) Maybe it makes more sense to > stop trying to compute all the alt_paths ahead of time, and just have > the function compute a single one to be used just-in-time at firmware > request time, and just iterate over board_types. > The just-in-time approach sounds like a good idea.
On Thu, Jan 6, 2022 at 7:40 PM Dmitry Osipenko <digetx@gmail.com> wrote: > 05.01.2022 16:22, Hector Martin пишет: > > On 05/01/2022 07.09, Dmitry Osipenko wrote: ... > > I'm confused; the array size is constant. What would index contain and > > why would would brcm_free_alt_fw_paths use it? Just as an iterator > > variable instead of using a local variable? Or do you mean count? > > Yes, use index for the count of active entries in the alt_paths[]. > > for (i = 0; i < alt_paths.index; i++) > kfree(alt_paths.path[i]); > > alt_paths.index = 0; > > or > > while (alt_paths.index) > kfree(alt_paths.path[--alt_paths.index]); Usual pattern is while (x--) kfree(x); easier to read, extend (if needed).
On 2022/01/06 23:20, Andy Shevchenko wrote: > On Wed, Jan 5, 2022 at 3:26 PM Hector Martin <marcan@marcan.st> wrote: >> On 04/01/2022 23.23, Andy Shevchenko wrote: >>> On Tue, Jan 4, 2022 at 9:29 AM Hector Martin <marcan@marcan.st> wrote: > > ... > >>>> +#define BRCMF_FW_MACADDR_FMT "macaddr=%pM" > >>>> + snprintf(&nvp->nvram[nvp->nvram_len], BRCMF_FW_MACADDR_LEN + 1, >>>> + BRCMF_FW_MACADDR_FMT, mac); >>> >>> Please, avoid using implict format string, it's dangerous from security p.o.v. >> >> What do you mean by implicit format string? > > When I read the above code I feel uncomfortable because no-one can see > (without additional action and more reading and checking) if it's > correct or not. This is potential to be error prone. > >> The format string is at the >> top of the file and its length is right next to it, which makes it >> harder for them to accidentally fall out of sync. > > It is not an argument. Just you may do the same in the code directly > and more explicitly: The point is that BRCMF_FW_MACADDR_LEN and BRCMF_FW_MACADDR_FMT need to be in sync, and BRCMF_FW_MACADDR_LEN is used in two different places. If I inline the format string into the code, someone could change it without changing the length, or changing the length inline only next to the format string. Then we overflow the NVRAM buffer because the allocation is not sized properly. By having them as defines, it is obvious that they go together, and if one changes the other one has to change too, and the nvram allocation can't end up improperly sized as long as they are kept in sync. > Also you don't check the return code of snprintf which means that you > don't care about the result, which seems to me wrong approach. If you > don't care about the result, so it means it's not very important, > right? > That snprintf can never fail as long as the format string/length are in sync. I'll make it BUG_ON(... != size), so it complains loudly if someone screws up the format string in the future.
On 2022/01/04 20:28, Andy Shevchenko wrote: > On Tue, Jan 4, 2022 at 9:28 AM Hector Martin <marcan@marcan.st> wrote: >> >> In order to make use of the multiple alt_path functionality, change >> board_type to an array. Bus drivers can pass in a NULL-terminated list >> of board type strings to try for the firmware fetch. > >> + /* strip extension at the end */ >> + strscpy(alt_path, path, BRCMF_FW_NAME_LEN); >> + alt_path[suffix - path] = 0; >> >> - alt_paths[0] = kstrdup(alt_path, GFP_KERNEL); >> + strlcat(alt_path, ".", BRCMF_FW_NAME_LEN); >> + strlcat(alt_path, board_types[i], BRCMF_FW_NAME_LEN); >> + strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); >> + >> + alt_paths[i] = kstrdup(alt_path, GFP_KERNEL); >> + brcmf_dbg(TRACE, "FW alt path: %s\n", alt_paths[i]); > > Consider replacing these string manipulations with kasprintf(). > Done, thanks!
06.01.2022 20:58, Andy Shevchenko пишет: > On Thu, Jan 6, 2022 at 7:40 PM Dmitry Osipenko <digetx@gmail.com> wrote: >> 05.01.2022 16:22, Hector Martin пишет: >>> On 05/01/2022 07.09, Dmitry Osipenko wrote: > > ... > >>> I'm confused; the array size is constant. What would index contain and >>> why would would brcm_free_alt_fw_paths use it? Just as an iterator >>> variable instead of using a local variable? Or do you mean count? >> >> Yes, use index for the count of active entries in the alt_paths[]. >> >> for (i = 0; i < alt_paths.index; i++) >> kfree(alt_paths.path[i]); >> >> alt_paths.index = 0; >> >> or >> >> while (alt_paths.index) >> kfree(alt_paths.path[--alt_paths.index]); > > Usual pattern is > > while (x--) > kfree(x); > > easier to read, extend (if needed). > That is indeed a usual patter for the driver removal code paths. I didn't like to have index of struct brcmf_fw underflowed, but I see now that fwctx is dynamically created and freed during driver probe, so it should be fine to use that usual pattern here too.
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.
On 2022/01/04 20:17, Andy Shevchenko wrote: > On Tue, Jan 4, 2022 at 9:28 AM Hector Martin <marcan@marcan.st> wrote: >> >> On Apple ARM64 platforms, firmware selection requires two properties >> that come from system firmware: the module-instance (aka "island", a >> codename representing a given hardware platform) and the antenna-sku. >> We map Apple's module codenames to board_types in the form >> "apple,<module-instance>". >> >> The mapped board_type is added to the DTS file in that form, while the >> antenna-sku is forwarded by our bootloader from the Apple Device Tree >> into the FDT. Grab them from the DT so firmware selection can use >> them. > >> + /* Apple ARM64 platforms have their own idea of board type, passed in >> + * via the device tree. They also have an antenna SKU parameter >> + */ >> + if (!of_property_read_string(np, "brcm,board-type", &prop)) >> + settings->board_type = devm_kstrdup(dev, prop, GFP_KERNEL); >> + >> + if (!of_property_read_string(np, "apple,antenna-sku", &prop)) >> + settings->antenna_sku = devm_kstrdup(dev, prop, GFP_KERNEL); > > No error checks? > But hold on, why do you need to copy them? Are you expecting this to be in DTO? Yeah, I was wondering about that... indeed it shouldn't be necessary to copy them.
On 2022/01/06 21:16, Arend van Spriel wrote: > On 1/4/2022 8:26 AM, Hector Martin wrote: >> In order to make use of the multiple alt_path functionality, change >> board_type to an array. Bus drivers can pass in a NULL-terminated list >> of board type strings to try for the firmware fetch. > > Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> >> Acked-by: Linus Walleij <linus.walleij@linaro.org> >> Signed-off-by: Hector Martin <marcan@marcan.st> >> --- >> .../broadcom/brcm80211/brcmfmac/firmware.c | 35 ++++++++++++------- >> .../broadcom/brcm80211/brcmfmac/firmware.h | 2 +- >> .../broadcom/brcm80211/brcmfmac/pcie.c | 4 ++- >> .../broadcom/brcm80211/brcmfmac/sdio.c | 2 +- >> 4 files changed, 27 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >> index 7570dbf22cdd..054ea3ed133e 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >> @@ -594,28 +594,39 @@ static int brcmf_fw_complete_request(const struct firmware *fw, >> return (cur->flags & BRCMF_FW_REQF_OPTIONAL) ? 0 : ret; >> } >> >> -static int brcm_alt_fw_paths(const char *path, const char *board_type, >> +static int brcm_alt_fw_paths(const char *path, struct brcmf_fw *fwctx, >> const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) >> { >> + const char **board_types = fwctx->req->board_types; >> + unsigned int i; >> char alt_path[BRCMF_FW_NAME_LEN]; >> const char *suffix; > > [...] > >> + for (i = 0; i < BRCMF_FW_MAX_ALT_PATHS; i++) { >> + if (!board_types[i]) >> + break; >> >> - strlcat(alt_path, ".", BRCMF_FW_NAME_LEN); >> - strlcat(alt_path, board_type, BRCMF_FW_NAME_LEN); >> - strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); >> + /* strip extension at the end */ >> + strscpy(alt_path, path, BRCMF_FW_NAME_LEN); >> + alt_path[suffix - path] = 0; >> >> - alt_paths[0] = kstrdup(alt_path, GFP_KERNEL); >> + strlcat(alt_path, ".", BRCMF_FW_NAME_LEN); >> + strlcat(alt_path, board_types[i], BRCMF_FW_NAME_LEN); >> + strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); >> + >> + alt_paths[i] = kstrdup(alt_path, GFP_KERNEL); >> + brcmf_dbg(TRACE, "FW alt path: %s\n", alt_paths[i]); > > Could use alt_path in the debug print thus avoiding additional array > access (working hard to find those nits to pick ;-) ). > So you're saying my code is so good you have to resort to nits on this level to make it clear you read it, right? ;-)
On January 7, 2022 5:02:13 AM Hector Martin <marcan@marcan.st> wrote: > On 2022/01/06 21:16, Arend van Spriel wrote: >> On 1/4/2022 8:26 AM, Hector Martin wrote: >>> In order to make use of the multiple alt_path functionality, change >>> board_type to an array. Bus drivers can pass in a NULL-terminated list >>> of board type strings to try for the firmware fetch. >> >> Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> >>> Acked-by: Linus Walleij <linus.walleij@linaro.org> >>> Signed-off-by: Hector Martin <marcan@marcan.st> >>> --- >>> .../broadcom/brcm80211/brcmfmac/firmware.c | 35 ++++++++++++------- >>> .../broadcom/brcm80211/brcmfmac/firmware.h | 2 +- >>> .../broadcom/brcm80211/brcmfmac/pcie.c | 4 ++- >>> .../broadcom/brcm80211/brcmfmac/sdio.c | 2 +- >>> 4 files changed, 27 insertions(+), 16 deletions(-) >>> >>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >>> index 7570dbf22cdd..054ea3ed133e 100644 >>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >>> @@ -594,28 +594,39 @@ static int brcmf_fw_complete_request(const struct >>> firmware *fw, >>> return (cur->flags & BRCMF_FW_REQF_OPTIONAL) ? 0 : ret; >>> } >>> >>> -static int brcm_alt_fw_paths(const char *path, const char *board_type, >>> +static int brcm_alt_fw_paths(const char *path, struct brcmf_fw *fwctx, >>> const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) >>> { >>> + const char **board_types = fwctx->req->board_types; >>> + unsigned int i; >>> char alt_path[BRCMF_FW_NAME_LEN]; >>> const char *suffix; >> >> [...] >> >>> + for (i = 0; i < BRCMF_FW_MAX_ALT_PATHS; i++) { >>> + if (!board_types[i]) >>> + break; >>> >>> - strlcat(alt_path, ".", BRCMF_FW_NAME_LEN); >>> - strlcat(alt_path, board_type, BRCMF_FW_NAME_LEN); >>> - strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); >>> + /* strip extension at the end */ >>> + strscpy(alt_path, path, BRCMF_FW_NAME_LEN); >>> + alt_path[suffix - path] = 0; >>> >>> - alt_paths[0] = kstrdup(alt_path, GFP_KERNEL); >>> + strlcat(alt_path, ".", BRCMF_FW_NAME_LEN); >>> + strlcat(alt_path, board_types[i], BRCMF_FW_NAME_LEN); >>> + strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); >>> + >>> + alt_paths[i] = kstrdup(alt_path, GFP_KERNEL); >>> + brcmf_dbg(TRACE, "FW alt path: %s\n", alt_paths[i]); >> >> Could use alt_path in the debug print thus avoiding additional array >> access (working hard to find those nits to pick ;-) ). > > So you're saying my code is so good you have to resort to nits on this > level to make it clear you read it, right? ;-) Don't read too much into this :-p Actually never liked the alt_path approach, but didn't come up with a better solution. Regards, Arend
On 2022/01/07 15:17, Arend Van Spriel wrote: > On January 7, 2022 5:02:13 AM Hector Martin <marcan@marcan.st> wrote: > >> On 2022/01/06 21:16, Arend van Spriel wrote: >>> On 1/4/2022 8:26 AM, Hector Martin wrote: >>>> In order to make use of the multiple alt_path functionality, change >>>> board_type to an array. Bus drivers can pass in a NULL-terminated list >>>> of board type strings to try for the firmware fetch. >>> >>> Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> >>>> Acked-by: Linus Walleij <linus.walleij@linaro.org> >>>> Signed-off-by: Hector Martin <marcan@marcan.st> >>>> --- >>>> .../broadcom/brcm80211/brcmfmac/firmware.c | 35 ++++++++++++------- >>>> .../broadcom/brcm80211/brcmfmac/firmware.h | 2 +- >>>> .../broadcom/brcm80211/brcmfmac/pcie.c | 4 ++- >>>> .../broadcom/brcm80211/brcmfmac/sdio.c | 2 +- >>>> 4 files changed, 27 insertions(+), 16 deletions(-) >>>> >>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >>>> index 7570dbf22cdd..054ea3ed133e 100644 >>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c >>>> @@ -594,28 +594,39 @@ static int brcmf_fw_complete_request(const struct >>>> firmware *fw, >>>> return (cur->flags & BRCMF_FW_REQF_OPTIONAL) ? 0 : ret; >>>> } >>>> >>>> -static int brcm_alt_fw_paths(const char *path, const char *board_type, >>>> +static int brcm_alt_fw_paths(const char *path, struct brcmf_fw *fwctx, >>>> const char *alt_paths[BRCMF_FW_MAX_ALT_PATHS]) >>>> { >>>> + const char **board_types = fwctx->req->board_types; >>>> + unsigned int i; >>>> char alt_path[BRCMF_FW_NAME_LEN]; >>>> const char *suffix; >>> >>> [...] >>> >>>> + for (i = 0; i < BRCMF_FW_MAX_ALT_PATHS; i++) { >>>> + if (!board_types[i]) >>>> + break; >>>> >>>> - strlcat(alt_path, ".", BRCMF_FW_NAME_LEN); >>>> - strlcat(alt_path, board_type, BRCMF_FW_NAME_LEN); >>>> - strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); >>>> + /* strip extension at the end */ >>>> + strscpy(alt_path, path, BRCMF_FW_NAME_LEN); >>>> + alt_path[suffix - path] = 0; >>>> >>>> - alt_paths[0] = kstrdup(alt_path, GFP_KERNEL); >>>> + strlcat(alt_path, ".", BRCMF_FW_NAME_LEN); >>>> + strlcat(alt_path, board_types[i], BRCMF_FW_NAME_LEN); >>>> + strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); >>>> + >>>> + alt_paths[i] = kstrdup(alt_path, GFP_KERNEL); >>>> + brcmf_dbg(TRACE, "FW alt path: %s\n", alt_paths[i]); >>> >>> Could use alt_path in the debug print thus avoiding additional array >>> access (working hard to find those nits to pick ;-) ). >> >> So you're saying my code is so good you have to resort to nits on this >> level to make it clear you read it, right? ;-) > > Don't read too much into this :-p Actually never liked the alt_path > approach, but didn't come up with a better solution. Yeah, it's not the prettiest approach. I redid this part of the patchset for v3 though, as I mentioned to Dmitry. Now I just iterate over board_types, which ends up being a lot cleaner as far as the changes required.
On Fri, Jan 7, 2022 at 5:12 AM Dmitry Osipenko <digetx@gmail.com> wrote: > 06.01.2022 20:58, Andy Shevchenko пишет: > > On Thu, Jan 6, 2022 at 7:40 PM Dmitry Osipenko <digetx@gmail.com> wrote: > >> 05.01.2022 16:22, Hector Martin пишет: ... > >> while (alt_paths.index) > >> kfree(alt_paths.path[--alt_paths.index]); > > > > Usual pattern is > > > > while (x--) > > kfree(x); I have to elaborate that my point is to have postdecrement in the while() instead of doing predecrement in its body. So the above example will look while (alt_paths.index--) kfree(alt_paths.path[alt_paths.index]); > > easier to read, extend (if needed). > > That is indeed a usual patter for the driver removal code paths. I > didn't like to have index of struct brcmf_fw underflowed, but I see now > that fwctx is dynamically created and freed during driver probe, so it > should be fine to use that usual pattern here too.
On 1/4/2022 8:26 AM, Hector Martin wrote: > On Apple platforms, firmware selection uses the following elements: > > Property Example Source > ============== ======= ======================== > * Chip name 4378 Device ID > * Chip revision B1 OTP > * Platform shikoku DT (ARM64) or ACPI (x86) > * Module type RASP OTP > * Module vendor m OTP > * Module version 6.11 OTP > * Antenna SKU X3 DT (ARM64) or ACPI (x86) > > In macOS, these firmwares are stored using filenames in this format > under /usr/share/firmware/wifi: > > C-4378__s-B1/P-shikoku-X3_M-RASP_V-m__m-6.11.txt > > To prepare firmwares for Linux, we rename these to a scheme following > the existing brcmfmac convention: > > brcmfmac<chip><lower(rev)>-pcie.apple,<platform>-<mod_type>-\ > <mod_vendor>-<mod_version>-<antenna_sku>.txt > > The NVRAM uses all the components, while the firmware and CLM blob only > use the chip/revision/platform/antenna_sku: > > brcmfmac<chip><lower(rev)>-pcie.apple,<platform>-<antenna_sku>.bin > > e.g. > > brcm/brcmfmac4378b1-pcie.apple,shikoku-RASP-m-6.11-X3.txt > brcm/brcmfmac4378b1-pcie.apple,shikoku-X3.bin > > In addition, since there are over 1000 files in total, many of which are > symlinks or outright duplicates, we deduplicate and prune the firmware > tree to reduce firmware filenames to fewer dimensions. For example, the > shikoku platform (MacBook Air M1 2020) simplifies to just 4 files: > > brcm/brcmfmac4378b1-pcie.apple,shikoku.clm_blob > brcm/brcmfmac4378b1-pcie.apple,shikoku.bin > brcm/brcmfmac4378b1-pcie.apple,shikoku-RASP-m.txt > brcm/brcmfmac4378b1-pcie.apple,shikoku-RASP-u.txt > > This reduces the total file count to around 170, of which 75 are > symlinks and 95 are regular files: 7 firmware blobs, 27 CLM blobs, and > 61 NVRAM config files. We also slightly process NVRAM files to correct > some formatting issues. > > To handle this, the driver must try the following path formats when > looking for firmware files: > > brcm/brcmfmac4378b1-pcie.apple,shikoku-RASP-m-6.11-X3.txt > brcm/brcmfmac4378b1-pcie.apple,shikoku-RASP-m-6.11.txt > brcm/brcmfmac4378b1-pcie.apple,shikoku-RASP-m.txt > brcm/brcmfmac4378b1-pcie.apple,shikoku-RASP.txt > brcm/brcmfmac4378b1-pcie.apple,shikoku-X3.txt * > brcm/brcmfmac4378b1-pcie.apple,shikoku.txt > > * Not relevant for NVRAM, only for firmware/CLM. > > The chip revision nominally comes from OTP on Apple platforms, but it > can be mapped to the PCI revision number, so we ignore the OTP revision > and continue to use the existing PCI revision mechanism to identify chip > revisions, as the driver already does for other chips. Unfortunately, > the mapping is not consistent between different chip types, so this has > to be determined experimentally. Not sure I understand this. The chip revision comes from the chipcommon register [1]. Maybe that is what you mean by "PCI revision number". For some chips it is possible OTP is used to override that. Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/pcie.c | 58 ++++++++++++++++++- > 1 file changed, 56 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > index 74c9a4f74813..250e0bd40cb3 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > @@ -2094,8 +2094,62 @@ brcmf_pcie_prepare_fw_request(struct brcmf_pciedev_info *devinfo) > fwreq->domain_nr = pci_domain_nr(devinfo->pdev->bus) + 1; > fwreq->bus_nr = devinfo->pdev->bus->number; > > - brcmf_dbg(PCIE, "Board: %s\n", devinfo->settings->board_type); > - fwreq->board_types[0] = devinfo->settings->board_type; > + /* Apple platforms with fancy firmware/NVRAM selection */ > + if (devinfo->settings->board_type && > + devinfo->settings->antenna_sku && > + devinfo->otp.valid) { > + char *buf; > + int len; > + > + brcmf_dbg(PCIE, "Apple board: %s\n", > + devinfo->settings->board_type); maybe good to use local reference for devinfo->settings->board_type, which is used several times below. > + > + /* Example: apple,shikoku-RASP-m-6.11-X3 */ > + len = (strlen(devinfo->settings->board_type) + 1 + > + strlen(devinfo->otp.module) + 1 + > + strlen(devinfo->otp.vendor) + 1 + > + strlen(devinfo->otp.version) + 1 + > + strlen(devinfo->settings->antenna_sku) + 1); > + > + /* apple,shikoku */ > + fwreq->board_types[5] = devinfo->settings->board_type; [1] https://elixir.bootlin.com/linux/latest/source/include/linux/bcma/bcma_driver_chipcommon.h#L12
On 1/4/2022 8:26 AM, Hector Martin wrote: > On Apple ARM64 platforms, firmware selection requires two properties > that come from system firmware: the module-instance (aka "island", a > codename representing a given hardware platform) and the antenna-sku. > We map Apple's module codenames to board_types in the form > "apple,<module-instance>". > > The mapped board_type is added to the DTS file in that form, while the > antenna-sku is forwarded by our bootloader from the Apple Device Tree > into the FDT. Grab them from the DT so firmware selection can use > them. Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../wireless/broadcom/brcm80211/brcmfmac/common.h | 1 + > .../net/wireless/broadcom/brcm80211/brcmfmac/of.c | 12 +++++++++++- > 2 files changed, 12 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h > index 8b5f49997c8b..d4aa25d646fe 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.h > @@ -50,6 +50,7 @@ struct brcmf_mp_device { > bool ignore_probe_fail; > struct brcmfmac_pd_cc *country_codes; > const char *board_type; > + const char *antenna_sku; > union { > struct brcmfmac_sdio_pd sdio; > } bus; > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c > index 513c7e6421b2..085d34176b78 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c > @@ -63,14 +63,24 @@ void brcmf_of_probe(struct device *dev, enum brcmf_bus_type bus_type, > { > struct brcmfmac_sdio_pd *sdio = &settings->bus.sdio; > struct device_node *root, *np = dev->of_node; > + const char *prop; > int irq; > int err; > u32 irqf; > u32 val; > > + /* Apple ARM64 platforms have their own idea of board type, passed in > + * via the device tree. They also have an antenna SKU parameter > + */ > + if (!of_property_read_string(np, "brcm,board-type", &prop)) > + settings->board_type = devm_kstrdup(dev, prop, GFP_KERNEL); > + > + if (!of_property_read_string(np, "apple,antenna-sku", &prop)) > + settings->antenna_sku = devm_kstrdup(dev, prop, GFP_KERNEL); > + > /* Set board-type to the first string of the machine compatible prop */ > root = of_find_node_by_path("/"); I assume this only returns NULL when there is no device tree. Is that a safe assumption (Rob)? If so you could bail out here if root is NULL... > - if (root) { > + if (root && !settings->board_type) { ...and only check the board_type here. Or only check board_type and lookup the root node inside this if-statement if it is not needed elsewhere in this probe function. > int i, len; > char *board_type; > const char *tmp;
On 1/4/2022 8:26 AM, Hector Martin wrote: > On Device Tree platforms, it is customary to be able to set the MAC > address via the Device Tree, as it is often stored in system firmware. > This is particularly relevant for Apple ARM64 platforms, where this > information comes from system configuration and passed through by the > bootloader into the DT. > > Implement support for this by fetching the platform MAC address and > adding or replacing the macaddr= property in nvram. This becomes the > dongle's default MAC address. > > On platforms with an SROM MAC address, this overrides it. On platforms > without one, such as Apple ARM64 devices, this is required for the > firmware to boot (it will fail if it does not have a valid MAC at all). What overrides what. Can you elaborate a bit? Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/firmware.c | 29 +++++++++++++++++-- > 1 file changed, 27 insertions(+), 2 deletions(-)
On 1/4/2022 8:26 AM, Hector Martin wrote: > Newer chips used on Apple platforms have a max_rxbufpost greater than > 512, which causes warnings when brcmf_msgbuf_rxbuf_data_fill tries to > put more entries in the ring than will fit. Increase the ring sizes > to 1024. Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/msgbuf.h | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-)
On 1/4/2022 8:26 AM, Hector Martin wrote: > The driver was enabling IRQs before the message processing was > initialized. This could cause IRQs to come in too early and crash the > driver. Instead, move the IRQ enable and hostready to a bus preinit > function, at which point everything is properly initialized. > > Fixes: 9e37f045d5e7 ("brcmfmac: Adding PCIe bus layer support.") Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../wireless/broadcom/brcm80211/brcmfmac/pcie.c | 16 +++++++++++++--- > 1 file changed, 13 insertions(+), 3 deletions(-)
On 1/4/2022 8:26 AM, Hector Martin wrote: > These newer PCIe core revisions include new sets of registers that must > be used instead of the legacy ones. Introduce a brcmf_pcie_reginfo to > hold the specific register offsets and values to use for a given > platform, and change all the register accesses to indirect through it. Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/pcie.c | 125 +++++++++++++++--- > 1 file changed, 105 insertions(+), 20 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > index 595815164e18..f3744e806157 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > @@ -118,6 +118,12 @@ static const struct brcmf_firmware_mapping brcmf_pcie_fwnames[] = { > #define BRCMF_PCIE_PCIE2REG_H2D_MAILBOX_0 0x140 > #define BRCMF_PCIE_PCIE2REG_H2D_MAILBOX_1 0x144 > > +#define BRCMF_PCIE_64_PCIE2REG_INTMASK 0xC14 > +#define BRCMF_PCIE_64_PCIE2REG_MAILBOXINT 0xC30 > +#define BRCMF_PCIE_64_PCIE2REG_MAILBOXMASK 0xC34 > +#define BRCMF_PCIE_64_PCIE2REG_H2D_MAILBOX_0 0xA20 > +#define BRCMF_PCIE_64_PCIE2REG_H2D_MAILBOX_1 0xA24 > + > #define BRCMF_PCIE2_INTA 0x01 > #define BRCMF_PCIE2_INTB 0x02 > > @@ -137,6 +143,8 @@ static const struct brcmf_firmware_mapping brcmf_pcie_fwnames[] = { > #define BRCMF_PCIE_MB_INT_D2H3_DB0 0x400000 > #define BRCMF_PCIE_MB_INT_D2H3_DB1 0x800000 > > +#define BRCMF_PCIE_MB_INT_FN0 (BRCMF_PCIE_MB_INT_FN0_0 | \ > + BRCMF_PCIE_MB_INT_FN0_1) > #define BRCMF_PCIE_MB_INT_D2H_DB (BRCMF_PCIE_MB_INT_D2H0_DB0 | \ > BRCMF_PCIE_MB_INT_D2H0_DB1 | \ > BRCMF_PCIE_MB_INT_D2H1_DB0 | \ > @@ -146,6 +154,40 @@ static const struct brcmf_firmware_mapping brcmf_pcie_fwnames[] = { > BRCMF_PCIE_MB_INT_D2H3_DB0 | \ > BRCMF_PCIE_MB_INT_D2H3_DB1) > > +#define BRCMF_PCIE_64_MB_INT_D2H0_DB0 0x1 > +#define BRCMF_PCIE_64_MB_INT_D2H0_DB1 0x2 > +#define BRCMF_PCIE_64_MB_INT_D2H1_DB0 0x4 > +#define BRCMF_PCIE_64_MB_INT_D2H1_DB1 0x8 > +#define BRCMF_PCIE_64_MB_INT_D2H2_DB0 0x10 > +#define BRCMF_PCIE_64_MB_INT_D2H2_DB1 0x20 > +#define BRCMF_PCIE_64_MB_INT_D2H3_DB0 0x40 > +#define BRCMF_PCIE_64_MB_INT_D2H3_DB1 0x80 Just an observation. So these are legacy ones with a 16 bit right shift... > +#define BRCMF_PCIE_64_MB_INT_D2H4_DB0 0x100 > +#define BRCMF_PCIE_64_MB_INT_D2H4_DB1 0x200 > +#define BRCMF_PCIE_64_MB_INT_D2H5_DB0 0x400 > +#define BRCMF_PCIE_64_MB_INT_D2H5_DB1 0x800 > +#define BRCMF_PCIE_64_MB_INT_D2H6_DB0 0x1000 > +#define BRCMF_PCIE_64_MB_INT_D2H6_DB1 0x2000 > +#define BRCMF_PCIE_64_MB_INT_D2H7_DB0 0x4000 > +#define BRCMF_PCIE_64_MB_INT_D2H7_DB1 0x8000 ...and these are new doorbell interrupts.
On 1/4/2022 8:26 AM, Hector Martin wrote: > This chip is present on Apple M1 (t8103) platforms: > > * atlantisb (apple,j274): Mac mini (M1, 2020) > * honshu (apple,j293): MacBook Pro (13-inch, M1, 2020) > * shikoku (apple,j313): MacBook Air (M1, 2020) > * capri (apple,j456): iMac (24-inch, 4x USB-C, M1, 2020) > * santorini (apple,j457): iMac (24-inch, 2x USB-C, M1, 2020) Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/chip.c | 2 ++ > drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 8 ++++++++ > .../net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h | 2 ++ > 3 files changed, 12 insertions(+) [...] > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > index f3744e806157..cc76f00724e6 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > @@ -58,6 +58,7 @@ BRCMF_FW_DEF(4365C, "brcmfmac4365c-pcie"); > BRCMF_FW_DEF(4366B, "brcmfmac4366b-pcie"); > BRCMF_FW_DEF(4366C, "brcmfmac4366c-pcie"); > BRCMF_FW_DEF(4371, "brcmfmac4371-pcie"); > +BRCMF_FW_CLM_DEF(4378B1, "brcmfmac4378b1-pcie"); > > /* firmware config files */ > MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-pcie.txt"); > @@ -87,6 +88,7 @@ static const struct brcmf_firmware_mapping brcmf_pcie_fwnames[] = { > BRCMF_FW_ENTRY(BRCM_CC_43664_CHIP_ID, 0xFFFFFFF0, 4366C), > BRCMF_FW_ENTRY(BRCM_CC_43666_CHIP_ID, 0xFFFFFFF0, 4366C), > BRCMF_FW_ENTRY(BRCM_CC_4371_CHIP_ID, 0xFFFFFFFF, 4371), > + BRCMF_FW_ENTRY(BRCM_CC_4378_CHIP_ID, 0xFFFFFFFF, 4378B1), /* 3 */ what does the trailing comment reflect? > }; > > #define BRCMF_PCIE_FW_UP_TIMEOUT 5000 /* msec */
On 1/4/2022 8:26 AM, Hector Martin wrote: > On DT platforms, the module-instance and antenna-sku-info properties > are passed in the DT. On ACPI platforms, module-instance is passed via > the analogous Apple device property mechanism, while the antenna SKU > info is instead obtained via an ACPI method that grabs it from > non-volatile storage. > > Add support for this, to allow proper firmware selection on Apple > platforms. > > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/Makefile | 2 + > .../broadcom/brcm80211/brcmfmac/acpi.c | 47 +++++++++++++++++++ > .../broadcom/brcm80211/brcmfmac/common.c | 1 + > .../broadcom/brcm80211/brcmfmac/common.h | 9 ++++ > 4 files changed, 59 insertions(+) > create mode 100644 drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c [...] > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c > new file mode 100644 > index 000000000000..2b1a4448b291 > --- /dev/null > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c > @@ -0,0 +1,47 @@ > +// SPDX-License-Identifier: ISC > +/* > + * Copyright The Asahi Linux Contributors > + */ > + > +#include <linux/acpi.h> > +#include "debug.h" > +#include "core.h" > +#include "common.h" > + > +void brcmf_acpi_probe(struct device *dev, enum brcmf_bus_type bus_type, > + struct brcmf_mp_device *settings) > +{ > + acpi_status status; > + const union acpi_object *o; > + struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL}; > + struct acpi_device *adev = ACPI_COMPANION(dev); > + > + if (!adev) > + return; > + > + if (!ACPI_FAILURE(acpi_dev_get_property(adev, "module-instance", > + ACPI_TYPE_STRING, &o))) { > + brcmf_dbg(INFO, "ACPI module-instance=%s\n", o->string.pointer); > + settings->board_type = devm_kasprintf(dev, GFP_KERNEL, > + "apple,%s", > + o->string.pointer); > + } else { > + brcmf_dbg(INFO, "No ACPI module-instance\n"); Do you need to obtain the antenna-sku when there is no module-instance? > + } > + > + status = acpi_evaluate_object(adev->handle, "RWCV", NULL, &buf); Can you clarify what the above does? What does the "RWCV" mean? > + o = buf.pointer; > + if (!ACPI_FAILURE(status) && o && o->type == ACPI_TYPE_BUFFER && > + o->buffer.length >= 2) { > + char *antenna_sku = devm_kzalloc(dev, 3, GFP_KERNEL); > + > + memcpy(antenna_sku, o->buffer.pointer, 2); > + brcmf_dbg(INFO, "ACPI RWCV data=%*phN antenna-sku=%s\n", > + (int)o->buffer.length, o->buffer.pointer, > + antenna_sku); > + > + settings->antenna_sku = antenna_sku; > + } else { > + brcmf_dbg(INFO, "No ACPI antenna-sku\n"); > + } > +}
On 1/4/2022 8:26 AM, Hector Martin wrote: > Newer Apple firmwares on chipsets without a hardware RNG require the > host to provide a buffer of 256 random bytes to the device on > initialization. This buffer is present immediately before NVRAM, > suffixed by a footer containing a magic number and the buffer length. > > This won't affect chips/firmwares that do not use this feature, so do it > unconditionally. Not sure what the general opinion is here, but pulling random bytes for naught seems wasteful to me. So if there is a way of knowing it is needed please make it conditional. Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/pcie.c | 30 +++++++++++++++++++ > 1 file changed, 30 insertions(+)
On 1/4/2022 8:26 AM, Hector Martin wrote: > This chip is present on at least these Apple T2 Macs: > > * hawaii: MacBook Air 13" (Late 2018) > * hawaii: MacBook Air 13" (True Tone, 2019) Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/chip.c | 1 + > drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 8 ++++++++ > .../net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h | 2 ++ > 3 files changed, 11 insertions(+) [...] > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > index a8cccfbea20b..fdba2b5b46f0 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > @@ -49,6 +49,7 @@ enum brcmf_pcie_state { > BRCMF_FW_DEF(43602, "brcmfmac43602-pcie"); > BRCMF_FW_DEF(4350, "brcmfmac4350-pcie"); > BRCMF_FW_DEF(4350C, "brcmfmac4350c2-pcie"); > +BRCMF_FW_CLM_DEF(4355C1, "brcmfmac4355c1-pcie"); > BRCMF_FW_CLM_DEF(4356, "brcmfmac4356-pcie"); > BRCMF_FW_CLM_DEF(43570, "brcmfmac43570-pcie"); > BRCMF_FW_DEF(4358, "brcmfmac4358-pcie"); > @@ -75,6 +76,7 @@ static const struct brcmf_firmware_mapping brcmf_pcie_fwnames[] = { > BRCMF_FW_ENTRY(BRCM_CC_4350_CHIP_ID, 0x000000FF, 4350C), > BRCMF_FW_ENTRY(BRCM_CC_4350_CHIP_ID, 0xFFFFFF00, 4350), > BRCMF_FW_ENTRY(BRCM_CC_43525_CHIP_ID, 0xFFFFFFF0, 4365C), > + BRCMF_FW_ENTRY(BRCM_CC_4355_CHIP_ID, 0xFFFFFFFF, 4355C1), /* 12 */ still intrigued what that trailing number means ;-) > BRCMF_FW_ENTRY(BRCM_CC_4356_CHIP_ID, 0xFFFFFFFF, 4356), > BRCMF_FW_ENTRY(BRCM_CC_43567_CHIP_ID, 0xFFFFFFFF, 43570), > BRCMF_FW_ENTRY(BRCM_CC_43569_CHIP_ID, 0xFFFFFFFF, 43570),
On 1/4/2022 8:26 AM, Hector Martin wrote: > This chip is present on at least these Apple T2 Macs: > > * tahiti: MacBook Pro 13" (2020, 2 TB3) > * formosa: MacBook Pro 13" (Touch/2019) > * fiji: MacBook Air 13" (Scissor, 2020) Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/chip.c | 1 + > drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 4 ++++ > drivers/net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h | 2 ++ > 3 files changed, 7 insertions(+)
On 1/4/2022 8:26 AM, Hector Martin wrote: > This chip exists in two revisions (B2=r3 and B3=r4) on different > platforms, and was added without regard to doing proper firmware > selection or differentiating between them. Fix this to have proper > per-revision firmwares and support Apple NVRAM selection. > > Revision B2 is present on at least these Apple T2 Macs: > > kauai: MacBook Pro 15" (Touch/2018-2019) > maui: MacBook Pro 13" (Touch/2018-2019) > lanai: Mac mini (Late 2018) > ekans: iMac Pro 27" (5K, Late 2017) > > And these non-T2 Macs: > > nihau: iMac 27" (5K, 2019) > > Revision B3 is present on at least these Apple T2 Macs: > > bali: MacBook Pro 16" (2019) > trinidad: MacBook Pro 13" (2020, 4 TB3) > borneo: MacBook Pro 16" (2019, 5600M) > kahana: Mac Pro (2019) > kahana: Mac Pro (2019, Rack) > hanauma: iMac 27" (5K, 2020) > kure: iMac 27" (5K, 2020, 5700/XT) > > Fixes: 24f0bd136264 ("brcmfmac: add the BRCM 4364 found in MacBook Pro 15,2") > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 11 +++++++++-- > 1 file changed, 9 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > index 87daabb15cd0..e4f2aff3c0d5 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > @@ -54,7 +54,8 @@ BRCMF_FW_CLM_DEF(4356, "brcmfmac4356-pcie"); > BRCMF_FW_CLM_DEF(43570, "brcmfmac43570-pcie"); > BRCMF_FW_DEF(4358, "brcmfmac4358-pcie"); > BRCMF_FW_DEF(4359, "brcmfmac4359-pcie"); > -BRCMF_FW_DEF(4364, "brcmfmac4364-pcie"); > +BRCMF_FW_CLM_DEF(4364B2, "brcmfmac4364b2-pcie"); > +BRCMF_FW_CLM_DEF(4364B3, "brcmfmac4364b3-pcie"); would this break things for people. Maybe better to keep the old name for the B2 variant. > BRCMF_FW_DEF(4365B, "brcmfmac4365b-pcie"); > BRCMF_FW_DEF(4365C, "brcmfmac4365c-pcie"); > BRCMF_FW_DEF(4366B, "brcmfmac4366b-pcie"); > @@ -84,7 +85,8 @@ static const struct brcmf_firmware_mapping brcmf_pcie_fwnames[] = { > BRCMF_FW_ENTRY(BRCM_CC_43570_CHIP_ID, 0xFFFFFFFF, 43570), > BRCMF_FW_ENTRY(BRCM_CC_4358_CHIP_ID, 0xFFFFFFFF, 4358), > BRCMF_FW_ENTRY(BRCM_CC_4359_CHIP_ID, 0xFFFFFFFF, 4359), > - BRCMF_FW_ENTRY(BRCM_CC_4364_CHIP_ID, 0xFFFFFFFF, 4364), > + BRCMF_FW_ENTRY(BRCM_CC_4364_CHIP_ID, 0x0000000F, 4364B2), /* 3 */ > + BRCMF_FW_ENTRY(BRCM_CC_4364_CHIP_ID, 0xFFFFFFF0, 4364B3), /* 4 */ okay. so it is the numerical chip revision. If so, please drop that comment. > BRCMF_FW_ENTRY(BRCM_CC_4365_CHIP_ID, 0x0000000F, 4365B), > BRCMF_FW_ENTRY(BRCM_CC_4365_CHIP_ID, 0xFFFFFFF0, 4365C), > BRCMF_FW_ENTRY(BRCM_CC_4366_CHIP_ID, 0x0000000F, 4366B),
Hector Martin <marcan@marcan.st> writes: > Hi everyone, > > Happy new year! This 35-patch series adds proper support for the > Broadcom FullMAC chips used on Apple T2 and M1 platforms: > > - BCM4355C1 > - BCM4364B2/B3 > - BCM4377B3 > - BCM4378B1 > - BCM4387C2 35 patches is a lot to review. It would make things easier for reviewers if you can split this into smaller patchsets, 10-12 patches per set is what I usually recommend. More info in the wiki link below.
On 2022/01/10 18:10, Arend van Spriel wrote: > On 1/4/2022 8:26 AM, Hector Martin wrote: >> This chip is present on Apple M1 (t8103) platforms: >> >> * atlantisb (apple,j274): Mac mini (M1, 2020) >> * honshu (apple,j293): MacBook Pro (13-inch, M1, 2020) >> * shikoku (apple,j313): MacBook Air (M1, 2020) >> * capri (apple,j456): iMac (24-inch, 4x USB-C, M1, 2020) >> * santorini (apple,j457): iMac (24-inch, 2x USB-C, M1, 2020) > > Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> >> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> >> Signed-off-by: Hector Martin <marcan@marcan.st> >> --- >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/chip.c | 2 ++ >> drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 8 ++++++++ >> .../net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h | 2 ++ >> 3 files changed, 12 insertions(+) > > [...] > >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c >> index f3744e806157..cc76f00724e6 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c >> @@ -58,6 +58,7 @@ BRCMF_FW_DEF(4365C, "brcmfmac4365c-pcie"); >> BRCMF_FW_DEF(4366B, "brcmfmac4366b-pcie"); >> BRCMF_FW_DEF(4366C, "brcmfmac4366c-pcie"); >> BRCMF_FW_DEF(4371, "brcmfmac4371-pcie"); >> +BRCMF_FW_CLM_DEF(4378B1, "brcmfmac4378b1-pcie"); >> >> /* firmware config files */ >> MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-pcie.txt"); >> @@ -87,6 +88,7 @@ static const struct brcmf_firmware_mapping brcmf_pcie_fwnames[] = { >> BRCMF_FW_ENTRY(BRCM_CC_43664_CHIP_ID, 0xFFFFFFF0, 4366C), >> BRCMF_FW_ENTRY(BRCM_CC_43666_CHIP_ID, 0xFFFFFFF0, 4366C), >> BRCMF_FW_ENTRY(BRCM_CC_4371_CHIP_ID, 0xFFFFFFFF, 4371), >> + BRCMF_FW_ENTRY(BRCM_CC_4378_CHIP_ID, 0xFFFFFFFF, 4378B1), /* 3 */ > > what does the trailing comment reflect? PCI revision IDs seen in the wild. The mask currently accepts all of them, but B1 specifically seems to map to rev3. This is important for 4364 since there are two revisions in the wild, and so that one has more selective masks. I can change it to "rev3" to make it more obvious. I'm actually not sure what the best approach for the masks is. We could also only accept known exact revisions; that would be better if a newer revision is incompatible, but worse if it is and would otherwise just work.
On 2022/01/10 18:11, Arend van Spriel wrote: > On 1/4/2022 8:26 AM, Hector Martin wrote: >> On DT platforms, the module-instance and antenna-sku-info properties >> are passed in the DT. On ACPI platforms, module-instance is passed via >> the analogous Apple device property mechanism, while the antenna SKU >> info is instead obtained via an ACPI method that grabs it from >> non-volatile storage. >> >> Add support for this, to allow proper firmware selection on Apple >> platforms. >> >> Signed-off-by: Hector Martin <marcan@marcan.st> >> --- >> .../broadcom/brcm80211/brcmfmac/Makefile | 2 + >> .../broadcom/brcm80211/brcmfmac/acpi.c | 47 +++++++++++++++++++ >> .../broadcom/brcm80211/brcmfmac/common.c | 1 + >> .../broadcom/brcm80211/brcmfmac/common.h | 9 ++++ >> 4 files changed, 59 insertions(+) >> create mode 100644 drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c > > [...] > >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c >> new file mode 100644 >> index 000000000000..2b1a4448b291 >> --- /dev/null >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c >> @@ -0,0 +1,47 @@ >> +// SPDX-License-Identifier: ISC >> +/* >> + * Copyright The Asahi Linux Contributors >> + */ >> + >> +#include <linux/acpi.h> >> +#include "debug.h" >> +#include "core.h" >> +#include "common.h" >> + >> +void brcmf_acpi_probe(struct device *dev, enum brcmf_bus_type bus_type, >> + struct brcmf_mp_device *settings) >> +{ >> + acpi_status status; >> + const union acpi_object *o; >> + struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL}; >> + struct acpi_device *adev = ACPI_COMPANION(dev); >> + >> + if (!adev) >> + return; >> + >> + if (!ACPI_FAILURE(acpi_dev_get_property(adev, "module-instance", >> + ACPI_TYPE_STRING, &o))) { >> + brcmf_dbg(INFO, "ACPI module-instance=%s\n", o->string.pointer); >> + settings->board_type = devm_kasprintf(dev, GFP_KERNEL, >> + "apple,%s", >> + o->string.pointer); >> + } else { >> + brcmf_dbg(INFO, "No ACPI module-instance\n"); > > Do you need to obtain the antenna-sku when there is no module-instance? In principle I don't think any machines would have antenna-sku and no module-instance, though the firmware selection will still work without it (it'll just end up using the DMI machine name instead). > >> + } >> + >> + status = acpi_evaluate_object(adev->handle, "RWCV", NULL, &buf); > > Can you clarify what the above does? What does the "RWCV" mean? No idea what it *means* :-) What it is, though, is the ACPI method name to get the antenna-sku.
On 2022/01/10 18:11, Arend van Spriel wrote: > On 1/4/2022 8:26 AM, Hector Martin wrote: >> Newer Apple firmwares on chipsets without a hardware RNG require the >> host to provide a buffer of 256 random bytes to the device on >> initialization. This buffer is present immediately before NVRAM, >> suffixed by a footer containing a magic number and the buffer length. >> >> This won't affect chips/firmwares that do not use this feature, so do it >> unconditionally. > > Not sure what the general opinion is here, but pulling random bytes for > naught seems wasteful to me. So if there is a way of knowing it is > needed please make it conditional. We could gate it on specific chips only, if you don't mind maintaining a list of those. AIUI that would be all the T2 platform chips or so (the newer two don't seem to need it). Alternatively we could just do this only if an Apple OTP is detected. That is already implicitly gated by the OTP offset chip list.
On 2022/01/10 18:12, Arend van Spriel wrote: > On 1/4/2022 8:26 AM, Hector Martin wrote: >> This chip exists in two revisions (B2=r3 and B3=r4) on different >> platforms, and was added without regard to doing proper firmware >> selection or differentiating between them. Fix this to have proper >> per-revision firmwares and support Apple NVRAM selection. >> >> Revision B2 is present on at least these Apple T2 Macs: >> >> kauai: MacBook Pro 15" (Touch/2018-2019) >> maui: MacBook Pro 13" (Touch/2018-2019) >> lanai: Mac mini (Late 2018) >> ekans: iMac Pro 27" (5K, Late 2017) >> >> And these non-T2 Macs: >> >> nihau: iMac 27" (5K, 2019) >> >> Revision B3 is present on at least these Apple T2 Macs: >> >> bali: MacBook Pro 16" (2019) >> trinidad: MacBook Pro 13" (2020, 4 TB3) >> borneo: MacBook Pro 16" (2019, 5600M) >> kahana: Mac Pro (2019) >> kahana: Mac Pro (2019, Rack) >> hanauma: iMac 27" (5K, 2020) >> kure: iMac 27" (5K, 2020, 5700/XT) >> >> Fixes: 24f0bd136264 ("brcmfmac: add the BRCM 4364 found in MacBook Pro 15,2") >> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> >> Signed-off-by: Hector Martin <marcan@marcan.st> >> --- >> .../net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 11 +++++++++-- >> 1 file changed, 9 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c >> index 87daabb15cd0..e4f2aff3c0d5 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c >> @@ -54,7 +54,8 @@ BRCMF_FW_CLM_DEF(4356, "brcmfmac4356-pcie"); >> BRCMF_FW_CLM_DEF(43570, "brcmfmac43570-pcie"); >> BRCMF_FW_DEF(4358, "brcmfmac4358-pcie"); >> BRCMF_FW_DEF(4359, "brcmfmac4359-pcie"); >> -BRCMF_FW_DEF(4364, "brcmfmac4364-pcie"); >> +BRCMF_FW_CLM_DEF(4364B2, "brcmfmac4364b2-pcie"); >> +BRCMF_FW_CLM_DEF(4364B3, "brcmfmac4364b3-pcie"); > > would this break things for people. Maybe better to keep the old name > for the B2 variant. Or the B3 variant... people have been using random copied firmwares with the same name, I guess. Probably even the wrong NVRAMs in some cases. And then I'd have to add a special case to the firmware extraction script to rename one of these two to not include the revision... Plus, newer firmwares require the random blob, so this only ever worked with old, obsolete firmwares... which I think have security vulnerabilities (there was an AWDL exploit recently IIRC). Honestly though, there are probably rather few people using upstream kernels on T2s. Certainly on the MacBooks, since the keyboard/touchpad aren't supported upstream yet... plus given that there was never any "official" firmware distributed under the revision-less name, none of this would work out of the box with upstream kernels anyway. FWIW, I've been in contact with the t2linux folks and users have been testing this patchset (that's how I got it tested on all the chips), so at least some people are already aware of the story and how to get the firmware named properly :-) >> - BRCMF_FW_ENTRY(BRCM_CC_4364_CHIP_ID, 0xFFFFFFFF, 4364), >> + BRCMF_FW_ENTRY(BRCM_CC_4364_CHIP_ID, 0x0000000F, 4364B2), /* 3 */ >> + BRCMF_FW_ENTRY(BRCM_CC_4364_CHIP_ID, 0xFFFFFFF0, 4364B3), /* 4 */ > > okay. so it is the numerical chip revision. If so, please drop that comment. > I figured it would be useful to document this somewhere, since the alphanumeric code -> rev number mapping doesn't seem to be consistent from chip to chip, and we might have to add a new revision in the future for an existing chip (which would require knowing the rev for the old one). Do you have any ideas?
On 2022/01/10 19:14, Kalle Valo wrote: > Hector Martin <marcan@marcan.st> writes: > >> Hi everyone, >> >> Happy new year! This 35-patch series adds proper support for the >> Broadcom FullMAC chips used on Apple T2 and M1 platforms: >> >> - BCM4355C1 >> - BCM4364B2/B3 >> - BCM4377B3 >> - BCM4378B1 >> - BCM4387C2 > > 35 patches is a lot to review. It would make things easier for reviewers > if you can split this into smaller patchsets, 10-12 patches per set is > what I usually recommend. More info in the wiki link below. The patches are already split into logical groupings, so I think there isn't much more to be gained by sending them separately. As I described in the cover letter: 01~09: Firmware selection stuff 10~14: Add support for BCM4378 15~20: Add BCM4355/4364/4377 on top 21~27: Add BCM4387 and its newer requirements 28~32: Misc fixes 33~35: TxCap & calibration support If you want to review the series piecemeal, feel free to stop at any of those boundaries; the series will still make sense and is useful at any of those stopping points. Note that the firmware selection stuff (in particular patches #4 and #6) will change quite a bit in v3 from the review feedback so far, so you might want to skip reviewing those in detail for v2.
On 1/10/2022 12:07 PM, Hector Martin wrote: > On 2022/01/10 18:11, Arend van Spriel wrote: >> On 1/4/2022 8:26 AM, Hector Martin wrote: >>> On DT platforms, the module-instance and antenna-sku-info properties >>> are passed in the DT. On ACPI platforms, module-instance is passed via >>> the analogous Apple device property mechanism, while the antenna SKU >>> info is instead obtained via an ACPI method that grabs it from >>> non-volatile storage. >>> >>> Add support for this, to allow proper firmware selection on Apple >>> platforms. >>> >>> Signed-off-by: Hector Martin <marcan@marcan.st> >>> --- >>> .../broadcom/brcm80211/brcmfmac/Makefile | 2 + >>> .../broadcom/brcm80211/brcmfmac/acpi.c | 47 +++++++++++++++++++ >>> .../broadcom/brcm80211/brcmfmac/common.c | 1 + >>> .../broadcom/brcm80211/brcmfmac/common.h | 9 ++++ >>> 4 files changed, 59 insertions(+) >>> create mode 100644 drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c >> >> [...] >> >>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c >>> new file mode 100644 >>> index 000000000000..2b1a4448b291 >>> --- /dev/null >>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c >>> @@ -0,0 +1,47 @@ >>> +// SPDX-License-Identifier: ISC >>> +/* >>> + * Copyright The Asahi Linux Contributors >>> + */ >>> + >>> +#include <linux/acpi.h> >>> +#include "debug.h" >>> +#include "core.h" >>> +#include "common.h" >>> + >>> +void brcmf_acpi_probe(struct device *dev, enum brcmf_bus_type bus_type, >>> + struct brcmf_mp_device *settings) >>> +{ >>> + acpi_status status; >>> + const union acpi_object *o; >>> + struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL}; >>> + struct acpi_device *adev = ACPI_COMPANION(dev); >>> + >>> + if (!adev) >>> + return; >>> + >>> + if (!ACPI_FAILURE(acpi_dev_get_property(adev, "module-instance", >>> + ACPI_TYPE_STRING, &o))) { >>> + brcmf_dbg(INFO, "ACPI module-instance=%s\n", o->string.pointer); >>> + settings->board_type = devm_kasprintf(dev, GFP_KERNEL, >>> + "apple,%s", >>> + o->string.pointer); >>> + } else { >>> + brcmf_dbg(INFO, "No ACPI module-instance\n"); >> >> Do you need to obtain the antenna-sku when there is no module-instance? > > In principle I don't think any machines would have antenna-sku and no > module-instance, though the firmware selection will still work without > it (it'll just end up using the DMI machine name instead). Right. That was my assumption as well. I would bail out here and skip obtaining the antenna-sku. >> >>> + } >>> + >>> + status = acpi_evaluate_object(adev->handle, "RWCV", NULL, &buf); >> >> Can you clarify what the above does? What does the "RWCV" mean? > > No idea what it *means* :-) > > What it is, though, is the ACPI method name to get the antenna-sku. Wow. So much for meaning-full naming ;-)
On 1/10/2022 12:09 PM, Hector Martin wrote: > On 2022/01/10 18:11, Arend van Spriel wrote: >> On 1/4/2022 8:26 AM, Hector Martin wrote: >>> Newer Apple firmwares on chipsets without a hardware RNG require the >>> host to provide a buffer of 256 random bytes to the device on >>> initialization. This buffer is present immediately before NVRAM, >>> suffixed by a footer containing a magic number and the buffer length. >>> >>> This won't affect chips/firmwares that do not use this feature, so do it >>> unconditionally. >> >> Not sure what the general opinion is here, but pulling random bytes for >> naught seems wasteful to me. So if there is a way of knowing it is >> needed please make it conditional. > > We could gate it on specific chips only, if you don't mind maintaining a > list of those. AIUI that would be all the T2 platform chips or so (the > newer two don't seem to need it). > > Alternatively we could just do this only if an Apple OTP is detected. > That is already implicitly gated by the OTP offset chip list. That sounds like a good approach. Regards, Arend
On 1/10/2022 12:20 PM, Hector Martin wrote: > On 2022/01/10 18:12, Arend van Spriel wrote: >> On 1/4/2022 8:26 AM, Hector Martin wrote: >>> This chip exists in two revisions (B2=r3 and B3=r4) on different >>> platforms, and was added without regard to doing proper firmware >>> selection or differentiating between them. Fix this to have proper >>> per-revision firmwares and support Apple NVRAM selection. >>> >>> Revision B2 is present on at least these Apple T2 Macs: >>> >>> kauai: MacBook Pro 15" (Touch/2018-2019) >>> maui: MacBook Pro 13" (Touch/2018-2019) >>> lanai: Mac mini (Late 2018) >>> ekans: iMac Pro 27" (5K, Late 2017) >>> >>> And these non-T2 Macs: >>> >>> nihau: iMac 27" (5K, 2019) >>> >>> Revision B3 is present on at least these Apple T2 Macs: >>> >>> bali: MacBook Pro 16" (2019) >>> trinidad: MacBook Pro 13" (2020, 4 TB3) >>> borneo: MacBook Pro 16" (2019, 5600M) >>> kahana: Mac Pro (2019) >>> kahana: Mac Pro (2019, Rack) >>> hanauma: iMac 27" (5K, 2020) >>> kure: iMac 27" (5K, 2020, 5700/XT) >>> >>> Fixes: 24f0bd136264 ("brcmfmac: add the BRCM 4364 found in MacBook Pro 15,2") >>> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> >>> Signed-off-by: Hector Martin <marcan@marcan.st> >>> --- >>> .../net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 11 +++++++++-- >>> 1 file changed, 9 insertions(+), 2 deletions(-) >>> >>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c >>> index 87daabb15cd0..e4f2aff3c0d5 100644 >>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c >>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c >>> @@ -54,7 +54,8 @@ BRCMF_FW_CLM_DEF(4356, "brcmfmac4356-pcie"); >>> BRCMF_FW_CLM_DEF(43570, "brcmfmac43570-pcie"); >>> BRCMF_FW_DEF(4358, "brcmfmac4358-pcie"); >>> BRCMF_FW_DEF(4359, "brcmfmac4359-pcie"); >>> -BRCMF_FW_DEF(4364, "brcmfmac4364-pcie"); >>> +BRCMF_FW_CLM_DEF(4364B2, "brcmfmac4364b2-pcie"); >>> +BRCMF_FW_CLM_DEF(4364B3, "brcmfmac4364b3-pcie"); >> >> would this break things for people. Maybe better to keep the old name >> for the B2 variant. > > Or the B3 variant... people have been using random copied firmwares with > the same name, I guess. Probably even the wrong NVRAMs in some cases. > And then I'd have to add a special case to the firmware extraction > script to rename one of these two to not include the revision... > > Plus, newer firmwares require the random blob, so this only ever worked > with old, obsolete firmwares... which I think have security > vulnerabilities (there was an AWDL exploit recently IIRC). > > Honestly though, there are probably rather few people using upstream > kernels on T2s. Certainly on the MacBooks, since the keyboard/touchpad > aren't supported upstream yet... plus given that there was never any > "official" firmware distributed under the revision-less name, none of > this would work out of the box with upstream kernels anyway. > > FWIW, I've been in contact with the t2linux folks and users have been > testing this patchset (that's how I got it tested on all the chips), so > at least some people are already aware of the story and how to get the > firmware named properly :-) Ok. When there is no brcmfmac4364-pcie.bin in linux-firmware repo we can safely rename. >>> - BRCMF_FW_ENTRY(BRCM_CC_4364_CHIP_ID, 0xFFFFFFFF, 4364), >>> + BRCMF_FW_ENTRY(BRCM_CC_4364_CHIP_ID, 0x0000000F, 4364B2), /* 3 */ >>> + BRCMF_FW_ENTRY(BRCM_CC_4364_CHIP_ID, 0xFFFFFFF0, 4364B3), /* 4 */ >> >> okay. so it is the numerical chip revision. If so, please drop that comment. >> > > I figured it would be useful to document this somewhere, since the > alphanumeric code -> rev number mapping doesn't seem to be consistent > from chip to chip, and we might have to add a new revision in the future > for an existing chip (which would require knowing the rev for the old > one). Do you have any ideas? Indeed the alphanumeric code differs from chip to chip depending on how much respins are necessary and what type of respin. We start a 'a0' aka numeric rev 0. For minor fixes we increase the digit, but for major fixes or new functionality we move to the next letter whereas the numeric revision simply increases.
Hector Martin <marcan@marcan.st> writes: > On 2022/01/10 19:14, Kalle Valo wrote: >> Hector Martin <marcan@marcan.st> writes: >> >>> Hi everyone, >>> >>> Happy new year! This 35-patch series adds proper support for the >>> Broadcom FullMAC chips used on Apple T2 and M1 platforms: >>> >>> - BCM4355C1 >>> - BCM4364B2/B3 >>> - BCM4377B3 >>> - BCM4378B1 >>> - BCM4387C2 >> >> 35 patches is a lot to review. It would make things easier for reviewers >> if you can split this into smaller patchsets, 10-12 patches per set is >> what I usually recommend. More info in the wiki link below. > > The patches are already split into logical groupings, so I think there > isn't much more to be gained by sending them separately. As I described > in the cover letter: > > 01~09: Firmware selection stuff > 10~14: Add support for BCM4378 > 15~20: Add BCM4355/4364/4377 on top > 21~27: Add BCM4387 and its newer requirements > 28~32: Misc fixes > 33~35: TxCap & calibration support > > If you want to review the series piecemeal, feel free to stop at any of > those boundaries; the series will still make sense and is useful at any > of those stopping points. Really, having smaller patchsets makes the patch flow so much smoother for everyone. If you want to submit huge patchsets then go ahead, but that will automatically drop the patches to the bottom of my queue.
Hector Martin <marcan@marcan.st> writes: > On 04/01/2022 23.12, Andy Shevchenko wrote: >> On Tue, Jan 4, 2022 at 9:29 AM Hector Martin <marcan@marcan.st> wrote: >>> >>> The driver was enabling IRQs before the message processing was >>> initialized. This could cause IRQs to come in too early and crash the >>> driver. Instead, move the IRQ enable and hostready to a bus preinit >>> function, at which point everything is properly initialized. >>> >>> Fixes: 9e37f045d5e7 ("brcmfmac: Adding PCIe bus layer support.") >> >> You should gather fixes at the beginning of the series, and even >> possible to send them as a separate series. In the current state it's >> unclear if there are dependencies on your new feature (must not be for >> fixes that meant to be backported). >> > > Thanks, I wasn't sure what order you wanted those in. I'll put them at > the top for v3. I think none of those should have any dependencies on > the rest of the patches, modulo some trivial rebase wrangling. If there are no dependencies, please send the brcmfmac fixes separately so that I can apply them earlier.
Hector Martin <marcan@marcan.st> writes: > On 2022/01/04 19:21, Arend van Spriel wrote: >> On 1/4/2022 8:26 AM, Hector Martin wrote: >>> On DT platforms, the module-instance and antenna-sku-info properties >>> are passed in the DT. On ACPI platforms, module-instance is passed via >>> the analogous Apple device property mechanism, while the antenna SKU >>> info is instead obtained via an ACPI method that grabs it from >>> non-volatile storage. >>> >>> Add support for this, to allow proper firmware selection on Apple >>> platforms. >>> >>> Signed-off-by: Hector Martin <marcan@marcan.st> >>> --- >>> .../broadcom/brcm80211/brcmfmac/Makefile | 2 + >>> .../broadcom/brcm80211/brcmfmac/acpi.c | 47 +++++++++++++++++++ >>> .../broadcom/brcm80211/brcmfmac/common.c | 1 + >>> .../broadcom/brcm80211/brcmfmac/common.h | 9 ++++ >>> 4 files changed, 59 insertions(+) >>> create mode 100644 drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c >>> >>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile >>> index 13c13504a6e8..19009eb9db93 100644 >>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile >>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile >>> @@ -47,3 +47,5 @@ brcmfmac-$(CONFIG_OF) += \ >>> of.o >>> brcmfmac-$(CONFIG_DMI) += \ >>> dmi.o >>> +brcmfmac-$(CONFIG_ACPI) += \ >>> + acpi.o >>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c >>> new file mode 100644 >>> index 000000000000..2b1a4448b291 >>> --- /dev/null >>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c >>> @@ -0,0 +1,47 @@ >>> +// SPDX-License-Identifier: ISC >>> +/* >>> + * Copyright The Asahi Linux Contributors >>> + */ >> >> Common format for copyright statement (in this folder) seems to be: >> >> Copyright (c) <YEAR> <COPYRIGHT_HOLDER> >> >> Regards, >> Arend > > I get this every time I submit a patch to a new subsystem :-) > > This is based on this best practice: > > https://www.linuxfoundation.org/blog/copyright-notices-in-open-source-software-projects/ I didn't know about this recommendation, thanks. > Basically, the year format quickly becomes outdated and is rather > useless, and listing specific names also ends up missing every > subsequent contributor, so more general copyright statements work better > for this kind of use case. In the end we all know git history is the > proper record of copyright status. > > I don't have a super strong opinion here, but we've been trying to > standardize on this format for contributions coming from our subproject, > and it feels more useful than a random contributor's name with a date 5 > years in the past :) If LF is fine with this approach, then it's good enough also for me. So at least from my point of view no need to make any changes.
On 1/4/2022 8:26 AM, Hector Martin wrote: > This new API version is required for at least the BCM4387 firmware. Add > support for it, with a fallback to the v1 API. Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Acked-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/cfg80211.c | 113 ++++++++++++++---- > .../broadcom/brcm80211/brcmfmac/feature.c | 1 + > .../broadcom/brcm80211/brcmfmac/feature.h | 4 +- > .../broadcom/brcm80211/brcmfmac/fwil_types.h | 49 +++++++- > 4 files changed, 145 insertions(+), 22 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > index fb727778312c..71e932a8302c 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > @@ -769,12 +769,50 @@ void brcmf_set_mpc(struct brcmf_if *ifp, int mpc) > } > } > > +static void brcmf_escan_prep(struct brcmf_cfg80211_info *cfg, > + struct brcmf_scan_params_v2_le *params_le, > + struct cfg80211_scan_request *request); I am not a fan of function prototypes so if it can be avoided by simply moving the function that would be preferred over this. > +static void brcmf_scan_params_v2_to_v1(struct brcmf_scan_params_v2_le *params_v2_le, > + struct brcmf_scan_params_le *params_le) > +{ [...] > + if (!brcmf_feat_is_enabled(ifp, BRCMF_FEAT_SCAN_V2)) { Okay. So it is not really a fallback. Phew! > + struct brcmf_escan_params_le *params_v1; > + > + params_size -= BRCMF_SCAN_PARAMS_V2_FIXED_SIZE; > + params_size += BRCMF_SCAN_PARAMS_FIXED_SIZE; > + params_v1 = kzalloc(params_size, GFP_KERNEL); > + params_v1->version = cpu_to_le32(BRCMF_ESCAN_REQ_VERSION); > + brcmf_scan_params_v2_to_v1(¶ms->params_v2_le, ¶ms_v1->params_le); > + kfree(params); > + params = params_v1; > + } > + > params->action = cpu_to_le16(WL_ESCAN_ACTION_START); > params->sync_id = cpu_to_le16(0x1234);
On 09/01/2022 05.03, Arend van Spriel wrote: >> The chip revision nominally comes from OTP on Apple platforms, but it >> can be mapped to the PCI revision number, so we ignore the OTP revision >> and continue to use the existing PCI revision mechanism to identify chip >> revisions, as the driver already does for other chips. Unfortunately, >> the mapping is not consistent between different chip types, so this has >> to be determined experimentally. > > Not sure I understand this. The chip revision comes from the chipcommon > register [1]. Maybe that is what you mean by "PCI revision number". For > some chips it is possible OTP is used to override that. What I mean is the Apple custom OTP segment stores a textual revision number, like "C0". Apple's driver uses this to pick a firmware. There is an ad-hoc mapping between this and the numeric revision (which as you say is present in chipcommon but AFAICT the same number also ends up as the Revision ID in PCI config space). > > Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> >> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> >> Signed-off-by: Hector Martin <marcan@marcan.st> >> --- >> .../broadcom/brcm80211/brcmfmac/pcie.c | 58 ++++++++++++++++++- >> 1 file changed, 56 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c >> index 74c9a4f74813..250e0bd40cb3 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c >> @@ -2094,8 +2094,62 @@ brcmf_pcie_prepare_fw_request(struct brcmf_pciedev_info *devinfo) >> fwreq->domain_nr = pci_domain_nr(devinfo->pdev->bus) + 1; >> fwreq->bus_nr = devinfo->pdev->bus->number; >> >> - brcmf_dbg(PCIE, "Board: %s\n", devinfo->settings->board_type); >> - fwreq->board_types[0] = devinfo->settings->board_type; >> + /* Apple platforms with fancy firmware/NVRAM selection */ >> + if (devinfo->settings->board_type && >> + devinfo->settings->antenna_sku && >> + devinfo->otp.valid) { >> + char *buf; >> + int len; >> + >> + brcmf_dbg(PCIE, "Apple board: %s\n", >> + devinfo->settings->board_type); > > maybe good to use local reference for devinfo->settings->board_type, > which is used several times below. Yup, and also antenna_sku.
On 09/01/2022 05.14, Arend van Spriel wrote: > On 1/4/2022 8:26 AM, Hector Martin wrote: >> On Device Tree platforms, it is customary to be able to set the MAC >> address via the Device Tree, as it is often stored in system firmware. >> This is particularly relevant for Apple ARM64 platforms, where this >> information comes from system configuration and passed through by the >> bootloader into the DT. >> >> Implement support for this by fetching the platform MAC address and >> adding or replacing the macaddr= property in nvram. This becomes the >> dongle's default MAC address. >> >> On platforms with an SROM MAC address, this overrides it. On platforms >> without one, such as Apple ARM64 devices, this is required for the >> firmware to boot (it will fail if it does not have a valid MAC at all). > > What overrides what. Can you elaborate a bit? The behavior seems to be: - Use the NVRAM MAC address, if any - Use the SROM MAC address, if any - Fail to boot So a platform with a module containing a MAC address may choose to override it using the DT mechanism with this patch. This is consistent with the behavior of other drivers implementing platform MAC support.
On 05/01/2022 04.46, Arend Van Spriel wrote: > On January 4, 2022 8:30:51 AM Hector Martin <marcan@marcan.st> wrote: > >> This new API version is required for at least the BCM4387 firmware. Add >> support for it, with a fallback to the v1 API. >> >> Acked-by: Linus Walleij <linus.walleij@linaro.org> >> Signed-off-by: Hector Martin <marcan@marcan.st> >> --- >> .../broadcom/brcm80211/brcmfmac/cfg80211.c | 113 ++++++++++++++---- >> .../broadcom/brcm80211/brcmfmac/feature.c | 1 + >> .../broadcom/brcm80211/brcmfmac/feature.h | 4 +- >> .../broadcom/brcm80211/brcmfmac/fwil_types.h | 49 +++++++- >> 4 files changed, 145 insertions(+), 22 deletions(-) > > Compiling this patch with C=2 gives following warnings: > > drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:1086:28: > warning: incorrect type in assignment (different base types) > drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:1086:28: > expected restricted __le16 [usertype] version > drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:1086:28: got int > drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:1148:38: > warning: incorrect type in assignment (different base types) > drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:1148:38: > expected restricted __le32 [usertype] scan_type > drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:1148:38: got int > drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:789:30: > warning: incorrect type in assignment (different base types) > drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:789:30: > expected unsigned char [usertype] scan_type > drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c:789:30: got > restricted __le32 [usertype] scan_type > > Will check if this is a valid warning. Those are valid bugs (it'd break on big endian platforms), thanks for checking this. Fixed for v3 :)
On 11/01/2022 17.50, Arend van Spriel wrote: > On 1/4/2022 8:26 AM, Hector Martin wrote: >> This new API version is required for at least the BCM4387 firmware. Add >> support for it, with a fallback to the v1 API. > > Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> >> Acked-by: Linus Walleij <linus.walleij@linaro.org> >> Signed-off-by: Hector Martin <marcan@marcan.st> >> --- >> .../broadcom/brcm80211/brcmfmac/cfg80211.c | 113 ++++++++++++++---- >> .../broadcom/brcm80211/brcmfmac/feature.c | 1 + >> .../broadcom/brcm80211/brcmfmac/feature.h | 4 +- >> .../broadcom/brcm80211/brcmfmac/fwil_types.h | 49 +++++++- >> 4 files changed, 145 insertions(+), 22 deletions(-) >> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c >> index fb727778312c..71e932a8302c 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c >> @@ -769,12 +769,50 @@ void brcmf_set_mpc(struct brcmf_if *ifp, int mpc) >> } >> } >> >> +static void brcmf_escan_prep(struct brcmf_cfg80211_info *cfg, >> + struct brcmf_scan_params_v2_le *params_le, >> + struct cfg80211_scan_request *request); > > I am not a fan of function prototypes so if it can be avoided by simply > moving the function that would be preferred over this. Moved the function for v3 :) >> + if (!brcmf_feat_is_enabled(ifp, BRCMF_FEAT_SCAN_V2)) { > > Okay. So it is not really a fallback. Phew! I meant fallback in case the feature is not present, not fallback from trying to use it without checking. Thankfully we can use a feature test for this :-)
On 1/4/2022 8:26 AM, Hector Martin wrote: > At least on BCM4387, the D11 cores are held in reset on cold startup and > firmware expects to release reset itself. Just assert reset here and let > firmware deassert it. Premature deassertion results in the firmware > failing to initialize properly some of the time, with strange AXI bus > errors. > > Also, BCM4387 has 3 cores, up from 2. The logic for handling that is in > brcmf_chip_ai_resetcore(), but since we aren't using that any more, just > handle it here. Makes sense. Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../net/wireless/broadcom/brcm80211/brcmfmac/chip.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-)
On 1/4/2022 8:26 AM, Hector Martin wrote: > BCM4387 has trailing odd-sized blocks as part of TCM which have > their size described as a multiple of 1024 instead of 8192. Handle this > so we can compute the TCM size properly. Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../wireless/broadcom/brcm80211/brcmfmac/chip.c | 17 ++++++++++++----- > 1 file changed, 12 insertions(+), 5 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/chip.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/chip.c > index 713546cebd5a..cfa93e3ef1a1 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/chip.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/chip.c > @@ -212,8 +212,8 @@ struct sbsocramregs { > #define ARMCR4_TCBANB_MASK 0xf > #define ARMCR4_TCBANB_SHIFT 0 > > -#define ARMCR4_BSZ_MASK 0x3f > -#define ARMCR4_BSZ_MULT 8192 > +#define ARMCR4_BSZ_MASK 0x7f > +#define ARMCR4_BLK_1K_MASK 0x200 > > struct brcmf_core_priv { > struct brcmf_core pub; > @@ -675,7 +675,8 @@ static u32 brcmf_chip_sysmem_ramsize(struct brcmf_core_priv *sysmem) > } > > /** Return the TCM-RAM size of the ARMCR4 core. */ > -static u32 brcmf_chip_tcm_ramsize(struct brcmf_core_priv *cr4) > +static u32 brcmf_chip_tcm_ramsize(struct brcmf_chip_priv *ci, > + struct brcmf_core_priv *cr4) Not sure why you add ci parameter here. It is not used below or am I overlooking something. > { > u32 corecap; > u32 memsize = 0; > @@ -683,6 +684,7 @@ static u32 brcmf_chip_tcm_ramsize(struct brcmf_core_priv *cr4) > u32 nbb; > u32 totb; > u32 bxinfo; > + u32 blksize; > u32 idx; > > corecap = brcmf_chip_core_read32(cr4, ARMCR4_CAP); > @@ -694,7 +696,12 @@ static u32 brcmf_chip_tcm_ramsize(struct brcmf_core_priv *cr4) > for (idx = 0; idx < totb; idx++) { > brcmf_chip_core_write32(cr4, ARMCR4_BANKIDX, idx); > bxinfo = brcmf_chip_core_read32(cr4, ARMCR4_BANKINFO); > - memsize += ((bxinfo & ARMCR4_BSZ_MASK) + 1) * ARMCR4_BSZ_MULT; > + if (bxinfo & ARMCR4_BLK_1K_MASK) > + blksize = 1024; > + else > + blksize = 8192; > + > + memsize += ((bxinfo & ARMCR4_BSZ_MASK) + 1) * blksize; > } > > return memsize;
On 1/19/2022 1:36 PM, Arend van Spriel wrote: > On 1/4/2022 8:26 AM, Hector Martin wrote: >> BCM4387 has trailing odd-sized blocks as part of TCM which have >> their size described as a multiple of 1024 instead of 8192. Handle this >> so we can compute the TCM size properly. So that is the deal. Wish someone over here told me about that :-p Gave my blessing already, but do have some remarks. > Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> >> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> >> Signed-off-by: Hector Martin <marcan@marcan.st> >> --- >> .../wireless/broadcom/brcm80211/brcmfmac/chip.c | 17 ++++++++++++----- >> 1 file changed, 12 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/chip.c >> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/chip.c >> index 713546cebd5a..cfa93e3ef1a1 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/chip.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/chip.c >> @@ -212,8 +212,8 @@ struct sbsocramregs { >> #define ARMCR4_TCBANB_MASK 0xf >> #define ARMCR4_TCBANB_SHIFT 0 >> -#define ARMCR4_BSZ_MASK 0x3f >> -#define ARMCR4_BSZ_MULT 8192 Instead of deleting can we leave it here and... >> +#define ARMCR4_BSZ_MASK 0x7f >> +#define ARMCR4_BLK_1K_MASK 0x200 >> struct brcmf_core_priv { >> struct brcmf_core pub; >> @@ -675,7 +675,8 @@ static u32 brcmf_chip_sysmem_ramsize(struct >> brcmf_core_priv *sysmem) >> } >> /** Return the TCM-RAM size of the ARMCR4 core. */ >> -static u32 brcmf_chip_tcm_ramsize(struct brcmf_core_priv *cr4) >> +static u32 brcmf_chip_tcm_ramsize(struct brcmf_chip_priv *ci, >> + struct brcmf_core_priv *cr4) > > Not sure why you add ci parameter here. It is not used below or am I > overlooking something. > >> { >> u32 corecap; >> u32 memsize = 0; >> @@ -683,6 +684,7 @@ static u32 brcmf_chip_tcm_ramsize(struct >> brcmf_core_priv *cr4) >> u32 nbb; >> u32 totb; >> u32 bxinfo; >> + u32 blksize; >> u32 idx; >> corecap = brcmf_chip_core_read32(cr4, ARMCR4_CAP); >> @@ -694,7 +696,12 @@ static u32 brcmf_chip_tcm_ramsize(struct >> brcmf_core_priv *cr4) >> for (idx = 0; idx < totb; idx++) { >> brcmf_chip_core_write32(cr4, ARMCR4_BANKIDX, idx); >> bxinfo = brcmf_chip_core_read32(cr4, ARMCR4_BANKINFO); >> - memsize += ((bxinfo & ARMCR4_BSZ_MASK) + 1) * ARMCR4_BSZ_MULT; >> + if (bxinfo & ARMCR4_BLK_1K_MASK) >> + blksize = 1024; >> + else >> + blksize = 8192; ... do following here instead: blksize = 8192; if (bxinfo & ARMCR4_BLK_1K_MASK) blksize >>= 3; [not sure if mailreader is screwing with indentation or what] >> + >> + memsize += ((bxinfo & ARMCR4_BSZ_MASK) + 1) * blksize; >> } >> return memsize;
On 1/4/2022 8:26 AM, Hector Martin wrote: > The calibration blob for a chip is normally stored in SROM and loaded > internally by the firmware. However, Apple ARM64 platforms instead store > it as part of platform configuration data, and provide it via the Apple > Device Tree. We forward this into the Linux DT in the bootloader. > > Add support for taking this blob from the DT and loading it into the > dongle. The loading mechanism is the same as used for the CLM and TxCap > blobs. > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/common.c | 24 +++++++++++++++++++ > .../broadcom/brcm80211/brcmfmac/common.h | 2 ++ > .../wireless/broadcom/brcm80211/brcmfmac/of.c | 8 +++++++ > 3 files changed, 34 insertions(+)
On 1/4/2022 8:26 AM, Hector Martin wrote: > The "wlc_ver" iovar returns information on the WLC and EPI versions. > This can be used to determine whether the PMKID_V2 and _V3 features are > supported. I have my doubts whether this mechanism will be reliable, but we can wait and see whatever hits the fan over time. I stayed away from this because it is not well guarded. Especially when there are 4 entities these days spinning firmware and tinkering on the firmware api without much collaboration. Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/feature.c | 48 +++++++++++++++++++ > .../broadcom/brcm80211/brcmfmac/feature.h | 4 +- > .../broadcom/brcm80211/brcmfmac/fwil_types.h | 25 ++++++++++ > 3 files changed, 76 insertions(+), 1 deletion(-)
On 1/4/2022 8:26 AM, Hector Martin wrote: > Add support for the new PMKID_V3 API, which allows performing PMKID > mutations individually, instead of requiring the driver to keep track of > the full list. This new API is required by at least BCM4387. > > Note that PMKID_V2 is not implemented yet. > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/cfg80211.c | 52 +++++++++++- > .../broadcom/brcm80211/brcmfmac/fwil_types.h | 83 +++++++++++++++++++ > 2 files changed, 132 insertions(+), 3 deletions(-)
On 1/4/2022 8:26 AM, Hector Martin wrote: > Apparently the hex passphrase mechanism does not work on newer > chips/firmware (e.g. BCM4387). It seems there was a simple way of > passing it in binary all along, so use that and avoid the hexification. > > OpenBSD has been doing it like this from the beginning, so this should > work on all chips. > > Also clear the structure before setting the PMK. This was leaking > uninitialized stack contents to the device. > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../wireless/broadcom/brcm80211/brcmfmac/cfg80211.c | 13 +++++++------ > 1 file changed, 7 insertions(+), 6 deletions(-)
On 1/4/2022 8:26 AM, Hector Martin wrote: > This chip is present on Apple M1 Pro/Max (t600x) platforms: > > * maldives (apple,j314s): MacBook Pro (14-inch, M1 Pro, 2021) > * maldives (apple,j314c): MacBook Pro (14-inch, M1 Max, 2021) > * madagascar (apple,j316s): MacBook Pro (16-inch, M1 Pro, 2021) > * madagascar (apple,j316c): MacBook Pro (16-inch, M1 Max, 2021) > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com>> Signed-off-by: Hector Martin <marcan@marcan.st> > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/chip.c | 2 ++ > drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 8 ++++++++ > .../net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h | 2 ++ > 3 files changed, 12 insertions(+) [...] > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > index e4f2aff3c0d5..0d76440ec228 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > @@ -63,6 +63,7 @@ BRCMF_FW_DEF(4366C, "brcmfmac4366c-pcie"); > BRCMF_FW_DEF(4371, "brcmfmac4371-pcie"); > BRCMF_FW_CLM_DEF(4377B3, "brcmfmac4377b3-pcie"); > BRCMF_FW_CLM_DEF(4378B1, "brcmfmac4378b1-pcie"); > +BRCMF_FW_CLM_DEF(4387C2, "brcmfmac4387c2-pcie"); > > /* firmware config files */ > MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-pcie.txt"); > @@ -96,6 +97,7 @@ static const struct brcmf_firmware_mapping brcmf_pcie_fwnames[] = { > BRCMF_FW_ENTRY(BRCM_CC_4371_CHIP_ID, 0xFFFFFFFF, 4371), > BRCMF_FW_ENTRY(BRCM_CC_4377_CHIP_ID, 0xFFFFFFFF, 4377B3), /* 4 */ > BRCMF_FW_ENTRY(BRCM_CC_4378_CHIP_ID, 0xFFFFFFFF, 4378B1), /* 3 */ > + BRCMF_FW_ENTRY(BRCM_CC_4387_CHIP_ID, 0xFFFFFFFF, 4387C2), /* 7 */ Regarding the revmask in this firmware mapping table my common practice was to disable older revisions and enable for given revision and newer until proven otherwise. So for the 4387c2 that would have to following mask 0xFFFFFF80 (if rev 7 indeed matches with c2). > }; > > #define BRCMF_PCIE_FW_UP_TIMEOUT 5000 /* msec */
On 1/4/2022 8:26 AM, Hector Martin wrote: > The TxCap blobs are additional data blobs used on Apple devices, and > are uploaded analogously to CLM blobs. Add core support for doing this. Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Acked-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/bus.h | 1 + > .../broadcom/brcm80211/brcmfmac/common.c | 97 +++++++++++++------ > 2 files changed, 71 insertions(+), 27 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h > index b13af8f631f3..f4bd98da9761 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h > @@ -39,6 +39,7 @@ enum brcmf_bus_protocol_type { > /* Firmware blobs that may be available */ > enum brcmf_blob_type { > BRCMF_BLOB_CLM, > + BRCMF_BLOB_TXCAP, > }; > > struct brcmf_mp_device; > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c > index c84c48e49fde..d65308c3f070 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c [...] > @@ -165,20 +157,64 @@ static int brcmf_c_process_clm_blob(struct brcmf_if *ifp) > } while ((datalen > 0) && (err == 0)); > [...] > +static int brcmf_c_process_txcap_blob(struct brcmf_if *ifp) > +{ > + struct brcmf_pub *drvr = ifp->drvr; > + struct brcmf_bus *bus = drvr->bus_if; > + const struct firmware *fw = NULL; > + s32 err; > + > + brcmf_dbg(TRACE, "Enter\n"); > + > + err = brcmf_bus_get_blob(bus, &fw, BRCMF_BLOB_TXCAP); > + if (err || !fw) { > + brcmf_info("no txcap_blob available (err=%d)\n", err); > + return 0; > + } > + > + brcmf_info("TxCap blob found, loading\n"); > + err = brcmf_c_download_blob(ifp, fw->data, fw->size, > + "txcapload", "txcapload_status"); Although unlikely that we end up here with a firmware that does not support this command it is not impossible. Should we handle that here or introduce a feature flag for txcap loading? > + release_firmware(fw); > return err; > }
On 1/4/2022 8:26 AM, Hector Martin wrote: > These blobs are named .txcap_blob, and exist alongside the existing > .clm_blob files. Use the existing firmware machinery to provide them to > the core. > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 13 +++++++++++++ > 1 file changed, 13 insertions(+)
On 19/01/2022 21.36, Arend van Spriel wrote: >> /** Return the TCM-RAM size of the ARMCR4 core. */ >> -static u32 brcmf_chip_tcm_ramsize(struct brcmf_core_priv *cr4) >> +static u32 brcmf_chip_tcm_ramsize(struct brcmf_chip_priv *ci, >> + struct brcmf_core_priv *cr4) > > Not sure why you add ci parameter here. It is not used below or am I > overlooking something. Oops, looks like junk left behind from when I was trying to figure this out. Removed. Sorry about that.
On 21/01/2022 16.36, Arend van Spriel wrote: > On 1/4/2022 8:26 AM, Hector Martin wrote: >> The TxCap blobs are additional data blobs used on Apple devices, and >> are uploaded analogously to CLM blobs. Add core support for doing this. > > Reviewed-by: Arend van Spriel <arend.vanspriel@broadcom.com> >> Acked-by: Linus Walleij <linus.walleij@linaro.org> >> Signed-off-by: Hector Martin <marcan@marcan.st> >> --- >> .../broadcom/brcm80211/brcmfmac/bus.h | 1 + >> .../broadcom/brcm80211/brcmfmac/common.c | 97 +++++++++++++------ >> 2 files changed, 71 insertions(+), 27 deletions(-) >> >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h >> index b13af8f631f3..f4bd98da9761 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/bus.h >> @@ -39,6 +39,7 @@ enum brcmf_bus_protocol_type { >> /* Firmware blobs that may be available */ >> enum brcmf_blob_type { >> BRCMF_BLOB_CLM, >> + BRCMF_BLOB_TXCAP, >> }; >> >> struct brcmf_mp_device; >> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c >> index c84c48e49fde..d65308c3f070 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c >> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/common.c > > [...] > >> @@ -165,20 +157,64 @@ static int brcmf_c_process_clm_blob(struct brcmf_if *ifp) >> } while ((datalen > 0) && (err == 0)); >> > > [...] > >> +static int brcmf_c_process_txcap_blob(struct brcmf_if *ifp) >> +{ >> + struct brcmf_pub *drvr = ifp->drvr; >> + struct brcmf_bus *bus = drvr->bus_if; >> + const struct firmware *fw = NULL; >> + s32 err; >> + >> + brcmf_dbg(TRACE, "Enter\n"); >> + >> + err = brcmf_bus_get_blob(bus, &fw, BRCMF_BLOB_TXCAP); >> + if (err || !fw) { >> + brcmf_info("no txcap_blob available (err=%d)\n", err); >> + return 0; >> + } >> + >> + brcmf_info("TxCap blob found, loading\n"); >> + err = brcmf_c_download_blob(ifp, fw->data, fw->size, >> + "txcapload", "txcapload_status"); > > Although unlikely that we end up here with a firmware that does not > support this command it is not impossible. Should we handle that here or > introduce a feature flag for txcap loading? Hmm, like trying to read txcapload_status to set the feature flag? Honestly though, if we end up here on an unsupported firmware that sounds like a firmware loading error, since if we have a TxCap blob for a given board we better have a firmware that supports it. So it doesn't feel too wrong to just error out entirely so the user knows something is horribly wrong, instead of trying to use what is probably the wrong firmware.
On 21/01/2022 16.35, Arend van Spriel wrote: > On 1/4/2022 8:26 AM, Hector Martin wrote: >> @@ -96,6 +97,7 @@ static const struct brcmf_firmware_mapping brcmf_pcie_fwnames[] = { >> BRCMF_FW_ENTRY(BRCM_CC_4371_CHIP_ID, 0xFFFFFFFF, 4371), >> BRCMF_FW_ENTRY(BRCM_CC_4377_CHIP_ID, 0xFFFFFFFF, 4377B3), /* 4 */ >> BRCMF_FW_ENTRY(BRCM_CC_4378_CHIP_ID, 0xFFFFFFFF, 4378B1), /* 3 */ >> + BRCMF_FW_ENTRY(BRCM_CC_4387_CHIP_ID, 0xFFFFFFFF, 4387C2), /* 7 */ > > Regarding the revmask in this firmware mapping table my common practice > was to disable older revisions and enable for given revision and newer > until proven otherwise. So for the 4387c2 that would have to following > mask 0xFFFFFF80 (if rev 7 indeed matches with c2). > Makes sense, I changed it to that for all the additions :)