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

[Judith Mendez <jm@ti.com>]

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