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

Reply via email to