drivers/mmc/host/sdhci_am654.c | 86 ++++++++++++++++++++++++++++++++++ 1 file changed, 86 insertions(+)
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 or pos edge of clock.
Due to some eMMC and SD failures seen across am62x platform,
do not set V1P8_SIGNAL_ENA by default, only enable the bit
for devices that require this bit in order to switch to 1v8
voltage for uhs modes.
Signed-off-by: Judith Mendez <jm@ti.com>
---
drivers/mmc/host/sdhci_am654.c | 86 ++++++++++++++++++++++++++++++++++
1 file changed, 86 insertions(+)
diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index 0aa3c40ea6ed8..fb6232e56606b 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -155,6 +155,7 @@ struct sdhci_am654_data {
u32 tuning_loop;
#define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
+#define SDHCI_AM654_QUIRK_SET_V1P8_ENA BIT(1)
};
struct window {
@@ -356,6 +357,79 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
sdhci_set_clock(host, clock);
}
+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);
+ u16 ctrl;
+ int ret;
+
+ if (host->version < SDHCI_SPEC_300)
+ return 0;
+
+ switch (ios->signal_voltage) {
+ case MMC_SIGNAL_VOLTAGE_330:
+ if (!(host->flags & SDHCI_SIGNALING_330))
+ return -EINVAL;
+
+ ctrl &= ~SDHCI_CTRL_VDD_180;
+ sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
+
+ if (!IS_ERR(mmc->supply.vqmmc)) {
+ ret = mmc_regulator_set_vqmmc(mmc, ios);
+ if (ret < 0) {
+ pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
+ mmc_hostname(mmc));
+ return -EIO;
+ }
+ }
+
+ usleep_range(5000, 5500);
+
+ ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+ if (!(ctrl & SDHCI_CTRL_VDD_180))
+ return 0;
+
+ pr_warn("%s: 3.3V regulator output did not become stable\n",
+ mmc_hostname(mmc));
+
+ return -EAGAIN;
+
+ case MMC_SIGNAL_VOLTAGE_180:
+ if (!(host->flags & SDHCI_SIGNALING_180))
+ return -EINVAL;
+
+ if (!IS_ERR(mmc->supply.vqmmc)) {
+ ret = mmc_regulator_set_vqmmc(mmc, ios);
+ if (ret < 0) {
+ pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
+ mmc_hostname(mmc));
+ return -EIO;
+ }
+ }
+
+ if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_SET_V1P8_ENA) {
+ ctrl |= SDHCI_CTRL_VDD_180;
+ sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
+
+ ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
+ if (ctrl & SDHCI_CTRL_VDD_180)
+ return 0;
+
+ pr_warn("%s: 1.8V regulator output did not become stable\n",
+ mmc_hostname(mmc));
+
+ return -EAGAIN;
+ }
+ return 0;
+
+ default:
+ return 0;
+ }
+}
+
static u8 sdhci_am654_write_power_on(struct sdhci_host *host, u8 val, int reg)
{
writeb(val, host->ioaddr + reg);
@@ -801,6 +875,8 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
struct sdhci_am654_data *sdhci_am654)
{
struct device *dev = &pdev->dev;
+ struct device_node *np = dev->of_node;
+ struct device_node *node;
int drv_strength;
int ret;
@@ -844,6 +920,15 @@ 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;
+ node = of_parse_phandle(np, "vmmc-supply", 0);
+
+ if (node) {
+ node = of_parse_phandle(np, "vqmmc-supply", 0);
+
+ if (!node)
+ sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SET_V1P8_ENA;
+ }
+
sdhci_get_of_property(pdev);
return 0;
@@ -940,6 +1025,7 @@ static int sdhci_am654_probe(struct platform_device *pdev)
goto err_pltfm_free;
}
+ host->mmc_host_ops.start_signal_voltage_switch = sdhci_am654_start_signal_voltage_switch;
host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning;
pm_runtime_get_noresume(dev);
base-commit: cf6444ba528f043398b112ac36e041a4d8685cb1
--
2.46.0On 6/09/24 20:50, Judith Mendez wrote:
> 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 or pos edge of clock.
>
> Due to some eMMC and SD failures seen across am62x platform,
> do not set V1P8_SIGNAL_ENA by default, only enable the bit
> for devices that require this bit in order to switch to 1v8
> voltage for uhs modes.
>
> Signed-off-by: Judith Mendez <jm@ti.com>
> ---
> drivers/mmc/host/sdhci_am654.c | 86 ++++++++++++++++++++++++++++++++++
> 1 file changed, 86 insertions(+)
>
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index 0aa3c40ea6ed8..fb6232e56606b 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -155,6 +155,7 @@ struct sdhci_am654_data {
> u32 tuning_loop;
>
> #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
> +#define SDHCI_AM654_QUIRK_SET_V1P8_ENA BIT(1)
It would be better for the quirk to represent the deviation
from normal i.e.
#define SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA BIT(1)
> };
>
> struct window {
> @@ -356,6 +357,79 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
> sdhci_set_clock(host, clock);
> }
>
> +int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc,
> + struct mmc_ios *ios)
Simpler to call sdhci_start_signal_voltage_switch() for the normal
case e.g.
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);
if ((sdhci_am654->quirks & SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) &&
ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
ret = mmc_regulator_set_vqmmc(mmc, ios);
if (ret < 0) {
pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
mmc_hostname(mmc));
return -EIO;
}
return 0;
}
return sdhci_start_signal_voltage_switch(mmc, 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);
> + u16 ctrl;
> + int ret;
> +
> + if (host->version < SDHCI_SPEC_300)
> + return 0;
> +
> + switch (ios->signal_voltage) {
> + case MMC_SIGNAL_VOLTAGE_330:
> + if (!(host->flags & SDHCI_SIGNALING_330))
> + return -EINVAL;
> +
> + ctrl &= ~SDHCI_CTRL_VDD_180;
> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
> +
> + if (!IS_ERR(mmc->supply.vqmmc)) {
> + ret = mmc_regulator_set_vqmmc(mmc, ios);
> + if (ret < 0) {
> + pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
> + mmc_hostname(mmc));
> + return -EIO;
> + }
> + }
> +
> + usleep_range(5000, 5500);
> +
> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> + if (!(ctrl & SDHCI_CTRL_VDD_180))
> + return 0;
> +
> + pr_warn("%s: 3.3V regulator output did not become stable\n",
> + mmc_hostname(mmc));
> +
> + return -EAGAIN;
> +
> + case MMC_SIGNAL_VOLTAGE_180:
> + if (!(host->flags & SDHCI_SIGNALING_180))
> + return -EINVAL;
> +
> + if (!IS_ERR(mmc->supply.vqmmc)) {
> + ret = mmc_regulator_set_vqmmc(mmc, ios);
> + if (ret < 0) {
> + pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
> + mmc_hostname(mmc));
> + return -EIO;
> + }
> + }
> +
> + if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_SET_V1P8_ENA) {
> + ctrl |= SDHCI_CTRL_VDD_180;
> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
> +
> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
> + if (ctrl & SDHCI_CTRL_VDD_180)
> + return 0;
> +
> + pr_warn("%s: 1.8V regulator output did not become stable\n",
> + mmc_hostname(mmc));
> +
> + return -EAGAIN;
> + }
> + return 0;
> +
> + default:
> + return 0;
> + }
> +}
> +
> static u8 sdhci_am654_write_power_on(struct sdhci_host *host, u8 val, int reg)
> {
> writeb(val, host->ioaddr + reg);
> @@ -801,6 +875,8 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
> struct sdhci_am654_data *sdhci_am654)
> {
> struct device *dev = &pdev->dev;
> + struct device_node *np = dev->of_node;
> + struct device_node *node;
> int drv_strength;
> int ret;
>
> @@ -844,6 +920,15 @@ 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;
>
> + node = of_parse_phandle(np, "vmmc-supply", 0);
> +
> + if (node) {
> + node = of_parse_phandle(np, "vqmmc-supply", 0);
> +
> + if (!node)
> + sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SET_V1P8_ENA;
> + }
It would be simpler without 'np' and 'node'. Not sure
what the rule is meant to be, but it could be something like:
if (of_parse_phandle(dev->of_node, "vmmc-supply", 0) &&
of_parse_phandle(dev->of_node, "vqmmc-supply", 0)
sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
> +
> sdhci_get_of_property(pdev);
>
> return 0;
> @@ -940,6 +1025,7 @@ static int sdhci_am654_probe(struct platform_device *pdev)
> goto err_pltfm_free;
> }
>
> + host->mmc_host_ops.start_signal_voltage_switch = sdhci_am654_start_signal_voltage_switch;
> host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning;
>
> pm_runtime_get_noresume(dev);
>
> base-commit: cf6444ba528f043398b112ac36e041a4d8685cb1
Hi Adrian,
On 9/9/24 1:26 AM, Adrian Hunter wrote:
> On 6/09/24 20:50, Judith Mendez wrote:
>> 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 or pos edge of clock.
>>
>> Due to some eMMC and SD failures seen across am62x platform,
>> do not set V1P8_SIGNAL_ENA by default, only enable the bit
>> for devices that require this bit in order to switch to 1v8
>> voltage for uhs modes.
>>
>> Signed-off-by: Judith Mendez <jm@ti.com>
>> ---
>> drivers/mmc/host/sdhci_am654.c | 86 ++++++++++++++++++++++++++++++++++
>> 1 file changed, 86 insertions(+)
>>
>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>> index 0aa3c40ea6ed8..fb6232e56606b 100644
>> --- a/drivers/mmc/host/sdhci_am654.c
>> +++ b/drivers/mmc/host/sdhci_am654.c
>> @@ -155,6 +155,7 @@ struct sdhci_am654_data {
>> u32 tuning_loop;
>>
>> #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
>> +#define SDHCI_AM654_QUIRK_SET_V1P8_ENA BIT(1)
>
> It would be better for the quirk to represent the deviation
> from normal i.e.
>
> #define SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA BIT(1)
>
>> };
>>
>> struct window {
>> @@ -356,6 +357,79 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
>> sdhci_set_clock(host, clock);
>> }
>>
>> +int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc,
>> + struct mmc_ios *ios)
>
> Simpler to call sdhci_start_signal_voltage_switch() for the normal
> case e.g.
This is simpler, so sure will use thanks.
>
> 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);
>
>
> if ((sdhci_am654->quirks & SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) &&
> ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
> ret = mmc_regulator_set_vqmmc(mmc, ios);
> if (ret < 0) {
> pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
> mmc_hostname(mmc));
> return -EIO;
> }
> return 0;
> }
>
> return sdhci_start_signal_voltage_switch(mmc, 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);
>> + u16 ctrl;
>> + int ret;
>> +
>> + if (host->version < SDHCI_SPEC_300)
>> + return 0;
>> +
>> + switch (ios->signal_voltage) {
>> + case MMC_SIGNAL_VOLTAGE_330:
>> + if (!(host->flags & SDHCI_SIGNALING_330))
>> + return -EINVAL;
>> +
>> + ctrl &= ~SDHCI_CTRL_VDD_180;
>> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>> +
>> + if (!IS_ERR(mmc->supply.vqmmc)) {
>> + ret = mmc_regulator_set_vqmmc(mmc, ios);
>> + if (ret < 0) {
>> + pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
>> + mmc_hostname(mmc));
>> + return -EIO;
>> + }
>> + }
>> +
>> + usleep_range(5000, 5500);
>> +
>> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>> + if (!(ctrl & SDHCI_CTRL_VDD_180))
>> + return 0;
>> +
>> + pr_warn("%s: 3.3V regulator output did not become stable\n",
>> + mmc_hostname(mmc));
>> +
>> + return -EAGAIN;
>> +
>> + case MMC_SIGNAL_VOLTAGE_180:
>> + if (!(host->flags & SDHCI_SIGNALING_180))
>> + return -EINVAL;
>> +
>> + if (!IS_ERR(mmc->supply.vqmmc)) {
>> + ret = mmc_regulator_set_vqmmc(mmc, ios);
>> + if (ret < 0) {
>> + pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
>> + mmc_hostname(mmc));
>> + return -EIO;
>> + }
>> + }
>> +
>> + if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_SET_V1P8_ENA) {
>> + ctrl |= SDHCI_CTRL_VDD_180;
>> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>> +
>> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>> + if (ctrl & SDHCI_CTRL_VDD_180)
>> + return 0;
>> +
>> + pr_warn("%s: 1.8V regulator output did not become stable\n",
>> + mmc_hostname(mmc));
>> +
>> + return -EAGAIN;
>> + }
>> + return 0;
>> +
>> + default:
>> + return 0;
>> + }
>> +}
>> +
>> static u8 sdhci_am654_write_power_on(struct sdhci_host *host, u8 val, int reg)
>> {
>> writeb(val, host->ioaddr + reg);
>> @@ -801,6 +875,8 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>> struct sdhci_am654_data *sdhci_am654)
>> {
>> struct device *dev = &pdev->dev;
>> + struct device_node *np = dev->of_node;
>> + struct device_node *node;
>> int drv_strength;
>> int ret;
>>
>> @@ -844,6 +920,15 @@ 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;
>>
>> + node = of_parse_phandle(np, "vmmc-supply", 0);
>> +
>> + if (node) {
>> + node = of_parse_phandle(np, "vqmmc-supply", 0);
>> +
>> + if (!node)
>> + sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SET_V1P8_ENA;
>> + }
>
> It would be simpler without 'np' and 'node'. Not sure
> what the rule is meant to be, but it could be something like:
>
> if (of_parse_phandle(dev->of_node, "vmmc-supply", 0) &&
> of_parse_phandle(dev->of_node, "vqmmc-supply", 0)
> sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
My issue with this is that I also need the quirk
(SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) for eMMC. DT node for eMMC does
not include vmmc and vqmmc supplies. That is why I had the quirk logic
inverted.
This patch fixes timing issues with both eMMC and SD. (:
~ Judith
>
>> +
>> sdhci_get_of_property(pdev);
>>
>> return 0;
>> @@ -940,6 +1025,7 @@ static int sdhci_am654_probe(struct platform_device *pdev)
>> goto err_pltfm_free;
>> }
>>
>> + host->mmc_host_ops.start_signal_voltage_switch = sdhci_am654_start_signal_voltage_switch;
>> host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning;
>>
>> pm_runtime_get_noresume(dev);
>>
>> base-commit: cf6444ba528f043398b112ac36e041a4d8685cb1
>
>
On 10/09/24 17:30, Judith Mendez wrote:
> Hi Adrian,
>
> On 9/9/24 1:26 AM, Adrian Hunter wrote:
>> On 6/09/24 20:50, Judith Mendez wrote:
>>> 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 or pos edge of clock.
>>>
>>> Due to some eMMC and SD failures seen across am62x platform,
>>> do not set V1P8_SIGNAL_ENA by default, only enable the bit
>>> for devices that require this bit in order to switch to 1v8
>>> voltage for uhs modes.
>>>
>>> Signed-off-by: Judith Mendez <jm@ti.com>
>>> ---
>>> drivers/mmc/host/sdhci_am654.c | 86 ++++++++++++++++++++++++++++++++++
>>> 1 file changed, 86 insertions(+)
>>>
>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>>> index 0aa3c40ea6ed8..fb6232e56606b 100644
>>> --- a/drivers/mmc/host/sdhci_am654.c
>>> +++ b/drivers/mmc/host/sdhci_am654.c
>>> @@ -155,6 +155,7 @@ struct sdhci_am654_data {
>>> u32 tuning_loop;
>>> #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
>>> +#define SDHCI_AM654_QUIRK_SET_V1P8_ENA BIT(1)
>>
>> It would be better for the quirk to represent the deviation
>> from normal i.e.
>>
>> #define SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA BIT(1)
>>
>>> };
>>> struct window {
>>> @@ -356,6 +357,79 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
>>> sdhci_set_clock(host, clock);
>>> }
>>> +int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc,
>>> + struct mmc_ios *ios)
>>
>> Simpler to call sdhci_start_signal_voltage_switch() for the normal
>> case e.g.
>
> This is simpler, so sure will use thanks.
>
>>
>> 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);
>>
>>
>> if ((sdhci_am654->quirks & SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) &&
>> ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
>> ret = mmc_regulator_set_vqmmc(mmc, ios);
>> if (ret < 0) {
>> pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
>> mmc_hostname(mmc));
>> return -EIO;
>> }
>> return 0;
>> }
>>
>> return sdhci_start_signal_voltage_switch(mmc, 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);
>>> + u16 ctrl;
>>> + int ret;
>>> +
>>> + if (host->version < SDHCI_SPEC_300)
>>> + return 0;
>>> +
>>> + switch (ios->signal_voltage) {
>>> + case MMC_SIGNAL_VOLTAGE_330:
>>> + if (!(host->flags & SDHCI_SIGNALING_330))
>>> + return -EINVAL;
>>> +
>>> + ctrl &= ~SDHCI_CTRL_VDD_180;
>>> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>> +
>>> + if (!IS_ERR(mmc->supply.vqmmc)) {
>>> + ret = mmc_regulator_set_vqmmc(mmc, ios);
>>> + if (ret < 0) {
>>> + pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
>>> + mmc_hostname(mmc));
>>> + return -EIO;
>>> + }
>>> + }
>>> +
>>> + usleep_range(5000, 5500);
>>> +
>>> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>> + if (!(ctrl & SDHCI_CTRL_VDD_180))
>>> + return 0;
>>> +
>>> + pr_warn("%s: 3.3V regulator output did not become stable\n",
>>> + mmc_hostname(mmc));
>>> +
>>> + return -EAGAIN;
>>> +
>>> + case MMC_SIGNAL_VOLTAGE_180:
>>> + if (!(host->flags & SDHCI_SIGNALING_180))
>>> + return -EINVAL;
>>> +
>>> + if (!IS_ERR(mmc->supply.vqmmc)) {
>>> + ret = mmc_regulator_set_vqmmc(mmc, ios);
>>> + if (ret < 0) {
>>> + pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
>>> + mmc_hostname(mmc));
>>> + return -EIO;
>>> + }
>>> + }
>>> +
>>> + if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_SET_V1P8_ENA) {
>>> + ctrl |= SDHCI_CTRL_VDD_180;
>>> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>> +
>>> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>> + if (ctrl & SDHCI_CTRL_VDD_180)
>>> + return 0;
>>> +
>>> + pr_warn("%s: 1.8V regulator output did not become stable\n",
>>> + mmc_hostname(mmc));
>>> +
>>> + return -EAGAIN;
>>> + }
>>> + return 0;
>>> +
>>> + default:
>>> + return 0;
>>> + }
>>> +}
>>> +
>>> static u8 sdhci_am654_write_power_on(struct sdhci_host *host, u8 val, int reg)
>>> {
>>> writeb(val, host->ioaddr + reg);
>>> @@ -801,6 +875,8 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>>> struct sdhci_am654_data *sdhci_am654)
>>> {
>>> struct device *dev = &pdev->dev;
>>> + struct device_node *np = dev->of_node;
>>> + struct device_node *node;
>>> int drv_strength;
>>> int ret;
>>> @@ -844,6 +920,15 @@ 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;
>>> + node = of_parse_phandle(np, "vmmc-supply", 0);
>>> +
>>> + if (node) {
>>> + node = of_parse_phandle(np, "vqmmc-supply", 0);
>>> +
>>> + if (!node)
>>> + sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SET_V1P8_ENA;
>>> + }
>>
>> It would be simpler without 'np' and 'node'. Not sure
>> what the rule is meant to be, but it could be something like:
>>
>> if (of_parse_phandle(dev->of_node, "vmmc-supply", 0) &&
>> of_parse_phandle(dev->of_node, "vqmmc-supply", 0)
>> sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
>
> My issue with this is that I also need the quirk (SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) for eMMC. DT node for eMMC does
> not include vmmc and vqmmc supplies. That is why I had the quirk logic
> inverted.
Ideally there would be a more direct way to distinguish eMMC, but
otherwise, having both supplies or neither would be:
if (!!of_parse_phandle(dev->of_node, "vmmc-supply", 0) ==
!!of_parse_phandle(dev->of_node, "vqmmc-supply", 0))
sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
>
> This patch fixes timing issues with both eMMC and SD. (:
>
> ~ Judith
>
>
>>
>>> +
>>> sdhci_get_of_property(pdev);
>>> return 0;
>>> @@ -940,6 +1025,7 @@ static int sdhci_am654_probe(struct platform_device *pdev)
>>> goto err_pltfm_free;
>>> }
>>> + host->mmc_host_ops.start_signal_voltage_switch = sdhci_am654_start_signal_voltage_switch;
>>> host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning;
>>> pm_runtime_get_noresume(dev);
>>>
>>> base-commit: cf6444ba528f043398b112ac36e041a4d8685cb1
>>
>>
>
Hi Adrian,
On 9/10/24 12:10 PM, Adrian Hunter wrote:
> On 10/09/24 17:30, Judith Mendez wrote:
>> Hi Adrian,
>>
>> On 9/9/24 1:26 AM, Adrian Hunter wrote:
>>> On 6/09/24 20:50, Judith Mendez wrote:
>>>> 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 or pos edge of clock.
>>>>
>>>> Due to some eMMC and SD failures seen across am62x platform,
>>>> do not set V1P8_SIGNAL_ENA by default, only enable the bit
>>>> for devices that require this bit in order to switch to 1v8
>>>> voltage for uhs modes.
>>>>
>>>> Signed-off-by: Judith Mendez <jm@ti.com>
>>>> ---
>>>> drivers/mmc/host/sdhci_am654.c | 86 ++++++++++++++++++++++++++++++++++
>>>> 1 file changed, 86 insertions(+)
>>>>
>>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>>>> index 0aa3c40ea6ed8..fb6232e56606b 100644
>>>> --- a/drivers/mmc/host/sdhci_am654.c
>>>> +++ b/drivers/mmc/host/sdhci_am654.c
>>>> @@ -155,6 +155,7 @@ struct sdhci_am654_data {
>>>> u32 tuning_loop;
>>>> #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
>>>> +#define SDHCI_AM654_QUIRK_SET_V1P8_ENA BIT(1)
>>>
>>> It would be better for the quirk to represent the deviation
>>> from normal i.e.
>>>
>>> #define SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA BIT(1)
>>>
>>>> };
>>>> struct window {
>>>> @@ -356,6 +357,79 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
>>>> sdhci_set_clock(host, clock);
>>>> }
>>>> +int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc,
>>>> + struct mmc_ios *ios)
>>>
>>> Simpler to call sdhci_start_signal_voltage_switch() for the normal
>>> case e.g.
>>
>> This is simpler, so sure will use thanks.
>>
>>>
>>> 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);
>>>
>>>
>>> if ((sdhci_am654->quirks & SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) &&
>>> ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
>>> ret = mmc_regulator_set_vqmmc(mmc, ios);
>>> if (ret < 0) {
>>> pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
>>> mmc_hostname(mmc));
>>> return -EIO;
>>> }
>>> return 0;
>>> }
>>>
>>> return sdhci_start_signal_voltage_switch(mmc, 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);
>>>> + u16 ctrl;
>>>> + int ret;
>>>> +
>>>> + if (host->version < SDHCI_SPEC_300)
>>>> + return 0;
>>>> +
>>>> + switch (ios->signal_voltage) {
>>>> + case MMC_SIGNAL_VOLTAGE_330:
>>>> + if (!(host->flags & SDHCI_SIGNALING_330))
>>>> + return -EINVAL;
>>>> +
>>>> + ctrl &= ~SDHCI_CTRL_VDD_180;
>>>> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>> +
>>>> + if (!IS_ERR(mmc->supply.vqmmc)) {
>>>> + ret = mmc_regulator_set_vqmmc(mmc, ios);
>>>> + if (ret < 0) {
>>>> + pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
>>>> + mmc_hostname(mmc));
>>>> + return -EIO;
>>>> + }
>>>> + }
>>>> +
>>>> + usleep_range(5000, 5500);
>>>> +
>>>> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>>> + if (!(ctrl & SDHCI_CTRL_VDD_180))
>>>> + return 0;
>>>> +
>>>> + pr_warn("%s: 3.3V regulator output did not become stable\n",
>>>> + mmc_hostname(mmc));
>>>> +
>>>> + return -EAGAIN;
>>>> +
>>>> + case MMC_SIGNAL_VOLTAGE_180:
>>>> + if (!(host->flags & SDHCI_SIGNALING_180))
>>>> + return -EINVAL;
>>>> +
>>>> + if (!IS_ERR(mmc->supply.vqmmc)) {
>>>> + ret = mmc_regulator_set_vqmmc(mmc, ios);
>>>> + if (ret < 0) {
>>>> + pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
>>>> + mmc_hostname(mmc));
>>>> + return -EIO;
>>>> + }
>>>> + }
>>>> +
>>>> + if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_SET_V1P8_ENA) {
>>>> + ctrl |= SDHCI_CTRL_VDD_180;
>>>> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>> +
>>>> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>>> + if (ctrl & SDHCI_CTRL_VDD_180)
>>>> + return 0;
>>>> +
>>>> + pr_warn("%s: 1.8V regulator output did not become stable\n",
>>>> + mmc_hostname(mmc));
>>>> +
>>>> + return -EAGAIN;
>>>> + }
>>>> + return 0;
>>>> +
>>>> + default:
>>>> + return 0;
>>>> + }
>>>> +}
>>>> +
>>>> static u8 sdhci_am654_write_power_on(struct sdhci_host *host, u8 val, int reg)
>>>> {
>>>> writeb(val, host->ioaddr + reg);
>>>> @@ -801,6 +875,8 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>>>> struct sdhci_am654_data *sdhci_am654)
>>>> {
>>>> struct device *dev = &pdev->dev;
>>>> + struct device_node *np = dev->of_node;
>>>> + struct device_node *node;
>>>> int drv_strength;
>>>> int ret;
>>>> @@ -844,6 +920,15 @@ 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;
>>>> + node = of_parse_phandle(np, "vmmc-supply", 0);
>>>> +
>>>> + if (node) {
>>>> + node = of_parse_phandle(np, "vqmmc-supply", 0);
>>>> +
>>>> + if (!node)
>>>> + sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SET_V1P8_ENA;
>>>> + }
>>>
>>> It would be simpler without 'np' and 'node'. Not sure
>>> what the rule is meant to be, but it could be something like:
>>>
>>> if (of_parse_phandle(dev->of_node, "vmmc-supply", 0) &&
>>> of_parse_phandle(dev->of_node, "vqmmc-supply", 0)
>>> sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
>>
>> My issue with this is that I also need the quirk (SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) for eMMC. DT node for eMMC does
>> not include vmmc and vqmmc supplies. That is why I had the quirk logic
>> inverted.
>
> Ideally there would be a more direct way to distinguish eMMC, but
> otherwise, having both supplies or neither would be:
>
> if (!!of_parse_phandle(dev->of_node, "vmmc-supply", 0) ==
> !!of_parse_phandle(dev->of_node, "vqmmc-supply", 0))
> sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
Not sure I love the double NOT, but ok, I can use this, will fix for v2.
Thanks for your suggestion!
~ Judith
>
>
>>
>> This patch fixes timing issues with both eMMC and SD. (:
>>
>> ~ Judith
>>
>>
>>>
>>>> +
>>>> sdhci_get_of_property(pdev);
>>>> return 0;
>>>> @@ -940,6 +1025,7 @@ static int sdhci_am654_probe(struct platform_device *pdev)
>>>> goto err_pltfm_free;
>>>> }
>>>> + host->mmc_host_ops.start_signal_voltage_switch = sdhci_am654_start_signal_voltage_switch;
>>>> host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning;
>>>> pm_runtime_get_noresume(dev);
>>>>
>>>> base-commit: cf6444ba528f043398b112ac36e041a4d8685cb1
>>>
>>>
>>
>
On 11/09/24 02:22, Judith Mendez wrote:
> Hi Adrian,
>
> On 9/10/24 12:10 PM, Adrian Hunter wrote:
>> On 10/09/24 17:30, Judith Mendez wrote:
>>> Hi Adrian,
>>>
>>> On 9/9/24 1:26 AM, Adrian Hunter wrote:
>>>> On 6/09/24 20:50, Judith Mendez wrote:
>>>>> 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 or pos edge of clock.
>>>>>
>>>>> Due to some eMMC and SD failures seen across am62x platform,
>>>>> do not set V1P8_SIGNAL_ENA by default, only enable the bit
>>>>> for devices that require this bit in order to switch to 1v8
>>>>> voltage for uhs modes.
>>>>>
>>>>> Signed-off-by: Judith Mendez <jm@ti.com>
>>>>> ---
>>>>> drivers/mmc/host/sdhci_am654.c | 86 ++++++++++++++++++++++++++++++++++
>>>>> 1 file changed, 86 insertions(+)
>>>>>
>>>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>>>>> index 0aa3c40ea6ed8..fb6232e56606b 100644
>>>>> --- a/drivers/mmc/host/sdhci_am654.c
>>>>> +++ b/drivers/mmc/host/sdhci_am654.c
>>>>> @@ -155,6 +155,7 @@ struct sdhci_am654_data {
>>>>> u32 tuning_loop;
>>>>> #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
>>>>> +#define SDHCI_AM654_QUIRK_SET_V1P8_ENA BIT(1)
>>>>
>>>> It would be better for the quirk to represent the deviation
>>>> from normal i.e.
>>>>
>>>> #define SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA BIT(1)
>>>>
>>>>> };
>>>>> struct window {
>>>>> @@ -356,6 +357,79 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
>>>>> sdhci_set_clock(host, clock);
>>>>> }
>>>>> +int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc,
>>>>> + struct mmc_ios *ios)
>>>>
>>>> Simpler to call sdhci_start_signal_voltage_switch() for the normal
>>>> case e.g.
>>>
>>> This is simpler, so sure will use thanks.
>>>
>>>>
>>>> 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);
>>>>
>>>>
>>>> if ((sdhci_am654->quirks & SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) &&
>>>> ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
>>>> ret = mmc_regulator_set_vqmmc(mmc, ios);
>>>> if (ret < 0) {
>>>> pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
>>>> mmc_hostname(mmc));
>>>> return -EIO;
>>>> }
>>>> return 0;
>>>> }
>>>>
>>>> return sdhci_start_signal_voltage_switch(mmc, 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);
>>>>> + u16 ctrl;
>>>>> + int ret;
>>>>> +
>>>>> + if (host->version < SDHCI_SPEC_300)
>>>>> + return 0;
>>>>> +
>>>>> + switch (ios->signal_voltage) {
>>>>> + case MMC_SIGNAL_VOLTAGE_330:
>>>>> + if (!(host->flags & SDHCI_SIGNALING_330))
>>>>> + return -EINVAL;
>>>>> +
>>>>> + ctrl &= ~SDHCI_CTRL_VDD_180;
>>>>> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>>> +
>>>>> + if (!IS_ERR(mmc->supply.vqmmc)) {
>>>>> + ret = mmc_regulator_set_vqmmc(mmc, ios);
>>>>> + if (ret < 0) {
>>>>> + pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
>>>>> + mmc_hostname(mmc));
>>>>> + return -EIO;
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + usleep_range(5000, 5500);
>>>>> +
>>>>> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>>>> + if (!(ctrl & SDHCI_CTRL_VDD_180))
>>>>> + return 0;
>>>>> +
>>>>> + pr_warn("%s: 3.3V regulator output did not become stable\n",
>>>>> + mmc_hostname(mmc));
>>>>> +
>>>>> + return -EAGAIN;
>>>>> +
>>>>> + case MMC_SIGNAL_VOLTAGE_180:
>>>>> + if (!(host->flags & SDHCI_SIGNALING_180))
>>>>> + return -EINVAL;
>>>>> +
>>>>> + if (!IS_ERR(mmc->supply.vqmmc)) {
>>>>> + ret = mmc_regulator_set_vqmmc(mmc, ios);
>>>>> + if (ret < 0) {
>>>>> + pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
>>>>> + mmc_hostname(mmc));
>>>>> + return -EIO;
>>>>> + }
>>>>> + }
>>>>> +
>>>>> + if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_SET_V1P8_ENA) {
>>>>> + ctrl |= SDHCI_CTRL_VDD_180;
>>>>> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>>> +
>>>>> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>>>> + if (ctrl & SDHCI_CTRL_VDD_180)
>>>>> + return 0;
>>>>> +
>>>>> + pr_warn("%s: 1.8V regulator output did not become stable\n",
>>>>> + mmc_hostname(mmc));
>>>>> +
>>>>> + return -EAGAIN;
>>>>> + }
>>>>> + return 0;
>>>>> +
>>>>> + default:
>>>>> + return 0;
>>>>> + }
>>>>> +}
>>>>> +
>>>>> static u8 sdhci_am654_write_power_on(struct sdhci_host *host, u8 val, int reg)
>>>>> {
>>>>> writeb(val, host->ioaddr + reg);
>>>>> @@ -801,6 +875,8 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>>>>> struct sdhci_am654_data *sdhci_am654)
>>>>> {
>>>>> struct device *dev = &pdev->dev;
>>>>> + struct device_node *np = dev->of_node;
>>>>> + struct device_node *node;
>>>>> int drv_strength;
>>>>> int ret;
>>>>> @@ -844,6 +920,15 @@ 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;
>>>>> + node = of_parse_phandle(np, "vmmc-supply", 0);
>>>>> +
>>>>> + if (node) {
>>>>> + node = of_parse_phandle(np, "vqmmc-supply", 0);
>>>>> +
>>>>> + if (!node)
>>>>> + sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SET_V1P8_ENA;
>>>>> + }
>>>>
>>>> It would be simpler without 'np' and 'node'. Not sure
>>>> what the rule is meant to be, but it could be something like:
>>>>
>>>> if (of_parse_phandle(dev->of_node, "vmmc-supply", 0) &&
>>>> of_parse_phandle(dev->of_node, "vqmmc-supply", 0)
>>>> sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
>>>
>>> My issue with this is that I also need the quirk (SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) for eMMC. DT node for eMMC does
>>> not include vmmc and vqmmc supplies. That is why I had the quirk logic
>>> inverted.
>>
>> Ideally there would be a more direct way to distinguish eMMC, but
>> otherwise, having both supplies or neither would be:
>>
>> if (!!of_parse_phandle(dev->of_node, "vmmc-supply", 0) ==
>> !!of_parse_phandle(dev->of_node, "vqmmc-supply", 0))
>> sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
>
>
> Not sure I love the double NOT, but ok, I can use this, will fix for v2.
It could use a comment, including about the eMMC thing.
>
> Thanks for your suggestion!
>
> ~ Judith
>
>>
>>
>>>
>>> This patch fixes timing issues with both eMMC and SD. (:
>>>
>>> ~ Judith
>>>
>>>
>>>>
>>>>> +
>>>>> sdhci_get_of_property(pdev);
>>>>> return 0;
>>>>> @@ -940,6 +1025,7 @@ static int sdhci_am654_probe(struct platform_device *pdev)
>>>>> goto err_pltfm_free;
>>>>> }
>>>>> + host->mmc_host_ops.start_signal_voltage_switch = sdhci_am654_start_signal_voltage_switch;
>>>>> host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning;
>>>>> pm_runtime_get_noresume(dev);
>>>>>
>>>>> base-commit: cf6444ba528f043398b112ac36e041a4d8685cb1
>>>>
>>>>
>>>
>>
>
Hi Adrian,
On 9/11/24 12:17 AM, Adrian Hunter wrote:
> On 11/09/24 02:22, Judith Mendez wrote:
>> Hi Adrian,
>>
>> On 9/10/24 12:10 PM, Adrian Hunter wrote:
>>> On 10/09/24 17:30, Judith Mendez wrote:
>>>> Hi Adrian,
>>>>
>>>> On 9/9/24 1:26 AM, Adrian Hunter wrote:
>>>>> On 6/09/24 20:50, Judith Mendez wrote:
>>>>>> 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 or pos edge of clock.
>>>>>>
>>>>>> Due to some eMMC and SD failures seen across am62x platform,
>>>>>> do not set V1P8_SIGNAL_ENA by default, only enable the bit
>>>>>> for devices that require this bit in order to switch to 1v8
>>>>>> voltage for uhs modes.
>>>>>>
>>>>>> Signed-off-by: Judith Mendez <jm@ti.com>
>>>>>> ---
>>>>>> drivers/mmc/host/sdhci_am654.c | 86 ++++++++++++++++++++++++++++++++++
>>>>>> 1 file changed, 86 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
>>>>>> index 0aa3c40ea6ed8..fb6232e56606b 100644
>>>>>> --- a/drivers/mmc/host/sdhci_am654.c
>>>>>> +++ b/drivers/mmc/host/sdhci_am654.c
>>>>>> @@ -155,6 +155,7 @@ struct sdhci_am654_data {
>>>>>> u32 tuning_loop;
>>>>>> #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
>>>>>> +#define SDHCI_AM654_QUIRK_SET_V1P8_ENA BIT(1)
>>>>>
>>>>> It would be better for the quirk to represent the deviation
>>>>> from normal i.e.
>>>>>
>>>>> #define SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA BIT(1)
>>>>>
>>>>>> };
>>>>>> struct window {
>>>>>> @@ -356,6 +357,79 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
>>>>>> sdhci_set_clock(host, clock);
>>>>>> }
>>>>>> +int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc,
>>>>>> + struct mmc_ios *ios)
>>>>>
>>>>> Simpler to call sdhci_start_signal_voltage_switch() for the normal
>>>>> case e.g.
>>>>
>>>> This is simpler, so sure will use thanks.
>>>>
>>>>>
>>>>> 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);
>>>>>
>>>>>
>>>>> if ((sdhci_am654->quirks & SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) &&
>>>>> ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
>>>>> ret = mmc_regulator_set_vqmmc(mmc, ios);
>>>>> if (ret < 0) {
>>>>> pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
>>>>> mmc_hostname(mmc));
>>>>> return -EIO;
>>>>> }
>>>>> return 0;
>>>>> }
>>>>>
>>>>> return sdhci_start_signal_voltage_switch(mmc, 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);
>>>>>> + u16 ctrl;
>>>>>> + int ret;
>>>>>> +
>>>>>> + if (host->version < SDHCI_SPEC_300)
>>>>>> + return 0;
>>>>>> +
>>>>>> + switch (ios->signal_voltage) {
>>>>>> + case MMC_SIGNAL_VOLTAGE_330:
>>>>>> + if (!(host->flags & SDHCI_SIGNALING_330))
>>>>>> + return -EINVAL;
>>>>>> +
>>>>>> + ctrl &= ~SDHCI_CTRL_VDD_180;
>>>>>> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>>>> +
>>>>>> + if (!IS_ERR(mmc->supply.vqmmc)) {
>>>>>> + ret = mmc_regulator_set_vqmmc(mmc, ios);
>>>>>> + if (ret < 0) {
>>>>>> + pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
>>>>>> + mmc_hostname(mmc));
>>>>>> + return -EIO;
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> + usleep_range(5000, 5500);
>>>>>> +
>>>>>> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>>>>> + if (!(ctrl & SDHCI_CTRL_VDD_180))
>>>>>> + return 0;
>>>>>> +
>>>>>> + pr_warn("%s: 3.3V regulator output did not become stable\n",
>>>>>> + mmc_hostname(mmc));
>>>>>> +
>>>>>> + return -EAGAIN;
>>>>>> +
>>>>>> + case MMC_SIGNAL_VOLTAGE_180:
>>>>>> + if (!(host->flags & SDHCI_SIGNALING_180))
>>>>>> + return -EINVAL;
>>>>>> +
>>>>>> + if (!IS_ERR(mmc->supply.vqmmc)) {
>>>>>> + ret = mmc_regulator_set_vqmmc(mmc, ios);
>>>>>> + if (ret < 0) {
>>>>>> + pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
>>>>>> + mmc_hostname(mmc));
>>>>>> + return -EIO;
>>>>>> + }
>>>>>> + }
>>>>>> +
>>>>>> + if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_SET_V1P8_ENA) {
>>>>>> + ctrl |= SDHCI_CTRL_VDD_180;
>>>>>> + sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
>>>>>> +
>>>>>> + ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
>>>>>> + if (ctrl & SDHCI_CTRL_VDD_180)
>>>>>> + return 0;
>>>>>> +
>>>>>> + pr_warn("%s: 1.8V regulator output did not become stable\n",
>>>>>> + mmc_hostname(mmc));
>>>>>> +
>>>>>> + return -EAGAIN;
>>>>>> + }
>>>>>> + return 0;
>>>>>> +
>>>>>> + default:
>>>>>> + return 0;
>>>>>> + }
>>>>>> +}
>>>>>> +
>>>>>> static u8 sdhci_am654_write_power_on(struct sdhci_host *host, u8 val, int reg)
>>>>>> {
>>>>>> writeb(val, host->ioaddr + reg);
>>>>>> @@ -801,6 +875,8 @@ static int sdhci_am654_get_of_property(struct platform_device *pdev,
>>>>>> struct sdhci_am654_data *sdhci_am654)
>>>>>> {
>>>>>> struct device *dev = &pdev->dev;
>>>>>> + struct device_node *np = dev->of_node;
>>>>>> + struct device_node *node;
>>>>>> int drv_strength;
>>>>>> int ret;
>>>>>> @@ -844,6 +920,15 @@ 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;
>>>>>> + node = of_parse_phandle(np, "vmmc-supply", 0);
>>>>>> +
>>>>>> + if (node) {
>>>>>> + node = of_parse_phandle(np, "vqmmc-supply", 0);
>>>>>> +
>>>>>> + if (!node)
>>>>>> + sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SET_V1P8_ENA;
>>>>>> + }
>>>>>
>>>>> It would be simpler without 'np' and 'node'. Not sure
>>>>> what the rule is meant to be, but it could be something like:
>>>>>
>>>>> if (of_parse_phandle(dev->of_node, "vmmc-supply", 0) &&
>>>>> of_parse_phandle(dev->of_node, "vqmmc-supply", 0)
>>>>> sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
>>>>
>>>> My issue with this is that I also need the quirk (SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) for eMMC. DT node for eMMC does
>>>> not include vmmc and vqmmc supplies. That is why I had the quirk logic
>>>> inverted.
>>>
>>> Ideally there would be a more direct way to distinguish eMMC, but
>>> otherwise, having both supplies or neither would be:
>>>
>>> if (!!of_parse_phandle(dev->of_node, "vmmc-supply", 0) ==
>>> !!of_parse_phandle(dev->of_node, "vqmmc-supply", 0))
>>> sdhci_am654->quirks |= SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA;
>>
>>
>> Not sure I love the double NOT, but ok, I can use this, will fix for v2.
>
> It could use a comment, including about the eMMC thing.
Sure, will add. Thanks.
>
>>
>> Thanks for your suggestion!
>>
>> ~ Judith
>>
>>>
>>>
>>>>
>>>> This patch fixes timing issues with both eMMC and SD. (:
>>>>
>>>> ~ Judith
>>>>
>>>>
>>>>>
>>>>>> +
>>>>>> sdhci_get_of_property(pdev);
>>>>>> return 0;
>>>>>> @@ -940,6 +1025,7 @@ static int sdhci_am654_probe(struct platform_device *pdev)
>>>>>> goto err_pltfm_free;
>>>>>> }
>>>>>> + host->mmc_host_ops.start_signal_voltage_switch = sdhci_am654_start_signal_voltage_switch;
>>>>>> host->mmc_host_ops.execute_tuning = sdhci_am654_execute_tuning;
>>>>>> pm_runtime_get_noresume(dev);
>>>>>>
>>>>>> base-commit: cf6444ba528f043398b112ac36e041a4d8685cb1
>>>>>
>>>>>
>>>>
>>>
>>
>
Hi Judith,
kernel test robot noticed the following build warnings:
[auto build test WARNING on cf6444ba528f043398b112ac36e041a4d8685cb1]
url: https://github.com/intel-lab-lkp/linux/commits/Judith-Mendez/mmc-sdhci_am654-Add-sdhci_am654_start_signal_voltage_switch/20240907-015144
base: cf6444ba528f043398b112ac36e041a4d8685cb1
patch link: https://lore.kernel.org/r/20240906175032.1580281-1-jm%40ti.com
patch subject: [PATCH v1] mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch
config: x86_64-allyesconfig (https://download.01.org/0day-ci/archive/20240908/202409080031.zgsuKKXB-lkp@intel.com/config)
compiler: clang version 18.1.5 (https://github.com/llvm/llvm-project 617a15a9eac96088ae5e9134248d8236e34b91b1)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240908/202409080031.zgsuKKXB-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409080031.zgsuKKXB-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/mmc/host/sdhci_am654.c:360:5: warning: no previous prototype for function 'sdhci_am654_start_signal_voltage_switch' [-Wmissing-prototypes]
360 | int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc,
| ^
drivers/mmc/host/sdhci_am654.c:360:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
360 | int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc,
| ^
| static
>> drivers/mmc/host/sdhci_am654.c:377:3: warning: variable 'ctrl' is uninitialized when used here [-Wuninitialized]
377 | ctrl &= ~SDHCI_CTRL_VDD_180;
| ^~~~
drivers/mmc/host/sdhci_am654.c:366:10: note: initialize the variable 'ctrl' to silence this warning
366 | u16 ctrl;
| ^
| = 0
2 warnings generated.
vim +/sdhci_am654_start_signal_voltage_switch +360 drivers/mmc/host/sdhci_am654.c
359
> 360 int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc,
361 struct mmc_ios *ios)
362 {
363 struct sdhci_host *host = mmc_priv(mmc);
364 struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
365 struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
366 u16 ctrl;
367 int ret;
368
369 if (host->version < SDHCI_SPEC_300)
370 return 0;
371
372 switch (ios->signal_voltage) {
373 case MMC_SIGNAL_VOLTAGE_330:
374 if (!(host->flags & SDHCI_SIGNALING_330))
375 return -EINVAL;
376
> 377 ctrl &= ~SDHCI_CTRL_VDD_180;
378 sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
379
380 if (!IS_ERR(mmc->supply.vqmmc)) {
381 ret = mmc_regulator_set_vqmmc(mmc, ios);
382 if (ret < 0) {
383 pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
384 mmc_hostname(mmc));
385 return -EIO;
386 }
387 }
388
389 usleep_range(5000, 5500);
390
391 ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
392 if (!(ctrl & SDHCI_CTRL_VDD_180))
393 return 0;
394
395 pr_warn("%s: 3.3V regulator output did not become stable\n",
396 mmc_hostname(mmc));
397
398 return -EAGAIN;
399
400 case MMC_SIGNAL_VOLTAGE_180:
401 if (!(host->flags & SDHCI_SIGNALING_180))
402 return -EINVAL;
403
404 if (!IS_ERR(mmc->supply.vqmmc)) {
405 ret = mmc_regulator_set_vqmmc(mmc, ios);
406 if (ret < 0) {
407 pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
408 mmc_hostname(mmc));
409 return -EIO;
410 }
411 }
412
413 if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_SET_V1P8_ENA) {
414 ctrl |= SDHCI_CTRL_VDD_180;
415 sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
416
417 ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
418 if (ctrl & SDHCI_CTRL_VDD_180)
419 return 0;
420
421 pr_warn("%s: 1.8V regulator output did not become stable\n",
422 mmc_hostname(mmc));
423
424 return -EAGAIN;
425 }
426 return 0;
427
428 default:
429 return 0;
430 }
431 }
432
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
Hi Judith,
kernel test robot noticed the following build warnings:
[auto build test WARNING on cf6444ba528f043398b112ac36e041a4d8685cb1]
url: https://github.com/intel-lab-lkp/linux/commits/Judith-Mendez/mmc-sdhci_am654-Add-sdhci_am654_start_signal_voltage_switch/20240907-015144
base: cf6444ba528f043398b112ac36e041a4d8685cb1
patch link: https://lore.kernel.org/r/20240906175032.1580281-1-jm%40ti.com
patch subject: [PATCH v1] mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch
config: openrisc-allyesconfig (https://download.01.org/0day-ci/archive/20240907/202409072350.685m24kB-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 14.1.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240907/202409072350.685m24kB-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202409072350.685m24kB-lkp@intel.com/
All warnings (new ones prefixed by >>):
>> drivers/mmc/host/sdhci_am654.c:360:5: warning: no previous prototype for 'sdhci_am654_start_signal_voltage_switch' [-Wmissing-prototypes]
360 | int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/sdhci_am654_start_signal_voltage_switch +360 drivers/mmc/host/sdhci_am654.c
359
> 360 int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc,
361 struct mmc_ios *ios)
362 {
363 struct sdhci_host *host = mmc_priv(mmc);
364 struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
365 struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
366 u16 ctrl;
367 int ret;
368
369 if (host->version < SDHCI_SPEC_300)
370 return 0;
371
372 switch (ios->signal_voltage) {
373 case MMC_SIGNAL_VOLTAGE_330:
374 if (!(host->flags & SDHCI_SIGNALING_330))
375 return -EINVAL;
376
377 ctrl &= ~SDHCI_CTRL_VDD_180;
378 sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
379
380 if (!IS_ERR(mmc->supply.vqmmc)) {
381 ret = mmc_regulator_set_vqmmc(mmc, ios);
382 if (ret < 0) {
383 pr_warn("%s: Switching to 3.3V signalling voltage failed\n",
384 mmc_hostname(mmc));
385 return -EIO;
386 }
387 }
388
389 usleep_range(5000, 5500);
390
391 ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
392 if (!(ctrl & SDHCI_CTRL_VDD_180))
393 return 0;
394
395 pr_warn("%s: 3.3V regulator output did not become stable\n",
396 mmc_hostname(mmc));
397
398 return -EAGAIN;
399
400 case MMC_SIGNAL_VOLTAGE_180:
401 if (!(host->flags & SDHCI_SIGNALING_180))
402 return -EINVAL;
403
404 if (!IS_ERR(mmc->supply.vqmmc)) {
405 ret = mmc_regulator_set_vqmmc(mmc, ios);
406 if (ret < 0) {
407 pr_warn("%s: Switching to 1.8V signalling voltage failed\n",
408 mmc_hostname(mmc));
409 return -EIO;
410 }
411 }
412
413 if (sdhci_am654->quirks & SDHCI_AM654_QUIRK_SET_V1P8_ENA) {
414 ctrl |= SDHCI_CTRL_VDD_180;
415 sdhci_writew(host, ctrl, SDHCI_HOST_CONTROL2);
416
417 ctrl = sdhci_readw(host, SDHCI_HOST_CONTROL2);
418 if (ctrl & SDHCI_CTRL_VDD_180)
419 return 0;
420
421 pr_warn("%s: 1.8V regulator output did not become stable\n",
422 mmc_hostname(mmc));
423
424 return -EAGAIN;
425 }
426 return 0;
427
428 default:
429 return 0;
430 }
431 }
432
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
© 2016 - 2024 Red Hat, Inc.