lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <wrzkhnslrwnpsv7gbdcvxobahun7l6foftftk676hxinfeqwjr@v3di5okva7co>
Date: Thu, 2 Oct 2025 17:33:27 +0200
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Dmitry Osipenko <dmitry.osipenko@...labora.com>
Cc: Mauro Carvalho Chehab <mchehab@...nel.org>, 
	Hans Verkuil <hverkuil@...all.nl>, jose.abreu@...opsys.com, nelson.costa@...opsys.com, 
	shawn.wen@...k-chips.com, nicolas.dufresne@...labora.com, kernel@...labora.com, 
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org, linux-rockchip@...ts.infradead.org
Subject: Re: [PATCH v2] media: synopsys: hdmirx: Detect broken interrupt

Hello Dmitry,

On Thu, Oct 02, 2025 at 05:07:50PM +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@...labora.com>
> ---
> Changelog:
> 
> v2: - Added PHY r/w lock and moved the clarifying error message as
>       was suggested by Sebastian Reichel.
> 
>  .../platform/synopsys/hdmirx/snps_hdmirx.c    | 90 ++++++++++++++++++-
>  .../platform/synopsys/hdmirx/snps_hdmirx.h    |  2 +
>  2 files changed, 90 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..e6456352dfa5 100644
> --- a/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c
> +++ b/drivers/media/platform/synopsys/hdmirx/snps_hdmirx.c

#include <linux/cleanup.h>

Otherwise LGTM and hopefully helps people to figure out the root
cause of their problems:

Reviewed-by: Sebastian Reichel <sebastian.reichel@...labora.com>

Greetings,

-- Sebastian

> @@ -132,12 +132,14 @@ struct snps_hdmirx_dev {
>  	struct delayed_work delayed_work_hotplug;
>  	struct delayed_work delayed_work_res_change;
>  	struct hdmirx_cec *cec;
> +	struct mutex phy_rw_lock; /* to protect phy r/w configuration */
>  	struct mutex stream_lock; /* to lock video stream capture */
>  	struct mutex work_lock; /* to lock the critical section of hotplug event */
>  	struct reset_control_bulk_data resets[HDMIRX_NUM_RST];
>  	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,11 +798,50 @@ 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;
> +
> +	guard(mutex)(&hdmirx_dev->phy_rw_lock);
> +
> +	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)
>  {
>  	struct device *dev = hdmirx_dev->dev;
>  
> +	guard(mutex)(&hdmirx_dev->phy_rw_lock);
> +
>  	reinit_completion(&hdmirx_dev->cr_write_done);
>  	/* clear irq status */
>  	hdmirx_clear_interrupt(hdmirx_dev, MAINUNIT_2_INT_CLEAR, 0xffffffff);
> @@ -1814,6 +1855,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 +2361,51 @@ 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 ret;
> +	u32 val;
> +
> +	enable_irq(hdmirx_dev->hdmi_irq);
> +
> +	hdmirx_writel(hdmirx_dev, PHYCREG_CONFIG0, 0x3);
> +
> +	ret = hdmirx_phy_register_read(hdmirx_dev,
> +				       HDMIPCS_DIG_CTRL_PATH_MAIN_FSM_FSM_CONFIG,
> +				       &val);
> +
> +	disable_irq(hdmirx_dev->hdmi_irq);
> +
> +	return ret;
> +}
> +
> +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 that we don't support
> +	 * in this driver.
> +	 */
> +	ret = hdmirx_detect_broken_interrupt(hdmirx_dev);
> +	if (ret)
> +		dev_err_probe(hdmirx_dev->dev, ret,
> +			      "interrupt not functioning, open-source TF-A is required by this driver\n");
> +
>  	/*
>  	 * 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 */
> @@ -2606,10 +2687,12 @@ static int hdmirx_probe(struct platform_device *pdev)
>  		return dev_err_probe(dev, PTR_ERR(hdmirx_dev->regs),
>  				     "failed to remap regs resource\n");
>  
> +	mutex_init(&hdmirx_dev->phy_rw_lock);
>  	mutex_init(&hdmirx_dev->stream_lock);
>  	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 +2706,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
> 

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