[U-Boot] [PATCH v4] usb: align buffers at cacheline
Marek Vasut
marex at denx.de
Fri Mar 2 16:59:21 CET 2012
> On Friday 02 March 2012 08:11 PM, Marek Vasut wrote:
> >> On Friday 02 March 2012 07:16 PM, 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 V3:
> >>>> - Removed local descriptor elements copy to global descriptor
> >>>> elements - Removed "Signed-off-by: Jim Lin<jilin at nvidia.com>"
> >>>> from commit
> >>>>
> >>>> message
> >>>>
> >>>> Changes for V4:
> >>>> - Added memcpy to copy local descriptor to global descriptor.
> >>>>
> >>>> Without that, USB version, class, vendor, product Id...etc is
> >>>> not
> >>>>
> >>>> configured. This information is useful for loading correct device
> >>>> driver and possible configuration.
> >>>>
> >>>> common/cmd_usb.c | 3 +-
> >>>> common/usb.c | 56
> >>>>
> >>>> ++++++++++++++++++++++------------------ 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(+), 59 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 6e21ae2..42a44e2 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;
> >>>>
> >>>> @@ -698,7 +699,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;
> >>>>
> >>>> @@ -798,7 +799,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;
> >>>>
> >>>> @@ -909,7 +910,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));
> >>>>
> >>>> if (err< tmp) {
> >>>>
> >>>> if (err< 0)
> >>>>
> >>>> printf("unable to get device descriptor
(error=%d)\n",
> >>>>
> >>>> @@ -919,14 +920,18 @@ int usb_new_device(struct usb_device *dev)
> >>>>
> >>>> "(expected %i, got %i)\n", tmp, err);
> >>>>
> >>>> return 1;
> >>>>
> >>>> }
> >>>>
> >>>> +
> >>>> + /* Now copy the local device descriptor to global device
> >>>> descriptor*/ + memcpy(&dev->descriptor, desc,
> >>>> sizeof(dev->descriptor));
> >>>
> >>> Hey, it's almost perfect!
> >>>
> >>> Just one last question -- why do you need to copy this stuff?
> >>
> >> We need to copy this stuff as I spoke in previous reply -
> >> "memcpy" is needed to configure Usb version, vendor id, prod Id ..etc
> >>
> >>> of the device. Its also useful to hook actual device driver and
> >>> detect no. of configuration supported. The effect can be verified
> >>
> >> with/without using memcpy
> >> in "usb tree" and "usb info" commands.
> >>
> >>> It's because dev-
> >>>
> >>>> descriptor is unaligned?
> >>
> >> No, As global dev - descriptor is unaligned we are passing aligned local
> >> dev-desc and memcpy in global.
> >
> > Can't we just align the global one?
>
> That's what I did in original patch where I am aligning it by adding the
> line
>
> + /* Device Descriptor */
> +#ifdef ARCH_DMA_MINALIGN
> + struct usb_device_descriptor descriptor
> + __attribute__((aligned(ARCH_DMA_MINALIGN)));
> +#else
> + struct usb_device_descriptor descriptor;
> +#endif
>
> in usb.h Line:112
>
> > M
>
I see ...and I told you it's wrong? I must have misunderstood, I'm sorry about
that. But if you actually do this, you can avoid memcpy, right?
M
More information about the U-Boot
mailing list