Message-ID: <0efb9361-62da-f064-200a-eefe2d258b65@marcan.st>
Date:   Sun, 2 Jan 2022 23:12:42 +0900
From:   Hector Martin <marcan@...can.st>
To:     Mark Kettenis <mark.kettenis@...all.nl>
Cc:     robh@...nel.org, kvalo@...eaurora.org, davem@...emloft.net,
        kuba@...nel.org, rafael@...nel.org, lenb@...nel.org,
        aspriel@...il.com, franky.lin@...adcom.com,
        hante.meuleman@...adcom.com, chi-hsien.lin@...ineon.com,
        wright.feng@...ineon.com, sven@...npeter.dev, alyssa@...enzweig.io,
        kettenis@...nbsd.org, zajec5@...il.com,
        pieter-paul.giesberts@...adcom.com, linus.walleij@...aro.org,
        hdegoede@...hat.com, linville@...driver.com,
        sandals@...stytoothpaste.net, linux-wireless@...r.kernel.org,
        netdev@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-acpi@...r.kernel.org,
        brcm80211-dev-list.pdl@...adcom.com,
        SHA-cyfmac-dev-list@...ineon.com
Subject: Re: [PATCH 01/34] dt-bindings: net: bcm4329-fmac: Add Apple
 properties & chips

On 2021/12/30 1:38, Mark Kettenis wrote:
>> From: Hector Martin <marcan@...can.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.

-- 
Hector Martin (marcan@...can.st)
Public Key: https://mrcn.st/pub

Powered by blists - more mailing lists