Most of the Aspeed machines use the UART5 device for the boot console,
and QEMU connects the first serial Chardev to this SoC device for this
purpose. See routine connect_serial_hds_to_uarts().

Nevertheless, some machines use another boot console, such as the fuji,
and commit 5d63d0c76c ("hw/arm/aspeed: Allow machine to set UART
default") introduced a SoC class attribute 'uart_default' and property
to be able to change the boot console device. It was later changed by
commit d2b3eaefb4 ("aspeed: Refactor UART init for multi-SoC machines").

The "uart" machine option goes a step further and lets the user define
the UART device from the QEMU command line without introducing a new
machine definition. For instance, to use device UART3 (mapped on
/dev/ttyS2 under Linux) instead of the default UART5, one would use :

  -M ast2500-evb,uart=uart3

Cc: Abhishek Singh Dagur <abhishek@drut.io>
Signed-off-by: Cédric Le Goater <clg@kaod.org>
---
 docs/system/arm/aspeed.rst | 10 ++++++++++
 hw/arm/aspeed.c            | 39 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 47 insertions(+), 2 deletions(-)

diff --git a/docs/system/arm/aspeed.rst b/docs/system/arm/aspeed.rst
index d4e293e7f9..e70f0aeea3 100644
--- a/docs/system/arm/aspeed.rst
+++ b/docs/system/arm/aspeed.rst
@@ -122,6 +122,10 @@ Options specific to Aspeed machines are :
 
  * ``spi-model`` to change the SPI Flash model.
 
+ * ``uart`` to change the default console device. Most of the machines
+   use the ``UART5`` device for a boot console, which is mapped on
+   ``/dev/ttyS4`` under Linux, but it is not always the case.
+
 For instance, to start the ``ast2500-evb`` machine with a different
 FMC chip and a bigger (64M) SPI chip, use :
 
@@ -129,6 +133,12 @@ FMC chip and a bigger (64M) SPI chip, use :
 
   -M ast2500-evb,fmc-model=mx25l25635e,spi-model=mx66u51235f
 
+To change the boot console and use device ``UART3`` (``/dev/ttyS2``
+under Linux), use :
+
+.. code-block:: bash
+
+  -M ast2500-evb,uart=uart3
 
 Aspeed minibmc family boards (``ast1030-evb``)
 ==================================================================
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 3d5488faf7..6c32f674b9 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -43,6 +43,7 @@ struct AspeedMachineState {
     AspeedSoCState soc;
     MemoryRegion boot_rom;
     bool mmio_exec;
+    uint32_t uart_chosen;
     char *fmc_model;
     char *spi_model;
 };
@@ -331,10 +332,11 @@ static void connect_serial_hds_to_uarts(AspeedMachineState *bmc)
     AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
     AspeedSoCState *s = &bmc->soc;
     AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
+    int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default;
 
-    aspeed_soc_uart_set_chr(s, amc->uart_default, serial_hd(0));
+    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++) {
-        if (uart == amc->uart_default) {
+        if (uart == uart_chosen) {
             continue;
         }
         aspeed_soc_uart_set_chr(s, uart, serial_hd(i));
@@ -1077,6 +1079,35 @@ static void aspeed_set_spi_model(Object *obj, const char *value, Error **errp)
     bmc->spi_model = g_strdup(value);
 }
 
+static char *aspeed_get_uart(Object *obj, Error **errp)
+{
+    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
+    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
+    int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default;
+
+    return g_strdup_printf("uart%d", uart_chosen - ASPEED_DEV_UART1 + 1);
+}
+
+static void aspeed_set_uart(Object *obj, const char *value, Error **errp)
+{
+    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
+    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
+    AspeedSoCClass *sc = ASPEED_SOC_CLASS(object_class_by_name(amc->soc_name));
+    int val;
+
+    if (sscanf(value, "uart%u", &val) != 1) {
+        error_setg(errp, "Bad value for \"uart\" property");
+        return;
+    }
+
+    /* The number of UART depends on the SoC */
+    if (val < 1 || val > sc->uarts_num) {
+        error_setg(errp, "\"uart\" should be in range [1 - %d]", sc->uarts_num);
+        return;
+    }
+    bmc->uart_chosen = ASPEED_DEV_UART1 + val - 1;
+}
+
 static void aspeed_machine_class_props_init(ObjectClass *oc)
 {
     object_class_property_add_bool(oc, "execute-in-place",
@@ -1085,6 +1116,10 @@ static void aspeed_machine_class_props_init(ObjectClass *oc)
     object_class_property_set_description(oc, "execute-in-place",
                            "boot directly from CE0 flash device");
 
+    object_class_property_add_str(oc, "uart", aspeed_get_uart, aspeed_set_uart);
+    object_class_property_set_description(oc, "uart",
+                           "Change the default UART to \"uartX\"");
+
     object_class_property_add_str(oc, "fmc-model", aspeed_get_fmc_model,
                                    aspeed_set_fmc_model);
     object_class_property_set_description(oc, "fmc-model",
-- 
2.40.0

On 8/5/23 09:58, Cédric Le Goater wrote:
> Most of the Aspeed machines use the UART5 device for the boot console,
> and QEMU connects the first serial Chardev to this SoC device for this
> purpose. See routine connect_serial_hds_to_uarts().
> 
> Nevertheless, some machines use another boot console, such as the fuji,
> and commit 5d63d0c76c ("hw/arm/aspeed: Allow machine to set UART
> default") introduced a SoC class attribute 'uart_default' and property
> to be able to change the boot console device. It was later changed by
> commit d2b3eaefb4 ("aspeed: Refactor UART init for multi-SoC machines").
> 
> The "uart" machine option goes a step further and lets the user define
> the UART device from the QEMU command line without introducing a new
> machine definition. For instance, to use device UART3 (mapped on
> /dev/ttyS2 under Linux) instead of the default UART5, one would use :
> 
>    -M ast2500-evb,uart=uart3
> 
> Cc: Abhishek Singh Dagur <abhishek@drut.io>
> Signed-off-by: Cédric Le Goater <clg@kaod.org>
> ---
>   docs/system/arm/aspeed.rst | 10 ++++++++++
>   hw/arm/aspeed.c            | 39 ++++++++++++++++++++++++++++++++++++--
>   2 files changed, 47 insertions(+), 2 deletions(-)
> 
> diff --git a/docs/system/arm/aspeed.rst b/docs/system/arm/aspeed.rst
> index d4e293e7f9..e70f0aeea3 100644
> --- a/docs/system/arm/aspeed.rst
> +++ b/docs/system/arm/aspeed.rst
> @@ -122,6 +122,10 @@ Options specific to Aspeed machines are :
>   
>    * ``spi-model`` to change the SPI Flash model.
>   
> + * ``uart`` to change the default console device. Most of the machines
> +   use the ``UART5`` device for a boot console, which is mapped on
> +   ``/dev/ttyS4`` under Linux, but it is not always the case.

This comment ...

>   For instance, to start the ``ast2500-evb`` machine with a different
>   FMC chip and a bigger (64M) SPI chip, use :
>   
> @@ -129,6 +133,12 @@ FMC chip and a bigger (64M) SPI chip, use :
>   
>     -M ast2500-evb,fmc-model=mx25l25635e,spi-model=mx66u51235f
>   
> +To change the boot console and use device ``UART3`` (``/dev/ttyS2``
> +under Linux), use :

... and this one suggest 'boot-console' could be a better name.

Or 'boot-console-index'.

> +.. code-block:: bash
> +
> +  -M ast2500-evb,uart=uart3
>   
>   Aspeed minibmc family boards (``ast1030-evb``)
>   ==================================================================
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 3d5488faf7..6c32f674b9 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -43,6 +43,7 @@ struct AspeedMachineState {
>       AspeedSoCState soc;
>       MemoryRegion boot_rom;
>       bool mmio_exec;
> +    uint32_t uart_chosen;
>       char *fmc_model;
>       char *spi_model;
>   };
> @@ -331,10 +332,11 @@ static void connect_serial_hds_to_uarts(AspeedMachineState *bmc)
>       AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
>       AspeedSoCState *s = &bmc->soc;
>       AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
> +    int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default;
>   
> -    aspeed_soc_uart_set_chr(s, amc->uart_default, serial_hd(0));
> +    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++) {
> -        if (uart == amc->uart_default) {
> +        if (uart == uart_chosen) {
>               continue;
>           }
>           aspeed_soc_uart_set_chr(s, uart, serial_hd(i));
> @@ -1077,6 +1079,35 @@ static void aspeed_set_spi_model(Object *obj, const char *value, Error **errp)
>       bmc->spi_model = g_strdup(value);
>   }
>   
> +static char *aspeed_get_uart(Object *obj, Error **errp)
> +{
> +    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
> +    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
> +    int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default;
> +
> +    return g_strdup_printf("uart%d", uart_chosen - ASPEED_DEV_UART1 + 1);
> +}
> +
> +static void aspeed_set_uart(Object *obj, const char *value, Error **errp)
> +{
> +    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
> +    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
> +    AspeedSoCClass *sc = ASPEED_SOC_CLASS(object_class_by_name(amc->soc_name));
> +    int val;
> +
> +    if (sscanf(value, "uart%u", &val) != 1) {
> +        error_setg(errp, "Bad value for \"uart\" property");

Why are you asking for a string and not an index?

> +        return;
> +    }
> +
> +    /* The number of UART depends on the SoC */
> +    if (val < 1 || val > sc->uarts_num) {
> +        error_setg(errp, "\"uart\" should be in range [1 - %d]", sc->uarts_num);
> +        return;
> +    }
> +    bmc->uart_chosen = ASPEED_DEV_UART1 + val - 1;
> +}
> +
>   static void aspeed_machine_class_props_init(ObjectClass *oc)
>   {
>       object_class_property_add_bool(oc, "execute-in-place",
> @@ -1085,6 +1116,10 @@ static void aspeed_machine_class_props_init(ObjectClass *oc)
>       object_class_property_set_description(oc, "execute-in-place",
>                              "boot directly from CE0 flash device");
>   
> +    object_class_property_add_str(oc, "uart", aspeed_get_uart, aspeed_set_uart);
> +    object_class_property_set_description(oc, "uart",
> +                           "Change the default UART to \"uartX\"");
> +
>       object_class_property_add_str(oc, "fmc-model", aspeed_get_fmc_model,
>                                      aspeed_set_fmc_model);
>       object_class_property_set_description(oc, "fmc-model",

On 5/30/23 23:22, Philippe Mathieu-Daudé wrote:
> On 8/5/23 09:58, Cédric Le Goater wrote:
>> Most of the Aspeed machines use the UART5 device for the boot console,
>> and QEMU connects the first serial Chardev to this SoC device for this
>> purpose. See routine connect_serial_hds_to_uarts().
>>
>> Nevertheless, some machines use another boot console, such as the fuji,
>> and commit 5d63d0c76c ("hw/arm/aspeed: Allow machine to set UART
>> default") introduced a SoC class attribute 'uart_default' and property
>> to be able to change the boot console device. It was later changed by
>> commit d2b3eaefb4 ("aspeed: Refactor UART init for multi-SoC machines").
>>
>> The "uart" machine option goes a step further and lets the user define
>> the UART device from the QEMU command line without introducing a new
>> machine definition. For instance, to use device UART3 (mapped on
>> /dev/ttyS2 under Linux) instead of the default UART5, one would use :
>>
>>    -M ast2500-evb,uart=uart3
>>
>> Cc: Abhishek Singh Dagur <abhishek@drut.io>
>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>> ---
>>   docs/system/arm/aspeed.rst | 10 ++++++++++
>>   hw/arm/aspeed.c            | 39 ++++++++++++++++++++++++++++++++++++--
>>   2 files changed, 47 insertions(+), 2 deletions(-)
>>
>> diff --git a/docs/system/arm/aspeed.rst b/docs/system/arm/aspeed.rst
>> index d4e293e7f9..e70f0aeea3 100644
>> --- a/docs/system/arm/aspeed.rst
>> +++ b/docs/system/arm/aspeed.rst
>> @@ -122,6 +122,10 @@ Options specific to Aspeed machines are :
>>    * ``spi-model`` to change the SPI Flash model.
>> + * ``uart`` to change the default console device. Most of the machines
>> +   use the ``UART5`` device for a boot console, which is mapped on
>> +   ``/dev/ttyS4`` under Linux, but it is not always the case.
> 
> This comment ...
> 
>>   For instance, to start the ``ast2500-evb`` machine with a different
>>   FMC chip and a bigger (64M) SPI chip, use :
>> @@ -129,6 +133,12 @@ FMC chip and a bigger (64M) SPI chip, use :
>>     -M ast2500-evb,fmc-model=mx25l25635e,spi-model=mx66u51235f
>> +To change the boot console and use device ``UART3`` (``/dev/ttyS2``
>> +under Linux), use :
> 
> ... and this one suggest 'boot-console' could be a better name.
> 
> Or 'boot-console-index'.

you might be right. people expect to find the console messages on
this device. I will think about it.

> 
>> +.. code-block:: bash
>> +
>> +  -M ast2500-evb,uart=uart3
>>   Aspeed minibmc family boards (``ast1030-evb``)
>>   ==================================================================
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 3d5488faf7..6c32f674b9 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -43,6 +43,7 @@ struct AspeedMachineState {
>>       AspeedSoCState soc;
>>       MemoryRegion boot_rom;
>>       bool mmio_exec;
>> +    uint32_t uart_chosen;
>>       char *fmc_model;
>>       char *spi_model;
>>   };
>> @@ -331,10 +332,11 @@ static void connect_serial_hds_to_uarts(AspeedMachineState *bmc)
>>       AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
>>       AspeedSoCState *s = &bmc->soc;
>>       AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
>> +    int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default;
>> -    aspeed_soc_uart_set_chr(s, amc->uart_default, serial_hd(0));
>> +    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++) {
>> -        if (uart == amc->uart_default) {
>> +        if (uart == uart_chosen) {
>>               continue;
>>           }
>>           aspeed_soc_uart_set_chr(s, uart, serial_hd(i));
>> @@ -1077,6 +1079,35 @@ static void aspeed_set_spi_model(Object *obj, const char *value, Error **errp)
>>       bmc->spi_model = g_strdup(value);
>>   }
>> +static char *aspeed_get_uart(Object *obj, Error **errp)
>> +{
>> +    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
>> +    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
>> +    int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default;
>> +
>> +    return g_strdup_printf("uart%d", uart_chosen - ASPEED_DEV_UART1 + 1);
>> +}
>> +
>> +static void aspeed_set_uart(Object *obj, const char *value, Error **errp)
>> +{
>> +    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
>> +    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
>> +    AspeedSoCClass *sc = ASPEED_SOC_CLASS(object_class_by_name(amc->soc_name));
>> +    int val;
>> +
>> +    if (sscanf(value, "uart%u", &val) != 1) {
>> +        error_setg(errp, "Bad value for \"uart\" property");
> 
> Why are you asking for a string and not an index?

because the literal name is what people find in the DT. See files
arch/arm/boot/dts/aspeed-bmc-* under Linux.

Thanks,

C.

> 
>> +        return;
>> +    }
>> +
>> +    /* The number of UART depends on the SoC */
>> +    if (val < 1 || val > sc->uarts_num) {
>> +        error_setg(errp, "\"uart\" should be in range [1 - %d]", sc->uarts_num);
>> +        return;
>> +    }
>> +    bmc->uart_chosen = ASPEED_DEV_UART1 + val - 1;
>> +}
>> +
>>   static void aspeed_machine_class_props_init(ObjectClass *oc)
>>   {
>>       object_class_property_add_bool(oc, "execute-in-place",
>> @@ -1085,6 +1116,10 @@ static void aspeed_machine_class_props_init(ObjectClass *oc)
>>       object_class_property_set_description(oc, "execute-in-place",
>>                              "boot directly from CE0 flash device");
>> +    object_class_property_add_str(oc, "uart", aspeed_get_uart, aspeed_set_uart);
>> +    object_class_property_set_description(oc, "uart",
>> +                           "Change the default UART to \"uartX\"");
>> +
>>       object_class_property_add_str(oc, "fmc-model", aspeed_get_fmc_model,
>>                                      aspeed_set_fmc_model);
>>       object_class_property_set_description(oc, "fmc-model",
> 

On 31/5/23 08:28, Cédric Le Goater wrote:
> On 5/30/23 23:22, Philippe Mathieu-Daudé wrote:
>> On 8/5/23 09:58, Cédric Le Goater wrote:
>>> Most of the Aspeed machines use the UART5 device for the boot console,
>>> and QEMU connects the first serial Chardev to this SoC device for this
>>> purpose. See routine connect_serial_hds_to_uarts().
>>>
>>> Nevertheless, some machines use another boot console, such as the fuji,
>>> and commit 5d63d0c76c ("hw/arm/aspeed: Allow machine to set UART
>>> default") introduced a SoC class attribute 'uart_default' and property
>>> to be able to change the boot console device. It was later changed by
>>> commit d2b3eaefb4 ("aspeed: Refactor UART init for multi-SoC machines").
>>>
>>> The "uart" machine option goes a step further and lets the user define
>>> the UART device from the QEMU command line without introducing a new
>>> machine definition. For instance, to use device UART3 (mapped on
>>> /dev/ttyS2 under Linux) instead of the default UART5, one would use :
>>>
>>>    -M ast2500-evb,uart=uart3
>>>
>>> Cc: Abhishek Singh Dagur <abhishek@drut.io>
>>> Signed-off-by: Cédric Le Goater <clg@kaod.org>
>>> ---
>>>   docs/system/arm/aspeed.rst | 10 ++++++++++
>>>   hw/arm/aspeed.c            | 39 ++++++++++++++++++++++++++++++++++++--
>>>   2 files changed, 47 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/docs/system/arm/aspeed.rst b/docs/system/arm/aspeed.rst
>>> index d4e293e7f9..e70f0aeea3 100644
>>> --- a/docs/system/arm/aspeed.rst
>>> +++ b/docs/system/arm/aspeed.rst
>>> @@ -122,6 +122,10 @@ Options specific to Aspeed machines are :
>>>    * ``spi-model`` to change the SPI Flash model.
>>> + * ``uart`` to change the default console device. Most of the machines
>>> +   use the ``UART5`` device for a boot console, which is mapped on
>>> +   ``/dev/ttyS4`` under Linux, but it is not always the case.
>>
>> This comment ...
>>
>>>   For instance, to start the ``ast2500-evb`` machine with a different
>>>   FMC chip and a bigger (64M) SPI chip, use :
>>> @@ -129,6 +133,12 @@ FMC chip and a bigger (64M) SPI chip, use :
>>>     -M ast2500-evb,fmc-model=mx25l25635e,spi-model=mx66u51235f
>>> +To change the boot console and use device ``UART3`` (``/dev/ttyS2``
>>> +under Linux), use :
>>
>> ... and this one suggest 'boot-console' could be a better name.
>>
>> Or 'boot-console-index'.
> 
> you might be right. people expect to find the console messages on
> this device. I will think about it.
> 
>>
>>> +.. code-block:: bash
>>> +
>>> +  -M ast2500-evb,uart=uart3
>>>   Aspeed minibmc family boards (``ast1030-evb``)
>>>   ==================================================================
>>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>>> index 3d5488faf7..6c32f674b9 100644
>>> --- a/hw/arm/aspeed.c
>>> +++ b/hw/arm/aspeed.c
>>> @@ -43,6 +43,7 @@ struct AspeedMachineState {
>>>       AspeedSoCState soc;
>>>       MemoryRegion boot_rom;
>>>       bool mmio_exec;
>>> +    uint32_t uart_chosen;
>>>       char *fmc_model;
>>>       char *spi_model;
>>>   };
>>> @@ -331,10 +332,11 @@ static void 
>>> connect_serial_hds_to_uarts(AspeedMachineState *bmc)
>>>       AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
>>>       AspeedSoCState *s = &bmc->soc;
>>>       AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s);
>>> +    int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : 
>>> amc->uart_default;
>>> -    aspeed_soc_uart_set_chr(s, amc->uart_default, serial_hd(0));
>>> +    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++) {
>>> -        if (uart == amc->uart_default) {
>>> +        if (uart == uart_chosen) {
>>>               continue;
>>>           }
>>>           aspeed_soc_uart_set_chr(s, uart, serial_hd(i));
>>> @@ -1077,6 +1079,35 @@ static void aspeed_set_spi_model(Object *obj, 
>>> const char *value, Error **errp)
>>>       bmc->spi_model = g_strdup(value);
>>>   }
>>> +static char *aspeed_get_uart(Object *obj, Error **errp)
>>> +{
>>> +    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
>>> +    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
>>> +    int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : 
>>> amc->uart_default;
>>> +
>>> +    return g_strdup_printf("uart%d", uart_chosen - ASPEED_DEV_UART1 
>>> + 1);
>>> +}
>>> +
>>> +static void aspeed_set_uart(Object *obj, const char *value, Error 
>>> **errp)
>>> +{
>>> +    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
>>> +    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
>>> +    AspeedSoCClass *sc = 
>>> ASPEED_SOC_CLASS(object_class_by_name(amc->soc_name));
>>> +    int val;
>>> +
>>> +    if (sscanf(value, "uart%u", &val) != 1) {
>>> +        error_setg(errp, "Bad value for \"uart\" property");
>>
>> Why are you asking for a string and not an index?
> 
> because the literal name is what people find in the DT. See files
> arch/arm/boot/dts/aspeed-bmc-* under Linux.

OK. After looking at this file, I suppose people would expect
a "bmc-console" property name:

   -M ast2500-evb,bmc-console=uart3

>>>> +static void aspeed_set_uart(Object *obj, const char *value, Error **errp)
>>>> +{
>>>> +    AspeedMachineState *bmc = ASPEED_MACHINE(obj);
>>>> +    AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc);
>>>> +    AspeedSoCClass *sc = ASPEED_SOC_CLASS(object_class_by_name(amc->soc_name));
>>>> +    int val;
>>>> +
>>>> +    if (sscanf(value, "uart%u", &val) != 1) {
>>>> +        error_setg(errp, "Bad value for \"uart\" property");
>>>
>>> Why are you asking for a string and not an index?
>>
>> because the literal name is what people find in the DT. See files
>> arch/arm/boot/dts/aspeed-bmc-* under Linux.
> 
> OK. After looking at this file, I suppose people would expect
> a "bmc-console" property name:
> 
>    -M ast2500-evb,bmc-console=uart3
> 

yes. This is better naming for the user. I will keep the internal
'uart_chosen' though.

Thanks,

C.