Re: [PATCH] i2c: imx-lpi2c: avoid accessing target registers on master-only controllers

[Frank Li <Frank.li@oss.nxp.com>]

On Mon, Aug 03, 2026 at 11:27:05AM +0800, carlos.song@oss.nxp.com wrote:
> From: Carlos Song <carlos.song@nxp.com>
>
> Not all LPI2C controller instances implement the Target block.
> Since commit 90311787f483 ("i2c: imx-lpi2c: reset controller in
> probe stage"), the driver unconditionally resets both the Master
> and Target blocks during probe.
>
> On controllers that do not support target mode, accessing the
> Target registers triggers an asynchronous SError and prevents the
> driver from probing successfully. For example on i.MX8QM:
>
>   SError Interrupt on CPU2, code 0x00000000bf000002 -- SError
>   Hardware name: Freescale i.MX8QM MEK (DT)
>   pc : lpi2c_imx_probe+0x280/0x594
>   lr : lpi2c_imx_probe+0x224/0x594
>   Kernel panic - not syncing: Asynchronous SError Interrupt
>
> The VERID register is implemented in the Master block and can be
> safely accessed on all controller variants. Its FEATURE field
> indicates whether target mode is supported.
>
> Read VERID during probe and use it to determine whether the
> Target block is present. Only access Target registers when target
> mode is supported and reject target registration requests with
> -EOPNOTSUPP otherwise.
>
> Fixes: 90311787f483 ("i2c: imx-lpi2c: reset controller in probe stage")
> Signed-off-by: Carlos Song <carlos.song@nxp.com>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/i2c/busses/i2c-imx-lpi2c.c | 27 +++++++++++++++++++++++----
>  1 file changed, 23 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index e1a4338bc51e..1cfd7a4c8237 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -29,6 +29,7 @@
>
>  #define DRIVER_NAME "imx-lpi2c"
>
> +#define LPI2C_VERID	0x00	/* i2c version ID */
>  #define LPI2C_PARAM	0x04	/* i2c RX/TX FIFO size */
>  #define LPI2C_MCR	0x10	/* i2c contrl register */
>  #define LPI2C_MSR	0x14	/* i2c status register */
> @@ -136,6 +137,9 @@
>  #define I2C_PM_LONG_TIMEOUT_MS	1000 /* Avoid dead lock caused by big clock prepare lock */
>  #define I2C_DMA_THRESHOLD	8 /* bytes */
>
> +/* Bit 0 indicates the presence of the target feature */
> +#define VERID_FEATURE_TARGET_PRESENT	BIT(0)
> +
>  enum lpi2c_imx_mode {
>  	STANDARD,	/* 100+Kbps */
>  	FAST,		/* 400+Kbps */
> @@ -194,6 +198,7 @@ struct lpi2c_imx_struct {
>  	bool			can_use_dma;
>  	struct lpi2c_imx_dma	*dma;
>  	struct i2c_client	*target;
> +	bool			target_supported;
>  	int			irq;
>  	const struct imx_lpi2c_hwdata *hwdata;
>  };
> @@ -1330,6 +1335,10 @@ static int lpi2c_imx_register_target(struct i2c_client *client)
>  	struct lpi2c_imx_struct *lpi2c_imx = i2c_get_adapdata(client->adapter);
>  	int ret;
>
> +	/* Reject target-mode registration on controllers that don't support it. */
> +	if (!lpi2c_imx->target_supported)
> +		return -EOPNOTSUPP;
> +
>  	if (lpi2c_imx->target)
>  		return -EBUSY;
>
> @@ -1546,13 +1555,23 @@ static int lpi2c_imx_probe(struct platform_device *pdev)
>  	pm_runtime_enable(&pdev->dev);
>
>  	/*
> -	 * Reset all internal controller registers of both Master and Target
> -	 * to avoid effects of previous status.
> +	 * Reset all internal controller registers to avoid effects of any
> +	 * state left over from a previous stage (e.g. the bootloader).
> +	 *
> +	 * The Master block (MCR) is present on every controller, so reset it
> +	 * unconditionally. VERID shows whether the target feature is supported.
> +	 * Do not touch the Target block (SCR) on a master-only controller to
> +	 * avoid an asynchronous SError.
>  	 */
>  	writel(MCR_RST, lpi2c_imx->base + LPI2C_MCR);
> -	writel(SCR_RST, lpi2c_imx->base + LPI2C_SCR);
>  	writel(0, lpi2c_imx->base + LPI2C_MCR);
> -	writel(0, lpi2c_imx->base + LPI2C_SCR);
> +
> +	lpi2c_imx->target_supported = !!(readl(lpi2c_imx->base + LPI2C_VERID) &
> +					 VERID_FEATURE_TARGET_PRESENT);
> +	if (lpi2c_imx->target_supported) {
> +		writel(SCR_RST, lpi2c_imx->base + LPI2C_SCR);
> +		writel(0, lpi2c_imx->base + LPI2C_SCR);
> +	}
>
>  	ret = devm_request_irq(&pdev->dev, lpi2c_imx->irq, lpi2c_imx_isr, IRQF_NO_SUSPEND,
>  			       pdev->name, lpi2c_imx);
> --
> 2.43.0
>
>