RE: [PATCH V3] i2c: imx-lpi2c: add target mode support

From: Carlos Song
Date: Wed Sep 11 2024 - 11:07:54 EST


> LPI2C support master controller and target controller enabled simultaneously.
> Both controllers share same SDA/SCL lines and interrupt source but has
> separate control and status registers.
> Even if target mode is enabled, LPI2C can still work normally as master
> controller at the same time.
>
> 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 client. For simple use and test, Linux
> I2C slave EEPROM backend can be used.
>

Hi, Andi

Just now I found I still have work to do! Before I notice to need to enrich commit log only.
Oh..It's a little embarrassing. Sorry for missing other comment. I will send V4 then finish the rest:).

> Signed-off-by: Carlos Song <carlos.song@xxxxxxx>
> ---
> Change for V3:
> - According to Andi's suggestion, enrich this patch commit log.
> No code change.
> Change for V2:
> - remove unused variable 'lpi2c_imx' in lpi2c_suspend_noirq.
> ---
> drivers/i2c/busses/i2c-imx-lpi2c.c | 252 ++++++++++++++++++++++++++++-
> 1 file changed, 248 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-imx-lpi2c.c
> b/drivers/i2c/busses/i2c-imx-lpi2c.c
> index f2bbd9898551..2d68faf6847e 100644
> --- a/drivers/i2c/busses/i2c-imx-lpi2c.c
> +++ b/drivers/i2c/busses/i2c-imx-lpi2c.c
> @@ -43,6 +43,20 @@
> #define LPI2C_MTDR 0x60 /* i2c master TX data register */
> #define LPI2C_MRDR 0x70 /* i2c master RX data register */
>
> +#define LPI2C_SCR 0x110 /* i2c target contrl register */
> +#define LPI2C_SSR 0x114 /* i2c target status register */
> +#define LPI2C_SIER 0x118 /* i2c target interrupt enable */
> +#define LPI2C_SDER 0x11C /* i2c target DMA enable */
> +#define LPI2C_SCFGR0 0x120 /* i2c target configuration */
> +#define LPI2C_SCFGR1 0x124 /* i2c target configuration */
> +#define LPI2C_SCFGR2 0x128 /* i2c target configuration */
> +#define LPI2C_SAMR 0x140 /* i2c target address match */
> +#define LPI2C_SASR 0x150 /* i2c target address status */
> +#define LPI2C_STAR 0x154 /* i2c target transmit ACK */
> +#define LPI2C_STDR 0x160 /* i2c target transmit data */
> +#define LPI2C_SRDR 0x170 /* i2c target receive data */
> +#define LPI2C_SRDROR 0x178 /* i2c target receive data read only */
> +
> /* i2c command */
> #define TRAN_DATA 0X00
> #define RECV_DATA 0X01
> @@ -76,6 +90,42 @@
> #define MDER_TDDE BIT(0)
> #define MDER_RDDE BIT(1)
>
> +#define SCR_SEN BIT(0)
> +#define SCR_RST BIT(1)
> +#define SCR_FILTEN BIT(4)
> +#define SCR_RTF BIT(8)
> +#define SCR_RRF BIT(9)
> +#define SCFGR1_RXSTALL BIT(1)
> +#define SCFGR1_TXDSTALL BIT(2)
> +#define SCFGR2_FILTSDA_SHIFT 24
> +#define SCFGR2_FILTSCL_SHIFT 16
> +#define SCFGR2_CLKHOLD(x) (x)
> +#define SCFGR2_FILTSDA(x) ((x) << SCFGR2_FILTSDA_SHIFT)
> +#define SCFGR2_FILTSCL(x) ((x) << SCFGR2_FILTSCL_SHIFT)
> +#define SSR_TDF BIT(0)
> +#define SSR_RDF BIT(1)
> +#define SSR_AVF BIT(2)
> +#define SSR_TAF BIT(3)
> +#define SSR_RSF BIT(8)
> +#define SSR_SDF BIT(9)
> +#define SSR_BEF BIT(10)
> +#define SSR_FEF BIT(11)
> +#define SSR_SBF BIT(24)
> +#define SSR_BBF BIT(25)
> +#define SSR_CLEAR_BITS (SSR_RSF | SSR_SDF | SSR_BEF | SSR_FEF)
> +#define SIER_TDIE BIT(0)
> +#define SIER_RDIE BIT(1)
> +#define SIER_AVIE BIT(2)
> +#define SIER_TAIE BIT(3)
> +#define SIER_RSIE BIT(8)
> +#define SIER_SDIE BIT(9)
> +#define SIER_BEIE BIT(10)
> +#define SIER_FEIE BIT(11)
> +#define SIER_AM0F BIT(12)
> +#define SASR_READ_REQ 0x1
> +#define SLAVE_INT_FLAG (SIER_TDIE | SIER_RDIE | SIER_AVIE | \
> + SIER_SDIE | SIER_BEIE)
> +
> #define I2C_CLK_RATIO 2
> #define CHUNK_DATA 256
>
> @@ -134,6 +184,7 @@ struct lpi2c_imx_struct {
> struct i2c_bus_recovery_info rinfo;
> bool can_use_dma;
> struct lpi2c_imx_dma *dma;
> + struct i2c_client *target;
> };
>
> static void lpi2c_imx_intctrl(struct lpi2c_imx_struct *lpi2c_imx, @@ -958,9
> +1009,57 @@ static int lpi2c_imx_xfer(struct i2c_adapter *adapter,
> return (result < 0) ? result : num;
> }
>
> -static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id)
> +static irqreturn_t lpi2c_imx_target_isr(struct lpi2c_imx_struct *lpi2c_imx,
> + u32 ssr, u32 sier_filter)
> +{
> + u8 value;
> + u32 sasr;
> +
> + /* Arbitration lost */
> + if (sier_filter & SSR_BEF) {
> + writel(0, lpi2c_imx->base + LPI2C_SIER);
> + return IRQ_HANDLED;
> + }
> +
> + /* Address detected */
> + if (sier_filter & SSR_AVF) {
> + sasr = readl(lpi2c_imx->base + LPI2C_SASR);
> + if (SASR_READ_REQ & sasr) {
> + /* Read request */
> + i2c_slave_event(lpi2c_imx->target,
> I2C_SLAVE_READ_REQUESTED, &value);
> + writel(value, lpi2c_imx->base + LPI2C_STDR);
> + goto ret;
> + } else {
> + /* Write request */
> + i2c_slave_event(lpi2c_imx->target,
> I2C_SLAVE_WRITE_REQUESTED, &value);
> + }
> + }
> +
> + if (sier_filter & SSR_SDF) {
> + /* STOP */
> + i2c_slave_event(lpi2c_imx->target, I2C_SLAVE_STOP, &value);
> + }
> +
> + if (sier_filter & SSR_TDF) {
> + /* Target send data */
> + i2c_slave_event(lpi2c_imx->target, I2C_SLAVE_READ_PROCESSED,
> &value);
> + writel(value, lpi2c_imx->base + LPI2C_STDR);
> + }
> +
> + if (sier_filter & SSR_RDF) {
> + /* Target receive data */
> + value = readl(lpi2c_imx->base + LPI2C_SRDR);
> + i2c_slave_event(lpi2c_imx->target, I2C_SLAVE_WRITE_RECEIVED,
> &value);
> + }
> +
> +ret:
> + /* Clear SSR */
> + writel(ssr & SSR_CLEAR_BITS, lpi2c_imx->base + LPI2C_SSR);
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t lpi2c_imx_master_isr(struct lpi2c_imx_struct
> +*lpi2c_imx)
> {
> - struct lpi2c_imx_struct *lpi2c_imx = dev_id;
> unsigned int enabled;
> unsigned int temp;
>
> @@ -980,6 +1079,119 @@ static irqreturn_t lpi2c_imx_isr(int irq, void
> *dev_id)
> return IRQ_HANDLED;
> }
>
> +static irqreturn_t lpi2c_imx_isr(int irq, void *dev_id) {
> + struct lpi2c_imx_struct *lpi2c_imx = dev_id;
> + u32 ssr, sier_filter;
> + unsigned int scr;
> +
> + if (lpi2c_imx->target) {
> + scr = readl(lpi2c_imx->base + LPI2C_SCR);
> + ssr = readl(lpi2c_imx->base + LPI2C_SSR);
> + sier_filter = ssr & readl(lpi2c_imx->base + LPI2C_SIER);
> + if ((scr & SCR_SEN) && sier_filter)
> + return lpi2c_imx_target_isr(lpi2c_imx, ssr, sier_filter);
> + else
> + return lpi2c_imx_master_isr(lpi2c_imx);
> + } else {
> + return lpi2c_imx_master_isr(lpi2c_imx);
> + }
> +}
> +
> +static void lpi2c_imx_target_init(struct lpi2c_imx_struct *lpi2c_imx) {
> + int temp;
> +
> + /* reset target module */
> + writel(SCR_RST, lpi2c_imx->base + LPI2C_SCR);
> + writel(0, lpi2c_imx->base + LPI2C_SCR);
> +
> + /* Set target addr */
> + 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
> + * releasing SCL signal. Lpi2c master will mistakenly think it is a stop
> + * 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
> + * 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) {
> + 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) {
> + struct lpi2c_imx_struct *lpi2c_imx = i2c_get_adapdata(client->adapter);
> + int ret;
> +
> + if (!lpi2c_imx->target)
> + return -EINVAL;
> +
> + /* Reset target address. */
> + writel(0, lpi2c_imx->base + LPI2C_SAMR);
> +
> + writel(SCR_RST, lpi2c_imx->base + LPI2C_SCR);
> + writel(0, lpi2c_imx->base + LPI2C_SCR);
> +
> + lpi2c_imx->target = NULL;
> +
> + ret = pm_runtime_put_sync(lpi2c_imx->adapter.dev.parent);
> + if (ret < 0)
> + dev_err(&lpi2c_imx->adapter.dev, "failed to suspend i2c controller");
> +
> + return ret;
> +}
> +
> static int lpi2c_imx_init_recovery_info(struct lpi2c_imx_struct *lpi2c_imx,
> struct platform_device *pdev)
> {
> @@ -1055,6 +1267,8 @@ static u32 lpi2c_imx_func(struct i2c_adapter
> *adapter) static const struct i2c_algorithm lpi2c_imx_algo = {
> .master_xfer = lpi2c_imx_xfer,
> .functionality = lpi2c_imx_func,
> + .reg_slave = lpi2c_imx_reg_target,
> + .unreg_slave = lpi2c_imx_unreg_target,
> };
>
> static const struct of_device_id lpi2c_imx_of_match[] = { @@ -1205,9
> +1419,39 @@ static int __maybe_unused lpi2c_runtime_resume(struct device
> *dev)
> return 0;
> }
>
> +static int lpi2c_suspend_noirq(struct device *dev) {
> + int ret;
> +
> + ret = pm_runtime_force_suspend(dev);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +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.
> + */
> + 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