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

[Sascha Hauer <sha@pengutronix.de>]

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 |