[PATCH 0/2] i3c: mipi-i3c-hci: DMA interrupt fixes

[PATCH 0/2] i3c: mipi-i3c-hci: DMA interrupt fixes

[Jarkko Nikula]

Hi

These are targeted for the v6.13 cycle, i.e for the i3c/next branch
after v6.12-rc1 is tagged.

Patch 1/2 is stable looking material but I think it gets visible only
after patch 2/2 on a MIPI I3C HCI v1.0+ compatible HW so in my opinnion
should go through normal development cycle.

AMD folks: You said you have non-functional DMA mode on your HW. Can the
patch 2/2 make it functional or are there some other than interrupt
issues?

Jarkko Nikula (2):
  i3c: mipi-i3c-hci: Mask ring interrupts before ring stop request
  i3c: mipi-i3c-hci: Handle interrupts according to current
    specifications

 drivers/i3c/master/mipi-i3c-hci/core.c | 16 +++-------------
 drivers/i3c/master/mipi-i3c-hci/dma.c  | 10 +++-------
 drivers/i3c/master/mipi-i3c-hci/hci.h  |  2 +-
 drivers/i3c/master/mipi-i3c-hci/pio.c  |  2 +-
 4 files changed, 8 insertions(+), 22 deletions(-)

-- 
2.45.2


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

[PATCH 1/2] i3c: mipi-i3c-hci: Mask ring interrupts before ring stop request

[Jarkko Nikula]

Bus cleanup path in DMA mode may trigger a RING_OP_STAT interrupt when
the ring is being stopped. Depending on timing between ring stop request
completion, interrupt handler removal and code execution this may lead
to a NULL pointer dereference in hci_dma_irq_handler() if it gets to run
after the io_data pointer is set to NULL in hci_dma_cleanup().

Prevent this my masking the ring interrupts before ring stop request.

Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
 drivers/i3c/master/mipi-i3c-hci/dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
index a918e96b21fd..13adc5840094 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dma.c
+++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
@@ -159,10 +159,10 @@ static void hci_dma_cleanup(struct i3c_hci *hci)
 	for (i = 0; i < rings->total; i++) {
 		rh = &rings->headers[i];
 
+		rh_reg_write(INTR_SIGNAL_ENABLE, 0);
 		rh_reg_write(RING_CONTROL, 0);
 		rh_reg_write(CR_SETUP, 0);
 		rh_reg_write(IBI_SETUP, 0);
-		rh_reg_write(INTR_SIGNAL_ENABLE, 0);
 
 		if (rh->xfer)
 			dma_free_coherent(&hci->master.dev,
-- 
2.45.2


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

[PATCH 2/2] i3c: mipi-i3c-hci: Handle interrupts according to current specifications

[Jarkko Nikula]

Current MIPI I3C HCI specification versions pre-1.0, 1.0. 1.1 and 1.2
don't have cascaded interrupt bits for the PIO and DMA (ring headers) in
the INTR_STATUS register as implemented currently in the code. Instead
bits 9:0 are marked as reserved with unspecified reset value.

To my understanding they were planned to be introduced in the version 2
and the original commit 9ad9a52cce28 ("i3c/master: introduce the
mipi-i3c-hci driver") was coding ahead according to a draft. With
remarks though.

This is causing that the DMA handler is not called until at least one
reserved bit 7:0 is set in the INTR_STATUS.

Since it looks that idea was dropped in later official versions and to
make able to handle DMA interrupts on an HW that is implemented
according to current specifications call assigned PIO or DMA IO handler
unconditionally.

While doing so remove cascaded interrupt bit definitions and the mask
argument passed to the handler functions.

Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
 drivers/i3c/master/mipi-i3c-hci/core.c | 16 +++-------------
 drivers/i3c/master/mipi-i3c-hci/dma.c  |  8 ++------
 drivers/i3c/master/mipi-i3c-hci/hci.h  |  2 +-
 drivers/i3c/master/mipi-i3c-hci/pio.c  |  2 +-
 4 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index a82c47c9986d..b190401c89c9 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -80,8 +80,6 @@
 #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_INTERNAL_ERR		BIT(10)	/* HC Internal Error */
-#define INTR_HC_PIO			BIT(8)	/* cascaded PIO interrupt */
-#define INTR_HC_RINGS			GENMASK(7, 0)
 
 #define DAT_SECTION			0x30	/* Device Address Table */
 #define DAT_ENTRY_SIZE			GENMASK(31, 28)
@@ -597,9 +595,6 @@ static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id)
 
 	if (val) {
 		reg_write(INTR_STATUS, val);
-	} else {
-		/* v1.0 does not have PIO cascaded notification bits */
-		val |= INTR_HC_PIO;
 	}
 
 	if (val & INTR_HC_RESET_CANCEL) {
@@ -610,14 +605,9 @@ static irqreturn_t i3c_hci_irq_handler(int irq, void *dev_id)
 		dev_err(&hci->master.dev, "Host Controller Internal Error\n");
 		val &= ~INTR_HC_INTERNAL_ERR;
 	}
-	if (val & INTR_HC_PIO) {
-		hci->io->irq_handler(hci, 0);
-		val &= ~INTR_HC_PIO;
-	}
-	if (val & INTR_HC_RINGS) {
-		hci->io->irq_handler(hci, val & INTR_HC_RINGS);
-		val &= ~INTR_HC_RINGS;
-	}
+
+	hci->io->irq_handler(hci);
+
 	if (val)
 		dev_err(&hci->master.dev, "unexpected INTR_STATUS %#x\n", val);
 	else
diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
index 13adc5840094..e8e56a8d2057 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dma.c
+++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
@@ -733,20 +733,16 @@ static void hci_dma_process_ibi(struct i3c_hci *hci, struct hci_rh_data *rh)
 	rh_reg_write(CHUNK_CONTROL, rh_reg_read(CHUNK_CONTROL) + ibi_chunks);
 }
 
-static bool hci_dma_irq_handler(struct i3c_hci *hci, unsigned int mask)
+static bool hci_dma_irq_handler(struct i3c_hci *hci)
 {
 	struct hci_rings_data *rings = hci->io_data;
 	unsigned int i;
 	bool handled = false;
 
-	for (i = 0; mask && i < rings->total; i++) {
+	for (i = 0; i < rings->total; i++) {
 		struct hci_rh_data *rh;
 		u32 status;
 
-		if (!(mask & BIT(i)))
-			continue;
-		mask &= ~BIT(i);
-
 		rh = &rings->headers[i];
 		status = rh_reg_read(INTR_STATUS);
 		DBG("rh%d status: %#x", i, status);
diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
index aaa47ac47381..69ea1d10414b 100644
--- a/drivers/i3c/master/mipi-i3c-hci/hci.h
+++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
@@ -115,7 +115,7 @@ static inline void hci_free_xfer(struct hci_xfer *xfer, unsigned int n)
 
 /* This abstracts PIO vs DMA operations */
 struct hci_io_ops {
-	bool (*irq_handler)(struct i3c_hci *hci, unsigned int mask);
+	bool (*irq_handler)(struct i3c_hci *hci);
 	int (*queue_xfer)(struct i3c_hci *hci, struct hci_xfer *xfer, int n);
 	bool (*dequeue_xfer)(struct i3c_hci *hci, struct hci_xfer *xfer, int n);
 	int (*request_ibi)(struct i3c_hci *hci, struct i3c_dev_desc *dev,
diff --git a/drivers/i3c/master/mipi-i3c-hci/pio.c b/drivers/i3c/master/mipi-i3c-hci/pio.c
index d0272aa93599..2fc71e696911 100644
--- a/drivers/i3c/master/mipi-i3c-hci/pio.c
+++ b/drivers/i3c/master/mipi-i3c-hci/pio.c
@@ -979,7 +979,7 @@ static void hci_pio_recycle_ibi_slot(struct i3c_hci *hci,
 	i3c_generic_ibi_recycle_slot(dev_ibi->pool, slot);
 }
 
-static bool hci_pio_irq_handler(struct i3c_hci *hci, unsigned int unused)
+static bool hci_pio_irq_handler(struct i3c_hci *hci)
 {
 	struct hci_pio_data *pio = hci->io_data;
 	u32 status;
-- 
2.45.2


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

Re: [PATCH 0/2] i3c: mipi-i3c-hci: DMA interrupt fixes

[Shyam Sundar S K]

Hi Jarkko,

On 9/20/2024 20:14, Jarkko Nikula wrote:
> Hi
> 
> These are targeted for the v6.13 cycle, i.e for the i3c/next branch
> after v6.12-rc1 is tagged.
> 
> Patch 1/2 is stable looking material but I think it gets visible only
> after patch 2/2 on a MIPI I3C HCI v1.0+ compatible HW so in my opinnion
> should go through normal development cycle.
> 
> AMD folks: You said you have non-functional DMA mode on your HW. Can the
> patch 2/2 make it functional or are there some other than interrupt
> issues?
> 

These are RTL issues within the hardware. I don't believe the patch
will help since the DMA mode never activates (at least with the
current ASIC).

Thanks,
Shyam

> Jarkko Nikula (2):
>   i3c: mipi-i3c-hci: Mask ring interrupts before ring stop request
>   i3c: mipi-i3c-hci: Handle interrupts according to current
>     specifications
> 
>  drivers/i3c/master/mipi-i3c-hci/core.c | 16 +++-------------
>  drivers/i3c/master/mipi-i3c-hci/dma.c  | 10 +++-------
>  drivers/i3c/master/mipi-i3c-hci/hci.h  |  2 +-
>  drivers/i3c/master/mipi-i3c-hci/pio.c  |  2 +-
>  4 files changed, 8 insertions(+), 22 deletions(-)
> 

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

Re: [PATCH 0/2] i3c: mipi-i3c-hci: DMA interrupt fixes

[Alexandre Belloni]

On Fri, 20 Sep 2024 17:44:30 +0300, Jarkko Nikula wrote:
> These are targeted for the v6.13 cycle, i.e for the i3c/next branch
> after v6.12-rc1 is tagged.
> 
> Patch 1/2 is stable looking material but I think it gets visible only
> after patch 2/2 on a MIPI I3C HCI v1.0+ compatible HW so in my opinnion
> should go through normal development cycle.
> 
> [...]

Applied, thanks!

[1/2] i3c: mipi-i3c-hci: Mask ring interrupts before ring stop request
      https://git.kernel.org/abelloni/c/6ca2738174e4
[2/2] i3c: mipi-i3c-hci: Handle interrupts according to current specifications
      https://git.kernel.org/abelloni/c/45357c9b37bb

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