[PATCH] ddr: imx8m: fix broken sharing of DRAM timing with TF-A for DFS

[PATCH] ddr: imx8m: fix broken sharing of DRAM timing with TF-A for DFS

[Ahmad Fatoum]

To facilitate dynamic frequency scaling, barebox will place the DRAM
parameters it has configured into a well-known location in DRAM
for consumption by TF-A.

This means that we need to be very picky about the struct layout for the
data we write there to maintain ABI-compatibility. Unfortunately, NXP
chose for the i.MX9 to add two fields into the middle, when it would
have been possible to cleanly embed the i.MX8 DRAM timings in the
i.MX9 DRAM timing struct. This is done now, so we need to arrange
ourselves with it and ensure that the i.MX8M DRAM timings we share with
TF-A don't have glimpses of the i.MX9 future.

As templating dram_config_save using the preprocessor would be
excessively ugly, we add a fixup that turns the common dram_timing_info
format into the format expected by TF-A on the i.MX8M family of SoCs.

Fixes: e6234f907416 ("ddr: Initial i.MX9 support")
Reported-by: Alvin Šipraga <ALSI@bang-olufsen.dk>
Tested-by: Alvin Šipraga <ALSI@bang-olufsen.dk>
Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 drivers/ddr/imx/imx8m_ddr_init.c |  7 ++++
 include/soc/imx/ddr.h            | 62 +++++++++++++++++++++-----------
 2 files changed, 49 insertions(+), 20 deletions(-)

diff --git a/drivers/ddr/imx/imx8m_ddr_init.c b/drivers/ddr/imx/imx8m_ddr_init.c
index d9a5d589f27b..c16e04d27483 100644
--- a/drivers/ddr/imx/imx8m_ddr_init.c
+++ b/drivers/ddr/imx/imx8m_ddr_init.c
@@ -481,6 +481,7 @@ static void ddrphy_init_set_dfi_clk(struct dram_controller *dram, unsigned int d
 
 int imx8m_ddr_init(struct dram_controller *dram, struct dram_timing_info *dram_timing)
 {
+	struct dram_timing_info *saved_timing;
 	unsigned long src_ddrc_rcr = MX8M_SRC_DDRC_RCR_ADDR;
 	unsigned int tmp, initial_drate, target_freq;
 	int ret;
@@ -644,5 +645,11 @@ int imx8m_ddr_init(struct dram_controller *dram, struct dram_timing_info *dram_t
 	/* save the dram timing config into memory */
 	dram_config_save(dram, dram_timing, IMX8M_SAVED_DRAM_TIMING_BASE);
 
+	saved_timing = (struct dram_timing_info *)IMX8M_SAVED_DRAM_TIMING_BASE;
+
+	/* There's no fsp_cfg for i.MX8, so we must close the gap again */
+	memmove(&saved_timing->fsp_config, &saved_timing->common_config,
+		sizeof(saved_timing->common_config));
+
 	return 0;
 }
diff --git a/include/soc/imx/ddr.h b/include/soc/imx/ddr.h
index 0225ac0e033c..caf33d3e34b9 100644
--- a/include/soc/imx/ddr.h
+++ b/include/soc/imx/ddr.h
@@ -6,6 +6,8 @@
 #ifndef __SOC_IMX_DDR_H
 #define __SOC_IMX_DDR_H
 
+#include <linux/stddef.h>
+
 /* user data type */
 enum fw_type {
 	FW_1D_IMAGE,
@@ -58,27 +60,47 @@ struct dram_fsp_msg {
 	__attribute__((deprecated("board-specific data here is ignored in favor of the defaults." \
 				  " You can probably remove the array")))
 
+/*
+ * The DRAM timing values are machine-generated by an NXP tool.
+ * This is evaluated by barebox to initialize and train the DDR and
+ * is then shared with TF-A to enable dynamic frequency scaling.
+ *
+ * To minimize both duplicate code and manual fixups needed for the
+ * NXP tool output, we use a struct definition that's a superset
+ * of both of i.MX8M and i.MX9. This means that each SoC type making
+ * use of this struct needs to translate it to the appropriate format
+ * expected by TF-A.
+ *
+ * i.e: Do not change the struct definition without adjusting
+ * dram_config_save and its callers to maintain ABI-compatibility.
+ */
 struct dram_timing_info {
-	/* umctl2 config */
-	struct dram_cfg_param *ddrc_cfg;
-	unsigned int ddrc_cfg_num;
-	/* fsp config */
-	struct dram_fsp_cfg *fsp_cfg;
-	unsigned int fsp_cfg_num;
-	/* ddrphy config */
-	struct dram_cfg_param *ddrphy_cfg;
-	unsigned int ddrphy_cfg_num;
-	/* ddr fsp train info */
-	struct dram_fsp_msg *fsp_msg;
-	unsigned int fsp_msg_num;
-	/* ddr phy trained CSR */
-	struct dram_cfg_param *ddrphy_trained_csr __deprecated_dram_timing_info;
-	unsigned int ddrphy_trained_csr_num __deprecated_dram_timing_info;
-	/* ddr phy PIE */
-	struct dram_cfg_param *ddrphy_pie;
-	unsigned int ddrphy_pie_num;
-	/* initialized drate table */
-	unsigned int fsp_table[4];
+	struct_group(umctl2_config,
+		struct dram_cfg_param *ddrc_cfg;
+		unsigned int ddrc_cfg_num;
+	);
+
+	struct_group(fsp_config, /* i.MX9-only */
+		struct dram_fsp_cfg *fsp_cfg;
+		unsigned int fsp_cfg_num;
+	);
+
+	struct_group(common_config,
+		/* ddrphy config */
+		struct dram_cfg_param *ddrphy_cfg;
+		unsigned int ddrphy_cfg_num;
+		/* ddr fsp train info */
+		struct dram_fsp_msg *fsp_msg;
+		unsigned int fsp_msg_num;
+		/* ddr phy trained CSR */
+		struct dram_cfg_param *ddrphy_trained_csr __deprecated_dram_timing_info;
+		unsigned int ddrphy_trained_csr_num __deprecated_dram_timing_info;
+		/* ddr phy PIE */
+		struct dram_cfg_param *ddrphy_pie;
+		unsigned int ddrphy_pie_num;
+		/* initialized drate table */
+		unsigned int fsp_table[4];
+	);
 };
 
 struct dram_controller {
-- 
2.39.2

Re: [PATCH] ddr: imx8m: fix broken sharing of DRAM timing with TF-A for DFS

[Sascha Hauer]

On Mon, 29 Apr 2024 16:16:10 +0200, Ahmad Fatoum wrote:
> To facilitate dynamic frequency scaling, barebox will place the DRAM
> parameters it has configured into a well-known location in DRAM
> for consumption by TF-A.
> 
> This means that we need to be very picky about the struct layout for the
> data we write there to maintain ABI-compatibility. Unfortunately, NXP
> chose for the i.MX9 to add two fields into the middle, when it would
> have been possible to cleanly embed the i.MX8 DRAM timings in the
> i.MX9 DRAM timing struct. This is done now, so we need to arrange
> ourselves with it and ensure that the i.MX8M DRAM timings we share with
> TF-A don't have glimpses of the i.MX9 future.
> 
> [...]

Applied, thanks!

[1/1] ddr: imx8m: fix broken sharing of DRAM timing with TF-A for DFS
      https://git.pengutronix.de/cgit/barebox/commit/?id=ea580697c269 (link may not be stable)

Best regards,
-- 
Sascha Hauer <s.hauer@pengutronix.de>