| Message ID | 20240205091415.935686-2-jamin_lin@aspeedtech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | uart base and hardcode boot address 0 | expand |
Hello Jamin, On 2/5/24 10:14, Jamin Lin wrote: > According to the design of ASPEED SOCS, the uart controller > is 1 base for ast10x0, ast2600, ast2500 and ast2400. > > However, the uart controller is 0 base for ast2700. > To support uart controller both 0 and 1 base, > adds uasrt_bases parameter in AspeedSoCClass > and set the default uart controller 1 base > for ast10x0, astt2600, ast2500 and ast2400. The board definition can set 'amc->uart_default' to choose a different default serial port for the console, or use the "bmc-console" machine option . Isn't it enough ? May be I am misunderstanding the need. To clarify, ASPEED_DEV_UART1 is in the first serial port on the boards. I think we chose to start the indexing at 1 because the Aspeed QEMU modeling began first with the UART model (console) and for simplicity, we copied the definitions of the device tree from Linux : serial0 = &uart1; serial1 = &uart2; serial2 = &uart3; serial3 = &uart4; serial4 = &uart5; serial5 = &vuart; We replicated this indexing starting at 1 to nearly all device models : ASPEED_DEV_UART1 - 13 ASPEED_DEV_SPI1 -2 ASPEED_DEV_EHCI1 -2 ASPEED_DEV_TIMER1 - 8 ASPEED_DEV_ETH1 -4 ASPEED_DEV_MII1 - 4 ASPEED_DEV_JTAG0 - 1 <--- !! ASPEED_DEV_FSI1 - 2 I don't know what would be ASPEED_DEV_UART0 in this context. May be you could send a simplified AST2700 SoC model with definitions of a minimum address space and IRQ space ? Or you could change the indexing to start at 0 if you prefer. Just be careful with the aspeed_set/get_bmc_console routines it you choose to. Thanks, C. > From datasheet description > ast2700: > Base Address of UART0 = 0x14c33000 > ast1030: > Base Address of UART1 = 0x7e783000 > ast2600: > Base Address of UART1 = 0x1E78 3000 > ast2500: > Base Address of UART1 = 0x1E78 3000 > > Signed-off-by: Troy Lee <troy_lee@aspeedtech.com> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> > --- > hw/arm/aspeed.c | 8 +++++--- > hw/arm/aspeed_ast10x0.c | 1 + > hw/arm/aspeed_ast2400.c | 2 ++ > hw/arm/aspeed_ast2600.c | 1 + > hw/arm/aspeed_soc_common.c | 4 ++-- > include/hw/arm/aspeed_soc.h | 1 + > 6 files changed, 12 insertions(+), 5 deletions(-) > > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c > index 09b1e823ba..218b81298e 100644 > --- a/hw/arm/aspeed.c > +++ b/hw/arm/aspeed.c > @@ -342,7 +342,7 @@ static void connect_serial_hds_to_uarts(AspeedMachineState *bmc) > int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default; > > aspeed_soc_uart_set_chr(s, uart_chosen, serial_hd(0)); > - for (int i = 1, uart = ASPEED_DEV_UART1; i < sc->uarts_num; i++, uart++) { > + for (int i = 1, uart = sc->uarts_base; i < sc->uarts_num; i++, uart++) { > if (uart == uart_chosen) { > continue; > } > @@ -1092,9 +1092,11 @@ static char *aspeed_get_bmc_console(Object *obj, Error **errp) > { > AspeedMachineState *bmc = ASPEED_MACHINE(obj); > AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc); > + AspeedSoCClass *sc = ASPEED_SOC_CLASS(obj); > + > int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default; > > - return g_strdup_printf("uart%d", uart_chosen - ASPEED_DEV_UART1 + 1); > + return g_strdup_printf("uart%d", uart_chosen - sc->uarts_base + 1); > } > > static void aspeed_set_bmc_console(Object *obj, const char *value, Error **errp) > @@ -1114,7 +1116,7 @@ static void aspeed_set_bmc_console(Object *obj, const char *value, Error **errp) > error_setg(errp, "\"uart\" should be in range [1 - %d]", sc->uarts_num); > return; > } > - bmc->uart_chosen = ASPEED_DEV_UART1 + val - 1; > + bmc->uart_chosen = sc->uarts_base + val - 1; > } > > static void aspeed_machine_class_props_init(ObjectClass *oc) > diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c > index c3b5116a6a..2634e0f654 100644 > --- a/hw/arm/aspeed_ast10x0.c > +++ b/hw/arm/aspeed_ast10x0.c > @@ -436,6 +436,7 @@ static void aspeed_soc_ast1030_class_init(ObjectClass *klass, void *data) > sc->wdts_num = 4; > sc->macs_num = 1; > sc->uarts_num = 13; > + sc->uarts_base = ASPEED_DEV_UART1; > sc->irqmap = aspeed_soc_ast1030_irqmap; > sc->memmap = aspeed_soc_ast1030_memmap; > sc->num_cpus = 1; > diff --git a/hw/arm/aspeed_ast2400.c b/hw/arm/aspeed_ast2400.c > index 8829561bb6..95da85fee0 100644 > --- a/hw/arm/aspeed_ast2400.c > +++ b/hw/arm/aspeed_ast2400.c > @@ -523,6 +523,7 @@ static void aspeed_soc_ast2400_class_init(ObjectClass *oc, void *data) > sc->wdts_num = 2; > sc->macs_num = 2; > sc->uarts_num = 5; > + sc->uarts_base = ASPEED_DEV_UART1; > sc->irqmap = aspeed_soc_ast2400_irqmap; > sc->memmap = aspeed_soc_ast2400_memmap; > sc->num_cpus = 1; > @@ -551,6 +552,7 @@ static void aspeed_soc_ast2500_class_init(ObjectClass *oc, void *data) > sc->wdts_num = 3; > sc->macs_num = 2; > sc->uarts_num = 5; > + sc->uarts_base = ASPEED_DEV_UART1; > sc->irqmap = aspeed_soc_ast2500_irqmap; > sc->memmap = aspeed_soc_ast2500_memmap; > sc->num_cpus = 1; > diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c > index 4ee32ea99d..f74561ecdc 100644 > --- a/hw/arm/aspeed_ast2600.c > +++ b/hw/arm/aspeed_ast2600.c > @@ -666,6 +666,7 @@ static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data) > sc->wdts_num = 4; > sc->macs_num = 4; > sc->uarts_num = 13; > + sc->uarts_base = ASPEED_DEV_UART1; > sc->irqmap = aspeed_soc_ast2600_irqmap; > sc->memmap = aspeed_soc_ast2600_memmap; > sc->num_cpus = 2; > diff --git a/hw/arm/aspeed_soc_common.c b/hw/arm/aspeed_soc_common.c > index 123a0c432c..3963436c3a 100644 > --- a/hw/arm/aspeed_soc_common.c > +++ b/hw/arm/aspeed_soc_common.c > @@ -36,7 +36,7 @@ bool aspeed_soc_uart_realize(AspeedSoCState *s, Error **errp) > AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); > SerialMM *smm; > > - for (int i = 0, uart = ASPEED_DEV_UART1; i < sc->uarts_num; i++, uart++) { > + for (int i = 0, uart = sc->uarts_base; i < sc->uarts_num; i++, uart++) { > smm = &s->uart[i]; > > /* Chardev property is set by the machine. */ > @@ -58,7 +58,7 @@ bool aspeed_soc_uart_realize(AspeedSoCState *s, Error **errp) > void aspeed_soc_uart_set_chr(AspeedSoCState *s, int dev, Chardev *chr) > { > AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); > - int i = dev - ASPEED_DEV_UART1; > + int i = dev - sc->uarts_base; > > g_assert(0 <= i && i < ARRAY_SIZE(s->uart) && i < sc->uarts_num); > qdev_prop_set_chr(DEVICE(&s->uart[i]), "chardev", chr); > diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h > index 9d0af84a8c..ce2bb51682 100644 > --- a/include/hw/arm/aspeed_soc.h > +++ b/include/hw/arm/aspeed_soc.h > @@ -140,6 +140,7 @@ struct AspeedSoCClass { > int wdts_num; > int macs_num; > int uarts_num; > + int uarts_base; > const int *irqmap; > const hwaddr *memmap; > uint32_t num_cpus;
On 5/2/24 11:46, Cédric Le Goater wrote: > Hello Jamin, > > On 2/5/24 10:14, Jamin Lin wrote: >> According to the design of ASPEED SOCS, the uart controller >> is 1 base for ast10x0, ast2600, ast2500 and ast2400. >> >> However, the uart controller is 0 base for ast2700. >> To support uart controller both 0 and 1 base, >> adds uasrt_bases parameter in AspeedSoCClass >> and set the default uart controller 1 base >> for ast10x0, astt2600, ast2500 and ast2400. > > The board definition can set 'amc->uart_default' to choose a different > default serial port for the console, or use the "bmc-console" machine > option . Isn't it enough ? May be I am misunderstanding the need. > > To clarify, > > ASPEED_DEV_UART1 is in the first serial port on the boards. > > I think we chose to start the indexing at 1 because the Aspeed QEMU > modeling began first with the UART model (console) and for simplicity, > we copied the definitions of the device tree from Linux : > > serial0 = &uart1; > serial1 = &uart2; > serial2 = &uart3; > serial3 = &uart4; > serial4 = &uart5; > serial5 = &vuart; > > We replicated this indexing starting at 1 to nearly all device models : > > ASPEED_DEV_UART1 - 13 > ASPEED_DEV_SPI1 -2 > ASPEED_DEV_EHCI1 -2 > ASPEED_DEV_TIMER1 - 8 > ASPEED_DEV_ETH1 -4 > ASPEED_DEV_MII1 - 4 > ASPEED_DEV_JTAG0 - 1 <--- !! > ASPEED_DEV_FSI1 - 2 > > I don't know what would be ASPEED_DEV_UART0 in this context. > > May be you could send a simplified AST2700 SoC model with definitions > of a minimum address space and IRQ space ? Looking at TF-A definitions, https://github.com/ARM-software/arm-trusted-firmware/commit/85f199b77447 #define UART_BASE U(0x14c33000) #define UART12_BASE (UART_BASE + 0xb00) #define CONSOLE_UART_BASE UART12_BASE As Cédric described, we have TF-A UART12_BASE -> QEMU ASPEED_DEV_UART13.
On 2/5/24 11:46, Cédric Le Goater wrote: > Hello Jamin, > > On 2/5/24 10:14, Jamin Lin wrote: >> According to the design of ASPEED SOCS, the uart controller >> is 1 base for ast10x0, ast2600, ast2500 and ast2400. >> >> However, the uart controller is 0 base for ast2700. >> To support uart controller both 0 and 1 base, >> adds uasrt_bases parameter in AspeedSoCClass >> and set the default uart controller 1 base >> for ast10x0, astt2600, ast2500 and ast2400. > > The board definition can set 'amc->uart_default' to choose a different > default serial port for the console, or use the "bmc-console" machine > option . Isn't it enough ? May be I am misunderstanding the need. > > To clarify, > > ASPEED_DEV_UART1 is in the first serial port on the boards. > > I think we chose to start the indexing at 1 because the Aspeed QEMU > modeling began first with the UART model (console) and for simplicity, > we copied the definitions of the device tree from Linux : > > serial0 = &uart1; > serial1 = &uart2; > serial2 = &uart3; > serial3 = &uart4; > serial4 = &uart5; > serial5 = &vuart; The uart definitions on the AST2700 are different : https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi serial0 = &uart0; serial1 = &uart1; serial2 = &uart2; serial3 = &uart3; serial4 = &uart4; serial5 = &uart5; serial6 = &uart6; serial7 = &uart7; serial8 = &uart8; ... I think the names in the DT (and consequently in the QEMU models) follow the IP names in the datasheet. I don't think we care in QEMU, so I would be inclined to change the indexing of the device names in QEMU and start at 0, which would introduce a discrepancy for the AST2400, AST2600, AST2600 SoC. Let's see what the other maintainers have to say. Thanks, C.
> -----Original Message----- > From: Philippe Mathieu-Daudé <philmd@linaro.org> > Sent: Monday, February 5, 2024 9:16 PM > To: Cédric Le Goater <clg@kaod.org>; Jamin Lin <jamin_lin@aspeedtech.com>; > Peter Maydell <peter.maydell@linaro.org>; Andrew Jeffery > <andrew@codeconstruct.com.au>; Joel Stanley <joel@jms.id.au>; open > list:ASPEED BMCs <qemu-arm@nongnu.org>; open list:All patches CC here > <qemu-devel@nongnu.org> > Cc: Troy Lee <troy_lee@aspeedtech.com> > Subject: Re: [PATCH v0 1/2] aspeed: support uart controller both 0 and 1 base > > On 5/2/24 11:46, Cédric Le Goater wrote: > > Hello Jamin, > > > > On 2/5/24 10:14, Jamin Lin wrote: > >> According to the design of ASPEED SOCS, the uart controller is 1 base > >> for ast10x0, ast2600, ast2500 and ast2400. > >> > >> However, the uart controller is 0 base for ast2700. > >> To support uart controller both 0 and 1 base, adds uasrt_bases > >> parameter in AspeedSoCClass and set the default uart controller 1 > >> base for ast10x0, astt2600, ast2500 and ast2400. > > > > The board definition can set 'amc->uart_default' to choose a different > > default serial port for the console, or use the "bmc-console" machine > > option . Isn't it enough ? May be I am misunderstanding the need. > > > > To clarify, > > > > ASPEED_DEV_UART1 is in the first serial port on the boards. > > > > I think we chose to start the indexing at 1 because the Aspeed QEMU > > modeling began first with the UART model (console) and for simplicity, > > we copied the definitions of the device tree from Linux : > > > > serial0 = &uart1; > > serial1 = &uart2; > > serial2 = &uart3; > > serial3 = &uart4; > > serial4 = &uart5; > > serial5 = &vuart; > > > > We replicated this indexing starting at 1 to nearly all device models : > > > > ASPEED_DEV_UART1 - 13 > > ASPEED_DEV_SPI1 -2 > > ASPEED_DEV_EHCI1 -2 > > ASPEED_DEV_TIMER1 - 8 > > ASPEED_DEV_ETH1 -4 > > ASPEED_DEV_MII1 - 4 > > ASPEED_DEV_JTAG0 - 1 <--- !! > > ASPEED_DEV_FSI1 - 2 > > > > I don't know what would be ASPEED_DEV_UART0 in this context. > > > > May be you could send a simplified AST2700 SoC model with definitions > > of a minimum address space and IRQ space ? > > Looking at TF-A definitions, > https://github.com/ARM-software/arm-trusted-firmware/commit/85f199b774 > 47 > > #define UART_BASE U(0x14c33000) > #define UART12_BASE (UART_BASE + 0xb00) > #define CONSOLE_UART_BASE UART12_BASE > > As Cédric described, we have TF-A UART12_BASE -> QEMU > ASPEED_DEV_UART13. As Cédric described, the UART definitions on the AST2700 are different : https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi serial0 = &uart0; serial1 = &uart1; serial2 = &uart2; serial3 = &uart3; serial4 = &uart4; serial5 = &uart5; serial6 = &uart6; serial7 = &uart7; serial8 = &uart8; According to the current design of ASPEED QEMU UART model, it used "1" base device name and Follow the IP names in the datasheet Take ast2600 for example: static const hwaddr aspeed_soc_ast2600_memmap[] = { [ASPEED_DEV_UART1] = 0x1E783000, } AST2600 datashee description: Base Address of UART1 = 0x1E78 3000 Base Address of UART2 = 0x1E78 D000 Base Address of UART3 = 0x1E78 E000 Base Address of UART4 = 0x1E78 F000 Base Address of UART5 = 0x1E78 4000 Base Address of UART6 = 0x1E79 0000 However, device name of uart controller had been changed to "0" base for ast2700. If we want to control uart0(ASPEED_DEV_UART1), we should set the memory map as following, static const hwaddr aspeed_soc_ast2700_memmap[] = { [ASPEED_DEV_UART1] = 0X14C33000, [ASPEED_DEV_UART2] = 0X14C33100, [ASPEED_DEV_UART3] = 0X14C33200, [ASPEED_DEV_UART4] = 0X14C33300, [ASPEED_DEV_UART5] = 0X12C1A000, } AST2700 datasheet description: AST2700 integrate 13 sets of UART. Base Address of UART0 = 0x14C3_3000 Base Address of UART1 = 0x14C3_3100 Base Address of UART2 = 0x14C3_3200 Base Address of UART3 = 0x14C3_3300 Base Address of UART4 = 0x12C1_A000 Base Address of UART5 = 0x14C3_3400 Base Address of UART6 = 0x14C3_3500 Base Address of UART7 = 0x14C3_3600 Base Address of UART8 = 0x14C3_3700 Base Address of UART9 = 0x14C3_3800 Base Address of UART10 = 0x14C3_3900 Base Address of UART11 = 0x14C3_3A00 Base Address of UART12 = 0x14C3_3B00 As you said, uart12 mapped ASPEED_DEV_UART13. The device naming will confuse users because the device name in qemu mismatch with ast2700 datasheet. That way why we want to add ASPEED_DEV_UART0 and set the memory map of AST2700 as following. static const hwaddr aspeed_soc_ast2700_memmap[] = { [ASPEED_DEV_UART0] = 0X14C33000, [ASPEED_DEV_UART1] = 0X14C33100, [ASPEED_DEV_UART2] = 0X14C33200, [ASPEED_DEV_UART3] = 0X14C33300, [ASPEED_DEV_UART4] = 0X12C1A000, [ASPEED_DEV_UART5] = 0X14C33400, [ASPEED_DEV_UART6] = 0X14C33500, [ASPEED_DEV_UART7] = 0X14C33600, [ASPEED_DEV_UART8] = 0X14C33700, [ASPEED_DEV_UART9] = 0X14C33800, [ASPEED_DEV_UART10] = 0X14C33900, [ASPEED_DEV_UART11] = 0X14C33A00, [ASPEED_DEV_UART12] = 0X14C33B00, } Thanks-Jamin
> -----Original Message----- > The uart definitions on the AST2700 are different : > > > https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm > 64/boot/dts/aspeed/aspeed-g7.dtsi > > serial0 = &uart0; > serial1 = &uart1; > serial2 = &uart2; > serial3 = &uart3; > serial4 = &uart4; > serial5 = &uart5; > serial6 = &uart6; > serial7 = &uart7; > serial8 = &uart8; > ... > > I think the names in the DT (and consequently in the QEMU models) follow the > IP names in the datasheet. > > I don't think we care in QEMU, so I would be inclined to change the indexing of > the device names in QEMU and start at 0, which would introduce a > discrepancy for the AST2400, AST2600, AST2600 SoC. > > Let's see what the other maintainers have to say. > > Thanks, > > C. Hi Cedric, Did you mean to change the naming of uart device to 0 base for all ASPEED SOCs? If yes, it seems we need to do the following changes. 1. add ASPEED_DEV_UART0 in aspeed_soc.h 2. Re-defined uart memory map for ast2600, ast10x0, ast2500 and ast2400(uart0 -> ASPEED_DEV_UART0) Take ast2600 for example: static const hwaddr aspeed_soc_ast2600_memmap[] = { [ASPEED_DEV_UART1] = 0x1E783000, ---> [ASPEED_DEV_UART0] [ASPEED_DEV_UART2] = 0x1E78D000, ---> [ASPEED_DEV_UART1] [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_UART12] }; If no, could you please descript it more detail? So, I can change it and re-send this patch series. By the way, I will send a new patch series to support AST2700 in two weeks. We encountered GIC issues. It seems that QEMU support GIC v3 but SPI did not support, yet. https://github.com/qemu/qemu/blob/master/hw/intc/arm_gicv3_dist.c#L383 https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi#L229 It think that we can discuss it in a new AST2700 patch series. Thanks-Jamin
[ ... ] > As you said, uart12 mapped ASPEED_DEV_UART13. > The device naming will confuse users because the device name in qemu mismatch with ast2700 datasheet. > > That way why we want to add ASPEED_DEV_UART0 and set the memory map of AST2700 as following. > static const hwaddr aspeed_soc_ast2700_memmap[] = { > [ASPEED_DEV_UART0] = 0X14C33000, > [ASPEED_DEV_UART1] = 0X14C33100, > [ASPEED_DEV_UART2] = 0X14C33200, > [ASPEED_DEV_UART3] = 0X14C33300, > [ASPEED_DEV_UART4] = 0X12C1A000, > [ASPEED_DEV_UART5] = 0X14C33400, > [ASPEED_DEV_UART6] = 0X14C33500, > [ASPEED_DEV_UART7] = 0X14C33600, > [ASPEED_DEV_UART8] = 0X14C33700, > [ASPEED_DEV_UART9] = 0X14C33800, > [ASPEED_DEV_UART10] = 0X14C33900, > [ASPEED_DEV_UART11] = 0X14C33A00, > [ASPEED_DEV_UART12] = 0X14C33B00, > So we would prefer to keep the QEMU IP names in sync with the datasheet, and in that case your proposal makes sense. A have a few comments that I will make on the patch. Thanks, C.
On 2/6/24 04:29, Jamin Lin wrote: >> -----Original Message----- >> The uart definitions on the AST2700 are different : >> >> >> https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm >> 64/boot/dts/aspeed/aspeed-g7.dtsi >> >> serial0 = &uart0; >> serial1 = &uart1; >> serial2 = &uart2; >> serial3 = &uart3; >> serial4 = &uart4; >> serial5 = &uart5; >> serial6 = &uart6; >> serial7 = &uart7; >> serial8 = &uart8; >> ... >> >> I think the names in the DT (and consequently in the QEMU models) follow the >> IP names in the datasheet. >> >> I don't think we care in QEMU, so I would be inclined to change the indexing of >> the device names in QEMU and start at 0, which would introduce a >> discrepancy for the AST2400, AST2600, AST2600 SoC. >> >> Let's see what the other maintainers have to say. >> >> Thanks, >> >> C. > Hi Cedric, > > Did you mean to change the naming of uart device to 0 base for all ASPEED SOCs? > If yes, it seems we need to do the following changes. > 1. add ASPEED_DEV_UART0 in aspeed_soc.h > 2. Re-defined uart memory map for ast2600, ast10x0, ast2500 and ast2400(uart0 -> ASPEED_DEV_UART0) > Take ast2600 for example: > static const hwaddr aspeed_soc_ast2600_memmap[] = { > [ASPEED_DEV_UART1] = 0x1E783000, ---> [ASPEED_DEV_UART0] > [ASPEED_DEV_UART2] = 0x1E78D000, ---> [ASPEED_DEV_UART1] > [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_UART12] > }; > If no, could you please descript it more detail? So, I can change it and re-send this patch series. Let's keep the datasheet names. I had forgotten the reason initially and from an HW POV it makes sense to keep them in sync. I will add some more comments to the patch. > By the way, I will send a new patch series to support AST2700 in two weeks. > We encountered GIC issues. It seems that QEMU support GIC v3 but SPI did not support, yet. > > https://github.com/qemu/qemu/blob/master/hw/intc/arm_gicv3_dist.c#L383 > https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/arm64/boot/dts/aspeed/aspeed-g7.dtsi#L229 If you did any hacks or workarounds in the QEMU models, please keep them separate from the other patches so that we can discuss. > It think that we can discuss it in a new AST2700 patch series. Sure. Thanks, C.
Hello Jmain, On 2/5/24 10:14, Jamin Lin wrote: > According to the design of ASPEED SOCS, the uart controller > is 1 base for ast10x0, ast2600, ast2500 and ast2400. Please rephrase saying somehting : the Aspeed datasheet refers to the UART controllers as UART1 - UART13 for the ast10x0, ast2600, ast2500 and ast2400 SoCs and the Aspeed ast2700 introduces an UART0. To keep the naming in the QEMU models in sync with the datasheet, let's introduce a new UART0 device name and do the required adjustements, etc ... > However, the uart controller is 0 base for ast2700. > To support uart controller both 0 and 1 base, > adds uasrt_bases parameter in AspeedSoCClass > and set the default uart controller 1 base > for ast10x0, astt2600, ast2500 and ast2400. > > From datasheet description > ast2700: > Base Address of UART0 = 0x14c33000 > ast1030: > Base Address of UART1 = 0x7e783000 > ast2600: > Base Address of UART1 = 0x1E78 3000 > ast2500: > Base Address of UART1 = 0x1E78 3000 We should also introduce ASPEED_DEV_UART0 enum. See below. > Signed-off-by: Troy Lee <troy_lee@aspeedtech.com> > Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com> > --- > hw/arm/aspeed.c | 8 +++++--- > hw/arm/aspeed_ast10x0.c | 1 + > hw/arm/aspeed_ast2400.c | 2 ++ > hw/arm/aspeed_ast2600.c | 1 + > hw/arm/aspeed_soc_common.c | 4 ++-- > include/hw/arm/aspeed_soc.h | 1 + > 6 files changed, 12 insertions(+), 5 deletions(-) > > diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c > index 09b1e823ba..218b81298e 100644 > --- a/hw/arm/aspeed.c > +++ b/hw/arm/aspeed.c > @@ -342,7 +342,7 @@ static void connect_serial_hds_to_uarts(AspeedMachineState *bmc) > int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default; > > aspeed_soc_uart_set_chr(s, uart_chosen, serial_hd(0)); > - for (int i = 1, uart = ASPEED_DEV_UART1; i < sc->uarts_num; i++, uart++) { > + for (int i = 1, uart = sc->uarts_base; i < sc->uarts_num; i++, uart++) { > if (uart == uart_chosen) { > continue; > } > @@ -1092,9 +1092,11 @@ static char *aspeed_get_bmc_console(Object *obj, Error **errp) > { > AspeedMachineState *bmc = ASPEED_MACHINE(obj); > AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc); > + AspeedSoCClass *sc = ASPEED_SOC_CLASS(obj); > + > int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default; > > - return g_strdup_printf("uart%d", uart_chosen - ASPEED_DEV_UART1 + 1); > + return g_strdup_printf("uart%d", uart_chosen - sc->uarts_base + 1); Wwe didn't have a ASPEED_DEV_UART0 at the time. The calculation above should be replaced with : "uart_chosen - ASPEED_DEV_UART0" > } > > static void aspeed_set_bmc_console(Object *obj, const char *value, Error **errp) > @@ -1114,7 +1116,7 @@ static void aspeed_set_bmc_console(Object *obj, const char *value, Error **errp) > error_setg(errp, "\"uart\" should be in range [1 - %d]", sc->uarts_num); The range in the reported error above needs a fix. It's not "1" anymore but "sc->uarts_base - ASPEED_DEV_UART0". Same for the test : if (val < 1 || val > sc->uarts_num) { > return; > } > - bmc->uart_chosen = ASPEED_DEV_UART1 + val - 1; > + bmc->uart_chosen = sc->uarts_base + val - 1; Should be ASPEED_DEV_UART0 + val. Thanks, C. > } > > static void aspeed_machine_class_props_init(ObjectClass *oc) > diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c > index c3b5116a6a..2634e0f654 100644 > --- a/hw/arm/aspeed_ast10x0.c > +++ b/hw/arm/aspeed_ast10x0.c > @@ -436,6 +436,7 @@ static void aspeed_soc_ast1030_class_init(ObjectClass *klass, void *data) > sc->wdts_num = 4; > sc->macs_num = 1; > sc->uarts_num = 13; > + sc->uarts_base = ASPEED_DEV_UART1; > sc->irqmap = aspeed_soc_ast1030_irqmap; > sc->memmap = aspeed_soc_ast1030_memmap; > sc->num_cpus = 1; > diff --git a/hw/arm/aspeed_ast2400.c b/hw/arm/aspeed_ast2400.c > index 8829561bb6..95da85fee0 100644 > --- a/hw/arm/aspeed_ast2400.c > +++ b/hw/arm/aspeed_ast2400.c > @@ -523,6 +523,7 @@ static void aspeed_soc_ast2400_class_init(ObjectClass *oc, void *data) > sc->wdts_num = 2; > sc->macs_num = 2; > sc->uarts_num = 5; > + sc->uarts_base = ASPEED_DEV_UART1; > sc->irqmap = aspeed_soc_ast2400_irqmap; > sc->memmap = aspeed_soc_ast2400_memmap; > sc->num_cpus = 1; > @@ -551,6 +552,7 @@ static void aspeed_soc_ast2500_class_init(ObjectClass *oc, void *data) > sc->wdts_num = 3; > sc->macs_num = 2; > sc->uarts_num = 5; > + sc->uarts_base = ASPEED_DEV_UART1; > sc->irqmap = aspeed_soc_ast2500_irqmap; > sc->memmap = aspeed_soc_ast2500_memmap; > sc->num_cpus = 1; > diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c > index 4ee32ea99d..f74561ecdc 100644 > --- a/hw/arm/aspeed_ast2600.c > +++ b/hw/arm/aspeed_ast2600.c > @@ -666,6 +666,7 @@ static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data) > sc->wdts_num = 4; > sc->macs_num = 4; > sc->uarts_num = 13; > + sc->uarts_base = ASPEED_DEV_UART1; > sc->irqmap = aspeed_soc_ast2600_irqmap; > sc->memmap = aspeed_soc_ast2600_memmap; > sc->num_cpus = 2; > diff --git a/hw/arm/aspeed_soc_common.c b/hw/arm/aspeed_soc_common.c > index 123a0c432c..3963436c3a 100644 > --- a/hw/arm/aspeed_soc_common.c > +++ b/hw/arm/aspeed_soc_common.c > @@ -36,7 +36,7 @@ bool aspeed_soc_uart_realize(AspeedSoCState *s, Error **errp) > AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); > SerialMM *smm; > > - for (int i = 0, uart = ASPEED_DEV_UART1; i < sc->uarts_num; i++, uart++) { > + for (int i = 0, uart = sc->uarts_base; i < sc->uarts_num; i++, uart++) { > smm = &s->uart[i]; > > /* Chardev property is set by the machine. */ > @@ -58,7 +58,7 @@ bool aspeed_soc_uart_realize(AspeedSoCState *s, Error **errp) > void aspeed_soc_uart_set_chr(AspeedSoCState *s, int dev, Chardev *chr) > { > AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); > - int i = dev - ASPEED_DEV_UART1; > + int i = dev - sc->uarts_base; > > g_assert(0 <= i && i < ARRAY_SIZE(s->uart) && i < sc->uarts_num); > qdev_prop_set_chr(DEVICE(&s->uart[i]), "chardev", chr); > diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h > index 9d0af84a8c..ce2bb51682 100644 > --- a/include/hw/arm/aspeed_soc.h > +++ b/include/hw/arm/aspeed_soc.h > @@ -140,6 +140,7 @@ struct AspeedSoCClass { > int wdts_num; > int macs_num; > int uarts_num; > + int uarts_base; > const int *irqmap; > const hwaddr *memmap; > uint32_t num_cpus;
> -----Original Message----- > From: Cédric Le Goater <clg@kaod.org> > Sent: Wednesday, February 7, 2024 1:00 AM > To: Jamin Lin <jamin_lin@aspeedtech.com>; Peter Maydell > <peter.maydell@linaro.org>; Andrew Jeffery <andrew@codeconstruct.com.au>; > Joel Stanley <joel@jms.id.au>; open list:ASPEED BMCs > <qemu-arm@nongnu.org>; open list:All patches CC here > <qemu-devel@nongnu.org> > Cc: Troy Lee <troy_lee@aspeedtech.com> > Subject: Re: [PATCH v0 1/2] aspeed: support uart controller both 0 and 1 base > > On 2/6/24 04:29, Jamin Lin wrote: > >> -----Original Message----- > >> The uart definitions on the AST2700 are different : > >> > >> > >> https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/ > >> arm > >> 64/boot/dts/aspeed/aspeed-g7.dtsi > >> > >> serial0 = &uart0; > >> serial1 = &uart1; > >> serial2 = &uart2; > >> serial3 = &uart3; > >> serial4 = &uart4; > >> serial5 = &uart5; > >> serial6 = &uart6; > >> serial7 = &uart7; > >> serial8 = &uart8; > >> ... > >> > >> I think the names in the DT (and consequently in the QEMU models) > >> follow the IP names in the datasheet. > >> > >> I don't think we care in QEMU, so I would be inclined to change the > >> indexing of the device names in QEMU and start at 0, which would > >> introduce a discrepancy for the AST2400, AST2600, AST2600 SoC. > >> > >> Let's see what the other maintainers have to say. > >> > >> Thanks, > >> > >> C. > > Hi Cedric, > > > > Did you mean to change the naming of uart device to 0 base for all ASPEED > SOCs? > > If yes, it seems we need to do the following changes. > > 1. add ASPEED_DEV_UART0 in aspeed_soc.h 2. Re-defined uart memory map > > for ast2600, ast10x0, ast2500 and ast2400(uart0 -> ASPEED_DEV_UART0) > > Take ast2600 for example: > > static const hwaddr aspeed_soc_ast2600_memmap[] = { > > [ASPEED_DEV_UART1] = 0x1E783000, ---> > [ASPEED_DEV_UART0] > > [ASPEED_DEV_UART2] = 0x1E78D000, ---> > [ASPEED_DEV_UART1] > > [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_UART12] > > }; > > If no, could you please descript it more detail? So, I can change it and re-send > this patch series. > > Let's keep the datasheet names. I had forgotten the reason initially and from > an HW POV it makes sense to keep them in sync. I will add some more > comments to the patch. > > > By the way, I will send a new patch series to support AST2700 in two weeks. > > We encountered GIC issues. It seems that QEMU support GIC v3 but SPI did > not support, yet. > > > > > https://github.com/qemu/qemu/blob/master/hw/intc/arm_gicv3_dist.c#L383 > > https://github.com/AspeedTech-BMC/linux/blob/aspeed-master-v6.6/arch/a > > rm64/boot/dts/aspeed/aspeed-g7.dtsi#L229 > > If you did any hacks or workarounds in the QEMU models, please keep them > separate from the other patches so that we can discuss. > Okay. Will do Thanks-Jamin > > It think that we can discuss it in a new AST2700 patch series. > Sure. > > Thanks, > > C. >
diff --git a/hw/arm/aspeed.c b/hw/arm/aspeed.c index 09b1e823ba..218b81298e 100644 --- a/hw/arm/aspeed.c +++ b/hw/arm/aspeed.c @@ -342,7 +342,7 @@ static void connect_serial_hds_to_uarts(AspeedMachineState *bmc) int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default; aspeed_soc_uart_set_chr(s, uart_chosen, serial_hd(0)); - for (int i = 1, uart = ASPEED_DEV_UART1; i < sc->uarts_num; i++, uart++) { + for (int i = 1, uart = sc->uarts_base; i < sc->uarts_num; i++, uart++) { if (uart == uart_chosen) { continue; } @@ -1092,9 +1092,11 @@ static char *aspeed_get_bmc_console(Object *obj, Error **errp) { AspeedMachineState *bmc = ASPEED_MACHINE(obj); AspeedMachineClass *amc = ASPEED_MACHINE_GET_CLASS(bmc); + AspeedSoCClass *sc = ASPEED_SOC_CLASS(obj); + int uart_chosen = bmc->uart_chosen ? bmc->uart_chosen : amc->uart_default; - return g_strdup_printf("uart%d", uart_chosen - ASPEED_DEV_UART1 + 1); + return g_strdup_printf("uart%d", uart_chosen - sc->uarts_base + 1); } static void aspeed_set_bmc_console(Object *obj, const char *value, Error **errp) @@ -1114,7 +1116,7 @@ static void aspeed_set_bmc_console(Object *obj, const char *value, Error **errp) error_setg(errp, "\"uart\" should be in range [1 - %d]", sc->uarts_num); return; } - bmc->uart_chosen = ASPEED_DEV_UART1 + val - 1; + bmc->uart_chosen = sc->uarts_base + val - 1; } static void aspeed_machine_class_props_init(ObjectClass *oc) diff --git a/hw/arm/aspeed_ast10x0.c b/hw/arm/aspeed_ast10x0.c index c3b5116a6a..2634e0f654 100644 --- a/hw/arm/aspeed_ast10x0.c +++ b/hw/arm/aspeed_ast10x0.c @@ -436,6 +436,7 @@ static void aspeed_soc_ast1030_class_init(ObjectClass *klass, void *data) sc->wdts_num = 4; sc->macs_num = 1; sc->uarts_num = 13; + sc->uarts_base = ASPEED_DEV_UART1; sc->irqmap = aspeed_soc_ast1030_irqmap; sc->memmap = aspeed_soc_ast1030_memmap; sc->num_cpus = 1; diff --git a/hw/arm/aspeed_ast2400.c b/hw/arm/aspeed_ast2400.c index 8829561bb6..95da85fee0 100644 --- a/hw/arm/aspeed_ast2400.c +++ b/hw/arm/aspeed_ast2400.c @@ -523,6 +523,7 @@ static void aspeed_soc_ast2400_class_init(ObjectClass *oc, void *data) sc->wdts_num = 2; sc->macs_num = 2; sc->uarts_num = 5; + sc->uarts_base = ASPEED_DEV_UART1; sc->irqmap = aspeed_soc_ast2400_irqmap; sc->memmap = aspeed_soc_ast2400_memmap; sc->num_cpus = 1; @@ -551,6 +552,7 @@ static void aspeed_soc_ast2500_class_init(ObjectClass *oc, void *data) sc->wdts_num = 3; sc->macs_num = 2; sc->uarts_num = 5; + sc->uarts_base = ASPEED_DEV_UART1; sc->irqmap = aspeed_soc_ast2500_irqmap; sc->memmap = aspeed_soc_ast2500_memmap; sc->num_cpus = 1; diff --git a/hw/arm/aspeed_ast2600.c b/hw/arm/aspeed_ast2600.c index 4ee32ea99d..f74561ecdc 100644 --- a/hw/arm/aspeed_ast2600.c +++ b/hw/arm/aspeed_ast2600.c @@ -666,6 +666,7 @@ static void aspeed_soc_ast2600_class_init(ObjectClass *oc, void *data) sc->wdts_num = 4; sc->macs_num = 4; sc->uarts_num = 13; + sc->uarts_base = ASPEED_DEV_UART1; sc->irqmap = aspeed_soc_ast2600_irqmap; sc->memmap = aspeed_soc_ast2600_memmap; sc->num_cpus = 2; diff --git a/hw/arm/aspeed_soc_common.c b/hw/arm/aspeed_soc_common.c index 123a0c432c..3963436c3a 100644 --- a/hw/arm/aspeed_soc_common.c +++ b/hw/arm/aspeed_soc_common.c @@ -36,7 +36,7 @@ bool aspeed_soc_uart_realize(AspeedSoCState *s, Error **errp) AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); SerialMM *smm; - for (int i = 0, uart = ASPEED_DEV_UART1; i < sc->uarts_num; i++, uart++) { + for (int i = 0, uart = sc->uarts_base; i < sc->uarts_num; i++, uart++) { smm = &s->uart[i]; /* Chardev property is set by the machine. */ @@ -58,7 +58,7 @@ bool aspeed_soc_uart_realize(AspeedSoCState *s, Error **errp) void aspeed_soc_uart_set_chr(AspeedSoCState *s, int dev, Chardev *chr) { AspeedSoCClass *sc = ASPEED_SOC_GET_CLASS(s); - int i = dev - ASPEED_DEV_UART1; + int i = dev - sc->uarts_base; g_assert(0 <= i && i < ARRAY_SIZE(s->uart) && i < sc->uarts_num); qdev_prop_set_chr(DEVICE(&s->uart[i]), "chardev", chr); diff --git a/include/hw/arm/aspeed_soc.h b/include/hw/arm/aspeed_soc.h index 9d0af84a8c..ce2bb51682 100644 --- a/include/hw/arm/aspeed_soc.h +++ b/include/hw/arm/aspeed_soc.h @@ -140,6 +140,7 @@ struct AspeedSoCClass { int wdts_num; int macs_num; int uarts_num; + int uarts_base; const int *irqmap; const hwaddr *memmap; uint32_t num_cpus;