| Message ID | 1582115146-28658-12-git-send-email-chee.hong.ang@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable ARM Trusted Firmware for U-Boot | expand |
On 2/19/20 1:25 PM, chee.hong.ang at intel.com wrote: > From: Chee Hong Ang <chee.hong.ang at intel.com> > > Allow clock manager driver to access the System Manager's Boot > Scratch Register 0 in non-secure mode (EL2) on SoC 64bits platform. > > Signed-off-by: Chee Hong Ang <chee.hong.ang at intel.com> > --- > arch/arm/mach-socfpga/clock_manager_agilex.c | 5 +++-- > arch/arm/mach-socfpga/clock_manager_s10.c | 5 +++-- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/mach-socfpga/clock_manager_agilex.c b/arch/arm/mach-socfpga/clock_manager_agilex.c > index 4ee2b7b..e5a0998 100644 > --- a/arch/arm/mach-socfpga/clock_manager_agilex.c > +++ b/arch/arm/mach-socfpga/clock_manager_agilex.c > @@ -12,6 +12,7 @@ > #include <asm/arch/system_manager.h> > #include <asm/io.h> > #include <dt-bindings/clock/agilex-clock.h> > +#include <asm/arch/secure_reg_helper.h> > > DECLARE_GLOBAL_DATA_PTR; > > @@ -65,8 +66,8 @@ unsigned int cm_get_l4_sys_free_clk_hz(void) > > u32 cm_get_qspi_controller_clk_hz(void) > { > - return readl(socfpga_get_sysmgr_addr() + > - SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > + SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > } > > void cm_print_clock_quick_summary(void) > diff --git a/arch/arm/mach-socfpga/clock_manager_s10.c b/arch/arm/mach-socfpga/clock_manager_s10.c > index 05e4212..02578cc 100644 > --- a/arch/arm/mach-socfpga/clock_manager_s10.c > +++ b/arch/arm/mach-socfpga/clock_manager_s10.c > @@ -9,6 +9,7 @@ > #include <asm/arch/clock_manager.h> > #include <asm/arch/handoff_s10.h> > #include <asm/arch/system_manager.h> > +#include <asm/arch/secure_reg_helper.h> > > DECLARE_GLOBAL_DATA_PTR; > > @@ -385,8 +386,8 @@ unsigned int cm_get_l4_sp_clk_hz(void) > > unsigned int cm_get_qspi_controller_clk_hz(void) > { > - return readl(socfpga_get_sysmgr_addr() + > - SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > + SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > } > > unsigned int cm_get_spi_controller_clk_hz(void) Shouldn't the IO accessors already provide the necessary abstraction ?
> On 2/19/20 1:25 PM, chee.hong.ang at intel.com wrote: > > From: Chee Hong Ang <chee.hong.ang at intel.com> > > > > Allow clock manager driver to access the System Manager's Boot Scratch > > Register 0 in non-secure mode (EL2) on SoC 64bits platform. > > > > Signed-off-by: Chee Hong Ang <chee.hong.ang at intel.com> > > --- > > arch/arm/mach-socfpga/clock_manager_agilex.c | 5 +++-- > > arch/arm/mach-socfpga/clock_manager_s10.c | 5 +++-- > > 2 files changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/arch/arm/mach-socfpga/clock_manager_agilex.c > > b/arch/arm/mach-socfpga/clock_manager_agilex.c > > index 4ee2b7b..e5a0998 100644 > > --- a/arch/arm/mach-socfpga/clock_manager_agilex.c > > +++ b/arch/arm/mach-socfpga/clock_manager_agilex.c > > @@ -12,6 +12,7 @@ > > #include <asm/arch/system_manager.h> > > #include <asm/io.h> > > #include <dt-bindings/clock/agilex-clock.h> > > +#include <asm/arch/secure_reg_helper.h> > > > > DECLARE_GLOBAL_DATA_PTR; > > > > @@ -65,8 +66,8 @@ unsigned int cm_get_l4_sys_free_clk_hz(void) > > > > u32 cm_get_qspi_controller_clk_hz(void) > > { > > - return readl(socfpga_get_sysmgr_addr() + > > - SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > > + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > > + > SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > > } > > > > void cm_print_clock_quick_summary(void) > > diff --git a/arch/arm/mach-socfpga/clock_manager_s10.c > > b/arch/arm/mach-socfpga/clock_manager_s10.c > > index 05e4212..02578cc 100644 > > --- a/arch/arm/mach-socfpga/clock_manager_s10.c > > +++ b/arch/arm/mach-socfpga/clock_manager_s10.c > > @@ -9,6 +9,7 @@ > > #include <asm/arch/clock_manager.h> > > #include <asm/arch/handoff_s10.h> > > #include <asm/arch/system_manager.h> > > +#include <asm/arch/secure_reg_helper.h> > > > > DECLARE_GLOBAL_DATA_PTR; > > > > @@ -385,8 +386,8 @@ unsigned int cm_get_l4_sp_clk_hz(void) > > > > unsigned int cm_get_qspi_controller_clk_hz(void) > > { > > - return readl(socfpga_get_sysmgr_addr() + > > - SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > > + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > > + > SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > > } > > > > unsigned int cm_get_spi_controller_clk_hz(void) > > Shouldn't the IO accessors already provide the necessary abstraction ? This function accesses the system manager registers, therefore it is required to call the secure register read function to make sure it still can access the system manager register if it's running EL2 (non-secure).
On 2/20/20 3:32 AM, Ang, Chee Hong wrote: >> On 2/19/20 1:25 PM, chee.hong.ang at intel.com wrote: >>> From: Chee Hong Ang <chee.hong.ang at intel.com> >>> >>> Allow clock manager driver to access the System Manager's Boot Scratch >>> Register 0 in non-secure mode (EL2) on SoC 64bits platform. >>> >>> Signed-off-by: Chee Hong Ang <chee.hong.ang at intel.com> >>> --- >>> arch/arm/mach-socfpga/clock_manager_agilex.c | 5 +++-- >>> arch/arm/mach-socfpga/clock_manager_s10.c | 5 +++-- >>> 2 files changed, 6 insertions(+), 4 deletions(-) >>> >>> diff --git a/arch/arm/mach-socfpga/clock_manager_agilex.c >>> b/arch/arm/mach-socfpga/clock_manager_agilex.c >>> index 4ee2b7b..e5a0998 100644 >>> --- a/arch/arm/mach-socfpga/clock_manager_agilex.c >>> +++ b/arch/arm/mach-socfpga/clock_manager_agilex.c >>> @@ -12,6 +12,7 @@ >>> #include <asm/arch/system_manager.h> >>> #include <asm/io.h> >>> #include <dt-bindings/clock/agilex-clock.h> >>> +#include <asm/arch/secure_reg_helper.h> >>> >>> DECLARE_GLOBAL_DATA_PTR; >>> >>> @@ -65,8 +66,8 @@ unsigned int cm_get_l4_sys_free_clk_hz(void) >>> >>> u32 cm_get_qspi_controller_clk_hz(void) >>> { >>> - return readl(socfpga_get_sysmgr_addr() + >>> - SYSMGR_SOC64_BOOT_SCRATCH_COLD0); >>> + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + >>> + >> SYSMGR_SOC64_BOOT_SCRATCH_COLD0); >>> } >>> >>> void cm_print_clock_quick_summary(void) >>> diff --git a/arch/arm/mach-socfpga/clock_manager_s10.c >>> b/arch/arm/mach-socfpga/clock_manager_s10.c >>> index 05e4212..02578cc 100644 >>> --- a/arch/arm/mach-socfpga/clock_manager_s10.c >>> +++ b/arch/arm/mach-socfpga/clock_manager_s10.c >>> @@ -9,6 +9,7 @@ >>> #include <asm/arch/clock_manager.h> >>> #include <asm/arch/handoff_s10.h> >>> #include <asm/arch/system_manager.h> >>> +#include <asm/arch/secure_reg_helper.h> >>> >>> DECLARE_GLOBAL_DATA_PTR; >>> >>> @@ -385,8 +386,8 @@ unsigned int cm_get_l4_sp_clk_hz(void) >>> >>> unsigned int cm_get_qspi_controller_clk_hz(void) >>> { >>> - return readl(socfpga_get_sysmgr_addr() + >>> - SYSMGR_SOC64_BOOT_SCRATCH_COLD0); >>> + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + >>> + >> SYSMGR_SOC64_BOOT_SCRATCH_COLD0); >>> } >>> >>> unsigned int cm_get_spi_controller_clk_hz(void) >> >> Shouldn't the IO accessors already provide the necessary abstraction ? > This function accesses the system manager registers, therefore it is required > to call the secure register read function to make sure it still can access > the system manager register if it's running EL2 (non-secure). But shouldn't the standard IO accessors handle that transparently ? What does Linux do ?
> -----Original Message----- > From: Marek Vasut <marex at denx.de> > Sent: Friday, February 21, 2020 12:48 AM > To: Ang, Chee Hong <chee.hong.ang at intel.com>; u-boot at lists.denx.de > Cc: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com>; See, Chin Liang > <chin.liang.see at intel.com>; Tan, Ley Foon <ley.foon.tan at intel.com>; > Westergreen, Dalon <dalon.westergreen at intel.com>; Gong, Richard > <richard.gong at intel.com>; Tom Rini <trini at konsulko.com>; Michal Simek > <michal.simek at xilinx.com> > Subject: Re: [PATCH v2 11/21] arm: socfpga: Secure register access for clock > manager (SoC 64bits) > > On 2/20/20 3:32 AM, Ang, Chee Hong wrote: > >> On 2/19/20 1:25 PM, chee.hong.ang at intel.com wrote: > >>> From: Chee Hong Ang <chee.hong.ang at intel.com> > >>> > >>> Allow clock manager driver to access the System Manager's Boot > >>> Scratch Register 0 in non-secure mode (EL2) on SoC 64bits platform. > >>> > >>> Signed-off-by: Chee Hong Ang <chee.hong.ang at intel.com> > >>> --- > >>> arch/arm/mach-socfpga/clock_manager_agilex.c | 5 +++-- > >>> arch/arm/mach-socfpga/clock_manager_s10.c | 5 +++-- > >>> 2 files changed, 6 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/arch/arm/mach-socfpga/clock_manager_agilex.c > >>> b/arch/arm/mach-socfpga/clock_manager_agilex.c > >>> index 4ee2b7b..e5a0998 100644 > >>> --- a/arch/arm/mach-socfpga/clock_manager_agilex.c > >>> +++ b/arch/arm/mach-socfpga/clock_manager_agilex.c > >>> @@ -12,6 +12,7 @@ > >>> #include <asm/arch/system_manager.h> #include <asm/io.h> #include > >>> <dt-bindings/clock/agilex-clock.h> > >>> +#include <asm/arch/secure_reg_helper.h> > >>> > >>> DECLARE_GLOBAL_DATA_PTR; > >>> > >>> @@ -65,8 +66,8 @@ unsigned int cm_get_l4_sys_free_clk_hz(void) > >>> > >>> u32 cm_get_qspi_controller_clk_hz(void) > >>> { > >>> - return readl(socfpga_get_sysmgr_addr() + > >>> - SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > >>> + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > >>> + > >> SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > >>> } > >>> > >>> void cm_print_clock_quick_summary(void) > >>> diff --git a/arch/arm/mach-socfpga/clock_manager_s10.c > >>> b/arch/arm/mach-socfpga/clock_manager_s10.c > >>> index 05e4212..02578cc 100644 > >>> --- a/arch/arm/mach-socfpga/clock_manager_s10.c > >>> +++ b/arch/arm/mach-socfpga/clock_manager_s10.c > >>> @@ -9,6 +9,7 @@ > >>> #include <asm/arch/clock_manager.h> #include > >>> <asm/arch/handoff_s10.h> #include <asm/arch/system_manager.h> > >>> +#include <asm/arch/secure_reg_helper.h> > >>> > >>> DECLARE_GLOBAL_DATA_PTR; > >>> > >>> @@ -385,8 +386,8 @@ unsigned int cm_get_l4_sp_clk_hz(void) > >>> > >>> unsigned int cm_get_qspi_controller_clk_hz(void) > >>> { > >>> - return readl(socfpga_get_sysmgr_addr() + > >>> - SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > >>> + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > >>> + > >> SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > >>> } > >>> > >>> unsigned int cm_get_spi_controller_clk_hz(void) > >> > >> Shouldn't the IO accessors already provide the necessary abstraction ? > > This function accesses the system manager registers, therefore it is > > required to call the secure register read function to make sure it > > still can access the system manager register if it's running EL2 (non-secure). > > But shouldn't the standard IO accessors handle that transparently ? Regarding this standard IO accessors, please refer to my reply in another email thread. > What does Linux do ? Currently, Linux run in EL1 (non-secure), it will crash if it's accessing the secure zones directly with standard memory I/O functions provided by kernel. It goes through the ATF by making SMC/PSCI calls to ATF to access the secure zones. Just Like what we did in this patchset. The only difference is kernel code always access those secure zones by making SMC/PSCI calls but U-Boot code get to choose the SMC/PSCI calls or standard I/O accessors in compile time because same code base in U-Boot may run in EL2 or EL3 depending on whether the code is built for SPL (EL3) or U-Boot proper without ATF (EL2).
> From: Chee Hong Ang <chee.hong.ang at intel.com> > > Allow clock manager driver to access the System Manager's Boot Scratch > Register 0 in non-secure mode (EL2) on SoC 64bits platform. > > Signed-off-by: Chee Hong Ang <chee.hong.ang at intel.com> > --- > arch/arm/mach-socfpga/clock_manager_agilex.c | 5 +++-- > arch/arm/mach-socfpga/clock_manager_s10.c | 5 +++-- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/mach-socfpga/clock_manager_agilex.c b/arch/arm/mach- > socfpga/clock_manager_agilex.c > index 4ee2b7b..e5a0998 100644 > --- a/arch/arm/mach-socfpga/clock_manager_agilex.c > +++ b/arch/arm/mach-socfpga/clock_manager_agilex.c > @@ -12,6 +12,7 @@ > #include <asm/arch/system_manager.h> > #include <asm/io.h> > #include <dt-bindings/clock/agilex-clock.h> > +#include <asm/arch/secure_reg_helper.h> > > DECLARE_GLOBAL_DATA_PTR; > > @@ -65,8 +66,8 @@ unsigned int cm_get_l4_sys_free_clk_hz(void) > > u32 cm_get_qspi_controller_clk_hz(void) > { > - return readl(socfpga_get_sysmgr_addr() + > - SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > + > SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > } > > void cm_print_clock_quick_summary(void) > diff --git a/arch/arm/mach-socfpga/clock_manager_s10.c b/arch/arm/mach- > socfpga/clock_manager_s10.c > index 05e4212..02578cc 100644 > --- a/arch/arm/mach-socfpga/clock_manager_s10.c > +++ b/arch/arm/mach-socfpga/clock_manager_s10.c > @@ -9,6 +9,7 @@ > #include <asm/arch/clock_manager.h> > #include <asm/arch/handoff_s10.h> > #include <asm/arch/system_manager.h> > +#include <asm/arch/secure_reg_helper.h> > > DECLARE_GLOBAL_DATA_PTR; > > @@ -385,8 +386,8 @@ unsigned int cm_get_l4_sp_clk_hz(void) > > unsigned int cm_get_qspi_controller_clk_hz(void) > { > - return readl(socfpga_get_sysmgr_addr() + > - SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > + > SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > } > > unsigned int cm_get_spi_controller_clk_hz(void) > -- > 2.7.4 SPL reads the clock info from handoff table (OCRAM) and write the clock info into the System Manager's boot scratch register. U-Boot proper will read from the System Manager's boot scratch register to get the clock info in case the handoff table (OCRAM) is no longer available. After some investigations, the handoff table in OCRAM should be preserved for warm boot. In other words, this handoff table should be left untouched. SPL and U-Boot should directly read the clock info from handoff table in OCRAM. Therefore, U-Boot proper no longer need to read the clock info from System Manager's boot scratch register (secure zone) from non-secure world (EL2).
Ang, Chee Hong <chee.hong.ang at intel.com> schrieb am Sa., 22. Feb. 2020, 06:30: > > From: Chee Hong Ang <chee.hong.ang at intel.com> > > > > Allow clock manager driver to access the System Manager's Boot Scratch > > Register 0 in non-secure mode (EL2) on SoC 64bits platform. > > > > Signed-off-by: Chee Hong Ang <chee.hong.ang at intel.com> > > --- > > arch/arm/mach-socfpga/clock_manager_agilex.c | 5 +++-- > > arch/arm/mach-socfpga/clock_manager_s10.c | 5 +++-- > > 2 files changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/arch/arm/mach-socfpga/clock_manager_agilex.c > b/arch/arm/mach- > > socfpga/clock_manager_agilex.c > > index 4ee2b7b..e5a0998 100644 > > --- a/arch/arm/mach-socfpga/clock_manager_agilex.c > > +++ b/arch/arm/mach-socfpga/clock_manager_agilex.c > > @@ -12,6 +12,7 @@ > > #include <asm/arch/system_manager.h> > > #include <asm/io.h> > > #include <dt-bindings/clock/agilex-clock.h> > > +#include <asm/arch/secure_reg_helper.h> > > > > DECLARE_GLOBAL_DATA_PTR; > > > > @@ -65,8 +66,8 @@ unsigned int cm_get_l4_sys_free_clk_hz(void) > > > > u32 cm_get_qspi_controller_clk_hz(void) > > { > > - return readl(socfpga_get_sysmgr_addr() + > > - SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > > + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > > + > > SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > > } > > > > void cm_print_clock_quick_summary(void) > > diff --git a/arch/arm/mach-socfpga/clock_manager_s10.c b/arch/arm/mach- > > socfpga/clock_manager_s10.c > > index 05e4212..02578cc 100644 > > --- a/arch/arm/mach-socfpga/clock_manager_s10.c > > +++ b/arch/arm/mach-socfpga/clock_manager_s10.c > > @@ -9,6 +9,7 @@ > > #include <asm/arch/clock_manager.h> > > #include <asm/arch/handoff_s10.h> > > #include <asm/arch/system_manager.h> > > +#include <asm/arch/secure_reg_helper.h> > > > > DECLARE_GLOBAL_DATA_PTR; > > > > @@ -385,8 +386,8 @@ unsigned int cm_get_l4_sp_clk_hz(void) > > > > unsigned int cm_get_qspi_controller_clk_hz(void) > > { > > - return readl(socfpga_get_sysmgr_addr() + > > - SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > > + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > > + > > SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > > } > > > > unsigned int cm_get_spi_controller_clk_hz(void) > > -- > > 2.7.4 > SPL reads the clock info from handoff table (OCRAM) and write > the clock info into the System Manager's boot scratch register. > U-Boot proper will read from the System Manager's boot scratch > register to get the clock info in case the handoff table (OCRAM) > is no longer available. > After some investigations, the handoff table in OCRAM should be preserved > for warm boot. In other words, this handoff table should be left untouched. > SPL and U-Boot should directly read the clock info from handoff table in > OCRAM. > Therefore, U-Boot proper no longer need to read the clock info from > System Manager's boot scratch register (secure zone) from non-secure world > (EL2). > I don't think that's a good idea: for security reasons, SPL memory should not be accessible from EL2 if it is required/used for the next reboot. Regards, Simon
Ang, Chee Hong <chee.hong.ang at intel.com<mailto:chee.hong.ang at intel.com>> schrieb am Sa., 22. Feb. 2020, 06:30: > From: Chee Hong Ang <chee.hong.ang at intel.com<mailto:chee.hong.ang at intel.com>> > > Allow clock manager driver to access the System Manager's Boot Scratch > Register 0 in non-secure mode (EL2) on SoC 64bits platform. > > Signed-off-by: Chee Hong Ang <chee.hong.ang at intel.com<mailto:chee.hong.ang at intel.com>> > --- > arch/arm/mach-socfpga/clock_manager_agilex.c | 5 +++-- > arch/arm/mach-socfpga/clock_manager_s10.c | 5 +++-- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/mach-socfpga/clock_manager_agilex.c b/arch/arm/mach- > socfpga/clock_manager_agilex.c > index 4ee2b7b..e5a0998 100644 > --- a/arch/arm/mach-socfpga/clock_manager_agilex.c > +++ b/arch/arm/mach-socfpga/clock_manager_agilex.c > @@ -12,6 +12,7 @@ > #include <asm/arch/system_manager.h> > #include <asm/io.h> > #include <dt-bindings/clock/agilex-clock.h> > +#include <asm/arch/secure_reg_helper.h> > > DECLARE_GLOBAL_DATA_PTR; > > @@ -65,8 +66,8 @@ unsigned int cm_get_l4_sys_free_clk_hz(void) > > u32 cm_get_qspi_controller_clk_hz(void) > { > - return readl(socfpga_get_sysmgr_addr() + > - SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > + > SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > } > > void cm_print_clock_quick_summary(void) > diff --git a/arch/arm/mach-socfpga/clock_manager_s10.c b/arch/arm/mach- > socfpga/clock_manager_s10.c > index 05e4212..02578cc 100644 > --- a/arch/arm/mach-socfpga/clock_manager_s10.c > +++ b/arch/arm/mach-socfpga/clock_manager_s10.c > @@ -9,6 +9,7 @@ > #include <asm/arch/clock_manager.h> > #include <asm/arch/handoff_s10.h> > #include <asm/arch/system_manager.h> > +#include <asm/arch/secure_reg_helper.h> > > DECLARE_GLOBAL_DATA_PTR; > > @@ -385,8 +386,8 @@ unsigned int cm_get_l4_sp_clk_hz(void) > > unsigned int cm_get_qspi_controller_clk_hz(void) > { > - return readl(socfpga_get_sysmgr_addr() + > - SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > + > SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > } > > unsigned int cm_get_spi_controller_clk_hz(void) > -- > 2.7.4 >SPL reads the clock info from handoff table (OCRAM) and write >the clock info into the System Manager's boot scratch register. >U-Boot proper will read from the System Manager's boot scratch >register to get the clock info in case the handoff table (OCRAM) >is no longer available. >After some investigations, the handoff table in OCRAM should be preserved >for warm boot. In other words, this handoff table should be left untouched. >SPL and U-Boot should directly read the clock info from handoff table in OCRAM. >Therefore, U-Boot proper no longer need to read the clock info from >System Manager's boot scratch register (secure zone) from non-secure world (EL2). >I don't think that's a good idea: for security reasons, SPL memory should not be accessible from EL2 if it is required/used for the next reboot. > >Regards, >Simon Right. I think I will have to go for proper high-level API in ATF for EL2 to query the clock frequency: INTEL_SIP_SMC_CLK_GET_QSPI
From: Ang, Chee Hong Sent: Saturday, February 22, 2020 6:00 PM To: Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com> Cc: U-Boot Mailing List <u-boot at lists.denx.de>; Marek Vasut <marex at denx.de>; See, Chin Liang <chin.liang.see at intel.com>; Tan, Ley Foon <ley.foon.tan at intel.com>; Westergreen, Dalon <dalon.westergreen at intel.com>; Gong, Richard <richard.gong at intel.com> Subject: RE: [PATCH v2 11/21] arm: socfpga: Secure register access for clock manager (SoC 64bits) Ang, Chee Hong <chee.hong.ang at intel.com<mailto:chee.hong.ang at intel.com>> schrieb am Sa., 22. Feb. 2020, 06:30: > From: Chee Hong Ang <chee.hong.ang at intel.com<mailto:chee.hong.ang at intel.com>> > > Allow clock manager driver to access the System Manager's Boot Scratch > Register 0 in non-secure mode (EL2) on SoC 64bits platform. > > Signed-off-by: Chee Hong Ang <chee.hong.ang at intel.com<mailto:chee.hong.ang at intel.com>> > --- > arch/arm/mach-socfpga/clock_manager_agilex.c | 5 +++-- > arch/arm/mach-socfpga/clock_manager_s10.c | 5 +++-- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/mach-socfpga/clock_manager_agilex.c b/arch/arm/mach- > socfpga/clock_manager_agilex.c > index 4ee2b7b..e5a0998 100644 > --- a/arch/arm/mach-socfpga/clock_manager_agilex.c > +++ b/arch/arm/mach-socfpga/clock_manager_agilex.c > @@ -12,6 +12,7 @@ > #include <asm/arch/system_manager.h> > #include <asm/io.h> > #include <dt-bindings/clock/agilex-clock.h> > +#include <asm/arch/secure_reg_helper.h> > > DECLARE_GLOBAL_DATA_PTR; > > @@ -65,8 +66,8 @@ unsigned int cm_get_l4_sys_free_clk_hz(void) > > u32 cm_get_qspi_controller_clk_hz(void) > { > - return readl(socfpga_get_sysmgr_addr() + > - SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > + > SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > } > > void cm_print_clock_quick_summary(void) > diff --git a/arch/arm/mach-socfpga/clock_manager_s10.c b/arch/arm/mach- > socfpga/clock_manager_s10.c > index 05e4212..02578cc 100644 > --- a/arch/arm/mach-socfpga/clock_manager_s10.c > +++ b/arch/arm/mach-socfpga/clock_manager_s10.c > @@ -9,6 +9,7 @@ > #include <asm/arch/clock_manager.h> > #include <asm/arch/handoff_s10.h> > #include <asm/arch/system_manager.h> > +#include <asm/arch/secure_reg_helper.h> > > DECLARE_GLOBAL_DATA_PTR; > > @@ -385,8 +386,8 @@ unsigned int cm_get_l4_sp_clk_hz(void) > > unsigned int cm_get_qspi_controller_clk_hz(void) > { > - return readl(socfpga_get_sysmgr_addr() + > - SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > + > SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > } > > unsigned int cm_get_spi_controller_clk_hz(void) > -- > 2.7.4 >SPL reads the clock info from handoff table (OCRAM) and write >the clock info into the System Manager's boot scratch register. >U-Boot proper will read from the System Manager's boot scratch >register to get the clock info in case the handoff table (OCRAM) >is no longer available. >After some investigations, the handoff table in OCRAM should be preserved >for warm boot. In other words, this handoff table should be left untouched. >SPL and U-Boot should directly read the clock info from handoff table in OCRAM. >Therefore, U-Boot proper no longer need to read the clock info from >System Manager's boot scratch register (secure zone) from non-secure world (EL2). >I don't think that's a good idea: for security reasons, SPL memory should not be accessible from EL2 if it is required/used for the next reboot. > >Regards, >Simon Right. I think I will have to go for proper high-level API in ATF for EL2 to query the clock frequency: INTEL_SIP_SMC_CLK_GET_QSPI I found out System Manager is read only in EL2 and read/write in EL3. Will drop this patch. No change required since it only read back from System Manager?s registers.
Ang, Chee Hong <chee.hong.ang at intel.com> schrieb am Mo., 24. Feb. 2020, 10:12: > > > > > *From:* Ang, Chee Hong > *Sent:* Saturday, February 22, 2020 6:00 PM > *To:* Simon Goldschmidt <simon.k.r.goldschmidt at gmail.com> > *Cc:* U-Boot Mailing List <u-boot at lists.denx.de>; Marek Vasut < > marex at denx.de>; See, Chin Liang <chin.liang.see at intel.com>; Tan, Ley Foon > <ley.foon.tan at intel.com>; Westergreen, Dalon <dalon.westergreen at intel.com>; > Gong, Richard <richard.gong at intel.com> > *Subject:* RE: [PATCH v2 11/21] arm: socfpga: Secure register access for > clock manager (SoC 64bits) > > > > Ang, Chee Hong <chee.hong.ang at intel.com> schrieb am Sa., 22. Feb. 2020, > 06:30: > > > From: Chee Hong Ang <chee.hong.ang at intel.com> > > > > Allow clock manager driver to access the System Manager's Boot Scratch > > Register 0 in non-secure mode (EL2) on SoC 64bits platform. > > > > Signed-off-by: Chee Hong Ang <chee.hong.ang at intel.com> > > --- > > arch/arm/mach-socfpga/clock_manager_agilex.c | 5 +++-- > > arch/arm/mach-socfpga/clock_manager_s10.c | 5 +++-- > > 2 files changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/arch/arm/mach-socfpga/clock_manager_agilex.c > b/arch/arm/mach- > > socfpga/clock_manager_agilex.c > > index 4ee2b7b..e5a0998 100644 > > --- a/arch/arm/mach-socfpga/clock_manager_agilex.c > > +++ b/arch/arm/mach-socfpga/clock_manager_agilex.c > > @@ -12,6 +12,7 @@ > > #include <asm/arch/system_manager.h> > > #include <asm/io.h> > > #include <dt-bindings/clock/agilex-clock.h> > > +#include <asm/arch/secure_reg_helper.h> > > > > DECLARE_GLOBAL_DATA_PTR; > > > > @@ -65,8 +66,8 @@ unsigned int cm_get_l4_sys_free_clk_hz(void) > > > > u32 cm_get_qspi_controller_clk_hz(void) > > { > > - return readl(socfpga_get_sysmgr_addr() + > > - SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > > + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > > + > > SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > > } > > > > void cm_print_clock_quick_summary(void) > > diff --git a/arch/arm/mach-socfpga/clock_manager_s10.c b/arch/arm/mach- > > socfpga/clock_manager_s10.c > > index 05e4212..02578cc 100644 > > --- a/arch/arm/mach-socfpga/clock_manager_s10.c > > +++ b/arch/arm/mach-socfpga/clock_manager_s10.c > > @@ -9,6 +9,7 @@ > > #include <asm/arch/clock_manager.h> > > #include <asm/arch/handoff_s10.h> > > #include <asm/arch/system_manager.h> > > +#include <asm/arch/secure_reg_helper.h> > > > > DECLARE_GLOBAL_DATA_PTR; > > > > @@ -385,8 +386,8 @@ unsigned int cm_get_l4_sp_clk_hz(void) > > > > unsigned int cm_get_qspi_controller_clk_hz(void) > > { > > - return readl(socfpga_get_sysmgr_addr() + > > - SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > > + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > > + > > SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > > } > > > > unsigned int cm_get_spi_controller_clk_hz(void) > > -- > > 2.7.4 > >SPL reads the clock info from handoff table (OCRAM) and write > >the clock info into the System Manager's boot scratch register. > >U-Boot proper will read from the System Manager's boot scratch > >register to get the clock info in case the handoff table (OCRAM) > >is no longer available. > >After some investigations, the handoff table in OCRAM should be preserved > >for warm boot. In other words, this handoff table should be left > untouched. > >SPL and U-Boot should directly read the clock info from handoff table in > OCRAM. > >Therefore, U-Boot proper no longer need to read the clock info from > >System Manager's boot scratch register (secure zone) from non-secure > world (EL2). > > > > >I don't think that's a good idea: for security reasons, SPL memory should > not be accessible from EL2 if it is required/used for the next reboot. > > > > > >Regards, > > >Simon > > Right. I think I will have to go for proper high-level API in ATF for EL2 > to query the clock frequency: > > INTEL_SIP_SMC_CLK_GET_QSPI > > > > I found out System Manager is read only in EL2 and read/write in EL3. > > Will drop this patch. > > No change required since it only read back from System Manager?s registers. > So reading these registers is allowed in EL2? I would have expected all access is blocked? Is this specified somewhere, or will it be? Regards, Simon >
Ang, Chee Hong <chee.hong.ang at intel.com<mailto:chee.hong.ang at intel.com>> schrieb am Sa., 22. Feb. 2020, 06:30: > From: Chee Hong Ang <chee.hong.ang at intel.com<mailto:chee.hong.ang at intel.com>> > > Allow clock manager driver to access the System Manager's Boot Scratch > Register 0 in non-secure mode (EL2) on SoC 64bits platform. > > Signed-off-by: Chee Hong Ang <chee.hong.ang at intel.com<mailto:chee.hong.ang at intel.com>> > --- > arch/arm/mach-socfpga/clock_manager_agilex.c | 5 +++-- > arch/arm/mach-socfpga/clock_manager_s10.c | 5 +++-- > 2 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/mach-socfpga/clock_manager_agilex.c b/arch/arm/mach- > socfpga/clock_manager_agilex.c > index 4ee2b7b..e5a0998 100644 > --- a/arch/arm/mach-socfpga/clock_manager_agilex.c > +++ b/arch/arm/mach-socfpga/clock_manager_agilex.c > @@ -12,6 +12,7 @@ > #include <asm/arch/system_manager.h> > #include <asm/io.h> > #include <dt-bindings/clock/agilex-clock.h> > +#include <asm/arch/secure_reg_helper.h> > > DECLARE_GLOBAL_DATA_PTR; > > @@ -65,8 +66,8 @@ unsigned int cm_get_l4_sys_free_clk_hz(void) > > u32 cm_get_qspi_controller_clk_hz(void) > { > - return readl(socfpga_get_sysmgr_addr() + > - SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > + > SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > } > > void cm_print_clock_quick_summary(void) > diff --git a/arch/arm/mach-socfpga/clock_manager_s10.c b/arch/arm/mach- > socfpga/clock_manager_s10.c > index 05e4212..02578cc 100644 > --- a/arch/arm/mach-socfpga/clock_manager_s10.c > +++ b/arch/arm/mach-socfpga/clock_manager_s10.c > @@ -9,6 +9,7 @@ > #include <asm/arch/clock_manager.h> > #include <asm/arch/handoff_s10.h> > #include <asm/arch/system_manager.h> > +#include <asm/arch/secure_reg_helper.h> > > DECLARE_GLOBAL_DATA_PTR; > > @@ -385,8 +386,8 @@ unsigned int cm_get_l4_sp_clk_hz(void) > > unsigned int cm_get_qspi_controller_clk_hz(void) > { > - return readl(socfpga_get_sysmgr_addr() + > - SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > + > SYSMGR_SOC64_BOOT_SCRATCH_COLD0); > } > > unsigned int cm_get_spi_controller_clk_hz(void) > -- > 2.7.4 >SPL reads the clock info from handoff table (OCRAM) and write >the clock info into the System Manager's boot scratch register. >U-Boot proper will read from the System Manager's boot scratch >register to get the clock info in case the handoff table (OCRAM) >is no longer available. >After some investigations, the handoff table in OCRAM should be preserved >for warm boot. In other words, this handoff table should be left untouched. >SPL and U-Boot should directly read the clock info from handoff table in OCRAM. >Therefore, U-Boot proper no longer need to read the clock info from >System Manager's boot scratch register (secure zone) from non-secure world (EL2). >I don't think that's a good idea: for security reasons, SPL memory should not be accessible from EL2 if it is required/used for the next reboot. > >Regards, >Simon Right. I think I will have to go for proper high-level API in ATF for EL2 to query the clock frequency: INTEL_SIP_SMC_CLK_GET_QSPI I found out System Manager is read only in EL2 and read/write in EL3. Will drop this patch. No change required since it only read back from System Manager?s registers. >So reading these registers is allowed in EL2? I would have expected all access is blocked? Is this specified somewhere, or will it be? > >Regards, >Simon Yes. I know this is confusing. I would have expected the read access be blocked as well. Unfortunately, this is not the case. Let me clarify further: Here is a list of Stratix10 System Manager?s register map: https://www.intel.com/content/www/us/en/programmable/hps/stratix-10/hps.html#topic/dwh1505406933720.html Any R/W registers between ?siliconid1? to ?mpu? are READ-ONLY in EL2. But are read/writable in EL3. If you click into one of these R/W registers: You will see a notice like this: Access: RW Access mode: PRIVILEGEMODE | SECURE Note: The processor must make a secure, privileged bus access to this register. It just said this register is read/writable in EL3 but it doesn?t specify it is read-only in EL2. I did some tests to read from these registers in EL2. It worked. It crashed the U-Boot with ?SError? exception if I tried to write something into one of these registers. For registers after ?mpu? which are ?boot_scratch_cold0? ? ?boot_scratch_cold9?: If you click into one of this boot scratch registers: https://www.intel.com/content/www/us/en/programmable/hps/stratix-10/hps.html#topic/lom1505406925262.html You don?t see any requirement about the processor need to be in secure mode to access this register. Previously I mentioned these registers are read-only in EL2. They are actually read/writable in EL2 and EL3. Sorry for the misinformation The clock manager drivers are writing/reading clock settings into these boot scratch registers so changes are not necessary for EL2. In summary, although ?boot_scratch_cold0? ? ?boot_scratch_cold9? registers are part of System Manager, but they are not marked as ?secure zone?. I missed this when working on this ATF flow :(
diff --git a/arch/arm/mach-socfpga/clock_manager_agilex.c b/arch/arm/mach-socfpga/clock_manager_agilex.c index 4ee2b7b..e5a0998 100644 --- a/arch/arm/mach-socfpga/clock_manager_agilex.c +++ b/arch/arm/mach-socfpga/clock_manager_agilex.c @@ -12,6 +12,7 @@ #include <asm/arch/system_manager.h> #include <asm/io.h> #include <dt-bindings/clock/agilex-clock.h> +#include <asm/arch/secure_reg_helper.h> DECLARE_GLOBAL_DATA_PTR; @@ -65,8 +66,8 @@ unsigned int cm_get_l4_sys_free_clk_hz(void) u32 cm_get_qspi_controller_clk_hz(void) { - return readl(socfpga_get_sysmgr_addr() + - SYSMGR_SOC64_BOOT_SCRATCH_COLD0); + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + + SYSMGR_SOC64_BOOT_SCRATCH_COLD0); } void cm_print_clock_quick_summary(void) diff --git a/arch/arm/mach-socfpga/clock_manager_s10.c b/arch/arm/mach-socfpga/clock_manager_s10.c index 05e4212..02578cc 100644 --- a/arch/arm/mach-socfpga/clock_manager_s10.c +++ b/arch/arm/mach-socfpga/clock_manager_s10.c @@ -9,6 +9,7 @@ #include <asm/arch/clock_manager.h> #include <asm/arch/handoff_s10.h> #include <asm/arch/system_manager.h> +#include <asm/arch/secure_reg_helper.h> DECLARE_GLOBAL_DATA_PTR; @@ -385,8 +386,8 @@ unsigned int cm_get_l4_sp_clk_hz(void) unsigned int cm_get_qspi_controller_clk_hz(void) { - return readl(socfpga_get_sysmgr_addr() + - SYSMGR_SOC64_BOOT_SCRATCH_COLD0); + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + + SYSMGR_SOC64_BOOT_SCRATCH_COLD0); } unsigned int cm_get_spi_controller_clk_hz(void)