RE: [PATCH v0 1/2] aspeed: support uart controller both 0 and 1 base

[Jamin Lin]

> -----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