Re: [U-Boot] [PATCH v4] usb: align buffers at cacheline

2012-03-06 Thread Mike Frysinger 
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


signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v4] usb: align buffers at cacheline

2012-03-06 Thread Marek Vasut 
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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v4] usb: align buffers at cacheline

2012-03-05 Thread 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


signature.asc
Description: This is a digitally signed message part.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v4] usb: align buffers at cacheline

2012-03-02 Thread Wolfgang Denk 
In message <201203021659.21568.ma...@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

-- 
DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de
Der Horizont vieler Menschen ist ein Kreis mit Radius Null --
und das nennen sie ihren Standpunkt.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH v4] usb: align buffers at cacheline

2012-03-02 Thread Marek Vasut 
> 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
>  ---
>  
>  Changes for V3:
>    - Removed local descriptor elements copy to global descriptor
>    elements - Removed "Signed-off-by: Jim Lin"
>    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_A

Re: [U-Boot] [PATCH v4] usb: align buffers at cacheline

2012-03-02 Thread puneets 

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

Changes for V3:
  - Removed local descriptor elements copy to global descriptor
  elements - Removed "Signed-off-by: Jim Lin" 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 cop

Re: [U-Boot] [PATCH v4] usb: align buffers at cacheline

2012-03-02 Thread Marek Vasut 
> 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
> >> ---
> >> 
> >> Changes for V3:
> >>  - Removed local descriptor elements copy to global descriptor
> >>  elements - Removed "Signed-off-by: Jim Lin" 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 @@

Re: [U-Boot] [PATCH v4] usb: align buffers at cacheline

2012-03-02 Thread puneets 

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

Changes for V3:
 - Removed local descriptor elements copy to global descriptor elements
 - Removed "Signed-off-by: Jim Lin" 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

Re: [U-Boot] [PATCH v4] usb: align buffers at cacheline

2012-03-02 Thread Marek Vasut 
> As DMA expects the buffers to be equal and larger then
> cache lines, This aligns buffers at cacheline.
> 
> Signed-off-by: Puneet Saxena 
> ---
> 
> Changes for V3:
> - Removed local descriptor elements copy to global descriptor elements
> - Removed "Signed-off-by: Jim Lin " 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
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailm