[PATCH 0/2] Fix V1P8_SIGNAL_ENA and HIGH_SPEED_ENA

[PATCH 0/2] Fix V1P8_SIGNAL_ENA and HIGH_SPEED_ENA

[Judith Mendez]

For all TI devices, timing was closed For Legacy and HS modes in
half cycle timing, where data is launched on the negative edge of
clock and latched on the following positive edge of clock. The
switch to full cycle timing happens when any of HIGH_SPEED_ENA,
V1P8_SIGNAL_ENA, or UHS_MODE_SELECT is set.

Currently HIGH_SPEED_ENA is set for HS modes and violates timing
requirements for TI devices so add a .set_hs_ena callback in
sdhci_am654 driver so that HIGH_SPEED_ENA is not set for this mode.

There are eMMC boot failures seen with V1P8_SIGNAL_ENA with a
specific Kingston eMMC due to the sequencing when enumerating to
HS200 mode. Since V1P8_SIGNAL_ENA is optional for eMMC, do not
set V1P8_SIGNAL_ENA be default. This fix was previously merged in
the kernel, but was reverted due to the "heuristics for enabling
the quirk"[0]. The new implementation applies the quirk based-off of
bus width, which should not be an issue since there is no internal
LDO for MMC0 8bit wide interface and hence V1P8_SIGNAL_ENA should only
effect timing for MMC0 interface.

[0] https://lore.kernel.org/linux-mmc/20250127-am654-mmc-regression-v2-1-9bb39fb12810@solid-run.com/

Judith Mendez (2):
  PENDING: mmc: sdhci*: Add set_hs_ena to sdhci_ops
  mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch

 drivers/mmc/host/sdhci.c       | 55 +++++++++++++++++++++-------------
 drivers/mmc/host/sdhci.h       |  2 ++
 drivers/mmc/host/sdhci_am654.c | 48 +++++++++++++++++++++++++++++
 3 files changed, 85 insertions(+), 20 deletions(-)

-- 
2.49.0

[PATCH 1/2] PENDING: mmc: sdhci*: Add set_hs_ena to sdhci_ops

[Judith Mendez]

This patch adds set_hs_ena call to sdhci_ops so that host
controller drivers have the option to implement a custom
callback for SDHCI_CTRL_HISPD control.

On TI devices (for HS modes and legacy mode), timing was closed on
half cycle timing. If any of HIGH_SPEED_ENA, UHS_MODE_SELECT, or
V1P8_SIGNAL_ENA is set for these modes, host controller switches to
full cycle timing which may cause read/write failures and/or cqe error
logs.

So add set_hs_ena() to sdhci_ops so each host controller driver
can implement their own .set_hs_ena callback.

Also add sdhci_am654_set_hs_ena to sdhci_am654 driver and only set
HIGH_SPEED_ENA, for modes > MMC_TIMING_SD_HS.

Signed-off-by: Judith Mendez <jm@ti.com>
---
 drivers/mmc/host/sdhci.c       | 55 +++++++++++++++++++++-------------
 drivers/mmc/host/sdhci.h       |  2 ++
 drivers/mmc/host/sdhci_am654.c | 16 ++++++++++
 3 files changed, 53 insertions(+), 20 deletions(-)

diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
index 5f78be7ae16d7..3a878cf0c59b9 100644
--- a/drivers/mmc/host/sdhci.c
+++ b/drivers/mmc/host/sdhci.c
@@ -2355,6 +2355,27 @@ static bool sdhci_presetable_values_change(struct sdhci_host *host, struct mmc_i
 	       (sdhci_preset_needed(host, ios->timing) || host->drv_type != ios->drv_type);
 }
 
+void sdhci_set_hs_ena(struct sdhci_host *host, unsigned char timing)
+{
+	u8 ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
+
+	if (timing == MMC_TIMING_SD_HS ||
+	    timing == MMC_TIMING_MMC_HS ||
+	    timing == MMC_TIMING_MMC_HS400 ||
+	    timing == MMC_TIMING_MMC_HS200 ||
+	    timing == MMC_TIMING_MMC_DDR52 ||
+	    timing == MMC_TIMING_UHS_SDR50 ||
+	    timing == MMC_TIMING_UHS_SDR104 ||
+	    timing == MMC_TIMING_UHS_DDR50 ||
+	    timing == MMC_TIMING_UHS_SDR25)
+		ctrl |= SDHCI_CTRL_HISPD;
+	else
+		ctrl &= ~SDHCI_CTRL_HISPD;
+
+	sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
+}
+EXPORT_SYMBOL_GPL(sdhci_set_hs_ena);
+
 void sdhci_set_ios_common(struct mmc_host *mmc, struct mmc_ios *ios)
 {
 	struct sdhci_host *host = mmc_priv(mmc);
@@ -2436,23 +2457,6 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 	    !sdhci_presetable_values_change(host, ios))
 		return;
 
-	ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
-
-	if (!(host->quirks & SDHCI_QUIRK_NO_HISPD_BIT)) {
-		if (ios->timing == MMC_TIMING_SD_HS ||
-		     ios->timing == MMC_TIMING_MMC_HS ||
-		     ios->timing == MMC_TIMING_MMC_HS400 ||
-		     ios->timing == MMC_TIMING_MMC_HS200 ||
-		     ios->timing == MMC_TIMING_MMC_DDR52 ||
-		     ios->timing == MMC_TIMING_UHS_SDR50 ||
-		     ios->timing == MMC_TIMING_UHS_SDR104 ||
-		     ios->timing == MMC_TIMING_UHS_DDR50 ||
-		     ios->timing == MMC_TIMING_UHS_SDR25)
-			ctrl |= SDHCI_CTRL_HISPD;
-		else
-			ctrl &= ~SDHCI_CTRL_HISPD;
-	}
-
 	if (host->version >= SDHCI_SPEC_300) {
 		u16 clk, ctrl_2;
 
@@ -2468,7 +2472,12 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 			sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
 		}
 
-		sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
+		if (!(host->quirks & SDHCI_QUIRK_NO_HISPD_BIT)) {
+			if (host->ops->set_hs_ena)
+				host->ops->set_hs_ena(host, ios->timing);
+			else
+				sdhci_set_hs_ena(host, ios->timing);
+		}
 
 		if (!host->preset_enabled) {
 			/*
@@ -2510,8 +2519,14 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
 
 		/* Re-enable SD Clock */
 		host->ops->set_clock(host, host->clock);
-	} else
-		sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
+	} else {
+		if (!(host->quirks & SDHCI_QUIRK_NO_HISPD_BIT)) {
+			if (host->ops->set_hs_ena)
+				host->ops->set_hs_ena(host, ios->timing);
+			else
+				sdhci_set_hs_ena(host, ios->timing);
+		}
+	}
 }
 EXPORT_SYMBOL_GPL(sdhci_set_ios);
 
diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
index cd0e35a805427..ebecb49792ca1 100644
--- a/drivers/mmc/host/sdhci.h
+++ b/drivers/mmc/host/sdhci.h
@@ -704,6 +704,7 @@ struct sdhci_ops {
 	void		(*set_timeout)(struct sdhci_host *host,
 				       struct mmc_command *cmd);
 	void		(*set_bus_width)(struct sdhci_host *host, int width);
+	void		(*set_hs_ena)(struct sdhci_host *host, unsigned char timing);
 	void (*platform_send_init_74_clocks)(struct sdhci_host *host,
 					     u8 power_mode);
 	unsigned int    (*get_ro)(struct sdhci_host *host);
@@ -857,6 +858,7 @@ int sdhci_get_ro(struct mmc_host *mmc);
 void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq);
 int sdhci_request_atomic(struct mmc_host *mmc, struct mmc_request *mrq);
 void sdhci_set_bus_width(struct sdhci_host *host, int width);
+void sdhci_set_hs_ena(struct sdhci_host *host, unsigned char timing);
 void sdhci_reset(struct sdhci_host *host, u8 mask);
 bool sdhci_do_reset(struct sdhci_host *host, u8 mask);
 void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing);
diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index f75c31815ab00..67a64de4972c9 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -429,6 +429,19 @@ static int sdhci_am654_execute_tuning(struct mmc_host *mmc, u32 opcode)
 	return 0;
 }
 
+static void sdhci_am654_set_hs_ena(struct sdhci_host *host, unsigned char timing)
+{
+	u8 ctrl = 0;
+
+	if (timing > MMC_TIMING_SD_HS) {
+		sdhci_set_hs_ena(host, timing);
+	} else {
+		ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
+		ctrl &= ~SDHCI_CTRL_HISPD;
+		sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
+	}
+}
+
 static u32 sdhci_am654_cqhci_irq(struct sdhci_host *host, u32 intmask)
 {
 	int cmd_error = 0;
@@ -578,6 +591,7 @@ static const struct sdhci_ops sdhci_am654_ops = {
 	.get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
 	.set_uhs_signaling = sdhci_set_uhs_signaling,
 	.set_bus_width = sdhci_set_bus_width,
+	.set_hs_ena = sdhci_am654_set_hs_ena,
 	.set_power = sdhci_set_power_and_bus_voltage,
 	.set_clock = sdhci_am654_set_clock,
 	.write_b = sdhci_am654_write_b,
@@ -608,6 +622,7 @@ static const struct sdhci_ops sdhci_j721e_8bit_ops = {
 	.get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
 	.set_uhs_signaling = sdhci_set_uhs_signaling,
 	.set_bus_width = sdhci_set_bus_width,
+	.set_hs_ena = sdhci_am654_set_hs_ena,
 	.set_power = sdhci_set_power_and_bus_voltage,
 	.set_clock = sdhci_am654_set_clock,
 	.write_b = sdhci_am654_write_b,
@@ -632,6 +647,7 @@ static const struct sdhci_ops sdhci_j721e_4bit_ops = {
 	.get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
 	.set_uhs_signaling = sdhci_set_uhs_signaling,
 	.set_bus_width = sdhci_set_bus_width,
+	.set_hs_ena = sdhci_am654_set_hs_ena,
 	.set_power = sdhci_set_power_and_bus_voltage,
 	.set_clock = sdhci_j721e_4bit_set_clock,
 	.write_b = sdhci_am654_write_b,
-- 
2.49.0

[PATCH 2/2] mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch

[Judith Mendez]

The sdhci_start_signal_voltage_switch function sets
V1P8_SIGNAL_ENA by default after switching to 1v8 signaling.
V1P8_SIGNAL_ENA determines whether to launch cmd/data on neg
edge (half cycle timing) or pos edge (full cycle timing) of
clock.

The sequence is to switch to 1.8 IO voltage, set V1P8_SIGNAL_ENA,
change bus width, then update HIGH_SPEED_ENA & UHS_MODE_SELECT.

During bus width change is when eMMC failures are seen with
Kingston eMMC. So, do not set V1P8_SIGNAL_ENA by default.
V1P8_SIGNAL_ENA is anyways optional for eMMC and only affects
timing on TI devices. For switching to DDR52, HS200, or HS400,
we should rely on HIGH_SPEED_ENA, to switch to full cycle
timing after the bus width is changed.

Signed-off-by: Judith Mendez <jm@ti.com>
---
 drivers/mmc/host/sdhci_am654.c | 32 ++++++++++++++++++++++++++++++++
 1 file changed, 32 insertions(+)

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index 67a64de4972c9..4e1156a2f1b8e 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -87,6 +87,7 @@
 #define CLOCK_TOO_SLOW_HZ	50000000
 #define SDHCI_AM654_AUTOSUSPEND_DELAY	-1
 #define RETRY_TUNING_MAX	10
+#define BUS_WIDTH_8		8
 
 /* Command Queue Host Controller Interface Base address */
 #define SDHCI_AM654_CQE_BASE_ADDR 0x200
@@ -155,6 +156,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 {
@@ -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);
@@ -819,6 +844,7 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
 	struct device *dev = &pdev->dev;
 	int drv_strength;
 	int ret;
+	u32 bus_width;
 
 	if (sdhci_am654->flags & DLL_PRESENT) {
 		ret = device_property_read_u32(dev, "ti,trm-icp",
@@ -860,6 +886,11 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
 	if (device_property_read_bool(dev, "ti,fails-without-test-cd"))
 		sdhci_am654->quirks |= SDHCI_AM654_QUIRK_FORCE_CDTEST;
 
+	/* Suppress V1P8_SIGNAL_ENA for eMMC */
+	device_property_read_u32(dev, "bus-width", &bus_width);
+	if (bus_width == BUS_WIDTH_8)
+		sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
+
 	sdhci_get_of_property(pdev);
 
 	return 0;
@@ -956,6 +987,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);
-- 
2.49.0

Re: [PATCH 1/2] PENDING: mmc: sdhci*: Add set_hs_ena to sdhci_ops

[Ulf Hansson]

On Tue, 8 Apr 2025 at 00:27, Judith Mendez <jm@ti.com> wrote:
>
> This patch adds set_hs_ena call to sdhci_ops so that host
> controller drivers have the option to implement a custom
> callback for SDHCI_CTRL_HISPD control.
>
> On TI devices (for HS modes and legacy mode), timing was closed on
> half cycle timing. If any of HIGH_SPEED_ENA, UHS_MODE_SELECT, or
> V1P8_SIGNAL_ENA is set for these modes, host controller switches to
> full cycle timing which may cause read/write failures and/or cqe error
> logs.
>
> So add set_hs_ena() to sdhci_ops so each host controller driver
> can implement their own .set_hs_ena callback.
>
> Also add sdhci_am654_set_hs_ena to sdhci_am654 driver and only set
> HIGH_SPEED_ENA, for modes > MMC_TIMING_SD_HS.
>
> Signed-off-by: Judith Mendez <jm@ti.com>

What does "PENDING" mean or compared to "PATCH"? I guess you want some
review and test of this - or there anything else?

> ---
>  drivers/mmc/host/sdhci.c       | 55 +++++++++++++++++++++-------------
>  drivers/mmc/host/sdhci.h       |  2 ++
>  drivers/mmc/host/sdhci_am654.c | 16 ++++++++++

I think it would be better to split this up in two parts. One for
sdhci and one for sdhci_am654.

Other than that I am going to defer to Adrian to see what he thinks about this.

Kind regards
Uffe

>  3 files changed, 53 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
> index 5f78be7ae16d7..3a878cf0c59b9 100644
> --- a/drivers/mmc/host/sdhci.c
> +++ b/drivers/mmc/host/sdhci.c
> @@ -2355,6 +2355,27 @@ static bool sdhci_presetable_values_change(struct sdhci_host *host, struct mmc_i
>                (sdhci_preset_needed(host, ios->timing) || host->drv_type != ios->drv_type);
>  }
>
> +void sdhci_set_hs_ena(struct sdhci_host *host, unsigned char timing)
> +{
> +       u8 ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
> +
> +       if (timing == MMC_TIMING_SD_HS ||
> +           timing == MMC_TIMING_MMC_HS ||
> +           timing == MMC_TIMING_MMC_HS400 ||
> +           timing == MMC_TIMING_MMC_HS200 ||
> +           timing == MMC_TIMING_MMC_DDR52 ||
> +           timing == MMC_TIMING_UHS_SDR50 ||
> +           timing == MMC_TIMING_UHS_SDR104 ||
> +           timing == MMC_TIMING_UHS_DDR50 ||
> +           timing == MMC_TIMING_UHS_SDR25)
> +               ctrl |= SDHCI_CTRL_HISPD;
> +       else
> +               ctrl &= ~SDHCI_CTRL_HISPD;
> +
> +       sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
> +}
> +EXPORT_SYMBOL_GPL(sdhci_set_hs_ena);
> +
>  void sdhci_set_ios_common(struct mmc_host *mmc, struct mmc_ios *ios)
>  {
>         struct sdhci_host *host = mmc_priv(mmc);
> @@ -2436,23 +2457,6 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>             !sdhci_presetable_values_change(host, ios))
>                 return;
>
> -       ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
> -
> -       if (!(host->quirks & SDHCI_QUIRK_NO_HISPD_BIT)) {
> -               if (ios->timing == MMC_TIMING_SD_HS ||
> -                    ios->timing == MMC_TIMING_MMC_HS ||
> -                    ios->timing == MMC_TIMING_MMC_HS400 ||
> -                    ios->timing == MMC_TIMING_MMC_HS200 ||
> -                    ios->timing == MMC_TIMING_MMC_DDR52 ||
> -                    ios->timing == MMC_TIMING_UHS_SDR50 ||
> -                    ios->timing == MMC_TIMING_UHS_SDR104 ||
> -                    ios->timing == MMC_TIMING_UHS_DDR50 ||
> -                    ios->timing == MMC_TIMING_UHS_SDR25)
> -                       ctrl |= SDHCI_CTRL_HISPD;
> -               else
> -                       ctrl &= ~SDHCI_CTRL_HISPD;
> -       }
> -
>         if (host->version >= SDHCI_SPEC_300) {
>                 u16 clk, ctrl_2;
>
> @@ -2468,7 +2472,12 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>                         sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>                 }
>
> -               sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
> +               if (!(host->quirks & SDHCI_QUIRK_NO_HISPD_BIT)) {
> +                       if (host->ops->set_hs_ena)
> +                               host->ops->set_hs_ena(host, ios->timing);
> +                       else
> +                               sdhci_set_hs_ena(host, ios->timing);
> +               }
>
>                 if (!host->preset_enabled) {
>                         /*
> @@ -2510,8 +2519,14 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>
>                 /* Re-enable SD Clock */
>                 host->ops->set_clock(host, host->clock);
> -       } else
> -               sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
> +       } else {
> +               if (!(host->quirks & SDHCI_QUIRK_NO_HISPD_BIT)) {
> +                       if (host->ops->set_hs_ena)
> +                               host->ops->set_hs_ena(host, ios->timing);
> +                       else
> +                               sdhci_set_hs_ena(host, ios->timing);
> +               }
> +       }
>  }
>  EXPORT_SYMBOL_GPL(sdhci_set_ios);
>
> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
> index cd0e35a805427..ebecb49792ca1 100644
> --- a/drivers/mmc/host/sdhci.h
> +++ b/drivers/mmc/host/sdhci.h
> @@ -704,6 +704,7 @@ struct sdhci_ops {
>         void            (*set_timeout)(struct sdhci_host *host,
>                                        struct mmc_command *cmd);
>         void            (*set_bus_width)(struct sdhci_host *host, int width);
> +       void            (*set_hs_ena)(struct sdhci_host *host, unsigned char timing);
>         void (*platform_send_init_74_clocks)(struct sdhci_host *host,
>                                              u8 power_mode);
>         unsigned int    (*get_ro)(struct sdhci_host *host);
> @@ -857,6 +858,7 @@ int sdhci_get_ro(struct mmc_host *mmc);
>  void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq);
>  int sdhci_request_atomic(struct mmc_host *mmc, struct mmc_request *mrq);
>  void sdhci_set_bus_width(struct sdhci_host *host, int width);
> +void sdhci_set_hs_ena(struct sdhci_host *host, unsigned char timing);
>  void sdhci_reset(struct sdhci_host *host, u8 mask);
>  bool sdhci_do_reset(struct sdhci_host *host, u8 mask);
>  void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing);
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index f75c31815ab00..67a64de4972c9 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -429,6 +429,19 @@ static int sdhci_am654_execute_tuning(struct mmc_host *mmc, u32 opcode)
>         return 0;
>  }
>
> +static void sdhci_am654_set_hs_ena(struct sdhci_host *host, unsigned char timing)
> +{
> +       u8 ctrl = 0;
> +
> +       if (timing > MMC_TIMING_SD_HS) {
> +               sdhci_set_hs_ena(host, timing);
> +       } else {
> +               ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
> +               ctrl &= ~SDHCI_CTRL_HISPD;
> +               sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
> +       }
> +}
> +
>  static u32 sdhci_am654_cqhci_irq(struct sdhci_host *host, u32 intmask)
>  {
>         int cmd_error = 0;
> @@ -578,6 +591,7 @@ static const struct sdhci_ops sdhci_am654_ops = {
>         .get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
>         .set_uhs_signaling = sdhci_set_uhs_signaling,
>         .set_bus_width = sdhci_set_bus_width,
> +       .set_hs_ena = sdhci_am654_set_hs_ena,
>         .set_power = sdhci_set_power_and_bus_voltage,
>         .set_clock = sdhci_am654_set_clock,
>         .write_b = sdhci_am654_write_b,
> @@ -608,6 +622,7 @@ static const struct sdhci_ops sdhci_j721e_8bit_ops = {
>         .get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
>         .set_uhs_signaling = sdhci_set_uhs_signaling,
>         .set_bus_width = sdhci_set_bus_width,
> +       .set_hs_ena = sdhci_am654_set_hs_ena,
>         .set_power = sdhci_set_power_and_bus_voltage,
>         .set_clock = sdhci_am654_set_clock,
>         .write_b = sdhci_am654_write_b,
> @@ -632,6 +647,7 @@ static const struct sdhci_ops sdhci_j721e_4bit_ops = {
>         .get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
>         .set_uhs_signaling = sdhci_set_uhs_signaling,
>         .set_bus_width = sdhci_set_bus_width,
> +       .set_hs_ena = sdhci_am654_set_hs_ena,
>         .set_power = sdhci_set_power_and_bus_voltage,
>         .set_clock = sdhci_j721e_4bit_set_clock,
>         .write_b = sdhci_am654_write_b,
> --
> 2.49.0
>

Re: [PATCH 1/2] PENDING: mmc: sdhci*: Add set_hs_ena to sdhci_ops

[Judith Mendez]

Hi Ulf,

On 4/9/25 7:48 AM, Ulf Hansson wrote:
> On Tue, 8 Apr 2025 at 00:27, Judith Mendez <jm@ti.com> wrote:
>>
>> This patch adds set_hs_ena call to sdhci_ops so that host
>> controller drivers have the option to implement a custom
>> callback for SDHCI_CTRL_HISPD control.
>>
>> On TI devices (for HS modes and legacy mode), timing was closed on
>> half cycle timing. If any of HIGH_SPEED_ENA, UHS_MODE_SELECT, or
>> V1P8_SIGNAL_ENA is set for these modes, host controller switches to
>> full cycle timing which may cause read/write failures and/or cqe error
>> logs.
>>
>> So add set_hs_ena() to sdhci_ops so each host controller driver
>> can implement their own .set_hs_ena callback.
>>
>> Also add sdhci_am654_set_hs_ena to sdhci_am654 driver and only set
>> HIGH_SPEED_ENA, for modes > MMC_TIMING_SD_HS.
>>
>> Signed-off-by: Judith Mendez <jm@ti.com>
> 
> What does "PENDING" mean or compared to "PATCH"? I guess you want some
> review and test of this - or there anything else?

So sorry, I meant to remove this PENDING tag, will remove for v2.
Review would be just fine.

> 
>> ---
>>   drivers/mmc/host/sdhci.c       | 55 +++++++++++++++++++++-------------
>>   drivers/mmc/host/sdhci.h       |  2 ++
>>   drivers/mmc/host/sdhci_am654.c | 16 ++++++++++
> 
> I think it would be better to split this up in two parts. One for
> sdhci and one for sdhci_am654

Sure no problem. Thanks
.
> 
> Other than that I am going to defer to Adrian to see what he thinks about this.
> 
> Kind regards
> Uffe
> 
>>   3 files changed, 53 insertions(+), 20 deletions(-)
>>
>> diff --git a/drivers/mmc/host/sdhci.c b/drivers/mmc/host/sdhci.c
>> index 5f78be7ae16d7..3a878cf0c59b9 100644
>> --- a/drivers/mmc/host/sdhci.c
>> +++ b/drivers/mmc/host/sdhci.c
>> @@ -2355,6 +2355,27 @@ static bool sdhci_presetable_values_change(struct sdhci_host *host, struct mmc_i
>>                 (sdhci_preset_needed(host, ios->timing) || host->drv_type != ios->drv_type);
>>   }
>>
>> +void sdhci_set_hs_ena(struct sdhci_host *host, unsigned char timing)
>> +{
>> +       u8 ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
>> +
>> +       if (timing == MMC_TIMING_SD_HS ||
>> +           timing == MMC_TIMING_MMC_HS ||
>> +           timing == MMC_TIMING_MMC_HS400 ||
>> +           timing == MMC_TIMING_MMC_HS200 ||
>> +           timing == MMC_TIMING_MMC_DDR52 ||
>> +           timing == MMC_TIMING_UHS_SDR50 ||
>> +           timing == MMC_TIMING_UHS_SDR104 ||
>> +           timing == MMC_TIMING_UHS_DDR50 ||
>> +           timing == MMC_TIMING_UHS_SDR25)
>> +               ctrl |= SDHCI_CTRL_HISPD;
>> +       else
>> +               ctrl &= ~SDHCI_CTRL_HISPD;
>> +
>> +       sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
>> +}
>> +EXPORT_SYMBOL_GPL(sdhci_set_hs_ena);
>> +
>>   void sdhci_set_ios_common(struct mmc_host *mmc, struct mmc_ios *ios)
>>   {
>>          struct sdhci_host *host = mmc_priv(mmc);
>> @@ -2436,23 +2457,6 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>              !sdhci_presetable_values_change(host, ios))
>>                  return;
>>
>> -       ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
>> -
>> -       if (!(host->quirks & SDHCI_QUIRK_NO_HISPD_BIT)) {
>> -               if (ios->timing == MMC_TIMING_SD_HS ||
>> -                    ios->timing == MMC_TIMING_MMC_HS ||
>> -                    ios->timing == MMC_TIMING_MMC_HS400 ||
>> -                    ios->timing == MMC_TIMING_MMC_HS200 ||
>> -                    ios->timing == MMC_TIMING_MMC_DDR52 ||
>> -                    ios->timing == MMC_TIMING_UHS_SDR50 ||
>> -                    ios->timing == MMC_TIMING_UHS_SDR104 ||
>> -                    ios->timing == MMC_TIMING_UHS_DDR50 ||
>> -                    ios->timing == MMC_TIMING_UHS_SDR25)
>> -                       ctrl |= SDHCI_CTRL_HISPD;
>> -               else
>> -                       ctrl &= ~SDHCI_CTRL_HISPD;
>> -       }
>> -
>>          if (host->version >= SDHCI_SPEC_300) {
>>                  u16 clk, ctrl_2;
>>
>> @@ -2468,7 +2472,12 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>                          sdhci_writew(host, clk, SDHCI_CLOCK_CONTROL);
>>                  }
>>
>> -               sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
>> +               if (!(host->quirks & SDHCI_QUIRK_NO_HISPD_BIT)) {
>> +                       if (host->ops->set_hs_ena)
>> +                               host->ops->set_hs_ena(host, ios->timing);
>> +                       else
>> +                               sdhci_set_hs_ena(host, ios->timing);
>> +               }
>>
>>                  if (!host->preset_enabled) {
>>                          /*
>> @@ -2510,8 +2519,14 @@ void sdhci_set_ios(struct mmc_host *mmc, struct mmc_ios *ios)
>>
>>                  /* Re-enable SD Clock */
>>                  host->ops->set_clock(host, host->clock);
>> -       } else
>> -               sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
>> +       } else {
>> +               if (!(host->quirks & SDHCI_QUIRK_NO_HISPD_BIT)) {
>> +                       if (host->ops->set_hs_ena)
>> +                               host->ops->set_hs_ena(host, ios->timing);
>> +                       else
>> +                               sdhci_set_hs_ena(host, ios->timing);
>> +               }
>> +       }
>>   }
>>   EXPORT_SYMBOL_GPL(sdhci_set_ios);
>>
>> diff --git a/drivers/mmc/host/sdhci.h b/drivers/mmc/host/sdhci.h
>> index cd0e35a805427..ebecb49792ca1 100644
>> --- a/drivers/mmc/host/sdhci.h
>> +++ b/drivers/mmc/host/sdhci.h
>> @@ -704,6 +704,7 @@ struct sdhci_ops {
>>          void            (*set_timeout)(struct sdhci_host *host,
>>                                         struct mmc_command *cmd);
>>          void            (*set_bus_width)(struct sdhci_host *host, int width);
>> +       void            (*set_hs_ena)(struct sdhci_host *host, unsigned char timing);
>>          void (*platform_send_init_74_clocks)(struct sdhci_host *host,
>>                                               u8 power_mode);
>>          unsigned int    (*get_ro)(struct sdhci_host *host);
>> @@ -857,6 +858,7 @@ int sdhci_get_ro(struct mmc_host *mmc);
>>   void sdhci_request(struct mmc_host *mmc, struct mmc_request *mrq);
>>   int sdhci_request_atomic(struct mmc_host *mmc, struct mmc_request *mrq);
>>   void sdhci_set_bus_width(struct sdhci_host *host, int width);
>> +void sdhci_set_hs_ena(struct sdhci_host *host, unsigned char timing);
>>   void sdhci_reset(struct sdhci_host *host, u8 mask);
>>   bool sdhci_do_reset(struct sdhci_host *host, u8 mask);
>>   void sdhci_set_uhs_signaling(struct sdhci_host *host, unsigned timing);
>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>> index f75c31815ab00..67a64de4972c9 100644
>> --- a/drivers/mmc/host/sdhci_am654.c
>> +++ b/drivers/mmc/host/sdhci_am654.c
>> @@ -429,6 +429,19 @@ static int sdhci_am654_execute_tuning(struct mmc_host *mmc, u32 opcode)
>>          return 0;
>>   }
>>
>> +static void sdhci_am654_set_hs_ena(struct sdhci_host *host, unsigned char timing)
>> +{
>> +       u8 ctrl = 0;
>> +
>> +       if (timing > MMC_TIMING_SD_HS) {
>> +               sdhci_set_hs_ena(host, timing);
>> +       } else {
>> +               ctrl = sdhci_readb(host, SDHCI_HOST_CONTROL);
>> +               ctrl &= ~SDHCI_CTRL_HISPD;
>> +               sdhci_writeb(host, ctrl, SDHCI_HOST_CONTROL);
>> +       }
>> +}
>> +
>>   static u32 sdhci_am654_cqhci_irq(struct sdhci_host *host, u32 intmask)
>>   {
>>          int cmd_error = 0;
>> @@ -578,6 +591,7 @@ static const struct sdhci_ops sdhci_am654_ops = {
>>          .get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
>>          .set_uhs_signaling = sdhci_set_uhs_signaling,
>>          .set_bus_width = sdhci_set_bus_width,
>> +       .set_hs_ena = sdhci_am654_set_hs_ena,
>>          .set_power = sdhci_set_power_and_bus_voltage,
>>          .set_clock = sdhci_am654_set_clock,
>>          .write_b = sdhci_am654_write_b,
>> @@ -608,6 +622,7 @@ static const struct sdhci_ops sdhci_j721e_8bit_ops = {
>>          .get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
>>          .set_uhs_signaling = sdhci_set_uhs_signaling,
>>          .set_bus_width = sdhci_set_bus_width,
>> +       .set_hs_ena = sdhci_am654_set_hs_ena,
>>          .set_power = sdhci_set_power_and_bus_voltage,
>>          .set_clock = sdhci_am654_set_clock,
>>          .write_b = sdhci_am654_write_b,
>> @@ -632,6 +647,7 @@ static const struct sdhci_ops sdhci_j721e_4bit_ops = {
>>          .get_timeout_clock = sdhci_pltfm_clk_get_max_clock,
>>          .set_uhs_signaling = sdhci_set_uhs_signaling,
>>          .set_bus_width = sdhci_set_bus_width,
>> +       .set_hs_ena = sdhci_am654_set_hs_ena,
>>          .set_power = sdhci_set_power_and_bus_voltage,
>>          .set_clock = sdhci_j721e_4bit_set_clock,
>>          .write_b = sdhci_am654_write_b,
>> --
>> 2.49.0
>>

Re: [PATCH 0/2] Fix V1P8_SIGNAL_ENA and HIGH_SPEED_ENA

[Hiago De Franco]

Hi Judith,

On Mon, Apr 07, 2025 at 05:27:00PM -0500, Judith Mendez wrote:
> For all TI devices, timing was closed For Legacy and HS modes in
> half cycle timing, where data is launched on the negative edge of
> clock and latched on the following positive edge of clock. The
> switch to full cycle timing happens when any of HIGH_SPEED_ENA,
> V1P8_SIGNAL_ENA, or UHS_MODE_SELECT is set.
> 
> Currently HIGH_SPEED_ENA is set for HS modes and violates timing
> requirements for TI devices so add a .set_hs_ena callback in
> sdhci_am654 driver so that HIGH_SPEED_ENA is not set for this mode.
> 
> There are eMMC boot failures seen with V1P8_SIGNAL_ENA with a
> specific Kingston eMMC due to the sequencing when enumerating to
> HS200 mode. Since V1P8_SIGNAL_ENA is optional for eMMC, do not
> set V1P8_SIGNAL_ENA be default. This fix was previously merged in
> the kernel, but was reverted due to the "heuristics for enabling
> the quirk"[0]. The new implementation applies the quirk based-off of
> bus width, which should not be an issue since there is no internal
> LDO for MMC0 8bit wide interface and hence V1P8_SIGNAL_ENA should only
> effect timing for MMC0 interface.
> 
> [0] https://lore.kernel.org/linux-mmc/20250127-am654-mmc-regression-v2-1-9bb39fb12810@solid-run.com/
> 
> Judith Mendez (2):
>   PENDING: mmc: sdhci*: Add set_hs_ena to sdhci_ops
>   mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch
> 
>  drivers/mmc/host/sdhci.c       | 55 +++++++++++++++++++++-------------
>  drivers/mmc/host/sdhci.h       |  2 ++
>  drivers/mmc/host/sdhci_am654.c | 48 +++++++++++++++++++++++++++++
>  3 files changed, 85 insertions(+), 20 deletions(-)
> 
> -- 
> 2.49.0
>

Thanks for the patches. We are currently experiencing this issue on the
AM62 Solo SoC (hardware: Toradex Verdin AM62 Solo 512 MB), with the
latest kernel 6.15-rc1. I tested your patches (added both on top of
6.15-rc1) and I can still see the issue, when the kernel boots:

root@verdin-am62-15412807:~# dmesg | grep -i mmc1
[    1.912123] mmc1: CQHCI version 5.10
[    1.985262] mmc1: SDHCI controller on fa00000.mmc [fa00000.mmc] using ADMA 64-bit
[    2.186954] mmc1: error -110 whilst initialising SD card
[    2.620625] mmc1: error -110 whilst initialising SD card
[    2.997642] mmc1: error -110 whilst initialising SD card
[    3.337071] mmc1: error -110 whilst initialising SD card

This does not happen if I use commit 941a7abd4666 ("mmc: sdhci_am654:
Add sdhci_am654_start_signal_voltage_switch"), as you described.

Is there anything I missing or should test to make it work?

Cheers,
Hiago.

Re: [PATCH 0/2] Fix V1P8_SIGNAL_ENA and HIGH_SPEED_ENA

[Judith Mendez]

Hi Hiago,

On 4/11/25 8:03 AM, Hiago De Franco wrote:
> Hi Judith,
> 
> On Mon, Apr 07, 2025 at 05:27:00PM -0500, Judith Mendez wrote:
>> For all TI devices, timing was closed For Legacy and HS modes in
>> half cycle timing, where data is launched on the negative edge of
>> clock and latched on the following positive edge of clock. The
>> switch to full cycle timing happens when any of HIGH_SPEED_ENA,
>> V1P8_SIGNAL_ENA, or UHS_MODE_SELECT is set.
>>
>> Currently HIGH_SPEED_ENA is set for HS modes and violates timing
>> requirements for TI devices so add a .set_hs_ena callback in
>> sdhci_am654 driver so that HIGH_SPEED_ENA is not set for this mode.
>>
>> There are eMMC boot failures seen with V1P8_SIGNAL_ENA with a
>> specific Kingston eMMC due to the sequencing when enumerating to
>> HS200 mode. Since V1P8_SIGNAL_ENA is optional for eMMC, do not
>> set V1P8_SIGNAL_ENA be default. This fix was previously merged in
>> the kernel, but was reverted due to the "heuristics for enabling
>> the quirk"[0]. The new implementation applies the quirk based-off of
>> bus width, which should not be an issue since there is no internal
>> LDO for MMC0 8bit wide interface and hence V1P8_SIGNAL_ENA should only
>> effect timing for MMC0 interface.
>>
>> [0] https://lore.kernel.org/linux-mmc/20250127-am654-mmc-regression-v2-1-9bb39fb12810@solid-run.com/
>>
>> Judith Mendez (2):
>>    PENDING: mmc: sdhci*: Add set_hs_ena to sdhci_ops
>>    mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch
>>
>>   drivers/mmc/host/sdhci.c       | 55 +++++++++++++++++++++-------------
>>   drivers/mmc/host/sdhci.h       |  2 ++
>>   drivers/mmc/host/sdhci_am654.c | 48 +++++++++++++++++++++++++++++
>>   3 files changed, 85 insertions(+), 20 deletions(-)
>>
>> -- 
>> 2.49.0
>>
> 
> Thanks for the patches. We are currently experiencing this issue on the
> AM62 Solo SoC (hardware: Toradex Verdin AM62 Solo 512 MB), with the
> latest kernel 6.15-rc1. I tested your patches (added both on top of
> 6.15-rc1) and I can still see the issue, when the kernel boots:
> 
> root@verdin-am62-15412807:~# dmesg | grep -i mmc1
> [    1.912123] mmc1: CQHCI version 5.10
> [    1.985262] mmc1: SDHCI controller on fa00000.mmc [fa00000.mmc] using ADMA 64-bit
> [    2.186954] mmc1: error -110 whilst initialising SD card
> [    2.620625] mmc1: error -110 whilst initialising SD card
> [    2.997642] mmc1: error -110 whilst initialising SD card
> [    3.337071] mmc1: error -110 whilst initialising SD card
> 
> This does not happen if I use commit 941a7abd4666 ("mmc: sdhci_am654:
> Add sdhci_am654_start_signal_voltage_switch"), as you described.
> 
> Is there anything I missing or should test to make it work?

The reason that patches fixes SD init for you is because in original 
patch, quirk was applied for both SD and eMMC with the exception of SD 
for am64x SoC. This patch only applies the quirk for eMMC.

We cannot use the original implementation because the logic applying the 
quirk is based off of vmmc/vqmmc nodes in DT. The assumption was that 
am64x based boards will only have vmmc node since there is an internal 
LDO that is used to switch IO signal voltage instead of vqmmc. However, 
SolidRun HimmingBoard-T board has a different implementation on voltage 
regulator nodes in DT and the quirk applied to them as well and breaks 
their SD boot. So we now only apply the quirk for eMMC. Without this 
quirk applied to SD, am62x SD will continue having some stability issues.

~ Judith

Re: [PATCH 0/2] Fix V1P8_SIGNAL_ENA and HIGH_SPEED_ENA

[Hiago De Franco]

On Fri, Apr 11, 2025 at 11:37:14AM -0500, Judith Mendez wrote:
> 
> The reason that patches fixes SD init for you is because in original patch,
> quirk was applied for both SD and eMMC with the exception of SD for am64x
> SoC. This patch only applies the quirk for eMMC.
> 
> We cannot use the original implementation because the logic applying the
> quirk is based off of vmmc/vqmmc nodes in DT. The assumption was that am64x
> based boards will only have vmmc node since there is an internal LDO that is
> used to switch IO signal voltage instead of vqmmc. However, SolidRun
> HimmingBoard-T board has a different implementation on voltage regulator
> nodes in DT and the quirk applied to them as well and breaks their SD boot.
> So we now only apply the quirk for eMMC. Without this quirk applied to SD,
> am62x SD will continue having some stability issues.
> 
> ~ Judith

I got the idea now, thanks for the explanation, I missed in the original
patches this was only about the eMMC and *not* the SD card. Is there any
plans to send patches to the SD card as well?

About that, I was wondering if instead of checking the bus_width to
apply the fix for the eMMC, we could set a devicetree property to
explicity apply the quirk. This way, we could also decide to apply it on
the SD node without checking any bus width. As example:

diff --git a/arch/arm64/boot/dts/ti/k3-am62-verdin.dtsi b/arch/arm64/boot/dts/ti/k3-am62-verdin.dtsi
index 1ea8f64b1b3b..c4423c09e809 100644
--- a/arch/arm64/boot/dts/ti/k3-am62-verdin.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am62-verdin.dtsi
@@ -1466,6 +1466,7 @@ &sdhci1 {
 	vmmc-supply = <&reg_sdhc1_vmmc>;
 	vqmmc-supply = <&reg_sdhc1_vqmmc>;
 	ti,fails-without-test-cd;
+	ti,suppress-v1p8-ena;
 	status = "disabled";
 };
 
diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index 4e1156a2f1b8..a0485c2bb549 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -87,7 +87,6 @@
 #define CLOCK_TOO_SLOW_HZ	50000000
 #define SDHCI_AM654_AUTOSUSPEND_DELAY	-1
 #define RETRY_TUNING_MAX	10
-#define BUS_WIDTH_8		8
 
 /* Command Queue Host Controller Interface Base address */
 #define SDHCI_AM654_CQE_BASE_ADDR 0x200
@@ -844,7 +843,6 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
 	struct device *dev = &pdev->dev;
 	int drv_strength;
 	int ret;
-	u32 bus_width;
 
 	if (sdhci_am654->flags & DLL_PRESENT) {
 		ret = device_property_read_u32(dev, "ti,trm-icp",
@@ -886,9 +884,11 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
 	if (device_property_read_bool(dev, "ti,fails-without-test-cd"))
 		sdhci_am654->quirks |= SDHCI_AM654_QUIRK_FORCE_CDTEST;
 
-	/* Suppress V1P8_SIGNAL_ENA for eMMC */
-	device_property_read_u32(dev, "bus-width", &bus_width);
-	if (bus_width == BUS_WIDTH_8)
+	/*
+	 * Suppress V1P8_SIGNAL_ENA for MMC devices if ti,suppress-v1p8-ena
+	 * flag is present.
+	 */
+	if (device_property_read_bool(dev, "ti,suppress-v1p8-ena"))
 		sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
 
 	sdhci_get_of_property(pdev);

Which makes our Kingston SD card work again:

root@verdin-am62-15412807:~# dmesg | grep -i mmc1
[    1.897055] mmc1: CQHCI version 5.10
[    2.043673] mmc1: SDHCI controller on fa00000.mmc [fa00000.mmc] using ADMA 64-bit
[    2.260610] mmc1: new UHS-I speed SDR104 SDHC card at address 0001
[    2.268150] mmcblk1: mmc1:0001 SD32G 29.1 GiB

Sorry if I missed something, would also be a valid solution?

Cheers,
Hiago.

Re: [PATCH 0/2] Fix V1P8_SIGNAL_ENA and HIGH_SPEED_ENA

[Judith Mendez]

Hi Hilago,


On 4/11/25 2:48 PM, Hiago De Franco wrote:
> On Fri, Apr 11, 2025 at 11:37:14AM -0500, Judith Mendez wrote:
>>
>> The reason that patches fixes SD init for you is because in original patch,
>> quirk was applied for both SD and eMMC with the exception of SD for am64x
>> SoC. This patch only applies the quirk for eMMC.
>>
>> We cannot use the original implementation because the logic applying the
>> quirk is based off of vmmc/vqmmc nodes in DT. The assumption was that am64x
>> based boards will only have vmmc node since there is an internal LDO that is
>> used to switch IO signal voltage instead of vqmmc. However, SolidRun
>> HimmingBoard-T board has a different implementation on voltage regulator
>> nodes in DT and the quirk applied to them as well and breaks their SD boot.
>> So we now only apply the quirk for eMMC. Without this quirk applied to SD,
>> am62x SD will continue having some stability issues.
>>
>> ~ Judith
> 
> I got the idea now, thanks for the explanation, I missed in the original
> patches this was only about the eMMC and *not* the SD card. Is there any
> plans to send patches to the SD card as well?
> 
> About that, I was wondering if instead of checking the bus_width to
> apply the fix for the eMMC, we could set a devicetree property to
> explicity apply the quirk. This way, we could also decide to apply it on
> the SD node without checking any bus width. As example:
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-am62-verdin.dtsi b/arch/arm64/boot/dts/ti/k3-am62-verdin.dtsi
> index 1ea8f64b1b3b..c4423c09e809 100644
> --- a/arch/arm64/boot/dts/ti/k3-am62-verdin.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-am62-verdin.dtsi
> @@ -1466,6 +1466,7 @@ &sdhci1 {
>   	vmmc-supply = <&reg_sdhc1_vmmc>;
>   	vqmmc-supply = <&reg_sdhc1_vqmmc>;
>   	ti,fails-without-test-cd;
> +	ti,suppress-v1p8-ena;
>   	status = "disabled";
>   };
>   
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index 4e1156a2f1b8..a0485c2bb549 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -87,7 +87,6 @@
>   #define CLOCK_TOO_SLOW_HZ	50000000
>   #define SDHCI_AM654_AUTOSUSPEND_DELAY	-1
>   #define RETRY_TUNING_MAX	10
> -#define BUS_WIDTH_8		8
>   
>   /* Command Queue Host Controller Interface Base address */
>   #define SDHCI_AM654_CQE_BASE_ADDR 0x200
> @@ -844,7 +843,6 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>   	struct device *dev = &pdev->dev;
>   	int drv_strength;
>   	int ret;
> -	u32 bus_width;
>   
>   	if (sdhci_am654->flags & DLL_PRESENT) {
>   		ret = device_property_read_u32(dev, "ti,trm-icp",
> @@ -886,9 +884,11 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>   	if (device_property_read_bool(dev, "ti,fails-without-test-cd"))
>   		sdhci_am654->quirks |= SDHCI_AM654_QUIRK_FORCE_CDTEST;
>   
> -	/* Suppress V1P8_SIGNAL_ENA for eMMC */
> -	device_property_read_u32(dev, "bus-width", &bus_width);
> -	if (bus_width == BUS_WIDTH_8)
> +	/*
> +	 * Suppress V1P8_SIGNAL_ENA for MMC devices if ti,suppress-v1p8-ena
> +	 * flag is present.
> +	 */
> +	if (device_property_read_bool(dev, "ti,suppress-v1p8-ena"))
>   		sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
>   
>   	sdhci_get_of_property(pdev);
> 
> Which makes our Kingston SD card work again:
> 
> root@verdin-am62-15412807:~# dmesg | grep -i mmc1
> [    1.897055] mmc1: CQHCI version 5.10
> [    2.043673] mmc1: SDHCI controller on fa00000.mmc [fa00000.mmc] using ADMA 64-bit
> [    2.260610] mmc1: new UHS-I speed SDR104 SDHC card at address 0001
> [    2.268150] mmcblk1: mmc1:0001 SD32G 29.1 GiB
> 
> Sorry if I missed something, would also be a valid solution?

Actually this was one of the previous implementations, I should have 
that original patch somewhere. My understanding was that we do not like 
adding new DT properties if we can find a way to apply the quirk in the 
driver.

If this implementation flies with the maintainers, then we can go back 
to DT property implementation.

Adrian, is this fine with you?

~ Judith

Re: [PATCH 0/2] Fix V1P8_SIGNAL_ENA and HIGH_SPEED_ENA

[Hiago De Franco]

Hi Judith,

On Fri, Apr 11, 2025 at 04:55:39PM -0500, Judith Mendez wrote:
> Actually this was one of the previous implementations, I should have that
> original patch somewhere. My understanding was that we do not like adding
> new DT properties if we can find a way to apply the quirk in the driver.

Got it, makes sense. This will work fine with eMMC, but for the SD card
I am not seeing other option. Maybe we could use both implementations?

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index 4e1156a2f1b8..db8ee66e76d8 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -888,7 +888,7 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,

        /* Suppress V1P8_SIGNAL_ENA for eMMC */
        device_property_read_u32(dev, "bus-width", &bus_width);
-       if (bus_width == BUS_WIDTH_8)
+       if (bus_width == BUS_WIDTH_8 || device_property_read_bool(dev, "ti,suppress-v1p8-ena"))
                sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;

So the driver applies the quirk for eMMC and we set the DT property for
SD card. Not sure which one is the best, but maybe it is another option.
Let's see what Adrian also thinks about that.

> 
> If this implementation flies with the maintainers, then we can go back to DT
> property implementation.
> 
> Adrian, is this fine with you?
> 
> ~ Judith

Cheers,
Hiago.

Re: [PATCH 0/2] Fix V1P8_SIGNAL_ENA and HIGH_SPEED_ENA

[Francesco Dolcini]

Hello Judith

On Fri, Apr 11, 2025 at 04:55:39PM -0500, Judith Mendez wrote:
> My understanding was that we do not like adding new DT properties if
> we can find a way to apply the quirk in the driver.
...

> If this implementation flies with the maintainers, then we can go back to DT
> property implementation.

Not sure if this is clear, but this patch is NOT working according to
our tests, we would need to fix it in a different way.

Francesco

> 
> 
> 
> 
> 
> 

Re: [PATCH 0/2] Fix V1P8_SIGNAL_ENA and HIGH_SPEED_ENA

[Judith Mendez]

Hi Hiago,

On 4/12/25 8:20 AM, Hiago De Franco wrote:
> Hi Judith,
> 
> On Fri, Apr 11, 2025 at 04:55:39PM -0500, Judith Mendez wrote:
>> Actually this was one of the previous implementations, I should have that
>> original patch somewhere. My understanding was that we do not like adding
>> new DT properties if we can find a way to apply the quirk in the driver.
> 
> Got it, makes sense. This will work fine with eMMC, but for the SD card
> I am not seeing other option. Maybe we could use both implementations?
> 
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index 4e1156a2f1b8..db8ee66e76d8 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -888,7 +888,7 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
> 
>          /* Suppress V1P8_SIGNAL_ENA for eMMC */
>          device_property_read_u32(dev, "bus-width", &bus_width);
> -       if (bus_width == BUS_WIDTH_8)
> +       if (bus_width == BUS_WIDTH_8 || device_property_read_bool(dev, "ti,suppress-v1p8-ena"))
>                  sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
> 
> So the driver applies the quirk for eMMC and we set the DT property for
> SD card. Not sure which one is the best, but maybe it is another option.
> Let's see what Adrian also thinks about that.

I like it, sure lets see what Adrian says, thanks for your suggestion.

~ Judith

Re: [PATCH 0/2] Fix V1P8_SIGNAL_ENA and HIGH_SPEED_ENA

[Judith Mendez]

Hi Francesco,

On 4/14/25 1:51 AM, Francesco Dolcini wrote:
> Hello Judith
> 
> On Fri, Apr 11, 2025 at 04:55:39PM -0500, Judith Mendez wrote:
>> My understanding was that we do not like adding new DT properties if
>> we can find a way to apply the quirk in the driver.
> ...
> 
>> If this implementation flies with the maintainers, then we can go back to DT
>> property implementation.
> 
> Not sure if this is clear, but this patch is NOT working according to
> our tests, we would need to fix it in a different way.

Not sure if you are following the thread with Hiago, but we are fixing
it a different way; add a new DT property: "ti,suppress-v1p8-ena" for SD
card which allows us to have granular control on when v1p8 is suppressed
for MMC1.

With this method, we make sure the quirk is never applied to your board (;

~ Judith

Re: [PATCH 0/2] Fix V1P8_SIGNAL_ENA and HIGH_SPEED_ENA

[Francesco Dolcini]

On Mon, Apr 14, 2025 at 09:37:53AM -0500, Judith Mendez wrote:
> Hi Francesco,
> 
> On 4/14/25 1:51 AM, Francesco Dolcini wrote:
> > Hello Judith
> > 
> > On Fri, Apr 11, 2025 at 04:55:39PM -0500, Judith Mendez wrote:
> > > My understanding was that we do not like adding new DT properties if
> > > we can find a way to apply the quirk in the driver.
> > ...
> > 
> > > If this implementation flies with the maintainers, then we can go back to DT
> > > property implementation.
> > 
> > Not sure if this is clear, but this patch is NOT working according to
> > our tests, we would need to fix it in a different way.
> 
> Not sure if you are following the thread with Hiago, but we are fixing
> it a different way; add a new DT property: "ti,suppress-v1p8-ena" for SD
> card which allows us to have granular control on when v1p8 is suppressed
> for MMC1.
> 
> With this method, we make sure the quirk is never applied to your board (;

My board needs the quirk ;-)
It was not clear to me that this change was going to be added to this
patch, because without it's not working.

Thanks for the clarification.

Francesco

Re: [PATCH 0/2] Fix V1P8_SIGNAL_ENA and HIGH_SPEED_ENA

[Judith Mendez]

Hi Francesco,

On 4/14/25 9:57 AM, Francesco Dolcini wrote:
> On Mon, Apr 14, 2025 at 09:37:53AM -0500, Judith Mendez wrote:
>> Hi Francesco,
>>
>> On 4/14/25 1:51 AM, Francesco Dolcini wrote:
>>> Hello Judith
>>>
>>> On Fri, Apr 11, 2025 at 04:55:39PM -0500, Judith Mendez wrote:
>>>> My understanding was that we do not like adding new DT properties if
>>>> we can find a way to apply the quirk in the driver.
>>> ...
>>>
>>>> If this implementation flies with the maintainers, then we can go back to DT
>>>> property implementation.
>>>
>>> Not sure if this is clear, but this patch is NOT working according to
>>> our tests, we would need to fix it in a different way.
>>
>> Not sure if you are following the thread with Hiago, but we are fixing
>> it a different way; add a new DT property: "ti,suppress-v1p8-ena" for SD
>> card which allows us to have granular control on when v1p8 is suppressed
>> for MMC1.
>>
>> With this method, we make sure the quirk is never applied to your board (;
> 
> My board needs the quirk ;-)
> It was not clear to me that this change was going to be added to this
> patch, because without it's not working.
> 
> Thanks for the clarification.


Understood, Ok seems like we are just waiting on Adrian or another MMC
maintainer to comment before sending v2 with Hiago's suggestion,
thanks.

~ Judith

Re: [PATCH 0/2] Fix V1P8_SIGNAL_ENA and HIGH_SPEED_ENA

[Judith Mendez]

Hello Adrian,

On 4/7/25 5:27 PM, Judith Mendez wrote:
> For all TI devices, timing was closed For Legacy and HS modes in
> half cycle timing, where data is launched on the negative edge of
> clock and latched on the following positive edge of clock. The
> switch to full cycle timing happens when any of HIGH_SPEED_ENA,
> V1P8_SIGNAL_ENA, or UHS_MODE_SELECT is set.
> 
> Currently HIGH_SPEED_ENA is set for HS modes and violates timing
> requirements for TI devices so add a .set_hs_ena callback in
> sdhci_am654 driver so that HIGH_SPEED_ENA is not set for this mode.
> 
> There are eMMC boot failures seen with V1P8_SIGNAL_ENA with a
> specific Kingston eMMC due to the sequencing when enumerating to
> HS200 mode. Since V1P8_SIGNAL_ENA is optional for eMMC, do not
> set V1P8_SIGNAL_ENA be default. This fix was previously merged in
> the kernel, but was reverted due to the "heuristics for enabling
> the quirk"[0]. The new implementation applies the quirk based-off of
> bus width, which should not be an issue since there is no internal
> LDO for MMC0 8bit wide interface and hence V1P8_SIGNAL_ENA should only
> effect timing for MMC0 interface.


On this patch series, I am bringing back the fix for V1P8_SIGNAL_ENA,
Ulf requested a change [0] which I am planning to do for v2. But I was
hoping to get your opinion on whether Hiago's suggestion [1] is doable
so I can add that as well to v2. Thanks for your attention.


[0] 
https://lore.kernel.org/linux-mmc/CAPDyKFqx-G4NynanFWrspz7-uXXF74RfjcU-Sw2nq2JhL3LPuQ@mail.gmail.com/
[1] 
https://lore.kernel.org/linux-mmc/20250412132012.xpjywokcpztb4jg4@hiago-nb/

~ Judith

Re: [PATCH 0/2] Fix V1P8_SIGNAL_ENA and HIGH_SPEED_ENA

[Adrian Hunter]

On 16/04/25 19:59, Judith Mendez wrote:
> Hello Adrian,
> 
> On 4/7/25 5:27 PM, Judith Mendez wrote:
>> For all TI devices, timing was closed For Legacy and HS modes in
>> half cycle timing, where data is launched on the negative edge of
>> clock and latched on the following positive edge of clock. The
>> switch to full cycle timing happens when any of HIGH_SPEED_ENA,
>> V1P8_SIGNAL_ENA, or UHS_MODE_SELECT is set.
>>
>> Currently HIGH_SPEED_ENA is set for HS modes and violates timing
>> requirements for TI devices so add a .set_hs_ena callback in
>> sdhci_am654 driver so that HIGH_SPEED_ENA is not set for this mode.
>>
>> There are eMMC boot failures seen with V1P8_SIGNAL_ENA with a
>> specific Kingston eMMC due to the sequencing when enumerating to
>> HS200 mode. Since V1P8_SIGNAL_ENA is optional for eMMC, do not
>> set V1P8_SIGNAL_ENA be default. This fix was previously merged in
>> the kernel, but was reverted due to the "heuristics for enabling
>> the quirk"[0]. The new implementation applies the quirk based-off of
>> bus width, which should not be an issue since there is no internal
>> LDO for MMC0 8bit wide interface and hence V1P8_SIGNAL_ENA should only
>> effect timing for MMC0 interface.
> 
> 
> On this patch series, I am bringing back the fix for V1P8_SIGNAL_ENA,
> Ulf requested a change [0] which I am planning to do for v2. But I was
> hoping to get your opinion on whether Hiago's suggestion [1] is doable
> so I can add that as well to v2. Thanks for your attention.
> 
> 
> [0] https://lore.kernel.org/linux-mmc/CAPDyKFqx-G4NynanFWrspz7-uXXF74RfjcU-Sw2nq2JhL3LPuQ@mail.gmail.com/
> [1] https://lore.kernel.org/linux-mmc/20250412132012.xpjywokcpztb4jg4@hiago-nb/
> 

Sorry for the slow reply - been a bit distracted.

I'll look at it properly tomorrow, but noticed
sdhci_am654_write_b() already is dealing with SDHCI_HOST_CONTROL
and SDHCI_CTRL_HISPD.  Can you make use of that instead of
a .set_hs_ena callback?

Re: [PATCH 0/2] Fix V1P8_SIGNAL_ENA and HIGH_SPEED_ENA

[Adrian Hunter]

On 16/04/25 22:11, Adrian Hunter wrote:
> On 16/04/25 19:59, Judith Mendez wrote:
>> Hello Adrian,
>>
>> On 4/7/25 5:27 PM, Judith Mendez wrote:
>>> For all TI devices, timing was closed For Legacy and HS modes in
>>> half cycle timing, where data is launched on the negative edge of
>>> clock and latched on the following positive edge of clock. The
>>> switch to full cycle timing happens when any of HIGH_SPEED_ENA,
>>> V1P8_SIGNAL_ENA, or UHS_MODE_SELECT is set.
>>>
>>> Currently HIGH_SPEED_ENA is set for HS modes and violates timing
>>> requirements for TI devices so add a .set_hs_ena callback in
>>> sdhci_am654 driver so that HIGH_SPEED_ENA is not set for this mode.
>>>
>>> There are eMMC boot failures seen with V1P8_SIGNAL_ENA with a
>>> specific Kingston eMMC due to the sequencing when enumerating to
>>> HS200 mode. Since V1P8_SIGNAL_ENA is optional for eMMC, do not
>>> set V1P8_SIGNAL_ENA be default. This fix was previously merged in
>>> the kernel, but was reverted due to the "heuristics for enabling
>>> the quirk"[0]. The new implementation applies the quirk based-off of
>>> bus width, which should not be an issue since there is no internal
>>> LDO for MMC0 8bit wide interface and hence V1P8_SIGNAL_ENA should only
>>> effect timing for MMC0 interface.
>>
>>
>> On this patch series, I am bringing back the fix for V1P8_SIGNAL_ENA,
>> Ulf requested a change [0] which I am planning to do for v2. But I was
>> hoping to get your opinion on whether Hiago's suggestion [1] is doable
>> so I can add that as well to v2. Thanks for your attention.
>>
>>
>> [0] https://lore.kernel.org/linux-mmc/CAPDyKFqx-G4NynanFWrspz7-uXXF74RfjcU-Sw2nq2JhL3LPuQ@mail.gmail.com/
>> [1] https://lore.kernel.org/linux-mmc/20250412132012.xpjywokcpztb4jg4@hiago-nb/
>>
> 
> Sorry for the slow reply - been a bit distracted.
> 
> I'll look at it properly tomorrow, but noticed
> sdhci_am654_write_b() already is dealing with SDHCI_HOST_CONTROL
> and SDHCI_CTRL_HISPD.  Can you make use of that instead of
> a .set_hs_ena callback?

Patch 1 continues to look unneeded because sdhci_am654_write_b()
seems to do the same thing.

WRT patch 2, the suggestion to add a DT property and check the bus
width seems fine to me.  DT properties can be added to identify
"broken" hardware that cannot be identified by the compatible
string.

Re: [PATCH 0/2] Fix V1P8_SIGNAL_ENA and HIGH_SPEED_ENA

[Judith Mendez]

Hi Adrian,

On 4/17/25 7:03 AM, Adrian Hunter wrote:
> On 16/04/25 22:11, Adrian Hunter wrote:
>> On 16/04/25 19:59, Judith Mendez wrote:
>>> Hello Adrian,
>>>
>>> On 4/7/25 5:27 PM, Judith Mendez wrote:
>>>> For all TI devices, timing was closed For Legacy and HS modes in
>>>> half cycle timing, where data is launched on the negative edge of
>>>> clock and latched on the following positive edge of clock. The
>>>> switch to full cycle timing happens when any of HIGH_SPEED_ENA,
>>>> V1P8_SIGNAL_ENA, or UHS_MODE_SELECT is set.
>>>>
>>>> Currently HIGH_SPEED_ENA is set for HS modes and violates timing
>>>> requirements for TI devices so add a .set_hs_ena callback in
>>>> sdhci_am654 driver so that HIGH_SPEED_ENA is not set for this mode.
>>>>
>>>> There are eMMC boot failures seen with V1P8_SIGNAL_ENA with a
>>>> specific Kingston eMMC due to the sequencing when enumerating to
>>>> HS200 mode. Since V1P8_SIGNAL_ENA is optional for eMMC, do not
>>>> set V1P8_SIGNAL_ENA be default. This fix was previously merged in
>>>> the kernel, but was reverted due to the "heuristics for enabling
>>>> the quirk"[0]. The new implementation applies the quirk based-off of
>>>> bus width, which should not be an issue since there is no internal
>>>> LDO for MMC0 8bit wide interface and hence V1P8_SIGNAL_ENA should only
>>>> effect timing for MMC0 interface.
>>>
>>>
>>> On this patch series, I am bringing back the fix for V1P8_SIGNAL_ENA,
>>> Ulf requested a change [0] which I am planning to do for v2. But I was
>>> hoping to get your opinion on whether Hiago's suggestion [1] is doable
>>> so I can add that as well to v2. Thanks for your attention.
>>>
>>>
>>> [0] https://lore.kernel.org/linux-mmc/CAPDyKFqx-G4NynanFWrspz7-uXXF74RfjcU-Sw2nq2JhL3LPuQ@mail.gmail.com/
>>> [1] https://lore.kernel.org/linux-mmc/20250412132012.xpjywokcpztb4jg4@hiago-nb/
>>>
>>
>> Sorry for the slow reply - been a bit distracted.
>>
>> I'll look at it properly tomorrow, but noticed
>> sdhci_am654_write_b() already is dealing with SDHCI_HOST_CONTROL
>> and SDHCI_CTRL_HISPD.  Can you make use of that instead of
>> a .set_hs_ena callback?
> 
> Patch 1 continues to look unneeded because sdhci_am654_write_b()
> seems to do the same thing.

You are right, I will drop the first patch then.

> 
> WRT patch 2, the suggestion to add a DT property and check the bus
> width seems fine to me.  DT properties can be added to identify
> "broken" hardware that cannot be identified by the compatible
> string.
> 
Cool, will add this functionality to v2, thanks

~ Judith