Re: [PATCH v4 0/7] Add IBM Huygens BMC machine for AST2700

Mikail,

On 8/13/26 21:30, Mikail Sadic wrote:
> This series adds initial support for the IBM Huygens BMC, the baseboard
> management controller of an IBM POWER12 server platform, based on the
> ASPEED AST2700 A2 SoC.
> 
> It introduces a CFAM-S FSI responder model, wires the AST2700 FSI
> controllers to their APB-to-OPB bridges, adds a TI UCD90320 PMBus
> power-sequencer model, adds an AST2700 UFS host controller on top of the
> generic sysbus UFS device, and finally adds the huygens-bmc machine that
> ties these together to boot OpenBMC from FMC flash and a UFS root
> filesystem.
> 
> Dependencies, both on-list, neither merged yet:
> 
> 1. Jeuk Kim's UFS restructure, which patches 5 and 6 build on:
> 
>       https://lore.kernel.org/qemu-devel/[email protected]/


Since this series is the primary user of the UFS restructure, would you
have time to review it ?

> 
>     The base-commit trailer below therefore names a local commit: current
>     master plus those two patches. Patches 1-4 and 7 do not need it.
> 
> 2. Jamin Lin's I2C block-read fix:
> 
>       https://patchwork.kernel.org/project/qemu-devel/patch/[email protected]/

Same here.

Thanks,

C.


