According to the design of ASPEED SOCS, the uart controller
is 1 base for ast10x0, ast2600, ast2500 and ast2400.

However, the uart controller is 0 base for ast2700.
To support uart controller both 0 and 1 base,
adds uasrt_bases parameter in AspeedSoCClass
and set the default uart controller 1 base
for ast10x0, astt2600, ast2500 and ast2400.

From datasheet description
ast2700:
Base Address of UART0 = 0x14c33000
ast1030:
Base Address of UART1 = 0x7e783000
ast2600:
Base Address of UART1 = 0x1E78 3000
ast2500:
Base Address of UART1 = 0x1E78 3000

Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/arm/aspeed.c             | 8 +++++---
 hw/arm/aspeed_ast10x0.c     | 1 +
 hw/arm/aspeed_ast2400.c     | 2 ++
 hw/arm/aspeed_ast2600.c     | 1 +
 hw/arm/aspeed_soc_common.c  | 4 ++--
 include/hw/arm/aspeed_soc.h | 1 +
 6 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 09b1e823ba..218b81298e 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -342,7 +342,7 @@ static void connect_serial_hds_to_uarts(AspeedMachineState *bmc)
     int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default;
 
     aspeed_soc_uart_set_chr(s, uart_chosen, serial_hd(0));
-    for (int i = 1, uart = ASPEED_DEV_UART1; i < sc->uarts_num; i++, uart++) {
+    for (int i = 1, uart = sc->uarts_base; i < sc->uarts_num; i++, uart++) {
         if (uart == uart_chosen) {
             continue;
         }
@@ -1092,9 +1092,11 @@ static char *aspeed_get_bmc_console(Object *obj, Error **errp)
 {
     AspeedMachineState *bmc = ASPEED_MACHINE(obj);
     AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
+    AspeedSoCClass *sc = ASPEED_SOC_CLASS(obj);
+
     int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default;
 
-    return g_strdup_printf("uart%d", uart_chosen - ASPEED_DEV_UART1 + 1);
+    return g_strdup_printf("uart%d", uart_chosen - sc->uarts_base + 1);
 }
 
 static void aspeed_set_bmc_console(Object *obj, const char *value, Error **errp)
@@ -1114,7 +1116,7 @@ static void aspeed_set_bmc_console(Object *obj, const char *value, Error **errp)
         error_setg(errp, "\"uart\" should be in range [1 - %d]", sc->uarts_num);
         return;
     }
-    bmc->uart_chosen = ASPEED_DEV_UART1 + val - 1;
+    bmc->uart_chosen = sc->uarts_base + val - 1;
 }
 
 static void aspeed_machine_class_props_init(ObjectClass *oc)
diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
index c3b5116a6a..2634e0f654 100644
--- a/hw/arm/aspeed_ast10x0.c
+++ b/hw/arm/aspeed_ast10x0.c
@@ -436,6 +436,7 @@ static void aspeed_soc_ast1030_class_init(ObjectClass *klass, void *data)
     sc->wdts_num = 4;
     sc->macs_num = 1;
     sc->uarts_num = 13;
+    sc->uarts_base = ASPEED_DEV_UART1;
     sc->irqmap = aspeed_soc_ast1030_irqmap;
     sc->memmap = aspeed_soc_ast1030_memmap;
     sc->num_cpus = 1;
diff --git a/hw/arm/aspeed_ast2400.c b/hw/arm/aspeed_ast2400.c
index 8829561bb6..95da85fee0 100644
--- a/hw/arm/aspeed_ast2400.c
+++ b/hw/arm/aspeed_ast2400.c
@@ -523,6 +523,7 @@ static void aspeed_soc_ast2400_class_init(ObjectClass *oc, void *data)
     sc->wdts_num     = 2;
     sc->macs_num     = 2;
     sc->uarts_num    = 5;
+    sc->uarts_base   = ASPEED_DEV_UART1;
     sc->irqmap       = aspeed_soc_ast2400_irqmap;
     sc->memmap       = aspeed_soc_ast2400_memmap;
     sc->num_cpus     = 1;
@@ -551,6 +552,7 @@ static void aspeed_soc_ast2500_class_init(ObjectClass *oc, void *data)
     sc->wdts_num     = 3;
     sc->macs_num     = 2;
     sc->uarts_num    = 5;
+    sc->uarts_base   = ASPEED_DEV_UART1;
     sc->irqmap       = aspeed_soc_ast2500_irqmap;
     sc->memmap       = aspeed_soc_ast2500_memmap;
     sc->num_cpus     = 1;
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 4ee32ea99d..f74561ecdc 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -666,6 +666,7 @@ static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
     sc->wdts_num     = 4;
     sc->macs_num     = 4;
     sc->uarts_num    = 13;
+    sc->uarts_base   = ASPEED_DEV_UART1;
     sc->irqmap       = aspeed_soc_ast2600_irqmap;
     sc->memmap       = aspeed_soc_ast2600_memmap;
     sc->num_cpus     = 2;
diff --git a/hw/arm/aspeed_soc_common.c b/hw/arm/aspeed_soc_common.c
index 123a0c432c..3963436c3a 100644
--- a/hw/arm/aspeed_soc_common.c
+++ b/hw/arm/aspeed_soc_common.c
@@ -36,7 +36,7 @@ bool aspeed_soc_uart_realize(AspeedSoCState *s, Error **errp)
     AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
     SerialMM *smm;
 
