[PATCH v1 0/8] Support RTC for AST2700

[PATCH v1 0/8] Support RTC for AST2700

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

Jamin Lin (8):
  aspeed/soc: Support RTC for AST2700
  hw/timer/aspeed: Fix coding style
  hw/timer/aspeed: Fix interrupt status does not be cleared for AST2600
  hw/sd/sdhci: Fix coding style
  hw/sd/sdhci: Introduce a new Write Protected pin inverted property
  hw/sd/aspeed_sdhci: Introduce Capabilities Register 2 for SD slot 0
    and 1
  hw/arm/aspeed: Invert sdhci write protected pin for AST2600 and
    AST2500 EVBs
  aspeed: Support create flash devices via command line for AST1030

 hw/arm/aspeed.c         | 30 ++++++++++++------
 hw/arm/aspeed_ast27x0.c | 11 +++++++
 hw/sd/aspeed_sdhci.c    | 40 ++++++++++++++++-------
 hw/sd/sdhci.c           | 70 ++++++++++++++++++++++++++++-------------
 hw/timer/aspeed_timer.c | 15 +++++----
 include/hw/arm/aspeed.h |  1 +
 include/hw/sd/sdhci.h   |  5 +++
 7 files changed, 123 insertions(+), 49 deletions(-)

-- 
2.34.1

[PATCH v1 1/8] aspeed/soc: Support RTC for AST2700

[Jamin Lin via]

The RTC controller between AST2600 and AST2700 are identical. Add RTC model for
AST2700 RTC support. The RTC controller registers base address is start at
0x12C0_F000 and its alarm interrupt is connected to GICINT13.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/arm/aspeed_ast27x0.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
index dca660eb6b..7ab4bec644 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -63,6 +63,7 @@ static const hwaddr aspeed_soc_ast2700_memmap[] = {
     [ASPEED_DEV_ADC]       =  0x14C00000,
     [ASPEED_DEV_I2C]       =  0x14C0F000,
     [ASPEED_DEV_GPIO]      =  0x14C0B000,
+    [ASPEED_DEV_RTC]       =  0x12C0F000,
 };
 
 #define AST2700_MAX_IRQ 288
@@ -376,6 +377,8 @@ static void aspeed_soc_ast2700_init(Object *obj)
 
     snprintf(typename, sizeof(typename), "aspeed.gpio-%s", socname);
     object_initialize_child(obj, "gpio", &s->gpio, typename);
+
+    object_initialize_child(obj, "rtc", &s->rtc, TYPE_ASPEED_RTC);
 }
 
 /*
@@ -670,6 +673,14 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp)
     sysbus_connect_irq(SYS_BUS_DEVICE(&s->gpio), 0,
                        aspeed_soc_get_irq(s, ASPEED_DEV_GPIO));
 
+    /* RTC */
+    if (!sysbus_realize(SYS_BUS_DEVICE(&s->rtc), errp)) {
+        return;
+    }
+    aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->rtc), 0, sc->memmap[ASPEED_DEV_RTC]);
+    sysbus_connect_irq(SYS_BUS_DEVICE(&s->rtc), 0,
+                       aspeed_soc_get_irq(s, ASPEED_DEV_RTC));
+
     create_unimplemented_device("ast2700.dpmcu", 0x11000000, 0x40000);
     create_unimplemented_device("ast2700.iomem0", 0x12000000, 0x01000000);
     create_unimplemented_device("ast2700.iomem1", 0x14000000, 0x01000000);
-- 
2.34.1

Re: [SPAM] [PATCH v1 1/8] aspeed/soc: Support RTC for AST2700

[Cédric Le Goater]

On 10/29/24 10:17, Jamin Lin wrote:
> The RTC controller between AST2600 and AST2700 are identical. Add RTC model for
> AST2700 RTC support. The RTC controller registers base address is start at
> 0x12C0_F000 and its alarm interrupt is connected to GICINT13.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/arm/aspeed_ast27x0.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
> index dca660eb6b..7ab4bec644 100644
> --- a/hw/arm/aspeed_ast27x0.c
> +++ b/hw/arm/aspeed_ast27x0.c
> @@ -63,6 +63,7 @@ static const hwaddr aspeed_soc_ast2700_memmap[] = {
>       [ASPEED_DEV_ADC]       =  0x14C00000,
>       [ASPEED_DEV_I2C]       =  0x14C0F000,
>       [ASPEED_DEV_GPIO]      =  0x14C0B000,
> +    [ASPEED_DEV_RTC]       =  0x12C0F000,
>   };
>   
>   #define AST2700_MAX_IRQ 288
> @@ -376,6 +377,8 @@ static void aspeed_soc_ast2700_init(Object *obj)
>   
>       snprintf(typename, sizeof(typename), "aspeed.gpio-%s", socname);
>       object_initialize_child(obj, "gpio", &s->gpio, typename);
> +
> +    object_initialize_child(obj, "rtc", &s->rtc, TYPE_ASPEED_RTC);
>   }
>   
>   /*
> @@ -670,6 +673,14 @@ static void aspeed_soc_ast2700_realize(DeviceState *dev, Error **errp)
>       sysbus_connect_irq(SYS_BUS_DEVICE(&s->gpio), 0,
>                          aspeed_soc_get_irq(s, ASPEED_DEV_GPIO));
>   
> +    /* RTC */
> +    if (!sysbus_realize(SYS_BUS_DEVICE(&s->rtc), errp)) {
> +        return;
> +    }
> +    aspeed_mmio_map(s, SYS_BUS_DEVICE(&s->rtc), 0, sc->memmap[ASPEED_DEV_RTC]);
> +    sysbus_connect_irq(SYS_BUS_DEVICE(&s->rtc), 0,
> +                       aspeed_soc_get_irq(s, ASPEED_DEV_RTC));
> +
>       create_unimplemented_device("ast2700.dpmcu", 0x11000000, 0x40000);
>       create_unimplemented_device("ast2700.iomem0", 0x12000000, 0x01000000);
>       create_unimplemented_device("ast2700.iomem1", 0x14000000, 0x01000000);

[PATCH v1 2/8] hw/timer/aspeed: Fix coding style

[Jamin Lin via]

Fix coding style issues from checkpatch.pl

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/timer/aspeed_timer.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
index b1f860ecfb..5af268ea9e 100644
--- a/hw/timer/aspeed_timer.c
+++ b/hw/timer/aspeed_timer.c
@@ -276,7 +276,8 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
         old_reload = t->reload;
         t->reload = calculate_min_ticks(t, value);
 
-        /* If the reload value was not previously set, or zero, and
+        /*
+         * If the reload value was not previously set, or zero, and
          * the current value is valid, try to start the timer if it is
          * enabled.
          */
@@ -312,7 +313,8 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
     }
 }
 
-/* Control register operations are broken out into helpers that can be
+/*
+ * Control register operations are broken out into helpers that can be
  * explicitly called on aspeed_timer_reset(), but also from
  * aspeed_timer_ctrl_op().
  */
@@ -396,7 +398,8 @@ static void aspeed_timer_set_ctrl(AspeedTimerCtrlState *s, uint32_t reg)
     AspeedTimer *t;
     const uint8_t enable_mask = BIT(op_enable);
 
-    /* Handle a dependency between the 'enable' and remaining three
+    /*
+     * Handle a dependency between the 'enable' and remaining three
      * configuration bits - i.e. if more than one bit in the control set has
      * changed, including the 'enable' bit, then we want either disable the
      * timer and perform configuration, or perform configuration and then
@@ -582,7 +585,6 @@ static void aspeed_2600_timer_write(AspeedTimerCtrlState *s, hwaddr offset,
     case 0x3C:
         aspeed_timer_set_ctrl(s, s->ctrl & ~tv);
         break;
-
     case 0x38:
     default:
         qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n",
@@ -623,7 +625,8 @@ static void aspeed_timer_reset(DeviceState *dev)
 
     for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
         AspeedTimer *t = &s->timers[i];
-        /* Explicitly call helpers to avoid any conditional behaviour through
+        /*
+         * Explicitly call helpers to avoid any conditional behaviour through
          * aspeed_timer_set_ctrl().
          */
         aspeed_timer_ctrl_enable(t, false);
-- 
2.34.1

Re: [SPAM] [PATCH v1 2/8] hw/timer/aspeed: Fix coding style

[Cédric Le Goater]

On 10/29/24 10:17, 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>

Thanks,

C.


