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

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

[BALATON Zoltan]

The interrupt enable registers are not reset to 0 on Freescale eSDHC
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. Use existing vendor property for Freescale eSDHC and set
the reset value of the interrupt registers to match Freescale
documentation.

Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
---
v2: Restrict to e500. Adding a reset method in a subclass does not
work because the common reset function is called directly on register
write from the guest but there's already provision for vendor specific
behaviour which can be used to restrict this to Freescale SoCs.

 hw/ppc/e500.c         | 1 +
 hw/sd/sdhci.c         | 4 ++++
 include/hw/sd/sdhci.h | 1 +
 3 files changed, 6 insertions(+)

diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
index 26933e0457..560eb42a12 100644
--- a/hw/ppc/e500.c
+++ b/hw/ppc/e500.c
@@ -1044,6 +1044,7 @@ void ppce500_init(MachineState *machine)
         dev = qdev_new(TYPE_SYSBUS_SDHCI);
         qdev_prop_set_uint8(dev, "sd-spec-version", 2);
         qdev_prop_set_uint8(dev, "endianness", DEVICE_BIG_ENDIAN);
+        qdev_prop_set_uint8(dev, "vendor", SDHCI_VENDOR_FSL);
         s = SYS_BUS_DEVICE(dev);
         sysbus_realize_and_unref(s, &error_fatal);
         sysbus_connect_irq(s, 0, qdev_get_gpio_in(mpicdev, MPC85XX_ESDHC_IRQ));
diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index 99dd4a4e95..afa3c6d448 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -307,6 +307,10 @@ static void sdhci_reset(SDHCIState *s)
     s->data_count = 0;
     s->stopped_state = sdhc_not_stopped;
     s->pending_insert_state = false;
+    if (s->vendor == SDHCI_VENDOR_FSL) {
+        s->norintstsen = 0x013f;
+        s->errintstsen = 0x117f;
+    }
 }
 
 static void sdhci_poweron_reset(DeviceState *dev)
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 38c08e2859..f722d8eb1c 100644
--- a/include/hw/sd/sdhci.h
+++ b/include/hw/sd/sdhci.h
@@ -110,6 +110,7 @@ typedef struct SDHCIState SDHCIState;
 
 #define SDHCI_VENDOR_NONE       0
 #define SDHCI_VENDOR_IMX        1