-    for (int i = 0, uart = ASPEED_DEV_UART1; i < sc->uarts_num; i++, uart++) {
+    for (int i = 0, uart = sc->uarts_base; i < sc->uarts_num; i++, uart++) {
         smm = &s->uart[i];
 
         /* Chardev property is set by the machine. */
@@ -58,7 +58,7 @@ bool aspeed_soc_uart_realize(AspeedSoCState *s, Error **errp)
 void aspeed_soc_uart_set_chr(AspeedSoCState *s, int dev, Chardev *chr)
 {
     AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
-    int i = dev - ASPEED_DEV_UART1;
+    int i = dev - sc->uarts_base;
 
     g_assert(0 <= i && i < ARRAY_SIZE(s->uart) && i < sc->uarts_num);
     qdev_prop_set_chr(DEVICE(&s->uart[i]), "chardev", chr);
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index 9d0af84a8c..ce2bb51682 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -140,6 +140,7 @@ struct AspeedSoCClass {
     int wdts_num;
     int macs_num;
     int uarts_num;
+    int uarts_base;
     const int *irqmap;
     const hwaddr *memmap;
     uint32_t num_cpus;
-- 
2.34.1

Hello Jmain,

On 2/5/24 10:14, Jamin Lin wrote:
> According to the design of ASPEED SOCS, the uart controller
> is 1 base for ast10x0, ast2600, ast2500 and ast2400.

Please rephrase saying somehting :

the Aspeed datasheet refers to the UART controllers as UART1 - UART13
for the ast10x0, ast2600, ast2500 and ast2400 SoCs and the Aspeed
ast2700 introduces an UART0. To keep the naming in the QEMU models
in sync with the datasheet, let's introduce a new  UART0 device name
and do the required adjustements, etc ...

> However, the uart controller is 0 base for ast2700.
> To support uart controller both 0 and 1 base,
> adds uasrt_bases parameter in AspeedSoCClass
> and set the default uart controller 1 base
> for ast10x0, astt2600, ast2500 and ast2400.
> 
>  From datasheet description
> ast2700:
> Base Address of UART0 = 0x14c33000
> ast1030:
> Base Address of UART1 = 0x7e783000
> ast2600:
> Base Address of UART1 = 0x1E78 3000
> ast2500:
> Base Address of UART1 = 0x1E78 3000

We should also introduce ASPEED_DEV_UART0 enum. See below.

> Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   hw/arm/aspeed.c             | 8 +++++---
>   hw/arm/aspeed_ast10x0.c     | 1 +
>   hw/arm/aspeed_ast2400.c     | 2 ++
>   hw/arm/aspeed_ast2600.c     | 1 +
>   hw/arm/aspeed_soc_common.c  | 4 ++--
>   include/hw/arm/aspeed_soc.h | 1 +
>   6 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 09b1e823ba..218b81298e 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -342,7 +342,7 @@ static void connect_serial_hds_to_uarts(AspeedMachineState *bmc)
>       int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default;
>   
>       aspeed_soc_uart_set_chr(s, uart_chosen, serial_hd(0));
> -    for (int i = 1, uart = ASPEED_DEV_UART1; i < sc->uarts_num; i++, uart++) {
> +    for (int i = 1, uart = sc->uarts_base; i < sc->uarts_num; i++, uart++) {
>           if (uart == uart_chosen) {
>               continue;
>           }
> @@ -1092,9 +1092,11 @@ static char *aspeed_get_bmc_console(Object *obj, Error **errp)
>   {
>       AspeedMachineState *bmc = ASPEED_MACHINE(obj);
>       AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
> +    AspeedSoCClass *sc = ASPEED_SOC_CLASS(obj);
> +
>       int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default;
>   
> -    return g_strdup_printf("uart%d", uart_chosen - ASPEED_DEV_UART1 + 1);
> +    return g_strdup_printf("uart%d", uart_chosen - sc->uarts_base + 1);

Wwe didn't have a ASPEED_DEV_UART0 at the time. The calculation above should
be replaced with : "uart_chosen - ASPEED_DEV_UART0"

>   }
>   
>   static void aspeed_set_bmc_console(Object *obj, const char *value, Error **errp)
> @@ -1114,7 +1116,7 @@ static void aspeed_set_bmc_console(Object *obj, const char *value, Error **errp)
>           error_setg(errp, "\"uart\" should be in range [1 - %d]", sc->uarts_num);

The range in the reported error above needs a fix. It's not "1" anymore
but "sc->uarts_base - ASPEED_DEV_UART0". Same for the test :

       if (val < 1 || val > sc->uarts_num) {


>           return;
>       }
> -    bmc->uart_chosen = ASPEED_DEV_UART1 + val - 1;
> +    bmc->uart_chosen = sc->uarts_base + val - 1;

Should be ASPEED_DEV_UART0 + val.


Thanks,

C.



>   }
>   
>   static void aspeed_machine_class_props_init(ObjectClass *oc)
> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
> index c3b5116a6a..2634e0f654 100644
> --- a/hw/arm/aspeed_ast10x0.c
> +++ b/hw/arm/aspeed_ast10x0.c
> @@ -436,6 +436,7 @@ static void aspeed_soc_ast1030_class_init(ObjectClass *klass, void *data)
>       sc->wdts_num = 4;
>       sc->macs_num = 1;
>       sc->uarts_num = 13;
> +    sc->uarts_base = ASPEED_DEV_UART1;
>       sc->irqmap = aspeed_soc_ast1030_irqmap;
>       sc->memmap = aspeed_soc_ast1030_memmap;
>       sc->num_cpus = 1;
> diff --git a/hw/arm/aspeed_ast2400.c b/hw/arm/aspeed_ast2400.c
> index 8829561bb6..95da85fee0 100644
> --- a/hw/arm/aspeed_ast2400.c
> +++ b/hw/arm/aspeed_ast2400.c
> @@ -523,6 +523,7 @@ static void aspeed_soc_ast2400_class_init(ObjectClass *oc, void *data)
>       sc->wdts_num     = 2;
>       sc->macs_num     = 2;
>       sc->uarts_num    = 5;
> +    sc->uarts_base   = ASPEED_DEV_UART1;
>       sc->irqmap       = aspeed_soc_ast2400_irqmap;
>       sc->memmap       = aspeed_soc_ast2400_memmap;
>       sc->num_cpus     = 1;
> @@ -551,6 +552,7 @@ static void aspeed_soc_ast2500_class_init(ObjectClass *oc, void *data)
>       sc->wdts_num     = 3;
>       sc->macs_num     = 2;
>       sc->uarts_num    = 5;
> +    sc->uarts_base   = ASPEED_DEV_UART1;
>       sc->irqmap       = aspeed_soc_ast2500_irqmap;
>       sc->memmap       = aspeed_soc_ast2500_memmap;
>       sc->num_cpus     = 1;
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index 4ee32ea99d..f74561ecdc 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -666,6 +666,7 @@ static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
>       sc->wdts_num     = 4;
>       sc->macs_num     = 4;
>       sc->uarts_num    = 13;
> +    sc->uarts_base   = ASPEED_DEV_UART1;
>       sc->irqmap       = aspeed_soc_ast2600_irqmap;
>       sc->memmap       = aspeed_soc_ast2600_memmap;
>       sc->num_cpus     = 2;
> diff --git a/hw/arm/aspeed_soc_common.c b/hw/arm/aspeed_soc_common.c
> index 123a0c432c..3963436c3a 100644
> --- a/hw/arm/aspeed_soc_common.c
> +++ b/hw/arm/aspeed_soc_common.c
> @@ -36,7 +36,7 @@ bool aspeed_soc_uart_realize(AspeedSoCState *s, Error **errp)
>       AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
>       SerialMM *smm;
>   
> -    for (int i = 0, uart = ASPEED_DEV_UART1; i < sc->uarts_num; i++, uart++) {
> +    for (int i = 0, uart = sc->uarts_base; i < sc->uarts_num; i++, uart++) {
>           smm = &s->uart[i];
>   
>           /* Chardev property is set by the machine. */
> @@ -58,7 +58,7 @@ bool aspeed_soc_uart_realize(AspeedSoCState *s, Error **errp)
>   void aspeed_soc_uart_set_chr(AspeedSoCState *s, int dev, Chardev *chr)
>   {
>       AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
> -    int i = dev - ASPEED_DEV_UART1;
> +    int i = dev - sc->uarts_base;
>   
>       g_assert(0 <= i && i < ARRAY_SIZE(s->uart) && i < sc->uarts_num);
>       qdev_prop_set_chr(DEVICE(&s->uart[i]), "chardev", chr);
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index 9d0af84a8c..ce2bb51682 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -140,6 +140,7 @@ struct AspeedSoCClass {
>       int wdts_num;
>       int macs_num;
>       int uarts_num;
> +    int uarts_base;
>       const int *irqmap;
>       const hwaddr *memmap;
>       uint32_t num_cpus;

Hello Jamin,

On 2/5/24 10:14, Jamin Lin wrote:
> According to the design of ASPEED SOCS, the uart controller
> is 1 base for ast10x0, ast2600, ast2500 and ast2400.
>
> However, the uart controller is 0 base for ast2700.
> To support uart controller both 0 and 1 base,
> adds uasrt_bases parameter in AspeedSoCClass
> and set the default uart controller 1 base
> for ast10x0, astt2600, ast2500 and ast2400.

The board definition can set 'amc->uart_default' to choose a different
default serial port for the console, or use the "bmc-console" machine
option . Isn't it enough ? May be I am misunderstanding the need.

To clarify,

ASPEED_DEV_UART1 is in the first serial port on the boards.

I think we chose to start the indexing at 1 because the Aspeed QEMU
modeling began first with the UART model (console) and for simplicity,
we copied the definitions of the device tree from Linux :

     serial0 = &uart1;
     serial1 = &uart2;
     serial2 = &uart3;
     serial3 = &uart4;
     serial4 = &uart5;
     serial5 = &vuart;

We replicated this indexing starting at 1 to nearly all device models :
  
     ASPEED_DEV_UART1 - 13
     ASPEED_DEV_SPI1 -2
     ASPEED_DEV_EHCI1 -2
     ASPEED_DEV_TIMER1 - 8
     ASPEED_DEV_ETH1 -4
     ASPEED_DEV_MII1 - 4
     ASPEED_DEV_JTAG0 - 1      <--- !!
     ASPEED_DEV_FSI1 - 2

I don't know what would be ASPEED_DEV_UART0 in this context.

May be you could send a simplified AST2700 SoC model with definitions
of a minimum address space and IRQ space ?

Or you could change the indexing to start at 0 if you prefer. Just be
careful with the aspeed_set/get_bmc_console routines it you choose to.

Thanks,

C.


>  From datasheet description
> ast2700:
> Base Address of UART0 = 0x14c33000
> ast1030:
> Base Address of UART1 = 0x7e783000
> ast2600:
> Base Address of UART1 = 0x1E78 3000
> ast2500:
> Base Address of UART1 = 0x1E78 3000
> 
> Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   hw/arm/aspeed.c             | 8 +++++---
>   hw/arm/aspeed_ast10x0.c     | 1 +
>   hw/arm/aspeed_ast2400.c     | 2 ++
>   hw/arm/aspeed_ast2600.c     | 1 +
>   hw/arm/aspeed_soc_common.c  | 4 ++--
>   include/hw/arm/aspeed_soc.h | 1 +
>   6 files changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 09b1e823ba..218b81298e 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -342,7 +342,7 @@ static void connect_serial_hds_to_uarts(AspeedMachineState *bmc)
>       int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default;
>   
>       aspeed_soc_uart_set_chr(s, uart_chosen, serial_hd(0));
> -    for (int i = 1, uart = ASPEED_DEV_UART1; i < sc->uarts_num; i++, uart++) {
> +    for (int i = 1, uart = sc->uarts_base; i < sc->uarts_num; i++, uart++) {
>           if (uart == uart_chosen) {
>               continue;
>           }
> @@ -1092,9 +1092,11 @@ static char *aspeed_get_bmc_console(Object *obj, Error **errp)
>   {
>       AspeedMachineState *bmc = ASPEED_MACHINE(obj);
>       AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
> +    AspeedSoCClass *sc = ASPEED_SOC_CLASS(obj);
> +
>       int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default;
>   
> -    return g_strdup_printf("uart%d", uart_chosen - ASPEED_DEV_UART1 + 1);
> +    return g_strdup_printf("uart%d", uart_chosen - sc->uarts_base + 1);
>   }
>   
>   static void aspeed_set_bmc_console(Object *obj, const char *value, Error **errp)
> @@ -1114,7 +1116,7 @@ static void aspeed_set_bmc_console(Object *obj, const char *value, Error **errp)
>           error_setg(errp, "\"uart\" should be in range [1 - %d]", sc->uarts_num);
>           return;
>       }
> -    bmc->uart_chosen = ASPEED_DEV_UART1 + val - 1;
> +    bmc->uart_chosen = sc->uarts_base + val - 1;
>   }
>   
>   static void aspeed_machine_class_props_init(ObjectClass *oc)
> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
> index c3b5116a6a..2634e0f654 100644
> --- a/hw/arm/aspeed_ast10x0.c
> +++ b/hw/arm/aspeed_ast10x0.c
> @@ -436,6 +436,7 @@ static void aspeed_soc_ast1030_class_init(ObjectClass *klass, void *data)
>       sc->wdts_num = 4;
>       sc->macs_num = 1;
>       sc->uarts_num = 13;
> +    sc->uarts_base = ASPEED_DEV_UART1;
>       sc->irqmap = aspeed_soc_ast1030_irqmap;
>       sc->memmap = aspeed_soc_ast1030_memmap;
>       sc->num_cpus = 1;
> diff --git a/hw/arm/aspeed_ast2400.c b/hw/arm/aspeed_ast2400.c
> index 8829561bb6..95da85fee0 100644
> --- a/hw/arm/aspeed_ast2400.c
> +++ b/hw/arm/aspeed_ast2400.c
> @@ -523,6 +523,7 @@ static void aspeed_soc_ast2400_class_init(ObjectClass *oc, void *data)
>       sc->wdts_num     = 2;
>       sc->macs_num     = 2;
>       sc->uarts_num    = 5;
> +    sc->uarts_base   = ASPEED_DEV_UART1;
>       sc->irqmap       = aspeed_soc_ast2400_irqmap;
>       sc->memmap       = aspeed_soc_ast2400_memmap;
>       sc->num_cpus     = 1;
> @@ -551,6 +552,7 @@ static void aspeed_soc_ast2500_class_init(ObjectClass *oc, void *data)
>       sc->wdts_num     = 3;
>       sc->macs_num     = 2;
>       sc->uarts_num    = 5;
> +    sc->uarts_base   = ASPEED_DEV_UART1;
>       sc->irqmap       = aspeed_soc_ast2500_irqmap;
>       sc->memmap       = aspeed_soc_ast2500_memmap;
>       sc->num_cpus     = 1;
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index 4ee32ea99d..f74561ecdc 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -666,6 +666,7 @@ static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
>       sc->wdts_num     = 4;
>       sc->macs_num     = 4;
>       sc->uarts_num    = 13;
> +    sc->uarts_base   = ASPEED_DEV_UART1;
>       sc->irqmap       = aspeed_soc_ast2600_irqmap;
>       sc->memmap       = aspeed_soc_ast2600_memmap;
>       sc->num_cpus     = 2;
> diff --git a/hw/arm/aspeed_soc_common.c b/hw/arm/aspeed_soc_common.c
> index 123a0c432c..3963436c3a 100644
> --- a/hw/arm/aspeed_soc_common.c
> +++ b/hw/arm/aspeed_soc_common.c
> @@ -36,7 +36,7 @@ bool aspeed_soc_uart_realize(AspeedSoCState *s, Error **errp)
>       AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
>       SerialMM *smm;
>   
> -    for (int i = 0, uart = ASPEED_DEV_UART1; i < sc->uarts_num; i++, uart++) {
> +    for (int i = 0, uart = sc->uarts_base; i < sc->uarts_num; i++, uart++) {
>           smm = &s->uart[i];
>   
>           /* Chardev property is set by the machine. */
> @@ -58,7 +58,7 @@ bool aspeed_soc_uart_realize(AspeedSoCState *s, Error **errp)
>   void aspeed_soc_uart_set_chr(AspeedSoCState *s, int dev, Chardev *chr)
>   {
>       AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
> -    int i = dev - ASPEED_DEV_UART1;
> +    int i = dev - sc->uarts_base;
>   
>       g_assert(0 <= i && i < ARRAY_SIZE(s->uart) && i < sc->uarts_num);
>       qdev_prop_set_chr(DEVICE(&s->uart[i]), "chardev", chr);
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index 9d0af84a8c..ce2bb51682 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -140,6 +140,7 @@ struct AspeedSoCClass {
>       int wdts_num;
>       int macs_num;
>       int uarts_num;
> +    int uarts_base;
>       const int *irqmap;
>       const hwaddr *memmap;
>       uint32_t num_cpus;

On 2/5/24 11:46, Cédric Le Goater wrote:
> Hello Jamin,
> 
> On 2/5/24 10:14, Jamin Lin wrote:
>> According to the design of ASPEED SOCS, the uart controller
>> is 1 base for ast10x0, ast2600, ast2500 and ast2400.
>>
>> However, the uart controller is 0 base for ast2700.
>> To support uart controller both 0 and 1 base,
>> adds uasrt_bases parameter in AspeedSoCClass
>> and set the default uart controller 1 base
>> for ast10x0, astt2600, ast2500 and ast2400.
> 
> The board definition can set 'amc->uart_default' to choose a different
> default serial port for the console, or use the "bmc-console" machine
> option . Isn't it enough ? May be I am misunderstanding the need.
> 
> To clarify,
> 
> ASPEED_DEV_UART1 is in the first serial port on the boards.
> 
> I think we chose to start the indexing at 1 because the Aspeed QEMU
> modeling began first with the UART model (console) and for simplicity,
> we copied the definitions of the device tree from Linux :
> 
>      serial0 = &uart1;
>      serial1 = &uart2;
>      serial2 = &uart3;
>      serial3 = &uart4;
>      serial4 = &uart5;
>      serial5 = &vuart;

The uart definitions on the AST2700 are different :
   
   https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi

	serial0 = &uart0;
	serial1 = &uart1;
	serial2 = &uart2;
	serial3 = &uart3;
	serial4 = &uart4;
	serial5 = &uart5;
	serial6 = &uart6;
	serial7 = &uart7;
	serial8 = &uart8;
         ...

I think the names in the DT (and consequently in the QEMU models)
follow the IP names in the datasheet.

I don't think we care in QEMU, so I would be inclined to change the
indexing of the device names in QEMU and start at 0, which would
introduce a discrepancy for the AST2400, AST2600, AST2600 SoC.

Let's see what the other maintainers have to say.

Thanks,

C.

> -----Original Message----- 
> The uart definitions on the AST2700 are different :
> 
> 
> https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm
> 64/boot/dts/aspeed/aspeed-g7.dtsi
> 
> 	serial0 = &uart0;
> 	serial1 = &uart1;
> 	serial2 = &uart2;
> 	serial3 = &uart3;
> 	serial4 = &uart4;
> 	serial5 = &uart5;
> 	serial6 = &uart6;
> 	serial7 = &uart7;
> 	serial8 = &uart8;
>          ...
> 
> I think the names in the DT (and consequently in the QEMU models) follow the
> IP names in the datasheet.
> 
> I don't think we care in QEMU, so I would be inclined to change the indexing of
> the device names in QEMU and start at 0, which would introduce a
> discrepancy for the AST2400, AST2600, AST2600 SoC.
> 
> Let's see what the other maintainers have to say.
> 
> Thanks,
> 
> C.
Hi Cedric, 

Did you mean to change the naming of uart device to 0 base for all ASPEED SOCs?
If yes, it seems we need to do the following changes.
1. add ASPEED_DEV_UART0 in aspeed_soc.h
2. Re-defined uart memory map for ast2600, ast10x0, ast2500 and ast2400(uart0 -> ASPEED_DEV_UART0)
Take ast2600 for example:
static const hwaddr aspeed_soc_ast2600_memmap[] = {
    [ASPEED_DEV_UART1]     = 0x1E783000, ---> [ASPEED_DEV_UART0]
    [ASPEED_DEV_UART2]     = 0x1E78D000, ---> [ASPEED_DEV_UART1]
    [ASPEED_DEV_UART3]     = 0x1E78E000,
    [ASPEED_DEV_UART4]     = 0x1E78F000,
    [ASPEED_DEV_UART5]     = 0x1E784000,
    [ASPEED_DEV_UART6]     = 0x1E790000,
    [ASPEED_DEV_UART7]     = 0x1E790100,
    [ASPEED_DEV_UART8]     = 0x1E790200,
    [ASPEED_DEV_UART9]     = 0x1E790300,
    [ASPEED_DEV_UART10]    = 0x1E790400,
    [ASPEED_DEV_UART11]    = 0x1E790500,
    [ASPEED_DEV_UART12]    = 0x1E790600,
    [ASPEED_DEV_UART13]    = 0x1E790700, ---> [ASPEED_DEV_UART12]
}; 
If no, could you please descript it more detail? So, I can change it and re-send this patch series.

By the way, I will send a new patch series to support AST2700 in two weeks.
We encountered GIC issues. It seems that QEMU support GIC v3 but SPI did not support, yet.
https://github.com/qemu/qemu/blob/master/hw/intc/arm_gicv3_dist.c#L383
https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi#L229
It think that we can discuss it in a new AST2700 patch series.
Thanks-Jamin

On 2/6/24 04:29, Jamin Lin wrote:
>> -----Original Message-----
>> The uart definitions on the AST2700 are different :
>>
>>
>> https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm
>> 64/boot/dts/aspeed/aspeed-g7.dtsi
>>
>> 	serial0 = &uart0;
>> 	serial1 = &uart1;
>> 	serial2 = &uart2;
>> 	serial3 = &uart3;
>> 	serial4 = &uart4;
>> 	serial5 = &uart5;
>> 	serial6 = &uart6;
>> 	serial7 = &uart7;
>> 	serial8 = &uart8;
>>           ...
>>
>> I think the names in the DT (and consequently in the QEMU models) follow the
>> IP names in the datasheet.
>>
>> I don't think we care in QEMU, so I would be inclined to change the indexing of
>> the device names in QEMU and start at 0, which would introduce a
>> discrepancy for the AST2400, AST2600, AST2600 SoC.
>>
>> Let's see what the other maintainers have to say.
>>
>> Thanks,
>>
>> C.
> Hi Cedric,
> 
> Did you mean to change the naming of uart device to 0 base for all ASPEED SOCs?
> If yes, it seems we need to do the following changes.
> 1. add ASPEED_DEV_UART0 in aspeed_soc.h
> 2. Re-defined uart memory map for ast2600, ast10x0, ast2500 and ast2400(uart0 -> ASPEED_DEV_UART0)
> Take ast2600 for example:
> static const hwaddr aspeed_soc_ast2600_memmap[] = {
>      [ASPEED_DEV_UART1]     = 0x1E783000, ---> [ASPEED_DEV_UART0]
>      [ASPEED_DEV_UART2]     = 0x1E78D000, ---> [ASPEED_DEV_UART1]
>      [ASPEED_DEV_UART3]     = 0x1E78E000,
>      [ASPEED_DEV_UART4]     = 0x1E78F000,
>      [ASPEED_DEV_UART5]     = 0x1E784000,
>      [ASPEED_DEV_UART6]     = 0x1E790000,
>      [ASPEED_DEV_UART7]     = 0x1E790100,
>      [ASPEED_DEV_UART8]     = 0x1E790200,
>      [ASPEED_DEV_UART9]     = 0x1E790300,
>      [ASPEED_DEV_UART10]    = 0x1E790400,
>      [ASPEED_DEV_UART11]    = 0x1E790500,
>      [ASPEED_DEV_UART12]    = 0x1E790600,
>      [ASPEED_DEV_UART13]    = 0x1E790700, ---> [ASPEED_DEV_UART12]
> };
> If no, could you please descript it more detail? So, I can change it and re-send this patch series.

Let's keep the datasheet names. I had forgotten the reason initially
and from an HW POV it makes sense to keep them in sync. I will add
some more comments to the patch.
  
> By the way, I will send a new patch series to support AST2700 in two weeks.
> We encountered GIC issues. It seems that QEMU support GIC v3 but SPI did not support, yet.
>
> https://github.com/qemu/qemu/blob/master/hw/intc/arm_gicv3_dist.c#L383
> https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi#L229

If you did any hacks or workarounds in the QEMU models, please keep them
separate from the other patches so that we can discuss.

> It think that we can discuss it in a new AST2700 patch series.
Sure.

Thanks,

C.

> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Wednesday, February 7, 2024 1:00 AM
> To: Jamin Lin <jamin_lin@aspeedtech.com>; Peter Maydell
> <peter.maydell@linaro.org>; Andrew Jeffery <andrew@codeconstruct.com.au>;
> Joel Stanley <joel@jms.id.au>; open list:ASPEED BMCs
> <qemu-arm@nongnu.org>; open list:All patches CC here
> <qemu-devel@nongnu.org>
> Cc: Troy Lee <troy_lee@aspeedtech.com>
> Subject: Re: [PATCH v0 1/2] aspeed: support uart controller both 0 and 1 base
> 
> On 2/6/24 04:29, Jamin Lin wrote:
> >> -----Original Message-----
> >> The uart definitions on the AST2700 are different :
> >>
> >>
> >> https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/
> >> arm
> >> 64/boot/dts/aspeed/aspeed-g7.dtsi
> >>
> >> 	serial0 = &uart0;
> >> 	serial1 = &uart1;
> >> 	serial2 = &uart2;
> >> 	serial3 = &uart3;
> >> 	serial4 = &uart4;
> >> 	serial5 = &uart5;
> >> 	serial6 = &uart6;
> >> 	serial7 = &uart7;
> >> 	serial8 = &uart8;
> >>           ...
> >>
> >> I think the names in the DT (and consequently in the QEMU models)
> >> follow the IP names in the datasheet.
> >>
> >> I don't think we care in QEMU, so I would be inclined to change the
> >> indexing of the device names in QEMU and start at 0, which would
> >> introduce a discrepancy for the AST2400, AST2600, AST2600 SoC.
> >>
> >> Let's see what the other maintainers have to say.
> >>
> >> Thanks,
> >>
> >> C.
> > Hi Cedric,
> >
> > Did you mean to change the naming of uart device to 0 base for all ASPEED
> SOCs?
> > If yes, it seems we need to do the following changes.
> > 1. add ASPEED_DEV_UART0 in aspeed_soc.h 2. Re-defined uart memory map
> > for ast2600, ast10x0, ast2500 and ast2400(uart0 -> ASPEED_DEV_UART0)
> > Take ast2600 for example:
> > static const hwaddr aspeed_soc_ast2600_memmap[] = {
> >      [ASPEED_DEV_UART1]     = 0x1E783000, --->
> [ASPEED_DEV_UART0]
> >      [ASPEED_DEV_UART2]     = 0x1E78D000, --->
> [ASPEED_DEV_UART1]
> >      [ASPEED_DEV_UART3]     = 0x1E78E000,
> >      [ASPEED_DEV_UART4]     = 0x1E78F000,
> >      [ASPEED_DEV_UART5]     = 0x1E784000,
> >      [ASPEED_DEV_UART6]     = 0x1E790000,
> >      [ASPEED_DEV_UART7]     = 0x1E790100,
> >      [ASPEED_DEV_UART8]     = 0x1E790200,
> >      [ASPEED_DEV_UART9]     = 0x1E790300,
> >      [ASPEED_DEV_UART10]    = 0x1E790400,
> >      [ASPEED_DEV_UART11]    = 0x1E790500,
> >      [ASPEED_DEV_UART12]    = 0x1E790600,
> >      [ASPEED_DEV_UART13]    = 0x1E790700, --->
> [ASPEED_DEV_UART12]
> > };
> > If no, could you please descript it more detail? So, I can change it and re-send
> this patch series.
> 
> Let's keep the datasheet names. I had forgotten the reason initially and from
> an HW POV it makes sense to keep them in sync. I will add some more
> comments to the patch.
> 
> > By the way, I will send a new patch series to support AST2700 in two weeks.
> > We encountered GIC issues. It seems that QEMU support GIC v3 but SPI did
> not support, yet.
> >
> >
> https://github.com/qemu/qemu/blob/master/hw/intc/arm_gicv3_dist.c#L383
> > https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/a
> > rm64/boot/dts/aspeed/aspeed-g7.dtsi#L229
> 
> If you did any hacks or workarounds in the QEMU models, please keep them
> separate from the other patches so that we can discuss.
> 
Okay. Will do
Thanks-Jamin
> > It think that we can discuss it in a new AST2700 patch series.
> Sure.
> 
> Thanks,
> 
> C.
> 

On 5/2/24 11:46, Cédric Le Goater wrote:
> Hello Jamin,
> 
> On 2/5/24 10:14, Jamin Lin wrote:
>> According to the design of ASPEED SOCS, the uart controller
>> is 1 base for ast10x0, ast2600, ast2500 and ast2400.
>>
>> However, the uart controller is 0 base for ast2700.
>> To support uart controller both 0 and 1 base,
>> adds uasrt_bases parameter in AspeedSoCClass
>> and set the default uart controller 1 base
>> for ast10x0, astt2600, ast2500 and ast2400.
> 
> The board definition can set 'amc->uart_default' to choose a different
> default serial port for the console, or use the "bmc-console" machine
> option . Isn't it enough ? May be I am misunderstanding the need.
> 
> To clarify,
> 
> ASPEED_DEV_UART1 is in the first serial port on the boards.
> 
> I think we chose to start the indexing at 1 because the Aspeed QEMU
> modeling began first with the UART model (console) and for simplicity,
> we copied the definitions of the device tree from Linux :
> 
>      serial0 = &uart1;
>      serial1 = &uart2;
>      serial2 = &uart3;
>      serial3 = &uart4;
>      serial4 = &uart5;
>      serial5 = &vuart;
> 
> We replicated this indexing starting at 1 to nearly all device models :
> 
>      ASPEED_DEV_UART1 - 13
>      ASPEED_DEV_SPI1 -2
>      ASPEED_DEV_EHCI1 -2
>      ASPEED_DEV_TIMER1 - 8
>      ASPEED_DEV_ETH1 -4
>      ASPEED_DEV_MII1 - 4
>      ASPEED_DEV_JTAG0 - 1      <--- !!
>      ASPEED_DEV_FSI1 - 2
> 
> I don't know what would be ASPEED_DEV_UART0 in this context.
> 
> May be you could send a simplified AST2700 SoC model with definitions
> of a minimum address space and IRQ space ?

Looking at TF-A definitions,
https://github.com/ARM-software/arm-trusted-firmware/commit/85f199b77447

#define UART_BASE	U(0x14c33000)
#define UART12_BASE	(UART_BASE + 0xb00)
#define CONSOLE_UART_BASE		UART12_BASE

As Cédric described, we have TF-A UART12_BASE -> QEMU ASPEED_DEV_UART13.

> -----Original Message-----
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Sent: Monday, February 5, 2024 9:16 PM
> To: Cédric Le Goater <clg@kaod.org>; Jamin Lin <jamin_lin@aspeedtech.com>;
> Peter Maydell <peter.maydell@linaro.org>; Andrew Jeffery
> <andrew@codeconstruct.com.au>; Joel Stanley <joel@jms.id.au>; open
> list:ASPEED BMCs <qemu-arm@nongnu.org>; open list:All patches CC here
> <qemu-devel@nongnu.org>
> Cc: Troy Lee <troy_lee@aspeedtech.com>
> Subject: Re: [PATCH v0 1/2] aspeed: support uart controller both 0 and 1 base
> 
> On 5/2/24 11:46, Cédric Le Goater wrote:
> > Hello Jamin,
> >
> > On 2/5/24 10:14, Jamin Lin wrote:
> >> According to the design of ASPEED SOCS, the uart controller is 1 base
> >> for ast10x0, ast2600, ast2500 and ast2400.
> >>
> >> However, the uart controller is 0 base for ast2700.
> >> To support uart controller both 0 and 1 base, adds uasrt_bases
> >> parameter in AspeedSoCClass and set the default uart controller 1
> >> base for ast10x0, astt2600, ast2500 and ast2400.
> >
> > The board definition can set 'amc->uart_default' to choose a different
> > default serial port for the console, or use the "bmc-console" machine
> > option . Isn't it enough ? May be I am misunderstanding the need.
> >
> > To clarify,
> >
> > ASPEED_DEV_UART1 is in the first serial port on the boards.
> >
> > I think we chose to start the indexing at 1 because the Aspeed QEMU
> > modeling began first with the UART model (console) and for simplicity,
> > we copied the definitions of the device tree from Linux :
> >
> >      serial0 = &uart1;
> >      serial1 = &uart2;
> >      serial2 = &uart3;
> >      serial3 = &uart4;
> >      serial4 = &uart5;
> >      serial5 = &vuart;
> >
> > We replicated this indexing starting at 1 to nearly all device models :
> >
> >      ASPEED_DEV_UART1 - 13
> >      ASPEED_DEV_SPI1 -2
> >      ASPEED_DEV_EHCI1 -2
> >      ASPEED_DEV_TIMER1 - 8
> >      ASPEED_DEV_ETH1 -4
> >      ASPEED_DEV_MII1 - 4
> >      ASPEED_DEV_JTAG0 - 1      <--- !!
> >      ASPEED_DEV_FSI1 - 2
> >
> > I don't know what would be ASPEED_DEV_UART0 in this context.
> >
> > May be you could send a simplified AST2700 SoC model with definitions
> > of a minimum address space and IRQ space ?
> 
> Looking at TF-A definitions,
> https://github.com/ARM-software/arm-trusted-firmware/commit/85f199b774
> 47
> 
> #define UART_BASE	U(0x14c33000)
> #define UART12_BASE	(UART_BASE + 0xb00)
> #define CONSOLE_UART_BASE		UART12_BASE
> 
> As Cédric described, we have TF-A UART12_BASE -> QEMU
> ASPEED_DEV_UART13.
As Cédric described, the UART definitions on the AST2700 are different :
   
   https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi

	serial0 = &uart0;
	serial1 = &uart1;
	serial2 = &uart2;
	serial3 = &uart3;
	serial4 = &uart4;
	serial5 = &uart5;
	serial6 = &uart6;
	serial7 = &uart7;
	serial8 = &uart8;

According to the current design of ASPEED QEMU UART model, it used "1" base device name and
Follow the IP names in the datasheet
Take ast2600 for example:
static const hwaddr aspeed_soc_ast2600_memmap[] = {
    [ASPEED_DEV_UART1]     = 0x1E783000,
}
AST2600 datashee description:
Base Address of UART1 = 0x1E78 3000
Base Address of UART2 = 0x1E78 D000
Base Address of UART3 = 0x1E78 E000
Base Address of UART4 = 0x1E78 F000
Base Address of UART5 = 0x1E78 4000
Base Address of UART6 = 0x1E79 0000

However, device name of uart controller had been changed to "0" base for ast2700.
If we want to control uart0(ASPEED_DEV_UART1), we should set the memory map as following,

static const hwaddr aspeed_soc_ast2700_memmap[] = {
    [ASPEED_DEV_UART1]     =  0X14C33000,
    [ASPEED_DEV_UART2]     =  0X14C33100,
    [ASPEED_DEV_UART3]     =  0X14C33200,
    [ASPEED_DEV_UART4]     =  0X14C33300,
    [ASPEED_DEV_UART5]     =  0X12C1A000,
}

AST2700 datasheet description:
AST2700 integrate 13 sets of UART.
Base Address of UART0 = 0x14C3_3000
Base Address of UART1 = 0x14C3_3100
Base Address of UART2 = 0x14C3_3200
Base Address of UART3 = 0x14C3_3300
Base Address of UART4 = 0x12C1_A000
Base Address of UART5 = 0x14C3_3400
Base Address of UART6 = 0x14C3_3500
Base Address of UART7 = 0x14C3_3600
Base Address of UART8 = 0x14C3_3700
Base Address of UART9 = 0x14C3_3800
Base Address of UART10 = 0x14C3_3900
Base Address of UART11 = 0x14C3_3A00
Base Address of UART12 = 0x14C3_3B00

As you said, uart12 mapped ASPEED_DEV_UART13.
The device naming will confuse users because the device name in qemu mismatch with ast2700 datasheet.

That way why we want to add ASPEED_DEV_UART0 and set the memory map of AST2700 as following.
static const hwaddr aspeed_soc_ast2700_memmap[] = {
    [ASPEED_DEV_UART0]     =  0X14C33000,
    [ASPEED_DEV_UART1]     =  0X14C33100,
    [ASPEED_DEV_UART2]     =  0X14C33200,
    [ASPEED_DEV_UART3]     =  0X14C33300,
    [ASPEED_DEV_UART4]     =  0X12C1A000,
    [ASPEED_DEV_UART5]     =  0X14C33400,
    [ASPEED_DEV_UART6]     =  0X14C33500,
    [ASPEED_DEV_UART7]     =  0X14C33600,
    [ASPEED_DEV_UART8]     =  0X14C33700,
    [ASPEED_DEV_UART9]     =  0X14C33800,
    [ASPEED_DEV_UART10]    =  0X14C33900,
    [ASPEED_DEV_UART11]    =  0X14C33A00,
    [ASPEED_DEV_UART12]    =  0X14C33B00,
}

Thanks-Jamin

[ ... ]


> As you said, uart12 mapped ASPEED_DEV_UART13.
> The device naming will confuse users because the device name in qemu mismatch with ast2700 datasheet.
>
> That way why we want to add ASPEED_DEV_UART0 and set the memory map of AST2700 as following.
> static const hwaddr aspeed_soc_ast2700_memmap[] = {
>      [ASPEED_DEV_UART0]     =  0X14C33000,
>      [ASPEED_DEV_UART1]     =  0X14C33100,
>      [ASPEED_DEV_UART2]     =  0X14C33200,
>      [ASPEED_DEV_UART3]     =  0X14C33300,
>      [ASPEED_DEV_UART4]     =  0X12C1A000,
>      [ASPEED_DEV_UART5]     =  0X14C33400,
>      [ASPEED_DEV_UART6]     =  0X14C33500,
>      [ASPEED_DEV_UART7]     =  0X14C33600,
>      [ASPEED_DEV_UART8]     =  0X14C33700,
>      [ASPEED_DEV_UART9]     =  0X14C33800,
>      [ASPEED_DEV_UART10]    =  0X14C33900,
>      [ASPEED_DEV_UART11]    =  0X14C33A00,
>      [ASPEED_DEV_UART12]    =  0X14C33B00,
>

So we would prefer to keep the QEMU IP names in sync with the datasheet,
and in that case your proposal makes sense.

A have a few comments that I will make on the patch.

Thanks,

C.