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

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

[Jamin Lin via]

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

-- 
2.34.1

[PATCH v3 1/3] hw/sd/sdhci: Fix coding style

[Jamin Lin via]

Fix coding style issues from checkpatch.pl

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
Reviewed-by: Cédric Le Goater <clg@redhat.com>
---
 hw/sd/sdhci.c | 64 +++++++++++++++++++++++++++++++++------------------
 1 file changed, 42 insertions(+), 22 deletions(-)

diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
index dbe5c2340c..37875c02c3 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -233,7 +233,7 @@ static void sdhci_raise_insertion_irq(void *opaque)
 
     if (s->norintsts & SDHC_NIS_REMOVE) {
         timer_mod(s->insert_timer,
-                       qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + SDHC_INSERTION_DELAY);
+                qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + SDHC_INSERTION_DELAY);
     } else {
         s->prnsts = 0x1ff0000;
         if (s->norintstsen & SDHC_NISEN_INSERT) {
@@ -251,7 +251,7 @@ static void sdhci_set_inserted(DeviceState *dev, bool level)
     if ((s->norintsts & SDHC_NIS_REMOVE) && level) {
         /* Give target some time to notice card ejection */
         timer_mod(s->insert_timer,
-                       qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + SDHC_INSERTION_DELAY);
+                qemu_clock_get_ns(QEMU_CLOCK_VIRTUAL) + SDHC_INSERTION_DELAY);
     } else {
         if (level) {
             s->prnsts = 0x1ff0000;
@@ -289,9 +289,11 @@ static void sdhci_reset(SDHCIState *s)
     timer_del(s->insert_timer);
     timer_del(s->transfer_timer);
 
-    /* Set all registers to 0. Capabilities/Version registers are not cleared
+    /*
+     * Set all registers to 0. Capabilities/Version registers are not cleared
      * and assumed to always preserve their value, given to them during
-     * initialization */
+     * initialization
+     */
     memset(&s->sdmasysad, 0, (uintptr_t)&s->capareg - (uintptr_t)&s->sdmasysad);
 
     /* Reset other state based on current card insertion/readonly status */
@@ -305,7 +307,8 @@ static void sdhci_reset(SDHCIState *s)
 
 static void sdhci_poweron_reset(DeviceState *dev)
 {
-    /* QOM (ie power-on) reset. This is identical to reset
+    /*
+     * QOM (ie power-on) reset. This is identical to reset
      * commanded via device register apart from handling of the
      * 'pending insert on powerup' quirk.
      */
@@ -445,8 +448,10 @@ static void sdhci_read_block_from_card(SDHCIState *s)
         s->prnsts &= ~SDHC_DAT_LINE_ACTIVE;
     }
 
-    /* If stop at block gap request was set and it's not the last block of
-     * data - generate Block Event interrupt */
+    /*
+     * If stop at block gap request was set and it's not the last block of
+     * data - generate Block Event interrupt
+     */
     if (s->stopped_state == sdhc_gap_read && (s->trnmod & SDHC_TRNS_MULTI) &&
             s->blkcnt != 1)    {
         s->prnsts &= ~SDHC_DAT_LINE_ACTIVE;
@@ -548,8 +553,10 @@ static void sdhci_write_block_to_card(SDHCIState *s)
     sdhci_update_irq(s);
 }
 
-/* Write @size bytes of @value data to host controller @s Buffer Data Port
- * register */
+/*
+ * Write @size bytes of @value data to host controller @s Buffer Data Port
+ * register
+ */
 static void sdhci_write_dataport(SDHCIState *s, uint32_t value, unsigned size)
 {
     unsigned i;
@@ -594,9 +601,11 @@ static void sdhci_sdma_transfer_multi_blocks(SDHCIState *s)
         return;
     }
 
-    /* XXX: Some sd/mmc drivers (for example, u-boot-slp) do not account for
+    /*
+     * XXX: Some sd/mmc drivers (for example, u-boot-slp) do not account for
      * possible stop at page boundary if initial address is not page aligned,
-     * allow them to work properly */
+     * allow them to work properly
+     */
     if ((s->sdmasysad % boundary_chk) == 0) {
         page_aligned = true;
     }
@@ -702,7 +711,8 @@ static void get_adma_description(SDHCIState *s, ADMADescr *dscr)
         dma_memory_read(s->dma_as, entry_addr, &adma2, sizeof(adma2),
                         MEMTXATTRS_UNSPECIFIED);
         adma2 = le64_to_cpu(adma2);
-        /* The spec does not specify endianness of descriptor table.
+        /*
+         * The spec does not specify endianness of descriptor table.
          * We currently assume that it is LE.
          */
         dscr->addr = (hwaddr)extract64(adma2, 32, 32) & ~0x3ull;
@@ -977,8 +987,10 @@ static bool sdhci_can_issue_command(SDHCIState *s)
     return true;
 }
 
-/* The Buffer Data Port register must be accessed in sequential and
- * continuous manner */
+/*
+ * The Buffer Data Port register must be accessed in sequential and
+ * continuous manner
+ */
 static inline bool
 sdhci_buff_access_is_sequential(SDHCIState *s, unsigned byte_num)
 {
@@ -1206,8 +1218,10 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
         MASKED_WRITE(s->argument, mask, value);
         break;
     case SDHC_TRNMOD:
-        /* DMA can be enabled only if it is supported as indicated by
-         * capabilities register */
+        /*
+         * DMA can be enabled only if it is supported as indicated by
+         * capabilities register
+         */
         if (!(s->capareg & R_SDHC_CAPAB_SDMA_MASK)) {
             value &= ~SDHC_TRNS_DMA;
         }
@@ -1279,8 +1293,10 @@ sdhci_write(void *opaque, hwaddr offset, uint64_t val, unsigned size)
         } else {
             s->norintsts &= ~SDHC_NIS_ERR;
         }
-        /* Quirk for Raspberry Pi: pending card insert interrupt
-         * appears when first enabled after power on */
+        /*
+         * Quirk for Raspberry Pi: pending card insert interrupt
+         * appears when first enabled after power on
+         */
         if ((s->norintstsen & SDHC_NISEN_INSERT) && s->pending_insert_state) {
             assert(s->pending_insert_quirk);
             s->norintsts |= SDHC_NIS_INSERT;
@@ -1396,8 +1412,10 @@ void sdhci_initfn(SDHCIState *s)
 {
     qbus_init(&s->sdbus, sizeof(s->sdbus), TYPE_SDHCI_BUS, DEVICE(s), "sd-bus");
 
-    s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_raise_insertion_irq, s);
-    s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL, sdhci_data_transfer, s);
+    s->insert_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+                                   sdhci_raise_insertion_irq, s);
+    s->transfer_timer = timer_new_ns(QEMU_CLOCK_VIRTUAL,
+                                     sdhci_data_transfer, s);
 
     s->io_ops = &sdhci_mmio_le_ops;
 }
