Am62l lpm #8

ti-sebin · 2025-02-18T11:07:45Z

Adding Deepsleep and RTC only + DDR LPM sequence

DhruvaG2000

Hi Sebin,

Thanks for the PR. We will still need some more rework when we go upstream with this but for now I have only left recommendations for minor improvements.

DhruvaG2000 · 2025-02-19T10:04:01Z

plat/ti/k3/common/am62l_psci.c

+#include <gtc.h>
+#include <plat_scmi_def.h>
+#include <rtc.h>
+#include <k3_console.h>


Please sort the includes

Thanks for the review Dhruva.
Updated the PR

DhruvaG2000 · 2025-02-19T10:27:07Z

plat/ti/k3/common/am62l_psci.c

+	/* TODO: create a static carvout for TIFS context save and restore */
+	uint64_t  context_save_addr = 0x90000000;
+	/* Pass 6 for RTC only + DDR mode */
+	uint32_t mode = 6; 


Is there no provision in this code to select mode at runtime?
Can we please have a TODO on what the actual plan for the mode selection logic will be?

Moved to a separate commit

DhruvaG2000 · 2025-02-19T10:35:42Z

plat/ti/k3/common/k3_bl1_setup.c

@@ -34,7 +34,8 @@ const mmap_region_t plat_k3_mmap[] = {
 #define MAIN_PLL_MMR_BASE			(0x04060000UL)
 #define MAIN_PLL_MMR_CFG_PLL8_HSDIV_CTRL0	(0x00008080UL)
 #define CTL_MMR_BASE_CFG5			(0x43050000U)
-#define CANUART_WAKE_OFF_MODE_STA		(0x1318U)
+#define CANUART_WAKE_OFF_MODE_STAT		(0x1318U)
+#define RTC_ONLY_PLUS_DDR_MAGIC_WORD	(0x6D555555U)


Nit: Check tabs here and align

Updated the PR

plat/ti/k3/common/k3_bl1_setup.c

DhruvaG2000 · 2025-02-19T10:42:03Z

plat/ti/k3/common/k3_bl1_setup.c

+
+		msg.buf = (uint8_t *)&a53_rom_msg_obj;
+		msg.len = sizeof(a53_rom_msg_obj);
+		ti_sci_transport_send(0, &msg);


unfortunately slipped through code review in PreBL PR, but this can easily be re-written as SP_NOTIFY instead of 0 no?

Changed to SP_NOTIFY, but for 62l this is a unused parameter int he function

DhruvaG2000 · 2025-02-19T10:50:26Z

plat/ti/k3/common/k3_bl1_setup.c

+	uart_ptr = (uint32_t *)(PADCONF_ADDR);
+	*uart_ptr = 0x50000;
+	uart_ptr = (uint32_t *)(PADCONF_ADDR + 0x4);
+	*uart_ptr = 0x10000;


Just wondering, why TIFS can't do this if it's needed for TIFS logs?
Also, isn't this duplicate code from what's being done just above??

Yes removed the extra commit

DhruvaG2000 · 2025-02-19T10:54:14Z

plat/ti/k3/board/am62l/soc.c

@@ -18,4 +22,12 @@ void ti_soc_init(void)
 {
 	generic_delay_timer_init();
 	ti_init_scmi_server();
+#ifdef TI_AM62L_LPM


It's anyway AM62L SOC specific. Do we still need this TI_AM62L_LPM ifdef?
IIRC this was more useful back when everything was in k3_* code?

Yes, it was not required, removed

plat/ti/k3/common/k3_bl31_setup.c

DhruvaG2000 · 2025-02-19T10:56:47Z

plat/ti/k3/common/k3_bl31_setup.c

@@ -19,6 +19,7 @@
 #include <k3_gicv3.h>
 #include <ti_sci.h>
 #include <ti_sci_transport.h>
+#include <board_def.h>


Why? Doesn't plat/ti/k3/include/platform_def.h already include this?

Yes, removed this, Thanks for pointing it out

plat/ti/k3/common/am62l_psci.c

plat/ti/k3/board/am62l/board.mk

plat/ti/k3/board/am62l/lpm/call_sram.S

DhruvaG2000 · 2025-02-20T12:07:42Z

plat/ti/k3/board/am62l/lpm/gtc.c

+#include <lib/mmio.h>
+#include <plat/common/platform.h>
+#include "gtc.h"
+#include "rtc.h"


I'm honestly not a big fan of the include " " convention. If your makefile includes are done correctly, this should just be <> . Check once if easy enough to do.
1 file is fine generally file.c including file.h
But in this case it's "rtc.h" != file.c

gtc.h is fine by me.

Updated the files
Thanks Dhruva

DhruvaG2000 · 2025-02-20T12:08:18Z

plat/ti/k3/board/am62l/lpm/gtc.c

+#define GTC_CFG1_CNTCR_EN      BIT(0)
+
+#define GTC_CFG0_BASE					(0xa80000UL)
+#define GTC_CFG1_BASE					(0xA90000UL)


Can we move all these #defines to the header file?

plat/ti/k3/board/am62l/lpm/gtc.c

DhruvaG2000 · 2025-02-20T12:25:27Z

plat/ti/k3/board/am62l/lpm/rtc.c

+#define RELOAD_FROM_BBD											BIT(31)
+
+#define RTC_BASE												(0x2b1f0000UL)
+#define WKUP_CTRL_MMR_SEC_2_BASE								(0x43020000UL)


All these base values will be SOC specific.
This is not the right place to #define them.

Moved to soc specific file
Thanks

DhruvaG2000 · 2025-02-20T12:26:11Z

plat/ti/k3/common/drivers/lpddr4/k3-ddrss.c

@@ -76,6 +76,8 @@ uint64_t ddr_ram_size = 0x80000000;
 #define DDR32SS_PMCTRL			(0x1000U)
 #define CANUART_WAKE_OFF_MODE_STAT	(0x1318U)
 #define RTC_ONLY_PLUS_DDR_MAGIC_WORD 	(0x6D555555U)
+#define WKUP_CTRL_MMR_SEC_4_BASE		(0x43040000UL)
+#define WKUP_CTRL_MMR_SEC_5_BASE		(0x43050000UL)


No base values in drivers.

Move to common file

DhruvaG2000 · 2025-02-20T12:26:36Z

plat/ti/k3/common/k3_bl1_setup.c

+#define CTL_MMR_BASE_CFG5					(0x43050000U)
+#define CANUART_WAKE_OFF_MODE_STAT			(0x1318U)
+#define RTC_ONLY_PLUS_DDR_MAGIC_WORD		(0x6D555555U)
+#define BL1_DONE_MSG_ID						(0x810A)


tabs vs spaces issues here and few lines above... please fix

Fixed it, Thanks

plat/ti/k3/common/k3_bl1_setup.c

DhruvaG2000 · 2025-02-20T12:28:03Z

plat/ti/k3/common/drivers/ti_sci/ti_sci.c

@@ -1789,3 +1789,44 @@ int ti_sci_lpm_get_next_sys_mode(uint8_t *next_mode)

 	return 0;
 }
+


For this commit, the $subject should probably mention TISCI... not just LPM.

Thanks Dhruva, updated the commit message

During AM62L LPM sequence TFA needs to send prepare sleep message to TIFS. Add support to send prepare sleep message to TIFS Change-Id: I9e9ef8dd2cfe10ca27760420d01c12545665522c Signed-off-by: Sebin Francis <[email protected]>

Device needs to go through a differnt boot sequence during RTC + DDR low power mode resume. RTC only + DDR resume can be detected by checking the OFF_MODE magic word in WKUP CTRL MMR. During resume 1. Device needs to send CORE_RESUME message to TIFS compared to sending done message to SEC ROM. 2. DRR needs to go through few extra sequence in RTC only + DDR low power mode to initialize the DDR without losing it context. Update the DDR driver to do the extra sequence when device is resuming from RTC only + DDR low power mode. Change-Id: I85a56c0a5d4722852b635e302cab73b726717965 Signed-off-by: Sebin Francis <[email protected]>

Add drivers that are required during low power mode. 1. RTC: Init, suspend and resume support 2. GTC: Save and restore of GTC time 3. Trace: Trace support during LPM 4. PLL: Save and restore of PLLs 5. PSC: Turn off and ON of LPSCs 6. Stub: Low level LPM sequence implementation 7. call-sram.S: Assembly file for initial resume sequence before executing c code 8. plat.ld: Custom linker file for placing stub in WKUP SRAM Change-Id: I0b6cea5d30f66c8d8370b6114fc8336756f36c4c Signed-off-by: Sebin Francis <[email protected]>

RTC used as a wake up source. Initialize the RTC during boot. LPM STUB will be executing the low level LPM sequence. it will keep the DDR in self refresh. Because of which it cannot run from DDR. Copy the stub from DDR to wkup sram. Add wkup sram in the mmu map table. Increase the BL31 size to accommodate the LPM stub. Change-Id: Idfaa681394371afb489a96b083758d12a5910616 Signed-off-by: Sebin Francis <[email protected]>

Add PSCI handlers for LPM entry and exit. In the handlers call the low level LPM sequence handlers. Change-Id: I0bf32ddb9751866fcfa04c75e7a40d8c5e79c2d7 Signed-off-by: Sebin Francis <[email protected]>

DhruvaG2000 · 2025-02-21T06:03:01Z

plat/ti/k3/board/am62l/lpm/lpm_trace.c

+	 */
+	do {
+		val = mmio_read_32(K3_WKUP_UART_BASE_ADDRESS + UART_16550_LSR);
+	} while ((i++ < 10000U) && ((val & UART_16550_LSR_TX_FIFO_E) == 0U));


If you're trying to do some kind of timeout mechanism follow something similar to what has been done in ti_mailbox_poll_status
Use a #define like K3_UART_TIMEOUT_TX or something better and take it from the plat/ti/k3/include/platform_def.h

DhruvaG2000 · 2025-02-21T06:07:34Z

plat/ti/k3/board/am62l/lpm/rtc.c

+	/* Unlock RTC MMRs */
+	mmio_write_32(K3_RTC_BASE + RTC_KICK0, 0x83E70B13);
+	mmio_write_32(K3_RTC_BASE + RTC_KICK1, 0x95A4F1E0);
+	while ((mmio_read_32(K3_RTC_BASE + RTC_GENRAL_CTL) & BIT(23)) == 0U) {


Wherever possible please have some timeout mechanisms.
Any potential while(1) scenario does not look good.

DhruvaG2000 · 2025-02-21T06:08:18Z

plat/ti/k3/board/am62l/lpm/rtc.c

+	mmio_write_32(K3_RTC_BASE + RTC_GENRAL_CTL, ctrl);
+
+	/* Wait for read, write to finish */
+	while ((mmio_read_32(K3_RTC_BASE + RTC_SYNCPEND) & BIT(0)) != 0U) {


add some timeout mechanism with ERROR print

DhruvaG2000 · 2025-02-21T06:08:33Z

plat/ti/k3/board/am62l/lpm/rtc.c

+
+	/* flush write; this causes a "write error" but removes SW_OFF condition */
+	while ((mmio_read_32(K3_RTC_BASE + RTC_SYNCPEND) & BIT(0)) != 0U) {
+	}


DhruvaG2000 · 2025-02-21T06:08:52Z

plat/ti/k3/board/am62l/lpm/rtc.c

+
+	/* resync of lock status takes a 32k clock cycle because of O32K_OSC_DEP_EN */
+	while ((mmio_read_32(K3_RTC_BASE + RTC_SYNCPEND) & BIT(0)) != 0U) {
+	}


timeout mechanism please.

DhruvaG2000 · 2025-02-21T06:09:02Z

plat/ti/k3/board/am62l/lpm/rtc.c

+	while ((mmio_read_32(K3_RTC_BASE + RTC_SYNCPEND) & BIT(2)) == 0U) {
+	}
+	while ((mmio_read_32(K3_RTC_BASE + RTC_SYNCPEND) & BIT(2)) != 0U) {
+	}


same here and above

DhruvaG2000 · 2025-02-21T06:10:13Z

plat/ti/k3/common/drivers/lpddr4/k3-ddrss.c

+	mmio_write_32((WKUP_CTRL_MMR_SEC_4_BASE + DDR32SS_PMCTRL), 0x0);
+	mmio_write_32((WKUP_CTRL_MMR_SEC_4_BASE + DDR32SS_PMCTRL), (0x1U << 31));
+	while ((mmio_read_32((WKUP_CTRL_MMR_SEC_4_BASE + DDR32SS_PMCTRL)) & 0x80000000) ==  0x0) {
+	}


add timeout

DhruvaG2000 · 2025-02-21T06:11:34Z

plat/ti/k3/common/k3_bl1_setup.c

+		a53_tifs_msg_obj.req.hdr.seq = 0x12;
+		a53_tifs_msg_obj.req.hdr.type = 0x0308;
+		a53_tifs_msg_obj.req.ctx_lo = 0x90000000U;
+		a53_tifs_msg_obj.req.ctx_hi = 0x00000000U;


All these assignments need to move into a ti_sci_setup_one_xfer kind of a function/ API. Please see if it can be done

manorit2001 · 2025-02-21T08:16:26Z

plat/ti/k3/common/plat.ld.S

@@ -0,0 +1,48 @@
+/*


Is this file used somewhere else? I see it being added in common folder instead of am62l specific folders..

manorit2001 · 2025-02-21T08:22:38Z

plat/ti/k3/include/platform_def.h

@@ -61,7 +61,7 @@
 * defined as default for our platform.
 */
 #define BL31_BASE	UL(0x00000000) /* PIE remapped on fly */
-#define BL31_SIZE	UL(0x00037000) /* For AM62L: Allow upto 225280 bytes */
+#define BL31_SIZE	UL(0x00050000) /* For AM62L: Allow up to 327680 bytes */


I don't think we have this much allocated for our jacinto devices, I think we only reserve 128kb and the rest of the sram is used by c7x firmware and other things... Wondering if it makes sense to split this for am62l as well.

DhruvaG2000 requested review from r-vignesh, DhruvaG2000, h-nagalla and manorit2001 February 18, 2025 11:12

ti-sebin force-pushed the am62l_lpm branch from 7272037 to b123739 Compare February 19, 2025 04:37

DhruvaG2000 requested changes Feb 19, 2025

View reviewed changes

ti-sebin force-pushed the am62l_lpm branch from b123739 to 3a1f845 Compare February 20, 2025 10:42

ti-sebin requested a review from DhruvaG2000 February 20, 2025 11:35

ti-sebin force-pushed the am62l_lpm branch from 7a316a7 to 1575968 Compare February 20, 2025 12:20

DhruvaG2000 requested changes Feb 20, 2025

View reviewed changes

ti-sebin force-pushed the am62l_lpm branch from 1575968 to d403930 Compare February 20, 2025 13:23

ti-sebin requested a review from DhruvaG2000 February 20, 2025 13:26

ti-sebin force-pushed the am62l_lpm branch from d403930 to 8acd29a Compare February 20, 2025 13:43

ti-sebin force-pushed the am62l_lpm branch from 8acd29a to 145d5e6 Compare February 21, 2025 04:36

ti-sebin added 3 commits February 21, 2025 10:48

PENDING: feat(ti): lpm: Add LPM entry and exit sequence

82eea1e

Add PSCI handlers for LPM entry and exit. In the handlers call the low level LPM sequence handlers. Change-Id: I0bf32ddb9751866fcfa04c75e7a40d8c5e79c2d7 Signed-off-by: Sebin Francis <[email protected]>

ti-sebin force-pushed the am62l_lpm branch from 145d5e6 to 82eea1e Compare February 21, 2025 05:18

DhruvaG2000 requested changes Feb 21, 2025

View reviewed changes

manorit2001 reviewed Feb 21, 2025

View reviewed changes

		@@ -1789,3 +1789,44 @@ int ti_sci_lpm_get_next_sys_mode(uint8_t *next_mode)

		return 0;
		}

Am62l lpm #8

Are you sure you want to change the base?

Am62l lpm #8

Conversation

ti-sebin commented Feb 18, 2025

DhruvaG2000 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment