Re: [PATCH 2/2] soc: ti: k3-socinfo: Add support for AM62P variants via NVMEM

[Judith Mendez <jm@ti.com>]

Hi Andrew,

On 2/4/26 3:54 PM, Andrew Davis wrote:
> On 2/4/26 3:37 PM, Judith Mendez wrote:
>> Add support for detecting AM62P silicon revisions.
>>
>> On AM62P, silicon revision is discovered with GP_SW1 register instead
>> of JTAGID register. Use the NVMEM framework to read GP_SW1 from the
>> gpsw-efuse nvmem provider to determine SoC revision.
>>
>> Signed-off-by: Judith Mendez <jm@ti.com>
>> ---
>>   drivers/soc/ti/k3-socinfo.c | 48 ++++++++++++++++++++++++++++++++++---
>>   1 file changed, 45 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/soc/ti/k3-socinfo.c b/drivers/soc/ti/k3-socinfo.c
>> index 42275cb5ba1c8..4b6947a9ceb4d 100644
>> --- a/drivers/soc/ti/k3-socinfo.c
>> +++ b/drivers/soc/ti/k3-socinfo.c
>> @@ -6,6 +6,7 @@
>>    */
>>   #include <linux/mfd/syscon.h>
>> +#include <linux/nvmem-consumer.h>
>>   #include <linux/of.h>
>>   #include <linux/of_address.h>
>>   #include <linux/regmap.h>
>> @@ -25,6 +26,9 @@
>>   #define CTRLMMR_WKUP_JTAGID_VARIANT_SHIFT    (28)
>>   #define CTRLMMR_WKUP_JTAGID_VARIANT_MASK    GENMASK(31, 28)
>> +#define GP_SW1_VALID_BIT            BIT(4)
>> +#define GP_SW1_ADR_MASK            GENMASK(3, 0)
>> +
>>   #define CTRLMMR_WKUP_JTAGID_PARTNO_SHIFT    (12)
>>   #define CTRLMMR_WKUP_JTAGID_PARTNO_MASK        GENMASK(27, 12)
>> @@ -70,6 +74,29 @@ static const char * const am62lx_rev_string_map[] = {
>>       "1.0", "1.1",
>>   };
>> +static const char * const am62p_gpsw_rev_string_map[] = {
>> +    "1.0", "1.1", "1.2",
>> +};
>> +
>> +static int
>> +k3_chipinfo_get_gpsw_variant(struct platform_device *pdev)
>> +{
>> +    struct device *dev = &pdev->dev;
>> +    u32 gpsw_val, adr_val = 0;
>> +    int ret;
>> +
>> +    ret = nvmem_cell_read_u32(dev, "gpsw1", &gpsw_val);
>> +    if (ret)
>> +        return ret;
>> +
>> +    if (!(gpsw_val & GP_SW1_VALID_BIT))
>> +        return 0;
> 
> Return -1 here so you will get the warning message about setting default 
> SR1.0.
> 
>> +
>> +    adr_val = gpsw_val & GP_SW1_ADR_MASK;
>> +
>> +    return adr_val;
> 
> Merge the above two lines,
> 
> return gpsw_val & GP_SW1_ADR_MASK;
> 
> Or maybe try using FIELD_GET() or similar if you are feeling fancy.
> 
>> +}
>> +
>>   static int
>>   k3_chipinfo_partno_to_names(unsigned int partno,
>>                   struct soc_device_attribute *soc_dev_attr)
>> @@ -86,9 +113,11 @@ k3_chipinfo_partno_to_names(unsigned int partno,
>>   }
>>   static int
>> -k3_chipinfo_variant_to_sr(unsigned int partno, unsigned int variant,
>> -              struct soc_device_attribute *soc_dev_attr)
>> +k3_chipinfo_variant_to_sr(struct platform_device *pdev, unsigned int 
>> partno,
> 
> You pass in the platform_device struct pointer, but only ever use the
> ->dev member, just pass in the "dev" device pointer.
> 
> Otherwise LGTM
> 
> Andrew
> 

Thanks for reviewing!
Will fix according to your review and respin the series.

~ Judith