[U-Boot] [PATCH 4/9] usbd driver and usb boot firmware support for SPEAr SoCs

Vipin Kumar
2009-12-19T07:02:23Z

Dear Wolfgang,

>>
>> Signed-off-by: Vipin <vipin.kumar@st.com>
>> ---
>> ?common/main.c ? ? ? ? ? ? ? ? ? ? ? ? | ? ?2 +
>> ?drivers/serial/usbtty.h ? ? ? ? ? ? ? | ? ?2 +
>> ?drivers/usb/gadget/Makefile ? ? ? ? ? | ? ?1 +
>> ?drivers/usb/gadget/spr_udc.c ? ? ? ? ?| ?996 +++++++++++++++++++++++++++++++++
>> ?include/asm-arm/arch-spear/spr_misc.h | ?126 +++++
>> ?include/usb/spr_udc.h ? ? ? ? ? ? ? ? | ?227 ++++++++
>> ?6 files changed, 1354 insertions(+), 0 deletions(-)
>> ?mode change 100644 => 100755 drivers/serial/usbtty.h
>> ?mode change 100644 => 100755 drivers/usb/gadget/Makefile
>> ?create mode 100755 drivers/usb/gadget/spr_udc.c
>> ?create mode 100644 include/asm-arm/arch-spear/spr_misc.h
>> ?create mode 100755 include/usb/spr_udc.h
>
> Please split into two patches: one with generic usbd driver, and the
> second adding support for SPEAr.
>

Ok. Accepted.
patch-set v2 would contain the changes

>> diff --git a/common/main.c b/common/main.c
>> index 10d8904..79f3018 100644
>> --- a/common/main.c
>> +++ b/common/main.c
>> @@ -397,6 +397,7 @@ void main_loop (void)
>>
>> ? ? ? debug ("### main_loop: bootcmd=\"%s\"\n", s ? s : "<UNDEFINED>");
>>
>> +#if !defined(CONFIG_SPEAR_USBTTY)
>> ? ? ? if (bootdelay >= 0 && s && !abortboot (bootdelay)) {
>> ?# ifdef CONFIG_AUTOBOOT_KEYED
>> ? ? ? ? ? ? ? int prev = disable_ctrlc(1); ? ?/* disable Control C checking */
>> @@ -413,6 +414,7 @@ void main_loop (void)
>> ? ? ? ? ? ? ? disable_ctrlc(prev); ? ?/* restore Control C checking */
>> ?# endif
>> ? ? ? }
>> +# endif
>
> Why would this be needed?

We also use u-boot as a firmware which runs on the board in a special
mode in which the firmware fetches and burns images into
NOR/NAND flashes. Under this mode, we would like to always jump to
u-boot prompt.
Also, the bootdelay variable should remain unchanged as we save default
environment variables as well.

>
>
>> diff --git a/drivers/usb/gadget/spr_udc.c b/drivers/usb/gadget/spr_udc.c
>> new file mode 100755
>> index 0000000..5b135c7
>> --- /dev/null
>> +++ b/drivers/usb/gadget/spr_udc.c
> ...
>> +/* Some kind of debugging output... */
>> +#if 1
>> +#define UDCDBG(str)
>> +#define UDCDBGA(fmt, args...)
>> +#else
>> +#define UDCDBG(str) serial_printf(str "\n")
>> +#define UDCDBGA(fmt, args...) serial_printf(fmt "\n", ##args)
>> +#endif
>
> This looks wrong. Should that be a "#ifndef DEBUG" instead of "#if 1"?
>
> And cannot we use standard debug facilities?
>

Ok, changed to #ifndef DEBUG
The standard debug facilities use printf and we also test and use this for
usbtty feature. So, a serial_printf is required

