Hello Kalle,

On Fri, Nov 25, 2016 at 4:06 PM, Valo, Kalle <kv...@qca.qualcomm.com> wrote:
> Kalle Valo <kv...@codeaurora.org> writes:
>> Martin Blumenstingl <martin.blumensti...@googlemail.com> writes:
>>> There are two types of swapping the EEPROM data in the ath9k driver.
>>> Before this series one type of swapping could not be used without the
>>> other.
>>> The first type of swapping looks at the "magic bytes" at the start of
>>> the EEPROM data and performs swab16 on the EEPROM contents if needed.
>>> The second type of swapping is EEPROM format specific and swaps
>>> specific fields within the EEPROM itself (swab16, swab32 - depends on
>>> the EEPROM format).
>>> With this series the second part now looks at the EEPMISC register
>>> inside the EEPROM, which uses a bit to indicate if the EEPROM data
>>> is Big Endian (this is also done by the FreeBSD kernel).
>>> This has a nice advantage: currently there are some out-of-tree hacks
>>> (in OpenWrt and LEDE) where the EEPROM has a Big Endian header on a
>>> Big Endian system (= no swab16 is performed) but the EEPROM itself
>>> indicates that it's data is Little Endian. Until now the out-of-tree
>>> code simply did a swab16 before passing the data to ath9k, so ath9k
>>> first did the swab16 - this also enabled the format specific swapping.
>>> These out-of-tree hacks are still working with the new logic, but it
>>> is recommended to remove them. This implementation is based on a
>>> discussion with Arnd Bergmann who raised concerns about the
>>> robustness and portability of the swapping logic in the original OF
>>> support patch review, see [0].
>>> After a second round of patches (= v1 of this series) neither Arnd
>>> Bergmann nor I were really happy with the complexity of the EEPROM
>>> swapping logic. Based on a discussion (see [1] and [2]) we decided
>>> that ath9k should use a defined format (specifying the endianness
>>> of the data - I went with __le16 and __le32) when accessing the
>>> EEPROM fields. A benefit of this is that we enable the EEPMISC based
>>> swapping logic by default, just like the FreeBSD driver, see [3]. On
>>> the devices which I have tested (see below) ath9k now works without
>>> having to specify the "endian_check" field in ath9k_platform_data (or
>>> a similar logic which could provide this via devicetree) as ath9k now
>>> detects the endianness automatically. Only EEPROMs which are mangled
>>> by some out-of-tree code still need the endian_check flag (or one can
>>> simply remove that mangling from the out-of-tree code).
>>> Testing:
>>> - tested by myself on AR9287 with Big Endian EEPROM
>>> - tested by myself on AR9227 with Little Endian EEPROM
>>> - tested by myself on AR9381 (using the ar9003_eeprom implementation,
>>>   which did not suffer from this whole problem)
>>> - how do we proceed with testing? maybe we could keep this in a
>>>   feature-branch and add these patches to LEDE once we have an ACK to
>>>   get more people to test this
>>> This series depends on my other series (v7):
>>> "add devicetree support to ath9k" - see [4]
>> I think this looks pretty good. If there's a bug somewhere it should be
>> quite easy to fix so I'm not that worried and would be willing to take
>> these as soon as I have applied the dependency series. IIRC your
>> devicetree patches will have at least one more review round so that will
>> take some time still. In the meantime it would be great if LEDE folks
>> could take a look at these and comment (or test).
> So are everyone happy with this? I haven't seen any comments. If I don't
> here anything I'm planning to take these, most likely for 4.11.
the patches have been in LEDE for almost two weeks now and I did not
see any reports of ath9k breakage (footnote below).

It seems that there are a few devices out there where the whole EEPROM
is swab16'ed which switches the position of the 1-byte fields
opCapFlags and eepMisc.
those still work fine with the new code, however I had a second patch
in LEDE [0] which results in ath9k_platform_data.endian_check NOT
being set anymore.
that endian_check flag was used before to swab16 the whole EEPROM, to
correct the position of the 1-byte fields again.
Currently we are fixing this in the firmware hotplug script: [1]
This is definitely not a blocker for this series though (if we want to
have a devicetree replacement for "ath9k_platform_data.endian_check"
then I'd work on that within a separate series, but I somewhat
consider these EEPROMs as "broken" so fixing them in
userspace/firmware hotplug script is fine for me)


ath9k-devel mailing list

Reply via email to