| Message ID | 1405703385-14580-3-git-send-email-siarhei.siamashka@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ian Campbell |
| Headers | show |
Hello Siarhei, On 18-07-14 19:09, Siarhei Siamashka wrote: > This is needed to have feature parity with the normal boot mode, > where the L2EN bit in the CP15 Auxiliary Control Register is set > by the BROM code right from the start. > > If this is not done, the Linux system ends up booted with the L2 cache > disabled. > I don't know a single about the sunxi, but shouldn't linux be patched instead. The commit message seems to indicate it is not an u-boot issue. Regards, Jeroen
Hi, On 07/18/2014 07:09 PM, Siarhei Siamashka wrote: > This is needed to have feature parity with the normal boot mode, > where the L2EN bit in the CP15 Auxiliary Control Register is set > by the BROM code right from the start. > > If this is not done, the Linux system ends up booted with the L2 cache > disabled. > > Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com> > --- > arch/arm/cpu/armv7/sunxi/board.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/arch/arm/cpu/armv7/sunxi/board.c b/arch/arm/cpu/armv7/sunxi/board.c > index 49c9448..86cf4c9 100644 > --- a/arch/arm/cpu/armv7/sunxi/board.c > +++ b/arch/arm/cpu/armv7/sunxi/board.c > @@ -69,6 +69,18 @@ void s_init(void) > "mcr p15, 0, r0, c1, c0, 1\n"); > #endif > > +#if defined(CONFIG_SPL_FEL) && (defined(CONFIG_SUN4I) || defined(CONFIG_SUN5I)) > + /* For ARM Cortex-A8 based hardware (sun4i and sun5i), the L2EN bit is > + * set by the BROM code in the "normal" mode, but not in the "FEL" mode. > + * Here we fix this inconsistency in the Auxiliary Ctl reg by also > + * setting the missing L2EN bit. > + */ > + asm volatile( > + "mrc p15, 0, r0, c1, c0, 1\n" > + "orr r0, r0, #2\n" > + "mcr p15, 0, r0, c1, c0, 1\n" : : : "r0"); > +#endif > + Wouldn't it be better to do this in the start_fel.S file you've introduced in the first patch of this series ? Regards, Hans
On Sat, 2014-07-19 at 13:20 +0200, Hans de Goede wrote: > Hi, > > On 07/18/2014 07:09 PM, Siarhei Siamashka wrote: > > This is needed to have feature parity with the normal boot mode, > > where the L2EN bit in the CP15 Auxiliary Control Register is set > > by the BROM code right from the start. > > > > If this is not done, the Linux system ends up booted with the L2 cache > > disabled. > > > > Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com> > > --- > > arch/arm/cpu/armv7/sunxi/board.c | 12 ++++++++++++ > > 1 file changed, 12 insertions(+) > > > > diff --git a/arch/arm/cpu/armv7/sunxi/board.c b/arch/arm/cpu/armv7/sunxi/board.c > > index 49c9448..86cf4c9 100644 > > --- a/arch/arm/cpu/armv7/sunxi/board.c > > +++ b/arch/arm/cpu/armv7/sunxi/board.c > > @@ -69,6 +69,18 @@ void s_init(void) > > "mcr p15, 0, r0, c1, c0, 1\n"); > > #endif > > > > +#if defined(CONFIG_SPL_FEL) && (defined(CONFIG_SUN4I) || defined(CONFIG_SUN5I)) > > + /* For ARM Cortex-A8 based hardware (sun4i and sun5i), the L2EN bit is > > + * set by the BROM code in the "normal" mode, but not in the "FEL" mode. > > + * Here we fix this inconsistency in the Auxiliary Ctl reg by also > > + * setting the missing L2EN bit. > > + */ > > + asm volatile( > > + "mrc p15, 0, r0, c1, c0, 1\n" > > + "orr r0, r0, #2\n" > > + "mcr p15, 0, r0, c1, c0, 1\n" : : : "r0"); > > +#endif > > + > > Wouldn't it be better to do this in the start_fel.S file you've introduced in > the first patch of this series ? That wouldn't remove the need for the ifdef if that's what you are thinking since it still needs to be sun4i/sun5i specific. I think doing it here is in keeping with setting ACTLR.SMP on the sun6i/sun7i platforms which is just above the context here. Acked-by: Ian Campbell <ijc@hellion.org.uk> Although I also wouldn't object to doing it in in start_fel.S if that is preferred. I expect this needs to be done on secondary processors. Need to keep that in mind if/when someone works on PSCI for sun[45]i. Ian.
On Fri, 2014-07-18 at 20:47 +0200, Jeroen Hofstee wrote: > Hello Siarhei, > > On 18-07-14 19:09, Siarhei Siamashka wrote: > > This is needed to have feature parity with the normal boot mode, > > where the L2EN bit in the CP15 Auxiliary Control Register is set > > by the BROM code right from the start. > > > > If this is not done, the Linux system ends up booted with the L2 cache > > disabled. > > > > I don't know a single about the sunxi, but shouldn't linux > be patched instead. The commit message seems to indicate > it is not an u-boot issue. The ACTLR may not be writeable from NS mode so it has to be setup in the bootloader before dropping to NS mode. In any case I think these sorts of low level platform specific details are the sort of thing which the bootloader probably ought to be setting up. Ian
On Mon, 2014-07-21 at 19:39 +0100, Ian Campbell wrote: > > I expect this needs to be done on secondary processors. Need to keep > that in mind if/when someone works on PSCI for sun[45]i. Except as Tom points out on IRC, sun[45]i are both single core... Oops! Ian.
Hello Ian, On 21-07-14 22:07, Ian Campbell wrote: > On Fri, 2014-07-18 at 20:47 +0200, Jeroen Hofstee wrote: >> Hello Siarhei, >> >> On 18-07-14 19:09, Siarhei Siamashka wrote: >>> This is needed to have feature parity with the normal boot mode, >>> where the L2EN bit in the CP15 Auxiliary Control Register is set >>> by the BROM code right from the start. >>> >>> If this is not done, the Linux system ends up booted with the L2 cache >>> disabled. >>> >> I don't know a single about the sunxi, but shouldn't linux >> be patched instead. The commit message seems to indicate >> it is not an u-boot issue. > The ACTLR may not be writeable from NS mode so it has to be setup in the > bootloader before dropping to NS mode. mmm, I guess there is something wrong with the boot sequence if the kernel itself can't access raw hw. > In any case I think these sorts of low level platform specific details > are the sort of thing which the bootloader probably ought to be setting > up. No, u-boot tries not to touch anything it doesn't use and, if anything disables it after use. Hence, this seems like a kernel bug and nothing to do with u-boot. They should be independent, iow a kernel should not rely on u-boot setting thing up, that is a bug. Regards, Jeroen
On Mon, 2014-07-21 at 22:39 +0200, Jeroen Hofstee wrote: > Hello Ian, > > On 21-07-14 22:07, Ian Campbell wrote: > > On Fri, 2014-07-18 at 20:47 +0200, Jeroen Hofstee wrote: > >> Hello Siarhei, > >> > >> On 18-07-14 19:09, Siarhei Siamashka wrote: > >>> This is needed to have feature parity with the normal boot mode, > >>> where the L2EN bit in the CP15 Auxiliary Control Register is set > >>> by the BROM code right from the start. > >>> > >>> If this is not done, the Linux system ends up booted with the L2 cache > >>> disabled. > >>> > >> I don't know a single about the sunxi, but shouldn't linux > >> be patched instead. The commit message seems to indicate > >> it is not an u-boot issue. > > The ACTLR may not be writeable from NS mode so it has to be setup in the > > bootloader before dropping to NS mode. > mmm, I guess there is something wrong with the boot sequence > if the kernel itself can't access raw hw. Do you know what ARM Secure and Non-Secure worlds are? The kernel expects to be launched in NS mode and simply cannot access this register. This is a feature not a bug. Ian.
On Mon, 21 Jul 2014 21:59:51 +0100 Ian Campbell <ijc@hellion.org.uk> wrote: > On Mon, 2014-07-21 at 22:39 +0200, Jeroen Hofstee wrote: > > Hello Ian, > > > > On 21-07-14 22:07, Ian Campbell wrote: > > > On Fri, 2014-07-18 at 20:47 +0200, Jeroen Hofstee wrote: > > >> Hello Siarhei, > > >> > > >> On 18-07-14 19:09, Siarhei Siamashka wrote: > > >>> This is needed to have feature parity with the normal boot mode, > > >>> where the L2EN bit in the CP15 Auxiliary Control Register is set > > >>> by the BROM code right from the start. > > >>> > > >>> If this is not done, the Linux system ends up booted with the L2 cache > > >>> disabled. > > >>> > > >> I don't know a single about the sunxi, but shouldn't linux > > >> be patched instead. The commit message seems to indicate > > >> it is not an u-boot issue. > > > The ACTLR may not be writeable from NS mode so it has to be setup in the > > > bootloader before dropping to NS mode. > > mmm, I guess there is something wrong with the boot sequence > > if the kernel itself can't access raw hw. > > Do you know what ARM Secure and Non-Secure worlds are? > > The kernel expects to be launched in NS mode and simply cannot access > this register. This is a feature not a bug. Just curious. Is there a modern consensus about how this all is supposed to be done nowadays? The last time I read anything about this subject was the following longish and already old discussion thread (which has probably already lost relevance): http://lists.linaro.org/pipermail/boot-architecture/2011-August/000060.html Since the Allwinner BROM does not forcefully drop us to the non-secure mode, we have the absolute freedom of choice and may implement any policy.
On Fri, 2014-07-25 at 03:21 +0300, Siarhei Siamashka wrote: > On Mon, 21 Jul 2014 21:59:51 +0100 > Ian Campbell <ijc@hellion.org.uk> wrote: > > > On Mon, 2014-07-21 at 22:39 +0200, Jeroen Hofstee wrote: > > > Hello Ian, > > > > > > On 21-07-14 22:07, Ian Campbell wrote: > > > > On Fri, 2014-07-18 at 20:47 +0200, Jeroen Hofstee wrote: > > > >> Hello Siarhei, > > > >> > > > >> On 18-07-14 19:09, Siarhei Siamashka wrote: > > > >>> This is needed to have feature parity with the normal boot mode, > > > >>> where the L2EN bit in the CP15 Auxiliary Control Register is set > > > >>> by the BROM code right from the start. > > > >>> > > > >>> If this is not done, the Linux system ends up booted with the L2 cache > > > >>> disabled. > > > >>> > > > >> I don't know a single about the sunxi, but shouldn't linux > > > >> be patched instead. The commit message seems to indicate > > > >> it is not an u-boot issue. > > > > The ACTLR may not be writeable from NS mode so it has to be setup in the > > > > bootloader before dropping to NS mode. > > > mmm, I guess there is something wrong with the boot sequence > > > if the kernel itself can't access raw hw. > > > > Do you know what ARM Secure and Non-Secure worlds are? > > > > The kernel expects to be launched in NS mode and simply cannot access > > this register. This is a feature not a bug. > > Just curious. Is there a modern consensus about how this all is > supposed to be done nowadays? The kernel's Booting.txt strongly encourages you to enter in HYP mode if it is available, which implies NS mode. This a basic requirement to use virtualisation (Xen or KVM etc). I believe that the general consensus is to run in NS mode on newer platforms by default. FWIW on v8 arm64 Linux documentation requires NS mode. > The last time I read anything about this subject was the following > longish and already old discussion thread (which has probably > already lost relevance): > http://lists.linaro.org/pipermail/boot-architecture/2011-August/000060.html > > Since the Allwinner BROM does not forcefully drop us to the non-secure > mode, we have the absolute freedom of choice and may implement any > policy. We do, and we should implement PSCI and NS boot for kernels. Ian.
diff --git a/arch/arm/cpu/armv7/sunxi/board.c b/arch/arm/cpu/armv7/sunxi/board.c index 49c9448..86cf4c9 100644 --- a/arch/arm/cpu/armv7/sunxi/board.c +++ b/arch/arm/cpu/armv7/sunxi/board.c @@ -69,6 +69,18 @@ void s_init(void) "mcr p15, 0, r0, c1, c0, 1\n"); #endif +#if defined(CONFIG_SPL_FEL) && (defined(CONFIG_SUN4I) || defined(CONFIG_SUN5I)) + /* For ARM Cortex-A8 based hardware (sun4i and sun5i), the L2EN bit is + * set by the BROM code in the "normal" mode, but not in the "FEL" mode. + * Here we fix this inconsistency in the Auxiliary Ctl reg by also + * setting the missing L2EN bit. + */ + asm volatile( + "mrc p15, 0, r0, c1, c0, 1\n" + "orr r0, r0, #2\n" + "mcr p15, 0, r0, c1, c0, 1\n" : : : "r0"); +#endif + clock_init(); timer_init(); gpio_init();
This is needed to have feature parity with the normal boot mode, where the L2EN bit in the CP15 Auxiliary Control Register is set by the BROM code right from the start. If this is not done, the Linux system ends up booted with the L2 cache disabled. Signed-off-by: Siarhei Siamashka <siarhei.siamashka@gmail.com> --- arch/arm/cpu/armv7/sunxi/board.c | 12 ++++++++++++ 1 file changed, 12 insertions(+)