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

[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 <maxime@xxxxxxxxxx> wrote:
> >
> > From: Hoegeun Kwon <hoegeun.kwon@xxxxxxxxxxx>
> >
> > 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 <hoegeun.kwon@xxxxxxxxxxx>
> > [Maxime: Changed the commit log, used clk_set_min_rate]
> > Signed-off-by: Maxime Ripard <maxime@xxxxxxxxxx>
> > Link: https://lore.kernel.org/r/20200901040759.29992-3-hoegeun.kwon@xxxxxxxxxxx
> > ---
> >  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 40000
> > +#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 <dave.stevenson@xxxxxxxxxxxxxxx>

I've fixed it, thanks!
Maxime

Attachment: signature.asc
Description: PGP signature