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

> +}
> +
>  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