In order to review Maulik's latest "rpmh_flush for non OSI targets"
patch series I've found myself trying to understand rpmh-rsc better.
To make it easier for others to do this in the future, add a whole lot
of comments / documentation.
As part of this there are a very small number of functional changes.
- We'll get a tiny performance boost by getting rid of the "cmd_cache"
which I believe was unnecessary.
- We now assume someone else is in charge of exclusivity for
tcs_invalidate() and have removed a lock in there. As per the
comments in the patch, this isn't expected to cause problems.
- tcs_is_free() no longer checks hardware state, but we think it
didn't need to.
These changes touch a lot of code in rpmh-rsc, so hopefully someone at
Qualcomm can test them out better than I did (I don't have every last
client of RPMH in my tree).
I've tried to structure the patches so that simpler / less
controversial patches are first. Those could certainly land on their
own without later patches. Many of the patches could also be dropped
and the others would still apply if they are controversial. If you
need help doing this then please yell.
These patches are based on Maulik's v16 series, AKA:
https://lore.kernel.org/r/[email protected]/
With all that, enjoy.
Changes in v3:
- ("...are not for IRQ") is new for v3.
- ("Don't double-check rpmh") replaces ("Warning if tcs_write...")
- Add "TCS" in title (Maulik).
- Adjusted comments for rpmh_rsc_write_ctrl_data().
- Comments for new enable_tcs_irq() function.
- Comments for new rpmh_rsc_cpu_pm_callback() function.
- Extra blank line removed (Maulik).
- IRQ registers aren't in TCS0 (Maulik).
- Kill find_match moves from patch #9 to patch #5 (Maulik).
- Mention in message that I also fixed up kernel-doc stuff.
- Moved comments patch after ("Kill cmd_cache and find_match...").
- One space after a period now (Maulik).
- Plural of TCS fixed to TCSes following Maulik's example.
- Re-added comment in tcs_write() about checking for same address.
- Rebased atop v16 ('Invoke rpmh_flush...') series.
- Replace ("...warn if state mismatch") w/ ("...just check tcs_in_use")
- Replaced ("irqsave()...") + ("...never -EBUSY") w/ ("Caller handles...")
- Rewrote commit message to adjust for patch order.
- __tcs_set_trigger() comments adjusted now that it can set or unset.
- get_tcs_for_msg() documents why it's safe to borrow the wake TCS.
- get_tcs_for_msg() no longer returns -EAGAIN.
Changes in v2:
- Comment tcs_is_free() new for v2; replaces old patch 6.
- Document bug of tcs_write() not handling -EAGAIN.
- Document get_tcs_for_msg() => -EAGAIN only for ACTIVE_ONLY.
- Document locks for updating "tcs_in_use" more.
- Document tcs_is_free() without drv->lock OK for tcs_invalidate().
- Document that rpmh_rsc_send_data() can be an implicit invalidate.
- Document two get_tcs_for_msg() issues if zero-active TCS.
- Fixed documentation of "tcs" param in find_slots().
- Got rid of useless "if (x) continue" at end of for loop.
- More clear that active-only xfers can happen on wake TCS sometimes.
- Now prose in comments instead of struct definitions.
- Pretty ASCII art from Stephen.
- Reword tcs_write() doc a bit.
Douglas Anderson (10):
drivers: qcom: rpmh-rsc: Clean code reading/writing TCS regs/cmds
drivers: qcom: rpmh-rsc: Document the register layout better
drivers: qcom: rpmh-rsc: Fold tcs_ctrl_write() into its single caller
drivers: qcom: rpmh-rsc: Remove get_tcs_of_type() abstraction
drivers: qcom: rpmh-rsc: Kill cmd_cache and find_match() with fire
drivers: qcom: rpmh-rsc: A lot of comments
drivers: qcom: rpmh-rsc: tcs_is_free() can just check tcs_in_use
drivers: qcom: rpmh-rsc: Don't double-check rpmh
drivers: qcom: rpmh-rsc: Caller handles tcs_invalidate() exclusivity
drivers: qcom: rpmh-rsc: read_tcs_reg()/write_tcs_reg() are not for
IRQ
drivers/soc/qcom/rpmh-internal.h | 66 +++--
drivers/soc/qcom/rpmh-rsc.c | 465 +++++++++++++++++++++----------
drivers/soc/qcom/rpmh.c | 5 +-
3 files changed, 363 insertions(+), 173 deletions(-)
--
2.26.0.292.g33ef6b2f38-goog
Perhaps it's just me, it took a really long time to understand what
the register layout of rpmh-rsc was just from the #defines. Let's add
a bunch of comments describing which blocks are part of other blocks.
Signed-off-by: Douglas Anderson <[email protected]>
---
Changes in v3:
- Extra blank line removed (Maulik).
- IRQ registers aren't in TCS0 (Maulik).
- One space after a period now (Maulik).
- Plural of TCS fixed to TCSes following Maulik's example.
- Rebased atop v16 ('Invoke rpmh_flush...') series.
Changes in v2:
- Now prose in comments instead of struct definitions.
- Pretty ASCII art from Stephen.
drivers/soc/qcom/rpmh-rsc.c | 79 ++++++++++++++++++++++++++++++++++---
1 file changed, 74 insertions(+), 5 deletions(-)
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 7d9e2c2f0e27..46455b1d93f1 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -43,14 +43,29 @@
#define DRV_NCPT_MASK 0x1F
#define DRV_NCPT_SHIFT 27
-/* Register offsets */
+/* Offsets for common TCS Registers, one bit per TCS */
#define RSC_DRV_IRQ_ENABLE 0x00
#define RSC_DRV_IRQ_STATUS 0x04
-#define RSC_DRV_IRQ_CLEAR 0x08
-#define RSC_DRV_CMD_WAIT_FOR_CMPL 0x10
+#define RSC_DRV_IRQ_CLEAR 0x08 /* w/o; write 1 to clear */
+
+/*
+ * Offsets for per TCS Registers.
+ *
+ * TCSes start at 0x10 from tcs_base and are stored one after another.
+ * Multiply tcs_id by RSC_DRV_TCS_OFFSET to find a given TCS and add one
+ * of the below to find a register.
+ */
+#define RSC_DRV_CMD_WAIT_FOR_CMPL 0x10 /* 1 bit per command */
#define RSC_DRV_CONTROL 0x14
-#define RSC_DRV_STATUS 0x18
-#define RSC_DRV_CMD_ENABLE 0x1C
+#define RSC_DRV_STATUS 0x18 /* zero if tcs is busy */
+#define RSC_DRV_CMD_ENABLE 0x1C /* 1 bit per command */
+
+/*
+ * Offsets for per command in a TCS.
+ *
+ * Commands (up to 16) start at 0x30 in a TCS; multiply command index
+ * by RSC_DRV_CMD_OFFSET and add one of the below to find a register.
+ */
#define RSC_DRV_CMD_MSGID 0x30
#define RSC_DRV_CMD_ADDR 0x34
#define RSC_DRV_CMD_DATA 0x38
@@ -67,6 +82,60 @@
#define CMD_STATUS_ISSUED BIT(8)
#define CMD_STATUS_COMPL BIT(16)
+/*
+ * Here's a high level overview of how all the registers in RPMH work
+ * together:
+ *
+ * - The main rpmh-rsc address is the base of a register space that can
+ * be used to find overall configuration of the hardware
+ * (DRV_PRNT_CHLD_CONFIG). Also found within the rpmh-rsc register
+ * space are all the TCS blocks. The offset of the TCS blocks is
+ * specified in the device tree by "qcom,tcs-offset" and used to
+ * compute tcs_base.
+ * - TCS blocks come one after another. Type, count, and order are
+ * specified by the device tree as "qcom,tcs-config".
+ * - Each TCS block has some registers, then space for up to 16 commands.
+ * Note that though address space is reserved for 16 commands, fewer
+ * might be present. See ncpt (num cmds per TCS).
+ *
+ * Here's a picture:
+ *
+ * +---------------------------------------------------+
+ * |RSC |
+ * | ctrl |
+ * | |
+ * | Drvs: |
+ * | +-----------------------------------------------+ |
+ * | |DRV0 | |
+ * | | ctrl/config | |
+ * | | IRQ | |
+ * | | | |
+ * | | TCSes: | |
+ * | | +------------------------------------------+ | |
+ * | | |TCS0 | | | | | | | | | | | | | | |
+ * | | | ctrl | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15| | |
+ * | | | | | | | | | | | | | | | | | |
+ * | | +------------------------------------------+ | |
+ * | | +------------------------------------------+ | |
+ * | | |TCS1 | | | | | | | | | | | | | | |
+ * | | | ctrl | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15| | |
+ * | | | | | | | | | | | | | | | | | |
+ * | | +------------------------------------------+ | |
+ * | | +------------------------------------------+ | |
+ * | | |TCS2 | | | | | | | | | | | | | | |
+ * | | | ctrl | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15| | |
+ * | | | | | | | | | | | | | | | | | |
+ * | | +------------------------------------------+ | |
+ * | | ...... | |
+ * | +-----------------------------------------------+ |
+ * | +-----------------------------------------------+ |
+ * | |DRV1 | |
+ * | | (same as DRV0) | |
+ * | +-----------------------------------------------+ |
+ * | ...... |
+ * +---------------------------------------------------+
+ */
+
static u32 read_tcs_cmd(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id)
{
return readl_relaxed(drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg +
--
2.26.0.292.g33ef6b2f38-goog
Auditing tcs_invalidate() made me worried. Specifically I saw that it
used spin_lock(), not spin_lock_irqsave(). That always worries me
unless I can trace for sure that I'm in the interrupt handler or that
someone else already disabled interrupts.
Looking more at it, there is actually no reason for these locks
anyway. Specifically the only reason you'd ever call
rpmh_rsc_invalidate() is if you cared that the sleep/wake TCSes were
empty. That means that they need to continue to be empty even after
rpmh_rsc_invalidate() returns. The only way that can happen is if the
caller already has done something to keep all other RPMH users out.
It should be noted that even though the caller is only worried about
making sleep/wake TCSes empty, they also need to worry about stopping
active-only transfers if they need to handle the case where
active-only transfers might borrow the wake TCS.
At the moment rpmh_rsc_invalidate() is only called in PM code from the
last CPU. If that later changes the caller will still need to solve
the above problems themselves, so these locks will never be useful.
Continuing to audit tcs_invalidate(), I found a bug. The function
didn't properly check for a borrowed TCS if we hadn't recently written
anything into the TCS. Specifically, if we've never written to the
WAKE_TCS (or we've flushed it recently) then tcs->slots is empty.
We'll early-out and we'll never call tcs_is_free().
I thought about fixing this bug by either deleting the early check for
bitmap_empty() or possibly only doing it if we knew we weren't on a
TCS that could be borrowed. However, I think it's better to just
delete the checks.
As argued above it's up to the caller to make sure that all other
users of RPMH are quiet before tcs_invalidate() is called. Since
callers need to handle the zero-active-TCS case anyway that means they
need to make sure that make sure the active-only transfers are quiet
before calling too. The one way tcs_invalidate() gets called today is
through rpmh_rsc_cpu_pm_callback() which calls
rpmh_rsc_ctrlr_is_busy() to handle this. When we have another path to
get to tcs_invalidate() it will also need to come up with something
similar and it won't need this extra check either. If we later find
some code path that actually needs this check back in (and somehow
manages to be race free) we can always add it back in.
Signed-off-by: Douglas Anderson <[email protected]>
---
Changes in v3:
- Replaced ("irqsave()...") + ("...never -EBUSY") w/ ("Caller handles...")
Changes in v2: None
drivers/soc/qcom/rpmh-internal.h | 2 +-
drivers/soc/qcom/rpmh-rsc.c | 38 +++++++++++---------------------
drivers/soc/qcom/rpmh.c | 5 +----
3 files changed, 15 insertions(+), 30 deletions(-)
diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index f06350cbc9a2..dba8510c0669 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -132,7 +132,7 @@ struct rsc_drv {
int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg);
int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv,
const struct tcs_request *msg);
-int rpmh_rsc_invalidate(struct rsc_drv *drv);
+void rpmh_rsc_invalidate(struct rsc_drv *drv);
void rpmh_tx_done(const struct tcs_request *msg, int r);
int rpmh_flush(struct rpmh_ctrlr *ctrlr);
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 10c026b2e1bc..a3b015196f15 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -198,50 +198,38 @@ static bool tcs_is_free(struct rsc_drv *drv, int tcs_id)
* This will clear the "slots" variable of the given tcs_group and also
* tell the hardware to forget about all entries.
*
- * Return: 0 if no problem, or -EAGAIN if the caller should try again in a
- * bit. Caller should make sure to enable interrupts between tries.
+ * The caller must ensure that no other RPMH actions are happening when this
+ * function is called, since otherwise the device may immediately become
+ * used again even before this function exits.
*/
-static int tcs_invalidate(struct rsc_drv *drv, int type)
+static void tcs_invalidate(struct rsc_drv *drv, int type)
{
int m;
struct tcs_group *tcs = &drv->tcs[type];
- spin_lock(&tcs->lock);
- if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
- spin_unlock(&tcs->lock);
- return 0;
- }
+ /* Caller ensures nobody else is running so no lock */
+ if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS))
+ return;
for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
- if (!tcs_is_free(drv, m)) {
- spin_unlock(&tcs->lock);
- return -EAGAIN;
- }
write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, m, 0);
write_tcs_reg_sync(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0);
}
bitmap_zero(tcs->slots, MAX_TCS_SLOTS);
- spin_unlock(&tcs->lock);
-
- return 0;
}
/**
* rpmh_rsc_invalidate() - Invalidate sleep and wake TCSes.
* @drv: The RSC controller.
*
- * Return: 0 if no problem, or -EAGAIN if the caller should try again in a
- * bit. Caller should make sure to enable interrupts between tries.
+ * The caller must ensure that no other RPMH actions are happening when this
+ * function is called, since otherwise the device may immediately become
+ * used again even before this function exits.
*/
-int rpmh_rsc_invalidate(struct rsc_drv *drv)
+void rpmh_rsc_invalidate(struct rsc_drv *drv)
{
- int ret;
-
- ret = tcs_invalidate(drv, SLEEP_TCS);
- if (!ret)
- ret = tcs_invalidate(drv, WAKE_TCS);
-
- return ret;
+ tcs_invalidate(drv, SLEEP_TCS);
+ tcs_invalidate(drv, WAKE_TCS);
}
/**
diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
index 88f88015ef03..02171c192aa1 100644
--- a/drivers/soc/qcom/rpmh.c
+++ b/drivers/soc/qcom/rpmh.c
@@ -439,7 +439,6 @@ static int send_single(struct rpmh_ctrlr *ctrlr, enum rpmh_state state,
*
* Return:
* * 0 - Success
- * * -EAGAIN - Retry again
* * Error code - Otherwise
*/
int rpmh_flush(struct rpmh_ctrlr *ctrlr)
@@ -453,9 +452,7 @@ int rpmh_flush(struct rpmh_ctrlr *ctrlr)
}
/* Invalidate the TCSes first to avoid stale data */
- ret = rpmh_rsc_invalidate(ctrlr_to_drv(ctrlr));
- if (ret)
- return ret;
+ rpmh_rsc_invalidate(ctrlr_to_drv(ctrlr));
/* First flush the cached batch requests */
ret = flush_batch(ctrlr);
--
2.26.0.292.g33ef6b2f38-goog
The get_tcs_of_type() function doesn't provide any value. It's not
conceptually difficult to access a value in an array, even if that
value is in a structure and we want a pointer to the value. Having
the function in there makes me feel like it's doing something fancier
like looping or searching. Remove it.
Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Maulik Shah <[email protected]>
Tested-by: Maulik Shah <[email protected]>
---
Changes in v3:
- Rebased atop v16 ('Invoke rpmh_flush...') series.
Changes in v2: None
drivers/soc/qcom/rpmh-rsc.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index e361b2331993..855a1dab7718 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -177,17 +177,10 @@ static bool tcs_is_free(struct rsc_drv *drv, int tcs_id)
read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id);
}
-static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type)
-{
- return &drv->tcs[type];
-}
-
static int tcs_invalidate(struct rsc_drv *drv, int type)
{
int m;
- struct tcs_group *tcs;
-
- tcs = get_tcs_of_type(drv, type);
+ struct tcs_group *tcs = &drv->tcs[type];
spin_lock(&tcs->lock);
if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
@@ -250,9 +243,9 @@ static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
* dedicated TCS for active state use, then re-purpose a wake TCS to
* send active votes.
*/
- tcs = get_tcs_of_type(drv, type);
+ tcs = &drv->tcs[type];
if (msg->state == RPMH_ACTIVE_ONLY_STATE && !tcs->num_tcs)
- tcs = get_tcs_of_type(drv, WAKE_TCS);
+ tcs = &drv->tcs[WAKE_TCS];
return tcs;
}
@@ -643,7 +636,7 @@ int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv, const struct tcs_request *msg)
static bool rpmh_rsc_ctrlr_is_busy(struct rsc_drv *drv)
{
int m;
- struct tcs_group *tcs = get_tcs_of_type(drv, ACTIVE_TCS);
+ struct tcs_group *tcs = &drv->tcs[ACTIVE_TCS];
/*
* If we made an active request on a RSC that does not have a
@@ -654,7 +647,7 @@ static bool rpmh_rsc_ctrlr_is_busy(struct rsc_drv *drv)
* lock before checking tcs_is_free().
*/
if (!tcs->num_tcs)
- tcs = get_tcs_of_type(drv, WAKE_TCS);
+ tcs = &drv->tcs[WAKE_TCS];
for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
if (!tcs_is_free(drv, m))
--
2.26.0.292.g33ef6b2f38-goog
This patch makes two changes, both of which should be no-ops:
1. Make read_tcs_reg() / read_tcs_cmd() symmetric to write_tcs_reg() /
write_tcs_cmd().
2. Change the order of operations in the above functions to make it
more obvious to me what the math is doing. Specifically first you
want to find the right TCS, then the right register, and then
multiply by the command ID if necessary.
Signed-off-by: Douglas Anderson <[email protected]>
Reviewed-by: Maulik Shah <[email protected]>
Tested-by: Maulik Shah <[email protected]>
---
Changes in v3:
- Add "TCS" in title (Maulik).
- Rebased atop v16 ('Invoke rpmh_flush...') series.
Changes in v2: None
drivers/soc/qcom/rpmh-rsc.c | 33 +++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 34b0e14c2f33..7d9e2c2f0e27 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -67,28 +67,33 @@
#define CMD_STATUS_ISSUED BIT(8)
#define CMD_STATUS_COMPL BIT(16)
-static u32 read_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id)
+static u32 read_tcs_cmd(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id)
{
- return readl_relaxed(drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id +
+ return readl_relaxed(drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg +
RSC_DRV_CMD_OFFSET * cmd_id);
}
+static u32 read_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id)
+{
+ return readl_relaxed(drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg);
+}
+
static void write_tcs_cmd(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id,
u32 data)
{
- writel_relaxed(data, drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id +
+ writel_relaxed(data, drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg +
RSC_DRV_CMD_OFFSET * cmd_id);
}
static void write_tcs_reg(struct rsc_drv *drv, int reg, int tcs_id, u32 data)
{
- writel_relaxed(data, drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id);
+ writel_relaxed(data, drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg);
}
static void write_tcs_reg_sync(struct rsc_drv *drv, int reg, int tcs_id,
u32 data)
{
- writel(data, drv->tcs_base + reg + RSC_DRV_TCS_OFFSET * tcs_id);
+ writel(data, drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg);
for (;;) {
if (data == readl(drv->tcs_base + reg +
RSC_DRV_TCS_OFFSET * tcs_id))
@@ -100,7 +105,7 @@ static void write_tcs_reg_sync(struct rsc_drv *drv, int reg, int tcs_id,
static bool tcs_is_free(struct rsc_drv *drv, int tcs_id)
{
return !test_bit(tcs_id, drv->tcs_in_use) &&
- read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id, 0);
+ read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id);
}
static struct tcs_group *get_tcs_of_type(struct rsc_drv *drv, int type)
@@ -207,7 +212,7 @@ static void __tcs_set_trigger(struct rsc_drv *drv, int tcs_id, bool trigger)
* While clearing ensure that the AMC mode trigger is cleared
* and then the mode enable is cleared.
*/
- enable = read_tcs_reg(drv, RSC_DRV_CONTROL, tcs_id, 0);
+ enable = read_tcs_reg(drv, RSC_DRV_CONTROL, tcs_id);
enable &= ~TCS_AMC_MODE_TRIGGER;
write_tcs_reg_sync(drv, RSC_DRV_CONTROL, tcs_id, enable);
enable &= ~TCS_AMC_MODE_ENABLE;
@@ -226,7 +231,7 @@ static void enable_tcs_irq(struct rsc_drv *drv, int tcs_id, bool enable)
{
u32 data;
- data = read_tcs_reg(drv, RSC_DRV_IRQ_ENABLE, 0, 0);
+ data = read_tcs_reg(drv, RSC_DRV_IRQ_ENABLE, 0);
if (enable)
data |= BIT(tcs_id);
else
@@ -245,7 +250,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
const struct tcs_request *req;
struct tcs_cmd *cmd;
- irq_status = read_tcs_reg(drv, RSC_DRV_IRQ_STATUS, 0, 0);
+ irq_status = read_tcs_reg(drv, RSC_DRV_IRQ_STATUS, 0);
for_each_set_bit(i, &irq_status, BITS_PER_LONG) {
req = get_req_from_tcs(drv, i);
@@ -259,7 +264,7 @@ static irqreturn_t tcs_tx_done(int irq, void *p)
u32 sts;
cmd = &req->cmds[j];
- sts = read_tcs_reg(drv, RSC_DRV_CMD_STATUS, i, j);
+ sts = read_tcs_cmd(drv, RSC_DRV_CMD_STATUS, i, j);
if (!(sts & CMD_STATUS_ISSUED) ||
((req->wait_for_compl || cmd->wait) &&
!(sts & CMD_STATUS_COMPL))) {
@@ -313,7 +318,7 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
cmd_msgid |= msg->wait_for_compl ? CMD_MSGID_RESP_REQ : 0;
cmd_msgid |= CMD_MSGID_WRITE;
- cmd_complete = read_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, tcs_id, 0);
+ cmd_complete = read_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, tcs_id);
for (i = 0, j = cmd_id; i < msg->num_cmds; i++, j++) {
cmd = &msg->cmds[i];
@@ -329,7 +334,7 @@ static void __tcs_buffer_write(struct rsc_drv *drv, int tcs_id, int cmd_id,
}
write_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, tcs_id, cmd_complete);
- cmd_enable |= read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id, 0);
+ cmd_enable |= read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id);
write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id, cmd_enable);
}
@@ -345,10 +350,10 @@ static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
if (tcs_is_free(drv, tcs_id))
continue;
- curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id, 0);
+ curr_enabled = read_tcs_reg(drv, RSC_DRV_CMD_ENABLE, tcs_id);
for_each_set_bit(j, &curr_enabled, MAX_CMDS_PER_TCS) {
- addr = read_tcs_reg(drv, RSC_DRV_CMD_ADDR, tcs_id, j);
+ addr = read_tcs_cmd(drv, RSC_DRV_CMD_ADDR, tcs_id, j);
for (k = 0; k < msg->num_cmds; k++) {
if (addr == msg->cmds[k].addr)
return -EBUSY;
--
2.26.0.292.g33ef6b2f38-goog
Hi,
Reviewed-by: Maulik Shah <[email protected]>
Thanks,
Maulik
On 4/8/2020 5:20 AM, Douglas Anderson wrote:
> Perhaps it's just me, it took a really long time to understand what
> the register layout of rpmh-rsc was just from the #defines. Let's add
> a bunch of comments describing which blocks are part of other blocks.
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
>
> Changes in v3:
> - Extra blank line removed (Maulik).
> - IRQ registers aren't in TCS0 (Maulik).
> - One space after a period now (Maulik).
> - Plural of TCS fixed to TCSes following Maulik's example.
> - Rebased atop v16 ('Invoke rpmh_flush...') series.
>
> Changes in v2:
> - Now prose in comments instead of struct definitions.
> - Pretty ASCII art from Stephen.
>
> drivers/soc/qcom/rpmh-rsc.c | 79 ++++++++++++++++++++++++++++++++++---
> 1 file changed, 74 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index 7d9e2c2f0e27..46455b1d93f1 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -43,14 +43,29 @@
> #define DRV_NCPT_MASK 0x1F
> #define DRV_NCPT_SHIFT 27
>
> -/* Register offsets */
> +/* Offsets for common TCS Registers, one bit per TCS */
> #define RSC_DRV_IRQ_ENABLE 0x00
> #define RSC_DRV_IRQ_STATUS 0x04
> -#define RSC_DRV_IRQ_CLEAR 0x08
> -#define RSC_DRV_CMD_WAIT_FOR_CMPL 0x10
> +#define RSC_DRV_IRQ_CLEAR 0x08 /* w/o; write 1 to clear */
> +
> +/*
> + * Offsets for per TCS Registers.
> + *
> + * TCSes start at 0x10 from tcs_base and are stored one after another.
> + * Multiply tcs_id by RSC_DRV_TCS_OFFSET to find a given TCS and add one
> + * of the below to find a register.
> + */
> +#define RSC_DRV_CMD_WAIT_FOR_CMPL 0x10 /* 1 bit per command */
> #define RSC_DRV_CONTROL 0x14
> -#define RSC_DRV_STATUS 0x18
> -#define RSC_DRV_CMD_ENABLE 0x1C
> +#define RSC_DRV_STATUS 0x18 /* zero if tcs is busy */
> +#define RSC_DRV_CMD_ENABLE 0x1C /* 1 bit per command */
> +
> +/*
> + * Offsets for per command in a TCS.
> + *
> + * Commands (up to 16) start at 0x30 in a TCS; multiply command index
> + * by RSC_DRV_CMD_OFFSET and add one of the below to find a register.
> + */
> #define RSC_DRV_CMD_MSGID 0x30
> #define RSC_DRV_CMD_ADDR 0x34
> #define RSC_DRV_CMD_DATA 0x38
> @@ -67,6 +82,60 @@
> #define CMD_STATUS_ISSUED BIT(8)
> #define CMD_STATUS_COMPL BIT(16)
>
> +/*
> + * Here's a high level overview of how all the registers in RPMH work
> + * together:
> + *
> + * - The main rpmh-rsc address is the base of a register space that can
> + * be used to find overall configuration of the hardware
> + * (DRV_PRNT_CHLD_CONFIG). Also found within the rpmh-rsc register
> + * space are all the TCS blocks. The offset of the TCS blocks is
> + * specified in the device tree by "qcom,tcs-offset" and used to
> + * compute tcs_base.
> + * - TCS blocks come one after another. Type, count, and order are
> + * specified by the device tree as "qcom,tcs-config".
> + * - Each TCS block has some registers, then space for up to 16 commands.
> + * Note that though address space is reserved for 16 commands, fewer
> + * might be present. See ncpt (num cmds per TCS).
> + *
> + * Here's a picture:
> + *
> + * +---------------------------------------------------+
> + * |RSC |
> + * | ctrl |
> + * | |
> + * | Drvs: |
> + * | +-----------------------------------------------+ |
> + * | |DRV0 | |
> + * | | ctrl/config | |
> + * | | IRQ | |
> + * | | | |
> + * | | TCSes: | |
> + * | | +------------------------------------------+ | |
> + * | | |TCS0 | | | | | | | | | | | | | | |
> + * | | | ctrl | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15| | |
> + * | | | | | | | | | | | | | | | | | |
> + * | | +------------------------------------------+ | |
> + * | | +------------------------------------------+ | |
> + * | | |TCS1 | | | | | | | | | | | | | | |
> + * | | | ctrl | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15| | |
> + * | | | | | | | | | | | | | | | | | |
> + * | | +------------------------------------------+ | |
> + * | | +------------------------------------------+ | |
> + * | | |TCS2 | | | | | | | | | | | | | | |
> + * | | | ctrl | 0| 1| 2| 3| 4| 5| .| .| .| .|14|15| | |
> + * | | | | | | | | | | | | | | | | | |
> + * | | +------------------------------------------+ | |
> + * | | ...... | |
> + * | +-----------------------------------------------+ |
> + * | +-----------------------------------------------+ |
> + * | |DRV1 | |
> + * | | (same as DRV0) | |
> + * | +-----------------------------------------------+ |
> + * | ...... |
> + * +---------------------------------------------------+
> + */
> +
> static u32 read_tcs_cmd(struct rsc_drv *drv, int reg, int tcs_id, int cmd_id)
> {
> return readl_relaxed(drv->tcs_base + RSC_DRV_TCS_OFFSET * tcs_id + reg +
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Hi,
On 4/8/2020 5:20 AM, Douglas Anderson wrote:
> Auditing tcs_invalidate() made me worried. Specifically I saw that it
> used spin_lock(), not spin_lock_irqsave(). That always worries me
> unless I can trace for sure that I'm in the interrupt handler or that
> someone else already disabled interrupts.
>
> Looking more at it, there is actually no reason for these locks
> anyway. Specifically the only reason you'd ever call
> rpmh_rsc_invalidate() is if you cared that the sleep/wake TCSes were
> empty. That means that they need to continue to be empty even after
> rpmh_rsc_invalidate() returns. The only way that can happen is if the
> caller already has done something to keep all other RPMH users out.
> It should be noted that even though the caller is only worried about
> making sleep/wake TCSes empty, they also need to worry about stopping
> active-only transfers if they need to handle the case where
> active-only transfers might borrow the wake TCS.
>
> At the moment rpmh_rsc_invalidate() is only called in PM code from the
> last CPU. If that later changes the caller will still need to solve
> the above problems themselves, so these locks will never be useful.
>
> Continuing to audit tcs_invalidate(), I found a bug. The function
> didn't properly check for a borrowed TCS if we hadn't recently written
> anything into the TCS. Specifically, if we've never written to the
> WAKE_TCS (or we've flushed it recently) then tcs->slots is empty.
> We'll early-out and we'll never call tcs_is_free().
>
> I thought about fixing this bug by either deleting the early check for
> bitmap_empty() or possibly only doing it if we knew we weren't on a
> TCS that could be borrowed. However, I think it's better to just
> delete the checks.
>
> As argued above it's up to the caller to make sure that all other
> users of RPMH are quiet before tcs_invalidate() is called. Since
> callers need to handle the zero-active-TCS case anyway that means they
> need to make sure that make sure the active-only transfers are quiet
make sure is twice, can you please drop once.
need to make sure that the active-only.....
other than this.
Reviewed-by: Maulik Shah <[email protected]>
Tested-by: Maulik Shah <[email protected]>
Thanks,
Maulik
> before calling too. The one way tcs_invalidate() gets called today is
> through rpmh_rsc_cpu_pm_callback() which calls
> rpmh_rsc_ctrlr_is_busy() to handle this. When we have another path to
> get to tcs_invalidate() it will also need to come up with something
> similar and it won't need this extra check either. If we later find
> some code path that actually needs this check back in (and somehow
> manages to be race free) we can always add it back in.
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
>
> Changes in v3:
> - Replaced ("irqsave()...") + ("...never -EBUSY") w/ ("Caller handles...")
>
> Changes in v2: None
>
> drivers/soc/qcom/rpmh-internal.h | 2 +-
> drivers/soc/qcom/rpmh-rsc.c | 38 +++++++++++---------------------
> drivers/soc/qcom/rpmh.c | 5 +----
> 3 files changed, 15 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
> index f06350cbc9a2..dba8510c0669 100644
> --- a/drivers/soc/qcom/rpmh-internal.h
> +++ b/drivers/soc/qcom/rpmh-internal.h
> @@ -132,7 +132,7 @@ struct rsc_drv {
> int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg);
> int rpmh_rsc_write_ctrl_data(struct rsc_drv *drv,
> const struct tcs_request *msg);
> -int rpmh_rsc_invalidate(struct rsc_drv *drv);
> +void rpmh_rsc_invalidate(struct rsc_drv *drv);
>
> void rpmh_tx_done(const struct tcs_request *msg, int r);
> int rpmh_flush(struct rpmh_ctrlr *ctrlr);
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index 10c026b2e1bc..a3b015196f15 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -198,50 +198,38 @@ static bool tcs_is_free(struct rsc_drv *drv, int tcs_id)
> * This will clear the "slots" variable of the given tcs_group and also
> * tell the hardware to forget about all entries.
> *
> - * Return: 0 if no problem, or -EAGAIN if the caller should try again in a
> - * bit. Caller should make sure to enable interrupts between tries.
> + * The caller must ensure that no other RPMH actions are happening when this
> + * function is called, since otherwise the device may immediately become
> + * used again even before this function exits.
> */
> -static int tcs_invalidate(struct rsc_drv *drv, int type)
> +static void tcs_invalidate(struct rsc_drv *drv, int type)
> {
> int m;
> struct tcs_group *tcs = &drv->tcs[type];
>
> - spin_lock(&tcs->lock);
> - if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS)) {
> - spin_unlock(&tcs->lock);
> - return 0;
> - }
> + /* Caller ensures nobody else is running so no lock */
> + if (bitmap_empty(tcs->slots, MAX_TCS_SLOTS))
> + return;
>
> for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
> - if (!tcs_is_free(drv, m)) {
> - spin_unlock(&tcs->lock);
> - return -EAGAIN;
> - }
> write_tcs_reg_sync(drv, RSC_DRV_CMD_ENABLE, m, 0);
> write_tcs_reg_sync(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0);
> }
> bitmap_zero(tcs->slots, MAX_TCS_SLOTS);
> - spin_unlock(&tcs->lock);
> -
> - return 0;
> }
>
> /**
> * rpmh_rsc_invalidate() - Invalidate sleep and wake TCSes.
> * @drv: The RSC controller.
> *
> - * Return: 0 if no problem, or -EAGAIN if the caller should try again in a
> - * bit. Caller should make sure to enable interrupts between tries.
> + * The caller must ensure that no other RPMH actions are happening when this
> + * function is called, since otherwise the device may immediately become
> + * used again even before this function exits.
> */
> -int rpmh_rsc_invalidate(struct rsc_drv *drv)
> +void rpmh_rsc_invalidate(struct rsc_drv *drv)
> {
> - int ret;
> -
> - ret = tcs_invalidate(drv, SLEEP_TCS);
> - if (!ret)
> - ret = tcs_invalidate(drv, WAKE_TCS);
> -
> - return ret;
> + tcs_invalidate(drv, SLEEP_TCS);
> + tcs_invalidate(drv, WAKE_TCS);
> }
>
> /**
> diff --git a/drivers/soc/qcom/rpmh.c b/drivers/soc/qcom/rpmh.c
> index 88f88015ef03..02171c192aa1 100644
> --- a/drivers/soc/qcom/rpmh.c
> +++ b/drivers/soc/qcom/rpmh.c
> @@ -439,7 +439,6 @@ static int send_single(struct rpmh_ctrlr *ctrlr, enum rpmh_state state,
> *
> * Return:
> * * 0 - Success
> - * * -EAGAIN - Retry again
> * * Error code - Otherwise
> */
> int rpmh_flush(struct rpmh_ctrlr *ctrlr)
> @@ -453,9 +452,7 @@ int rpmh_flush(struct rpmh_ctrlr *ctrlr)
> }
>
> /* Invalidate the TCSes first to avoid stale data */
> - ret = rpmh_rsc_invalidate(ctrlr_to_drv(ctrlr));
> - if (ret)
> - return ret;
> + rpmh_rsc_invalidate(ctrlr_to_drv(ctrlr));
>
> /* First flush the cached batch requests */
> ret = flush_batch(ctrlr);
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation