[PATCH 0/3] drm/vc4: Support HDMI QHD or higher output

[PATCH 0/3] drm/vc4: Support HDMI QHD or higher output

[Hoegeun Kwon]

Hi everyone,

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.

Rebased on top of next-20200708 and [1].

[1] : [PATCH v4 00/78] drm/vc4: Support BCM2711 Display Pipeline (Maxime's patchset)

Hoegeun Kwon (3):
  clk: bcm: rpi: Add register to control pixel bvb clk
  ARM: dts: bcm2711: Add bvb clock for hdmi-pixel
  drm/vc4: hdmi: Add pixel bvb clock control

 arch/arm/boot/dts/bcm2711-rpi-4-b.dts |  6 ++++--
 drivers/clk/bcm/clk-raspberrypi.c     |  1 +
 drivers/gpu/drm/vc4/vc4_hdmi.c        | 25 +++++++++++++++++++++++++
 drivers/gpu/drm/vc4/vc4_hdmi.h        |  1 +
 4 files changed, 31 insertions(+), 2 deletions(-)

-- 
2.17.1

[PATCH 1/3] clk: bcm: rpi: Add register to control pixel bvb clk

[Hoegeun Kwon]

To use QHD or higher, we need to modify the pixel_bvb_clk value. So
add register to control this clock.

Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
---
 drivers/clk/bcm/clk-raspberrypi.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/clk/bcm/clk-raspberrypi.c b/drivers/clk/bcm/clk-raspberrypi.c
index 5cc82954e1ce..f89b9cfc4309 100644
--- a/drivers/clk/bcm/clk-raspberrypi.c
+++ b/drivers/clk/bcm/clk-raspberrypi.c
@@ -271,6 +271,7 @@ static int raspberrypi_discover_clocks(struct raspberrypi_clk *rpi,
 		case RPI_FIRMWARE_CORE_CLK_ID:
 		case RPI_FIRMWARE_M2MC_CLK_ID:
 		case RPI_FIRMWARE_V3D_CLK_ID:
+		case RPI_FIRMWARE_PIXEL_BVB_CLK_ID:
 			hw = raspberrypi_clk_register(rpi, clks->parent,
 						      clks->id);
 			if (IS_ERR(hw))
-- 
2.17.1

[PATCH 2/3] ARM: dts: bcm2711: Add bvb clock for hdmi-pixel

[Hoegeun Kwon]

It is necessary to control the hdmi pixel bvb clock. Add bvb clock.

Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
---
 arch/arm/boot/dts/bcm2711-rpi-4-b.dts | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/arch/arm/boot/dts/bcm2711-rpi-4-b.dts b/arch/arm/boot/dts/bcm2711-rpi-4-b.dts
index b93eb30e1ddb..90dee4cb38bc 100644
--- a/arch/arm/boot/dts/bcm2711-rpi-4-b.dts
+++ b/arch/arm/boot/dts/bcm2711-rpi-4-b.dts
@@ -172,12 +172,14 @@
 };
 
 &hdmi0 {
-	clocks = <&firmware_clocks 13>, <&dvp 0>;
+	clocks = <&firmware_clocks 13>, <&dvp 0>, <&firmware_clocks 14>;
+	clock-names = "hdmi", "clk-108M", "bvb";
 	status = "okay";
 };
 
 &hdmi1 {
-	clocks = <&firmware_clocks 13>, <&dvp 1>;
+	clocks = <&firmware_clocks 13>, <&dvp 1>, <&firmware_clocks 14>;
+	clock-names = "hdmi", "clk-108M", "bvb";
 	status = "okay";
 };
 
-- 
2.17.1

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

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

Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
---
 drivers/gpu/drm/vc4/vc4_hdmi.c | 25 +++++++++++++++++++++++++
 drivers/gpu/drm/vc4/vc4_hdmi.h |  1 +
 2 files changed, 26 insertions(+)

diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
index 95ec5eedea39..eb3192d1fd86 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.c
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
@@ -80,6 +80,7 @@
 # define VC4_HD_M_ENABLE			BIT(0)
 
 #define CEC_CLOCK_FREQ 40000
+#define VC4_HSM_MID_CLOCK 149985000
 
 static int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused)
 {
@@ -380,6 +381,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);
 
@@ -638,6 +640,23 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder)
 		return;
 	}
 
+	ret = clk_set_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);
 
@@ -1593,6 +1612,12 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi)
 		return PTR_ERR(vc4_hdmi->audio_clock);
 	}
 
