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

[Carlos Song (OSS)]

Gentle ping to avoid missing in mail sea.

Thanks.


NXP Confidential
> -----Original Message-----
> From: Frank Li (OSS) <frank.li@xxxxxxxxxxx>
> Sent: Saturday, August 8, 2026 3:25 AM
> To: Carlos Song (OSS) <carlos.song@xxxxxxxxxxx>
> Cc: Aisheng Dong <aisheng.dong@xxxxxxx>; andi.shyti@xxxxxxxxxx; Frank Li
> <frank.li@xxxxxxx>; s.hauer@xxxxxxxxxxxxxx; kernel@xxxxxxxxxxxxxx;
> festevam@xxxxxxxxx; Carlos Song <carlos.song@xxxxxxx>;
> linux-i2c@xxxxxxxxxxxxxxx; imx@xxxxxxxxxxxxxxx;
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH] i2c: imx-lpi2c: avoid accessing target registers on
> master-only controllers
>
> On Mon, Aug 03, 2026 at 11:27:05AM +0800, carlos.song@xxxxxxxxxxx wrote:
> > From: Carlos Song <carlos.song@xxxxxxx>
> >
> > 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@xxxxxxx>
> > ---
>
> Reviewed-by: Frank Li <Frank.Li@xxxxxxx>
>
> >  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
> >
> >