> 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<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? > > 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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot