El dom, 16 abr 2023 a las 15:37, Christian Lamparter (<chunkeey@xxxxxxxxx>) escribió: > > On 4/16/23 12:50, Toke Høiland-Jørgensen wrote: > > Christian Lamparter <chunkeey@xxxxxxxxx> writes: > > > >> On 4/15/23 18:02, Christian Lamparter wrote: > >>> Hi, > >>> > >>> On 4/15/23 17:25, Toke Høiland-Jørgensen wrote: > >>>> Álvaro Fernández Rojas <noltari@xxxxxxxxx> writes: > >>>> > >>>>> BCM63xx (Big Endian MIPS) devices store the calibration data in MTD > >>>>> partitions but it needs to be swapped in order to work, otherwise it fails: > >>>>> ath9k 0000:00:01.0: enabling device (0000 -> 0002) > >>>>> ath: phy0: Ignoring endianness difference in EEPROM magic bytes. > >>>>> ath: phy0: Bad EEPROM VER 0x0001 or REV 0x00e0 > >>>>> ath: phy0: Unable to initialize hardware; initialization status: -22 > >>>>> ath9k 0000:00:01.0: Failed to initialize device > >>>>> ath9k: probe of 0000:00:01.0 failed with error -22 > >>>> > >>>> How does this affect other platforms? Why was the NO_EEP_SWAP flag set > >>>> in the first place? Christian, care to comment on this? > >>> > >>> I knew this would come up. I've written what I know and remember in the > >>> pull-request/buglink. > >>> > >>> Maybe this can be added to the commit? > >>> Link: https://github.com/openwrt/openwrt/pull/12365 > >>> > >>> | From what I remember, the ah->ah_flags |= AH_NO_EEP_SWAP; was copied verbatim from ath9k_of_init's request_eeprom. > >>> > >>> Since the existing request_firmware eeprom fetcher code set the flag, > >>> the nvmem code had to do it too. > >>> > >>> In theory, I don't think that not setting the AH_NO_EEP_SWAP flag will cause havoc. > >>> I don't know if there are devices out there, which have a swapped magic (which is > >>> used to detect the endianess), but the caldata is in the correct endiannes (or > >>> vice versa - Magic is correct, but data needs swapping). > >>> > >>> I can run tests with it on a Netzgear WNDR3700v2 (AR7161+2xAR9220) > >>> and FritzBox 7360v2 (Lantiq XWAY+AR9220). (But these worked fine. > >>> So I don't expect there to be a new issue there). > >> > >> Nope! This is a classic self-own!... Well at least, this now gets documented! > >> > >> Here are my findings. Please excuse the overlong lines. > >> > >> ## The good news / AVM FritzBox 7360v2 ## > >> > >> The good news: The AVM FritzBox 7360v2 worked the same as before. > > > > [...] > > > >> ## The not so good news / Netgear WNDR3700v2 ## > >> > >> But not the Netgar WNDR3700v2. One WiFi (The 2.4G, reported itself now as the 5G @0000:00:11.0 - > >> doesn't really work now), and the real 5G WiFi (@0000:00:12.0) failed with: > >> "phy1: Bad EEPROM VER 0x0001 or REV 0x06e0" > > > > [...] > > > > Alright, so IIUC, we have a situation where some devices only work > > *with* the flag, and some devices only work *without* the flag? So we'll > > need some kind of platform-specific setting? Could we put this in the > > device trees, or is there a better solution? > > Depends. From what I gather, ath9k calls this "need_swap". Thing is, > the flag in the EEPROM is called "AR5416_EEPMISC_BIG_ENDIAN". In the > official documentation about the AR9170 Base EEPROM (has the same base > structure as AR5008 up to AR92xx) this is specified as: > > "Only bit 0 is defined as Big Endian. This bit should be written as 1 > when the structure is interpreted in big Endian byte ordering. This bit > must be reviewed before any larger than byte parameters can be interpreted." > > It makes sense that on a Big-Endian MIPS device (like the Netgear WNDR3700v2), > the caldata should be in "Big-Endian" too... so no swapping is necessary. > > Looking in ath9k's eeprom.c function ath9k_hw_nvram_swap_data() that deals > with this eepmisc flag: > > | if (ah->eep_ops->get_eepmisc(ah) & AR5416_EEPMISC_BIG_ENDIAN) { > | *swap_needed = true; > | ath_dbg(common, EEPROM, > | "Big Endian EEPROM detected according to EEPMISC register.\n"); > | } else { > | *swap_needed = false; > | } > > This doesn't take into consideration that swapping is not needed if > the data is in big endian format on a big endian device. So, this > could be changed so that the *swap_needed is only true if the flag and > device endiannes disagrees? > > That said, Martin and Felix have written their reasons in the cover letter > and patches for why the code is what it is: > <https://ath9k-devel.ath9k.narkive.com/2q5A6nu0/patch-0-5-ath9k-eeprom-swapping-improvements> > > Toke, What's your take on this? Having something similar like the > check_endian bool... but for OF? Or more logic that can somehow > figure out if it's big or little endian. I already have v2 patches adding a new device tree flag, but I'll wait for Toke's answer before sending them. In my patches I added a new DT flag "qca,endian-check", which follows the one from ath9k_platform_data (endian_check). > > Cheers, > Christian Best regards, Álvaro.
