| 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
| ||
|
Message-ID: <do3p4ncc6issxwqam3oeo54xtoi6jvw7maeprdbfkdn3b3aabr@ilwktxqyf4ap>
Date: Thu, 2 Oct 2025 15:18:41 +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 v1] media: synopsys: hdmirx: Detect broken interrupt
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@...labora.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
>
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists