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

[Wolfgang Denk <wd@denx.de>]

Dear Vipin KUMAR,

In message <1260955110-5656-5-git-send-email-vipin.kumar@st.com> you wrote:
> 
> 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.

> 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?


> 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?

> +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.

> +/*
> + * 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?

> +
> +			/* 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.

Too long lines.

> +		/* 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.


> +struct misc_regs {
...
> +	u32 BIST5_RSLT_REG;		/* 0x118 */
> +	u32 SYST_ERROR_REG;		/* 0x11C */
...
> +	u32 RAS_GPP1_IN;		/* 0x8000 */

Variable names must be lower case.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
"Now here's something you're really going to like!"
- Rocket J. Squirrel