diff mbox series

[1/8,v6] tpm: refactor function names and macros for infineon v1.2 TPM

Message ID 20211107213312.263357-2-ilias.apalodimas@linaro.org
State Superseded
Headers show
Series TPM cleanups and MMIO driver | expand

Commit Message

Ilias Apalodimas Nov. 7, 2021, 9:33 p.m. UTC
With the upcoming TPM2 API, some of the function names are part of the new
header file.  So switch conflicting driver defined function names and
defines.

Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 drivers/tpm/tpm_tis_infineon.c | 34 +++++++++++++++++-----------------
 1 file changed, 17 insertions(+), 17 deletions(-)

Comments

Heinrich Schuchardt Nov. 8, 2021, 3:50 p.m. UTC | #1
On 11/7/21 22:33, Ilias Apalodimas wrote:
> With the upcoming TPM2 API, some of the function names are part of the new
> header file.  So switch conflicting driver defined function names and
> defines.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>   drivers/tpm/tpm_tis_infineon.c | 34 +++++++++++++++++-----------------
>   1 file changed, 17 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/tpm/tpm_tis_infineon.c b/drivers/tpm/tpm_tis_infineon.c
> index f414e5657db9..aa66e931bada 100644
> --- a/drivers/tpm/tpm_tis_infineon.c
> +++ b/drivers/tpm/tpm_tis_infineon.c
> @@ -50,10 +50,10 @@ static const char * const chip_name[] = {
>   	[UNKNOWN] = "unknown/fallback to slb9635",
>   };
>
> -#define	TPM_ACCESS(l)			(0x0000 | ((l) << 4))
> -#define	TPM_STS(l)			(0x0001 | ((l) << 4))
> -#define	TPM_DATA_FIFO(l)		(0x0005 | ((l) << 4))
> -#define	TPM_DID_VID(l)			(0x0006 | ((l) << 4))
> +#define	TPM1_ACCESS(l)			(0x0000 | ((l) << 4))
> +#define	TPM1_STS(l)			(0x0001 | ((l) << 4))
> +#define	TPM1_DATA_FIFO(l)		(0x0005 | ((l) << 4))
> +#define	TPM1_DID_VID(l)			(0x0006 | ((l) << 4))

To what does '1' relate here?

* Do you mean TPMv1?
* Or are the constants Infenion specific?

If the constants are TPMv1 specific, they should be in a header file.
If the constants are Infineon specific we could put _INFINEON_ into the
constant name.

Linux has resolved the conflict by putting the TPMv2 TIS constants into
drivers/char/tpm/tpm_tis_core.h which is not included by
drivers/char/tpm/tpm_i2c_infineon.c.

Best regards

Heinrich

>
>   /*
>    * tpm_tis_i2c_read() - read from TPM register
> @@ -197,7 +197,7 @@ static int tpm_tis_i2c_check_locality(struct udevice *dev, int loc)
>   	u8 buf;
>   	int rc;
>
> -	rc = tpm_tis_i2c_read(dev, TPM_ACCESS(loc), &buf, 1);
> +	rc = tpm_tis_i2c_read(dev, TPM1_ACCESS(loc), &buf, 1);
>   	if (rc < 0)
>   		return rc;
>
> @@ -215,12 +215,12 @@ static void tpm_tis_i2c_release_locality(struct udevice *dev, int loc,
>   	const u8 mask = TPM_ACCESS_REQUEST_PENDING | TPM_ACCESS_VALID;
>   	u8 buf;
>
> -	if (tpm_tis_i2c_read(dev, TPM_ACCESS(loc), &buf, 1) < 0)
> +	if (tpm_tis_i2c_read(dev, TPM1_ACCESS(loc), &buf, 1) < 0)
>   		return;
>
>   	if (force || (buf & mask) == mask) {
>   		buf = TPM_ACCESS_ACTIVE_LOCALITY;
> -		tpm_tis_i2c_write(dev, TPM_ACCESS(loc), &buf, 1);
> +		tpm_tis_i2c_write(dev, TPM1_ACCESS(loc), &buf, 1);
>   	}
>   }
>
> @@ -240,7 +240,7 @@ static int tpm_tis_i2c_request_locality(struct udevice *dev, int loc)
>   		return rc;
>   	}
>
> -	rc = tpm_tis_i2c_write(dev, TPM_ACCESS(loc), &buf, 1);
> +	rc = tpm_tis_i2c_write(dev, TPM1_ACCESS(loc), &buf, 1);
>   	if (rc) {
>   		debug("%s: Failed to write to TPM: %d\n", __func__, rc);
>   		return rc;
> @@ -271,7 +271,7 @@ static u8 tpm_tis_i2c_status(struct udevice *dev)
>   	/* NOTE: Since i2c read may fail, return 0 in this case --> time-out */
>   	u8 buf;
>
> -	if (tpm_tis_i2c_read(dev, TPM_STS(chip->locality), &buf, 1) < 0)
> +	if (tpm_tis_i2c_read(dev, TPM1_STS(chip->locality), &buf, 1) < 0)
>   		return 0;
>   	else
>   		return buf;
> @@ -286,7 +286,7 @@ static int tpm_tis_i2c_ready(struct udevice *dev)
>   	u8 buf = TPM_STS_COMMAND_READY;
>
>   	debug("%s\n", __func__);
> -	rc = tpm_tis_i2c_write_long(dev, TPM_STS(chip->locality), &buf, 1);
> +	rc = tpm_tis_i2c_write_long(dev, TPM1_STS(chip->locality), &buf, 1);
>   	if (rc)
>   		debug("%s: rc=%d\n", __func__, rc);
>
> @@ -306,7 +306,7 @@ static ssize_t tpm_tis_i2c_get_burstcount(struct udevice *dev)
>   	stop = chip->timeout_d;
>   	do {
>   		/* Note: STS is little endian */
> -		addr = TPM_STS(chip->locality) + 1;
> +		addr = TPM1_STS(chip->locality) + 1;
>   		if (tpm_tis_i2c_read(dev, addr, buf, 3) < 0)
>   			burstcnt = 0;
>   		else
> @@ -360,7 +360,7 @@ static int tpm_tis_i2c_recv_data(struct udevice *dev, u8 *buf, size_t count)
>   		if (burstcnt > (count - size))
>   			burstcnt = count - size;
>
> -		rc = tpm_tis_i2c_read(dev, TPM_DATA_FIFO(chip->locality),
> +		rc = tpm_tis_i2c_read(dev, TPM1_DATA_FIFO(chip->locality),
>   				      &(buf[size]), burstcnt);
>   		if (rc == 0)
>   			size += burstcnt;
> @@ -462,7 +462,7 @@ static int tpm_tis_i2c_send(struct udevice *dev, const u8 *buf, size_t len)
>   			burstcnt = CONFIG_TPM_TIS_I2C_BURST_LIMITATION_LEN;
>   #endif /* CONFIG_TPM_TIS_I2C_BURST_LIMITATION */
>
> -		rc = tpm_tis_i2c_write(dev, TPM_DATA_FIFO(chip->locality),
> +		rc = tpm_tis_i2c_write(dev, TPM1_DATA_FIFO(chip->locality),
>   				       &(buf[count]), burstcnt);
>   		if (rc == 0)
>   			count += burstcnt;
> @@ -482,7 +482,7 @@ static int tpm_tis_i2c_send(struct udevice *dev, const u8 *buf, size_t len)
>   	}
>
>   	/* Go and do it */
> -	rc = tpm_tis_i2c_write(dev, TPM_STS(chip->locality), &sts, 1);
> +	rc = tpm_tis_i2c_write(dev, TPM1_STS(chip->locality), &sts, 1);
>   	if (rc < 0)
>   		return rc;
>   	debug("%s: done, rc=%d\n", __func__, rc);
> @@ -525,7 +525,7 @@ static int tpm_tis_i2c_init(struct udevice *dev)
>   		return rc;
>
>   	/* Read four bytes from DID_VID register */
> -	if (tpm_tis_i2c_read(dev, TPM_DID_VID(0), (uchar *)&vendor, 4) < 0) {
> +	if (tpm_tis_i2c_read(dev, TPM1_DID_VID(0), (uchar *)&vendor, 4) < 0) {
>   		tpm_tis_i2c_release_locality(dev, 0, 1);
>   		return -EIO;
>   	}
> @@ -583,7 +583,7 @@ static int tpm_tis_i2c_close(struct udevice *dev)
>   	return 0;
>   }
>
> -static int tpm_tis_get_desc(struct udevice *dev, char *buf, int size)
> +static int tpm_tis_i2c_get_desc(struct udevice *dev, char *buf, int size)
>   {
>   	struct tpm_chip *chip = dev_get_priv(dev);
>
> @@ -615,7 +615,7 @@ static int tpm_tis_i2c_probe(struct udevice *dev)
>   static const struct tpm_ops tpm_tis_i2c_ops = {
>   	.open		= tpm_tis_i2c_open,
>   	.close		= tpm_tis_i2c_close,
> -	.get_desc	= tpm_tis_get_desc,
> +	.get_desc	= tpm_tis_i2c_get_desc,
>   	.send		= tpm_tis_i2c_send,
>   	.recv		= tpm_tis_i2c_recv,
>   	.cleanup	= tpm_tis_i2c_cleanup,
>
Simon Glass Nov. 8, 2021, 3:58 p.m. UTC | #2
On Sun, 7 Nov 2021 at 14:33, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> With the upcoming TPM2 API, some of the function names are part of the new
> header file.  So switch conflicting driver defined function names and
> defines.
>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  drivers/tpm/tpm_tis_infineon.c | 34 +++++++++++++++++-----------------
>  1 file changed, 17 insertions(+), 17 deletions(-)
>

Reviewed-by: Simon Glass <sjg@chromium.org>
Ilias Apalodimas Nov. 8, 2021, 4:28 p.m. UTC | #3
Hi Heinrich

On Mon, 8 Nov 2021 at 17:55, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> On 11/7/21 22:33, Ilias Apalodimas wrote:
> > With the upcoming TPM2 API, some of the function names are part of the new
> > header file.  So switch conflicting driver defined function names and
> > defines.
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >   drivers/tpm/tpm_tis_infineon.c | 34 +++++++++++++++++-----------------
> >   1 file changed, 17 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/tpm/tpm_tis_infineon.c b/drivers/tpm/tpm_tis_infineon.c
> > index f414e5657db9..aa66e931bada 100644
> > --- a/drivers/tpm/tpm_tis_infineon.c
> > +++ b/drivers/tpm/tpm_tis_infineon.c
> > @@ -50,10 +50,10 @@ static const char * const chip_name[] = {
> >       [UNKNOWN] = "unknown/fallback to slb9635",
> >   };
> >
> > -#define      TPM_ACCESS(l)                   (0x0000 | ((l) << 4))
> > -#define      TPM_STS(l)                      (0x0001 | ((l) << 4))
> > -#define      TPM_DATA_FIFO(l)                (0x0005 | ((l) << 4))
> > -#define      TPM_DID_VID(l)                  (0x0006 | ((l) << 4))
> > +#define      TPM1_ACCESS(l)                  (0x0000 | ((l) << 4))
> > +#define      TPM1_STS(l)                     (0x0001 | ((l) << 4))
> > +#define      TPM1_DATA_FIFO(l)               (0x0005 | ((l) << 4))
> > +#define      TPM1_DID_VID(l)                 (0x0006 | ((l) << 4))
>
> To what does '1' relate here?
>
> * Do you mean TPMv1?

Yea that was the intention

> * Or are the constants Infenion specific?

I think so.  I can't find that definition in any of the TCG specs [1]
[2] (search for 0f04 to get to the description table).

>
> If the constants are TPMv1 specific, they should be in a header file.
> If the constants are Infineon specific we could put _INFINEON_ into the
> constant name.

Ok I'll swap 1 with INFINEON

>
> Linux has resolved the conflict by putting the TPMv2 TIS constants into
> drivers/char/tpm/tpm_tis_core.h which is not included by
> drivers/char/tpm/tpm_i2c_infineon.c.

Yea and they still define them differently into that infineon TPM.

[1] https://trustedcomputinggroup.org/wp-content/uploads/TCG_PCClientTPMInterfaceSpecification_TIS__1-3_27_03212013.pdf
[2] https://trustedcomputinggroup.org/wp-content/uploads/PC-Client-Specific-Platform-TPM-Profile-for-TPM-2-0-v1-03-22-170516_final.pdf

Thanks
/Ilias
>
> Best regards
>
> Heinrich
>
> >

[...]
diff mbox series

Patch

diff --git a/drivers/tpm/tpm_tis_infineon.c b/drivers/tpm/tpm_tis_infineon.c
index f414e5657db9..aa66e931bada 100644
--- a/drivers/tpm/tpm_tis_infineon.c
+++ b/drivers/tpm/tpm_tis_infineon.c
@@ -50,10 +50,10 @@  static const char * const chip_name[] = {
 	[UNKNOWN] = "unknown/fallback to slb9635",
 };
 
-#define	TPM_ACCESS(l)			(0x0000 | ((l) << 4))
-#define	TPM_STS(l)			(0x0001 | ((l) << 4))
-#define	TPM_DATA_FIFO(l)		(0x0005 | ((l) << 4))
-#define	TPM_DID_VID(l)			(0x0006 | ((l) << 4))
+#define	TPM1_ACCESS(l)			(0x0000 | ((l) << 4))
+#define	TPM1_STS(l)			(0x0001 | ((l) << 4))
+#define	TPM1_DATA_FIFO(l)		(0x0005 | ((l) << 4))
+#define	TPM1_DID_VID(l)			(0x0006 | ((l) << 4))
 
 /*
  * tpm_tis_i2c_read() - read from TPM register
@@ -197,7 +197,7 @@  static int tpm_tis_i2c_check_locality(struct udevice *dev, int loc)
 	u8 buf;
 	int rc;
 
-	rc = tpm_tis_i2c_read(dev, TPM_ACCESS(loc), &buf, 1);
+	rc = tpm_tis_i2c_read(dev, TPM1_ACCESS(loc), &buf, 1);
 	if (rc < 0)
 		return rc;
 
@@ -215,12 +215,12 @@  static void tpm_tis_i2c_release_locality(struct udevice *dev, int loc,
 	const u8 mask = TPM_ACCESS_REQUEST_PENDING | TPM_ACCESS_VALID;
 	u8 buf;
 
-	if (tpm_tis_i2c_read(dev, TPM_ACCESS(loc), &buf, 1) < 0)
+	if (tpm_tis_i2c_read(dev, TPM1_ACCESS(loc), &buf, 1) < 0)
 		return;
 
 	if (force || (buf & mask) == mask) {
 		buf = TPM_ACCESS_ACTIVE_LOCALITY;
-		tpm_tis_i2c_write(dev, TPM_ACCESS(loc), &buf, 1);
+		tpm_tis_i2c_write(dev, TPM1_ACCESS(loc), &buf, 1);
 	}
 }
 
@@ -240,7 +240,7 @@  static int tpm_tis_i2c_request_locality(struct udevice *dev, int loc)
 		return rc;
 	}
 
-	rc = tpm_tis_i2c_write(dev, TPM_ACCESS(loc), &buf, 1);
+	rc = tpm_tis_i2c_write(dev, TPM1_ACCESS(loc), &buf, 1);
 	if (rc) {
 		debug("%s: Failed to write to TPM: %d\n", __func__, rc);
 		return rc;
@@ -271,7 +271,7 @@  static u8 tpm_tis_i2c_status(struct udevice *dev)
 	/* NOTE: Since i2c read may fail, return 0 in this case --> time-out */
 	u8 buf;
 
-	if (tpm_tis_i2c_read(dev, TPM_STS(chip->locality), &buf, 1) < 0)
+	if (tpm_tis_i2c_read(dev, TPM1_STS(chip->locality), &buf, 1) < 0)
 		return 0;
 	else
 		return buf;
@@ -286,7 +286,7 @@  static int tpm_tis_i2c_ready(struct udevice *dev)
 	u8 buf = TPM_STS_COMMAND_READY;
 
 	debug("%s\n", __func__);
-	rc = tpm_tis_i2c_write_long(dev, TPM_STS(chip->locality), &buf, 1);
+	rc = tpm_tis_i2c_write_long(dev, TPM1_STS(chip->locality), &buf, 1);
 	if (rc)
 		debug("%s: rc=%d\n", __func__, rc);
 
@@ -306,7 +306,7 @@  static ssize_t tpm_tis_i2c_get_burstcount(struct udevice *dev)
 	stop = chip->timeout_d;
 	do {
 		/* Note: STS is little endian */
-		addr = TPM_STS(chip->locality) + 1;
+		addr = TPM1_STS(chip->locality) + 1;
 		if (tpm_tis_i2c_read(dev, addr, buf, 3) < 0)
 			burstcnt = 0;
 		else
@@ -360,7 +360,7 @@  static int tpm_tis_i2c_recv_data(struct udevice *dev, u8 *buf, size_t count)
 		if (burstcnt > (count - size))
 			burstcnt = count - size;
 
-		rc = tpm_tis_i2c_read(dev, TPM_DATA_FIFO(chip->locality),
+		rc = tpm_tis_i2c_read(dev, TPM1_DATA_FIFO(chip->locality),
 				      &(buf[size]), burstcnt);
 		if (rc == 0)
 			size += burstcnt;
@@ -462,7 +462,7 @@  static int tpm_tis_i2c_send(struct udevice *dev, const u8 *buf, size_t len)
 			burstcnt = CONFIG_TPM_TIS_I2C_BURST_LIMITATION_LEN;
 #endif /* CONFIG_TPM_TIS_I2C_BURST_LIMITATION */
 
-		rc = tpm_tis_i2c_write(dev, TPM_DATA_FIFO(chip->locality),
+		rc = tpm_tis_i2c_write(dev, TPM1_DATA_FIFO(chip->locality),
 				       &(buf[count]), burstcnt);
 		if (rc == 0)
 			count += burstcnt;
@@ -482,7 +482,7 @@  static int tpm_tis_i2c_send(struct udevice *dev, const u8 *buf, size_t len)
 	}
 
 	/* Go and do it */
-	rc = tpm_tis_i2c_write(dev, TPM_STS(chip->locality), &sts, 1);
+	rc = tpm_tis_i2c_write(dev, TPM1_STS(chip->locality), &sts, 1);
 	if (rc < 0)
 		return rc;
 	debug("%s: done, rc=%d\n", __func__, rc);
@@ -525,7 +525,7 @@  static int tpm_tis_i2c_init(struct udevice *dev)
 		return rc;
 
 	/* Read four bytes from DID_VID register */
-	if (tpm_tis_i2c_read(dev, TPM_DID_VID(0), (uchar *)&vendor, 4) < 0) {
+	if (tpm_tis_i2c_read(dev, TPM1_DID_VID(0), (uchar *)&vendor, 4) < 0) {
 		tpm_tis_i2c_release_locality(dev, 0, 1);
 		return -EIO;
 	}
@@ -583,7 +583,7 @@  static int tpm_tis_i2c_close(struct udevice *dev)
 	return 0;
 }
 
-static int tpm_tis_get_desc(struct udevice *dev, char *buf, int size)
+static int tpm_tis_i2c_get_desc(struct udevice *dev, char *buf, int size)
 {
 	struct tpm_chip *chip = dev_get_priv(dev);
 
@@ -615,7 +615,7 @@  static int tpm_tis_i2c_probe(struct udevice *dev)
 static const struct tpm_ops tpm_tis_i2c_ops = {
 	.open		= tpm_tis_i2c_open,
 	.close		= tpm_tis_i2c_close,
-	.get_desc	= tpm_tis_get_desc,
+	.get_desc	= tpm_tis_i2c_get_desc,
 	.send		= tpm_tis_i2c_send,
 	.recv		= tpm_tis_i2c_recv,
 	.cleanup	= tpm_tis_i2c_cleanup,