Hi Cedric,
> From: Cédric Le Goater <c...@kaod.org>
> Sent: Tuesday, January 21, 2025 6:39 PM
> To: Jamin Lin <jamin_...@aspeedtech.com>; Peter Maydell
> <peter.mayd...@linaro.org>; Steven Lee <steven_...@aspeedtech.com>; Troy
> Lee <leet...@gmail.com>; Andrew Jeffery <and...@codeconstruct.com.au>;
> Joel Stanley <j...@jms.id.au>; Philippe Mathieu-Daudé <phi...@linaro.org>;
> Bin Meng <bmeng...@gmail.com>; open list:ASPEED BMCs
> <qemu-...@nongnu.org>; open list:All patches CC here
> <qemu-devel@nongnu.org>; open list:SD (Secure Card)
> <qemu-bl...@nongnu.org>; Bernhard Beschow <shen...@gmail.com>
> Cc: Troy Lee <troy_...@aspeedtech.com>; Yunlin Tang
> <yunlin.t...@aspeedtech.com>; Cédric Le Goater <c...@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_...@aspeedtech.com>
> > Acked-by: Cédric Le Goater <c...@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;
> >
