From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-16.0 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id CE8EEC64E7C for ; Sun, 29 Nov 2020 03:30:36 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 72FD720829 for ; Sun, 29 Nov 2020 03:30:36 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (1024-bit key) header.d=kernel.org header.i=@kernel.org header.b="lMrDZ3lx" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1726343AbgK2DaV (ORCPT ); Sat, 28 Nov 2020 22:30:21 -0500 Received: from mail.kernel.org ([198.145.29.99]:43574 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1725616AbgK2DaU (ORCPT ); Sat, 28 Nov 2020 22:30:20 -0500 Received: from kernel.org (83-245-197-237.elisa-laajakaista.fi [83.245.197.237]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPSA id 746912080D; Sun, 29 Nov 2020 03:29:37 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=default; t=1606620579; bh=72TodPAIlIa4JUl4YdHHD3J7JAjew3h7a3MJtL/4TFs=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=lMrDZ3lxAfYNvC/R6ZI4w+Vt9io/Qw16LQBRW7oKZRAGLPA+aQ4qx7vP0+eq0uknk 3alVfpmQEiS4VqlQuUMXC6pU2RvDLDsLmiydaP9+7XUXT+zoviqRw8zHzsEVh68/yM VB4BttGeTrC2ZV/Lbvsp5CtIykOyVS0c8hvaGVbo= Date: Sun, 29 Nov 2020 05:29:33 +0200 From: Jarkko Sakkinen To: Ezequiel Garcia Cc: Adrian Ratiu , linux-integrity@vger.kernel.org, Peter Huewe , Jason Gunthorpe , Helen Koike , Duncan Laurie , Stephen Boyd , kernel@collabora.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] char: tpm: add i2c driver for cr50 Message-ID: <20201129032933.GE39488@kernel.org> References: <20201120172345.4040187-1-adrian.ratiu@collabora.com> <20201123220643.GA16777@kernel.org> <7edf80b70e4dd67d6f95c796c1ae26df9e51ba8d.camel@kernel.org> <6409c32842ab080d91d1851a58f7ec7bb4524336.camel@collabora.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <6409c32842ab080d91d1851a58f7ec7bb4524336.camel@collabora.com> Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Nov 26, 2020 at 03:19:24AM -0300, Ezequiel Garcia 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@chromium.org" > > > > > > > > > > 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 > > > > > Signed-off-by: Duncan Laurie > > > > > [swboyd@chromium.org: 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 > > > > > Signed-off-by: Fabien Lahoudere > > > > > Signed-off-by: Adrian Ratiu > > > > > --- > > > > > 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 > > > > > + * 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 > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > +#include > > > > > + > > > > > +#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 It's an internal function to begin with. /Jarkko