Re: [PATCH v1 1/4] wifi: brcmfmac: Rename Cypress 89459 to BCM4355

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

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

On January 4, 2023 5:35:08 PM Hector Martin <marcan@marcan.st> wrote:

> On 04/01/2023 22.29, Arend van Spriel wrote:
>> On 1/4/2023 11:01 AM, 'Hector Martin' via BRCM80211-DEV-LIST,PDL wrote:
>>> The commit that introduced support for this chip incorrectly claimed it
>>> is a Cypress-specific part, while in actuality it is just a variant of
>>> BCM4355 silicon (as evidenced by the chip ID).
>>>
>>> The relationship between Cypress products and Broadcom products isn't
>>> entirely clear, but given what little information is available and prior
>>> art in the driver, it seems the convention should be that originally
>>> Broadcom parts should retain the Broadcom name.
>>>
>>> Thus, rename the relevant constants and firmware file. Also rename the
>>> specific 89459 PCIe ID to BCM43596, which seems to be the original
>>> subvariant name for this PCI ID (as defined in the out-of-tree bcmdhd
>>> driver). Also declare the firmware as CLM-capable, since it is.
>>>
>>> Fixes: dce45ded7619 ("brcmfmac: Support 89459 pcie")
>>> Signed-off-by: Hector Martin <marcan@marcan.st>
>>> ---
>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/chip.c   | 5 ++---
>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c   | 8 ++++----
>>> .../net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h | 6 +++---
>>> 3 files changed, 9 insertions(+), 10 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/chip.c 
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/chip.c
>>> index 121893bbaa1d..3e42c2bd0d9a 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/chip.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/chip.c
>>
>> [...]
>>
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c 
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
>>> index ae57a9a3ab05..3264be485e20 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c
>>
>> [...]
>>
>>> @@ -2590,6 +2590,7 @@ static const struct pci_device_id 
>>> brcmf_pcie_devid_table[] = {
>>> BRCMF_PCIE_DEVICE(BRCM_PCIE_4350_DEVICE_ID, WCC),
>>> BRCMF_PCIE_DEVICE_SUB(0x4355, BRCM_PCIE_VENDOR_ID_BROADCOM, 0x4355, WCC),
>>> BRCMF_PCIE_DEVICE(BRCM_PCIE_4354_RAW_DEVICE_ID, WCC),
>>> + BRCMF_PCIE_DEVICE(BRCM_PCIE_4355_RAW_DEVICE_ID, WCC),
>>
>> A bit of a problem here. If Cypress want to support this device,
>> regardless how they branded it, they will provide its firmware. Given
>> that they initially added it (as 89459) I suppose we should mark it with
>> CYW and not WCC. Actually, see my comment below on RAW dev ids.
>
> Right, I thought we might wind up with this issue. So then the question
> becomes: can we give responsibility over PCI ID 0x4415 to Cypress and
> mark just that one as CYW (if so it probably makes sense to keep that
> labeled CYW89459 instead of BCM43596), and if not, is there some other
> way to tell apart Cypress and Broadcom products we can use? I believe
> the Apple side firmware is developed by Broadcom, not Cypress.
>
> Note that even if we split by PCI device ID here, we still have a
> problem with firmware selection, since that means we're requesting the
> same firmware filename for both vendors (since that only tests the chip
> ID and revision ID). If Apple is the *only* Broadcom customer using
> these chips then we can get away with this, since I can just make sure
> the fancy Apple firmware selection will never collide with the vanilla
> firmware filename. But if other customers of both companies are both
> shipping the same chip with different and incompatible generic firmware,
> we need some way to tell them apart.

AFAIK Apple chips are exclusive. The vendor marking was added by recent 
patch series I worked on. So per device id we assign the vendor. If 
needed we can use subvendor or subdevid to separate them appropriately.

>
>
>>
>>> BRCMF_PCIE_DEVICE(BRCM_PCIE_4356_DEVICE_ID, WCC),
>>> BRCMF_PCIE_DEVICE(BRCM_PCIE_43567_DEVICE_ID, WCC),
>>> BRCMF_PCIE_DEVICE(BRCM_PCIE_43570_DEVICE_ID, WCC),
>>
>> [...]
>>
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h 
>>> b/drivers/net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h
>>> index f4939cf62767..cacc43db86eb 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h
>>
>> [...]
>>
>>> @@ -72,6 +72,7 @@
>>> #define BRCM_PCIE_4350_DEVICE_ID 0x43a3
>>> #define BRCM_PCIE_4354_DEVICE_ID 0x43df
>>> #define BRCM_PCIE_4354_RAW_DEVICE_ID 0x4354
>>> +#define BRCM_PCIE_4355_RAW_DEVICE_ID 0x4355
>>
>> I would remove all RAW device ids. These should not be observed outside
>> chip vendor walls.
>
> Ack, I'll remove this one instead of renaming it (or I can just drop all
> the existing RAW IDs first in one commit at the head of v2 if you prefer
> that).

Let's drop the existing RAW IDs with a separate patch explaining why ;-)

Regards,
Arend



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