Added 64-bit MMIO accessors for ARMv8

Make MMU functions reusable. Platform code can setup its own MMU tables.

Also fix a typo of "TCR_EL3_IPS_BITS" in cache_v8.c.

Signed-off-by York Sun <[email protected]>

CC David Feng <[email protected]>

---
Change log:
v4: new patch, splitted from v3 2/4
Revise set_pgtable_section() to be reused by platform MMU code
Add inline function set_ttbr_tcr_mair() to be used by this and platform mmu code

 arch/arm/cpu/armv8/cache_v8.c    |   49 ++++++++++++++++----------------------
 arch/arm/include/asm/armv8/mmu.h |   23 ++++++++++++++++++
 2 files changed, 43 insertions(+), 29 deletions(-)

arch/arm/cpu/armv8/cache_v8.c +20 -29

index a96ecda..67dbd46 100644
@@ -12,15 +12,14 @@
 DECLARE_GLOBAL_DATA_PTR;
 
 #ifndef CONFIG_SYS_DCACHE_OFF
-
-static void set_pgtable_section(u64 section, u64 memory_type)
+void set_pgtable_section(u64 *page_table, u64 index, u64 section,
+			 u64 memory_type)
 {
-	u64 *page_table = (u64 *)gd->arch.tlb_addr;
 	u64 value;
 
-	value = (section << SECTION_SHIFT) | PMD_TYPE_SECT | PMD_SECT_AF;
+	value = section | PMD_TYPE_SECT | PMD_SECT_AF;
 	value |= PMD_ATTRINDX(memory_type);
-	page_table[section] = value;
+	page_table[index] = value;
 }
 
 /* to activate the MMU we need to set up virtual memory */

