[PATCH v3 1/5] i3c: mipi-i3c-hci: Allow only relevant INTR_STATUS bit updates

[PATCH v3 1/5] i3c: mipi-i3c-hci: Allow only relevant INTR_STATUS bit updates

[Jarkko Nikula]

Since MIPI I3C HCI specification version v0.8 INTR_STATUS bits 9:0 are
reserved. Version v0.5 has bits 9 and 5:0 in use but not handled by the
current driver code and not needed in DMA transfers.

PIO transfers with v0.5 would require changes to both
core.c: i3c_hci_irq_handler() and pio.c: hci_pio_irq_handler() though.

For these reasons don't enable signal updates from INTR_STATUS bits 9:0.

It allow to get rid of "unexpected INTR_STATUS" error messages on old
v0.5 IP version and is a no-op for later versions starting from v0.8.

Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
v3: Changed the last commit log sentence and clarified more the comment
about INTR_STATUS_ENABLE signal updates according to Frank Li's suggestion.
---
 drivers/i3c/master/mipi-i3c-hci/core.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index a71226d7ca59..ba7aa6bbcec5 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -699,9 +699,14 @@ static int i3c_hci_init(struct i3c_hci *hci)
 	if (ret)
 		return -ENXIO;
 
-	/* Disable all interrupts and allow all signal updates */
+	/* Disable all interrupts */
 	reg_write(INTR_SIGNAL_ENABLE, 0x0);
-	reg_write(INTR_STATUS_ENABLE, 0xffffffff);
+	/*
+	 * Only allow bit 31:10 signal updates because
+	 * Bit 0:9 are reserved in IP version >= 0.8
+	 * Bit 0:5 are defined in IP version < 0.8 but not handled by PIO code
+	 */
+	reg_write(INTR_STATUS_ENABLE, GENMASK(31, 10));
 
 	/* Make sure our data ordering fits the host's */
 	regval = reg_read(HC_CONTROL);
-- 
2.47.2


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

[PATCH v3 2/5] i3c: mipi-i3c-hci: Fix handling status of i3c_hci_irq_handler()

[Jarkko Nikula]

Return IRQ_HANDLED from the i3c_hci_irq_handler() only if some
INTR_STATUS bit was set or if DMA/PIO handler handled it.

Currently it returns IRQ_HANDLED in case INTR_STATUS is zero and IO
handler returns false. Which could be the case if interrupt comes from
other device or is spurious.

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
v3: Added Reviewed-by
---
 drivers/i3c/master/mipi-i3c-hci/core.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index ba7aa6bbcec5..780e9db7e21e 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -594,6 +594,7 @@ static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id)
 
 	if (val) {
 		reg_write(INTR_STATUS, val);
+		result = IRQ_HANDLED;
 	}
 
 	if (val & INTR_HC_RESET_CANCEL) {
@@ -605,12 +606,11 @@ static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id)
 		val &= ~INTR_HC_INTERNAL_ERR;
 	}
 
-	hci->io->irq_handler(hci);
+	if (hci->io->irq_handler(hci))
+		result = IRQ_HANDLED;
 
 	if (val)
 		dev_err(&hci->master.dev, "unexpected INTR_STATUS %#x\n", val);
-	else
-		result = IRQ_HANDLED;
 
 	return result;
 }
-- 
2.47.2


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

[PATCH v3 3/5] i3c: mipi-i3c-hci: Clear INTR_STATUS unconditionally

[Jarkko Nikula]

Status fields in INTR_STATUS register are write 1 to clear so do it
unconditionally and move clearing of them out of an if block.

Suggested-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
 drivers/i3c/master/mipi-i3c-hci/core.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index 780e9db7e21e..4c4100d2d9af 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -590,12 +590,11 @@ static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id)
 	u32 val;
 
 	val = reg_read(INTR_STATUS);
+	reg_write(INTR_STATUS, val);
 	DBG("INTR_STATUS = %#x", val);
 
