[v0 0/2] uart base and hardcode boot address 0

[v0 0/2] uart base and hardcode boot address 0

[Jamin Lin via]

v0:
1. support uart controller both 0 and 1 base
2. fix hardcode boot address 0

Jamin Lin (2):
  aspeed: support uart controller both 0 and 1 base
  aspeed: fix hardcode boot address 0

 hw/arm/aspeed.c             | 12 ++++++++----
 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, 15 insertions(+), 6 deletions(-)

-- 
2.34.1

[PATCH v0 1/2] aspeed: support uart controller both 0 and 1 base

[Jamin Lin via]

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

[PATCH v0 2/2] aspeed: fix hardcode boot address 0

[Jamin Lin via]

In the previous design of QEMU model for ASPEED SOCs, it set the boot
address at 0 which was the hardcode setting for ast10x0, ast2600,
ast2500 and ast2400.

According to the design of ast2700, it has bootmcu which is used for
executing SPL and initialize DRAM, then, CPUs(cortex-a35)
execute u-boot, kernel and rofs. QEMU will only support CPU(coretax-a35)
parts and the boot address is "0x400000000" for ast2700.
Therefore, fixed hardcode boot address 0.

Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/arm/aspeed.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 218b81298e..82a92e8142 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -289,12 +289,14 @@ static void aspeed_install_boot_rom(AspeedMachineState *bmc, BlockBackend *blk,
                                     uint64_t rom_size)
 {
     AspeedSoCState *soc = bmc->soc;
+    AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(soc);
 
     memory_region_init_rom(&bmc->boot_rom, NULL, "aspeed.boot_rom", rom_size,
                            &error_abort);
     memory_region_add_subregion_overlap(&soc->spi_boot_container, 0,
                                         &bmc->boot_rom, 1);
-    write_boot_rom(blk, ASPEED_SOC_SPI_BOOT_ADDR, rom_size, &error_abort);
+    write_boot_rom(blk, sc->memmap[ASPEED_DEV_SPI_BOOT],
+                   rom_size, &error_abort);
 }
 
 void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,
-- 
2.34.1

Re: [PATCH v0 1/2] aspeed: support uart controller both 0 and 1 base

[Cédric Le Goater]

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;

Re: [PATCH v0 1/2] aspeed: support uart controller both 0 and 1 base

[Philippe Mathieu-Daudé]

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.

Re: [PATCH v0 2/2] aspeed: fix hardcode boot address 0

[Philippe Mathieu-Daudé]

Hi Jamin,

On 5/2/24 10:14, Jamin Lin via wrote:
> In the previous design of QEMU model for ASPEED SOCs, it set the boot
> address at 0 which was the hardcode setting for ast10x0, ast2600,
> ast2500 and ast2400.
> 
> According to the design of ast2700, it has bootmcu which is used for
> executing SPL and initialize DRAM,

Out of curiosity, what architecture is this MCU?

> then, CPUs(cortex-a35)
> execute u-boot, kernel and rofs. QEMU will only support CPU(coretax-a35)
> parts and the boot address is "0x400000000" for ast2700.

OK, but I don't get how you get from here ...

> Therefore, fixed hardcode boot address 0.

... to here.

> Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   hw/arm/aspeed.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 218b81298e..82a92e8142 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -289,12 +289,14 @@ static void aspeed_install_boot_rom(AspeedMachineState *bmc, BlockBackend *blk,
>                                       uint64_t rom_size)
>   {
>       AspeedSoCState *soc = bmc->soc;
> +    AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(soc);
>   
>       memory_region_init_rom(&bmc->boot_rom, NULL, "aspeed.boot_rom", rom_size,
>                              &error_abort);
>       memory_region_add_subregion_overlap(&soc->spi_boot_container, 0,
>                                           &bmc->boot_rom, 1);
> -    write_boot_rom(blk, ASPEED_SOC_SPI_BOOT_ADDR, rom_size, &error_abort);
> +    write_boot_rom(blk, sc->memmap[ASPEED_DEV_SPI_BOOT],
> +                   rom_size, &error_abort);

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

>   }
>   
>   void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,

Re: [PATCH v0 2/2] aspeed: fix hardcode boot address 0

[Cédric Le Goater]

On 2/5/24 10:14, Jamin Lin wrote:
> In the previous design of QEMU model for ASPEED SOCs, it set the boot
> address at 0 which was the hardcode setting for ast10x0, ast2600,
> ast2500 and ast2400.
> 
> According to the design of ast2700, it has bootmcu which is used for
> executing SPL and initialize DRAM, then, CPUs(cortex-a35)
> execute u-boot, kernel and rofs. QEMU will only support CPU(coretax-a35)
> parts and the boot address is "0x400000000" for ast2700.

On the previous SoC, the ASPEED_DEV_SPI_BOOT region is an alias, at 0x0,
to the FMC CE0 region, mapped at 0x20000000.

Is 0x400000000 (or 0x40000000 ?) the address for FMC CE0 region on the
ast2700 ? or an alias ?

What is the cortex-a35 reset address ?

It would help to also introduce a basic skeleton of the ast2700 SoC.

Anyhow, this change makes sense. Could you please respin and also
remove ASPEED_SOC_SPI_BOOT_ADDR. ?

Thanks,

C.

> Therefore, fixed hardcode boot address 0.
> 
> Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   hw/arm/aspeed.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 218b81298e..82a92e8142 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -289,12 +289,14 @@ static void aspeed_install_boot_rom(AspeedMachineState *bmc, BlockBackend *blk,
>                                       uint64_t rom_size)
>   {
>       AspeedSoCState *soc = bmc->soc;
> +    AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(soc);
>   
>       memory_region_init_rom(&bmc->boot_rom, NULL, "aspeed.boot_rom", rom_size,
>                              &error_abort);
>       memory_region_add_subregion_overlap(&soc->spi_boot_container, 0,
>                                           &bmc->boot_rom, 1);
> -    write_boot_rom(blk, ASPEED_SOC_SPI_BOOT_ADDR, rom_size, &error_abort);
> +    write_boot_rom(blk, sc->memmap[ASPEED_DEV_SPI_BOOT],
> +                   rom_size, &error_abort);
>   }
>   
>   void aspeed_board_init_flashes(AspeedSMCState *s, const char *flashtype,

Re: [PATCH v0 1/2] aspeed: support uart controller both 0 and 1 base

[Cédric Le Goater]

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.

RE: [PATCH v0 2/2] aspeed: fix hardcode boot address 0

[Jamin Lin]

> -----Original Message-----
> From: Philippe Mathieu-Daudé <philmd@linaro.org>
> Sent: Monday, February 5, 2024 9:20 PM
> To: Jamin Lin <jamin_lin@aspeedtech.com>; Cédric Le Goater <clg@kaod.org>;
> 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 2/2] aspeed: fix hardcode boot address 0
> 
> Hi Jamin,
> 
> On 5/2/24 10:14, Jamin Lin via wrote:
> > In the previous design of QEMU model for ASPEED SOCs, it set the boot
> > address at 0 which was the hardcode setting for ast10x0, ast2600,
> > ast2500 and ast2400.
> >
> > According to the design of ast2700, it has bootmcu which is used for
> > executing SPL and initialize DRAM,
> 
> Out of curiosity, what architecture is this MCU?
MCU is riscv-ibex and its architecture is riscv-32.

> 
> > then, CPUs(cortex-a35)
> > execute u-boot, kernel and rofs. QEMU will only support
> > CPU(coretax-a35) parts and the boot address is "0x400000000" for ast2700.
> 
> OK, but I don't get how you get from here ...
> 
Our design make MCU execute SPL and copy u-boot image from SPI to DRAM at address 0x400000000 at SPL boot stage.
However, QEMU will only support to emulate CPU sides (coretex-a35) for ast2700, that was why we want to change the boot address at 0x400000000
And use the following start command by QEMU.

./qemu-system-aarch64 -M ast2750-evb -nographic -m 8G \
 -device loader,addr=0x400000000,file=${IMGDIR}/u-boot-nodtb.bin,force-raw=on \
 -device loader,addr=$((0x400000000 + ${UBOOT_SIZE})),file=${IMGDIR}/u-boot.dtb,force-raw=on \
 ---
 ---

By the way, I will send a new patch series to support ast2700 in two weeks and 
We set memory map for ast2700 as following.

