[PATCH 0/5] hw/arm/aspeed: Add fuji machine type

[PATCH 0/5] hw/arm/aspeed: Add fuji machine type

[pdel]

From: Peter Delevoryas <pdel@fb.com>

Hello!

This patch series creates an Aspeed machine type for Facebook's OpenBMC
platform "fuji".

The first 2 commits do some refactoring, to allow Aspeed machines to
configure the first serial device. Most board configurations use UART5
for the console, but fuji uses UART1. Neither of these should change the
behavior for any machine types.

The third commit adds the fuji machine type definition, utilizing the
"serial_dev" option from the previous two commits to configure the
console device as UART1 instead of UART5.

After the third commit, you can test booting a fuji image as follows:

    # Build a fuji image from Facebook's OpenBMC repository.
    git clone https://github.com/facebook/openbmc
    cd openbmc
    ./sync_yocto.sh
    source openbmc-init-build-env fuji build-fuji
    bitbake fuji-image

    dd if=/dev/zero of=/tmp/fuji-image.mtd bs=1M count=128
    dd if=./tmp/deploy/images/fuji/flash-fuji of=/tmp/fuji-image.mtd \
        bs=1k conv=notrunc

    git clone https://github.com/peterdelevoryas/qemu
    cd qemu
    ./configure --target-list=arm-softmmu
    make -j $(nproc)
    
    # Attempt to boot the fuji image: you should not see any console
    # output.
    ./build/arm-softmmu/qemu-system-arm -machine fuji \
        -drive file=/tmp/fuji-image.mtd,format=raw,if=mtd -serial stdio

You shouldn't see any serial console output, because U-Boot hangs in
clock rate initialization due to a divide-by-zero. The last 2 commits
fixup the clock registers to avoid the divide-by-zero, and fuji boots
successfully after that.

I organized the patch series with the clock rate fixes last because it
was more natural to test the behavior before and after the fix, but I
can understand if you'd like those patches to come first, or to even be
added completely independently from the fuji patch series.

This is my first contribution to QEMU, and I tried to follow the
wiki/etc as best as possible, but I'm sure I probably made some
mistakes, so let me know how best to submit this.

Peter Delevoryas (5):
  hw/arm/aspeed: Add get_irq to AspeedSoCClass
  hw/arm/aspeed: Select console UART from machine
  hw/arm/aspeed: Add fuji machine type
  hw/arm/aspeed: Fix AST2600_CLK_SEL3 address
  hw/arm/aspeed: Initialize AST2600 clock selection registers

 hw/arm/aspeed.c             | 20 ++++++++++++++++++++
 hw/arm/aspeed_ast2600.c     |  6 +-----
 hw/arm/aspeed_soc.c         |  6 +-----
 hw/misc/aspeed_scu.c        | 15 +++++++++++++--
 include/hw/arm/aspeed.h     |  1 +
 include/hw/arm/aspeed_soc.h |  1 +
 6 files changed, 37 insertions(+), 12 deletions(-)

-- 
2.30.2

[PATCH 1/5] hw/arm/aspeed: Add get_irq to AspeedSoCClass

[pdel]

From: Peter Delevoryas <pdel@fb.com>

The AST2500 uses different logic than the AST2600 for getting IRQ's.
This adds a virtual `get_irq` function to the Aspeed SOC class, so that
the shared initialization code in `hw/arm/aspeed.c` can retrieve IRQ's.

Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
 hw/arm/aspeed_ast2600.c     | 1 +
 hw/arm/aspeed_soc.c         | 1 +
 include/hw/arm/aspeed_soc.h | 1 +
 3 files changed, 3 insertions(+)

diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index e3013128c6..56e1adb343 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -527,6 +527,7 @@ static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
     sc->irqmap       = aspeed_soc_ast2600_irqmap;
     sc->memmap       = aspeed_soc_ast2600_memmap;
     sc->num_cpus     = 2;
+    sc->get_irq      = aspeed_soc_get_irq;
 }
 
 static const TypeInfo aspeed_soc_ast2600_type_info = {
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 3ad6c56fa9..c373182299 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -476,6 +476,7 @@ static void aspeed_soc_ast2400_class_init(ObjectClass *oc, void *data)
     sc->irqmap       = aspeed_soc_ast2400_irqmap;
     sc->memmap       = aspeed_soc_ast2400_memmap;
     sc->num_cpus     = 1;
+    sc->get_irq      = aspeed_soc_get_irq;
 }
 
 static const TypeInfo aspeed_soc_ast2400_type_info = {
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index d9161d26d6..ca16e1e383 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -84,6 +84,7 @@ struct AspeedSoCClass {
     const int *irqmap;
     const hwaddr *memmap;
     uint32_t num_cpus;
+    qemu_irq (*get_irq)(AspeedSoCState *s, int ctrl);
 };
 
 
-- 
2.30.2

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

[pdel]

From: Peter Delevoryas <pdel@fb.com>

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

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

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

[PATCH 3/5] hw/arm/aspeed: Add fuji machine type

[pdel]

From: Peter Delevoryas <pdel@fb.com>

Fuji uses the AST2600, so it's very similar to `ast2600-evb`, but it has
a few slight differences, such as using UART1 for the serial console.

Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
 hw/arm/aspeed.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
index ff53d12395..d2eb516a1d 100644
--- a/hw/arm/aspeed.c
+++ b/hw/arm/aspeed.c
@@ -1029,6 +1029,15 @@ static void aspeed_machine_rainier_class_init(ObjectClass *oc, void *data)
         aspeed_soc_num_cpus(amc->soc_name);
 };
 
