mbox series

[0/2] Fix V1P8_SIGNAL_ENA and HIGH_SPEED_ENA

Message ID 20250407222702.2199047-1-jm@ti.com
Headers show
Series Fix V1P8_SIGNAL_ENA and HIGH_SPEED_ENA | expand

Message

Judith Mendez April 7, 2025, 10:27 p.m. UTC
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(-)

Comments

Ulf Hansson April 9, 2025, 12:48 p.m. UTC | #1
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
>
Judith Mendez April 9, 2025, 5:11 p.m. UTC | #2
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
>>
Hiago De Franco April 11, 2025, 1:03 p.m. UTC | #3
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.
Judith Mendez April 11, 2025, 4:37 p.m. UTC | #4
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
Hiago De Franco April 11, 2025, 7:48 p.m. UTC | #5
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.
Judith Mendez April 11, 2025, 9:55 p.m. UTC | #6
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
Hiago De Franco April 12, 2025, 1:20 p.m. UTC | #7
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.
Francesco Dolcini April 14, 2025, 6:51 a.m. UTC | #8
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

> 
> 
> 
> 
> 
>
Judith Mendez April 14, 2025, 2:31 p.m. UTC | #9
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
Judith Mendez April 14, 2025, 2:37 p.m. UTC | #10
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
Francesco Dolcini April 14, 2025, 2:57 p.m. UTC | #11
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
Judith Mendez April 14, 2025, 10:56 p.m. UTC | #12
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
Judith Mendez April 16, 2025, 4:59 p.m. UTC | #13
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
Adrian Hunter April 16, 2025, 7:11 p.m. UTC | #14
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?
Adrian Hunter April 17, 2025, 12:03 p.m. UTC | #15
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.
Judith Mendez April 17, 2025, 3:05 p.m. UTC | #16
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