diff mbox series

[2/5] hw/arm/aspeed: Select console UART from machine

Message ID 20210827210417.4022054-3-pdel@fb.com
State New
Headers show
Series hw/arm/aspeed: Add fuji machine type | expand

Commit Message

Peter Delevoryas Aug. 27, 2021, 9:04 p.m. UTC
From: Peter Delevoryas <pdel@fb.com>

This change replaces the UART serial device initialization code with machine
configuration data, making it so that we have a single code path for console
UART initialization, but allowing different machines to use different
UART's. This is relevant because the Aspeed chips have 2 debug UART's, UART5
and UART1, and while most machines just use UART5, some use UART1.

Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
 hw/arm/aspeed.c         | 7 +++++++
 hw/arm/aspeed_ast2600.c | 5 -----
 hw/arm/aspeed_soc.c     | 5 -----
 include/hw/arm/aspeed.h | 1 +
 4 files changed, 8 insertions(+), 10 deletions(-)

Comments

Cédric Le Goater Aug. 28, 2021, 8:25 a.m. UTC | #1
On 8/27/21 11:04 PM, pdel@fb.com wrote:
> From: Peter Delevoryas <pdel@fb.com>
> 
> This change replaces the UART serial device initialization code with machine
> configuration data, making it so that we have a single code path for console
> UART initialization, but allowing different machines to use different
> UART's. This is relevant because the Aspeed chips have 2 debug UART's, UART5
> and UART1, and while most machines just use UART5, some use UART1.

I think this is controlled by SCU510. If so, we should have a different HW 
strapping for the new machine and check the configuration at the SoC level,
in aspeed_ast2600.c, to change the serial initialization.


Thanks,

C. 
 
> 
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> ---
>  hw/arm/aspeed.c         | 7 +++++++
>  hw/arm/aspeed_ast2600.c | 5 -----
>  hw/arm/aspeed_soc.c     | 5 -----
>  include/hw/arm/aspeed.h | 1 +
>  4 files changed, 8 insertions(+), 10 deletions(-)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 9d43e26c51..ff53d12395 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -14,6 +14,7 @@
>  #include "hw/arm/boot.h"
>  #include "hw/arm/aspeed.h"
>  #include "hw/arm/aspeed_soc.h"
> +#include "hw/char/serial.h"
>  #include "hw/i2c/i2c_mux_pca954x.h"
>  #include "hw/i2c/smbus_eeprom.h"
>  #include "hw/misc/pca9552.h"
> @@ -21,6 +22,7 @@
>  #include "hw/misc/led.h"
>  #include "hw/qdev-properties.h"
>  #include "sysemu/block-backend.h"
> +#include "sysemu/sysemu.h"
>  #include "hw/loader.h"
>  #include "qemu/error-report.h"
>  #include "qemu/units.h"
> @@ -352,6 +354,10 @@ static void aspeed_machine_init(MachineState *machine)
>      }
>      qdev_realize(DEVICE(&bmc->soc), NULL, &error_abort);
>  
> +    serial_mm_init(get_system_memory(), sc->memmap[amc->serial_dev], 2,
> +                   sc->get_irq(&bmc->soc, amc->serial_dev), 38400,
> +                   serial_hd(0), DEVICE_LITTLE_ENDIAN);
> +
>      memory_region_add_subregion(get_system_memory(),
>                                  sc->memmap[ASPEED_DEV_SDRAM],
>                                  &bmc->ram_container);
> @@ -804,6 +810,7 @@ static void aspeed_machine_class_init(ObjectClass *oc, void *data)
>      mc->no_parallel = 1;
>      mc->default_ram_id = "ram";
>      amc->macs_mask = ASPEED_MAC0_ON;
> +    amc->serial_dev = ASPEED_DEV_UART5;
>  
>      aspeed_machine_class_props_init(oc);
>  }
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index 56e1adb343..a27b0de482 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -322,11 +322,6 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
>      }
>  
> -    /* UART - attach an 8250 to the IO space as our UART5 */
> -    serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
> -                   aspeed_soc_get_irq(s, ASPEED_DEV_UART5),
> -                   38400, serial_hd(0), DEVICE_LITTLE_ENDIAN);
> -
>      /* I2C */
>      object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
>                               &error_abort);
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index c373182299..0c09d1e5b4 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -287,11 +287,6 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
>      }
>  
> -    /* UART - attach an 8250 to the IO space as our UART5 */
> -    serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
> -                   aspeed_soc_get_irq(s, ASPEED_DEV_UART5), 38400,
> -                   serial_hd(0), DEVICE_LITTLE_ENDIAN);
> -
>      /* I2C */
>      object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
>                               &error_abort);
> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
> index c9747b15fc..9f5b9f04d6 100644
> --- a/include/hw/arm/aspeed.h
> +++ b/include/hw/arm/aspeed.h
> @@ -38,6 +38,7 @@ struct AspeedMachineClass {
>      uint32_t num_cs;
>      uint32_t macs_mask;
>      void (*i2c_init)(AspeedMachineState *bmc);
> +    uint32_t serial_dev;
>  };
>  
>  
>
Peter Delevoryas Aug. 28, 2021, 3:58 p.m. UTC | #2
I think I’m a little confused on this part. What I meant by “most machines just use UART5” was that most DTS’s use “stdout-path=&uart5”, but fuji uses “stdout-path=&uart1”. I do see that SCU510 includes a bit related to UART, but it’s for disabling booting from UART1 and UART5. I just care about the console aspect, not booting.

This is the commit that changed the serial console from UART5 to UART1 in fuji’s DTS: https://github.com/facebook/openbmc-uboot/commit/afeddd6e27b5f094bbc4805dc8c1c22b3b7fb203

I don’t know what the platform.S AST_SCU_MFP_CTRL7 changes do (maybe setting some GPIO for UART1?), but as far as I understand, I don’t think using UART1 should require any extra registers from the datasheet.

An alternate design I considered was UART5=serial_hd(0) and UART1=serial_hd(1), maybe that would be more appropriate? I don’t think anybody uses both UART’s simultaneously though, so I didn’t pursue that design.

Some link references:
Elbert DTS uses “stdout-path=&uart5” https://github.com/facebook/openbmc-uboot/blob/openbmc/helium/v2019.04/arch/arm/dts/aspeed-bmc-facebook-elbert.dts#L17
Fuji DTS uses “stdout-path=&uart1” https://github.com/facebook/openbmc-uboot/blob/openbmc/helium/v2019.04/arch/arm/dts/aspeed-bmc-facebook-fuji.dts#L17

From: Cédric Le Goater <clg@kaod.org>
Date: Saturday, August 28, 2021 at 1:26 AM
To: Peter Delevoryas <pdel@fb.com>
Cc: joel@jms.id.au <joel@jms.id.au>, qemu-devel@nongnu.org <qemu-devel@nongnu.org>, qemu-arm@nongnu.org <qemu-arm@nongnu.org>
Subject: Re: [PATCH 2/5] hw/arm/aspeed: Select console UART from machine
On 8/27/21 11:04 PM, pdel@fb.com wrote:
> From: Peter Delevoryas <pdel@fb.com>
>
> This change replaces the UART serial device initialization code with machine
> configuration data, making it so that we have a single code path for console
> UART initialization, but allowing different machines to use different
> UART's. This is relevant because the Aspeed chips have 2 debug UART's, UART5
> and UART1, and while most machines just use UART5, some use UART1.

I think this is controlled by SCU510. If so, we should have a different HW
strapping for the new machine and check the configuration at the SoC level,
in aspeed_ast2600.c, to change the serial initialization.


Thanks,

C.

>
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> ---
>  hw/arm/aspeed.c         | 7 +++++++
>  hw/arm/aspeed_ast2600.c | 5 -----
>  hw/arm/aspeed_soc.c     | 5 -----
>  include/hw/arm/aspeed.h | 1 +
>  4 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index 9d43e26c51..ff53d12395 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -14,6 +14,7 @@
>  #include "hw/arm/boot.h"
>  #include "hw/arm/aspeed.h"
>  #include "hw/arm/aspeed_soc.h"
> +#include "hw/char/serial.h"
>  #include "hw/i2c/i2c_mux_pca954x.h"
>  #include "hw/i2c/smbus_eeprom.h"
>  #include "hw/misc/pca9552.h"
> @@ -21,6 +22,7 @@
>  #include "hw/misc/led.h"
>  #include "hw/qdev-properties.h"
>  #include "sysemu/block-backend.h"
> +#include "sysemu/sysemu.h"
>  #include "hw/loader.h"
>  #include "qemu/error-report.h"
>  #include "qemu/units.h"
> @@ -352,6 +354,10 @@ static void aspeed_machine_init(MachineState *machine)
>      }
>      qdev_realize(DEVICE(&bmc->soc), NULL, &error_abort);
>
> +    serial_mm_init(get_system_memory(), sc->memmap[amc->serial_dev], 2,
> +                   sc->get_irq(&bmc->soc, amc->serial_dev), 38400,
> +                   serial_hd(0), DEVICE_LITTLE_ENDIAN);
> +
>      memory_region_add_subregion(get_system_memory(),
>                                  sc->memmap[ASPEED_DEV_SDRAM],
>                                  &bmc->ram_container);
> @@ -804,6 +810,7 @@ static void aspeed_machine_class_init(ObjectClass *oc, void *data)
>      mc->no_parallel = 1;
>      mc->default_ram_id = "ram";
>      amc->macs_mask = ASPEED_MAC0_ON;
> +    amc->serial_dev = ASPEED_DEV_UART5;
>
>      aspeed_machine_class_props_init(oc);
>  }
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index 56e1adb343..a27b0de482 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -322,11 +322,6 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
>      }
>
> -    /* UART - attach an 8250 to the IO space as our UART5 */
> -    serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
> -                   aspeed_soc_get_irq(s, ASPEED_DEV_UART5),
> -                   38400, serial_hd(0), DEVICE_LITTLE_ENDIAN);
> -
>      /* I2C */
>      object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
>                               &error_abort);
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index c373182299..0c09d1e5b4 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -287,11 +287,6 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
>      }
>
> -    /* UART - attach an 8250 to the IO space as our UART5 */
> -    serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
> -                   aspeed_soc_get_irq(s, ASPEED_DEV_UART5), 38400,
> -                   serial_hd(0), DEVICE_LITTLE_ENDIAN);
> -
>      /* I2C */
>      object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
>                               &error_abort);
> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
> index c9747b15fc..9f5b9f04d6 100644
> --- a/include/hw/arm/aspeed.h
> +++ b/include/hw/arm/aspeed.h
> @@ -38,6 +38,7 @@ struct AspeedMachineClass {
>      uint32_t num_cs;
>      uint32_t macs_mask;
>      void (*i2c_init)(AspeedMachineState *bmc);
> +    uint32_t serial_dev;
>  };
>
>
>
Cédric Le Goater Aug. 31, 2021, 8:15 a.m. UTC | #3
On 8/28/21 5:58 PM, Peter Delevoryas wrote:
> I think I’m a little confused on this part. What I meant by “most machines just use UART5” was that most DTS’s use “stdout-path=&uart5”, but fuji uses “stdout-path=&uart1”. I /do/ see that SCU510 includes a bit related to UART, but it’s for disabling booting from UART1 and UART5. I just care about the console aspect, not booting.

OK. 

We want to keep the initialization of the SoC devices under the SoC and not 
under the board. That's the main reason behind my suggestion. 
 
>  
> 
> This is the commit that changed the serial console from UART5 to UART1 in fuji’s DTS: https://github.com/facebook/openbmc-uboot/commit/afeddd6e27b5f094bbc4805dc8c1c22b3b7fb203 <https://github.com/facebook/openbmc-uboot/commit/afeddd6e27b5f094bbc4805dc8c1c22b3b7fb203>
> 
>  
> 
> I don’t know what the platform.S AST_SCU_MFP_CTRL7 changes do (maybe setting some GPIO for UART1?), but as far as I understand, I don’t think using UART1 should require any extra registers from the datasheet.
> 
>  
> 
> An alternate design I considered was UART5=serial_hd(0) and UART1=serial_hd(1),
> maybe that would be more appropriate? I don’t think anybody uses both UART’s 
> simultaneously though, so I didn’t pursue that design.

I would prefer that. This patch has been in my tree for years :

  https://github.com/legoater/qemu/commit/138713ee1d3d84682b85c1b01577a7e86ab3fed4

See

  https://github.com/legoater/qemu/commits/aspeed-6.2

May be this is what you simply need ? You can use the ast2600-evb machine for 
your tests. Tell me how it goes. 

AFAICT, you will need to resend PATCH 5 only.

Thanks,

C.


>  
> 
> Some link references:
> 
> Elbert DTS uses “stdout-path=&uart5” https://github.com/facebook/openbmc-uboot/blob/openbmc/helium/v2019.04/arch/arm/dts/aspeed-bmc-facebook-elbert.dts#L17
> 
> Fuji DTS uses “stdout-path=&uart1” https://github.com/facebook/openbmc-uboot/blob/openbmc/helium/v2019.04/arch/arm/dts/aspeed-bmc-facebook-fuji.dts#L17
> 
>  
> 
> *From: *Cédric Le Goater <clg@kaod.org>
> *Date: *Saturday, August 28, 2021 at 1:26 AM
> *To: *Peter Delevoryas <pdel@fb.com>
> *Cc: *joel@jms.id.au <joel@jms.id.au>, qemu-devel@nongnu.org <qemu-devel@nongnu.org>, qemu-arm@nongnu.org <qemu-arm@nongnu.org>
> *Subject: *Re: [PATCH 2/5] hw/arm/aspeed: Select console UART from machine
> 
> On 8/27/21 11:04 PM, pdel@fb.com wrote:
>> From: Peter Delevoryas <pdel@fb.com>
>>
>> This change replaces the UART serial device initialization code with machine
>> configuration data, making it so that we have a single code path for console
>> UART initialization, but allowing different machines to use different
>> UART's. This is relevant because the Aspeed chips have 2 debug UART's, UART5
>> and UART1, and while most machines just use UART5, some use UART1.
> 
> I think this is controlled by SCU510. If so, we should have a different HW
> strapping for the new machine and check the configuration at the SoC level,
> in aspeed_ast2600.c, to change the serial initialization.
> 
> 
> Thanks,
> 
> C.
>  
>>
>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>> ---
>>  hw/arm/aspeed.c         | 7 +++++++
>>  hw/arm/aspeed_ast2600.c | 5 -----
>>  hw/arm/aspeed_soc.c     | 5 -----
>>  include/hw/arm/aspeed.h | 1 +
>>  4 files changed, 8 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 9d43e26c51..ff53d12395 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -14,6 +14,7 @@
>>  #include "hw/arm/boot.h"
>>  #include "hw/arm/aspeed.h"
>>  #include "hw/arm/aspeed_soc.h"
>> +#include "hw/char/serial.h"
>>  #include "hw/i2c/i2c_mux_pca954x.h"
>>  #include "hw/i2c/smbus_eeprom.h"
>>  #include "hw/misc/pca9552.h"
>> @@ -21,6 +22,7 @@
>>  #include "hw/misc/led.h"
>>  #include "hw/qdev-properties.h"
>>  #include "sysemu/block-backend.h"
>> +#include "sysemu/sysemu.h"
>>  #include "hw/loader.h"
>>  #include "qemu/error-report.h"
>>  #include "qemu/units.h"
>> @@ -352,6 +354,10 @@ static void aspeed_machine_init(MachineState *machine)
>>      }
>>      qdev_realize(DEVICE(&bmc->soc), NULL, &error_abort);
>> 
>> +    serial_mm_init(get_system_memory(), sc->memmap[amc->serial_dev], 2,
>> +                   sc->get_irq(&bmc->soc, amc->serial_dev), 38400,
>> +                   serial_hd(0), DEVICE_LITTLE_ENDIAN);
>> +
>>      memory_region_add_subregion(get_system_memory(),
>>                                  sc->memmap[ASPEED_DEV_SDRAM],
>>                                  &bmc->ram_container);
>> @@ -804,6 +810,7 @@ static void aspeed_machine_class_init(ObjectClass *oc, void *data)
>>      mc->no_parallel = 1;
>>      mc->default_ram_id = "ram";
>>      amc->macs_mask = ASPEED_MAC0_ON;
>> +    amc->serial_dev = ASPEED_DEV_UART5;
>> 
>>      aspeed_machine_class_props_init(oc);
>>  }
>> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
>> index 56e1adb343..a27b0de482 100644
>> --- a/hw/arm/aspeed_ast2600.c
>> +++ b/hw/arm/aspeed_ast2600.c
>> @@ -322,11 +322,6 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
>>      }
>> 
>> -    /* UART - attach an 8250 to the IO space as our UART5 */
>> -    serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
>> -                   aspeed_soc_get_irq(s, ASPEED_DEV_UART5),
>> -                   38400, serial_hd(0), DEVICE_LITTLE_ENDIAN);
>> -
>>      /* I2C */
>>      object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
>>                               &error_abort);
>> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
>> index c373182299..0c09d1e5b4 100644
>> --- a/hw/arm/aspeed_soc.c
>> +++ b/hw/arm/aspeed_soc.c
>> @@ -287,11 +287,6 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
>>      }
>> 
>> -    /* UART - attach an 8250 to the IO space as our UART5 */
>> -    serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
>> -                   aspeed_soc_get_irq(s, ASPEED_DEV_UART5), 38400,
>> -                   serial_hd(0), DEVICE_LITTLE_ENDIAN);
>> -
>>      /* I2C */
>>      object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
>>                               &error_abort);
>> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
>> index c9747b15fc..9f5b9f04d6 100644
>> --- a/include/hw/arm/aspeed.h
>> +++ b/include/hw/arm/aspeed.h
>> @@ -38,6 +38,7 @@ struct AspeedMachineClass {
>>      uint32_t num_cs;
>>      uint32_t macs_mask;
>>      void (*i2c_init)(AspeedMachineState *bmc);
>> +    uint32_t serial_dev;
>>  };
>> 
>> 
>>
>
Cédric Le Goater Aug. 31, 2021, 10:39 a.m. UTC | #4
On 8/28/21 5:58 PM, Peter Delevoryas wrote:
> I think I’m a little confused on this part. What I meant by “most machines just use UART5” was that most DTS’s use “stdout-path=&uart5”, but fuji uses “stdout-path=&uart1”. I /do/ see that SCU510 includes a bit related to UART, but it’s for disabling booting from UART1 and UART5. I just care about the console aspect, not booting.

The UART can be switched with SCU70[29] on the AST2500, btw.

>
> 
> This is the commit that changed the serial console from UART5 to UART1 in fuji’s DTS: https://github.com/facebook/openbmc-uboot/commit/afeddd6e27b5f094bbc4805dc8c1c22b3b7fb203 <https://github.com/facebook/openbmc-uboot/commit/afeddd6e27b5f094bbc4805dc8c1c22b3b7fb203>

That's an AST2620-A1. May be we should also introduce a new SoC then. I will try
to get the datasheet.
  
> I don’t know what the platform.S AST_SCU_MFP_CTRL7 changes do (maybe setting some GPIO for UART1?),

Yes. The patch above enables the UART1 RX function. 

Is TX always enabled ? Something to check on real HW.

>  but as far as I understand, I don’t think using UART1 should require any extra registers from the datasheet.
>  
> 
> An alternate design I considered was UART5=serial_hd(0) and UART1=serial_hd(1), maybe that would be more appropriate? I don’t think anybody uses both UART’s simultaneously though, so I didn’t pursue that design.

An other simple idea would be activate both UART5 and UART1 on the AST2600 
SoC if this is how the HW works. Or add a bool "enable-uart1" at the SoC 
level and set it from the machine. 

In any case, we will need a serial backend for both.


Thanks,

C.

>  
> 
> Some link references:
> 
> Elbert DTS uses “stdout-path=&uart5” https://github.com/facebook/openbmc-uboot/blob/openbmc/helium/v2019.04/arch/arm/dts/aspeed-bmc-facebook-elbert.dts#L17
> 
> Fuji DTS uses “stdout-path=&uart1” https://github.com/facebook/openbmc-uboot/blob/openbmc/helium/v2019.04/arch/arm/dts/aspeed-bmc-facebook-fuji.dts#L17
> 
>  
> 
> *From: *Cédric Le Goater <clg@kaod.org>
> *Date: *Saturday, August 28, 2021 at 1:26 AM
> *To: *Peter Delevoryas <pdel@fb.com>
> *Cc: *joel@jms.id.au <joel@jms.id.au>, qemu-devel@nongnu.org <qemu-devel@nongnu.org>, qemu-arm@nongnu.org <qemu-arm@nongnu.org>
> *Subject: *Re: [PATCH 2/5] hw/arm/aspeed: Select console UART from machine
> 
> On 8/27/21 11:04 PM, pdel@fb.com wrote:
>> From: Peter Delevoryas <pdel@fb.com>
>>
>> This change replaces the UART serial device initialization code with machine
>> configuration data, making it so that we have a single code path for console
>> UART initialization, but allowing different machines to use different
>> UART's. This is relevant because the Aspeed chips have 2 debug UART's, UART5
>> and UART1, and while most machines just use UART5, some use UART1.
> 
> I think this is controlled by SCU510. If so, we should have a different HW
> strapping for the new machine and check the configuration at the SoC level,
> in aspeed_ast2600.c, to change the serial initialization.
> 
> 
> Thanks,
> 
> C.
>  
>>
>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>> ---
>>  hw/arm/aspeed.c         | 7 +++++++
>>  hw/arm/aspeed_ast2600.c | 5 -----
>>  hw/arm/aspeed_soc.c     | 5 -----
>>  include/hw/arm/aspeed.h | 1 +
>>  4 files changed, 8 insertions(+), 10 deletions(-)
>>
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index 9d43e26c51..ff53d12395 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -14,6 +14,7 @@
>>  #include "hw/arm/boot.h"
>>  #include "hw/arm/aspeed.h"
>>  #include "hw/arm/aspeed_soc.h"
>> +#include "hw/char/serial.h"
>>  #include "hw/i2c/i2c_mux_pca954x.h"
>>  #include "hw/i2c/smbus_eeprom.h"
>>  #include "hw/misc/pca9552.h"
>> @@ -21,6 +22,7 @@
>>  #include "hw/misc/led.h"
>>  #include "hw/qdev-properties.h"
>>  #include "sysemu/block-backend.h"
>> +#include "sysemu/sysemu.h"
>>  #include "hw/loader.h"
>>  #include "qemu/error-report.h"
>>  #include "qemu/units.h"
>> @@ -352,6 +354,10 @@ static void aspeed_machine_init(MachineState *machine)
>>      }
>>      qdev_realize(DEVICE(&bmc->soc), NULL, &error_abort);
>> 
>> +    serial_mm_init(get_system_memory(), sc->memmap[amc->serial_dev], 2,
>> +                   sc->get_irq(&bmc->soc, amc->serial_dev), 38400,
>> +                   serial_hd(0), DEVICE_LITTLE_ENDIAN);
>> +
>>      memory_region_add_subregion(get_system_memory(),
>>                                  sc->memmap[ASPEED_DEV_SDRAM],
>>                                  &bmc->ram_container);
>> @@ -804,6 +810,7 @@ static void aspeed_machine_class_init(ObjectClass *oc, void *data)
>>      mc->no_parallel = 1;
>>      mc->default_ram_id = "ram";
>>      amc->macs_mask = ASPEED_MAC0_ON;
>> +    amc->serial_dev = ASPEED_DEV_UART5;
>> 
>>      aspeed_machine_class_props_init(oc);
>>  }
>> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
>> index 56e1adb343..a27b0de482 100644
>> --- a/hw/arm/aspeed_ast2600.c
>> +++ b/hw/arm/aspeed_ast2600.c
>> @@ -322,11 +322,6 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
>>      }
>> 
>> -    /* UART - attach an 8250 to the IO space as our UART5 */
>> -    serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
>> -                   aspeed_soc_get_irq(s, ASPEED_DEV_UART5),
>> -                   38400, serial_hd(0), DEVICE_LITTLE_ENDIAN);
>> -
>>      /* I2C */
>>      object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
>>                               &error_abort);
>> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
>> index c373182299..0c09d1e5b4 100644
>> --- a/hw/arm/aspeed_soc.c
>> +++ b/hw/arm/aspeed_soc.c
>> @@ -287,11 +287,6 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>>          sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
>>      }
>> 
>> -    /* UART - attach an 8250 to the IO space as our UART5 */
>> -    serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
>> -                   aspeed_soc_get_irq(s, ASPEED_DEV_UART5), 38400,
>> -                   serial_hd(0), DEVICE_LITTLE_ENDIAN);
>> -
>>      /* I2C */
>>      object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
>>                               &error_abort);
>> diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
>> index c9747b15fc..9f5b9f04d6 100644
>> --- a/include/hw/arm/aspeed.h
>> +++ b/include/hw/arm/aspeed.h
>> @@ -38,6 +38,7 @@ struct AspeedMachineClass {
>>      uint32_t num_cs;
>>      uint32_t macs_mask;
>>      void (*i2c_init)(AspeedMachineState *bmc);
>> +    uint32_t serial_dev;
>>  };
>> 
>> 
>>
>
Andrew Jeffery Aug. 31, 2021, 11:23 a.m. UTC | #5
Hi Cédric, Peter,

On Tue, 31 Aug 2021, at 20:09, Cédric Le Goater wrote:
> On 8/28/21 5:58 PM, Peter Delevoryas wrote:
> > I think I’m a little confused on this part. What I meant by “most machines just use UART5” was that most DTS’s use “stdout-path=&uart5”, but fuji uses “stdout-path=&uart1”. I /do/ see that SCU510 includes a bit related to UART, but it’s for disabling booting from UART1 and UART5. I just care about the console aspect, not booting.
> 
> The UART can be switched with SCU70[29] on the AST2500, btw.

If it helps, neither the AST2600's "Boot from UART" feature nor the 
AST2[456]00's "Debug UART" feature are related to which UART is used as 
the BMC console by u-boot and/or the kernel - the latter is entirely a 
software thing.

The "Debug UART" is a hardware backdoor, a UART-to-AHB bridge 
implemented by the SoC. It provides a shell environment that allows you 
to issue transactions directly on the AHB if you perform a magic knock. 
I have a driver for it implemented here:

https://github.com/amboar/cve-2019-6260/blob/master/src/debug.c

SCU70[29] on the AST2500 selects whether this backdoor is exposed on 
UART1 or UART5.

The "Boot from UART" feature is implemented in the AST2600 ROM code as 
a fallback for loading the SPL if fetching it from SPI-NOR or the eMMC 
fails, or the SPL is incorrectly signed for secure-boot.

I think Peter is on the right track with this patch?

Andrew
Cédric Le Goater Aug. 31, 2021, 1:34 p.m. UTC | #6
On 8/31/21 1:23 PM, Andrew Jeffery wrote:
> Hi Cédric, Peter,
> 
> On Tue, 31 Aug 2021, at 20:09, Cédric Le Goater wrote:
>> On 8/28/21 5:58 PM, Peter Delevoryas wrote:
>>> I think I’m a little confused on this part. What I meant by “most machines just use UART5” was that most DTS’s use “stdout-path=&uart5”, but fuji uses “stdout-path=&uart1”. I /do/ see that SCU510 includes a bit related to UART, but it’s for disabling booting from UART1 and UART5. I just care about the console aspect, not booting.
>>
>> The UART can be switched with SCU70[29] on the AST2500, btw.
> 
> If it helps, neither the AST2600's "Boot from UART" feature nor the 
> AST2[456]00's "Debug UART" feature are related to which UART is used as 
> the BMC console by u-boot and/or the kernel - the latter is entirely a 
> software thing.

ok. 

I don't think we should initialize all 5 UARTs of SoC and let the user define 
all the expected devices on the command. Unless we want to do something like
'macs_mask' ? but at the SoC level. It might be overkill for the need.

My suggestion is have the Aspeed board tell the SoC which uart was selected 
for the console. That can be done with an extra "serial-dev" int property at 
the SoC level, defaults to ASPEED_DEV_UART5, like for the machine. 

The serial init needs a change  : 

    /* UART - attach an 8250 to the IO space as our UART5 */
    serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
                   aspeed_soc_get_irq(s, ASPEED_DEV_UART5), 38400,
                   serial_hd(0), DEVICE_LITTLE_ENDIAN);

but it stays where it is currently, under the SoC.

> The "Debug UART" is a hardware backdoor, a UART-to-AHB bridge 
> implemented by the SoC. It provides a shell environment that allows you 
> to issue transactions directly on the AHB if you perform a magic knock. 
> I have a driver for it implemented here:
> 
> https://github.com/amboar/cve-2019-6260/blob/master/src/debug.c
> 
> SCU70[29] on the AST2500 selects whether this backdoor is exposed on 
> UART1 or UART5.
> 
> The "Boot from UART" feature is implemented in the AST2600 ROM code as 
> a fallback for loading the SPL if fetching it from SPI-NOR or the eMMC 
> fails, or the SPL is incorrectly signed for secure-boot.
>
> I think Peter is on the right track with this patch?

Yes. nearly. Sorry for the confusion on how to handle this Peter. A machine 
*and* a SoC property should to the trick. 

'amc->serial_dev' is a good idea. You need a similar one under the SoC.

Thanks for the feedback Andrew,

C.
Peter Delevoryas Aug. 31, 2021, 1:51 p.m. UTC | #7
I’ll resend PATCH 5, and like you’re suggesting, I can use the existing ast2600-evb machine type for testing.

    # Setup serial_hd(1) as UART1 in 2600 SoC realize (I won’t include this in the diff though)
    UART5=serial_hd(0)
    UART1=serial_hd(1)

    qemu-system-arm -machine ast2600-evb -serial null -serial stdio

> On Aug 31, 2021, at 1:15 AM, Cédric Le Goater <clg@kaod.org> wrote:
> 
> I would prefer that. This patch has been in my tree for years :
> 
>  https://github.com/legoater/qemu/commit/138713ee1d3d84682b85c1b01577a7e86ab3fed4
> 
> See
> 
>  https://github.com/legoater/qemu/commits/aspeed-6.2
> 
> May be this is what you simply need ? You can use the ast2600-evb machine for 
> your tests. Tell me how it goes. 
> 
> AFAICT, you will need to resend PATCH 5 only.
> 
> Thanks,
> 
> C.
Cédric Le Goater Aug. 31, 2021, 2:06 p.m. UTC | #8
On 8/31/21 3:51 PM, Peter Delevoryas wrote:
> I’ll resend PATCH 5, and like you’re suggesting, I can use the existing ast2600-evb machine type for testing.
> 
>     # Setup serial_hd(1) as UART1 in 2600 SoC realize (I won’t include this in the diff though)
>     UART5=serial_hd(0)
>     UART1=serial_hd(1)
> 
>     qemu-system-arm -machine ast2600-evb -serial null -serial stdio

yes. that's one way to do it. But it's a bit hacky. 

On the other hand, if we were to initialize all 5 UARTs, the user would 
have to add 4 "-serial null" devices to use UART5 as a console (and none 
for UART1). Which is not that good for a user API. We added the 'macs_mask' 
for a similar reason btw.

I think your proposal plus the extra SoC property is better though. 

Thanks for testing.

C.
Peter Delevoryas Aug. 31, 2021, 2:07 p.m. UTC | #9
> On Aug 31, 2021, at 6:34 AM, Cédric Le Goater <clg@kaod.org> wrote:
> 
> On 8/31/21 1:23 PM, Andrew Jeffery wrote:
>> Hi Cédric, Peter,
>> 
>> On Tue, 31 Aug 2021, at 20:09, Cédric Le Goater wrote:
>>> On 8/28/21 5:58 PM, Peter Delevoryas wrote:
>>>> I think I’m a little confused on this part. What I meant by “most machines just use UART5” was that most DTS’s use “stdout-path=&uart5”, but fuji uses “stdout-path=&uart1”. I /do/ see that SCU510 includes a bit related to UART, but it’s for disabling booting from UART1 and UART5. I just care about the console aspect, not booting.
>>> 
>>> The UART can be switched with SCU70[29] on the AST2500, btw.
>> 
>> If it helps, neither the AST2600's "Boot from UART" feature nor the 
>> AST2[456]00's "Debug UART" feature are related to which UART is used as 
>> the BMC console by u-boot and/or the kernel - the latter is entirely a 
>> software thing.
> 
> ok. 
> 
> I don't think we should initialize all 5 UARTs of SoC and let the user define 
> all the expected devices on the command. Unless we want to do something like
> 'macs_mask' ? but at the SoC level. It might be overkill for the need.
> 
> My suggestion is have the Aspeed board tell the SoC which uart was selected 
> for the console. That can be done with an extra "serial-dev" int property at 
> the SoC level, defaults to ASPEED_DEV_UART5, like for the machine. 
> 
> The serial init needs a change  : 
> 
>    /* UART - attach an 8250 to the IO space as our UART5 */
>    serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
>                   aspeed_soc_get_irq(s, ASPEED_DEV_UART5), 38400,
>                   serial_hd(0), DEVICE_LITTLE_ENDIAN);
> 
> but it stays where it is currently, under the SoC.

Oh ok, yeah that design sounds good to me, I can submit a patch like that.

I’ll make sure to resubmit PATCH 5, the register fixes, separately though.

> 
>> The "Debug UART" is a hardware backdoor, a UART-to-AHB bridge 
>> implemented by the SoC. It provides a shell environment that allows you 
>> to issue transactions directly on the AHB if you perform a magic knock. 
>> I have a driver for it implemented here:
>> 
>> https://github.com/amboar/cve-2019-6260/blob/master/src/debug.c
>> 
>> SCU70[29] on the AST2500 selects whether this backdoor is exposed on 
>> UART1 or UART5.
>> 
>> The "Boot from UART" feature is implemented in the AST2600 ROM code as 
>> a fallback for loading the SPL if fetching it from SPI-NOR or the eMMC 
>> fails, or the SPL is incorrectly signed for secure-boot.
>> 
>> I think Peter is on the right track with this patch?
> 
> Yes. nearly. Sorry for the confusion on how to handle this Peter. A machine 
> *and* a SoC property should to the trick. 
> 
> 'amc->serial_dev' is a good idea. You need a similar one under the SoC.
> 
> Thanks for the feedback Andrew,
> 
> C. 

Ah, thanks for clarifying Andrew! I was looking at the datasheet again yesterday,
and it seemed like the “debug” and “boot” features might be distinct from rx/tx,
but to be honest I had no idea and came to this opinion mostly by accident.

So, I’ll resubmit PATCH 5 separately, and submit another patch with this
machine + SoC property design by itself.

Thanks for your consideration, I really appreciate it!

Thanks,
Peter
Philippe Mathieu-Daudé Aug. 31, 2021, 3:57 p.m. UTC | #10
On 8/31/21 3:34 PM, Cédric Le Goater wrote:
> On 8/31/21 1:23 PM, Andrew Jeffery wrote:
>> Hi Cédric, Peter,
>>
>> On Tue, 31 Aug 2021, at 20:09, Cédric Le Goater wrote:
>>> On 8/28/21 5:58 PM, Peter Delevoryas wrote:
>>>> I think I’m a little confused on this part. What I meant by “most machines just use UART5” was that most DTS’s use “stdout-path=&uart5”, but fuji uses “stdout-path=&uart1”. I /do/ see that SCU510 includes a bit related to UART, but it’s for disabling booting from UART1 and UART5. I just care about the console aspect, not booting.
>>>
>>> The UART can be switched with SCU70[29] on the AST2500, btw.
>>
>> If it helps, neither the AST2600's "Boot from UART" feature nor the 
>> AST2[456]00's "Debug UART" feature are related to which UART is used as 
>> the BMC console by u-boot and/or the kernel - the latter is entirely a 
>> software thing.
> 
> ok. 
> 
> I don't think we should initialize all 5 UARTs of SoC and let the user define 
> all the expected devices on the command. Unless we want to do something like
> 'macs_mask' ? but at the SoC level. It might be overkill for the need.

I'm not sure I'm following what you are suggesting. If we are talking
about QEMU device initialization, QEMU must initialize all devices
on the board, regardless the guest code uses them or not.
Cédric Le Goater Aug. 31, 2021, 4:37 p.m. UTC | #11
>> I don't think we should initialize all 5 UARTs of SoC and let the user define 
>> all the expected devices on the command. Unless we want to do something like
>> 'macs_mask' ? but at the SoC level. It might be overkill for the need.
> 
> I'm not sure I'm following what you are suggesting. If we are talking
> about QEMU device initialization, QEMU must initialize all devices
> on the board, regardless the guest code uses them or not.

The console is UART5 by default for all Aspeed boards and the SoC model 
choose not to initialize UARTs [1-4] to simplify the command line and
avoid : 

  -serial null -serial null -serial null -serial null -serial stdio

This new fuji board uses a firmware which enables UART1 for the console. 
So we have to change which UART is initialized. The simplest way is 
to tell the SoC through a property and change appropriately :

    serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
                   aspeed_soc_get_irq(s, ASPEED_DEV_UART5), 38400,
                   serial_hd(0), DEVICE_LITTLE_ENDIAN);

The above is doing the shortcut : serial0 <-> UART5.

Cheers,

C.
diff mbox series

Patch

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index 9d43e26c51..ff53d12395 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -14,6 +14,7 @@ 
 #include "hw/arm/boot.h"
 #include "hw/arm/aspeed.h"
 #include "hw/arm/aspeed_soc.h"
+#include "hw/char/serial.h"
 #include "hw/i2c/i2c_mux_pca954x.h"
 #include "hw/i2c/smbus_eeprom.h"
 #include "hw/misc/pca9552.h"
@@ -21,6 +22,7 @@ 
 #include "hw/misc/led.h"
 #include "hw/qdev-properties.h"
 #include "sysemu/block-backend.h"
+#include "sysemu/sysemu.h"
 #include "hw/loader.h"
 #include "qemu/error-report.h"
 #include "qemu/units.h"
@@ -352,6 +354,10 @@  static void aspeed_machine_init(MachineState *machine)
     }
     qdev_realize(DEVICE(&bmc->soc), NULL, &error_abort);
 
+    serial_mm_init(get_system_memory(), sc->memmap[amc->serial_dev], 2,
+                   sc->get_irq(&bmc->soc, amc->serial_dev), 38400,
+                   serial_hd(0), DEVICE_LITTLE_ENDIAN);
+
     memory_region_add_subregion(get_system_memory(),
                                 sc->memmap[ASPEED_DEV_SDRAM],
                                 &bmc->ram_container);
@@ -804,6 +810,7 @@  static void aspeed_machine_class_init(ObjectClass *oc, void *data)
     mc->no_parallel = 1;
     mc->default_ram_id = "ram";
     amc->macs_mask = ASPEED_MAC0_ON;
+    amc->serial_dev = ASPEED_DEV_UART5;
 
     aspeed_machine_class_props_init(oc);
 }
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 56e1adb343..a27b0de482 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -322,11 +322,6 @@  static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
         sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
     }
 
-    /* UART - attach an 8250 to the IO space as our UART5 */
-    serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
-                   aspeed_soc_get_irq(s, ASPEED_DEV_UART5),
-                   38400, serial_hd(0), DEVICE_LITTLE_ENDIAN);
-
     /* I2C */
     object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
                              &error_abort);
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index c373182299..0c09d1e5b4 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -287,11 +287,6 @@  static void aspeed_soc_realize(DeviceState *dev, Error **errp)
         sysbus_connect_irq(SYS_BUS_DEVICE(&s->timerctrl), i, irq);
     }
 
-    /* UART - attach an 8250 to the IO space as our UART5 */
-    serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
-                   aspeed_soc_get_irq(s, ASPEED_DEV_UART5), 38400,
-                   serial_hd(0), DEVICE_LITTLE_ENDIAN);
-
     /* I2C */
     object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
                              &error_abort);
diff --git a/include/hw/arm/aspeed.h b/include/hw/arm/aspeed.h
index c9747b15fc..9f5b9f04d6 100644
--- a/include/hw/arm/aspeed.h
+++ b/include/hw/arm/aspeed.h
@@ -38,6 +38,7 @@  struct AspeedMachineClass {
     uint32_t num_cs;
     uint32_t macs_mask;
     void (*i2c_init)(AspeedMachineState *bmc);
+    uint32_t serial_dev;
 };