diff mbox series

[v2,2/2] armv8: Disable SYS_DCACHE_OFF & SYS_ICACHE_OFF for ARM64

Message ID 20230822075112.717992-3-bhupesh.sharma@linaro.org
State New
Headers show
Series Disable setting ICACHE and DCACHE off for ARM64 platforms | expand

Commit Message

Bhupesh Sharma Aug. 22, 2023, 7:51 a.m. UTC
Ideally SYS_ICACHE_OFF and SYS_DCACHE_OFF should never be set for
ARM64 (ARMv8) platforms, as it makes booting u-boot slower on these
platforms.

However, if some platforms require ICACHE or DCACHE  to be disabled
only for the smaller SPL stage, we should support such configurations
in u-boot as well.

Compile-tested for:
- qemu arm64
- imx8
- stm32

and run-tested on:
- Qualcomm RB3 platform

Cc: Tom Rini <trini@konsulko.com>
Cc: Simon Glass <sjg@chromium.org>
Cc: Peng Fan <peng.fan@nxp.com>
Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
---
 arch/arm/Kconfig            | 2 ++
 arch/arm/cpu/armv8/Makefile | 4 ++--
 2 files changed, 4 insertions(+), 2 deletions(-)

Comments

Tom Rini Aug. 23, 2023, 2:41 p.m. UTC | #1
On Tue, Aug 22, 2023 at 01:21:12PM +0530, Bhupesh Sharma wrote:

> Ideally SYS_ICACHE_OFF and SYS_DCACHE_OFF should never be set for
> ARM64 (ARMv8) platforms, as it makes booting u-boot slower on these
> platforms.
> 
> However, if some platforms require ICACHE or DCACHE  to be disabled
> only for the smaller SPL stage, we should support such configurations
> in u-boot as well.
> 
> Compile-tested for:
> - qemu arm64
> - imx8
> - stm32
> 
> and run-tested on:
> - Qualcomm RB3 platform
> 
> Cc: Tom Rini <trini@konsulko.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Peng Fan <peng.fan@nxp.com>
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>

Reviewed-by: Tom Rini <trini@konsulko.com>
Tom Rini Aug. 30, 2023, 6:52 p.m. UTC | #2
On Tue, Aug 22, 2023 at 01:21:12PM +0530, Bhupesh Sharma wrote:

> Ideally SYS_ICACHE_OFF and SYS_DCACHE_OFF should never be set for
> ARM64 (ARMv8) platforms, as it makes booting u-boot slower on these
> platforms.
> 
> However, if some platforms require ICACHE or DCACHE  to be disabled
> only for the smaller SPL stage, we should support such configurations
> in u-boot as well.
> 
> Compile-tested for:
> - qemu arm64
> - imx8
> - stm32
> 
> and run-tested on:
> - Qualcomm RB3 platform
> 
> Cc: Tom Rini <trini@konsulko.com>
> Cc: Simon Glass <sjg@chromium.org>
> Cc: Peng Fan <peng.fan@nxp.com>
> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> Reviewed-by: Tom Rini <trini@konsulko.com>
> ---
>  arch/arm/Kconfig            | 2 ++
>  arch/arm/cpu/armv8/Makefile | 4 ++--
>  2 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> index 36ee1e9a3c..92bff715e3 100644
> --- a/arch/arm/Kconfig
> +++ b/arch/arm/Kconfig
> @@ -141,6 +141,7 @@ config THUMB2_KERNEL
>  
>  config SYS_ICACHE_OFF
>  	bool "Do not enable icache"
> +	depends on !ARM64
>  	help
>  	  Do not enable instruction cache in U-Boot.
>  
> @@ -153,6 +154,7 @@ config SPL_SYS_ICACHE_OFF
>  
>  config SYS_DCACHE_OFF
>  	bool "Do not enable dcache"
> +	depends on !ARM64
>  	help
>  	  Do not enable data cache in U-Boot.
>  
> diff --git a/arch/arm/cpu/armv8/Makefile b/arch/arm/cpu/armv8/Makefile
> index bba4f570db..0d0c1728e4 100644
> --- a/arch/arm/cpu/armv8/Makefile
> +++ b/arch/arm/cpu/armv8/Makefile
> @@ -5,13 +5,13 @@
>  
>  extra-y	:= start.o
>  
> +obj-y	+= cache_v8.o
>  obj-y	+= cpu.o
>  ifndef CONFIG_$(SPL_TPL_)TIMER
>  obj-$(CONFIG_SYS_ARCH_TIMER) += generic_timer.o
>  endif
>  ifndef CONFIG_$(SPL_)SYS_DCACHE_OFF
> -obj-y	+= cache_v8.o
> -obj-y	+= cache.o
> +obj-y  += cache.o
>  endif
>  ifdef CONFIG_SPL_BUILD
>  obj-$(CONFIG_ARMV8_SPL_EXCEPTION_VECTORS) += exceptions.o

