[PATCH v4 4/5] arm: mach-k3: j721e-init.c: Add support for CONFIG_K3_OPP_LOW

[Manorit Chawdhry]

Hi Aniket,

On 13:02-20241119, Aniket Limaye wrote:
> Hi Manorit,
> 
> On 19/11/24 11:42, Manorit Chawdhry wrote:
> > Hi Aniket,
> > 
> > On 06:02-20241119, Aniket Limaye wrote:
> > > The default j7200 devicetree and k3_avs driver set 2GHz/1GHz frequency
> > > for A72/MSMC clks and the OPP_NOM voltage.
> > > 
> > > J7200 SOCs may support OPP_LOW Operating Performance Point:
> > > 1GHz/500MHz clks for A72/MSMC and OPP_LOW AVS voltage read from efuse.
> > > 
> > > Hence, add a config check in board_init_f() to select OPP_LOW specs:
> > > - Check if OPP_LOW AVS voltage read from efuse is valid.
> > > - Use the device IDs and clock IDs (TISCI docs [0]) to find the A72 and
> > >    MSMC clock frequencies in the devicetree.
> > > - Fixup the clock frequencies in devicetree as per OPP_LOW spec.
> > > 
> > > k3_avs driver programs the OPP_LOW AVS voltage for VDD_CPU through
> > > k3_avs_notify_freq() callback from clk_k3.
> > > 
> > > [0]: https://software-dl.ti.com/tisci/esd/latest/5_soc_doc/j7200/clocks.html
> > > 
> > > Signed-off-by: Aniket Limaye <a-limaye at ti.com>
> > > ---
> > [..]
> > > ---
> > >   arch/arm/mach-k3/j721e/j721e_init.c | 92 ++++++++++++++++++++++++++++-
> > >   1 file changed, 91 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/arch/arm/mach-k3/j721e/j721e_init.c b/arch/arm/mach-k3/j721e/j721e_init.c
> > > index e9ed8cb267c..805b28af8e4 100644
> > > --- a/arch/arm/mach-k3/j721e/j721e_init.c
> > > +++ b/arch/arm/mach-k3/j721e/j721e_init.c
> > > @@ -19,6 +19,7 @@
> > 
> > [..]
> > > +#if defined(CONFIG_CPU_V7R) && defined(CONFIG_K3_AVS0)
> > > +static int get_clock_index_by_dev_id(ofnode node, u32 dev_id, u32 clk_id)
> > 
> > I think the following function can be re-usable by other K3 socs too,
> > can we put it in some common place instead of j721e_init? Maybe clk-k3.c
> > and exported from there?
> > 
> 
> Yeah that does make sense to do... There are already similar usages, I
> think, in other k3 platforms. Having said that I would like to take that up
> as a separate series since the DT index search is not directly related to
> OPP_LOW Perf Point setting for j7200.
> 
> Let me know if you think that the k3 DT clock index search belongs in the
> same series itself...

I mean you did make the function so I think might as well put it in the
correct location but it's a non blocker for me, was just a suggestion.
Would let you see how you want to do it.

> 
> > > +
> > > +static int fdt_fixup_a72ss_clock_frequency(void)
> > > +{
> > > +	int index, size;
> > > +	u32 *rates;
> > > +	ofnode node;
> > > +
> > > +	node = ofnode_by_compatible(ofnode_null(), "ti,am654-rproc");
> > > +	if (!ofnode_valid(node)) {
> > > +		printf("%s: A72 not found\n", __func__);
> > > +		return -ENODEV;
> > > +	}
> > > +
> > > +	rates = fdt_getprop_w(ofnode_to_fdt(node), ofnode_to_offset(node),
> > > +			      "assigned-clock-rates", &size);
> > > +	if (!rates) {
> > > +		printf("%s: Wrong A72 assigned-clocks-rates configuration\n", __func__);
> > > +		return -1;
> > > +	}
> > > +
> > > +	/* Update A72 Clock Frequency to OPP_LOW spec */
> > > +	index = get_clock_index_by_dev_id(node,
> > > +					  DEV_A72SS0_CORE0_0_ID,
> > > +					  DEV_A72SS0_CORE0_0_ARM_CLK_CLK_ID);
> > > +	if (index < 0 || index >= (size / sizeof(u32))) {
> > 
> > When would we encounter the second if..?
> 
> Assuming you mean:
> 	index >= (size / sizeof(u32))
> 
> The RHS is the count of "assigned-clock-rates" in the node and the LHS
> (index) is the id of the "assigned-clocks".
> It's possible (and I see it in a few cases too) that count of
> "assigned-clocks" > count of "assigned-clock-rates". In that case the
> "index" here, even if parsed correctly, could turn out to be outside of the
> bounds of the array "rates". Which should be check against.

Ah okay, thanks for explaining!

Reviewed-by: Manorit Chawdhry <m-chawdhry at ti.com>

Regards,
Manorit

> 
> Regards,
> Aniket
> 
> > 
> > > +		printf("%s: Wrong A72 assigned-clocks configuration\n", __func__);
> > > +		return -1;
> > > +	}
> > > +	rates[index] = cpu_to_fdt32(1000000000);
> > > +	printf("Changed A72 CPU frequency to %dHz in DT\n", 1000000000);
> > > +
> > > +	/* Update MSMC Clock Frequency to OPP_LOW spec */
> > > +	index = get_clock_index_by_dev_id(node,
> > > +					  DEV_A72SS0_CORE0_ID,
> > > +					  DEV_A72SS0_CORE0_MSMC_CLK_ID);
> > > +	if (index < 0 || index >= (size / sizeof(u32))) {
> > > +		printf("%s: Wrong A72 assigned-clocks configuration\n", __func__);
> > > +		return -1;
> > > +	}
> > > +	rates[index] = cpu_to_fdt32(500000000);
> > > +	printf("Changed MSMC frequency to %dHz in DT\n", 500000000);
> > > +
> > > +	return 0;
> > > +}
> > > +#endif
> > [..]
> > 
> > Regards,
> > Manorit