Re: [PATCH 2/3] soc: ti: k3-socinfo: Export SoC registration helper

[Moteen Shah <m-shah@ti.com>]

On 19/10/26 23:08, Andrew Davis wrote:
> On 10/19/26 3:07 AM, Moteen Shah wrote:
>> The k3-socinfo driver reads the JTAG ID through a dedicated MMIO 
>> region and
>> then decodes it and registers the SoC device.
>>
>> Newer TI SoCs like TDA54 use a different path for accessing the JTAG ID
>> register, but the decoding of the JTAG ID into the SoC information 
>> remains
>> the same.
>>
>> Split the decode and registration part out of k3_chipinfo_probe() into
>> k3_socinfo_register() and export it, so that other drivers can 
>> register the
>> SoC device once they have read the JTAG ID through their own access 
>> path.
>>
>> Since the helper can now be called from modular drivers, it must not 
>> leak
>> the SoC device when the caller goes away. Tie the registration to the
>> calling device with a devm action, which unregisters the SoC device and
>> frees its attributes on unbind. Callers need no unregister 
>> counterpart and
>> the helper keeps returning an int. If the soc bus is not up yet, the
>> attributes are only parked for later registration and there is 
>> nothing to
>> undo, so no action is added in that case.
>>
>> Signed-off-by: Moteen Shah <m-shah@ti.com>
>> ---
>>   drivers/soc/ti/k3-socinfo.c       | 101 +++++++++++++++++++++++-------
>>   include/linux/soc/ti/k3-socinfo.h |  11 ++++
>>   2 files changed, 90 insertions(+), 22 deletions(-)
>>   create mode 100644 include/linux/soc/ti/k3-socinfo.h
>>
>> diff --git a/drivers/soc/ti/k3-socinfo.c b/drivers/soc/ti/k3-socinfo.c
>> index 48dbef3acceb4..5fae03004e350 100644
>> --- a/drivers/soc/ti/k3-socinfo.c
>> +++ b/drivers/soc/ti/k3-socinfo.c
>> @@ -5,6 +5,8 @@
>>    * Copyright (C) 2020 Texas Instruments Incorporated - 
>> http://www.ti.com
>>    */
>>   +#include <linux/device.h>
>> +#include <linux/export.h>
>>   #include <linux/mfd/syscon.h>
>>   #include <linux/nvmem-consumer.h>
>>   #include <linux/of.h>
>> @@ -12,6 +14,7 @@
>>   #include <linux/regmap.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/slab.h>
>> +#include <linux/soc/ti/k3-socinfo.h>
>>   #include <linux/string.h>
>>   #include <linux/sys_soc.h>
>>   @@ -152,38 +155,44 @@ k3_chipinfo_variant_to_sr(struct device *dev, 
>> unsigned int partno,
>>       return -ENODEV;
>>   }
>>   -static const struct regmap_config k3_chipinfo_regmap_cfg = {
>> -    .reg_bits = 32,
>> -    .val_bits = 32,
>> -    .reg_stride = 4,
>> +struct k3_socinfo {
>> +    struct soc_device *soc_dev;
>> +    struct soc_device_attribute *soc_dev_attr;
>>   };
>>   -static int k3_chipinfo_probe(struct platform_device *pdev)
>> +static void k3_socinfo_unregister(void *data)
>> +{
>> +    struct k3_socinfo *info = data;
>> +
>> +    soc_device_unregister(info->soc_dev);
>> +    /* The attributes must only be freed after the unregister. */
>> +    kfree(info->soc_dev_attr->revision);
>> +    kfree(info->soc_dev_attr);
>> +}
>> +
>> +/**
>> + * k3_socinfo_register() - Decode a JTAG ID and register the SoC device
>> + * @dev: device on whose behalf the SoC device is registered
>> + * @jtag_id: raw JTAG ID register value read by the caller
>> + *
>> + * The registration is managed by @dev: the SoC device and its 
>> attributes are
>> + * released automatically when @dev is unbound, so callers need no 
>> matching
>> + * unregister call. If the soc bus is not yet available, the 
>> registration is
>> + * deferred until it is and cannot be released by @dev.
>> + *
>> + * Return: 0 on success, negative errno on failure.
>> + */
>> +int k3_socinfo_register(struct device *dev, u32 jtag_id)
>>   {
>> -    struct device_node *node = pdev->dev.of_node;
>> +    struct device_node *node;
>> +    struct k3_socinfo *info;
>>       struct soc_device_attribute *soc_dev_attr;
>> -    struct device *dev = &pdev->dev;
>>       struct soc_device *soc_dev;
>> -    struct regmap *regmap;
>> -    void __iomem *base;
>>       u32 partno_id;
>>       u32 variant;
>> -    u32 jtag_id;
>>       u32 mfg;
>>       int ret;
>>   -    base = devm_platform_ioremap_resource(pdev, 0);
>> -    if (IS_ERR(base))
>> -        return PTR_ERR(base);
>> -
>> -    regmap = devm_regmap_init_mmio(dev, base, &k3_chipinfo_regmap_cfg);
>> -    if (IS_ERR(regmap))
>> -        return PTR_ERR(regmap);
>> -
>> -    ret = regmap_read(regmap, CTRLMMR_WKUP_JTAGID_REG, &jtag_id);
>> -    if (ret < 0)
>> -        return ret;
>> -
>>       mfg = (jtag_id & CTRLMMR_WKUP_JTAGID_MFG_MASK) >>
>>              CTRLMMR_WKUP_JTAGID_MFG_SHIFT;
>>   @@ -198,6 +207,10 @@ static int k3_chipinfo_probe(struct 
>> platform_device *pdev)
>>       partno_id = (jtag_id & CTRLMMR_WKUP_JTAGID_PARTNO_MASK) >>
>>            CTRLMMR_WKUP_JTAGID_PARTNO_SHIFT;
>>   +    info = devm_kzalloc(dev, sizeof(*info), GFP_KERNEL);
>> +    if (!info)
>> +        return -ENOMEM;
>> +
>>       soc_dev_attr = kzalloc_obj(*soc_dev_attr);
>>       if (!soc_dev_attr)
>>           return -ENOMEM;
>> @@ -224,6 +237,20 @@ static int k3_chipinfo_probe(struct 
>> platform_device *pdev)
>>           goto err_free_rev;
>>       }
>>   +    /*
>> +     * A NULL handle means the soc bus is not up yet and the attributes
>> +     * were parked for registration at soc bus init. They must stay
>> +     * allocated and cannot be unregistered, so there is nothing to 
>> undo.
>
> This seems really messy, the only time drivers need to deal with this
> early_soc_dev_attr hackyness is if they can register before the soc bus,
> which only happens for drivers using early_initcall(), but that is
> not the case for either driver using k3_socinfo_register().
>
> Can you explain why you added all this?


Hey Andrew,
A miss on my end, I was trying to cover for an edge case which won't 
happen in the way we are using the socinfo.

i will make it unconditional for V2, should I add a WARN_ON(!soc_dev), 
in maybe a case if some user in future does an early call anyhow?

Thanks for the review.

Regards,
Moteen

>
> Otherwise just allocate soc_dev_attr using devm, k3_socinfo_unregister()
> action can be added unconditionally, and only needs to call
> soc_device_unregister().
>
> Andrew
>
>> +     */
>> +    if (soc_dev) {
>> +        info->soc_dev = soc_dev;
>> +        info->soc_dev_attr = soc_dev_attr;
>> +        ret = devm_add_action_or_reset(dev, k3_socinfo_unregister,
>> +                           info);
>> +        if (ret)
>> +            return ret;
>> +    }
>> +
>>       dev_info(dev, "Family:%s rev:%s JTAGID[0x%08x] Detected\n",
>>            soc_dev_attr->family,
>>            soc_dev_attr->revision, jtag_id);
>> @@ -236,6 +263,36 @@ static int k3_chipinfo_probe(struct 
>> platform_device *pdev)
>>       kfree(soc_dev_attr);
>>       return ret;
>>   }
>> +EXPORT_SYMBOL_GPL(k3_socinfo_register);
>> +
>> +static const struct regmap_config k3_chipinfo_regmap_cfg = {
>> +    .reg_bits = 32,
>> +    .val_bits = 32,
>> +    .reg_stride = 4,
>> +};
>> +
>> +static int k3_chipinfo_probe(struct platform_device *pdev)
>> +{
>> +    struct device *dev = &pdev->dev;
>> +    struct regmap *regmap;
>> +    void __iomem *base;
>> +    u32 jtag_id;
>> +    int ret;
>> +
>> +    base = devm_platform_ioremap_resource(pdev, 0);
>> +    if (IS_ERR(base))
>> +        return PTR_ERR(base);
>> +
>> +    regmap = devm_regmap_init_mmio(dev, base, &k3_chipinfo_regmap_cfg);
>> +    if (IS_ERR(regmap))
>> +        return PTR_ERR(regmap);
>> +
>> +    ret = regmap_read(regmap, CTRLMMR_WKUP_JTAGID_REG, &jtag_id);
>> +    if (ret < 0)
>> +        return ret;
>> +
>> +    return k3_socinfo_register(dev, jtag_id);
>> +}
>>     static const struct of_device_id k3_chipinfo_of_match[] = {
>>       { .compatible = "ti,am654-chipid", },
>> diff --git a/include/linux/soc/ti/k3-socinfo.h 
>> b/include/linux/soc/ti/k3-socinfo.h
>> new file mode 100644
>> index 0000000000000..487e69d0069d1
>> --- /dev/null
>> +++ b/include/linux/soc/ti/k3-socinfo.h
>> @@ -0,0 +1,11 @@
>> +/* SPDX-License-Identifier: GPL-2.0 */
>> +#ifndef __LINUX_SOC_TI_K3_SOCINFO_H__
>> +#define __LINUX_SOC_TI_K3_SOCINFO_H__
>> +
>> +#include <linux/types.h>
>> +
>> +struct device;
>> +
>> +int k3_socinfo_register(struct device *dev, u32 jtag_id);
>> +
>> +#endif /* __LINUX_SOC_TI_K3_SOCINFO_H__ */
>