[U-Boot-Users] [PATCH 2/3] Added USB PCI-OHCI chips support, interrupt pipe support and usb event poll support.

[Zhang Wei-r63237]

Hi, Markus, 

Thanks! Please see my inline comments:

> -----Original Message-----
> From: Markus Klotzbücher [mailto:mk at denx.de] 
> Hi Zhang Wei,
> 
> Thanks for your comments.
> 
> "Zhang Wei-r63237" <Wei.Zhang at freescale.com> writes:
> 
> > <...>
> >> >  
> >> > -#define readl(a) (*((vu_long *)(a)))
> >> > -#define writel(a, b) (*((vu_long *)(b)) = ((vu_long)a))
> >> > +#define readl(a) m32_swap(*((vu_long *)(a)))
> >> > +#define writel(a, b) (*((vu_long *)(b)) = m32_swap((vu_long)a))
> >> 
> >> Can you explain why this is needed? I'm well aware that the
> > endianness
> >> handling in the USB Code has become pretty awfull, so I'm trying to
> > at
> >> least understand why what is needed.
> >> 
> >
> > Yes, the endian is a really awfull problem. :) If you do 
> not swap the
> > value, you will get or put a wrong endian data in big-endian boards,
> > such as most powerpc boards. I've printed the debug information with
> > the register value by using readl() in our MPC8641HPCN board
> > (big-endian), no swap, the register value is endianness 
> different than
> > linux kernel usb-driver. It can not work. So, in x86 or the other
> > little-endian platforms, the swap is no need, but in big-endian
> > platforms, the swap is must be.
> 
> So this BE CPU and LE Controller again. The only problem I 
> see with your
> fix is that because you _always_ do byteswapping for register accesses
> on BE CPUs, this would break support for the case BE CPU and BE OHCI
> Controller (but which doesn't exist in U-Boot so far).
> 
> No need to change this now, because I'm going to clean this 
> up soon, but
> I'd appreciate your help in testing once this is done.
> 

I'm using m32_swap(), it is not _always_ do. You can see following codes in usb_ohci.c:

#if defined(CONFIG_440EP) || defined(CONFIG_MPC5200)
# define m16_swap(x) (x)
# define m32_swap(x) (x)
#else
# define m16_swap(x) swap_16(x)
# define m32_swap(x) swap_32(x)
#endif

And the swap_32() is also defined in the file include/usb.h:

#ifdef LITTLEENDIAN
# define swap_16(x) (x)
# define swap_32(x) (x)
#else
# define swap_16(x) __swap_16(x)
# define swap_32(x) __swap_32(x)
#endif /* LITTLEENDIAN */

So It _only_ works on BE platforms.

Best Regards,
Zhang Wei