Re: [U-Boot] [PATCH 3/7] tpm: Add a driver for H1/Cr50

On Sat, Nov 2, 2019 at 4:01 PM Simon Glass sjg@chromium.org wrote:

H1 is a Google security chip present in recent Chromebooks, Pixel phones and other devices. Cr50 is the name of the software that runs on H1 in Chromebooks.

This chip is used to handle TPM-like functionality and also has quite a few additional features.

Add a driver for this.

+/* Wait for interrupt to indicate TPM is ready */ +static int cr50_i2c_wait_tpm_ready(struct udevice *dev) +{

•   struct cr50_priv *priv = dev_get_priv(dev);
  

•   ulong timeout;
  

•   int i;
  

•   if (!dm_gpio_is_valid(&priv->ready_gpio)) {
  

•           /* Fixed delay if interrupt not supported */
  

•           udelay(TIMEOUT_NO_IRQ_US);
  

•           return 0;
  

•   }
  

•   timeout = timer_get_us() + TIMEOUT_IRQ_US;
  

•   i = 0;
  

•   while (!dm_gpio_get_value(&priv->ready_gpio)) {
  

•           i++;
  

•           if (timer_get_us() > timeout) {
  

•                   printf("Timeout\n");
  

•                   /*
  

•                    * Use this instead of -ETIMEDOUT which is used by i2c
  

•                    */
  

•                   return -ETIME;
  

•           }
  

•   }
  

•   return 0;
  

+}

Timeout loops look more naturally if they are done as do {} while. See:

i = 0; do { if (dm_gpio_get_value(&priv->ready_gpio)) return 0;

i++; /* What is this for? */ } while (timeout >= timer_get_us());

printf("Timeout\n"); /* * Use this instead of -ETIMEDOUT which is used by i2c */ return -ETIME; }

+static int cr50_i2c_write(struct udevice *dev, u8 addr, const u8 *buffer,

•                     size_t len)
  

+{

•   u8 buf[len + 1];
  

VLA?!

•   int ret;
  

•   if (len > CR50_MAX_BUF_SIZE) {
  

•           log_err("Length %zd is too large\n", len);
  

•           return -E2BIG;
  

•   }
  

+}

+static int request_locality(struct udevice *dev, int loc) +{

•   struct cr50_priv *priv = dev_get_priv(dev);
  

•   u8 buf = TPM_ACCESS_REQUEST_USE;
  

•   ulong timeout;
  

•   int ret;
  

•   ret = check_locality(dev, loc);
  

•   if (ret < 0)
  

•           return ret;
  

•   else if (ret == loc)
  

Here 'else' is redundant.

•           return loc;
  

•   timeout = timer_get_us() + TIMEOUT_LONG_US;
  

•   while (timer_get_us() < timeout) {
  

Same comment for timeout loops (btw, there is a chance in a while {} loop that it won't do even first iteretation).

•           ret = check_locality(dev, loc);
  

•           if (ret < 0)
  

•                   return ret;
  

•           if (ret == loc) {
  

•                   priv->locality = loc;
  

•                   log_debug("Set locality to %x\n", loc);
  

•                   return loc;
  

•           }
  

•           udelay(TIMEOUT_SHORT_US);
  

•   }
  

•   printf("Request locality failed\n");
  

•   return -ETIMEDOUT;
  

+}

•   timeout = timer_get_us() + TIMEOUT_LONG_US;
  

•   while (timer_get_us() < timeout) {
  

Ditto for all timeout loop related comments.

•           if (cr50_i2c_read(dev, tpm_sts(priv->locality),
  

•                             (u8 *)&buf, sizeof(buf)) < 0) {
  

•                   udelay(TIMEOUT_SHORT_US);
  

•                   continue;
  

•           }
  

•   timeout = timer_get_us() + TIMEOUT_LONG_US;
  

•   do {
  

•           ret = cr50_i2c_status(dev);
  

•           if (ret)
  

•                   goto out_err;
  

•           if (!(ret & TPM_STS_COMMAND_READY))
  

•                   break;
  

•           if (timer_get_us() > timeout)
  

•                   goto out_err;
  

•           ret = cr50_i2c_ready(dev);
  

•           if (ret)
  

•                   goto out_err;
  

•   } while (1);
  

Better to have non-infinite loops (i.o.w. loops with understandable exit conditional).