From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marex@denx.de>
Date: Tue, 3 Jul 2012 23:23:40 +0200
Subject: [U-Boot] [PATCH v4 3/6] mcx: Disable DCACHE since USB EHCI is
	enabled
In-Reply-To: <4FF351B0.30109@emcraft.com>
References: <1340209283-3404-1-git-send-email-trini@ti.com>
	<201206302127.10984.marex@denx.de> <4FF351B0.30109@emcraft.com>
Message-ID: <201207032323.40848.marex@denx.de>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

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