| Message ID | 20180226090917.7iabysywbv6h4rqr@alienware17 (mailing list archive) |
|---|---|
| State | Rejected |
| Delegated to: | Kalle Valo |
| Headers | show |
Bas Vermeulen <bvermeul@blackstar.nl> writes: > A random (little endian eeprom'd) ar9278 card didn't work on my > PowerMac G5 without allowing the driver to byte-swap the eeprom. > > Introduce a module parameter endian_check to allow this to happen, > and the PCIe card to function correctly on BE powerpc. > > Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl> > --- > drivers/net/wireless/ath/ath9k/init.c | 6 +++++- > 1 file changed, 5 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c > index fa58a32227f5..421039dc060a 100644 > --- a/drivers/net/wireless/ath/ath9k/init.c > +++ b/drivers/net/wireless/ath/ath9k/init.c > @@ -67,6 +67,9 @@ static int ath9k_ps_enable; > module_param_named(ps_enable, ath9k_ps_enable, int, 0444); > MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave"); > > +static int ath9k_endian_check; > +module_param_named(endian_check, ath9k_endian_check, int, 0444); > +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness compatibility"); > #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT > > int ath9k_use_chanctx; > @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc) > ether_addr_copy(common->macaddr, mac); > > ah->ah_flags &= ~AH_USE_EEPROM; > - ah->ah_flags |= AH_NO_EEP_SWAP; > + if (!ath9k_endian_check) > + ah->ah_flags |= AH_NO_EEP_SWAP; A bit annoying to have a module parameter, isn't there any automatic way to detect/try this? But on the other hand I guess this isn't a common problem as nobody has reported this before?
On 26-02-18 10:54, Kalle Valo wrote: > Bas Vermeulen <bvermeul@blackstar.nl> writes: > >> A random (little endian eeprom'd) ar9278 card didn't work on my >> PowerMac G5 without allowing the driver to byte-swap the eeprom. >> >> Introduce a module parameter endian_check to allow this to happen, >> and the PCIe card to function correctly on BE powerpc. >> >> Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl> >> --- >> drivers/net/wireless/ath/ath9k/init.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c >> index fa58a32227f5..421039dc060a 100644 >> --- a/drivers/net/wireless/ath/ath9k/init.c >> +++ b/drivers/net/wireless/ath/ath9k/init.c >> @@ -67,6 +67,9 @@ static int ath9k_ps_enable; >> module_param_named(ps_enable, ath9k_ps_enable, int, 0444); >> MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave"); >> >> +static int ath9k_endian_check; >> +module_param_named(endian_check, ath9k_endian_check, int, 0444); >> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness compatibility"); >> #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT >> >> int ath9k_use_chanctx; >> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc) >> ether_addr_copy(common->macaddr, mac); >> >> ah->ah_flags &= ~AH_USE_EEPROM; >> - ah->ah_flags |= AH_NO_EEP_SWAP; >> + if (!ath9k_endian_check) >> + ah->ah_flags |= AH_NO_EEP_SWAP; > A bit annoying to have a module parameter, isn't there any automatic way > to detect/try this? But on the other hand I guess this isn't a common > problem as nobody has reported this before? There is an automatic way to detect this, but that is disabled by the AH_NO_EEP_SWAP flag. The platform initialisation does not set this flag if the endian_check member of pdata is set to true, but there is no way to not set this when using a device tree. I used a module parameter instead of a device tree variable because I don't know of a way to modify the device tree my PowerMac boots with. Bas Vermeulen
Am 26.02.2018 um 10:54 schrieb Kalle Valo: > Bas Vermeulen <bvermeul@blackstar.nl> writes: > >> A random (little endian eeprom'd) ar9278 card didn't work on my >> PowerMac G5 without allowing the driver to byte-swap the eeprom. >> >> Introduce a module parameter endian_check to allow this to happen, >> and the PCIe card to function correctly on BE powerpc. >> >> Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl> >> --- >> drivers/net/wireless/ath/ath9k/init.c | 6 +++++- >> 1 file changed, 5 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c >> index fa58a32227f5..421039dc060a 100644 >> --- a/drivers/net/wireless/ath/ath9k/init.c >> +++ b/drivers/net/wireless/ath/ath9k/init.c >> @@ -67,6 +67,9 @@ static int ath9k_ps_enable; >> module_param_named(ps_enable, ath9k_ps_enable, int, 0444); >> MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave"); >> >> +static int ath9k_endian_check; >> +module_param_named(endian_check, ath9k_endian_check, int, 0444); >> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness compatibility"); >> #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT >> >> int ath9k_use_chanctx; >> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc) >> ether_addr_copy(common->macaddr, mac); >> >> ah->ah_flags &= ~AH_USE_EEPROM; >> - ah->ah_flags |= AH_NO_EEP_SWAP; >> + if (!ath9k_endian_check) >> + ah->ah_flags |= AH_NO_EEP_SWAP; > A bit annoying to have a module parameter, isn't there any automatic way > to detect/try this? But on the other hand I guess this isn't a common > problem as nobody has reported this before? There is a way by simply checking the eeprom magic on this chipset >
Am 26.02.2018 um 11:07 schrieb Bas Vermeulen: > On 26-02-18 10:54, Kalle Valo wrote: >> Bas Vermeulen <bvermeul@blackstar.nl> writes: >> >>> A random (little endian eeprom'd) ar9278 card didn't work on my >>> PowerMac G5 without allowing the driver to byte-swap the eeprom. >>> >>> Introduce a module parameter endian_check to allow this to happen, >>> and the PCIe card to function correctly on BE powerpc. >>> >>> Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl> >>> --- >>> drivers/net/wireless/ath/ath9k/init.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/wireless/ath/ath9k/init.c >>> b/drivers/net/wireless/ath/ath9k/init.c >>> index fa58a32227f5..421039dc060a 100644 >>> --- a/drivers/net/wireless/ath/ath9k/init.c >>> +++ b/drivers/net/wireless/ath/ath9k/init.c >>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable; >>> module_param_named(ps_enable, ath9k_ps_enable, int, 0444); >>> MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave"); >>> +static int ath9k_endian_check; >>> +module_param_named(endian_check, ath9k_endian_check, int, 0444); >>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness >>> compatibility"); >>> #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT >>> int ath9k_use_chanctx; >>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc) >>> ether_addr_copy(common->macaddr, mac); >>> ah->ah_flags &= ~AH_USE_EEPROM; >>> - ah->ah_flags |= AH_NO_EEP_SWAP; >>> + if (!ath9k_endian_check) >>> + ah->ah_flags |= AH_NO_EEP_SWAP; >> A bit annoying to have a module parameter, isn't there any automatic way >> to detect/try this? But on the other hand I guess this isn't a common >> problem as nobody has reported this before? > There is an automatic way to detect this, but that is disabled by the > AH_NO_EEP_SWAP flag. > The platform initialisation does not set this flag if the endian_check > member of pdata is set > to true, but there is no way to not set this when using a device tree. > I used a module > parameter instead of a device tree variable because I don't know of a > way to modify the > device tree my PowerMac boots with. have you tried to compile it without device tree support? since its just a pcie card, i dont think that devicetree is required here it should run fine without it. > > Bas Vermeulen >
On 26-02-18 12:28, Sebastian Gottschall wrote: > Am 26.02.2018 um 10:54 schrieb Kalle Valo: >> Bas Vermeulen <bvermeul@blackstar.nl> writes: >> >>> A random (little endian eeprom'd) ar9278 card didn't work on my >>> PowerMac G5 without allowing the driver to byte-swap the eeprom. >>> >>> Introduce a module parameter endian_check to allow this to happen, >>> and the PCIe card to function correctly on BE powerpc. >>> >>> Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl> >>> --- >>> drivers/net/wireless/ath/ath9k/init.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/wireless/ath/ath9k/init.c >>> b/drivers/net/wireless/ath/ath9k/init.c >>> index fa58a32227f5..421039dc060a 100644 >>> --- a/drivers/net/wireless/ath/ath9k/init.c >>> +++ b/drivers/net/wireless/ath/ath9k/init.c >>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable; >>> module_param_named(ps_enable, ath9k_ps_enable, int, 0444); >>> MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave"); >>> +static int ath9k_endian_check; >>> +module_param_named(endian_check, ath9k_endian_check, int, 0444); >>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness >>> compatibility"); >>> #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT >>> int ath9k_use_chanctx; >>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc) >>> ether_addr_copy(common->macaddr, mac); >>> ah->ah_flags &= ~AH_USE_EEPROM; >>> - ah->ah_flags |= AH_NO_EEP_SWAP; >>> + if (!ath9k_endian_check) >>> + ah->ah_flags |= AH_NO_EEP_SWAP; >> A bit annoying to have a module parameter, isn't there any automatic way >> to detect/try this? But on the other hand I guess this isn't a common >> problem as nobody has reported this before? > There is a way by simply checking the eeprom magic on this chipset I am well aware. The AH_NO_EEP_SWAP flag disables fixing the eeprom by swapping the data read from eeprom. AH_NO_EEP_SWAP is enabled by default in ath9k_of_init() without this patch. I am happy if the AH_NO_EEP_SWAP flag is not set, that would fix my problem, but changes the current behaviour. I wanted to keep the current behaviour by default, and give me and others a way to make it work on big endian machines with cards with little endian eeproms. Bas Vermeulen
On 26-02-18 12:30, Sebastian Gottschall wrote: > Am 26.02.2018 um 11:07 schrieb Bas Vermeulen: >> On 26-02-18 10:54, Kalle Valo wrote: >>> Bas Vermeulen <bvermeul@blackstar.nl> writes: >>> >>>> A random (little endian eeprom'd) ar9278 card didn't work on my >>>> PowerMac G5 without allowing the driver to byte-swap the eeprom. >>>> >>>> Introduce a module parameter endian_check to allow this to happen, >>>> and the PCIe card to function correctly on BE powerpc. >>>> >>>> Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl> >>>> --- >>>> drivers/net/wireless/ath/ath9k/init.c | 6 +++++- >>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/net/wireless/ath/ath9k/init.c >>>> b/drivers/net/wireless/ath/ath9k/init.c >>>> index fa58a32227f5..421039dc060a 100644 >>>> --- a/drivers/net/wireless/ath/ath9k/init.c >>>> +++ b/drivers/net/wireless/ath/ath9k/init.c >>>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable; >>>> module_param_named(ps_enable, ath9k_ps_enable, int, 0444); >>>> MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave"); >>>> +static int ath9k_endian_check; >>>> +module_param_named(endian_check, ath9k_endian_check, int, 0444); >>>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness >>>> compatibility"); >>>> #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT >>>> int ath9k_use_chanctx; >>>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc) >>>> ether_addr_copy(common->macaddr, mac); >>>> ah->ah_flags &= ~AH_USE_EEPROM; >>>> - ah->ah_flags |= AH_NO_EEP_SWAP; >>>> + if (!ath9k_endian_check) >>>> + ah->ah_flags |= AH_NO_EEP_SWAP; >>> A bit annoying to have a module parameter, isn't there any automatic >>> way >>> to detect/try this? But on the other hand I guess this isn't a common >>> problem as nobody has reported this before? >> There is an automatic way to detect this, but that is disabled by the >> AH_NO_EEP_SWAP flag. >> The platform initialisation does not set this flag if the >> endian_check member of pdata is set >> to true, but there is no way to not set this when using a device >> tree. I used a module >> parameter instead of a device tree variable because I don't know of a >> way to modify the >> device tree my PowerMac boots with. > have you tried to compile it without device tree support? since its > just a pcie card, i dont think that devicetree is required here > it should run fine without it. The driver will still set AH_NO_EEP_SWAP regardless, and will still not swap the eeprom from little endian to big endian on big endian machines. See drivers/net/wireless/ath/ath9k/eeprom.c:188 and drivers/net/wireless/ath/ath9k/init.c (lines 593 and 645). The reason I'm talking about device trees here is because I could have used a device tree parameter instead of a module parameter. Bas Vermeulen
Bas Vermeulen <bvermeul@blackstar.nl> writes: > On 26-02-18 10:54, Kalle Valo wrote: >> Bas Vermeulen <bvermeul@blackstar.nl> writes: >> >>> A random (little endian eeprom'd) ar9278 card didn't work on my >>> PowerMac G5 without allowing the driver to byte-swap the eeprom. >>> >>> Introduce a module parameter endian_check to allow this to happen, >>> and the PCIe card to function correctly on BE powerpc. >>> >>> Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl> >>> --- >>> drivers/net/wireless/ath/ath9k/init.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c >>> index fa58a32227f5..421039dc060a 100644 >>> --- a/drivers/net/wireless/ath/ath9k/init.c >>> +++ b/drivers/net/wireless/ath/ath9k/init.c >>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable; >>> module_param_named(ps_enable, ath9k_ps_enable, int, 0444); >>> MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave"); >>> +static int ath9k_endian_check; >>> +module_param_named(endian_check, ath9k_endian_check, int, 0444); >>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness compatibility"); >>> #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT >>> int ath9k_use_chanctx; >>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc) >>> ether_addr_copy(common->macaddr, mac); >>> ah->ah_flags &= ~AH_USE_EEPROM; >>> - ah->ah_flags |= AH_NO_EEP_SWAP; >>> + if (!ath9k_endian_check) >>> + ah->ah_flags |= AH_NO_EEP_SWAP; >> A bit annoying to have a module parameter, isn't there any automatic way >> to detect/try this? But on the other hand I guess this isn't a common >> problem as nobody has reported this before? > > There is an automatic way to detect this, but that is disabled by the > AH_NO_EEP_SWAP flag. Ah, I didn't check the code at all. > The platform initialisation does not set this flag if the endian_check > member of pdata is set to true, but there is no way to not set this > when using a device tree. I used a module parameter instead of a > device tree variable because I don't know of a way to modify the > device tree my PowerMac boots with. Ok, makes sense. A module parameter is not an ideal solution I guess it's ok in this case.
On 02/26/2018 04:07 AM, Bas Vermeulen wrote: > On 26-02-18 10:54, Kalle Valo wrote: >> Bas Vermeulen <bvermeul@blackstar.nl> writes: >> >>> A random (little endian eeprom'd) ar9278 card didn't work on my >>> PowerMac G5 without allowing the driver to byte-swap the eeprom. >>> >>> Introduce a module parameter endian_check to allow this to happen, >>> and the PCIe card to function correctly on BE powerpc. >>> >>> Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl> >>> --- >>> drivers/net/wireless/ath/ath9k/init.c | 6 +++++- >>> 1 file changed, 5 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/net/wireless/ath/ath9k/init.c >>> b/drivers/net/wireless/ath/ath9k/init.c >>> index fa58a32227f5..421039dc060a 100644 >>> --- a/drivers/net/wireless/ath/ath9k/init.c >>> +++ b/drivers/net/wireless/ath/ath9k/init.c >>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable; >>> module_param_named(ps_enable, ath9k_ps_enable, int, 0444); >>> MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave"); >>> +static int ath9k_endian_check; >>> +module_param_named(endian_check, ath9k_endian_check, int, 0444); >>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness compatibility"); >>> #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT >>> int ath9k_use_chanctx; >>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc) >>> ether_addr_copy(common->macaddr, mac); >>> ah->ah_flags &= ~AH_USE_EEPROM; >>> - ah->ah_flags |= AH_NO_EEP_SWAP; >>> + if (!ath9k_endian_check) >>> + ah->ah_flags |= AH_NO_EEP_SWAP; >> A bit annoying to have a module parameter, isn't there any automatic way >> to detect/try this? But on the other hand I guess this isn't a common >> problem as nobody has reported this before? > There is an automatic way to detect this, but that is disabled by the > AH_NO_EEP_SWAP flag. > The platform initialisation does not set this flag if the endian_check member of > pdata is set > to true, but there is no way to not set this when using a device tree. I used a > module > parameter instead of a device tree variable because I don't know of a way to > modify the > device tree my PowerMac boots with. Shouldn't you be able to set ath9k_endian_check inside #ifdef __BIG_ENDIAN ... #endif in the initialization? I think that would achieve the same functionality without requiring the user to set a module parameter. I agree that you want to stay away from the device tree in a PPC computer. Larry
On 26-02-18 17:32, Larry Finger wrote: > On 02/26/2018 04:07 AM, Bas Vermeulen wrote: >> On 26-02-18 10:54, Kalle Valo wrote: >>> Bas Vermeulen <bvermeul@blackstar.nl> writes: >>> >>>> A random (little endian eeprom'd) ar9278 card didn't work on my >>>> PowerMac G5 without allowing the driver to byte-swap the eeprom. >>>> >>>> Introduce a module parameter endian_check to allow this to happen, >>>> and the PCIe card to function correctly on BE powerpc. >>>> >>>> Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl> >>>> --- >>>> drivers/net/wireless/ath/ath9k/init.c | 6 +++++- >>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/net/wireless/ath/ath9k/init.c >>>> b/drivers/net/wireless/ath/ath9k/init.c >>>> index fa58a32227f5..421039dc060a 100644 >>>> --- a/drivers/net/wireless/ath/ath9k/init.c >>>> +++ b/drivers/net/wireless/ath/ath9k/init.c >>>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable; >>>> module_param_named(ps_enable, ath9k_ps_enable, int, 0444); >>>> MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave"); >>>> +static int ath9k_endian_check; >>>> +module_param_named(endian_check, ath9k_endian_check, int, 0444); >>>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness >>>> compatibility"); >>>> #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT >>>> int ath9k_use_chanctx; >>>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc) >>>> ether_addr_copy(common->macaddr, mac); >>>> ah->ah_flags &= ~AH_USE_EEPROM; >>>> - ah->ah_flags |= AH_NO_EEP_SWAP; >>>> + if (!ath9k_endian_check) >>>> + ah->ah_flags |= AH_NO_EEP_SWAP; >>> A bit annoying to have a module parameter, isn't there any automatic >>> way >>> to detect/try this? But on the other hand I guess this isn't a common >>> problem as nobody has reported this before? >> There is an automatic way to detect this, but that is disabled by the >> AH_NO_EEP_SWAP flag. >> The platform initialisation does not set this flag if the >> endian_check member of pdata is set >> to true, but there is no way to not set this when using a device >> tree. I used a module >> parameter instead of a device tree variable because I don't know of a >> way to modify the >> device tree my PowerMac boots with. > > Shouldn't you be able to set ath9k_endian_check inside #ifdef > __BIG_ENDIAN ... #endif in the initialization? I think that would > achieve the same functionality without requiring the user to set a > module parameter. I could have done that, but didn't want to change the default behaviour. This patch keeps the current behaviour on all platforms if the module parameter is not set. I don't have the means to test mips and other platforms this could be used on. I don't mind having to set a module parameter, I mind not being able to change the behaviour. Bas Vermeulen
On Mon, 2018-02-26 at 17:44 +0100, Bas Vermeulen wrote: > On 26-02-18 17:32, Larry Finger wrote: > > On 02/26/2018 04:07 AM, Bas Vermeulen wrote: > > > On 26-02-18 10:54, Kalle Valo wrote: > > > > Bas Vermeulen <bvermeul@blackstar.nl> writes: > > > > > > > > > A random (little endian eeprom'd) ar9278 card didn't work on > > > > > my > > > > > PowerMac G5 without allowing the driver to byte-swap the > > > > > eeprom. > > > > > > > > > > Introduce a module parameter endian_check to allow this to > > > > > happen, > > > > > and the PCIe card to function correctly on BE powerpc. > > > > > > > > > > Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl> > > > > > --- > > > > > drivers/net/wireless/ath/ath9k/init.c | 6 +++++- > > > > > 1 file changed, 5 insertions(+), 1 deletion(-) > > > > > > > > > > diff --git a/drivers/net/wireless/ath/ath9k/init.c > > > > > b/drivers/net/wireless/ath/ath9k/init.c > > > > > index fa58a32227f5..421039dc060a 100644 > > > > > --- a/drivers/net/wireless/ath/ath9k/init.c > > > > > +++ b/drivers/net/wireless/ath/ath9k/init.c > > > > > @@ -67,6 +67,9 @@ static int ath9k_ps_enable; > > > > > module_param_named(ps_enable, ath9k_ps_enable, int, 0444); > > > > > MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave"); > > > > > +static int ath9k_endian_check; > > > > > +module_param_named(endian_check, ath9k_endian_check, int, > > > > > 0444); > > > > > +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness > > > > > compatibility"); > > > > > #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT > > > > > int ath9k_use_chanctx; > > > > > @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc > > > > > *sc) > > > > > ether_addr_copy(common->macaddr, mac); > > > > > ah->ah_flags &= ~AH_USE_EEPROM; > > > > > - ah->ah_flags |= AH_NO_EEP_SWAP; > > > > > + if (!ath9k_endian_check) > > > > > + ah->ah_flags |= AH_NO_EEP_SWAP; > > > > > > > > A bit annoying to have a module parameter, isn't there any > > > > automatic > > > > way > > > > to detect/try this? But on the other hand I guess this isn't a > > > > common > > > > problem as nobody has reported this before? > > > > > > There is an automatic way to detect this, but that is disabled by > > > the > > > AH_NO_EEP_SWAP flag. > > > The platform initialisation does not set this flag if the > > > endian_check member of pdata is set > > > to true, but there is no way to not set this when using a device > > > tree. I used a module > > > parameter instead of a device tree variable because I don't know > > > of a > > > way to modify the > > > device tree my PowerMac boots with. > > > > Shouldn't you be able to set ath9k_endian_check inside #ifdef > > __BIG_ENDIAN ... #endif in the initialization? I think that would > > achieve the same functionality without requiring the user to set a > > module parameter. > > I could have done that, but didn't want to change the default > behaviour. > This patch keeps the > current behaviour on all platforms if the module parameter is not > set. I > don't have the means > to test mips and other platforms this could be used on. I don't mind > having to set a module > parameter, I mind not being able to change the behaviour Still, module parameters are an awful user experience because you need to know that they exist, what they do, and whether you need it or not. It doesn't just work. Is there no way to autodetect the endian-ness of the firmware itself, and if the known machine endian-ness isn't the same then swap? This seems like a solvable problem. Dan
On 26-2-2018 18:42, Dan Williams wrote: > On Mon, 2018-02-26 at 17:44 +0100, Bas Vermeulen wrote: >> On 26-02-18 17:32, Larry Finger wrote: >>> On 02/26/2018 04:07 AM, Bas Vermeulen wrote: >>>> On 26-02-18 10:54, Kalle Valo wrote: >>>>> Bas Vermeulen <bvermeul@blackstar.nl> writes: >>>>> >>>>>> A random (little endian eeprom'd) ar9278 card didn't work on >>>>>> my >>>>>> PowerMac G5 without allowing the driver to byte-swap the >>>>>> eeprom. >>>>>> >>>>>> Introduce a module parameter endian_check to allow this to >>>>>> happen, >>>>>> and the PCIe card to function correctly on BE powerpc. >>>>>> >>>>>> Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl> >>>>>> --- >>>>>> drivers/net/wireless/ath/ath9k/init.c | 6 +++++- >>>>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>>>> >>>>>> diff --git a/drivers/net/wireless/ath/ath9k/init.c >>>>>> b/drivers/net/wireless/ath/ath9k/init.c >>>>>> index fa58a32227f5..421039dc060a 100644 >>>>>> --- a/drivers/net/wireless/ath/ath9k/init.c >>>>>> +++ b/drivers/net/wireless/ath/ath9k/init.c >>>>>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable; >>>>>> module_param_named(ps_enable, ath9k_ps_enable, int, 0444); >>>>>> MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave"); >>>>>> +static int ath9k_endian_check; >>>>>> +module_param_named(endian_check, ath9k_endian_check, int, >>>>>> 0444); >>>>>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness >>>>>> compatibility"); >>>>>> #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT >>>>>> int ath9k_use_chanctx; >>>>>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc >>>>>> *sc) >>>>>> ether_addr_copy(common->macaddr, mac); >>>>>> ah->ah_flags &= ~AH_USE_EEPROM; >>>>>> - ah->ah_flags |= AH_NO_EEP_SWAP; >>>>>> + if (!ath9k_endian_check) >>>>>> + ah->ah_flags |= AH_NO_EEP_SWAP; >>>>> A bit annoying to have a module parameter, isn't there any >>>>> automatic >>>>> way >>>>> to detect/try this? But on the other hand I guess this isn't a >>>>> common >>>>> problem as nobody has reported this before? >>>> There is an automatic way to detect this, but that is disabled by >>>> the >>>> AH_NO_EEP_SWAP flag. >>>> The platform initialisation does not set this flag if the >>>> endian_check member of pdata is set >>>> to true, but there is no way to not set this when using a device >>>> tree. I used a module >>>> parameter instead of a device tree variable because I don't know >>>> of a >>>> way to modify the >>>> device tree my PowerMac boots with. >>> Shouldn't you be able to set ath9k_endian_check inside #ifdef >>> __BIG_ENDIAN ... #endif in the initialization? I think that would >>> achieve the same functionality without requiring the user to set a >>> module parameter. >> I could have done that, but didn't want to change the default >> behaviour. >> This patch keeps the >> current behaviour on all platforms if the module parameter is not >> set. I >> don't have the means >> to test mips and other platforms this could be used on. I don't mind >> having to set a module >> parameter, I mind not being able to change the behaviour > Still, module parameters are an awful user experience because you need > to know that they exist, what they do, and whether you need it or not. > It doesn't just work. > > Is there no way to autodetect the endian-ness of the firmware itself, > and if the known machine endian-ness isn't the same then swap? This > seems like a solvable problem. The problem is already solved, but the solution is disabled by the AH_NO_EEP_SWAP flag set in both initialization functions. See ath9k/init.c and ath9k/eeprom.c. My patch just disables the flag when a module parameter is set, which enables the solution. I can disable the flag without a problem, but that might have unintended side effects on some platforms. If the consensus is that's the better solution I can prepare a patch, but that would have to come from someone more knowledgeable than me. Bas Vermeulen
On 26-02-18 15:52, Kalle Valo wrote: > Bas Vermeulen <bvermeul@blackstar.nl> writes: > >> On 26-02-18 10:54, Kalle Valo wrote: >>> Bas Vermeulen <bvermeul@blackstar.nl> writes: >>> >>>> A random (little endian eeprom'd) ar9278 card didn't work on my >>>> PowerMac G5 without allowing the driver to byte-swap the eeprom. >>>> >>>> Introduce a module parameter endian_check to allow this to happen, >>>> and the PCIe card to function correctly on BE powerpc. >>>> >>>> Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl> >>>> --- >>>> drivers/net/wireless/ath/ath9k/init.c | 6 +++++- >>>> 1 file changed, 5 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c >>>> index fa58a32227f5..421039dc060a 100644 >>>> --- a/drivers/net/wireless/ath/ath9k/init.c >>>> +++ b/drivers/net/wireless/ath/ath9k/init.c >>>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable; >>>> module_param_named(ps_enable, ath9k_ps_enable, int, 0444); >>>> MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave"); >>>> +static int ath9k_endian_check; >>>> +module_param_named(endian_check, ath9k_endian_check, int, 0444); >>>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness compatibility"); >>>> #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT >>>> int ath9k_use_chanctx; >>>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc) >>>> ether_addr_copy(common->macaddr, mac); >>>> ah->ah_flags &= ~AH_USE_EEPROM; >>>> - ah->ah_flags |= AH_NO_EEP_SWAP; >>>> + if (!ath9k_endian_check) >>>> + ah->ah_flags |= AH_NO_EEP_SWAP; >>> A bit annoying to have a module parameter, isn't there any automatic way >>> to detect/try this? But on the other hand I guess this isn't a common >>> problem as nobody has reported this before? >> There is an automatic way to detect this, but that is disabled by the >> AH_NO_EEP_SWAP flag. > Ah, I didn't check the code at all. > >> The platform initialisation does not set this flag if the endian_check >> member of pdata is set to true, but there is no way to not set this >> when using a device tree. I used a module parameter instead of a >> device tree variable because I don't know of a way to modify the >> device tree my PowerMac boots with. > Ok, makes sense. A module parameter is not an ideal solution I guess > it's ok in this case. Kalle: Are there any changes you want me to make in order to get this accepted? I didn't see anything for me to resolve, but I may have missed something. I can submit a patch to not set the AH_NO_EEP_SWAP flag by default if you prefer, as that would fix my problem as well. I am just not sure that doesn't break things for some platform/device I don't have. Bas Vermeulen
Bas Vermeulen <bvermeul@blackstar.nl> writes: >>>>> --- a/drivers/net/wireless/ath/ath9k/init.c >>>>> +++ b/drivers/net/wireless/ath/ath9k/init.c >>>>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable; >>>>> module_param_named(ps_enable, ath9k_ps_enable, int, 0444); >>>>> MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave"); >>>>> +static int ath9k_endian_check; >>>>> +module_param_named(endian_check, ath9k_endian_check, int, 0444); >>>>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness compatibility"); >>>>> #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT >>>>> int ath9k_use_chanctx; >>>>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc) >>>>> ether_addr_copy(common->macaddr, mac); >>>>> ah->ah_flags &= ~AH_USE_EEPROM; >>>>> - ah->ah_flags |= AH_NO_EEP_SWAP; >>>>> + if (!ath9k_endian_check) >>>>> + ah->ah_flags |= AH_NO_EEP_SWAP; >>>> A bit annoying to have a module parameter, isn't there any automatic way >>>> to detect/try this? But on the other hand I guess this isn't a common >>>> problem as nobody has reported this before? >>> >>> There is an automatic way to detect this, but that is disabled by the >>> AH_NO_EEP_SWAP flag. >> >> Ah, I didn't check the code at all. >> >>> The platform initialisation does not set this flag if the endian_check >>> member of pdata is set to true, but there is no way to not set this >>> when using a device tree. I used a module parameter instead of a >>> device tree variable because I don't know of a way to modify the >>> device tree my PowerMac boots with. >> >> Ok, makes sense. A module parameter is not an ideal solution I guess >> it's ok in this case. >> > Kalle: Are there any changes you want me to make in order to get this > accepted? I didn't see anything for me to resolve, but I may have > missed something. > > I can submit a patch to not set the AH_NO_EEP_SWAP flag by default if > you prefer, as that would fix my problem as well. I am just not sure > that doesn't break things for some platform/device I don't have. I'm not really sure what to do. Basically this is a choise between bad for user experience (the module parameter) or risk of regressions (disable AH_NO_EEP_SWAP by default). As ath9k is used in very exotic hardware I'm starting to lean towards the module parameter approach (your patch) but would like to know what others think, especially Felix (CCed). Full patch here: https://patchwork.kernel.org/patch/10241731/
+ Martin On 3/14/2018 3:34 PM, Kalle Valo wrote: > Bas Vermeulen <bvermeul@blackstar.nl> writes: > >>>>>> --- a/drivers/net/wireless/ath/ath9k/init.c >>>>>> +++ b/drivers/net/wireless/ath/ath9k/init.c >>>>>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable; >>>>>> module_param_named(ps_enable, ath9k_ps_enable, int, 0444); >>>>>> MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave"); >>>>>> +static int ath9k_endian_check; >>>>>> +module_param_named(endian_check, ath9k_endian_check, int, 0444); >>>>>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness compatibility"); >>>>>> #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT >>>>>> int ath9k_use_chanctx; >>>>>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc) >>>>>> ether_addr_copy(common->macaddr, mac); >>>>>> ah->ah_flags &= ~AH_USE_EEPROM; >>>>>> - ah->ah_flags |= AH_NO_EEP_SWAP; >>>>>> + if (!ath9k_endian_check) >>>>>> + ah->ah_flags |= AH_NO_EEP_SWAP; >>>>> A bit annoying to have a module parameter, isn't there any automatic way >>>>> to detect/try this? But on the other hand I guess this isn't a common >>>>> problem as nobody has reported this before? >>>> >>>> There is an automatic way to detect this, but that is disabled by the >>>> AH_NO_EEP_SWAP flag. >>> >>> Ah, I didn't check the code at all. >>> >>>> The platform initialisation does not set this flag if the endian_check >>>> member of pdata is set to true, but there is no way to not set this >>>> when using a device tree. I used a module parameter instead of a >>>> device tree variable because I don't know of a way to modify the >>>> device tree my PowerMac boots with. >>> >>> Ok, makes sense. A module parameter is not an ideal solution I guess >>> it's ok in this case. >>> >> Kalle: Are there any changes you want me to make in order to get this >> accepted? I didn't see anything for me to resolve, but I may have >> missed something. >> >> I can submit a patch to not set the AH_NO_EEP_SWAP flag by default if >> you prefer, as that would fix my problem as well. I am just not sure >> that doesn't break things for some platform/device I don't have. > > I'm not really sure what to do. Basically this is a choise between bad > for user experience (the module parameter) or risk of regressions > (disable AH_NO_EEP_SWAP by default). As ath9k is used in very exotic > hardware I'm starting to lean towards the module parameter approach > (your patch) but would like to know what others think, especially Felix > (CCed). Hi Kalle, Sorry for barging in, but I figured git history might tell us something. The flag was originally added by Felix (commit a59dadbeeaf7 ("ath9k: add support for endian swap of eeprom from platform data")) and the function ath9k_of_init() was added by Martin (CCed): commit 138b41253d9c9f9a06c8b086880cd3e839a23d69 Author: Martin Blumenstingl <martin.blumenstingl@googlemail.com> Date: Sun Oct 16 22:59:07 2016 +0200 ath9k: parse the device configuration from an OF node Maybe he recalls what device(s) needed it. Regards, Arend
Hello everyone, On Wed, Mar 14, 2018 at 10:34 PM, Arend van Spriel <arend.vanspriel@broadcom.com> wrote: > + Martin > > On 3/14/2018 3:34 PM, Kalle Valo wrote: >> >> Bas Vermeulen <bvermeul@blackstar.nl> writes: >> >>>>>>> --- a/drivers/net/wireless/ath/ath9k/init.c >>>>>>> +++ b/drivers/net/wireless/ath/ath9k/init.c >>>>>>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable; >>>>>>> module_param_named(ps_enable, ath9k_ps_enable, int, 0444); >>>>>>> MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave"); >>>>>>> +static int ath9k_endian_check; >>>>>>> +module_param_named(endian_check, ath9k_endian_check, int, 0444); >>>>>>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness >>>>>>> compatibility"); >>>>>>> #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT >>>>>>> int ath9k_use_chanctx; >>>>>>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc) >>>>>>> ether_addr_copy(common->macaddr, mac); >>>>>>> ah->ah_flags &= ~AH_USE_EEPROM; >>>>>>> - ah->ah_flags |= AH_NO_EEP_SWAP; >>>>>>> + if (!ath9k_endian_check) >>>>>>> + ah->ah_flags |= AH_NO_EEP_SWAP; >>>>>> >>>>>> A bit annoying to have a module parameter, isn't there any automatic >>>>>> way >>>>>> to detect/try this? But on the other hand I guess this isn't a common >>>>>> problem as nobody has reported this before? >>>>> >>>>> >>>>> There is an automatic way to detect this, but that is disabled by the >>>>> AH_NO_EEP_SWAP flag. >>>> >>>> >>>> Ah, I didn't check the code at all. >>>> >>>>> The platform initialisation does not set this flag if the endian_check >>>>> member of pdata is set to true, but there is no way to not set this >>>>> when using a device tree. I used a module parameter instead of a >>>>> device tree variable because I don't know of a way to modify the >>>>> device tree my PowerMac boots with. >>>> >>>> >>>> Ok, makes sense. A module parameter is not an ideal solution I guess >>>> it's ok in this case. >>>> >>> Kalle: Are there any changes you want me to make in order to get this >>> accepted? I didn't see anything for me to resolve, but I may have >>> missed something. >>> >>> I can submit a patch to not set the AH_NO_EEP_SWAP flag by default if >>> you prefer, as that would fix my problem as well. I am just not sure >>> that doesn't break things for some platform/device I don't have. >> >> >> I'm not really sure what to do. Basically this is a choise between bad >> for user experience (the module parameter) or risk of regressions >> (disable AH_NO_EEP_SWAP by default). As ath9k is used in very exotic >> hardware I'm starting to lean towards the module parameter approach >> (your patch) but would like to know what others think, especially Felix >> (CCed). > > > Hi Kalle, > > Sorry for barging in, but I figured git history might tell us something. The > flag was originally added by Felix (commit a59dadbeeaf7 ("ath9k: add support > for endian swap of eeprom from platform data")) and the function > ath9k_of_init() was added by Martin (CCed): > > commit 138b41253d9c9f9a06c8b086880cd3e839a23d69 > Author: Martin Blumenstingl <martin.blumenstingl@googlemail.com> > Date: Sun Oct 16 22:59:07 2016 +0200 > > ath9k: parse the device configuration from an OF node > > Maybe he recalls what device(s) needed it. lots embedded devices (supported by OpenWrt) use ath9k chips my primary target was the BT HomeHub 5A and some AVM Fritz Box (all using a lantiq SoC, but there are many more). these typically ship with: - a generic ath9k EEPROM which is sometimes even stored on NAND (= not directly connected to the ath9k chipset), which is why we need the "qca,no-eeprom" - some ship with a broken EEPROM that enables the 5GHz band on a 2.4GHz-only card, which is why we need "qca,disable-5ghz" - some ship with an EEPROM that doesn't have a unique mac address (the mac address is sometimes also stored on NAND), which is why we support the "mac-address" property - some ship an EEPROM where only the magic bytes are swapped (but the content is not), while others have both (magic bytes and content) byte-swapped looking at ath9k_of_init it seems that "ah->ah_flags &= ~AH_USE_EEPROM" and "ah->ah_flags |= AH_NO_EEP_SWAP" are called unconditionally. maybe these should be part of the "qca,no-eeprom" if-block a few lines above I also added Mathias to the CC list @Mathias: I believe all our .dts files in OpenWrt which specify an ath9k chipset also set the "qca,no-eeprom" property, right (a quick check suggests "yes")? Regards Martin
On 19 March 2018 at 09:11, Martin Blumenstingl <martin.blumenstingl@googlemail.com> wrote: > - some ship with a broken EEPROM that enables the 5GHz band on a > 2.4GHz-only card, which is why we need "qca,disable-5ghz" No. You just need to use ieee80211-freq-limit.
Hi Rafal, On Tue, Mar 20, 2018 at 10:07 PM, Rafał Miłecki <zajec5@gmail.com> wrote: > On 19 March 2018 at 09:11, Martin Blumenstingl > <martin.blumenstingl@googlemail.com> wrote: >> - some ship with a broken EEPROM that enables the 5GHz band on a >> 2.4GHz-only card, which is why we need "qca,disable-5ghz" > > No. You just need to use ieee80211-freq-limit. you are right with this one, "qca,disable-5ghz" is an out-of-tree patch and ieee80211-freq-limit support should be added upstream instead however, the cause is still the same (devices are shipped with an EEPROM which has the 5GHz band enabled while it shouldn't have) Regards Martin
19.03.2018 09:11, Martin Blumenstingl: > Hello everyone, > > On Wed, Mar 14, 2018 at 10:34 PM, Arend van Spriel > <arend.vanspriel@broadcom.com> wrote: >> + Martin >> >> On 3/14/2018 3:34 PM, Kalle Valo wrote: >>> >>> Bas Vermeulen <bvermeul@blackstar.nl> writes: >>> >>>>>>>> --- a/drivers/net/wireless/ath/ath9k/init.c >>>>>>>> +++ b/drivers/net/wireless/ath/ath9k/init.c >>>>>>>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable; >>>>>>>> module_param_named(ps_enable, ath9k_ps_enable, int, 0444); >>>>>>>> MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave"); >>>>>>>> +static int ath9k_endian_check; >>>>>>>> +module_param_named(endian_check, ath9k_endian_check, int, 0444); >>>>>>>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness >>>>>>>> compatibility"); >>>>>>>> #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT >>>>>>>> int ath9k_use_chanctx; >>>>>>>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc) >>>>>>>> ether_addr_copy(common->macaddr, mac); >>>>>>>> ah->ah_flags &= ~AH_USE_EEPROM; >>>>>>>> - ah->ah_flags |= AH_NO_EEP_SWAP; >>>>>>>> + if (!ath9k_endian_check) >>>>>>>> + ah->ah_flags |= AH_NO_EEP_SWAP; >>>>>>> >>>>>>> A bit annoying to have a module parameter, isn't there any automatic >>>>>>> way >>>>>>> to detect/try this? But on the other hand I guess this isn't a common >>>>>>> problem as nobody has reported this before? >>>>>> >>>>>> >>>>>> There is an automatic way to detect this, but that is disabled by the >>>>>> AH_NO_EEP_SWAP flag. >>>>> >>>>> >>>>> Ah, I didn't check the code at all. >>>>> >>>>>> The platform initialisation does not set this flag if the endian_check >>>>>> member of pdata is set to true, but there is no way to not set this >>>>>> when using a device tree. I used a module parameter instead of a >>>>>> device tree variable because I don't know of a way to modify the >>>>>> device tree my PowerMac boots with. >>>>> >>>>> >>>>> Ok, makes sense. A module parameter is not an ideal solution I guess >>>>> it's ok in this case. >>>>> >>>> Kalle: Are there any changes you want me to make in order to get this >>>> accepted? I didn't see anything for me to resolve, but I may have >>>> missed something. >>>> >>>> I can submit a patch to not set the AH_NO_EEP_SWAP flag by default if >>>> you prefer, as that would fix my problem as well. I am just not sure >>>> that doesn't break things for some platform/device I don't have. >>> >>> >>> I'm not really sure what to do. Basically this is a choise between bad >>> for user experience (the module parameter) or risk of regressions >>> (disable AH_NO_EEP_SWAP by default). As ath9k is used in very exotic >>> hardware I'm starting to lean towards the module parameter approach >>> (your patch) but would like to know what others think, especially Felix >>> (CCed). >> >> >> Hi Kalle, >> >> Sorry for barging in, but I figured git history might tell us something. The >> flag was originally added by Felix (commit a59dadbeeaf7 ("ath9k: add support >> for endian swap of eeprom from platform data")) and the function >> ath9k_of_init() was added by Martin (CCed): >> >> commit 138b41253d9c9f9a06c8b086880cd3e839a23d69 >> Author: Martin Blumenstingl <martin.blumenstingl@googlemail.com> >> Date: Sun Oct 16 22:59:07 2016 +0200 >> >> ath9k: parse the device configuration from an OF node >> >> Maybe he recalls what device(s) needed it. > lots embedded devices (supported by OpenWrt) use ath9k chips > > my primary target was the BT HomeHub 5A and some AVM Fritz Box (all > using a lantiq SoC, but there are many more). these typically ship > with: > - a generic ath9k EEPROM which is sometimes even stored on NAND (= not > directly connected to the ath9k chipset), which is why we need the > "qca,no-eeprom" > - some ship with a broken EEPROM that enables the 5GHz band on a > 2.4GHz-only card, which is why we need "qca,disable-5ghz" > - some ship with an EEPROM that doesn't have a unique mac address (the > mac address is sometimes also stored on NAND), which is why we support > the "mac-address" property > - some ship an EEPROM where only the magic bytes are swapped (but the > content is not), while others have both (magic bytes and content) > byte-swapped > > looking at ath9k_of_init it seems that "ah->ah_flags &= > ~AH_USE_EEPROM" and "ah->ah_flags |= AH_NO_EEP_SWAP" are called > unconditionally. > maybe these should be part of the "qca,no-eeprom" if-block a few lines above > > I also added Mathias to the CC list > @Mathias: I believe all our .dts files in OpenWrt which specify an > ath9k chipset also set the "qca,no-eeprom" property, right (a quick > check suggests "yes")? Yes, they all should have the qca,no-eeprom property. If not, it is a bug in the OpenWrt dts files. Mathias
On 14-03-18 15:34, Kalle Valo wrote: > Bas Vermeulen <bvermeul@blackstar.nl> writes: > >>>>>> --- a/drivers/net/wireless/ath/ath9k/init.c >>>>>> +++ b/drivers/net/wireless/ath/ath9k/init.c >>>>>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable; >>>>>> module_param_named(ps_enable, ath9k_ps_enable, int, 0444); >>>>>> MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave"); >>>>>> +static int ath9k_endian_check; >>>>>> +module_param_named(endian_check, ath9k_endian_check, int, 0444); >>>>>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness compatibility"); >>>>>> #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT >>>>>> int ath9k_use_chanctx; >>>>>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc) >>>>>> ether_addr_copy(common->macaddr, mac); >>>>>> ah->ah_flags &= ~AH_USE_EEPROM; >>>>>> - ah->ah_flags |= AH_NO_EEP_SWAP; >>>>>> + if (!ath9k_endian_check) >>>>>> + ah->ah_flags |= AH_NO_EEP_SWAP; >>>>> A bit annoying to have a module parameter, isn't there any automatic way >>>>> to detect/try this? But on the other hand I guess this isn't a common >>>>> problem as nobody has reported this before? >>>> There is an automatic way to detect this, but that is disabled by the >>>> AH_NO_EEP_SWAP flag. >>> Ah, I didn't check the code at all. >>> >>>> The platform initialisation does not set this flag if the endian_check >>>> member of pdata is set to true, but there is no way to not set this >>>> when using a device tree. I used a module parameter instead of a >>>> device tree variable because I don't know of a way to modify the >>>> device tree my PowerMac boots with. >>> Ok, makes sense. A module parameter is not an ideal solution I guess >>> it's ok in this case. >>> >> Kalle: Are there any changes you want me to make in order to get this >> accepted? I didn't see anything for me to resolve, but I may have >> missed something. >> >> I can submit a patch to not set the AH_NO_EEP_SWAP flag by default if >> you prefer, as that would fix my problem as well. I am just not sure >> that doesn't break things for some platform/device I don't have. > I'm not really sure what to do. Basically this is a choise between bad > for user experience (the module parameter) or risk of regressions > (disable AH_NO_EEP_SWAP by default). As ath9k is used in very exotic > hardware I'm starting to lean towards the module parameter approach > (your patch) but would like to know what others think, especially Felix > (CCed). > > Full patch here: > > https://patchwork.kernel.org/patch/10241731/ Just another ping. As I understood things, all OpenWRT dts' use qca,no-eeprom, and perhaps we could move ~AH_USE_EEPROM and |= AH_NO_EEP_SWAP in that if block. This would solve my problem, as I need to have AH_NO_EEP_SWAP removed from flags for my card (which is a little endian eeprom/card used on a big endian machine). Would you like me to prepare a patch for this? Is there anyone who can test this on the various OpenWRT boards/soc and or other configurations? I don't want to break things for other people. Bas Vermeulen
Hello Bas, On Tue, Apr 10, 2018 at 11:05 AM, Bas Vermeulen <bvermeul@blackstar.nl> wrote: > On 14-03-18 15:34, Kalle Valo wrote: >> >> Bas Vermeulen <bvermeul@blackstar.nl> writes: >> >>>>>>> --- a/drivers/net/wireless/ath/ath9k/init.c >>>>>>> +++ b/drivers/net/wireless/ath/ath9k/init.c >>>>>>> @@ -67,6 +67,9 @@ static int ath9k_ps_enable; >>>>>>> module_param_named(ps_enable, ath9k_ps_enable, int, 0444); >>>>>>> MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave"); >>>>>>> +static int ath9k_endian_check; >>>>>>> +module_param_named(endian_check, ath9k_endian_check, int, 0444); >>>>>>> +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness >>>>>>> compatibility"); >>>>>>> #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT >>>>>>> int ath9k_use_chanctx; >>>>>>> @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc) >>>>>>> ether_addr_copy(common->macaddr, mac); >>>>>>> ah->ah_flags &= ~AH_USE_EEPROM; >>>>>>> - ah->ah_flags |= AH_NO_EEP_SWAP; >>>>>>> + if (!ath9k_endian_check) >>>>>>> + ah->ah_flags |= AH_NO_EEP_SWAP; >>>>>> >>>>>> A bit annoying to have a module parameter, isn't there any automatic >>>>>> way >>>>>> to detect/try this? But on the other hand I guess this isn't a common >>>>>> problem as nobody has reported this before? >>>>> >>>>> There is an automatic way to detect this, but that is disabled by the >>>>> AH_NO_EEP_SWAP flag. >>>> >>>> Ah, I didn't check the code at all. >>>> >>>>> The platform initialisation does not set this flag if the endian_check >>>>> member of pdata is set to true, but there is no way to not set this >>>>> when using a device tree. I used a module parameter instead of a >>>>> device tree variable because I don't know of a way to modify the >>>>> device tree my PowerMac boots with. >>>> >>>> Ok, makes sense. A module parameter is not an ideal solution I guess >>>> it's ok in this case. >>>> >>> Kalle: Are there any changes you want me to make in order to get this >>> accepted? I didn't see anything for me to resolve, but I may have >>> missed something. >>> >>> I can submit a patch to not set the AH_NO_EEP_SWAP flag by default if >>> you prefer, as that would fix my problem as well. I am just not sure >>> that doesn't break things for some platform/device I don't have. >> >> I'm not really sure what to do. Basically this is a choise between bad >> for user experience (the module parameter) or risk of regressions >> (disable AH_NO_EEP_SWAP by default). As ath9k is used in very exotic >> hardware I'm starting to lean towards the module parameter approach >> (your patch) but would like to know what others think, especially Felix >> (CCed). >> >> Full patch here: >> >> https://patchwork.kernel.org/patch/10241731/ > > > Just another ping. As I understood things, all OpenWRT dts' use > qca,no-eeprom, and perhaps we could > move ~AH_USE_EEPROM and |= AH_NO_EEP_SWAP in that if block. > > This would solve my problem, as I need to have AH_NO_EEP_SWAP removed from > flags for my card (which is a > little endian eeprom/card used on a big endian machine). > > Would you like me to prepare a patch for this? Is there anyone who can test > this on the various OpenWRT > boards/soc and or other configurations? I don't want to break things for > other people. can you please prepare a patch for this? if you want I can test it on two OpenWrt devices and see if it breaks anything (please give me a few days to test though) Regards Martin
diff --git a/drivers/net/wireless/ath/ath9k/init.c b/drivers/net/wireless/ath/ath9k/init.c index fa58a32227f5..421039dc060a 100644 --- a/drivers/net/wireless/ath/ath9k/init.c +++ b/drivers/net/wireless/ath/ath9k/init.c @@ -67,6 +67,9 @@ static int ath9k_ps_enable; module_param_named(ps_enable, ath9k_ps_enable, int, 0444); MODULE_PARM_DESC(ps_enable, "Enable WLAN PowerSave"); +static int ath9k_endian_check; +module_param_named(endian_check, ath9k_endian_check, int, 0444); +MODULE_PARM_DESC(endian_check, "Check EEPROM for endianness compatibility"); #ifdef CONFIG_ATH9K_CHANNEL_CONTEXT int ath9k_use_chanctx; @@ -587,7 +590,8 @@ static int ath9k_of_init(struct ath_softc *sc) ether_addr_copy(common->macaddr, mac); ah->ah_flags &= ~AH_USE_EEPROM; - ah->ah_flags |= AH_NO_EEP_SWAP; + if (!ath9k_endian_check) + ah->ah_flags |= AH_NO_EEP_SWAP; return 0; }
A random (little endian eeprom'd) ar9278 card didn't work on my PowerMac G5 without allowing the driver to byte-swap the eeprom. Introduce a module parameter endian_check to allow this to happen, and the PCIe card to function correctly on BE powerpc. Signed-off-by: Bas Vermeulen <bvermeul@blackstar.nl> --- drivers/net/wireless/ath/ath9k/init.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)