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;