PXA27x UDC driver.

-----Original Message-----
From: [email protected] [mailto:[email protected]] On Behalf Of Rodolfo

Sent: Thursday, June 28, 2007 6:36 PM
To: [email protected]
Cc: [email protected]; 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.

Quoted 2 lines · 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. :)

arch/arm/mach-pxa/generic.c +19 -0

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

arch/arm/mach-pxa/generic.c +49 -0

+/* #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.

arch/arm/mach-pxa/generic.c +17 -0

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

arch/arm/mach-pxa/generic.c +4 -0

+module_param(fifo_mode, ushort, 0);
+MODULE_PARM_DESC(fifo_mode, "pxa27x udc fifo mode");
+#endif
+

arch/arm/mach-pxa/generic.c +24 -0

+
+static void pxa27x_ep_fifo_flush(struct usb_ep *ep);
+static void nuke(struct pxa27x_ep *, int status);
+
+/* one GPIO should control a D+ pullup, so host sees this device (or not) */
+static void pullup_off(void)
+{
+	struct pxa2xx_udc_mach_info *mach = the_controller->mach;
+
+	if (mach->gpio_pullup)
+		udc_gpio_set(mach->gpio_pullup, 0);
+	else if (mach->udc_command)
+		mach->udc_command(PXA2XX_UDC_CMD_DISCONNECT);
+}
+
+static void pullup_on(void)
+{
+	struct pxa2xx_udc_mach_info *mach = the_controller->mach;
+
+	if (mach->gpio_pullup)
+		udc_gpio_set(mach->gpio_pullup, 1);
+	else if (mach->udc_command)
+		mach->udc_command(PXA2XX_UDC_CMD_CONNECT);
+}

arch/arm/mach-pxa/generic.c +61 -0

+		DMSG("%s, bad %s maxpacket\n", __FUNCTION__, _ep->name);
+		return -ERANGE;
+	}
+
+	dev = ep->dev;
+	if (!dev->driver || dev->gadget.speed == USB_SPEED_UNKNOWN) {
+		DMSG("%s, bogus device state\n", __FUNCTION__);
+		return -ESHUTDOWN;
+	}
+
+	ep->desc = desc;
+	ep->dma = -1;
+	ep->stopped = 0;
+	ep->pio_irqs = ep->dma_irqs = 0;
+	ep->ep.maxpacket = le16_to_cpu(desc->wMaxPacketSize);
+
+	/* flush fifo (mostly for OUT buffers) */
+	pxa27x_ep_fifo_flush(_ep);
+
+	/* ... reset halt state too, if we could ... */
+
+#ifdef USE_DMA
+	/* for (some) bulk and ISO endpoints, try to get a DMA channel and
+	 * bind it to the endpoint.  otherwise use PIO. 
+	 */
+	DMSG("%s: called attributes=%d\n", __FUNCTION__, ep->ep_type);
+	switch (ep->ep_type) {
+	case USB_ENDPOINT_XFER_ISOC:
+		if (le16_to_cpu(desc->wMaxPacketSize) % 32)
+			break;
+		/* fall through */
+	case USB_ENDPOINT_XFER_BULK:
+		if (!use_dma || !ep->reg_drcmr)
+			break;
+		ep->dma = pxa_request_dma((char *)_ep->name,
+				  (le16_to_cpu(desc->wMaxPacketSize) > 64)
+					  ? DMA_PRIO_MEDIUM	/* some iso */
+					  : DMA_PRIO_LOW,
+					  dma_nodesc_handler, ep);
+		if (ep->dma >= 0) {
+			*ep->reg_drcmr = DRCMR_MAPVLD | ep->dma;
+			DMSG("%s using dma%d\n", _ep->name, ep->dma);
+		}
+	default:
+		break;
+	}
+#endif
+	DBG(DBG_VERBOSE, "enabled %s\n", _ep->name);
+	return 0;
+}
+
+/*-------------------------------------------------------------------------*/
+
+/* for the pxa27x, these can just wrap kmalloc/kfree.  gadget drivers
+ * must still pass correctly initialized endpoints, since other controller
+ * drivers may care about how it's currently set up (dma issues etc).
+ */
+
+/*
+ * 	pxa27x_ep_alloc_request - allocate a request data structure
+ */

arch/arm/mach-pxa/generic.c +31 -0

+{
+	u32 *buf;
+	int length, count, remain;
+
+	buf = (u32 *) (req->req.buf + req->req.actual);
+	prefetch(buf);
+
+	/* how big will this packet be? */
+	length = min(req->req.length - req->req.actual, max);
+	req->req.actual += length;
+
+	remain = length & 0x3;
+	count = length & ~(0x3);
+
+	while (likely(count)) {
+		*uddr = *buf++;
+		count -= 4;
+	}
+
+	if (remain) {
+		volatile u8 *reg = (u8 *) uddr;
+		char *rd = (u8 *) buf;
+
+		while (remain--) {
+			*reg = *rd++;
+		}
+	}
+
+	return length;
+}
+
 

arch/arm/mach-pxa/generic.c +9 -0

+static inline void validate_fifo_size(struct pxa27x_ep *pxa_ep, u8 bmAttributes)
+{
+	switch (bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) {
+	case USB_ENDPOINT_XFER_CONTROL:
+		pxa_ep->fifo_size = EP0_FIFO_SIZE;
+		break;
+	case USB_ENDPOINT_XFER_ISOC:
+		pxa_ep->fifo_size = ISO_FIFO_SIZE;
+		break;

arch/arm/mach-pxa/generic.c +10 -0

+#endif				/* UDC_PROC_FILE */
+
+	DMSG("registered gadget driver '%s'\n", driver->driver.name);
+	udc_enable(dev);
+	dump_state(dev);
+
+	return 0;
+
+      create_file_error:
+	driver->unbind(&dev->gadget);

arch/arm/mach-pxa/generic.c +8 -0

+			if (i < 0) {
+				/* hardware automagic preventing STALL... */
+				if (dev->req_config) {
+					/* hardware sometimes neglects to tell
+					 * tell us about config change events,
+					 * so later ones may fail...
+					 */
+					WARN("config change %02x fail %d?\n",

arch/arm/mach-pxa/generic.c +11 -0

+				/* the watchdog timer helps deal with cases
+				 * where udc seems to clear FST wrongly, and
+				 * then NAKs instead of STALLing.
+				 */
+				ep0start(dev, UDCCSR0_FST | UDCCSR0_FTF,
+					 "stall");
+				start_watchdog(dev);
+				dev->ep0state = EP0_STALL;
+				LED_EP0_OFF;
+
+				/* deferred i/o == no response yet */

arch/arm/mach-pxa/generic.c +51 -0

+		} else {
+			/* some random early IRQ:
+			 * - we acked FST
+			 * - IPR cleared
+			 * - OPC got set, without SA (likely status stage)
+			 */
+			UDCCSR0 = udccsr0 & (UDCCSR0_SA | UDCCSR0_OPC);
+		}
+		break;
+	case EP0_IN_DATA_PHASE:	/* GET_DESCRIPTOR etc */
+		if (udccsr0 & UDCCSR0_OPC) {
+			UDCCSR0 = UDCCSR0_OPC | UDCCSR0_FTF;
+			DBG(DBG_VERBOSE, "ep0in premature status\n");
+			if (req)
+				done(ep, req, 0);
+			ep0_idle(dev);
+		} else {	/* irq was IPR clearing */
+
+			if (req) {
+				/* this IN packet might finish the request */
+				(void)write_ep0_fifo(ep, req);
+			}	/* else IN token before response was written */
+		}
+		break;
+	case EP0_OUT_DATA_PHASE:	/* SET_DESCRIPTOR etc */
+		if (udccsr0 & UDCCSR0_OPC) {
+			if (req) {
+				/* this OUT packet might finish the request */
+				if (read_ep0_fifo(ep, req))
+					done(ep, req, 0);
+				/* else more OUT packets expected */
+			}	/* else OUT token before read was issued */
+		} else {	/* irq was IPR clearing */
+
+			DBG(DBG_VERBOSE, "ep0out premature status\n");
+			if (req)
+				done(ep, req, 0);
+			ep0_idle(dev);
+		}
+		break;
+	case EP0_STALL:
+		UDCCSR0 = UDCCSR0_FST;
+		break;
+	}
+	UDCISR0 = UDCISR_INT(0, UDCISR_INT_MASK);
+}
+
 
...
 
+static void udc_init_ep(struct pxa27x_udc *dev)
+{
+	int i;
+

arch/arm/mach-pxa/generic.c +26 -0

+		INIT_LIST_HEAD(&ep->queue);
+	}
+}
+
+/*-------------------------------------------------------------------------*/
+
+static void nop_release(struct device *dev)
+{
+	DMSG("%s %s\n", __FUNCTION__, dev->bus_id);
+}
+
+/* this uses load-time allocation and initialization (instead of
+ * doing it at run-time) to save code, eliminate fault paths, and
+ * be more obviously correct.
+ */
+static struct pxa27x_udc memory = {
+	.gadget = {
+		   .ops = &pxa27x_udc_ops,
+		   .ep0 = &memory.ep[0].ep,
+		   .name = driver_name,
+		   .dev = {
+			   .bus_id = "gadget",
+			   .release = nop_release,
+			   },
+		   },
+

arch/arm/mach-pxa/generic.c +9 -0

+		  }
+};
+
+#define CP15R0_VENDOR_MASK	0xffffe000
+
+#define CP15R0_XSCALE_VALUE	0x69054000	/* intel/arm/xscale */
+
+/*
+ * 	probe - binds to the platform device

arch/arm/mach-pxa/generic.c +51 -0

+	if ((chiprev & CP15R0_VENDOR_MASK) != CP15R0_XSCALE_VALUE) {
+		printk(KERN_ERR "%s: not XScale!\n", driver_name);
+		return -ENODEV;
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0)
+		return -ENODEV;
+	pr_debug("%s: IRQ %d\n", driver_name, irq);
+
+	/* other non-static parts of init */
+	udc->dev = dev;
+	udc->mach = dev->platform_data;
+
+	/* Disable irq, erase old events and disable the pull up on the bus */
+	UDCICR0 = 0x00000000;
+	UDCICR1 = 0x00000000;
+	UDCISR0 = 0xffffffff;
+	UDCISR1 = 0xffffffff;
+	if (udc->mach->gpio_pullup)
+		udc_gpio_init_pullup(udc->mach->gpio_pullup);
+
+	init_timer(&udc->timer);
+	udc->timer.function = udc_watchdog;
+	udc->timer.data = (unsigned long)udc;
+
+	device_initialize(&udc->gadget.dev);
+	udc->gadget.dev.parent = dev;
+	udc->gadget.dev.dma_mask = dev->dma_mask;
+
+	the_controller = udc;
+	dev_set_drvdata(dev, udc);
+
+	udc_disable(udc);
+	udc_init_ep(udc);
+	udc_reinit(udc);
+
+	/* irq setup after old hardware state is cleaned up */
+	retval = request_irq(irq, pxa27x_udc_irq, 0, driver_name, udc);
+	if (retval != 0) {
+		printk(KERN_ERR "%s: can't get irq %i, err %d\n",
+		       driver_name, irq, retval);
+		return -EBUSY;
+	}
+	udc->got_irq = 1;
+
+	create_proc_files();
+
+	return 0;
+}
+
 
