Re: [PATCH] hw/sd/sdhci: Set reset value of interrupt registers

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

On 6/2/25 14:49, BALATON Zoltan wrote:
> On Thu, 6 Feb 2025, Philippe Mathieu-Daudé wrote:
>> On 6/2/25 13:49, BALATON Zoltan wrote:
>>> On Thu, 6 Feb 2025, Philippe Mathieu-Daudé wrote:
>>>> On 15/1/25 20:04, BALATON Zoltan wrote:
>>>>> The interrupt enable registers are not reset to 0 but some bits are
>>>>> enabled on reset. At least some U-Boot versions seem to expect this
>>>>> and not initialise these registers before expecting interrupts. The
>>>>> numbers in this patch match what QorIQ P1022 has on reset and fix
>>>>> U-Boot for this SoC and should not break other drivers that initialise
>>>>> (and thus overwrite) these reset values.
>>>>>
>>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>>> ---
>>>>> I've also noticed that the work around marked with an XXX comment near
>>>>> line 600 breaks the U-Boot I've tested so I need to disable it:
>>>>> if ((s->sdmasysad % boundary_chk) == 0) {
>>>>> -        page_aligned = true;
>>>>> +//        page_aligned = true;
>>>>> }
>>>>> What should this hack fix and could it be now removed or somehow
>>>>> restricted to cases where it's needed?
>>>>
>>>> Cc'ing Jamin for
>>>> https://lore.kernel.org/qemu-devel/20241213031205.641009-2- 
>>>> jamin_lin@aspeedtech.com/
>>>>
>>>>>
>>>>> hw/sd/sdhci.c | 2 ++
>>>>>   1 file changed, 2 insertions(+)
>>>>>
>>>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>>>>> index 58375483e3..88eb0bfcb2 100644
>>>>> --- a/hw/sd/sdhci.c
>>>>> +++ b/hw/sd/sdhci.c
>>>>> @@ -303,6 +303,8 @@ static void sdhci_reset(SDHCIState *s)
>>>>>       s->data_count = 0;
>>>>>       s->stopped_state = sdhc_not_stopped;
>>>>>       s->pending_insert_state = false;
>>>>> +    s->norintstsen = 0x013f;
>>>>> +    s->errintstsen = 0x117f;
>>>>
>>>> I guess the problem is earlier:
>>>>
>>>>    /*
>>>>     * Set all registers to 0. Capabilities/Version registers are not 
>>>> cleared
>>>>     * and assumed to always preserve their value, given to them during
>>>>     * initialization
>>>>     */
>>>>    memset(&s->sdmasysad, 0, (uintptr_t)&s->capareg - (uintptr_t)&s- 
>>>> >sdmasysad);
>>>>
>>>> Not all registers have to be reset.
>>>
>>> Nothing seems to program those registers before reset but the reset 
>>> values are documented (for Freescale eSDHCI) to be the above so just 
>>> not zeroing them does not seem to be enough. Bernhard has similar 
>>> patch in his branch, not sure if he came up with that separately or 
>>> took this one. Do you have some docs on which regs should not be reset?
>>
>> The header precises what is being modeled here:
>>
>> * SD Association Host Standard Specification v2.0 controller emulation
>> *
>> * Datasheet: 
>> PartA2_SD_Host_Controller_Simplified_Specification_Ver2.00.pdf
>>
>> I can not see the reset values you mentioned there.
>>
>> What is wrong with adding a TYPE_FREESCALE_ESDHC, like the
>> TYPE_IMX_USDHC / TYPE_S3C_SDHCI types? Then you can add your
>> reset handler fixing your fields after sdhci_poweron_reset().
> 
> Nothing's wrong with that, I just did not notice those existing 
> variants. Maybe that's the way to go then with this too. Bernhard, do 
> you plan to do that when cleaning up your tree or should I do another 
> version?

I'm not sure this is the best way to do what you need, but it'd be one
with less impacts on other SDHCI-based models IMO.