@@ -1445,11 +1463,13 @@ void sdhci_common_realize(SDHCIState *s, Error **errp)
 
 void sdhci_common_unrealize(SDHCIState *s)
 {
-    /* This function is expected to be called only once for each class:
+    /*
+     * This function is expected to be called only once for each class:
      * - SysBus:    via DeviceClass->unrealize(),
      * - PCI:       via PCIDeviceClass->exit().
      * However to avoid double-free and/or use-after-free we still nullify
-     * this variable (better safe than sorry!). */
+     * this variable (better safe than sorry!).
+     */
     g_free(s->fifo_buffer);
     s->fifo_buffer = NULL;
 }
-- 
2.34.1

Re: [PATCH v3 1/3] hw/sd/sdhci: Fix coding style

[Philippe Mathieu-Daudé]

On 14/11/24 10:48, Jamin Lin wrote:
> Fix coding style issues from checkpatch.pl
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> Reviewed-by: Cédric Le Goater <clg@redhat.com>
> ---
>   hw/sd/sdhci.c | 64 +++++++++++++++++++++++++++++++++------------------
>   1 file changed, 42 insertions(+), 22 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

[Jamin Lin via]

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>
---
 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;
 
-- 
2.34.1

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

[Philippe Mathieu-Daudé]

On 14/11/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>
> ---
>   hw/sd/sdhci.c         | 6 ++++++
>   include/hw/sd/sdhci.h | 5 +++++
>   2 files changed, 11 insertions(+)

Acked-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

[Cédric Le Goater]

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/

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;
>   

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;
> >

[PATCH v3 3/3] hw/arm/aspeed: Invert sdhci write protected pin for AST2600 EVB

[Jamin Lin via]

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

