[U-Boot] [PATCH 15/25] dm: tpm: Add a uclass for Trusted Platform Modules

[christophe.ricard <christophe.ricard@gmail.com>]

Hi Simon,

I think we are pretty inline for the uclass.
Please find below some few remarks.

On 11/08/2015 16:48, Simon Glass wrote:
> Add a new uclass for TPMs which uses almost the same TIS (TPM Interface
> Specification) as is currently implemented. Since init() is handled by the
> normal driver model probe() method, we don't need to implement that. Also
> rename the transfer method to xfer() which is a less clumbsy name.
>
> Once all drivers and users are converted to driver model we can remove the
> old code.
>
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
>
>   drivers/tpm/Kconfig      |  9 +++++
>   drivers/tpm/Makefile     |  2 +
>   drivers/tpm/tpm-uclass.c | 57 ++++++++++++++++++++++++++++
>   include/dm/uclass-id.h   |  1 +
>   include/tis.h            | 97 ++++++++++++++++++++++++++++++++++++++++++++++++
>   5 files changed, 166 insertions(+)
>   create mode 100644 drivers/tpm/tpm-uclass.c
>
> diff --git a/drivers/tpm/Kconfig b/drivers/tpm/Kconfig
> index 993d2d7..800239e 100644
> --- a/drivers/tpm/Kconfig
> +++ b/drivers/tpm/Kconfig
> @@ -1,3 +1,12 @@
> +config DM_TPM
> +	bool "Enable driver model for Trusted Platform Module drivers"
> +	depends on DM && TPM
> +	help
> +          Enable driver model for TPMs. The TIS interface (tis_open(),
> +	  tis_sendrecv(), etc.) is then implemented by the TPM uclass. Note
> +	  that even with driver model only a single TPM is currently
> +	  supported, since the tpm library assumes this.
> +
>   config TPM_TIS_SANDBOX
>   	bool "Enable sandbox TPM driver"
>   	depends on SANDBOX
> diff --git a/drivers/tpm/Makefile b/drivers/tpm/Makefile
> index 597966c..0d328f8 100644
> --- a/drivers/tpm/Makefile
> +++ b/drivers/tpm/Makefile
> @@ -3,6 +3,8 @@
>   # SPDX-License-Identifier:	GPL-2.0+
>   #
>   
> +obj-$(CONFIG_DM_TPM) += tpm-uclass.o
> +
>   obj-$(CONFIG_TPM_ATMEL_TWI) += tpm_atmel_twi.o
>   obj-$(CONFIG_TPM_TIS_I2C) += tpm_tis_i2c.o
>   obj-$(CONFIG_TPM_TIS_LPC) += tpm_tis_lpc.o
> diff --git a/drivers/tpm/tpm-uclass.c b/drivers/tpm/tpm-uclass.c
> new file mode 100644
> index 0000000..ccade5b
> --- /dev/null
> +++ b/drivers/tpm/tpm-uclass.c
> @@ -0,0 +1,57 @@
> +/*
> + * Copyright (c) 2015 Google, Inc
> + * Written by Simon Glass <sjg@chromium.org>
> + *
> + * SPDX-License-Identifier:	GPL-2.0+
> + */
> +
> +#include <common.h>
> +#include <dm.h>
> +#include <tis.h>
> +
> +int tis_open(struct udevice *dev)
> +{
> +	struct tpm_ops *ops = tpm_get_ops(dev);
> +
> +	if (!ops->open)
> +		return -ENOSYS;
> +
> +	return ops->open(dev);
> +}
> +
> +int tis_close(struct udevice *dev)
> +{
> +	struct tpm_ops *ops = tpm_get_ops(dev);
> +
> +	if (!ops->close)
> +		return -ENOSYS;
> +
> +	return ops->close(dev);
> +}
> +
> +int tis_get_desc(struct udevice *dev, char *buf, int size)
> +{
> +	struct tpm_ops *ops = tpm_get_ops(dev);
> +
> +	if (!ops->get_desc)
> +		return -ENOSYS;
> +
> +	return ops->get_desc(dev, buf, size);
> +}
> +
> +int tis_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> +	uint8_t *recvbuf, size_t *recv_size)
> +{
> +	struct tpm_ops *ops = tpm_get_ops(dev);
> +
> +	if (!ops->xfer)
> +		return -ENOSYS;
> +
> +	return ops->xfer(dev, sendbuf, send_size, recvbuf, recv_size);
> +}
tis_xfer could be more generic and rely on tpm_transmit from original tpm.c.
The command duration could be calculated at probe time during driver 
initialisation running one single getcapability command.
> +
> +UCLASS_DRIVER(tpm) = {
> +	.id             = UCLASS_TPM,
> +	.name           = "tpm",
> +	.flags          = DM_UC_FLAG_SEQ_ALIAS,
> +};
> diff --git a/include/dm/uclass-id.h b/include/dm/uclass-id.h
> index c744044..3eff895 100644
> --- a/include/dm/uclass-id.h
> +++ b/include/dm/uclass-id.h
> @@ -54,6 +54,7 @@ enum uclass_id {
>   	UCLASS_SPI_GENERIC,	/* Generic SPI flash target */
>   	UCLASS_SYSCON,		/* System configuration device */
>   	UCLASS_THERMAL,		/* Thermal sensor */
> +	UCLASS_TPM,		/* Trusted Platform Module TIS interface */
>   	UCLASS_USB,		/* USB bus */
>   	UCLASS_USB_DEV_GENERIC,	/* USB generic device */
>   	UCLASS_USB_HUB,		/* USB hub */
> diff --git a/include/tis.h b/include/tis.h
> index 40a1f86..6620554 100644
> --- a/include/tis.h
> +++ b/include/tis.h
> @@ -7,6 +7,102 @@
>   #ifndef __TIS_H
>   #define __TIS_H
>   
> +#ifdef CONFIG_DM_TPM
> +struct tpm_ops {
As per a previous comment, an init handler could be usefull.
> +	/**
> +	 * open() - Request access to locality 0 for the caller
> +	 *
> +	 * After all commands have been completed the caller should call
> +	 * tis_close().
> +	 *
> +	 * @dev:	Device to close
> +	 * @return 0 ok OK, -ve on error
> +	 */
> +	int (*open)(struct udevice *dev);
> +
> +	/**
> +	 * tis_close() - Close the current session
> +	 *
> +	 * Releasing the locked locality. Returns 0 on success, -ve 1 on
> +	 * failure (in case lock removal did not succeed).
> +	 *
> +	 * @dev:	Device to close
> +	 * @return 0 ok OK, -ve on error
> +	 */
> +	int (*close)(struct udevice *dev);
> +
> +	/**
> +	 * get_desc() - Get a text description of the TPM
> +	 *
> +	 * @dev:	Device to check
> +	 * @buf:	Buffer to put the string
> +	 * @size:	Maximum size of buffer
> +	 * @return length of string, or -ENOSPC it no space
> +	 */
> +	int (*get_desc)(struct udevice *dev, char *buf, int size);
> +
> +	/**
> +	 * xfer() - send data to the TPM and get response
> +	 *
> +	 * @dev:	Device to talk to
> +	 * @sendbuf:	Buffer of the data to send
> +	 * @send_size:	Size of the data to send
> +	 * @recvbuf:	Buffer to save the response to
> +	 * @recv_size:	Pointer to the size of the response buffer
> +	 *
> +	 * Returns 0 on success (and places the number of response bytes at
> +	 * recv_size) or -ve on failure.
> +	 */
> +	int (*xfer)(struct udevice *dev, const uint8_t *sendbuf,
> +		    size_t send_size, uint8_t *recvbuf, size_t *recv_size);
> +};
> +
> +#define tpm_get_ops(dev)        ((struct tpm_ops *)(dev)->driver->ops)
why not device_get_ops(dev) ?
> +
> +/*
> + * open() - Request access to locality 0 for the caller
> + *
> + * After all commands have been completed the caller is supposed to
> + * call tis_close().
> + *
> + * Returns 0 on success, -ve on failure.
> + */
> +int tis_open(struct udevice *dev);
> +
> +/*
> + * tis_close() - Close the current session
> + *
> + * Releasing the locked locality. Returns 0 on success, -ve 1 on
> + * failure (in case lock removal did not succeed).
> + */
> +int tis_close(struct udevice *dev);
> +
> +/**
> + * tis_get_desc() - Get a text description of the TPM
> + *
> + * @dev:	Device to check
> + * @buf:	Buffer to put the string
> + * @size:	Maximum size of buffer
> + * @return length of string, or -ENOSPC it no space
> + */
> +int tis_get_desc(struct udevice *dev, char *buf, int size);
> +
> +/*
> + * tis_sendrecv() - send data to the TPM and get response
> + *
> + * @sendbuf - buffer of the data to send
> + * @send_size size of the data to send
> + * @recvbuf - memory to save the response to
> + * @recv_len - pointer to the size of the response buffer
> + *
> + * Returns 0 on success (and places the number of response bytes at
> + * recv_len) or -ve on failure.
> + */
> +int tis_xfer(struct udevice *dev, const uint8_t *sendbuf, size_t send_size,
> +	     uint8_t *recvbuf, size_t *recv_size);
> +
As at the moment there is a 1 - 1 link with TPM and a platform, are you 
sure udevice should be a parameter ?
> +#else
> +
>   #include <common.h>
>   
>   /* Low-level interface to access TPM */
> @@ -53,5 +149,6 @@ int tis_close(void);
>    */
>   int tis_sendrecv(const uint8_t *sendbuf, size_t send_size, uint8_t *recvbuf,
>   			size_t *recv_len);
> +#endif
>   
>   #endif /* __TIS_H */
Best Regards
Christophe