[PATCH v2 00/21] Enable ARM Trusted Firmware for U-Boot
From: "Ang, Chee Hong" chee.hong.ang@intel.com
v2 changes:
- Default defconfig support SPL + ATF + U-Boot boot flow: (socfpga_stratix10_defconfig / socfpga_agilex_defconfig)
SPL (EL3) -> ATF-BL31 (EL3) -> U-Boot Proper (EL2) -> Linux (EL1)
Added new defconfig to support existing SPL + U-Boot (no ATF) boot flow: (socfpga_stratix10_nofw_defconfig / socfpga_agilex_nofw_defconfig)
SPL (EL3) -> U-Boot Proper (EL3) -> Linux (EL1)
- SMC/PSCI calls (EL2) / secure code (EL3) is built in compile time
- Mailbox/FPGA reconfiguration driver now support ATF or without ATF
v1: https://lists.denx.de/pipermail/u-boot/2019-December/392424.html
These patchsets have dependency on: https://lists.denx.de/pipermail/u-boot/2019-September/384906.html
Chee Hong Ang (21): configs: agilex: Remove CONFIG_OF_EMBED arm: socfpga: add fit source file for pack itb with ATF arm: socfpga: Add function for checking description from FIT image arm: socfpga: Load FIT image with ATF support arm: socfpga: Override 'lowlevel_init' to support ATF configs: socfpga: Enable FIT image loading with ATF support arm: socfpga: Disable "spin-table" method for booting Linux arm: socfpga: Add SMC helper function for Intel SOCFPGA (64bits) arm: socfpga: Define SMC function identifiers for PSCI SiP services arm: socfpga: Add secure register access helper functions for SoC 64bits arm: socfpga: Secure register access for clock manager (SoC 64bits) arm: socfpga: Secure register access in PHY mode setup arm: socfpga: Secure register access for reading PLL frequency mmc: dwmmc: socfpga: Secure register access in MMC driver net: designware: socfpga: Secure register access in MAC driver arm: socfpga: Secure register access in Reset Manager driver arm: socfpga: stratix10: Initialize timer in SPL arm: socfpga: Bridge reset invokes SMC service calls in EL2 arm: socfpga: stratix10: Add ATF support to FPGA reconfig driver arm: socfpga: mailbox: Add 'SYSTEM_RESET' PSCI support to mbox_reset_cold() configs: socfpga: Add defconfig for Agilex and Stratix 10 without ATF support
arch/arm/mach-socfpga/Kconfig | 2 - arch/arm/mach-socfpga/Makefile | 8 + arch/arm/mach-socfpga/board.c | 10 + arch/arm/mach-socfpga/clock_manager_agilex.c | 5 +- arch/arm/mach-socfpga/clock_manager_s10.c | 5 +- arch/arm/mach-socfpga/include/mach/misc.h | 3 + .../mach-socfpga/include/mach/secure_reg_helper.h | 20 ++ arch/arm/mach-socfpga/lowlevel_init.S | 85 +++++ arch/arm/mach-socfpga/mailbox_s10.c | 4 + arch/arm/mach-socfpga/misc_s10.c | 49 ++- arch/arm/mach-socfpga/reset_manager_s10.c | 31 +- arch/arm/mach-socfpga/secure_reg_helper.c | 57 ++++ arch/arm/mach-socfpga/timer_s10.c | 3 +- arch/arm/mach-socfpga/wrap_pll_config_s10.c | 9 +- board/altera/soc64/its/fit_spl_atf.its | 52 +++ configs/socfpga_agilex_defconfig | 9 +- ...lex_defconfig => socfpga_agilex_nofw_defconfig} | 2 +- configs/socfpga_stratix10_defconfig | 8 +- ..._defconfig => socfpga_stratix10_nofw_defconfig} | 2 +- drivers/fpga/stratix10.c | 141 +++++++- drivers/mmc/socfpga_dw_mmc.c | 7 +- drivers/net/dwmac_socfpga.c | 5 +- include/configs/socfpga_soc64_common.h | 4 + include/linux/intel-smc.h | 374 +++++++++++++++++++++ 24 files changed, 857 insertions(+), 38 deletions(-) create mode 100644 arch/arm/mach-socfpga/include/mach/secure_reg_helper.h create mode 100644 arch/arm/mach-socfpga/lowlevel_init.S create mode 100644 arch/arm/mach-socfpga/secure_reg_helper.c create mode 100644 board/altera/soc64/its/fit_spl_atf.its copy configs/{socfpga_agilex_defconfig => socfpga_agilex_nofw_defconfig} (97%) copy configs/{socfpga_stratix10_defconfig => socfpga_stratix10_nofw_defconfig} (97%) create mode 100644 include/linux/intel-smc.h
From: Chee Hong Ang chee.hong.ang@intel.com
CONFIG_OF_EMBED was primarily enabled to support the agilex spl hex file requirements. Since this option now produces a warning during build, and the spl hex can be created using alternate methods, CONFIG_OF_EMBED is no longer needed.
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com --- configs/socfpga_agilex_defconfig | 1 - 1 file changed, 1 deletion(-)
diff --git a/configs/socfpga_agilex_defconfig b/configs/socfpga_agilex_defconfig index 4fd84ad..693a774 100644 --- a/configs/socfpga_agilex_defconfig +++ b/configs/socfpga_agilex_defconfig @@ -29,7 +29,6 @@ CONFIG_CMD_CACHE=y CONFIG_CMD_EXT4=y CONFIG_CMD_FAT=y CONFIG_CMD_FS_GENERIC=y -CONFIG_OF_EMBED=y CONFIG_DEFAULT_DEVICE_TREE="socfpga_agilex_socdk" CONFIG_ENV_IS_IN_MMC=y CONFIG_NET_RANDOM_ETHADDR=y
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@intel.com
CONFIG_OF_EMBED was primarily enabled to support the agilex spl hex file requirements. Since this option now produces a warning during build, and the spl hex can be created using alternate methods, CONFIG_OF_EMBED is no longer needed.
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com
If I apply just this patch, is the platform still bootable ?
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@intel.com
CONFIG_OF_EMBED was primarily enabled to support the agilex spl hex file requirements. Since this option now produces a warning during build, and the spl hex can be created using alternate methods, CONFIG_OF_EMBED is no longer needed.
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com
If I apply just this patch, is the platform still bootable ?
Yes. I tested on the platform. There is a similar patch from Dalon for Stratix10 and it still yet to be applied to mainline: https://lists.denx.de/pipermail/u-boot/2019-September/384906.html
I hope the patch from Dalon get applied to mainline before these patchsets.
In fact, this "CONFIG_OF_EMBED" produce warning during build. Better get rid of this.
On 2/20/20 3:12 AM, Ang, Chee Hong wrote:
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@intel.com
CONFIG_OF_EMBED was primarily enabled to support the agilex spl hex file requirements. Since this option now produces a warning during build, and the spl hex can be created using alternate methods, CONFIG_OF_EMBED is no longer needed.
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com
If I apply just this patch, is the platform still bootable ?
Yes. I tested on the platform. There is a similar patch from Dalon for Stratix10 and it still yet to be applied to mainline: https://lists.denx.de/pipermail/u-boot/2019-September/384906.html
I hope the patch from Dalon get applied to mainline before these patchsets.
In fact, this "CONFIG_OF_EMBED" produce warning during build. Better get rid of this.
If you just remove OF_EMBED, will the DT still be correctly passed in ? The warning is there for a reason and just removing OF_EMBED to silence the warning isn't the right approach. But if this works on Agilex, fine.
On Thu, 2020-02-20 at 17:44 +0100, Marek Vasut wrote:
On 2/20/20 3:12 AM, Ang, Chee Hong wrote:
On 2/19/20 1:25 PM,
mailto:chee.hong.ang@intel.com
chee.hong.ang@intel.com
wrote:
From: Chee Hong Ang <
mailto:chee.hong.ang@intel.com
chee.hong.ang@intel.com
CONFIG_OF_EMBED was primarily enabled to support the agilex spl hex
file requirements. Since this option now produces a warning during
build, and the spl hex can be created using alternate methods,
CONFIG_OF_EMBED is no longer needed.
Signed-off-by: Chee Hong Ang <
mailto:chee.hong.ang@intel.com
chee.hong.ang@intel.com
If I apply just this patch, is the platform still bootable ?
Yes. I tested on the platform.
There is a similar patch from Dalon for Stratix10 and it still yet to be applied
to mainline:
https://lists.denx.de/pipermail/u-boot/2019-September/384906.html
https://lists.denx.de/pipermail/u-boot/2019-September/384906.html
I hope the patch from Dalon get applied to mainline before these patchsets.
In fact, this "CONFIG_OF_EMBED" produce warning during build.
Better get rid of this.
If you just remove OF_EMBED, will the DT still be correctly passed in ?
The warning is there for a reason and just removing OF_EMBED to silence
the warning isn't the right approach. But if this works on Agilex, fine.
Yes, it is fine, the u-boot binary and dtb are just cat'ed together instead.
--dalon
On 2/20/20 6:04 PM, Westergreen, Dalon wrote:
Please fix your mailer, it makes your reply completely unreadable.
On Thu, 2020-02-20 at 17:44 +0100, Marek Vasut wrote:
On 2/20/20 3:12 AM, Ang, Chee Hong wrote:
On 2/19/20 1:25 PM,
mailto:chee.hong.ang@intel.com
chee.hong.ang@intel.com
wrote:
From: Chee Hong Ang <
mailto:chee.hong.ang@intel.com
chee.hong.ang@intel.com
CONFIG_OF_EMBED was primarily enabled to support the agilex spl hex
file requirements. Since this option now produces a warning during
build, and the spl hex can be created using alternate methods,
CONFIG_OF_EMBED is no longer needed.
OK, so this patch removes functionality. Can that functionality be retained ? Or what can be done here?
[...]
On 2/20/20 6:04 PM, Westergreen, Dalon wrote:
Please fix your mailer, it makes your reply completely unreadable.
On Thu, 2020-02-20 at 17:44 +0100, Marek Vasut wrote:
On 2/20/20 3:12 AM, Ang, Chee Hong wrote:
On 2/19/20 1:25 PM,
mailto:chee.hong.ang@intel.com
chee.hong.ang@intel.com
wrote:
From: Chee Hong Ang <
mailto:chee.hong.ang@intel.com
chee.hong.ang@intel.com
CONFIG_OF_EMBED was primarily enabled to support the agilex spl hex
file requirements. Since this option now produces a warning during
build, and the spl hex can be created using alternate methods,
CONFIG_OF_EMBED is no longer needed.
OK, so this patch removes functionality. Can that functionality be retained ? Or what can be done here?
The functionality is no longer required.
[...]
On 2/21/20 7:15 PM, Ang, Chee Hong wrote:
On 2/20/20 6:04 PM, Westergreen, Dalon wrote:
Please fix your mailer, it makes your reply completely unreadable.
On Thu, 2020-02-20 at 17:44 +0100, Marek Vasut wrote:
On 2/20/20 3:12 AM, Ang, Chee Hong wrote:
On 2/19/20 1:25 PM,
mailto:chee.hong.ang@intel.com
chee.hong.ang@intel.com
wrote:
From: Chee Hong Ang <
mailto:chee.hong.ang@intel.com
chee.hong.ang@intel.com
CONFIG_OF_EMBED was primarily enabled to support the agilex spl hex
file requirements. Since this option now produces a warning during
build, and the spl hex can be created using alternate methods,
CONFIG_OF_EMBED is no longer needed.
OK, so this patch removes functionality. Can that functionality be retained ? Or what can be done here?
The functionality is no longer required.
Because you always depend on ATF now ?
On 2/21/20 7:15 PM, Ang, Chee Hong wrote:
On 2/20/20 6:04 PM, Westergreen, Dalon wrote:
Please fix your mailer, it makes your reply completely unreadable.
On Thu, 2020-02-20 at 17:44 +0100, Marek Vasut wrote:
On 2/20/20 3:12 AM, Ang, Chee Hong wrote:
On 2/19/20 1:25 PM,
mailto:chee.hong.ang@intel.com
chee.hong.ang@intel.com
wrote:
From: Chee Hong Ang <
mailto:chee.hong.ang@intel.com
chee.hong.ang@intel.com
CONFIG_OF_EMBED was primarily enabled to support the agilex spl hex
file requirements. Since this option now produces a warning during
build, and the spl hex can be created using alternate methods,
CONFIG_OF_EMBED is no longer needed.
OK, so this patch removes functionality. Can that functionality be retained ? Or what can be done here?
The functionality is no longer required.
Because you always depend on ATF now ?
No. We have 2 different defconfigs in the patchset, one use ATF one without ATF. Both no longer using CONFIG_OF_EMBED and they worked fine.
On 2/24/20 3:26 AM, Ang, Chee Hong wrote:
On 2/21/20 7:15 PM, Ang, Chee Hong wrote:
On 2/20/20 6:04 PM, Westergreen, Dalon wrote:
Please fix your mailer, it makes your reply completely unreadable.
On Thu, 2020-02-20 at 17:44 +0100, Marek Vasut wrote:
On 2/20/20 3:12 AM, Ang, Chee Hong wrote:
On 2/19/20 1:25 PM,
mailto:chee.hong.ang@intel.com
chee.hong.ang@intel.com
wrote:
From: Chee Hong Ang <
mailto:chee.hong.ang@intel.com
chee.hong.ang@intel.com
CONFIG_OF_EMBED was primarily enabled to support the agilex spl hex
file requirements. Since this option now produces a warning during
build, and the spl hex can be created using alternate methods,
CONFIG_OF_EMBED is no longer needed.
OK, so this patch removes functionality. Can that functionality be retained ? Or what can be done here?
The functionality is no longer required.
Because you always depend on ATF now ?
No. We have 2 different defconfigs in the patchset, one use ATF one without ATF. Both no longer using CONFIG_OF_EMBED and they worked fine.
OK. So how is the .hex file generated now ?
On Tue, 2020-02-25 at 18:55 +0100, Marek Vasut wrote:
On 2/24/20 3:26 AM, Ang, Chee Hong wrote:
On 2/21/20 7:15 PM, Ang, Chee Hong wrote:
On 2/20/20 6:04 PM, Westergreen, Dalon wrote:
Please fix your mailer, it makes your reply completely unreadable.
On Thu, 2020-02-20 at 17:44 +0100, Marek Vasut wrote:
On 2/20/20 3:12 AM, Ang, Chee Hong wrote:
On 2/19/20 1:25 PM,
<mailto:
mailto:chee.hong.ang@intel.com
chee.hong.ang@intel.com
mailto:chee.hong.ang@intel.com
chee.hong.ang@intel.com
wrote:
From: Chee Hong Ang <
<mailto:
mailto:chee.hong.ang@intel.com
chee.hong.ang@intel.com
mailto:chee.hong.ang@intel.com
chee.hong.ang@intel.com
CONFIG_OF_EMBED was primarily enabled to support the agilex spl hex
file requirements. Since this option now produces a warning during
build, and the spl hex can be created using alternate methods,
CONFIG_OF_EMBED is no longer needed.
OK, so this patch removes functionality.
Can that functionality be retained ? Or what can be done here?
The functionality is no longer required.
Because you always depend on ATF now ?
No. We have 2 different defconfigs in the patchset, one use ATF one without
ATF. Both no longer using CONFIG_OF_EMBED and they worked fine.
OK. So how is the .hex file generated now ?
In our tree the patch i submitted to generate the hex file via objcopy is being used. I can resubmit this?
Otherwise, people are just manually calling objcopy to convert the cat'ed spl-dtb.bin to a hex file offset to the appropriate address.
--dalon
On 2/25/20 7:26 PM, Westergreen, Dalon wrote:
On Tue, 2020-02-25 at 18:55 +0100, Marek Vasut wrote:
On 2/24/20 3:26 AM, Ang, Chee Hong wrote:
On 2/21/20 7:15 PM, Ang, Chee Hong wrote:
On 2/20/20 6:04 PM, Westergreen, Dalon wrote:
Please fix your mailer, it makes your reply completely unreadable.
Sorry, I'm not reading further, your reply is again completely unreadable :-(
[...]
On Tue, 2020-02-25 at 18:55 +0100, Marek Vasut wrote:
On 2/24/20 3:26 AM, Ang, Chee Hong wrote:
On 2/21/20 7:15 PM, Ang, Chee Hong wrote:
On 2/20/20 6:04 PM, Westergreen, Dalon wrote:
Please fix your mailer, it makes your reply completely unreadable.
On Thu, 2020-02-20 at 17:44 +0100, Marek Vasut wrote:
On 2/20/20 3:12 AM, Ang, Chee Hong wrote:
On 2/19/20 1:25 PM,
mailto:chee.hong.ang@intel.com
chee.hong.ang@intel.com
wrote:
From: Chee Hong Ang <
mailto:chee.hong.ang@intel.com
chee.hong.ang@intel.com
CONFIG_OF_EMBED was primarily enabled to support the agilex spl hex
file requirements. Since this option now produces a warning during
build, and the spl hex can be created using alternate methods,
CONFIG_OF_EMBED is no longer needed.
OK, so this patch removes functionality. Can that functionality be retained ? Or what can be done here?
The functionality is no longer required.
Because you always depend on ATF now ?
No. We have 2 different defconfigs in the patchset, one use ATF one without ATF. Both no longer using CONFIG_OF_EMBED and they worked fine.
OK. So how is the .hex file generated now ?
Sorry, re-installed my OS and neglected to turn off HTML for my email default...
In our tree the patch i submitted to generate the hex file via objcopy is being used. I can resubmit this?
Otherwise, people are just manually calling objcopy to convert the cat'ed spl- dtb.bin to a hex file offset to the appropriate address.
--dalon
From: Chee Hong Ang chee.hong.ang@intel.com
Generate a FIT image for Intel SOCFPGA(64bits) which include U-boot proper, ATF and DTB for U-boot proper.
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com --- board/altera/soc64/its/fit_spl_atf.its | 52 ++++++++++++++++++++++++++++++++++ 1 file changed, 52 insertions(+) create mode 100644 board/altera/soc64/its/fit_spl_atf.its
diff --git a/board/altera/soc64/its/fit_spl_atf.its b/board/altera/soc64/its/fit_spl_atf.its new file mode 100644 index 0000000..8296fc0 --- /dev/null +++ b/board/altera/soc64/its/fit_spl_atf.its @@ -0,0 +1,52 @@ +/* + * Copyright (C) 2019 Intel Corporation. All rights reserved + * + * SPDX-License-Identifier: GPL-2.0 + */ + +/dts-v1/; + +/ { + description = "FIT image with U-Boot proper, ATF bl31, DTB"; + #address-cells = <1>; + + images { + uboot { + description = "U-Boot (64-bit)"; + data = /incbin/("../../../../u-boot-nodtb.bin"); + type = "standalone"; + os = "U-Boot"; + arch = "arm64"; + compression = "none"; + load = <0x00200000>; + }; + + atf { + description = "ARM Trusted Firmware"; + data = /incbin/("../../../../bl31.bin"); + type = "firmware"; + os = "arm-trusted-firmware"; + arch = "arm64"; + compression = "none"; + load = <0x00001000>; + entry = <0x00001000>; + }; + + fdt { + description = "Stratix 10 flat device-tree"; + data = /incbin/("../../../../u-boot.dtb"); + type = "flat_dt"; + compression = "none"; + }; + }; + + configurations { + default = "conf"; + conf { + description = "Intel Stratix 10"; + firmware = "atf"; + loadables = "uboot"; + fdt = "fdt"; + }; + }; +};
From: Chee Hong Ang chee.hong.ang@intel.com
Add board_fit_config_name_match() for matching board name with device tree files in FIT image. This will ensure correct DTB file is loaded for different board type. Currently, we are not supporting multiple device tree files in FIT image therefore this function basically do nothing for now.
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com --- arch/arm/mach-socfpga/board.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/arch/arm/mach-socfpga/board.c b/arch/arm/mach-socfpga/board.c index 7c8c05c..5757041 100644 --- a/arch/arm/mach-socfpga/board.c +++ b/arch/arm/mach-socfpga/board.c @@ -86,3 +86,13 @@ int g_dnl_board_usb_cable_connected(void) return 1; } #endif + +#ifdef CONFIG_SPL_BUILD +__weak int board_fit_config_name_match(const char *name) +{ + /* Just empty function now - can't decide what to choose */ + debug("%s: %s\n", __func__, name); + + return 0; +} +#endif
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@intel.com
Add board_fit_config_name_match() for matching board name with device tree files in FIT image. This will ensure correct DTB file is loaded for different board type. Currently, we are not supporting multiple device tree files in FIT image therefore this function basically do nothing for now.
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com
arch/arm/mach-socfpga/board.c | 10 ++++++++++ 1 file changed, 10 insertions(+)
diff --git a/arch/arm/mach-socfpga/board.c b/arch/arm/mach-socfpga/board.c index 7c8c05c..5757041 100644 --- a/arch/arm/mach-socfpga/board.c +++ b/arch/arm/mach-socfpga/board.c @@ -86,3 +86,13 @@ int g_dnl_board_usb_cable_connected(void) return 1; } #endif
+#ifdef CONFIG_SPL_BUILD +__weak int board_fit_config_name_match(const char *name)
Why is this __weak ?
[...]
On Wed, 2020-02-19 at 18:11 +0100, Marek Vasut wrote:
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@intel.com Add board_fit_config_name_match() for matching board name withdevice tree files in FIT image. This will ensure correct DTBfile is loaded for different board type. Currently, we are notsupporting multiple device tree files in FIT image therefore thisfunction basically do nothing for now. Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com--- arch/arm/mach- socfpga/board.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/arch/arm/mach-socfpga/board.c b/arch/arm/mach- socfpga/board.cindex 7c8c05c..5757041 100644--- a/arch/arm/mach- socfpga/board.c+++ b/arch/arm/mach-socfpga/board.c@@ -86,3 +86,13 @@ int g_dnl_board_usb_cable_connected(void) return 1; } #endif++#ifdef CONFIG_SPL_BUILD+__weak int board_fit_config_name_match(const char *name)
Why is this __weak ?
The intent is to allow users to override this in their specific board implementation.
[...]
On 2/19/20 6:31 PM, Dalon L Westergreen wrote:
On Wed, 2020-02-19 at 18:11 +0100, Marek Vasut wrote:
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@intel.com Add board_fit_config_name_match() for matching board name withdevice tree files in FIT image. This will ensure correct DTBfile is loaded for different board type. Currently, we are notsupporting multiple device tree files in FIT image therefore thisfunction basically do nothing for now. Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com--- arch/arm/mach- socfpga/board.c | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/arch/arm/mach-socfpga/board.c b/arch/arm/mach- socfpga/board.cindex 7c8c05c..5757041 100644--- a/arch/arm/mach- socfpga/board.c+++ b/arch/arm/mach-socfpga/board.c@@ -86,3 +86,13 @@ int g_dnl_board_usb_cable_connected(void) return 1; } #endif++#ifdef CONFIG_SPL_BUILD+__weak int board_fit_config_name_match(const char *name)
Why is this __weak ?
The intent is to allow users to override this in their specific board implementation.
OK. Please add that into the commit message.
From: Chee Hong Ang chee.hong.ang@intel.com
Instead of loading u-boot proper image (u-boot.img), SPL now loads FIT image (u-boot.itb) which includes u-boot proper, ATF and u-boot proper's DTB.
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com --- include/configs/socfpga_soc64_common.h | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/include/configs/socfpga_soc64_common.h b/include/configs/socfpga_soc64_common.h index 87c7345..f035381 100644 --- a/include/configs/socfpga_soc64_common.h +++ b/include/configs/socfpga_soc64_common.h @@ -197,6 +197,10 @@ unsigned int cm_get_l4_sys_free_clk_hz(void);
/* SPL SDMMC boot support */ #define CONFIG_SYS_MMCSD_FS_BOOT_PARTITION 1 +#ifdef CONFIG_SPL_LOAD_FIT +#define CONFIG_SPL_FS_LOAD_PAYLOAD_NAME "u-boot.itb" +#else #define CONFIG_SPL_FS_LOAD_PAYLOAD_NAME "u-boot.img" +#endif
#endif /* __CONFIG_SOCFPGA_SOC64_COMMON_H__ */
From: Chee Hong Ang chee.hong.ang@intel.com
Override 'lowlevel_init' to support booting ATF from SPL on Intel SOCFPGA (64bits) platforms.
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com --- arch/arm/mach-socfpga/Makefile | 2 + arch/arm/mach-socfpga/lowlevel_init.S | 85 +++++++++++++++++++++++++++++++++++ 2 files changed, 87 insertions(+) create mode 100644 arch/arm/mach-socfpga/lowlevel_init.S
diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-socfpga/Makefile index 418f543..3310e92 100644 --- a/arch/arm/mach-socfpga/Makefile +++ b/arch/arm/mach-socfpga/Makefile @@ -29,6 +29,7 @@ endif
ifdef CONFIG_TARGET_SOCFPGA_STRATIX10 obj-y += clock_manager_s10.o +obj-y += lowlevel_init.o obj-y += mailbox_s10.o obj-y += misc_s10.o obj-y += mmu-arm64_s10.o @@ -41,6 +42,7 @@ endif
ifdef CONFIG_TARGET_SOCFPGA_AGILEX obj-y += clock_manager_agilex.o +obj-y += lowlevel_init.o obj-y += mailbox_s10.o obj-y += misc_s10.o obj-y += mmu-arm64_s10.o diff --git a/arch/arm/mach-socfpga/lowlevel_init.S b/arch/arm/mach-socfpga/lowlevel_init.S new file mode 100644 index 0000000..68053a0 --- /dev/null +++ b/arch/arm/mach-socfpga/lowlevel_init.S @@ -0,0 +1,85 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (C) 2019, Intel Corporation + */ + +#include <asm-offsets.h> +#include <config.h> +#include <linux/linkage.h> +#include <asm/macro.h> + +ENTRY(lowlevel_init) + mov x29, lr /* Save LR */ + +#if defined(CONFIG_GICV2) || defined(CONFIG_GICV3) +#ifdef CONFIG_SPL_ATF + branch_if_slave x0, 2f +#else + branch_if_slave x0, 1f +#endif + + ldr x0, =GICD_BASE + bl gic_init_secure +#ifdef CONFIG_SPL_BUILD + b 2f +#else + b 3f +#endif + +1: +#if defined(CONFIG_GICV3) + ldr x0, =GICR_BASE + bl gic_init_secure_percpu +#elif defined(CONFIG_GICV2) + ldr x0, =GICD_BASE + ldr x1, =GICC_BASE + bl gic_init_secure_percpu +#endif +#endif + +#ifdef CONFIG_ARMV8_MULTIENTRY + branch_if_master x0, x1, 3f + + /* + * Slave should wait for master clearing spin table. + * This sync prevent slaves observing incorrect + * value of spin table and jumping to wrong place. + */ +#if defined(CONFIG_GICV2) || defined(CONFIG_GICV3) +#ifdef CONFIG_GICV2 + ldr x0, =GICC_BASE +#endif + bl gic_wait_for_interrupt +#endif + + /* + * All slaves will enter EL2 and optionally EL1. + */ + adr x4, lowlevel_in_el2 + ldr x5, =ES_TO_AARCH64 + bl armv8_switch_to_el2 + +lowlevel_in_el2: +#ifdef CONFIG_ARMV8_SWITCH_TO_EL1 + adr x4, lowlevel_in_el1 + ldr x5, =ES_TO_AARCH64 + bl armv8_switch_to_el1 + +lowlevel_in_el1: +#endif +#endif /* CONFIG_ARMV8_MULTIENTRY */ + +2: +#ifdef CONFIG_SPL_BUILD + ldr x4, =CPU_RELEASE_ADDR + ldr x5, [x4] + cbz x5, checkslavecpu + br x5 +checkslavecpu: + branch_if_slave x0, 2b +#endif + +3: + mov lr, x29 /* Restore LR */ + ret +ENDPROC(lowlevel_init)
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote: [...]
diff --git a/arch/arm/mach-socfpga/lowlevel_init.S b/arch/arm/mach-socfpga/lowlevel_init.S new file mode 100644 index 0000000..68053a0 --- /dev/null +++ b/arch/arm/mach-socfpga/lowlevel_init.S
This should be some lowlevel_init_64.S to make it clear it's only for arm64 platforms.
@@ -0,0 +1,85 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*
- Copyright (C) 2019, Intel Corporation
- */
+#include <asm-offsets.h> +#include <config.h> +#include <linux/linkage.h> +#include <asm/macro.h>
+ENTRY(lowlevel_init)
- mov x29, lr /* Save LR */
+#if defined(CONFIG_GICV2) || defined(CONFIG_GICV3) +#ifdef CONFIG_SPL_ATF
- branch_if_slave x0, 2f
+#else
- branch_if_slave x0, 1f
+#endif
- ldr x0, =GICD_BASE
- bl gic_init_secure
+#ifdef CONFIG_SPL_BUILD
- b 2f
+#else
- b 3f
+#endif
Can't this be done in C code ? Can we reduce the ifdeffery ?
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote: [...]
diff --git a/arch/arm/mach-socfpga/lowlevel_init.S b/arch/arm/mach-socfpga/lowlevel_init.S new file mode 100644 index 0000000..68053a0 --- /dev/null +++ b/arch/arm/mach-socfpga/lowlevel_init.S
This should be some lowlevel_init_64.S to make it clear it's only for arm64 platforms.
OK. It makes sense. Thanks.
@@ -0,0 +1,85 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*
- Copyright (C) 2019, Intel Corporation */
+#include <asm-offsets.h> +#include <config.h> +#include <linux/linkage.h> +#include <asm/macro.h>
+ENTRY(lowlevel_init)
- mov x29, lr /* Save LR */
+#if defined(CONFIG_GICV2) || defined(CONFIG_GICV3) #ifdef +CONFIG_SPL_ATF
- branch_if_slave x0, 2f
+#else
- branch_if_slave x0, 1f
+#endif
- ldr x0, =GICD_BASE
- bl gic_init_secure
+#ifdef CONFIG_SPL_BUILD
- b 2f
+#else
- b 3f
+#endif
Can't this be done in C code ? Can we reduce the ifdeffery ?
This lowlevel_init function is shared by SPL and U-Boot and they run in slightly different flow. I don't think this can be done in C code but let me see what I can do to further optimize the flow to reduce the ifdeffery.
On 2/20/20 3:27 AM, Ang, Chee Hong wrote:
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote: [...]
diff --git a/arch/arm/mach-socfpga/lowlevel_init.S b/arch/arm/mach-socfpga/lowlevel_init.S new file mode 100644 index 0000000..68053a0 --- /dev/null +++ b/arch/arm/mach-socfpga/lowlevel_init.S
This should be some lowlevel_init_64.S to make it clear it's only for arm64 platforms.
OK. It makes sense. Thanks.
@@ -0,0 +1,85 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*
- Copyright (C) 2019, Intel Corporation */
+#include <asm-offsets.h> +#include <config.h> +#include <linux/linkage.h> +#include <asm/macro.h>
+ENTRY(lowlevel_init)
- mov x29, lr /* Save LR */
+#if defined(CONFIG_GICV2) || defined(CONFIG_GICV3) #ifdef +CONFIG_SPL_ATF
- branch_if_slave x0, 2f
+#else
- branch_if_slave x0, 1f
+#endif
- ldr x0, =GICD_BASE
- bl gic_init_secure
+#ifdef CONFIG_SPL_BUILD
- b 2f
+#else
- b 3f
+#endif
Can't this be done in C code ? Can we reduce the ifdeffery ?
This lowlevel_init function is shared by SPL and U-Boot and they run in slightly different flow.
What does this 'different flow' mean ?
I don't think this can be done in C code but let me see what I can do to further optimize the flow to reduce the ifdeffery.
That would be nice, thanks.
On 2/20/20 3:27 AM, Ang, Chee Hong wrote:
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote: [...]
diff --git a/arch/arm/mach-socfpga/lowlevel_init.S b/arch/arm/mach-socfpga/lowlevel_init.S new file mode 100644 index 0000000..68053a0 --- /dev/null +++ b/arch/arm/mach-socfpga/lowlevel_init.S
This should be some lowlevel_init_64.S to make it clear it's only for arm64 platforms.
OK. It makes sense. Thanks.
@@ -0,0 +1,85 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*
- Copyright (C) 2019, Intel Corporation */
+#include <asm-offsets.h> +#include <config.h> +#include <linux/linkage.h> +#include <asm/macro.h>
+ENTRY(lowlevel_init)
- mov x29, lr /* Save LR */
+#if defined(CONFIG_GICV2) || defined(CONFIG_GICV3) #ifdef +CONFIG_SPL_ATF
- branch_if_slave x0, 2f
+#else
- branch_if_slave x0, 1f
+#endif
- ldr x0, =GICD_BASE
- bl gic_init_secure
+#ifdef CONFIG_SPL_BUILD
- b 2f
+#else
- b 3f
+#endif
Can't this be done in C code ? Can we reduce the ifdeffery ?
This lowlevel_init function is shared by SPL and U-Boot and they run in slightly different flow.
What does this 'different flow' mean ?
This has something to with multi-cores CPU such as A53. For SPL, we need to make sure the slave CPUs (CPU1/2/3) trapped in a 'place' Where they could be 'activated' by kernel for multi-processor environment. It means the kernel get to 'activate' the slave CPUs from master CPU (CPU0) U-Boot proper only run on master CPU (CPU0). The rest of slave CPUs are trapped in the beginning of SPL waiting to be 'activated' by kernel.
In U-Boot proper, only master CPU gets to run this code and it will just do the basic GIC setup and skip the 'trap'. The 'trap' is to prevent the slave CPUs from running the same SPL, ATF and U-Boot code as the master CPU in parallel. Only single core (maser CPU) is needed for bootloaders and firmware.
I don't think this can be done in C code but let me see what I can do to further optimize the flow to reduce the ifdeffery.
That would be nice, thanks.
On 2/20/20 8:05 PM, Ang, Chee Hong wrote:
On 2/20/20 3:27 AM, Ang, Chee Hong wrote:
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote: [...]
diff --git a/arch/arm/mach-socfpga/lowlevel_init.S b/arch/arm/mach-socfpga/lowlevel_init.S new file mode 100644 index 0000000..68053a0 --- /dev/null +++ b/arch/arm/mach-socfpga/lowlevel_init.S
This should be some lowlevel_init_64.S to make it clear it's only for arm64 platforms.
OK. It makes sense. Thanks.
@@ -0,0 +1,85 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*
- Copyright (C) 2019, Intel Corporation */
+#include <asm-offsets.h> +#include <config.h> +#include <linux/linkage.h> +#include <asm/macro.h>
+ENTRY(lowlevel_init)
- mov x29, lr /* Save LR */
+#if defined(CONFIG_GICV2) || defined(CONFIG_GICV3) #ifdef +CONFIG_SPL_ATF
- branch_if_slave x0, 2f
+#else
- branch_if_slave x0, 1f
+#endif
- ldr x0, =GICD_BASE
- bl gic_init_secure
+#ifdef CONFIG_SPL_BUILD
- b 2f
+#else
- b 3f
+#endif
Can't this be done in C code ? Can we reduce the ifdeffery ?
This lowlevel_init function is shared by SPL and U-Boot and they run in slightly different flow.
What does this 'different flow' mean ?
This has something to with multi-cores CPU such as A53. For SPL, we need to make sure the slave CPUs (CPU1/2/3) trapped in a 'place' Where they could be 'activated' by kernel for multi-processor environment. It means the kernel get to 'activate' the slave CPUs from master CPU (CPU0) U-Boot proper only run on master CPU (CPU0). The rest of slave CPUs are trapped in the beginning of SPL waiting to be 'activated' by kernel.
OK, so the secondary CPUs are spinning until the kernel releases them.
In U-Boot proper, only master CPU gets to run this code and it will just do the basic GIC setup and skip the 'trap'. The 'trap' is to prevent the slave CPUs from running the same SPL, ATF and U-Boot code as the master CPU in parallel. Only single core (maser CPU) is needed for bootloaders and firmware.
I would expect all the other SMP platforms solved this issue with secondary CPUs already, so why is agilex special ?
On 2/20/20 8:05 PM, Ang, Chee Hong wrote:
On 2/20/20 3:27 AM, Ang, Chee Hong wrote:
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote: [...]
diff --git a/arch/arm/mach-socfpga/lowlevel_init.S b/arch/arm/mach-socfpga/lowlevel_init.S new file mode 100644 index 0000000..68053a0 --- /dev/null +++ b/arch/arm/mach-socfpga/lowlevel_init.S
This should be some lowlevel_init_64.S to make it clear it's only for arm64 platforms.
OK. It makes sense. Thanks.
@@ -0,0 +1,85 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*
- Copyright (C) 2019, Intel Corporation */
+#include <asm-offsets.h> +#include <config.h> +#include <linux/linkage.h> +#include <asm/macro.h>
+ENTRY(lowlevel_init)
- mov x29, lr /* Save LR */
+#if defined(CONFIG_GICV2) || defined(CONFIG_GICV3) #ifdef +CONFIG_SPL_ATF
- branch_if_slave x0, 2f
+#else
- branch_if_slave x0, 1f
+#endif
- ldr x0, =GICD_BASE
- bl gic_init_secure
+#ifdef CONFIG_SPL_BUILD
- b 2f
+#else
- b 3f
+#endif
Can't this be done in C code ? Can we reduce the ifdeffery ?
This lowlevel_init function is shared by SPL and U-Boot and they run in slightly different flow.
What does this 'different flow' mean ?
This has something to with multi-cores CPU such as A53. For SPL, we need to make sure the slave CPUs (CPU1/2/3) trapped in a 'place' Where they could be 'activated' by kernel for multi-processor environment. It means the kernel get to 'activate' the slave CPUs from master CPU (CPU0) U-Boot proper only run on master CPU (CPU0). The rest of slave CPUs are trapped in the beginning of SPL waiting to be 'activated' by kernel.
OK, so the secondary CPUs are spinning until the kernel releases them.
In U-Boot proper, only master CPU gets to run this code and it will just do the basic GIC setup and skip the 'trap'. The 'trap' is to prevent the slave CPUs from running the same SPL, ATF and U-Boot code as the master CPU in parallel. Only single core (maser CPU) is needed for
bootloaders and firmware.
I would expect all the other SMP platforms solved this issue with secondary CPUs already, so why is agilex special ?
Nothing special. Just like other SMP platforms, I am just telling you the master CPU always skips the 'spinning trap' in SPL and U-Boot proper.
On 2/20/20 8:05 PM, Ang, Chee Hong wrote:
On 2/20/20 3:27 AM, Ang, Chee Hong wrote:
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote: [...] > diff --git a/arch/arm/mach-socfpga/lowlevel_init.S > b/arch/arm/mach-socfpga/lowlevel_init.S > new file mode 100644 > index 0000000..68053a0 > --- /dev/null > +++ b/arch/arm/mach-socfpga/lowlevel_init.S
This should be some lowlevel_init_64.S to make it clear it's only for arm64 platforms.
OK. It makes sense. Thanks.
> @@ -0,0 +1,85 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +/* > + * Copyright (C) 2019, Intel Corporation */ > + > +#include <asm-offsets.h> > +#include <config.h> > +#include <linux/linkage.h> > +#include <asm/macro.h> > + > +ENTRY(lowlevel_init) > + mov x29, lr /* Save LR */ > + > +#if defined(CONFIG_GICV2) || defined(CONFIG_GICV3) #ifdef > +CONFIG_SPL_ATF > + branch_if_slave x0, 2f > +#else > + branch_if_slave x0, 1f > +#endif > + > + ldr x0, =GICD_BASE > + bl gic_init_secure > +#ifdef CONFIG_SPL_BUILD > + b 2f > +#else > + b 3f > +#endif
Can't this be done in C code ? Can we reduce the ifdeffery ?
This lowlevel_init function is shared by SPL and U-Boot and they run in slightly different flow.
What does this 'different flow' mean ?
This has something to with multi-cores CPU such as A53. For SPL, we need to make sure the slave CPUs (CPU1/2/3) trapped in a 'place' Where they could be 'activated' by kernel for multi-processor environment. It means the kernel get to 'activate' the slave CPUs from master CPU (CPU0) U-Boot proper only run on master CPU (CPU0). The rest of slave CPUs are trapped in the beginning of SPL waiting to be 'activated' by kernel.
OK, so the secondary CPUs are spinning until the kernel releases them.
In U-Boot proper, only master CPU gets to run this code and it will just do the basic GIC setup and skip the 'trap'. The 'trap' is to prevent the slave CPUs from running the same SPL, ATF and U-Boot code as the master CPU in parallel. Only single core (maser CPU) is needed for
bootloaders and firmware.
I would expect all the other SMP platforms solved this issue with secondary CPUs already, so why is agilex special ?
Nothing special. Just like other SMP platforms, I am just telling you the master CPU always skips the 'spinning trap' in SPL and U-Boot proper.
ATF is now used to activate the secondary CPUs by Linux. This lowlevel_init is to make sure secondary CPUs trap in ATF instead of SPL after ATF is initialized.
On 2/21/20 7:46 PM, Ang, Chee Hong wrote:
On 2/20/20 8:05 PM, Ang, Chee Hong wrote:
On 2/20/20 3:27 AM, Ang, Chee Hong wrote:
> On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote: > [...] >> diff --git a/arch/arm/mach-socfpga/lowlevel_init.S >> b/arch/arm/mach-socfpga/lowlevel_init.S >> new file mode 100644 >> index 0000000..68053a0 >> --- /dev/null >> +++ b/arch/arm/mach-socfpga/lowlevel_init.S > > This should be some lowlevel_init_64.S to make it clear it's only > for > arm64 platforms. OK. It makes sense. Thanks. > >> @@ -0,0 +1,85 @@ >> +/* SPDX-License-Identifier: GPL-2.0 */ >> +/* >> + * Copyright (C) 2019, Intel Corporation */ >> + >> +#include <asm-offsets.h> >> +#include <config.h> >> +#include <linux/linkage.h> >> +#include <asm/macro.h> >> + >> +ENTRY(lowlevel_init) >> + mov x29, lr /* Save LR */ >> + >> +#if defined(CONFIG_GICV2) || defined(CONFIG_GICV3) #ifdef >> +CONFIG_SPL_ATF >> + branch_if_slave x0, 2f >> +#else >> + branch_if_slave x0, 1f >> +#endif >> + >> + ldr x0, =GICD_BASE >> + bl gic_init_secure >> +#ifdef CONFIG_SPL_BUILD >> + b 2f >> +#else >> + b 3f >> +#endif > > Can't this be done in C code ? Can we reduce the ifdeffery ? This lowlevel_init function is shared by SPL and U-Boot and they run in slightly different flow.
What does this 'different flow' mean ?
This has something to with multi-cores CPU such as A53. For SPL, we need to make sure the slave CPUs (CPU1/2/3) trapped in a 'place' Where they could be 'activated' by kernel for multi-processor environment. It means the kernel get to 'activate' the slave CPUs from master CPU (CPU0) U-Boot proper only run on master CPU (CPU0). The rest of slave CPUs are trapped in the beginning of SPL waiting to be 'activated' by kernel.
OK, so the secondary CPUs are spinning until the kernel releases them.
In U-Boot proper, only master CPU gets to run this code and it will just do the basic GIC setup and skip the 'trap'. The 'trap' is to prevent the slave CPUs from running the same SPL, ATF and U-Boot code as the master CPU in parallel. Only single core (maser CPU) is needed for
bootloaders and firmware.
I would expect all the other SMP platforms solved this issue with secondary CPUs already, so why is agilex special ?
Nothing special. Just like other SMP platforms, I am just telling you the master CPU always skips the 'spinning trap' in SPL and U-Boot proper.
ATF is now used to activate the secondary CPUs by Linux. This lowlevel_init is to make sure secondary CPUs trap in ATF instead of SPL after ATF is initialized.
So that's what it is. OK, please document it in the commit message, thanks.
From: Chee Hong Ang chee.hong.ang@intel.com
SPL now loads ATF (BL31), U-Boot proper and DTB from FIT image. The new boot flow with ATF support is as follow:
SPL -> ATF (BL31) -> U-Boot proper -> OS (Linux)
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com --- configs/socfpga_agilex_defconfig | 8 +++++++- configs/socfpga_stratix10_defconfig | 8 +++++++- 2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/configs/socfpga_agilex_defconfig b/configs/socfpga_agilex_defconfig index 693a774..0065ff0 100644 --- a/configs/socfpga_agilex_defconfig +++ b/configs/socfpga_agilex_defconfig @@ -1,6 +1,6 @@ CONFIG_ARM=y CONFIG_ARCH_SOCFPGA=y -CONFIG_SYS_TEXT_BASE=0x1000 +CONFIG_SYS_TEXT_BASE=0x200000 CONFIG_SYS_MALLOC_F_LEN=0x2000 CONFIG_ENV_SIZE=0x1000 CONFIG_ENV_OFFSET=0x200 @@ -10,10 +10,16 @@ CONFIG_TARGET_SOCFPGA_AGILEX_SOCDK=y CONFIG_IDENT_STRING="socfpga_agilex" CONFIG_SPL_FS_FAT=y CONFIG_SPL_TEXT_BASE=0xFFE00000 +CONFIG_FIT=y +CONFIG_SPL_LOAD_FIT=y +CONFIG_SPL_FIT_SOURCE="board/altera/soc64/its/fit_spl_atf.its" CONFIG_BOOTDELAY=5 +CONFIG_SPL_LEGACY_IMAGE_SUPPORT=y CONFIG_SPL_CACHE=y CONFIG_SPL_SPI_LOAD=y CONFIG_SYS_SPI_U_BOOT_OFFS=0x3c00000 +CONFIG_SPL_ATF=y +CONFIG_SPL_ATF_NO_PLATFORM_PARAM=y CONFIG_HUSH_PARSER=y CONFIG_SYS_PROMPT="SOCFPGA_AGILEX # " CONFIG_CMD_MEMTEST=y diff --git a/configs/socfpga_stratix10_defconfig b/configs/socfpga_stratix10_defconfig index 26db40f..4882d50 100644 --- a/configs/socfpga_stratix10_defconfig +++ b/configs/socfpga_stratix10_defconfig @@ -1,6 +1,6 @@ CONFIG_ARM=y CONFIG_ARCH_SOCFPGA=y -CONFIG_SYS_TEXT_BASE=0x1000 +CONFIG_SYS_TEXT_BASE=0x200000 CONFIG_SYS_MALLOC_F_LEN=0x2000 CONFIG_ENV_SIZE=0x1000 CONFIG_ENV_OFFSET=0x200 @@ -10,9 +10,15 @@ CONFIG_TARGET_SOCFPGA_STRATIX10_SOCDK=y CONFIG_IDENT_STRING="socfpga_stratix10" CONFIG_SPL_FS_FAT=y CONFIG_SPL_TEXT_BASE=0xFFE00000 +CONFIG_FIT=y +CONFIG_SPL_LOAD_FIT=y +CONFIG_SPL_FIT_SOURCE="board/altera/soc64/its/fit_spl_atf.its" CONFIG_BOOTDELAY=5 +CONFIG_SPL_LEGACY_IMAGE_SUPPORT=y CONFIG_SPL_SPI_LOAD=y CONFIG_SYS_SPI_U_BOOT_OFFS=0x3C00000 +CONFIG_SPL_ATF=y +CONFIG_SPL_ATF_NO_PLATFORM_PARAM=y CONFIG_HUSH_PARSER=y CONFIG_SYS_PROMPT="SOCFPGA_STRATIX10 # " CONFIG_CMD_MEMTEST=y
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@intel.com
SPL now loads ATF (BL31), U-Boot proper and DTB from FIT image. The new boot flow with ATF support is as follow:
SPL -> ATF (BL31) -> U-Boot proper -> OS (Linux)
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com
configs/socfpga_agilex_defconfig | 8 +++++++- configs/socfpga_stratix10_defconfig | 8 +++++++- 2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/configs/socfpga_agilex_defconfig b/configs/socfpga_agilex_defconfig index 693a774..0065ff0 100644 --- a/configs/socfpga_agilex_defconfig +++ b/configs/socfpga_agilex_defconfig @@ -1,6 +1,6 @@ CONFIG_ARM=y CONFIG_ARCH_SOCFPGA=y -CONFIG_SYS_TEXT_BASE=0x1000 +CONFIG_SYS_TEXT_BASE=0x200000
Why did the text base change ?
CONFIG_SYS_MALLOC_F_LEN=0x2000 CONFIG_ENV_SIZE=0x1000 CONFIG_ENV_OFFSET=0x200 @@ -10,10 +10,16 @@ CONFIG_TARGET_SOCFPGA_AGILEX_SOCDK=y CONFIG_IDENT_STRING="socfpga_agilex" CONFIG_SPL_FS_FAT=y CONFIG_SPL_TEXT_BASE=0xFFE00000 +CONFIG_FIT=y +CONFIG_SPL_LOAD_FIT=y +CONFIG_SPL_FIT_SOURCE="board/altera/soc64/its/fit_spl_atf.its" CONFIG_BOOTDELAY=5 +CONFIG_SPL_LEGACY_IMAGE_SUPPORT=y
Is legacy image support really needed ?
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@intel.com
SPL now loads ATF (BL31), U-Boot proper and DTB from FIT image. The new boot flow with ATF support is as follow:
SPL -> ATF (BL31) -> U-Boot proper -> OS (Linux)
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com
configs/socfpga_agilex_defconfig | 8 +++++++- configs/socfpga_stratix10_defconfig | 8 +++++++- 2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/configs/socfpga_agilex_defconfig b/configs/socfpga_agilex_defconfig index 693a774..0065ff0 100644 --- a/configs/socfpga_agilex_defconfig +++ b/configs/socfpga_agilex_defconfig @@ -1,6 +1,6 @@ CONFIG_ARM=y CONFIG_ARCH_SOCFPGA=y -CONFIG_SYS_TEXT_BASE=0x1000 +CONFIG_SYS_TEXT_BASE=0x200000
Why did the text base change ?
This defconfig support ATF. ATF will occupy from this address range (0x1000) U-Boot now starts at 0x200000.
CONFIG_SYS_MALLOC_F_LEN=0x2000 CONFIG_ENV_SIZE=0x1000 CONFIG_ENV_OFFSET=0x200 @@ -10,10 +10,16 @@ CONFIG_TARGET_SOCFPGA_AGILEX_SOCDK=y CONFIG_IDENT_STRING="socfpga_agilex" CONFIG_SPL_FS_FAT=y CONFIG_SPL_TEXT_BASE=0xFFE00000 +CONFIG_FIT=y +CONFIG_SPL_LOAD_FIT=y +CONFIG_SPL_FIT_SOURCE="board/altera/soc64/its/fit_spl_atf.its" CONFIG_BOOTDELAY=5 +CONFIG_SPL_LEGACY_IMAGE_SUPPORT=y
Is legacy image support really needed ?
Let me check. Will get rid of this if it's not needed. Thanks.
On 2/20/20 3:15 AM, Ang, Chee Hong wrote:
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@intel.com
SPL now loads ATF (BL31), U-Boot proper and DTB from FIT image. The new boot flow with ATF support is as follow:
SPL -> ATF (BL31) -> U-Boot proper -> OS (Linux)
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com
configs/socfpga_agilex_defconfig | 8 +++++++- configs/socfpga_stratix10_defconfig | 8 +++++++- 2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/configs/socfpga_agilex_defconfig b/configs/socfpga_agilex_defconfig index 693a774..0065ff0 100644 --- a/configs/socfpga_agilex_defconfig +++ b/configs/socfpga_agilex_defconfig @@ -1,6 +1,6 @@ CONFIG_ARM=y CONFIG_ARCH_SOCFPGA=y -CONFIG_SYS_TEXT_BASE=0x1000 +CONFIG_SYS_TEXT_BASE=0x200000
Why did the text base change ?
This defconfig support ATF. ATF will occupy from this address range (0x1000) U-Boot now starts at 0x200000.
Then please document it in the commit message.
CONFIG_SYS_MALLOC_F_LEN=0x2000 CONFIG_ENV_SIZE=0x1000 CONFIG_ENV_OFFSET=0x200 @@ -10,10 +10,16 @@ CONFIG_TARGET_SOCFPGA_AGILEX_SOCDK=y CONFIG_IDENT_STRING="socfpga_agilex" CONFIG_SPL_FS_FAT=y CONFIG_SPL_TEXT_BASE=0xFFE00000 +CONFIG_FIT=y +CONFIG_SPL_LOAD_FIT=y +CONFIG_SPL_FIT_SOURCE="board/altera/soc64/its/fit_spl_atf.its" CONFIG_BOOTDELAY=5 +CONFIG_SPL_LEGACY_IMAGE_SUPPORT=y
Is legacy image support really needed ?
Let me check. Will get rid of this if it's not needed. Thanks.
Thanks
Am 20.02.2020 um 17:45 schrieb Marek Vasut:
On 2/20/20 3:15 AM, Ang, Chee Hong wrote:
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@intel.com
SPL now loads ATF (BL31), U-Boot proper and DTB from FIT image. The new boot flow with ATF support is as follow:
SPL -> ATF (BL31) -> U-Boot proper -> OS (Linux)
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com
configs/socfpga_agilex_defconfig | 8 +++++++- configs/socfpga_stratix10_defconfig | 8 +++++++- 2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/configs/socfpga_agilex_defconfig b/configs/socfpga_agilex_defconfig index 693a774..0065ff0 100644 --- a/configs/socfpga_agilex_defconfig +++ b/configs/socfpga_agilex_defconfig @@ -1,6 +1,6 @@ CONFIG_ARM=y CONFIG_ARCH_SOCFPGA=y -CONFIG_SYS_TEXT_BASE=0x1000 +CONFIG_SYS_TEXT_BASE=0x200000
Why did the text base change ?
This defconfig support ATF. ATF will occupy from this address range (0x1000) U-Boot now starts at 0x200000.
Then please document it in the commit message.
Or even better, could we have 2 defconfigs, one for ATF, one for non-ATF? That way, we get build coverage that this still works.
Regards, Simon
CONFIG_SYS_MALLOC_F_LEN=0x2000 CONFIG_ENV_SIZE=0x1000 CONFIG_ENV_OFFSET=0x200 @@ -10,10 +10,16 @@ CONFIG_TARGET_SOCFPGA_AGILEX_SOCDK=y CONFIG_IDENT_STRING="socfpga_agilex" CONFIG_SPL_FS_FAT=y CONFIG_SPL_TEXT_BASE=0xFFE00000 +CONFIG_FIT=y +CONFIG_SPL_LOAD_FIT=y +CONFIG_SPL_FIT_SOURCE="board/altera/soc64/its/fit_spl_atf.its" CONFIG_BOOTDELAY=5 +CONFIG_SPL_LEGACY_IMAGE_SUPPORT=y
Is legacy image support really needed ?
Let me check. Will get rid of this if it's not needed. Thanks.
Thanks
On Thu, 2020-02-20 at 21:00 +0100, Simon Goldschmidt wrote:
Am 20.02.2020 um 17:45 schrieb Marek Vasut:
On 2/20/20 3:15 AM, Ang, Chee Hong wrote:
On 2/19/20 1:25 PM,
mailto:chee.hong.ang@intel.com
chee.hong.ang@intel.com
wrote:
From: Chee Hong Ang <
mailto:chee.hong.ang@intel.com
chee.hong.ang@intel.com
SPL now loads ATF (BL31), U-Boot proper and DTB from FIT image. The
new boot flow with ATF support is as follow:
SPL -> ATF (BL31) -> U-Boot proper -> OS (Linux)
Signed-off-by: Chee Hong Ang <
mailto:chee.hong.ang@intel.com
chee.hong.ang@intel.com
---
configs/socfpga_agilex_defconfig | 8 +++++++-
configs/socfpga_stratix10_defconfig | 8 +++++++-
2 files changed, 14 insertions(+), 2 deletions(-)
diff --git a/configs/socfpga_agilex_defconfig
b/configs/socfpga_agilex_defconfig
index 693a774..0065ff0 100644
--- a/configs/socfpga_agilex_defconfig
+++ b/configs/socfpga_agilex_defconfig
@@ -1,6 +1,6 @@
CONFIG_ARM=y
CONFIG_ARCH_SOCFPGA=y
-CONFIG_SYS_TEXT_BASE=0x1000
+CONFIG_SYS_TEXT_BASE=0x200000
Why did the text base change ?
This defconfig support ATF.
ATF will occupy from this address range (0x1000)
U-Boot now starts at 0x200000.
Then please document it in the commit message.
Or even better, could we have 2 defconfigs, one for ATF, one for
non-ATF? That way, we get build coverage that this still works.
This patch set does already add a separate defconfig for non-ATF in 21/21
--dalon
Regards,
Simon
CONFIG_SYS_MALLOC_F_LEN=0x2000
CONFIG_ENV_SIZE=0x1000
CONFIG_ENV_OFFSET=0x200
@@ -10,10 +10,16 @@ CONFIG_TARGET_SOCFPGA_AGILEX_SOCDK=y
CONFIG_IDENT_STRING="socfpga_agilex"
CONFIG_SPL_FS_FAT=y
CONFIG_SPL_TEXT_BASE=0xFFE00000
+CONFIG_FIT=y
+CONFIG_SPL_LOAD_FIT=y
+CONFIG_SPL_FIT_SOURCE="board/altera/soc64/its/fit_spl_atf.its"
CONFIG_BOOTDELAY=5
+CONFIG_SPL_LEGACY_IMAGE_SUPPORT=y
Is legacy image support really needed ?
Let me check. Will get rid of this if it's not needed. Thanks.
Thanks
From: Chee Hong Ang chee.hong.ang@intel.com
Standard PSCI function "CPU_ON" provided by ATF is now used by Linux kernel to bring up the secondary CPUs to enable SMP booting in Linux on SoC 64bits platform.
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com --- arch/arm/mach-socfpga/Kconfig | 2 -- 1 file changed, 2 deletions(-)
diff --git a/arch/arm/mach-socfpga/Kconfig b/arch/arm/mach-socfpga/Kconfig index 969698c..6549033 100644 --- a/arch/arm/mach-socfpga/Kconfig +++ b/arch/arm/mach-socfpga/Kconfig @@ -33,7 +33,6 @@ config TARGET_SOCFPGA_AGILEX bool select ARMV8_MULTIENTRY select ARMV8_SET_SMPEN - select ARMV8_SPIN_TABLE select CLK select NCORE_CACHE select SPL_CLK if SPL @@ -77,7 +76,6 @@ config TARGET_SOCFPGA_STRATIX10 bool select ARMV8_MULTIENTRY select ARMV8_SET_SMPEN - select ARMV8_SPIN_TABLE select FPGA_STRATIX10
choice
From: Chee Hong Ang chee.hong.ang@intel.com
Allow U-Boot proper running in non-secure mode (EL2) to invoke SMC call to ATF's PSCI runtime services such as System Manager's registers access, 2nd phase bitstream FPGA reconfiguration, Remote System Update (RSU) and etc.
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com --- arch/arm/mach-socfpga/include/mach/misc.h | 3 +++ arch/arm/mach-socfpga/misc_s10.c | 26 ++++++++++++++++++++++++++ 2 files changed, 29 insertions(+)
diff --git a/arch/arm/mach-socfpga/include/mach/misc.h b/arch/arm/mach-socfpga/include/mach/misc.h index f6de1cc..b5625e1 100644 --- a/arch/arm/mach-socfpga/include/mach/misc.h +++ b/arch/arm/mach-socfpga/include/mach/misc.h @@ -43,4 +43,7 @@ void do_bridge_reset(int enable, unsigned int mask); void socfpga_pl310_clear(void); void socfpga_get_managers_addr(void);
+#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) +int invoke_smc(u32 func_id, u64 *args, int arg_len, u64 *ret_arg, int ret_len); +#endif #endif /* _SOCFPGA_MISC_H_ */ diff --git a/arch/arm/mach-socfpga/misc_s10.c b/arch/arm/mach-socfpga/misc_s10.c index a3f5b43..25c3ff6 100644 --- a/arch/arm/mach-socfpga/misc_s10.c +++ b/arch/arm/mach-socfpga/misc_s10.c @@ -164,3 +164,29 @@ void do_bridge_reset(int enable, unsigned int mask)
socfpga_bridges_reset(enable); } + +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) +int invoke_smc(u32 func_id, u64 *args, int arg_len, u64 *ret_arg, int ret_len) +{ + int i; + struct pt_regs regs; + + memset(®s, 0, sizeof(regs)); + + regs.regs[0] = func_id; + + if (args) { + for (i = 0; i < arg_len; i++) + regs.regs[i + 1] = args[i]; + } + + smc_call(®s); + + if (ret_arg) { + for (i = 0; i < ret_len; i++) + ret_arg[i] = regs.regs[i + 1]; + } + + return regs.regs[0]; +} +#endif
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote: [...]
+#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) +int invoke_smc(u32 func_id, u64 *args, int arg_len, u64 *ret_arg, int ret_len) +{
- int i;
- struct pt_regs regs;
- memset(®s, 0, sizeof(regs));
- regs.regs[0] = func_id;
- if (args) {
for (i = 0; i < arg_len; i++)regs.regs[i + 1] = args[i];
Is this memcpy() ?
- }
- smc_call(®s);
- if (ret_arg) {
for (i = 0; i < ret_len; i++)ret_arg[i] = regs.regs[i + 1];
memcpy() ?
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote: [...]
+#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) int +invoke_smc(u32 func_id, u64 *args, int arg_len, u64 *ret_arg, int +ret_len) {
- int i;
- struct pt_regs regs;
- memset(®s, 0, sizeof(regs));
- regs.regs[0] = func_id;
- if (args) {
for (i = 0; i < arg_len; i++)regs.regs[i + 1] = args[i];Is this memcpy() ?
Will change this.
- }
- smc_call(®s);
- if (ret_arg) {
for (i = 0; i < ret_len; i++)ret_arg[i] = regs.regs[i + 1];memcpy() ?
Will change this too. Thanks.
From: Chee Hong Ang chee.hong.ang@intel.com
This header file defines the Secure Monitor Call (SMC) message protocol for ATF (BL31) PSCI runtime services. It includes all the PSCI SiP function identifiers for the secure runtime services provided by ATF. The secure runtime services include System Manager's registers access, 2nd phase bitstream FPGA reconfiguration, Remote System Update (RSU) and etc.
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com --- include/linux/intel-smc.h | 374 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 374 insertions(+) create mode 100644 include/linux/intel-smc.h
diff --git a/include/linux/intel-smc.h b/include/linux/intel-smc.h new file mode 100644 index 0000000..27710ac --- /dev/null +++ b/include/linux/intel-smc.h @@ -0,0 +1,374 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (C) 2017-2018, Intel Corporation + */ + +#ifndef __INTEL_SMC_H +#define __INTEL_SMC_H + +#include <linux/arm-smccc.h> +#include <linux/bitops.h> + +/* + * This file defines the Secure Monitor Call (SMC) message protocol used for + * service layer driver in normal world (EL1) to communicate with secure + * monitor software in Secure Monitor Exception Level 3 (EL3). + * + * This file is shared with secure firmware (FW) which is out of kernel tree. + * + * An ARM SMC instruction takes a function identifier and up to 6 64-bit + * register values as arguments, and can return up to 4 64-bit register + * value. The operation of the secure monitor is determined by the parameter + * values passed in through registers. + + * EL1 and EL3 communicates pointer as physical address rather than the + * virtual address. + */ + +/* + * Functions specified by ARM SMC Calling convention: + * + * FAST call executes atomic operations, returns when the requested operation + * has completed. + * STD call starts a operation which can be preempted by a non-secure + * interrupt. The call can return before the requested operation has + * completed. + * + * a0..a7 is used as register names in the descriptions below, on arm32 + * that translates to r0..r7 and on arm64 to w0..w7. + */ + +#define INTEL_SIP_SMC_STD_CALL_VAL(func_num) \ + ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL, ARM_SMCCC_SMC_64, \ + ARM_SMCCC_OWNER_SIP, (func_num)) + +#define INTEL_SIP_SMC_FAST_CALL_VAL(func_num) \ + ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, ARM_SMCCC_SMC_64, \ + ARM_SMCCC_OWNER_SIP, (func_num)) + +/* + * Return values in INTEL_SIP_SMC_* call + * + * INTEL_SIP_SMC_RETURN_UNKNOWN_FUNCTION: + * Secure monitor software doesn't recognize the request. + * + * INTEL_SIP_SMC_STATUS_OK: + * FPGA configuration completed successfully, + * In case of FPGA configuration write operation, it means secure monitor + * software can accept the next chunk of FPGA configuration data. + * + * INTEL_SIP_SMC_FPGA_CONFIG_STATUS_BUSY: + * In case of FPGA configuration write operation, it means secure monitor + * software is still processing previous data & can't accept the next chunk + * of data. Service driver needs to issue + * INTEL_SIP_SMC_FPGA_CONFIG_COMPLETED_WRITE call to query the + * completed block(s). + * + * INTEL_SIP_SMC_FPGA_CONFIG_STATUS_ERROR: + * There is error during the FPGA configuration process. + * + * INTEL_SIP_SMC_REG_ERROR: + * There is error during a read or write operation of the protected + * registers. + */ +#define INTEL_SIP_SMC_RETURN_UNKNOWN_FUNCTION 0xFFFFFFFF +#define INTEL_SIP_SMC_STATUS_OK 0x0 +#define INTEL_SIP_SMC_FPGA_CONFIG_STATUS_BUSY 0x1 +#define INTEL_SIP_SMC_FPGA_CONFIG_STATUS_REJECTED 0x2 +#define INTEL_SIP_SMC_FPGA_CONFIG_STATUS_ERROR 0x4 +#define INTEL_SIP_SMC_REG_ERROR 0x5 +#define INTEL_SIP_SMC_RSU_ERROR 0x7 + +/* + * Request INTEL_SIP_SMC_FPGA_CONFIG_START + * + * Sync call used by service driver at EL1 to request the FPGA in EL3 to + * be prepare to receive a new configuration. + * + * Call register usage: + * a0: INTEL_SIP_SMC_FPGA_CONFIG_START. + * a1: flag for full or partial configuration + * 0 full reconfiguration. + * 1 partial reconfiguration. + * a2-7: not used. + * + * Return status: + * a0: INTEL_SIP_SMC_STATUS_OK, or INTEL_SIP_SMC_FPGA_CONFIG_STATUS_ERROR. + * a1-3: not used. + */ +#define INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_START 1 +#define INTEL_SIP_SMC_FPGA_CONFIG_START \ + INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_START) + +/* + * Request INTEL_SIP_SMC_FPGA_CONFIG_WRITE + * + * Async call used by service driver at EL1 to provide FPGA configuration data + * to secure world. + * + * Call register usage: + * a0: INTEL_SIP_SMC_FPGA_CONFIG_WRITE. + * a1: 64bit physical address of the configuration data memory block + * a2: Size of configuration data block. + * a3-7: not used. + * + * Return status: + * a0: INTEL_SIP_SMC_STATUS_OK, INTEL_SIP_SMC_FPGA_CONFIG_STATUS_BUSY or + * INTEL_SIP_SMC_FPGA_CONFIG_STATUS_ERROR. + * a1: 64bit physical address of 1st completed memory block if any completed + * block, otherwise zero value. + * a2: 64bit physical address of 2nd completed memory block if any completed + * block, otherwise zero value. + * a3: 64bit physical address of 3rd completed memory block if any completed + * block, otherwise zero value. + */ +#define INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_WRITE 2 +#define INTEL_SIP_SMC_FPGA_CONFIG_WRITE \ + INTEL_SIP_SMC_STD_CALL_VAL(INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_WRITE) + +/* + * Request INTEL_SIP_SMC_FPGA_CONFIG_COMPLETED_WRITE + * + * Sync call used by service driver at EL1 to track the completed write + * transactions. This request is called after INTEL_SIP_SMC_FPGA_CONFIG_WRITE + * call returns INTEL_SIP_SMC_FPGA_CONFIG_STATUS_BUSY. + * + * Call register usage: + * a0: INTEL_SIP_SMC_FPGA_CONFIG_COMPLETED_WRITE. + * a1-7: not used. + * + * Return status: + * a0: INTEL_SIP_SMC_STATUS_OK, INTEL_SIP_SMC_FPGA_CONFIG_STATUS_BUSY or + * INTEL_SIP_SMC_FPGA_CONFIG_STATUS_ERROR. + * a1: 64bit physical address of 1st completed memory block. + * a2: 64bit physical address of 2nd completed memory block if + * any completed block, otherwise zero value. + * a3: 64bit physical address of 3rd completed memory block if + * any completed block, otherwise zero value. + */ +#define INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_COMPLETED_WRITE 3 +#define INTEL_SIP_SMC_FPGA_CONFIG_COMPLETED_WRITE \ +INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_COMPLETED_WRITE) + +/* + * Request INTEL_SIP_SMC_FPGA_CONFIG_ISDONE + * + * Sync call used by service driver at EL1 to inform secure world that all + * data are sent, to check whether or not the secure world had completed + * the FPGA configuration process. + * Set 1 in 2nd argument (a1) to query the status of the first phase FPGA + * configuration by sending MBOX_CONFIG_STATUS to SDM. Otherwise this sync + * call always query the status of 2nd phase FPGA reconfiguration by sending + * MBOX_RECONFIG_STATUS to SDM. + * + * Call register usage: + * a0: INTEL_SIP_SMC_FPGA_CONFIG_ISDONE. + * a1: set 1 to send MBOX_CONFIG_STATUS otherwise send MBOX_RECONFIG_STATUS + * a2-7: not used. + * + * Return status: + * a0: INTEL_SIP_SMC_STATUS_OK, INTEL_SIP_SMC_FPGA_CONFIG_STATUS_BUSY or + * INTEL_SIP_SMC_FPGA_CONFIG_STATUS_ERROR. + * a1-3: not used. + */ +#define INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_ISDONE 4 +#define INTEL_SIP_SMC_FPGA_CONFIG_ISDONE \ + INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_ISDONE) + +/* + * Request INTEL_SIP_SMC_FPGA_CONFIG_GET_MEM + * + * Sync call used by service driver at EL1 to query the physical address of + * memory block reserved by secure monitor software. + * + * Call register usage: + * a0:INTEL_SIP_SMC_FPGA_CONFIG_GET_MEM. + * a1-7: not used. + * + * Return status: + * a0: INTEL_SIP_SMC_STATUS_OK or INTEL_SIP_SMC_FPGA_CONFIG_STATUS_ERROR. + * a1: start of physical address of reserved memory block. + * a2: size of reserved memory block. + * a3: not used. + */ +#define INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_GET_MEM 5 +#define INTEL_SIP_SMC_FPGA_CONFIG_GET_MEM \ + INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_GET_MEM) + +/* + * Request INTEL_SIP_SMC_REG_READ + * + * Read a protected register using SMCCC + * + * Call register usage: + * a0: INTEL_SIP_SMC_REG_READ. + * a1: register address. + * a2-7: not used. + * + * Return status: + * a0: INTEL_SIP_SMC_STATUS_OK or INTEL_SIP_SMC_REG_ERROR. + * a1: Value in the register + * a2-3: not used. + */ +#define INTEL_SIP_SMC_FUNCID_REG_READ 7 +#define INTEL_SIP_SMC_REG_READ \ + INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_REG_READ) + +/* + * Request INTEL_SIP_SMC_REG_WRITE + * + * Write a protected register using SMCCC + * + * Call register usage: + * a0: INTEL_SIP_SMC_REG_WRITE. + * a1: register address + * a2: value to program into register. + * a3-7: not used. + * + * Return status: + * a0: INTEL_SIP_SMC_STATUS_OK or INTEL_SIP_SMC_REG_ERROR. + * a1-3: not used. + */ +#define INTEL_SIP_SMC_FUNCID_REG_WRITE 8 +#define INTEL_SIP_SMC_REG_WRITE \ + INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_REG_WRITE) + +/* + * Request INTEL_SIP_SMC_FUNCID_REG_UPDATE + * + * Update one or more bits in a protected register using a + * read-modify-write operation. + * + * Call register usage: + * a0: INTEL_SIP_SMC_REG_UPDATE. + * a1: register address + * a2: Write Mask. + * a3: Value to write. + * a4-7: not used. + * + * Return status: + * a0: INTEL_SIP_SMC_STATUS_OK or INTEL_SIP_SMC_REG_ERROR. + * a1-3: Not used. + */ +#define INTEL_SIP_SMC_FUNCID_REG_UPDATE 9 +#define INTEL_SIP_SMC_REG_UPDATE \ + INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_REG_UPDATE) + +/* + * Request INTEL_SIP_SMC_RSU_STATUS + * + * Sync call used by service driver at EL1 to query the RSU status + * + * Call register usage: + * a0 INTEL_SIP_SMC_RSU_STATUS + * a1-7 not used + * + * Return status + * a0: Current Image + * a1: Last Failing Image + * a2: Version [width 32 bit] | State [width 32 bit] + * a3: Error details [width 32 bit] | Error location [width 32 bit] + * + * Or + * + * a0: INTEL_SIP_SMC_RSU_ERROR + */ +#define INTEL_SIP_SMC_FUNCID_RSU_STATUS 11 +#define INTEL_SIP_SMC_RSU_STATUS \ + INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_RSU_STATUS) + +/* + * Request INTEL_SIP_SMC_RSU_UPDATE + * + * Sync call used by service driver at EL1 to tell you next reboot is RSU_UPDATE + * + * Call register usage: + * a0 INTEL_SIP_SMC_RSU_UPDATE + * a1 64bit physical address of the configuration data memory in flash + * a2-7 not used + * + * Return status + * a0 INTEL_SIP_SMC_STATUS_OK + */ +#define INTEL_SIP_SMC_FUNCID_RSU_UPDATE 12 +#define INTEL_SIP_SMC_RSU_UPDATE \ + INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_RSU_UPDATE) + +/* + * Request INTEL_SIP_SMC_ECC_DBE + * + * Sync call used by service driver at EL1 alert EL3 that a Double Bit + * ECC error has occurred. + * + * Call register usage: + * a0 INTEL_SIP_SMC_ECC_DBE + * a1 SysManager Double Bit Error value + * a2-7 not used + * + * Return status + * a0 INTEL_SIP_SMC_STATUS_OK + */ +#define INTEL_SIP_SMC_FUNCID_ECC_DBE 13 +#define INTEL_SIP_SMC_ECC_DBE \ + INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_ECC_DBE) + +/* + * Request INTEL_SIP_SMC_RSU_NOTIFY + * + * Sync call used by service driver at EL1 to report HPS software execution + * stage + * + * Call register usage: + * a0 INTEL_SIP_SMC_RSU_NOTIFY + * a1 32bit HPS software execution stage + * a2-7 not used + * + * Return status + * a0 INTEL_SIP_SMC_STATUS_OK or INTEL_SIP_SMC_REG_ERROR. + */ +#define INTEL_SIP_SMC_FUNCID_RSU_NOTIFY 14 +#define INTEL_SIP_SMC_RSU_NOTIFY \ + INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_RSU_NOTIFY) + +/* + * Request INTEL_SIP_SMC_RSU_RETRY_COUNTER + * + * Sync call used by service driver at EL1 to query the RSU retry counter + * + * Call register usage: + * a0 INTEL_SIP_SMC_RSU_RETRY_COUNTER + * a1-7 not used + * + * Return status + * a0 INTEL_SIP_SMC_STATUS_OK or INTEL_SIP_SMC_RSU_ERROR. + * a1 retry counter + */ +#define INTEL_SIP_SMC_FUNCID_RSU_RETRY_COUNTER 15 +#define INTEL_SIP_SMC_RSU_RETRY_COUNTER \ + INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_RSU_RETRY_COUNTER) + +/* + * Request INTEL_SIP_SMC_MBOX_SEND_CMD + * + * Sync call used by service driver at EL1 to send mailbox command to SDM + * + * Call register usage: + * a0 INTEL_SIP_SMC_MBOX_SEND_CMD + * a1 Mailbox command + * a2 64bit physical address pointer to command's arguments + * a3 Length of the argument + * a4 Urgent command enable = 1 / disable = 0 + * a5 64bit physical address pointer to a buffer for receiving responses + * a6 Length of the buffer + * + * Return status + * a0 INTEL_SIP_SMC_STATUS_OK or INTEL_SIP_SMC_RSU_ERROR. + * a1 Status of mailbox response + * a2 64bit physical address pointer to a buffer for receiving responses + * a3 Received length in the buffer + */ +#define INTEL_SIP_SMC_FUNCID_MBOX_SEND_CMD 30 +#define INTEL_SIP_SMC_MBOX_SEND_CMD \ + INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_MBOX_SEND_CMD) + +#endif
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote: [...]
+++ b/include/linux/intel-smc.h @@ -0,0 +1,374 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*
- Copyright (C) 2017-2018, Intel Corporation
2020 ?
[...]
- */
+#define INTEL_SIP_SMC_RETURN_UNKNOWN_FUNCTION 0xFFFFFFFF +#define INTEL_SIP_SMC_STATUS_OK 0x0 +#define INTEL_SIP_SMC_FPGA_CONFIG_STATUS_BUSY 0x1 +#define INTEL_SIP_SMC_FPGA_CONFIG_STATUS_REJECTED 0x2 +#define INTEL_SIP_SMC_FPGA_CONFIG_STATUS_ERROR 0x4 +#define INTEL_SIP_SMC_REG_ERROR 0x5 +#define INTEL_SIP_SMC_RSU_ERROR 0x7
Indent with tabs
[...]
+/*
- Request INTEL_SIP_SMC_MBOX_SEND_CMD
- Sync call used by service driver at EL1 to send mailbox command to SDM
- Call register usage:
- a0 INTEL_SIP_SMC_MBOX_SEND_CMD
- a1 Mailbox command
- a2 64bit physical address pointer to command's arguments
- a3 Length of the argument
- a4 Urgent command enable = 1 / disable = 0
- a5 64bit physical address pointer to a buffer for receiving responses
- a6 Length of the buffer
- Return status
- a0 INTEL_SIP_SMC_STATUS_OK or INTEL_SIP_SMC_RSU_ERROR.
- a1 Status of mailbox response
- a2 64bit physical address pointer to a buffer for receiving responses
- a3 Received length in the buffer
- */
+#define INTEL_SIP_SMC_FUNCID_MBOX_SEND_CMD 30 +#define INTEL_SIP_SMC_MBOX_SEND_CMD \
- INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_MBOX_SEND_CMD)
Is this macro a function call or a value ?
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote: [...]
+++ b/include/linux/intel-smc.h @@ -0,0 +1,374 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*
- Copyright (C) 2017-2018, Intel Corporation
2020 ?
This file is new in U-Boot but it already exists in Linux kernel and adapted from there. Should I change it to 2020 ?
[...]
- */
+#define INTEL_SIP_SMC_RETURN_UNKNOWN_FUNCTION
0xFFFFFFFF
+#define INTEL_SIP_SMC_STATUS_OK 0x0 +#define INTEL_SIP_SMC_FPGA_CONFIG_STATUS_BUSY 0x1 +#define INTEL_SIP_SMC_FPGA_CONFIG_STATUS_REJECTED 0x2 +#define INTEL_SIP_SMC_FPGA_CONFIG_STATUS_ERROR 0x4 +#define INTEL_SIP_SMC_REG_ERROR 0x5 +#define INTEL_SIP_SMC_RSU_ERROR 0x7
Indent with tabs
OK.
[...]
+/*
- Request INTEL_SIP_SMC_MBOX_SEND_CMD
- Sync call used by service driver at EL1 to send mailbox command to SDM
- Call register usage:
- a0 INTEL_SIP_SMC_MBOX_SEND_CMD
- a1 Mailbox command
- a2 64bit physical address pointer to command's arguments
- a3 Length of the argument
- a4 Urgent command enable = 1 / disable = 0
- a5 64bit physical address pointer to a buffer for receiving responses
- a6 Length of the buffer
- Return status
- a0 INTEL_SIP_SMC_STATUS_OK or INTEL_SIP_SMC_RSU_ERROR.
- a1 Status of mailbox response
- a2 64bit physical address pointer to a buffer for receiving responses
- a3 Received length in the buffer
- */
+#define INTEL_SIP_SMC_FUNCID_MBOX_SEND_CMD 30 +#define INTEL_SIP_SMC_MBOX_SEND_CMD \
INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_MBOX_SEN D_CMD)
Is this macro a function call or a value ?
It adds some value on top of ' INTEL_SIP_SMC_FUNCID_MBOX_SEND_CMD ' through some bitwise operation to produce a final value. So it's a value.
On 2/20/20 2:42 AM, Ang, Chee Hong wrote:
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote: [...]
+++ b/include/linux/intel-smc.h @@ -0,0 +1,374 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/*
- Copyright (C) 2017-2018, Intel Corporation
2020 ?
This file is new in U-Boot but it already exists in Linux kernel and adapted from there. Should I change it to 2020 ?
Then it's 2017-2020 . [...]
From: Chee Hong Ang chee.hong.ang@intel.com
These secure register access functions allow U-Boot proper running at EL2 (non-secure) to access System Manager's secure registers by calling the ATF's PSCI runtime services (EL3/secure). If these helper functions are called from secure mode (EL3), the helper function will direct access the secure registers without calling the ATF's PSCI runtime services.
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com --- arch/arm/mach-socfpga/Makefile | 6 +++ .../mach-socfpga/include/mach/secure_reg_helper.h | 20 ++++++++ arch/arm/mach-socfpga/secure_reg_helper.c | 57 ++++++++++++++++++++++ 3 files changed, 83 insertions(+) create mode 100644 arch/arm/mach-socfpga/include/mach/secure_reg_helper.h create mode 100644 arch/arm/mach-socfpga/secure_reg_helper.c
diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-socfpga/Makefile index 3310e92..e59587b 100644 --- a/arch/arm/mach-socfpga/Makefile +++ b/arch/arm/mach-socfpga/Makefile @@ -53,6 +53,12 @@ obj-y += wrap_pinmux_config_s10.o obj-y += wrap_pll_config_s10.o endif
+ifndef CONFIG_SPL_BUILD +ifdef CONFIG_SPL_ATF +obj-y += secure_reg_helper.o +endif +endif + ifdef CONFIG_SPL_BUILD ifdef CONFIG_TARGET_SOCFPGA_GEN5 obj-y += spl_gen5.o diff --git a/arch/arm/mach-socfpga/include/mach/secure_reg_helper.h b/arch/arm/mach-socfpga/include/mach/secure_reg_helper.h new file mode 100644 index 0000000..de581fc --- /dev/null +++ b/arch/arm/mach-socfpga/include/mach/secure_reg_helper.h @@ -0,0 +1,20 @@ +/* SPDX-License-Identifier: GPL-2.0 + * + * Copyright (C) 2019 Intel Corporation <www.intel.com> + * + */ + +#ifndef _SECURE_REG_HELPER_H_ +#define _SECURE_REG_HELPER_H_ + +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr); +void socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr); +void socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 mask, u32 val); +#else +#define socfpga_secure_reg_read32 readl +#define socfpga_secure_reg_write32 writel +#define socfpga_secure_reg_update32 clrsetbits_le32 +#endif + +#endif /* _SECURE_REG_HELPER_H_ */ diff --git a/arch/arm/mach-socfpga/secure_reg_helper.c b/arch/arm/mach-socfpga/secure_reg_helper.c new file mode 100644 index 0000000..46658a2 --- /dev/null +++ b/arch/arm/mach-socfpga/secure_reg_helper.c @@ -0,0 +1,57 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2019 Intel Corporation <www.intel.com> + * + */ + +#include <common.h> +#include <hang.h> +#include <asm/io.h> +#include <asm/system.h> +#include <asm/arch/misc.h> +#include <linux/intel-smc.h> + +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr) +{ + int ret; + u64 ret_arg; + u64 args[1]; + + args[0] = (u64)reg_addr; + ret = invoke_smc(INTEL_SIP_SMC_REG_READ, args, 1, &ret_arg, 1); + if (ret) { + /* SMC call return error */ + hang(); + } + + return ret_arg; +} + +void socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr) +{ + int ret; + u64 args[2]; + + args[0] = (u64)reg_addr; + args[1] = val; + ret = invoke_smc(INTEL_SIP_SMC_REG_WRITE, args, 2, NULL, 0); + if (ret) { + /* SMC call return error */ + hang(); + } +} + +void socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 mask, u32 val) +{ + int ret; + u64 args[3]; + + args[0] = (u64)reg_addr; + args[1] = mask; + args[2] = val; + ret = invoke_smc(INTEL_SIP_SMC_REG_UPDATE, args, 3, NULL, 0); + if (ret) { + /* SMC call return error */ + hang(); + } +}
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@intel.com
These secure register access functions allow U-Boot proper running at EL2 (non-secure) to access System Manager's secure registers by calling the ATF's PSCI runtime services (EL3/secure). If these helper functions are called from secure mode (EL3), the helper function will direct access the secure registers without calling the ATF's PSCI runtime services.
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com
arch/arm/mach-socfpga/Makefile | 6 +++ .../mach-socfpga/include/mach/secure_reg_helper.h | 20 ++++++++ arch/arm/mach-socfpga/secure_reg_helper.c | 57 ++++++++++++++++++++++ 3 files changed, 83 insertions(+) create mode 100644 arch/arm/mach-socfpga/include/mach/secure_reg_helper.h create mode 100644 arch/arm/mach-socfpga/secure_reg_helper.c
diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-socfpga/Makefile index 3310e92..e59587b 100644 --- a/arch/arm/mach-socfpga/Makefile +++ b/arch/arm/mach-socfpga/Makefile @@ -53,6 +53,12 @@ obj-y += wrap_pinmux_config_s10.o obj-y += wrap_pll_config_s10.o endif
+ifndef CONFIG_SPL_BUILD +ifdef CONFIG_SPL_ATF +obj-y += secure_reg_helper.o
obj-$(FOO) += bar.o , you don't need the inner ifdef
+endif +endif
[...]
+++ b/arch/arm/mach-socfpga/include/mach/secure_reg_helper.h @@ -0,0 +1,20 @@ +/* SPDX-License-Identifier: GPL-2.0
- Copyright (C) 2019 Intel Corporation <www.intel.com>
- */
+#ifndef _SECURE_REG_HELPER_H_ +#define _SECURE_REG_HELPER_H_
+#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr); +void socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr); +void socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 mask, u32 val); +#else +#define socfpga_secure_reg_read32 readl +#define socfpga_secure_reg_write32 writel +#define socfpga_secure_reg_update32 clrsetbits_le32 +#endif
I think I don't understand how this is supposed to work. Would every place in U-Boot have to be patched to call these functions now ?
[...]
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@intel.com
These secure register access functions allow U-Boot proper running at EL2 (non-secure) to access System Manager's secure registers by calling the ATF's PSCI runtime services (EL3/secure). If these helper functions are called from secure mode (EL3), the helper function will direct access the secure registers without calling the ATF's PSCI runtime services.
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com
arch/arm/mach-socfpga/Makefile | 6 +++ .../mach-socfpga/include/mach/secure_reg_helper.h | 20 ++++++++ arch/arm/mach-socfpga/secure_reg_helper.c | 57
++++++++++++++++++++++
3 files changed, 83 insertions(+) create mode 100644 arch/arm/mach-socfpga/include/mach/secure_reg_helper.h create mode 100644 arch/arm/mach-socfpga/secure_reg_helper.c
diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-socfpga/Makefile index 3310e92..e59587b 100644 --- a/arch/arm/mach-socfpga/Makefile +++ b/arch/arm/mach-socfpga/Makefile @@ -53,6 +53,12 @@ obj-y += wrap_pinmux_config_s10.o obj-y += wrap_pll_config_s10.o endif
+ifndef CONFIG_SPL_BUILD +ifdef CONFIG_SPL_ATF +obj-y += secure_reg_helper.o
obj-$(FOO) += bar.o , you don't need the inner ifdef
OK. It's cleaner. Thanks.
+endif +endif
[...]
+++ b/arch/arm/mach-socfpga/include/mach/secure_reg_helper.h @@ -0,0 +1,20 @@ +/* SPDX-License-Identifier: GPL-2.0
- Copyright (C) 2019 Intel Corporation <www.intel.com>
- */
+#ifndef _SECURE_REG_HELPER_H_ +#define _SECURE_REG_HELPER_H_
+#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr); void +socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr); void +socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 mask, u32 val); +#else +#define socfpga_secure_reg_read32 readl +#define socfpga_secure_reg_write32 writel +#define socfpga_secure_reg_update32 clrsetbits_le32 +#endif
I think I don't understand how this is supposed to work. Would every place in U- Boot have to be patched to call these functions now ?
Not every register access need this. Only those accessing registers in secure zone such as 'System Manager' registers need to call this. It's basically determine whether the driver should issue SMC/PSCI call if it's running in EL2 (non-secure) or access the registers directly by simply using readl/writel and etc if it's running in EL3 (secure). Accessing those registers in secure zone in non-secure mode (EL2) will cause SError exception. So we can determine this behaviour in compile time: SPL always running in EL3. So it just simply fallback to use readl/writel/clrsetbits_le32.
For U-Boot proper (SSBL), there are 2 scenarios: 1) If CONFIG_SPL_ATF is defined, it means ATF is supported. It implies that U-Boot proper will be running in EL2 (non-secure), then it will use SMC/PSCI calls to access the secure registers.
2) CONFIG_SPL_ATF is not defined, no ATF support. U-Boot proper will be running in EL3 which will fall back to simply using the direct access functions (readl/writel and etc).
[...]
On 2/20/20 3:02 AM, Ang, Chee Hong wrote: [...]
+#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr); void +socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr); void +socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 mask, u32 val); +#else +#define socfpga_secure_reg_read32 readl +#define socfpga_secure_reg_write32 writel +#define socfpga_secure_reg_update32 clrsetbits_le32 +#endif
I think I don't understand how this is supposed to work. Would every place in U- Boot have to be patched to call these functions now ?
Not every register access need this. Only those accessing registers in secure zone such as 'System Manager' registers need to call this. It's basically determine whether the driver should issue SMC/PSCI call if it's running in EL2 (non-secure) or access the registers directly by simply using readl/writel and etc if it's running in EL3 (secure). Accessing those registers in secure zone in non-secure mode (EL2) will cause SError exception. So we can determine this behaviour in compile time: SPL always running in EL3. So it just simply fallback to use readl/writel/clrsetbits_le32.
For U-Boot proper (SSBL), there are 2 scenarios:
- If CONFIG_SPL_ATF is defined, it means ATF is supported. It implies that U-Boot proper will be
running in EL2 (non-secure), then it will use SMC/PSCI calls to access the secure registers.
- CONFIG_SPL_ATF is not defined, no ATF support. U-Boot proper will be running in EL3 which
will fall back to simply using the direct access functions (readl/writel and etc).
I would expect the standard IO accessors would or should handle this stuff ?
On 2/20/20 3:02 AM, Ang, Chee Hong wrote: [...]
+#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr); void +socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr); void +socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 mask, u32 +val); #else +#define socfpga_secure_reg_read32 readl +#define socfpga_secure_reg_write32 writel +#define socfpga_secure_reg_update32 clrsetbits_le32 +#endif
I think I don't understand how this is supposed to work. Would every place in U- Boot have to be patched to call these functions now ?
Not every register access need this. Only those accessing registers in secure zone such as 'System Manager' registers need to call this. It's basically determine whether the driver should issue SMC/PSCI call if it's running in EL2 (non-secure) or access the registers directly by simply using
readl/writel and etc if it's running in EL3 (secure).
Accessing those registers in secure zone in non-secure mode (EL2) will cause
SError exception.
So we can determine this behaviour in compile time: SPL always running in EL3. So it just simply fallback to use
readl/writel/clrsetbits_le32.
For U-Boot proper (SSBL), there are 2 scenarios:
- If CONFIG_SPL_ATF is defined, it means ATF is supported. It implies
that U-Boot proper will be running in EL2 (non-secure), then it will use
SMC/PSCI calls to access the secure registers.
- CONFIG_SPL_ATF is not defined, no ATF support. U-Boot proper will
be running in EL3 which will fall back to simply using the direct access functions
(readl/writel and etc).
I would expect the standard IO accessors would or should handle this stuff ?
Standard IO accessors are just general memory read/write functions designed to be compatible with general hardware platforms. Not all platforms have secure/non-secure hardware zones. I don't think they should handle this.
If it's running in EL3 (secure mode) the standard I/O accessors will work just fine because EL3 can access to all secure/non-secure zones. In the header file, you can see the secure I/O accessors will be replaced by the standard I/O accessors if it's built for SPL and U-Boot proper without ATF. Because both are running in EL3 (secure).
If ATF is enabled, SPL will be still running in EL3 but U-Boot proper will be running in EL2 (non-secure). If any code accessing those secure zones without going through ATF (making SMC/PSCI calls to EL3), it will trigger 'SError' exceptions and crash the U-Boot.
On 2/20/20 6:54 PM, Ang, Chee Hong wrote:
On 2/20/20 3:02 AM, Ang, Chee Hong wrote: [...]
+#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr); void +socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr); void +socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 mask, u32 +val); #else +#define socfpga_secure_reg_read32 readl +#define socfpga_secure_reg_write32 writel +#define socfpga_secure_reg_update32 clrsetbits_le32 +#endif
I think I don't understand how this is supposed to work. Would every place in U- Boot have to be patched to call these functions now ?
Not every register access need this. Only those accessing registers in secure zone such as 'System Manager' registers need to call this. It's basically determine whether the driver should issue SMC/PSCI call if it's running in EL2 (non-secure) or access the registers directly by simply using
readl/writel and etc if it's running in EL3 (secure).
Accessing those registers in secure zone in non-secure mode (EL2) will cause
SError exception.
So we can determine this behaviour in compile time: SPL always running in EL3. So it just simply fallback to use
readl/writel/clrsetbits_le32.
For U-Boot proper (SSBL), there are 2 scenarios:
- If CONFIG_SPL_ATF is defined, it means ATF is supported. It implies
that U-Boot proper will be running in EL2 (non-secure), then it will use
SMC/PSCI calls to access the secure registers.
- CONFIG_SPL_ATF is not defined, no ATF support. U-Boot proper will
be running in EL3 which will fall back to simply using the direct access functions
(readl/writel and etc).
I would expect the standard IO accessors would or should handle this stuff ?
Standard IO accessors are just general memory read/write functions designed to be compatible with general hardware platforms. Not all platforms have secure/non-secure hardware zones. I don't think they should handle this.
If it's running in EL3 (secure mode) the standard I/O accessors will work just fine because EL3 can access to all secure/non-secure zones. In the header file, you can see the secure I/O accessors will be replaced by the standard I/O accessors if it's built for SPL and U-Boot proper without ATF. Because both are running in EL3 (secure).
If ATF is enabled, SPL will be still running in EL3 but U-Boot proper will be running in EL2 (non-secure). If any code accessing those secure zones without going through ATF (making SMC/PSCI calls to EL3), it will trigger 'SError' exceptions and crash the U-Boot.
Hmmm, if U-Boot is running in EL2 (non-secure), why would it ever access secure zones in the first place ?
And if that's really necessary, should the ATF really provide secure-mode register access primitives or should it provide some more high-level interface instead ?
On 2/20/20 6:54 PM, Ang, Chee Hong wrote:
On 2/20/20 3:02 AM, Ang, Chee Hong wrote: [...]
+#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr); void +socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr); void +socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 mask, u32 +val); #else +#define socfpga_secure_reg_read32 readl +#define socfpga_secure_reg_write32 writel +#define socfpga_secure_reg_update32 clrsetbits_le32 +#endif
I think I don't understand how this is supposed to work. Would every place in U- Boot have to be patched to call these functions now ?
Not every register access need this. Only those accessing registers in secure zone such as 'System Manager' registers need to call this. It's basically determine whether the driver should issue SMC/PSCI call if it's running in EL2 (non-secure) or access the registers directly by simply using
readl/writel and etc if it's running in EL3 (secure).
Accessing those registers in secure zone in non-secure mode (EL2) will cause
SError exception.
So we can determine this behaviour in compile time: SPL always running in EL3. So it just simply fallback to use
readl/writel/clrsetbits_le32.
For U-Boot proper (SSBL), there are 2 scenarios:
- If CONFIG_SPL_ATF is defined, it means ATF is supported. It
implies that U-Boot proper will be running in EL2 (non-secure), then it will use
SMC/PSCI calls to access the secure registers.
- CONFIG_SPL_ATF is not defined, no ATF support. U-Boot proper will
be running in EL3 which will fall back to simply using the direct access functions
(readl/writel and etc).
I would expect the standard IO accessors would or should handle this stuff ?
Standard IO accessors are just general memory read/write functions designed to be compatible with general hardware platforms. Not all platforms have secure/non-secure hardware zones. I don't think they should
handle this.
If it's running in EL3 (secure mode) the standard I/O accessors will work just fine because EL3 can access to all secure/non-secure zones. In the header file, you can see the secure I/O accessors will be replaced by the standard I/O accessors if it's built for SPL and U-Boot proper without ATF. Because both are
running in EL3 (secure).
If ATF is enabled, SPL will be still running in EL3 but U-Boot proper will be running in EL2 (non-secure). If any code accessing those secure zones without going through ATF (making SMC/PSCI calls to EL3), it will trigger 'SError'
exceptions and crash the U-Boot.
Hmmm, if U-Boot is running in EL2 (non-secure), why would it ever access secure zones in the first place ?
SPL and U-Boot reuse the same drivers code. It runs without issue in SPL (EL3) but it crashes if running the same driver code in EL2 which access some secure zone registers. The System Manager (secure zone) contains some register which control the behaviours of EMAC/SDMMC and etc. Clock manager driver rely on those registers in System Manager as well for retrieving clock information. These clock/EMAC/SDMMC drivers access the System Manager (secure zone).
And if that's really necessary, should the ATF really provide secure-mode register access primitives or should it provide some more high-level interface instead ?
I see your point. I should have mentioned to you that these secure-mode register access provided by ATF actually do more stuffs than just primitive accessors. ATF only allow certain secure registers required by the non-secure driver to be accessed. It will check the secure register address and block access if the register address is not allowed to be accessed by non-secure world (EL2). So these secure register access provided by ATF is not just simple accessor/delegate which simply allow access to any secure zone from non-secure world without any restrictions. I would say the secure register access provided by ATF is a 'middle-level' interface not just some primitive accessors.
Currently, we have like 20+ secure registers allowed access by drivers running in non-secure mode (U-Boot proper / Linux). I don't think we want to define and maintain those high level interfaces for each of those secure register accesses in ATF and U-Boot.
On 2/21/20 7:01 PM, Ang, Chee Hong wrote:
On 2/20/20 6:54 PM, Ang, Chee Hong wrote:
On 2/20/20 3:02 AM, Ang, Chee Hong wrote: [...]
> +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) > +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr); void > +socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr); void > +socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 mask, u32 > +val); #else > +#define socfpga_secure_reg_read32 readl > +#define socfpga_secure_reg_write32 writel > +#define socfpga_secure_reg_update32 clrsetbits_le32 > +#endif
I think I don't understand how this is supposed to work. Would every place in U- Boot have to be patched to call these functions now ?
Not every register access need this. Only those accessing registers in secure zone such as 'System Manager' registers need to call this. It's basically determine whether the driver should issue SMC/PSCI call if it's running in EL2 (non-secure) or access the registers directly by simply using
readl/writel and etc if it's running in EL3 (secure).
Accessing those registers in secure zone in non-secure mode (EL2) will cause
SError exception.
So we can determine this behaviour in compile time: SPL always running in EL3. So it just simply fallback to use
readl/writel/clrsetbits_le32.
For U-Boot proper (SSBL), there are 2 scenarios:
- If CONFIG_SPL_ATF is defined, it means ATF is supported. It
implies that U-Boot proper will be running in EL2 (non-secure), then it will use
SMC/PSCI calls to access the secure registers.
- CONFIG_SPL_ATF is not defined, no ATF support. U-Boot proper will
be running in EL3 which will fall back to simply using the direct access functions
(readl/writel and etc).
I would expect the standard IO accessors would or should handle this stuff ?
Standard IO accessors are just general memory read/write functions designed to be compatible with general hardware platforms. Not all platforms have secure/non-secure hardware zones. I don't think they should
handle this.
If it's running in EL3 (secure mode) the standard I/O accessors will work just fine because EL3 can access to all secure/non-secure zones. In the header file, you can see the secure I/O accessors will be replaced by the standard I/O accessors if it's built for SPL and U-Boot proper without ATF. Because both are
running in EL3 (secure).
If ATF is enabled, SPL will be still running in EL3 but U-Boot proper will be running in EL2 (non-secure). If any code accessing those secure zones without going through ATF (making SMC/PSCI calls to EL3), it will trigger 'SError'
exceptions and crash the U-Boot.
Hmmm, if U-Boot is running in EL2 (non-secure), why would it ever access secure zones in the first place ?
SPL and U-Boot reuse the same drivers code. It runs without issue in SPL (EL3) but it crashes if running the same driver code in EL2 which access some secure zone registers. The System Manager (secure zone) contains some register which control the behaviours of EMAC/SDMMC and etc. Clock manager driver rely on those registers in System Manager as well for retrieving clock information. These clock/EMAC/SDMMC drivers access the System Manager (secure zone).
Maybe those registers should only be configured by the secure OS, so maybe those drivers should be fixed ?
And if that's really necessary, should the ATF really provide secure-mode register access primitives or should it provide some more high-level interface instead ?
I see your point. I should have mentioned to you that these secure-mode register access provided by ATF actually do more stuffs than just primitive accessors.
So socfpga_secure_reg_read32 does not just read a register ? Then the naming is misleading . And documentation is missing.
ATF only allow certain secure registers required by the non-secure driver to be accessed. It will check the secure register address and block access if the register address is not allowed to be accessed by non-secure world (EL2).
Why don't you configure those secure registers in the secure mode then ? It seems like that's the purpose of those registers being secure only.
So these secure register access provided by ATF is not just simple accessor/delegate which simply allow access to any secure zone from non-secure world without any restrictions. I would say the secure register access provided by ATF is a 'middle-level' interface not just some primitive accessors.
Currently, we have like 20+ secure registers allowed access by drivers running in non-secure mode (U-Boot proper / Linux). I don't think we want to define and maintain those high level interfaces for each of those secure register accesses in ATF and U-Boot.
See above.
On 2/21/20 7:01 PM, Ang, Chee Hong wrote:
On 2/20/20 6:54 PM, Ang, Chee Hong wrote:
On 2/20/20 3:02 AM, Ang, Chee Hong wrote: [...]
>> +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) >> +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr); void >> +socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr); void >> +socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 mask, u32 >> +val); #else >> +#define socfpga_secure_reg_read32 readl >> +#define socfpga_secure_reg_write32 writel >> +#define socfpga_secure_reg_update32 clrsetbits_le32 >> +#endif > > I think I don't understand how this is supposed to work. Would > every place in U- Boot have to be patched to call these functions now ?
Not every register access need this. Only those accessing registers in secure zone such as 'System Manager' registers need to call
this.
It's basically determine whether the driver should issue SMC/PSCI call if it's running in EL2 (non-secure) or access the registers directly by simply using
readl/writel and etc if it's running in EL3 (secure).
Accessing those registers in secure zone in non-secure mode (EL2) will cause
SError exception.
So we can determine this behaviour in compile time: SPL always running in EL3. So it just simply fallback to use
readl/writel/clrsetbits_le32.
For U-Boot proper (SSBL), there are 2 scenarios:
- If CONFIG_SPL_ATF is defined, it means ATF is supported. It
implies that U-Boot proper will be running in EL2 (non-secure), then it will use
SMC/PSCI calls to access the secure registers.
- CONFIG_SPL_ATF is not defined, no ATF support. U-Boot proper
will be running in EL3 which will fall back to simply using the direct access functions
(readl/writel and etc).
I would expect the standard IO accessors would or should handle this stuff ?
Standard IO accessors are just general memory read/write functions designed to be compatible with general hardware platforms. Not all platforms have secure/non-secure hardware zones. I don't think they should
handle this.
If it's running in EL3 (secure mode) the standard I/O accessors will work just fine because EL3 can access to all secure/non-secure zones. In the header file, you can see the secure I/O accessors will be replaced by the standard I/O accessors if it's built for SPL and U-Boot proper without ATF. Because both are
running in EL3 (secure).
If ATF is enabled, SPL will be still running in EL3 but U-Boot proper will be running in EL2 (non-secure). If any code accessing those secure zones without going through ATF (making SMC/PSCI calls to EL3), it will trigger 'SError'
exceptions and crash the U-Boot.
Hmmm, if U-Boot is running in EL2 (non-secure), why would it ever access secure zones in the first place ?
SPL and U-Boot reuse the same drivers code. It runs without issue in SPL (EL3) but it crashes if running the same driver code in EL2 which access
some secure zone registers.
The System Manager (secure zone) contains some register which control the behaviours of EMAC/SDMMC and etc. Clock manager driver rely on those registers in System Manager as well for retrieving clock information. These clock/EMAC/SDMMC drivers access the System Manager
(secure zone).
Maybe those registers should only be configured by the secure OS, so maybe those drivers should be fixed ?
And if that's really necessary, should the ATF really provide secure-mode register access primitives or should it provide some more high-
level interface instead ?
I see your point. I should have mentioned to you that these secure-mode register access provided by ATF actually do more stuffs than just
primitive accessors.
So socfpga_secure_reg_read32 does not just read a register ? Then the naming is misleading . And documentation is missing.
ATF only allow certain secure registers required by the non-secure driver to be
accessed.
It will check the secure register address and block access if the register address is not allowed to be accessed by non-secure world (EL2).
Why don't you configure those secure registers in the secure mode then ? It seems like that's the purpose of those registers being secure only.
So these secure register access provided by ATF is not just simple accessor/delegate which simply allow access to any secure zone from non-
secure world without any restrictions.
I would say the secure register access provided by ATF is a 'middle-level' interface not just some primitive accessors.
Currently, we have like 20+ secure registers allowed access by drivers running in non-secure mode (U-Boot proper / Linux). I don't think we want to define and maintain those high level interfaces for each of those secure register accesses in ATF and U-Boot.
See above.
OK. Then these secure access register should be set up in SPL (EL3). U-Boot drivers shouldn't access them at all because the driver may be running in SPL(EL3) and in U-Boot proper (EL2) too. I can take a look at those drivers accessing secure registers and try to move/decouple those secure access from U-Boot drivers to SPL (EL3) then we no longer need those secure register access functions. I am not sure doing this will impact the functionality of the U-Boot driver running in EL2 or not. I can refactor those drivers and try it out.
On 2/21/20 8:06 PM, Ang, Chee Hong wrote:
On 2/21/20 7:01 PM, Ang, Chee Hong wrote:
On 2/20/20 6:54 PM, Ang, Chee Hong wrote:
On 2/20/20 3:02 AM, Ang, Chee Hong wrote: [...] >>> +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) >>> +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr); void >>> +socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr); void >>> +socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 mask, u32 >>> +val); #else >>> +#define socfpga_secure_reg_read32 readl >>> +#define socfpga_secure_reg_write32 writel >>> +#define socfpga_secure_reg_update32 clrsetbits_le32 >>> +#endif >> >> I think I don't understand how this is supposed to work. Would >> every place in U- Boot have to be patched to call these functions now ? > > Not every register access need this. Only those accessing > registers in secure zone such as 'System Manager' registers need to call
this.
> It's basically determine whether the driver should issue SMC/PSCI > call if it's running in EL2 (non-secure) or access the registers > directly by simply using readl/writel and etc if it's running in EL3 (secure). > Accessing those registers in secure zone in non-secure mode (EL2) > will cause SError exception. > So we can determine this behaviour in compile time: > SPL always running in EL3. So it just simply fallback to use readl/writel/clrsetbits_le32. > > For U-Boot proper (SSBL), there are 2 scenarios: > 1) If CONFIG_SPL_ATF is defined, it means ATF is supported. It > implies that U-Boot proper will be running in EL2 (non-secure), > then it will use SMC/PSCI calls to access the secure registers. > > 2) CONFIG_SPL_ATF is not defined, no ATF support. U-Boot proper > will be running in EL3 which will fall back to simply using the > direct access functions (readl/writel and etc).
I would expect the standard IO accessors would or should handle this stuff ?
Standard IO accessors are just general memory read/write functions designed to be compatible with general hardware platforms. Not all platforms have secure/non-secure hardware zones. I don't think they should
handle this.
If it's running in EL3 (secure mode) the standard I/O accessors will work just fine because EL3 can access to all secure/non-secure zones. In the header file, you can see the secure I/O accessors will be replaced by the standard I/O accessors if it's built for SPL and U-Boot proper without ATF. Because both are
running in EL3 (secure).
If ATF is enabled, SPL will be still running in EL3 but U-Boot proper will be running in EL2 (non-secure). If any code accessing those secure zones without going through ATF (making SMC/PSCI calls to EL3), it will trigger 'SError'
exceptions and crash the U-Boot.
Hmmm, if U-Boot is running in EL2 (non-secure), why would it ever access secure zones in the first place ?
SPL and U-Boot reuse the same drivers code. It runs without issue in SPL (EL3) but it crashes if running the same driver code in EL2 which access
some secure zone registers.
The System Manager (secure zone) contains some register which control the behaviours of EMAC/SDMMC and etc. Clock manager driver rely on those registers in System Manager as well for retrieving clock information. These clock/EMAC/SDMMC drivers access the System Manager
(secure zone).
Maybe those registers should only be configured by the secure OS, so maybe those drivers should be fixed ?
And if that's really necessary, should the ATF really provide secure-mode register access primitives or should it provide some more high-
level interface instead ?
I see your point. I should have mentioned to you that these secure-mode register access provided by ATF actually do more stuffs than just
primitive accessors.
So socfpga_secure_reg_read32 does not just read a register ? Then the naming is misleading . And documentation is missing.
ATF only allow certain secure registers required by the non-secure driver to be
accessed.
It will check the secure register address and block access if the register address is not allowed to be accessed by non-secure world (EL2).
Why don't you configure those secure registers in the secure mode then ? It seems like that's the purpose of those registers being secure only.
So these secure register access provided by ATF is not just simple accessor/delegate which simply allow access to any secure zone from non-
secure world without any restrictions.
I would say the secure register access provided by ATF is a 'middle-level' interface not just some primitive accessors.
Currently, we have like 20+ secure registers allowed access by drivers running in non-secure mode (U-Boot proper / Linux). I don't think we want to define and maintain those high level interfaces for each of those secure register accesses in ATF and U-Boot.
See above.
OK. Then these secure access register should be set up in SPL (EL3). U-Boot drivers shouldn't access them at all because the driver may be running in SPL(EL3) and in U-Boot proper (EL2) too. I can take a look at those drivers accessing secure registers and try to move/decouple those secure access from U-Boot drivers to SPL (EL3) then we no longer need those secure register access functions.
I think that would be great, no ?
I am not sure doing this will impact the functionality of the U-Boot driver running in EL2 or not. I can refactor those drivers and try it out.
Thanks!
On 2/21/20 8:06 PM, Ang, Chee Hong wrote:
On 2/21/20 7:01 PM, Ang, Chee Hong wrote:
On 2/20/20 6:54 PM, Ang, Chee Hong wrote:
> On 2/20/20 3:02 AM, Ang, Chee Hong wrote: > [...] >>>> +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) >>>> +u32 socfpga_secure_reg_read32(phys_addr_t reg_addr); void >>>> +socfpga_secure_reg_write32(u32 val, phys_addr_t reg_addr); >>>> +void socfpga_secure_reg_update32(phys_addr_t reg_addr, u32 >>>> +mask, u32 val); #else >>>> +#define socfpga_secure_reg_read32 readl >>>> +#define socfpga_secure_reg_write32 writel >>>> +#define socfpga_secure_reg_update32 clrsetbits_le32 >>>> +#endif >>> >>> I think I don't understand how this is supposed to work. Would >>> every place in U- Boot have to be patched to call these functions now ? >> >> Not every register access need this. Only those accessing >> registers in secure zone such as 'System Manager' registers need >> to call
this.
>> It's basically determine whether the driver should issue >> SMC/PSCI call if it's running in EL2 (non-secure) or access the >> registers directly by simply using > readl/writel and etc if it's running in EL3 (secure). >> Accessing those registers in secure zone in non-secure mode >> (EL2) will cause > SError exception. >> So we can determine this behaviour in compile time: >> SPL always running in EL3. So it just simply fallback to use > readl/writel/clrsetbits_le32. >> >> For U-Boot proper (SSBL), there are 2 scenarios: >> 1) If CONFIG_SPL_ATF is defined, it means ATF is supported. It >> implies that U-Boot proper will be running in EL2 (non-secure), >> then it will use > SMC/PSCI calls to access the secure registers. >> >> 2) CONFIG_SPL_ATF is not defined, no ATF support. U-Boot proper >> will be running in EL3 which will fall back to simply using the >> direct access functions > (readl/writel and etc). > > I would expect the standard IO accessors would or should handle this
stuff ?
Standard IO accessors are just general memory read/write functions designed to be compatible with general hardware platforms. Not all platforms have secure/non-secure hardware zones. I don't think they should
handle this.
If it's running in EL3 (secure mode) the standard I/O accessors will work just fine because EL3 can access to all secure/non-secure zones. In the header file, you can see the secure I/O accessors will be replaced by the standard I/O accessors if it's built for SPL and U-Boot proper without ATF. Because both are
running in EL3 (secure).
If ATF is enabled, SPL will be still running in EL3 but U-Boot proper will be running in EL2 (non-secure). If any code accessing those secure zones without going through ATF (making SMC/PSCI calls to EL3), it will trigger 'SError'
exceptions and crash the U-Boot.
Hmmm, if U-Boot is running in EL2 (non-secure), why would it ever access secure zones in the first place ?
SPL and U-Boot reuse the same drivers code. It runs without issue in SPL (EL3) but it crashes if running the same driver code in EL2 which access
some secure zone registers.
The System Manager (secure zone) contains some register which control the behaviours of EMAC/SDMMC and etc. Clock manager driver rely on those registers in System Manager as well for retrieving clock information. These clock/EMAC/SDMMC drivers access the System Manager
(secure zone).
Maybe those registers should only be configured by the secure OS, so maybe those drivers should be fixed ?
And if that's really necessary, should the ATF really provide secure-mode register access primitives or should it provide some more high-
level interface instead ?
I see your point. I should have mentioned to you that these secure-mode register access provided by ATF actually do more stuffs than just
primitive accessors.
So socfpga_secure_reg_read32 does not just read a register ? Then the naming is misleading . And documentation is missing.
ATF only allow certain secure registers required by the non-secure driver to be
accessed.
It will check the secure register address and block access if the register address is not allowed to be accessed by non-secure world (EL2).
Why don't you configure those secure registers in the secure mode then ? It seems like that's the purpose of those registers being secure only.
So these secure register access provided by ATF is not just simple accessor/delegate which simply allow access to any secure zone from non-
secure world without any restrictions.
I would say the secure register access provided by ATF is a 'middle-level' interface not just some primitive accessors.
Currently, we have like 20+ secure registers allowed access by drivers running in non-secure mode (U-Boot proper / Linux). I don't think we want to define and maintain those high level interfaces for each of those secure register accesses in ATF and U-Boot.
See above.
OK. Then these secure access register should be set up in SPL (EL3). U-Boot drivers shouldn't access them at all because the driver may be running in SPL(EL3) and in U-Boot proper (EL2) too. I can take a look at those drivers accessing secure registers and try to move/decouple those secure access from U-Boot drivers to SPL (EL3) then we no longer need those secure register access functions.
I think that would be great, no ?
Since the SDMMC/DWMAC drivers read the device tree to configure the behaviour of the hardware via the secure registers. I think it should still be part of the driver instead of configuring the hardware in different places. I have proposed using ATF's high-level APIs to achieve this when the driver is running in EL2. I have already proposed this in other email threads. Are you OK with this approach ? .
I am not sure doing this will impact the functionality of the U-Boot driver running in EL2 or not. I can refactor those drivers and try it out.
Thanks!
On 2/24/20 3:21 AM, Ang, Chee Hong wrote: [...]
Currently, we have like 20+ secure registers allowed access by drivers running in non-secure mode (U-Boot proper / Linux). I don't think we want to define and maintain those high level interfaces for each of those secure register accesses in ATF and U-Boot.
See above.
OK. Then these secure access register should be set up in SPL (EL3). U-Boot drivers shouldn't access them at all because the driver may be running in SPL(EL3) and in U-Boot proper (EL2) too. I can take a look at those drivers accessing secure registers and try to move/decouple those secure access from U-Boot drivers to SPL (EL3) then we no longer need those secure register access functions.
I think that would be great, no ?
Since the SDMMC/DWMAC drivers read the device tree to configure the behaviour of the hardware via the secure registers. I think it should still be part of the driver instead of configuring the hardware in different places. I have proposed using ATF's high-level APIs to achieve this when the driver is running in EL2. I have already proposed this in other email threads. Are you OK with this approach ?
I think something more high level might be a good idea here.
On 2/24/20 3:21 AM, Ang, Chee Hong wrote: [...]
Currently, we have like 20+ secure registers allowed access by drivers running in non-secure mode (U-Boot proper / Linux). I don't think we want to define and maintain those high level interfaces for each of those secure register accesses in ATF and U-Boot.
See above.
OK. Then these secure access register should be set up in SPL (EL3). U-Boot drivers shouldn't access them at all because the driver may be running in SPL(EL3) and in U-Boot proper (EL2) too. I can take a look at those drivers accessing secure registers and try to move/decouple those secure access from U-Boot drivers to SPL (EL3) then we no longer need those secure register access functions.
I think that would be great, no ?
Since the SDMMC/DWMAC drivers read the device tree to configure the behaviour of the hardware via the secure registers. I think it should still be part of the driver instead of configuring the hardware in different places. I have proposed using ATF's high-level APIs to achieve this
when the driver is running in EL2.
I have already proposed this in other email threads. Are you OK with this approach ?
I think something more high level might be a good idea here.
What do you mean by 'more high level' ? We handle this in SPL (EL3) ?
Since you are the author of this 'drivers/net/dwmac_socfpga.c': https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/net/dwmac_socfpga.c... https://gitlab.denx.de/u-boot/u-boot/blob/master/arch/arm/dts/socfpga_strati...
Your driver selects the PHY interface (RGMII/RMII and etc) using the following register (part of System Manager): https://www.intel.com/content/www/us/en/programmable/hps/stratix-10/hps.html...
I personally think this PHY interface select for EMACx shouldn't be part of System Manager. I don't see the security benefits here by making this PHY interface select as 'secure zone' register.
Same applies to DW MMC driver as well: https://gitlab.denx.de/u-boot/u-boot/blob/master/drivers/mmc/socfpga_dw_mmc....
It sets the following register in System Manager (secure zone) to configure the SDMMC clocks: https://www.intel.com/content/www/us/en/programmable/hps/stratix-10/hps.html...
Don't you think these things should be part of driver itself as what we are doing now instead of removing these from drivers and place them in SPL (EL3)?
On 2/24/20 3:21 AM, Ang, Chee Hong wrote: [...]
> Currently, we have like 20+ secure registers allowed access by > drivers running in non-secure mode (U-Boot proper / Linux). > I don't think we want to define and maintain those high level > interfaces for each of those secure register accesses in ATF and U-Boot.
See above.
OK. Then these secure access register should be set up in SPL (EL3). U-Boot drivers shouldn't access them at all because the driver may be running in SPL(EL3) and in U-Boot proper (EL2) too. I can take a look at those drivers accessing secure registers and try to move/decouple those secure access from U-Boot drivers to SPL (EL3) then we no longer need those secure register access functions.
I think that would be great, no ?
Since the SDMMC/DWMAC drivers read the device tree to configure the behaviour of the hardware via the secure registers. I think it should still be part of the driver instead of configuring the hardware in different places. I have proposed using ATF's high-level APIs to achieve this
when the driver is running in EL2.
I have already proposed this in other email threads. Are you OK with this approach ?
I think something more high level might be a good idea here.
What do you mean by 'more high level' ? We handle this in SPL (EL3) ?
Since you are the author of this 'drivers/net/dwmac_socfpga.c': https://gitlab.denx.de/u-boot/u- boot/blob/master/drivers/net/dwmac_socfpga.c#L101 https://gitlab.denx.de/u-boot/u- boot/blob/master/arch/arm/dts/socfpga_stratix10.dtsi#L98
Your driver selects the PHY interface (RGMII/RMII and etc) using the following register (part of System Manager): https://www.intel.com/content/www/us/en/programmable/hps/stratix- 10/hps.html#topic/jng1505406892594.html
I personally think this PHY interface select for EMACx shouldn't be part of System Manager. I don't see the security benefits here by making this PHY interface select as 'secure zone' register.
Same applies to DW MMC driver as well: https://gitlab.denx.de/u-boot/u- boot/blob/master/drivers/mmc/socfpga_dw_mmc.c#L60
It sets the following register in System Manager (secure zone) to configure the SDMMC clocks: https://www.intel.com/content/www/us/en/programmable/hps/stratix- 10/hps.html#topic/gil1505406886282.html
Don't you think these things should be part of driver itself as what we are doing now instead of removing these from drivers and place them in SPL (EL3)?
These 2 drivers that access the system manager: drivers/mmc/socfpga_dw_mmc.c - MMC driver access System Manager's 'SDMMC' register to set clock phase https://www.intel.com/content/www/us/en/programmable/hps/stratix-10/topic/gi...
drivers/net/dwmac_socfpga.c - MAC driver access System Manager's 'EMACx' registers to set MAC's PHY interface: https://www.intel.com/content/www/us/en/programmable/hps/stratix-10/hps.html...
Gen5/Arria10/Stratix10 & Agilex all using these drivers. They do not cause any issue in Gen5/Arria10 because there is no 'secure zone' in System Manager. For Stratix10 & Agilex, it has issue with U-Boot proper running in EL2.
I don't think is good idea to separate those System Manager access from these 2 drivers and move them to SPL as they are shared by all SOCFPGA platforms.
I think the proper way to resolve this is we add a new System Manager driver under 'drivers/misc'. The device type should be 'UCLASS_MISC'. Then we make changes to those drivers (SDMMC/MAC) to access the System Manager through the System Manager driver by using 'misc_read(dev...)' & 'misc_write(dev...)' The System Manager driver should decide whether to access those 'secure zone' registers directly in EL3 or making SMC call to ATF to access the System Manager if it's running in EL2.
Linux has similar MAC driver running in EL1 (non-secure) accessing System Manager: https://elixir.bootlin.com/linux/v5.5/source/drivers/net/ethernet/stmicro/st...
Linux MAC driver access System Manager via this 'altr,system_manager' driver: https://elixir.bootlin.com/linux/v5.5/source/drivers/mfd/altera-sysmgr.c#L44 System Manager driver will make SMC/PSCI call to ATF to access the System Manager's registers.
Ang, Chee Hong chee.hong.ang@intel.com schrieb am Fr., 28. Feb. 2020, 03:53:
On 2/24/20 3:21 AM, Ang, Chee Hong wrote: [...]
>> Currently, we have like 20+ secure registers allowed access by >> drivers running in non-secure mode (U-Boot proper / Linux). >> I don't think we want to define and maintain those high level >> interfaces for each of those secure register accesses in ATF and
U-Boot.
> > See above. OK. Then these secure access register should be set up in SPL
(EL3).
U-Boot drivers shouldn't access them at all because the driver may be running in SPL(EL3) and in U-Boot proper (EL2) too. I can take a look at those drivers accessing secure registers and try to move/decouple those secure access from U-Boot drivers to SPL (EL3) then we no longer need those secure register access
functions.
I think that would be great, no ?
Since the SDMMC/DWMAC drivers read the device tree to configure the behaviour of the hardware via the secure registers. I think it should still be part of the driver instead of configuring the hardware in different places. I have proposed using ATF's high-level APIs to achieve this
when the driver is running in EL2.
I have already proposed this in other email threads. Are you OK with this approach ?
I think something more high level might be a good idea here.
What do you mean by 'more high level' ? We handle this in SPL (EL3) ?
Since you are the author of this 'drivers/net/dwmac_socfpga.c': https://gitlab.denx.de/u-boot/u- boot/blob/master/drivers/net/dwmac_socfpga.c#L101 https://gitlab.denx.de/u-boot/u- boot/blob/master/arch/arm/dts/socfpga_stratix10.dtsi#L98
Your driver selects the PHY interface (RGMII/RMII and etc) using the
following
register (part of System Manager): https://www.intel.com/content/www/us/en/programmable/hps/stratix- 10/hps.html#topic/jng1505406892594.html
I personally think this PHY interface select for EMACx shouldn't be part
of
System Manager. I don't see the security benefits here by making this PHY interface
select as
'secure zone' register.
Same applies to DW MMC driver as well: https://gitlab.denx.de/u-boot/u- boot/blob/master/drivers/mmc/socfpga_dw_mmc.c#L60
It sets the following register in System Manager (secure zone) to
configure the
SDMMC clocks: https://www.intel.com/content/www/us/en/programmable/hps/stratix- 10/hps.html#topic/gil1505406886282.html
Don't you think these things should be part of driver itself as what we
are doing
now instead of removing these from drivers and place them in SPL (EL3)?
These 2 drivers that access the system manager: drivers/mmc/socfpga_dw_mmc.c
- MMC driver access System Manager's 'SDMMC' register to set clock phase
https://www.intel.com/content/www/us/en/programmable/hps/stratix-10/topic/gi...
drivers/net/dwmac_socfpga.c
- MAC driver access System Manager's 'EMACx' registers to set MAC's PHY
interface:
https://www.intel.com/content/www/us/en/programmable/hps/stratix-10/hps.html...
Gen5/Arria10/Stratix10 & Agilex all using these drivers. They do not cause any issue in Gen5/Arria10 because there is no 'secure zone' in System Manager. For Stratix10 & Agilex, it has issue with U-Boot proper running in EL2.
I don't think is good idea to separate those System Manager access from these 2 drivers and move them to SPL as they are shared by all SOCFPGA platforms.
I think the proper way to resolve this is we add a new System Manager driver under 'drivers/misc'. The device type should be 'UCLASS_MISC'.
I have a pending patchset that adds such a sysmgr driver at least for gen5. I haven't published it yet because the whole series as one makes gen5 SRAM overlow, but maybe that part can be split out...
Regards, Simon
Then we make changes to those drivers (SDMMC/MAC) to access the System
Manager through the System Manager driver by using 'misc_read(dev...)' & 'misc_write(dev...)' The System Manager driver should decide whether to access those 'secure zone' registers directly in EL3 or making SMC call to ATF to access the System Manager if it's running in EL2.
Linux has similar MAC driver running in EL1 (non-secure) accessing System Manager:
https://elixir.bootlin.com/linux/v5.5/source/drivers/net/ethernet/stmicro/st...
Linux MAC driver access System Manager via this 'altr,system_manager' driver:
https://elixir.bootlin.com/linux/v5.5/source/drivers/mfd/altera-sysmgr.c#L44 System Manager driver will make SMC/PSCI call to ATF to access the System Manager's registers.
Ang, Chee Hong <chee.hong.ang@intel.commailto:chee.hong.ang@intel.com> schrieb am Fr., 28. Feb. 2020, 03:53:
On 2/24/20 3:21 AM, Ang, Chee Hong wrote: [...]
> Currently, we have like 20+ secure registers allowed access by > drivers running in non-secure mode (U-Boot proper / Linux). > I don't think we want to define and maintain those high level > interfaces for each of those secure register accesses in ATF and U-Boot.
See above.
OK. Then these secure access register should be set up in SPL (EL3). U-Boot drivers shouldn't access them at all because the driver may be running in SPL(EL3) and in U-Boot proper (EL2) too. I can take a look at those drivers accessing secure registers and try to move/decouple those secure access from U-Boot drivers to SPL (EL3) then we no longer need those secure register access functions.
I think that would be great, no ?
Since the SDMMC/DWMAC drivers read the device tree to configure the behaviour of the hardware via the secure registers. I think it should still be part of the driver instead of configuring the hardware in different places. I have proposed using ATF's high-level APIs to achieve this
when the driver is running in EL2.
I have already proposed this in other email threads. Are you OK with this approach ?
I think something more high level might be a good idea here.
What do you mean by 'more high level' ? We handle this in SPL (EL3) ?
Since you are the author of this 'drivers/net/dwmac_socfpga.c': https://gitlab.denx.de/u-boot/u- boot/blob/master/drivers/net/dwmac_socfpga.c#L101 https://gitlab.denx.de/u-boot/u- boot/blob/master/arch/arm/dts/socfpga_stratix10.dtsi#L98
Your driver selects the PHY interface (RGMII/RMII and etc) using the following register (part of System Manager): https://www.intel.com/content/www/us/en/programmable/hps/stratix- 10/hps.html#topic/jng1505406892594.html
I personally think this PHY interface select for EMACx shouldn't be part of System Manager. I don't see the security benefits here by making this PHY interface select as 'secure zone' register.
Same applies to DW MMC driver as well: https://gitlab.denx.de/u-boot/u- boot/blob/master/drivers/mmc/socfpga_dw_mmc.c#L60
It sets the following register in System Manager (secure zone) to configure the SDMMC clocks: https://www.intel.com/content/www/us/en/programmable/hps/stratix- 10/hps.html#topic/gil1505406886282.html
Don't you think these things should be part of driver itself as what we are doing now instead of removing these from drivers and place them in SPL (EL3)?
These 2 drivers that access the system manager: drivers/mmc/socfpga_dw_mmc.c - MMC driver access System Manager's 'SDMMC' register to set clock phase https://www.intel.com/content/www/us/en/programmable/hps/stratix-10/topic/gi...
drivers/net/dwmac_socfpga.c - MAC driver access System Manager's 'EMACx' registers to set MAC's PHY interface: https://www.intel.com/content/www/us/en/programmable/hps/stratix-10/hps.html...
Gen5/Arria10/Stratix10 & Agilex all using these drivers. They do not cause any issue in Gen5/Arria10 because there is no 'secure zone' in System Manager. For Stratix10 & Agilex, it has issue with U-Boot proper running in EL2.
I don't think is good idea to separate those System Manager access from these 2 drivers and move them to SPL as they are shared by all SOCFPGA platforms.
I think the proper way to resolve this is we add a new System Manager driver under 'drivers/misc'. The device type should be 'UCLASS_MISC'.
I have a pending patchset that adds such a sysmgr driver at least for gen5. I haven't published it yet because the whole >series as one makes gen5 SRAM overlow, but maybe that part can be split out...
Regards, Simon
Thanks. I am working on the changes. I will need to test that out to make sure the new sysmgr driver works on all socfpga platforms. Will let you guys review whether this is a better/cleaner solution to handle the system manager access.
Then we make changes to those drivers (SDMMC/MAC) to access the System Manager through the System Manager driver by using 'misc_read(dev...)' & 'misc_write(dev...)' The System Manager driver should decide whether to access those 'secure zone' registers directly in EL3 or making SMC call to ATF to access the System Manager if it's running in EL2.
Linux has similar MAC driver running in EL1 (non-secure) accessing System Manager: https://elixir.bootlin.com/linux/v5.5/source/drivers/net/ethernet/stmicro/st...
Linux MAC driver access System Manager via this 'altr,system_manager' driver: https://elixir.bootlin.com/linux/v5.5/source/drivers/mfd/altera-sysmgr.c#L44 System Manager driver will make SMC/PSCI call to ATF to access the System Manager's registers.
From: Chee Hong Ang chee.hong.ang@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@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)
On 2/19/20 1:25 PM, chee.hong.ang@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@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@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@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@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@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@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@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@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@denx.de Sent: Friday, February 21, 2020 12:48 AM To: Ang, Chee Hong chee.hong.ang@intel.com; u-boot@lists.denx.de Cc: Simon Goldschmidt simon.k.r.goldschmidt@gmail.com; See, Chin Liang chin.liang.see@intel.com; Tan, Ley Foon ley.foon.tan@intel.com; Westergreen, Dalon dalon.westergreen@intel.com; Gong, Richard richard.gong@intel.com; Tom Rini trini@konsulko.com; Michal Simek michal.simek@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@intel.com wrote:
From: Chee Hong Ang chee.hong.ang@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@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@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@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@intel.com schrieb am Sa., 22. Feb. 2020, 06:30:
From: Chee Hong Ang chee.hong.ang@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@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@intel.commailto:chee.hong.ang@intel.com> schrieb am Sa., 22. Feb. 2020, 06:30:
From: Chee Hong Ang <chee.hong.ang@intel.commailto:chee.hong.ang@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@intel.commailto:chee.hong.ang@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@gmail.com Cc: U-Boot Mailing List u-boot@lists.denx.de; Marek Vasut marex@denx.de; See, Chin Liang chin.liang.see@intel.com; Tan, Ley Foon ley.foon.tan@intel.com; Westergreen, Dalon dalon.westergreen@intel.com; Gong, Richard richard.gong@intel.com Subject: RE: [PATCH v2 11/21] arm: socfpga: Secure register access for clock manager (SoC 64bits)
Ang, Chee Hong <chee.hong.ang@intel.commailto:chee.hong.ang@intel.com> schrieb am Sa., 22. Feb. 2020, 06:30:
From: Chee Hong Ang <chee.hong.ang@intel.commailto:chee.hong.ang@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@intel.commailto:chee.hong.ang@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@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@gmail.com *Cc:* U-Boot Mailing List u-boot@lists.denx.de; Marek Vasut < marex@denx.de>; See, Chin Liang chin.liang.see@intel.com; Tan, Ley Foon ley.foon.tan@intel.com; Westergreen, Dalon dalon.westergreen@intel.com; Gong, Richard richard.gong@intel.com *Subject:* RE: [PATCH v2 11/21] arm: socfpga: Secure register access for clock manager (SoC 64bits)
Ang, Chee Hong chee.hong.ang@intel.com schrieb am Sa., 22. Feb. 2020, 06:30:
From: Chee Hong Ang chee.hong.ang@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@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@intel.commailto:chee.hong.ang@intel.com> schrieb am Sa., 22. Feb. 2020, 06:30:
From: Chee Hong Ang <chee.hong.ang@intel.commailto:chee.hong.ang@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@intel.commailto:chee.hong.ang@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...
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... 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 :(
From: Chee Hong Ang chee.hong.ang@intel.com
Allow access to System Manager's EMAC control register from non-secure mode during PHY mode setup.
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com --- arch/arm/mach-socfpga/misc_s10.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-socfpga/misc_s10.c b/arch/arm/mach-socfpga/misc_s10.c index 25c3ff6..6593308 100644 --- a/arch/arm/mach-socfpga/misc_s10.c +++ b/arch/arm/mach-socfpga/misc_s10.c @@ -18,6 +18,7 @@ #include <asm/pl310.h> #include <linux/libfdt.h> #include <asm/arch/mailbox_s10.h> +#include <asm/arch/secure_reg_helper.h>
#include <dt-bindings/reset/altr,rst-mgr-s10.h>
@@ -65,9 +66,9 @@ static u32 socfpga_phymode_setup(u32 gmac_index, const char *phymode) else return -EINVAL;
- clrsetbits_le32(socfpga_get_sysmgr_addr() + SYSMGR_SOC64_EMAC0 + - gmac_index, - SYSMGR_EMACGRP_CTRL_PHYSEL_MASK, modereg); + socfpga_secure_reg_update32(socfpga_get_sysmgr_addr() + + SYSMGR_SOC64_EMAC0 + gmac_index, + SYSMGR_EMACGRP_CTRL_PHYSEL_MASK, modereg);
return 0; }
From: Chee Hong Ang chee.hong.ang@intel.com
Allow access to System Manager's EMAC control register from non-secure mode during PHY mode setup.
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com
arch/arm/mach-socfpga/misc_s10.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-socfpga/misc_s10.c b/arch/arm/mach- socfpga/misc_s10.c index 25c3ff6..6593308 100644 --- a/arch/arm/mach-socfpga/misc_s10.c +++ b/arch/arm/mach-socfpga/misc_s10.c @@ -18,6 +18,7 @@ #include <asm/pl310.h> #include <linux/libfdt.h> #include <asm/arch/mailbox_s10.h> +#include <asm/arch/secure_reg_helper.h>
#include <dt-bindings/reset/altr,rst-mgr-s10.h>
@@ -65,9 +66,9 @@ static u32 socfpga_phymode_setup(u32 gmac_index, const char *phymode) else return -EINVAL;
- clrsetbits_le32(socfpga_get_sysmgr_addr() + SYSMGR_SOC64_EMAC0 +
gmac_index,SYSMGR_EMACGRP_CTRL_PHYSEL_MASK, modereg);
- socfpga_secure_reg_update32(socfpga_get_sysmgr_addr() +
SYSMGR_SOC64_EMAC0 + gmac_index,SYSMGR_EMACGRP_CTRL_PHYSEL_MASK,modereg);
return 0; } -- 2.7.4
Looks like this PHY setup is redundant and no longer needed because it is already being taken care in 'drivers/net/dwmac_socfpga.c' which is written by Marek. Will check with Ley Foon whether this socfpga_phymode_setup() can be safely removed.
On 2/22/20 6:35 AM, Ang, Chee Hong wrote:
From: Chee Hong Ang chee.hong.ang@intel.com
Allow access to System Manager's EMAC control register from non-secure mode during PHY mode setup.
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com
arch/arm/mach-socfpga/misc_s10.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)
diff --git a/arch/arm/mach-socfpga/misc_s10.c b/arch/arm/mach- socfpga/misc_s10.c index 25c3ff6..6593308 100644 --- a/arch/arm/mach-socfpga/misc_s10.c +++ b/arch/arm/mach-socfpga/misc_s10.c @@ -18,6 +18,7 @@ #include <asm/pl310.h> #include <linux/libfdt.h> #include <asm/arch/mailbox_s10.h> +#include <asm/arch/secure_reg_helper.h>
#include <dt-bindings/reset/altr,rst-mgr-s10.h>
@@ -65,9 +66,9 @@ static u32 socfpga_phymode_setup(u32 gmac_index, const char *phymode) else return -EINVAL;
- clrsetbits_le32(socfpga_get_sysmgr_addr() + SYSMGR_SOC64_EMAC0 +
gmac_index,SYSMGR_EMACGRP_CTRL_PHYSEL_MASK, modereg);
- socfpga_secure_reg_update32(socfpga_get_sysmgr_addr() +
SYSMGR_SOC64_EMAC0 + gmac_index,SYSMGR_EMACGRP_CTRL_PHYSEL_MASK,modereg);
return 0; } -- 2.7.4
Looks like this PHY setup is redundant and no longer needed because it is already being taken care in 'drivers/net/dwmac_socfpga.c' which is written by Marek. Will check with Ley Foon whether this socfpga_phymode_setup() can be safely removed.
Nice
From: Chee Hong Ang chee.hong.ang@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@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); }
From: Chee Hong Ang chee.hong.ang@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@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@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@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@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@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@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@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@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@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@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@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.
From: Chee Hong Ang chee.hong.ang@intel.com
Allow MMC driver to access System Manager's SDMMC control register in non-secure mode (EL2).
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com --- drivers/mmc/socfpga_dw_mmc.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/socfpga_dw_mmc.c b/drivers/mmc/socfpga_dw_mmc.c index 786cdc7..558f246 100644 --- a/drivers/mmc/socfpga_dw_mmc.c +++ b/drivers/mmc/socfpga_dw_mmc.c @@ -5,6 +5,7 @@
#include <common.h> #include <asm/arch/clock_manager.h> +#include <asm/arch/secure_reg_helper.h> #include <asm/arch/system_manager.h> #include <clk.h> #include <dm.h> @@ -57,10 +58,12 @@ static void socfpga_dwmci_clksel(struct dwmci_host *host)
debug("%s: drvsel %d smplsel %d\n", __func__, priv->drvsel, priv->smplsel); - writel(sdmmc_mask, socfpga_get_sysmgr_addr() + SYSMGR_SDMMC); + socfpga_secure_reg_write32(sdmmc_mask, socfpga_get_sysmgr_addr() + + SYSMGR_SDMMC);
debug("%s: SYSMGR_SDMMCGRP_CTRL_REG = 0x%x\n", __func__, - readl(socfpga_get_sysmgr_addr() + SYSMGR_SDMMC)); + socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + + SYSMGR_SDMMC));
/* Enable SDMMC clock */ setbits_le32(socfpga_get_clkmgr_addr() + CLKMGR_PERPLL_EN,
From: Chee Hong Ang chee.hong.ang@intel.com
Allow MMC driver to access System Manager's SDMMC control register in non- secure mode (EL2).
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com
drivers/mmc/socfpga_dw_mmc.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)
diff --git a/drivers/mmc/socfpga_dw_mmc.c b/drivers/mmc/socfpga_dw_mmc.c index 786cdc7..558f246 100644 --- a/drivers/mmc/socfpga_dw_mmc.c +++ b/drivers/mmc/socfpga_dw_mmc.c @@ -5,6 +5,7 @@
#include <common.h> #include <asm/arch/clock_manager.h> +#include <asm/arch/secure_reg_helper.h> #include <asm/arch/system_manager.h> #include <clk.h> #include <dm.h> @@ -57,10 +58,12 @@ static void socfpga_dwmci_clksel(struct dwmci_host *host)
debug("%s: drvsel %d smplsel %d\n", __func__, priv->drvsel, priv->smplsel);
- writel(sdmmc_mask, socfpga_get_sysmgr_addr() + SYSMGR_SDMMC);
socfpga_secure_reg_write32(sdmmc_mask, socfpga_get_sysmgr_addr()
SYSMGR_SDMMC);debug("%s: SYSMGR_SDMMCGRP_CTRL_REG = 0x%x\n", __func__,
readl(socfpga_get_sysmgr_addr() + SYSMGR_SDMMC));
socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() +SYSMGR_SDMMC));/* Enable SDMMC clock */ setbits_le32(socfpga_get_clkmgr_addr() + CLKMGR_PERPLL_EN,
-- 2.7.4
This SDMMC driver need to access System Manager's SDMMC control register (secure zone) to configure the clock's phase shift settings. I don't think this could be separated from the driver. I will add/define a high-level API in ATF to be used by this SDMMC driver from non-secure world (EL2). This high-level API (SMC/PSCI calls) will be properly documented in 'include/linux/intel-smc.h'
From: Chee Hong Ang chee.hong.ang@intel.com
Allow MAC driver to access System Manager's EMAC control registers in non-secure mode.
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com --- drivers/net/dwmac_socfpga.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/dwmac_socfpga.c b/drivers/net/dwmac_socfpga.c index e93561d..293c660 100644 --- a/drivers/net/dwmac_socfpga.c +++ b/drivers/net/dwmac_socfpga.c @@ -17,6 +17,7 @@ #include <dm/device_compat.h> #include <linux/err.h>
+#include <asm/arch/secure_reg_helper.h> #include <asm/arch/system_manager.h>
struct dwmac_socfpga_platdata { @@ -98,8 +99,8 @@ static int dwmac_socfpga_probe(struct udevice *dev) reset_assert_bulk(&reset_bulk);
modemask = SYSMGR_EMACGRP_CTRL_PHYSEL_MASK << pdata->reg_shift; - clrsetbits_le32(pdata->phy_intf, modemask, - modereg << pdata->reg_shift); + socfpga_secure_reg_update32((phys_addr_t)pdata->phy_intf, modemask, + modereg << pdata->reg_shift);
reset_release_bulk(&reset_bulk);
From: Chee Hong Ang chee.hong.ang@intel.com
Allow MAC driver to access System Manager's EMAC control registers in non- secure mode.
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com
drivers/net/dwmac_socfpga.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/net/dwmac_socfpga.c b/drivers/net/dwmac_socfpga.c index e93561d..293c660 100644 --- a/drivers/net/dwmac_socfpga.c +++ b/drivers/net/dwmac_socfpga.c @@ -17,6 +17,7 @@ #include <dm/device_compat.h> #include <linux/err.h>
+#include <asm/arch/secure_reg_helper.h> #include <asm/arch/system_manager.h>
struct dwmac_socfpga_platdata { @@ -98,8 +99,8 @@ static int dwmac_socfpga_probe(struct udevice *dev) reset_assert_bulk(&reset_bulk);
modemask = SYSMGR_EMACGRP_CTRL_PHYSEL_MASK << pdata-
reg_shift;
- clrsetbits_le32(pdata->phy_intf, modemask,
modereg << pdata->reg_shift);
- socfpga_secure_reg_update32((phys_addr_t)pdata->phy_intf,
modemask,
modereg << pdata->reg_shift);reset_release_bulk(&reset_bulk);
-- 2.7.4
This MAC driver need to access System Manager's EMAC control register (secure zone) to configure the PHY interface (GMII/RGMII/RMII). This should be part of this MAC driver as well. I will add/define a high-level API in ATF to be used by this MAC driver from non-secure world (EL2). This high-level API (SMC/PSCI calls) will be properly documented in 'include/linux/intel-smc.h'
From: Chee Hong Ang chee.hong.ang@intel.com
Allow socfpga_bridges_reset() function in Reset Manager driver to access System Manager's register in non-secure mode (EL2).
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com --- arch/arm/mach-socfpga/reset_manager_s10.c | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-)
diff --git a/arch/arm/mach-socfpga/reset_manager_s10.c b/arch/arm/mach-socfpga/reset_manager_s10.c index c743077..d03f121 100644 --- a/arch/arm/mach-socfpga/reset_manager_s10.c +++ b/arch/arm/mach-socfpga/reset_manager_s10.c @@ -7,6 +7,7 @@ #include <common.h> #include <asm/io.h> #include <asm/arch/reset_manager.h> +#include <asm/arch/secure_reg_helper.h> #include <asm/arch/system_manager.h> #include <dt-bindings/reset/altr,rst-mgr-s10.h>
@@ -56,34 +57,37 @@ void socfpga_bridges_reset(int enable) { if (enable) { /* clear idle request to all bridges */ - setbits_le32(socfpga_get_sysmgr_addr() + - SYSMGR_SOC64_NOC_IDLEREQ_CLR, ~0); + socfpga_secure_reg_update32(socfpga_get_sysmgr_addr() + + SYSMGR_SOC64_NOC_IDLEREQ_CLR, + ~0, ~0);
/* Release all bridges from reset state */ clrbits_le32(socfpga_get_rstmgr_addr() + RSTMGR_SOC64_BRGMODRST, ~0);
/* Poll until all idleack to 0 */ - while (readl(socfpga_get_sysmgr_addr() + - SYSMGR_SOC64_NOC_IDLEACK)) + while (socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + + SYSMGR_SOC64_NOC_IDLEACK)) ; } else { /* set idle request to all bridges */ - writel(~0, - socfpga_get_sysmgr_addr() + - SYSMGR_SOC64_NOC_IDLEREQ_SET); + socfpga_secure_reg_write32(~0, socfpga_get_sysmgr_addr() + + SYSMGR_SOC64_NOC_IDLEREQ_SET);
/* Enable the NOC timeout */ - writel(1, socfpga_get_sysmgr_addr() + SYSMGR_SOC64_NOC_TIMEOUT); + socfpga_secure_reg_write32(1, socfpga_get_sysmgr_addr() + + SYSMGR_SOC64_NOC_TIMEOUT);
/* Poll until all idleack to 1 */ - while ((readl(socfpga_get_sysmgr_addr() + SYSMGR_SOC64_NOC_IDLEACK) ^ - (SYSMGR_NOC_H2F_MSK | SYSMGR_NOC_LWH2F_MSK))) + while ((socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + + SYSMGR_SOC64_NOC_IDLEACK) ^ (SYSMGR_NOC_H2F_MSK | + SYSMGR_NOC_LWH2F_MSK))) ;
/* Poll until all idlestatus to 1 */ - while ((readl(socfpga_get_sysmgr_addr() + SYSMGR_SOC64_NOC_IDLESTATUS) ^ - (SYSMGR_NOC_H2F_MSK | SYSMGR_NOC_LWH2F_MSK))) + while ((socfpga_secure_reg_read32(socfpga_get_sysmgr_addr() + + SYSMGR_SOC64_NOC_IDLESTATUS) ^ (SYSMGR_NOC_H2F_MSK | + SYSMGR_NOC_LWH2F_MSK))) ;
/* Reset all bridges (except NOR DDR scheduler & F2S) */ @@ -92,7 +96,8 @@ void socfpga_bridges_reset(int enable) RSTMGR_BRGMODRST_FPGA2SOC_MASK));
/* Disable NOC timeout */ - writel(0, socfpga_get_sysmgr_addr() + SYSMGR_SOC64_NOC_TIMEOUT); + socfpga_secure_reg_write32(0, socfpga_get_sysmgr_addr() + + SYSMGR_SOC64_NOC_TIMEOUT); } }
From: Chee Hong Ang chee.hong.ang@intel.com
Allow socfpga_bridges_reset() function in Reset Manager driver to access System Manager's register in non-secure mode (EL2).
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com
arch/arm/mach-socfpga/reset_manager_s10.c | 31 ++++++++++++++++++------
1 file changed, 18 insertions(+), 13 deletions(-)
diff --git a/arch/arm/mach-socfpga/reset_manager_s10.c b/arch/arm/mach- socfpga/reset_manager_s10.c index c743077..d03f121 100644 --- a/arch/arm/mach-socfpga/reset_manager_s10.c +++ b/arch/arm/mach-socfpga/reset_manager_s10.c @@ -7,6 +7,7 @@ #include <common.h> #include <asm/io.h> #include <asm/arch/reset_manager.h> +#include <asm/arch/secure_reg_helper.h> #include <asm/arch/system_manager.h> #include <dt-bindings/reset/altr,rst-mgr-s10.h>
@@ -56,34 +57,37 @@ void socfpga_bridges_reset(int enable) { if (enable) { /* clear idle request to all bridges */
setbits_le32(socfpga_get_sysmgr_addr() +SYSMGR_SOC64_NOC_IDLEREQ_CLR, ~0);
socfpga_secure_reg_update32(socfpga_get_sysmgr_addr() +SYSMGR_SOC64_NOC_IDLEREQ_CLR,~0, ~0);/* Release all bridges from reset state */ clrbits_le32(socfpga_get_rstmgr_addr() +
RSTMGR_SOC64_BRGMODRST, ~0);
/* Poll until all idleack to 0 */
while (readl(socfpga_get_sysmgr_addr() +SYSMGR_SOC64_NOC_IDLEACK))
while (socfpga_secure_reg_read32(socfpga_get_sysmgr_addr()SYSMGR_SOC64_NOC_IDLEACK)) ; } else { /* set idle request to all bridges */
writel(~0,socfpga_get_sysmgr_addr() +SYSMGR_SOC64_NOC_IDLEREQ_SET);
socfpga_secure_reg_write32(~0, socfpga_get_sysmgr_addr() +SYSMGR_SOC64_NOC_IDLEREQ_SET);/* Enable the NOC timeout */
writel(1, socfpga_get_sysmgr_addr() +SYSMGR_SOC64_NOC_TIMEOUT);
socfpga_secure_reg_write32(1, socfpga_get_sysmgr_addr() +SYSMGR_SOC64_NOC_TIMEOUT);/* Poll until all idleack to 1 */
while ((readl(socfpga_get_sysmgr_addr() +SYSMGR_SOC64_NOC_IDLEACK) ^
(SYSMGR_NOC_H2F_MSK |SYSMGR_NOC_LWH2F_MSK)))
while ((socfpga_secure_reg_read32(socfpga_get_sysmgr_addr()SYSMGR_SOC64_NOC_IDLEACK) ^(SYSMGR_NOC_H2F_MSK |
SYSMGR_NOC_LWH2F_MSK))) ;/* Poll until all idlestatus to 1 */
while ((readl(socfpga_get_sysmgr_addr() +SYSMGR_SOC64_NOC_IDLESTATUS) ^
(SYSMGR_NOC_H2F_MSK |SYSMGR_NOC_LWH2F_MSK)))
while ((socfpga_secure_reg_read32(socfpga_get_sysmgr_addr()SYSMGR_SOC64_NOC_IDLESTATUS) ^(SYSMGR_NOC_H2F_MSK |
SYSMGR_NOC_LWH2F_MSK))) ;/* Reset all bridges (except NOR DDR scheduler & F2S) */ @@ -
92,7 +96,8 @@ void socfpga_bridges_reset(int enable) RSTMGR_BRGMODRST_FPGA2SOC_MASK));
/* Disable NOC timeout */
writel(0, socfpga_get_sysmgr_addr() +SYSMGR_SOC64_NOC_TIMEOUT);
socfpga_secure_reg_write32(0, socfpga_get_sysmgr_addr() +SYSMGR_SOC64_NOC_TIMEOUT);}
-- 2.7.4
ATF already has similar function to enable/disable the socfpga bridge. This function is needed in U-Boot proper (non-secure, EL2) for enabling the bridge for soft IP access after FPGA is configured. Will add a high-level API in ATF for non-secure world. The API info will be documented in 'include/linux/intel-smc.h'
From: Chee Hong Ang chee.hong.ang@intel.com
Initialize timer in SPL running in secure mode (EL3) and skip timer initialization in U-Boot proper running in non-secure mode (EL2).
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com --- arch/arm/mach-socfpga/timer_s10.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-socfpga/timer_s10.c b/arch/arm/mach-socfpga/timer_s10.c index 5723789..0fa56c3 100644 --- a/arch/arm/mach-socfpga/timer_s10.c +++ b/arch/arm/mach-socfpga/timer_s10.c @@ -13,6 +13,7 @@ */ int timer_init(void) { +#ifdef CONFIG_SPL_BUILD int enable = 0x3; /* timer enable + output signal masked */ int loadval = ~0;
@@ -21,6 +22,6 @@ int timer_init(void) /* enable processor pysical counter */ asm volatile("msr cntp_ctl_el0, %0" : : "r" (enable)); asm volatile("msr cntp_tval_el0, %0" : : "r" (loadval)); - +#endif return 0; }
From: Chee Hong Ang chee.hong.ang@intel.com
In EL3, do_bridge_reset() directly send mailbox commands to SDM to query the FPGA configuration status. If running in non-secure mode (EL2), it invokes SMC service calls to ATF (EL3) to perform the query.
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com --- arch/arm/mach-socfpga/misc_s10.c | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/arch/arm/mach-socfpga/misc_s10.c b/arch/arm/mach-socfpga/misc_s10.c index 6593308..6c6ad17 100644 --- a/arch/arm/mach-socfpga/misc_s10.c +++ b/arch/arm/mach-socfpga/misc_s10.c @@ -17,6 +17,7 @@ #include <asm/arch/misc.h> #include <asm/pl310.h> #include <linux/libfdt.h> +#include <linux/intel-smc.h> #include <asm/arch/mailbox_s10.h> #include <asm/arch/secure_reg_helper.h>
@@ -154,11 +155,24 @@ void do_bridge_reset(int enable, unsigned int mask) { /* Check FPGA status before bridge enable */ if (enable) { +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) + u64 arg = 1; + + /* Send MBOX_RECONFIG_STATUS to SDM */ + int ret = invoke_smc(INTEL_SIP_SMC_FPGA_CONFIG_ISDONE, NULL, 0, + NULL, 0); + + if (ret && ret != INTEL_SIP_SMC_FPGA_CONFIG_STATUS_BUSY) { + /* Send MBOX_CONFIG_STATUS to SDM */ + ret = invoke_smc(INTEL_SIP_SMC_FPGA_CONFIG_ISDONE, + &arg, 1, NULL, 0); + } +#else int ret = mbox_get_fpga_config_status(MBOX_RECONFIG_STATUS);
if (ret && ret != MBOX_CFGSTAT_STATE_CONFIG) ret = mbox_get_fpga_config_status(MBOX_CONFIG_STATUS); - +#endif if (ret) return; }
From: Chee Hong Ang chee.hong.ang@intel.com
FPGA recpnfiguration driver will call the ATF's PSCI runtime services if it's running in non-secure mode (EL2).
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com --- drivers/fpga/stratix10.c | 141 ++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 140 insertions(+), 1 deletion(-)
diff --git a/drivers/fpga/stratix10.c b/drivers/fpga/stratix10.c index d8e3250..050a179 100644 --- a/drivers/fpga/stratix10.c +++ b/drivers/fpga/stratix10.c @@ -5,11 +5,148 @@
#include <common.h> #include <altera.h> -#include <asm/arch/mailbox_s10.h>
#define RECONFIG_STATUS_POLL_RESP_TIMEOUT_MS 60000 #define RECONFIG_STATUS_INTERVAL_DELAY_US 1000000
+#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) + +#include <asm/arch/misc.h> +#include <linux/intel-smc.h> + +#define BITSTREAM_CHUNK_SIZE 0xFFFF0 +#define RECONFIG_STATUS_POLL_RETRY_MAX 100 + +/* + * Polling the FPGA configuration status. + * Return 0 for success, non-zero for error. + */ +static int reconfig_status_polling_resp(void) +{ + int ret; + unsigned long start = get_timer(0); + + while (1) { + ret = invoke_smc(INTEL_SIP_SMC_FPGA_CONFIG_ISDONE, NULL, 0, + NULL, 0); + + if (!ret) + return 0; /* configuration success */ + + if (ret != INTEL_SIP_SMC_FPGA_CONFIG_STATUS_BUSY) + return ret; + + if (get_timer(start) > RECONFIG_STATUS_POLL_RESP_TIMEOUT_MS) + break; /* time out */ + + puts("."); + udelay(RECONFIG_STATUS_INTERVAL_DELAY_US); + } + + return -ETIMEDOUT; +} + +static int send_bistream(const void *rbf_data, size_t rbf_size) +{ + int i; + u64 res_buf[3]; + u64 args[2]; + u32 xfer_count = 0; + int ret, wr_ret = 0, retry = 0; + size_t buf_size = (rbf_size > BITSTREAM_CHUNK_SIZE) ? + BITSTREAM_CHUNK_SIZE : rbf_size; + + while (rbf_size || xfer_count) { + if (!wr_ret && rbf_size) { + args[0] = (u64)rbf_data; + args[1] = buf_size; + wr_ret = invoke_smc(INTEL_SIP_SMC_FPGA_CONFIG_WRITE, + args, 2, NULL, 0); + + debug("wr_ret = %d, rbf_data = %p, buf_size = %08lx\n", + wr_ret, rbf_data, buf_size); + + if (wr_ret == INTEL_SIP_SMC_FPGA_CONFIG_STATUS_REJECTED) + continue; + + rbf_size -= buf_size; + rbf_data += buf_size; + + if (buf_size >= rbf_size) + buf_size = rbf_size; + + xfer_count++; + puts("."); + } else { + ret = invoke_smc( + INTEL_SIP_SMC_FPGA_CONFIG_COMPLETED_WRITE, + NULL, 0, res_buf, 3); + if (!ret) { + for (i = 0; i < 3; i++) { + if (!res_buf[i]) + break; + xfer_count--; + wr_ret = 0; + retry = 0; + } + } else if (ret != + INTEL_SIP_SMC_FPGA_CONFIG_STATUS_BUSY) + return ret; + else if (!xfer_count) + return INTEL_SIP_SMC_FPGA_CONFIG_STATUS_ERROR; + + if (++retry >= RECONFIG_STATUS_POLL_RETRY_MAX) + return -ETIMEDOUT; + + udelay(20000); + } + } + + return 0; +} + +/* + * This is the interface used by FPGA driver. + * Return 0 for success, non-zero for error. + */ +int stratix10_load(Altera_desc *desc, const void *rbf_data, size_t rbf_size) +{ + int ret; + + debug("Invoking FPGA_CONFIG_START...\n"); + + ret = invoke_smc(INTEL_SIP_SMC_FPGA_CONFIG_START, NULL, 0, NULL, 0); + + if (ret) { + puts("Failure in RECONFIG mailbox command!\n"); + return ret; + } + + ret = send_bistream(rbf_data, rbf_size); + if (ret) { + printf("Error sending bitstream!\n"); + return ret; + } + + /* Make sure we don't send MBOX_RECONFIG_STATUS too fast */ + udelay(RECONFIG_STATUS_INTERVAL_DELAY_US); + + debug("Polling with MBOX_RECONFIG_STATUS...\n"); + ret = reconfig_status_polling_resp(); + if (ret) { + printf("FPGA reconfiguration failed!"); + return ret; + } + + puts("FPGA reconfiguration OK!\n"); + + return ret; +} + +#else + +#include <asm/arch/mailbox_s10.h> + static const struct mbox_cfgstat_state { int err_no; const char *error_name; @@ -281,3 +418,5 @@ int stratix10_load(Altera_desc *desc, const void *rbf_data, size_t rbf_size)
return ret; } + +#endif
From: Chee Hong Ang chee.hong.ang@intel.com
mbox_reset_cold() will invoke ATF's PSCI service when running in non-secure mode (EL2).
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com --- arch/arm/mach-socfpga/mailbox_s10.c | 4 ++++ 1 file changed, 4 insertions(+)
diff --git a/arch/arm/mach-socfpga/mailbox_s10.c b/arch/arm/mach-socfpga/mailbox_s10.c index f30e7f8..6b39576 100644 --- a/arch/arm/mach-socfpga/mailbox_s10.c +++ b/arch/arm/mach-socfpga/mailbox_s10.c @@ -330,6 +330,9 @@ error:
int mbox_reset_cold(void) { +#if !defined(CONFIG_SPL_BUILD) && defined(CONFIG_SPL_ATF) + psci_system_reset(); +#else int ret;
ret = mbox_send_cmd(MBOX_ID_UBOOT, MBOX_REBOOT_HPS, MBOX_CMD_DIRECT, @@ -338,6 +341,7 @@ int mbox_reset_cold(void) /* mailbox sent failure, wait for watchdog to kick in */ hang(); } +#endif return 0; }
From: Chee Hong Ang chee.hong.ang@intel.com
Booting Agilex and Stratix 10 without ATF support.
SPL -> U-Boot proper -> OS (Linux)
Signed-off-by: Chee Hong Ang chee.hong.ang@intel.com --- configs/socfpga_agilex_nofw_defconfig | 59 ++++++++++++++++++++++++++++++ configs/socfpga_stratix10_nofw_defconfig | 63 ++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+) create mode 100644 configs/socfpga_agilex_nofw_defconfig create mode 100644 configs/socfpga_stratix10_nofw_defconfig
diff --git a/configs/socfpga_agilex_nofw_defconfig b/configs/socfpga_agilex_nofw_defconfig new file mode 100644 index 0000000..3d63f8b --- /dev/null +++ b/configs/socfpga_agilex_nofw_defconfig @@ -0,0 +1,59 @@ +CONFIG_ARM=y +CONFIG_ARCH_SOCFPGA=y +CONFIG_SYS_TEXT_BASE=0x1000 +CONFIG_SYS_MALLOC_F_LEN=0x2000 +CONFIG_ENV_SIZE=0x1000 +CONFIG_ENV_OFFSET=0x200 +CONFIG_DM_GPIO=y +CONFIG_NR_DRAM_BANKS=2 +CONFIG_TARGET_SOCFPGA_AGILEX_SOCDK=y +CONFIG_IDENT_STRING="socfpga_agilex" +CONFIG_SPL_FS_FAT=y +# CONFIG_PSCI_RESET is not set +CONFIG_SPL_TEXT_BASE=0xFFE00000 +CONFIG_BOOTDELAY=5 +CONFIG_SPL_CACHE=y +CONFIG_SPL_SPI_LOAD=y +CONFIG_SYS_SPI_U_BOOT_OFFS=0x3c00000 +CONFIG_HUSH_PARSER=y +CONFIG_SYS_PROMPT="SOCFPGA_AGILEX # " +CONFIG_CMD_MEMTEST=y +CONFIG_CMD_GPIO=y +CONFIG_CMD_I2C=y +CONFIG_CMD_MMC=y +CONFIG_CMD_SPI=y +CONFIG_CMD_USB=y +CONFIG_CMD_DHCP=y +CONFIG_CMD_MII=y +CONFIG_CMD_PING=y +CONFIG_CMD_CACHE=y +CONFIG_CMD_EXT4=y +CONFIG_CMD_FAT=y +CONFIG_CMD_FS_GENERIC=y +CONFIG_DEFAULT_DEVICE_TREE="socfpga_agilex_socdk" +CONFIG_ENV_IS_IN_MMC=y +CONFIG_NET_RANDOM_ETHADDR=y +CONFIG_SPL_DM_SEQ_ALIAS=y +CONFIG_SPL_ALTERA_SDRAM=y +CONFIG_DWAPB_GPIO=y +CONFIG_DM_I2C=y +CONFIG_SYS_I2C_DW=y +CONFIG_DM_MMC=y +CONFIG_MMC_DW=y +CONFIG_SF_DEFAULT_MODE=0x2003 +CONFIG_SPI_FLASH_SPANSION=y +CONFIG_SPI_FLASH_STMICRO=y +CONFIG_PHY_MICREL=y +CONFIG_PHY_MICREL_KSZ90X1=y +CONFIG_DM_ETH=y +CONFIG_ETH_DESIGNWARE=y +CONFIG_MII=y +CONFIG_DM_RESET=y +CONFIG_SPI=y +CONFIG_CADENCE_QSPI=y +CONFIG_DESIGNWARE_SPI=y +CONFIG_USB=y +CONFIG_DM_USB=y +CONFIG_USB_DWC2=y +CONFIG_USB_STORAGE=y +# CONFIG_SPL_USE_TINY_PRINTF is not set diff --git a/configs/socfpga_stratix10_nofw_defconfig b/configs/socfpga_stratix10_nofw_defconfig new file mode 100644 index 0000000..22169a2 --- /dev/null +++ b/configs/socfpga_stratix10_nofw_defconfig @@ -0,0 +1,63 @@ +CONFIG_ARM=y +CONFIG_ARCH_SOCFPGA=y +CONFIG_SYS_TEXT_BASE=0x1000 +CONFIG_SYS_MALLOC_F_LEN=0x2000 +CONFIG_ENV_SIZE=0x1000 +CONFIG_ENV_OFFSET=0x200 +CONFIG_DM_GPIO=y +CONFIG_NR_DRAM_BANKS=2 +CONFIG_TARGET_SOCFPGA_STRATIX10_SOCDK=y +CONFIG_IDENT_STRING="socfpga_stratix10" +CONFIG_SPL_FS_FAT=y +# CONFIG_PSCI_RESET is not set +CONFIG_SPL_TEXT_BASE=0xFFE00000 +CONFIG_BOOTDELAY=5 +CONFIG_SPL_SPI_LOAD=y +CONFIG_SYS_SPI_U_BOOT_OFFS=0x3C00000 +CONFIG_HUSH_PARSER=y +CONFIG_SYS_PROMPT="SOCFPGA_STRATIX10 # " +CONFIG_CMD_MEMTEST=y +# CONFIG_CMD_FLASH is not set +CONFIG_CMD_GPIO=y +CONFIG_CMD_I2C=y +CONFIG_CMD_MMC=y +CONFIG_CMD_SPI=y +CONFIG_CMD_USB=y +CONFIG_CMD_DHCP=y +CONFIG_CMD_MII=y +CONFIG_CMD_PING=y +CONFIG_CMD_CACHE=y +CONFIG_CMD_EXT4=y +CONFIG_CMD_FAT=y +CONFIG_CMD_FS_GENERIC=y +CONFIG_DEFAULT_DEVICE_TREE="socfpga_stratix10_socdk" +CONFIG_ENV_IS_IN_MMC=y +CONFIG_SYS_RELOC_GD_ENV_ADDR=y +CONFIG_NET_RANDOM_ETHADDR=y +CONFIG_SPL_DM_SEQ_ALIAS=y +CONFIG_SPL_ALTERA_SDRAM=y +CONFIG_DWAPB_GPIO=y +CONFIG_DM_I2C=y +CONFIG_SYS_I2C_DW=y +CONFIG_DM_MMC=y +CONFIG_MMC_DW=y +CONFIG_MTD=y +CONFIG_SF_DEFAULT_MODE=0x2003 +CONFIG_SPI_FLASH_SPANSION=y +CONFIG_SPI_FLASH_STMICRO=y +CONFIG_PHY_MICREL=y +CONFIG_PHY_MICREL_KSZ90X1=y +CONFIG_DM_ETH=y +CONFIG_ETH_DESIGNWARE=y +CONFIG_MII=y +CONFIG_DM_RESET=y +CONFIG_SPI=y +CONFIG_CADENCE_QSPI=y +CONFIG_DESIGNWARE_SPI=y +CONFIG_USB=y +CONFIG_DM_USB=y +CONFIG_USB_DWC2=y +CONFIG_USB_STORAGE=y +CONFIG_DESIGNWARE_WATCHDOG=y +CONFIG_WDT=y +# CONFIG_SPL_USE_TINY_PRINTF is not set
participants (6)
-
Ang, Chee Hong -
chee.hong.ang＠intel.com
-
Dalon L Westergreen -
Marek Vasut -
Simon Goldschmidt -
Westergreen, Dalon