Hi Philippe,

> Subject: Re: [PATCH v3 0/3] Introduce a new Write Protected pin inverted
> property
> 
> 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);
> 

Thanks for suggestion.

Unfortunately, I still got the "Write-Protected" status after I used "qemu_irq_invert(irq)".

root@ast2600-default# dmesg | grep "mmc"
[   13.889764] mmc2: SDHCI controller on 1e740200.sdhci [1e740200.sdhci] using ADMA
[   13.889848] mmc1: SDHCI controller on 1e740100.sdhci [1e740100.sdhci] using ADMA
[   16.739901] mmc1: new high speed SD card at address 4dd7
[   16.740330] mmc2: new high speed SD card at address e502
[   16.745232] mmcblk2: mmc2:e502 QEMU! 128 MiB (ro)
[   16.748765] mmcblk1: mmc1:4dd7 QEMU! 128 MiB (ro)

I dump RO status of SDHCI driver and I still got the READONLY status.
root@ast2600-default:/# cat /sys/bus/mmc/devices/mmc1\:4dd7/block/mmcblk1/ro
1

Our FW added “sdhci,wp-inverted” in SDHCI bus driver.
https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm/boot/dts/aspeed/aspeed-ast2600-evb.dts#L757
This property was defined here, https://github.com/torvalds/linux/blob/master/Documentation/devicetree/bindings/mmc/mmc-controller.yaml#L71

According to the design of SDHCI drivers, it read the PRESENT STATE REGISTER at SDHCI_WRITE_PROTECT bit.
If the value of this bit is 0, the status is readonly.
However, our FW added “wp-inverted” property, so it inverted the readonly status in line 2582.
Using qemu_irq_invert(irq) did not change the PRESENT STATE REGISTER value and 
that was why I added a new property in SD common model(sdhci.c) to make users to change this bit value.

https://github.com/torvalds/linux/blob/master/drivers/mmc/host/sdhci-pltfm.c#L42
https://github.com/torvalds/linux/blob/master/drivers/mmc/host/sdhci-pltfm.c#L87
https://github.com/torvalds/linux/blob/master/drivers/mmc/host/sdhci.c#L2574
https://github.com/torvalds/linux/blob/master/drivers/mmc/host/sdhci.c#L2581

Thanks-Jamin

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