> ---
>   hw/timer/aspeed_timer.c | 13 ++++++++-----
>   1 file changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> index b1f860ecfb..5af268ea9e 100644
> --- a/hw/timer/aspeed_timer.c
> +++ b/hw/timer/aspeed_timer.c
> @@ -276,7 +276,8 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
>           old_reload = t->reload;
>           t->reload = calculate_min_ticks(t, value);
>   
> -        /* If the reload value was not previously set, or zero, and
> +        /*
> +         * If the reload value was not previously set, or zero, and
>            * the current value is valid, try to start the timer if it is
>            * enabled.
>            */
> @@ -312,7 +313,8 @@ static void aspeed_timer_set_value(AspeedTimerCtrlState *s, int timer, int reg,
>       }
>   }
>   
> -/* Control register operations are broken out into helpers that can be
> +/*
> + * Control register operations are broken out into helpers that can be
>    * explicitly called on aspeed_timer_reset(), but also from
>    * aspeed_timer_ctrl_op().
>    */
> @@ -396,7 +398,8 @@ static void aspeed_timer_set_ctrl(AspeedTimerCtrlState *s, uint32_t reg)
>       AspeedTimer *t;
>       const uint8_t enable_mask = BIT(op_enable);
>   
> -    /* Handle a dependency between the 'enable' and remaining three
> +    /*
> +     * Handle a dependency between the 'enable' and remaining three
>        * configuration bits - i.e. if more than one bit in the control set has
>        * changed, including the 'enable' bit, then we want either disable the
>        * timer and perform configuration, or perform configuration and then
> @@ -582,7 +585,6 @@ static void aspeed_2600_timer_write(AspeedTimerCtrlState *s, hwaddr offset,
>       case 0x3C:
>           aspeed_timer_set_ctrl(s, s->ctrl & ~tv);
>           break;
> -
>       case 0x38:
>       default:
>           qemu_log_mask(LOG_GUEST_ERROR, "%s: Bad offset 0x%" HWADDR_PRIx "\n",
> @@ -623,7 +625,8 @@ static void aspeed_timer_reset(DeviceState *dev)
>   
>       for (i = 0; i < ASPEED_TIMER_NR_TIMERS; i++) {
>           AspeedTimer *t = &s->timers[i];
> -        /* Explicitly call helpers to avoid any conditional behaviour through
> +        /*
> +         * Explicitly call helpers to avoid any conditional behaviour through
>            * aspeed_timer_set_ctrl().
>            */
>           aspeed_timer_ctrl_enable(t, false);

[PATCH v1 3/8] hw/timer/aspeed: Fix interrupt status does not be cleared for AST2600

[Jamin Lin via]

According to the datasheet of AST2600 description, interrupt status set by HW
and clear to "0" by software writing "1" on the specific bit.

Therefore, if firmware set the specific bit "1" in the interrupt status
register(0x34), the specific bit of "s->irq_sts" should be cleared 0.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/timer/aspeed_timer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
index 5af268ea9e..149f7cc5a6 100644
--- a/hw/timer/aspeed_timer.c
+++ b/hw/timer/aspeed_timer.c
@@ -580,7 +580,7 @@ static void aspeed_2600_timer_write(AspeedTimerCtrlState *s, hwaddr offset,
 
     switch (offset) {
     case 0x34:
-        s->irq_sts &= tv;
+        s->irq_sts &= ~tv;
         break;
     case 0x3C:
         aspeed_timer_set_ctrl(s, s->ctrl & ~tv);
-- 
2.34.1

Re: [PATCH v1 3/8] hw/timer/aspeed: Fix interrupt status does not be cleared for AST2600

[Andrew Jeffery]

On Tue, 2024-10-29 at 17:17 +0800, Jamin Lin wrote:
> According to the datasheet of AST2600 description, interrupt status
> set by HW
> and clear to "0" by software writing "1" on the specific bit.
> 
> Therefore, if firmware set the specific bit "1" in the interrupt
> status
> register(0x34), the specific bit of "s->irq_sts" should be cleared 0.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>

Hah, the datasheet table for the register uses `RW` to describe the
bits and not `W1C`, but there's a foot-note in the table that says
they're W1C bits.

Fixes: fadefada4d07 ("aspeed/timer: Add support for IRQ status register on the AST2600")
Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>

Thanks,

Andrew

Re: [SPAM] [PATCH v1 3/8] hw/timer/aspeed: Fix interrupt status does not be cleared for AST2600

[Cédric Le Goater]

On 10/29/24 10:17, Jamin Lin wrote:
> According to the datasheet of AST2600 description, interrupt status set by HW
> and clear to "0" by software writing "1" on the specific bit.
> 
> Therefore, if firmware set the specific bit "1" in the interrupt status
> register(0x34), the specific bit of "s->irq_sts" should be cleared 0.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.


> ---
>   hw/timer/aspeed_timer.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/timer/aspeed_timer.c b/hw/timer/aspeed_timer.c
> index 5af268ea9e..149f7cc5a6 100644
> --- a/hw/timer/aspeed_timer.c
> +++ b/hw/timer/aspeed_timer.c
> @@ -580,7 +580,7 @@ static void aspeed_2600_timer_write(AspeedTimerCtrlState *s, hwaddr offset,
>   
>       switch (offset) {
>       case 0x34:
> -        s->irq_sts &= tv;
> +        s->irq_sts &= ~tv;
>           break;
>       case 0x3C:
>           aspeed_timer_set_ctrl(s, s->ctrl & ~tv);

[PATCH v1 4/8] 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>
---
 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

Re: [SPAM] [PATCH v1 4/8] hw/sd/sdhci: Fix coding style

[Cédric Le Goater]

On 10/29/24 10:17, 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>

Thanks,

C.


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

[PATCH v1 5/8] 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 board 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_invert"
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..bdf5cbfb80 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_invert) {
+        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-invert", SDHCIState,
+                     wp_invert, false),
     DEFINE_PROP_END_OF_LIST(),
 };
 
diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
index 6cd2822f1d..d68f4788e7 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_invert to true inverted the signal.
+     */
+    bool wp_invert;
 };
 typedef struct SDHCIState SDHCIState;
 
-- 
2.34.1

Re: [PATCH v1 5/8] hw/sd/sdhci: Introduce a new Write Protected pin inverted property

[Andrew Jeffery]

On Tue, 2024-10-29 at 17:17 +0800, 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 board
> 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_invert"
> 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..bdf5cbfb80 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_invert) {
> +        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-invert", SDHCIState,

Can we line the name up with the mmc-controller devicetree binding
("wp-inverted")?

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

Andrew

> +                     wp_invert, false),
>      DEFINE_PROP_END_OF_LIST(),
>  };
>  
> diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h
> index 6cd2822f1d..d68f4788e7 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_invert to true inverted the signal.
> +     */
> +    bool wp_invert;
>  };
>  typedef struct SDHCIState SDHCIState;
>  

