Re: [PATCH v4 03/11] hw: allwinner-r40: Complete uart devices

[Niek Linnenbank <nieklinnenbank@gmail.com>]

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

Hi Qianfan,


On Wed, May 10, 2023 at 12:30 PM <qianfanguijin@163.com> wrote:

> From: qianfan Zhao <qianfanguijin@163.com>
>
> R40 has eight UARTs, support both 16450 and 16550 compatible modes.
>
> Signed-off-by: qianfan Zhao <qianfanguijin@163.com>
> ---
>  hw/arm/allwinner-r40.c         | 31 ++++++++++++++++++++++++++++---
>  include/hw/arm/allwinner-r40.h |  8 ++++++++
>  2 files changed, 36 insertions(+), 3 deletions(-)
>
> diff --git a/hw/arm/allwinner-r40.c b/hw/arm/allwinner-r40.c
> index 128c0ca470..537a90b23d 100644
> --- a/hw/arm/allwinner-r40.c
> +++ b/hw/arm/allwinner-r40.c
> @@ -45,6 +45,13 @@ const hwaddr allwinner_r40_memmap[] = {
>      [AW_R40_DEV_CCU]        = 0x01c20000,
>      [AW_R40_DEV_PIT]        = 0x01c20c00,
>      [AW_R40_DEV_UART0]      = 0x01c28000,
> +    [AW_R40_DEV_UART1]      = 0x01c28400,
> +    [AW_R40_DEV_UART2]      = 0x01c28800,
> +    [AW_R40_DEV_UART3]      = 0x01c28c00,
> +    [AW_R40_DEV_UART4]      = 0x01c29000,
> +    [AW_R40_DEV_UART5]      = 0x01c29400,
> +    [AW_R40_DEV_UART6]      = 0x01c29800,
> +    [AW_R40_DEV_UART7]      = 0x01c29c00,
>

After adding the uarts to the memory map here, you should remove them from
the unimplemented array.

     [AW_R40_DEV_GIC_DIST]   = 0x01c81000,
>      [AW_R40_DEV_GIC_CPU]    = 0x01c82000,
>      [AW_R40_DEV_GIC_HYP]    = 0x01c84000,
> @@ -160,6 +167,10 @@ enum {
>      AW_R40_GIC_SPI_UART1     =  2,
>      AW_R40_GIC_SPI_UART2     =  3,
>      AW_R40_GIC_SPI_UART3     =  4,
>

Since you put the addition of UART1-7 in this patch, probably it makes
sense to have adding the lines 'AW_R40_GIC_SPI_UART1/2/3' also part of this
patch.

With the two above remarks resolved, the patch looks good to me.

Reviewed-by: Niek Linnenbank <nieklinnenbank@gmail.com>

Regards,
Niek

> +    AW_R40_GIC_SPI_UART4     = 17,
> +    AW_R40_GIC_SPI_UART5     = 18,
> +    AW_R40_GIC_SPI_UART6     = 19,
> +    AW_R40_GIC_SPI_UART7     = 20,
>      AW_R40_GIC_SPI_TIMER0    = 22,
>      AW_R40_GIC_SPI_TIMER1    = 23,
>      AW_R40_GIC_SPI_MMC0      = 32,
> @@ -387,9 +398,23 @@ static void allwinner_r40_realize(DeviceState *dev,
> Error **errp)
>      }
>
>      /* UART0. For future clocktree API: All UARTS are connected to
> APB2_CLK. */
> -    serial_mm_init(get_system_memory(), s->memmap[AW_R40_DEV_UART0], 2,
> -                   qdev_get_gpio_in(DEVICE(&s->gic),
> AW_R40_GIC_SPI_UART0),
> -                   115200, serial_hd(0), DEVICE_NATIVE_ENDIAN);
> +    for (int i = 0; i < AW_R40_NUM_UARTS; i++) {
> +        static const int uart_irqs[AW_R40_NUM_UARTS] = {
> +            AW_R40_GIC_SPI_UART0,
> +            AW_R40_GIC_SPI_UART1,
> +            AW_R40_GIC_SPI_UART2,
> +            AW_R40_GIC_SPI_UART3,
> +            AW_R40_GIC_SPI_UART4,
> +            AW_R40_GIC_SPI_UART5,
> +            AW_R40_GIC_SPI_UART6,
> +            AW_R40_GIC_SPI_UART7,
> +        };
> +        const hwaddr addr = s->memmap[AW_R40_DEV_UART0 + i];
> +
> +        serial_mm_init(get_system_memory(), addr, 2,
> +                       qdev_get_gpio_in(DEVICE(&s->gic), uart_irqs[i]),
> +                       115200, serial_hd(i), DEVICE_NATIVE_ENDIAN);
> +    }
>
>      /* Unimplemented devices */
>      for (i = 0; i < ARRAY_SIZE(r40_unimplemented); i++) {
> diff --git a/include/hw/arm/allwinner-r40.h
> b/include/hw/arm/allwinner-r40.h
> index 3be9dc962b..959b5dc4e0 100644
> --- a/include/hw/arm/allwinner-r40.h
> +++ b/include/hw/arm/allwinner-r40.h
> @@ -41,6 +41,13 @@ enum {
>      AW_R40_DEV_CCU,
>      AW_R40_DEV_PIT,
>      AW_R40_DEV_UART0,
> +    AW_R40_DEV_UART1,
> +    AW_R40_DEV_UART2,
> +    AW_R40_DEV_UART3,
> +    AW_R40_DEV_UART4,
> +    AW_R40_DEV_UART5,
> +    AW_R40_DEV_UART6,
> +    AW_R40_DEV_UART7,
>      AW_R40_DEV_GIC_DIST,
>      AW_R40_DEV_GIC_CPU,
>      AW_R40_DEV_GIC_HYP,
> @@ -70,6 +77,7 @@ OBJECT_DECLARE_SIMPLE_TYPE(AwR40State, AW_R40)
>   * which are currently emulated by the R40 SoC code.
>   */
>  #define AW_R40_NUM_MMCS         4
> +#define AW_R40_NUM_UARTS        8
>
>  struct AwR40State {
>      /*< private >*/
> --
> 2.25.1
>
>

-- 
Niek Linnenbank

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