[PATCH 1/2] ddr: imx8m: write fsp_table entry automatically

[PATCH 1/2] ddr: imx8m: write fsp_table entry automatically

[Marco Felsch]

Currently the fsp_table must be set manually within the 'struct
dram_timing_info'. Since the 'struct fsp_msg' already has all
information needed for the fsp_table we can use it to set it
automatically. This approach is less error-prone and avoids information
duplication.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/ddr/imx8m/ddrphy_train.c |  3 +++
 drivers/ddr/imx8m/helper.c       | 19 +++++++++++++++++++
 include/soc/imx8m/ddr.h          |  2 ++
 3 files changed, 24 insertions(+)

diff --git a/drivers/ddr/imx8m/ddrphy_train.c b/drivers/ddr/imx8m/ddrphy_train.c
index e9d35afdfb..09f1e77295 100644
--- a/drivers/ddr/imx8m/ddrphy_train.c
+++ b/drivers/ddr/imx8m/ddrphy_train.c
@@ -165,6 +165,9 @@ int ddr_cfg_phy(struct dram_timing_info *dram_timing, unsigned type)
 
 		dwc_ddrphy_apb_wr(0xd0000, 0x1);
 
+		/* Training done, add entry to fsp_table */
+		dram_write_fsp_table_entry(dram_timing, fsp_msg->drate);
+
 		fsp_msg++;
 	}
 
diff --git a/drivers/ddr/imx8m/helper.c b/drivers/ddr/imx8m/helper.c
index 98e4084958..e71401c1b2 100644
--- a/drivers/ddr/imx8m/helper.c
+++ b/drivers/ddr/imx8m/helper.c
@@ -18,6 +18,25 @@
 #define DMEM_OFFSET_ADDR 0x00054000
 #define DDR_TRAIN_CODE_BASE_ADDR IP2APB_DDRPHY_IPS_BASE_ADDR(0)
 
