[U-Boot] [PATCH v3 1/2] usb: align buffers at cacheline
Marek Vasut
marex at denx.de
Fri Mar 2 13:58:03 CET 2012
> Hi,
>
> On Friday 02 March 2012 04:13 PM, Marek Vasut wrote:
> >> Hi,
> >>
> >> On Friday 02 March 2012 12:08 AM, Marek Vasut wrote:
> >>>> 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?
> >>
> >> Yes, I will send this fix in separate patch. To address partial
> >> invalidate issue
> >> will send another patch in "ehci_hcd()".
> >
> > Very good! I'm really glad we're working towards the proper solution,
> > thanks for all the effort you put into it!
> >
> >>>>>> 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 :-(
> >>
> >> Actually in previous comments it was said to not cache align "struct
> >> usb_device_descriptor descriptor; /* Device Descriptor */ Line:112
> >> usb.h, in header file. So another way is to define cache aligned local
> >> variable and pass it to "usb_get_descriptor". After returning from
> >> "usb_get_descriptor", memcpy this buffer to global variable
> >> "dev->descriptor".
> >> I verified the devices, usb mouse, mass-storage etc... that without this
> >> memcpy to global variable, its not breaking anything.
> >> That's why I avoided memcpy.
> >
> > Oh ... maybe the global variable is unneeded at all and using the local
> > only is OK?
>
> Investigated further and found that "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.
>
> I strongly feel we could avoid "memcpy" by making global device desc in
> usb.h Line:112, cache align as done in
> scsi.h,Line:30 in this patch and this is accepted also?
We try to make as many things local as possible.
M
More information about the U-Boot
mailing list