Skip to contentSkip to conversation

arm: mmu: Add missing volatile for reading SCTLR register

PATCH New Alison Wang 3 participants 4 messages Sep 9, 2015 – Oct 16, 2015 Arm (32-bit)

lore
Share & tools
lore

Alison Wang

author
#0 lore raw

PATCH 1/1 arm: mmu: Add missing volatile for reading SCTLR register

When building u-boot with the latest Linaro toolchain, such as gcc-linaro-4.9, u-boot will hang at PCIE init on LS1021A platform. The issue is reported on
http://comments.gmane.org/gmane.linux.linaro.toolchain/5163.

As volatile is missing when reading SCTLR register and SCTLR is set according to the value read from SCTLR, it causes CR_M bit is not set. Then MMU is not enabled, the access to VA for PCIE fails.

This patch will add the missing volatile for reading SCTLR register.

Signed-off-by Alison Wang <[email protected]>
---
 arch/arm/include/asm/system.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
arch/arm/include/asm/system.h +1 -1
index 89f2294..f5096dc 100644
@@ -155,7 +155,7 @@ void flush_l3_cache(void);
 static inline unsigned int get_cr(void)
 {
 	unsigned int val;
-	asm("mrc p15, 0, %0, c1, c0, 0	@ get CR" : "=r" (val) : : "cc");
+	asm volatile("mrc p15, 0, %0, c1, c0, 0	@ get CR" : "=r" (val) : : "cc");
 	return val;
 }
Signature · 1 line
2.1.0.27.g96db324
#1 lore raw

PATCH 1/1 arm: mmu: Add missing volatile for reading SCTLR register

Hello Alison,

Sorry for the late comment.

Quoted 8 lines · Alison Wang

When building u-boot with the latest Linaro toolchain, such as gcc-linaro-4.9, u-boot will hang at PCIE init on LS1021A platform. The issue is reported on
http://comments.gmane.org/gmane.linux.linaro.toolchain/5163.

As volatile is missing when reading SCTLR register and SCTLR is set according to the value read from SCTLR, it causes CR_M bit is not set. Then MMU is not enabled, the access to VA for PCIE fails.

This comment seems misleading to me: it appears to imply that the "volatile" qualifier is added to alter the semantics of the mrc statement, whereas it is added only to prevent the compiler from optimizing out the whole asm statement and making get_cr() return garbage.

I would prefer a commit message such as:

Add 'volatile' qualifier to the asm statement in get_cr() so that the statement is not optimized out by the compiler.

Without the 'volatile', get_cr() returns a wrong value which prevents enabling the MMU and later causes a PCIE VA access failure.

If this is fine with you, I'll apply the patch with the comment altered as above.

Amicalement,

Signature · 1 line
Albert.
#2 lore raw

PATCH 1/1 arm: mmu: Add missing volatile for reading SCTLR register

Hello Albert,

Quoted 22 lines
Quoted 8 lines · Alison Wang

When building u-boot with the latest Linaro toolchain, such as gcc-linaro-4.9, u-boot will hang at PCIE init on LS1021A platform. The issue is reported on
http://comments.gmane.org/gmane.linux.linaro.toolchain/5163.

As volatile is missing when reading SCTLR register and SCTLR is set according to the value read from SCTLR, it causes CR_M bit is not set. Then MMU is not enabled, the access to VA for PCIE fails.

This comment seems misleading to me: it appears to imply that the "volatile" qualifier is added to alter the semantics of the mrc statement, whereas it is added only to prevent the compiler from optimizing out the whole asm statement and making get_cr() return garbage.

I would prefer a commit message such as:

Add 'volatile' qualifier to the asm statement in get_cr() so that the statement is not optimized out by the compiler.

Quoted 7 lines

Without the 'volatile', get_cr() returns a wrong value which prevents enabling the MMU and later causes a PCIE VA access failure.

If this is fine with you, I'll apply the patch with the comment altered as above.

[Alison Wang] Your commit message is clear and fine with me, please use it. Thanks a lot. :)

Best Regards,
Alison Wang

#3 lore raw

PATCH 1/1 arm: mmu: Add missing volatile for reading SCTLR register

In reply to Alison Wang

Hello Alison,

Quoted 5 lines · Alison Wang

When building u-boot with the latest Linaro toolchain, such as gcc-linaro-4.9, u-boot will hang at PCIE init on LS1021A platform. The issue is reported on
http://comments.gmane.org/gmane.linux.linaro.toolchain/5163.

Quoted 7 lines

As volatile is missing when reading SCTLR register and SCTLR is set according to the value read from SCTLR, it causes CR_M bit is not set. Then MMU is not enabled, the access to VA for PCIE fails.

This patch will add the missing volatile for reading SCTLR register.

Signed-off-by Alison Wang <[email protected]>

Applied (with the commit message rewritten as discussed), thanks!

Amicalement,

Signature · 1 line
Albert.