diff mbox series

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

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

Commit Message

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

Comments

Arend van Spriel Jan. 10, 2022, 9:12 a.m. UTC | #1
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 Jan. 10, 2022, 11:20 a.m. UTC | #2
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?
Arend van Spriel Jan. 10, 2022, 12:02 p.m. UTC | #3
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.
diff mbox series

Patch

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");
 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 */
 	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),
@@ -2051,6 +2053,11 @@  static int brcmf_pcie_read_otp(struct brcmf_pciedev_info *devinfo)
 		base = 0x8c0;
 		words = 0xb2;
 		break;
+	case BRCM_CC_4364_CHIP_ID:
+		coreid = BCMA_CORE_CHIPCOMMON;
+		base = 0x8c0;
+		words = 0x1a0;
+		break;
 	case BRCM_CC_4377_CHIP_ID:
 	case BRCM_CC_4378_CHIP_ID:
 		coreid = BCMA_CORE_GCI;