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: stable@xxxxxxxxxxxxxxx
> > Reviewed-by: Konrad Dybcio <konrad.dybcio@xxxxxxxxxx>
> > Signed-off-by: Elliot Berman <quic_eberman@xxxxxxxxxxx>
> > Signed-off-by: Maulik Shah <quic_mkshah@xxxxxxxxxxx>
>
> 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 <quic_eberman@xxxxxxxxxxx> # 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/20240119-rpmh-rsc-fixes-v2-1-e42c0a9e36f0@xxxxxxxxxxx
> > Changes in v2:
> > - Use GENMASK() and FIELD_GET()
> > - Link to v1: https://lore.kernel.org/r/20240117-rpmh-rsc-fixes-v1-1-71ee4f8f72a4@xxxxxxxxxxx
> > ---
> > 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 <quic_mkshah@xxxxxxxxxxx>
> >
