Message-ID:
 <VI1PR04MB50055F0397031B6518E49751E89B2@VI1PR04MB5005.eurprd04.prod.outlook.com>
Date: Wed, 11 Sep 2024 15:07:42 +0000
From: Carlos Song <carlos.song@....com>
To: "andi.shyti@...nel.org" <andi.shyti@...nel.org>, Aisheng Dong
	<aisheng.dong@....com>, "shawnguo@...nel.org" <shawnguo@...nel.org>,
	"s.hauer@...gutronix.de" <s.hauer@...gutronix.de>, "kernel@...gutronix.de"
	<kernel@...gutronix.de>, "festevam@...il.com" <festevam@...il.com>, Frank Li
	<frank.li@....com>
CC: "linux-i2c@...r.kernel.org" <linux-i2c@...r.kernel.org>,
	"imx@...ts.linux.dev" <imx@...ts.linux.dev>,
	"linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>
Subject: RE: [PATCH V3] i2c: imx-lpi2c: add target mode support

> 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@....com>
> ---
> 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

Powered by blists - more mailing lists