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

[Andy Shevchenko]

On Sat, Nov 2, 2019 at 4:01 PM Simon Glass <sjg at 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).


-- 
With Best Regards,
Andy Shevchenko