[PATCH v2 00/21] Enable ARM Trusted Firmware for U-Boot

[PATCH v2 00/21] Enable ARM Trusted Firmware for U-Boot

[chee.hong.ang at intel.com]

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

-- 
2.7.4

[PATCH v2 01/21] configs: agilex: Remove CONFIG_OF_EMBED

[chee.hong.ang at intel.com]

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
-- 
2.7.4

[PATCH v2 02/21] arm: socfpga: add fit source file for pack itb with ATF

[chee.hong.ang at intel.com]

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";
+		};
+	};
+};
-- 
2.7.4

[PATCH v2 03/21] arm: socfpga: Add function for checking description from FIT image

[chee.hong.ang at intel.com]

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
-- 
2.7.4

[PATCH v2 04/21] arm: socfpga: Load FIT image with ATF support

[chee.hong.ang at intel.com]

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__ */
-- 
2.7.4

[PATCH v2 05/21] arm: socfpga: Override 'lowlevel_init' to support ATF

[chee.hong.ang at intel.com]

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

[PATCH v2 06/21] configs: socfpga: Enable FIT image loading with ATF support

[chee.hong.ang at intel.com]

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
-- 
2.7.4

[PATCH v2 07/21] arm: socfpga: Disable "spin-table" method for booting Linux

[chee.hong.ang at intel.com]

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
-- 
2.7.4

[PATCH v2 08/21] arm: socfpga: Add SMC helper function for Intel SOCFPGA (64bits)

[chee.hong.ang at intel.com]

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(&regs, 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(&regs);
+
+	if (ret_arg) {
+		for (i = 0; i < ret_len; i++)
+			ret_arg[i] = regs.regs[i + 1];
+	}
+
+	return regs.regs[0];
+}
+#endif
-- 
2.7.4

[PATCH v2 09/21] arm: socfpga: Define SMC function identifiers for PSCI SiP services

[chee.hong.ang at intel.com]

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
-- 
2.7.4

[PATCH v2 10/21] arm: socfpga: Add secure register access helper functions for SoC 64bits

[chee.hong.ang at intel.com]

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();
+	}
+}
-- 
2.7.4

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

[chee.hong.ang at intel.com]

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

[PATCH v2 12/21] arm: socfpga: Secure register access in PHY mode setup

[chee.hong.ang at intel.com]

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

[PATCH v2 13/21] arm: socfpga: Secure register access for reading PLL frequency

[chee.hong.ang at intel.com]

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

[PATCH v2 14/21] mmc: dwmmc: socfpga: Secure register access in MMC driver

[chee.hong.ang at intel.com]

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

[PATCH v2 15/21] net: designware: socfpga: Secure register access in MAC driver

[chee.hong.ang at intel.com]

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

[PATCH v2 16/21] arm: socfpga: Secure register access in Reset Manager driver

[chee.hong.ang at intel.com]

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

[PATCH v2 17/21] arm: socfpga: stratix10: Initialize timer in SPL

[chee.hong.ang at intel.com]

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;
 }
-- 
2.7.4

[PATCH v2 18/21] arm: socfpga: Bridge reset invokes SMC service calls in EL2

[chee.hong.ang at intel.com]

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;
 	}
-- 
2.7.4

[PATCH v2 19/21] arm: socfpga: stratix10: Add ATF support to FPGA reconfig driver

[chee.hong.ang at intel.com]

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
-- 
2.7.4

[PATCH v2 20/21] arm: socfpga: mailbox: Add 'SYSTEM_RESET' PSCI support to mbox_reset_cold()

[chee.hong.ang at intel.com]

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;
 }
 
-- 
2.7.4

[PATCH v2 21/21] configs: socfpga: Add defconfig for Agilex and Stratix 10 without ATF support

[chee.hong.ang at intel.com]

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
-- 
2.7.4

[PATCH v2 01/21] configs: agilex: Remove CONFIG_OF_EMBED

[Marek Vasut]

On 2/19/20 1:25 PM, chee.hong.ang at 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 ?

[PATCH v2 03/21] arm: socfpga: Add function for checking description from FIT image

[Marek Vasut]

On 2/19/20 1:25 PM, chee.hong.ang at 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 ?

[...]

[PATCH v2 05/21] arm: socfpga: Override 'lowlevel_init' to support ATF

[Marek Vasut]

On 2/19/20 1:25 PM, chee.hong.ang at 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 ?

[PATCH v2 06/21] configs: socfpga: Enable FIT image loading with ATF support

[Marek Vasut]

On 2/19/20 1:25 PM, chee.hong.ang at 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 ?

[PATCH v2 08/21] arm: socfpga: Add SMC helper function for Intel SOCFPGA (64bits)

[Marek Vasut]

On 2/19/20 1:25 PM, chee.hong.ang at 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(&regs, 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(&regs);
> +
> +	if (ret_arg) {
> +		for (i = 0; i < ret_len; i++)
> +			ret_arg[i] = regs.regs[i + 1];

memcpy() ?

[PATCH v2 09/21] arm: socfpga: Define SMC function identifiers for PSCI SiP services

[Marek Vasut]

On 2/19/20 1:25 PM, chee.hong.ang at 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 ?

[PATCH v2 10/21] arm: socfpga: Add secure register access helper functions for SoC 64bits

[Marek Vasut]

On 2/19/20 1:25 PM, chee.hong.ang at 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 ?

[...]

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

[Marek Vasut]

On 2/19/20 1:25 PM, chee.hong.ang at 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 ?

[PATCH v2 03/21] arm: socfpga: Add function for checking description from FIT image

[Dalon L Westergreen]

On Wed, 2020-02-19 at 18:11 +0100, Marek Vasut wrote:
> On 2/19/20 1:25 PM, chee.hong.ang at 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.


> 
> [...]

[PATCH v2 03/21] arm: socfpga: Add function for checking description from FIT image

[Marek Vasut]

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

[PATCH v2 08/21] arm: socfpga: Add SMC helper function for Intel SOCFPGA (64bits)

[Ang, Chee Hong]

> On 2/19/20 1:25 PM, chee.hong.ang at 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(&regs, 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(&regs);
> > +
> > +	if (ret_arg) {
> > +		for (i = 0; i < ret_len; i++)
> > +			ret_arg[i] = regs.regs[i + 1];
> 
> memcpy() ?
Will change this too. Thanks.

[PATCH v2 09/21] arm: socfpga: Define SMC function identifiers for PSCI SiP services

[Ang, Chee Hong]

> On 2/19/20 1:25 PM, chee.hong.ang at 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.

[PATCH v2 10/21] arm: socfpga: Add secure register access helper functions for SoC 64bits

[Ang, Chee Hong]

> On 2/19/20 1:25 PM, chee.hong.ang at 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).
> 
> [...]

[PATCH v2 01/21] configs: agilex: Remove CONFIG_OF_EMBED

[Ang, Chee Hong]

> On 2/19/20 1:25 PM, chee.hong.ang at 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.

[PATCH v2 06/21] configs: socfpga: Enable FIT image loading with ATF support

[Ang, Chee Hong]

> On 2/19/20 1:25 PM, chee.hong.ang at 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.

[PATCH v2 05/21] arm: socfpga: Override 'lowlevel_init' to support ATF

[Ang, Chee Hong]

> On 2/19/20 1:25 PM, chee.hong.ang at 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.

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

[Ang, Chee Hong]

> On 2/19/20 1:25 PM, chee.hong.ang at 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).

[PATCH v2 01/21] configs: agilex: Remove CONFIG_OF_EMBED

[Marek Vasut]

On 2/20/20 3:12 AM, Ang, Chee Hong wrote:
>> On 2/19/20 1:25 PM, chee.hong.ang at intel.com wrote:
>>> From: Chee Hong Ang <chee.hong.ang@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.

[PATCH v2 05/21] arm: socfpga: Override 'lowlevel_init' to support ATF

[Marek Vasut]

On 2/20/20 3:27 AM, Ang, Chee Hong wrote:
>> On 2/19/20 1:25 PM, chee.hong.ang at 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.

[PATCH v2 06/21] configs: socfpga: Enable FIT image loading with ATF support

[Marek Vasut]

On 2/20/20 3:15 AM, Ang, Chee Hong wrote:
>> On 2/19/20 1:25 PM, chee.hong.ang at intel.com wrote:
>>> From: Chee Hong Ang <chee.hong.ang@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

[PATCH v2 09/21] arm: socfpga: Define SMC function identifiers for PSCI SiP services

[Marek Vasut]

On 2/20/20 2:42 AM, Ang, Chee Hong wrote:
>> On 2/19/20 1:25 PM, chee.hong.ang at 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 .
[...]

[PATCH v2 10/21] arm: socfpga: Add secure register access helper functions for SoC 64bits

[Marek Vasut]

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 ?

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

[Marek Vasut]

On 2/20/20 3:32 AM, Ang, Chee Hong wrote:
>> On 2/19/20 1:25 PM, chee.hong.ang at intel.com wrote:
>>> From: Chee Hong Ang <chee.hong.ang@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 ?

[PATCH v2 01/21] configs: agilex: Remove CONFIG_OF_EMBED

[Westergreen, Dalon]

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 at intel.com

 wrote:

From: Chee Hong Ang <

<mailto:chee.hong.ang@intel.com>

chee.hong.ang at 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 at 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

[PATCH v2 10/21] arm: socfpga: Add secure register access helper functions for SoC 64bits

[Ang, Chee Hong]

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

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

[Ang, Chee Hong]

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

[PATCH v2 05/21] arm: socfpga: Override 'lowlevel_init' to support ATF

[Ang, Chee Hong]

> On 2/20/20 3:27 AM, Ang, Chee Hong wrote:
> >> On 2/19/20 1:25 PM, chee.hong.ang at 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.

[PATCH v2 06/21] configs: socfpga: Enable FIT image loading with ATF support

[Simon Goldschmidt]

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 at 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
> 

[PATCH v2 06/21] configs: socfpga: Enable FIT image loading with ATF support

[Westergreen, Dalon]

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 at intel.com

 wrote:

From: Chee Hong Ang <

<mailto:chee.hong.ang@intel.com>

chee.hong.ang at 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 at 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

[PATCH v2 01/21] configs: agilex: Remove CONFIG_OF_EMBED

[Marek Vasut]

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 at intel.com
> 
>  wrote:
> 
> From: Chee Hong Ang <
> 
> <mailto:chee.hong.ang@intel.com>
> 
> chee.hong.ang at 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?

[...]

[PATCH v2 05/21] arm: socfpga: Override 'lowlevel_init' to support ATF

[Marek Vasut]

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 at 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 ?

[PATCH v2 10/21] arm: socfpga: Add secure register access helper functions for SoC 64bits

[Marek Vasut]

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 ?

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 ?

[PATCH v2 10/21] arm: socfpga: Add secure register access helper functions for SoC 64bits

[Ang, Chee Hong]

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

[PATCH v2 10/21] arm: socfpga: Add secure register access helper functions for SoC 64bits

[Marek Vasut]

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.

[PATCH v2 01/21] configs: agilex: Remove CONFIG_OF_EMBED

[Ang, Chee Hong]

> 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 at intel.com
> >
> >  wrote:
> >
> > From: Chee Hong Ang <
> >
> > <mailto:chee.hong.ang@intel.com>
> >
> > chee.hong.ang at 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.
> 
> [...]

[PATCH v2 05/21] arm: socfpga: Override 'lowlevel_init' to support ATF

[Ang, Chee Hong]

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

[PATCH v2 05/21] arm: socfpga: Override 'lowlevel_init' to support ATF

[Ang, Chee Hong]

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

[PATCH v2 10/21] arm: socfpga: Add secure register access helper functions for SoC 64bits

[Ang, Chee Hong]

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

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

[Ang, Chee Hong]

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

[PATCH v2 12/21] arm: socfpga: Secure register access in PHY mode setup

[Ang, Chee Hong]

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

[PATCH v2 13/21] arm: socfpga: Secure register access for reading PLL frequency

[Ang, Chee Hong]

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

[PATCH v2 14/21] mmc: dwmmc: socfpga: Secure register access in MMC driver

[Ang, Chee Hong]

> 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'

[PATCH v2 15/21] net: designware: socfpga: Secure register access in MAC driver

[Ang, Chee Hong]

> 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'

[PATCH v2 16/21] arm: socfpga: Secure register access in Reset Manager driver

[Ang, Chee Hong]

> 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'

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

[Simon Goldschmidt]

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

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

[Ang, Chee Hong]

Ang, Chee Hong <chee.hong.ang at intel.com<mailto:chee.hong.ang@intel.com>> schrieb am Sa., 22. Feb. 2020, 06:30:
> From: Chee Hong Ang <chee.hong.ang at intel.com<mailto: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 at intel.com<mailto: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

[PATCH v2 13/21] arm: socfpga: Secure register access for reading PLL frequency

[Ang, Chee Hong]

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

[PATCH v2 01/21] configs: agilex: Remove CONFIG_OF_EMBED

[Marek Vasut]

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 at intel.com
>>>
>>>  wrote:
>>>
>>> From: Chee Hong Ang <
>>>
>>> <mailto:chee.hong.ang@intel.com>
>>>
>>> chee.hong.ang at 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 ?

[PATCH v2 05/21] arm: socfpga: Override 'lowlevel_init' to support ATF

[Marek Vasut]

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

[PATCH v2 10/21] arm: socfpga: Add secure register access helper functions for SoC 64bits

[Marek Vasut]

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!

[PATCH v2 12/21] arm: socfpga: Secure register access in PHY mode setup

[Marek Vasut]

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

[PATCH v2 13/21] arm: socfpga: Secure register access for reading PLL frequency

[Marek Vasut]

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?

[PATCH v2 13/21] arm: socfpga: Secure register access for reading PLL frequency

[Ang, Chee Hong]

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

[PATCH v2 10/21] arm: socfpga: Add secure register access helper functions for SoC 64bits

[Ang, Chee Hong]

> 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!

[PATCH v2 01/21] configs: agilex: Remove CONFIG_OF_EMBED

[Ang, Chee Hong]

> 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 at intel.com
> >>>
> >>>  wrote:
> >>>
> >>> From: Chee Hong Ang <
> >>>
> >>> <mailto:chee.hong.ang@intel.com>
> >>>
> >>> chee.hong.ang at 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.

[PATCH v2 13/21] arm: socfpga: Secure register access for reading PLL frequency

[Ang, Chee Hong]

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

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

[Ang, Chee Hong]

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 at intel.com<mailto:chee.hong.ang@intel.com>> schrieb am Sa., 22. Feb. 2020, 06:30:
> From: Chee Hong Ang <chee.hong.ang at intel.com<mailto: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 at intel.com<mailto: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.

[PATCH v2 13/21] arm: socfpga: Secure register access for reading PLL frequency

[Ang, Chee Hong]

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

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

[Simon Goldschmidt]

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 at 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

>

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

[Ang, Chee Hong]

Ang, Chee Hong <chee.hong.ang at intel.com<mailto:chee.hong.ang@intel.com>> schrieb am Sa., 22. Feb. 2020, 06:30:
> From: Chee Hong Ang <chee.hong.ang at intel.com<mailto: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 at intel.com<mailto: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#topic/dwh1505406933720.html

Any R/W registers between ?siliconid1? to ?mpu? are READ-ONLY in EL2. But are read/writable in EL3.
If you click into one of these R/W registers:
You will see a notice like this:

Access: RW
Access mode: PRIVILEGEMODE | SECURE
Note: The processor must make a secure, privileged bus access to this register.

It just said this register is read/writable in EL3 but it doesn?t specify it is read-only in EL2.
I did some tests to read from these registers in EL2. It worked.
It crashed the U-Boot with ?SError? exception if I tried to write something into one of these registers.

For registers after ?mpu? which are ?boot_scratch_cold0? ? ?boot_scratch_cold9?:

If you click into one of this boot scratch registers:
https://www.intel.com/content/www/us/en/programmable/hps/stratix-10/hps.html#topic/lom1505406925262.html
You don?t see any requirement about the processor need to be in secure mode to access this register.

Previously I mentioned these registers are read-only in EL2.
They are actually read/writable in EL2 and EL3. Sorry for the misinformation

The clock manager drivers are writing/reading clock settings into these boot scratch registers
so changes are not necessary for EL2.

In summary, although ?boot_scratch_cold0? ? ?boot_scratch_cold9? registers are part of
System Manager, but they are not marked as ?secure zone?.
I missed this when working on this ATF flow :(

[PATCH v2 10/21] arm: socfpga: Add secure register access helper functions for SoC 64bits

[Marek Vasut]

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.

[PATCH v2 01/21] configs: agilex: Remove CONFIG_OF_EMBED

[Marek Vasut]

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 at intel.com
>>>>>
>>>>>  wrote:
>>>>>
>>>>> From: Chee Hong Ang <
>>>>>
>>>>> <mailto:chee.hong.ang@intel.com>
>>>>>
>>>>> chee.hong.ang at 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 ?

[PATCH v2 01/21] configs: agilex: Remove CONFIG_OF_EMBED

[Westergreen, Dalon]

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 at intel.com

>


<mailto:chee.hong.ang@intel.com>

chee.hong.ang at intel.com



 wrote:


From: Chee Hong Ang <


<mailto:

<mailto:chee.hong.ang@intel.com>

chee.hong.ang at intel.com

>


<mailto:chee.hong.ang@intel.com>

chee.hong.ang at 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

[PATCH v2 01/21] configs: agilex: Remove CONFIG_OF_EMBED

[Marek Vasut]

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

[...]

[PATCH v2 01/21] configs: agilex: Remove CONFIG_OF_EMBED

[Westergreen, Dalon]

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 at intel.com
> > > > > > 
> > > > > >  wrote:
> > > > > > 
> > > > > > From: Chee Hong Ang <
> > > > > > 
> > > > > > <mailto:chee.hong.ang@intel.com>
> > > > > > 
> > > > > > chee.hong.ang at 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

[PATCH v2 10/21] arm: socfpga: Add secure register access helper functions for SoC 64bits

[Ang, Chee Hong]

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

[PATCH v2 10/21] arm: socfpga: Add secure register access helper functions for SoC 64bits

[Ang, Chee Hong]

> > 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/gil1505406886282.html

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#topic/jng1505406892594.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/stmmac/dwmac-socfpga.c#L302

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.

[PATCH v2 10/21] arm: socfpga: Add secure register access helper functions for SoC 64bits

[Simon Goldschmidt]

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/gil1505406886282.html
>
> 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#topic/jng1505406892594.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/stmmac/dwmac-socfpga.c#L302
>
> 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.
>

[PATCH v2 10/21] arm: socfpga: Add secure register access helper functions for SoC 64bits

[Ang, Chee Hong]

Ang, Chee Hong <chee.hong.ang at intel.com<mailto: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/gil1505406886282.html

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#topic/jng1505406892594.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/stmmac/dwmac-socfpga.c#L302

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.