[PATCH v2 0/4] drm/imagination: add AM62P/AM67A/J722S support

[PATCH v2 0/4] drm/imagination: add AM62P/AM67A/J722S support

[Michael Walle]

The AM62P and AM67A/J722S feature the same BXS-4 GPU as the J721S2.
In theory, one have to just add the DT node. But it turns out, that
the clock handling is not working. If I understood Nishan Menon
correct, it is working on the J721S2 because there, the clock is
shared, while on the AM62P the GPU has its own PLL.
In the latter case, the driver will fail with a WARN() because the
queried clock rate is zero due to a wrong cached value.

This was tested on the sa67 board which is based on the AM67A SoC.

v2:
 - collect ACKs
 - rebase onto latest -next
 - new patch which enables 800MHz operation for the sa67 board

v1:
 - https://lore.kernel.org/r/20250915143440.2362812-1-mwalle@kernel.org/
 - Don't set the clock to 800MHz in the soc dtsi. 800MHz is only
   possible if the core voltage is 0.85V. Just use the hardware
   default of 720MHz. A board device tree can set the 800MHz if
   applicable. Thanks Nishan.
 - Also add the new compatible to a conditional in the DT schema.
   Thanks Andrew.
 - Dropped the wrong of_clk_set_defaults() and instead disable
   caching of the clock rate.

RFC:
 - https://lore.kernel.org/r/20250716134717.4085567-1-mwalle@kernel.org/

Michael Walle (4):
  dt-bindings: gpu: img: Add AM62P SoC specific compatible
  clk: keystone: don't cache clock rate
  arm64: dts: ti: add GPU node
  arm64: dts: ti: sa67: set the GPU clock to 800MHz

 .../devicetree/bindings/gpu/img,powervr-rogue.yaml    |  2 ++
 .../arm64/boot/dts/ti/k3-am62p-j722s-common-main.dtsi | 11 +++++++++++
 arch/arm64/boot/dts/ti/k3-am67a-kontron-sa67-base.dts |  6 ++++++
 drivers/clk/keystone/sci-clk.c                        |  8 ++++++++
 4 files changed, 27 insertions(+)

-- 
2.47.3

[PATCH v2 1/4] dt-bindings: gpu: img: Add AM62P SoC specific compatible

[Michael Walle]

The AM62P and the J722S features the same BXS-4 GPU as the J721S2. Add a
new SoC specific compatible.

Signed-off-by: Michael Walle <mwalle@kernel.org>
Acked-by: Conor Dooley <conor.dooley@microchip.com>
---
 Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
index 86ef68985317..a1f54dbae3f3 100644
--- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
+++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
@@ -40,6 +40,7 @@ properties:
           - const: img,img-rogue
       - items:
           - enum:
+              - ti,am62p-gpu
               - ti,j721s2-gpu
           - const: img,img-bxs-4-64
           - const: img,img-rogue
@@ -100,6 +101,7 @@ allOf:
           contains:
             enum:
               - ti,am62-gpu
+              - ti,am62p-gpu
               - ti,j721s2-gpu
     then:
       properties:
-- 
2.47.3

[PATCH v2 2/4] clk: keystone: don't cache clock rate

[Michael Walle]

The TISCI firmware will return 0 if the clock or consumer is not
enabled although there is a stored value in the firmware. IOW a call to
set rate will work but at get rate will always return 0 if the clock is
disabled.
The clk framework will try to cache the clock rate when it's requested
by a consumer. If the clock or consumer is not enabled at that point,
the cached value is 0, which is wrong. Thus, disable the cache
altogether.

Signed-off-by: Michael Walle <mwalle@kernel.org>
Reviewed-by: Kevin Hilman <khilman@baylibre.com>
Reviewed-by: Randolph Sapp <rs@ti.com>
---
 drivers/clk/keystone/sci-clk.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