>> +static struct udc_endp_regs *const outep_regs_p =
>> + ? ? ? ? ? ? &((struct udc_regs *const)CONFIG_SYS_USBD_BASE)->out_regs[0];
>> +static struct udc_endp_regs *const inep_regs_p =
>> + ? ? ? ? ? ? &((struct udc_regs *const)CONFIG_SYS_USBD_BASE)->in_regs[0];
>> +
>> +/*
>> + * udc_state_transition - Write the next packet to TxFIFO.
>> + * @initial: Initial state.
>> + * @final: ? Final state.
>> + *
>> + * Helper function to implement device state changes. The device states and
>> + * the events that transition between them are:
>> + *
>> + * ? ? ? ? ? ? ? ? ? ? ? ? ? STATE_ATTACHED
>> + * ? ? ? ? ? ? ? ? ? ? ? ? ? || ? ? ?/\
>> + * ? ? ? ? ? ? ? ? ? ? ? ? ? \/ ? ? ?||
>> + * ? DEVICE_HUB_CONFIGURED ? ? ? ? ? ? ? ? ? DEVICE_HUB_RESET
>> + * ? ? ? ? ? ? ? ? ? ? ? ? ? || ? ? ?/\
>> + * ? ? ? ? ? ? ? ? ? ? ? ? ? \/ ? ? ?||
>> + * ? ? ? ? ? ? ? ? ? ? ? ? ? STATE_POWERED
>> + * ? ? ? ? ? ? ? ? ? ? ? ? ? || ? ? ?/\
>> + * ? ? ? ? ? ? ? ? ? ? ? ? ? \/ ? ? ?||
>> + * ? DEVICE_RESET ? ? ? ? ? ? ? ? ? ? ? ? ? ?DEVICE_POWER_INTERRUPTION
>> + * ? ? ? ? ? ? ? ? ? ? ? ? ? || ? ? ?/\
>> + * ? ? ? ? ? ? ? ? ? ? ? ? ? \/ ? ? ?||
>> + * ? ? ? ? ? ? ? ? ? ? ? ? ? STATE_DEFAULT
>> + * ? ? ? ? ? ? ? ? ? ? ? ? ? || ? ? ?/\
>> + * ? ? ? ? ? ? ? ? ? ? ? ? ? \/ ? ? ?||
>> + * ? DEVICE_ADDRESS_ASSIGNED ? ? ? ? ? ? ? ? DEVICE_RESET
>> + * ? ? ? ? ? ? ? ? ? ? ? ? ? || ? ? ?/\
>> + * ? ? ? ? ? ? ? ? ? ? ? ? ? \/ ? ? ?||
>> + * ? ? ? ? ? ? ? ? ? ? ? ? ? STATE_ADDRESSED
>> + * ? ? ? ? ? ? ? ? ? ? ? ? ? || ? ? ?/\
>> + * ? ? ? ? ? ? ? ? ? ? ? ? ? \/ ? ? ?||
>> + * ? DEVICE_CONFIGURED ? ? ? ? ? ? ? ? ? ? ? DEVICE_DE_CONFIGURED
>> + * ? ? ? ? ? ? ? ? ? ? ? ? ? || ? ? ?/\
>> + * ? ? ? ? ? ? ? ? ? ? ? ? ? \/ ? ? ?||
>> + * ? ? ? ? ? ? ? ? ? ? ? ? ? STATE_CONFIGURED
>> + *
>> + * udc_state_transition transitions up (in the direction from STATE_ATTACHED
>> + * to STATE_CONFIGURED) from the specified initial state to the specified final
>> + * state, passing through each intermediate state on the way. If the initial
>> + * state is at or above (i.e. nearer to STATE_CONFIGURED) the final state, then
>> + * no state transitions will take place.
>> + *
>> + * udc_state_transition also transitions down (in the direction from
>> + * STATE_CONFIGURED to STATE_ATTACHED) from the specified initial state to the
>> + * specified final state, passing through each intermediate state on the way.
>> + * If the initial state is at or below (i.e. nearer to STATE_ATTACHED) the final
>> + * state, then no state transitions will take place.
>> + *
>> + * This function must only be called with interrupts disabled.
>> + */
>> +static void udc_state_transition(usb_device_state_t initial,
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?usb_device_state_t final)
> ...
>> +/* Stall endpoint */
>> +static void udc_stall_ep(u32 ep_num)
> ...
>> +static void *get_fifo(int ep_num, int in)
> ...
>> +static short usbgetpckfromfifo(int epNum, u8 *bufp, u32 len)
> ...
>> +static void usbputpcktofifo(int epNum, u8 *bufp, u32 len)
> ...
>
> So far this code looks pretty generic to me.

No, this code is not generic.

>> +/*
>> + * spear_write_noniso_tx_fifo - Write the next packet to TxFIFO.
>> + * @endpoint: ? ? ? ? ? ? ? ?Endpoint pointer.
>> + *
>> + * If the endpoint has an active tx_urb, then the next packet of data from the
>> + * URB is written to the tx FIFO. ?The total amount of data in the urb is given
>> + * by urb->actual_length. ?The maximum amount of data that can be sent in any
>> + * one packet is given by endpoint->tx_packetSize. ?The number of data bytes
>> + * from this URB that have already been transmitted is given by endpoint->sent.
>> + * endpoint->last is updated by this routine with the number of data bytes
>> + * transmitted in this packet.
>> + *
>> + */
>> +static void spear_write_noniso_tx_fifo(struct usb_endpoint_instance
>> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?*endpoint)
>
> Now her eit becomes clearly SPEAr-specific. Should this not be split
> into separate source files to allow reuse of the common code by other
> processors?
>

Again, all the code is spear specific. Probably, only the unction names
are confusing

>> +
>> + ? ? ? ? ? ? ? ? ? ? /* This ensures that USBD packet fifo is accessed
>> + ? ? ? ? ? ? ? ? ? ? ? ?:- through word aligned pointer or
>> + ? ? ? ? ? ? ? ? ? ? ? ?:- through non word aligned pointer but only with a
>> + ? ? ? ? ? ? ? ? ? ? ? ?max length to make the next packet word aligned */
>
> Incorrect multiline comment style.
>

Ok. Corrected globally

> Too long lines.
>

I assume line length of 80 is acceptable. Right?

>> + ? ? ? ? ? ? /* This rx interrupt must be for a control write data
>> + ? ? ? ? ? ? ?* stage packet.
>> + ? ? ? ? ? ? ?*
>> + ? ? ? ? ? ? ?* We don't support control write data stages.
>> + ? ? ? ? ? ? ?* We should never end up here.
>> + ? ? ? ? ? ? ?*/
>
> Incorrect multiline comment style. Please fix globally.

Ok. Corrected globally

>
>> +struct misc_regs {
> ...
>> + ? ? u32 BIST5_RSLT_REG; ? ? ? ? ? ? /* 0x118 */
>> + ? ? u32 SYST_ERROR_REG; ? ? ? ? ? ? /* 0x11C */
> ...
>> + ? ? u32 RAS_GPP1_IN; ? ? ? ? ? ? ? ?/* 0x8000 */
>
> Variable names must be lower case.
>

Ok. Corrected globally

Best Reagrds
Vipin Kumar