RE: [PATCH v1 5/8] hw/sd/sdhci: Introduce a new Write Protected pin inverted property

[Jamin Lin]

Hi Andrew, 

> Subject: Re: [PATCH v1 5/8] hw/sd/sdhci: Introduce a new Write Protected pin
> inverted property
> 
> On Tue, 2024-10-29 at 17:17 +0800, 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 board 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_invert"
> > 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..bdf5cbfb80 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_invert) {
> > +        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-invert", SDHCIState,
> 
> Can we line the name up with the mmc-controller devicetree binding
> ("wp-inverted")?
> 
Thanks for suggestion and review.
Will update it.

Jamin
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Docume
> ntation/devicetree/bindings/mmc/mmc-controller.yaml#n71
> 
> Andrew
> 
> > +                     wp_invert, false),
> >      DEFINE_PROP_END_OF_LIST(),
> >  };
> >
> > diff --git a/include/hw/sd/sdhci.h b/include/hw/sd/sdhci.h index
> > 6cd2822f1d..d68f4788e7 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_invert to true inverted the signal.
> > +     */
> > +    bool wp_invert;
> >  };
> >  typedef struct SDHCIState SDHCIState;
> >

[PATCH v1 6/8] hw/sd/aspeed_sdhci: Introduce Capabilities Register 2 for SD slot 0 and 1

[Jamin Lin via]

The size of SDHCI capabilities register is 64bits, so introduces new
Capabilities Register 2 for SD slot 0 (0x144) and SD slot1 (0x244).

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/sd/aspeed_sdhci.c | 40 +++++++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/hw/sd/aspeed_sdhci.c b/hw/sd/aspeed_sdhci.c
index 427e5336a8..b73c18fbff 100644
--- a/hw/sd/aspeed_sdhci.c
+++ b/hw/sd/aspeed_sdhci.c
@@ -24,8 +24,10 @@
 #define  ASPEED_SDHCI_DEBOUNCE_RESET 0x00000005
 #define ASPEED_SDHCI_BUS             0x08
 #define ASPEED_SDHCI_SDIO_140        0x10
+#define ASPEED_SDHCI_SDIO_144        0x14
 #define ASPEED_SDHCI_SDIO_148        0x18
 #define ASPEED_SDHCI_SDIO_240        0x20
+#define ASPEED_SDHCI_SDIO_244        0x24
 #define ASPEED_SDHCI_SDIO_248        0x28
 #define ASPEED_SDHCI_WP_POL          0xec
 #define ASPEED_SDHCI_CARD_DET        0xf0