+#define SDHCI_VENDOR_FSL        2
 
 /*
  * Controller does not provide transfer-complete interrupt when not
-- 
2.30.9

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

[BALATON Zoltan]

On Mon, 10 Feb 2025, BALATON Zoltan wrote:
> The interrupt enable registers are not reset to 0 on Freescale eSDHC
> 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. Use existing vendor property for Freescale eSDHC and set
> the reset value of the interrupt registers to match Freescale
> documentation.

Ping?

> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> v2: Restrict to e500. Adding a reset method in a subclass does not
> work because the common reset function is called directly on register
> write from the guest but there's already provision for vendor specific
> behaviour which can be used to restrict this to Freescale SoCs.
>
> hw/ppc/e500.c         | 1 +
> hw/sd/sdhci.c         | 4 ++++
> include/hw/sd/sdhci.h | 1 +
> 3 files changed, 6 insertions(+)
>
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 26933e0457..560eb42a12 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -1044,6 +1044,7 @@ void ppce500_init(MachineState *machine)
>         dev = qdev_new(TYPE_SYSBUS_SDHCI);
>         qdev_prop_set_uint8(dev, "sd-spec-version", 2);
>         qdev_prop_set_uint8(dev, "endianness", DEVICE_BIG_ENDIAN);
> +        qdev_prop_set_uint8(dev, "vendor", SDHCI_VENDOR_FSL);
>         s = SYS_BUS_DEVICE(dev);
>         sysbus_realize_and_unref(s, &error_fatal);
>         sysbus_connect_irq(s, 0, qdev_get_gpio_in(mpicdev, MPC85XX_ESDHC_IRQ));
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 99dd4a4e95..afa3c6d448 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -307,6 +307,10 @@ static void sdhci_reset(SDHCIState *s)
>     s->data_count = 0;
>     s->stopped_state = sdhc_not_stopped;
>     s->pending_insert_state = false;
> +    if (s->vendor == SDHCI_VENDOR_FSL) {
> +        s->norintstsen = 0x013f;
> +        s->errintstsen = 0x117f;
> +    }
> }
>
> static void sdhci_poweron_reset(DeviceState *dev)
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index 38c08e2859..f722d8eb1c 100644
> --- a/include/hw/sd/sdhci.h
> +++ b/include/hw/sd/sdhci.h
> @@ -110,6 +110,7 @@ typedef struct SDHCIState SDHCIState;
>
> #define SDHCI_VENDOR_NONE       0
> #define SDHCI_VENDOR_IMX        1
> +#define SDHCI_VENDOR_FSL        2
>
> /*
>  * Controller does not provide transfer-complete interrupt when not
>

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

[Bernhard Beschow]

Am 1. März 2025 16:02:05 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>On Mon, 10 Feb 2025, BALATON Zoltan wrote:
>> The interrupt enable registers are not reset to 0 on Freescale eSDHC
>> 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. Use existing vendor property for Freescale eSDHC and set
>> the reset value of the interrupt registers to match Freescale
>> documentation.
>
>Ping?
>
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> v2: Restrict to e500. Adding a reset method in a subclass does not
>> work because the common reset function is called directly on register
>> write from the guest but there's already provision for vendor specific
>> behaviour which can be used to restrict this to Freescale SoCs.
>> 
>> hw/ppc/e500.c         | 1 +
>> hw/sd/sdhci.c         | 4 ++++
>> include/hw/sd/sdhci.h | 1 +
>> 3 files changed, 6 insertions(+)
>> 
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index 26933e0457..560eb42a12 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -1044,6 +1044,7 @@ void ppce500_init(MachineState *machine)
>>         dev = qdev_new(TYPE_SYSBUS_SDHCI);
>>         qdev_prop_set_uint8(dev, "sd-spec-version", 2);
>>         qdev_prop_set_uint8(dev, "endianness", DEVICE_BIG_ENDIAN);
>> +        qdev_prop_set_uint8(dev, "vendor", SDHCI_VENDOR_FSL);
>>         s = SYS_BUS_DEVICE(dev);
>>         sysbus_realize_and_unref(s, &error_fatal);
>>         sysbus_connect_irq(s, 0, qdev_get_gpio_in(mpicdev, MPC85XX_ESDHC_IRQ));
>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>> index 99dd4a4e95..afa3c6d448 100644
>> --- a/hw/sd/sdhci.c
>> +++ b/hw/sd/sdhci.c
>> @@ -307,6 +307,10 @@ static void sdhci_reset(SDHCIState *s)
>>     s->data_count = 0;
>>     s->stopped_state = sdhc_not_stopped;
>>     s->pending_insert_state = false;
>> +    if (s->vendor == SDHCI_VENDOR_FSL) {
>> +        s->norintstsen = 0x013f;
>> +        s->errintstsen = 0x117f;
>> +    }
>> }
>> 
>> static void sdhci_poweron_reset(DeviceState *dev)
>> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
>> index 38c08e2859..f722d8eb1c 100644
>> --- a/include/hw/sd/sdhci.h
>> +++ b/include/hw/sd/sdhci.h
>> @@ -110,6 +110,7 @@ typedef struct SDHCIState SDHCIState;
>> 
>> #define SDHCI_VENDOR_NONE       0
>> #define SDHCI_VENDOR_IMX        1
>> +#define SDHCI_VENDOR_FSL        2

From an i.MX point of view the sole "FSL" postfix seems confusing since these SoCs are from the same vendor. What about "FSL_ESDHC"?

Best regards,
Bernhard

>> 
>> /*
>>  * Controller does not provide transfer-complete interrupt when not
>> 

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

[BALATON Zoltan]

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

On Mon, 3 Mar 2025, Bernhard Beschow wrote:
> Am 1. März 2025 16:02:05 UTC schrieb BALATON Zoltan <balaton@eik.bme.hu>:
>> On Mon, 10 Feb 2025, BALATON Zoltan wrote:
>>> The interrupt enable registers are not reset to 0 on Freescale eSDHC
>>> 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. Use existing vendor property for Freescale eSDHC and set
>>> the reset value of the interrupt registers to match Freescale
>>> documentation.
>>
>> Ping?
>>
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>> v2: Restrict to e500. Adding a reset method in a subclass does not
>>> work because the common reset function is called directly on register
>>> write from the guest but there's already provision for vendor specific
>>> behaviour which can be used to restrict this to Freescale SoCs.
>>>
>>> hw/ppc/e500.c         | 1 +
>>> hw/sd/sdhci.c         | 4 ++++
>>> include/hw/sd/sdhci.h | 1 +
>>> 3 files changed, 6 insertions(+)
>>>
>>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>>> index 26933e0457..560eb42a12 100644
>>> --- a/hw/ppc/e500.c
>>> +++ b/hw/ppc/e500.c
>>> @@ -1044,6 +1044,7 @@ void ppce500_init(MachineState *machine)
>>>         dev = qdev_new(TYPE_SYSBUS_SDHCI);
>>>         qdev_prop_set_uint8(dev, "sd-spec-version", 2);
>>>         qdev_prop_set_uint8(dev, "endianness", DEVICE_BIG_ENDIAN);
>>> +        qdev_prop_set_uint8(dev, "vendor", SDHCI_VENDOR_FSL);
>>>         s = SYS_BUS_DEVICE(dev);
>>>         sysbus_realize_and_unref(s, &error_fatal);
>>>         sysbus_connect_irq(s, 0, qdev_get_gpio_in(mpicdev, MPC85XX_ESDHC_IRQ));
>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>>> index 99dd4a4e95..afa3c6d448 100644
>>> --- a/hw/sd/sdhci.c
>>> +++ b/hw/sd/sdhci.c
>>> @@ -307,6 +307,10 @@ static void sdhci_reset(SDHCIState *s)
>>>     s->data_count = 0;
>>>     s->stopped_state = sdhc_not_stopped;
>>>     s->pending_insert_state = false;
>>> +    if (s->vendor == SDHCI_VENDOR_FSL) {
>>> +        s->norintstsen = 0x013f;
>>> +        s->errintstsen = 0x117f;
>>> +    }
>>> }
>>>
>>> static void sdhci_poweron_reset(DeviceState *dev)
>>> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
>>> index 38c08e2859..f722d8eb1c 100644
>>> --- a/include/hw/sd/sdhci.h
>>> +++ b/include/hw/sd/sdhci.h
>>> @@ -110,6 +110,7 @@ typedef struct SDHCIState SDHCIState;
>>>
>>> #define SDHCI_VENDOR_NONE       0
>>> #define SDHCI_VENDOR_IMX        1
>>> +#define SDHCI_VENDOR_FSL        2
>
> From an i.MX point of view the sole "FSL" postfix seems confusing since 
> these SoCs are from the same vendor. What about "FSL_ESDHC"?

That's too long and not a vendor. How about FSL_IMX instead? But maybe 
it's not really a problem as IMX is already a separate define and other 
devices use the FSL prefix so maybe it's not that confusing to worth 
changing.

Regards,
BALATON Zoltan

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

[Philippe Mathieu-Daudé]

Hi Zoltan,

On 10/2/25 17:03, BALATON Zoltan wrote:
> The interrupt enable registers are not reset to 0 on Freescale eSDHC
> 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. Use existing vendor property for Freescale eSDHC and set
> the reset value of the interrupt registers to match Freescale
> documentation.
> 
> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
> ---
> v2: Restrict to e500. Adding a reset method in a subclass does not
> work because the common reset function is called directly on register
> write from the guest but there's already provision for vendor specific
> behaviour which can be used to restrict this to Freescale SoCs.
> 
>   hw/ppc/e500.c         | 1 +
>   hw/sd/sdhci.c         | 4 ++++
>   include/hw/sd/sdhci.h | 1 +
>   3 files changed, 6 insertions(+)
> 
> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
> index 26933e0457..560eb42a12 100644
> --- a/hw/ppc/e500.c
> +++ b/hw/ppc/e500.c
> @@ -1044,6 +1044,7 @@ void ppce500_init(MachineState *machine)
>           dev = qdev_new(TYPE_SYSBUS_SDHCI);
>           qdev_prop_set_uint8(dev, "sd-spec-version", 2);
>           qdev_prop_set_uint8(dev, "endianness", DEVICE_BIG_ENDIAN);
> +        qdev_prop_set_uint8(dev, "vendor", SDHCI_VENDOR_FSL);
>           s = SYS_BUS_DEVICE(dev);
>           sysbus_realize_and_unref(s, &error_fatal);
>           sysbus_connect_irq(s, 0, qdev_get_gpio_in(mpicdev, MPC85XX_ESDHC_IRQ));
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index 99dd4a4e95..afa3c6d448 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -307,6 +307,10 @@ static void sdhci_reset(SDHCIState *s)
>       s->data_count = 0;
>       s->stopped_state = sdhc_not_stopped;
>       s->pending_insert_state = false;
> +    if (s->vendor == SDHCI_VENDOR_FSL) {
> +        s->norintstsen = 0x013f;
> +        s->errintstsen = 0x117f;

I'd rather do like capareg, and add:

   DEFINE_PROP_UINT16("norintstsen", _state, norintstsen, 0),
   ...

Then SoC code sets it:

   qdev_prop_set_uint16(dev, "norintstsen", 0x013f);
   ...

WDYT?

> +    }
>   }

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

[BALATON Zoltan]

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

On Mon, 3 Mar 2025, Philippe Mathieu-Daudé wrote:
> Hi Zoltan,
>
> On 10/2/25 17:03, BALATON Zoltan wrote:
>> The interrupt enable registers are not reset to 0 on Freescale eSDHC
>> 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. Use existing vendor property for Freescale eSDHC and set
>> the reset value of the interrupt registers to match Freescale
>> documentation.
>> 
>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>> ---
>> v2: Restrict to e500. Adding a reset method in a subclass does not
>> work because the common reset function is called directly on register
>> write from the guest but there's already provision for vendor specific
>> behaviour which can be used to restrict this to Freescale SoCs.
>>
>>   hw/ppc/e500.c         | 1 +
>>   hw/sd/sdhci.c         | 4 ++++
>>   include/hw/sd/sdhci.h | 1 +
>>   3 files changed, 6 insertions(+)
>> 
>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>> index 26933e0457..560eb42a12 100644
>> --- a/hw/ppc/e500.c
>> +++ b/hw/ppc/e500.c
>> @@ -1044,6 +1044,7 @@ void ppce500_init(MachineState *machine)
>>           dev = qdev_new(TYPE_SYSBUS_SDHCI);
>>           qdev_prop_set_uint8(dev, "sd-spec-version", 2);
>>           qdev_prop_set_uint8(dev, "endianness", DEVICE_BIG_ENDIAN);
>> +        qdev_prop_set_uint8(dev, "vendor", SDHCI_VENDOR_FSL);
>>           s = SYS_BUS_DEVICE(dev);
>>           sysbus_realize_and_unref(s, &error_fatal);
>>           sysbus_connect_irq(s, 0, qdev_get_gpio_in(mpicdev, 
>> MPC85XX_ESDHC_IRQ));
>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>> index 99dd4a4e95..afa3c6d448 100644
>> --- a/hw/sd/sdhci.c
>> +++ b/hw/sd/sdhci.c
>> @@ -307,6 +307,10 @@ static void sdhci_reset(SDHCIState *s)
>>       s->data_count = 0;
>>       s->stopped_state = sdhc_not_stopped;
>>       s->pending_insert_state = false;
>> +    if (s->vendor == SDHCI_VENDOR_FSL) {
>> +        s->norintstsen = 0x013f;
>> +        s->errintstsen = 0x117f;
>
> I'd rather do like capareg, and add:
>
>  DEFINE_PROP_UINT16("norintstsen", _state, norintstsen, 0),
>  ...

I don't see what you mean. capareg does not seem to be set via a property.

> Then SoC code sets it:
>
>  qdev_prop_set_uint16(dev, "norintstsen", 0x013f);
>  ...
>
> WDYT?

I think it may be overkill to add properties for this if there are no 
other vendor or variant that needs this. Also properties are for something 
the user may want to set as those can be changed with QEMU command line 
and these reset values aren't something the user should change so I think 
this patch is the simplest solution now.

Regards,
BALATON Zoltan

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

[BALATON Zoltan]

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

On Mon, 3 Mar 2025, BALATON Zoltan wrote:
> On Mon, 3 Mar 2025, Philippe Mathieu-Daudé wrote:
>> Hi Zoltan,
>> 
>> On 10/2/25 17:03, BALATON Zoltan wrote:
>>> The interrupt enable registers are not reset to 0 on Freescale eSDHC
>>> 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. Use existing vendor property for Freescale eSDHC and set
>>> the reset value of the interrupt registers to match Freescale
>>> documentation.
>>> 
>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>> ---
>>> v2: Restrict to e500. Adding a reset method in a subclass does not
>>> work because the common reset function is called directly on register
>>> write from the guest but there's already provision for vendor specific
>>> behaviour which can be used to restrict this to Freescale SoCs.
>>>
>>>   hw/ppc/e500.c         | 1 +
>>>   hw/sd/sdhci.c         | 4 ++++
>>>   include/hw/sd/sdhci.h | 1 +
>>>   3 files changed, 6 insertions(+)
>>> 
>>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>>> index 26933e0457..560eb42a12 100644
>>> --- a/hw/ppc/e500.c
>>> +++ b/hw/ppc/e500.c
>>> @@ -1044,6 +1044,7 @@ void ppce500_init(MachineState *machine)
>>>           dev = qdev_new(TYPE_SYSBUS_SDHCI);
>>>           qdev_prop_set_uint8(dev, "sd-spec-version", 2);
>>>           qdev_prop_set_uint8(dev, "endianness", DEVICE_BIG_ENDIAN);
>>> +        qdev_prop_set_uint8(dev, "vendor", SDHCI_VENDOR_FSL);
>>>           s = SYS_BUS_DEVICE(dev);
>>>           sysbus_realize_and_unref(s, &error_fatal);
>>>           sysbus_connect_irq(s, 0, qdev_get_gpio_in(mpicdev, 
>>> MPC85XX_ESDHC_IRQ));
>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>>> index 99dd4a4e95..afa3c6d448 100644
>>> --- a/hw/sd/sdhci.c
>>> +++ b/hw/sd/sdhci.c
>>> @@ -307,6 +307,10 @@ static void sdhci_reset(SDHCIState *s)
>>>       s->data_count = 0;
>>>       s->stopped_state = sdhc_not_stopped;
>>>       s->pending_insert_state = false;
>>> +    if (s->vendor == SDHCI_VENDOR_FSL) {
>>> +        s->norintstsen = 0x013f;
>>> +        s->errintstsen = 0x117f;
>> 
>> I'd rather do like capareg, and add:
>>
>>  DEFINE_PROP_UINT16("norintstsen", _state, norintstsen, 0),
>>  ...
>
> I don't see what you mean. capareg does not seem to be set via a property.
>
>> Then SoC code sets it:
>>
>>  qdev_prop_set_uint16(dev, "norintstsen", 0x013f);
>>  ...
>> 
>> WDYT?
>
> I think it may be overkill to add properties for this if there are no other 
> vendor or variant that needs this. Also properties are for something the user 
> may want to set as those can be changed with QEMU command line and these 
> reset values aren't something the user should change so I think this patch is 
> the simplest solution now.

This patch wasn't in the pull request but I haven't seen an answer to this 
message either so was it missed or do you have furhter comments? Bernhard 
has a comment about naming of SDHCI_VENDOR_FSL but I think the already 
existing IMX name is what's wrong not the one added in this patch but I 
don't think that's really that confusing to worth further effort. We still 
have time as this can be considered a fix but I'd like this to not get 
forgotten so I bring it up again.

Regards,
BALATON Zoltan

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

[Philippe Mathieu-Daudé]

On 6/3/25 19:23, BALATON Zoltan wrote:
> On Mon, 3 Mar 2025, BALATON Zoltan wrote:
>> On Mon, 3 Mar 2025, Philippe Mathieu-Daudé wrote:
>>> Hi Zoltan,
>>>
>>> On 10/2/25 17:03, BALATON Zoltan wrote:
>>>> The interrupt enable registers are not reset to 0 on Freescale eSDHC
>>>> 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. Use existing vendor property for Freescale eSDHC and set
>>>> the reset value of the interrupt registers to match Freescale
>>>> documentation.
>>>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>> v2: Restrict to e500. Adding a reset method in a subclass does not
>>>> work because the common reset function is called directly on register
>>>> write from the guest but there's already provision for vendor specific
>>>> behaviour which can be used to restrict this to Freescale SoCs.
>>>>
>>>>   hw/ppc/e500.c         | 1 +
>>>>   hw/sd/sdhci.c         | 4 ++++
>>>>   include/hw/sd/sdhci.h | 1 +
>>>>   3 files changed, 6 insertions(+)
>>>>
>>>> diff --git a/hw/ppc/e500.c b/hw/ppc/e500.c
>>>> index 26933e0457..560eb42a12 100644
>>>> --- a/hw/ppc/e500.c
>>>> +++ b/hw/ppc/e500.c
>>>> @@ -1044,6 +1044,7 @@ void ppce500_init(MachineState *machine)
>>>>           dev = qdev_new(TYPE_SYSBUS_SDHCI);
>>>>           qdev_prop_set_uint8(dev, "sd-spec-version", 2);
>>>>           qdev_prop_set_uint8(dev, "endianness", DEVICE_BIG_ENDIAN);
>>>> +        qdev_prop_set_uint8(dev, "vendor", SDHCI_VENDOR_FSL);
>>>>           s = SYS_BUS_DEVICE(dev);
>>>>           sysbus_realize_and_unref(s, &error_fatal);
>>>>           sysbus_connect_irq(s, 0, qdev_get_gpio_in(mpicdev, 
>>>> MPC85XX_ESDHC_IRQ));
>>>> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
>>>> index 99dd4a4e95..afa3c6d448 100644
>>>> --- a/hw/sd/sdhci.c
>>>> +++ b/hw/sd/sdhci.c
>>>> @@ -307,6 +307,10 @@ static void sdhci_reset(SDHCIState *s)
>>>>       s->data_count = 0;
>>>>       s->stopped_state = sdhc_not_stopped;
>>>>       s->pending_insert_state = false;
>>>> +    if (s->vendor == SDHCI_VENDOR_FSL) {
>>>> +        s->norintstsen = 0x013f;
>>>> +        s->errintstsen = 0x117f;
>>>
>>> I'd rather do like capareg, and add:
>>>
>>>  DEFINE_PROP_UINT16("norintstsen", _state, norintstsen, 0),
>>>  ...
>>
>> I don't see what you mean. capareg does not seem to be set via a 
>> property.
>>
>>> Then SoC code sets it:
>>>
>>>  qdev_prop_set_uint16(dev, "norintstsen", 0x013f);
>>>  ...
>>>
>>> WDYT?
>>
>> I think it may be overkill to add properties for this if there are no 
>> other vendor or variant that needs this. Also properties are for 
>> something the user may want to set as those can be changed with QEMU 
>> command line and these reset values aren't something the user should 
>> change so I think this patch is the simplest solution now.
> 
> This patch wasn't in the pull request but I haven't seen an answer to 
> this message either so was it missed or do you have furhter comments? 
> Bernhard has a comment about naming of SDHCI_VENDOR_FSL but I think the 
> already existing IMX name is what's wrong not the one added in this 
> patch but I don't think that's really that confusing to worth further 
> effort. We still have time as this can be considered a fix but I'd like 
> this to not get forgotten so I bring it up again.

I'll post a v3 with what I had in mind.

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

[Philippe Mathieu-Daudé]

On 6/3/25 19:23, BALATON Zoltan wrote:
> On Mon, 3 Mar 2025, BALATON Zoltan wrote:
>> On Mon, 3 Mar 2025, Philippe Mathieu-Daudé wrote:
>>> Hi Zoltan,
>>>
>>> On 10/2/25 17:03, BALATON Zoltan wrote:
>>>> The interrupt enable registers are not reset to 0 on Freescale eSDHC
>>>> 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. Use existing vendor property for Freescale eSDHC and set
>>>> the reset value of the interrupt registers to match Freescale
>>>> documentation.
>>>>
>>>> Signed-off-by: BALATON Zoltan <balaton@eik.bme.hu>
>>>> ---
>>>> v2: Restrict to e500. Adding a reset method in a subclass does not
>>>> work because the common reset function is called directly on register
>>>> write from the guest but there's already provision for vendor specific
>>>> behaviour which can be used to restrict this to Freescale SoCs.
>>>>
>>>>   hw/ppc/e500.c         | 1 +
>>>>   hw/sd/sdhci.c         | 4 ++++
>>>>   include/hw/sd/sdhci.h | 1 +
>>>>   3 files changed, 6 insertions(+)


> This patch wasn't in the pull request but I haven't seen an answer to 
> this message either so was it missed or do you have furhter comments? 
> Bernhard has a comment about naming of SDHCI_VENDOR_FSL but I think the 
> already existing IMX name is what's wrong not the one added in this 
> patch but I don't think that's really that confusing to worth further 
> effort. We still have time as this can be considered a fix but I'd like 
> this to not get forgotten so I bring it up again.

Patch queued, thanks.