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

[PATCH 0/3] 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 an AM67A.

v1:
 - 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 (3):
  dt-bindings: gpu: img: Add AM62P SoC specific compatible
  clk: keystone: don't cache clock rate
  arm64: dts: ti: add GPU node

 .../devicetree/bindings/gpu/img,powervr-rogue.yaml    |  2 ++
 .../arm64/boot/dts/ti/k3-am62p-j722s-common-main.dtsi | 11 +++++++++++
 drivers/clk/keystone/sci-clk.c                        |  8 ++++++++
 3 files changed, 21 insertions(+)

-- 
2.39.5

[PATCH 1/3] 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>
---
 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 c87d7bece0ec..a207a57c013b 100644
--- a/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
+++ b/Documentation/devicetree/bindings/gpu/img,powervr-rogue.yaml
@@ -28,6 +28,7 @@ properties:
           - const: img,img-rogue
       - items:
           - enum:
+              - ti,am62p-gpu
               - ti,j721s2-gpu
           - const: img,img-bxs-4-64
           - const: img,img-rogue
@@ -140,6 +141,7 @@ allOf:
           contains:
             enum:
               - ti,am62-gpu
+              - ti,am62p-gpu
               - ti,j721s2-gpu
     then:
       properties:
-- 
2.39.5

[PATCH 2/3] 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>
---
I guess to make it work correctly with the caching of the linux
subsystem a new flag to query the real clock rate is needed. That
way, one could also query the default value without having to turn
the clock and consumer on first. That can be retrofitted later and
the driver could query the firmware capabilities.

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.
---
 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 c5894fc9395e..d73858b5ca7a 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.39.5

