Re: [PATCH v5 75/80] drm/vc4: hdmi: Add pixel BVB clock control

2020-09-08 Thread Maxime Ripard
Hi,

On Fri, Sep 04, 2020 at 10:46:26AM +0100, Dave Stevenson wrote:
> On Thu, 3 Sep 2020 at 09:03, Maxime Ripard  wrote:
> >
> > From: Hoegeun Kwon 
> >
> > The BCM2711 has another clock that needs to be ramped up depending on the
> > pixel rate: the pixel BVB clock. Add the code to adjust that clock when
> > changing the mode.
> >
> > Signed-off-by: Hoegeun Kwon 
> > [Maxime: Changed the commit log, used clk_set_min_rate]
> > Signed-off-by: Maxime Ripard 
> > Link: 
> > https://lore.kernel.org/r/20200901040759.29992-3-hoegeun.k...@samsung.com
> > ---
> >  drivers/gpu/drm/vc4/vc4_hdmi.c | 23 +++
> >  drivers/gpu/drm/vc4/vc4_hdmi.h |  1 +
> >  2 files changed, 24 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > index ab7abb409de2..39508107dafd 100644
> > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > @@ -54,6 +54,7 @@
> >  #include "vc4_regs.h"
> >
> >  #define CEC_CLOCK_FREQ 4
> > +#define VC4_HSM_MID_CLOCK 149985000
> 
> I didn't flag it earlier, but this is a bit of a weird name for the
> define. I know it wants to be concise, but it made me do a double take
> as to what it is for.
> I'm currently applying all these patches to our Raspberry Pi tree and
> actually CEC needs a fixed HSM on Pi0-3 to avoid recomputing all the
> timings. So I have a VC4_HSM_CLOCK define which is the fixed clock
> rate for Pi 0-3.
> This one is more a threshold for HSM to control BVB, and my brain
> starts to hurt over what it should be called.
> 
> Unless there are other comments around this patchset (and I hope to
> read through the remaining ones today), then I don't consider it a
> blocker, but we can probably do better as and when we add the next
> threshold for 4k60.
> My current understanding is that the clock has to be an integer divide
> of 600MHz, and at least the pixel rate / 2, so the only link to HSM is
> due to HSM being 101% of pixel rate, but I will try to find
> confirmation of that.

I'm currently working on the 4k60 support, so it will go away soon
(using your suggestion) so there's no need to overthink it :)

> >  static int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused)
> >  {
> > @@ -344,6 +345,7 @@ static void vc4_hdmi_encoder_post_crtc_powerdown(struct 
> > drm_encoder *encoder)
> > HDMI_WRITE(HDMI_VID_CTL,
> >HDMI_READ(HDMI_VID_CTL) & ~VC4_HD_VID_CTL_ENABLE);
> >
> > +   clk_disable_unprepare(vc4_hdmi->pixel_bvb_clock);
> > clk_disable_unprepare(vc4_hdmi->hsm_clock);
> > clk_disable_unprepare(vc4_hdmi->pixel_clock);
> >
> > @@ -516,6 +518,27 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct 
> > drm_encoder *encoder)
> > return;
> > }
> >
> > +   /*
> > +* FIXME: When the pixel freq is 594MHz (4k60), this needs to be 
> > setup
> > +* at 150MHz.
> > +*/
> 
> Typo here. For 4k60 we need 300MHz (pixel clock / 2)
> 
> Otherwise
> Reviewed-by: Dave Stevenson 

I've fixed it, thanks!
Maxime


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


Re: [PATCH v5 75/80] drm/vc4: hdmi: Add pixel BVB clock control

2020-09-04 Thread Dave Stevenson
Hi Maxime

On Thu, 3 Sep 2020 at 09:03, Maxime Ripard  wrote:
>
> From: Hoegeun Kwon 
>
> The BCM2711 has another clock that needs to be ramped up depending on the
> pixel rate: the pixel BVB clock. Add the code to adjust that clock when
> changing the mode.
>
> Signed-off-by: Hoegeun Kwon 
> [Maxime: Changed the commit log, used clk_set_min_rate]
> Signed-off-by: Maxime Ripard 
> Link: 
> https://lore.kernel.org/r/20200901040759.29992-3-hoegeun.k...@samsung.com
> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 23 +++
>  drivers/gpu/drm/vc4/vc4_hdmi.h |  1 +
>  2 files changed, 24 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index ab7abb409de2..39508107dafd 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -54,6 +54,7 @@
>  #include "vc4_regs.h"
>
>  #define CEC_CLOCK_FREQ 4
> +#define VC4_HSM_MID_CLOCK 149985000

I didn't flag it earlier, but this is a bit of a weird name for the
define. I know it wants to be concise, but it made me do a double take
as to what it is for.
I'm currently applying all these patches to our Raspberry Pi tree and
actually CEC needs a fixed HSM on Pi0-3 to avoid recomputing all the
timings. So I have a VC4_HSM_CLOCK define which is the fixed clock
rate for Pi 0-3.
This one is more a threshold for HSM to control BVB, and my brain
starts to hurt over what it should be called.

Unless there are other comments around this patchset (and I hope to
read through the remaining ones today), then I don't consider it a
blocker, but we can probably do better as and when we add the next
threshold for 4k60.
My current understanding is that the clock has to be an integer divide
of 600MHz, and at least the pixel rate / 2, so the only link to HSM is
due to HSM being 101% of pixel rate, but I will try to find
confirmation of that.

>
>  static int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused)
>  {
> @@ -344,6 +345,7 @@ static void vc4_hdmi_encoder_post_crtc_powerdown(struct 
> drm_encoder *encoder)
> HDMI_WRITE(HDMI_VID_CTL,
>HDMI_READ(HDMI_VID_CTL) & ~VC4_HD_VID_CTL_ENABLE);
>
> +   clk_disable_unprepare(vc4_hdmi->pixel_bvb_clock);
> clk_disable_unprepare(vc4_hdmi->hsm_clock);
> clk_disable_unprepare(vc4_hdmi->pixel_clock);
>
> @@ -516,6 +518,27 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct 
> drm_encoder *encoder)
> return;
> }
>
> +   /*
> +* FIXME: When the pixel freq is 594MHz (4k60), this needs to be setup
> +* at 150MHz.
> +*/

Typo here. For 4k60 we need 300MHz (pixel clock / 2)

Otherwise
Reviewed-by: Dave Stevenson 

> +   ret = clk_set_min_rate(vc4_hdmi->pixel_bvb_clock,
> +  (hsm_rate > VC4_HSM_MID_CLOCK ? 15000 : 
> 7500));
> +   if (ret) {
> +   DRM_ERROR("Failed to set pixel bvb clock rate: %d\n", ret);
> +   clk_disable_unprepare(vc4_hdmi->hsm_clock);
> +   clk_disable_unprepare(vc4_hdmi->pixel_clock);
> +   return;
> +   }
> +
> +   ret = clk_prepare_enable(vc4_hdmi->pixel_bvb_clock);
> +   if (ret) {
> +   DRM_ERROR("Failed to turn on pixel bvb clock: %d\n", ret);
> +   clk_disable_unprepare(vc4_hdmi->hsm_clock);
> +   clk_disable_unprepare(vc4_hdmi->pixel_clock);
> +   return;
> +   }
> +
> if (vc4_hdmi->variant->reset)
> vc4_hdmi->variant->reset(vc4_hdmi);
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
> index 34138e0dd4a6..59639b405b7f 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.h
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
> @@ -119,6 +119,7 @@ struct vc4_hdmi {
> struct clk *pixel_clock;
> struct clk *hsm_clock;
> struct clk *audio_clock;
> +   struct clk *pixel_bvb_clock;
>
> struct debugfs_regset32 hdmi_regset;
> struct debugfs_regset32 hd_regset;
> --
> git-series 0.9.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel


[PATCH v5 75/80] drm/vc4: hdmi: Add pixel BVB clock control

2020-09-04 Thread Maxime Ripard
From: Hoegeun Kwon 

The BCM2711 has another clock that needs to be ramped up depending on the
pixel rate: the pixel BVB clock. Add the code to adjust that clock when
changing the mode.

Signed-off-by: Hoegeun Kwon 
[Maxime: Changed the commit log, used clk_set_min_rate]
Signed-off-by: Maxime Ripard 
Link: https://lore.kernel.org/r/20200901040759.29992-3-hoegeun.k...@samsung.com
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 23 +++
 drivers/gpu/drm/vc4/vc4_hdmi.h |  1 +
 2 files changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index ab7abb409de2..39508107dafd 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -54,6 +54,7 @@
 #include "vc4_regs.h"
 
 #define CEC_CLOCK_FREQ 4
+#define VC4_HSM_MID_CLOCK 149985000
 
 static int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused)
 {
@@ -344,6 +345,7 @@ static void vc4_hdmi_encoder_post_crtc_powerdown(struct 
drm_encoder *encoder)
HDMI_WRITE(HDMI_VID_CTL,
   HDMI_READ(HDMI_VID_CTL) & ~VC4_HD_VID_CTL_ENABLE);
 
+   clk_disable_unprepare(vc4_hdmi->pixel_bvb_clock);
clk_disable_unprepare(vc4_hdmi->hsm_clock);
clk_disable_unprepare(vc4_hdmi->pixel_clock);
 
@@ -516,6 +518,27 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct 
drm_encoder *encoder)
return;
}
 
+   /*
+* FIXME: When the pixel freq is 594MHz (4k60), this needs to be setup
+* at 150MHz.
+*/
+   ret = clk_set_min_rate(vc4_hdmi->pixel_bvb_clock,
+  (hsm_rate > VC4_HSM_MID_CLOCK ? 15000 : 
7500));
+   if (ret) {
+   DRM_ERROR("Failed to set pixel bvb clock rate: %d\n", ret);
+   clk_disable_unprepare(vc4_hdmi->hsm_clock);
+   clk_disable_unprepare(vc4_hdmi->pixel_clock);
+   return;
+   }
+
+   ret = clk_prepare_enable(vc4_hdmi->pixel_bvb_clock);
+   if (ret) {
+   DRM_ERROR("Failed to turn on pixel bvb clock: %d\n", ret);
+   clk_disable_unprepare(vc4_hdmi->hsm_clock);
+   clk_disable_unprepare(vc4_hdmi->pixel_clock);
+   return;
+   }
+
if (vc4_hdmi->variant->reset)
vc4_hdmi->variant->reset(vc4_hdmi);
 
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index 34138e0dd4a6..59639b405b7f 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -119,6 +119,7 @@ struct vc4_hdmi {
struct clk *pixel_clock;
struct clk *hsm_clock;
struct clk *audio_clock;
+   struct clk *pixel_bvb_clock;
 
struct debugfs_regset32 hdmi_regset;
struct debugfs_regset32 hd_regset;
-- 
git-series 0.9.1
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel