RE: [PATCH v3 2/3] hw/sd/sdhci: Introduce a new Write Protected pin inverted property

[Jamin Lin]

Hi Cedric, 

> From: Cédric Le Goater <clg@kaod.org>
> Sent: Tuesday, January 21, 2025 6:39 PM
> To: Jamin Lin <jamin_lin@aspeedtech.com>; Peter Maydell
> <peter.maydell@linaro.org>; Steven Lee <steven_lee@aspeedtech.com>; Troy
> Lee <leetroy@gmail.com>; Andrew Jeffery <andrew@codeconstruct.com.au>;
> Joel Stanley <joel@jms.id.au>; Philippe Mathieu-Daudé <philmd@linaro.org>;
> Bin Meng <bmeng.cn@gmail.com>; open list:ASPEED BMCs
> <qemu-arm@nongnu.org>; open list:All patches CC here
> <qemu-devel@nongnu.org>; open list:SD (Secure Card)
> <qemu-block@nongnu.org>; Bernhard Beschow <shentey@gmail.com>
> Cc: Troy Lee <troy_lee@aspeedtech.com>; Yunlin Tang
> <yunlin.tang@aspeedtech.com>; Cédric Le Goater <clg@redhat.com>
> Subject: Re: [PATCH v3 2/3] hw/sd/sdhci: Introduce a new Write Protected pin
> inverted property
> 
> Jamin,
> 
> +Bernhard
> 
> On 11/14/24 10:48, Jamin Lin wrote:
> > The Write Protect pin of SDHCI model is default active low to match
> > the SDHCI spec. So, write enable the bit 19 should be 1 and write
> > protected the bit 19 should be 0 at the Present State Register (0x24).
> > However, some boards are design Write Protected pin active high. In
> > other words, write enable the bit 19 should be 0 and write protected
> > the bit 19 should be 1 at the Present State Register (0x24). To support it,
> introduces a new "wp-inverted"
> > property and set it false by default.
> >
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > Acked-by: Cédric Le Goater <clg@redhat.com>
> 
> When you have some time, could you please check that this change fits the
> aspeed needs :
> 
> 
> https://lore.kernel.org/qemu-devel/20250108092538.11474-9-shentey@gmail.
> com/
> 
Thanks for your suggestion and help.

This patch is on the SD Card side.
My change is on the SDHCI (Host Controller) side.

I tried to directly set both the "wp-active-low" property and the 
"readonly_active_low" 
variable to "true" and "false" in "sd.c". 
Unfortunately, I still got the READONLY status in SDHCI.
The reason is that the SDHCI register 0x24 is not inverted.
Thanks-Jamin

root@ast2600-default:~# cat 
/sys/bus/mmc/drivers/mmcblk/mmc1\:f3a5/block/mmcblk1/ro
1

root@ast2600-default:~# dmesg | grep "mmc"
[   13.131526] mmc0: SDHCI controller on 1e750100.sdhci [1e750100.sdhci] using 
ADMA
[   13.212462] mmc0: Failed to initialize a non-removable card
[   13.829166] mmc1: SDHCI controller on 1e740100.sdhci [1e740100.sdhci] using 
ADMA
[   13.832527] mmc2: SDHCI controller on 1e740200.sdhci [1e740200.sdhci] using 
ADMA
[   13.890041] mmc2: new high speed SD card at address cae3
[   13.896243] mmcblk2: mmc2:cae3 QEMU! 128 MiB (ro)
[   13.899294] mmc1: new high speed SD card at address f3a5
[   13.915949] mmcblk1: mmc1:f3a5 QEMU! 128 MiB (ro)

root@ast2600-default:~# mount /dev/mmcblk1 /mnt
[  107.822908] EXT4-fs (mmcblk1): mounted filesystem 
3e5fa086-9be6-453a-8533-bca6cba15873 ro with ordered data mode. Quota mode: 
disabled.
mount: /mnt: WARNING: source write-protected, mounted read-only.

root@ast2600-default:~# touch /mnt/1111111
touch: /mnt/1111111: Read-only file system

> It should be merged shortly.
> 
> Thanks,
> 
> C.
> 
> 
> 
> 
> > ---
> >   hw/sd/sdhci.c         | 6 ++++++
> >   include/hw/sd/sdhci.h | 5 +++++
> >   2 files changed, 11 insertions(+)
> >
> > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index
> > 37875c02c3..06d1e24086 100644
> > --- a/hw/sd/sdhci.c
> > +++ b/hw/sd/sdhci.c
> > @@ -274,6 +274,10 @@ static void sdhci_set_readonly(DeviceState *dev,
> bool level)
> >   {
> >       SDHCIState *s = (SDHCIState *)dev;
> >
> > +    if (s->wp_inverted) {
> > +        level = !level;
> > +    }
> > +
> >       if (level) {
> >           s->prnsts &= ~SDHC_WRITE_PROTECT;
> >       } else {
> > @@ -1550,6 +1554,8 @@ static Property sdhci_sysbus_properties[] = {
> >                        false),
> >       DEFINE_PROP_LINK("dma", SDHCIState,
> >                        dma_mr, TYPE_MEMORY_REGION,
> MemoryRegion *),
> > +    DEFINE_PROP_BOOL("wp-inverted", SDHCIState,
> > +                     wp_inverted, false),
> >       DEFINE_PROP_END_OF_LIST(),
> >   };
> >
> > diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h index
> > 6cd2822f1d..38c08e2859 100644
> > --- a/include/hw/sd/sdhci.h
> > +++ b/include/hw/sd/sdhci.h
> > @@ -100,6 +100,11 @@ struct SDHCIState {
> >       uint8_t sd_spec_version;
> >       uint8_t uhs_mode;
> >       uint8_t vendor;        /* For vendor specific functionality */
> > +    /*
> > +     * Write Protect pin default active low for detecting SD card
> > +     * to be protected. Set wp_inverted to invert the signal.
> > +     */
> > +    bool wp_inverted;
> >   };
> >   typedef struct SDHCIState SDHCIState;
> >