Re: [U-Boot] [PATCH v4 2/2] usb: gadget: add Faraday FOTG210 USB gadget support

[Kuo-Jung Su]

2013/5/8 Marek Vasut <ma...@denx.de>:
> Dear Kuo-Jung Su,
>
>> From: Kuo-Jung Su <dant...@faraday-tech.com>
>>
>> The Faraday FOTG210 is an OTG chip which could operate
>> as either an EHCI Host or a USB Device as a time.
>>
>> Signed-off-by: Kuo-Jung Su <dant...@faraday-tech.com>
>> CC: Marek Vasut <ma...@denx.de>
>> ---
>> Changes for v4:
>>    - Use only macro constants and named bit/mask
>>    - Remove dcache_enable() from usb_gadget_register_driver()
>>
>> Changes for v3:
>>    - Coding Style cleanup.
>>    - Drop bit fields from c struct.
>>    - Drop macros for wirtel()/readl(), call them directly.
>>    - Always insert a blank line between declarations and code.
>>    - Replace all the infinite wait loop with a timeout.
>>    - Add '__iomem' to all the declaration of HW register pointers.
>>
>> Changes for v2:
>>    - Coding Style cleanup.
>>    - Use readl(), writel(), clrsetbits_le32() to replace REG() macros.
>>    - Use structure based hardware registers to replace the macro constants.
>>    - Replace BIT() with BIT_MASK().
>>    - echi-faraday: Remove debug codes.
>>
>>  drivers/usb/gadget/Makefile       |    1 +
>>  drivers/usb/gadget/fotg210.c      |  961
>> +++++++++++++++++++++++++++++++++++++ drivers/usb/gadget/gadget_chips.h |
>>   8 +
>>  3 files changed, 970 insertions(+)
>>  create mode 100644 drivers/usb/gadget/fotg210.c
>>
>> diff --git a/drivers/usb/gadget/Makefile b/drivers/usb/gadget/Makefile
>> index e545b6b..432cf17 100644
>> --- a/drivers/usb/gadget/Makefile
>> +++ b/drivers/usb/gadget/Makefile
>> @@ -35,6 +35,7 @@ endif
>>  # new USB gadget layer dependencies
>>  ifdef CONFIG_USB_GADGET
>>  COBJS-$(CONFIG_USB_GADGET_S3C_UDC_OTG) += s3c_udc_otg.o
>> +COBJS-$(CONFIG_USB_GADGET_FOTG210) += fotg210.o
>>  COBJS-$(CONFIG_USBDOWNLOAD_GADGET) += g_dnl.o
>>  COBJS-$(CONFIG_DFU_FUNCTION) += f_dfu.o
>>  endif
>> diff --git a/drivers/usb/gadget/fotg210.c b/drivers/usb/gadget/fotg210.c
>> new file mode 100644
>> index 0000000..70797ae
>> --- /dev/null
>> +++ b/drivers/usb/gadget/fotg210.c
>> @@ -0,0 +1,961 @@
>> +/*
>> + * Faraday USB 2.0 OTG Controller
>> + *
>> + * (C) Copyright 2010 Faraday Technology
>> + * Dante Su <dant...@faraday-tech.com>
>> + *
>> + * This file is released under the terms of GPL v2 and any later version.
>> + * See the file COPYING in the root directory of the source tree for
>> details. + */
>> +
>> +#include <common.h>
>> +#include <command.h>
>> +#include <config.h>
>> +#include <net.h>
>> +#include <malloc.h>
>> +#include <asm/io.h>
>> +#include <asm/errno.h>
>> +#include <linux/types.h>
>> +#include <linux/usb/ch9.h>
>> +#include <linux/usb/gadget.h>
>> +
>> +#include <usb/fotg210.h>
>> +
>> +#define CFG_NUM_ENDPOINTS            4
>> +#define CFG_EP0_MAX_PACKET_SIZE      64
>> +#define CFG_EPX_MAX_PACKET_SIZE      512
>> +
>> +#define CFG_CMD_TIMEOUT (CONFIG_SYS_HZ >> 2) /* 250 ms */
>> +#define CFG_RST_TIMEOUT (CONFIG_SYS_HZ >> 2) /* 250 ms */
>
> Uh, how's this supposed to work? Why don't you just use mdelay(250) with 
> those?
>

1. The CFG_CMD_TIMEOUT is used for polling dma or ep0 fifo ready,
    put a permanent delay (i.e. mdelay()) here would slow down the
    overall performance.

2. The CFG_RST_TIMEOUT is used for polling chip reset or fifo reset.
    It's definitely O.K to replace it with a permanent delay, so I'll
do it later

> [...]
>
>> +static inline int ep_reset(struct fotg210_chip *chip, uint8_t ep_addr)
>> +{
>> +     int ep = ep_addr & USB_ENDPOINT_NUMBER_MASK;
>> +     struct fotg210_regs __iomem *regs = chip->regs;
>
> No need for __iomem here.
>

Got it, thanks

> [...]
>
>> +     /* 3. DMA target setup */
>> +#ifndef CONFIG_SYS_DCACHE_OFF
>> +     if (ep->desc->bEndpointAddress & USB_DIR_IN)
>> +             flush_dcache_range((uint32_t)buf, (uint32_t)buf + len);
>> +     else
>> +             invalidate_dcache_range((uint32_t)buf, (uint32_t)buf + len);
>> +#endif
>
> The cache stuff will be optimized out anyway, no ?
>

That's right, I'll have the #ifdef statement removed later

> [...]
>
>> +                     if (readl(&regs->dev_ctrl) & DEVCTRL_HS) {
>> +                             puts("fotg210: HS\n");
>> +                             chip->gadget.speed = USB_SPEED_HIGH;
>> +                             writel(0x044c, &regs->sof_mtr);
>> +                     } else {
>> +                             puts("fotg210: FS\n");
>> +                             chip->gadget.speed = USB_SPEED_FULL;
>> +                             writel(0x2710, &regs->sof_mtr);
>
> Magic values, please fix globally as previously asked.
>

Sorry, I forget to fix this one.

> [...]
>



--
Best wishes,
Kuo-Jung Su
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot