Re: [PATCH v2] mmc: dw_mmc-pltfm: Fix usage of phase_map for SoCFPGA

["Brandt, Oliver - Lenze" <Oliver.Brandt@lenze.com>]

On Thu, 2026-11-12 at 09:45 +0000, Brandt, Oliver - Lenze wrote:
> With a kernel 7.3.3 we saw the following on a SoCFPGA (Cyclone 5) based
> board during initialization when using a high-speed SD-card:
> 
>   dw_mmc ff704000.mmc: DW MMC controller at irq 53,32 bit host data width,1024 deep fifo
>   dw_mmc ff704000.mmc: Got CD GPIO
>   dw_mmc ff704000.mmc: Got WP GPIO
>   mmc_host mmc0: Bus speed = 50000000Hz (req 400000Hz, actual 396825HZ div = 63)
>   mmc_host mmc0: Bus speed = 50000000Hz (req 50000000Hz, actual 50000000HZ div = 0)
>   mmc0: error -110 whilst initialising SD card
>   mmc_host mmc0: Bus speed = 50000000Hz (req 300000Hz, actual 297619HZ div = 84)
>   mmc_host mmc0: Bus speed = 50000000Hz (req 50000000Hz, actual 50000000HZ div = 0)
>   mmc0: error -110 whilst initialising SD card
>   mmc_host mmc0: Bus speed = 50000000Hz (req 200000Hz, actual 200000HZ div = 125)
>   mmc_host mmc0: Bus speed = 50000000Hz (req 50000000Hz, actual 50000000HZ div = 0)
>   mmc0: error -110 whilst initialising SD card
>   mmc_host mmc0: Bus speed = 50000000Hz (req 100000Hz, actual 100000HZ div = 250)
>   mmc_host mmc0: Bus speed = 50000000Hz (req 50000000Hz, actual 50000000HZ div = 0)
>   mmc0: error -110 whilst initialising SD card
> 
> The SD-card was not usable in the following.
> 
> Reason: Since commit 3f1628baa51e ("mmc: dw_mmc-pltfm: Use phase_map for
> SoCFPGA clock phase configuration") host->phase_map is used in order to
> get the phase settings for MMC_TIMING_SD_HS. This is done in drv_data->init
> (dw_mci_socfpga_priv_init()) which is called before dw_mci_init_host(). As
> host->phase_map will be initialized only during dw_mci_init_host(), we'll
> call dw_mci_socfpga_priv_init() without the needed data.
> 
> Do it like other users of dw_mci_platfom and use drv_data->set_ios.
> 
> Fixes: 3f1628baa51e ("mmc: dw_mmc-pltfm: Use phase_map for SoCFPGA clock phase configuration")
> Cc: stable@vger.kernel.org
> Signed-off-by: Oliver Brandt <oliver.brandt@lenze.com>
> ---
> Changes in v2:
> - Add 'return' in case of missing regmap (Sashiko AI).
> - Avoid repeated execution (Sashiko AI).
> - Write register 'hs_timing' even if MMC_TIMING_SD_HS is not requested
>   (Sashiko AI).
> - Link to v1: https://lore.kernel.org/linux-mmc/2a03a222c4a9d330ebc7157de09ec33f72875f28.camel@lenze.com/T/#u
> 
>  drivers/mmc/host/dw_mmc-pltfm.c | 14 +++++++-------
>  1 file changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/mmc/host/dw_mmc-pltfm.c b/drivers/mmc/host/dw_mmc-pltfm.c
> index cf38bb118dc2c..b51574a0a1fe1 100644
> --- a/drivers/mmc/host/dw_mmc-pltfm.c
> +++ b/drivers/mmc/host/dw_mmc-pltfm.c
> @@ -56,22 +56,23 @@ int dw_mci_pltfm_register(struct platform_device *pdev,
>  }
>  EXPORT_SYMBOL_GPL(dw_mci_pltfm_register);
>  
> -static int dw_mci_socfpga_priv_init(struct dw_mci *host)
> +static void dw_mci_socfpga_priv_set_ios(struct dw_mci *host, struct mmc_ios *ios)
>  {
>  	struct device_node *np = host->dev->of_node;
>  	struct mmc_clk_phase phase;
>  	struct regmap *sys_mgr_base_addr;
>  	u32 reg_offset, reg_shift;
>  	int hs_timing;
> +	static int phase_already_set;
>  
>  	phase = host->phase_map.phase[MMC_TIMING_SD_HS];
> -	if (!phase.valid)
> -		return 0;
> +	if (!phase.valid || phase_already_set)
> +		return;
>  
>  	sys_mgr_base_addr = altr_sysmgr_regmap_lookup_by_phandle(np, "altr,sysmgr-syscon");
>  	if (IS_ERR(sys_mgr_base_addr)) {
>  		dev_warn(host->dev, "clk-phase-sd-hs was specified, but failed to find altr,sys-mgr regmap!\n");
> -		return 0;
> +		return;
>  	}
>  
>  	of_property_read_u32_index(np, "altr,sysmgr-syscon", 1, &reg_offset);
> @@ -82,12 +83,11 @@ static int dw_mci_socfpga_priv_init(struct dw_mci *host)
>  
>  	hs_timing = SYSMGR_SDMMC_CTRL_SET(phase.in_deg, phase.out_deg, reg_shift);
>  	regmap_write(sys_mgr_base_addr, reg_offset, hs_timing);
> -
> -	return 0;
> +	phase_already_set = 1;
>  }
>  
>  static const struct dw_mci_drv_data socfpga_drv_data = {
> -	.init		= dw_mci_socfpga_priv_init,
> +	.set_ios	= dw_mci_socfpga_priv_set_ios,
>  };
>  
>  static const struct of_device_id dw_mci_pltfm_match[] = {

Sashiko pointed out that using a static variable is "not a good
idea". Even if, at the moment, we don't have any in-tree DTS with
more than one instance of the controller, a cleaner solution
seems to be needed.

I've already prepared a new (hopefully better) patch. This one
should be ignored. Sorry for the noise.

Regards,
Oliver