+void dram_write_fsp_table_entry(struct dram_timing_info *timing_info,
+				unsigned int data_rate)
+{
+	unsigned int i;
+
+	for (i = 0; i < ARRAY_SIZE(timing_info->fsp_table); i++) {
+		unsigned int entry = timing_info->fsp_table[i];
+
+		/* Skip already existing entries */
+		if (entry == data_rate)
+			return;
+
+		if (entry == 0) {
+			timing_info->fsp_table[i] = data_rate;
+			return;
+		}
+	}
+}
+
 void ddrphy_trained_csr_save(struct dram_cfg_param *ddrphy_csr,
 			     unsigned int num)
 {
diff --git a/include/soc/imx8m/ddr.h b/include/soc/imx8m/ddr.h
index 2149ae4325..222ab8e0f4 100644
--- a/include/soc/imx8m/ddr.h
+++ b/include/soc/imx8m/ddr.h
@@ -436,6 +436,8 @@ int ddr_cfg_phy(struct dram_timing_info *timing_info, enum ddrc_type ddrc_type);
 void load_lpddr4_phy_pie(void);
 void ddrphy_trained_csr_save(struct dram_cfg_param *param, unsigned int num);
 void dram_config_save(struct dram_timing_info *info, unsigned long base);
+void dram_write_fsp_table_entry(struct dram_timing_info *timing_info,
+				unsigned int data_rate);
 
 /* utils function for ddr phy training */
 int wait_ddrphy_training_complete(void);
-- 
2.30.2

[PATCH 2/2] ARM: i.MX8M: remove fsp_table entry

[Marco Felsch]

Barebox now have the support to write the fsp_table entries
automatically based on fsp_msg.drata information. Remove the fsp_table
entry and let the common code do the rest for us.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 arch/arm/boards/mnt-reform/lpddr4-timing.c     | 1 -
 arch/arm/boards/nxp-imx8mn-evk/ddr4-timing.c   | 1 -
 arch/arm/boards/nxp-imx8mn-evk/lpddr4-timing.c | 1 -
 arch/arm/boards/nxp-imx8mp-evk/lpddr4-timing.c | 1 -
 4 files changed, 4 deletions(-)

diff --git a/arch/arm/boards/mnt-reform/lpddr4-timing.c b/arch/arm/boards/mnt-reform/lpddr4-timing.c
index 0b5853000d..38fa2c50f1 100644
--- a/arch/arm/boards/mnt-reform/lpddr4-timing.c
+++ b/arch/arm/boards/mnt-reform/lpddr4-timing.c
@@ -1008,5 +1008,4 @@ struct dram_timing_info mnt_reform_dram_timing = {
 	.fsp_msg_num = ARRAY_SIZE(mnt_reform_lpddr4_dram_fsp_msg),
 	.ddrphy_pie = mnt_reform_lpddr4_phy_pie,
 	.ddrphy_pie_num = ARRAY_SIZE(mnt_reform_lpddr4_phy_pie),
-	.fsp_table = { 3200, 667, },
 };
diff --git a/arch/arm/boards/nxp-imx8mn-evk/ddr4-timing.c b/arch/arm/boards/nxp-imx8mn-evk/ddr4-timing.c
index 131a63156e..df10bdcc9d 100644
--- a/arch/arm/boards/nxp-imx8mn-evk/ddr4-timing.c
+++ b/arch/arm/boards/nxp-imx8mn-evk/ddr4-timing.c
@@ -1050,5 +1050,4 @@ struct dram_timing_info imx8mn_evk_ddr4_timing = {
 	.ddrphy_trained_csr_num = ARRAY_SIZE(ddr_ddrphy_trained_csr),
 	.ddrphy_pie = ddr_phy_pie,
 	.ddrphy_pie_num = ARRAY_SIZE(ddr_phy_pie),
-	.fsp_table = { 2400, 1066, },
 };
diff --git a/arch/arm/boards/nxp-imx8mn-evk/lpddr4-timing.c b/arch/arm/boards/nxp-imx8mn-evk/lpddr4-timing.c
index 940b21cedb..63851c87d4 100644
--- a/arch/arm/boards/nxp-imx8mn-evk/lpddr4-timing.c
+++ b/arch/arm/boards/nxp-imx8mn-evk/lpddr4-timing.c
@@ -1586,5 +1586,4 @@ struct dram_timing_info imx8mn_evk_lpddr4_timing = {
 	.ddrphy_trained_csr_num = ARRAY_SIZE(ddr_ddrphy_trained_csr),
 	.ddrphy_pie = ddr_phy_pie,
 	.ddrphy_pie_num = ARRAY_SIZE(ddr_phy_pie),
-	.fsp_table = { 3200, 400, 100, },
 };
diff --git a/arch/arm/boards/nxp-imx8mp-evk/lpddr4-timing.c b/arch/arm/boards/nxp-imx8mp-evk/lpddr4-timing.c
index 3028bc084c..b8c6b78017 100644
--- a/arch/arm/boards/nxp-imx8mp-evk/lpddr4-timing.c
+++ b/arch/arm/boards/nxp-imx8mp-evk/lpddr4-timing.c
@@ -1844,5 +1844,4 @@ struct dram_timing_info imx8mp_evk_dram_timing = {
 	.ddrphy_trained_csr_num = ARRAY_SIZE(ddr_ddrphy_trained_csr),
 	.ddrphy_pie = ddr_phy_pie,
 	.ddrphy_pie_num = ARRAY_SIZE(ddr_phy_pie),
-	.fsp_table = { 4000, 400, 100, },
 };
-- 
2.30.2

Re: [PATCH 1/2] ddr: imx8m: write fsp_table entry automatically

[Sascha Hauer]

On Wed, Sep 21, 2022 at 07:20:51PM +0200, Marco Felsch wrote:
> Currently the fsp_table must be set manually within the 'struct
> dram_timing_info'. Since the 'struct fsp_msg' already has all
> information needed for the fsp_table we can use it to set it
> automatically. This approach is less error-prone and avoids information
> duplication.

Is this really worth it? The whole dram timing code comes directly from
the NXP DRAM tools, so the fsp_table comes back with each new board or
with a DRAM timing update.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 1/2] ddr: imx8m: write fsp_table entry automatically

