Re: [PATCH V4] i2c: imx-lpi2c: add target mode support

[Carlos Song]

> Hi Carlos,
> 
> On Thu, Sep 12, 2024 at 04:24:13PM GMT, carlos.song@xxxxxxx wrote:
> > From: Carlos Song <carlos.song@xxxxxxx>
> >
> > LPI2C support master controller and target controller enabled
> > simultaneously. Both controllers share same SDA/SCL lines and
> 
> /same/the same/
> 
> > interrupt source but has separate control and status registers.
> 
> /separate/a separate/
> 
> > Even if target mode is enabled, LPI2C can still work normally as
> > master controller at the same time.
> 
> It's not what happens in the irq handler, though (I left a comment in irq
> handler).
>
> > This patch supports basic target data read/write operations in 7-bit
> > target address. LPI2C target mode can be enabled by using I2C slave
> > backend. I2C slave backend behave like a standard I2C
> 
> /behave/behaves/
> 
> > client. For simple use and test, Linux I2C slave EEPROM backend can be
> > used.
> >
> > Signed-off-by: Carlos Song <carlos.song@xxxxxxx>
> 
> ...
> 
> > +static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id) {
> > +     struct lpi2c_imx_struct *lpi2c_imx = dev_id;
> > +
> > +     if (lpi2c_imx->target) {
> > +             u32 scr = readl(lpi2c_imx->base + LPI2C_SCR);
> > +             u32 ssr = readl(lpi2c_imx->base + LPI2C_SSR);
> > +             u32 sier_filter = ssr & readl(lpi2c_imx->base +
> > + LPI2C_SIER);
> > +
> > +             /* Target is enabled and trigger irq then enter target
> > + irq handler */
> 
> This sentence is a bit hard to understand, how about:
> 
>                 /*
>                  * The target is enabled and an interrupt has
>                  * been triggered. Enter the target's irq handler.
>                  */
> 
> > +             if ((scr & SCR_SEN) && sier_filter)
> > +                     return lpi2c_imx_target_isr(lpi2c_imx, ssr,
> > + sier_filter);
> 
> Can't the interrupt be generated by the master if lpi2c_imx->target is assigned?
> 
> In the git log you are describing a different behavior.
> 
Hi, Andi,

Thanks for your suggestion.

In fact, if lpi2c_imx->target is assigned, master still can generate the interrupt and will also can enter
master's irq handler.

Because LPI2C has independent slave register group and master register group.
SCR register is slave control register, SSR register is slave status register and SIER register is slave interrupt enable register.
So Even if target is assigned, and if the interrupt is triggered by master, this judgment will NOT hit:
		if ((scr & SCR_SEN) && sier_filter)
			return lpi2c_imx_target_isr(lpi2c_imx, ssr, sier_filter);

so logic will keep going then enter the lpi2c_imx_master_isr().

Over all, when lpi2c_imx->target is assigned, either enter the master handler or the target handler.
When lpi2c_imx->target is not assigned, only enter the master handler.

Maybe my commit log "master mode and target mode can work at the same time" is misleading.
I will change to "Both master mode and target mode can be enabled".

Do I understand you correctly? If I do. Please tell me I will send V5 patch for other fixes.

Carlos
> > +     }
> > +
> > +     /* Otherwise triggered by master then handle irq in master
> > + handler */
> 
> Otherwise the interrupt has been triggered by the master. Enter the master's
> irq handler.
> 
> > +     return lpi2c_imx_master_isr(lpi2c_imx); }
> > +
> > +static void lpi2c_imx_target_init(struct lpi2c_imx_struct *lpi2c_imx)
> > +{
> > +     u32 temp;
> > +
> > +     /* reset target module */
> > +     writel(SCR_RST, lpi2c_imx->base + LPI2C_SCR);
> > +     writel(0, lpi2c_imx->base + LPI2C_SCR);
> > +
> > +     /* Set target addr */
> 
> /addr/address/
> 
> > +     writel((lpi2c_imx->target->addr << 1), lpi2c_imx->base +
> > + LPI2C_SAMR);
> > +
> > +     writel(SCFGR1_RXSTALL | SCFGR1_TXDSTALL, lpi2c_imx->base +
> > + LPI2C_SCFGR1);
> > +
> > +     /*
> > +      * set SCFGR2: FILTSDA, FILTSCL and CLKHOLD
> > +      *
> > +      * FILTSCL/FILTSDA can eliminate signal skew. It should generally be
> > +      * set to the same value and should be set >= 50ns.
> > +      *
> > +      * CLKHOLD is only used when clock stretching is enabled, but it will
> > +      * extend the clock stretching to ensure there is an additional delay
> > +      * between the target driving SDA and the target releasing the SCL
> pin.
> > +      *
> > +      * CLKHOLD setting is crucial for lpi2c target. When master read data
> > +      * from target, if there is a delay caused by cpu idle, excessive load,
> > +      * or other delays between two bytes in one message transmission. so
> it
> > +      * will cause a short interval time between the driving SDA
> > + signal and
> 
> /transmission. so it will/transmittion, it will/
> 
> > +      * releasing SCL signal. Lpi2c master will mistakenly think it
> > + is a stop
> 
> /Lpi2c/The lpi2c/
> 
> > +      * signal resulting in an arbitration failure. This issue can be avoided
> > +      * by setting CLKHOLD.
> > +      *
> > +      * In order to ensure lpi2c function normally when the lpi2c speed is as
> > +      * low as 100kHz, CLKHOLD should be set 3 and it is also
> > + compatible with
> 
> /3/to 3/
> 
> > +      * higher clock frequency like 400kHz and 1MHz.
> > +      */
> > +     temp = SCFGR2_FILTSDA(2) | SCFGR2_FILTSCL(2) |
> SCFGR2_CLKHOLD(3);
> > +     writel(temp, lpi2c_imx->base + LPI2C_SCFGR2);
> > +
> > +     /*
> > +      * Enable module:
> > +      * SCR_FILTEN can enable digital filter and output delay counter for
> LPI2C
> > +      * target mode. So SCR_FILTEN need be asserted when enable
> SDA/SCL FILTER
> > +      * and CLKHOLD.
> > +      */
> > +     writel(SCR_SEN | SCR_FILTEN, lpi2c_imx->base + LPI2C_SCR);
> > +
> > +     /* Enable interrupt from i2c module */
> > +     writel(SLAVE_INT_FLAG, lpi2c_imx->base + LPI2C_SIER); }
> > +
> > +static int lpi2c_imx_reg_target(struct i2c_client *client)
> 
> lpi2c_imx_register_target as a name is a bit better, in my opinion
> 
> > +{
> > +     struct lpi2c_imx_struct *lpi2c_imx =
> i2c_get_adapdata(client->adapter);
> > +     int ret;
> > +
> > +     if (lpi2c_imx->target)
> > +             return -EBUSY;
> > +
> > +     lpi2c_imx->target = client;
> > +
> > +     ret = pm_runtime_resume_and_get(lpi2c_imx->adapter.dev.parent);
> > +     if (ret < 0) {
> > +             dev_err(&lpi2c_imx->adapter.dev, "failed to resume i2c
> controller");
> > +             return ret;
> > +     }
> > +
> > +     lpi2c_imx_target_init(lpi2c_imx);
> > +
> > +     return 0;
> > +}
> > +
> > +static int lpi2c_imx_unreg_target(struct i2c_client *client)
> 
> lpi2c_imx_unregister_target sounds better to me.
> 
> > +{
> > +     struct lpi2c_imx_struct *lpi2c_imx =
> i2c_get_adapdata(client->adapter);
> > +     int ret;
> > +
> > +     if (!lpi2c_imx->target)
> > +             return -EINVAL;
> > +
> 
> ...
> 
> > +static int lpi2c_suspend_noirq(struct device *dev) {
> > +     int ret;
> > +
> > +     ret = pm_runtime_force_suspend(dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     return 0;
> 
> This function can simply be:
> 
>    static int lpi2c_suspend_noirq(struct device *dev)
>    {
>         return pm_runtime_force_suspend(dev);
>    }
> 
> but I'm not strong for it, your choice.
> 
> > +}
> > +
> > +static int lpi2c_resume_noirq(struct device *dev) {
> > +     struct lpi2c_imx_struct *lpi2c_imx = dev_get_drvdata(dev);
> > +     int ret;
> > +
> > +     ret = pm_runtime_force_resume(dev);
> > +     if (ret)
> > +             return ret;
> > +
> > +     /*
> > +      * If i2c module powered down in system suspend, register
> > +      * value will lose. So reinit target when system resume.
> > +      */
> 
> Can we re-write this to something like:
> 
>         /*
>          * If the I2C module powers down during system suspend,
>          * the register values will be lost. Therefore, reinitialize
>          * the target when the system resumes.
>          */
> 
> Thanks,
> Andi
> 
> > +     if (lpi2c_imx->target)
> > +             lpi2c_imx_target_init(lpi2c_imx);
> > +
> > +     return 0;
> > +}
> > +
> >  static const struct dev_pm_ops lpi2c_pm_ops = {
> > -     SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
> > -                                   pm_runtime_force_resume)
> > +     SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(lpi2c_suspend_noirq,
> > +                                   lpi2c_resume_noirq)
> >       SET_RUNTIME_PM_OPS(lpi2c_runtime_suspend,
> >                          lpi2c_runtime_resume, NULL)  };
> > --
> > 2.34.1
> >