| Message ID | 1475972112-2819-4-git-send-email-jarkko.sakkinen@linux.intel.com |
|---|---|
| State | New |
| Headers | show |
> > Request and relinquish locality for the driver use in order to be a better citizen > in a multi locality environment like with TXT as it uses locality 2. > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > --- > drivers/char/tpm/tpm_crb.c | 36 ++++++++++++++++++++++++------------ > 1 file changed, 24 insertions(+), 12 deletions(-) > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index > ffd3a6c..9e07cf3 100644 > --- a/drivers/char/tpm/tpm_crb.c > +++ b/drivers/char/tpm/tpm_crb.c > @@ -34,6 +34,15 @@ enum crb_defaults { > CRB_ACPI_START_INDEX = 1, > }; > > +enum crb_loc_ctrl { > + CRB_LOC_CTRL_REQUEST_ACCESS = BIT(0), > + CRB_LOC_CTRL_RELINQUISH = BIT(1), > +}; > + > +enum crb_loc_state { > + CRB_LOC_STATE_LOC_ASSIGNED = BIT(1), > +}; > + > enum crb_ctrl_req { > CRB_CTRL_REQ_CMD_READY = BIT(0), > CRB_CTRL_REQ_GO_IDLE = BIT(1), > @@ -98,12 +107,8 @@ struct crb_priv { > * @dev: crb device > * @priv: crb private data > * > - * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ > - * The device should respond within TIMEOUT_C by clearing the bit. > - * Anyhow, we do not wait here as a consequent CMD_READY request > - * will be handled correctly even if idle was not completed. > - * > - * The function does nothing for devices with ACPI-start method. > + * Put device to the idle state and relinquish locality. The function > + does > + * nothing for devices with the ACPI-start method. > * > * Return: 0 always > */ > @@ -112,6 +117,7 @@ static int __maybe_unused crb_go_idle(struct device > *dev, struct crb_priv *priv) > if (priv->flags & CRB_FL_ACPI_START) > return 0; > > + iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs->loc_ctrl); Please don't mix different functionalities in one function Also those functions are called from runtime pm, this has nothing to do with the power management > iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs->ctrl_req); > /* we don't really care when this settles */ > > @@ -143,11 +149,8 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg, > u32 mask, u32 value, > * @dev: crb device > * @priv: crb private data > * > - * Write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ > - * and poll till the device acknowledge it by clearing the bit. > - * The device should respond within TIMEOUT_C. > - * > - * The function does nothing for devices with ACPI-start method > + * Try to wake up the device and request locality. The function does > + nothing > + * for devices with the ACPI-start method. > * > * Return: 0 on success -ETIME on timeout; > */ > @@ -162,7 +165,16 @@ static int __maybe_unused crb_cmd_ready(struct > device *dev, > CRB_CTRL_REQ_CMD_READY /* mask */, > 0, /* value */ > TPM2_TIMEOUT_C)) { > - dev_warn(dev, "cmdReady timed out\n"); > + dev_warn(dev, "TPM_CRB_CTRL_REQ_x.cmdReady timed > out\n"); > + return -ETIME; > + } > + > + iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs->loc_ctrl); > + if (!crb_wait_for_reg_32(&priv->regs->loc_state, > + CRB_LOC_STATE_LOC_ASSIGNED, /* mask */ > + CRB_LOC_STATE_LOC_ASSIGNED, /* value */ > + TPM2_TIMEOUT_C)) { > + dev_warn(dev, "TPM_LOC_STATE_x.requestAccess timed > out\n"); Same here > return -ETIME; > } > > -- ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Sun, Oct 09, 2016 at 06:35:28AM +0000, Winkler, Tomas wrote: > > > > > > Request and relinquish locality for the driver use in order to be a better citizen > > in a multi locality environment like with TXT as it uses locality 2. > > > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > --- > > drivers/char/tpm/tpm_crb.c | 36 ++++++++++++++++++++++++------------ > > 1 file changed, 24 insertions(+), 12 deletions(-) > > > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index > > ffd3a6c..9e07cf3 100644 > > --- a/drivers/char/tpm/tpm_crb.c > > +++ b/drivers/char/tpm/tpm_crb.c > > @@ -34,6 +34,15 @@ enum crb_defaults { > > CRB_ACPI_START_INDEX = 1, > > }; > > > > +enum crb_loc_ctrl { > > + CRB_LOC_CTRL_REQUEST_ACCESS = BIT(0), > > + CRB_LOC_CTRL_RELINQUISH = BIT(1), > > +}; > > + > > +enum crb_loc_state { > > + CRB_LOC_STATE_LOC_ASSIGNED = BIT(1), > > +}; > > + > > enum crb_ctrl_req { > > CRB_CTRL_REQ_CMD_READY = BIT(0), > > CRB_CTRL_REQ_GO_IDLE = BIT(1), > > @@ -98,12 +107,8 @@ struct crb_priv { > > * @dev: crb device > > * @priv: crb private data > > * > > - * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ > > - * The device should respond within TIMEOUT_C by clearing the bit. > > - * Anyhow, we do not wait here as a consequent CMD_READY request > > - * will be handled correctly even if idle was not completed. > > - * > > - * The function does nothing for devices with ACPI-start method. > > + * Put device to the idle state and relinquish locality. The function > > + does > > + * nothing for devices with the ACPI-start method. > > * > > * Return: 0 always > > */ > > @@ -112,6 +117,7 @@ static int __maybe_unused crb_go_idle(struct device > > *dev, struct crb_priv *priv) > > if (priv->flags & CRB_FL_ACPI_START) > > return 0; > > > > + iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs->loc_ctrl); > > > Please don't mix different functionalities in one function ?? > Also those functions are called from runtime pm, this has nothing to > do with the power management It all depends on granularity. If you want to make an argument, could you propose a better granularity? Do you think it'd be better to do it for each transmission? You are saying that this is all bad without saying really backing up your statements by any means. /Jarkko ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
> > > > > > > > Request and relinquish locality for the driver use in order to be a better > citizen > > > in a multi locality environment like with TXT as it uses locality 2. > > > > > > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > --- > > > drivers/char/tpm/tpm_crb.c | 36 > > > ++++++++++++++++++++++++------------ > > > 1 file changed, 24 insertions(+), 12 deletions(-) > > > > > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c > > > index > > > ffd3a6c..9e07cf3 100644 > > > --- a/drivers/char/tpm/tpm_crb.c > > > +++ b/drivers/char/tpm/tpm_crb.c > > > @@ -34,6 +34,15 @@ enum crb_defaults { > > > CRB_ACPI_START_INDEX = 1, > > > }; > > > > > > +enum crb_loc_ctrl { > > > + CRB_LOC_CTRL_REQUEST_ACCESS = BIT(0), > > > + CRB_LOC_CTRL_RELINQUISH = BIT(1), > > > +}; > > > + > > > +enum crb_loc_state { > > > + CRB_LOC_STATE_LOC_ASSIGNED = BIT(1), > > > +}; > > > + > > > enum crb_ctrl_req { > > > CRB_CTRL_REQ_CMD_READY = BIT(0), > > > CRB_CTRL_REQ_GO_IDLE = BIT(1), > > > @@ -98,12 +107,8 @@ struct crb_priv { > > > * @dev: crb device > > > * @priv: crb private data > > > * > > > - * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ > > > - * The device should respond within TIMEOUT_C by clearing the bit. > > > - * Anyhow, we do not wait here as a consequent CMD_READY request > > > - * will be handled correctly even if idle was not completed. > > > - * > > > - * The function does nothing for devices with ACPI-start method. > > > + * Put device to the idle state and relinquish locality. The > > > + function does > > > + * nothing for devices with the ACPI-start method. > > > * > > > * Return: 0 always > > > */ > > > @@ -112,6 +117,7 @@ static int __maybe_unused crb_go_idle(struct > > > device *dev, struct crb_priv *priv) > > > if (priv->flags & CRB_FL_ACPI_START) > > > return 0; > > > > > > + iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs->loc_ctrl); > > > > > > Please don't mix different functionalities in one function > > ?? > > > Also those functions are called from runtime pm, this has nothing to > > do with the power management > > It all depends on granularity. If you want to make an argument, could you > propose a better granularity? Do you think it'd be better to do it for each > transmission? Not sure, I don't believe we closed the design here with all the parties, you are jumping ahead. > You are saying that this is all bad without saying really backing up your > statements by any means. You are right, I assumed it's pretty obvious, I'm taking this to my attention not do it again. So my point is if you have a function which is named go_idle it probably does go idle flow and no other things like relinquish functionality that's for code readability and style. Also you lost degree of freedom even now you may want perform each of these operation for each tpm request, that might not be true in general, we would like to witch to runtime auto suspend it won't work anymore, for example. Also as you pointed right now we are not clear on the granularity of the locality access. Thanks Tomas ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
On Sun, Oct 09, 2016 at 09:43:59AM +0000, Winkler, Tomas wrote: > > > > > > > > > > > > Request and relinquish locality for the driver use in order to be a better > > citizen > > > > in a multi locality environment like with TXT as it uses locality 2. > > > > > > > > > > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > > > --- > > > > drivers/char/tpm/tpm_crb.c | 36 > > > > ++++++++++++++++++++++++------------ > > > > 1 file changed, 24 insertions(+), 12 deletions(-) > > > > > > > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c > > > > index > > > > ffd3a6c..9e07cf3 100644 > > > > --- a/drivers/char/tpm/tpm_crb.c > > > > +++ b/drivers/char/tpm/tpm_crb.c > > > > @@ -34,6 +34,15 @@ enum crb_defaults { > > > > CRB_ACPI_START_INDEX = 1, > > > > }; > > > > > > > > +enum crb_loc_ctrl { > > > > + CRB_LOC_CTRL_REQUEST_ACCESS = BIT(0), > > > > + CRB_LOC_CTRL_RELINQUISH = BIT(1), > > > > +}; > > > > + > > > > +enum crb_loc_state { > > > > + CRB_LOC_STATE_LOC_ASSIGNED = BIT(1), > > > > +}; > > > > + > > > > enum crb_ctrl_req { > > > > CRB_CTRL_REQ_CMD_READY = BIT(0), > > > > CRB_CTRL_REQ_GO_IDLE = BIT(1), > > > > @@ -98,12 +107,8 @@ struct crb_priv { > > > > * @dev: crb device > > > > * @priv: crb private data > > > > * > > > > - * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ > > > > - * The device should respond within TIMEOUT_C by clearing the bit. > > > > - * Anyhow, we do not wait here as a consequent CMD_READY request > > > > - * will be handled correctly even if idle was not completed. > > > > - * > > > > - * The function does nothing for devices with ACPI-start method. > > > > + * Put device to the idle state and relinquish locality. The > > > > + function does > > > > + * nothing for devices with the ACPI-start method. > > > > * > > > > * Return: 0 always > > > > */ > > > > @@ -112,6 +117,7 @@ static int __maybe_unused crb_go_idle(struct > > > > device *dev, struct crb_priv *priv) > > > > if (priv->flags & CRB_FL_ACPI_START) > > > > return 0; > > > > > > > > + iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs->loc_ctrl); > > > > > > > > > Please don't mix different functionalities in one function > > > > ?? > > > > > Also those functions are called from runtime pm, this has nothing to > > > do with the power management > > > > It all depends on granularity. If you want to make an argument, could you > > propose a better granularity? Do you think it'd be better to do it for each > > transmission? > > Not sure, I don't believe we closed the design here with all the > parties, you are jumping ahead. I also added RFC tag. I have had this done for a while but it has had a dependency for runtime PM so I decided to make it available. > > You are saying that this is all bad without saying really backing up your > > statements by any means. > > You are right, I assumed it's pretty obvious, I'm taking this to my > attention not do it again. > > So my point is if you have a function which is named go_idle it > probably does go idle flow and no other things like relinquish > functionality that's for code readability and style. Also you lost When you go to idle you certain tasks before going to sleep. If the granularity matches, relinquishing locality is one of those tasks. > degree of freedom even now you may want perform each of these > operation for each tpm request, that might not be true in general, we > would like to witch to runtime auto suspend it won't work anymore, for > example. Also as you pointed right now we are not clear on the > granularity of the locality access. That's not true. You don't loose any freedoms. You can start with something simple like once per transmission or once per idle/ready. If that does not scale, then it must be adjusted. I think this is really business as usual... > Thanks > Tomas /Jarkko ------------------------------------------------------------------------------ Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c index ffd3a6c..9e07cf3 100644 --- a/drivers/char/tpm/tpm_crb.c +++ b/drivers/char/tpm/tpm_crb.c @@ -34,6 +34,15 @@ enum crb_defaults { CRB_ACPI_START_INDEX = 1, }; +enum crb_loc_ctrl { + CRB_LOC_CTRL_REQUEST_ACCESS = BIT(0), + CRB_LOC_CTRL_RELINQUISH = BIT(1), +}; + +enum crb_loc_state { + CRB_LOC_STATE_LOC_ASSIGNED = BIT(1), +}; + enum crb_ctrl_req { CRB_CTRL_REQ_CMD_READY = BIT(0), CRB_CTRL_REQ_GO_IDLE = BIT(1), @@ -98,12 +107,8 @@ struct crb_priv { * @dev: crb device * @priv: crb private data * - * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ - * The device should respond within TIMEOUT_C by clearing the bit. - * Anyhow, we do not wait here as a consequent CMD_READY request - * will be handled correctly even if idle was not completed. - * - * The function does nothing for devices with ACPI-start method. + * Put device to the idle state and relinquish locality. The function does + * nothing for devices with the ACPI-start method. * * Return: 0 always */ @@ -112,6 +117,7 @@ static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv *priv) if (priv->flags & CRB_FL_ACPI_START) return 0; + iowrite32(CRB_LOC_CTRL_RELINQUISH, &priv->regs->loc_ctrl); iowrite32(CRB_CTRL_REQ_GO_IDLE, &priv->regs->ctrl_req); /* we don't really care when this settles */ @@ -143,11 +149,8 @@ static bool crb_wait_for_reg_32(u32 __iomem *reg, u32 mask, u32 value, * @dev: crb device * @priv: crb private data * - * Write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ - * and poll till the device acknowledge it by clearing the bit. - * The device should respond within TIMEOUT_C. - * - * The function does nothing for devices with ACPI-start method + * Try to wake up the device and request locality. The function does nothing + * for devices with the ACPI-start method. * * Return: 0 on success -ETIME on timeout; */ @@ -162,7 +165,16 @@ static int __maybe_unused crb_cmd_ready(struct device *dev, CRB_CTRL_REQ_CMD_READY /* mask */, 0, /* value */ TPM2_TIMEOUT_C)) { - dev_warn(dev, "cmdReady timed out\n"); + dev_warn(dev, "TPM_CRB_CTRL_REQ_x.cmdReady timed out\n"); + return -ETIME; + } + + iowrite32(CRB_LOC_CTRL_REQUEST_ACCESS, &priv->regs->loc_ctrl); + if (!crb_wait_for_reg_32(&priv->regs->loc_state, + CRB_LOC_STATE_LOC_ASSIGNED, /* mask */ + CRB_LOC_STATE_LOC_ASSIGNED, /* value */ + TPM2_TIMEOUT_C)) { + dev_warn(dev, "TPM_LOC_STATE_x.requestAccess timed out\n"); return -ETIME; }
Request and relinquish locality for the driver use in order to be a better citizen in a multi locality environment like with TXT as it uses locality 2. Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- drivers/char/tpm/tpm_crb.c | 36 ++++++++++++++++++++++++------------ 1 file changed, 24 insertions(+), 12 deletions(-)