[U-Boot] [PATCH] ehci-hcd.c, musb_core, usb.h: Add USB_DMA_MINALIGN define for cache alignment

[Ilya Yanok]

Hi Marek,

On Thu, Jul 5, 2012 at 12:24 AM, Marek Vasut <marex at denx.de> wrote:

>
> > -static struct QH qh_list __attribute__((aligned(32)));
> > +static char __qh_list[ALIGN(sizeof(struct QH), USB_DMA_MINALIGN)]
> > +                     __attribute__((aligned(USB_DMA_MINALIGN)));
> > +static struct QH *qh_list = (struct QH *)__qh_list;
>
>
> Maybe we should create DEFINE_ALIGNED_VARIABLE as a common.h macro?
>

Yep. I even thought about this but decided not to do... can't recall why.
Now I think it's really a good idea.


>
> >  static struct descriptor {
> >       struct usb_hub_descriptor hub;
> > @@ -172,13 +174,13 @@ static int ehci_td_buffer(struct qTD *td, void
> *buf,
> > size_t sz) {
> >       uint32_t delta, next;
> >       uint32_t addr = (uint32_t)buf;
> > -     size_t rsz = roundup(sz, 32);
> > +     size_t rsz = roundup(sz, USB_DMA_MINALIGN);
> >       int idx;
> >
> >       if (sz != rsz)
> >               debug("EHCI-HCD: Misaligned buffer size (%08x)\n", sz);
> >
> > -     if (addr & 31)
> > +     if (addr != ALIGN(addr, USB_DMA_MINALIGN))
> >               debug("EHCI-HCD: Misaligned buffer address (%p)\n", buf);
>
> Good :)
>

Well, thinking more about this, it's actually a wrong place to check
this... It can be setup packet buffer, which can be unaligned (this is ok
cause it's only flushed and never invalidated). And size can always be
unaligned...



> >       /* Flush dcache */
> > -     flush_dcache_range((uint32_t)&qh_list,
> > -             (uint32_t)&qh_list + sizeof(struct QH));
> > -     flush_dcache_range((uint32_t)&qh, (uint32_t)&qh + sizeof(struct
> QH));
> > -     flush_dcache_range((uint32_t)qtd, (uint32_t)qtd + sizeof(qtd));
> > +     flush_dcache_range((uint32_t)qh_list,
> > +             (uint32_t)qh_list + ALIGN(sizeof(struct QH),
> USB_DMA_MINALIGN));
> > +     flush_dcache_range((uint32_t)qh, (uint32_t)qh +
> > +             ALIGN(sizeof(struct QH), USB_DMA_MINALIGN));
> > +     flush_dcache_range((uint32_t)qtd, (uint32_t)qtd +
> > +             ALIGN(3*sizeof(*qtd), USB_DMA_MINALIGN));
>
> Maybe we should make a macro for those things here to prevent such
> spaghetti of
> code ?
>

Hm.. Maybe. Ideas? ;) Actually I also thought about moving all this stuff
to a single proper aligned buffer and do flush/invalidate for a whole
buffer at once. It can save us some space... but it's BSS anyway... Don't
know if it's worth it...

Regards, Ilya.