public inbox for linux-i3c@lists.infradead.org
 help / color / mirror / Atom feed
* [PATCH] i3c: ast2600: Generate T-bits to terminate IBI storm
@ 2025-06-11  4:02 Billy Tsai
  2025-06-11 17:05 ` Frank Li
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Billy Tsai @ 2025-06-11  4:02 UTC (permalink / raw)
  To: jk, alexandre.belloni, Frank.Li, aniketmaurya, billy_tsai,
	jarkko.nikula, Shyam-sundar.S-k, wsa+renesas, linux-i3c,
	linux-kernel, BMC-SW, elbadrym, romlem

Under certain conditions, such as when an IBI interrupt is received and
SDA remains high after the address phase, the I3C controller will enter
an infinite loop attempting to read data until a T-bit is detected.
This commit addresses the issue by generating a fake T-bit to terminate
the IBI storm when the received IBI data length exceeds the maximum
allowable IBI payload.
This issue cannot be resolved using the abort function, as it is
ineffective when the I3C FSM is in the Servicing IBI Transfer (0xE) or
Clock Extension (0x12) states.

Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
---
 drivers/i3c/master/ast2600-i3c-master.c | 60 +++++++++++++++++++++++++
 drivers/i3c/master/dw-i3c-master.c      | 14 ++++++
 drivers/i3c/master/dw-i3c-master.h      |  9 ++++
 3 files changed, 83 insertions(+)

diff --git a/drivers/i3c/master/ast2600-i3c-master.c b/drivers/i3c/master/ast2600-i3c-master.c
index e05e83812c71..6ac0122474d0 100644
--- a/drivers/i3c/master/ast2600-i3c-master.c
+++ b/drivers/i3c/master/ast2600-i3c-master.c
@@ -33,11 +33,28 @@
 #define AST2600_I3CG_REG1_SA_EN			BIT(15)
 #define AST2600_I3CG_REG1_INST_ID_MASK		GENMASK(19, 16)
 #define AST2600_I3CG_REG1_INST_ID(x)		(((x) << 16) & AST2600_I3CG_REG1_INST_ID_MASK)
+#define AST2600_I3CG_REG1_SCL_SW_MODE_OE	BIT(20)
+#define AST2600_I3CG_REG1_SCL_OUT_SW_MODE_VAL	BIT(21)
+#define AST2600_I3CG_REG1_SCL_IN_SW_MODE_VAL	BIT(23)
+#define AST2600_I3CG_REG1_SDA_SW_MODE_OE	BIT(24)
+#define AST2600_I3CG_REG1_SDA_OUT_SW_MODE_VAL	BIT(25)
+#define AST2600_I3CG_REG1_SDA_IN_SW_MODE_VAL	BIT(27)
+#define AST2600_I3CG_REG1_SCL_IN_SW_MODE_EN	BIT(28)
+#define AST2600_I3CG_REG1_SDA_IN_SW_MODE_EN	BIT(29)
+#define AST2600_I3CG_REG1_SCL_OUT_SW_MODE_EN	BIT(30)
+#define AST2600_I3CG_REG1_SDA_OUT_SW_MODE_EN	BIT(31)
 
 #define AST2600_DEFAULT_SDA_PULLUP_OHMS		2000
 
 #define DEV_ADDR_TABLE_IBI_PEC			BIT(11)
 
+#define IBI_QUEUE_STATUS		0x18
+#define PRESENT_STATE			0x54
+#define   CM_TFR_STS			GENMASK(13, 8)
+#define     CM_TFR_STS_MASTER_SERV_IBI	0xe
+#define   SDA_LINE_SIGNAL_LEVEL		BIT(1)
+#define   SCL_LINE_SIGNAL_LEVEL		BIT(0)
+
 struct ast2600_i3c {
 	struct dw_i3c_master dw;
 	struct regmap *global_regs;
@@ -117,9 +134,52 @@ static void ast2600_i3c_set_dat_ibi(struct dw_i3c_master *i3c,
 	}
 }
 
+static bool ast2600_i3c_fsm_exit_serv_ibi(struct dw_i3c_master *dw)
+{
+	u32 state;
+
+	/*
+	 * Clear the IBI queue to enable the hardware to generate SCL and
+	 * begin detecting the T-bit low to stop reading IBI data.
+	 */
+	readl(dw->regs + IBI_QUEUE_STATUS);
+	state = FIELD_GET(CM_TFR_STS, readl(dw->regs + PRESENT_STATE));
+	if (state == CM_TFR_STS_MASTER_SERV_IBI)
+		return false;
+
+	return true;
+}
+
+static void ast2600_i3c_gen_tbits_in(struct dw_i3c_master *dw)
+{
+	struct ast2600_i3c *i3c = to_ast2600_i3c(dw);
+	bool is_idle;
+	int ret;
+
+	regmap_write_bits(i3c->global_regs, AST2600_I3CG_REG1(i3c->global_idx),
+			  AST2600_I3CG_REG1_SDA_IN_SW_MODE_VAL,
+			  AST2600_I3CG_REG1_SDA_IN_SW_MODE_VAL);
+	regmap_write_bits(i3c->global_regs, AST2600_I3CG_REG1(i3c->global_idx),
+			  AST2600_I3CG_REG1_SDA_IN_SW_MODE_EN,
+			  AST2600_I3CG_REG1_SDA_IN_SW_MODE_EN);
+
+	regmap_write_bits(i3c->global_regs, AST2600_I3CG_REG1(i3c->global_idx),
+			  AST2600_I3CG_REG1_SDA_IN_SW_MODE_VAL, 0);
+	ret = readx_poll_timeout_atomic(ast2600_i3c_fsm_exit_serv_ibi, dw,
+					is_idle, is_idle, 0, 2000000);
+	regmap_write_bits(i3c->global_regs, AST2600_I3CG_REG1(i3c->global_idx),
+			  AST2600_I3CG_REG1_SDA_IN_SW_MODE_EN, 0);
+	if (ret)
+		dev_err(&dw->base.dev,
+			"Failed to exit the I3C fsm from %lx(MASTER_SERV_IBI): %d",
+			FIELD_GET(CM_TFR_STS, readl(dw->regs + PRESENT_STATE)),
+			ret);
+}
+
 static const struct dw_i3c_platform_ops ast2600_i3c_ops = {
 	.init = ast2600_i3c_init,
 	.set_dat_ibi = ast2600_i3c_set_dat_ibi,
+	.gen_tbits_in = ast2600_i3c_gen_tbits_in,
 };
 
 static int ast2600_i3c_probe(struct platform_device *pdev)
diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
index 611c22b72c15..380e6a29c7b8 100644
--- a/drivers/i3c/master/dw-i3c-master.c
+++ b/drivers/i3c/master/dw-i3c-master.c
@@ -158,6 +158,14 @@
 #define DATA_BUFFER_STATUS_LEVEL_TX(x)	((x) & GENMASK(7, 0))
 
 #define PRESENT_STATE			0x54
+#define   CM_TFR_ST_STS			GENMASK(21, 16)
+#define     CM_TFR_ST_STS_HALT		0x13
+#define   CM_TFR_STS			GENMASK(13, 8)
+#define     CM_TFR_STS_MASTER_SERV_IBI	0xe
+#define     CM_TFR_STS_MASTER_HALT	0xf
+#define     CM_TFR_STS_SLAVE_HALT	0x6
+#define   SDA_LINE_SIGNAL_LEVEL		BIT(1)
+#define   SCL_LINE_SIGNAL_LEVEL		BIT(0)
 #define CCC_DEVICE_STATUS		0x58
 #define DEVICE_ADDR_TABLE_POINTER	0x5c
 #define DEVICE_ADDR_TABLE_DEPTH(x)	(((x) & GENMASK(31, 16)) >> 16)
@@ -1393,6 +1401,8 @@ static void dw_i3c_master_handle_ibi_sir(struct dw_i3c_master *master,
 	unsigned long flags;
 	u8 addr, len;
 	int idx;
+	bool terminate_ibi = false;
+	u32 state;
 
 	addr = IBI_QUEUE_IBI_ADDR(status);
 	len = IBI_QUEUE_STATUS_DATA_LEN(status);
@@ -1435,6 +1445,7 @@ static void dw_i3c_master_handle_ibi_sir(struct dw_i3c_master *master,
 		dev_dbg_ratelimited(&master->base.dev,
 				    "IBI payload len %d greater than max %d\n",
 				    len, dev->ibi->max_payload_len);
+		terminate_ibi = true;
 		goto err_drain;
 	}
 
@@ -1450,6 +1461,9 @@ static void dw_i3c_master_handle_ibi_sir(struct dw_i3c_master *master,
 
 err_drain:
 	dw_i3c_master_drain_ibi_queue(master, len);
+	state = FIELD_GET(CM_TFR_STS, readl(master->regs + PRESENT_STATE));
+	if (terminate_ibi && state == CM_TFR_STS_MASTER_SERV_IBI)
+		master->platform_ops->gen_tbits_in(master);
 
 	spin_unlock_irqrestore(&master->devs_lock, flags);
 }
diff --git a/drivers/i3c/master/dw-i3c-master.h b/drivers/i3c/master/dw-i3c-master.h
index c5cb695c16ab..1da485e42e74 100644
--- a/drivers/i3c/master/dw-i3c-master.h
+++ b/drivers/i3c/master/dw-i3c-master.h
@@ -89,6 +89,15 @@ struct dw_i3c_platform_ops {
 	 */
 	void (*set_dat_ibi)(struct dw_i3c_master *i3c,
 			    struct i3c_dev_desc *dev, bool enable, u32 *reg);
+	/*
+	 * Gerenating the fake t-bit (SDA low) to stop the IBI storm when the received
+	 * data length of IBI is larger than the maximum IBI payload.
+	 *
+	 * When an IBI is received and SDA remains high after the address phase, the i3c
+	 * controller may enter an infinite loop while trying to read data until the t-bit
+	 * appears
+	 */
+	void (*gen_tbits_in)(struct dw_i3c_master *i3c);
 };
 
 extern int dw_i3c_common_probe(struct dw_i3c_master *master,
-- 
2.25.1


-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] i3c: ast2600: Generate T-bits to terminate IBI storm
  2025-06-11  4:02 [PATCH] i3c: ast2600: Generate T-bits to terminate IBI storm Billy Tsai
@ 2025-06-11 17:05 ` Frank Li
  2025-06-18  5:02   ` Billy Tsai
  2025-06-11 19:44 ` kernel test robot
  2025-06-16  2:31 ` Joshua Yeong
  2 siblings, 1 reply; 5+ messages in thread
From: Frank Li @ 2025-06-11 17:05 UTC (permalink / raw)
  To: Billy Tsai
  Cc: jk, alexandre.belloni, aniketmaurya, jarkko.nikula,
	Shyam-sundar.S-k, wsa+renesas, linux-i3c, linux-kernel, BMC-SW,
	elbadrym, romlem

On Wed, Jun 11, 2025 at 12:02:03PM +0800, Billy Tsai wrote:
> Under certain conditions, such as when an IBI interrupt is received and
> SDA remains high after the address phase,

Can you descript more clear?

Generally IBI happen at below two case
1. SDA pull down by target
2. Address arbitration happen

Then Host send out ACK,  according to your descrpition, look like host
NACK target IBI request.

> the I3C controller will enter
> an infinite loop attempting to read data until a T-bit is detected.

BCR/DCR have indicate IBI mandatory data length. You should know how many
data need be read according to IBI won's target address. Why relay on T-bit,
which just is used for when target have less data than what expected.

> This commit addresses the issue by generating a fake T-bit to terminate
> the IBI storm when the received IBI data length exceeds the maximum
> allowable IBI payload.

Add empty line here.

> This issue cannot be resolved using the abort function, as it is
> ineffective when the I3C FSM is in the Servicing IBI Transfer (0xE) or
> Clock Extension (0x12) states.

why ineffective?

Frank
>
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
>  drivers/i3c/master/ast2600-i3c-master.c | 60 +++++++++++++++++++++++++
>  drivers/i3c/master/dw-i3c-master.c      | 14 ++++++
>  drivers/i3c/master/dw-i3c-master.h      |  9 ++++
>  3 files changed, 83 insertions(+)
>
> diff --git a/drivers/i3c/master/ast2600-i3c-master.c b/drivers/i3c/master/ast2600-i3c-master.c
> index e05e83812c71..6ac0122474d0 100644
> --- a/drivers/i3c/master/ast2600-i3c-master.c
> +++ b/drivers/i3c/master/ast2600-i3c-master.c
> @@ -33,11 +33,28 @@
>  #define AST2600_I3CG_REG1_SA_EN			BIT(15)
>  #define AST2600_I3CG_REG1_INST_ID_MASK		GENMASK(19, 16)
>  #define AST2600_I3CG_REG1_INST_ID(x)		(((x) << 16) & AST2600_I3CG_REG1_INST_ID_MASK)
> +#define AST2600_I3CG_REG1_SCL_SW_MODE_OE	BIT(20)
> +#define AST2600_I3CG_REG1_SCL_OUT_SW_MODE_VAL	BIT(21)
> +#define AST2600_I3CG_REG1_SCL_IN_SW_MODE_VAL	BIT(23)
> +#define AST2600_I3CG_REG1_SDA_SW_MODE_OE	BIT(24)
> +#define AST2600_I3CG_REG1_SDA_OUT_SW_MODE_VAL	BIT(25)
> +#define AST2600_I3CG_REG1_SDA_IN_SW_MODE_VAL	BIT(27)
> +#define AST2600_I3CG_REG1_SCL_IN_SW_MODE_EN	BIT(28)
> +#define AST2600_I3CG_REG1_SDA_IN_SW_MODE_EN	BIT(29)
> +#define AST2600_I3CG_REG1_SCL_OUT_SW_MODE_EN	BIT(30)
> +#define AST2600_I3CG_REG1_SDA_OUT_SW_MODE_EN	BIT(31)
>
>  #define AST2600_DEFAULT_SDA_PULLUP_OHMS		2000
>
>  #define DEV_ADDR_TABLE_IBI_PEC			BIT(11)
>
> +#define IBI_QUEUE_STATUS		0x18
> +#define PRESENT_STATE			0x54
> +#define   CM_TFR_STS			GENMASK(13, 8)
> +#define     CM_TFR_STS_MASTER_SERV_IBI	0xe
> +#define   SDA_LINE_SIGNAL_LEVEL		BIT(1)
> +#define   SCL_LINE_SIGNAL_LEVEL		BIT(0)
> +
>  struct ast2600_i3c {
>  	struct dw_i3c_master dw;
>  	struct regmap *global_regs;
> @@ -117,9 +134,52 @@ static void ast2600_i3c_set_dat_ibi(struct dw_i3c_master *i3c,
>  	}
>  }
>
> +static bool ast2600_i3c_fsm_exit_serv_ibi(struct dw_i3c_master *dw)
> +{
> +	u32 state;
> +
> +	/*
> +	 * Clear the IBI queue to enable the hardware to generate SCL and
> +	 * begin detecting the T-bit low to stop reading IBI data.
> +	 */
> +	readl(dw->regs + IBI_QUEUE_STATUS);
> +	state = FIELD_GET(CM_TFR_STS, readl(dw->regs + PRESENT_STATE));
> +	if (state == CM_TFR_STS_MASTER_SERV_IBI)
> +		return false;
> +
> +	return true;
> +}
> +
> +static void ast2600_i3c_gen_tbits_in(struct dw_i3c_master *dw)
> +{
> +	struct ast2600_i3c *i3c = to_ast2600_i3c(dw);
> +	bool is_idle;
> +	int ret;
> +
> +	regmap_write_bits(i3c->global_regs, AST2600_I3CG_REG1(i3c->global_idx),
> +			  AST2600_I3CG_REG1_SDA_IN_SW_MODE_VAL,
> +			  AST2600_I3CG_REG1_SDA_IN_SW_MODE_VAL);
> +	regmap_write_bits(i3c->global_regs, AST2600_I3CG_REG1(i3c->global_idx),
> +			  AST2600_I3CG_REG1_SDA_IN_SW_MODE_EN,
> +			  AST2600_I3CG_REG1_SDA_IN_SW_MODE_EN);
> +
> +	regmap_write_bits(i3c->global_regs, AST2600_I3CG_REG1(i3c->global_idx),
> +			  AST2600_I3CG_REG1_SDA_IN_SW_MODE_VAL, 0);
> +	ret = readx_poll_timeout_atomic(ast2600_i3c_fsm_exit_serv_ibi, dw,
> +					is_idle, is_idle, 0, 2000000);
> +	regmap_write_bits(i3c->global_regs, AST2600_I3CG_REG1(i3c->global_idx),
> +			  AST2600_I3CG_REG1_SDA_IN_SW_MODE_EN, 0);
> +	if (ret)
> +		dev_err(&dw->base.dev,
> +			"Failed to exit the I3C fsm from %lx(MASTER_SERV_IBI): %d",
> +			FIELD_GET(CM_TFR_STS, readl(dw->regs + PRESENT_STATE)),
> +			ret);
> +}
> +
>  static const struct dw_i3c_platform_ops ast2600_i3c_ops = {
>  	.init = ast2600_i3c_init,
>  	.set_dat_ibi = ast2600_i3c_set_dat_ibi,
> +	.gen_tbits_in = ast2600_i3c_gen_tbits_in,
>  };
>
>  static int ast2600_i3c_probe(struct platform_device *pdev)
> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index 611c22b72c15..380e6a29c7b8 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -158,6 +158,14 @@
>  #define DATA_BUFFER_STATUS_LEVEL_TX(x)	((x) & GENMASK(7, 0))
>
>  #define PRESENT_STATE			0x54
> +#define   CM_TFR_ST_STS			GENMASK(21, 16)
> +#define     CM_TFR_ST_STS_HALT		0x13
> +#define   CM_TFR_STS			GENMASK(13, 8)
> +#define     CM_TFR_STS_MASTER_SERV_IBI	0xe
> +#define     CM_TFR_STS_MASTER_HALT	0xf
> +#define     CM_TFR_STS_SLAVE_HALT	0x6
> +#define   SDA_LINE_SIGNAL_LEVEL		BIT(1)
> +#define   SCL_LINE_SIGNAL_LEVEL		BIT(0)
>  #define CCC_DEVICE_STATUS		0x58
>  #define DEVICE_ADDR_TABLE_POINTER	0x5c
>  #define DEVICE_ADDR_TABLE_DEPTH(x)	(((x) & GENMASK(31, 16)) >> 16)
> @@ -1393,6 +1401,8 @@ static void dw_i3c_master_handle_ibi_sir(struct dw_i3c_master *master,
>  	unsigned long flags;
>  	u8 addr, len;
>  	int idx;
> +	bool terminate_ibi = false;
> +	u32 state;
>
>  	addr = IBI_QUEUE_IBI_ADDR(status);
>  	len = IBI_QUEUE_STATUS_DATA_LEN(status);
> @@ -1435,6 +1445,7 @@ static void dw_i3c_master_handle_ibi_sir(struct dw_i3c_master *master,
>  		dev_dbg_ratelimited(&master->base.dev,
>  				    "IBI payload len %d greater than max %d\n",
>  				    len, dev->ibi->max_payload_len);
> +		terminate_ibi = true;
>  		goto err_drain;
>  	}
>
> @@ -1450,6 +1461,9 @@ static void dw_i3c_master_handle_ibi_sir(struct dw_i3c_master *master,
>
>  err_drain:
>  	dw_i3c_master_drain_ibi_queue(master, len);
> +	state = FIELD_GET(CM_TFR_STS, readl(master->regs + PRESENT_STATE));
> +	if (terminate_ibi && state == CM_TFR_STS_MASTER_SERV_IBI)
> +		master->platform_ops->gen_tbits_in(master);
>
>  	spin_unlock_irqrestore(&master->devs_lock, flags);
>  }
> diff --git a/drivers/i3c/master/dw-i3c-master.h b/drivers/i3c/master/dw-i3c-master.h
> index c5cb695c16ab..1da485e42e74 100644
> --- a/drivers/i3c/master/dw-i3c-master.h
> +++ b/drivers/i3c/master/dw-i3c-master.h
> @@ -89,6 +89,15 @@ struct dw_i3c_platform_ops {
>  	 */
>  	void (*set_dat_ibi)(struct dw_i3c_master *i3c,
>  			    struct i3c_dev_desc *dev, bool enable, u32 *reg);
> +	/*
> +	 * Gerenating the fake t-bit (SDA low) to stop the IBI storm when the received
> +	 * data length of IBI is larger than the maximum IBI payload.
> +	 *
> +	 * When an IBI is received and SDA remains high after the address phase, the i3c
> +	 * controller may enter an infinite loop while trying to read data until the t-bit
> +	 * appears
> +	 */
> +	void (*gen_tbits_in)(struct dw_i3c_master *i3c);
>  };
>
>  extern int dw_i3c_common_probe(struct dw_i3c_master *master,
> --
> 2.25.1
>

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] i3c: ast2600: Generate T-bits to terminate IBI storm
  2025-06-11  4:02 [PATCH] i3c: ast2600: Generate T-bits to terminate IBI storm Billy Tsai
  2025-06-11 17:05 ` Frank Li
@ 2025-06-11 19:44 ` kernel test robot
  2025-06-16  2:31 ` Joshua Yeong
  2 siblings, 0 replies; 5+ messages in thread
From: kernel test robot @ 2025-06-11 19:44 UTC (permalink / raw)
  To: Billy Tsai, jk, alexandre.belloni, Frank.Li, aniketmaurya,
	jarkko.nikula, Shyam-sundar.S-k, wsa+renesas, linux-i3c,
	linux-kernel, BMC-SW, elbadrym, romlem
  Cc: oe-kbuild-all

Hi Billy,

kernel test robot noticed the following build errors:

[auto build test ERROR on linus/master]
[also build test ERROR on v6.16-rc1 next-20250611]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Billy-Tsai/i3c-ast2600-Generate-T-bits-to-terminate-IBI-storm/20250611-120300
base:   linus/master
patch link:    https://lore.kernel.org/r/20250611040203.487734-1-billy_tsai%40aspeedtech.com
patch subject: [PATCH] i3c: ast2600: Generate T-bits to terminate IBI storm
config: powerpc64-randconfig-r072-20250612 (https://download.01.org/0day-ci/archive/20250612/202506120357.1kFiVfmV-lkp@intel.com/config)
compiler: powerpc64-linux-gcc (GCC) 8.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250612/202506120357.1kFiVfmV-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506120357.1kFiVfmV-lkp@intel.com/

All error/warnings (new ones prefixed by >>):

   drivers/i3c/master/ast2600-i3c-master.c: In function 'ast2600_i3c_fsm_exit_serv_ibi':
>> drivers/i3c/master/ast2600-i3c-master.c:146:10: error: implicit declaration of function 'FIELD_GET' [-Werror=implicit-function-declaration]
     state = FIELD_GET(CM_TFR_STS, readl(dw->regs + PRESENT_STATE));
             ^~~~~~~~~
   In file included from include/linux/device.h:15,
                    from include/linux/platform_device.h:13,
                    from drivers/i3c/master/ast2600-i3c-master.c:11:
   drivers/i3c/master/ast2600-i3c-master.c: In function 'ast2600_i3c_gen_tbits_in':
>> drivers/i3c/master/ast2600-i3c-master.c:174:4: warning: format '%lx' expects argument of type 'long unsigned int', but argument 3 has type 'int' [-Wformat=]
       "Failed to exit the I3C fsm from %lx(MASTER_SERV_IBI): %d",
       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:110:16: note: in definition of macro 'dev_printk_index_wrap'
      _p_func(dev, fmt, ##__VA_ARGS__);   \
                   ^~~
   include/linux/dev_printk.h:154:49: note: in expansion of macro 'dev_fmt'
     dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__)
                                                    ^~~~~~~
   drivers/i3c/master/ast2600-i3c-master.c:173:3: note: in expansion of macro 'dev_err'
      dev_err(&dw->base.dev,
      ^~~~~~~
   cc1: some warnings being treated as errors


vim +/FIELD_GET +146 drivers/i3c/master/ast2600-i3c-master.c

   136	
   137	static bool ast2600_i3c_fsm_exit_serv_ibi(struct dw_i3c_master *dw)
   138	{
   139		u32 state;
   140	
   141		/*
   142		 * Clear the IBI queue to enable the hardware to generate SCL and
   143		 * begin detecting the T-bit low to stop reading IBI data.
   144		 */
   145		readl(dw->regs + IBI_QUEUE_STATUS);
 > 146		state = FIELD_GET(CM_TFR_STS, readl(dw->regs + PRESENT_STATE));
   147		if (state == CM_TFR_STS_MASTER_SERV_IBI)
   148			return false;
   149	
   150		return true;
   151	}
   152	
   153	static void ast2600_i3c_gen_tbits_in(struct dw_i3c_master *dw)
   154	{
   155		struct ast2600_i3c *i3c = to_ast2600_i3c(dw);
   156		bool is_idle;
   157		int ret;
   158	
   159		regmap_write_bits(i3c->global_regs, AST2600_I3CG_REG1(i3c->global_idx),
   160				  AST2600_I3CG_REG1_SDA_IN_SW_MODE_VAL,
   161				  AST2600_I3CG_REG1_SDA_IN_SW_MODE_VAL);
   162		regmap_write_bits(i3c->global_regs, AST2600_I3CG_REG1(i3c->global_idx),
   163				  AST2600_I3CG_REG1_SDA_IN_SW_MODE_EN,
   164				  AST2600_I3CG_REG1_SDA_IN_SW_MODE_EN);
   165	
   166		regmap_write_bits(i3c->global_regs, AST2600_I3CG_REG1(i3c->global_idx),
   167				  AST2600_I3CG_REG1_SDA_IN_SW_MODE_VAL, 0);
   168		ret = readx_poll_timeout_atomic(ast2600_i3c_fsm_exit_serv_ibi, dw,
   169						is_idle, is_idle, 0, 2000000);
   170		regmap_write_bits(i3c->global_regs, AST2600_I3CG_REG1(i3c->global_idx),
   171				  AST2600_I3CG_REG1_SDA_IN_SW_MODE_EN, 0);
   172		if (ret)
   173			dev_err(&dw->base.dev,
 > 174				"Failed to exit the I3C fsm from %lx(MASTER_SERV_IBI): %d",
   175				FIELD_GET(CM_TFR_STS, readl(dw->regs + PRESENT_STATE)),
   176				ret);
   177	}
   178	

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] i3c: ast2600: Generate T-bits to terminate IBI storm
  2025-06-11  4:02 [PATCH] i3c: ast2600: Generate T-bits to terminate IBI storm Billy Tsai
  2025-06-11 17:05 ` Frank Li
  2025-06-11 19:44 ` kernel test robot
@ 2025-06-16  2:31 ` Joshua Yeong
  2 siblings, 0 replies; 5+ messages in thread
From: Joshua Yeong @ 2025-06-16  2:31 UTC (permalink / raw)
  To: linux-i3c@lists.infradead.org

On 11-Jun-2025 12:02 PM, Billy Tsai wrote:
> Under certain conditions, such as when an IBI interrupt is received and
> SDA remains high after the address phase, the I3C controller will enter
> an infinite loop attempting to read data until a T-bit is detected.
> This commit addresses the issue by generating a fake T-bit to terminate
> the IBI storm when the received IBI data length exceeds the maximum
> allowable IBI payload.
> This issue cannot be resolved using the abort function, as it is
> ineffective when the I3C FSM is in the Servicing IBI Transfer (0xE) or
> Clock Extension (0x12) states.
> 
> Signed-off-by: Billy Tsai <billy_tsai@aspeedtech.com>
> ---
>  drivers/i3c/master/ast2600-i3c-master.c | 60 +++++++++++++++++++++++++
>  drivers/i3c/master/dw-i3c-master.c      | 14 ++++++
>  drivers/i3c/master/dw-i3c-master.h      |  9 ++++
>  3 files changed, 83 insertions(+)
> 
> diff --git a/drivers/i3c/master/ast2600-i3c-master.c b/drivers/i3c/master/ast2600-i3c-master.c
> index e05e83812c71..6ac0122474d0 100644
> --- a/drivers/i3c/master/ast2600-i3c-master.c
> +++ b/drivers/i3c/master/ast2600-i3c-master.c
> @@ -33,11 +33,28 @@
>  #define AST2600_I3CG_REG1_SA_EN			BIT(15)
>  #define AST2600_I3CG_REG1_INST_ID_MASK		GENMASK(19, 16)
>  #define AST2600_I3CG_REG1_INST_ID(x)		(((x) << 16) & AST2600_I3CG_REG1_INST_ID_MASK)
> +#define AST2600_I3CG_REG1_SCL_SW_MODE_OE	BIT(20)
> +#define AST2600_I3CG_REG1_SCL_OUT_SW_MODE_VAL	BIT(21)
> +#define AST2600_I3CG_REG1_SCL_IN_SW_MODE_VAL	BIT(23)
> +#define AST2600_I3CG_REG1_SDA_SW_MODE_OE	BIT(24)
> +#define AST2600_I3CG_REG1_SDA_OUT_SW_MODE_VAL	BIT(25)
> +#define AST2600_I3CG_REG1_SDA_IN_SW_MODE_VAL	BIT(27)
> +#define AST2600_I3CG_REG1_SCL_IN_SW_MODE_EN	BIT(28)
> +#define AST2600_I3CG_REG1_SDA_IN_SW_MODE_EN	BIT(29)
> +#define AST2600_I3CG_REG1_SCL_OUT_SW_MODE_EN	BIT(30)
> +#define AST2600_I3CG_REG1_SDA_OUT_SW_MODE_EN	BIT(31)
>  
>  #define AST2600_DEFAULT_SDA_PULLUP_OHMS		2000
>  
>  #define DEV_ADDR_TABLE_IBI_PEC			BIT(11)
>  
> +#define IBI_QUEUE_STATUS		0x18
> +#define PRESENT_STATE			0x54
> +#define   CM_TFR_STS			GENMASK(13, 8)
> +#define     CM_TFR_STS_MASTER_SERV_IBI	0xe
> +#define   SDA_LINE_SIGNAL_LEVEL		BIT(1)
> +#define   SCL_LINE_SIGNAL_LEVEL		BIT(0)
> +
>  struct ast2600_i3c {
>  	struct dw_i3c_master dw;
>  	struct regmap *global_regs;
> @@ -117,9 +134,52 @@ static void ast2600_i3c_set_dat_ibi(struct dw_i3c_master *i3c,
>  	}
>  }
>  
> +static bool ast2600_i3c_fsm_exit_serv_ibi(struct dw_i3c_master *dw)
> +{
> +	u32 state;
> +
> +	/*
> +	 * Clear the IBI queue to enable the hardware to generate SCL and
> +	 * begin detecting the T-bit low to stop reading IBI data.
> +	 */
> +	readl(dw->regs + IBI_QUEUE_STATUS);
> +	state = FIELD_GET(CM_TFR_STS, readl(dw->regs + PRESENT_STATE));
> +	if (state == CM_TFR_STS_MASTER_SERV_IBI)
> +		return false;
> +
> +	return true;
> +}
> +
> +static void ast2600_i3c_gen_tbits_in(struct dw_i3c_master *dw)
> +{
> +	struct ast2600_i3c *i3c = to_ast2600_i3c(dw);
> +	bool is_idle;
> +	int ret;
> +
> +	regmap_write_bits(i3c->global_regs, AST2600_I3CG_REG1(i3c->global_idx),
> +			  AST2600_I3CG_REG1_SDA_IN_SW_MODE_VAL,
> +			  AST2600_I3CG_REG1_SDA_IN_SW_MODE_VAL);
> +	regmap_write_bits(i3c->global_regs, AST2600_I3CG_REG1(i3c->global_idx),
> +			  AST2600_I3CG_REG1_SDA_IN_SW_MODE_EN,
> +			  AST2600_I3CG_REG1_SDA_IN_SW_MODE_EN);
> +
> +	regmap_write_bits(i3c->global_regs, AST2600_I3CG_REG1(i3c->global_idx),
> +			  AST2600_I3CG_REG1_SDA_IN_SW_MODE_VAL, 0);
> +	ret = readx_poll_timeout_atomic(ast2600_i3c_fsm_exit_serv_ibi, dw,
> +					is_idle, is_idle, 0, 2000000);
> +	regmap_write_bits(i3c->global_regs, AST2600_I3CG_REG1(i3c->global_idx),
> +			  AST2600_I3CG_REG1_SDA_IN_SW_MODE_EN, 0);
> +	if (ret)
> +		dev_err(&dw->base.dev,
> +			"Failed to exit the I3C fsm from %lx(MASTER_SERV_IBI): %d",
> +			FIELD_GET(CM_TFR_STS, readl(dw->regs + PRESENT_STATE)),
> +			ret);
> +}
> +
>  static const struct dw_i3c_platform_ops ast2600_i3c_ops = {
>  	.init = ast2600_i3c_init,
>  	.set_dat_ibi = ast2600_i3c_set_dat_ibi,
> +	.gen_tbits_in = ast2600_i3c_gen_tbits_in,
>  };
>  
>  static int ast2600_i3c_probe(struct platform_device *pdev)
> diff --git a/drivers/i3c/master/dw-i3c-master.c b/drivers/i3c/master/dw-i3c-master.c
> index 611c22b72c15..380e6a29c7b8 100644
> --- a/drivers/i3c/master/dw-i3c-master.c
> +++ b/drivers/i3c/master/dw-i3c-master.c
> @@ -158,6 +158,14 @@
>  #define DATA_BUFFER_STATUS_LEVEL_TX(x)	((x) & GENMASK(7, 0))
>  
>  #define PRESENT_STATE			0x54
> +#define   CM_TFR_ST_STS			GENMASK(21, 16)
> +#define     CM_TFR_ST_STS_HALT		0x13
> +#define   CM_TFR_STS			GENMASK(13, 8)
> +#define     CM_TFR_STS_MASTER_SERV_IBI	0xe
> +#define     CM_TFR_STS_MASTER_HALT	0xf
> +#define     CM_TFR_STS_SLAVE_HALT	0x6
> +#define   SDA_LINE_SIGNAL_LEVEL		BIT(1)
> +#define   SCL_LINE_SIGNAL_LEVEL		BIT(0)
>  #define CCC_DEVICE_STATUS		0x58
>  #define DEVICE_ADDR_TABLE_POINTER	0x5c
>  #define DEVICE_ADDR_TABLE_DEPTH(x)	(((x) & GENMASK(31, 16)) >> 16)
> @@ -1393,6 +1401,8 @@ static void dw_i3c_master_handle_ibi_sir(struct dw_i3c_master *master,
>  	unsigned long flags;
>  	u8 addr, len;
>  	int idx;
> +	bool terminate_ibi = false;
> +	u32 state;
>  
>  	addr = IBI_QUEUE_IBI_ADDR(status);
>  	len = IBI_QUEUE_STATUS_DATA_LEN(status);
> @@ -1435,6 +1445,7 @@ static void dw_i3c_master_handle_ibi_sir(struct dw_i3c_master *master,
>  		dev_dbg_ratelimited(&master->base.dev,
>  				    "IBI payload len %d greater than max %d\n",
>  				    len, dev->ibi->max_payload_len);
> +		terminate_ibi = true;
>  		goto err_drain;
>  	}
>  
> @@ -1450,6 +1461,9 @@ static void dw_i3c_master_handle_ibi_sir(struct dw_i3c_master *master,
>  
>  err_drain:
>  	dw_i3c_master_drain_ibi_queue(master, len);
> +	state = FIELD_GET(CM_TFR_STS, readl(master->regs + PRESENT_STATE));
> +	if (terminate_ibi && state == CM_TFR_STS_MASTER_SERV_IBI)
> +		master->platform_ops->gen_tbits_in(master);
>  
>  	spin_unlock_irqrestore(&master->devs_lock, flags);
>  }

Would it makes more sense to move `->gen_tbits_in(master);` logic to 
`if (dev->ibi->max_payload_len < len) ` above instead? 

The IBI queue would be properly flush at the end of function and don't need 
additional `terminate_ibi ` variable. If there were IBI storm, there might be
IBI coming in right after `dw_i3c_master_drain_ibi_queue` here which will
trigger another round of interrupt anyway.

Thanks
Joshua

> diff --git a/drivers/i3c/master/dw-i3c-master.h b/drivers/i3c/master/dw-i3c-master.h
> index c5cb695c16ab..1da485e42e74 100644
> --- a/drivers/i3c/master/dw-i3c-master.h
> +++ b/drivers/i3c/master/dw-i3c-master.h
> @@ -89,6 +89,15 @@ struct dw_i3c_platform_ops {
>  	 */
>  	void (*set_dat_ibi)(struct dw_i3c_master *i3c,
>  			    struct i3c_dev_desc *dev, bool enable, u32 *reg);
> +	/*
> +	 * Gerenating the fake t-bit (SDA low) to stop the IBI storm when the received
> +	 * data length of IBI is larger than the maximum IBI payload.
> +	 *
> +	 * When an IBI is received and SDA remains high after the address phase, the i3c
> +	 * controller may enter an infinite loop while trying to read data until the t-bit
> +	 * appears
> +	 */
> +	void (*gen_tbits_in)(struct dw_i3c_master *i3c);
>  };
>  
>  extern int dw_i3c_common_probe(struct dw_i3c_master *master,

-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] i3c: ast2600: Generate T-bits to terminate IBI storm
  2025-06-11 17:05 ` Frank Li
@ 2025-06-18  5:02   ` Billy Tsai
  0 siblings, 0 replies; 5+ messages in thread
From: Billy Tsai @ 2025-06-18  5:02 UTC (permalink / raw)
  To: Frank Li
  Cc: jk@codeconstruct.com.au, alexandre.belloni@bootlin.com,
	aniketmaurya@google.com, jarkko.nikula@linux.intel.com,
	Shyam-sundar.S-k@amd.com, wsa+renesas@sang-engineering.com,
	linux-i3c@lists.infradead.org, linux-kernel@vger.kernel.org,
	BMC-SW, elbadrym@google.com, romlem@google.com

On Wed, Jun 11, 2025 at 12:02:03PM +0800, Billy Tsai wrote:
> > Under certain conditions, such as when an IBI interrupt is received and
> > SDA remains high after the address phase,

> Can you descript more clear?
> 
> Generally IBI happen at below two case
> 1. SDA pull down by target
> 2. Address arbitration happen
> 
> Then Host send out ACK,  according to your descrpition, look like host
> NACK target IBI request.

No, it means that the Host acknowledges the IBI request. If the target device
then encounters any issue—for example, if the device shuts down at that
moment—the SDA will remain high during the data transfer phase, and the Host
will miss the T-bit to terminate the transfer.

> > the I3C controller will enter
> > an infinite loop attempting to read data until a T-bit is detected.

> BCR/DCR have indicate IBI mandatory data length. You should know how many
> data need be read according to IBI won's target address. Why relay on T-bit,
> which just is used for when target have less data than what expected.

IBI relies on the T-bit to terminate the data transfer, and the BCR is only
used to indicate whether the IBI includes data or not; it does not specify the
IBI data length. The IBI data length is controlled via the SETMRL/GETMRL
commands. The specification does not state that the Host must use the IBI data
length to terminate the transfer. However, this behavior can lead to an
unrecoverable condition in our hardware. Therefore, this patch is necessary to
recover the hardware from such scenarios.


> > This commit addresses the issue by generating a fake T-bit to terminate
> > the IBI storm when the received IBI data length exceeds the maximum
> > allowable IBI payload.

> Add empty line here.

> > This issue cannot be resolved using the abort function, as it is
> > ineffective when the I3C FSM is in the Servicing IBI Transfer (0xE) or
> > Clock Extension (0x12) states.

> why ineffective?

The abort function is not effective when the I3C FSM is in either of these
two states; this is a hardware limitation.

Thanks

Billy Tsai
-- 
linux-i3c mailing list
linux-i3c@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-i3c

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2025-06-18  5:03 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-11  4:02 [PATCH] i3c: ast2600: Generate T-bits to terminate IBI storm Billy Tsai
2025-06-11 17:05 ` Frank Li
2025-06-18  5:02   ` Billy Tsai
2025-06-11 19:44 ` kernel test robot
2025-06-16  2:31 ` Joshua Yeong

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox