Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Mon, 7 Sep 2020 18:21:49 +0200
From:   Maxime Ripard <maxime@cerno.tech>
To:     Dave Stevenson <dave.stevenson@raspberrypi.com>
Cc:     Nicolas Saenz Julienne <nsaenzjulienne@suse.de>,
        Eric Anholt <eric@anholt.net>,
        DRI Development <dri-devel@lists.freedesktop.org>,
        linux-rpi-kernel@lists.infradead.org,
        bcm-kernel-feedback-list@broadcom.com,
        linux-arm-kernel@lists.infradead.org,
        LKML <linux-kernel@vger.kernel.org>,
        Tim Gover <tim.gover@raspberrypi.com>,
        Phil Elwell <phil@raspberrypi.com>,
        Hoegeun Kwon <hoegeun.kwon@samsung.com>
Subject: Re: [PATCH v5 75/80] drm/vc4: hdmi: Add pixel BVB clock control
Message-ID: <20200907162149.plabkjrgajjqbtiu@gilmour.lan>
References: <cover.dddc064d8bb83e46744336af67dcb13139e5747d.1599120059.git-series.maxime@cerno.tech>
 <d757ddd6549da140f178563e5fd2bf1d129913fd.1599120059.git-series.maxime@cerno.tech>
 <CAPY8ntC_Tobv+4TtgtYhvAst00_zsfxZdZd=torcb57SQ0CMQA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha256;
        protocol="application/pgp-signature"; boundary="rvxp5il2lqnzas2n"
Content-Disposition: inline
In-Reply-To: <CAPY8ntC_Tobv+4TtgtYhvAst00_zsfxZdZd=torcb57SQ0CMQA@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


--rvxp5il2lqnzas2n
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

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@cerno.tech> wrote:
> >
> > From: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> >
> > The BCM2711 has another clock that needs to be ramped up depending on t=
he
> > pixel rate: the pixel BVB clock. Add the code to adjust that clock when
> > changing the mode.
> >
> > Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> > [Maxime: Changed the commit log, used clk_set_min_rate]
> > Signed-off-by: Maxime Ripard <maxime@cerno.tech>
> > Link: https://lore.kernel.org/r/20200901040759.29992-3-hoegeun.kwon@sam=
sung.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_h=
dmi.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
>=20
> 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.
>=20
> 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(st=
ruct 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(st=
ruct drm_encoder *encoder)
> >                 return;
> >         }
> >
> > +       /*
> > +        * FIXME: When the pixel freq is 594MHz (4k60), this needs to b=
e setup
> > +        * at 150MHz.
> > +        */
>=20
> Typo here. For 4k60 we need 300MHz (pixel clock / 2)
>=20
> Otherwise
> Reviewed-by: Dave Stevenson <dave.stevenson@raspberrypi.com>

I've fixed it, thanks!
Maxime

--rvxp5il2lqnzas2n
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iHUEABYIAB0WIQRcEzekXsqa64kGDp7j7w1vZxhRxQUCX1ZeHQAKCRDj7w1vZxhR
xe1NAQDBoDF7y+tHlr5DcJFlIwZ3QQRh7Hc5OIfBiA9mYLIdkwD/fneGxtomigY5
/txCXfqzYna4dCVjluu3UqPV14GrHwg=
=gt4+
-----END PGP SIGNATURE-----

--rvxp5il2lqnzas2n--