| Message ID | 1582115146-28658-14-git-send-email-chee.hong.ang@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Enable ARM Trusted Firmware for U-Boot | expand |
> From: Chee Hong Ang <chee.hong.ang at intel.com> > > Allow reading external oscillator and FPGA clock's frequency from System > Manager's Boot Scratch Register 1 and Boot Scratch Register 2 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/wrap_pll_config_s10.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/mach-socfpga/wrap_pll_config_s10.c b/arch/arm/mach- > socfpga/wrap_pll_config_s10.c > index 3da8579..7bd92d0 100644 > --- a/arch/arm/mach-socfpga/wrap_pll_config_s10.c > +++ b/arch/arm/mach-socfpga/wrap_pll_config_s10.c > @@ -9,6 +9,7 @@ > #include <asm/io.h> > #include <asm/arch/handoff_s10.h> > #include <asm/arch/system_manager.h> > +#include <asm/arch/secure_reg_helper.h> > > const struct cm_config * const cm_get_default_config(void) { @@ -39,8 +40,8 > @@ const unsigned int cm_get_osc_clk_hz(void) > writel(clock, > socfpga_get_sysmgr_addr() + > SYSMGR_SOC64_BOOT_SCRATCH_COLD1); #endif > - return readl(socfpga_get_sysmgr_addr() + > - SYSMGR_SOC64_BOOT_SCRATCH_COLD1); > + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > + > SYSMGR_SOC64_BOOT_SCRATCH_COLD1); > } > > const unsigned int cm_get_intosc_clk_hz(void) @@ -56,6 +57,6 @@ const > unsigned int cm_get_fpga_clk_hz(void) > writel(clock, > socfpga_get_sysmgr_addr() + > SYSMGR_SOC64_BOOT_SCRATCH_COLD2); #endif > - return readl(socfpga_get_sysmgr_addr() + > - SYSMGR_SOC64_BOOT_SCRATCH_COLD2); > + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > + > SYSMGR_SOC64_BOOT_SCRATCH_COLD2); > } > -- > 2.7.4 This clock info could be directly read from the handoff table (OCRAM) instead of the System Manager's boot scratch register (secure zone). Please refer to my full explanation in my previous email reply: [PATCH v2 11/21] arm: socfpga: Secure register access for clock manager (SoC 64bits)
> > From: Chee Hong Ang <chee.hong.ang at intel.com> > > > > Allow reading external oscillator and FPGA clock's frequency from > > System Manager's Boot Scratch Register 1 and Boot Scratch Register 2 > > 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/wrap_pll_config_s10.c | 9 +++++---- > > 1 file changed, 5 insertions(+), 4 deletions(-) > > > > diff --git a/arch/arm/mach-socfpga/wrap_pll_config_s10.c > > b/arch/arm/mach- socfpga/wrap_pll_config_s10.c index 3da8579..7bd92d0 > > 100644 > > --- a/arch/arm/mach-socfpga/wrap_pll_config_s10.c > > +++ b/arch/arm/mach-socfpga/wrap_pll_config_s10.c > > @@ -9,6 +9,7 @@ > > #include <asm/io.h> > > #include <asm/arch/handoff_s10.h> > > #include <asm/arch/system_manager.h> > > +#include <asm/arch/secure_reg_helper.h> > > > > const struct cm_config * const cm_get_default_config(void) { @@ > > -39,8 +40,8 @@ const unsigned int cm_get_osc_clk_hz(void) > > writel(clock, > > socfpga_get_sysmgr_addr() + > > SYSMGR_SOC64_BOOT_SCRATCH_COLD1); #endif > > - return readl(socfpga_get_sysmgr_addr() + > > - SYSMGR_SOC64_BOOT_SCRATCH_COLD1); > > + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > > + > > SYSMGR_SOC64_BOOT_SCRATCH_COLD1); > > } > > > > const unsigned int cm_get_intosc_clk_hz(void) @@ -56,6 +57,6 @@ const > > unsigned int cm_get_fpga_clk_hz(void) > > writel(clock, > > socfpga_get_sysmgr_addr() + > > SYSMGR_SOC64_BOOT_SCRATCH_COLD2); #endif > > - return readl(socfpga_get_sysmgr_addr() + > > - SYSMGR_SOC64_BOOT_SCRATCH_COLD2); > > + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > > + > > SYSMGR_SOC64_BOOT_SCRATCH_COLD2); > > } > > -- > > 2.7.4 > This clock info could be directly read from the handoff table (OCRAM) instead of > the System Manager's boot scratch register (secure zone). > Please refer to my full explanation in my previous email reply: > [PATCH v2 11/21] arm: socfpga: Secure register access for clock manager (SoC > 64bits) Simon raised a good security concern on this approach. I will drop this approach. Will go for high-level APIs in ATF for clock queries: INTEL_SIP_SMC_CLK_GET_OSC INTEL_SIP_SMC_CLK_GET_FPGA
On 2/22/20 11:05 AM, Ang, Chee Hong wrote: >>> From: Chee Hong Ang <chee.hong.ang at intel.com> >>> >>> Allow reading external oscillator and FPGA clock's frequency from >>> System Manager's Boot Scratch Register 1 and Boot Scratch Register 2 >>> 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/wrap_pll_config_s10.c | 9 +++++---- >>> 1 file changed, 5 insertions(+), 4 deletions(-) >>> >>> diff --git a/arch/arm/mach-socfpga/wrap_pll_config_s10.c >>> b/arch/arm/mach- socfpga/wrap_pll_config_s10.c index 3da8579..7bd92d0 >>> 100644 >>> --- a/arch/arm/mach-socfpga/wrap_pll_config_s10.c >>> +++ b/arch/arm/mach-socfpga/wrap_pll_config_s10.c >>> @@ -9,6 +9,7 @@ >>> #include <asm/io.h> >>> #include <asm/arch/handoff_s10.h> >>> #include <asm/arch/system_manager.h> >>> +#include <asm/arch/secure_reg_helper.h> >>> >>> const struct cm_config * const cm_get_default_config(void) { @@ >>> -39,8 +40,8 @@ const unsigned int cm_get_osc_clk_hz(void) >>> writel(clock, >>> socfpga_get_sysmgr_addr() + >>> SYSMGR_SOC64_BOOT_SCRATCH_COLD1); #endif >>> - return readl(socfpga_get_sysmgr_addr() + >>> - SYSMGR_SOC64_BOOT_SCRATCH_COLD1); >>> + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + >>> + >>> SYSMGR_SOC64_BOOT_SCRATCH_COLD1); >>> } >>> >>> const unsigned int cm_get_intosc_clk_hz(void) @@ -56,6 +57,6 @@ const >>> unsigned int cm_get_fpga_clk_hz(void) >>> writel(clock, >>> socfpga_get_sysmgr_addr() + >>> SYSMGR_SOC64_BOOT_SCRATCH_COLD2); #endif >>> - return readl(socfpga_get_sysmgr_addr() + >>> - SYSMGR_SOC64_BOOT_SCRATCH_COLD2); >>> + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + >>> + >>> SYSMGR_SOC64_BOOT_SCRATCH_COLD2); >>> } >>> -- >>> 2.7.4 >> This clock info could be directly read from the handoff table (OCRAM) instead of >> the System Manager's boot scratch register (secure zone). >> Please refer to my full explanation in my previous email reply: >> [PATCH v2 11/21] arm: socfpga: Secure register access for clock manager (SoC >> 64bits) > Simon raised a good security concern on this approach. I will drop this approach. > Will go for high-level APIs in ATF for clock queries: > INTEL_SIP_SMC_CLK_GET_OSC > INTEL_SIP_SMC_CLK_GET_FPGA Can't you have a clock driver read out the clock tree once and then have all the drivers in U-Boot just get clock settings from the clock driver?
> On 2/22/20 11:05 AM, Ang, Chee Hong wrote: > >>> From: Chee Hong Ang <chee.hong.ang at intel.com> > >>> > >>> Allow reading external oscillator and FPGA clock's frequency from > >>> System Manager's Boot Scratch Register 1 and Boot Scratch Register 2 > >>> 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/wrap_pll_config_s10.c | 9 +++++---- > >>> 1 file changed, 5 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/arch/arm/mach-socfpga/wrap_pll_config_s10.c > >>> b/arch/arm/mach- socfpga/wrap_pll_config_s10.c index > >>> 3da8579..7bd92d0 > >>> 100644 > >>> --- a/arch/arm/mach-socfpga/wrap_pll_config_s10.c > >>> +++ b/arch/arm/mach-socfpga/wrap_pll_config_s10.c > >>> @@ -9,6 +9,7 @@ > >>> #include <asm/io.h> > >>> #include <asm/arch/handoff_s10.h> > >>> #include <asm/arch/system_manager.h> > >>> +#include <asm/arch/secure_reg_helper.h> > >>> > >>> const struct cm_config * const cm_get_default_config(void) { @@ > >>> -39,8 +40,8 @@ const unsigned int cm_get_osc_clk_hz(void) > >>> writel(clock, > >>> socfpga_get_sysmgr_addr() + > >>> SYSMGR_SOC64_BOOT_SCRATCH_COLD1); #endif > >>> - return readl(socfpga_get_sysmgr_addr() + > >>> - SYSMGR_SOC64_BOOT_SCRATCH_COLD1); > >>> + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > >>> + > >>> SYSMGR_SOC64_BOOT_SCRATCH_COLD1); > >>> } > >>> > >>> const unsigned int cm_get_intosc_clk_hz(void) @@ -56,6 +57,6 @@ > >>> const unsigned int cm_get_fpga_clk_hz(void) > >>> writel(clock, > >>> socfpga_get_sysmgr_addr() + > >>> SYSMGR_SOC64_BOOT_SCRATCH_COLD2); #endif > >>> - return readl(socfpga_get_sysmgr_addr() + > >>> - SYSMGR_SOC64_BOOT_SCRATCH_COLD2); > >>> + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > >>> + > >>> SYSMGR_SOC64_BOOT_SCRATCH_COLD2); > >>> } > >>> -- > >>> 2.7.4 > >> This clock info could be directly read from the handoff table (OCRAM) > >> instead of the System Manager's boot scratch register (secure zone). > >> Please refer to my full explanation in my previous email reply: > >> [PATCH v2 11/21] arm: socfpga: Secure register access for clock > >> manager (SoC > >> 64bits) > > Simon raised a good security concern on this approach. I will drop this > approach. > > Will go for high-level APIs in ATF for clock queries: > > INTEL_SIP_SMC_CLK_GET_OSC > > INTEL_SIP_SMC_CLK_GET_FPGA > > Can't you have a clock driver read out the clock tree once and then have all the > drivers in U-Boot just get clock settings from the clock driver? Yes. This could be part of the clock driver (clock_manager_s10 / agilex.c) instead of scattering around in different places.
> > On 2/22/20 11:05 AM, Ang, Chee Hong wrote: > > >>> From: Chee Hong Ang <chee.hong.ang at intel.com> > > >>> > > >>> Allow reading external oscillator and FPGA clock's frequency from > > >>> System Manager's Boot Scratch Register 1 and Boot Scratch Register > > >>> 2 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/wrap_pll_config_s10.c | 9 +++++---- > > >>> 1 file changed, 5 insertions(+), 4 deletions(-) > > >>> > > >>> diff --git a/arch/arm/mach-socfpga/wrap_pll_config_s10.c > > >>> b/arch/arm/mach- socfpga/wrap_pll_config_s10.c index > > >>> 3da8579..7bd92d0 > > >>> 100644 > > >>> --- a/arch/arm/mach-socfpga/wrap_pll_config_s10.c > > >>> +++ b/arch/arm/mach-socfpga/wrap_pll_config_s10.c > > >>> @@ -9,6 +9,7 @@ > > >>> #include <asm/io.h> > > >>> #include <asm/arch/handoff_s10.h> #include > > >>> <asm/arch/system_manager.h> > > >>> +#include <asm/arch/secure_reg_helper.h> > > >>> > > >>> const struct cm_config * const cm_get_default_config(void) { @@ > > >>> -39,8 +40,8 @@ const unsigned int cm_get_osc_clk_hz(void) > > >>> writel(clock, > > >>> socfpga_get_sysmgr_addr() + > > >>> SYSMGR_SOC64_BOOT_SCRATCH_COLD1); #endif > > >>> - return readl(socfpga_get_sysmgr_addr() + > > >>> - SYSMGR_SOC64_BOOT_SCRATCH_COLD1); > > >>> + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > > >>> + > > >>> SYSMGR_SOC64_BOOT_SCRATCH_COLD1); > > >>> } > > >>> > > >>> const unsigned int cm_get_intosc_clk_hz(void) @@ -56,6 +57,6 @@ > > >>> const unsigned int cm_get_fpga_clk_hz(void) > > >>> writel(clock, > > >>> socfpga_get_sysmgr_addr() + > > >>> SYSMGR_SOC64_BOOT_SCRATCH_COLD2); #endif > > >>> - return readl(socfpga_get_sysmgr_addr() + > > >>> - SYSMGR_SOC64_BOOT_SCRATCH_COLD2); > > >>> + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + > > >>> + > > >>> SYSMGR_SOC64_BOOT_SCRATCH_COLD2); > > >>> } > > >>> -- > > >>> 2.7.4 > > >> This clock info could be directly read from the handoff table > > >> (OCRAM) instead of the System Manager's boot scratch register (secure > zone). > > >> Please refer to my full explanation in my previous email reply: > > >> [PATCH v2 11/21] arm: socfpga: Secure register access for clock > > >> manager (SoC > > >> 64bits) > > > Simon raised a good security concern on this approach. I will drop > > > this > > approach. > > > Will go for high-level APIs in ATF for clock queries: > > > INTEL_SIP_SMC_CLK_GET_OSC > > > INTEL_SIP_SMC_CLK_GET_FPGA > > > > Can't you have a clock driver read out the clock tree once and then > > have all the drivers in U-Boot just get clock settings from the clock driver? In 'wrap_pll_config_s10.c': cm_get_osc_clk_hz() cm_get_intosc_clk_hz() cm_get_fpga_clk_hz() All the clock settings returned by S10/Agilex clock manager drivers derived from these 3 clock sources (listed above) then all other U-Boot drivers get the clock settings from the clock driver. Can you help clarify what do you mean by "read out the clock tree once" ? Anyway, there will be only one high-level API for reading the OSC/FPGA/QSPI clock sources depending on which clock source chosen by caller. Please ignore my reply below. > Yes. This could be part of the clock driver (clock_manager_s10 / agilex.c) instead > of scattering around in different places.
> > > On 2/22/20 11:05 AM, Ang, Chee Hong wrote: > > > >>> From: Chee Hong Ang <chee.hong.ang at intel.com> > > > >>> > > > >>> Allow reading external oscillator and FPGA clock's frequency > > > >>> from System Manager's Boot Scratch Register 1 and Boot Scratch > > > >>> Register > > > >>> 2 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/wrap_pll_config_s10.c | 9 +++++---- > > > >>> 1 file changed, 5 insertions(+), 4 deletions(-) > > > >>> > > > >>> diff --git a/arch/arm/mach-socfpga/wrap_pll_config_s10.c > > > >>> b/arch/arm/mach- socfpga/wrap_pll_config_s10.c index > > > >>> 3da8579..7bd92d0 > > > >>> 100644 > > > >>> --- a/arch/arm/mach-socfpga/wrap_pll_config_s10.c > > > >>> +++ b/arch/arm/mach-socfpga/wrap_pll_config_s10.c > > > >>> @@ -9,6 +9,7 @@ > > > >>> #include <asm/io.h> > > > >>> #include <asm/arch/handoff_s10.h> #include > > > >>> <asm/arch/system_manager.h> > > > >>> +#include <asm/arch/secure_reg_helper.h> > > > >>> > > > >>> const struct cm_config * const cm_get_default_config(void) { > > > >>> @@ > > > >>> -39,8 +40,8 @@ const unsigned int cm_get_osc_clk_hz(void) > > > >>> writel(clock, > > > >>> socfpga_get_sysmgr_addr() + > > > >>> SYSMGR_SOC64_BOOT_SCRATCH_COLD1); #endif > > > >>> - return readl(socfpga_get_sysmgr_addr() + > > > >>> - SYSMGR_SOC64_BOOT_SCRATCH_COLD1); > > > >>> + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() > + > > > >>> + > > > >>> SYSMGR_SOC64_BOOT_SCRATCH_COLD1); } > > > >>> > > > >>> const unsigned int cm_get_intosc_clk_hz(void) @@ -56,6 +57,6 @@ > > > >>> const unsigned int cm_get_fpga_clk_hz(void) > > > >>> writel(clock, > > > >>> socfpga_get_sysmgr_addr() + > > > >>> SYSMGR_SOC64_BOOT_SCRATCH_COLD2); #endif > > > >>> - return readl(socfpga_get_sysmgr_addr() + > > > >>> - SYSMGR_SOC64_BOOT_SCRATCH_COLD2); > > > >>> + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() > + > > > >>> + > > > >>> SYSMGR_SOC64_BOOT_SCRATCH_COLD2); } > > > >>> -- > > > >>> 2.7.4 > > > >> This clock info could be directly read from the handoff table > > > >> (OCRAM) instead of the System Manager's boot scratch register > > > >> (secure > > zone). > > > >> Please refer to my full explanation in my previous email reply: > > > >> [PATCH v2 11/21] arm: socfpga: Secure register access for clock > > > >> manager (SoC > > > >> 64bits) > > > > Simon raised a good security concern on this approach. I will drop > > > > this > > > approach. > > > > Will go for high-level APIs in ATF for clock queries: > > > > INTEL_SIP_SMC_CLK_GET_OSC > > > > INTEL_SIP_SMC_CLK_GET_FPGA > > > > > > Can't you have a clock driver read out the clock tree once and then > > > have all the drivers in U-Boot just get clock settings from the clock driver? > In 'wrap_pll_config_s10.c': > cm_get_osc_clk_hz() > cm_get_intosc_clk_hz() > cm_get_fpga_clk_hz() > > All the clock settings returned by S10/Agilex clock manager drivers derived from > these 3 clock sources (listed above) then all other U-Boot drivers get the clock > settings from the clock driver. > > Can you help clarify what do you mean by "read out the clock tree once" ? > > Anyway, there will be only one high-level API for reading the OSC/FPGA/QSPI > clock sources depending on which clock source chosen by caller. > > Please ignore my reply below. > > Yes. This could be part of the clock driver (clock_manager_s10 / > > agilex.c) instead of scattering around in different places. Please ignore all my replies above. Will drop this patch. System Manager is read only in EL2 and read/write in EL3. No change required since it only read back from System Manager?s registers.
diff --git a/arch/arm/mach-socfpga/wrap_pll_config_s10.c b/arch/arm/mach-socfpga/wrap_pll_config_s10.c index 3da8579..7bd92d0 100644 --- a/arch/arm/mach-socfpga/wrap_pll_config_s10.c +++ b/arch/arm/mach-socfpga/wrap_pll_config_s10.c @@ -9,6 +9,7 @@ #include <asm/io.h> #include <asm/arch/handoff_s10.h> #include <asm/arch/system_manager.h> +#include <asm/arch/secure_reg_helper.h> const struct cm_config * const cm_get_default_config(void) { @@ -39,8 +40,8 @@ const unsigned int cm_get_osc_clk_hz(void) writel(clock, socfpga_get_sysmgr_addr() + SYSMGR_SOC64_BOOT_SCRATCH_COLD1); #endif - return readl(socfpga_get_sysmgr_addr() + - SYSMGR_SOC64_BOOT_SCRATCH_COLD1); + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + + SYSMGR_SOC64_BOOT_SCRATCH_COLD1); } const unsigned int cm_get_intosc_clk_hz(void) @@ -56,6 +57,6 @@ const unsigned int cm_get_fpga_clk_hz(void) writel(clock, socfpga_get_sysmgr_addr() + SYSMGR_SOC64_BOOT_SCRATCH_COLD2); #endif - return readl(socfpga_get_sysmgr_addr() + - SYSMGR_SOC64_BOOT_SCRATCH_COLD2); + return socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + + SYSMGR_SOC64_BOOT_SCRATCH_COLD2); }