dd Loongson Security Engine chip driver

[PATCH v11 3/4] tpm: Add a driver for Loongson TPM device

Posted by Qunqin Zhao 3 months, 3 weeks ago

Loongson Security Engine supports random number generation, hash,
symmetric encryption and asymmetric encryption. Based on these
encryption functions, TPM2 have been implemented in the Loongson
Security Engine firmware. This driver is responsible for copying data
into the memory visible to the firmware and receiving data from the
firmware.

Co-developed-by: Yinggang Gu <guyinggang@loongson.cn>
Signed-off-by: Yinggang Gu <guyinggang@loongson.cn>
Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn>
Reviewed-by: Huacai Chen <chenhuacai@loongson.cn>
---
 drivers/char/tpm/Kconfig        |  9 ++++
 drivers/char/tpm/Makefile       |  1 +
 drivers/char/tpm/tpm_loongson.c | 84 +++++++++++++++++++++++++++++++++
 3 files changed, 94 insertions(+)
 create mode 100644 drivers/char/tpm/tpm_loongson.c

diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
index dddd702b2..ba3924eb1 100644
--- a/drivers/char/tpm/Kconfig
+++ b/drivers/char/tpm/Kconfig
@@ -189,6 +189,15 @@ config TCG_IBMVTPM
 	  will be accessible from within Linux.  To compile this driver
 	  as a module, choose M here; the module will be called tpm_ibmvtpm.
 
+config TCG_LOONGSON
+	tristate "Loongson TPM Interface"
+	depends on MFD_LOONGSON_SE
+	help
+	  If you want to make Loongson TPM support available, say Yes and
+	  it will be accessible from within Linux. To compile this
+	  driver as a module, choose M here; the module will be called
+	  tpm_loongson.
+
 config TCG_XEN
 	tristate "XEN TPM Interface"
 	depends on TCG_TPM && XEN
diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
index 9de1b3ea3..5b5cdc0d3 100644
--- a/drivers/char/tpm/Makefile
+++ b/drivers/char/tpm/Makefile
@@ -46,3 +46,4 @@ obj-$(CONFIG_TCG_ARM_CRB_FFA) += tpm_crb_ffa.o
 obj-$(CONFIG_TCG_VTPM_PROXY) += tpm_vtpm_proxy.o
 obj-$(CONFIG_TCG_FTPM_TEE) += tpm_ftpm_tee.o
 obj-$(CONFIG_TCG_SVSM) += tpm_svsm.o
+obj-$(CONFIG_TCG_LOONGSON) += tpm_loongson.o
diff --git a/drivers/char/tpm/tpm_loongson.c b/drivers/char/tpm/tpm_loongson.c
new file mode 100644
index 000000000..5cbdb37f8
--- /dev/null
+++ b/drivers/char/tpm/tpm_loongson.c
@@ -0,0 +1,84 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Copyright (c) 2025 Loongson Technology Corporation Limited. */
+
+#include <linux/device.h>
+#include <linux/mfd/loongson-se.h>
+#include <linux/platform_device.h>
+#include <linux/wait.h>
+
+#include "tpm.h"
+
+struct tpm_loongson_cmd {
+	u32 cmd_id;
+	u32 data_off;
+	u32 data_len;
+	u32 pad[5];
+};
+
+static int tpm_loongson_recv(struct tpm_chip *chip, u8 *buf, size_t count)
+{
+	struct loongson_se_engine *tpm_engine = dev_get_drvdata(&chip->dev);
+	struct tpm_loongson_cmd *cmd_ret = tpm_engine->command_ret;
+
+	if (cmd_ret->data_len > count)
+		return -EIO;
+
+	memcpy(buf, tpm_engine->data_buffer, cmd_ret->data_len);
+
+	return cmd_ret->data_len;
+}
+
+static int tpm_loongson_send(struct tpm_chip *chip, u8 *buf, size_t count)
+{
+	struct loongson_se_engine *tpm_engine = dev_get_drvdata(&chip->dev);
+	struct tpm_loongson_cmd *cmd = tpm_engine->command;
+
+	if (count > tpm_engine->buffer_size)
+		return -E2BIG;
+
+	cmd->data_len = count;
+	memcpy(tpm_engine->data_buffer, buf, count);
+
+	return loongson_se_send_engine_cmd(tpm_engine);
+}
+
+static const struct tpm_class_ops tpm_loongson_ops = {
+	.flags = TPM_OPS_AUTO_STARTUP,
+	.recv = tpm_loongson_recv,
+	.send = tpm_loongson_send,
+};
+
+static int tpm_loongson_probe(struct platform_device *pdev)
+{
+	struct loongson_se_engine *tpm_engine;
+	struct device *dev = &pdev->dev;
+	struct tpm_loongson_cmd *cmd;
+	struct tpm_chip *chip;
+
+	tpm_engine = loongson_se_init_engine(dev->parent, SE_ENGINE_TPM);
+	if (!tpm_engine)
+		return -ENODEV;
+	cmd = tpm_engine->command;
+	cmd->cmd_id = SE_CMD_TPM;
+	cmd->data_off = tpm_engine->buffer_off;
+
+	chip = tpmm_chip_alloc(dev, &tpm_loongson_ops);
+	if (IS_ERR(chip))
+		return PTR_ERR(chip);
+	chip->flags = TPM_CHIP_FLAG_TPM2 | TPM_CHIP_FLAG_IRQ;
+	dev_set_drvdata(&chip->dev, tpm_engine);
+
+	return tpm_chip_register(chip);
+}
+
+static struct platform_driver tpm_loongson = {
+	.probe   = tpm_loongson_probe,
+	.driver  = {
+		.name  = "loongson-tpm",
+	},
+};
+module_platform_driver(tpm_loongson);
+
+MODULE_ALIAS("platform:loongson-tpm");
+MODULE_LICENSE("GPL");
+MODULE_DESCRIPTION("Loongson TPM driver");
-- 
2.45.2

Re: [PATCH v11 3/4] tpm: Add a driver for Loongson TPM device

Posted by Jarkko Sakkinen 3 months, 2 weeks ago

On Thu, Jun 19, 2025 at 10:51:37AM +0800, Qunqin Zhao wrote:
> Loongson Security Engine supports random number generation, hash,
> symmetric encryption and asymmetric encryption. Based on these
> encryption functions, TPM2 have been implemented in the Loongson
> Security Engine firmware. This driver is responsible for copying data
> into the memory visible to the firmware and receiving data from the
> firmware.
> 
> Co-developed-by: Yinggang Gu <guyinggang@loongson.cn>
> Signed-off-by: Yinggang Gu <guyinggang@loongson.cn>
> Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn>
> Reviewed-by: Huacai Chen <chenhuacai@loongson.cn>
> ---
>  drivers/char/tpm/Kconfig        |  9 ++++
>  drivers/char/tpm/Makefile       |  1 +
>  drivers/char/tpm/tpm_loongson.c | 84 +++++++++++++++++++++++++++++++++
>  3 files changed, 94 insertions(+)
>  create mode 100644 drivers/char/tpm/tpm_loongson.c
> 
> diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> index dddd702b2..ba3924eb1 100644
> --- a/drivers/char/tpm/Kconfig
> +++ b/drivers/char/tpm/Kconfig
> @@ -189,6 +189,15 @@ config TCG_IBMVTPM
>  	  will be accessible from within Linux.  To compile this driver
>  	  as a module, choose M here; the module will be called tpm_ibmvtpm.
>  
> +config TCG_LOONGSON
> +	tristate "Loongson TPM Interface"
> +	depends on MFD_LOONGSON_SE
> +	help
> +	  If you want to make Loongson TPM support available, say Yes and
> +	  it will be accessible from within Linux. To compile this
> +	  driver as a module, choose M here; the module will be called
> +	  tpm_loongson.
> +
>  config TCG_XEN
>  	tristate "XEN TPM Interface"
>  	depends on TCG_TPM && XEN
> diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> index 9de1b3ea3..5b5cdc0d3 100644
> --- a/drivers/char/tpm/Makefile
> +++ b/drivers/char/tpm/Makefile
> @@ -46,3 +46,4 @@ obj-$(CONFIG_TCG_ARM_CRB_FFA) += tpm_crb_ffa.o
>  obj-$(CONFIG_TCG_VTPM_PROXY) += tpm_vtpm_proxy.o
>  obj-$(CONFIG_TCG_FTPM_TEE) += tpm_ftpm_tee.o
>  obj-$(CONFIG_TCG_SVSM) += tpm_svsm.o
> +obj-$(CONFIG_TCG_LOONGSON) += tpm_loongson.o
> diff --git a/drivers/char/tpm/tpm_loongson.c b/drivers/char/tpm/tpm_loongson.c
> new file mode 100644
> index 000000000..5cbdb37f8
> --- /dev/null
> +++ b/drivers/char/tpm/tpm_loongson.c
> @@ -0,0 +1,84 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2025 Loongson Technology Corporation Limited. */
> +
> +#include <linux/device.h>
> +#include <linux/mfd/loongson-se.h>
> +#include <linux/platform_device.h>
> +#include <linux/wait.h>
> +
> +#include "tpm.h"
> +
> +struct tpm_loongson_cmd {
> +	u32 cmd_id;
> +	u32 data_off;
> +	u32 data_len;
> +	u32 pad[5];
> +};
> +
> +static int tpm_loongson_recv(struct tpm_chip *chip, u8 *buf, size_t count)
> +{
> +	struct loongson_se_engine *tpm_engine = dev_get_drvdata(&chip->dev);
> +	struct tpm_loongson_cmd *cmd_ret = tpm_engine->command_ret;
> +
> +	if (cmd_ret->data_len > count)
> +		return -EIO;
> +
> +	memcpy(buf, tpm_engine->data_buffer, cmd_ret->data_len);
> +
> +	return cmd_ret->data_len;
> +}
> +
> +static int tpm_loongson_send(struct tpm_chip *chip, u8 *buf, size_t count)
> +{
> +	struct loongson_se_engine *tpm_engine = dev_get_drvdata(&chip->dev);
> +	struct tpm_loongson_cmd *cmd = tpm_engine->command;
> +
> +	if (count > tpm_engine->buffer_size)
> +		return -E2BIG;
> +
> +	cmd->data_len = count;
> +	memcpy(tpm_engine->data_buffer, buf, count);
> +
> +	return loongson_se_send_engine_cmd(tpm_engine);
> +}
> +
> +static const struct tpm_class_ops tpm_loongson_ops = {
> +	.flags = TPM_OPS_AUTO_STARTUP,
> +	.recv = tpm_loongson_recv,
> +	.send = tpm_loongson_send,
> +};
> +
> +static int tpm_loongson_probe(struct platform_device *pdev)
> +{
> +	struct loongson_se_engine *tpm_engine;
> +	struct device *dev = &pdev->dev;
> +	struct tpm_loongson_cmd *cmd;
> +	struct tpm_chip *chip;
> +
> +	tpm_engine = loongson_se_init_engine(dev->parent, SE_ENGINE_TPM);
> +	if (!tpm_engine)
> +		return -ENODEV;
> +	cmd = tpm_engine->command;
> +	cmd->cmd_id = SE_CMD_TPM;
> +	cmd->data_off = tpm_engine->buffer_off;
> +
> +	chip = tpmm_chip_alloc(dev, &tpm_loongson_ops);
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +	chip->flags = TPM_CHIP_FLAG_TPM2 | TPM_CHIP_FLAG_IRQ;
> +	dev_set_drvdata(&chip->dev, tpm_engine);
> +
> +	return tpm_chip_register(chip);
> +}
> +
> +static struct platform_driver tpm_loongson = {
> +	.probe   = tpm_loongson_probe,
> +	.driver  = {
> +		.name  = "loongson-tpm",

This patch looks otherwise great but I'd prefer here tho use
"tpm_loongson_probe" for the value of the name field.

> +	},
> +};
> +module_platform_driver(tpm_loongson);
> +
> +MODULE_ALIAS("platform:loongson-tpm");
> +MODULE_LICENSE("GPL");
> +MODULE_DESCRIPTION("Loongson TPM driver");
> -- 
> 2.45.2
> 

BR, Jarkko

Re: [PATCH v11 3/4] tpm: Add a driver for Loongson TPM device

Posted by Lee Jones 3 months, 2 weeks ago

On Wed, 25 Jun 2025, Jarkko Sakkinen wrote:

> On Thu, Jun 19, 2025 at 10:51:37AM +0800, Qunqin Zhao wrote:
> > Loongson Security Engine supports random number generation, hash,
> > symmetric encryption and asymmetric encryption. Based on these
> > encryption functions, TPM2 have been implemented in the Loongson
> > Security Engine firmware. This driver is responsible for copying data
> > into the memory visible to the firmware and receiving data from the
> > firmware.
> > 
> > Co-developed-by: Yinggang Gu <guyinggang@loongson.cn>
> > Signed-off-by: Yinggang Gu <guyinggang@loongson.cn>
> > Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn>
> > Reviewed-by: Huacai Chen <chenhuacai@loongson.cn>
> > ---
> >  drivers/char/tpm/Kconfig        |  9 ++++
> >  drivers/char/tpm/Makefile       |  1 +
> >  drivers/char/tpm/tpm_loongson.c | 84 +++++++++++++++++++++++++++++++++
> >  3 files changed, 94 insertions(+)
> >  create mode 100644 drivers/char/tpm/tpm_loongson.c
> > 
> > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > index dddd702b2..ba3924eb1 100644
> > --- a/drivers/char/tpm/Kconfig
> > +++ b/drivers/char/tpm/Kconfig
> > @@ -189,6 +189,15 @@ config TCG_IBMVTPM
> >  	  will be accessible from within Linux.  To compile this driver
> >  	  as a module, choose M here; the module will be called tpm_ibmvtpm.
> >  
> > +config TCG_LOONGSON
> > +	tristate "Loongson TPM Interface"
> > +	depends on MFD_LOONGSON_SE
> > +	help
> > +	  If you want to make Loongson TPM support available, say Yes and
> > +	  it will be accessible from within Linux. To compile this
> > +	  driver as a module, choose M here; the module will be called
> > +	  tpm_loongson.
> > +
> >  config TCG_XEN
> >  	tristate "XEN TPM Interface"
> >  	depends on TCG_TPM && XEN
> > diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> > index 9de1b3ea3..5b5cdc0d3 100644
> > --- a/drivers/char/tpm/Makefile
> > +++ b/drivers/char/tpm/Makefile
> > @@ -46,3 +46,4 @@ obj-$(CONFIG_TCG_ARM_CRB_FFA) += tpm_crb_ffa.o
> >  obj-$(CONFIG_TCG_VTPM_PROXY) += tpm_vtpm_proxy.o
> >  obj-$(CONFIG_TCG_FTPM_TEE) += tpm_ftpm_tee.o
> >  obj-$(CONFIG_TCG_SVSM) += tpm_svsm.o
> > +obj-$(CONFIG_TCG_LOONGSON) += tpm_loongson.o
> > diff --git a/drivers/char/tpm/tpm_loongson.c b/drivers/char/tpm/tpm_loongson.c
> > new file mode 100644
> > index 000000000..5cbdb37f8
> > --- /dev/null
> > +++ b/drivers/char/tpm/tpm_loongson.c
> > @@ -0,0 +1,84 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Copyright (c) 2025 Loongson Technology Corporation Limited. */
> > +
> > +#include <linux/device.h>
> > +#include <linux/mfd/loongson-se.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/wait.h>
> > +
> > +#include "tpm.h"
> > +
> > +struct tpm_loongson_cmd {
> > +	u32 cmd_id;
> > +	u32 data_off;
> > +	u32 data_len;
> > +	u32 pad[5];
> > +};
> > +
> > +static int tpm_loongson_recv(struct tpm_chip *chip, u8 *buf, size_t count)
> > +{
> > +	struct loongson_se_engine *tpm_engine = dev_get_drvdata(&chip->dev);
> > +	struct tpm_loongson_cmd *cmd_ret = tpm_engine->command_ret;
> > +
> > +	if (cmd_ret->data_len > count)
> > +		return -EIO;
> > +
> > +	memcpy(buf, tpm_engine->data_buffer, cmd_ret->data_len);
> > +
> > +	return cmd_ret->data_len;
> > +}
> > +
> > +static int tpm_loongson_send(struct tpm_chip *chip, u8 *buf, size_t count)
> > +{
> > +	struct loongson_se_engine *tpm_engine = dev_get_drvdata(&chip->dev);
> > +	struct tpm_loongson_cmd *cmd = tpm_engine->command;
> > +
> > +	if (count > tpm_engine->buffer_size)
> > +		return -E2BIG;
> > +
> > +	cmd->data_len = count;
> > +	memcpy(tpm_engine->data_buffer, buf, count);
> > +
> > +	return loongson_se_send_engine_cmd(tpm_engine);
> > +}
> > +
> > +static const struct tpm_class_ops tpm_loongson_ops = {
> > +	.flags = TPM_OPS_AUTO_STARTUP,
> > +	.recv = tpm_loongson_recv,
> > +	.send = tpm_loongson_send,
> > +};
> > +
> > +static int tpm_loongson_probe(struct platform_device *pdev)
> > +{
> > +	struct loongson_se_engine *tpm_engine;
> > +	struct device *dev = &pdev->dev;
> > +	struct tpm_loongson_cmd *cmd;
> > +	struct tpm_chip *chip;
> > +
> > +	tpm_engine = loongson_se_init_engine(dev->parent, SE_ENGINE_TPM);
> > +	if (!tpm_engine)
> > +		return -ENODEV;
> > +	cmd = tpm_engine->command;
> > +	cmd->cmd_id = SE_CMD_TPM;
> > +	cmd->data_off = tpm_engine->buffer_off;
> > +
> > +	chip = tpmm_chip_alloc(dev, &tpm_loongson_ops);
> > +	if (IS_ERR(chip))
> > +		return PTR_ERR(chip);
> > +	chip->flags = TPM_CHIP_FLAG_TPM2 | TPM_CHIP_FLAG_IRQ;
> > +	dev_set_drvdata(&chip->dev, tpm_engine);
> > +
> > +	return tpm_chip_register(chip);
> > +}
> > +
> > +static struct platform_driver tpm_loongson = {
> > +	.probe   = tpm_loongson_probe,
> > +	.driver  = {
> > +		.name  = "loongson-tpm",
> 
> This patch looks otherwise great but I'd prefer here tho use
> "tpm_loongson_probe" for the value of the name field.

Where does this stipulation come from?  No other driver does this [0].
driver.name should be a nicely formatted, human readable string
describing the name of the device.  Not a function name.

[0] git grep -A15 "static struct platform_driver" | grep ".name = .*probe"

-- 
Lee Jones [李琼斯]

Re: [PATCH v11 3/4] tpm: Add a driver for Loongson TPM device

Posted by Jarkko Sakkinen 3 months, 2 weeks ago

On Wed, Jun 25, 2025 at 09:05:27AM +0100, Lee Jones wrote:
> On Wed, 25 Jun 2025, Jarkko Sakkinen wrote:
> 
> > On Thu, Jun 19, 2025 at 10:51:37AM +0800, Qunqin Zhao wrote:
> > > Loongson Security Engine supports random number generation, hash,
> > > symmetric encryption and asymmetric encryption. Based on these
> > > encryption functions, TPM2 have been implemented in the Loongson
> > > Security Engine firmware. This driver is responsible for copying data
> > > into the memory visible to the firmware and receiving data from the
> > > firmware.
> > > 
> > > Co-developed-by: Yinggang Gu <guyinggang@loongson.cn>
> > > Signed-off-by: Yinggang Gu <guyinggang@loongson.cn>
> > > Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn>
> > > Reviewed-by: Huacai Chen <chenhuacai@loongson.cn>
> > > ---
> > >  drivers/char/tpm/Kconfig        |  9 ++++
> > >  drivers/char/tpm/Makefile       |  1 +
> > >  drivers/char/tpm/tpm_loongson.c | 84 +++++++++++++++++++++++++++++++++
> > >  3 files changed, 94 insertions(+)
> > >  create mode 100644 drivers/char/tpm/tpm_loongson.c
> > > 
> > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > > index dddd702b2..ba3924eb1 100644
> > > --- a/drivers/char/tpm/Kconfig
> > > +++ b/drivers/char/tpm/Kconfig
> > > @@ -189,6 +189,15 @@ config TCG_IBMVTPM
> > >  	  will be accessible from within Linux.  To compile this driver
> > >  	  as a module, choose M here; the module will be called tpm_ibmvtpm.
> > >  
> > > +config TCG_LOONGSON
> > > +	tristate "Loongson TPM Interface"
> > > +	depends on MFD_LOONGSON_SE
> > > +	help
> > > +	  If you want to make Loongson TPM support available, say Yes and
> > > +	  it will be accessible from within Linux. To compile this
> > > +	  driver as a module, choose M here; the module will be called
> > > +	  tpm_loongson.
> > > +
> > >  config TCG_XEN
> > >  	tristate "XEN TPM Interface"
> > >  	depends on TCG_TPM && XEN
> > > diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> > > index 9de1b3ea3..5b5cdc0d3 100644
> > > --- a/drivers/char/tpm/Makefile
> > > +++ b/drivers/char/tpm/Makefile
> > > @@ -46,3 +46,4 @@ obj-$(CONFIG_TCG_ARM_CRB_FFA) += tpm_crb_ffa.o
> > >  obj-$(CONFIG_TCG_VTPM_PROXY) += tpm_vtpm_proxy.o
> > >  obj-$(CONFIG_TCG_FTPM_TEE) += tpm_ftpm_tee.o
> > >  obj-$(CONFIG_TCG_SVSM) += tpm_svsm.o
> > > +obj-$(CONFIG_TCG_LOONGSON) += tpm_loongson.o
> > > diff --git a/drivers/char/tpm/tpm_loongson.c b/drivers/char/tpm/tpm_loongson.c
> > > new file mode 100644
> > > index 000000000..5cbdb37f8
> > > --- /dev/null
> > > +++ b/drivers/char/tpm/tpm_loongson.c
> > > @@ -0,0 +1,84 @@
> > > +// SPDX-License-Identifier: GPL-2.0
> > > +/* Copyright (c) 2025 Loongson Technology Corporation Limited. */
> > > +
> > > +#include <linux/device.h>
> > > +#include <linux/mfd/loongson-se.h>
> > > +#include <linux/platform_device.h>
> > > +#include <linux/wait.h>
> > > +
> > > +#include "tpm.h"
> > > +
> > > +struct tpm_loongson_cmd {
> > > +	u32 cmd_id;
> > > +	u32 data_off;
> > > +	u32 data_len;
> > > +	u32 pad[5];
> > > +};
> > > +
> > > +static int tpm_loongson_recv(struct tpm_chip *chip, u8 *buf, size_t count)
> > > +{
> > > +	struct loongson_se_engine *tpm_engine = dev_get_drvdata(&chip->dev);
> > > +	struct tpm_loongson_cmd *cmd_ret = tpm_engine->command_ret;
> > > +
> > > +	if (cmd_ret->data_len > count)
> > > +		return -EIO;
> > > +
> > > +	memcpy(buf, tpm_engine->data_buffer, cmd_ret->data_len);
> > > +
> > > +	return cmd_ret->data_len;
> > > +}
> > > +
> > > +static int tpm_loongson_send(struct tpm_chip *chip, u8 *buf, size_t count)
> > > +{
> > > +	struct loongson_se_engine *tpm_engine = dev_get_drvdata(&chip->dev);
> > > +	struct tpm_loongson_cmd *cmd = tpm_engine->command;
> > > +
> > > +	if (count > tpm_engine->buffer_size)
> > > +		return -E2BIG;
> > > +
> > > +	cmd->data_len = count;
> > > +	memcpy(tpm_engine->data_buffer, buf, count);
> > > +
> > > +	return loongson_se_send_engine_cmd(tpm_engine);
> > > +}
> > > +
> > > +static const struct tpm_class_ops tpm_loongson_ops = {
> > > +	.flags = TPM_OPS_AUTO_STARTUP,
> > > +	.recv = tpm_loongson_recv,
> > > +	.send = tpm_loongson_send,
> > > +};
> > > +
> > > +static int tpm_loongson_probe(struct platform_device *pdev)
> > > +{
> > > +	struct loongson_se_engine *tpm_engine;
> > > +	struct device *dev = &pdev->dev;
> > > +	struct tpm_loongson_cmd *cmd;
> > > +	struct tpm_chip *chip;
> > > +
> > > +	tpm_engine = loongson_se_init_engine(dev->parent, SE_ENGINE_TPM);
> > > +	if (!tpm_engine)
> > > +		return -ENODEV;
> > > +	cmd = tpm_engine->command;
> > > +	cmd->cmd_id = SE_CMD_TPM;
> > > +	cmd->data_off = tpm_engine->buffer_off;
> > > +
> > > +	chip = tpmm_chip_alloc(dev, &tpm_loongson_ops);
> > > +	if (IS_ERR(chip))
> > > +		return PTR_ERR(chip);
> > > +	chip->flags = TPM_CHIP_FLAG_TPM2 | TPM_CHIP_FLAG_IRQ;
> > > +	dev_set_drvdata(&chip->dev, tpm_engine);
> > > +
> > > +	return tpm_chip_register(chip);
> > > +}
> > > +
> > > +static struct platform_driver tpm_loongson = {
> > > +	.probe   = tpm_loongson_probe,
> > > +	.driver  = {
> > > +		.name  = "loongson-tpm",
> > 
> > This patch looks otherwise great but I'd prefer here tho use
> > "tpm_loongson_probe" for the value of the name field.
> 
> Where does this stipulation come from?  No other driver does this [0].
> driver.name should be a nicely formatted, human readable string
> describing the name of the device.  Not a function name.

What defines "human-readable" here? I see both as somewhat the
same level of "readability" ;-)

> 
> [0] git grep -A15 "static struct platform_driver" | grep ".name = .*probe"

What I'm getting:

$ git grep -l -e platform_driver_register --or -e module_platform_driver
drivers/char/tpm | xargs git grep "\.name"
drivers/char/tpm/tpm_atmel.c:           .name = "tpm_atmel",
drivers/char/tpm/tpm_ftpm_tee.c:                .name = "ftpm-tee",
drivers/char/tpm/tpm_ftpm_tee.c:                .name           =
"optee-ftpm",
drivers/char/tpm/tpm_nsc.c:             .name    = "tpm_nsc",
drivers/char/tpm/tpm_svsm.c:            .name = "tpm-svsm",
drivers/char/tpm/tpm_tis.c:     .name = "tpm_tis",
drivers/char/tpm/tpm_tis.c:             .name           = "tpm_tis",
drivers/char/tpm/tpm_tis_synquacer.c:           .name           =
"tpm_tis_synquacer",

Do you consider e.g, "tpm_tis" as "less human-readable".

I don't necessarily fight against the name chosen. Your arguments
just plain no make sense, so I just merely want to understand this.
That's all.

> 
> -- 
> Lee Jones [李琼斯]

BR, Jarkko

Re: [PATCH v11 3/4] tpm: Add a driver for Loongson TPM device

Posted by Lee Jones 3 months, 2 weeks ago

On Wed, 25 Jun 2025, Jarkko Sakkinen wrote:

> On Wed, Jun 25, 2025 at 09:05:27AM +0100, Lee Jones wrote:
> > On Wed, 25 Jun 2025, Jarkko Sakkinen wrote:
> > 
> > > On Thu, Jun 19, 2025 at 10:51:37AM +0800, Qunqin Zhao wrote:
> > > > Loongson Security Engine supports random number generation, hash,
> > > > symmetric encryption and asymmetric encryption. Based on these
> > > > encryption functions, TPM2 have been implemented in the Loongson
> > > > Security Engine firmware. This driver is responsible for copying data
> > > > into the memory visible to the firmware and receiving data from the
> > > > firmware.
> > > > 
> > > > Co-developed-by: Yinggang Gu <guyinggang@loongson.cn>
> > > > Signed-off-by: Yinggang Gu <guyinggang@loongson.cn>
> > > > Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn>
> > > > Reviewed-by: Huacai Chen <chenhuacai@loongson.cn>
> > > > ---
> > > >  drivers/char/tpm/Kconfig        |  9 ++++
> > > >  drivers/char/tpm/Makefile       |  1 +
> > > >  drivers/char/tpm/tpm_loongson.c | 84 +++++++++++++++++++++++++++++++++
> > > >  3 files changed, 94 insertions(+)
> > > >  create mode 100644 drivers/char/tpm/tpm_loongson.c
> > > > 
> > > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > > > index dddd702b2..ba3924eb1 100644
> > > > --- a/drivers/char/tpm/Kconfig
> > > > +++ b/drivers/char/tpm/Kconfig
> > > > @@ -189,6 +189,15 @@ config TCG_IBMVTPM
> > > >  	  will be accessible from within Linux.  To compile this driver
> > > >  	  as a module, choose M here; the module will be called tpm_ibmvtpm.
> > > >  
> > > > +config TCG_LOONGSON
> > > > +	tristate "Loongson TPM Interface"
> > > > +	depends on MFD_LOONGSON_SE
> > > > +	help
> > > > +	  If you want to make Loongson TPM support available, say Yes and
> > > > +	  it will be accessible from within Linux. To compile this
> > > > +	  driver as a module, choose M here; the module will be called
> > > > +	  tpm_loongson.
> > > > +
> > > >  config TCG_XEN
> > > >  	tristate "XEN TPM Interface"
> > > >  	depends on TCG_TPM && XEN
> > > > diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> > > > index 9de1b3ea3..5b5cdc0d3 100644
> > > > --- a/drivers/char/tpm/Makefile
> > > > +++ b/drivers/char/tpm/Makefile
> > > > @@ -46,3 +46,4 @@ obj-$(CONFIG_TCG_ARM_CRB_FFA) += tpm_crb_ffa.o
> > > >  obj-$(CONFIG_TCG_VTPM_PROXY) += tpm_vtpm_proxy.o
> > > >  obj-$(CONFIG_TCG_FTPM_TEE) += tpm_ftpm_tee.o
> > > >  obj-$(CONFIG_TCG_SVSM) += tpm_svsm.o
> > > > +obj-$(CONFIG_TCG_LOONGSON) += tpm_loongson.o
> > > > diff --git a/drivers/char/tpm/tpm_loongson.c b/drivers/char/tpm/tpm_loongson.c
> > > > new file mode 100644
> > > > index 000000000..5cbdb37f8
> > > > --- /dev/null
> > > > +++ b/drivers/char/tpm/tpm_loongson.c
> > > > @@ -0,0 +1,84 @@
> > > > +// SPDX-License-Identifier: GPL-2.0
> > > > +/* Copyright (c) 2025 Loongson Technology Corporation Limited. */
> > > > +
> > > > +#include <linux/device.h>
> > > > +#include <linux/mfd/loongson-se.h>
> > > > +#include <linux/platform_device.h>
> > > > +#include <linux/wait.h>
> > > > +
> > > > +#include "tpm.h"
> > > > +
> > > > +struct tpm_loongson_cmd {
> > > > +	u32 cmd_id;
> > > > +	u32 data_off;
> > > > +	u32 data_len;
> > > > +	u32 pad[5];
> > > > +};
> > > > +
> > > > +static int tpm_loongson_recv(struct tpm_chip *chip, u8 *buf, size_t count)
> > > > +{
> > > > +	struct loongson_se_engine *tpm_engine = dev_get_drvdata(&chip->dev);
> > > > +	struct tpm_loongson_cmd *cmd_ret = tpm_engine->command_ret;
> > > > +
> > > > +	if (cmd_ret->data_len > count)
> > > > +		return -EIO;
> > > > +
> > > > +	memcpy(buf, tpm_engine->data_buffer, cmd_ret->data_len);
> > > > +
> > > > +	return cmd_ret->data_len;
> > > > +}
> > > > +
> > > > +static int tpm_loongson_send(struct tpm_chip *chip, u8 *buf, size_t count)
> > > > +{
> > > > +	struct loongson_se_engine *tpm_engine = dev_get_drvdata(&chip->dev);
> > > > +	struct tpm_loongson_cmd *cmd = tpm_engine->command;
> > > > +
> > > > +	if (count > tpm_engine->buffer_size)
> > > > +		return -E2BIG;
> > > > +
> > > > +	cmd->data_len = count;
> > > > +	memcpy(tpm_engine->data_buffer, buf, count);
> > > > +
> > > > +	return loongson_se_send_engine_cmd(tpm_engine);
> > > > +}
> > > > +
> > > > +static const struct tpm_class_ops tpm_loongson_ops = {
> > > > +	.flags = TPM_OPS_AUTO_STARTUP,
> > > > +	.recv = tpm_loongson_recv,
> > > > +	.send = tpm_loongson_send,
> > > > +};
> > > > +
> > > > +static int tpm_loongson_probe(struct platform_device *pdev)
> > > > +{
> > > > +	struct loongson_se_engine *tpm_engine;
> > > > +	struct device *dev = &pdev->dev;
> > > > +	struct tpm_loongson_cmd *cmd;
> > > > +	struct tpm_chip *chip;
> > > > +
> > > > +	tpm_engine = loongson_se_init_engine(dev->parent, SE_ENGINE_TPM);
> > > > +	if (!tpm_engine)
> > > > +		return -ENODEV;
> > > > +	cmd = tpm_engine->command;
> > > > +	cmd->cmd_id = SE_CMD_TPM;
> > > > +	cmd->data_off = tpm_engine->buffer_off;
> > > > +
> > > > +	chip = tpmm_chip_alloc(dev, &tpm_loongson_ops);
> > > > +	if (IS_ERR(chip))
> > > > +		return PTR_ERR(chip);
> > > > +	chip->flags = TPM_CHIP_FLAG_TPM2 | TPM_CHIP_FLAG_IRQ;
> > > > +	dev_set_drvdata(&chip->dev, tpm_engine);
> > > > +
> > > > +	return tpm_chip_register(chip);
> > > > +}
> > > > +
> > > > +static struct platform_driver tpm_loongson = {
> > > > +	.probe   = tpm_loongson_probe,
> > > > +	.driver  = {
> > > > +		.name  = "loongson-tpm",
> > > 
> > > This patch looks otherwise great but I'd prefer here tho use
> > > "tpm_loongson_probe" for the value of the name field.
> > 
> > Where does this stipulation come from?  No other driver does this [0].
> > driver.name should be a nicely formatted, human readable string
> > describing the name of the device.  Not a function name.
> 
> What defines "human-readable" here? I see both as somewhat the
> same level of "readability" ;-)
> 
> > 
> > [0] git grep -A15 "static struct platform_driver" | grep ".name = .*probe"
> 
> What I'm getting:
> 
> $ git grep -l -e platform_driver_register --or -e module_platform_driver
> drivers/char/tpm | xargs git grep "\.name"
> drivers/char/tpm/tpm_atmel.c:           .name = "tpm_atmel",
> drivers/char/tpm/tpm_ftpm_tee.c:                .name = "ftpm-tee",
> drivers/char/tpm/tpm_ftpm_tee.c:                .name           =
> "optee-ftpm",
> drivers/char/tpm/tpm_nsc.c:             .name    = "tpm_nsc",
> drivers/char/tpm/tpm_svsm.c:            .name = "tpm-svsm",
> drivers/char/tpm/tpm_tis.c:     .name = "tpm_tis",
> drivers/char/tpm/tpm_tis.c:             .name           = "tpm_tis",
> drivers/char/tpm/tpm_tis_synquacer.c:           .name           =
> "tpm_tis_synquacer",
> 
> Do you consider e.g, "tpm_tis" as "less human-readable".
> 
> I don't necessarily fight against the name chosen. Your arguments
> just plain no make sense, so I just merely want to understand this.
> That's all.

In 64% of cases '-' is preferred to '_' for device names.

Human readable is probably a bit of a stretch in this context, so I'll
retract that part of the statement.  However, we should be using device
names, not names of functions which remain meaningless (which is what I
really meant by 'readable') to the user.  Where else do you see the
.probe() function name being used as a device name?

-- 
Lee Jones [李琼斯]

Re: [PATCH v11 3/4] tpm: Add a driver for Loongson TPM device

Posted by Jarkko Sakkinen 3 months, 2 weeks ago

On Wed, Jun 25, 2025 at 02:40:47PM +0100, Lee Jones wrote:
> On Wed, 25 Jun 2025, Jarkko Sakkinen wrote:
> 
> > On Wed, Jun 25, 2025 at 09:05:27AM +0100, Lee Jones wrote:
> > > On Wed, 25 Jun 2025, Jarkko Sakkinen wrote:
> > > 
> > > > On Thu, Jun 19, 2025 at 10:51:37AM +0800, Qunqin Zhao wrote:
> > > > > Loongson Security Engine supports random number generation, hash,
> > > > > symmetric encryption and asymmetric encryption. Based on these
> > > > > encryption functions, TPM2 have been implemented in the Loongson
> > > > > Security Engine firmware. This driver is responsible for copying data
> > > > > into the memory visible to the firmware and receiving data from the
> > > > > firmware.
> > > > > 
> > > > > Co-developed-by: Yinggang Gu <guyinggang@loongson.cn>
> > > > > Signed-off-by: Yinggang Gu <guyinggang@loongson.cn>
> > > > > Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn>
> > > > > Reviewed-by: Huacai Chen <chenhuacai@loongson.cn>
> > > > > ---
> > > > >  drivers/char/tpm/Kconfig        |  9 ++++
> > > > >  drivers/char/tpm/Makefile       |  1 +
> > > > >  drivers/char/tpm/tpm_loongson.c | 84 +++++++++++++++++++++++++++++++++
> > > > >  3 files changed, 94 insertions(+)
> > > > >  create mode 100644 drivers/char/tpm/tpm_loongson.c
> > > > > 
> > > > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > > > > index dddd702b2..ba3924eb1 100644
> > > > > --- a/drivers/char/tpm/Kconfig
> > > > > +++ b/drivers/char/tpm/Kconfig
> > > > > @@ -189,6 +189,15 @@ config TCG_IBMVTPM
> > > > >  	  will be accessible from within Linux.  To compile this driver
> > > > >  	  as a module, choose M here; the module will be called tpm_ibmvtpm.
> > > > >  
> > > > > +config TCG_LOONGSON
> > > > > +	tristate "Loongson TPM Interface"
> > > > > +	depends on MFD_LOONGSON_SE
> > > > > +	help
> > > > > +	  If you want to make Loongson TPM support available, say Yes and
> > > > > +	  it will be accessible from within Linux. To compile this
> > > > > +	  driver as a module, choose M here; the module will be called
> > > > > +	  tpm_loongson.
> > > > > +
> > > > >  config TCG_XEN
> > > > >  	tristate "XEN TPM Interface"
> > > > >  	depends on TCG_TPM && XEN
> > > > > diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> > > > > index 9de1b3ea3..5b5cdc0d3 100644
> > > > > --- a/drivers/char/tpm/Makefile
> > > > > +++ b/drivers/char/tpm/Makefile
> > > > > @@ -46,3 +46,4 @@ obj-$(CONFIG_TCG_ARM_CRB_FFA) += tpm_crb_ffa.o
> > > > >  obj-$(CONFIG_TCG_VTPM_PROXY) += tpm_vtpm_proxy.o
> > > > >  obj-$(CONFIG_TCG_FTPM_TEE) += tpm_ftpm_tee.o
> > > > >  obj-$(CONFIG_TCG_SVSM) += tpm_svsm.o
> > > > > +obj-$(CONFIG_TCG_LOONGSON) += tpm_loongson.o
> > > > > diff --git a/drivers/char/tpm/tpm_loongson.c b/drivers/char/tpm/tpm_loongson.c
> > > > > new file mode 100644
> > > > > index 000000000..5cbdb37f8
> > > > > --- /dev/null
> > > > > +++ b/drivers/char/tpm/tpm_loongson.c
> > > > > @@ -0,0 +1,84 @@
> > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > +/* Copyright (c) 2025 Loongson Technology Corporation Limited. */
> > > > > +
> > > > > +#include <linux/device.h>
> > > > > +#include <linux/mfd/loongson-se.h>
> > > > > +#include <linux/platform_device.h>
> > > > > +#include <linux/wait.h>
> > > > > +
> > > > > +#include "tpm.h"
> > > > > +
> > > > > +struct tpm_loongson_cmd {
> > > > > +	u32 cmd_id;
> > > > > +	u32 data_off;
> > > > > +	u32 data_len;
> > > > > +	u32 pad[5];
> > > > > +};
> > > > > +
> > > > > +static int tpm_loongson_recv(struct tpm_chip *chip, u8 *buf, size_t count)
> > > > > +{
> > > > > +	struct loongson_se_engine *tpm_engine = dev_get_drvdata(&chip->dev);
> > > > > +	struct tpm_loongson_cmd *cmd_ret = tpm_engine->command_ret;
> > > > > +
> > > > > +	if (cmd_ret->data_len > count)
> > > > > +		return -EIO;
> > > > > +
> > > > > +	memcpy(buf, tpm_engine->data_buffer, cmd_ret->data_len);
> > > > > +
> > > > > +	return cmd_ret->data_len;
> > > > > +}
> > > > > +
> > > > > +static int tpm_loongson_send(struct tpm_chip *chip, u8 *buf, size_t count)
> > > > > +{
> > > > > +	struct loongson_se_engine *tpm_engine = dev_get_drvdata(&chip->dev);
> > > > > +	struct tpm_loongson_cmd *cmd = tpm_engine->command;
> > > > > +
> > > > > +	if (count > tpm_engine->buffer_size)
> > > > > +		return -E2BIG;
> > > > > +
> > > > > +	cmd->data_len = count;
> > > > > +	memcpy(tpm_engine->data_buffer, buf, count);
> > > > > +
> > > > > +	return loongson_se_send_engine_cmd(tpm_engine);
> > > > > +}
> > > > > +
> > > > > +static const struct tpm_class_ops tpm_loongson_ops = {
> > > > > +	.flags = TPM_OPS_AUTO_STARTUP,
> > > > > +	.recv = tpm_loongson_recv,
> > > > > +	.send = tpm_loongson_send,
> > > > > +};
> > > > > +
> > > > > +static int tpm_loongson_probe(struct platform_device *pdev)
> > > > > +{
> > > > > +	struct loongson_se_engine *tpm_engine;
> > > > > +	struct device *dev = &pdev->dev;
> > > > > +	struct tpm_loongson_cmd *cmd;
> > > > > +	struct tpm_chip *chip;
> > > > > +
> > > > > +	tpm_engine = loongson_se_init_engine(dev->parent, SE_ENGINE_TPM);
> > > > > +	if (!tpm_engine)
> > > > > +		return -ENODEV;
> > > > > +	cmd = tpm_engine->command;
> > > > > +	cmd->cmd_id = SE_CMD_TPM;
> > > > > +	cmd->data_off = tpm_engine->buffer_off;
> > > > > +
> > > > > +	chip = tpmm_chip_alloc(dev, &tpm_loongson_ops);
> > > > > +	if (IS_ERR(chip))
> > > > > +		return PTR_ERR(chip);
> > > > > +	chip->flags = TPM_CHIP_FLAG_TPM2 | TPM_CHIP_FLAG_IRQ;
> > > > > +	dev_set_drvdata(&chip->dev, tpm_engine);
> > > > > +
> > > > > +	return tpm_chip_register(chip);
> > > > > +}
> > > > > +
> > > > > +static struct platform_driver tpm_loongson = {
> > > > > +	.probe   = tpm_loongson_probe,
> > > > > +	.driver  = {
> > > > > +		.name  = "loongson-tpm",
> > > > 
> > > > This patch looks otherwise great but I'd prefer here tho use
> > > > "tpm_loongson_probe" for the value of the name field.
> > > 
> > > Where does this stipulation come from?  No other driver does this [0].
> > > driver.name should be a nicely formatted, human readable string
> > > describing the name of the device.  Not a function name.
> > 
> > What defines "human-readable" here? I see both as somewhat the
> > same level of "readability" ;-)
> > 
> > > 
> > > [0] git grep -A15 "static struct platform_driver" | grep ".name = .*probe"
> > 
> > What I'm getting:
> > 
> > $ git grep -l -e platform_driver_register --or -e module_platform_driver
> > drivers/char/tpm | xargs git grep "\.name"
> > drivers/char/tpm/tpm_atmel.c:           .name = "tpm_atmel",
> > drivers/char/tpm/tpm_ftpm_tee.c:                .name = "ftpm-tee",
> > drivers/char/tpm/tpm_ftpm_tee.c:                .name           =
> > "optee-ftpm",
> > drivers/char/tpm/tpm_nsc.c:             .name    = "tpm_nsc",
> > drivers/char/tpm/tpm_svsm.c:            .name = "tpm-svsm",
> > drivers/char/tpm/tpm_tis.c:     .name = "tpm_tis",
> > drivers/char/tpm/tpm_tis.c:             .name           = "tpm_tis",
> > drivers/char/tpm/tpm_tis_synquacer.c:           .name           =
> > "tpm_tis_synquacer",
> > 
> > Do you consider e.g, "tpm_tis" as "less human-readable".
> > 
> > I don't necessarily fight against the name chosen. Your arguments
> > just plain no make sense, so I just merely want to understand this.
> > That's all.
> 
> In 64% of cases '-' is preferred to '_' for device names.
> 
> Human readable is probably a bit of a stretch in this context, so I'll
> retract that part of the statement.  However, we should be using device
> names, not names of functions which remain meaningless (which is what I
> really meant by 'readable') to the user.  Where else do you see the
> .probe() function name being used as a device name?

Oops now I see what you mean. I meant to write "tpm_loongson", i.e.
matching tpm_tis, tpm_crb etc. Sorry my bad.

> 
> -- 
> Lee Jones [李琼斯]

BR, Jarkko

Re: [PATCH v11 3/4] tpm: Add a driver for Loongson TPM device

Posted by Lee Jones 3 months, 2 weeks ago

On Wed, 25 Jun 2025, Jarkko Sakkinen wrote:

> On Wed, Jun 25, 2025 at 02:40:47PM +0100, Lee Jones wrote:
> > On Wed, 25 Jun 2025, Jarkko Sakkinen wrote:
> > 
> > > On Wed, Jun 25, 2025 at 09:05:27AM +0100, Lee Jones wrote:
> > > > On Wed, 25 Jun 2025, Jarkko Sakkinen wrote:
> > > > 
> > > > > On Thu, Jun 19, 2025 at 10:51:37AM +0800, Qunqin Zhao wrote:
> > > > > > Loongson Security Engine supports random number generation, hash,
> > > > > > symmetric encryption and asymmetric encryption. Based on these
> > > > > > encryption functions, TPM2 have been implemented in the Loongson
> > > > > > Security Engine firmware. This driver is responsible for copying data
> > > > > > into the memory visible to the firmware and receiving data from the
> > > > > > firmware.
> > > > > > 
> > > > > > Co-developed-by: Yinggang Gu <guyinggang@loongson.cn>
> > > > > > Signed-off-by: Yinggang Gu <guyinggang@loongson.cn>
> > > > > > Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn>
> > > > > > Reviewed-by: Huacai Chen <chenhuacai@loongson.cn>
> > > > > > ---
> > > > > >  drivers/char/tpm/Kconfig        |  9 ++++
> > > > > >  drivers/char/tpm/Makefile       |  1 +
> > > > > >  drivers/char/tpm/tpm_loongson.c | 84 +++++++++++++++++++++++++++++++++
> > > > > >  3 files changed, 94 insertions(+)
> > > > > >  create mode 100644 drivers/char/tpm/tpm_loongson.c
> > > > > > 
> > > > > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > > > > > index dddd702b2..ba3924eb1 100644
> > > > > > --- a/drivers/char/tpm/Kconfig
> > > > > > +++ b/drivers/char/tpm/Kconfig
> > > > > > @@ -189,6 +189,15 @@ config TCG_IBMVTPM
> > > > > >  	  will be accessible from within Linux.  To compile this driver
> > > > > >  	  as a module, choose M here; the module will be called tpm_ibmvtpm.
> > > > > >  
> > > > > > +config TCG_LOONGSON
> > > > > > +	tristate "Loongson TPM Interface"
> > > > > > +	depends on MFD_LOONGSON_SE
> > > > > > +	help
> > > > > > +	  If you want to make Loongson TPM support available, say Yes and
> > > > > > +	  it will be accessible from within Linux. To compile this
> > > > > > +	  driver as a module, choose M here; the module will be called
> > > > > > +	  tpm_loongson.
> > > > > > +
> > > > > >  config TCG_XEN
> > > > > >  	tristate "XEN TPM Interface"
> > > > > >  	depends on TCG_TPM && XEN
> > > > > > diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> > > > > > index 9de1b3ea3..5b5cdc0d3 100644
> > > > > > --- a/drivers/char/tpm/Makefile
> > > > > > +++ b/drivers/char/tpm/Makefile
> > > > > > @@ -46,3 +46,4 @@ obj-$(CONFIG_TCG_ARM_CRB_FFA) += tpm_crb_ffa.o
> > > > > >  obj-$(CONFIG_TCG_VTPM_PROXY) += tpm_vtpm_proxy.o
> > > > > >  obj-$(CONFIG_TCG_FTPM_TEE) += tpm_ftpm_tee.o
> > > > > >  obj-$(CONFIG_TCG_SVSM) += tpm_svsm.o
> > > > > > +obj-$(CONFIG_TCG_LOONGSON) += tpm_loongson.o
> > > > > > diff --git a/drivers/char/tpm/tpm_loongson.c b/drivers/char/tpm/tpm_loongson.c
> > > > > > new file mode 100644
> > > > > > index 000000000..5cbdb37f8
> > > > > > --- /dev/null
> > > > > > +++ b/drivers/char/tpm/tpm_loongson.c
> > > > > > @@ -0,0 +1,84 @@
> > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > +/* Copyright (c) 2025 Loongson Technology Corporation Limited. */
> > > > > > +
> > > > > > +#include <linux/device.h>
> > > > > > +#include <linux/mfd/loongson-se.h>
> > > > > > +#include <linux/platform_device.h>
> > > > > > +#include <linux/wait.h>
> > > > > > +
> > > > > > +#include "tpm.h"
> > > > > > +
> > > > > > +struct tpm_loongson_cmd {
> > > > > > +	u32 cmd_id;
> > > > > > +	u32 data_off;
> > > > > > +	u32 data_len;
> > > > > > +	u32 pad[5];
> > > > > > +};
> > > > > > +
> > > > > > +static int tpm_loongson_recv(struct tpm_chip *chip, u8 *buf, size_t count)
> > > > > > +{
> > > > > > +	struct loongson_se_engine *tpm_engine = dev_get_drvdata(&chip->dev);
> > > > > > +	struct tpm_loongson_cmd *cmd_ret = tpm_engine->command_ret;
> > > > > > +
> > > > > > +	if (cmd_ret->data_len > count)
> > > > > > +		return -EIO;
> > > > > > +
> > > > > > +	memcpy(buf, tpm_engine->data_buffer, cmd_ret->data_len);
> > > > > > +
> > > > > > +	return cmd_ret->data_len;
> > > > > > +}
> > > > > > +
> > > > > > +static int tpm_loongson_send(struct tpm_chip *chip, u8 *buf, size_t count)
> > > > > > +{
> > > > > > +	struct loongson_se_engine *tpm_engine = dev_get_drvdata(&chip->dev);
> > > > > > +	struct tpm_loongson_cmd *cmd = tpm_engine->command;
> > > > > > +
> > > > > > +	if (count > tpm_engine->buffer_size)
> > > > > > +		return -E2BIG;
> > > > > > +
> > > > > > +	cmd->data_len = count;
> > > > > > +	memcpy(tpm_engine->data_buffer, buf, count);
> > > > > > +
> > > > > > +	return loongson_se_send_engine_cmd(tpm_engine);
> > > > > > +}
> > > > > > +
> > > > > > +static const struct tpm_class_ops tpm_loongson_ops = {
> > > > > > +	.flags = TPM_OPS_AUTO_STARTUP,
> > > > > > +	.recv = tpm_loongson_recv,
> > > > > > +	.send = tpm_loongson_send,
> > > > > > +};
> > > > > > +
> > > > > > +static int tpm_loongson_probe(struct platform_device *pdev)
> > > > > > +{
> > > > > > +	struct loongson_se_engine *tpm_engine;
> > > > > > +	struct device *dev = &pdev->dev;
> > > > > > +	struct tpm_loongson_cmd *cmd;
> > > > > > +	struct tpm_chip *chip;
> > > > > > +
> > > > > > +	tpm_engine = loongson_se_init_engine(dev->parent, SE_ENGINE_TPM);
> > > > > > +	if (!tpm_engine)
> > > > > > +		return -ENODEV;
> > > > > > +	cmd = tpm_engine->command;
> > > > > > +	cmd->cmd_id = SE_CMD_TPM;
> > > > > > +	cmd->data_off = tpm_engine->buffer_off;
> > > > > > +
> > > > > > +	chip = tpmm_chip_alloc(dev, &tpm_loongson_ops);
> > > > > > +	if (IS_ERR(chip))
> > > > > > +		return PTR_ERR(chip);
> > > > > > +	chip->flags = TPM_CHIP_FLAG_TPM2 | TPM_CHIP_FLAG_IRQ;
> > > > > > +	dev_set_drvdata(&chip->dev, tpm_engine);
> > > > > > +
> > > > > > +	return tpm_chip_register(chip);
> > > > > > +}
> > > > > > +
> > > > > > +static struct platform_driver tpm_loongson = {
> > > > > > +	.probe   = tpm_loongson_probe,
> > > > > > +	.driver  = {
> > > > > > +		.name  = "loongson-tpm",
> > > > > 
> > > > > This patch looks otherwise great but I'd prefer here tho use
> > > > > "tpm_loongson_probe" for the value of the name field.
> > > > 
> > > > Where does this stipulation come from?  No other driver does this [0].
> > > > driver.name should be a nicely formatted, human readable string
> > > > describing the name of the device.  Not a function name.
> > > 
> > > What defines "human-readable" here? I see both as somewhat the
> > > same level of "readability" ;-)
> > > 
> > > > 
> > > > [0] git grep -A15 "static struct platform_driver" | grep ".name = .*probe"
> > > 
> > > What I'm getting:
> > > 
> > > $ git grep -l -e platform_driver_register --or -e module_platform_driver
> > > drivers/char/tpm | xargs git grep "\.name"
> > > drivers/char/tpm/tpm_atmel.c:           .name = "tpm_atmel",
> > > drivers/char/tpm/tpm_ftpm_tee.c:                .name = "ftpm-tee",
> > > drivers/char/tpm/tpm_ftpm_tee.c:                .name           =
> > > "optee-ftpm",
> > > drivers/char/tpm/tpm_nsc.c:             .name    = "tpm_nsc",
> > > drivers/char/tpm/tpm_svsm.c:            .name = "tpm-svsm",
> > > drivers/char/tpm/tpm_tis.c:     .name = "tpm_tis",
> > > drivers/char/tpm/tpm_tis.c:             .name           = "tpm_tis",
> > > drivers/char/tpm/tpm_tis_synquacer.c:           .name           =
> > > "tpm_tis_synquacer",
> > > 
> > > Do you consider e.g, "tpm_tis" as "less human-readable".
> > > 
> > > I don't necessarily fight against the name chosen. Your arguments
> > > just plain no make sense, so I just merely want to understand this.
> > > That's all.
> > 
> > In 64% of cases '-' is preferred to '_' for device names.
> > 
> > Human readable is probably a bit of a stretch in this context, so I'll
> > retract that part of the statement.  However, we should be using device
> > names, not names of functions which remain meaningless (which is what I
> > really meant by 'readable') to the user.  Where else do you see the
> > .probe() function name being used as a device name?
> 
> Oops now I see what you mean. I meant to write "tpm_loongson", i.e.
> matching tpm_tis, tpm_crb etc. Sorry my bad.

Ah, gotcha.  No worries.

"tpm_loongson" wouldn't be my preference, but is acceptable.

-- 
Lee Jones [李琼斯]

Re: [PATCH v11 3/4] tpm: Add a driver for Loongson TPM device

Posted by Jarkko Sakkinen 3 months, 2 weeks ago

On Thu, Jun 26, 2025 at 11:30:30AM +0100, Lee Jones wrote:
> On Wed, 25 Jun 2025, Jarkko Sakkinen wrote:
> 
> > On Wed, Jun 25, 2025 at 02:40:47PM +0100, Lee Jones wrote:
> > > On Wed, 25 Jun 2025, Jarkko Sakkinen wrote:
> > > 
> > > > On Wed, Jun 25, 2025 at 09:05:27AM +0100, Lee Jones wrote:
> > > > > On Wed, 25 Jun 2025, Jarkko Sakkinen wrote:
> > > > > 
> > > > > > On Thu, Jun 19, 2025 at 10:51:37AM +0800, Qunqin Zhao wrote:
> > > > > > > Loongson Security Engine supports random number generation, hash,
> > > > > > > symmetric encryption and asymmetric encryption. Based on these
> > > > > > > encryption functions, TPM2 have been implemented in the Loongson
> > > > > > > Security Engine firmware. This driver is responsible for copying data
> > > > > > > into the memory visible to the firmware and receiving data from the
> > > > > > > firmware.
> > > > > > > 
> > > > > > > Co-developed-by: Yinggang Gu <guyinggang@loongson.cn>
> > > > > > > Signed-off-by: Yinggang Gu <guyinggang@loongson.cn>
> > > > > > > Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn>
> > > > > > > Reviewed-by: Huacai Chen <chenhuacai@loongson.cn>
> > > > > > > ---
> > > > > > >  drivers/char/tpm/Kconfig        |  9 ++++
> > > > > > >  drivers/char/tpm/Makefile       |  1 +
> > > > > > >  drivers/char/tpm/tpm_loongson.c | 84 +++++++++++++++++++++++++++++++++
> > > > > > >  3 files changed, 94 insertions(+)
> > > > > > >  create mode 100644 drivers/char/tpm/tpm_loongson.c
> > > > > > > 
> > > > > > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > > > > > > index dddd702b2..ba3924eb1 100644
> > > > > > > --- a/drivers/char/tpm/Kconfig
> > > > > > > +++ b/drivers/char/tpm/Kconfig
> > > > > > > @@ -189,6 +189,15 @@ config TCG_IBMVTPM
> > > > > > >  	  will be accessible from within Linux.  To compile this driver
> > > > > > >  	  as a module, choose M here; the module will be called tpm_ibmvtpm.
> > > > > > >  
> > > > > > > +config TCG_LOONGSON
> > > > > > > +	tristate "Loongson TPM Interface"
> > > > > > > +	depends on MFD_LOONGSON_SE
> > > > > > > +	help
> > > > > > > +	  If you want to make Loongson TPM support available, say Yes and
> > > > > > > +	  it will be accessible from within Linux. To compile this
> > > > > > > +	  driver as a module, choose M here; the module will be called
> > > > > > > +	  tpm_loongson.
> > > > > > > +
> > > > > > >  config TCG_XEN
> > > > > > >  	tristate "XEN TPM Interface"
> > > > > > >  	depends on TCG_TPM && XEN
> > > > > > > diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> > > > > > > index 9de1b3ea3..5b5cdc0d3 100644
> > > > > > > --- a/drivers/char/tpm/Makefile
> > > > > > > +++ b/drivers/char/tpm/Makefile
> > > > > > > @@ -46,3 +46,4 @@ obj-$(CONFIG_TCG_ARM_CRB_FFA) += tpm_crb_ffa.o
> > > > > > >  obj-$(CONFIG_TCG_VTPM_PROXY) += tpm_vtpm_proxy.o
> > > > > > >  obj-$(CONFIG_TCG_FTPM_TEE) += tpm_ftpm_tee.o
> > > > > > >  obj-$(CONFIG_TCG_SVSM) += tpm_svsm.o
> > > > > > > +obj-$(CONFIG_TCG_LOONGSON) += tpm_loongson.o
> > > > > > > diff --git a/drivers/char/tpm/tpm_loongson.c b/drivers/char/tpm/tpm_loongson.c
> > > > > > > new file mode 100644
> > > > > > > index 000000000..5cbdb37f8
> > > > > > > --- /dev/null
> > > > > > > +++ b/drivers/char/tpm/tpm_loongson.c
> > > > > > > @@ -0,0 +1,84 @@
> > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > +/* Copyright (c) 2025 Loongson Technology Corporation Limited. */
> > > > > > > +
> > > > > > > +#include <linux/device.h>
> > > > > > > +#include <linux/mfd/loongson-se.h>
> > > > > > > +#include <linux/platform_device.h>
> > > > > > > +#include <linux/wait.h>
> > > > > > > +
> > > > > > > +#include "tpm.h"
> > > > > > > +
> > > > > > > +struct tpm_loongson_cmd {
> > > > > > > +	u32 cmd_id;
> > > > > > > +	u32 data_off;
> > > > > > > +	u32 data_len;
> > > > > > > +	u32 pad[5];
> > > > > > > +};
> > > > > > > +
> > > > > > > +static int tpm_loongson_recv(struct tpm_chip *chip, u8 *buf, size_t count)
> > > > > > > +{
> > > > > > > +	struct loongson_se_engine *tpm_engine = dev_get_drvdata(&chip->dev);
> > > > > > > +	struct tpm_loongson_cmd *cmd_ret = tpm_engine->command_ret;
> > > > > > > +
> > > > > > > +	if (cmd_ret->data_len > count)
> > > > > > > +		return -EIO;
> > > > > > > +
> > > > > > > +	memcpy(buf, tpm_engine->data_buffer, cmd_ret->data_len);
> > > > > > > +
> > > > > > > +	return cmd_ret->data_len;
> > > > > > > +}
> > > > > > > +
> > > > > > > +static int tpm_loongson_send(struct tpm_chip *chip, u8 *buf, size_t count)
> > > > > > > +{
> > > > > > > +	struct loongson_se_engine *tpm_engine = dev_get_drvdata(&chip->dev);
> > > > > > > +	struct tpm_loongson_cmd *cmd = tpm_engine->command;
> > > > > > > +
> > > > > > > +	if (count > tpm_engine->buffer_size)
> > > > > > > +		return -E2BIG;
> > > > > > > +
> > > > > > > +	cmd->data_len = count;
> > > > > > > +	memcpy(tpm_engine->data_buffer, buf, count);
> > > > > > > +
> > > > > > > +	return loongson_se_send_engine_cmd(tpm_engine);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static const struct tpm_class_ops tpm_loongson_ops = {
> > > > > > > +	.flags = TPM_OPS_AUTO_STARTUP,
> > > > > > > +	.recv = tpm_loongson_recv,
> > > > > > > +	.send = tpm_loongson_send,
> > > > > > > +};
> > > > > > > +
> > > > > > > +static int tpm_loongson_probe(struct platform_device *pdev)
> > > > > > > +{
> > > > > > > +	struct loongson_se_engine *tpm_engine;
> > > > > > > +	struct device *dev = &pdev->dev;
> > > > > > > +	struct tpm_loongson_cmd *cmd;
> > > > > > > +	struct tpm_chip *chip;
> > > > > > > +
> > > > > > > +	tpm_engine = loongson_se_init_engine(dev->parent, SE_ENGINE_TPM);
> > > > > > > +	if (!tpm_engine)
> > > > > > > +		return -ENODEV;
> > > > > > > +	cmd = tpm_engine->command;
> > > > > > > +	cmd->cmd_id = SE_CMD_TPM;
> > > > > > > +	cmd->data_off = tpm_engine->buffer_off;
> > > > > > > +
> > > > > > > +	chip = tpmm_chip_alloc(dev, &tpm_loongson_ops);
> > > > > > > +	if (IS_ERR(chip))
> > > > > > > +		return PTR_ERR(chip);
> > > > > > > +	chip->flags = TPM_CHIP_FLAG_TPM2 | TPM_CHIP_FLAG_IRQ;
> > > > > > > +	dev_set_drvdata(&chip->dev, tpm_engine);
> > > > > > > +
> > > > > > > +	return tpm_chip_register(chip);
> > > > > > > +}
> > > > > > > +
> > > > > > > +static struct platform_driver tpm_loongson = {
> > > > > > > +	.probe   = tpm_loongson_probe,
> > > > > > > +	.driver  = {
> > > > > > > +		.name  = "loongson-tpm",
> > > > > > 
> > > > > > This patch looks otherwise great but I'd prefer here tho use
> > > > > > "tpm_loongson_probe" for the value of the name field.
> > > > > 
> > > > > Where does this stipulation come from?  No other driver does this [0].
> > > > > driver.name should be a nicely formatted, human readable string
> > > > > describing the name of the device.  Not a function name.
> > > > 
> > > > What defines "human-readable" here? I see both as somewhat the
> > > > same level of "readability" ;-)
> > > > 
> > > > > 
> > > > > [0] git grep -A15 "static struct platform_driver" | grep ".name = .*probe"
> > > > 
> > > > What I'm getting:
> > > > 
> > > > $ git grep -l -e platform_driver_register --or -e module_platform_driver
> > > > drivers/char/tpm | xargs git grep "\.name"
> > > > drivers/char/tpm/tpm_atmel.c:           .name = "tpm_atmel",
> > > > drivers/char/tpm/tpm_ftpm_tee.c:                .name = "ftpm-tee",
> > > > drivers/char/tpm/tpm_ftpm_tee.c:                .name           =
> > > > "optee-ftpm",
> > > > drivers/char/tpm/tpm_nsc.c:             .name    = "tpm_nsc",
> > > > drivers/char/tpm/tpm_svsm.c:            .name = "tpm-svsm",
> > > > drivers/char/tpm/tpm_tis.c:     .name = "tpm_tis",
> > > > drivers/char/tpm/tpm_tis.c:             .name           = "tpm_tis",
> > > > drivers/char/tpm/tpm_tis_synquacer.c:           .name           =
> > > > "tpm_tis_synquacer",
> > > > 
> > > > Do you consider e.g, "tpm_tis" as "less human-readable".
> > > > 
> > > > I don't necessarily fight against the name chosen. Your arguments
> > > > just plain no make sense, so I just merely want to understand this.
> > > > That's all.
> > > 
> > > In 64% of cases '-' is preferred to '_' for device names.
> > > 
> > > Human readable is probably a bit of a stretch in this context, so I'll
> > > retract that part of the statement.  However, we should be using device
> > > names, not names of functions which remain meaningless (which is what I
> > > really meant by 'readable') to the user.  Where else do you see the
> > > .probe() function name being used as a device name?
> > 
> > Oops now I see what you mean. I meant to write "tpm_loongson", i.e.
> > matching tpm_tis, tpm_crb etc. Sorry my bad.
> 
> Ah, gotcha.  No worries.
> 
> "tpm_loongson" wouldn't be my preference, but is acceptable.

It's more like that I'm worried about coherency. There's now bunch
of convention (looking at grep). So I need to pick one and not
increase chaos further :-) tpm_* is the preference as it is still
dominating convention.


> 
> -- 
> Lee Jones [李琼斯]

BR, Jarkko

Re: [PATCH v11 3/4] tpm: Add a driver for Loongson TPM device

Posted by Huacai Chen 3 months, 2 weeks ago

On Thu, Jun 26, 2025 at 6:59 PM Jarkko Sakkinen <jarkko@kernel.org> wrote:
>
> On Thu, Jun 26, 2025 at 11:30:30AM +0100, Lee Jones wrote:
> > On Wed, 25 Jun 2025, Jarkko Sakkinen wrote:
> >
> > > On Wed, Jun 25, 2025 at 02:40:47PM +0100, Lee Jones wrote:
> > > > On Wed, 25 Jun 2025, Jarkko Sakkinen wrote:
> > > >
> > > > > On Wed, Jun 25, 2025 at 09:05:27AM +0100, Lee Jones wrote:
> > > > > > On Wed, 25 Jun 2025, Jarkko Sakkinen wrote:
> > > > > >
> > > > > > > On Thu, Jun 19, 2025 at 10:51:37AM +0800, Qunqin Zhao wrote:
> > > > > > > > Loongson Security Engine supports random number generation, hash,
> > > > > > > > symmetric encryption and asymmetric encryption. Based on these
> > > > > > > > encryption functions, TPM2 have been implemented in the Loongson
> > > > > > > > Security Engine firmware. This driver is responsible for copying data
> > > > > > > > into the memory visible to the firmware and receiving data from the
> > > > > > > > firmware.
> > > > > > > >
> > > > > > > > Co-developed-by: Yinggang Gu <guyinggang@loongson.cn>
> > > > > > > > Signed-off-by: Yinggang Gu <guyinggang@loongson.cn>
> > > > > > > > Signed-off-by: Qunqin Zhao <zhaoqunqin@loongson.cn>
> > > > > > > > Reviewed-by: Huacai Chen <chenhuacai@loongson.cn>
> > > > > > > > ---
> > > > > > > >  drivers/char/tpm/Kconfig        |  9 ++++
> > > > > > > >  drivers/char/tpm/Makefile       |  1 +
> > > > > > > >  drivers/char/tpm/tpm_loongson.c | 84 +++++++++++++++++++++++++++++++++
> > > > > > > >  3 files changed, 94 insertions(+)
> > > > > > > >  create mode 100644 drivers/char/tpm/tpm_loongson.c
> > > > > > > >
> > > > > > > > diff --git a/drivers/char/tpm/Kconfig b/drivers/char/tpm/Kconfig
> > > > > > > > index dddd702b2..ba3924eb1 100644
> > > > > > > > --- a/drivers/char/tpm/Kconfig
> > > > > > > > +++ b/drivers/char/tpm/Kconfig
> > > > > > > > @@ -189,6 +189,15 @@ config TCG_IBMVTPM
> > > > > > > >     will be accessible from within Linux.  To compile this driver
> > > > > > > >     as a module, choose M here; the module will be called tpm_ibmvtpm.
> > > > > > > >
> > > > > > > > +config TCG_LOONGSON
> > > > > > > > + tristate "Loongson TPM Interface"
> > > > > > > > + depends on MFD_LOONGSON_SE
> > > > > > > > + help
> > > > > > > > +   If you want to make Loongson TPM support available, say Yes and
> > > > > > > > +   it will be accessible from within Linux. To compile this
> > > > > > > > +   driver as a module, choose M here; the module will be called
> > > > > > > > +   tpm_loongson.
> > > > > > > > +
> > > > > > > >  config TCG_XEN
> > > > > > > >   tristate "XEN TPM Interface"
> > > > > > > >   depends on TCG_TPM && XEN
> > > > > > > > diff --git a/drivers/char/tpm/Makefile b/drivers/char/tpm/Makefile
> > > > > > > > index 9de1b3ea3..5b5cdc0d3 100644
> > > > > > > > --- a/drivers/char/tpm/Makefile
> > > > > > > > +++ b/drivers/char/tpm/Makefile
> > > > > > > > @@ -46,3 +46,4 @@ obj-$(CONFIG_TCG_ARM_CRB_FFA) += tpm_crb_ffa.o
> > > > > > > >  obj-$(CONFIG_TCG_VTPM_PROXY) += tpm_vtpm_proxy.o
> > > > > > > >  obj-$(CONFIG_TCG_FTPM_TEE) += tpm_ftpm_tee.o
> > > > > > > >  obj-$(CONFIG_TCG_SVSM) += tpm_svsm.o
> > > > > > > > +obj-$(CONFIG_TCG_LOONGSON) += tpm_loongson.o
> > > > > > > > diff --git a/drivers/char/tpm/tpm_loongson.c b/drivers/char/tpm/tpm_loongson.c
> > > > > > > > new file mode 100644
> > > > > > > > index 000000000..5cbdb37f8
> > > > > > > > --- /dev/null
> > > > > > > > +++ b/drivers/char/tpm/tpm_loongson.c
> > > > > > > > @@ -0,0 +1,84 @@
> > > > > > > > +// SPDX-License-Identifier: GPL-2.0
> > > > > > > > +/* Copyright (c) 2025 Loongson Technology Corporation Limited. */
> > > > > > > > +
> > > > > > > > +#include <linux/device.h>
> > > > > > > > +#include <linux/mfd/loongson-se.h>
> > > > > > > > +#include <linux/platform_device.h>
> > > > > > > > +#include <linux/wait.h>
> > > > > > > > +
> > > > > > > > +#include "tpm.h"
> > > > > > > > +
> > > > > > > > +struct tpm_loongson_cmd {
> > > > > > > > + u32 cmd_id;
> > > > > > > > + u32 data_off;
> > > > > > > > + u32 data_len;
> > > > > > > > + u32 pad[5];
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +static int tpm_loongson_recv(struct tpm_chip *chip, u8 *buf, size_t count)
> > > > > > > > +{
> > > > > > > > + struct loongson_se_engine *tpm_engine = dev_get_drvdata(&chip->dev);
> > > > > > > > + struct tpm_loongson_cmd *cmd_ret = tpm_engine->command_ret;
> > > > > > > > +
> > > > > > > > + if (cmd_ret->data_len > count)
> > > > > > > > +         return -EIO;
> > > > > > > > +
> > > > > > > > + memcpy(buf, tpm_engine->data_buffer, cmd_ret->data_len);
> > > > > > > > +
> > > > > > > > + return cmd_ret->data_len;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static int tpm_loongson_send(struct tpm_chip *chip, u8 *buf, size_t count)
> > > > > > > > +{
> > > > > > > > + struct loongson_se_engine *tpm_engine = dev_get_drvdata(&chip->dev);
> > > > > > > > + struct tpm_loongson_cmd *cmd = tpm_engine->command;
> > > > > > > > +
> > > > > > > > + if (count > tpm_engine->buffer_size)
> > > > > > > > +         return -E2BIG;
> > > > > > > > +
> > > > > > > > + cmd->data_len = count;
> > > > > > > > + memcpy(tpm_engine->data_buffer, buf, count);
> > > > > > > > +
> > > > > > > > + return loongson_se_send_engine_cmd(tpm_engine);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static const struct tpm_class_ops tpm_loongson_ops = {
> > > > > > > > + .flags = TPM_OPS_AUTO_STARTUP,
> > > > > > > > + .recv = tpm_loongson_recv,
> > > > > > > > + .send = tpm_loongson_send,
> > > > > > > > +};
> > > > > > > > +
> > > > > > > > +static int tpm_loongson_probe(struct platform_device *pdev)
> > > > > > > > +{
> > > > > > > > + struct loongson_se_engine *tpm_engine;
> > > > > > > > + struct device *dev = &pdev->dev;
> > > > > > > > + struct tpm_loongson_cmd *cmd;
> > > > > > > > + struct tpm_chip *chip;
> > > > > > > > +
> > > > > > > > + tpm_engine = loongson_se_init_engine(dev->parent, SE_ENGINE_TPM);
> > > > > > > > + if (!tpm_engine)
> > > > > > > > +         return -ENODEV;
> > > > > > > > + cmd = tpm_engine->command;
> > > > > > > > + cmd->cmd_id = SE_CMD_TPM;
> > > > > > > > + cmd->data_off = tpm_engine->buffer_off;
> > > > > > > > +
> > > > > > > > + chip = tpmm_chip_alloc(dev, &tpm_loongson_ops);
> > > > > > > > + if (IS_ERR(chip))
> > > > > > > > +         return PTR_ERR(chip);
> > > > > > > > + chip->flags = TPM_CHIP_FLAG_TPM2 | TPM_CHIP_FLAG_IRQ;
> > > > > > > > + dev_set_drvdata(&chip->dev, tpm_engine);
> > > > > > > > +
> > > > > > > > + return tpm_chip_register(chip);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +static struct platform_driver tpm_loongson = {
> > > > > > > > + .probe   = tpm_loongson_probe,
> > > > > > > > + .driver  = {
> > > > > > > > +         .name  = "loongson-tpm",
> > > > > > >
> > > > > > > This patch looks otherwise great but I'd prefer here tho use
> > > > > > > "tpm_loongson_probe" for the value of the name field.
> > > > > >
> > > > > > Where does this stipulation come from?  No other driver does this [0].
> > > > > > driver.name should be a nicely formatted, human readable string
> > > > > > describing the name of the device.  Not a function name.
> > > > >
> > > > > What defines "human-readable" here? I see both as somewhat the
> > > > > same level of "readability" ;-)
> > > > >
> > > > > >
> > > > > > [0] git grep -A15 "static struct platform_driver" | grep ".name = .*probe"
> > > > >
> > > > > What I'm getting:
> > > > >
> > > > > $ git grep -l -e platform_driver_register --or -e module_platform_driver
> > > > > drivers/char/tpm | xargs git grep "\.name"
> > > > > drivers/char/tpm/tpm_atmel.c:           .name = "tpm_atmel",
> > > > > drivers/char/tpm/tpm_ftpm_tee.c:                .name = "ftpm-tee",
> > > > > drivers/char/tpm/tpm_ftpm_tee.c:                .name           =
> > > > > "optee-ftpm",
> > > > > drivers/char/tpm/tpm_nsc.c:             .name    = "tpm_nsc",
> > > > > drivers/char/tpm/tpm_svsm.c:            .name = "tpm-svsm",
> > > > > drivers/char/tpm/tpm_tis.c:     .name = "tpm_tis",
> > > > > drivers/char/tpm/tpm_tis.c:             .name           = "tpm_tis",
> > > > > drivers/char/tpm/tpm_tis_synquacer.c:           .name           =
> > > > > "tpm_tis_synquacer",
> > > > >
> > > > > Do you consider e.g, "tpm_tis" as "less human-readable".
> > > > >
> > > > > I don't necessarily fight against the name chosen. Your arguments
> > > > > just plain no make sense, so I just merely want to understand this.
> > > > > That's all.
> > > >
> > > > In 64% of cases '-' is preferred to '_' for device names.
> > > >
> > > > Human readable is probably a bit of a stretch in this context, so I'll
> > > > retract that part of the statement.  However, we should be using device
> > > > names, not names of functions which remain meaningless (which is what I
> > > > really meant by 'readable') to the user.  Where else do you see the
> > > > .probe() function name being used as a device name?
> > >
> > > Oops now I see what you mean. I meant to write "tpm_loongson", i.e.
> > > matching tpm_tis, tpm_crb etc. Sorry my bad.
> >
> > Ah, gotcha.  No worries.
> >
> > "tpm_loongson" wouldn't be my preference, but is acceptable.
>
> It's more like that I'm worried about coherency. There's now bunch
> of convention (looking at grep). So I need to pick one and not
> increase chaos further :-) tpm_* is the preference as it is still
> dominating convention.
But there is another coherency, you can see this in the 1st patch:

+static const struct mfd_cell engines[] = {
+ { .name = "loongson-rng" },
+ { .name = "loongson-tpm" },
+};

Huacai

>
>
> >
> > --
> > Lee Jones [李琼斯]
>
> BR, Jarkko
>

Re: [PATCH v11 3/4] tpm: Add a driver for Loongson TPM device

Posted by Jarkko Sakkinen 3 months, 2 weeks ago

On Thu, Jun 26, 2025 at 08:48:35PM +0800, Huacai Chen wrote:
> But there is another coherency, you can see this in the 1st patch:
> 
> +static const struct mfd_cell engines[] = {
> + { .name = "loongson-rng" },
> + { .name = "loongson-tpm" },
> +};

I thought already after ffa driver from ARM that I need to fix up
the naming a bit before it explodes. Thus, I'll stick to this.

And e.g., I could easily find DRM driver with opposite order.

> 
> Huacai

BR, Jarkko

Re: [PATCH v11 3/4] tpm: Add a driver for Loongson TPM device

Posted by Qunqin Zhao 3 months, 2 weeks ago

在 2025/6/27 上午2:29, Jarkko Sakkinen 写道:
> On Thu, Jun 26, 2025 at 08:48:35PM +0800, Huacai Chen wrote:
>> But there is another coherency, you can see this in the 1st patch:
>>
>> +static const struct mfd_cell engines[] = {
>> + { .name = "loongson-rng" },
>> + { .name = "loongson-tpm" },
>> +};
> I thought already after ffa driver from ARM that I need to fix up
> the naming a bit before it explodes. Thus, I'll stick to this.
>
> And e.g., I could easily find DRM driver with opposite order.
Next revision:

+static const struct mfd_cell engines[] = {
+	{ .name = "loongson-rng" },
+	{ .name = "tpm_loongson" },
+};
Then
"loongson-rng" can match MFD and Crypto subsystem naming convention.
"tpm_loongson" can match TPM subsystem naming convention.


Thanks,
Qunqin

>> Huacai
> BR, Jarkko

Re: [PATCH v11 3/4] tpm: Add a driver for Loongson TPM device

Posted by Jarkko Sakkinen 3 months, 1 week ago

On Fri, Jun 27, 2025 at 02:46:11PM +0800, Qunqin Zhao wrote:
> 
> 在 2025/6/27 上午2:29, Jarkko Sakkinen 写道:
> > On Thu, Jun 26, 2025 at 08:48:35PM +0800, Huacai Chen wrote:
> > > But there is another coherency, you can see this in the 1st patch:
> > > 
> > > +static const struct mfd_cell engines[] = {
> > > + { .name = "loongson-rng" },
> > > + { .name = "loongson-tpm" },
> > > +};
> > I thought already after ffa driver from ARM that I need to fix up
> > the naming a bit before it explodes. Thus, I'll stick to this.
> > 
> > And e.g., I could easily find DRM driver with opposite order.
> Next revision:
> 
> +static const struct mfd_cell engines[] = {
> +	{ .name = "loongson-rng" },
> +	{ .name = "tpm_loongson" },
> +};
> Then
> "loongson-rng" can match MFD and Crypto subsystem naming convention.
> "tpm_loongson" can match TPM subsystem naming convention.

Great, this works for me. Thank you, appreciate it!

> 
> 
> Thanks,
> Qunqin
> 
> > > Huacai
> > BR, Jarkko
> 

BR, Jarkko

Re: [PATCH v11 3/4] tpm: Add a driver for Loongson TPM device

Posted by Huacai Chen 3 months, 1 week ago

On Fri, Jun 27, 2025 at 2:47 PM Qunqin Zhao <zhaoqunqin@loongson.cn> wrote:
>
>
> 在 2025/6/27 上午2:29, Jarkko Sakkinen 写道:
> > On Thu, Jun 26, 2025 at 08:48:35PM +0800, Huacai Chen wrote:
> >> But there is another coherency, you can see this in the 1st patch:
> >>
> >> +static const struct mfd_cell engines[] = {
> >> + { .name = "loongson-rng" },
> >> + { .name = "loongson-tpm" },
> >> +};
> > I thought already after ffa driver from ARM that I need to fix up
> > the naming a bit before it explodes. Thus, I'll stick to this.
> >
> > And e.g., I could easily find DRM driver with opposite order.
> Next revision:
>
> +static const struct mfd_cell engines[] = {
> +       { .name = "loongson-rng" },
> +       { .name = "tpm_loongson" },
> +};
> Then
> "loongson-rng" can match MFD and Crypto subsystem naming convention.
> "tpm_loongson" can match TPM subsystem naming convention.
If possible, I prefer the old names.

Huacai

>
>
> Thanks,
> Qunqin
>
> >> Huacai
> > BR, Jarkko
>

[PATCH v11 1/4] mfd: Add support for Loongson Security Engine chip controller
[PATCH v11 2/4] crypto: loongson - add Loongson RNG driver support
[PATCH v11 3/4] tpm: Add a driver for Loongson TPM device
[PATCH v11 4/4] MAINTAINERS: Add entry for Loongson Security Engine drivers