index 9d5071223f4c..0a1565fdbb3b 100644
--- a/drivers/clk/keystone/sci-clk.c
+++ b/drivers/clk/keystone/sci-clk.c
@@ -333,6 +333,14 @@ static int _sci_clk_build(struct sci_clk_provider *provider,
 
 	init.ops = &sci_clk_ops;
 	init.num_parents = sci_clk->num_parents;
+
+	/*
+	 * A clock rate query to the SCI firmware will return 0 if either the
+	 * clock itself is disabled or the attached device/consumer is disabled.
+	 * This makes it inherently unsuitable for the caching of the clk
+	 * framework.
+	 */
+	init.flags = CLK_GET_RATE_NOCACHE;
 	sci_clk->hw.init = &init;
 
 	ret = devm_clk_hw_register(provider->dev, &sci_clk->hw);
-- 
2.47.3

Re: [PATCH v2 2/4] clk: keystone: don't cache clock rate

[Nishanth Menon]

On 13:47-20251223, Michael Walle wrote:
> The TISCI firmware will return 0 if the clock or consumer is not
> enabled although there is a stored value in the firmware. IOW a call to
> set rate will work but at get rate will always return 0 if the clock is
> disabled.
> The clk framework will try to cache the clock rate when it's requested
> by a consumer. If the clock or consumer is not enabled at that point,
> the cached value is 0, which is wrong. Thus, disable the cache
> altogether.
> 
> Signed-off-by: Michael Walle <mwalle@kernel.org>
> Reviewed-by: Kevin Hilman <khilman@baylibre.com>
> Reviewed-by: Randolph Sapp <rs@ti.com>
> ---
>  drivers/clk/keystone/sci-clk.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
> index 9d5071223f4c..0a1565fdbb3b 100644
> --- a/drivers/clk/keystone/sci-clk.c
> +++ b/drivers/clk/keystone/sci-clk.c
> @@ -333,6 +333,14 @@ static int _sci_clk_build(struct sci_clk_provider *provider,
>  
>  	init.ops = &sci_clk_ops;
>  	init.num_parents = sci_clk->num_parents;
> +
> +	/*
> +	 * A clock rate query to the SCI firmware will return 0 if either the
> +	 * clock itself is disabled or the attached device/consumer is disabled.
> +	 * This makes it inherently unsuitable for the caching of the clk
> +	 * framework.
> +	 */
> +	init.flags = CLK_GET_RATE_NOCACHE;
>  	sci_clk->hw.init = &init;
>  
>  	ret = devm_clk_hw_register(provider->dev, &sci_clk->hw);
> -- 
> 2.47.3
> 

Reviewed-by: Nishanth Menon <nm@ti.com>

I wish there was a better scheme, but inherently, just like SCMI and
other systems where power management co-processor controls clocks, there
is no real feasible caching scheme I can think of. I wonder if Stephen
or others have a thought on this?

That said, I wonder if we need fixes tag to this? I am sure there are
other clocks susceptible to this as well. I wonder if
commit 3c13933c6033 ("clk: keystone: sci-clk: add support for
dynamically probing clocks") is the appropriate tag?


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

Re: [PATCH v2 2/4] clk: keystone: don't cache clock rate

[Michael Walle]

[-- Attachment #1: Type: text/plain, Size: 2932 bytes --]

On Tue Dec 30, 2025 at 9:12 PM CET, Nishanth Menon wrote:
> On 13:47-20251223, Michael Walle wrote:
>> The TISCI firmware will return 0 if the clock or consumer is not
>> enabled although there is a stored value in the firmware. IOW a call to
>> set rate will work but at get rate will always return 0 if the clock is
>> disabled.
>> The clk framework will try to cache the clock rate when it's requested
>> by a consumer. If the clock or consumer is not enabled at that point,
>> the cached value is 0, which is wrong. Thus, disable the cache
>> altogether.
>> 
>> Signed-off-by: Michael Walle <mwalle@kernel.org>
>> Reviewed-by: Kevin Hilman <khilman@baylibre.com>
>> Reviewed-by: Randolph Sapp <rs@ti.com>
>> ---
>>  drivers/clk/keystone/sci-clk.c | 8 ++++++++
>>  1 file changed, 8 insertions(+)
>> 
>> diff --git a/drivers/clk/keystone/sci-clk.c b/drivers/clk/keystone/sci-clk.c
>> index 9d5071223f4c..0a1565fdbb3b 100644
>> --- a/drivers/clk/keystone/sci-clk.c
>> +++ b/drivers/clk/keystone/sci-clk.c
>> @@ -333,6 +333,14 @@ static int _sci_clk_build(struct sci_clk_provider *provider,
>>  
>>  	init.ops = &sci_clk_ops;
>>  	init.num_parents = sci_clk->num_parents;
>> +
>> +	/*
>> +	 * A clock rate query to the SCI firmware will return 0 if either the
>> +	 * clock itself is disabled or the attached device/consumer is disabled.
>> +	 * This makes it inherently unsuitable for the caching of the clk
>> +	 * framework.
>> +	 */
>> +	init.flags = CLK_GET_RATE_NOCACHE;
>>  	sci_clk->hw.init = &init;
>>  
>>  	ret = devm_clk_hw_register(provider->dev, &sci_clk->hw);
>> -- 
>> 2.47.3
>> 
>
> Reviewed-by: Nishanth Menon <nm@ti.com>
>
> I wish there was a better scheme, but inherently, just like SCMI and
> other systems where power management co-processor controls clocks, there
> is no real feasible caching scheme I can think of. I wonder if Stephen
> or others have a thought on this?
>
> That said, I wonder if we need fixes tag to this? I am sure there are
> other clocks susceptible to this as well. I wonder if
> commit 3c13933c6033 ("clk: keystone: sci-clk: add support for
> dynamically probing clocks") is the appropriate tag?

From my previous versions of this patch:

> Regarding a Fixes: tag. I didn't include one because it might have a
> slight performance impact because the firmware has to be queried
> every time now and it doesn't have been a problem for now. OTOH I've
> enabled tracing during boot and there were just a handful
> clock_{get/set}_rate() calls.

I'm still undecided if this needs a Fixes tag or not. Strictly
speaking it would need one. Although, I'm not sure it's the one
you mentioned, because the culprit is the "we return 0 if the clock
or it's consumer is disabled", which then caches the wrong value.
So it is probably the very first commit b745c0794e2f ("clk:
keystone: Add sci-clk driver support").

-michael

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 297 bytes --]

[PATCH v2 3/4] arm64: dts: ti: add GPU node

[Michael Walle]

The J722S features a BXS-4 GPU. Add the node for it.

Signed-off-by: Michael Walle <mwalle@kernel.org>
---
 .../arm64/boot/dts/ti/k3-am62p-j722s-common-main.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-am62p-j722s-common-main.dtsi b/arch/arm64/boot/dts/ti/k3-am62p-j722s-common-main.dtsi
index 3cf7c2b3ce2d..a55e5108e7b6 100644
--- a/arch/arm64/boot/dts/ti/k3-am62p-j722s-common-main.dtsi
+++ b/arch/arm64/boot/dts/ti/k3-am62p-j722s-common-main.dtsi
@@ -1110,6 +1110,17 @@ dphy0: phy@30110000 {
 		status = "disabled";
 	};
 
+	gpu: gpu@fd80000 {
+		compatible = "ti,am62p-gpu", "img,img-bxs-4-64", "img,img-rogue";
+		reg = <0x00 0x0fd80000 0x00 0x80000>;
+		clocks = <&k3_clks 237 3>;
+		clock-names = "core";
+		interrupts = <GIC_SPI 241 IRQ_TYPE_LEVEL_HIGH>;
+		power-domains = <&k3_pds 237 TI_SCI_PD_EXCLUSIVE>,
+				<&k3_pds 242 TI_SCI_PD_EXCLUSIVE>;
+		power-domain-names = "a", "b";
+	};
+
 	vpu: video-codec@30210000 {
 		compatible = "ti,j721s2-wave521c", "cnm,wave521c";
 		reg = <0x00 0x30210000 0x00 0x10000>;
-- 
2.47.3

[PATCH v2 4/4] arm64: dts: ti: sa67: set the GPU clock to 800MHz

[Michael Walle]

The board is running with a core voltage of 0.85V and therefore we can
clock the GPU at 800MHz.

Signed-off-by: Michael Walle <mwalle@kernel.org>
---
 arch/arm64/boot/dts/ti/k3-am67a-kontron-sa67-base.dts | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/boot/dts/ti/k3-am67a-kontron-sa67-base.dts b/arch/arm64/boot/dts/ti/k3-am67a-kontron-sa67-base.dts
index 7169d934adac..0c7f3e10a150 100644
--- a/arch/arm64/boot/dts/ti/k3-am67a-kontron-sa67-base.dts
+++ b/arch/arm64/boot/dts/ti/k3-am67a-kontron-sa67-base.dts
@@ -292,6 +292,12 @@ &cpsw_port1 {
 	status = "okay";
 };
 
+&gpu {
+	/* The VCORE is 0.85V, thus we can use a faster GPU clock. */
+	assigned-clocks = <&k3_clks 237 3>;
+	assigned-clock-rates = <800000000>;
+};
+
 &main_gpio0 {
 	gpio-line-names =
 		"", "", "", "", "", "", "", "SOC_SDIO_PWR_EN", "VSD_SEL",
-- 
2.47.3