Message-ID: <80165218-1295-b028-f77c-6924d572434e@loongson.cn>
Date: Mon, 26 May 2025 10:26:39 +0800
From: Qunqin Zhao <zhaoqunqin@...ngson.cn>
To: Lee Jones <lee@...nel.org>
Cc: herbert@...dor.apana.org.au, davem@...emloft.net, peterhuewe@....de,
 jarkko@...nel.org, linux-kernel@...r.kernel.org, loongarch@...ts.linux.dev,
 linux-crypto@...r.kernel.org, jgg@...pe.ca, linux-integrity@...r.kernel.org,
 pmenzel@...gen.mpg.de, Yinggang Gu <guyinggang@...ngson.cn>,
 Huacai Chen <chenhuacai@...ngson.cn>
Subject: Re: [PATCH v9 1/5] mfd: Add support for Loongson Security Engine chip
 controller


在 2025/5/22 下午9:46, Lee Jones 写道:
> On Tue, 06 May 2025, Qunqin Zhao wrote:
>
>> Loongson Security Engine chip supports RNG, SM2, SM3 and SM4 accelerator
>> engines. This is the base driver for other specific engine drivers.
>>
>> Co-developed-by: Yinggang Gu <guyinggang@...ngson.cn>
>> Signed-off-by: Yinggang Gu <guyinggang@...ngson.cn>
>> Signed-off-by: Qunqin Zhao <zhaoqunqin@...ngson.cn>
>> Reviewed-by: Huacai Chen <chenhuacai@...ngson.cn>
>> ---
>> v8: As explained in the cover letter, moved this driver form MFD to here.
>>      Cleanned up coding style. Added some comments. Divided DMA memory
>>      equally among all engines.
>>
>> v7: Moved Kconfig entry between MFD_INTEL_M10_BMC_PMCI and MFD_QNAP_MCU.
>>
>>      Renamed se_enable_int_locked() to se_enable_int(), then moved the
>>      lock out of se_disable_int().
>>   
>>      "se_send_genl_cmd" ---> "se_send_cmd".
>>      "struct lsse_ch" ---> "struct se_channel".
>>
>> v6: Replace all "ls6000se" with "loongson"
>> v5: Registered "ls6000se-rng" device.
>> v3-v4: None
>>
>>   drivers/mfd/Kconfig             |  11 ++
>>   drivers/mfd/Makefile            |   2 +
>>   drivers/mfd/loongson-se.c       | 235 ++++++++++++++++++++++++++++++++
>>   include/linux/mfd/loongson-se.h |  52 +++++++
>>   4 files changed, 300 insertions(+)
>>   create mode 100644 drivers/mfd/loongson-se.c
>>   create mode 100644 include/linux/mfd/loongson-se.h
> General premise seems okay.
>
> Couple of questions and styling / readability issues.
>
>> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
>> index 22b936310..c2f94b315 100644
>> --- a/drivers/mfd/Kconfig
>> +++ b/drivers/mfd/Kconfig
>> @@ -2369,6 +2369,17 @@ config MFD_INTEL_M10_BMC_PMCI
>>   	  additional drivers must be enabled in order to use the functionality
>>   	  of the device.
>>   
>> +config MFD_LOONGSON_SE
>> +	tristate "Loongson Security Engine chip controller driver"
>> +	depends on LOONGARCH && ACPI
>> +	select MFD_CORE
>> +	help
>> +	  The Loongson Security Engine chip supports RNG, SM2, SM3 and
>> +	  SM4 accelerator engines. Each engine have its own DMA buffer
>> +	  provided by the controller. The kernel cannot directly send
>> +	  commands to the engine and must first send them to the controller,
>> +	  which will forward them to the corresponding engine.
>> +
>>   config MFD_QNAP_MCU
>>   	tristate "QNAP microcontroller unit core driver"
>>   	depends on SERIAL_DEV_BUS
>> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
>> index 948cbdf42..fc50601ca 100644
>> --- a/drivers/mfd/Makefile
>> +++ b/drivers/mfd/Makefile
>> @@ -290,3 +290,5 @@ obj-$(CONFIG_MFD_RSMU_I2C)	+= rsmu_i2c.o rsmu_core.o
>>   obj-$(CONFIG_MFD_RSMU_SPI)	+= rsmu_spi.o rsmu_core.o
>>   
>>   obj-$(CONFIG_MFD_UPBOARD_FPGA)	+= upboard-fpga.o
>> +
>> +obj-$(CONFIG_MFD_LOONGSON_SE)	+= loongson-se.o
>> diff --git a/drivers/mfd/loongson-se.c b/drivers/mfd/loongson-se.c
>> new file mode 100644
>> index 000000000..ce38d8221
>> --- /dev/null
>> +++ b/drivers/mfd/loongson-se.c
>> @@ -0,0 +1,235 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/* Copyright (C) 2025 Loongson Technology Corporation Limited */
> Author(s)?
Will add
>> +#include <linux/acpi.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/errno.h>
>> +#include <linux/init.h>
>> +#include <linux/interrupt.h>
>> +#include <linux/iopoll.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mfd/core.h>
>> +#include <linux/mfd/loongson-se.h>
>> +#include <linux/module.h>
>> +#include <linux/platform_device.h>
>> +
>> +struct loongson_se {
>> +	void __iomem *base;
>> +	spinlock_t dev_lock;
>> +	struct completion cmd_completion;
>> +
>> +	void *dmam_base;
>> +	int dmam_size;
>> +
>> +	struct mutex engine_init_lock;
>> +	struct loongson_se_engine engines[SE_ENGINE_MAX];
>> +};
>> +
>> +struct loongson_se_controller_cmd {
>> +	u32 command_id;
>> +	u32 info[7];
>> +};
>> +
>> +static int loongson_se_poll(struct loongson_se *se, u32 int_bit)
>> +{
>> +	u32 status;
>> +	int err;
>> +
>> +	spin_lock_irq(&se->dev_lock);
>> +
>> +	/* Notify the controller that the engine needs to be started */
>> +	writel(int_bit, se->base + SE_L2SINT_SET);
> Code that is squished together is difficult to read.
>
> '\n'
All '\n' will fix
>> +	/* Polling until the controller has forwarded the engine command */
>> +	err = readl_relaxed_poll_timeout_atomic(se->base + SE_L2SINT_STAT, status,
>> +						!(status & int_bit), 1, 10000);
> How long is that?  Why was that number chosen?
In theory, the hardware will clear int_bit within 10ms.
> Please define the type, like:
>
> LOONSON_ENGINE_CMD_TIMEOUT_MS 10000

Will do this.  But  this should be us.

LOONSON_ENGINE_CMD_TIMEOUT_US 10000

>
> ... or whatever it is.
>
>> +	spin_unlock_irq(&se->dev_lock);
>> +
>> +	return err;
>> +}
>> +
>> +static int loongson_se_send_controller_cmd(struct loongson_se *se,
>> +					   struct loongson_se_controller_cmd *cmd)
>> +{
>> +	u32 *send_cmd = (u32 *)cmd;
>> +	int err, i;
>> +
>> +	for (i = 0; i < SE_SEND_CMD_REG_LEN; i++)
>> +		writel(send_cmd[i], se->base + SE_SEND_CMD_REG + i * 4);
> Is there any reason not to use regmap?

I'm not familiar with regmap, readl/writel is fine for me.

If regmap is necessary, I will try using it.

>> +	err = loongson_se_poll(se, SE_INT_CONTROLLER);
>> +	if (err)
>> +		return err;
>> +
>> +	return wait_for_completion_interruptible(&se->cmd_completion);
>> +}
>> +
>> +int loongson_se_send_engine_cmd(struct loongson_se_engine *engine)
>> +{
>> +	/* After engine initialization, the controller already knows
>> +	 * where to obtain engine commands from. Now all we need to
>> +	 * do is notify the controller that the engine needs to be started.
>> +	 */
> This is not a proper multi-line comment as per Coding Style.
Will fix
>
>> +	int err = loongson_se_poll(engine->se, BIT(engine->id));
>> +
>> +	if (err)
>> +		return err;
>> +
>> +	return wait_for_completion_interruptible(&engine->completion);
>> +}
>> +EXPORT_SYMBOL_GPL(loongson_se_send_engine_cmd);
>> +
>> +struct loongson_se_engine *loongson_se_init_engine(struct device *dev, int id)
> What calls this?  Whose 'dev' is that?

