> 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<pune...@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<ji...@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? > > >>>> 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 > > Thanx, > Puneet No, thank you ! M _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot