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.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Reply via email to