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

[PATCH v3 0/4] 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.
V3. fix lower case corruption in the first patch

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 v3 0/4] 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.
V3. fix lower case corruption in the first patch

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 v3 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@linux.intel.com>
oringal patch:
'tpm_crb: implement power tpm crb power management'


Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
---
V2: do not export the functions via tpm ops
V3: fix lower case corruption; adjust function documentation

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

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 6e9d1bca712f..b6923a8b3ff7 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -83,6 +83,75 @@ struct crb_priv {
 	u32 cmd_size;
 };
 
+/**
+ * crb_go_idle - request tpm crb device to go the idle state
+ *
+ * @dev:  crb device
+ * @priv: crb private data
+ *
+ * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ
+ * The device should respond within TIMEOUT_C by clearing the bit.
+ * Anyhow, we do not wait here as a consequent CMD_READY request
+ * will be handled correctly even if idle was not completed.
+ *
+ * The function does nothing for devices with ACPI-start method.
+ *
+ * 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 - request tpm crb device to enter ready state
+ *
+ * @dev:  crb device
+ * @priv: crb private data
+ *
+ * Write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ
+ * and poll till the device acknowledge it by clearing the bit.
+ * The device should respond within TIMEOUT_C.
+ *
+ * The function does nothing for devices with ACPI-start method
+ *
+ * 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 v3 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
V3: fix lower case corruption; adjust function documentation

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

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 6e9d1bca712f..b6923a8b3ff7 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -83,6 +83,75 @@ struct crb_priv {
 	u32 cmd_size;
 };
 
+/**
+ * crb_go_idle - request tpm crb device to go the idle state
+ *
+ * @dev:  crb device
+ * @priv: crb private data
+ *
+ * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ
+ * The device should respond within TIMEOUT_C by clearing the bit.
+ * Anyhow, we do not wait here as a consequent CMD_READY request
+ * will be handled correctly even if idle was not completed.
+ *
+ * The function does nothing for devices with ACPI-start method.
+ *
+ * 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 - request tpm crb device to enter ready state
+ *
+ * @dev:  crb device
+ * @priv: crb private data
+ *
+ * Write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ
+ * and poll till the device acknowledge it by clearing the bit.
+ * The device should respond within TIMEOUT_C.
+ *
+ * The function does nothing for devices with ACPI-start method
+ *
+ * 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 v3 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@intel.com>
---
V2: cmd read need to be called also before crb_init as this will run
 self test.
V3: resend.

 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 b6923a8b3ff7..e945177cf2c8 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -318,6 +318,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;
@@ -345,12 +346,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);
@@ -358,7 +374,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
@@ -366,12 +383,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)
@@ -415,7 +438,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 v3 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.
V3: resend.

 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 b6923a8b3ff7..e945177cf2c8 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -318,6 +318,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;
@@ -345,12 +346,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);
@@ -358,7 +374,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
@@ -366,12 +383,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)
@@ -415,7 +438,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 v3 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@intel.com>
---
V2: new in the series
V3: resend
 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 e945177cf2c8..b0c0e2c9022b 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -265,21 +265,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;
@@ -401,6 +386,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;
@@ -438,11 +424,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 v3 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
V3: resend
 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 e945177cf2c8..b0c0e2c9022b 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -265,21 +265,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;
@@ -401,6 +386,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;
@@ -438,11 +424,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 v4 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@intel.com>
---
V2: new in the series
V3: resend

 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 b0c0e2c9022b..94700f71d65e 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"
@@ -152,8 +153,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);
@@ -436,9 +435,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;
 }
@@ -450,9 +456,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

[PATCH v4 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
V3: resend

 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 b0c0e2c9022b..94700f71d65e 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"
@@ -152,8 +153,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);
@@ -436,9 +435,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;
 }
@@ -450,9 +456,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 v3 0/4] tpm/tpm_crb: implement power management.

[Winkler, Tomas]

On Mon, 2016-09-12 at 16:04 +0300, Tomas Winkler wrote:
> 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.
> V3. fix lower case corruption in the first patch
> 
Jarkko, had you chance to test v3 series one on your side, is this okay
to go?

Thanks
Tomas

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

[Winkler, Tomas]

On Mon, 2016-09-12 at 16:04 +0300, Tomas Winkler wrote:
> 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.
> V3. fix lower case corruption in the first patch
> 
Jarkko, had you chance to test v3 series one on your side, is this okay
to go?

Thanks
Tomas

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

[Jarkko Sakkinen]

On Wed, Sep 14, 2016 at 06:28:03AM +0000, Winkler, Tomas wrote:
> On Mon, 2016-09-12 at 16:04 +0300, Tomas Winkler wrote:
> > 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.
> > V3. fix lower case corruption in the first patch
> > 
> Jarkko, had you chance to test v3 series one on your side, is this okay
> to go?

Opens for me are:

- pm_runtime_put_sync() is still used 
- callback names
- wait_for_tpm_stat

> Thanks
> Tomas

/Jarkko

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

[Jarkko Sakkinen]

On Wed, Sep 14, 2016 at 06:28:03AM +0000, Winkler, Tomas wrote:
> On Mon, 2016-09-12 at 16:04 +0300, Tomas Winkler wrote:
> > 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.
> > V3. fix lower case corruption in the first patch
> > 
> Jarkko, had you chance to test v3 series one on your side, is this okay
> to go?

Opens for me are:

- pm_runtime_put_sync() is still used 
- callback names
- wait_for_tpm_stat

> Thanks
> Tomas

/Jarkko

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

[Jarkko Sakkinen]

On Wed, Sep 14, 2016 at 07:06:02PM +0300, Jarkko Sakkinen wrote:
> On Wed, Sep 14, 2016 at 06:28:03AM +0000, Winkler, Tomas wrote:
> > On Mon, 2016-09-12 at 16:04 +0300, Tomas Winkler wrote:
> > > 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.
> > > V3. fix lower case corruption in the first patch
> > > 
> > Jarkko, had you chance to test v3 series one on your side, is this okay
> > to go?
> 
> Opens for me are:
> 
> - pm_runtime_put_sync() is still used 
> - callback names
> - wait_for_tpm_stat

I also tested v3 and it works for me.

/Jarkko

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

[Jarkko Sakkinen]

On Wed, Sep 14, 2016 at 07:06:02PM +0300, Jarkko Sakkinen wrote:
> On Wed, Sep 14, 2016 at 06:28:03AM +0000, Winkler, Tomas wrote:
> > On Mon, 2016-09-12 at 16:04 +0300, Tomas Winkler wrote:
> > > 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.
> > > V3. fix lower case corruption in the first patch
> > > 
> > Jarkko, had you chance to test v3 series one on your side, is this okay
> > to go?
> 
> Opens for me are:
> 
> - pm_runtime_put_sync() is still used 
> - callback names
> - wait_for_tpm_stat

I also tested v3 and it works for me.

/Jarkko

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

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

[Jarkko Sakkinen]

On Wed, Sep 14, 2016 at 07:06:52PM +0300, Jarkko Sakkinen wrote:
> On Wed, Sep 14, 2016 at 07:06:02PM +0300, Jarkko Sakkinen wrote:
> > On Wed, Sep 14, 2016 at 06:28:03AM +0000, Winkler, Tomas wrote:
> > > On Mon, 2016-09-12 at 16:04 +0300, Tomas Winkler wrote:
> > > > 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.
> > > > V3. fix lower case corruption in the first patch
> > > > 
> > > Jarkko, had you chance to test v3 series one on your side, is this okay
> > > to go?
> > 
> > Opens for me are:
> > 
> > - pm_runtime_put_sync() is still used 
> > - callback names
> > - wait_for_tpm_stat

I think I got you wit with_for_tpm_stat. It's CRB specific internal
thing and thus using wait_for_tpm_stat would be abusing that function
and making it dependent on CRB driver internals.

Could you add a note on this to the commit message and I would be
find with your implementation.

I started to think about this after I wrote about callback names.

/Jarkko

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

[Jarkko Sakkinen]

On Wed, Sep 14, 2016 at 07:06:52PM +0300, Jarkko Sakkinen wrote:
> On Wed, Sep 14, 2016 at 07:06:02PM +0300, Jarkko Sakkinen wrote:
> > On Wed, Sep 14, 2016 at 06:28:03AM +0000, Winkler, Tomas wrote:
> > > On Mon, 2016-09-12 at 16:04 +0300, Tomas Winkler wrote:
> > > > 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.
> > > > V3. fix lower case corruption in the first patch
> > > > 
> > > Jarkko, had you chance to test v3 series one on your side, is this okay
> > > to go?
> > 
> > Opens for me are:
> > 
> > - pm_runtime_put_sync() is still used 
> > - callback names
> > - wait_for_tpm_stat

I think I got you wit with_for_tpm_stat. It's CRB specific internal
thing and thus using wait_for_tpm_stat would be abusing that function
and making it dependent on CRB driver internals.

Could you add a note on this to the commit message and I would be
find with your implementation.

I started to think about this after I wrote about callback names.

/Jarkko

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

[Jarkko Sakkinen]

On Mon, Sep 12, 2016 at 04:04:18PM +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@linux.intel.com>
> oringal patch:
> 'tpm_crb: implement power tpm crb power management'
> 
> 
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> ---
> V2: do not export the functions via tpm ops
> V3: fix lower case corruption; adjust function documentation
> 
>  drivers/char/tpm/tpm_crb.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 6e9d1bca712f..b6923a8b3ff7 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -83,6 +83,75 @@ struct crb_priv {
>  	u32 cmd_size;
>  };
>  
> +/**
> + * crb_go_idle - request tpm crb device to go the idle state
> + *
> + * @dev:  crb device
> + * @priv: crb private data
> + *
> + * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ
> + * The device should respond within TIMEOUT_C by clearing the bit.
> + * Anyhow, we do not wait here as a consequent CMD_READY request
> + * will be handled correctly even if idle was not completed.
> + *
> + * The function does nothing for devices with ACPI-start method.
> + *
> + * 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 - request tpm crb device to enter ready state
> + *
> + * @dev:  crb device
> + * @priv: crb private data
> + *
> + * Write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ
> + * and poll till the device acknowledge it by clearing the bit.
> + * The device should respond within TIMEOUT_C.
> + *
> + * The function does nothing for devices with ACPI-start method
> + *
> + * 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));

Since this is HW specific this is right thing to do and not abuse
wait_for_tpm_stat. However, this should be documented to the commit
message.

> +
> +	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
> 

Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

/Jarkko

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

[Jarkko Sakkinen]

On Mon, Sep 12, 2016 at 04:04:18PM +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
> V3: fix lower case corruption; adjust function documentation
> 
>  drivers/char/tpm/tpm_crb.c | 69 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 69 insertions(+)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 6e9d1bca712f..b6923a8b3ff7 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -83,6 +83,75 @@ struct crb_priv {
>  	u32 cmd_size;
>  };
>  
> +/**
> + * crb_go_idle - request tpm crb device to go the idle state
> + *
> + * @dev:  crb device
> + * @priv: crb private data
> + *
> + * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ
> + * The device should respond within TIMEOUT_C by clearing the bit.
> + * Anyhow, we do not wait here as a consequent CMD_READY request
> + * will be handled correctly even if idle was not completed.
> + *
> + * The function does nothing for devices with ACPI-start method.
> + *
> + * 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 - request tpm crb device to enter ready state
> + *
> + * @dev:  crb device
> + * @priv: crb private data
> + *
> + * Write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ
> + * and poll till the device acknowledge it by clearing the bit.
> + * The device should respond within TIMEOUT_C.
> + *
> + * The function does nothing for devices with ACPI-start method
> + *
> + * 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));

Since this is HW specific this is right thing to do and not abuse
wait_for_tpm_stat. However, this should be documented to the commit
message.

> +
> +	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
> 

Tested-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

/Jarkko

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

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

[Jarkko Sakkinen]

On Mon, Sep 12, 2016 at 04:04:19PM +0300, Tomas Winkler wrote:
> 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@intel.com>

Tested-by: Jarkko Sakkinen <jarkko.sakkinn@linux.intel.com>
Reviewed-by: Jarkko Sakkinen <jarkko.sakkinn@linux.intel.com>

/Jarkko

> ---
> V2: cmd read need to be called also before crb_init as this will run
>  self test.
> V3: resend.
> 
>  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 b6923a8b3ff7..e945177cf2c8 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -318,6 +318,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;
> @@ -345,12 +346,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);
> @@ -358,7 +374,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
> @@ -366,12 +383,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)
> @@ -415,7 +438,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
> 

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

[Jarkko Sakkinen]

On Mon, Sep 12, 2016 at 04:04:20PM +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.
> 
> Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>

Use pm_runtime_put().

Tested-by: Jarkko Sakkinen <jarkko.sakkinn@linux.intel.com>

> ---
> V2: new in the series
> V3: resend
>  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 e945177cf2c8..b0c0e2c9022b 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -265,21 +265,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;
> @@ -401,6 +386,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;
> @@ -438,11 +424,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
> 

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

[Winkler, Tomas]

> Subject: Re: [PATCH v3 1/4] tpm/tpm_crb: implement tpm crb idle state
> 
> On Mon, Sep 12, 2016 at 04:04:18PM +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@linux.intel.com> oringal
> > patch:
> > 'tpm_crb: implement power tpm crb power management'
> >
> >
> > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > ---
> > V2: do not export the functions via tpm ops
> > V3: fix lower case corruption; adjust function documentation
> >
> >  drivers/char/tpm/tpm_crb.c | 69
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 69 insertions(+)
> >
> > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > index 6e9d1bca712f..b6923a8b3ff7 100644
> > --- a/drivers/char/tpm/tpm_crb.c
> > +++ b/drivers/char/tpm/tpm_crb.c
> > @@ -83,6 +83,75 @@ struct crb_priv {
> >  	u32 cmd_size;
> >  };
> >
> > +/**
> > + * crb_go_idle - request tpm crb device to go the idle state
> > + *
> > + * @dev:  crb device
> > + * @priv: crb private data
> > + *
> > + * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ
> > + * The device should respond within TIMEOUT_C by clearing the bit.
> > + * Anyhow, we do not wait here as a consequent CMD_READY request
> > + * will be handled correctly even if idle was not completed.
> > + *
> > + * The function does nothing for devices with ACPI-start method.
> > + *
> > + * 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 - request tpm crb device to enter ready state
> > + *
> > + * @dev:  crb device
> > + * @priv: crb private data
> > + *
> > + * Write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ
> > + * and poll till the device acknowledge it by clearing the bit.
> > + * The device should respond within TIMEOUT_C.
> > + *
> > + * The function does nothing for devices with ACPI-start method
> > + *
> > + * 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));
> 
> Since this is HW specific this is right thing to do and not abuse
> wait_for_tpm_stat. However, this should be documented to the commit
> message.
I will respin just this patch and not the whole series, as the fix is only in the commit message. 
Tomas

> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> /Jarkko

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

[Winkler, Tomas]

> Subject: Re: [PATCH v3 1/4] tpm/tpm_crb: implement tpm crb idle state
> 
> On Mon, Sep 12, 2016 at 04:04:18PM +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
> > V3: fix lower case corruption; adjust function documentation
> >
> >  drivers/char/tpm/tpm_crb.c | 69
> > ++++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 69 insertions(+)
> >
> > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > index 6e9d1bca712f..b6923a8b3ff7 100644
> > --- a/drivers/char/tpm/tpm_crb.c
> > +++ b/drivers/char/tpm/tpm_crb.c
> > @@ -83,6 +83,75 @@ struct crb_priv {
> >  	u32 cmd_size;
> >  };
> >
> > +/**
> > + * crb_go_idle - request tpm crb device to go the idle state
> > + *
> > + * @dev:  crb device
> > + * @priv: crb private data
> > + *
> > + * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ
> > + * The device should respond within TIMEOUT_C by clearing the bit.
> > + * Anyhow, we do not wait here as a consequent CMD_READY request
> > + * will be handled correctly even if idle was not completed.
> > + *
> > + * The function does nothing for devices with ACPI-start method.
> > + *
> > + * 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 - request tpm crb device to enter ready state
> > + *
> > + * @dev:  crb device
> > + * @priv: crb private data
> > + *
> > + * Write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ
> > + * and poll till the device acknowledge it by clearing the bit.
> > + * The device should respond within TIMEOUT_C.
> > + *
> > + * The function does nothing for devices with ACPI-start method
> > + *
> > + * 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));
> 
> Since this is HW specific this is right thing to do and not abuse
> wait_for_tpm_stat. However, this should be documented to the commit
> message.
I will respin just this patch and not the whole series, as the fix is only in the commit message. 
Tomas

> Tested-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> 
> /Jarkko

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

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

[Jarkko Sakkinen]

On Thu, Sep 15, 2016 at 06:28:27AM +0000, Winkler, Tomas wrote:
> > Subject: Re: [PATCH v3 1/4] tpm/tpm_crb: implement tpm crb idle state
> > 
> > On Mon, Sep 12, 2016 at 04:04:18PM +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@linux.intel.com> oringal
> > > patch:
> > > 'tpm_crb: implement power tpm crb power management'
> > >
> > >
> > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > ---
> > > V2: do not export the functions via tpm ops
> > > V3: fix lower case corruption; adjust function documentation
> > >
> > >  drivers/char/tpm/tpm_crb.c | 69
> > > ++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 69 insertions(+)
> > >
> > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > > index 6e9d1bca712f..b6923a8b3ff7 100644
> > > --- a/drivers/char/tpm/tpm_crb.c
> > > +++ b/drivers/char/tpm/tpm_crb.c
> > > @@ -83,6 +83,75 @@ struct crb_priv {
> > >  	u32 cmd_size;
> > >  };
> > >
> > > +/**
> > > + * crb_go_idle - request tpm crb device to go the idle state
> > > + *
> > > + * @dev:  crb device
> > > + * @priv: crb private data
> > > + *
> > > + * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ
> > > + * The device should respond within TIMEOUT_C by clearing the bit.
> > > + * Anyhow, we do not wait here as a consequent CMD_READY request
> > > + * will be handled correctly even if idle was not completed.
> > > + *
> > > + * The function does nothing for devices with ACPI-start method.
> > > + *
> > > + * 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 - request tpm crb device to enter ready state
> > > + *
> > > + * @dev:  crb device
> > > + * @priv: crb private data
> > > + *
> > > + * Write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ
> > > + * and poll till the device acknowledge it by clearing the bit.
> > > + * The device should respond within TIMEOUT_C.
> > > + *
> > > + * The function does nothing for devices with ACPI-start method
> > > + *
> > > + * 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));
> > 
> > Since this is HW specific this is right thing to do and not abuse
> > wait_for_tpm_stat. However, this should be documented to the commit
> > message.
> I will respin just this patch and not the whole series, as the fix is only in the commit message. 
> Tomas

Works for me. I can update pm_runtime_sync(). Then I'm ready
to apply these.

/Jarkko

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

[Jarkko Sakkinen]

On Thu, Sep 15, 2016 at 06:28:27AM +0000, Winkler, Tomas wrote:
> > Subject: Re: [PATCH v3 1/4] tpm/tpm_crb: implement tpm crb idle state
> > 
> > On Mon, Sep 12, 2016 at 04:04:18PM +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
> > > V3: fix lower case corruption; adjust function documentation
> > >
> > >  drivers/char/tpm/tpm_crb.c | 69
> > > ++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 69 insertions(+)
> > >
> > > diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> > > index 6e9d1bca712f..b6923a8b3ff7 100644
> > > --- a/drivers/char/tpm/tpm_crb.c
> > > +++ b/drivers/char/tpm/tpm_crb.c
> > > @@ -83,6 +83,75 @@ struct crb_priv {
> > >  	u32 cmd_size;
> > >  };
> > >
> > > +/**
> > > + * crb_go_idle - request tpm crb device to go the idle state
> > > + *
> > > + * @dev:  crb device
> > > + * @priv: crb private data
> > > + *
> > > + * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ
> > > + * The device should respond within TIMEOUT_C by clearing the bit.
> > > + * Anyhow, we do not wait here as a consequent CMD_READY request
> > > + * will be handled correctly even if idle was not completed.
> > > + *
> > > + * The function does nothing for devices with ACPI-start method.
> > > + *
> > > + * 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 - request tpm crb device to enter ready state
> > > + *
> > > + * @dev:  crb device
> > > + * @priv: crb private data
> > > + *
> > > + * Write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ
> > > + * and poll till the device acknowledge it by clearing the bit.
> > > + * The device should respond within TIMEOUT_C.
> > > + *
> > > + * The function does nothing for devices with ACPI-start method
> > > + *
> > > + * 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));
> > 
> > Since this is HW specific this is right thing to do and not abuse
> > wait_for_tpm_stat. However, this should be documented to the commit
> > message.
> I will respin just this patch and not the whole series, as the fix is only in the commit message. 
> Tomas

Works for me. I can update pm_runtime_sync(). Then I'm ready
to apply these.

/Jarkko

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

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

[Winkler, Tomas]

> > > Subject: Re: [PATCH v3 1/4] tpm/tpm_crb: implement tpm crb idle
> > > state
> > >
> > > On Mon, Sep 12, 2016 at 04:04:18PM +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@linux.intel.com> oringal
> > > > patch:
> > > > 'tpm_crb: implement power tpm crb power management'
> > > >
> > > >
> > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > > ---
> > > > V2: do not export the functions via tpm ops
> > > > V3: fix lower case corruption; adjust function documentation
> > > >
> > > >  drivers/char/tpm/tpm_crb.c | 69
> > > > ++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 69 insertions(+)
> > > >
> > > > diff --git a/drivers/char/tpm/tpm_crb.c
> > > > b/drivers/char/tpm/tpm_crb.c index 6e9d1bca712f..b6923a8b3ff7
> > > > 100644
> > > > --- a/drivers/char/tpm/tpm_crb.c
> > > > +++ b/drivers/char/tpm/tpm_crb.c
> > > > @@ -83,6 +83,75 @@ struct crb_priv {
> > > >  	u32 cmd_size;
> > > >  };
> > > >
> > > > +/**
> > > > + * crb_go_idle - request tpm crb device to go the idle state
> > > > + *
> > > > + * @dev:  crb device
> > > > + * @priv: crb private data
> > > > + *
> > > > + * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ
> > > > + * The device should respond within TIMEOUT_C by clearing the bit.
> > > > + * Anyhow, we do not wait here as a consequent CMD_READY request
> > > > + * will be handled correctly even if idle was not completed.
> > > > + *
> > > > + * The function does nothing for devices with ACPI-start method.
> > > > + *
> > > > + * 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 - request tpm crb device to enter ready state
> > > > + *
> > > > + * @dev:  crb device
> > > > + * @priv: crb private data
> > > > + *
> > > > + * Write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ
> > > > + * and poll till the device acknowledge it by clearing the bit.
> > > > + * The device should respond within TIMEOUT_C.
> > > > + *
> > > > + * The function does nothing for devices with ACPI-start method
> > > > + *
> > > > + * 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));
> > >
> > > Since this is HW specific this is right thing to do and not abuse
> > > wait_for_tpm_stat. However, this should be documented to the commit
> > > message.
> > I will respin just this patch and not the whole series, as the fix is only in the
> commit message.
> > Tomas
> 
> Works for me. I can update pm_runtime_sync(). Then I'm ready to apply
> these.

What do you mean by pm_runtime_sync()?

Tomas

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

[Winkler, Tomas]

> > > Subject: Re: [PATCH v3 1/4] tpm/tpm_crb: implement tpm crb idle
> > > state
> > >
> > > On Mon, Sep 12, 2016 at 04:04:18PM +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
> > > > V3: fix lower case corruption; adjust function documentation
> > > >
> > > >  drivers/char/tpm/tpm_crb.c | 69
> > > > ++++++++++++++++++++++++++++++++++++++++++++++
> > > >  1 file changed, 69 insertions(+)
> > > >
> > > > diff --git a/drivers/char/tpm/tpm_crb.c
> > > > b/drivers/char/tpm/tpm_crb.c index 6e9d1bca712f..b6923a8b3ff7
> > > > 100644
> > > > --- a/drivers/char/tpm/tpm_crb.c
> > > > +++ b/drivers/char/tpm/tpm_crb.c
> > > > @@ -83,6 +83,75 @@ struct crb_priv {
> > > >  	u32 cmd_size;
> > > >  };
> > > >
> > > > +/**
> > > > + * crb_go_idle - request tpm crb device to go the idle state
> > > > + *
> > > > + * @dev:  crb device
> > > > + * @priv: crb private data
> > > > + *
> > > > + * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ
> > > > + * The device should respond within TIMEOUT_C by clearing the bit.
> > > > + * Anyhow, we do not wait here as a consequent CMD_READY request
> > > > + * will be handled correctly even if idle was not completed.
> > > > + *
> > > > + * The function does nothing for devices with ACPI-start method.
> > > > + *
> > > > + * 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 - request tpm crb device to enter ready state
> > > > + *
> > > > + * @dev:  crb device
> > > > + * @priv: crb private data
> > > > + *
> > > > + * Write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ
> > > > + * and poll till the device acknowledge it by clearing the bit.
> > > > + * The device should respond within TIMEOUT_C.
> > > > + *
> > > > + * The function does nothing for devices with ACPI-start method
> > > > + *
> > > > + * 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));
> > >
> > > Since this is HW specific this is right thing to do and not abuse
> > > wait_for_tpm_stat. However, this should be documented to the commit
> > > message.
> > I will respin just this patch and not the whole series, as the fix is only in the
> commit message.
> > Tomas
> 
> Works for me. I can update pm_runtime_sync(). Then I'm ready to apply
> these.

What do you mean by pm_runtime_sync()?

Tomas


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

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

[Jarkko Sakkinen]

On Thu, Sep 15, 2016 at 08:23:03AM +0000, Winkler, Tomas wrote:
> > > > Subject: Re: [PATCH v3 1/4] tpm/tpm_crb: implement tpm crb idle
> > > > state
> > > >
> > > > On Mon, Sep 12, 2016 at 04:04:18PM +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@linux.intel.com> oringal
> > > > > patch:
> > > > > 'tpm_crb: implement power tpm crb power management'
> > > > >
> > > > >
> > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > > > ---
> > > > > V2: do not export the functions via tpm ops
> > > > > V3: fix lower case corruption; adjust function documentation
> > > > >
> > > > >  drivers/char/tpm/tpm_crb.c | 69
> > > > > ++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 69 insertions(+)
> > > > >
> > > > > diff --git a/drivers/char/tpm/tpm_crb.c
> > > > > b/drivers/char/tpm/tpm_crb.c index 6e9d1bca712f..b6923a8b3ff7
> > > > > 100644
> > > > > --- a/drivers/char/tpm/tpm_crb.c
> > > > > +++ b/drivers/char/tpm/tpm_crb.c
> > > > > @@ -83,6 +83,75 @@ struct crb_priv {
> > > > >  	u32 cmd_size;
> > > > >  };
> > > > >
> > > > > +/**
> > > > > + * crb_go_idle - request tpm crb device to go the idle state
> > > > > + *
> > > > > + * @dev:  crb device
> > > > > + * @priv: crb private data
> > > > > + *
> > > > > + * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ
> > > > > + * The device should respond within TIMEOUT_C by clearing the bit.
> > > > > + * Anyhow, we do not wait here as a consequent CMD_READY request
> > > > > + * will be handled correctly even if idle was not completed.
> > > > > + *
> > > > > + * The function does nothing for devices with ACPI-start method.
> > > > > + *
> > > > > + * 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 - request tpm crb device to enter ready state
> > > > > + *
> > > > > + * @dev:  crb device
> > > > > + * @priv: crb private data
> > > > > + *
> > > > > + * Write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ
> > > > > + * and poll till the device acknowledge it by clearing the bit.
> > > > > + * The device should respond within TIMEOUT_C.
> > > > > + *
> > > > > + * The function does nothing for devices with ACPI-start method
> > > > > + *
> > > > > + * 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));
> > > >
> > > > Since this is HW specific this is right thing to do and not abuse
> > > > wait_for_tpm_stat. However, this should be documented to the commit
> > > > message.
> > > I will respin just this patch and not the whole series, as the fix is only in the
> > commit message.
> > > Tomas
> > 
> > Works for me. I can update pm_runtime_sync(). Then I'm ready to apply
> > these.
> 
> What do you mean by pm_runtime_sync()?

Typo. I already commente v2 of the series that pm_runtime_put should be
used instead of pm_runtime_put_sync.

/Jarkko

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

[Jarkko Sakkinen]

On Thu, Sep 15, 2016 at 08:23:03AM +0000, Winkler, Tomas wrote:
> > > > Subject: Re: [PATCH v3 1/4] tpm/tpm_crb: implement tpm crb idle
> > > > state
> > > >
> > > > On Mon, Sep 12, 2016 at 04:04:18PM +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
> > > > > V3: fix lower case corruption; adjust function documentation
> > > > >
> > > > >  drivers/char/tpm/tpm_crb.c | 69
> > > > > ++++++++++++++++++++++++++++++++++++++++++++++
> > > > >  1 file changed, 69 insertions(+)
> > > > >
> > > > > diff --git a/drivers/char/tpm/tpm_crb.c
> > > > > b/drivers/char/tpm/tpm_crb.c index 6e9d1bca712f..b6923a8b3ff7
> > > > > 100644
> > > > > --- a/drivers/char/tpm/tpm_crb.c
> > > > > +++ b/drivers/char/tpm/tpm_crb.c
> > > > > @@ -83,6 +83,75 @@ struct crb_priv {
> > > > >  	u32 cmd_size;
> > > > >  };
> > > > >
> > > > > +/**
> > > > > + * crb_go_idle - request tpm crb device to go the idle state
> > > > > + *
> > > > > + * @dev:  crb device
> > > > > + * @priv: crb private data
> > > > > + *
> > > > > + * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ
> > > > > + * The device should respond within TIMEOUT_C by clearing the bit.
> > > > > + * Anyhow, we do not wait here as a consequent CMD_READY request
> > > > > + * will be handled correctly even if idle was not completed.
> > > > > + *
> > > > > + * The function does nothing for devices with ACPI-start method.
> > > > > + *
> > > > > + * 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 - request tpm crb device to enter ready state
> > > > > + *
> > > > > + * @dev:  crb device
> > > > > + * @priv: crb private data
> > > > > + *
> > > > > + * Write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ
> > > > > + * and poll till the device acknowledge it by clearing the bit.
> > > > > + * The device should respond within TIMEOUT_C.
> > > > > + *
> > > > > + * The function does nothing for devices with ACPI-start method
> > > > > + *
> > > > > + * 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));
> > > >
> > > > Since this is HW specific this is right thing to do and not abuse
> > > > wait_for_tpm_stat. However, this should be documented to the commit
> > > > message.
> > > I will respin just this patch and not the whole series, as the fix is only in the
> > commit message.
> > > Tomas
> > 
> > Works for me. I can update pm_runtime_sync(). Then I'm ready to apply
> > these.
> 
> What do you mean by pm_runtime_sync()?

Typo. I already commente v2 of the series that pm_runtime_put should be
used instead of pm_runtime_put_sync.

/Jarkko

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

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

[Winkler, Tomas]

> On Thu, Sep 15, 2016 at 08:23:03AM +0000, Winkler, Tomas wrote:
> > > > > Subject: Re: [PATCH v3 1/4] tpm/tpm_crb: implement tpm crb idle
> > > > > state
> > > > >
> > > > > On Mon, Sep 12, 2016 at 04:04:18PM +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@linux.intel.com>
> > > > > > oringal
> > > > > > patch:
> > > > > > 'tpm_crb: implement power tpm crb power management'
> > > > > >
> > > > > >
> > > > > > Signed-off-by: Tomas Winkler <tomas.winkler@intel.com>
> > > > > > ---
> > > > > > V2: do not export the functions via tpm ops
> > > > > > V3: fix lower case corruption; adjust function documentation
> > > > > >
> > > > > >  drivers/char/tpm/tpm_crb.c | 69
> > > > > > ++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 69 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/char/tpm/tpm_crb.c
> > > > > > b/drivers/char/tpm/tpm_crb.c index 6e9d1bca712f..b6923a8b3ff7
> > > > > > 100644
> > > > > > --- a/drivers/char/tpm/tpm_crb.c
> > > > > > +++ b/drivers/char/tpm/tpm_crb.c
> > > > > > @@ -83,6 +83,75 @@ struct crb_priv {
> > > > > >  	u32 cmd_size;
> > > > > >  };
> > > > > >
> > > > > > +/**
> > > > > > + * crb_go_idle - request tpm crb device to go the idle state
> > > > > > + *
> > > > > > + * @dev:  crb device
> > > > > > + * @priv: crb private data
> > > > > > + *
> > > > > > + * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ
> > > > > > + * The device should respond within TIMEOUT_C by clearing the
> bit.
> > > > > > + * Anyhow, we do not wait here as a consequent CMD_READY
> > > > > > +request
> > > > > > + * will be handled correctly even if idle was not completed.
> > > > > > + *
> > > > > > + * The function does nothing for devices with ACPI-start method.
> > > > > > + *
> > > > > > + * 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 - request tpm crb device to enter ready
> > > > > > +state
> > > > > > + *
> > > > > > + * @dev:  crb device
> > > > > > + * @priv: crb private data
> > > > > > + *
> > > > > > + * Write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ
> > > > > > + * and poll till the device acknowledge it by clearing the bit.
> > > > > > + * The device should respond within TIMEOUT_C.
> > > > > > + *
> > > > > > + * The function does nothing for devices with ACPI-start
> > > > > > +method
> > > > > > + *
> > > > > > + * 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));
> > > > >
> > > > > Since this is HW specific this is right thing to do and not
> > > > > abuse wait_for_tpm_stat. However, this should be documented to
> > > > > the commit message.
> > > > I will respin just this patch and not the whole series, as the fix
> > > > is only in the
> > > commit message.
> > > > Tomas
> > >
> > > Works for me. I can update pm_runtime_sync(). Then I'm ready to
> > > apply these.
> >
> > What do you mean by pm_runtime_sync()?
> 
> Typo. I already commente v2 of the series that pm_runtime_put should be
> used instead of pm_runtime_put_sync.

This is not necessary, it's okay to suspend in asynchronous mode.
Tomas

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

[Winkler, Tomas]

> On Thu, Sep 15, 2016 at 08:23:03AM +0000, Winkler, Tomas wrote:
> > > > > Subject: Re: [PATCH v3 1/4] tpm/tpm_crb: implement tpm crb idle
> > > > > state
> > > > >
> > > > > On Mon, Sep 12, 2016 at 04:04:18PM +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
> > > > > > V3: fix lower case corruption; adjust function documentation
> > > > > >
> > > > > >  drivers/char/tpm/tpm_crb.c | 69
> > > > > > ++++++++++++++++++++++++++++++++++++++++++++++
> > > > > >  1 file changed, 69 insertions(+)
> > > > > >
> > > > > > diff --git a/drivers/char/tpm/tpm_crb.c
> > > > > > b/drivers/char/tpm/tpm_crb.c index 6e9d1bca712f..b6923a8b3ff7
> > > > > > 100644
> > > > > > --- a/drivers/char/tpm/tpm_crb.c
> > > > > > +++ b/drivers/char/tpm/tpm_crb.c
> > > > > > @@ -83,6 +83,75 @@ struct crb_priv {
> > > > > >  	u32 cmd_size;
> > > > > >  };
> > > > > >
> > > > > > +/**
> > > > > > + * crb_go_idle - request tpm crb device to go the idle state
> > > > > > + *
> > > > > > + * @dev:  crb device
> > > > > > + * @priv: crb private data
> > > > > > + *
> > > > > > + * Write CRB_CTRL_REQ_GO_IDLE to TPM_CRB_CTRL_REQ
> > > > > > + * The device should respond within TIMEOUT_C by clearing the
> bit.
> > > > > > + * Anyhow, we do not wait here as a consequent CMD_READY
> > > > > > +request
> > > > > > + * will be handled correctly even if idle was not completed.
> > > > > > + *
> > > > > > + * The function does nothing for devices with ACPI-start method.
> > > > > > + *
> > > > > > + * 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 - request tpm crb device to enter ready
> > > > > > +state
> > > > > > + *
> > > > > > + * @dev:  crb device
> > > > > > + * @priv: crb private data
> > > > > > + *
> > > > > > + * Write CRB_CTRL_REQ_CMD_READY to TPM_CRB_CTRL_REQ
> > > > > > + * and poll till the device acknowledge it by clearing the bit.
> > > > > > + * The device should respond within TIMEOUT_C.
> > > > > > + *
> > > > > > + * The function does nothing for devices with ACPI-start
> > > > > > +method
> > > > > > + *
> > > > > > + * 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));
> > > > >
> > > > > Since this is HW specific this is right thing to do and not
> > > > > abuse wait_for_tpm_stat. However, this should be documented to
> > > > > the commit message.
> > > > I will respin just this patch and not the whole series, as the fix
> > > > is only in the
> > > commit message.
> > > > Tomas
> > >
> > > Works for me. I can update pm_runtime_sync(). Then I'm ready to
> > > apply these.
> >
> > What do you mean by pm_runtime_sync()?
> 
> Typo. I already commente v2 of the series that pm_runtime_put should be
> used instead of pm_runtime_put_sync.

This is not necessary, it's okay to suspend in asynchronous mode.
Tomas


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

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

[Jarkko Sakkinen]

On Thu, Sep 15, 2016 at 09:23:29AM +0300, Jarkko Sakkinen wrote:
> On Mon, Sep 12, 2016 at 04:04:19PM +0300, Tomas Winkler wrote:
> > 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@intel.com>
> 
> Tested-by: Jarkko Sakkinen <jarkko.sakkinn@linux.intel.com>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinn@linux.intel.com>

I noticed something odd or at least not described in the commit message.

> /Jarkko
> 
> > ---
> > V2: cmd read need to be called also before crb_init as this will run
> >  self test.
> > V3: resend.
> > 
> >  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 b6923a8b3ff7..e945177cf2c8 100644
> > --- a/drivers/char/tpm/tpm_crb.c
> > +++ b/drivers/char/tpm/tpm_crb.c
> > @@ -318,6 +318,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;
> > @@ -345,12 +346,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);
> > @@ -358,7 +374,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
> > @@ -366,12 +383,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)
> > @@ -415,7 +438,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;

You do this already in crb_map_io() that is called before crb_init().
What is the purpose of this extra crb_cmd_ready()? Looks unrelated at
least to the described workaround.

> > +
> > +	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

/Jarkko

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

[Jarkko Sakkinen]

On Thu, Sep 15, 2016 at 09:23:29AM +0300, Jarkko Sakkinen wrote:
> On Mon, Sep 12, 2016 at 04:04:19PM +0300, Tomas Winkler wrote:
> > 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>
> 
> Tested-by: Jarkko Sakkinen <jarkko.sakkinn-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> Reviewed-by: Jarkko Sakkinen <jarkko.sakkinn-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

I noticed something odd or at least not described in the commit message.

> /Jarkko
> 
> > ---
> > V2: cmd read need to be called also before crb_init as this will run
> >  self test.
> > V3: resend.
> > 
> >  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 b6923a8b3ff7..e945177cf2c8 100644
> > --- a/drivers/char/tpm/tpm_crb.c
> > +++ b/drivers/char/tpm/tpm_crb.c
> > @@ -318,6 +318,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;
> > @@ -345,12 +346,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);
> > @@ -358,7 +374,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
> > @@ -366,12 +383,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)
> > @@ -415,7 +438,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;

You do this already in crb_map_io() that is called before crb_init().
What is the purpose of this extra crb_cmd_ready()? Looks unrelated at
least to the described workaround.

> > +
> > +	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

/Jarkko

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

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

[Winkler, Tomas]

> 
> On Thu, Sep 15, 2016 at 09:23:29AM +0300, Jarkko Sakkinen wrote:
> > On Mon, Sep 12, 2016 at 04:04:19PM +0300, Tomas Winkler wrote:
> > > 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@intel.com>
> >
> > Tested-by: Jarkko Sakkinen <jarkko.sakkinn@linux.intel.com>
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinn@linux.intel.com>
> 
> I noticed something odd or at least not described in the commit message.
> 
> > /Jarkko
> >
> > > ---
> > > V2: cmd read need to be called also before crb_init as this will run
> > > self test.
> > > V3: resend.
> > >
> > >  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 b6923a8b3ff7..e945177cf2c8 100644
> > > --- a/drivers/char/tpm/tpm_crb.c
> > > +++ b/drivers/char/tpm/tpm_crb.c
> > > @@ -318,6 +318,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;
> > > @@ -345,12 +346,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);
> > > @@ -358,7 +374,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 @@ -366,12 +383,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) @@ -415,7
> > > +438,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;
> 
> You do this already in crb_map_io() that is called before crb_init().
> What is the purpose of this extra crb_cmd_ready()? Looks unrelated at least to
> the described workaround.

Runtime pm can be not compiled into kernel, in that case you would have nonfunctional devices, hence we start the  init flow in explicitly enabled state. 
Thanks
Tomas 

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

[Winkler, Tomas]

> 
> On Thu, Sep 15, 2016 at 09:23:29AM +0300, Jarkko Sakkinen wrote:
> > On Mon, Sep 12, 2016 at 04:04:19PM +0300, Tomas Winkler wrote:
> > > 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>
> >
> > Tested-by: Jarkko Sakkinen <jarkko.sakkinn-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > Reviewed-by: Jarkko Sakkinen <jarkko.sakkinn-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> 
> I noticed something odd or at least not described in the commit message.
> 
> > /Jarkko
> >
> > > ---
> > > V2: cmd read need to be called also before crb_init as this will run
> > > self test.
> > > V3: resend.
> > >
> > >  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 b6923a8b3ff7..e945177cf2c8 100644
> > > --- a/drivers/char/tpm/tpm_crb.c
> > > +++ b/drivers/char/tpm/tpm_crb.c
> > > @@ -318,6 +318,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;
> > > @@ -345,12 +346,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);
> > > @@ -358,7 +374,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 @@ -366,12 +383,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) @@ -415,7
> > > +438,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;
> 
> You do this already in crb_map_io() that is called before crb_init().
> What is the purpose of this extra crb_cmd_ready()? Looks unrelated at least to
> the described workaround.

Runtime pm can be not compiled into kernel, in that case you would have nonfunctional devices, hence we start the  init flow in explicitly enabled state. 
Thanks
Tomas 

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