I'm adding Michal here because this changes the behavior on xilinx
platforms that had the icache off.  I was under the impression that at
the levels U-Boot runs at on aarch64, we just couldn't have
icache/dcache off, but I guess that's wrong?
Michal Simek Aug. 31, 2023, 8:24 a.m. UTC | #3
On 8/30/23 20:52, Tom Rini wrote:
> On Tue, Aug 22, 2023 at 01:21:12PM +0530, Bhupesh Sharma wrote:
> 
>> Ideally SYS_ICACHE_OFF and SYS_DCACHE_OFF should never be set for
>> ARM64 (ARMv8) platforms, as it makes booting u-boot slower on these
>> platforms.
>>
>> However, if some platforms require ICACHE or DCACHE  to be disabled
>> only for the smaller SPL stage, we should support such configurations
>> in u-boot as well.

I am missing the reason why this change should be accepted.

>>
>> Compile-tested for:
>> - qemu arm64

qemu doesn't model caches that's why I don't think it is relevant here.

>> - imx8
>> - stm32
>>
>> and run-tested on:
>> - Qualcomm RB3 platform
>>
>> Cc: Tom Rini <trini@konsulko.com>
>> Cc: Simon Glass <sjg@chromium.org>
>> Cc: Peng Fan <peng.fan@nxp.com>
>> Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
>> Reviewed-by: Tom Rini <trini@konsulko.com>
>> ---
>>   arch/arm/Kconfig            | 2 ++
>>   arch/arm/cpu/armv8/Makefile | 4 ++--
>>   2 files changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
>> index 36ee1e9a3c..92bff715e3 100644
>> --- a/arch/arm/Kconfig
>> +++ b/arch/arm/Kconfig
>> @@ -141,6 +141,7 @@ config THUMB2_KERNEL
>>   
>>   config SYS_ICACHE_OFF
>>   	bool "Do not enable icache"
>> +	depends on !ARM64
>>   	help
>>   	  Do not enable instruction cache in U-Boot.
>>   
>> @@ -153,6 +154,7 @@ config SPL_SYS_ICACHE_OFF
>>   
>>   config SYS_DCACHE_OFF
>>   	bool "Do not enable dcache"
>> +	depends on !ARM64
>>   	help
>>   	  Do not enable data cache in U-Boot.
>>   
>> diff --git a/arch/arm/cpu/armv8/Makefile b/arch/arm/cpu/armv8/Makefile
>> index bba4f570db..0d0c1728e4 100644
>> --- a/arch/arm/cpu/armv8/Makefile
>> +++ b/arch/arm/cpu/armv8/Makefile
>> @@ -5,13 +5,13 @@
>>   
>>   extra-y	:= start.o
>>   
>> +obj-y	+= cache_v8.o
>>   obj-y	+= cpu.o
>>   ifndef CONFIG_$(SPL_TPL_)TIMER
>>   obj-$(CONFIG_SYS_ARCH_TIMER) += generic_timer.o
>>   endif
>>   ifndef CONFIG_$(SPL_)SYS_DCACHE_OFF
>> -obj-y	+= cache_v8.o
>> -obj-y	+= cache.o
>> +obj-y  += cache.o
>>   endif
>>   ifdef CONFIG_SPL_BUILD
>>   obj-$(CONFIG_ARMV8_SPL_EXCEPTION_VECTORS) += exceptions.o
> 
> I'm adding Michal here because this changes the behavior on xilinx
> platforms that had the icache off.  I was under the impression that at
> the levels U-Boot runs at on aarch64, we just couldn't have
> icache/dcache off, but I guess that's wrong?
> 

