[PATCH v2 1/5] imx9: Add support for saving DDR training data to NVM
Simona Toaca
simona.toaca at oss.nxp.com
Tue Mar 17 13:36:26 CET 2026
Hi Marek,
On Mon, Mar 16, 2026 at 01:13:02PM +0100, Marek Vasut wrote:
> On 3/16/26 9:15 AM, Simona Toaca (OSS) wrote:
>
> > diff --git a/arch/arm/include/asm/arch-imx9/ddr.h b/arch/arm/include/asm/arch-imx9/ddr.h
> > index a8e3f7354c7..d2afbe59ffb 100644
> > --- a/arch/arm/include/asm/arch-imx9/ddr.h
> > +++ b/arch/arm/include/asm/arch-imx9/ddr.h
> > @@ -1,6 +1,6 @@
> > /* SPDX-License-Identifier: GPL-2.0+ */
> > /*
> > - * Copyright 2022 NXP
> > + * Copyright 2022-2025 NXP
>
> 2026
>
> > */
> > #ifndef __ASM_ARCH_IMX8M_DDR_H
> > @@ -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.
> > +/* 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?
[ ... ]
> > +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?
> > + /* 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.
> > + .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
> > + }, {
> > +#endif /* CONFIG_IMX_SNPS_DDR_PHY_QB_GEN */
> > /* empty entry to split table entry 5 if needed when TEEs are used */
> > 0,
> > }, {
> > diff --git a/drivers/ddr/imx/imx9/Kconfig b/drivers/ddr/imx/imx9/Kconfig
> > index 0a45340ffb6..7c244ddb5dd 100644
> > --- a/drivers/ddr/imx/imx9/Kconfig
> > +++ b/drivers/ddr/imx/imx9/Kconfig
> > @@ -29,4 +29,12 @@ config SAVED_DRAM_TIMING_BASE
> > info into memory for low power use.
> > default 0x2051C000
>
> Please fix ^ that one in separate patch, default goes after depends.
>
Yes, will do.
> > +config SAVED_QB_STATE_BASE
>
> IMX_SNPS_DDR_PHY_QB_SAVED_STATE_ADDRESS ... to make the prefix of this
> Kconfig symbol aligned with IMX_SNPS_DDR_PHY_QB_GEN below .
>
> > + hex "Define the base address for saved QuickBoot state"
> > + depends on IMX_SNPS_DDR_PHY_QB_GEN
>
> "default" goes here.
>
> > + help
> > + Once DRAM is trained, need to save the dram related timing
> > + info into memory in order to be reachable from U-Boot.
> > + default 0x8fe00000
> > +
> > endmenu
> > diff --git a/drivers/ddr/imx/phy/Kconfig b/drivers/ddr/imx/phy/Kconfig
> > index d3e589b23c4..e8d0c005689 100644
> > --- a/drivers/ddr/imx/phy/Kconfig
> > +++ b/drivers/ddr/imx/phy/Kconfig
> > @@ -2,3 +2,10 @@ config IMX_SNPS_DDR_PHY
> > bool "i.MX Snopsys DDR PHY"
> > help
> > Select the DDR PHY driver support on i.MX8M and i.MX9 SOC.
> > +
> > +config IMX_SNPS_DDR_PHY_QB_GEN
> > + bool "i.MX Synopsys DDR PHY training data saving for QuickBoot mode"
> > + depends on IMX94 || IMX95
> > + help
> > + Select the DDR PHY training data saving for
> > + QuickBoot support on i.MX9 SOC.
Thank you. Will fix these issues in the next version.
Simona
More information about the U-Boot
mailing list