Currently the TIMER_BASE_CONFIG0 register gets initialized to a fixed
value as initially found in vendor driver code supporting the RK3588
SoC.  As a matter of fact the value matches the rate of the HDMI TX
reference clock, which is roughly 428.57 MHz.

However, on RK3576 SoC that rate is slightly lower, i.e. 396.00 MHz, and
the incorrect register configuration breaks CEC functionality.

Set the timer base according to the actual reference clock rate that
shall be provided by the platform driver.

While at it, also drop the unnecessary empty lines in
dw_hdmi_qp_init_hw().

Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@collabora.com>
---
 drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c | 11 ++++++++---
 include/drm/bridge/dw_hdmi_qp.h              |  1 +
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
index 96455b3bb7b6a3f6ad488d10bc9ba90a1b56e4c8..42a90e0383061dad6c8416af21b27db7a3ba6d7d 100644
--- a/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
+++ b/drivers/gpu/drm/bridge/synopsys/dw-hdmi-qp.c
@@ -162,6 +162,7 @@ struct dw_hdmi_qp {
 		void *data;
 	} phy;
 
+	unsigned long ref_clk_rate;
 	struct regmap *regm;
 
 	unsigned long tmds_char_rate;
@@ -1223,13 +1224,11 @@ static void dw_hdmi_qp_init_hw(struct dw_hdmi_qp *hdmi)
 {
 	dw_hdmi_qp_write(hdmi, 0, MAINUNIT_0_INT_MASK_N);
 	dw_hdmi_qp_write(hdmi, 0, MAINUNIT_1_INT_MASK_N);
-	dw_hdmi_qp_write(hdmi, 428571429, TIMER_BASE_CONFIG0);
+	dw_hdmi_qp_write(hdmi, hdmi->ref_clk_rate, TIMER_BASE_CONFIG0);
 
 	/* Software reset */
 	dw_hdmi_qp_write(hdmi, 0x01, I2CM_CONTROL0);
-
 	dw_hdmi_qp_write(hdmi, 0x085c085c, I2CM_FM_SCL_CONFIG0);
-
 	dw_hdmi_qp_mod(hdmi, 0, I2CM_FM_EN, I2CM_INTERFACE_CONTROL0);
 
 	/* Clear DONE and ERROR interrupts */
@@ -1255,6 +1254,11 @@ struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,
 		return ERR_PTR(-ENODEV);
 	}
 
+	if (!plat_data->ref_clk_rate) {
+		dev_err(dev, "Missing ref_clk rate\n");
+		return ERR_PTR(-ENODEV);
+	}
+
 	hdmi = devm_drm_bridge_alloc(dev, struct dw_hdmi_qp, bridge,
 				     &dw_hdmi_qp_bridge_funcs);
 	if (IS_ERR(hdmi))
@@ -1274,6 +1278,7 @@ struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,
 
 	hdmi->phy.ops = plat_data->phy_ops;
 	hdmi->phy.data = plat_data->phy_data;
+	hdmi->ref_clk_rate = plat_data->ref_clk_rate;
 
 	dw_hdmi_qp_init_hw(hdmi);
 
diff --git a/include/drm/bridge/dw_hdmi_qp.h b/include/drm/bridge/dw_hdmi_qp.h
index b4a9b739734ec7b67013b683fe6017551aa19172..76ecf31301997718604a05f70ce9eab8695e26b5 100644
--- a/include/drm/bridge/dw_hdmi_qp.h
+++ b/include/drm/bridge/dw_hdmi_qp.h
@@ -24,6 +24,7 @@ struct dw_hdmi_qp_plat_data {
 	void *phy_data;
 	int main_irq;
 	int cec_irq;
+	unsigned long ref_clk_rate;
 };
 
 struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,

-- 
2.50.1

Hi Cristian,

On Mon, 25 Aug 2025 at 10:57, Cristian Ciocaltea
<cristian.ciocaltea@collabora.com> wrote:
> @@ -1255,6 +1254,11 @@ struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,
>                 return ERR_PTR(-ENODEV);
>         }
>
> +       if (!plat_data->ref_clk_rate) {
> +               dev_err(dev, "Missing ref_clk rate\n");
> +               return ERR_PTR(-ENODEV);
> +       }

This introduces another bisect cliff, as the Rockchip integration
isn't added until patch 5/6, meaning together with the previous patch
the driver isn't usable between patches 1-5. It would be most sensible
I think to keep a default until the users have been fixed up. But
maybe a better sequence for this series would be:
* dev_err_probe() cleanup (easy, no dependencies)
* add refclk to plat_data (populated but unused)
* use refclk instead of hardcoded frequency in bridge driver, make it mandatory
* add CEC IRQ to plat_data (populated but unused)
* add CEC support to driver, probably make it not mandatory to provide
CEC IRQ in DT since it doesn't seem required for correct operation?
* enable CEC in defconfig

Cheers,
Daniel

Hi Daniel,

On 8/29/25 6:21 PM, Daniel Stone wrote:
> Hi Cristian,
> 
> On Mon, 25 Aug 2025 at 10:57, Cristian Ciocaltea
> <cristian.ciocaltea@collabora.com> wrote:
>> @@ -1255,6 +1254,11 @@ struct dw_hdmi_qp *dw_hdmi_qp_bind(struct platform_device *pdev,
>>                 return ERR_PTR(-ENODEV);
>>         }
>>
>> +       if (!plat_data->ref_clk_rate) {
>> +               dev_err(dev, "Missing ref_clk rate\n");
>> +               return ERR_PTR(-ENODEV);
>> +       }
> 
> This introduces another bisect cliff, as the Rockchip integration
> isn't added until patch 5/6, meaning together with the previous patch
> the driver isn't usable between patches 1-5. It would be most sensible
> I think to keep a default until the users have been fixed up. But
> maybe a better sequence for this series would be:
> * dev_err_probe() cleanup (easy, no dependencies)
> * add refclk to plat_data (populated but unused)
> * use refclk instead of hardcoded frequency in bridge driver, make it mandatory
> * add CEC IRQ to plat_data (populated but unused)
> * add CEC support to driver, probably make it not mandatory to provide
> CEC IRQ in DT since it doesn't seem required for correct operation?
> * enable CEC in defconfig

Yeah, this is pretty similar to how the initial series looked like.  The
current sequence follows Heiko's suggestion, which I (still) think it's the
correct approach.

Both bisect issues are now fixed in v4:

https://lore.kernel.org/all/20250903-rk3588-hdmi-cec-v4-0-fa25163c4b08@collabora.com/

Thanks for the review!

Regards,
Cristian