[v4] soc: qcom: rpmh-rsc: Enhance check for VRM in-flight request

Commit Message

Maulik Shah (mkshah) Feb. 15, 2024, 5:25 a.m. UTC

Each RPMh VRM accelerator resource has 3 or 4 contiguous 4-byte aligned
addresses associated with it. These control voltage, enable state, mode,
and in legacy targets, voltage headroom. The current in-flight request
checking logic looks for exact address matches. Requests for different
addresses of the same RPMh resource as thus not detected as in-flight.

Add new cmd-db API cmd_db_match_resource_addr() to enhance the in-flight
request check for VRM requests by ignoring the address offset.

This ensures that only one request is allowed to be in-flight for a given
VRM resource. This is needed to avoid scenarios where request commands are
carried out by RPMh hardware out-of-order leading to LDO regulator
over-current protection triggering.

Fixes: 658628e7ef78 ("drivers: qcom: rpmh-rsc: add RPMH controller for QCOM SoCs")
cc: [e-mail]
Reviewed-by: Konrad Dybcio <[e-mail]>
Tested-by: Elliot Berman <[e-mail]> # sm8650-qrd
Signed-off-by: Maulik Shah <[e-mail]>
---
Changes in v4:
- Simplify cmd_db_match_resource_addr()
- Remove unrelated changes to newly added logic
- Update function description comments
- Replace Signed-off-by: with Tested-by: from Elliot
- Link to v3: https://lore.kernel.org/r/[e-mail]
Changes in v3:
- Fix s-o-b chain
- Add cmd-db API to compare addresses
- Reuse already defined resource types in cmd-db
- Add Fixes tag and Cc to stable
- Retain Reviewed-by tag of v2
- Link to v2: https://lore.kernel.org/r/[e-mail]
Changes in v2:
- Use GENMASK() and FIELD_GET()
- Link to v1: https://lore.kernel.org/r/[e-mail]
---
 drivers/soc/qcom/cmd-db.c   | 32 +++++++++++++++++++++++++++++++-
 drivers/soc/qcom/rpmh-rsc.c |  3 ++-
 include/soc/qcom/cmd-db.h   | 10 +++++++++-
 3 files changed, 42 insertions(+), 3 deletions(-)


---
base-commit: 615d300648869c774bd1fe54b4627bb0c20faed4
change-id: 20240210-rpmh-rsc-fixes-372a79ab364b

Best regards,

Comments

Bjorn Andersson March 18, 2024, 3:18 a.m. UTC | #1

On Thu, Feb 15, 2024 at 10:55:44AM +0530, Maulik Shah wrote:
> Each RPMh VRM accelerator resource has 3 or 4 contiguous 4-byte aligned
> addresses associated with it. These control voltage, enable state, mode,
> and in legacy targets, voltage headroom. The current in-flight request
> checking logic looks for exact address matches. Requests for different
> addresses of the same RPMh resource as thus not detected as in-flight.
> 
> Add new cmd-db API cmd_db_match_resource_addr() to enhance the in-flight
> request check for VRM requests by ignoring the address offset.
> 
> This ensures that only one request is allowed to be in-flight for a given
> VRM resource. This is needed to avoid scenarios where request commands are
> carried out by RPMh hardware out-of-order leading to LDO regulator
> over-current protection triggering.
> 
> Fixes: 658628e7ef78 ("drivers: qcom: rpmh-rsc: add RPMH controller for QCOM SoCs")
> cc: [e-mail]
> Reviewed-by: Konrad Dybcio <[e-mail]>
> Tested-by: Elliot Berman <[e-mail]> # sm8650-qrd
> Signed-off-by: Maulik Shah <[e-mail]>
> ---
> Changes in v4:
> - Simplify cmd_db_match_resource_addr()
> - Remove unrelated changes to newly added logic
> - Update function description comments

You changed things, but retained Konrad's Reviewed-by...

> - Replace Signed-off-by: with Tested-by: from Elliot

Did Elliot test v4, or did you just translate his incorrect(?) s-o-b
from v1 into a tested-by?

We're about to write into the git history that Elliot says this version
is tested to the best of his abilities...

