Re: [PATCH v2 20/35] brcmfmac: pcie: Perform correct BCM4364 firmware selection

[Arend van Spriel <arend.vanspriel@broadcom.com>]

[-- Attachment #1: Type: text/plain, Size: 4284 bytes --]

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.

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4219 bytes --]