| Message ID | 1368440618-11359-2-git-send-email-gautam.vivek@samsung.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Marek Vasut |
| Headers | show |
On Mon, May 13, 2013 at 7:23 PM, Vivek Gautam <gautam.vivek@samsung.com>wrote: > 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.park@samsung.com> > > Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com> > Cc: Lukasz Majewski <l.majewski@samsung.com> > Cc: Piotr Wilczek <p.wilczek@samsung.com> > Cc: Kyungmin Park <kyungmin.park@samsung.com> > Cc: Lukasz Dalek <luk0104@gmail.com> > Cc: Marek Vasut <marex@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 >
Hi, On Mon, May 13, 2013 at 4:20 PM, Kyungmin Park <kmpark@infradead.org> wrote: > On Mon, May 13, 2013 at 7:23 PM, Vivek Gautam <gautam.vivek@samsung.com>wrote: > >> 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.park@samsung.com> > >> >> Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com> >> Cc: Lukasz Majewski <l.majewski@samsung.com> >> Cc: Piotr Wilczek <p.wilczek@samsung.com> >> Cc: Kyungmin Park <kyungmin.park@samsung.com> >> Cc: Lukasz Dalek <luk0104@gmail.com> >> Cc: Marek Vasut <marex@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 >
Hi Kyungmin, On Mon, 13 May 2013 19:50:28 +0900, Kyungmin Park <kmpark@infradead.org> wrote: > On Mon, May 13, 2013 at 7:23 PM, Vivek Gautam <gautam.vivek@samsung.com>wrote: > > > 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,
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.vivek@samsung.com> > Cc: Lukasz Majewski <l.majewski@samsung.com> > Cc: Piotr Wilczek <p.wilczek@samsung.com> > Cc: Kyungmin Park <kyungmin.park@samsung.com> > Cc: Lukasz Dalek <luk0104@gmail.com> > Cc: Marek Vasut <marex@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
> -----Original Message----- > From: Vivek Gautam [mailto:gautam.vivek@samsung.com] > Sent: Monday, May 13, 2013 12:24 PM > To: u-boot@lists.denx.de > Cc: gautam.vivek@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.vivek@samsung.com> > Cc: Lukasz Majewski <l.majewski@samsung.com> > Cc: Piotr Wilczek <p.wilczek@samsung.com> > Cc: Kyungmin Park <kyungmin.park@samsung.com> > Cc: Lukasz Dalek <luk0104@gmail.com> > Cc: Marek Vasut <marex@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
Dear Vivek Gautam, > Use get_unaligned() while fetching wMaxPacketSize to avoid > voilating any alignment rules. > > Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com> > Cc: Lukasz Majewski <l.majewski@samsung.com> > Cc: Piotr Wilczek <p.wilczek@samsung.com> > Cc: Kyungmin Park <kyungmin.park@samsung.com> > Cc: Lukasz Dalek <luk0104@gmail.com> > Cc: Marek Vasut <marex@denx.de> I'll pick both, thanks. Best regards, Marek Vasut
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",
Use get_unaligned() while fetching wMaxPacketSize to avoid voilating any alignment rules. Signed-off-by: Vivek Gautam <gautam.vivek@samsung.com> Cc: Lukasz Majewski <l.majewski@samsung.com> Cc: Piotr Wilczek <p.wilczek@samsung.com> Cc: Kyungmin Park <kyungmin.park@samsung.com> Cc: Lukasz Dalek <luk0104@gmail.com> Cc: Marek Vasut <marex@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(-)