+static void aspeed_machine_fuji_class_init(ObjectClass *oc, void *data)
+{
+    MachineClass *mc = MACHINE_CLASS(oc);
+    AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
+
+    mc->desc = "Facebook Fuji BMC (Aspeed AST2600, Cortex-A7)";
+    amc->serial_dev = ASPEED_DEV_UART1;
+}
+
 static const TypeInfo aspeed_machine_types[] = {
     {
         .name          = MACHINE_TYPE_NAME("palmetto-bmc"),
@@ -1078,6 +1087,10 @@ static const TypeInfo aspeed_machine_types[] = {
         .name          = MACHINE_TYPE_NAME("rainier-bmc"),
         .parent        = TYPE_ASPEED_MACHINE,
         .class_init    = aspeed_machine_rainier_class_init,
+    }, {
+        .name          = MACHINE_TYPE_NAME("fuji"),
+        .parent        = MACHINE_TYPE_NAME("ast2600-evb"),
+        .class_init    = aspeed_machine_fuji_class_init,
     }, {
         .name          = TYPE_ASPEED_MACHINE,
         .parent        = TYPE_MACHINE,
-- 
2.30.2

[PATCH 4/5] hw/arm/aspeed: Fix AST2600_CLK_SEL3 address

[pdel]

From: Peter Delevoryas <pdel@fb.com>

This register address is not actually used anywhere, and the datasheet
specifies that it's zero-initialized by default anyways, but the address
is incorrect. This just corrects the address.

Fixes: e09cf36321f6 ("hw: aspeed_scu: Add AST2600 support")
Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
 hw/misc/aspeed_scu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
index 40a38ebd85..c373e678f0 100644
--- a/hw/misc/aspeed_scu.c
+++ b/hw/misc/aspeed_scu.c
@@ -108,7 +108,7 @@
 #define AST2600_EPLL_EXT          TO_REG(0x244)
 #define AST2600_CLK_SEL           TO_REG(0x300)
 #define AST2600_CLK_SEL2          TO_REG(0x304)
-#define AST2600_CLK_SEL3          TO_REG(0x310)
+#define AST2600_CLK_SEL3          TO_REG(0x308)
 #define AST2600_HW_STRAP1         TO_REG(0x500)
 #define AST2600_HW_STRAP1_CLR     TO_REG(0x504)
 #define AST2600_HW_STRAP1_PROT    TO_REG(0x508)
-- 
2.30.2

[PATCH 5/5] hw/arm/aspeed: Initialize AST2600 clock selection registers

[pdel]

From: Peter Delevoryas <pdel@fb.com>

UART5 is typically used as the default debug UART on the AST2600, but
UART1 is also designed to be a debug UART. All the AST2600 UART's have
semi-configurable clock rates through registers in the System Control
Unit (SCU), but only UART5 works out of the box with zero-initialized
values. The rest of the UART's expect a few of the registers to be
initialized to non-zero values, or else the clock rate calculation will
yield zero or undefined (due to a divide-by-zero).

For reference, the U-Boot clock rate driver here shows the calculation:

    https://github.com/facebook/openbmc-uboot/blob/main/drivers/clk/aspeed/clk_ast2600.c#L357)

To summarize, UART5 allows selection from 4 rates: 24 MHz, 192 MHz, 24 /
13 MHz, and 192 / 13 MHz. The other UART's allow selecting either the
"low" rate (UARTCLK) or the "high" rate (HUARTCLK). UARTCLK and HUARTCLK
are configurable themselves:

    UARTCLK = UXCLK * R / (N * 2)
    HUARTCLK = HUXCLK * HR / (HN * 2)

UXCLK and HUXCLK are also configurable, and depend on the APLL and/or
HPLL clock rates, which also derive from complicated calculations. Long
story short, there's lots of multiplication and division from
configurable registers, and most of these registers are zero-initialized
in QEMU, which at best is unexpected and at worst causes this clock rate
driver to hang from divide-by-zero's. This can also be difficult to
diagnose, because it may cause U-Boot to hang before serial console
initialization completes, requiring intervention from gdb.

This change just initializes all of these registers with default values
from the datasheet.

Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
 hw/misc/aspeed_scu.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
index c373e678f0..d51fe8564d 100644
--- a/hw/misc/aspeed_scu.c
+++ b/hw/misc/aspeed_scu.c
@@ -104,11 +104,16 @@
 #define AST2600_SDRAM_HANDSHAKE   TO_REG(0x100)
 #define AST2600_HPLL_PARAM        TO_REG(0x200)
 #define AST2600_HPLL_EXT          TO_REG(0x204)
+#define AST2600_APLL_PARAM        TO_REG(0x210)
 #define AST2600_MPLL_EXT          TO_REG(0x224)
 #define AST2600_EPLL_EXT          TO_REG(0x244)
 #define AST2600_CLK_SEL           TO_REG(0x300)
 #define AST2600_CLK_SEL2          TO_REG(0x304)
 #define AST2600_CLK_SEL3          TO_REG(0x308)
+#define AST2600_CLK_SEL4          TO_REG(0x310)
+#define AST2600_CLK_SEL5          TO_REG(0x314)
+#define AST2600_UARTCLK_PARAM     TO_REG(0x338)
+#define AST2600_HUARTCLK_PARAM    TO_REG(0x33C)
 #define AST2600_HW_STRAP1         TO_REG(0x500)
 #define AST2600_HW_STRAP1_CLR     TO_REG(0x504)
 #define AST2600_HW_STRAP1_PROT    TO_REG(0x508)
@@ -658,9 +663,15 @@ static const uint32_t ast2600_a1_resets[ASPEED_AST2600_SCU_NR_REGS] = {
     [AST2600_CLK_STOP_CTRL2]    = 0xFFF0FFF0,
     [AST2600_SDRAM_HANDSHAKE]   = 0x00000000,
     [AST2600_HPLL_PARAM]        = 0x1000405F,
+    [AST2600_APLL_PARAM]        = 0x1000405F,
     [AST2600_CHIP_ID0]          = 0x1234ABCD,
     [AST2600_CHIP_ID1]          = 0x88884444,
-
+    [AST2600_CLK_SEL2]          = 0x00700000,
+    [AST2600_CLK_SEL3]          = 0x00000000,
+    [AST2600_CLK_SEL4]          = 0xF3F40000,
+    [AST2600_CLK_SEL5]          = 0x30000000,
+    [AST2600_UARTCLK_PARAM]     = 0x00014506,
+    [AST2600_HUARTCLK_PARAM]    = 0x000145C0,
 };
 
 static void aspeed_ast2600_scu_reset(DeviceState *dev)
-- 
2.30.2

Re: [PATCH 1/5] hw/arm/aspeed: Add get_irq to AspeedSoCClass

[Peter Delevoryas]

[-- Attachment #1: Type: text/plain, Size: 2460 bytes --]

Just noticed, I forgot to initialize get_irq for the AST2500. I didn’t notice it because I hadn’t tested booting an image for -machine ast2500-evb. I’ll make sure to test with images for all 3 chip revisions, then I’ll resubmit with PATCH v2. I’ll wait a little while though, for comments on the rest of the series.

From: pdel@fb.com <pdel@fb.com>
Date: Friday, August 27, 2021 at 2:06 PM
To:
Cc: clg@kaod.org <clg@kaod.org>, joel@jms.id.au <joel@jms.id.au>, qemu-devel@nongnu.org <qemu-devel@nongnu.org>, qemu-arm@nongnu.org <qemu-arm@nongnu.org>, Peter Delevoryas <pdel@fb.com>
Subject: [PATCH 1/5] hw/arm/aspeed: Add get_irq to AspeedSoCClass
From: Peter Delevoryas <pdel@fb.com>

The AST2500 uses different logic than the AST2600 for getting IRQ's.
This adds a virtual `get_irq` function to the Aspeed SOC class, so that
the shared initialization code in `hw/arm/aspeed.c` can retrieve IRQ's.

Signed-off-by: Peter Delevoryas <pdel@fb.com>
---
 hw/arm/aspeed_ast2600.c     | 1 +
 hw/arm/aspeed_soc.c         | 1 +
 include/hw/arm/aspeed_soc.h | 1 +
 3 files changed, 3 insertions(+)

diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
index e3013128c6..56e1adb343 100644
--- a/hw/arm/aspeed_ast2600.c
+++ b/hw/arm/aspeed_ast2600.c
@@ -527,6 +527,7 @@ static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
     sc->irqmap       = aspeed_soc_ast2600_irqmap;
     sc->memmap       = aspeed_soc_ast2600_memmap;
     sc->num_cpus     = 2;
+    sc->get_irq      = aspeed_soc_get_irq;
 }

 static const TypeInfo aspeed_soc_ast2600_type_info = {
diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
index 3ad6c56fa9..c373182299 100644
--- a/hw/arm/aspeed_soc.c
+++ b/hw/arm/aspeed_soc.c
@@ -476,6 +476,7 @@ static void aspeed_soc_ast2400_class_init(ObjectClass *oc, void *data)
     sc->irqmap       = aspeed_soc_ast2400_irqmap;
     sc->memmap       = aspeed_soc_ast2400_memmap;
     sc->num_cpus     = 1;
+    sc->get_irq      = aspeed_soc_get_irq;
 }

 static const TypeInfo aspeed_soc_ast2400_type_info = {
diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
index d9161d26d6..ca16e1e383 100644
--- a/include/hw/arm/aspeed_soc.h
+++ b/include/hw/arm/aspeed_soc.h
@@ -84,6 +84,7 @@ struct AspeedSoCClass {
     const int *irqmap;
     const hwaddr *memmap;
     uint32_t num_cpus;
+    qemu_irq (*get_irq)(AspeedSoCState *s, int ctrl);
 };


--
2.30.2

[-- Attachment #2: Type: text/html, Size: 4902 bytes --]

Re: [PATCH 4/5] hw/arm/aspeed: Fix AST2600_CLK_SEL3 address

[Cédric Le Goater]

On 8/27/21 11:04 PM, pdel@fb.com wrote:
> From: Peter Delevoryas <pdel@fb.com>
> 
> This register address is not actually used anywhere, and the datasheet
> specifies that it's zero-initialized by default anyways, but the address
> is incorrect. This just corrects the address.
> 
> Fixes: e09cf36321f6 ("hw: aspeed_scu: Add AST2600 support")
> Signed-off-by: Peter Delevoryas <pdel@fb.com>

This is covered by a patch already sent by Joel. 

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

Thanks,

C.

> ---
>  hw/misc/aspeed_scu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> index 40a38ebd85..c373e678f0 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -108,7 +108,7 @@
>  #define AST2600_EPLL_EXT          TO_REG(0x244)
>  #define AST2600_CLK_SEL           TO_REG(0x300)
>  #define AST2600_CLK_SEL2          TO_REG(0x304)
> -#define AST2600_CLK_SEL3          TO_REG(0x310)
> +#define AST2600_CLK_SEL3          TO_REG(0x308)
>  #define AST2600_HW_STRAP1         TO_REG(0x500)
>  #define AST2600_HW_STRAP1_CLR     TO_REG(0x504)
>  #define AST2600_HW_STRAP1_PROT    TO_REG(0x508)
> 

Re: [PATCH 5/5] hw/arm/aspeed: Initialize AST2600 clock selection registers

[Cédric Le Goater]

On 8/27/21 11:04 PM, pdel@fb.com wrote:
> From: Peter Delevoryas <pdel@fb.com>
> 
> UART5 is typically used as the default debug UART on the AST2600, but
> UART1 is also designed to be a debug UART. All the AST2600 UART's have
> semi-configurable clock rates through registers in the System Control
> Unit (SCU), but only UART5 works out of the box with zero-initialized
> values. The rest of the UART's expect a few of the registers to be
> initialized to non-zero values, or else the clock rate calculation will
> yield zero or undefined (due to a divide-by-zero).
>
> For reference, the U-Boot clock rate driver here shows the calculation:
> 
>     https://github.com/facebook/openbmc-uboot/blob/main/drivers/clk/aspeed/clk_ast2600.c#L357)
> 
> To summarize, UART5 allows selection from 4 rates: 24 MHz, 192 MHz, 24 /
> 13 MHz, and 192 / 13 MHz. The other UART's allow selecting either the
> "low" rate (UARTCLK) or the "high" rate (HUARTCLK). UARTCLK and HUARTCLK
> are configurable themselves:
> 
>     UARTCLK = UXCLK * R / (N * 2)
>     HUARTCLK = HUXCLK * HR / (HN * 2)
> 
> UXCLK and HUXCLK are also configurable, and depend on the APLL and/or
> HPLL clock rates, which also derive from complicated calculations. Long
> story short, there's lots of multiplication and division from
> configurable registers, and most of these registers are zero-initialized
> in QEMU, which at best is unexpected and at worst causes this clock rate
> driver to hang from divide-by-zero's. This can also be difficult to
> diagnose, because it may cause U-Boot to hang before serial console
> initialization completes, requiring intervention from gdb.
> 
> This change just initializes all of these registers with default values
> from the datasheet.
> 
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> ---
>  hw/misc/aspeed_scu.c | 13 ++++++++++++-
>  1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> index c373e678f0..d51fe8564d 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -104,11 +104,16 @@
>  #define AST2600_SDRAM_HANDSHAKE   TO_REG(0x100)
>  #define AST2600_HPLL_PARAM        TO_REG(0x200)
>  #define AST2600_HPLL_EXT          TO_REG(0x204)
> +#define AST2600_APLL_PARAM        TO_REG(0x210)
>  #define AST2600_MPLL_EXT          TO_REG(0x224)
>  #define AST2600_EPLL_EXT          TO_REG(0x244)
>  #define AST2600_CLK_SEL           TO_REG(0x300)
>  #define AST2600_CLK_SEL2          TO_REG(0x304)
>  #define AST2600_CLK_SEL3          TO_REG(0x308)
> +#define AST2600_CLK_SEL4          TO_REG(0x310)
> +#define AST2600_CLK_SEL5          TO_REG(0x314)
> +#define AST2600_UARTCLK_PARAM     TO_REG(0x338)
> +#define AST2600_HUARTCLK_PARAM    TO_REG(0x33C)
>  #define AST2600_HW_STRAP1         TO_REG(0x500)
>  #define AST2600_HW_STRAP1_CLR     TO_REG(0x504)
>  #define AST2600_HW_STRAP1_PROT    TO_REG(0x508)
> @@ -658,9 +663,15 @@ static const uint32_t ast2600_a1_resets[ASPEED_AST2600_SCU_NR_REGS] = {
>      [AST2600_CLK_STOP_CTRL2]    = 0xFFF0FFF0,
>      [AST2600_SDRAM_HANDSHAKE]   = 0x00000000,
>      [AST2600_HPLL_PARAM]        = 0x1000405F,
> +    [AST2600_APLL_PARAM]        = 0x1000405F,
>      [AST2600_CHIP_ID0]          = 0x1234ABCD,
>      [AST2600_CHIP_ID1]          = 0x88884444,
> -
> +    [AST2600_CLK_SEL2]          = 0x00700000,
> +    [AST2600_CLK_SEL3]          = 0x00000000,
> +    [AST2600_CLK_SEL4]          = 0xF3F40000,
> +    [AST2600_CLK_SEL5]          = 0x30000000,
> +    [AST2600_UARTCLK_PARAM]     = 0x00014506,
> +    [AST2600_HUARTCLK_PARAM]    = 0x000145C0,
>  };
>  
>  static void aspeed_ast2600_scu_reset(DeviceState *dev)
> 

Some parts have been already covered by the A3 emulation changes 
provided by Joel. I will merge the UART registers only.

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

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

[Cédric Le Goater]

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

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


Thanks,

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

Re: [PATCH 1/5] hw/arm/aspeed: Add get_irq to AspeedSoCClass

[Cédric Le Goater]

Hello Peter,

On 8/27/21 11:04 PM, pdel@fb.com wrote:
> From: Peter Delevoryas <pdel@fb.com>
> 
> The AST2500 uses different logic than the AST2600 for getting IRQ's.
> This adds a virtual `get_irq` function to the Aspeed SOC class, so that
> the shared initialization code in `hw/arm/aspeed.c` can retrieve IRQ's.

Thanks for this new machine. See my comment on patch 2. We might need 
to rework the serial initialization which would deprecate this change.

C.

> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> ---
>  hw/arm/aspeed_ast2600.c     | 1 +
>  hw/arm/aspeed_soc.c         | 1 +
>  include/hw/arm/aspeed_soc.h | 1 +
>  3 files changed, 3 insertions(+)
> 
> diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c
> index e3013128c6..56e1adb343 100644
> --- a/hw/arm/aspeed_ast2600.c
> +++ b/hw/arm/aspeed_ast2600.c
> @@ -527,6 +527,7 @@ static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data)
>      sc->irqmap       = aspeed_soc_ast2600_irqmap;
>      sc->memmap       = aspeed_soc_ast2600_memmap;
>      sc->num_cpus     = 2;
> +    sc->get_irq      = aspeed_soc_get_irq;
>  }
>  
>  static const TypeInfo aspeed_soc_ast2600_type_info = {
> diff --git a/hw/arm/aspeed_soc.c b/hw/arm/aspeed_soc.c
> index 3ad6c56fa9..c373182299 100644
> --- a/hw/arm/aspeed_soc.c
> +++ b/hw/arm/aspeed_soc.c
> @@ -476,6 +476,7 @@ static void aspeed_soc_ast2400_class_init(ObjectClass *oc, void *data)
>      sc->irqmap       = aspeed_soc_ast2400_irqmap;
>      sc->memmap       = aspeed_soc_ast2400_memmap;
>      sc->num_cpus     = 1;
> +    sc->get_irq      = aspeed_soc_get_irq;
>  }
>  
>  static const TypeInfo aspeed_soc_ast2400_type_info = {
> diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h
> index d9161d26d6..ca16e1e383 100644
> --- a/include/hw/arm/aspeed_soc.h
> +++ b/include/hw/arm/aspeed_soc.h
> @@ -84,6 +84,7 @@ struct AspeedSoCClass {
>      const int *irqmap;
>      const hwaddr *memmap;
>      uint32_t num_cpus;
> +    qemu_irq (*get_irq)(AspeedSoCState *s, int ctrl);
>  };
>  
>  
> 

Re: [PATCH 3/5] hw/arm/aspeed: Add fuji machine type

[Cédric Le Goater]

On 8/27/21 11:04 PM, pdel@fb.com wrote:
> From: Peter Delevoryas <pdel@fb.com>
> 
> Fuji uses the AST2600, so it's very similar to `ast2600-evb`, but it has
> a few slight differences, such as using UART1 for the serial console.

Do we have a public DTS for this board ? 

Thanks,

C.

> 
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> ---
>  hw/arm/aspeed.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index ff53d12395..d2eb516a1d 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -1029,6 +1029,15 @@ static void aspeed_machine_rainier_class_init(ObjectClass *oc, void *data)
>          aspeed_soc_num_cpus(amc->soc_name);
>  };
>  
> +static void aspeed_machine_fuji_class_init(ObjectClass *oc, void *data)
> +{
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +    AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
> +
> +    mc->desc = "Facebook Fuji BMC (Aspeed AST2600, Cortex-A7)";
> +    amc->serial_dev = ASPEED_DEV_UART1;
> +}
> +
>  static const TypeInfo aspeed_machine_types[] = {
>      {
>          .name          = MACHINE_TYPE_NAME("palmetto-bmc"),
> @@ -1078,6 +1087,10 @@ static const TypeInfo aspeed_machine_types[] = {
>          .name          = MACHINE_TYPE_NAME("rainier-bmc"),
>          .parent        = TYPE_ASPEED_MACHINE,
>          .class_init    = aspeed_machine_rainier_class_init,
> +    }, {
> +        .name          = MACHINE_TYPE_NAME("fuji"),
> +        .parent        = MACHINE_TYPE_NAME("ast2600-evb"),
> +        .class_init    = aspeed_machine_fuji_class_init,
>      }, {
>          .name          = TYPE_ASPEED_MACHINE,
>          .parent        = TYPE_MACHINE,
> 

Re: [PATCH 4/5] hw/arm/aspeed: Fix AST2600_CLK_SEL3 address

[Peter Delevoryas]

[-- Attachment #1: Type: text/plain, Size: 1575 bytes --]

Oh, thanks, I’ll remove this part!

From: Cédric Le Goater <clg@kaod.org>
Date: Saturday, August 28, 2021 at 1:15 AM
To: Peter Delevoryas <pdel@fb.com>
Cc: joel@jms.id.au <joel@jms.id.au>, qemu-devel@nongnu.org <qemu-devel@nongnu.org>, qemu-arm@nongnu.org <qemu-arm@nongnu.org>
Subject: Re: [PATCH 4/5] hw/arm/aspeed: Fix AST2600_CLK_SEL3 address
On 8/27/21 11:04 PM, pdel@fb.com wrote:
> From: Peter Delevoryas <pdel@fb.com>
>
> This register address is not actually used anywhere, and the datasheet
> specifies that it's zero-initialized by default anyways, but the address
> is incorrect. This just corrects the address.
>
> Fixes: e09cf36321f6 ("hw: aspeed_scu: Add AST2600 support")
> Signed-off-by: Peter Delevoryas <pdel@fb.com>

This is covered by a patch already sent by Joel.

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

Thanks,

C.

> ---
>  hw/misc/aspeed_scu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/misc/aspeed_scu.c b/hw/misc/aspeed_scu.c
> index 40a38ebd85..c373e678f0 100644
> --- a/hw/misc/aspeed_scu.c
> +++ b/hw/misc/aspeed_scu.c
> @@ -108,7 +108,7 @@
>  #define AST2600_EPLL_EXT          TO_REG(0x244)
>  #define AST2600_CLK_SEL           TO_REG(0x300)
>  #define AST2600_CLK_SEL2          TO_REG(0x304)
> -#define AST2600_CLK_SEL3          TO_REG(0x310)
> +#define AST2600_CLK_SEL3          TO_REG(0x308)
>  #define AST2600_HW_STRAP1         TO_REG(0x500)
>  #define AST2600_HW_STRAP1_CLR     TO_REG(0x504)
>  #define AST2600_HW_STRAP1_PROT    TO_REG(0x508)
>

[-- Attachment #2: Type: text/html, Size: 3956 bytes --]

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

[Peter Delevoryas]

[-- Attachment #1: Type: text/plain, Size: 5933 bytes --]

I think I’m a little confused on this part. What I meant by “most machines just use UART5” was that most DTS’s use “stdout-path=&uart5”, but fuji uses “stdout-path=&uart1”. I do see that SCU510 includes a bit related to UART, but it’s for disabling booting from UART1 and UART5. I just care about the console aspect, not booting.

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

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

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

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

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

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


Thanks,

C.

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

[-- Attachment #2: Type: text/html, Size: 11176 bytes --]

Re: [PATCH 3/5] hw/arm/aspeed: Add fuji machine type

[Peter Delevoryas]

[-- Attachment #1: Type: text/plain, Size: 2205 bytes --]

Actually yes! I should have included a link to it: https://github.com/facebook/openbmc-uboot/blob/openbmc/helium/v2019.04/arch/arm/dts/aspeed-bmc-facebook-fuji.dts

From: Cédric Le Goater <clg@kaod.org>
Date: Saturday, August 28, 2021 at 1:28 AM
To: Peter Delevoryas <pdel@fb.com>
Cc: joel@jms.id.au <joel@jms.id.au>, qemu-devel@nongnu.org <qemu-devel@nongnu.org>, qemu-arm@nongnu.org <qemu-arm@nongnu.org>
Subject: Re: [PATCH 3/5] hw/arm/aspeed: Add fuji machine type
On 8/27/21 11:04 PM, pdel@fb.com wrote:
> From: Peter Delevoryas <pdel@fb.com>
>
> Fuji uses the AST2600, so it's very similar to `ast2600-evb`, but it has
> a few slight differences, such as using UART1 for the serial console.

Do we have a public DTS for this board ?

Thanks,

C.

>
> Signed-off-by: Peter Delevoryas <pdel@fb.com>
> ---
>  hw/arm/aspeed.c | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
> index ff53d12395..d2eb516a1d 100644
> --- a/hw/arm/aspeed.c
> +++ b/hw/arm/aspeed.c
> @@ -1029,6 +1029,15 @@ static void aspeed_machine_rainier_class_init(ObjectClass *oc, void *data)
>          aspeed_soc_num_cpus(amc->soc_name);
>  };
>
> +static void aspeed_machine_fuji_class_init(ObjectClass *oc, void *data)
> +{
> +    MachineClass *mc = MACHINE_CLASS(oc);
> +    AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
> +
> +    mc->desc = "Facebook Fuji BMC (Aspeed AST2600, Cortex-A7)";
> +    amc->serial_dev = ASPEED_DEV_UART1;
> +}
> +
>  static const TypeInfo aspeed_machine_types[] = {
>      {
>          .name          = MACHINE_TYPE_NAME("palmetto-bmc"),
> @@ -1078,6 +1087,10 @@ static const TypeInfo aspeed_machine_types[] = {
>          .name          = MACHINE_TYPE_NAME("rainier-bmc"),
>          .parent        = TYPE_ASPEED_MACHINE,
>          .class_init    = aspeed_machine_rainier_class_init,
> +    }, {
> +        .name          = MACHINE_TYPE_NAME("fuji"),
> +        .parent        = MACHINE_TYPE_NAME("ast2600-evb"),
> +        .class_init    = aspeed_machine_fuji_class_init,
>      }, {
>          .name          = TYPE_ASPEED_MACHINE,
>          .parent        = TYPE_MACHINE,
>

[-- Attachment #2: Type: text/html, Size: 5120 bytes --]

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

[Cédric Le Goater]

On 8/28/21 5:58 PM, Peter Delevoryas wrote:
> I think I’m a little confused on this part. What I meant by “most machines just use UART5” was that most DTS’s use “stdout-path=&uart5”, but fuji uses “stdout-path=&uart1”. I /do/ see that SCU510 includes a bit related to UART, but it’s for disabling booting from UART1 and UART5. I just care about the console aspect, not booting.

OK. 

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

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

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

See

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

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

AFAICT, you will need to resend PATCH 5 only.

Thanks,

C.


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

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

[Cédric Le Goater]

On 8/28/21 5:58 PM, Peter Delevoryas wrote:
> I think I’m a little confused on this part. What I meant by “most machines just use UART5” was that most DTS’s use “stdout-path=&uart5”, but fuji uses “stdout-path=&uart1”. I /do/ see that SCU510 includes a bit related to UART, but it’s for disabling booting from UART1 and UART5. I just care about the console aspect, not booting.

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

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

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

Yes. The patch above enables the UART1 RX function. 

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

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

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

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


Thanks,

C.

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

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

[Andrew Jeffery]

Hi Cédric, Peter,

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

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

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

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

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

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

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

Andrew

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

[Cédric Le Goater]

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

ok. 

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

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

The serial init needs a change  : 

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

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

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

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

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

Thanks for the feedback Andrew,

C. 

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

[Peter Delevoryas]

I’ll resend PATCH 5, and like you’re suggesting, I can use the existing ast2600-evb machine type for testing.

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

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

> On Aug 31, 2021, at 1:15 AM, Cédric Le Goater <clg@kaod.org> wrote:
> 
> I would prefer that. This patch has been in my tree for years :
> 
>  https://github.com/legoater/qemu/commit/138713ee1d3d84682b85c1b01577a7e86ab3fed4
> 
> See
> 
>  https://github.com/legoater/qemu/commits/aspeed-6.2
> 
> May be this is what you simply need ? You can use the ast2600-evb machine for 
> your tests. Tell me how it goes. 
> 
> AFAICT, you will need to resend PATCH 5 only.
> 
> Thanks,
> 
> C.

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

[Cédric Le Goater]

On 8/31/21 3:51 PM, Peter Delevoryas wrote:
> I’ll resend PATCH 5, and like you’re suggesting, I can use the existing ast2600-evb machine type for testing.
> 
>     # Setup serial_hd(1) as UART1 in 2600 SoC realize (I won’t include this in the diff though)
>     UART5=serial_hd(0)
>     UART1=serial_hd(1)
> 
>     qemu-system-arm -machine ast2600-evb -serial null -serial stdio

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

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

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

Thanks for testing.

C.

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

[Peter Delevoryas]

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

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

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

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

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

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

Thanks for your consideration, I really appreciate it!

Thanks,
Peter

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

[Philippe Mathieu-Daudé]

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

I'm not sure I'm following what you are suggesting. If we are talking
about QEMU device initialization, QEMU must initialize all devices
on the board, regardless the guest code uses them or not.

Re: [PATCH 3/5] hw/arm/aspeed: Add fuji machine type

[Philippe Mathieu-Daudé]

On 8/28/21 6:00 PM, Peter Delevoryas wrote:
> Actually yes! I should have included a link to it:
> https://github.com/facebook/openbmc-uboot/blob/openbmc/helium/v2019.04/arch/arm/dts/aspeed-bmc-facebook-fuji.dts

(On technical lists, it's best to avoid top-posting, and to
instead reply inline to make the conversation easier to follow).

Can you add a test for your board please? See examples
in tests/acceptance/boot_linux_console.py:
- test_arm_ast2500_romulus_openbmc_v2_9_0
- test_arm_ast2600_debian

> *From: *Cédric Le Goater <clg@kaod.org>
> *Date: *Saturday, August 28, 2021 at 1:28 AM
> *To: *Peter Delevoryas <pdel@fb.com>
> *Cc: *joel@jms.id.au <joel@jms.id.au>, qemu-devel@nongnu.org
> <qemu-devel@nongnu.org>, qemu-arm@nongnu.org <qemu-arm@nongnu.org>
> *Subject: *Re: [PATCH 3/5] hw/arm/aspeed: Add fuji machine type
> 
> On 8/27/21 11:04 PM, pdel@fb.com wrote:
>> From: Peter Delevoryas <pdel@fb.com>
>>
>> Fuji uses the AST2600, so it's very similar to `ast2600-evb`, but it has
>> a few slight differences, such as using UART1 for the serial console.
> 
> Do we have a public DTS for this board ?
> 
> Thanks,
> 
> C.
> 
>>
>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>> ---
>>  hw/arm/aspeed.c | 13 +++++++++++++
>>  1 file changed, 13 insertions(+)
>>
>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>> index ff53d12395..d2eb516a1d 100644
>> --- a/hw/arm/aspeed.c
>> +++ b/hw/arm/aspeed.c
>> @@ -1029,6 +1029,15 @@ static void
> aspeed_machine_rainier_class_init(ObjectClass *oc, void *data)
>>          aspeed_soc_num_cpus(amc->soc_name);
>>  };
>> 
>> +static void aspeed_machine_fuji_class_init(ObjectClass *oc, void *data)
>> +{
>> +    MachineClass *mc = MACHINE_CLASS(oc);
>> +    AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
>> +
>> +    mc->desc = "Facebook Fuji BMC (Aspeed AST2600, Cortex-A7)";
>> +    amc->serial_dev = ASPEED_DEV_UART1;
>> +}
>> +
>>  static const TypeInfo aspeed_machine_types[] = {
>>      {
>>          .name          = MACHINE_TYPE_NAME("palmetto-bmc"),
>> @@ -1078,6 +1087,10 @@ static const TypeInfo aspeed_machine_types[] = {
>>          .name          = MACHINE_TYPE_NAME("rainier-bmc"),
>>          .parent        = TYPE_ASPEED_MACHINE,
>>          .class_init    = aspeed_machine_rainier_class_init,
>> +    }, {
>> +        .name          = MACHINE_TYPE_NAME("fuji"),
>> +        .parent        = MACHINE_TYPE_NAME("ast2600-evb"),
>> +        .class_init    = aspeed_machine_fuji_class_init,
>>      }, {
>>          .name          = TYPE_ASPEED_MACHINE,
>>          .parent        = TYPE_MACHINE,
>>
> 

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

[Cédric Le Goater]

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

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

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

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

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

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

Cheers,

C.

Re: [PATCH 3/5] hw/arm/aspeed: Add fuji machine type

[Peter Delevoryas]

> On Aug 31, 2021, at 9:00 AM, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> 
> On 8/28/21 6:00 PM, Peter Delevoryas wrote:
>> Actually yes! I should have included a link to it:
>> https://github.com/facebook/openbmc-uboot/blob/openbmc/helium/v2019.04/arch/arm/dts/aspeed-bmc-facebook-fuji.dts
> 
> (On technical lists, it's best to avoid top-posting, and to
> instead reply inline to make the conversation easier to follow).
> 
> Can you add a test for your board please? See examples
> in tests/acceptance/boot_linux_console.py:
> - test_arm_ast2500_romulus_openbmc_v2_9_0
> - test_arm_ast2600_debian

(Sorry about that, I’ll reply inline now)

Yeah absolutely, thanks for pointing those out: I actually resubmitted [PATCH 5/5]
separately already, should I just submit it again as [PATCH v2] with the tests,
or would you want to reply to that patch? I’ll include a cover letter that has a
diff-stat of the test changes regardless I suppose, so it’s probably not necessary
right?

Thanks,
Peter

> 
>> *From: *Cédric Le Goater <clg@kaod.org>
>> *Date: *Saturday, August 28, 2021 at 1:28 AM
>> *To: *Peter Delevoryas <pdel@fb.com>
>> *Cc: *joel@jms.id.au <joel@jms.id.au>, qemu-devel@nongnu.org
>> <qemu-devel@nongnu.org>, qemu-arm@nongnu.org <qemu-arm@nongnu.org>
>> *Subject: *Re: [PATCH 3/5] hw/arm/aspeed: Add fuji machine type
>> 
>> On 8/27/21 11:04 PM, pdel@fb.com wrote:
>>> From: Peter Delevoryas <pdel@fb.com>
>>> 
>>> Fuji uses the AST2600, so it's very similar to `ast2600-evb`, but it has
>>> a few slight differences, such as using UART1 for the serial console.
>> 
>> Do we have a public DTS for this board ?
>> 
>> Thanks,
>> 
>> C.
>> 
>>> 
>>> Signed-off-by: Peter Delevoryas <pdel@fb.com>
>>> ---
>>>   hw/arm/aspeed.c | 13 +++++++++++++
>>>   1 file changed, 13 insertions(+)
>>> 
>>> diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c
>>> index ff53d12395..d2eb516a1d 100644
>>> --- a/hw/arm/aspeed.c
>>> +++ b/hw/arm/aspeed.c
>>> @@ -1029,6 +1029,15 @@ static void
>> aspeed_machine_rainier_class_init(ObjectClass *oc, void *data)
>>>           aspeed_soc_num_cpus(amc->soc_name);
>>>   };
>>>  
>>> +static void aspeed_machine_fuji_class_init(ObjectClass *oc, void *data)
>>> +{
>>> +    MachineClass *mc = MACHINE_CLASS(oc);
>>> +    AspeedMachineClass *amc = ASPEED_MACHINE_CLASS(oc);
>>> +
>>> +    mc->desc = "Facebook Fuji BMC (Aspeed AST2600, Cortex-A7)";
>>> +    amc->serial_dev = ASPEED_DEV_UART1;
>>> +}
>>> +
>>>   static const TypeInfo aspeed_machine_types[] = {
>>>       {
>>>           .name          = MACHINE_TYPE_NAME("palmetto-bmc"),
>>> @@ -1078,6 +1087,10 @@ static const TypeInfo aspeed_machine_types[] = {
>>>           .name          = MACHINE_TYPE_NAME("rainier-bmc"),
>>>           .parent        = TYPE_ASPEED_MACHINE,
>>>           .class_init    = aspeed_machine_rainier_class_init,
>>> +    }, {
>>> +        .name          = MACHINE_TYPE_NAME("fuji"),
>>> +        .parent        = MACHINE_TYPE_NAME("ast2600-evb"),
>>> +        .class_init    = aspeed_machine_fuji_class_init,
>>>       }, {
>>>           .name          = TYPE_ASPEED_MACHINE,
>>>           .parent        = TYPE_MACHINE,
>>> 
>> 
>