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

[Philippe Mathieu-Daudé]

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)) {
---