[U-Boot] [PATCH 4/8] OMAP3 Add usb device support

[Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>]

Hi,

	please be carefull you have a lot's of ligne which exceed 80 chars
	limit

> +
> +/* Define MUSB_DEBUG before including this file to get debug macros */
> +#ifdef MUSB_DEBUG
> +
> +static inline void MUSB_PRINT_PWR(u8 b)
please lowercase
> +{
> +	serial_printf("\tpower   0x%2.2x\n", b);
maybe a macro will simplify the code
#define MUSB_FLAGS_PRINT(x, y)	\
	if (b & MUSB_##x_##y)	\
		serial_printf("\t\t"#y"\n");

static inline void musb_print_pwr(u8 b)
{
	serial_printf("\tpower   0x%2.2x\n", b);
	MUSB_FLAGS_PRINT(POWER, ISOUPDATE);
	MUSB_FLAGS_PRINT(POWER, SOFTCONN);
	MUSB_FLAGS_PRINT(POWER, HSENAB);
	MUSB_FLAGS_PRINT(POWER, HSMODE);
	MUSB_FLAGS_PRINT(POWER, RESET);
	MUSB_FLAGS_PRINT(POWER, RESUME);
	MUSB_FLAGS_PRINT(POWER, SUSPENDM);
	MUSB_FLAGS_PRINT(POWER, ENSUSPEND);
}
and so on
> +
> +static inline void MUSB_PRINT_CSR0(u16 w)
please lowercase and so on
> +{
> +	serial_printf("\tcsr0    0x%4.4x\n", w);
> +	if (w & MUSB_CSR0_FLUSHFIFO)
> +		serial_printf("\t\tFLUSHFIFO\n");
> +	if (w & MUSB_CSR0_P_SVDSETUPEND)
> +		serial_printf("\t\tSERV_SETUPEND\n");
> +	if (w & MUSB_CSR0_P_SVDRXPKTRDY)
> +		serial_printf("\t\tSERV_RXPKTRDY\n");
> +	if (w & MUSB_CSR0_P_SENDSTALL)
> +		serial_printf("\t\tSENDSTALL\n");
> +	if (w & MUSB_CSR0_P_SETUPEND)
> +		serial_printf("\t\tSETUPEND\n");
> +	if (w & MUSB_CSR0_P_DATAEND)
> +		serial_printf("\t\tDATAEND\n");
> +	if (w & MUSB_CSR0_P_SENTSTALL)
> +		serial_printf("\t\tSENTSTALL\n");
> +	if (w & MUSB_CSR0_TXPKTRDY)
> +		serial_printf("\t\tTXPKTRDY\n");
> +	if (w & MUSB_CSR0_RXPKTRDY)
> +		serial_printf("\t\tRXPKTRDY\n");
> +}
> +

> +
> +#define MAX_ENDPOINT 15
maybe we can put it configurable
> +
> +#define GET_ENDPOINT(dev,ep)						\
> +(((struct usb_device_instance *)(dev))->bus->endpoint_array + ep)
> +
> +#define SET_EP0_STATE(s)						\
> +do {									\
> +	if ((0 <= (s)) && (SET_ADDRESS >= (s))) {			\
> +		if ((s) != ep0_state) {					\
> +			if ((debug_setup) && (debug_level > 1))		\
> +				serial_printf("INFO : Changing state from %s to %s in %s at line %d\n", ep0_state_strings[ep0_state], ep0_state_strings[s], __PRETTY_FUNCTION__, __LINE__); \
too long please split
> +			ep0_state = s;					\
> +		}							\
> +	} else {							\
> +		if (debug_level > 0)					\
> +			serial_printf("Error at %s %d with setting state %d is invalid\n", __PRETTY_FUNCTION__, __LINE__, s); \
too long please split
> +	}								\
> +} while (0)
> +
> +/* static implies these initialized to 0 or NULL */
> +static int debug_setup;
> +static int debug_level;
> +static struct musb_epinfo epinfo[MAX_ENDPOINT * 2];
> +static enum ep0_state_enum { IDLE = 0, TX, RX, SET_ADDRESS } ep0_state = IDLE;
this will be better
static enum ep0_state_enum {
	IDLE = 0,
	TX,
	RX,
	SET_ADDRESS
} ep0_state = IDLE;
> +static char *ep0_state_strings[4] = {
> +	"IDLE",
> +	"TX",
> +	"RX",
> +	"SET_ADDRESS",
> +};
> +
> +static struct urb *ep0_urb;
> +struct usb_endpoint_instance *ep0_endpoint;
> +static struct usb_device_instance *udc_device;
> +static int enabled;
> +
> +#ifdef MUSB_DEBUG
> +static void musb_db_regs(void)
> +{
> +	u8 b;
> +	u16 w;
> +
> +	b = readb(&musbr->faddr);
> +	serial_printf("\tfaddr   0x%2.2x\n", b);
> +
> +	b = readb(&musbr->power);
> +	MUSB_PRINT_PWR(b);
> +
> +	w = readw(&musbr->ep[0].ep0.csr0);
> +	MUSB_PRINT_CSR0(w);
> +
> +	b = readb(&musbr->devctl);
> +	MUSB_PRINT_DEVCTL(b);
> +
> +	b = readb(&musbr->ep[0].ep0.configdata);
> +	MUSB_PRINT_CONFIG(b);
> +
> +	w = readw(&musbr->frame);
> +	serial_printf("\tframe   0x%4.4x\n", w);
> +
> +	b = readb(&musbr->index);
> +	serial_printf("\tindex   0x%2.2x\n", b);
> +
> +	w = readw(&musbr->ep[1].epN.rxmaxp);
> +	MUSB_PRINT_RXMAXP(w);
> +
> +	w = readw(&musbr->ep[1].epN.rxcsr);
> +	MUSB_PRINT_RXCSR(w);
> +
> +	w = readw(&musbr->ep[1].epN.txmaxp);
> +	MUSB_PRINT_TXMAXP(w);
> +
> +	w = readw(&musbr->ep[1].epN.txcsr);
> +	MUSB_PRINT_TXCSR(w);
> +}
> +#else
> +#define musb_db_regs()
> +#endif /* DEBUG_MUSB */
> +
> +static void musb_peri_softconnect(void)
> +{
> +	u8 power, devctl;
> +	u8 intrusb;
> +	u16 intrrx, intrtx;
> +
> +	/* Power off MUSB */
> +	power = readb(&musbr->power);
> +	power &= ~MUSB_POWER_SOFTCONN;
> +	writeb(power, &musbr->power);
> +
> +	/* Read intr to clear */
> +	intrusb = readb(&musbr->intrusb);
> +	intrrx = readw(&musbr->intrrx);
> +	intrtx = readw(&musbr->intrtx);
> +
> +	udelay(2 * 500000); /* 1 sec */
why not udelay(1000 * 1000) as other place?
> +
> +	/* Power on MUSB */
> +	power = readb(&musbr->power);
> +	power |= MUSB_POWER_SOFTCONN;
> +	/*
> +	 * The usb device interface is usb 1.1
> +	 * Disable 2.0 high speed by clearring the hsenable bit.
> +	 */
> +	power &= ~MUSB_POWER_HSENAB;
> +	writeb(power, &musbr->power);
> +
> +	/* Check if device is in b-peripheral mode */
> +	devctl = readb(&musbr->devctl);
> +	if (!(devctl & MUSB_DEVCTL_BDEVICE) ||
> +	    (devctl & MUSB_DEVCTL_HM)) {
> +		serial_printf("ERROR : Unsupport USB mode\n");
> +		serial_printf("Check that mini-B USB cable is attached to the device\n");
evenif it's too long it's better for search
> +	}
> +
> +	if (debug_setup && (debug_level > 1))
> +		musb_db_regs();
> +}
> +
> +static void musb_peri_reset(void)
> +{
> +	if ((debug_setup) && (debug_level > 1))
> +		serial_printf("INFO : %s reset\n", __PRETTY_FUNCTION__);
what do you mean y __PRETTY_FUNCTION__?
> +
> +	/* the device address is reset by the event */
> +	usbd_device_event_irq(udc_device, DEVICE_RESET, 0);
> +	SET_EP0_STATE(IDLE);
> +	if (ep0_endpoint)
> +		ep0_endpoint->endpoint_address = 0xff;
> +}
> +
> +static void musb_peri_resume(void)
> +{
> +	/* noop */
> +}
> +
> +static void musb_peri_ep0_stall(void)
> +{
> +	u16 csr0;
please add an empty line
> +	csr0 = readw(&musbr->ep[0].ep0.csr0);
> +	csr0 |= MUSB_CSR0_P_SENDSTALL;
> +	writew(csr0, &musbr->ep[0].ep0.csr0);
> +	if ((debug_setup) && (debug_level > 1))
> +		serial_printf("INFO : %s stall\n", __PRETTY_FUNCTION__);
> +}
> +
> +static void musb_peri_ep0_ack_req(void)
> +{
> +	u16 csr0;
please add an empty line
> +	csr0 = readw(&musbr->ep[0].ep0.csr0);
> +	csr0 |= MUSB_CSR0_P_SVDRXPKTRDY;
> +	writew(csr0, &musbr->ep[0].ep0.csr0);
> +}
> +
> +static void musb_ep0_tx_ready(void)
> +{
> +	u16 csr0;
please add an empty line
> +	csr0 = readw(&musbr->ep[0].ep0.csr0);
> +	csr0 |= MUSB_CSR0_TXPKTRDY;
> +	writew(csr0, &musbr->ep[0].ep0.csr0);
> +}
> +
> +static void musb_peri_ep0_last(void)
> +{
> +	u16 csr0;
please add an empty line
> +	csr0 = readw(&musbr->ep[0].ep0.csr0);
> +	csr0 |= MUSB_CSR0_P_DATAEND;
> +	writew(csr0, &musbr->ep[0].ep0.csr0);
> +}
is it not possible to create a single function which take
MUSB_CSR0_P_DATAEND, MUSB_CSR0_TXPKTRDY, MUSB_CSR0_P_SVDRXPKTRDY,
MUSB_CSR0_P_SENDSTALL etc... as paramter?
> +
> +static void musb_peri_ep0_set_address(void)
> +{
> +	writeb(udc_device->address, &musbr->faddr);
> +	SET_EP0_STATE(IDLE);
> +	usbd_device_event_irq(udc_device, DEVICE_ADDRESS_ASSIGNED, 0);
> +	if ((debug_setup) && (debug_level > 1))
> +		serial_printf("INFO : %s Address set to %d\n", __PRETTY_FUNCTION__, udc_device->address);
> +}
> +
> +static void musb_peri_rx_ack(unsigned int ep)
> +{
> +	u16 peri_rxcsr;
> +	peri_rxcsr = readw(&musbr->ep[ep].epN.rxcsr);
> +	peri_rxcsr &= ~MUSB_RXCSR_RXPKTRDY;
> +	writew(peri_rxcsr, &musbr->ep[ep].epN.rxcsr);
> +}
> +
> +static void musb_peri_tx_ready(unsigned int ep)
> +{
> +	u16 peri_txcsr;
> +	peri_txcsr = readw(&musbr->ep[ep].epN.txcsr);
> +	peri_txcsr |= MUSB_TXCSR_TXPKTRDY;
> +	writew(peri_txcsr, &musbr->ep[ep].epN.txcsr);
> +}
> +
> +static void musb_peri_ep0_zero_data_request(int err)
> +{
> +	musb_peri_ep0_ack_req();
> +
> +	if (err) {
> +		musb_peri_ep0_stall();
> +		SET_EP0_STATE(IDLE);
> +	} else {
> +
> +		musb_peri_ep0_last();
> +
> +		/* USBD state */
> +		switch (ep0_urb->device_request.bRequest) {
> +		case USB_REQ_SET_ADDRESS:
> +			if ((debug_setup) && (debug_level > 1))
> +				serial_printf("INFO : %s received set address\n", __PRETTY_FUNCTION__);
maybe you can define a debug macro to reduce the code length
and avoid the if everywhere
> +			break;
> +
> +		case USB_REQ_SET_CONFIGURATION:
> +			if ((debug_setup) && (debug_level > 1))
> +				serial_printf("INFO : %s Configured\n", __PRETTY_FUNCTION__);
> +			usbd_device_event_irq(udc_device, DEVICE_CONFIGURED, 0);
> +			break;
> +		}
> +
> +		/* EP0 state */
> +		if (USB_REQ_SET_ADDRESS == ep0_urb->device_request.bRequest) {
> +			SET_EP0_STATE(SET_ADDRESS);
> +		} else {
> +			SET_EP0_STATE(IDLE);
> +		}
> +	}
> +}
> +
> +static void musb_peri_ep0_rx_data_request(void)
> +{
> +	/*
> +	 * This is the completion of the data OUT / RX
> +	 *
> +	 * Host is sending data to ep0 that is not
> +	 * part of setup.  This comes from the cdc_recv_setup
> +	 * op that is device specific.
> +	 *
> +	 */
> +	musb_peri_ep0_ack_req();
> +
> +	ep0_endpoint->rcv_urb = ep0_urb;
> +	ep0_urb->actual_length = 0;
> +	SET_EP0_STATE(RX);
> +}
> +
> +static void musb_peri_ep0_tx_data_request(int err)
> +{
> +	if (err) {
> +		musb_peri_ep0_stall();
> +		SET_EP0_STATE(IDLE);
> +	} else {
> +		musb_peri_ep0_ack_req();
> +
> +		ep0_endpoint->tx_urb = ep0_urb;
> +		ep0_endpoint->sent = 0;
> +		SET_EP0_STATE(TX);
> +	}
> +}
> +
> +static void musb_peri_ep0_idle(void)
> +{
> +	u16 count0;
> +	int err;
> +	u16 csr0;
> +
> +	csr0 = readw(&musbr->ep[0].ep0.csr0);
> +
> +	if (!(MUSB_CSR0_RXPKTRDY & csr0))
> +		goto end;
> +
> +	count0 = readw(&musbr->ep[0].ep0.count0);
> +	if (count0 == 0)
> +		goto end;
> +
> +	if (count0 != 8) {
> +		if ((debug_setup) && (debug_level > 1))
> +			serial_printf("WARN : %s SETUP incorrect size %d\n",
> +				      __PRETTY_FUNCTION__, count0);
> +		musb_peri_ep0_stall();
> +		goto end;
> +	}
> +
> +	read_fifo(0, count0, &ep0_urb->device_request);
> +
> +	if (debug_level > 2) {
> +		PRINT_USB_DEVICE_REQUEST(&ep0_urb->device_request);
> +	}
> +
> +	if (0 == ep0_urb->device_request.wLength) {
please invert the test
> +		err = ep0_recv_setup(ep0_urb);
> +
> +		/* Zero data request */
> +		musb_peri_ep0_zero_data_request(err);
> +	} else {
> +		/* Is data coming or going ? */
> +		u8 reqType = ep0_urb->device_request.bmRequestType;
please add an empty line
> +		if (USB_REQ_DEVICE2HOST == (reqType & USB_REQ_DIRECTION_MASK)) {
> +			err = ep0_recv_setup(ep0_urb);
> +			/* Device to host */
> +			musb_peri_ep0_tx_data_request(err);
> +		} else {
> +			/*
> +			 * Host to device
> +			 *
> +			 * The RX routine will call ep0_recv_setup
> +			 * when the data packet has arrived.
> +			 */
> +			musb_peri_ep0_rx_data_request();
> +		}
> +	}
> +
> +end:
> +	return;
why? will not be usefull to have an error return?
> +}
> +
> +
> diff --git a/include/usb.h b/include/usb.h
> index 378a23b..198d5bb 100644
> --- a/include/usb.h
> +++ b/include/usb.h
> @@ -131,7 +131,8 @@ struct usb_device {
>  #if defined(CONFIG_USB_UHCI) || defined(CONFIG_USB_OHCI) || \
>  	defined(CONFIG_USB_EHCI) || defined(CONFIG_USB_OHCI_NEW) || \
>  	defined(CONFIG_USB_SL811HS) || defined(CONFIG_USB_ISP116X_HCD) || \
> -	defined(CONFIG_USB_R8A66597_HCD) || defined(CONFIG_USB_DAVINCI)
> +	defined(CONFIG_USB_R8A66597_HCD) || defined(CONFIG_USB_DAVINCI) || \
> +	defined(CONFIG_USB_OMAP3)
a simple CONFIG will be better
>  
>  int usb_lowlevel_init(void);
>  int usb_lowlevel_stop(void);

Best Regards,
J.