Re: [PATCH v1 1/1] nvmem: aspeed: Add read-only AST2600 OTP support

[Tan Siewert <tan.siewert@9elements.com>]

On 11/13/26 4:42 AM, Prasanth Kumar Padarthi wrote:
> Expose permitted AST2600 OTP data through NVMEM using the existing SBC
> node. Serialize controller transactions, bound completion polling and
> reject reads into the configured secure prefix.

This only repeats what the code does. Please explain *why*. Who uses the 
data and which cells?

> 
> Adapt the read sequence from ASPEED's downstream driver, preserving its
> attribution. Register the driver for ASPEED review and restrict raw OTP
> access to root.

If you mean the MAINTAINERS hunk, just say so.

> 
> Link: https://github.com/AspeedTech-BMC/linux/blob/769f62b7baa84d6998723b0ea60280e380183553/drivers/soc/aspeed/ast2600-otp.c

No DCO Sign-off (neither from you nor Neal). And given that you've used 
the downstream driver as base it'd be cool if you can keep Neal as 
Co-developed-by in (with Neal's agreement).

> ---
> Tested on ASRock Rack B650D4U, AST2600 A3, OpenBMC Linux 6.18.24:

Please test against the base commit you've used (i.e. 7.4-rc3). Using a 
vendor / OpenBMC tree here is not useful at all.

> - Repeated, paired and cross-pair reads matched.
> - Four concurrent readers completed 40 matching reads.
> - Bytes 8172..8183 matched a separate vendor-derived reader.
> - Synthetic pre-MMIO timeout, failure latching and rebind recovery passed.
> Host fault-injection tests passed all 42 polling-timeout cases.
> 
>   MAINTAINERS                 |   1 +
>   drivers/nvmem/Kconfig       |  12 ++
>   drivers/nvmem/Makefile      |   1 +
>   drivers/nvmem/ast2600-otp.c | 259 ++++++++++++++++++++++++++++++++++++
>   4 files changed, 273 insertions(+)
>   create mode 100644 drivers/nvmem/ast2600-otp.c
> 
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 93abbb3..c92ea8e 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -2813,6 +2813,7 @@ F:	Documentation/devicetree/bindings/arm/aspeed/
>   F:	arch/arm/boot/dts/aspeed/
>   F:	arch/arm/mach-aspeed/
>   F:	arch/arm64/boot/dts/aspeed/
> +F:	drivers/nvmem/ast2600-otp.c
>   N:	aspeed
>   
>   ARM/AXIADO ARCHITECTURE
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index d2f5bb2..cdcd071 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -164,6 +164,18 @@ config NVMEM_APPLE_SPMI
>   	  This driver can also be built as a module. If so, the module
>   	  will be called apple-nvmem-spmi.
>   
> +config NVMEM_ASPEED_AST2600_OTP
> +	tristate "ASPEED AST2600 OTP support"
> +	depends on ARCH_ASPEED || COMPILE_TEST
> +	depends on HAS_IOMEM
> +	help
> +	  Read-only access to the AST2600 OTP data region. The configured
> +	  secure prefix is not exposed. Fuse programming is not supported.
> +	  The controller reads paired 32-bit words through command registers.

This is implementation-specific and doesn't help whether to enable the 
option. Drop.

> +	  Access to raw OTP data is restricted to root.
> +
> +	  If built as a module, the module will be called ast2600-otp.
> +

"If unsure, select N"?

>   config NVMEM_BCM_OCOTP
>   	tristate "Broadcom On-Chip OTP Controller support"
>   	depends on ARCH_BCM_IPROC || COMPILE_TEST
> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> index 5e70bad..9fac69d 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -23,6 +23,7 @@ obj-$(CONFIG_NVMEM_APPLE_EFUSES)	+= nvmem-apple-efuses.o
>   nvmem-apple-efuses-y 			:= apple-efuses.o
>   obj-$(CONFIG_NVMEM_APPLE_SPMI)		+= apple_nvmem_spmi.o
>   apple_nvmem_spmi-y			:= apple-spmi-nvmem.o
> +obj-$(CONFIG_NVMEM_ASPEED_AST2600_OTP)	+= ast2600-otp.o

Maybe call the file "nvmem-aspeed-otp" or so? Would be nice if we can 
extend the driver for AST2700 support later on.

>   obj-$(CONFIG_NVMEM_BCM_OCOTP)		+= nvmem-bcm-ocotp.o
>   nvmem-bcm-ocotp-y			:= bcm-ocotp.o
>   obj-$(CONFIG_NVMEM_BRCM_NVRAM)		+= nvmem_brcm_nvram.o
> diff --git a/drivers/nvmem/ast2600-otp.c b/drivers/nvmem/ast2600-otp.c
> new file mode 100644
> index 0000000..94b1225
> --- /dev/null
> +++ b/drivers/nvmem/ast2600-otp.c
> @@ -0,0 +1,259 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) ASPEED Technology Inc.
> + *
> + * ASPEED AST2600 OTP Controller driver
> + * Read sequencing adapted from ASPEED's downstream ast2600-otp driver.

This comment feels unnecessary. It's already been said in the commit 
description.

> + */
> +
> +#include <linux/align.h>

align could be removed if you use round_down() or word & ~1 maybe?

> +#include <linux/bitfield.h>
> +#include <linux/delay.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/of.h>

You don't call any of_ functions. Replace with mod_devicetable.h.

> +#include <linux/platform_device.h>
> +#include <linux/slab.h>

unused. devm_kzalloc comes through platform_device.h.

> +#include <linux/string.h>
> +
> +#define AST2600_OTP_PROTECT_KEY		0x00
> +#define AST2600_OTP_CMD			0x04
> +#define AST2600_OTP_ADDR		0x10
> +#define AST2600_OTP_STATUS		0x14

The SB Controller driver checks for the OTP_PROTECTED bit. What happens 
if OTP protection is enabled?

> +#define AST2600_OTP_COMPARE_1		0x20
> +#define AST2600_OTP_COMPARE_2		0x24
> +
> +#define AST2600_OTP_CMD_READ		0x23b1e361
> +#define AST2600_OTP_CMD_WRITE_MODE	0x23b1e362
> +#define AST2600_OTP_UNLOCK_KEY		0x349fe38a
> +#define AST2600_OTP_STATUS_DONE		0x6

Describe the bits here. Either two BIT() or GENMASK(2, 1).

> +#define AST2600_OTP_TIMEOUT_US		100000

Where does this come from?

> +
> +#define AST2600_OTP_DATA_WORDS		0x800
> +#define AST2600_OTP_SIZE			(AST2600_OTP_DATA_WORDS * sizeof(u32))
> +#define AST2600_OTP_CONF0_ADDR		0x800
> +#define AST2600_OTP_CONF0_SEC_SIZE		GENMASK(21, 16)
> +#define AST2600_OTP_SEC_SIZE_UNIT		(32 * sizeof(u32))
> +
> +struct ast2600_otp {
> +	void __iomem *base;
> +	/* Serialize the complete unlock, read and relock sequence. */
> +	struct mutex lock;
> +	bool failed;
> +};
> +
> +static int ast2600_otp_wait_complete(struct ast2600_otp *otp)
> +{
> +	u32 status;
> +	int i, ret;
> +
> +	/* Allow the command to start before sampling the idle flags. */
> +	udelay(1);

You could use read_poll_timeout too?

> +
> +	/* Match the downstream sequence: observe both idle flags twice. */
> +	for (i = 0; i < 2; i++) {

This is not a valid reason. Do you really have to poll twice? Is this an 
erratum? If so, please cite it.

> +		ret = readl_poll_timeout(otp->base + AST2600_OTP_STATUS,
> +					 status,
> +					 (status & AST2600_OTP_STATUS_DONE) ==
> +					 AST2600_OTP_STATUS_DONE,
> +					 1, AST2600_OTP_TIMEOUT_US);
> +		if (ret) {
> +			/* Do not reuse a controller with an outstanding command. */
> +			otp->failed = true;

That'd disable the device on a single timeout. Do we really have to do 
this, or would it be enough to retry on the next access? If you want to 
keep it, then a dev_err() would be good instead of failing it silently.

> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}
> +
> +static int ast2600_otp_prepare_read(struct ast2600_otp *otp)
> +{
> +	static const u32 mode_addresses[] = { 0x3000, 0x5000, 0x1000 };
> +	int i, ret;
> +
> +	ret = ast2600_otp_wait_complete(otp);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * Restore the macro's default read mode, as in otp_soak(ctx, 0).
> +	 * These writes target volatile mode registers, not OTP fuse cells.
> +	 * No fuse programming command or programming timing is used.
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(mode_addresses); i++) {
> +		writel(mode_addresses[i], otp->base + AST2600_OTP_ADDR);
> +		writel(0, otp->base + AST2600_OTP_COMPARE_1);
> +		writel(AST2600_OTP_CMD_WRITE_MODE, otp->base + AST2600_OTP_CMD);

Setting the OTP back to default doesn't need write mode if I understand 
the datasheet correctly.

> +		ret = ast2600_otp_wait_complete(otp);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +
> +static int ast2600_otp_read_pair(struct ast2600_otp *otp,
> +				 unsigned int aligned_word,
> +				 u32 *word0, u32 *word1)
> +{
> +	int ret;
> +
> +	/* The hardware reads two words starting at an even word index. */
> +	if ((aligned_word & 1) || aligned_word >= AST2600_OTP_DATA_WORDS)
> +		return -EINVAL;

You're already passing an aligned index everywhere which makes this code 
redundant. It also blocks the read for CONF0, so please drop this check 
and use read_pair() for CONF0 too.

> +
> +	writel(aligned_word, otp->base + AST2600_OTP_ADDR);
> +
> +	writel(AST2600_OTP_CMD_READ,
> +	       otp->base + AST2600_OTP_CMD);
> +
> +	ret = ast2600_otp_wait_complete(otp);
> +	if (ret)
> +		return ret;
> +
> +	*word0 = readl(otp->base + AST2600_OTP_COMPARE_1);
> +	*word1 = readl(otp->base + AST2600_OTP_COMPARE_2);
> +
> +	return 0;
> +}
> +
> +static int ast2600_otp_read(void *context, unsigned int offset,
> +			    void *val, size_t bytes)
> +{
> +	struct ast2600_otp *otp = context;
> +	u8 *buf = val;
> +	unsigned int word_index;
> +	unsigned int aligned_word;
> +	unsigned int secure_size;
> +	__le32 pair[2];
> +	u32 conf0;
> +	u32 word0, word1;
> +	size_t chunk, skip;
> +	int ret;
> +
> +	if (!bytes)
> +		return -EINVAL;

OTP could be empty too?

> +
> +	if (offset % sizeof(u32) || bytes % sizeof(u32))
> +		return -EINVAL;
> +
> +	if (offset >= AST2600_OTP_SIZE ||
> +	    bytes > AST2600_OTP_SIZE - offset)
> +		return -EINVAL;
> +
> +	mutex_lock(&otp->lock);

guard(mutex)(&priv->lock); ? Requires <linux/cleanup.h>.

> +
> +	if (otp->failed) {
> +		ret = -EIO;
> +		goto out_unlock;
> +	}
> +
> +	writel(AST2600_OTP_UNLOCK_KEY, otp->base + AST2600_OTP_PROTECT_KEY);
> +
> +	ret = ast2600_otp_prepare_read(otp);
> +	if (ret)
> +		goto out_lock;

The secure region size is fixed once programmed. Maybe read it once in 
probe instead of on every access? With nvmem_keepout, this would clean 
up the read path a bit.

> +
> +	/* Configuration word 0 describes the unreadable secure data prefix. */
> +	writel(AST2600_OTP_CONF0_ADDR, otp->base + AST2600_OTP_ADDR);
> +	writel(AST2600_OTP_CMD_READ, otp->base + AST2600_OTP_CMD);
> +	ret = ast2600_otp_wait_complete(otp);
> +	if (ret)
> +		goto out_lock;
> +
> +	conf0 = readl(otp->base + AST2600_OTP_COMPARE_1);
> +	secure_size = FIELD_GET(AST2600_OTP_CONF0_SEC_SIZE, conf0) *
> +		      AST2600_OTP_SEC_SIZE_UNIT;

You can also keep out the secure region by using nvmem_keepout.

> +	/* Conservatively deny the entire secure prefix, including its header. */
> +	if (offset < secure_size) {
> +		ret = -EACCES;
> +		goto out_lock;
> +	}
> +
> +	while (bytes) {
> +		word_index = offset / sizeof(u32);
> +		aligned_word = ALIGN_DOWN(word_index, 2);
> +
> +		ret = ast2600_otp_read_pair(otp, aligned_word,
> +					    &word0, &word1);
> +		if (ret)
> +			goto out_lock;
> +
> +		pair[0] = cpu_to_le32(word0);
> +		pair[1] = cpu_to_le32(word1);
> +		skip = (word_index - aligned_word) * sizeof(u32);
> +		chunk = min(bytes, sizeof(pair) - skip);
> +
> +		memcpy(buf, (u8 *)pair + skip, chunk);
> +		buf += chunk;
> +		offset += chunk;
> +		bytes -= chunk;

NVMEM already returns the data in the correct order if I see this right. 
You can store the readl() results directly into a u32*buf var. That'd 
make the pair/skip/chunk code and endianness conversion unnecessary.

> +	}
> +
> +	ret = 0;
> +
> +out_lock:
> +	writel(0, otp->base + AST2600_OTP_PROTECT_KEY);
> +	/* Flush the protection write before another reader takes the mutex. */
> +	readl(otp->base + AST2600_OTP_PROTECT_KEY);

for what are you reading the otp key register here?

> +out_unlock:
> +	mutex_unlock(&otp->lock);
> +	return ret;
> +}
> +
> +static int ast2600_otp_probe(struct platform_device *pdev)
> +{
> +	struct nvmem_config config = {};
> +	struct nvmem_device *nvmem;
> +	struct ast2600_otp *otp;

Can we name this priv maybe?

> +
> +	otp = devm_kzalloc(&pdev->dev, sizeof(*otp), GFP_KERNEL);

Maybe assign &pdev->dev as struct device * variable and use that one 
instead?

> +	if (!otp)
> +		return -ENOMEM;
> +
> +	otp->base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(otp->base))
> +		return PTR_ERR(otp->base);
> +
> +	mutex_init(&otp->lock);

devm_mutex_init?

> +
> +	config.dev = &pdev->dev;

Same here with the var.

> +	config.name = "ast2600-otp";
> +	config.id = NVMEM_DEVID_AUTO;
> +	config.owner = THIS_MODULE;
> +	config.type = NVMEM_TYPE_OTP;
> +	config.read_only = true;
> +	config.root_only = true;
> +	config.word_size = sizeof(u32);
> +	config.stride = sizeof(u32);
> +	config.size = AST2600_OTP_SIZE;
> +	config.reg_read = ast2600_otp_read;

These can already be set in the config struct before.

> +	config.priv = otp;
> +
> +	nvmem = devm_nvmem_register(&pdev->dev, &config);

And here.

> +	return PTR_ERR_OR_ZERO(nvmem);
> +}
> +
> +static const struct of_device_id ast2600_otp_of_match[] = {
> +	{ .compatible = "aspeed,ast2600-sbc" },

The SB Controller driver has "additionalProperties: false" set and 
doesn't allow nvmem properties.

Please add "$ref: /schemas/nvmem/nvmem.yaml#" and 
"unevaluatedProperties: false" as a separate patch.

Tan

> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, ast2600_otp_of_match);
> +
> +static struct platform_driver ast2600_otp_driver = {
> +	.probe = ast2600_otp_probe,
> +	.driver = {
> +		.name = "ast2600-otp",
> +		.of_match_table = ast2600_otp_of_match,
> +	},
> +};
> +module_platform_driver(ast2600_otp_driver);
> +
> +MODULE_AUTHOR("Neal Liu <neal_liu@aspeedtech.com>");
> +MODULE_AUTHOR("Prasanth Kumar Padarthi <prasanth.padarthi10@gmail.com>");
> +MODULE_DESCRIPTION("ASPEED AST2600 read-only OTP NVMEM driver");
> +MODULE_LICENSE("GPL");
> base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4