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

Commit Message

Maulik Shah Feb. 12, 2024, 4:48 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]>
Signed-off-by: Elliot Berman <[e-mail]>
Signed-off-by: Maulik Shah <[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   | 41 +++++++++++++++++++++++++++++++++++------
 drivers/soc/qcom/rpmh-rsc.c |  3 ++-
 include/soc/qcom/cmd-db.h   | 10 +++++++++-
 3 files changed, 46 insertions(+), 8 deletions(-)


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

Best regards,

Comments

Subbaraman Narayanamurthy Feb. 13, 2024, 3:52 a.m. UTC | #1

Hi Maulik,

> +bool cmd_db_match_resource_addr(u32 addr1, u32 addr2)
> +{

<snip>

> +	if (SLAVE_ID(addr1) == CMD_DB_HW_VRM
> +	    && VRM_ADDR(addr1) == VRM_ADDR(addr2))
> +		return true;
> +	else if (addr1 == addr2)
> +		return true;
> +	else
> +		return false;

Minor..it would be better if you modify it as following.

+	if (addr1 == addr2)
+		return true;
+	else if (SLAVE_ID(addr1) == CMD_DB_HW_VRM
+	    && VRM_ADDR(addr1) == VRM_ADDR(addr2))
+		return true;
+
+	return false;

-Subbaraman

--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project.

Mukesh Ojha Feb. 13, 2024, 5:54 a.m. UTC | #2

On 2/13/2024 9:22 AM, Subbaraman Narayanamurthy wrote:
> Hi Maulik,
> 
>> +bool cmd_db_match_resource_addr(u32 addr1, u32 addr2)
>> +{
> 
> <snip>
> 
>> +	if (SLAVE_ID(addr1) == CMD_DB_HW_VRM
>> +	    && VRM_ADDR(addr1) == VRM_ADDR(addr2))
>> +		return true;
>> +	else if (addr1 == addr2)
>> +		return true;
>> +	else
>> +		return false;
> 
> Minor..it would be better if you modify it as following.
> 
> +	if (addr1 == addr2)
> +		return true;
> +	else if (SLAVE_ID(addr1) == CMD_DB_HW_VRM
> +	    && VRM_ADDR(addr1) == VRM_ADDR(addr2))
> +		return true;
> +
> +	return false;

Even better if it becomes one statement for true rest with
false..
> 
> -Subbaraman
> 
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project.

Probably, you don't want to include this in your reply.

-Mukesh

>

Maulik Shah Feb. 13, 2024, 9:51 a.m. UTC | #3

Hi,

On 2/13/2024 11:24 AM, Mukesh Ojha wrote:
> 
> 
> On 2/13/2024 9:22 AM, Subbaraman Narayanamurthy wrote:
>> Hi Maulik,
>>
>>> +bool cmd_db_match_resource_addr(u32 addr1, u32 addr2)
>>> +{
>>
>> <snip>
>>
>>> +    if (SLAVE_ID(addr1) == CMD_DB_HW_VRM
>>> +        && VRM_ADDR(addr1) == VRM_ADDR(addr2))
>>> +        return true;
>>> +    else if (addr1 == addr2)
>>> +        return true;
>>> +    else
>>> +        return false;
>>
>> Minor..it would be better if you modify it as following.
>>
>> +    if (addr1 == addr2)
>> +        return true;
>> +    else if (SLAVE_ID(addr1) == CMD_DB_HW_VRM
>> +        && VRM_ADDR(addr1) == VRM_ADDR(addr2))
>> +        return true;
>> +
>> +    return false;
> 
> Even better if it becomes one statement for true rest with
> false..

Thanks for the review.

I will fix in v4, will wait for sometime if there are any other comments 
to take care along with this.

Thanks,
Maulik

Bjorn Andersson Feb. 14, 2024, 6:08 a.m. UTC | #4

On Mon, Feb 12, 2024 at 10:18:08AM +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]>
> Signed-off-by: Elliot Berman <[e-mail]>
> Signed-off-by: Maulik Shah <[e-mail]>

This says, "Elliot first certified the origin of the patch, then Maulik
took and certified the origin of the patch". But according to the From:
the author of the patch is you, Maulik.

How was Elliot able to certify the patch's origin before you, when
you're the author?

If the two of you collaborated, also add Co-developed-by: Elliot above
his s-o-b.

> ---
> 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   | 41 +++++++++++++++++++++++++++++++++++------
>  drivers/soc/qcom/rpmh-rsc.c |  3 ++-
>  include/soc/qcom/cmd-db.h   | 10 +++++++++-
>  3 files changed, 46 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c
> index a5fd68411bed..e87682b9755e 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>
> @@ -15,8 +19,8 @@
>  
>  #define NUM_PRIORITY		2
>  #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
> @@ -221,9 +225,34 @@ const void *cmd_db_read_aux_data(const char *id, size_t *len)
>  EXPORT_SYMBOL_GPL(cmd_db_read_aux_data);
>  
>  /**
> - * cmd_db_read_slave_id - Get the slave ID for a given resource address
> + * cmd_db_match_resource_addr - Compare if both Resource addresses are same

() after the function name, please.

> + *
> + * @addr1: Resource address to compare
> + * @addr2: Resource address to compare
> + *
> + * Return: true on matching addresses, false otherwise

"Return: true if the two addresses refer to the same resource"

> + */
> +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 (SLAVE_ID(addr1) == CMD_DB_HW_VRM
> +	    && VRM_ADDR(addr1) == VRM_ADDR(addr2))

One line please, it's just 83 characters.

> +		return true;
> +	else if (addr1 == addr2)
> +		return true;
> +	else
> +		return false;
> +}
> +EXPORT_SYMBOL_GPL(cmd_db_match_resource_addr);
> +
> +/**
> + * cmd_db_read_slave_id - Get the slave ID for a given resource name
>   *
> - * @id: Resource id to query the DB for version
> + * @id: Resource id to query the DB for slave id

Although trivial, it's unrelated to the newly introduced logic. Please
submit a separate patch. Please also then add the () after the function
name.

Regards,
Bjorn

>   *
>   * Return: cmd_db_hw_type enum on success, CMD_DB_HW_INVALID on error
>   */
> @@ -238,7 +267,7 @@ enum cmd_db_hw_type cmd_db_read_slave_id(const char *id)
>  		return CMD_DB_HW_INVALID;
>  
>  	addr = le32_to_cpu(ent->addr);
> -	return (addr >> SLAVE_ID_SHIFT) & SLAVE_ID_MASK;
> +	return SLAVE_ID(addr);
>  }
>  EXPORT_SYMBOL_GPL(cmd_db_read_slave_id);
>  
> 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 Feb. 14, 2024, 6:59 p.m. UTC | #5

On Wed, Feb 14, 2024 at 12:08:43AM -0600, Bjorn Andersson wrote:
> On Mon, Feb 12, 2024 at 10:18:08AM +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]>
> > Signed-off-by: Elliot Berman <[e-mail]>
> > Signed-off-by: Maulik Shah <[e-mail]>
> 
> This says, "Elliot first certified the origin of the patch, then Maulik
> took and certified the origin of the patch". But according to the From:
> the author of the patch is you, Maulik.
> 
> How was Elliot able to certify the patch's origin before you, when
> you're the author?
> 
> If the two of you collaborated, also add Co-developed-by: Elliot above
> his s-o-b.
> 

