BCM2835 clock driver, power domains and CLK_SET_RATE_GATE

BCM2835 clock driver, power domains and CLK_SET_RATE_GATE

[maxime]

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

Hi,

I had to fix a regression today that was mostly due to the fact that
clk-bcm2835 and clk-raspberrypi have fairly diverging behaviour.

https://lore.kernel.org/dri-devel/20221021131339.2203291-1-maxime@cerno.tech/

The "fix" is rather suboptimal and I'd like to address it properly.

Even though the commit message tries to explain everything and why other
solutions wouldn't work, the underlying cause is that clk-bcm2835 has a
bunch of restrictions that I don't really get and we have to work
around. I'm not sure I have the full picture either, so please take the
following with a grain of salt.

The first one is that the HSM clock discussed in that patch is part of
the HDMI power domain, so it has to be powered for the clock to retain
its configuration.

The side effect is that we need to initialize the clock at boot time so
that it has some rate. This is pretty important since that clock feeds
the register and thus a failure to initialize would lock the CPU on a
register access.

The second one is that since clk-bcm2835 has CLK_SET_RATE_GATE, we have
to call clk_set_min_rate() before clk_prepare_enable(), or
clk_set_min_rate() is likely to fail.

So we need in runtime_resume to call clk_set_min_rate with an absolute
minimum for the HDMI controller to be functional and then enable the
clock.

But then, since we have enabled the HSM clock by the time
pm_runtime_resume_and_get in vc4_hdmi_encoder_pre_crtc_configure()
returns, the clock is enabled so we can't adjust its rate based on the
resolution after the runtime_resume.

But if we do it before runtime_resume, then the clk_set_min_rate() call
in runtime_resume will override the "resolution rate" with the "absolute
minimum" rate.

Removing CLK_SET_RATE_GATE would solve this entirely, since we could
move the pre_crtc_configure clk_set_min_rate() call after
runtime_resume, and it would work just fine.

But the interaction with the power domain is weird to me. I'm not even
sure how it's even supposed to work if one has to disable the clock (and
its power domain?) to change the rate, but the configuration won't stick?

So I guess my question is how can we untangle this a bit so that we can
simplify the users of that driver?

Thanks!
Maxime

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

Re: BCM2835 clock driver, power domains and CLK_SET_RATE_GATE

[Stefan Wahren]

Hi Maxime,

Am 21.10.22 um 16:05 schrieb maxime@cerno.tech:
> Hi,
>
> I had to fix a regression today that was mostly due to the fact that
> clk-bcm2835 and clk-raspberrypi have fairly diverging behaviour.
>
> https://lore.kernel.org/dri-devel/20221021131339.2203291-1-maxime@cerno.tech/
>
> The "fix" is rather suboptimal and I'd like to address it properly.
>
> Even though the commit message tries to explain everything and why other
> solutions wouldn't work, the underlying cause is that clk-bcm2835 has a
> bunch of restrictions that I don't really get and we have to work
> around. I'm not sure I have the full picture either, so please take the
> following with a grain of salt.
>
> The first one is that the HSM clock discussed in that patch is part of
> the HDMI power domain, so it has to be powered for the clock to retain
> its configuration.
>
> The side effect is that we need to initialize the clock at boot time so
> that it has some rate. This is pretty important since that clock feeds
> the register and thus a failure to initialize would lock the CPU on a
> register access.
>
> The second one is that since clk-bcm2835 has CLK_SET_RATE_GATE, we have
> to call clk_set_min_rate() before clk_prepare_enable(), or
> clk_set_min_rate() is likely to fail.
>
> So we need in runtime_resume to call clk_set_min_rate with an absolute
> minimum for the HDMI controller to be functional and then enable the
> clock.
>
> But then, since we have enabled the HSM clock by the time
> pm_runtime_resume_and_get in vc4_hdmi_encoder_pre_crtc_configure()
> returns, the clock is enabled so we can't adjust its rate based on the
> resolution after the runtime_resume.
>
> But if we do it before runtime_resume, then the clk_set_min_rate() call
> in runtime_resume will override the "resolution rate" with the "absolute
> minimum" rate.
>
> Removing CLK_SET_RATE_GATE would solve this entirely, since we could
> move the pre_crtc_configure clk_set_min_rate() call after
> runtime_resume, and it would work just fine.
>
> But the interaction with the power domain is weird to me. I'm not even
> sure how it's even supposed to work if one has to disable the clock (and
> its power domain?) to change the rate, but the configuration won't stick?
>
> So I guess my question is how can we untangle this a bit so that we can
> simplify the users of that driver?

sorry, but i'm unable to help you here. This is out of my scope :-(

Stefan

>
> Thanks!
> Maxime

Re: BCM2835 clock driver, power domains and CLK_SET_RATE_GATE

[maxime]

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

Hi Dom,

On Fri, Oct 21, 2022 at 03:26:23PM +0100, Dom Cobley wrote:
> I believe our view is that clk-bcm2835 is fundamentally unsafe, as it
> writes to registers the firmware is in control of, and any calls should be
> migrated to clk-raspberrypi which should be safe.
> 
> If there are any missing features of clk-raspberrypi, then we can support
> them.

Thanks for your input. I'm inclined to send a patch switching over to
the firmware clocks for the HDMI controller then.

There's one thing that isn't really clear to me, is there any difference
in terms of firmware capabilities between all the RPi0-3 versions?

Thanks!
Maxime

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