+	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);
+	}
+
 	vc4_hdmi->reset = devm_reset_control_get(dev, NULL);
 	if (IS_ERR(vc4_hdmi->reset)) {
 		DRM_ERROR("Failed to get HDMI reset line\n");
diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
index 0806c6d9f24e..63c6f8bddf1d 100644
--- a/drivers/gpu/drm/vc4/vc4_hdmi.h
+++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
@@ -147,6 +147,7 @@ struct vc4_hdmi {
 	struct clk *pixel_clock;
 	struct clk *hsm_clock;
 	struct clk *audio_clock;
+	struct clk *pixel_bvb_clock;
 
 	struct reset_control *reset;
 
-- 
2.17.1

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

[Stefan Wahren]

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?

I'm a little bit afraid, this change could break with older firmware
versions on BCM283x.

Best regards
Stefan

>
> Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> ---
>  drivers/gpu/drm/vc4/vc4_hdmi.c | 25 +++++++++++++++++++++++++
>  drivers/gpu/drm/vc4/vc4_hdmi.h |  1 +
>  2 files changed, 26 insertions(+)
>
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> index 95ec5eedea39..eb3192d1fd86 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> @@ -80,6 +80,7 @@
>  # define VC4_HD_M_ENABLE			BIT(0)
>  
>  #define CEC_CLOCK_FREQ 40000
> +#define VC4_HSM_MID_CLOCK 149985000
>  
>  static int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused)
>  {
> @@ -380,6 +381,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);
>  
> @@ -638,6 +640,23 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder)
>  		return;
>  	}
>  
> +	ret = clk_set_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);
>  
> @@ -1593,6 +1612,12 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi)
>  		return PTR_ERR(vc4_hdmi->audio_clock);
>  	}
>  
> +	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);
> +	}
> +
>  	vc4_hdmi->reset = devm_reset_control_get(dev, NULL);
>  	if (IS_ERR(vc4_hdmi->reset)) {
>  		DRM_ERROR("Failed to get HDMI reset line\n");
> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
> index 0806c6d9f24e..63c6f8bddf1d 100644
> --- a/drivers/gpu/drm/vc4/vc4_hdmi.h
> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
> @@ -147,6 +147,7 @@ struct vc4_hdmi {
>  	struct clk *pixel_clock;
>  	struct clk *hsm_clock;
>  	struct clk *audio_clock;
> +	struct clk *pixel_bvb_clock;
>  
>  	struct reset_control *reset;
>  

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

[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'.

Please comment, what do you think?

>
> I'm a little bit afraid, this change could break with older firmware
> versions on BCM283x.

Tested it several times with libdrm modetest.

I expect there will be no problem.


Best regards,

Hoegeun

>
> Best regards
> Stefan
>
>> Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
>> ---
>>   drivers/gpu/drm/vc4/vc4_hdmi.c | 25 +++++++++++++++++++++++++
>>   drivers/gpu/drm/vc4/vc4_hdmi.h |  1 +
>>   2 files changed, 26 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
>> index 95ec5eedea39..eb3192d1fd86 100644
>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
>> @@ -80,6 +80,7 @@
>>   # define VC4_HD_M_ENABLE			BIT(0)
>>   
>>   #define CEC_CLOCK_FREQ 40000
>> +#define VC4_HSM_MID_CLOCK 149985000
>>   
>>   static int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused)
>>   {
>> @@ -380,6 +381,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);
>>   
>> @@ -638,6 +640,23 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder)
>>   		return;
>>   	}
>>   
>> +	ret = clk_set_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);
>>   
>> @@ -1593,6 +1612,12 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi)
>>   		return PTR_ERR(vc4_hdmi->audio_clock);
>>   	}
>>   
>> +	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);
>> +	}
>> +
>>   	vc4_hdmi->reset = devm_reset_control_get(dev, NULL);
>>   	if (IS_ERR(vc4_hdmi->reset)) {
>>   		DRM_ERROR("Failed to get HDMI reset line\n");
>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
>> index 0806c6d9f24e..63c6f8bddf1d 100644
>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.h
>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
>> @@ -147,6 +147,7 @@ struct vc4_hdmi {
>>   	struct clk *pixel_clock;
>>   	struct clk *hsm_clock;
>>   	struct clk *audio_clock;
>> +	struct clk *pixel_bvb_clock;
>>   
>>   	struct reset_control *reset;
>>   
>

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

[Stefan Wahren]

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?

>
> Please comment, what do you think?
>
>> I'm a little bit afraid, this change could break with older firmware
>> versions on BCM283x.
> Tested it several times with libdrm modetest.
>
> I expect there will be no problem.
>
>
> Best regards,
>
> Hoegeun
>
>> Best regards
>> Stefan
>>
>>> Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
>>> ---
>>>   drivers/gpu/drm/vc4/vc4_hdmi.c | 25 +++++++++++++++++++++++++
>>>   drivers/gpu/drm/vc4/vc4_hdmi.h |  1 +
>>>   2 files changed, 26 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
>>> index 95ec5eedea39..eb3192d1fd86 100644
>>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
>>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
>>> @@ -80,6 +80,7 @@
>>>   # define VC4_HD_M_ENABLE			BIT(0)
>>>   
>>>   #define CEC_CLOCK_FREQ 40000
>>> +#define VC4_HSM_MID_CLOCK 149985000
>>>   
>>>   static int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused)
>>>   {
>>> @@ -380,6 +381,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);
>>>   
>>> @@ -638,6 +640,23 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder)
>>>   		return;
>>>   	}
>>>   
>>> +	ret = clk_set_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);
>>>   
>>> @@ -1593,6 +1612,12 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi)
>>>   		return PTR_ERR(vc4_hdmi->audio_clock);
>>>   	}
>>>   
>>> +	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);
>>> +	}
>>> +
>>>   	vc4_hdmi->reset = devm_reset_control_get(dev, NULL);
>>>   	if (IS_ERR(vc4_hdmi->reset)) {
>>>   		DRM_ERROR("Failed to get HDMI reset line\n");
>>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
>>> index 0806c6d9f24e..63c6f8bddf1d 100644
>>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.h
>>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
>>> @@ -147,6 +147,7 @@ struct vc4_hdmi {
>>>   	struct clk *pixel_clock;
>>>   	struct clk *hsm_clock;
>>>   	struct clk *audio_clock;
>>> +	struct clk *pixel_bvb_clock;
>>>   
>>>   	struct reset_control *reset;
>>>   
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

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

[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;
         }

         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

>
>> Please comment, what do you think?
>>
>>> I'm a little bit afraid, this change could break with older firmware
>>> versions on BCM283x.
>> Tested it several times with libdrm modetest.
>>
>> I expect there will be no problem.
>>
>>
>> Best regards,
>>
>> Hoegeun
>>
>>> Best regards
>>> Stefan
>>>
>>>> Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
>>>> ---
>>>>    drivers/gpu/drm/vc4/vc4_hdmi.c | 25 +++++++++++++++++++++++++
>>>>    drivers/gpu/drm/vc4/vc4_hdmi.h |  1 +
>>>>    2 files changed, 26 insertions(+)
>>>>
>>>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
>>>> index 95ec5eedea39..eb3192d1fd86 100644
>>>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
>>>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
>>>> @@ -80,6 +80,7 @@
>>>>    # define VC4_HD_M_ENABLE			BIT(0)
>>>>    
>>>>    #define CEC_CLOCK_FREQ 40000
>>>> +#define VC4_HSM_MID_CLOCK 149985000
>>>>    
>>>>    static int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused)
>>>>    {
>>>> @@ -380,6 +381,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);
>>>>    
>>>> @@ -638,6 +640,23 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder)
>>>>    		return;
>>>>    	}
>>>>    
>>>> +	ret = clk_set_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);
>>>>    
>>>> @@ -1593,6 +1612,12 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi)
>>>>    		return PTR_ERR(vc4_hdmi->audio_clock);
>>>>    	}
>>>>    
>>>> +	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);
>>>> +	}
>>>> +
>>>>    	vc4_hdmi->reset = devm_reset_control_get(dev, NULL);
>>>>    	if (IS_ERR(vc4_hdmi->reset)) {
>>>>    		DRM_ERROR("Failed to get HDMI reset line\n");
>>>> diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
>>>> index 0806c6d9f24e..63c6f8bddf1d 100644
>>>> --- a/drivers/gpu/drm/vc4/vc4_hdmi.h
>>>> +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
>>>> @@ -147,6 +147,7 @@ struct vc4_hdmi {
>>>>    	struct clk *pixel_clock;
>>>>    	struct clk *hsm_clock;
>>>>    	struct clk *audio_clock;
>>>> +	struct clk *pixel_bvb_clock;
>>>>    
>>>>    	struct reset_control *reset;
>>>>    
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>

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
>

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2478 bytes --]

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.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Dave Stevenson]

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

  Dave

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

[Hoegeun Kwon]

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
>

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

[Stefan Wahren]

Hi Maxime,

Am 28.08.20 um 17:25 schrieb Maxime Ripard:
> 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.

you are right. Sorry for the noise.

Best regards

>
> Maxime

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

[Maxime Ripard]

Hi Stefan,

On Thu, Aug 27, 2020 at 11:49:34AM +0200, 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?

It won't cause any issue on a non-RPi4 platform since the clock pointer
will be NULL and clk_set_rate can deal with NULL pointers just fine:
https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L2221

For the older DTBs, it shouldn't be an issue either. We haven't merged
the binding yet, so we don't have an upstream DTB using it

Maxime

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

[Dave Stevenson]

Hi Stefan & Hoegeun

On Wed, 26 Aug 2020 at 11:04, Stefan Wahren <stefan.wahren@i2se.com> 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?
>
> I'm a little bit afraid, this change could break with older firmware
> versions on BCM283x.

Thanks for your keen eye on these things.

BVB only exists on 2711, not 283x.

It runs at 2 pixels/clock, must be an integer divider of I believe
600MHz, and between 75 and 300MHz.
This aim of this patch is fine as we currently only go up to 4k30, but
for 4k60 the BVB will need to be set to 300MHz.

Thanks
  Dave

