Downstream version of RK3588 U-Boot uses customized TF-A that remaps
HDMIRX hardware interrupt, routing it via firmware that isn't supported
by upstream driver.

Detect broken interrupt and print a clarifying error message about a need
to use open-source TF-A with this driver.

Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
---
 .../platform/synopsys/hdmirx/snps_hdmirx.c    | 85 ++++++++++++++++++-
 .../platform/synopsys/hdmirx/snps_hdmirx.h    |  2 +
 2 files changed, 85 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
index b7d278b3889f..faca601d72a4 100644
--- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
+++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
@@ -138,6 +138,7 @@ struct snps_hdmirx_dev {
 	struct clk_bulk_data *clks;
 	struct regmap *grf;
 	struct regmap *vo1_grf;
+	struct completion cr_read_done;
 	struct completion cr_write_done;
 	struct completion timer_base_lock;
 	struct completion avi_pkt_rcv;
@@ -796,6 +797,41 @@ static int wait_reg_bit_status(struct snps_hdmirx_dev *hdmirx_dev, u32 reg,
 	return 0;
 }
 
+static int hdmirx_phy_register_read(struct snps_hdmirx_dev *hdmirx_dev,
+				    u32 phy_reg, u32 *val)
+{
+	struct device *dev = hdmirx_dev->dev;
+	u32 status;
+
+	reinit_completion(&hdmirx_dev->cr_read_done);
+	/* clear irq status */
+	hdmirx_clear_interrupt(hdmirx_dev, MAINUNIT_2_INT_CLEAR, 0xffffffff);
+	/* en irq */
+	hdmirx_update_bits(hdmirx_dev, MAINUNIT_2_INT_MASK_N,
+			   PHYCREG_CR_READ_DONE, PHYCREG_CR_READ_DONE);
+	/* write phy reg addr */
+	hdmirx_writel(hdmirx_dev, PHYCREG_CONFIG1, phy_reg);
+	/* config read enable */
+	hdmirx_writel(hdmirx_dev, PHYCREG_CONTROL, PHYCREG_CR_PARA_READ_P);
+
+	if (!wait_for_completion_timeout(&hdmirx_dev->cr_read_done,
+					 msecs_to_jiffies(20))) {
+		dev_err(dev, "%s wait cr read done failed\n", __func__);
+		return -ETIMEDOUT;
+	}
+
+	/* read phy reg value */
+	status = hdmirx_readl(hdmirx_dev, PHYCREG_STATUS);
+	if (!(status & PHYCREG_CR_PARA_DATAVALID)) {
+		dev_err(dev, "%s cr read failed\n", __func__);
+		return -EINVAL;
+	}
+
+	*val = status & PHYCREG_CR_PARA_RD_DATA_MASK;
+
+	return 0;
+}
+
 static int hdmirx_phy_register_write(struct snps_hdmirx_dev *hdmirx_dev,
 				     u32 phy_reg, u32 val)
 {
@@ -1814,6 +1850,13 @@ static void mainunit_2_int_handler(struct snps_hdmirx_dev *hdmirx_dev,
 		*handled = true;
 	}
 
+	if (status & PHYCREG_CR_READ_DONE) {
+		hdmirx_update_bits(hdmirx_dev, MAINUNIT_2_INT_MASK_N,
+				   PHYCREG_CR_READ_DONE, 0);
+		complete(&hdmirx_dev->cr_read_done);
+		*handled = true;
+	}
+
 	if (status & TMDSVALID_STABLE_CHG) {
 		process_signal_change(hdmirx_dev);
 		v4l2_dbg(2, debug, v4l2_dev, "%s: TMDSVALID_STABLE_CHG\n", __func__);
@@ -2313,18 +2356,52 @@ static void hdmirx_disable_all_interrupts(struct snps_hdmirx_dev *hdmirx_dev)
 	hdmirx_clear_interrupt(hdmirx_dev, CEC_INT_CLEAR, 0xffffffff);
 }
 
-static void hdmirx_init(struct snps_hdmirx_dev *hdmirx_dev)
+static int hdmirx_detect_broken_interrupt(struct snps_hdmirx_dev *hdmirx_dev)
+{
+	int err;
+	u32 val;
+
+	enable_irq(hdmirx_dev->hdmi_irq);
+
+	hdmirx_writel(hdmirx_dev, PHYCREG_CONFIG0, 0x3);
+
+	err = hdmirx_phy_register_read(hdmirx_dev,
+				       HDMIPCS_DIG_CTRL_PATH_MAIN_FSM_FSM_CONFIG,
+				       &val);
+
+	disable_irq(hdmirx_dev->hdmi_irq);
+
+	if (err)
+		return dev_err_probe(hdmirx_dev->dev, err,
+				     "interrupt not functioning, open-source TF-A is required by this driver\n");
+
+	return 0;
+}
+
+static int hdmirx_init(struct snps_hdmirx_dev *hdmirx_dev)
 {
+	int ret;
+
 	hdmirx_update_bits(hdmirx_dev, PHY_CONFIG, PHY_RESET | PHY_PDDQ, 0);
 
 	regmap_write(hdmirx_dev->vo1_grf, VO1_GRF_VO1_CON2,
 		     (HDMIRX_SDAIN_MSK | HDMIRX_SCLIN_MSK) |
 		     ((HDMIRX_SDAIN_MSK | HDMIRX_SCLIN_MSK) << 16));
+
+	/*
+	 * RK3588 downstream version of TF-A remaps HDMIRX interrupt and
+	 * requires use of a vendor-specific FW API by the driver that we
+	 * don't support in this driver.
+	 */
+	ret = hdmirx_detect_broken_interrupt(hdmirx_dev);
+
 	/*
 	 * Some interrupts are enabled by default, so we disable
 	 * all interrupts and clear interrupts status first.
 	 */
 	hdmirx_disable_all_interrupts(hdmirx_dev);
+
+	return ret;
 }
 
 /* hdmi-4k-300mhz EDID produced by v4l2-ctl tool */
@@ -2610,6 +2687,7 @@ static int hdmirx_probe(struct platform_device *pdev)
 	mutex_init(&hdmirx_dev->work_lock);
 	spin_lock_init(&hdmirx_dev->rst_lock);
 
+	init_completion(&hdmirx_dev->cr_read_done);
 	init_completion(&hdmirx_dev->cr_write_done);
 	init_completion(&hdmirx_dev->timer_base_lock);
 	init_completion(&hdmirx_dev->avi_pkt_rcv);
@@ -2623,7 +2701,10 @@ static int hdmirx_probe(struct platform_device *pdev)
 	hdmirx_dev->timings = cea640x480;
 
 	hdmirx_enable(dev);
-	hdmirx_init(hdmirx_dev);
+
+	ret = hdmirx_init(hdmirx_dev);
+	if (ret)
+		goto err_pm;
 
 	v4l2_dev = &hdmirx_dev->v4l2_dev;
 	strscpy(v4l2_dev->name, dev_name(dev), sizeof(v4l2_dev->name));
diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
index 220ab99ca611..a719439d3ca0 100644
--- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
+++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
@@ -114,6 +114,8 @@
 #define PHYCREG_CR_PARA_WRITE_P			BIT(1)
 #define PHYCREG_CR_PARA_READ_P			BIT(0)
 #define PHYCREG_STATUS				0x00f4
+#define PHYCREG_CR_PARA_DATAVALID		BIT(24)
+#define PHYCREG_CR_PARA_RD_DATA_MASK		GENMASK(15, 0)
 
 #define MAINUNIT_STATUS				0x0150
 #define TMDSVALID_STABLE_ST			BIT(1)
-- 
2.51.0

Hello Dmitry,

On Wed, Oct 01, 2025 at 08:50:44PM +0300, Dmitry Osipenko wrote:
> Downstream version of RK3588 U-Boot uses customized TF-A that remaps
> HDMIRX hardware interrupt, routing it via firmware that isn't supported
> by upstream driver.
> 
> Detect broken interrupt and print a clarifying error message about a need
> to use open-source TF-A with this driver.
> 
> Signed-off-by: Dmitry Osipenko <dmitry.osipenko@collabora.com>
> ---
>  .../platform/synopsys/hdmirx/snps_hdmirx.c    | 85 ++++++++++++++++++-
>  .../platform/synopsys/hdmirx/snps_hdmirx.h    |  2 +
>  2 files changed, 85 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> index b7d278b3889f..faca601d72a4 100644
> --- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> +++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> @@ -138,6 +138,7 @@ struct snps_hdmirx_dev {
>  	struct clk_bulk_data *clks;
>  	struct regmap *grf;
>  	struct regmap *vo1_grf;
> +	struct completion cr_read_done;
>  	struct completion cr_write_done;
>  	struct completion timer_base_lock;
>  	struct completion avi_pkt_rcv;
> @@ -796,6 +797,41 @@ static int wait_reg_bit_status(struct snps_hdmirx_dev *hdmirx_dev, u32 reg,
>  	return 0;
>  }
>  
> +static int hdmirx_phy_register_read(struct snps_hdmirx_dev *hdmirx_dev,
> +				    u32 phy_reg, u32 *val)
> +{
> +	struct device *dev = hdmirx_dev->dev;
> +	u32 status;
> +
> +	reinit_completion(&hdmirx_dev->cr_read_done);
> +	/* clear irq status */
> +	hdmirx_clear_interrupt(hdmirx_dev, MAINUNIT_2_INT_CLEAR, 0xffffffff);
> +	/* en irq */
> +	hdmirx_update_bits(hdmirx_dev, MAINUNIT_2_INT_MASK_N,
> +			   PHYCREG_CR_READ_DONE, PHYCREG_CR_READ_DONE);
> +	/* write phy reg addr */
> +	hdmirx_writel(hdmirx_dev, PHYCREG_CONFIG1, phy_reg);
> +	/* config read enable */
> +	hdmirx_writel(hdmirx_dev, PHYCREG_CONTROL, PHYCREG_CR_PARA_READ_P);
> +
> +	if (!wait_for_completion_timeout(&hdmirx_dev->cr_read_done,
> +					 msecs_to_jiffies(20))) {
> +		dev_err(dev, "%s wait cr read done failed\n", __func__);
> +		return -ETIMEDOUT;
> +	}
> +
> +	/* read phy reg value */
> +	status = hdmirx_readl(hdmirx_dev, PHYCREG_STATUS);
> +	if (!(status & PHYCREG_CR_PARA_DATAVALID)) {
> +		dev_err(dev, "%s cr read failed\n", __func__);
> +		return -EINVAL;
> +	}
> +
> +	*val = status & PHYCREG_CR_PARA_RD_DATA_MASK;
> +
> +	return 0;
> +}

Do you expect this to be used in other places in the driver? In that
case there should probably be some locking, since the hardware interface
obviously cannot handle concurrency. Otherwise maybe add a comment,
that the function may not be called if concurrency is possible?

>  static int hdmirx_phy_register_write(struct snps_hdmirx_dev *hdmirx_dev,
>  				     u32 phy_reg, u32 val)
>  {
> @@ -1814,6 +1850,13 @@ static void mainunit_2_int_handler(struct snps_hdmirx_dev *hdmirx_dev,
>  		*handled = true;
>  	}
>  
> +	if (status & PHYCREG_CR_READ_DONE) {
> +		hdmirx_update_bits(hdmirx_dev, MAINUNIT_2_INT_MASK_N,
> +				   PHYCREG_CR_READ_DONE, 0);
> +		complete(&hdmirx_dev->cr_read_done);
> +		*handled = true;
> +	}
> +
>  	if (status & TMDSVALID_STABLE_CHG) {
>  		process_signal_change(hdmirx_dev);
>  		v4l2_dbg(2, debug, v4l2_dev, "%s: TMDSVALID_STABLE_CHG\n", __func__);
> @@ -2313,18 +2356,52 @@ static void hdmirx_disable_all_interrupts(struct snps_hdmirx_dev *hdmirx_dev)
>  	hdmirx_clear_interrupt(hdmirx_dev, CEC_INT_CLEAR, 0xffffffff);
>  }
>  
> -static void hdmirx_init(struct snps_hdmirx_dev *hdmirx_dev)
> +static int hdmirx_detect_broken_interrupt(struct snps_hdmirx_dev *hdmirx_dev)
> +{
> +	int err;
> +	u32 val;
> +
> +	enable_irq(hdmirx_dev->hdmi_irq);
> +
> +	hdmirx_writel(hdmirx_dev, PHYCREG_CONFIG0, 0x3);
> +
> +	err = hdmirx_phy_register_read(hdmirx_dev,
> +				       HDMIPCS_DIG_CTRL_PATH_MAIN_FSM_FSM_CONFIG,
> +				       &val);
> +
> +	disable_irq(hdmirx_dev->hdmi_irq);
> +
> +	if (err)
> +		return dev_err_probe(hdmirx_dev->dev, err,
> +				     "interrupt not functioning, open-source TF-A is required by this driver\n");
> +
> +	return 0;

I think it's better to just return err here (without dev_err_probe) [...]

> +}
> +
> +static int hdmirx_init(struct snps_hdmirx_dev *hdmirx_dev)
>  {
> +	int ret;
> +
>  	hdmirx_update_bits(hdmirx_dev, PHY_CONFIG, PHY_RESET | PHY_PDDQ, 0);
>  
>  	regmap_write(hdmirx_dev->vo1_grf, VO1_GRF_VO1_CON2,
>  		     (HDMIRX_SDAIN_MSK | HDMIRX_SCLIN_MSK) |
>  		     ((HDMIRX_SDAIN_MSK | HDMIRX_SCLIN_MSK) << 16));
> +
> +	/*
> +	 * RK3588 downstream version of TF-A remaps HDMIRX interrupt and
> +	 * requires use of a vendor-specific FW API by the driver that we
> +	 * don't support in this driver.
> +	 */
> +	ret = hdmirx_detect_broken_interrupt(hdmirx_dev);

[...] and have the dev_err_probe() moved to this place, so that the
comment about the Rockchip vendor TF-A is directly in front of the
error message.

Otherwise LGTM.

Greetings,

-- Sebastian

>  	/*
>  	 * Some interrupts are enabled by default, so we disable
>  	 * all interrupts and clear interrupts status first.
>  	 */
>  	hdmirx_disable_all_interrupts(hdmirx_dev);
> +
> +	return ret;
>  }
>  
>  /* hdmi-4k-300mhz EDID produced by v4l2-ctl tool */
> @@ -2610,6 +2687,7 @@ static int hdmirx_probe(struct platform_device *pdev)
>  	mutex_init(&hdmirx_dev->work_lock);
>  	spin_lock_init(&hdmirx_dev->rst_lock);
>  
> +	init_completion(&hdmirx_dev->cr_read_done);
>  	init_completion(&hdmirx_dev->cr_write_done);
>  	init_completion(&hdmirx_dev->timer_base_lock);
>  	init_completion(&hdmirx_dev->avi_pkt_rcv);
> @@ -2623,7 +2701,10 @@ static int hdmirx_probe(struct platform_device *pdev)
>  	hdmirx_dev->timings = cea640x480;
>  
>  	hdmirx_enable(dev);
> -	hdmirx_init(hdmirx_dev);
> +
> +	ret = hdmirx_init(hdmirx_dev);
> +	if (ret)
> +		goto err_pm;
>  
>  	v4l2_dev = &hdmirx_dev->v4l2_dev;
>  	strscpy(v4l2_dev->name, dev_name(dev), sizeof(v4l2_dev->name));
> diff --git a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
> index 220ab99ca611..a719439d3ca0 100644
> --- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
> +++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.h
> @@ -114,6 +114,8 @@
>  #define PHYCREG_CR_PARA_WRITE_P			BIT(1)
>  #define PHYCREG_CR_PARA_READ_P			BIT(0)
>  #define PHYCREG_STATUS				0x00f4
> +#define PHYCREG_CR_PARA_DATAVALID		BIT(24)
> +#define PHYCREG_CR_PARA_RD_DATA_MASK		GENMASK(15, 0)
>  
>  #define MAINUNIT_STATUS				0x0150
>  #define TMDSVALID_STABLE_ST			BIT(1)
> -- 
> 2.51.0
> 

Hi,

On 10/2/25 16:18, Sebastian Reichel wrote:
>> +	*val = status & PHYCREG_CR_PARA_RD_DATA_MASK;
>> +
>> +	return 0;
>> +}
> Do you expect this to be used in other places in the driver? In that
> case there should probably be some locking, since the hardware interface
> obviously cannot handle concurrency. Otherwise maybe add a comment,
> that the function may not be called if concurrency is possible?

Don't expect this function to be used in other places and haven't added
locking on purpose to keep the code cleaner. Will add the comment.

Thanks for all suggestions.

-- 
Best regards,
Dmitry

On 10/2/25 16:36, Dmitry Osipenko wrote:
> Hi,
> 
> On 10/2/25 16:18, Sebastian Reichel wrote:
>>> +	*val = status & PHYCREG_CR_PARA_RD_DATA_MASK;
>>> +
>>> +	return 0;
>>> +}
>> Do you expect this to be used in other places in the driver? In that
>> case there should probably be some locking, since the hardware interface
>> obviously cannot handle concurrency. Otherwise maybe add a comment,
>> that the function may not be called if concurrency is possible?
> 
> Don't expect this function to be used in other places and haven't added
> locking on purpose to keep the code cleaner. Will add the comment.
> 
> Thanks for all suggestions.
> 

On a second thought, this would be a case where the guard() thingy would
be useful and help keeping code clean, will use it in v2.

-- 
Best regards,
Dmitry