Re: [PATCH 2/2] hw/sd/sdhci: Extract uSDHC-specific quirk

Hi Bin,

On 21/7/26 12:42, Philippe Mathieu-Daudé wrote:
> On 21/7/26 12:29, Bernhard Beschow wrote:
>>
>>
>> Am 21. Juli 2026 08:38:19 UTC schrieb Bin Meng <[email protected]>:
>>> Hi Bernhard,
>>>
>>> On Tue, Jul 21, 2026 at 5:11 AM Bernhard Beschow <[email protected]> 
>>> wrote:
>>>>
>>>>
>>>>
>>>> Am 20. Juli 2026 14:50:24 UTC schrieb Bin Meng <[email protected]>:
>>>>
>>>> Hi Bin,
>>>>
>>>> Thanks for yor reviews and R-b tags!
>>>>
>>>>> On Mon, Jul 20, 2026 at 5:19 AM Bernhard Beschow 
>>>>> <[email protected]> wrote:
>>>>>>
>>>>>> In Linux, the ESDHC_MIX_CTRL qirk is guarded by esdhc_is_usdhc() 
>>>>>> while
>>>>>> the eSDHC code path uses the standard SDHC interface. Extract the 
>>>>>> quirk
>>>>>> into a new `usdhc_write()` function.
>>>>>>
>>>>>> Fixes file system corruption on emulated i.MX53 where Linux'
>>>>>> esdhc_is_usdhc() returns false. The same likely happens on e500 and
>>>>>> imx25-pdk machines.
>>>>>>
>>>>>> Fixes: 75e98bc4f859 ("hw/sd/sdhci: Add TYPE_FSL_ESDHC_BE")
>>>>>> cc: qemu-stable
>>>>>> Signed-off-by: Bernhard Beschow <[email protected]>
>>>>>> ---
>>>>>>   hw/sd/sdhci.c | 73 ++++++++++++++++++++++++++++++ 
>>>>>> +--------------------
>>>>>>   1 file changed, 44 insertions(+), 29 deletions(-)
>>>>>>
>>>>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>>>>>> index c86dfa281f..e58a610397 100644
>>>>>> --- a/hw/sd/sdhci.c
>>>>>> +++ b/hw/sd/sdhci.c
>>>>>> @@ -1795,34 +1795,6 @@ esdhc_write(void *opaque, hwaddr offset, 
>>>>>> uint64_t val, unsigned size)
>>>>>>           sdhci_write(opaque, offset, value, size);
>>>>>>           break;
>>>>>>
>>>>>> -    case ESDHC_MIX_CTRL:
>>>>>> -        /*
>>>>>> -         * So, when SD/MMC stack in Linux tries to write to 
>>>>>> "Transfer
>>>>>> -         * Mode Register", ESDHC i.MX quirk code will translate it
>>>>>> -         * into a write to ESDHC_MIX_CTRL, so we do the opposite in
>>>>>> -         * order to get where we started
>>>>>> -         *
>>>>>> -         * Note that Auto CMD23 Enable bit is located in a wrong 
>>>>>> place
>>>>>> -         * on i.MX, but since it is not used by QEMU we do not care.
>>>>>> -         *
>>>>>> -         * We don't want to call sdhci_write(.., SDHC_TRNMOD, ...)
>>>>>> -         * here because it will result in a call to
>>>>>> -         * sdhci_send_command(s) which we don't want.
>>>>>> -         *
>>>>>> -         */
>>>>>> -        s->trnmod = value & UINT16_MAX;
>>>>>> -        break;
>>>>>> -    case SDHC_TRNMOD:
>>>>>
>>>>> Looks like this register is not eSDHC specific?
>>>>
>>>> Not quite sure what you mean exactly. The SDHC_TRNMOD isn't eSDHC 
>>>> specific but as per the comment above trnmod partial handling is 
>>>> deferred here, i.e. needs to be intercepted.
>>>
>>> If I read the Linux driver correctly, I think we should do something
>>> like below when writing to SDHC_TRNMOD:
>>>
>>>   case SDHC_TRNMOD:
>>>       if (is_usdhc) {
>>>           val |= s->trnmod;
>>>       }
>>>       sdhci_write(opaque, offset, val, size);
>>>       break;
>>>
>>> Because for eSDHC, the transfer mode is already passed in the value,
>>> and oring previous s->trnmod may cause some bits being set to old
>>> previous value unexpectedly. Please double check.
>>
>> By moving out of esdhc_write() into a dedicated usdhc_write() method 
>> we achieve exactly that, no?

What is your final tought, should we proceed with v2?