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

[Judith Mendez <jm@ti.com>]

Hi Andrew,

On 8/8/25 8:50 AM, Andrew Davis wrote:
> On 8/7/25 5:51 PM, Judith Mendez wrote:
>> This adds a support for detecting AM62P SR1.0, SR1.1, SR1.2.
>>
>> On AM62P, silicon revision is discovered with GP_SW1 instead of JTAGID
>> register, so read GP_SW1 to determine SoC revision only on AM62P.
>>
>> Signed-off-by: Judith Mendez <jm@ti.com>
>> ---
>>   drivers/soc/ti/k3-socinfo.c | 27 +++++++++++++++++++++++++--
>>   1 file changed, 25 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/soc/ti/k3-socinfo.c b/drivers/soc/ti/k3-socinfo.c
>> index d716be113c84..81d078f15cd2 100644
>> --- a/drivers/soc/ti/k3-socinfo.c
>> +++ b/drivers/soc/ti/k3-socinfo.c
>> @@ -15,6 +15,9 @@
>>   #include <linux/sys_soc.h>
>>   #define CTRLMMR_WKUP_JTAGID_REG        0
>> +#define CTRLMMR_WKUP_GP_SW1_OFFSET        544
>> +#define GP_SW1_MOD_OPR                16
>> +
>>   /*
>>    * Bits:
>>    *  31-28 VARIANT    Device variant
>> @@ -66,6 +69,10 @@ static const char * const j721e_rev_string_map[] = {
>>       "1.0", "1.1", "2.0",
>>   };
>> +static const char * const am62p_gpsw_rev_string_map[] = {
>> +    "1.0", "1.1", "1.2",
>> +};
>> +
>>   static int
>>   k3_chipinfo_partno_to_names(unsigned int partno,
>>                   struct soc_device_attribute *soc_dev_attr)
>> @@ -83,7 +90,7 @@ 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)
>> +              struct soc_device_attribute *soc_dev_attr, u32 gp_sw1)
>>   {
>>       switch (partno) {
>>       case JTAG_ID_PARTNO_J721E:
>> @@ -92,6 +99,14 @@ k3_chipinfo_variant_to_sr(unsigned int partno, 
>> unsigned int variant,
>>           soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%s",
>>                              j721e_rev_string_map[variant]);
>>           break;
>> +    case JTAG_ID_PARTNO_AM62PX:
>> +        /* Always parse AM62P variant from GP_SW1 */
>> +        variant = gp_sw1 % GP_SW1_MOD_OPR;
>> +        if (variant >= ARRAY_SIZE(am62p_gpsw_rev_string_map))
>> +            goto err_unknown_variant;
>> +        soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%s",
>> +                           am62p_gpsw_rev_string_map[variant]);
>> +        break;
>>       default:
>>           variant++;
>>           soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%x.0",
>> @@ -121,6 +136,7 @@ static int k3_chipinfo_probe(struct 
>> platform_device *pdev)
>>       struct soc_device *soc_dev;
>>       struct regmap *regmap;
>>       void __iomem *base;
>> +    u32 gp_sw1_val = 0;
>>       u32 partno_id;
>>       u32 variant;
>>       u32 jtag_id;
>> @@ -163,7 +179,14 @@ static int k3_chipinfo_probe(struct 
>> platform_device *pdev)
>>           goto err;
>>       }
>> -    ret = k3_chipinfo_variant_to_sr(partno_id, variant, soc_dev_attr);
>> +    if (partno_id == JTAG_ID_PARTNO_AM62PX) {
>> +        ret = regmap_read(regmap, CTRLMMR_WKUP_JTAGID_REG +
>> +                  CTRLMMR_WKUP_GP_SW1_OFFSET, &gp_sw1_val);
> 
> This is wrong, you cannot read from the register GP_SW1 (offset 544). In DT
> the region is 4 bytes long (reg = <0x14 0x4>;).
> 
> This only worked in your testing because the above ioremap_resource() 
> has to
> map in page sized(4K) chunks and we didn't set max_register in our 
> regmap_config
> so no bounds checking is done. If we fix either of the above then this read
> will stop working.
> 
> So yes you'll have to make DT changes and fight it out with Krzysztof. My
> suggestion is to make a efuse region for the 3 GPSW registers and use a
> phandle from this node to fetch the extra info you need to get revision.


Sure I also believe that is the correct implementation and is actually
the way I approached the issue in v1.

But it seems like Krzysztof will not take that implementation [0]
https://lore.kernel.org/linux-mmc/736f09e0-075a-48e0-9b32-6b8805a7ee2a@kernel.org/

so what to do then....

~ Judith