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;
MIME-Version: 1.0
References: <cover.dddc064d8bb83e46744336af67dcb13139e5747d.1599120059.git-series.maxime@cerno.tech>
 <d757ddd6549da140f178563e5fd2bf1d129913fd.1599120059.git-series.maxime@cerno.tech>
In-Reply-To: <d757ddd6549da140f178563e5fd2bf1d129913fd.1599120059.git-series.maxime@cerno.tech>
From:   Dave Stevenson <dave.stevenson@raspberrypi.com>
Date:   Fri, 4 Sep 2020 10:46:26 +0100
Message-ID: <CAPY8ntC_Tobv+4TtgtYhvAst00_zsfxZdZd=torcb57SQ0CMQA@mail.gmail.com>
Subject: Re: [PATCH v5 75/80] drm/vc4: hdmi: Add pixel BVB clock control
To:     Maxime Ripard <maxime@cerno.tech>
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>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Maxime

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 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@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@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 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.

>
>  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@raspberrypi.com>

> +       ret = clk_set_min_rate(vc4_hdmi->pixel_bvb_clock,
> +                              (hsm_rate > VC4_HSM_MID_CLOCK ? 150000000 : 75000000));
> +       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