yes. We are using icache off for mini u-boot configurations for quite a long 
time. I tried to find out any commit why we started to use it but didn't find 
any record. But will ask team to retest that use cases to see if that is working 
or not.
In our case we are using small full U-Boot not any SPL variant.

My biggest issue with the patch is that it is not clear what issue it is trying 
to solve as I have mentioned above.
It just says that ideally icache/dcache should be on. If you want to enforce it 
on your platform you can select it when that platform is selected and don't need 
enforce it for other platforms.

Thanks,
Michal
Tom Rini Aug. 31, 2023, 1:31 p.m. UTC | #4
On Thu, Aug 31, 2023 at 10:24:07AM +0200, Michal Simek wrote:
> 
> 
> On 8/30/23 20:52, Tom Rini wrote:
> > On Tue, Aug 22, 2023 at 01:21:12PM +0530, Bhupesh Sharma wrote:
> > 
> > > Ideally SYS_ICACHE_OFF and SYS_DCACHE_OFF should never be set for
> > > ARM64 (ARMv8) platforms, as it makes booting u-boot slower on these
> > > platforms.
> > > 
> > > However, if some platforms require ICACHE or DCACHE  to be disabled
> > > only for the smaller SPL stage, we should support such configurations
> > > in u-boot as well.
> 
> I am missing the reason why this change should be accepted.
> 
> > > 
> > > Compile-tested for:
> > > - qemu arm64
> 
> qemu doesn't model caches that's why I don't think it is relevant here.
> 
> > > - imx8
> > > - stm32
> > > 
> > > and run-tested on:
> > > - Qualcomm RB3 platform
> > > 
> > > Cc: Tom Rini <trini@konsulko.com>
> > > Cc: Simon Glass <sjg@chromium.org>
> > > Cc: Peng Fan <peng.fan@nxp.com>
> > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> > > Reviewed-by: Tom Rini <trini@konsulko.com>
> > > ---
> > >   arch/arm/Kconfig            | 2 ++
> > >   arch/arm/cpu/armv8/Makefile | 4 ++--
> > >   2 files changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > > index 36ee1e9a3c..92bff715e3 100644
> > > --- a/arch/arm/Kconfig
> > > +++ b/arch/arm/Kconfig
> > > @@ -141,6 +141,7 @@ config THUMB2_KERNEL
> > >   config SYS_ICACHE_OFF
> > >   	bool "Do not enable icache"
> > > +	depends on !ARM64
> > >   	help
> > >   	  Do not enable instruction cache in U-Boot.
> > > @@ -153,6 +154,7 @@ config SPL_SYS_ICACHE_OFF
> > >   config SYS_DCACHE_OFF
> > >   	bool "Do not enable dcache"
> > > +	depends on !ARM64
> > >   	help
> > >   	  Do not enable data cache in U-Boot.
> > > diff --git a/arch/arm/cpu/armv8/Makefile b/arch/arm/cpu/armv8/Makefile
> > > index bba4f570db..0d0c1728e4 100644
> > > --- a/arch/arm/cpu/armv8/Makefile
> > > +++ b/arch/arm/cpu/armv8/Makefile
> > > @@ -5,13 +5,13 @@
> > >   extra-y	:= start.o
> > > +obj-y	+= cache_v8.o
> > >   obj-y	+= cpu.o
> > >   ifndef CONFIG_$(SPL_TPL_)TIMER
> > >   obj-$(CONFIG_SYS_ARCH_TIMER) += generic_timer.o
> > >   endif
> > >   ifndef CONFIG_$(SPL_)SYS_DCACHE_OFF
> > > -obj-y	+= cache_v8.o
> > > -obj-y	+= cache.o
> > > +obj-y  += cache.o
> > >   endif
> > >   ifdef CONFIG_SPL_BUILD
> > >   obj-$(CONFIG_ARMV8_SPL_EXCEPTION_VECTORS) += exceptions.o
> > 
> > I'm adding Michal here because this changes the behavior on xilinx
> > platforms that had the icache off.  I was under the impression that at
> > the levels U-Boot runs at on aarch64, we just couldn't have
> > icache/dcache off, but I guess that's wrong?
> > 
> 
> yes. We are using icache off for mini u-boot configurations for quite a long
> time. I tried to find out any commit why we started to use it but didn't
> find any record. But will ask team to retest that use cases to see if that
> is working or not.
> In our case we are using small full U-Boot not any SPL variant.
> 
> My biggest issue with the patch is that it is not clear what issue it is
> trying to solve as I have mentioned above.
> It just says that ideally icache/dcache should be on. If you want to enforce
> it on your platform you can select it when that platform is selected and
> don't need enforce it for other platforms.

Thanks Michal. Bhupesh, I was wrong in v1, having caches off is allowed,
so you just need to fix issues like part one does where code doesn't
compile when the options are disabled, and then are there further
changes needed for your usecase?
Bhupesh Sharma Aug. 31, 2023, 2:35 p.m. UTC | #5
On Thu, 31 Aug 2023 at 19:01, Tom Rini <trini@konsulko.com> wrote:
>
> On Thu, Aug 31, 2023 at 10:24:07AM +0200, Michal Simek wrote:
> >
> >
> > On 8/30/23 20:52, Tom Rini wrote:
> > > On Tue, Aug 22, 2023 at 01:21:12PM +0530, Bhupesh Sharma wrote:
> > >
> > > > Ideally SYS_ICACHE_OFF and SYS_DCACHE_OFF should never be set for
> > > > ARM64 (ARMv8) platforms, as it makes booting u-boot slower on these
> > > > platforms.
> > > >
> > > > However, if some platforms require ICACHE or DCACHE  to be disabled
> > > > only for the smaller SPL stage, we should support such configurations
> > > > in u-boot as well.
> >
> > I am missing the reason why this change should be accepted.
> >
> > > >
> > > > Compile-tested for:
> > > > - qemu arm64
> >
> > qemu doesn't model caches that's why I don't think it is relevant here.
> >
> > > > - imx8
> > > > - stm32
> > > >
> > > > and run-tested on:
> > > > - Qualcomm RB3 platform
> > > >
> > > > Cc: Tom Rini <trini@konsulko.com>
> > > > Cc: Simon Glass <sjg@chromium.org>
> > > > Cc: Peng Fan <peng.fan@nxp.com>
> > > > Signed-off-by: Bhupesh Sharma <bhupesh.sharma@linaro.org>
> > > > Reviewed-by: Tom Rini <trini@konsulko.com>
> > > > ---
> > > >   arch/arm/Kconfig            | 2 ++
> > > >   arch/arm/cpu/armv8/Makefile | 4 ++--
> > > >   2 files changed, 4 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
> > > > index 36ee1e9a3c..92bff715e3 100644
> > > > --- a/arch/arm/Kconfig
> > > > +++ b/arch/arm/Kconfig
> > > > @@ -141,6 +141,7 @@ config THUMB2_KERNEL
> > > >   config SYS_ICACHE_OFF
> > > >           bool "Do not enable icache"
> > > > + depends on !ARM64
> > > >           help
> > > >             Do not enable instruction cache in U-Boot.
> > > > @@ -153,6 +154,7 @@ config SPL_SYS_ICACHE_OFF
> > > >   config SYS_DCACHE_OFF
> > > >           bool "Do not enable dcache"
> > > > + depends on !ARM64
> > > >           help
> > > >             Do not enable data cache in U-Boot.
> > > > diff --git a/arch/arm/cpu/armv8/Makefile b/arch/arm/cpu/armv8/Makefile
> > > > index bba4f570db..0d0c1728e4 100644
> > > > --- a/arch/arm/cpu/armv8/Makefile
> > > > +++ b/arch/arm/cpu/armv8/Makefile
> > > > @@ -5,13 +5,13 @@
> > > >   extra-y := start.o
> > > > +obj-y    += cache_v8.o
> > > >   obj-y   += cpu.o
> > > >   ifndef CONFIG_$(SPL_TPL_)TIMER
> > > >   obj-$(CONFIG_SYS_ARCH_TIMER) += generic_timer.o
> > > >   endif
> > > >   ifndef CONFIG_$(SPL_)SYS_DCACHE_OFF
> > > > -obj-y    += cache_v8.o
> > > > -obj-y    += cache.o
> > > > +obj-y  += cache.o
> > > >   endif
> > > >   ifdef CONFIG_SPL_BUILD
> > > >   obj-$(CONFIG_ARMV8_SPL_EXCEPTION_VECTORS) += exceptions.o
> > >
> > > I'm adding Michal here because this changes the behavior on xilinx
> > > platforms that had the icache off.  I was under the impression that at
> > > the levels U-Boot runs at on aarch64, we just couldn't have
> > > icache/dcache off, but I guess that's wrong?
> > >
> >
> > yes. We are using icache off for mini u-boot configurations for quite a long
> > time. I tried to find out any commit why we started to use it but didn't
> > find any record. But will ask team to retest that use cases to see if that
> > is working or not.
> > In our case we are using small full U-Boot not any SPL variant.
> >
> > My biggest issue with the patch is that it is not clear what issue it is
> > trying to solve as I have mentioned above.
> > It just says that ideally icache/dcache should be on. If you want to enforce
> > it on your platform you can select it when that platform is selected and
> > don't need enforce it for other platforms.
>
> Thanks Michal. Bhupesh, I was wrong in v1, having caches off is allowed,
> so you just need to fix issues like part one does where code doesn't
> compile when the options are disabled, and then are there further
> changes needed for your usecase?

