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 (though just to be sure, best to try
this atop Maulik's patches where it should be super obvious that we
always invalidate before writing sleep/wake TCSs.
- I think I've eliminated a possible deadlock on "nosmp" systems,
though it was mostly theoretical.
- Possibly we could get a warning in some cases if I misunderstood how
tcs_is_free() works. It'd be easy to remove the warning, though.
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) and review them soon-ish so they can land
and future patches can be based on them.
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.
With all that, enjoy.
Douglas Anderson (9):
drivers: qcom: rpmh-rsc: Clean code reading/writing 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: A lot of comments
drivers: qcom: rpmh-rsc: Only use "tcs_in_use" for ACTIVE_ONLY
drivers: qcom: rpmh-rsc: Warning if tcs_write() used for non-active
drivers: qcom: rpmh-rsc: spin_lock_irqsave() for tcs_invalidate()
drivers: qcom: rpmh-rsc: Kill cmd_cache and find_match() with fire
drivers/soc/qcom/rpmh-internal.h | 45 ++--
drivers/soc/qcom/rpmh-rsc.c | 390 +++++++++++++++++++++++--------
2 files changed, 313 insertions(+), 122 deletions(-)
--
2.25.1.481.gfbce0eb801-goog
From trawling through the code (see the "A lot of comments" change) I
found that "tcs_in_use" was only kept up-to-date for ACTIVE_ONLY TCSs.
...yet tcs_is_free() was checking the variable called from
tcs_invalidate() and tcs_invalidate() is only used for non-ACTIVE_ONLY
TCSs.
Let's change tcs_invalidate() to just check the "RSC_DRV_STATUS"
register, which was presumably the important part.
It also feels like for ACTIVE_ONLY TCSs that it probably wasn't
important to check the "RSC_DRV_STATUS". We'll keep doing it just in
case but we'll add a warning if it ever actually mattered.
Signed-off-by: Douglas Anderson <[email protected]>
---
drivers/soc/qcom/rpmh-rsc.c | 23 +++++++++++++++++++----
1 file changed, 19 insertions(+), 4 deletions(-)
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 190226151029..c63441182358 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -164,7 +164,7 @@ static void write_tcs_reg_sync(struct rsc_drv *drv, int reg, int tcs_id,
}
/**
- * tcs_is_free() - Return if a TCS is totally free.
+ * tcs_is_free() - Return if an ACTIVE_ONLY TCS is totally free.
* @drv: The RSC controller.
* @tcs_id: The global ID of this TCS.
*
@@ -177,8 +177,23 @@ 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);
+ if (test_bit(tcs_id, drv->tcs_in_use))
+ return false;
+
+ if (read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id) != 0)
+ return true;
+
+ /*
+ * If this warning never ever hits then we can change this function
+ * to just look at "tcs_in_use" and skip the read of the
+ * RSC_DRV_STATUS register.
+ *
+ * If this warning _does_ hit, we should figure out if this is just
+ * the way the hardware works or if there is some bug being pointed
+ * out.
+ */
+ WARN(1, "Driver thought TCS was free but HW reported busy\n");
+ return false;
}
/**
@@ -204,7 +219,7 @@ static int tcs_invalidate(struct rsc_drv *drv, int type)
}
for (m = tcs->offset; m < tcs->offset + tcs->num_tcs; m++) {
- if (!tcs_is_free(drv, m)) {
+ if (read_tcs_reg(drv, RSC_DRV_STATUS, m) == 0) {
spin_unlock(&tcs->lock);
return -EAGAIN;
}
--
2.25.1.481.gfbce0eb801-goog
As talked about in the comments introduced by the patch ("drivers:
qcom: rpmh-rsc: A lot of comments"), the find_match() function()
didn't seem very sensible. Remove it and the cmd_cache array that it
needed.
Nothing should stop us from exploring some fancy ways to avoid fully
invalidating the whole sleep/wake TCSs all the time in the future, but
find_match() doesn't seem well enough thought out to keep while we
wait for something better to arrive.
This patch isn't quite a no-op. Specifically:
* It should be a slight performance boost of not searching through so
many arrays.
* There is slightly less self-checking of commands written to the
sleep/wake sets. If it truly is an error to write to the same
address more than once in a tcs_group then some cases (but not all)
would have been caught before.
[1] https://lore.kernel.org/r/[email protected]
Signed-off-by: Douglas Anderson <[email protected]>
---
It's possible that this might need the latest version of Maulik's
rpmh.c patches to work properly so we can really be sure that we
always invalidate before we flush.
drivers/soc/qcom/rpmh-internal.h | 2 -
drivers/soc/qcom/rpmh-rsc.c | 104 +------------------------------
2 files changed, 1 insertion(+), 105 deletions(-)
diff --git a/drivers/soc/qcom/rpmh-internal.h b/drivers/soc/qcom/rpmh-internal.h
index 49df01af7701..7206a1ac97e8 100644
--- a/drivers/soc/qcom/rpmh-internal.h
+++ b/drivers/soc/qcom/rpmh-internal.h
@@ -34,7 +34,6 @@ struct rsc_drv;
* trigger
* End: get irq, access req,
* grab drv->lock, clear tcs_in_use, drop drv->lock
- * @cmd_cache: Flattened cache of cmds in sleep/wake TCS; num_tcs * ncpt big.
* @slots: Indicates which of @cmd_addr are occupied; only used for
* SLEEP / WAKE TCSs. Things are tightly packed in the
* case that (ncpt < MAX_CMDS_PER_TCS). That is if ncpt = 2 and
@@ -49,7 +48,6 @@ struct tcs_group {
int ncpt;
spinlock_t lock;
const struct tcs_request *req[MAX_TCS_PER_TYPE];
- u32 *cmd_cache;
DECLARE_BITMAP(slots, MAX_TCS_SLOTS);
};
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 0297da5ceeaa..733373ed56bd 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -630,93 +630,6 @@ int rpmh_rsc_send_data(struct rsc_drv *drv, const struct tcs_request *msg)
return ret;
}
-/**
- * find_match() - Find if the cmd sequence is in the tcs_group
- * @tcs: The tcs_group to search. Either sleep or wake.
- * @cmd: The command sequence to search for; only addr is looked at.
- * @len: The number of commands in the sequence.
- *
- * Searches through the given tcs_group to see if a given command sequence
- * is in there.
- *
- * Two sequences are matches if they modify the same set of addresses in
- * the same order. The value of the data is not considered when deciding if
- * two things are matches.
- *
- * How this function works is best understood by example. For our example,
- * we'll imagine our tcs group contains these (cmd, data) tuples:
- * [(a, A), (b, B), (c, C), (d, D), (e, E), (f, F), (g, G), (h, H)]
- * ...in other words it has an element where (addr=a, data=A), etc.
- * ...we'll assume that there is one TCS in the group that can store 8 commands.
- *
- * - find_match([(a, X)]) => 0
- * - find_match([(c, X), (d, X)]) => 2
- * - find_match([(c, X), (d, X), (e, X)]) => 2
- * - find_match([(z, X)]) => -ENODATA
- * - find_match([(a, X), (y, X)]) => -EINVAL (and warning printed)
- * - find_match([(g, X), (h, X), (i, X)]) => -EINVAL (and warning printed)
- * - find_match([(y, X), (a, X)]) => -ENODATA
- *
- * NOTE: This function overall seems like it has questionable value.
- * - It can be used to update a message in the TCS with new data, but I
- * don't believe we actually do that--we always fully invalidate and
- * re-write everything. Specifically it would be too limiting to force
- * someone not to change the set of addresses written to each time.
- * - This function could be attempting to avoid writing different data to
- * the same address twice in a tcs_group. If that's the goal, it doesn't
- * do a great job since find_match([(y, X), (a, X)]) return -ENODATA in my
- * above example.
- * - If you originally wrote [(a, A), (b, B), (c, C)] and later tried to
- * write [(a, A), (b, B)] it'd look like a match and we wouldn't consider
- * it an error that the size got shorter.
- * - If two clients wrote sequences that happened to be placed in slots next
- * to each other then a later check could match a sequence that was the
- * size of both together.
- *
- * TODO: in light of the above, prehaps we can just remove this function?
- * If we later come up with fancy algorithms for updating everything without
- * full invalidations we can come up with something then.
- *
- * Only for use on sleep/wake TCSs since those are the only ones we maintain
- * tcs->slots and tcs->cmd_cache for.
- *
- * Must be called with the tcs_lock for the group held.
- *
- * Return: If the given command sequence wasn't in the tcs_group: -ENODATA.
- * If the given command sequence was in the tcs_group: the index of
- * the slot in the tcs_group where the first command is.
- * In some error cases (see above), -EINVAL.
- */
-static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,
- int len)
-{
- int i, j;
-
- /* Check for already cached commands */
- for_each_set_bit(i, tcs->slots, MAX_TCS_SLOTS) {
- if (tcs->cmd_cache[i] != cmd[0].addr)
- continue;
- if (i + len >= tcs->num_tcs * tcs->ncpt)
- goto seq_err;
- for (j = 0; j < len; j++) {
- /*
- * TODO: it's actually not valid to look at
- * "cmd_cache[x]" if "slots[x]" doesn't have a bit
- * set. Should add a check.
- */
- if (tcs->cmd_cache[i + j] != cmd[j].addr)
- goto seq_err;
- }
- return i;
- }
-
- return -ENODATA;
-
-seq_err:
- WARN(1, "Message does not match previous sequence.\n");
- return -EINVAL;
-}
-
/**
* find_slots() - Find a place to write the given message.
* @tcs: The controller.
@@ -728,7 +641,7 @@ static int find_match(const struct tcs_group *tcs, const struct tcs_cmd *cmd,
* start writing the message.
*
* Only for use on sleep/wake TCSs since those are the only ones we maintain
- * tcs->slots and tcs->cmd_cache for.
+ * tcs->slots for.
*
* Must be called with the tcs_lock for the group held.
*
@@ -740,11 +653,6 @@ static int find_slots(struct tcs_group *tcs, const struct tcs_request *msg,
int slot, offset;
int i = 0;
- /* Find if we already have the msg in our TCS */
- slot = find_match(tcs, msg->cmds, msg->num_cmds);
- if (slot >= 0)
- goto copy_data;
-
/* Do over, until we can fit the full payload in a single TCS */
do {
slot = bitmap_find_next_zero_area(tcs->slots, MAX_TCS_SLOTS,
@@ -754,11 +662,7 @@ static int find_slots(struct tcs_group *tcs, const struct tcs_request *msg,
i += tcs->ncpt;
} while (slot + msg->num_cmds - 1 >= i);
-copy_data:
bitmap_set(tcs->slots, slot, msg->num_cmds);
- /* Copy the addresses of the resources over to the slots */
- for (i = 0; i < msg->num_cmds; i++)
- tcs->cmd_cache[slot + i] = msg->cmds[i].addr;
offset = slot / tcs->ncpt;
*tcs_id = offset + tcs->offset;
@@ -889,12 +793,6 @@ static int rpmh_probe_tcs_config(struct platform_device *pdev,
*/
if (tcs->type == ACTIVE_TCS)
continue;
-
- tcs->cmd_cache = devm_kcalloc(&pdev->dev,
- tcs->num_tcs * ncpt, sizeof(u32),
- GFP_KERNEL);
- if (!tcs->cmd_cache)
- return -ENOMEM;
}
drv->num_tcs = st;
--
2.25.1.481.gfbce0eb801-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]>
---
drivers/soc/qcom/rpmh-rsc.c | 13 +++----------
1 file changed, 3 insertions(+), 10 deletions(-)
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index 099603bf14bf..a1298035bcd2 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -169,17 +169,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)) {
@@ -245,9 +238,9 @@ static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
* dedicated AMC, and therefore would invalidate the sleep and wake
* TCSes before making an active state request.
*/
- 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];
if (tcs->num_tcs) {
ret = rpmh_rsc_invalidate(drv);
if (ret)
--
2.25.1.481.gfbce0eb801-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]>
---
drivers/soc/qcom/rpmh-rsc.c | 31 ++++++++++++++++++-------------
1 file changed, 18 insertions(+), 13 deletions(-)
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index e278fc11fe5c..5c88b8cd5bf8 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -61,28 +61,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))
@@ -94,7 +99,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)
@@ -212,7 +217,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);
@@ -226,7 +231,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))) {
@@ -265,7 +270,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];
@@ -281,7 +286,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);
}
@@ -294,7 +299,7 @@ static void __tcs_trigger(struct rsc_drv *drv, int tcs_id)
* 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;
@@ -319,10 +324,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.25.1.481.gfbce0eb801-goog
Hi,
On Fri, Mar 6, 2020 at 4:00 PM Douglas Anderson <[email protected]> wrote:
>
> From trawling through the code (see the "A lot of comments" change) I
> found that "tcs_in_use" was only kept up-to-date for ACTIVE_ONLY TCSs.
> ...yet tcs_is_free() was checking the variable called from
> tcs_invalidate() and tcs_invalidate() is only used for non-ACTIVE_ONLY
> TCSs.
>
> Let's change tcs_invalidate() to just check the "RSC_DRV_STATUS"
> register, which was presumably the important part.
>
> It also feels like for ACTIVE_ONLY TCSs that it probably wasn't
> important to check the "RSC_DRV_STATUS". We'll keep doing it just in
> case but we'll add a warning if it ever actually mattered.
>
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
>
> drivers/soc/qcom/rpmh-rsc.c | 23 +++++++++++++++++++----
> 1 file changed, 19 insertions(+), 4 deletions(-)
After other RPMH email threads, it's possible that this patch isn't
quite right. ...but also the code wasn't quite right before.
Specifically if we have 0 active TCSs then it's possible that we've
used a wake TCS to send an active-only request. That would be a case
where "tcs_in_use" might be set and we'd need to make sure that
tcs_invalidate() checks it. However:
1. We need to add locking to tcs_invalidate() since "tcs_in_use" is
protected by drv->lock and tcs_invalidate() didn't grab that lock.
2. We should add documentation explaining the situation.
I'm still waiting on overall review / testing of my series, but I'll
put it on my list to address this for v2.
-Doug
Hi,
On Fri, Mar 6, 2020 at 4:00 PM Douglas Anderson <[email protected]> wrote:
>
> @@ -889,12 +793,6 @@ static int rpmh_probe_tcs_config(struct platform_device *pdev,
> */
> if (tcs->type == ACTIVE_TCS)
> continue;
> -
> - tcs->cmd_cache = devm_kcalloc(&pdev->dev,
> - tcs->num_tcs * ncpt, sizeof(u32),
> - GFP_KERNEL);
> - if (!tcs->cmd_cache)
> - return -ENOMEM;
During later code inspection I happened to notice that the "if" test
above the code I removed can also be removed. I'll do that in v2.
The code after the v1 patch doesn't hurt, it's just silly to have the
"if (blah) continue" at the end of the loop.
I'll wait on sending a v2 until I get some testing / review feedback
on v1 or enough time passes.
-Doug
Hi,
On 3/7/2020 5:29 AM, Douglas Anderson wrote:
> 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().
i agree that there are two different write function doing same thing except last addition (RSC_DRV_CMD_OFFSET * cmd_id)
can you please rename write_tcs_cmd() to write_tcs_reg(), add above operation in it, and then remove existing write_tcs_reg().
this way we have only one read and one write function.
so at the end we will two function as,
static u32 read_tcs_reg(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 +
RSC_DRV_CMD_OFFSET * cmd_id);
}
static void write_tcs_reg(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 +
RSC_DRV_CMD_OFFSET * cmd_id);
}
>
> 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.
With above change, i don't think you need to re-order this.
specifically from tcs->base, we find right "reg" first and if it happens to be tcs then intended tcs, and then cmd inside tcs.
Thanks,
Maulik
> Signed-off-by: Douglas Anderson <[email protected]>
> ---
>
> drivers/soc/qcom/rpmh-rsc.c | 31 ++++++++++++++++++-------------
> 1 file changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index e278fc11fe5c..5c88b8cd5bf8 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -61,28 +61,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))
> @@ -94,7 +99,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)
> @@ -212,7 +217,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);
> @@ -226,7 +231,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))) {
> @@ -265,7 +270,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];
> @@ -281,7 +286,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);
> }
>
> @@ -294,7 +299,7 @@ static void __tcs_trigger(struct rsc_drv *drv, int tcs_id)
> * 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;
> @@ -319,10 +324,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;
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Hi,
On 3/7/2020 5:29 AM, Douglas Anderson wrote:
> 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 (though just to be sure, best to try
> this atop Maulik's patches where it should be super obvious that we
> always invalidate before writing sleep/wake TCSs.
> - I think I've eliminated a possible deadlock on "nosmp" systems,
> though it was mostly theoretical.
> - Possibly we could get a warning in some cases if I misunderstood how
> tcs_is_free() works. It'd be easy to remove the warning, though.
>
> 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 can help these get tested.
Thanks,
Maulik
> and review them soon-ish so they can land
> and future patches can be based on them.
>
> 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.
>
> With all that, enjoy.
>
>
> Douglas Anderson (9):
> drivers: qcom: rpmh-rsc: Clean code reading/writing 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: A lot of comments
> drivers: qcom: rpmh-rsc: Only use "tcs_in_use" for ACTIVE_ONLY
> drivers: qcom: rpmh-rsc: Warning if tcs_write() used for non-active
> drivers: qcom: rpmh-rsc: spin_lock_irqsave() for tcs_invalidate()
> drivers: qcom: rpmh-rsc: Kill cmd_cache and find_match() with fire
>
> drivers/soc/qcom/rpmh-internal.h | 45 ++--
> drivers/soc/qcom/rpmh-rsc.c | 390 +++++++++++++++++++++++--------
> 2 files changed, 313 insertions(+), 122 deletions(-)
>
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Hi,
On 3/7/2020 5:29 AM, Douglas Anderson wrote:
> 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]>
> ---
>
> drivers/soc/qcom/rpmh-rsc.c | 13 +++----------
> 1 file changed, 3 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index 099603bf14bf..a1298035bcd2 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -169,17 +169,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)) {
> @@ -245,9 +238,9 @@ static struct tcs_group *get_tcs_for_msg(struct rsc_drv *drv,
> * dedicated AMC, and therefore would invalidate the sleep and wake
> * TCSes before making an active state request.
> */
> - 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];
> if (tcs->num_tcs) {
> ret = rpmh_rsc_invalidate(drv);
> if (ret)
Reviewed-by: Maulik Shah <[email protected]>
Thanks,
Maulik
--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
Hi,
On Wed, Mar 11, 2020 at 1:47 AM Maulik Shah <[email protected]> wrote:
>
> Hi,
>
> On 3/7/2020 5:29 AM, Douglas Anderson wrote:
> > 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().
>
> i agree that there are two different write function doing same thing except last addition (RSC_DRV_CMD_OFFSET * cmd_id)
>
> can you please rename write_tcs_cmd() to write_tcs_reg(), add above operation in it, and then remove existing write_tcs_reg().
> this way we have only one read and one write function.
>
> so at the end we will two function as,
>
> static u32 read_tcs_reg(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 +
> RSC_DRV_CMD_OFFSET * cmd_id);
> }
>
> static void write_tcs_reg(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 +
> RSC_DRV_CMD_OFFSET * cmd_id);
> }
I can if you insist and this is still better than the existing
(inconsistent) code.
...but I still feel that having two functions adds value here.
Anyone else who is CCed want to weigh in and tie break?
> > 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.
> With above change, i don't think you need to re-order this.
> specifically from tcs->base, we find right "reg" first and if it happens to be tcs then intended tcs, and then cmd inside tcs.
There was never any "need" to re-order. That math works out to be the
same. This is just clearer.
As an example, let's look at this:
struct point {
int x;
int y;
};
struct point points[10];
Let's say you have:
void *points_base = &(points[0]);
...and now you want to find &(points[5].y). What does your math look like?
a) points_base + (sizeof(struct point) * 5) + 4 ;
...or...
b) points_base + 4 + (sizeof(struct point) * 5);
Both calculations give the same result, but I am arguring that "a)" is
more intuitive. Specifically you deal with the array access first and
then deal with the offset within the structure that you found.
-Doug
Hi,
On Wed, Mar 11, 2020 at 08:03:27AM -0700, Doug Anderson wrote:
> Hi,
>
> On Wed, Mar 11, 2020 at 1:47 AM Maulik Shah <[email protected]> wrote:
> >
> > Hi,
> >
> > On 3/7/2020 5:29 AM, Douglas Anderson wrote:
> > > 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().
> >
> > i agree that there are two different write function doing same thing except last addition (RSC_DRV_CMD_OFFSET * cmd_id)
> >
> > can you please rename write_tcs_cmd() to write_tcs_reg(), add above operation in it, and then remove existing write_tcs_reg().
> > this way we have only one read and one write function.
> >
> > so at the end we will two function as,
> >
> > static u32 read_tcs_reg(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 +
> > RSC_DRV_CMD_OFFSET * cmd_id);
> > }
> >
> > static void write_tcs_reg(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 +
> > RSC_DRV_CMD_OFFSET * cmd_id);
> > }
>
> I can if you insist and this is still better than the existing
> (inconsistent) code.
>
> ...but I still feel that having two functions adds value here.
>
>
> Anyone else who is CCed want to weigh in and tie break?
I agree with Doug, having two functions makes the code that calls them
clearer. It makes it evident when a command is read/written and doesn't require
a useless extra parameter when accessing a non-command register.
> > > 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.
> > With above change, i don't think you need to re-order this.
> > specifically from tcs->base, we find right "reg" first and if it happens to be tcs then intended tcs, and then cmd inside tcs.
>
> There was never any "need" to re-order. That math works out to be the
> same. This is just clearer.
>
> As an example, let's look at this:
>
> struct point {
> int x;
> int y;
> };
> struct point points[10];
>
> Let's say you have:
> void *points_base = &(points[0]);
>
> ...and now you want to find &(points[5].y). What does your math look like?
>
> a) points_base + (sizeof(struct point) * 5) + 4 ;
>
> ...or...
>
> b) points_base + 4 + (sizeof(struct point) * 5);
>
>
> Both calculations give the same result, but I am arguring that "a)" is
> more intuitive. Specifically you deal with the array access first and
> then deal with the offset within the structure that you found.
+1
Quoting Matthias Kaehlcke (2020-03-11 09:17:26)
> Hi,
>
> On Wed, Mar 11, 2020 at 08:03:27AM -0700, Doug Anderson wrote:
> > Hi,
> >
> > On Wed, Mar 11, 2020 at 1:47 AM Maulik Shah <[email protected]> wrote:
> > >
> > > Hi,
> > >
> > > On 3/7/2020 5:29 AM, Douglas Anderson wrote:
> > > > 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().
> > >
> > > i agree that there are two different write function doing same thing except last addition (RSC_DRV_CMD_OFFSET * cmd_id)
> > >
> > > can you please rename write_tcs_cmd() to write_tcs_reg(), add above operation in it, and then remove existing write_tcs_reg().
> > > this way we have only one read and one write function.
> > >
> > > so at the end we will two function as,
> > >
> > > static u32 read_tcs_reg(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 +
> > > RSC_DRV_CMD_OFFSET * cmd_id);
> > > }
> > >
> > > static void write_tcs_reg(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 +
> > > RSC_DRV_CMD_OFFSET * cmd_id);
> > > }
> >
> > I can if you insist and this is still better than the existing
> > (inconsistent) code.
> >
> > ...but I still feel that having two functions adds value here.
> >
> >
> > Anyone else who is CCed want to weigh in and tie break?
>
> I agree with Doug, having two functions makes the code that calls them
> clearer. It makes it evident when a command is read/written and doesn't require
> a useless extra parameter when accessing a non-command register.
Me too! In fact, I asked for this when this driver was introduced and I
was half-ignored[1]. Making sure we never have to pass 0 to one of these
functions should be a goal.
From two years ago:
>
> Is m the type of TCS (sleep, active, wake) and n is just an offset?
> Maybe you can replace m with 'tcs_type' and n with 'index' or 'i' or
> 'offset'. And then don't use this function to write the random TCS
> registers that don't have to do with the TCS command slots? I see
> various places where there are things like:
>
> > + write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, m, 0, 0);
> > + write_tcs_reg(drv, RSC_DRV_CMD_WAIT_FOR_CMPL, m, 0, cmd_complete);
> > + write_tcs_reg(drv, RSC_DRV_CMD_ENABLE, m, 0, cmd_enable);
>
> And 'n' is 0, meaning you rely on that 0 killing that last part of the
> equation (RSC_DRV_CMD_OFFSET * n). But if we had a write_tcs_reg(drv,
> reg, m, data) and a write_tcs_cmd(drv, reg, m, n, data) then it would be
> clearer.
>
> Even better, add a void *base to a 'struct tcs' and then pass that
> struct to the tcs_read/write APIs and then have that pull out a
> tcs->base + reg or tcs->base + reg + RSC_DRV_CMD_OFFSET * index.
>
[1] https://lore.kernel.org/linux-arm-msm/[email protected]/