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@xxxxxx>
> > ---
> >   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