| Message ID | 20210425110200.3050-1-shawn.guo@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | brcmfmac: use ISO3166 country code and 0 rev as fallback | expand |
On 4/25/2021 1:02 PM, Shawn Guo wrote: > Instead of aborting country code setup in firmware, use ISO3166 country > code and 0 rev as fallback, when country_codes mapping table is not > configured. This fallback saves the country_codes table setup for recent > brcmfmac chipsets/firmwares, which just use ISO3166 code and require no > revision number. I am somewhat surprised, but with the brcm-spinoffs (cypress/infineon and synaptics) my understanding may have been surpassed by reality. Would you happen to know which chipsets/firmwares require only ISO3166 code and no rev? Regards, Arend > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > --- > .../broadcom/brcm80211/brcmfmac/cfg80211.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > index f4405d7861b6..6cb09c7c37b6 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > @@ -7442,18 +7442,23 @@ static s32 brcmf_translate_country_code(struct brcmf_pub *drvr, char alpha2[2], > s32 found_index; > int i; > > - country_codes = drvr->settings->country_codes; > - if (!country_codes) { > - brcmf_dbg(TRACE, "No country codes configured for device\n"); > - return -EINVAL; > - } > - > if ((alpha2[0] == ccreq->country_abbrev[0]) && > (alpha2[1] == ccreq->country_abbrev[1])) { > brcmf_dbg(TRACE, "Country code already set\n"); > return -EAGAIN; > } > > + country_codes = drvr->settings->country_codes; > + if (!country_codes) { > + brcmf_dbg(TRACE, "No country codes configured for device, using ISO3166 code and 0 rev\n"); > + memset(ccreq, 0, sizeof(*ccreq)); > + ccreq->country_abbrev[0] = alpha2[0]; > + ccreq->country_abbrev[1] = alpha2[1]; > + ccreq->ccode[0] = alpha2[0]; > + ccreq->ccode[1] = alpha2[1]; > + return 0; > + } > + > found_index = -1; > for (i = 0; i < country_codes->table_size; i++) { > cc = &country_codes->table[i]; >
On Wed, Apr 28, 2021 at 02:03:07PM +0200, Arend van Spriel wrote: > On 4/25/2021 1:02 PM, Shawn Guo wrote: > > Instead of aborting country code setup in firmware, use ISO3166 country > > code and 0 rev as fallback, when country_codes mapping table is not > > configured. This fallback saves the country_codes table setup for recent > > brcmfmac chipsets/firmwares, which just use ISO3166 code and require no > > revision number. > > I am somewhat surprised, but with the brcm-spinoffs (cypress/infineon and > synaptics) my understanding may have been surpassed by reality. Would you > happen to know which chipsets/firmwares require only ISO3166 code and no > rev? The "no rev" here actually means 'rev' field being zero. The chipset I'm running is a BCM43012 from Synaptics, I think. Firmware: BCM43012/2 wl0: Apr 16 2021 15:25:36 version 18.35.389.63.t2 (wlan=r836194) FWID 01-a8c7bac Shawn
On Wed, Apr 28, 2021 at 08:42:29PM +0800, Shawn Guo wrote: > On Wed, Apr 28, 2021 at 02:03:07PM +0200, Arend van Spriel wrote: > > On 4/25/2021 1:02 PM, Shawn Guo wrote: > > > Instead of aborting country code setup in firmware, use ISO3166 country > > > code and 0 rev as fallback, when country_codes mapping table is not > > > configured. This fallback saves the country_codes table setup for recent > > > brcmfmac chipsets/firmwares, which just use ISO3166 code and require no > > > revision number. > > > > I am somewhat surprised, but with the brcm-spinoffs (cypress/infineon and > > synaptics) my understanding may have been surpassed by reality. Would you > > happen to know which chipsets/firmwares require only ISO3166 code and no > > rev? > > The "no rev" here actually means 'rev' field being zero. The chipset > I'm running is a BCM43012 from Synaptics, I think. > > Firmware: BCM43012/2 wl0: Apr 16 2021 15:25:36 version 18.35.389.63.t2 (wlan=r836194) FWID 01-a8c7bac Arend, Does it make sense? Or is there actually a country code mapping table I'm not aware of? Shawn
On May 23, 2021 8:12:29 AM Shawn Guo <shawn.guo@linaro.org> wrote: > On Wed, Apr 28, 2021 at 08:42:29PM +0800, Shawn Guo wrote: >> On Wed, Apr 28, 2021 at 02:03:07PM +0200, Arend van Spriel wrote: >>> On 4/25/2021 1:02 PM, Shawn Guo wrote: >>>> Instead of aborting country code setup in firmware, use ISO3166 country >>>> code and 0 rev as fallback, when country_codes mapping table is not >>>> configured. This fallback saves the country_codes table setup for recent >>>> brcmfmac chipsets/firmwares, which just use ISO3166 code and require no >>>> revision number. >>> >>> I am somewhat surprised, but with the brcm-spinoffs (cypress/infineon and >>> synaptics) my understanding may have been surpassed by reality. Would you >>> happen to know which chipsets/firmwares require only ISO3166 code and no >>> rev? >> >> The "no rev" here actually means 'rev' field being zero. The chipset >> I'm running is a BCM43012 from Synaptics, I think. >> >> Firmware: BCM43012/2 wl0: Apr 16 2021 15:25:36 version 18.35.389.63.t2 >> (wlan=r836194) FWID 01-a8c7bac > > Arend, > > Does it make sense? Or is there actually a country code mapping table > I'm not aware of? I recall the firmware always include a rev 0 for each country code. I will have to ask internally whether that may be used for any chipset. If so, it seems reasonable to use rev 0 as fallback when no mapping table is provided. Regards, Arend -- This electronic communication and the information and any files transmitted with it, or attached to it, are confidential and are intended solely for the use of the individual or entity to whom it is addressed and may contain information that is confidential, legally privileged, protected by privacy laws, or otherwise restricted from disclosure to anyone else. If you are not the intended recipient or the person responsible for delivering the e-mail to the intended recipient, you are hereby notified that any use, copying, distributing, dissemination, forwarding, printing, or copying of this e-mail is strictly prohibited. If you received this e-mail in error, please return the e-mail to the sender, delete it from your computer, and destroy any printed copy of it.
Shawn Guo <shawn.guo@linaro.org> wrote: > Instead of aborting country code setup in firmware, use ISO3166 country > code and 0 rev as fallback, when country_codes mapping table is not > configured. This fallback saves the country_codes table setup for recent > brcmfmac chipsets/firmwares, which just use ISO3166 code and require no > revision number. > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> Patch applied to wireless-drivers-next.git, thanks. b0b524f079a2 brcmfmac: use ISO3166 country code and 0 rev as fallback -- https://patchwork.kernel.org/project/linux-wireless/patch/20210425110200.3050-1-shawn.guo@linaro.org/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On 25.04.21 13:02, Shawn Guo wrote: > Instead of aborting country code setup in firmware, use ISO3166 country > code and 0 rev as fallback, when country_codes mapping table is not > configured. This fallback saves the country_codes table setup for recent > brcmfmac chipsets/firmwares, which just use ISO3166 code and require no > revision number. This patch breaks wireless support on RockPro64. At least the access point is not usable, station mode not tested. brcmfmac: brcmf_c_preinit_dcmds: Firmware: BCM4359/9 wl0: Mar 6 2017 10:16:06 version 9.87.51.7 (r686312) FWID 01-4dcc75d9 Reverting this patch makes the access point show up again with linux-5.14 . Regards, Soeren > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > --- > .../broadcom/brcm80211/brcmfmac/cfg80211.c | 17 +++++++++++------ > 1 file changed, 11 insertions(+), 6 deletions(-) > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > index f4405d7861b6..6cb09c7c37b6 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > @@ -7442,18 +7442,23 @@ static s32 brcmf_translate_country_code(struct brcmf_pub *drvr, char alpha2[2], > s32 found_index; > int i; > > - country_codes = drvr->settings->country_codes; > - if (!country_codes) { > - brcmf_dbg(TRACE, "No country codes configured for device\n"); > - return -EINVAL; > - } > - > if ((alpha2[0] == ccreq->country_abbrev[0]) && > (alpha2[1] == ccreq->country_abbrev[1])) { > brcmf_dbg(TRACE, "Country code already set\n"); > return -EAGAIN; > } > > + country_codes = drvr->settings->country_codes; > + if (!country_codes) { > + brcmf_dbg(TRACE, "No country codes configured for device, using ISO3166 code and 0 rev\n"); > + memset(ccreq, 0, sizeof(*ccreq)); > + ccreq->country_abbrev[0] = alpha2[0]; > + ccreq->country_abbrev[1] = alpha2[1]; > + ccreq->ccode[0] = alpha2[0]; > + ccreq->ccode[1] = alpha2[1]; > + return 0; > + } > + > found_index = -1; > for (i = 0; i < country_codes->table_size; i++) { > cc = &country_codes->table[i];
Hi Soeren, On Tue, Sep 07, 2021 at 09:22:52PM +0200, Soeren Moch wrote: > On 25.04.21 13:02, Shawn Guo wrote: > > Instead of aborting country code setup in firmware, use ISO3166 country > > code and 0 rev as fallback, when country_codes mapping table is not > > configured. This fallback saves the country_codes table setup for recent > > brcmfmac chipsets/firmwares, which just use ISO3166 code and require no > > revision number. > This patch breaks wireless support on RockPro64. At least the access > point is not usable, station mode not tested. > > brcmfmac: brcmf_c_preinit_dcmds: Firmware: BCM4359/9 wl0: Mar 6 2017 > 10:16:06 version 9.87.51.7 (r686312) FWID 01-4dcc75d9 > > Reverting this patch makes the access point show up again with linux-5.14 . Sorry for breaking your device! So it sounds like you do not have country_codes configured for your BCM4359/9 device, while it needs particular `rev` setup for the ccode you are testing with. It was "working" likely because you have a static `ccode` and `regrev` setting in nvram file. But roaming to a different region will mostly get you a broken WiFi support. Is it possible to set up the country_codes for your device to get it work properly? Shawn > > Regards, > Soeren > > Signed-off-by: Shawn Guo <shawn.guo@linaro.org> > > --- > > .../broadcom/brcm80211/brcmfmac/cfg80211.c | 17 +++++++++++------ > > 1 file changed, 11 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > > index f4405d7861b6..6cb09c7c37b6 100644 > > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c > > @@ -7442,18 +7442,23 @@ static s32 brcmf_translate_country_code(struct brcmf_pub *drvr, char alpha2[2], > > s32 found_index; > > int i; > > > > - country_codes = drvr->settings->country_codes; > > - if (!country_codes) { > > - brcmf_dbg(TRACE, "No country codes configured for device\n"); > > - return -EINVAL; > > - } > > - > > if ((alpha2[0] == ccreq->country_abbrev[0]) && > > (alpha2[1] == ccreq->country_abbrev[1])) { > > brcmf_dbg(TRACE, "Country code already set\n"); > > return -EAGAIN; > > } > > > > + country_codes = drvr->settings->country_codes; > > + if (!country_codes) { > > + brcmf_dbg(TRACE, "No country codes configured for device, using ISO3166 code and 0 rev\n"); > > + memset(ccreq, 0, sizeof(*ccreq)); > > + ccreq->country_abbrev[0] = alpha2[0]; > > + ccreq->country_abbrev[1] = alpha2[1]; > > + ccreq->ccode[0] = alpha2[0]; > > + ccreq->ccode[1] = alpha2[1]; > > + return 0; > > + } > > + > > found_index = -1; > > for (i = 0; i < country_codes->table_size; i++) { > > cc = &country_codes->table[i]; >
Hi Shawn, On 08.09.21 03:00, Shawn Guo wrote: > Hi Soeren, > > On Tue, Sep 07, 2021 at 09:22:52PM +0200, Soeren Moch wrote: >> On 25.04.21 13:02, Shawn Guo wrote: >>> Instead of aborting country code setup in firmware, use ISO3166 country >>> code and 0 rev as fallback, when country_codes mapping table is not >>> configured. This fallback saves the country_codes table setup for recent >>> brcmfmac chipsets/firmwares, which just use ISO3166 code and require no >>> revision number. >> This patch breaks wireless support on RockPro64. At least the access >> point is not usable, station mode not tested. >> >> brcmfmac: brcmf_c_preinit_dcmds: Firmware: BCM4359/9 wl0: Mar 6 2017 >> 10:16:06 version 9.87.51.7 (r686312) FWID 01-4dcc75d9 >> >> Reverting this patch makes the access point show up again with linux-5.14 . > Sorry for breaking your device! > > So it sounds like you do not have country_codes configured for your > BCM4359/9 device, while it needs particular `rev` setup for the ccode > you are testing with. It was "working" likely because you have a static > `ccode` and `regrev` setting in nvram file. It always has been a mystery to me how country codes are configured for this device. Before I read your patch I did not even know that a translation table is required. Is there some documentation how this is supposed to work? Not sure if this makes a difference, BCM4359/9 is a Cypress device I think, I added mainline support for it some time ago. I have installed different firmware files, brcmfmac4359-sdio.clm_blob, brcmfmac4359-sdio.bin, brcmfmac4359-sdio.txt, the latter also linked as brcmfmac4359-sdio.pine64,rockpro64-2.1.txt. This probably is the nvram file. ccode and regrev are set to zero, which probably means 'international save settings". > But roaming to a different > region will mostly get you a broken WiFi support. Is it possible to set > up the country_codes for your device to get it work properly? In linux-5.13 it worked, probably with save settings (not all channels selectable, limited tx power), with linux-5.14 it stopped working, so it is a regression. I personally would like to learn how all this is configured properly. For general use I think save settings are better than no wifi at all with this patch. This fallback to ISO CC seams to work with newer (Synaptics?) devices only. Soeren > > Shawn > >> Regards, >> Soeren >>> Signed-off-by: Shawn Guo <shawn.guo@linaro.org> >>> --- >>> .../broadcom/brcm80211/brcmfmac/cfg80211.c | 17 +++++++++++------ >>> 1 file changed, 11 insertions(+), 6 deletions(-) >>> >>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c >>> index f4405d7861b6..6cb09c7c37b6 100644 >>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c >>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c >>> @@ -7442,18 +7442,23 @@ static s32 brcmf_translate_country_code(struct brcmf_pub *drvr, char alpha2[2], >>> s32 found_index; >>> int i; >>> >>> - country_codes = drvr->settings->country_codes; >>> - if (!country_codes) { >>> - brcmf_dbg(TRACE, "No country codes configured for device\n"); >>> - return -EINVAL; >>> - } >>> - >>> if ((alpha2[0] == ccreq->country_abbrev[0]) && >>> (alpha2[1] == ccreq->country_abbrev[1])) { >>> brcmf_dbg(TRACE, "Country code already set\n"); >>> return -EAGAIN; >>> } >>> >>> + country_codes = drvr->settings->country_codes; >>> + if (!country_codes) { >>> + brcmf_dbg(TRACE, "No country codes configured for device, using ISO3166 code and 0 rev\n"); >>> + memset(ccreq, 0, sizeof(*ccreq)); >>> + ccreq->country_abbrev[0] = alpha2[0]; >>> + ccreq->country_abbrev[1] = alpha2[1]; >>> + ccreq->ccode[0] = alpha2[0]; >>> + ccreq->ccode[1] = alpha2[1]; >>> + return 0; >>> + } >>> + >>> found_index = -1; >>> for (i = 0; i < country_codes->table_size; i++) { >>> cc = &country_codes->table[i];
On Wed, Sep 08, 2021 at 07:08:06AM +0200, Soeren Moch wrote: > Hi Shawn, > > On 08.09.21 03:00, Shawn Guo wrote: > > Hi Soeren, > > > > On Tue, Sep 07, 2021 at 09:22:52PM +0200, Soeren Moch wrote: > >> On 25.04.21 13:02, Shawn Guo wrote: > >>> Instead of aborting country code setup in firmware, use ISO3166 country > >>> code and 0 rev as fallback, when country_codes mapping table is not > >>> configured. This fallback saves the country_codes table setup for recent > >>> brcmfmac chipsets/firmwares, which just use ISO3166 code and require no > >>> revision number. > >> This patch breaks wireless support on RockPro64. At least the access > >> point is not usable, station mode not tested. > >> > >> brcmfmac: brcmf_c_preinit_dcmds: Firmware: BCM4359/9 wl0: Mar 6 2017 > >> 10:16:06 version 9.87.51.7 (r686312) FWID 01-4dcc75d9 > >> > >> Reverting this patch makes the access point show up again with linux-5.14 . > > Sorry for breaking your device! > > > > So it sounds like you do not have country_codes configured for your > > BCM4359/9 device, while it needs particular `rev` setup for the ccode > > you are testing with. It was "working" likely because you have a static > > `ccode` and `regrev` setting in nvram file. > It always has been a mystery to me how country codes are configured for > this device. Before I read your patch I did not even know that a > translation table is required. Is there some documentation how this is > supposed to work? Not sure if this makes a difference, BCM4359/9 is a > Cypress device I think, I added mainline support for it some time ago. One way to add the translation table is using DT. You can find more info and example in following commits: b41936227078 ("dt-bindings: bcm4329-fmac: add optional brcm,ccode-map") 1a3ac5c651a0 ("brcmfmac: support parse country code map from DT") > > I have installed different firmware files, brcmfmac4359-sdio.clm_blob, > brcmfmac4359-sdio.bin, brcmfmac4359-sdio.txt, the latter also linked as > brcmfmac4359-sdio.pine64,rockpro64-2.1.txt. This probably is the nvram > file. ccode and regrev are set to zero, which probably means > 'international save settings". I'm not sure how this 'international save settings' works for brcmfmac devices. Do you have more info or any pointers? > > But roaming to a different > > region will mostly get you a broken WiFi support. Is it possible to set > > up the country_codes for your device to get it work properly? > In linux-5.13 it worked, probably with save settings (not all channels > selectable, limited tx power), with linux-5.14 it stopped working, so it > is a regression. > I personally would like to learn how all this is configured properly. > For general use I think save settings are better than no wifi at all > with this patch. This fallback to ISO CC seams to work with newer > (Synaptics?) devices only. I do not mind you send a reverting if you have problem to add a proper translation table for your device. But that would mean I have to add a pretty "meaningless" translation table for my devices :( Shawn
Hi Shawn, On 09.09.21 04:20, Shawn Guo wrote: > On Wed, Sep 08, 2021 at 07:08:06AM +0200, Soeren Moch wrote: >> Hi Shawn, >> >> On 08.09.21 03:00, Shawn Guo wrote: >>> Hi Soeren, >>> >>> On Tue, Sep 07, 2021 at 09:22:52PM +0200, Soeren Moch wrote: >>>> On 25.04.21 13:02, Shawn Guo wrote: >>>>> Instead of aborting country code setup in firmware, use ISO3166 country >>>>> code and 0 rev as fallback, when country_codes mapping table is not >>>>> configured. This fallback saves the country_codes table setup for recent >>>>> brcmfmac chipsets/firmwares, which just use ISO3166 code and require no >>>>> revision number. >>>> This patch breaks wireless support on RockPro64. At least the access >>>> point is not usable, station mode not tested. >>>> >>>> brcmfmac: brcmf_c_preinit_dcmds: Firmware: BCM4359/9 wl0: Mar 6 2017 >>>> 10:16:06 version 9.87.51.7 (r686312) FWID 01-4dcc75d9 >>>> >>>> Reverting this patch makes the access point show up again with linux-5.14 . >>> Sorry for breaking your device! >>> >>> So it sounds like you do not have country_codes configured for your >>> BCM4359/9 device, while it needs particular `rev` setup for the ccode >>> you are testing with. It was "working" likely because you have a static >>> `ccode` and `regrev` setting in nvram file. >> It always has been a mystery to me how country codes are configured for >> this device. Before I read your patch I did not even know that a >> translation table is required. Is there some documentation how this is >> supposed to work? Not sure if this makes a difference, BCM4359/9 is a >> Cypress device I think, I added mainline support for it some time ago. > One way to add the translation table is using DT. You can find more > info and example in following commits: > > b41936227078 ("dt-bindings: bcm4329-fmac: add optional brcm,ccode-map") > 1a3ac5c651a0 ("brcmfmac: support parse country code map from DT") OK, thanks. When one way is to use DT, what is the 'traditional way' to add such table? And maybe the more interesting question, where can these settings be obtained from? The tweaked device specific settings probably from the device vendor, good luck! But the general country specific settings, as you are obviously interested in with your trivial mapping, shouldn't they go into driver directly? Only to be overruled when device specific settings are available via DT? And of course only for device/firmware combinations that support this general mapping, so that other devices with 'unknown mapping' are not broken by this enhancement? >> I have installed different firmware files, brcmfmac4359-sdio.clm_blob, >> brcmfmac4359-sdio.bin, brcmfmac4359-sdio.txt, the latter also linked as >> brcmfmac4359-sdio.pine64,rockpro64-2.1.txt. This probably is the nvram >> file. ccode and regrev are set to zero, which probably means >> 'international save settings". > I'm not sure how this 'international save settings' works for brcmfmac > devices. Do you have more info or any pointers? The correct term in this context probably is 'world regulatory domain', the most restrictive wifi settings that can be used all over the world. This usually is taken as default by cfg80211, apparently also for (some?) brcmfmac devices/firmwares. These 'world' settings can be replaced by more permissive country specific regulatory domain settings, but for brcmfmac devices this seems to be firmware specific and requires this country mapping. I have seen a country code "00" for the world regulatory domain in the past, not sure if this is standard or a device/driver/software specific hack and if this can be used for brcmfmac (mapping from string "00" to country_code=0 ?). For sure here are more experienced wifi developers who know better. >>> But roaming to a different >>> region will mostly get you a broken WiFi support. Is it possible to set >>> up the country_codes for your device to get it work properly? >> In linux-5.13 it worked, probably with save settings (not all channels >> selectable, limited tx power), with linux-5.14 it stopped working, so it >> is a regression. >> I personally would like to learn how all this is configured properly. >> For general use I think save settings are better than no wifi at all >> with this patch. This fallback to ISO CC seams to work with newer >> (Synaptics?) devices only. > I do not mind you send a reverting if you have problem to add a proper > translation table for your device. But that would mean I have to add > a pretty "meaningless" translation table for my devices :( > Is this not the usual DT policy, that missing optional properties should not prevent a device to work, that old dtbs should still work when new properties are added? I'm not sure what's the best way forward. A plain revert of this patch would at least bring back wifi support for RockPro64 devices with existing dtbs. Maybe someone else has a better proposal how to proceed. Regards, Soeren
On Thu, Sep 09, 2021 at 10:39:58AM +0200, Soeren Moch wrote: > Hi Shawn, > > On 09.09.21 04:20, Shawn Guo wrote: > > On Wed, Sep 08, 2021 at 07:08:06AM +0200, Soeren Moch wrote: > >> Hi Shawn, > >> > >> On 08.09.21 03:00, Shawn Guo wrote: > >>> Hi Soeren, > >>> > >>> On Tue, Sep 07, 2021 at 09:22:52PM +0200, Soeren Moch wrote: > >>>> On 25.04.21 13:02, Shawn Guo wrote: > >>>>> Instead of aborting country code setup in firmware, use ISO3166 country > >>>>> code and 0 rev as fallback, when country_codes mapping table is not > >>>>> configured. This fallback saves the country_codes table setup for recent > >>>>> brcmfmac chipsets/firmwares, which just use ISO3166 code and require no > >>>>> revision number. > >>>> This patch breaks wireless support on RockPro64. At least the access > >>>> point is not usable, station mode not tested. > >>>> > >>>> brcmfmac: brcmf_c_preinit_dcmds: Firmware: BCM4359/9 wl0: Mar 6 2017 > >>>> 10:16:06 version 9.87.51.7 (r686312) FWID 01-4dcc75d9 > >>>> > >>>> Reverting this patch makes the access point show up again with linux-5.14 . > >>> Sorry for breaking your device! > >>> > >>> So it sounds like you do not have country_codes configured for your > >>> BCM4359/9 device, while it needs particular `rev` setup for the ccode > >>> you are testing with. It was "working" likely because you have a static > >>> `ccode` and `regrev` setting in nvram file. > >> It always has been a mystery to me how country codes are configured for > >> this device. Before I read your patch I did not even know that a > >> translation table is required. Is there some documentation how this is > >> supposed to work? Not sure if this makes a difference, BCM4359/9 is a > >> Cypress device I think, I added mainline support for it some time ago. > > One way to add the translation table is using DT. You can find more > > info and example in following commits: > > > > b41936227078 ("dt-bindings: bcm4329-fmac: add optional brcm,ccode-map") > > 1a3ac5c651a0 ("brcmfmac: support parse country code map from DT") > OK, thanks. > When one way is to use DT, what is the 'traditional way' to add such table? To be honest, I don't know what the 'traditional way' is, because I haven't seen how `country_codes` is used before I add DT support of it. > And maybe the more interesting question, where can these settings be > obtained from? The tweaked device specific settings probably from the > device vendor, good luck! > But the general country specific settings, as you are obviously > interested in with your trivial mapping, shouldn't they go into driver > directly? Only to be overruled when device specific settings are > available via DT? And of course only for device/firmware combinations > that support this general mapping, so that other devices with 'unknown > mapping' are not broken by this enhancement? The patch was accepted based on Arend's assumption[1] that every chipset/firmware include a rev 0 for each country code , but it's never been officially confirmed. And from what you report here, it doesn't seem to stand unfortunately. [1] https://lore.kernel.org/netdev/17998013ac0.279b.9b12b7fc0a3841636cfb5e919b41b954@broadcom.com/ > >> I have installed different firmware files, brcmfmac4359-sdio.clm_blob, > >> brcmfmac4359-sdio.bin, brcmfmac4359-sdio.txt, the latter also linked as > >> brcmfmac4359-sdio.pine64,rockpro64-2.1.txt. This probably is the nvram > >> file. ccode and regrev are set to zero, which probably means > >> 'international save settings". > > I'm not sure how this 'international save settings' works for brcmfmac > > devices. Do you have more info or any pointers? > The correct term in this context probably is 'world regulatory domain', > the most restrictive wifi settings that can be used all over the world. > This usually is taken as default by cfg80211, apparently also for > (some?) brcmfmac devices/firmwares. > > These 'world' settings can be replaced by more permissive country > specific regulatory domain settings, but for brcmfmac devices this seems > to be firmware specific and requires this country mapping. > > I have seen a country code "00" for the world regulatory domain in the > past, not sure if this is standard or a device/driver/software specific > hack and if this can be used for brcmfmac (mapping from string "00" to > country_code=0 ?). For sure here are more experienced wifi developers > who know better. Yeah, it would be nice if someone can help clarify what both `ccode` and `regrev` in nvram file being zero means, like what should be working and what's not. > >>> But roaming to a different > >>> region will mostly get you a broken WiFi support. Is it possible to set > >>> up the country_codes for your device to get it work properly? > >> In linux-5.13 it worked, probably with save settings (not all channels > >> selectable, limited tx power), with linux-5.14 it stopped working, so it > >> is a regression. > >> I personally would like to learn how all this is configured properly. > >> For general use I think save settings are better than no wifi at all > >> with this patch. This fallback to ISO CC seams to work with newer > >> (Synaptics?) devices only. > > I do not mind you send a reverting if you have problem to add a proper > > translation table for your device. But that would mean I have to add > > a pretty "meaningless" translation table for my devices :( > > > Is this not the usual DT policy, that missing optional properties should > not prevent a device to work, that old dtbs should still work when new > properties are added? > > I'm not sure what's the best way forward. A plain revert of this patch > would at least bring back wifi support for RockPro64 devices with > existing dtbs. Maybe someone else has a better proposal how to proceed. Go ahead to revert if we do not hear a better solution, I would say. Shawn
Shawn Guo <shawn.guo@linaro.org> writes: >> Is this not the usual DT policy, that missing optional properties should >> not prevent a device to work, that old dtbs should still work when new >> properties are added? >> >> I'm not sure what's the best way forward. A plain revert of this patch >> would at least bring back wifi support for RockPro64 devices with >> existing dtbs. Maybe someone else has a better proposal how to proceed. > > Go ahead to revert if we do not hear a better solution, I would say. Yes, please do send a revert. And remember to explain the regression in the commit log. -- https://patchwork.kernel.org/project/linux-wireless/list/ https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On 21.09.21 11:20, Kalle Valo wrote: > Shawn Guo <shawn.guo@linaro.org> writes: > >>> Is this not the usual DT policy, that missing optional properties should >>> not prevent a device to work, that old dtbs should still work when new >>> properties are added? >>> >>> I'm not sure what's the best way forward. A plain revert of this patch >>> would at least bring back wifi support for RockPro64 devices with >>> existing dtbs. Maybe someone else has a better proposal how to proceed. >> Go ahead to revert if we do not hear a better solution, I would say. > Yes, please do send a revert. And remember to explain the regression in > the commit log. > I sent a revert patch. Sorry for the delay, Soeren
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c index f4405d7861b6..6cb09c7c37b6 100644 --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/cfg80211.c @@ -7442,18 +7442,23 @@ static s32 brcmf_translate_country_code(struct brcmf_pub *drvr, char alpha2[2], s32 found_index; int i; - country_codes = drvr->settings->country_codes; - if (!country_codes) { - brcmf_dbg(TRACE, "No country codes configured for device\n"); - return -EINVAL; - } - if ((alpha2[0] == ccreq->country_abbrev[0]) && (alpha2[1] == ccreq->country_abbrev[1])) { brcmf_dbg(TRACE, "Country code already set\n"); return -EAGAIN; } + country_codes = drvr->settings->country_codes; + if (!country_codes) { + brcmf_dbg(TRACE, "No country codes configured for device, using ISO3166 code and 0 rev\n"); + memset(ccreq, 0, sizeof(*ccreq)); + ccreq->country_abbrev[0] = alpha2[0]; + ccreq->country_abbrev[1] = alpha2[1]; + ccreq->ccode[0] = alpha2[0]; + ccreq->ccode[1] = alpha2[1]; + return 0; + } + found_index = -1; for (i = 0; i < country_codes->table_size; i++) { cc = &country_codes->table[i];
Instead of aborting country code setup in firmware, use ISO3166 country code and 0 rev as fallback, when country_codes mapping table is not configured. This fallback saves the country_codes table setup for recent brcmfmac chipsets/firmwares, which just use ISO3166 code and require no revision number. Signed-off-by: Shawn Guo <shawn.guo@linaro.org> --- .../broadcom/brcm80211/brcmfmac/cfg80211.c | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)