>     This one is a runtime rather than a build dependency, which is why the
>     series is not based on it. Everything compiles and every machine
>     constructs without it, but the UCD90320 in patch 4 will not probe,
>     because the ucd9000 driver identifies the device with an SMBus block
>     read and the AST2700 reports the block length from a register that
>     patch models. I have tested his patch against that driver and it works;
>     the Huygens-BMC boot was run with it applied.
> 
>     The guest also needs a kernel that reads the block length from the
>     right register on AST2700, i.e. one carrying
> 
>       https://github.com/AspeedTech-BMC/linux/commit/f9938ec5799e
> 
> Patch summary:
> 
>    1. fsi/cfam: a common CFAM model holding the slot, config table,
>       responder and local bus, with the existing cfam as its first
>       derivative. No functional change to the existing cfam.
>    2. fsi/cfam: the CFAM-S as a second derivative - its own config table,
>       the slave-ID alias window, and a v1 mailbox engine added to lbus.c
>       alongside the scratchpad.
>    3. arm/aspeed: wire the two AST2700 FSI controllers to their APB-to-OPB
>       bridges.
>    4. hw/sensor: TI UCD90320 24-rail PMBus power sequencer.
>    5. ufs: a configurable logical-block-size on ufs-lu, and an unmapped LUN
>       answering INQUIRY as "not connected". The ufs PCI device keeps its
>       current property set and geometry.
>    6. ufs/aspeed: the AST2700 UFS host controller as a subclass of
>       TYPE_SYSBUS_UFS, adding only the UFSHCI version.
>    7. arm/aspeed: the huygens-bmc machine.
> 
> Changes since v3:
> 
> * Dropped the i2c/aspeed patch (v3 4/8). Jamin Lin has posted the correct
>    implementation, which the UCD90320 now depends on instead; reviewed and
>    tested, see the dependency note above. The guest also needs the AST2700
>    kernel fix linked there.
> 
> * Rebased onto current master, which is past the 11.1.0 release. v3 was on
>    v11.1.0-rc3. The only conflict was hw/arm/Kconfig, where the UCD90320
>    select landed next to the newly added ADC128D818 one.
> 
> * No other patch changed. Patches 1-3 and 5-7 are the v3 patches with
>    only their numbering shifted.
> 
> Not addressed in this series:
> 
> * CFAM extended addressing (Miles): the mode where a single CFAM has 8MB
>    rather than four cascaded CFAMs sharing that space. This changes the
>    addressing model of the existing FSI/CFAM code rather than anything this
>    series introduces, so it's left as follow-up work. It does bear on
>    patch 2 though: the CFAM-S there aliases one 2MiB slot across the 8MiB
>    window to fold the slave-ID views together, and if extended addressing
>    is modelled properly that window is probably the wrong shape or should
>    not exist at all.
> 
> * A functional test for the machine. The v1 test depended on non-public
>    Huygens images and was dropped; it will be resent once images can be
>    published.
> 
> Testing done:
> 
> * Whole series builds, and each of the 7 patches builds standalone.
> * make check passes.
> * huygens-bmc boots OpenBMC to the login prompt and multi-user target
>    from FMC flash + UFS storage (internal images), with Jamin's I2C patch
>    applied. The UCD90320 probes, reports its device identification over an
>    SMBus block read, and the power sequencer publishes system power state.
> * PCI UFS qtests pass (13/13), unchanged by patch 5.
> * aspeed_fsi-test passes (4/4), unchanged by the CFAM patches.
> * huygens-bmc, ast2700-evb and ast2600-evb all construct successfully.
> * On huygens-bmc, the UFS version register reads 0x00000200, and the
>    ufs-lu created by the board reports a 512-byte block size and passes it
>    to its backing scsi-hd. The PCI ufs device with no property set still
>    reports 4096.
> 
> Changes since v2 (unchanged from the v3 posting, for reference):
> 
> * UFS: rebased onto Jeuk Kim's restructure, as requested. The v2 core
>    refactor (v2 patch 6) is dropped entirely, Jeuk's patch 1 supersedes it.
>    hw/ufs/aspeed_ufs.c is now a subclass of TYPE_SYSBUS_UFS whose only
>    AST2700-specific behavior is pinning the controller version register
>    to 2.0, chained after the parent realize. MMIO, IRQ, DMA address space,
>    properties, migration state and unrealize all come from the generic
>    sysbus device, and the Aspeed type adds no instance fields.
> 
>    The two behaviors Huygens needs that the restructure does not provide
>    are now their own patch, placed ahead of the Aspeed patch: the ufs-lu
>    logical-block-size property (default unchanged, so the PCI device is
>    unaffected) and the unmapped-LUN INQUIRY reporting "not connected"
>    rather than failing the request. logical-block-size now also rejects
>    non-powers-of-two, values outside 512..UFS_BLOCK_SIZE, and a drive
>    shorter than one block, which would previously have realized a
>    zero-block unit.
> 
>    The Aspeed SoC Kconfig now selects UFS_SYSBUS. It previously relied on
>    UFS_PCI being enabled via "imply PCI_DEVICES".
> 
> * CFAM: dropped the State suffix from the instance struct names (Miles).
>    FSICFAMCommonState becomes FSICFAMCommon; FSICFAMState, which predates
>    this series, becomes FSICFAM to match rather than leaving the two
>    spellings side by side in one header; FSICFAMSState becomes FSICFAMS.
>    The FSI prefix is kept. The class structs keep their suffix, as is
>    usual for QOM. Also fixed patch 1 to check the realize_engines() return
>    value.
> 
> * Machine: reworded the commit message (Miles). No code change.
> 
> Note: checkpatch reports a couple of MAINTAINERS/SPDX warnings for the
> new files; they are covered by existing wildcard entries in the ASPEED
> and FSI sections.
> 
> Thank you again for all of the feedback.
> 
> Mikail Sadic (7):
>    fsi/cfam: Add common CFAM base class
>    fsi/cfam: Add CFAM-S model
>    arm/aspeed: Wire AST2700 FSI controllers to APB-to-OPB bridges
>    hw/sensor: Add UCD90320 model
>    ufs: Make the logical block size configurable and answer absent LUNs
>    ufs/aspeed: Add AST2700 UFS host controller
>    arm/aspeed: Add AST2700 Huygens machine
> 
>   MAINTAINERS                     |   3 +
>   docs/specs/aspeed-ufs.rst       |  46 ++++++
>   docs/specs/fsi.rst              |  15 ++
>   docs/specs/index.rst            |   2 +
>   docs/specs/ucd90320.rst         |  36 +++++
>   docs/system/arm/aspeed.rst      |  50 ++++++-
>   hw/ufs/ufs.h                    |   3 +
>   include/hw/arm/aspeed.h         |   1 +
>   include/hw/arm/aspeed_soc.h     |   3 +
>   include/hw/fsi/cfam-s.h         |  40 ++++++
>   include/hw/fsi/cfam.h           |  68 ++++++++-
>   include/hw/fsi/fsi-master.h     |   4 +-
>   include/hw/fsi/lbus.h           |  11 ++
>   include/hw/ufs/aspeed_ufs.h     |  15 ++
>   hw/arm/aspeed.c                 |  25 ++++
>   hw/arm/aspeed_ast27x0.c         |  31 ++++
>   hw/arm/aspeed_ast27x0_huygens.c | 241 ++++++++++++++++++++++++++++++++
>   hw/fsi/cfam-s.c                 |  91 ++++++++++++
>   hw/fsi/cfam.c                   | 125 ++++++++++-------
>   hw/fsi/fsi-master.c             |   9 +-
>   hw/fsi/lbus.c                   |  69 +++++++++
>   hw/sensor/ucd90320.c            | 169 ++++++++++++++++++++++
>   hw/ufs/aspeed_ufs.c             |  67 +++++++++
>   hw/ufs/lu.c                     |  67 ++++++++-
>   hw/ufs/ufs.c                    |   2 +-
>   hw/arm/Kconfig                  |   2 +
>   hw/arm/meson.build              |   1 +
>   hw/fsi/meson.build              |   2 +-
>   hw/fsi/trace-events             |   2 +
>   hw/sensor/Kconfig               |   4 +
>   hw/sensor/meson.build           |   1 +
>   hw/ufs/meson.build              |   1 +
>   32 files changed, 1142 insertions(+), 64 deletions(-)
>   create mode 100644 docs/specs/aspeed-ufs.rst
>   create mode 100644 docs/specs/ucd90320.rst
>   create mode 100644 include/hw/fsi/cfam-s.h
>   create mode 100644 include/hw/ufs/aspeed_ufs.h
>   create mode 100644 hw/arm/aspeed_ast27x0_huygens.c
>   create mode 100644 hw/fsi/cfam-s.c
>   create mode 100644 hw/sensor/ucd90320.c
>   create mode 100644 hw/ufs/aspeed_ufs.c
> 
> 
> base-commit: 7a66901b243ca3b401db8a8e45245e0518339974