-	if (val) {
-		reg_write(INTR_STATUS, val);
+	if (val)
 		result = IRQ_HANDLED;
-	}
 
 	if (val & INTR_HC_RESET_CANCEL) {
 		DBG("cancelled reset");
-- 
2.47.2


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

[PATCH v3 4/5] i3c: mipi-i3c-hci: Change name of INTR_STATUS bit 11

[Jarkko Nikula]

INTR_STATUS bit 11 INTR_HC_RESET_CANCEL was probably projected for the
MIPI I3C HCI specification version 2 but was not ever implemented.

This bit is first time specified in the v1.2 as HC_SEQ_CANCEL_STAT
"Host Controller Cancelled Transaction Sequence". Update the definition
and debug print of it accordingly.

While at it, change DBG() print to dev_dbg().

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
v3: Added Reviewed-by
---
 drivers/i3c/master/mipi-i3c-hci/core.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index 4c4100d2d9af..0bb74173ca94 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -78,7 +78,7 @@
 #define INTR_SIGNAL_ENABLE		0x28
 #define INTR_FORCE			0x2c
 #define INTR_HC_CMD_SEQ_UFLOW_STAT	BIT(12)	/* Cmd Sequence Underflow */
-#define INTR_HC_RESET_CANCEL		BIT(11)	/* HC Cancelled Reset */
+#define INTR_HC_SEQ_CANCEL		BIT(11)	/* HC Cancelled Transaction Sequence */
 #define INTR_HC_INTERNAL_ERR		BIT(10)	/* HC Internal Error */
 
 #define DAT_SECTION			0x30	/* Device Address Table */
@@ -596,9 +596,10 @@ static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id)
 	if (val)
 		result = IRQ_HANDLED;
 
-	if (val & INTR_HC_RESET_CANCEL) {
-		DBG("cancelled reset");
-		val &= ~INTR_HC_RESET_CANCEL;
+	if (val & INTR_HC_SEQ_CANCEL) {
+		dev_dbg(&hci->master.dev,
+			"Host Controller Cancelled Transaction Sequence\n");
+		val &= ~INTR_HC_SEQ_CANCEL;
 	}
 	if (val & INTR_HC_INTERNAL_ERR) {
 		dev_err(&hci->master.dev, "Host Controller Internal Error\n");
-- 
2.47.2


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

[PATCH v3 5/5] i3c: mipi-i3c-hci: Move unexpected INTR_STATUS print before IO handler

[Jarkko Nikula]

Move "unexpected INTR_STATUS" error print before calling the IO handler
as it is more consistent that way. Otherwise it may be confusing if
generic interrupt related prints are mixed with IO handler prints.

Since this error print is more indication of missing code rather than
runtime error downgrade it to dev_warn_once().

Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
v2: Added Reviewed-by
---
 drivers/i3c/master/mipi-i3c-hci/core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index 0bb74173ca94..bc4538694540 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -606,12 +606,13 @@ static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id)
 		val &= ~INTR_HC_INTERNAL_ERR;
 	}
 
+	if (val)
+		dev_warn_once(&hci->master.dev,
+			      "unexpected INTR_STATUS %#x\n", val);
+
 	if (hci->io->irq_handler(hci))
 		result = IRQ_HANDLED;
 
-	if (val)
-		dev_err(&hci->master.dev, "unexpected INTR_STATUS %#x\n", val);
-
 	return result;
 }
 
-- 
2.47.2


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

Re: [PATCH v3 1/5] i3c: mipi-i3c-hci: Allow only relevant INTR_STATUS bit updates

[Jarkko Nikula]

Hi Frank

On 4/9/25 5:03 PM, Jarkko Nikula wrote:
> Since MIPI I3C HCI specification version v0.8 INTR_STATUS bits 9:0 are
> reserved. Version v0.5 has bits 9 and 5:0 in use but not handled by the
> current driver code and not needed in DMA transfers.
> 
> PIO transfers with v0.5 would require changes to both
> core.c: i3c_hci_irq_handler() and pio.c: hci_pio_irq_handler() though.
> 
> For these reasons don't enable signal updates from INTR_STATUS bits 9:0.
> 
> It allow to get rid of "unexpected INTR_STATUS" error messages on old
> v0.5 IP version and is a no-op for later versions starting from v0.8.
> 
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> ---
> v3: Changed the last commit log sentence and clarified more the comment
> about INTR_STATUS_ENABLE signal updates according to Frank Li's suggestion.
> ---
>   drivers/i3c/master/mipi-i3c-hci/core.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
Was this and patch 3/5 now ok in your point of view?

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

Re: [PATCH v3 1/5] i3c: mipi-i3c-hci: Allow only relevant INTR_STATUS bit updates

[Frank Li]

