[PATCH] soc: ti: k3-socinfo: Fix the silicon revision misprint

[PATCH] soc: ti: k3-socinfo: Fix the silicon revision misprint

[Thejasvi Konduru]

For J721E PG1.1 the silicon revision is reported as 2.0 instead of
1.1. This is because the k3-socinfo.c code assumes the silicon revisions
are 1.0, 2.0 for every platform.

Fixed this by creating a separate list of silicon revisions for J721E.

Fixes: 907a2b7e2fc7 ("soc: ti: add k3 platforms chipid module driver")
Signed-off-by: Thejasvi Konduru <t-konduru@ti.com>
---
 drivers/soc/ti/k3-socinfo.c | 33 +++++++++++++++++++++++++--------
 1 file changed, 25 insertions(+), 8 deletions(-)

diff --git a/drivers/soc/ti/k3-socinfo.c b/drivers/soc/ti/k3-socinfo.c
index d15764e19d96..365bc37793a1 100644
--- a/drivers/soc/ti/k3-socinfo.c
+++ b/drivers/soc/ti/k3-socinfo.c
@@ -46,6 +46,8 @@ static const struct k3_soc_id {
 	{ 0xBB8D, "AM62AX" },
 };
 
+static char *soc_revision_j721e[] = {"1.0", "1.1"};
+
 static int
 k3_chipinfo_partno_to_names(unsigned int partno,
 			    struct soc_device_attribute *soc_dev_attr)
@@ -61,6 +63,21 @@ k3_chipinfo_partno_to_names(unsigned int partno,
 	return -EINVAL;
 }
 
+void
+k3_chipinfo_silicon_rev(unsigned int variant,
+			struct soc_device_attribute *soc_dev_attr)
+{
+	const char *family_name = soc_dev_attr->family;
+	int j721e_lookup_arr_size = ARRAY_SIZE(soc_revision_j721e);
+
+	if (!strcmp(family_name, "J721E") && variant < j721e_lookup_arr_size) {
+		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%s", soc_revision_j721e[variant]);
+	} else {
+		variant++;
+		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%x.0", variant);
+	}
+}
+
 static int k3_chipinfo_probe(struct platform_device *pdev)
 {
 	struct device_node *node = pdev->dev.of_node;
@@ -92,7 +109,6 @@ static int k3_chipinfo_probe(struct platform_device *pdev)
 
 	variant = (jtag_id & CTRLMMR_WKUP_JTAGID_VARIANT_MASK) >>
 		  CTRLMMR_WKUP_JTAGID_VARIANT_SHIFT;
-	variant++;
 
 	partno_id = (jtag_id & CTRLMMR_WKUP_JTAGID_PARTNO_MASK) >>
 		 CTRLMMR_WKUP_JTAGID_PARTNO_SHIFT;
@@ -101,17 +117,18 @@ static int k3_chipinfo_probe(struct platform_device *pdev)
 	if (!soc_dev_attr)
 		return -ENOMEM;
 
-	soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%x.0", variant);
-	if (!soc_dev_attr->revision) {
-		ret = -ENOMEM;
-		goto err;
-	}
-
 	ret = k3_chipinfo_partno_to_names(partno_id, soc_dev_attr);
 	if (ret) {
 		dev_err(dev, "Unknown SoC JTAGID[0x%08X]\n", jtag_id);
 		ret = -ENODEV;
-		goto err_free_rev;
+		goto err;
+	}
+
+	k3_chipinfo_silicon_rev(variant, soc_dev_attr);
+
+	if (!soc_dev_attr->revision) {
+		ret = -ENOMEM;
+		goto err;
 	}
 
 	node = of_find_node_by_path("/");
-- 
2.40.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] soc: ti: k3-socinfo: Fix the silicon revision misprint

[Nishanth Menon]

On 13:33-20230607, Thejasvi Konduru wrote:
> For J721E PG1.1 the silicon revision is reported as 2.0 instead of

There is no PG1.1. There is SR1.1

> 1.1. This is because the k3-socinfo.c code assumes the silicon revisions
> are 1.0, 2.0 for every platform.
> 
> Fixed this by creating a separate list of silicon revisions for J721E.

what we are doing is to add to the silicon revision detection.

> 
> Fixes: 907a2b7e2fc7 ("soc: ti: add k3 platforms chipid module driver")

This is'nt a fixes.

> Signed-off-by: Thejasvi Konduru <t-konduru@ti.com>
> ---
>  drivers/soc/ti/k3-socinfo.c | 33 +++++++++++++++++++++++++--------
>  1 file changed, 25 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/soc/ti/k3-socinfo.c b/drivers/soc/ti/k3-socinfo.c
> index d15764e19d96..365bc37793a1 100644
> --- a/drivers/soc/ti/k3-socinfo.c
> +++ b/drivers/soc/ti/k3-socinfo.c
> @@ -46,6 +46,8 @@ static const struct k3_soc_id {
>  	{ 0xBB8D, "AM62AX" },
>  };
>  
> +static char *soc_revision_j721e[] = {"1.0", "1.1"};
> +
>  static int
>  k3_chipinfo_partno_to_names(unsigned int partno,
>  			    struct soc_device_attribute *soc_dev_attr)
> @@ -61,6 +63,21 @@ k3_chipinfo_partno_to_names(unsigned int partno,
>  	return -EINVAL;
>  }
>  
> +void
> +k3_chipinfo_silicon_rev(unsigned int variant,
> +			struct soc_device_attribute *soc_dev_attr)
> +{
> +	const char *family_name = soc_dev_attr->family;
> +	int j721e_lookup_arr_size = ARRAY_SIZE(soc_revision_j721e);
> +
> +	if (!strcmp(family_name, "J721E") && variant < j721e_lookup_arr_size) {
> +		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%s", soc_revision_j721e[variant]);
> +	} else {
> +		variant++;
> +		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%x.0", variant);
> +	}

I am not comfortable with if else here. Why not extend k3_soc_id
structure to include the variant LuT? Are there exceptions to this rule
(Say AM65x?), those would make sense to handle with a compare against
the partno?

> +}
> +
>  static int k3_chipinfo_probe(struct platform_device *pdev)
>  {
>  	struct device_node *node = pdev->dev.of_node;
> @@ -92,7 +109,6 @@ static int k3_chipinfo_probe(struct platform_device *pdev)
>  
>  	variant = (jtag_id & CTRLMMR_WKUP_JTAGID_VARIANT_MASK) >>
>  		  CTRLMMR_WKUP_JTAGID_VARIANT_SHIFT;
> -	variant++;
>  
>  	partno_id = (jtag_id & CTRLMMR_WKUP_JTAGID_PARTNO_MASK) >>
>  		 CTRLMMR_WKUP_JTAGID_PARTNO_SHIFT;
> @@ -101,17 +117,18 @@ static int k3_chipinfo_probe(struct platform_device *pdev)
>  	if (!soc_dev_attr)
>  		return -ENOMEM;
>  
> -	soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%x.0", variant);
> -	if (!soc_dev_attr->revision) {
> -		ret = -ENOMEM;
> -		goto err;
> -	}
> -
>  	ret = k3_chipinfo_partno_to_names(partno_id, soc_dev_attr);
>  	if (ret) {
>  		dev_err(dev, "Unknown SoC JTAGID[0x%08X]\n", jtag_id);
>  		ret = -ENODEV;
> -		goto err_free_rev;
> +		goto err;
> +	}
> +
> +	k3_chipinfo_silicon_rev(variant, soc_dev_attr);
> +
> +	if (!soc_dev_attr->revision) {
> +		ret = -ENOMEM;

-ENOMEM? I dont see a alloc in the changes.

> +		goto err;
>  	}
>  
>  	node = of_find_node_by_path("/");
> -- 
> 2.40.1
> 

-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] soc: ti: k3-socinfo: Fix the silicon revision misprint

[Neha Malcom Francis]

Hi Nishanth

On 07/06/23 16:13, Nishanth Menon wrote:
> On 13:33-20230607, Thejasvi Konduru wrote:
>> For J721E PG1.1 the silicon revision is reported as 2.0 instead of
> 
> There is no PG1.1. There is SR1.1
> 
>> 1.1. This is because the k3-socinfo.c code assumes the silicon revisions
>> are 1.0, 2.0 for every platform.
>>
>> Fixed this by creating a separate list of silicon revisions for J721E.
> 
> what we are doing is to add to the silicon revision detection.
> 
>>
>> Fixes: 907a2b7e2fc7 ("soc: ti: add k3 platforms chipid module driver")
> 
> This is'nt a fixes.
> 
>> Signed-off-by: Thejasvi Konduru <t-konduru@ti.com>
>> ---
>>   drivers/soc/ti/k3-socinfo.c | 33 +++++++++++++++++++++++++--------
>>   1 file changed, 25 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/soc/ti/k3-socinfo.c b/drivers/soc/ti/k3-socinfo.c
>> index d15764e19d96..365bc37793a1 100644
>> --- a/drivers/soc/ti/k3-socinfo.c
>> +++ b/drivers/soc/ti/k3-socinfo.c
>> @@ -46,6 +46,8 @@ static const struct k3_soc_id {
>>   	{ 0xBB8D, "AM62AX" },
>>   };
>>   
>> +static char *soc_revision_j721e[] = {"1.0", "1.1"};
>> +
>>   static int
>>   k3_chipinfo_partno_to_names(unsigned int partno,
>>   			    struct soc_device_attribute *soc_dev_attr)
>> @@ -61,6 +63,21 @@ k3_chipinfo_partno_to_names(unsigned int partno,
>>   	return -EINVAL;
>>   }
>>   
>> +void
>> +k3_chipinfo_silicon_rev(unsigned int variant,
>> +			struct soc_device_attribute *soc_dev_attr)
>> +{
>> +	const char *family_name = soc_dev_attr->family;
>> +	int j721e_lookup_arr_size = ARRAY_SIZE(soc_revision_j721e);
>> +
>> +	if (!strcmp(family_name, "J721E") && variant < j721e_lookup_arr_size) {
>> +		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%s", soc_revision_j721e[variant]);
>> +	} else {
>> +		variant++;
>> +		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%x.0", variant);
>> +	}
> 
> I am not comfortable with if else here. Why not extend k3_soc_id
> structure to include the variant LuT? Are there exceptions to this rule
> (Say AM65x?), those would make sense to handle with a compare against
> the partno?
> 

Trying to revive this patch, I see what you are saying is similar to the way 
detection has already been implemented in U-Boot (drivers/soc/soc_ti_k3.c) if 
I'm not mistaken.

But I can't find any existing exception to this "family --> version" rule that 
forces us to use "partno --> version". Checked through all AM65x device TRMs 
available in ti.com; all seem to use common partno. So maybe I am not on the 
same page, did you mean something else?


>> +}
>> +
>>   static int k3_chipinfo_probe(struct platform_device *pdev)
>>   {
>>   	struct device_node *node = pdev->dev.of_node;
>> @@ -92,7 +109,6 @@ static int k3_chipinfo_probe(struct platform_device *pdev)
>>   
>>   	variant = (jtag_id & CTRLMMR_WKUP_JTAGID_VARIANT_MASK) >>
>>   		  CTRLMMR_WKUP_JTAGID_VARIANT_SHIFT;
>> -	variant++;
>>   
>>   	partno_id = (jtag_id & CTRLMMR_WKUP_JTAGID_PARTNO_MASK) >>
>>   		 CTRLMMR_WKUP_JTAGID_PARTNO_SHIFT;
>> @@ -101,17 +117,18 @@ static int k3_chipinfo_probe(struct platform_device *pdev)
>>   	if (!soc_dev_attr)
>>   		return -ENOMEM;
>>   
>> -	soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%x.0", variant);
>> -	if (!soc_dev_attr->revision) {
>> -		ret = -ENOMEM;
>> -		goto err;
>> -	}
>> -
>>   	ret = k3_chipinfo_partno_to_names(partno_id, soc_dev_attr);
>>   	if (ret) {
>>   		dev_err(dev, "Unknown SoC JTAGID[0x%08X]\n", jtag_id);
>>   		ret = -ENODEV;
>> -		goto err_free_rev;
>> +		goto err;
>> +	}
>> +
>> +	k3_chipinfo_silicon_rev(variant, soc_dev_attr);
>> +
>> +	if (!soc_dev_attr->revision) {
>> +		ret = -ENOMEM;
> 
> -ENOMEM? I dont see a alloc in the changes.
> 
>> +		goto err;
>>   	}
>>   
>>   	node = of_find_node_by_path("/");
>> -- 
>> 2.40.1
>>
> 

-- 
Thanking You
Neha Malcom Francis

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] soc: ti: k3-socinfo: Fix the silicon revision misprint

[Nishanth Menon]

On 12:07-20230912, Neha Malcom Francis wrote:

[...]