Sure Tom. I will go back to sending v1 as v3 with some minor changes.

Thanks,
Bhupesh
diff mbox series

Patch

diff --git a/arch/arm/Kconfig b/arch/arm/Kconfig
index 36ee1e9a3c..92bff715e3 100644
--- a/arch/arm/Kconfig
+++ b/arch/arm/Kconfig
@@ -141,6 +141,7 @@  config THUMB2_KERNEL
 
 config SYS_ICACHE_OFF
 	bool "Do not enable icache"
+	depends on !ARM64
 	help
 	  Do not enable instruction cache in U-Boot.
 
@@ -153,6 +154,7 @@  config SPL_SYS_ICACHE_OFF
 
 config SYS_DCACHE_OFF
 	bool "Do not enable dcache"
+	depends on !ARM64
 	help
 	  Do not enable data cache in U-Boot.
 
diff --git a/arch/arm/cpu/armv8/Makefile b/arch/arm/cpu/armv8/Makefile
index bba4f570db..0d0c1728e4 100644
--- a/arch/arm/cpu/armv8/Makefile
+++ b/arch/arm/cpu/armv8/Makefile
@@ -5,13 +5,13 @@ 
 
 extra-y	:= start.o
 
+obj-y	+= cache_v8.o
 obj-y	+= cpu.o
 ifndef CONFIG_$(SPL_TPL_)TIMER
 obj-$(CONFIG_SYS_ARCH_TIMER) += generic_timer.o
 endif
 ifndef CONFIG_$(SPL_)SYS_DCACHE_OFF
-obj-y	+= cache_v8.o
-obj-y	+= cache.o
+obj-y  += cache.o
 endif
 ifdef CONFIG_SPL_BUILD
 obj-$(CONFIG_ARMV8_SPL_EXCEPTION_VECTORS) += exceptions.o