Re: [PATCH v5] mmc: sdhci_am654: Add SDHCI_QUIRK2_SUPPRESS_V1P8_ENA quirk to am62 compatible

[Adrian Hunter <adrian.hunter@intel.com>]

On 14/05/2025 03:25, Judith Mendez wrote:
> Add a new struct for platform data for the ti,am62-sdhci compatible to
> apply additional quirks, namely "SDHCI_QUIRK2_SUPPRESS_V1P8_ENA", to
> host controllers with am62 compatible.

Could add:

Note, the fix was originally introduced by commit 941a7abd4666
("mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch") but was
found to be applied too broadly and had to be reverted.

> 
> This fixes MMC init failures seen across am62x boards.
> 
> Fixes: ac5a41b472b4 ("Revert "mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch"")
> Fixes: 941a7abd4666 ("mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch")
> Cc: stable@vger.kernel.org
> Suggested-by: Nishanth Menon <nm@ti.com>
> Signed-off-by: Judith Mendez <jm@ti.com>

Minor cosmetic comments, otherwise:

Acked-by: Adrian Hunter <adrian.hunter@intel.com>

> ---
> Changes since v4:
> - Drop support for quirk in sdhci.c
> - Add quirk member to driver data struct that shall be per compatible quirks
> 
> Tested on am62x SK, am64x EVM and am64x SK. Tested-bys are welcome
> 
> Link to v4:
> https://lore.kernel.org/linux-mmc/20250424180036.1541568-1-jm@ti.com/
> Link to v3 RESEND:
> https://lore.kernel.org/linux-mmc/8678d284-db12-451a-b789-2b75f9932f9f@ti.com
> Link to v2:
> https://lore.kernel.org/linux-mmc/20250417182652.3521104-1-jm@ti.com/
> Link to v1:
> https://lore.kernel.org/linux-mmc/20250407222702.2199047-1-jm@ti.com/
> ---
>  drivers/mmc/host/sdhci_am654.c | 41 +++++++++++++++++++++++++++++++++-
>  1 file changed, 40 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index f75c31815ab0..c2bea228a67a 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -155,6 +155,7 @@ struct sdhci_am654_data {
>  	u32 tuning_loop;
>  
>  #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
> +#define SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA BIT(1)
>  };
>  
>  struct window {
> @@ -166,6 +167,7 @@ struct window {
>  struct sdhci_am654_driver_data {
>  	const struct sdhci_pltfm_data *pdata;
>  	u32 flags;
> +	u32 quirks;
>  #define IOMUX_PRESENT	(1 << 0)
>  #define FREQSEL_2_BIT	(1 << 1)
>  #define STRBSEL_4_BIT	(1 << 2)
> @@ -356,6 +358,29 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
>  	sdhci_set_clock(host, clock);
>  }
>  
> +static int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios)
> +{
> +	struct sdhci_host *host = mmc_priv(mmc);
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
> +	int ret;
> +
> +	if ((sdhci_am654->quirks & SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) &&
> +	    ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
> +		if (!IS_ERR(mmc->supply.vqmmc)) {
> +			ret = mmc_regulator_set_vqmmc(mmc, ios);
> +			if (ret < 0) {
> +				pr_err("%s: Switching to 1.8V signalling voltage failed,\n",
> +				       mmc_hostname(mmc));
> +				return -EIO;
> +			}
> +		}
> +		return 0;
> +	}
> +
> +	return sdhci_start_signal_voltage_switch(mmc, ios);
> +}
> +
>  static u8 sdhci_am654_write_power_on(struct sdhci_host *host, u8 val, int reg)
>  {
>  	writeb(val, host->ioaddr + reg);
> @@ -650,6 +675,18 @@ static const struct sdhci_am654_driver_data sdhci_j721e_4bit_drvdata = {
>  	.flags = IOMUX_PRESENT,
>  };
>  
> +static const struct sdhci_pltfm_data sdhci_am62_4bit_pdata = {
> +	.ops = &sdhci_j721e_4bit_ops,
> +	.quirks = SDHCI_QUIRK_MULTIBLOCK_READ_ACMD12,
> +	.quirks2 = SDHCI_QUIRK2_PRESET_VALUE_BROKEN,
> +};

sdhci_am62_4bit_pdata is the same as sdhci_j721e_4bit_pdata

> +
> +static const struct sdhci_am654_driver_data sdhci_am62_4bit_drvdata = {
> +	.pdata = &sdhci_am62_4bit_pdata,
> +	.flags = IOMUX_PRESENT,
> +	.quirks = SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA,
> +};
> +
>  static const struct soc_device_attribute sdhci_am654_devices[] = {
>  	{ .family = "AM65X",
>  	  .revision = "SR1.0",
> @@ -872,7 +909,7 @@ static const struct of_device_id sdhci_am654_of_match[] = {
>  	},
>  	{
>  		.compatible = "ti,am62-sdhci",
> -		.data = &sdhci_j721e_4bit_drvdata,
> +		.data = &sdhci_am62_4bit_drvdata,
>  	},
>  	{ /* sentinel */ }
>  };
> @@ -906,6 +943,7 @@ static int sdhci_am654_probe(struct platform_device *pdev)
>  	pltfm_host = sdhci_priv(host);
>  	sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
>  	sdhci_am654->flags = drvdata->flags;
> +	sdhci_am654->quirks = drvdata->quirks;
>  
>  	clk_xin = devm_clk_get(dev, "clk_xin");
>  	if (IS_ERR(clk_xin)) {
> @@ -940,6 +978,7 @@ static int sdhci_am654_probe(struct platform_device *pdev)
>  		goto err_pltfm_free;
>  	}
>  
> +	host->mmc_host_ops.start_signal_voltage_switch = sdhci_am654_start_signal_voltage_switch;
>  	host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning;
>  
>  	pm_runtime_get_noresume(dev);
> 
> base-commit: aa94665adc28f3fdc3de2979ac1e98bae961d6ca