[U-Boot] [PATCH] arm: mmu: Add missing volatile for reading SCTLR register

[U-Boot] [PATCH] arm: mmu: Add missing volatile for reading SCTLR register

[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 patch will add the missing volatile for reading SCTLR register.

Signed-off-by: Alison Wang <alison.wang@freescale.com>
---
 arch/arm/include/asm/system.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/arch/arm/include/asm/system.h b/arch/arm/include/asm/system.h
index 89f2294..f5096dc 100644
--- a/arch/arm/include/asm/system.h
+++ b/arch/arm/include/asm/system.h
@@ -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;
 }
 
-- 
2.1.0.27.g96db324

[U-Boot] [PATCH] arm: mmu: Add missing volatile for reading SCTLR register

[Albert ARIBAUD]

Hello Alison,

Sorry for the late comment.

On Wed, 9 Sep 2015 10:22:02 +0800, Alison Wang <b18965@freescale.com>
wrote:
> 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,
-- 
Albert.

[U-Boot] [PATCH] arm: mmu: Add missing volatile for reading SCTLR register

[Huan Wang]

Hello Albert,
 
> On Wed, 9 Sep 2015 10:22:02 +0800, Alison Wang <b18965@freescale.com>
> wrote:
> > 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.
> 
[Alison Wang] Your commit message is clear and fine with me, please use it.
Thanks a lot. :)


Best Regards,
Alison Wang

[U-Boot] [PATCH] arm: mmu: Add missing volatile for reading SCTLR register

[Albert ARIBAUD]

Hello Alison,

On Wed, 9 Sep 2015 10:22:02 +0800, Alison Wang <b18965@freescale.com>
wrote:
> 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 <alison.wang@freescale.com>

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

Amicalement,
-- 
Albert.