| Message ID | 1441765322-33308-1-git-send-email-b18965@freescale.com |
|---|---|
| State | Accepted |
| Delegated to: | Albert ARIBAUD |
| Headers | show |
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,
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
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,
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; }
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(-)