Am 6. Februar 2025 13:49:38 UTC schrieb BALATON Zoltan <bala...@eik.bme.hu>:
>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 <bala...@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_...@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?
Feel free to submit a patch. I'm actually in favor of a dedicated device model
which was also my first approach:
<https://lore.kernel.org/qemu-devel/20221003203142.24355-14-shen...@gmail.com/>
Perhaps we could have dedicated source files as well.
Best regards,
Bernhard
>
>Regards,
>BALATON Zoltan