On Wed, Apr 09, 2025 at 05:03:57PM +0300, Jarkko Nikula wrote:
> Since MIPI I3C HCI specification version v0.8 INTR_STATUS bits 9:0 are
> reserved. Version v0.5 has bits 9 and 5:0 in use but not handled by the
> current driver code and not needed in DMA transfers.
>
> PIO transfers with v0.5 would require changes to both
> core.c: i3c_hci_irq_handler() and pio.c: hci_pio_irq_handler() though.
>
> For these reasons don't enable signal updates from INTR_STATUS bits 9:0.
>
> It allow to get rid of "unexpected INTR_STATUS" error messages on old
> v0.5 IP version and is a no-op for later versions starting from v0.8.
>
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
> v3: Changed the last commit log sentence and clarified more the comment
> about INTR_STATUS_ENABLE signal updates according to Frank Li's suggestion.
> ---
>  drivers/i3c/master/mipi-i3c-hci/core.c | 9 +++++++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> index a71226d7ca59..ba7aa6bbcec5 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> @@ -699,9 +699,14 @@ static int i3c_hci_init(struct i3c_hci *hci)
>  	if (ret)
>  		return -ENXIO;
>
> -	/* Disable all interrupts and allow all signal updates */
> +	/* Disable all interrupts */
>  	reg_write(INTR_SIGNAL_ENABLE, 0x0);
> -	reg_write(INTR_STATUS_ENABLE, 0xffffffff);
> +	/*
> +	 * Only allow bit 31:10 signal updates because
> +	 * Bit 0:9 are reserved in IP version >= 0.8
> +	 * Bit 0:5 are defined in IP version < 0.8 but not handled by PIO code
> +	 */
> +	reg_write(INTR_STATUS_ENABLE, GENMASK(31, 10));
>
>  	/* Make sure our data ordering fits the host's */
>  	regval = reg_read(HC_CONTROL);
> --
> 2.47.2
>
>
> --
> linux-i3c mailing list
> linux-i3c@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c

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

Re: [PATCH v3 3/5] i3c: mipi-i3c-hci: Clear INTR_STATUS unconditionally

[Frank Li]

On Wed, Apr 09, 2025 at 05:03:59PM +0300, Jarkko Nikula wrote:
> Status fields in INTR_STATUS register are write 1 to clear so do it
> unconditionally and move clearing of them out of an if block.
>
> Suggested-by: Frank Li <Frank.Li@nxp.com>
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> ---
>  drivers/i3c/master/mipi-i3c-hci/core.c | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> index 780e9db7e21e..4c4100d2d9af 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> @@ -590,12 +590,11 @@ static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id)
>  	u32 val;
>
>  	val = reg_read(INTR_STATUS);
> +	reg_write(INTR_STATUS, val);
>  	DBG("INTR_STATUS = %#x", val);
>
> -	if (val) {
> -		reg_write(INTR_STATUS, val);
> +	if (val)
>  		result = IRQ_HANDLED;
> -	}
>
>  	if (val & INTR_HC_RESET_CANCEL) {
>  		DBG("cancelled reset");
> --
> 2.47.2
>
>
> --
> linux-i3c mailing list
> linux-i3c@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-i3c

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

Re: [PATCH v3 1/5] i3c: mipi-i3c-hci: Allow only relevant INTR_STATUS bit updates

[Alexandre Belloni]

On Wed, 09 Apr 2025 17:03:57 +0300, Jarkko Nikula wrote:
> Since MIPI I3C HCI specification version v0.8 INTR_STATUS bits 9:0 are
> reserved. Version v0.5 has bits 9 and 5:0 in use but not handled by the
> current driver code and not needed in DMA transfers.
> 
> PIO transfers with v0.5 would require changes to both
> core.c: i3c_hci_irq_handler() and pio.c: hci_pio_irq_handler() though.
> 
> [...]

Applied, thanks!

[1/5] i3c: mipi-i3c-hci: Allow only relevant INTR_STATUS bit updates
      https://git.kernel.org/abelloni/c/eeeec6c5475e
[2/5] i3c: mipi-i3c-hci: Fix handling status of i3c_hci_irq_handler()
      https://git.kernel.org/abelloni/c/279c24021b83
[3/5] i3c: mipi-i3c-hci: Clear INTR_STATUS unconditionally
      https://git.kernel.org/abelloni/c/a7035a8ee966
[4/5] i3c: mipi-i3c-hci: Change name of INTR_STATUS bit 11
      https://git.kernel.org/abelloni/c/7479d2675c50
[5/5] i3c: mipi-i3c-hci: Move unexpected INTR_STATUS print before IO handler
      https://git.kernel.org/abelloni/c/bd916806632d

Best regards,

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

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