From: Lee Jones <lee@kernel.org>
To: MANNURU VENKATESWARLU <v-mannuru@ti.com>
Cc: linux-arm-kernel@lists.infradead.org, mfd@lists.linux.dev,
linux-kernel@vger.kernel.org, n-francis@ti.com, s-k6@ti.com,
bb@ti.com
Subject: Re: [RFC PATCH 05/22] mfd: ti-ddrss: Add TI K3 DDR subsystem MFD core driver
Date: Thu, 23 Jul 2026 13:58:00 +0100 [thread overview]
Message-ID: <20260723125800.GJ3363113@google.com> (raw)
In-Reply-To: <20260714125553.3304282-1-v-mannuru@ti.com>
On Tue, 14 Jul 2026, MANNURU VENKATESWARLU wrote:
> Add a multi-function device core driver for the TI K3 DDR
> subsystem wrapper. The driver maps DDR controller registers, reads
> SoC-specific configuration via device match data, and instantiates
> the MR4 refresh-rate and PMU child devices.
>
> Signed-off-by: MANNURU VENKATESWARLU <v-mannuru@ti.com>
> ---
> drivers/mfd/Kconfig | 13 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/ti-ddrss-core.c | 280 +++++++++++++++++++++++++++++++++++
> include/linux/mfd/ti-ddrss.h | 53 +++++++
> 4 files changed, 347 insertions(+)
> create mode 100644 drivers/mfd/ti-ddrss-core.c
> create mode 100644 include/linux/mfd/ti-ddrss.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index 35f6e9b76d056..a13ecc212f58e 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1834,6 +1834,19 @@ config MFD_TI_LP87565
> This driver can also be built as a module. If so, the module
> will be called lp87565.
>
> +config MFD_TI_DDRSS
> + tristate "TI K3 DDR Subsystem"
> + depends on ARCH_K3 || COMPILE_TEST
> + select MFD_CORE
> + select REGMAP_MMIO
> + help
> + Core MFD driver for TI K3 DDR subsystem. Provides register access
> + management and coordinates hwmon temperature monitoring and perf
> + counter child drivers for DDR performance and thermal monitoring.
> +
> + This driver can also be built as a module. If so, the module
> + will be called ti-ddrss-core.
> +
> config MFD_TPS65218
> tristate "TI TPS65218 Power Management chips"
> depends on I2C && OF
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index dd4bb7e77c336..1258d09f3a40b 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -27,6 +27,7 @@ obj-$(CONFIG_MFD_MACSMC) += macsmc.o
> obj-$(CONFIG_MFD_TI_LP873X) += lp873x.o
> obj-$(CONFIG_MFD_TI_LP87565) += lp87565.o
> obj-$(CONFIG_MFD_TI_AM335X_TSCADC) += ti_am335x_tscadc.o
> +obj-$(CONFIG_MFD_TI_DDRSS) += ti-ddrss-core.o
>
> obj-$(CONFIG_MFD_STMPE) += stmpe.o
> obj-$(CONFIG_STMPE_I2C) += stmpe-i2c.o
> diff --git a/drivers/mfd/ti-ddrss-core.c b/drivers/mfd/ti-ddrss-core.c
> new file mode 100644
> index 0000000000000..a12a54e3461e1
> --- /dev/null
> +++ b/drivers/mfd/ti-ddrss-core.c
> @@ -0,0 +1,280 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/*
> + * ti-ddrss-core.c -- TI DDR Subsystem MFD core driver
No filenames please - they have a habit of bit-rotting.
No such thing as an "MFD core driver", please describe the device.
> + *
> + * Copyright (C) 2026 Texas Instruments Incorporated - https://www.ti.com/
> + */
> +
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/ti-ddrss.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_address.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +
> +/* J7 Register offsets */
> +#define J7_INT_STAT 0x494
> +#define J7_INT_ACK 0x49C
> +#define J7_INT_MASK 0x4A4
> +#define J7_TEMP_REG0 0x288
> +#define J7_TEMP_REG1 0x28C
> +
> +/* AM62 Register offsets */
> +#define AM62_INT_STAT_MASTER 0x538
> +#define AM62_INT_MASK_MASTER 0x53C
> +#define AM62_INT_MASK_MISC 0x594
> +#define AM62_INT_STAT_MISC 0x554
> +#define AM62_INT_ACK_MISC 0x574
> +#define AM62_TEMP_REG0 0x2F8
> +#define AM62_TEMP_REG1 0x2FC
> +
> +/* AM62A Register offsets */
> +#define AM62A_INT_STAT_MASTER 0x558
> +#define AM62A_INT_MASK_MASTER 0x55C
> +#define AM62A_INT_MASK_MISC 0x5B4
> +#define AM62A_INT_STAT_MISC 0x574
> +#define AM62A_INT_ACK_MISC 0x594
> +#define AM62A_TEMP_REG0 0x304
> +#define AM62A_TEMP_REG1 0x308
> +
> +/*
> + * tRAS_MAX / tREF register offsets (byte offset = CTL_N * 4)
> + *
> + * J7: TRAS_MAX F0/F1/F2 = CTL_44/46/48, 17-bit [16:0]
> + * TREF F0/F1/F2 = CTL_61/63/65, 20-bit [19:0]
> + * AM62/64: TRAS_MAX F0/F1/F2 = CTL_55/58/61, 20-bit [19:0]
> + * TREF F0/F1/F2 = CTL_73/75/77, 20-bit [19:0]
> + * AM62A/P: TRAS_MAX F0/F1/F2 = CTL_57/60/63, 20-bit [19:0]
> + * TREF F0/F1/F2 = CTL_75/77/79, 20-bit [19:0]
> + */
> +#define J7_TRAS_MAX_F0 0x0B0
> +#define J7_TRAS_MAX_F1 0x0B8
> +#define J7_TRAS_MAX_F2 0x0C0
> +#define J7_TREF_F0 0x0F4
> +#define J7_TREF_F1 0x0FC
> +#define J7_TREF_F2 0x104
> +
> +#define AM62_TRAS_MAX_F0 0x0DC
> +#define AM62_TRAS_MAX_F1 0x0E8
> +#define AM62_TRAS_MAX_F2 0x0F4
> +#define AM62_TREF_F0 0x124
> +#define AM62_TREF_F1 0x12C
> +#define AM62_TREF_F2 0x134
> +
> +#define AM62A_TRAS_MAX_F0 0x0E4
> +#define AM62A_TRAS_MAX_F1 0x0F0
> +#define AM62A_TRAS_MAX_F2 0x0FC
> +#define AM62A_TREF_F0 0x12C
> +#define AM62A_TREF_F1 0x134
> +#define AM62A_TREF_F2 0x13C
> +
> +static const struct reg_field j7_reg[] = {
> + /* J7: single bit (28) covers both group and TUF - no separate masking levels */
> + [K3_DDR_INT_STAT_MASTER] = REG_FIELD(J7_INT_STAT, 28, 28),
> + [K3_DDR_INT_MASK_MASTER_MISC] = REG_FIELD(J7_INT_MASK, 28, 28),
> + [K3_DDR_INT_MASK_MASTER_GLOBAL] = { 0 }, /* not applicable for J7 */
> + [K3_DDR_INT_MASK_TUF] = { 0 }, /* not applicable for J7 */
> + [K3_DDR_INT_STAT_TUF] = REG_FIELD(J7_INT_STAT, 28, 28),
> + [K3_DDR_INT_ACK_TUF] = REG_FIELD(J7_INT_ACK, 28, 28),
Why this step? It looks odd. Line them all up or none please.
> + [K3_DDR_TEMP_REG0_FIELD0] = REG_FIELD(J7_TEMP_REG0, 8, 10),
> + [K3_DDR_TEMP_REG0_FIELD1] = REG_FIELD(J7_TEMP_REG0, 16, 18),
> + [K3_DDR_TEMP_REG1_FIELD0] = REG_FIELD(J7_TEMP_REG1, 0, 2),
> + [K3_DDR_TEMP_REG1_FIELD1] = REG_FIELD(J7_TEMP_REG1, 8, 10),
> + [K3_DDR_TRAS_MAX_F0] = REG_FIELD(J7_TRAS_MAX_F0, 0, 16),
> + [K3_DDR_TRAS_MAX_F1] = REG_FIELD(J7_TRAS_MAX_F1, 0, 16),
> + [K3_DDR_TRAS_MAX_F2] = REG_FIELD(J7_TRAS_MAX_F2, 0, 16),
> + [K3_DDR_TREF_F0] = REG_FIELD(J7_TREF_F0, 0, 19),
> + [K3_DDR_TREF_F1] = REG_FIELD(J7_TREF_F1, 0, 19),
> + [K3_DDR_TREF_F2] = REG_FIELD(J7_TREF_F2, 0, 19),
> +};
> +
> +static const struct reg_field am62_reg[] = {
> + [K3_DDR_INT_STAT_MASTER] = REG_FIELD(AM62_INT_STAT_MASTER, 7, 7),
> + [K3_DDR_INT_MASK_MASTER_MISC] = REG_FIELD(AM62_INT_MASK_MASTER, 7, 7),
> + [K3_DDR_INT_MASK_MASTER_GLOBAL] = REG_FIELD(AM62_INT_MASK_MASTER, 31, 31),
> + [K3_DDR_INT_MASK_TUF] = REG_FIELD(AM62_INT_MASK_MISC, 5, 5),
> + [K3_DDR_INT_STAT_TUF] = REG_FIELD(AM62_INT_STAT_MISC, 5, 5),
> + [K3_DDR_INT_ACK_TUF] = REG_FIELD(AM62_INT_ACK_MISC, 5, 5),
> + [K3_DDR_TEMP_REG0_FIELD0] = REG_FIELD(AM62_TEMP_REG0, 24, 26),
> + [K3_DDR_TEMP_REG0_FIELD1] = REG_FIELD(AM62_TEMP_REG0, 28, 30),
> + [K3_DDR_TEMP_REG1_FIELD0] = REG_FIELD(AM62_TEMP_REG1, 0, 2),
> + [K3_DDR_TEMP_REG1_FIELD1] = REG_FIELD(AM62_TEMP_REG1, 4, 6),
> + [K3_DDR_TRAS_MAX_F0] = REG_FIELD(AM62_TRAS_MAX_F0, 0, 19),
> + [K3_DDR_TRAS_MAX_F1] = REG_FIELD(AM62_TRAS_MAX_F1, 0, 19),
> + [K3_DDR_TRAS_MAX_F2] = REG_FIELD(AM62_TRAS_MAX_F2, 0, 19),
> + [K3_DDR_TREF_F0] = REG_FIELD(AM62_TREF_F0, 0, 19),
> + [K3_DDR_TREF_F1] = REG_FIELD(AM62_TREF_F1, 0, 19),
> + [K3_DDR_TREF_F2] = REG_FIELD(AM62_TREF_F2, 0, 19),
> +};
> +
> +static const struct reg_field am62a_reg[] = {
> + [K3_DDR_INT_STAT_MASTER] = REG_FIELD(AM62A_INT_STAT_MASTER, 7, 7),
> + [K3_DDR_INT_MASK_MASTER_MISC] = REG_FIELD(AM62A_INT_MASK_MASTER, 7, 7),
> + [K3_DDR_INT_MASK_MASTER_GLOBAL] = REG_FIELD(AM62A_INT_MASK_MASTER, 31, 31),
> + [K3_DDR_INT_MASK_TUF] = REG_FIELD(AM62A_INT_MASK_MISC, 5, 5),
> + [K3_DDR_INT_STAT_TUF] = REG_FIELD(AM62A_INT_STAT_MISC, 5, 5),
> + [K3_DDR_INT_ACK_TUF] = REG_FIELD(AM62A_INT_ACK_MISC, 5, 5),
> + [K3_DDR_TEMP_REG0_FIELD0] = REG_FIELD(AM62A_TEMP_REG0, 8, 10),
> + [K3_DDR_TEMP_REG0_FIELD1] = REG_FIELD(AM62A_TEMP_REG0, 12, 14),
> + [K3_DDR_TEMP_REG1_FIELD0] = REG_FIELD(AM62A_TEMP_REG1, 0, 2),
> + [K3_DDR_TEMP_REG1_FIELD1] = REG_FIELD(AM62A_TEMP_REG1, 4, 6),
> + [K3_DDR_TRAS_MAX_F0] = REG_FIELD(AM62A_TRAS_MAX_F0, 0, 19),
> + [K3_DDR_TRAS_MAX_F1] = REG_FIELD(AM62A_TRAS_MAX_F1, 0, 19),
> + [K3_DDR_TRAS_MAX_F2] = REG_FIELD(AM62A_TRAS_MAX_F2, 0, 19),
> + [K3_DDR_TREF_F0] = REG_FIELD(AM62A_TREF_F0, 0, 19),
> + [K3_DDR_TREF_F1] = REG_FIELD(AM62A_TREF_F1, 0, 19),
> + [K3_DDR_TREF_F2] = REG_FIELD(AM62A_TREF_F2, 0, 19),
> +};
> +
> +static const struct k3_ddr_cfg j7_cfg = {
> + .cfg_fields = j7_reg,
Make it obvious that this is an array, else it looks like a value.
j7_registers would be better. Same below.
> + .has_intr_group = false,
> + .identifier = "J7",
> +};
> +
> +static const struct k3_ddr_cfg am62_cfg = {
> + .cfg_fields = am62_reg,
> + .has_intr_group = true,
> + .has_mask_misc = true,
> + .identifier = "AM62",
> +};
> +
> +static const struct k3_ddr_cfg am62a_cfg = {
> + .cfg_fields = am62a_reg,
> + .has_intr_group = true,
> + .has_mask_misc = true,
> + .identifier = "AM62A",
> +};
> +
> +static const struct k3_ddr_cfg am64_cfg = {
> + .cfg_fields = am62_reg,
> + .has_intr_group = true,
> + .has_mask_misc = true,
> + .identifier = "AM64",
> +};
> +
> +static const struct k3_ddr_cfg am62p_cfg = {
> + .cfg_fields = am62a_reg,
> + .has_intr_group = true,
> + .has_mask_misc = true,
> + .identifier = "AM62P",
> +};
> +
> +static const struct regmap_config ti_ddrss_regmap_config = {
> + .reg_bits = 32,
> + .val_bits = 32,
> + .reg_stride = 4,
> + .fast_io = true,
> +};
> +
> +static const struct mfd_cell ti_ddrss_cells[] = {
> + MFD_CELL_NAME("ti-ddrss-mr4"),
> + MFD_CELL_OF("ti-k3-ddr-pmu", NULL, NULL, 0, 0, "ti,k3-ddr-pmu"),
> +};
> +
> +static int ti_ddrss_probe(struct platform_device *pdev)
> +{
> + struct device_node *child_np, *pmu_np;
> + struct ti_ddrss_dev *ddrss;
_dev is confusing. It should be _ddata.
Then call the variable dddata and we'll all know what this is.
> + struct device *dev = &pdev->dev;
> + struct resource res;
> + void __iomem *base;
> + int irq, ncells, ret;
> +
> + ddrss = devm_kzalloc(dev, sizeof(*ddrss), GFP_KERNEL);
> + if (!ddrss)
> + return -ENOMEM;
> +
> + ddrss->dev = dev;
> + ddrss->cfg = device_get_match_data(dev);
> + if (!ddrss->cfg)
> + return dev_err_probe(dev, -ENODEV, "No match data found\n");
> +
> + child_np = of_get_child_by_name(dev->of_node, "ddr");
> + if (!child_np)
> + return dev_err_probe(dev, -ENODEV, "ddr child node not found\n");
This isn't a very user-friendly error message. What's 'ddr'?
> + ret = of_address_to_resource(child_np, 0, &res);
> + if (ret) {
> + of_node_put(child_np);
> + return dev_err_probe(dev, ret, "Failed to get register address\n");
> + }
> +
> + base = devm_ioremap_resource(dev, &res);
> + of_node_put(child_np);
> +
> + if (IS_ERR(base))
> + return PTR_ERR(base);
Are we assuming that this is -ENOMEM? Nothing else possible?
> + ddrss->base = base;
Why use the local variable at all?
> + ddrss->sscfg = devm_platform_ioremap_resource_byname(pdev, "ss_cfg");
> + if (IS_ERR(ddrss->sscfg))
> + return dev_err_probe(dev, PTR_ERR(ddrss->sscfg),
> + "Failed to map SSCFG registers\n");
devm_platform_ioremap_resource_byname() should already spit out an error log.
> + ddrss->regmap = devm_regmap_init_mmio(dev, base, &ti_ddrss_regmap_config);
> + if (IS_ERR(ddrss->regmap))
> + return dev_err_probe(dev, PTR_ERR(ddrss->regmap), "Failed to init regmap\n");
> +
> + irq = platform_get_irq(pdev, 0);
> + if (irq < 0)
> + return irq;
> +
> + ddrss->irq = irq;
As above.
> + ret = devm_pm_runtime_enable(dev);
> + if (ret)
> + return ret;
> +
> + pm_runtime_get_noresume(dev);
> +
> + platform_set_drvdata(pdev, ddrss);
> +
> + pmu_np = of_get_child_by_name(dev->of_node, "pmu");
> + ncells = pmu_np ? ARRAY_SIZE(ti_ddrss_cells) : 1;
Deserves a comment.
> + of_node_put(pmu_np);
> +
> + ret = devm_mfd_add_devices(dev, PLATFORM_DEVID_AUTO,
> + ti_ddrss_cells, ncells, NULL, 0, NULL);
> + if (ret) {
> + pm_runtime_put_noidle(dev);
> + return dev_err_probe(dev, ret, "Failed to register child devices\n");
> + }
> +
> + return 0;
> +}
> +
> +static void ti_ddrss_remove(struct platform_device *pdev)
> +{
> + pm_runtime_put_noidle(&pdev->dev);
> +}
devm_add_action_or_reset()?
> +static const struct of_device_id ti_ddrss_of_match[] = {
> + { .compatible = "ti,j721s2-ddrss", .data = &j7_cfg },
> + { .compatible = "ti,j721e-ddrss", .data = &j7_cfg },
> + { .compatible = "ti,j7-ddrss", .data = &j7_cfg },
> + { .compatible = "ti,am62-ddrss", .data = &am62_cfg },
> + { .compatible = "ti,am62a-ddrss", .data = &am62a_cfg },
> + { .compatible = "ti,am64-ddrss", .data = &am64_cfg },
> + { .compatible = "ti,am62p-ddrss", .data = &am62p_cfg },
Tab?
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, ti_ddrss_of_match);
> +
> +static struct platform_driver ti_ddrss_driver = {
> + .driver = {
> + .name = "ti-ddrss",
> + .of_match_table = ti_ddrss_of_match,
> + },
> + .probe = ti_ddrss_probe,
> + .remove = ti_ddrss_remove,
> +};
> +
Nit: Remove this line please.
> +module_platform_driver(ti_ddrss_driver);
> +
> +MODULE_DESCRIPTION("TI K3 DDR Subsystem core driver");
> +MODULE_AUTHOR("Texas Instruments Inc");
That's not what this is for.
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/mfd/ti-ddrss.h b/include/linux/mfd/ti-ddrss.h
> new file mode 100644
> index 0000000000000..32ea3527978e0
> --- /dev/null
> +++ b/include/linux/mfd/ti-ddrss.h
> @@ -0,0 +1,53 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +/*
> + * ti-ddrss.h -- TI DDR Subsystem MFD device header
Filenames.
> + * Copyright (C) 2026 Texas Instruments Incorporated - https://www.ti.com/
> + */
> +
> +#ifndef __LINUX_MFD_TI_DDRSS_H
> +#define __LINUX_MFD_TI_DDRSS_H
> +
> +#include <linux/regmap.h>
> +
> +enum k3_ddr_fields {
> + K3_DDR_INT_STAT_MASTER,
> + K3_DDR_INT_MASK_MASTER_MISC, /* J7: combined bit; AM62/AM62A: MISC group */
> + K3_DDR_INT_MASK_MASTER_GLOBAL, /* top-level enable - AM62/AM62A only */
> + K3_DDR_INT_MASK_TUF, /* TUF bit in MISC mask - AM62/AM62A only */
> + K3_DDR_INT_STAT_TUF,
> + K3_DDR_INT_ACK_TUF,
> + K3_DDR_TEMP_REG0_FIELD0,
> + K3_DDR_TEMP_REG0_FIELD1,
> + K3_DDR_TEMP_REG1_FIELD0,
> + K3_DDR_TEMP_REG1_FIELD1,
> + /* tRAS_MAX and tREF for the three DDR Frequency Set Points.
> + * Must stay contiguous in this order: ti-ddrss-mr4.c uses
> + * K3_DDR_TRAS_MAX_F0+i and K3_DDR_TREF_F0+i to iterate FSPs.
> + */
This is not a properly formatted multi-line comment.
> + K3_DDR_TRAS_MAX_F0,
> + K3_DDR_TRAS_MAX_F1,
> + K3_DDR_TRAS_MAX_F2,
> + K3_DDR_TREF_F0,
> + K3_DDR_TREF_F1,
> + K3_DDR_TREF_F2,
> + K3_DDR_MAX_FIELDS
> +};
> +
> +struct k3_ddr_cfg {
> + const struct reg_field *cfg_fields;
> + bool has_intr_group; /* AM62/AM62A: separate group status register */
> + bool has_mask_misc; /* AM62/AM62A: three-level interrupt unmasking */
> + const char *identifier;
> +};
> +
> +struct ti_ddrss_dev {
> + struct device *dev;
> + struct regmap *regmap;
> + int irq;
> + const struct k3_ddr_cfg *cfg;
> + void __iomem *base; /* DDR controller base (regmap) */
> + void __iomem *sscfg; /* SSCFG base (PMU counters live here) */
> +};
> +
> +#endif
> --
> 2.34.1
>
--
Lee Jones
prev parent reply other threads:[~2026-07-23 12:58 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-07-14 12:55 [RFC PATCH 05/22] mfd: ti-ddrss: Add TI K3 DDR subsystem MFD core driver MANNURU VENKATESWARLU
2026-07-23 12:58 ` Lee Jones [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20260723125800.GJ3363113@google.com \
--to=lee@kernel.org \
--cc=bb@ti.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=mfd@lists.linux.dev \
--cc=n-francis@ti.com \
--cc=s-k6@ti.com \
--cc=v-mannuru@ti.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox