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.

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

Reply via email to