Even my Co-developed-by is being generous :-) All I had done was copy
the commit and resolve couple merge conflicts.

Maulik, you can swap my S-o-B for a:

Tested-by: Elliot Berman <[e-mail]> # sm8650-qrd

> > ---
> > 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   | 41 +++++++++++++++++++++++++++++++++++------
> >  drivers/soc/qcom/rpmh-rsc.c |  3 ++-
> >  include/soc/qcom/cmd-db.h   | 10 +++++++++-
> >  3 files changed, 46 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c
> > index a5fd68411bed..e87682b9755e 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>
> > @@ -15,8 +19,8 @@
> >  
> >  #define NUM_PRIORITY		2
> >  #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
> > @@ -221,9 +225,34 @@ const void *cmd_db_read_aux_data(const char *id, size_t *len)
> >  EXPORT_SYMBOL_GPL(cmd_db_read_aux_data);
> >  
> >  /**
> > - * cmd_db_read_slave_id - Get the slave ID for a given resource address
> > + * cmd_db_match_resource_addr - Compare if both Resource addresses are same
> 
> () after the function name, please.
> 
> > + *
> > + * @addr1: Resource address to compare
> > + * @addr2: Resource address to compare
> > + *
> > + * Return: true on matching addresses, false otherwise
> 
> "Return: true if the two addresses refer to the same resource"
> 
> > + */
> > +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 (SLAVE_ID(addr1) == CMD_DB_HW_VRM
> > +	    && VRM_ADDR(addr1) == VRM_ADDR(addr2))
> 
> One line please, it's just 83 characters.
> 
> > +		return true;
> > +	else if (addr1 == addr2)
> > +		return true;
> > +	else
> > +		return false;
> > +}
> > +EXPORT_SYMBOL_GPL(cmd_db_match_resource_addr);
> > +
> > +/**
> > + * cmd_db_read_slave_id - Get the slave ID for a given resource name
> >   *
> > - * @id: Resource id to query the DB for version
> > + * @id: Resource id to query the DB for slave id
> 
> Although trivial, it's unrelated to the newly introduced logic. Please
> submit a separate patch. Please also then add the () after the function
> name.
> 
> Regards,
> Bjorn
> 
> >   *
> >   * Return: cmd_db_hw_type enum on success, CMD_DB_HW_INVALID on error
> >   */
> > @@ -238,7 +267,7 @@ enum cmd_db_hw_type cmd_db_read_slave_id(const char *id)
> >  		return CMD_DB_HW_INVALID;
> >  
> >  	addr = le32_to_cpu(ent->addr);
> > -	return (addr >> SLAVE_ID_SHIFT) & SLAVE_ID_MASK;
> > +	return SLAVE_ID(addr);
> >  }
> >  EXPORT_SYMBOL_GPL(cmd_db_read_slave_id);
> >  
> > 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]>
> >

