Re: [PATCH 1/7] firmware: add support for RPMI

[Yao Zi <me@ziyao.cc>]

On Fri, Jun 26, 2026 at 01:15:42PM -0700, Charles Perry wrote:
> RISC-V Platform Management Interface (RPMI) defines an OS-agnostic
> protocol for communication between an Application Processor (AP) and a
> platform microcontroller (PuC) [1]
> 
> Add UCLASS_RPMI to abstract the implementation details of an RPMI
> transport layer. A transport driver should implement the struct
> rpmi_transport_ops callback interface while an RPMI client driver should
> use struct rpmi_chan and the rpmi_* API:
> 
>   * rpmi_get_by_index()
>   * rpmi_open()
>   * rpmi_close()
>   * rpmi_get_attr()
>   * rpmi_send_with_resp()
>   * rpmi_send_posted()
> 
> The most important part of the API is rpmi_send_with_resp() and
> rpmi_send_posted() which is used to send a message and possibly receive
> a response from the firmware.
> 
> rpmi_get_attr() can be used to retrieve attributes of the channel like
> the implementation ID or the maximum message size.  This is to retrieve
> information that lives outside of the service group of the rpmi_chan,
> either in the "base service group" or in the transport layer itself.
> 
> rpmi_get_by_index() can be used to initialize a struct rpmi_chan using
> info from the device tree.
> 
> rpmi_open() and rpmi_close() must be called once at initialization and
> deinitialization by clients to let the transport layer do some stateful
> things if needed.
> 
> A helper function is also added: rpmi_check_versions() which is just a
> wrapper around rpmi_get_attr() to do some basic version checking that
> all clients should perform.
> 
> [1]: https://github.com/riscv-non-isa/riscv-rpmi
> 
> Signed-off-by: Charles Perry <charles.perry@microchip.com>
> ---
>  MAINTAINERS                         |   6 ++
>  drivers/firmware/Kconfig            |   1 +
>  drivers/firmware/Makefile           |   1 +
>  drivers/firmware/rpmi/Kconfig       |   9 ++
>  drivers/firmware/rpmi/Makefile      |   1 +
>  drivers/firmware/rpmi/rpmi-uclass.c | 138 ++++++++++++++++++++++++
>  include/dm/uclass-id.h              |   1 +
>  include/rpmi-uclass.h               |  79 ++++++++++++++
>  include/rpmi.h                      | 159 ++++++++++++++++++++++++++++
>  include/rpmi_proto.h                |  63 +++++++++++
>  10 files changed, 458 insertions(+)
>  create mode 100644 drivers/firmware/rpmi/Kconfig
>  create mode 100644 drivers/firmware/rpmi/Makefile
>  create mode 100644 drivers/firmware/rpmi/rpmi-uclass.c
>  create mode 100644 include/rpmi-uclass.h
>  create mode 100644 include/rpmi.h
>  create mode 100644 include/rpmi_proto.h

...

> diff --git a/drivers/firmware/rpmi/rpmi-uclass.c b/drivers/firmware/rpmi/rpmi-uclass.c
> new file mode 100644
> index 000000000000..b725f9e59441
> --- /dev/null
> +++ b/drivers/firmware/rpmi/rpmi-uclass.c
> @@ -0,0 +1,138 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2026 Microchip Technology Inc. All rights reserved.
> + */
> +
> +#define LOG_CATEGORY UCLASS_RPMI
> +
> +#include <dm.h>
> +#include <log.h>
> +#include <rpmi-uclass.h>
> +#include <dm/device_compat.h>

Should we sort the headers? This applies to rest of the series, too.

...

> +int rpmi_check_versions(struct rpmi_chan *chan, u32 min_proto_version,
> +			u32 servicegroup_id_expect,
> +			u32 min_servicegroup_version)
> +{
> +	u32 proto_version, servicegroup_id, servicegroup_version;
> +	int ret;
> +
> +	ret = rpmi_get_attr(chan, RPMI_ATTR_SPEC_VERSION, &proto_version);
> +	if (ret)
> +		return ret;
> +
> +	if (proto_version < min_proto_version) {
> +		dev_err(chan->dev, "protocol version mismatch\n");

The error is caused by a "too old" protocol version instead of a
"mismatched" one, shouldn't we be precise here?

Furthermore, it would be good to print the the expected and implemented
versions as well to ease debugging. debug() could be used so we don't
get the binary size increased in a normal build. So do the
servicegroup_id and servicegroup_version's case.

> +		return -EINVAL;
> +	}
> +
> +	ret = rpmi_get_attr(chan, RPMI_ATTR_SERVICEGROUP_ID, &servicegroup_id);
> +	if (ret)
> +		return ret;
> +
> +	if (servicegroup_id != servicegroup_id_expect) {
> +		dev_err(chan->dev, "service group match fail\n");
> +		return -EINVAL;
> +	}
> +
> +	ret = rpmi_get_attr(chan, RPMI_ATTR_SERVICEGROUP_VERSION,
> +			    &servicegroup_version);
> +	if (ret)
> +		return ret;
> +
> +	if (servicegroup_version < min_servicegroup_version) {
> +		dev_err(chan->dev, "service group version mismatch\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}

...

> diff --git a/include/rpmi.h b/include/rpmi.h
> new file mode 100644
> index 000000000000..de802026b31a
> --- /dev/null
> +++ b/include/rpmi.h

...

> +static inline int rpmi_to_linux_error(int rpmi_error)
> +{
> +	switch (rpmi_error) {
> +	case RPMI_SUCCESS:
> +		return 0;
> +	case RPMI_ERR_INVALID_PARAM:
> +	case RPMI_ERR_BAD_RANGE:
> +	case RPMI_ERR_INVALID_STATE:
> +		return -EINVAL;
> +	case RPMI_ERR_DENIED:
> +		return -EPERM;
> +	case RPMI_ERR_INVALID_ADDR:
> +	case RPMI_ERR_HW_FAULT:
> +		return -EFAULT;

RPMI_ERR_HW_FAULT means "failed due to hardware fault" according to
RPMI specification, does it really count as "-EFAULT" which is
"bad address"?

> +	case RPMI_ERR_ALREADY:
> +		return -EALREADY;
> +	case RPMI_ERR_BUSY:
> +		return -EBUSY;
> +	case RPMI_ERR_TIMEOUT:
> +		return -ETIMEDOUT;
> +	case RPMI_ERR_IO:
> +		return -ECOMM;
> +	case RPMI_ERR_FAILED:
> +	case RPMI_ERR_NOTSUPP:
> +	case RPMI_ERR_NO_DATA:
> +	case RPMI_ERR_EXTENSION:
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}

Regards,
Yao Zi