[U-Boot] [Patch v6 3/5] ARMv8/FSL_LSCH3: Add FSL_LSCH3 SoC

[York Sun]

On 06/18/2014 02:43 AM, Mark Rutland wrote:
> Hi York,
> 
> Apologies for the late reply on this.
> 
> I'm somewhat concerned regarding the issues you're seeing with cacheable
> page table walks, but I'll ignore that for the moment so we can get the
> ordering of maintenance sorted.
> 
> On Thu, Jun 05, 2014 at 08:23:09PM +0100, York Sun wrote:
>> Freescale LayerScape with Chassis Generation 3 is a set of SoCs with
>> ARMv8 cores and 3rd generation of Chassis. We use different MMU setup
>> to support memory map and cache attribute for these SoCs. MMU and cache
>> are enabled very early to bootst performance, especially for early
>> development on emulators. After u-boot relocates to DDR, a new MMU
>> table with QBMan cache access is created in DDR. SMMU pagesize is set
>> in SMMU_sACR register. Both DDR3 and DDR4 are supported.
>>
>> Signed-off-by: York Sun <yorksun at freescale.com>
>> Signed-off-by: Varun Sethi <Varun.Sethi at freescale.com>
>> Signed-off-by: Arnab Basu <arnab.basu at freescale.com>
>> ---
> 
> [...]
> 
>> +       /* point TTBR to the new table */
>> +       el = current_el();
> 
> Could we not place a dsb() here...
> 
>> +       if (el == 1) {
>> +               asm volatile("dsb sy;msr ttbr0_el1, %0;isb"
>> +                            : : "r" ((u64)level0_table) : "memory");
>> +       } else if (el == 2) {
>> +               asm volatile("dsb sy;msr ttbr0_el2, %0;isb"
>> +                            : : "r" ((u64)level0_table) : "memory");
>> +       } else if (el == 3) {
>> +               asm volatile("dsb sy;msr ttbr0_el3, %0;isb"
>> +                            : : "r" ((u64)level0_table) : "memory");
>> +       } else {
>> +               hang();
>> +       }
> 
> ...and an isb() here?
> 
> It would save duplicating them for each EL.

Sure. I can move them out.

> 
>> +       /*
>> +        * MMU is already enabled, just need to invalidate TLB to load the
>> +        * new table. The new table is compatible with the current table, if
>> +        * MMU somehow walks through the new table before invalidation TLB,
>> +        * it still works. So we don't need to turn off MMU here.
>> +        */
>> +}
>> +
>> +int arch_cpu_init(void)
>> +{
>> +       icache_enable();
> 
> Just to check: the icache is clean/invalid at this point?

It is not, but it is done first thing inside icache_enable().

> 
>> +       __asm_invalidate_dcache_all();
>> +       __asm_invalidate_tlb_all();
>> +       early_mmu_setup();
>> +       set_sctlr(get_sctlr() | CR_C);
>> +       return 0;
>> +}
> 
> [...]
> 
>> +/*
>> + * This function is called from lib/board.c.
>> + * It recreates MMU table in main memory. MMU and d-cache are enabled earlier.
>> + * There is no need to disable d-cache for this operation.
>> + */
>> +void enable_caches(void)
>> +{
>> +       final_mmu_setup();
>> +       flush_dcache_range(gd->arch.tlb_addr,
>> +                          gd->arch.tlb_addr +  gd->arch.tlb_size);
>> +       __asm_invalidate_tlb_all();
>> +}
> 
> This looks wrong, given your previous comments that you couldn't get the
> MMU to lookup from the cache.
> 
> In final_mmu_setup() you pointed the TTBR0_ELx to the new tables, but at
> that point the tables aren't guaranteed to be in memory, because they
> haven't been flushed. The MMU can start fetching the garbage from memory
> immediately, and things might go wrong before __asm_invalidate_tlb_all()
> blows away the garbage the MMU read from memory (for instance, the
> mapping covering the enable_caches function might get replaced by
> garbage).
> 
> If the page table walks are non-cacheable, you need to flush the tables
> to memory before programming the relevant TTBR.
> 

Agreed here. I will move the flushing before setting TTBR.

Thanks for review. v7 patch is coming after I verify all the changes.

York