Re: [SeaBIOS] [PATCH v2 2/5] tpm: Set timeouts and durations to microsecond values

[Kevin O'Connor]

On Sat, Nov 21, 2015 at 08:32:37AM -0500, Stefan Berger wrote:
> From: Stefan Berger
>
> Fix the timeouts and durations -- they are provided in microseconds.
> Adapt the TPM driver for it.
>
> Get TPM specific timeout and duration values earlier from the device.
> Signed-off-by: Stefan Berger
>
> src/hw/tpm_drivers.c | 11 ++++++-----
>  src/hw/tpm_drivers.h | 23 ++++++++++++++---------
>  src/tcgbios.c        |  8 ++++----
>  3 files changed, 24 insertions(+), 18 deletions(-)
>
> diff --git a/src/hw/tpm_drivers.c b/src/hw/tpm_drivers.c
> index 0bf5997..fcf176e 100644
> --- a/src/hw/tpm_drivers.c
> +++ b/src/hw/tpm_drivers.c
> @@ -93,15 +93,16 @@ static u32 tis_wait_sts(u8 locty, u32 time, u8 mask, u8 expect)
>          return 0;
>  u32 rc = 1;
> u32 ctr = 0;
> while (time > 0) {
> while (time > ctr) {
>     u8 sts = readb(TIS_REG(locty, TIS_REG_STS));
>     if ((sts & mask) == expect) {
>         rc = 0;
>         break;
>     }
>    msleep(1);
>    time--;
>    usleep(1000);
>    ctr += 1000;
> }

The preferred way to do this is:

u32 end = timer_calc_usec(time);
    for (;;) {
        ...
        if (timer_check(end)) {
            warn_timeout();
            return -1;
        }
        yield();
    }

[...]
> --- a/src/hw/tpm_drivers.h
> +++ b/src/hw/tpm_drivers.h
> @@ -66,12 +66,13 @@ extern struct tpm_driver tpm_drivers[];
>  #define TIS_ACCESS_REQUEST_USE         (1 << 1) /* 0x02 */
>  #define TIS_ACCESS_TPM_ESTABLISHMENT   (1 << 0) /* 0x01 */
>
> -#define SCALER 10
>
> -#define TIS_DEFAULT_TIMEOUT_A          (750  * SCALER)
> -#define TIS_DEFAULT_TIMEOUT_B          (2000 * SCALER)
> -#define TIS_DEFAULT_TIMEOUT_C          (750  * SCALER)
> -#define TIS_DEFAULT_TIMEOUT_D          (750  * SCALER)
> +/*
> Default TIS timeouts used before getting them from the TPM itself
> */
>
> +#define TIS_DEFAULT_TIMEOUT_A           750 * 1000 /* ms */
> +#define TIS_DEFAULT_TIMEOUT_B          2000 * 1000 /* us */
> +#define TIS_DEFAULT_TIMEOUT_C           750 * 1000 /* us */
> +#define TIS_DEFAULT_TIMEOUT_D           750 * 1000 /* us */

Parenthesis need to be around any math in macros (eg, (750*1000) ).

Otherwise the series looks good to me.  Thanks!

-Kevin