Re: [PATCH v3 1/1] DDR training: enabled DDR training for Shikra SPL

[Tom Rini]

On Tue, Nov 03, 2026 at 01:13:15PM +0530, KR Sowmiya wrote:

> Add support for:
> 
> Reading DDR training data from eMMC during Shikra boot flow and
> sending it to QcLIB via interface table.
> 
> Saving DDR training data in eMMC when QcLIB performs DDR training
> and sends the data via interface table.
> 
> Signed-off-by: KR Sowmiya <kr.sowm...@oss.qualcomm.com>
> ---
>  arch/arm/mach-snapdragon/Kconfig              |  67 +++++++++

This part looks good now, thanks.

>  arch/arm/mach-snapdragon/board_spl.c          | 128 ++++++++++++++++++

It feels like every patch series is touching board_spl.c and adding to
it, and it's going to end up over 1000 lines. Can someone that has a
good handle on how this will look once all the current SoCs are
supported step in and suggest how to split things up into multiple
files, and then we rework these series?

[snip]
> diff --git a/arch/arm/mach-snapdragon/board_spl.c 
> b/arch/arm/mach-snapdragon/board_spl.c
> index 31a06b1e002..62db86fe628 100644
> --- a/arch/arm/mach-snapdragon/board_spl.c
> +++ b/arch/arm/mach-snapdragon/board_spl.c
> @@ -77,6 +77,134 @@ int qcom_spl_get_boot_blk_desc(struct blk_desc **descp)
>       return 0;
>  }
>  
> +#if (IS_ENABLED(CONFIG_QCOM_DDR_TRAINING_SUPPORTED))

Extra parens aren't needed. In fact...

[snip]
> @@ -383,7 +384,11 @@ int qcom_spl_soc_pre_qclib_routine(void)
>  {
>       qcom_spl_soc_shrm_reset();
>  
> +     if (IS_ENABLED(CONFIG_QCOM_DDR_TRAINING_SUPPORTED))
> +             return qcom_spl_ddr_training_restore();
> +
>       return 0;
> +
>  }
>  
>  void qcom_spl_soc_rpm_reset(void)
> @@ -417,6 +422,22 @@ int qcom_spl_soc_post_qclib_routine(struct 
> interface_table *table)
>               return ret;
>  
>       qclib_set_ddr_sr_exit_address(entry.address);
> +
> +     if (IS_ENABLED(CONFIG_QCOM_DDR_TRAINING_SUPPORTED)) {
> +             ret = qcom_spl_get_iftbl_entry_by_name(table, 
> SHIKRA_DDR_TRAINING_DATA,
> +                                                    &entry);
> +             if (ret)
> +                     return ret;
> +
> +             if (entry.attributes & QCLIB_ATTR_SAVE_TO_STORAGE) {
> +                     ret = qcom_spl_ddr_training_save(entry.address, 
> entry.size);
> +                     if (ret) {
> +                             pr_err("DDR training: save failed (%d)\n", ret);
> +                             return ret;
> +                     }
> +             }
> +     }
> +
>       return 0;
>  }

We're using 'if (IS_ENABLED(...))' so that the C compiler / linker and
not the preprocessor discard the code for us, so they shouldn't be
needed at all.

-- 
Tom

signature.asc
Description: PGP signature