Re: [PATCH 3/3] soc: ti: Add TDA54 SCM configuration syscon support

[Andrew Davis <afd@ti.com>]

On 10/21/26 1:36 AM, Moteen Shah wrote:
> 
> On 19/10/26 13:37, Moteen Shah wrote:
>> Add the TDA54 SCM configuration driver providing access to the SCM register
>> space through regmap/syscon.
>>
>> The driver handles proxy-based register access and SCM unlock/claim
>> handling, and registers TDA54 SoC information using the common K3 SoC
>> info helper exported by k3-socinfo, for which the TDA54 part number is
>> added to the SoC ID table.
>>
>> Default the option to m on ARCH_K3, so that the driver gets built for TDA54
>> boards without requiring a defconfig change.
>>
>> Signed-off-by: Moteen Shah <m-shah@ti.com>
>> Signed-off-by: Aniket Limaye <a-limaye@ti.com>
>> ---
>>   drivers/soc/ti/Kconfig             |  16 +++
>>   drivers/soc/ti/Makefile            |   1 +
>>   drivers/soc/ti/k3-socinfo.c        |   2 +
>>   drivers/soc/ti/tda54-mcu-sysconf.c | 216 +++++++++++++++++++++++++++++
>>   4 files changed, 235 insertions(+)
>>   create mode 100644 drivers/soc/ti/tda54-mcu-sysconf.c
>>
>> diff --git a/drivers/soc/ti/Kconfig b/drivers/soc/ti/Kconfig
>> index 163aadd589d3a..ea5da6722da29 100644
>> --- a/drivers/soc/ti/Kconfig
>> +++ b/drivers/soc/ti/Kconfig
>> @@ -82,6 +82,22 @@ config TI_PRUSS
>>         processors on various TI SoCs. It's safe to say N here if you're
>>         not interested in the PRU or if you are unsure.
>> +config TI_TDA54_MCU_SYSCONF
>> +    tristate "TI TDA54 SCM configuration controller"
>> +    depends on ARCH_K3 || COMPILE_TEST
>> +    depends on OF
>> +    default m if ARCH_K3
>> +    select MFD_SYSCON
>> +    select TI_K3_SOCINFO
>> +    help
>> +      Support the SCM configuration controller found on TI TDA54.
>> +      The controller provides a syscon regmap and is also the
>> +      hardware access path used to obtain and register K3 SoC
>> +      identification information.
>> +
>> +      This driver can be built as a module, in which case the module
>> +      will be called tda54-mcu-sysconf.
>> +
>>   endif # SOC_TI
>>   config TI_SCI_INTA_MSI_DOMAIN
>> diff --git a/drivers/soc/ti/Makefile b/drivers/soc/ti/Makefile
>> index cb800a745e66d..33f4ddca32ed6 100644
>> --- a/drivers/soc/ti/Makefile
>> +++ b/drivers/soc/ti/Makefile
>> @@ -11,4 +11,5 @@ obj-$(CONFIG_TI_SCI_INTA_MSI_DOMAIN)    += ti_sci_inta_msi.o
>>   obj-$(CONFIG_TI_K3_RINGACC)        += k3-ringacc.o
>>   obj-$(CONFIG_TI_K3_SOCINFO)        += k3-socinfo.o
>>   obj-$(CONFIG_TI_PRUSS)            += pruss.o
>> +obj-$(CONFIG_TI_TDA54_MCU_SYSCONF)    += tda54-mcu-sysconf.o
>>   obj-$(CONFIG_POWER_AVS_OMAP)        += smartreflex.o
>> diff --git a/drivers/soc/ti/k3-socinfo.c b/drivers/soc/ti/k3-socinfo.c
>> index 5fae03004e350..9869dd6358d1f 100644
>> --- a/drivers/soc/ti/k3-socinfo.c
>> +++ b/drivers/soc/ti/k3-socinfo.c
>> @@ -50,6 +50,7 @@
>>   #define JTAG_ID_PARTNO_AM62PX        0xBB9D
>>   #define JTAG_ID_PARTNO_J722S        0xBBA0
>>   #define JTAG_ID_PARTNO_AM62LX        0xBBA7
>> +#define JTAG_ID_PARTNO_TDA54        0xBBC1
>>   static const struct k3_soc_id {
>>       unsigned int id;
>> @@ -66,6 +67,7 @@ static const struct k3_soc_id {
>>       { JTAG_ID_PARTNO_AM62PX, "AM62PX" },
>>       { JTAG_ID_PARTNO_J722S, "J722S" },
>>       { JTAG_ID_PARTNO_AM62LX, "AM62LX" },
>> +    { JTAG_ID_PARTNO_TDA54, "TDA54" },
>>   };
>>   static const char * const j721e_rev_string_map[] = {
>> diff --git a/drivers/soc/ti/tda54-mcu-sysconf.c b/drivers/soc/ti/tda54-mcu-sysconf.c
>> new file mode 100644
>> index 0000000000000..8598080f6c48e
>> --- /dev/null
>> +++ b/drivers/soc/ti/tda54-mcu-sysconf.c
>> @@ -0,0 +1,216 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * TDA54 SCM Configuration syscon driver
>> + *
>> + * Copyright (C) 2026 Texas Instruments Incorporated - https://www.ti.com/
>> + */
>> +#include <linux/bitfield.h>
>> +#include <linux/bitops.h>
>> +#include <linux/io.h>
>> +#include <linux/mfd/syscon.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/regmap.h>
>> +#include <linux/soc/ti/k3-socinfo.h>
>> +#include <linux/types.h>
>> +
>> +/* Size of a single proxy window into the scm_conf register space */
>> +#define TDA54_SCM_CONF_PROXY_STRIDE        0x4000
>> +
>> +#define TDA54_JTAG_ID_REG_OFFSET        0x14
>> +
>> +#define TDA54_SCM_CONF_LOCK_KICK0_UNLOCK_VAL    0x68EF3490
>> +#define TDA54_SCM_CONF_LOCK_KICK1_UNLOCK_VAL    0xD172BC5A
>> +
>> +/*
>> + * Claim registers start at 0x3100. Claim register i holds the claim bit
>> + * for each of the 32 registers in the (i * TDA54_SCM_CONF_CLAIM_BLOCK_SIZE)
>> + * .. (i * TDA54_SCM_CONF_CLAIM_BLOCK_SIZE + 0x7c) block, one bit per
>> + * register, i.e. bit b of claim register i corresponds to the register at
>> + * offset i * TDA54_SCM_CONF_CLAIM_BLOCK_SIZE + b * 4.
>> + */
>> +#define TDA54_SCM_CONF_CLAIM_REG_STRIDE        0x4
>> +#define TDA54_SCM_CONF_CLAIM_REGS_PER_WORD    32
>> +#define TDA54_SCM_CONF_CLAIM_BLOCK_SIZE        \
>> +    (TDA54_SCM_CONF_CLAIM_REGS_PER_WORD * 0x4)
>> +
>> +/* Per-SoC register offset data (platform data) */
>> +struct tda54_scm_conf_data {
>> +    u32 lock_offset;
>> +    u32 claim_offset;
>> +    u32 claim_status_offset;
>> +};
>> +
>> +static const struct tda54_scm_conf_data tda54_scm_conf_data = {
>> +    .lock_offset        = 0x3008,
>> +    .claim_offset        = 0x3100,
>> +    .claim_status_offset    = 0x3300,
>> +};
>> +
>> +struct tda54_scm_conf {
>> +    void __iomem *base;
>> +    u32 proxy;
>> +    const struct tda54_scm_conf_data *pdata;
>> +};
>> +
>> +/*
>> + * Make @reg writable through this proxy.
>> + *
>> + * Proxy 0 is the default access proxy for all unclaimed registers. Its claim
>> + * bits are tied low, so it cannot claim anything and needs no claim to write;
>> + * it can write @reg unless another proxy has claimed it, which is reported by
>> + * the claim status.
>> + *
>> + * Any other proxy has to claim @reg first. Registers claimed by a proxy can no
>> + * longer be written through any other proxy's address, until that proxy
>> + * releases the claim. The claim status is set once any proxy owns @reg, so
>> + * read the claim register back to check that the claim is held by this proxy.
>> + */
>> +static int tda54_scm_conf_claim(struct tda54_scm_conf *scm, unsigned int reg)
>> +{
>> +    unsigned int block = reg / TDA54_SCM_CONF_CLAIM_BLOCK_SIZE;
>> +    unsigned int bit = (reg % TDA54_SCM_CONF_CLAIM_BLOCK_SIZE) /
>> +               TDA54_SCM_CONF_CLAIM_REG_STRIDE;
>> +    unsigned int claim_reg = scm->pdata->claim_offset +
>> +                 block * TDA54_SCM_CONF_CLAIM_REG_STRIDE;
>> +    unsigned int status_reg = scm->pdata->claim_status_offset +
>> +                  block * TDA54_SCM_CONF_CLAIM_REG_STRIDE;
>> +    u32 val;
>> +
>> +    if (!scm->proxy) {
>> +        if (readl(scm->base + status_reg) & BIT(bit))
>> +            return -EBUSY;
>> +
>> +        return 0;
>> +    }
>> +
>> +    val = readl(scm->base + claim_reg);
>> +    val |= BIT(bit);
>> +    writel(val, scm->base + claim_reg);
>> +
>> +    if (!(readl(scm->base + claim_reg) & BIT(bit)))
>> +        return -EBUSY;
>> +
>> +    return 0;
>> +}
>> +
>> +static int tda54_scm_conf_reg_read(void *context, unsigned int reg,
>> +                   unsigned int *val)
>> +{
>> +    struct tda54_scm_conf *scm = context;
>> +
>> +    *val = readl(scm->base + reg);
>> +
>> +    return 0;
>> +}
>> +
>> +static int tda54_scm_conf_reg_write(void *context, unsigned int reg,
>> +                    unsigned int val)
>> +{
>> +    struct tda54_scm_conf *scm = context;
>> +    int ret;
>> +
>> +    ret = tda54_scm_conf_claim(scm, reg);
>> +    if (ret)
>> +        return ret;
>> +
>> +    writel(val, scm->base + reg);
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct regmap_config tda54_scm_conf_regmap_cfg = {
>> +    .reg_bits = 32,
>> +    .val_bits = 32,
>> +    .reg_stride = 4,
>> +    .reg_read = tda54_scm_conf_reg_read,
>> +    .reg_write = tda54_scm_conf_reg_write,
>> +};
>> +
>> +static int tda54_scm_conf_register_socinfo(struct device *dev,
>> +                       struct regmap *regmap)
>> +{
>> +    u32 jtag_id;
>> +    int ret;
>> +
>> +    ret = regmap_read(regmap, TDA54_JTAG_ID_REG_OFFSET, &jtag_id);
>> +    if (ret)
>> +        return dev_err_probe(dev, ret, "failed to read JTAG ID\n");
>> +
>> +    return k3_socinfo_register(dev, jtag_id);
>> +}
>> +
>> +static int tda54_scm_conf_probe(struct platform_device *pdev)
>> +{
>> +    struct device *dev = &pdev->dev;
>> +    struct tda54_scm_conf *scm;
>> +    struct regmap *regmap;
>> +    void __iomem *base;
>> +    int ret;
>> +
>> +    scm = devm_kzalloc(dev, sizeof(*scm), GFP_KERNEL);
>> +    if (!scm)
>> +        return -ENOMEM;
>> +
>> +    base = devm_platform_ioremap_resource(pdev, 0);
>> +    if (IS_ERR(base))
>> +        return PTR_ERR(base);
>> +
>> +    if (of_property_read_u32(dev->of_node, "ti,proxy-id", &scm->proxy))
>> +        return dev_err_probe(dev, -ENODATA, "missing ti,proxy-id property\n");
>> +
>> +    if (scm->proxy >= 8)
>> +        return dev_err_probe(dev, -EINVAL,
>> +                     "ti,proxy-id must be between 0 and 7\n");
>> +
>> +    scm->base = base + scm->proxy * TDA54_SCM_CONF_PROXY_STRIDE;
>> +
>> +    /* Get register offsets from driver data (matched by compatible) */
>> +    scm->pdata = of_device_get_match_data(dev);
>> +
>> +    regmap = devm_regmap_init(dev, NULL, scm, &tda54_scm_conf_regmap_cfg);
>> +    if (IS_ERR(regmap))
>> +        return PTR_ERR(regmap);
>> +
>> +    writel(TDA54_SCM_CONF_LOCK_KICK0_UNLOCK_VAL, scm->base + scm->pdata->lock_offset);
>> +    writel(TDA54_SCM_CONF_LOCK_KICK1_UNLOCK_VAL, scm->base + scm->pdata->lock_offset + 4);
>> +
>> +    /*
>> +     * There is no way to unregister a syscon regmap, so publish it only
>> +     * once nothing else in probe can fail.
>> +     */
>> +    ret = of_syscon_register_regmap(dev->of_node, regmap);
>> +    if (ret)
>> +        return dev_err_probe(dev, ret, "failed to register syscon regmap\n");
>> +
>> +    /*
>> +     * SoC identification is informational, so a failure here must not fail
>> +     * the probe. It is registered last and released through devres.
>> +     */
>> +    ret = tda54_scm_conf_register_socinfo(dev, regmap);
>> +    if (ret)
>> +        dev_warn(dev, "SoC info not registered: %d\n", ret);
>> +
>> +    return 0;
>> +}
>> +
>> +static const struct of_device_id tda54_scm_conf_of_match[] = {
>> +    { .compatible = "ti,tda54-mcu-scm-conf", .data = &tda54_scm_conf_data },
>> +    { /* sentinel */ },
>> +};
>> +MODULE_DEVICE_TABLE(of, tda54_scm_conf_of_match);
>> +
>> +static struct platform_driver tda54_scm_conf_driver = {
>> +    .probe = tda54_scm_conf_probe,
>> +    .driver = {
>> +        .name = "k3-tda54-scm-conf",
>> +        .of_match_table = tda54_scm_conf_of_match,
>> +        .suppress_bind_attrs = true,
>> +    },
>> +};
>> +
>> +module_platform_driver(tda54_scm_conf_driver);
>> +
>> +MODULE_AUTHOR("Moteen Shah <m-shah@ti.com>");
>> +MODULE_DESCRIPTION("TDA54 SCM Configuration syscon driver");
>> +MODULE_LICENSE("GPL");
> 
> Hello all,
> 
> I was going by the reviews from Sashiko[0], I had similar concern before sharing this patch upstream.

And we discussed this during internal review of this series also (this
went to v10 on our internal review list, that is the one downside of
internal review, lots of earlier discussion context is lost/unavailable
when finally doing the public posting).

Anyway, to summarize we came to the conclusion that the issue is
valid, but not specific to this driver. The problem is the same
for any driver/module that exports pinned memory resources (dma-buf
exporting modules deal with the same). The solution will take work
at the framework level and be outside of the scope of this series.

> It would be really great if I can have some suggestions on this.
> 
> 1. Have it inbuilt
> 2. Remove the module_exit, make the module unremovable
> 3. Look towards a core syscon fixup?
> 
> Option 3 would be to add of_syscon_unregister_regmap(np) and subsequent consumer driver changes.
> 

That would be one part of this, but that doesn't solve the
core issue of what to do with consumers who have already
gotten a reference to the regmap which would still become
invalid after this module unregisters.

One options I see is to make sure that consumers using
device_node_to_regmap() causes a reference to the regmap
exporting driver to be upped. This way this module would
not be unloadable until all consumers have returned the
reference (a way for consumers to return the reference is
also needed).

Andrew

> [0]https://sashiko.dev/#/patchset/20261019080736.944127-1-m-shah%40ti.com
> 
> Regards,
> Moteen
>