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

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.