[PATCH v2 11/21] arm: socfpga: Secure register access for clock manager (SoC 64bits)

[Ang, Chee Hong]

> -----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).