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

[Matt Coster <Matt.Coster@imgtec.com>]

[-- Attachment #1.1: Type: text/plain, Size: 4602 bytes --]

On 22/04/2025 13:04, Nishanth Menon wrote:
> On 18:51-20250421, Randolph Sapp wrote:
>> 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>;
> 
> On the basis of the above discussion, Matt,
> please add:
> assigned-clocks = <&k3_clks 130 1>;
> assigned-clock-rates = <800000000>;

As requested, I've sent a v3 with these properties added. I checked
using:

$ make CHECK_DTBS=1 ti/k3-am68-sk-base-board.dtb

Which reported no issues. Does this mean these properties are defined as
globally allowed somewhere, and that we will not need to add them
specifically to our bindings?

Cheers,
Matt

> 
>>>> +		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;
>>>> +	};
>>>>   };
>>>>
>>
> 


-- 
Matt Coster
E: matt.coster@imgtec.com

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]