Re: [PATCH v2] char: tpm: add i2c driver for cr50

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, 26 Nov 2020, Ezequiel Garcia <ezequiel@xxxxxxxxxxxxx> wrote:
On Thu, 2020-11-26 at 05:30 +0200, Jarkko Sakkinen wrote:
On Tue, 2020-11-24 at 10:14 -0300, Ezequiel Garcia wrote: > Hi Jarkko, Thanks for your review. On Tue, 2020-11-24 at > 00:06 +0200, Jarkko Sakkinen wrote: > > On Fri, Nov 20, 2020 at 07:23:45PM +0200, Adrian Ratiu > > wrote: > > > From: "dlaurie@xxxxxxxxxxxx" <dlaurie@xxxxxxxxxxxx> Add > > > TPM 2.0 compatible I2C interface for chips with cr50 > > > firmware. The firmware running on the currently > > > supported H1 MCU requires a special driver to handle its > > > specific protocol, and this makes it unsuitable to use > > > tpm_tis_core_* and instead it must implement the > > > underlying TPM protocol similar to the other I2C TPM > > > drivers. - All 4 byes of status register must be > > > read/written at once. - FIFO and burst count is limited > > > to 63 and must be drained by AP. - Provides an interrupt > > > to indicate when read response data is ready and when the > > > TPM is finished processing write data. This driver is > > > based on the existing infineon I2C TPM driver, which most > > > closely matches the cr50 i2c protocol behavior. Cc: > > > Helen Koike <helen.koike@xxxxxxxxxxxxx> Signed-off-by: > > > Duncan Laurie <dlaurie@xxxxxxxxxxxx> > > > [swboyd@xxxxxxxxxxxx: Depend on i2c even if it's a > > > module, replace boilier plate with SPDX tag, drop > > > asm/byteorder.h include, simplify return from probe] > > > Signed-off-by: Stephen Boyd <swboyd@xxxxxxxxxxxx> > > > Signed-off-by: Fabien Lahoudere > > > <fabien.lahoudere@xxxxxxxxxxxxx> Signed-off-by: Adrian > > > Ratiu <adrian.ratiu@xxxxxxxxxxxxx> --- Changes in v2: > > > - Various small fixes all over (reorder includes, > > > MAX_BUFSIZE, > > > comments, etc) > > > - Reworked return values of i2c_wait_tpm_ready() to fix > > > timeout > > > mis-handling so ret == 0 now means success, the wait > > > period jiffies is ignored because that number is > > > meaningless and return a proper timeout error in case > > > jiffies == 0. > > > - Make i2c default to 1 message per transfer (requested > > > by > > > Helen) > > > - Move -EIO error reporting to transfer function to > > > cleanup > > > transfer() itself and its R/W callers > > > - Remove magic value hardcodings and introduce enum > > > force_release. v1 posted at > > > https://lkml.org/lkml/2020/2/25/349 Applies on > > > next-20201120, tested on Chromebook EVE. --- > > > drivers/char/tpm/Kconfig | 10 + > > > drivers/char/tpm/Makefile | 2 + > > > drivers/char/tpm/tpm_tis_i2c_cr50.c | 768 > > > ++++++++++++++++++++++++++++ > > > 3 files changed, 780 insertions(+) create mode 100644 > > > drivers/char/tpm/tpm_tis_i2c_cr50.c > > > diff --git a/drivers/char/tpm/Kconfig > > > b/drivers/char/tpm/Kconfig index > > > a18c314da211..4308f9ca7a43 100644 --- > > > a/drivers/char/tpm/Kconfig +++ b/drivers/char/tpm/Kconfig > > > @@ -86,6 +86,16 @@ config TCG_TIS_SYNQUACER > > > To compile this driver as a module, choose M > > > here; the module will be called > > > tpm_tis_synquacer. > > > > > > +config TCG_TIS_I2C_CR50 + tristate "TPM Interface > > > Specification 2.0 Interface (I2C - CR50)" + depends > > > on I2C + select TCG_CR50 + help + > > > This is a driver for the Google cr50 I2C TPM interface > > > which is a + custom microcontroller and requires > > > a custom i2c protocol interface + to handle the > > > limitations of the hardware. To compile this driver + > > > as a module, choose M here; the module will be called > > > tcg_tis_i2c_cr50. + > > > config TCG_TIS_I2C_ATMEL > > > tristate "TPM Interface Specification 1.2 > > > Interface (I2C > > > - Atmel)" > > > depends on I2C > > > diff --git a/drivers/char/tpm/Makefile > > > b/drivers/char/tpm/Makefile index > > > 84db4fb3a9c9..66d39ea6bd10 100644 --- > > > a/drivers/char/tpm/Makefile +++ > > > b/drivers/char/tpm/Makefile @@ -27,6 +27,8 @@ > > > obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o > > > tpm_tis_spi-y := tpm_tis_spi_main.o > > > tpm_tis_spi-$(CONFIG_TCG_TIS_SPI_CR50) += > > > tpm_tis_spi_cr50.o > > > +obj-$(CONFIG_TCG_TIS_I2C_CR50) += tpm_tis_i2c_cr50.o + > > > obj-$(CONFIG_TCG_TIS_I2C_ATMEL) += tpm_i2c_atmel.o > > > obj-$(CONFIG_TCG_TIS_I2C_INFINEON) += tpm_i2c_infineon.o > > > obj-$(CONFIG_TCG_TIS_I2C_NUVOTON) += tpm_i2c_nuvoton.o > > > diff --git a/drivers/char/tpm/tpm_tis_i2c_cr50.c > > > b/drivers/char/tpm/tpm_tis_i2c_cr50.c new file mode > > > 100644 index 000000000000..37555dafdca0 --- /dev/null +++ > > > b/drivers/char/tpm/tpm_tis_i2c_cr50.c @@ -0,0 +1,768 @@ > > > +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright > > > 2016 Google Inc. + * + * Based on Linux Kernel TPM > > > driver by + * Peter Huewe <peter.huewe@xxxxxxxxxxxx> + * > > > Copyright (C) 2011 Infineon Technologies + */ + +/* + * > > > cr50 is a firmware for H1 secure modules that requires > > > special + * handling for the I2C interface. + * + * - > > > Use an interrupt for transaction status instead of > > > hardcoded delays + * - Must use write+wait+read read > > > protocol + * - All 4 bytes of status register must be > > > read/written at once + * - Burst count max is 63 bytes, > > > and burst count behaves + * slightly differently than > > > other I2C TPMs + * - When reading from FIFO the full > > > burstcnt must be read + * instead of just reading > > > header and determining the remainder + */ + +#include > > > <linux/acpi.h> +#include <linux/completion.h> +#include > > > <linux/i2c.h> +#include <linux/interrupt.h> +#include > > > <linux/module.h> +#include <linux/pm.h> +#include > > > <linux/slab.h> +#include <linux/wait.h> + +#include > > > "tpm_tis_core.h" + +#define CR50_MAX_BUFSIZE 64 > > > +#define CR50_TIMEOUT_SHORT_MS 2 /* Short timeout > > > during transactions */ +#define CR50_TIMEOUT_NOIRQ_MS 20 > > > /* Timeout for TPM ready without IRQ */ +#define > > > CR50_I2C_DID_VID 0x00281ae0L +#define > > > CR50_I2C_MAX_RETRIES 3 /* Max retries due to I2C > > > errors */ +#define CR50_I2C_RETRY_DELAY_LO 55 > > > /* Min usecs between retries on I2C */ +#define > > > CR50_I2C_RETRY_DELAY_HI 65 /* Max usecs > > > between retries on I2C */ > > CR50_ -> TPM_CR50_ > > > + +#define TPM_I2C_ACCESS(l) (0x0000 | ((l) << 4)) > > > +#define TPM_I2C_STS(l) (0x0001 | ((l) << 4)) > > > +#define TPM_I2C_DATA_FIFO(l) (0x0005 | ((l) << 4)) > > > +#define TPM_I2C_DID_VID(l) (0x0006 | ((l) << 4)) + > > > +struct priv_data { + int irq; + int > > > locality; + struct completion tpm_ready; + u8 > > > buf[CR50_MAX_BUFSIZE]; +}; > > tpm_i2c_cr50_priv_data > > > + +enum force_release { + CR50_NO_FORCE = 0x0, + > > > CR50_FORCE = 0x1, +}; > > I'd just #define TPM_I2C_CR50_NO_FORCE 0 #define > > TPM_I2C_CR50_FORCE 1 > A proper enumerated type has advantages over a preprocessor > macro: even if the compiler won't warn you, static analyzers > can warn about a misuse. Why don't you just use "bool", "true" and "false"? I ignored that this has nothing to do with the hardware last time.

Well, boolean parameters are a known anti-pattern [1]. [1] https://people.mpi-inf.mpg.de/~jblanche/api-design.pdf

Funny because that's what I wrote a few days ago in my v3 WIP branch, #defines and a bool function parameter. XD

As Jarkko correctly observed these values are unrelated to the HW, we just need to distinguish FORCE vs NO_FORCE and IMO any method will do just as well for such a small and obvious use case.

So based on the arguments given, I'll stop bikeshedding and just use enums :)

Thank you both,
Adrian



[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Linux Kernel]     [Linux Kernel Hardening]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux SCSI]

  Powered by Linux