[U-Boot] [PATCH] net: fec: Avoid MX28 bus sync issue

[Albert ARIBAUD <albert.u.boot@aribaud.net>]

Hi Troy,

On Mon, 15 Jul 2013 12:59:56 -0700, Troy Kisky
<troy.kisky@boundarydevices.com> wrote:

> On 7/15/2013 10:39 AM, Troy Kisky wrote:
> > On 7/15/2013 6:41 AM, Albert ARIBAUD wrote:
> >> Hi Troy,
> >>
> >> On Fri, 12 Jul 2013 19:43:07 -0700, Troy Kisky
> >> <troy.kisky@boundarydevices.com> wrote:
> >>
> >>> On 7/11/2013 4:18 PM, Fabio Estevam wrote:
> >>>> On Thu, Jul 11, 2013 at 8:03 PM, Marek Vasut <marex@denx.de> wrote:
> >>>>> The MX28 multi-layer AHB bus can be too slow and trigger the
> >>>>> FEC DMA too early, before all the data hit the DRAM. This patch
> >>>>> ensures the data are written in the RAM before the DMA starts.
> >>>>> Please see the comment in the patch for full details.
> >>>>>
> >>>>> This patch was produced with an amazing help from Albert Aribaud,
> >>>>> who pointed out it can possibly be such a bus synchronisation
> >>>>> issue.
> >>>>>
> >>>>> Signed-off-by: Marek Vasut <marex@denx.de>
> >>>>> Cc: Albert ARIBAUD <albert.u.boot@aribaud.net>
> >>>>> Cc: Fabio Estevam <fabio.estevam@freescale.com>
> >>>>> Cc: Stefano Babic <sbabic@denx.de>
> >>>> Excellent, managed to transfer 90MB via TFTP on mx28evk without a
> >>>> single timeout.
> >>>>
> >>>> Tested-by: Fabio Estevam <fabio.estevam@freescale.com>
> >>>> _______________________________________________
> >>>> U-Boot mailing list
> >>>> U-Boot at lists.denx.de
> >>>> http://lists.denx.de/mailman/listinfo/u-boot
> >>>>
> >>> Perhaps this because our memory barriers are lacking.
> >>>
> >>> Linux has this code
> >>> asm/io.h:#define writel(v,c)            ({ __iowmb();
> >>> writel_relaxed(v,c); })
> >>>
> >>> asm/io.h-#ifdef CONFIG_ARM_DMA_MEM_BUFFERABLE
> >>> asm/io.h-#include <asm/barrier.h>
> >>> asm/io.h-#define __iormb()              rmb()
> >>> asm/io.h:#define __iowmb()              wmb()
> >>> asm/io.h-#else
> >>> asm/io.h-#define __iormb()              do { } while (0)
> >>> asm/io.h:#define __iowmb()              do { } while (0)
> >>> asm/io.h-#endif
> >>>
> >>> asm/io.h-#ifdef CONFIG_ARM_DMA_MEM_BUFFERABLE
> >>> asm/io.h-#include <asm/barrier.h>
> >>> asm/io.h-#define __iormb()              rmb()
> >>> asm/io.h:#define __iowmb()              wmb()
> >>> asm/io.h-#else
> >>> asm/io.h-#define __iormb()              do { } while (0)
> >>> asm/io.h:#define __iowmb()              do { } while (0)
> >>> asm/io.h-#endif
> >>>
> >>> asm/barrier.h-#elif defined(CONFIG_ARM_DMA_MEM_BUFFERABLE) ||
> >>> defined(CONFIG_SMP)
> >>> asm/barrier.h-#define mb()              do { dsb(); outer_sync(); }
> >>> while (0)
> >>> asm/barrier.h-#define rmb()             dsb()
> >>> asm/barrier.h:#define wmb()             mb()
> >>> asm/barrier.h-#else
> >>> asm/barrier.h-#define mb()              barrier()
> >>> asm/barrier.h-#define rmb()             barrier()
> >>> asm/barrier.h:#define wmb()             barrier()
> >>> asm/barrier.h-#endif
> >>>
> >>> asm/barrier.h-#if __LINUX_ARM_ARCH__ >= 7
> >>> asm/barrier.h-#define isb() __asm__ __volatile__ ("isb" : : : "memory")
> >>> asm/barrier.h:#define dsb() __asm__ __volatile__ ("dsb" : : : "memory")
> >>> asm/barrier.h-#define dmb() __asm__ __volatile__ ("dmb" : : : "memory")
> >>> asm/barrier.h-#elif defined(CONFIG_CPU_XSC3) || __LINUX_ARM_ARCH__ == 6
> >>> asm/barrier.h-#define isb() __asm__ __volatile__ ("mcr p15, 0, %0, c7,
> >>> c5, 4" \
> >>> asm/barrier.h-                              : : "r" (0) : "memory")
> >>> asm/barrier.h:#define dsb() __asm__ __volatile__ ("mcr p15, 0, %0, c7,
> >>> c10, 4" \
> >>> asm/barrier.h-                              : : "r" (0) : "memory")
> >>> asm/barrier.h-#define dmb() __asm__ __volatile__ ("mcr p15, 0, %0, c7,
> >>> c10, 5" \
> >>>
> >>> _____________________________________
> >>> Can you try just adding a dsb() instead of the dummy read?
> >>>
> >>> If this also fixes your problem, it seems better to fix our writel 
> >>> macro
> >> Not sure I understand who you are answering to exactly, as Fabio
> >> indicates his problem is solved.
> >>
> >> Besides, Marek and I had in fact investigated barriers, adding some as
> >> I did in times past in mvgbe.c, and fiddling with the one already in
> >> dcache_flush_range(). None of this had any effect.
> >
> > You tried adding a  dsb()  to dcache_flush_range()?
> > That should have fixed the problem as well.
> >
> >>
> >> However, if our barriers are lacking, then they may fail us somehow on
> >> other occasions, so best is if we analyze the failings. Can you clarify
> >> in what respect exactly they are lacking? For instance, are all our
> >> barriers lacking, or only some, and which ones?
> >>
> >> Amicalement,
> >
> > Linux has
> >
> > Kconfig:config ARM_DMA_MEM_BUFFERABLE
> > Kconfig-        bool "Use non-cacheable memory for DMA" if (CPU_V6 || 
> > CPU_V6K) && !CPU_V7
> > Kconfig-        depends on !(MACH_REALVIEW_PB1176 || 
> > REALVIEW_EB_ARM11MP || \
> > Kconfig-                     MACH_REALVIEW_PB11MP)
> > Kconfig-        default y if CPU_V6 || CPU_V6K || CPU_V7
> > Kconfig-        help
> >
> >
> > So, if this symbol is y then all writel/readl will be preceded by a 
> > dsb() as shown above.
> >
> > However, u-boot probably uses cacheable memory for dma, so maybe 
> > u-boot is also correct with
> >
> > asm/io.h-#define dmb()          __asm__ __volatile__ ("" : : : "memory")
> > asm/io.h-#define __iormb()    dmb()
> > asm/io.h:#define __iowmb()    dmb()
> >
> >
> > and no dsb(), but perhaps flush_dcache still needs one at the end.
> >
> >
> > Troy
> >
> 
> for armv7, flush dcache does have a dsb.
> 
> //u-boot
> #define CP15DSB asm volatile ("mcr     p15, 0, %0, c7, c10, 4" : : "r" (0))
> 
> //linux
> asm/barrier.h:#define dsb() __asm__ __volatile__ ("mcr p15, 0, %0, c7, 
> c10, 4"  : : "r" (0) : "memory")
> 
> 
> 
> Don't know why I didn't see that before.
> So, I don't know why that wasn't good enough.
> 
> Maybe  CONFIG_SYS_DCACHE_OFF was set and
> 
> void flush_dcache_range(unsigned long start, unsigned long stop)
> {
> }
> 
> Needs a dsb too???

I think everything is explained in Marek's commit message.

> Troy

Amicalement,
-- 
Albert.