[PATCH v2] tpm: Add support for new Infineon I2C TPM (SLB 9645 TT 1.2 I2C)

[PATCH v2] tpm: Add support for new Infineon I2C TPM (SLB 9645 TT 1.2 I2C)

[Peter Huewe]

This driver adds support for Infineon's new SLB 9645 TT 1.2 I2C TPMs,
which supports clockstretching, combined reads and a bus speed of
up to 400khz. The device also has a new device id.

The driver works now also fine with device trees, so you can
instantiate your device by adding:
 +       tpm {
 +               compatible = "infineon,slb9645tt";
 +               reg = <0x20>;
 +       };
 for SLB 9645 devices or

 +       tpm {
 +               compatible = "infineon,slb9635tt";
 +               reg = <0x20>;
 +       };
 for SLB 9635 devices

to your device tree.
tpm_i2c_infineon is also retained as a compatible id as a fallback to
slb9635 protocol.

The driver was tested on Beaglebone.

Signed-off-by: Peter Huewe <peter.huewe@infineon.com>
---
Please apply on top of "char/tpm: Convert struct i2c_msg initialization to C99 format" by Shubhrajyoti Datta

 .../devicetree/bindings/i2c/trivial-devices.txt    |   2 +
 drivers/char/tpm/tpm_i2c_infineon.c                | 137 +++++++++++++++------
 2 files changed, 99 insertions(+), 40 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/trivial-devices.txt b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
index 446859f..ad6a738 100644
--- a/Documentation/devicetree/bindings/i2c/trivial-devices.txt
+++ b/Documentation/devicetree/bindings/i2c/trivial-devices.txt
@@ -35,6 +35,8 @@ fsl,mc13892		MC13892: Power Management Integrated Circuit (PMIC) for i.MX35/51
 fsl,mma8450		MMA8450Q: Xtrinsic Low-power, 3-axis Xtrinsic Accelerometer
 fsl,mpr121		MPR121: Proximity Capacitive Touch Sensor Controller
 fsl,sgtl5000		SGTL5000: Ultra Low-Power Audio Codec
+infineon,slb9635tt	Infineon SLB9635 (Soft-) I2C TPM (old protocol, max 100khz)
+infineon,slb9645tt	Infineon SLB9645 I2C TPM (new protocol, max 400khz)
 maxim,ds1050		5 Bit Programmable, Pulse-Width Modulator
 maxim,max1237		Low-Power, 4-/12-Channel, 2-Wire Serial, 12-Bit ADCs
 maxim,max6625		9-Bit/12-Bit Temperature Sensors with I²C-Compatible Serial Interface
diff --git a/drivers/char/tpm/tpm_i2c_infineon.c b/drivers/char/tpm/tpm_i2c_infineon.c
index 8e47e2b..e868dde 100644
--- a/drivers/char/tpm/tpm_i2c_infineon.c
+++ b/drivers/char/tpm/tpm_i2c_infineon.c
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2012 Infineon Technologies
+ * Copyright (C) 2012,2013 Infineon Technologies
  *
  * Authors:
  * Peter Huewe <peter.huewe@infineon.com>
@@ -56,13 +56,21 @@
 #define TPM_TIMEOUT_US_HI  (TPM_TIMEOUT_US_LOW + 2000)
 
 /* expected value for DIDVID register */
-#define TPM_TIS_I2C_DID_VID 0x000b15d1L
+#define TPM_TIS_I2C_DID_VID_9635 0xd1150b00L
+#define TPM_TIS_I2C_DID_VID_9645 0x001a15d1L
+
+enum i2c_chip_type {
+	SLB9635,
+	SLB9645,
+	UNKNOWN,
+};
 
 /* Structure to store I2C TPM specific stuff */
 struct tpm_inf_dev {
 	struct i2c_client *client;
 	u8 buf[TPM_BUFSIZE + sizeof(u8)]; /* max. buffer size + addr */
 	struct tpm_chip *chip;
+	enum i2c_chip_type chip_type;
 };
 
 static struct tpm_inf_dev tpm_dev;
@@ -101,8 +109,9 @@ static int iic_tpm_read(u8 addr, u8 *buffer, size_t len)
 		.len = len,
 		.buf = buffer
 	};
+	struct i2c_msg msgs[] = {msg1, msg2};
 
-	int rc;
+	int rc = 0;
 	int count;
 
 	/* Lock the adapter for the duration of the whole sequence. */
@@ -110,30 +119,49 @@ static int iic_tpm_read(u8 addr, u8 *buffer, size_t len)
 		return -EOPNOTSUPP;
 	i2c_lock_adapter(tpm_dev.client->adapter);
 
-	for (count = 0; count < MAX_COUNT; count++) {
-		rc = __i2c_transfer(tpm_dev.client->adapter, &msg1, 1);
-		if (rc > 0)
-			break;	/* break here to skip sleep */
-
-		usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
-	}
-
-	if (rc <= 0)
-		goto out;
-
-	/* After the TPM has successfully received the register address it needs
-	 * some time, thus we're sleeping here again, before retrieving the data
-	 */
-	for (count = 0; count < MAX_COUNT; count++) {
-		usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
-		rc = __i2c_transfer(tpm_dev.client->adapter, &msg2, 1);
-		if (rc > 0)
-			break;
+	if (tpm_dev.chip_type == SLB9645) {
+		/* use a combined read for newer chips
+		 * unfortunately the smbus functions are not suitable due to
+		 * the 32 byte limit of the smbus.
+		 * retries should usually not be needed, but are kept just to
+		 * be on the safe side.
+		*/
+		for (count = 0; count < MAX_COUNT; count++) {
+			rc = __i2c_transfer(tpm_dev.client->adapter, msgs, 2);
+			if (rc > 0)
+				break;	/* break here to skip sleep */
+			usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
+		}
+	} else {
+		/* slb9635 protocol should work in all cases */
+		for (count = 0; count < MAX_COUNT; count++) {
+			rc = __i2c_transfer(tpm_dev.client->adapter, &msg1, 1);
+			if (rc > 0)
+				break;	/* break here to skip sleep */
+
+			usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
+		}
 
+		if (rc <= 0)
+			goto out;
+
+		/* After the TPM has successfully received the register address
+		 * it needs some time, thus we're sleeping here again, before
+		 * retrieving the data
+		 */
+		for (count = 0; count < MAX_COUNT; count++) {
+			usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
+			rc = __i2c_transfer(tpm_dev.client->adapter, &msg2, 1);
+			if (rc > 0)
+				break;
+		}
 	}
 
 out:
 	i2c_unlock_adapter(tpm_dev.client->adapter);
+	/* take care of 'guard time' */
+	usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
+
 	if (rc <= 0)
 		return -EIO;
 
@@ -167,16 +195,19 @@ static int iic_tpm_write_generic(u8 addr, u8 *buffer, size_t len,
 	/*
 	 * NOTE: We have to use these special mechanisms here and unfortunately
 	 * cannot rely on the standard behavior of i2c_transfer.
+	 * Even for newer chips the smbus functions are not
+	 * suitable due to the 32 byte limit of the smbus.
 	 */
 	for (count = 0; count < max_count; count++) {
 		rc = __i2c_transfer(tpm_dev.client->adapter, &msg1, 1);
 		if (rc > 0)
 			break;
-
 		usleep_range(sleep_low, sleep_hi);
 	}
 
 	i2c_unlock_adapter(tpm_dev.client->adapter);
+	/* take care of 'guard time' */
+	usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
 	if (rc <= 0)
 		return -EIO;
 
@@ -296,11 +327,18 @@ static int request_locality(struct tpm_chip *chip, int loc)
 static u8 tpm_tis_i2c_status(struct tpm_chip *chip)
 {
 	/* NOTE: since I2C read may fail, return 0 in this case --> time-out */
-	u8 buf;
-	if (iic_tpm_read(TPM_STS(chip->vendor.locality), &buf, 1) < 0)
-		return 0;
-	else
-		return buf;
+	u8 buf = 0xFF;
+	u8 i = 0;
+
+	do {
+		if (iic_tpm_read(TPM_STS(chip->vendor.locality), &buf, 1) < 0)
+			return 0;
+
+		i++;
+	/* if locallity is set STS should not be 0xFF */
+	} while ((buf == 0xFF) && i < 10);
+
+	return buf;
 }
 
 static void tpm_tis_i2c_ready(struct tpm_chip *chip)
@@ -341,7 +379,7 @@ static int wait_for_stat(struct tpm_chip *chip, u8 mask, unsigned long timeout,
 
 	/* check current status */
 	*status = tpm_tis_i2c_status(chip);
-	if ((*status & mask) == mask)
+	if ((*status != 0xFF) && (*status & mask) == mask)
 		return 0;
 
 	stop = jiffies + timeout;
@@ -385,7 +423,6 @@ static int recv_data(struct tpm_chip *chip, u8 *buf, size_t count)
 		/* avoid endless loop in case of broken HW */
 		if (retries > MAX_COUNT_LONG)
 			return -EIO;
-
 	}
 	return size;
 }
@@ -493,7 +530,6 @@ static int tpm_tis_i2c_send(struct tpm_chip *chip, u8 *buf, size_t len)
 			rc = -EIO;
 			goto out_err;
 		}
-
 	}
 
 	/* write last byte */
@@ -581,6 +617,7 @@ static int tpm_tis_i2c_init(struct device *dev)
 
 	chip = tpm_register_hardware(dev, &tpm_tis_i2c);
 	if (!chip) {
+		dev_err(dev, "could not register hardware\n");
 		rc = -ENODEV;
 		goto out_err;
 	}
@@ -595,20 +632,24 @@ static int tpm_tis_i2c_init(struct device *dev)
 	chip->vendor.timeout_d = msecs_to_jiffies(TIS_SHORT_TIMEOUT);
 
 	if (request_locality(chip, 0) != 0) {
+		dev_err(dev, "could not request locality\n");
 		rc = -ENODEV;
 		goto out_vendor;
 	}
 
 	/* read four bytes from DID_VID register */
 	if (iic_tpm_read(TPM_DID_VID(0), (u8 *)&vendor, 4) < 0) {
+		dev_err(dev, "could not read vendor id\n");
 		rc = -EIO;
 		goto out_release;
 	}
 
-	/* create DID_VID register value, after swapping to little-endian */
-	vendor = be32_to_cpu((__be32) vendor);
-
-	if (vendor != TPM_TIS_I2C_DID_VID) {
+	if (vendor == TPM_TIS_I2C_DID_VID_9645) {
+		tpm_dev.chip_type = SLB9645;
+	} else if (vendor == TPM_TIS_I2C_DID_VID_9635) {
+		tpm_dev.chip_type = SLB9635;
+	} else {
+		dev_err(dev, "vendor id did not match! ID was %08x\n", vendor);
 		rc = -ENODEV;
 		goto out_release;
 	}
@@ -644,22 +685,38 @@ out_err:
 
 static const struct i2c_device_id tpm_tis_i2c_table[] = {
 	{"tpm_i2c_infineon", 0},
+	{"slb9635tt", 0},
+	{"slb9645tt", 1},
 	{},
 };
 
 MODULE_DEVICE_TABLE(i2c, tpm_tis_i2c_table);
+
+#ifdef CONFIG_OF
+static const struct of_device_id tpm_tis_i2c_of_match[] = {
+	{ .compatible = "infineon,tpm_i2c_infineon", .data = (void *)0 },
+	{ .compatible = "infineon,slb9635tt", .data = (void *)0 },
+	{ .compatible = "infineon,slb9645tt", .data = (void *)1 },
+	{},
+};
+MODULE_DEVICE_TABLE(of, tpm_tis_i2c_of_match);
+#endif
+
 static SIMPLE_DEV_PM_OPS(tpm_tis_i2c_ops, tpm_pm_suspend, tpm_pm_resume);
 
 static int tpm_tis_i2c_probe(struct i2c_client *client,
 			     const struct i2c_device_id *id)
 {
 	int rc;
-	if (tpm_dev.client != NULL)
+	struct device *dev = &(client->dev);
+
+	if (tpm_dev.client != NULL) {
+		dev_err(dev, "This driver only supports one client at a time\n");
 		return -EBUSY;	/* We only support one client */
+	}
 
 	if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C)) {
-		dev_err(&client->dev,
-			"no algorithms associated to the i2c bus\n");
+		dev_err(dev, "no algorithms associated to the i2c bus\n");
 		return -ENODEV;
 	}
 
@@ -695,7 +752,6 @@ static int tpm_tis_i2c_remove(struct i2c_client *client)
 }
 
 static struct i2c_driver tpm_tis_i2c_driver = {
-
 	.id_table = tpm_tis_i2c_table,
 	.probe = tpm_tis_i2c_probe,
 	.remove = tpm_tis_i2c_remove,
@@ -703,11 +759,12 @@ static struct i2c_driver tpm_tis_i2c_driver = {
 		   .name = "tpm_i2c_infineon",
 		   .owner = THIS_MODULE,
 		   .pm = &tpm_tis_i2c_ops,
+		   .of_match_table = of_match_ptr(tpm_tis_i2c_of_match),
 		   },
 };
 
 module_i2c_driver(tpm_tis_i2c_driver);
 MODULE_AUTHOR("Peter Huewe <peter.huewe@infineon.com>");
 MODULE_DESCRIPTION("TPM TIS I2C Infineon Driver");
-MODULE_VERSION("2.1.5");
+MODULE_VERSION("2.2.0");
 MODULE_LICENSE("GPL");
-- 
1.8.1.msysgit.1

Re: [tpmdd-devel] [PATCH v2] tpm: Add support for new Infineon I2C TPM (SLB 9645 TT 1.2 I2C)

[Jason Gunthorpe]

On Mon, Mar 04, 2013 at 11:41:04AM -0600, Kent Yoder wrote:
> >  	.id_table = tpm_tis_i2c_table,
> >  	.probe = tpm_tis_i2c_probe,
> >  	.remove = tpm_tis_i2c_remove,
> > @@ -703,11 +759,12 @@ static struct i2c_driver tpm_tis_i2c_driver = {
> >  		   .name = "tpm_i2c_infineon",
> >  		   .owner = THIS_MODULE,
> >  		   .pm = &tpm_tis_i2c_ops,
> > +		   .of_match_table = of_match_ptr(tpm_tis_i2c_of_match),
> 
>   Please put this line inside an ifdef CONFIG_OF, since of_match_ptr
> lives in there.

The OF stuff is designed to hide the CONFIG_OF from using code, 
of_match_ptr(x) expends to NULL when CONFIG_OF is not set.

It is optional to include the MATCH_TABLE bit in a CONFI_OF #ifdef,
lots of places don't.

Jason

Re: [tpmdd-devel] [PATCH v2] tpm: Add support for new Infineon I2C TPM (SLB 9645 TT 1.2 I2C)

[Peter Hüwe]

Hi Kent,

short reply from my private account - so only talking on behalf of myself.

Am Montag, 4. März 2013, 18:41:04 schrieb Kent Yoder:
> Hi Peter,
> 
>   Sorry for the long delay in getting this reviewed...
No problem.

> 
> > -	for (count = 0; count < MAX_COUNT; count++) {
> > -		usleep_range(SLEEP_DURATION_LOW, SLEEP_DURATION_HI);
> > -		rc = __i2c_transfer(tpm_dev.client->adapter, &msg2, 1);
> > -		if (rc > 0)
> > -			break;
> > +	if (tpm_dev.chip_type == SLB9645) {
> > +		/* use a combined read for newer chips
> > +		 * unfortunately the smbus functions are not suitable due to
> > +		 * the 32 byte limit of the smbus.
> > +		 * retries should usually not be needed, but are kept just to
> > +		 * be on the safe side.
> > +		*/
> 
>   Bump out that last line of comment by a column.

You mean I should add 1 space - okay, no problem.
(and I'm nitty picky.... ;) *g*


> > 
> >  	if (rc <= 0)
> >  	
> >  		return -EIO;
> 
>  This confused me for a second, but I see __i2c_transfer returns the number
> of messages transferred. Maybe worth adding a comment.

Maybe I can add one - I'll think about it.


> > 
> > +
> > +#ifdef CONFIG_OF
> > +static const struct of_device_id tpm_tis_i2c_of_match[] = {
> > +	{ .compatible = "infineon,tpm_i2c_infineon", .data = (void *)0 },
> > +	{ .compatible = "infineon,slb9635tt", .data = (void *)0 },
> > +	{ .compatible = "infineon,slb9645tt", .data = (void *)1 },
> 
>   Here "name" and "type" are left empty in of_device_id. Will there be
> times when those are needed? Like informational messages from the OF
> subsystem?

Hmm, what do you propose?
name = chip type ? or name = tpm_i2c_infineon?
type = tpm ?



> > 
> > +		   .of_match_table = of_match_ptr(tpm_tis_i2c_of_match),
> 
>   Please put this line inside an ifdef CONFIG_OF, since of_match_ptr
> lives in there.
NACK.
of.h has already the ifdef CONFIG_OF for of_match_ptr and defines it either as 

#define of_match_ptr(_ptr)      (_ptr)
or
#define of_match_ptr(_ptr)      NULL
depending on CONFIG_OF is set.



>   Won't you also need to add OF to Kconfig?
Not really, as the only stuff we're using is the compatible id - the driver can 
live without it and can be probed from userspace or plain old platform data.
I probably have compile tested it with and without CONFIG_OF.


Is it worth a v3?
 or a small update patch which adds the of name/type and the space? (I can 
create this patch immediately)


Thanks,
Peter

Re: [tpmdd-devel] [PATCH v2] tpm: Add support for new Infineon I2C TPM (SLB 9645 TT 1.2 I2C)

[Kent Yoder]

>> >
>> > +
>> > +#ifdef CONFIG_OF
>> > +static const struct of_device_id tpm_tis_i2c_of_match[] = {
>> > +   { .compatible = "infineon,tpm_i2c_infineon", .data = (void *)0 },
>> > +   { .compatible = "infineon,slb9635tt", .data = (void *)0 },
>> > +   { .compatible = "infineon,slb9645tt", .data = (void *)1 },
>>
>>   Here "name" and "type" are left empty in of_device_id. Will there be
>> times when those are needed? Like informational messages from the OF
>> subsystem?
>
> Hmm, what do you propose?
> name = chip type ? or name = tpm_i2c_infineon?
> type = tpm ?

  Doesn't matter to me, I just wanted to be sure you hadn't missed anything.

>> > +              .of_match_table = of_match_ptr(tpm_tis_i2c_of_match),
>>
>>   Please put this line inside an ifdef CONFIG_OF, since of_match_ptr
>> lives in there.
> NACK.
> of.h has already the ifdef CONFIG_OF for of_match_ptr and defines it either as
>
> #define of_match_ptr(_ptr)      (_ptr)
> or
> #define of_match_ptr(_ptr)      NULL
> depending on CONFIG_OF is set.

  Thanks.

>>   Won't you also need to add OF to Kconfig?
> Not really, as the only stuff we're using is the compatible id - the driver can
> live without it and can be probed from userspace or plain old platform data.
> I probably have compile tested it with and without CONFIG_OF.

  Ok.

> Is it worth a v3?
>  or a small update patch which adds the of name/type and the space? (I can
> create this patch immediately)

  Not a problem, I can apply as-is with the fixup.

Kent

>
> Thanks,
> Peter
>
>
>

Re: [tpmdd-devel] [PATCH v2] tpm: Add support for new Infineon I2C TPM (SLB 9645 TT 1.2 I2C)

[Kent Yoder]

On Mon, Mar 4, 2013 at 2:08 PM, Kent Yoder <shpedoikal@gmail.com> wrote:
>>> >
>>> > +
>>> > +#ifdef CONFIG_OF
>>> > +static const struct of_device_id tpm_tis_i2c_of_match[] = {
>>> > +   { .compatible = "infineon,tpm_i2c_infineon", .data = (void *)0 },
>>> > +   { .compatible = "infineon,slb9635tt", .data = (void *)0 },
>>> > +   { .compatible = "infineon,slb9645tt", .data = (void *)1 },
>>>
>>>   Here "name" and "type" are left empty in of_device_id. Will there be
>>> times when those are needed? Like informational messages from the OF
>>> subsystem?
>>
>> Hmm, what do you propose?
>> name = chip type ? or name = tpm_i2c_infineon?
>> type = tpm ?
>
>   Doesn't matter to me, I just wanted to be sure you hadn't missed anything.
>
>>> > +              .of_match_table = of_match_ptr(tpm_tis_i2c_of_match),
>>>
>>>   Please put this line inside an ifdef CONFIG_OF, since of_match_ptr
>>> lives in there.
>> NACK.
>> of.h has already the ifdef CONFIG_OF for of_match_ptr and defines it either as
>>
>> #define of_match_ptr(_ptr)      (_ptr)
>> or
>> #define of_match_ptr(_ptr)      NULL
>> depending on CONFIG_OF is set.
>
>   Thanks.
>
>>>   Won't you also need to add OF to Kconfig?
>> Not really, as the only stuff we're using is the compatible id - the driver can
>> live without it and can be probed from userspace or plain old platform data.
>> I probably have compile tested it with and without CONFIG_OF.
>
>   Ok.
>
>> Is it worth a v3?
>>  or a small update patch which adds the of name/type and the space? (I can
>> create this patch immediately)
>
>   Not a problem, I can apply as-is with the fixup.

Staged here:

https://github.com/shpedoikal/linux.git tpmdd-03-04-13

Thanks,
Kent

> Kent
>
>>
>> Thanks,
>> Peter
>>
>>
>>