[U-Boot] [PATCH 4/8] OMAP3 Add usb device support
Tom
Tom.Rix at windriver.com
Sun Sep 6 15:35:06 CEST 2009
Jean-Christophe PLAGNIOL-VILLARD wrote:
> Hi,
>
> please be carefull you have a lot's of ligne which exceed 80 chars
> limit
>
>
Yes I saw these from check_patch.
I will try to reduce these.
>> +
>> +/* Define MUSB_DEBUG before including this file to get debug macros */
>> +#ifdef MUSB_DEBUG
>> +
>> +static inline void MUSB_PRINT_PWR(u8 b)
>>
> please lowercase
>
OK
>> +{
>> + 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
>
Thanks.
I will give this a try.
>> +
>> +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
>
mmmm I will see
>> +
>> +#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
>
Ok.
>> + } \
>> +} 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;
>
ok
>> +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?
>
cruft begone!
ok. i will clean this up.
>> +
>> + /* 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__?
>
This a gcc compiler macro for printing out the function of the calling
routine. It is useful in the macro's like the state machine changer
to show which function did the change instead of the usual
__LINE__ + __FILE__ + eyeball time. It is a non-standard cpp macro
and would bite us when compiling without gcc. Does u-boot build
with non-gcc ?
>> +
>> + /* 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
>
ok
>> + 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
>
ok,
>> + 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
>
ok
>> + 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
>
empty lines for everyone!
>> + 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
>
ok i will look into it.
>> + 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
>
ok
>> + 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?
>
I am not sure it needs an error.
I will check.
>> +}
>> +
>> +
>> 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
>
Can this be a follow-on ?
Tom
>>
>> int usb_lowlevel_init(void);
>> int usb_lowlevel_stop(void);
>>
>
> Best Regards,
> J.
>
More information about the U-Boot
mailing list