Hi Ilias,
On Fri, 2 Jul 2021 at 14:29, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
On Fri, Jul 02, 2021 at 07:54:13PM +0200, Heinrich Schuchardt wrote:
On 7/2/21 2:52 PM, Ilias Apalodimas wrote:
Add a TPMv2 TIS MMIO compatible driver. This useful for a couple of reasons. First of all we can support a TPMv2 on devices that have this hardware (e.g the newly added SynQuacer box). We can also use the driver in our QEMU setups and test the EFI TCG2 protocol, which cannot be tested with our sandbox.
Ideally we should create an abstraction for all TIS compatible TPMs we support. Let's plug in the mmio driver first.
This would match Linux' linux/drivers/char/tpm/tpm_tis_core.c. The individual drivers provide the TIS level operations of struct tpm_tis_phy_ops and call tpm_tis_core_init() to load the tpm_tis_core module which supplies the TPM level operations of struct tpm_class_ops.
Why don't you start with a separate tis_core module supporting the tis_mmio driver? This would avoid pulling out the core functions out of this driver and provide a template for refactoring the other drivers.
Yep, I am commenting more on this below. [...]
+/*
- swtpm driver for TCG/TIS TPM (trusted platform module).
swtpm is not easy to understand. I would prefer something like
'Driver for the TPM software emulation provided by the swtpm package (https://github.com/stefanberger/swtpm)'.
Sure
- Specifications at www.trustedcomputinggroup.org
- */
+#include <common.h> +#include <dm.h> +#include <log.h> +#include <tpm-v2.h> +#include <linux/bitops.h> +#include <linux/compiler.h> +#include <linux/delay.h> +#include <linux/errno.h> +#include <linux/types.h> +#include <linux/io.h> +#include <linux/unaligned/be_byteshift.h> +#include "tpm_tis.h" +#include "tpm_internal.h"
+enum tis_int_flags {
- TPM_GLOBAL_INT_ENABLE = 0x80000000,
- TPM_INTF_BURST_COUNT_STATIC = 0x100,
- TPM_INTF_CMD_READY_INT = 0x080,
- TPM_INTF_INT_EDGE_FALLING = 0x040,
- TPM_INTF_INT_EDGE_RISING = 0x020,
- TPM_INTF_INT_LEVEL_LOW = 0x010,
- TPM_INTF_INT_LEVEL_HIGH = 0x008,
- TPM_INTF_LOCALITY_CHANGE_INT = 0x004,
- TPM_INTF_STS_VALID_INT = 0x002,
- TPM_INTF_DATA_AVAIL_INT = 0x001,
+};
These definitions match Linux' tpm_tis_core.h.
Should they be moved to an include of the same name in U-Boot?
I got a tpm_tis.h in this series, so I might as well move those there
[...]
I would prefer if you would add these functions to a structure struct tpm_tis_phy_ops which you can use to pull out the core driver.
Me too :). In fact that matches the design I proposed over IRC. I just didn't have too much free time to fix it, so I went ahead and posted the driver matching what's already in U-Boot. That being said, what's proposed here is the right was forward. Basically if we do it like this then any future TPM TIS driver will only have to define the read/write ops for the underlyng bus.
I'm not sure about the irc side, but it is best to do things on the mailing list so people can see it.
There seems to be quite a bit of duplicated code in this new driver. I think it would be better to come up with the interface first, as Heinrich suggests. Would something like regmap be good enough?
Regards, Simon