[PATCH v2 1/5] imx9: Add support for saving DDR training data to NVM

[Marek Vasut]

On 3/17/26 1:36 PM, Simona Toaca wrote:

Hi,

>>> @@ -100,6 +100,56 @@ struct dram_timing_info {
>>>    extern struct dram_timing_info dram_timing;
>>> +#if IS_ENABLED(CONFIG_IMX95) || IS_ENABLED(CONFIG_IMX94) /* CONFIG_IMX95 || CONFIG_IMX94 */
>>> +#if IS_ENABLED(CONFIG_IMX_SNPS_DDR_PHY_QB_GEN)
>>
>> These are only macros and structure definitions, do they need to be
>> conditionally compiled ?
>>
> 
> I agree that this might be redundant, especially since IMX_SNPS_DDR_PHY_QB_GEN
> depends on IMX94 || IMX95. I would leave the IMX_SNPS_DDR_PHY_QB_GEN
> ifdef so that it is clear in which context this structure is used,
> if it is also ok with you. If not, I will remove it.

Remove it, it is superfluous. If you need to denote context, add code 
comment.

>>> +/* Quick Boot related */
>>> +#define DDRPHY_QB_CSR_SIZE	5168
>>> +#define DDRPHY_QB_ACSM_SIZE	4 * 1024
>>
>> (4 * 1024) to avoid side effects
>>
>>> +#define DDRPHY_QB_MSB_SIZE	0x200
>>> +#define DDRPHY_QB_PSTATES	0
>>> +#define DDRPHY_QB_PST_SIZE	DDRPHY_QB_PSTATES * 4 * 1024
>>
>> Add ( )
> 
> Should I add () to all the numbers so it is consistent, or
> just to the macros that contain computations?

The ones which contain more than plain integer and can have side effect 
if used in the wrong place.

> [ ... ]
> 
>>> +static int do_qb_mmc(int dev, bool save, bool is_bootdev)
>>> +{
>>> +	struct mmc *mmc;
>>> +	int ret = 0, mmc_dev;
>>> +	bool has_hw_part;
>>> +	u8 orig_part, part;
>>> +	u32 offset;
>>> +	void *buf;
>>> +
>>> +	mmc_dev = mmc_get_device_index(dev);
>>> +	if (mmc_dev < 0)
>>> +		return mmc_dev;
>>> +
>>> +	ret = mmc_find_device(&mmc, mmc_dev);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (!mmc->has_init)
>>> +		ret = mmc_init(mmc);
>>
>> mmc_init() already handles the mmc->has_init conditional, drop it.
>>
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	has_hw_part = !IS_SD(mmc) && mmc->part_config != MMCPART_NOAVAILABLE;
>>> +
>>> +	if (has_hw_part) {
>>> +		orig_part = mmc_get_blk_desc(mmc)->hwpart;
>>> +		part = EXT_CSD_EXTRACT_BOOT_PART(mmc->part_config);
>>> +
>>> +		/* Select the partition */
>>> +		ret = mmc_switch_part(mmc, part);
>>> +		if (ret)
>>> +			return ret;
>>> +	}
>>> +
>>> +	ret = get_qbdata_offset(mmc, MMC_DEV, &offset, is_bootdev);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	if (save) {
>>> +		/* QB data is stored in DDR -> can use it as buf */
>>> +		buf = (void *)CONFIG_SAVED_QB_STATE_BASE;
>>> +		ret = blk_dwrite(mmc_get_blk_desc(mmc),
>>> +				 offset / mmc->write_bl_len,
>>> +				 QB_STATE_LOAD_SIZE / mmc->write_bl_len,
>>> +				 (const void *)buf);
>>> +	} else {
>>> +		/* erase */
>>> +		ret = blk_derase(mmc_get_blk_desc(mmc),
>>> +				 offset / mmc->write_bl_len,
>>> +				 QB_STATE_LOAD_SIZE / mmc->write_bl_len);
>>> +	}
>>> +
>>> +	ret = (ret > 0) ? 0 : -1;
>>
>> Use CMD_RET_SUCCESS and co.
>>
> 
> I am using these enum values in cmd_qb. However, here I would have
> do this 'if' at every return statement, since the other methods
> return 0 or an errno code. Should I still change this?

If this function does not implement a command, then it should be renamed 
to something less confusing.

>>> +	/* Return to original partition */
>>> +	if (has_hw_part)
>>> +		ret |= mmc_switch_part(mmc, orig_part);
> 
> [ ... ]
> 
>>> diff --git a/arch/arm/mach-imx/imx9/scmi/soc.c b/arch/arm/mach-imx/imx9/scmi/soc.c
>>> index 17269ddd2fc..eb9bfe19a69 100644
>>> --- a/arch/arm/mach-imx/imx9/scmi/soc.c
>>> +++ b/arch/arm/mach-imx/imx9/scmi/soc.c
>>> @@ -281,6 +281,15 @@ static struct mm_region imx9_mem_map[] = {
>>>    			 PTE_BLOCK_NON_SHARE |
>>>    			 PTE_BLOCK_PXN | PTE_BLOCK_UXN
>>>    	}, {
>>> +#if IS_ENABLED(CONFIG_IMX_SNPS_DDR_PHY_QB_GEN)
>>> +		/* QB data */
>>
>> Is the ifdeffery necessary ?
>>
>> Where are the QB data located in this case, DRAM or SRAM ?
>>
> 
> It is necessary because CONFIG_SAVED_QB_STATE_BASE is only
> defined when CONFIG_IMX_SNPS_DDR_PHY_QB_GEN is enabled. So it
> throws a compilation error without this. And this is DRAM.

Are there going to be any users that will actually want ot disable this 
functionality ? Why would they do that ?

>>> +		.virt = CONFIG_SAVED_QB_STATE_BASE,
>>> +		.phys = CONFIG_SAVED_QB_STATE_BASE,
>>> +		.size = 0x200000UL,	/* 2M */
>>> +		.attrs = PTE_BLOCK_MEMTYPE(MT_NORMAL) |
>>> +			 PTE_BLOCK_OUTER_SHARE
>>> +	}, {

[...]