Re: [PATCH v2 2/2] arm64: dts: ti: k3-j721s2: Add GPU node

[Randolph Sapp <rs@ti.com>]

On Sat Apr 19, 2025 at 11:15 AM CDT, Udit Kumar wrote:
> Hello Matt,
>
> On 4/17/2025 2:40 PM, Matt Coster wrote:
>> The J721S2 binding is based on the TI downstream binding in commit
>> 54b0f2a00d92 ("arm64: dts: ti: k3-j721s2-main: add gpu node") from [1]
>> but with updated compatible strings.
>
> Downstream kernel[1] sha 5657fc069e8b3 ("PENDING: arm64: dts: ti: 
> k3-j721s2-main: add gpu node")
>
> also has assigned-clock-rates.
>
> Please check if gpu node needs assigned-rate too.

If I remember correctly, J721S2 was one of the few platforms that actually
defaulted to 800MHz, so it may not be necessary for that platform specifically.
(I don't have a board to test this right now though. This very well may have
changed.) AM62 also defaults to the correct value, and that one I did manage to
verify.

That being said, Udit is right, it's generally a good idea to explicitly set the
clock speed for our devices. I know AM62P, for example, used to default our
clock to the bus speed.

At one point though this driver was experimenting with a DVFS mechanism. Matt,
use of assigned-clocks shouldn't interfere with that assuming there is no
defined opp-table, right? May be a good idea to set our usual 800 MHz for J721S2
and 500 MHz for AM625. This shouldn't require any binding related changes.

On the topic of opp tables for the GPU, I did some testing on the proprietary
driver a little while back. These devices do not support voltage scaling and
simple frequency scaling saw a general decrease in performance and increase in
power draw for the usual utilization bursts a single application running at 60
FPS generates. I have a feeling this will carry over to the open source driver,
but we can always do additional testing if you are curious.

- Randolph

>> The clock[2] and power[3] indices were verified from HTML docs, while
>> the intterupt index comes from the TRM[4] (appendix
>> "J721S2_Appendix_20241106_Public.xlsx", "Interrupts (inputs)",
>> "GPU_BXS464_WRAP0_GPU_SS_0_OS_IRQ_OUT_0").
>
>> [1]: https://git.ti.com/cgit/ti-linux-kernel/ti-linux-kernel
>> [2]: https://downloads.ti.com/tisci/esd/latest/5_soc_doc/j721s2/clocks.html
>> [3]: https://downloads.ti.com/tisci/esd/latest/5_soc_doc/j721s2/devices.html
>> [4]: https://www.ti.com/lit/zip/spruj28 (revision E)
>>
>> Reviewed-by: Randolph Sapp <rs@ti.com>
>> Signed-off-by: Matt Coster <matt.coster@imgtec.com>
>> ---
>> Changes in v2:
>> - Add interrupt reference details
>> - Add Randolph's Rb
>> - Link to v1: https://lore.kernel.org/r/20250415-bxs-4-64-dts-v1-2-f7d3fa06625d@imgtec.com
>>
>> This patch was previously sent as [DO NOT MERGE]:
>> https://lore.kernel.org/r/20250410-sets-bxs-4-64-patch-v1-v6-18-eda620c5865f@imgtec.com
>> ---
>>   arch/arm64/boot/dts/ti/k3-j721s2-main.dtsi | 12 ++++++++++++
>>   1 file changed, 12 insertions(+)
>>
>> diff --git a/arch/arm64/boot/dts/ti/k3-j721s2-main.dtsi b/arch/arm64/boot/dts/ti/k3-j721s2-main.dtsi
>> index 92bf48fdbeba45ecca8c854db5f72fd3666239c5..a79ac41b2c1f51b7193e6133864428bd35a5e835 100644
>> --- a/arch/arm64/boot/dts/ti/k3-j721s2-main.dtsi
>> +++ b/arch/arm64/boot/dts/ti/k3-j721s2-main.dtsi
>> @@ -2048,4 +2048,16 @@ watchdog8: watchdog@23f0000 {
>>   		/* reserved for MAIN_R5F1_1 */
>>   		status = "reserved";
>>   	};
>> +
>> +	gpu: gpu@4e20000000 {
>> +		compatible = "ti,j721s2-gpu", "img,img-bxs-4-64", "img,img-rogue";
>> +		reg = <0x4e 0x20000000 0x00 0x80000>;
>> +		clocks = <&k3_clks 130 1>;
>> +		clock-names = "core";
>> +		interrupts = <GIC_SPI 24 IRQ_TYPE_LEVEL_HIGH>;
>> +		power-domains = <&k3_pds 130 TI_SCI_PD_EXCLUSIVE>,
>> +				<&k3_pds 373 TI_SCI_PD_EXCLUSIVE>;
>> +		power-domain-names = "a", "b";
>> +		dma-coherent;
>> +	};
>>   };
>>