static const hwaddr aspeed_soc_ast2700_memmap[] = {
    [ASPEED_DEV_SPI_BOOT]  =  0x400000000,
    [ASPEED_DEV_SRAM]      =  0x10000000,

Jamin
> > Therefore, fixed hardcode boot address 0.
> 
> ... to here.
> 
> > Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > ---
> >   hw/arm/aspeed.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c index
> > 218b81298e..82a92e8142 100644
> > --- a/hw/arm/aspeed.c
> > +++ b/hw/arm/aspeed.c
> > @@ -289,12 +289,14 @@ static void
> aspeed_install_boot_rom(AspeedMachineState *bmc, BlockBackend *blk,
> >                                       uint64_t rom_size)
> >   {
> >       AspeedSoCState *soc = bmc->soc;
> > +    AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(soc);
> >
> >       memory_region_init_rom(&bmc->boot_rom, NULL,
> "aspeed.boot_rom", rom_size,
> >                              &error_abort);
> >       memory_region_add_subregion_overlap(&soc->spi_boot_container,
> 0,
> >                                           &bmc->boot_rom, 1);
> > -    write_boot_rom(blk, ASPEED_SOC_SPI_BOOT_ADDR, rom_size,
> &error_abort);
> > +    write_boot_rom(blk, sc->memmap[ASPEED_DEV_SPI_BOOT],
> > +                   rom_size, &error_abort);
> 
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 
> >   }
> >
> >   void aspeed_board_init_flashes(AspeedSMCState *s, const char
> > *flashtype,

RE: [PATCH v0 2/2] aspeed: fix hardcode boot address 0

[Jamin Lin]

> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Monday, February 5, 2024 9:34 PM
> 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 2/2] aspeed: fix hardcode boot address 0
> 
> On 2/5/24 10:14, Jamin Lin wrote:
> > In the previous design of QEMU model for ASPEED SOCs, it set the boot
> > address at 0 which was the hardcode setting for ast10x0, ast2600,
> > ast2500 and ast2400.
> >
> > According to the design of ast2700, it has bootmcu which is used for
> > executing SPL and initialize DRAM, then, CPUs(cortex-a35) execute
> > u-boot, kernel and rofs. QEMU will only support CPU(coretax-a35) parts
> > and the boot address is "0x400000000" for ast2700.
> 
> On the previous SoC, the ASPEED_DEV_SPI_BOOT region is an alias, at 0x0, to
> the FMC CE0 region, mapped at 0x20000000.
> 
> Is 0x400000000 (or 0x40000000 ?) the address for FMC CE0 region on the
> ast2700 ? or an alias ?
> 
It is "0x4 00000000"(64bits address). CPU is armv8 cortex-a35 which is 64 bits CPU. 
The dram base address is "0x4 00000000".
The SPL base address is "0x1 00000000".
FMC_CS0 region mapped at "0x1 00000000" address.

> What is the cortex-a35 reset address ?
> 
> It would help to also introduce a basic skeleton of the ast2700 SoC.
> 
AST2700
Primary Service Processor:
Embedded quad-core ARM Cortex A35 64-bit RISC CPU
Maximum running frequency: 1.6GHZ
Support: MMU, FPU, NEON, trust-zone, GIC-500 controller and so on.

BootMCU:
Ibex-riscv 32bits riscv.

Boot flow
ROM Code -> BootMCU(SPL) -> CPU Cortex A35(U-boot-> kernel -> rofs)

> Anyhow, this change makes sense. Could you please respin and also remove
> ASPEED_SOC_SPI_BOOT_ADDR. ?
> 
Okay, will remove it.

> Thanks,
> 
> C.
> 
> > Therefore, fixed hardcode boot address 0.
> >
> > Signed-off-by: Troy Lee <troy_lee@aspeedtech.com>
> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> > ---
> >   hw/arm/aspeed.c | 4 +++-
> >   1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c index
> > 218b81298e..82a92e8142 100644
> > --- a/hw/arm/aspeed.c
> > +++ b/hw/arm/aspeed.c
> > @@ -289,12 +289,14 @@ static void
> aspeed_install_boot_rom(AspeedMachineState *bmc, BlockBackend *blk,
> >                                       uint64_t rom_size)
> >   {
> >       AspeedSoCState *soc = bmc->soc;
> > +    AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(soc);
> >
> >       memory_region_init_rom(&bmc->boot_rom, NULL,
> "aspeed.boot_rom", rom_size,
> >                              &error_abort);
> >       memory_region_add_subregion_overlap(&soc->spi_boot_container,
> 0,
> >                                           &bmc->boot_rom, 1);
> > -    write_boot_rom(blk, ASPEED_SOC_SPI_BOOT_ADDR, rom_size,
> &error_abort);
> > +    write_boot_rom(blk, sc->memmap[ASPEED_DEV_SPI_BOOT],
> > +                   rom_size, &error_abort);
> >   }
> >
> >   void aspeed_board_init_flashes(AspeedSMCState *s, const char
> > *flashtype,

RE: [PATCH v0 1/2] aspeed: support uart controller both 0 and 1 base

[Jamin Lin]

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

RE: [PATCH v0 1/2] aspeed: support uart controller both 0 and 1 base

[Jamin Lin]

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

Re: [PATCH v0 1/2] aspeed: support uart controller both 0 and 1 base

[Cédric Le Goater]

[ ... ]


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

Re: [v0 0/2] uart base and hardcode boot address 0

[Cédric Le Goater]

On 2/5/24 10:14, Jamin Lin wrote:
> v0:

usually we start at v1, so the next version would be a v2. Indexing again :)


Thanks,

C.



> 1. support uart controller both 0 and 1 base
> 2. fix hardcode boot address 0
> 
> Jamin Lin (2):
>    aspeed: support uart controller both 0 and 1 base
>    aspeed: fix hardcode boot address 0
> 
>   hw/arm/aspeed.c             | 12 ++++++++----
>   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, 15 insertions(+), 6 deletions(-)
> 

Re: [PATCH v0 2/2] aspeed: fix hardcode boot address 0

[Cédric Le Goater]

On 2/6/24 02:48, Jamin Lin wrote:
>> -----Original Message-----
>> From: Philippe Mathieu-Daudé <philmd@linaro.org>
>> Sent: Monday, February 5, 2024 9:20 PM
>> To: Jamin Lin <jamin_lin@aspeedtech.com>; Cédric Le Goater <clg@kaod.org>;
>> 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 2/2] aspeed: fix hardcode boot address 0
>>
>> Hi Jamin,
>>
>> On 5/2/24 10:14, Jamin Lin via wrote:
>>> In the previous design of QEMU model for ASPEED SOCs, it set the boot
>>> address at 0 which was the hardcode setting for ast10x0, ast2600,
>>> ast2500 and ast2400.
>>>
>>> According to the design of ast2700, it has bootmcu which is used for
>>> executing SPL and initialize DRAM,
>>
>> Out of curiosity, what architecture is this MCU?
> MCU is riscv-ibex and its architecture is riscv-32.
> 
>>
>>> then, CPUs(cortex-a35)
>>> execute u-boot, kernel and rofs. QEMU will only support
>>> CPU(coretax-a35) parts and the boot address is "0x400000000" for ast2700.
>>
>> OK, but I don't get how you get from here ...
>>
> Our design make MCU execute SPL and copy u-boot image from SPI to DRAM at address 0x400000000 at SPL boot stage.
> However, QEMU will only support to emulate CPU sides (coretex-a35) for ast2700, 

The fby35 is an example of a machine with two ARM SoCs : ast1030-a1
and ast2600-a3. There is work in progress for heterogeneous QEMU
machines and It might be possible to model RISC-V and ARM one day.

> that was why we want to change the boot address at 0x400000000
> And use the following start command by QEMU.
> 
> ./qemu-system-aarch64 -M ast2750-evb -nographic -m 8G \
>   -device loader,addr=0x400000000,file=${IMGDIR}/u-boot-nodtb.bin,force-raw=on \
>   -device loader,addr=$((0x400000000 + ${UBOOT_SIZE})),file=${IMGDIR}/u-boot.dtb,force-raw=on \
>   ---
>   ---
ok. Makes sense.
  
> By the way, I will send a new patch series to support ast2700 in two weeks and
> We set memory map for ast2700 as following.
> 
> static const hwaddr aspeed_soc_ast2700_memmap[] = {
>      [ASPEED_DEV_SPI_BOOT]  =  0x400000000,
>      [ASPEED_DEV_SRAM]      =  0x10000000,

Excellent !

Thanks,

C.

Re: [PATCH v0 1/2] aspeed: support uart controller both 0 and 1 base

[Cédric Le Goater]

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.

Re: [PATCH v0 1/2] aspeed: support uart controller both 0 and 1 base

[Cédric Le Goater]

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;

RE: [v0 0/2] uart base and hardcode boot address 0

[Jamin Lin]

> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Wednesday, February 7, 2024 12:48 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: [v0 0/2] uart base and hardcode boot address 0
> 
> On 2/5/24 10:14, Jamin Lin wrote:
> > v0:
> 
> usually we start at v1, so the next version would be a v2. Indexing again :)
> 
Got it.
Thanks-Jamin
> 
> Thanks,
> 
> C.
> 
> 
> 
> > 1. support uart controller both 0 and 1 base 2. fix hardcode boot
> > address 0
> >
> > Jamin Lin (2):
> >    aspeed: support uart controller both 0 and 1 base
> >    aspeed: fix hardcode boot address 0
> >
> >   hw/arm/aspeed.c             | 12 ++++++++----
> >   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, 15 insertions(+), 6 deletions(-)
> >

RE: [PATCH v0 1/2] aspeed: support uart controller both 0 and 1 base

[Jamin Lin]

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