...
 

arch/arm/mach-pxa/generic.c +11 -0

+#ifdef USE_DMA
+	volatile u32				*reg_drcmr;
+#define	drcmr(n)  .reg_drcmr = & DRCMR ## n ,
+#else
+#define	drcmr(n)  
+#endif
+
+#ifdef CONFIG_PM
+	unsigned				udccsr_value;
+	unsigned				udccr_value;
+#endif

arch/arm/mach-pxa/generic.c +4 -0

+struct pxa27x_udc {
+	struct usb_gadget			gadget;
+	struct usb_gadget_driver		*driver;
+

arch/arm/mach-pxa/generic.c +11 -0

+	struct timer_list			timer;
+
+	struct device				*dev;
+	struct pxa2xx_udc_mach_info		*mach;
+	u64					dma_mask;
+	struct pxa27x_ep			ep [UDC_EP_NUM];
+
+	unsigned				configuration, 
+						interface, 
+						alternate;
+#ifdef CONFIG_PM

arch/arm/mach-pxa/generic.c +18 -0

+/*-------------------------------------------------------------------------*/
+
+/*
+ * Debugging support vanishes in non-debug builds.  DBG_NORMAL should be
+ * mostly silent during normal use/testing, with no timing side-effects.
+ */
+#define DBG_NORMAL	1	/* error paths, device state transitions */
+#define DBG_VERBOSE	2	/* add some success path trace info */
+#define DBG_NOISY	3	/* ... even more: request level */
+#define DBG_VERY_NOISY	4	/* ... even more: packet level */
+
+#ifdef DEBUG
+
+static const char *state_name[] = {
+	"EP0_IDLE",
+	"EP0_IN_DATA_PHASE", "EP0_OUT_DATA_PHASE",
+	"EP0_END_XFER", "EP0_STALL"
+};

arch/arm/mach-pxa/generic.c +11 -0

+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" : "",

arch/arm/mach-pxa/generic.c +6 -0

+dump_udccsr0(const char *label)
+{
+	u32		udccsr0 = UDCCSR0;
+
+	DMSG("%s %s 0x%08x =%s%s%s%s%s%s%s\n",
+		label, state_name[the_controller->ep0state], udccsr0,

arch/arm/mach-pxa/generic.c +24 -0

+dump_state(struct pxa27x_udc *dev)
+{
+	unsigned	i;
+
+	DMSG("%s, udcicr %02X.%02X, udcsir %02X.%02x, udcfnr %02X\n",
+		state_name[dev->ep0state],
+		UDCICR1, UDCICR0, UDCISR1, UDCISR0, UDCFNR);
+	dump_udccr("udccr");
+
+	if (!dev->driver) {
+		DMSG("no gadget driver bound\n");
+		return;
+	} else
+		DMSG("ep0 driver '%s'\n", dev->driver->driver.name);
+
+	
+	dump_udccsr0 ("udccsr0");
+	DMSG("ep0 IN %lu/%lu, OUT %lu/%lu\n",
+		dev->stats.write.bytes, dev->stats.write.ops,
+		dev->stats.read.bytes, dev->stats.read.ops);
+
+	for (i = 1; i < UDC_EP_NUM; i++) {
+		if (dev->ep [i].desc == 0)
+			continue;

Quoted 5 lines

arch/arm/mach-pxa/generic.c +0 -0

@@ -282,7 +282,11 @@ static struct resource pxa2xx_udc_resources[] = {
 static u64 udc_dma_mask = ~(u32)0;
 

arch/arm/mach-pxa/generic.c static struct resource pxa2xx_udc_resources[] = { · line 289

Quoted 6 lines

arch/arm/mach-pxa/generic.c +4 -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?

arch/arm/mach-pxa/generic.c static struct resource pxa2xx_udc_resources[] = {

Quoted 5 lines

arch/arm/mach-pxa/generic.c +4 -0

+ * 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.

Quoted 1 line

+#undef USE_DMA

So DMA is busted?

arch/arm/mach-pxa/generic.c

Quoted 5 lines

arch/arm/mach-pxa/generic.c +5 -0

+#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.

arch/arm/mach-pxa/generic.c

Quoted 4 lines

arch/arm/mach-pxa/generic.c +4 -0

+#ifndef	enable_disconnect_irq
+#define	enable_disconnect_irq()		do {} while (0)
+#define	disable_disconnect_irq()	do {} while (0)
+#endif

hm, OK, I guess this is somewhere where a macro is appropriate.

arch/arm/mach-pxa/generic.c

Quoted 4 lines

arch/arm/mach-pxa/generic.c +4 -0

+					/* hardware sometimes neglects to tell
+					 * tell us about config change events,
+					 * so later ones may fail...
+					 */

arch/arm/mach-pxa/generic.c

Quoted 7 lines

arch/arm/mach-pxa/generic.c +7 -0

+					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!
+					 */

HEX_DISPLAY2 gets different treatment from HEX_DISPLAY here.

arch/arm/mach-pxa/generic.c

Quoted 2 lines

arch/arm/mach-pxa/generic.c +2 -0

+#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?

Quoted 1 line

+#undef DISABLE_TEST_MODE

enabling DISABLE_TEST_MODE seens to enable test mode. Confused.

drivers/usb/gadget/pxa27x_udc.c line 426

Quoted 4 lines

drivers/usb/gadget/pxa27x_udc.c new file +3 -0

+
+static int
+write_packet(volatile u32 * uddr, struct pxa27x_request *req, unsigned max)

Please review Documentation/volatile-considered-harmful.txt

Quoted 4 lines

drivers/usb/gadget/pxa27x_udc.c line 426

Quoted 2 lines

drivers/usb/gadget/pxa27x_udc.c new file +2 -0

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

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.

Quoted 10 lines · David Brownell

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

Quoted 5 lines

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

Quoted 11 lines · David Brownell

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

Quoted 5 lines

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? :)

Quoted 4 lines · Rodolfo Giometti

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/

Quoted 17 lines · David Brownell

Quoted 4 lines · Rodolfo Giometti

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

Quoted 4 lines

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

Quoted 12 lines

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

Quoted 21 lines · Dmitry Krivoschekov

Quoted 16 lines · David Brownell

Quoted 4 lines · Rodolfo Giometti

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

Quoted 4 lines

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

Quoted 6 lines

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

Quoted 3 lines

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.

Quoted 6 lines

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.

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

Quoted 6 lines · David Brownell

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?

+#define NAME_SIZE 18

drivers/usb/gadget/pxa27x_udc.c new file +3 -0

+udc_proc_read(char *page, char **start, off_t off, int count,
+	      int *eof, void *_dev)
+{

+static DEVICE_ATTR(function, S_IRUGO, show_function, NULL);

drivers/usb/gadget/pxa27x_udc.h new file +4 -0

+struct pxa27x_udc {
+	struct usb_gadget			gadget;
+	struct usb_gadget_driver		*driver;
+

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! :)

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! :)

drivers/usb/gadget/ether.c +7 -0

+/* 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)

drivers/usb/gadget/ether.c +95 -0

+
+#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 =%s%s%s%s%s%s%s\n",
+		label, state_name[the_controller->ep0state], udccsr0,
+		(udccsr0 & UDCCSR0_SA) ? " sa" : "",
+		(udccsr0 & UDCCSR0_RNE) ? " rne" : "",
+		(udccsr0 & UDCCSR0_FST) ? " fst" : "",
+		(udccsr0 & UDCCSR0_SST) ? " sst" : "",
+		(udccsr0 & UDCCSR0_DME) ? " dme" : "",
+		(udccsr0 & UDCCSR0_IPR) ? " ipr" : "",
+		(udccsr0 & UDCCSR0_OPC) ? " opr" : "");
+}
+
+static void __attribute__ ((__unused__))
+dump_state(struct pxa27x_udc *dev)
+{
+	unsigned	i;
+
+	DMSG("%s, udcicr %02X.%02X, udcsir %02X.%02x, udcfnr %02X\n",
+		state_name[dev->ep0state],
+		UDCICR1, UDCICR0, UDCISR1, UDCISR0, UDCFNR);
+	dump_udccr("udccr");
+
+	if (!dev->driver) {
+		DMSG("no gadget driver bound\n");
+		return;
+	} else
+		DMSG("ep0 driver '%s'\n", dev->driver->driver.name);
+
+	
+	dump_udccsr0 ("udccsr0");
+	DMSG("ep0 IN %lu/%lu, OUT %lu/%lu\n",
+		dev->stats.write.bytes, dev->stats.write.ops,
+		dev->stats.read.bytes, dev->stats.read.ops);
+
+	for (i = 1; i < UDC_EP_NUM; i++) {
+		if (dev->ep [i].desc == 0)
+			continue;
+		DMSG ("udccs%d = %02x\n", i, *dev->ep->reg_udccsr);
+	}
+}
+
+#if 0
+static void dump_regs(u8 ep)
+{
+	DMSG("EP:%d UDCCSR:0x%08x UDCBCR:0x%08x\n UDCCR:0x%08x\n",
+		ep,UDCCSN(ep), UDCBCN(ep), UDCCN(ep));
+}
+static void dump_req (struct pxa27x_request *req)
+{
+	struct usb_request *r = &req->req;
+
+	DMSG("%s: buf:0x%08x length:%d dma:0x%08x actual:%d\n",
+			__FUNCTION__, (unsigned)r->buf, r->length, 
+			r->dma,	r->actual);
+}
+#endif
+
+#else
+
+#define DMSG(stuff...)		do{}while(0)
+
+#define	dump_udccr(x)	do{}while(0)
+#define	dump_udccsr0(x)	do{}while(0)

drivers/usb/gadget/ether.c +3 -0

+
+
+#endif /* __LINUX_USB_GADGET_PXA27X_H */

drivers/usb/gadget/ether.c MODULE_PARM_DESC(host_addr, "Host Ethernet Address"); · line 294

Quoted 5 lines

drivers/usb/gadget/ether.c +4 -0

+#ifdef CONFIG_USB_GADGET_PXA27X
+#define DEV_CONFIG_SUBSET
+#endif
+

Bad - can we not make this runtime dependent?

drivers/usb/gadget/pxa27x_udc.c

Quoted 4 lines

drivers/usb/gadget/pxa27x_udc.c new file +4 -0

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

Pointers which are cleared are set to NULL not zero. Zero is an integer. NULL is a pointer. Don't confuse the two.