[PATCH v2 0/2] Imagination BXS-4-64 MC1 GPU support (DTS changes)

[PATCH v2 0/2] Imagination BXS-4-64 MC1 GPU support (DTS changes)

[Matt Coster]

Now that the binding and driver changes to support the Imagination
BXS-4-64 [1] have landed in a DRM tree, here are the corresponding DTS
changes without the [DO NOT MERGE] tag.

This GPU is found in the TI AM68 family of SoCs, with initial support
added to the k3-j721s2 devicetree and tested on a TI SK-AM68 board.

[1]: https://lore.kernel.org/r/20250410-sets-bxs-4-64-patch-v1-v6-0-eda620c5865f@imgtec.com

Reviewed-by: Randolph Sapp <rs@ti.com>
Signed-off-by: Matt Coster <matt.coster@imgtec.com>

---
Changes in v2:
- Add details of the source of the interrupt index
- Add Randolph's Rb
- Link to v1: https://lore.kernel.org/r/20250415-bxs-4-64-dts-v1-0-f7d3fa06625d@imgtec.com

---
Matt Coster (2):
      arm64: dts: ti: k3-am62: New GPU binding details
      arm64: dts: ti: k3-j721s2: Add GPU node

 arch/arm64/boot/dts/ti/k3-am62-main.dtsi   |  4 +++-
 arch/arm64/boot/dts/ti/k3-j721s2-main.dtsi | 12 ++++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)
---
base-commit: d6fe216caf15d196e1bf76591440f8f17d58e7ee
change-id: 20250415-bxs-4-64-dts-c984d0876556

[PATCH v2 1/2] arm64: dts: ti: k3-am62: New GPU binding details

[Matt Coster]

Use the new compatible string and power domain name as introduced in
commit 2c01d9099859 ("dt-bindings: gpu: img: Future-proofing
enhancements")[1].

[1]: https://lore.kernel.org/r/20250410-sets-bxs-4-64-patch-v1-v6-1-eda620c5865f@imgtec.com

Reviewed-by: Randolph Sapp <rs@ti.com>
Signed-off-by: Matt Coster <matt.coster@imgtec.com>
---
Changes in v2:
- Add Randolph's Rb
- Link to v1: https://lore.kernel.org/r/20250415-bxs-4-64-dts-v1-1-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-17-eda620c5865f@imgtec.com
---
 arch/arm64/boot/dts/ti/k3-am62-main.dtsi | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/boot/dts/ti/k3-am62-main.dtsi b/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
index 7d355aa73ea2116723735f70b9351cefcd8bc118..d17b25cae196b08d24adbe7c913ccaba7eed37eb 100644
--- a/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am62-main.dtsi
@@ -691,12 +691,14 @@ ospi0: spi@fc40000 {
 	};
 
 	gpu: gpu@fd00000 {
-		compatible = "ti,am62-gpu", "img,img-axe";
+		compatible = "ti,am62-gpu", "img,img-axe-1-16m", "img,img-axe",
+			     "img,img-rogue";
 		reg = <0x00 0x0fd00000 0x00 0x20000>;
 		clocks = <&k3_clks 187 0>;
 		clock-names = "core";
 		interrupts = <GIC_SPI 86 IRQ_TYPE_LEVEL_HIGH>;
 		power-domains = <&k3_pds 187 TI_SCI_PD_EXCLUSIVE>;
+		power-domain-names = "a";
 	};
 
 	cpsw3g: ethernet@8000000 {

-- 
2.49.0

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

[Matt Coster]

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.

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

-- 
2.49.0

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

[Kumar, Udit]

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.


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

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

[Randolph Sapp]

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

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

[Nishanth Menon]

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

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

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

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

[Matt Coster]

[-- 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 --]

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

[Randolph Sapp]

On Tue Apr 22, 2025 at 11:22 AM CDT, Matt Coster wrote:
> 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

Yeah, they are defined by the clock meta-schema [1] and inherited by the core
meta-schema [2] that you are using.

[1] https://github.com/devicetree-org/dt-schema/blob/e6ea659d2baa30df1ec0fcc4f8354208692489eb/dtschema/meta-schemas/clocks.yaml#L26
[2] https://github.com/devicetree-org/dt-schema/blob/e6ea659d2baa30df1ec0fcc4f8354208692489eb/dtschema/meta-schemas/core.yaml#L29

- Randolph

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