Re: [U-Boot] [PATCH 0/4] USB: Fix EHCI to work with data cache enabled
Dear Marek, After applying these 4 patches I don't see any alignment spew and it detects the mass storage device perfectly though I see a warning "EHCI timed out on TD - token=0x80008c80" consistently in each "usb reset". Thanx & Regards, Puneet On Monday 09 April 2012 08:42 AM, Marek Vasut wrote: Dear Marek Vasut, This patchset reworks the USB core and EHCI-HCD to work with data caches. Marek Vasut (3): USB: Drop ehci_alloc/ehci_free in ehci-hcd USB: Drop cache flush bloat in EHCI-HCD USB: Document the QH and qTD antics in EHCI-HCD Puneet Saxena (1): USB: Align buffers at cacheline common/cmd_usb.c|3 +- common/usb.c| 22 ++-- common/usb_hub.c| 27 ++-- common/usb_storage.c| 59 +- disk/part_dos.c |2 +- drivers/usb/host/ehci-hcd.c | 284 ++- include/scsi.h | 4 +- include/usb.h |4 +- 8 files changed, 155 insertions(+), 250 deletions(-) Cc: Puneet Saxena Puneet, can you please test this stuff? It should fix your cache misalignment issues. Mine are now gone and USB works with caches on too. I'd like to queue this for -next. Best regards, Marek Vasut --- 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. --- ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v9] usb: align buffers at cacheline
Hi Marek, I tested on tegra2, Seaboard and didn't see these messages though see lots of messages "ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7c608" which are expected. PFA the logs if you can make out anything useful. Try to increase the delay in handshake(); Thanks, Puneet On Wednesday 04 April 2012 01:31 PM, Marek Vasut wrote: Dear Puneet Saxena, This avoids cache-alignment warnings shown in console when a usb command is entered. Whenever X bytes of unaligned buffer is invalidated, arm core invalidates X + Y bytes as per the cache line size and throws these warnings. Signed-off-by: Puneet Saxena --- I think we're almost there, hurray! :-) Though on m28evk this still has issues: => usb reset (Re)start USB... USB: Register 10011 NbrPorts 1 USB EHCI 1.00 scanning bus for devices... EHCI timed out on TD - token=0x80008c80 EHCI timed out on TD - token=0x80008d80 EHCI timed out on TD - token=0x80008c80 EHCI timed out on TD - token=0x80008c80 EHCI timed out on TD - token=0x80008c80 ERROR: NOT USB_CONFIG_DESC 1 EHCI timed out on TD - token=0x80008d80 2 USB Device(s) found scanning bus for storage devices... 0 Storage Device(s) found I have a single USB pendrive connected to the board. Also note, that if I disable the caches, it all works even with your patch applied. So I suspect there's something even more to this (maybe broken ehci_invalidate_dcache() ? ). Where did you test these patches? Thanks! Best regards, Marek Vasut --- 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. --- Tegra2 (SeaBoard) # usb reset (Re)start USB... USB: Register 10011 NbrPorts 1 USB EHCI 1.00 scanning bus for devices... ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3 fb7c608 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3 fb7c628 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7c608 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3ffb34b2 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7c5e8 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7c6a9 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7c5e8 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7c628 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7c388 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7c53f ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7c388 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7c53f ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7c388 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7c53f ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7c388 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7c53f 2 USB Device(s) found scanning bus for storage devices... ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7cd88 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7ce01 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7ce01 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7ce01 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7ce01 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7ce01 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7ce01 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7ce01 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7ce01 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7ce01 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7ce01 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7ce01 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7ce01 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7ce01 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7ce01 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7ce01 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7ce01 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7ce01 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7ce01 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7ce01 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7ce01 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7ce01 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fb7ce01 ERROR: v7_dca
Re: [U-Boot] [PATCH v8] usb: align buffers at cacheline
Hi Marek/Tom, As I understood correctly. I would merge my changes in "denx-uboot-arm/master" as "u-boot-tegra/master", where actually I tested my patch, is pulled in "denx-uboot-arm/master". Will that work... Thanks & Regards, Puneet On Monday 02 April 2012 10:02 PM, Marek Vasut wrote: Dear Tom Warren, Marek, -Original Message- From: Marek Vasut [mailto:ma...@denx.de] Sent: Monday, April 02, 2012 9:12 AM To: Tom Warren Cc: Puneet Saxena; Mike Frysinger; u-boot@lists.denx.de; s...@chromium.org; li...@bohmer.net; tr...@ti.com; albert.u.b...@aribaud.net Subject: Re: [PATCH v8] usb: align buffers at cacheline Dear Tom Warren, Marek, Puneet, et al., -Original Message- From: Marek Vasut [mailto:ma...@denx.de] Sent: Monday, March 19, 2012 8:47 AM To: Tom Warren Cc: Puneet Saxena; Mike Frysinger; u-boot@lists.denx.de; s...@chromium.org; li...@bohmer.net; tr...@ti.com; albert.u.b...@aribaud.net Subject: Re: [PATCH v8] usb: align buffers at cacheline Dear Tom Warren, Marek, -Original Message- From: Marek Vasut [mailto:marek.va...@gmail.com] Sent: Monday, March 19, 2012 7:43 AM To: Puneet Saxena Cc: Mike Frysinger; u-boot@lists.denx.de; s...@chromium.org; li...@bohmer.net; tr...@ti.com; Tom Warren Subject: Re: [PATCH v8] usb: align buffers at cacheline Dear Puneet Saxena, Hi Marek, I adapted my patch for git://git.denx.de/u-boot-usb.git master branch. As the build for target "Seaboard" is broken once I enable USB in Seaboard.h, I am unable to test my changes on "u- boot-usb". u-boot-usb is forked off the mainline u-boot.git ... tegra stuff likely isn't pulled there. Simon, can you tell me when this is gonna be there? Simon's USB/fdt patch set will be pulled into -arm first, then -main, when he adapts the 'panic' putc patchset (implement pre-console putc for fdt warning) to meet with Wolfgang's approval. Once that's done (hopefully early this week), I can submit another pull request to finally get this series in. Simon - can we have an ETA? I'd love to see some ETA on u-boot-arm push from Albert Aribaud :-( Albert, how're you doing ? Thanks! u-boot-tegra/master has the fdt/USB patches, and has been pulled into ARM master. We need the cacheline/buffer alignment patch now, to remove the volcanic spew whenever you do any USB commands. Can you guys (Puneet& Marek, and Mike/Simon if necessary) work together to get this in soon? Ain't you gonna help us? I believe the patch is really close to be usable, it just needs some easy debugging, won't you volunteer, it'd really help? :) I can apply the latest patch (v8?) and see how much spew is still present, but I (a) have no expertise in cache/USB issues and (b) have a ton of patches to apply/push for Tegra2 and (as soon as those are in) Tegra3, so my BW is limited, and finally (c) this is Puneet's work, and I'd like to see him complete it (with help from the list). Well, everyone does. And that's how FOSS works -- you scratch your own itch by sending a patch, thus helping everyone else ;-) And hey, my boards don't have caches enabled (yet) so this is low-prio for me. Sure, if someone has this as a high-prio thing, patch is very welcome :) Tom Thanks, Tom Tom I would have to merge lots of changes from "denx-uboot-tegra" to make it working for "u-boot-usb.git" master branch. I can send the adapted patch if it's needed. The problem I see is that there are important USB patches in mainline, though they are not in -tegra. So the approach I'd take if I were you would be to rebase -tegra atop of mainline for yourself and then apply your patch. Does that work for you? Thanks, Puneet Thanks! Best regards, Marek Vasut Best regards, Marek Vasut --- 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. --- ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v8] usb: align buffers at cacheline
Hi Marek, I adapted my patch for git://git.denx.de/u-boot-usb.git master branch. As the build for target "Seaboard" is broken once I enable USB in Seaboard.h, I am unable to test my changes on "u-boot-usb". I would have to merge lots of changes from "denx-uboot-tegra" to make it working for "u-boot-usb.git" master branch. I can send the adapted patch if it's needed. Thanks, Puneet On Friday 16 March 2012 02:22 PM, Marek Vasut wrote: Dear Puneet Saxena, I hope you found the rebased patch I attached useful. Hi Marek, I need to remove the changes in ehci_hcd.c as per Mike's comment and adapt my patch for git://git.denx.de/u-boot-usb.git master branch. So will be sending next patch with the above changes, shortly. Let me rebase u-boot-usb.git on top of mainline u-boot for you (done) :) With this resultant patch we see warnings for few start address warnings(related to ehci_hcd.c) and stop address warnings. This is ok, let's work on it slowly and do it properly :) I'll be happy to assist you by testing etc. Warnings due to ehci_hcd.c, can be avoided by giving delay of 1ms in after handshake(), but this is a HACK and a proper solution. Working on the root cause. I see ... thanks for your efforts! Thanks& Regards, Puneet Thank you! Best regards, Marek Vasut --- 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. --- ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v8] usb: align buffers at cacheline
Hi Marek, I need to remove the changes in ehci_hcd.c as per Mike's comment and adapt my patch for git://git.denx.de/u-boot-usb.git master branch. So will be sending next patch with the above changes, shortly. With this resultant patch we see warnings for few start address warnings(related to ehci_hcd.c) and stop address warnings. Warnings due to ehci_hcd.c, can be avoided by giving delay of 1ms in after handshake(), but this is a HACK and a proper solution. Working on the root cause. Thanks & Regards, Puneet On Friday 16 March 2012 10:09 AM, Marek Vasut wrote: Dear Puneet Saxena, What's the development on this patch? I gave it a run (find attachment, I rebased it), but it doesn't work (alignment issues in ehci_hcd). Even if I added a bounce buffer, it still didn't work :-( Best regards, Marek Vasut --- 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. --- ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v8] usb: align buffers at cacheline
Hi Marek,
On Thursday 08 March 2012 07:42 PM, Marek Vasut wrote:
Dear puneets,
Hi Marek,
On Thursday 08 March 2012 03:36 AM, Marek Vasut wrote:
Dear puneets,
Hi Mike,
On Tuesday 06 March 2012 08:37 AM, Mike Frysinger wrote:
* PGP Signed by an unknown key
On Monday 05 March 2012 09:46:21 Puneet Saxena wrote:
As DMA expects the buffers to be equal and larger then
cache lines, This aligns buffers at cacheline.
i don't think this statement is true. DMA doesn't care about alignment
(well, some do, but it's not related to cache lines but rather some
other restriction in the peripheral DMA itself). what does matter is
that cache operations operate on cache lines and not individual bytes.
hence the core arm code was updated to warn when someone told it to
invalidate X bytes but the hardware literally could not, so it had to
invalidate X + Y bytes.
Agreed, Will update the commit message in next patchset.
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
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
i think this is wrong and merely hides the errors from higher up
instead of fixing them. the point of the warning was to tell you that
the code was invalidating *too many* bytes. this code still
invalidates too many bytes without any justification as for why it's
OK to do here. further, this code path only matters to the
invalidation logic, not the flush logic. -mike
The sole purpose of this patch to remove the warnings as start/stop
address sent for invalidating
is unaligned. Without this patch code works fine but with lots of
spew...Which we don't want and discussed
in earlier thread which Simon posted. Please have a look on following
link.
As I understood, you agree that we need to align start/stop buffer
address and also agree that
to align stop address we need to align size as start address is already
aligned.
Now, "why its OK to do here"?
We could have aligned the size in two places, cache_qtd() and cache_qh()
but then we need to place alignment check
at all the places where size is passed. So I thought better Aligning at
flush_invalidate() and "ALIGN" macro does not
increase the size if size is already aligned.
Actually I have to agree with Mike here. Can you please remove that
ALIGN() (and all others you might have added)? If it does spew, that's
ok and it tells us something is wrong in the USB core subsystem. Such
stuff can be fixed in subsequent patch.
Sorry, I could not understand "(and all others you might have added)".
Do you want me remove any HACK in the patch which is using ALIGN or
making stop address
No, only such hacks where it's certain they will either invalidate or flush some
areas that weren't allocated for them, like this ALIGN you did here. This can
cause trouble that will be very hard to find.
aligned? The patch has only the above line to make stop address align
and rest of the code makes
start address align. Just to confirm, you are fine with the start
address alignment code in the patch?
The start address alignment you do also aligns the end to the cacheline, doesn't
it? (at least that's what I believe the macro is supposed to do).
Yes, start address alignment also aligns start address at the cache
line. So, removing
stop address alignment code as depicted above, should make this patch
acceptable?
Best regards,
Marek Vasut
Thanx& Regards,
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.
---
Best regards,
Marek Vasut
Thanx & Regards,
Puneet
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v8] usb: align buffers at cacheline
Hi Marek,
On Thursday 08 March 2012 03:36 AM, Marek Vasut wrote:
Dear puneets,
Hi Mike,
On Tuesday 06 March 2012 08:37 AM, Mike Frysinger wrote:
* PGP Signed by an unknown key
On Monday 05 March 2012 09:46:21 Puneet Saxena wrote:
As DMA expects the buffers to be equal and larger then
cache lines, This aligns buffers at cacheline.
i don't think this statement is true. DMA doesn't care about alignment
(well, some do, but it's not related to cache lines but rather some
other restriction in the peripheral DMA itself). what does matter is
that cache operations operate on cache lines and not individual bytes.
hence the core arm code was updated to warn when someone told it to
invalidate X bytes but the hardware literally could not, so it had to
invalidate X + Y bytes.
Agreed, Will update the commit message in next patchset.
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
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
i think this is wrong and merely hides the errors from higher up instead
of fixing them. the point of the warning was to tell you that the code
was invalidating *too many* bytes. this code still invalidates too many
bytes without any justification as for why it's OK to do here. further,
this code path only matters to the invalidation logic, not the flush
logic. -mike
The sole purpose of this patch to remove the warnings as start/stop
address sent for invalidating
is unaligned. Without this patch code works fine but with lots of
spew...Which we don't want and discussed
in earlier thread which Simon posted. Please have a look on following link.
As I understood, you agree that we need to align start/stop buffer
address and also agree that
to align stop address we need to align size as start address is already
aligned.
Now, "why its OK to do here"?
We could have aligned the size in two places, cache_qtd() and cache_qh()
but then we need to place alignment check
at all the places where size is passed. So I thought better Aligning at
flush_invalidate() and "ALIGN" macro does not
increase the size if size is already aligned.
Actually I have to agree with Mike here. Can you please remove that ALIGN() (and
all others you might have added)? If it does spew, that's ok and it tells us
something is wrong in the USB core subsystem. Such stuff can be fixed in
subsequent patch.
Sorry, I could not understand "(and all others you might have added)".
Do you want me remove any HACK in the patch which is using ALIGN or
making stop address
aligned? The patch has only the above line to make stop address align
and rest of the code makes
start address align. Just to confirm, you are fine with the start
address alignment code in the patch?
Best regards,
Marek Vasut
Thanx & Regards,
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.
---
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v8] usb: align buffers at cacheline
Hi Mike,
Forgot to write the link in below mail.
Here is the link:
http://lists.denx.de/pipermail/u-boot/2011-December/112138.html
Do you want me to show a warning where I am making size as cacheline size?
Thanx & Regards,,
Puneet
On Wednesday 07 March 2012 12:42 PM, puneets wrote:
Hi Mike,
On Tuesday 06 March 2012 08:37 AM, Mike Frysinger wrote:
* PGP Signed by an unknown key
On Monday 05 March 2012 09:46:21 Puneet Saxena wrote:
As DMA expects the buffers to be equal and larger then
cache lines, This aligns buffers at cacheline.
i don't think this statement is true. DMA doesn't care about alignment (well,
some do, but it's not related to cache lines but rather some other restriction
in the peripheral DMA itself). what does matter is that cache operations
operate on cache lines and not individual bytes. hence the core arm code was
updated to warn when someone told it to invalidate X bytes but the hardware
literally could not, so it had to invalidate X + Y bytes.
Agreed, Will update the commit message in next patchset.
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
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
i think this is wrong and merely hides the errors from higher up instead of
fixing them. the point of the warning was to tell you that the code was
invalidating *too many* bytes. this code still invalidates too many bytes
without any justification as for why it's OK to do here. further, this code
path only matters to the invalidation logic, not the flush logic.
-mike
The sole purpose of this patch to remove the warnings as start/stop
address sent for invalidating
is unaligned. Without this patch code works fine but with lots of
spew...Which we don't want and discussed
in earlier thread which Simon posted. Please have a look on following link.
As I understood, you agree that we need to align start/stop buffer
address and also agree that
to align stop address we need to align size as start address is already
aligned.
Now, "why its OK to do here"?
We could have aligned the size in two places, cache_qtd() and cache_qh()
but then we need to place alignment check
at all the places where size is passed. So I thought better Aligning at
flush_invalidate() and "ALIGN" macro does not
increase the size if size is already aligned.
* Unknown Key
* 0xE837F581
Thanx& Regards,
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.
---
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v8] usb: align buffers at cacheline
Hi Mike,
On Tuesday 06 March 2012 08:37 AM, Mike Frysinger wrote:
* PGP Signed by an unknown key
On Monday 05 March 2012 09:46:21 Puneet Saxena wrote:
As DMA expects the buffers to be equal and larger then
cache lines, This aligns buffers at cacheline.
i don't think this statement is true. DMA doesn't care about alignment (well,
some do, but it's not related to cache lines but rather some other restriction
in the peripheral DMA itself). what does matter is that cache operations
operate on cache lines and not individual bytes. hence the core arm code was
updated to warn when someone told it to invalidate X bytes but the hardware
literally could not, so it had to invalidate X + Y bytes.
Agreed, Will update the commit message in next patchset.
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
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
i think this is wrong and merely hides the errors from higher up instead of
fixing them. the point of the warning was to tell you that the code was
invalidating *too many* bytes. this code still invalidates too many bytes
without any justification as for why it's OK to do here. further, this code
path only matters to the invalidation logic, not the flush logic.
-mike
The sole purpose of this patch to remove the warnings as start/stop
address sent for invalidating
is unaligned. Without this patch code works fine but with lots of
spew...Which we don't want and discussed
in earlier thread which Simon posted. Please have a look on following link.
As I understood, you agree that we need to align start/stop buffer
address and also agree that
to align stop address we need to align size as start address is already
aligned.
Now, "why its OK to do here"?
We could have aligned the size in two places, cache_qtd() and cache_qh()
but then we need to place alignment check
at all the places where size is passed. So I thought better Aligning at
flush_invalidate() and "ALIGN" macro does not
increase the size if size is already aligned.
* Unknown Key
* 0xE837F581
Thanx & Regards,
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.
---
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v8] usb: align buffers at cacheline
Hi Simon, I do see only first warning on all the devices and rest of the warnings on a few mass storage device. My patch fixing these warnings is not accepted. Please see below link for further info - http://lists.denx.de/pipermail/u-boot/2012-March/119404.html IMO, these warnings spew when we expects some info e.g. device descriptor, manf id, prod id... from the device. The root cause of the issue is some race condition in H/w due to which, even though we receive "STD_ASS"(Async sequence status) as 0 still transfer descriptor token is not updated. Thanx & Regards, Puneet On Monday 05 March 2012 11:48 PM, Simon Glass wrote: Hi Puneet, On Mon, Mar 5, 2012 at 6:46 AM, Puneet Saxena wrote: As DMA expects the buffers to be equal and larger then cache lines, This aligns buffers at cacheline. Signed-off-by: Puneet Saxena Tested on Seaboard: Tested-by: Simon Glass Acked-by: Simon Glass I do still see a few alignment warnings, but only a tidy fraction of what we had. Do you see these? Tegra2 (SeaBoard) # usb start (Re)start USB... USB: Register 10011 NbrPorts 1 USB EHCI 1.00 scanning bus for devices... ERROR: v7_dcache_inval_range - start address is not aligned - 0x3fbed6f2 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fbed732 ERROR: v7_dcache_inval_range - start address is not aligned - 0x3fbed484 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fbed584 ERROR: v7_dcache_inval_range - start address is not aligned - 0x3fbed492 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fbed592 ERROR: v7_dcache_inval_range - start address is not aligned - 0x3fbed49e ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fbed59e ERROR: v7_dcache_inval_range - start address is not aligned - 0x3fbed4a2 ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3fbed582 2 USB Device(s) found scanning bus for storage devices... 1 Storage Device(s) found Regards, Simon --- 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. --- ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v3 2/2] usb: Add CONFIG to fetch string descriptor
Hi Marek,
On Monday 05 March 2012 06:18 PM, Marek Vasut wrote:
Dear Puneet Saxena,
Hi Marek,
On Thursday 01 March 2012 05:15 PM, Marek Vasut wrote:
Hi!
Hi Marek,
On Thursday 01 March 2012 02:59 AM, Marek Vasut wrote:
Add "CONFIG_USB_STRING_FETCH" to fetch first string descriptor
length and then pass this length to fetch string descriptor.
Signed-off-by: Puneet Saxena
---
Changes for V2:
- Change existing config by "CONFIG_USB_STRING_FETCH"
Changes for V3:
- Removed extra new line
- Explained "CONFIG_USB_STRING_FETCH" in top level README
README |4
common/usb.c|4
include/configs/tegra2-common.h |2 ++
3 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/README b/README
index 7adf7c7..c045a37 100644
--- a/README
+++ b/README
@@ -1138,6 +1138,10 @@ The following options need to be configured:
CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the
txfilltuning field in the EHCI controller on reset.
+ CONFIG_USB_STRING_FETCH
+ Enables settings to USB core to handle string issues
which
+ few devices can not handle.
+
- USB Device:
Define the below if you wish to use the USB console.
Once firmware is rebuilt from a serial console issue the
diff --git a/common/usb.c b/common/usb.c
index 191bc5b..a73cb60 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -658,9 +658,13 @@ static int usb_string_sub(struct usb_device
*dev, unsigned int langid, {
int rc;
+#ifdef CONFIG_USB_STRING_FETCH
Shouldn't this be something like ... CONFIG_USB_AVOID_STRING_FETCH
then?
Anyway, how come some devices can't handle it? What happens then?
What devices are those (exact type etc)?
I believe the bug is deeper and adding extra config options can be
avoided, what do you think?
Thanks!
M
It does not avoid string fetch.
Well it does certainly not call usb_get_string()
Precisely, it avoids fetching string desc of 255 bytes. so better name
could be
"CONFIG_USB_AVOID_STRING_FETCH_255". Thanks for your comment.
I checked with few mass storage devices that they does not handle
string descriptor request correctly and so we get
start/stop Cache alignment error.
Cache alignment error ? Wow, how's that actually related to SUB string
fetching? Maybe we should manually realign the result then? I still
don't really understand what you're seeing, sorry ... can you please
elaborate?
The particular mass storage device is -
Vendor: Kingston Rev: PMAP Prod: DataTraveler 2.0
Type: Removable Hard Disk
Capacity: 1906.0 MB = 1.8 GB (3903488 x 512)
When code tries to read Manufacturing Id..prod id...etc..it passes cache
aligned buffer "tbuf" in
"usb_string()" @usb.c. Inside "usb_string_sub()", usb_get_string()
passes as default 255 bytes to fetch string
descriptor.
The code in "ehci_submit_async() " invalidates *partially* the passed
buffer though we pass aligned buffer and "STD_ASS"
is received. Though it should invalidate only the cached line which is
equal(~32) to string desc length.
Hm ... so this means the bug is in ehci_hcd? Maybe the ehci_hcd should be
fixed to correctly handle caches then?
If we give delay of 1 ms after handshake() the cache alignment warning
does not spew...but delay of 1 ms is not acceptable.
So I enabled the code to fetch first string desc length and then fetch
string desc fetch, in this way the issue is resolved.
It makes sense also to fetch string desc length and then actual string
desc
I see, I understand now just about everything but the ehci problem, see
above. Your explanation was very helpful.
Let's work this out together!
Cheers!
M
Is this issue fixed or is this patch still needed? Thanks in advance!
This issue is not fixed and still need a patch to fix root cause,
explained above.
Note that this issue is observed even though we pass aligned address and
expects something from the device.
Best regards,
Marek Vasut
Thanx & Regards,
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.
---
___
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
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
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 v3 1/2] usb: align buffers at cacheline
Hi,
On Friday 02 March 2012 04:13 PM, Marek Vasut wrote:
Hi,
On Friday 02 March 2012 12:08 AM, Marek Vasut wrote:
On Thursday 01 March 2012 03:05 AM, 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 V2:
- Use "ARCH_DMA_MINALIGN" directly
- Use "ALIGN" to align size as cacheline
- Removed headers from usb.h
- Send 8 bytes of device descriptor size to read
Max packet size
scsi.h header is needed to avoid extra memcpy from local buffer
to global buffer.
Changes for V3:
- Removed local descriptor elements copy to global descriptor
elements - Removed "Signed-off-by: Jim Lin"
from commit
message
common/cmd_usb.c|3 +-
common/usb.c| 57
++--- 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(+), 60 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 63a11c8..191bc5b 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;
@@ -694,7 +695,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;
@@ -794,7 +795,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;
@@ -842,7 +843,10 @@ int usb_new_device(struct usb_device *dev)
dev->epmaxpacketin[0] = 64;
dev->epmaxpacketout[0] = 64;
- err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, 64);
+ desc->bMaxPacketSize0 = 0;
+ /*8 bytes of the descriptor to read Max packet size*/
+ err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc,
+ 8);
Did some unrelated addition/change creep in here?
No, This is the fix for the similar issue as is discussed for string
fetch().
When the device partially fills the passed buffer and we try to
invalidate the partial buffer
the cache alignment error crops up.
The code in "ehci_submit_async() " invalidates *partially* the passed
buffer though we pass aligned b
Re: [U-Boot] [PATCH v3 1/2] usb: align buffers at cacheline
Hi,
On Friday 02 March 2012 12:08 AM, Marek Vasut wrote:
On Thursday 01 March 2012 03:05 AM, 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 V2:
- Use "ARCH_DMA_MINALIGN" directly
- Use "ALIGN" to align size as cacheline
- Removed headers from usb.h
- Send 8 bytes of device descriptor size to read
Max packet size
scsi.h header is needed to avoid extra memcpy from local buffer
to global buffer.
Changes for V3:
- Removed local descriptor elements copy to global descriptor
elements - Removed "Signed-off-by: Jim Lin" from
commit
message
common/cmd_usb.c|3 +-
common/usb.c| 57
++--- 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(+), 60 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 63a11c8..191bc5b 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;
@@ -694,7 +695,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;
@@ -794,7 +795,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;
@@ -842,7 +843,10 @@ int usb_new_device(struct usb_device *dev)
dev->epmaxpacketin[0] = 64;
dev->epmaxpacketout[0] = 64;
- err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, 64);
+ desc->bMaxPacketSize0 = 0;
+ /*8 bytes of the descriptor to read Max packet size*/
+ err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc,
+ 8);
Did some unrelated addition/change creep in here?
No, This is the fix for the similar issue as is discussed for string
fetch().
When the device partially fills the passed buffer and we try to
invalidate the partial buffer
the cache alignment error crops up.
The code in "ehci_submit_async() " invalidates *partially* the passed
buffer though we pass aligned buffer after "STD_ASS"
is received. Actually it should invalidate only the cach
Re: [U-Boot] [PATCH v3 1/2] usb: align buffers at cacheline
On Thursday 01 March 2012 03:05 AM, 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 V2:
- Use "ARCH_DMA_MINALIGN" directly
- Use "ALIGN" to align size as cacheline
- Removed headers from usb.h
- Send 8 bytes of device descriptor size to read
Max packet size
scsi.h header is needed to avoid extra memcpy from local buffer
to global buffer.
Changes for V3:
- Removed local descriptor elements copy to global descriptor elements
- Removed "Signed-off-by: Jim Lin" from commit
message
common/cmd_usb.c|3 +-
common/usb.c| 57
++--- 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(+), 60 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 63a11c8..191bc5b 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;
@@ -694,7 +695,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;
@@ -794,7 +795,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;
@@ -842,7 +843,10 @@ int usb_new_device(struct usb_device *dev)
dev->epmaxpacketin[0] = 64;
dev->epmaxpacketout[0] = 64;
- err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, 64);
+ desc->bMaxPacketSize0 = 0;
+ /*8 bytes of the descriptor to read Max packet size*/
+ err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc,
+ 8);
Did some unrelated addition/change creep in here?
No, This is the fix for the similar issue as is discussed for string
fetch().
When the device partially fills the passed buffer and we try to
invalidate the partial buffer
the cache alignment error crops up.
The code in "ehci_submit_async() " invalidates *partially* the passed
buffer though we pass aligned buffer after "STD_ASS"
is received. Actually it should invalidate only the cached line which is
equal(~32) to device desc length.
If we pass actual device desc length the cache alignment error does not
spew.
Note that here we are aligning the buffer still the error come
Re: [U-Boot] [PATCH v3 2/2] usb: Add CONFIG to fetch string descriptor
Hi Marek,
On Thursday 01 March 2012 05:15 PM, Marek Vasut wrote:
Hi!
Hi Marek,
On Thursday 01 March 2012 02:59 AM, Marek Vasut wrote:
Add "CONFIG_USB_STRING_FETCH" to fetch first string descriptor length
and then pass this length to fetch string descriptor.
Signed-off-by: Puneet Saxena
---
Changes for V2:
- Change existing config by "CONFIG_USB_STRING_FETCH"
Changes for V3:
- Removed extra new line
- Explained "CONFIG_USB_STRING_FETCH" in top level README
README |4
common/usb.c|4
include/configs/tegra2-common.h |2 ++
3 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/README b/README
index 7adf7c7..c045a37 100644
--- a/README
+++ b/README
@@ -1138,6 +1138,10 @@ The following options need to be configured:
CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the
txfilltuning field in the EHCI controller on reset.
+ CONFIG_USB_STRING_FETCH
+ Enables settings to USB core to handle string issues which
+ few devices can not handle.
+
- USB Device:
Define the below if you wish to use the USB console.
Once firmware is rebuilt from a serial console issue the
diff --git a/common/usb.c b/common/usb.c
index 191bc5b..a73cb60 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -658,9 +658,13 @@ static int usb_string_sub(struct usb_device *dev,
unsigned int langid, {
int rc;
+#ifdef CONFIG_USB_STRING_FETCH
Shouldn't this be something like ... CONFIG_USB_AVOID_STRING_FETCH then?
Anyway, how come some devices can't handle it? What happens then? What
devices are those (exact type etc)?
I believe the bug is deeper and adding extra config options can be
avoided, what do you think?
Thanks!
M
It does not avoid string fetch.
Well it does certainly not call usb_get_string()
Precisely, it avoids fetching string desc of 255 bytes. so better name
could be
"CONFIG_USB_AVOID_STRING_FETCH_255". Thanks for your comment.
I checked with few mass storage devices that they does not handle string
descriptor request correctly and so we get
start/stop Cache alignment error.
Cache alignment error ? Wow, how's that actually related to SUB string fetching?
Maybe we should manually realign the result then? I still don't really
understand what you're seeing, sorry ... can you please elaborate?
The particular mass storage device is -
Vendor: Kingston Rev: PMAP Prod: DataTraveler 2.0
Type: Removable Hard Disk
Capacity: 1906.0 MB = 1.8 GB (3903488 x 512)
When code tries to read Manufacturing Id..prod id...etc..it passes cache
aligned buffer "tbuf" in
"usb_string()" @usb.c. Inside "usb_string_sub()", usb_get_string()
passes as default 255 bytes to fetch string
descriptor.
The code in "ehci_submit_async() " invalidates *partially* the passed
buffer though we pass aligned buffer and "STD_ASS"
is received. Though it should invalidate only the cached line which is
equal(~32) to string desc length.
If we give delay of 1 ms after handshake() the cache alignment warning
does not spew...but delay of 1 ms is not acceptable.
So I enabled the code to fetch first string desc length and then fetch
string desc fetch, in this way the issue is resolved.
It makes sense also to fetch string desc length and then actual string
desc
Thanks,
Puneet
One way is, blacklist those devices by
vendor id/product id..
etc. This is done in Linux kernel. Plz see Message.c (drivers\usb\core)
Line: 722.
Blacklisting the device requires a framework to detect the
device...However this could be achieved
simply with this implementation.
Sure, I see your intention, but I still don't get the problem, see above. Can
you also tell me if there's particular device that's broken and which one is it
(precise type etc)?
Thanks a lot for your cooperation on this matter!
M
+ rc = -1;
+#else
/* Try to read the string descriptor by asking for the maximum
* possible number of bytes */
rc = usb_get_string(dev, langid, index, buf, 255);
+#endif
/* If that failed try to read the descriptor length, then
* ask for just that many bytes */
diff --git a/include/configs/tegra2-common.h
b/include/configs/tegra2-common.h index 266d0e5..d20b49c 100644
--- a/include/configs/tegra2-common.h
+++ b/include/configs/tegra2-common.h
@@ -93,6 +93,8 @@
#define CONFIG_USB_EHCI_TXFIFO_THRESH10
#define CONFIG_EHCI_IS_TDI
#define CONFIG_EHCI_DCACHE
+/* string descriptors must not be fetched using a 255-byte read */
+#define CONFIG_USB_STRING_FETCH
/* include default commands */
#include
---
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 prohibit
Re: [U-Boot] [PATCH v3 2/2] usb: Add CONFIG to fetch string descriptor
Hi Marek,
On Thursday 01 March 2012 02:59 AM, Marek Vasut wrote:
Add "CONFIG_USB_STRING_FETCH" to fetch first string descriptor length
and then pass this length to fetch string descriptor.
Signed-off-by: Puneet Saxena
---
Changes for V2:
- Change existing config by "CONFIG_USB_STRING_FETCH"
Changes for V3:
- Removed extra new line
- Explained "CONFIG_USB_STRING_FETCH" in top level README
README |4
common/usb.c|4
include/configs/tegra2-common.h |2 ++
3 files changed, 10 insertions(+), 0 deletions(-)
diff --git a/README b/README
index 7adf7c7..c045a37 100644
--- a/README
+++ b/README
@@ -1138,6 +1138,10 @@ The following options need to be configured:
CONFIG_USB_EHCI_TXFIFO_THRESH enables setting of the
txfilltuning field in the EHCI controller on reset.
+ CONFIG_USB_STRING_FETCH
+ Enables settings to USB core to handle string issues which
+ few devices can not handle.
+
- USB Device:
Define the below if you wish to use the USB console.
Once firmware is rebuilt from a serial console issue the
diff --git a/common/usb.c b/common/usb.c
index 191bc5b..a73cb60 100644
--- a/common/usb.c
+++ b/common/usb.c
@@ -658,9 +658,13 @@ static int usb_string_sub(struct usb_device *dev,
unsigned int langid, {
int rc;
+#ifdef CONFIG_USB_STRING_FETCH
Shouldn't this be something like ... CONFIG_USB_AVOID_STRING_FETCH then?
Anyway, how come some devices can't handle it? What happens then? What devices
are those (exact type etc)?
I believe the bug is deeper and adding extra config options can be avoided, what
do you think?
Thanks!
M
It does not avoid string fetch.
I checked with few mass storage devices that they does not handle string
descriptor request correctly and so we get
start/stop Cache alignment error. One way is, blacklist those devices by
vendor id/product id..
etc. This is done in Linux kernel. Plz see Message.c (drivers\usb\core)
Line: 722.
Blacklisting the device requires a framework to detect the
device...However this could be achieved
simply with this implementation.
+ rc = -1;
+#else
/* Try to read the string descriptor by asking for the maximum
* possible number of bytes */
rc = usb_get_string(dev, langid, index, buf, 255);
+#endif
/* If that failed try to read the descriptor length, then
* ask for just that many bytes */
diff --git a/include/configs/tegra2-common.h
b/include/configs/tegra2-common.h index 266d0e5..d20b49c 100644
--- a/include/configs/tegra2-common.h
+++ b/include/configs/tegra2-common.h
@@ -93,6 +93,8 @@
#define CONFIG_USB_EHCI_TXFIFO_THRESH 10
#define CONFIG_EHCI_IS_TDI
#define CONFIG_EHCI_DCACHE
+/* string descriptors must not be fetched using a 255-byte read */
+#define CONFIG_USB_STRING_FETCH
/* include default commands */
#include
---
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.
---
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH v2 1/2] usb: align buffers at cacheline
Hi Marek,
IMO, Simon has already mentioned the reason of using
ALLOC_CACHE_ALIGN_BUFFER,
Please find below my explanation about other doubts.
On Monday 27 February 2012 10:19 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
Signed-off-by: Jim Lin
---
First of all, thanks for this patch, it really helps. But, there are a few
things that concern me.
Changes for V2:
- Use "ARCH_DMA_MINALIGN" directly
- Use "ALIGN" to align size as cacheline
- Removed headers from usb.h
- Send 8 bytes of device descriptor size to read
Max packet size
scsi.h header is needed to avoid extra memcpy from local buffer
to global buffer.
common/cmd_usb.c|3 +-
common/usb.c| 61
-- common/usb_storage.c|
59 +++-- disk/part_dos.c |
2 +-
drivers/usb/host/ehci-hcd.c |8 +
include/scsi.h |4 ++-
6 files changed, 77 insertions(+), 60 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);
Why not memalign(), do you want to allocate on stack so badly ?
+
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 63a11c8..2279459 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));
DTTO, btw does the setup packet need to be aligned too ?
We need to align setup packet as it's used for constructing SETUP
Transfer descriptor buffer.
Plz see the code - Ehci-hcd.c (drivers\usb\host) Line No - 402.
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;
@@ -694,7 +695,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;
@@ -794,7 +795,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);
Why not simply memalign() them all?
/* We still haven't set the Address yet */
addr = dev->devnum;
@@ -842,7 +843,10 @@ int usb_new_device(struct usb_device *dev)
dev->epmaxpacketin[0] = 64;
dev->epmaxpacketout[0] = 64;
- err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, 64);
+ desc->bMaxPacketSize0 = 0;
+ /*8 bytes of the descriptor to read Max packet size*/
+ err = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc,
+ 8);
if (err< 0) {
USB_PRINTF("usb_new_device: usb_get_descriptor() failed\n");
return 1;
@@ -905,7 +909,7 @@ int usb_new_device(struct usb_device *dev)
tmp = sizeof(dev->descriptor);
err = usb_get_descriptor(dev, USB_
Re: [U-Boot] [PATCH 1/2] usb: align buffers at cacheline
Hi,
On Friday 24 February 2012 06:12 PM, Simon Glass wrote:
Hi,
On Thu, Feb 23, 2012 at 6:25 AM, Puneet Saxena wrote:
As DMA expects the buffers to be equal and larger then
cache lines, This aligns buffers at cacheline.
Signed-off-by: Puneet Saxena
Signed-off-by: Jim Lin
---
Changes for v2:
- Split the commit in to 2 commits
- "ARCH_DMA_MINALIGN" replacement
- Making stop address cache line aligned by
making size as multiple of cache line
- incorporated Marek and Mike's comment
common/cmd_usb.c|3 +-
common/usb.c| 53 ++
common/usb_storage.c| 66 ++
drivers/usb/host/ehci-hcd.c |9 ++
include/scsi.h |8 -
include/usb.h |8 -
6 files changed, 88 insertions(+), 59 deletions(-)
diff --git a/drivers/usb/host/ehci-hcd.c b/drivers/usb/host/ehci-hcd.c
index d893b2a..82652f5 100644
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
@@ -120,6 +120,15 @@ 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 nultiple of cache line size.
+*/
+ if (size& (ARCH_DMA_MINALIGN - 1))
+ size = ((size / ARCH_DMA_MINALIGN) + 1)
+ * ARCH_DMA_MINALIGN;
Can we just use:
size = ALIGN(size, ARCH_DMA_MINALIGN)
here or does it increase size even if already aligned?
size = ALIGN(size, ARCH_DMA_MINALIGN) can be used in place of this.
The code in patch does not increase the size if size is already aligned.
if (flush)
flush_dcache_range(addr, addr + size);
else
diff --git a/include/scsi.h b/include/scsi.h
index c52759c..c1f573e 100644
--- a/include/scsi.h
+++ b/include/scsi.h
@@ -26,7 +26,13 @@
typedef struct SCSI_cmd_block{
unsigned char cmd[16];
/* command */
- unsigned char sense_buf[64]; /* for request sense */
+ /* for request sense */
+#ifdef ARCH_DMA_MINALIGN
+ unsigned char sense_buf[64]
+ __attribute__((aligned(ARCH_DMA_MINALIGN)));
+#else
+ unsigned char sense_buf[64];
+#endif
I think Mike said this, but I thought ARCH_DMA_MINALIGN should always
be defined.
Is it possible to align something in the middle of a structure? How
does that work? I'm suppose you have this working, I would just like
to understand what the resulting code does in this case.
I verified that ARCH_DMA_MINALIGN is already defined so I will not check
it in next patch.
Yes it would align the variable in middle at the time of instantiating
the structure.
Compiler generates the addresses of this variable and structure
variable, as 32 __BYTE__ aligned(cache line is 32).
It try to accommodate all the variables in these two addresses till 32 byte.
Another solution could be, Align whole structure of cacheline and keep
the buffer as first element.
However in case more than one buffers are in a structure, only option is
to align individual buffer in place of
whole structure.
unsigned char status;
/* SCSI Status */
unsigned char target;
/* Target ID */
unsigned char lun;
/* Target LUN*/
diff --git a/include/usb.h b/include/usb.h
index 06170cd..d38817a 100644
--- a/include/usb.h
+++ b/include/usb.h
@@ -109,7 +109,13 @@ struct usb_device {
int epmaxpacketout[16]; /* OUTput endpoint specific maximums */
int configno; /* selected config number */
- struct usb_device_descriptor descriptor; /* Device Descriptor */
+/* Device Descriptor */
+#ifdef ARCH_DMA_MINALIGN
+ struct usb_device_descriptor descriptor
+ __attribute__((aligned(ARCH_DMA_MINALIGN)));
+#else
+ struct usb_device_descriptor descriptor;
+#endif
Same question here, it seems even more exotic - maybe you will need a
memcpy somewhere after all?
struct usb_config config; /* config descriptor */
int have_langid;/* whether string_langid is valid yet */
--
1.7.1
Regards,
Simon
---
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, p
Re: [U-Boot] [PATCH 1/2] usb: align buffers at cacheline
Hi Mike,
On Thursday 23 February 2012 11:45 PM, Mike Frysinger wrote:
* PGP Signed by an unknown key
On Thursday 23 February 2012 09:25:25 Puneet Saxena wrote:
--- a/common/usb_storage.c
+++ b/common/usb_storage.c
-static unsigned char usb_stor_buf[512];
-static ccb usb_ccb;
+#ifdef ARCH_DMA_MINALIGN
+ static ccb usb_ccb __attribute__((aligned(ARCH_DMA_MINALIGN)));
+#else
+ static ccb usb_ccb;
+#endif
don't use ifdef's. you may assume that ARCH_DMA_MINALIGN is always defined.
after all, the ALLOC_CACHE_ALIGN_BUFFER() helper does.
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;
+ /* GJ */
+ memset(usb_stor_buf, 0, sizeof(usb_stor_buf));
you shrunk the buffer to 36 bytes, so that's good. but the memset() should be
dropped. see what i posted to Marek when he tried moving this buffer locally
if you want background.
--- a/drivers/usb/host/ehci-hcd.c
+++ b/drivers/usb/host/ehci-hcd.c
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 nultiple of cache line size.
+*/
+ if (size& (ARCH_DMA_MINALIGN - 1))
+ size = ((size / ARCH_DMA_MINALIGN) + 1)
+ * ARCH_DMA_MINALIGN;
i don't think this logic should exist at all. the arch is responsible for
flushing enough of its cache to cover the requested size.
The patches under review is used to avoid the warnings -
1) ERROR: v7_dcache_inval_range - start address is not aligned - 0x3ffbd5d0
2) ERROR: v7_dcache_inval_range - stop address is not aligned - 0x3ffbd5f0
This piece of code assures that second warning should not appear.
By allocating Buffers cache line aligned we are making sure that "start
address"
will always be aligned. Arch specific code is taking care of
flushing/invalidating the
cache as per the requested size but 2nd warning appears before doing that.
As you see in code: stop address = start address + size, so we are
required to assure
size to be multiple of cacheline(start address we are aligning already).
The above piece
of code is meant for this.
other option could be to add code in arch specific file to align stop
address. e.g in "v7_dcache_maint_range@
Cache_v7.c (arch\arm\cpu\armv7)"
--- a/include/scsi.h
+++ b/include/scsi.h
typedef struct SCSI_cmd_block{
unsigned char cmd[16];/* command */
- unsigned char sense_buf[64]; /* for request sense */
+ /* for request sense */
+#ifdef ARCH_DMA_MINALIGN
+ unsigned char sense_buf[64]
+ __attribute__((aligned(ARCH_DMA_MINALIGN)));
+#else
+ unsigned char sense_buf[64];
+#endif
--- a/include/usb.h
+++ b/include/usb.h
struct usb_device {
int epmaxpacketout[16]; /* OUTput endpoint specific maximums */
int configno; /* selected config number */
- struct usb_device_descriptor descriptor; /* Device Descriptor */
+/* Device Descriptor */
+#ifdef ARCH_DMA_MINALIGN
+ struct usb_device_descriptor descriptor
+ __attribute__((aligned(ARCH_DMA_MINALIGN)));
+#else
+ struct usb_device_descriptor descriptor;
+#endif
i don't think either of these headers should be changed. find& fix the code
that is causing a problem.
IMHO, scsi.h should be changed otherwise we may need to memcpy aligned
local buffer to
this global buffer in "usb_request_sense @ Usb_storage.c (common).
wrt the other random ALLOC_CACHE_ALIGN_BUFFER() changes, they look OK to me.
-mike
* Unknown Key
* 0xE837F581
---
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.
---
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] [PATCH 2/2] usb: Add quirk "USB_QUIRK_STRING_FETCH_255"
On Thursday 23 February 2012 09:34 PM, Tom Warren wrote: Puneet, -Original Message- From: Puneet Saxena Sent: Thursday, February 23, 2012 7:25 AM To: u-boot@lists.denx.de; s...@chromium.org Cc: vap...@gentoo.org; Tom Warren; Puneet Saxena Subject: [PATCH 2/2] usb: Add quirk "USB_QUIRK_STRING_FETCH_255" Add a quirk "USB_QUIRK_STRING_FETCH_255", borrowed from Linux kernel to fetch string using descriptor length then fetch actual bytes returned in descriptor buffer. Signed-off-by: Puneet Saxena --- Changes for v2: - Add quirk for fetching actual bytes common/usb.c|4 include/configs/tegra2-common.h |7 +++ 2 files changed, 11 insertions(+), 0 deletions(-) diff --git a/common/usb.c b/common/usb.c index 75926aa..cd85c18 100644 --- a/common/usb.c +++ b/common/usb.c @@ -660,7 +660,11 @@ static int usb_string_sub(struct usb_device *dev, unsigned int langid, /* Try to read the string descriptor by asking for the maximum * possible number of bytes */ +#ifdef USB_QUIRK_STRING_FETCH_255 + rc = -4; +#else rc = usb_get_string(dev, langid, index, buf, 255); +#endif /* If that failed try to read the descriptor length, then * ask for just that many bytes */ diff --git a/include/configs/tegra2-common.h b/include/configs/tegra2- common.h index 266d0e5..51cc200 100644 --- a/include/configs/tegra2-common.h +++ b/include/configs/tegra2-common.h @@ -172,4 +172,11 @@ #define CONFIG_TEGRA2_GPIO #define CONFIG_CMD_GPIO + +/* + * Imported the quirk from Linux kernel */ +/* string descriptors must not be fetched using a 255-byte read */ +#define USB_QUIRK_STRING_FETCH_255 0x0001 + #endif /* __TEGRA2_COMMON_H */ -- 1.7.1 Make sure you include the USB custodian/expert when submitting USB patches (Remy Bohmer, li...@bohmer.net). Also, as TomR says, this should be a CONFIG_USB_QUIRK_xxx string. Note that it doesn't need a 0x0001 - #define'ing a switch means it's explicitly enabled - no need for 1 or 0 (see all the other CONFIG_ defines in the config headers). Tom My thought of adding "#define USB_QUIRK_STRING_FETCH_255 0x0001" instead "#define USB_QUIRK_STRING_FETCH_255" is, some time in future we might have to implement kernel quirk kind of functionality to make generic implementation. Its not needed as of now. Will incorporate in next patch. Thanks, 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. --- ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
