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,
Rodolfo
-- 
GNU/Linux Solutions                  e-mail:    giometti@enneenne.com

Linux Device Driver                             giometti@gnudd.com

Embedded Systems                     		giometti@linux.it

UNIX programming                     phone:     +39 349 2432127

We're moving the PXA kernel towards being able to be built to support
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
Err, no.  There's a perfectly good replacement.  DMA pools.  Please use
Pointers which are cleared are set to NULL not zero.  Zero is an integer.
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:
-

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
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
ISTR this class of error would be reported by sparse ("make check").
- Dave

-

Aren't there about 300 different versions of the PXA27x gadget

driver out there?

-

Thanks for taking the lead on this!  I can't wait to have a sane PXA27x

gadget driver in mainline.
XScale is a Marvell product now, not Intel.  How about
Please copy the boilerplate on this:
          To compile this driver as a module, choose M here: the

          module will be called pxa27x_udc.
I wouldn't pass NULL to a printf-format-string-using function, and ':'

would be a more traditional separator than ','.  (Many instances of the
Extra space after ", and Bus is misspelled.  This printk should include
No reason not to write
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)
More parentheses and/or indentation to make it clear how the ?: nests
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
This stuff is wierd.  It would be slightly more sane to just have the
Useless printk.  Should probably propagate ERR_PTR(-ENOMEM) back up the
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
No reason for this to be u32, use int.  (Unless there's a significant
Add the device name or some other identifier here.  And there's probably
This define definitely doesn't belong here, and I don't think it belongs

in this header file at all -- or if it does, it needs a less generic

No space                                          ^ here.
-andy

-

You should probably also cc: linux-usb-devel@lists.sourceforge.net and

David Brownell for UDC matters.
- Leo

-

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. :)
Thanks,
Rodolfo
P.S. Please, in the replay also add:

linux-arm-kernel@lists.arm.linux.org.uk, linux-kernel@vger.kernel.org

and Andrew Morton <akpm@linux-foundation.org> where I already sent the

patch.
-- 
GNU/Linux Solutions                  e-mail:    giometti@enneenne.com

Linux Device Driver                             giometti@gnudd.com

Embedded Systems                     		giometti@linux.it

UNIX programming                     phone:     +39 349 2432127

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/

-

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,
-

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
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
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:    giometti@enneenne.com

Linux Device Driver                             giometti@gnudd.com

Embedded Systems                     		giometti@linux.it

UNIX programming                     phone:     +39 349 2432127

-

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
-

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.

-

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

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
-

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
Thanks a lot,
Rodolfo
-- 
GNU/Linux Solutions                  e-mail:    giometti@enneenne.com

Linux Device Driver                             giometti@gnudd.com

Embedded Systems                     		giometti@linux.it

UNIX programming                     phone:     +39 349 2432127

-

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
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
-

You should leave the usb_ep_autoconfig() as is, i.e. do not introduce

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

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,
-

On Thu, 28 Jun 2007 16:12:07 +0200
Please feed it through the current version of scripts/checkpatch.pl and
This looks odd.  The same driver presents itself under a different name
Why?  -DDEBUG is normally provided on the command line.  If the user _did_
gag, this looks like a mess.  Thise sort of logic should be implemented in
Unneeded initialisation (checkpatch should catch this)
kernel comments normally look like
/*

 * endpoint related parts of the api to the usb controller hardware,

 * used by gadget driver; and the inner talker-to-hardware core.

 *
We'd normally put braces around the "if" clause if there are braces around
We'd normally lay this out as
	if (ep->ep_type != USB_ENDPOINT_XFER_BULK &&

	    desc->bmAttributes != USB_ENDPOINT_XFER_INT) {
that expression is quite hard to read.
	/* hardware _could_ do smaller, but driver doesn't */

	if ((desc->bmAttributes == USB_ENDPOINT_XFER_BULK &&

		le16_to_cpu(desc->wMaxPacketSize) != BULK_FIFO_SIZE) ||

	    !desc->wMaxPacketSize) {
this looks fishy.  local_irq_disable() only provides cpu-local protection.

Is this code SMP-correct?  What's happening here?  A comment is needed
There's no point in inlining this.  With only one callsite the compiler

will inline it anyway.  If we grow a second callsite, this fucntion is too

large to inline.
Use min_t, not an open-coded cast.
Please only use macros when you *must* use macros.  When for some reason

you cannot use C.  This is not such a case.  IOW, convert to static
We normally leave zero blank lines between the } and the EXPORT_SYMBOL
Probably too big to inline.
what's that label doing all the way over there.  It makes it rather hard to
Burying returns deep inside large functions is unpopular: it can easily

lead to resource leaks and locking errors as the code evolves.  It makes
#define INT_FIFO_SIZE	8U
but this doesn't.  So we risk getting unused-var warnings in caller...
[ message continues ]

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:    giometti@enneenne.com

Linux Device Driver                             giometti@gnudd.com

Embedded Systems                     		giometti@linux.it

UNIX programming                     phone:     +39 349 2432127

-

ioremap() the memory area for the UDC registers and use an __iomem

cookie.
Lothar Wassmann

--

___________________________________________________________
Ka-Ro electronics GmbH | Pascalstra

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

-

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.
The PXA 27x platform devices should really have been in a

different file ... the PXA platform has been rather neglected,
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
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
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 an...
[ message continues ]