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;
Subject: Re: [PATCH 3/3] drm/vc4: hdmi: Add pixel bvb clock control
To:     Dave Stevenson <dave.stevenson@raspberrypi.com>,
        Maxime Ripard <maxime@cerno.tech>
Cc:     Stefan Wahren <stefan.wahren@i2se.com>,
        Nicolas Saenz Julienne <nsaenzjulienne@suse.de>,
        Eric Anholt <eric@anholt.net>, devicetree@vger.kernel.org,
        Tim Gover <tim.gover@raspberrypi.com>, kdasu.kdev@gmail.com,
        sboyd@kernel.org, mturquette@baylibre.com,
        LKML <linux-kernel@vger.kernel.org>,
        DRI Development <dri-devel@lists.freedesktop.org>,
        Phil Elwell <phil@raspberrypi.com>, robh+dt@kernel.org,
        bcm-kernel-feedback-list@broadcom.com,
        linux-rpi-kernel@lists.infradead.org, linux-clk@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        Hoegeun Kwon <hoegeun.kwon@samsung.com>
From:   Hoegeun Kwon <hoegeun.kwon@samsung.com>
Message-ID: <f84797d1-22b6-7673-d0dd-398d339a5ed2@samsung.com>
Date:   Tue, 1 Sep 2020 11:07:26 +0900
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
        Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <CAPY8ntDeLOb+GvpLDX1BFrhtYyGQzJ01pWHiS30r-2ZUJTg8Gw@mail.gmail.com>
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Content-Type: text/plain; charset="utf-8"
CMS-TYPE: 101P
DLP-Filter: Pass
References: <20200821071045.24501-1-hoegeun.kwon@samsung.com>
        <CGME20200821071122epcas1p3d00dda4665f94192ac5e9ee829d0557d@epcas1p3.samsung.com>
        <20200821071045.24501-4-hoegeun.kwon@samsung.com>
        <61c199bf-852f-82d3-089a-a0a435343acf@i2se.com>
        <80749dcd-d4b2-68a1-f3ca-c19a120f6f7b@samsung.com>
        <84c423e8-25a6-8f23-cc80-7a17ce03fd1d@i2se.com>
        <a19de8d5-2b01-cb62-38a2-b0732068025c@samsung.com>
        <a3231281-3bd0-e7c9-1bb0-f05848621e82@i2se.com>
        <20200828152510.jhhqvka6fmouozff@gilmour.lan>
        <CAPY8ntDeLOb+GvpLDX1BFrhtYyGQzJ01pWHiS30r-2ZUJTg8Gw@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Thank you reviews by Dave, Maxime and Stefan.

On 8/29/20 12:37 AM, Dave Stevenson wrote:
> Hi Maxime, Stefan, and Hoegeun
>
> On Fri, 28 Aug 2020 at 16:25, Maxime Ripard <maxime@cerno.tech> wrote:
>> Hi,
>>
>> On Fri, Aug 28, 2020 at 02:45:49PM +0200, Stefan Wahren wrote:
>>> Am 28.08.20 um 08:30 schrieb Hoegeun Kwon:
>>>> On 8/27/20 6:49 PM, Stefan Wahren wrote:
>>>>> Am 27.08.20 um 06:35 schrieb Hoegeun Kwon:
>>>>>> Hi Stefan,
>>>>>>
>>>>>> Thank you for your review.
>>>>>>
>>>>>>
>>>>>> On 8/26/20 7:04 PM, Stefan Wahren wrote:
>>>>>>> Hi Hoeguen,
>>>>>>>
>>>>>>> Am 21.08.20 um 09:10 schrieb Hoegeun Kwon:
>>>>>>>> There is a problem that the output does not work at a resolution
>>>>>>>> exceeding FHD. To solve this, we need to adjust the bvb clock at a
>>>>>>>> resolution exceeding FHD.
>>>>>>> this patch introduces a mandatory clock, please update
>>>>>>> brcm,bcm2835-hdmi.yaml first.
>>>>>>>
>>>>>>> Is this clock physically available on BCM283x or only on BCM2711?
>>>>>> As far as I know, BCM2711 raspberry pi 4 supports 4k,
>>>>>>
>>>>>> don't supported on pi 3 and pi 3+.
>>>>>>
>>>>>> Since 4k is not supported in versions prior to Raspberry Pi 4,
>>>>>>
>>>>>> I don't think we need to modify the bvb clock.
>>>>>>
>>>>>>
>>>>>> So I think it is better to update 'brcm,bcm2711-hdmi.yaml'
>>>>>>
>>>>>> instead of 'brcm,bcm2835-hdmi.yaml'.
>>>>> You are correct please update only brcm,bcm2711-hdmi.yaml.
>>>>>
>>>>> My concern was that the function vc4_hdmi_encoder_pre_crtc_configure()
>>>>> is called on a non-bcm2711 platform or on a Raspberry Pi 4 with an older
>>>>> DTB. So making the BVB clock optional might be better?
>>>> You are right, if use old dtb, we have a problem with the hdmi driver.
>>>>
>>>> So how about modifying it like this?
>>>>
>>>> @@ -1614,8 +1614,8 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi
>>>> *vc4_hdmi)
>>>>
>>>>           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");
>>>> -               return PTR_ERR(vc4_hdmi->pixel_bvb_clock);
>>>> +               DRM_WARN("Failed to get pixel bvb clock\n");
>>>> +               vc4_hdmi->pixel_bvb_clock = NULL;
>>>>           }
>>> i think the better solution would be devm_clk_get_optional(), which
>>> return NULL in case the clock doesn't exist.
>> It's not really optional though. BCM2711 will require it in order to run
>> properly (as Hoegeun experienced), and the previous SoCs won't.
>>
>> If we use clk_get_optional and that the DT is missing the clock on the
>> BCM2711, we will silently ignore it which doesn't sound great.
> Am I missing something here? (I know I missed this earlier)
> We're in vc5_hdmi_init_resources, which is inherently bcm2711 only.
> bcm283x will go through vc4_hdmi_init_resources.
>
> As long as vc4_hdmi_init_resources has left vc4_hdmi->pixel_bvb_clock
> at NULL, then the clock framework will be happy to do a nop.
>
> For BCM2711 an old DT would have issues, but, as Maxime has stated, no
> binding or upstream DTB has been merged yet, so it can be made
> mandatory.

If so, it seems good to set bvb_clock to mandatory without taking into

account the BCM2711 an old DTB as it hasn't been merged yet.

I will send version 2 patches.

> Making it optional drops you back on whatever the firmware might have
> set it to, which may be sufficient for some resolutions but not
> others.

As a result of checking by adding bvb_clock when I operated it with

the firmware, it was confirmed that the firmware increased the bvb_clock

from 75000000 to 150000000 when the FHD was exceeded.


Best regards

Hoegeun

>
>    Dave
>