[PATCH 3/3] 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 d0cfd6821b18..d02f7fba327b 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
@@ -1232,6 +1232,17 @@ dsi0: dsi@30500000 {
 		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.39.5

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

[Conor Dooley]

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

Acked-by: Conor Dooley <conor.dooley@microchip.com>

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

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

[Kevin Hilman]

Michael Walle <mwalle@kernel.org> writes:

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

Hmm, it also seems wrong to me that the clock framework would cache a
clock rate when it's disabled.  On platforms with clocks that may have
shared management (eg. TISCI or other platforms using SCMI) it's
entirely possible that when Linux has disabled a clock, some other
entity may have changed it.

Could another solution here be to have the clk framework only cache when
clocks are enabled?

> Thus, disable the cache altogether.
>
> Signed-off-by: Michael Walle <mwalle@kernel.org>
> ---
> I guess to make it work correctly with the caching of the linux
> subsystem a new flag to query the real clock rate is needed. That
> way, one could also query the default value without having to turn
> the clock and consumer on first. That can be retrofitted later and
> the driver could query the firmware capabilities.
>
> 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.

The performance hit is not just about boot time, it's for *every*
[get|set]_rate call.  Since TISCI is relatively slow (involves RPC,
mailbox, etc. to remote core), this may have a performance impact
elsewhere too.  That being said, I'm hoping it's unlikely that
[get|set]_rate calls are in the fast path.

All of that being said, I think the impacts of this patch are pretty
minimal, so I don't have any real objections.

Reviewed-by: Kevin Hilman <khilman@baylibre.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 c5894fc9395e..d73858b5ca7a 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.39.5

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

[Michael Walle]

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

On Wed Sep 17, 2025 at 5:24 PM CEST, Kevin Hilman wrote:
> Michael Walle <mwalle@kernel.org> writes:
>
> > 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.
>
> Hmm, it also seems wrong to me that the clock framework would cache a
> clock rate when it's disabled.  On platforms with clocks that may have
> shared management (eg. TISCI or other platforms using SCMI) it's
> entirely possible that when Linux has disabled a clock, some other
> entity may have changed it.
>
> Could another solution here be to have the clk framework only cache when
> clocks are enabled?

It's not just the clock which has to be enabled, but also it's
consumer. I.e. for this case, the GPU has to be enabled, until that
is the case the get_rate always returns 0. The clk framework already
has support for the runtime power management of the clock itself,
see for example clk_recalc().

> > Thus, disable the cache altogether.
> >
> > Signed-off-by: Michael Walle <mwalle@kernel.org>
> > ---
> > I guess to make it work correctly with the caching of the linux
> > subsystem a new flag to query the real clock rate is needed. That
> > way, one could also query the default value without having to turn
> > the clock and consumer on first. That can be retrofitted later and
> > the driver could query the firmware capabilities.
> >
> > 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.
>
> The performance hit is not just about boot time, it's for *every*
> [get|set]_rate call.  Since TISCI is relatively slow (involves RPC,
> mailbox, etc. to remote core), this may have a performance impact
> elsewhere too.

Yes of course. I have just looked what happened during boot and
(short) after the boot. I haven't had any real application running,
though, so that's not representative.

> That being said, I'm hoping it's unlikely that
> [get|set]_rate calls are in the fast path.
>
> All of that being said, I think the impacts of this patch are pretty
> minimal, so I don't have any real objections.
>
> Reviewed-by: Kevin Hilman <khilman@baylibre.com>

Thanks!

-michael

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

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

[Dhruva Gole]

Hi Michael,

On Sep 18, 2025 at 11:48:34 +0200, Michael Walle wrote:
> On Wed Sep 17, 2025 at 5:24 PM CEST, Kevin Hilman wrote:
> > Michael Walle <mwalle@kernel.org> writes:
> >
> > > 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.
> >
> > Hmm, it also seems wrong to me that the clock framework would cache a
> > clock rate when it's disabled.  On platforms with clocks that may have
> > shared management (eg. TISCI or other platforms using SCMI) it's
> > entirely possible that when Linux has disabled a clock, some other
> > entity may have changed it.
> >
> > Could another solution here be to have the clk framework only cache when
> > clocks are enabled?
> 
> It's not just the clock which has to be enabled, but also it's
> consumer. I.e. for this case, the GPU has to be enabled, until that
> is the case the get_rate always returns 0. The clk framework already
> has support for the runtime power management of the clock itself,
> see for example clk_recalc().

Why did we move away from the earlier approach [1] again?
[1] https://lore.kernel.org/all/20250716134717.4085567-3-mwalle@kernel.org/


> 
> > > Thus, disable the cache altogether.
> > >
> > > Signed-off-by: Michael Walle <mwalle@kernel.org>
> > > ---
> > > I guess to make it work correctly with the caching of the linux
> > > subsystem a new flag to query the real clock rate is needed. That
> > > way, one could also query the default value without having to turn
> > > the clock and consumer on first. That can be retrofitted later and
> > > the driver could query the firmware capabilities.
> > >
> > > 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.
> >
> > The performance hit is not just about boot time, it's for *every*
> > [get|set]_rate call.  Since TISCI is relatively slow (involves RPC,
> > mailbox, etc. to remote core), this may have a performance impact
> > elsewhere too.
> 
> Yes of course. I have just looked what happened during boot and
> (short) after the boot. I haven't had any real application running,
> though, so that's not representative.

I am not sure what cpufreq governor you had running, but depending on the governor,
filesystem, etc. cpufreq can end up potentially doing a lot more of
the clk_get|set_rates which could have some series performance degradation
is what I'm worried about. Earlier maybe the clk_get_rate part was
returning the cached CPU freqs, but now it will each time go query the
firmware for it (unnecessarily)

I currently don't have any solid data to say how much of an impact
for sure but I can run some tests locally and find out...

> 
> > That being said, I'm hoping it's unlikely that
> > [get|set]_rate calls are in the fast path.
> >
> > All of that being said, I think the impacts of this patch are pretty
> > minimal, so I don't have any real objections.
> >
> > Reviewed-by: Kevin Hilman <khilman@baylibre.com>
> 
> Thanks!
> 
> -michael



-- 
Best regards,
Dhruva Gole
Texas Instruments Incorporated

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

[Michael Walle]

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

Hi,

> > > > 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.
> > >
> > > Hmm, it also seems wrong to me that the clock framework would cache a
> > > clock rate when it's disabled.  On platforms with clocks that may have
> > > shared management (eg. TISCI or other platforms using SCMI) it's
> > > entirely possible that when Linux has disabled a clock, some other
> > > entity may have changed it.
> > >
> > > Could another solution here be to have the clk framework only cache when
> > > clocks are enabled?
> > 
> > It's not just the clock which has to be enabled, but also it's
> > consumer. I.e. for this case, the GPU has to be enabled, until that
> > is the case the get_rate always returns 0. The clk framework already
> > has support for the runtime power management of the clock itself,
> > see for example clk_recalc().
>
> Why did we move away from the earlier approach [1] again?
> [1] https://lore.kernel.org/all/20250716134717.4085567-3-mwalle@kernel.org/

Because that not fixing the root case. Also it probably doesn't work
if there is no assigned-clocks property. Nishanth asked for the
latter and the soc dtsi should rely on the hardware default.

> > > > Thus, disable the cache altogether.
> > > >
> > > > Signed-off-by: Michael Walle <mwalle@kernel.org>
> > > > ---
> > > > I guess to make it work correctly with the caching of the linux
> > > > subsystem a new flag to query the real clock rate is needed. That
> > > > way, one could also query the default value without having to turn
> > > > the clock and consumer on first. That can be retrofitted later and
> > > > the driver could query the firmware capabilities.
> > > >
> > > > 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.
> > >
> > > The performance hit is not just about boot time, it's for *every*
> > > [get|set]_rate call.  Since TISCI is relatively slow (involves RPC,
> > > mailbox, etc. to remote core), this may have a performance impact
> > > elsewhere too.
> > 
> > Yes of course. I have just looked what happened during boot and
> > (short) after the boot. I haven't had any real application running,
> > though, so that's not representative.
>
> I am not sure what cpufreq governor you had running, but depending on the governor,
> filesystem, etc. cpufreq can end up potentially doing a lot more of
> the clk_get|set_rates which could have some series performance degradation
> is what I'm worried about. Earlier maybe the clk_get_rate part was
> returning the cached CPU freqs, but now it will each time go query the
> firmware for it (unnecessarily)

There doesn't seem to be a cpufreq compatible for the am67a (which
might make sense to add though). But I'm wondering how much energy
that will save because the SoC can't do voltage scaling.

