[PATCH v2 0/8] Suspend to RAM support for K3 J7200

[PATCH v2 0/8] Suspend to RAM support for K3 J7200

[Thomas Richard]

This series is the U-Boot part of the work to add the suspend to RAM
support for the K3 J7200 EVM board.

During the boot R5 SPL makes a copy of DM-Firmware in memory.
Resume detection is done by reading a magic value in a pmic register
(set by DM-Firmware).

If a resume is detected, R5 SPL run the exit retention sequence of the
DDR. Then it restores TF-A in SRAM (it saved itself to DDR), and load
DM-Firmware using the copy done during the boot (fit image processing
is skipped).

The copy of TF-A/DM-Firmware, the SPL stack and malloc are located in a
reserved memory region (for the kernel point of view) to avoid any
memory corruption.

Changes in v2:
- Set exit retention code for DDR behind CONFIG_K3_J721E_DDRSS
- Check if TF-A is running in DRAM, if yes no need to restore it
- Remove BL31_START macro, and get TF-A start address from the fit image
- Remove the test_enter_suspend command
- Link to v1: https://lore.kernel.org/u-boot/20231016141135.325698-1-thomas.richard@bootlin.com/

Gowtham Tammana (1):
  DO NOT MERGE: arm: dts: k3-j7200-r5-common: Add pmic node for esm

Gregory CLEMENT (2):
  configs: j7200_evm_r5: Used reserved memory in DDR for stack
  configs: j7200_evm_r5: Move address used for allocation in the
    reserved space

Thomas Richard (5):
  board: ti: j721e: Add resume detection for J7200
  ram: k3-ddrss: Add exit retention support
  board: ti: j721e: Add the missing part of exit retention for k3-ddrss
    (J7200)
  board: ti: j721e: During resume spl restores TF-A and DM-Firmware
  arm: mach-k3: j7200: Skip fit processing when resuming

 .../arm/dts/k3-j7200-r5-common-proc-board.dts |  17 +-
 arch/arm/mach-k3/common.c                     |  48 ++-
 arch/arm/mach-k3/include/mach/j721e_spl.h     |  28 ++
 arch/arm/mach-k3/sysfw-loader.c               |  16 +-
 board/ti/j721e/evm.c                          |  79 +++++
 configs/j7200_evm_r5_defconfig                |   4 +-
 drivers/ram/k3-ddrss/k3-ddrss.c               | 307 ++++++++++++++++++
 7 files changed, 489 insertions(+), 10 deletions(-)

-- 
2.39.2

[PATCH v2 1/8] DO NOT MERGE: arm: dts: k3-j7200-r5-common: Add pmic node for esm

[Thomas Richard]

From: Gowtham Tammana <g-tammana@ti.com>

Add pmic tps659413 node needed for ESM error event handling.

Signed-off-by: Gowtham Tammana <g-tammana@ti.com>
Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---

(no changes since v1)

 arch/arm/dts/k3-j7200-r5-common-proc-board.dts | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/arch/arm/dts/k3-j7200-r5-common-proc-board.dts b/arch/arm/dts/k3-j7200-r5-common-proc-board.dts
index e62f9218e8..2bf0d5e3c0 100644
--- a/arch/arm/dts/k3-j7200-r5-common-proc-board.dts
+++ b/arch/arm/dts/k3-j7200-r5-common-proc-board.dts
@@ -249,13 +249,24 @@
 
 &wkup_i2c0 {
 	bootph-pre-ram;
+	pinctrl-names = "default";
+	pinctrl-0 = <&wkup_i2c0_pins_default>;
+	clock-frequency = <400000>;
+
+	tps659413: tps659413@48 {
+		compatible = "ti,tps659413";
+		reg = <0x48>;
+		bootph-pre-ram;
+
+		regulators_a: regulators {
+			bootph-pre-ram;
+		};
+	};
+
 	lp876441: lp876441@4c {
 		compatible = "ti,lp876441";
 		reg = <0x4c>;
 		bootph-pre-ram;
-		pinctrl-names = "default";
-		pinctrl-0 = <&wkup_i2c0_pins_default>;
-		clock-frequency = <400000>;
 
 		regulators: regulators {
 			bootph-pre-ram;
-- 
2.39.2

[PATCH v2 2/8] configs: j7200_evm_r5: Used reserved memory in DDR for stack

[Thomas Richard]

From: Gregory CLEMENT <gregory.clement@bootlin.com>

When resuming from suspend to ram, we load again the DM firmware.
However, we have to be sure to not modify the memory used by
Linux.

Currently the SPL stack in DDR was in place that could be used by
Linux. Instead of it use a memory address that is located in a
reserved location that won't be used by Linux.

Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---

(no changes since v1)

 configs/j7200_evm_r5_defconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configs/j7200_evm_r5_defconfig b/configs/j7200_evm_r5_defconfig
index c4dd33627b..7450529d66 100644
--- a/configs/j7200_evm_r5_defconfig
+++ b/configs/j7200_evm_r5_defconfig
@@ -20,7 +20,7 @@ CONFIG_DM_RESET=y
 CONFIG_SPL_MMC=y
 CONFIG_SPL_SERIAL=y
 CONFIG_SPL_DRIVERS_MISC=y
-CONFIG_SPL_STACK_R_ADDR=0x82000000
+CONFIG_SPL_STACK_R_ADDR=0xa5300000
 CONFIG_SPL_FS_FAT=y
 CONFIG_SPL_LIBDISK_SUPPORT=y
 CONFIG_SPL_SPI_FLASH_SUPPORT=y
-- 
2.39.2

Re: [PATCH v2 2/8] configs: j7200_evm_r5: Used reserved memory in DDR for stack

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1518 bytes --]

On Tue, Nov 07, 2023 at 05:17:56PM +0100, Thomas Richard wrote:
> From: Gregory CLEMENT <gregory.clement@bootlin.com>
> 
> When resuming from suspend to ram, we load again the DM firmware.
> However, we have to be sure to not modify the memory used by
> Linux.
> 
> Currently the SPL stack in DDR was in place that could be used by
> Linux. Instead of it use a memory address that is located in a
> reserved location that won't be used by Linux.
> 
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> ---
> 
> (no changes since v1)
> 
>  configs/j7200_evm_r5_defconfig | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/configs/j7200_evm_r5_defconfig b/configs/j7200_evm_r5_defconfig
> index c4dd33627b..7450529d66 100644
> --- a/configs/j7200_evm_r5_defconfig
> +++ b/configs/j7200_evm_r5_defconfig
> @@ -20,7 +20,7 @@ CONFIG_DM_RESET=y
>  CONFIG_SPL_MMC=y
>  CONFIG_SPL_SERIAL=y
>  CONFIG_SPL_DRIVERS_MISC=y
> -CONFIG_SPL_STACK_R_ADDR=0x82000000
> +CONFIG_SPL_STACK_R_ADDR=0xa5300000
>  CONFIG_SPL_FS_FAT=y
>  CONFIG_SPL_LIBDISK_SUPPORT=y
>  CONFIG_SPL_SPI_FLASH_SUPPORT=y

This is fine but I want to see a previous change that sets
CONFIG_SPL_SIZE_LIMIT and related to the right / useful values
(j721e/j721s2 and some other K3 platforms set this as well) for j7200,
and then SPL_SIZE_LIMIT_PROVIDE_STACK being set in this patch, and other
options adjusted as needed.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH v2 3/8] configs: j7200_evm_r5: Move address used for allocation in the reserved space

[Thomas Richard]

From: Gregory CLEMENT <gregory.clement@bootlin.com>

Else it could corrupt the memory of Linux when exiting suspend to ram.

Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
---

(no changes since v1)

 configs/j7200_evm_r5_defconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/configs/j7200_evm_r5_defconfig b/configs/j7200_evm_r5_defconfig
index 7450529d66..1e7daf9b4a 100644
--- a/configs/j7200_evm_r5_defconfig
+++ b/configs/j7200_evm_r5_defconfig
@@ -39,7 +39,7 @@ CONFIG_SPL_STACK_R=y
 CONFIG_SPL_SEPARATE_BSS=y
 CONFIG_SYS_SPL_MALLOC=y
 CONFIG_HAS_CUSTOM_SPL_MALLOC_START=y
-CONFIG_CUSTOM_SYS_SPL_MALLOC_ADDR=0x84000000
+CONFIG_CUSTOM_SYS_SPL_MALLOC_ADDR=0xa5400000
 CONFIG_SYS_SPL_MALLOC_SIZE=0x1000000
 CONFIG_SPL_EARLY_BSS=y
 CONFIG_SYS_MMCSD_RAW_MODE_U_BOOT_USE_SECTOR=y
-- 
2.39.2

[PATCH v2 4/8] board: ti: j721e: Add resume detection for J7200

[Thomas Richard]

Add the capability to detect a resume.
To detect the resume, SPL searches a magic value (0xBA) in a register
of PMICA.
This value is set by DM-Firmware during the suspend sequence.

Based on the work of Gregory CLEMENT <gregory.clement@bootlin.com>

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---

(no changes since v1)

 board/ti/j721e/evm.c | 54 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/board/ti/j721e/evm.c b/board/ti/j721e/evm.c
index 38fe447d8f..b4b94c8c69 100644
--- a/board/ti/j721e/evm.c
+++ b/board/ti/j721e/evm.c
@@ -22,6 +22,9 @@
 #include <spl.h>
 #include <dm.h>
 #include <dm/uclass-internal.h>
+#if (IS_ENABLED(CONFIG_SPL_BUILD) && IS_ENABLED(CONFIG_TARGET_J7200_R5_EVM))
+#include <power/pmic.h>
+#endif
 
 #include "../common/board_detect.h"
 
@@ -528,6 +531,57 @@ err_free_gpio:
 	}
 }
 
+#if (IS_ENABLED(CONFIG_SPL_BUILD) && IS_ENABLED(CONFIG_TARGET_J7200_R5_EVM))
+
+#define SCRATCH_PAD_REG_3 0xCB
+
+#define MAGIC_SUSPEND 0xBA
+
+static int resuming = -1;
+
+int board_is_resuming(void)
+{
+	struct udevice *pmica, *pmicb;
+	int ret;
+
+	if (resuming >= 0)
+		goto end;
+
+	ret = uclass_get_device_by_name(UCLASS_PMIC,
+					"tps659413@48", &pmica);
+	if (ret) {
+		printf("Getting PMICA init failed: %d\n", ret);
+		resuming = 0;
+		goto end;
+	}
+	debug("%s: PMICA is detected (%s)\n", __func__, pmica->name);
+
+	ret = uclass_get_device_by_name(UCLASS_PMIC,
+					"lp876441@4c", &pmicb);
+	if (ret) {
+		printf("Getting PMICB init failed: %d\n", ret);
+		resuming = 0;
+		goto end;
+	}
+	debug("%s: PMICB is detected (%s)\n", __func__, pmicb->name);
+
+	if ((pmic_reg_read(pmica, SCRATCH_PAD_REG_3) == MAGIC_SUSPEND)) {
+		resuming = 1;
+		debug("%s: board is resuming\n", __func__);
+
+		/* clean magic suspend */
+		if (pmic_reg_write(pmica, SCRATCH_PAD_REG_3, 0))
+			printf("Failed to clean magic value for suspend detection in PMICA\n");
+	} else {
+		resuming = 0;
+		debug("%s: board is booting (no resume detected)\n", __func__);
+	}
+end:
+	return resuming;
+}
+
+#endif /* CONFIG_SPL_BUILD && CONFIG_TARGET_J7200_R5_EVM */
+
 void spl_board_init(void)
 {
 #if defined(CONFIG_ESM_K3) || defined(CONFIG_ESM_PMIC)
-- 
2.39.2

Re: [PATCH v2 4/8] board: ti: j721e: Add resume detection for J7200

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1896 bytes --]

On Tue, Nov 07, 2023 at 05:17:58PM +0100, Thomas Richard wrote:

> Add the capability to detect a resume.
> To detect the resume, SPL searches a magic value (0xBA) in a register
> of PMICA.
> This value is set by DM-Firmware during the suspend sequence.
> 
> Based on the work of Gregory CLEMENT <gregory.clement@bootlin.com>
> 
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> ---
> 
> (no changes since v1)
> 
>  board/ti/j721e/evm.c | 54 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 54 insertions(+)
> 
> diff --git a/board/ti/j721e/evm.c b/board/ti/j721e/evm.c
> index 38fe447d8f..b4b94c8c69 100644
> --- a/board/ti/j721e/evm.c
> +++ b/board/ti/j721e/evm.c
> @@ -22,6 +22,9 @@
>  #include <spl.h>
>  #include <dm.h>
>  #include <dm/uclass-internal.h>
> +#if (IS_ENABLED(CONFIG_SPL_BUILD) && IS_ENABLED(CONFIG_TARGET_J7200_R5_EVM))
> +#include <power/pmic.h>
> +#endif

We _really_ should not be guarding include files. If the code doesn't
compile with the header included, we need to figure out what's wrong
with the header or if, ugh, a more worst case of moving the #include to
by the now-guarded functions.

[snip]
> +#if (IS_ENABLED(CONFIG_SPL_BUILD) && IS_ENABLED(CONFIG_TARGET_J7200_R5_EVM))
> +
> +#define SCRATCH_PAD_REG_3 0xCB
> +
> +#define MAGIC_SUSPEND 0xBA
> +
> +static int resuming = -1;
> +
> +int board_is_resuming(void)

I wonder if we should (a) have a file for just r5 related code and then
(b) rely on this being discarded on link anyhow for non-SPL and we just
comment that this is only used in resume in SPL. Guarding functions with
#if (IS_ENABLED(CONFIG_A) && IS_ENABLED(CONFIG_B))
isn't what the macros were intended for (and yes, I know checkpatch.pl
complains otherwise, which is a point of contention).

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH v2 5/8] ram: k3-ddrss: Add exit retention support

[Thomas Richard]

Add the exit retention support.
The enter retention is done by DM-Firmware.
A part of the exit retention sequence is specific to the board.
It is done in board_k3_ddrss_lpddr4_release_retention.

Based on the work of Gregory CLEMENT <gregory.clement@bootlin.com>

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>

---

Changes in v2:
- Set exit retention code for DDR behind CONFIG_K3_J721E_DDRSS

 drivers/ram/k3-ddrss/k3-ddrss.c | 307 ++++++++++++++++++++++++++++++++
 1 file changed, 307 insertions(+)

diff --git a/drivers/ram/k3-ddrss/k3-ddrss.c b/drivers/ram/k3-ddrss/k3-ddrss.c
index b54557f02c..e47f615032 100644
--- a/drivers/ram/k3-ddrss/k3-ddrss.c
+++ b/drivers/ram/k3-ddrss/k3-ddrss.c
@@ -413,6 +413,306 @@ static int k3_ddrss_ofdata_to_priv(struct udevice *dev)
 	return ret;
 }
 
+#if defined(CONFIG_K3_J721E_DDRSS)
+
+void __weak board_k3_ddrss_lpddr4_release_retention(void) {}
+
+int __weak board_is_resuming(void)
+{
+	return 0;
+}
+
+#define PLL12_CTRL  0x0068C020
+
+#define k3_ddrss_readreg(k3_ddrss, block, shift, reg, pt) do {		\
+		u32 offset = 0U;					\
+		u32 result = 0U;					\
+		TH_OFFSET_FROM_REG(reg, shift, offset);			\
+		result = k3_ddrss->driverdt->readreg(&k3_ddrss->pd, block, offset, pt); \
+		if (result > 0U) {					\
+			printf("%s: Failed to read %s\n", __func__, xstr(reg));	\
+			hang();						\
+		}							\
+	} while (0)
+
+#define k3_ddrss_writereg(k3_ddrss, block, shift, reg, value) do {	\
+		u32 offset = 0U;					\
+		u32 result = 0U;					\
+		TH_OFFSET_FROM_REG(reg, shift, offset);			\
+		result = k3_ddrss->driverdt->writereg(&k3_ddrss->pd, block, offset, value); \
+		if (result > 0U) {					\
+			printf("%s: Failed to write %s\n", __func__, xstr(reg)); \
+			hang();						\
+		}							\
+	} while (0)
+
+#define k3_ddrss_readreg_ctl(ddrss, reg, pt) \
+	k3_ddrss_readreg(ddrss, LPDDR4_CTL_REGS, CTL_SHIFT, reg, pt)
+
+#define k3_ddrss_readreg_pi(ddrss, reg, pt) \
+	k3_ddrss_readreg(ddrss, LPDDR4_PHY_INDEP_REGS, PI_SHIFT, reg, pt)
+
+#define k3_ddrss_readreg_phy(ddrss, reg, pt) \
+	k3_ddrss_readreg(ddrss, LPDDR4_PHY_REGS, PHY_SHIFT, reg, pt)
+
+#define k3_ddrss_writereg_ctl(ddrss, reg, value) \
+	k3_ddrss_writereg(ddrss, LPDDR4_CTL_REGS, CTL_SHIFT, reg, value)
+
+#define k3_ddrss_writereg_pi(ddrss, reg, value) \
+	k3_ddrss_writereg(ddrss, LPDDR4_PHY_INDEP_REGS, PI_SHIFT, reg, value)
+
+#define k3_ddrss_writereg_phy(ddrss, reg, value) \
+	k3_ddrss_writereg(ddrss, LPDDR4_PHY_REGS, PHY_SHIFT, reg, value)
+
+#define k3_ddrss_set_ctl(k3_ddrss, reg, mask) do {	\
+	u32 val;					\
+	k3_ddrss_readreg_ctl(ddrss, reg, &val);		\
+	val |= mask;					\
+	k3_ddrss_writereg_ctl(ddrss, reg, val);		\
+	} while (0)
+
+#define k3_ddrss_clr_ctl(k3_ddrss, reg, mask) do {		\
+		u32 val;					\
+		k3_ddrss_readreg_ctl(ddrss, reg, &val);		\
+		val &= ~(mask);					\
+		k3_ddrss_writereg_ctl(ddrss, reg, val);		\
+	} while (0)
+
+#define k3_ddrss_set_pi(k3_ddrss, reg, mask) do {		\
+		u32 val;					\
+		k3_ddrss_readreg_pi(ddrss, reg, &val);		\
+		val |= mask;					\
+		k3_ddrss_writereg_pi(ddrss, reg, val);		\
+	} while (0)
+
+#define k3_ddrss_clr_pi(k3_ddrss, reg, mask) do {		\
+		u32 val;					\
+		k3_ddrss_readreg_pi(ddrss, reg, &val);		\
+		val &= ~(mask);					\
+		k3_ddrss_writereg_pi(ddrss, reg, val);		\
+	} while (0)
+
+#define k3_ddrss_set_phy(ddrss, reg, mask) do {		\
+	u32 val;					\
+	k3_ddrss_readreg_phy(ddrss, reg, &val);		\
+	val |= mask;					\
+	k3_ddrss_writereg_phy(ddrss, reg, val);		\
+	} while (0)
+
+#define k3_ddrss_clr_phy(ddrss, reg, mask) do {		\
+	u32 val;					\
+	k3_ddrss_readreg_phy(ddrss, reg, &val);		\
+	val &= ~(mask);					\
+	k3_ddrss_writereg_phy(ddrss, reg, val);		\
+	} while (0)
+
+static void k3_ddrss_lpddr4_exit_retention(struct k3_ddrss_desc *ddrss)
+{
+	u32 regval;
+	unsigned long tmo;
+	volatile unsigned int val;
+
+	/* disable auto entry / exit */
+	k3_ddrss_clr_ctl(ddrss, LPDDR4__LP_STATE_CS0__REG, (0xF << 24) | (0xF << 16));
+
+	/* Configure DFI Interface, DDR retention exit occurs through PHY */
+	k3_ddrss_readreg_phy(ddrss, LPDDR4__PHY_SET_DFI_INPUT_0__REG, &regval);
+	regval &= ~0xF0F; // Set DFI_Input_1 = 0
+	regval |= 0x01;   // Set DFI Input_0 = 1
+	k3_ddrss_writereg_phy(ddrss, LPDDR4__PHY_SET_DFI_INPUT_0__REG, regval);
+
+	/* PWRUP_SREFRESH_EXIT = 1 */
+	k3_ddrss_set_ctl(ddrss, LPDDR4__PWRUP_SREFRESH_EXIT__REG, 0x1);
+
+	/* PI_PWRUP_SREFRESH_EXIT = 0 */
+	k3_ddrss_clr_ctl(ddrss, LPDDR4__PI_COL_DIFF__REG, 0x1 << 16);
+
+	/* DFIBUS_BOOT_FREQ = 0 */
+	k3_ddrss_clr_ctl(ddrss, LPDDR4__DFIBUS_BOOT_FREQ__REG, 0x3);
+
+	/* DFIBUS_FREQ_INIT = 2 */
+	k3_ddrss_readreg_ctl(ddrss, LPDDR4__PHY_INDEP_TRAIN_MODE__REG, &regval);
+	regval &= ~(0x3 << 24);
+	regval |= (0x1 << 24);
+	k3_ddrss_writereg_ctl(ddrss, LPDDR4__PHY_INDEP_TRAIN_MODE__REG, regval);
+
+	/* Force Leveling during Initialization, Enable Link Training */
+
+	/* PI_INIT_LVL_EN = 1 */
+	k3_ddrss_set_pi(ddrss, LPDDR4__PI_NORMAL_LVL_SEQ__REG, (1 << 8));
+
+	/* PI_DRAM_INIT_EN = 0 */
+	k3_ddrss_clr_pi(ddrss, LPDDR4__PI_DLL_RST__REG, (0x1 << 8));
+
+	/* PI_DFI_PHYMSTR_STATE_SEL_R = 1  (force memory into self-refresh) */
+	k3_ddrss_set_pi(ddrss, LPDDR4__PI_DFI_VERSION__REG, (1 << 24));
+
+	/*  PHY_INDEP_INIT_MODE = 0 */
+	k3_ddrss_clr_ctl(ddrss, LPDDR4__PHY_INDEP_TRAIN_MODE__REG, (0x1 << 16));
+	/* PHY_INDEP_TRAIN_MODE = 1 */
+	k3_ddrss_set_ctl(ddrss, LPDDR4__PHY_INDEP_TRAIN_MODE__REG, 0x1);
+
+	/* PI_INIT_WORK_FREQ = 1 */
+	k3_ddrss_readreg_pi(ddrss, LPDDR4__PI_INIT_WORK_FREQ__REG, &regval);
+	regval &= ~0x1F;
+	regval |= 0x01;
+	k3_ddrss_writereg_pi(ddrss, LPDDR4__PI_INIT_WORK_FREQ__REG, regval);
+
+	/* PI_FREQ_MAP[2:0] */
+	k3_ddrss_writereg_pi(ddrss, LPDDR4__PI_FREQ_MAP__REG, 0x03);
+
+	/* Training/leveling configurations for different frequency set points */
+
+	/* PI_CALVL_EN_F0 = 01b; PI_CALVL_EN_F1 = 01b; PI_CALVL_EN_F2 = 01b */
+	k3_ddrss_readreg_pi(ddrss, LPDDR4__PI_CALVL_EN_F0__REG, &regval);
+	regval &= ~0x030303;
+	regval |= 0x010101;
+	k3_ddrss_writereg_pi(ddrss, LPDDR4__PI_CALVL_EN_F0__REG, regval);
+
+	/* PI_WRLVL_EN_F0 = 00b; PI_WRLVL_EN_F1 = 01b */
+	k3_ddrss_readreg_pi(ddrss, LPDDR4__PI_TDFI_CTRL_DELAY_F1__REG, &regval);
+	regval &= ~0x03030000;
+	regval |= 0x01000000;
+	k3_ddrss_writereg_pi(ddrss, LPDDR4__PI_TDFI_CTRL_DELAY_F1__REG, regval);
+
+	/* PI_WRLVL_EN_F2 = 01b */
+	k3_ddrss_readreg_pi(ddrss, LPDDR4__PI_WRLVL_EN_F2__REG, &regval);
+	regval &= ~0x03;
+	regval |= 0x01;
+	k3_ddrss_writereg_pi(ddrss, LPDDR4__PI_WRLVL_EN_F2__REG, regval);
+
+	/* PI_RDLVL_EN_F0 = 00b; PI_RDLVL_EN_F1 = 01b */
+	k3_ddrss_readreg_pi(ddrss, LPDDR4__PI_RDLVL_EN_F0__REG, &regval);
+	regval &= ~0x030003;
+	regval |= 0x010000;
+	k3_ddrss_writereg_pi(ddrss, LPDDR4__PI_RDLVL_EN_F0__REG, regval);
+
+	/* PI_RDLVL_EN_F2 = 01b */
+	k3_ddrss_readreg_pi(ddrss, LPDDR4__PI_RDLVL_EN_F2__REG, &regval);
+	regval &= ~0x03;
+	regval |= 0x01;
+	k3_ddrss_writereg_pi(ddrss, LPDDR4__PI_RDLVL_EN_F2__REG, regval);
+
+	/* PI_RDLVL_GATE_EN_F0 = 00b; PI_RDLVL_GATE_EN_F1 = 01b */
+	k3_ddrss_readreg_pi(ddrss, LPDDR4__PI_RDLVL_EN_F0__REG, &regval);
+	regval &= ~0x03000300;
+	regval |= 0x01000000;
+	k3_ddrss_writereg_pi(ddrss, LPDDR4__PI_RDLVL_EN_F0__REG, regval);
+
+	/* PI_RDLVL_GATE_EN_F2 = 01b */
+	k3_ddrss_readreg_pi(ddrss, LPDDR4__PI_RDLVL_EN_F2__REG, &regval);
+	regval &= ~0x0300;
+	regval |= 0x0100;
+	k3_ddrss_writereg_pi(ddrss, LPDDR4__PI_RDLVL_EN_F2__REG, regval);
+
+	/* PI_WDQLVL_EN_F0 = 00b */
+	k3_ddrss_clr_pi(ddrss, LPDDR4__PI_WDQLVL_VREF_DELTA_F0__REG, (0x3 << 8));
+
+	/* PI_WDQLVL_EN_F1 = 01b */
+	k3_ddrss_readreg_pi(ddrss, LPDDR4__PI_WDQLVL_VREF_DELTA_F1__REG, &regval);
+	regval &= ~(0x3 << 24);
+	regval |= (0x1 << 24);
+	k3_ddrss_writereg_pi(ddrss, LPDDR4__PI_WDQLVL_VREF_DELTA_F1__REG, regval);
+
+	/* PI_WDQLVL_EN_F2 = 01b */
+	k3_ddrss_readreg_pi(ddrss, LPDDR4__PI_WDQLVL_VREF_DELTA_F2__REG, &regval);
+	regval &= ~(0x3 << 8);
+	regval |= (0x1 << 8);
+	k3_ddrss_writereg_pi(ddrss, LPDDR4__PI_WDQLVL_VREF_DELTA_F2__REG, regval);
+
+	*(unsigned int *)PLL12_CTRL |= 0x80000000;
+	val = *(volatile unsigned int *)PLL12_CTRL;
+
+	if ((val & 0x80000000) != 0x80000000)
+		val = *(volatile unsigned int *)PLL12_CTRL;
+
+	board_k3_ddrss_lpddr4_release_retention();
+
+	/* LPC_SR_PHYMSTR_EN */
+	k3_ddrss_clr_ctl(ddrss, LPDDR4__LPC_SR_CTRLUPD_EN__REG, (0x1 << 16));
+	/* PI_START=1 */
+	k3_ddrss_set_pi(ddrss, LPDDR4__PI_START__REG, 0x01);
+	/* START=1 */
+	k3_ddrss_set_ctl(ddrss, LPDDR4__START__REG, 0x01);
+
+	debug("%s: PPL12 = %x\n", __func__, *(volatile unsigned int *)PLL12_CTRL);
+	ddrss->ddr_fhs_cnt = 5;
+	k3_lpddr4_freq_update(ddrss);
+	debug("%s: PPL12 = %x\n", __func__, *(volatile unsigned int *)PLL12_CTRL);
+
+	tmo = timer_get_us() + 100000;
+	do {
+		k3_ddrss_readreg_pi(ddrss, LPDDR4__PI_INT_STATUS__REG, &regval);
+		debug("%s: DDRSS_PI_79 = %x\n", __func__, regval);
+		if (timer_get_us() > tmo) {
+			printf("%s:%d timeout error\n", __func__, __LINE__);
+			hang();
+		}
+	} while ((regval & (1 << 0)) == 0x00);
+
+	tmo = timer_get_us() + 4000;
+	do {
+		k3_ddrss_readreg_ctl(ddrss, LPDDR4__INT_STATUS_0__REG, &regval);
+		if (timer_get_us() > tmo) {
+			printf("%s:%d timeout error\n", __func__, __LINE__);
+			hang();
+		}
+	} while ((regval & (1 << 9)) == 0x00);
+
+	k3_ddrss_readreg_pi(ddrss, LPDDR4__PI_INT_STATUS__REG, &regval);
+	debug("%s: PI interrupt status: 0x%08x\n", __func__, regval);
+
+	k3_ddrss_readreg_ctl(ddrss, LPDDR4__INT_STATUS_0__REG, &regval);
+	debug("%s: Controller interrupt status: 0x%08x\n", __func__, regval);
+
+	debug("%s: Successfully exited Retention\n", __func__);
+
+	k3_ddrss_readreg_ctl(ddrss, LPDDR4__LP_STATE_CS0__REG, &regval);
+	debug("%s: LP State: 0x%08x\n", __func__, regval);
+
+	k3_ddrss_writereg_ctl(ddrss, LPDDR4__INT_ACK_0__REG, (0x1 << 10));
+
+	k3_ddrss_readreg_ctl(ddrss, LPDDR4__CKSRX_F1__REG, &regval);
+	regval &= ~(0x7F << 24);
+	regval |= (0x2 << 24); // set low power mode exit
+	k3_ddrss_writereg_ctl(ddrss, LPDDR4__CKSRX_F1__REG, regval);
+
+	k3_ddrss_readreg_ctl(ddrss, LPDDR4__LP_STATE_CS0__REG, &regval);
+	debug("%s: LP State: 0x%08x\n", __func__, regval);
+
+	/* wait until low power operation has been completed */
+	tmo = timer_get_us() + 4000;
+	do {
+		k3_ddrss_readreg_ctl(ddrss, LPDDR4__INT_STATUS_0__REG, &regval);
+		if (timer_get_us() > tmo) {
+			printf("%s:%d timeout error\n", __func__, __LINE__);
+			hang();
+		}
+	} while ((regval & (0x1 << 10)) == 0);
+
+	k3_ddrss_readreg_ctl(ddrss, LPDDR4__LP_STATE_CS0__REG, &regval);
+	debug("%s: LP State: 0x%08x\n", __func__, regval);
+
+	k3_ddrss_writereg_ctl(ddrss, LPDDR4__INT_ACK_0__REG, (0x1 << 10));
+
+	/*
+	 * bit 6 / 14 -- lp_state valid
+	 * bits 13:8 / 5:0 0x0F SRPD Long with Mem and Controller Clk Gating
+	 */
+	tmo = timer_get_us() + 4000;
+	do {
+		k3_ddrss_readreg_ctl(ddrss, LPDDR4__LP_STATE_CS0__REG, &regval);
+		if (timer_get_us() > tmo) {
+			printf("%s:%d timeout error\n", __func__, __LINE__);
+			hang();
+		}
+	} while ((regval & 0x4F4F) != 0x4040);
+
+	k3_ddrss_readreg_ctl(ddrss, LPDDR4__LP_STATE_CS0__REG, &regval);
+	debug("%s: LP State: 0x%08x\n", __func__, regval);
+}
+#endif /* CONFIG_K3_J721E_DDRSS */
+
 void k3_lpddr4_probe(struct k3_ddrss_desc *ddrss)
 {
 	u32 status = 0U;
@@ -637,6 +937,13 @@ static int k3_ddrss_probe(struct udevice *dev)
 	if (ret)
 		return ret;
 
+#if defined(CONFIG_K3_J721E_DDRSS)
+	if (board_is_resuming()) {
+		k3_ddrss_lpddr4_exit_retention(ddrss);
+		return 0;
+	}
+#endif
+
 	k3_lpddr4_start(ddrss);
 
 	if (ddrss->ti_ecc_enabled) {
-- 
2.39.2

[PATCH v2 6/8] board: ti: j721e: Add the missing part of exit retention for k3-ddrss (J7200)

[Thomas Richard]

Add the board specific part of the exit retention sequence for k3-ddrss

Based on the work of Gregory CLEMENT <gregory.clement@bootlin.com>

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
---

(no changes since v1)

 board/ti/j721e/evm.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/board/ti/j721e/evm.c b/board/ti/j721e/evm.c
index b4b94c8c69..e6f46f911a 100644
--- a/board/ti/j721e/evm.c
+++ b/board/ti/j721e/evm.c
@@ -533,15 +533,22 @@ err_free_gpio:
 
 #if (IS_ENABLED(CONFIG_SPL_BUILD) && IS_ENABLED(CONFIG_TARGET_J7200_R5_EVM))
 
+static struct udevice *pmica;
+static struct udevice *pmicb;
+
+#define GPIO_OUT_1        0x3D
+
 #define SCRATCH_PAD_REG_3 0xCB
 
 #define MAGIC_SUSPEND 0xBA
 
+#define DDR_RET_VAL BIT(1)
+#define DDR_RET_CLK BIT(2)
+
 static int resuming = -1;
 
 int board_is_resuming(void)
 {
-	struct udevice *pmica, *pmicb;
 	int ret;
 
 	if (resuming >= 0)
@@ -580,6 +587,24 @@ end:
 	return resuming;
 }
 
+void board_k3_ddrss_lpddr4_release_retention(void)
+{
+	int regval;
+
+	/* Set DDR_RET Signal Low on PMIC B */
+	regval = pmic_reg_read(pmicb, GPIO_OUT_1) & ~DDR_RET_VAL;
+
+	pmic_reg_write(pmicb, GPIO_OUT_1, regval);
+
+	/* Now toggle the CLK of the latch for DDR ret */
+	pmic_reg_write(pmicb, GPIO_OUT_1, regval | DDR_RET_CLK);
+	pmic_reg_write(pmicb, GPIO_OUT_1, regval & ~(DDR_RET_CLK));
+	pmic_reg_write(pmicb, GPIO_OUT_1, regval | DDR_RET_CLK);
+	pmic_reg_write(pmicb, GPIO_OUT_1, regval & ~(DDR_RET_CLK));
+
+	pmic_reg_write(pmica, 0x86, 0x3);
+}
+
 #endif /* CONFIG_SPL_BUILD && CONFIG_TARGET_J7200_R5_EVM */
 
 void spl_board_init(void)
-- 
2.39.2

Re: [PATCH v2 6/8] board: ti: j721e: Add the missing part of exit retention for k3-ddrss (J7200)

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 862 bytes --]

On Tue, Nov 07, 2023 at 05:18:00PM +0100, Thomas Richard wrote:

> Add the board specific part of the exit retention sequence for k3-ddrss
> 
> Based on the work of Gregory CLEMENT <gregory.clement@bootlin.com>
> 
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>

If this ends up being physical board design specific and so someone
making a new design for this SoC with a custom board layout entirely
needs something else, we should think harder about what's in
board_is_resuming() vs what we document the function does. If however,
this is generic to the SoC and will be needed, it should be folded in
with the previous patch and the commit message expanded, and the
documentation part of this series explain clearly what the functions are
responsible for.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 6/8] board: ti: j721e: Add the missing part of exit retention for k3-ddrss (J7200)

