Re: [tpmdd-devel] [PATCH 2/3] tpm_crb: encapsulate crb_wait_for_reg_32
On Tue, Oct 11, 2016 at 10:21:03AM +, Winkler, Tomas wrote: > > Encapsulated crb_wait_for_reg32() so that state changes in other CRB > > registers > > than TPM_CRB_CTRL_REQ_x can be waited. > > > > Signed-off-by: Jarkko Sakkinen > > --- > > drivers/char/tpm/tpm_crb.c | 40 +++- > > 1 file changed, 23 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index > > c34318b..45f53c2 100644 > > --- a/drivers/char/tpm/tpm_crb.c > > +++ b/drivers/char/tpm/tpm_crb.c > > @@ -121,6 +121,25 @@ static int __maybe_unused crb_go_idle(struct device > > *dev, struct crb_priv *priv) > > return 0; > > } > > > > +static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value, > > + unsigned long timeout) > This is a boiler plate register polling function I would call it _poll_ > rather the _wait_ > > > +{ > > + ktime_t start; > > + ktime_t stop; > > + > > + start = ktime_get(); > > + stop = ktime_add(start, ms_to_ktime(timeout)); > > + > > + do { > > + if ((ioread32(reg) & mask) == value) > I prefer the register value is synced to variable, this inlining is > harder to add adhoc debug prints. Also you removed the debug print > out that I know when this settled which is important for catching > bugs. I can add it but can you just briefly explain why the warning is not enough? > > + return true; > > + > > + usleep_range(50, 100); > How do you know this is correct sleep time, I've tuned that for power > gating I'm not sure you this fits also for locality. Does it matter as long as it is less than for the timeout? /Jarkko -- Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot ___ tpmdd-devel mailing list tpmdd-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
Re: [tpmdd-devel] [PATCH 2/3] tpm_crb: encapsulate crb_wait_for_reg_32
> Encapsulated crb_wait_for_reg32() so that state changes in other CRB registers > than TPM_CRB_CTRL_REQ_x can be waited. > > Signed-off-by: Jarkko Sakkinen > --- > drivers/char/tpm/tpm_crb.c | 40 +++- > 1 file changed, 23 insertions(+), 17 deletions(-) > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index > c34318b..45f53c2 100644 > --- a/drivers/char/tpm/tpm_crb.c > +++ b/drivers/char/tpm/tpm_crb.c > @@ -121,6 +121,25 @@ static int __maybe_unused crb_go_idle(struct device > *dev, struct crb_priv *priv) > return 0; > } > > +static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value, > + unsigned long timeout) This is a boiler plate register polling function I would call it _poll_ rather the _wait_ > +{ > + ktime_t start; > + ktime_t stop; > + > + start = ktime_get(); > + stop = ktime_add(start, ms_to_ktime(timeout)); > + > + do { > + if ((ioread32(reg) & mask) == value) I prefer the register value is synced to variable, this inlining is harder to add adhoc debug prints. Also you removed the debug print out that I know when this settled which is important for catching bugs. > + return true; > + > + usleep_range(50, 100); How do you know this is correct sleep time, I've tuned that for power gating I'm not sure you this fits also for locality. > + } while (ktime_before(ktime_get(), stop)); > + > + return false; > +} > + > /** > * crb_cmd_ready - request tpm crb device to enter ready state > * > @@ -138,27 +157,14 @@ static int __maybe_unused crb_go_idle(struct device > *dev, struct crb_priv *priv) static int __maybe_unused crb_cmd_ready(struct > device *dev, > struct crb_priv *priv) > { > - ktime_t stop, start; > - u32 req; > - > if (priv->flags & CRB_FL_ACPI_START) > return 0; > > iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->regs_t->ctrl_req); > - > - start = ktime_get(); > - stop = ktime_add(start, ms_to_ktime(TPM2_TIMEOUT_C)); > - do { > - req = ioread32(&priv->regs_t->ctrl_req); > - if (!(req & CRB_CTRL_REQ_CMD_READY)) { > - dev_dbg(dev, "cmdReady in %lld usecs\n", > - ktime_to_us(ktime_sub(ktime_get(), start))); > - return 0; > - } > - usleep_range(50, 100); > - } while (ktime_before(ktime_get(), stop)); > - > - if (ioread32(&priv->regs_t->ctrl_req) & CRB_CTRL_REQ_CMD_READY) { > + if (!crb_wait_for_reg_32(&priv->regs_t->ctrl_req, > + CRB_CTRL_REQ_CMD_READY /* mask */, > + 0, /* value */ > + TPM2_TIMEOUT_C)) { > dev_warn(dev, "cmdReady timed out\n"); > return -ETIME; > } > -- > 2.7.4 > > > -- > Check out the vibrant tech community on one of the world's most > engaging tech sites, SlashDot.org! http://sdm.link/slashdot > ___ > tpmdd-devel mailing list > tpmdd-devel@lists.sourceforge.net > https://lists.sourceforge.net/lists/listinfo/tpmdd-devel -- Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot ___ tpmdd-devel mailing list tpmdd-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
[tpmdd-devel] [PATCH 2/3] tpm_crb: encapsulate crb_wait_for_reg_32
Encapsulated crb_wait_for_reg32() so that state changes in other CRB registers than TPM_CRB_CTRL_REQ_x can be waited. Signed-off-by: Jarkko Sakkinen --- drivers/char/tpm/tpm_crb.c | 40 +++- 1 file changed, 23 insertions(+), 17 deletions(-) diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index c34318b..45f53c2 100644 --- a/drivers/char/tpm/tpm_crb.c +++ b/drivers/char/tpm/tpm_crb.c @@ -121,6 +121,25 @@ static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv *priv) return 0; } +static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value, + unsigned long timeout) +{ + ktime_t start; + ktime_t stop; + + start = ktime_get(); + stop = ktime_add(start, ms_to_ktime(timeout)); + + do { + if ((ioread32(reg) & mask) == value) + return true; + + usleep_range(50, 100); + } while (ktime_before(ktime_get(), stop)); + + return false; +} + /** * crb_cmd_ready - request tpm crb device to enter ready state * @@ -138,27 +157,14 @@ static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv *priv) static int __maybe_unused crb_cmd_ready(struct device *dev, struct crb_priv *priv) { - ktime_t stop, start; - u32 req; - if (priv->flags & CRB_FL_ACPI_START) return 0; iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->regs_t->ctrl_req); - - start = ktime_get(); - stop = ktime_add(start, ms_to_ktime(TPM2_TIMEOUT_C)); - do { - req = ioread32(&priv->regs_t->ctrl_req); - if (!(req & CRB_CTRL_REQ_CMD_READY)) { - dev_dbg(dev, "cmdReady in %lld usecs\n", - ktime_to_us(ktime_sub(ktime_get(), start))); - return 0; - } - usleep_range(50, 100); - } while (ktime_before(ktime_get(), stop)); - - if (ioread32(&priv->regs_t->ctrl_req) & CRB_CTRL_REQ_CMD_READY) { + if (!crb_wait_for_reg_32(&priv->regs_t->ctrl_req, +CRB_CTRL_REQ_CMD_READY /* mask */, +0, /* value */ +TPM2_TIMEOUT_C)) { dev_warn(dev, "cmdReady timed out\n"); return -ETIME; } -- 2.7.4 -- Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot ___ tpmdd-devel mailing list tpmdd-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/tpmdd-devel