From mboxrd@z Thu Jan 1 00:00:00 1970 From: Markus =?iso-8859-1?Q?Klotzb=FCcher?= Date: Tue, 15 May 2007 08:27:36 +0000 Subject: [U-Boot-Users] [PATCH 2/3] Added USB PCI-OHCI chips support, interrupt pipe support and usb event poll support. In-Reply-To: <46B96294322F7D458F9648B60E15112C234179@zch01exm26.fsl.freescale.net> (Zhang Wei-r's message of "Tue, 15 May 2007 10:35:08 +0800") References: <11774728931751-git-send-email-wei.zhang@freescale.com> <87zm47xy0a.fsf@denx.de> <46B96294322F7D458F9648B60E15112C234179@zch01exm26.fsl.freescale.net> Message-ID: <874pme330n.fsf@denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Hi Zhang Wei, Thanks for your comments. "Zhang Wei-r63237" 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