| Message ID | 1330695356-7016-1-git-send-email-puneets@nvidia.com |
|---|---|
| State | Superseded |
| Delegated to: | Marek Vasut |
| Headers | show |
> 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 V3: > - Removed local descriptor elements copy to global descriptor elements > - Removed "Signed-off-by: Jim Lin <jilin@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? It's because dev- >descriptor is unaligned? M
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@nvidia.com> >> --- >> >> Changes for V3: >> - Removed local descriptor elements copy to global descriptor elements >> - Removed "Signed-off-by: Jim Lin<jilin@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. > M Thanx, Puneet ----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. -----------------------------------------------------------------------------------
> 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@nvidia.com> > >> --- > >> > >> Changes for V3: > >> - Removed local descriptor elements copy to global descriptor > >> elements - Removed "Signed-off-by: Jim Lin<jilin@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? M
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@nvidia.com> >>>> --- >>>> >>>> Changes for V3: >>>> - Removed local descriptor elements copy to global descriptor >>>> elements - Removed "Signed-off-by: Jim Lin<jilin@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 Thanx, Puneet ----------------------------------------------------------------------------------- This email message is for the sole use of the intended recipient(s) and may contain confidential information. Any unauthorized review, use, disclosure or distribution is prohibited. If you are not the intended recipient, please contact the sender by reply email and destroy all copies of the original message. -----------------------------------------------------------------------------------
> 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@nvidia.com> > >>>> --- > >>>> > >>>> Changes for V3: > >>>> - Removed local descriptor elements copy to global descriptor > >>>> elements - Removed "Signed-off-by: Jim Lin<jilin@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
In message <201203021659.21568.marex@denx.de> you wrote: > ... > > 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? And eventually wd can also avoid the #ifdef ? I guess the __attribute__((aligned...)) would not hurt anything? Best regards, Wolfgang Denk
On Friday 02 March 2012 11:45:15 Wolfgang Denk wrote: > > > 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? > > And eventually wd can also avoid the #ifdef ? I guess the > __attribute__((aligned...)) would not hurt anything? the reason i disliked that was because it adds padding to the structure. on my system, seems to go from 144 bytes to 160, and the other goes from 1352 to 1376. the scsi structure isn't specific to usb either. i can't tell if this is a structure that represents data on the wire ... the fact it's written all using char types makes me suspicious. if it is, then obviously we can't change the padding in the struct. further, it doesn't seem like Linux imposes this restriction at the structure level (does it do memcopies instead ?), and imposing it on arbitrary members in there w/out documentation easily leads to rot. if the code changes and no longer needs this alignment, how do we tell ? if the code starts transferring another structure, do we end up aligning every member in there until there's padding everywhere ? -mike
Dear Mike Frysinger, > On Friday 02 March 2012 11:45:15 Wolfgang Denk wrote: > > > > 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? > > > > And eventually wd can also avoid the #ifdef ? I guess the > > __attribute__((aligned...)) would not hurt anything? > > the reason i disliked that was because it adds padding to the structure. > on my system, seems to go from 144 bytes to 160, and the other goes from > 1352 to 1376. the scsi structure isn't specific to usb either. i can't > tell if this is a structure that represents data on the wire ... the fact > it's written all using char types makes me suspicious. if it is, then > obviously we can't change the padding in the struct. > > further, it doesn't seem like Linux imposes this restriction at the > structure level (does it do memcopies instead ?), and imposing it on > arbitrary members in there w/out documentation easily leads to rot. if > the code changes and no longer needs this alignment, how do we tell ? if > the code starts transferring another structure, do we end up aligning > every member in there until there's padding everywhere ? > -mike I believe this is OK, we're properly aligning only descriptors. Note that the USB stack in linux and in uboot is different. Best regards, Marek Vasut
On Tuesday 06 March 2012 03:24:34 Marek Vasut wrote: > > On Friday 02 March 2012 11:45:15 Wolfgang Denk wrote: > > > > > 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 > > > > > > > > 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? > > > > > > And eventually wd can also avoid the #ifdef ? I guess the > > > __attribute__((aligned...)) would not hurt anything? > > > > the reason i disliked that was because it adds padding to the structure. > > on my system, seems to go from 144 bytes to 160, and the other goes from > > 1352 to 1376. the scsi structure isn't specific to usb either. i can't > > tell if this is a structure that represents data on the wire ... the fact > > it's written all using char types makes me suspicious. if it is, then > > obviously we can't change the padding in the struct. > > > > further, it doesn't seem like Linux imposes this restriction at the > > structure level (does it do memcopies instead ?), and imposing it on > > arbitrary members in there w/out documentation easily leads to rot. if > > the code changes and no longer needs this alignment, how do we tell ? if > > the code starts transferring another structure, do we end up aligning > > every member in there until there's padding everywhere ? > > I believe this is OK, we're properly aligning only descriptors. you're looking at one change (the one quoted above). i was referring to the scsi structure change (which is not quoted above). > Note that the USB stack in linux and in uboot is different. i know the stacks are different, but they do share. my point was that if Linux is managing this, then why can't we ? or maybe we're fighting over an insignificant memcpy ... the scsi buffer is all of 64 bytes. on some systems, that's like 1 cache line :P. -mike
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)); + /* correct le values */ le16_to_cpus(&dev->descriptor.bcdUSB); le16_to_cpus(&dev->descriptor.idVendor); le16_to_cpus(&dev->descriptor.idProduct); le16_to_cpus(&dev->descriptor.bcdDevice); /* only support for one config for now */ - usb_get_configuration_no(dev, &tmpbuf[0], 0); - usb_parse_config(dev, &tmpbuf[0], 0); + usb_get_configuration_no(dev, tmpbuf, 0); + usb_parse_config(dev, tmpbuf, 0); usb_set_maxpacket(dev); /* we set the default configuration here */ if (usb_set_configuration(dev, dev->config.desc.bConfigurationValue)) { @@ -1080,7 +1085,7 @@ static int hub_port_reset(struct usb_device *dev, int port, unsigned short *portstat) { int tries; - struct usb_port_status portsts; + ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1); unsigned short portstatus, portchange; USB_HUB_PRINTF("hub_port_reset: resetting port %d...\n", port); @@ -1089,13 +1094,13 @@ static int hub_port_reset(struct usb_device *dev, int port, usb_set_port_feature(dev, port + 1, USB_PORT_FEAT_RESET); wait_ms(200); - if (usb_get_port_status(dev, port + 1, &portsts) < 0) { + if (usb_get_port_status(dev, port + 1, portsts) < 0) { USB_HUB_PRINTF("get_port_status failed status %lX\n", dev->status); return -1; } - portstatus = le16_to_cpu(portsts.wPortStatus); - portchange = le16_to_cpu(portsts.wPortChange); + portstatus = le16_to_cpu(portsts->wPortStatus); + portchange = le16_to_cpu(portsts->wPortChange); USB_HUB_PRINTF("portstatus %x, change %x, %s\n", portstatus, portchange, @@ -1133,19 +1138,19 @@ static int hub_port_reset(struct usb_device *dev, int port, void usb_hub_port_connect_change(struct usb_device *dev, int port) { struct usb_device *usb; - struct usb_port_status portsts; + ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1); unsigned short portstatus; /* Check status */ - if (usb_get_port_status(dev, port + 1, &portsts) < 0) { + if (usb_get_port_status(dev, port + 1, portsts) < 0) { USB_HUB_PRINTF("get_port_status failed\n"); return; } - portstatus = le16_to_cpu(portsts.wPortStatus); + portstatus = le16_to_cpu(portsts->wPortStatus); USB_HUB_PRINTF("portstatus %x, change %x, %s\n", portstatus, - le16_to_cpu(portsts.wPortChange), + le16_to_cpu(portsts->wPortChange), portspeed(portstatus)); /* Clear the connection change status */ @@ -1194,7 +1199,8 @@ void usb_hub_port_connect_change(struct usb_device *dev, int port) int usb_hub_configure(struct usb_device *dev) { int i; - unsigned char buffer[USB_BUFSIZ], *bitmap; + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, USB_BUFSIZ); + unsigned char *bitmap; struct usb_hub_descriptor *descriptor; struct usb_hub_device *hub; #ifdef USB_HUB_DEBUG @@ -1316,16 +1322,16 @@ int usb_hub_configure(struct usb_device *dev) usb_hub_power_on(hub); for (i = 0; i < dev->maxchild; i++) { - struct usb_port_status portsts; + ALLOC_CACHE_ALIGN_BUFFER(struct usb_port_status, portsts, 1); unsigned short portstatus, portchange; - if (usb_get_port_status(dev, i + 1, &portsts) < 0) { + if (usb_get_port_status(dev, i + 1, portsts) < 0) { USB_HUB_PRINTF("get_port_status failed\n"); continue; } - portstatus = le16_to_cpu(portsts.wPortStatus); - portchange = le16_to_cpu(portsts.wPortChange); + portstatus = le16_to_cpu(portsts->wPortStatus); + portchange = le16_to_cpu(portsts->wPortChange); USB_HUB_PRINTF("Port %d Status %X Change %X\n", i + 1, portstatus, portchange); diff --git a/common/usb_storage.c b/common/usb_storage.c index de84c8d..88ca390 100644 --- a/common/usb_storage.c +++ b/common/usb_storage.c @@ -79,8 +79,7 @@ static const unsigned char us_direction[256/8] = { }; #define US_DIRECTION(x) ((us_direction[x>>3] >> (x & 7)) & 1) -static unsigned char usb_stor_buf[512]; -static ccb usb_ccb; +static ccb usb_ccb __attribute__((aligned(ARCH_DMA_MINALIGN))); /* * CBI style @@ -210,17 +209,17 @@ int usb_stor_info(void) static unsigned int usb_get_max_lun(struct us_data *us) { int len; - unsigned char result; + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, result, 1); len = usb_control_msg(us->pusb_dev, usb_rcvctrlpipe(us->pusb_dev, 0), US_BBB_GET_MAX_LUN, USB_TYPE_CLASS | USB_RECIP_INTERFACE | USB_DIR_IN, 0, us->ifnum, - &result, sizeof(result), + result, sizeof(char), USB_CNTL_TIMEOUT * 5); USB_STOR_PRINTF("Get Max LUN -> len = %i, result = %i\n", - len, (int) result); - return (len > 0) ? result : 0; + len, (int) *result); + return (len > 0) ? *result : 0; } /******************************************************************************* @@ -233,9 +232,6 @@ int usb_stor_scan(int mode) unsigned char i; struct usb_device *dev; - /* GJ */ - memset(usb_stor_buf, 0, sizeof(usb_stor_buf)); - if (mode == 1) printf(" scanning bus for storage devices... "); @@ -499,7 +495,7 @@ int usb_stor_BBB_comdat(ccb *srb, struct us_data *us) int actlen; int dir_in; unsigned int pipe; - umass_bbb_cbw_t cbw; + ALLOC_CACHE_ALIGN_BUFFER(umass_bbb_cbw_t, cbw, 1); dir_in = US_DIRECTION(srb->cmd[0]); @@ -522,16 +518,16 @@ int usb_stor_BBB_comdat(ccb *srb, struct us_data *us) /* always OUT to the ep */ pipe = usb_sndbulkpipe(us->pusb_dev, us->ep_out); - cbw.dCBWSignature = cpu_to_le32(CBWSIGNATURE); - cbw.dCBWTag = cpu_to_le32(CBWTag++); - cbw.dCBWDataTransferLength = cpu_to_le32(srb->datalen); - cbw.bCBWFlags = (dir_in ? CBWFLAGS_IN : CBWFLAGS_OUT); - cbw.bCBWLUN = srb->lun; - cbw.bCDBLength = srb->cmdlen; + cbw->dCBWSignature = cpu_to_le32(CBWSIGNATURE); + cbw->dCBWTag = cpu_to_le32(CBWTag++); + cbw->dCBWDataTransferLength = cpu_to_le32(srb->datalen); + cbw->bCBWFlags = (dir_in ? CBWFLAGS_IN : CBWFLAGS_OUT); + cbw->bCBWLUN = srb->lun; + cbw->bCDBLength = srb->cmdlen; /* copy the command data into the CBW command data buffer */ /* DST SRC LEN!!! */ - memcpy(cbw.CBWCDB, srb->cmd, srb->cmdlen); - result = usb_bulk_msg(us->pusb_dev, pipe, &cbw, UMASS_BBB_CBW_SIZE, + memcpy(cbw->CBWCDB, srb->cmd, srb->cmdlen); + result = usb_bulk_msg(us->pusb_dev, pipe, cbw, UMASS_BBB_CBW_SIZE, &actlen, USB_CNTL_TIMEOUT * 5); if (result < 0) USB_STOR_PRINTF("usb_stor_BBB_comdat:usb_bulk_msg error\n"); @@ -675,7 +671,7 @@ int usb_stor_BBB_transport(ccb *srb, struct us_data *us) int dir_in; int actlen, data_actlen; unsigned int pipe, pipein, pipeout; - umass_bbb_csw_t csw; + ALLOC_CACHE_ALIGN_BUFFER(umass_bbb_csw_t, csw, 1); #ifdef BBB_XPORT_TRACE unsigned char *ptr; int index; @@ -733,7 +729,7 @@ st: retry = 0; again: USB_STOR_PRINTF("STATUS phase\n"); - result = usb_bulk_msg(us->pusb_dev, pipein, &csw, UMASS_BBB_CSW_SIZE, + result = usb_bulk_msg(us->pusb_dev, pipein, csw, UMASS_BBB_CSW_SIZE, &actlen, USB_CNTL_TIMEOUT*5); /* special handling of STALL in STATUS phase */ @@ -753,28 +749,28 @@ again: return USB_STOR_TRANSPORT_FAILED; } #ifdef BBB_XPORT_TRACE - ptr = (unsigned char *)&csw; + ptr = (unsigned char *)csw; for (index = 0; index < UMASS_BBB_CSW_SIZE; index++) printf("ptr[%d] %#x ", index, ptr[index]); printf("\n"); #endif /* misuse pipe to get the residue */ - pipe = le32_to_cpu(csw.dCSWDataResidue); + pipe = le32_to_cpu(csw->dCSWDataResidue); if (pipe == 0 && srb->datalen != 0 && srb->datalen - data_actlen != 0) pipe = srb->datalen - data_actlen; - if (CSWSIGNATURE != le32_to_cpu(csw.dCSWSignature)) { + if (CSWSIGNATURE != le32_to_cpu(csw->dCSWSignature)) { USB_STOR_PRINTF("!CSWSIGNATURE\n"); usb_stor_BBB_reset(us); return USB_STOR_TRANSPORT_FAILED; - } else if ((CBWTag - 1) != le32_to_cpu(csw.dCSWTag)) { + } else if ((CBWTag - 1) != le32_to_cpu(csw->dCSWTag)) { USB_STOR_PRINTF("!Tag\n"); usb_stor_BBB_reset(us); return USB_STOR_TRANSPORT_FAILED; - } else if (csw.bCSWStatus > CSWSTATUS_PHASE) { + } else if (csw->bCSWStatus > CSWSTATUS_PHASE) { USB_STOR_PRINTF(">PHASE\n"); usb_stor_BBB_reset(us); return USB_STOR_TRANSPORT_FAILED; - } else if (csw.bCSWStatus == CSWSTATUS_PHASE) { + } else if (csw->bCSWStatus == CSWSTATUS_PHASE) { USB_STOR_PRINTF("=PHASE\n"); usb_stor_BBB_reset(us); return USB_STOR_TRANSPORT_FAILED; @@ -782,7 +778,7 @@ again: USB_STOR_PRINTF("transferred %dB instead of %ldB\n", data_actlen, srb->datalen); return USB_STOR_TRANSPORT_FAILED; - } else if (csw.bCSWStatus == CSWSTATUS_FAILED) { + } else if (csw->bCSWStatus == CSWSTATUS_FAILED) { USB_STOR_PRINTF("FAILED\n"); return USB_STOR_TRANSPORT_FAILED; } @@ -1343,7 +1339,8 @@ int usb_stor_get_info(struct usb_device *dev, struct us_data *ss, block_dev_desc_t *dev_desc) { unsigned char perq, modi; - unsigned long cap[2]; + ALLOC_CACHE_ALIGN_BUFFER(unsigned long, cap, 2); + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, usb_stor_buf, 36); unsigned long *capacity, *blksz; ccb *pccb = &usb_ccb; @@ -1367,9 +1364,9 @@ int usb_stor_get_info(struct usb_device *dev, struct us_data *ss, /* drive is removable */ dev_desc->removable = 1; } - memcpy(&dev_desc->vendor[0], &usb_stor_buf[8], 8); - memcpy(&dev_desc->product[0], &usb_stor_buf[16], 16); - memcpy(&dev_desc->revision[0], &usb_stor_buf[32], 4); + memcpy(&dev_desc->vendor[0], (const void *) &usb_stor_buf[8], 8); + memcpy(&dev_desc->product[0], (const void *) &usb_stor_buf[16], 16); + memcpy(&dev_desc->revision[0], (const void *) &usb_stor_buf[32], 4); dev_desc->vendor[8] = 0; dev_desc->product[16] = 0; dev_desc->revision[4] = 0; diff --git a/disk/part_dos.c b/disk/part_dos.c index b5bcb37..70211ee 100644 --- a/disk/part_dos.c +++ b/disk/part_dos.c @@ -87,7 +87,7 @@ static int test_block_type(unsigned char *buffer) int test_part_dos (block_dev_desc_t *dev_desc) { - unsigned char buffer[dev_desc->blksz]; + ALLOC_CACHE_ALIGN_BUFFER(unsigned char, buffer, dev_desc->blksz); if ((dev_desc->block_read(dev_desc->dev, 0, 1, (ulong *) buffer) != 1) || (buffer[DOS_PART_MAGIC_OFFSET + 0] != 0x55) || diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c index d893b2a..eb5220b 100644 --- a/drivers/usb/host/ehci-hcd.c +++ b/drivers/usb/host/ehci-hcd.c @@ -120,6 +120,14 @@ static struct descriptor { */ static void flush_invalidate(u32 addr, int size, int flush) { + /* + * Size is the bytes actually moved during transaction, + * which may not equal to the cache line. This results + * stop address passed for invalidating cache may not be aligned. + * Therfore making size as multiple of cache line size. + */ + size = ALIGN(size, ARCH_DMA_MINALIGN); + if (flush) flush_dcache_range(addr, addr + size); else diff --git a/include/scsi.h b/include/scsi.h index c52759c..89ae45f 100644 --- a/include/scsi.h +++ b/include/scsi.h @@ -26,7 +26,9 @@ typedef struct SCSI_cmd_block{ unsigned char cmd[16]; /* command */ - unsigned char sense_buf[64]; /* for request sense */ + /* for request sense */ + unsigned char sense_buf[64] + __attribute__((aligned(ARCH_DMA_MINALIGN))); unsigned char status; /* SCSI Status */ unsigned char target; /* Target ID */ unsigned char lun; /* Target LUN */
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 V3: - Removed local descriptor elements copy to global descriptor elements - Removed "Signed-off-by: Jim Lin <jilin@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(-)