[v1] i.MX and SDHCI improvements

[PATCH 08/14] hw/sd/sd: Allow for inverting polarities of presence and write-protect GPIOs

Posted by Bernhard Beschow 2 months, 4 weeks ago

Signed-off-by: Bernhard Beschow <shentey@gmail.com>
---
 hw/sd/sd.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/hw/sd/sd.c b/hw/sd/sd.c
index aa8d86e1af..a50e5c20c8 100644
--- a/hw/sd/sd.c
+++ b/hw/sd/sd.c
@@ -181,6 +181,8 @@ struct SDState {
     qemu_irq inserted_cb;
     QEMUTimer *ocr_power_timer;
     bool enable;
+    bool readonly_active_low;
+    bool inserted_active_low;
     uint8_t dat_lines;
     bool cmd_line;
 };
@@ -876,8 +878,8 @@ static void sd_reset(DeviceState *dev)
     sd->cmd_line = true;
     sd->multi_blk_cnt = 0;
 
-    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd));
-    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd));
+    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd) ^ sd->readonly_active_low);
+    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd) ^ sd->inserted_active_low);
 }
 
 static void sd_cardchange(void *opaque, bool load, Error **errp)
@@ -896,9 +898,9 @@ static void sd_cardchange(void *opaque, bool load, Error **errp)
     }
 
     if (sd->me_no_qdev_me_kill_mammoth_with_rocks) {
-        qemu_set_irq(sd->inserted_cb, inserted);
+        qemu_set_irq(sd->inserted_cb, inserted ^ sd->inserted_active_low);
         if (inserted) {
-            qemu_set_irq(sd->readonly_cb, readonly);
+            qemu_set_irq(sd->readonly_cb, readonly ^ sd->readonly_active_low);
         }
     } else {
         sdbus = SD_BUS(qdev_get_parent_bus(dev));
@@ -2797,6 +2799,8 @@ static void emmc_realize(DeviceState *dev, Error **errp)
 
 static const Property sdmmc_common_properties[] = {
     DEFINE_PROP_DRIVE("drive", SDState, blk),
+    DEFINE_PROP_BOOL("cd-active-low", SDState, inserted_active_low, false),
+    DEFINE_PROP_BOOL("wp-active-low", SDState, readonly_active_low, false),
 };
 
 static const Property sd_properties[] = {
-- 
2.47.1

Re: [PATCH 08/14] hw/sd/sd: Allow for inverting polarities of presence and write-protect GPIOs

Posted by Philippe Mathieu-Daudé 2 months, 3 weeks ago

Hi Bernhard,

On 8/1/25 10:25, Bernhard Beschow wrote:
> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
> ---
>   hw/sd/sd.c | 12 ++++++++----
>   1 file changed, 8 insertions(+), 4 deletions(-)


> @@ -876,8 +878,8 @@ static void sd_reset(DeviceState *dev)
>       sd->cmd_line = true;
>       sd->multi_blk_cnt = 0;
>   
> -    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd));
> -    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd));
> +    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd) ^ sd->readonly_active_low);

Please embed in sd_get_readonly(),

> +    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd) ^ sd->inserted_active_low);

and sd_get_inserted().

>   }
>   
>   static void sd_cardchange(void *opaque, bool load, Error **errp)
> @@ -896,9 +898,9 @@ static void sd_cardchange(void *opaque, bool load, Error **errp)
>       }
>   
>       if (sd->me_no_qdev_me_kill_mammoth_with_rocks) {
> -        qemu_set_irq(sd->inserted_cb, inserted);
> +        qemu_set_irq(sd->inserted_cb, inserted ^ sd->inserted_active_low);

Use sd_get_inserted(),

>           if (inserted) {
> -            qemu_set_irq(sd->readonly_cb, readonly);
> +            qemu_set_irq(sd->readonly_cb, readonly ^ sd->readonly_active_low);

and sd_get_readonly() here.

>           }
>       } else {
>           sdbus = SD_BUS(qdev_get_parent_bus(dev));
> @@ -2797,6 +2799,8 @@ static void emmc_realize(DeviceState *dev, Error **errp)
>   
>   static const Property sdmmc_common_properties[] = {
>       DEFINE_PROP_DRIVE("drive", SDState, blk),
> +    DEFINE_PROP_BOOL("cd-active-low", SDState, inserted_active_low, false),
> +    DEFINE_PROP_BOOL("wp-active-low", SDState, readonly_active_low, false),
>   };
With the requested changes:
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

Re: [PATCH 08/14] hw/sd/sd: Allow for inverting polarities of presence and write-protect GPIOs

Posted by Bernhard Beschow 2 months, 3 weeks ago


Am 9. Januar 2025 11:40:10 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>Hi Bernhard,
>
>On 8/1/25 10:25, Bernhard Beschow wrote:
>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>> ---
>>   hw/sd/sd.c | 12 ++++++++----
>>   1 file changed, 8 insertions(+), 4 deletions(-)
>
>
>> @@ -876,8 +878,8 @@ static void sd_reset(DeviceState *dev)
>>       sd->cmd_line = true;
>>       sd->multi_blk_cnt = 0;
>>   -    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd));
>> -    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd));
>> +    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd) ^ sd->readonly_active_low);
>
>Please embed in sd_get_readonly(),
>
>> +    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd) ^ sd->inserted_active_low);
>
>and sd_get_inserted().

Are you sure? I deliberately implemented it as is because embedding would change the internal logic of the device as well as SDCardClass::{get_inserted, get_readonly}.

Best regards,
Bernhard

>
>>   }
>>     static void sd_cardchange(void *opaque, bool load, Error **errp)
>> @@ -896,9 +898,9 @@ static void sd_cardchange(void *opaque, bool load, Error **errp)
>>       }
>>         if (sd->me_no_qdev_me_kill_mammoth_with_rocks) {
>> -        qemu_set_irq(sd->inserted_cb, inserted);
>> +        qemu_set_irq(sd->inserted_cb, inserted ^ sd->inserted_active_low);
>
>Use sd_get_inserted(),
>
>>           if (inserted) {
>> -            qemu_set_irq(sd->readonly_cb, readonly);
>> +            qemu_set_irq(sd->readonly_cb, readonly ^ sd->readonly_active_low);
>
>and sd_get_readonly() here.
>
>>           }
>>       } else {
>>           sdbus = SD_BUS(qdev_get_parent_bus(dev));
>> @@ -2797,6 +2799,8 @@ static void emmc_realize(DeviceState *dev, Error **errp)
>>     static const Property sdmmc_common_properties[] = {
>>       DEFINE_PROP_DRIVE("drive", SDState, blk),
>> +    DEFINE_PROP_BOOL("cd-active-low", SDState, inserted_active_low, false),
>> +    DEFINE_PROP_BOOL("wp-active-low", SDState, readonly_active_low, false),
>>   };
>With the requested changes:
>Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>

Re: [PATCH 08/14] hw/sd/sd: Allow for inverting polarities of presence and write-protect GPIOs

Posted by Philippe Mathieu-Daudé 2 months, 3 weeks ago

On 9/1/25 17:20, Bernhard Beschow wrote:
> 
> 
> Am 9. Januar 2025 11:40:10 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>> Hi Bernhard,
>>
>> On 8/1/25 10:25, Bernhard Beschow wrote:
>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>> ---
>>>    hw/sd/sd.c | 12 ++++++++----
>>>    1 file changed, 8 insertions(+), 4 deletions(-)
>>
>>
>>> @@ -876,8 +878,8 @@ static void sd_reset(DeviceState *dev)
>>>        sd->cmd_line = true;
>>>        sd->multi_blk_cnt = 0;
>>>    -    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd));
>>> -    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd));
>>> +    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd) ^ sd->readonly_active_low);
>>
>> Please embed in sd_get_readonly(),
>>
>>> +    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd) ^ sd->inserted_active_low);
>>
>> and sd_get_inserted().
> 
> Are you sure? I deliberately implemented it as is because embedding would change the internal logic of the device as well as SDCardClass::{get_inserted, get_readonly}.

Yes, this is why I requested that change. Why don't you think it is correct?

> 
> Best regards,
> Bernhard

>> With the requested changes:
>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>

Re: [PATCH 08/14] hw/sd/sd: Allow for inverting polarities of presence and write-protect GPIOs

Posted by Bernhard Beschow 2 months, 2 weeks ago

Am 12. Januar 2025 18:06:04 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>On 9/1/25 17:20, Bernhard Beschow wrote:
>> 
>> 
>> Am 9. Januar 2025 11:40:10 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>>> Hi Bernhard,
>>> 
>>> On 8/1/25 10:25, Bernhard Beschow wrote:
>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>> ---
>>>>    hw/sd/sd.c | 12 ++++++++----
>>>>    1 file changed, 8 insertions(+), 4 deletions(-)
>>> 
>>> 
>>>> @@ -876,8 +878,8 @@ static void sd_reset(DeviceState *dev)
>>>>        sd->cmd_line = true;
>>>>        sd->multi_blk_cnt = 0;
>>>>    -    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd));
>>>> -    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd));
>>>> +    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd) ^ sd->readonly_active_low);
>>> 
>>> Please embed in sd_get_readonly(),
>>> 
>>>> +    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd) ^ sd->inserted_active_low);
>>> 
>>> and sd_get_inserted().
>> 
>> Are you sure? I deliberately implemented it as is because embedding would change the internal logic of the device as well as SDCardClass::{get_inserted, get_readonly}.
>
>Yes, this is why I requested that change. Why don't you think it is correct?

I'm asking because I think that moving the xor inside the methods would break the device model.

The goal of the active_low attributes is to invert the polarity of the GPIOs only which allows to model boards where these are inverted. IOW they are intended to influence the wiring. That is in contrast to the two methods which determine the internal logic of the device model. They are also used as virtual method implementations of SDCardClass::{get_inserted, get_readonly} which determine the logic of the sd bus. Moving the xor inside inverts their return values if s->*_active_low are true, effectively flipping every if statement, which seems wrong to me. What do I miss?

Best regards,
Bernhard

>
>> 
>> Best regards,
>> Bernhard
>
>>> With the requested changes:
>>> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
>>> 
>

Re: [PATCH 08/14] hw/sd/sd: Allow for inverting polarities of presence and write-protect GPIOs

Posted by Philippe Mathieu-Daudé 2 months, 2 weeks ago

On 17/1/25 00:20, Bernhard Beschow wrote:
> 
> 
> Am 12. Januar 2025 18:06:04 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>> On 9/1/25 17:20, Bernhard Beschow wrote:
>>>
>>>
>>> Am 9. Januar 2025 11:40:10 UTC schrieb "Philippe Mathieu-Daudé" <philmd@linaro.org>:
>>>> Hi Bernhard,
>>>>
>>>> On 8/1/25 10:25, Bernhard Beschow wrote:
>>>>> Signed-off-by: Bernhard Beschow <shentey@gmail.com>
>>>>> ---
>>>>>     hw/sd/sd.c | 12 ++++++++----
>>>>>     1 file changed, 8 insertions(+), 4 deletions(-)
>>>>
>>>>
>>>>> @@ -876,8 +878,8 @@ static void sd_reset(DeviceState *dev)
>>>>>         sd->cmd_line = true;
>>>>>         sd->multi_blk_cnt = 0;
>>>>>     -    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd));
>>>>> -    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd));
>>>>> +    qemu_set_irq(sd->readonly_cb, sd_get_readonly(sd) ^ sd->readonly_active_low);
>>>>
>>>> Please embed in sd_get_readonly(),
>>>>
>>>>> +    qemu_set_irq(sd->inserted_cb, sd_get_inserted(sd) ^ sd->inserted_active_low);
>>>>
>>>> and sd_get_inserted().
>>>
>>> Are you sure? I deliberately implemented it as is because embedding would change the internal logic of the device as well as SDCardClass::{get_inserted, get_readonly}.
>>
>> Yes, this is why I requested that change. Why don't you think it is correct?
> 
> I'm asking because I think that moving the xor inside the methods would break the device model.
> 
> The goal of the active_low attributes is to invert the polarity of the GPIOs only which allows to model boards where these are inverted. IOW they are intended to influence the wiring. That is in contrast to the two methods which determine the internal logic of the device model. They are also used as virtual method implementations of SDCardClass::{get_inserted, get_readonly} which determine the logic of the sd bus. Moving the xor inside inverts their return values if s->*_active_low are true, effectively flipping every if statement, which seems wrong to me. What do I miss?

Right... Then maybe we should model polarity in qemu_irq.

Patches 7 & 8 queued, thanks!