Re: [PATCH 2/7] firmware: rpmi: add support for the SBI MPXY transport

[Yao Zi <me@ziyao.cc>]

On Fri, Jun 26, 2026 at 01:15:43PM -0700, Charles Perry wrote:
> This adds an RPMI transport driver for the Supervisor Binary Interface
> (SBI) message proxy (MPXY) extension [1].
> 
> This is for S-Mode U-Boot where some supervisor software (such as
> OpenSBI) runs in M-Mode and handles the low level communication with the
> PuC.
> 
> [1]: https://github.com/riscv-non-isa/riscv-sbi-doc (chapter 20)
> 
> Signed-off-by: Charles Perry <charles.perry@microchip.com>
> ---
>  arch/riscv/include/asm/sbi.h          |  57 ++++
>  drivers/firmware/rpmi/Kconfig         |   9 +
>  drivers/firmware/rpmi/Makefile        |   1 +
>  drivers/firmware/rpmi/rpmi-sbi-mpxy.c | 449 ++++++++++++++++++++++++++
>  4 files changed, 516 insertions(+)
>  create mode 100644 drivers/firmware/rpmi/rpmi-sbi-mpxy.c

...

> diff --git a/drivers/firmware/rpmi/rpmi-sbi-mpxy.c b/drivers/firmware/rpmi/rpmi-sbi-mpxy.c
> new file mode 100644
> index 000000000000..84af4787187a
> --- /dev/null
> +++ b/drivers/firmware/rpmi/rpmi-sbi-mpxy.c

...

> +/* RPMI message protocol specific MPXY attributes */
> +enum sbi_mpxy_rpmi_attribute_id {
> +	SBI_MPXY_RPMI_ATTR_SERVICEGROUP_ID = SBI_MPXY_ATTR_MSGPROTO_ATTR_START,
> +	SBI_MPXY_RPMI_ATTR_SERVICEGROUP_VERSION,
> +	SBI_MPXY_RPMI_ATTR_IMPL_ID,
> +	SBI_MPXY_RPMI_ATTR_IMPL_VERSION,
> +	SBI_MPXY_RPMI_ATTR_MAX_ID
> +};
> +
> +/*
> + * RPMI specific SBI MPXY channel attributes.
> + */
> +struct sbi_mpxy_rpmi_channel_attrs {

Would it make sense to organize sbi_mpxy_rpmi_channel_attrs as an
array indexed by sbi_mpxy_rpmi_attributed_id subtracted by
SBI_MPXY_RPMI_ATTR_MSGPROTO_ATTR_START? With which we could simplify
rpmi_sbi_mpxy_get_attr().

> +	/* RPMI service group ID */
> +	u32 servicegroup_id;
> +	/* RPMI service group version */
> +	u32 servicegroup_version;
> +	/* RPMI implementation ID */
> +	u32 impl_id;
> +	/* RPMI implementation version */
> +	u32 impl_version;
> +};

...

> +static int rpmi_sbi_mpxy_get_attr(struct rpmi_chan *chan,
> +				  enum rpmi_attribute_id id, u32 *data)
> +{
> +	struct rpmi_sbi_mpxy *mpxy = dev_get_priv(chan->dev);
> +	struct sbi_mpxy_chan_desc *desc = &mpxy->channel_descs[chan->id];
> +
> +	switch (id) {
> +	case RPMI_ATTR_SPEC_VERSION:
> +		*data = desc->attrs.msg_proto_version;
> +		break;
> +	case RPMI_ATTR_MAX_MSG_DATA_SIZE:
> +		*data = desc->max_xfer_len;
> +		break;
> +	case RPMI_ATTR_SERVICEGROUP_ID:
> +		*data = desc->rpmi_attrs.servicegroup_id;
> +		break;
> +	case RPMI_ATTR_SERVICEGROUP_VERSION:
> +		*data = desc->rpmi_attrs.servicegroup_version;
> +		break;
> +	case RPMI_ATTR_IMPL_ID:
> +		*data = desc->rpmi_attrs.impl_id;
> +		break;
> +	case RPMI_ATTR_IMPL_VERSION:
> +		*data = desc->rpmi_attrs.impl_version;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	return 0;
> +}

...

> +static int rpmi_sbi_mpxy_probe(struct udevice *dev)
> +{
> +	struct rpmi_sbi_mpxy *mpxy = dev_get_priv(dev);
> +	struct sbi_mpxy_chan_desc *desc;
> +	unsigned long flags;
> +	struct sbiret sret;
> +	int i, ret;
> +
> +	mpxy->dev = dev;
> +
> +	/* Probe for SBI MPXY extension */
> +	if (sbi_get_spec_version() < sbi_mk_version(1, 0) ||
> +	    sbi_probe_extension(SBI_EXT_MPXY) <= 0) {
> +		dev_info(dev, "SBI MPXY extension not available\n");
> +		return -ENODEV;
> +	}
> +
> +	/* Find-out shared memory size */
> +	ret = rpmi_sbi_mpxy_get_shmem_size(&mpxy->shmem_size);
> +	if (ret) {
> +		dev_err(dev, "failed to get MPXY shared memory size\n");
> +		return -ENODEV;
> +	}
> +
> +	mpxy->shmem = memalign(PAGE_SIZE, mpxy->shmem_size);
> +	if (!mpxy->shmem)
> +		return -ENOMEM;
> +
> +	/* Setup shmem in OVERWRITE mode. (flags[1:0] = 00b) */
> +	flags = 0;
> +	sret = sbi_ecall(SBI_EXT_MPXY, SBI_EXT_MPXY_SET_SHMEM,
> +			 virt_to_phys(mpxy->shmem), 0, flags, 0, 0, 0);
> +	if (sret.error) {
> +		ret = sbi_to_linux_error(sret.error);
> +		goto err;
> +	}
> +
> +	ret = rpmi_sbi_mpxy_get_channel_ids(mpxy);
> +	if (ret)
> +		goto err;
> +
> +	for (i = 0; i < mpxy->channel_count; i++) {
> +		desc = &mpxy->channel_descs[i];
> +		desc->max_xfer_len =
> +			min((u32)mpxy->shmem_size, desc->attrs.msg_max_len);
> +	}
> +
> +	return 0;
> +
> +err:
> +	if (mpxy->channel_descs)
> +		free(mpxy->channel_descs);
> +
> +	if (mpxy->shmem)
> +		free(mpxy->shmem);

free() is no-op when the argument is NULL, so the check is redundant.
This applies to rpmi_sbi_mpxy_remove() and other similar cases in the
series, too.

> +
> +	return ret;
> +}
> +
> +static int rpmi_sbi_mpxy_remove(struct udevice *dev)
> +{
> +	struct rpmi_sbi_mpxy *mpxy = dev_get_priv(dev);
> +
> +	if (mpxy->channel_descs)
> +		free(mpxy->channel_descs);
> +
> +	if (mpxy->shmem)
> +		free(mpxy->shmem);
> +
> +	return 0;
> +}

Regards,
Yao Zi