Attention is currently required from: Michał Żygowski, Christian Walter, Krystian Hebel.
Hello Michał Żygowski, Krystian Hebel,
I'd like you to do a code review. Please visit
https://review.coreboot.org/c/coreboot/+/68990
to review the following change.
Change subject: drivers/i2c/tpm: splice tpm_vendor_specific struct ......................................................................
drivers/i2c/tpm: splice tpm_vendor_specific struct
Move `locality` field to `struct tpm_inf_dev` and put the rest directly into `tpm_chip`.
Change-Id: Ic3644290963aca9f8dc7cd8ef754352865ef8d2c Ticket: https://ticket.coreboot.org/issues/433 Signed-off-by: Sergii Dmytruk sergii.dmytruk@3mdeb.com --- M src/drivers/i2c/tpm/cr50.c M src/drivers/i2c/tpm/tis.c M src/drivers/i2c/tpm/tpm.c M src/drivers/i2c/tpm/tpm.h 4 files changed, 106 insertions(+), 106 deletions(-)
git pull ssh://review.coreboot.org:29418/coreboot refs/changes/90/68990/1
diff --git a/src/drivers/i2c/tpm/cr50.c b/src/drivers/i2c/tpm/cr50.c index 42eeff4..d908823 100644 --- a/src/drivers/i2c/tpm/cr50.c +++ b/src/drivers/i2c/tpm/cr50.c @@ -39,6 +39,7 @@
struct tpm_inf_dev { int bus; + int locality; unsigned int addr; uint8_t buf[CR50_MAX_BUFSIZE + sizeof(uint8_t)]; }; @@ -224,11 +225,10 @@ }
/* cr50 requires all 4 bytes of status register to be read */ -static uint8_t cr50_i2c_tis_status(struct tpm_chip *chip) +static uint8_t cr50_i2c_tis_status(void) { uint8_t buf[4]; - if (cr50_i2c_read(TPM_STS(chip->vendor.locality), - buf, sizeof(buf)) < 0) { + if (cr50_i2c_read(TPM_STS(tpm_dev.locality), buf, sizeof(buf)) < 0) { printk(BIOS_ERR, "%s: Failed to read status\n", __func__); return 0; } @@ -236,17 +236,16 @@ }
/* cr50 requires all 4 bytes of status register to be written */ -static void cr50_i2c_tis_ready(struct tpm_chip *chip) +static void cr50_i2c_tis_ready(void) { uint8_t buf[4] = { TPM_STS_COMMAND_READY }; - cr50_i2c_write(TPM_STS(chip->vendor.locality), buf, sizeof(buf)); + cr50_i2c_write(TPM_STS(tpm_dev.locality), buf, sizeof(buf)); mdelay(CR50_TIMEOUT_SHORT_MS); }
/* cr50 uses bytes 3:2 of status register for burst count and * all 4 bytes must be read */ -static int cr50_i2c_wait_burststs(struct tpm_chip *chip, uint8_t mask, - size_t *burst, int *status) +static int cr50_i2c_wait_burststs(uint8_t mask, size_t *burst, int *status) { uint8_t buf[4]; struct stopwatch sw; @@ -254,8 +253,7 @@ stopwatch_init_msecs_expire(&sw, CR50_TIMEOUT_LONG_MS);
while (!stopwatch_expired(&sw)) { - if (cr50_i2c_read(TPM_STS(chip->vendor.locality), - buf, sizeof(buf)) != 0) { + if (cr50_i2c_read(TPM_STS(tpm_dev.locality), buf, sizeof(buf)) != 0) { mdelay(CR50_TIMEOUT_SHORT_MS); continue; } @@ -275,18 +273,17 @@ return -1; }
-static int cr50_i2c_tis_recv(struct tpm_chip *chip, uint8_t *buf, - size_t buf_len) +static int cr50_i2c_tis_recv(uint8_t *buf, size_t buf_len) { size_t burstcnt, current, len, expected; - uint8_t addr = TPM_DATA_FIFO(chip->vendor.locality); + uint8_t addr = TPM_DATA_FIFO(tpm_dev.locality); uint8_t mask = TPM_STS_VALID | TPM_STS_DATA_AVAIL; int status;
if (buf_len < TPM_HEADER_SIZE) goto out_err;
- if (cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status) < 0) { + if (cr50_i2c_wait_burststs(mask, &burstcnt, &status) < 0) { printk(BIOS_ERR, "%s: First chunk not available\n", __func__); goto out_err; } @@ -309,7 +306,7 @@ current = burstcnt; while (current < expected) { /* Read updated burst count and check status */ - if (cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status) < 0) + if (cr50_i2c_wait_burststs(mask, &burstcnt, &status) < 0) goto out_err;
len = MIN(burstcnt, expected - current); @@ -322,7 +319,7 @@ }
/* Ensure TPM is done reading data */ - if (cr50_i2c_wait_burststs(chip, TPM_STS_VALID, &burstcnt, &status) < 0) + if (cr50_i2c_wait_burststs(TPM_STS_VALID, &burstcnt, &status) < 0) goto out_err; if (status & TPM_STS_DATA_AVAIL) { printk(BIOS_ERR, "%s: Data still available\n", __func__); @@ -333,12 +330,12 @@
out_err: /* Abort current transaction if still pending */ - if (cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY) - cr50_i2c_tis_ready(chip); + if (cr50_i2c_tis_status() & TPM_STS_COMMAND_READY) + cr50_i2c_tis_ready(); return -1; }
-static int cr50_i2c_tis_send(struct tpm_chip *chip, uint8_t *buf, size_t len) +static int cr50_i2c_tis_send(uint8_t *buf, size_t len) { int status; size_t burstcnt, limit, sent = 0; @@ -348,14 +345,14 @@ stopwatch_init_msecs_expire(&sw, CR50_TIMEOUT_LONG_MS);
/* Wait until TPM is ready for a command */ - while (!(cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY)) { + while (!(cr50_i2c_tis_status() & TPM_STS_COMMAND_READY)) { if (stopwatch_expired(&sw)) { printk(BIOS_ERR, "%s: Command ready timeout\n", __func__); return -1; }
- cr50_i2c_tis_ready(chip); + cr50_i2c_tis_ready(); }
while (len > 0) { @@ -366,14 +363,13 @@ mask |= TPM_STS_DATA_EXPECT;
/* Read burst count and check status */ - if (cr50_i2c_wait_burststs(chip, mask, &burstcnt, &status) < 0) + if (cr50_i2c_wait_burststs(mask, &burstcnt, &status) < 0) goto out_err;
/* Use burstcnt - 1 to account for the address byte * that is inserted by cr50_i2c_write() */ limit = MIN(burstcnt - 1, len); - if (cr50_i2c_write(TPM_DATA_FIFO(chip->vendor.locality), - &buf[sent], limit) != 0) { + if (cr50_i2c_write(TPM_DATA_FIFO(tpm_dev.locality), &buf[sent], limit) != 0) { printk(BIOS_ERR, "%s: Write failed\n", __func__); goto out_err; } @@ -383,7 +379,7 @@ }
/* Ensure TPM is not expecting more data */ - if (cr50_i2c_wait_burststs(chip, TPM_STS_VALID, &burstcnt, &status) < 0) + if (cr50_i2c_wait_burststs(TPM_STS_VALID, &burstcnt, &status) < 0) goto out_err; if (status & TPM_STS_DATA_EXPECT) { printk(BIOS_ERR, "%s: Data still expected\n", __func__); @@ -391,8 +387,7 @@ }
/* Start the TPM command */ - if (cr50_i2c_write(TPM_STS(chip->vendor.locality), tpm_go, - sizeof(tpm_go)) < 0) { + if (cr50_i2c_write(TPM_STS(tpm_dev.locality), tpm_go, sizeof(tpm_go)) < 0) { printk(BIOS_ERR, "%s: Start command failed\n", __func__); goto out_err; } @@ -400,21 +395,20 @@
out_err: /* Abort current transaction if still pending */ - if (cr50_i2c_tis_status(chip) & TPM_STS_COMMAND_READY) - cr50_i2c_tis_ready(chip); + if (cr50_i2c_tis_status() & TPM_STS_COMMAND_READY) + cr50_i2c_tis_ready(); return -1; }
static void cr50_vendor_init(struct tpm_chip *chip) { - memset(&chip->vendor, 0, sizeof(struct tpm_vendor_specific)); - chip->vendor.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID; - chip->vendor.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID; - chip->vendor.req_canceled = TPM_STS_COMMAND_READY; - chip->vendor.status = &cr50_i2c_tis_status; - chip->vendor.recv = &cr50_i2c_tis_recv; - chip->vendor.send = &cr50_i2c_tis_send; - chip->vendor.cancel = &cr50_i2c_tis_ready; + chip->req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID; + chip->req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID; + chip->req_canceled = TPM_STS_COMMAND_READY; + chip->status = &cr50_i2c_tis_status; + chip->recv = &cr50_i2c_tis_recv; + chip->send = &cr50_i2c_tis_send; + chip->cancel = &cr50_i2c_tis_ready; }
int tpm_vendor_probe(unsigned int bus, uint32_t addr) @@ -422,7 +416,7 @@ return 0; }
-static int cr50_i2c_probe(struct tpm_chip *chip, uint32_t *did_vid) +static int cr50_i2c_probe(uint32_t *did_vid) { int retries;
@@ -471,7 +465,7 @@
cr50_vendor_init(chip);
- if (cr50_i2c_probe(chip, &did_vid)) + if (cr50_i2c_probe(&did_vid)) return -1;
if (ENV_SEPARATE_VERSTAGE || ENV_BOOTBLOCK) diff --git a/src/drivers/i2c/tpm/tis.c b/src/drivers/i2c/tpm/tis.c index 5b199be..f010bf3 100644 --- a/src/drivers/i2c/tpm/tis.c +++ b/src/drivers/i2c/tpm/tis.c @@ -54,7 +54,7 @@ memcpy(&count, sbuf + TPM_CMD_COUNT_BYTE, sizeof(count)); count = be32_to_cpu(count);
- if (!chip.vendor.send || !chip.vendor.status || !chip.vendor.cancel) + if (!chip.send || !chip.status || !chip.cancel) return -1;
if (count == 0) { @@ -67,8 +67,8 @@ return -1; }
- ASSERT(chip.vendor.send); - rc = chip.vendor.send(&chip, (uint8_t *) sbuf, count); + ASSERT(chip.send); + rc = chip.send((uint8_t *) sbuf, count); if (rc < 0) { printk(BIOS_DEBUG, "%s: tpm_send error\n", __func__); goto out; @@ -76,14 +76,13 @@
int timeout = 2 * 60 * 1000; /* two minutes timeout */ while (timeout) { - ASSERT(chip.vendor.status); - uint8_t status = chip.vendor.status(&chip); - if ((status & chip.vendor.req_complete_mask) == - chip.vendor.req_complete_val) { + ASSERT(chip.status); + uint8_t status = chip.status(); + if ((status & chip.req_complete_mask) == chip.req_complete_val) { goto out_recv; }
- if (status == chip.vendor.req_canceled) { + if (status == chip.req_canceled) { printk(BIOS_DEBUG, "%s: Operation Canceled\n", __func__); rc = -1; @@ -93,15 +92,15 @@ timeout--; }
- ASSERT(chip.vendor.cancel); - chip.vendor.cancel(&chip); + ASSERT(chip.cancel); + chip.cancel(); printk(BIOS_DEBUG, "%s: Operation Timed out\n", __func__); rc = -1; //ETIME; goto out;
out_recv:
- rc = chip.vendor.recv(&chip, (uint8_t *) rbuf, rbufsiz); + rc = chip.recv((uint8_t *) rbuf, rbufsiz); if (rc < 0) printk(BIOS_DEBUG, "%s: tpm_recv: error %d\n", __func__, rc); out: diff --git a/src/drivers/i2c/tpm/tpm.c b/src/drivers/i2c/tpm/tpm.c index 606f14d..68b7042 100644 --- a/src/drivers/i2c/tpm/tpm.c +++ b/src/drivers/i2c/tpm/tpm.c @@ -59,6 +59,7 @@ /* Structure to store I2C TPM specific stuff */ struct tpm_inf_dev { int bus; + int locality; unsigned int addr; unsigned int sleep_short; /* Short sleep duration in usec */ unsigned int sleep_long; /* Long sleep duration in usec */ @@ -219,7 +220,7 @@ MAX_COUNT_LONG); }
-static int check_locality(struct tpm_chip *chip, int loc) +static int check_locality(int loc) { uint8_t buf;
@@ -228,14 +229,14 @@
if ((buf & (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID)) == (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID)) { - chip->vendor.locality = loc; + tpm_dev.locality = loc; return loc; }
return -1; }
-static void release_locality(struct tpm_chip *chip, int loc, int force) +static void release_locality(int loc, int force) { uint8_t buf; if (iic_tpm_read(TPM_ACCESS(loc), &buf, 1) < 0) @@ -248,11 +249,11 @@ } }
-static int request_locality(struct tpm_chip *chip, int loc) +static int request_locality(int loc) { uint8_t buf = TPM_ACCESS_REQUEST_USE;
- if (check_locality(chip, loc) >= 0) + if (check_locality(loc) >= 0) return loc; /* we already have the locality */
iic_tpm_write(TPM_ACCESS(loc), &buf, 1); @@ -260,7 +261,7 @@ /* wait for burstcount */ int timeout = 2 * 1000; /* 2s timeout */ while (timeout) { - if (check_locality(chip, loc) >= 0) + if (check_locality(loc) >= 0) return loc; mdelay(TPM_TIMEOUT); timeout--; @@ -269,11 +270,11 @@ return -1; }
-static uint8_t tpm_tis_i2c_status(struct tpm_chip *chip) +static uint8_t tpm_tis_i2c_status(void) { /* NOTE: Since I2C read may fail, return 0 in this case --> time-out */ uint8_t buf; - if (iic_tpm_read(TPM_STS(chip->vendor.locality), &buf, 1) < 0) + if (iic_tpm_read(TPM_STS(tpm_dev.locality), &buf, 1) < 0) return 0; else if (buf == 0xff) /* Some TPMs sometimes randomly return 0xff. */ return 0; @@ -281,14 +282,14 @@ return buf; }
-static void tpm_tis_i2c_ready(struct tpm_chip *chip) +static void tpm_tis_i2c_ready(void) { /* this causes the current command to be aborted */ uint8_t buf = TPM_STS_COMMAND_READY; - iic_tpm_write_long(TPM_STS(chip->vendor.locality), &buf, 1); + iic_tpm_write_long(TPM_STS(tpm_dev.locality), &buf, 1); }
-static ssize_t get_burstcount(struct tpm_chip *chip) +static ssize_t get_burstcount(void) { ssize_t burstcnt; uint8_t buf[3]; @@ -297,7 +298,7 @@ int timeout = 2 * 1000; /* 2s timeout */ while (timeout) { /* Note: STS is little endian */ - if (iic_tpm_read(TPM_STS(chip->vendor.locality) + 1, buf, 3) + if (iic_tpm_read(TPM_STS(tpm_dev.locality) + 1, buf, 3) < 0) burstcnt = 0; else @@ -311,11 +312,11 @@ return -1; }
-static int wait_for_stat(struct tpm_chip *chip, uint8_t mask, int *status) +static int wait_for_stat(uint8_t mask, int *status) { unsigned long timeout = 2 * 1024; while (timeout) { - *status = tpm_tis_i2c_status(chip); + *status = tpm_tis_i2c_status(); if ((*status & mask) == mask) return 0; mdelay(TPM_TIMEOUT); @@ -325,12 +326,12 @@ return -1; }
-static int recv_data(struct tpm_chip *chip, uint8_t *buf, size_t count) +static int recv_data(uint8_t *buf, size_t count) { size_t size = 0;
while (size < count) { - ssize_t burstcnt = get_burstcount(chip); + ssize_t burstcnt = get_burstcount(); int rc;
/* burstcount < 0 = TPM is busy */ @@ -341,7 +342,7 @@ if (burstcnt > (count - size)) burstcnt = count - size;
- rc = iic_tpm_read(TPM_DATA_FIFO(chip->vendor.locality), + rc = iic_tpm_read(TPM_DATA_FIFO(tpm_dev.locality), &(buf[size]), burstcnt); if (rc == 0) @@ -350,7 +351,7 @@ return size; }
-static int tpm_tis_i2c_recv(struct tpm_chip *chip, uint8_t *buf, size_t count) +static int tpm_tis_i2c_recv(uint8_t *buf, size_t count) { int size = 0; uint32_t expected; @@ -362,7 +363,7 @@ }
/* read first 10 bytes, including tag, paramsize, and result */ - size = recv_data(chip, buf, TPM_HEADER_SIZE); + size = recv_data(buf, TPM_HEADER_SIZE); if (size < TPM_HEADER_SIZE) { printk(BIOS_DEBUG, "%s: Unable to read header\n", __func__); goto out; @@ -375,15 +376,14 @@ goto out; }
- size += recv_data(chip, &buf[TPM_HEADER_SIZE], - expected - TPM_HEADER_SIZE); + size += recv_data(&buf[TPM_HEADER_SIZE], expected - TPM_HEADER_SIZE); if (size < expected) { printk(BIOS_DEBUG, "%s: Unable to read remainder of result\n", __func__); size = -1; goto out; }
- wait_for_stat(chip, TPM_STS_VALID, &status); + wait_for_stat(TPM_STS_VALID, &status); if (status & TPM_STS_DATA_AVAIL) { /* retry? */ printk(BIOS_DEBUG, "%s: Error left over data\n", __func__); size = -1; @@ -391,12 +391,12 @@ }
out: - tpm_tis_i2c_ready(chip); + tpm_tis_i2c_ready();
return size; }
-static int tpm_tis_i2c_send(struct tpm_chip *chip, uint8_t *buf, size_t len) +static int tpm_tis_i2c_send(uint8_t *buf, size_t len) { int status; size_t count = 0; @@ -405,15 +405,15 @@ if (len > TPM_BUFSIZE) return -1; /* command is too long for our TPM, sorry */
- status = tpm_tis_i2c_status(chip); + status = tpm_tis_i2c_status(); if ((status & TPM_STS_COMMAND_READY) == 0) { - tpm_tis_i2c_ready(chip); - if (wait_for_stat(chip, TPM_STS_COMMAND_READY, &status) < 0) + tpm_tis_i2c_ready(); + if (wait_for_stat(TPM_STS_COMMAND_READY, &status) < 0) goto out_err; }
while (count < len - 1) { - ssize_t burstcnt = get_burstcount(chip); + ssize_t burstcnt = get_burstcount();
/* burstcount < 0 = TPM is busy */ if (burstcnt < 0) @@ -422,29 +422,29 @@ if (burstcnt > (len-1-count)) burstcnt = len-1-count;
- if (iic_tpm_write(TPM_DATA_FIFO(chip->vendor.locality), - &(buf[count]), burstcnt) == 0) + if (iic_tpm_write(TPM_DATA_FIFO(tpm_dev.locality), + &(buf[count]), burstcnt) == 0) count += burstcnt;
- wait_for_stat(chip, TPM_STS_VALID, &status); + wait_for_stat(TPM_STS_VALID, &status); if ((status & TPM_STS_DATA_EXPECT) == 0) goto out_err; }
/* write last byte */ - iic_tpm_write(TPM_DATA_FIFO(chip->vendor.locality), &(buf[count]), 1); + iic_tpm_write(TPM_DATA_FIFO(tpm_dev.locality), &(buf[count]), 1);
- wait_for_stat(chip, TPM_STS_VALID, &status); + wait_for_stat(TPM_STS_VALID, &status); if ((status & TPM_STS_DATA_EXPECT) != 0) goto out_err;
/* go and do it */ - iic_tpm_write(TPM_STS(chip->vendor.locality), &sts, 1); + iic_tpm_write(TPM_STS(tpm_dev.locality), &sts, 1);
return len;
out_err: - tpm_tis_i2c_ready(chip); + tpm_tis_i2c_ready();
return -1; } @@ -507,18 +507,17 @@ tpm_dev.sleep_short = SLEEP_DURATION; tpm_dev.sleep_long = SLEEP_DURATION_LONG;
- memset(&chip->vendor, 0, sizeof(struct tpm_vendor_specific)); chip->is_open = 1;
- chip->vendor.req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID; - chip->vendor.req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID; - chip->vendor.req_canceled = TPM_STS_COMMAND_READY; - chip->vendor.status = &tpm_tis_i2c_status; - chip->vendor.recv = &tpm_tis_i2c_recv; - chip->vendor.send = &tpm_tis_i2c_send; - chip->vendor.cancel = &tpm_tis_i2c_ready; + chip->req_complete_mask = TPM_STS_DATA_AVAIL | TPM_STS_VALID; + chip->req_complete_val = TPM_STS_DATA_AVAIL | TPM_STS_VALID; + chip->req_canceled = TPM_STS_COMMAND_READY; + chip->status = &tpm_tis_i2c_status; + chip->recv = &tpm_tis_i2c_recv; + chip->send = &tpm_tis_i2c_send; + chip->cancel = &tpm_tis_i2c_ready;
- if (request_locality(chip, 0) != 0) + if (request_locality(0) != 0) return -1;
/* Read four bytes from DID_VID register */ @@ -547,6 +546,6 @@ return 0;
out_err: - release_locality(chip, 0, 1); + release_locality(0, 1); return -1; } diff --git a/src/drivers/i2c/tpm/tpm.h b/src/drivers/i2c/tpm/tpm.h index 868b306..d4176cc 100644 --- a/src/drivers/i2c/tpm/tpm.h +++ b/src/drivers/i2c/tpm/tpm.h @@ -37,22 +37,16 @@ #define TPM_DATA_FIFO(l) (0x0005 | ((l) << 4)) #define TPM_DID_VID(l) (0x0006 | ((l) << 4))
-struct tpm_chip; - -struct tpm_vendor_specific { +struct tpm_chip { + int is_open; uint8_t req_complete_mask; uint8_t req_complete_val; uint8_t req_canceled; - int (*recv)(struct tpm_chip *, uint8_t *, size_t); - int (*send)(struct tpm_chip *, uint8_t *, size_t); - void (*cancel)(struct tpm_chip *); - uint8_t (*status)(struct tpm_chip *); - int locality; -};
-struct tpm_chip { - int is_open; - struct tpm_vendor_specific vendor; + int (*recv)(uint8_t *buf, size_t len); + int (*send)(uint8_t *buf, size_t len); + void (*cancel)(void); + uint8_t (*status)(void); };
/* ---------- Interface for TPM vendor ------------ */