Introduce generic TPM support in u-boot

TPM (Trusted Platform Module) is an integrated circuit and software platform that provides computer manufacturers with the core components of a subsystem used to assure authenticity, integrity and confidentiality.

drivers/tpm/generic_lpc_tpm.c new file +0 -0

new file mode 100644
index 0000000..8319286

drivers/tpm/generic_lpc_tpm.c new file +25 -0

+
+static struct lpc_tpm *lpc_tpm_dev =
+	(struct lpc_tpm *)CONFIG_TPM_TIS_BASE_ADDRESS;
+
+/* Some registers' bit field definitions */
+#define TIS_STS_VALID                  (1 << 7) /* 0x80 */
+#define TIS_STS_COMMAND_READY          (1 << 6) /* 0x40 */
+#define TIS_STS_TPM_GO                 (1 << 5) /* 0x20 */
+#define TIS_STS_DATA_AVAILABLE         (1 << 4) /* 0x10 */
+#define TIS_STS_EXPECT                 (1 << 3) /* 0x08 */
+#define TIS_STS_RESPONSE_RETRY         (1 << 1) /* 0x02 */
+
+#define TIS_ACCESS_TPM_REG_VALID_STS   (1 << 7) /* 0x80 */
+#define TIS_ACCESS_ACTIVE_LOCALITY     (1 << 5) /* 0x20 */
+#define TIS_ACCESS_BEEN_SEIZED         (1 << 4) /* 0x10 */
+#define TIS_ACCESS_SEIZE               (1 << 3) /* 0x08 */
+#define TIS_ACCESS_PENDING_REQUEST     (1 << 2) /* 0x04 */
+#define TIS_ACCESS_REQUEST_USE         (1 << 1) /* 0x02 */
+#define TIS_ACCESS_TPM_ESTABLISHMENT   (1 << 0) /* 0x01 */
+
+#define TIS_STS_BURST_COUNT_MASK       (0xffff)
+#define TIS_STS_BURST_COUNT_SHIFT      (8)
+
+/*
+ * Error value returned if a tpm register does not enter the expected

state + * after continuous polling. No actual TPM register reading ever returns ~0, + * so this value is a safe error indication to be mixed with possible status + * register values.
+ */

drivers/tpm/generic_lpc_tpm.c new file +7 -0

+#define TPM_DRIVER_ERR		(-1)
+
+ /* 1 second is plenty for anything TPM does.*/
+#define MAX_DELAY_US	(1000 * 1000)
+
+/* Retrieve burst count value out of the status register contents. */
+#define BURST_COUNT(status) ((u16)(((status) >> TIS_STS_BURST_COUNT_SHIFT)

& \ + TIS_STS_BURST_COUNT_MASK))

drivers/tpm/generic_lpc_tpm.c new file +3 -0

+
+/*
+ * Structures defined below allow creating descriptions of TPM

vendor/device + * ID information for run time discovery. The only device the system knows + * about at this time is Infineon slb9635
+ */

drivers/tpm/generic_lpc_tpm.c new file +22 -0

+struct device_name {
+	u16 dev_id;
+	const char * const dev_name;
+};
+
+struct vendor_name {
+	u16 vendor_id;
+	const char *vendor_name;
+	const struct device_name *dev_names;
+};
+
+static const struct device_name infineon_devices[] = {
+	{0xb, "SLB9635 TT 1.2"},
+	{0}
+};
+
+static const struct vendor_name vendor_names[] = {
+	{0x15d1, "Infineon", infineon_devices},
+};
+
+/*
+ * Cached vendor/device ID pair to indicate that the device has been

already + * discovered
+ */

drivers/tpm/generic_lpc_tpm.c new file +8 -0

+static u32 tis_senddata(const u8 * const data, u32 len)
+{
+	u32 offset = 0;
+	u16 burst = 0;
+	u32 max_cycles = 0;
+	u8 locality = 0;
+	u32 value;
+

drivers/tpm/generic_lpc_tpm.c new file +85 -0

+		unsigned count;
+
+		/* Wait till the device is ready to accept more data. */
+		while (!burst) {
+			if (max_cycles++ == MAX_DELAY_US) {
+				printf("%s:%d failed to feed %d bytes of %d\n",
+				       __FILE__, __LINE__, len - offset, len);
+				return TPM_DRIVER_ERR;
+			}
+			udelay(1);
+			burst = BURST_COUNT(tpm_read(&lpc_tpm_dev->locality
+						     [locality].tpm_status));
+		}
+
+		max_cycles = 0;
+
+		/*
+		 * Calculate number of bytes the TPM is ready to accept in one
+		 * shot.
+		 *
+		 * We want to send the last byte outside of the loop (hence
+		 * the -1 below) to make sure that the 'expected' status bit
+		 * changes to zero exactly after the last byte is fed into the
+		 * FIFO.
+		 */
+		count = min(burst, len - offset - 1);
+		while (count--)
+			tpm_write(data[offset++],
+				  &lpc_tpm_dev->locality[locality].data);
+
+		value = tis_wait_reg(&lpc_tpm_dev->locality
+				     [locality].tpm_status,
+				     TIS_STS_VALID, TIS_STS_VALID);
+
+		if ((value == TPM_TIMEOUT_ERR) || !(value & TIS_STS_EXPECT)) {
+			printf("%s:%d TPM command feed overflow\n",
+			       __FILE__, __LINE__);
+			return TPM_DRIVER_ERR;
+		}
+
+		burst = BURST_COUNT(value);
+		if ((offset == (len - 1)) && burst)
+			/*
+			 * We need to be able to send the last byte to the
+			 * device, so burst size must be nonzero before we
+			 * break out.
+			 */
+			break;
+	}
+
+	/* Send the last byte. */
+	tpm_write(data[offset++], &lpc_tpm_dev->locality[locality].data);
+	/*
+	 * Verify that TPM does not expect any more data as part of this
+	 * command.
+	 */
+	value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,
+			     TIS_STS_VALID, TIS_STS_VALID);
+	if ((value == TPM_TIMEOUT_ERR) || (value & TIS_STS_EXPECT)) {
+		printf("%s:%d unexpected TPM status 0x%x\n",
+		       __FILE__, __LINE__, value);
+		return TPM_DRIVER_ERR;
+	}
+
+	/* OK, sitting pretty, let's start the command execution. */
+	tpm_write(TIS_STS_TPM_GO, &lpc_tpm_dev->locality[locality].tpm_status);
+	return 0;
+}
+
+/*
+ * tis_readresponse()
+ *
+ * read the TPM device response after a command was issued.
+ *
+ * @buffer - address where to read the response, byte by byte.
+ * @len - pointer to the size of buffer
+ *
+ * On success stores the number of received bytes to len and returns 0. On
+ * errors (misformatted TPM data or synchronization problems) returns
+ * TPM_DRIVER_ERR.
+ */
+static u32 tis_readresponse(u8 *buffer, u32 *len)
+{
+	u16 burst_count;
+	u32 status;

drivers/tpm/generic_lpc_tpm.c new file +51 -0

+		printf("%s:%d failed processing command\n",
+		       __FILE__, __LINE__);
+		return TPM_DRIVER_ERR;
+	}
+
+	do {
+		while ((burst_count = BURST_COUNT(status)) == 0) {
+			if (max_cycles++ == MAX_DELAY_US) {
+				printf("%s:%d TPM stuck on read\n",
+				       __FILE__, __LINE__);
+				return TPM_DRIVER_ERR;
+			}
+			udelay(1);
+			status = tpm_read(&lpc_tpm_dev->locality
+					  [locality].tpm_status);
+		}
+
+		max_cycles = 0;
+
+		while (burst_count-- && (offset < expected_count)) {
+			buffer[offset++] = (u8) tpm_read(&lpc_tpm_dev->locality
+							 [locality].data);
+
+			if (offset == 6) {
+				/*
+				 * We got the first six bytes of the reply,
+				 * let's figure out how many bytes to expect
+				 * total - it is stored as a 4 byte number in
+				 * network order, starting with offset 2 into
+				 * the body of the reply.
+				 */
+				u32 real_length;
+				memcpy(&real_length,
+				       buffer + 2,
+				       sizeof(real_length));
+				expected_count = be32_to_cpu(real_length);
+
+				if ((expected_count < offset) ||
+				    (expected_count > *len)) {
+					printf("%s:%d bad response size %d\n",
+					       __FILE__, __LINE__,
+					       expected_count);
+					return TPM_DRIVER_ERR;
+				}
+			}
+		}
+
+		/* Wait for the next portion */
+		status = tis_wait_reg(&lpc_tpm_dev->locality
+				      [locality].tpm_status,
+				      TIS_STS_VALID, TIS_STS_VALID);

drivers/tpm/generic_lpc_tpm.c new file +12 -0

+
+	/*
+	 * Make sure we indeed read all there was. The TIS_STS_VALID bit is
+	 * known to be set.
+	 */
+	if (status & TIS_STS_DATA_AVAILABLE) {
+		printf("%s:%d wrong receive status %x\n",
+		       __FILE__, __LINE__, status);
+		return TPM_DRIVER_ERR;
+	}
+
+	/* Tell the TPM that we are done. */

drivers/tpm/generic_lpc_tpm.c new file +16 -0

+
+int tis_open(void)
+{
+	u8 locality = 0; /* we use locality zero for everything */
+
+	if (tis_close())
+		return TPM_DRIVER_ERR;
+
+	/* now request access to locality */
+	tpm_write(TIS_ACCESS_REQUEST_USE,
+		  &lpc_tpm_dev->locality[locality].access);
+
+
+	/* did we get a lock? */
+	if (tis_wait_reg(&lpc_tpm_dev->locality[locality].access,
+			 TIS_ACCESS_ACTIVE_LOCALITY,

Quoted 4 lines · Vadim Bendebury

TPM (Trusted Platform Module) is an integrated circuit and software platform that provides computer manufacturers with the core components of a subsystem used to assure authenticity, integrity and confidentiality.

[...]

Quick points:
* The license

* Use debug() when fitting

Quoted 5 lines

drivers/tpm/generic_lpc_tpm.c new file +0 -0

new file mode 100644
index 0000000..8319286

[...]

drivers/tpm/generic_lpc_tpm.c line 59

Quoted 4 lines

drivers/tpm/generic_lpc_tpm.c new file +4 -0

+
+struct lpc_tpm {
+ ? ? struct tpm_locality locality[TPM_TOTAL_LOCALITIES];
+};

Do you need such envelope ?

Quoted 26 lines

drivers/tpm/generic_lpc_tpm.c new file +25 -0

+
+static struct lpc_tpm *lpc_tpm_dev =
+ ? ? (struct lpc_tpm *)CONFIG_TPM_TIS_BASE_ADDRESS;
+
+/* Some registers' bit field definitions */
+#define TIS_STS_VALID ? ? ? ? ? ? ? ? ?(1 << 7) /* 0x80 */
+#define TIS_STS_COMMAND_READY ? ? ? ? ?(1 << 6) /* 0x40 */
+#define TIS_STS_TPM_GO ? ? ? ? ? ? ? ? (1 << 5) /* 0x20 */
+#define TIS_STS_DATA_AVAILABLE ? ? ? ? (1 << 4) /* 0x10 */
+#define TIS_STS_EXPECT ? ? ? ? ? ? ? ? (1 << 3) /* 0x08 */
+#define TIS_STS_RESPONSE_RETRY ? ? ? ? (1 << 1) /* 0x02 */
+
+#define TIS_ACCESS_TPM_REG_VALID_STS ? (1 << 7) /* 0x80 */
+#define TIS_ACCESS_ACTIVE_LOCALITY ? ? (1 << 5) /* 0x20 */
+#define TIS_ACCESS_BEEN_SEIZED ? ? ? ? (1 << 4) /* 0x10 */
+#define TIS_ACCESS_SEIZE ? ? ? ? ? ? ? (1 << 3) /* 0x08 */
+#define TIS_ACCESS_PENDING_REQUEST ? ? (1 << 2) /* 0x04 */
+#define TIS_ACCESS_REQUEST_USE ? ? ? ? (1 << 1) /* 0x02 */
+#define TIS_ACCESS_TPM_ESTABLISHMENT ? (1 << 0) /* 0x01 */
+
+#define TIS_STS_BURST_COUNT_MASK ? ? ? (0xffff)
+#define TIS_STS_BURST_COUNT_SHIFT ? ? ?(8)
+
+/*
+ * Error value returned if a tpm register does not enter the expected

state + * after continuous polling. No actual TPM register reading ever

drivers/tpm/generic_lpc_tpm.c line 91

Quoted 6 lines

returns ~0, + * so this value is a safe error indication to be mixed with possible status + * register values.
+ */

drivers/tpm/generic_lpc_tpm.c new file +3 -0

+#define TPM_TIMEOUT_ERR ? ? ? ? ? ? ? ? ? ? ?(~0)
+
+/* Error value returned on various TPM driver errors */

Dot missing at the end.

Quoted 8 lines

drivers/tpm/generic_lpc_tpm.c new file +7 -0

+#define TPM_DRIVER_ERR ? ? ? ? ? ? ? (-1)
+
+ /* 1 second is plenty for anything TPM does.*/
+#define MAX_DELAY_US (1000 * 1000)
+
+/* Retrieve burst count value out of the status register contents. */
+#define BURST_COUNT(status) ((u16)(((status) >> TIS_STS_BURST_COUNT_SHIFT)

& \ + ? ? ? ? ? ? ? ? ? ? ? ? ? ?TIS_STS_BURST_COUNT_MASK))

Do you need the cast ?

Quoted 30 lines

drivers/tpm/generic_lpc_tpm.c new file +3 -0

+
+/*
+ * Structures defined below allow creating descriptions of TPM

vendor/device + * ID information for run time discovery. The only device the system knows + * about at this time is Infineon slb9635
+ */

drivers/tpm/generic_lpc_tpm.c new file +22 -0

+struct device_name {
+ ? ? u16 dev_id;
+ ? ? const char * const dev_name;
+};
+
+struct vendor_name {
+ ? ? u16 vendor_id;
+ ? ? const char *vendor_name;
+ ? ? const struct device_name *dev_names;
+};
+
+static const struct device_name infineon_devices[] = {
+ ? ? {0xb, "SLB9635 TT 1.2"},
+ ? ? {0}
+};
+
+static const struct vendor_name vendor_names[] = {
+ ? ? {0x15d1, "Infineon", infineon_devices},
+};
+
+/*
+ * Cached vendor/device ID pair to indicate that the device has been

already + * discovered
+ */

drivers/tpm/generic_lpc_tpm.c line 139

Quoted 10 lines

drivers/tpm/generic_lpc_tpm.c new file +10 -0

+static u32 vendor_dev_id;
+
+/* TPM access going through macros to make tracing easier. */
+#define tpm_read(ptr) ({ \
+ ? ? u32 ?__ret; \
+ ? ? __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
+ ? ? ?debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
+ ? ? ? ? ? ?(u32)ptr - (u32)lpc_tpm_dev, __ret); \
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?__ret; })
+

Make this inline function

drivers/tpm/generic_lpc_tpm.c

Quoted 9 lines

drivers/tpm/generic_lpc_tpm.c new file +9 -0

+#define tpm_write(value, ptr) ({ ? ? ? ? ? ? ? ?\
+ ? ? u32 __v = value; ? ? ? ?\
+ ? ? debug(PREFIX "Write reg 0x%x with 0x%x\n", \
+ ? ? ? ? ? ?(u32)ptr - (u32)lpc_tpm_dev, __v); \
+ ? ? if (sizeof(*ptr) == 1) \
+ ? ? ? ? ? ? writeb(__v, ptr); \
+ ? ? else \
+ ? ? ? ? ? ? writel(__v, ptr); })
+

DTTO

[...]

Quoted 12 lines

drivers/tpm/generic_lpc_tpm.c new file +12 -0

+static u32 tis_senddata(const u8 * const data, u32 len)
+{
+ ? ? u32 offset = 0;
+ ? ? u16 burst = 0;
+ ? ? u32 max_cycles = 0;
+ ? ? u8 locality = 0;
+ ? ? u32 value;
+
+ ? ? value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,
+ ? ? ? ? ? ? ? ? ? ? ? ? ?TIS_STS_COMMAND_READY, TIS_STS_COMMAND_READY);
+ ? ? if (value == TPM_TIMEOUT_ERR) {
+ ? ? ? ? ? ? printf("%s:%d - failed to get 'command_ready' status\n",

Quoted 6 lines

+ ? ? ? ? ? ? ? ? ? ?__FILE__, __LINE__);
+ ? ? ? ? ? ? return TPM_DRIVER_ERR;
+ ? ? }
+ ? ? burst = BURST_COUNT(value);
+
+ ? ? while (1) {

No endless loops please.

Quoted 90 lines

+ ? ? ? ? ? ? unsigned count;

drivers/tpm/generic_lpc_tpm.c new file +89 -0

+
+ ? ? ? ? ? ? /* Wait till the device is ready to accept more data. */
+ ? ? ? ? ? ? while (!burst) {
+ ? ? ? ? ? ? ? ? ? ? if (max_cycles++ == MAX_DELAY_US) {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? printf("%s:%d failed to feed %d bytes of %d\n",
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?__FILE__, __LINE__, len - offset, len);
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? return TPM_DRIVER_ERR;
+ ? ? ? ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? ? ? ? udelay(1);
+ ? ? ? ? ? ? ? ? ? ? burst = BURST_COUNT(tpm_read(&lpc_tpm_dev->locality
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?[locality].tpm_status));
+ ? ? ? ? ? ? }
+
+ ? ? ? ? ? ? max_cycles = 0;
+
+ ? ? ? ? ? ? /*
+ ? ? ? ? ? ? ?* Calculate number of bytes the TPM is ready to accept in one
+ ? ? ? ? ? ? ?* shot.
+ ? ? ? ? ? ? ?*
+ ? ? ? ? ? ? ?* We want to send the last byte outside of the loop (hence
+ ? ? ? ? ? ? ?* the -1 below) to make sure that the 'expected' status bit
+ ? ? ? ? ? ? ?* changes to zero exactly after the last byte is fed into the
+ ? ? ? ? ? ? ?* FIFO.
+ ? ? ? ? ? ? ?*/
+ ? ? ? ? ? ? count = min(burst, len - offset - 1);
+ ? ? ? ? ? ? while (count--)
+ ? ? ? ? ? ? ? ? ? ? tpm_write(data[offset++],
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &lpc_tpm_dev->locality[locality].data);
+
+ ? ? ? ? ? ? value = tis_wait_reg(&lpc_tpm_dev->locality
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?[locality].tpm_status,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?TIS_STS_VALID, TIS_STS_VALID);
+
+ ? ? ? ? ? ? if ((value == TPM_TIMEOUT_ERR) || !(value & TIS_STS_EXPECT)) {
+ ? ? ? ? ? ? ? ? ? ? printf("%s:%d TPM command feed overflow\n",
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ?__FILE__, __LINE__);
+ ? ? ? ? ? ? ? ? ? ? return TPM_DRIVER_ERR;
+ ? ? ? ? ? ? }
+
+ ? ? ? ? ? ? burst = BURST_COUNT(value);
+ ? ? ? ? ? ? if ((offset == (len - 1)) && burst)
+ ? ? ? ? ? ? ? ? ? ? /*
+ ? ? ? ? ? ? ? ? ? ? ?* We need to be able to send the last byte to the
+ ? ? ? ? ? ? ? ? ? ? ?* device, so burst size must be nonzero before we
+ ? ? ? ? ? ? ? ? ? ? ?* break out.
+ ? ? ? ? ? ? ? ? ? ? ?*/
+ ? ? ? ? ? ? ? ? ? ? break;
+ ? ? }
+
+ ? ? /* Send the last byte. */
+ ? ? tpm_write(data[offset++], &lpc_tpm_dev->locality[locality].data);
+ ? ? /*
+ ? ? ?* Verify that TPM does not expect any more data as part of this
+ ? ? ?* command.
+ ? ? ?*/
+ ? ? value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,
+ ? ? ? ? ? ? ? ? ? ? ? ? ?TIS_STS_VALID, TIS_STS_VALID);
+ ? ? if ((value == TPM_TIMEOUT_ERR) || (value & TIS_STS_EXPECT)) {
+ ? ? ? ? ? ? printf("%s:%d unexpected TPM status 0x%x\n",
+ ? ? ? ? ? ? ? ? ? ?__FILE__, __LINE__, value);
+ ? ? ? ? ? ? return TPM_DRIVER_ERR;
+ ? ? }
+
+ ? ? /* OK, sitting pretty, let's start the command execution. */
+ ? ? tpm_write(TIS_STS_TPM_GO, &lpc_tpm_dev->locality[locality].tpm_status);
+ ? ? return 0;
+}
+
+/*
+ * tis_readresponse()
+ *
+ * read the TPM device response after a command was issued.
+ *
+ * @buffer - address where to read the response, byte by byte.
+ * @len - pointer to the size of buffer
+ *
+ * On success stores the number of received bytes to len and returns 0. On
+ * errors (misformatted TPM data or synchronization problems) returns
+ * TPM_DRIVER_ERR.
+ */
+static u32 tis_readresponse(u8 *buffer, u32 *len)
+{
+ ? ? u16 burst_count;
+ ? ? u32 status;
+ ? ? u32 offset = 0;
+ ? ? u8 locality = 0;
+ ? ? const u32 has_data = TIS_STS_DATA_AVAILABLE | TIS_STS_VALID;
+ ? ? u32 expected_count = *len;
+ ? ? int max_cycles = 0;

Quoted 5 lines

+
+ ? ? /* Wait for the TPM to process the command */
+ ? ? status = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? has_data, has_data);
+ ? ? if (status == TPM_TIMEOUT_ERR) {

Just make it return non-zero for timeout and check for non-zero. And unify the variable names please.

Quoted 55 lines

+ ? ? ? ? ? ? printf("%s:%d failed processing command\n",
+ ? ? ? ? ? ? ? ? ? ?__FILE__, __LINE__);
+ ? ? ? ? ? ? return TPM_DRIVER_ERR;
+ ? ? }

drivers/tpm/generic_lpc_tpm.c new file +51 -0

+
+ ? ? do {
+ ? ? ? ? ? ? while ((burst_count = BURST_COUNT(status)) == 0) {
+ ? ? ? ? ? ? ? ? ? ? if (max_cycles++ == MAX_DELAY_US) {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? printf("%s:%d TPM stuck on read\n",
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?__FILE__, __LINE__);
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? return TPM_DRIVER_ERR;
+ ? ? ? ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? ? ? ? udelay(1);
+ ? ? ? ? ? ? ? ? ? ? status = tpm_read(&lpc_tpm_dev->locality
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? [locality].tpm_status);
+ ? ? ? ? ? ? }
+
+ ? ? ? ? ? ? max_cycles = 0;
+
+ ? ? ? ? ? ? while (burst_count-- && (offset < expected_count)) {
+ ? ? ? ? ? ? ? ? ? ? buffer[offset++] = (u8) tpm_read(&lpc_tpm_dev->locality
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?[locality].data);
+
+ ? ? ? ? ? ? ? ? ? ? if (offset == 6) {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? /*
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?* We got the first six bytes of the reply,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?* let's figure out how many bytes to expect
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?* total - it is stored as a 4 byte number in
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?* network order, starting with offset 2 into
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?* the body of the reply.
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?*/
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? u32 real_length;
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? memcpy(&real_length,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?buffer + 2,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?sizeof(real_length));
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? expected_count = be32_to_cpu(real_length);
+
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? if ((expected_count < offset) ||
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (expected_count > *len)) {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? printf("%s:%d bad response size %d\n",
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?__FILE__, __LINE__,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?expected_count);
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? return TPM_DRIVER_ERR;
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? }
+
+ ? ? ? ? ? ? /* Wait for the next portion */
+ ? ? ? ? ? ? status = tis_wait_reg(&lpc_tpm_dev->locality
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? [locality].tpm_status,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? TIS_STS_VALID, TIS_STS_VALID);
+ ? ? ? ? ? ? if (status == TPM_TIMEOUT_ERR) {
+ ? ? ? ? ? ? ? ? ? ? printf("%s:%d failed to read response\n",
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ?__FILE__, __LINE__);
+ ? ? ? ? ? ? ? ? ? ? return TPM_DRIVER_ERR;

drivers/tpm/generic_lpc_tpm.c

Quoted 6 lines

+ ? ? ? ? ? ? }

drivers/tpm/generic_lpc_tpm.c new file +5 -0

+
+ ? ? ? ? ? ? if (offset == expected_count)
+ ? ? ? ? ? ? ? ? ? ? break; ?/* We got all we need */
+
+ ? ? } while ((status & has_data) == has_data);

No endless loop please.

Quoted 16 lines

drivers/tpm/generic_lpc_tpm.c new file +16 -0

+
+ ? ? /*
+ ? ? ?* Make sure we indeed read all there was. The TIS_STS_VALID bit is
+ ? ? ?* known to be set.
+ ? ? ?*/
+ ? ? if (status & TIS_STS_DATA_AVAILABLE) {
+ ? ? ? ? ? ? printf("%s:%d wrong receive status %x\n",
+ ? ? ? ? ? ? ? ? ? ?__FILE__, __LINE__, status);
+ ? ? ? ? ? ? return TPM_DRIVER_ERR;
+ ? ? }
+
+ ? ? /* Tell the TPM that we are done. */
+ ? ? tpm_write(TIS_STS_COMMAND_READY, &lpc_tpm_dev->locality
+ ? ? ? ? ? ? ? [locality].tpm_status);
+ ? ? *len = offset;
+ ? ? return 0;

drivers/tpm/generic_lpc_tpm.c line 430

Quoted 6 lines

drivers/tpm/generic_lpc_tpm.c new file +6 -0

+}
+
+int tis_init(void)
+{
+ ? ? return tis_probe();
+}

Make tis_probe into tis_init and be done with it ?

Quoted 26 lines

drivers/tpm/generic_lpc_tpm.c new file +26 -0

+
+int tis_open(void)
+{
+ ? ? u8 locality = 0; /* we use locality zero for everything */
+
+ ? ? if (tis_close())
+ ? ? ? ? ? ? return TPM_DRIVER_ERR;
+
+ ? ? /* now request access to locality */
+ ? ? tpm_write(TIS_ACCESS_REQUEST_USE,
+ ? ? ? ? ? ? ? &lpc_tpm_dev->locality[locality].access);
+
+
+ ? ? /* did we get a lock? */
+ ? ? if (tis_wait_reg(&lpc_tpm_dev->locality[locality].access,
+ ? ? ? ? ? ? ? ? ? ? ?TIS_ACCESS_ACTIVE_LOCALITY,
+ ? ? ? ? ? ? ? ? ? ? ?TIS_ACCESS_ACTIVE_LOCALITY) == TPM_TIMEOUT_ERR) {
+ ? ? ? ? ? ? printf("%s:%d - failed to lock locality %d\n",
+ ? ? ? ? ? ? ? ? ? ?__FILE__, __LINE__, locality);
+ ? ? ? ? ? ? return TPM_DRIVER_ERR;
+ ? ? }
+
+ ? ? tpm_write(TIS_STS_COMMAND_READY,
+ ? ? ? ? ? ? ? &lpc_tpm_dev->locality[locality].tpm_status);
+ ? ? return 0;
+}

[...]

Cheers

Dear Marek Vasut,

thank you for your comments, please see below:

Quoted 6 lines · Marek Vasut

Quoted 4 lines · Vadim Bendebury

TPM (Trusted Platform Module) is an integrated circuit and software platform that provides computer manufacturers with the core components of a subsystem used to assure authenticity, integrity and confidentiality.

Quoted 4 lines

[...]

Quick points:
* The license

Please suggest the appropriate file header text.

Quoted 2 lines

* Use debug() when fitting

It seems to me debug/puts/printf are used where appropriate - do you have some cases in mind which need to be changed?

Quoted 9 lines

Quoted 6 lines

(quoted diff) +0 -0

b/drivers/tpm/generic_lpc_tpm.c new file mode 100644 index 0000000..8319286

drivers/tpm/generic_lpc_tpm.c new file +0 -0

[...]

Quoted 6 lines

drivers/tpm/generic_lpc_tpm.c line 59

Quoted 4 lines

drivers/tpm/generic_lpc_tpm.c new file +4 -0

+
+struct lpc_tpm {
+     struct tpm_locality locality[TPM_TOTAL_LOCALITIES];
+};

Do you need such envelope ?

I think I do - this accurately describes the structure of the chip.

Quoted 27 lines

Quoted 27 lines

drivers/tpm/generic_lpc_tpm.c new file +25 -0

+
+static struct lpc_tpm *lpc_tpm_dev =
+     (struct lpc_tpm *)CONFIG_TPM_TIS_BASE_ADDRESS;
+
+/* Some registers' bit field definitions */
+#define TIS_STS_VALID                  (1 << 7) /* 0x80 */
+#define TIS_STS_COMMAND_READY          (1 << 6) /* 0x40 */
+#define TIS_STS_TPM_GO                 (1 << 5) /* 0x20 */
+#define TIS_STS_DATA_AVAILABLE         (1 << 4) /* 0x10 */
+#define TIS_STS_EXPECT                 (1 << 3) /* 0x08 */
+#define TIS_STS_RESPONSE_RETRY         (1 << 1) /* 0x02 */
+
+#define TIS_ACCESS_TPM_REG_VALID_STS   (1 << 7) /* 0x80 */
+#define TIS_ACCESS_ACTIVE_LOCALITY     (1 << 5) /* 0x20 */
+#define TIS_ACCESS_BEEN_SEIZED         (1 << 4) /* 0x10 */
+#define TIS_ACCESS_SEIZE               (1 << 3) /* 0x08 */
+#define TIS_ACCESS_PENDING_REQUEST     (1 << 2) /* 0x04 */
+#define TIS_ACCESS_REQUEST_USE         (1 << 1) /* 0x02 */
+#define TIS_ACCESS_TPM_ESTABLISHMENT   (1 << 0) /* 0x01 */
+
+#define TIS_STS_BURST_COUNT_MASK       (0xffff)
+#define TIS_STS_BURST_COUNT_SHIFT      (8)
+
+/*
+ * Error value returned if a tpm register does not enter the expected

state + * after continuous polling. No actual TPM register reading ever

Quoted 8 lines

drivers/tpm/generic_lpc_tpm.c line 91

Quoted 6 lines

returns ~0, + * so this value is a safe error indication to be mixed with possible status + * register values.
+ */

drivers/tpm/generic_lpc_tpm.c new file +3 -0

+#define TPM_TIMEOUT_ERR                      (~0)
+
+/* Error value returned on various TPM driver errors */

Dot missing at the end.

ok.

Quoted 12 lines

Quoted 10 lines

drivers/tpm/generic_lpc_tpm.c new file +7 -0

+#define TPM_DRIVER_ERR               (-1)
+
+ /* 1 second is plenty for anything TPM does.*/
+#define MAX_DELAY_US (1000 * 1000)
+
+/* Retrieve burst count value out of the status register contents. */
+#define BURST_COUNT(status) ((u16)(((status) >>

TIS_STS_BURST_COUNT_SHIFT) & \ +
TIS_STS_BURST_COUNT_MASK))

Do you need the cast ?

I think it demonstrates the intentional truncation of the value, it gets assigned to u16 values down the road, prevents compiler warnings about assigning incompatible values in some cases.

Quoted 44 lines

Quoted 31 lines

drivers/tpm/generic_lpc_tpm.c new file +3 -0

+
+/*
+ * Structures defined below allow creating descriptions of TPM

vendor/device + * ID information for run time discovery. The only device the system knows + * about at this time is Infineon slb9635
+ */

drivers/tpm/generic_lpc_tpm.c new file +22 -0

+struct device_name {
+     u16 dev_id;
+     const char * const dev_name;
+};
+
+struct vendor_name {
+     u16 vendor_id;
+     const char *vendor_name;
+     const struct device_name *dev_names;
+};
+
+static const struct device_name infineon_devices[] = {
+     {0xb, "SLB9635 TT 1.2"},
+     {0}
+};
+
+static const struct vendor_name vendor_names[] = {
+     {0x15d1, "Infineon", infineon_devices},
+};
+
+/*
+ * Cached vendor/device ID pair to indicate that the device has been

already + * discovered
+ */

drivers/tpm/generic_lpc_tpm.c line 139

Quoted 10 lines

drivers/tpm/generic_lpc_tpm.c new file +10 -0

+static u32 vendor_dev_id;
+
+/* TPM access going through macros to make tracing easier. */
+#define tpm_read(ptr) ({ \
+     u32  __ret; \
+     __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
+      debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
+            (u32)ptr - (u32)lpc_tpm_dev, __ret); \
+                                      __ret; })
+

Make this inline function

Quoted 11 lines

drivers/tpm/generic_lpc_tpm.c line 148

Quoted 9 lines

drivers/tpm/generic_lpc_tpm.c new file +9 -0

+#define tpm_write(value, ptr) ({                \
+     u32 __v = value;        \
+     debug(PREFIX "Write reg 0x%x with 0x%x\n", \
+            (u32)ptr - (u32)lpc_tpm_dev, __v); \
+     if (sizeof(*ptr) == 1) \
+             writeb(__v, ptr); \
+     else \
+             writel(__v, ptr); })
+

DTTO

Are you sure these will work as inline functions?

Quoted 15 lines

[...]

Quoted 12 lines

drivers/tpm/generic_lpc_tpm.c new file +10 -0

+static u32 tis_senddata(const u8 * const data, u32 len)
+{
+     u32 offset = 0;
+     u16 burst = 0;
+     u32 max_cycles = 0;
+     u8 locality = 0;
+     u32 value;
+
+     value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,
+                          TIS_STS_COMMAND_READY,

TIS_STS_COMMAND_READY); + if (value == TPM_TIMEOUT_ERR) {
+ printf("%s:%d - failed to get 'command_ready' status\n",

Quoted 8 lines

drivers/tpm/generic_lpc_tpm.c line 252

Quoted 6 lines

drivers/tpm/generic_lpc_tpm.c new file +6 -0

+                    __FILE__, __LINE__);
+             return TPM_DRIVER_ERR;
+     }
+     burst = BURST_COUNT(value);
+
+     while (1) {

No endless loops please.

You are the third person looking at this, but the first one uncomfortable with this. Obviously this is not an endless loop, there are three points where the loop terminates, two on timeout errors and one on success.

Quoted 91 lines

Quoted 91 lines

drivers/tpm/generic_lpc_tpm.c new file +6 -0

+             unsigned count;
+
+             /* Wait till the device is ready to accept more data. */
+             while (!burst) {
+                     if (max_cycles++ == MAX_DELAY_US) {
+                             printf("%s:%d failed to feed %d bytes of

%d\n", + __FILE__, __LINE__, len -
offset, len); + return TPM_DRIVER_ERR;

drivers/tpm/generic_lpc_tpm.c new file +3 -0

+                     }
+                     udelay(1);
+                     burst =

BURST_COUNT(tpm_read(&lpc_tpm_dev->locality +

                       [locality].tpm_status)); +             }

drivers/tpm/generic_lpc_tpm.c new file +5 -0

+
+             max_cycles = 0;
+
+             /*
+              * Calculate number of bytes the TPM is ready to accept in

one + * shot.

drivers/tpm/generic_lpc_tpm.c new file +2 -0

+              *
+              * We want to send the last byte outside of the loop

(hence + * the -1 below) to make sure that the 'expected'
status bit + * changes to zero exactly after the last byte
is fed into the + * FIFO.

drivers/tpm/generic_lpc_tpm.c new file +11 -0

+              */
+             count = min(burst, len - offset - 1);
+             while (count--)
+                     tpm_write(data[offset++],
+                               &lpc_tpm_dev->locality[locality].data);
+
+             value = tis_wait_reg(&lpc_tpm_dev->locality
+                                  [locality].tpm_status,
+                                  TIS_STS_VALID, TIS_STS_VALID);
+
+             if ((value == TPM_TIMEOUT_ERR) || !(value &

TIS_STS_EXPECT)) { + printf("%s:%d TPM command feed
overflow\n", + __FILE__, __LINE__);

drivers/tpm/generic_lpc_tpm.c new file +29 -0

+                     return TPM_DRIVER_ERR;
+             }
+
+             burst = BURST_COUNT(value);
+             if ((offset == (len - 1)) && burst)
+                     /*
+                      * We need to be able to send the last byte to the
+                      * device, so burst size must be nonzero before we
+                      * break out.
+                      */
+                     break;
+     }
+
+     /* Send the last byte. */
+     tpm_write(data[offset++], &lpc_tpm_dev->locality[locality].data);
+     /*
+      * Verify that TPM does not expect any more data as part of this
+      * command.
+      */
+     value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,
+                          TIS_STS_VALID, TIS_STS_VALID);
+     if ((value == TPM_TIMEOUT_ERR) || (value & TIS_STS_EXPECT)) {
+             printf("%s:%d unexpected TPM status 0x%x\n",
+                    __FILE__, __LINE__, value);
+             return TPM_DRIVER_ERR;
+     }
+
+     /* OK, sitting pretty, let's start the command execution. */
+     tpm_write(TIS_STS_TPM_GO,

&lpc_tpm_dev->locality[locality].tpm_status); + return 0;

drivers/tpm/generic_lpc_tpm.c new file +11 -0

+}
+
+/*
+ * tis_readresponse()
+ *
+ * read the TPM device response after a command was issued.
+ *
+ * @buffer - address where to read the response, byte by byte.
+ * @len - pointer to the size of buffer
+ *
+ * On success stores the number of received bytes to len and returns 0.

On + * errors (misformatted TPM data or synchronization problems) returns + * TPM_DRIVER_ERR.
+ */

drivers/tpm/generic_lpc_tpm.c new file +9 -0

+static u32 tis_readresponse(u8 *buffer, u32 *len)
+{
+     u16 burst_count;
+     u32 status;
+     u32 offset = 0;
+     u8 locality = 0;
+     const u32 has_data = TIS_STS_DATA_AVAILABLE | TIS_STS_VALID;
+     u32 expected_count = *len;
+     int max_cycles = 0;

Quoted 8 lines

drivers/tpm/generic_lpc_tpm.c line 347

Quoted 5 lines

drivers/tpm/generic_lpc_tpm.c new file +5 -0

+
+     /* Wait for the TPM to process the command */
+     status = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,
+                           has_data, has_data);
+     if (status == TPM_TIMEOUT_ERR) {

Just make it return non-zero for timeout and check for non-zero. And unify the variable names please.

What variable names are you referring to, can you please elaborate.

Quoted 57 lines

Quoted 57 lines

drivers/tpm/generic_lpc_tpm.c new file +21 -0

+             printf("%s:%d failed processing command\n",
+                    __FILE__, __LINE__);
+             return TPM_DRIVER_ERR;
+     }
+
+     do {
+             while ((burst_count = BURST_COUNT(status)) == 0) {
+                     if (max_cycles++ == MAX_DELAY_US) {
+                             printf("%s:%d TPM stuck on read\n",
+                                    __FILE__, __LINE__);
+                             return TPM_DRIVER_ERR;
+                     }
+                     udelay(1);
+                     status = tpm_read(&lpc_tpm_dev->locality
+                                       [locality].tpm_status);
+             }
+
+             max_cycles = 0;
+
+             while (burst_count-- && (offset < expected_count)) {
+                     buffer[offset++] = (u8)

tpm_read(&lpc_tpm_dev->locality +

               [locality].data); +

drivers/tpm/generic_lpc_tpm.c new file +3 -0

+                     if (offset == 6) {
+                             /*
+                              * We got the first six bytes of the

reply, + * let's figure out how many bytes
to expect + * total - it is stored as a 4
byte number in + * network order, starting
with offset 2 into + * the body of the
reply.

drivers/tpm/generic_lpc_tpm.c new file +10 -0

+                              */
+                             u32 real_length;
+                             memcpy(&real_length,
+                                    buffer + 2,
+                                    sizeof(real_length));
+                             expected_count = be32_to_cpu(real_length);
+
+                             if ((expected_count < offset) ||
+                                 (expected_count > *len)) {
+                                     printf("%s:%d bad response size

%d\n", + __FILE__, __LINE__,

drivers/tpm/generic_lpc_tpm.c new file +14 -0

+                                            expected_count);
+                                     return TPM_DRIVER_ERR;
+                             }
+                     }
+             }
+
+             /* Wait for the next portion */
+             status = tis_wait_reg(&lpc_tpm_dev->locality
+                                   [locality].tpm_status,
+                                   TIS_STS_VALID, TIS_STS_VALID);
+             if (status == TPM_TIMEOUT_ERR) {
+                     printf("%s:%d failed to read response\n",
+                            __FILE__, __LINE__);
+                     return TPM_DRIVER_ERR;

Quoted 8 lines

drivers/tpm/generic_lpc_tpm.c line 408

Quoted 6 lines

drivers/tpm/generic_lpc_tpm.c new file +6 -0

+             }
+
+             if (offset == expected_count)
+                     break;  /* We got all we need */
+
+     } while ((status & has_data) == has_data);

No endless loop please.

I am not sure why you call this endless - it terminates on a few events. The thing is that it is not even known in advance how many cycles the loop will have to run, but it has necessary stop gaps to prevent it from running forever.

Quoted 17 lines

Quoted 17 lines

drivers/tpm/generic_lpc_tpm.c new file +3 -0

+
+     /*
+      * Make sure we indeed read all there was. The TIS_STS_VALID bit

is + * known to be set.

drivers/tpm/generic_lpc_tpm.c new file +12 -0

+      */
+     if (status & TIS_STS_DATA_AVAILABLE) {
+             printf("%s:%d wrong receive status %x\n",
+                    __FILE__, __LINE__, status);
+             return TPM_DRIVER_ERR;
+     }
+
+     /* Tell the TPM that we are done. */
+     tpm_write(TIS_STS_COMMAND_READY, &lpc_tpm_dev->locality
+               [locality].tpm_status);
+     *len = offset;
+     return 0;

Quoted 8 lines

drivers/tpm/generic_lpc_tpm.c line 430

Quoted 6 lines

drivers/tpm/generic_lpc_tpm.c new file +6 -0

+}
+
+int tis_init(void)
+{
+     return tis_probe();
+}

Make tis_probe into tis_init and be done with it ?

ok

Quoted 29 lines

Quoted 26 lines

drivers/tpm/generic_lpc_tpm.c new file +26 -0

+
+int tis_open(void)
+{
+     u8 locality = 0; /* we use locality zero for everything */
+
+     if (tis_close())
+             return TPM_DRIVER_ERR;
+
+     /* now request access to locality */
+     tpm_write(TIS_ACCESS_REQUEST_USE,
+               &lpc_tpm_dev->locality[locality].access);
+
+
+     /* did we get a lock? */
+     if (tis_wait_reg(&lpc_tpm_dev->locality[locality].access,
+                      TIS_ACCESS_ACTIVE_LOCALITY,
+                      TIS_ACCESS_ACTIVE_LOCALITY) == TPM_TIMEOUT_ERR) {
+             printf("%s:%d - failed to lock locality %d\n",
+                    __FILE__, __LINE__, locality);
+             return TPM_DRIVER_ERR;
+     }
+
+     tpm_write(TIS_STS_COMMAND_READY,
+               &lpc_tpm_dev->locality[locality].tpm_status);
+     return 0;
+}

[...]

Quoted 1 line

Cheers

cheers,
/vb

Quoted 11 lines · Vadim Bendebury

Dear Marek Vasut,

thank you for your comments, please see below:

Quoted 6 lines · Marek Vasut

Quoted 4 lines · Vadim Bendebury

TPM (Trusted Platform Module) is an integrated circuit and software platform that provides computer manufacturers with the core components of a subsystem used to assure authenticity, integrity and confidentiality.

Quoted 6 lines

Quoted 4 lines

[...]

Quick points:
* The license

Please suggest the appropriate file header text.

Uh ... you should know the license !!!

Quoted 1 line

Quoted 8 lines

Quoted 6 lines

drivers/tpm/generic_lpc_tpm.c line 59

Quoted 4 lines

drivers/tpm/generic_lpc_tpm.c new file +4 -0

+
+struct lpc_tpm {
+ ? ? struct tpm_locality locality[TPM_TOTAL_LOCALITIES];
+};

Do you need such envelope ?

I think I do - this accurately describes the structure of the chip.

There's just one member ... it's weird?

Quoted 4 lines

Quoted 2 lines

Dot missing at the end.

ok.

Please fix globally.

Quoted 13 lines

Quoted 12 lines

Quoted 10 lines

drivers/tpm/generic_lpc_tpm.c new file +7 -0

+#define TPM_DRIVER_ERR ? ? ? ? ? ? ? (-1)
+
+ /* 1 second is plenty for anything TPM does.*/
+#define MAX_DELAY_US (1000 * 1000)
+
+/* Retrieve burst count value out of the status register contents. */
+#define BURST_COUNT(status) ((u16)(((status) >>

TIS_STS_BURST_COUNT_SHIFT) & \ +
?TIS_STS_BURST_COUNT_MASK))

Do you need the cast ?

Quoted 3 lines

I think it demonstrates the intentional truncation of the value, it gets assigned to u16 values down the road, prevents compiler warnings about assigning incompatible values in some cases.

Make it an inline function then, this will do the typechecking for you.

Quoted 56 lines

Quoted 55 lines

Quoted 31 lines

drivers/tpm/generic_lpc_tpm.c new file +3 -0

+
+/*
+ * Structures defined below allow creating descriptions of TPM

vendor/device + * ID information for run time discovery. The only device the system knows + * about at this time is Infineon slb9635
+ */

drivers/tpm/generic_lpc_tpm.c new file +22 -0

+struct device_name {
+ ? ? u16 dev_id;
+ ? ? const char * const dev_name;
+};
+
+struct vendor_name {
+ ? ? u16 vendor_id;
+ ? ? const char *vendor_name;
+ ? ? const struct device_name *dev_names;
+};
+
+static const struct device_name infineon_devices[] = {
+ ? ? {0xb, "SLB9635 TT 1.2"},
+ ? ? {0}
+};
+
+static const struct vendor_name vendor_names[] = {
+ ? ? {0x15d1, "Infineon", infineon_devices},
+};
+
+/*
+ * Cached vendor/device ID pair to indicate that the device has been

already + * discovered
+ */

drivers/tpm/generic_lpc_tpm.c line 139

Quoted 10 lines

drivers/tpm/generic_lpc_tpm.c new file +10 -0

+static u32 vendor_dev_id;
+
+/* TPM access going through macros to make tracing easier. */
+#define tpm_read(ptr) ({ \
+ ? ? u32 ?__ret; \
+ ? ? __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
+ ? ? ?debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
+ ? ? ? ? ? ?(u32)ptr - (u32)lpc_tpm_dev, __ret); \
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?__ret; })
+

Make this inline function

Review

Quoted 9 lines

(quoted diff) +9 -0

+#define tpm_write(value, ptr) ({ ? ? ? ? ? ? ? ?\
+ ? ? u32 __v = value; ? ? ? ?\
+ ? ? debug(PREFIX "Write reg 0x%x with 0x%x\n", \
+ ? ? ? ? ? ?(u32)ptr - (u32)lpc_tpm_dev, __v); \
+ ? ? if (sizeof(*ptr) == 1) \
+ ? ? ? ? ? ? writeb(__v, ptr); \
+ ? ? else \
+ ? ? ? ? ? ? writel(__v, ptr); })
+

DTTO

Quoted 1 line

Are you sure these will work as inline functions?

Why not ? Also, why do you introduce the __v ?

Quoted 24 lines

Quoted 15 lines

[...]

Quoted 12 lines

(quoted diff) +10 -0

+static u32 tis_senddata(const u8 * const data, u32 len)
+{
+ ? ? u32 offset = 0;
+ ? ? u16 burst = 0;
+ ? ? u32 max_cycles = 0;
+ ? ? u8 locality = 0;
+ ? ? u32 value;
+
+ ? ? value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,
+ ? ? ? ? ? ? ? ? ? ? ? ? ?TIS_STS_COMMAND_READY,

TIS_STS_COMMAND_READY); + ? ? if (value == TPM_TIMEOUT_ERR) {
+ ? ? ? ? ? ? printf("%s:%d - failed to get 'command_ready' status\n",

Quoted 8 lines

Quoted 6 lines

+ ? ? ? ? ? ? ? ? ? ?__FILE__, __LINE__);
+ ? ? ? ? ? ? return TPM_DRIVER_ERR;
+ ? ? }
+ ? ? burst = BURST_COUNT(value);
+
+ ? ? while (1) {

No endless loops please.

Quoted 4 lines

You are the third person looking at this, but the first one uncomfortable with this. Obviously this is not an endless loop, there are three points where the loop terminates, two on timeout errors and one on success.

So there's no case where this can loop endlessly ? fine.

Quoted 100 lines

Quoted 99 lines

Quoted 86 lines

+ ? ? ? ? ? ? unsigned count;
+
+ ? ? ? ? ? ? /* Wait till the device is ready to accept more data. */
+ ? ? ? ? ? ? while (!burst) {
+ ? ? ? ? ? ? ? ? ? ? if (max_cycles++ == MAX_DELAY_US) {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? printf("%s:%d failed to feed %d bytes of %d\n", + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?__FILE__, __LINE__, len - offset, len); + ? ? ? ? ? ? ? ? ? ? ? ? ? ? return TPM_DRIVER_ERR;
+ ? ? ? ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? ? ? ? udelay(1);
+ ? ? ? ? ? ? ? ? ? ? burst = BURST_COUNT(tpm_read(&lpc_tpm_dev->locality +
? ? ? ? ? ? ? ? ? ? ? ?[locality].tpm_status)); + ? ? ? ? ? ? }

(quoted diff) +5 -0

+
+ ? ? ? ? ? ? max_cycles = 0;
+
+ ? ? ? ? ? ? /*
+ ? ? ? ? ? ? ?* Calculate number of bytes the TPM is ready to accept in

one + ? ? ? ? ? ? ?* shot.
+ ? ? ? ? ? ? ?*
+ ? ? ? ? ? ? ?* We want to send the last byte outside of the loop (hence + ? ? ? ? ? ? ?* the -1 below) to make sure that the 'expected' status bit + ? ? ? ? ? ? ?* changes to zero exactly after the last byte is fed into the + ? ? ? ? ? ? ?* FIFO.
+ ? ? ? ? ? ? ?*/
+ ? ? ? ? ? ? count = min(burst, len - offset - 1);
+ ? ? ? ? ? ? while (count--)
+ ? ? ? ? ? ? ? ? ? ? tpm_write(data[offset++],
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? &lpc_tpm_dev->locality[locality].data);

(quoted diff) +6 -0

+
+ ? ? ? ? ? ? value = tis_wait_reg(&lpc_tpm_dev->locality
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?[locality].tpm_status,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?TIS_STS_VALID, TIS_STS_VALID);
+
+ ? ? ? ? ? ? if ((value == TPM_TIMEOUT_ERR) || !(value &

TIS_STS_EXPECT)) { + ? ? ? ? ? ? ? ? ? ? printf("%s:%d TPM command feed overflow\n", + ? ? ? ? ? ? ? ? ? ? ? ? ? ?__FILE__, __LINE__);
+ ? ? ? ? ? ? ? ? ? ? return TPM_DRIVER_ERR;
+ ? ? ? ? ? ? }

(quoted diff) +27 -0

+
+ ? ? ? ? ? ? burst = BURST_COUNT(value);
+ ? ? ? ? ? ? if ((offset == (len - 1)) && burst)
+ ? ? ? ? ? ? ? ? ? ? /*
+ ? ? ? ? ? ? ? ? ? ? ?* We need to be able to send the last byte to the
+ ? ? ? ? ? ? ? ? ? ? ?* device, so burst size must be nonzero before we
+ ? ? ? ? ? ? ? ? ? ? ?* break out.
+ ? ? ? ? ? ? ? ? ? ? ?*/
+ ? ? ? ? ? ? ? ? ? ? break;
+ ? ? }
+
+ ? ? /* Send the last byte. */
+ ? ? tpm_write(data[offset++], &lpc_tpm_dev->locality[locality].data);
+ ? ? /*
+ ? ? ?* Verify that TPM does not expect any more data as part of this
+ ? ? ?* command.
+ ? ? ?*/
+ ? ? value = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,
+ ? ? ? ? ? ? ? ? ? ? ? ? ?TIS_STS_VALID, TIS_STS_VALID);
+ ? ? if ((value == TPM_TIMEOUT_ERR) || (value & TIS_STS_EXPECT)) {
+ ? ? ? ? ? ? printf("%s:%d unexpected TPM status 0x%x\n",
+ ? ? ? ? ? ? ? ? ? ?__FILE__, __LINE__, value);
+ ? ? ? ? ? ? return TPM_DRIVER_ERR;
+ ? ? }
+
+ ? ? /* OK, sitting pretty, let's start the command execution. */
+ ? ? tpm_write(TIS_STS_TPM_GO,

&lpc_tpm_dev->locality[locality].tpm_status); + ? ? return 0;

drivers/tpm/generic_lpc_tpm.c new file +11 -0

+}
+
+/*
+ * tis_readresponse()
+ *
+ * read the TPM device response after a command was issued.
+ *
+ * @buffer - address where to read the response, byte by byte.
+ * @len - pointer to the size of buffer
+ *
+ * On success stores the number of received bytes to len and returns 0.

On + * errors (misformatted TPM data or synchronization problems) returns + * TPM_DRIVER_ERR.
+ */

(quoted diff) +4 -0

+static u32 tis_readresponse(u8 *buffer, u32 *len)
+{
+ ? ? u16 burst_count;
+ ? ? u32 status;

Review

Quoted 10 lines

(quoted diff) +10 -0

+ ? ? u32 offset = 0;
+ ? ? u8 locality = 0;
+ ? ? const u32 has_data = TIS_STS_DATA_AVAILABLE | TIS_STS_VALID;
+ ? ? u32 expected_count = *len;
+ ? ? int max_cycles = 0;
+
+ ? ? /* Wait for the TPM to process the command */
+ ? ? status = tis_wait_reg(&lpc_tpm_dev->locality[locality].tpm_status,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? has_data, has_data);
+ ? ? if (status == TPM_TIMEOUT_ERR) {

Just make it return non-zero for timeout and check for non-zero. And unify the variable names please.

Quoted 1 line

What variable names are you referring to, can you please elaborate.

"value" in the previous function, "status" in this one. Why the inconsistence ?

Quoted 66 lines

Quoted 57 lines

Quoted 57 lines

+ ? ? ? ? ? ? printf("%s:%d failed processing command\n",
+ ? ? ? ? ? ? ? ? ? ?__FILE__, __LINE__);
+ ? ? ? ? ? ? return TPM_DRIVER_ERR;
+ ? ? }

(quoted diff) +17 -0

+
+ ? ? do {
+ ? ? ? ? ? ? while ((burst_count = BURST_COUNT(status)) == 0) {
+ ? ? ? ? ? ? ? ? ? ? if (max_cycles++ == MAX_DELAY_US) {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? printf("%s:%d TPM stuck on read\n",
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?__FILE__, __LINE__);
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? return TPM_DRIVER_ERR;
+ ? ? ? ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? ? ? ? udelay(1);
+ ? ? ? ? ? ? ? ? ? ? status = tpm_read(&lpc_tpm_dev->locality
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? [locality].tpm_status);
+ ? ? ? ? ? ? }
+
+ ? ? ? ? ? ? max_cycles = 0;
+
+ ? ? ? ? ? ? while (burst_count-- && (offset < expected_count)) {
+ ? ? ? ? ? ? ? ? ? ? buffer[offset++] = (u8)

tpm_read(&lpc_tpm_dev->locality +
? ? ? ? ? ? ? ?[locality].data); +
+ ? ? ? ? ? ? ? ? ? ? if (offset == 6) {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? /*
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?* We got the first six bytes of the reply, + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?* let's figure out how many bytes to expect + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?* total - it is stored as a 4 byte number in + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?* network order, starting with offset 2 into + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?* the body of the reply.
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?*/
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? u32 real_length;
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? memcpy(&real_length,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?buffer + 2,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?sizeof(real_length));
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? expected_count = be32_to_cpu(real_length); +
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? if ((expected_count < offset) ||
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? (expected_count > *len)) {
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? printf("%s:%d bad response size %d\n", + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?__FILE__, __LINE__,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?expected_count);
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? return TPM_DRIVER_ERR;
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? ? ? ? ? }
+ ? ? ? ? ? ? }
+
+ ? ? ? ? ? ? /* Wait for the next portion */
+ ? ? ? ? ? ? status = tis_wait_reg(&lpc_tpm_dev->locality
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? [locality].tpm_status,
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? TIS_STS_VALID, TIS_STS_VALID);
+ ? ? ? ? ? ? if (status == TPM_TIMEOUT_ERR) {
+ ? ? ? ? ? ? ? ? ? ? printf("%s:%d failed to read response\n",
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ?__FILE__, __LINE__);
+ ? ? ? ? ? ? ? ? ? ? return TPM_DRIVER_ERR;

Quoted 8 lines

Review

Quoted 6 lines

+ ? ? ? ? ? ? }

(quoted diff) +5 -0

+
+ ? ? ? ? ? ? if (offset == expected_count)
+ ? ? ? ? ? ? ? ? ? ? break; ?/* We got all we need */
+
+ ? ? } while ((status & has_data) == has_data);

No endless loop please.

Quoted 4 lines

I am not sure why you call this endless - it terminates on a few events. The thing is that it is not even known in advance how many cycles the loop will have to run, but it has necessary stop gaps to prevent it from running forever.

See above, is it possible for this to hang ? If not, ok.

Quoted 57 lines

Quoted 17 lines

Quoted 17 lines

+
+ ? ? /*
+ ? ? ?* Make sure we indeed read all there was. The TIS_STS_VALID bit is + ? ? ?* known to be set.
+ ? ? ?*/
+ ? ? if (status & TIS_STS_DATA_AVAILABLE) {
+ ? ? ? ? ? ? printf("%s:%d wrong receive status %x\n",
+ ? ? ? ? ? ? ? ? ? ?__FILE__, __LINE__, status);
+ ? ? ? ? ? ? return TPM_DRIVER_ERR;
+ ? ? }
+
+ ? ? /* Tell the TPM that we are done. */
+ ? ? tpm_write(TIS_STS_COMMAND_READY, &lpc_tpm_dev->locality
+ ? ? ? ? ? ? ? [locality].tpm_status);
+ ? ? *len = offset;
+ ? ? return 0;

Quoted 8 lines

drivers/tpm/generic_lpc_tpm.c line 430

Quoted 6 lines

drivers/tpm/generic_lpc_tpm.c new file +6 -0

+}
+
+int tis_init(void)
+{
+ ? ? return tis_probe();
+}

Make tis_probe into tis_init and be done with it ?

ok

Quoted 29 lines

Quoted 26 lines

(quoted diff) +26 -0

+
+int tis_open(void)
+{
+ ? ? u8 locality = 0; /* we use locality zero for everything */
+
+ ? ? if (tis_close())
+ ? ? ? ? ? ? return TPM_DRIVER_ERR;
+
+ ? ? /* now request access to locality */
+ ? ? tpm_write(TIS_ACCESS_REQUEST_USE,
+ ? ? ? ? ? ? ? &lpc_tpm_dev->locality[locality].access);
+
+
+ ? ? /* did we get a lock? */
+ ? ? if (tis_wait_reg(&lpc_tpm_dev->locality[locality].access,
+ ? ? ? ? ? ? ? ? ? ? ?TIS_ACCESS_ACTIVE_LOCALITY,
+ ? ? ? ? ? ? ? ? ? ? ?TIS_ACCESS_ACTIVE_LOCALITY) == TPM_TIMEOUT_ERR) {
+ ? ? ? ? ? ? printf("%s:%d - failed to lock locality %d\n",
+ ? ? ? ? ? ? ? ? ? ?__FILE__, __LINE__, locality);
+ ? ? ? ? ? ? return TPM_DRIVER_ERR;
+ ? ? }
+
+ ? ? tpm_write(TIS_STS_COMMAND_READY,
+ ? ? ? ? ? ? ? &lpc_tpm_dev->locality[locality].tpm_status);
+ ? ? return 0;
+}

[...]

Quoted 4 lines

Quoted 1 line

Cheers

cheers,
/vb

Cheers

Quoted 6 lines · Marek Vasut

Quoted 5 lines · Vadim Bendebury

Dear Marek Vasut,

thank you for your comments, please see below:

On Sat, Oct 15, 2011 at 11:08 AM, Marek Vasut <[email protected]>

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines · Vadim Bendebury

Quoted 4 lines

TPM (Trusted Platform Module) is an integrated circuit and software platform that provides computer manufacturers with the core components of a subsystem used to assure authenticity, integrity and confidentiality.

[...]

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

Quick points:
* The license

Please suggest the appropriate file header text.

Uh ... you should know the license !!!

removed the BSD part

[..]

Quoted 7 lines

Quoted 7 lines

Quoted 6 lines

drivers/tpm/generic_lpc_tpm.c line 59

Quoted 4 lines

drivers/tpm/generic_lpc_tpm.c new file +4 -0

+
+struct lpc_tpm {
+     struct tpm_locality locality[TPM_TOTAL_LOCALITIES];
+};

Do you need such envelope ?

Quoted 3 lines

Quoted 1 line

I think I do - this accurately describes the structure of the chip.

There's just one member ... it's weird?

I think it is appropriate in this case to encapsulate the entire chip description in a structure. Among other things makes it easier to pass a pointer to the chip description around.

[..]

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Dot missing at the end.

ok.

Please fix globally.

done

Quoted 12 lines

Quoted 12 lines

Quoted 11 lines

Quoted 9 lines

drivers/tpm/generic_lpc_tpm.c new file +6 -0

+#define TPM_DRIVER_ERR               (-1)
+
+ /* 1 second is plenty for anything TPM does.*/
+#define MAX_DELAY_US (1000 * 1000)
+
+/* Retrieve burst count value out of the status register contents.

*/ +#define BURST_COUNT(status) ((u16)(((status) >> TIS_STS_BURST_COUNT_SHIFT) & \ +
TIS_STS_BURST_COUNT_MASK))

Do you need the cast ?

Quoted 5 lines

Quoted 3 lines

I think it demonstrates the intentional truncation of the value, it gets assigned to u16 values down the road, prevents compiler warnings about assigning incompatible values in some cases.

Make it an inline function then, this will do the typechecking for you.

I am not sure what is wrong with a short macro in this case - is this against the coding style?

Quoted 56 lines

Quoted 56 lines

Quoted 55 lines

Quoted 31 lines

drivers/tpm/generic_lpc_tpm.c new file +3 -0

+
+/*
+ * Structures defined below allow creating descriptions of TPM

vendor/device + * ID information for run time discovery. The only device the system knows + * about at this time is Infineon slb9635
+ */

drivers/tpm/generic_lpc_tpm.c new file +22 -0

+struct device_name {
+     u16 dev_id;
+     const char * const dev_name;
+};
+
+struct vendor_name {
+     u16 vendor_id;
+     const char *vendor_name;
+     const struct device_name *dev_names;
+};
+
+static const struct device_name infineon_devices[] = {
+     {0xb, "SLB9635 TT 1.2"},
+     {0}
+};
+
+static const struct vendor_name vendor_names[] = {
+     {0x15d1, "Infineon", infineon_devices},
+};
+
+/*
+ * Cached vendor/device ID pair to indicate that the device has been

already + * discovered
+ */

drivers/tpm/generic_lpc_tpm.c line 139

Quoted 10 lines

drivers/tpm/generic_lpc_tpm.c new file +10 -0

+static u32 vendor_dev_id;
+
+/* TPM access going through macros to make tracing easier. */
+#define tpm_read(ptr) ({ \
+     u32  __ret; \
+     __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
+      debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
+            (u32)ptr - (u32)lpc_tpm_dev, __ret); \
+                                      __ret; })
+

Make this inline function

drivers/tpm/generic_lpc_tpm.c line 148

Quoted 9 lines

drivers/tpm/generic_lpc_tpm.c new file +9 -0

+#define tpm_write(value, ptr) ({                \
+     u32 __v = value;        \
+     debug(PREFIX "Write reg 0x%x with 0x%x\n", \
+            (u32)ptr - (u32)lpc_tpm_dev, __v); \
+     if (sizeof(*ptr) == 1) \
+             writeb(__v, ptr); \
+     else \
+             writel(__v, ptr); })
+

DTTO

Quoted 3 lines

Quoted 1 line

Are you sure these will work as inline functions?

Why not ? Also, why do you introduce the __v ?

macro vs function: need to be able to tell the pointed object size at run time.

__v is needed to avoid side effects when invoking the macro.

Quoted 7 lines · Vadim Bendebury

Quoted 6 lines · Marek Vasut

Quoted 5 lines · Vadim Bendebury

Dear Marek Vasut,

thank you for your comments, please see below:

On Sat, Oct 15, 2011 at 11:08 AM, Marek Vasut <[email protected]>

Quoted 11 lines · wrote:

Quoted 11 lines

Quoted 11 lines

Quoted 10 lines · Vadim Bendebury

Quoted 4 lines

TPM (Trusted Platform Module) is an integrated circuit and software platform that provides computer manufacturers with the core components of a subsystem used to assure authenticity, integrity and confidentiality.

[...]

Quick points:
* The license

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Please suggest the appropriate file header text.

Uh ... you should know the license !!!

removed the BSD part

Are you sure you're not relicensing code you don't own ? I'm just curious, what's the origin of the code ? I'd prefer to avoid legal crap.

Quoted 11 lines

[..]

Quoted 9 lines

Quoted 8 lines

Quoted 6 lines

drivers/tpm/generic_lpc_tpm.c line 59

Quoted 4 lines

drivers/tpm/generic_lpc_tpm.c new file +4 -0

+
+struct lpc_tpm {
+ ? ? struct tpm_locality locality[TPM_TOTAL_LOCALITIES];
+};

Do you need such envelope ?

I think I do - this accurately describes the structure of the chip.

Quoted 5 lines

Quoted 1 line

There's just one member ... it's weird?

I think it is appropriate in this case to encapsulate the entire chip description in a structure. Among other things makes it easier to pass a pointer to the chip description around.

can't you pass the locality array ?

Quoted 27 lines

[..]

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Dot missing at the end.

ok.

Please fix globally.

done

Quoted 16 lines

Quoted 15 lines

Quoted 11 lines

Quoted 9 lines

drivers/tpm/generic_lpc_tpm.c new file +6 -0

+#define TPM_DRIVER_ERR ? ? ? ? ? ? ? (-1)
+
+ /* 1 second is plenty for anything TPM does.*/
+#define MAX_DELAY_US (1000 * 1000)
+
+/* Retrieve burst count value out of the status register contents.

*/ +#define BURST_COUNT(status) ((u16)(((status) >> TIS_STS_BURST_COUNT_SHIFT) & \ + ?TIS_STS_BURST_COUNT_MASK))

Do you need the cast ?

I think it demonstrates the intentional truncation of the value, it gets assigned to u16 values down the road, prevents compiler warnings about assigning incompatible values in some cases.

Quoted 4 lines

Quoted 1 line

Make it an inline function then, this will do the typechecking for you.

I am not sure what is wrong with a short macro in this case - is this against the coding style?

It doesn't do typechecking.

Quoted 56 lines

Quoted 56 lines

Quoted 56 lines

Quoted 55 lines

Quoted 31 lines

drivers/tpm/generic_lpc_tpm.c new file +3 -0

+
+/*
+ * Structures defined below allow creating descriptions of TPM

vendor/device + * ID information for run time discovery. The only device the system knows + * about at this time is Infineon slb9635
+ */

drivers/tpm/generic_lpc_tpm.c new file +22 -0

+struct device_name {
+ ? ? u16 dev_id;
+ ? ? const char * const dev_name;
+};
+
+struct vendor_name {
+ ? ? u16 vendor_id;
+ ? ? const char *vendor_name;
+ ? ? const struct device_name *dev_names;
+};
+
+static const struct device_name infineon_devices[] = {
+ ? ? {0xb, "SLB9635 TT 1.2"},
+ ? ? {0}
+};
+
+static const struct vendor_name vendor_names[] = {
+ ? ? {0x15d1, "Infineon", infineon_devices},
+};
+
+/*
+ * Cached vendor/device ID pair to indicate that the device has been

already + * discovered
+ */

drivers/tpm/generic_lpc_tpm.c line 139

Quoted 10 lines

drivers/tpm/generic_lpc_tpm.c new file +10 -0

+static u32 vendor_dev_id;
+
+/* TPM access going through macros to make tracing easier. */
+#define tpm_read(ptr) ({ \
+ ? ? u32 ?__ret; \
+ ? ? __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
+ ? ? ?debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
+ ? ? ? ? ? ?(u32)ptr - (u32)lpc_tpm_dev, __ret); \
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?__ret; })
+

Make this inline function

Review

Quoted 9 lines

(quoted diff) +9 -0

+#define tpm_write(value, ptr) ({ ? ? ? ? ? ? ? ?\
+ ? ? u32 __v = value; ? ? ? ?\
+ ? ? debug(PREFIX "Write reg 0x%x with 0x%x\n", \
+ ? ? ? ? ? ?(u32)ptr - (u32)lpc_tpm_dev, __v); \
+ ? ? if (sizeof(*ptr) == 1) \
+ ? ? ? ? ? ? writeb(__v, ptr); \
+ ? ? else \
+ ? ? ? ? ? ? writel(__v, ptr); })
+

DTTO

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Are you sure these will work as inline functions?

Why not ? Also, why do you introduce the __v ?

macro vs function: need to be able to tell the pointed object size at run time.

This seems wrong like hell.

Quoted 2 lines

__v is needed to avoid side effects when invoking the macro.

Side effects ? What side effects ?

[...]

Cheers
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Quoted 5 lines

Quoted 3 lines

I am not sure what is wrong with a short macro in this case - is this against the coding style?

It doesn't do typechecking.

but the code around it does, doesn't it?

Sorry, as I said, I am new here: how does this work on this project - does the submitter have to agree to all reviewer's comments? Can I ask

somebody else to confirm that using a macro in this case in inappropriate?

Dear Vadim Bendebury,

Quoted 7 lines · In message <CANy1bu+DZmD_Z=AU8LdTUy7ewHfwMU-8PvH1OA1jba9Tz1xuxA@mail.gmail.com>…

Quoted 5 lines

Quoted 3 lines

I am not sure what is wrong with a short macro in this case - is this against the coding style?

It doesn't do typechecking.

but the code around it does, doesn't it?

I explained this yesterday, too. ?Functions are preferred over macros. In this case ther eis no reason not to use a function.

Quoted 2 lines

Sorry, as I said, I am new here: how does this work on this project - does the submitter have to agree to all reviewer's comments? Can I ask

No, you don't have to agree. ?But we also don't have to accept code that we don't like ;-)

Quoted 2 lines

somebody else to confirm that using a macro in this case in inappropriate?

I already did.

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de The average woman would rather have beauty than brains, ?because ?the average man can see better than he can think.
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

Also, what about situations when one reviewer requests a certain implementation and another one finds it inappropriate?

Can you please also confirm that having a structure with a single element as an array is "weird" and must be changed to passing around a pointer to a single element without the size (or maybe the idea is that the pointer AND the size need to be passed around)?

Are macros acceptable to wrap input output with debug messages, as was suggested earlier on this list, or should I replace each macro with two inline functions?

Dear Vadim Bendebury,

Quoted 3 lines · In message <CANy1buJCjdQnpQ6EHTjJ3jOvGe5fKqDoDpevofhu6_-2Y7L6gg@mail.gmail.com>…

Also, what about situations when one reviewer requests a certain implementation and another one finds it inappropriate?

Here we had several people (Marek and me) asking the same thing. And actually my message was intended to tell you that I agree with Marek.

In case of doubt, someone has to make a final decision.

In this specific case I already decided (and told you) that I want to see a function instead of a macro.

Quoted 4 lines

Can you please also confirm that having a structure with a single element as an array is "weird" and must be changed to passing around a pointer to a single element without the size (or maybe the idea is that the pointer AND the size need to be passed around)?

Yes, I consider this weird, too. ?And you failed to provide a good explanation why you think this would be needed so far.

Quoted 3 lines

Are macros acceptable to wrap input output with debug messages, as was suggested earlier on this list, or should I replace each macro with two inline functions?

Sorry, I don't remember which code you are referring to here.

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de "I'm growing older, but not up." ? ? ? ? ? ? ? ? ? ? ?- Jimmy Buffett

Quoted 3 lines

Yes, I consider this weird, too. And you failed to provide a good explanation why you think this would be needed so far.

My explanation is that it is better readable when the entire information about a chip is contained in one type, as opposed to having the size separate and needed to be carried around (instead of using ARRAY_SIZE() when needed). When the pointer to the chip structure is passed around, the recipient does not have to be aware of a separate definition, all information about the chip is contained in a structure.

Sorry, I am restating the reasons here because I am not sure if you wrote your previous reply before seeing them.

Please let me know if you don't find this explanation convincing, I will change the code as you suggest.

Quoted 4 lines

Quoted 3 lines

Are macros acceptable to wrap input output with debug messages, as was suggested earlier on this list, or should I replace each macro with two inline functions?

Quoted 2 lines

Sorry, I don't remember which code you are referring to here.

I am referring to this message:

http://lists.denx.de/pipermail/u-boot/2011-October/104780.html

Dear Vadim Bendebury,

Quoted 11 lines · In message <CAC3GErH1GpLTEfH3ELi5ebtJPz0HLcJxj3YX3GGUU9HgcHErGA@mail.gmail.com>…

Quoted 3 lines

Yes, I consider this weird, too. ?And you failed to provide a good explanation why you think this would be needed so far.

My explanation is that it is better readable when the entire information about a chip is contained in one type, as opposed to having the size separate and needed to be carried around (instead of using ARRAY_SIZE() when needed). When the pointer to the chip structure is passed around, the recipient does not have to be aware of a separate definition, all information about the chip is contained in a structure.

I read your explanation, but I still fail to uinderstand why we need an additinal wrapper around the internal strcut, which is the only element.

Quoted 2 lines

Sorry, I am restating the reasons here because I am not sure if you wrote your previous reply before seeing them.

I did. That's why I wrote you failed to provide a _good_ explanation.

Quoted 10 lines

Please let me know if you don't find this explanation convincing, I will change the code as you suggest.

Quoted 6 lines

Quoted 3 lines

Are macros acceptable to wrap input output with debug messages, as was suggested earlier on this list, or should I replace each macro with two inline functions?

Sorry, I don't remember which code you are referring to here.

Quoted 3 lines

I am referring to this message:

http://lists.denx.de/pipermail/u-boot/2011-October/104780.html

Yes, I know that. ?But why would one macro turn into two inline functions?

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de Alan Turing thought about criteria to settle the question of ?whether machines ?can think, a question of which we now know that it is about as relevant as the question of whether submarines can swim. ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? -- Edsger Dijkstra
_______________________________________________
U-Boot mailing list
U-Boot at lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot

because this chip is sensitive to the access cycle size (byte versus word).

When using macros one can rely on the passed in pointer type to decide which access to use (byte vs word). If using inline functions, there would be two separate functions required for read (byte and u32) and write (byte and u32).

Dear Vadim Bendebury,

Quoted 3 lines · In message <CANy1buJ0mhpDY948c+PM6u4EsWtoHw3+-u+m_FNiG3BztBinVA@mail.gmail.com>…

because this chip is sensitive to the access cycle size (byte versus word).

Quoted 4 lines

When using macros one can rely on the passed in pointer type to decide which access to use (byte vs word). If using inline functions, there would be two separate functions required for read (byte and u32) and write (byte and u32).

This would then be an improvement in the code, as we then can see what the code is actually doing.

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de What the gods would destroy they first submit to ?an ?IEEE ?standards committee.

Quoted 23 lines · Marek Vasut

Quoted 7 lines · Vadim Bendebury

Quoted 6 lines · Marek Vasut

Quoted 5 lines · Vadim Bendebury

Dear Marek Vasut,

thank you for your comments, please see below:

On Sat, Oct 15, 2011 at 11:08 AM, Marek Vasut <[email protected]>

Quoted 13 lines · wrote:

Quoted 13 lines

Quoted 12 lines

Quoted 10 lines · Vadim Bendebury

Quoted 4 lines

TPM (Trusted Platform Module) is an integrated circuit and software platform that provides computer manufacturers with the core components of a subsystem used to assure authenticity, integrity and confidentiality.

[...]

Quick points:
* The license

Please suggest the appropriate file header text.

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

Uh ... you should know the license !!!

removed the BSD part

Are you sure you're not relicensing code you don't own ? I'm just curious, what's the origin of the code ? I'd prefer to avoid legal crap.

I am sure.

Quoted 14 lines

Quoted 14 lines

[..]

Quoted 10 lines

Quoted 8 lines

Quoted 6 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+
+struct lpc_tpm {
+     struct tpm_locality locality[TPM_TOTAL_LOCALITIES];
+};

Do you need such envelope ?

I think I do - this accurately describes the structure of the chip.

There's just one member ... it's weird?

Quoted 5 lines

Quoted 3 lines

I think it is appropriate in this case to encapsulate the entire chip description in a structure. Among other things makes it easier to pass a pointer to the chip description around.

can't you pass the locality array ?

no, because it would not be clear how big the array is.

Quoted 27 lines

Quoted 27 lines

[..]

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Dot missing at the end.

ok.

Please fix globally.

done

Quoted 16 lines

Quoted 15 lines

Quoted 11 lines

Quoted 9 lines

(quoted diff) +6 -0

+#define TPM_DRIVER_ERR               (-1)
+
+ /* 1 second is plenty for anything TPM does.*/
+#define MAX_DELAY_US (1000 * 1000)
+
+/* Retrieve burst count value out of the status register

contents. */ +#define BURST_COUNT(status) ((u16)(((status) >> TIS_STS_BURST_COUNT_SHIFT) & \ +
TIS_STS_BURST_COUNT_MASK))

Do you need the cast ?

I think it demonstrates the intentional truncation of the value, it gets assigned to u16 values down the road, prevents compiler warnings about assigning incompatible values in some cases.

Quoted 7 lines

Quoted 5 lines

Quoted 2 lines

Make it an inline function then, this will do the typechecking for you.

I am not sure what is wrong with a short macro in this case - is this against the coding style?

It doesn't do typechecking.

but the code around it does, doesn't it?

Sorry, as I said, I am new here: how does this work on this project - does the submitter have to agree to all reviewer's comments? Can I ask somebody else to confirm that using a macro in this case in inappropriate?

Quoted 59 lines

Quoted 59 lines

Quoted 58 lines

Quoted 55 lines

Quoted 43 lines

Quoted 30 lines

(quoted diff) +3 -0

+
+/*
+ * Structures defined below allow creating descriptions of TPM

vendor/device + * ID information for run time discovery. The only device the system knows + * about at this time is Infineon slb9635
+ */

(quoted diff) +22 -0

+struct device_name {
+     u16 dev_id;
+     const char * const dev_name;
+};
+
+struct vendor_name {
+     u16 vendor_id;
+     const char *vendor_name;
+     const struct device_name *dev_names;
+};
+
+static const struct device_name infineon_devices[] = {
+     {0xb, "SLB9635 TT 1.2"},
+     {0}
+};
+
+static const struct vendor_name vendor_names[] = {
+     {0x15d1, "Infineon", infineon_devices},
+};
+
+/*
+ * Cached vendor/device ID pair to indicate that the device has

been already + * discovered
+ */

Review

Quoted 10 lines

(quoted diff) +10 -0

+static u32 vendor_dev_id;
+
+/* TPM access going through macros to make tracing easier. */
+#define tpm_read(ptr) ({ \
+     u32  __ret; \
+     __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
+      debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
+            (u32)ptr - (u32)lpc_tpm_dev, __ret); \
+                                      __ret; })
+

Make this inline function

Quoted 11 lines

Review

Quoted 9 lines

(quoted diff) +9 -0

+#define tpm_write(value, ptr) ({                \
+     u32 __v = value;        \
+     debug(PREFIX "Write reg 0x%x with 0x%x\n", \
+            (u32)ptr - (u32)lpc_tpm_dev, __v); \
+     if (sizeof(*ptr) == 1) \
+             writeb(__v, ptr); \
+     else \
+             writel(__v, ptr); })
+

DTTO

Quoted 1 line

Are you sure these will work as inline functions?

Why not ? Also, why do you introduce the __v ?

Quoted 4 lines

Quoted 2 lines

macro vs function: need to be able to tell the pointed object size at run time.

This seems wrong like hell.

You are entitled to your opinion, but you should not be suggesting to change this code to inline functions, because it would break it.

Quoted 4 lines

Quoted 2 lines

__v is needed to avoid side effects when invoking the macro.

Side effects ? What side effects ?

https://www.securecoding.cert.org/confluence/display/seccode/PRE31-C.+Avoid
+side-effects+in+arguments+to+unsafe+macros

Quoted 26 lines · Vadim Bendebury

Quoted 25 lines · Marek Vasut

Quoted 7 lines · Vadim Bendebury

Quoted 6 lines · Marek Vasut

Quoted 5 lines · Vadim Bendebury

Dear Marek Vasut,

thank you for your comments, please see below:

On Sat, Oct 15, 2011 at 11:08 AM, Marek Vasut <[email protected]>

Quoted 15 lines · wrote:

Quoted 14 lines

Quoted 6 lines

Quoted 6 lines · Vadim Bendebury

Quoted 4 lines

TPM (Trusted Platform Module) is an integrated circuit and software platform that provides computer manufacturers with the core components of a subsystem used to assure authenticity, integrity and confidentiality.

Quoted 6 lines

Quoted 4 lines

[...]

Quick points:
* The license

Please suggest the appropriate file header text.

Uh ... you should know the license !!!

Quoted 6 lines

Quoted 4 lines

Quoted 1 line

removed the BSD part

Are you sure you're not relicensing code you don't own ? I'm just curious, what's the origin of the code ? I'd prefer to avoid legal crap.

I am sure.

Would you mind answering my second question please ?

Quoted 18 lines

Quoted 18 lines

Quoted 17 lines

[..]

Quoted 10 lines

Quoted 8 lines

Quoted 6 lines

Review

Quoted 4 lines

(quoted diff) +4 -0

+
+struct lpc_tpm {
+ ? ? struct tpm_locality locality[TPM_TOTAL_LOCALITIES];
+};

Do you need such envelope ?

I think I do - this accurately describes the structure of the chip.

There's just one member ... it's weird?

I think it is appropriate in this case to encapsulate the entire chip description in a structure. Among other things makes it easier to pass a pointer to the chip description around.

Quoted 3 lines

Quoted 1 line

can't you pass the locality array ?

no, because it would not be clear how big the array is.

TPM_TOTAL_LOCALITIES big ?

Quoted 35 lines

Quoted 34 lines

Quoted 27 lines

[..]

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

Dot missing at the end.

ok.

Please fix globally.

done

Quoted 16 lines

Quoted 15 lines

Quoted 11 lines

Quoted 9 lines

(quoted diff) +6 -0

+#define TPM_DRIVER_ERR ? ? ? ? ? ? ? (-1)
+
+ /* 1 second is plenty for anything TPM does.*/
+#define MAX_DELAY_US (1000 * 1000)
+
+/* Retrieve burst count value out of the status register

contents. */ +#define BURST_COUNT(status) ((u16)(((status) >> TIS_STS_BURST_COUNT_SHIFT) & \ + ?TIS_STS_BURST_COUNT_MASK))

Do you need the cast ?

I think it demonstrates the intentional truncation of the value, it gets assigned to u16 values down the road, prevents compiler warnings about assigning incompatible values in some cases.

Quoted 5 lines

Quoted 2 lines

Make it an inline function then, this will do the typechecking for you.

I am not sure what is wrong with a short macro in this case - is this against the coding style?

It doesn't do typechecking.

Quoted 7 lines

but the code around it does, doesn't it?

Sorry, as I said, I am new here: how does this work on this project - does the submitter have to agree to all reviewer's comments? Can I ask somebody else to confirm that using a macro in this case in inappropriate?

Quoted 62 lines

Quoted 62 lines

Quoted 61 lines

Quoted 55 lines

Quoted 55 lines

Quoted 54 lines

Quoted 30 lines

(quoted diff) +3 -0

+
+/*
+ * Structures defined below allow creating descriptions of TPM

vendor/device + * ID information for run time discovery. The only device the system knows + * about at this time is Infineon slb9635
+ */

(quoted diff) +22 -0

+struct device_name {
+ ? ? u16 dev_id;
+ ? ? const char * const dev_name;
+};
+
+struct vendor_name {
+ ? ? u16 vendor_id;
+ ? ? const char *vendor_name;
+ ? ? const struct device_name *dev_names;
+};
+
+static const struct device_name infineon_devices[] = {
+ ? ? {0xb, "SLB9635 TT 1.2"},
+ ? ? {0}
+};
+
+static const struct vendor_name vendor_names[] = {
+ ? ? {0x15d1, "Infineon", infineon_devices},
+};
+
+/*
+ * Cached vendor/device ID pair to indicate that the device has

been already + * discovered
+ */

Review

Quoted 10 lines

(quoted diff) +10 -0

+static u32 vendor_dev_id;
+
+/* TPM access going through macros to make tracing easier. */
+#define tpm_read(ptr) ({ \
+ ? ? u32 ?__ret; \
+ ? ? __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
+ ? ? ?debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
+ ? ? ? ? ? ?(u32)ptr - (u32)lpc_tpm_dev, __ret); \
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?__ret; })
+

Make this inline function

Review

Quoted 9 lines

(quoted diff) +9 -0

+#define tpm_write(value, ptr) ({ ? ? ? ? ? ? ? ?\
+ ? ? u32 __v = value; ? ? ? ?\
+ ? ? debug(PREFIX "Write reg 0x%x with 0x%x\n", \
+ ? ? ? ? ? ?(u32)ptr - (u32)lpc_tpm_dev, __v); \
+ ? ? if (sizeof(*ptr) == 1) \
+ ? ? ? ? ? ? writeb(__v, ptr); \
+ ? ? else \
+ ? ? ? ? ? ? writel(__v, ptr); })
+

DTTO

Quoted 3 lines

Quoted 1 line

Are you sure these will work as inline functions?

Why not ? Also, why do you introduce the __v ?

macro vs function: need to be able to tell the pointed object size at run time.

Quoted 4 lines

Quoted 1 line

This seems wrong like hell.

You are entitled to your opinion, but you should not be suggesting to change this code to inline functions, because it would break it.

Then write it so it won't break please.

Quoted 7 lines

Quoted 4 lines

Quoted 2 lines

__v is needed to avoid side effects when invoking the macro.

Side effects ? What side effects ?

https://www.securecoding.cert.org/confluence/display/seccode/PRE31-C.+Avoid
+side-effects+in+arguments+to+unsafe+macros

I still don't see it. You use the variable in printf() and writeX(), neither of which change the variable ... so where's the sideeffect ?

Cheers

Quoted 4 lines · Marek Vasut

Quoted 3 lines · Vadim Bendebury

Quoted 2 lines · Marek Vasut

Quoted 1 line · Vadim Bendebury

On Sat, Oct 15, 2011 at 2:09 PM, Marek Vasut <[email protected]>

Quoted 30 lines

Quoted 30 lines

Quoted 29 lines

Quoted 7 lines

Quoted 7 lines · Vadim Bendebury

Quoted 6 lines

Dear Marek Vasut,

thank you for your comments, please see below:

On Sat, Oct 15, 2011 at 11:08 AM, Marek Vasut
<[email protected]>

Quoted 8 lines · wrote:

Quoted 8 lines

Quoted 8 lines

Quoted 8 lines · Vadim Bendebury

Quoted 4 lines

TPM (Trusted Platform Module) is an integrated circuit and software platform that provides computer manufacturers with the core components of a subsystem used to assure authenticity, integrity and confidentiality.

[...]

Quoted 8 lines

Quoted 6 lines

Quoted 4 lines

Quoted 2 lines

Quick points:
* The license

Please suggest the appropriate file header text.

Uh ... you should know the license !!!

removed the BSD part

Are you sure you're not relicensing code you don't own ? I'm just curious, what's the origin of the code ? I'd prefer to avoid legal crap.

Quoted 3 lines

Quoted 1 line

I am sure.

Would you mind answering my second question please ?

I wrote this from scratch.

Quoted 9 lines

Quoted 6 lines

Quoted 3 lines

Quoted 1 line

__v is needed to avoid side effects when invoking the macro.

Side effects ? What side effects ?

https://www.securecoding.cert.org/confluence/display/seccode/PRE31-C.+Av
oid +side-effects+in+arguments+to+unsafe+macros

I still don't see it. You use the variable in printf() and writeX(), neither of which change the variable ... so where's the sideeffect ?

The side effect comes from the calling site.

When data[count++] is used as a macro argument, if there is no intermediate variable defined in the macro declaration, macro expansion inserts data[count++] in the code several times (as many times as the parameter is used in the macro declaration), and in this particular case gets executed twice, resulting in count' advancing by 2 and wrong data' values used.

Quoted 2 lines

Cheers

cheers,
/vb

Quoted 2 lines

Make it an inline function then, this will do the typechecking for you.

I am not sure what is wrong with a short macro in this case - is this against the coding style?

Quoted 14 lines · Vadim Bendebury

Quoted 12 lines · Marek Vasut

Quoted 2 lines · Vadim Bendebury

drivers/tpm/generic_lpc_tpm.c new file +0 -0

[...]

drivers/tpm/generic_lpc_tpm.c line 59

Quoted 4 lines

drivers/tpm/generic_lpc_tpm.c new file +4 -0

+
+struct lpc_tpm {
+     struct tpm_locality locality[TPM_TOTAL_LOCALITIES];
+};

Do you need such envelope ?

Quoted 1 line

I think I do - this accurately describes the structure of the chip.

There's just one member ... it's weird?

Quoted 5 lines

Quoted 3 lines

Quoted 1 line

+/* Error value returned on various TPM driver errors */

Dot missing at the end.

ok.

Please fix globally.

Quoted 23 lines

Quoted 22 lines

drivers/tpm/generic_lpc_tpm.c line 139

Quoted 8 lines

drivers/tpm/generic_lpc_tpm.c new file +8 -0

+/* TPM access going through macros to make tracing easier. */
+#define tpm_read(ptr) ({ \
+     u32  __ret; \
+     __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
+      debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
+            (u32)ptr - (u32)lpc_tpm_dev, __ret); \
+                                      __ret; })
+

Make this inline function

drivers/tpm/generic_lpc_tpm.c line 148

Quoted 9 lines

drivers/tpm/generic_lpc_tpm.c new file +9 -0

+#define tpm_write(value, ptr) ({                \
+     u32 __v = value;        \
+     debug(PREFIX "Write reg 0x%x with 0x%x\n", \
+            (u32)ptr - (u32)lpc_tpm_dev, __v); \
+     if (sizeof(*ptr) == 1) \
+             writeb(__v, ptr); \
+     else \
+             writel(__v, ptr); })
+

DTTO

Quoted 1 line

Are you sure these will work as inline functions?

Why not ? Also, why do you introduce the __v ?

TPM (Trusted Platform Module) is an integrated circuit and software platform that provides computer manufacturers with the core components of a subsystem used to assure authenticity, integrity and confidentiality.

drivers/tpm/Makefile new file +4 -0

+++ b/drivers/tpm/generic_lpc_tpm.c

@@ -0,0 +1,488 @@
+/*
+ * Copyright (c) 2011 The Chromium OS Authors.
+ * Released under the 2-clause BSD license.

+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.

Dear Vadim Bendebury,

Quoted 4 lines · In message <[email protected]> you wrote:

TPM (Trusted Platform Module) is an integrated circuit and software platform that provides computer manufacturers with the core components of a subsystem used to assure authenticity, integrity and confidentiality.

...

drivers/tpm/Makefile line 3

Quoted 6 lines

drivers/tpm/Makefile new file +3 -0

@@ -0,0 +1,27 @@
+# Copyright (c) 2011 The Chromium OS Authors. All rights reserved.
+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.

Please fix this. ? This has been pointed out before. ?Please work a bit more careful. ?Thanks.

...

Quoted 5 lines

+++ b/drivers/tpm/generic_lpc_tpm.c

drivers/tpm/Makefile new file +3 -0

@@ -0,0 +1,488 @@
+/*
+ * Copyright (c) 2011 The Chromium OS Authors.
+ * Released under the 2-clause BSD license.

...

Quoted 4 lines

+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License as
+ * published by the Free Software Foundation; either version 2 of
+ * the License, or (at your option) any later version.

If we can have it under GPLv2+, then we don't need the BSD part. Please drop these lines.

...

drivers/tpm/Makefile

Quoted 7 lines

drivers/tpm/Makefile new file +7 -0

+/* TPM access going through macros to make tracing easier. */
+#define tpm_read(ptr) ({ \
+ ? ? u32 ?__ret; \
+ ? ? __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
+ ? ? ?debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
+ ? ? ? ? ? ?(u32)ptr - (u32)lpc_tpm_dev, __ret); \
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?__ret; })

Please test with something like this instead:

? ? ? ?debug("%sRead reg 0x%x returns 0x%x\n", PREFIX, ...

It might result in smaller code. ?Please verify. ?If so, please fix globally.

...

Quoted 7 lines

+ ? ? vid = didvid & 0xffff;
+ ? ? did = (didvid >> 16) & 0xffff;
+ ? ? for (i = 0; i < ARRAY_SIZE(vendor_names); i++) {
+ ? ? ? ? ? ? int j = 0;
+ ? ? ? ? ? ? u16 known_did;
+ ? ? ? ? ? ? if (vid == vendor_names[i].vendor_id)
+ ? ? ? ? ? ? ? ? ? ? vendor_name = vendor_names[i].vendor_name;

Please separate declarations and code by a blank line.

Quoted 9 lines

+ ? ? ? ? ? ? burst = BURST_COUNT(value);
+ ? ? ? ? ? ? if ((offset == (len - 1)) && burst)
+ ? ? ? ? ? ? ? ? ? ? /*
+ ? ? ? ? ? ? ? ? ? ? ?* We need to be able to send the last byte to the
+ ? ? ? ? ? ? ? ? ? ? ?* device, so burst size must be nonzero before we
+ ? ? ? ? ? ? ? ? ? ? ?* break out.
+ ? ? ? ? ? ? ? ? ? ? ?*/
+ ? ? ? ? ? ? ? ? ? ? break;

We require braces around multiline statements.

Best regards,

Wolfgang Denk

--
DENX Software Engineering GmbH, ? ? MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de "Deliver yesterday, code today, think tomorrow."

drivers/tpm/generic_lpc_tpm.c

Quoted 5 lines · On Friday 14 October 2011 23:38:50 Vadim Bendebury wrote:

drivers/tpm/generic_lpc_tpm.c new file +2 -0

 
+#define TPM_TIMEOUT_ERR ? ? ? ? ? ? ? ? ? ? ?(~0)
+#define TPM_DRIVER_ERR ? ? ? ? ? ? ? (-1)

these are the same thing. ?another reason why you shouldn't mix ~ with normal values. ?use -2 or something.

drivers/tpm/generic_lpc_tpm.c

Quoted 7 lines

drivers/tpm/generic_lpc_tpm.c new file +7 -0

+/* TPM access going through macros to make tracing easier. */
+#define tpm_read(ptr) ({ \
+ ? ? u32 ?__ret; \
+ ? ? __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
+ ? ? ?debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
+ ? ? ? ? ? ?(u32)ptr - (u32)lpc_tpm_dev, __ret); \
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?__ret; })

that last "__ret;" is indented way too far. ?it should be on the same level as "u32 __ret;" and such.

Quoted 8 lines

+#define tpm_write(value, ptr) ({ ? ? ? ? ? ? ? ?\
+ ? ? u32 __v = value; ? ? ? ?\
+ ? ? debug(PREFIX "Write reg 0x%x with 0x%x\n", \
+ ? ? ? ? ? ?(u32)ptr - (u32)lpc_tpm_dev, __v); \
+ ? ? if (sizeof(*ptr) == 1) \
+ ? ? ? ? ? ? writeb(__v, ptr); \
+ ? ? else \
+ ? ? ? ? ? ? writel(__v, ptr); })

({...}) doesn't make sense here. ?this should be a do{...}while(0).

Quoted 2 lines

+ ? ? ? ? ? ? printf("%s:%d - failed to get 'command_ready' status\n",
+ ? ? ? ? ? ? ? ? ? ?__FILE__, __LINE__);

replace __FILE__/__LINE__ with __func__ here and everywhere else in the file -mike

drivers/tpm/generic_lpc_tpm.c

Quoted 5 lines · On Friday 14 October 2011 23:38:50 Vadim Bendebury wrote:

drivers/tpm/generic_lpc_tpm.c new file +2 -0

 
+#define TPM_TIMEOUT_ERR ? ? ? ? ? ? ? ? ? ? ?(~0)
+#define TPM_DRIVER_ERR ? ? ? ? ? ? ? (-1)

these are the same thing. ?another reason why you shouldn't mix ~ with normal values. ?use -2 or something.

drivers/tpm/generic_lpc_tpm.c

Quoted 7 lines

drivers/tpm/generic_lpc_tpm.c new file +7 -0

+/* TPM access going through macros to make tracing easier. */
+#define tpm_read(ptr) ({ \
+ ? ? u32 ?__ret; \
+ ? ? __ret = (sizeof(*ptr) == 1) ? readb(ptr) : readl(ptr); \
+ ? ? ?debug(PREFIX "Read reg 0x%x returns 0x%x\n", \
+ ? ? ? ? ? ?(u32)ptr - (u32)lpc_tpm_dev, __ret); \
+ ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? ?__ret; })

that last "__ret;" is indented way too far. ?it should be on the same level as "u32 __ret;" and such.

Quoted 8 lines

+#define tpm_write(value, ptr) ({ ? ? ? ? ? ? ? ?\
+ ? ? u32 __v = value; ? ? ? ?\
+ ? ? debug(PREFIX "Write reg 0x%x with 0x%x\n", \
+ ? ? ? ? ? ?(u32)ptr - (u32)lpc_tpm_dev, __v); \
+ ? ? if (sizeof(*ptr) == 1) \
+ ? ? ? ? ? ? writeb(__v, ptr); \
+ ? ? else \
+ ? ? ? ? ? ? writel(__v, ptr); })

({...}) doesn't make sense here. ?this should be a do{...}while(0).