Child device driver would call this, like "loongson-tpm" and "loongson-rng".

   "dev" is  the device we probed in "loongson_se_probe".

>
>> +{
>> +	struct loongson_se *se = dev_get_drvdata(dev);
>> +	struct loongson_se_engine *engine = &se->engines[id];
>> +	struct loongson_se_controller_cmd cmd;
>> +
>> +	engine->se = se;
>> +	engine->id = id;
>> +	init_completion(&engine->completion);
>> +
>> +	/* Divide DMA memory equally among all engines */
>> +	engine->buffer_size = se->dmam_size / SE_ENGINE_MAX;
>> +	engine->buffer_off = (se->dmam_size / SE_ENGINE_MAX) * id;
>> +	engine->data_buffer = se->dmam_base + engine->buffer_off;
>> +
>> +	/*
>> +	 * There has no engine0, use its data buffer as command buffer for other
>> +	 * engines. The DMA memory size is obtained from the ACPI table, which
>> +	 * ensures that the data buffer size of engine0 is larger than the
>> +	 * command buffer size of all engines.
>> +	 */
>> +	engine->command = se->dmam_base + id * (2 * SE_ENGINE_CMD_SIZE);
> Why 2?
One for engine->command, another for engine->command_ret.
>> +	engine->command_ret = engine->command + SE_ENGINE_CMD_SIZE;
>> +
>> +	mutex_lock(&se->engine_init_lock);
>
....
>> +	while (nr_irq) {
>> +		irq = platform_get_irq(pdev, --nr_irq);
> Do the decrement separately at the end of the statement, not hidden here.
>
> Or, probably better still, use a for() loop.
Will use a for() loop
>
>> +		err = devm_request_irq(dev, irq, se_irq_handler, 0, "loongson-se", se);
>> +		if (err)
>> +			dev_err(dev, "failed to request irq: %d\n", irq);
> IRQ
Will fix
>
>> +	}
>> +
>> +	err = loongson_se_init(se, paddr, se->dmam_size);
>> +	if (err)
>> +		return err;
>> +
>> +	return devm_mfd_add_devices(dev, 0, engines, ARRAY_SIZE(engines), NULL, 0, NULL);
> Why 0?
Next revision:

return devm_mfd_add_devices(dev, PLATFORM_DEVID_NONE, engines, ARRAY_SIZE(engines), NULL, 0, NULL);

>
>> +}
>> +
>> +static const struct acpi_device_id loongson_se_acpi_match[] = {
>> +	{"LOON0011", 0},
> There should be spaces after the '{' and before the '}'.
Sorry, Will fix
>
>> +	{}
>> +};
>> +MODULE_DEVICE_TABLE(acpi, loongson_se_acpi_match);
>> +
>> +static struct platform_driver loongson_se_driver = {
>> +	.probe   = loongson_se_probe,
>> +	.driver  = {
>> +		.name  = "loongson-se",
>> +		.acpi_match_table = loongson_se_acpi_match,
>> +	},
>> +};
>> +module_platform_driver(loongson_se_driver);
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Yinggang Gu <guyinggang@...ngson.cn>");
>> +MODULE_AUTHOR("Qunqin Zhao <zhaoqunqin@...ngson.cn>");
>> +MODULE_DESCRIPTION("Loongson Security Engine chip controller driver");
>> diff --git a/include/linux/mfd/loongson-se.h b/include/linux/mfd/loongson-se.h
>> new file mode 100644
>> index 000000000..f962d6143
>> --- /dev/null
>> +++ b/include/linux/mfd/loongson-se.h
>> @@ -0,0 +1,52 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ */
>> +/* Copyright (C) 2025 Loongson Technology Corporation Limited */
>> +
>> +#ifndef __LOONGSON_SE_H__
>> +#define __LOONGSON_SE_H__
> __MFD_*
Will fix
>
>> +#define SE_SEND_CMD_REG			0x0
>> +#define SE_SEND_CMD_REG_LEN		0x8
>> +/* controller command id */
> Uppercase char to start comments.
>
> "ID"

Will fix


Thanks,

Qunqin.

Powered by blists - more mailing lists