Re: BCM2835 clock driver, power domains and CLK_SET_RATE_GATE

[Stefan Wahren <stefan.wahren@i2se.com>]

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