Message ID | 20240630073605.2164346-6-jacobe.zang@wesion.com (mailing list archive) |
---|---|
State | Changes Requested |
Delegated to: | Kalle Valo |
Headers | show |
Series | Add AP6275P wireless support | expand |
Am 30.06.24 um 09:36 schrieb Jacobe Zang: > This module features BCM43752A2 chipset. The firmware requires > randomness seeding, so enabled it. > > Co-developed-by: Ondrej Jirman <megi@xff.cz> > Signed-off-by: Ondrej Jirman <megi@xff.cz> > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com> > Link: https://megous.com/git/linux/commit/?h=ap6275p-6.10&id=1a99573bc8ed412e60e1969c0b29d53a0e5782e0 > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 5 ++++- > .../net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h | 2 ++ > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > index e84f562fc91b8..f427d664cf3a5 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > @@ -71,6 +71,7 @@ BRCMF_FW_CLM_DEF(4377B3, "brcmfmac4377b3-pcie"); > BRCMF_FW_CLM_DEF(4378B1, "brcmfmac4378b1-pcie"); > BRCMF_FW_CLM_DEF(4378B3, "brcmfmac4378b3-pcie"); > BRCMF_FW_CLM_DEF(4387C2, "brcmfmac4387c2-pcie"); > +BRCMF_FW_CLM_DEF(43752, "brcmfmac43752-pcie"); > > /* firmware config files */ > MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-pcie.txt"); > @@ -105,6 +106,7 @@ static const struct brcmf_firmware_mapping brcmf_pcie_fwnames[] = { > BRCMF_FW_ENTRY(BRCM_CC_43664_CHIP_ID, 0xFFFFFFF0, 4366C), > BRCMF_FW_ENTRY(BRCM_CC_43666_CHIP_ID, 0xFFFFFFF0, 4366C), > BRCMF_FW_ENTRY(BRCM_CC_4371_CHIP_ID, 0xFFFFFFFF, 4371), > + BRCMF_FW_ENTRY(BRCM_CC_43752_CHIP_ID, 0xFFFFFFFF, 43752), > BRCMF_FW_ENTRY(BRCM_CC_4377_CHIP_ID, 0xFFFFFFFF, 4377B3), /* revision ID 4 */ > BRCMF_FW_ENTRY(BRCM_CC_4378_CHIP_ID, 0x0000000F, 4378B1), /* revision ID 3 */ > BRCMF_FW_ENTRY(BRCM_CC_4378_CHIP_ID, 0xFFFFFFE0, 4378B3), /* revision ID 5 */ > @@ -1721,7 +1723,7 @@ static int brcmf_pcie_download_fw_nvram(struct brcmf_pciedev_info *devinfo, > memcpy_toio(devinfo->tcm + address, nvram, nvram_len); > brcmf_fw_nvram_free(nvram); > > - if (devinfo->otp.valid) { > + if (devinfo->otp.valid || devinfo->ci->chip == BRCM_CC_43752_CHIP_ID) { As a reviewer, i would expect an explanation in the commit message or in the code for this quirk. > size_t rand_len = BRCMF_RANDOM_SEED_LENGTH; > struct brcmf_random_seed_footer footer = { > .length = cpu_to_le32(rand_len), > @@ -2710,6 +2712,7 @@ static const struct pci_device_id brcmf_pcie_devid_table[] = { > BRCMF_PCIE_DEVICE(BRCM_PCIE_4366_5G_DEVICE_ID, BCA), > BRCMF_PCIE_DEVICE(BRCM_PCIE_4371_DEVICE_ID, WCC), > BRCMF_PCIE_DEVICE(BRCM_PCIE_43596_DEVICE_ID, CYW), > + BRCMF_PCIE_DEVICE(BRCM_PCIE_43752_DEVICE_ID, WCC), > BRCMF_PCIE_DEVICE(BRCM_PCIE_4377_DEVICE_ID, WCC), > BRCMF_PCIE_DEVICE(BRCM_PCIE_4378_DEVICE_ID, WCC), > BRCMF_PCIE_DEVICE(BRCM_PCIE_4387_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 44684bf1b9acc..c1e22c589d85e 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h > +++ b/drivers/net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h > @@ -52,6 +52,7 @@ > #define BRCM_CC_43664_CHIP_ID 43664 > #define BRCM_CC_43666_CHIP_ID 43666 > #define BRCM_CC_4371_CHIP_ID 0x4371 > +#define BRCM_CC_43752_CHIP_ID 43752 I think this should be converted to hexadecimal to be consistent. > #define BRCM_CC_4377_CHIP_ID 0x4377 > #define BRCM_CC_4378_CHIP_ID 0x4378 > #define BRCM_CC_4387_CHIP_ID 0x4387 > @@ -94,6 +95,7 @@ > #define BRCM_PCIE_4366_5G_DEVICE_ID 0x43c5 > #define BRCM_PCIE_4371_DEVICE_ID 0x440d > #define BRCM_PCIE_43596_DEVICE_ID 0x4415 > +#define BRCM_PCIE_43752_DEVICE_ID 0x449d > #define BRCM_PCIE_4377_DEVICE_ID 0x4488 > #define BRCM_PCIE_4378_DEVICE_ID 0x4425 > #define BRCM_PCIE_4387_DEVICE_ID 0x4433
On Sun, Jun 30, 2024 at 10:34:39AM GMT, Stefan Wahren wrote: > Am 30.06.24 um 09:36 schrieb Jacobe Zang: > > This module features BCM43752A2 chipset. The firmware requires > > randomness seeding, so enabled it. > > > > Co-developed-by: Ondrej Jirman <megi@xff.cz> > > Signed-off-by: Ondrej Jirman <megi@xff.cz> > > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com> > > Link: https://megous.com/git/linux/commit/?h=ap6275p-6.10&id=1a99573bc8ed412e60e1969c0b29d53a0e5782e0 > > --- > > drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 5 ++++- > > .../net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h | 2 ++ > > 2 files changed, 6 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > > index e84f562fc91b8..f427d664cf3a5 100644 > > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > > @@ -71,6 +71,7 @@ BRCMF_FW_CLM_DEF(4377B3, "brcmfmac4377b3-pcie"); > > BRCMF_FW_CLM_DEF(4378B1, "brcmfmac4378b1-pcie"); > > BRCMF_FW_CLM_DEF(4378B3, "brcmfmac4378b3-pcie"); > > BRCMF_FW_CLM_DEF(4387C2, "brcmfmac4387c2-pcie"); > > +BRCMF_FW_CLM_DEF(43752, "brcmfmac43752-pcie"); > > > > /* firmware config files */ > > MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-pcie.txt"); > > @@ -105,6 +106,7 @@ static const struct brcmf_firmware_mapping brcmf_pcie_fwnames[] = { > > BRCMF_FW_ENTRY(BRCM_CC_43664_CHIP_ID, 0xFFFFFFF0, 4366C), > > BRCMF_FW_ENTRY(BRCM_CC_43666_CHIP_ID, 0xFFFFFFF0, 4366C), > > BRCMF_FW_ENTRY(BRCM_CC_4371_CHIP_ID, 0xFFFFFFFF, 4371), > > + BRCMF_FW_ENTRY(BRCM_CC_43752_CHIP_ID, 0xFFFFFFFF, 43752), > > BRCMF_FW_ENTRY(BRCM_CC_4377_CHIP_ID, 0xFFFFFFFF, 4377B3), /* revision ID 4 */ > > BRCMF_FW_ENTRY(BRCM_CC_4378_CHIP_ID, 0x0000000F, 4378B1), /* revision ID 3 */ > > BRCMF_FW_ENTRY(BRCM_CC_4378_CHIP_ID, 0xFFFFFFE0, 4378B3), /* revision ID 5 */ > > @@ -1721,7 +1723,7 @@ static int brcmf_pcie_download_fw_nvram(struct brcmf_pciedev_info *devinfo, > > memcpy_toio(devinfo->tcm + address, nvram, nvram_len); > > brcmf_fw_nvram_free(nvram); > > > > - if (devinfo->otp.valid) { > > + if (devinfo->otp.valid || devinfo->ci->chip == BRCM_CC_43752_CHIP_ID) { > As a reviewer, i would expect an explanation in the commit message or in > the code for this quirk. That's the "The firmware requires randomness seeding, so enabled it." part of the commit message. > > size_t rand_len = BRCMF_RANDOM_SEED_LENGTH; > > struct brcmf_random_seed_footer footer = { > > .length = cpu_to_le32(rand_len), > > @@ -2710,6 +2712,7 @@ static const struct pci_device_id brcmf_pcie_devid_table[] = { > > BRCMF_PCIE_DEVICE(BRCM_PCIE_4366_5G_DEVICE_ID, BCA), > > BRCMF_PCIE_DEVICE(BRCM_PCIE_4371_DEVICE_ID, WCC), > > BRCMF_PCIE_DEVICE(BRCM_PCIE_43596_DEVICE_ID, CYW), > > + BRCMF_PCIE_DEVICE(BRCM_PCIE_43752_DEVICE_ID, WCC), > > BRCMF_PCIE_DEVICE(BRCM_PCIE_4377_DEVICE_ID, WCC), > > BRCMF_PCIE_DEVICE(BRCM_PCIE_4378_DEVICE_ID, WCC), > > BRCMF_PCIE_DEVICE(BRCM_PCIE_4387_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 44684bf1b9acc..c1e22c589d85e 100644 > > --- a/drivers/net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h > > +++ b/drivers/net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h > > @@ -52,6 +52,7 @@ > > #define BRCM_CC_43664_CHIP_ID 43664 > > #define BRCM_CC_43666_CHIP_ID 43666 > > #define BRCM_CC_4371_CHIP_ID 0x4371 > > +#define BRCM_CC_43752_CHIP_ID 43752 > I think this should be converted to hexadecimal to be consistent. > > #define BRCM_CC_4377_CHIP_ID 0x4377 > > #define BRCM_CC_4378_CHIP_ID 0x4378 > > #define BRCM_CC_4387_CHIP_ID 0x4387 > > @@ -94,6 +95,7 @@ > > #define BRCM_PCIE_4366_5G_DEVICE_ID 0x43c5 > > #define BRCM_PCIE_4371_DEVICE_ID 0x440d > > #define BRCM_PCIE_43596_DEVICE_ID 0x4415 > > +#define BRCM_PCIE_43752_DEVICE_ID 0x449d > > #define BRCM_PCIE_4377_DEVICE_ID 0x4488 > > #define BRCM_PCIE_4378_DEVICE_ID 0x4425 > > #define BRCM_PCIE_4387_DEVICE_ID 0x4433 >
Am 30.06.24 um 10:34 schrieb Stefan Wahren: > Am 30.06.24 um 09:36 schrieb Jacobe Zang: >> 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 44684bf1b9acc..c1e22c589d85e 100644 >> --- a/drivers/net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h >> +++ b/drivers/net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h >> @@ -52,6 +52,7 @@ >> #define BRCM_CC_43664_CHIP_ID 43664 >> #define BRCM_CC_43666_CHIP_ID 43666 >> #define BRCM_CC_4371_CHIP_ID 0x4371 >> +#define BRCM_CC_43752_CHIP_ID 43752 > I think this should be converted to hexadecimal to be consistent. Sorry, please ignore this comment >> #define BRCM_CC_4377_CHIP_ID 0x4377 >> #define BRCM_CC_4378_CHIP_ID 0x4378 >> #define BRCM_CC_4387_CHIP_ID 0x4387 >> @@ -94,6 +95,7 @@ >> #define BRCM_PCIE_4366_5G_DEVICE_ID 0x43c5 >> #define BRCM_PCIE_4371_DEVICE_ID 0x440d >> #define BRCM_PCIE_43596_DEVICE_ID 0x4415 >> +#define BRCM_PCIE_43752_DEVICE_ID 0x449d >> #define BRCM_PCIE_4377_DEVICE_ID 0x4488 >> #define BRCM_PCIE_4378_DEVICE_ID 0x4425 >> #define BRCM_PCIE_4387_DEVICE_ID 0x4433 >
On June 30, 2024 9:36:40 AM Jacobe Zang <jacobe.zang@wesion.com> wrote: > This module features BCM43752A2 chipset. The firmware requires > randomness seeding, so enabled it. > > Co-developed-by: Ondrej Jirman <megi@xff.cz> > Signed-off-by: Ondrej Jirman <megi@xff.cz> > Signed-off-by: Jacobe Zang <jacobe.zang@wesion.com> > Link: > https://megous.com/git/linux/commit/?h=ap6275p-6.10&id=1a99573bc8ed412e60e1969c0b29d53a0e5782e0 > --- > drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c | 5 ++++- > .../net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h | 2 ++ > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > index e84f562fc91b8..f427d664cf3a5 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c [...] > @@ -1721,7 +1723,7 @@ static int brcmf_pcie_download_fw_nvram(struct > brcmf_pciedev_info *devinfo, > memcpy_toio(devinfo->tcm + address, nvram, nvram_len); > brcmf_fw_nvram_free(nvram); > > - if (devinfo->otp.valid) { > + if (devinfo->otp.valid || devinfo->ci->chip So the problem here is that otp validity is not unambiguous. The seed requirement is mandated by firmware and was introduced for new WCC chips moving forward. I would suggest to have a need_seed flag in the driver_data of the device table (below). It currently is used only for vendor identification. Regards, Arend > == BRCM_CC_43752_CHIP_ID) { > size_t rand_len = BRCMF_RANDOM_SEED_LENGTH; > struct brcmf_random_seed_footer footer = { > .length = cpu_to_le32(rand_len), > @@ -2710,6 +2712,7 @@ static const struct pci_device_id > brcmf_pcie_devid_table[] = { > BRCMF_PCIE_DEVICE(BRCM_PCIE_4366_5G_DEVICE_ID, BCA), > BRCMF_PCIE_DEVICE(BRCM_PCIE_4371_DEVICE_ID, WCC), > BRCMF_PCIE_DEVICE(BRCM_PCIE_43596_DEVICE_ID, CYW), > + BRCMF_PCIE_DEVICE(BRCM_PCIE_43752_DEVICE_ID, WCC), > BRCMF_PCIE_DEVICE(BRCM_PCIE_4377_DEVICE_ID, WCC), > BRCMF_PCIE_DEVICE(BRCM_PCIE_4378_DEVICE_ID, WCC), > BRCMF_PCIE_DEVICE(BRCM_PCIE_4387_DEVICE_ID, WCC),
>> @@ -1721,7 +1723,7 @@ static int brcmf_pcie_download_fw_nvram(struct >> brcmf_pciedev_info *devinfo, >> memcpy_toio(devinfo->tcm + address, nvram, nvram_len); >> brcmf_fw_nvram_free(nvram); >> >> - if (devinfo->otp.valid) { >> + if (devinfo->otp.valid || devinfo->ci->chip > > So the problem here is that otp validity is not unambiguous. The seed > requirement is mandated by firmware and was introduced for new WCC chips > moving forward. > > I would suggest to have a need_seed flag in the driver_data of the device > table (below). It currently is used only for vendor identification. Because of brcmf_pcie_setup function also need the driver_data, so I tried to define a global variable named NEED_SEED_FLAG. But marco define didn't allow to modify value in it, do you think where is the best place to assign the flag? Or you still think should add a new macro named BRCMF_PCIE_DEVICE_NEED_SEED? To be honest, I'm not quite sure which way is the best. > Regards, > Arend > >> == BRCM_CC_43752_CHIP_ID) { >> size_t rand_len = BRCMF_RANDOM_SEED_LENGTH; >> struct brcmf_random_seed_footer footer = { >> .length = cpu_to_le32(rand_len), >> @@ -2710,6 +2712,7 @@ static const struct pci_device_id >> brcmf_pcie_devid_table[] = { >> BRCMF_PCIE_DEVICE(BRCM_PCIE_4366_5G_DEVICE_ID, BCA), >> BRCMF_PCIE_DEVICE(BRCM_PCIE_4371_DEVICE_ID, WCC), >> BRCMF_PCIE_DEVICE(BRCM_PCIE_43596_DEVICE_ID, CYW), >> + BRCMF_PCIE_DEVICE(BRCM_PCIE_43752_DEVICE_ID, WCC), >> BRCMF_PCIE_DEVICE(BRCM_PCIE_4377_DEVICE_ID, WCC), >> BRCMF_PCIE_DEVICE(BRCM_PCIE_4378_DEVICE_ID, WCC), >> BRCMF_PCIE_DEVICE(BRCM_PCIE_4387_DEVICE_ID, WCC), --- Best Regards Jacobe
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c index e84f562fc91b8..f427d664cf3a5 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/pcie.c @@ -71,6 +71,7 @@ BRCMF_FW_CLM_DEF(4377B3, "brcmfmac4377b3-pcie"); BRCMF_FW_CLM_DEF(4378B1, "brcmfmac4378b1-pcie"); BRCMF_FW_CLM_DEF(4378B3, "brcmfmac4378b3-pcie"); BRCMF_FW_CLM_DEF(4387C2, "brcmfmac4387c2-pcie"); +BRCMF_FW_CLM_DEF(43752, "brcmfmac43752-pcie"); /* firmware config files */ MODULE_FIRMWARE(BRCMF_FW_DEFAULT_PATH "brcmfmac*-pcie.txt"); @@ -105,6 +106,7 @@ static const struct brcmf_firmware_mapping brcmf_pcie_fwnames[] = { BRCMF_FW_ENTRY(BRCM_CC_43664_CHIP_ID, 0xFFFFFFF0, 4366C), BRCMF_FW_ENTRY(BRCM_CC_43666_CHIP_ID, 0xFFFFFFF0, 4366C), BRCMF_FW_ENTRY(BRCM_CC_4371_CHIP_ID, 0xFFFFFFFF, 4371), + BRCMF_FW_ENTRY(BRCM_CC_43752_CHIP_ID, 0xFFFFFFFF, 43752), BRCMF_FW_ENTRY(BRCM_CC_4377_CHIP_ID, 0xFFFFFFFF, 4377B3), /* revision ID 4 */ BRCMF_FW_ENTRY(BRCM_CC_4378_CHIP_ID, 0x0000000F, 4378B1), /* revision ID 3 */ BRCMF_FW_ENTRY(BRCM_CC_4378_CHIP_ID, 0xFFFFFFE0, 4378B3), /* revision ID 5 */ @@ -1721,7 +1723,7 @@ static int brcmf_pcie_download_fw_nvram(struct brcmf_pciedev_info *devinfo, memcpy_toio(devinfo->tcm + address, nvram, nvram_len); brcmf_fw_nvram_free(nvram); - if (devinfo->otp.valid) { + if (devinfo->otp.valid || devinfo->ci->chip == BRCM_CC_43752_CHIP_ID) { size_t rand_len = BRCMF_RANDOM_SEED_LENGTH; struct brcmf_random_seed_footer footer = { .length = cpu_to_le32(rand_len), @@ -2710,6 +2712,7 @@ static const struct pci_device_id brcmf_pcie_devid_table[] = { BRCMF_PCIE_DEVICE(BRCM_PCIE_4366_5G_DEVICE_ID, BCA), BRCMF_PCIE_DEVICE(BRCM_PCIE_4371_DEVICE_ID, WCC), BRCMF_PCIE_DEVICE(BRCM_PCIE_43596_DEVICE_ID, CYW), + BRCMF_PCIE_DEVICE(BRCM_PCIE_43752_DEVICE_ID, WCC), BRCMF_PCIE_DEVICE(BRCM_PCIE_4377_DEVICE_ID, WCC), BRCMF_PCIE_DEVICE(BRCM_PCIE_4378_DEVICE_ID, WCC), BRCMF_PCIE_DEVICE(BRCM_PCIE_4387_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 44684bf1b9acc..c1e22c589d85e 100644 --- a/drivers/net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h +++ b/drivers/net/wireless/broadcom/brcm80211/include/brcm_hw_ids.h @@ -52,6 +52,7 @@ #define BRCM_CC_43664_CHIP_ID 43664 #define BRCM_CC_43666_CHIP_ID 43666 #define BRCM_CC_4371_CHIP_ID 0x4371 +#define BRCM_CC_43752_CHIP_ID 43752 #define BRCM_CC_4377_CHIP_ID 0x4377 #define BRCM_CC_4378_CHIP_ID 0x4378 #define BRCM_CC_4387_CHIP_ID 0x4387 @@ -94,6 +95,7 @@ #define BRCM_PCIE_4366_5G_DEVICE_ID 0x43c5 #define BRCM_PCIE_4371_DEVICE_ID 0x440d #define BRCM_PCIE_43596_DEVICE_ID 0x4415 +#define BRCM_PCIE_43752_DEVICE_ID 0x449d #define BRCM_PCIE_4377_DEVICE_ID 0x4488 #define BRCM_PCIE_4378_DEVICE_ID 0x4425 #define BRCM_PCIE_4387_DEVICE_ID 0x4433