-michael

> I currently don't have any solid data to say how much of an impact
> for sure but I can run some tests locally and find out...
>
> > 
> > > That being said, I'm hoping it's unlikely that
> > > [get|set]_rate calls are in the fast path.
> > >
> > > All of that being said, I think the impacts of this patch are pretty
> > > minimal, so I don't have any real objections.
> > >
> > > Reviewed-by: Kevin Hilman <khilman@baylibre.com>
> > 
> > Thanks!
> > 
> > -michael


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

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

[Randolph Sapp]

On Mon Sep 15, 2025 at 9:34 AM CDT, 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>
> ---
> I guess to make it work correctly with the caching of the linux
> subsystem a new flag to query the real clock rate is needed. That
> way, one could also query the default value without having to turn
> the clock and consumer on first. That can be retrofitted later and
> the driver could query the firmware capabilities.
>
> 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.
> ---
>  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 c5894fc9395e..d73858b5ca7a 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);


Thanks for looking into this Michael. I'm still convinced that it's unusual to
report 0 for a clock rate when the device is powered down. In most cases it's
not actually 0 and is actually just in bypass mode.

I was told it's a way to indicate clock status and probably won't be changing
any time soon though. Ignore the fact that we also already have a separate way
to query clock status. :)

This series looks good, but won't quite result in a functional GPU without the
following patch: https://lore.kernel.org/all/20250808232522.1296240-1-rs@ti.com/

I suppose I'll submit that again on it's own.

Reviewed-by: Randolph Sapp <rs@ti.com>

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

[Michael Walle]

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

Hi,

> > 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>
> > ---
> > I guess to make it work correctly with the caching of the linux
> > subsystem a new flag to query the real clock rate is needed. That
> > way, one could also query the default value without having to turn
> > the clock and consumer on first. That can be retrofitted later and
> > the driver could query the firmware capabilities.
> >
> > 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.
> > ---
> >  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 c5894fc9395e..d73858b5ca7a 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);
>
>
> Thanks for looking into this Michael. I'm still convinced that it's unusual to
> report 0 for a clock rate when the device is powered down. In most cases it's
> not actually 0 and is actually just in bypass mode.

Yeah. And you can't query the clock rate you've just set if the
clock is off (and if enabled the clock will have the frequency of
the last set_rate). I still think that is a gap in the firmware
interface.

> I was told it's a way to indicate clock status and probably won't be changing
> any time soon though. Ignore the fact that we also already have a separate way
> to query clock status. :)
>
> This series looks good, but won't quite result in a functional GPU without the
> following patch: https://lore.kernel.org/all/20250808232522.1296240-1-rs@ti.com/

Ahh, I was puzzled why it was working for me. But then I've noticed
that your patch is for the am62p. I'm working with the am67a and
there the ranges are correct for the GPU.

> I suppose I'll submit that again on it's own.
>
> Reviewed-by: Randolph Sapp <rs@ti.com>

Thanks.

-michael

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

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

[Maxime Ripard]

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

On Wed, Sep 17, 2025 at 08:24:47AM -0700, Kevin Hilman wrote:
> Michael Walle <mwalle@kernel.org> writes:
> 
> > 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.
> 
> Hmm, it also seems wrong to me that the clock framework would cache a
> clock rate when it's disabled.  On platforms with clocks that may have
> shared management (eg. TISCI or other platforms using SCMI) it's
> entirely possible that when Linux has disabled a clock, some other
> entity may have changed it.

It doesn't really help that the CCF doesn't seem to agree on if it
should do that in the first place :)

In the original clk API definition, you're not supposed to call
clk_get_rate() when the clock is disabled.

https://elixir.bootlin.com/linux/v6.16.8/source/include/linux/clk.h#L746

However, it's been allowed by the CCF since forever:

https://elixir.bootlin.com/linux/v6.16.8/source/drivers/clk/clk.c#L1986

But then, some drivers will return 0 as a valid value, and not an error
code (whatever 0Hz for a clock means).

It's kind of a mess, and very regression prone, so I don't really expect
it to change anytime soon.

Maxime

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

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

[Randolph Sapp]

On Wed Sep 17, 2025 at 10:24 AM CDT, Kevin Hilman wrote:
> Michael Walle <mwalle@kernel.org> writes:
>
>> 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.
>
> Hmm, it also seems wrong to me that the clock framework would cache a
> clock rate when it's disabled.  On platforms with clocks that may have
> shared management (eg. TISCI or other platforms using SCMI) it's
> entirely possible that when Linux has disabled a clock, some other
> entity may have changed it.
>
> Could another solution here be to have the clk framework only cache when
> clocks are enabled?

So I looked into that. There are still about 34 clock operations that are
functionally uncached, but it does seem more logical than treating everything as
uncached.

Side note, why would someone even want to read the rate of an unprepared clock?
I dumped some debug info for all the clocks tripping this new uncached path.
Seems weird that it's even happening this often. Even weirder that it's
apparently happening 3 times to cpu0's core clock on the board I'm currently
testing.

Some devices it only happens once, others get it two or three times. Most of
them seem to be obvious - someone trying to read a rate before the clock was
prepared as part of a probe sequence. One seemed to be happening directly after
a clk_prepare_enable call that completed successfully. Not sure how that could
happen.

>> Thus, disable the cache altogether.
>>
>> Signed-off-by: Michael Walle <mwalle@kernel.org>
>> ---
>> I guess to make it work correctly with the caching of the linux
>> subsystem a new flag to query the real clock rate is needed. That
>> way, one could also query the default value without having to turn
>> the clock and consumer on first. That can be retrofitted later and
>> the driver could query the firmware capabilities.
>>
>> 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.
>
> The performance hit is not just about boot time, it's for *every*
> [get|set]_rate call.  Since TISCI is relatively slow (involves RPC,
> mailbox, etc. to remote core), this may have a performance impact
> elsewhere too.  That being said, I'm hoping it's unlikely that
> [get|set]_rate calls are in the fast path.
>
> All of that being said, I think the impacts of this patch are pretty
> minimal, so I don't have any real objections.
>
> Reviewed-by: Kevin Hilman <khilman@baylibre.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 c5894fc9395e..d73858b5ca7a 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.39.5

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

[Maxime Ripard]

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

On Wed, Sep 24, 2025 at 09:26:17PM -0500, Randolph Sapp wrote:
> On Wed Sep 17, 2025 at 10:24 AM CDT, Kevin Hilman wrote:
> > Michael Walle <mwalle@kernel.org> writes:
> >
> >> 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.
> >
> > Hmm, it also seems wrong to me that the clock framework would cache a
> > clock rate when it's disabled.  On platforms with clocks that may have
> > shared management (eg. TISCI or other platforms using SCMI) it's
> > entirely possible that when Linux has disabled a clock, some other
> > entity may have changed it.
> >
> > Could another solution here be to have the clk framework only cache when
> > clocks are enabled?
> 
> So I looked into that. There are still about 34 clock operations that are
> functionally uncached, but it does seem more logical than treating everything as
> uncached.
> 
> Side note, why would someone even want to read the rate of an unprepared clock?
> I dumped some debug info for all the clocks tripping this new uncached path.
> Seems weird that it's even happening this often. Even weirder that it's
> apparently happening 3 times to cpu0's core clock on the board I'm currently
> testing.

The short, unsatisfying, answer is that the API explicitly allowed it so far.

It's also somewhat natural when you have a functional rate to set it up
before enabling it and the logic driven by it so that you would avoid a
rate change, or something like a cycle where you would enable, shut
down, reparent, enable the clock again.

In such a case, we would either need the cache, or to read the rate, to
know if we have to change the clock rate in the first place.

Maxime

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

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

[Randolph Sapp]

On Thu Sep 25, 2025 at 6:43 AM CDT, Maxime Ripard wrote:
> On Wed, Sep 24, 2025 at 09:26:17PM -0500, Randolph Sapp wrote:
>> On Wed Sep 17, 2025 at 10:24 AM CDT, Kevin Hilman wrote:
>> > Michael Walle <mwalle@kernel.org> writes:
>> >
>> >> 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.
>> >
>> > Hmm, it also seems wrong to me that the clock framework would cache a
>> > clock rate when it's disabled.  On platforms with clocks that may have
>> > shared management (eg. TISCI or other platforms using SCMI) it's
>> > entirely possible that when Linux has disabled a clock, some other
>> > entity may have changed it.
>> >
>> > Could another solution here be to have the clk framework only cache when
>> > clocks are enabled?
>> 
>> So I looked into that. There are still about 34 clock operations that are
>> functionally uncached, but it does seem more logical than treating everything as
>> uncached.
>> 
>> Side note, why would someone even want to read the rate of an unprepared clock?
>> I dumped some debug info for all the clocks tripping this new uncached path.
>> Seems weird that it's even happening this often. Even weirder that it's
>> apparently happening 3 times to cpu0's core clock on the board I'm currently
>> testing.
>
> The short, unsatisfying, answer is that the API explicitly allowed it so far.
>
> It's also somewhat natural when you have a functional rate to set it up
> before enabling it and the logic driven by it so that you would avoid a
> rate change, or something like a cycle where you would enable, shut
> down, reparent, enable the clock again.
>
> In such a case, we would either need the cache, or to read the rate, to
> know if we have to change the clock rate in the first place.
>
> Maxime

Thanks Maxime. I'll take this as a hint to stop digging for the moment. Right
now, treating unprepared clocks as untrusted / uncached makes sense and
shouldn't be too much of a performance issue.

- Randolph