According to the design of AST2600 EVB, the Write Protected pin is active
high by default. To support it, introduces a new "sdhci_wp_inverted"
property in ASPEED MACHINE State and set it true for AST2600 EVB
and set "wp_inverted" property true of sdhci-generic model.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>
---
 hw/arm/aspeed.c         | 7 +++++++
 include/hw/arm/aspeed.h | 1 +
 2 files changed, 8 insertions(+)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 6ca145362c..a58b7160cd 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -413,6 +413,12 @@ static void aspeed_machine_init(MachineState *machine)
                              OBJECT(get_system_memory()), &error_abort);
     object_property_set_link(OBJECT(bmc->soc), "dram",
                              OBJECT(machine->ram), &error_abort);
+    if (amc->sdhci_wp_inverted) {
+        for (i = 0; i < bmc->soc->sdhci.num_slots; i++) {
+            object_property_set_bool(OBJECT(&bmc->soc->sdhci.slots[i]),
+                                     "wp-inverted", true, &error_abort);
+        }
+    }
     if (machine->kernel_filename) {
         /*
          * When booting with a -kernel command line there is no u-boot
@@ -1419,6 +1425,7 @@ static void aspeed_machine_ast2600_evb_class_init(ObjectClass *oc, void *data)
     amc->num_cs    = 1;
     amc->macs_mask = ASPEED_MAC0_ON | ASPEED_MAC1_ON | ASPEED_MAC2_ON |
                      ASPEED_MAC3_ON;
+    amc->sdhci_wp_inverted = true;
     amc->i2c_init  = ast2600_evb_i2c_init;
     mc->default_ram_size = 1 * GiB;
     aspeed_machine_class_init_cpus_defaults(mc);
diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
index cbeacb214c..9cae45a1c9 100644
--- a/include/hw/arm/aspeed.h
+++ b/include/hw/arm/aspeed.h
@@ -39,6 +39,7 @@ struct AspeedMachineClass {
     uint32_t macs_mask;
     void (*i2c_init)(AspeedMachineState *bmc);
     uint32_t uart_default;
+    bool sdhci_wp_inverted;
 };
 
 
-- 
2.34.1

Re: [PATCH v3 3/3] hw/arm/aspeed: Invert sdhci write protected pin for AST2600 EVB

[Philippe Mathieu-Daudé]

On 14/11/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).
> 
> According to the design of AST2600 EVB, the Write Protected pin is active
> high by default. To support it, introduces a new "sdhci_wp_inverted"
> property in ASPEED MACHINE State and set it true for AST2600 EVB
> and set "wp_inverted" property true of sdhci-generic model.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>
> ---
>   hw/arm/aspeed.c         | 7 +++++++
>   include/hw/arm/aspeed.h | 1 +
>   2 files changed, 8 insertions(+)

Acked-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

[Cédric Le Goater]

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 ?

Thanks,

C.

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

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

[Cédric Le Goater]

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

Nice ! I didn't know about qemu_irq_invert().

Jamin, could you please give it a try and respin ?

Thanks,

C.

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

[Peter Maydell]

On Wed, 27 Nov 2024 at 11:26, Cédric Le Goater <clg@kaod.org> wrote:
>
>
> > 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)) {
> > ---
>
> Nice ! I didn't know about qemu_irq_invert().

I am not a fan of qemu_irq_invert(). It's one of those ancient
pre-QOM APIs that we ideally would get rid of at some point.
Two problems with it:

(1) It allocates and returns a qemu_irq directly,
so in the patch above you're effectively leaking that
allocation. (Not a big deal since the SoC object is going to
be around for the life of the QEMU process, but probably
the clang leak-sanitizer will complain.)

(2) It calls qemu_irq_raise() directly, immediately. This is
kind of bogus in a realize function, where you're not supposed
to be raising IRQ lines yet; it also doesn't do anything about
reset, so if the device on the other end *did* care about seeing
that 0->1 transition then it will be broken on system-reset
because the transition won't happen. (Handling "device is supposed
to have an asserted-as-1 line coming out of reset" is not
something that we do very well in QEMU generally. In theory
3-phase reset is supposed to help with this by letting you do
the assert-the-line in the reset-exit phase, but in practice we
typically just don't model the line-assertion at all and
trust that the reset state of the device on the far end is
what it ought to be anyway...)

I would not recommend using qemu_irq_invert() in new code.

I guess in an ideal world we'd implement a QOM object
that encapsulated the the "not gate" logic, similar to
TYPE_OR_IRQ. (Though for TYPE_OR_IRQ we made the mistake
of making it inherit from TYPE_DEVICE, not TYPE_SYSBUS_DEVICE,
so it doesn't get reset properly on system reset and so
the "what happens to the output on reset" is still not
really correct.)

thanks
-- PMM

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

[Cédric Le Goater]

Hello,

> I would not recommend using qemu_irq_invert() in new code.
> 
> I guess in an ideal world we'd implement a QOM object
> that encapsulated the the "not gate" logic, similar to
> TYPE_OR_IRQ. (Though for TYPE_OR_IRQ we made the mistake
> of making it inherit from TYPE_DEVICE, not TYPE_SYSBUS_DEVICE,
> so it doesn't get reset properly on system reset and so
> the "what happens to the output on reset" is still not
> really correct.)

I see how this would work with TYPE_PL181 model but I don't
understand how this could work with TYPE_SYSBUS_SDHCI since
we don't have a gpio line to invert the level. Am I missing
something ?

Thanks,

C.

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

[Peter Maydell]

On Tue, 7 Jan 2025 at 17:55, Cédric Le Goater <clg@kaod.org> wrote:
>
> Hello,
>
> > I would not recommend using qemu_irq_invert() in new code.
> >
> > I guess in an ideal world we'd implement a QOM object
> > that encapsulated the the "not gate" logic, similar to
> > TYPE_OR_IRQ. (Though for TYPE_OR_IRQ we made the mistake
> > of making it inherit from TYPE_DEVICE, not TYPE_SYSBUS_DEVICE,
> > so it doesn't get reset properly on system reset and so
> > the "what happens to the output on reset" is still not
> > really correct.)
>
> I see how this would work with TYPE_PL181 model but I don't
> understand how this could work with TYPE_SYSBUS_SDHCI since
> we don't have a gpio line to invert the level. Am I missing
> something ?

You have a gpio line, i.e. a qemu_irq, because you're passing
it to qemu_irq_invert(), and you could instead connect it into
a hypothetical TYPE_NOT_IRQ device. qemu_irq are GPIO lines,
the type just has an odd name for historical reasons.

-- PMM

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

[Cédric Le Goater]

On 1/7/25 23:36, Peter Maydell wrote:
> On Tue, 7 Jan 2025 at 17:55, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> Hello,
>>
>>> I would not recommend using qemu_irq_invert() in new code.
>>>
>>> I guess in an ideal world we'd implement a QOM object
>>> that encapsulated the the "not gate" logic, similar to
>>> TYPE_OR_IRQ. (Though for TYPE_OR_IRQ we made the mistake
>>> of making it inherit from TYPE_DEVICE, not TYPE_SYSBUS_DEVICE,
>>> so it doesn't get reset properly on system reset and so
>>> the "what happens to the output on reset" is still not
>>> really correct.)
>>
>> I see how this would work with TYPE_PL181 model but I don't
>> understand how this could work with TYPE_SYSBUS_SDHCI since
>> we don't have a gpio line to invert the level. Am I missing
>> something ?
> 
> You have a gpio line, i.e. a qemu_irq, because you're passing
> it to qemu_irq_invert(), 

This part was proposed by Philippe and is not related to the
proposal.

I am not very familiar with these devices and AFAUI, the
SHDCIState irq being modified has a much broader scope than
a write protection toggle/level. I suppose the irq covers all
kind of transfers.

> and you could instead connect it into
> a hypothetical TYPE_NOT_IRQ device. qemu_irq are GPIO lines,
> the type just has an odd name for historical reasons.

Yes. I was looking for something like this but we don't have
a "card-read-only" GPIO line like in TYPE_PL181. Hence my
confusion.

Thanks,

C.

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

[Jamin Lin]

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

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

[Cédric Le Goater]

On 11/27/24 12:23, Philippe Mathieu-Daudé wrote:
> 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:

This change is implementing the polarity of a WP pin as described
in [1]. I think modeling that with a QOM property should be ok,
since we don't have GPIO lines in the generic SDHCI device model.
Or we could introduce a SDBusClass class attribute and define
new types for it ?

Thanks,

C.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/devicetree/bindings/mmc/mmc-controller.yaml#n52