Messages in this thread | | | Date | Wed, 4 Jan 2023 20:26:39 +0100 | From | Arend van Spriel <> | Subject | Re: [PATCH v1 1/4] wifi: brcmfmac: Rename Cypress 89459 to BCM4355 |
| |
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
[unhandled content-type:application/pkcs7-signature] | |