[U-Boot] [PATCH 2/2] pxa25x: Add USB ethernet gadget driver

[Marek Vasut <marex@denx.de>]

Dear ?ukasz Da?ek,


[...]

The following comment still doesn't seem right.

> +/*
> --------------------------------------------------------------------------
> - + *	endpoint related parts of the api to the usb controller hardware, +
> *	used by gadget driver; and the inner talker-to-hardware core.
> + *
> --------------------------------------------------------------------------
> - + */
[...]

> +
> +/*
> + * when a driver is successfully registered, it will receive
> + * control requests including set_configuration(), which enables
> + * non-control requests.  then usb traffic follows until a
> + * disconnect is reported.  then a host may connect again, or
> + * the driver might get unbound.
> + */
> +int usb_gadget_register_driver(struct usb_gadget_driver *driver)
> +{

All this should certainly go into arch/arm/cpu/pxa/ ... as something like 
"get_cpu_revision" or such ... don't you think? Then it can be used elsewhere 
too if needed, give it some thought ;-)

> +	struct pxa25x_udc *dev = &memory;
> +	int retval;
> +	uint32_t chiprev;
> +
> +	if (!driver
> +			|| driver->speed < USB_SPEED_FULL
> +			|| !driver->disconnect
> +			|| !driver->setup)
> +		return -EINVAL;
> +	if (!dev)
> +		return -ENODEV;
> +	if (dev->driver)
> +		return -EBUSY;
> +
> +	/* Enable clock for usb controller */
> +	setbits_le32(CKEN, CKEN11_USB);
> +
> +	/* first hook up the driver ... */
> +	dev->driver = driver;
> +	dev->pullup = 1;
> +
> +	/* insist on Intel/ARM/XScale */
> +	asm("mrc%? p15, 0, %0, c0, c0" : "=r" (chiprev));
> +	if ((chiprev & CP15R0_VENDOR_MASK) != CP15R0_XSCALE_VALUE) {
> +		printf("%s: not XScale!\n", DRIVER_NAME);
> +		return -ENODEV;
> +	}
> +
> +	/* trigger chiprev-specific logic */
> +	switch (chiprev & CP15R0_PRODREV_MASK) {
> +	case PXA255_A0:
> +		dev->has_cfr = 1;
> +		break;
> +	case PXA250_A0:
> +	case PXA250_A1:
> +		/* A0/A1 "not released"; ep 13, 15 unusable */
> +		/* fall through */
> +	case PXA250_B2: case PXA210_B2:
> +	case PXA250_B1: case PXA210_B1:
> +	case PXA250_B0: case PXA210_B0:
> +		/* OUT-DMA is broken ... */
> +		/* fall through */
> +	case PXA250_C0: case PXA210_C0:
> +		break;
> +	default:
> +		printf("%s: unrecognized processor: %08x\n",
> +			DRIVER_NAME, chiprev);
> +		return -ENODEV;
> +	}
> +
> +	the_controller = dev;
> +
> +	/* prepare watchdog timer */
> +	dev->watchdog.running = 0;
> +	dev->watchdog.period = 5000 * CONFIG_SYS_HZ / 1000000; /* 5 ms */
> +	dev->watchdog.function = udc_watchdog;
> +
> +	udc_disable(dev);
> +	udc_reinit(dev);
> +
> +	dev->mach = &mach_info;
> +
> +	dev->gadget.name = "pxa2xx_udc";
> +	retval = driver->bind(&dev->gadget);
> +	if (retval) {
> +		printf("bind to driver %s --> error %d\n",
> +				DRIVER_NAME, retval);
> +		dev->driver = NULL;
> +		return retval;
> +	}
> +
> +	/*
> +	 * ... then enable host detection and ep0; and we're ready
> +	 * for set_configuration as well as eventual disconnect.
> +	 */
> +	printf("registered gadget driver '%s'\n", DRIVER_NAME);
> +
> +	pullup(dev);
> +	dump_state(dev);
> +	return 0;
> +}
[...]

> +struct pxa25x_request {
> +	struct usb_request			req;
> +	struct list_head			queue;
> +};
> +
> +enum ep0_state {
> +	EP0_IDLE,
> +	EP0_IN_DATA_PHASE,
> +	EP0_OUT_DATA_PHASE,
> +	EP0_END_XFER,
> +	EP0_STALL,
> +};
> +
> +#define EP0_FIFO_SIZE	((unsigned)16)

Wasn't there something like 16U or something instead of the typecast?

> +#define BULK_FIFO_SIZE	((unsigned)64)
> +#define ISO_FIFO_SIZE	((unsigned)256)
> +#define INT_FIFO_SIZE	((unsigned)8)
> +
> +struct udc_stats {
> +	struct ep0stats {
> +		unsigned long		ops;
> +		unsigned long		bytes;
> +	} read, write;
> +	unsigned long			irqs;
> +};
> +
> +#ifdef CONFIG_USB_PXA25X_SMALL
> +/* when memory's tight, SMALL config saves code+data.  */
> +#define	PXA_UDC_NUM_ENDPOINTS	3
> +#endif
> +
> +#ifndef	PXA_UDC_NUM_ENDPOINTS
> +#define	PXA_UDC_NUM_ENDPOINTS	16
> +#endif
> +
> +struct pxa25x_watchdog {
> +	unsigned				running:1;
> +	ulong					period;
> +	ulong					base;
> +	struct pxa25x_udc			*udc;
> +
> +	void (*function)(struct pxa25x_udc *udc);
> +};
> +
> +struct pxa25x_udc {
> +	struct usb_gadget			gadget;
> +	struct usb_gadget_driver		*driver;
> +	struct pxa25x_udc_regs			*regs;
> +
> +	enum ep0_state				ep0state;
> +	struct udc_stats			stats;
> +	unsigned				got_irq:1,
> +						pullup:1,
> +						has_cfr:1,
> +						req_pending:1,
> +						req_std:1,
> +						req_config:1,
> +						active:1;
> +
> +	struct clk				*clk;
> +	struct pxa2xx_udc_mach_info		*mach;
> +	u64					dma_mask;
> +	struct pxa25x_ep			ep[PXA_UDC_NUM_ENDPOINTS];
> +
> +	struct pxa25x_watchdog			watchdog;
> +};
> +
> +/*------------------------------------------------------------------------
> -*/ +
> +static struct pxa25x_udc *the_controller;
> +
> +/*------------------------------------------------------------------------
> -*/ +
> +#ifdef DEBUG

Functions should certainly not be in a header file.

> +static const char * const state_name[] = {
> +	"EP0_IDLE",
> +	"EP0_IN_DATA_PHASE", "EP0_OUT_DATA_PHASE",
> +	"EP0_END_XFER", "EP0_STALL"
> +};
> +
> +static void
> +dump_udccr(const char *label)
> +{
> +	u32 udccr = readl(&UDC_REGS->udccr);
> +	debug("%s %02X =%s%s%s%s%s%s%s%s\n",
> +		label, udccr,
> +		(udccr & UDCCR_REM) ? " rem" : "",
> +		(udccr & UDCCR_RSTIR) ? " rstir" : "",
> +		(udccr & UDCCR_SRM) ? " srm" : "",
> +		(udccr & UDCCR_SUSIR) ? " susir" : "",
> +		(udccr & UDCCR_RESIR) ? " resir" : "",
> +		(udccr & UDCCR_RSM) ? " rsm" : "",
> +		(udccr & UDCCR_UDA) ? " uda" : "",
> +		(udccr & UDCCR_UDE) ? " ude" : "");
> +}
> +
> +static void
> +dump_udccs0(const char *label)
> +{
> +	u32 udccs0 = readl(&UDC_REGS->udccs[0]);
> +
> +	debug("%s %s %02X =%s%s%s%s%s%s%s%s\n",
> +		label, state_name[the_controller->ep0state], udccs0,
> +		(udccs0 & UDCCS0_SA) ? " sa" : "",
> +		(udccs0 & UDCCS0_RNE) ? " rne" : "",
> +		(udccs0 & UDCCS0_FST) ? " fst" : "",
> +		(udccs0 & UDCCS0_SST) ? " sst" : "",
> +		(udccs0 & UDCCS0_DRWF) ? " dwrf" : "",
> +		(udccs0 & UDCCS0_FTF) ? " ftf" : "",
> +		(udccs0 & UDCCS0_IPR) ? " ipr" : "",
> +		(udccs0 & UDCCS0_OPR) ? " opr" : "");
> +}
> +
> +static void
> +dump_state(struct pxa25x_udc *dev)
> +{
> +	u32 tmp;
> +	unsigned i;
> +
> +	debug("%s, uicr %02X.%02X, usir %02X.%02x, ufnr %02X.%02X\n",
> +		state_name[dev->ep0state],
> +		readl(&UDC_REGS->uicr1), readl(&UDC_REGS->uicr0),
> +		readl(&UDC_REGS->usir1), readl(&UDC_REGS->usir0),
> +		readl(&UDC_REGS->ufnrh), readl(&UDC_REGS->ufnrl));
> +	dump_udccr("udccr");
> +	if (dev->has_cfr) {
> +		tmp = readl(&UDC_REGS->udccfr);
> +		debug("udccfr %02X =%s%s\n", tmp,
> +			(tmp & UDCCFR_AREN) ? " aren" : "",
> +			(tmp & UDCCFR_ACM) ? " acm" : "");
> +	}
> +
> +	if (!dev->driver) {
> +		debug("no gadget driver bound\n");
> +		return;
> +	} else
> +		debug("ep0 driver '%s'\n", "ether");
> +
> +	dump_udccs0("udccs0");
> +	debug("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 < PXA_UDC_NUM_ENDPOINTS; i++) {
> +		if (dev->ep[i].desc == NULL)
> +			continue;
> +		debug("udccs%d = %02x\n", i, *dev->ep->reg_udccs);
> +	}
> +}
> +
> +#ifdef DEBUG_NOISY
> +# define NOISY 1
> +#else
> +# define NOISY 0
> +#endif
> +
> +#else
> +
> +#define NOISY 0
> +#define	dump_udccr(x)	do {} while (0)
> +#define	dump_udccs0(x)	do {} while (0)
> +#define	dump_state(x)	do {} while (0)

Please make these empty inline functions (for the sake of typechecking, compiler 
will optimize them out well).

> +#endif
> +
> +#endif /* __LINUX_USB_GADGET_PXA25X_H */