Re: [RFC PATCH 0/4] espi: introduce eSPI bus framework

["M, Krishnamoorthi" <krishnamoorthi.m@amd.com>]

Hi Chung,

On 8/10/2026 11:43 AM, YH Chung wrote:
> Hi Krishnamoorthi,
> 
>> YH Chung, would you be open to collaborating on the slave-side
>> interfaces of the new eSPI framework? Happy to discuss further on the
>> list or off-list to align on the design before the next revision.
> 
> Thanks for reaching out. I am glad to share some design considerations
> from a target-side point of view.
> 
> After reading the series, I think it might be useful to align on the
> layering between the eSPI core, channel implementations, and hardware
> drivers as part of defining the target-side interfaces.
> 
> 1. Reuse existing kernel subsystems for the individual channels
> 
> I agree that we should reuse existing kernel subsystems where their
> semantics match, for example GPIO for general-purpose Virtual Wire
> groups, MCTP for MCTP-over-OOB, and MTD for flash access. Other Virtual
> Wire groups and OOB protocols may need different consumers.
> 
> The eSPI subsystem could provide common adapters between those
> subsystems and the corresponding channel instead of requiring each
> controller or target hardware driver to implement the integration
> independently.
> 
> For example, Controller Attached Flash Sharing (CAFS) and Target
> Attached Flash Sharing (TAFS) have different ownership and transaction
> directions, but share the eSPI Flash packet and request/completion
> semantics. A common Flash layer could provide that protocol handling,
> with a requester-side MTD frontend that turns MTD operations into eSPI
> requests and a provider-side backend that services requests using
> locally attached flash.

Reusing GPIO for VWire, MCTP for OOB, and MTD for Flash where semantics 
match is the right direction.

For CAFS/TAFS, the same espi-flash channel type is used on both sides: a 
requester role exposing an MTD frontend (controller owns flash), and a 
provider role servicing requests from local flash (target owns flash). 
It's the same channel/protocol layer and packet/completion semantics on 
each side; on any given endpoint, only one role is active.

> 
> 2. Consider the boundary between channel semantics and hardware transport
> 
> The current struct espi_controller_ops exposes high-level operations
> such as periph_io_read(), oob_send(), and flash_read(). These may be
> useful as channel-consumer APIs, but I wonder whether they are too
> high-level for the hardware-driver interface itself.
> 
> Would it make sense to keep high-level behavior in the channel layers
> while defining the hardware-facing boundary in terms of per-channel
> transmit and receive primitives?
> 
> Conceptually:
> 
>      TX: core/channel -> *_tx()      -> hardware
>      RX: hardware IRQ -> espi_*_rx() -> core/channel
> 
> The packet or request structure could remain channel-specific. The main
> idea is to keep register, FIFO, and DMA handling below this boundary and
> eSPI channel semantics above it.
> 
> For example, a controller read from target-attached flash could use the
> same transport interface on both sides:
> 
>      Controller (requester)        Target (flash owner)
>      ----------------------             --------------------
> 
>      MTD frontend
>         |
>      Flash layer
>         |
>      build READ request
>         |
>      flash_tx()    ------- READ ------->   espi_flash_rx()
>                                      |
>                                   Flash provider
>                                      |
>                                   local MTD read
>                                      |
>      espi_flash_rx() <-- COMPLETION --- flash_tx()
>         |
>      match request
>         |
>      complete MTD read
> 
> One option would be for controller and target drivers to use the same
> low-level endpoint interface, including the same per-channel *_tx()
> callbacks and espi_*_rx() entry points. A common endpoint object could
> carry the role, for example:
> 
>      enum espi_role {
>          ESPI_ROLE_CONTROLLER,
>          ESPI_ROLE_TARGET,
>      };
> 
> The endpoint role and capabilities would determine which transaction
> types are valid and which optional operations are implemented. Object
> lifetime, capabilities, packet definitions, and request state could
> also be shared. Linux SPI's shared spi_controller infrastructure for
> host and target roles may be a useful reference, although eSPI's
> role-specific protocol behavior is more asymmetric.
> 
> The benefit of this boundary is that common packet and channel protocol
> handling can be implemented once while each hardware driver remains
> focused on its registers, FIFOs, DMA, and interrupts. It should reduce
> duplication and role-specific divergence, make support for additional
> controller or target hardware easier to add, and allow both roles to be
> tested against the same transport contract.
> 
> Channel-independent commands such as GET/SET_CONFIGURATION and
> GET_STATUS may likewise need role-specific callbacks within this common
> endpoint interface: command submission/completion on the controller
> side and configuration-provider callbacks on the target side. These
> callbacks could be optional to support hardware-assisted
> implementations.

The TX/RX boundary is a clean separation and we agree hardware drivers 
should focus purely on registers, FIFOs and DMA while channel semantics 
live above. That means the current high-level ops (flash_read(), 
oob_send(), ...) move up into the channel layers, and the hardware 
boundary becomes your per-channel *_tx() / espi_*_rx() primitives.

As an alternative to a shared espi_role endpoint, we are considering one
device per channel under the target (CS#), single target shown for
clarity:

     espi0                     (controller)
     `-- espi0-cs0             (target at Chip Select 0)
         |-- espi0-cs0-periph  -> I/O + memory
         |-- espi0-cs0-vwire   -> gpiochip (GP VWire groups; system
         |                        VWires handled separately)
         |-- espi0-cs0-oob     -> MCTP
         `-- espi0-cs0-flash   -> MTD

Both controller and target drivers populate the same per-channel ops and 
the role (requester/provider) stays local to each channel driver and 
decides direction and which optional callbacks exist. The 
channel-independent commands (GET/SET_CONFIGURATION, GET_STATUS) are 
link-level, so they stay on the controller device with role-specific 
callbacks. This gives the same separation as your endpoint model.

> 
> 3. Leave room for asynchronous deferred transaction handling
> 
> This does not necessarily need to be implemented in the initial
> framework. A synchronous API may be a practical first step, provided
> the hardware-facing interface does not prevent asynchronous handling
> from being added later.
> 
> As a future improvement, the common channel layer could track
> outstanding non-posted Peripheral and Flash requests and complete them
> when the corresponding completion packets arrive. For tagged requests,
> this state would be scoped by endpoint/Chip Select#, channel, and tag.
> A complete request object would also need to handle split completions,
> timeouts, errors, and cancellation during channel or link reset.
> 

Agreed on synchronous first. We will keep the boundary open for later 
async tracking of non-posted requests, scoped by CS#/channel/tag.

Thanks for the detailed design considerations. Do you have any 
comments/suggestions about the per channel approach as a starting point? 
Happy to continue on or off the list.

Thanks,
Krishna

> The exact model could remain channel-specific because OOB is
> message-oriented and Virtual Wire is event/state-oriented. Keeping this
> possibility open would allow asynchronous handling to be added later
> without changing the hardware-driver interface.
> 
> These are my initial thoughts from the target-side implementation
> perspective. I hope they are useful when considering the layering and
> public interfaces for the next revision, and I would be interested in
> your thoughts on the proposed transport boundary and common endpoint
> model.
> 
> Regards,
> Yun-Hsuan Chung