[U-Boot] [PATCH v3 1/2] usb: align buffers at cacheline
Marek Vasut
marex at denx.de
Thu Mar 1 19:38:24 CET 2012
> On Thursday 01 March 2012 03:05 AM, Marek Vasut wrote:
> >> As DMA expects the buffers to be equal and larger then
> >> cache lines, This aligns buffers at cacheline.
> >>
> >> Signed-off-by: Puneet Saxena<puneets at nvidia.com>
> >> ---
> >>
> >> Changes for V2:
> >> - Use "ARCH_DMA_MINALIGN" directly
> >> - Use "ALIGN" to align size as cacheline
> >> - Removed headers from usb.h
> >> - Send 8 bytes of device descriptor size to read
> >>
> >> Max packet size
> >>
> >> scsi.h header is needed to avoid extra memcpy from local buffer
> >> to global buffer.
> >>
> >> Changes for V3:
> >> - Removed local descriptor elements copy to global descriptor
> >> elements - Removed "Signed-off-by: Jim Lin<jilin at nvidia.com>" from
> >> commit
> >>
> >> message
> >>
> >> common/cmd_usb.c | 3 +-
> >> common/usb.c | 57
> >>
> >> ++++++++++++++++++++++------------------- common/usb_storage.c |
> >> 59 ++++++++++++++++++++---------------------- disk/part_dos.c
> >>
> >> | 2 +-
> >>
> >> drivers/usb/host/ehci-hcd.c | 8 ++++++
> >> include/scsi.h | 4 ++-
> >> 6 files changed, 73 insertions(+), 60 deletions(-)
> >>
> >> diff --git a/common/cmd_usb.c b/common/cmd_usb.c
> >> index 320667f..bca9d94 100644
> >> --- a/common/cmd_usb.c
> >> +++ b/common/cmd_usb.c
> >> @@ -150,7 +150,8 @@ void usb_display_class_sub(unsigned char dclass,
> >> unsigned char subclass,
> >>
> >> void usb_display_string(struct usb_device *dev, int index)
> >> {
> >>
> >> - char buffer[256];
> >> + ALLOC_CACHE_ALIGN_BUFFER(char, buffer, 256);
> >> +
> >>
> >> if (index != 0) {
> >>
> >> if (usb_string(dev, index,&buffer[0], 256)> 0)
> >>
> >> printf("String: \"%s\"", buffer);
> >>
> >> diff --git a/common/usb.c b/common/usb.c
> >> index 63a11c8..191bc5b 100644
> >> --- a/common/usb.c
> >> +++ b/common/usb.c
> >> @@ -73,7 +73,6 @@ static struct usb_device usb_dev[USB_MAX_DEVICE];
> >>
> >> static int dev_index;
> >> static int running;
> >> static int asynch_allowed;
> >>
> >> -static struct devrequest setup_packet;
> >>
> >> char usb_started; /* flag for the started/stopped USB status */
> >>
> >> @@ -185,23 +184,25 @@ int usb_control_msg(struct usb_device *dev,
> >> unsigned int pipe, unsigned short value, unsigned short index,
> >>
> >> void *data, unsigned short size, int timeout)
> >>
> >> {
> >>
> >> + ALLOC_CACHE_ALIGN_BUFFER(struct devrequest, setup_packet,
> >> + sizeof(struct devrequest));
> >>
> >> if ((timeout == 0)&& (!asynch_allowed)) {
> >>
> >> /* request for a asynch control pipe is not allowed */
> >> return -1;
> >>
> >> }
> >>
> >> /* set setup command */
> >>
> >> - setup_packet.requesttype = requesttype;
> >> - setup_packet.request = request;
> >> - setup_packet.value = cpu_to_le16(value);
> >> - setup_packet.index = cpu_to_le16(index);
> >> - setup_packet.length = cpu_to_le16(size);
> >> + setup_packet->requesttype = requesttype;
> >> + setup_packet->request = request;
> >> + setup_packet->value = cpu_to_le16(value);
> >> + setup_packet->index = cpu_to_le16(index);
> >> + setup_packet->length = cpu_to_le16(size);
> >>
> >> USB_PRINTF("usb_control_msg: request: 0x%X, requesttype: 0x%X, " \
> >>
> >> "value 0x%X index 0x%X length 0x%X\n",
> >> request, requesttype, value, index, size);
> >>
> >> dev->status = USB_ST_NOT_PROC; /*not yet processed */
> >>
> >> - submit_control_msg(dev, pipe, data, size,&setup_packet);
> >> + submit_control_msg(dev, pipe, data, size, setup_packet);
> >>
> >> if (timeout == 0)
> >>
> >> return (int)size;
> >>
> >> @@ -694,7 +695,7 @@ static int usb_string_sub(struct usb_device *dev,
> >> unsigned int langid, */
> >>
> >> int usb_string(struct usb_device *dev, int index, char *buf, size_t
> >> size) {
> >>
> >> - unsigned char mybuf[USB_BUFSIZ];
> >> + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, mybuf, USB_BUFSIZ);
> >>
> >> unsigned char *tbuf;
> >> int err;
> >> unsigned int u, idx;
> >>
> >> @@ -794,7 +795,7 @@ int usb_new_device(struct usb_device *dev)
> >>
> >> {
> >>
> >> int addr, err;
> >> int tmp;
> >>
> >> - unsigned char tmpbuf[USB_BUFSIZ];
> >> + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, tmpbuf, USB_BUFSIZ);
> >>
> >> /* We still haven't set the Address yet */
> >> addr = dev->devnum;
> >>
> >> @@ -842,7 +843,10 @@ int usb_new_device(struct usb_device *dev)
> >>
> >> dev->epmaxpacketin[0] = 64;
> >> dev->epmaxpacketout[0] = 64;
> >>
> >> - err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, 64);
> >> + desc->bMaxPacketSize0 = 0;
> >> + /*8 bytes of the descriptor to read Max packet size*/
> >> + err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc,
> >> + 8);
> >
> > Did some unrelated addition/change creep in here?
>
> No, This is the fix for the similar issue as is discussed for string
> fetch().
> When the device partially fills the passed buffer and we try to
> invalidate the partial buffer
> the cache alignment error crops up.
>
> The code in "ehci_submit_async() " invalidates *partially* the passed
> buffer though we pass aligned buffer after "STD_ASS"
> is received. Actually it should invalidate only the cached line which is
> equal(~32) to device desc length.
>
> If we pass actual device desc length the cache alignment error does not
> spew.
> Note that here we are aligning the buffer still the error comes.
Then please send this fix as a separate patch. And I think ehci_hcd is what
should be fixed then as I said in the other email, or am I wrong?
>
> >> if (err< 0) {
> >>
> >> USB_PRINTF("usb_new_device: usb_get_descriptor() failed\n");
> >> return 1;
> >>
> >> @@ -905,7 +909,7 @@ int usb_new_device(struct usb_device *dev)
> >>
> >> tmp = sizeof(dev->descriptor);
> >>
> >> err = usb_get_descriptor(dev, USB_DT_DEVICE, 0,
> >>
> >> - &dev->descriptor, sizeof(dev->descriptor));
> >> + desc, sizeof(dev->descriptor));
> >
> > Won't this change (desc) break anything?
>
> Its not breaking any thing. For safer side we could add memcpy to copy
> from local desc
> to global desc. What you say?
What do you mean? So you changed the use from some global variable to different
(local) variable? This might break stuff I fear :-(
>
> >> if (err< tmp) {
> >>
> >> if (err< 0)
> >>
> >> printf("unable to get device descriptor (error=%d)\n",
> >
> > The rest seems fine, from now on it seems to be only matter of trivial
> > fix. Thanks for your effort so far!
> >
> > M
>
> If rest of the code is fine in [Patch V3 1/2] except these two issue can
> it be acknowledged for up-streaming?
Well, there are those two issues which I'd really prefer to be fixed before
accepting the code. I believe you can understand why.
Thanks!
M
More information about the U-Boot
mailing list