| Message ID | 20210827210417.4022054-3-pdel@fb.com |
|---|---|
| State | New |
| Headers | show |
| Series | hw/arm/aspeed: Add fuji machine type | expand |
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; > }; > > >
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; > }; > > >
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; >> }; >> >> >> >
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; >> }; >> >> >> >
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
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.
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.
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.
> 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
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.
>> 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 --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; };