| Message ID | 20200414131010.v2.2.I8550512081c89ec7a545018a7d2d9418a27c1a7a@changeid (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2,1/2] soc: qcom: rpmh-rsc: Factor "tcs_reg_addr" and "tcs_cmd_addr" calculation | expand |
Quoting Douglas Anderson (2020-04-14 13:10:16) > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c > index f988e9cc2c30..02fc114ffb4f 100644 > --- a/drivers/soc/qcom/rpmh-rsc.c > +++ b/drivers/soc/qcom/rpmh-rsc.c > @@ -174,12 +175,13 @@ static void write_tcs_reg(const struct rsc_drv *drv, int reg, int tcs_id, > static void write_tcs_reg_sync(const struct rsc_drv *drv, int reg, int tcs_id, > u32 data) > { > + u32 new_data; > + > writel(data, tcs_reg_addr(drv, reg, tcs_id)); > - for (;;) { > - if (data == readl(tcs_reg_addr(drv, reg, tcs_id))) > - break; > - udelay(1); > - } > + if (readl_poll_timeout_atomic(tcs_reg_addr(drv, reg, tcs_id), new_data, > + new_data == data, 1, USEC_PER_SEC)) > + pr_err("%s: error writing %#x to %d:%d\n", drv->name, Shouldn't the register be hex? That seems to be how the registers are represented. But I guess tcs_id is decimal and can't be translated to be meaningful enough to indicate which TCS it is like the sleep or wake one. > + data, tcs_id, reg); > }
Hi, On Tue, Apr 14, 2020 at 1:30 PM Stephen Boyd <swboyd@chromium.org> wrote: > > Quoting Douglas Anderson (2020-04-14 13:10:16) > > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c > > index f988e9cc2c30..02fc114ffb4f 100644 > > --- a/drivers/soc/qcom/rpmh-rsc.c > > +++ b/drivers/soc/qcom/rpmh-rsc.c > > @@ -174,12 +175,13 @@ static void write_tcs_reg(const struct rsc_drv *drv, int reg, int tcs_id, > > static void write_tcs_reg_sync(const struct rsc_drv *drv, int reg, int tcs_id, > > u32 data) > > { > > + u32 new_data; > > + > > writel(data, tcs_reg_addr(drv, reg, tcs_id)); > > - for (;;) { > > - if (data == readl(tcs_reg_addr(drv, reg, tcs_id))) > > - break; > > - udelay(1); > > - } > > + if (readl_poll_timeout_atomic(tcs_reg_addr(drv, reg, tcs_id), new_data, > > + new_data == data, 1, USEC_PER_SEC)) > > + pr_err("%s: error writing %#x to %d:%d\n", drv->name, > > Shouldn't the register be hex? That seems to be how the registers are > represented. But I guess tcs_id is decimal and can't be translated to be > meaningful enough to indicate which TCS it is like the sleep or wake > one. Good point. Should I quickly spin a v3 just so this is all ready to go, or wait to see if there is any additional feedback? -Doug
Quoting Doug Anderson (2020-04-14 13:39:15) > Hi, > > On Tue, Apr 14, 2020 at 1:30 PM Stephen Boyd <swboyd@chromium.org> wrote: > > > > Quoting Douglas Anderson (2020-04-14 13:10:16) > > > diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c > > > index f988e9cc2c30..02fc114ffb4f 100644 > > > --- a/drivers/soc/qcom/rpmh-rsc.c > > > +++ b/drivers/soc/qcom/rpmh-rsc.c > > > @@ -174,12 +175,13 @@ static void write_tcs_reg(const struct rsc_drv *drv, int reg, int tcs_id, > > > static void write_tcs_reg_sync(const struct rsc_drv *drv, int reg, int tcs_id, > > > u32 data) > > > { > > > + u32 new_data; > > > + > > > writel(data, tcs_reg_addr(drv, reg, tcs_id)); > > > - for (;;) { > > > - if (data == readl(tcs_reg_addr(drv, reg, tcs_id))) > > > - break; > > > - udelay(1); > > > - } > > > + if (readl_poll_timeout_atomic(tcs_reg_addr(drv, reg, tcs_id), new_data, > > > + new_data == data, 1, USEC_PER_SEC)) > > > + pr_err("%s: error writing %#x to %d:%d\n", drv->name, > > > > Shouldn't the register be hex? That seems to be how the registers are > > represented. But I guess tcs_id is decimal and can't be translated to be > > meaningful enough to indicate which TCS it is like the sleep or wake > > one. > > Good point. Should I quickly spin a v3 just so this is all ready to > go, or wait to see if there is any additional feedback? > That's my only complaint, so if maintainers fix it then you can have my RB tag. Reviewed-by: Stephen Boyd <swboyd@chromium.org>
diff --git a/drivers/soc/qcom/rpmh-rsc.c b/drivers/soc/qcom/rpmh-rsc.c index f988e9cc2c30..02fc114ffb4f 100644 --- a/drivers/soc/qcom/rpmh-rsc.c +++ b/drivers/soc/qcom/rpmh-rsc.c @@ -10,6 +10,7 @@ #include <linux/delay.h> #include <linux/interrupt.h> #include <linux/io.h> +#include <linux/iopoll.h> #include <linux/kernel.h> #include <linux/list.h> #include <linux/of.h> @@ -174,12 +175,13 @@ static void write_tcs_reg(const struct rsc_drv *drv, int reg, int tcs_id, static void write_tcs_reg_sync(const struct rsc_drv *drv, int reg, int tcs_id, u32 data) { + u32 new_data; + writel(data, tcs_reg_addr(drv, reg, tcs_id)); - for (;;) { - if (data == readl(tcs_reg_addr(drv, reg, tcs_id))) - break; - udelay(1); - } + if (readl_poll_timeout_atomic(tcs_reg_addr(drv, reg, tcs_id), new_data, + new_data == data, 1, USEC_PER_SEC)) + pr_err("%s: error writing %#x to %d:%d\n", drv->name, + data, tcs_id, reg); } /**
If our data still isn't there after 1 second, shout and give up. Reported-by: Joe Perches <joe@perches.com> Signed-off-by: Douglas Anderson <dianders@chromium.org> --- Changes in v2: - Patch ("Timeout after 1 second") new for v2. drivers/soc/qcom/rpmh-rsc.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)