mbox series

[v2,00/35] brcmfmac: Support Apple T2 and M1 platforms

Message ID 20220104072658.69756-1-marcan@marcan.st
Headers show
Series brcmfmac: Support Apple T2 and M1 platforms | expand

Message

Hector Martin Jan. 4, 2022, 7:26 a.m. UTC
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.

# Acknowledgements

This patch series was developed referencing the OpenBSD support for the
BCM4378 [3] and the bcmdhd-4359 GPL release [4], which contained some
interesting tidbits about newer chips, registers, OTP, etc.

[1] https://gist.github.com/marcan/5cfaad948e224279f09a4a79ccafd9b6
[2] https://github.com/AsahiLinux/asahi-installer/blob/main/src/firmware/wifi.py
[3] https://github.com/openbsd/src/blob/master/sys/dev/pci/if_bwfm_pci.c
[4] https://github.com/StreamUnlimited/broadcom-bcmdhd-4359/
[5] https://github.com/StreamUnlimited/broadcom-bcmdhd-4359/blob/master/dhd_pcie.h#L594

# Known bugs

WPA3 does not yet work on M1 platforms. This is probably more missing
firmware interfaces; I'll look into it shortly and it can go into v3 or
a separate add-on patch.

# Changes since v1

- Replaced new DT compatibles with pciXXXX,YYYY ones (this seems to be
  the way to do it)
- Replaced apple,module instance DT prop with brcm,board-type (more
  generic)
- Reduced stack usage of brcmf_pmksa_v3_op
- Changed alt_paths/board_names to be a fixed, max size instead of
  statically allocated.
- Fixed broken build halfway through the series
- Addressed other review comments (style/etc)
- Fixed typos and other minor issues

Hector Martin (35):
  dt-bindings: net: bcm4329-fmac: Add Apple properties & chips
  brcmfmac: pcie: Declare missing firmware files in pcie.c
  brcmfmac: firmware: Handle per-board clm_blob files
  brcmfmac: firmware: Support having multiple alt paths
  brcmfmac: pcie/sdio/usb: Get CLM blob via standard firmware mechanism
  brcmfmac: firmware: Support passing in multiple board_types
  brcmfmac: pcie: Read Apple OTP information
  brcmfmac: of: Fetch Apple properties
  brcmfmac: pcie: Perform firmware selection for Apple platforms
  brcmfmac: firmware: Allow platform to override macaddr
  brcmfmac: msgbuf: Increase RX ring sizes to 1024
  brcmfmac: pcie: Fix crashes due to early IRQs
  brcmfmac: pcie: Support PCIe core revisions >= 64
  brcmfmac: pcie: Add IDs/properties for BCM4378
  ACPI / property: Support strings in Apple _DSM props
  brcmfmac: acpi: Add support for fetching Apple ACPI properties
  brcmfmac: pcie: Provide a buffer of random bytes to the device
  brcmfmac: pcie: Add IDs/properties for BCM4355
  brcmfmac: pcie: Add IDs/properties for BCM4377
  brcmfmac: pcie: Perform correct BCM4364 firmware selection
  brcmfmac: chip: Only disable D11 cores; handle an arbitrary number
  brcmfmac: chip: Handle 1024-unit sizes for TCM blocks
  brcmfmac: cfg80211: Add support for scan params v2
  brcmfmac: feature: Add support for setting feats based on WLC version
  brcmfmac: cfg80211: Add support for PMKID_V3 operations
  brcmfmac: cfg80211: Pass the PMK in binary instead of hex
  brcmfmac: pcie: Add IDs/properties for BCM4387
  brcmfmac: pcie: Replace brcmf_pcie_copy_mem_todev with memcpy_toio
  brcmfmac: pcie: Read the console on init and shutdown
  brcmfmac: pcie: Release firmwares in the brcmf_pcie_setup error path
  brcmfmac: firmware: Allocate space for default boardrev in nvram
  brcmfmac: fwil: Constify iovar name arguments
  brcmfmac: common: Add support for downloading TxCap blobs
  brcmfmac: pcie: Load and provide TxCap blobs
  brcmfmac: common: Add support for external calibration blobs

 .../net/wireless/brcm,bcm4329-fmac.yaml       |  37 +-
 drivers/acpi/x86/apple.c                      |  11 +-
 .../broadcom/brcm80211/brcmfmac/Makefile      |   2 +
 .../broadcom/brcm80211/brcmfmac/acpi.c        |  47 ++
 .../broadcom/brcm80211/brcmfmac/bus.h         |  20 +-
 .../broadcom/brcm80211/brcmfmac/cfg80211.c    | 178 +++++-
 .../broadcom/brcm80211/brcmfmac/chip.c        |  36 +-
 .../broadcom/brcm80211/brcmfmac/common.c      | 130 +++-
 .../broadcom/brcm80211/brcmfmac/common.h      |  12 +
 .../broadcom/brcm80211/brcmfmac/feature.c     |  49 ++
 .../broadcom/brcm80211/brcmfmac/feature.h     |   6 +-
 .../broadcom/brcm80211/brcmfmac/firmware.c    | 136 +++-
 .../broadcom/brcm80211/brcmfmac/firmware.h    |   4 +-
 .../broadcom/brcm80211/brcmfmac/fwil.c        |  34 +-
 .../broadcom/brcm80211/brcmfmac/fwil.h        |  28 +-
 .../broadcom/brcm80211/brcmfmac/fwil_types.h  | 157 ++++-
 .../broadcom/brcm80211/brcmfmac/msgbuf.h      |   4 +-
 .../wireless/broadcom/brcm80211/brcmfmac/of.c |  20 +-
 .../broadcom/brcm80211/brcmfmac/pcie.c        | 599 +++++++++++++++---
 .../broadcom/brcm80211/brcmfmac/sdio.c        |  39 +-
 .../broadcom/brcm80211/brcmfmac/sdio.h        |   2 +
 .../broadcom/brcm80211/brcmfmac/usb.c         |  23 +-
 .../broadcom/brcm80211/include/brcm_hw_ids.h  |   8 +
 include/linux/bcma/bcma_driver_chipcommon.h   |   1 +
 24 files changed, 1313 insertions(+), 270 deletions(-)
 create mode 100644 drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c

Comments

Andy Shevchenko Jan. 4, 2022, 2:24 p.m. UTC | #1
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;
Andy Shevchenko Jan. 4, 2022, 2:28 p.m. UTC | #2
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.
Arend van Spriel Jan. 4, 2022, 7:46 p.m. UTC | #3
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
Hector Martin Jan. 6, 2022, 1:12 p.m. UTC | #4
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;
>
Kalle Valo Jan. 10, 2022, 1:46 p.m. UTC | #5
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 Jan. 17, 2022, 6:57 a.m. UTC | #6
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 :)