* [PATCH 1/3] soc/qcom: rpmh: properly fix synchronous requests
2026-04-13 11:06 [PATCH 0/3] Qualcomm: rpmh and regulator fixes Casey Connolly
@ 2026-04-13 11:06 ` Casey Connolly
2026-04-13 11:06 ` [PATCH 2/3] soc/qcom: rpmh: only allow rpmh writes of a single command Casey Connolly
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Casey Connolly @ 2026-04-13 11:06 UTC (permalink / raw)
To: Sumit Garg, u-boot-qcom, u-boot
Cc: Casey Connolly, Neil Armstrong, Tom Rini, Jaehoon Chung, Peng Fan,
Aswin Murugan
We only ever use a single TCS group and it's always AMC. Disabling AMC
immediately after triggering the TCS group might be contributing to some
of the issues we hit, so let's just defer cleaning things up until just
before we boot the OS. The CMD_ENABLE register is cleared for every rpmh
transation to only select the appropriate commands, and it seems like we
don't need to unset the trigger bit for an AMC either.
Signed-off-by: Casey Connolly <casey.connolly@linaro.org>
---
drivers/soc/qcom/rpmh-rsc.c | 70 ++++++++++++++++++++++++++++++++++-----------
1 file changed, 53 insertions(+), 17 deletions(-)
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 0b821cc6f9d8..5bf54429e793 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -293,9 +293,9 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
if (msg->wait_for_compl)
cmd_msgid |= CMD_MSGID_RESP_REQ;
- cmd_complete = read_tcs_reg(drv, drv->regs[RSC_DRV_CMD_WAIT_FOR_CMPL], tcs_id);
+ cmd_complete = 0; //read_tcs_reg(drv, drv->regs[RSC_DRV_CMD_WAIT_FOR_CMPL], tcs_id);
for (i = 0, j = cmd_id; i < msg->num_cmds; i++, j++) {
cmd = &msg->cmds[i];
cmd_enable |= BIT(j);
@@ -306,17 +306,20 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
write_tcs_cmd(drv, drv->regs[RSC_DRV_CMD_MSGID], tcs_id, j, msgid);
write_tcs_cmd(drv, drv->regs[RSC_DRV_CMD_ADDR], tcs_id, j, cmd->addr);
if (!msg->is_read)
write_tcs_cmd(drv, drv->regs[RSC_DRV_CMD_DATA], tcs_id, j, cmd->data);
- debug("tcs(%d): [%s] cmd_id: %d: msgid: %#x addr: %#x data: %#x complete: %#x\n",
+ debug("tcs(%d): [%s] cmd_id: %d: msgid: %#x addr: %#x data: %#x read: %d\n",
tcs_id, msg->state == RPMH_ACTIVE_ONLY_STATE ? "active" : "?", j, msgid,
- cmd->addr, cmd->data, cmd_complete);
+ cmd->addr, cmd->data, msg->is_read);
}
- cmd_enable |= read_tcs_reg(drv, drv->regs[RSC_DRV_CMD_ENABLE], tcs_id);
write_tcs_reg(drv, drv->regs[RSC_DRV_CMD_ENABLE], tcs_id, cmd_enable);
/* U-Boot: Tell the DRV to wait for completion (?) so we can poll on DRV_STATUS */
/* This register applies to the entire TCS group not per command */
+ /*
+ * FIXME: this seems to rather be a way to impose ordering on commands when multiple are
+ * sent in a single TCS request, needs more testing.
+ */
write_tcs_reg(drv, drv->regs[RSC_DRV_CMD_WAIT_FOR_CMPL], tcs_id, cmd_complete);
}
/**
@@ -343,15 +346,14 @@ static void __tcs_set_trigger(struct rsc_drv *drv, int tcs_id, bool trigger)
/*
* HW req: Clear the DRV_CONTROL and enable TCS again
* While clearing ensure that the AMC mode trigger is cleared
- * and then the mode enable is cleared.
+ * U-Boot: keep AMC_MODE_ENABLE flag since clearing it might be
+ * interrupting in-flight commands.
*/
enable = read_tcs_reg(drv, reg, tcs_id);
enable &= ~TCS_AMC_MODE_TRIGGER;
write_tcs_reg_sync(drv, reg, tcs_id, enable);
- enable &= ~TCS_AMC_MODE_ENABLE;
- write_tcs_reg_sync(drv, reg, tcs_id, enable);
if (trigger) {
/* Enable the AMC mode on the TCS and then trigger the TCS */
enable = TCS_AMC_MODE_ENABLE;
@@ -387,12 +389,13 @@ int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
{
struct tcs_group *tcs;
int tcs_id, i = 0;
u32 val;
+ u32 addr;
tcs = get_tcs_for_msg(drv, msg);
- if (IS_ERR_OR_NULL(tcs))
- return 0;
+ if (IS_ERR(tcs))
+ return PTR_ERR(tcs);
/* U-Boot is single-threaded, always use the first TCS as we'll never conflict */
tcs_id = tcs->offset;
if (!read_tcs_reg(drv, drv->regs[RSC_DRV_STATUS], tcs_id)) {
@@ -400,9 +403,8 @@ int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
return -EBUSY;
}
tcs->req[tcs_id - tcs->offset] = msg;
- generic_set_bit(tcs_id, drv->tcs_in_use);
/*
* These two can be done after the lock is released because:
* - We marked "tcs_in_use" under lock.
@@ -413,28 +415,35 @@ int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
*/
__tcs_buffer_write(drv, tcs_id, 0, msg);
__tcs_set_trigger(drv, tcs_id, true);
- /* U-Boot: Now wait for the TCS to be cleared, indicating that we're done. */
+ /*
+ * U-Boot: Now wait for the status done flag and for the TCS to be cleared.
+ * Just waiting for one of these isn't always enough. On reads we have to wait for
+ * the status register to indicate that the response data is ready.
+ */
+ for (i = 0; i < USEC_PER_SEC; i++) {
+ addr = readl_relaxed(drv->tcs_base + drv->regs[RSC_DRV_IRQ_STATUS]);
+ if (addr & BIT(tcs_id))
+ break;
+ udelay(1);
+ }
+
for (i = 0; i < USEC_PER_SEC; i++) {
val = read_tcs_cmd(drv, drv->regs[RSC_DRV_CMD_STATUS], tcs_id, 0);
if (val & CMD_STATUS_COMPL)
break;
udelay(1);
}
- /* U-Boot: read the response now we know it's available */
+ /* U-Boot: read the response when it becomes available */
if (msg->is_read) {
msg->cmds[0].data = read_tcs_cmd(drv, drv->regs[RSC_DRV_CMD_RESP_DATA], tcs_id, 0);
log_debug("data response: %#x\n", msg->cmds[0].data);
}
__tcs_set_trigger(drv, tcs_id, false);
-
- /* Reclaim the TCS */
- write_tcs_reg(drv, drv->regs[RSC_DRV_CMD_ENABLE], tcs_id, 0);
writel_relaxed(BIT(tcs_id), drv->tcs_base + drv->regs[RSC_DRV_IRQ_CLEAR]);
- generic_clear_bit(tcs_id, drv->tcs_in_use);
if (i == USEC_PER_SEC) {
log_err("%s: error writing %#x to %d:%#x\n", drv->name,
msg->cmds[0].addr, tcs_id, drv->regs[RSC_DRV_CMD_ADDR]);
@@ -563,8 +572,34 @@ static int rpmh_rsc_probe(struct udevice *dev)
return ret;
}
+static int rpmh_rsc_remove(struct udevice *dev)
+{
+ struct rsc_drv *drv = dev_get_priv(dev);
+ struct tcs_group *tcs = &drv->tcs[ACTIVE_TCS];
+ u32 tcs_id = tcs->offset;
+
+ for (int i = 0; i < 10 && !read_tcs_reg(drv, drv->regs[RSC_DRV_STATUS], tcs_id); i++) {
+ if (i == 0)
+ printf("Waiting for TCS %d to be free...\n", tcs_id);
+ udelay(100);
+ }
+
+ /* Clean up for the OS! */
+ write_tcs_reg(drv, drv->regs[RSC_DRV_CMD_WAIT_FOR_CMPL], tcs_id, 0);
+
+ /* Clear the AMC enable bit for all TCS commands we used */
+ write_tcs_reg_sync(drv, drv->regs[RSC_DRV_CONTROL], tcs_id, 0);
+ /* Disable all commands for the single AMC we used */
+ write_tcs_reg_sync(drv, drv->regs[RSC_DRV_CMD_ENABLE], tcs_id, 0);
+
+ /* Make sure IRQs are all clear too */
+ writel_relaxed(tcs->mask, drv->tcs_base + drv->regs[RSC_DRV_IRQ_CLEAR]);
+
+ return 0;
+}
+
static const struct udevice_id qcom_rpmh_ids[] = {
{ .compatible = "qcom,rpmh-rsc" },
{ }
};
@@ -573,11 +608,12 @@ U_BOOT_DRIVER(qcom_rpmh_rsc) = {
.name = "qcom_rpmh_rsc",
.id = UCLASS_MISC,
.priv_auto = sizeof(struct rsc_drv),
.probe = rpmh_rsc_probe,
+ .remove = rpmh_rsc_remove,
.of_match = qcom_rpmh_ids,
/* rpmh is under CLUSTER_PD which we don't support, so skip trying to enable PDs */
- .flags = DM_FLAG_DEFAULT_PD_CTRL_OFF,
+ .flags = DM_FLAG_DEFAULT_PD_CTRL_OFF | DM_FLAG_OS_PREPARE,
};
MODULE_DESCRIPTION("Qualcomm Technologies, Inc. RPMh Driver");
MODULE_LICENSE("GPL v2");
--
2.51.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 2/3] soc/qcom: rpmh: only allow rpmh writes of a single command
2026-04-13 11:06 [PATCH 0/3] Qualcomm: rpmh and regulator fixes Casey Connolly
2026-04-13 11:06 ` [PATCH 1/3] soc/qcom: rpmh: properly fix synchronous requests Casey Connolly
@ 2026-04-13 11:06 ` Casey Connolly
2026-04-13 11:06 ` [PATCH 3/3] power: regulator: qcom-rpmh: propagate votes to parent supplies Casey Connolly
2026-05-01 5:59 ` [PATCH 0/3] Qualcomm: rpmh and regulator fixes Sumit Garg
3 siblings, 0 replies; 6+ messages in thread
From: Casey Connolly @ 2026-04-13 11:06 UTC (permalink / raw)
To: Sumit Garg, u-boot-qcom, u-boot
Cc: Casey Connolly, Neil Armstrong, Tom Rini, Jaehoon Chung, Peng Fan,
Aswin Murugan
With how we currently handle synchronous RPMh access in U-Boot (being
derived from the async-heavy Linux driver), with available documentation
it doesn't seem to be easy to get multiple commands per TCS request
to work correctly, resulting in a broken state. This likely needs more
testing to find a better fix but for now this seems to resolve issues we
hit in Linux on sm8650.
Signed-off-by: Casey Connolly <casey.connolly@linaro.org>
---
drivers/interconnect/qcom/bcm-voter.c | 16 ++++++++++------
drivers/soc/qcom/rpmh.c | 6 ++++++
2 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/drivers/interconnect/qcom/bcm-voter.c b/drivers/interconnect/qcom/bcm-voter.c
index 361b03f207a9..5d284448fdf4 100644
--- a/drivers/interconnect/qcom/bcm-voter.c
+++ b/drivers/interconnect/qcom/bcm-voter.c
@@ -292,15 +292,19 @@ int qcom_icc_bcm_voter_commit(struct bcm_voter *voter)
tcs_list_gen(voter, QCOM_ICC_BUCKET_AMC, cmds, commit_idx);
if (!commit_idx[0])
goto out;
- for (int i = 0 ; commit_idx[i] ; ++i) {
- ret = rpmh_write(voter->dev, RPMH_ACTIVE_ONLY_STATE,
- &cmds[i], commit_idx[i]);
- if (ret) {
- pr_err("Error sending AMC RPMH requests (%d)\n", ret);
- goto out;
+ /* U-Boot: don't combine votes into a single message since it doesn't work properly */
+ for (int i = 0, k = 0 ; commit_idx[i] ; ++i) {
+ for (int j = 0; j < commit_idx[i]; j++) {
+ ret = rpmh_write(voter->dev, RPMH_ACTIVE_ONLY_STATE,
+ &cmds[k + j], 1);
+ if (ret) {
+ pr_err("Error sending AMC RPMH requests (%d)\n", ret);
+ goto out;
+ }
}
+ k += commit_idx[i];
}
/* TOFIX vote for WAKE & SLEEP ?? */
diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
index b55e23c4417d..685ac5d27cef 100644
--- a/drivers/soc/qcom/rpmh.c
+++ b/drivers/soc/qcom/rpmh.c
@@ -132,8 +132,14 @@ int rpmh_write(const struct udevice *dev, enum rpmh_state state,
{
DEFINE_RPMH_MSG_ONSTACK(dev, state, rpm_msg);
int ret;
+ // FIXME: can't seem to get multiple commands per message to work properly
+ if (n > 1) {
+ pr_err("%s: ERROR: RPMh writes with multiple commands don't work!\n", dev->name);
+ return -EINVAL;
+ }
+
ret = __fill_rpmh_msg(&rpm_msg, state, cmd, n, false);
if (ret)
return ret;
--
2.51.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH 3/3] power: regulator: qcom-rpmh: propagate votes to parent supplies
2026-04-13 11:06 [PATCH 0/3] Qualcomm: rpmh and regulator fixes Casey Connolly
2026-04-13 11:06 ` [PATCH 1/3] soc/qcom: rpmh: properly fix synchronous requests Casey Connolly
2026-04-13 11:06 ` [PATCH 2/3] soc/qcom: rpmh: only allow rpmh writes of a single command Casey Connolly
@ 2026-04-13 11:06 ` Casey Connolly
2026-05-01 5:59 ` [PATCH 0/3] Qualcomm: rpmh and regulator fixes Sumit Garg
3 siblings, 0 replies; 6+ messages in thread
From: Casey Connolly @ 2026-04-13 11:06 UTC (permalink / raw)
To: Sumit Garg, u-boot-qcom, u-boot
Cc: Casey Connolly, Neil Armstrong, Tom Rini, Jaehoon Chung, Peng Fan,
Aswin Murugan
Parse the parent supply data, fetch parent supplies and propagate
set_enable() calls to the parent regulator.
Not sure if necessary but it doesn't hurt
Signed-off-by: Casey Connolly <casey.connolly@linaro.org>
---
drivers/power/regulator/qcom-rpmh-regulator.c | 35 +++++++++++++++++++++++++--
1 file changed, 33 insertions(+), 2 deletions(-)
diff --git a/drivers/power/regulator/qcom-rpmh-regulator.c b/drivers/power/regulator/qcom-rpmh-regulator.c
index 3f0f18454698..de71af9b0f0d 100644
--- a/drivers/power/regulator/qcom-rpmh-regulator.c
+++ b/drivers/power/regulator/qcom-rpmh-regulator.c
@@ -159,8 +159,9 @@ struct rpmh_vreg {
struct udevice *dev;
u32 addr;
const struct rpmh_vreg_hw_data *hw_data;
bool always_wait_for_ack;
+ struct udevice *supply;
int enabled;
bool bypassed;
int uv;
@@ -330,8 +331,26 @@ static int rpmh_regulator_set_enable_state(struct udevice *rdev,
debug("%s: set_enable %d (current %d)\n", rdev->name, enable,
vreg->enabled);
+ if (vreg->enabled <= 0 && !enable)
+ return 0;
+ if (vreg->enabled >= 1 && enable) {
+ vreg->enabled++;
+ return 0;
+ }
+
+ if (vreg->supply) {
+ debug("%s: set supply %s enable state %d\n",
+ rdev->name, vreg->supply->name, enable);
+ ret = regulator_set_enable(vreg->supply, enable);
+ if (ret < 0) {
+ debug("%s: failed to set supply %s enable state %d: %d\n",
+ rdev->name, vreg->supply->name, enable, ret);
+ return ret;
+ }
+ }
+
if (vreg->enabled == -EINVAL &&
vreg->uv != -ENOTRECOVERABLE) {
ret = _rpmh_regulator_vrm_set_value(rdev,
vreg->uv, true);
@@ -339,10 +358,13 @@ static int rpmh_regulator_set_enable_state(struct udevice *rdev,
return ret;
}
ret = rpmh_regulator_send_request(vreg, &cmd, enable);
- if (!ret)
- vreg->enabled = enable;
+ if (!ret) {
+ if (vreg->enabled < 0)
+ vreg->enabled = 0;
+ vreg->enabled += enable ? 1 : -1;
+ }
return ret;
}
@@ -837,8 +859,10 @@ static int rpmh_regulator_probe(struct udevice *dev)
{
const struct rpmh_vreg_init_data *init_data;
struct rpmh_vreg *priv;
struct dm_regulator_uclass_plat *plat_data;
+ int ret;
+ char name[32] = { 0 };
init_data = (const struct rpmh_vreg_init_data *)dev_get_driver_data(dev);
priv = dev_get_priv(dev);
plat_data = dev_get_uclass_plat(dev);
@@ -849,8 +873,15 @@ static int rpmh_regulator_probe(struct udevice *dev)
dev_err(dev, "Failed to read RPMh address for %s\n", dev->name);
return -ENODEV;
}
+ strlcpy(name, init_data->supply_name, sizeof(name));
+ strlcat(name, "-supply", sizeof(name));
+ ret = device_get_supply_regulator(dev->parent, name, &priv->supply);
+ if (ret)
+ printf("Failed to get supply regulator %s for %s: %d\n",
+ init_data->supply_name, dev->name, ret);
+
priv->hw_data = init_data->hw_data;
priv->enabled = -EINVAL;
priv->uv = -ENOTRECOVERABLE;
if (ofnode_read_u32(dev_ofnode(dev), "regulator-initial-mode", &priv->mode))
--
2.51.0
^ permalink raw reply related [flat|nested] 6+ messages in thread