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

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

2012-03-02 Thread Puneet Saxena 
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 V3:
- Removed local descriptor elements copy to global descriptor elements
- Removed Signed-off-by: Jim Lin ji...@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));
+
/* 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);
-

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 pune...@nvidia.com
 ---
 
 Changes for V3:
 - Removed local descriptor elements copy to global descriptor elements
 - Removed Signed-off-by: Jim Lin ji...@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
___
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 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 Saxenapune...@nvidia.com
---

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

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 Saxenapune...@nvidia.com
  ---
  
  Changes for V3:
   - Removed local descriptor elements copy to global descriptor
   elements - Removed Signed-off-by: Jim Linji...@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

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 Saxenapune...@nvidia.com
---

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

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 Saxenapune...@nvidia.com
  ---
  
  Changes for V3:
- Removed local descriptor elements copy to global descriptor
elements - Removed Signed-off-by: Jim Linji...@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

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