| Message ID | 20240117-rpmh-rsc-fixes-v1-1-71ee4f8f72a4@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | soc: qcom: rpmh-rsc: Enhance check for VREG in-flight request | expand |
On 1/17/24 09:54, Maulik Shah wrote: > Each RPMh VREG 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. > > Enhance the in-flight request check for VREG requests by ignoring the > address offset. This ensures that only one request is allowed to be > in-flight for a given VREG 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. > > Signed-off-by: Maulik Shah <quic_mkshah@quicinc.com> > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> > --- > drivers/soc/qcom/rpmh-rsc.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c > index a021dc71807b..5371d7e3090a 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 > @@ -91,6 +92,15 @@ enum { > #define CMD_STATUS_ISSUED BIT(8) > #define CMD_STATUS_COMPL BIT(16) > > +#define ACCL_TYPE(addr) ((addr >> 16) & 0xF) > +#define VREG_ADDR(addr) (addr & ~0xF) It would be nice to add some #define FNAME GENMASK(x, y) accessed with FIELD_GET(FNAME, foobar), so that the code is a bit more self-explanatory Konrad
Hi, On 1/17/2024 5:19 PM, Konrad Dybcio wrote: >> diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c >> index a021dc71807b..5371d7e3090a 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 >> @@ -91,6 +92,15 @@ enum { >> #define CMD_STATUS_ISSUED BIT(8) >> #define CMD_STATUS_COMPL BIT(16) >> +#define ACCL_TYPE(addr) ((addr >> 16) & 0xF) >> +#define VREG_ADDR(addr) (addr & ~0xF) > > It would be nice to add some #define FNAME GENMASK(x, y) accessed > with FIELD_GET(FNAME, foobar), so that the code is a bit more > self-explanatory > > Konrad Thanks for the review. Updates in v2 to use GENMASK() and FIELD_GET(). Thanks, Maulik
On Wed, Jan 17, 2024 at 02:24:10PM +0530, Maulik Shah wrote: > Each RPMh VREG 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. > > Enhance the in-flight request check for VREG requests by ignoring the > address offset. This ensures that only one request is allowed to be > in-flight for a given VREG 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. > > Signed-off-by: Maulik Shah <quic_mkshah@quicinc.com> > Signed-off-by: Elliot Berman <quic_eberman@quicinc.com> Two minor things: 1. Does this deserve a Fixes: tag? 2. The Signed-off-by chain here confuses me, you sent the patch so your SOB should be last, but then that makes me believe Elliot was the author which I don't think is reflected here (no From: line). Please read [0] for a bit more details [0] https://www.kernel.org/doc/html/latest/process/submitting-patches.html#developer-s-certificate-of-origin-1-1 > --- > drivers/soc/qcom/rpmh-rsc.c | 20 +++++++++++++++++++- > 1 file changed, 19 insertions(+), 1 deletion(-) > > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c > index a021dc71807b..5371d7e3090a 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 > @@ -91,6 +92,15 @@ enum { > #define CMD_STATUS_ISSUED BIT(8) > #define CMD_STATUS_COMPL BIT(16) > > +#define ACCL_TYPE(addr) ((addr >> 16) & 0xF) > +#define VREG_ADDR(addr) (addr & ~0xF) > + > +enum { > + HW_ACCL_CLK = 0x3, > + HW_ACCL_VREG, > + HW_ACCL_BUS, > +}; > + > /* > * Here's a high level overview of how all the registers in RPMH work > * together: > @@ -557,7 +567,15 @@ 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) > + /* > + * Each RPMh VREG accelerator resource has 3 or 4 contiguous 4-byte > + * aligned addresses associated with it. Ignore the offset to check > + * for in-flight VREG requests. > + */ > + if (HW_ACCL_VREG == ACCL_TYPE(msg->cmds[k].addr) && > + VREG_ADDR(addr) == VREG_ADDR(msg->cmds[k].addr)) > + return -EBUSY; > + else if (addr == msg->cmds[k].addr) > return -EBUSY; > } > } > > --- > base-commit: 943b9f0ab2cfbaea148dd6ac279957eb08b96904 > change-id: 20240117-rpmh-rsc-fixes-6c43c7051828 > > Best regards, > -- > Maulik Shah <quic_mkshah@quicinc.com> > >
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c index a021dc71807b..5371d7e3090a 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 @@ -91,6 +92,15 @@ enum { #define CMD_STATUS_ISSUED BIT(8) #define CMD_STATUS_COMPL BIT(16) +#define ACCL_TYPE(addr) ((addr >> 16) & 0xF) +#define VREG_ADDR(addr) (addr & ~0xF) + +enum { + HW_ACCL_CLK = 0x3, + HW_ACCL_VREG, + HW_ACCL_BUS, +}; + /* * Here's a high level overview of how all the registers in RPMH work * together: @@ -557,7 +567,15 @@ 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) + /* + * Each RPMh VREG accelerator resource has 3 or 4 contiguous 4-byte + * aligned addresses associated with it. Ignore the offset to check + * for in-flight VREG requests. + */ + if (HW_ACCL_VREG == ACCL_TYPE(msg->cmds[k].addr) && + VREG_ADDR(addr) == VREG_ADDR(msg->cmds[k].addr)) + return -EBUSY; + else if (addr == msg->cmds[k].addr) return -EBUSY; } }