[PATCH 0/4] drm/vc4: hdmi: Firmware clocks cleanup

[PATCH 0/4] drm/vc4: hdmi: Firmware clocks cleanup

[Maxime Ripard]

Hi,

In order to accomodate the Pi0-3 using the clk-bcm2835 and the Pi4 using the
clk-raspberrypi clock drivers for the HDMI clocks, we piled a number of
workarounds over the years.

Since 6.2, we've switched the Pi0-3 to the clk-raspberrypi driver, so we can
now remove those workarounds.

Let me know what you think,
Maxime

To: Emma Anholt <emma@anholt.net>
To: Maxime Ripard <mripard@kernel.org>
To: David Airlie <airlied@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
Signed-off-by: Maxime Ripard <maxime@cerno.tech>

---
Maxime Ripard (4):
      drm/vc4: hdmi: Replace hardcoded value by define
      drm/vc4: hdmi: Enable power domain before setting minimum
      Revert "drm/vc4: hdmi: Fix HSM clock too low on Pi4"
      Revert "drm/vc4: hdmi: Enforce the minimum rate at runtime_resume"

 drivers/gpu/drm/vc4/vc4_hdmi.c | 46 ++++++++++++------------------------------
 drivers/gpu/drm/vc4/vc4_hdmi.h |  1 -
 2 files changed, 13 insertions(+), 34 deletions(-)
---
base-commit: 9fbee811e479aca2f3523787cae1f46553141b40
change-id: 20230126-rpi-display-fw-clk-cleanup-19d1b051a04c

Best regards,
-- 
Maxime Ripard <maxime@cerno.tech>

[PATCH 1/4] drm/vc4: hdmi: Replace hardcoded value by define

[Maxime Ripard]

The 120MHz value hardcoded in the call to max_t to compute the HSM rate
is defined in the driver as HSM_MIN_CLOCK_FREQ, let's switch to it so
that it's more readable.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 14628864487a..98fa306dbd24 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -1482,7 +1482,9 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder,
 	 * Additionally, the AXI clock needs to be at least 25% of
 	 * pixel clock, but HSM ends up being the limiting factor.
 	 */
-	hsm_rate = max_t(unsigned long, 120000000, (tmds_char_rate / 100) * 101);
+	hsm_rate = max_t(unsigned long,
+			 HSM_MIN_CLOCK_FREQ,
+			 (tmds_char_rate / 100) * 101);
 	ret = clk_set_min_rate(vc4_hdmi->hsm_clock, hsm_rate);
 	if (ret) {
 		DRM_ERROR("Failed to set HSM clock rate: %d\n", ret);

-- 
2.39.1

Re: [PATCH 1/4] drm/vc4: hdmi: Replace hardcoded value by define

[Javier Martinez Canillas]

On 1/26/23 18:05, Maxime Ripard wrote:
> The 120MHz value hardcoded in the call to max_t to compute the HSM rate
> is defined in the driver as HSM_MIN_CLOCK_FREQ, let's switch to it so
> that it's more readable.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 2/4] drm/vc4: hdmi: Enable power domain before setting minimum

[Maxime Ripard]

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(-)

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);

-- 
2.39.1

Re: [PATCH 2/4] drm/vc4: hdmi: Enable power domain before setting minimum

[Javier Martinez Canillas]

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>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 3/4] Revert "drm/vc4: hdmi: Fix HSM clock too low on Pi4"

[Maxime Ripard]

This reverts commit 3bc6a37f59f21a8bfaf74d0975b2eb0b2d52a065.

Commit 3bc6a37f59f2 ("drm/vc4: hdmi: Fix HSM clock too low on Pi4") was
introduced to work around an issue partly due to the clk-bcm2835 driver
on the RaspberryPi0-3.

Since we're not using that driver for our HDMI clocks, we can now revert
that inelegant solution.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 21 ++++-----------------
 drivers/gpu/drm/vc4/vc4_hdmi.h |  1 -
 2 files changed, 4 insertions(+), 18 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 9dd722b9ae3a..e82fe17c9532 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -3189,16 +3189,9 @@ static int vc4_hdmi_init_resources(struct drm_device *drm,
 		DRM_ERROR("Failed to get HDMI state machine clock\n");
 		return PTR_ERR(vc4_hdmi->hsm_clock);
 	}
-
 	vc4_hdmi->audio_clock = vc4_hdmi->hsm_clock;
 	vc4_hdmi->cec_clock = vc4_hdmi->hsm_clock;
 
-	vc4_hdmi->hsm_rpm_clock = devm_clk_get(dev, "hdmi");
-	if (IS_ERR(vc4_hdmi->hsm_rpm_clock)) {
-		DRM_ERROR("Failed to get HDMI state machine clock\n");
-		return PTR_ERR(vc4_hdmi->hsm_rpm_clock);
-	}
-
 	return 0;
 }
 
@@ -3281,12 +3274,6 @@ static int vc5_hdmi_init_resources(struct drm_device *drm,
 		return PTR_ERR(vc4_hdmi->hsm_clock);
 	}
 
-	vc4_hdmi->hsm_rpm_clock = devm_clk_get(dev, "hdmi");
-	if (IS_ERR(vc4_hdmi->hsm_rpm_clock)) {
-		DRM_ERROR("Failed to get HDMI state machine clock\n");
-		return PTR_ERR(vc4_hdmi->hsm_rpm_clock);
-	}
-
 	vc4_hdmi->pixel_bvb_clock = devm_clk_get(dev, "bvb");
 	if (IS_ERR(vc4_hdmi->pixel_bvb_clock)) {
 		DRM_ERROR("Failed to get pixel bvb clock\n");
@@ -3350,7 +3337,7 @@ static int vc4_hdmi_runtime_suspend(struct device *dev)
 {
 	struct vc4_hdmi *vc4_hdmi = dev_get_drvdata(dev);
 
-	clk_disable_unprepare(vc4_hdmi->hsm_rpm_clock);
+	clk_disable_unprepare(vc4_hdmi->hsm_clock);
 
 	return 0;
 }
@@ -3368,11 +3355,11 @@ static int vc4_hdmi_runtime_resume(struct device *dev)
 	 * its frequency while the power domain is active so that it
 	 * keeps its rate.
 	 */
-	ret = clk_set_min_rate(vc4_hdmi->hsm_rpm_clock, HSM_MIN_CLOCK_FREQ);
+	ret = clk_set_min_rate(vc4_hdmi->hsm_clock, HSM_MIN_CLOCK_FREQ);
 	if (ret)
 		return ret;
 
-	ret = clk_prepare_enable(vc4_hdmi->hsm_rpm_clock);
+	ret = clk_prepare_enable(vc4_hdmi->hsm_clock);
 	if (ret)
 		return ret;
 
@@ -3385,7 +3372,7 @@ static int vc4_hdmi_runtime_resume(struct device *dev)
 	 * case, it will lead to a silent CPU stall. Let's make sure we
 	 * prevent such a case.
 	 */
-	rate = clk_get_rate(vc4_hdmi->hsm_rpm_clock);
+	rate = clk_get_rate(vc4_hdmi->hsm_clock);
 	if (!rate) {
 		ret = -EINVAL;
 		goto err_disable_clk;
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index dc3ccd8002a0..e3619836ca17 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -164,7 +164,6 @@ struct vc4_hdmi {
 	struct clk *cec_clock;
 	struct clk *pixel_clock;
 	struct clk *hsm_clock;
-	struct clk *hsm_rpm_clock;
 	struct clk *audio_clock;
 	struct clk *pixel_bvb_clock;
 

-- 
2.39.1

Re: [PATCH 3/4] Revert "drm/vc4: hdmi: Fix HSM clock too low on Pi4"

[Javier Martinez Canillas]

On 1/26/23 18:05, Maxime Ripard wrote:
> This reverts commit 3bc6a37f59f21a8bfaf74d0975b2eb0b2d52a065.
> 
> Commit 3bc6a37f59f2 ("drm/vc4: hdmi: Fix HSM clock too low on Pi4") was
> introduced to work around an issue partly due to the clk-bcm2835 driver
> on the RaspberryPi0-3.
> 
> Since we're not using that driver for our HDMI clocks, we can now revert
> that inelegant solution.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

[PATCH 4/4] Revert "drm/vc4: hdmi: Enforce the minimum rate at runtime_resume"

[Maxime Ripard]

This reverts commit ae71ab585c819f83aec84f91eb01157a90552ef2.

Commit ae71ab585c81 ("drm/vc4: hdmi: Enforce the minimum rate at
runtime_resume") was introduced to work around an issue partly due to
the clk-bcm2835 driver on the RaspberryPi0-3.

Since we're not using that driver for our HDMI clocks, we can now revert
it.

Signed-off-by: Maxime Ripard <maxime@cerno.tech>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 9 ---------
 1 file changed, 9 deletions(-)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index e82fe17c9532..18d84aab54bb 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -3350,15 +3350,6 @@ static int vc4_hdmi_runtime_resume(struct device *dev)
 	unsigned long rate;
 	int ret;
 
-	/*
-	 * The HSM clock is in the HDMI power domain, so we need to set
-	 * its frequency while the power domain is active so that it
-	 * keeps its rate.
-	 */
-	ret = clk_set_min_rate(vc4_hdmi->hsm_clock, HSM_MIN_CLOCK_FREQ);
-	if (ret)
-		return ret;
-
 	ret = clk_prepare_enable(vc4_hdmi->hsm_clock);
 	if (ret)
 		return ret;

-- 
2.39.1

Re: [PATCH 4/4] Revert "drm/vc4: hdmi: Enforce the minimum rate at runtime_resume"

[Javier Martinez Canillas]

On 1/26/23 18:05, Maxime Ripard wrote:
> This reverts commit ae71ab585c819f83aec84f91eb01157a90552ef2.
> 
> Commit ae71ab585c81 ("drm/vc4: hdmi: Enforce the minimum rate at
> runtime_resume") was introduced to work around an issue partly due to
> the clk-bcm2835 driver on the RaspberryPi0-3.
> 
> Since we're not using that driver for our HDMI clocks, we can now revert
> it.
> 
> Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 0/4] drm/vc4: hdmi: Firmware clocks cleanup

[Maxime Ripard]

On Thu, 26 Jan 2023 18:05:46 +0100, Maxime Ripard wrote:
> In order to accomodate the Pi0-3 using the clk-bcm2835 and the Pi4 using the
> clk-raspberrypi clock drivers for the HDMI clocks, we piled a number of
> workarounds over the years.
> 
> Since 6.2, we've switched the Pi0-3 to the clk-raspberrypi driver, so we can
> now remove those workarounds.
> 
> [...]

Applied to drm/drm-misc (drm-misc-next).

Thanks!
Maxime