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

[Ang, Chee Hong]

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 :(