[Thomas Richard]

On 11/7/23 19:18, Tom Rini wrote:
> On Tue, Nov 07, 2023 at 05:18:00PM +0100, Thomas Richard wrote:
> 
>> Add the board specific part of the exit retention sequence for k3-ddrss
>>
>> Based on the work of Gregory CLEMENT <gregory.clement@bootlin.com>
>>
>> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
>> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> 
> If this ends up being physical board design specific and so someone
> making a new design for this SoC with a custom board layout entirely
> needs something else, we should think harder about what's in
> board_is_resuming() vs what we document the function does. If however,
> this is generic to the SoC and will be needed, it should be folded in
> with the previous patch and the commit message expanded, and the
> documentation part of this series explain clearly what the functions are
> responsible for.
> 

Hi Tom,

Thanks for all these interesting feedback's.

From my point of view, board_is_resuming will work with all j7* SoCs and
all new designs for these SoCs.
This function only read and erase a scratchpad register in the PMIC A.
So it should be ok for all designs which have the PMIC@48.

For the function board_k3_ddrss_lpddr4_release_retention, it's a little
bit more tricky. The sequence is specific to the SOC, but the GPIOs used
are specific to the board.

Regards,

Thomas

-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 6/8] board: ti: j721e: Add the missing part of exit retention for k3-ddrss (J7200)

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1903 bytes --]

On Thu, Nov 09, 2023 at 11:43:12AM +0100, Thomas Richard wrote:
> On 11/7/23 19:18, Tom Rini wrote:
> > On Tue, Nov 07, 2023 at 05:18:00PM +0100, Thomas Richard wrote:
> > 
> >> Add the board specific part of the exit retention sequence for k3-ddrss
> >>
> >> Based on the work of Gregory CLEMENT <gregory.clement@bootlin.com>
> >>
> >> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> >> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> > 
> > If this ends up being physical board design specific and so someone
> > making a new design for this SoC with a custom board layout entirely
> > needs something else, we should think harder about what's in
> > board_is_resuming() vs what we document the function does. If however,
> > this is generic to the SoC and will be needed, it should be folded in
> > with the previous patch and the commit message expanded, and the
> > documentation part of this series explain clearly what the functions are
> > responsible for.
> > 
> 
> Hi Tom,
> 
> Thanks for all these interesting feedback's.
> 
> From my point of view, board_is_resuming will work with all j7* SoCs and
> all new designs for these SoCs.
> This function only read and erase a scratchpad register in the PMIC A.
> So it should be ok for all designs which have the PMIC@48.
> 
> For the function board_k3_ddrss_lpddr4_release_retention, it's a little
> bit more tricky. The sequence is specific to the SOC, but the GPIOs used
> are specific to the board.

I suspect this is going to be the case where a judgement call will have
to be made on theory vs practical on if the GPIOs will be duplicated to
custom designs (it's tricky! TI gave us a working solution, just use
it!) or changed based on other needs. I'll hope that perhaps people know
who to talk with in-private to get some unofficial feedback on such
things.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

[PATCH v2 7/8] board: ti: j721e: During resume spl restores TF-A and DM-Firmware

[Thomas Richard]

During the boot a copy of DM-Firmware is done in a reserved memory
area before it starts.
When resuming, R5 SPL uses this copy of DM-Firmware instead of the fit
image.
TF-A which saved itself in this same memory area, is restored in
SRAM by R5 SPL.

Based on the work of Gregory CLEMENT <gregory.clement@bootlin.com>

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>

---

Changes in v2:
- Check if TF-A is running in DRAM, if yes no need to restore it
- Remove BL31_START macro, and get TF-A start address from the fit image

 arch/arm/mach-k3/common.c                 | 48 ++++++++++++++++++++++-
 arch/arm/mach-k3/include/mach/j721e_spl.h | 28 +++++++++++++
 arch/arm/mach-k3/sysfw-loader.c           |  9 +++--
 3 files changed, 81 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
index a35110429b..737a1a28c6 100644
--- a/arch/arm/mach-k3/common.c
+++ b/arch/arm/mach-k3/common.c
@@ -26,6 +26,7 @@
 #include <env.h>
 #include <elf.h>
 #include <soc.h>
+#include <hang.h>
 
 #if IS_ENABLED(CONFIG_SYS_K3_SPL_ATF)
 enum {
@@ -221,6 +222,11 @@ void release_resources_for_core_shutdown(void)
 	}
 }
 
+__weak int board_is_resuming(void)
+{
+	return 0;
+}
+
 void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
 {
 	typedef void __noreturn (*image_entry_noargs_t)(void);
@@ -235,6 +241,32 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
 	if (ret)
 		panic("rproc failed to be initialized (%d)\n", ret);
 
+	if (board_is_resuming()) {
+#if IS_ENABLED(CONFIG_SOC_K3_J721E)
+		if (!valid_elf_image(LPM_DM_SAVE))
+			panic("%s: DM-Firmware image is not valid, it cannot be loaded\n",
+			      __func__);
+
+		loadaddr = load_elf_image_phdr(LPM_DM_SAVE);
+
+		/*
+		 * Check if the start address of TF-A is in DRAM.
+		 * If not it means TF-A was running in SRAM, so it shall be
+		 * restored.
+		 */
+		if (*(ulong *)(LPM_BL31_START_SAVE) < CFG_SYS_SDRAM_BASE)
+			memcpy((void *)*(uintptr_t *)(LPM_BL31_START_SAVE),
+			       (void *)LPM_BL31_SAVE, BL31_SIZE);
+
+		ret = rproc_load(1, *(ulong *)(LPM_BL31_RESUME_SAVE), BL31_SIZE);
+		if (ret)
+			panic("%s: ATF failed to load on rproc (%d)\n", __func__, ret);
+
+		debug("%s: jumping to address %x\n", __func__, loadaddr);
+		goto start_arm64;
+#endif
+	}
+
 	init_env();
 
 	if (!fit_image_info[IMAGE_ID_DM_FW].image_start) {
@@ -250,6 +282,10 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
 		fit_image_info[IMAGE_ID_ATF].image_start =
 			spl_image->entry_point;
 
+#if IS_ENABLED(CONFIG_SOC_K3_J721E)
+	*(uintptr_t *)(LPM_BL31_START_SAVE) = fit_image_info[IMAGE_ID_ATF].image_start;
+#endif
+
 	ret = rproc_load(1, fit_image_info[IMAGE_ID_ATF].image_start, 0x200);
 	if (ret)
 		panic("%s: ATF failed to load on rproc (%d)\n", __func__, ret);
@@ -289,8 +325,18 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
 		loadaddr = load_elf_image_phdr(loadaddr);
 	} else {
 		loadaddr = fit_image_info[IMAGE_ID_DM_FW].image_start;
-		if (valid_elf_image(loadaddr))
+		if (valid_elf_image(loadaddr)) {
 			loadaddr = load_elf_image_phdr(loadaddr);
+#if IS_ENABLED(CONFIG_SOC_K3_J721E)
+			if (fit_image_info[IMAGE_ID_DM_FW].image_len > (BUFFER_ADDR - LPM_DM_SAVE))
+				log_warning("%s\n: Not enough space to save DM-Firmware",
+					    __func__);
+			else
+				memcpy((void *)LPM_DM_SAVE,
+				       (void *)fit_image_info[IMAGE_ID_DM_FW].image_start,
+					   fit_image_info[IMAGE_ID_DM_FW].image_len);
+#endif
+		}
 	}
 
 	debug("%s: jumping to address %x\n", __func__, loadaddr);
diff --git a/arch/arm/mach-k3/include/mach/j721e_spl.h b/arch/arm/mach-k3/include/mach/j721e_spl.h
index e8947917a6..8e0f141ed6 100644
--- a/arch/arm/mach-k3/include/mach/j721e_spl.h
+++ b/arch/arm/mach-k3/include/mach/j721e_spl.h
@@ -42,4 +42,32 @@
 #define K3_PRIMARY_BOOTMODE		0x0
 #define K3_BACKUP_BOOTMODE		0x1
 
+/* Starting buffer address is 1MB before the stack address in DDR */
+#define BUFFER_ADDR (CONFIG_SPL_STACK_R_ADDR - SZ_1M)
+
+/* This is actually the whole size of the SRAM */
+#define BL31_SIZE    0x20000
+
+/* This address belongs to a reserved memory region for the point of view of
+ * Linux, U-boot SPL must use the same address to restore TF-A and resume
+ * entry point address
+ */
+#define LPM_SAVE		0xA5000000
+#define LPM_BL31_SAVE		LPM_SAVE
+#define LPM_BL31_RESUME_SAVE	LPM_BL31_SAVE + BL31_SIZE
+#define LPM_BL31_START_SAVE	LPM_BL31_RESUME_SAVE + __SIZEOF_POINTER__
+#define LPM_DM_SAVE		LPM_BL31_START_SAVE  + __SIZEOF_POINTER__
+
+/* Check if the copy of TF-A and DM-Firmware in DRAM does not overlap an
+ * over memory section.
+ * The resume address of TF-A is also saved in DRAM.
+ * At build time we don't know the DM-Firmware size, so we keep 512k to
+ * save it.
+ */
+#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_TARGET_J7200_R5_EVM)
+#if ((LPM_DM_SAVE + SZ_512K) > BUFFER_ADDR)
+#error Not enough space to save DM-Firmware, TF-A and context for S2R
+#endif
+#endif
+
 #endif
diff --git a/arch/arm/mach-k3/sysfw-loader.c b/arch/arm/mach-k3/sysfw-loader.c
index 9be2d9eaea..e6d452e590 100644
--- a/arch/arm/mach-k3/sysfw-loader.c
+++ b/arch/arm/mach-k3/sysfw-loader.c
@@ -84,13 +84,16 @@ static bool sysfw_loaded;
 static void *sysfw_load_address;
 
 /*
- * Populate SPL hook to override the default load address used by the SPL
- * loader function with a custom address for SYSFW loading.
+ * Populate SPL hook to override the default load address used by the
+ * SPL loader function with a custom address for SYSFW loading. In
+ * other case use also a custom address located in a reserved memory
+ * region. It ensures that Linux memory won't be corrupted by SPL during
+ * suspend to ram.
  */
 struct legacy_img_hdr *spl_get_load_buffer(ssize_t offset, size_t size)
 {
 	if (sysfw_loaded)
-		return (struct legacy_img_hdr *)(CONFIG_TEXT_BASE + offset);
+		return (struct legacy_img_hdr *)(BUFFER_ADDR + offset);
 	else if (sysfw_load_address)
 		return sysfw_load_address;
 	else
-- 
2.39.2

Re: [PATCH v2 7/8] board: ti: j721e: During resume spl restores TF-A and DM-Firmware

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3841 bytes --]

On Tue, Nov 07, 2023 at 05:18:01PM +0100, Thomas Richard wrote:

> During the boot a copy of DM-Firmware is done in a reserved memory
> area before it starts.
> When resuming, R5 SPL uses this copy of DM-Firmware instead of the fit
> image.
> TF-A which saved itself in this same memory area, is restored in
> SRAM by R5 SPL.
> 
> Based on the work of Gregory CLEMENT <gregory.clement@bootlin.com>
> 
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> 
> ---
> 
> Changes in v2:
> - Check if TF-A is running in DRAM, if yes no need to restore it
> - Remove BL31_START macro, and get TF-A start address from the fit image
> 
>  arch/arm/mach-k3/common.c                 | 48 ++++++++++++++++++++++-
>  arch/arm/mach-k3/include/mach/j721e_spl.h | 28 +++++++++++++
>  arch/arm/mach-k3/sysfw-loader.c           |  9 +++--
>  3 files changed, 81 insertions(+), 4 deletions(-)

One problem here is we aren't updating something under doc/, possibly
doc/board/ti/k3.rst unless you want to start populating something
entirely new under doc/develop/. And then using kernel-doc comments here
and including the code in the rst.  And so this is a global comment to
the series as well, and I'll leave sorting out when/where to introduce
that part up to your discretion.

[snip]
> diff --git a/arch/arm/mach-k3/include/mach/j721e_spl.h b/arch/arm/mach-k3/include/mach/j721e_spl.h
> index e8947917a6..8e0f141ed6 100644
> --- a/arch/arm/mach-k3/include/mach/j721e_spl.h
> +++ b/arch/arm/mach-k3/include/mach/j721e_spl.h
> @@ -42,4 +42,32 @@
>  #define K3_PRIMARY_BOOTMODE		0x0
>  #define K3_BACKUP_BOOTMODE		0x1
>  
> +/* Starting buffer address is 1MB before the stack address in DDR */
> +#define BUFFER_ADDR (CONFIG_SPL_STACK_R_ADDR - SZ_1M)

Assuming this is the full level of configurability we need to
BUFFER_ADDR please add some Kconfig logic to make sure we can't not set
CONFIG_SPL_STACK_R_ADDR. That might take a little trial-and-error and
if we just cannot enforce this via Kconfig, let me know and I'll think
about what we want to do instead next.

> +/* This is actually the whole size of the SRAM */
> +#define BL31_SIZE    0x20000

If this is all of SRAM then is there not already a define?

> +/* This address belongs to a reserved memory region for the point of view of
> + * Linux, U-boot SPL must use the same address to restore TF-A and resume
> + * entry point address
> + */
> +#define LPM_SAVE		0xA5000000
> +#define LPM_BL31_SAVE		LPM_SAVE
> +#define LPM_BL31_RESUME_SAVE	LPM_BL31_SAVE + BL31_SIZE
> +#define LPM_BL31_START_SAVE	LPM_BL31_RESUME_SAVE + __SIZEOF_POINTER__
> +#define LPM_DM_SAVE		LPM_BL31_START_SAVE  + __SIZEOF_POINTER__

Is any of this somewhere in the devicetree as well? I'm not saying "and
so pull it from that!" is a must, but I'd like to at least know "that
would be horrible to do", and fall back to having this needed
co-operation documented somewhere and ideally somewhere / somehow that
getting the values wrong is clear. Debugging resume failures at run-time
is much more painful than build time.

> +/* Check if the copy of TF-A and DM-Firmware in DRAM does not overlap an
> + * over memory section.
> + * The resume address of TF-A is also saved in DRAM.
> + * At build time we don't know the DM-Firmware size, so we keep 512k to
> + * save it.
> + */
> +#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_TARGET_J7200_R5_EVM)
> +#if ((LPM_DM_SAVE + SZ_512K) > BUFFER_ADDR)
> +#error Not enough space to save DM-Firmware, TF-A and context for S2R
> +#endif
> +#endif

This is really the kind of thing I'd like to see enforced via Kconfig if
at all possible. Sometimes yes we just have to CPP and #error, I just
want to know we can't.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 7/8] board: ti: j721e: During resume spl restores TF-A and DM-Firmware

[Andrew Davis]

On 11/7/23 10:18 AM, Thomas Richard wrote:
> During the boot a copy of DM-Firmware is done in a reserved memory
> area before it starts.
> When resuming, R5 SPL uses this copy of DM-Firmware instead of the fit
> image.
> TF-A which saved itself in this same memory area, is restored in
> SRAM by R5 SPL.
> 
> Based on the work of Gregory CLEMENT <gregory.clement@bootlin.com>
> 
> Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
> Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>
> 
> ---
> 
> Changes in v2:
> - Check if TF-A is running in DRAM, if yes no need to restore it
> - Remove BL31_START macro, and get TF-A start address from the fit image
> 
>   arch/arm/mach-k3/common.c                 | 48 ++++++++++++++++++++++-
>   arch/arm/mach-k3/include/mach/j721e_spl.h | 28 +++++++++++++
>   arch/arm/mach-k3/sysfw-loader.c           |  9 +++--
>   3 files changed, 81 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-k3/common.c b/arch/arm/mach-k3/common.c
> index a35110429b..737a1a28c6 100644
> --- a/arch/arm/mach-k3/common.c
> +++ b/arch/arm/mach-k3/common.c
> @@ -26,6 +26,7 @@
>   #include <env.h>
>   #include <elf.h>
>   #include <soc.h>
> +#include <hang.h>
>   
>   #if IS_ENABLED(CONFIG_SYS_K3_SPL_ATF)
>   enum {
> @@ -221,6 +222,11 @@ void release_resources_for_core_shutdown(void)
>   	}
>   }
>   
> +__weak int board_is_resuming(void)
> +{
> +	return 0;
> +}
> +
>   void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
>   {
>   	typedef void __noreturn (*image_entry_noargs_t)(void);
> @@ -235,6 +241,32 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
>   	if (ret)
>   		panic("rproc failed to be initialized (%d)\n", ret);
>   
> +	if (board_is_resuming()) {
> +#if IS_ENABLED(CONFIG_SOC_K3_J721E)
> +		if (!valid_elf_image(LPM_DM_SAVE))
> +			panic("%s: DM-Firmware image is not valid, it cannot be loaded\n",
> +			      __func__);
> +
> +		loadaddr = load_elf_image_phdr(LPM_DM_SAVE);
> +
> +		/*
> +		 * Check if the start address of TF-A is in DRAM.
> +		 * If not it means TF-A was running in SRAM, so it shall be
> +		 * restored.
> +		 */
> +		if (*(ulong *)(LPM_BL31_START_SAVE) < CFG_SYS_SDRAM_BASE)
> +			memcpy((void *)*(uintptr_t *)(LPM_BL31_START_SAVE),
> +			       (void *)LPM_BL31_SAVE, BL31_SIZE);

This will not work. The memory where TF-A is running will be firewalled and
SPL absolutely cannot be securely trusted to load TF-A. Especially from an
unencrypted location in DDR. TF-A must be loaded as it is today using signed
certificate images. You should know this, I explained it all when you tried
the same in TF-A:

https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/23992

NAK

Andrew

> +
> +		ret = rproc_load(1, *(ulong *)(LPM_BL31_RESUME_SAVE), BL31_SIZE);
> +		if (ret)
> +			panic("%s: ATF failed to load on rproc (%d)\n", __func__, ret);
> +
> +		debug("%s: jumping to address %x\n", __func__, loadaddr);
> +		goto start_arm64;
> +#endif
> +	}
> +
>   	init_env();
>   
>   	if (!fit_image_info[IMAGE_ID_DM_FW].image_start) {
> @@ -250,6 +282,10 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
>   		fit_image_info[IMAGE_ID_ATF].image_start =
>   			spl_image->entry_point;
>   
> +#if IS_ENABLED(CONFIG_SOC_K3_J721E)
> +	*(uintptr_t *)(LPM_BL31_START_SAVE) = fit_image_info[IMAGE_ID_ATF].image_start;
> +#endif
> +
>   	ret = rproc_load(1, fit_image_info[IMAGE_ID_ATF].image_start, 0x200);
>   	if (ret)
>   		panic("%s: ATF failed to load on rproc (%d)\n", __func__, ret);
> @@ -289,8 +325,18 @@ void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
>   		loadaddr = load_elf_image_phdr(loadaddr);
>   	} else {
>   		loadaddr = fit_image_info[IMAGE_ID_DM_FW].image_start;
> -		if (valid_elf_image(loadaddr))
> +		if (valid_elf_image(loadaddr)) {
>   			loadaddr = load_elf_image_phdr(loadaddr);
> +#if IS_ENABLED(CONFIG_SOC_K3_J721E)
> +			if (fit_image_info[IMAGE_ID_DM_FW].image_len > (BUFFER_ADDR - LPM_DM_SAVE))
> +				log_warning("%s\n: Not enough space to save DM-Firmware",
> +					    __func__);
> +			else
> +				memcpy((void *)LPM_DM_SAVE,
> +				       (void *)fit_image_info[IMAGE_ID_DM_FW].image_start,
> +					   fit_image_info[IMAGE_ID_DM_FW].image_len);
> +#endif
> +		}
>   	}
>   
>   	debug("%s: jumping to address %x\n", __func__, loadaddr);
> diff --git a/arch/arm/mach-k3/include/mach/j721e_spl.h b/arch/arm/mach-k3/include/mach/j721e_spl.h
> index e8947917a6..8e0f141ed6 100644
> --- a/arch/arm/mach-k3/include/mach/j721e_spl.h
> +++ b/arch/arm/mach-k3/include/mach/j721e_spl.h
> @@ -42,4 +42,32 @@
>   #define K3_PRIMARY_BOOTMODE		0x0
>   #define K3_BACKUP_BOOTMODE		0x1
>   
> +/* Starting buffer address is 1MB before the stack address in DDR */
> +#define BUFFER_ADDR (CONFIG_SPL_STACK_R_ADDR - SZ_1M)
> +
> +/* This is actually the whole size of the SRAM */
> +#define BL31_SIZE    0x20000
> +
> +/* This address belongs to a reserved memory region for the point of view of
> + * Linux, U-boot SPL must use the same address to restore TF-A and resume
> + * entry point address
> + */
> +#define LPM_SAVE		0xA5000000
> +#define LPM_BL31_SAVE		LPM_SAVE
> +#define LPM_BL31_RESUME_SAVE	LPM_BL31_SAVE + BL31_SIZE
> +#define LPM_BL31_START_SAVE	LPM_BL31_RESUME_SAVE + __SIZEOF_POINTER__
> +#define LPM_DM_SAVE		LPM_BL31_START_SAVE  + __SIZEOF_POINTER__
> +
> +/* Check if the copy of TF-A and DM-Firmware in DRAM does not overlap an
> + * over memory section.
> + * The resume address of TF-A is also saved in DRAM.
> + * At build time we don't know the DM-Firmware size, so we keep 512k to
> + * save it.
> + */
> +#if defined(CONFIG_SPL_BUILD) && defined(CONFIG_TARGET_J7200_R5_EVM)
> +#if ((LPM_DM_SAVE + SZ_512K) > BUFFER_ADDR)
> +#error Not enough space to save DM-Firmware, TF-A and context for S2R
> +#endif
> +#endif
> +
>   #endif
> diff --git a/arch/arm/mach-k3/sysfw-loader.c b/arch/arm/mach-k3/sysfw-loader.c
> index 9be2d9eaea..e6d452e590 100644
> --- a/arch/arm/mach-k3/sysfw-loader.c
> +++ b/arch/arm/mach-k3/sysfw-loader.c
> @@ -84,13 +84,16 @@ static bool sysfw_loaded;
>   static void *sysfw_load_address;
>   
>   /*
> - * Populate SPL hook to override the default load address used by the SPL
> - * loader function with a custom address for SYSFW loading.
> + * Populate SPL hook to override the default load address used by the
> + * SPL loader function with a custom address for SYSFW loading. In
> + * other case use also a custom address located in a reserved memory
> + * region. It ensures that Linux memory won't be corrupted by SPL during
> + * suspend to ram.
>    */
>   struct legacy_img_hdr *spl_get_load_buffer(ssize_t offset, size_t size)
>   {
>   	if (sysfw_loaded)
> -		return (struct legacy_img_hdr *)(CONFIG_TEXT_BASE + offset);
> +		return (struct legacy_img_hdr *)(BUFFER_ADDR + offset);
>   	else if (sysfw_load_address)
>   		return sysfw_load_address;
>   	else

Re: [PATCH v2 7/8] board: ti: j721e: During resume spl restores TF-A and DM-Firmware

[Thomas Richard]

On 11/8/23 18:30, Andrew Davis wrote:
>>   void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
>>   {
>>       typedef void __noreturn (*image_entry_noargs_t)(void);
>> @@ -235,6 +241,32 @@ void __noreturn jump_to_image_no_args(struct
>> spl_image_info *spl_image)
>>       if (ret)
>>           panic("rproc failed to be initialized (%d)\n", ret);
>>   +    if (board_is_resuming()) {
>> +#if IS_ENABLED(CONFIG_SOC_K3_J721E)
>> +        if (!valid_elf_image(LPM_DM_SAVE))
>> +            panic("%s: DM-Firmware image is not valid, it cannot be
>> loaded\n",
>> +                  __func__);
>> +
>> +        loadaddr = load_elf_image_phdr(LPM_DM_SAVE);
>> +
>> +        /*
>> +         * Check if the start address of TF-A is in DRAM.
>> +         * If not it means TF-A was running in SRAM, so it shall be
>> +         * restored.
>> +         */
>> +        if (*(ulong *)(LPM_BL31_START_SAVE) < CFG_SYS_SDRAM_BASE)
>> +            memcpy((void *)*(uintptr_t *)(LPM_BL31_START_SAVE),
>> +                   (void *)LPM_BL31_SAVE, BL31_SIZE);
> 
> This will not work. The memory where TF-A is running will be firewalled and
> SPL absolutely cannot be securely trusted to load TF-A. Especially from an
> unencrypted location in DDR. TF-A must be loaded as it is today using
> signed
> certificate images. You should know this, I explained it all when you tried
> the same in TF-A:
> 
> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/23992

Hi Andrew,

We understood that GP devices are not impacted (we had this information
from TI, probably Manorit I don't remember), and Manorit confirmed it in
the TF-A review.

Maybe I could add a check of the device type to not impact HS devices.

Regards,

Thomas

> 
> NAK
> 
> Andrew
> 

Re: [PATCH v2 7/8] board: ti: j721e: During resume spl restores TF-A and DM-Firmware

[Andrew Davis]

On 11/9/23 5:29 AM, Thomas Richard wrote:
> On 11/8/23 18:30, Andrew Davis wrote:
>>>    void __noreturn jump_to_image_no_args(struct spl_image_info *spl_image)
>>>    {
>>>        typedef void __noreturn (*image_entry_noargs_t)(void);
>>> @@ -235,6 +241,32 @@ void __noreturn jump_to_image_no_args(struct
>>> spl_image_info *spl_image)
>>>        if (ret)
>>>            panic("rproc failed to be initialized (%d)\n", ret);
>>>    +    if (board_is_resuming()) {
>>> +#if IS_ENABLED(CONFIG_SOC_K3_J721E)
>>> +        if (!valid_elf_image(LPM_DM_SAVE))
>>> +            panic("%s: DM-Firmware image is not valid, it cannot be
>>> loaded\n",
>>> +                  __func__);
>>> +
>>> +        loadaddr = load_elf_image_phdr(LPM_DM_SAVE);
>>> +
>>> +        /*
>>> +         * Check if the start address of TF-A is in DRAM.
>>> +         * If not it means TF-A was running in SRAM, so it shall be
>>> +         * restored.
>>> +         */
>>> +        if (*(ulong *)(LPM_BL31_START_SAVE) < CFG_SYS_SDRAM_BASE)
>>> +            memcpy((void *)*(uintptr_t *)(LPM_BL31_START_SAVE),
>>> +                   (void *)LPM_BL31_SAVE, BL31_SIZE);
>>
>> This will not work. The memory where TF-A is running will be firewalled and
>> SPL absolutely cannot be securely trusted to load TF-A. Especially from an
>> unencrypted location in DDR. TF-A must be loaded as it is today using
>> signed
>> certificate images. You should know this, I explained it all when you tried
>> the same in TF-A:
>>
>> https://review.trustedfirmware.org/c/TF-A/trusted-firmware-a/+/23992
> 
> Hi Andrew,
> 
> We understood that GP devices are not impacted (we had this information
> from TI, probably Manorit I don't remember), and Manorit confirmed it in
> the TF-A review.
> 
> Maybe I could add a check of the device type to not impact HS devices.
> 

I'm not interested in GP devices, and neither are most our customers.
Those are development devices, customers go to production with secured
devices.

Saying "let's make it work on GP only, then we will figure it out on HS
later" was a mistake we made back in OMAP class device days. It made
bringing support to production secured devices (HS) miserable as we had
to unroll all the hacks that only worked on the development devices (GP).

Your method here is completely unusable on HS and will need a ground up
rewrite for HS. Since the solution for HS will also work on GP, but
not the other way around, you need to start with the HS solution.

I'll make this same point over on the TF-A review then let's continue
discussion over there only. If you cant get the TF-A part in then no
need for this U-Boot part.

Andrew

> Regards,
> 
> Thomas
> 
>>
>> NAK
>>
>> Andrew
>>
> 

[PATCH v2 8/8] arm: mach-k3: j7200: Skip fit processing when resuming

[Thomas Richard]

No need to process fit image during resume.
All needed parts are already available in DRAM.

Based on the work of Gregory CLEMENT <gregory.clement@bootlin.com>

Signed-off-by: Thomas Richard <thomas.richard@bootlin.com>
Signed-off-by: Gregory CLEMENT <gregory.clement@bootlin.com>

---

(no changes since v1)

 arch/arm/mach-k3/sysfw-loader.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/arch/arm/mach-k3/sysfw-loader.c b/arch/arm/mach-k3/sysfw-loader.c
index e6d452e590..89daf9b7f8 100644
--- a/arch/arm/mach-k3/sysfw-loader.c
+++ b/arch/arm/mach-k3/sysfw-loader.c
@@ -100,6 +100,11 @@ struct legacy_img_hdr *spl_get_load_buffer(ssize_t offset, size_t size)
 		panic("SYSFW load address not defined!");
 }
 
+__weak int board_is_resuming(void)
+{
+	return 0;
+}
+
 /*
  * Populate SPL hook to skip the default SPL loader FIT post-processing steps
  * during SYSFW loading and return to the calling function so we can perform
@@ -107,7 +112,7 @@ struct legacy_img_hdr *spl_get_load_buffer(ssize_t offset, size_t size)
  */
 bool spl_load_simple_fit_skip_processing(void)
 {
-	return !sysfw_loaded;
+	return board_is_resuming() ? true : !sysfw_loaded;
 }
 
 static int fit_get_data_by_name(const void *fit, int images, const char *name,
-- 
2.39.2

Re: [PATCH v2 0/8] Suspend to RAM support for K3 J7200

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1193 bytes --]

On Tue, Nov 07, 2023 at 05:17:54PM +0100, Thomas Richard wrote:

> This series is the U-Boot part of the work to add the suspend to RAM
> support for the K3 J7200 EVM board.
> 
> During the boot R5 SPL makes a copy of DM-Firmware in memory.
> Resume detection is done by reading a magic value in a pmic register
> (set by DM-Firmware).
> 
> If a resume is detected, R5 SPL run the exit retention sequence of the
> DDR. Then it restores TF-A in SRAM (it saved itself to DDR), and load
> DM-Firmware using the copy done during the boot (fit image processing
> is skipped).
> 
> The copy of TF-A/DM-Firmware, the SPL stack and malloc are located in a
> reserved memory region (for the kernel point of view) to avoid any
> memory corruption.

I'm glad to see all of this in general. In specific, this work will get
merged once (a) the DTS changes are upstream to the kernel and (b) TI
has/plans on doing further cleanup and re-org of the K3 support code
overall and how J7200 is handled vs J721E vs J784S4. At least the parts
that matter for this series I am hopeful won't take too long, and this
can be rebased on top of that work without being too difficult.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]