| Message ID | 20200311161104.RFT.v2.6.Icf2213131ea652087f100129359052c83601f8b0@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drivers: qcom: rpmh-rsc: Cleanup / add lots of comments | expand |
Hi, On 3/12/2020 4:43 AM, Douglas Anderson wrote: > tcs_is_free() had two checks in it: does the software think that the > TCS is free and does the hardware think that the TCS is free. Let's > comment this and also add a warning in the case that software and > hardware disagree, at least for ACTIVE_ONLY TCS. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > > Changes in v2: > - Comment tcs_is_free() new for v2; replaces old patch 6. > > drivers/soc/qcom/rpmh-rsc.c | 23 +++++++++++++++++++++-- > 1 file changed, 21 insertions(+), 2 deletions(-) > > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c > index 9d2669cbd994..93f5d1fb71ca 100644 > --- a/drivers/soc/qcom/rpmh-rsc.c > +++ b/drivers/soc/qcom/rpmh-rsc.c > @@ -181,8 +181,27 @@ 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 software thinks it's in use then it's definitely in use */ > + if (test_bit(tcs_id, drv->tcs_in_use)) > + return false; > + > + /* If hardware agrees it's free then it's definitely free */ > + if (read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id) != 0) > + return true; > + > + /* > + * If we're here then software and hardware disagree about whether > + * the TCS is free. Software thinks it is free and hardware thinks > + * it is not. > + * > + * Maybe this should be a warning in all cases, but it's almost > + * certainly a warning for the ACTIVE_TCS where nobody else should > + * be doing anything else behind our backs. For now we'll just > + * warn there and then still return that we're in use. > + */ > + WARN(drv->tcs[tcs_id].type == ACTIVE_TCS, > + "Driver thought TCS was free but HW reported busy\n"); This warning can come for borrowed WAKE_TCS as well. > + return false; > } We have a patch on downstream variant to optimize this by only checking tcs_in_use flag (SW check) and HW check is removed. 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); + return !test_bit(tcs_id, drv->tcs_in_use); } With this we are good and don't require to put above warning as well. if you want me to upload, i can post it and then you can drop this change from your series. Or if you want to modify it as above and keep in this series i am ok. Thanks, Maulik > > /**
Hi, On Wed, Apr 1, 2020 at 4:39 AM Maulik Shah <mkshah@codeaurora.org> wrote: > > Hi, > > On 3/12/2020 4:43 AM, Douglas Anderson wrote: > > tcs_is_free() had two checks in it: does the software think that the > > TCS is free and does the hardware think that the TCS is free. Let's > > comment this and also add a warning in the case that software and > > hardware disagree, at least for ACTIVE_ONLY TCS. > > > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > > --- > > > > Changes in v2: > > - Comment tcs_is_free() new for v2; replaces old patch 6. > > > > drivers/soc/qcom/rpmh-rsc.c | 23 +++++++++++++++++++++-- > > 1 file changed, 21 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c > > index 9d2669cbd994..93f5d1fb71ca 100644 > > --- a/drivers/soc/qcom/rpmh-rsc.c > > +++ b/drivers/soc/qcom/rpmh-rsc.c > > @@ -181,8 +181,27 @@ 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 software thinks it's in use then it's definitely in use */ > > + if (test_bit(tcs_id, drv->tcs_in_use)) > > + return false; > > + > > + /* If hardware agrees it's free then it's definitely free */ > > + if (read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id) != 0) > > + return true; > > + > > + /* > > + * If we're here then software and hardware disagree about whether > > + * the TCS is free. Software thinks it is free and hardware thinks > > + * it is not. > > + * > > + * Maybe this should be a warning in all cases, but it's almost > > + * certainly a warning for the ACTIVE_TCS where nobody else should > > + * be doing anything else behind our backs. For now we'll just > > + * warn there and then still return that we're in use. > > + */ > > + WARN(drv->tcs[tcs_id].type == ACTIVE_TCS, > > + "Driver thought TCS was free but HW reported busy\n"); > This warning can come for borrowed WAKE_TCS as well. > > + return false; > > } > > We have a patch on downstream variant to optimize this by only checking > tcs_in_use flag (SW check) and HW check is removed. > > 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); > + return !test_bit(tcs_id, drv->tcs_in_use); > } > > With this we are good and don't require to put above warning as well. > > if you want me to upload, i can post it and then you can drop this > change from your series. > > Or if you want to modify it as above and keep in this series i am ok. Probably easiest for me to replace this patch in the series with one that removes the read from RSC_DRV_STATUS. Then it will all be clearer.
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c index 9d2669cbd994..93f5d1fb71ca 100644 --- a/drivers/soc/qcom/rpmh-rsc.c +++ b/drivers/soc/qcom/rpmh-rsc.c @@ -181,8 +181,27 @@ 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 software thinks it's in use then it's definitely in use */ + if (test_bit(tcs_id, drv->tcs_in_use)) + return false; + + /* If hardware agrees it's free then it's definitely free */ + if (read_tcs_reg(drv, RSC_DRV_STATUS, tcs_id) != 0) + return true; + + /* + * If we're here then software and hardware disagree about whether + * the TCS is free. Software thinks it is free and hardware thinks + * it is not. + * + * Maybe this should be a warning in all cases, but it's almost + * certainly a warning for the ACTIVE_TCS where nobody else should + * be doing anything else behind our backs. For now we'll just + * warn there and then still return that we're in use. + */ + WARN(drv->tcs[tcs_id].type == ACTIVE_TCS, + "Driver thought TCS was free but HW reported busy\n"); + return false; } /**
tcs_is_free() had two checks in it: does the software think that the TCS is free and does the hardware think that the TCS is free. Let's comment this and also add a warning in the case that software and hardware disagree, at least for ACTIVE_ONLY TCS. Signed-off-by: Douglas Anderson <dianders@chromium.org> --- Changes in v2: - Comment tcs_is_free() new for v2; replaces old patch 6. drivers/soc/qcom/rpmh-rsc.c | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-)