Tegra234 and Tegra241 chips have QSPI controller that supports TCG
PC Client Specific TPM Interface Specification (TIS) flow control.
Since the controller only supports half duplex, sw wait polling
(flow control using full duplex transfers) method implemented in
tpm_tis_spi_main.c does not suffice.
Added extended driver to disable sw flow control and send
all transfers in single message. Flow control is handled by hardware.
V2: Fix dt schema errors.
Krishna Yarlagadda (4):
dt-bindings: tpm: Add compatible for Tegra TPM
tpm: tegra: Support SPI tpm wait state detect
spi: dt-bindings: Add Tegra TPM wait polling flag
spi: tegra210-quad: Enable TPM wait polling
.../security/tpm/nvidia,tegra-tpm-spi.yaml | 34 +++++
...nvidia,tegra210-quad-peripheral-props.yaml | 6 +
drivers/char/tpm/Makefile | 1 +
drivers/char/tpm/tpm_tis_spi.h | 1 +
drivers/char/tpm/tpm_tis_spi_main.c | 4 +-
drivers/char/tpm/tpm_tis_spi_tegra.c | 123 ++++++++++++++++++
drivers/spi/spi-tegra210-quad.c | 16 +++
7 files changed, 184 insertions(+), 1 deletion(-)
create mode 100644 Documentation/devicetree/bindings/security/tpm/nvidia,tegra-tpm-spi.yaml
create mode 100644 drivers/char/tpm/tpm_tis_spi_tegra.c
--
2.17.1
Tegra234 and Tegra241 devices have QSPI controller that supports TPM
devices. Since the controller only supports half duplex, sw wait polling
method implemented in tpm_tis_spi does not suffice. Wait polling as per
protocol is a hardware feature.
Add compatible for Tegra TPM driver with hardware flow control.
Signed-off-by: Krishna Yarlagadda <[email protected]>
---
.../security/tpm/nvidia,tegra-tpm-spi.yaml | 34 +++++++++++++++++++
1 file changed, 34 insertions(+)
create mode 100644 Documentation/devicetree/bindings/security/tpm/nvidia,tegra-tpm-spi.yaml
diff --git a/Documentation/devicetree/bindings/security/tpm/nvidia,tegra-tpm-spi.yaml b/Documentation/devicetree/bindings/security/tpm/nvidia,tegra-tpm-spi.yaml
new file mode 100644
index 000000000000..dcb78db7355c
--- /dev/null
+++ b/Documentation/devicetree/bindings/security/tpm/nvidia,tegra-tpm-spi.yaml
@@ -0,0 +1,34 @@
+# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/security/tpm/nvidia,tegra-tpm-spi.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Tegra QSPI TPM driver
+
+maintainers:
+ - Thierry Reding <[email protected]>
+ - Jonathan Hunter <[email protected]>
+
+properties:
+ compatible:
+ enum:
+ - nvidia,tegra-tpm-spi
+
+ reg:
+ maxItems: 1
+
+required:
+ - compatible
+ - reg
+
+unevaluatedProperties: false
+
+examples:
+ - |
+ qspi1@3250000 {
+ tpm@0 {
+ compatible = "nvidia,tegra-tpm-spi";
+ reg = <0>;
+ };
+ };
--
2.17.1
Tegra234 and Tegra241 chips have QSPI controller that supports TCG
TIS hardware flow control. Since the controller only supports half
duplex, sw wait polling method implemented in tpm_tis_spi does not
suffice. Added extending driver to disable sw flow control and send
all transfers in single message.
Signed-off-by: Krishna Yarlagadda <[email protected]>
---
drivers/char/tpm/Makefile | 1 +
drivers/char/tpm/tpm_tis_spi.h | 1 +
drivers/char/tpm/tpm_tis_spi_main.c | 4 +-
drivers/char/tpm/tpm_tis_spi_tegra.c | 123 +++++++++++++++++++++++++++
4 files changed, 128 insertions(+), 1 deletion(-)
create mode 100644 drivers/char/tpm/tpm_tis_spi_tegra.c
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 0222b1ddb310..445b15493cb3 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -25,6 +25,7 @@ obj-$(CONFIG_TCG_TIS_SYNQUACER) += tpm_tis_synquacer.o
obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o
tpm_tis_spi-y := tpm_tis_spi_main.o
+tpm_tis_spi-y += tpm_tis_spi_tegra.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
diff --git a/drivers/char/tpm/tpm_tis_spi.h b/drivers/char/tpm/tpm_tis_spi.h
index d0f66f6f1931..feaea14b428b 100644
--- a/drivers/char/tpm/tpm_tis_spi.h
+++ b/drivers/char/tpm/tpm_tis_spi.h
@@ -31,6 +31,7 @@ extern int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy,
extern int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
u8 *in, const u8 *out);
+extern int tegra_tpm_spi_probe(struct spi_device *spi);
#ifdef CONFIG_TCG_TIS_SPI_CR50
extern int cr50_spi_probe(struct spi_device *spi);
#else
diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c
index a0963a3e92bd..5d4502a4461a 100644
--- a/drivers/char/tpm/tpm_tis_spi_main.c
+++ b/drivers/char/tpm/tpm_tis_spi_main.c
@@ -198,7 +198,7 @@ static int tpm_tis_spi_driver_probe(struct spi_device *spi)
const struct spi_device_id *spi_dev_id = spi_get_device_id(spi);
tpm_tis_spi_probe_func probe_func;
- probe_func = of_device_get_match_data(&spi->dev);
+ probe_func = device_get_match_data(&spi->dev);
if (!probe_func) {
if (spi_dev_id) {
probe_func = (tpm_tis_spi_probe_func)spi_dev_id->driver_data;
@@ -227,6 +227,7 @@ static const struct spi_device_id tpm_tis_spi_id[] = {
{ "tpm_tis_spi", (unsigned long)tpm_tis_spi_probe },
{ "tpm_tis-spi", (unsigned long)tpm_tis_spi_probe },
{ "cr50", (unsigned long)cr50_spi_probe },
+ { "tegra-tpm-spi", (unsigned long)tegra_tpm_spi_probe },
{}
};
MODULE_DEVICE_TABLE(spi, tpm_tis_spi_id);
@@ -236,6 +237,7 @@ static const struct of_device_id of_tis_spi_match[] = {
{ .compatible = "infineon,slb9670", .data = tpm_tis_spi_probe },
{ .compatible = "tcg,tpm_tis-spi", .data = tpm_tis_spi_probe },
{ .compatible = "google,cr50", .data = cr50_spi_probe },
+ { .compatible = "nvidia,tegra-tpm-spi", .data = tegra_tpm_spi_probe },
{}
};
MODULE_DEVICE_TABLE(of, of_tis_spi_match);
diff --git a/drivers/char/tpm/tpm_tis_spi_tegra.c b/drivers/char/tpm/tpm_tis_spi_tegra.c
new file mode 100644
index 000000000000..23f20684513d
--- /dev/null
+++ b/drivers/char/tpm/tpm_tis_spi_tegra.c
@@ -0,0 +1,123 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2023 NVIDIA CORPORATION.
+ *
+ * This device driver implements TEGRA QSPI hw wait detection for chips
+ *
+ * It is based on tpm_tis_spi driver by Peter Huewe and Christophe Ricard.
+ */
+
+#include <linux/completion.h>
+#include <linux/interrupt.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/pm.h>
+#include <linux/spi/spi.h>
+#include <linux/wait.h>
+
+#include "tpm_tis_core.h"
+#include "tpm_tis_spi.h"
+
+#define MAX_SPI_FRAMESIZE 64
+
+int tpm_tis_spi_tegra_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
+ u8 *in, const u8 *out)
+{
+ struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
+ int ret = 0;
+ struct spi_message m;
+ struct spi_transfer spi_xfer[3];
+ u8 transfer_len;
+
+ spi_bus_lock(phy->spi_device->master);
+
+ while (len) {
+ transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
+
+ spi_message_init(&m);
+ phy->iobuf[0] = (in ? 0x80 : 0) | (transfer_len - 1);
+ phy->iobuf[1] = 0xd4;
+ phy->iobuf[2] = addr >> 8;
+ phy->iobuf[3] = addr;
+
+ memset(&spi_xfer, 0, sizeof(spi_xfer));
+
+ spi_xfer[0].tx_buf = phy->iobuf;
+ spi_xfer[0].len = 1;
+ spi_message_add_tail(&spi_xfer[0], &m);
+
+ spi_xfer[1].tx_buf = phy->iobuf + 1;
+ spi_xfer[1].len = 3;
+ spi_message_add_tail(&spi_xfer[1], &m);
+
+ if (out) {
+ spi_xfer[2].tx_buf = &phy->iobuf[4];
+ spi_xfer[2].rx_buf = NULL;
+ memcpy(&phy->iobuf[4], out, transfer_len);
+ out += transfer_len;
+ }
+ if (in) {
+ spi_xfer[2].tx_buf = NULL;
+ spi_xfer[2].rx_buf = &phy->iobuf[4];
+ }
+ spi_xfer[2].len = transfer_len;
+ spi_message_add_tail(&spi_xfer[2], &m);
+
+ reinit_completion(&phy->ready);
+ ret = spi_sync_locked(phy->spi_device, &m);
+ if (ret < 0)
+ goto exit;
+
+ if (in) {
+ memcpy(in, &phy->iobuf[4], transfer_len);
+ in += transfer_len;
+ }
+
+ len -= transfer_len;
+ }
+
+exit:
+ spi_bus_unlock(phy->spi_device->master);
+ return ret;
+}
+
+static int tpm_tis_spi_tegra_read_bytes(struct tpm_tis_data *data, u32 addr,
+ u16 len, u8 *result,
+ enum tpm_tis_io_mode io_mode)
+{
+ return tpm_tis_spi_tegra_transfer(data, addr, len, result, NULL);
+}
+
+static int tpm_tis_spi_tegra_write_bytes(struct tpm_tis_data *data, u32 addr,
+ u16 len, const u8 *value,
+ enum tpm_tis_io_mode io_mode)
+{
+ return tpm_tis_spi_tegra_transfer(data, addr, len, NULL, value);
+}
+
+static const struct tpm_tis_phy_ops tegra_tpm_spi_phy_ops = {
+ .read_bytes = tpm_tis_spi_tegra_read_bytes,
+ .write_bytes = tpm_tis_spi_tegra_write_bytes,
+};
+
+int tegra_tpm_spi_probe(struct spi_device *dev)
+{
+ struct tpm_tis_spi_phy *phy;
+ int irq;
+
+ phy = devm_kzalloc(&dev->dev, sizeof(struct tpm_tis_spi_phy),
+ GFP_KERNEL);
+ if (!phy)
+ return -ENOMEM;
+
+ phy->flow_control = NULL;
+
+ /* If the SPI device has an IRQ then use that */
+ if (dev->irq > 0)
+ irq = dev->irq;
+ else
+ irq = -1;
+
+ init_completion(&phy->ready);
+ return tpm_tis_spi_init(dev, phy, irq, &tegra_tpm_spi_phy_ops);
+}
--
2.17.1
Add "nvidia,wait-polling" flag to enable TCG TIS hardware flow control.
Signed-off-by: Krishna Yarlagadda <[email protected]>
---
.../bindings/spi/nvidia,tegra210-quad-peripheral-props.yaml | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/devicetree/bindings/spi/nvidia,tegra210-quad-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/nvidia,tegra210-quad-peripheral-props.yaml
index 2c3cada75339..19d2b30cadbf 100644
--- a/Documentation/devicetree/bindings/spi/nvidia,tegra210-quad-peripheral-props.yaml
+++ b/Documentation/devicetree/bindings/spi/nvidia,tegra210-quad-peripheral-props.yaml
@@ -29,4 +29,10 @@ properties:
minimum: 0
maximum: 255
+ nvidia,wait-polling:
+ description:
+ Enable TPM wait polling feature for QSPI as specified in TCG PC Client
+ Specific TPM Interface Specification (TIS).
+ $ref: /schemas/types.yaml#/definitions/flag
+
additionalProperties: true
--
2.17.1
Trusted Platform Module defines flow control where slave will drive
data line at specified clock cycles. Tegra241 has TPM wait state
detections support when using combined sequence transfers. Enabling
the feature based on device tree flag.
Signed-off-by: Krishna Yarlagadda <[email protected]>
---
drivers/spi/spi-tegra210-quad.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/spi/spi-tegra210-quad.c b/drivers/spi/spi-tegra210-quad.c
index 9f356612ba7e..ea8a08a3d838 100644
--- a/drivers/spi/spi-tegra210-quad.c
+++ b/drivers/spi/spi-tegra210-quad.c
@@ -142,6 +142,7 @@
#define QSPI_GLOBAL_CONFIG 0X1a4
#define QSPI_CMB_SEQ_EN BIT(0)
+#define QSPI_TPM_WAIT_POLL_EN BIT(1)
#define QSPI_CMB_SEQ_ADDR 0x1a8
#define QSPI_ADDRESS_VALUE_SET(X) (((x) & 0xFFFF) << 0)
@@ -170,6 +171,7 @@ struct tegra_qspi_soc_data {
struct tegra_qspi_client_data {
int tx_clk_tap_delay;
int rx_clk_tap_delay;
+ bool wait_polling;
};
struct tegra_qspi {
@@ -934,6 +936,8 @@ static struct tegra_qspi_client_data *tegra_qspi_parse_cdata_dt(struct spi_devic
&cdata->tx_clk_tap_delay);
device_property_read_u32(&spi->dev, "nvidia,rx-clk-tap-delay",
&cdata->rx_clk_tap_delay);
+ cdata->wait_polling =
+ device_property_read_bool(&spi->dev, "nvidia,wait-polling");
return cdata;
}
@@ -991,6 +995,14 @@ static void tegra_qspi_dump_regs(struct tegra_qspi *tqspi)
dev_dbg(tqspi->dev, "TRANS_STAT: 0x%08x | FIFO_STATUS: 0x%08x\n",
tegra_qspi_readl(tqspi, QSPI_TRANS_STATUS),
tegra_qspi_readl(tqspi, QSPI_FIFO_STATUS));
+ dev_dbg(tqspi->dev, "GLOBAL_CFG: 0x%08x\n",
+ tegra_qspi_readl(tqspi, QSPI_GLOBAL_CONFIG));
+ dev_dbg(tqspi->dev, "CMB_CMD: 0x%08x | CMB_CMD_CFG: 0x%08x\n",
+ tegra_qspi_readl(tqspi, QSPI_CMB_SEQ_CMD),
+ tegra_qspi_readl(tqspi, QSPI_CMB_SEQ_CMD_CFG));
+ dev_dbg(tqspi->dev, "CMB_ADDR: 0x%08x | CMB_ADDR_CFG: 0x%08x\n",
+ tegra_qspi_readl(tqspi, QSPI_CMB_SEQ_ADDR),
+ tegra_qspi_readl(tqspi, QSPI_CMB_SEQ_ADDR_CFG));
}
static void tegra_qspi_handle_error(struct tegra_qspi *tqspi)
@@ -1056,6 +1068,7 @@ static int tegra_qspi_combined_seq_xfer(struct tegra_qspi *tqspi,
bool is_first_msg = true;
struct spi_transfer *xfer;
struct spi_device *spi = msg->spi;
+ struct tegra_qspi_client_data *cdata = spi->controller_data;
u8 transfer_phase = 0;
u32 cmd1 = 0, dma_ctl = 0;
int ret = 0;
@@ -1065,6 +1078,8 @@ static int tegra_qspi_combined_seq_xfer(struct tegra_qspi *tqspi,
/* Enable Combined sequence mode */
val = tegra_qspi_readl(tqspi, QSPI_GLOBAL_CONFIG);
+ if (cdata->wait_polling)
+ val |= QSPI_TPM_WAIT_POLL_EN;
val |= QSPI_CMB_SEQ_EN;
tegra_qspi_writel(tqspi, val, QSPI_GLOBAL_CONFIG);
/* Process individual transfer list */
@@ -1192,6 +1207,7 @@ static int tegra_qspi_non_combined_seq_xfer(struct tegra_qspi *tqspi,
/* Disable Combined sequence mode */
val = tegra_qspi_readl(tqspi, QSPI_GLOBAL_CONFIG);
val &= ~QSPI_CMB_SEQ_EN;
+ val &= ~QSPI_TPM_WAIT_POLL_EN;
tegra_qspi_writel(tqspi, val, QSPI_GLOBAL_CONFIG);
list_for_each_entry(transfer, &msg->transfers, transfer_list) {
struct spi_transfer *xfer = transfer;
--
2.17.1
On Fri, Feb 3, 2023 at 7:02 AM Krishna Yarlagadda
<[email protected]> wrote:
>
Please use get_maintainers.pl. In particular, resend to DT list so
automated checks work.
> Tegra234 and Tegra241 devices have QSPI controller that supports TPM
> devices. Since the controller only supports half duplex, sw wait polling
> method implemented in tpm_tis_spi does not suffice. Wait polling as per
> protocol is a hardware feature.
>
> Add compatible for Tegra TPM driver with hardware flow control.
>
> Signed-off-by: Krishna Yarlagadda <[email protected]>
> ---
> .../security/tpm/nvidia,tegra-tpm-spi.yaml | 34 +++++++++++++++++++
> 1 file changed, 34 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/security/tpm/nvidia,tegra-tpm-spi.yaml
>
> diff --git a/Documentation/devicetree/bindings/security/tpm/nvidia,tegra-tpm-spi.yaml b/Documentation/devicetree/bindings/security/tpm/nvidia,tegra-tpm-spi.yaml
> new file mode 100644
> index 000000000000..dcb78db7355c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/security/tpm/nvidia,tegra-tpm-spi.yaml
> @@ -0,0 +1,34 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/security/tpm/nvidia,tegra-tpm-spi.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Tegra QSPI TPM driver
Bindings are for h/w, not drivers.
> +
> +maintainers:
> + - Thierry Reding <[email protected]>
> + - Jonathan Hunter <[email protected]>
> +
> +properties:
> + compatible:
> + enum:
> + - nvidia,tegra-tpm-spi
> +
> + reg:
> + maxItems: 1
> +
> +required:
> + - compatible
> + - reg
> +
> +unevaluatedProperties: false
> +
> +examples:
> + - |
> + qspi1@3250000 {
spi@...
> + tpm@0 {
> + compatible = "nvidia,tegra-tpm-spi";
Tegra has a TPM chip/block? This doesn't seem right.
> + reg = <0>;
> + };
> + };
> --
> 2.17.1
>
On Fri, Feb 3, 2023 at 7:02 AM Krishna Yarlagadda
<[email protected]> wrote:
>
> Add "nvidia,wait-polling" flag to enable TCG TIS hardware flow control.
Tell me something that the diff doesn't.
>
> Signed-off-by: Krishna Yarlagadda <[email protected]>
> ---
> .../bindings/spi/nvidia,tegra210-quad-peripheral-props.yaml | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/devicetree/bindings/spi/nvidia,tegra210-quad-peripheral-props.yaml b/Documentation/devicetree/bindings/spi/nvidia,tegra210-quad-peripheral-props.yaml
> index 2c3cada75339..19d2b30cadbf 100644
> --- a/Documentation/devicetree/bindings/spi/nvidia,tegra210-quad-peripheral-props.yaml
> +++ b/Documentation/devicetree/bindings/spi/nvidia,tegra210-quad-peripheral-props.yaml
> @@ -29,4 +29,10 @@ properties:
> minimum: 0
> maximum: 255
>
> + nvidia,wait-polling:
> + description:
> + Enable TPM wait polling feature for QSPI as specified in TCG PC Client
> + Specific TPM Interface Specification (TIS).
> + $ref: /schemas/types.yaml#/definitions/flag
Why do you need this flag when you have a compatible that also
indicates you have a quirk.
If this a TPM feature, why is it enabled for every single SPI slave device?
If the fundamental issue is the controller only supports half-duplex,
why can't you just check that from the driver? Can't the SPI subsystem
tell you that the host controller is half-duplex? Though sometimes
that may be board level property I suppose. If so, define the h/w
quirk, not the driver mode in DT. Half-duplex is probably something
everyone could use, not just Nvidia.
Please discuss this series internally with the folks you marked as
maintainers. It has issues I'm sure they would have also pointed out.
Rob
Hi Krishna,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on char-misc/char-misc-next char-misc/char-misc-linus broonie-spi/for-next robh/for-next linus/master v6.2-rc6 next-20230203]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Krishna-Yarlagadda/dt-bindings-tpm-Add-compatible-for-Tegra-TPM/20230203-210314
patch link: https://lore.kernel.org/r/20230203130133.32901-3-kyarlagadda%40nvidia.com
patch subject: [Patch V2 2/4] tpm: tegra: Support SPI tpm wait state detect
config: x86_64-randconfig-k001 (https://download.01.org/0day-ci/archive/20230204/[email protected]/config)
compiler: clang version 14.0.6 (https://github.com/llvm/llvm-project f28c006a5895fc0e329fe15fead81e37457cb1d1)
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/825363f7e8d0d426c45bbba6cb3c5d9b79b7e6aa
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Krishna-Yarlagadda/dt-bindings-tpm-Add-compatible-for-Tegra-TPM/20230203-210314
git checkout 825363f7e8d0d426c45bbba6cb3c5d9b79b7e6aa
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=x86_64 SHELL=/bin/bash drivers/char/tpm/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
In file included from drivers/char/tpm/tpm_tis_spi_tegra.c:18:
In file included from drivers/char/tpm/tpm_tis_core.h:22:
In file included from drivers/char/tpm/tpm.h:28:
include/linux/tpm_eventlog.h:167:6: warning: variable 'mapping_size' set but not used [-Wunused-but-set-variable]
int mapping_size;
^
>> drivers/char/tpm/tpm_tis_spi_tegra.c:23:5: warning: no previous prototype for function 'tpm_tis_spi_tegra_transfer' [-Wmissing-prototypes]
int tpm_tis_spi_tegra_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
^
drivers/char/tpm/tpm_tis_spi_tegra.c:23:1: note: declare 'static' if the function is not intended to be used outside of this translation unit
int tpm_tis_spi_tegra_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
^
static
2 warnings generated.
vim +/tpm_tis_spi_tegra_transfer +23 drivers/char/tpm/tpm_tis_spi_tegra.c
22
> 23 int tpm_tis_spi_tegra_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
24 u8 *in, const u8 *out)
25 {
26 struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
27 int ret = 0;
28 struct spi_message m;
29 struct spi_transfer spi_xfer[3];
30 u8 transfer_len;
31
32 spi_bus_lock(phy->spi_device->master);
33
34 while (len) {
35 transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
36
37 spi_message_init(&m);
38 phy->iobuf[0] = (in ? 0x80 : 0) | (transfer_len - 1);
39 phy->iobuf[1] = 0xd4;
40 phy->iobuf[2] = addr >> 8;
41 phy->iobuf[3] = addr;
42
43 memset(&spi_xfer, 0, sizeof(spi_xfer));
44
45 spi_xfer[0].tx_buf = phy->iobuf;
46 spi_xfer[0].len = 1;
47 spi_message_add_tail(&spi_xfer[0], &m);
48
49 spi_xfer[1].tx_buf = phy->iobuf + 1;
50 spi_xfer[1].len = 3;
51 spi_message_add_tail(&spi_xfer[1], &m);
52
53 if (out) {
54 spi_xfer[2].tx_buf = &phy->iobuf[4];
55 spi_xfer[2].rx_buf = NULL;
56 memcpy(&phy->iobuf[4], out, transfer_len);
57 out += transfer_len;
58 }
59 if (in) {
60 spi_xfer[2].tx_buf = NULL;
61 spi_xfer[2].rx_buf = &phy->iobuf[4];
62 }
63 spi_xfer[2].len = transfer_len;
64 spi_message_add_tail(&spi_xfer[2], &m);
65
66 reinit_completion(&phy->ready);
67 ret = spi_sync_locked(phy->spi_device, &m);
68 if (ret < 0)
69 goto exit;
70
71 if (in) {
72 memcpy(in, &phy->iobuf[4], transfer_len);
73 in += transfer_len;
74 }
75
76 len -= transfer_len;
77 }
78
79 exit:
80 spi_bus_unlock(phy->spi_device->master);
81 return ret;
82 }
83
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Hi Krishna,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on char-misc/char-misc-testing]
[also build test WARNING on char-misc/char-misc-next char-misc/char-misc-linus broonie-spi/for-next robh/for-next linus/master v6.2-rc6 next-20230203]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Krishna-Yarlagadda/dt-bindings-tpm-Add-compatible-for-Tegra-TPM/20230203-210314
patch link: https://lore.kernel.org/r/20230203130133.32901-3-kyarlagadda%40nvidia.com
patch subject: [Patch V2 2/4] tpm: tegra: Support SPI tpm wait state detect
config: loongarch-buildonly-randconfig-r003-20230204 (https://download.01.org/0day-ci/archive/20230204/[email protected]/config)
compiler: loongarch64-linux-gcc (GCC) 12.1.0
reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# https://github.com/intel-lab-lkp/linux/commit/825363f7e8d0d426c45bbba6cb3c5d9b79b7e6aa
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Krishna-Yarlagadda/dt-bindings-tpm-Add-compatible-for-Tegra-TPM/20230203-210314
git checkout 825363f7e8d0d426c45bbba6cb3c5d9b79b7e6aa
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=loongarch olddefconfig
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross W=1 O=build_dir ARCH=loongarch SHELL=/bin/bash drivers/char/tpm/
If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <[email protected]>
All warnings (new ones prefixed by >>):
>> drivers/char/tpm/tpm_tis_spi_tegra.c:23:5: warning: no previous prototype for 'tpm_tis_spi_tegra_transfer' [-Wmissing-prototypes]
23 | int tpm_tis_spi_tegra_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
vim +/tpm_tis_spi_tegra_transfer +23 drivers/char/tpm/tpm_tis_spi_tegra.c
22
> 23 int tpm_tis_spi_tegra_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
24 u8 *in, const u8 *out)
25 {
26 struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
27 int ret = 0;
28 struct spi_message m;
29 struct spi_transfer spi_xfer[3];
30 u8 transfer_len;
31
32 spi_bus_lock(phy->spi_device->master);
33
34 while (len) {
35 transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
36
37 spi_message_init(&m);
38 phy->iobuf[0] = (in ? 0x80 : 0) | (transfer_len - 1);
39 phy->iobuf[1] = 0xd4;
40 phy->iobuf[2] = addr >> 8;
41 phy->iobuf[3] = addr;
42
43 memset(&spi_xfer, 0, sizeof(spi_xfer));
44
45 spi_xfer[0].tx_buf = phy->iobuf;
46 spi_xfer[0].len = 1;
47 spi_message_add_tail(&spi_xfer[0], &m);
48
49 spi_xfer[1].tx_buf = phy->iobuf + 1;
50 spi_xfer[1].len = 3;
51 spi_message_add_tail(&spi_xfer[1], &m);
52
53 if (out) {
54 spi_xfer[2].tx_buf = &phy->iobuf[4];
55 spi_xfer[2].rx_buf = NULL;
56 memcpy(&phy->iobuf[4], out, transfer_len);
57 out += transfer_len;
58 }
59 if (in) {
60 spi_xfer[2].tx_buf = NULL;
61 spi_xfer[2].rx_buf = &phy->iobuf[4];
62 }
63 spi_xfer[2].len = transfer_len;
64 spi_message_add_tail(&spi_xfer[2], &m);
65
66 reinit_completion(&phy->ready);
67 ret = spi_sync_locked(phy->spi_device, &m);
68 if (ret < 0)
69 goto exit;
70
71 if (in) {
72 memcpy(in, &phy->iobuf[4], transfer_len);
73 in += transfer_len;
74 }
75
76 len -= transfer_len;
77 }
78
79 exit:
80 spi_bus_unlock(phy->spi_device->master);
81 return ret;
82 }
83
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests
Dear Krishna,
Thank you for your patch.
Am 03.02.23 um 14:01 schrieb Krishna Yarlagadda:
> Tegra234 and Tegra241 chips have QSPI controller that supports TCG
> TIS hardware flow control. Since the controller only supports half
> duplex, sw wait polling method implemented in tpm_tis_spi does not
> suffice. Added extending driver to disable sw flow control and send
I’d use imperative mood and maybe use another verb:
Add dedicated Tegra driver …
> all transfers in single message.
Please add how you tested and benchmarked this patch.
> Signed-off-by: Krishna Yarlagadda <[email protected]>
> ---
> drivers/char/tpm/Makefile | 1 +
> drivers/char/tpm/tpm_tis_spi.h | 1 +
> drivers/char/tpm/tpm_tis_spi_main.c | 4 +-
> drivers/char/tpm/tpm_tis_spi_tegra.c | 123 +++++++++++++++++++++++++++
> 4 files changed, 128 insertions(+), 1 deletion(-)
> create mode 100644 drivers/char/tpm/tpm_tis_spi_tegra.c
>
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index 0222b1ddb310..445b15493cb3 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -25,6 +25,7 @@ obj-$(CONFIG_TCG_TIS_SYNQUACER) += tpm_tis_synquacer.o
>
> obj-$(CONFIG_TCG_TIS_SPI) += tpm_tis_spi.o
> tpm_tis_spi-y := tpm_tis_spi_main.o
> +tpm_tis_spi-y += tpm_tis_spi_tegra.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
> diff --git a/drivers/char/tpm/tpm_tis_spi.h b/drivers/char/tpm/tpm_tis_spi.h
> index d0f66f6f1931..feaea14b428b 100644
> --- a/drivers/char/tpm/tpm_tis_spi.h
> +++ b/drivers/char/tpm/tpm_tis_spi.h
> @@ -31,6 +31,7 @@ extern int tpm_tis_spi_init(struct spi_device *spi, struct tpm_tis_spi_phy *phy,
> extern int tpm_tis_spi_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
> u8 *in, const u8 *out);
>
> +extern int tegra_tpm_spi_probe(struct spi_device *spi);
> #ifdef CONFIG_TCG_TIS_SPI_CR50
> extern int cr50_spi_probe(struct spi_device *spi);
> #else
> diff --git a/drivers/char/tpm/tpm_tis_spi_main.c b/drivers/char/tpm/tpm_tis_spi_main.c
> index a0963a3e92bd..5d4502a4461a 100644
> --- a/drivers/char/tpm/tpm_tis_spi_main.c
> +++ b/drivers/char/tpm/tpm_tis_spi_main.c
> @@ -198,7 +198,7 @@ static int tpm_tis_spi_driver_probe(struct spi_device *spi)
> const struct spi_device_id *spi_dev_id = spi_get_device_id(spi);
> tpm_tis_spi_probe_func probe_func;
>
> - probe_func = of_device_get_match_data(&spi->dev);
> + probe_func = device_get_match_data(&spi->dev);
> if (!probe_func) {
> if (spi_dev_id) {
> probe_func = (tpm_tis_spi_probe_func)spi_dev_id->driver_data;
> @@ -227,6 +227,7 @@ static const struct spi_device_id tpm_tis_spi_id[] = {
> { "tpm_tis_spi", (unsigned long)tpm_tis_spi_probe },
> { "tpm_tis-spi", (unsigned long)tpm_tis_spi_probe },
> { "cr50", (unsigned long)cr50_spi_probe },
> + { "tegra-tpm-spi", (unsigned long)tegra_tpm_spi_probe },
> {}
> };
> MODULE_DEVICE_TABLE(spi, tpm_tis_spi_id);
> @@ -236,6 +237,7 @@ static const struct of_device_id of_tis_spi_match[] = {
> { .compatible = "infineon,slb9670", .data = tpm_tis_spi_probe },
> { .compatible = "tcg,tpm_tis-spi", .data = tpm_tis_spi_probe },
> { .compatible = "google,cr50", .data = cr50_spi_probe },
> + { .compatible = "nvidia,tegra-tpm-spi", .data = tegra_tpm_spi_probe },
> {}
> };
> MODULE_DEVICE_TABLE(of, of_tis_spi_match);
> diff --git a/drivers/char/tpm/tpm_tis_spi_tegra.c b/drivers/char/tpm/tpm_tis_spi_tegra.c
> new file mode 100644
> index 000000000000..23f20684513d
> --- /dev/null
> +++ b/drivers/char/tpm/tpm_tis_spi_tegra.c
> @@ -0,0 +1,123 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2023 NVIDIA CORPORATION.
> + *
> + * This device driver implements TEGRA QSPI hw wait detection for chips
> + *
> + * It is based on tpm_tis_spi driver by Peter Huewe and Christophe Ricard.
> + */
> +
> +#include <linux/completion.h>
> +#include <linux/interrupt.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/pm.h>
> +#include <linux/spi/spi.h>
> +#include <linux/wait.h>
> +
> +#include "tpm_tis_core.h"
> +#include "tpm_tis_spi.h"
> +
> +#define MAX_SPI_FRAMESIZE 64
> +
> +int tpm_tis_spi_tegra_transfer(struct tpm_tis_data *data, u32 addr, u16 len,
> + u8 *in, const u8 *out)
> +{
> + struct tpm_tis_spi_phy *phy = to_tpm_tis_spi_phy(data);
> + int ret = 0;
> + struct spi_message m;
> + struct spi_transfer spi_xfer[3];
> + u8 transfer_len;
Just use `unsigned int`? [1]
> +
> + spi_bus_lock(phy->spi_device->master);
> +
> + while (len) {
> + transfer_len = min_t(u16, len, MAX_SPI_FRAMESIZE);
> +
> + spi_message_init(&m);
> + phy->iobuf[0] = (in ? 0x80 : 0) | (transfer_len - 1);
> + phy->iobuf[1] = 0xd4;
> + phy->iobuf[2] = addr >> 8;
> + phy->iobuf[3] = addr;
> +
> + memset(&spi_xfer, 0, sizeof(spi_xfer));
> +
> + spi_xfer[0].tx_buf = phy->iobuf;
> + spi_xfer[0].len = 1;
> + spi_message_add_tail(&spi_xfer[0], &m);
> +
> + spi_xfer[1].tx_buf = phy->iobuf + 1;
> + spi_xfer[1].len = 3;
> + spi_message_add_tail(&spi_xfer[1], &m);
> +
> + if (out) {
> + spi_xfer[2].tx_buf = &phy->iobuf[4];
> + spi_xfer[2].rx_buf = NULL;
> + memcpy(&phy->iobuf[4], out, transfer_len);
> + out += transfer_len;
> + }
> + if (in) {
> + spi_xfer[2].tx_buf = NULL;
> + spi_xfer[2].rx_buf = &phy->iobuf[4];
> + }
> + spi_xfer[2].len = transfer_len;
> + spi_message_add_tail(&spi_xfer[2], &m);
> +
> + reinit_completion(&phy->ready);
> + ret = spi_sync_locked(phy->spi_device, &m);
> + if (ret < 0)
> + goto exit;
> +
> + if (in) {
> + memcpy(in, &phy->iobuf[4], transfer_len);
> + in += transfer_len;
> + }
> +
> + len -= transfer_len;
> + }
> +
> +exit:
> + spi_bus_unlock(phy->spi_device->master);
> + return ret;
> +}
> +
> +static int tpm_tis_spi_tegra_read_bytes(struct tpm_tis_data *data, u32 addr,
> + u16 len, u8 *result,
> + enum tpm_tis_io_mode io_mode)
> +{
> + return tpm_tis_spi_tegra_transfer(data, addr, len, result, NULL);
> +}
> +
> +static int tpm_tis_spi_tegra_write_bytes(struct tpm_tis_data *data, u32 addr,
> + u16 len, const u8 *value,
> + enum tpm_tis_io_mode io_mode)
> +{
> + return tpm_tis_spi_tegra_transfer(data, addr, len, NULL, value);
> +}
> +
> +static const struct tpm_tis_phy_ops tegra_tpm_spi_phy_ops = {
> + .read_bytes = tpm_tis_spi_tegra_read_bytes,
> + .write_bytes = tpm_tis_spi_tegra_write_bytes,
> +};
> +
> +int tegra_tpm_spi_probe(struct spi_device *dev)
> +{
> + struct tpm_tis_spi_phy *phy;
> + int irq;
> +
> + phy = devm_kzalloc(&dev->dev, sizeof(struct tpm_tis_spi_phy),
> + GFP_KERNEL);
> + if (!phy)
> + return -ENOMEM;
> +
> + phy->flow_control = NULL;
> +
> + /* If the SPI device has an IRQ then use that */
> + if (dev->irq > 0)
> + irq = dev->irq;
> + else
> + irq = -1;
Use ternary operator?
irq = dev->irq > 0 ? dev->irq : -1;
> +
> + init_completion(&phy->ready);
> + return tpm_tis_spi_init(dev, phy, irq, &tegra_tpm_spi_phy_ops);
> +}
Kind regards,
Paul
[1]: https://notabs.org/coding/smallIntsBigPenalty.htm
On Mon, Feb 06, 2023 at 12:02:56PM +0100, Paul Menzel wrote:
> Am 03.02.23 um 14:01 schrieb Krishna Yarlagadda:
> > + /* If the SPI device has an IRQ then use that */
> > + if (dev->irq > 0)
> > + irq = dev->irq;
> > + else
> > + irq = -1;
> Use ternary operator?
> irq = dev->irq > 0 ? dev->irq : -1;
No, please write the code using normal conditional instructions. This
isn't the IOCCC and the ternery operator is rarely a legibility aid.
On Mon, Feb 06, 2023 at 01:19:04PM +0000, Mark Brown wrote:
> On Mon, Feb 06, 2023 at 12:02:56PM +0100, Paul Menzel wrote:
> > Am 03.02.23 um 14:01 schrieb Krishna Yarlagadda:
>
> > > + /* If the SPI device has an IRQ then use that */
> > > + if (dev->irq > 0)
> > > + irq = dev->irq;
> > > + else
> > > + irq = -1;
>
> > Use ternary operator?
>
> > irq = dev->irq > 0 ? dev->irq : -1;
>
> No, please write the code using normal conditional instructions. This
> isn't the IOCCC and the ternery operator is rarely a legibility aid.
Looks like the SPI core sets dev->irq = 0 for any error other than
-EPROBE_DEFER and the TPM TIS core checks for irq != 0 before trying to
setup that IRQ, so seems like we can just skip this altogether and pass
in dev->irq directly.
Thierry
> -----Original Message-----
> From: Rob Herring <[email protected]>
> Sent: 04 February 2023 01:20
> To: Krishna Yarlagadda <[email protected]>
> Cc: [email protected]; [email protected]; [email protected];
> [email protected]; [email protected]; linux-
> [email protected]; [email protected]; linux-
> [email protected]; [email protected];
> [email protected]; Jonathan Hunter <[email protected]>;
> Sowjanya Komatineni <[email protected]>; Laxman Dewangan
> <[email protected]>
> Subject: Re: [Patch V2 3/4] spi: dt-bindings: Add Tegra TPM wait polling flag
>
> External email: Use caution opening links or attachments
>
>
> On Fri, Feb 3, 2023 at 7:02 AM Krishna Yarlagadda
> <[email protected]> wrote:
> >
> > Add "nvidia,wait-polling" flag to enable TCG TIS hardware flow control.
>
> Tell me something that the diff doesn't.
>
> >
> > Signed-off-by: Krishna Yarlagadda <[email protected]>
> > ---
> > .../bindings/spi/nvidia,tegra210-quad-peripheral-props.yaml | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/spi/nvidia,tegra210-quad-
> peripheral-props.yaml
> b/Documentation/devicetree/bindings/spi/nvidia,tegra210-quad-peripheral-
> props.yaml
> > index 2c3cada75339..19d2b30cadbf 100644
> > --- a/Documentation/devicetree/bindings/spi/nvidia,tegra210-quad-
> peripheral-props.yaml
> > +++ b/Documentation/devicetree/bindings/spi/nvidia,tegra210-quad-
> peripheral-props.yaml
> > @@ -29,4 +29,10 @@ properties:
> > minimum: 0
> > maximum: 255
> >
> > + nvidia,wait-polling:
> > + description:
> > + Enable TPM wait polling feature for QSPI as specified in TCG PC Client
> > + Specific TPM Interface Specification (TIS).
> > + $ref: /schemas/types.yaml#/definitions/flag
>
> Why do you need this flag when you have a compatible that also
> indicates you have a quirk.
>
> If this a TPM feature, why is it enabled for every single SPI slave device?
>
> If the fundamental issue is the controller only supports half-duplex,
> why can't you just check that from the driver? Can't the SPI subsystem
> tell you that the host controller is half-duplex? Though sometimes
> that may be board level property I suppose. If so, define the h/w
> quirk, not the driver mode in DT. Half-duplex is probably something
> everyone could use, not just Nvidia.
>
> Please discuss this series internally with the folks you marked as
> maintainers. It has issues I'm sure they would have also pointed out.
>
> Rob
QSPI is a multi-chip-select controller and HW wait polling is only for TPM
Both controller and device would need a flag/setting to identify support
for this feature. Using SPI controller flags and SPI device mode flags to
avoid device tree flags. Moved HW/alternate TPM flow control into existing
driver. No need of new compatible now.
KY