Re: [PATCH 11/12] aspeed: Introduce a "uart" machine option

[Philippe Mathieu-Daudé]

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