Re: [PATCH v2 1/4] soc: qcom: rpmh: Allow non-child devices to issue write commands

[Bartosz Golaszewski]

On Fri, 29 May 2026 03:05:35 +0200, Fenglin Wu
<fenglin.wu@xxxxxxxxxxxxxxxx> said:
> Currently, the RPMH driver only allows child devices of the RPMH
> controller to issue commands, as it assumes dev->parent points to the
> RSC device.
>
> There is a possibility that certain devices which are not children of
> the RPMH controller want to send commands for special control at the
> RPMH side. For example, in PMH0101 PMICs, there are bidirectional
> level shifter (LS) peripherals, and each LS works with a pair of PMIC
> GPIOs. The control of the LS, which is combined with the GPIO
> configuration, is handled by RPMH firmware for sharing the resource
> between different subsystems. From a hardware point of view, the LS
> functionality is tied to a pair of PMIC GPIOs, so its control is more
> suitable to be added in the pinctrl-spmi-gpio driver by adding the
> level-shifter function. However, the pinctrl-spmi-gpio device is a
> child device of the SPMI controller, not the RPMH controller.
>
> This patch extends the RPMH driver to support write commands from any
> device that has a pointer to the RPMH controller device:
>
> 1. Add rpmh_get_ctrlr_dev() to lookup controller via device tree
>    phandle "qcom,rpmh"
> 2. Add new APIs: rpmh_write_async_ctrlr() and rpmh_write_ctrlr()
>    that accept controller device pointer directly
>
> With this change, the pinctrl-spmi-gpio driver is able to issue write
> commands to the RPMH controller by using the controller device pointer,
> and vote for enabling the level-shifter function.
>
> Signed-off-by: Fenglin Wu <fenglin.wu@xxxxxxxxxxxxxxxx>
> ---
>  drivers/soc/qcom/rpmh.c | 173 +++++++++++++++++++++++++++++++++++++++++++-----
>  include/soc/qcom/rpmh.h |  21 ++++++
>  2 files changed, 179 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
> index ca37da3dc2b1..9dbc42b775d9 100644
> --- a/drivers/soc/qcom/rpmh.c
> +++ b/drivers/soc/qcom/rpmh.c
> @@ -12,6 +12,7 @@
>  #include <linux/lockdep.h>
>  #include <linux/module.h>
>  #include <linux/of.h>
> +#include <linux/of_platform.h>
>  #include <linux/platform_device.h>
>  #include <linux/slab.h>
>  #include <linux/spinlock.h>
> @@ -76,6 +77,21 @@ static struct rpmh_ctrlr *get_rpmh_ctrlr(const struct device *dev)
>  	return &drv->client;
>  }
>
> +static struct rpmh_ctrlr *get_rpmh_ctrlr_from_dev(const struct device *ctrl_dev)
> +{
> +	struct rsc_drv *drv;
> +
> +	if (!ctrl_dev)
> +		return ERR_PTR(-EINVAL);
> +
> +	drv = dev_get_drvdata(ctrl_dev);
> +
> +	if (!drv)
> +		return ERR_PTR(-ENODEV);
> +
> +	return &drv->client;
> +}
> +
>  void rpmh_tx_done(const struct tcs_request *msg)
>  {
>  	struct rpmh_request *rpm_msg = container_of(msg, struct rpmh_request,
> @@ -156,23 +172,11 @@ static struct cache_req *cache_rpm_request(struct rpmh_ctrlr *ctrlr,
>  	return req;
>  }
>
> -/**
> - * __rpmh_write: Cache and send the RPMH request
> - *
> - * @dev: The device making the request
> - * @state: Active/Sleep request type
> - * @rpm_msg: The data that needs to be sent (cmds).
> - *
> - * Cache the RPMH request and send if the state is ACTIVE_ONLY.
> - * SLEEP/WAKE_ONLY requests are not sent to the controller at
> - * this time. Use rpmh_flush() to send them to the controller.
> - */
> -static int __rpmh_write(const struct device *dev, enum rpmh_state state,
> -			struct rpmh_request *rpm_msg)
> +static int __rpmh_write_direct(struct rpmh_ctrlr *ctrlr, enum rpmh_state state,
> +			       struct rpmh_request *rpm_msg)
>  {
> -	struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev);
> -	int ret = -EINVAL;
>  	struct cache_req *req;
> +	int ret = -EINVAL;
>  	int i;
>
>  	/* Cache the request in our store and link the payload */
> @@ -193,6 +197,25 @@ static int __rpmh_write(const struct device *dev, enum rpmh_state state,
>  	return ret;
>  }
>
> +/**
> + * __rpmh_write: Cache and send the RPMH request
> + *
> + * @dev: The device making the request
> + * @state: Active/Sleep request type
> + * @rpm_msg: The data that needs to be sent (cmds).
> + *
> + * Cache the RPMH request and send if the state is ACTIVE_ONLY.
> + * SLEEP/WAKE_ONLY requests are not sent to the controller at
> + * this time. Use rpmh_flush() to send them to the controller.
> + */
> +static int __rpmh_write(const struct device *dev, enum rpmh_state state,
> +			struct rpmh_request *rpm_msg)
> +{
> +	struct rpmh_ctrlr *ctrlr = get_rpmh_ctrlr(dev);
> +
> +	return __rpmh_write_direct(ctrlr, state, rpm_msg);
> +}
> +
>  static int __fill_rpmh_msg(struct rpmh_request *req, enum rpmh_state state,
>  		const struct tcs_cmd *cmd, u32 n)
>  {
> @@ -271,6 +294,126 @@ int rpmh_write(const struct device *dev, enum rpmh_state state,
>  }
>  EXPORT_SYMBOL_GPL(rpmh_write);
>
> +static void rpmh_put_device(void *data)
> +{

Can you cast it here to struct device * to make it clear what is being
released?

> +	put_device(data);
> +}
> +
> +/**
> + * rpmh_get_ctrlr_dev: Get RPMH controller device from device tree
> + *
> + * @dev: Device with "qcom,rpmh" phandle property
> + *
> + * Returns: Pointer to RPMH controller device, with a devm action registered
> + * on @dev to release the reference when @dev is unbound.

Then I believe it should be called devm_rpmh_get_ctrlr_dev() with the non-devm
variant not registering the devres action.

> + */
> +struct device *rpmh_get_ctrlr_dev(struct device *dev)
> +{
> +	struct device_node *rpmh_np;
> +	struct platform_device *pdev;
> +	struct device_link *link;
> +	int ret;
> +
> +	rpmh_np = of_parse_phandle(dev->of_node, "qcom,rpmh", 0);
> +	if (!rpmh_np)
> +		return ERR_PTR(-ENODEV);
> +
> +	pdev = of_find_device_by_node(rpmh_np);
> +	of_node_put(rpmh_np);
> +
> +	if (!pdev)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	link = device_link_add(dev, &pdev->dev,
> +			       DL_FLAG_AUTOREMOVE_CONSUMER | DL_FLAG_PM_RUNTIME);

This should already be covered by driver core as there already exists a phandle
link from dev->of_node to rpmh_np.

> +	if (!link) {
> +		put_device(&pdev->dev);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	ret = devm_add_action_or_reset(dev, rpmh_put_device, &pdev->dev);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	return &pdev->dev;
> +}
> +EXPORT_SYMBOL_GPL(rpmh_get_ctrlr_dev);
> +
> +/**
> + * rpmh_write_async_ctrlr: Write RPMH commands with the controller device pointer
> + *
> + * @ctrl_dev: The RPMH controller device
> + * @state: Active/sleep set
> + * @cmd: The payload data
> + * @n: The number of elements in payload
> + *
> + * Write a set of RPMH commands, the order of commands is maintained
> + * and will be sent as a single shot.

s/shot/batch/?

> + */
> +int rpmh_write_async_ctrlr(const struct device *ctrl_dev, enum rpmh_state state,
> +			   const struct tcs_cmd *cmd, u32 n)
> +{

The signature of this is the same as that of rpmh_write_async() and the two
functions share a lot of code. Can we not rework rpmh_write_async() to
internally check if it needs to call get_rpmh_ctrlr_from_dev() and avoid
introducing new interfaces?

> +	struct rpmh_request *rpm_msg;
> +	struct rpmh_ctrlr *ctrlr;
> +	int ret;
> +
> +	ctrlr = get_rpmh_ctrlr_from_dev(ctrl_dev);
> +	if (IS_ERR(ctrlr))
> +		return PTR_ERR(ctrlr);
> +
> +	rpm_msg = kzalloc_obj(*rpm_msg, GFP_ATOMIC);
> +	if (!rpm_msg)
> +		return -ENOMEM;
> +	rpm_msg->needs_free = true;
> +
> +	ret = __fill_rpmh_msg(rpm_msg, state, cmd, n);
> +	if (ret) {
> +		kfree(rpm_msg);
> +		return ret;
> +	}
> +
> +	return __rpmh_write_direct(ctrlr, state, rpm_msg);
> +}
> +EXPORT_SYMBOL_GPL(rpmh_write_async_ctrlr);
> +
> +/**
> + * rpmh_write_ctrlr: Write RPMH commands and block until response,
> + * with the controller device pointer
> + *
> + * @ctrlr_dev: The RPMH controller device
> + * @state: Active/sleep set
> + * @cmd: The payload data
> + * @n: The number of elements in @cmd
> + *
> + * May sleep. Do not call from atomic contexts.

Add might_sleep() at the top of the function?

> + */
> +int rpmh_write_ctrlr(const struct device *ctrlr_dev, enum rpmh_state state,
> +		     const struct tcs_cmd *cmd, u32 n)
> +{
> +	DECLARE_COMPLETION_ONSTACK(compl);
> +	/* dev is unused in the synchronous non-batch path; pass NULL */
> +	DEFINE_RPMH_MSG_ONSTACK(NULL, state, &compl, rpm_msg);
> +	struct rpmh_ctrlr *ctrlr;
> +	int ret;
> +
> +	ctrlr = get_rpmh_ctrlr_from_dev(ctrlr_dev);
> +	if (IS_ERR(ctrlr))
> +		return PTR_ERR(ctrlr);
> +
> +	ret = __fill_rpmh_msg(&rpm_msg, state, cmd, n);
> +	if (ret)
> +		return ret;
> +
> +	ret = __rpmh_write_direct(ctrlr, state, &rpm_msg);
> +	if (ret)
> +		return ret;
> +
> +	ret = wait_for_completion_timeout(&compl, RPMH_TIMEOUT_MS);
> +	WARN_ON(!ret);
> +	return (ret > 0) ? 0 : -ETIMEDOUT;
> +}
> +EXPORT_SYMBOL_GPL(rpmh_write_ctrlr);
> +
>  static void cache_batch(struct rpmh_ctrlr *ctrlr, struct batch_cache_req *req)
>  {
>  	unsigned long flags;
> diff --git a/include/soc/qcom/rpmh.h b/include/soc/qcom/rpmh.h
> index bdbee1a97d36..90ddcd7ca2fe 100644
> --- a/include/soc/qcom/rpmh.h
> +++ b/include/soc/qcom/rpmh.h
> @@ -22,6 +22,14 @@ int rpmh_write_batch(const struct device *dev, enum rpmh_state state,
>
>  void rpmh_invalidate(const struct device *dev);
>
> +struct device *rpmh_get_ctrlr_dev(struct device *dev);
> +
> +int rpmh_write_async_ctrlr(const struct device *ctrl_dev, enum rpmh_state state,
> +			   const struct tcs_cmd *cmd, u32 n);
> +
> +int rpmh_write_ctrlr(const struct device *ctrlr_dev, enum rpmh_state state,
> +		     const struct tcs_cmd *cmd, u32 n);
> +
>  #else
>
>  static inline int rpmh_write(const struct device *dev, enum rpmh_state state,
> @@ -42,6 +50,19 @@ static inline void rpmh_invalidate(const struct device *dev)
>  {
>  }
>
> +static inline struct device *rpmh_get_ctrlr_dev(struct device *dev)
> +{ return ERR_PTR(-ENODEV); }
> +
> +static inline int rpmh_write_async_ctrlr(const struct device *ctrl_dev,
> +					  enum rpmh_state state,
> +					  const struct tcs_cmd *cmd, u32 n)
> +{ return -ENODEV; }
> +
> +static inline int rpmh_write_ctrlr(const struct device *ctrlr_dev,
> +				    enum rpmh_state state,
> +				    const struct tcs_cmd *cmd, u32 n)
> +{ return -ENODEV; }
> +
>  #endif /* CONFIG_QCOM_RPMH */
>
>  #endif /* __SOC_QCOM_RPMH_H__ */
>
> --
> 2.43.0
>
>

Bartosz