[PATCH 0/4 V2] tpm/tpm_crb: implement power management.

[PATCH 0/4 V2] tpm/tpm_crb: implement power management.

[Tomas Winkler]

Te overall platform ability to enter a low power state is also
conditioned on the ability of a tpm device to go to idle state.
This series should provide this feature.

Unfortunately, there is a HW bug on Intel PTT devices on Skylake,
Kabylake, and Broxton devices, where certain registers lost retention
during TPM idle state. Hence this implementation takes this into
consideration and implement the feature based only on access to
registers that retain their state. This still conforms to the spec
and should be correct also on non Intle devices.

V2: Utilize runtime_pm for driving tpm crb idle states.


Tomas Winkler (4):
  tpm/tpm_crb: implement tpm crb idle state
  tmp/tpm_crb: fix Intel PTT hw bug during idle state
  tpm/tpm_crb: open code the crb_init into acpi_add
  tmp/tpm_crb: implment runtime pm for tpm_crb

 drivers/char/tpm/tpm-interface.c |   5 ++
 drivers/char/tpm/tpm_crb.c       | 166 +++++++++++++++++++++++++++++++++------
 2 files changed, 147 insertions(+), 24 deletions(-)

-- 
2.7.4


------------------------------------------------------------------------------

[PATCH v2 1/4] tpm/tpm_crb: implement tpm crb idle state

[Tomas Winkler]

The register TPM_CRB_CTRL_REQ_x contains bits goIdle and cmdReady for
SW to indicate that the device can enter or should exit the idle state.

The legacy ACPI-start (SMI + DMA) based devices do not support these
bits and the idle state management is not exposed to the host SW.
Thus, this functionality only is enabled only for a CRB start (MMIO)
based devices.

Based on Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
oringal patch:
'tpm_crb: implement power tpm crb power management'

Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
V2: do not export the functions via tpm ops

 drivers/char/tpm/tpm_crb.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 6e9d1bca712f..49023ac3dea1 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -83,6 +83,68 @@ struct crb_priv {
 	u32 cmd_size;
 };
 
+/**
+ * crb_go_idle - 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.
+ *
+ * @dev:  crb device
+ * @priv: crb private data
+ * Return:  0 always
+ */
+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_CTRL_REQ_GO_IDLE, &priv->cca->req);
+	/* we don't really care when this settles */
+
+	return 0;
+}
+
+/**
+ * crb_cmd_ready - 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
+ *
+ * @dev:  crb device
+ * @priv: crb private data
+ *
+ * Return:  0 on success -ETIME on timeout;
+ */
+static int __maybe_unused crb_cmd_ready(struct device *dev,
+					struct crb_priv *priv)
+{
+	ktime_t stop, start;
+
+	if (priv->flags & CRB_FL_ACPI_START)
+		return 0;
+
+	iowrite32(CRB_CTRL_REQ_CMD_READY, &priv->cca->req);
+
+	start = ktime_get();
+	stop = ktime_add(start, ms_to_ktime(TPM2_TIMEOUT_C));
+	do {
+		if (!(ioread32(&priv->cca->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->cca->req) & CRB_CTRL_REQ_CMD_READY) {
+		dev_warn(dev, "cmdReady timed out\n");
+		return -ETIME;
+	}
+
+	return 0;
+}
+
 static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
 
 static u8 crb_status(struct tpm_chip *chip)
-- 
2.7.4


------------------------------------------------------------------------------

[PATCH v2 2/4] tmp/tpm_crb: fix Intel PTT hw bug during idle state

[Tomas Winkler]

There is a HW bug in Skylake, and Broxton PCH Intel PTT device, where
most of the registers in the control area except START, REQUEST, CANCEL,
and LOC_CTRL lost retention when the device is in the idle state. Hence
we need to bring the device to ready state before accessing the other
registers. The fix brings device to ready state before trying to read
command and response buffer addresses in order to remap the for access.

Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
V2: cmd read need to be called also before crb_init as this will run
 self test.

 drivers/char/tpm/tpm_crb.c | 47 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 39 insertions(+), 8 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 49023ac3dea1..69e305f49310 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -311,6 +311,7 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	struct list_head resources;
 	struct resource io_res;
 	struct device *dev = &device->dev;
+	u32 pa_high, pa_low;
 	u64 cmd_pa;
 	u32 cmd_size;
 	u64 rsp_pa;
@@ -338,12 +339,27 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	if (IS_ERR(priv->cca))
 		return PTR_ERR(priv->cca);
 
-	cmd_pa = ((u64) ioread32(&priv->cca->cmd_pa_high) << 32) |
-		  (u64) ioread32(&priv->cca->cmd_pa_low);
+	/*
+	 * PTT HW bug w/a: wake up the device to access
+	 * possibly not retained registers.
+	 */
+	ret = crb_cmd_ready(dev, priv);
+	if (ret)
+		return ret;
+
+	pa_high = ioread32(&priv->cca->cmd_pa_high);
+	pa_low  = ioread32(&priv->cca->cmd_pa_low);
+	cmd_pa = ((u64)pa_high << 32) | pa_low;
 	cmd_size = ioread32(&priv->cca->cmd_size);
+
+	dev_dbg(dev, "cmd_hi = %X cmd_low = %X cmd_size %X\n",
+		pa_high, pa_low, cmd_size);
+
 	priv->cmd = crb_map_res(dev, priv, &io_res, cmd_pa, cmd_size);
-	if (IS_ERR(priv->cmd))
-		return PTR_ERR(priv->cmd);
+	if (IS_ERR(priv->cmd)) {
+		ret = PTR_ERR(priv->cmd);
+		goto out;
+	}
 
 	memcpy_fromio(&rsp_pa, &priv->cca->rsp_pa, 8);
 	rsp_pa = le64_to_cpu(rsp_pa);
@@ -351,7 +367,8 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 
 	if (cmd_pa != rsp_pa) {
 		priv->rsp = crb_map_res(dev, priv, &io_res, rsp_pa, rsp_size);
-		return PTR_ERR_OR_ZERO(priv->rsp);
+		ret = PTR_ERR_OR_ZERO(priv->rsp);
+		goto out;
 	}
 
 	/* According to the PTP specification, overlapping command and response
@@ -359,12 +376,18 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	 */
 	if (cmd_size != rsp_size) {
 		dev_err(dev, FW_BUG "overlapping command and response buffer sizes are not identical");
-		return -EINVAL;
+		ret = -EINVAL;
+		goto out;
 	}
+
 	priv->cmd_size = cmd_size;
 
 	priv->rsp = priv->cmd;
-	return 0;
+
+out:
+	crb_go_idle(dev, priv);
+
+	return ret;
 }
 
 static int crb_acpi_add(struct acpi_device *device)
@@ -408,7 +431,15 @@ static int crb_acpi_add(struct acpi_device *device)
 	if (rc)
 		return rc;
 
-	return crb_init(device, priv);
+	rc  = crb_cmd_ready(dev, priv);
+	if (rc)
+		return rc;
+
+	rc = crb_init(device, priv);
+	if (rc)
+		crb_go_idle(dev, priv);
+
+	return rc;
 }
 
 static int crb_acpi_remove(struct acpi_device *device)
-- 
2.7.4


------------------------------------------------------------------------------

[PATCH v2 3/4] tpm/tpm_crb: open code the crb_init into acpi_add

[Tomas Winkler]

This is preparation step for implementing tpm crb
runtime pm. We need to have tpm chip allocated
and populated before we access the runtime handlers.

Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
V2: new in the series

 drivers/char/tpm/tpm_crb.c | 26 ++++++++++----------------
 1 file changed, 10 insertions(+), 16 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 69e305f49310..524f34b5371e 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -258,21 +258,6 @@ static const struct tpm_class_ops tpm_crb = {
 	.req_complete_val = CRB_DRV_STS_COMPLETE,
 };
 
-static int crb_init(struct acpi_device *device, struct crb_priv *priv)
-{
-	struct tpm_chip *chip;
-
-	chip = tpmm_chip_alloc(&device->dev, &tpm_crb);
-	if (IS_ERR(chip))
-		return PTR_ERR(chip);
-
-	dev_set_drvdata(&chip->dev, priv);
-	chip->acpi_dev_handle = device->handle;
-	chip->flags = TPM_CHIP_FLAG_TPM2;
-
-	return tpm_chip_register(chip);
-}
-
 static int crb_check_resource(struct acpi_resource *ares, void *data)
 {
 	struct resource *io_res = data;
@@ -394,6 +379,7 @@ static int crb_acpi_add(struct acpi_device *device)
 {
 	struct acpi_table_tpm2 *buf;
 	struct crb_priv *priv;
+	struct tpm_chip *chip;
 	struct device *dev = &device->dev;
 	acpi_status status;
 	u32 sm;
@@ -431,11 +417,19 @@ static int crb_acpi_add(struct acpi_device *device)
 	if (rc)
 		return rc;
 
+	chip = tpmm_chip_alloc(dev, &tpm_crb);
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+
+	dev_set_drvdata(&chip->dev, priv);
+	chip->acpi_dev_handle = device->handle;
+	chip->flags = TPM_CHIP_FLAG_TPM2;
+
 	rc  = crb_cmd_ready(dev, priv);
 	if (rc)
 		return rc;
 
-	rc = crb_init(device, priv);
+	rc = tpm_chip_register(chip);
 	if (rc)
 		crb_go_idle(dev, priv);
 
-- 
2.7.4


------------------------------------------------------------------------------

[PATCH v2 4/4] tmp/tpm_crb: implement runtime pm for tpm_crb

[Tomas Winkler]

Utilize runtime_pm for driving tpm crb idle states.
The framework calls cmd_ready from the pm_runtime_resume handler
and go idle from the pm_runtime_suspend handler.
The TPM framework should wake the device before transmit and receive.
In case the runtime_pm framework is not enabled, the device will be in
ready state.

Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
---
V2 new in the series 
 drivers/char/tpm/tpm-interface.c |  5 +++++
 drivers/char/tpm/tpm_crb.c       | 37 ++++++++++++++++++++++++++++++++++---
 2 files changed, 39 insertions(+), 3 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index fd863ff30f79..9ccad3a9875f 100644
--- a/drivers/char/tpm/tpm-interface.c
+++ b/drivers/char/tpm/tpm-interface.c
@@ -29,6 +29,7 @@
 #include <linux/mutex.h>
 #include <linux/spinlock.h>
 #include <linux/freezer.h>
+#include <linux/pm_runtime.h>
 
 #include "tpm.h"
 #include "tpm_eventlog.h"
@@ -353,6 +354,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz,
 	if (!(flags & TPM_TRANSMIT_UNLOCKED))
 		mutex_lock(&chip->tpm_mutex);
 
+	pm_runtime_get_sync(chip->dev.parent);
+
 	rc = chip->ops->send(chip, (u8 *) buf, count);
 	if (rc < 0) {
 		dev_err(&chip->dev,
@@ -394,6 +397,8 @@ out_recv:
 		dev_err(&chip->dev,
 			"tpm_transmit: tpm_recv: error %zd\n", rc);
 out:
+	pm_runtime_put_sync(chip->dev.parent);
+
 	if (!(flags & TPM_TRANSMIT_UNLOCKED))
 		mutex_unlock(&chip->tpm_mutex);
 	return rc;
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 524f34b5371e..5b4c1f6246ed 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -19,6 +19,7 @@
 #include <linux/highmem.h>
 #include <linux/rculist.h>
 #include <linux/module.h>
+#include <linux/pm_runtime.h>
 #include "tpm.h"
 
 #define ACPI_SIG_TPM2 "TPM2"
@@ -145,8 +146,6 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
 	return 0;
 }
 
-static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
-
 static u8 crb_status(struct tpm_chip *chip)
 {
 	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
@@ -429,9 +428,16 @@ static int crb_acpi_add(struct acpi_device *device)
 	if (rc)
 		return rc;
 
+	pm_runtime_set_active(dev);
+	pm_runtime_enable(dev);
+
 	rc = tpm_chip_register(chip);
-	if (rc)
+	if (rc) {
 		crb_go_idle(dev, priv);
+		pm_runtime_disable(dev);
+	}
+
+	pm_runtime_put(dev);
 
 	return rc;
 }
@@ -443,9 +449,34 @@ static int crb_acpi_remove(struct acpi_device *device)
 
 	tpm_chip_unregister(chip);
 
+	pm_runtime_disable(dev);
+
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int crb_pm_runtime_suspend(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
+
+	return crb_go_idle(dev, priv);
+}
+
+static int crb_pm_runtime_resume(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
+
+	return crb_cmd_ready(dev, priv);
+}
+#endif /* CONFIG_PM */
+
+static const struct dev_pm_ops crb_pm = {
+	SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume)
+	SET_RUNTIME_PM_OPS(crb_pm_runtime_suspend, crb_pm_runtime_resume, NULL)
+};
+
 static struct acpi_device_id crb_device_ids[] = {
 	{"MSFT0101", 0},
 	{"", 0},
-- 
2.7.4


------------------------------------------------------------------------------

Re: [PATCH v2 1/4] tpm/tpm_crb: implement tpm crb idle state

[Jarkko Sakkinen]

Could you also put this into linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org?

On Mon, Sep 12, 2016 at 11:54:58AM +0300, Tomas Winkler wrote:
> The register TPM_CRB_CTRL_REQ_x contains bits goIdle and cmdReady for
> SW to indicate that the device can enter or should exit the idle state.
> 
> The legacy ACPI-start (SMI + DMA) based devices do not support these
> bits and the idle state management is not exposed to the host SW.
> Thus, this functionality only is enabled only for a CRB start (MMIO)
> based devices.
> 
> Based on Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> oringal patch:
> 'tpm_crb: implement power tpm crb power management'
> 
> Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
> V2: do not export the functions via tpm ops

I'm not sure about this. Even if callbacks are there tpm_crb and other
device drivers can use runtime PM internally (if they want).

>  drivers/char/tpm/tpm_crb.c | 62 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 62 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 6e9d1bca712f..49023ac3dea1 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -83,6 +83,68 @@ struct crb_priv {
>  	u32 cmd_size;
>  };
>  
> +/**
> + * crb_go_idle - 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.

Why the function descriptions have different formatting than elsewhere
in the subsystem?

> + *
> + * @dev:  crb device

'pdev' would be a better name to differentiate from character device.  I
know that in crb_acpi_add the name of the local is dev but it was just a
bad choice by me.

Also documentation could be:

@pdev:	CRB platform device

> + * @priv: crb private data

@priv:	CRB private data

> + * return:  0 always

According to

https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt

it should be 'Return:'.

Why this isn't a void function?

> + */
> +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_ctrl_req_go_idle, &priv->cca->req);
> +	/* we don't really care when this settles */
> +
> +	return 0;
> +}
> +
> +/**
> + * crb_cmd_ready - 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
> + *
> + * @dev:  crb device
> + * @priv: crb private data
> + *
> + * return:  0 on success -etime on timeout;

Same stuff about the documentation as for the previous function.

Also, I don't like the naming. I would rather have the names I did for
[1]. There I have 'go_to_idle' and 'go_to_ready', which are much more
obvious. I'm can live also with go_ready and go_idle if you prefer that.

> + */
> +static int __maybe_unused crb_cmd_ready(struct device *dev,
> +					struct crb_priv *priv)
> +{
> +	ktime_t stop, start;
> +
> +	if (priv->flags & crb_fl_acpi_start)
> +		return 0;
> +
> +	iowrite32(crb_ctrl_req_cmd_ready, &priv->cca->req);
> +
> +	start = ktime_get();
> +	stop = ktime_add(start, ms_to_ktime(tpm2_timeout_c));
> +	do {
> +		if (!(ioread32(&priv->cca->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->cca->req) & crb_ctrl_req_cmd_ready) {
> +		dev_warn(dev, "cmdready timed out\n");
> +		return -etime;
> +	}
> +
> +	return 0;
> +}
> +

Please use wait_for_tpm_stat(). Please argument in th commit message
if you don't. So far the arguments haven't made sense to me.

I think the whole status thing should be redesigned to have common
synthetized status code shared by all TPM drivers but the way I use it
in [1] works. It's bit ugly but I rather have that than duplicate code.

>  static simple_dev_pm_ops(crb_pm, tpm_pm_suspend, tpm_pm_resume);
>  
>  static u8 crb_status(struct tpm_chip *chip)
> -- 
> 2.7.4
> 

[1] http://git.infradead.org/users/jjs/linux-tpmdd.git/commitdiff/7a1172b5b3cb38083ae931309db216db3c528efe

/Jarkko

------------------------------------------------------------------------------

Re: [PATCH v2 1/4] tpm/tpm_crb: implement tpm crb idle state

[Winkler, Tomas]

> -----Original Message-----
> From: Jarkko Sakkinen [mailto:jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org]
> Sent: Monday, September 12, 2016 15:01
> To: Winkler, Tomas <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Jason Gunthorpe
> <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Subject: Re: [PATCH v2 1/4] tpm/tpm_crb: implement tpm crb idle state
> 
> Could you also put this into linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org?
> 
> On Mon, Sep 12, 2016 at 11:54:58AM +0300, Tomas Winkler wrote:
> > The register TPM_CRB_CTRL_REQ_x contains bits goIdle and cmdReady for
> > SW to indicate that the device can enter or should exit the idle state.
> >
> > The legacy ACPI-start (SMI + DMA) based devices do not support these
> > bits and the idle state management is not exposed to the host SW.
> > Thus, this functionality only is enabled only for a CRB start (MMIO)
> > based devices.
> >
> > Based on Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> oringal
> > patch:
> > 'tpm_crb: implement power tpm crb power management'
> >
> > Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > ---
> > V2: do not export the functions via tpm ops
> 
> I'm not sure about this. Even if callbacks are there tpm_crb and other device
> drivers can use runtime PM internally (if they want).
> 
> >  drivers/char/tpm/tpm_crb.c | 62
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 62 insertions(+)
> >
> > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > index 6e9d1bca712f..49023ac3dea1 100644
> > --- a/drivers/char/tpm/tpm_crb.c
> > +++ b/drivers/char/tpm/tpm_crb.c
> > @@ -83,6 +83,68 @@ struct crb_priv {
> >  	u32 cmd_size;
> >  };
> >
> > +/**
> > + * crb_go_idle - 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.
> 
> Why the function descriptions have different formatting than elsewhere in
> the subsystem?
Oops c&p error, moved the code to much around. 
 
> 
> > + *
> > + * @dev:  crb device
> 
> 'pdev' would be a better name to differentiate from character device.  I
> know that in crb_acpi_add the name of the local is dev but it was just a bad
> choice by me.
> 
> Also documentation could be:
> 
> @pdev:	CRB platform device

I find pdev here  a bit misleading this is not a platform device, also if we want to clean up the variable names this should not be mixed within these patches. 
> 
> > + * @priv: crb private data
> 
> @priv:	CRB private data
> 
> > + * return:  0 always
> 
> According to
> 
> https://www.kernel.org/doc/Documentation/kernel-doc-nano-HOWTO.txt
> 
> it should be 'Return:'.
Opps, wrong 'vi' sequence :)
> 
> Why this isn't a void function?

In general we can fail here on timeout, we just ignore it for this particular hardware.

> 	
> > + */
> > +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_ctrl_req_go_idle, &priv->cca->req);
> > +	/* we don't really care when this settles */
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * crb_cmd_ready - 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
> > + *
> > + * @dev:  crb device
> > + * @priv: crb private data
> > + *
> > + * return:  0 on success -etime on timeout;
> 
> Same stuff about the documentation as for the previous function.
Same here 'vi' sequence while pasting, will fix 

> Also, I don't like the naming. I would rather have the names I did for [1].
> There I have 'go_to_idle' and 'go_to_ready', which are much more obvious.
> I'm can live also with go_ready and go_idle if you prefer that.

go_idle and cmd_ready follow the TMP2 spec 

> > + */
> > +static int __maybe_unused crb_cmd_ready(struct device *dev,
> > +					struct crb_priv *priv)
> > +{
> > +	ktime_t stop, start;
> > +
> > +	if (priv->flags & crb_fl_acpi_start)
> > +		return 0;
> > +
> > +	iowrite32(crb_ctrl_req_cmd_ready, &priv->cca->req);
> > +
> > +	start = ktime_get();
> > +	stop = ktime_add(start, ms_to_ktime(tpm2_timeout_c));
> > +	do {
> > +		if (!(ioread32(&priv->cca->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->cca->req) & crb_ctrl_req_cmd_ready) {
> > +		dev_warn(dev, "cmdready timed out\n");
> > +		return -etime;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> 
> Please use wait_for_tpm_stat(). Please argument in th commit message if
> you don't. So far the arguments haven't made sense to me.

I say it again this should not go via tpm_stat this is conceptually wrong.
Second why would you want to use such complex function that also cumulates side effects 
for simple polling of register that should be used at one place. Plus other reason I've mentioned before.

Last you need 'struct tpm_chip' available which just not the case here.

> I think the whole status thing should be redesigned to have common
> synthetized status code shared by all TPM drivers but the way I use it in [1]
> works. It's bit ugly but I rather have that than duplicate code.

Sorry but the such polling loops is duplicated on many places just in the tpm subsystem, 
This is not case of code duplication. 
> 
> >  static simple_dev_pm_ops(crb_pm, tpm_pm_suspend,
> tpm_pm_resume);
> >
> >  static u8 crb_status(struct tpm_chip *chip)
> > --
> > 2.7.4
> >
> 
> [1] http://git.infradead.org/users/jjs/linux-
> tpmdd.git/commitdiff/7a1172b5b3cb38083ae931309db216db3c528efe
> 
> /Jarkko

Will resend. 


------------------------------------------------------------------------------

Re: [PATCH v2 4/4] tmp/tpm_crb: implement runtime pm for tpm_crb

[Jarkko Sakkinen]

On Mon, Sep 12, 2016 at 11:55:01AM +0300, Tomas Winkler wrote:
> Utilize runtime_pm for driving tpm crb idle states.
> The framework calls cmd_ready from the pm_runtime_resume handler
> and go idle from the pm_runtime_suspend handler.
> The TPM framework should wake the device before transmit and receive.
> In case the runtime_pm framework is not enabled, the device will be in
> ready state.
> 
> Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> ---
> V2 new in the series 
>  drivers/char/tpm/tpm-interface.c |  5 +++++
>  drivers/char/tpm/tpm_crb.c       | 37 ++++++++++++++++++++++++++++++++++---
>  2 files changed, 39 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index fd863ff30f79..9ccad3a9875f 100644
> --- a/drivers/char/tpm/tpm-interface.c
> +++ b/drivers/char/tpm/tpm-interface.c
> @@ -29,6 +29,7 @@
>  #include <linux/mutex.h>
>  #include <linux/spinlock.h>
>  #include <linux/freezer.h>
> +#include <linux/pm_runtime.h>
>  
>  #include "tpm.h"
>  #include "tpm_eventlog.h"
> @@ -353,6 +354,8 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const u8 *buf, size_t bufsiz,
>  	if (!(flags & TPM_TRANSMIT_UNLOCKED))
>  		mutex_lock(&chip->tpm_mutex);
>  
> +	pm_runtime_get_sync(chip->dev.parent);
> +
>  	rc = chip->ops->send(chip, (u8 *) buf, count);
>  	if (rc < 0) {
>  		dev_err(&chip->dev,
> @@ -394,6 +397,8 @@ out_recv:
>  		dev_err(&chip->dev,
>  			"tpm_transmit: tpm_recv: error %zd\n", rc);
>  out:
> +	pm_runtime_put_sync(chip->dev.parent);

This can be just pm_runtime_put.

/Jarkko

> +
>  	if (!(flags & TPM_TRANSMIT_UNLOCKED))
>  		mutex_unlock(&chip->tpm_mutex);
>  	return rc;
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 524f34b5371e..5b4c1f6246ed 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -19,6 +19,7 @@
>  #include <linux/highmem.h>
>  #include <linux/rculist.h>
>  #include <linux/module.h>
> +#include <linux/pm_runtime.h>
>  #include "tpm.h"
>  
>  #define ACPI_SIG_TPM2 "TPM2"
> @@ -145,8 +146,6 @@ static int __maybe_unused crb_cmd_ready(struct device *dev,
>  	return 0;
>  }
>  
> -static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
> -
>  static u8 crb_status(struct tpm_chip *chip)
>  {
>  	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> @@ -429,9 +428,16 @@ static int crb_acpi_add(struct acpi_device *device)
>  	if (rc)
>  		return rc;
>  
> +	pm_runtime_set_active(dev);
> +	pm_runtime_enable(dev);
> +
>  	rc = tpm_chip_register(chip);
> -	if (rc)
> +	if (rc) {
>  		crb_go_idle(dev, priv);
> +		pm_runtime_disable(dev);
> +	}
> +
> +	pm_runtime_put(dev);
>  
>  	return rc;
>  }
> @@ -443,9 +449,34 @@ static int crb_acpi_remove(struct acpi_device *device)
>  
>  	tpm_chip_unregister(chip);
>  
> +	pm_runtime_disable(dev);
> +
>  	return 0;
>  }
>  
> +#ifdef CONFIG_PM
> +static int crb_pm_runtime_suspend(struct device *dev)
> +{
> +	struct tpm_chip *chip = dev_get_drvdata(dev);
> +	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> +
> +	return crb_go_idle(dev, priv);
> +}
> +
> +static int crb_pm_runtime_resume(struct device *dev)
> +{
> +	struct tpm_chip *chip = dev_get_drvdata(dev);
> +	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> +
> +	return crb_cmd_ready(dev, priv);
> +}
> +#endif /* CONFIG_PM */
> +
> +static const struct dev_pm_ops crb_pm = {
> +	SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume)
> +	SET_RUNTIME_PM_OPS(crb_pm_runtime_suspend, crb_pm_runtime_resume, NULL)
> +};
> +
>  static struct acpi_device_id crb_device_ids[] = {
>  	{"MSFT0101", 0},
>  	{"", 0},
> -- 
> 2.7.4
> 

------------------------------------------------------------------------------

Re: [PATCH v2 1/4] tpm/tpm_crb: implement tpm crb idle state

[Jarkko Sakkinen]

On Mon, Sep 12, 2016 at 03:01:09PM +0300, Jarkko Sakkinen wrote:
> Could you also put this into linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org?
> 
> On Mon, Sep 12, 2016 at 11:54:58AM +0300, Tomas Winkler wrote:
> > The register TPM_CRB_CTRL_REQ_x contains bits goIdle and cmdReady for
> > SW to indicate that the device can enter or should exit the idle state.
> > 
> > The legacy ACPI-start (SMI + DMA) based devices do not support these
> > bits and the idle state management is not exposed to the host SW.
> > Thus, this functionality only is enabled only for a CRB start (MMIO)
> > based devices.
> > 
> > Based on Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > oringal patch:
> > 'tpm_crb: implement power tpm crb power management'
> > 
> > Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > ---
> > V2: do not export the functions via tpm ops
> 
> I'm not sure about this. Even if callbacks are there tpm_crb and other
> device drivers can use runtime PM internally (if they want).

I give this some rethought. Using runtime PM is fine.

/Jarkko

------------------------------------------------------------------------------

Re: [PATCH v2 1/4] tpm/tpm_crb: implement tpm crb idle state

[Winkler, Tomas]

> -----Original Message-----
> From: Jarkko Sakkinen [mailto:jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org]
> Sent: Monday, September 12, 2016 16:32
> To: Winkler, Tomas <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> Cc: tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; Jason Gunthorpe
> <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Subject: Re: [PATCH v2 1/4] tpm/tpm_crb: implement tpm crb idle state
> 
> On Mon, Sep 12, 2016 at 03:01:09PM +0300, Jarkko Sakkinen wrote:
> > Could you also put this into linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org?
> >
> > On Mon, Sep 12, 2016 at 11:54:58AM +0300, Tomas Winkler wrote:
> > > The register TPM_CRB_CTRL_REQ_x contains bits goIdle and cmdReady
> > > for SW to indicate that the device can enter or should exit the idle state.
> > >
> > > The legacy ACPI-start (SMI + DMA) based devices do not support these
> > > bits and the idle state management is not exposed to the host SW.
> > > Thus, this functionality only is enabled only for a CRB start (MMIO)
> > > based devices.
> > >
> > > Based on Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> oringal
> > > patch:
> > > 'tpm_crb: implement power tpm crb power management'
> > >
> > > Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > ---
> > > V2: do not export the functions via tpm ops
> >
> > I'm not sure about this. Even if callbacks are there tpm_crb and other
> > device drivers can use runtime PM internally (if they want).
> 
> I give this some rethought. Using runtime PM is fine.

Appreciated. 
Thanks
Tomas 


------------------------------------------------------------------------------

Re: [PATCH v2 1/4] tpm/tpm_crb: implement tpm crb idle state

[Jason Gunthorpe]

On Mon, Sep 12, 2016 at 11:54:58AM +0300, Tomas Winkler wrote:
> The register TPM_CRB_CTRL_REQ_x contains bits goIdle and cmdReady for
> SW to indicate that the device can enter or should exit the idle state.
> 
> The legacy ACPI-start (SMI + DMA) based devices do not support these
> bits and the idle state management is not exposed to the host SW.
> Thus, this functionality only is enabled only for a CRB start (MMIO)
> based devices.
> 
> Based on Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> oringal patch:
> 'tpm_crb: implement power tpm crb power management'
> 
> Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> V2: do not export the functions via tpm ops

You need to restructure this series, do not implement functions that
are not used in a patch.

> +static int __maybe_unused crb_go_idle(struct device *dev, struct crb_priv *priv)

.. and then don't hide the warnings with maybe_unused :(

Jason

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 1/4] tpm/tpm_crb: implement tpm crb idle state

[Jason Gunthorpe]

On Mon, Sep 12, 2016 at 12:25:21PM +0000, Winkler, Tomas wrote:

> > @pdev:	CRB platform device
> 
> I find pdev here  a bit misleading this is not a platform device,
> also if we want to clean up the variable names this should not be
> mixed within these patches.

These days within tpm core it means 'parent device'

Jason

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 3/4] tpm/tpm_crb: open code the crb_init into acpi_add

[Jason Gunthorpe]

On Mon, Sep 12, 2016 at 11:55:00AM +0300, Tomas Winkler wrote:
> This is preparation step for implementing tpm crb
> runtime pm. We need to have tpm chip allocated
> and populated before we access the runtime handlers.

This should probably be the first patch and then you can use the chip
in the signtuare for those functions you added, which is sort of the
point..

Jason

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 1/4] tpm/tpm_crb: implement tpm crb idle state

[Jarkko Sakkinen]

On Mon, Sep 12, 2016 at 11:39:02AM -0600, Jason Gunthorpe wrote:
> On Mon, Sep 12, 2016 at 12:25:21PM +0000, Winkler, Tomas wrote:
> 
> > > @pdev:	CRB platform device
> > 
> > I find pdev here  a bit misleading this is not a platform device,
> > also if we want to clean up the variable names this should not be
> > mixed within these patches.
> 
> These days within tpm core it means 'parent device'

That's a better name, agreed. 

> Jason

/Jarkko

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 1/4] tpm/tpm_crb: implement tpm crb idle state

[Winkler, Tomas]

> 
> On Mon, Sep 12, 2016 at 11:54:58AM +0300, Tomas Winkler wrote:
> > The register TPM_CRB_CTRL_REQ_x contains bits goIdle and cmdReady for
> > SW to indicate that the device can enter or should exit the idle state.
> >
> > The legacy ACPI-start (SMI + DMA) based devices do not support these
> > bits and the idle state management is not exposed to the host SW.
> > Thus, this functionality only is enabled only for a CRB start (MMIO)
> > based devices.
> >
> > Based on Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> oringal
> > patch:
> > 'tpm_crb: implement power tpm crb power management'
> >
> > Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > V2: do not export the functions via tpm ops
> 
> You need to restructure this series, do not implement functions that are not
> used in a patch.

It's perfectly fine to stage patches for easier review, even if it cause harmless warning.

> > +static int __maybe_unused crb_go_idle(struct device *dev, struct
> > +crb_priv *priv)
> 
> .. and then don't hide the warnings with maybe_unused :(

This is not hiding,   the issue is that when runtime pm is not compiled and the hw bug is fixed in next gen of the hardware this function will be unused. 

Tomas


------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 1/4] tpm/tpm_crb: implement tpm crb idle state

[Winkler, Tomas]

> On Mon, Sep 12, 2016 at 11:39:02AM -0600, Jason Gunthorpe wrote:
> > On Mon, Sep 12, 2016 at 12:25:21PM +0000, Winkler, Tomas wrote:
> >
> > > > @pdev:	CRB platform device
> > >
> > > I find pdev here  a bit misleading this is not a platform device,
> > > also if we want to clean up the variable names this should not be
> > > mixed within these patches.
> >
> > These days within tpm core it means 'parent device'
> 
> That's a better name, agreed.

Just the tpm_crb is the low level driver hence it this context it Is not a parent device,  the is the device :)

Tomas 

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 1/4] tpm/tpm_crb: implement tpm crb idle state