[Marco Felsch]

On 22-09-22, Sascha Hauer wrote:
> On Wed, Sep 21, 2022 at 07:20:51PM +0200, Marco Felsch wrote:
> > Currently the fsp_table must be set manually within the 'struct
> > dram_timing_info'. Since the 'struct fsp_msg' already has all
> > information needed for the fsp_table we can use it to set it
> > automatically. This approach is less error-prone and avoids information
> > duplication.
> 
> Is this really worth it? The whole dram timing code comes directly from
> the NXP DRAM tools, so the fsp_table comes back with each new board or
> with a DRAM timing update.

I had a problem with the missing fsp_table for the MX8MM-EVK and a buggy
TF-A. Therefore I would say yes, since it is not guaranteed that the
fsp_table is existing.

Regards,
  Marco

Re: [PATCH 1/2] ddr: imx8m: write fsp_table entry automatically

[Sascha Hauer]

On Wed, Sep 21, 2022 at 07:20:51PM +0200, Marco Felsch wrote:
> Currently the fsp_table must be set manually within the 'struct
> dram_timing_info'. Since the 'struct fsp_msg' already has all
> information needed for the fsp_table we can use it to set it
> automatically. This approach is less error-prone and avoids information
> duplication.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/ddr/imx8m/ddrphy_train.c |  3 +++
>  drivers/ddr/imx8m/helper.c       | 19 +++++++++++++++++++
>  include/soc/imx8m/ddr.h          |  2 ++
>  3 files changed, 24 insertions(+)
> 
> diff --git a/drivers/ddr/imx8m/ddrphy_train.c b/drivers/ddr/imx8m/ddrphy_train.c
> index e9d35afdfb..09f1e77295 100644
> --- a/drivers/ddr/imx8m/ddrphy_train.c
> +++ b/drivers/ddr/imx8m/ddrphy_train.c
> @@ -165,6 +165,9 @@ int ddr_cfg_phy(struct dram_timing_info *dram_timing, unsigned type)
>  
>  		dwc_ddrphy_apb_wr(0xd0000, 0x1);
>  
> +		/* Training done, add entry to fsp_table */
> +		dram_write_fsp_table_entry(dram_timing, fsp_msg->drate);
> +
>  		fsp_msg++;
>  	}
>  
> diff --git a/drivers/ddr/imx8m/helper.c b/drivers/ddr/imx8m/helper.c
> index 98e4084958..e71401c1b2 100644
> --- a/drivers/ddr/imx8m/helper.c
> +++ b/drivers/ddr/imx8m/helper.c
> @@ -18,6 +18,25 @@
>  #define DMEM_OFFSET_ADDR 0x00054000
>  #define DDR_TRAIN_CODE_BASE_ADDR IP2APB_DDRPHY_IPS_BASE_ADDR(0)
>  
> +void dram_write_fsp_table_entry(struct dram_timing_info *timing_info,
> +				unsigned int data_rate)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(timing_info->fsp_table); i++) {
> +		unsigned int entry = timing_info->fsp_table[i];
> +
> +		/* Skip already existing entries */
> +		if (entry == data_rate)
> +			return;
> +
> +		if (entry == 0) {
> +			timing_info->fsp_table[i] = data_rate;
> +			return;
> +		}
> +	}
> +}

The input struct dram_timing_info * comes directly from the boards and
should be treated as const. If you go that route please parse the
.fsp_msg data directly in dram_config_save().

Another way could be to just check the .fsp_msg and .fsp_table for
consistency and warn about it if necessary.
In the end the input data from the boards is redundant here. We are
not going to change the input data format as that comes from the NXP
DRAM tools. Just ignoring the .fsp_table like you do here might be
confusing as well.

Sascha

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |