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 [off-list ref] wrote:

quoted

Hi,

On Fri, Aug 28, 2020 at 02:45:49PM +0200, Stefan Wahren wrote:

quoted

Am 28.08.20 um 08:30 schrieb Hoegeun Kwon:

quoted

On 8/27/20 6:49 PM, Stefan Wahren wrote:

quoted

Am 27.08.20 um 06:35 schrieb Hoegeun Kwon:

quoted

Hi Stefan,

Thank you for your review.


On 8/26/20 7:04 PM, Stefan Wahren wrote:

quoted

Hi Hoeguen,

Am 21.08.20 um 09:10 schrieb Hoegeun Kwon:

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel