* [PATCH 0/2] hw: aspeed: Init all UART's with serial devices
@ 2022-05-13 4:02 ` Peter Delevoryas
0 siblings, 0 replies; 24+ messages in thread
From: Peter Delevoryas @ 2022-05-13 4:02 UTC (permalink / raw)
Cc: pdel, irischenlj, patrick, qemu-arm, qemu-devel, clg, zev,
openbmc, andrew, peter.maydell, joel
CC'ing Zev and OpenBMC since this was motivated by a problem Zev had there:
https://lore.kernel.org/openbmc/YnzGnWjkYdMUUNyM@hatter.bewilderbeest.net/
This series adds all the missing UART's in the Aspeed chips, and initializes
them all with serial devices (even if there is no peer character device provided
by the QEMU user).
This allows users to quickly test UART output without any code changes. In fact,
you could even connect all the UART's to separate sockets and check which one is
emitting data.
The first commit is just focusing on adding the missing hardware #define's.
The second commit has more info on the state of the whole Aspeed BMC UART cli
interface, and adds the additional initialization code.
By the way, could I put this code into aspeed_soc.h or something? If not,
maybe after this I'll add a file for common code, so that we can move
towards unifying everything.
Peter Delevoryas (2):
hw: aspeed: Add missing UART's
hw: aspeed: Init all UART's with serial devices
hw/arm/aspeed_ast10x0.c | 38 ++++++++++++++++++++++++++++++++++---
hw/arm/aspeed_ast2600.c | 29 +++++++++++++++++++++++++++-
hw/arm/aspeed_soc.c | 16 +++++++++++++++-
include/hw/arm/aspeed_soc.h | 8 ++++++++
4 files changed, 86 insertions(+), 5 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 0/2] hw: aspeed: Init all UART's with serial devices
@ 2022-05-13 4:02 ` Peter Delevoryas
0 siblings, 0 replies; 24+ messages in thread
From: Peter Delevoryas @ 2022-05-13 4:02 UTC (permalink / raw)
Cc: peter.maydell, zev, andrew, irischenlj, openbmc, qemu-devel,
qemu-arm, clg, pdel, joel
CC'ing Zev and OpenBMC since this was motivated by a problem Zev had there:
https://lore.kernel.org/openbmc/YnzGnWjkYdMUUNyM@hatter.bewilderbeest.net/
This series adds all the missing UART's in the Aspeed chips, and initializes
them all with serial devices (even if there is no peer character device provided
by the QEMU user).
This allows users to quickly test UART output without any code changes. In fact,
you could even connect all the UART's to separate sockets and check which one is
emitting data.
The first commit is just focusing on adding the missing hardware #define's.
The second commit has more info on the state of the whole Aspeed BMC UART cli
interface, and adds the additional initialization code.
By the way, could I put this code into aspeed_soc.h or something? If not,
maybe after this I'll add a file for common code, so that we can move
towards unifying everything.
Peter Delevoryas (2):
hw: aspeed: Add missing UART's
hw: aspeed: Init all UART's with serial devices
hw/arm/aspeed_ast10x0.c | 38 ++++++++++++++++++++++++++++++++++---
hw/arm/aspeed_ast2600.c | 29 +++++++++++++++++++++++++++-
hw/arm/aspeed_soc.c | 16 +++++++++++++++-
include/hw/arm/aspeed_soc.h | 8 ++++++++
4 files changed, 86 insertions(+), 5 deletions(-)
--
2.30.2
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/2] hw: aspeed: Add missing UART's
2022-05-13 4:02 ` Peter Delevoryas
@ 2022-05-13 4:02 ` Peter Delevoryas
-1 siblings, 0 replies; 24+ messages in thread
From: Peter Delevoryas @ 2022-05-13 4:02 UTC (permalink / raw)
Cc: pdel, irischenlj, patrick, qemu-arm, qemu-devel, clg, zev,
openbmc, andrew, peter.maydell, joel
This adds the missing UART memory and IRQ mappings for the AST2400, AST2500,
AST2600, and AST1030.
This also includes the new UART interfaces added in the AST2600 and AST1030
from UART6 to UART13. The addresses and interrupt numbers for these two
later chips are identical.
Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
hw/arm/aspeed_ast10x0.c | 24 ++++++++++++++++++++++++
hw/arm/aspeed_ast2600.c | 19 +++++++++++++++++++
hw/arm/aspeed_soc.c | 6 ++++++
include/hw/arm/aspeed_soc.h | 8 ++++++++
4 files changed, 57 insertions(+)
diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
index 4271549282..f65dc139da 100644
--- a/hw/arm/aspeed_ast10x0.c
+++ b/hw/arm/aspeed_ast10x0.c
@@ -33,14 +33,38 @@ static const hwaddr aspeed_soc_ast1030_memmap[] = {
[ASPEED_DEV_SBC] = 0x7E6F2000,
[ASPEED_DEV_GPIO] = 0x7E780000,
[ASPEED_DEV_TIMER1] = 0x7E782000,
+ [ASPEED_DEV_UART1] = 0x7E783000,
+ [ASPEED_DEV_UART2] = 0x7E78D000,
+ [ASPEED_DEV_UART3] = 0x7E78E000,
+ [ASPEED_DEV_UART4] = 0x7E78F000,
[ASPEED_DEV_UART5] = 0x7E784000,
+ [ASPEED_DEV_UART6] = 0x7E790000,
+ [ASPEED_DEV_UART7] = 0x7E790100,
+ [ASPEED_DEV_UART8] = 0x7E790200,
+ [ASPEED_DEV_UART9] = 0x7E790300,
+ [ASPEED_DEV_UART10] = 0x7E790400,
+ [ASPEED_DEV_UART11] = 0x7E790500,
+ [ASPEED_DEV_UART12] = 0x7E790600,
+ [ASPEED_DEV_UART13] = 0x7E790700,
[ASPEED_DEV_WDT] = 0x7E785000,
[ASPEED_DEV_LPC] = 0x7E789000,
[ASPEED_DEV_I2C] = 0x7E7B0000,
};
static const int aspeed_soc_ast1030_irqmap[] = {
+ [ASPEED_DEV_UART1] = 47,
+ [ASPEED_DEV_UART2] = 48,
+ [ASPEED_DEV_UART3] = 49,
+ [ASPEED_DEV_UART4] = 50,
[ASPEED_DEV_UART5] = 8,
+ [ASPEED_DEV_UART6] = 57,
+ [ASPEED_DEV_UART7] = 58,
+ [ASPEED_DEV_UART8] = 59,
+ [ASPEED_DEV_UART9] = 60,
+ [ASPEED_DEV_UART10] = 61,
+ [ASPEED_DEV_UART11] = 62,
+ [ASPEED_DEV_UART12] = 63,
+ [ASPEED_DEV_UART13] = 64,
[ASPEED_DEV_GPIO] = 11,
[ASPEED_DEV_TIMER1] = 16,
[ASPEED_DEV_TIMER2] = 17,
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index eedda7badc..1b72800682 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -61,7 +61,18 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
[ASPEED_DEV_IBT] = 0x1E789140,
[ASPEED_DEV_I2C] = 0x1E78A000,
[ASPEED_DEV_UART1] = 0x1E783000,
+ [ASPEED_DEV_UART2] = 0x1E78D000,
+ [ASPEED_DEV_UART3] = 0x1E78E000,
+ [ASPEED_DEV_UART4] = 0x1E78F000,
[ASPEED_DEV_UART5] = 0x1E784000,
+ [ASPEED_DEV_UART6] = 0x1E790000,
+ [ASPEED_DEV_UART7] = 0x1E790100,
+ [ASPEED_DEV_UART8] = 0x1E790200,
+ [ASPEED_DEV_UART9] = 0x1E790300,
+ [ASPEED_DEV_UART10] = 0x1E790400,
+ [ASPEED_DEV_UART11] = 0x1E790500,
+ [ASPEED_DEV_UART12] = 0x1E790600,
+ [ASPEED_DEV_UART13] = 0x1E790700,
[ASPEED_DEV_VUART] = 0x1E787000,
[ASPEED_DEV_I3C] = 0x1E7A0000,
[ASPEED_DEV_SDRAM] = 0x80000000,
@@ -78,6 +89,14 @@ static const int aspeed_soc_ast2600_irqmap[] = {
[ASPEED_DEV_UART3] = 49,
[ASPEED_DEV_UART4] = 50,
[ASPEED_DEV_UART5] = 8,
+ [ASPEED_DEV_UART6] = 57,
+ [ASPEED_DEV_UART7] = 58,
+ [ASPEED_DEV_UART8] = 59,
+ [ASPEED_DEV_UART9] = 60,
+ [ASPEED_DEV_UART10] = 61,
+ [ASPEED_DEV_UART11] = 62,
+ [ASPEED_DEV_UART12] = 63,
+ [ASPEED_DEV_UART13] = 64,
[ASPEED_DEV_VUART] = 8,
[ASPEED_DEV_FMC] = 39,
[ASPEED_DEV_SDMC] = 0,
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 58714cb2a0..2cd03d49da 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -48,6 +48,9 @@ static const hwaddr aspeed_soc_ast2400_memmap[] = {
[ASPEED_DEV_ETH1] = 0x1E660000,
[ASPEED_DEV_ETH2] = 0x1E680000,
[ASPEED_DEV_UART1] = 0x1E783000,
+ [ASPEED_DEV_UART2] = 0x1E78D000,
+ [ASPEED_DEV_UART3] = 0x1E78E000,
+ [ASPEED_DEV_UART4] = 0x1E78F000,
[ASPEED_DEV_UART5] = 0x1E784000,
[ASPEED_DEV_VUART] = 0x1E787000,
[ASPEED_DEV_SDRAM] = 0x40000000,
@@ -80,6 +83,9 @@ static const hwaddr aspeed_soc_ast2500_memmap[] = {
[ASPEED_DEV_ETH1] = 0x1E660000,
[ASPEED_DEV_ETH2] = 0x1E680000,
[ASPEED_DEV_UART1] = 0x1E783000,
+ [ASPEED_DEV_UART2] = 0x1E78D000,
+ [ASPEED_DEV_UART3] = 0x1E78E000,
+ [ASPEED_DEV_UART4] = 0x1E78F000,
[ASPEED_DEV_UART5] = 0x1E784000,
[ASPEED_DEV_VUART] = 0x1E787000,
[ASPEED_DEV_SDRAM] = 0x80000000,
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index e13af374b9..3f7f815275 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -104,6 +104,14 @@ enum {
ASPEED_DEV_UART3,
ASPEED_DEV_UART4,
ASPEED_DEV_UART5,
+ ASPEED_DEV_UART6,
+ ASPEED_DEV_UART7,
+ ASPEED_DEV_UART8,
+ ASPEED_DEV_UART9,
+ ASPEED_DEV_UART10,
+ ASPEED_DEV_UART11,
+ ASPEED_DEV_UART12,
+ ASPEED_DEV_UART13,
ASPEED_DEV_VUART,
ASPEED_DEV_FMC,
ASPEED_DEV_SPI1,
--
2.30.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 1/2] hw: aspeed: Add missing UART's
@ 2022-05-13 4:02 ` Peter Delevoryas
0 siblings, 0 replies; 24+ messages in thread
From: Peter Delevoryas @ 2022-05-13 4:02 UTC (permalink / raw)
Cc: peter.maydell, zev, andrew, irischenlj, openbmc, qemu-devel,
qemu-arm, clg, pdel, joel
This adds the missing UART memory and IRQ mappings for the AST2400, AST2500,
AST2600, and AST1030.
This also includes the new UART interfaces added in the AST2600 and AST1030
from UART6 to UART13. The addresses and interrupt numbers for these two
later chips are identical.
Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
hw/arm/aspeed_ast10x0.c | 24 ++++++++++++++++++++++++
hw/arm/aspeed_ast2600.c | 19 +++++++++++++++++++
hw/arm/aspeed_soc.c | 6 ++++++
include/hw/arm/aspeed_soc.h | 8 ++++++++
4 files changed, 57 insertions(+)
diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
index 4271549282..f65dc139da 100644
--- a/hw/arm/aspeed_ast10x0.c
+++ b/hw/arm/aspeed_ast10x0.c
@@ -33,14 +33,38 @@ static const hwaddr aspeed_soc_ast1030_memmap[] = {
[ASPEED_DEV_SBC] = 0x7E6F2000,
[ASPEED_DEV_GPIO] = 0x7E780000,
[ASPEED_DEV_TIMER1] = 0x7E782000,
+ [ASPEED_DEV_UART1] = 0x7E783000,
+ [ASPEED_DEV_UART2] = 0x7E78D000,
+ [ASPEED_DEV_UART3] = 0x7E78E000,
+ [ASPEED_DEV_UART4] = 0x7E78F000,
[ASPEED_DEV_UART5] = 0x7E784000,
+ [ASPEED_DEV_UART6] = 0x7E790000,
+ [ASPEED_DEV_UART7] = 0x7E790100,
+ [ASPEED_DEV_UART8] = 0x7E790200,
+ [ASPEED_DEV_UART9] = 0x7E790300,
+ [ASPEED_DEV_UART10] = 0x7E790400,
+ [ASPEED_DEV_UART11] = 0x7E790500,
+ [ASPEED_DEV_UART12] = 0x7E790600,
+ [ASPEED_DEV_UART13] = 0x7E790700,
[ASPEED_DEV_WDT] = 0x7E785000,
[ASPEED_DEV_LPC] = 0x7E789000,
[ASPEED_DEV_I2C] = 0x7E7B0000,
};
static const int aspeed_soc_ast1030_irqmap[] = {
+ [ASPEED_DEV_UART1] = 47,
+ [ASPEED_DEV_UART2] = 48,
+ [ASPEED_DEV_UART3] = 49,
+ [ASPEED_DEV_UART4] = 50,
[ASPEED_DEV_UART5] = 8,
+ [ASPEED_DEV_UART6] = 57,
+ [ASPEED_DEV_UART7] = 58,
+ [ASPEED_DEV_UART8] = 59,
+ [ASPEED_DEV_UART9] = 60,
+ [ASPEED_DEV_UART10] = 61,
+ [ASPEED_DEV_UART11] = 62,
+ [ASPEED_DEV_UART12] = 63,
+ [ASPEED_DEV_UART13] = 64,
[ASPEED_DEV_GPIO] = 11,
[ASPEED_DEV_TIMER1] = 16,
[ASPEED_DEV_TIMER2] = 17,
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index eedda7badc..1b72800682 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -61,7 +61,18 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
[ASPEED_DEV_IBT] = 0x1E789140,
[ASPEED_DEV_I2C] = 0x1E78A000,
[ASPEED_DEV_UART1] = 0x1E783000,
+ [ASPEED_DEV_UART2] = 0x1E78D000,
+ [ASPEED_DEV_UART3] = 0x1E78E000,
+ [ASPEED_DEV_UART4] = 0x1E78F000,
[ASPEED_DEV_UART5] = 0x1E784000,
+ [ASPEED_DEV_UART6] = 0x1E790000,
+ [ASPEED_DEV_UART7] = 0x1E790100,
+ [ASPEED_DEV_UART8] = 0x1E790200,
+ [ASPEED_DEV_UART9] = 0x1E790300,
+ [ASPEED_DEV_UART10] = 0x1E790400,
+ [ASPEED_DEV_UART11] = 0x1E790500,
+ [ASPEED_DEV_UART12] = 0x1E790600,
+ [ASPEED_DEV_UART13] = 0x1E790700,
[ASPEED_DEV_VUART] = 0x1E787000,
[ASPEED_DEV_I3C] = 0x1E7A0000,
[ASPEED_DEV_SDRAM] = 0x80000000,
@@ -78,6 +89,14 @@ static const int aspeed_soc_ast2600_irqmap[] = {
[ASPEED_DEV_UART3] = 49,
[ASPEED_DEV_UART4] = 50,
[ASPEED_DEV_UART5] = 8,
+ [ASPEED_DEV_UART6] = 57,
+ [ASPEED_DEV_UART7] = 58,
+ [ASPEED_DEV_UART8] = 59,
+ [ASPEED_DEV_UART9] = 60,
+ [ASPEED_DEV_UART10] = 61,
+ [ASPEED_DEV_UART11] = 62,
+ [ASPEED_DEV_UART12] = 63,
+ [ASPEED_DEV_UART13] = 64,
[ASPEED_DEV_VUART] = 8,
[ASPEED_DEV_FMC] = 39,
[ASPEED_DEV_SDMC] = 0,
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 58714cb2a0..2cd03d49da 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -48,6 +48,9 @@ static const hwaddr aspeed_soc_ast2400_memmap[] = {
[ASPEED_DEV_ETH1] = 0x1E660000,
[ASPEED_DEV_ETH2] = 0x1E680000,
[ASPEED_DEV_UART1] = 0x1E783000,
+ [ASPEED_DEV_UART2] = 0x1E78D000,
+ [ASPEED_DEV_UART3] = 0x1E78E000,
+ [ASPEED_DEV_UART4] = 0x1E78F000,
[ASPEED_DEV_UART5] = 0x1E784000,
[ASPEED_DEV_VUART] = 0x1E787000,
[ASPEED_DEV_SDRAM] = 0x40000000,
@@ -80,6 +83,9 @@ static const hwaddr aspeed_soc_ast2500_memmap[] = {
[ASPEED_DEV_ETH1] = 0x1E660000,
[ASPEED_DEV_ETH2] = 0x1E680000,
[ASPEED_DEV_UART1] = 0x1E783000,
+ [ASPEED_DEV_UART2] = 0x1E78D000,
+ [ASPEED_DEV_UART3] = 0x1E78E000,
+ [ASPEED_DEV_UART4] = 0x1E78F000,
[ASPEED_DEV_UART5] = 0x1E784000,
[ASPEED_DEV_VUART] = 0x1E787000,
[ASPEED_DEV_SDRAM] = 0x80000000,
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index e13af374b9..3f7f815275 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -104,6 +104,14 @@ enum {
ASPEED_DEV_UART3,
ASPEED_DEV_UART4,
ASPEED_DEV_UART5,
+ ASPEED_DEV_UART6,
+ ASPEED_DEV_UART7,
+ ASPEED_DEV_UART8,
+ ASPEED_DEV_UART9,
+ ASPEED_DEV_UART10,
+ ASPEED_DEV_UART11,
+ ASPEED_DEV_UART12,
+ ASPEED_DEV_UART13,
ASPEED_DEV_VUART,
ASPEED_DEV_FMC,
ASPEED_DEV_SPI1,
--
2.30.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 2/2] hw: aspeed: Init all UART's with serial devices
2022-05-13 4:02 ` Peter Delevoryas
@ 2022-05-13 4:02 ` Peter Delevoryas
-1 siblings, 0 replies; 24+ messages in thread
From: Peter Delevoryas @ 2022-05-13 4:02 UTC (permalink / raw)
Cc: pdel, irischenlj, patrick, qemu-arm, qemu-devel, clg, zev,
openbmc, andrew, peter.maydell, joel
Usually, QEMU users just provide one serial device on the command line,
either through "-nographic" or "-serial stdio -display none", or just using
VNC and popping up a window. We try to match what the user expects, which is
to connect the first (and usually only) serial device to the UART a board is
using as serial0.
Most Aspeed machines in hw/arm/aspeed.c use UART5 for serial0 in their
device tree, so we connect UART5 to the first serial device. Some machines
use UART1 though, or UART3, so the uart_default property lets us specify
that in a board definition.
In order to specify a nonstandard serial0 UART, a user basically *must* add
a new board definition in hw/arm/aspeed.c. There's no way to do this without
recompiling QEMU, besides constructing the machine completely from scratch
on the command line.
To provide more flexibility, we can also support the user specifying more
serial devices, and connect them to the UART memory regions if possible.
Even if a user doesn't specify any extra serial devices, it's useful to
initialize these memory regions as UART's, so that they respond to the guest
OS more naturally. At the moment, they will just always return zero's for
everything, and some UART registers have a default non-zero state.
With this change, if a new OpenBMC image uses UART3 or some other
nonstandard UART for serial0, you can still use it with the EVB without
recompiling QEMU, even though uart-default=UART5 for the EVB.
For example, Facebook's Wedge100 BMC uses UART3: you can fetch an image from
Github[1] and get the serial console output even while running the palmetto
machine type, because we explicitly specify that we want UART3 to be
connected to stdio.
qemu-system-arm -machine palmetto-bmc \
-drive file=wedge100.mtd,format=raw,if=mtd \
-serial null -serial null -serial null -serial stdio -display none
Similarly, you can boot a Fuji BMC image[2], which uses UART1, using the
AST2600 EVB machine:
qemu-system-arm -machine ast2600-evb \
-drive file=fuji.mtd,format=raw,if=mtd \
-serial null -serial stdio -display none
This is kind of complicated, of course: it might be more natural to get rid
of the uart_default attribute completely, and initialize UART's
sequentially. But, keeping backward compatibility and the way most users
know how to use QEMU in mind, this seems to make the most sense.
[1] https://github.com/facebook/openbmc/releases/download/v2021.49.0/wedge100.mtd
[2] https://github.com/facebook/openbmc/releases/download/v2021.49.0/fuji.mtd
Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
hw/arm/aspeed_ast10x0.c | 14 +++++++++++---
hw/arm/aspeed_ast2600.c | 10 +++++++++-
hw/arm/aspeed_soc.c | 10 +++++++++-
3 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
index f65dc139da..5e6f3a8fed 100644
--- a/hw/arm/aspeed_ast10x0.c
+++ b/hw/arm/aspeed_ast10x0.c
@@ -215,10 +215,18 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
qdev_get_gpio_in(DEVICE(&s->armv7m),
sc->irqmap[ASPEED_DEV_KCS] + aspeed_lpc_kcs_4));
- /* UART5 - attach an 8250 to the IO space as our UART */
- serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
- aspeed_soc_get_irq(s, ASPEED_DEV_UART5),
+ /* UART - attach 8250's to the IO space for each UART */
+ serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
+ aspeed_soc_get_irq(s, s->uart_default),
38400, serial_hd(0), DEVICE_LITTLE_ENDIAN);
+ for (int i = 1, uart = ASPEED_DEV_UART1; i < 13; i++, uart++) {
+ if (uart == s->uart_default) {
+ uart++;
+ }
+ serial_mm_init(get_system_memory(), sc->memmap[uart], 2,
+ aspeed_soc_get_irq(s, uart), 38400, serial_hd(i),
+ DEVICE_LITTLE_ENDIAN);
+ }
/* Timer */
object_property_set_link(OBJECT(&s->timerctrl), "scu", OBJECT(&s->scu),
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 1b72800682..cbeca7f655 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -372,10 +372,18 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
sysbus_connect_irq(SYS_BUS_DEVICE(&s->adc), 0,
aspeed_soc_get_irq(s, ASPEED_DEV_ADC));
- /* UART - attach an 8250 to the IO space as our UART */
+ /* UART - attach 8250's to the IO space for each UART */
serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
aspeed_soc_get_irq(s, s->uart_default), 38400,
serial_hd(0), DEVICE_LITTLE_ENDIAN);
+ for (int i = 1, uart = ASPEED_DEV_UART1; i < 13; i++, uart++) {
+ if (uart == s->uart_default) {
+ uart++;
+ }
+ serial_mm_init(get_system_memory(), sc->memmap[uart], 2,
+ aspeed_soc_get_irq(s, uart), 38400, serial_hd(i),
+ DEVICE_LITTLE_ENDIAN);
+ }
/* I2C */
object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 2cd03d49da..1fc1ed808d 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -303,10 +303,18 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
sysbus_connect_irq(SYS_BUS_DEVICE(&s->adc), 0,
aspeed_soc_get_irq(s, ASPEED_DEV_ADC));
- /* UART - attach an 8250 to the IO space as our UART */
+ /* UART - attach 8250's to the IO space for each UART */
serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
aspeed_soc_get_irq(s, s->uart_default), 38400,
serial_hd(0), DEVICE_LITTLE_ENDIAN);
+ for (int i = 1, uart = ASPEED_DEV_UART1; i < 5; i++, uart++) {
+ if (uart == s->uart_default) {
+ uart++;
+ }
+ serial_mm_init(get_system_memory(), sc->memmap[uart], 2,
+ aspeed_soc_get_irq(s, uart), 38400, serial_hd(i),
+ DEVICE_LITTLE_ENDIAN);
+ }
/* I2C */
object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
--
2.30.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* [PATCH 2/2] hw: aspeed: Init all UART's with serial devices
@ 2022-05-13 4:02 ` Peter Delevoryas
0 siblings, 0 replies; 24+ messages in thread
From: Peter Delevoryas @ 2022-05-13 4:02 UTC (permalink / raw)
Cc: peter.maydell, zev, andrew, irischenlj, openbmc, qemu-devel,
qemu-arm, clg, pdel, joel
Usually, QEMU users just provide one serial device on the command line,
either through "-nographic" or "-serial stdio -display none", or just using
VNC and popping up a window. We try to match what the user expects, which is
to connect the first (and usually only) serial device to the UART a board is
using as serial0.
Most Aspeed machines in hw/arm/aspeed.c use UART5 for serial0 in their
device tree, so we connect UART5 to the first serial device. Some machines
use UART1 though, or UART3, so the uart_default property lets us specify
that in a board definition.
In order to specify a nonstandard serial0 UART, a user basically *must* add
a new board definition in hw/arm/aspeed.c. There's no way to do this without
recompiling QEMU, besides constructing the machine completely from scratch
on the command line.
To provide more flexibility, we can also support the user specifying more
serial devices, and connect them to the UART memory regions if possible.
Even if a user doesn't specify any extra serial devices, it's useful to
initialize these memory regions as UART's, so that they respond to the guest
OS more naturally. At the moment, they will just always return zero's for
everything, and some UART registers have a default non-zero state.
With this change, if a new OpenBMC image uses UART3 or some other
nonstandard UART for serial0, you can still use it with the EVB without
recompiling QEMU, even though uart-default=UART5 for the EVB.
For example, Facebook's Wedge100 BMC uses UART3: you can fetch an image from
Github[1] and get the serial console output even while running the palmetto
machine type, because we explicitly specify that we want UART3 to be
connected to stdio.
qemu-system-arm -machine palmetto-bmc \
-drive file=wedge100.mtd,format=raw,if=mtd \
-serial null -serial null -serial null -serial stdio -display none
Similarly, you can boot a Fuji BMC image[2], which uses UART1, using the
AST2600 EVB machine:
qemu-system-arm -machine ast2600-evb \
-drive file=fuji.mtd,format=raw,if=mtd \
-serial null -serial stdio -display none
This is kind of complicated, of course: it might be more natural to get rid
of the uart_default attribute completely, and initialize UART's
sequentially. But, keeping backward compatibility and the way most users
know how to use QEMU in mind, this seems to make the most sense.
[1] https://github.com/facebook/openbmc/releases/download/v2021.49.0/wedge100.mtd
[2] https://github.com/facebook/openbmc/releases/download/v2021.49.0/fuji.mtd
Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
hw/arm/aspeed_ast10x0.c | 14 +++++++++++---
hw/arm/aspeed_ast2600.c | 10 +++++++++-
hw/arm/aspeed_soc.c | 10 +++++++++-
3 files changed, 29 insertions(+), 5 deletions(-)
diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
index f65dc139da..5e6f3a8fed 100644
--- a/hw/arm/aspeed_ast10x0.c
+++ b/hw/arm/aspeed_ast10x0.c
@@ -215,10 +215,18 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
qdev_get_gpio_in(DEVICE(&s->armv7m),
sc->irqmap[ASPEED_DEV_KCS] + aspeed_lpc_kcs_4));
- /* UART5 - attach an 8250 to the IO space as our UART */
- serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
- aspeed_soc_get_irq(s, ASPEED_DEV_UART5),
+ /* UART - attach 8250's to the IO space for each UART */
+ serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
+ aspeed_soc_get_irq(s, s->uart_default),
38400, serial_hd(0), DEVICE_LITTLE_ENDIAN);
+ for (int i = 1, uart = ASPEED_DEV_UART1; i < 13; i++, uart++) {
+ if (uart == s->uart_default) {
+ uart++;
+ }
+ serial_mm_init(get_system_memory(), sc->memmap[uart], 2,
+ aspeed_soc_get_irq(s, uart), 38400, serial_hd(i),
+ DEVICE_LITTLE_ENDIAN);
+ }
/* Timer */
object_property_set_link(OBJECT(&s->timerctrl), "scu", OBJECT(&s->scu),
diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index 1b72800682..cbeca7f655 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -372,10 +372,18 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
sysbus_connect_irq(SYS_BUS_DEVICE(&s->adc), 0,
aspeed_soc_get_irq(s, ASPEED_DEV_ADC));
- /* UART - attach an 8250 to the IO space as our UART */
+ /* UART - attach 8250's to the IO space for each UART */
serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
aspeed_soc_get_irq(s, s->uart_default), 38400,
serial_hd(0), DEVICE_LITTLE_ENDIAN);
+ for (int i = 1, uart = ASPEED_DEV_UART1; i < 13; i++, uart++) {
+ if (uart == s->uart_default) {
+ uart++;
+ }
+ serial_mm_init(get_system_memory(), sc->memmap[uart], 2,
+ aspeed_soc_get_irq(s, uart), 38400, serial_hd(i),
+ DEVICE_LITTLE_ENDIAN);
+ }
/* I2C */
object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 2cd03d49da..1fc1ed808d 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -303,10 +303,18 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
sysbus_connect_irq(SYS_BUS_DEVICE(&s->adc), 0,
aspeed_soc_get_irq(s, ASPEED_DEV_ADC));
- /* UART - attach an 8250 to the IO space as our UART */
+ /* UART - attach 8250's to the IO space for each UART */
serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
aspeed_soc_get_irq(s, s->uart_default), 38400,
serial_hd(0), DEVICE_LITTLE_ENDIAN);
+ for (int i = 1, uart = ASPEED_DEV_UART1; i < 5; i++, uart++) {
+ if (uart == s->uart_default) {
+ uart++;
+ }
+ serial_mm_init(get_system_memory(), sc->memmap[uart], 2,
+ aspeed_soc_get_irq(s, uart), 38400, serial_hd(i),
+ DEVICE_LITTLE_ENDIAN);
+ }
/* I2C */
object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
--
2.30.2
^ permalink raw reply related [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] hw: aspeed: Add missing UART's
2022-05-13 4:02 ` Peter Delevoryas
@ 2022-05-13 5:21 ` Cédric Le Goater
-1 siblings, 0 replies; 24+ messages in thread
From: Cédric Le Goater @ 2022-05-13 5:21 UTC (permalink / raw)
To: Peter Delevoryas
Cc: irischenlj, patrick, qemu-arm, qemu-devel, zev, openbmc, andrew,
peter.maydell, joel
On 5/13/22 06:02, Peter Delevoryas wrote:
> This adds the missing UART memory and IRQ mappings for the AST2400, AST2500,
> AST2600, and AST1030.
>
> This also includes the new UART interfaces added in the AST2600 and AST1030
> from UART6 to UART13. The addresses and interrupt numbers for these two
> later chips are identical.
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
>
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> ---
> hw/arm/aspeed_ast10x0.c | 24 ++++++++++++++++++++++++
> hw/arm/aspeed_ast2600.c | 19 +++++++++++++++++++
> hw/arm/aspeed_soc.c | 6 ++++++
> include/hw/arm/aspeed_soc.h | 8 ++++++++
> 4 files changed, 57 insertions(+)
>
> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
> index 4271549282..f65dc139da 100644
> --- a/hw/arm/aspeed_ast10x0.c
> +++ b/hw/arm/aspeed_ast10x0.c
> @@ -33,14 +33,38 @@ static const hwaddr aspeed_soc_ast1030_memmap[] = {
> [ASPEED_DEV_SBC] = 0x7E6F2000,
> [ASPEED_DEV_GPIO] = 0x7E780000,
> [ASPEED_DEV_TIMER1] = 0x7E782000,
> + [ASPEED_DEV_UART1] = 0x7E783000,
> + [ASPEED_DEV_UART2] = 0x7E78D000,
> + [ASPEED_DEV_UART3] = 0x7E78E000,
> + [ASPEED_DEV_UART4] = 0x7E78F000,
> [ASPEED_DEV_UART5] = 0x7E784000,
> + [ASPEED_DEV_UART6] = 0x7E790000,
> + [ASPEED_DEV_UART7] = 0x7E790100,
> + [ASPEED_DEV_UART8] = 0x7E790200,
> + [ASPEED_DEV_UART9] = 0x7E790300,
> + [ASPEED_DEV_UART10] = 0x7E790400,
> + [ASPEED_DEV_UART11] = 0x7E790500,
> + [ASPEED_DEV_UART12] = 0x7E790600,
> + [ASPEED_DEV_UART13] = 0x7E790700,
> [ASPEED_DEV_WDT] = 0x7E785000,
> [ASPEED_DEV_LPC] = 0x7E789000,
> [ASPEED_DEV_I2C] = 0x7E7B0000,
> };
>
> static const int aspeed_soc_ast1030_irqmap[] = {
> + [ASPEED_DEV_UART1] = 47,
> + [ASPEED_DEV_UART2] = 48,
> + [ASPEED_DEV_UART3] = 49,
> + [ASPEED_DEV_UART4] = 50,
> [ASPEED_DEV_UART5] = 8,
> + [ASPEED_DEV_UART6] = 57,
> + [ASPEED_DEV_UART7] = 58,
> + [ASPEED_DEV_UART8] = 59,
> + [ASPEED_DEV_UART9] = 60,
> + [ASPEED_DEV_UART10] = 61,
> + [ASPEED_DEV_UART11] = 62,
> + [ASPEED_DEV_UART12] = 63,
> + [ASPEED_DEV_UART13] = 64,
> [ASPEED_DEV_GPIO] = 11,
> [ASPEED_DEV_TIMER1] = 16,
> [ASPEED_DEV_TIMER2] = 17,
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index eedda7badc..1b72800682 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -61,7 +61,18 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
> [ASPEED_DEV_IBT] = 0x1E789140,
> [ASPEED_DEV_I2C] = 0x1E78A000,
> [ASPEED_DEV_UART1] = 0x1E783000,
> + [ASPEED_DEV_UART2] = 0x1E78D000,
> + [ASPEED_DEV_UART3] = 0x1E78E000,
> + [ASPEED_DEV_UART4] = 0x1E78F000,
> [ASPEED_DEV_UART5] = 0x1E784000,
> + [ASPEED_DEV_UART6] = 0x1E790000,
> + [ASPEED_DEV_UART7] = 0x1E790100,
> + [ASPEED_DEV_UART8] = 0x1E790200,
> + [ASPEED_DEV_UART9] = 0x1E790300,
> + [ASPEED_DEV_UART10] = 0x1E790400,
> + [ASPEED_DEV_UART11] = 0x1E790500,
> + [ASPEED_DEV_UART12] = 0x1E790600,
> + [ASPEED_DEV_UART13] = 0x1E790700,
> [ASPEED_DEV_VUART] = 0x1E787000,
> [ASPEED_DEV_I3C] = 0x1E7A0000,
> [ASPEED_DEV_SDRAM] = 0x80000000,
> @@ -78,6 +89,14 @@ static const int aspeed_soc_ast2600_irqmap[] = {
> [ASPEED_DEV_UART3] = 49,
> [ASPEED_DEV_UART4] = 50,
> [ASPEED_DEV_UART5] = 8,
> + [ASPEED_DEV_UART6] = 57,
> + [ASPEED_DEV_UART7] = 58,
> + [ASPEED_DEV_UART8] = 59,
> + [ASPEED_DEV_UART9] = 60,
> + [ASPEED_DEV_UART10] = 61,
> + [ASPEED_DEV_UART11] = 62,
> + [ASPEED_DEV_UART12] = 63,
> + [ASPEED_DEV_UART13] = 64,
> [ASPEED_DEV_VUART] = 8,
> [ASPEED_DEV_FMC] = 39,
> [ASPEED_DEV_SDMC] = 0,
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index 58714cb2a0..2cd03d49da 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -48,6 +48,9 @@ static const hwaddr aspeed_soc_ast2400_memmap[] = {
> [ASPEED_DEV_ETH1] = 0x1E660000,
> [ASPEED_DEV_ETH2] = 0x1E680000,
> [ASPEED_DEV_UART1] = 0x1E783000,
> + [ASPEED_DEV_UART2] = 0x1E78D000,
> + [ASPEED_DEV_UART3] = 0x1E78E000,
> + [ASPEED_DEV_UART4] = 0x1E78F000,
> [ASPEED_DEV_UART5] = 0x1E784000,
> [ASPEED_DEV_VUART] = 0x1E787000,
> [ASPEED_DEV_SDRAM] = 0x40000000,
> @@ -80,6 +83,9 @@ static const hwaddr aspeed_soc_ast2500_memmap[] = {
> [ASPEED_DEV_ETH1] = 0x1E660000,
> [ASPEED_DEV_ETH2] = 0x1E680000,
> [ASPEED_DEV_UART1] = 0x1E783000,
> + [ASPEED_DEV_UART2] = 0x1E78D000,
> + [ASPEED_DEV_UART3] = 0x1E78E000,
> + [ASPEED_DEV_UART4] = 0x1E78F000,
> [ASPEED_DEV_UART5] = 0x1E784000,
> [ASPEED_DEV_VUART] = 0x1E787000,
> [ASPEED_DEV_SDRAM] = 0x80000000,
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index e13af374b9..3f7f815275 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -104,6 +104,14 @@ enum {
> ASPEED_DEV_UART3,
> ASPEED_DEV_UART4,
> ASPEED_DEV_UART5,
> + ASPEED_DEV_UART6,
> + ASPEED_DEV_UART7,
> + ASPEED_DEV_UART8,
> + ASPEED_DEV_UART9,
> + ASPEED_DEV_UART10,
> + ASPEED_DEV_UART11,
> + ASPEED_DEV_UART12,
> + ASPEED_DEV_UART13,
> ASPEED_DEV_VUART,
> ASPEED_DEV_FMC,
> ASPEED_DEV_SPI1,
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 1/2] hw: aspeed: Add missing UART's
@ 2022-05-13 5:21 ` Cédric Le Goater
0 siblings, 0 replies; 24+ messages in thread
From: Cédric Le Goater @ 2022-05-13 5:21 UTC (permalink / raw)
To: Peter Delevoryas
Cc: peter.maydell, zev, andrew, irischenlj, openbmc, qemu-devel,
qemu-arm, joel
On 5/13/22 06:02, Peter Delevoryas wrote:
> This adds the missing UART memory and IRQ mappings for the AST2400, AST2500,
> AST2600, and AST1030.
>
> This also includes the new UART interfaces added in the AST2600 and AST1030
> from UART6 to UART13. The addresses and interrupt numbers for these two
> later chips are identical.
Reviewed-by: Cédric Le Goater <clg@kaod.org>
Thanks,
C.
>
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> ---
> hw/arm/aspeed_ast10x0.c | 24 ++++++++++++++++++++++++
> hw/arm/aspeed_ast2600.c | 19 +++++++++++++++++++
> hw/arm/aspeed_soc.c | 6 ++++++
> include/hw/arm/aspeed_soc.h | 8 ++++++++
> 4 files changed, 57 insertions(+)
>
> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
> index 4271549282..f65dc139da 100644
> --- a/hw/arm/aspeed_ast10x0.c
> +++ b/hw/arm/aspeed_ast10x0.c
> @@ -33,14 +33,38 @@ static const hwaddr aspeed_soc_ast1030_memmap[] = {
> [ASPEED_DEV_SBC] = 0x7E6F2000,
> [ASPEED_DEV_GPIO] = 0x7E780000,
> [ASPEED_DEV_TIMER1] = 0x7E782000,
> + [ASPEED_DEV_UART1] = 0x7E783000,
> + [ASPEED_DEV_UART2] = 0x7E78D000,
> + [ASPEED_DEV_UART3] = 0x7E78E000,
> + [ASPEED_DEV_UART4] = 0x7E78F000,
> [ASPEED_DEV_UART5] = 0x7E784000,
> + [ASPEED_DEV_UART6] = 0x7E790000,
> + [ASPEED_DEV_UART7] = 0x7E790100,
> + [ASPEED_DEV_UART8] = 0x7E790200,
> + [ASPEED_DEV_UART9] = 0x7E790300,
> + [ASPEED_DEV_UART10] = 0x7E790400,
> + [ASPEED_DEV_UART11] = 0x7E790500,
> + [ASPEED_DEV_UART12] = 0x7E790600,
> + [ASPEED_DEV_UART13] = 0x7E790700,
> [ASPEED_DEV_WDT] = 0x7E785000,
> [ASPEED_DEV_LPC] = 0x7E789000,
> [ASPEED_DEV_I2C] = 0x7E7B0000,
> };
>
> static const int aspeed_soc_ast1030_irqmap[] = {
> + [ASPEED_DEV_UART1] = 47,
> + [ASPEED_DEV_UART2] = 48,
> + [ASPEED_DEV_UART3] = 49,
> + [ASPEED_DEV_UART4] = 50,
> [ASPEED_DEV_UART5] = 8,
> + [ASPEED_DEV_UART6] = 57,
> + [ASPEED_DEV_UART7] = 58,
> + [ASPEED_DEV_UART8] = 59,
> + [ASPEED_DEV_UART9] = 60,
> + [ASPEED_DEV_UART10] = 61,
> + [ASPEED_DEV_UART11] = 62,
> + [ASPEED_DEV_UART12] = 63,
> + [ASPEED_DEV_UART13] = 64,
> [ASPEED_DEV_GPIO] = 11,
> [ASPEED_DEV_TIMER1] = 16,
> [ASPEED_DEV_TIMER2] = 17,
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index eedda7badc..1b72800682 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -61,7 +61,18 @@ static const hwaddr aspeed_soc_ast2600_memmap[] = {
> [ASPEED_DEV_IBT] = 0x1E789140,
> [ASPEED_DEV_I2C] = 0x1E78A000,
> [ASPEED_DEV_UART1] = 0x1E783000,
> + [ASPEED_DEV_UART2] = 0x1E78D000,
> + [ASPEED_DEV_UART3] = 0x1E78E000,
> + [ASPEED_DEV_UART4] = 0x1E78F000,
> [ASPEED_DEV_UART5] = 0x1E784000,
> + [ASPEED_DEV_UART6] = 0x1E790000,
> + [ASPEED_DEV_UART7] = 0x1E790100,
> + [ASPEED_DEV_UART8] = 0x1E790200,
> + [ASPEED_DEV_UART9] = 0x1E790300,
> + [ASPEED_DEV_UART10] = 0x1E790400,
> + [ASPEED_DEV_UART11] = 0x1E790500,
> + [ASPEED_DEV_UART12] = 0x1E790600,
> + [ASPEED_DEV_UART13] = 0x1E790700,
> [ASPEED_DEV_VUART] = 0x1E787000,
> [ASPEED_DEV_I3C] = 0x1E7A0000,
> [ASPEED_DEV_SDRAM] = 0x80000000,
> @@ -78,6 +89,14 @@ static const int aspeed_soc_ast2600_irqmap[] = {
> [ASPEED_DEV_UART3] = 49,
> [ASPEED_DEV_UART4] = 50,
> [ASPEED_DEV_UART5] = 8,
> + [ASPEED_DEV_UART6] = 57,
> + [ASPEED_DEV_UART7] = 58,
> + [ASPEED_DEV_UART8] = 59,
> + [ASPEED_DEV_UART9] = 60,
> + [ASPEED_DEV_UART10] = 61,
> + [ASPEED_DEV_UART11] = 62,
> + [ASPEED_DEV_UART12] = 63,
> + [ASPEED_DEV_UART13] = 64,
> [ASPEED_DEV_VUART] = 8,
> [ASPEED_DEV_FMC] = 39,
> [ASPEED_DEV_SDMC] = 0,
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index 58714cb2a0..2cd03d49da 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -48,6 +48,9 @@ static const hwaddr aspeed_soc_ast2400_memmap[] = {
> [ASPEED_DEV_ETH1] = 0x1E660000,
> [ASPEED_DEV_ETH2] = 0x1E680000,
> [ASPEED_DEV_UART1] = 0x1E783000,
> + [ASPEED_DEV_UART2] = 0x1E78D000,
> + [ASPEED_DEV_UART3] = 0x1E78E000,
> + [ASPEED_DEV_UART4] = 0x1E78F000,
> [ASPEED_DEV_UART5] = 0x1E784000,
> [ASPEED_DEV_VUART] = 0x1E787000,
> [ASPEED_DEV_SDRAM] = 0x40000000,
> @@ -80,6 +83,9 @@ static const hwaddr aspeed_soc_ast2500_memmap[] = {
> [ASPEED_DEV_ETH1] = 0x1E660000,
> [ASPEED_DEV_ETH2] = 0x1E680000,
> [ASPEED_DEV_UART1] = 0x1E783000,
> + [ASPEED_DEV_UART2] = 0x1E78D000,
> + [ASPEED_DEV_UART3] = 0x1E78E000,
> + [ASPEED_DEV_UART4] = 0x1E78F000,
> [ASPEED_DEV_UART5] = 0x1E784000,
> [ASPEED_DEV_VUART] = 0x1E787000,
> [ASPEED_DEV_SDRAM] = 0x80000000,
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index e13af374b9..3f7f815275 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -104,6 +104,14 @@ enum {
> ASPEED_DEV_UART3,
> ASPEED_DEV_UART4,
> ASPEED_DEV_UART5,
> + ASPEED_DEV_UART6,
> + ASPEED_DEV_UART7,
> + ASPEED_DEV_UART8,
> + ASPEED_DEV_UART9,
> + ASPEED_DEV_UART10,
> + ASPEED_DEV_UART11,
> + ASPEED_DEV_UART12,
> + ASPEED_DEV_UART13,
> ASPEED_DEV_VUART,
> ASPEED_DEV_FMC,
> ASPEED_DEV_SPI1,
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] hw: aspeed: Init all UART's with serial devices
2022-05-13 4:02 ` Peter Delevoryas
@ 2022-05-13 5:31 ` Cédric Le Goater
-1 siblings, 0 replies; 24+ messages in thread
From: Cédric Le Goater @ 2022-05-13 5:31 UTC (permalink / raw)
To: Peter Delevoryas
Cc: peter.maydell, zev, andrew, irischenlj, openbmc, qemu-devel,
qemu-arm, joel
On 5/13/22 06:02, Peter Delevoryas wrote:
> Usually, QEMU users just provide one serial device on the command line,
> either through "-nographic" or "-serial stdio -display none", or just using
> VNC and popping up a window. We try to match what the user expects, which is
> to connect the first (and usually only) serial device to the UART a board is
> using as serial0.
>
> Most Aspeed machines in hw/arm/aspeed.c use UART5 for serial0 in their
> device tree, so we connect UART5 to the first serial device. Some machines
> use UART1 though, or UART3, so the uart_default property lets us specify
> that in a board definition.
>
> In order to specify a nonstandard serial0 UART, a user basically *must* add
> a new board definition in hw/arm/aspeed.c. There's no way to do this without
> recompiling QEMU, besides constructing the machine completely from scratch
> on the command line.
>
> To provide more flexibility, we can also support the user specifying more
> serial devices, and connect them to the UART memory regions if possible.
> Even if a user doesn't specify any extra serial devices, it's useful to
> initialize these memory regions as UART's, so that they respond to the guest
> OS more naturally. At the moment, they will just always return zero's for
> everything, and some UART registers have a default non-zero state.
>
> With this change, if a new OpenBMC image uses UART3 or some other
> nonstandard UART for serial0, you can still use it with the EVB without
> recompiling QEMU, even though uart-default=UART5 for the EVB.
>
> For example, Facebook's Wedge100 BMC uses UART3: you can fetch an image from
> Github[1] and get the serial console output even while running the palmetto
> machine type, because we explicitly specify that we want UART3 to be
> connected to stdio.
>
> qemu-system-arm -machine palmetto-bmc \
> -drive file=wedge100.mtd,format=raw,if=mtd \
> -serial null -serial null -serial null -serial stdio -display none
>
> Similarly, you can boot a Fuji BMC image[2], which uses UART1, using the
> AST2600 EVB machine:
>
> qemu-system-arm -machine ast2600-evb \
> -drive file=fuji.mtd,format=raw,if=mtd \
> -serial null -serial stdio -display none
>
> This is kind of complicated, of course: it might be more natural to get rid
> of the uart_default attribute completely, and initialize UART's
> sequentially. But, keeping backward compatibility and the way most users
> know how to use QEMU in mind, this seems to make the most sense.
>
> [1] https://github.com/facebook/openbmc/releases/download/v2021.49.0/wedge100.mtd
> [2] https://github.com/facebook/openbmc/releases/download/v2021.49.0/fuji.mtd
>
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> ---
> hw/arm/aspeed_ast10x0.c | 14 +++++++++++---
> hw/arm/aspeed_ast2600.c | 10 +++++++++-
> hw/arm/aspeed_soc.c | 10 +++++++++-
> 3 files changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
> index f65dc139da..5e6f3a8fed 100644
> --- a/hw/arm/aspeed_ast10x0.c
> +++ b/hw/arm/aspeed_ast10x0.c
> @@ -215,10 +215,18 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
> qdev_get_gpio_in(DEVICE(&s->armv7m),
> sc->irqmap[ASPEED_DEV_KCS] + aspeed_lpc_kcs_4));
>
> - /* UART5 - attach an 8250 to the IO space as our UART */
> - serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
> - aspeed_soc_get_irq(s, ASPEED_DEV_UART5),
> + /* UART - attach 8250's to the IO space for each UART */
> + serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
> + aspeed_soc_get_irq(s, s->uart_default),
That's a fix for aspeed_ast10x0 that should come first.
> 38400, serial_hd(0), DEVICE_LITTLE_ENDIAN);
> + for (int i = 1, uart = ASPEED_DEV_UART1; i < 13; i++, uart++) {
'13' should be a AspeecSoCClass attribute. The number of uarts varies
depending on the SoC model and we might want to model that one day.
> + if (uart == s->uart_default) {
> + uart++;
> + }
Shouldn't we test serial_hd(i) validity ?
Thanks,
C.
> + serial_mm_init(get_system_memory(), sc->memmap[uart], 2,
> + aspeed_soc_get_irq(s, uart), 38400, serial_hd(i),
> + DEVICE_LITTLE_ENDIAN);
> + }
>
> /* Timer */
> object_property_set_link(OBJECT(&s->timerctrl), "scu", OBJECT(&s->scu),
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index 1b72800682..cbeca7f655 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -372,10 +372,18 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
> sysbus_connect_irq(SYS_BUS_DEVICE(&s->adc), 0,
> aspeed_soc_get_irq(s, ASPEED_DEV_ADC));
>
> - /* UART - attach an 8250 to the IO space as our UART */
> + /* UART - attach 8250's to the IO space for each UART */
> serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
> aspeed_soc_get_irq(s, s->uart_default), 38400,
> serial_hd(0), DEVICE_LITTLE_ENDIAN);
> + for (int i = 1, uart = ASPEED_DEV_UART1; i < 13; i++, uart++) {
> + if (uart == s->uart_default) {
> + uart++;
> + }
> + serial_mm_init(get_system_memory(), sc->memmap[uart], 2,
> + aspeed_soc_get_irq(s, uart), 38400, serial_hd(i),
> + DEVICE_LITTLE_ENDIAN);
> + }
>
> /* I2C */
> object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index 2cd03d49da..1fc1ed808d 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -303,10 +303,18 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
> sysbus_connect_irq(SYS_BUS_DEVICE(&s->adc), 0,
> aspeed_soc_get_irq(s, ASPEED_DEV_ADC));
>
> - /* UART - attach an 8250 to the IO space as our UART */
> + /* UART - attach 8250's to the IO space for each UART */
> serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
> aspeed_soc_get_irq(s, s->uart_default), 38400,
> serial_hd(0), DEVICE_LITTLE_ENDIAN);
> + for (int i = 1, uart = ASPEED_DEV_UART1; i < 5; i++, uart++) {
> + if (uart == s->uart_default) {
> + uart++;
> + }
> + serial_mm_init(get_system_memory(), sc->memmap[uart], 2,
> + aspeed_soc_get_irq(s, uart), 38400, serial_hd(i),
> + DEVICE_LITTLE_ENDIAN);
> + }
>
> /* I2C */
> object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] hw: aspeed: Init all UART's with serial devices
@ 2022-05-13 5:31 ` Cédric Le Goater
0 siblings, 0 replies; 24+ messages in thread
From: Cédric Le Goater @ 2022-05-13 5:31 UTC (permalink / raw)
To: Peter Delevoryas
Cc: irischenlj, patrick, qemu-arm, qemu-devel, zev, openbmc, andrew,
peter.maydell, joel
On 5/13/22 06:02, Peter Delevoryas wrote:
> Usually, QEMU users just provide one serial device on the command line,
> either through "-nographic" or "-serial stdio -display none", or just using
> VNC and popping up a window. We try to match what the user expects, which is
> to connect the first (and usually only) serial device to the UART a board is
> using as serial0.
>
> Most Aspeed machines in hw/arm/aspeed.c use UART5 for serial0 in their
> device tree, so we connect UART5 to the first serial device. Some machines
> use UART1 though, or UART3, so the uart_default property lets us specify
> that in a board definition.
>
> In order to specify a nonstandard serial0 UART, a user basically *must* add
> a new board definition in hw/arm/aspeed.c. There's no way to do this without
> recompiling QEMU, besides constructing the machine completely from scratch
> on the command line.
>
> To provide more flexibility, we can also support the user specifying more
> serial devices, and connect them to the UART memory regions if possible.
> Even if a user doesn't specify any extra serial devices, it's useful to
> initialize these memory regions as UART's, so that they respond to the guest
> OS more naturally. At the moment, they will just always return zero's for
> everything, and some UART registers have a default non-zero state.
>
> With this change, if a new OpenBMC image uses UART3 or some other
> nonstandard UART for serial0, you can still use it with the EVB without
> recompiling QEMU, even though uart-default=UART5 for the EVB.
>
> For example, Facebook's Wedge100 BMC uses UART3: you can fetch an image from
> Github[1] and get the serial console output even while running the palmetto
> machine type, because we explicitly specify that we want UART3 to be
> connected to stdio.
>
> qemu-system-arm -machine palmetto-bmc \
> -drive file=wedge100.mtd,format=raw,if=mtd \
> -serial null -serial null -serial null -serial stdio -display none
>
> Similarly, you can boot a Fuji BMC image[2], which uses UART1, using the
> AST2600 EVB machine:
>
> qemu-system-arm -machine ast2600-evb \
> -drive file=fuji.mtd,format=raw,if=mtd \
> -serial null -serial stdio -display none
>
> This is kind of complicated, of course: it might be more natural to get rid
> of the uart_default attribute completely, and initialize UART's
> sequentially. But, keeping backward compatibility and the way most users
> know how to use QEMU in mind, this seems to make the most sense.
>
> [1] https://github.com/facebook/openbmc/releases/download/v2021.49.0/wedge100.mtd
> [2] https://github.com/facebook/openbmc/releases/download/v2021.49.0/fuji.mtd
>
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> ---
> hw/arm/aspeed_ast10x0.c | 14 +++++++++++---
> hw/arm/aspeed_ast2600.c | 10 +++++++++-
> hw/arm/aspeed_soc.c | 10 +++++++++-
> 3 files changed, 29 insertions(+), 5 deletions(-)
>
> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
> index f65dc139da..5e6f3a8fed 100644
> --- a/hw/arm/aspeed_ast10x0.c
> +++ b/hw/arm/aspeed_ast10x0.c
> @@ -215,10 +215,18 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
> qdev_get_gpio_in(DEVICE(&s->armv7m),
> sc->irqmap[ASPEED_DEV_KCS] + aspeed_lpc_kcs_4));
>
> - /* UART5 - attach an 8250 to the IO space as our UART */
> - serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
> - aspeed_soc_get_irq(s, ASPEED_DEV_UART5),
> + /* UART - attach 8250's to the IO space for each UART */
> + serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
> + aspeed_soc_get_irq(s, s->uart_default),
That's a fix for aspeed_ast10x0 that should come first.
> 38400, serial_hd(0), DEVICE_LITTLE_ENDIAN);
> + for (int i = 1, uart = ASPEED_DEV_UART1; i < 13; i++, uart++) {
'13' should be a AspeecSoCClass attribute. The number of uarts varies
depending on the SoC model and we might want to model that one day.
> + if (uart == s->uart_default) {
> + uart++;
> + }
Shouldn't we test serial_hd(i) validity ?
Thanks,
C.
> + serial_mm_init(get_system_memory(), sc->memmap[uart], 2,
> + aspeed_soc_get_irq(s, uart), 38400, serial_hd(i),
> + DEVICE_LITTLE_ENDIAN);
> + }
>
> /* Timer */
> object_property_set_link(OBJECT(&s->timerctrl), "scu", OBJECT(&s->scu),
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index 1b72800682..cbeca7f655 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -372,10 +372,18 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
> sysbus_connect_irq(SYS_BUS_DEVICE(&s->adc), 0,
> aspeed_soc_get_irq(s, ASPEED_DEV_ADC));
>
> - /* UART - attach an 8250 to the IO space as our UART */
> + /* UART - attach 8250's to the IO space for each UART */
> serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
> aspeed_soc_get_irq(s, s->uart_default), 38400,
> serial_hd(0), DEVICE_LITTLE_ENDIAN);
> + for (int i = 1, uart = ASPEED_DEV_UART1; i < 13; i++, uart++) {
> + if (uart == s->uart_default) {
> + uart++;
> + }
> + serial_mm_init(get_system_memory(), sc->memmap[uart], 2,
> + aspeed_soc_get_irq(s, uart), 38400, serial_hd(i),
> + DEVICE_LITTLE_ENDIAN);
> + }
>
> /* I2C */
> object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index 2cd03d49da..1fc1ed808d 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -303,10 +303,18 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
> sysbus_connect_irq(SYS_BUS_DEVICE(&s->adc), 0,
> aspeed_soc_get_irq(s, ASPEED_DEV_ADC));
>
> - /* UART - attach an 8250 to the IO space as our UART */
> + /* UART - attach 8250's to the IO space for each UART */
> serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
> aspeed_soc_get_irq(s, s->uart_default), 38400,
> serial_hd(0), DEVICE_LITTLE_ENDIAN);
> + for (int i = 1, uart = ASPEED_DEV_UART1; i < 5; i++, uart++) {
> + if (uart == s->uart_default) {
> + uart++;
> + }
> + serial_mm_init(get_system_memory(), sc->memmap[uart], 2,
> + aspeed_soc_get_irq(s, uart), 38400, serial_hd(i),
> + DEVICE_LITTLE_ENDIAN);
> + }
>
> /* I2C */
> object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] hw: aspeed: Init all UART's with serial devices
2022-05-13 5:31 ` Cédric Le Goater
@ 2022-05-13 21:08 ` Peter Delevoryas
-1 siblings, 0 replies; 24+ messages in thread
From: Peter Delevoryas @ 2022-05-13 21:08 UTC (permalink / raw)
Cc: Peter Maydell, zev, Andrew Jeffery, Iris Chen, OpenBMC List,
Cameron Esfahani via, qemu-arm, Joel Stanley,
Cédric Le Goater
> On May 12, 2022, at 10:31 PM, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 5/13/22 06:02, Peter Delevoryas wrote:
>> Usually, QEMU users just provide one serial device on the command line,
>> either through "-nographic" or "-serial stdio -display none", or just using
>> VNC and popping up a window. We try to match what the user expects, which is
>> to connect the first (and usually only) serial device to the UART a board is
>> using as serial0.
>> Most Aspeed machines in hw/arm/aspeed.c use UART5 for serial0 in their
>> device tree, so we connect UART5 to the first serial device. Some machines
>> use UART1 though, or UART3, so the uart_default property lets us specify
>> that in a board definition.
>> In order to specify a nonstandard serial0 UART, a user basically *must* add
>> a new board definition in hw/arm/aspeed.c. There's no way to do this without
>> recompiling QEMU, besides constructing the machine completely from scratch
>> on the command line.
>> To provide more flexibility, we can also support the user specifying more
>> serial devices, and connect them to the UART memory regions if possible.
>> Even if a user doesn't specify any extra serial devices, it's useful to
>> initialize these memory regions as UART's, so that they respond to the guest
>> OS more naturally. At the moment, they will just always return zero's for
>> everything, and some UART registers have a default non-zero state.
>> With this change, if a new OpenBMC image uses UART3 or some other
>> nonstandard UART for serial0, you can still use it with the EVB without
>> recompiling QEMU, even though uart-default=UART5 for the EVB.
>> For example, Facebook's Wedge100 BMC uses UART3: you can fetch an image from
>> Github[1] and get the serial console output even while running the palmetto
>> machine type, because we explicitly specify that we want UART3 to be
>> connected to stdio.
>> qemu-system-arm -machine palmetto-bmc \
>> -drive file=wedge100.mtd,format=raw,if=mtd \
>> -serial null -serial null -serial null -serial stdio -display none
>> Similarly, you can boot a Fuji BMC image[2], which uses UART1, using the
>> AST2600 EVB machine:
>> qemu-system-arm -machine ast2600-evb \
>> -drive file=fuji.mtd,format=raw,if=mtd \
>> -serial null -serial stdio -display none
>> This is kind of complicated, of course: it might be more natural to get rid
>> of the uart_default attribute completely, and initialize UART's
>> sequentially. But, keeping backward compatibility and the way most users
>> know how to use QEMU in mind, this seems to make the most sense.
>> [1] https://github.com/facebook/openbmc/releases/download/v2021.49.0/wedge100.mtd
>> [2] https://github.com/facebook/openbmc/releases/download/v2021.49.0/fuji.mtd
>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>> ---
>> hw/arm/aspeed_ast10x0.c | 14 +++++++++++---
>> hw/arm/aspeed_ast2600.c | 10 +++++++++-
>> hw/arm/aspeed_soc.c | 10 +++++++++-
>> 3 files changed, 29 insertions(+), 5 deletions(-)
>> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
>> index f65dc139da..5e6f3a8fed 100644
>> --- a/hw/arm/aspeed_ast10x0.c
>> +++ b/hw/arm/aspeed_ast10x0.c
>> @@ -215,10 +215,18 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
>> qdev_get_gpio_in(DEVICE(&s->armv7m),
>> sc->irqmap[ASPEED_DEV_KCS] + aspeed_lpc_kcs_4));
>> - /* UART5 - attach an 8250 to the IO space as our UART */
>> - serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
>> - aspeed_soc_get_irq(s, ASPEED_DEV_UART5),
>> + /* UART - attach 8250's to the IO space for each UART */
>> + serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
>> + aspeed_soc_get_irq(s, s->uart_default),
>
> That's a fix for aspeed_ast10x0 that should come first.
Good point, I’ll separate this into another patch in the series instead
of doing it right here.
>
>> 38400, serial_hd(0), DEVICE_LITTLE_ENDIAN);
>> + for (int i = 1, uart = ASPEED_DEV_UART1; i < 13; i++, uart++) {
>
> '13' should be a AspeecSoCClass attribute. The number of uarts varies
> depending on the SoC model and we might want to model that one day.
True, I’ll add a patch to the series that includes that.
>
>> + if (uart == s->uart_default) {
>> + uart++;
>> + }
>
> Shouldn't we test serial_hd(i) validity ?
I was actually intentionally skipping that. If serial_hd(i)
doesn’t exist, the function will return NULL.
Chardev *serial_hd(int i)
{
assert(i >= 0);
if (i < num_serial_hds) {
return serial_hds[i];
}
return NULL;
}
So then, the serial device’s CharBackend’s “Chardev *chr”
will be initialized as NULL. Looking at all of the
usage of this attribute in “hw/char/serial.c”, I think
that’s ok, the read/write functions will just be no-ops.
They all have guards for “chr == NULL”. Take this one
as an example:
int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len)
{
Chardev *s = be->chr;
if (!s) {
return 0;
}
return qemu_chr_write(s, buf, len, false);
}
On the other hand, most of the rest of the serial device
code will run, include setting and clearing the line
status register and stuff like that. In some FB code[1] using
UART’s, processes will actually go to 100% CPU usage in QEMU
polling the line status register if it doesn’t have the
transmitter-empty bit set, mostly because the author didn’t write
fault-tolerant code, but also because it doesn’t align with how
the hardware behaves by default (I think). So, that was my
motivation for initializing serial devices, but not always
connecting a chardev backend. But I’m open to other
interpretations of how things should be setup too.
If you’d like me to only initialize a UART if a chardev backend
is provided for it, then to satisfy my use case, I would
just always make sure our test infrastructure runs QEMU
with all serial devices connected to chardevs. So, either way,
this change is still useful, and will satisfy my requirements.
Thanks,
Peter
[1] https://github.com/facebook/openbmc/blob/036586d1995ea8ce06b9fa56cc357741e9f0bb90/common/recipes-core/rackmon/rackmon/modbus.c#L69
>
> Thanks,
>
> C.
>
>> + serial_mm_init(get_system_memory(), sc->memmap[uart], 2,
>> + aspeed_soc_get_irq(s, uart), 38400, serial_hd(i),
>> + DEVICE_LITTLE_ENDIAN);
>> + }
>> /* Timer */
>> object_property_set_link(OBJECT(&s->timerctrl), "scu", OBJECT(&s->scu),
>> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
>> index 1b72800682..cbeca7f655 100644
>> --- a/hw/arm/aspeed_ast2600.c
>> +++ b/hw/arm/aspeed_ast2600.c
>> @@ -372,10 +372,18 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>> sysbus_connect_irq(SYS_BUS_DEVICE(&s->adc), 0,
>> aspeed_soc_get_irq(s, ASPEED_DEV_ADC));
>> - /* UART - attach an 8250 to the IO space as our UART */
>> + /* UART - attach 8250's to the IO space for each UART */
>> serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
>> aspeed_soc_get_irq(s, s->uart_default), 38400,
>> serial_hd(0), DEVICE_LITTLE_ENDIAN);
>> + for (int i = 1, uart = ASPEED_DEV_UART1; i < 13; i++, uart++) {
>> + if (uart == s->uart_default) {
>> + uart++;
>> + }
>> + serial_mm_init(get_system_memory(), sc->memmap[uart], 2,
>> + aspeed_soc_get_irq(s, uart), 38400, serial_hd(i),
>> + DEVICE_LITTLE_ENDIAN);
>> + }
>> /* I2C */
>> object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
>> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
>> index 2cd03d49da..1fc1ed808d 100644
>> --- a/hw/arm/aspeed_soc.c
>> +++ b/hw/arm/aspeed_soc.c
>> @@ -303,10 +303,18 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>> sysbus_connect_irq(SYS_BUS_DEVICE(&s->adc), 0,
>> aspeed_soc_get_irq(s, ASPEED_DEV_ADC));
>> - /* UART - attach an 8250 to the IO space as our UART */
>> + /* UART - attach 8250's to the IO space for each UART */
>> serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
>> aspeed_soc_get_irq(s, s->uart_default), 38400,
>> serial_hd(0), DEVICE_LITTLE_ENDIAN);
>> + for (int i = 1, uart = ASPEED_DEV_UART1; i < 5; i++, uart++) {
>> + if (uart == s->uart_default) {
>> + uart++;
>> + }
>> + serial_mm_init(get_system_memory(), sc->memmap[uart], 2,
>> + aspeed_soc_get_irq(s, uart), 38400, serial_hd(i),
>> + DEVICE_LITTLE_ENDIAN);
>> + }
>> /* I2C */
>> object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] hw: aspeed: Init all UART's with serial devices
@ 2022-05-13 21:08 ` Peter Delevoryas
0 siblings, 0 replies; 24+ messages in thread
From: Peter Delevoryas @ 2022-05-13 21:08 UTC (permalink / raw)
Cc: Iris Chen, patrick, qemu-arm, Cameron Esfahani via, zev,
OpenBMC List, Andrew Jeffery, Peter Maydell, Joel Stanley,
Cédric Le Goater
> On May 12, 2022, at 10:31 PM, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 5/13/22 06:02, Peter Delevoryas wrote:
>> Usually, QEMU users just provide one serial device on the command line,
>> either through "-nographic" or "-serial stdio -display none", or just using
>> VNC and popping up a window. We try to match what the user expects, which is
>> to connect the first (and usually only) serial device to the UART a board is
>> using as serial0.
>> Most Aspeed machines in hw/arm/aspeed.c use UART5 for serial0 in their
>> device tree, so we connect UART5 to the first serial device. Some machines
>> use UART1 though, or UART3, so the uart_default property lets us specify
>> that in a board definition.
>> In order to specify a nonstandard serial0 UART, a user basically *must* add
>> a new board definition in hw/arm/aspeed.c. There's no way to do this without
>> recompiling QEMU, besides constructing the machine completely from scratch
>> on the command line.
>> To provide more flexibility, we can also support the user specifying more
>> serial devices, and connect them to the UART memory regions if possible.
>> Even if a user doesn't specify any extra serial devices, it's useful to
>> initialize these memory regions as UART's, so that they respond to the guest
>> OS more naturally. At the moment, they will just always return zero's for
>> everything, and some UART registers have a default non-zero state.
>> With this change, if a new OpenBMC image uses UART3 or some other
>> nonstandard UART for serial0, you can still use it with the EVB without
>> recompiling QEMU, even though uart-default=UART5 for the EVB.
>> For example, Facebook's Wedge100 BMC uses UART3: you can fetch an image from
>> Github[1] and get the serial console output even while running the palmetto
>> machine type, because we explicitly specify that we want UART3 to be
>> connected to stdio.
>> qemu-system-arm -machine palmetto-bmc \
>> -drive file=wedge100.mtd,format=raw,if=mtd \
>> -serial null -serial null -serial null -serial stdio -display none
>> Similarly, you can boot a Fuji BMC image[2], which uses UART1, using the
>> AST2600 EVB machine:
>> qemu-system-arm -machine ast2600-evb \
>> -drive file=fuji.mtd,format=raw,if=mtd \
>> -serial null -serial stdio -display none
>> This is kind of complicated, of course: it might be more natural to get rid
>> of the uart_default attribute completely, and initialize UART's
>> sequentially. But, keeping backward compatibility and the way most users
>> know how to use QEMU in mind, this seems to make the most sense.
>> [1] https://github.com/facebook/openbmc/releases/download/v2021.49.0/wedge100.mtd
>> [2] https://github.com/facebook/openbmc/releases/download/v2021.49.0/fuji.mtd
>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>> ---
>> hw/arm/aspeed_ast10x0.c | 14 +++++++++++---
>> hw/arm/aspeed_ast2600.c | 10 +++++++++-
>> hw/arm/aspeed_soc.c | 10 +++++++++-
>> 3 files changed, 29 insertions(+), 5 deletions(-)
>> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
>> index f65dc139da..5e6f3a8fed 100644
>> --- a/hw/arm/aspeed_ast10x0.c
>> +++ b/hw/arm/aspeed_ast10x0.c
>> @@ -215,10 +215,18 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
>> qdev_get_gpio_in(DEVICE(&s->armv7m),
>> sc->irqmap[ASPEED_DEV_KCS] + aspeed_lpc_kcs_4));
>> - /* UART5 - attach an 8250 to the IO space as our UART */
>> - serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
>> - aspeed_soc_get_irq(s, ASPEED_DEV_UART5),
>> + /* UART - attach 8250's to the IO space for each UART */
>> + serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
>> + aspeed_soc_get_irq(s, s->uart_default),
>
> That's a fix for aspeed_ast10x0 that should come first.
Good point, I’ll separate this into another patch in the series instead
of doing it right here.
>
>> 38400, serial_hd(0), DEVICE_LITTLE_ENDIAN);
>> + for (int i = 1, uart = ASPEED_DEV_UART1; i < 13; i++, uart++) {
>
> '13' should be a AspeecSoCClass attribute. The number of uarts varies
> depending on the SoC model and we might want to model that one day.
True, I’ll add a patch to the series that includes that.
>
>> + if (uart == s->uart_default) {
>> + uart++;
>> + }
>
> Shouldn't we test serial_hd(i) validity ?
I was actually intentionally skipping that. If serial_hd(i)
doesn’t exist, the function will return NULL.
Chardev *serial_hd(int i)
{
assert(i >= 0);
if (i < num_serial_hds) {
return serial_hds[i];
}
return NULL;
}
So then, the serial device’s CharBackend’s “Chardev *chr”
will be initialized as NULL. Looking at all of the
usage of this attribute in “hw/char/serial.c”, I think
that’s ok, the read/write functions will just be no-ops.
They all have guards for “chr == NULL”. Take this one
as an example:
int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len)
{
Chardev *s = be->chr;
if (!s) {
return 0;
}
return qemu_chr_write(s, buf, len, false);
}
On the other hand, most of the rest of the serial device
code will run, include setting and clearing the line
status register and stuff like that. In some FB code[1] using
UART’s, processes will actually go to 100% CPU usage in QEMU
polling the line status register if it doesn’t have the
transmitter-empty bit set, mostly because the author didn’t write
fault-tolerant code, but also because it doesn’t align with how
the hardware behaves by default (I think). So, that was my
motivation for initializing serial devices, but not always
connecting a chardev backend. But I’m open to other
interpretations of how things should be setup too.
If you’d like me to only initialize a UART if a chardev backend
is provided for it, then to satisfy my use case, I would
just always make sure our test infrastructure runs QEMU
with all serial devices connected to chardevs. So, either way,
this change is still useful, and will satisfy my requirements.
Thanks,
Peter
[1] https://github.com/facebook/openbmc/blob/036586d1995ea8ce06b9fa56cc357741e9f0bb90/common/recipes-core/rackmon/rackmon/modbus.c#L69
>
> Thanks,
>
> C.
>
>> + serial_mm_init(get_system_memory(), sc->memmap[uart], 2,
>> + aspeed_soc_get_irq(s, uart), 38400, serial_hd(i),
>> + DEVICE_LITTLE_ENDIAN);
>> + }
>> /* Timer */
>> object_property_set_link(OBJECT(&s->timerctrl), "scu", OBJECT(&s->scu),
>> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
>> index 1b72800682..cbeca7f655 100644
>> --- a/hw/arm/aspeed_ast2600.c
>> +++ b/hw/arm/aspeed_ast2600.c
>> @@ -372,10 +372,18 @@ static void aspeed_soc_ast2600_realize(DeviceState *dev, Error **errp)
>> sysbus_connect_irq(SYS_BUS_DEVICE(&s->adc), 0,
>> aspeed_soc_get_irq(s, ASPEED_DEV_ADC));
>> - /* UART - attach an 8250 to the IO space as our UART */
>> + /* UART - attach 8250's to the IO space for each UART */
>> serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
>> aspeed_soc_get_irq(s, s->uart_default), 38400,
>> serial_hd(0), DEVICE_LITTLE_ENDIAN);
>> + for (int i = 1, uart = ASPEED_DEV_UART1; i < 13; i++, uart++) {
>> + if (uart == s->uart_default) {
>> + uart++;
>> + }
>> + serial_mm_init(get_system_memory(), sc->memmap[uart], 2,
>> + aspeed_soc_get_irq(s, uart), 38400, serial_hd(i),
>> + DEVICE_LITTLE_ENDIAN);
>> + }
>> /* I2C */
>> object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
>> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
>> index 2cd03d49da..1fc1ed808d 100644
>> --- a/hw/arm/aspeed_soc.c
>> +++ b/hw/arm/aspeed_soc.c
>> @@ -303,10 +303,18 @@ static void aspeed_soc_realize(DeviceState *dev, Error **errp)
>> sysbus_connect_irq(SYS_BUS_DEVICE(&s->adc), 0,
>> aspeed_soc_get_irq(s, ASPEED_DEV_ADC));
>> - /* UART - attach an 8250 to the IO space as our UART */
>> + /* UART - attach 8250's to the IO space for each UART */
>> serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
>> aspeed_soc_get_irq(s, s->uart_default), 38400,
>> serial_hd(0), DEVICE_LITTLE_ENDIAN);
>> + for (int i = 1, uart = ASPEED_DEV_UART1; i < 5; i++, uart++) {
>> + if (uart == s->uart_default) {
>> + uart++;
>> + }
>> + serial_mm_init(get_system_memory(), sc->memmap[uart], 2,
>> + aspeed_soc_get_irq(s, uart), 38400, serial_hd(i),
>> + DEVICE_LITTLE_ENDIAN);
>> + }
>> /* I2C */
>> object_property_set_link(OBJECT(&s->i2c), "dram", OBJECT(s->dram_mr),
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/2] hw: aspeed: Init all UART's with serial devices
2022-05-13 4:02 ` Peter Delevoryas
@ 2022-05-13 21:39 ` Zev Weiss
-1 siblings, 0 replies; 24+ messages in thread
From: Zev Weiss @ 2022-05-13 21:39 UTC (permalink / raw)
To: Peter Delevoryas
Cc: peter.maydell, andrew, irischenlj, openbmc, qemu-devel, qemu-arm,
clg, joel
On Thu, May 12, 2022 at 09:02:18PM PDT, Peter Delevoryas wrote:
> CC'ing Zev and OpenBMC since this was motivated by a problem Zev had there:
>
> https://lore.kernel.org/openbmc/YnzGnWjkYdMUUNyM@hatter.bewilderbeest.net/
>
> This series adds all the missing UART's in the Aspeed chips, and initializes
> them all with serial devices (even if there is no peer character device provided
> by the QEMU user).
>
> This allows users to quickly test UART output without any code changes. In fact,
> you could even connect all the UART's to separate sockets and check which one is
> emitting data.
>
Thanks Peter -- I tried this out with an ahe-50dc u-boot build (ast2400
with stdio on uart3), and with
-serial null -serial null -serial null -serial mon:stdio
added to the command-line I get the u-boot stdio and the qemu monitor in
my terminal as expected.
Tested-by: Zev Weiss <zev@bewilderbeest.net>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/2] hw: aspeed: Init all UART's with serial devices
@ 2022-05-13 21:39 ` Zev Weiss
0 siblings, 0 replies; 24+ messages in thread
From: Zev Weiss @ 2022-05-13 21:39 UTC (permalink / raw)
To: Peter Delevoryas
Cc: irischenlj, patrick, qemu-arm, qemu-devel, clg, openbmc, andrew,
peter.maydell, joel
On Thu, May 12, 2022 at 09:02:18PM PDT, Peter Delevoryas wrote:
> CC'ing Zev and OpenBMC since this was motivated by a problem Zev had there:
>
> https://lore.kernel.org/openbmc/YnzGnWjkYdMUUNyM@hatter.bewilderbeest.net/
>
> This series adds all the missing UART's in the Aspeed chips, and initializes
> them all with serial devices (even if there is no peer character device provided
> by the QEMU user).
>
> This allows users to quickly test UART output without any code changes. In fact,
> you could even connect all the UART's to separate sockets and check which one is
> emitting data.
>
Thanks Peter -- I tried this out with an ahe-50dc u-boot build (ast2400
with stdio on uart3), and with
-serial null -serial null -serial null -serial mon:stdio
added to the command-line I get the u-boot stdio and the qemu monitor in
my terminal as expected.
Tested-by: Zev Weiss <zev@bewilderbeest.net>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] hw: aspeed: Init all UART's with serial devices
2022-05-13 21:08 ` Peter Delevoryas
@ 2022-05-14 7:30 ` Cédric Le Goater
-1 siblings, 0 replies; 24+ messages in thread
From: Cédric Le Goater @ 2022-05-14 7:30 UTC (permalink / raw)
To: Peter Delevoryas
Cc: Iris Chen, patrick, qemu-arm, Cameron Esfahani via, zev,
OpenBMC List, Andrew Jeffery, Peter Maydell, Joel Stanley
On 5/13/22 23:08, Peter Delevoryas wrote:
>
>
>> On May 12, 2022, at 10:31 PM, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> On 5/13/22 06:02, Peter Delevoryas wrote:
>>> Usually, QEMU users just provide one serial device on the command line,
>>> either through "-nographic" or "-serial stdio -display none", or just using
>>> VNC and popping up a window. We try to match what the user expects, which is
>>> to connect the first (and usually only) serial device to the UART a board is
>>> using as serial0.
>>> Most Aspeed machines in hw/arm/aspeed.c use UART5 for serial0 in their
>>> device tree, so we connect UART5 to the first serial device. Some machines
>>> use UART1 though, or UART3, so the uart_default property lets us specify
>>> that in a board definition.
>>> In order to specify a nonstandard serial0 UART, a user basically *must* add
>>> a new board definition in hw/arm/aspeed.c. There's no way to do this without
>>> recompiling QEMU, besides constructing the machine completely from scratch
>>> on the command line.
>>> To provide more flexibility, we can also support the user specifying more
>>> serial devices, and connect them to the UART memory regions if possible.
>>> Even if a user doesn't specify any extra serial devices, it's useful to
>>> initialize these memory regions as UART's, so that they respond to the guest
>>> OS more naturally. At the moment, they will just always return zero's for
>>> everything, and some UART registers have a default non-zero state.
>>> With this change, if a new OpenBMC image uses UART3 or some other
>>> nonstandard UART for serial0, you can still use it with the EVB without
>>> recompiling QEMU, even though uart-default=UART5 for the EVB.
>>> For example, Facebook's Wedge100 BMC uses UART3: you can fetch an image from
>>> Github[1] and get the serial console output even while running the palmetto
>>> machine type, because we explicitly specify that we want UART3 to be
>>> connected to stdio.
>>> qemu-system-arm -machine palmetto-bmc \
>>> -drive file=wedge100.mtd,format=raw,if=mtd \
>>> -serial null -serial null -serial null -serial stdio -display none
>>> Similarly, you can boot a Fuji BMC image[2], which uses UART1, using the
>>> AST2600 EVB machine:
>>> qemu-system-arm -machine ast2600-evb \
>>> -drive file=fuji.mtd,format=raw,if=mtd \
>>> -serial null -serial stdio -display none
>>> This is kind of complicated, of course: it might be more natural to get rid
>>> of the uart_default attribute completely, and initialize UART's
>>> sequentially. But, keeping backward compatibility and the way most users
>>> know how to use QEMU in mind, this seems to make the most sense.
>>> [1] https://github.com/facebook/openbmc/releases/download/v2021.49.0/wedge100.mtd
>>> [2] https://github.com/facebook/openbmc/releases/download/v2021.49.0/fuji.mtd
>>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>>> ---
>>> hw/arm/aspeed_ast10x0.c | 14 +++++++++++---
>>> hw/arm/aspeed_ast2600.c | 10 +++++++++-
>>> hw/arm/aspeed_soc.c | 10 +++++++++-
>>> 3 files changed, 29 insertions(+), 5 deletions(-)
>>> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
>>> index f65dc139da..5e6f3a8fed 100644
>>> --- a/hw/arm/aspeed_ast10x0.c
>>> +++ b/hw/arm/aspeed_ast10x0.c
>>> @@ -215,10 +215,18 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
>>> qdev_get_gpio_in(DEVICE(&s->armv7m),
>>> sc->irqmap[ASPEED_DEV_KCS] + aspeed_lpc_kcs_4));
>>> - /* UART5 - attach an 8250 to the IO space as our UART */
>>> - serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
>>> - aspeed_soc_get_irq(s, ASPEED_DEV_UART5),
>>> + /* UART - attach 8250's to the IO space for each UART */
>>> + serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
>>> + aspeed_soc_get_irq(s, s->uart_default),
>>
>> That's a fix for aspeed_ast10x0 that should come first.
>
> Good point, I’ll separate this into another patch in the series instead
> of doing it right here.
>
>>
>>> 38400, serial_hd(0), DEVICE_LITTLE_ENDIAN);
>>> + for (int i = 1, uart = ASPEED_DEV_UART1; i < 13; i++, uart++) {
>>
>> '13' should be a AspeecSoCClass attribute. The number of uarts varies
>> depending on the SoC model and we might want to model that one day.
>
> True, I’ll add a patch to the series that includes that.
>
>>
>>> + if (uart == s->uart_default) {
>>> + uart++;
>>> + }
>>
>> Shouldn't we test serial_hd(i) validity ?
>
> I was actually intentionally skipping that. If serial_hd(i)
> doesn’t exist, the function will return NULL.
>
> Chardev *serial_hd(int i)
> {
> assert(i >= 0);
> if (i < num_serial_hds) {
> return serial_hds[i];
> }
> return NULL;
> }
>
> So then, the serial device’s CharBackend’s “Chardev *chr”
> will be initialized as NULL. Looking at all of the
> usage of this attribute in “hw/char/serial.c”, I think
> that’s ok, the read/write functions will just be no-ops.
> They all have guards for “chr == NULL”. Take this one
> as an example:
>
> int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len)
> {
> Chardev *s = be->chr;
>
> if (!s) {
> return 0;
> }
>
> return qemu_chr_write(s, buf, len, false);
> }
>
> On the other hand, most of the rest of the serial device
> code will run, include setting and clearing the line
> status register and stuff like that. In some FB code[1] using
> UART’s, processes will actually go to 100% CPU usage in QEMU
> polling the line status register if it doesn’t have the
> transmitter-empty bit set, mostly because the author didn’t write
> fault-tolerant code, but also because it doesn’t align with how
> the hardware behaves by default (I think). So, that was my
> motivation for initializing serial devices, but not always
> connecting a chardev backend. But I’m open to other
> interpretations of how things should be setup too.
>
> If you’d like me to only initialize a UART if a chardev backend
> is provided for it, then to satisfy my use case, I would
> just always make sure our test infrastructure runs QEMU
> with all serial devices connected to chardevs. So, either way,
> this change is still useful, and will satisfy my requirements.
The problem is that it is breaking compatibility with previous QEMUs.
We can not change the command line to :
qemu-system-arm -machine palmetto-bmc \
-drive file=wedge100.mtd,format=raw,if=mtd \
-serial null -serial null -serial null -serial stdio -display none
What about adding a machine "uart" option ? like we did for the fmc/spi
models
Thanks,
C.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] hw: aspeed: Init all UART's with serial devices
@ 2022-05-14 7:30 ` Cédric Le Goater
0 siblings, 0 replies; 24+ messages in thread
From: Cédric Le Goater @ 2022-05-14 7:30 UTC (permalink / raw)
To: Peter Delevoryas
Cc: Peter Maydell, zev, Andrew Jeffery, Iris Chen, OpenBMC List,
Cameron Esfahani via, qemu-arm, Joel Stanley
On 5/13/22 23:08, Peter Delevoryas wrote:
>
>
>> On May 12, 2022, at 10:31 PM, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> On 5/13/22 06:02, Peter Delevoryas wrote:
>>> Usually, QEMU users just provide one serial device on the command line,
>>> either through "-nographic" or "-serial stdio -display none", or just using
>>> VNC and popping up a window. We try to match what the user expects, which is
>>> to connect the first (and usually only) serial device to the UART a board is
>>> using as serial0.
>>> Most Aspeed machines in hw/arm/aspeed.c use UART5 for serial0 in their
>>> device tree, so we connect UART5 to the first serial device. Some machines
>>> use UART1 though, or UART3, so the uart_default property lets us specify
>>> that in a board definition.
>>> In order to specify a nonstandard serial0 UART, a user basically *must* add
>>> a new board definition in hw/arm/aspeed.c. There's no way to do this without
>>> recompiling QEMU, besides constructing the machine completely from scratch
>>> on the command line.
>>> To provide more flexibility, we can also support the user specifying more
>>> serial devices, and connect them to the UART memory regions if possible.
>>> Even if a user doesn't specify any extra serial devices, it's useful to
>>> initialize these memory regions as UART's, so that they respond to the guest
>>> OS more naturally. At the moment, they will just always return zero's for
>>> everything, and some UART registers have a default non-zero state.
>>> With this change, if a new OpenBMC image uses UART3 or some other
>>> nonstandard UART for serial0, you can still use it with the EVB without
>>> recompiling QEMU, even though uart-default=UART5 for the EVB.
>>> For example, Facebook's Wedge100 BMC uses UART3: you can fetch an image from
>>> Github[1] and get the serial console output even while running the palmetto
>>> machine type, because we explicitly specify that we want UART3 to be
>>> connected to stdio.
>>> qemu-system-arm -machine palmetto-bmc \
>>> -drive file=wedge100.mtd,format=raw,if=mtd \
>>> -serial null -serial null -serial null -serial stdio -display none
>>> Similarly, you can boot a Fuji BMC image[2], which uses UART1, using the
>>> AST2600 EVB machine:
>>> qemu-system-arm -machine ast2600-evb \
>>> -drive file=fuji.mtd,format=raw,if=mtd \
>>> -serial null -serial stdio -display none
>>> This is kind of complicated, of course: it might be more natural to get rid
>>> of the uart_default attribute completely, and initialize UART's
>>> sequentially. But, keeping backward compatibility and the way most users
>>> know how to use QEMU in mind, this seems to make the most sense.
>>> [1] https://github.com/facebook/openbmc/releases/download/v2021.49.0/wedge100.mtd
>>> [2] https://github.com/facebook/openbmc/releases/download/v2021.49.0/fuji.mtd
>>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>>> ---
>>> hw/arm/aspeed_ast10x0.c | 14 +++++++++++---
>>> hw/arm/aspeed_ast2600.c | 10 +++++++++-
>>> hw/arm/aspeed_soc.c | 10 +++++++++-
>>> 3 files changed, 29 insertions(+), 5 deletions(-)
>>> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
>>> index f65dc139da..5e6f3a8fed 100644
>>> --- a/hw/arm/aspeed_ast10x0.c
>>> +++ b/hw/arm/aspeed_ast10x0.c
>>> @@ -215,10 +215,18 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
>>> qdev_get_gpio_in(DEVICE(&s->armv7m),
>>> sc->irqmap[ASPEED_DEV_KCS] + aspeed_lpc_kcs_4));
>>> - /* UART5 - attach an 8250 to the IO space as our UART */
>>> - serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
>>> - aspeed_soc_get_irq(s, ASPEED_DEV_UART5),
>>> + /* UART - attach 8250's to the IO space for each UART */
>>> + serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
>>> + aspeed_soc_get_irq(s, s->uart_default),
>>
>> That's a fix for aspeed_ast10x0 that should come first.
>
> Good point, I’ll separate this into another patch in the series instead
> of doing it right here.
>
>>
>>> 38400, serial_hd(0), DEVICE_LITTLE_ENDIAN);
>>> + for (int i = 1, uart = ASPEED_DEV_UART1; i < 13; i++, uart++) {
>>
>> '13' should be a AspeecSoCClass attribute. The number of uarts varies
>> depending on the SoC model and we might want to model that one day.
>
> True, I’ll add a patch to the series that includes that.
>
>>
>>> + if (uart == s->uart_default) {
>>> + uart++;
>>> + }
>>
>> Shouldn't we test serial_hd(i) validity ?
>
> I was actually intentionally skipping that. If serial_hd(i)
> doesn’t exist, the function will return NULL.
>
> Chardev *serial_hd(int i)
> {
> assert(i >= 0);
> if (i < num_serial_hds) {
> return serial_hds[i];
> }
> return NULL;
> }
>
> So then, the serial device’s CharBackend’s “Chardev *chr”
> will be initialized as NULL. Looking at all of the
> usage of this attribute in “hw/char/serial.c”, I think
> that’s ok, the read/write functions will just be no-ops.
> They all have guards for “chr == NULL”. Take this one
> as an example:
>
> int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len)
> {
> Chardev *s = be->chr;
>
> if (!s) {
> return 0;
> }
>
> return qemu_chr_write(s, buf, len, false);
> }
>
> On the other hand, most of the rest of the serial device
> code will run, include setting and clearing the line
> status register and stuff like that. In some FB code[1] using
> UART’s, processes will actually go to 100% CPU usage in QEMU
> polling the line status register if it doesn’t have the
> transmitter-empty bit set, mostly because the author didn’t write
> fault-tolerant code, but also because it doesn’t align with how
> the hardware behaves by default (I think). So, that was my
> motivation for initializing serial devices, but not always
> connecting a chardev backend. But I’m open to other
> interpretations of how things should be setup too.
>
> If you’d like me to only initialize a UART if a chardev backend
> is provided for it, then to satisfy my use case, I would
> just always make sure our test infrastructure runs QEMU
> with all serial devices connected to chardevs. So, either way,
> this change is still useful, and will satisfy my requirements.
The problem is that it is breaking compatibility with previous QEMUs.
We can not change the command line to :
qemu-system-arm -machine palmetto-bmc \
-drive file=wedge100.mtd,format=raw,if=mtd \
-serial null -serial null -serial null -serial stdio -display none
What about adding a machine "uart" option ? like we did for the fmc/spi
models
Thanks,
C.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] hw: aspeed: Init all UART's with serial devices
2022-05-14 7:30 ` Cédric Le Goater
@ 2022-05-14 7:39 ` Peter Delevoryas
-1 siblings, 0 replies; 24+ messages in thread
From: Peter Delevoryas @ 2022-05-14 7:39 UTC (permalink / raw)
Cc: Peter Maydell, zev, Andrew Jeffery, Iris Chen, OpenBMC List,
Cameron Esfahani via, qemu-arm, Joel Stanley,
Cédric Le Goater
> On May 14, 2022, at 12:30 AM, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 5/13/22 23:08, Peter Delevoryas wrote:
>>> On May 12, 2022, at 10:31 PM, Cédric Le Goater <clg@kaod.org> wrote:
>>>
>>> On 5/13/22 06:02, Peter Delevoryas wrote:
>>>> Usually, QEMU users just provide one serial device on the command line,
>>>> either through "-nographic" or "-serial stdio -display none", or just using
>>>> VNC and popping up a window. We try to match what the user expects, which is
>>>> to connect the first (and usually only) serial device to the UART a board is
>>>> using as serial0.
>>>> Most Aspeed machines in hw/arm/aspeed.c use UART5 for serial0 in their
>>>> device tree, so we connect UART5 to the first serial device. Some machines
>>>> use UART1 though, or UART3, so the uart_default property lets us specify
>>>> that in a board definition.
>>>> In order to specify a nonstandard serial0 UART, a user basically *must* add
>>>> a new board definition in hw/arm/aspeed.c. There's no way to do this without
>>>> recompiling QEMU, besides constructing the machine completely from scratch
>>>> on the command line.
>>>> To provide more flexibility, we can also support the user specifying more
>>>> serial devices, and connect them to the UART memory regions if possible.
>>>> Even if a user doesn't specify any extra serial devices, it's useful to
>>>> initialize these memory regions as UART's, so that they respond to the guest
>>>> OS more naturally. At the moment, they will just always return zero's for
>>>> everything, and some UART registers have a default non-zero state.
>>>> With this change, if a new OpenBMC image uses UART3 or some other
>>>> nonstandard UART for serial0, you can still use it with the EVB without
>>>> recompiling QEMU, even though uart-default=UART5 for the EVB.
>>>> For example, Facebook's Wedge100 BMC uses UART3: you can fetch an image from
>>>> Github[1] and get the serial console output even while running the palmetto
>>>> machine type, because we explicitly specify that we want UART3 to be
>>>> connected to stdio.
>>>> qemu-system-arm -machine palmetto-bmc \
>>>> -drive file=wedge100.mtd,format=raw,if=mtd \
>>>> -serial null -serial null -serial null -serial stdio -display none
>>>> Similarly, you can boot a Fuji BMC image[2], which uses UART1, using the
>>>> AST2600 EVB machine:
>>>> qemu-system-arm -machine ast2600-evb \
>>>> -drive file=fuji.mtd,format=raw,if=mtd \
>>>> -serial null -serial stdio -display none
>>>> This is kind of complicated, of course: it might be more natural to get rid
>>>> of the uart_default attribute completely, and initialize UART's
>>>> sequentially. But, keeping backward compatibility and the way most users
>>>> know how to use QEMU in mind, this seems to make the most sense.
>>>> [1] https://github.com/facebook/openbmc/releases/download/v2021.49.0/wedge100.mtd
>>>> [2] https://github.com/facebook/openbmc/releases/download/v2021.49.0/fuji.mtd
>>>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>>>> ---
>>>> hw/arm/aspeed_ast10x0.c | 14 +++++++++++---
>>>> hw/arm/aspeed_ast2600.c | 10 +++++++++-
>>>> hw/arm/aspeed_soc.c | 10 +++++++++-
>>>> 3 files changed, 29 insertions(+), 5 deletions(-)
>>>> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
>>>> index f65dc139da..5e6f3a8fed 100644
>>>> --- a/hw/arm/aspeed_ast10x0.c
>>>> +++ b/hw/arm/aspeed_ast10x0.c
>>>> @@ -215,10 +215,18 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
>>>> qdev_get_gpio_in(DEVICE(&s->armv7m),
>>>> sc->irqmap[ASPEED_DEV_KCS] + aspeed_lpc_kcs_4));
>>>> - /* UART5 - attach an 8250 to the IO space as our UART */
>>>> - serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
>>>> - aspeed_soc_get_irq(s, ASPEED_DEV_UART5),
>>>> + /* UART - attach 8250's to the IO space for each UART */
>>>> + serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
>>>> + aspeed_soc_get_irq(s, s->uart_default),
>>>
>>> That's a fix for aspeed_ast10x0 that should come first.
>> Good point, I’ll separate this into another patch in the series instead
>> of doing it right here.
>>>
>>>> 38400, serial_hd(0), DEVICE_LITTLE_ENDIAN);
>>>> + for (int i = 1, uart = ASPEED_DEV_UART1; i < 13; i++, uart++) {
>>>
>>> '13' should be a AspeecSoCClass attribute. The number of uarts varies
>>> depending on the SoC model and we might want to model that one day.
>> True, I’ll add a patch to the series that includes that.
>>>
>>>> + if (uart == s->uart_default) {
>>>> + uart++;
>>>> + }
>>>
>>> Shouldn't we test serial_hd(i) validity ?
>> I was actually intentionally skipping that. If serial_hd(i)
>> doesn’t exist, the function will return NULL.
>> Chardev *serial_hd(int i)
>> {
>> assert(i >= 0);
>> if (i < num_serial_hds) {
>> return serial_hds[i];
>> }
>> return NULL;
>> }
>> So then, the serial device’s CharBackend’s “Chardev *chr”
>> will be initialized as NULL. Looking at all of the
>> usage of this attribute in “hw/char/serial.c”, I think
>> that’s ok, the read/write functions will just be no-ops.
>> They all have guards for “chr == NULL”. Take this one
>> as an example:
>> int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len)
>> {
>> Chardev *s = be->chr;
>> if (!s) {
>> return 0;
>> }
>> return qemu_chr_write(s, buf, len, false);
>> }
>> On the other hand, most of the rest of the serial device
>> code will run, include setting and clearing the line
>> status register and stuff like that. In some FB code[1] using
>> UART’s, processes will actually go to 100% CPU usage in QEMU
>> polling the line status register if it doesn’t have the
>> transmitter-empty bit set, mostly because the author didn’t write
>> fault-tolerant code, but also because it doesn’t align with how
>> the hardware behaves by default (I think). So, that was my
>> motivation for initializing serial devices, but not always
>> connecting a chardev backend. But I’m open to other
>> interpretations of how things should be setup too.
>> If you’d like me to only initialize a UART if a chardev backend
>> is provided for it, then to satisfy my use case, I would
>> just always make sure our test infrastructure runs QEMU
>> with all serial devices connected to chardevs. So, either way,
>> this change is still useful, and will satisfy my requirements.
>
> The problem is that it is breaking compatibility with previous QEMUs.
It is? We can still run things the old way too, I specifically
wrote this with the intention that it would support backwards
compatibility.
The first “-serial” argument is connected to the “s->uart_default” UART.
Then the remaining “-serial” arguments (if they are present at all)
are connected sequentially to the remaining UART’s.
If only one -serial argument is provided, or a user uses -nographic/etc,
then it just does what it has always done. It should be completely optional.
The only change in behavior is that now more UART’s will be initialized,
so if some guest OS was depending on the line status register for an
unconnected UART always reading “transmit buffer not-empty”, then
they will be in trouble. But that’s not how the hardware would
work normally anyways.
> We can not change the command line to :
>
> qemu-system-arm -machine palmetto-bmc \
> -drive file=wedge100.mtd,format=raw,if=mtd \
> -serial null -serial null -serial null -serial stdio -display none
>
> What about adding a machine "uart" option ? like we did for the fmc/spi
> models
>
> Thanks,
>
> C.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] hw: aspeed: Init all UART's with serial devices
@ 2022-05-14 7:39 ` Peter Delevoryas
0 siblings, 0 replies; 24+ messages in thread
From: Peter Delevoryas @ 2022-05-14 7:39 UTC (permalink / raw)
Cc: Iris Chen, patrick, qemu-arm, Cameron Esfahani via, zev,
OpenBMC List, Andrew Jeffery, Peter Maydell, Joel Stanley,
Cédric Le Goater
> On May 14, 2022, at 12:30 AM, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 5/13/22 23:08, Peter Delevoryas wrote:
>>> On May 12, 2022, at 10:31 PM, Cédric Le Goater <clg@kaod.org> wrote:
>>>
>>> On 5/13/22 06:02, Peter Delevoryas wrote:
>>>> Usually, QEMU users just provide one serial device on the command line,
>>>> either through "-nographic" or "-serial stdio -display none", or just using
>>>> VNC and popping up a window. We try to match what the user expects, which is
>>>> to connect the first (and usually only) serial device to the UART a board is
>>>> using as serial0.
>>>> Most Aspeed machines in hw/arm/aspeed.c use UART5 for serial0 in their
>>>> device tree, so we connect UART5 to the first serial device. Some machines
>>>> use UART1 though, or UART3, so the uart_default property lets us specify
>>>> that in a board definition.
>>>> In order to specify a nonstandard serial0 UART, a user basically *must* add
>>>> a new board definition in hw/arm/aspeed.c. There's no way to do this without
>>>> recompiling QEMU, besides constructing the machine completely from scratch
>>>> on the command line.
>>>> To provide more flexibility, we can also support the user specifying more
>>>> serial devices, and connect them to the UART memory regions if possible.
>>>> Even if a user doesn't specify any extra serial devices, it's useful to
>>>> initialize these memory regions as UART's, so that they respond to the guest
>>>> OS more naturally. At the moment, they will just always return zero's for
>>>> everything, and some UART registers have a default non-zero state.
>>>> With this change, if a new OpenBMC image uses UART3 or some other
>>>> nonstandard UART for serial0, you can still use it with the EVB without
>>>> recompiling QEMU, even though uart-default=UART5 for the EVB.
>>>> For example, Facebook's Wedge100 BMC uses UART3: you can fetch an image from
>>>> Github[1] and get the serial console output even while running the palmetto
>>>> machine type, because we explicitly specify that we want UART3 to be
>>>> connected to stdio.
>>>> qemu-system-arm -machine palmetto-bmc \
>>>> -drive file=wedge100.mtd,format=raw,if=mtd \
>>>> -serial null -serial null -serial null -serial stdio -display none
>>>> Similarly, you can boot a Fuji BMC image[2], which uses UART1, using the
>>>> AST2600 EVB machine:
>>>> qemu-system-arm -machine ast2600-evb \
>>>> -drive file=fuji.mtd,format=raw,if=mtd \
>>>> -serial null -serial stdio -display none
>>>> This is kind of complicated, of course: it might be more natural to get rid
>>>> of the uart_default attribute completely, and initialize UART's
>>>> sequentially. But, keeping backward compatibility and the way most users
>>>> know how to use QEMU in mind, this seems to make the most sense.
>>>> [1] https://github.com/facebook/openbmc/releases/download/v2021.49.0/wedge100.mtd
>>>> [2] https://github.com/facebook/openbmc/releases/download/v2021.49.0/fuji.mtd
>>>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>>>> ---
>>>> hw/arm/aspeed_ast10x0.c | 14 +++++++++++---
>>>> hw/arm/aspeed_ast2600.c | 10 +++++++++-
>>>> hw/arm/aspeed_soc.c | 10 +++++++++-
>>>> 3 files changed, 29 insertions(+), 5 deletions(-)
>>>> diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c
>>>> index f65dc139da..5e6f3a8fed 100644
>>>> --- a/hw/arm/aspeed_ast10x0.c
>>>> +++ b/hw/arm/aspeed_ast10x0.c
>>>> @@ -215,10 +215,18 @@ static void aspeed_soc_ast1030_realize(DeviceState *dev_soc, Error **errp)
>>>> qdev_get_gpio_in(DEVICE(&s->armv7m),
>>>> sc->irqmap[ASPEED_DEV_KCS] + aspeed_lpc_kcs_4));
>>>> - /* UART5 - attach an 8250 to the IO space as our UART */
>>>> - serial_mm_init(get_system_memory(), sc->memmap[ASPEED_DEV_UART5], 2,
>>>> - aspeed_soc_get_irq(s, ASPEED_DEV_UART5),
>>>> + /* UART - attach 8250's to the IO space for each UART */
>>>> + serial_mm_init(get_system_memory(), sc->memmap[s->uart_default], 2,
>>>> + aspeed_soc_get_irq(s, s->uart_default),
>>>
>>> That's a fix for aspeed_ast10x0 that should come first.
>> Good point, I’ll separate this into another patch in the series instead
>> of doing it right here.
>>>
>>>> 38400, serial_hd(0), DEVICE_LITTLE_ENDIAN);
>>>> + for (int i = 1, uart = ASPEED_DEV_UART1; i < 13; i++, uart++) {
>>>
>>> '13' should be a AspeecSoCClass attribute. The number of uarts varies
>>> depending on the SoC model and we might want to model that one day.
>> True, I’ll add a patch to the series that includes that.
>>>
>>>> + if (uart == s->uart_default) {
>>>> + uart++;
>>>> + }
>>>
>>> Shouldn't we test serial_hd(i) validity ?
>> I was actually intentionally skipping that. If serial_hd(i)
>> doesn’t exist, the function will return NULL.
>> Chardev *serial_hd(int i)
>> {
>> assert(i >= 0);
>> if (i < num_serial_hds) {
>> return serial_hds[i];
>> }
>> return NULL;
>> }
>> So then, the serial device’s CharBackend’s “Chardev *chr”
>> will be initialized as NULL. Looking at all of the
>> usage of this attribute in “hw/char/serial.c”, I think
>> that’s ok, the read/write functions will just be no-ops.
>> They all have guards for “chr == NULL”. Take this one
>> as an example:
>> int qemu_chr_fe_write(CharBackend *be, const uint8_t *buf, int len)
>> {
>> Chardev *s = be->chr;
>> if (!s) {
>> return 0;
>> }
>> return qemu_chr_write(s, buf, len, false);
>> }
>> On the other hand, most of the rest of the serial device
>> code will run, include setting and clearing the line
>> status register and stuff like that. In some FB code[1] using
>> UART’s, processes will actually go to 100% CPU usage in QEMU
>> polling the line status register if it doesn’t have the
>> transmitter-empty bit set, mostly because the author didn’t write
>> fault-tolerant code, but also because it doesn’t align with how
>> the hardware behaves by default (I think). So, that was my
>> motivation for initializing serial devices, but not always
>> connecting a chardev backend. But I’m open to other
>> interpretations of how things should be setup too.
>> If you’d like me to only initialize a UART if a chardev backend
>> is provided for it, then to satisfy my use case, I would
>> just always make sure our test infrastructure runs QEMU
>> with all serial devices connected to chardevs. So, either way,
>> this change is still useful, and will satisfy my requirements.
>
> The problem is that it is breaking compatibility with previous QEMUs.
It is? We can still run things the old way too, I specifically
wrote this with the intention that it would support backwards
compatibility.
The first “-serial” argument is connected to the “s->uart_default” UART.
Then the remaining “-serial” arguments (if they are present at all)
are connected sequentially to the remaining UART’s.
If only one -serial argument is provided, or a user uses -nographic/etc,
then it just does what it has always done. It should be completely optional.
The only change in behavior is that now more UART’s will be initialized,
so if some guest OS was depending on the line status register for an
unconnected UART always reading “transmit buffer not-empty”, then
they will be in trouble. But that’s not how the hardware would
work normally anyways.
> We can not change the command line to :
>
> qemu-system-arm -machine palmetto-bmc \
> -drive file=wedge100.mtd,format=raw,if=mtd \
> -serial null -serial null -serial null -serial stdio -display none
>
> What about adding a machine "uart" option ? like we did for the fmc/spi
> models
>
> Thanks,
>
> C.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] hw: aspeed: Init all UART's with serial devices
2022-05-14 7:39 ` Peter Delevoryas
@ 2022-05-15 21:19 ` Cédric Le Goater
-1 siblings, 0 replies; 24+ messages in thread
From: Cédric Le Goater @ 2022-05-15 21:19 UTC (permalink / raw)
To: Peter Delevoryas
Cc: Peter Maydell, zev, Andrew Jeffery, Iris Chen, OpenBMC List,
Cameron Esfahani via, qemu-arm, Joel Stanley
[ ... ]
>> The problem is that it is breaking compatibility with previous QEMUs.
>
> It is? We can still run things the old way too, I specifically
> wrote this with the intention that it would support backwards
> compatibility.
You are right. Let's start with your patchset. We can add the "uart"
machine option when the need arises.
I have sent a small cleanup of aspeed_soc_get_irq() that should avoid
the duplication of the serial init in the different SoC models. Please
give it a try.
Thanks,
C.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] hw: aspeed: Init all UART's with serial devices
@ 2022-05-15 21:19 ` Cédric Le Goater
0 siblings, 0 replies; 24+ messages in thread
From: Cédric Le Goater @ 2022-05-15 21:19 UTC (permalink / raw)
To: Peter Delevoryas
Cc: Iris Chen, patrick, qemu-arm, Cameron Esfahani via, zev,
OpenBMC List, Andrew Jeffery, Peter Maydell, Joel Stanley
[ ... ]
>> The problem is that it is breaking compatibility with previous QEMUs.
>
> It is? We can still run things the old way too, I specifically
> wrote this with the intention that it would support backwards
> compatibility.
You are right. Let's start with your patchset. We can add the "uart"
machine option when the need arises.
I have sent a small cleanup of aspeed_soc_get_irq() that should avoid
the duplication of the serial init in the different SoC models. Please
give it a try.
Thanks,
C.
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] hw: aspeed: Init all UART's with serial devices
2022-05-15 21:19 ` Cédric Le Goater
@ 2022-05-15 22:55 ` Peter Delevoryas
-1 siblings, 0 replies; 24+ messages in thread
From: Peter Delevoryas @ 2022-05-15 22:55 UTC (permalink / raw)
Cc: Peter Maydell, zev, Andrew Jeffery, Iris Chen, OpenBMC List,
Cameron Esfahani via, qemu-arm, Joel Stanley, Peter Delevoryas,
Cédric Le Goater
> On May 15, 2022, at 2:19 PM, Cédric Le Goater <clg@kaod.org> wrote:
>
> [ ... ]
>
>>> The problem is that it is breaking compatibility with previous QEMUs.
>> It is? We can still run things the old way too, I specifically
>> wrote this with the intention that it would support backwards
>> compatibility.
>
> You are right. Let's start with your patchset. We can add the "uart"
> machine option when the need arises.
>
> I have sent a small cleanup of aspeed_soc_get_irq() that should avoid
> the duplication of the serial init in the different SoC models. Please
> give it a try.
>
Oh this is fantastic, thanks! I’ll gladly put the serial init code into
a function declared in aspeed_soc.h then.
> Thanks,
>
> C.
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] hw: aspeed: Init all UART's with serial devices
@ 2022-05-15 22:55 ` Peter Delevoryas
0 siblings, 0 replies; 24+ messages in thread
From: Peter Delevoryas @ 2022-05-15 22:55 UTC (permalink / raw)
Cc: Iris Chen, patrick, qemu-arm, Cameron Esfahani via, zev,
OpenBMC List, Andrew Jeffery, Peter Maydell, Joel Stanley,
Cédric Le Goater, Peter Delevoryas
> On May 15, 2022, at 2:19 PM, Cédric Le Goater <clg@kaod.org> wrote:
>
> [ ... ]
>
>>> The problem is that it is breaking compatibility with previous QEMUs.
>> It is? We can still run things the old way too, I specifically
>> wrote this with the intention that it would support backwards
>> compatibility.
>
> You are right. Let's start with your patchset. We can add the "uart"
> machine option when the need arises.
>
> I have sent a small cleanup of aspeed_soc_get_irq() that should avoid
> the duplication of the serial init in the different SoC models. Please
> give it a try.
>
Oh this is fantastic, thanks! I’ll gladly put the serial init code into
a function declared in aspeed_soc.h then.
> Thanks,
>
> C.
>
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] hw: aspeed: Init all UART's with serial devices
2022-05-13 21:08 ` Peter Delevoryas
@ 2022-05-16 14:56 ` Peter Maydell
-1 siblings, 0 replies; 24+ messages in thread
From: Peter Maydell @ 2022-05-16 14:56 UTC (permalink / raw)
To: Peter Delevoryas
Cc: zev, Andrew Jeffery, Iris Chen, OpenBMC List,
Cameron Esfahani via, qemu-arm, Joel Stanley,
Cédric Le Goater
On Fri, 13 May 2022 at 22:09, Peter Delevoryas <pdel@fb.com> wrote
> I was actually intentionally skipping that. If serial_hd(i)
> doesn’t exist, the function will return NULL.
>
> Chardev *serial_hd(int i)
> {
> assert(i >= 0);
> if (i < num_serial_hds) {
> return serial_hds[i];
> }
> return NULL;
> }
>
> So then, the serial device’s CharBackend’s “Chardev *chr”
> will be initialized as NULL. Looking at all of the
> usage of this attribute in “hw/char/serial.c”, I think
> that’s ok, the read/write functions will just be no-ops.
> They all have guards for “chr == NULL”.
Yes, this is deliberate. We added these in commit 12051d82f0040
because otherwise lots of board/SoC code would have to create
NullChardev dummy backends (or forget to and then crash depending
on the user's commandline).
thanks
-- PMM
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/2] hw: aspeed: Init all UART's with serial devices
@ 2022-05-16 14:56 ` Peter Maydell
0 siblings, 0 replies; 24+ messages in thread
From: Peter Maydell @ 2022-05-16 14:56 UTC (permalink / raw)
To: Peter Delevoryas
Cc: Iris Chen, patrick, qemu-arm, Cameron Esfahani via, zev,
OpenBMC List, Andrew Jeffery, Joel Stanley,
Cédric Le Goater
On Fri, 13 May 2022 at 22:09, Peter Delevoryas <pdel@fb.com> wrote
> I was actually intentionally skipping that. If serial_hd(i)
> doesn’t exist, the function will return NULL.
>
> Chardev *serial_hd(int i)
> {
> assert(i >= 0);
> if (i < num_serial_hds) {
> return serial_hds[i];
> }
> return NULL;
> }
>
> So then, the serial device’s CharBackend’s “Chardev *chr”
> will be initialized as NULL. Looking at all of the
> usage of this attribute in “hw/char/serial.c”, I think
> that’s ok, the read/write functions will just be no-ops.
> They all have guards for “chr == NULL”.
Yes, this is deliberate. We added these in commit 12051d82f0040
because otherwise lots of board/SoC code would have to create
NullChardev dummy backends (or forget to and then crash depending
on the user's commandline).
thanks
-- PMM
^ permalink raw reply [flat|nested] 24+ messages in thread
end of thread, other threads:[~2022-05-16 15:39 UTC | newest]
Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-05-13 4:02 [PATCH 0/2] hw: aspeed: Init all UART's with serial devices Peter Delevoryas
2022-05-13 4:02 ` Peter Delevoryas
2022-05-13 4:02 ` [PATCH 1/2] hw: aspeed: Add missing UART's Peter Delevoryas
2022-05-13 4:02 ` Peter Delevoryas
2022-05-13 5:21 ` Cédric Le Goater
2022-05-13 5:21 ` Cédric Le Goater
2022-05-13 4:02 ` [PATCH 2/2] hw: aspeed: Init all UART's with serial devices Peter Delevoryas
2022-05-13 4:02 ` Peter Delevoryas
2022-05-13 5:31 ` Cédric Le Goater
2022-05-13 5:31 ` Cédric Le Goater
2022-05-13 21:08 ` Peter Delevoryas
2022-05-13 21:08 ` Peter Delevoryas
2022-05-14 7:30 ` Cédric Le Goater
2022-05-14 7:30 ` Cédric Le Goater
2022-05-14 7:39 ` Peter Delevoryas
2022-05-14 7:39 ` Peter Delevoryas
2022-05-15 21:19 ` Cédric Le Goater
2022-05-15 21:19 ` Cédric Le Goater
2022-05-15 22:55 ` Peter Delevoryas
2022-05-15 22:55 ` Peter Delevoryas
2022-05-16 14:56 ` Peter Maydell
2022-05-16 14:56 ` Peter Maydell
2022-05-13 21:39 ` [PATCH 0/2] " Zev Weiss
2022-05-13 21:39 ` Zev Weiss
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.