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

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

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().