@@ -28,10 +27,13 @@ static void mmu_setup(void)
 {
 	int i, j, el;
 	bd_t *bd = gd->bd;
+	u64 *page_table = (u64 *)gd->arch.tlb_addr;
 
 	/* Setup an identity-mapping for all spaces */
-	for (i = 0; i < (PGTABLE_SIZE >> 3); i++)
-		set_pgtable_section(i, MT_DEVICE_NGNRNE);
+	for (i = 0; i < (PGTABLE_SIZE >> 3); i++) {
+		set_pgtable_section(page_table, i, i << SECTION_SHIFT,
+				    MT_DEVICE_NGNRNE);
+	}
 
 	/* Setup an identity-mapping for all RAM space */
 	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {

@@ -39,36 +41,25 @@ static void mmu_setup(void)
 		ulong end = bd->bi_dram[i].start + bd->bi_dram[i].size;
 		for (j = start >> SECTION_SHIFT;
 		     j < end >> SECTION_SHIFT; j++) {
-			set_pgtable_section(j, MT_NORMAL);
+			set_pgtable_section(page_table, j, j << SECTION_SHIFT,
+					    MT_NORMAL);
 		}
 	}
 
 	/* load TTBR0 */
 	el = current_el();
 	if (el == 1) {
-		asm volatile("msr ttbr0_el1, %0"
-			     : : "r" (gd->arch.tlb_addr) : "memory");
-		asm volatile("msr tcr_el1, %0"
-			     : : "r" (TCR_FLAGS | TCR_EL1_IPS_BITS)
-			     : "memory");
-		asm volatile("msr mair_el1, %0"
-			     : : "r" (MEMORY_ATTRIBUTES) : "memory");
+		set_ttbr_tcr_mair(el, gd->arch.tlb_addr,
+				  TCR_FLAGS | TCR_EL1_IPS_BITS,
+				  MEMORY_ATTRIBUTES);
 	} else if (el == 2) {
-		asm volatile("msr ttbr0_el2, %0"
-			     : : "r" (gd->arch.tlb_addr) : "memory");
-		asm volatile("msr tcr_el2, %0"
-			     : : "r" (TCR_FLAGS | TCR_EL2_IPS_BITS)
-			     : "memory");
-		asm volatile("msr mair_el2, %0"
-			     : : "r" (MEMORY_ATTRIBUTES) : "memory");
+		set_ttbr_tcr_mair(el, gd->arch.tlb_addr,
+				  TCR_FLAGS | TCR_EL2_IPS_BITS,
+				  MEMORY_ATTRIBUTES);
 	} else {
-		asm volatile("msr ttbr0_el3, %0"
-			     : : "r" (gd->arch.tlb_addr) : "memory");
-		asm volatile("msr tcr_el3, %0"
-			     : : "r" (TCR_FLAGS | TCR_EL2_IPS_BITS)
-			     : "memory");
-		asm volatile("msr mair_el3, %0"
-			     : : "r" (MEMORY_ATTRIBUTES) : "memory");
+		set_ttbr_tcr_mair(el, gd->arch.tlb_addr,
+				  TCR_FLAGS | TCR_EL3_IPS_BITS,
+				  MEMORY_ATTRIBUTES);
 	}
 
 	/* enable the mmu */

arch/arm/include/asm/armv8/mmu.h +0 -0

index 1193e76..7de4ff9 100644

Quoted 1 line · York Sun

Make MMU functions reusable. Platform code can setup its own MMU tables.

What exactly does platform code need to setup its own tables for?

Quoted 111 lines

Also fix a typo of "TCR_EL3_IPS_BITS" in cache_v8.c.

Signed-off-by York Sun <[email protected]>

CC David Feng <[email protected]>

---
Change log:
v4: new patch, splitted from v3 2/4
Revise set_pgtable_section() to be reused by platform MMU code
Add inline function set_ttbr_tcr_mair() to be used by this and platform mmu code

 arch/arm/cpu/armv8/cache_v8.c    |   49 ++++++++++++++++----------------------
 arch/arm/include/asm/armv8/mmu.h |   23 ++++++++++++++++++
 2 files changed, 43 insertions(+), 29 deletions(-)

arch/arm/cpu/armv8/cache_v8.c +20 -29

index a96ecda..67dbd46 100644
@@ -12,15 +12,14 @@
 DECLARE_GLOBAL_DATA_PTR;
 
 #ifndef CONFIG_SYS_DCACHE_OFF
-
-static void set_pgtable_section(u64 section, u64 memory_type)
+void set_pgtable_section(u64 *page_table, u64 index, u64 section,
+			 u64 memory_type)
 {
-	u64 *page_table = (u64 *)gd->arch.tlb_addr;
 	u64 value;
 
-	value = (section << SECTION_SHIFT) | PMD_TYPE_SECT | PMD_SECT_AF;
+	value = section | PMD_TYPE_SECT | PMD_SECT_AF;
 	value |= PMD_ATTRINDX(memory_type);
-	page_table[section] = value;
+	page_table[index] = value;
 }
 
 /* to activate the MMU we need to set up virtual memory */

@@ -28,10 +27,13 @@ static void mmu_setup(void)
 {
 	int i, j, el;
 	bd_t *bd = gd->bd;
+	u64 *page_table = (u64 *)gd->arch.tlb_addr;
 
 	/* Setup an identity-mapping for all spaces */
-	for (i = 0; i < (PGTABLE_SIZE >> 3); i++)
-		set_pgtable_section(i, MT_DEVICE_NGNRNE);
+	for (i = 0; i < (PGTABLE_SIZE >> 3); i++) {
+		set_pgtable_section(page_table, i, i << SECTION_SHIFT,
+				    MT_DEVICE_NGNRNE);
+	}
 
 	/* Setup an identity-mapping for all RAM space */
 	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {

@@ -39,36 +41,25 @@ static void mmu_setup(void)
 		ulong end = bd->bi_dram[i].start + bd->bi_dram[i].size;
 		for (j = start >> SECTION_SHIFT;
 		     j < end >> SECTION_SHIFT; j++) {
-			set_pgtable_section(j, MT_NORMAL);
+			set_pgtable_section(page_table, j, j << SECTION_SHIFT,
+					    MT_NORMAL);
 		}
 	}
 
 	/* load TTBR0 */
 	el = current_el();
 	if (el == 1) {
-		asm volatile("msr ttbr0_el1, %0"
-			     : : "r" (gd->arch.tlb_addr) : "memory");
-		asm volatile("msr tcr_el1, %0"
-			     : : "r" (TCR_FLAGS | TCR_EL1_IPS_BITS)
-			     : "memory");
-		asm volatile("msr mair_el1, %0"
-			     : : "r" (MEMORY_ATTRIBUTES) : "memory");
+		set_ttbr_tcr_mair(el, gd->arch.tlb_addr,
+				  TCR_FLAGS | TCR_EL1_IPS_BITS,
+				  MEMORY_ATTRIBUTES);
 	} else if (el == 2) {
-		asm volatile("msr ttbr0_el2, %0"
-			     : : "r" (gd->arch.tlb_addr) : "memory");
-		asm volatile("msr tcr_el2, %0"
-			     : : "r" (TCR_FLAGS | TCR_EL2_IPS_BITS)
-			     : "memory");
-		asm volatile("msr mair_el2, %0"
-			     : : "r" (MEMORY_ATTRIBUTES) : "memory");
+		set_ttbr_tcr_mair(el, gd->arch.tlb_addr,
+				  TCR_FLAGS | TCR_EL2_IPS_BITS,
+				  MEMORY_ATTRIBUTES);
 	} else {
-		asm volatile("msr ttbr0_el3, %0"
-			     : : "r" (gd->arch.tlb_addr) : "memory");
-		asm volatile("msr tcr_el3, %0"
-			     : : "r" (TCR_FLAGS | TCR_EL2_IPS_BITS)
-			     : "memory");
-		asm volatile("msr mair_el3, %0"
-			     : : "r" (MEMORY_ATTRIBUTES) : "memory");
+		set_ttbr_tcr_mair(el, gd->arch.tlb_addr,
+				  TCR_FLAGS | TCR_EL3_IPS_BITS,
+				  MEMORY_ATTRIBUTES);
 	}
 
 	/* enable the mmu */

arch/arm/include/asm/armv8/mmu.h +0 -0

index 1193e76..7de4ff9 100644
@@ -108,4 +108,27 @@
 				TCR_IRGN_WBWA |		\
 				TCR_T0SZ(VA_BITS))
 

arch/arm/include/asm/armv8/mmu.h line 116

Quoted 6 lines

arch/arm/include/asm/armv8/mmu.h +6 -0

+#ifndef __ASSEMBLY__
+void set_pgtable_section(u64 *page_table, u64 index,
+			 u64 section, u64 memory_type);
+static inline void set_ttbr_tcr_mair(int el, u64 table, u64 tcr, u64 attr)
+{
+	asm volatile("dsb sy;isb");

Huh? This wasn't anywhere before. Is the isb necessary?

When is this function expected to be called? Is the MMU expected to be on?

arch/arm/include/asm/armv8/mmu.h line 120

Quoted 5 lines

arch/arm/include/asm/armv8/mmu.h +4 -0

+	if (el == 1) {
+		asm volatile("msr ttbr0_el1, %0" : : "r" (table) : "memory");
+		asm volatile("msr tcr_el1, %0" : : "r" (tcr) : "memory");
+		asm volatile("msr mair_el1, %0" : : "r" (attr) : "memory");

If the MMU is on, this looks really scary -- what are you expecting to change in a single invocation?

arch/arm/include/asm/armv8/mmu.h line 131

Quoted 12 lines

arch/arm/include/asm/armv8/mmu.h +11 -0

+	} else if (el == 2) {
+		asm volatile("msr ttbr0_el2, %0" : : "r" (table) : "memory");
+		asm volatile("msr tcr_el2, %0" : : "r" (tcr) : "memory");
+		asm volatile("msr mair_el2, %0" : : "r" (attr) : "memory");
+	} else if (el == 3) {
+		asm volatile("msr ttbr0_el3, %0" : : "r" (table) : "memory");
+		asm volatile("msr tcr_el3, %0" : : "r" (tcr) : "memory");
+		asm volatile("msr mair_el3, %0" : : "r" (attr) : "memory");
+	} else {
+		hang();
+	}

And no synchronisation to ensure that these writes are complete or even ordered w.r.t. each other?

Quoted 4 lines · Mark Rutland

Quoted 1 line · York Sun

Make MMU functions reusable. Platform code can setup its own MMU tables.

What exactly does platform code need to setup its own tables for?

The general ARMv8 MMU table is not detail enough to control memory attribute like cache for all addresses. We have devices mapping to addresses with different requirement for cache control.

Quoted 112 lines

Quoted 112 lines

Also fix a typo of "TCR_EL3_IPS_BITS" in cache_v8.c.

Signed-off-by York Sun <[email protected]>

CC David Feng <[email protected]>

---
Change log:
v4: new patch, splitted from v3 2/4
Revise set_pgtable_section() to be reused by platform MMU code
Add inline function set_ttbr_tcr_mair() to be used by this and platform mmu code

 arch/arm/cpu/armv8/cache_v8.c    |   49 ++++++++++++++++----------------------
 arch/arm/include/asm/armv8/mmu.h |   23 ++++++++++++++++++
 2 files changed, 43 insertions(+), 29 deletions(-)

arch/arm/cpu/armv8/cache_v8.c +20 -29

index a96ecda..67dbd46 100644
@@ -12,15 +12,14 @@
 DECLARE_GLOBAL_DATA_PTR;
 
 #ifndef CONFIG_SYS_DCACHE_OFF
-
-static void set_pgtable_section(u64 section, u64 memory_type)
+void set_pgtable_section(u64 *page_table, u64 index, u64 section,
+			 u64 memory_type)
 {
-	u64 *page_table = (u64 *)gd->arch.tlb_addr;
 	u64 value;
 
-	value = (section << SECTION_SHIFT) | PMD_TYPE_SECT | PMD_SECT_AF;
+	value = section | PMD_TYPE_SECT | PMD_SECT_AF;
 	value |= PMD_ATTRINDX(memory_type);
-	page_table[section] = value;
+	page_table[index] = value;
 }
 
 /* to activate the MMU we need to set up virtual memory */

@@ -28,10 +27,13 @@ static void mmu_setup(void)
 {
 	int i, j, el;
 	bd_t *bd = gd->bd;
+	u64 *page_table = (u64 *)gd->arch.tlb_addr;
 
 	/* Setup an identity-mapping for all spaces */
-	for (i = 0; i < (PGTABLE_SIZE >> 3); i++)
-		set_pgtable_section(i, MT_DEVICE_NGNRNE);
+	for (i = 0; i < (PGTABLE_SIZE >> 3); i++) {
+		set_pgtable_section(page_table, i, i << SECTION_SHIFT,
+				    MT_DEVICE_NGNRNE);
+	}
 
 	/* Setup an identity-mapping for all RAM space */
 	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {

@@ -39,36 +41,25 @@ static void mmu_setup(void)
 		ulong end = bd->bi_dram[i].start + bd->bi_dram[i].size;
 		for (j = start >> SECTION_SHIFT;
 		     j < end >> SECTION_SHIFT; j++) {
-			set_pgtable_section(j, MT_NORMAL);
+			set_pgtable_section(page_table, j, j << SECTION_SHIFT,
+					    MT_NORMAL);
 		}
 	}
 
 	/* load TTBR0 */
 	el = current_el();
 	if (el == 1) {
-		asm volatile("msr ttbr0_el1, %0"
-			     : : "r" (gd->arch.tlb_addr) : "memory");
-		asm volatile("msr tcr_el1, %0"
-			     : : "r" (TCR_FLAGS | TCR_EL1_IPS_BITS)
-			     : "memory");
-		asm volatile("msr mair_el1, %0"
-			     : : "r" (MEMORY_ATTRIBUTES) : "memory");
+		set_ttbr_tcr_mair(el, gd->arch.tlb_addr,
+				  TCR_FLAGS | TCR_EL1_IPS_BITS,
+				  MEMORY_ATTRIBUTES);
 	} else if (el == 2) {
-		asm volatile("msr ttbr0_el2, %0"
-			     : : "r" (gd->arch.tlb_addr) : "memory");
-		asm volatile("msr tcr_el2, %0"
-			     : : "r" (TCR_FLAGS | TCR_EL2_IPS_BITS)
-			     : "memory");
-		asm volatile("msr mair_el2, %0"
-			     : : "r" (MEMORY_ATTRIBUTES) : "memory");
+		set_ttbr_tcr_mair(el, gd->arch.tlb_addr,
+				  TCR_FLAGS | TCR_EL2_IPS_BITS,
+				  MEMORY_ATTRIBUTES);
 	} else {
-		asm volatile("msr ttbr0_el3, %0"
-			     : : "r" (gd->arch.tlb_addr) : "memory");
-		asm volatile("msr tcr_el3, %0"
-			     : : "r" (TCR_FLAGS | TCR_EL2_IPS_BITS)
-			     : "memory");
-		asm volatile("msr mair_el3, %0"
-			     : : "r" (MEMORY_ATTRIBUTES) : "memory");
+		set_ttbr_tcr_mair(el, gd->arch.tlb_addr,
+				  TCR_FLAGS | TCR_EL3_IPS_BITS,
+				  MEMORY_ATTRIBUTES);
 	}
 
 	/* enable the mmu */

arch/arm/include/asm/armv8/mmu.h +0 -0

index 1193e76..7de4ff9 100644
@@ -108,4 +108,27 @@
 				TCR_IRGN_WBWA |		\
 				TCR_T0SZ(VA_BITS))
 

Quoted 8 lines

arch/arm/include/asm/armv8/mmu.h line 116

Quoted 6 lines

arch/arm/include/asm/armv8/mmu.h +6 -0

+#ifndef __ASSEMBLY__
+void set_pgtable_section(u64 *page_table, u64 index,
+			 u64 section, u64 memory_type);
+static inline void set_ttbr_tcr_mair(int el, u64 table, u64 tcr, u64 attr)
+{
+	asm volatile("dsb sy;isb");

Huh? This wasn't anywhere before. Is the isb necessary?

Probably not, but to make sure all previous instructions finish.

Quoted 3 lines

When is this function expected to be called? Is the MMU expected to be on?

The general ARMv8 code calls this when MMU is off. For the SoC I am enabling, it is called when MMU is off for the first time, but MMU on for the 2nd time.

Quoted 9 lines

arch/arm/include/asm/armv8/mmu.h line 120

Quoted 6 lines

arch/arm/include/asm/armv8/mmu.h +4 -0

+	if (el == 1) {
+		asm volatile("msr ttbr0_el1, %0" : : "r" (table) : "memory");
+		asm volatile("msr tcr_el1, %0" : : "r" (tcr) : "memory");
+		asm volatile("msr mair_el1, %0" : : "r" (attr) : "memory");

If the MMU is on, this looks really scary -- what are you expecting to change in a single invocation?

It is not scary for general ARMv8 code. MMU is off then this is called. For FSL SoC, I have these called for the 2nd time, when MMU is on. The 2nd time call points to a new MMU table.

Quoted 17 lines

arch/arm/include/asm/armv8/mmu.h line 131

Quoted 13 lines

arch/arm/include/asm/armv8/mmu.h +11 -0

+	} else if (el == 2) {
+		asm volatile("msr ttbr0_el2, %0" : : "r" (table) : "memory");
+		asm volatile("msr tcr_el2, %0" : : "r" (tcr) : "memory");
+		asm volatile("msr mair_el2, %0" : : "r" (attr) : "memory");
+	} else if (el == 3) {
+		asm volatile("msr ttbr0_el3, %0" : : "r" (table) : "memory");
+		asm volatile("msr tcr_el3, %0" : : "r" (tcr) : "memory");
+		asm volatile("msr mair_el3, %0" : : "r" (attr) : "memory");
+	} else {
+		hang();
+	}

And no synchronisation to ensure that these writes are complete or even ordered w.r.t. each other?

That's why I added asm volatile("dsb sy;isb") before them. The order of these write doesn't matter. See the code before my change
http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/cpu/armv8/cache_v8.c;h=a96ecda7e3146996d20a2e46b975dced2769255c;hb=HEAD

For the 2nd write used by FSL SoC, I need to flush the cache to make sure the table is in main DDR and perform TLB invalidation. That's when the loading of new MMU table happens.

Quoted 6 lines · York Sun

Quoted 4 lines · Mark Rutland

Quoted 1 line · York Sun

Make MMU functions reusable. Platform code can setup its own MMU tables.

What exactly does platform code need to setup its own tables for?

Quoted 3 lines

The general ARMv8 MMU table is not detail enough to control memory attribute like cache for all addresses. We have devices mapping to addresses with different requirement for cache control.

And there are no APIs for creating device mappings rather than exporting the raw pagetable accessors and hard-coding them differently in every board file?

Quoted 121 lines

Quoted 120 lines

Quoted 109 lines

Also fix a typo of "TCR_EL3_IPS_BITS" in cache_v8.c.

Signed-off-by York Sun <[email protected]>

CC David Feng <[email protected]>

---
Change log:
v4: new patch, splitted from v3 2/4
Revise set_pgtable_section() to be reused by platform MMU code
Add inline function set_ttbr_tcr_mair() to be used by this and platform mmu code

 arch/arm/cpu/armv8/cache_v8.c    |   49 ++++++++++++++++----------------------
 arch/arm/include/asm/armv8/mmu.h |   23 ++++++++++++++++++
 2 files changed, 43 insertions(+), 29 deletions(-)

arch/arm/cpu/armv8/cache_v8.c +20 -29

index a96ecda..67dbd46 100644
@@ -12,15 +12,14 @@
 DECLARE_GLOBAL_DATA_PTR;
 
 #ifndef CONFIG_SYS_DCACHE_OFF
-
-static void set_pgtable_section(u64 section, u64 memory_type)
+void set_pgtable_section(u64 *page_table, u64 index, u64 section,
+			 u64 memory_type)
 {
-	u64 *page_table = (u64 *)gd->arch.tlb_addr;
 	u64 value;
 
-	value = (section << SECTION_SHIFT) | PMD_TYPE_SECT | PMD_SECT_AF;
+	value = section | PMD_TYPE_SECT | PMD_SECT_AF;
 	value |= PMD_ATTRINDX(memory_type);
-	page_table[section] = value;
+	page_table[index] = value;
 }
 
 /* to activate the MMU we need to set up virtual memory */

@@ -28,10 +27,13 @@ static void mmu_setup(void)
 {
 	int i, j, el;
 	bd_t *bd = gd->bd;
+	u64 *page_table = (u64 *)gd->arch.tlb_addr;
 
 	/* Setup an identity-mapping for all spaces */
-	for (i = 0; i < (PGTABLE_SIZE >> 3); i++)
-		set_pgtable_section(i, MT_DEVICE_NGNRNE);
+	for (i = 0; i < (PGTABLE_SIZE >> 3); i++) {
+		set_pgtable_section(page_table, i, i << SECTION_SHIFT,
+				    MT_DEVICE_NGNRNE);
+	}
 
 	/* Setup an identity-mapping for all RAM space */
 	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {

@@ -39,36 +41,25 @@ static void mmu_setup(void)
 		ulong end = bd->bi_dram[i].start + bd->bi_dram[i].size;
 		for (j = start >> SECTION_SHIFT;
 		     j < end >> SECTION_SHIFT; j++) {
-			set_pgtable_section(j, MT_NORMAL);
+			set_pgtable_section(page_table, j, j << SECTION_SHIFT,
+					    MT_NORMAL);
 		}
 	}
 
 	/* load TTBR0 */
 	el = current_el();
 	if (el == 1) {
-		asm volatile("msr ttbr0_el1, %0"
-			     : : "r" (gd->arch.tlb_addr) : "memory");
-		asm volatile("msr tcr_el1, %0"
-			     : : "r" (TCR_FLAGS | TCR_EL1_IPS_BITS)
-			     : "memory");
-		asm volatile("msr mair_el1, %0"
-			     : : "r" (MEMORY_ATTRIBUTES) : "memory");
+		set_ttbr_tcr_mair(el, gd->arch.tlb_addr,
+				  TCR_FLAGS | TCR_EL1_IPS_BITS,
+				  MEMORY_ATTRIBUTES);
 	} else if (el == 2) {
-		asm volatile("msr ttbr0_el2, %0"
-			     : : "r" (gd->arch.tlb_addr) : "memory");
-		asm volatile("msr tcr_el2, %0"
-			     : : "r" (TCR_FLAGS | TCR_EL2_IPS_BITS)
-			     : "memory");
-		asm volatile("msr mair_el2, %0"
-			     : : "r" (MEMORY_ATTRIBUTES) : "memory");
+		set_ttbr_tcr_mair(el, gd->arch.tlb_addr,
+				  TCR_FLAGS | TCR_EL2_IPS_BITS,
+				  MEMORY_ATTRIBUTES);
 	} else {
-		asm volatile("msr ttbr0_el3, %0"
-			     : : "r" (gd->arch.tlb_addr) : "memory");
-		asm volatile("msr tcr_el3, %0"
-			     : : "r" (TCR_FLAGS | TCR_EL2_IPS_BITS)
-			     : "memory");
-		asm volatile("msr mair_el3, %0"
-			     : : "r" (MEMORY_ATTRIBUTES) : "memory");
+		set_ttbr_tcr_mair(el, gd->arch.tlb_addr,
+				  TCR_FLAGS | TCR_EL3_IPS_BITS,
+				  MEMORY_ATTRIBUTES);
 	}
 
 	/* enable the mmu */

arch/arm/include/asm/armv8/mmu.h +0 -0

index 1193e76..7de4ff9 100644

arch/arm/include/asm/armv8/mmu.h line 116

Quoted 9 lines

arch/arm/include/asm/armv8/mmu.h +6 -0

@@ -108,4 +108,27 @@
 				TCR_IRGN_WBWA |		\
 				TCR_T0SZ(VA_BITS))
 
+#ifndef __ASSEMBLY__
+void set_pgtable_section(u64 *page_table, u64 index,
+			 u64 section, u64 memory_type);
+static inline void set_ttbr_tcr_mair(int el, u64 table, u64 tcr, u64 attr)
+{
+	asm volatile("dsb sy;isb");

Huh? This wasn't anywhere before. Is the isb necessary?

Quoted 1 line

Probably not, but to make sure all previous instructions finish.

Which instructions do you care about completing?

You'll certainly want any page table writes to complete, but is anything else in-flight at this point in time?

Quoted 7 lines

Quoted 4 lines

When is this function expected to be called? Is the MMU expected to be on?

The general ARMv8 code calls this when MMU is off. For the SoC I am enabling, it is called when MMU is off for the first time, but MMU on for the 2nd time.

Ok.

Quoted 10 lines

Quoted 9 lines

arch/arm/include/asm/armv8/mmu.h line 120

Quoted 6 lines

arch/arm/include/asm/armv8/mmu.h +4 -0

+	if (el == 1) {
+		asm volatile("msr ttbr0_el1, %0" : : "r" (table) : "memory");
+		asm volatile("msr tcr_el1, %0" : : "r" (tcr) : "memory");
+		asm volatile("msr mair_el1, %0" : : "r" (attr) : "memory");

If the MMU is on, this looks really scary -- what are you expecting to change in a single invocation?

Quoted 3 lines

It is not scary for general ARMv8 code. MMU is off then this is called. For FSL SoC, I have these called for the 2nd time, when MMU is on. The 2nd time call points to a new MMU table.

Oh but it is ;)

Quoted 18 lines

Quoted 17 lines

arch/arm/include/asm/armv8/mmu.h line 131

Quoted 13 lines

arch/arm/include/asm/armv8/mmu.h +11 -0

+	} else if (el == 2) {
+		asm volatile("msr ttbr0_el2, %0" : : "r" (table) : "memory");
+		asm volatile("msr tcr_el2, %0" : : "r" (tcr) : "memory");
+		asm volatile("msr mair_el2, %0" : : "r" (attr) : "memory");
+	} else if (el == 3) {
+		asm volatile("msr ttbr0_el3, %0" : : "r" (table) : "memory");
+		asm volatile("msr tcr_el3, %0" : : "r" (tcr) : "memory");
+		asm volatile("msr mair_el3, %0" : : "r" (attr) : "memory");
+	} else {
+		hang();
+	}

And no synchronisation to ensure that these writes are complete or even ordered w.r.t. each other?

Quoted 3 lines

That's why I added asm volatile("dsb sy;isb") before them. The order of these write doesn't matter. See the code before my change
http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/cpu/armv8/cache_v8.c;h=a96ecda7e3146996d20a2e46b975dced2769255c;hb=HEAD

Ok, so that orders them against everything _before_ them, but not with respect to each other, or anything _after_ them.

When the MMU is off, you are correct that the ordering of these operations w.r.t. each other doesn't matter. With the MMU on that's not true as the CPU can rerder the operations and can walk the page tables asynchronously.

Changing the MAIR at runtime is somewhat scary because you may be changing attributes for entries which are active in the cache and/or TLBs. I'm not sure I follow why you need to change it once the MMU is on -- there are plenty of subfields in the MAIR that you could configure before turning the MMU on for use later.

Likewise changing the TCR at runtime is somewhat scary because you can change attributes of active cache and/or TLB entries, change fields that affect the way page tables are walked (TG*. T*SZ), all asynchronously w.r.t. the logic that walsk the page tables.

Changing the TTBR is fine, except that we didn't invalidate old entries with a TLBI, so we may even have partial translations cahced in the TLBs, and we can get odd translations generated from the combination of the old and new tables.

Quoted 4 lines

For the 2nd write used by FSL SoC, I need to flush the cache to make sure the table is in main DDR and perform TLB invalidation. That's when the loading of new MMU table happens.

I missed the TLB invaliation -- where is that meant to happen?

Quoted 7 lines · Mark Rutland

Quoted 6 lines · York Sun

Quoted 4 lines · Mark Rutland

Quoted 1 line · York Sun

Make MMU functions reusable. Platform code can setup its own MMU tables.

What exactly does platform code need to setup its own tables for?

Quoted 8 lines

Quoted 3 lines

The general ARMv8 MMU table is not detail enough to control memory attribute like cache for all addresses. We have devices mapping to addresses with different requirement for cache control.

And there are no APIs for creating device mappings rather than exporting the raw pagetable accessors and hard-coding them differently in every board file?

That's a good question. At this point, only two platforms are using ARMv8 code. I am expecting FSL ARMv8 implementation will stay similar, i.e. covered by the file I added. If that's not the case, or more ARMv8 SoCs need special MMU table, we then should introduce such API. Having a full function MMU API may be an overkill for U-boot. We don't need dynamic MMU anyway.

Quoted 124 lines

Quoted 123 lines

Quoted 113 lines

Quoted 113 lines

Also fix a typo of "TCR_EL3_IPS_BITS" in cache_v8.c.

Signed-off-by York Sun <[email protected]>

CC David Feng <[email protected]>

---
Change log:
v4: new patch, splitted from v3 2/4
Revise set_pgtable_section() to be reused by platform MMU code
Add inline function set_ttbr_tcr_mair() to be used by this and platform mmu code

 arch/arm/cpu/armv8/cache_v8.c    |   49 ++++++++++++++++----------------------
 arch/arm/include/asm/armv8/mmu.h |   23 ++++++++++++++++++
 2 files changed, 43 insertions(+), 29 deletions(-)

arch/arm/cpu/armv8/cache_v8.c +20 -29

index a96ecda..67dbd46 100644
@@ -12,15 +12,14 @@
 DECLARE_GLOBAL_DATA_PTR;
 
 #ifndef CONFIG_SYS_DCACHE_OFF
-
-static void set_pgtable_section(u64 section, u64 memory_type)
+void set_pgtable_section(u64 *page_table, u64 index, u64 section,
+			 u64 memory_type)
 {
-	u64 *page_table = (u64 *)gd->arch.tlb_addr;
 	u64 value;
 
-	value = (section << SECTION_SHIFT) | PMD_TYPE_SECT | PMD_SECT_AF;
+	value = section | PMD_TYPE_SECT | PMD_SECT_AF;
 	value |= PMD_ATTRINDX(memory_type);
-	page_table[section] = value;
+	page_table[index] = value;
 }
 
 /* to activate the MMU we need to set up virtual memory */

@@ -28,10 +27,13 @@ static void mmu_setup(void)
 {
 	int i, j, el;
 	bd_t *bd = gd->bd;
+	u64 *page_table = (u64 *)gd->arch.tlb_addr;
 
 	/* Setup an identity-mapping for all spaces */
-	for (i = 0; i < (PGTABLE_SIZE >> 3); i++)
-		set_pgtable_section(i, MT_DEVICE_NGNRNE);
+	for (i = 0; i < (PGTABLE_SIZE >> 3); i++) {
+		set_pgtable_section(page_table, i, i << SECTION_SHIFT,
+				    MT_DEVICE_NGNRNE);
+	}
 
 	/* Setup an identity-mapping for all RAM space */
 	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {

@@ -39,36 +41,25 @@ static void mmu_setup(void)
 		ulong end = bd->bi_dram[i].start + bd->bi_dram[i].size;
 		for (j = start >> SECTION_SHIFT;
 		     j < end >> SECTION_SHIFT; j++) {
-			set_pgtable_section(j, MT_NORMAL);
+			set_pgtable_section(page_table, j, j << SECTION_SHIFT,
+					    MT_NORMAL);
 		}
 	}
 
 	/* load TTBR0 */
 	el = current_el();
 	if (el == 1) {
-		asm volatile("msr ttbr0_el1, %0"
-			     : : "r" (gd->arch.tlb_addr) : "memory");
-		asm volatile("msr tcr_el1, %0"
-			     : : "r" (TCR_FLAGS | TCR_EL1_IPS_BITS)
-			     : "memory");
-		asm volatile("msr mair_el1, %0"
-			     : : "r" (MEMORY_ATTRIBUTES) : "memory");
+		set_ttbr_tcr_mair(el, gd->arch.tlb_addr,
+				  TCR_FLAGS | TCR_EL1_IPS_BITS,
+				  MEMORY_ATTRIBUTES);
 	} else if (el == 2) {
-		asm volatile("msr ttbr0_el2, %0"
-			     : : "r" (gd->arch.tlb_addr) : "memory");
-		asm volatile("msr tcr_el2, %0"
-			     : : "r" (TCR_FLAGS | TCR_EL2_IPS_BITS)
-			     : "memory");
-		asm volatile("msr mair_el2, %0"
-			     : : "r" (MEMORY_ATTRIBUTES) : "memory");
+		set_ttbr_tcr_mair(el, gd->arch.tlb_addr,
+				  TCR_FLAGS | TCR_EL2_IPS_BITS,
+				  MEMORY_ATTRIBUTES);
 	} else {
-		asm volatile("msr ttbr0_el3, %0"
-			     : : "r" (gd->arch.tlb_addr) : "memory");
-		asm volatile("msr tcr_el3, %0"
-			     : : "r" (TCR_FLAGS | TCR_EL2_IPS_BITS)
-			     : "memory");
-		asm volatile("msr mair_el3, %0"
-			     : : "r" (MEMORY_ATTRIBUTES) : "memory");
+		set_ttbr_tcr_mair(el, gd->arch.tlb_addr,
+				  TCR_FLAGS | TCR_EL3_IPS_BITS,
+				  MEMORY_ATTRIBUTES);
 	}
 
 	/* enable the mmu */

arch/arm/include/asm/armv8/mmu.h +0 -0

index 1193e76..7de4ff9 100644
@@ -108,4 +108,27 @@
 				TCR_IRGN_WBWA |		\
 				TCR_T0SZ(VA_BITS))
 

Quoted 8 lines

arch/arm/include/asm/armv8/mmu.h line 116

Quoted 6 lines

arch/arm/include/asm/armv8/mmu.h +6 -0

+#ifndef __ASSEMBLY__
+void set_pgtable_section(u64 *page_table, u64 index,
+			 u64 section, u64 memory_type);
+static inline void set_ttbr_tcr_mair(int el, u64 table, u64 tcr, u64 attr)
+{
+	asm volatile("dsb sy;isb");

Huh? This wasn't anywhere before. Is the isb necessary?

Probably not, but to make sure all previous instructions finish.

Quoted 4 lines

Which instructions do you care about completing?

You'll certainly want any page table writes to complete, but is anything else in-flight at this point in time?

Before calling this inline function, MMU tables were created. We want the writing completes before setting these registers.

Quoted 67 lines

Quoted 7 lines

Quoted 4 lines

When is this function expected to be called? Is the MMU expected to be on?

The general ARMv8 code calls this when MMU is off. For the SoC I am enabling, it is called when MMU is off for the first time, but MMU on for the 2nd time.

Ok.

Quoted 10 lines

Quoted 9 lines

arch/arm/include/asm/armv8/mmu.h line 120

Quoted 6 lines

arch/arm/include/asm/armv8/mmu.h +4 -0

+	if (el == 1) {
+		asm volatile("msr ttbr0_el1, %0" : : "r" (table) : "memory");
+		asm volatile("msr tcr_el1, %0" : : "r" (tcr) : "memory");
+		asm volatile("msr mair_el1, %0" : : "r" (attr) : "memory");

If the MMU is on, this looks really scary -- what are you expecting to change in a single invocation?

Quoted 3 lines

It is not scary for general ARMv8 code. MMU is off then this is called. For FSL SoC, I have these called for the 2nd time, when MMU is on. The 2nd time call points to a new MMU table.

Oh but it is ;)

Quoted 18 lines

Quoted 17 lines

arch/arm/include/asm/armv8/mmu.h line 131

Quoted 13 lines

arch/arm/include/asm/armv8/mmu.h +11 -0

+	} else if (el == 2) {
+		asm volatile("msr ttbr0_el2, %0" : : "r" (table) : "memory");
+		asm volatile("msr tcr_el2, %0" : : "r" (tcr) : "memory");
+		asm volatile("msr mair_el2, %0" : : "r" (attr) : "memory");
+	} else if (el == 3) {
+		asm volatile("msr ttbr0_el3, %0" : : "r" (table) : "memory");
+		asm volatile("msr tcr_el3, %0" : : "r" (tcr) : "memory");
+		asm volatile("msr mair_el3, %0" : : "r" (attr) : "memory");
+	} else {
+		hang();
+	}

And no synchronisation to ensure that these writes are complete or even ordered w.r.t. each other?

Quoted 3 lines

That's why I added asm volatile("dsb sy;isb") before them. The order of these write doesn't matter. See the code before my change
http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/cpu/armv8/cache_v8.c;h=a96ecda7e3146996d20a2e46b975dced2769255c;hb=HEAD

Ok, so that orders them against everything _before_ them, but not with respect to each other, or anything _after_ them.

When the MMU is off, you are correct that the ordering of these operations w.r.t. each other doesn't matter. With the MMU on that's not true as the CPU can rerder the operations and can walk the page tables asynchronously.

Changing the MAIR at runtime is somewhat scary because you may be changing attributes for entries which are active in the cache and/or TLBs. I'm not sure I follow why you need to change it once the MMU is on -- there are plenty of subfields in the MAIR that you could configure before turning the MMU on for use later.

Likewise changing the TCR at runtime is somewhat scary because you can change attributes of active cache and/or TLB entries, change fields that affect the way page tables are walked (TG*. T*SZ), all asynchronously w.r.t. the logic that walsk the page tables.

Quoted 4 lines

Changing the TTBR is fine, except that we didn't invalidate old entries with a TLBI, so we may even have partial translations cahced in the TLBs, and we can get odd translations generated from the combination of the old and new tables.

Maybe a more proper way to do so is to change TTBR only. That's actually what I really need. Or if I really need to change all of them, I should turn off MMU first.

Quoted 6 lines

Quoted 3 lines

For the 2nd write used by FSL SoC, I need to flush the cache to make sure the table is in main DDR and perform TLB invalidation. That's when the loading of new MMU table happens.

I missed the TLB invaliation -- where is that meant to happen?

After flushing the new MMU table into DDR. To your point above, I will send a new version with updating TTBR only.

Quoted 17 lines · York Sun

Quoted 7 lines · Mark Rutland

Quoted 6 lines · York Sun

Quoted 4 lines · Mark Rutland

Quoted 1 line · York Sun

Make MMU functions reusable. Platform code can setup its own MMU tables.

What exactly does platform code need to setup its own tables for?

Quoted 8 lines

Quoted 3 lines

The general ARMv8 MMU table is not detail enough to control memory attribute like cache for all addresses. We have devices mapping to addresses with different requirement for cache control.

And there are no APIs for creating device mappings rather than exporting the raw pagetable accessors and hard-coding them differently in every board file?

Quoted 5 lines

That's a good question. At this point, only two platforms are using ARMv8 code. I am expecting FSL ARMv8 implementation will stay similar, i.e. covered by the file I added. If that's not the case, or more ARMv8 SoCs need special MMU table, we then should introduce such API. Having a full function MMU API may be an overkill for U-boot. We don't need dynamic MMU anyway.

Maybe. It just seems to me that it would be possible to pre-allocate an empty table that we could place device (nGnRnE?) mappings in. Then all you'd need to call from board code is a function to map a range, rather than having to duplicate logic for creating the tables you want.

Quoted 125 lines

Quoted 125 lines

Quoted 124 lines

Quoted 114 lines

Quoted 114 lines

Also fix a typo of "TCR_EL3_IPS_BITS" in cache_v8.c.

Signed-off-by York Sun <[email protected]>

CC David Feng <[email protected]>

---
Change log:
v4: new patch, splitted from v3 2/4
Revise set_pgtable_section() to be reused by platform MMU code
Add inline function set_ttbr_tcr_mair() to be used by this and platform mmu code

 arch/arm/cpu/armv8/cache_v8.c    |   49 ++++++++++++++++----------------------
 arch/arm/include/asm/armv8/mmu.h |   23 ++++++++++++++++++
 2 files changed, 43 insertions(+), 29 deletions(-)

arch/arm/cpu/armv8/cache_v8.c +20 -29

index a96ecda..67dbd46 100644
@@ -12,15 +12,14 @@
 DECLARE_GLOBAL_DATA_PTR;
 
 #ifndef CONFIG_SYS_DCACHE_OFF
-
-static void set_pgtable_section(u64 section, u64 memory_type)
+void set_pgtable_section(u64 *page_table, u64 index, u64 section,
+			 u64 memory_type)
 {
-	u64 *page_table = (u64 *)gd->arch.tlb_addr;
 	u64 value;
 
-	value = (section << SECTION_SHIFT) | PMD_TYPE_SECT | PMD_SECT_AF;
+	value = section | PMD_TYPE_SECT | PMD_SECT_AF;
 	value |= PMD_ATTRINDX(memory_type);
-	page_table[section] = value;
+	page_table[index] = value;
 }
 
 /* to activate the MMU we need to set up virtual memory */

@@ -28,10 +27,13 @@ static void mmu_setup(void)
 {
 	int i, j, el;
 	bd_t *bd = gd->bd;
+	u64 *page_table = (u64 *)gd->arch.tlb_addr;
 
 	/* Setup an identity-mapping for all spaces */
-	for (i = 0; i < (PGTABLE_SIZE >> 3); i++)
-		set_pgtable_section(i, MT_DEVICE_NGNRNE);
+	for (i = 0; i < (PGTABLE_SIZE >> 3); i++) {
+		set_pgtable_section(page_table, i, i << SECTION_SHIFT,
+				    MT_DEVICE_NGNRNE);
+	}
 
 	/* Setup an identity-mapping for all RAM space */
 	for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {

@@ -39,36 +41,25 @@ static void mmu_setup(void)
 		ulong end = bd->bi_dram[i].start + bd->bi_dram[i].size;
 		for (j = start >> SECTION_SHIFT;
 		     j < end >> SECTION_SHIFT; j++) {
-			set_pgtable_section(j, MT_NORMAL);
+			set_pgtable_section(page_table, j, j << SECTION_SHIFT,
+					    MT_NORMAL);
 		}
 	}
 
 	/* load TTBR0 */
 	el = current_el();
 	if (el == 1) {
-		asm volatile("msr ttbr0_el1, %0"
-			     : : "r" (gd->arch.tlb_addr) : "memory");
-		asm volatile("msr tcr_el1, %0"
-			     : : "r" (TCR_FLAGS | TCR_EL1_IPS_BITS)
-			     : "memory");
-		asm volatile("msr mair_el1, %0"
-			     : : "r" (MEMORY_ATTRIBUTES) : "memory");
+		set_ttbr_tcr_mair(el, gd->arch.tlb_addr,
+				  TCR_FLAGS | TCR_EL1_IPS_BITS,
+				  MEMORY_ATTRIBUTES);
 	} else if (el == 2) {
-		asm volatile("msr ttbr0_el2, %0"
-			     : : "r" (gd->arch.tlb_addr) : "memory");
-		asm volatile("msr tcr_el2, %0"
-			     : : "r" (TCR_FLAGS | TCR_EL2_IPS_BITS)
-			     : "memory");
-		asm volatile("msr mair_el2, %0"
-			     : : "r" (MEMORY_ATTRIBUTES) : "memory");
+		set_ttbr_tcr_mair(el, gd->arch.tlb_addr,
+				  TCR_FLAGS | TCR_EL2_IPS_BITS,
+				  MEMORY_ATTRIBUTES);
 	} else {
-		asm volatile("msr ttbr0_el3, %0"
-			     : : "r" (gd->arch.tlb_addr) : "memory");
-		asm volatile("msr tcr_el3, %0"
-			     : : "r" (TCR_FLAGS | TCR_EL2_IPS_BITS)
-			     : "memory");
-		asm volatile("msr mair_el3, %0"
-			     : : "r" (MEMORY_ATTRIBUTES) : "memory");
+		set_ttbr_tcr_mair(el, gd->arch.tlb_addr,
+				  TCR_FLAGS | TCR_EL3_IPS_BITS,
+				  MEMORY_ATTRIBUTES);
 	}
 
 	/* enable the mmu */

arch/arm/include/asm/armv8/mmu.h +0 -0

index 1193e76..7de4ff9 100644
@@ -108,4 +108,27 @@
 				TCR_IRGN_WBWA |		\
 				TCR_T0SZ(VA_BITS))
 

Quoted 8 lines

arch/arm/include/asm/armv8/mmu.h line 116

Quoted 6 lines

arch/arm/include/asm/armv8/mmu.h +6 -0

+#ifndef __ASSEMBLY__
+void set_pgtable_section(u64 *page_table, u64 index,
+			 u64 section, u64 memory_type);
+static inline void set_ttbr_tcr_mair(int el, u64 table, u64 tcr, u64 attr)
+{
+	asm volatile("dsb sy;isb");

Huh? This wasn't anywhere before. Is the isb necessary?

Probably not, but to make sure all previous instructions finish.

Quoted 7 lines

Quoted 4 lines

Which instructions do you care about completing?

You'll certainly want any page table writes to complete, but is anything else in-flight at this point in time?

Before calling this inline function, MMU tables were created. We want the writing completes before setting these registers.

Sure, but the dsb sy guarantees that.

The dsb sy will ensure that all reads and writes before it have become visible to all observers in the full system shareability domain (including the MMU) before any subsequent instructions (in program order) can execute.

I'd only expect to see a "dsb $DOMAIN; isb" sequence is when we're changing things that affect the instruction stream (I-cache maintenance, changing the mapping underlying the currently executing stream).

Quoted 68 lines

Quoted 68 lines

Quoted 8 lines

Quoted 5 lines

When is this function expected to be called? Is the MMU expected to be on?

The general ARMv8 code calls this when MMU is off. For the SoC I am enabling, it is called when MMU is off for the first time, but MMU on for the 2nd time.

Ok.

Quoted 10 lines

Quoted 9 lines

arch/arm/include/asm/armv8/mmu.h line 120

Quoted 6 lines

arch/arm/include/asm/armv8/mmu.h +4 -0

+	if (el == 1) {
+		asm volatile("msr ttbr0_el1, %0" : : "r" (table) : "memory");
+		asm volatile("msr tcr_el1, %0" : : "r" (tcr) : "memory");
+		asm volatile("msr mair_el1, %0" : : "r" (attr) : "memory");

If the MMU is on, this looks really scary -- what are you expecting to change in a single invocation?

Quoted 3 lines

It is not scary for general ARMv8 code. MMU is off then this is called. For FSL SoC, I have these called for the 2nd time, when MMU is on. The 2nd time call points to a new MMU table.

Oh but it is ;)

Quoted 18 lines

Quoted 17 lines

arch/arm/include/asm/armv8/mmu.h line 131

Quoted 13 lines

arch/arm/include/asm/armv8/mmu.h +11 -0

+	} else if (el == 2) {
+		asm volatile("msr ttbr0_el2, %0" : : "r" (table) : "memory");
+		asm volatile("msr tcr_el2, %0" : : "r" (tcr) : "memory");
+		asm volatile("msr mair_el2, %0" : : "r" (attr) : "memory");
+	} else if (el == 3) {
+		asm volatile("msr ttbr0_el3, %0" : : "r" (table) : "memory");
+		asm volatile("msr tcr_el3, %0" : : "r" (tcr) : "memory");
+		asm volatile("msr mair_el3, %0" : : "r" (attr) : "memory");
+	} else {
+		hang();
+	}

And no synchronisation to ensure that these writes are complete or even ordered w.r.t. each other?

Quoted 3 lines

That's why I added asm volatile("dsb sy;isb") before them. The order of these write doesn't matter. See the code before my change
http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/cpu/armv8/cache_v8.c;h=a96ecda7e3146996d20a2e46b975dced2769255c;hb=HEAD

Ok, so that orders them against everything _before_ them, but not with respect to each other, or anything _after_ them.

When the MMU is off, you are correct that the ordering of these operations w.r.t. each other doesn't matter. With the MMU on that's not true as the CPU can rerder the operations and can walk the page tables asynchronously.

Changing the MAIR at runtime is somewhat scary because you may be changing attributes for entries which are active in the cache and/or TLBs. I'm not sure I follow why you need to change it once the MMU is on -- there are plenty of subfields in the MAIR that you could configure before turning the MMU on for use later.

Likewise changing the TCR at runtime is somewhat scary because you can change attributes of active cache and/or TLB entries, change fields that affect the way page tables are walked (TG*. T*SZ), all asynchronously w.r.t. the logic that walsk the page tables.

Quoted 8 lines

Quoted 4 lines

Changing the TTBR is fine, except that we didn't invalidate old entries with a TLBI, so we may even have partial translations cahced in the TLBs, and we can get odd translations generated from the combination of the old and new tables.

Maybe a more proper way to do so is to change TTBR only. That's actually what I really need. Or if I really need to change all of them, I should turn off MMU first.

In fact, if you had an API for creating device mappings, you wouldn't even need to change the TTBR -- you'd just need to replace some existing invalid entries with the new mapping, make them visible to the MMU with a dsb, and then carry on. The TLBs aren't allowed to cache invalid mappings, so on the next access to those mappings, the appropriate page table entry will be read into the TLBs.

If you alter multiple levels of a live table then you'll need barriers (DMBs) between populating those levels. If you back device mappings by a single level of table then you can avoid that. Even better, with an API you can do the simple thing today then make it more advanced in future if necessary without having to change your board code.

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

For the 2nd write used by FSL SoC, I need to flush the cache to make sure the table is in main DDR and perform TLB invalidation. That's when the loading of new MMU table happens.

I missed the TLB invaliation -- where is that meant to happen?

After flushing the new MMU table into DDR. To your point above, I will send a new version with updating TTBR only.

Unfortunately, that's too early. The MMU is asynchronous w.r.t. the CPU and so it can re-read the existing tables into the TLBs _before_ it has been programmed with the new table address. The TLBI has to happen _after_ the old tables are no longer pointed to by the TTBR.

What you need to do to replace the active set of tables (assuming that the new mapping has the instruction stream mapped in an identical way) is:

- Write the tables.

- DSB to make them visible to the MMU.

- Write to the appropriate TTBR_*.

- ISB to complete the write to the TTBR_*.

- TLBI to invalidate the old mappings the the TLBs.

- DSB to complete the TLBI.

- If you've changed the instruction stream or system state that will affect the instruction stream, ISB to flush the CPU pipeline.

Quoted 24 lines · Mark Rutland

Quoted 17 lines · York Sun

Quoted 7 lines · Mark Rutland

Quoted 6 lines · York Sun

Quoted 4 lines · Mark Rutland

Quoted 1 line · York Sun

Make MMU functions reusable. Platform code can setup its own MMU tables.

What exactly does platform code need to setup its own tables for?

Quoted 8 lines

Quoted 3 lines

The general ARMv8 MMU table is not detail enough to control memory attribute like cache for all addresses. We have devices mapping to addresses with different requirement for cache control.

And there are no APIs for creating device mappings rather than exporting the raw pagetable accessors and hard-coding them differently in every board file?

Quoted 5 lines

That's a good question. At this point, only two platforms are using ARMv8 code. I am expecting FSL ARMv8 implementation will stay similar, i.e. covered by the file I added. If that's not the case, or more ARMv8 SoCs need special MMU table, we then should introduce such API. Having a full function MMU API may be an overkill for U-boot. We don't need dynamic MMU anyway.

Quoted 4 lines

Maybe. It just seems to me that it would be possible to pre-allocate an empty table that we could place device (nGnRnE?) mappings in. Then all you'd need to call from board code is a function to map a range, rather than having to duplicate logic for creating the tables you want.

It sounds good, but not the case. For the three level tables I am using (level0, level1, level2), I don't have level2 table for every address, that will be too many. Instead, I have a lot of blocks for level1. When I need some fine control within a level1 block range, I have to create a new level2 table. It is doable, but I will hold on that if I can use static table.

Quoted 141 lines

Quoted 126 lines

Quoted 126 lines

Quoted 125 lines

Quoted 115 lines

Quoted 115 lines

Also fix a typo of "TCR_EL3_IPS_BITS" in cache_v8.c.

Signed-off-by York Sun <[email protected]>

CC David Feng <[email protected]>

---
Change log:
v4: new patch, splitted from v3 2/4
Revise set_pgtable_section() to be reused by platform MMU code
Add inline function set_ttbr_tcr_mair() to be used by this and platform mmu code

 arch/arm/cpu/armv8/cache_v8.c    |   49 ++++++++++++++++----------------------
 arch/arm/include/asm/armv8/mmu.h |   23 ++++++++++++++++++
 2 files changed, 43 insertions(+), 29 deletions(-)

arch/arm/cpu/armv8/cache_v8.c +20 -29

index a96ecda..67dbd46 100644
@@ -12,15 +12,14 @@
 DECLARE_GLOBAL_DATA_PTR;
 
 #ifndef CONFIG_SYS_DCACHE_OFF
-
-static void set_pgtable_section(u64 section, u64 memory_type)
+void set_pgtable_section(u64 *page_table, u64 index, u64 section,
+                         u64 memory_type)
 {
-        u64 *page_table = (u64 *)gd->arch.tlb_addr;
         u64 value;
 
-        value = (section << SECTION_SHIFT) | PMD_TYPE_SECT | PMD_SECT_AF;
+        value = section | PMD_TYPE_SECT | PMD_SECT_AF;
         value |= PMD_ATTRINDX(memory_type);
-        page_table[section] = value;
+        page_table[index] = value;
 }
 
 /* to activate the MMU we need to set up virtual memory */

@@ -28,10 +27,13 @@ static void mmu_setup(void)
 {
         int i, j, el;
         bd_t *bd = gd->bd;
+        u64 *page_table = (u64 *)gd->arch.tlb_addr;
 
         /* Setup an identity-mapping for all spaces */
-        for (i = 0; i < (PGTABLE_SIZE >> 3); i++)
-                set_pgtable_section(i, MT_DEVICE_NGNRNE);
+        for (i = 0; i < (PGTABLE_SIZE >> 3); i++) {
+                set_pgtable_section(page_table, i, i << SECTION_SHIFT,
+                                    MT_DEVICE_NGNRNE);
+        }
 
         /* Setup an identity-mapping for all RAM space */
         for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {

@@ -39,36 +41,25 @@ static void mmu_setup(void)
                 ulong end = bd->bi_dram[i].start + bd->bi_dram[i].size;
                 for (j = start >> SECTION_SHIFT;
                      j < end >> SECTION_SHIFT; j++) {
-                        set_pgtable_section(j, MT_NORMAL);
+                        set_pgtable_section(page_table, j, j << SECTION_SHIFT,
+                                            MT_NORMAL);
                 }
         }
 
         /* load TTBR0 */
         el = current_el();
         if (el == 1) {
-                asm volatile("msr ttbr0_el1, %0"
-                             : : "r" (gd->arch.tlb_addr) : "memory");
-                asm volatile("msr tcr_el1, %0"
-                             : : "r" (TCR_FLAGS | TCR_EL1_IPS_BITS)
-                             : "memory");
-                asm volatile("msr mair_el1, %0"
-                             : : "r" (MEMORY_ATTRIBUTES) : "memory");
+                set_ttbr_tcr_mair(el, gd->arch.tlb_addr,
+                                  TCR_FLAGS | TCR_EL1_IPS_BITS,
+                                  MEMORY_ATTRIBUTES);
         } else if (el == 2) {
-                asm volatile("msr ttbr0_el2, %0"
-                             : : "r" (gd->arch.tlb_addr) : "memory");
-                asm volatile("msr tcr_el2, %0"
-                             : : "r" (TCR_FLAGS | TCR_EL2_IPS_BITS)
-                             : "memory");
-                asm volatile("msr mair_el2, %0"
-                             : : "r" (MEMORY_ATTRIBUTES) : "memory");
+                set_ttbr_tcr_mair(el, gd->arch.tlb_addr,
+                                  TCR_FLAGS | TCR_EL2_IPS_BITS,
+                                  MEMORY_ATTRIBUTES);
         } else {
-                asm volatile("msr ttbr0_el3, %0"
-                             : : "r" (gd->arch.tlb_addr) : "memory");
-                asm volatile("msr tcr_el3, %0"
-                             : : "r" (TCR_FLAGS | TCR_EL2_IPS_BITS)
-                             : "memory");
-                asm volatile("msr mair_el3, %0"
-                             : : "r" (MEMORY_ATTRIBUTES) : "memory");
+                set_ttbr_tcr_mair(el, gd->arch.tlb_addr,
+                                  TCR_FLAGS | TCR_EL3_IPS_BITS,
+                                  MEMORY_ATTRIBUTES);
         }
 
         /* enable the mmu */

arch/arm/include/asm/armv8/mmu.h +0 -0

index 1193e76..7de4ff9 100644
@@ -108,4 +108,27 @@
                                 TCR_IRGN_WBWA |         \
                                 TCR_T0SZ(VA_BITS))
 

Quoted 8 lines

arch/arm/include/asm/armv8/mmu.h line 116

Quoted 6 lines

arch/arm/include/asm/armv8/mmu.h +6 -0

+#ifndef __ASSEMBLY__
+void set_pgtable_section(u64 *page_table, u64 index,
+                         u64 section, u64 memory_type);
+static inline void set_ttbr_tcr_mair(int el, u64 table, u64 tcr, u64 attr)
+{
+        asm volatile("dsb sy;isb");

Huh? This wasn't anywhere before. Is the isb necessary?

Probably not, but to make sure all previous instructions finish.

Quoted 7 lines

Quoted 4 lines

Which instructions do you care about completing?

You'll certainly want any page table writes to complete, but is anything else in-flight at this point in time?

Before calling this inline function, MMU tables were created. We want the writing completes before setting these registers.

Sure, but the dsb sy guarantees that.

The dsb sy will ensure that all reads and writes before it have become visible to all observers in the full system shareability domain (including the MMU) before any subsequent instructions (in program order) can execute.

Quoted 3 lines

I'd only expect to see a "dsb $DOMAIN; isb" sequence is when we're changing things that affect the instruction stream (I-cache maintenance, changing the mapping underlying the currently executing stream).

Thanks. I can drop "isb". Please comment on v5 patches.

Quoted 85 lines

Quoted 69 lines

Quoted 69 lines

Quoted 9 lines

Quoted 6 lines

When is this function expected to be called? Is the MMU expected to be on?

The general ARMv8 code calls this when MMU is off. For the SoC I am enabling, it is called when MMU is off for the first time, but MMU on for the 2nd time.

Ok.

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

arch/arm/include/asm/armv8/mmu.h +4 -0

+        if (el == 1) {
+                asm volatile("msr ttbr0_el1, %0" : : "r" (table) : "memory");
+                asm volatile("msr tcr_el1, %0" : : "r" (tcr) : "memory");
+                asm volatile("msr mair_el1, %0" : : "r" (attr) : "memory");

Quoted 6 lines

Quoted 2 lines

If the MMU is on, this looks really scary -- what are you expecting to change in a single invocation?

It is not scary for general ARMv8 code. MMU is off then this is called. For FSL SoC, I have these called for the 2nd time, when MMU is on. The 2nd time call points to a new MMU table.

Oh but it is ;)

Quoted 14 lines

Quoted 14 lines

Quoted 13 lines

arch/arm/include/asm/armv8/mmu.h +11 -0

+        } else if (el == 2) {
+                asm volatile("msr ttbr0_el2, %0" : : "r" (table) : "memory");
+                asm volatile("msr tcr_el2, %0" : : "r" (tcr) : "memory");
+                asm volatile("msr mair_el2, %0" : : "r" (attr) : "memory");
+        } else if (el == 3) {
+                asm volatile("msr ttbr0_el3, %0" : : "r" (table) : "memory");
+                asm volatile("msr tcr_el3, %0" : : "r" (tcr) : "memory");
+                asm volatile("msr mair_el3, %0" : : "r" (attr) : "memory");
+        } else {
+                hang();
+        }

Quoted 7 lines

Quoted 3 lines

And no synchronisation to ensure that these writes are complete or even ordered w.r.t. each other?

That's why I added asm volatile("dsb sy;isb") before them. The order of these write doesn't matter. See the code before my change
http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/cpu/armv8/cache_v8.c;h=a96ecda7e3146996d20a2e46b975dced2769255c;hb=HEAD

Ok, so that orders them against everything _before_ them, but not with respect to each other, or anything _after_ them.

When the MMU is off, you are correct that the ordering of these operations w.r.t. each other doesn't matter. With the MMU on that's not true as the CPU can rerder the operations and can walk the page tables asynchronously.

Changing the MAIR at runtime is somewhat scary because you may be changing attributes for entries which are active in the cache and/or TLBs. I'm not sure I follow why you need to change it once the MMU is on -- there are plenty of subfields in the MAIR that you could configure before turning the MMU on for use later.

Likewise changing the TCR at runtime is somewhat scary because you can change attributes of active cache and/or TLB entries, change fields that affect the way page tables are walked (TG*. T*SZ), all asynchronously w.r.t. the logic that walsk the page tables.

Quoted 8 lines

Quoted 4 lines

Changing the TTBR is fine, except that we didn't invalidate old entries with a TLBI, so we may even have partial translations cahced in the TLBs, and we can get odd translations generated from the combination of the old and new tables.

Maybe a more proper way to do so is to change TTBR only. That's actually what I really need. Or if I really need to change all of them, I should turn off MMU first.

In fact, if you had an API for creating device mappings, you wouldn't even need to change the TTBR -- you'd just need to replace some existing invalid entries with the new mapping, make them visible to the MMU with a dsb, and then carry on. The TLBs aren't allowed to cache invalid mappings, so on the next access to those mappings, the appropriate page table entry will be read into the TLBs.

Quoted 6 lines

If you alter multiple levels of a live table then you'll need barriers (DMBs) between populating those levels. If you back device mappings by a single level of table then you can avoid that. Even better, with an API you can do the simple thing today then make it more advanced in future if necessary without having to change your board code.

No objection here on the idea. But again this is not the case. My first MMU table is in SRAM, which is small and will be used for other purpose. The 2nd MMU table is in DDR. I could copy the table and do the maintenance as you said. For now, I want to stick with the static table and only create the API when I have to.

Quoted 24 lines

Quoted 8 lines

Quoted 6 lines

Quoted 3 lines

For the 2nd write used by FSL SoC, I need to flush the cache to make sure the table is in main DDR and perform TLB invalidation. That's when the loading of new MMU table happens.

I missed the TLB invaliation -- where is that meant to happen?

After flushing the new MMU table into DDR. To your point above, I will send a new version with updating TTBR only.

Unfortunately, that's too early. The MMU is asynchronous w.r.t. the CPU and so it can re-read the existing tables into the TLBs _before_ it has been programmed with the new table address. The TLBI has to happen _after_ the old tables are no longer pointed to by the TTBR.

What you need to do to replace the active set of tables (assuming that the new mapping has the instruction stream mapped in an identical way) is:

- Write the tables.

- DSB to make them visible to the MMU.

- Write to the appropriate TTBR_*.

Quoted 10 lines

- ISB to complete the write to the TTBR_*.

- TLBI to invalidate the old mappings the the TLBs.

- DSB to complete the TLBI.

- If you've changed the instruction stream or system state that will affect the instruction stream, ISB to flush the CPU pipeline.

Here is the flow I have (as of v5 patch)

Write the tables

(I removed dsb here in v5, need to add back)

Write TTBR

(I missed isb here, need to add)

Flush dcache (otherwise the table will not be in DDR. Yes, I verified)

TLBI

(DSB and ISB TLBI function)

I will need to send v6 patch to add the missing parts.

Quoted 30 lines · York Sun

Quoted 24 lines · Mark Rutland

Quoted 17 lines · York Sun

Quoted 7 lines · Mark Rutland

Quoted 6 lines · York Sun

Quoted 4 lines · Mark Rutland

Quoted 1 line · York Sun

Make MMU functions reusable. Platform code can setup its own MMU tables.

What exactly does platform code need to setup its own tables for?

Quoted 8 lines

Quoted 3 lines

The general ARMv8 MMU table is not detail enough to control memory attribute like cache for all addresses. We have devices mapping to addresses with different requirement for cache control.

And there are no APIs for creating device mappings rather than exporting the raw pagetable accessors and hard-coding them differently in every board file?

Quoted 5 lines

That's a good question. At this point, only two platforms are using ARMv8 code. I am expecting FSL ARMv8 implementation will stay similar, i.e. covered by the file I added. If that's not the case, or more ARMv8 SoCs need special MMU table, we then should introduce such API. Having a full function MMU API may be an overkill for U-boot. We don't need dynamic MMU anyway.

Quoted 4 lines

Maybe. It just seems to me that it would be possible to pre-allocate an empty table that we could place device (nGnRnE?) mappings in. Then all you'd need to call from board code is a function to map a range, rather than having to duplicate logic for creating the tables you want.

Quoted 5 lines

It sounds good, but not the case. For the three level tables I am using (level0, level1, level2), I don't have level2 table for every address, that will be too many. Instead, I have a lot of blocks for level1. When I need some fine control within a level1 block range, I have to create a new level2 table. It is doable, but I will hold on that if I can use static table.

While my suggestion might not be the best, I'm not sure I follow, unless you always want to idmap devices?

If you don't idmap devices, then you can place all of the disparate physical mappings within a single table unless you have very large peripherals to map?

Quoted 142 lines

Quoted 142 lines

Quoted 127 lines

Quoted 127 lines

Quoted 126 lines

Quoted 116 lines

Quoted 116 lines

Also fix a typo of "TCR_EL3_IPS_BITS" in cache_v8.c.

Signed-off-by York Sun <[email protected]>

CC David Feng <[email protected]>

---
Change log:
v4: new patch, splitted from v3 2/4
Revise set_pgtable_section() to be reused by platform MMU code
Add inline function set_ttbr_tcr_mair() to be used by this and platform mmu code

 arch/arm/cpu/armv8/cache_v8.c    |   49 ++++++++++++++++----------------------
 arch/arm/include/asm/armv8/mmu.h |   23 ++++++++++++++++++
 2 files changed, 43 insertions(+), 29 deletions(-)

arch/arm/cpu/armv8/cache_v8.c +20 -29

index a96ecda..67dbd46 100644
@@ -12,15 +12,14 @@
 DECLARE_GLOBAL_DATA_PTR;
 
 #ifndef CONFIG_SYS_DCACHE_OFF
-
-static void set_pgtable_section(u64 section, u64 memory_type)
+void set_pgtable_section(u64 *page_table, u64 index, u64 section,
+                         u64 memory_type)
 {
-        u64 *page_table = (u64 *)gd->arch.tlb_addr;
         u64 value;
 
-        value = (section << SECTION_SHIFT) | PMD_TYPE_SECT | PMD_SECT_AF;
+        value = section | PMD_TYPE_SECT | PMD_SECT_AF;
         value |= PMD_ATTRINDX(memory_type);
-        page_table[section] = value;
+        page_table[index] = value;
 }
 
 /* to activate the MMU we need to set up virtual memory */

@@ -28,10 +27,13 @@ static void mmu_setup(void)
 {
         int i, j, el;
         bd_t *bd = gd->bd;
+        u64 *page_table = (u64 *)gd->arch.tlb_addr;
 
         /* Setup an identity-mapping for all spaces */
-        for (i = 0; i < (PGTABLE_SIZE >> 3); i++)
-                set_pgtable_section(i, MT_DEVICE_NGNRNE);
+        for (i = 0; i < (PGTABLE_SIZE >> 3); i++) {
+                set_pgtable_section(page_table, i, i << SECTION_SHIFT,
+                                    MT_DEVICE_NGNRNE);
+        }
 
         /* Setup an identity-mapping for all RAM space */
         for (i = 0; i < CONFIG_NR_DRAM_BANKS; i++) {

@@ -39,36 +41,25 @@ static void mmu_setup(void)
                 ulong end = bd->bi_dram[i].start + bd->bi_dram[i].size;
                 for (j = start >> SECTION_SHIFT;
                      j < end >> SECTION_SHIFT; j++) {
-                        set_pgtable_section(j, MT_NORMAL);
+                        set_pgtable_section(page_table, j, j << SECTION_SHIFT,
+                                            MT_NORMAL);
                 }
         }
 
         /* load TTBR0 */
         el = current_el();
         if (el == 1) {
-                asm volatile("msr ttbr0_el1, %0"
-                             : : "r" (gd->arch.tlb_addr) : "memory");
-                asm volatile("msr tcr_el1, %0"
-                             : : "r" (TCR_FLAGS | TCR_EL1_IPS_BITS)
-                             : "memory");
-                asm volatile("msr mair_el1, %0"
-                             : : "r" (MEMORY_ATTRIBUTES) : "memory");
+                set_ttbr_tcr_mair(el, gd->arch.tlb_addr,
+                                  TCR_FLAGS | TCR_EL1_IPS_BITS,
+                                  MEMORY_ATTRIBUTES);
         } else if (el == 2) {
-                asm volatile("msr ttbr0_el2, %0"
-                             : : "r" (gd->arch.tlb_addr) : "memory");
-                asm volatile("msr tcr_el2, %0"
-                             : : "r" (TCR_FLAGS | TCR_EL2_IPS_BITS)
-                             : "memory");
-                asm volatile("msr mair_el2, %0"
-                             : : "r" (MEMORY_ATTRIBUTES) : "memory");
+                set_ttbr_tcr_mair(el, gd->arch.tlb_addr,
+                                  TCR_FLAGS | TCR_EL2_IPS_BITS,
+                                  MEMORY_ATTRIBUTES);
         } else {
-                asm volatile("msr ttbr0_el3, %0"
-                             : : "r" (gd->arch.tlb_addr) : "memory");
-                asm volatile("msr tcr_el3, %0"
-                             : : "r" (TCR_FLAGS | TCR_EL2_IPS_BITS)
-                             : "memory");
-                asm volatile("msr mair_el3, %0"
-                             : : "r" (MEMORY_ATTRIBUTES) : "memory");
+                set_ttbr_tcr_mair(el, gd->arch.tlb_addr,
+                                  TCR_FLAGS | TCR_EL3_IPS_BITS,
+                                  MEMORY_ATTRIBUTES);
         }
 
         /* enable the mmu */

arch/arm/include/asm/armv8/mmu.h +0 -0

index 1193e76..7de4ff9 100644
@@ -108,4 +108,27 @@
                                 TCR_IRGN_WBWA |         \
                                 TCR_T0SZ(VA_BITS))
 

Quoted 8 lines

arch/arm/include/asm/armv8/mmu.h line 116

Quoted 6 lines

arch/arm/include/asm/armv8/mmu.h +6 -0

+#ifndef __ASSEMBLY__
+void set_pgtable_section(u64 *page_table, u64 index,
+                         u64 section, u64 memory_type);
+static inline void set_ttbr_tcr_mair(int el, u64 table, u64 tcr, u64 attr)
+{
+        asm volatile("dsb sy;isb");

Huh? This wasn't anywhere before. Is the isb necessary?

Probably not, but to make sure all previous instructions finish.

Quoted 7 lines

Quoted 4 lines

Which instructions do you care about completing?

You'll certainly want any page table writes to complete, but is anything else in-flight at this point in time?

Before calling this inline function, MMU tables were created. We want the writing completes before setting these registers.

Sure, but the dsb sy guarantees that.

The dsb sy will ensure that all reads and writes before it have become visible to all observers in the full system shareability domain (including the MMU) before any subsequent instructions (in program order) can execute.

Quoted 5 lines

Quoted 3 lines

I'd only expect to see a "dsb $DOMAIN; isb" sequence is when we're changing things that affect the instruction stream (I-cache maintenance, changing the mapping underlying the currently executing stream).

Thanks. I can drop "isb". Please comment on v5 patches.

Will do momentarily.

Quoted 92 lines

Quoted 85 lines

Quoted 69 lines

Quoted 69 lines

Quoted 9 lines

Quoted 6 lines

When is this function expected to be called? Is the MMU expected to be on?

The general ARMv8 code calls this when MMU is off. For the SoC I am enabling, it is called when MMU is off for the first time, but MMU on for the 2nd time.

Ok.

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

arch/arm/include/asm/armv8/mmu.h +4 -0

+        if (el == 1) {
+                asm volatile("msr ttbr0_el1, %0" : : "r" (table) : "memory");
+                asm volatile("msr tcr_el1, %0" : : "r" (tcr) : "memory");
+                asm volatile("msr mair_el1, %0" : : "r" (attr) : "memory");

Quoted 6 lines

Quoted 2 lines

If the MMU is on, this looks really scary -- what are you expecting to change in a single invocation?

It is not scary for general ARMv8 code. MMU is off then this is called. For FSL SoC, I have these called for the 2nd time, when MMU is on. The 2nd time call points to a new MMU table.

Oh but it is ;)

Quoted 14 lines

Quoted 14 lines

Quoted 13 lines

arch/arm/include/asm/armv8/mmu.h +11 -0

+        } else if (el == 2) {
+                asm volatile("msr ttbr0_el2, %0" : : "r" (table) : "memory");
+                asm volatile("msr tcr_el2, %0" : : "r" (tcr) : "memory");
+                asm volatile("msr mair_el2, %0" : : "r" (attr) : "memory");
+        } else if (el == 3) {
+                asm volatile("msr ttbr0_el3, %0" : : "r" (table) : "memory");
+                asm volatile("msr tcr_el3, %0" : : "r" (tcr) : "memory");
+                asm volatile("msr mair_el3, %0" : : "r" (attr) : "memory");
+        } else {
+                hang();
+        }

Quoted 7 lines

Quoted 3 lines

And no synchronisation to ensure that these writes are complete or even ordered w.r.t. each other?

That's why I added asm volatile("dsb sy;isb") before them. The order of these write doesn't matter. See the code before my change
http://git.denx.de/?p=u-boot.git;a=blob;f=arch/arm/cpu/armv8/cache_v8.c;h=a96ecda7e3146996d20a2e46b975dced2769255c;hb=HEAD

Ok, so that orders them against everything _before_ them, but not with respect to each other, or anything _after_ them.

When the MMU is off, you are correct that the ordering of these operations w.r.t. each other doesn't matter. With the MMU on that's not true as the CPU can rerder the operations and can walk the page tables asynchronously.

Changing the MAIR at runtime is somewhat scary because you may be changing attributes for entries which are active in the cache and/or TLBs. I'm not sure I follow why you need to change it once the MMU is on -- there are plenty of subfields in the MAIR that you could configure before turning the MMU on for use later.

Likewise changing the TCR at runtime is somewhat scary because you can change attributes of active cache and/or TLB entries, change fields that affect the way page tables are walked (TG*. T*SZ), all asynchronously w.r.t. the logic that walsk the page tables.

Quoted 8 lines

Quoted 4 lines

Changing the TTBR is fine, except that we didn't invalidate old entries with a TLBI, so we may even have partial translations cahced in the TLBs, and we can get odd translations generated from the combination of the old and new tables.

Maybe a more proper way to do so is to change TTBR only. That's actually what I really need. Or if I really need to change all of them, I should turn off MMU first.

In fact, if you had an API for creating device mappings, you wouldn't even need to change the TTBR -- you'd just need to replace some existing invalid entries with the new mapping, make them visible to the MMU with a dsb, and then carry on. The TLBs aren't allowed to cache invalid mappings, so on the next access to those mappings, the appropriate page table entry will be read into the TLBs.

Quoted 6 lines

If you alter multiple levels of a live table then you'll need barriers (DMBs) between populating those levels. If you back device mappings by a single level of table then you can avoid that. Even better, with an API you can do the simple thing today then make it more advanced in future if necessary without having to change your board code.

Quoted 4 lines

No objection here on the idea. But again this is not the case. My first MMU table is in SRAM, which is small and will be used for other purpose. The 2nd MMU table is in DDR. I could copy the table and do the maintenance as you said. For now, I want to stick with the static table and only create the API when I have to.

Sure, if your tables are in SRAM then trying to do a load of dynamic allocation isn't going to work.

My fear is that while that sounds OK with a single user to do a quick havk and poke the tables directly, we'll end up with everyone doing that, and no-one will try to unify things. It is very diffifcult to unify such variation after the fact.

Quoted 41 lines

Quoted 25 lines

Quoted 9 lines

Quoted 7 lines

Quoted 4 lines

For the 2nd write used by FSL SoC, I need to flush the cache to make sure the table is in main DDR and perform TLB invalidation. That's when the loading of new MMU table happens.

I missed the TLB invaliation -- where is that meant to happen?

After flushing the new MMU table into DDR. To your point above, I will send a new version with updating TTBR only.

Unfortunately, that's too early. The MMU is asynchronous w.r.t. the CPU and so it can re-read the existing tables into the TLBs _before_ it has been programmed with the new table address. The TLBI has to happen _after_ the old tables are no longer pointed to by the TTBR.

What you need to do to replace the active set of tables (assuming that the new mapping has the instruction stream mapped in an identical way) is:

- Write the tables.

- DSB to make them visible to the MMU.

- Write to the appropriate TTBR_*.

Quoted 10 lines

- ISB to complete the write to the TTBR_*.

- TLBI to invalidate the old mappings the the TLBs.

- DSB to complete the TLBI.

- If you've changed the instruction stream or system state that will affect the instruction stream, ISB to flush the CPU pipeline.

Here is the flow I have (as of v5 patch)

Write the tables

(I removed dsb here in v5, need to add back)

Quoted 5 lines

Write TTBR

(I missed isb here, need to add)

Flush dcache (otherwise the table will not be in DDR. Yes, I verified)

This looks odd -- why do we need the tables to be in DDR? Why would we flush them here, when the address is partially visible to the MMU?

Quoted 4 lines

TLBI

(DSB and ISB TLBI function)

Why another TLBI?

Quoted 3 lines

I will need to send v6 patch to add the missing parts.

Sure, I'll hold off replies here until then.

Quoted 26 lines

Quoted 26 lines

Quoted 10 lines

What you need to do to replace the active set of tables (assuming that the new mapping has the instruction stream mapped in an identical way) is:

- Write the tables.

- DSB to make them visible to the MMU.

- Write to the appropriate TTBR_*.

Quoted 10 lines

- ISB to complete the write to the TTBR_*.

- TLBI to invalidate the old mappings the the TLBs.

- DSB to complete the TLBI.

- If you've changed the instruction stream or system state that will affect the instruction stream, ISB to flush the CPU pipeline.

Here is the flow I have (as of v5 patch)

Write the tables

(I removed dsb here in v5, need to add back)

Quoted 8 lines

Quoted 5 lines

Write TTBR

(I missed isb here, need to add)

Flush dcache (otherwise the table will not be in DDR. Yes, I verified)

This looks odd -- why do we need the tables to be in DDR? Why would we flush them here, when the address is partially visible to the MMU?

This sounds odd but it actually makes sense. Let's say we have new tables created by u-boot. The new tables are in the address of DDR with D-cache enabled. If we don't flush the cache, the moment TLBs are invalidated, MMU will fetch the tables from TTBR which points to DDR. Without a valid TLB, MMU cannot get the table from D-cache, it has to fetch from the DDR directly. I have verified this by checking waveforms of the SoC and exercised code in both ways.

[...]

Quoted 35 lines

Quoted 26 lines

Quoted 26 lines

Quoted 10 lines

What you need to do to replace the active set of tables (assuming that the new mapping has the instruction stream mapped in an identical way) is:

- Write the tables.

- DSB to make them visible to the MMU.

- Write to the appropriate TTBR_*.

Quoted 10 lines

- ISB to complete the write to the TTBR_*.

- TLBI to invalidate the old mappings the the TLBs.

- DSB to complete the TLBI.

- If you've changed the instruction stream or system state that will affect the instruction stream, ISB to flush the CPU pipeline.

Here is the flow I have (as of v5 patch)

Write the tables

(I removed dsb here in v5, need to add back)

Quoted 8 lines

Quoted 5 lines

Write TTBR

(I missed isb here, need to add)

Flush dcache (otherwise the table will not be in DDR. Yes, I verified)

This looks odd -- why do we need the tables to be in DDR? Why would we flush them here, when the address is partially visible to the MMU?

Quoted 6 lines

This sounds odd but it actually makes sense. Let's say we have new tables created by u-boot. The new tables are in the address of DDR with D-cache enabled. If we don't flush the cache, the moment TLBs are invalidated, MMU will fetch the tables from TTBR which points to DDR. Without a valid TLB, MMU cannot get the table from D-cache, it has to fetch from the DDR directly. I have verified this by checking waveforms of the SoC and exercised code in both ways.

How is TCR_EL2.SH0 (or TCR_EL1.SH*) configured?

You'll only need to flush the cache if they're configured non shareable.

Quoted 44 lines · Mark Rutland

[...]

Quoted 41 lines

Quoted 26 lines

Quoted 26 lines

Quoted 10 lines

What you need to do to replace the active set of tables (assuming that the new mapping has the instruction stream mapped in an identical way) is:

- Write the tables.

- DSB to make them visible to the MMU.

- Write to the appropriate TTBR_*.

Quoted 10 lines

- ISB to complete the write to the TTBR_*.

- TLBI to invalidate the old mappings the the TLBs.

- DSB to complete the TLBI.

- If you've changed the instruction stream or system state that will affect the instruction stream, ISB to flush the CPU pipeline.

Here is the flow I have (as of v5 patch)

Write the tables

(I removed dsb here in v5, need to add back)

Quoted 8 lines

Quoted 5 lines

Write TTBR

(I missed isb here, need to add)

Flush dcache (otherwise the table will not be in DDR. Yes, I verified)

This looks odd -- why do we need the tables to be in DDR? Why would we flush them here, when the address is partially visible to the MMU?

This sounds odd but it actually makes sense. Let's say we have new tables created by u-boot. The new tables are in the address of DDR with D-cache enabled. If we don't flush the cache, the moment TLBs are invalidated, MMU will fetch the tables from TTBR which points to DDR. Without a valid TLB, MMU cannot get the table from D-cache, it has to fetch from the DDR directly. I have verified this by checking waveforms of the SoC and exercised code in both ways.

Quoted 3 lines

How is TCR_EL2.SH0 (or TCR_EL1.SH*) configured?

You'll only need to flush the cache if they're configured non shareable.

It is configured as non shareable.

Quoted 47 lines · York Sun

Quoted 46 lines · Mark Rutland

[...]

Quoted 35 lines

Quoted 26 lines

Quoted 26 lines

Quoted 10 lines

What you need to do to replace the active set of tables (assuming that the new mapping has the instruction stream mapped in an identical way) is:

- Write the tables.

- DSB to make them visible to the MMU.

- Write to the appropriate TTBR_*.

Quoted 10 lines

- ISB to complete the write to the TTBR_*.

- TLBI to invalidate the old mappings the the TLBs.

- DSB to complete the TLBI.

- If you've changed the instruction stream or system state that will affect the instruction stream, ISB to flush the CPU pipeline.

Here is the flow I have (as of v5 patch)

Write the tables

(I removed dsb here in v5, need to add back)

Quoted 8 lines

Quoted 5 lines

Write TTBR

(I missed isb here, need to add)

Flush dcache (otherwise the table will not be in DDR. Yes, I verified)

This looks odd -- why do we need the tables to be in DDR? Why would we flush them here, when the address is partially visible to the MMU?

Quoted 6 lines

This sounds odd but it actually makes sense. Let's say we have new tables created by u-boot. The new tables are in the address of DDR with D-cache enabled. If we don't flush the cache, the moment TLBs are invalidated, MMU will fetch the tables from TTBR which points to DDR. Without a valid TLB, MMU cannot get the table from D-cache, it has to fetch from the DDR directly. I have verified this by checking waveforms of the SoC and exercised code in both ways.

How is TCR_EL2.SH0 (or TCR_EL1.SH*) configured?

Quoted 3 lines

Quoted 1 line

You'll only need to flush the cache if they're configured non shareable.

It is configured as non shareable.

Is there any reason not to configure them as inner shareable? That way the MMU will look in the D-cache, and you won't have to spend time flushing them.

Quoted 5 lines

Quoted 3 lines

How is TCR_EL2.SH0 (or TCR_EL1.SH*) configured?

You'll only need to flush the cache if they're configured non shareable.

It is configured as non shareable.

Is there any reason not to configure them as inner shareable? That way the MMU will look in the D-cache, and you won't have to spend time flushing them.

Quoted 10 lines · Mark Rutland

Quoted 5 lines

Quoted 3 lines

How is TCR_EL2.SH0 (or TCR_EL1.SH*) configured?

You'll only need to flush the cache if they're configured non shareable.

It is configured as non shareable.

Is there any reason not to configure them as inner shareable? That way the MMU will look in the D-cache, and you won't have to spend time flushing them.

Mark,

I appreciate the reminder. I tried on our emulator. With inner share set for TCR SH0 bits, u-boot works with the flushing, but doesn't work without flushing. It went to exception.

Can you share more information about the inner share? I need to follow up with our designer to confirm.

Quoted 18 lines · York Sun

Quoted 10 lines · Mark Rutland

Quoted 5 lines

Quoted 3 lines

How is TCR_EL2.SH0 (or TCR_EL1.SH*) configured?

You'll only need to flush the cache if they're configured non shareable.

It is configured as non shareable.

Is there any reason not to configure them as inner shareable? That way the MMU will look in the D-cache, and you won't have to spend time flushing them.

Mark,

I appreciate the reminder. I tried on our emulator. With inner share set for TCR SH0 bits, u-boot works with the flushing, but doesn't work without flushing. It went to exception.

Quoted 3 lines

Can you share more information about the inner share? I need to follow up with our designer to confirm.

A second thought, do I need to set the first MMU table so DDR is inner shareable?

Hi,

Apologies for the delay in replying.

Quoted 19 lines · York Sun

Quoted 18 lines · York Sun

Quoted 10 lines · Mark Rutland

Quoted 5 lines

Quoted 3 lines

How is TCR_EL2.SH0 (or TCR_EL1.SH*) configured?

You'll only need to flush the cache if they're configured non shareable.

It is configured as non shareable.

Is there any reason not to configure them as inner shareable? That way the MMU will look in the D-cache, and you won't have to spend time flushing them.

Mark,

I appreciate the reminder. I tried on our emulator. With inner share set for TCR SH0 bits, u-boot works with the flushing, but doesn't work without flushing. It went to exception.

Quoted 5 lines

Quoted 3 lines

Can you share more information about the inner share? I need to follow up with our designer to confirm.

A second thought, do I need to set the first MMU table so DDR is inner shareable?

I assume you mean configuring MAIR_ELx such that the mapping covering DDR is cacheable for the inner shareable domain? If so, yes.

Hi,

Apologies for the delay in replying.

Quoted 19 lines · York Sun

Quoted 18 lines · York Sun

Quoted 10 lines · Mark Rutland

Quoted 5 lines

Quoted 3 lines

How is TCR_EL2.SH0 (or TCR_EL1.SH*) configured?

You'll only need to flush the cache if they're configured non shareable.

It is configured as non shareable.

Is there any reason not to configure them as inner shareable? That way the MMU will look in the D-cache, and you won't have to spend time flushing them.

Mark,

I appreciate the reminder. I tried on our emulator. With inner share set for TCR SH0 bits, u-boot works with the flushing, but doesn't work without flushing. It went to exception.

Quoted 5 lines

Quoted 3 lines

Can you share more information about the inner share? I need to follow up with our designer to confirm.

A second thought, do I need to set the first MMU table so DDR is inner shareable?

I assume you mean configuring MAIR_ELx such that the mapping covering DDR is cacheable for the inner shareable domain? If so, yes.

Quoted 24 lines

Hi,

Apologies for the delay in replying.

Quoted 19 lines · York Sun

Quoted 18 lines · York Sun

Quoted 10 lines · Mark Rutland

Quoted 5 lines

Quoted 3 lines

How is TCR_EL2.SH0 (or TCR_EL1.SH*) configured?

You'll only need to flush the cache if they're configured non shareable.

It is configured as non shareable.

Is there any reason not to configure them as inner shareable? That way the MMU will look in the D-cache, and you won't have to spend time flushing them.

Mark,

I appreciate the reminder. I tried on our emulator. With inner share set for TCR SH0 bits, u-boot works with the flushing, but doesn't work without flushing. It went to exception.

Quoted 9 lines

Quoted 5 lines

Quoted 3 lines

Can you share more information about the inner share? I need to follow up with our designer to confirm.

A second thought, do I need to set the first MMU table so DDR is inner shareable?

I assume you mean configuring MAIR_ELx such that the mapping covering DDR is cacheable for the inner shareable domain? If so, yes.

Mark,

I tried both inner share and outer. It doesn't work without flushing the cache. I will keep this part of code until I learn otherwise.

York

hi York,

Quoted 35 lines · Mark Rutland

Quoted 24 lines

Hi,

Apologies for the delay in replying.

Quoted 19 lines · York Sun

Quoted 18 lines · York Sun

Quoted 10 lines · Mark Rutland

Quoted 5 lines

Quoted 3 lines

How is TCR_EL2.SH0 (or TCR_EL1.SH*) configured?

You'll only need to flush the cache if they're configured non shareable.

It is configured as non shareable.

Is there any reason not to configure them as inner shareable? That way the MMU will look in the D-cache, and you won't have to spend time flushing them.

Mark,

I appreciate the reminder. I tried on our emulator. With inner share set for TCR SH0 bits, u-boot works with the flushing, but doesn't work without flushing. It went to exception.

Quoted 9 lines

Quoted 5 lines

Quoted 3 lines

Can you share more information about the inner share? I need to follow up with our designer to confirm.

A second thought, do I need to set the first MMU table so DDR is inner shareable?

I assume you mean configuring MAIR_ELx such that the mapping covering DDR is cacheable for the inner shareable domain? If so, yes.

Quoted 7 lines

Mark,

I tried both inner share and outer. It doesn't work without flushing the cache. I will keep this part of code until I learn otherwise.

York

The shareability attribute is different with cacheablilty attribute, it means whether the memory region would be accessed by other processors. If a memory region is configured as non sharable the access will not be snooped but still can be cached. It is the situation that current u-boot-armv8 used due to only master processor is active. If secondary processors also access the same memory region it should be configured as inner-sharable or outer-sharable otherwise the cache coherence between processors will not be maintained.
The above is what I know. Wish this could help.

Quoted 2 lines · Mark Rutland

Quoted 1 line · York Sun

On 06/05/2014 03:09 AM, Mark Rutland wrote:

Quoted 7 lines

Quoted 4 lines

No objection here on the idea. But again this is not the case. My first MMU table is in SRAM, which is small and will be used for other purpose. The 2nd MMU table is in DDR. I could copy the table and do the maintenance as you said. For now, I want to stick with the static table and only create the API when I have to.

Sure, if your tables are in SRAM then trying to do a load of dynamic allocation isn't going to work.

Quoted 4 lines

My fear is that while that sounds OK with a single user to do a quick havk and poke the tables directly, we'll end up with everyone doing that, and no-one will try to unify things. It is very diffifcult to unify such variation after the fact.

That's a good reason. Let me start to code the API. It will take a while to cover the complexity of the multilevel tables and sizes. It will be a separated patch for later review. I don't want that to delay this patch set. I am hoping to get this set in for 2014.07 release.

Quoted 3 lines · York Sun

Quoted 2 lines · Mark Rutland

Quoted 1 line · York Sun

On 06/05/2014 03:09 AM, Mark Rutland wrote:

[...]

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

No objection here on the idea. But again this is not the case. My first MMU table is in SRAM, which is small and will be used for other purpose. The 2nd MMU table is in DDR. I could copy the table and do the maintenance as you said. For now, I want to stick with the static table and only create the API when I have to.

Quoted 2 lines

Quoted 2 lines

Sure, if your tables are in SRAM then trying to do a load of dynamic allocation isn't going to work.

Why do you need to turn on the MMU early using SRAM in the first place? Can't you delay that until after DDR setup?

Quoted 10 lines

Quoted 5 lines

My fear is that while that sounds OK with a single user to do a quick havk and poke the tables directly, we'll end up with everyone doing that, and no-one will try to unify things. It is very diffifcult to unify such variation after the fact.

That's a good reason. Let me start to code the API. It will take a while to cover the complexity of the multilevel tables and sizes. It will be a separated patch for later review. I don't want that to delay this patch set. I am hoping to get this set in for 2014.07 release.

If I was maintainer I would say no because few people come back later to clean-up their mess. If you want to get platform support in now, perhaps you should just add the base platform first and add mmu setup later. Surely you don't need the MMU to just boot to u-boot shell.

Quoted 12 lines · Rob Herring

Quoted 3 lines · York Sun

Quoted 2 lines · Mark Rutland

Quoted 1 line · York Sun

On 06/05/2014 03:09 AM, Mark Rutland wrote:

[...]

Quoted 5 lines

Quoted 5 lines

Quoted 4 lines

No objection here on the idea. But again this is not the case. My first MMU table is in SRAM, which is small and will be used for other purpose. The 2nd MMU table is in DDR. I could copy the table and do the maintenance as you said. For now, I want to stick with the static table and only create the API when I have to.

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

Sure, if your tables are in SRAM then trying to do a load of dynamic allocation isn't going to work.

Why do you need to turn on the MMU early using SRAM in the first place? Can't you delay that until after DDR setup?

Logically yes. But it runs too slow without cache on emulator.

Quoted 11 lines

Quoted 10 lines

Quoted 5 lines

My fear is that while that sounds OK with a single user to do a quick havk and poke the tables directly, we'll end up with everyone doing that, and no-one will try to unify things. It is very diffifcult to unify such variation after the fact.

That's a good reason. Let me start to code the API. It will take a while to cover the complexity of the multilevel tables and sizes. It will be a separated patch for later review. I don't want that to delay this patch set. I am hoping to get this set in for 2014.07 release.

Quoted 5 lines

If I was maintainer I would say no because few people come back later to clean-up their mess. If you want to get platform support in now, perhaps you should just add the base platform first and add mmu setup later. Surely you don't need the MMU to just boot to u-boot shell.

My plan is to get the first platform in, then I will maintain Freescale stuff. So you can be sure I will continue to improve it. One reason to get these code in early is to enable our partners and early adopters to use u-boot. After all, this is the second ARMv8 platform. The first one vexpress_aemv8a doesn't even support cache.

/*
* Performs a clean & invalidation of the entire data cache at all levels */
void flush_dcache_all(void)

{
	__asm_flush_dcache_all();

arch/arm/cpu/armv8/cache_v8.c +1 -0

+	flush_l3_cache();

}

Dear York,
My mailing list disabled a few days. Maybe I missed something important.

Quoted 9 lines

/*
* Performs a clean & invalidation of the entire data cache at all levels */
void flush_dcache_all(void)

{
	__asm_flush_dcache_all();

arch/arm/cpu/armv8/cache_v8.c +1 -0

+	flush_l3_cache();

}

I thought the L3 cache is not included in the cache hierarchy.
So, how about define it as external cache operations named with outer_cache_* just like armv7 did?