Re: [linux-usb-devel] [PATCH] PXA27x UDC driver.
On Monday 09 July 2007, Rodolfo Giometti wrote: > On Sat, Jun 30, 2007 at 06:46:14AM -0700, David Brownell wrote: > > > > Has someone actually signed up to develop and maintain such a > > controller driver? If so, that would be a Fine Thing, but not > > one I've heard rumored before. I've assumed that the best we'd > > have is someone (Rodolfo?!) finally cleaning up a pxa27x version > > so it could be merged. > > That's what I wish to do... I wish cleaning up as much possible my > pxa27x version af the driver so we can have something common where we > can work on. Good! > Sorry for the delay but in these next days I'll read back all your > suggestions and I'll provide a new patch ASAP. > > However I would like avoiding to remove what I did on > usb_ep_autoconfig() since I got functional g_ether device with RNDIS > support... there could be another way to resolve this problem? I *really* don't want to change the whole gadget stack just to make one excessively quirky bit of hardware work with it ... when we know there's a simple solution that doesn't involve that. (Changing the whole gadget stack would involve retesting a boatload of systems... not all of which are easily tested. Plus, those changes would need far more review than they can get just now.) So for now, just do what I outlined before: set up the pxa27x UDC to resemble the pxa2[156] chips: a bunch of bulk endpoint with no altsettings and acting the same in all three hardware-supported configs. That will support RNDIS. In fact the only thing it _won't_ handle is the "real CDC Ethernet" mode. Once that's working, we can revisit the issue of how to configure endpoint hardware ... focussing on just that issue, and with enough flexibility that non-pxa27x hardware can also be addressed. - Dave - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] PXA27x UDC driver.
Hi, > > > +static int > > > +write_packet(volatile u32 * uddr, struct pxa27x_request *req, unsigned > > > max) > > > > Please review Documentation/volatile-considered-harmful.txt > > The attibute volatile here is necessary in order to avoid getting the > following warnings from the compiler: > [...] > > Do you know how I can resolve this? > ioremap() the memory area for the UDC registers and use an __iomem cookie. Lothar Wassmann -- ___ Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10 Geschäftsführer: Matthias Kaussen, Rolf Rosenstein Handelsregistereintrag: Amtsgericht Aachen, HRB 4996 www.karo-electronics.de | [EMAIL PROTECTED] ___ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] PXA27x UDC driver.
On Thu, Jun 28, 2007 at 02:29:49PM -0700, Andrew Morton wrote: > > > > + > > +static int > > +write_packet(volatile u32 * uddr, struct pxa27x_request *req, unsigned max) > > Please review Documentation/volatile-considered-harmful.txt The attibute volatile here is necessary in order to avoid getting the following warnings from the compiler: CC drivers/usb/gadget/pxa27x_udc.o drivers/usb/gadget/pxa27x_udc.c: In function `write_ep0_fifo': drivers/usb/gadget/pxa27x_udc.c:512: warning: passing arg 1 of `write_packet' discards qualifiers from pointer target type drivers/usb/gadget/pxa27x_udc.c: In function `pxa27x_ep_alloc': drivers/usb/gadget/pxa27x_udc.c:1206: warning: assignment discards qualifiers from pointer target type drivers/usb/gadget/pxa27x_udc.c:1207: warning: assignment discards qualifiers from pointer target type drivers/usb/gadget/pxa27x_udc.c:1208: warning: assignment discards qualifiers from pointer target type drivers/usb/gadget/pxa27x_udc.c:1209: warning: assignment discards qualifiers from pointer target type drivers/usb/gadget/pxa27x_udc.c: At top level: drivers/usb/gadget/pxa27x_udc.c:2159: warning: initialization discards qualifiers from pointer target type drivers/usb/gadget/pxa27x_udc.c:2160: warning: initialization discards qualifiers from pointer target type This because the variables reg_* are assigned as: .reg_udccsr = where UDCCSR0 are defined as: #define UDCCSR0 __REG(0x40600100) /* UDC Control/Status register - Endpoint 0 */ and: define __REG(x) (*((volatile u32 *)io_p2v(x))) Do you know how I can resolve this? Thanks in advance, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-usb-devel] [PATCH] PXA27x UDC driver.
Rodolfo Giometti wrote: That's what I wish to do... I wish cleaning up as much possible my pxa27x version af the driver so we can have something common where we can work on. Sorry for the delay but in these next days I'll read back all your suggestions and I'll provide a new patch ASAP. However I would like avoiding to remove what I did on usb_ep_autoconfig() since I got functional g_ether device with RNDIS support... there could be another way to resolve this problem? Thanks a lot, Rodolfo Rodolfo, I am eager to see an "official" USB driver. I have used a patch I found on the web that was attributed to you for 2.6.20 in our recent project. It patched cleanly and has worked fairly well. I am busy with other projects right now but might be able to squeeze some time to test on our board. We have a custom PXA 270 board and we are moving to the PXA 320. I was very concerned that we have no 270 driver in mainline and no hope of a 320 driver. Regards, Vern InHand Electronics, Inc. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-usb-devel] [PATCH] PXA27x UDC driver.
Rodolfo Giometti wrote: That's what I wish to do... I wish cleaning up as much possible my pxa27x version af the driver so we can have something common where we can work on. Sorry for the delay but in these next days I'll read back all your suggestions and I'll provide a new patch ASAP. However I would like avoiding to remove what I did on usb_ep_autoconfig() since I got functional g_ether device with RNDIS support... there could be another way to resolve this problem? Thanks a lot, Rodolfo Rodolfo, I am eager to see an official USB driver. I have used a patch I found on the web that was attributed to you for 2.6.20 in our recent project. It patched cleanly and has worked fairly well. I am busy with other projects right now but might be able to squeeze some time to test on our board. We have a custom PXA 270 board and we are moving to the PXA 320. I was very concerned that we have no 270 driver in mainline and no hope of a 320 driver. Regards, Vern InHand Electronics, Inc. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] PXA27x UDC driver.
On Thu, Jun 28, 2007 at 02:29:49PM -0700, Andrew Morton wrote: + +static int +write_packet(volatile u32 * uddr, struct pxa27x_request *req, unsigned max) Please review Documentation/volatile-considered-harmful.txt The attibute volatile here is necessary in order to avoid getting the following warnings from the compiler: CC drivers/usb/gadget/pxa27x_udc.o drivers/usb/gadget/pxa27x_udc.c: In function `write_ep0_fifo': drivers/usb/gadget/pxa27x_udc.c:512: warning: passing arg 1 of `write_packet' discards qualifiers from pointer target type drivers/usb/gadget/pxa27x_udc.c: In function `pxa27x_ep_alloc': drivers/usb/gadget/pxa27x_udc.c:1206: warning: assignment discards qualifiers from pointer target type drivers/usb/gadget/pxa27x_udc.c:1207: warning: assignment discards qualifiers from pointer target type drivers/usb/gadget/pxa27x_udc.c:1208: warning: assignment discards qualifiers from pointer target type drivers/usb/gadget/pxa27x_udc.c:1209: warning: assignment discards qualifiers from pointer target type drivers/usb/gadget/pxa27x_udc.c: At top level: drivers/usb/gadget/pxa27x_udc.c:2159: warning: initialization discards qualifiers from pointer target type drivers/usb/gadget/pxa27x_udc.c:2160: warning: initialization discards qualifiers from pointer target type This because the variables reg_* are assigned as: .reg_udccsr = UDCCSR0 where UDCCSR0 are defined as: #define UDCCSR0 __REG(0x40600100) /* UDC Control/Status register - Endpoint 0 */ and: define __REG(x) (*((volatile u32 *)io_p2v(x))) Do you know how I can resolve this? Thanks in advance, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] PXA27x UDC driver.
Hi, +static int +write_packet(volatile u32 * uddr, struct pxa27x_request *req, unsigned max) Please review Documentation/volatile-considered-harmful.txt The attibute volatile here is necessary in order to avoid getting the following warnings from the compiler: [...] Do you know how I can resolve this? ioremap() the memory area for the UDC registers and use an __iomem cookie. Lothar Wassmann -- ___ Ka-Ro electronics GmbH | Pascalstraße 22 | D - 52076 Aachen Phone: +49 2408 1402-0 | Fax: +49 2408 1402-10 Geschäftsführer: Matthias Kaussen, Rolf Rosenstein Handelsregistereintrag: Amtsgericht Aachen, HRB 4996 www.karo-electronics.de | [EMAIL PROTECTED] ___ - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-usb-devel] [PATCH] PXA27x UDC driver.
On Monday 09 July 2007, Rodolfo Giometti wrote: On Sat, Jun 30, 2007 at 06:46:14AM -0700, David Brownell wrote: Has someone actually signed up to develop and maintain such a controller driver? If so, that would be a Fine Thing, but not one I've heard rumored before. I've assumed that the best we'd have is someone (Rodolfo?!) finally cleaning up a pxa27x version so it could be merged. That's what I wish to do... I wish cleaning up as much possible my pxa27x version af the driver so we can have something common where we can work on. Good! Sorry for the delay but in these next days I'll read back all your suggestions and I'll provide a new patch ASAP. However I would like avoiding to remove what I did on usb_ep_autoconfig() since I got functional g_ether device with RNDIS support... there could be another way to resolve this problem? I *really* don't want to change the whole gadget stack just to make one excessively quirky bit of hardware work with it ... when we know there's a simple solution that doesn't involve that. (Changing the whole gadget stack would involve retesting a boatload of systems... not all of which are easily tested. Plus, those changes would need far more review than they can get just now.) So for now, just do what I outlined before: set up the pxa27x UDC to resemble the pxa2[156] chips: a bunch of bulk endpoint with no altsettings and acting the same in all three hardware-supported configs. That will support RNDIS. In fact the only thing it _won't_ handle is the real CDC Ethernet mode. Once that's working, we can revisit the issue of how to configure endpoint hardware ... focussing on just that issue, and with enough flexibility that non-pxa27x hardware can also be addressed. - Dave - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-usb-devel] [PATCH] PXA27x UDC driver.
On Sat, Jun 30, 2007 at 06:46:14AM -0700, David Brownell wrote: > > Has someone actually signed up to develop and maintain such a > controller driver? If so, that would be a Fine Thing, but not > one I've heard rumored before. I've assumed that the best we'd > have is someone (Rodolfo?!) finally cleaning up a pxa27x version > so it could be merged. That's what I wish to do... I wish cleaning up as much possible my pxa27x version af the driver so we can have something common where we can work on. Sorry for the delay but in these next days I'll read back all your suggestions and I'll provide a new patch ASAP. However I would like avoiding to remove what I did on usb_ep_autoconfig() since I got functional g_ether device with RNDIS support... there could be another way to resolve this problem? Thanks a lot, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-usb-devel] [PATCH] PXA27x UDC driver.
On Sat, Jun 30, 2007 at 06:46:14AM -0700, David Brownell wrote: Has someone actually signed up to develop and maintain such a controller driver? If so, that would be a Fine Thing, but not one I've heard rumored before. I've assumed that the best we'd have is someone (Rodolfo?!) finally cleaning up a pxa27x version so it could be merged. That's what I wish to do... I wish cleaning up as much possible my pxa27x version af the driver so we can have something common where we can work on. Sorry for the delay but in these next days I'll read back all your suggestions and I'll provide a new patch ASAP. However I would like avoiding to remove what I did on usb_ep_autoconfig() since I got functional g_ether device with RNDIS support... there could be another way to resolve this problem? Thanks a lot, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-usb-devel] [PATCH] PXA27x UDC driver.
David Brownell wrote: > On Friday 29 June 2007, Dmitry Krivoschekov wrote: >> David Brownell wrote: >>> On Thursday 28 June 2007, Rodolfo Giometti wrote: >>> As suggest by Leo let me propose to you my new patch for PXA27x UDC support. Please, let me know what I have to do for kernel inclusion. :) >>> Let's start with *JUST* a driver, not trying to update everything >>> else in the USB Gadget stack so that it looks like it's designed >>> specifically to handle all of Intel's design botches related to >>> endpoint config ... and work worse for essentially everything else. >>> >>> (Unlike pretty much every other vendor, Intel wanted hardware config >>> management. It was unusably buggy in pxa21x/25x/26x, and not much >>> better in pxa27x.) >>> >>> >>> So in technical terms, and to repeat what I've said before: just >>> configure it to act more like a PXA 25x chip (no altsettings) and >>> get it so it passes all the tests [1], modulo errata which have no >>> workarounds >> Other options are: >> >> 1. pre-program endpoints so the setting covers all gadget >>configurations, it seems it's feasible. The only appreciable >>change is, CDC Ethernet config number should be 3 instead of 1. >>It should not break anything. > > ISTR looking at this in some detail a few years back, and > concluding that it wouldn't quite work. That was before > started to hear back from people that the pxa27x hardware > didn't really match its documentation, too ... > > >> 2. Implement a FAKE call of GET_CONFIGURATION command so upon >>gadget binding you can issue the command and program endpoints >>according to the received gadget configuration. > > That would involve *multiple* such fake calls. Not the most > elegant of solutions, but ISTR using it in some other context. > > But again, that presumes sane hardware, or at least hardware > that matches the docs. And people who've looked at this in > more pain than I have are consistent in telling me that pxa27x > endpoint configuration has problems that the docs and errata > do not describe. (If it were just one person, rather than four > different developers, I'd be somewhat skeptical.) Hence my > eventual conclusion (and advice) to just stop trying to use > that misfeature. > Actually it's nor a feature nor a misfeature, i.e. you can't just disable it, so you can't stop using it. The only default endpoint is ep0, other endpoints need to be configured. So, if you want to use some static endpoints setting, you will have to program this configuration and you can't avoid endpoint configuration stage. That is, you may always run into that configuration problems. Thankfully, personally I haven't faced the problems. I used number 1 option plus some logic that reshuffle ep list so appropriate endpoints to be chosen by usb_ep_autoconfig(). It's not a perfect solution but, it worked at least with three gadgets g_ether, g_serial, g_file_storage. To be honest I dislike the solution but it seems it's the only way to avoid changing of gadget files. If I could change gadget files, probably I'd modify gadgets ..._bind calls so on pxa27x they don't use usb_ep_autoconfig but just pass USB descriptor to device driver but device driver, in turn, configures the controller according to the descriptor. > >> Also, considering that PXA3XX processors include PXA27x-compatible >> USB device controller it makes sense to develop a driver that >> will support both processor families. Hopefully PXA3XX arch >> support will be merged some day (the arch support has been already >> submitted here, but I don't know about its current status). > > Has someone actually signed up to develop and maintain such a > controller driver? If so, that would be a Fine Thing, but not > one I've heard rumored before. I've assumed that the best we'd > have is someone (Rodolfo?!) finally cleaning up a pxa27x version > so it could be merged. > > If it was just a cleanup I think we would have the driver long time ago. Unless we decide to make some appropriate changes to gadget files, I doubt we will see the driver in mainline kernel. Regards, Dmitry - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-usb-devel] [PATCH] PXA27x UDC driver.
David Brownell wrote: On Friday 29 June 2007, Dmitry Krivoschekov wrote: David Brownell wrote: On Thursday 28 June 2007, Rodolfo Giometti wrote: As suggest by Leo let me propose to you my new patch for PXA27x UDC support. Please, let me know what I have to do for kernel inclusion. :) Let's start with *JUST* a driver, not trying to update everything else in the USB Gadget stack so that it looks like it's designed specifically to handle all of Intel's design botches related to endpoint config ... and work worse for essentially everything else. (Unlike pretty much every other vendor, Intel wanted hardware config management. It was unusably buggy in pxa21x/25x/26x, and not much better in pxa27x.) So in technical terms, and to repeat what I've said before: just configure it to act more like a PXA 25x chip (no altsettings) and get it so it passes all the tests [1], modulo errata which have no workarounds Other options are: 1. pre-program endpoints so the setting covers all gadget configurations, it seems it's feasible. The only appreciable change is, CDC Ethernet config number should be 3 instead of 1. It should not break anything. ISTR looking at this in some detail a few years back, and concluding that it wouldn't quite work. That was before started to hear back from people that the pxa27x hardware didn't really match its documentation, too ... 2. Implement a FAKE call of GET_CONFIGURATION command so upon gadget binding you can issue the command and program endpoints according to the received gadget configuration. That would involve *multiple* such fake calls. Not the most elegant of solutions, but ISTR using it in some other context. But again, that presumes sane hardware, or at least hardware that matches the docs. And people who've looked at this in more pain than I have are consistent in telling me that pxa27x endpoint configuration has problems that the docs and errata do not describe. (If it were just one person, rather than four different developers, I'd be somewhat skeptical.) Hence my eventual conclusion (and advice) to just stop trying to use that misfeature. Actually it's nor a feature nor a misfeature, i.e. you can't just disable it, so you can't stop using it. The only default endpoint is ep0, other endpoints need to be configured. So, if you want to use some static endpoints setting, you will have to program this configuration and you can't avoid endpoint configuration stage. That is, you may always run into that configuration problems. Thankfully, personally I haven't faced the problems. I used number 1 option plus some logic that reshuffle ep list so appropriate endpoints to be chosen by usb_ep_autoconfig(). It's not a perfect solution but, it worked at least with three gadgets g_ether, g_serial, g_file_storage. To be honest I dislike the solution but it seems it's the only way to avoid changing of gadget files. If I could change gadget files, probably I'd modify gadgets ..._bind calls so on pxa27x they don't use usb_ep_autoconfig but just pass USB descriptor to device driver but device driver, in turn, configures the controller according to the descriptor. Also, considering that PXA3XX processors include PXA27x-compatible USB device controller it makes sense to develop a driver that will support both processor families. Hopefully PXA3XX arch support will be merged some day (the arch support has been already submitted here, but I don't know about its current status). Has someone actually signed up to develop and maintain such a controller driver? If so, that would be a Fine Thing, but not one I've heard rumored before. I've assumed that the best we'd have is someone (Rodolfo?!) finally cleaning up a pxa27x version so it could be merged. If it was just a cleanup I think we would have the driver long time ago. Unless we decide to make some appropriate changes to gadget files, I doubt we will see the driver in mainline kernel. Regards, Dmitry - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] PXA27x UDC driver.
On Sunday 01 July 2007, Russell King - ARM Linux wrote:
> >
> > +#ifdef CONFIG_USB_GADGET_PXA27X
> > +#define DEV_CONFIG_SUBSET
> > +#endif
> > +
>
> Bad - can we not make this runtime dependent?
The #define controls which descriptors are statically linked
into every object. I suppose someone could submit a patch which
does that differently ... I'm not keen on having lots of "will
never use" data bloating any driver, but it could be moved to
__initdata, at the cost of adding code to kmalloc (and kfree)
all the individual descriptors. Restricting code bloat to init
sections would still be worse than no bloat...
> > +static void *pxa27x_ep_alloc_buffer(struct usb_ep *_ep, unsigned bytes,
> > + dma_addr_t * dma, unsigned int gfp_flags)
> > +{
> > + ...
>
> Err, no. There's a perfectly good replacement. DMA pools. Please use
> the provided infrastructure instead of inventing your own solution.
This was cloned from code which predates dma pools. :)
Regardless, I'm planning to remove that interface from the gadget stack.
It's utility is far exceeded by the number of controller driver bugs
in that area. Plus, if any gadget driver needs such a mechanism, the
dma_pool stuff that now exists could be called directly.
> Pointers which are cleared are set to NULL not zero. Zero is an integer.
> NULL is a pointer. Don't confuse the two.
ISTR this class of error would be reported by sparse ("make check").
- Dave
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] PXA27x UDC driver.
On Thu, Jun 28, 2007 at 12:36:20PM +0200, Rodolfo Giometti wrote:
> attached you can find new version of my patch for PXA27x UDC driver
> support against kernel 2.6.22-rc3 (but it applies also ro rc6).
>
> I'd like to know what I have to do in order to prepare this patch for
> kernel inclusion. It's time PXA27x has its UDC driver into linux! :)
We're moving the PXA kernel towards being able to be built to support
both PXA25x and PXA27x at the same time.
> diff --git a/arch/arm/mach-pxa/generic.c b/arch/arm/mach-pxa/generic.c
> index 64b08b7..7f390fd 100644
> --- a/arch/arm/mach-pxa/generic.c
> +++ b/arch/arm/mach-pxa/generic.c
> @@ -282,7 +282,11 @@ static struct resource pxa2xx_udc_resources[] = {
> static u64 udc_dma_mask = ~(u32)0;
>
> static struct platform_device udc_device = {
> +#ifdef CONFIG_PXA27x
> + .name = "pxa27x-udc",
> +#else
> .name = "pxa2xx-udc",
> +#endif
This precludes that aim.
> diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c
> index 325bf7c..d4f5870 100644
> --- a/drivers/usb/gadget/ether.c
> +++ b/drivers/usb/gadget/ether.c
> @@ -257,10 +257,6 @@ MODULE_PARM_DESC(host_addr, "Host Ethernet Address");
> #define DEV_CONFIG_CDC
> #endif
>
> -#ifdef CONFIG_USB_GADGET_PXA27X
> -#define DEV_CONFIG_CDC
> -#endif
> -
Good.
> #ifdef CONFIG_USB_GADGET_S3C2410
> #define DEV_CONFIG_CDC
> #endif
> @@ -292,6 +288,10 @@ MODULE_PARM_DESC(host_addr, "Host Ethernet Address");
> #define DEV_CONFIG_SUBSET
> #endif
>
> +#ifdef CONFIG_USB_GADGET_PXA27X
> +#define DEV_CONFIG_SUBSET
> +#endif
> +
Bad - can we not make this runtime dependent?
> +#ifdef CONFIG_EMBEDDED
> +/* few strings, and little code to use them */
> +#undef DEBUG
Very very silly. So, if you want to turn on debugging, and also select
some features which are only accessible with EMBEDDED enabled, you have
to edit this file. No. Get rid of this. If people want to get rid of
the space used by debugging, they can just turn debugging off. No need
to make it any harder than that.
> +/* PXA cache needs flushing with DMA I/O (it's dma-incoherent), but there's
> + * no device-affinity and the heap works perfectly well for i/o buffers.
> + * It wastes much less memory than dma_alloc_coherent() would, and even
> + * prevents cacheline (32 bytes wide) sharing problems.
> + */
> +static void *pxa27x_ep_alloc_buffer(struct usb_ep *_ep, unsigned bytes,
> + dma_addr_t * dma, unsigned int gfp_flags)
> +{
> + char *retval;
> +
> + retval = kmalloc(bytes, gfp_flags & ~(__GFP_DMA | __GFP_HIGHMEM));
> + if (retval)
> +#ifdef USE_DMA
> + *dma = virt_to_bus(retval);
> +#else
> + *dma = (dma_addr_t) ~0;
> +#endif
Err, no. There's a perfectly good replacement. DMA pools. Please use
the provided infrastructure instead of inventing your own solution.
> + create_file_error:
> + driver->unbind(>gadget);
> +
> + device_bind_error:
> + device_del(>gadget.dev);
> +
> + device_add_error:
> + dev->driver = 0;
> + dev->gadget.dev.driver = 0;
Pointers which are cleared are set to NULL not zero. Zero is an integer.
NULL is a pointer. Don't confuse the two.
> + /* insist on Intel/ARM/XScale */
> + asm("mrc%? p15, 0, %0, c0, c0":"=r"(chiprev));
> + if ((chiprev & CP15R0_VENDOR_MASK) != CP15R0_XSCALE_VALUE) {
> + printk(KERN_ERR "%s: not XScale!\n", driver_name);
> + return -ENODEV;
> + }
Please use:
if ((processor_id & CP15R0_VENDOR_MASK) != CP15R0_XSCALE_VALUE)
it's already in a variable which is exported, so there's no point not
using it. Alternatively, use:
chiprev = read_cpuid(CPUID_ID);
> +#ifdef DEBUG
> +
> +static const char *state_name[] = {
> + "EP0_IDLE",
> + "EP0_IN_DATA_PHASE", "EP0_OUT_DATA_PHASE",
> + "EP0_END_XFER", "EP0_STALL"
> +};
> +
> +#define DMSG(stuff...) printk(KERN_ERR "udc: " stuff)
Is pr_debug() buggy?
> +
> +#ifdef VERBOSE
> +#define UDC_DEBUG DBG_VERBOSE
> +#else
> +#define UDC_DEBUG DBG_NORMAL
> +#endif
> +
> +static void __attribute__ ((__unused__))
> +dump_udccr(const char *label)
> +{
> + u32 udccr = UDCCR;
> + DMSG("%s 0x%08x =%s%s%s%s%s%s%s%s%s%s, con=%d,inter=%d,altinter=%d\n",
> + label, udccr,
> + (udccr & UDCCR_OEN) ? " oen":"",
> + (udccr & UDCCR_AALTHNP) ? " aalthnp":"",
> + (udccr & UDCCR_AHNP) ? " rem" : "",
> + (udccr & UDCCR_BHNP) ? " rstir" : "",
> + (udccr & UDCCR_DWRE) ? " dwre" : "",
> + (udccr & UDCCR_SMAC) ? " smac" : "",
> + (udccr & UDCCR_EMCE) ? " emce" : "",
> + (udccr & UDCCR_UDR) ? " udr" : "",
> + (udccr & UDCCR_UDA) ? " uda" : "",
> + (udccr & UDCCR_UDE) ? " ude" : "",
> + (udccr & UDCCR_ACN) >> UDCCR_ACN_S,
> + (udccr & UDCCR_AIN) >> UDCCR_AIN_S,
> +
Re: [PATCH] PXA27x UDC driver.
On Thu, Jun 28, 2007 at 12:36:20PM +0200, Rodolfo Giometti wrote: > attached you can find new version of my patch for PXA27x UDC driver > support against kernel 2.6.22-rc3 (but it applies also ro rc6). > > I'd like to know what I have to do in order to prepare this patch for > kernel inclusion. It's time PXA27x has its UDC driver into linux! :) Aren't there about 300 different versions of the PXA27x gadget driver out there? - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] PXA27x UDC driver.
On Thu, Jun 28, 2007 at 12:36:20PM +0200, Rodolfo Giometti wrote: attached you can find new version of my patch for PXA27x UDC driver support against kernel 2.6.22-rc3 (but it applies also ro rc6). I'd like to know what I have to do in order to prepare this patch for kernel inclusion. It's time PXA27x has its UDC driver into linux! :) Aren't there about 300 different versions of the PXA27x gadget driver out there? - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] PXA27x UDC driver.
On Thu, Jun 28, 2007 at 12:36:20PM +0200, Rodolfo Giometti wrote:
attached you can find new version of my patch for PXA27x UDC driver
support against kernel 2.6.22-rc3 (but it applies also ro rc6).
I'd like to know what I have to do in order to prepare this patch for
kernel inclusion. It's time PXA27x has its UDC driver into linux! :)
We're moving the PXA kernel towards being able to be built to support
both PXA25x and PXA27x at the same time.
diff --git a/arch/arm/mach-pxa/generic.c b/arch/arm/mach-pxa/generic.c
index 64b08b7..7f390fd 100644
--- a/arch/arm/mach-pxa/generic.c
+++ b/arch/arm/mach-pxa/generic.c
@@ -282,7 +282,11 @@ static struct resource pxa2xx_udc_resources[] = {
static u64 udc_dma_mask = ~(u32)0;
static struct platform_device udc_device = {
+#ifdef CONFIG_PXA27x
+ .name = pxa27x-udc,
+#else
.name = pxa2xx-udc,
+#endif
This precludes that aim.
diff --git a/drivers/usb/gadget/ether.c b/drivers/usb/gadget/ether.c
index 325bf7c..d4f5870 100644
--- a/drivers/usb/gadget/ether.c
+++ b/drivers/usb/gadget/ether.c
@@ -257,10 +257,6 @@ MODULE_PARM_DESC(host_addr, Host Ethernet Address);
#define DEV_CONFIG_CDC
#endif
-#ifdef CONFIG_USB_GADGET_PXA27X
-#define DEV_CONFIG_CDC
-#endif
-
Good.
#ifdef CONFIG_USB_GADGET_S3C2410
#define DEV_CONFIG_CDC
#endif
@@ -292,6 +288,10 @@ MODULE_PARM_DESC(host_addr, Host Ethernet Address);
#define DEV_CONFIG_SUBSET
#endif
+#ifdef CONFIG_USB_GADGET_PXA27X
+#define DEV_CONFIG_SUBSET
+#endif
+
Bad - can we not make this runtime dependent?
+#ifdef CONFIG_EMBEDDED
+/* few strings, and little code to use them */
+#undef DEBUG
Very very silly. So, if you want to turn on debugging, and also select
some features which are only accessible with EMBEDDED enabled, you have
to edit this file. No. Get rid of this. If people want to get rid of
the space used by debugging, they can just turn debugging off. No need
to make it any harder than that.
+/* PXA cache needs flushing with DMA I/O (it's dma-incoherent), but there's
+ * no device-affinity and the heap works perfectly well for i/o buffers.
+ * It wastes much less memory than dma_alloc_coherent() would, and even
+ * prevents cacheline (32 bytes wide) sharing problems.
+ */
+static void *pxa27x_ep_alloc_buffer(struct usb_ep *_ep, unsigned bytes,
+ dma_addr_t * dma, unsigned int gfp_flags)
+{
+ char *retval;
+
+ retval = kmalloc(bytes, gfp_flags ~(__GFP_DMA | __GFP_HIGHMEM));
+ if (retval)
+#ifdef USE_DMA
+ *dma = virt_to_bus(retval);
+#else
+ *dma = (dma_addr_t) ~0;
+#endif
Err, no. There's a perfectly good replacement. DMA pools. Please use
the provided infrastructure instead of inventing your own solution.
+ create_file_error:
+ driver-unbind(dev-gadget);
+
+ device_bind_error:
+ device_del(dev-gadget.dev);
+
+ device_add_error:
+ dev-driver = 0;
+ dev-gadget.dev.driver = 0;
Pointers which are cleared are set to NULL not zero. Zero is an integer.
NULL is a pointer. Don't confuse the two.
+ /* insist on Intel/ARM/XScale */
+ asm(mrc%? p15, 0, %0, c0, c0:=r(chiprev));
+ if ((chiprev CP15R0_VENDOR_MASK) != CP15R0_XSCALE_VALUE) {
+ printk(KERN_ERR %s: not XScale!\n, driver_name);
+ return -ENODEV;
+ }
Please use:
if ((processor_id CP15R0_VENDOR_MASK) != CP15R0_XSCALE_VALUE)
it's already in a variable which is exported, so there's no point not
using it. Alternatively, use:
chiprev = read_cpuid(CPUID_ID);
+#ifdef DEBUG
+
+static const char *state_name[] = {
+ EP0_IDLE,
+ EP0_IN_DATA_PHASE, EP0_OUT_DATA_PHASE,
+ EP0_END_XFER, EP0_STALL
+};
+
+#define DMSG(stuff...) printk(KERN_ERR udc: stuff)
Is pr_debug() buggy?
+
+#ifdef VERBOSE
+#define UDC_DEBUG DBG_VERBOSE
+#else
+#define UDC_DEBUG DBG_NORMAL
+#endif
+
+static void __attribute__ ((__unused__))
+dump_udccr(const char *label)
+{
+ u32 udccr = UDCCR;
+ DMSG(%s 0x%08x =%s%s%s%s%s%s%s%s%s%s, con=%d,inter=%d,altinter=%d\n,
+ label, udccr,
+ (udccr UDCCR_OEN) ? oen:,
+ (udccr UDCCR_AALTHNP) ? aalthnp:,
+ (udccr UDCCR_AHNP) ? rem : ,
+ (udccr UDCCR_BHNP) ? rstir : ,
+ (udccr UDCCR_DWRE) ? dwre : ,
+ (udccr UDCCR_SMAC) ? smac : ,
+ (udccr UDCCR_EMCE) ? emce : ,
+ (udccr UDCCR_UDR) ? udr : ,
+ (udccr UDCCR_UDA) ? uda : ,
+ (udccr UDCCR_UDE) ? ude : ,
+ (udccr UDCCR_ACN) UDCCR_ACN_S,
+ (udccr UDCCR_AIN) UDCCR_AIN_S,
+ (udccr UDCCR_AAISN) UDCCR_AAISN_S );
+}
+
+static void __attribute__ ((__unused__))
+dump_udccsr0(const char *label)
+{
+ u32 udccsr0 = UDCCSR0;
+
+ DMSG(%s %s 0x%08x
Re: [PATCH] PXA27x UDC driver.
On Sunday 01 July 2007, Russell King - ARM Linux wrote:
+#ifdef CONFIG_USB_GADGET_PXA27X
+#define DEV_CONFIG_SUBSET
+#endif
+
Bad - can we not make this runtime dependent?
The #define controls which descriptors are statically linked
into every object. I suppose someone could submit a patch which
does that differently ... I'm not keen on having lots of will
never use data bloating any driver, but it could be moved to
__initdata, at the cost of adding code to kmalloc (and kfree)
all the individual descriptors. Restricting code bloat to init
sections would still be worse than no bloat...
+static void *pxa27x_ep_alloc_buffer(struct usb_ep *_ep, unsigned bytes,
+ dma_addr_t * dma, unsigned int gfp_flags)
+{
+ ...
Err, no. There's a perfectly good replacement. DMA pools. Please use
the provided infrastructure instead of inventing your own solution.
This was cloned from code which predates dma pools. :)
Regardless, I'm planning to remove that interface from the gadget stack.
It's utility is far exceeded by the number of controller driver bugs
in that area. Plus, if any gadget driver needs such a mechanism, the
dma_pool stuff that now exists could be called directly.
Pointers which are cleared are set to NULL not zero. Zero is an integer.
NULL is a pointer. Don't confuse the two.
ISTR this class of error would be reported by sparse (make check).
- Dave
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-usb-devel] [PATCH] PXA27x UDC driver.
On Thursday 28 June 2007, Andrew Morton wrote: > > +#undef DISABLE_TEST_MODE > > enabling DISABLE_TEST_MODE seens to enable test mode. Confused. Blame it on Intel. ISTR that early pxa2[156]x silicon had something called "test mode". And a boatload of errata, with a common thread in workarounds: using the "test mode" helped many things work better, although it was neither necessary nor sufficient. If one were to #define DISABLE_TEST_MODE, software could experiment with other workarounds ... and maybe be able to get the documented "double buffering" feature to work. Later silicon stopped documenting "test mode" as such, effectively making certain workarounds become the standard way to use the chip. Which doesn't explain why pxa270 code has pxa2[156]x devel hooks, but explains why pxa2[156]x code wants the hardware "test mode" to be enabled ... except during developer experiments. - Dave - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-usb-devel] [PATCH] PXA27x UDC driver.
On Friday 29 June 2007, Dmitry Krivoschekov wrote: > David Brownell wrote: > > On Thursday 28 June 2007, Rodolfo Giometti wrote: > > > >> As suggest by Leo let me propose to you my new patch for PXA27x UDC > >> support. > >> > >> Please, let me know what I have to do for kernel inclusion. :) > > > > Let's start with *JUST* a driver, not trying to update everything > > else in the USB Gadget stack so that it looks like it's designed > > specifically to handle all of Intel's design botches related to > > endpoint config ... and work worse for essentially everything else. > > > > (Unlike pretty much every other vendor, Intel wanted hardware config > > management. It was unusably buggy in pxa21x/25x/26x, and not much > > better in pxa27x.) > > > > > > So in technical terms, and to repeat what I've said before: just > > configure it to act more like a PXA 25x chip (no altsettings) and > > get it so it passes all the tests [1], modulo errata which have no > > workarounds > > Other options are: > > 1. pre-program endpoints so the setting covers all gadget >configurations, it seems it's feasible. The only appreciable >change is, CDC Ethernet config number should be 3 instead of 1. >It should not break anything. ISTR looking at this in some detail a few years back, and concluding that it wouldn't quite work. That was before started to hear back from people that the pxa27x hardware didn't really match its documentation, too ... > 2. Implement a FAKE call of GET_CONFIGURATION command so upon >gadget binding you can issue the command and program endpoints >according to the received gadget configuration. That would involve *multiple* such fake calls. Not the most elegant of solutions, but ISTR using it in some other context. But again, that presumes sane hardware, or at least hardware that matches the docs. And people who've looked at this in more pain than I have are consistent in telling me that pxa27x endpoint configuration has problems that the docs and errata do not describe. (If it were just one person, rather than four different developers, I'd be somewhat skeptical.) Hence my eventual conclusion (and advice) to just stop trying to use that misfeature. > Also, considering that PXA3XX processors include PXA27x-compatible > USB device controller it makes sense to develop a driver that > will support both processor families. Hopefully PXA3XX arch > support will be merged some day (the arch support has been already > submitted here, but I don't know about its current status). Has someone actually signed up to develop and maintain such a controller driver? If so, that would be a Fine Thing, but not one I've heard rumored before. I've assumed that the best we'd have is someone (Rodolfo?!) finally cleaning up a pxa27x version so it could be merged. - Dave > > > Regards, > Dmitry > > > ... then submit that. No epautoconfig updates, no > > patches to every gadget driver to cope with updated autoconfig. > > > > Once there's a basic working no-frills version merged, then we can > > talk about whether things in the rest of the stack should change > > to accomodate the bizarre concepts of this controller. > > > > - Dave > > > > > > [1] http://www.linux-usb.org/usbtest/ > > > - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] PXA27x UDC driver.
On Friday 29 June 2007, Rodolfo Giometti wrote: > On Thu, Jun 28, 2007 at 02:53:22PM -0700, David Brownell wrote: > > > > Let's start with *JUST* a driver, not trying to update everything > > else in the USB Gadget stack so that it looks like it's designed > > specifically to handle all of Intel's design botches related to > > endpoint config ... and work worse for essentially everything else. > > > > (Unlike pretty much every other vendor, Intel wanted hardware config > > management. It was unusably buggy in pxa21x/25x/26x, and not much > > better in pxa27x.) > > > > > > So in technical terms, and to repeat what I've said before: just > > configure it to act more like a PXA 25x chip (no altsettings) and > > get it so it passes all the tests [1], modulo errata which have no > > workarounds ... then submit that. No epautoconfig updates, no > > patches to every gadget driver to cope with updated autoconfig. > > This looks interesting... as you alredy told this driver derives from > an older one, I just maintained it till now. > > If I well understand I should remove usb_ep_autoconfig() and program > into the controller only one (the default) configuration. Is that > right? Other than the fact that "configuration" means something very specific in USB terms, so there would be three of them ... yes. That's what the PXA2[156]x UDC hardware does. > Currently I tested the driver only with ether gadget, but if I do as > above, should I get the driver working with all gadgets automagically? > :) There's not much point in merging a driver that only works with one of what are currently six gadget drivers ... others are on the way. Yes, the point of working more like PXA2[156]x hardware is that we know that kind of hardware setup works with everything ... while we know that every attempt to use the PXA27x magic has failed on some of those drivers. (By all reports, because of hardware bugs.) - Dave - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-usb-devel] [PATCH] PXA27x UDC driver.
On Friday 29 June 2007, Dmitry Krivoschekov wrote: David Brownell wrote: On Thursday 28 June 2007, Rodolfo Giometti wrote: As suggest by Leo let me propose to you my new patch for PXA27x UDC support. Please, let me know what I have to do for kernel inclusion. :) Let's start with *JUST* a driver, not trying to update everything else in the USB Gadget stack so that it looks like it's designed specifically to handle all of Intel's design botches related to endpoint config ... and work worse for essentially everything else. (Unlike pretty much every other vendor, Intel wanted hardware config management. It was unusably buggy in pxa21x/25x/26x, and not much better in pxa27x.) So in technical terms, and to repeat what I've said before: just configure it to act more like a PXA 25x chip (no altsettings) and get it so it passes all the tests [1], modulo errata which have no workarounds Other options are: 1. pre-program endpoints so the setting covers all gadget configurations, it seems it's feasible. The only appreciable change is, CDC Ethernet config number should be 3 instead of 1. It should not break anything. ISTR looking at this in some detail a few years back, and concluding that it wouldn't quite work. That was before started to hear back from people that the pxa27x hardware didn't really match its documentation, too ... 2. Implement a FAKE call of GET_CONFIGURATION command so upon gadget binding you can issue the command and program endpoints according to the received gadget configuration. That would involve *multiple* such fake calls. Not the most elegant of solutions, but ISTR using it in some other context. But again, that presumes sane hardware, or at least hardware that matches the docs. And people who've looked at this in more pain than I have are consistent in telling me that pxa27x endpoint configuration has problems that the docs and errata do not describe. (If it were just one person, rather than four different developers, I'd be somewhat skeptical.) Hence my eventual conclusion (and advice) to just stop trying to use that misfeature. Also, considering that PXA3XX processors include PXA27x-compatible USB device controller it makes sense to develop a driver that will support both processor families. Hopefully PXA3XX arch support will be merged some day (the arch support has been already submitted here, but I don't know about its current status). Has someone actually signed up to develop and maintain such a controller driver? If so, that would be a Fine Thing, but not one I've heard rumored before. I've assumed that the best we'd have is someone (Rodolfo?!) finally cleaning up a pxa27x version so it could be merged. - Dave Regards, Dmitry ... then submit that. No epautoconfig updates, no patches to every gadget driver to cope with updated autoconfig. Once there's a basic working no-frills version merged, then we can talk about whether things in the rest of the stack should change to accomodate the bizarre concepts of this controller. - Dave [1] http://www.linux-usb.org/usbtest/ - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] PXA27x UDC driver.
On Friday 29 June 2007, Rodolfo Giometti wrote: On Thu, Jun 28, 2007 at 02:53:22PM -0700, David Brownell wrote: Let's start with *JUST* a driver, not trying to update everything else in the USB Gadget stack so that it looks like it's designed specifically to handle all of Intel's design botches related to endpoint config ... and work worse for essentially everything else. (Unlike pretty much every other vendor, Intel wanted hardware config management. It was unusably buggy in pxa21x/25x/26x, and not much better in pxa27x.) So in technical terms, and to repeat what I've said before: just configure it to act more like a PXA 25x chip (no altsettings) and get it so it passes all the tests [1], modulo errata which have no workarounds ... then submit that. No epautoconfig updates, no patches to every gadget driver to cope with updated autoconfig. This looks interesting... as you alredy told this driver derives from an older one, I just maintained it till now. If I well understand I should remove usb_ep_autoconfig() and program into the controller only one (the default) configuration. Is that right? Other than the fact that configuration means something very specific in USB terms, so there would be three of them ... yes. That's what the PXA2[156]x UDC hardware does. Currently I tested the driver only with ether gadget, but if I do as above, should I get the driver working with all gadgets automagically? :) There's not much point in merging a driver that only works with one of what are currently six gadget drivers ... others are on the way. Yes, the point of working more like PXA2[156]x hardware is that we know that kind of hardware setup works with everything ... while we know that every attempt to use the PXA27x magic has failed on some of those drivers. (By all reports, because of hardware bugs.) - Dave - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-usb-devel] [PATCH] PXA27x UDC driver.
On Thursday 28 June 2007, Andrew Morton wrote: +#undef DISABLE_TEST_MODE enabling DISABLE_TEST_MODE seens to enable test mode. Confused. Blame it on Intel. ISTR that early pxa2[156]x silicon had something called test mode. And a boatload of errata, with a common thread in workarounds: using the test mode helped many things work better, although it was neither necessary nor sufficient. If one were to #define DISABLE_TEST_MODE, software could experiment with other workarounds ... and maybe be able to get the documented double buffering feature to work. Later silicon stopped documenting test mode as such, effectively making certain workarounds become the standard way to use the chip. Which doesn't explain why pxa270 code has pxa2[156]x devel hooks, but explains why pxa2[156]x code wants the hardware test mode to be enabled ... except during developer experiments. - Dave - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] PXA27x UDC driver.
Thanks for taking the lead on this! I can't wait to have a sane PXA27x
gadget driver in mainline.
On Thu, Jun 28, 2007 at 12:36:20PM +0200, Rodolfo Giometti wrote:
> +config USB_GADGET_PXA27X
> + boolean "PXA 27x"
> + depends on ARCH_PXA && PXA27x
> + help
> +Intel's PXA 27x series XScale processors include an integrated
XScale is a Marvell product now, not Intel. How about
XScale PXA 27x processors (from Marvell, formerly Intel) include ...
> +Say "y" to link the driver statically, or "m" to build a
> +dynamically linked module called "pxa2xx_udc" and force all
> +gadget drivers to also be dynamically linked.
Please copy the boilerplate on this:
To compile this driver as a module, choose M here: the
module will be called pxa27x_udc.
(Note the fixed module name, too.)
> + if (!_ep || !ep->desc) {
> + DMSG("%s, %s not enabled\n", __FUNCTION__,
> + _ep ? ep->ep.name : NULL);
I wouldn't pass NULL to a printf-format-string-using function, and ':'
would be a more traditional separator than ','. (Many instances of the
latter.)
> + if (dcsr & DCSR_BUSERR) {
> + DCSR(dmach) = DCSR_BUSERR;
> + printk(KERN_ERR " Buss Error\n");
Extra space after ", and Bus is misspelled. This printk should include
as much information about the error as reasonable.
> +static int pxa27x_ep_fifo_status(struct usb_ep *_ep)
> +{
> + struct pxa27x_ep *ep;
> +
> + ep = container_of(_ep, struct pxa27x_ep, ep);
No reason not to write
struct pxa27x_ep *ep = container_of(_ep, struct pxa27x_ep, ep);
> + while (((*ep->reg_udccsr) & UDCCSR_BNE) != 0)
> + (void)*ep->reg_udcdr;
That looks funky. On x86 I think you'd want a cpu_relax() in the loop
body, but I don't know if that's necessary on ARM. At the very least,
there's no reason to waste every other volatile read, so you should
remove the ->reg_udcdr read outside of the condition. Instead, the
standard seems to be
while (((*ep->reg_udccsr) & UDCCSR_BNE) != 0)
;
> + *ep->reg_udccsr = UDCCSR_PC | UDCCSR_FST | UDCCSR_TRN
> + | (ep->ep_type == USB_ENDPOINT_XFER_ISOC)
> + ? 0 : UDCCSR_SST;
More parentheses and/or indentation to make it clear how the ?: nests
with |.
> +#define NAME_SIZE 18
If this code isn't killed by David Brownell's comments, please either
make this a local variable or move the define to the top of the file
(and give it a name that describes what name it's the size of).
> + DMSG("udccsr=0x%8x, udcbcr=0x%8x, udcdr=0x%8x,"
> + "udccr0=0x%8x\n",
> + (unsigned)pxa_ep->reg_udccsr,
> + (unsigned)pxa_ep->reg_udcbcr,
> + (unsigned)pxa_ep->reg_udcdr, (unsigned)pxa_ep->reg_udccr);
Print pointers using %p.
> +#if 0
> + tmp |= (pxa_ep->interface << UDCCONR_IN_S) & UDCCONR_IN;
> + tmp |= (pxa_ep->aisn << UDCCONR_AISN_S) & UDCCONR_AISN;
> +#else
> + tmp |= (0 << UDCCONR_IN_S) & UDCCONR_IN;
> + tmp |= (0 << UDCCONR_AISN_S) & UDCCONR_AISN;
> +#endif
This stuff is wierd. It would be slightly more sane to just have the
code commented out, with a comment explaining why.
> + name = kmalloc(NAME_SIZE, GFP_KERNEL);
> + if (!name) {
> + printk(KERN_ERR "%s: Error\n", __FUNCTION__);
Useless printk. Should probably propagate ERR_PTR(-ENOMEM) back up the
call tree instead. (Several instances.)
> +udc_proc_read(char *page, char **start, off_t off, int count,
> + int *eof, void *_dev)
> +{
[snip]
> + t = scnprintf(next, size,
> + "uicr %02X.%02X, usir %02X.%02x, ufnr %02X\n",
> + UDCICR1, UDCICR0, UDCISR1, UDCISR0, UDCFNR);
> + size -= t;
> + next += t;
This code will get a lot simpler if you use seq_printf instead.
> +#define create_proc_files() \
> + create_proc_read_entry(proc_node_name, 0, NULL, udc_proc_read, dev)
> +#define remove_proc_files() \
> + remove_proc_entry(proc_node_name, NULL)
> +#else/* !UDC_PROC_FILE */
> +#define create_proc_files() do {} while (0)
> +#define remove_proc_files() do {} while (0)
> +#endif /* UDC_PROC_FILE */
The create_proc_files()/remove_proc_files() macros are nasty, and will
become unnecessary if you move the proc stuff to a separate file and use
Kconfig to optionally build it.
> +static DEVICE_ATTR(function, S_IRUGO, show_function, NULL);
Huh? This isn't used AFAICS.
> +static void udc_reinit(struct pxa27x_udc *dev)
> +{
> + u32 i;
No reason for this to be u32, use int. (Unless there's a significant
benefit in the generated code on ARM, perhaps.)
> + if (UDCCR & UDCCR_EMCE) {
> + printk(KERN_ERR
> +": There are error in configuration, udc disabled\n");
Add the device name or some other identifier here. And there's probably
a missing return or
Re: [linux-usb-devel] [PATCH] PXA27x UDC driver.
Rodolfo Giometti wrote: > On Thu, Jun 28, 2007 at 02:53:22PM -0700, David Brownell wrote: >> Let's start with *JUST* a driver, not trying to update everything >> else in the USB Gadget stack so that it looks like it's designed >> specifically to handle all of Intel's design botches related to >> endpoint config ... and work worse for essentially everything else. >> >> (Unlike pretty much every other vendor, Intel wanted hardware config >> management. It was unusably buggy in pxa21x/25x/26x, and not much >> better in pxa27x.) >> >> >> So in technical terms, and to repeat what I've said before: just >> configure it to act more like a PXA 25x chip (no altsettings) and >> get it so it passes all the tests [1], modulo errata which have no >> workarounds ... then submit that. No epautoconfig updates, no >> patches to every gadget driver to cope with updated autoconfig. > > This looks interesting... as you alredy told this driver derives from > an older one, I just maintained it till now. > > If I well understand I should remove usb_ep_autoconfig() and program > into the controller only one (the default) configuration. Is that > right? You should leave the usb_ep_autoconfig() as is, i.e. do not introduce any new parameters to it. > > Currently I tested the driver only with ether gadget, Why have you submitted the limited driver then? You should test the driver with all gadgets. Also, please report how it works in DMA and PIO modes. And what transfer rate the driver achives in both modes. > but if I do as > above, should I get the driver working with all gadgets automagically? Probably no, but technically there is no reason to not support all gadgets. BTW, I suggest that you wait until PXA3XX arch support will be merged into mainline kernel, then, pxa27x_udc-compatible driver will probably be submitted too. Taking into account that pxa27_udc have not been merged for years, I think extra 2-3 month is ok. Regards, Dmitry > :) > > Thanks a lot, > > Rodolfo > - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-usb-devel] [PATCH] PXA27x UDC driver.
David Brownell wrote: > On Thursday 28 June 2007, Rodolfo Giometti wrote: > >> As suggest by Leo let me propose to you my new patch for PXA27x UDC >> support. >> >> Please, let me know what I have to do for kernel inclusion. :) > > Let's start with *JUST* a driver, not trying to update everything > else in the USB Gadget stack so that it looks like it's designed > specifically to handle all of Intel's design botches related to > endpoint config ... and work worse for essentially everything else. > > (Unlike pretty much every other vendor, Intel wanted hardware config > management. It was unusably buggy in pxa21x/25x/26x, and not much > better in pxa27x.) > > > So in technical terms, and to repeat what I've said before: just > configure it to act more like a PXA 25x chip (no altsettings) and > get it so it passes all the tests [1], modulo errata which have no > workarounds Other options are: 1. pre-program endpoints so the setting covers all gadget configurations, it seems it's feasible. The only appreciable change is, CDC Ethernet config number should be 3 instead of 1. It should not break anything. 2. Implement a FAKE call of GET_CONFIGURATION command so upon gadget binding you can issue the command and program endpoints according to the received gadget configuration. Also, considering that PXA3XX processors include PXA27x-compatible USB device controller it makes sense to develop a driver that will support both processor families. Hopefully PXA3XX arch support will be merged some day (the arch support has been already submitted here, but I don't know about its current status). Regards, Dmitry > ... then submit that. No epautoconfig updates, no > patches to every gadget driver to cope with updated autoconfig. > > Once there's a basic working no-frills version merged, then we can > talk about whether things in the rest of the stack should change > to accomodate the bizarre concepts of this controller. > > - Dave > > > [1] http://www.linux-usb.org/usbtest/ > - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] PXA27x UDC driver.
On Thu, Jun 28, 2007 at 02:53:22PM -0700, David Brownell wrote: > > Let's start with *JUST* a driver, not trying to update everything > else in the USB Gadget stack so that it looks like it's designed > specifically to handle all of Intel's design botches related to > endpoint config ... and work worse for essentially everything else. > > (Unlike pretty much every other vendor, Intel wanted hardware config > management. It was unusably buggy in pxa21x/25x/26x, and not much > better in pxa27x.) > > > So in technical terms, and to repeat what I've said before: just > configure it to act more like a PXA 25x chip (no altsettings) and > get it so it passes all the tests [1], modulo errata which have no > workarounds ... then submit that. No epautoconfig updates, no > patches to every gadget driver to cope with updated autoconfig. This looks interesting... as you alredy told this driver derives from an older one, I just maintained it till now. If I well understand I should remove usb_ep_autoconfig() and program into the controller only one (the default) configuration. Is that right? Currently I tested the driver only with ether gadget, but if I do as above, should I get the driver working with all gadgets automagically? :) Thanks a lot, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] PXA27x UDC driver.
On Thu, Jun 28, 2007 at 02:53:22PM -0700, David Brownell wrote: Let's start with *JUST* a driver, not trying to update everything else in the USB Gadget stack so that it looks like it's designed specifically to handle all of Intel's design botches related to endpoint config ... and work worse for essentially everything else. (Unlike pretty much every other vendor, Intel wanted hardware config management. It was unusably buggy in pxa21x/25x/26x, and not much better in pxa27x.) So in technical terms, and to repeat what I've said before: just configure it to act more like a PXA 25x chip (no altsettings) and get it so it passes all the tests [1], modulo errata which have no workarounds ... then submit that. No epautoconfig updates, no patches to every gadget driver to cope with updated autoconfig. This looks interesting... as you alredy told this driver derives from an older one, I just maintained it till now. If I well understand I should remove usb_ep_autoconfig() and program into the controller only one (the default) configuration. Is that right? Currently I tested the driver only with ether gadget, but if I do as above, should I get the driver working with all gadgets automagically? :) Thanks a lot, Rodolfo -- GNU/Linux Solutions e-mail:[EMAIL PROTECTED] Linux Device Driver [EMAIL PROTECTED] Embedded Systems[EMAIL PROTECTED] UNIX programming phone: +39 349 2432127 - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-usb-devel] [PATCH] PXA27x UDC driver.
David Brownell wrote: On Thursday 28 June 2007, Rodolfo Giometti wrote: As suggest by Leo let me propose to you my new patch for PXA27x UDC support. Please, let me know what I have to do for kernel inclusion. :) Let's start with *JUST* a driver, not trying to update everything else in the USB Gadget stack so that it looks like it's designed specifically to handle all of Intel's design botches related to endpoint config ... and work worse for essentially everything else. (Unlike pretty much every other vendor, Intel wanted hardware config management. It was unusably buggy in pxa21x/25x/26x, and not much better in pxa27x.) So in technical terms, and to repeat what I've said before: just configure it to act more like a PXA 25x chip (no altsettings) and get it so it passes all the tests [1], modulo errata which have no workarounds Other options are: 1. pre-program endpoints so the setting covers all gadget configurations, it seems it's feasible. The only appreciable change is, CDC Ethernet config number should be 3 instead of 1. It should not break anything. 2. Implement a FAKE call of GET_CONFIGURATION command so upon gadget binding you can issue the command and program endpoints according to the received gadget configuration. Also, considering that PXA3XX processors include PXA27x-compatible USB device controller it makes sense to develop a driver that will support both processor families. Hopefully PXA3XX arch support will be merged some day (the arch support has been already submitted here, but I don't know about its current status). Regards, Dmitry ... then submit that. No epautoconfig updates, no patches to every gadget driver to cope with updated autoconfig. Once there's a basic working no-frills version merged, then we can talk about whether things in the rest of the stack should change to accomodate the bizarre concepts of this controller. - Dave [1] http://www.linux-usb.org/usbtest/ - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [linux-usb-devel] [PATCH] PXA27x UDC driver.
Rodolfo Giometti wrote: On Thu, Jun 28, 2007 at 02:53:22PM -0700, David Brownell wrote: Let's start with *JUST* a driver, not trying to update everything else in the USB Gadget stack so that it looks like it's designed specifically to handle all of Intel's design botches related to endpoint config ... and work worse for essentially everything else. (Unlike pretty much every other vendor, Intel wanted hardware config management. It was unusably buggy in pxa21x/25x/26x, and not much better in pxa27x.) So in technical terms, and to repeat what I've said before: just configure it to act more like a PXA 25x chip (no altsettings) and get it so it passes all the tests [1], modulo errata which have no workarounds ... then submit that. No epautoconfig updates, no patches to every gadget driver to cope with updated autoconfig. This looks interesting... as you alredy told this driver derives from an older one, I just maintained it till now. If I well understand I should remove usb_ep_autoconfig() and program into the controller only one (the default) configuration. Is that right? You should leave the usb_ep_autoconfig() as is, i.e. do not introduce any new parameters to it. Currently I tested the driver only with ether gadget, Why have you submitted the limited driver then? You should test the driver with all gadgets. Also, please report how it works in DMA and PIO modes. And what transfer rate the driver achives in both modes. but if I do as above, should I get the driver working with all gadgets automagically? Probably no, but technically there is no reason to not support all gadgets. BTW, I suggest that you wait until PXA3XX arch support will be merged into mainline kernel, then, pxa27x_udc-compatible driver will probably be submitted too. Taking into account that pxa27_udc have not been merged for years, I think extra 2-3 month is ok. Regards, Dmitry :) Thanks a lot, Rodolfo - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] PXA27x UDC driver.
Thanks for taking the lead on this! I can't wait to have a sane PXA27x
gadget driver in mainline.
On Thu, Jun 28, 2007 at 12:36:20PM +0200, Rodolfo Giometti wrote:
+config USB_GADGET_PXA27X
+ boolean PXA 27x
+ depends on ARCH_PXA PXA27x
+ help
+Intel's PXA 27x series XScale processors include an integrated
XScale is a Marvell product now, not Intel. How about
XScale PXA 27x processors (from Marvell, formerly Intel) include ...
+Say y to link the driver statically, or m to build a
+dynamically linked module called pxa2xx_udc and force all
+gadget drivers to also be dynamically linked.
Please copy the boilerplate on this:
To compile this driver as a module, choose M here: the
module will be called pxa27x_udc.
(Note the fixed module name, too.)
+ if (!_ep || !ep-desc) {
+ DMSG(%s, %s not enabled\n, __FUNCTION__,
+ _ep ? ep-ep.name : NULL);
I wouldn't pass NULL to a printf-format-string-using function, and ':'
would be a more traditional separator than ','. (Many instances of the
latter.)
+ if (dcsr DCSR_BUSERR) {
+ DCSR(dmach) = DCSR_BUSERR;
+ printk(KERN_ERR Buss Error\n);
Extra space after , and Bus is misspelled. This printk should include
as much information about the error as reasonable.
+static int pxa27x_ep_fifo_status(struct usb_ep *_ep)
+{
+ struct pxa27x_ep *ep;
+
+ ep = container_of(_ep, struct pxa27x_ep, ep);
No reason not to write
struct pxa27x_ep *ep = container_of(_ep, struct pxa27x_ep, ep);
+ while (((*ep-reg_udccsr) UDCCSR_BNE) != 0)
+ (void)*ep-reg_udcdr;
That looks funky. On x86 I think you'd want a cpu_relax() in the loop
body, but I don't know if that's necessary on ARM. At the very least,
there's no reason to waste every other volatile read, so you should
remove the -reg_udcdr read outside of the condition. Instead, the
standard seems to be
while (((*ep-reg_udccsr) UDCCSR_BNE) != 0)
;
+ *ep-reg_udccsr = UDCCSR_PC | UDCCSR_FST | UDCCSR_TRN
+ | (ep-ep_type == USB_ENDPOINT_XFER_ISOC)
+ ? 0 : UDCCSR_SST;
More parentheses and/or indentation to make it clear how the ?: nests
with |.
+#define NAME_SIZE 18
If this code isn't killed by David Brownell's comments, please either
make this a local variable or move the define to the top of the file
(and give it a name that describes what name it's the size of).
+ DMSG(udccsr=0x%8x, udcbcr=0x%8x, udcdr=0x%8x,
+ udccr0=0x%8x\n,
+ (unsigned)pxa_ep-reg_udccsr,
+ (unsigned)pxa_ep-reg_udcbcr,
+ (unsigned)pxa_ep-reg_udcdr, (unsigned)pxa_ep-reg_udccr);
Print pointers using %p.
+#if 0
+ tmp |= (pxa_ep-interface UDCCONR_IN_S) UDCCONR_IN;
+ tmp |= (pxa_ep-aisn UDCCONR_AISN_S) UDCCONR_AISN;
+#else
+ tmp |= (0 UDCCONR_IN_S) UDCCONR_IN;
+ tmp |= (0 UDCCONR_AISN_S) UDCCONR_AISN;
+#endif
This stuff is wierd. It would be slightly more sane to just have the
code commented out, with a comment explaining why.
+ name = kmalloc(NAME_SIZE, GFP_KERNEL);
+ if (!name) {
+ printk(KERN_ERR %s: Error\n, __FUNCTION__);
Useless printk. Should probably propagate ERR_PTR(-ENOMEM) back up the
call tree instead. (Several instances.)
+udc_proc_read(char *page, char **start, off_t off, int count,
+ int *eof, void *_dev)
+{
[snip]
+ t = scnprintf(next, size,
+ uicr %02X.%02X, usir %02X.%02x, ufnr %02X\n,
+ UDCICR1, UDCICR0, UDCISR1, UDCISR0, UDCFNR);
+ size -= t;
+ next += t;
This code will get a lot simpler if you use seq_printf instead.
+#define create_proc_files() \
+ create_proc_read_entry(proc_node_name, 0, NULL, udc_proc_read, dev)
+#define remove_proc_files() \
+ remove_proc_entry(proc_node_name, NULL)
+#else/* !UDC_PROC_FILE */
+#define create_proc_files() do {} while (0)
+#define remove_proc_files() do {} while (0)
+#endif /* UDC_PROC_FILE */
The create_proc_files()/remove_proc_files() macros are nasty, and will
become unnecessary if you move the proc stuff to a separate file and use
Kconfig to optionally build it.
+static DEVICE_ATTR(function, S_IRUGO, show_function, NULL);
Huh? This isn't used AFAICS.
+static void udc_reinit(struct pxa27x_udc *dev)
+{
+ u32 i;
No reason for this to be u32, use int. (Unless there's a significant
benefit in the generated code on ARM, perhaps.)
+ if (UDCCR UDCCR_EMCE) {
+ printk(KERN_ERR
+: There are error in configuration, udc disabled\n);
Add the device name or some other identifier here. And there's probably
a missing return or goto.
+#define CP15R0_VENDOR_MASK 0xe000
+
+#define CP15R0_XSCALE_VALUE 0x69054000 /* intel/arm/xscale */
Re: [PATCH] PXA27x UDC driver.
By the way, there are three or four versions of a pxa27x UDC
driver floating around; which one is this? One you updated?
It looks like it forked from the pxa2xx driver several years ago
but never got cleaned up ... e.g. it's a different controller,
so none of the comments relevant to earlier controller versions
(pxa250 rev a0 etc) could possibly apply, or even later ones
(like pxa255, effectively end-of-the-line for that UDC).
Quite a lot of the code issues in this driver are inherited from
some rather ancient code dating to ... 2.4.19-rmk7 kernels, if
I don't mis-recall. Some of them are even specific to the now
obsolete "Lubbock" reference hardware.
In short: whatever version of a pxa27x_udc driver gets submitted,
much cleanup seems *STILL* to be needed.
On Thursday 28 June 2007, Andrew Morton wrote:
> > --- a/arch/arm/mach-pxa/generic.c
> > +++ b/arch/arm/mach-pxa/generic.c
> > @@ -282,7 +282,11 @@ static struct resource pxa2xx_udc_resources[] = {
> > static u64 udc_dma_mask = ~(u32)0;
> >
> > static struct platform_device udc_device = {
> > +#ifdef CONFIG_PXA27x
> > + .name = "pxa27x-udc",
> > +#else
> > .name = "pxa2xx-udc",
> > +#endif
>
> This looks odd. The same driver presents itself under a different name
> according to a config option?
The PXA 27x platform devices should really have been in a
different file ... the PXA platform has been rather neglected,
since it effectively has no maintainer.
>
> > + * This UDC hardware wants to implement a bit too much USB protocol, so
> > + * it constrains the sorts of USB configuration change events that work.
> > + * The errata for these chips are misleading; some "fixed" bugs from
> > + * pxa250 a0/a1 b0/b1/b2 sure act like they're still there.
And *STILL* people carry around comments from the pxa2xx_udc
code (for "x" in 1, 6, and mostly 5).
ISTR that every time someone has submitted a pxa27x driver I've
had to ask that obsolete comments be removed. Still waiting...
> > +#undef USE_DMA
>
> So DMA is busted?
This driver was largely cut'n'paste from pxa2xx_udc, which
had to cope with errata which mostly meant that it couldn't
work. Then there were *DESIGN BUGS* in the DMA, making it
not worth using in the most significant cases ... even in
the later chip revisions where DMA would allegedly work.
(Which reminds me, maybe I should just rip DMA out of the
pxa2xx_udc code ... nobody seems to have picked it up and
finished it, so there's no point in keeping that around.)
I'm told that DMA works better in pxa27x, and that at least
one version of the pxa27x_udc driver enabled it by default.
(For TX, if not RX where it's most useful...) This would
seem not to be that version.
> > +#ifdef CONFIG_EMBEDDED
> > +/* few strings, and little code to use them */
> > +#undef DEBUG
> > +#undef UDC_PROC_FILE
> > +#endif
>
> gag, this looks like a mess. Thise sort of logic should be implemented in
> Kconfig, not in .c.
The debug file stuff *DOES* have a Kconfig hook...
> > +#ifndefenable_disconnect_irq
> > +#defineenable_disconnect_irq() do {} while (0)
> > +#definedisable_disconnect_irq()do {} while (0)
> > +#endif
>
> hm, OK, I guess this is somewhere where a macro is appropriate.
Not really. It's an artifact of a rather bizarre FPGA design
on the PXA25x reference design ("Lubbock"), which was very
gratuitously different from what Intel docs recommended. The
normal case is that there is a single IRQ.
That stuff is long gone even from pxa2xx_udc ... and in any case,
the VBUS irqs should not be called "connect/disconnect", it's
just confusing to do that.
> > + /* hardware sometimes neglects to tell
> > +* tell us about config change events,
> > +* so later ones may fail...
> > +*/
Curious ... did that pxa255 erratum transfer somehow to the
newer pxa27x silicon?
> > + WARN("config change %02x fail %d?\n",
> > +u.r.bRequest, i);
> > + return;
> > + /* TODO experiment: if has_cfr,
> > +* hardware didn't ACK; maybe we
> > +* could actually STALL!
> > +*/
Another comment that shouldn't be relevant on this driver.
ISTR that all PXA 27x chips have CFR. It was new in PXA255,
which is why the pxa2xx_udc had to cope with its absence.
> HEX_DISPLAY2 gets different treatment from HEX_DISPLAY here.
Yeah, but all that stuff was specific to the Lubbock platform.
Best to just remove it.
> > +#define WARN(stuff...) printk(KERN_WARNING "udc: " stuff)
> > +#define INFO(stuff...) printk(KERN_INFO "udc: " stuff)
>
> hrm. Why does every driver in the tree need to invent its own boilerplate
Re: [PATCH] PXA27x UDC driver.
On Thursday 28 June 2007, Rodolfo Giometti wrote: > As suggest by Leo let me propose to you my new patch for PXA27x UDC > support. > > Please, let me know what I have to do for kernel inclusion. :) Let's start with *JUST* a driver, not trying to update everything else in the USB Gadget stack so that it looks like it's designed specifically to handle all of Intel's design botches related to endpoint config ... and work worse for essentially everything else. (Unlike pretty much every other vendor, Intel wanted hardware config management. It was unusably buggy in pxa21x/25x/26x, and not much better in pxa27x.) So in technical terms, and to repeat what I've said before: just configure it to act more like a PXA 25x chip (no altsettings) and get it so it passes all the tests [1], modulo errata which have no workarounds ... then submit that. No epautoconfig updates, no patches to every gadget driver to cope with updated autoconfig. Once there's a basic working no-frills version merged, then we can talk about whether things in the rest of the stack should change to accomodate the bizarre concepts of this controller. - Dave [1] http://www.linux-usb.org/usbtest/ - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] PXA27x UDC driver.
On Thu, 28 Jun 2007 16:12:07 +0200
Rodolfo Giometti <[EMAIL PROTECTED]> wrote:
> On Thu, Jun 28, 2007 at 07:15:06PM +0800, Li Yang-r58472 wrote:
>
> > You should probably also cc: [EMAIL PROTECTED] and
> > David Brownell for UDC matters.
>
> As suggest by Leo let me propose to you my new patch for PXA27x UDC
> support.
>
> Please, let me know what I have to do for kernel inclusion. :)
>
Please feed it through the current version of scripts/checkpatch.pl and
think about fixing the large amount of trivia spew which ensues.
> diff --git a/arch/arm/mach-pxa/generic.c b/arch/arm/mach-pxa/generic.c
> index 64b08b7..7f390fd 100644
> --- a/arch/arm/mach-pxa/generic.c
> +++ b/arch/arm/mach-pxa/generic.c
> @@ -282,7 +282,11 @@ static struct resource pxa2xx_udc_resources[] = {
> static u64 udc_dma_mask = ~(u32)0;
>
> static struct platform_device udc_device = {
> +#ifdef CONFIG_PXA27x
> + .name = "pxa27x-udc",
> +#else
> .name = "pxa2xx-udc",
> +#endif
This looks odd. The same driver presents itself under a different name
according to a config option?
> @@ -0,0 +1,2395 @@
> +/*
> + * linux/drivers/usb/gadget/pxa27x_udc.c
> + * Intel PXA2xx and IXP4xx on-chip full speed USB device controllers
> + *
> + * Copyright (C) 2002 Intrinsyc, Inc. (Frank Becker)
> + * Copyright (C) 2003 Robert Schwebel, Pengutronix
> + * Copyright (C) 2003 Benedikt Spranger, Pengutronix
> + * Copyright (C) 2003 David Brownell
> + * Copyright (C) 2003 Joshua Wise
> + * Copyright (C) 2004 Intel Corporation
> + * Copyright (C) 2007 Rodolfo Giometti <[EMAIL PROTECTED]>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
> + *
> + */
> +
> +#undef DEBUG
Why? -DDEBUG is normally provided on the command line. If the user _did_
go and set DEBUG, he presumably wants DEBUG.
> +/* #define VERBOSE DBG_VERBOSE */
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include
> +#include
> +
> +#include
> +
> +/*
> + * This driver handles the USB Device Controller (UDC) in Intel's PXA 27x
> + * series processors.
> + * Such controller drivers work with a gadget driver. The gadget driver
> + * returns descriptors, implements configuration and data protocols used
> + * by the host to interact with this device, and allocates endpoints to
> + * the different protocol interfaces. The controller driver virtualizes
> + * usb hardware so that the gadget drivers will be more portable.
> + *
> + * This UDC hardware wants to implement a bit too much USB protocol, so
> + * it constrains the sorts of USB configuration change events that work.
> + * The errata for these chips are misleading; some "fixed" bugs from
> + * pxa250 a0/a1 b0/b1/b2 sure act like they're still there.
> + */
> +
> +#define DRIVER_VERSION "28-Jun-2007"
> +#define DRIVER_DESC "PXA 27x USB Device Controller driver"
> +
> +static const char driver_name[] = "pxa27x_udc";
> +
> +static const char ep0name[] = "ep0";
> +
> +#undef USE_DMA
So DMA is busted?
> +#undef DISABLE_TEST_MODE
enabling DISABLE_TEST_MODE seens to enable test mode. Confused.
> +#ifdef CONFIG_PROC_FS
> +#define UDC_PROC_FILE
> +#endif
> +
> +#include "pxa27x_udc.h"
> +
> +#ifdef CONFIG_EMBEDDED
> +/* few strings, and little code to use them */
> +#undef DEBUG
> +#undef UDC_PROC_FILE
> +#endif
gag, this looks like a mess. Thise sort of logic should be implemented in
Kconfig, not in .c.
> +#ifdef USE_DMA
> +static int use_dma = 1;
> +module_param(use_dma, bool, 0);
> +MODULE_PARM_DESC(use_dma, "true to use dma");
> +
> +static void dma_nodesc_handler(int dmach, void *_ep);
> +static void kick_dma(struct pxa27x_ep *ep, struct pxa27x_request *req);
> +
> +#define DMASTR " (dma support)"
> +
> +#else/* !USE_DMA */
> +#define DMASTR " (pio only)"
> +#endif
> +
> +#ifdef CONFIG_USB_PXA27X_SMALL
> +#define SIZE_STR " (small)"
> +#else
> +#define SIZE_STR ""
> +#endif
> +
>
RE: [PATCH] PXA27x UDC driver.
> -Original Message- > From: [EMAIL PROTECTED] > [mailto:[EMAIL PROTECTED] On Behalf Of Rodolfo Giometti > Sent: Thursday, June 28, 2007 6:36 PM > To: [EMAIL PROTECTED] > Cc: linux-kernel@vger.kernel.org; Andrew Morton > Subject: [PATCH] PXA27x UDC driver. > > Hello, > > attached you can find new version of my patch for PXA27x UDC driver > support against kernel 2.6.22-rc3 (but it applies also ro rc6). > > I'd like to know what I have to do in order to prepare this patch for > kernel inclusion. It's time PXA27x has its UDC driver into linux! :) > > Thanks for your suggestions, You should probably also cc: [EMAIL PROTECTED] and David Brownell for UDC matters. - Leo - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] PXA27x UDC driver.
-Original Message- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] On Behalf Of Rodolfo Giometti Sent: Thursday, June 28, 2007 6:36 PM To: [EMAIL PROTECTED] Cc: linux-kernel@vger.kernel.org; Andrew Morton Subject: [PATCH] PXA27x UDC driver. Hello, attached you can find new version of my patch for PXA27x UDC driver support against kernel 2.6.22-rc3 (but it applies also ro rc6). I'd like to know what I have to do in order to prepare this patch for kernel inclusion. It's time PXA27x has its UDC driver into linux! :) Thanks for your suggestions, You should probably also cc: [EMAIL PROTECTED] and David Brownell for UDC matters. - Leo - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] PXA27x UDC driver.
On Thu, 28 Jun 2007 16:12:07 +0200
Rodolfo Giometti [EMAIL PROTECTED] wrote:
On Thu, Jun 28, 2007 at 07:15:06PM +0800, Li Yang-r58472 wrote:
You should probably also cc: [EMAIL PROTECTED] and
David Brownell for UDC matters.
As suggest by Leo let me propose to you my new patch for PXA27x UDC
support.
Please, let me know what I have to do for kernel inclusion. :)
Please feed it through the current version of scripts/checkpatch.pl and
think about fixing the large amount of trivia spew which ensues.
diff --git a/arch/arm/mach-pxa/generic.c b/arch/arm/mach-pxa/generic.c
index 64b08b7..7f390fd 100644
--- a/arch/arm/mach-pxa/generic.c
+++ b/arch/arm/mach-pxa/generic.c
@@ -282,7 +282,11 @@ static struct resource pxa2xx_udc_resources[] = {
static u64 udc_dma_mask = ~(u32)0;
static struct platform_device udc_device = {
+#ifdef CONFIG_PXA27x
+ .name = pxa27x-udc,
+#else
.name = pxa2xx-udc,
+#endif
This looks odd. The same driver presents itself under a different name
according to a config option?
@@ -0,0 +1,2395 @@
+/*
+ * linux/drivers/usb/gadget/pxa27x_udc.c
+ * Intel PXA2xx and IXP4xx on-chip full speed USB device controllers
+ *
+ * Copyright (C) 2002 Intrinsyc, Inc. (Frank Becker)
+ * Copyright (C) 2003 Robert Schwebel, Pengutronix
+ * Copyright (C) 2003 Benedikt Spranger, Pengutronix
+ * Copyright (C) 2003 David Brownell
+ * Copyright (C) 2003 Joshua Wise
+ * Copyright (C) 2004 Intel Corporation
+ * Copyright (C) 2007 Rodolfo Giometti [EMAIL PROTECTED]
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ *
+ */
+
+#undef DEBUG
Why? -DDEBUG is normally provided on the command line. If the user _did_
go and set DEBUG, he presumably wants DEBUG.
+/* #define VERBOSE DBG_VERBOSE */
+
+#include linux/module.h
+#include linux/kernel.h
+#include linux/ioport.h
+#include linux/types.h
+#include linux/version.h
+#include linux/errno.h
+#include linux/delay.h
+#include linux/sched.h
+#include linux/slab.h
+#include linux/init.h
+#include linux/timer.h
+#include linux/list.h
+#include linux/interrupt.h
+#include linux/proc_fs.h
+#include linux/mm.h
+#include linux/device.h
+#include linux/dma-mapping.h
+#include linux/platform_device.h
+
+#include asm/byteorder.h
+#include asm/dma.h
+#include asm/io.h
+#include asm/irq.h
+#include asm/system.h
+#include asm/mach-types.h
+#include asm/unaligned.h
+#include asm/hardware.h
+#include asm/arch/pxa-regs.h
+
+#include linux/usb/ch9.h
+#include linux/usb_gadget.h
+
+#include asm/arch/udc.h
+
+/*
+ * This driver handles the USB Device Controller (UDC) in Intel's PXA 27x
+ * series processors.
+ * Such controller drivers work with a gadget driver. The gadget driver
+ * returns descriptors, implements configuration and data protocols used
+ * by the host to interact with this device, and allocates endpoints to
+ * the different protocol interfaces. The controller driver virtualizes
+ * usb hardware so that the gadget drivers will be more portable.
+ *
+ * This UDC hardware wants to implement a bit too much USB protocol, so
+ * it constrains the sorts of USB configuration change events that work.
+ * The errata for these chips are misleading; some fixed bugs from
+ * pxa250 a0/a1 b0/b1/b2 sure act like they're still there.
+ */
+
+#define DRIVER_VERSION 28-Jun-2007
+#define DRIVER_DESC PXA 27x USB Device Controller driver
+
+static const char driver_name[] = pxa27x_udc;
+
+static const char ep0name[] = ep0;
+
+#undef USE_DMA
So DMA is busted?
+#undef DISABLE_TEST_MODE
enabling DISABLE_TEST_MODE seens to enable test mode. Confused.
+#ifdef CONFIG_PROC_FS
+#define UDC_PROC_FILE
+#endif
+
+#include pxa27x_udc.h
+
+#ifdef CONFIG_EMBEDDED
+/* few strings, and little code to use them */
+#undef DEBUG
+#undef UDC_PROC_FILE
+#endif
gag, this looks like a mess. Thise sort of logic should be implemented in
Kconfig, not in .c.
+#ifdef USE_DMA
+static int use_dma = 1;
+module_param(use_dma, bool, 0);
+MODULE_PARM_DESC(use_dma, true to use dma);
+
+static void dma_nodesc_handler(int dmach, void *_ep);
+static void kick_dma(struct pxa27x_ep *ep, struct pxa27x_request *req);
+
+#define
Re: [PATCH] PXA27x UDC driver.
On Thursday 28 June 2007, Rodolfo Giometti wrote: As suggest by Leo let me propose to you my new patch for PXA27x UDC support. Please, let me know what I have to do for kernel inclusion. :) Let's start with *JUST* a driver, not trying to update everything else in the USB Gadget stack so that it looks like it's designed specifically to handle all of Intel's design botches related to endpoint config ... and work worse for essentially everything else. (Unlike pretty much every other vendor, Intel wanted hardware config management. It was unusably buggy in pxa21x/25x/26x, and not much better in pxa27x.) So in technical terms, and to repeat what I've said before: just configure it to act more like a PXA 25x chip (no altsettings) and get it so it passes all the tests [1], modulo errata which have no workarounds ... then submit that. No epautoconfig updates, no patches to every gadget driver to cope with updated autoconfig. Once there's a basic working no-frills version merged, then we can talk about whether things in the rest of the stack should change to accomodate the bizarre concepts of this controller. - Dave [1] http://www.linux-usb.org/usbtest/ - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH] PXA27x UDC driver.
By the way, there are three or four versions of a pxa27x UDC
driver floating around; which one is this? One you updated?
It looks like it forked from the pxa2xx driver several years ago
but never got cleaned up ... e.g. it's a different controller,
so none of the comments relevant to earlier controller versions
(pxa250 rev a0 etc) could possibly apply, or even later ones
(like pxa255, effectively end-of-the-line for that UDC).
Quite a lot of the code issues in this driver are inherited from
some rather ancient code dating to ... 2.4.19-rmk7 kernels, if
I don't mis-recall. Some of them are even specific to the now
obsolete Lubbock reference hardware.
In short: whatever version of a pxa27x_udc driver gets submitted,
much cleanup seems *STILL* to be needed.
On Thursday 28 June 2007, Andrew Morton wrote:
--- a/arch/arm/mach-pxa/generic.c
+++ b/arch/arm/mach-pxa/generic.c
@@ -282,7 +282,11 @@ static struct resource pxa2xx_udc_resources[] = {
static u64 udc_dma_mask = ~(u32)0;
static struct platform_device udc_device = {
+#ifdef CONFIG_PXA27x
+ .name = pxa27x-udc,
+#else
.name = pxa2xx-udc,
+#endif
This looks odd. The same driver presents itself under a different name
according to a config option?
The PXA 27x platform devices should really have been in a
different file ... the PXA platform has been rather neglected,
since it effectively has no maintainer.
+ * This UDC hardware wants to implement a bit too much USB protocol, so
+ * it constrains the sorts of USB configuration change events that work.
+ * The errata for these chips are misleading; some fixed bugs from
+ * pxa250 a0/a1 b0/b1/b2 sure act like they're still there.
And *STILL* people carry around comments from the pxa2xx_udc
code (for x in 1, 6, and mostly 5).
ISTR that every time someone has submitted a pxa27x driver I've
had to ask that obsolete comments be removed. Still waiting...
+#undef USE_DMA
So DMA is busted?
This driver was largely cut'n'paste from pxa2xx_udc, which
had to cope with errata which mostly meant that it couldn't
work. Then there were *DESIGN BUGS* in the DMA, making it
not worth using in the most significant cases ... even in
the later chip revisions where DMA would allegedly work.
(Which reminds me, maybe I should just rip DMA out of the
pxa2xx_udc code ... nobody seems to have picked it up and
finished it, so there's no point in keeping that around.)
I'm told that DMA works better in pxa27x, and that at least
one version of the pxa27x_udc driver enabled it by default.
(For TX, if not RX where it's most useful...) This would
seem not to be that version.
+#ifdef CONFIG_EMBEDDED
+/* few strings, and little code to use them */
+#undef DEBUG
+#undef UDC_PROC_FILE
+#endif
gag, this looks like a mess. Thise sort of logic should be implemented in
Kconfig, not in .c.
The debug file stuff *DOES* have a Kconfig hook...
+#ifndefenable_disconnect_irq
+#defineenable_disconnect_irq() do {} while (0)
+#definedisable_disconnect_irq()do {} while (0)
+#endif
hm, OK, I guess this is somewhere where a macro is appropriate.
Not really. It's an artifact of a rather bizarre FPGA design
on the PXA25x reference design (Lubbock), which was very
gratuitously different from what Intel docs recommended. The
normal case is that there is a single IRQ.
That stuff is long gone even from pxa2xx_udc ... and in any case,
the VBUS irqs should not be called connect/disconnect, it's
just confusing to do that.
+ /* hardware sometimes neglects to tell
+* tell us about config change events,
+* so later ones may fail...
+*/
Curious ... did that pxa255 erratum transfer somehow to the
newer pxa27x silicon?
+ WARN(config change %02x fail %d?\n,
+u.r.bRequest, i);
+ return;
+ /* TODO experiment: if has_cfr,
+* hardware didn't ACK; maybe we
+* could actually STALL!
+*/
Another comment that shouldn't be relevant on this driver.
ISTR that all PXA 27x chips have CFR. It was new in PXA255,
which is why the pxa2xx_udc had to cope with its absence.
HEX_DISPLAY2 gets different treatment from HEX_DISPLAY here.
Yeah, but all that stuff was specific to the Lubbock platform.
Best to just remove it.
+#define WARN(stuff...) printk(KERN_WARNING udc: stuff)
+#define INFO(stuff...) printk(KERN_INFO udc: stuff)
hrm. Why does every driver in the tree need to invent its own boilerplate
infrastructure?
can we use dev_warn() here or something?
That stuff dates from 2.4.19-rmk7 kernel
