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

[Stefan Wahren]

Hi,

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.

Best regards

>
>          vc4_hdmi->reset = devm_reset_control_get(dev, NULL);
>
> If we modify like this, if there is no bvb clock in dtb, then 
> pixel_bvb_clock will be null
>
> and it will not affect the clk control function below.
>
>    - clk_set_rate()
>    - clk_prepare_enable()
>    - clk_disable_unprepare()
>
>
> Best regards
>
> Hoegeun
>