| Message ID | 20211226153624.162281-2-marcan@marcan.st |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | brcmfmac: Support Apple T2 and M1 platforms | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dtbs-check | success | |
| robh/dt-meta-schema | fail | build log |
On Sun, Dec 26, 2021 at 4:36 PM Hector Martin <marcan@marcan.st> wrote: > This binding is currently used for SDIO devices, but these chips are > also used as PCIe devices on DT platforms and may be represented in the > DT. Re-use the existing binding and add chip compatibles used by Apple > T2 and M1 platforms (the T2 ones are not known to be used in DT > platforms, but we might as well document them). > > Then, add properties required for firmware selection and calibration on > M1 machines. > > Signed-off-by: Hector Martin <marcan@marcan.st> Makes sense to me! Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > + brcm,cal-blob: > + $ref: /schemas/types.yaml#/definitions/uint8-array > + description: A per-device calibration blob for the Wi-Fi radio. This > + should be filled in by the bootloader from platform configuration > + data, if necessary, and will be uploaded to the device if present. This is especially nice. This way on other systems U-Boot can read the calibration file (usually stored in a special partition) and modify the device tree to include this, then we don't need the driver to learn about any specific file locations for calibrations or worry about inserting it from userspace. Yours, Linus Walleij
On Mon, 27 Dec 2021 00:35:51 +0900, Hector Martin wrote: > This binding is currently used for SDIO devices, but these chips are > also used as PCIe devices on DT platforms and may be represented in the > DT. Re-use the existing binding and add chip compatibles used by Apple > T2 and M1 platforms (the T2 ones are not known to be used in DT > platforms, but we might as well document them). > > Then, add properties required for firmware selection and calibration on > M1 machines. > > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../net/wireless/brcm,bcm4329-fmac.yaml | 32 +++++++++++++++++-- > 1 file changed, 29 insertions(+), 3 deletions(-) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml: properties:apple,antenna-sku: '$def' is not one of ['$ref', 'additionalItems', 'additionalProperties', 'allOf', 'anyOf', 'const', 'contains', 'default', 'dependencies', 'dependentRequired', 'dependentSchemas', 'deprecated', 'description', 'else', 'enum', 'exclusiveMaximum', 'exclusiveMinimum', 'items', 'if', 'minItems', 'minimum', 'maxItems', 'maximum', 'multipleOf', 'not', 'oneOf', 'pattern', 'patternProperties', 'properties', 'required', 'then', 'type', 'typeSize', 'unevaluatedProperties', 'uniqueItems'] from schema $id: http://devicetree.org/meta-schemas/keywords.yaml# /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml: properties:apple,antenna-sku: 'oneOf' conditional failed, one must be fixed: 'type' is a required property hint: A vendor boolean property can use "type: boolean" Additional properties are not allowed ('$def' was unexpected) hint: A vendor boolean property can use "type: boolean" /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml: properties:apple,antenna-sku: 'oneOf' conditional failed, one must be fixed: 'enum' is a required property 'const' is a required property hint: A vendor string property with exact values has an implicit type from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml# /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml: properties:apple,antenna-sku: 'oneOf' conditional failed, one must be fixed: '$ref' is a required property 'allOf' is a required property hint: A vendor property needs a $ref to types.yaml from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml# hint: Vendor specific properties must have a type and description unless they have a defined, common suffix. from schema $id: http://devicetree.org/meta-schemas/vendor-props.yaml# /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml: ignoring, error in schema: properties: apple,antenna-sku Documentation/devicetree/bindings/mmc/mmc-controller.example.dt.yaml:0:0: /example-0/mmc@1c12000/wifi@1: failed to match any schema with compatible: ['brcm,bcm4329-fmac'] Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.example.dt.yaml:0:0: /example-0/mmc@80118000/wifi@1: failed to match any schema with compatible: ['brcm,bcm4334-fmac', 'brcm,bcm4329-fmac'] Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.example.dt.yaml:0:0: /example-0/mmc@80118000/wifi@1: failed to match any schema with compatible: ['brcm,bcm4334-fmac', 'brcm,bcm4329-fmac'] doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/1573232 This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
On Mon, Dec 27, 2021 at 12:35:51AM +0900, Hector Martin wrote: > This binding is currently used for SDIO devices, but these chips are > also used as PCIe devices on DT platforms and may be represented in the > DT. Re-use the existing binding and add chip compatibles used by Apple > T2 and M1 platforms (the T2 ones are not known to be used in DT > platforms, but we might as well document them). > > Then, add properties required for firmware selection and calibration on > M1 machines. > > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../net/wireless/brcm,bcm4329-fmac.yaml | 32 +++++++++++++++++-- > 1 file changed, 29 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml b/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml > index c11f23b20c4c..2530ff3e7b90 100644 > --- a/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml > +++ b/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml > @@ -4,7 +4,7 @@ > $id: http://devicetree.org/schemas/net/wireless/brcm,bcm4329-fmac.yaml# > $schema: http://devicetree.org/meta-schemas/core.yaml# > > -title: Broadcom BCM4329 family fullmac wireless SDIO devices > +title: Broadcom BCM4329 family fullmac wireless SDIO/PCIE devices > > maintainers: > - Arend van Spriel <arend@broadcom.com> > @@ -36,16 +36,22 @@ properties: > - brcm,bcm43455-fmac > - brcm,bcm43456-fmac > - brcm,bcm4354-fmac > + - brcm,bcm4355c1-fmac > - brcm,bcm4356-fmac > - brcm,bcm4359-fmac > + - brcm,bcm4364b2-fmac > + - brcm,bcm4364b3-fmac > + - brcm,bcm4377b3-fmac > + - brcm,bcm4378b1-fmac > + - brcm,bcm4387c2-fmac > - cypress,cyw4373-fmac > - cypress,cyw43012-fmac > - const: brcm,bcm4329-fmac > - const: brcm,bcm4329-fmac > > reg: > - description: SDIO function number for the device, for most cases > - this will be 1. > + description: SDIO function number for the device (for most cases > + this will be 1) or PCI device identifier. > > interrupts: > maxItems: 1 > @@ -75,6 +81,26 @@ properties: > items: > pattern: '^[A-Z][A-Z]-[A-Z][0-9A-Z]-[0-9]+$' > > + brcm,cal-blob: > + $ref: /schemas/types.yaml#/definitions/uint8-array > + description: A per-device calibration blob for the Wi-Fi radio. This > + should be filled in by the bootloader from platform configuration > + data, if necessary, and will be uploaded to the device if present. > + > + apple,module-instance: > + $ref: /schemas/types.yaml#/definitions/string > + description: Module codename used to identify a specific board on > + Apple platforms. This is used to build the firmware filenames, to allow > + different platforms to have different firmware and/or NVRAM config. > + > + apple,antenna-sku: > + $def: /schemas/types.yaml#/definitions/string > + description: Antenna SKU used to identify a specific antenna configuration > + on Apple platforms. This is use to build firmware filenames, to allow > + platforms with different antenna configs to have different firmware and/or > + NVRAM. This would normally be filled in by the bootloader from platform > + configuration data. Is there a known set of strings that can be defined? There's also the somewhat standard 'firmware-name' property that serves similar purpose, but if there's multiple files, then I guess this approach is fine. Rob
On 28/12/2021 01.36, Rob Herring wrote: > On Mon, Dec 27, 2021 at 12:35:51AM +0900, Hector Martin wrote: >> + brcm,cal-blob: >> + $ref: /schemas/types.yaml#/definitions/uint8-array >> + description: A per-device calibration blob for the Wi-Fi radio. This >> + should be filled in by the bootloader from platform configuration >> + data, if necessary, and will be uploaded to the device if present. >> + >> + apple,module-instance: >> + $ref: /schemas/types.yaml#/definitions/string >> + description: Module codename used to identify a specific board on >> + Apple platforms. This is used to build the firmware filenames, to allow >> + different platforms to have different firmware and/or NVRAM config. >> + >> + apple,antenna-sku: >> + $def: /schemas/types.yaml#/definitions/string >> + description: Antenna SKU used to identify a specific antenna configuration >> + on Apple platforms. This is use to build firmware filenames, to allow >> + platforms with different antenna configs to have different firmware and/or >> + NVRAM. This would normally be filled in by the bootloader from platform >> + configuration data. > > Is there a known set of strings that can be defined? For apple,module-instance there is, though it will grow with every new machine. If you're happy with me pushing updates to this through asahi-soc I can keep it maintained as we add DTs and compatibles there. I'm curious whether you prefer this approach or something like brcm,board-name instead. Right now we do: apple,module-instance = "honshu" That gets converted to board_name="apple,honshu" in the code, which is what the firmwares are named after (plus extra info later appended, if the rest of the Apple data is available). But we could also do: brcm,board-name = "apple,honshu" The latter would be more generically useful for other platforms, since it would allow e.g. having DTs for different boards that use the same WiFi module/subsystem and thus a compatible NVRAM fw file alias to the same file name (right now this is done with symlinks in /lib/firmware, one for each equivalent board). For non-Apple platforms (i.e. if antenna-sku and/or the OTP aren't available to do the funky Apple firmware selection), this just ends up replacing what would normally be the OF root node compatible in the firmware filename. E.g. right now we have: brcmfmac43430-sdio.AP6212.txt brcmfmac43430-sdio.raspberrypi,3-model-b.txt brcmfmac43430-sdio.raspberrypi,model-zero-w.txt -> brcmfmac43430-sdio.raspberrypi,3-model-b.txt brcmfmac43430-sdio.sinovoip,bpi-m2-plus.txt -> brcmfmac43430-sdio.AP6212.txt brcmfmac43430-sdio.sinovoip,bpi-m2-ultra.txt -> brcmfmac43430-sdio.AP6212.txt brcmfmac43430-sdio.sinovoip,bpi-m2-zero.txt -> brcmfmac43430-sdio.AP6212.txt brcmfmac43430-sdio.sinovoip,bpi-m3.txt -> brcmfmac43430-sdio.AP6212.txt And this could allow the sinovoip.* DTs to say: brcm,board-name = "AP6212"; And the rPi zero one: brcm,board-name = "raspberrypi,3-model-b"; And avoid the symlinks. The antenna-sku thing is specific to the Apple firmware selection process and doesn't make sense as a more generic property. antenna-sku right now always seems to be one of "ID", "X0", "X2", "X3", though that could presumably change in the future. I can add this to the binding if you want, though since this will be filled in by the bootloader from platform data we wouldn't be validating it anyway. Not sure if it's worth it. > There's also the somewhat standard 'firmware-name' property that > serves similar purpose, but if there's multiple files, then I guess > this approach is fine. Yeah, and the firmware name is constructed using non-DT information too (and we have several attempted filenames times several firmware types), so it wouldn't be complete.
> From: Hector Martin <marcan@marcan.st> > Date: Tue, 28 Dec 2021 02:23:02 +0900 > > On 28/12/2021 01.36, Rob Herring wrote: > > On Mon, Dec 27, 2021 at 12:35:51AM +0900, Hector Martin wrote: > >> + brcm,cal-blob: > >> + $ref: /schemas/types.yaml#/definitions/uint8-array > >> + description: A per-device calibration blob for the Wi-Fi radio. This > >> + should be filled in by the bootloader from platform configuration > >> + data, if necessary, and will be uploaded to the device if present. > >> + > >> + apple,module-instance: > >> + $ref: /schemas/types.yaml#/definitions/string > >> + description: Module codename used to identify a specific board on > >> + Apple platforms. This is used to build the firmware filenames, to allow > >> + different platforms to have different firmware and/or NVRAM config. > >> + > >> + apple,antenna-sku: > >> + $def: /schemas/types.yaml#/definitions/string > >> + description: Antenna SKU used to identify a specific antenna configuration > >> + on Apple platforms. This is use to build firmware filenames, to allow > >> + platforms with different antenna configs to have different firmware and/or > >> + NVRAM. This would normally be filled in by the bootloader from platform > >> + configuration data. > > > > Is there a known set of strings that can be defined? > > For apple,module-instance there is, though it will grow with every new > machine. If you're happy with me pushing updates to this through > asahi-soc I can keep it maintained as we add DTs and compatibles there. > > I'm curious whether you prefer this approach or something like > brcm,board-name instead. Right now we do: > > apple,module-instance = "honshu" > > That gets converted to board_name="apple,honshu" in the code, which is > what the firmwares are named after (plus extra info later appended, if > the rest of the Apple data is available). > > But we could also do: > > brcm,board-name = "apple,honshu" > > The latter would be more generically useful for other platforms, since > it would allow e.g. having DTs for different boards that use the same > WiFi module/subsystem and thus a compatible NVRAM fw file alias to the > same file name (right now this is done with symlinks in /lib/firmware, > one for each equivalent board). For non-Apple platforms (i.e. if > antenna-sku and/or the OTP aren't available to do the funky Apple > firmware selection), this just ends up replacing what would normally be > the OF root node compatible in the firmware filename. > > E.g. right now we have: > > brcmfmac43430-sdio.AP6212.txt > brcmfmac43430-sdio.raspberrypi,3-model-b.txt > brcmfmac43430-sdio.raspberrypi,model-zero-w.txt -> brcmfmac43430-sdio.raspberrypi,3-model-b.txt > brcmfmac43430-sdio.sinovoip,bpi-m2-plus.txt -> brcmfmac43430-sdio.AP6212.txt > brcmfmac43430-sdio.sinovoip,bpi-m2-ultra.txt -> brcmfmac43430-sdio.AP6212.txt > brcmfmac43430-sdio.sinovoip,bpi-m2-zero.txt -> brcmfmac43430-sdio.AP6212.txt > brcmfmac43430-sdio.sinovoip,bpi-m3.txt -> brcmfmac43430-sdio.AP6212.txt > > And this could allow the sinovoip.* DTs to say: > brcm,board-name = "AP6212"; > > And the rPi zero one: > brcm,board-name = "raspberrypi,3-model-b"; > > And avoid the symlinks. > > The antenna-sku thing is specific to the Apple firmware selection > process and doesn't make sense as a more generic property. > > antenna-sku right now always seems to be one of "ID", "X0", "X2", "X3", > though that could presumably change in the future. I can add this to the > binding if you want, though since this will be filled in by the > bootloader from platform data we wouldn't be validating it anyway. Not > sure if it's worth it. Actually what Apple does here makes quite a bit of sense. Typically WiFi chips are integrated with some analog components into a shielded module. The AP6212 mentioned above is an example of such a module. I suspect that the module defines some of the characteristics encoded in the "nvmram" files, but certainly not all because the connected antenna will also affect how the thing behaves. Of course many SBCs come without an antenna so the actual antenna depends on whatever the user connects to the board. So using a module-specific "nvram" file is probably the best one can do here. So I think if you want to have a generic module name property, it should be called "brcm,module-name" instead of "brcm,board-name". However... > > There's also the somewhat standard 'firmware-name' property that > > serves similar purpose, but if there's multiple files, then I guess > > this approach is fine. > > Yeah, and the firmware name is constructed using non-DT information too > (and we have several attempted filenames times several firmware types), > so it wouldn't be complete. ...if the way the firmware name is constructed remains Apple-specific because of this non-DT information, keeping the "apple,xxx" properties has the benefit of signalling that firmware names constructed this way are desired. Or rather, their absence can signal that the Apple-specific code in the driver should be skipped.
> From: Hector Martin <marcan@marcan.st> > Cc: Hector Martin <marcan@marcan.st>, Sven Peter <sven@svenpeter.dev>, > Alyssa Rosenzweig <alyssa@rosenzweig.io>, > Mark Kettenis <kettenis@openbsd.org>, > Rafał Miłecki <zajec5@gmail.com>, > Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>, > Linus Walleij <linus.walleij@linaro.org>, > Hans de Goede <hdegoede@redhat.com>, > "John W. Linville" <linville@tuxdriver.com>, > "Daniel (Deognyoun) Kim" <dekim@broadcom.com>, > "brian m. carlson" <sandals@crustytoothpaste.net>, > linux-wireless@vger.kernel.org, netdev@vger.kernel.org, > devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, > linux-acpi@vger.kernel.org, brcm80211-dev-list.pdl@broadcom.com, > SHA-cyfmac-dev-list@infineon.com > Date: Mon, 27 Dec 2021 00:35:51 +0900 > > This binding is currently used for SDIO devices, but these chips are > also used as PCIe devices on DT platforms and may be represented in the > DT. Re-use the existing binding and add chip compatibles used by Apple > T2 and M1 platforms (the T2 ones are not known to be used in DT > platforms, but we might as well document them). > > Then, add properties required for firmware selection and calibration on > M1 machines. > > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../net/wireless/brcm,bcm4329-fmac.yaml | 32 +++++++++++++++++-- > 1 file changed, 29 insertions(+), 3 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml b/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml > index c11f23b20c4c..2530ff3e7b90 100644 > --- a/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml > +++ b/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml > @@ -4,7 +4,7 @@ > $id: http://devicetree.org/schemas/net/wireless/brcm,bcm4329-fmac.yaml# > $schema: http://devicetree.org/meta-schemas/core.yaml# > > -title: Broadcom BCM4329 family fullmac wireless SDIO devices > +title: Broadcom BCM4329 family fullmac wireless SDIO/PCIE devices > > maintainers: > - Arend van Spriel <arend@broadcom.com> > @@ -36,16 +36,22 @@ properties: > - brcm,bcm43455-fmac > - brcm,bcm43456-fmac > - brcm,bcm4354-fmac > + - brcm,bcm4355c1-fmac > - brcm,bcm4356-fmac > - brcm,bcm4359-fmac > + - brcm,bcm4364b2-fmac > + - brcm,bcm4364b3-fmac > + - brcm,bcm4377b3-fmac > + - brcm,bcm4378b1-fmac > + - brcm,bcm4387c2-fmac > - cypress,cyw4373-fmac > - cypress,cyw43012-fmac > - const: brcm,bcm4329-fmac > - const: brcm,bcm4329-fmac I suppose this helps with validation of device trees. However, nodes for PCI devices are not supposed to have a "compatible" property as the PCI vendor and device IDs are supposed to be used to identify a device. That does raise the question how a schema for additional properties for PCI device nodes is supposed to be defined... > reg: > - description: SDIO function number for the device, for most cases > - this will be 1. > + description: SDIO function number for the device (for most cases > + this will be 1) or PCI device identifier. > > interrupts: > maxItems: 1 > @@ -75,6 +81,26 @@ properties: > items: > pattern: '^[A-Z][A-Z]-[A-Z][0-9A-Z]-[0-9]+$' > > + brcm,cal-blob: > + $ref: /schemas/types.yaml#/definitions/uint8-array > + description: A per-device calibration blob for the Wi-Fi radio. This > + should be filled in by the bootloader from platform configuration > + data, if necessary, and will be uploaded to the device if present. > + > + apple,module-instance: > + $ref: /schemas/types.yaml#/definitions/string > + description: Module codename used to identify a specific board on > + Apple platforms. This is used to build the firmware filenames, to allow > + different platforms to have different firmware and/or NVRAM config. > + > + apple,antenna-sku: > + $def: /schemas/types.yaml#/definitions/string > + description: Antenna SKU used to identify a specific antenna configuration > + on Apple platforms. This is use to build firmware filenames, to allow > + platforms with different antenna configs to have different firmware and/or > + NVRAM. This would normally be filled in by the bootloader from platform > + configuration data. > + > required: > - compatible > - reg > -- > 2.33.0 > >
On 2021/12/30 1:38, Mark Kettenis wrote: >> From: Hector Martin <marcan@marcan.st> >> Date: Tue, 28 Dec 2021 02:23:02 +0900 >> >> On 28/12/2021 01.36, Rob Herring wrote: >>> On Mon, Dec 27, 2021 at 12:35:51AM +0900, Hector Martin wrote: >>>> + brcm,cal-blob: >>>> + $ref: /schemas/types.yaml#/definitions/uint8-array >>>> + description: A per-device calibration blob for the Wi-Fi radio. This >>>> + should be filled in by the bootloader from platform configuration >>>> + data, if necessary, and will be uploaded to the device if present. >>>> + >>>> + apple,module-instance: >>>> + $ref: /schemas/types.yaml#/definitions/string >>>> + description: Module codename used to identify a specific board on >>>> + Apple platforms. This is used to build the firmware filenames, to allow >>>> + different platforms to have different firmware and/or NVRAM config. >>>> + >>>> + apple,antenna-sku: >>>> + $def: /schemas/types.yaml#/definitions/string >>>> + description: Antenna SKU used to identify a specific antenna configuration >>>> + on Apple platforms. This is use to build firmware filenames, to allow >>>> + platforms with different antenna configs to have different firmware and/or >>>> + NVRAM. This would normally be filled in by the bootloader from platform >>>> + configuration data. >>> >>> Is there a known set of strings that can be defined? >> >> For apple,module-instance there is, though it will grow with every new >> machine. If you're happy with me pushing updates to this through >> asahi-soc I can keep it maintained as we add DTs and compatibles there. >> >> I'm curious whether you prefer this approach or something like >> brcm,board-name instead. Right now we do: >> >> apple,module-instance = "honshu" >> >> That gets converted to board_name="apple,honshu" in the code, which is >> what the firmwares are named after (plus extra info later appended, if >> the rest of the Apple data is available). >> >> But we could also do: >> >> brcm,board-name = "apple,honshu" >> >> The latter would be more generically useful for other platforms, since >> it would allow e.g. having DTs for different boards that use the same >> WiFi module/subsystem and thus a compatible NVRAM fw file alias to the >> same file name (right now this is done with symlinks in /lib/firmware, >> one for each equivalent board). For non-Apple platforms (i.e. if >> antenna-sku and/or the OTP aren't available to do the funky Apple >> firmware selection), this just ends up replacing what would normally be >> the OF root node compatible in the firmware filename. >> >> E.g. right now we have: >> >> brcmfmac43430-sdio.AP6212.txt >> brcmfmac43430-sdio.raspberrypi,3-model-b.txt >> brcmfmac43430-sdio.raspberrypi,model-zero-w.txt -> brcmfmac43430-sdio.raspberrypi,3-model-b.txt >> brcmfmac43430-sdio.sinovoip,bpi-m2-plus.txt -> brcmfmac43430-sdio.AP6212.txt >> brcmfmac43430-sdio.sinovoip,bpi-m2-ultra.txt -> brcmfmac43430-sdio.AP6212.txt >> brcmfmac43430-sdio.sinovoip,bpi-m2-zero.txt -> brcmfmac43430-sdio.AP6212.txt >> brcmfmac43430-sdio.sinovoip,bpi-m3.txt -> brcmfmac43430-sdio.AP6212.txt >> >> And this could allow the sinovoip.* DTs to say: >> brcm,board-name = "AP6212"; >> >> And the rPi zero one: >> brcm,board-name = "raspberrypi,3-model-b"; >> >> And avoid the symlinks. >> >> The antenna-sku thing is specific to the Apple firmware selection >> process and doesn't make sense as a more generic property. >> >> antenna-sku right now always seems to be one of "ID", "X0", "X2", "X3", >> though that could presumably change in the future. I can add this to the >> binding if you want, though since this will be filled in by the >> bootloader from platform data we wouldn't be validating it anyway. Not >> sure if it's worth it. > > Actually what Apple does here makes quite a bit of sense. Typically > WiFi chips are integrated with some analog components into a shielded > module. The AP6212 mentioned above is an example of such a module. I > suspect that the module defines some of the characteristics encoded in > the "nvmram" files, but certainly not all because the connected > antenna will also affect how the thing behaves. Of course many SBCs > come without an antenna so the actual antenna depends on whatever the > user connects to the board. So using a module-specific "nvram" file > is probably the best one can do here. So I think if you want to have > a generic module name property, it should be called "brcm,module-name" > instead of "brcm,board-name". However... > >>> There's also the somewhat standard 'firmware-name' property that >>> serves similar purpose, but if there's multiple files, then I guess >>> this approach is fine. >> >> Yeah, and the firmware name is constructed using non-DT information too >> (and we have several attempted filenames times several firmware types), >> so it wouldn't be complete. > > ...if the way the firmware name is constructed remains Apple-specific > because of this non-DT information, keeping the "apple,xxx" properties > has the benefit of signalling that firmware names constructed this way > are desired. Or rather, their absence can signal that the > Apple-specific code in the driver should be skipped. > The current logic is that if all the information is available (OTP, antenna-sku, board-name but that always gets filled in by default from DT/DMI global info) it will use the Apple mechanism, otherwise it will use the standard one. So in this case we could still use brcm,board-name/module-name for Apple devices, and we'd keep apple,antenna-sku as one of the two triggers to do Apple firmware selection. Other devices may use nothing (use default DMI or DT device name/compatible as module-name) or specify an override, and they will still get firmwares with that name in them and a fallback to generic firmware, which is the current behavior.
On 2021/12/30 1:42, Mark Kettenis wrote: >> From: Hector Martin <marcan@marcan.st> >> Cc: Hector Martin <marcan@marcan.st>, Sven Peter <sven@svenpeter.dev>, >> Alyssa Rosenzweig <alyssa@rosenzweig.io>, >> Mark Kettenis <kettenis@openbsd.org>, >> Rafał Miłecki <zajec5@gmail.com>, >> Pieter-Paul Giesberts <pieter-paul.giesberts@broadcom.com>, >> Linus Walleij <linus.walleij@linaro.org>, >> Hans de Goede <hdegoede@redhat.com>, >> "John W. Linville" <linville@tuxdriver.com>, >> "Daniel (Deognyoun) Kim" <dekim@broadcom.com>, >> "brian m. carlson" <sandals@crustytoothpaste.net>, >> linux-wireless@vger.kernel.org, netdev@vger.kernel.org, >> devicetree@vger.kernel.org, linux-kernel@vger.kernel.org, >> linux-acpi@vger.kernel.org, brcm80211-dev-list.pdl@broadcom.com, >> SHA-cyfmac-dev-list@infineon.com >> Date: Mon, 27 Dec 2021 00:35:51 +0900 >> >> This binding is currently used for SDIO devices, but these chips are >> also used as PCIe devices on DT platforms and may be represented in the >> DT. Re-use the existing binding and add chip compatibles used by Apple >> T2 and M1 platforms (the T2 ones are not known to be used in DT >> platforms, but we might as well document them). >> >> Then, add properties required for firmware selection and calibration on >> M1 machines. >> >> Signed-off-by: Hector Martin <marcan@marcan.st> >> --- >> .../net/wireless/brcm,bcm4329-fmac.yaml | 32 +++++++++++++++++-- >> 1 file changed, 29 insertions(+), 3 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml b/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml >> index c11f23b20c4c..2530ff3e7b90 100644 >> --- a/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml >> +++ b/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml >> @@ -4,7 +4,7 @@ >> $id: http://devicetree.org/schemas/net/wireless/brcm,bcm4329-fmac.yaml# >> $schema: http://devicetree.org/meta-schemas/core.yaml# >> >> -title: Broadcom BCM4329 family fullmac wireless SDIO devices >> +title: Broadcom BCM4329 family fullmac wireless SDIO/PCIE devices >> >> maintainers: >> - Arend van Spriel <arend@broadcom.com> >> @@ -36,16 +36,22 @@ properties: >> - brcm,bcm43455-fmac >> - brcm,bcm43456-fmac >> - brcm,bcm4354-fmac >> + - brcm,bcm4355c1-fmac >> - brcm,bcm4356-fmac >> - brcm,bcm4359-fmac >> + - brcm,bcm4364b2-fmac >> + - brcm,bcm4364b3-fmac >> + - brcm,bcm4377b3-fmac >> + - brcm,bcm4378b1-fmac >> + - brcm,bcm4387c2-fmac >> - cypress,cyw4373-fmac >> - cypress,cyw43012-fmac >> - const: brcm,bcm4329-fmac >> - const: brcm,bcm4329-fmac > > I suppose this helps with validation of device trees. However, nodes > for PCI devices are not supposed to have a "compatible" property as > the PCI vendor and device IDs are supposed to be used to identify a > device. > > That does raise the question how a schema for additional properties > for PCI device nodes is supposed to be defined... Apparently using a "pciVVVV,DDDD" compatible is one way, see bindings/net/wireless/qca,ath9k.yaml There's apparently exactly one example of this in in-tree devicetrees: boot/dts/rockchip/rk3399-gru-chromebook.dtsi I guess this is the way to go then, unless Rob has a different idea :)
diff --git a/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml b/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml index c11f23b20c4c..2530ff3e7b90 100644 --- a/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml +++ b/Documentation/devicetree/bindings/net/wireless/brcm,bcm4329-fmac.yaml @@ -4,7 +4,7 @@ $id: http://devicetree.org/schemas/net/wireless/brcm,bcm4329-fmac.yaml# $schema: http://devicetree.org/meta-schemas/core.yaml# -title: Broadcom BCM4329 family fullmac wireless SDIO devices +title: Broadcom BCM4329 family fullmac wireless SDIO/PCIE devices maintainers: - Arend van Spriel <arend@broadcom.com> @@ -36,16 +36,22 @@ properties: - brcm,bcm43455-fmac - brcm,bcm43456-fmac - brcm,bcm4354-fmac + - brcm,bcm4355c1-fmac - brcm,bcm4356-fmac - brcm,bcm4359-fmac + - brcm,bcm4364b2-fmac + - brcm,bcm4364b3-fmac + - brcm,bcm4377b3-fmac + - brcm,bcm4378b1-fmac + - brcm,bcm4387c2-fmac - cypress,cyw4373-fmac - cypress,cyw43012-fmac - const: brcm,bcm4329-fmac - const: brcm,bcm4329-fmac reg: - description: SDIO function number for the device, for most cases - this will be 1. + description: SDIO function number for the device (for most cases + this will be 1) or PCI device identifier. interrupts: maxItems: 1 @@ -75,6 +81,26 @@ properties: items: pattern: '^[A-Z][A-Z]-[A-Z][0-9A-Z]-[0-9]+$' + brcm,cal-blob: + $ref: /schemas/types.yaml#/definitions/uint8-array + description: A per-device calibration blob for the Wi-Fi radio. This + should be filled in by the bootloader from platform configuration + data, if necessary, and will be uploaded to the device if present. + + apple,module-instance: + $ref: /schemas/types.yaml#/definitions/string + description: Module codename used to identify a specific board on + Apple platforms. This is used to build the firmware filenames, to allow + different platforms to have different firmware and/or NVRAM config. + + apple,antenna-sku: + $def: /schemas/types.yaml#/definitions/string + description: Antenna SKU used to identify a specific antenna configuration + on Apple platforms. This is use to build firmware filenames, to allow + platforms with different antenna configs to have different firmware and/or + NVRAM. This would normally be filled in by the bootloader from platform + configuration data. + required: - compatible - reg
This binding is currently used for SDIO devices, but these chips are also used as PCIe devices on DT platforms and may be represented in the DT. Re-use the existing binding and add chip compatibles used by Apple T2 and M1 platforms (the T2 ones are not known to be used in DT platforms, but we might as well document them). Then, add properties required for firmware selection and calibration on M1 machines. Signed-off-by: Hector Martin <marcan@marcan.st> --- .../net/wireless/brcm,bcm4329-fmac.yaml | 32 +++++++++++++++++-- 1 file changed, 29 insertions(+), 3 deletions(-)