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.

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

-- 
2.28.0.rc0.105.gf9edc3c819-goog

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
> 

On Fri, Jul 17, 2020 at 1:32 PM Cédric Le Goater <clg@kaod.org> wrote:
>
> 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.

Hmm, I didn't expect this to be a problem when hosting the images on github...?

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

Great! Thanks a lot for reviewing and testing.

I've incorporated the feedback I got today, but I'm keeping them as
separate commits for now. I can either fold them into a v7 series or
post them as follow-ups.

Thanks!

Havard

Hi Havard,

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

I've been quite busy, now looking back a this series. Do you have a v7
in preparation or should I keep reviewing it?

Hopefully v7 would be the one Peter queue for merging once 5.2 window
opens :)

Regards,

Phil.

Hi Philippe,

On Mon, Aug 3, 2020 at 12:08 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Hi Havard,
>
> 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.
> >
> > 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.
>
> I've been quite busy, now looking back a this series. Do you have a v7
> in preparation or should I keep reviewing it?

I have a few fixes queued up, but I didn't turn it into a v7 series
yet. I can probably have that ready by tomorrow if you prefer.

Or I can wait a bit longer and queue up more fixes. The merge window
is expected to open next week at the earliest right?

> Hopefully v7 would be the one Peter queue for merging once 5.2 window
> opens :)

I hope so too :)

Havard