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

[Alexander Bulekov]

Hi Havard,
I fuzzed the npcm750-evb machine until I hit over 85% coverage over all
the new npcm.*\.c files. The only thing I found specific to the new
code, so far:

cat << EOF | ./qemu-system-arm -machine npcm750-evb -m 128M -qtest stdio 
write 0xf0009040 0x4 0xc4c4c4c4
write 0xf0009040 0x4 0x4
EOF

ERROR:../hw/timer/npcm7xx_timer.c:160:npcm7xx_timer_pause: assertion failed: 
(t->remaining_ns > 0)
Bail out! ERROR:../hw/timer/npcm7xx_timer.c:160:npcm7xx_timer_pause: assertion 
failed: (t->remaining_ns > 0)
Aborted

I'm doing the same for the quanta-gsj machine, but I'm not sure whether
it will cover more code, so I'm happy to leave a:

Tested-by: Alexander Bulekov <alxndr@bu.edu>

for the patches that add new virtual-device code (1-5, 7-12 ?)
-Alex


On 200824 1716, Havard Skinnemoen via wrote:
> I also pushed this and the previous patchsets to my qemu fork on github.
> The branches are named npcm7xx-v[1-8].
> 
>   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.
> 
> Changes since v7:
> 
>   - Move register enums to .c files throughout, leaving a single
>     NPCM7XX_FOO_NR_REGS definition behind in the .h file. A QEMU_BUILD_BUG_ON
>     should alert anyone accidentally expanding the register enum that they 
> need
>     to update the corresponding NR_REGS define, which in turn has a comment
>     reminding them to update the vmstate version_id as well.
>   - Skip loading the bootrom if a kernel filename is provided by the user.
>   - New patch adding a board setup stub to tweak clocks before booting 
> directly
>     into the kernel.
>   - Add stuff to meson files instead of Makefiles.
>   - Try to disable the slowest drivers and services to speed up the flash boot
>     acceptance test a bit. This is somewhat based on the following
>     systemd-analyze blame report:
>     https://gist.github.com/hskinnemoen/475cb0676530cd2cebaa1754cf16ca97
> 
> Changes since v6:
> 
>   - Use size_to_str to report DRAM sizes in npcm7xx_gcr.
>   - Simplify the interrupt logic in npcm7xx_timer.
>   - Update global bios_name instead of temporary.
>   - Add npcm7xx_bootrom to MAINTAINERS and pc-bios/README.
>   - Use a predefined name for the gsj boot image in the acceptance test.
> 
> 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 (14):
>   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
>   hw/arm/npcm7xx: add board setup stub for CPU and UART clocks
>   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               | 112 +++++
>  include/hw/mem/npcm7xx_mc.h            |  36 ++
>  include/hw/misc/npcm7xx_clk.h          |  48 +++
>  include/hw/misc/npcm7xx_gcr.h          |  43 ++
>  include/hw/nvram/npcm7xx_otp.h         |  79 ++++
>  include/hw/ssi/npcm7xx_fiu.h           |  73 ++++
>  include/hw/timer/npcm7xx_timer.h       |  78 ++++
>  hw/arm/npcm7xx.c                       | 532 +++++++++++++++++++++++
>  hw/arm/npcm7xx_boards.c                | 197 +++++++++
>  hw/mem/npcm7xx_mc.c                    |  84 ++++
>  hw/misc/npcm7xx_clk.c                  | 266 ++++++++++++
>  hw/misc/npcm7xx_gcr.c                  | 269 ++++++++++++
>  hw/nvram/npcm7xx_otp.c                 | 439 +++++++++++++++++++
>  hw/ssi/npcm7xx_fiu.c                   | 572 +++++++++++++++++++++++++
>  hw/timer/npcm7xx_timer.c               | 509 ++++++++++++++++++++++
>  .gitmodules                            |   3 +
>  MAINTAINERS                            |  10 +
>  hw/arm/Kconfig                         |   9 +
>  hw/arm/meson.build                     |   1 +
>  hw/mem/meson.build                     |   1 +
>  hw/misc/meson.build                    |   4 +
>  hw/misc/trace-events                   |   8 +
>  hw/nvram/meson.build                   |   1 +
>  hw/ssi/meson.build                     |   1 +
>  hw/ssi/trace-events                    |  11 +
>  hw/timer/meson.build                   |   1 +
>  hw/timer/trace-events                  |   5 +
>  pc-bios/README                         |   6 +
>  pc-bios/npcm7xx_bootrom.bin            | Bin 0 -> 768 bytes
>  roms/Makefile                          |   7 +
>  roms/vbootrom                          |   1 +
>  tests/acceptance/boot_linux_console.py |  83 ++++
>  36 files changed, 3582 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
> 
> -- 
> 2.28.0.297.g1956fa8f8d-goog
> 
>