Re: [PATCH v2 16/17] hw/arm: Add NPCM8XX SoC

[Peter Maydell]

On Thu, 26 Dec 2024 at 08:29, Hao Wu <wuhaotsh@google.com> wrote:
>
> Signed-off-by: Hao Wu <wuhaotsh@google.com>
> ---
>  configs/devices/aarch64-softmmu/default.mak |   1 +
>  hw/arm/Kconfig                              |  11 +
>  hw/arm/meson.build                          |   1 +
>  hw/arm/npcm8xx.c                            | 810 ++++++++++++++++++++
>  include/hw/arm/npcm8xx.h                    | 107 +++
>  5 files changed, 930 insertions(+)
>  create mode 100644 hw/arm/npcm8xx.c
>  create mode 100644 include/hw/arm/npcm8xx.h
>
> diff --git a/configs/devices/aarch64-softmmu/default.mak 
> b/configs/devices/aarch64-softmmu/default.mak
> index f82a04c27d..5ea1cc2dd1 100644
> --- a/configs/devices/aarch64-softmmu/default.mak
> +++ b/configs/devices/aarch64-softmmu/default.mak
> @@ -8,3 +8,4 @@ include ../arm-softmmu/default.mak
>  # CONFIG_XLNX_ZYNQMP_ARM=n
>  # CONFIG_XLNX_VERSAL=n
>  # CONFIG_SBSA_REF=n
> +CONFIG_NPCM8XX=y

This should be a commented-out "n" (the way we arrange
this has flipped around since you originally wrote
the board support).

> diff --git a/hw/arm/Kconfig b/hw/arm/Kconfig
> index e779b5af95..4fd5a41739 100644
> --- a/hw/arm/Kconfig
> +++ b/hw/arm/Kconfig
> @@ -481,6 +481,17 @@ config NPCM7XX
>      select PCA954X
>      select USB_OHCI_SYSBUS
>
> +config NPCM8XX
> +    bool
> +    select ARM_GIC
> +    select SMBUS
> +    select PL310  # cache controller
> +    select NPCM7XX
> +    select SERIAL
> +    select SSI
> +    select UNIMP

You want a "depends on TCG && ARM" clause: compare the
other stanzas in the file.
(This is the other half of the flipping-around noted above).

>  config FSL_IMX25
>      bool
>      default y
> diff --git a/hw/arm/meson.build b/hw/arm/meson.build
> index 490234b3b8..d7813c089c 100644
> --- a/hw/arm/meson.build
> +++ b/hw/arm/meson.build
> @@ -12,6 +12,7 @@ arm_ss.add(when: 'CONFIG_MUSICPAL', if_true: 
> files('musicpal.c'))
>  arm_ss.add(when: 'CONFIG_NETDUINOPLUS2', if_true: files('netduinoplus2.c'))
>  arm_ss.add(when: 'CONFIG_OLIMEX_STM32_H405', if_true: 
> files('olimex-stm32-h405.c'))
>  arm_ss.add(when: 'CONFIG_NPCM7XX', if_true: files('npcm7xx.c', 
> 'npcm7xx_boards.c'))
> +arm_ss.add(when: 'CONFIG_NPCM8XX', if_true: files('npcm8xx.c'))
>  arm_ss.add(when: 'CONFIG_REALVIEW', if_true: files('realview.c'))
>  arm_ss.add(when: 'CONFIG_SBSA_REF', if_true: files('sbsa-ref.c'))
>  arm_ss.add(when: 'CONFIG_STELLARIS', if_true: files('stellaris.c'))
> diff --git a/hw/arm/npcm8xx.c b/hw/arm/npcm8xx.c
> new file mode 100644
> index 0000000000..ed4185f37d
> --- /dev/null
> +++ b/hw/arm/npcm8xx.c
> @@ -0,0 +1,810 @@
> +/*
> + * Nuvoton NPCM8xx SoC family.
> + *
> + * Copyright 2022 Google LLC
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms of the GNU General Public License as published by the
> + * Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful, but 
> WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
> + * for more details.
> + */
> +
> +#include "qemu/osdep.h"
> +
> +#include "hw/arm/boot.h"
> +#include "hw/arm/npcm8xx.h"
> +#include "hw/char/serial-mm.h"
> +#include "hw/intc/arm_gic.h"
> +#include "hw/loader.h"
> +#include "hw/misc/unimp.h"
> +#include "hw/qdev-clock.h"
> +#include "hw/qdev-properties.h"
> +#include "qapi/error.h"
> +#include "qemu/units.h"
> +#include "system/system.h"
> +
> +#define ARM_PHYS_TIMER_PPI  30
> +#define ARM_VIRT_TIMER_PPI  27
> +#define ARM_HYP_TIMER_PPI   26
> +#define ARM_SEC_TIMER_PPI   29

We have defines for these basically-standardized IRQ
numbers in include/hw/arm/bsa.h now.

> +
> +/*
> + * This covers the whole MMIO space. We'll use this to catch any MMIO 
> accesses
> + * that aren't handled by a device.
> + */
> +#define NPCM8XX_MMIO_BA         (0x80000000)
> +#define NPCM8XX_MMIO_SZ         (0x7ffd0000)

You don't need brackets around simple constants in a #define.


> +/* Total number of GIC interrupts, including internal Cortex-A35 interrupts. 
> */
> +#define NPCM8XX_NUM_IRQ         (288)
> +#define NPCM8XX_PPI_BASE(cpu)   ((NPCM8XX_NUM_IRQ - 32) + (cpu) * 32)

These "32"s should probably be "GIC_INTERNAL"s.

> +        /* IRQ for watchdogs */
> +        sysbus_connect_irq(sbd, NPCM7XX_TIMERS_PER_CTRL,
> +                npcm8xx_irq(s, NPCM8XX_WDG0_IRQ + i));
> +        /* GPIO that connects clk module with watchdog */
> +        /* TODO: Check this.*/

Good idea :-)

> +        qdev_connect_gpio_out_named(DEVICE(&s->tim[i]),
> +                NPCM7XX_WATCHDOG_RESET_GPIO_OUT, 0,
> +                qdev_get_gpio_in_named(DEVICE(&s->clk),
> +                        NPCM7XX_WATCHDOG_RESET_GPIO_IN, i));
> +    }

Otherwise looks OK, but again, review by somebody who
knows the hardware would be nice.

thanks
-- PMM