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

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

[BALATON Zoltan]

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?

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;
 }
 
 static void sdhci_poweron_reset(DeviceState *dev)
-- 
2.30.9

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

[BALATON Zoltan]

On Wed, 15 Jan 2025, 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.

Ping?

> 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?
>
> 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;
> }
>
> static void sdhci_poweron_reset(DeviceState *dev)
>

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

[Philippe Mathieu-Daudé]

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.

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2208 bytes --]

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?

Regards,
BALATON Zoltan

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

[Philippe Mathieu-Daudé]

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

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

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 3157 bytes --]

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?

Regards,
BALATON Zoltan

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

[Philippe Mathieu-Daudé]

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.

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

[Bernhard Beschow]

Am 6. Februar 2025 13:49:38 UTC schrieb BALATON Zoltan <balaton@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 <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?

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-shentey@gmail.com/> Perhaps we could have dedicated source files as well.

Best regards,
Bernhard

>
>Regards,
>BALATON Zoltan