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

[PATCH v2 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.

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 and
    AST2500 EVBs

 hw/arm/aspeed.c         |  8 +++++
 hw/sd/sdhci.c           | 70 ++++++++++++++++++++++++++++-------------
 include/hw/arm/aspeed.h |  1 +
 include/hw/sd/sdhci.h   |  5 +++
 4 files changed, 62 insertions(+), 22 deletions(-)

-- 
2.34.1

[PATCH v2 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.

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 and
    AST2500 EVBs

 hw/arm/aspeed.c         |  8 +++++
 hw/sd/sdhci.c           | 70 ++++++++++++++++++++++++++++-------------
 include/hw/arm/aspeed.h |  1 +
 include/hw/sd/sdhci.h   |  5 +++
 4 files changed, 62 insertions(+), 22 deletions(-)

-- 
2.34.1

[PATCH v2 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 ed01499391..db7d547156 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -234,7 +234,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) {
@@ -252,7 +252,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;
@@ -290,9 +290,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 */
@@ -306,7 +308,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.
      */
@@ -446,8 +449,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;
@@ -549,8 +554,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;
@@ -595,9 +602,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;
     }
@@ -703,7 +712,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;
@@ -978,8 +988,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)
 {
@@ -1207,8 +1219,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;
         }
@@ -1280,8 +1294,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;
@@ -1397,8 +1413,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;
 }
@@ -1446,11 +1464,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

[PATCH v2 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 ed01499391..db7d547156 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -234,7 +234,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) {
@@ -252,7 +252,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;
@@ -290,9 +290,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 */
@@ -306,7 +308,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.
      */
@@ -446,8 +449,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;
@@ -549,8 +554,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;
@@ -595,9 +602,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;
     }
@@ -703,7 +712,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;
@@ -978,8 +988,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)
 {
@@ -1207,8 +1219,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;
         }
@@ -1280,8 +1294,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;
@@ -1397,8 +1413,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;
 }
@@ -1446,11 +1464,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

[PATCH v2 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>
---
 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 db7d547156..c675543873 100644
--- a/hw/sd/sdhci.c
+++ b/hw/sd/sdhci.c
@@ -275,6 +275,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 {
@@ -1551,6 +1555,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..25ad9ed778 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 true inverted the signal.
+     */
+    bool wp_inverted;
 };
 typedef struct SDHCIState SDHCIState;
 
-- 
2.34.1

[PATCH v2 3/3] hw/arm/aspeed: Invert sdhci write protected pin for AST2600 and AST2500 EVBs

[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 AST2500 and AST2600 EVBs, 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 AST2500 and AST2600 EVBs
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         | 8 ++++++++
 include/hw/arm/aspeed.h | 1 +
 2 files changed, 9 insertions(+)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index e447923536..b10033d536 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -403,6 +403,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
@@ -1308,6 +1314,7 @@ static void aspeed_machine_ast2500_evb_class_init(ObjectClass *oc, void *data)
     amc->fmc_model = "mx25l25635e";
     amc->spi_model = "mx25l25635f";
     amc->num_cs    = 1;
+    amc->sdhci_wp_inverted = true;
     amc->i2c_init  = ast2500_evb_i2c_init;
     mc->default_ram_size       = 512 * MiB;
     aspeed_machine_class_init_cpus_defaults(mc);
@@ -1409,6 +1416,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 v2 0/3] Introduce a new Write Protected pin inverted property

[Jamin Lin]

Hi Cedric, Andrew

> Subject: [PATCH v2 0/3] Introduce a new Write Protected pin inverted property
> 
> 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.
> 
> 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 and
>     AST2500 EVBs
> 
>  hw/arm/aspeed.c         |  8 +++++
>  hw/sd/sdhci.c           | 70 ++++++++++++++++++++++++++++-------------
>  include/hw/arm/aspeed.h |  1 +
>  include/hw/sd/sdhci.h   |  5 +++
>  4 files changed, 62 insertions(+), 22 deletions(-)
> 

Could you please help to review this patch series?
Thanks-Jamin

> --
> 2.34.1

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

[Cédric Le Goater]

Hello Jamin,

On 11/14/24 06:32, Jamin Lin wrote:
> Hi Cedric, Andrew
> 
>> Subject: [PATCH v2 0/3] Introduce a new Write Protected pin inverted property
>>
>> 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.
>>
>> 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 and
>>      AST2500 EVBs
>>
>>   hw/arm/aspeed.c         |  8 +++++
>>   hw/sd/sdhci.c           | 70 ++++++++++++++++++++++++++++-------------
>>   include/hw/arm/aspeed.h |  1 +
>>   include/hw/sd/sdhci.h   |  5 +++
>>   4 files changed, 62 insertions(+), 22 deletions(-)
>>
> 
> Could you please help to review this patch series?

We would need an Ack from the sd maintainer on patch 2. Then,
I can apply on the aspeed queue. That's material for QEMU 10.0.

Thanks,

C.

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

[Cédric Le Goater]

On 11/4/24 04:21, 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(+)
> 
> diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c
> index db7d547156..c675543873 100644
> --- a/hw/sd/sdhci.c
> +++ b/hw/sd/sdhci.c
> @@ -275,6 +275,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 {
> @@ -1551,6 +1555,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..25ad9ed778 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 true inverted the signal.

In case you respin, may be you could change the last sentence to :
"Set wp_inverted to invert the signal."


Thanks,

C.


> +     */
> +    bool wp_inverted;
>   };
>   typedef struct SDHCIState SDHCIState;
>   

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

[Jamin Lin]

Hi Cedric,

> 
> Hello Jamin,
> 
> On 11/14/24 06:32, Jamin Lin wrote:
> > Hi Cedric, Andrew
> >
> >> Subject: [PATCH v2 0/3] Introduce a new Write Protected pin inverted
> >> property
> >>
> >> 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.
> >>
> >> 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 and
> >>      AST2500 EVBs
> >>
> >>   hw/arm/aspeed.c         |  8 +++++
> >>   hw/sd/sdhci.c           | 70
> ++++++++++++++++++++++++++++-------------
> >>   include/hw/arm/aspeed.h |  1 +
> >>   include/hw/sd/sdhci.h   |  5 +++
> >>   4 files changed, 62 insertions(+), 22 deletions(-)
> >>
> >
> > Could you please help to review this patch series?
> 
> We would need an Ack from the sd maintainer on patch 2. Then, I can apply on
> the aspeed queue. That's material for QEMU 10.0.
> 
Got it.
Thanks for your kindly support.
Jamin
> Thanks,
> 
> C.

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

[Jamin Lin]

Hi Cedric,

> 
> On 11/4/24 04:21, 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(+)
> >
> > diff --git a/hw/sd/sdhci.c b/hw/sd/sdhci.c index
> > db7d547156..c675543873 100644
> > --- a/hw/sd/sdhci.c
> > +++ b/hw/sd/sdhci.c
> > @@ -275,6 +275,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 {
> > @@ -1551,6 +1555,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..25ad9ed778 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 true inverted the signal.
> 
> In case you respin, may be you could change the last sentence to :
> "Set wp_inverted to invert the signal."
> 
> 
Thanks for your review.
Will update it.
Jamin
> Thanks,
> 
> C.
> 
> 
> > +     */
> > +    bool wp_inverted;
> >   };
> >   typedef struct SDHCIState SDHCIState;
> >