Re: [PATCH 3/3] drm/vc4: hdmi: Add pixel bvb clock control

[Hoegeun Kwon <hoegeun.kwon@samsung.com>]

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
>