Re: [PATCH] PXA27x UDC driver.

2007-07-10 Thread Lothar Wassmann 
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.

2007-07-10 Thread Rodolfo Giometti 
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: [PATCH] PXA27x UDC driver.

2007-07-10 Thread Rodolfo Giometti 
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.

2007-07-10 Thread Lothar Wassmann 
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.

2007-07-01 Thread David Brownell 
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.

2007-07-01 Thread Russell King - ARM Linux 
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.

2007-07-01 Thread Lennert Buytenhek 
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.

2007-07-01 Thread Lennert Buytenhek 
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.

2007-07-01 Thread Russell King - ARM Linux 
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.

2007-07-01 Thread David Brownell 
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.

2007-06-30 Thread David Brownell 
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: [PATCH] PXA27x UDC driver.

2007-06-30 Thread David Brownell 
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: [PATCH] PXA27x UDC driver.

2007-06-29 Thread Andy Isaacson 
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: [PATCH] PXA27x UDC driver.

2007-06-29 Thread Rodolfo Giometti 
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.

2007-06-29 Thread Rodolfo Giometti 
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.

2007-06-29 Thread Andy Isaacson 
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.

2007-06-28 Thread David Brownell 
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.

2007-06-28 Thread David Brownell 
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.

2007-06-28 Thread Andrew Morton 
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.

2007-06-28 Thread Li Yang-r58472 
> -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.

2007-06-28 Thread Li Yang-r58472 
 -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.

2007-06-28 Thread Andrew Morton 
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.

2007-06-28 Thread David Brownell 
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.

2007-06-28 Thread David Brownell 
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