Re: [PATCH v6 00/13] Add Nuvoton NPCM730/NPCM750 SoCs and two BMC machines

["Cédric Le Goater" <clg@kaod.org>]

On 7/17/20 8:02 AM, Havard Skinnemoen wrote:
> I also pushed this and the previous two patchsets to my qemu fork on github.
> The branches are named npcm7xx-v[1-6].
> 
>   https://github.com/hskinnemoen/qemu
> 
> This patch series models enough of the Nuvoton NPCM730 and NPCM750 SoCs to boot
> an OpenBMC image built for quanta-gsj. This includes device models for:
> 
>   - Global Configuration Registers
>   - Clock Control
>   - Timers
>   - Fuses
>   - Memory Controller
>   - Flash Controller
> 
> These modules, along with the existing Cortex A9 CPU cores and built-in
> peripherals, are integrated into a NPCM730 or NPCM750 SoC, which in turn form
> the foundation for the quanta-gsj and npcm750-evb machines, respectively. The
> two SoCs are very similar; the only difference is that NPCM730 is missing some
> peripherals that NPCM750 has, and which are not considered essential for
> datacenter use (e.g. graphics controllers). For more information, see
> 
> https://www.nuvoton.com/products/cloud-computing/ibmc/
> 
> Both quanta-gsj and npcm750-evb correspond to real boards supported by OpenBMC.
> At the end of the series, qemu can boot an OpenBMC image built for one of these
> boards with some minor modifications.
> 
> The patches in this series were developed by Google and reviewed by Nuvoton. We
> will be maintaining the machine and peripheral support together.
> 
> The data sheet for these SoCs is not generally available. Please let me know if
> more comments are needed to understand the device behavior.

I think this series is ready to go.

Patch 6 "roms: Add virtual Boot ROM for NPCM7xx SoCs" needs a few
add-ons which can come later.

Patch 13 "tests/acceptance: console boot tests for quanta-gsj" needs
an Acked-by by our test gurus. Aspeed needs the same kind of tests
if possible. It depends on the pressure that the QEMU CI will put on 
the web servers hosting the images.

I think that the rest of the comments are minor and can be fixed
as follow-ups when 5.2 is being assembled. 

Thanks,

C.


