diff mbox series

[v2] Revert "mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch"

Message ID 20250127-am654-mmc-regression-v2-1-9bb39fb12810@solid-run.com
State New
Headers show
Series [v2] Revert "mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch" | expand

Commit Message

Josua Mayer Jan. 27, 2025, 8:12 p.m. UTC
This reverts commit 941a7abd4666912b84ab209396fdb54b0dae685d.

This commit uses presence of device-tree properties vmmc-supply and
vqmmc-supply for deciding whether to enable a quirk affecting timing of
clock and data.
The intention was to address issues observed with eMMC and SD on AM62
platforms.

This new quirk is however also enabled for AM64 breaking microSD access
on the SolidRun HimmingBoard-T which is supported in-tree since v6.11,
causing a regression. During boot microSD initialization now fails with
the error below:

[    2.008520] mmc1: SDHCI controller on fa00000.mmc [fa00000.mmc] using ADMA 64-bit
[    2.115348] mmc1: error -110 whilst initialising SD card

The heuristics for enabling the quirk are clearly not correct as they
break at least one but potentially many existing boards.

Revert the change and restore original behaviour until a more
appropriate method of selecting the quirk is derived.

Fixes: 941a7abd4666 ("mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch")
Closes: https://lore.kernel.org/linux-mmc/a70fc9fc-186f-4165-a652-3de50733763a@solid-run.com/
Cc: stable@vger.kernel.org
Signed-off-by: Josua Mayer <josua@solid-run.com>
Acked-by: Adrian Hunter <adrian.hunter@intel.com>
---
Changes in v2:
- Fixed "Fixes:" tag invalid commit description copied from history
  (Reported-by: Adrian Hunter <adrian.hunter@intel.com>)
  (Reported-by: Greg KH <gregkh@linuxfoundation.org>)
- Link to v1: https://lore.kernel.org/r/20250127-am654-mmc-regression-v1-1-d831f9a13ae9@solid-run.com
---
 drivers/mmc/host/sdhci_am654.c | 30 ------------------------------
 1 file changed, 30 deletions(-)


---
base-commit: ffd294d346d185b70e28b1a28abe367bbfe53c04
change-id: 20250127-am654-mmc-regression-ed289f8967c2

Best regards,

Comments

Ulf Hansson Feb. 3, 2025, 2:04 p.m. UTC | #1
On Mon, 27 Jan 2025 at 21:12, Josua Mayer <josua@solid-run.com> wrote:
>
> This reverts commit 941a7abd4666912b84ab209396fdb54b0dae685d.
>
> This commit uses presence of device-tree properties vmmc-supply and
> vqmmc-supply for deciding whether to enable a quirk affecting timing of
> clock and data.
> The intention was to address issues observed with eMMC and SD on AM62
> platforms.
>
> This new quirk is however also enabled for AM64 breaking microSD access
> on the SolidRun HimmingBoard-T which is supported in-tree since v6.11,
> causing a regression. During boot microSD initialization now fails with
> the error below:
>
> [    2.008520] mmc1: SDHCI controller on fa00000.mmc [fa00000.mmc] using ADMA 64-bit
> [    2.115348] mmc1: error -110 whilst initialising SD card
>
> The heuristics for enabling the quirk are clearly not correct as they
> break at least one but potentially many existing boards.
>
> Revert the change and restore original behaviour until a more
> appropriate method of selecting the quirk is derived.
>
> Fixes: 941a7abd4666 ("mmc: sdhci_am654: Add sdhci_am654_start_signal_voltage_switch")
> Closes: https://lore.kernel.org/linux-mmc/a70fc9fc-186f-4165-a652-3de50733763a@solid-run.com/
> Cc: stable@vger.kernel.org
> Signed-off-by: Josua Mayer <josua@solid-run.com>
> Acked-by: Adrian Hunter <adrian.hunter@intel.com>

Applied this for fixes, thanks!

Kind regards
Uffe



> ---
> Changes in v2:
> - Fixed "Fixes:" tag invalid commit description copied from history
>   (Reported-by: Adrian Hunter <adrian.hunter@intel.com>)
>   (Reported-by: Greg KH <gregkh@linuxfoundation.org>)
> - Link to v1: https://lore.kernel.org/r/20250127-am654-mmc-regression-v1-1-d831f9a13ae9@solid-run.com
> ---
>  drivers/mmc/host/sdhci_am654.c | 30 ------------------------------
>  1 file changed, 30 deletions(-)
>
> diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
> index b73f673db92bbc042392995e715815e15ace6005..f75c31815ab00d17b5757063521f56ba5643babe 100644
> --- a/drivers/mmc/host/sdhci_am654.c
> +++ b/drivers/mmc/host/sdhci_am654.c
> @@ -155,7 +155,6 @@ struct sdhci_am654_data {
>         u32 tuning_loop;
>
>  #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
> -#define SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA BIT(1)
>  };
>
>  struct window {
> @@ -357,29 +356,6 @@ static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
>         sdhci_set_clock(host, clock);
>  }
>
> -static int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios)
> -{
> -       struct sdhci_host *host = mmc_priv(mmc);
> -       struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> -       struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
> -       int ret;
> -
> -       if ((sdhci_am654->quirks & SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) &&
> -           ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
> -               if (!IS_ERR(mmc->supply.vqmmc)) {
> -                       ret = mmc_regulator_set_vqmmc(mmc, ios);
> -                       if (ret < 0) {
> -                               pr_err("%s: Switching to 1.8V signalling voltage failed,\n",
> -                                      mmc_hostname(mmc));
> -                               return -EIO;
> -                       }
> -               }
> -               return 0;
> -       }
> -
> -       return sdhci_start_signal_voltage_switch(mmc, ios);
> -}
> -
>  static u8 sdhci_am654_write_power_on(struct sdhci_host *host, u8 val, int reg)
>  {
>         writeb(val, host->ioaddr + reg);
> @@ -868,11 +844,6 @@ 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 ena for eMMC and SD with vqmmc supply */
> -       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;
> @@ -969,7 +940,6 @@ 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: ffd294d346d185b70e28b1a28abe367bbfe53c04
> change-id: 20250127-am654-mmc-regression-ed289f8967c2
>
> Best regards,
> --
> Josua Mayer <josua@solid-run.com>
>
Judith Mendez Feb. 5, 2025, 7:39 p.m. UTC | #2
Hi all,

On 1/27/25 2:12 PM, Josua Mayer wrote:
> This reverts commit 941a7abd4666912b84ab209396fdb54b0dae685d.
> 
> This commit uses presence of device-tree properties vmmc-supply and
> vqmmc-supply for deciding whether to enable a quirk affecting timing of
> clock and data.
> The intention was to address issues observed with eMMC and SD on AM62
> platforms.
> 
> This new quirk is however also enabled for AM64 breaking microSD access
> on the SolidRun HimmingBoard-T which is supported in-tree since v6.11,
> causing a regression. During boot microSD initialization now fails with
> the error below:
> 
> [    2.008520] mmc1: SDHCI controller on fa00000.mmc [fa00000.mmc] using ADMA 64-bit
> [    2.115348] mmc1: error -110 whilst initialising SD card
> 
> The heuristics for enabling the quirk are clearly not correct as they
> break at least one but potentially many existing boards.
> 
> Revert the change and restore original behaviour until a more
> appropriate method of selecting the quirk is derived.


Somehow I missed these emails, apologies.

Thanks for reporting this issue Josua.

We do need this patch for am62x devices since it fixes timing issues
with a variety of SD cards on those boards, but if there is a
regression, too bad, patch had to be reverted.

I will look again into how to implement this quirk, I think using the
voltage regulator nodes to discover if we need this quirk might not have
been a good idea, based on your explanation. I believe I did test the
patch on am64x SK and am64x EVM boards and saw no boot issue there,
so the issue seems related to the voltage regulator nodes existing in DT
(the heuristics for enabling the quirk) as you call it.

Again, thanks for reporting, will look into fixing this issue for am62x
again soon.

~ Judith
Sverdlin, Alexander March 19, 2025, 10:22 a.m. UTC | #3
Hi Judith, Ulf,

On Wed, 2025-02-05 at 13:39 -0600, Judith Mendez wrote:
> Hi all,
> 
> On 1/27/25 2:12 PM, Josua Mayer wrote:
> > This reverts commit 941a7abd4666912b84ab209396fdb54b0dae685d.
> > 
> > This commit uses presence of device-tree properties vmmc-supply and
> > vqmmc-supply for deciding whether to enable a quirk affecting timing of
> > clock and data.
> > The intention was to address issues observed with eMMC and SD on AM62
> > platforms.
> > 
> > This new quirk is however also enabled for AM64 breaking microSD access
> > on the SolidRun HimmingBoard-T which is supported in-tree since v6.11,
> > causing a regression. During boot microSD initialization now fails with
> > the error below:
> > 
> > [    2.008520] mmc1: SDHCI controller on fa00000.mmc [fa00000.mmc] using ADMA 64-bit
> > [    2.115348] mmc1: error -110 whilst initialising SD card
> > 
> > The heuristics for enabling the quirk are clearly not correct as they
> > break at least one but potentially many existing boards.
> > 
> > Revert the change and restore original behaviour until a more
> > appropriate method of selecting the quirk is derived.
> 
> 
> Somehow I missed these emails, apologies.
> 
> Thanks for reporting this issue Josua.
> 
> We do need this patch for am62x devices since it fixes timing issues
> with a variety of SD cards on those boards, but if there is a
> regression, too bad, patch had to be reverted.
> 
> I will look again into how to implement this quirk, I think using the
> voltage regulator nodes to discover if we need this quirk might not have
> been a good idea, based on your explanation. I believe I did test the
> patch on am64x SK and am64x EVM boards and saw no boot issue there,
> so the issue seems related to the voltage regulator nodes existing in DT
> (the heuristics for enabling the quirk) as you call it.
> 
> Again, thanks for reporting, will look into fixing this issue for am62x
> again soon.

does it mean, that 14afef2333af
("arm64: dts: ti: k3-am62-main: Update otap/itap values") has to be reverted
as well, for the time being?
Judith Mendez March 19, 2025, 4:25 p.m. UTC | #4
Hi, Alexander,

On 3/19/25 5:22 AM, Sverdlin, Alexander wrote:
> Hi Judith, Ulf,
> 
> On Wed, 2025-02-05 at 13:39 -0600, Judith Mendez wrote:
>> Hi all,
>>
>> On 1/27/25 2:12 PM, Josua Mayer wrote:
>>> This reverts commit 941a7abd4666912b84ab209396fdb54b0dae685d.
>>>
>>> This commit uses presence of device-tree properties vmmc-supply and
>>> vqmmc-supply for deciding whether to enable a quirk affecting timing of
>>> clock and data.
>>> The intention was to address issues observed with eMMC and SD on AM62
>>> platforms.
>>>
>>> This new quirk is however also enabled for AM64 breaking microSD access
>>> on the SolidRun HimmingBoard-T which is supported in-tree since v6.11,
>>> causing a regression. During boot microSD initialization now fails with
>>> the error below:
>>>
>>> [    2.008520] mmc1: SDHCI controller on fa00000.mmc [fa00000.mmc] using ADMA 64-bit
>>> [    2.115348] mmc1: error -110 whilst initialising SD card
>>>
>>> The heuristics for enabling the quirk are clearly not correct as they
>>> break at least one but potentially many existing boards.
>>>
>>> Revert the change and restore original behaviour until a more
>>> appropriate method of selecting the quirk is derived.
>>
>>
>> Somehow I missed these emails, apologies.
>>
>> Thanks for reporting this issue Josua.
>>
>> We do need this patch for am62x devices since it fixes timing issues
>> with a variety of SD cards on those boards, but if there is a
>> regression, too bad, patch had to be reverted.
>>
>> I will look again into how to implement this quirk, I think using the
>> voltage regulator nodes to discover if we need this quirk might not have
>> been a good idea, based on your explanation. I believe I did test the
>> patch on am64x SK and am64x EVM boards and saw no boot issue there,
>> so the issue seems related to the voltage regulator nodes existing in DT
>> (the heuristics for enabling the quirk) as you call it.
>>
>> Again, thanks for reporting, will look into fixing this issue for am62x
>> again soon.
> 
> does it mean, that 14afef2333af
> ("arm64: dts: ti: k3-am62-main: Update otap/itap values") has to be reverted
> as well, for the time being?

So sorry for the delay in response.

Does this fix: ("arm64: dts: ti: k3-am62-main: Update otap/itap values")
cause any issues for you?

The otap/itap fix is actually setting tap settings according to the
device datasheet since they were wrong in the first place.

The values in the datasheet are the optimal tap settings for our
boards based off of bench characterization results. If these values
provide issues for you, please let me know.


Changing topic:

Going back to the reverted patch. What the patch does is that it
tries to switch data launch from the rising clock edge to the
falling clock edge if we find two voltage supplies for SD/SDIO, one
for powering the SD/SDIO and another for IO voltage switch, or for
the case that no voltage supplies exist (eMMC).

(this was based off-of some internal debug that resulted with a
request to unset V1P8_SIGNAL_ENA to fix timing issues)

However, if you had one voltage supply, the patch should not have
affected you at all and I am really confused why you see an issue
downstream with only one voltage supply.

That being said, I have dug up more information on V1P8_SIGNAL_ENA.
If HIGH_SPEED_ENA is set or if V1P8_SIGNAL_ENA is set, these two bits
are OR'd and if any of the two is set, then data launch always happens
on rising clock edge. This should be the case for any of the UHS modes
or > mmc_hs mode for MMC.

If this is true, we should be setting HIGH_SPEED_ENA anyways for UHS
modes and thus this patch should have done nothing in this sense, since
data launch should still be happening on the rising clock edge.

I am still digging up more information to make sure disabling
V1P8_SIGNAL_ENA has no other implications.


~ Judith
Judith Mendez March 19, 2025, 5:07 p.m. UTC | #5
Hi, Alexander, Joshua,


Let me correct who I direct my questions/responses for.

On 3/19/25 11:25 AM, Judith Mendez wrote:
> Hi, Alexander,
> 
> On 3/19/25 5:22 AM, Sverdlin, Alexander wrote:
>> Hi Judith, Ulf,
>>
>> On Wed, 2025-02-05 at 13:39 -0600, Judith Mendez wrote:
>>> Hi all,
>>>
>>> On 1/27/25 2:12 PM, Josua Mayer wrote:
>>>> This reverts commit 941a7abd4666912b84ab209396fdb54b0dae685d.
>>>>
>>>> This commit uses presence of device-tree properties vmmc-supply and
>>>> vqmmc-supply for deciding whether to enable a quirk affecting timing of
>>>> clock and data.
>>>> The intention was to address issues observed with eMMC and SD on AM62
>>>> platforms.
>>>>
>>>> This new quirk is however also enabled for AM64 breaking microSD access
>>>> on the SolidRun HimmingBoard-T which is supported in-tree since v6.11,
>>>> causing a regression. During boot microSD initialization now fails with
>>>> the error below:
>>>>
>>>> [    2.008520] mmc1: SDHCI controller on fa00000.mmc [fa00000.mmc] 
>>>> using ADMA 64-bit
>>>> [    2.115348] mmc1: error -110 whilst initialising SD card
>>>>
>>>> The heuristics for enabling the quirk are clearly not correct as they
>>>> break at least one but potentially many existing boards.
>>>>
>>>> Revert the change and restore original behaviour until a more
>>>> appropriate method of selecting the quirk is derived.
>>>
>>>
>>> Somehow I missed these emails, apologies.
>>>
>>> Thanks for reporting this issue Josua.
>>>
>>> We do need this patch for am62x devices since it fixes timing issues
>>> with a variety of SD cards on those boards, but if there is a
>>> regression, too bad, patch had to be reverted.
>>>
>>> I will look again into how to implement this quirk, I think using the
>>> voltage regulator nodes to discover if we need this quirk might not have
>>> been a good idea, based on your explanation. I believe I did test the
>>> patch on am64x SK and am64x EVM boards and saw no boot issue there,
>>> so the issue seems related to the voltage regulator nodes existing in DT
>>> (the heuristics for enabling the quirk) as you call it.
>>>
>>> Again, thanks for reporting, will look into fixing this issue for am62x
>>> again soon.
>>
>> does it mean, that 14afef2333af
>> ("arm64: dts: ti: k3-am62-main: Update otap/itap values") has to be 
>> reverted
>> as well, for the time being?
> 
> So sorry for the delay in response.
> 
> Does this fix: ("arm64: dts: ti: k3-am62-main: Update otap/itap values")
> cause any issues for you?
> 
> The otap/itap fix is actually setting tap settings according to the
> device datasheet since they were wrong in the first place.
> 
> The values in the datasheet are the optimal tap settings for our
> boards based off of bench characterization results. If these values
> provide issues for you, please let me know.

This first part is for Alexander.

> 
> 
> Changing topic:
> 
> Going back to the reverted patch. What the patch does is that it
> tries to switch data launch from the rising clock edge to the
> falling clock edge if we find two voltage supplies for SD/SDIO, one
> for powering the SD/SDIO and another for IO voltage switch, or for
> the case that no voltage supplies exist (eMMC).
> 
> (this was based off-of some internal debug that resulted with a
> request to unset V1P8_SIGNAL_ENA to fix timing issues)
> 
> However, if you had one voltage supply, the patch should not have
> affected you at all and I am really confused why you see an issue
> downstream with only one voltage supply.
> 
> That being said, I have dug up more information on V1P8_SIGNAL_ENA.
> If HIGH_SPEED_ENA is set or if V1P8_SIGNAL_ENA is set, these two bits
> are OR'd and if any of the two is set, then data launch always happens
> on rising clock edge. This should be the case for any of the UHS modes
> or > mmc_hs mode for MMC.
> 
> If this is true, we should be setting HIGH_SPEED_ENA anyways for UHS
> modes and thus this patch should have done nothing in this sense, since
> data launch should still be happening on the rising clock edge.
> 
> I am still digging up more information to make sure disabling
> V1P8_SIGNAL_ENA has no other implications.
> 

This second part is to Joshua.

> 
> ~ Judith
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
> 
>
Sverdlin, Alexander March 19, 2025, 5:19 p.m. UTC | #6
Hi Judith,

On Wed, 2025-03-19 at 11:25 -0500, Judith Mendez wrote:
> > > > This reverts commit 941a7abd4666912b84ab209396fdb54b0dae685d.
> > > > 
> > > > This commit uses presence of device-tree properties vmmc-supply and
> > > > vqmmc-supply for deciding whether to enable a quirk affecting timing of
> > > > clock and data.
> > > > The intention was to address issues observed with eMMC and SD on AM62
> > > > platforms.
> > > > 
> > > > This new quirk is however also enabled for AM64 breaking microSD access
> > > > on the SolidRun HimmingBoard-T which is supported in-tree since v6.11,
> > > > causing a regression. During boot microSD initialization now fails with
> > > > the error below:
> > > > 
> > > > [    2.008520] mmc1: SDHCI controller on fa00000.mmc [fa00000.mmc] using ADMA 64-bit
> > > > [    2.115348] mmc1: error -110 whilst initialising SD card
> > > > 
> > > > The heuristics for enabling the quirk are clearly not correct as they
> > > > break at least one but potentially many existing boards.
> > > > 
> > > > Revert the change and restore original behaviour until a more
> > > > appropriate method of selecting the quirk is derived.
> > > 
> > > 
> > > Somehow I missed these emails, apologies.
> > > 
> > > Thanks for reporting this issue Josua.
> > > 
> > > We do need this patch for am62x devices since it fixes timing issues
> > > with a variety of SD cards on those boards, but if there is a
> > > regression, too bad, patch had to be reverted.
> > > 
> > > I will look again into how to implement this quirk, I think using the
> > > voltage regulator nodes to discover if we need this quirk might not have
> > > been a good idea, based on your explanation. I believe I did test the
> > > patch on am64x SK and am64x EVM boards and saw no boot issue there,
> > > so the issue seems related to the voltage regulator nodes existing in DT
> > > (the heuristics for enabling the quirk) as you call it.
> > > 
> > > Again, thanks for reporting, will look into fixing this issue for am62x
> > > again soon.
> > 
> > does it mean, that 14afef2333af
> > ("arm64: dts: ti: k3-am62-main: Update otap/itap values") has to be reverted
> > as well, for the time being?
> 
> So sorry for the delay in response.
> 
> Does this fix: ("arm64: dts: ti: k3-am62-main: Update otap/itap values")
> cause any issues for you?
> 
> The otap/itap fix is actually setting tap settings according to the
> device datasheet since they were wrong in the first place.
> 
> The values in the datasheet are the optimal tap settings for our
> boards based off of bench characterization results. If these values
> provide issues for you, please let me know.

I've just noticed that 14afef2333af mentioned the reverted 941a7abd4666
in a way that one may think of it as a dependency:
---
    Now that we have fixed timing issues for am62x [1], lets
    change the otap/itap values back according to the device
    datasheet.
    
    [1] https://lore.kernel.org/linux-mmc/20240913185403.1339115-1-jm@ti.com/
---

that why I wanted to double check with you. But if you say they are actually
independent, that's fine for me!
Judith Mendez March 19, 2025, 5:27 p.m. UTC | #7
Hi Alexander,

On 3/19/25 12:19 PM, Sverdlin, Alexander wrote:
> Hi Judith,
> 
> On Wed, 2025-03-19 at 11:25 -0500, Judith Mendez wrote:
>>>>> This reverts commit 941a7abd4666912b84ab209396fdb54b0dae685d.
>>>>>
>>>>> This commit uses presence of device-tree properties vmmc-supply and
>>>>> vqmmc-supply for deciding whether to enable a quirk affecting timing of
>>>>> clock and data.
>>>>> The intention was to address issues observed with eMMC and SD on AM62
>>>>> platforms.
>>>>>
>>>>> This new quirk is however also enabled for AM64 breaking microSD access
>>>>> on the SolidRun HimmingBoard-T which is supported in-tree since v6.11,
>>>>> causing a regression. During boot microSD initialization now fails with
>>>>> the error below:
>>>>>
>>>>> [    2.008520] mmc1: SDHCI controller on fa00000.mmc [fa00000.mmc] using ADMA 64-bit
>>>>> [    2.115348] mmc1: error -110 whilst initialising SD card
>>>>>
>>>>> The heuristics for enabling the quirk are clearly not correct as they
>>>>> break at least one but potentially many existing boards.
>>>>>
>>>>> Revert the change and restore original behaviour until a more
>>>>> appropriate method of selecting the quirk is derived.
>>>>
>>>>
>>>> Somehow I missed these emails, apologies.
>>>>
>>>> Thanks for reporting this issue Josua.
>>>>
>>>> We do need this patch for am62x devices since it fixes timing issues
>>>> with a variety of SD cards on those boards, but if there is a
>>>> regression, too bad, patch had to be reverted.
>>>>
>>>> I will look again into how to implement this quirk, I think using the
>>>> voltage regulator nodes to discover if we need this quirk might not have
>>>> been a good idea, based on your explanation. I believe I did test the
>>>> patch on am64x SK and am64x EVM boards and saw no boot issue there,
>>>> so the issue seems related to the voltage regulator nodes existing in DT
>>>> (the heuristics for enabling the quirk) as you call it.
>>>>
>>>> Again, thanks for reporting, will look into fixing this issue for am62x
>>>> again soon.
>>>
>>> does it mean, that 14afef2333af
>>> ("arm64: dts: ti: k3-am62-main: Update otap/itap values") has to be reverted
>>> as well, for the time being?
>>
>> So sorry for the delay in response.
>>
>> Does this fix: ("arm64: dts: ti: k3-am62-main: Update otap/itap values")
>> cause any issues for you?
>>
>> The otap/itap fix is actually setting tap settings according to the
>> device datasheet since they were wrong in the first place.
>>
>> The values in the datasheet are the optimal tap settings for our
>> boards based off of bench characterization results. If these values
>> provide issues for you, please let me know.
> 
> I've just noticed that 14afef2333af mentioned the reverted 941a7abd4666
> in a way that one may think of it as a dependency:
> ---
>      Now that we have fixed timing issues for am62x [1], lets
>      change the otap/itap values back according to the device
>      datasheet.
>      
>      [1] https://lore.kernel.org/linux-mmc/20240913185403.1339115-1-jm@ti.com/
> ---
> 
> that why I wanted to double check with you. But if you say they are actually
> independent, that's fine for me!
> 

Well actually, the reason why they were not fixed correctly in the first
place is because we had timing issues on am62x devices. Since we had now
"fixed" those timing issues with this patch and some other similar
patches in the host controller driver. I went ahead and fixed the tap
settings as per characterization results.

The current setting are supposed to be the final and best/correct
settings, but again, if you see any obvious issues, please let me know.

~ Judith
diff mbox series

Patch

diff --git a/drivers/mmc/host/sdhci_am654.c b/drivers/mmc/host/sdhci_am654.c
index b73f673db92bbc042392995e715815e15ace6005..f75c31815ab00d17b5757063521f56ba5643babe 100644
--- a/drivers/mmc/host/sdhci_am654.c
+++ b/drivers/mmc/host/sdhci_am654.c
@@ -155,7 +155,6 @@  struct sdhci_am654_data {
 	u32 tuning_loop;
 
 #define SDHCI_AM654_QUIRK_FORCE_CDTEST BIT(0)
-#define SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA BIT(1)
 };
 
 struct window {
@@ -357,29 +356,6 @@  static void sdhci_j721e_4bit_set_clock(struct sdhci_host *host,
 	sdhci_set_clock(host, clock);
 }
 
-static int sdhci_am654_start_signal_voltage_switch(struct mmc_host *mmc, struct mmc_ios *ios)
-{
-	struct sdhci_host *host = mmc_priv(mmc);
-	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
-	struct sdhci_am654_data *sdhci_am654 = sdhci_pltfm_priv(pltfm_host);
-	int ret;
-
-	if ((sdhci_am654->quirks & SDHCI_AM654_QUIRK_SUPPRESS_V1P8_ENA) &&
-	    ios->signal_voltage == MMC_SIGNAL_VOLTAGE_180) {
-		if (!IS_ERR(mmc->supply.vqmmc)) {
-			ret = mmc_regulator_set_vqmmc(mmc, ios);
-			if (ret < 0) {
-				pr_err("%s: Switching to 1.8V signalling voltage failed,\n",
-				       mmc_hostname(mmc));
-				return -EIO;
-			}
-		}
-		return 0;
-	}
-
-	return sdhci_start_signal_voltage_switch(mmc, ios);
-}
-
 static u8 sdhci_am654_write_power_on(struct sdhci_host *host, u8 val, int reg)
 {
 	writeb(val, host->ioaddr + reg);
@@ -868,11 +844,6 @@  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 ena for eMMC and SD with vqmmc supply */
-	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;
@@ -969,7 +940,6 @@  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);