Date:   Sun, 16 Apr 2023 16:28:24 +0200
From:   Álvaro Fernández Rojas <noltari@...il.com>
To:     Christian Lamparter <chunkeey@...il.com>
Cc:     Toke Høiland-Jørgensen <toke@...e.dk>,
        f.fainelli@...il.com, jonas.gorski@...il.com, nbd@....name,
        kvalo@...nel.org, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com, linux-wireless@...r.kernel.org,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ath9k: fix calibration data endianness

El dom, 16 abr 2023 a las 15:37, Christian Lamparter
(<chunkeey@...il.com>) escribió:
>
> On 4/16/23 12:50, Toke Høiland-Jørgensen wrote:
> > Christian Lamparter <chunkeey@...il.com> 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@...il.com> 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.

Powered by blists - more mailing lists