> Best regards
> Stefan
>
> >
> > Signed-off-by: Hoegeun Kwon <hoegeun.kwon@samsung.com>
> > ---
> >  drivers/gpu/drm/vc4/vc4_hdmi.c | 25 +++++++++++++++++++++++++
> >  drivers/gpu/drm/vc4/vc4_hdmi.h |  1 +
> >  2 files changed, 26 insertions(+)
> >
> > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.c b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > index 95ec5eedea39..eb3192d1fd86 100644
> > --- a/drivers/gpu/drm/vc4/vc4_hdmi.c
> > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.c
> > @@ -80,6 +80,7 @@
> >  # define VC4_HD_M_ENABLE                     BIT(0)
> >
> >  #define CEC_CLOCK_FREQ 40000
> > +#define VC4_HSM_MID_CLOCK 149985000
> >
> >  static int vc4_hdmi_debugfs_regs(struct seq_file *m, void *unused)
> >  {
> > @@ -380,6 +381,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);
> >
> > @@ -638,6 +640,23 @@ static void vc4_hdmi_encoder_pre_crtc_configure(struct drm_encoder *encoder)
> >               return;
> >       }
> >
> > +     ret = clk_set_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);
> >
> > @@ -1593,6 +1612,12 @@ static int vc5_hdmi_init_resources(struct vc4_hdmi *vc4_hdmi)
> >               return PTR_ERR(vc4_hdmi->audio_clock);
> >       }
> >
> > +     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);
> > +     }
> > +
> >       vc4_hdmi->reset = devm_reset_control_get(dev, NULL);
> >       if (IS_ERR(vc4_hdmi->reset)) {
> >               DRM_ERROR("Failed to get HDMI reset line\n");
> > diff --git a/drivers/gpu/drm/vc4/vc4_hdmi.h b/drivers/gpu/drm/vc4/vc4_hdmi.h
> > index 0806c6d9f24e..63c6f8bddf1d 100644
> > --- a/drivers/gpu/drm/vc4/vc4_hdmi.h
> > +++ b/drivers/gpu/drm/vc4/vc4_hdmi.h
> > @@ -147,6 +147,7 @@ struct vc4_hdmi {
> >       struct clk *pixel_clock;
> >       struct clk *hsm_clock;
> >       struct clk *audio_clock;
> > +     struct clk *pixel_bvb_clock;
> >
> >       struct reset_control *reset;
> >
>

Re: [PATCH 0/3] drm/vc4: Support HDMI QHD or higher output

[Stefan Wahren]

Hi Hoegeun,

Am 21.08.20 um 09:10 schrieb Hoegeun Kwon:
> Hi everyone,
>
> 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.
thanks for providing this.
>
> Rebased on top of next-20200708 and [1].
>
> [1] : [PATCH v4 00/78] drm/vc4: Support BCM2711 Display Pipeline (Maxime's patchset)
>
> Hoegeun Kwon (3):the 
>   clk: bcm: rpi: Add register to control pixel bvb clk
>   ARM: dts: bcm2711: Add bvb clock for hdmi-pixel
>   drm/vc4: hdmi: Add pixel bvb clock control

Please change the order of your last 2 patches. First the driver changes
and then the device tree sources.

Regards
Stefan

>
>  arch/arm/boot/dts/bcm2711-rpi-4-b.dts |  6 ++++--
>  drivers/clk/bcm/clk-raspberrypi.c     |  1 +
>  drivers/gpu/drm/vc4/vc4_hdmi.c        | 25 +++++++++++++++++++++++++
>  drivers/gpu/drm/vc4/vc4_hdmi.h        |  1 +
>  4 files changed, 31 insertions(+), 2 deletions(-)
>

Re: [PATCH 0/3] drm/vc4: Support HDMI QHD or higher output

[Maxime Ripard]

Hi!

On Fri, Aug 21, 2020 at 04:10:42PM +0900, Hoegeun Kwon wrote:
> Hi everyone,
> 
> 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.
> 
> Rebased on top of next-20200708 and [1].
> 
> [1] : [PATCH v4 00/78] drm/vc4: Support BCM2711 Display Pipeline (Maxime's patchset)

Thanks a lot for figuring it out

I have to send a new (and hopefully final) version next week to address some changes suggested by
Dave, so I'll squash your patches into the series

Thanks again!
Maxime