Re: [PATCH v2 1/2] soc: soc_ti_k3: Add support for AM62P variants

[Judith Mendez <jm@ti.com>]

Hi Tom,

On 8/6/25 10:49 AM, Tom Rini wrote:
> On Wed, Aug 06, 2025 at 10:45:29AM -0500, Judith Mendez wrote:
>> Hi Tom,
>>
>> On 8/5/25 5:39 PM, Tom Rini wrote:
>>> On Tue, Aug 05, 2025 at 11:14:18AM -0500, 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 register instead
>>>> of JTAGID register, so introduce GP_SW register range to determine SoC
>>>> revision for AM62P.
>>>>
>>>> Signed-off-by: Judith Mendez <jm@ti.com>
>>>> ---
>>>>    drivers/soc/soc_ti_k3.c | 70 ++++++++++++++++++++++++++++++++++++++---
>>>>    1 file changed, 65 insertions(+), 5 deletions(-)
>>> [snip]
>>>> @@ -130,17 +184,23 @@ static const struct soc_ops soc_ti_k3_ops = {
>>>>    int soc_ti_k3_probe(struct udevice *dev)
>>>>    {
>>>>    	struct soc_ti_k3_plat *plat = dev_get_plat(dev);
>>>> -	u32 idreg;
>>>> +	u32 gp_sw1_val = 0;
>>>>    	void *idreg_addr;
>>>> +	u32 idreg;
>>>> -	idreg_addr = dev_read_addr_ptr(dev);
>>>> +	idreg_addr = dev_read_addr_index_ptr(dev, 0);
>>>>    	if (!idreg_addr)
>>>>    		return -EINVAL;
>>>>    	idreg = readl(idreg_addr);
>>>> +#if IS_ENABLED(CONFIG_SOC_K3_AM62P5)
>>>> +	if (soc_ti_k3_variant_in_gp_sw(idreg))
>>>> +		gp_sw1_val = soc_ti_k3_get_variant_alternate(dev, idreg);
>>>> +#endif /* CONFIG_SOC_K3_AM62P5 */
>>>
>>> This isn't quite what I meant, as it will generate warnings about unused
>>> variables for the tables, on other platforms, yes? What I was thinking
>>> was:
>>> if (IS_ENABLED(CONFIG_SOC_K3_AM62P5) && soc_ti_k3_variant_in_gp_sw(idreg))
>>> 	gp_sw1_val = soc_ti_k3_get_variant_alternate(dev, idreg);
>>> which shouldn't. And then can we check the other platforms similarly to
>>> save space or no? Or am I unclear with what I'm thinking (or it's not
>>> possible, I didn't dig at the rest of the code much)? Thanks.
>>>
>>
>> It is not very clear, but let me clarify:soc_ti_k3_get_variant_alternate
>> should only get called for AM62P and get_rev_string should get called
>> for all SoCs.
>>
>> So it only makes sense to do this then:
>>
>> if (IS_ENABLED(CONFIG_SOC_K3_AM62P5) && soc_ti_k3_variant_in_gp_sw(idreg))
>> 	gp_sw1_val = soc_ti_k3_get_variant_alternate(dev, idreg);
>>
>> plat->revision = get_rev_string(idreg, gp_sw1_val);
> 
> Right, then the linker should normally be able to discard all of the
> am62p5 stuff on non-am62p5 platforms. Looking at the driver more now,
> OK, there's not anything we can save on the other cases that I was
> thinking about.
>

Understood, then will respin with the following change:

if (IS_ENABLED(CONFIG_SOC_K3_AM62P5) && soc_ti_k3_variant_in_gp_sw(idreg))
	gp_sw1_val = soc_ti_k3_get_variant_alternate(dev, idreg);

	plat->revision = get_rev_string(idreg, gp_sw1_val);

Thanks for reviewing (:

~ Judith