Maulik Shah Feb. 15, 2024, 4:02 a.m. UTC | #6

Hi,

On 2/14/2024 11:38 AM, Bjorn Andersson wrote:

>>   /**
>> - * cmd_db_read_slave_id - Get the slave ID for a given resource address
>> + * cmd_db_match_resource_addr - Compare if both Resource addresses are same
> 
> () after the function name, please.
> 

Thanks for the review, Addressed in v4.

>> + *
>> + * @addr1: Resource address to compare
>> + * @addr2: Resource address to compare
>> + *
>> + * Return: true on matching addresses, false otherwise
> 
> "Return: true if the two addresses refer to the same resource"
> 

Addressed in v4.

>> + */
>> +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 (SLAVE_ID(addr1) == CMD_DB_HW_VRM
>> +	    && VRM_ADDR(addr1) == VRM_ADDR(addr2))
> 
> One line please, it's just 83 characters.

Addressed in v4.

>> +		return true;
>> +	else if (addr1 == addr2)
>> +		return true;
>> +	else
>> +		return false;
>> +}
>> +EXPORT_SYMBOL_GPL(cmd_db_match_resource_addr);
>> +
>> +/**
>> + * cmd_db_read_slave_id - Get the slave ID for a given resource name
>>    *
>> - * @id: Resource id to query the DB for version
>> + * @id: Resource id to query the DB for slave id
> 
> Although trivial, it's unrelated to the newly introduced logic. Please
> submit a separate patch. Please also then add the () after the function
> name.
> 
> Regards,
> Bjorn

Sure, i will send out separate patch for other comments to update.

Thanks,
Maulik

Maulik Shah Feb. 15, 2024, 4:04 a.m. UTC | #7

Hi,

On 2/15/2024 12:29 AM, Elliot Berman wrote:
> On Wed, Feb 14, 2024 at 12:08:43AM -0600, Bjorn Andersson wrote:
>> On Mon, Feb 12, 2024 at 10:18:08AM +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]>
>>> Signed-off-by: Elliot Berman <[e-mail]>
>>> Signed-off-by: Maulik Shah <[e-mail]>
>>
>> This says, "Elliot first certified the origin of the patch, then Maulik
>> took and certified the origin of the patch". But according to the From:
>> the author of the patch is you, Maulik.
>>
>> How was Elliot able to certify the patch's origin before you, when
>> you're the author?
>>
>> If the two of you collaborated, also add Co-developed-by: Elliot above
>> his s-o-b.
>>
> 
> Even my Co-developed-by is being generous :-) All I had done was copy
> the commit and resolve couple merge conflicts.
> 
> Maulik, you can swap my S-o-B for a:
> 
> Tested-by: Elliot Berman <[e-mail]> # sm8650-qrd
> 

Addressed in v4 to change to tested-by.

Thanks,
Maulik

Patch

diff --git a/drivers/soc/qcom/cmd-db.c b/drivers/soc/qcom/cmd-db.c
index a5fd68411bed..e87682b9755e 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>
@@ -15,8 +19,8 @@ 
 
 #define NUM_PRIORITY		2
 #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
@@ -221,9 +225,34 @@  const void *cmd_db_read_aux_data(const char *id, size_t *len)
 EXPORT_SYMBOL_GPL(cmd_db_read_aux_data);
 
 /**
- * cmd_db_read_slave_id - Get the slave ID for a given resource address
+ * cmd_db_match_resource_addr - Compare if both Resource addresses are same
+ *
+ * @addr1: Resource address to compare
+ * @addr2: Resource address to compare
+ *
+ * Return: true on matching addresses, 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 (SLAVE_ID(addr1) == CMD_DB_HW_VRM
+	    && VRM_ADDR(addr1) == VRM_ADDR(addr2))
+		return true;
+	else if (addr1 == addr2)
+		return true;
+	else
+		return false;
+}
+EXPORT_SYMBOL_GPL(cmd_db_match_resource_addr);
+
+/**
+ * cmd_db_read_slave_id - Get the slave ID for a given resource name
  *
- * @id: Resource id to query the DB for version
+ * @id: Resource id to query the DB for slave id
  *
  * Return: cmd_db_hw_type enum on success, CMD_DB_HW_INVALID on error
  */
@@ -238,7 +267,7 @@  enum cmd_db_hw_type cmd_db_read_slave_id(const char *id)
 		return CMD_DB_HW_INVALID;
 
 	addr = le32_to_cpu(ent->addr);
-	return (addr >> SLAVE_ID_SHIFT) & SLAVE_ID_MASK;
+	return SLAVE_ID(addr);
 }
 EXPORT_SYMBOL_GPL(cmd_db_read_slave_id);
 
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; }