Re: [PATCH 8/8] mmc: sdhci-cadence6: Add DLL master control and improve tuning reliability

[Tanmay Kathpalia <tanmay.kathpalia@altera.com>]

On 11/18/2025 11:54 AM, Peng Fan wrote:
> On Mon, Nov 10, 2025 at 09:37:37AM -0800, Tanmay Kathpalia wrote:
>> - Add support for configuring the PHY DLL master control register for all
>>   SD/eMMC timing modes (DS, HS, SDR, DDR, HS200, HS400) by extending the
>>   PHY configuration arrays and writing the value during PHY adjustment.
>> - Fix tuning reliability by toggling the DLL reset before and after
>>   updating the PHY_DLL_SLAVE_CTRL_REG_ADDR register.
>>
>> Signed-off-by: Tanmay Kathpalia <tanmay.kathpalia@altera.com>
>> Reviewed-by: Balsundar Ponnusamy <balsundar.ponnusamy@altera.com>
>> ---
>> drivers/mmc/sdhci-cadence6.c | 21 ++++++++++++++++++++-
>> 1 file changed, 20 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/mmc/sdhci-cadence6.c b/drivers/mmc/sdhci-cadence6.c
>> index d9467293807..91a245aa490 100644
>> --- a/drivers/mmc/sdhci-cadence6.c
>> +++ b/drivers/mmc/sdhci-cadence6.c
>> @@ -58,7 +58,7 @@
>> #define PHY_DLL_SLAVE_CTRL_REG_READ_DQS_CMD_DELAY	GENMASK(31, 24)
>> #define PHY_DLL_SLAVE_CTRL_REG_READ_DQS_DELAY		GENMASK(7, 0)
>>
>> -#define SDHCI_CDNS6_PHY_CFG_NUM		4
>> +#define SDHCI_CDNS6_PHY_CFG_NUM		5
>> #define SDHCI_CDNS6_CTRL_CFG_NUM	4
>>
>> struct sdhci_cdns6_phy_cfg {
>> @@ -76,6 +76,7 @@ static struct sdhci_cdns6_phy_cfg sd_ds_phy_cfgs[] = {
>> 	{ "cdns,phy-gate-lpbk-ctrl-delay-sd-ds", 0x01A00040, },
>> 	{ "cdns,phy-dll-slave-ctrl-sd-ds", 0x00000000, },
>> 	{ "cdns,phy-dq-timing-delay-sd-ds", 0x00000001, },
>> +	{ "cdns,phy-dll-master-ctrl-sd-ds", 0x00800004, },
>> };
>>
>> static struct sdhci_cdns6_phy_cfg sd_hs_phy_cfgs[] = {
>> @@ -83,6 +84,7 @@ static struct sdhci_cdns6_phy_cfg sd_hs_phy_cfgs[] = {
>> 	{ "cdns,phy-gate-lpbk-ctrl-delay-sd-hs", 0x01A00040, },
>> 	{ "cdns,phy-dll-slave-ctrl-sd-hs", 0x00000000, },
>> 	{ "cdns,phy-dq-timing-delay-sd-hs", 0x00000001, },
>> +	{ "cdns,phy-dll-master-ctrl-sd-hs", 0x00800004, },
>> };
>>
>> static struct sdhci_cdns6_phy_cfg emmc_sdr_phy_cfgs[] = {
>> @@ -90,6 +92,7 @@ static struct sdhci_cdns6_phy_cfg emmc_sdr_phy_cfgs[] = {
>> 	{ "cdns,phy-gate-lpbk-ctrl-delay-emmc-sdr", 0x01A00040, },
>> 	{ "cdns,phy-dll-slave-ctrl-emmc-sdr", 0x00000000, },
>> 	{ "cdns,phy-dq-timing-delay-emmc-sdr", 0x00000001, },
>> +	{ "cdns,phy-dll-master-ctrl-emmc-sdr", 0x00800004, },
>> };
>>
>> static struct sdhci_cdns6_phy_cfg emmc_ddr_phy_cfgs[] = {
>> @@ -97,6 +100,7 @@ static struct sdhci_cdns6_phy_cfg emmc_ddr_phy_cfgs[] = {
>> 	{ "cdns,phy-gate-lpbk-ctrl-delay-emmc-ddr", 0x01A00040, },
>> 	{ "cdns,phy-dll-slave-ctrl-emmc-ddr", 0x00000000, },
>> 	{ "cdns,phy-dq-timing-delay-emmc-ddr", 0x10000001, },
>> +	{ "cdns,phy-dll-master-ctrl-emmc-ddr", 0x00800004, },
>> };
>>
>> static struct sdhci_cdns6_phy_cfg emmc_hs200_phy_cfgs[] = {
>> @@ -104,6 +108,7 @@ static struct sdhci_cdns6_phy_cfg emmc_hs200_phy_cfgs[] = {
>> 	{ "cdns,phy-gate-lpbk-ctrl-delay-emmc-hs200", 0x01A00040, },
>> 	{ "cdns,phy-dll-slave-ctrl-emmc-hs200", 0x00DADA00, },
>> 	{ "cdns,phy-dq-timing-delay-emmc-hs200", 0x00000001, },
>> +	{ "cdns,phy-dll-master-ctrl-emmc-hs200", 0x00000004, },
>> };
>>
>> static struct sdhci_cdns6_phy_cfg emmc_hs400_phy_cfgs[] = {
>> @@ -111,6 +116,7 @@ static struct sdhci_cdns6_phy_cfg emmc_hs400_phy_cfgs[] = {
>> 	{ "cdns,phy-gate-lpbk-ctrl-delay-emmc-hs400", 0x01A00040, },
>> 	{ "cdns,phy-dll-slave-ctrl-emmc-hs400", 0x00DAD800, },
>> 	{ "cdns,phy-dq-timing-delay-emmc-hs400", 0x00000001, },
>> +	{ "cdns,phy-dll-master-ctrl-emmc-hs400", 0x00000004, },
>> };
>>
>> static struct sdhci_cdns6_ctrl_cfg sd_ds_ctrl_cfgs[] = {
>> @@ -252,6 +258,7 @@ int sdhci_cdns6_phy_adj(struct udevice *dev, struct sdhci_cdns_plat *plat, u32 m
>>
>> 	sdhci_cdns6_write_phy_reg(plat, PHY_DQS_TIMING_REG_ADDR, sdhci_cdns6_phy_cfgs[0].val);
>> 	sdhci_cdns6_write_phy_reg(plat, PHY_GATE_LPBK_CTRL_REG_ADDR, sdhci_cdns6_phy_cfgs[1].val);
>> +	sdhci_cdns6_write_phy_reg(plat, PHY_DLL_MASTER_CTRL_REG_ADDR, sdhci_cdns6_phy_cfgs[4].val);
>> 	sdhci_cdns6_write_phy_reg(plat, PHY_DLL_SLAVE_CTRL_REG_ADDR, sdhci_cdns6_phy_cfgs[2].val);
>>
>> 	/* Switch Off the DLL Reset */
>> @@ -296,6 +303,7 @@ int sdhci_cdns6_phy_init(struct udevice *dev, struct sdhci_cdns_plat *plat)
>> int sdhci_cdns6_set_tune_val(struct sdhci_cdns_plat *plat, unsigned int val)
>> {
>> 	u32 tmp, tuneval;
>> +	int ret;
>>
>> 	tuneval = (val * 256) / SDHCI_CDNS_MAX_TUNING_LOOP;
>>
>> @@ -304,7 +312,18 @@ int sdhci_cdns6_set_tune_val(struct sdhci_cdns_plat *plat, unsigned int val)
>> 		 PHY_DLL_SLAVE_CTRL_REG_READ_DQS_DELAY);
>> 	tmp |= FIELD_PREP(PHY_DLL_SLAVE_CTRL_REG_READ_DQS_CMD_DELAY, tuneval) |
>> 		FIELD_PREP(PHY_DLL_SLAVE_CTRL_REG_READ_DQS_DELAY, tuneval);
>> +
>> +	/* Switch On the DLL Reset */
>> +	sdhci_cdns6_reset_phy_dll(plat, true);
> 
> There is no err return check, but
> 
>> +
>> 	sdhci_cdns6_write_phy_reg(plat, PHY_DLL_SLAVE_CTRL_REG_ADDR, tmp);
>>
>> +	/* Switch Off the DLL Reset */
>> +	ret = sdhci_cdns6_reset_phy_dll(plat, false);
> 
> there is err check here. Should these be aligned or the usage is intentional?
> 

The difference is intentional.
sdhci_cdns6_reset_phy_dll always returns 0 when the second argument is 
true (i.e., when switching the DLL reset On).
When switching the DLL reset Off (false), the function waits for the PHY 
initialization completion bit to be set within the defined time period, 
so a return value check is necessary to catch any failure during this step.

> Regards
> Peng
> 
>> +	if (ret) {
>> +		printf("sdhci_cdns6_reset_phy is not completed\n");
>> +		return ret;
>> +	}
>> +
>> 	return 0;
>> }
>> -- 
>> 2.43.7
>>
>>