[PATCH 0/3] Runtime PM for TPM2 CRB chips

[PATCH 0/3] Runtime PM for TPM2 CRB chips

[Jarkko Sakkinen]

Support for runtime PM with TPM2 CRB chips such as PTT in Skylake.

Jarkko Sakkinen (3):
  tpm_crb: fix crb_req_canceled behavior
  tpm, tpm_crb: remove wmb()'s
  tpm, tpm_crb: runtime power management

 drivers/char/tpm/tpm-interface.c |  3 ++
 drivers/char/tpm/tpm_crb.c       | 72 ++++++++++++++++++++++++++++++++++++----
 2 files changed, 68 insertions(+), 7 deletions(-)

-- 
2.7.4

[PATCH 0/3] Runtime PM for TPM2 CRB chips

[Jarkko Sakkinen]

Support for runtime PM with TPM2 CRB chips such as PTT in Skylake.

Jarkko Sakkinen (3):
  tpm_crb: fix crb_req_canceled behavior
  tpm, tpm_crb: remove wmb()'s
  tpm, tpm_crb: runtime power management

 drivers/char/tpm/tpm-interface.c |  3 ++
 drivers/char/tpm/tpm_crb.c       | 72 ++++++++++++++++++++++++++++++++++++----
 2 files changed, 68 insertions(+), 7 deletions(-)

-- 
2.7.4

[PATCH 1/3] tpm_crb: fix crb_req_canceled behavior

[Jarkko Sakkinen]

The req_canceled() callback is used by tpm_transmit() periodically to
check whether the request has been canceled while it is receiving a
response from the TPM.

The TPM_CRB_CTRL_CANCEL register was cleared already in the crb_cancel
callback, which has two consequences:

* Cancel might not happen.
* req_canceled() always returns zero.

A better place to clear the register is when starting to send a new
command. The behavior of TPM_CRB_CTRL_CANCEL is described in the
section 5.5.3.6 of the PTP specification.

CC: stable@vger.kernel.org
Fixes: 30fc8d138e91 ("tpm: TPM 2.0 CRB Interface")
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm_crb.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 1547636..b4d46ae 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -142,6 +142,11 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
 	int rc = 0;
 
+	/* Zero the cancel register so that the next command will not get
+	 * canceled.
+	 */
+	iowrite32(0, &priv->cca->cancel);
+
 	if (len > ioread32(&priv->cca->cmd_size)) {
 		dev_err(&chip->dev,
 			"invalid command count value %x %zx\n",
@@ -175,8 +180,6 @@ static void crb_cancel(struct tpm_chip *chip)
 
 	if ((priv->flags & CRB_FL_ACPI_START) && crb_do_acpi_start(chip))
 		dev_err(&chip->dev, "ACPI Start failed\n");
-
-	iowrite32(0, &priv->cca->cancel);
 }
 
 static bool crb_req_canceled(struct tpm_chip *chip, u8 status)
-- 
2.7.4

[PATCH 1/3] tpm_crb: fix crb_req_canceled behavior

[Jarkko Sakkinen]

The req_canceled() callback is used by tpm_transmit() periodically to
check whether the request has been canceled while it is receiving a
response from the TPM.

The TPM_CRB_CTRL_CANCEL register was cleared already in the crb_cancel
callback, which has two consequences:

* Cancel might not happen.
* req_canceled() always returns zero.

A better place to clear the register is when starting to send a new
command. The behavior of TPM_CRB_CTRL_CANCEL is described in the
section 5.5.3.6 of the PTP specification.

CC: stable@vger.kernel.org
Fixes: 30fc8d138e91 ("tpm: TPM 2.0 CRB Interface")
Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm_crb.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 1547636..b4d46ae 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -142,6 +142,11 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len)
 	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
 	int rc = 0;
 
+	/* Zero the cancel register so that the next command will not get
+	 * canceled.
+	 */
+	iowrite32(0, &priv->cca->cancel);
+
 	if (len > ioread32(&priv->cca->cmd_size)) {
 		dev_err(&chip->dev,
 			"invalid command count value %x %zx\n",
@@ -175,8 +180,6 @@ static void crb_cancel(struct tpm_chip *chip)
 
 	if ((priv->flags & CRB_FL_ACPI_START) && crb_do_acpi_start(chip))
 		dev_err(&chip->dev, "ACPI Start failed\n");
-
-	iowrite32(0, &priv->cca->cancel);
 }
 
 static bool crb_req_canceled(struct tpm_chip *chip, u8 status)
-- 
2.7.4

[PATCH 2/3] tpm, tpm_crb: remove wmb()'s

[Jarkko Sakkinen]

wmb()'s are not needed as iowrite32() is used.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm_crb.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index b4d46ae..ca2cad9 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -175,9 +175,6 @@ static void crb_cancel(struct tpm_chip *chip)
 
 	iowrite32(cpu_to_le32(CRB_CANCEL_INVOKE), &priv->cca->cancel);
 
-	/* Make sure that cmd is populated before issuing cancel. */
-	wmb();
-
 	if ((priv->flags & CRB_FL_ACPI_START) && crb_do_acpi_start(chip))
 		dev_err(&chip->dev, "ACPI Start failed\n");
 }
-- 
2.7.4

[PATCH 2/3] tpm, tpm_crb: remove wmb()'s

[Jarkko Sakkinen]

wmb()'s are not needed as iowrite32() is used.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
---
 drivers/char/tpm/tpm_crb.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index b4d46ae..ca2cad9 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -175,9 +175,6 @@ static void crb_cancel(struct tpm_chip *chip)
 
 	iowrite32(cpu_to_le32(CRB_CANCEL_INVOKE), &priv->cca->cancel);
 
-	/* Make sure that cmd is populated before issuing cancel. */
-	wmb();
-
 	if ((priv->flags & CRB_FL_ACPI_START) && crb_do_acpi_start(chip))
 		dev_err(&chip->dev, "ACPI Start failed\n");
 }
-- 
2.7.4


------------------------------------------------------------------------------
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. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e

[PATCH 3/3] tpm, tpm_crb: runtime power management

[Jarkko Sakkinen]

The register TPM_CRB_CTRL_REQ_0 contains bits goIdle and cmdReady for
invoking the chip to suspend and resume. This commit implements runtime
PM for tpm_crb by using these bits.

The legacy ACPI start (SMI + DMA) based devices do not support these
bits. Thus this functionality only is enabled only for CRB start (MMIO)
based devices.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm-interface.c |  3 ++
 drivers/char/tpm/tpm_crb.c       | 62 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 5e3c1b6..3b85648 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"
@@ -350,6 +351,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
 		return -E2BIG;
 	}
 
+	pm_runtime_get_sync(chip->dev.parent);
 	mutex_lock(&chip->tpm_mutex);
 
 	rc = chip->ops->send(chip, (u8 *) buf, count);
@@ -394,6 +396,7 @@ out_recv:
 			"tpm_transmit: tpm_recv: error %zd\n", rc);
 out:
 	mutex_unlock(&chip->tpm_mutex);
+	pm_runtime_put_sync(chip->dev.parent);
 	return rc;
 }
 
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index ca2cad9..71cc7cd 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -20,6 +20,7 @@
 #include <linux/rculist.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include "tpm.h"
 
 #define ACPI_SIG_TPM2 "TPM2"
@@ -41,7 +42,6 @@ enum crb_ca_request {
 
 enum crb_ca_status {
 	CRB_CA_STS_ERROR	= BIT(0),
-	CRB_CA_STS_TPM_IDLE	= BIT(1),
 };
 
 enum crb_start {
@@ -68,6 +68,8 @@ struct crb_control_area {
 
 enum crb_status {
 	CRB_STS_COMPLETE	= BIT(0),
+	CRB_STS_READY		= BIT(1),
+	CRB_STS_IDLE		= BIT(2),
 };
 
 enum crb_flags {
@@ -81,9 +83,52 @@ struct crb_priv {
 	struct crb_control_area __iomem *cca;
 	u8 __iomem *cmd;
 	u8 __iomem *rsp;
+	wait_queue_head_t idle_queue;
 };
 
-static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
+static int __maybe_unused crb_runtime_suspend(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
+	u32 req;
+
+	if (priv->flags & CRB_FL_ACPI_START)
+		return 0;
+
+	req = ioread32(&priv->cca->req);
+
+	iowrite32(cpu_to_le32(req | CRB_CA_REQ_GO_IDLE), &priv->cca->req);
+
+	if (wait_for_tpm_stat(chip, CRB_STS_IDLE, chip->timeout_c,
+			      &priv->idle_queue, false))
+		dev_warn(&chip->dev, "idle timed out\n");
+
+	return 0;
+}
+
+static int __maybe_unused crb_runtime_resume(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
+	u32 req;
+
+	if (priv->flags & CRB_FL_ACPI_START)
+		return 0;
+
+	req = ioread32(&priv->cca->req);
+	iowrite32(cpu_to_le32(req | CRB_CA_REQ_CMD_READY), &priv->cca->req);
+
+	if (wait_for_tpm_stat(chip, CRB_STS_READY, chip->timeout_c,
+			      &priv->idle_queue, false))
+		dev_warn(&chip->dev, "wake timed out\n");
+
+	return 0;
+}
+
+static const struct dev_pm_ops crb_pm = {
+	SET_RUNTIME_PM_OPS(crb_runtime_suspend, crb_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume)
+};
 
 static u8 crb_status(struct tpm_chip *chip)
 {
@@ -94,6 +139,14 @@ static u8 crb_status(struct tpm_chip *chip)
 	    CRB_START_INVOKE)
 		sts |= CRB_STS_COMPLETE;
 
+	if ((ioread32(&priv->cca->req) & CRB_CA_REQ_CMD_READY) !=
+	    CRB_CA_REQ_CMD_READY)
+		sts |= CRB_STS_READY;
+
+	if ((ioread32(&priv->cca->req) & CRB_CA_REQ_GO_IDLE) !=
+	    CRB_CA_REQ_GO_IDLE)
+		sts |= CRB_STS_IDLE;
+
 	return sts;
 }
 
@@ -206,6 +259,8 @@ static int crb_init(struct acpi_device *device, struct crb_priv *priv)
 	if (IS_ERR(chip))
 		return PTR_ERR(chip);
 
+	pm_runtime_set_active(&device->dev);
+	pm_runtime_enable(&device->dev);
 	dev_set_drvdata(&chip->dev, priv);
 	chip->acpi_dev_handle = device->handle;
 	chip->flags = TPM_CHIP_FLAG_TPM2;
@@ -348,6 +403,8 @@ static int crb_acpi_add(struct acpi_device *device)
 	    !strcmp(acpi_device_hid(device), "MSFT0101"))
 		priv->flags |= CRB_FL_CRB_START;
 
+	init_waitqueue_head(&priv->idle_queue);
+
 	if (sm == ACPI_TPM2_START_METHOD ||
 	    sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
 		priv->flags |= CRB_FL_ACPI_START;
@@ -366,6 +423,7 @@ static int crb_acpi_remove(struct acpi_device *device)
 
 	tpm_chip_unregister(chip);
 
+	pm_runtime_disable(dev);
 	return 0;
 }
 
-- 
2.7.4

[PATCH 3/3] tpm, tpm_crb: runtime power management

[Jarkko Sakkinen]

The register TPM_CRB_CTRL_REQ_0 contains bits goIdle and cmdReady for
invoking the chip to suspend and resume. This commit implements runtime
PM for tpm_crb by using these bits.

The legacy ACPI start (SMI + DMA) based devices do not support these
bits. Thus this functionality only is enabled only for CRB start (MMIO)
based devices.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm-interface.c |  3 ++
 drivers/char/tpm/tpm_crb.c       | 62 ++++++++++++++++++++++++++++++++++++++--
 2 files changed, 63 insertions(+), 2 deletions(-)

diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
index 5e3c1b6..3b85648 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"
@@ -350,6 +351,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
 		return -E2BIG;
 	}
 
+	pm_runtime_get_sync(chip->dev.parent);
 	mutex_lock(&chip->tpm_mutex);
 
 	rc = chip->ops->send(chip, (u8 *) buf, count);
@@ -394,6 +396,7 @@ out_recv:
 			"tpm_transmit: tpm_recv: error %zd\n", rc);
 out:
 	mutex_unlock(&chip->tpm_mutex);
+	pm_runtime_put_sync(chip->dev.parent);
 	return rc;
 }
 
diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index ca2cad9..71cc7cd 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -20,6 +20,7 @@
 #include <linux/rculist.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
 #include "tpm.h"
 
 #define ACPI_SIG_TPM2 "TPM2"
@@ -41,7 +42,6 @@ enum crb_ca_request {
 
 enum crb_ca_status {
 	CRB_CA_STS_ERROR	= BIT(0),
-	CRB_CA_STS_TPM_IDLE	= BIT(1),
 };
 
 enum crb_start {
@@ -68,6 +68,8 @@ struct crb_control_area {
 
 enum crb_status {
 	CRB_STS_COMPLETE	= BIT(0),
+	CRB_STS_READY		= BIT(1),
+	CRB_STS_IDLE		= BIT(2),
 };
 
 enum crb_flags {
@@ -81,9 +83,52 @@ struct crb_priv {
 	struct crb_control_area __iomem *cca;
 	u8 __iomem *cmd;
 	u8 __iomem *rsp;
+	wait_queue_head_t idle_queue;
 };
 
-static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
+static int __maybe_unused crb_runtime_suspend(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
+	u32 req;
+
+	if (priv->flags & CRB_FL_ACPI_START)
+		return 0;
+
+	req = ioread32(&priv->cca->req);
+
+	iowrite32(cpu_to_le32(req | CRB_CA_REQ_GO_IDLE), &priv->cca->req);
+
+	if (wait_for_tpm_stat(chip, CRB_STS_IDLE, chip->timeout_c,
+			      &priv->idle_queue, false))
+		dev_warn(&chip->dev, "idle timed out\n");
+
+	return 0;
+}
+
+static int __maybe_unused crb_runtime_resume(struct device *dev)
+{
+	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct crb_priv *priv = dev_get_drvdata(&chip->dev);
+	u32 req;
+
+	if (priv->flags & CRB_FL_ACPI_START)
+		return 0;
+
+	req = ioread32(&priv->cca->req);
+	iowrite32(cpu_to_le32(req | CRB_CA_REQ_CMD_READY), &priv->cca->req);
+
+	if (wait_for_tpm_stat(chip, CRB_STS_READY, chip->timeout_c,
+			      &priv->idle_queue, false))
+		dev_warn(&chip->dev, "wake timed out\n");
+
+	return 0;
+}
+
+static const struct dev_pm_ops crb_pm = {
+	SET_RUNTIME_PM_OPS(crb_runtime_suspend, crb_runtime_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume)
+};
 
 static u8 crb_status(struct tpm_chip *chip)
 {
@@ -94,6 +139,14 @@ static u8 crb_status(struct tpm_chip *chip)
 	    CRB_START_INVOKE)
 		sts |= CRB_STS_COMPLETE;
 
+	if ((ioread32(&priv->cca->req) & CRB_CA_REQ_CMD_READY) !=
+	    CRB_CA_REQ_CMD_READY)
+		sts |= CRB_STS_READY;
+
+	if ((ioread32(&priv->cca->req) & CRB_CA_REQ_GO_IDLE) !=
+	    CRB_CA_REQ_GO_IDLE)
+		sts |= CRB_STS_IDLE;
+
 	return sts;
 }
 
@@ -206,6 +259,8 @@ static int crb_init(struct acpi_device *device, struct crb_priv *priv)
 	if (IS_ERR(chip))
 		return PTR_ERR(chip);
 
+	pm_runtime_set_active(&device->dev);
+	pm_runtime_enable(&device->dev);
 	dev_set_drvdata(&chip->dev, priv);
 	chip->acpi_dev_handle = device->handle;
 	chip->flags = TPM_CHIP_FLAG_TPM2;
@@ -348,6 +403,8 @@ static int crb_acpi_add(struct acpi_device *device)
 	    !strcmp(acpi_device_hid(device), "MSFT0101"))
 		priv->flags |= CRB_FL_CRB_START;
 
+	init_waitqueue_head(&priv->idle_queue);
+
 	if (sm == ACPI_TPM2_START_METHOD ||
 	    sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
 		priv->flags |= CRB_FL_ACPI_START;
@@ -366,6 +423,7 @@ static int crb_acpi_remove(struct acpi_device *device)
 
 	tpm_chip_unregister(chip);
 
+	pm_runtime_disable(dev);
 	return 0;
 }
 
-- 
2.7.4

Re: [tpmdd-devel] [PATCH 3/3] tpm, tpm_crb: runtime power management

[Tomas Winkler]

On Wed, Jun 8, 2016 at 2:02 AM, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
> The register TPM_CRB_CTRL_REQ_0 contains bits goIdle and cmdReady for
> invoking the chip to suspend and resume. This commit implements runtime
> PM for tpm_crb by using these bits.
>
> The legacy ACPI start (SMI + DMA) based devices do not support these
> bits. Thus this functionality only is enabled only for CRB start (MMIO)
> based devices.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  drivers/char/tpm/tpm-interface.c |  3 ++
>  drivers/char/tpm/tpm_crb.c       | 62 ++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 63 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 5e3c1b6..3b85648 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"
> @@ -350,6 +351,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
>                 return -E2BIG;
>         }
>
> +       pm_runtime_get_sync(chip->dev.parent);
>         mutex_lock(&chip->tpm_mutex);
>
>         rc = chip->ops->send(chip, (u8 *) buf, count);
> @@ -394,6 +396,7 @@ out_recv:
>                         "tpm_transmit: tpm_recv: error %zd\n", rc);
>  out:
>         mutex_unlock(&chip->tpm_mutex);
> +       pm_runtime_put_sync(chip->dev.parent);
>         return rc;
>  }
>
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index ca2cad9..71cc7cd 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -20,6 +20,7 @@
>  #include <linux/rculist.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include "tpm.h"
>
>  #define ACPI_SIG_TPM2 "TPM2"
> @@ -41,7 +42,6 @@ enum crb_ca_request {
>
>  enum crb_ca_status {
>         CRB_CA_STS_ERROR        = BIT(0),
> -       CRB_CA_STS_TPM_IDLE     = BIT(1),
>  };
>
>  enum crb_start {
> @@ -68,6 +68,8 @@ struct crb_control_area {
>
>  enum crb_status {
>         CRB_STS_COMPLETE        = BIT(0),
> +       CRB_STS_READY           = BIT(1),
> +       CRB_STS_IDLE            = BIT(2),
>  };
>
>  enum crb_flags {
> @@ -81,9 +83,52 @@ struct crb_priv {
>         struct crb_control_area __iomem *cca;
>         u8 __iomem *cmd;
>         u8 __iomem *rsp;
> +       wait_queue_head_t idle_queue;


I'm failing to find the code that is calling wake_up_interruptible(idle_queue);

>  };
>
> -static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
> +static int __maybe_unused crb_runtime_suspend(struct device *dev)
> +{
> +       struct tpm_chip *chip = dev_get_drvdata(dev);
> +       struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> +       u32 req;
> +
> +       if (priv->flags & CRB_FL_ACPI_START)
> +               return 0;
> +
> +       req = ioread32(&priv->cca->req);
> +
> +       iowrite32(cpu_to_le32(req | CRB_CA_REQ_GO_IDLE), &priv->cca->req);
> +
> +       if (wait_for_tpm_stat(chip, CRB_STS_IDLE, chip->timeout_c,
> +                             &priv->idle_queue, false))
> +               dev_warn(&chip->dev, "idle timed out\n");

Unfortunately you cannot do that as there is an HW errata, the status
register value might not be defined here during power transition
You should poll for request CRB_CA_REQ_GO_IDLE goes to 0 as descried
in the spec . only after that you can check for the status register
(thought it's maybe not needed)

> +
> +       return 0;
> +}
> +
> +static int __maybe_unused crb_runtime_resume(struct device *dev)

why this is marked unused, why just not compile it out? if the
CONFIG_PM is not set?

> +{
> +       struct tpm_chip *chip = dev_get_drvdata(dev);
> +       struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> +       u32 req;
> +
> +       if (priv->flags & CRB_FL_ACPI_START)
> +               return 0;
> +
> +       req = ioread32(&priv->cca->req);
> +       iowrite32(cpu_to_le32(req | CRB_CA_REQ_CMD_READY), &priv->cca->req);
> +
> +       if (wait_for_tpm_stat(chip, CRB_STS_READY, chip->timeout_c,
> +                             &priv->idle_queue, false))
> +               dev_warn(&chip->dev, "wake timed out\n");

Same here,  you should wait for CRB_CA_REQ_CMD_READ to get cleared,
only after that it is safe to check the status register.

> +
> +       return 0;
> +}
> +
> +static const struct dev_pm_ops crb_pm = {
> +       SET_RUNTIME_PM_OPS(crb_runtime_suspend, crb_runtime_resume, NULL)
> +       SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume)
> +};
>
>  static u8 crb_status(struct tpm_chip *chip)
>  {
> @@ -94,6 +139,14 @@ static u8 crb_status(struct tpm_chip *chip)
>             CRB_START_INVOKE)
>                 sts |= CRB_STS_COMPLETE;
>
> +       if ((ioread32(&priv->cca->req) & CRB_CA_REQ_CMD_READY) !=
> +           CRB_CA_REQ_CMD_READY)
> +               sts |= CRB_STS_READY;

There is meaning for checking this w/o the actual transition i.e.
setting the before

> +       if ((ioread32(&priv->cca->req) & CRB_CA_REQ_GO_IDLE) !=
> +           CRB_CA_REQ_GO_IDLE)
> +               sts |= CRB_STS_IDLE;
Same here.
> +
>         return sts;
>  }
>
> @@ -206,6 +259,8 @@ static int crb_init(struct acpi_device *device, struct crb_priv *priv)
>         if (IS_ERR(chip))
>                 return PTR_ERR(chip);
>
> +       pm_runtime_set_active(&device->dev);
> +       pm_runtime_enable(&device->dev);
>         dev_set_drvdata(&chip->dev, priv);
>         chip->acpi_dev_handle = device->handle;
>         chip->flags = TPM_CHIP_FLAG_TPM2;
> @@ -348,6 +403,8 @@ static int crb_acpi_add(struct acpi_device *device)
>             !strcmp(acpi_device_hid(device), "MSFT0101"))
>                 priv->flags |= CRB_FL_CRB_START;
>
> +       init_waitqueue_head(&priv->idle_queue);
> +
>         if (sm == ACPI_TPM2_START_METHOD ||
>             sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
>                 priv->flags |= CRB_FL_ACPI_START;
> @@ -366,6 +423,7 @@ static int crb_acpi_remove(struct acpi_device *device)
>
>         tpm_chip_unregister(chip);
>
> +       pm_runtime_disable(dev);
>         return 0;
>  }
>
Thanks
Tomas

Re: [tpmdd-devel] [PATCH 3/3] tpm, tpm_crb: runtime power management

[Tomas Winkler]

On Wed, Jun 8, 2016 at 2:02 AM, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
> The register TPM_CRB_CTRL_REQ_0 contains bits goIdle and cmdReady for
> invoking the chip to suspend and resume. This commit implements runtime
> PM for tpm_crb by using these bits.
>
> The legacy ACPI start (SMI + DMA) based devices do not support these
> bits. Thus this functionality only is enabled only for CRB start (MMIO)
> based devices.
>
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  drivers/char/tpm/tpm-interface.c |  3 ++
>  drivers/char/tpm/tpm_crb.c       | 62 ++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 63 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> index 5e3c1b6..3b85648 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"
> @@ -350,6 +351,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
>                 return -E2BIG;
>         }
>
> +       pm_runtime_get_sync(chip->dev.parent);
>         mutex_lock(&chip->tpm_mutex);
>
>         rc = chip->ops->send(chip, (u8 *) buf, count);
> @@ -394,6 +396,7 @@ out_recv:
>                         "tpm_transmit: tpm_recv: error %zd\n", rc);
>  out:
>         mutex_unlock(&chip->tpm_mutex);
> +       pm_runtime_put_sync(chip->dev.parent);
>         return rc;
>  }
>
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index ca2cad9..71cc7cd 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -20,6 +20,7 @@
>  #include <linux/rculist.h>
>  #include <linux/module.h>
>  #include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
>  #include "tpm.h"
>
>  #define ACPI_SIG_TPM2 "TPM2"
> @@ -41,7 +42,6 @@ enum crb_ca_request {
>
>  enum crb_ca_status {
>         CRB_CA_STS_ERROR        = BIT(0),
> -       CRB_CA_STS_TPM_IDLE     = BIT(1),
>  };
>
>  enum crb_start {
> @@ -68,6 +68,8 @@ struct crb_control_area {
>
>  enum crb_status {
>         CRB_STS_COMPLETE        = BIT(0),
> +       CRB_STS_READY           = BIT(1),
> +       CRB_STS_IDLE            = BIT(2),
>  };
>
>  enum crb_flags {
> @@ -81,9 +83,52 @@ struct crb_priv {
>         struct crb_control_area __iomem *cca;
>         u8 __iomem *cmd;
>         u8 __iomem *rsp;
> +       wait_queue_head_t idle_queue;


I'm failing to find the code that is calling wake_up_interruptible(idle_queue);

>  };
>
> -static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
> +static int __maybe_unused crb_runtime_suspend(struct device *dev)
> +{
> +       struct tpm_chip *chip = dev_get_drvdata(dev);
> +       struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> +       u32 req;
> +
> +       if (priv->flags & CRB_FL_ACPI_START)
> +               return 0;
> +
> +       req = ioread32(&priv->cca->req);
> +
> +       iowrite32(cpu_to_le32(req | CRB_CA_REQ_GO_IDLE), &priv->cca->req);
> +
> +       if (wait_for_tpm_stat(chip, CRB_STS_IDLE, chip->timeout_c,
> +                             &priv->idle_queue, false))
> +               dev_warn(&chip->dev, "idle timed out\n");

Unfortunately you cannot do that as there is an HW errata, the status
register value might not be defined here during power transition
You should poll for request CRB_CA_REQ_GO_IDLE goes to 0 as descried
in the spec . only after that you can check for the status register
(thought it's maybe not needed)

> +
> +       return 0;
> +}
> +
> +static int __maybe_unused crb_runtime_resume(struct device *dev)

why this is marked unused, why just not compile it out? if the
CONFIG_PM is not set?

> +{
> +       struct tpm_chip *chip = dev_get_drvdata(dev);
> +       struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> +       u32 req;
> +
> +       if (priv->flags & CRB_FL_ACPI_START)
> +               return 0;
> +
> +       req = ioread32(&priv->cca->req);
> +       iowrite32(cpu_to_le32(req | CRB_CA_REQ_CMD_READY), &priv->cca->req);
> +
> +       if (wait_for_tpm_stat(chip, CRB_STS_READY, chip->timeout_c,
> +                             &priv->idle_queue, false))
> +               dev_warn(&chip->dev, "wake timed out\n");

Same here,  you should wait for CRB_CA_REQ_CMD_READ to get cleared,
only after that it is safe to check the status register.

> +
> +       return 0;
> +}
> +
> +static const struct dev_pm_ops crb_pm = {
> +       SET_RUNTIME_PM_OPS(crb_runtime_suspend, crb_runtime_resume, NULL)
> +       SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume)
> +};
>
>  static u8 crb_status(struct tpm_chip *chip)
>  {
> @@ -94,6 +139,14 @@ static u8 crb_status(struct tpm_chip *chip)
>             CRB_START_INVOKE)
>                 sts |= CRB_STS_COMPLETE;
>
> +       if ((ioread32(&priv->cca->req) & CRB_CA_REQ_CMD_READY) !=
> +           CRB_CA_REQ_CMD_READY)
> +               sts |= CRB_STS_READY;

There is meaning for checking this w/o the actual transition i.e.
setting the before

> +       if ((ioread32(&priv->cca->req) & CRB_CA_REQ_GO_IDLE) !=
> +           CRB_CA_REQ_GO_IDLE)
> +               sts |= CRB_STS_IDLE;
Same here.
> +
>         return sts;
>  }
>
> @@ -206,6 +259,8 @@ static int crb_init(struct acpi_device *device, struct crb_priv *priv)
>         if (IS_ERR(chip))
>                 return PTR_ERR(chip);
>
> +       pm_runtime_set_active(&device->dev);
> +       pm_runtime_enable(&device->dev);
>         dev_set_drvdata(&chip->dev, priv);
>         chip->acpi_dev_handle = device->handle;
>         chip->flags = TPM_CHIP_FLAG_TPM2;
> @@ -348,6 +403,8 @@ static int crb_acpi_add(struct acpi_device *device)
>             !strcmp(acpi_device_hid(device), "MSFT0101"))
>                 priv->flags |= CRB_FL_CRB_START;
>
> +       init_waitqueue_head(&priv->idle_queue);
> +
>         if (sm == ACPI_TPM2_START_METHOD ||
>             sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
>                 priv->flags |= CRB_FL_ACPI_START;
> @@ -366,6 +423,7 @@ static int crb_acpi_remove(struct acpi_device *device)
>
>         tpm_chip_unregister(chip);
>
> +       pm_runtime_disable(dev);
>         return 0;
>  }
>
Thanks
Tomas

Re: [tpmdd-devel] [PATCH 3/3] tpm, tpm_crb: runtime power management

[Jarkko Sakkinen]

Hi Thomas,

I'm on a vacation this week but I'll give you quick answers :)

On Tue, Jun 14, 2016 at 04:14:58PM +0300, Tomas Winkler wrote:
> On Wed, Jun 8, 2016 at 2:02 AM, Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> > The register TPM_CRB_CTRL_REQ_0 contains bits goIdle and cmdReady for
> > invoking the chip to suspend and resume. This commit implements runtime
> > PM for tpm_crb by using these bits.
> >
> > The legacy ACPI start (SMI + DMA) based devices do not support these
> > bits. Thus this functionality only is enabled only for CRB start (MMIO)
> > based devices.
> >
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> >  drivers/char/tpm/tpm-interface.c |  3 ++
> >  drivers/char/tpm/tpm_crb.c       | 62 ++++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 63 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > index 5e3c1b6..3b85648 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"
> > @@ -350,6 +351,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> >                 return -E2BIG;
> >         }
> >
> > +       pm_runtime_get_sync(chip->dev.parent);
> >         mutex_lock(&chip->tpm_mutex);
> >
> >         rc = chip->ops->send(chip, (u8 *) buf, count);
> > @@ -394,6 +396,7 @@ out_recv:
> >                         "tpm_transmit: tpm_recv: error %zd\n", rc);
> >  out:
> >         mutex_unlock(&chip->tpm_mutex);
> > +       pm_runtime_put_sync(chip->dev.parent);
> >         return rc;
> >  }
> >
> > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > index ca2cad9..71cc7cd 100644
> > --- a/drivers/char/tpm/tpm_crb.c
> > +++ b/drivers/char/tpm/tpm_crb.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/rculist.h>
> >  #include <linux/module.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> >  #include "tpm.h"
> >
> >  #define ACPI_SIG_TPM2 "TPM2"
> > @@ -41,7 +42,6 @@ enum crb_ca_request {
> >
> >  enum crb_ca_status {
> >         CRB_CA_STS_ERROR        = BIT(0),
> > -       CRB_CA_STS_TPM_IDLE     = BIT(1),
> >  };
> >
> >  enum crb_start {
> > @@ -68,6 +68,8 @@ struct crb_control_area {
> >
> >  enum crb_status {
> >         CRB_STS_COMPLETE        = BIT(0),
> > +       CRB_STS_READY           = BIT(1),
> > +       CRB_STS_IDLE            = BIT(2),
> >  };
> >
> >  enum crb_flags {
> > @@ -81,9 +83,52 @@ struct crb_priv {
> >         struct crb_control_area __iomem *cca;
> >         u8 __iomem *cmd;
> >         u8 __iomem *rsp;
> > +       wait_queue_head_t idle_queue;
> 
> 
> I'm failing to find the code that is calling wake_up_interruptible(idle_queue);

Ugh, my bad. This actually should not be declared at all. Will remove it
from the next version and NULL should be passed to wait_for_tpm_stat()
as the driver does not yet support interrupts (Haswell did not have
them, not sure about later gens).


> >  };
> >
> > -static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
> > +static int __maybe_unused crb_runtime_suspend(struct device *dev)
> > +{
> > +       struct tpm_chip *chip = dev_get_drvdata(dev);
> > +       struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> > +       u32 req;
> > +
> > +       if (priv->flags & CRB_FL_ACPI_START)
> > +               return 0;
> > +
> > +       req = ioread32(&priv->cca->req);
> > +
> > +       iowrite32(cpu_to_le32(req | CRB_CA_REQ_GO_IDLE), &priv->cca->req);
> > +
> > +       if (wait_for_tpm_stat(chip, CRB_STS_IDLE, chip->timeout_c,
> > +                             &priv->idle_queue, false))
> > +               dev_warn(&chip->dev, "idle timed out\n");
> 
> Unfortunately you cannot do that as there is an HW errata, the status
> register value might not be defined here during power transition
> You should poll for request CRB_CA_REQ_GO_IDLE goes to 0 as descried
> in the spec . only after that you can check for the status register
> (thought it's maybe not needed)

And I do exactly what you are asking me to do.

> > +
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused crb_runtime_resume(struct device *dev)
> 
> why this is marked unused, why just not compile it out? if the
> CONFIG_PM is not set?

It is compiled out if it is unused. Why would you want to trash the code
with #ifdef cages if they are not necessary? I can add /* CONFIG_PM */
before the function if that makes it cleaner.

> > +{
> > +       struct tpm_chip *chip = dev_get_drvdata(dev);
> > +       struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> > +       u32 req;
> > +
> > +       if (priv->flags & CRB_FL_ACPI_START)
> > +               return 0;
> > +
> > +       req = ioread32(&priv->cca->req);
> > +       iowrite32(cpu_to_le32(req | CRB_CA_REQ_CMD_READY), &priv->cca->req);
> > +
> > +       if (wait_for_tpm_stat(chip, CRB_STS_READY, chip->timeout_c,
> > +                             &priv->idle_queue, false))
> > +               dev_warn(&chip->dev, "wake timed out\n");
> 
> Same here,  you should wait for CRB_CA_REQ_CMD_READ to get cleared,
> only after that it is safe to check the status register.

It does exactly that. I'm not using CRB status register for anything.

> > +
> > +       return 0;
> > +}
> > +
> > +static const struct dev_pm_ops crb_pm = {
> > +       SET_RUNTIME_PM_OPS(crb_runtime_suspend, crb_runtime_resume, NULL)
> > +       SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume)
> > +};
> >
> >  static u8 crb_status(struct tpm_chip *chip)
> >  {
> > @@ -94,6 +139,14 @@ static u8 crb_status(struct tpm_chip *chip)
> >             CRB_START_INVOKE)
> >                 sts |= CRB_STS_COMPLETE;
> >
> > +       if ((ioread32(&priv->cca->req) & CRB_CA_REQ_CMD_READY) !=
> > +           CRB_CA_REQ_CMD_READY)
> > +               sts |= CRB_STS_READY;
> 
> There is meaning for checking this w/o the actual transition i.e.
> setting the before

I'm not sure what you are trying to say.

/Jarkko

Re: [tpmdd-devel] [PATCH 3/3] tpm, tpm_crb: runtime power management

[Jarkko Sakkinen]

Hi Thomas,

I'm on a vacation this week but I'll give you quick answers :)

On Tue, Jun 14, 2016 at 04:14:58PM +0300, Tomas Winkler wrote:
> On Wed, Jun 8, 2016 at 2:02 AM, Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> > The register TPM_CRB_CTRL_REQ_0 contains bits goIdle and cmdReady for
> > invoking the chip to suspend and resume. This commit implements runtime
> > PM for tpm_crb by using these bits.
> >
> > The legacy ACPI start (SMI + DMA) based devices do not support these
> > bits. Thus this functionality only is enabled only for CRB start (MMIO)
> > based devices.
> >
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > ---
> >  drivers/char/tpm/tpm-interface.c |  3 ++
> >  drivers/char/tpm/tpm_crb.c       | 62 ++++++++++++++++++++++++++++++++++++++--
> >  2 files changed, 63 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > index 5e3c1b6..3b85648 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"
> > @@ -350,6 +351,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> >                 return -E2BIG;
> >         }
> >
> > +       pm_runtime_get_sync(chip->dev.parent);
> >         mutex_lock(&chip->tpm_mutex);
> >
> >         rc = chip->ops->send(chip, (u8 *) buf, count);
> > @@ -394,6 +396,7 @@ out_recv:
> >                         "tpm_transmit: tpm_recv: error %zd\n", rc);
> >  out:
> >         mutex_unlock(&chip->tpm_mutex);
> > +       pm_runtime_put_sync(chip->dev.parent);
> >         return rc;
> >  }
> >
> > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > index ca2cad9..71cc7cd 100644
> > --- a/drivers/char/tpm/tpm_crb.c
> > +++ b/drivers/char/tpm/tpm_crb.c
> > @@ -20,6 +20,7 @@
> >  #include <linux/rculist.h>
> >  #include <linux/module.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/pm_runtime.h>
> >  #include "tpm.h"
> >
> >  #define ACPI_SIG_TPM2 "TPM2"
> > @@ -41,7 +42,6 @@ enum crb_ca_request {
> >
> >  enum crb_ca_status {
> >         CRB_CA_STS_ERROR        = BIT(0),
> > -       CRB_CA_STS_TPM_IDLE     = BIT(1),
> >  };
> >
> >  enum crb_start {
> > @@ -68,6 +68,8 @@ struct crb_control_area {
> >
> >  enum crb_status {
> >         CRB_STS_COMPLETE        = BIT(0),
> > +       CRB_STS_READY           = BIT(1),
> > +       CRB_STS_IDLE            = BIT(2),
> >  };
> >
> >  enum crb_flags {
> > @@ -81,9 +83,52 @@ struct crb_priv {
> >         struct crb_control_area __iomem *cca;
> >         u8 __iomem *cmd;
> >         u8 __iomem *rsp;
> > +       wait_queue_head_t idle_queue;
> 
> 
> I'm failing to find the code that is calling wake_up_interruptible(idle_queue);

Ugh, my bad. This actually should not be declared at all. Will remove it
from the next version and NULL should be passed to wait_for_tpm_stat()
as the driver does not yet support interrupts (Haswell did not have
them, not sure about later gens).


> >  };
> >
> > -static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
> > +static int __maybe_unused crb_runtime_suspend(struct device *dev)
> > +{
> > +       struct tpm_chip *chip = dev_get_drvdata(dev);
> > +       struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> > +       u32 req;
> > +
> > +       if (priv->flags & CRB_FL_ACPI_START)
> > +               return 0;
> > +
> > +       req = ioread32(&priv->cca->req);
> > +
> > +       iowrite32(cpu_to_le32(req | CRB_CA_REQ_GO_IDLE), &priv->cca->req);
> > +
> > +       if (wait_for_tpm_stat(chip, CRB_STS_IDLE, chip->timeout_c,
> > +                             &priv->idle_queue, false))
> > +               dev_warn(&chip->dev, "idle timed out\n");
> 
> Unfortunately you cannot do that as there is an HW errata, the status
> register value might not be defined here during power transition
> You should poll for request CRB_CA_REQ_GO_IDLE goes to 0 as descried
> in the spec . only after that you can check for the status register
> (thought it's maybe not needed)

And I do exactly what you are asking me to do.

> > +
> > +       return 0;
> > +}
> > +
> > +static int __maybe_unused crb_runtime_resume(struct device *dev)
> 
> why this is marked unused, why just not compile it out? if the
> CONFIG_PM is not set?

It is compiled out if it is unused. Why would you want to trash the code
with #ifdef cages if they are not necessary? I can add /* CONFIG_PM */
before the function if that makes it cleaner.

> > +{
> > +       struct tpm_chip *chip = dev_get_drvdata(dev);
> > +       struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> > +       u32 req;
> > +
> > +       if (priv->flags & CRB_FL_ACPI_START)
> > +               return 0;
> > +
> > +       req = ioread32(&priv->cca->req);
> > +       iowrite32(cpu_to_le32(req | CRB_CA_REQ_CMD_READY), &priv->cca->req);
> > +
> > +       if (wait_for_tpm_stat(chip, CRB_STS_READY, chip->timeout_c,
> > +                             &priv->idle_queue, false))
> > +               dev_warn(&chip->dev, "wake timed out\n");
> 
> Same here,  you should wait for CRB_CA_REQ_CMD_READ to get cleared,
> only after that it is safe to check the status register.

It does exactly that. I'm not using CRB status register for anything.

> > +
> > +       return 0;
> > +}
> > +
> > +static const struct dev_pm_ops crb_pm = {
> > +       SET_RUNTIME_PM_OPS(crb_runtime_suspend, crb_runtime_resume, NULL)
> > +       SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume)
> > +};
> >
> >  static u8 crb_status(struct tpm_chip *chip)
> >  {
> > @@ -94,6 +139,14 @@ static u8 crb_status(struct tpm_chip *chip)
> >             CRB_START_INVOKE)
> >                 sts |= CRB_STS_COMPLETE;
> >
> > +       if ((ioread32(&priv->cca->req) & CRB_CA_REQ_CMD_READY) !=
> > +           CRB_CA_REQ_CMD_READY)
> > +               sts |= CRB_STS_READY;
> 
> There is meaning for checking this w/o the actual transition i.e.
> setting the before

I'm not sure what you are trying to say.

/Jarkko

Re: [tpmdd-devel] [PATCH 3/3] tpm, tpm_crb: runtime power management

[Jarkko Sakkinen]

On Thu, Jun 16, 2016 at 09:57:35PM +0200, Jarkko Sakkinen wrote:
> Hi Thomas,
> 
> I'm on a vacation this week but I'll give you quick answers :)
> 
> On Tue, Jun 14, 2016 at 04:14:58PM +0300, Tomas Winkler wrote:
> > On Wed, Jun 8, 2016 at 2:02 AM, Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> wrote:
> > > The register TPM_CRB_CTRL_REQ_0 contains bits goIdle and cmdReady for
> > > invoking the chip to suspend and resume. This commit implements runtime
> > > PM for tpm_crb by using these bits.
> > >
> > > The legacy ACPI start (SMI + DMA) based devices do not support these
> > > bits. Thus this functionality only is enabled only for CRB start (MMIO)
> > > based devices.
> > >
> > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > ---
> > >  drivers/char/tpm/tpm-interface.c |  3 ++
> > >  drivers/char/tpm/tpm_crb.c       | 62 ++++++++++++++++++++++++++++++++++++++--
> > >  2 files changed, 63 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > > index 5e3c1b6..3b85648 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"
> > > @@ -350,6 +351,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> > >                 return -E2BIG;
> > >         }
> > >
> > > +       pm_runtime_get_sync(chip->dev.parent);
> > >         mutex_lock(&chip->tpm_mutex);
> > >
> > >         rc = chip->ops->send(chip, (u8 *) buf, count);
> > > @@ -394,6 +396,7 @@ out_recv:
> > >                         "tpm_transmit: tpm_recv: error %zd\n", rc);
> > >  out:
> > >         mutex_unlock(&chip->tpm_mutex);
> > > +       pm_runtime_put_sync(chip->dev.parent);
> > >         return rc;
> > >  }
> > >
> > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > > index ca2cad9..71cc7cd 100644
> > > --- a/drivers/char/tpm/tpm_crb.c
> > > +++ b/drivers/char/tpm/tpm_crb.c
> > > @@ -20,6 +20,7 @@
> > >  #include <linux/rculist.h>
> > >  #include <linux/module.h>
> > >  #include <linux/platform_device.h>
> > > +#include <linux/pm_runtime.h>
> > >  #include "tpm.h"
> > >
> > >  #define ACPI_SIG_TPM2 "TPM2"
> > > @@ -41,7 +42,6 @@ enum crb_ca_request {
> > >
> > >  enum crb_ca_status {
> > >         CRB_CA_STS_ERROR        = BIT(0),
> > > -       CRB_CA_STS_TPM_IDLE     = BIT(1),
> > >  };
> > >
> > >  enum crb_start {
> > > @@ -68,6 +68,8 @@ struct crb_control_area {
> > >
> > >  enum crb_status {
> > >         CRB_STS_COMPLETE        = BIT(0),
> > > +       CRB_STS_READY           = BIT(1),
> > > +       CRB_STS_IDLE            = BIT(2),
> > >  };
> > >
> > >  enum crb_flags {
> > > @@ -81,9 +83,52 @@ struct crb_priv {
> > >         struct crb_control_area __iomem *cca;
> > >         u8 __iomem *cmd;
> > >         u8 __iomem *rsp;
> > > +       wait_queue_head_t idle_queue;
> > 
> > 
> > I'm failing to find the code that is calling wake_up_interruptible(idle_queue);
> 
> Ugh, my bad. This actually should not be declared at all. Will remove it
> from the next version and NULL should be passed to wait_for_tpm_stat()
> as the driver does not yet support interrupts (Haswell did not have
> them, not sure about later gens).
> 
> 
> > >  };
> > >
> > > -static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
> > > +static int __maybe_unused crb_runtime_suspend(struct device *dev)
> > > +{
> > > +       struct tpm_chip *chip = dev_get_drvdata(dev);
> > > +       struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> > > +       u32 req;
> > > +
> > > +       if (priv->flags & CRB_FL_ACPI_START)
> > > +               return 0;
> > > +
> > > +       req = ioread32(&priv->cca->req);
> > > +
> > > +       iowrite32(cpu_to_le32(req | CRB_CA_REQ_GO_IDLE), &priv->cca->req);
> > > +
> > > +       if (wait_for_tpm_stat(chip, CRB_STS_IDLE, chip->timeout_c,
> > > +                             &priv->idle_queue, false))
> > > +               dev_warn(&chip->dev, "idle timed out\n");
> > 
> > Unfortunately you cannot do that as there is an HW errata, the status
> > register value might not be defined here during power transition
> > You should poll for request CRB_CA_REQ_GO_IDLE goes to 0 as descried
> > in the spec . only after that you can check for the status register
> > (thought it's maybe not needed)
> 
> And I do exactly what you are asking me to do.
> 
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int __maybe_unused crb_runtime_resume(struct device *dev)
> > 
> > why this is marked unused, why just not compile it out? if the
> > CONFIG_PM is not set?
> 
> It is compiled out if it is unused. Why would you want to trash the code
> with #ifdef cages if they are not necessary? I can add /* CONFIG_PM */
> before the function if that makes it cleaner.
> 
> > > +{
> > > +       struct tpm_chip *chip = dev_get_drvdata(dev);
> > > +       struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> > > +       u32 req;
> > > +
> > > +       if (priv->flags & CRB_FL_ACPI_START)
> > > +               return 0;
> > > +
> > > +       req = ioread32(&priv->cca->req);
> > > +       iowrite32(cpu_to_le32(req | CRB_CA_REQ_CMD_READY), &priv->cca->req);
> > > +
> > > +       if (wait_for_tpm_stat(chip, CRB_STS_READY, chip->timeout_c,
> > > +                             &priv->idle_queue, false))
> > > +               dev_warn(&chip->dev, "wake timed out\n");
> > 
> > Same here,  you should wait for CRB_CA_REQ_CMD_READ to get cleared,
> > only after that it is safe to check the status register.
> 
> It does exactly that. I'm not using CRB status register for anything.
> 
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static const struct dev_pm_ops crb_pm = {
> > > +       SET_RUNTIME_PM_OPS(crb_runtime_suspend, crb_runtime_resume, NULL)
> > > +       SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume)
> > > +};
> > >
> > >  static u8 crb_status(struct tpm_chip *chip)
> > >  {
> > > @@ -94,6 +139,14 @@ static u8 crb_status(struct tpm_chip *chip)
> > >             CRB_START_INVOKE)
> > >                 sts |= CRB_STS_COMPLETE;
> > >
> > > +       if ((ioread32(&priv->cca->req) & CRB_CA_REQ_CMD_READY) !=
> > > +           CRB_CA_REQ_CMD_READY)
> > > +               sts |= CRB_STS_READY;
> > 
> > There is meaning for checking this w/o the actual transition i.e.
> > setting the before
> 
> I'm not sure what you are trying to say.

... but I can undertand why this looks so confusing to you. Maybe it
would be a better idea to completely discard the use of
wait_for_tpm_stat() and changes to crb_status() and do instead the
following in runtime_suspend/resume (example is for resume):

1. Set REQ_CMD_READY.
2. Sleep for TIMEOUT C.
3. Check the register and return -ETIME if it is not cleared.

I think this patch is abusing wait_for_tpm_stat() and it is not really
good fit here. How does this sound?

/Jarkko

Re: [tpmdd-devel] [PATCH 3/3] tpm, tpm_crb: runtime power management

[Jarkko Sakkinen]

On Thu, Jun 16, 2016 at 09:57:35PM +0200, Jarkko Sakkinen wrote:
> Hi Thomas,
> 
> I'm on a vacation this week but I'll give you quick answers :)
> 
> On Tue, Jun 14, 2016 at 04:14:58PM +0300, Tomas Winkler wrote:
> > On Wed, Jun 8, 2016 at 2:02 AM, Jarkko Sakkinen
> > <jarkko.sakkinen@linux.intel.com> wrote:
> > > The register TPM_CRB_CTRL_REQ_0 contains bits goIdle and cmdReady for
> > > invoking the chip to suspend and resume. This commit implements runtime
> > > PM for tpm_crb by using these bits.
> > >
> > > The legacy ACPI start (SMI + DMA) based devices do not support these
> > > bits. Thus this functionality only is enabled only for CRB start (MMIO)
> > > based devices.
> > >
> > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > > ---
> > >  drivers/char/tpm/tpm-interface.c |  3 ++
> > >  drivers/char/tpm/tpm_crb.c       | 62 ++++++++++++++++++++++++++++++++++++++--
> > >  2 files changed, 63 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> > > index 5e3c1b6..3b85648 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"
> > > @@ -350,6 +351,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> > >                 return -E2BIG;
> > >         }
> > >
> > > +       pm_runtime_get_sync(chip->dev.parent);
> > >         mutex_lock(&chip->tpm_mutex);
> > >
> > >         rc = chip->ops->send(chip, (u8 *) buf, count);
> > > @@ -394,6 +396,7 @@ out_recv:
> > >                         "tpm_transmit: tpm_recv: error %zd\n", rc);
> > >  out:
> > >         mutex_unlock(&chip->tpm_mutex);
> > > +       pm_runtime_put_sync(chip->dev.parent);
> > >         return rc;
> > >  }
> > >
> > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > > index ca2cad9..71cc7cd 100644
> > > --- a/drivers/char/tpm/tpm_crb.c
> > > +++ b/drivers/char/tpm/tpm_crb.c
> > > @@ -20,6 +20,7 @@
> > >  #include <linux/rculist.h>
> > >  #include <linux/module.h>
> > >  #include <linux/platform_device.h>
> > > +#include <linux/pm_runtime.h>
> > >  #include "tpm.h"
> > >
> > >  #define ACPI_SIG_TPM2 "TPM2"
> > > @@ -41,7 +42,6 @@ enum crb_ca_request {
> > >
> > >  enum crb_ca_status {
> > >         CRB_CA_STS_ERROR        = BIT(0),
> > > -       CRB_CA_STS_TPM_IDLE     = BIT(1),
> > >  };
> > >
> > >  enum crb_start {
> > > @@ -68,6 +68,8 @@ struct crb_control_area {
> > >
> > >  enum crb_status {
> > >         CRB_STS_COMPLETE        = BIT(0),
> > > +       CRB_STS_READY           = BIT(1),
> > > +       CRB_STS_IDLE            = BIT(2),
> > >  };
> > >
> > >  enum crb_flags {
> > > @@ -81,9 +83,52 @@ struct crb_priv {
> > >         struct crb_control_area __iomem *cca;
> > >         u8 __iomem *cmd;
> > >         u8 __iomem *rsp;
> > > +       wait_queue_head_t idle_queue;
> > 
> > 
> > I'm failing to find the code that is calling wake_up_interruptible(idle_queue);
> 
> Ugh, my bad. This actually should not be declared at all. Will remove it
> from the next version and NULL should be passed to wait_for_tpm_stat()
> as the driver does not yet support interrupts (Haswell did not have
> them, not sure about later gens).
> 
> 
> > >  };
> > >
> > > -static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
> > > +static int __maybe_unused crb_runtime_suspend(struct device *dev)
> > > +{
> > > +       struct tpm_chip *chip = dev_get_drvdata(dev);
> > > +       struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> > > +       u32 req;
> > > +
> > > +       if (priv->flags & CRB_FL_ACPI_START)
> > > +               return 0;
> > > +
> > > +       req = ioread32(&priv->cca->req);
> > > +
> > > +       iowrite32(cpu_to_le32(req | CRB_CA_REQ_GO_IDLE), &priv->cca->req);
> > > +
> > > +       if (wait_for_tpm_stat(chip, CRB_STS_IDLE, chip->timeout_c,
> > > +                             &priv->idle_queue, false))
> > > +               dev_warn(&chip->dev, "idle timed out\n");
> > 
> > Unfortunately you cannot do that as there is an HW errata, the status
> > register value might not be defined here during power transition
> > You should poll for request CRB_CA_REQ_GO_IDLE goes to 0 as descried
> > in the spec . only after that you can check for the status register
> > (thought it's maybe not needed)
> 
> And I do exactly what you are asking me to do.
> 
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static int __maybe_unused crb_runtime_resume(struct device *dev)
> > 
> > why this is marked unused, why just not compile it out? if the
> > CONFIG_PM is not set?
> 
> It is compiled out if it is unused. Why would you want to trash the code
> with #ifdef cages if they are not necessary? I can add /* CONFIG_PM */
> before the function if that makes it cleaner.
> 
> > > +{
> > > +       struct tpm_chip *chip = dev_get_drvdata(dev);
> > > +       struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> > > +       u32 req;
> > > +
> > > +       if (priv->flags & CRB_FL_ACPI_START)
> > > +               return 0;
> > > +
> > > +       req = ioread32(&priv->cca->req);
> > > +       iowrite32(cpu_to_le32(req | CRB_CA_REQ_CMD_READY), &priv->cca->req);
> > > +
> > > +       if (wait_for_tpm_stat(chip, CRB_STS_READY, chip->timeout_c,
> > > +                             &priv->idle_queue, false))
> > > +               dev_warn(&chip->dev, "wake timed out\n");
> > 
> > Same here,  you should wait for CRB_CA_REQ_CMD_READ to get cleared,
> > only after that it is safe to check the status register.
> 
> It does exactly that. I'm not using CRB status register for anything.
> 
> > > +
> > > +       return 0;
> > > +}
> > > +
> > > +static const struct dev_pm_ops crb_pm = {
> > > +       SET_RUNTIME_PM_OPS(crb_runtime_suspend, crb_runtime_resume, NULL)
> > > +       SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume)
> > > +};
> > >
> > >  static u8 crb_status(struct tpm_chip *chip)
> > >  {
> > > @@ -94,6 +139,14 @@ static u8 crb_status(struct tpm_chip *chip)
> > >             CRB_START_INVOKE)
> > >                 sts |= CRB_STS_COMPLETE;
> > >
> > > +       if ((ioread32(&priv->cca->req) & CRB_CA_REQ_CMD_READY) !=
> > > +           CRB_CA_REQ_CMD_READY)
> > > +               sts |= CRB_STS_READY;
> > 
> > There is meaning for checking this w/o the actual transition i.e.
> > setting the before
> 
> I'm not sure what you are trying to say.

... but I can undertand why this looks so confusing to you. Maybe it
would be a better idea to completely discard the use of
wait_for_tpm_stat() and changes to crb_status() and do instead the
following in runtime_suspend/resume (example is for resume):

1. Set REQ_CMD_READY.
2. Sleep for TIMEOUT C.
3. Check the register and return -ETIME if it is not cleared.

I think this patch is abusing wait_for_tpm_stat() and it is not really
good fit here. How does this sound?

/Jarkko

Re: [tpmdd-devel] [PATCH 3/3] tpm, tpm_crb: runtime power management

[Tomas Winkler]

On Thu, Jun 16, 2016 at 10:57 PM, Jarkko Sakkinen
<jarkko.sakkinen@linux.intel.com> wrote:
> Hi Thomas,
>
> I'm on a vacation this week but I'll give you quick answers :)
>
> On Tue, Jun 14, 2016 at 04:14:58PM +0300, Tomas Winkler wrote:
>> On Wed, Jun 8, 2016 at 2:02 AM, Jarkko Sakkinen
>> <jarkko.sakkinen@linux.intel.com> wrote:
>> > The register TPM_CRB_CTRL_REQ_0 contains bits goIdle and cmdReady for
>> > invoking the chip to suspend and resume. This commit implements runtime
>> > PM for tpm_crb by using these bits.
>> >
>> > The legacy ACPI start (SMI + DMA) based devices do not support these
>> > bits. Thus this functionality only is enabled only for CRB start (MMIO)
>> > based devices.
>> >
>> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
>> > ---
>> >  drivers/char/tpm/tpm-interface.c |  3 ++
>> >  drivers/char/tpm/tpm_crb.c       | 62 ++++++++++++++++++++++++++++++++++++++--
>> >  2 files changed, 63 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> > index 5e3c1b6..3b85648 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"
>> > @@ -350,6 +351,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
>> >                 return -E2BIG;
>> >         }
>> >
>> > +       pm_runtime_get_sync(chip->dev.parent);
>> >         mutex_lock(&chip->tpm_mutex);
>> >
>> >         rc = chip->ops->send(chip, (u8 *) buf, count);
>> > @@ -394,6 +396,7 @@ out_recv:
>> >                         "tpm_transmit: tpm_recv: error %zd\n", rc);
>> >  out:
>> >         mutex_unlock(&chip->tpm_mutex);
>> > +       pm_runtime_put_sync(chip->dev.parent);
>> >         return rc;
>> >  }
>> >
>> > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
>> > index ca2cad9..71cc7cd 100644
>> > --- a/drivers/char/tpm/tpm_crb.c
>> > +++ b/drivers/char/tpm/tpm_crb.c
>> > @@ -20,6 +20,7 @@
>> >  #include <linux/rculist.h>
>> >  #include <linux/module.h>
>> >  #include <linux/platform_device.h>
>> > +#include <linux/pm_runtime.h>
>> >  #include "tpm.h"
>> >
>> >  #define ACPI_SIG_TPM2 "TPM2"
>> > @@ -41,7 +42,6 @@ enum crb_ca_request {
>> >
>> >  enum crb_ca_status {
>> >         CRB_CA_STS_ERROR        = BIT(0),
>> > -       CRB_CA_STS_TPM_IDLE     = BIT(1),
>> >  };
>> >
>> >  enum crb_start {
>> > @@ -68,6 +68,8 @@ struct crb_control_area {
>> >
>> >  enum crb_status {
>> >         CRB_STS_COMPLETE        = BIT(0),
>> > +       CRB_STS_READY           = BIT(1),
>> > +       CRB_STS_IDLE            = BIT(2),
>> >  };
>> >
>> >  enum crb_flags {
>> > @@ -81,9 +83,52 @@ struct crb_priv {
>> >         struct crb_control_area __iomem *cca;
>> >         u8 __iomem *cmd;
>> >         u8 __iomem *rsp;
>> > +       wait_queue_head_t idle_queue;
>>
>>
>> I'm failing to find the code that is calling wake_up_interruptible(idle_queue);
>
> Ugh, my bad. This actually should not be declared at all. Will remove it
> from the next version and NULL should be passed to wait_for_tpm_stat()
> as the driver does not yet support interrupts (Haswell did not have
> them, not sure about later gens).
>
>
>> >  };
>> >
>> > -static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
>> > +static int __maybe_unused crb_runtime_suspend(struct device *dev)
>> > +{
>> > +       struct tpm_chip *chip = dev_get_drvdata(dev);
>> > +       struct crb_priv *priv = dev_get_drvdata(&chip->dev);
>> > +       u32 req;
>> > +
>> > +       if (priv->flags & CRB_FL_ACPI_START)
>> > +               return 0;
>> > +
>> > +       req = ioread32(&priv->cca->req);
>> > +
>> > +       iowrite32(cpu_to_le32(req | CRB_CA_REQ_GO_IDLE), &priv->cca->req);
>> > +
>> > +       if (wait_for_tpm_stat(chip, CRB_STS_IDLE, chip->timeout_c,
>> > +                             &priv->idle_queue, false))
>> > +               dev_warn(&chip->dev, "idle timed out\n");
>>
>> Unfortunately you cannot do that as there is an HW errata, the status
>> register value might not be defined here during power transition
>> You should poll for request CRB_CA_REQ_GO_IDLE goes to 0 as descried
>> in the spec . only after that you can check for the status register
>> (thought it's maybe not needed)
>
> And I do exactly what you are asking me to do.
>
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static int __maybe_unused crb_runtime_resume(struct device *dev)
>>
>> why this is marked unused, why just not compile it out? if the
>> CONFIG_PM is not set?
>
> It is compiled out if it is unused. Why would you want to trash the code
> with #ifdef cages if they are not necessary? I can add /* CONFIG_PM */
> before the function if that makes it cleaner.

I'm not sure about that, I believe it  just suppresses warnings.
You will need something --gc-sessions int the linker, I'm not sure
this is used by kernel.
>
>> > +{
>> > +       struct tpm_chip *chip = dev_get_drvdata(dev);
>> > +       struct crb_priv *priv = dev_get_drvdata(&chip->dev);
>> > +       u32 req;
>> > +
>> > +       if (priv->flags & CRB_FL_ACPI_START)
>> > +               return 0;
>> > +
>> > +       req = ioread32(&priv->cca->req);
>> > +       iowrite32(cpu_to_le32(req | CRB_CA_REQ_CMD_READY), &priv->cca->req);
>> > +
>> > +       if (wait_for_tpm_stat(chip, CRB_STS_READY, chip->timeout_c,
>> > +                             &priv->idle_queue, false))
>> > +               dev_warn(&chip->dev, "wake timed out\n");
>>
>> Same here,  you should wait for CRB_CA_REQ_CMD_READ to get cleared,
>> only after that it is safe to check the status register.
>
> It does exactly that. I'm not using CRB status register for anything.
>
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static const struct dev_pm_ops crb_pm = {
>> > +       SET_RUNTIME_PM_OPS(crb_runtime_suspend, crb_runtime_resume, NULL)
>> > +       SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume)
>> > +};
>> >
>> >  static u8 crb_status(struct tpm_chip *chip)
>> >  {
>> > @@ -94,6 +139,14 @@ static u8 crb_status(struct tpm_chip *chip)
>> >             CRB_START_INVOKE)
>> >                 sts |= CRB_STS_COMPLETE;
>> >
>> > +       if ((ioread32(&priv->cca->req) & CRB_CA_REQ_CMD_READY) !=
>> > +           CRB_CA_REQ_CMD_READY)
>> > +               sts |= CRB_STS_READY;
>>
>> There is meaning for checking this w/o the actual transition i.e.
>> setting the before
>
> I'm not sure what you are trying to say.

Sorry, my bad, it should be: There is NO meaning for checking
CRB_CA_REQ_CMD_READY is 0  if this wasn't asserted before, In
interrupt language  it's not edge sensitive not level sensitive

Tomas

Re: [PATCH 3/3] tpm, tpm_crb: runtime power management

[Tomas Winkler]

On Thu, Jun 16, 2016 at 10:57 PM, Jarkko Sakkinen
<jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
> Hi Thomas,
>
> I'm on a vacation this week but I'll give you quick answers :)
>
> On Tue, Jun 14, 2016 at 04:14:58PM +0300, Tomas Winkler wrote:
>> On Wed, Jun 8, 2016 at 2:02 AM, Jarkko Sakkinen
>> <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
>> > The register TPM_CRB_CTRL_REQ_0 contains bits goIdle and cmdReady for
>> > invoking the chip to suspend and resume. This commit implements runtime
>> > PM for tpm_crb by using these bits.
>> >
>> > The legacy ACPI start (SMI + DMA) based devices do not support these
>> > bits. Thus this functionality only is enabled only for CRB start (MMIO)
>> > based devices.
>> >
>> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
>> > ---
>> >  drivers/char/tpm/tpm-interface.c |  3 ++
>> >  drivers/char/tpm/tpm_crb.c       | 62 ++++++++++++++++++++++++++++++++++++++--
>> >  2 files changed, 63 insertions(+), 2 deletions(-)
>> >
>> > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
>> > index 5e3c1b6..3b85648 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"
>> > @@ -350,6 +351,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
>> >                 return -E2BIG;
>> >         }
>> >
>> > +       pm_runtime_get_sync(chip->dev.parent);
>> >         mutex_lock(&chip->tpm_mutex);
>> >
>> >         rc = chip->ops->send(chip, (u8 *) buf, count);
>> > @@ -394,6 +396,7 @@ out_recv:
>> >                         "tpm_transmit: tpm_recv: error %zd\n", rc);
>> >  out:
>> >         mutex_unlock(&chip->tpm_mutex);
>> > +       pm_runtime_put_sync(chip->dev.parent);
>> >         return rc;
>> >  }
>> >
>> > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
>> > index ca2cad9..71cc7cd 100644
>> > --- a/drivers/char/tpm/tpm_crb.c
>> > +++ b/drivers/char/tpm/tpm_crb.c
>> > @@ -20,6 +20,7 @@
>> >  #include <linux/rculist.h>
>> >  #include <linux/module.h>
>> >  #include <linux/platform_device.h>
>> > +#include <linux/pm_runtime.h>
>> >  #include "tpm.h"
>> >
>> >  #define ACPI_SIG_TPM2 "TPM2"
>> > @@ -41,7 +42,6 @@ enum crb_ca_request {
>> >
>> >  enum crb_ca_status {
>> >         CRB_CA_STS_ERROR        = BIT(0),
>> > -       CRB_CA_STS_TPM_IDLE     = BIT(1),
>> >  };
>> >
>> >  enum crb_start {
>> > @@ -68,6 +68,8 @@ struct crb_control_area {
>> >
>> >  enum crb_status {
>> >         CRB_STS_COMPLETE        = BIT(0),
>> > +       CRB_STS_READY           = BIT(1),
>> > +       CRB_STS_IDLE            = BIT(2),
>> >  };
>> >
>> >  enum crb_flags {
>> > @@ -81,9 +83,52 @@ struct crb_priv {
>> >         struct crb_control_area __iomem *cca;
>> >         u8 __iomem *cmd;
>> >         u8 __iomem *rsp;
>> > +       wait_queue_head_t idle_queue;
>>
>>
>> I'm failing to find the code that is calling wake_up_interruptible(idle_queue);
>
> Ugh, my bad. This actually should not be declared at all. Will remove it
> from the next version and NULL should be passed to wait_for_tpm_stat()
> as the driver does not yet support interrupts (Haswell did not have
> them, not sure about later gens).
>
>
>> >  };
>> >
>> > -static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
>> > +static int __maybe_unused crb_runtime_suspend(struct device *dev)
>> > +{
>> > +       struct tpm_chip *chip = dev_get_drvdata(dev);
>> > +       struct crb_priv *priv = dev_get_drvdata(&chip->dev);
>> > +       u32 req;
>> > +
>> > +       if (priv->flags & CRB_FL_ACPI_START)
>> > +               return 0;
>> > +
>> > +       req = ioread32(&priv->cca->req);
>> > +
>> > +       iowrite32(cpu_to_le32(req | CRB_CA_REQ_GO_IDLE), &priv->cca->req);
>> > +
>> > +       if (wait_for_tpm_stat(chip, CRB_STS_IDLE, chip->timeout_c,
>> > +                             &priv->idle_queue, false))
>> > +               dev_warn(&chip->dev, "idle timed out\n");
>>
>> Unfortunately you cannot do that as there is an HW errata, the status
>> register value might not be defined here during power transition
>> You should poll for request CRB_CA_REQ_GO_IDLE goes to 0 as descried
>> in the spec . only after that you can check for the status register
>> (thought it's maybe not needed)
>
> And I do exactly what you are asking me to do.
>
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static int __maybe_unused crb_runtime_resume(struct device *dev)
>>
>> why this is marked unused, why just not compile it out? if the
>> CONFIG_PM is not set?
>
> It is compiled out if it is unused. Why would you want to trash the code
> with #ifdef cages if they are not necessary? I can add /* CONFIG_PM */
> before the function if that makes it cleaner.

I'm not sure about that, I believe it  just suppresses warnings.
You will need something --gc-sessions int the linker, I'm not sure
this is used by kernel.
>
>> > +{
>> > +       struct tpm_chip *chip = dev_get_drvdata(dev);
>> > +       struct crb_priv *priv = dev_get_drvdata(&chip->dev);
>> > +       u32 req;
>> > +
>> > +       if (priv->flags & CRB_FL_ACPI_START)
>> > +               return 0;
>> > +
>> > +       req = ioread32(&priv->cca->req);
>> > +       iowrite32(cpu_to_le32(req | CRB_CA_REQ_CMD_READY), &priv->cca->req);
>> > +
>> > +       if (wait_for_tpm_stat(chip, CRB_STS_READY, chip->timeout_c,
>> > +                             &priv->idle_queue, false))
>> > +               dev_warn(&chip->dev, "wake timed out\n");
>>
>> Same here,  you should wait for CRB_CA_REQ_CMD_READ to get cleared,
>> only after that it is safe to check the status register.
>
> It does exactly that. I'm not using CRB status register for anything.
>
>> > +
>> > +       return 0;
>> > +}
>> > +
>> > +static const struct dev_pm_ops crb_pm = {
>> > +       SET_RUNTIME_PM_OPS(crb_runtime_suspend, crb_runtime_resume, NULL)
>> > +       SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume)
>> > +};
>> >
>> >  static u8 crb_status(struct tpm_chip *chip)
>> >  {
>> > @@ -94,6 +139,14 @@ static u8 crb_status(struct tpm_chip *chip)
>> >             CRB_START_INVOKE)
>> >                 sts |= CRB_STS_COMPLETE;
>> >
>> > +       if ((ioread32(&priv->cca->req) & CRB_CA_REQ_CMD_READY) !=
>> > +           CRB_CA_REQ_CMD_READY)
>> > +               sts |= CRB_STS_READY;
>>
>> There is meaning for checking this w/o the actual transition i.e.
>> setting the before
>
> I'm not sure what you are trying to say.

Sorry, my bad, it should be: There is NO meaning for checking
CRB_CA_REQ_CMD_READY is 0  if this wasn't asserted before, In
interrupt language  it's not edge sensitive not level sensitive

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/zohomanageengine

Re: [tpmdd-devel] [PATCH 3/3] tpm, tpm_crb: runtime power management

[Jason Gunthorpe]

On Thu, Jun 16, 2016 at 11:26:48PM +0300, Tomas Winkler wrote:

> > It is compiled out if it is unused. Why would you want to trash the code
> > with #ifdef cages if they are not necessary? I can add /* CONFIG_PM */
> > before the function if that makes it cleaner.
> 
> I'm not sure about that, I believe it  just suppresses warnings.
> You will need something --gc-sessions int the linker, I'm not sure
> this is used by kernel.

No, it is fine. The compiler drops unused static functions, that is
what the attribute is for. It always does this, so supressing the
warning is the desire behavior. The kernel preference is to avoid
ifdefs and always compile all the code to avoid problems with config
option combinations.

--gc-sections isn't useful unless -ffunction-section is used, which the 
kernel doesn't do. Fortunately the dead code is removed by the
compiler, not the linker.

Jason

Re: [PATCH 3/3] tpm, tpm_crb: runtime power management

[Jason Gunthorpe]

On Thu, Jun 16, 2016 at 11:26:48PM +0300, Tomas Winkler wrote:

> > It is compiled out if it is unused. Why would you want to trash the code
> > with #ifdef cages if they are not necessary? I can add /* CONFIG_PM */
> > before the function if that makes it cleaner.
> 
> I'm not sure about that, I believe it  just suppresses warnings.
> You will need something --gc-sessions int the linker, I'm not sure
> this is used by kernel.

No, it is fine. The compiler drops unused static functions, that is
what the attribute is for. It always does this, so supressing the
warning is the desire behavior. The kernel preference is to avoid
ifdefs and always compile all the code to avoid problems with config
option combinations.

--gc-sections isn't useful unless -ffunction-section is used, which the 
kernel doesn't do. Fortunately the dead code is removed by the
compiler, not the linker.

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/zohomanageengine

Re: [tpmdd-devel] [PATCH 3/3] tpm, tpm_crb: runtime power management

[Jarkko Sakkinen]

On Thu, Jun 16, 2016 at 11:26:48PM +0300, Tomas Winkler wrote:
> On Thu, Jun 16, 2016 at 10:57 PM, Jarkko Sakkinen
> <jarkko.sakkinen@linux.intel.com> wrote:
> > Hi Thomas,
> >
> > I'm on a vacation this week but I'll give you quick answers :)
> >
> > On Tue, Jun 14, 2016 at 04:14:58PM +0300, Tomas Winkler wrote:
> >> On Wed, Jun 8, 2016 at 2:02 AM, Jarkko Sakkinen
> >> <jarkko.sakkinen@linux.intel.com> wrote:
> >> > The register TPM_CRB_CTRL_REQ_0 contains bits goIdle and cmdReady for
> >> > invoking the chip to suspend and resume. This commit implements runtime
> >> > PM for tpm_crb by using these bits.
> >> >
> >> > The legacy ACPI start (SMI + DMA) based devices do not support these
> >> > bits. Thus this functionality only is enabled only for CRB start (MMIO)
> >> > based devices.
> >> >
> >> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> >> > ---
> >> >  drivers/char/tpm/tpm-interface.c |  3 ++
> >> >  drivers/char/tpm/tpm_crb.c       | 62 ++++++++++++++++++++++++++++++++++++++--
> >> >  2 files changed, 63 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> >> > index 5e3c1b6..3b85648 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"
> >> > @@ -350,6 +351,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> >> >                 return -E2BIG;
> >> >         }
> >> >
> >> > +       pm_runtime_get_sync(chip->dev.parent);
> >> >         mutex_lock(&chip->tpm_mutex);
> >> >
> >> >         rc = chip->ops->send(chip, (u8 *) buf, count);
> >> > @@ -394,6 +396,7 @@ out_recv:
> >> >                         "tpm_transmit: tpm_recv: error %zd\n", rc);
> >> >  out:
> >> >         mutex_unlock(&chip->tpm_mutex);
> >> > +       pm_runtime_put_sync(chip->dev.parent);
> >> >         return rc;
> >> >  }
> >> >
> >> > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> >> > index ca2cad9..71cc7cd 100644
> >> > --- a/drivers/char/tpm/tpm_crb.c
> >> > +++ b/drivers/char/tpm/tpm_crb.c
> >> > @@ -20,6 +20,7 @@
> >> >  #include <linux/rculist.h>
> >> >  #include <linux/module.h>
> >> >  #include <linux/platform_device.h>
> >> > +#include <linux/pm_runtime.h>
> >> >  #include "tpm.h"
> >> >
> >> >  #define ACPI_SIG_TPM2 "TPM2"
> >> > @@ -41,7 +42,6 @@ enum crb_ca_request {
> >> >
> >> >  enum crb_ca_status {
> >> >         CRB_CA_STS_ERROR        = BIT(0),
> >> > -       CRB_CA_STS_TPM_IDLE     = BIT(1),
> >> >  };
> >> >
> >> >  enum crb_start {
> >> > @@ -68,6 +68,8 @@ struct crb_control_area {
> >> >
> >> >  enum crb_status {
> >> >         CRB_STS_COMPLETE        = BIT(0),
> >> > +       CRB_STS_READY           = BIT(1),
> >> > +       CRB_STS_IDLE            = BIT(2),
> >> >  };
> >> >
> >> >  enum crb_flags {
> >> > @@ -81,9 +83,52 @@ struct crb_priv {
> >> >         struct crb_control_area __iomem *cca;
> >> >         u8 __iomem *cmd;
> >> >         u8 __iomem *rsp;
> >> > +       wait_queue_head_t idle_queue;
> >>
> >>
> >> I'm failing to find the code that is calling wake_up_interruptible(idle_queue);
> >
> > Ugh, my bad. This actually should not be declared at all. Will remove it
> > from the next version and NULL should be passed to wait_for_tpm_stat()
> > as the driver does not yet support interrupts (Haswell did not have
> > them, not sure about later gens).
> >
> >
> >> >  };
> >> >
> >> > -static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
> >> > +static int __maybe_unused crb_runtime_suspend(struct device *dev)
> >> > +{
> >> > +       struct tpm_chip *chip = dev_get_drvdata(dev);
> >> > +       struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> >> > +       u32 req;
> >> > +
> >> > +       if (priv->flags & CRB_FL_ACPI_START)
> >> > +               return 0;
> >> > +
> >> > +       req = ioread32(&priv->cca->req);
> >> > +
> >> > +       iowrite32(cpu_to_le32(req | CRB_CA_REQ_GO_IDLE), &priv->cca->req);
> >> > +
> >> > +       if (wait_for_tpm_stat(chip, CRB_STS_IDLE, chip->timeout_c,
> >> > +                             &priv->idle_queue, false))
> >> > +               dev_warn(&chip->dev, "idle timed out\n");
> >>
> >> Unfortunately you cannot do that as there is an HW errata, the status
> >> register value might not be defined here during power transition
> >> You should poll for request CRB_CA_REQ_GO_IDLE goes to 0 as descried
> >> in the spec . only after that you can check for the status register
> >> (thought it's maybe not needed)
> >
> > And I do exactly what you are asking me to do.
> >
> >> > +
> >> > +       return 0;
> >> > +}
> >> > +
> >> > +static int __maybe_unused crb_runtime_resume(struct device *dev)
> >>
> >> why this is marked unused, why just not compile it out? if the
> >> CONFIG_PM is not set?
> >
> > It is compiled out if it is unused. Why would you want to trash the code
> > with #ifdef cages if they are not necessary? I can add /* CONFIG_PM */
> > before the function if that makes it cleaner.
> 
> I'm not sure about that, I believe it  just suppresses warnings.
> You will need something --gc-sessions int the linker, I'm not sure
> this is used by kernel.

It is used in lot of places. git grep gave me 1482 matches.

> >> > +{
> >> > +       struct tpm_chip *chip = dev_get_drvdata(dev);
> >> > +       struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> >> > +       u32 req;
> >> > +
> >> > +       if (priv->flags & CRB_FL_ACPI_START)
> >> > +               return 0;
> >> > +
> >> > +       req = ioread32(&priv->cca->req);
> >> > +       iowrite32(cpu_to_le32(req | CRB_CA_REQ_CMD_READY), &priv->cca->req);
> >> > +
> >> > +       if (wait_for_tpm_stat(chip, CRB_STS_READY, chip->timeout_c,
> >> > +                             &priv->idle_queue, false))
> >> > +               dev_warn(&chip->dev, "wake timed out\n");
> >>
> >> Same here,  you should wait for CRB_CA_REQ_CMD_READ to get cleared,
> >> only after that it is safe to check the status register.
> >
> > It does exactly that. I'm not using CRB status register for anything.
> >
> >> > +
> >> > +       return 0;
> >> > +}
> >> > +
> >> > +static const struct dev_pm_ops crb_pm = {
> >> > +       SET_RUNTIME_PM_OPS(crb_runtime_suspend, crb_runtime_resume, NULL)
> >> > +       SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume)
> >> > +};
> >> >
> >> >  static u8 crb_status(struct tpm_chip *chip)
> >> >  {
> >> > @@ -94,6 +139,14 @@ static u8 crb_status(struct tpm_chip *chip)
> >> >             CRB_START_INVOKE)
> >> >                 sts |= CRB_STS_COMPLETE;
> >> >
> >> > +       if ((ioread32(&priv->cca->req) & CRB_CA_REQ_CMD_READY) !=
> >> > +           CRB_CA_REQ_CMD_READY)
> >> > +               sts |= CRB_STS_READY;
> >>
> >> There is meaning for checking this w/o the actual transition i.e.
> >> setting the before
> >
> > I'm not sure what you are trying to say.
> 
> Sorry, my bad, it should be: There is NO meaning for checking
> CRB_CA_REQ_CMD_READY is 0  if this wasn't asserted before, In
> interrupt language  it's not edge sensitive not level sensitive

Got you but it should work because I take advatange of this in the case
I first set it. Anyway, this approach that I chose is crap and I'll
revise the whole patch in a way that I explained in my follow-up reply
:)

> Tomas

/Jarkko

Re: [PATCH 3/3] tpm, tpm_crb: runtime power management

[Jarkko Sakkinen]

On Thu, Jun 16, 2016 at 11:26:48PM +0300, Tomas Winkler wrote:
> On Thu, Jun 16, 2016 at 10:57 PM, Jarkko Sakkinen
> <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
> > Hi Thomas,
> >
> > I'm on a vacation this week but I'll give you quick answers :)
> >
> > On Tue, Jun 14, 2016 at 04:14:58PM +0300, Tomas Winkler wrote:
> >> On Wed, Jun 8, 2016 at 2:02 AM, Jarkko Sakkinen
> >> <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org> wrote:
> >> > The register TPM_CRB_CTRL_REQ_0 contains bits goIdle and cmdReady for
> >> > invoking the chip to suspend and resume. This commit implements runtime
> >> > PM for tpm_crb by using these bits.
> >> >
> >> > The legacy ACPI start (SMI + DMA) based devices do not support these
> >> > bits. Thus this functionality only is enabled only for CRB start (MMIO)
> >> > based devices.
> >> >
> >> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> >> > ---
> >> >  drivers/char/tpm/tpm-interface.c |  3 ++
> >> >  drivers/char/tpm/tpm_crb.c       | 62 ++++++++++++++++++++++++++++++++++++++--
> >> >  2 files changed, 63 insertions(+), 2 deletions(-)
> >> >
> >> > diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c
> >> > index 5e3c1b6..3b85648 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"
> >> > @@ -350,6 +351,7 @@ ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf,
> >> >                 return -E2BIG;
> >> >         }
> >> >
> >> > +       pm_runtime_get_sync(chip->dev.parent);
> >> >         mutex_lock(&chip->tpm_mutex);
> >> >
> >> >         rc = chip->ops->send(chip, (u8 *) buf, count);
> >> > @@ -394,6 +396,7 @@ out_recv:
> >> >                         "tpm_transmit: tpm_recv: error %zd\n", rc);
> >> >  out:
> >> >         mutex_unlock(&chip->tpm_mutex);
> >> > +       pm_runtime_put_sync(chip->dev.parent);
> >> >         return rc;
> >> >  }
> >> >
> >> > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> >> > index ca2cad9..71cc7cd 100644
> >> > --- a/drivers/char/tpm/tpm_crb.c
> >> > +++ b/drivers/char/tpm/tpm_crb.c
> >> > @@ -20,6 +20,7 @@
> >> >  #include <linux/rculist.h>
> >> >  #include <linux/module.h>
> >> >  #include <linux/platform_device.h>
> >> > +#include <linux/pm_runtime.h>
> >> >  #include "tpm.h"
> >> >
> >> >  #define ACPI_SIG_TPM2 "TPM2"
> >> > @@ -41,7 +42,6 @@ enum crb_ca_request {
> >> >
> >> >  enum crb_ca_status {
> >> >         CRB_CA_STS_ERROR        = BIT(0),
> >> > -       CRB_CA_STS_TPM_IDLE     = BIT(1),
> >> >  };
> >> >
> >> >  enum crb_start {
> >> > @@ -68,6 +68,8 @@ struct crb_control_area {
> >> >
> >> >  enum crb_status {
> >> >         CRB_STS_COMPLETE        = BIT(0),
> >> > +       CRB_STS_READY           = BIT(1),
> >> > +       CRB_STS_IDLE            = BIT(2),
> >> >  };
> >> >
> >> >  enum crb_flags {
> >> > @@ -81,9 +83,52 @@ struct crb_priv {
> >> >         struct crb_control_area __iomem *cca;
> >> >         u8 __iomem *cmd;
> >> >         u8 __iomem *rsp;
> >> > +       wait_queue_head_t idle_queue;
> >>
> >>
> >> I'm failing to find the code that is calling wake_up_interruptible(idle_queue);
> >
> > Ugh, my bad. This actually should not be declared at all. Will remove it
> > from the next version and NULL should be passed to wait_for_tpm_stat()
> > as the driver does not yet support interrupts (Haswell did not have
> > them, not sure about later gens).
> >
> >
> >> >  };
> >> >
> >> > -static SIMPLE_DEV_PM_OPS(crb_pm, tpm_pm_suspend, tpm_pm_resume);
> >> > +static int __maybe_unused crb_runtime_suspend(struct device *dev)
> >> > +{
> >> > +       struct tpm_chip *chip = dev_get_drvdata(dev);
> >> > +       struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> >> > +       u32 req;
> >> > +
> >> > +       if (priv->flags & CRB_FL_ACPI_START)
> >> > +               return 0;
> >> > +
> >> > +       req = ioread32(&priv->cca->req);
> >> > +
> >> > +       iowrite32(cpu_to_le32(req | CRB_CA_REQ_GO_IDLE), &priv->cca->req);
> >> > +
> >> > +       if (wait_for_tpm_stat(chip, CRB_STS_IDLE, chip->timeout_c,
> >> > +                             &priv->idle_queue, false))
> >> > +               dev_warn(&chip->dev, "idle timed out\n");
> >>
> >> Unfortunately you cannot do that as there is an HW errata, the status
> >> register value might not be defined here during power transition
> >> You should poll for request CRB_CA_REQ_GO_IDLE goes to 0 as descried
> >> in the spec . only after that you can check for the status register
> >> (thought it's maybe not needed)
> >
> > And I do exactly what you are asking me to do.
> >
> >> > +
> >> > +       return 0;
> >> > +}
> >> > +
> >> > +static int __maybe_unused crb_runtime_resume(struct device *dev)
> >>
> >> why this is marked unused, why just not compile it out? if the
> >> CONFIG_PM is not set?
> >
> > It is compiled out if it is unused. Why would you want to trash the code
> > with #ifdef cages if they are not necessary? I can add /* CONFIG_PM */
> > before the function if that makes it cleaner.
> 
> I'm not sure about that, I believe it  just suppresses warnings.
> You will need something --gc-sessions int the linker, I'm not sure
> this is used by kernel.

It is used in lot of places. git grep gave me 1482 matches.

> >> > +{
> >> > +       struct tpm_chip *chip = dev_get_drvdata(dev);
> >> > +       struct crb_priv *priv = dev_get_drvdata(&chip->dev);
> >> > +       u32 req;
> >> > +
> >> > +       if (priv->flags & CRB_FL_ACPI_START)
> >> > +               return 0;
> >> > +
> >> > +       req = ioread32(&priv->cca->req);
> >> > +       iowrite32(cpu_to_le32(req | CRB_CA_REQ_CMD_READY), &priv->cca->req);
> >> > +
> >> > +       if (wait_for_tpm_stat(chip, CRB_STS_READY, chip->timeout_c,
> >> > +                             &priv->idle_queue, false))
> >> > +               dev_warn(&chip->dev, "wake timed out\n");
> >>
> >> Same here,  you should wait for CRB_CA_REQ_CMD_READ to get cleared,
> >> only after that it is safe to check the status register.
> >
> > It does exactly that. I'm not using CRB status register for anything.
> >
> >> > +
> >> > +       return 0;
> >> > +}
> >> > +
> >> > +static const struct dev_pm_ops crb_pm = {
> >> > +       SET_RUNTIME_PM_OPS(crb_runtime_suspend, crb_runtime_resume, NULL)
> >> > +       SET_SYSTEM_SLEEP_PM_OPS(tpm_pm_suspend, tpm_pm_resume)
> >> > +};
> >> >
> >> >  static u8 crb_status(struct tpm_chip *chip)
> >> >  {
> >> > @@ -94,6 +139,14 @@ static u8 crb_status(struct tpm_chip *chip)
> >> >             CRB_START_INVOKE)
> >> >                 sts |= CRB_STS_COMPLETE;
> >> >
> >> > +       if ((ioread32(&priv->cca->req) & CRB_CA_REQ_CMD_READY) !=
> >> > +           CRB_CA_REQ_CMD_READY)
> >> > +               sts |= CRB_STS_READY;
> >>
> >> There is meaning for checking this w/o the actual transition i.e.
> >> setting the before
> >
> > I'm not sure what you are trying to say.
> 
> Sorry, my bad, it should be: There is NO meaning for checking
> CRB_CA_REQ_CMD_READY is 0  if this wasn't asserted before, In
> interrupt language  it's not edge sensitive not level sensitive

Got you but it should work because I take advatange of this in the case
I first set it. Anyway, this approach that I chose is crap and I'll
revise the whole patch in a way that I explained in my follow-up reply
:)

> Tomas

/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/zohomanageengine