| Message ID | 20230822075112.717992-3-bhupesh.sharma@linaro.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Disable setting ICACHE and DCACHE off for ARM64 platforms | expand |
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>
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?
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
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?
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 --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
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(-)