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

From: Ulf Hansson
Date: Wed Apr 09 2025 - 08:49:40 EST

Next message: James Bottomley: "Re: commit 7b025f3f85ed causes NULL pointer dereference"
Previous message: syzbot: "[syzbot] [net?] KASAN: null-ptr-deref Write in rcuref_put (4)"
Next in thread: Judith Mendez: "Re: [PATCH 1/2] PENDING: mmc: sdhci*: Add set_hs_ena to sdhci_ops"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, 8 Apr 2025 at 00:27, Judith Mendez <jm@xxxxxx> 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@xxxxxx>

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
>

Next message: James Bottomley: "Re: commit 7b025f3f85ed causes NULL pointer dereference"
Previous message: syzbot: "[syzbot] [net?] KASAN: null-ptr-deref Write in rcuref_put (4)"
Next in thread: Judith Mendez: "Re: [PATCH 1/2] PENDING: mmc: sdhci*: Add set_hs_ena to sdhci_ops"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]