@@ -35,21 +37,27 @@
 
 static uint64_t aspeed_sdhci_read(void *opaque, hwaddr addr, unsigned int size)
 {
-    uint32_t val = 0;
+    uint64_t val = 0;
     AspeedSDHCIState *sdhci = opaque;
 
     switch (addr) {
     case ASPEED_SDHCI_SDIO_140:
-        val = (uint32_t)sdhci->slots[0].capareg;
+        val = extract64(sdhci->slots[0].capareg, 0, 32);
+        break;
+    case ASPEED_SDHCI_SDIO_144:
+        val = extract64(sdhci->slots[0].capareg, 32, 32);
         break;
     case ASPEED_SDHCI_SDIO_148:
-        val = (uint32_t)sdhci->slots[0].maxcurr;
+        val = extract64(sdhci->slots[0].maxcurr, 0, 32);
         break;
     case ASPEED_SDHCI_SDIO_240:
-        val = (uint32_t)sdhci->slots[1].capareg;
+        val = extract64(sdhci->slots[1].capareg, 0, 32);
+        break;
+    case ASPEED_SDHCI_SDIO_244:
+        val = extract64(sdhci->slots[1].capareg, 32, 32);
         break;
     case ASPEED_SDHCI_SDIO_248:
-        val = (uint32_t)sdhci->slots[1].maxcurr;
+         val = extract64(sdhci->slots[1].maxcurr, 0, 32);
         break;
     default:
         if (addr < ASPEED_SDHCI_REG_SIZE) {
@@ -61,9 +69,9 @@ static uint64_t aspeed_sdhci_read(void *opaque, hwaddr addr, unsigned int size)
         }
     }
 
-    trace_aspeed_sdhci_read(addr, size, (uint64_t) val);
+    trace_aspeed_sdhci_read(addr, size, val);
 
-    return (uint64_t)val;
+    return val;
 }
 
 static void aspeed_sdhci_write(void *opaque, hwaddr addr, uint64_t val,
@@ -79,16 +87,26 @@ static void aspeed_sdhci_write(void *opaque, hwaddr addr, uint64_t val,
         sdhci->regs[TO_REG(addr)] = (uint32_t)val & ~ASPEED_SDHCI_INFO_RESET;
         break;
     case ASPEED_SDHCI_SDIO_140:
-        sdhci->slots[0].capareg = (uint64_t)(uint32_t)val;
+    sdhci->slots[0].capareg = deposit64(sdhci->slots[0].capareg, 0, 32, val);
+    break;
+    case ASPEED_SDHCI_SDIO_144:
+    sdhci->slots[0].capareg = deposit64(sdhci->slots[0].capareg, 32, 32, val);
         break;
     case ASPEED_SDHCI_SDIO_148:
-        sdhci->slots[0].maxcurr = (uint64_t)(uint32_t)val;
+        sdhci->slots[0].maxcurr = deposit64(sdhci->slots[0].maxcurr,
+                                            0, 32, val);
         break;
     case ASPEED_SDHCI_SDIO_240:
-        sdhci->slots[1].capareg = (uint64_t)(uint32_t)val;
+        sdhci->slots[1].capareg = deposit64(sdhci->slots[1].capareg,
+                                            0, 32, val);
+        break;
+    case ASPEED_SDHCI_SDIO_244:
+        sdhci->slots[1].capareg = deposit64(sdhci->slots[1].capareg,
+                                            32, 32, val);
         break;
     case ASPEED_SDHCI_SDIO_248:
-        sdhci->slots[1].maxcurr = (uint64_t)(uint32_t)val;
+        sdhci->slots[1].maxcurr = deposit64(sdhci->slots[0].maxcurr,
+                                            0, 32, val);
         break;
     default:
         if (addr < ASPEED_SDHCI_REG_SIZE) {
-- 
2.34.1

Re: [SPAM] [PATCH v1 6/8] hw/sd/aspeed_sdhci: Introduce Capabilities Register 2 for SD slot 0 and 1

[Cédric Le Goater]

On 10/29/24 10:17, Jamin Lin wrote:
> The size of SDHCI capabilities register is 64bits, so introduces new
> Capabilities Register 2 for SD slot 0 (0x144) and SD slot1 (0x244).
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>

I will fix the code alignment issues.

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

> ---
>   hw/sd/aspeed_sdhci.c | 40 +++++++++++++++++++++++++++++-----------
>   1 file changed, 29 insertions(+), 11 deletions(-)
> 
> diff --git a/hw/sd/aspeed_sdhci.c b/hw/sd/aspeed_sdhci.c
> index 427e5336a8..b73c18fbff 100644
> --- a/hw/sd/aspeed_sdhci.c
> +++ b/hw/sd/aspeed_sdhci.c
> @@ -24,8 +24,10 @@
>   #define  ASPEED_SDHCI_DEBOUNCE_RESET 0x00000005
>   #define ASPEED_SDHCI_BUS             0x08
>   #define ASPEED_SDHCI_SDIO_140        0x10
> +#define ASPEED_SDHCI_SDIO_144        0x14
>   #define ASPEED_SDHCI_SDIO_148        0x18
>   #define ASPEED_SDHCI_SDIO_240        0x20
> +#define ASPEED_SDHCI_SDIO_244        0x24
>   #define ASPEED_SDHCI_SDIO_248        0x28
>   #define ASPEED_SDHCI_WP_POL          0xec
>   #define ASPEED_SDHCI_CARD_DET        0xf0
> @@ -35,21 +37,27 @@
>   
>   static uint64_t aspeed_sdhci_read(void *opaque, hwaddr addr, unsigned int size)
>   {
> -    uint32_t val = 0;
> +    uint64_t val = 0;
>       AspeedSDHCIState *sdhci = opaque;
>   
>       switch (addr) {
>       case ASPEED_SDHCI_SDIO_140:
> -        val = (uint32_t)sdhci->slots[0].capareg;
> +        val = extract64(sdhci->slots[0].capareg, 0, 32);
> +        break;
> +    case ASPEED_SDHCI_SDIO_144:
> +        val = extract64(sdhci->slots[0].capareg, 32, 32);
>           break;
>       case ASPEED_SDHCI_SDIO_148:
> -        val = (uint32_t)sdhci->slots[0].maxcurr;
> +        val = extract64(sdhci->slots[0].maxcurr, 0, 32);
>           break;
>       case ASPEED_SDHCI_SDIO_240:
> -        val = (uint32_t)sdhci->slots[1].capareg;
> +        val = extract64(sdhci->slots[1].capareg, 0, 32);
> +        break;
> +    case ASPEED_SDHCI_SDIO_244:
> +        val = extract64(sdhci->slots[1].capareg, 32, 32);
>           break;
>       case ASPEED_SDHCI_SDIO_248:
> -        val = (uint32_t)sdhci->slots[1].maxcurr;
> +         val = extract64(sdhci->slots[1].maxcurr, 0, 32);
>           break;
>       default:
>           if (addr < ASPEED_SDHCI_REG_SIZE) {
> @@ -61,9 +69,9 @@ static uint64_t aspeed_sdhci_read(void *opaque, hwaddr addr, unsigned int size)
>           }
>       }
>   
> -    trace_aspeed_sdhci_read(addr, size, (uint64_t) val);
> +    trace_aspeed_sdhci_read(addr, size, val);
>   
> -    return (uint64_t)val;
> +    return val;
>   }
>   
>   static void aspeed_sdhci_write(void *opaque, hwaddr addr, uint64_t val,
> @@ -79,16 +87,26 @@ static void aspeed_sdhci_write(void *opaque, hwaddr addr, uint64_t val,
>           sdhci->regs[TO_REG(addr)] = (uint32_t)val & ~ASPEED_SDHCI_INFO_RESET;
>           break;
>       case ASPEED_SDHCI_SDIO_140:
> -        sdhci->slots[0].capareg = (uint64_t)(uint32_t)val;
> +    sdhci->slots[0].capareg = deposit64(sdhci->slots[0].capareg, 0, 32, val);
> +    break;
> +    case ASPEED_SDHCI_SDIO_144:
> +    sdhci->slots[0].capareg = deposit64(sdhci->slots[0].capareg, 32, 32, val);
>           break;
>       case ASPEED_SDHCI_SDIO_148:
> -        sdhci->slots[0].maxcurr = (uint64_t)(uint32_t)val;
> +        sdhci->slots[0].maxcurr = deposit64(sdhci->slots[0].maxcurr,
> +                                            0, 32, val);
>           break;
>       case ASPEED_SDHCI_SDIO_240:
> -        sdhci->slots[1].capareg = (uint64_t)(uint32_t)val;
> +        sdhci->slots[1].capareg = deposit64(sdhci->slots[1].capareg,
> +                                            0, 32, val);
> +        break;
> +    case ASPEED_SDHCI_SDIO_244:
> +        sdhci->slots[1].capareg = deposit64(sdhci->slots[1].capareg,
> +                                            32, 32, val);
>           break;
>       case ASPEED_SDHCI_SDIO_248:
> -        sdhci->slots[1].maxcurr = (uint64_t)(uint32_t)val;
> +        sdhci->slots[1].maxcurr = deposit64(sdhci->slots[0].maxcurr,
> +                                            0, 32, val);
>           break;
>       default:
>           if (addr < ASPEED_SDHCI_REG_SIZE) {

Re: [PATCH v1 6/8] hw/sd/aspeed_sdhci: Introduce Capabilities Register 2 for SD slot 0 and 1

[Philippe Mathieu-Daudé]

On 29/10/24 06:17, Jamin Lin wrote:
> The size of SDHCI capabilities register is 64bits, so introduces new
> Capabilities Register 2 for SD slot 0 (0x144) and SD slot1 (0x244).
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   hw/sd/aspeed_sdhci.c | 40 +++++++++++++++++++++++++++++-----------
>   1 file changed, 29 insertions(+), 11 deletions(-)

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

[PATCH v1 7/8] 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_invert
property in ASPEED MACHINE state and set it true for AST2500 and AST2600 EVBs
and set "wp_invert" property true of sdhci-generic model.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 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 b4b1ce9efb..0468602d95 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_invert) {
+        for (i = 0; i < bmc->soc->sdhci.num_slots; i++) {
+            object_property_set_bool(OBJECT(&bmc->soc->sdhci.slots[i]),
+                                     "wp-invert", 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_invert = 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_invert = 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..879bdb96ee 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_invert;
 };
 
 
-- 
2.34.1

Re: [PATCH v1 7/8] hw/arm/aspeed: Invert sdhci write protected pin for AST2600 and AST2500 EVBs

[Andrew Jeffery]

On Tue, 2024-10-29 at 17:17 +0800, 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 AST2500 and AST2600 EVBs, the Write
> Protected pin
> is active high by default. To support it, introduces a new
> sdhci_wp_invert
> property in ASPEED MACHINE state and set it true for AST2500 and
> AST2600 EVBs
> and set "wp_invert" property true of sdhci-generic model.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>  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 b4b1ce9efb..0468602d95 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_invert) {
> +        for (i = 0; i < bmc->soc->sdhci.num_slots; i++) {
> +            object_property_set_bool(OBJECT(&bmc->soc-
> >sdhci.slots[i]),
> +                                     "wp-invert", 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_invert = 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_invert = 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..879bdb96ee 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_invert;

Other than also calling this `sdhci_wp_inverted` to match my comment on
the earlier patch about the model property and devicetree bindings,

Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>

RE: [PATCH v1 7/8] hw/arm/aspeed: Invert sdhci write protected pin for AST2600 and AST2500 EVBs

[Jamin Lin]

Hi Andrew,

> Subject: Re: [PATCH v1 7/8] hw/arm/aspeed: Invert sdhci write protected pin
> for AST2600 and AST2500 EVBs
> 
> On Tue, 2024-10-29 at 17:17 +0800, 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 AST2500 and AST2600 EVBs, the Write
> > Protected pin is active high by default. To support it, introduces a
> > new sdhci_wp_invert property in ASPEED MACHINE state and set it true
> > for AST2500 and
> > AST2600 EVBs
> > and set "wp_invert" property true of sdhci-generic model.
> >
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > ---
> >  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
> > b4b1ce9efb..0468602d95 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_invert) {
> > +        for (i = 0; i < bmc->soc->sdhci.num_slots; i++) {
> > +            object_property_set_bool(OBJECT(&bmc->soc-
> > >sdhci.slots[i]),
> > +                                     "wp-invert", 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_invert = 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_invert = 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..879bdb96ee 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_invert;
> 
> Other than also calling this `sdhci_wp_inverted` to match my comment on the
> earlier patch about the model property and devicetree bindings,
> 
Thanks for review and suggestion.
Will update it to "sdhci_wp_inverted"

Jamin
> Reviewed-by: Andrew Jeffery <andrew@codeconstruct.com.au>

[PATCH v1 8/8] aspeed: Support create flash devices via command line for AST1030

[Jamin Lin via]

Add a "if-statement" in aspeed_minibmc_machine_init function. If users add
"-nodefaults" in command line, the flash devices should be created by users
setting. Otherwise, the flash devices are created at machine init.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/arm/aspeed.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 0468602d95..e161e6b1c5 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -1602,18 +1602,20 @@ static void aspeed_minibmc_machine_init(MachineState *machine)
     connect_serial_hds_to_uarts(bmc);
     qdev_realize(DEVICE(bmc->soc), NULL, &error_abort);
 
-    aspeed_board_init_flashes(&bmc->soc->fmc,
-                              bmc->fmc_model ? bmc->fmc_model : amc->fmc_model,
-                              amc->num_cs,
-                              0);
+    if (defaults_enabled()) {
+        aspeed_board_init_flashes(&bmc->soc->fmc,
+                            bmc->fmc_model ? bmc->fmc_model : amc->fmc_model,
+                            amc->num_cs,
+                            0);
 
-    aspeed_board_init_flashes(&bmc->soc->spi[0],
-                              bmc->spi_model ? bmc->spi_model : amc->spi_model,
-                              amc->num_cs, amc->num_cs);
+        aspeed_board_init_flashes(&bmc->soc->spi[0],
+                            bmc->spi_model ? bmc->spi_model : amc->spi_model,
+                            amc->num_cs, amc->num_cs);
 
-    aspeed_board_init_flashes(&bmc->soc->spi[1],
-                              bmc->spi_model ? bmc->spi_model : amc->spi_model,
-                              amc->num_cs, (amc->num_cs * 2));
+        aspeed_board_init_flashes(&bmc->soc->spi[1],
+                            bmc->spi_model ? bmc->spi_model : amc->spi_model,
+                            amc->num_cs, (amc->num_cs * 2));
+    }
 
     if (amc->i2c_init) {
         amc->i2c_init(bmc);
-- 
2.34.1

Re: [SPAM] [PATCH v1 8/8] aspeed: Support create flash devices via command line for AST1030

[Cédric Le Goater]

On 10/29/24 10:17, Jamin Lin wrote:
> Add a "if-statement" in aspeed_minibmc_machine_init function. If users add
> "-nodefaults" in command line, the flash devices should be created by users
> setting. Otherwise, the flash devices are created at machine init.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>



Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.

> ---
>   hw/arm/aspeed.c | 22 ++++++++++++----------
>   1 file changed, 12 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 0468602d95..e161e6b1c5 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -1602,18 +1602,20 @@ static void aspeed_minibmc_machine_init(MachineState *machine)
>       connect_serial_hds_to_uarts(bmc);
>       qdev_realize(DEVICE(bmc->soc), NULL, &error_abort);
>   
> -    aspeed_board_init_flashes(&bmc->soc->fmc,
> -                              bmc->fmc_model ? bmc->fmc_model : amc->fmc_model,
> -                              amc->num_cs,
> -                              0);
> +    if (defaults_enabled()) {
> +        aspeed_board_init_flashes(&bmc->soc->fmc,
> +                            bmc->fmc_model ? bmc->fmc_model : amc->fmc_model,
> +                            amc->num_cs,
> +                            0);
>   
> -    aspeed_board_init_flashes(&bmc->soc->spi[0],
> -                              bmc->spi_model ? bmc->spi_model : amc->spi_model,
> -                              amc->num_cs, amc->num_cs);
> +        aspeed_board_init_flashes(&bmc->soc->spi[0],
> +                            bmc->spi_model ? bmc->spi_model : amc->spi_model,
> +                            amc->num_cs, amc->num_cs);
>   
> -    aspeed_board_init_flashes(&bmc->soc->spi[1],
> -                              bmc->spi_model ? bmc->spi_model : amc->spi_model,
> -                              amc->num_cs, (amc->num_cs * 2));
> +        aspeed_board_init_flashes(&bmc->soc->spi[1],
> +                            bmc->spi_model ? bmc->spi_model : amc->spi_model,
> +                            amc->num_cs, (amc->num_cs * 2));
> +    }
>   
>       if (amc->i2c_init) {
>           amc->i2c_init(bmc);

Re: [SPAM] [PATCH v1 0/8] Support RTC for AST2700

[Cédric Le Goater]

On 10/29/24 10:17, 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.
> 
> Jamin Lin (8):
>    aspeed/soc: Support RTC for AST2700
>    hw/timer/aspeed: Fix coding style
>    hw/timer/aspeed: Fix interrupt status does not be cleared for AST2600
>    hw/sd/sdhci: Fix coding style
>    hw/sd/sdhci: Introduce a new Write Protected pin inverted property
>    hw/sd/aspeed_sdhci: Introduce Capabilities Register 2 for SD slot 0
>      and 1
>    hw/arm/aspeed: Invert sdhci write protected pin for AST2600 and
>      AST2500 EVBs
>    aspeed: Support create flash devices via command line for AST1030
> 
>   hw/arm/aspeed.c         | 30 ++++++++++++------
>   hw/arm/aspeed_ast27x0.c | 11 +++++++
>   hw/sd/aspeed_sdhci.c    | 40 ++++++++++++++++-------
>   hw/sd/sdhci.c           | 70 ++++++++++++++++++++++++++++-------------
>   hw/timer/aspeed_timer.c | 15 +++++----
>   include/hw/arm/aspeed.h |  1 +
>   include/hw/sd/sdhci.h   |  5 +++
>   7 files changed, 123 insertions(+), 49 deletions(-)
> 

Applied 1,2,3,6,8 to aspeed-next.

Thanks,

C.

RE: [SPAM] [PATCH v1 0/8] Support RTC for AST2700

[Jamin Lin]

Hi Cedric,

> Subject: Re: [SPAM] [PATCH v1 0/8] Support RTC for AST2700
> 
> On 10/29/24 10:17, 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.
> >
> > Jamin Lin (8):
> >    aspeed/soc: Support RTC for AST2700
> >    hw/timer/aspeed: Fix coding style
> >    hw/timer/aspeed: Fix interrupt status does not be cleared for AST2600
> >    hw/sd/sdhci: Fix coding style
> >    hw/sd/sdhci: Introduce a new Write Protected pin inverted property
> >    hw/sd/aspeed_sdhci: Introduce Capabilities Register 2 for SD slot 0
> >      and 1
> >    hw/arm/aspeed: Invert sdhci write protected pin for AST2600 and
> >      AST2500 EVBs
> >    aspeed: Support create flash devices via command line for AST1030
> >
> >   hw/arm/aspeed.c         | 30 ++++++++++++------
> >   hw/arm/aspeed_ast27x0.c | 11 +++++++
> >   hw/sd/aspeed_sdhci.c    | 40 ++++++++++++++++-------
> >   hw/sd/sdhci.c           | 70
> ++++++++++++++++++++++++++++-------------
> >   hw/timer/aspeed_timer.c | 15 +++++----
> >   include/hw/arm/aspeed.h |  1 +
> >   include/hw/sd/sdhci.h   |  5 +++
> >   7 files changed, 123 insertions(+), 49 deletions(-)
> >
> 
> Applied 1,2,3,6,8 to aspeed-next.
> 

Thanks for your help.
Will resend 4,5 and 7 patches in v2.
Thanks-Jamin
> Thanks,
> 
> C.
>