| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ceb7869a-2bdd-4a13-ad63-6f29bb8a2ab9@ti.com>
Date: Wed, 9 Apr 2025 12:11:59 -0500
From: Judith Mendez <jm@...com>
To: Ulf Hansson <ulf.hansson@...aro.org>
CC: Adrian Hunter <adrian.hunter@...el.com>, <linux-mmc@...r.kernel.org>,
<linux-kernel@...r.kernel.org>, Josua Mayer <josua@...id-run.com>,
Moteen
Shah <m-shah@...com>
Subject: Re: [PATCH 1/2] PENDING: mmc: sdhci*: Add set_hs_ena to sdhci_ops
Hi Ulf,
On 4/9/25 7:48 AM, Ulf Hansson wrote:
> On Tue, 8 Apr 2025 at 00:27, Judith Mendez <jm@...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@...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
>>
Powered by blists - more mailing lists