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

[Markus Klotzbücher]

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.

>
>> >  #define min_t(type,x,y) ({ type __x = (x); type __y = (y); 
>> __x < __y
>> > ? __x: __y; })
>> >  
>> > -#undef DEBUG
>> > +#ifdef CONFIG_PCI_OHCI
>> > +static struct pci_device_id ohci_pci_ids[] = {
>> > +	{0x10b9, 0x5237},	/* ULI1575 PCI OHCI module ids */
>> > +	/* Please add supported PCI OHCI controller ids here */
>> > +	{0, 0}
>> > +};
>> > +#endif
>> > +
>> >  #ifdef DEBUG
>> > #define dbg(format, arg...) printf("DEBUG: " format "\n", ## arg)
>> >  #else
>> > @@ -114,15 +130,11 @@ struct ohci_hcca ghcca[1];
>> >  struct ohci_hcca *phcca;
>> >  /* this allocates EDs for all possible endpoints */
>> >  struct ohci_device ohci_dev;
>> > -/* urb_priv */
>> > -urb_priv_t urb_priv;
>> >  /* RHSC flag */
>> >  int got_rhsc;
>> >  /* device which was disconnected */
>> >  struct usb_device *devgone;
>> >  
>> > -/* flag guarding URB transation */
>> > -int urb_finished = 0;
>> 
>> Can you explain why this flag is not needed anymore? We had a 
>> discussion
>> about this some time ago and agreed that is should not be 
>> removed unless
>> we understand exactly why it was put there.
>> 
>
> You misunderstands it. :) I do not remove it. I just move it to the
> individual urb structure.
> I'll explain it. If no interrupt pipe support, a global urb_finished
> flag is ok, only one urb should be processed in the same time. But if
> the interrupt pipe support is added, it's difference. The interrupt
> pipe is an 'endless' urb, there are more than one urb should be
> processed in the same time. When the interrupt pipe is in processing,
> the other urb such as ctrl urb is also should be processed. So I move
> the urb_finished flag to individual urb structure: urb->finished.
>
> Such as below:
>
>> > /* we're about to begin a new transaction here so mark the URB
>> > unfinished */
>> > -	urb_finished = 0;
>> > +	urb->finished = 0;
>> >  
>> >  	/* every endpoint has a ed, locate and fill it */
>> > -	if (!(ed = ep_add_ed (dev, pipe))) {
>> > +	if (!(ed = ep_add_ed (dev, pipe, interval, 1))) {
>> >  		err("sohci_submit_job: ENOMEM");
>> >  		return -1;

I get it, thanks for explaining.

Best regards

Markus

--
DENX Software Engineering GmbH, HRB 165235 Munich, CEO: Wolfgang Denk
Office:  Kirchenstr. 5,       D-82194 Groebenzell,            Germany