[U-Boot] [PATCH v4 3/6] mcx: Disable DCACHE since USB EHCI is enabled

Marek Vasut
2012-07-03T21:23:40Z

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