> > > +void
> > > +k3_chipinfo_silicon_rev(unsigned int variant,
> > > +			struct soc_device_attribute *soc_dev_attr)
> > > +{
> > > +	const char *family_name = soc_dev_attr->family;
> > > +	int j721e_lookup_arr_size = ARRAY_SIZE(soc_revision_j721e);
> > > +
> > > +	if (!strcmp(family_name, "J721E") && variant < j721e_lookup_arr_size) {
> > > +		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%s", soc_revision_j721e[variant]);
> > > +	} else {
> > > +		variant++;
> > > +		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%x.0", variant);
> > > +	}
> > 
> > I am not comfortable with if else here. Why not extend k3_soc_id
> > structure to include the variant LuT? Are there exceptions to this rule
> > (Say AM65x?), those would make sense to handle with a compare against
> > the partno?
> > 
> 
> Trying to revive this patch, I see what you are saying is similar to the way
> detection has already been implemented in U-Boot (drivers/soc/soc_ti_k3.c)
> if I'm not mistaken.

Yes.

> 
> But I can't find any existing exception to this "family --> version" rule
> that forces us to use "partno --> version". Checked through all AM65x device
> TRMs available in ti.com; all seem to use common partno. So maybe I am not
> on the same page, did you mean something else?

https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf
CTRLMMR_WKUP_JTAGID:: VARIANT field: SR2.0: 1h SR1.0: 0h
Latest data sheet: https://www.ti.com/lit/ds/symlink/am6548.pdf
indicates SR 2.1

How is this detected?

What I indicated is a LUT table similar to
https://git.ti.com/cgit/k3conf/k3conf/tree/common/socinfo.c#n382

This allows a switch statement to handle custom SR handling schemes or
use LUT with variants that use VARIANT field to handle things properly.

[...]
-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH] soc: ti: k3-socinfo: Fix the silicon revision misprint

[Neha Malcom Francis]

Hi Nishanth

On 13/09/23 16:58, Nishanth Menon wrote:
> On 12:07-20230912, Neha Malcom Francis wrote:
> 
> [...]
> 
>>>> +void
>>>> +k3_chipinfo_silicon_rev(unsigned int variant,
>>>> +			struct soc_device_attribute *soc_dev_attr)
>>>> +{
>>>> +	const char *family_name = soc_dev_attr->family;
>>>> +	int j721e_lookup_arr_size = ARRAY_SIZE(soc_revision_j721e);
>>>> +
>>>> +	if (!strcmp(family_name, "J721E") && variant < j721e_lookup_arr_size) {
>>>> +		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%s", soc_revision_j721e[variant]);
>>>> +	} else {
>>>> +		variant++;
>>>> +		soc_dev_attr->revision = kasprintf(GFP_KERNEL, "SR%x.0", variant);
>>>> +	}
>>>
>>> I am not comfortable with if else here. Why not extend k3_soc_id
>>> structure to include the variant LuT? Are there exceptions to this rule
>>> (Say AM65x?), those would make sense to handle with a compare against
>>> the partno?
>>>
>>
>> Trying to revive this patch, I see what you are saying is similar to the way
>> detection has already been implemented in U-Boot (drivers/soc/soc_ti_k3.c)
>> if I'm not mistaken.
> 
> Yes.
> 
>>
>> But I can't find any existing exception to this "family --> version" rule
>> that forces us to use "partno --> version". Checked through all AM65x device
>> TRMs available in ti.com; all seem to use common partno. So maybe I am not
>> on the same page, did you mean something else?
> 
> https://www.ti.com/lit/ug/spruid7e/spruid7e.pdf
> CTRLMMR_WKUP_JTAGID:: VARIANT field: SR2.0: 1h SR1.0: 0h
> Latest data sheet: https://www.ti.com/lit/ds/symlink/am6548.pdf
> indicates SR 2.1
> 
> How is this detected?

Detection of the ".x" bit is still a WIP and needs some alignment internally 
before I can add that patch. So for now, working on cleaning up the known issues 
of the driver.

> 
> What I indicated is a LUT table similar to
> https://git.ti.com/cgit/k3conf/k3conf/tree/common/socinfo.c#n382
> 
> This allows a switch statement to handle custom SR handling schemes or
> use LUT with variants that use VARIANT field to handle things properly.
> 

This makes sense, will work on the patch accordingly. Thanks!

> [...]

-- 
Thanking You
Neha Malcom Francis

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel