Re: usb: dwc2: regression on MyBook Live Duo / Canyonlands since 4.3.0-rc4

[Benjamin Herrenschmidt]

On Mon, 2016-05-09 at 21:06 +0200, Christian Lamparter via Linuxppc-dev wrote:
> 
> I ran into the following issues:
> ÂÂÂÂÂÂÂÂ- gadget.c uses ioread32_rep [0] & iowrite32_rep [1].
> ÂÂÂÂÂÂÂÂÂ This is interesting because both of these functions actually use
> ÂÂÂÂÂÂÂÂÂ the __raw_io* on powerpc. This is because powerpc uses the default
> ÂÂÂÂÂÂÂÂÂ defines of include/asm-generic/io.h [2].
> 
> ÂÂÂÂÂÂÂÂÂ Ideally, this should be done by sth like a writesl_be or writesl(e)
> ÂÂÂÂÂÂÂÂÂ function. But I found none so for now: Let's make a ugly hack:
> ÂÂÂÂÂÂÂÂÂ to_correct_endian that will work for testing, but will be replaced.

No. The _rep variants always must use native endian. If for some reason
your device requires something different then the device is more broken
than I thought. IE. even with a BE core and an LE device, we use non-
byeswap _rep versions, this isn't just because we use "defaults" on
powerpc for example, it's necessary to work.

Here too, I could suggest you google for my talk on the subject :-) But
if you think closely about it, you'll figure out that FIFOs don't have
an endian per-se, thus what matters is which byte is first in ascending
address order, and that byte must land in memory in the first address
as well.

Interpreting the resulting data might require endian swaps, but
actually transferring to/from the fifo doesn't.

> ÂÂ - dwc2_readl, dwc2_writel. I have no insight in the craziness that's
> ÂÂÂÂÂÂÂÂÂ going on with MIPS and the memory barrier. But it got me thinking
> ÂÂÂÂÂÂÂÂÂ rather than CONFIG_MIPS, isn't there a umbrella
> ÂÂÂÂÂÂÂÂÂ "ARCH_HAS_NEED_MEMORY_BARRIER_FOR_MMIO" config symbol?
> 
> ÂÂÂÂÂÂÂÂ- is_little_endian (do we want a separate is_big_endian?)
> ÂÂÂÂÂÂÂÂÂ Also, do we want to be able to overwrite the detection code
> ÂÂÂÂÂÂÂÂÂ if the endian setting was set in the device tree?. For now
> ÂÂÂÂÂÂÂÂÂ it always does auto detection (see dwc2_detect_endiannes() ).

If auto-detect always work, no need to bother with the device-tree. A
single flag is enough, either is_big or is_little, doesn't matter.

"readl" should always be little, readl_be should always be big, though
the latter isn't supported on all archs. However the new accessors in
iomap.h should provide a "be" variant on all archs so switching to
these might be a good idea.

> ÂÂÂÂÂÂÂÂ( - 80 character per line issues, is it possible to drop the
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ hsotg->reg + REGISTER from the dwc2_readl/writel since we
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ pass the hsotg now anyway and do the reg + REGISTER
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ calculation in the accessor? I played around with macros
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ as most functions calling the accessors have the hsotg
> ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ variable anyway )

Or just use a temp variable, the compiler should deal with it the same
way.

Ben.
Â
> Regards,
> Christian