[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 for your feedback, please see my inline comments: 

> -----Original Message-----
> From: Markus Klotzbücher [mailto:mk at denx.de] 
> 
> Zhang Wei,
> 
> I have some minor issues with your this patch.
> 
> Zhang Wei <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.


> >  #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;
> >  	}

Best Regards,
Zhang Wei