| Message ID | 20230126-rpi-display-fw-clk-cleanup-v1-2-d646ff6fb842@cerno.tech (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/vc4: hdmi: Firmware clocks cleanup | expand |
On 1/26/23 18:05, Maxime Ripard wrote: > On the RaspberryPi0-3, the HSM clock was provided by the clk-bcm2835 > driver, but on the Pi4 it was provided by the firmware through the > clk-raspberrypi driver. > > The clk-bcm2835 driver registers the HSM clock using the > CLK_SET_RATE_GATE flag that prevents any modification to the rate while > the clock is active. > > This meant that we needed to call clk_set_min_rate() before our call to > pm_runtime_resume_and_get() since our runtime_resume implementation > needs to enable the HSM clock for the HDMI controller registers to be > functional. > > However, the HSM clock is part of the HDMI power domain which might not > be powered prior to the pm_runtime_resume_and_get() call, so we could > end up changing the rate of the HSM clock while its power domain was > disabled. > > We recently changed the backing driver for the RaspberryPi0-3 to > clk-raspberrypi though, which doesn't have such restrictions. We can > thus move the clk_set_min_rate() after our call to runtime_resume and > avoid the access while the power domain is disabled. > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- I'm not familiar with the RPi clock hierarchy but the commit message explains why a clk_set_min_rate() was needed before the pm_runtime_resume_and_get(), and why that isn't needed anymore after the switch to clk-raspberrypi driver. And certainly, the correct thing to do is to enable the power domain that a controller is part of, before attempting to change the rate for one of its clocks. So the patch looks good to me. Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c index 98fa306dbd24..9dd722b9ae3a 100644 --- a/drivers/gpu/drm/vc4/vc4_hdmi.c +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c @@ -1466,6 +1466,12 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, if (!drm_dev_enter(drm, &idx)) goto out; + ret = pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev); + if (ret < 0) { + DRM_ERROR("Failed to retain power domain: %d\n", ret); + goto err_dev_exit; + } + /* * As stated in RPi's vc4 firmware "HDMI state machine (HSM) clock must * be faster than pixel clock, infinitesimally faster, tested in @@ -1488,13 +1494,7 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder, ret = clk_set_min_rate(vc4_hdmi->hsm_clock, hsm_rate); if (ret) { DRM_ERROR("Failed to set HSM clock rate: %d\n", ret); - goto err_dev_exit; - } - - ret = pm_runtime_resume_and_get(&vc4_hdmi->pdev->dev); - if (ret < 0) { - DRM_ERROR("Failed to retain power domain: %d\n", ret); - goto err_dev_exit; + goto err_put_runtime_pm; } ret = clk_set_rate(vc4_hdmi->pixel_clock, tmds_char_rate);
On the RaspberryPi0-3, the HSM clock was provided by the clk-bcm2835 driver, but on the Pi4 it was provided by the firmware through the clk-raspberrypi driver. The clk-bcm2835 driver registers the HSM clock using the CLK_SET_RATE_GATE flag that prevents any modification to the rate while the clock is active. This meant that we needed to call clk_set_min_rate() before our call to pm_runtime_resume_and_get() since our runtime_resume implementation needs to enable the HSM clock for the HDMI controller registers to be functional. However, the HSM clock is part of the HDMI power domain which might not be powered prior to the pm_runtime_resume_and_get() call, so we could end up changing the rate of the HSM clock while its power domain was disabled. We recently changed the backing driver for the RaspberryPi0-3 to clk-raspberrypi though, which doesn't have such restrictions. We can thus move the clk_set_min_rate() after our call to runtime_resume and avoid the access while the power domain is disabled. Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/gpu/drm/vc4/vc4_hdmi.c | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-)