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