* Re: [PATCH v1 1/1] nvmem: aspeed: Add read-only AST2600 OTP support
2026-11-13 3:42 ` [PATCH v1 1/1] " Prasanth Kumar Padarthi
@ 2026-11-13 13:05 ` Tan Siewert
0 siblings, 0 replies; 3+ messages in thread
From: Tan Siewert @ 2026-11-13 13:05 UTC (permalink / raw)
To: Prasanth Kumar Padarthi, srini
Cc: joel, andrew, ryan_chen, billy_tsai, neal_liu, linux-kernel,
linux-arm-kernel, linux-aspeed
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
^ permalink raw reply [flat|nested] 3+ messages in thread