| Message ID | 1340230468-12811-4-git-send-email-trini@ti.com |
|---|---|
| State | Rejected |
| Delegated to: | Tom Rini |
| Headers | show |
Hi, 21.06.2012 02:14, Tom Rini wrote: > USB EHCI and DCACHE are not compatible, so disable DCACHE support at > build-time as run-time disable is insufficient for USB use. Sorry for missing this discussion. I think compile-time disabling of the cache is too brutal. ehci-hcd cache handling is broken anyway: doing unaligned flushes/invalidates is a bug, and we know for sure that upper layers don't care about alignment (and I bet ehci-hcd does this even for its internal buffers). So what's the point in all this cache handling in ehci-hcd? It's not going to work anyway and just produces problems. So I suggest to just disable all this stuff until generic code will be fixed. Alternatively we can do bounce-buffering inside driver. Regards, Ilya.
Dear Ilya Yanok, > Hi, > > 21.06.2012 02:14, Tom Rini wrote: > > USB EHCI and DCACHE are not compatible, so disable DCACHE support at > > build-time as run-time disable is insufficient for USB use. > > Sorry for missing this discussion. I think compile-time disabling of the > cache is too brutal. > ehci-hcd cache handling is broken anyway: doing unaligned > flushes/invalidates is a bug, and we know for sure that upper layers > don't care about alignment (and I bet ehci-hcd does this even for its > internal buffers). So what's the point in all this cache handling in > ehci-hcd? It's not going to work anyway and just produces problems. So I > suggest to just disable all this stuff until generic code will be fixed. > Alternatively we can do bounce-buffering inside driver. We should rather introduce generic bounce buffer. But the upper layers are getting fixed recently so we should be getting there. > > Regards, Ilya. Best regards, Marek Vasut
Dear Marek, 28.06.2012 02:48, Marek Vasut wrote: >> Sorry for missing this discussion. I think compile-time disabling of the >> cache is too brutal. >> ehci-hcd cache handling is broken anyway: doing unaligned >> flushes/invalidates is a bug, and we know for sure that upper layers >> don't care about alignment (and I bet ehci-hcd does this even for its >> internal buffers). So what's the point in all this cache handling in >> ehci-hcd? It's not going to work anyway and just produces problems. So I >> suggest to just disable all this stuff until generic code will be fixed. >> Alternatively we can do bounce-buffering inside driver. > We should rather introduce generic bounce buffer. But the upper layers are > getting fixed recently so we should be getting there. Really? Don't forget my old patch [1] then ;) Still I think we should rip off all the cache stuff from ehci-hcd until all patches for upper layers are included. Again, this stuff doesn't do proper things now anyway and USB won't work with dcache enabled. BTW, I think this was under #ifdef CONFIG_EHCI_DCACHE last time looked at it. Was this changed by your commit? I think that's the source of the problem this series tries to address: you've taken buggy code out of #ifdef ;) I think it's better to just put it back until upper layers won't be fixed. Regards, Ilya. [1] http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/114235
Dear Ilya Yanok, > Dear Marek, > > 28.06.2012 02:48, Marek Vasut wrote: > >> Sorry for missing this discussion. I think compile-time disabling of the > >> cache is too brutal. > >> ehci-hcd cache handling is broken anyway: doing unaligned > >> flushes/invalidates is a bug, and we know for sure that upper layers > >> don't care about alignment (and I bet ehci-hcd does this even for its > >> internal buffers). So what's the point in all this cache handling in > >> ehci-hcd? It's not going to work anyway and just produces problems. So I > >> suggest to just disable all this stuff until generic code will be fixed. > >> Alternatively we can do bounce-buffering inside driver. > > > > We should rather introduce generic bounce buffer. But the upper layers > > are getting fixed recently so we should be getting there. > > Really? Don't forget my old patch [1] then ;) > Still I think we should rip off all the cache stuff from ehci-hcd until > all patches for upper layers are included. Again, this stuff doesn't do > proper things now anyway and USB won't work with dcache enabled. Have you tested? I enabled dcache on m28 and tried asix ethernet (needed a patch) and loading from ext2 and vfat (worked). > BTW, I think this was under #ifdef CONFIG_EHCI_DCACHE last time looked > at it. Was this changed by your commit? I think that's the source of the > problem this series tries to address: you've taken buggy code out of > #ifdef ;) I think it's better to just put it back until upper layers > won't be fixed. > > Regards, Ilya. > > [1] http://thread.gmane.org/gmane.comp.boot-loaders.u-boot/114235 Best regards, Marek Vasut
Dear Marek, 28.06.2012 18:37, Marek Vasut wrote: >> Really? Don't forget my old patch [1] then ;) >> Still I think we should rip off all the cache stuff from ehci-hcd until >> all patches for upper layers are included. Again, this stuff doesn't do >> proper things now anyway and USB won't work with dcache enabled. > Have you tested? I enabled dcache on m28 and tried asix ethernet (needed a Surely. (but that probably was an AM3517 with 64 byte cache line) > > patch) and loading from ext2 and vfat (worked). This is just a coincedence ;) Regards, Ilya.
Dear Ilya Yanok, > Dear Marek, > > 28.06.2012 18:37, Marek Vasut wrote: > >> Really? Don't forget my old patch [1] then ;) > >> Still I think we should rip off all the cache stuff from ehci-hcd until > >> all patches for upper layers are included. Again, this stuff doesn't do > >> proper things now anyway and USB won't work with dcache enabled. > > > > Have you tested? I enabled dcache on m28 and tried asix ethernet (needed > > a > > Surely. (but that probably was an AM3517 with 64 byte cache line) m28 is imx28 with 32byte cacheline > > patch) and loading from ext2 and vfat (worked). > > This is just a coincedence ;) Not really, did you check mainline uboot and the fixes in those? > Regards, Ilya. Best regards, Marek Vasut
On 06/28/2012 07:37 AM, Marek Vasut wrote: > Dear Ilya Yanok, > >> Dear Marek, >> >> 28.06.2012 02:48, Marek Vasut wrote: >>>> Sorry for missing this discussion. I think compile-time disabling of the >>>> cache is too brutal. >>>> ehci-hcd cache handling is broken anyway: doing unaligned >>>> flushes/invalidates is a bug, and we know for sure that upper layers >>>> don't care about alignment (and I bet ehci-hcd does this even for its >>>> internal buffers). So what's the point in all this cache handling in >>>> ehci-hcd? It's not going to work anyway and just produces problems. So I >>>> suggest to just disable all this stuff until generic code will be fixed. >>>> Alternatively we can do bounce-buffering inside driver. >>> >>> We should rather introduce generic bounce buffer. But the upper layers >>> are getting fixed recently so we should be getting there. >> >> Really? Don't forget my old patch [1] then ;) >> Still I think we should rip off all the cache stuff from ehci-hcd until >> all patches for upper layers are included. Again, this stuff doesn't do >> proper things now anyway and USB won't work with dcache enabled. > > Have you tested? I enabled dcache on m28 and tried asix ethernet (needed a > patch) and loading from ext2 and vfat (worked). So then we have more places that accidentially aligned to 32bytes since this does not work on TI parts which require 64byte alignment.
Dear Tom Rini, > On 06/28/2012 07:37 AM, Marek Vasut wrote: > > Dear Ilya Yanok, > > > >> Dear Marek, > >> > >> 28.06.2012 02:48, Marek Vasut wrote: > >>>> Sorry for missing this discussion. I think compile-time disabling of > >>>> the cache is too brutal. > >>>> ehci-hcd cache handling is broken anyway: doing unaligned > >>>> flushes/invalidates is a bug, and we know for sure that upper layers > >>>> don't care about alignment (and I bet ehci-hcd does this even for its > >>>> internal buffers). So what's the point in all this cache handling in > >>>> ehci-hcd? It's not going to work anyway and just produces problems. So > >>>> I suggest to just disable all this stuff until generic code will be > >>>> fixed. Alternatively we can do bounce-buffering inside driver. > >>> > >>> We should rather introduce generic bounce buffer. But the upper layers > >>> are getting fixed recently so we should be getting there. > >> > >> Really? Don't forget my old patch [1] then ;) > >> Still I think we should rip off all the cache stuff from ehci-hcd until > >> all patches for upper layers are included. Again, this stuff doesn't do > >> proper things now anyway and USB won't work with dcache enabled. > > > > Have you tested? I enabled dcache on m28 and tried asix ethernet (needed > > a patch) and loading from ext2 and vfat (worked). > > So then we have more places that accidentially aligned to 32bytes since > this does not work on TI parts which require 64byte alignment. Oh, this is very good it's broken. People actually started whining. Now we have to wait until they start identifying the problematic places and fixing them. Best regards, Marek Vasut
On Fri, Jun 29, 2012 at 12:01:58AM +0200, Marek Vasut wrote: > Dear Tom Rini, > > > On 06/28/2012 07:37 AM, Marek Vasut wrote: > > > Dear Ilya Yanok, > > > > > >> Dear Marek, > > >> > > >> 28.06.2012 02:48, Marek Vasut wrote: > > >>>> Sorry for missing this discussion. I think compile-time disabling of > > >>>> the cache is too brutal. > > >>>> ehci-hcd cache handling is broken anyway: doing unaligned > > >>>> flushes/invalidates is a bug, and we know for sure that upper layers > > >>>> don't care about alignment (and I bet ehci-hcd does this even for its > > >>>> internal buffers). So what's the point in all this cache handling in > > >>>> ehci-hcd? It's not going to work anyway and just produces problems. So > > >>>> I suggest to just disable all this stuff until generic code will be > > >>>> fixed. Alternatively we can do bounce-buffering inside driver. > > >>> > > >>> We should rather introduce generic bounce buffer. But the upper layers > > >>> are getting fixed recently so we should be getting there. > > >> > > >> Really? Don't forget my old patch [1] then ;) > > >> Still I think we should rip off all the cache stuff from ehci-hcd until > > >> all patches for upper layers are included. Again, this stuff doesn't do > > >> proper things now anyway and USB won't work with dcache enabled. > > > > > > Have you tested? I enabled dcache on m28 and tried asix ethernet (needed > > > a patch) and loading from ext2 and vfat (worked). > > > > So then we have more places that accidentially aligned to 32bytes since > > this does not work on TI parts which require 64byte alignment. > > Oh, this is very good it's broken. People actually started whining. Now we have > to wait until they start identifying the problematic places and fixing them. Uh-hunh. So I guess for v2012.07 we'll build-time disable dcache for beagle and omap3_evm and leave it on for mcx and see who has time and hardware to fix things for v2012.10.
Dear Tom Rini, > On Fri, Jun 29, 2012 at 12:01:58AM +0200, Marek Vasut wrote: > > Dear Tom Rini, > > > > > On 06/28/2012 07:37 AM, Marek Vasut wrote: > > > > Dear Ilya Yanok, > > > > > > > >> Dear Marek, > > > >> > > > >> 28.06.2012 02:48, Marek Vasut wrote: > > > >>>> Sorry for missing this discussion. I think compile-time disabling > > > >>>> of the cache is too brutal. > > > >>>> ehci-hcd cache handling is broken anyway: doing unaligned > > > >>>> flushes/invalidates is a bug, and we know for sure that upper > > > >>>> layers don't care about alignment (and I bet ehci-hcd does this > > > >>>> even for its internal buffers). So what's the point in all this > > > >>>> cache handling in ehci-hcd? It's not going to work anyway and > > > >>>> just produces problems. So I suggest to just disable all this > > > >>>> stuff until generic code will be fixed. Alternatively we can do > > > >>>> bounce-buffering inside driver. > > > >>> > > > >>> We should rather introduce generic bounce buffer. But the upper > > > >>> layers are getting fixed recently so we should be getting there. > > > >> > > > >> Really? Don't forget my old patch [1] then ;) > > > >> Still I think we should rip off all the cache stuff from ehci-hcd > > > >> until all patches for upper layers are included. Again, this stuff > > > >> doesn't do proper things now anyway and USB won't work with dcache > > > >> enabled. > > > > > > > > Have you tested? I enabled dcache on m28 and tried asix ethernet > > > > (needed a patch) and loading from ext2 and vfat (worked). > > > > > > So then we have more places that accidentially aligned to 32bytes since > > > this does not work on TI parts which require 64byte alignment. > > > > Oh, this is very good it's broken. People actually started whining. Now > > we have to wait until they start identifying the problematic places and > > fixing them. > > Uh-hunh. So I guess for v2012.07 we'll build-time disable dcache for > beagle and omap3_evm Didn't you fix the issues? > and leave it on for mcx and see who has time and > hardware to fix things for v2012.10. Or we fix it for mcx too until .07 is out ? Best regards, Marek Vasut
On 06/28/2012 03:36 PM, Marek Vasut wrote: > Dear Tom Rini, > >> On Fri, Jun 29, 2012 at 12:01:58AM +0200, Marek Vasut wrote: >>> Dear Tom Rini, >>> >>>> On 06/28/2012 07:37 AM, Marek Vasut wrote: >>>>> Dear Ilya Yanok, >>>>> >>>>>> Dear Marek, >>>>>> >>>>>> 28.06.2012 02:48, Marek Vasut wrote: >>>>>>>> Sorry for missing this discussion. I think compile-time disabling >>>>>>>> of the cache is too brutal. >>>>>>>> ehci-hcd cache handling is broken anyway: doing unaligned >>>>>>>> flushes/invalidates is a bug, and we know for sure that upper >>>>>>>> layers don't care about alignment (and I bet ehci-hcd does this >>>>>>>> even for its internal buffers). So what's the point in all this >>>>>>>> cache handling in ehci-hcd? It's not going to work anyway and >>>>>>>> just produces problems. So I suggest to just disable all this >>>>>>>> stuff until generic code will be fixed. Alternatively we can do >>>>>>>> bounce-buffering inside driver. >>>>>>> >>>>>>> We should rather introduce generic bounce buffer. But the upper >>>>>>> layers are getting fixed recently so we should be getting there. >>>>>> >>>>>> Really? Don't forget my old patch [1] then ;) >>>>>> Still I think we should rip off all the cache stuff from ehci-hcd >>>>>> until all patches for upper layers are included. Again, this stuff >>>>>> doesn't do proper things now anyway and USB won't work with dcache >>>>>> enabled. >>>>> >>>>> Have you tested? I enabled dcache on m28 and tried asix ethernet >>>>> (needed a patch) and loading from ext2 and vfat (worked). >>>> >>>> So then we have more places that accidentially aligned to 32bytes since >>>> this does not work on TI parts which require 64byte alignment. >>> >>> Oh, this is very good it's broken. People actually started whining. Now >>> we have to wait until they start identifying the problematic places and >>> fixing them. >> >> Uh-hunh. So I guess for v2012.07 we'll build-time disable dcache for >> beagle and omap3_evm > > Didn't you fix the issues? > >> and leave it on for mcx and see who has time and >> hardware to fix things for v2012.10. > > Or we fix it for mcx too until .07 is out ? To clarify for everyone, the first part of this series fixes some alignment issues for things that were not starting address aligned. There still exist end-address alignment issues within ehci-hcd. The time I have for this problem right now boils down to disable dcache for these boards so that USB is still functional.
Dear Tom Rini, > On 06/28/2012 03:36 PM, Marek Vasut wrote: > > Dear Tom Rini, > > > >> On Fri, Jun 29, 2012 at 12:01:58AM +0200, Marek Vasut wrote: > >>> Dear Tom Rini, > >>> > >>>> On 06/28/2012 07:37 AM, Marek Vasut wrote: > >>>>> Dear Ilya Yanok, > >>>>> > >>>>>> Dear Marek, > >>>>>> > >>>>>> 28.06.2012 02:48, Marek Vasut wrote: > >>>>>>>> Sorry for missing this discussion. I think compile-time disabling > >>>>>>>> of the cache is too brutal. > >>>>>>>> ehci-hcd cache handling is broken anyway: doing unaligned > >>>>>>>> flushes/invalidates is a bug, and we know for sure that upper > >>>>>>>> layers don't care about alignment (and I bet ehci-hcd does this > >>>>>>>> even for its internal buffers). So what's the point in all this > >>>>>>>> cache handling in ehci-hcd? It's not going to work anyway and > >>>>>>>> just produces problems. So I suggest to just disable all this > >>>>>>>> stuff until generic code will be fixed. Alternatively we can do > >>>>>>>> bounce-buffering inside driver. > >>>>>>> > >>>>>>> We should rather introduce generic bounce buffer. But the upper > >>>>>>> layers are getting fixed recently so we should be getting there. > >>>>>> > >>>>>> Really? Don't forget my old patch [1] then ;) > >>>>>> Still I think we should rip off all the cache stuff from ehci-hcd > >>>>>> until all patches for upper layers are included. Again, this stuff > >>>>>> doesn't do proper things now anyway and USB won't work with dcache > >>>>>> enabled. > >>>>> > >>>>> Have you tested? I enabled dcache on m28 and tried asix ethernet > >>>>> (needed a patch) and loading from ext2 and vfat (worked). > >>>> > >>>> So then we have more places that accidentially aligned to 32bytes > >>>> since this does not work on TI parts which require 64byte alignment. > >>> > >>> Oh, this is very good it's broken. People actually started whining. Now > >>> we have to wait until they start identifying the problematic places and > >>> fixing them. > >> > >> Uh-hunh. So I guess for v2012.07 we'll build-time disable dcache for > >> beagle and omap3_evm > > > > Didn't you fix the issues? > > > >> and leave it on for mcx and see who has time and > >> hardware to fix things for v2012.10. > > > > Or we fix it for mcx too until .07 is out ? > > To clarify for everyone, the first part of this series fixes some > alignment issues for things that were not starting address aligned. > There still exist end-address alignment issues within ehci-hcd. The > time I have for this problem right now boils down to disable dcache for > these boards so that USB is still functional. To clarify it even further -- it always worked just by sheer coincidence ... Best regards, Marek Vasut
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 06/28/2012 05:54 PM, Marek Vasut wrote: > Dear Tom Rini, > >> On 06/28/2012 03:36 PM, Marek Vasut wrote: >>> Dear Tom Rini, >>> >>>> On Fri, Jun 29, 2012 at 12:01:58AM +0200, Marek Vasut wrote: >>>>> Dear Tom Rini, >>>>> >>>>>> On 06/28/2012 07:37 AM, Marek Vasut wrote: >>>>>>> Dear Ilya Yanok, >>>>>>> >>>>>>>> Dear Marek, >>>>>>>> >>>>>>>> 28.06.2012 02:48, Marek Vasut wrote: >>>>>>>>>> Sorry for missing this discussion. I think >>>>>>>>>> compile-time disabling of the cache is too >>>>>>>>>> brutal. ehci-hcd cache handling is broken >>>>>>>>>> anyway: doing unaligned flushes/invalidates is a >>>>>>>>>> bug, and we know for sure that upper layers don't >>>>>>>>>> care about alignment (and I bet ehci-hcd does >>>>>>>>>> this even for its internal buffers). So what's >>>>>>>>>> the point in all this cache handling in >>>>>>>>>> ehci-hcd? It's not going to work anyway and just >>>>>>>>>> produces problems. So I suggest to just disable >>>>>>>>>> all this stuff until generic code will be fixed. >>>>>>>>>> Alternatively we can do bounce-buffering inside >>>>>>>>>> driver. >>>>>>>>> >>>>>>>>> We should rather introduce generic bounce buffer. >>>>>>>>> But the upper layers are getting fixed recently so >>>>>>>>> we should be getting there. >>>>>>>> >>>>>>>> Really? Don't forget my old patch [1] then ;) Still >>>>>>>> I think we should rip off all the cache stuff from >>>>>>>> ehci-hcd until all patches for upper layers are >>>>>>>> included. Again, this stuff doesn't do proper things >>>>>>>> now anyway and USB won't work with dcache enabled. >>>>>>> >>>>>>> Have you tested? I enabled dcache on m28 and tried >>>>>>> asix ethernet (needed a patch) and loading from ext2 >>>>>>> and vfat (worked). >>>>>> >>>>>> So then we have more places that accidentially aligned >>>>>> to 32bytes since this does not work on TI parts which >>>>>> require 64byte alignment. >>>>> >>>>> Oh, this is very good it's broken. People actually started >>>>> whining. Now we have to wait until they start identifying >>>>> the problematic places and fixing them. >>>> >>>> Uh-hunh. So I guess for v2012.07 we'll build-time disable >>>> dcache for beagle and omap3_evm >>> >>> Didn't you fix the issues? >>> >>>> and leave it on for mcx and see who has time and hardware to >>>> fix things for v2012.10. >>> >>> Or we fix it for mcx too until .07 is out ? >> >> To clarify for everyone, the first part of this series fixes some >> alignment issues for things that were not starting address >> aligned. There still exist end-address alignment issues within >> ehci-hcd. The time I have for this problem right now boils down >> to disable dcache for these boards so that USB is still >> functional. > > To clarify it even further -- it always worked just by sheer > coincidence ... No, it didn't. It used to work in a timely manner, with dcache disabled but support enabled at build time. Now it works, in an unusably slow manner, with dcache disabled but support enabled at build time. It continues to work in a timely manner with dcache support disabled at build time. On any platform with >32byte alignment requirements for cache flushing. - -- Tom -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iQIcBAEBAgAGBQJP7Q+BAAoJENk4IS6UOR1WhRIP/1NlLlF4iqiPAr7ZhnVWHUS5 IO76xwcIc69ZZsbD46EAG3XvUpmSkszP7fmr9KZerPYylH4Ne4TOtxqF7yg8rVnx 856tosee9PU4l5yUbt3JbT/XYoT5ivcw7n058PO3BliHcSSZ7BEMsaKOEOUzm+x7 Alzu76UM+YUkIGfGnQMWQnAcT0alNp/aa2KZriWPCOTKj2NKghRuO7xlBF6KGSBz S/9MaiWnC0PvkCc2fhYt7JsIItz19gnx31M/JhU6gTxOR1WfMAlsQfjDXs5wTZI+ rCuW0H/GX9O3FpXG33Uecf2dzg8e7t45qXRND/7sfHCx4M2nE4HWY+RvSP1XCacH MTTqeBZm7RpDWKO7P4dQW8GWLkUzNYLOySTyLp2Rnh7882C1QaNbRWplvC+4NB3g KbSK8H0PqEu/pVJ3Dl2kutMY6PcKX/H1l7x4swEIoNtor6Lx3wE5rzhjjDqElGl7 HVWVjI2Hgco9F043pCjkS3Mb7pNIcJ3/yMjDC8C7PWDLrW9+fPwkbZxqCTJGaBWS Qlo6RbKxE4yDwDtbJXJspy1ML0jK2DO0NbFD+LDfIrecpKUpTtxBee/b4E6DGcil LJ6OWprUnxYVM4LHUjdvI9pcX92gwOw6EXpcvPNiHOl9ZO8nThCs5QElg2OAJmlw qgNzEI3zqq4GimdS+0oh =Lc3K -----END PGP SIGNATURE-----
Dear Marek, 28.06.2012 19:41, Marek Vasut wrote: >> Surely. (but that probably was an AM3517 with 64 byte cache line) > m28 is imx28 with 32byte cacheline You are lucky then. But some systems have bigger cacheline, right? >>> patch) and loading from ext2 and vfat (worked). >> This is just a coincedence ;) > Not really, did you check mainline uboot and the fixes in those? Surely I did. Let's take a look: > do { > /* Invalidate dcache */ > invalidate_dcache_range((uint32_t)&qh_list, > (uint32_t)&qh_list + sizeof(struct QH)); > invalidate_dcache_range((uint32_t)&qh, > (uint32_t)&qh + sizeof(struct QH)); > invalidate_dcache_range((uint32_t)qtd, > (uint32_t)qtd + sizeof(qtd)); These guys are 32-byte aligned, right? So the assumption is that cacheline is not greater than 32. Sounds like a bug to me (though could be fixed rather easily with USB_MINALIGN introduced by Tom). > token = hc32_to_cpu(vtd->qt_token); > if (!(token & 0x80)) > break; > WATCHDOG_RESET(); > } while (get_timer(ts) < timeout); > > /* Invalidate the memory area occupied by buffer */ > invalidate_dcache_range(((uint32_t)buffer & ~31), > ((uint32_t)buffer & ~31) + roundup(length, 32)); That's worse. First of all, rounding is wrong: consider buffer=31, length=32. But that's easy to fix too. The bad part is that with writeback cache you can't just enlarge the range to cover the buffer and fit alignment requirements -- this can potentially destroy some changes that are in the same cache line but not yet stored to RAM. Regards, Ilya.
Dear Marek, 29.06.2012 04:54, Marek Vasut wrote: >> To clarify for everyone, the first part of this series fixes some >> alignment issues for things that were not starting address aligned. >> There still exist end-address alignment issues within ehci-hcd. The >> time I have for this problem right now boils down to disable dcache for >> these boards so that USB is still functional. > To clarify it even further -- it always worked just by sheer coincidence ... Not exactly. It never worked (at least on my systems) with D-Cache enabled. But at least we had a choice of run-time disabled dcache. With the recent changes we have to disable cache support at compile time. Regards, Ilya.
Dear Ilya Yanok, > Dear Marek, > > 28.06.2012 19:41, Marek Vasut wrote: > >> Surely. (but that probably was an AM3517 with 64 byte cache line) > > > > m28 is imx28 with 32byte cacheline > > You are lucky then. But some systems have bigger cacheline, right? Yes, and I just got a perfect system for the job. > > >>> patch) and loading from ext2 and vfat (worked). > >> > >> This is just a coincedence ;) > > > > Not really, did you check mainline uboot and the fixes in those? > > Surely I did. Let's take a look: > > do { > > > > /* Invalidate dcache */ > > invalidate_dcache_range((uint32_t)&qh_list, > > > > (uint32_t)&qh_list + sizeof(struct QH)); > > > > invalidate_dcache_range((uint32_t)&qh, > > > > (uint32_t)&qh + sizeof(struct QH)); > > > > invalidate_dcache_range((uint32_t)qtd, > > > > (uint32_t)qtd + sizeof(qtd)); > > These guys are 32-byte aligned, right? So the assumption is that > cacheline is not greater than 32. Sounds like a bug to me (though could > be fixed rather easily with USB_MINALIGN introduced by Tom). Oooh, good catch indeed. Actually, look at the structures, even they have some weird alignment crap in them. Maybe that can also be dropped and the alignment shall be fixed properly. I'll have to research this more. > > > token = hc32_to_cpu(vtd->qt_token); > > if (!(token & 0x80)) > > > > break; > > > > WATCHDOG_RESET(); > > > > } while (get_timer(ts) < timeout); > > > > /* Invalidate the memory area occupied by buffer */ > > invalidate_dcache_range(((uint32_t)buffer & ~31), > > > > ((uint32_t)buffer & ~31) + roundup(length, 32)); > > That's worse. First of all, rounding is wrong: consider buffer=31, > length=32. But that's easy to fix too. The bad part is that with > writeback cache you can't just enlarge the range to cover the buffer and > fit alignment requirements -- this can potentially destroy some changes > that are in the same cache line but not yet stored to RAM. Correct, so we have multiple bugs in here that need to be squashed ASAP. Ilya, can you possibly submit a patch for this? > > Regards, Ilya. Best regards, Marek Vasut
Dear Ilya Yanok, > Dear Marek, > > 29.06.2012 04:54, Marek Vasut wrote: > >> To clarify for everyone, the first part of this series fixes some > >> alignment issues for things that were not starting address aligned. > >> There still exist end-address alignment issues within ehci-hcd. The > >> time I have for this problem right now boils down to disable dcache for > >> these boards so that USB is still functional. > > > > To clarify it even further -- it always worked just by sheer coincidence > > ... > > Not exactly. It never worked (at least on my systems) with D-Cache > enabled. But at least we had a choice of run-time disabled dcache. With > the recent changes we have to disable cache support at compile time. I see what you're after. But do you consider runtime disabling the cache is the way to go or it's a way of hiding bugs? > Regards, Ilya. Best regards, Marek Vasut
Dear Marek, 30.06.2012 23:27, Marek Vasut wrote: >>> do { >>> >>> /* Invalidate dcache */ >>> invalidate_dcache_range((uint32_t)&qh_list, >>> >>> (uint32_t)&qh_list + sizeof(struct QH)); >>> >>> invalidate_dcache_range((uint32_t)&qh, >>> >>> (uint32_t)&qh + sizeof(struct QH)); >>> >>> invalidate_dcache_range((uint32_t)qtd, >>> >>> (uint32_t)qtd + sizeof(qtd)); >> These guys are 32-byte aligned, right? So the assumption is that >> cacheline is not greater than 32. Sounds like a bug to me (though could >> be fixed rather easily with USB_MINALIGN introduced by Tom). > Oooh, good catch indeed. Actually, look at the structures, even they have some > weird alignment crap in them. Maybe that can also be dropped and the alignment > shall be fixed properly. I'll have to research this more. I guess that's because they have to be both address and size aligned. >>> token = hc32_to_cpu(vtd->qt_token); >>> if (!(token& 0x80)) >>> >>> break; >>> >>> WATCHDOG_RESET(); >>> >>> } while (get_timer(ts)< timeout); >>> >>> /* Invalidate the memory area occupied by buffer */ >>> invalidate_dcache_range(((uint32_t)buffer& ~31), >>> >>> ((uint32_t)buffer& ~31) + roundup(length, 32)); >> That's worse. First of all, rounding is wrong: consider buffer=31, >> length=32. But that's easy to fix too. The bad part is that with >> writeback cache you can't just enlarge the range to cover the buffer and >> fit alignment requirements -- this can potentially destroy some changes >> that are in the same cache line but not yet stored to RAM. > Correct, so we have multiple bugs in here that need to be squashed ASAP. > > Ilya, can you possibly submit a patch for this? Hm.. Fixing alignment for qh, qd and stuff is easy, I can surely submit a patch for this. But I don't really know what to do this the last one: if we are at this point there is no correct way out... We can increase the range to cover the buffer or decrease it to be inside buffer -- but both options are incorrect. Actually we should return error when unaligned buffer is submitted in the first place... But this will likely break a lot of systems... Probably put this check under if (dcache_enabled())? Regards, Ilya.
Dear Marek, 30.06.2012 23:28, Marek Vasut wrote: >> Not exactly. It never worked (at least on my systems) with D-Cache >> enabled. But at least we had a choice of run-time disabled dcache. With >> the recent changes we have to disable cache support at compile time. > I see what you're after. But do you consider runtime disabling the cache is the > way to go or it's a way of hiding bugs? Both ;) And now we are going to hide even more bugs with compile-time disabling :( Regards, Ilya.
On 07/03/2012 01:13 PM, Ilya Yanok wrote: > Dear Marek, > > 30.06.2012 23:28, Marek Vasut wrote: >>> Not exactly. It never worked (at least on my systems) with D-Cache >>> enabled. But at least we had a choice of run-time disabled dcache. With >>> the recent changes we have to disable cache support at compile time. >> I see what you're after. But do you consider runtime disabling the >> cache is the >> way to go or it's a way of hiding bugs? > > Both ;) And now we are going to hide even more bugs with compile-time > disabling :( Does someone wish to argue we should disable USB support instead on these platforms? I don't see anyone arguing "I have time to fix this for v2012.07".
Hi Tom, 04.07.2012 00:43, Tom Rini wrote: > On 07/03/2012 01:13 PM, Ilya Yanok wrote: >> Dear Marek, >> >> 30.06.2012 23:28, Marek Vasut wrote: >>>> Not exactly. It never worked (at least on my systems) with D-Cache >>>> enabled. But at least we had a choice of run-time disabled dcache. With >>>> the recent changes we have to disable cache support at compile time. >>> I see what you're after. But do you consider runtime disabling the >>> cache is the >>> way to go or it's a way of hiding bugs? >> Both ;) And now we are going to hide even more bugs with compile-time >> disabling :( > Does someone wish to argue we should disable USB support instead on > these platforms? I don't see anyone arguing "I have time to fix this > for v2012.07". I just looked at the code more carefully and it seems that most of the upper layers are in much better shape than I thought. So I think we should just extend your 2/6 patch to fix both address and size for structs QH and qtd and don't mess with buffer at all: if we got unaligned buffer -- it's definetely upper layer bug so we should produce some noise in this case. As I said upper layers seems to be in good shape so hopefully there won't be too much noise. Hm, probably we should put buffer invalidation under if(dcache_enabled()) to leave run-time cache disabling as rescue option for broken upper-layer code.. I'm working on the patch now and hopefully will post it this night. Regards, Ilya.
Dear Ilya Yanok, > Dear Marek, > > 30.06.2012 23:27, Marek Vasut wrote: > >>> do { > >>> > >>> /* Invalidate dcache */ > >>> invalidate_dcache_range((uint32_t)&qh_list, > >>> > >>> (uint32_t)&qh_list + sizeof(struct QH)); > >>> > >>> invalidate_dcache_range((uint32_t)&qh, > >>> > >>> (uint32_t)&qh + sizeof(struct QH)); > >>> > >>> invalidate_dcache_range((uint32_t)qtd, > >>> > >>> (uint32_t)qtd + sizeof(qtd)); > >> > >> These guys are 32-byte aligned, right? So the assumption is that > >> cacheline is not greater than 32. Sounds like a bug to me (though could > >> be fixed rather easily with USB_MINALIGN introduced by Tom). > > > > Oooh, good catch indeed. Actually, look at the structures, even they have > > some weird alignment crap in them. Maybe that can also be dropped and > > the alignment shall be fixed properly. I'll have to research this more. > > I guess that's because they have to be both address and size aligned. > > >>> token = hc32_to_cpu(vtd->qt_token); > >>> if (!(token& 0x80)) > >>> > >>> break; > >>> > >>> WATCHDOG_RESET(); > >>> > >>> } while (get_timer(ts)< timeout); > >>> > >>> /* Invalidate the memory area occupied by buffer */ > >>> invalidate_dcache_range(((uint32_t)buffer& ~31), > >>> > >>> ((uint32_t)buffer& ~31) + roundup(length, 32)); > >> > >> That's worse. First of all, rounding is wrong: consider buffer=31, > >> length=32. But that's easy to fix too. The bad part is that with > >> writeback cache you can't just enlarge the range to cover the buffer and > >> fit alignment requirements -- this can potentially destroy some changes > >> that are in the same cache line but not yet stored to RAM. > > > > Correct, so we have multiple bugs in here that need to be squashed ASAP. > > > > Ilya, can you possibly submit a patch for this? > > Hm.. Fixing alignment for qh, qd and stuff is easy, I can surely submit > a patch for this. Well ... partial patch is still better than none. > But I don't really know what to do this the last one: > if we are at this point there is no correct way out... We can increase > the range to cover the buffer or decrease it to be inside buffer -- but > both options are incorrect. Actually we should return error when > unaligned buffer is submitted in the first place... But this will likely > break a lot of systems... Probably put this check under if > (dcache_enabled())? Bounce buffer I guess ... with warning that you'll hit performance penalty becaue you're dumb and doing unaligned transfer. And for internal uboot stuff, we can align it. > > Regards, Ilya. Best regards, Marek Vasut
Dear Tom Rini, > On 07/03/2012 01:13 PM, Ilya Yanok wrote: > > Dear Marek, > > > > 30.06.2012 23:28, Marek Vasut wrote: > >>> Not exactly. It never worked (at least on my systems) with D-Cache > >>> enabled. But at least we had a choice of run-time disabled dcache. With > >>> the recent changes we have to disable cache support at compile time. > >> > >> I see what you're after. But do you consider runtime disabling the > >> cache is the > >> way to go or it's a way of hiding bugs? > > > > Both ;) And now we are going to hide even more bugs with compile-time > > disabling :( > > Does someone wish to argue we should disable USB support instead on > these platforms? I don't see anyone arguing "I have time to fix this > for v2012.07". I don't have time to fix this, I'm hacking 200% already. Best regards, Marek Vasut
Dear Ilya Yanok, > Hi Tom, > > 04.07.2012 00:43, Tom Rini wrote: > > On 07/03/2012 01:13 PM, Ilya Yanok wrote: > >> Dear Marek, > >> > >> 30.06.2012 23:28, Marek Vasut wrote: > >>>> Not exactly. It never worked (at least on my systems) with D-Cache > >>>> enabled. But at least we had a choice of run-time disabled dcache. > >>>> With the recent changes we have to disable cache support at compile > >>>> time. > >>> > >>> I see what you're after. But do you consider runtime disabling the > >>> cache is the > >>> way to go or it's a way of hiding bugs? > >> > >> Both ;) And now we are going to hide even more bugs with compile-time > >> disabling :( > > > > Does someone wish to argue we should disable USB support instead on > > these platforms? I don't see anyone arguing "I have time to fix this > > for v2012.07". > > I just looked at the code more carefully and it seems that most of the > upper layers are in much better shape than I thought. So I think we > should just extend your 2/6 patch to fix both address and size for > structs QH and qtd and don't mess with buffer at all: if we got > unaligned buffer -- it's definetely upper layer bug so we should produce > some noise in this case. As I said upper layers seems to be in good > shape so hopefully there won't be too much noise. > > Hm, probably we should put buffer invalidation under > if(dcache_enabled()) to leave run-time cache disabling as rescue option > for broken upper-layer code.. > > I'm working on the patch now and hopefully will post it this night. Ilya, thank you for saving my back ;-) And thank you for investing your time into this. > Regards, Ilya. Best regards, Marek Vasut
Hi All, 04.07.2012 04:14, Marek Vasut wrote: > Ilya, thank you for saving my back ;-) > > And thank you for investing your time into this. You are welcome ;) Just posted the patch. No dealing with unaligned buffers from upper layers for now but at least fatload with aligned address works fine. Regards, Ilya.
diff --git a/include/configs/mcx.h b/include/configs/mcx.h index f6a83a8..0b29b08 100644 --- a/include/configs/mcx.h +++ b/include/configs/mcx.h @@ -110,9 +110,9 @@ #define CONFIG_OMAP3_GPIO_5 #define CONFIG_USB_EHCI #define CONFIG_USB_EHCI_OMAP +#define CONFIG_SYS_DCACHE_OFF /* USB_EHCI is unusable with DCACHE support */ #define CONFIG_USB_ULPI #define CONFIG_USB_ULPI_VIEWPORT_OMAP -/*#define CONFIG_EHCI_DCACHE*/ /* leave it disabled for now */ #define CONFIG_OMAP_EHCI_PHY1_RESET_GPIO 154 #define CONFIG_OMAP_EHCI_PHY2_RESET_GPIO 152 #define CONFIG_SYS_USB_EHCI_MAX_ROOT_PORTS 3
USB EHCI and DCACHE are not compatible, so disable DCACHE support at build-time as run-time disable is insufficient for USB use. Cc: Ilya Yanok <yanok@emcraft.com> Signed-off-by: Tom Rini <trini@ti.com> --- include/configs/mcx.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)