[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d50e7612-0b17-0f65-872f-90e08bed9948@ti.com>
Date: Tue, 12 Sep 2023 12:07:50 +0530
From: Neha Malcom Francis <n-francis@...com>
To: Nishanth Menon <nm@...com>, Thejasvi Konduru <t-konduru@...com>
CC: <linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>,
Santosh Shilimkar <ssantosh@...nel.org>,
Tero Kristo <t-kristo@...com>,
Grygorii Strashko <grygorii.strashko@...com>,
Lokesh Vutla <lokeshvutla@...com>,
Apurva Nandan <a-nandan@...com>, Udit Kumar <u-kumar1@...com>
Subject: Re: [PATCH] soc: ti: k3-socinfo: Fix the silicon revision misprint
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@...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
Powered by blists - more mailing lists