> - Link to v3: https://lore.kernel.org/r/[e-mail]
> Changes in v3:
> - Fix s-o-b chain
> - Add cmd-db API to compare addresses
> - Reuse already defined resource types in cmd-db
> - Add Fixes tag and Cc to stable
> - Retain Reviewed-by tag of v2
> - Link to v2: https://lore.kernel.org/r/[e-mail]
> Changes in v2:
> - Use GENMASK() and FIELD_GET()
> - Link to v1: https://lore.kernel.org/r/[e-mail]
> ---
>  drivers/soc/qcom/cmd-db.c   | 32 +++++++++++++++++++++++++++++++-
>  drivers/soc/qcom/rpmh-rsc.c |  3 ++-
>  include/soc/qcom/cmd-db.h   | 10 +++++++++-
>  3 files changed, 42 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c
> index a5fd68411bed..86fb2cd4f484 100644
> --- a/drivers/soc/qcom/cmd-db.c
> +++ b/drivers/soc/qcom/cmd-db.c
> @@ -1,6 +1,10 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
> -/* Copyright (c) 2016-2018, 2020, The Linux Foundation. All rights reserved. */
> +/*
> + * Copyright (c) 2016-2018, 2020, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
>  
> +#include <linux/bitfield.h>
>  #include <linux/debugfs.h>
>  #include <linux/kernel.h>
>  #include <linux/module.h>
> @@ -17,6 +21,8 @@
>  #define MAX_SLV_ID		8
>  #define SLAVE_ID_MASK		0x7
>  #define SLAVE_ID_SHIFT		16
> +#define SLAVE_ID(addr)		FIELD_GET(GENMASK(19, 16), addr)
> +#define VRM_ADDR(addr)		FIELD_GET(GENMASK(19, 4), addr)
>  
>  /**
>   * struct entry_header: header for each entry in cmddb
> @@ -220,6 +226,30 @@ const void *cmd_db_read_aux_data(const char *id, size_t *len)
>  }
>  EXPORT_SYMBOL_GPL(cmd_db_read_aux_data);
>  
> +/**
> + * cmd_db_match_resource_addr() - Compare if both Resource addresses are same
> + *
> + * @addr1: Resource address to compare
> + * @addr2: Resource address to compare
> + *
> + * Return: true if two addresses refer to the same resource, false otherwise
> + */
> +bool cmd_db_match_resource_addr(u32 addr1, u32 addr2)
> +{
> +	/*
> +	 * Each RPMh VRM accelerator resource has 3 or 4 contiguous 4-byte
> +	 * aligned addresses associated with it. Ignore the offset to check
> +	 * for VRM requests.
> +	 */
> +	if (addr1 == addr2)
> +		return true;
> +	else if (SLAVE_ID(addr1) == CMD_DB_HW_VRM && VRM_ADDR(addr1) == VRM_ADDR(addr2))

Is it a fact or a coincidence that GENMASK(19, 16) of the address
happens to have the same value as the associated rsc_hdr's slv_id?


I recognize that we probably don't want to start naively traversing the
headers here, but as previously requested I would still like to have an
interface where I can pass in an address and have its entry_header->id
resolved, for tracing/debug purposes. Perhaps there's some common
ground?

Regards,
Bjorn

> +		return true;
> +
> +	return false;
> +}
> +EXPORT_SYMBOL_GPL(cmd_db_match_resource_addr);
> +
>  /**
>   * cmd_db_read_slave_id - Get the slave ID for a given resource address
>   *
> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
> index a021dc71807b..daf64be966fe 100644
> --- a/drivers/soc/qcom/rpmh-rsc.c
> +++ b/drivers/soc/qcom/rpmh-rsc.c
> @@ -1,6 +1,7 @@
>  // SPDX-License-Identifier: GPL-2.0
>  /*
>   * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
>   */
>  
>  #define pr_fmt(fmt) "%s " fmt, KBUILD_MODNAME
> @@ -557,7 +558,7 @@ static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
>  		for_each_set_bit(j, &curr_enabled, MAX_CMDS_PER_TCS) {
>  			addr = read_tcs_cmd(drv, drv->regs[RSC_DRV_CMD_ADDR], i, j);
>  			for (k = 0; k < msg->num_cmds; k++) {
> -				if (addr == msg->cmds[k].addr)
> +				if (cmd_db_match_resource_addr(msg->cmds[k].addr, addr))
>  					return -EBUSY;
>  			}
>  		}
> diff --git a/include/soc/qcom/cmd-db.h b/include/soc/qcom/cmd-db.h
> index c8bb56e6852a..47a6cab75e63 100644
> --- a/include/soc/qcom/cmd-db.h
> +++ b/include/soc/qcom/cmd-db.h
> @@ -1,5 +1,8 @@
>  /* SPDX-License-Identifier: GPL-2.0 */
> -/* Copyright (c) 2016-2018, The Linux Foundation. All rights reserved. */
> +/*
> + * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
> + * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
> + */
>  
>  #ifndef __QCOM_COMMAND_DB_H__
>  #define __QCOM_COMMAND_DB_H__
> @@ -21,6 +24,8 @@ u32 cmd_db_read_addr(const char *resource_id);
>  
>  const void *cmd_db_read_aux_data(const char *resource_id, size_t *len);
>  
> +bool cmd_db_match_resource_addr(u32 addr1, u32 addr2);
> +
>  enum cmd_db_hw_type cmd_db_read_slave_id(const char *resource_id);
>  
>  int cmd_db_ready(void);
> @@ -31,6 +36,9 @@ static inline u32 cmd_db_read_addr(const char *resource_id)
>  static inline const void *cmd_db_read_aux_data(const char *resource_id, size_t *len)
>  { return ERR_PTR(-ENODEV); }
>  
> +static inline bool cmd_db_match_resource_addr(u32 addr1, u32 addr2)
> +{ return false; }
> +
>  static inline enum cmd_db_hw_type cmd_db_read_slave_id(const char *resource_id)
>  { return -ENODEV; }
>  
> 
> ---
> base-commit: 615d300648869c774bd1fe54b4627bb0c20faed4
> change-id: 20240210-rpmh-rsc-fixes-372a79ab364b
> 
> Best regards,
> -- 
> Maulik Shah <[e-mail]>
>