[Jason Gunthorpe]

On Mon, Sep 12, 2016 at 08:26:34PM +0000, Winkler, Tomas wrote:
> > > Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > V2: do not export the functions via tpm ops
> > 
> > You need to restructure this series, do not implement functions that are not
> > used in a patch.
> 
> It's perfectly fine to stage patches for easier review, even if it
> cause harmless warning.

That really isn't consistent with the kernel process, please don't do
it.

> > > +static int __maybe_unused crb_go_idle(struct device *dev, struct
> > > +crb_priv *priv)
> > 
> > .. and then don't hide the warnings with maybe_unused :(
> 
> This is not hiding, the issue is that when runtime pm is not
> compiled and the hw bug is fixed in next gen of the hardware this
> function will be unused.

Hum, that might be ok, but it might be better to wrapper the functions
in the runtime pm ifdef

Jason

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 3/4] tpm/tpm_crb: open code the crb_init into acpi_add

[Winkler, Tomas]

> 
> On Mon, Sep 12, 2016 at 11:55:00AM +0300, Tomas Winkler wrote:
> > This is preparation step for implementing tpm crb runtime pm. We need
> > to have tpm chip allocated and populated before we access the runtime
> > handlers.
> 
> This should probably be the first patch and then you can use the chip in the
> signtuare for those functions you added, which is sort of the point..

This series is staged differently  before, first to address the hw bug in a minimalistic approach  just locally  within the tpm_crb as this has to be backported, This is a must, w/o it device just doesn't work,  and only on top of them add runtime pm framework which is important but not necessary feature. 
 
Tomas

------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 1/4] tpm/tpm_crb: implement tpm crb idle state

[Winkler, Tomas]

> 
> On Mon, Sep 12, 2016 at 08:26:34PM +0000, Winkler, Tomas wrote:
> > > > Signed-off-by: Tomas Winkler <tomas.winkler-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
> > > > V2: do not export the functions via tpm ops
> > >
> > > You need to restructure this series, do not implement functions that
> > > are not used in a patch.
> >
> > It's perfectly fine to stage patches for easier review, even if it
> > cause harmless warning.
> 
> That really isn't consistent with the kernel process, please don't do it.

Fortunately none of us is a priest of the kernel process :),  I have some mileage in kernel too, and I agree it's not so encouraged but it's okay to easy on review.

> 
> > > > +static int __maybe_unused crb_go_idle(struct device *dev, struct
> > > > +crb_priv *priv)
> > >
> > > .. and then don't hide the warnings with maybe_unused :(
> >
> > This is not hiding, the issue is that when runtime pm is not compiled
> > and the hw bug is fixed in next gen of the hardware this function will
> > be unused.
> 
> Hum, that might be ok, but it might be better to wrapper the functions in the
> runtime pm ifdef

This would break the hw again... right.
Thanks
Tomas 


------------------------------------------------------------------------------
What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic
patterns at an interface-level. Reveals which users, apps, and protocols are 
consuming the most bandwidth. Provides multi-vendor support for NetFlow, 
J-Flow, sFlow and other flows. Make informed decisions using capacity 
planning reports. http://sdm.link/zohodev2dev

Re: [PATCH v2 1/4] tpm/tpm_crb: implement tpm crb idle state

[Jarkko Sakkinen]

On Mon, Sep 12, 2016 at 12:25:21PM +0000, Winkler, Tomas wrote:
> > Also, I don't like the naming. I would rather have the names I did for [1].
> > There I have 'go_to_idle' and 'go_to_ready', which are much more obvious.
> > I'm can live also with go_ready and go_idle if you prefer that.
> 
> go_idle and cmd_ready follow the TMP2 spec 

'goIdle' and 'cmdReady' are CRB specific (PTP spec). I think here it
would make sense to have readable names. 'cmd_ready' sounds like it
would be return a status code or something.

I'm open for other suggestions than the ones that I proposed abouve
but cmd_ready sounds like it would return a status code.
`
> > Please use wait_for_tpm_stat(). Please argument in th commit message if
> > you don't. So far the arguments haven't made sense to me.
> 
> I say it again this should not go via tpm_stat this is conceptually
> wrong.  Second why would you want to use such complex function that
> also cumulates side effects for simple polling of register that should
> be used at one place. Plus other reason I've mentioned before.
> 
> Last you need 'struct tpm_chip' available which just not the case here.

I do agree that the status callback and these masks that we have is a
mess.

I think the tpmdd should migrate to simple design where would have
common synthetized status codes like

enum tpm_chip_status {
	TPM_CHIP_READY = BIT(0),
	TPM_CHIP_IDLE = BIT(1),
	TPM_CHIP_CANCELED = BIT(2), /* also idle */
};

(you might want to wait for two possible end states, that's why bits 
 for states is a better idea than increasing number)

However, until we have such thing, it would make sense not to introduce
redundant code or go through the longer path and fix the status code
handling.

/Jarkko

------------------------------------------------------------------------------