Re: [PATCH v3 0/3] Introduce a new Write Protected pin inverted property

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

On 27/11/24 10:44, Cédric Le Goater wrote:
> On 11/14/24 10:48, Jamin Lin wrote:
>> change from v1:
>> 1. Support RTC for AST2700.
>> 2. Support SDHCI write protected pin inverted for AST2500 and AST2600.
>> 3. Introduce Capabilities Register 2 for SD slot 0 and 1.
>> 4. Support create flash devices via command line for AST1030.
>>
>> change from v2:
>> replace wp-invert with wp-inverted and fix review issues.
>>
>> change from v3:
>> 1. add reviewer suggestion about wp_inverted comment
>> 2. AST2500 EVB does not need to set wp-inverted property of sdhci model
>>     
>> https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm/boot/dts/aspeed/aspeed-ast2500-evb.dts#L110
>>
>> Jamin Lin (3):
>>    hw/sd/sdhci: Fix coding style
>>    hw/sd/sdhci: Introduce a new Write Protected pin inverted property
>>    hw/arm/aspeed: Invert sdhci write protected pin for AST2600 EVB
>>
>>   hw/arm/aspeed.c         |  7 +++++
>>   hw/sd/sdhci.c           | 70 ++++++++++++++++++++++++++++-------------
>>   include/hw/arm/aspeed.h |  1 +
>>   include/hw/sd/sdhci.h   |  5 +++
>>   4 files changed, 61 insertions(+), 22 deletions(-)
>>
> 
> Philippe,
> 
> I plan to queue patch 2-3 for QEMU 10.0. Is that ok for you ?

Having to modify sdhci.c internals is dubious, since inversion
occurs out of this block. If this is the soc/board layer, isn't
better to model at this level? Smth like:

-- >8 --
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index be3eb70cdd7..aad9be66b75 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -559,8 +559,9 @@ static void aspeed_soc_ast2600_realize(DeviceState 
*dev, Error **errp)
      }
      aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->sdhci), 0,
                      sc->memmap[ASPEED_DEV_SDHCI]);
+    irq = aspeed_soc_get_irq(s, ASPEED_DEV_SDHCI);
      sysbus_connect_irq(SYS_BUS_DEVICE(&s->sdhci), 0,
-                       aspeed_soc_get_irq(s, ASPEED_DEV_SDHCI));
+                       sc->sdhci_wp_inverted ? qemu_irq_invert(irq) : irq);

      /* eMMC */
      if (!sysbus_realize(SYS_BUS_DEVICE(&s->emmc), errp)) {
---