Elliot Berman March 18, 2024, 9:22 p.m. UTC | #2

On Sun, Mar 17, 2024 at 10:18:25PM -0500, Bjorn Andersson wrote:
> On Thu, Feb 15, 2024 at 10:55:44AM +0530, Maulik Shah wrote:
> > Each RPMh VRM accelerator resource has 3 or 4 contiguous 4-byte aligned
> > addresses associated with it. These control voltage, enable state, mode,
> > and in legacy targets, voltage headroom. The current in-flight request
> > checking logic looks for exact address matches. Requests for different
> > addresses of the same RPMh resource as thus not detected as in-flight.
> > 
> > Add new cmd-db API cmd_db_match_resource_addr() to enhance the in-flight
> > request check for VRM requests by ignoring the address offset.
> > 
> > This ensures that only one request is allowed to be in-flight for a given
> > VRM resource. This is needed to avoid scenarios where request commands are
> > carried out by RPMh hardware out-of-order leading to LDO regulator
> > over-current protection triggering.
> > 
> > Fixes: 658628e7ef78 ("drivers: qcom: rpmh-rsc: add RPMH controller for QCOM SoCs")
> > cc: [e-mail]
> > Reviewed-by: Konrad Dybcio <[e-mail]>
> > Tested-by: Elliot Berman <[e-mail]> # sm8650-qrd
> > Signed-off-by: Maulik Shah <[e-mail]>
> > ---
> > Changes in v4:
> > - Simplify cmd_db_match_resource_addr()
> > - Remove unrelated changes to newly added logic
> > - Update function description comments
> 
> You changed things, but retained Konrad's Reviewed-by...
> 
> > - Replace Signed-off-by: with Tested-by: from Elliot
> 
> Did Elliot test v4, or did you just translate his incorrect(?) s-o-b
> from v1 into a tested-by?
> 
> We're about to write into the git history that Elliot says this version
> is tested to the best of his abilities...
> 

No, I asked for it:

https://lore.kernel.org/all/[e-mail]/

Bjorn Andersson April 21, 2024, 10:29 p.m. UTC | #3

On Thu, 15 Feb 2024 10:55:44 +0530, Maulik Shah wrote:
> Each RPMh VRM accelerator resource has 3 or 4 contiguous 4-byte aligned
> addresses associated with it. These control voltage, enable state, mode,
> and in legacy targets, voltage headroom. The current in-flight request
> checking logic looks for exact address matches. Requests for different
> addresses of the same RPMh resource as thus not detected as in-flight.
> 
> Add new cmd-db API cmd_db_match_resource_addr() to enhance the in-flight
> request check for VRM requests by ignoring the address offset.
> 
> [...]

Applied, thanks!

[1/1] soc: qcom: rpmh-rsc: Enhance check for VRM in-flight request
      commit: f592cc5794747b81e53b53dd6e80219ee25f0611

Best regards,

Patch

diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c
index a5fd68411bed..86fb2cd4f484 100644
--- a/drivers/soc/qcom/cmd-db.c
+++ b/drivers/soc/qcom/cmd-db.c
@@ -1,6 +1,10 @@ 
 /* SPDX-License-Identifier: GPL-2.0 */
-/* Copyright (c) 2016-2018, 2020, The Linux Foundation. All rights reserved. */
+/*
+ * Copyright (c) 2016-2018, 2020, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
+ */
 
+#include <linux/bitfield.h>
 #include <linux/debugfs.h>
 #include <linux/kernel.h>
 #include <linux/module.h>
@@ -17,6 +21,8 @@ 
 #define MAX_SLV_ID		8
 #define SLAVE_ID_MASK		0x7
 #define SLAVE_ID_SHIFT		16
+#define SLAVE_ID(addr)		FIELD_GET(GENMASK(19, 16), addr)
+#define VRM_ADDR(addr)		FIELD_GET(GENMASK(19, 4), addr)
 
 /**
  * struct entry_header: header for each entry in cmddb
@@ -220,6 +226,30 @@  const void *cmd_db_read_aux_data(const char *id, size_t *len)
 }
 EXPORT_SYMBOL_GPL(cmd_db_read_aux_data);
 
+/**
+ * cmd_db_match_resource_addr() - Compare if both Resource addresses are same
+ *
+ * @addr1: Resource address to compare
+ * @addr2: Resource address to compare
+ *
+ * Return: true if two addresses refer to the same resource, false otherwise
+ */
+bool cmd_db_match_resource_addr(u32 addr1, u32 addr2)
+{
+	/*
+	 * Each RPMh VRM accelerator resource has 3 or 4 contiguous 4-byte
+	 * aligned addresses associated with it. Ignore the offset to check
+	 * for VRM requests.
+	 */
+	if (addr1 == addr2)
+		return true;
+	else if (SLAVE_ID(addr1) == CMD_DB_HW_VRM && VRM_ADDR(addr1) == VRM_ADDR(addr2))
+		return true;
+
+	return false;
+}
+EXPORT_SYMBOL_GPL(cmd_db_match_resource_addr);
+
 /**
  * cmd_db_read_slave_id - Get the slave ID for a given resource address
  *
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c
index a021dc71807b..daf64be966fe 100644
--- a/drivers/soc/qcom/rpmh-rsc.c
+++ b/drivers/soc/qcom/rpmh-rsc.c
@@ -1,6 +1,7 @@ 
 // SPDX-License-Identifier: GPL-2.0
 /*
  * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2023-2024, Qualcomm Innovation Center, Inc. All rights reserved.
  */
 
 #define pr_fmt(fmt) "%s " fmt, KBUILD_MODNAME
@@ -557,7 +558,7 @@  static int check_for_req_inflight(struct rsc_drv *drv, struct tcs_group *tcs,
 		for_each_set_bit(j, &curr_enabled, MAX_CMDS_PER_TCS) {
 			addr = read_tcs_cmd(drv, drv->regs[RSC_DRV_CMD_ADDR], i, j);
 			for (k = 0; k < msg->num_cmds; k++) {
-				if (addr == msg->cmds[k].addr)
+				if (cmd_db_match_resource_addr(msg->cmds[k].addr, addr))
 					return -EBUSY;
 			}
 		}
diff --git a/include/soc/qcom/cmd-db.h b/include/soc/qcom/cmd-db.h
index c8bb56e6852a..47a6cab75e63 100644
--- a/include/soc/qcom/cmd-db.h
+++ b/include/soc/qcom/cmd-db.h
@@ -1,5 +1,8 @@ 
 /* SPDX-License-Identifier: GPL-2.0 */
-/* Copyright (c) 2016-2018, The Linux Foundation. All rights reserved. */
+/*
+ * Copyright (c) 2016-2018, The Linux Foundation. All rights reserved.
+ * Copyright (c) 2024, Qualcomm Innovation Center, Inc. All rights reserved.
+ */
 
 #ifndef __QCOM_COMMAND_DB_H__
 #define __QCOM_COMMAND_DB_H__
@@ -21,6 +24,8 @@  u32 cmd_db_read_addr(const char *resource_id);
 
 const void *cmd_db_read_aux_data(const char *resource_id, size_t *len);
 
+bool cmd_db_match_resource_addr(u32 addr1, u32 addr2);
+
 enum cmd_db_hw_type cmd_db_read_slave_id(const char *resource_id);
 
 int cmd_db_ready(void);
@@ -31,6 +36,9 @@  static inline u32 cmd_db_read_addr(const char *resource_id)
 static inline const void *cmd_db_read_aux_data(const char *resource_id, size_t *len)
 { return ERR_PTR(-ENODEV); }
 
+static inline bool cmd_db_match_resource_addr(u32 addr1, u32 addr2)
+{ return false; }
+
 static inline enum cmd_db_hw_type cmd_db_read_slave_id(const char *resource_id)
 { return -ENODEV; }