> Changes since v5:
> 
>   - Boot ROM included, as a git submodule and a binary blob, and loaded by
>     default, so the -bios option is usually not necessary anymore.
>   - Two acceptance tests added (openbmc image boot, and direct kernel boot).
>   - npcm7xx_load_kernel() moved to SoC code.
>   - NPCM7XX_TIMER_REF_HZ definition moved to CLK header.
>   - Comments added clarifying available SPI flash chip selects.
>   - Error handling adjustments:
>       - Errors from CPU and GCR realization are propagated through the SoC
>         since they may be triggered by user-configurable parameters.
>       - Machine init uses error_fatal instead of error_abort for SoC
>         realization flash init. This makes error messages more helpful.
>       - Comments added to indicate whether peripherals may fail to realize.
>       - Use ERRP_GUARD() instead of Error *err when possible.
>   - Default CPU type is now set, and attempting to set it to anything else
>     will fail.
>   - Format string fixes (use HWADDR_PRIx, etc.)
>   - Simplified memory size encoding and error checking in npcm7xx_gcr.
>   - Encapsulate non-obvious pointer subtraction into helper functions in the
>     FIU and TIMER modules.
>   - Incorporate review feedback into the FIU module:
>       - Add select/deselect trace events.
>       - Use npcm7xx_fiu_{de,}select() consistently.
>       - Use extract/deposit in more places for consistency.
>       - Use -Wimplicit-fallthrough compatible fallthrough comments.
>       - Use qdev_init_gpio_out_named instead of sysbus_init_irq for chip
>         selects.
>   - Incorporate review feedback into the TIMER module:
>       - Assert that we never pause a timer that has already expired, instead of
>         trying to handle it. This should be safe since QEMU_CLOCK_VIRTUAL is
>         stopped while this code is running.
>       - Simplify the switch blocks in the read and write handlers.
> 
> I made a change to error out if a flash drive was not specified, but reverted
> it because it caused make check to fail (qom-test). When specifying a NULL
> block device, the m25p flash device initializes its in-memory storage with 0xff
> and doesn't attempt to write anything back. This seems correct to me.
> 
> Changes since v4:
> 
>   - OTP cleanups suggested by Philippe Mathieu-Daudé.
>       - Added fuse array definitions based on public Nuvoton bootblock code.
>       - Moved class structure to .c file since it's only used internally.
>       - Readability improvements.
>   - Split the first patch and folded parts of it into three other patches so
>     that CONFIG_NPCM7XX is only enabled after the initial NPCM7xx machine
>     support is added.
>   - DRAM init moved to machine init code.
>   - Consistently use lower-case hex literals.
>   - Switched to fine-grained unimplemented devices, based on public bootblock
>     source code. Added a tiny SRAM that got left out previously.
>   - Simplified error handling in npcm7xx_realize() since the board code will
>     abort anyway, and SoCs are not hot-pluggable.
> 
> Changes since v3:
> 
>   - License headers are now GPL v2-or-later throughout.
>   - Added vmstate throughout (except the memory controller, which doesn't
>     really have any state worth saving). Successfully booted a gsj image
>     with two stop/savevm/quit/loadvm cycles along the way.
>       - JFFS2 really doesn't like it if I let qemu keep running after savevm,
>         and then jump back in time with loadvm. I assume this is expected.
>   - Fixed an error API violation in npcm7xx_realize, removed pointless error
>     check after object_property_set_link().
>   - Switched the OTP device to use an embedded array instead of a g_malloc0'd
>     one because I couldn't figure out how to set up vmstate for the latter.
> 
> Changes since v2:
> 
>   - Simplified the MAINTAINERS entry.
>   - Added link to OpenPOWER jenkins for gsj BMC images.
>   - Reverted the smpboot change, back to byte swapping.
>   - Adapted to upstream API changes:
>       - sysbus_init_child_obj -> object_initialize_child
>       - object_property_set_bool -> qdev_realize / sysbus_realize
>       - ssi_create_slave_no_init -> qdev_new
>       - qdev_init_nofail -> qdev_realize_and_unref
>       - ssi_auto_connect_slaves removed
>   - Moved Boot ROM loading from soc to machine init.
>   - Plumbed power-on-straps property from GCR to the machine init code so it
>     can be properly initialized. Turns out npcm750 memory init doesn't work
>     without this. npcm730 is fine either way, though I'm not sure why.
>   - Reworked the flash init code so it looks more like aspeed (i.e. the flash
>     device gets added even if there's no drive).
> 
> Changes since v1 (requested by reviewers):
> 
>   - Clarify the source of CLK reset values.
>   - Made smpboot a constant byte array, eliinated byte swapping.
>   - NPCM7xxState now stores an array of ARMCPUs, not pointers to ARMCPUs.
>   - Clarify why EL3 is disabled.
>   - Introduce NPCM7XX_NUM_IRQ constant.
>   - Set the number of CPUs according to SoC variant, and disallow command line
>     overrides (i.e. you can no longer override the number of CPUs with the -smp
>     parameter). This is trying to follow the spirit of
>     https://patchwork.kernel.org/patch/11595407/.
>   - Switch register operations to DEVICE_LITTLE_ENDIAN throughout.
>   - Machine documentation added (new patch).
> 
> Changes since v1 to support flash booting:
> 
>   - GCR reset value changes to get past memory initialization when booting
>     from flash (patches 2 and 5):
>       - INTCR2 now indicates that the DDR controller is initialized.
>       - INTCR3 is initialized according to DDR memory size. A realize()
> 	method was implemented to achieve this.
>   - Refactor the machine initialization a bit to make it easier to drop in
>     machine-specific flash initialization (patch 6).
>   - Extend the series with additional patches to enable booting from flash:
>       - Boot ROM (through the -bios option).
>       - OTP (fuse) controller.
>       - Memory Controller stub (just enough to skip memory training).
>       - Flash controller.
>       - Board-specific flash initialization.
> 
> Thanks for reviewing,
> 
> Havard
> 
> Havard Skinnemoen (13):
>   hw/misc: Add NPCM7xx System Global Control Registers device model
>   hw/misc: Add NPCM7xx Clock Controller device model
>   hw/timer: Add NPCM7xx Timer device model
>   hw/arm: Add NPCM730 and NPCM750 SoC models
>   hw/arm: Add two NPCM7xx-based machines
>   roms: Add virtual Boot ROM for NPCM7xx SoCs
>   hw/arm: Load -bios image as a boot ROM for npcm7xx
>   hw/nvram: NPCM7xx OTP device model
>   hw/mem: Stubbed out NPCM7xx Memory Controller model
>   hw/ssi: NPCM7xx Flash Interface Unit device model
>   hw/arm: Wire up BMC boot flash for npcm750-evb and quanta-gsj
>   docs/system: Add Nuvoton machine documentation
>   tests/acceptance: console boot tests for quanta-gsj
> 
>  docs/system/arm/nuvoton.rst            |  90 +++++
>  docs/system/target-arm.rst             |   1 +
>  Makefile                               |   1 +
>  default-configs/arm-softmmu.mak        |   1 +
>  include/hw/arm/npcm7xx.h               | 111 +++++
>  include/hw/mem/npcm7xx_mc.h            |  36 ++
>  include/hw/misc/npcm7xx_clk.h          |  72 ++++
>  include/hw/misc/npcm7xx_gcr.h          |  76 ++++
>  include/hw/nvram/npcm7xx_otp.h         |  88 ++++
>  include/hw/ssi/npcm7xx_fiu.h           | 100 +++++
>  include/hw/timer/npcm7xx_timer.h       |  96 +++++
>  hw/arm/npcm7xx.c                       | 500 +++++++++++++++++++++++
>  hw/arm/npcm7xx_boards.c                | 195 +++++++++
>  hw/mem/npcm7xx_mc.c                    |  84 ++++
>  hw/misc/npcm7xx_clk.c                  | 234 +++++++++++
>  hw/misc/npcm7xx_gcr.c                  | 227 +++++++++++
>  hw/nvram/npcm7xx_otp.c                 | 424 +++++++++++++++++++
>  hw/ssi/npcm7xx_fiu.c                   | 539 +++++++++++++++++++++++++
>  hw/timer/npcm7xx_timer.c               | 489 ++++++++++++++++++++++
>  .gitmodules                            |   3 +
>  MAINTAINERS                            |   8 +
>  hw/arm/Kconfig                         |   9 +
>  hw/arm/Makefile.objs                   |   1 +
>  hw/mem/Makefile.objs                   |   1 +
>  hw/misc/Makefile.objs                  |   2 +
>  hw/misc/trace-events                   |   8 +
>  hw/nvram/Makefile.objs                 |   1 +
>  hw/ssi/Makefile.objs                   |   1 +
>  hw/ssi/trace-events                    |  11 +
>  hw/timer/Makefile.objs                 |   1 +
>  hw/timer/trace-events                  |   5 +
>  pc-bios/npcm7xx_bootrom.bin            | Bin 0 -> 768 bytes
>  roms/Makefile                          |   7 +
>  roms/vbootrom                          |   1 +
>  tests/acceptance/boot_linux_console.py |  65 +++
>  35 files changed, 3488 insertions(+)
>  create mode 100644 docs/system/arm/nuvoton.rst
>  create mode 100644 include/hw/arm/npcm7xx.h
>  create mode 100644 include/hw/mem/npcm7xx_mc.h
>  create mode 100644 include/hw/misc/npcm7xx_clk.h
>  create mode 100644 include/hw/misc/npcm7xx_gcr.h
>  create mode 100644 include/hw/nvram/npcm7xx_otp.h
>  create mode 100644 include/hw/ssi/npcm7xx_fiu.h
>  create mode 100644 include/hw/timer/npcm7xx_timer.h
>  create mode 100644 hw/arm/npcm7xx.c
>  create mode 100644 hw/arm/npcm7xx_boards.c
>  create mode 100644 hw/mem/npcm7xx_mc.c
>  create mode 100644 hw/misc/npcm7xx_clk.c
>  create mode 100644 hw/misc/npcm7xx_gcr.c
>  create mode 100644 hw/nvram/npcm7xx_otp.c
>  create mode 100644 hw/ssi/npcm7xx_fiu.c
>  create mode 100644 hw/timer/npcm7xx_timer.c
>  create mode 100644 pc-bios/npcm7xx_bootrom.bin
>  create mode 160000 roms/vbootrom
>