From mboxrd@z Thu Jan  1 00:00:00 1970
From: Amit Virdi <amit.virdi@st.com>
Date: Mon, 9 Apr 2012 14:08:07 +0530
Subject: [U-Boot] [PATCH V2] USB:gadget:designware USB OTG implementation
In-Reply-To: <201204041023.07386.marex@denx.de>
References: <27065c311969ab4ce9d10fba7921ed9f47970d6e.1333462753.git.amit.virdi@st.com>
	<201204041023.07386.marex@denx.de>
Message-ID: <4F829FEF.3080906@st.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

Dear Marek,

[...]

>> +static void dwc_otg_read_packet(struct dwc_ep *ep, u16 bytes)
>> +{
>> +     u32 i;
>> +     int word_count = (bytes + 3) / 4;
>> +     u32 *fifo = dev_if->data_fifo[0];
>> +     u32 *data_buff = (u32 *) ep->xfer_buff;
>> +     u32 unaligned;
>> +     /*
>> +      * This requires reading data from the FIFO into a u32 temp buffer,
>> +      * then moving it into the data buffer.
>> +      */
>> +     if ((bytes<  4)&&  (bytes>  0)) {
>> +             unaligned = readl(fifo);
>> +             memcpy(data_buff,&unaligned, bytes);
>> +     } else {
>> +             for (i = 0; i<  word_count; i++, data_buff++)
>> +                     *data_buff = readl(fifo);
>
> Thinking of this, will this really handle unaligned access of length for example
> 5 ?
>

If data_buff is unaligned, there will be a problem!

>> +     }
>> +}
>> +
>> +/* Handle RX transaction on non-ISO endpoint. */
>> +static void dw_udc_epn_rx(struct dwc_ep *ep, int bcnt)
>> +{
>> +     struct urb *urb;
>> +     struct usb_endpoint_instance *endpoint = dw_find_ep(ep->num);
>> +
>> +     if (endpoint) {
>
> if (!endpoint)
>   return;
>
> ... code ...
>

Ok.


>> +             urb = endpoint->rcv_urb;
>> +
>> +             if (urb) {
>> +                     ep->xfer_buff = urb->buffer + urb->actual_length;
>> +                     dwc_otg_read_packet(ep, bcnt);
>> +                     usbd_rcv_complete(endpoint, bcnt, 0);
>> +             }
>> +     }
>> +}

[...]

>> +     /* program pkt count */
>> +     temp = ep->xfer_len;
>> +     temp |= (1<<  PKTCNT_SHIFT);
>> +     writel(temp,&in_ep_regs->dieptsiz);
>> +
>> +     /* enable EP*/
>
> missing space before ending comment
>

Ok, 'll correct.

>> +     setbits_le32(&in_ep_regs->diepctl, EPENA | CNAK);
>> +
>> +     /* clear TX Fifo Empty intr*/
>> +     writel(NPTXFEMPTY,&core_global_regs->gintsts);
>> +
>> +     setbits_le32(&core_global_regs->gintmsk, NPTXFEMPTY);
>> +
>> +     start = get_timer(0);
>> +     while (!(readl(&core_global_regs->gintsts)&  NPTXFEMPTY)) {
>> +             if (get_timer(start)>  timeout) {
>> +                     printf("%s: NPTXFEMPTY: TimeOUT\n", __func__);
>> +                     WATCHDOG_RESET();
>> +             }
>> +     }
>> +
>> +     /* write to fifo */
>> +     if ((ep->xfer_len<  4)&&  (ep->xfer_len>  0)) {
>> +             memcpy(&unaligned, data_buff, ep->xfer_len);
>> +             *fifo = unaligned;
>> +     } else {
>> +             for (i = 0; i<  dword_count; i++, data_buff++)
>> +                     *fifo = *data_buff;
>
> DTTO, will this handle unaligned xfer of size>  4 properly ?
>

Again, this part needs to be made more generic. I'll amend it in V3.

>> +     }
>> +
>> +     writel(NPTXFEMPTY,&core_global_regs->gintsts);
>> +

[...]

>> +/* This function returns pointer to out ep struct with number num */
>> +static struct dwc_ep *get_out_ep(u32 num)
>> +{
>> +     u32 i;
>> +     int num_out_eps = MAX_EPS_CHANNELS;
>> +     struct dwc_pcd *pcd =&dev_if->pcd;
>> +
>> +     if (num == 0)
>> +             return&pcd->ep0;
>> +
>> +     for (i = 0; i<  num_out_eps; ++i) {
>
> i++ ... ++i has no meaning here (not even a compiler hint).
>

Ok, 'll amend

>> +             if (pcd->out_ep[i].num == num)
>> +                     return&pcd->out_ep[i];
>> +     }
>> +
>> +     return 0;
>> +}
>> +

[...]

>> + *           to the destination buffer
>> + */
>
> Otherwise, I think you did a pretty decent job, one more round and I'm queueing
> this ;-)
>
> .

:)

Sorry for my delayed response.

Best Regards
Amit Virdi