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

[PATCH v2 1/4] 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.

This change gets rid of "unexpected INTR_STATUS" 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>
---
v2: Simplified the last sentence according to Frank Li's suggestion.
---
 drivers/i3c/master/mipi-i3c-hci/core.c | 5 +++--
 1 file changed, 3 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..e139d7e4d252 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -699,9 +699,10 @@ 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);
+	/* Allow signal updates relevant to IP versions 0.8 and beyond */
+	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 v2 2/4] 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.

Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
 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 e139d7e4d252..e5593b6e897e 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 v2 3/4] 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().

Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
 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 e5593b6e897e..84c372740020 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 */
@@ -597,9 +597,10 @@ static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id)
 		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 v2 4/4] 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 84c372740020..5c173249c0ac 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -607,12 +607,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 v2 1/4] i3c: mipi-i3c-hci: Allow only relevant INTR_STATUS bit updates

[Frank Li]

On Wed, Mar 12, 2025 at 01:10:46PM +0200, 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.
>
> This change gets rid of "unexpected INTR_STATUS" on old v0.5 IP version
> and is a no-op for later versions starting from v0.8.

submitting-patches.rst don't perfer words: "this change" or "this patch".

Get rid of "unexpected INTR_STATUS" on old v0.5 IP version ...

And below nit

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

>
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
> ---
> v2: Simplified the last sentence according to Frank Li's suggestion.
> ---
>  drivers/i3c/master/mipi-i3c-hci/core.c | 5 +++--
>  1 file changed, 3 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..e139d7e4d252 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> @@ -699,9 +699,10 @@ 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);
> +	/* Allow signal updates relevant to IP versions 0.8 and beyond */

Nit:

Only allow bit 31:10 signal update because
Bit 0:9 is not neededed for DMA transfer when version >= 0.8
Bit 0:9 is not handled by pio driver when version < 0.8.

Frank
> +	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 v2 3/4] i3c: mipi-i3c-hci: Change name of INTR_STATUS bit 11

[Frank Li]

On Wed, Mar 12, 2025 at 01:10:48PM +0200, Jarkko Nikula wrote:
> 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>
> ---
>  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 e5593b6e897e..84c372740020 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 */
> @@ -597,9 +597,10 @@ static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id)
>  		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

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

Re: [PATCH v2 2/4] i3c: mipi-i3c-hci: Fix handling status of i3c_hci_irq_handler()

[Frank Li]

On Wed, Mar 12, 2025 at 01:10:47PM +0200, Jarkko Nikula wrote:
> 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>
> ---
>  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 e139d7e4d252..e5593b6e897e 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);

I suggest move 'reg_write(INTR_STATUS, val);' out of if block later.

Frank

> +		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

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