[U-Boot] [PATCH v3 1/2] usb: align buffers at cacheline

[Marek Vasut <marex@denx.de>]

> 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@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@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