Re: [U-Boot] [PATCH 2/2] usb: gadget: Use unaligned access for wMaxPacketSize

2013-05-14 Thread Piotr Wilczek 

 -Original Message-
 From: Vivek Gautam [mailto:gautam.vi...@samsung.com]
 Sent: Monday, May 13, 2013 12:24 PM
 To: u-boot@lists.denx.de
 Cc: gautam.vi...@samsung.com; Lukasz Majewski; Piotr Wilczek; Kyungmin
 Park; Lukasz Dalek; Marek Vasut
 Subject: [PATCH 2/2] usb: gadget: Use unaligned access for
 wMaxPacketSize
 
 Use get_unaligned() while fetching wMaxPacketSize to avoid voilating
 any alignment rules.
 
 Signed-off-by: Vivek Gautam gautam.vi...@samsung.com
 Cc: Lukasz Majewski l.majew...@samsung.com
 Cc: Piotr Wilczek p.wilc...@samsung.com
 Cc: Kyungmin Park kyungmin.p...@samsung.com
 Cc: Lukasz Dalek luk0...@gmail.com
 Cc: Marek Vasut ma...@denx.de
 ---
 
 Just did a build test on u-boot-usb/master branch.
 Need to be tested further.
 
 Based on u-boot-usb/next.
 
  drivers/usb/gadget/f_mass_storage.c |3 ++-
  drivers/usb/gadget/pxa25x_udc.c |   13 +++--
  2 files changed, 9 insertions(+), 7 deletions(-)
 

Tested 'f_mass_storage.c' on u-boot/samsung.
It works fine with or without this patch on Trats2.

Best regards,
Piotr Wilczek



___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 2/2] usb: gadget: Use unaligned access for wMaxPacketSize

2013-05-14 Thread Marek Vasut 
Dear Vivek Gautam,

 Use get_unaligned() while fetching wMaxPacketSize to avoid
 voilating any alignment rules.
 
 Signed-off-by: Vivek Gautam gautam.vi...@samsung.com
 Cc: Lukasz Majewski l.majew...@samsung.com
 Cc: Piotr Wilczek p.wilc...@samsung.com
 Cc: Kyungmin Park kyungmin.p...@samsung.com
 Cc: Lukasz Dalek luk0...@gmail.com
 Cc: Marek Vasut ma...@denx.de

I'll pick both, thanks.

Best regards,
Marek Vasut
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

[U-Boot] [PATCH 2/2] usb: gadget: Use unaligned access for wMaxPacketSize

2013-05-13 Thread Vivek Gautam 
Use get_unaligned() while fetching wMaxPacketSize to avoid
voilating any alignment rules.

Signed-off-by: Vivek Gautam gautam.vi...@samsung.com
Cc: Lukasz Majewski l.majew...@samsung.com
Cc: Piotr Wilczek p.wilc...@samsung.com
Cc: Kyungmin Park kyungmin.p...@samsung.com
Cc: Lukasz Dalek luk0...@gmail.com
Cc: Marek Vasut ma...@denx.de
---

Just did a build test on u-boot-usb/master branch.
Need to be tested further.

Based on u-boot-usb/next.

 drivers/usb/gadget/f_mass_storage.c |3 ++-
 drivers/usb/gadget/pxa25x_udc.c |   13 +++--
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/usb/gadget/f_mass_storage.c 
b/drivers/usb/gadget/f_mass_storage.c
index c28866f..45bc132 100644
--- a/drivers/usb/gadget/f_mass_storage.c
+++ b/drivers/usb/gadget/f_mass_storage.c
@@ -2261,7 +2261,8 @@ reset:
if (rc)
goto reset;
fsg-bulk_out_enabled = 1;
-   common-bulk_out_maxpacket = le16_to_cpu(d-wMaxPacketSize);
+   common-bulk_out_maxpacket =
+   le16_to_cpu(get_unaligned(d-wMaxPacketSize));
clear_bit(IGNORE_BULK_OUT, fsg-atomic_bitflags);
 
/* Allocate the requests */
diff --git a/drivers/usb/gadget/pxa25x_udc.c b/drivers/usb/gadget/pxa25x_udc.c
index 9ce98f0..085503d 100644
--- a/drivers/usb/gadget/pxa25x_udc.c
+++ b/drivers/usb/gadget/pxa25x_udc.c
@@ -314,7 +314,8 @@ static int pxa25x_ep_enable(struct usb_ep *_ep,
if (!_ep || !desc || ep-desc || _ep-name == ep0name
|| desc-bDescriptorType != USB_DT_ENDPOINT
|| ep-bEndpointAddress != desc-bEndpointAddress
-   || ep-fifo_size  le16_to_cpu(desc-wMaxPacketSize)) {
+   || ep-fifo_size 
+  le16_to_cpu(get_unaligned(desc-wMaxPacketSize))) {
printf(%s, bad ep or descriptor\n, __func__);
return -EINVAL;
}
@@ -329,9 +330,9 @@ static int pxa25x_ep_enable(struct usb_ep *_ep,
 
/* hardware _could_ do smaller, but driver doesn't */
if ((desc-bmAttributes == USB_ENDPOINT_XFER_BULK
-le16_to_cpu(desc-wMaxPacketSize)
+le16_to_cpu(get_unaligned(desc-wMaxPacketSize))
!= BULK_FIFO_SIZE)
-   || !desc-wMaxPacketSize) {
+   || !get_unaligned(desc-wMaxPacketSize)) {
printf(%s, bad %s maxpacket\n, __func__, _ep-name);
return -ERANGE;
}
@@ -345,7 +346,7 @@ static int pxa25x_ep_enable(struct usb_ep *_ep,
ep-desc = desc;
ep-stopped = 0;
ep-pio_irqs = 0;
-   ep-ep.maxpacket = le16_to_cpu(desc-wMaxPacketSize);
+   ep-ep.maxpacket = le16_to_cpu(get_unaligned(desc-wMaxPacketSize));
 
/* flush fifo (mostly for OUT buffers) */
pxa25x_ep_fifo_flush(_ep);
@@ -485,7 +486,7 @@ write_fifo(struct pxa25x_ep *ep, struct pxa25x_request *req)
 {
unsigned max;
 
-   max = le16_to_cpu(ep-desc-wMaxPacketSize);
+   max = le16_to_cpu(get_unaligned(ep-desc-wMaxPacketSize));
do {
unsigned count;
int is_last, is_short;
@@ -766,7 +767,7 @@ pxa25x_ep_queue(struct usb_ep *_ep, struct usb_request 
*_req, gfp_t gfp_flags)
 */
if (unlikely(ep-bmAttributes == USB_ENDPOINT_XFER_ISOC
 req-req.length 
-   le16_to_cpu(ep-desc-wMaxPacketSize)))
+   le16_to_cpu(get_unaligned(ep-desc-wMaxPacketSize
return -EMSGSIZE;
 
debug_cond(NOISY, %s queue req %p, len %d buf %p\n,
-- 
1.7.6.5

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 2/2] usb: gadget: Use unaligned access for wMaxPacketSize

2013-05-13 Thread Kyungmin Park 
On Mon, May 13, 2013 at 7:23 PM, Vivek Gautam gautam.vi...@samsung.comwrote:

 Use get_unaligned() while fetching wMaxPacketSize to avoid
 voilating any alignment rules.


It's another story, can we get performance gain with unaligned access
feature? In case of kernel, we got some gains.

Anyway, good job!

Acked-by: Kyungmin Park kyungmin.p...@samsung.com


 Signed-off-by: Vivek Gautam gautam.vi...@samsung.com
 Cc: Lukasz Majewski l.majew...@samsung.com
 Cc: Piotr Wilczek p.wilc...@samsung.com
 Cc: Kyungmin Park kyungmin.p...@samsung.com
 Cc: Lukasz Dalek luk0...@gmail.com
 Cc: Marek Vasut ma...@denx.de
 ---

 Just did a build test on u-boot-usb/master branch.
 Need to be tested further.

 Based on u-boot-usb/next.

  drivers/usb/gadget/f_mass_storage.c |3 ++-
  drivers/usb/gadget/pxa25x_udc.c |   13 +++--
  2 files changed, 9 insertions(+), 7 deletions(-)

 diff --git a/drivers/usb/gadget/f_mass_storage.c
 b/drivers/usb/gadget/f_mass_storage.c
 index c28866f..45bc132 100644
 --- a/drivers/usb/gadget/f_mass_storage.c
 +++ b/drivers/usb/gadget/f_mass_storage.c
 @@ -2261,7 +2261,8 @@ reset:
 if (rc)
 goto reset;
 fsg-bulk_out_enabled = 1;
 -   common-bulk_out_maxpacket = le16_to_cpu(d-wMaxPacketSize);
 +   common-bulk_out_maxpacket =
 +
 le16_to_cpu(get_unaligned(d-wMaxPacketSize));
 clear_bit(IGNORE_BULK_OUT, fsg-atomic_bitflags);

 /* Allocate the requests */
 diff --git a/drivers/usb/gadget/pxa25x_udc.c
 b/drivers/usb/gadget/pxa25x_udc.c
 index 9ce98f0..085503d 100644
 --- a/drivers/usb/gadget/pxa25x_udc.c
 +++ b/drivers/usb/gadget/pxa25x_udc.c
 @@ -314,7 +314,8 @@ static int pxa25x_ep_enable(struct usb_ep *_ep,
 if (!_ep || !desc || ep-desc || _ep-name == ep0name
 || desc-bDescriptorType != USB_DT_ENDPOINT
 || ep-bEndpointAddress != desc-bEndpointAddress
 -   || ep-fifo_size 
 le16_to_cpu(desc-wMaxPacketSize)) {
 +   || ep-fifo_size 
 +
  le16_to_cpu(get_unaligned(desc-wMaxPacketSize))) {
 printf(%s, bad ep or descriptor\n, __func__);
 return -EINVAL;
 }
 @@ -329,9 +330,9 @@ static int pxa25x_ep_enable(struct usb_ep *_ep,

 /* hardware _could_ do smaller, but driver doesn't */
 if ((desc-bmAttributes == USB_ENDPOINT_XFER_BULK
 -le16_to_cpu(desc-wMaxPacketSize)
 +   
 le16_to_cpu(get_unaligned(desc-wMaxPacketSize))
 != BULK_FIFO_SIZE)
 -   || !desc-wMaxPacketSize) {
 +   || !get_unaligned(desc-wMaxPacketSize)) {
 printf(%s, bad %s maxpacket\n, __func__, _ep-name);
 return -ERANGE;
 }
 @@ -345,7 +346,7 @@ static int pxa25x_ep_enable(struct usb_ep *_ep,
 ep-desc = desc;
 ep-stopped = 0;
 ep-pio_irqs = 0;
 -   ep-ep.maxpacket = le16_to_cpu(desc-wMaxPacketSize);
 +   ep-ep.maxpacket =
 le16_to_cpu(get_unaligned(desc-wMaxPacketSize));

 /* flush fifo (mostly for OUT buffers) */
 pxa25x_ep_fifo_flush(_ep);
 @@ -485,7 +486,7 @@ write_fifo(struct pxa25x_ep *ep, struct pxa25x_request
 *req)
  {
 unsigned max;

 -   max = le16_to_cpu(ep-desc-wMaxPacketSize);
 +   max = le16_to_cpu(get_unaligned(ep-desc-wMaxPacketSize));
 do {
 unsigned count;
 int is_last, is_short;
 @@ -766,7 +767,7 @@ pxa25x_ep_queue(struct usb_ep *_ep, struct usb_request
 *_req, gfp_t gfp_flags)
  */
 if (unlikely(ep-bmAttributes == USB_ENDPOINT_XFER_ISOC
  req-req.length 
 -   le16_to_cpu(ep-desc-wMaxPacketSize)))
 +
 le16_to_cpu(get_unaligned(ep-desc-wMaxPacketSize
 return -EMSGSIZE;

 debug_cond(NOISY, %s queue req %p, len %d buf %p\n,
 --
 1.7.6.5

 ___
 U-Boot mailing list
 U-Boot@lists.denx.de
 http://lists.denx.de/mailman/listinfo/u-boot

___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 2/2] usb: gadget: Use unaligned access for wMaxPacketSize

2013-05-13 Thread Vivek Gautam 
Hi,


On Mon, May 13, 2013 at 4:20 PM, Kyungmin Park kmp...@infradead.org wrote:
 On Mon, May 13, 2013 at 7:23 PM, Vivek Gautam gautam.vi...@samsung.comwrote:

 Use get_unaligned() while fetching wMaxPacketSize to avoid
 voilating any alignment rules.

typo here s/voilating/violating



 It's another story, can we get performance gain with unaligned access
 feature? In case of kernel, we got some gains.

This change was necessary since we get data abort if we try to fetch
wMaxPacketSize
which violates the natural alignment rules being a member of struct
usb_endpoint_descriptor.

I am not sure of the performance gains with this. However, if you can
guide me how to
measure that, we can get the numbers if any.


 Anyway, good job!

Thanks !


 Acked-by: Kyungmin Park kyungmin.p...@samsung.com


 Signed-off-by: Vivek Gautam gautam.vi...@samsung.com
 Cc: Lukasz Majewski l.majew...@samsung.com
 Cc: Piotr Wilczek p.wilc...@samsung.com
 Cc: Kyungmin Park kyungmin.p...@samsung.com
 Cc: Lukasz Dalek luk0...@gmail.com
 Cc: Marek Vasut ma...@denx.de
 ---

 Just did a build test on u-boot-usb/master branch.
 Need to be tested further.

 Based on u-boot-usb/next.

  drivers/usb/gadget/f_mass_storage.c |3 ++-
  drivers/usb/gadget/pxa25x_udc.c |   13 +++--
  2 files changed, 9 insertions(+), 7 deletions(-)

 diff --git a/drivers/usb/gadget/f_mass_storage.c
 b/drivers/usb/gadget/f_mass_storage.c
 index c28866f..45bc132 100644
 --- a/drivers/usb/gadget/f_mass_storage.c
 +++ b/drivers/usb/gadget/f_mass_storage.c
 @@ -2261,7 +2261,8 @@ reset:
 if (rc)
 goto reset;
 fsg-bulk_out_enabled = 1;
 -   common-bulk_out_maxpacket = le16_to_cpu(d-wMaxPacketSize);
 +   common-bulk_out_maxpacket =
 +
 le16_to_cpu(get_unaligned(d-wMaxPacketSize));
 clear_bit(IGNORE_BULK_OUT, fsg-atomic_bitflags);

 /* Allocate the requests */
 diff --git a/drivers/usb/gadget/pxa25x_udc.c
 b/drivers/usb/gadget/pxa25x_udc.c
 index 9ce98f0..085503d 100644
 --- a/drivers/usb/gadget/pxa25x_udc.c
 +++ b/drivers/usb/gadget/pxa25x_udc.c
 @@ -314,7 +314,8 @@ static int pxa25x_ep_enable(struct usb_ep *_ep,
 if (!_ep || !desc || ep-desc || _ep-name == ep0name
 || desc-bDescriptorType != USB_DT_ENDPOINT
 || ep-bEndpointAddress != desc-bEndpointAddress
 -   || ep-fifo_size 
 le16_to_cpu(desc-wMaxPacketSize)) {
 +   || ep-fifo_size 
 +
  le16_to_cpu(get_unaligned(desc-wMaxPacketSize))) {
 printf(%s, bad ep or descriptor\n, __func__);
 return -EINVAL;
 }
 @@ -329,9 +330,9 @@ static int pxa25x_ep_enable(struct usb_ep *_ep,

 /* hardware _could_ do smaller, but driver doesn't */
 if ((desc-bmAttributes == USB_ENDPOINT_XFER_BULK
 -le16_to_cpu(desc-wMaxPacketSize)
 +   
 le16_to_cpu(get_unaligned(desc-wMaxPacketSize))
 != BULK_FIFO_SIZE)
 -   || !desc-wMaxPacketSize) {
 +   || !get_unaligned(desc-wMaxPacketSize)) {
 printf(%s, bad %s maxpacket\n, __func__, _ep-name);
 return -ERANGE;
 }
 @@ -345,7 +346,7 @@ static int pxa25x_ep_enable(struct usb_ep *_ep,
 ep-desc = desc;
 ep-stopped = 0;
 ep-pio_irqs = 0;
 -   ep-ep.maxpacket = le16_to_cpu(desc-wMaxPacketSize);
 +   ep-ep.maxpacket =
 le16_to_cpu(get_unaligned(desc-wMaxPacketSize));

 /* flush fifo (mostly for OUT buffers) */
 pxa25x_ep_fifo_flush(_ep);
 @@ -485,7 +486,7 @@ write_fifo(struct pxa25x_ep *ep, struct pxa25x_request
 *req)
  {
 unsigned max;

 -   max = le16_to_cpu(ep-desc-wMaxPacketSize);
 +   max = le16_to_cpu(get_unaligned(ep-desc-wMaxPacketSize));
 do {
 unsigned count;
 int is_last, is_short;
 @@ -766,7 +767,7 @@ pxa25x_ep_queue(struct usb_ep *_ep, struct usb_request
 *_req, gfp_t gfp_flags)
  */
 if (unlikely(ep-bmAttributes == USB_ENDPOINT_XFER_ISOC
  req-req.length 
 -   le16_to_cpu(ep-desc-wMaxPacketSize)))
 +
 le16_to_cpu(get_unaligned(ep-desc-wMaxPacketSize
 return -EMSGSIZE;

 debug_cond(NOISY, %s queue req %p, len %d buf %p\n,
 --
 1.7.6.5

 ___
 U-Boot mailing list
 U-Boot@lists.denx.de
 http://lists.denx.de/mailman/listinfo/u-boot


 ___
 U-Boot mailing list
 U-Boot@lists.denx.de
 http://lists.denx.de/mailman/listinfo/u-boot




-- 
Best Regards
Vivek
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 2/2] usb: gadget: Use unaligned access for wMaxPacketSize

2013-05-13 Thread Albert ARIBAUD 
Hi Kyungmin,

On Mon, 13 May 2013 19:50:28 +0900, Kyungmin Park
kmp...@infradead.org wrote:

 On Mon, May 13, 2013 at 7:23 PM, Vivek Gautam gautam.vi...@samsung.comwrote:
 
  Use get_unaligned() while fetching wMaxPacketSize to avoid
  voilating any alignment rules.
 
 
 It's another story, can we get performance gain with unaligned access
 feature? In case of kernel, we got some gains.

Please do not forget that U-Boot does not run on ARMv7+ only; it runs
on many ARM generations and on many non-ARM architectures too. Some of
these targets have a severe penalty on unaligned access; some cannot
even perform unaligned accesses.

File drivers/usb/gadget/f_mass_storage.c is generic and must build and
run for such targets too.

The best approach is to properly align fields wherever possible, and
when not possible, to use unaligned access macros (and if posisble
detect unaligned accesses at runtime).

Amicalement,
-- 
Albert.
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Re: [U-Boot] [PATCH 2/2] usb: gadget: Use unaligned access for wMaxPacketSize

2013-05-13 Thread Łukasz Dałek 

On 13.05.2013 12:23, Vivek Gautam wrote:

Use get_unaligned() while fetching wMaxPacketSize to avoid
voilating any alignment rules.

Signed-off-by: Vivek Gautam gautam.vi...@samsung.com
Cc: Lukasz Majewski l.majew...@samsung.com
Cc: Piotr Wilczek p.wilc...@samsung.com
Cc: Kyungmin Park kyungmin.p...@samsung.com
Cc: Lukasz Dalek luk0...@gmail.com
Cc: Marek Vasut ma...@denx.de
---

Just did a build test on u-boot-usb/master branch.
Need to be tested further.

Based on u-boot-usb/next.

  drivers/usb/gadget/f_mass_storage.c |3 ++-
  drivers/usb/gadget/pxa25x_udc.c |   13 +++--
  2 files changed, 9 insertions(+), 7 deletions(-)


Tested pxa25x_udc.c on u-boot/master on h2200 (PXA255) and works fine.

Yours sincerely,
Lukasz Dalek
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot