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=-10.8 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, MENTIONS_GIT_HOSTING,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham 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 91184C07E96 for ; Fri, 2 Jul 2021 20:29:31 +0000 (UTC) Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 98192613BC for ; Fri, 2 Jul 2021 20:29:30 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 98192613BC Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id A2596828A1; Fri, 2 Jul 2021 22:29:28 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.b="qUgQqoPo"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 8B7408296F; Fri, 2 Jul 2021 22:29:26 +0200 (CEST) Received: from mail-wr1-x431.google.com (mail-wr1-x431.google.com [IPv6:2a00:1450:4864:20::431]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 375FC81D48 for ; Fri, 2 Jul 2021 22:29:23 +0200 (CEST) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=ilias.apalodimas@linaro.org Received: by mail-wr1-x431.google.com with SMTP id a13so13840156wrf.10 for ; Fri, 02 Jul 2021 13:29:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=HKHNvR03uehbo5oiaaVbdaXMlQpvxr0NNGaSaSQ++SA=; b=qUgQqoPoXNPO+XGK+I/Q/bgQ9uR+MwjWW/+dwKyaoahNAmhaVONiiCLYUUDxFj7Ju9 vuiGwCCfUEGDTuM0mM/n2Tqxibcepxf6O6vlreBcfzk5wwuRj4DqausKHauJEXYY90q5 XRPhnJFExLNcBK2L3lYpKUfmNBlo+jra8Nzu5SQhbCKq5fXGwo+QmQqe+elqBpxKNMUU TPDMJEXvBckFgtLbK5aX6Bvqcm/AdW8RC21HH4lATQE+ckZ7c5kYdpDFgKfnr7V1sto4 lsFFZtQHAiqoadg3xuwOrLgUsTFhYjlgrszAKg19luX9p7A6vB48+K6GQngRXlppmPik Pz3g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=HKHNvR03uehbo5oiaaVbdaXMlQpvxr0NNGaSaSQ++SA=; b=jZLzuNtgAX2GACW2VDlZqLR0/QgDmwVfl8h6Fx9zxnGUdGLDFr/E425dJBsl/pWuzH fpRTJcQ9I1a9BaKuZBkL4sidiVinN0E1g+DBwI10kropIHgNfpvkuW3S8irS1677gXIz VOz7l2AlHFwefhuN2D/zzvLsgHtHnk9WqbBaf5G+KgktojS7bZTL71QtqEarOPffA0Aq yoi4S8HEzwgXYAzkozvgpAwGuoCc5jIjyt6ARVAtehpBwpWAD5iuAPpxvEDsAFhnuL9E Jn4T9U2yy5p97Vu0wHDxnSklq4sTcD1jWPEuwdtG9uLE4NQWm0p2ALaPN+oEcB4KTOrr 3ulg== X-Gm-Message-State: AOAM530tfPrNecmeZFzRfMoWFuRmNCN5ldum3lRknUrlylXjj7sZyyNr etvPBMzs3AjyukjEHsgmS+iccw== X-Google-Smtp-Source: ABdhPJxYfwrumHhGdxbbLrYoNCu9jDPGmS53n3XnF3+wmk+HosO46UFEVEIwht9Y/Vg8fMmyMM50XQ== X-Received: by 2002:adf:d203:: with SMTP id j3mr1583134wrh.292.1625257762742; Fri, 02 Jul 2021 13:29:22 -0700 (PDT) Received: from enceladus (ppp-94-66-242-227.home.otenet.gr. [94.66.242.227]) by smtp.gmail.com with ESMTPSA id y16sm4121261wrw.42.2021.07.02.13.29.21 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 02 Jul 2021 13:29:22 -0700 (PDT) Date: Fri, 2 Jul 2021 23:29:19 +0300 From: Ilias Apalodimas To: Heinrich Schuchardt Cc: Simon Glass , Johannes Holland , Dhananjay Phadke , u-boot@lists.denx.de Subject: Re: [PATCH 1/1] tpm2: Add a TPMv2 MMIO TIS driver Message-ID: References: <20210702125234.19145-1-ilias.apalodimas@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.34 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.2 at phobos.denx.de X-Virus-Status: Clean 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 > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#include > > +#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. > > Best regards > > Heinrich > > > + > > +static int tpm_tis_write_bytes(struct udevice *udev, u32 addr, u16 len, > > + const u8 *value) > > +{ > > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > > + > > + while (len--) > > + iowrite8(*value++, drv_data->iobase + addr); > > + return 0; > > +} > > + > > +static __maybe_unused int tpm_tis_read16(struct udevice *udev, u32 addr, > > + u16 *result) > > +{ > > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > > + > > + *result = ioread16(drv_data->iobase + addr); > > + return 0; > > +} > > + > > +static int tpm_tis_read32(struct udevice *udev, u32 addr, u32 *result) > > +{ > > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > > + > > + *result = ioread32(drv_data->iobase + addr); > > + return 0; > > +} > > + > > +static int tpm_tis_write32(struct udevice *udev, u32 addr, u32 value) > > +{ > > + struct tpm_tis_chip_data *drv_data = (void *)dev_get_driver_data(udev); > > + > > + iowrite32(value, drv_data->iobase + addr); > > + return 0; > > +} > > + > > +static int tpm_tis_get_desc(struct udevice *udev, char *buf, int size) > > +{ > > + struct tpm_chip *chip = dev_get_priv(udev); > > + > > + if (size < 80) > > + return -ENOSPC; > > + > > + return snprintf(buf, size, > > + "%s v2.0: VendorID 0x%04x, DeviceID 0x%04x, RevisionID 0x%02x [%s]", > > + udev->name, chip->vend_dev & 0xFFFF, > > + chip->vend_dev >> 16, chip->rid, > > + (chip->is_open ? "open" : "closed")); > > +} > > + > > +static bool tpm_tis_check_locality(struct udevice *udev, int loc) > > +{ > > + struct tpm_chip *chip = dev_get_priv(udev); > > + u8 locality; > > + > > + tpm_tis_read_bytes(udev, TPM_ACCESS(loc), 1, &locality); > > + if ((locality & (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID | > > + TPM_ACCESS_REQUEST_USE)) == > > + (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID)) { > > + chip->locality = loc; > > + return true; > > + } > > + > > + return false; > > +} > > + > > +static int tpm_tis_request_locality(struct udevice *udev, int loc) > > +{ > > + struct tpm_chip *chip = dev_get_priv(udev); > > + u8 buf = TPM_ACCESS_REQUEST_USE; > > + unsigned long start, stop; > > + > > + if (tpm_tis_check_locality(udev, loc)) > > + return 0; > > + > > + tpm_tis_write_bytes(udev, TPM_ACCESS(loc), 1, &buf); > > + start = get_timer(0); > > + stop = chip->timeout_a; > > + do { > > + if (tpm_tis_check_locality(udev, loc)) > > + return 0; > > + mdelay(TPM_TIMEOUT_MS); > > + } while (get_timer(start) < stop); > > + > > + return -1; > > +} > > + > > +static int tpm_tis_status(struct udevice *udev, u8 *status) > > +{ > > + struct tpm_chip *chip = dev_get_priv(udev); > > + > > + if (chip->locality < 0) > > + return -EINVAL; > > + > > + tpm_tis_read_bytes(udev, TPM_STS(chip->locality), 1, status); > > + > > + if ((*status & TPM_STS_READ_ZERO)) { > > + log_err("TPM returned invalid status\n"); > > + return -EINVAL; > > + } > > + > > + return 0; > > +} > > + > > +static int tpm_tis_release_locality(struct udevice *udev, int loc) > > +{ > > + struct tpm_chip *chip = dev_get_priv(udev); > > + u8 buf = TPM_ACCESS_ACTIVE_LOCALITY; > > + int ret; > > + > > + if (chip->locality < 0) > > + return 0; > > + > > + ret = tpm_tis_write_bytes(udev, TPM_ACCESS(loc), 1, &buf); > > + chip->locality = -1; > > + > > + return ret; > > +} > > + > > +static int tpm_tis_wait_for_stat(struct udevice *udev, u8 mask, > > + unsigned long timeout, u8 *status) > > +{ > > + unsigned long start = get_timer(0); > > + unsigned long stop = timeout;