Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36;
Date:   Thu, 14 Dec 2023 06:51:30 -0600
From:   Nishanth Menon <nm@ti.com>
To:     Neha Malcom Francis <n-francis@ti.com>
CC:     <robh+dt@kernel.org>, <krzysztof.kozlowski+dt@linaro.org>,
        <vigneshr@ti.com>, <linux-arm-kernel@lists.infradead.org>,
        <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        <a-nandan@ti.com>, <kristo@kernel.org>, <u-kumar1@ti.com>
Subject: Re: [PATCH] arm64: dts: ti: k3-j721e: Add support for DFS in J721E
 A72
Message-ID: <20231214125130.zqtq6ioj4c533wha@elbow>
References: <20231214075637.176586-1-n-francis@ti.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <20231214075637.176586-1-n-francis@ti.com>
Precedence: bulk

On 13:26-20231214, Neha Malcom Francis wrote:
> Add 2G, 1.5G, 1G, 750M, 500M and 250M as the supported frequencies for
> A72. This enables support for Dynamic Frequency Scaling (DFS).
> 

Just curious, since I picked up the PMIC support... can we do dvfs? if
not, please indicate that in the commit message.

> Signed-off-by: Neha Malcom Francis <n-francis@ti.com>
> ---
> Boot logs:
> https://gist.github.com/nehamalcom/e3c3d0446f0467e7fd28706f7ffaeea8
> 
> J721E SoC has three different speed grade devices (see [1], 7.5
> Operating Performance Points) which as of today are indiscernible in
> software, users of a different speed grade device must manually change
> the DTS to ensure their maximum speed frequency is supported.
> 
> [1] https://www.ti.com/lit/gpn/tda4vm
This is critical info in the commit message and in documentation of
source.

I am also concerned if the table should be separated out as a dtsi and
included at board.dts level to prevent downstream users going crazy..

Are you absolutely sure this has no detection logic that can be
implemented? Almost all TI K3 SoCs seem to have a standard scheme to
detect the speed grades till date. /me wonders what the heck happened
here..

> 
>  arch/arm64/boot/dts/ti/k3-j721e.dtsi | 35 ++++++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/arch/arm64/boot/dts/ti/k3-j721e.dtsi b/arch/arm64/boot/dts/ti/k3-j721e.dtsi
> index a200810df54a..fe92879f5812 100644
> --- a/arch/arm64/boot/dts/ti/k3-j721e.dtsi
> +++ b/arch/arm64/boot/dts/ti/k3-j721e.dtsi
> @@ -48,6 +48,9 @@ cpu0: cpu@0 {
>  			d-cache-line-size = <64>;
>  			d-cache-sets = <256>;
>  			next-level-cache = <&L2_0>;
> +			clocks = <&k3_clks 202 2>;
> +			clock-names = "cpu";
> +			operating-points-v2 = <&cpu0_opp_table>;
>  		};
>  
>  		cpu1: cpu@1 {
> @@ -62,9 +65,41 @@ cpu1: cpu@1 {
>  			d-cache-line-size = <64>;
>  			d-cache-sets = <256>;
>  			next-level-cache = <&L2_0>;
> +			clocks = <&k3_clks 203 0>;
> +			clock-names = "cpu";
> +			operating-points-v2 = <&cpu0_opp_table>;
>  		};
>  	};
>  
> +	cpu0_opp_table: opp-table {
> +		compatible = "operating-points-v2";
Don't you need opp-shared ?
> +
> +		opp6-2000000000 {
> +			opp-hz = /bits/ 64 <2000000000>;
> +		};
> +
> +		opp5-1500000000 {
> +			opp-hz = /bits/ 64 <1500000000>;
> +		};
> +
> +		opp4-1000000000 {
> +			opp-hz = /bits/ 64 <1000000000>;
> +		};
> +
> +		opp3-750000000 {
> +			opp-hz = /bits/ 64 <750000000>;
> +		};
> +
> +		opp2-500000000 {
> +			opp-hz = /bits/ 64 <500000000>;
> +		};
> +
> +		opp1-250000000 {
> +			opp-hz = /bits/ 64 <250000000>;
Could you add clock-latency-ns ?
> +		};
> +
> +	};
> +
>  	L2_0: l2-cache0 {
>  		compatible = "cache";
>  		cache-level = <2>;
> -- 
> 2.34.1
> 


-- 
Regards,
Nishanth Menon
Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3  1A34 DDB5 849D 1736 249D