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

[Havard Skinnemoen]

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