[PATCH v4 0/4] mipi-i3c-hci: Make able to work with IOMMU enabled

[PATCH v4 0/4] mipi-i3c-hci: Make able to work with IOMMU enabled

[Jarkko Nikula]

Hi

Here's the 4th version of common I3C core DMA mapping and bounce buffering
handling and making the MIPI I3C HCI able to work with IOMMU enabled.

Changes to the v3:
- patch 1/4: Renamed variable "need_bounce" to "force_bounce" and document
  both true and false argument values.
- patch 3/4: Added Reviewed-by tag.

Changes to the v2:
- patch 1/4: Use explicit kfree() calls in i3c_master_dma_unmap_single().
  Empty line added to the i3c_master_dma_map_single().
- patch 2/4: Call explicitly i3c_master_dma_unmap_single() in
  hci_dma_unmap_xfer().
- patch 4/4: Added Reviewed-by tag.

Changes to the v1:
- DMA mapping length is also cache_line_size() aligned in order to avoid
  possible double bouncing from the SWIOTLB.
- Using DEFINE_FREE and __free infra for cleanup and changes to the other
  patches accordingly.
- Added more description to the patch 1/4 commit log and slight corrections
  to others.


Jarkko Nikula (4):
  i3c: master: Add helpers for DMA mapping and bounce buffer handling
  i3c: mipi-i3c-hci: Use core helpers for DMA mapping and bounce
    buffering
  i3c: mipi-i3c-hci: Use physical device pointer with DMA API
  i3c: mipi-i3c-hci: Use own DMA bounce buffer management for I2C
    transfers

 drivers/i3c/master.c                   | 74 ++++++++++++++++++++++++++
 drivers/i3c/master/mipi-i3c-hci/core.c | 40 +-------------
 drivers/i3c/master/mipi-i3c-hci/dma.c  | 69 ++++++++++++++----------
 drivers/i3c/master/mipi-i3c-hci/hci.h  |  3 +-
 include/linux/i3c/master.h             | 26 +++++++++
 5 files changed, 144 insertions(+), 68 deletions(-)

-- 
2.47.2


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

[PATCH v4 1/4] i3c: master: Add helpers for DMA mapping and bounce buffer handling

[Jarkko Nikula]

Some I3C controllers such as MIPI I3C HCI may pad the last DWORD (32-bit)
with stale data from the RX FIFO in DMA transfers if the receive length
is not DWORD aligned and when the device DMA is IOMMU mapped.

In such a case, a properly sized bounce buffer is required in order to
avoid possible data corruption. In a review discussion, proposal was to
have a common helpers in I3C core for DMA mapping and bounce buffer
handling.

Drivers may use the helper i3c_master_dma_map_single() to map a buffer
for a DMA transfer. It internally allocates a bounce buffer if buffer is
not DMA'able or when the driver requires it for a transfer.

Helper i3c_master_dma_unmap_single() does the needed cleanups and
data copying from the bounce buffer.

Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
 drivers/i3c/master.c       | 74 ++++++++++++++++++++++++++++++++++++++
 include/linux/i3c/master.h | 26 ++++++++++++++
 2 files changed, 100 insertions(+)

diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
index 2ef898a8fd80..033d06cabca2 100644
--- a/drivers/i3c/master.c
+++ b/drivers/i3c/master.c
@@ -8,6 +8,7 @@
 #include <linux/atomic.h>
 #include <linux/bug.h>
 #include <linux/device.h>
+#include <linux/dma-mapping.h>
 #include <linux/err.h>
 #include <linux/export.h>
 #include <linux/kernel.h>
@@ -1727,6 +1728,79 @@ int i3c_master_do_daa(struct i3c_master_controller *master)
 }
 EXPORT_SYMBOL_GPL(i3c_master_do_daa);
 
+/**
+ * i3c_master_dma_map_single() - Map buffer for single DMA transfer
+ * @dev: device object of a device doing DMA
+ * @buf: destination/source buffer for DMA
+ * @len: length of transfer
+ * @force_bounce: true, force to use a bounce buffer,
+ *                false, function will auto check is a bounce buffer required
+ * @dir: DMA direction
+ *
+ * Map buffer for a DMA transfer and allocate a bounce buffer if required.
+ *
+ * Return: I3C DMA transfer descriptor or NULL in case of error.
+ */
+struct i3c_dma *i3c_master_dma_map_single(struct device *dev, void *buf,
+	size_t len, bool force_bounce, enum dma_data_direction dir)
+{
+	struct i3c_dma *dma_xfer __free(kfree) = NULL;
+	void *bounce __free(kfree) = NULL;
+	void *dma_buf = buf;
+
+	dma_xfer = kzalloc(sizeof(*dma_xfer), GFP_KERNEL);
+	if (!dma_xfer)
+		return NULL;
+
+	dma_xfer->dev = dev;
+	dma_xfer->buf = buf;
+	dma_xfer->dir = dir;
+	dma_xfer->len = len;
+	dma_xfer->map_len = len;
+
+	if (is_vmalloc_addr(buf))
+		force_bounce = true;
+
+	if (force_bounce) {
+		dma_xfer->map_len = ALIGN(len, cache_line_size());
+		if (dir == DMA_FROM_DEVICE)
+			bounce = kzalloc(dma_xfer->map_len, GFP_KERNEL);
+		else
+			bounce = kmemdup(buf, dma_xfer->map_len, GFP_KERNEL);
+		if (!bounce)
+			return NULL;
+		dma_buf = bounce;
+	}
+
+	dma_xfer->addr = dma_map_single(dev, dma_buf, dma_xfer->map_len, dir);
+	if (dma_mapping_error(dev, dma_xfer->addr))
+		return NULL;
+
+	dma_xfer->bounce_buf = no_free_ptr(bounce);
+	return no_free_ptr(dma_xfer);
+}
+EXPORT_SYMBOL_GPL(i3c_master_dma_map_single);
+
+/**
+ * i3c_master_dma_unmap_single() - Unmap buffer after DMA
+ * @dma_xfer: DMA transfer and mapping descriptor
+ *
+ * Unmap buffer and cleanup DMA transfer descriptor.
+ */
+void i3c_master_dma_unmap_single(struct i3c_dma *dma_xfer)
+{
+	dma_unmap_single(dma_xfer->dev, dma_xfer->addr,
+			 dma_xfer->map_len, dma_xfer->dir);
+	if (dma_xfer->bounce_buf) {
+		if (dma_xfer->dir == DMA_FROM_DEVICE)
+			memcpy(dma_xfer->buf, dma_xfer->bounce_buf,
+			       dma_xfer->len);
+		kfree(dma_xfer->bounce_buf);
+	}
+	kfree(dma_xfer);
+}
+EXPORT_SYMBOL_GPL(i3c_master_dma_unmap_single);
+
 /**
  * i3c_master_set_info() - set master device information
  * @master: master used to send frames on the bus
diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
index 043f5c7ff398..c52a82dd79a6 100644
--- a/include/linux/i3c/master.h
+++ b/include/linux/i3c/master.h
@@ -558,6 +558,26 @@ struct i3c_master_controller {
 #define i3c_bus_for_each_i3cdev(bus, dev)				\
 	list_for_each_entry(dev, &(bus)->devs.i3c, common.node)
 
+/**
+ * struct i3c_dma - DMA transfer and mapping descriptor
+ * @dev: device object of a device doing DMA
+ * @buf: destination/source buffer for DMA
+ * @len: length of transfer
+ * @map_len: length of DMA mapping
+ * @addr: mapped DMA address for a Host Controller Driver
+ * @dir: DMA direction
+ * @bounce_buf: an allocated bounce buffer if transfer needs it or NULL
+ */
+struct i3c_dma {
+	struct device *dev;
+	void *buf;
+	size_t len;
+	size_t map_len;
+	dma_addr_t addr;
+	enum dma_data_direction dir;
+	void *bounce_buf;
+};
+
 int i3c_master_do_i2c_xfers(struct i3c_master_controller *master,
 			    const struct i2c_msg *xfers,
 			    int nxfers);
@@ -575,6 +595,12 @@ int i3c_master_get_free_addr(struct i3c_master_controller *master,
 int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
 				  u8 addr);
 int i3c_master_do_daa(struct i3c_master_controller *master);
+struct i3c_dma *i3c_master_dma_map_single(struct device *dev, void *ptr,
+					  size_t len, bool force_bounce,
+					  enum dma_data_direction dir);
+void i3c_master_dma_unmap_single(struct i3c_dma *dma_xfer);
+DEFINE_FREE(i3c_master_dma_unmap_single, void *,
+	    if (_T) i3c_master_dma_unmap_single(_T))
 
 int i3c_master_set_info(struct i3c_master_controller *master,
 			const struct i3c_device_info *info);
-- 
2.47.2


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

[PATCH v4 2/4] i3c: mipi-i3c-hci: Use core helpers for DMA mapping and bounce buffering

[Jarkko Nikula]

So far only I3C private and I2C transfers have required a bounce buffer
for DMA transfers when buffer is not DMA'able.

It was observed that when the device DMA is IOMMU mapped and the receive
length is not a multiple of DWORDs (32-bit), the last DWORD is padded
with stale data from the RX FIFO, corrupting 1-3 bytes beyond the
expected data.

A similar issue, though less severe, occurs when an I3C target returns
less data than requested. In this case, the padding does not exceed the
requested number of bytes, assuming the device DMA is not IOMMU mapped.

Therefore, all I3C private transfer, CCC command payload and I2C
transfer receive buffers must be properly sized for the DMA being IOMMU
mapped. Even if those buffers are already DMA safe, their size may not
be DWORD aligned.

To prepare for the device DMA being IOMMU mapped and to address the
above issue, use helpers from I3C core for DMA mapping and bounce
buffering for all DMA transfers.

For now, require bounce buffer only when the buffer is in the
vmalloc() area to avoid unnecessary copying with CCC commands and
DMA-safe I2C transfers.

Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
 drivers/i3c/master/mipi-i3c-hci/core.c | 34 --------------------------
 drivers/i3c/master/mipi-i3c-hci/dma.c  | 27 +++++++++-----------
 drivers/i3c/master/mipi-i3c-hci/hci.h  |  3 +--
 3 files changed, 12 insertions(+), 52 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index 60f1175f1f37..b2977b6ac9f7 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -272,34 +272,6 @@ static int i3c_hci_daa(struct i3c_master_controller *m)
 	return hci->cmd->perform_daa(hci);
 }
 
-static int i3c_hci_alloc_safe_xfer_buf(struct i3c_hci *hci,
-				       struct hci_xfer *xfer)
-{
-	if (hci->io != &mipi_i3c_hci_dma ||
-	    xfer->data == NULL || !is_vmalloc_addr(xfer->data))
-		return 0;
-
-	if (xfer->rnw)
-		xfer->bounce_buf = kzalloc(xfer->data_len, GFP_KERNEL);
-	else
-		xfer->bounce_buf = kmemdup(xfer->data,
-					   xfer->data_len, GFP_KERNEL);
-
-	return xfer->bounce_buf == NULL ? -ENOMEM : 0;
-}
-
-static void i3c_hci_free_safe_xfer_buf(struct i3c_hci *hci,
-				       struct hci_xfer *xfer)
-{
-	if (hci->io != &mipi_i3c_hci_dma || xfer->bounce_buf == NULL)
-		return;
-
-	if (xfer->rnw)
-		memcpy(xfer->data, xfer->bounce_buf, xfer->data_len);
-
-	kfree(xfer->bounce_buf);
-}
-
 static int i3c_hci_priv_xfers(struct i3c_dev_desc *dev,
 			      struct i3c_priv_xfer *i3c_xfers,
 			      int nxfers)
@@ -333,9 +305,6 @@ static int i3c_hci_priv_xfers(struct i3c_dev_desc *dev,
 		}
 		hci->cmd->prep_i3c_xfer(hci, dev, &xfer[i]);
 		xfer[i].cmd_desc[0] |= CMD_0_ROC;
-		ret = i3c_hci_alloc_safe_xfer_buf(hci, &xfer[i]);
-		if (ret)
-			goto out;
 	}
 	last = i - 1;
 	xfer[last].cmd_desc[0] |= CMD_0_TOC;
@@ -359,9 +328,6 @@ static int i3c_hci_priv_xfers(struct i3c_dev_desc *dev,
 	}
 
 out:
-	for (i = 0; i < nxfers; i++)
-		i3c_hci_free_safe_xfer_buf(hci, &xfer[i]);
-
 	hci_free_xfer(xfer, nxfers);
 	return ret;
 }
diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
index 491dfe70b660..351851859f02 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dma.c
+++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
@@ -349,9 +349,7 @@ static void hci_dma_unmap_xfer(struct i3c_hci *hci,
 		xfer = xfer_list + i;
 		if (!xfer->data)
 			continue;
-		dma_unmap_single(&hci->master.dev,
-				 xfer->data_dma, xfer->data_len,
-				 xfer->rnw ? DMA_FROM_DEVICE : DMA_TO_DEVICE);
+		i3c_master_dma_unmap_single(xfer->dma);
 	}
 }
 
@@ -362,7 +360,6 @@ static int hci_dma_queue_xfer(struct i3c_hci *hci,
 	struct hci_rh_data *rh;
 	unsigned int i, ring, enqueue_ptr;
 	u32 op1_val, op2_val;
-	void *buf;
 
 	/* For now we only use ring 0 */
 	ring = 0;
@@ -373,6 +370,8 @@ static int hci_dma_queue_xfer(struct i3c_hci *hci,
 	for (i = 0; i < n; i++) {
 		struct hci_xfer *xfer = xfer_list + i;
 		u32 *ring_data = rh->xfer + rh->xfer_struct_sz * enqueue_ptr;
+		enum dma_data_direction dir = xfer->rnw ? DMA_FROM_DEVICE :
+							  DMA_TO_DEVICE;
 
 		/* store cmd descriptor */
 		*ring_data++ = xfer->cmd_desc[0];
@@ -391,21 +390,17 @@ static int hci_dma_queue_xfer(struct i3c_hci *hci,
 
 		/* 2nd and 3rd words of Data Buffer Descriptor Structure */
 		if (xfer->data) {
-			buf = xfer->bounce_buf ? xfer->bounce_buf : xfer->data;
-			xfer->data_dma =
-				dma_map_single(&hci->master.dev,
-					       buf,
-					       xfer->data_len,
-					       xfer->rnw ?
-						  DMA_FROM_DEVICE :
-						  DMA_TO_DEVICE);
-			if (dma_mapping_error(&hci->master.dev,
-					      xfer->data_dma)) {
+			xfer->dma = i3c_master_dma_map_single(&hci->master.dev,
+							      xfer->data,
+							      xfer->data_len,
+							      false,
+							      dir);
+			if (!xfer->dma) {
 				hci_dma_unmap_xfer(hci, xfer_list, i);
 				return -ENOMEM;
 			}
-			*ring_data++ = lower_32_bits(xfer->data_dma);
-			*ring_data++ = upper_32_bits(xfer->data_dma);
+			*ring_data++ = lower_32_bits(xfer->dma->addr);
+			*ring_data++ = upper_32_bits(xfer->dma->addr);
 		} else {
 			*ring_data++ = 0;
 			*ring_data++ = 0;
diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
index 69ea1d10414b..33bc4906df1f 100644
--- a/drivers/i3c/master/mipi-i3c-hci/hci.h
+++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
@@ -94,8 +94,7 @@ struct hci_xfer {
 		};
 		struct {
 			/* DMA specific */
-			dma_addr_t data_dma;
-			void *bounce_buf;
+			struct i3c_dma *dma;
 			int ring_number;
 			int ring_entry;
 		};
-- 
2.47.2


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

[PATCH v4 3/4] i3c: mipi-i3c-hci: Use physical device pointer with DMA API

[Jarkko Nikula]

DMA transfer faults on Intel hardware when the IOMMU is enabled and
driver initialization will fail when attempting to do the first transfer:

	DMAR: DRHD: handling fault status reg 2
	DMAR: [DMA Read NO_PASID] Request device [00:11.0] fault addr 0x676e3000 [fault reason 0x71] SM: Present bit in first-level paging entry is clear
 	i3c mipi-i3c-hci.0: ring 0: Transfer Aborted
	mipi-i3c-hci mipi-i3c-hci.0: probe with driver mipi-i3c-hci failed with error -62

Reason for this is that the IOMMU setup is done for the physical devices
only and not for the virtual I3C Controller device object.

Therefore use the pointer to a physical device object with the DMA API.

Due to a data corruption observation when the device DMA is IOMMU
mapped, a properly sized receive bounce buffer is required if transfer
length is not a multiple of DWORDs.

Reported-by: Prabhakaran, Krishna <krishna.prabhakaran@intel.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/dma.c | 46 +++++++++++++++++++--------
 1 file changed, 33 insertions(+), 13 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
index 351851859f02..09688ada4912 100644
--- a/drivers/i3c/master/mipi-i3c-hci/dma.c
+++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
@@ -14,6 +14,7 @@
 #include <linux/errno.h>
 #include <linux/i3c/master.h>
 #include <linux/io.h>
+#include <linux/pci.h>
 
 #include "hci.h"
 #include "cmd.h"
@@ -138,6 +139,7 @@ struct hci_rh_data {
 };
 
 struct hci_rings_data {
+	struct device *sysdev;
 	unsigned int total;
 	struct hci_rh_data headers[] __counted_by(total);
 };
@@ -165,20 +167,20 @@ static void hci_dma_cleanup(struct i3c_hci *hci)
 		rh_reg_write(IBI_SETUP, 0);
 
 		if (rh->xfer)
-			dma_free_coherent(&hci->master.dev,
+			dma_free_coherent(rings->sysdev,
 					  rh->xfer_struct_sz * rh->xfer_entries,
 					  rh->xfer, rh->xfer_dma);
 		if (rh->resp)
-			dma_free_coherent(&hci->master.dev,
+			dma_free_coherent(rings->sysdev,
 					  rh->resp_struct_sz * rh->xfer_entries,
 					  rh->resp, rh->resp_dma);
 		kfree(rh->src_xfers);
 		if (rh->ibi_status)
-			dma_free_coherent(&hci->master.dev,
+			dma_free_coherent(rings->sysdev,
 					  rh->ibi_status_sz * rh->ibi_status_entries,
 					  rh->ibi_status, rh->ibi_status_dma);
 		if (rh->ibi_data_dma)
-			dma_unmap_single(&hci->master.dev, rh->ibi_data_dma,
+			dma_unmap_single(rings->sysdev, rh->ibi_data_dma,
 					 rh->ibi_chunk_sz * rh->ibi_chunks_total,
 					 DMA_FROM_DEVICE);
 		kfree(rh->ibi_data);
@@ -194,11 +196,23 @@ static int hci_dma_init(struct i3c_hci *hci)
 {
 	struct hci_rings_data *rings;
 	struct hci_rh_data *rh;
+	struct device *sysdev;
 	u32 regval;
 	unsigned int i, nr_rings, xfers_sz, resps_sz;
 	unsigned int ibi_status_ring_sz, ibi_data_ring_sz;
 	int ret;
 
+	/*
+	 * Set pointer to a physical device that does DMA and has IOMMU setup
+	 * done for it in case of enabled IOMMU and use it with the DMA API.
+	 * Here such device is either
+	 * "mipi-i3c-hci" platform device (OF/ACPI enumeration) parent or
+	 * grandparent (PCI enumeration).
+	 */
+	sysdev = hci->master.dev.parent;
+	if (sysdev->parent && dev_is_pci(sysdev->parent))
+		sysdev = sysdev->parent;
+
 	regval = rhs_reg_read(CONTROL);
 	nr_rings = FIELD_GET(MAX_HEADER_COUNT_CAP, regval);
 	dev_info(&hci->master.dev, "%d DMA rings available\n", nr_rings);
@@ -213,6 +227,7 @@ static int hci_dma_init(struct i3c_hci *hci)
 		return -ENOMEM;
 	hci->io_data = rings;
 	rings->total = nr_rings;
+	rings->sysdev = sysdev;
 
 	regval = FIELD_PREP(MAX_HEADER_COUNT, rings->total);
 	rhs_reg_write(CONTROL, regval);
@@ -239,9 +254,9 @@ static int hci_dma_init(struct i3c_hci *hci)
 		xfers_sz = rh->xfer_struct_sz * rh->xfer_entries;
 		resps_sz = rh->resp_struct_sz * rh->xfer_entries;
 
-		rh->xfer = dma_alloc_coherent(&hci->master.dev, xfers_sz,
+		rh->xfer = dma_alloc_coherent(rings->sysdev, xfers_sz,
 					      &rh->xfer_dma, GFP_KERNEL);
-		rh->resp = dma_alloc_coherent(&hci->master.dev, resps_sz,
+		rh->resp = dma_alloc_coherent(rings->sysdev, resps_sz,
 					      &rh->resp_dma, GFP_KERNEL);
 		rh->src_xfers =
 			kmalloc_array(rh->xfer_entries, sizeof(*rh->src_xfers),
@@ -295,16 +310,16 @@ static int hci_dma_init(struct i3c_hci *hci)
 		ibi_data_ring_sz = rh->ibi_chunk_sz * rh->ibi_chunks_total;
 
 		rh->ibi_status =
-			dma_alloc_coherent(&hci->master.dev, ibi_status_ring_sz,
+			dma_alloc_coherent(rings->sysdev, ibi_status_ring_sz,
 					   &rh->ibi_status_dma, GFP_KERNEL);
 		rh->ibi_data = kmalloc(ibi_data_ring_sz, GFP_KERNEL);
 		ret = -ENOMEM;
 		if (!rh->ibi_status || !rh->ibi_data)
 			goto err_out;
 		rh->ibi_data_dma =
-			dma_map_single(&hci->master.dev, rh->ibi_data,
+			dma_map_single(rings->sysdev, rh->ibi_data,
 				       ibi_data_ring_sz, DMA_FROM_DEVICE);
-		if (dma_mapping_error(&hci->master.dev, rh->ibi_data_dma)) {
+		if (dma_mapping_error(rings->sysdev, rh->ibi_data_dma)) {
 			rh->ibi_data_dma = 0;
 			ret = -ENOMEM;
 			goto err_out;
@@ -372,6 +387,7 @@ static int hci_dma_queue_xfer(struct i3c_hci *hci,
 		u32 *ring_data = rh->xfer + rh->xfer_struct_sz * enqueue_ptr;
 		enum dma_data_direction dir = xfer->rnw ? DMA_FROM_DEVICE :
 							  DMA_TO_DEVICE;
+		bool need_bounce;
 
 		/* store cmd descriptor */
 		*ring_data++ = xfer->cmd_desc[0];
@@ -390,10 +406,13 @@ static int hci_dma_queue_xfer(struct i3c_hci *hci,
 
 		/* 2nd and 3rd words of Data Buffer Descriptor Structure */
 		if (xfer->data) {
-			xfer->dma = i3c_master_dma_map_single(&hci->master.dev,
+			need_bounce = device_iommu_mapped(rings->sysdev) &&
+				      xfer->rnw &&
+				      xfer->data_len != ALIGN(xfer->data_len, 4);
+			xfer->dma = i3c_master_dma_map_single(rings->sysdev,
 							      xfer->data,
 							      xfer->data_len,
-							      false,
+							      need_bounce,
 							      dir);
 			if (!xfer->dma) {
 				hci_dma_unmap_xfer(hci, xfer_list, i);
@@ -581,6 +600,7 @@ static void hci_dma_recycle_ibi_slot(struct i3c_hci *hci,
 
 static void hci_dma_process_ibi(struct i3c_hci *hci, struct hci_rh_data *rh)
 {
+	struct hci_rings_data *rings = hci->io_data;
 	struct i3c_dev_desc *dev;
 	struct i3c_hci_dev_data *dev_data;
 	struct hci_dma_dev_ibi_data *dev_ibi;
@@ -691,7 +711,7 @@ static void hci_dma_process_ibi(struct i3c_hci *hci, struct hci_rh_data *rh)
 			* rh->ibi_chunk_sz;
 	if (first_part > ibi_size)
 		first_part = ibi_size;
-	dma_sync_single_for_cpu(&hci->master.dev, ring_ibi_data_dma,
+	dma_sync_single_for_cpu(rings->sysdev, ring_ibi_data_dma,
 				first_part, DMA_FROM_DEVICE);
 	memcpy(slot->data, ring_ibi_data, first_part);
 
@@ -700,7 +720,7 @@ static void hci_dma_process_ibi(struct i3c_hci *hci, struct hci_rh_data *rh)
 		/* we wrap back to the start and copy remaining data */
 		ring_ibi_data = rh->ibi_data;
 		ring_ibi_data_dma = rh->ibi_data_dma;
-		dma_sync_single_for_cpu(&hci->master.dev, ring_ibi_data_dma,
+		dma_sync_single_for_cpu(rings->sysdev, ring_ibi_data_dma,
 					ibi_size - first_part, DMA_FROM_DEVICE);
 		memcpy(slot->data + first_part, ring_ibi_data,
 		       ibi_size - first_part);
-- 
2.47.2


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

[PATCH v4 4/4] i3c: mipi-i3c-hci: Use own DMA bounce buffer management for I2C transfers

[Jarkko Nikula]

Stop using I2C DMA-safe API for two reasons:
- Not needed if driver is using PIO mode.
- DMA transfers needs a DWORD align sized receive bounce buffer when the
  device DMA is IOMMU mapped, which is causing needless double bounce
  buffering in that case.

Cc: Billy Tsai <billy_tsai@aspeedtech.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 | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
index b2977b6ac9f7..7a467ef65787 100644
--- a/drivers/i3c/master/mipi-i3c-hci/core.c
+++ b/drivers/i3c/master/mipi-i3c-hci/core.c
@@ -348,7 +348,7 @@ static int i3c_hci_i2c_xfers(struct i2c_dev_desc *dev,
 		return -ENOMEM;
 
 	for (i = 0; i < nxfers; i++) {
-		xfer[i].data = i2c_get_dma_safe_msg_buf(&i2c_xfers[i], 1);
+		xfer[i].data = i2c_xfers[i].buf;
 		xfer[i].data_len = i2c_xfers[i].len;
 		xfer[i].rnw = i2c_xfers[i].flags & I2C_M_RD;
 		hci->cmd->prep_i2c_xfer(hci, dev, &xfer[i]);
@@ -374,10 +374,6 @@ static int i3c_hci_i2c_xfers(struct i2c_dev_desc *dev,
 	}
 
 out:
-	for (i = 0; i < nxfers; i++)
-		i2c_put_dma_safe_msg_buf(xfer[i].data, &i2c_xfers[i],
-					 ret ? false : true);
-
 	hci_free_xfer(xfer, nxfers);
 	return ret;
 }
-- 
2.47.2


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

Re: [PATCH v4 1/4] i3c: master: Add helpers for DMA mapping and bounce buffer handling

[Frank Li]

On Fri, Aug 22, 2025 at 01:56:27PM +0300, Jarkko Nikula wrote:
> Some I3C controllers such as MIPI I3C HCI may pad the last DWORD (32-bit)
> with stale data from the RX FIFO in DMA transfers if the receive length
> is not DWORD aligned and when the device DMA is IOMMU mapped.
>
> In such a case, a properly sized bounce buffer is required in order to
> avoid possible data corruption. In a review discussion, proposal was to
> have a common helpers in I3C core for DMA mapping and bounce buffer
> handling.
>
> Drivers may use the helper i3c_master_dma_map_single() to map a buffer
> for a DMA transfer. It internally allocates a bounce buffer if buffer is
> not DMA'able or when the driver requires it for a transfer.
>
> Helper i3c_master_dma_unmap_single() does the needed cleanups and
> data copying from the bounce buffer.
>
> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>

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

> ---
>  drivers/i3c/master.c       | 74 ++++++++++++++++++++++++++++++++++++++
>  include/linux/i3c/master.h | 26 ++++++++++++++
>  2 files changed, 100 insertions(+)
>
> diff --git a/drivers/i3c/master.c b/drivers/i3c/master.c
> index 2ef898a8fd80..033d06cabca2 100644
> --- a/drivers/i3c/master.c
> +++ b/drivers/i3c/master.c
> @@ -8,6 +8,7 @@
>  #include <linux/atomic.h>
>  #include <linux/bug.h>
>  #include <linux/device.h>
> +#include <linux/dma-mapping.h>
>  #include <linux/err.h>
>  #include <linux/export.h>
>  #include <linux/kernel.h>
> @@ -1727,6 +1728,79 @@ int i3c_master_do_daa(struct i3c_master_controller *master)
>  }
>  EXPORT_SYMBOL_GPL(i3c_master_do_daa);
>
> +/**
> + * i3c_master_dma_map_single() - Map buffer for single DMA transfer
> + * @dev: device object of a device doing DMA
> + * @buf: destination/source buffer for DMA
> + * @len: length of transfer
> + * @force_bounce: true, force to use a bounce buffer,
> + *                false, function will auto check is a bounce buffer required
> + * @dir: DMA direction
> + *
> + * Map buffer for a DMA transfer and allocate a bounce buffer if required.
> + *
> + * Return: I3C DMA transfer descriptor or NULL in case of error.
> + */
> +struct i3c_dma *i3c_master_dma_map_single(struct device *dev, void *buf,
> +	size_t len, bool force_bounce, enum dma_data_direction dir)
> +{
> +	struct i3c_dma *dma_xfer __free(kfree) = NULL;
> +	void *bounce __free(kfree) = NULL;
> +	void *dma_buf = buf;
> +
> +	dma_xfer = kzalloc(sizeof(*dma_xfer), GFP_KERNEL);
> +	if (!dma_xfer)
> +		return NULL;
> +
> +	dma_xfer->dev = dev;
> +	dma_xfer->buf = buf;
> +	dma_xfer->dir = dir;
> +	dma_xfer->len = len;
> +	dma_xfer->map_len = len;
> +
> +	if (is_vmalloc_addr(buf))
> +		force_bounce = true;
> +
> +	if (force_bounce) {
> +		dma_xfer->map_len = ALIGN(len, cache_line_size());
> +		if (dir == DMA_FROM_DEVICE)
> +			bounce = kzalloc(dma_xfer->map_len, GFP_KERNEL);
> +		else
> +			bounce = kmemdup(buf, dma_xfer->map_len, GFP_KERNEL);
> +		if (!bounce)
> +			return NULL;
> +		dma_buf = bounce;
> +	}
> +
> +	dma_xfer->addr = dma_map_single(dev, dma_buf, dma_xfer->map_len, dir);
> +	if (dma_mapping_error(dev, dma_xfer->addr))
> +		return NULL;
> +
> +	dma_xfer->bounce_buf = no_free_ptr(bounce);
> +	return no_free_ptr(dma_xfer);
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_dma_map_single);
> +
> +/**
> + * i3c_master_dma_unmap_single() - Unmap buffer after DMA
> + * @dma_xfer: DMA transfer and mapping descriptor
> + *
> + * Unmap buffer and cleanup DMA transfer descriptor.
> + */
> +void i3c_master_dma_unmap_single(struct i3c_dma *dma_xfer)
> +{
> +	dma_unmap_single(dma_xfer->dev, dma_xfer->addr,
> +			 dma_xfer->map_len, dma_xfer->dir);
> +	if (dma_xfer->bounce_buf) {
> +		if (dma_xfer->dir == DMA_FROM_DEVICE)
> +			memcpy(dma_xfer->buf, dma_xfer->bounce_buf,
> +			       dma_xfer->len);
> +		kfree(dma_xfer->bounce_buf);
> +	}
> +	kfree(dma_xfer);
> +}
> +EXPORT_SYMBOL_GPL(i3c_master_dma_unmap_single);
> +
>  /**
>   * i3c_master_set_info() - set master device information
>   * @master: master used to send frames on the bus
> diff --git a/include/linux/i3c/master.h b/include/linux/i3c/master.h
> index 043f5c7ff398..c52a82dd79a6 100644
> --- a/include/linux/i3c/master.h
> +++ b/include/linux/i3c/master.h
> @@ -558,6 +558,26 @@ struct i3c_master_controller {
>  #define i3c_bus_for_each_i3cdev(bus, dev)				\
>  	list_for_each_entry(dev, &(bus)->devs.i3c, common.node)
>
> +/**
> + * struct i3c_dma - DMA transfer and mapping descriptor
> + * @dev: device object of a device doing DMA
> + * @buf: destination/source buffer for DMA
> + * @len: length of transfer
> + * @map_len: length of DMA mapping
> + * @addr: mapped DMA address for a Host Controller Driver
> + * @dir: DMA direction
> + * @bounce_buf: an allocated bounce buffer if transfer needs it or NULL
> + */
> +struct i3c_dma {
> +	struct device *dev;
> +	void *buf;
> +	size_t len;
> +	size_t map_len;
> +	dma_addr_t addr;
> +	enum dma_data_direction dir;
> +	void *bounce_buf;
> +};
> +
>  int i3c_master_do_i2c_xfers(struct i3c_master_controller *master,
>  			    const struct i2c_msg *xfers,
>  			    int nxfers);
> @@ -575,6 +595,12 @@ int i3c_master_get_free_addr(struct i3c_master_controller *master,
>  int i3c_master_add_i3c_dev_locked(struct i3c_master_controller *master,
>  				  u8 addr);
>  int i3c_master_do_daa(struct i3c_master_controller *master);
> +struct i3c_dma *i3c_master_dma_map_single(struct device *dev, void *ptr,
> +					  size_t len, bool force_bounce,
> +					  enum dma_data_direction dir);
> +void i3c_master_dma_unmap_single(struct i3c_dma *dma_xfer);
> +DEFINE_FREE(i3c_master_dma_unmap_single, void *,
> +	    if (_T) i3c_master_dma_unmap_single(_T))
>
>  int i3c_master_set_info(struct i3c_master_controller *master,
>  			const struct i3c_device_info *info);
> --
> 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 v4 2/4] i3c: mipi-i3c-hci: Use core helpers for DMA mapping and bounce buffering

[Frank Li]

On Fri, Aug 22, 2025 at 01:56:28PM +0300, Jarkko Nikula wrote:
> So far only I3C private and I2C transfers have required a bounce buffer
> for DMA transfers when buffer is not DMA'able.
>
> It was observed that when the device DMA is IOMMU mapped and the receive
> length is not a multiple of DWORDs (32-bit), the last DWORD is padded
> with stale data from the RX FIFO, corrupting 1-3 bytes beyond the
> expected data.
>
> A similar issue, though less severe, occurs when an I3C target returns
> less data than requested. In this case, the padding does not exceed the
> requested number of bytes, assuming the device DMA is not IOMMU mapped.
>
> Therefore, all I3C private transfer, CCC command payload and I2C
> transfer receive buffers must be properly sized for the DMA being IOMMU
> mapped. Even if those buffers are already DMA safe, their size may not
> be DWORD aligned.
>
> To prepare for the device DMA being IOMMU mapped and to address the
> above issue, use helpers from I3C core for DMA mapping and bounce
> buffering for all DMA transfers.
>
> For now, require bounce buffer only when the buffer is in the
> vmalloc() area to avoid unnecessary copying with CCC commands and
> DMA-safe I2C transfers.
>
> 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 | 34 --------------------------
>  drivers/i3c/master/mipi-i3c-hci/dma.c  | 27 +++++++++-----------
>  drivers/i3c/master/mipi-i3c-hci/hci.h  |  3 +--
>  3 files changed, 12 insertions(+), 52 deletions(-)
>
> diff --git a/drivers/i3c/master/mipi-i3c-hci/core.c b/drivers/i3c/master/mipi-i3c-hci/core.c
> index 60f1175f1f37..b2977b6ac9f7 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/core.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/core.c
> @@ -272,34 +272,6 @@ static int i3c_hci_daa(struct i3c_master_controller *m)
>  	return hci->cmd->perform_daa(hci);
>  }
>
> -static int i3c_hci_alloc_safe_xfer_buf(struct i3c_hci *hci,
> -				       struct hci_xfer *xfer)
> -{
> -	if (hci->io != &mipi_i3c_hci_dma ||
> -	    xfer->data == NULL || !is_vmalloc_addr(xfer->data))
> -		return 0;
> -
> -	if (xfer->rnw)
> -		xfer->bounce_buf = kzalloc(xfer->data_len, GFP_KERNEL);
> -	else
> -		xfer->bounce_buf = kmemdup(xfer->data,
> -					   xfer->data_len, GFP_KERNEL);
> -
> -	return xfer->bounce_buf == NULL ? -ENOMEM : 0;
> -}
> -
> -static void i3c_hci_free_safe_xfer_buf(struct i3c_hci *hci,
> -				       struct hci_xfer *xfer)
> -{
> -	if (hci->io != &mipi_i3c_hci_dma || xfer->bounce_buf == NULL)
> -		return;
> -
> -	if (xfer->rnw)
> -		memcpy(xfer->data, xfer->bounce_buf, xfer->data_len);
> -
> -	kfree(xfer->bounce_buf);
> -}
> -
>  static int i3c_hci_priv_xfers(struct i3c_dev_desc *dev,
>  			      struct i3c_priv_xfer *i3c_xfers,
>  			      int nxfers)
> @@ -333,9 +305,6 @@ static int i3c_hci_priv_xfers(struct i3c_dev_desc *dev,
>  		}
>  		hci->cmd->prep_i3c_xfer(hci, dev, &xfer[i]);
>  		xfer[i].cmd_desc[0] |= CMD_0_ROC;
> -		ret = i3c_hci_alloc_safe_xfer_buf(hci, &xfer[i]);
> -		if (ret)
> -			goto out;
>  	}
>  	last = i - 1;
>  	xfer[last].cmd_desc[0] |= CMD_0_TOC;
> @@ -359,9 +328,6 @@ static int i3c_hci_priv_xfers(struct i3c_dev_desc *dev,
>  	}
>
>  out:
> -	for (i = 0; i < nxfers; i++)
> -		i3c_hci_free_safe_xfer_buf(hci, &xfer[i]);
> -
>  	hci_free_xfer(xfer, nxfers);
>  	return ret;
>  }
> diff --git a/drivers/i3c/master/mipi-i3c-hci/dma.c b/drivers/i3c/master/mipi-i3c-hci/dma.c
> index 491dfe70b660..351851859f02 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/dma.c
> +++ b/drivers/i3c/master/mipi-i3c-hci/dma.c
> @@ -349,9 +349,7 @@ static void hci_dma_unmap_xfer(struct i3c_hci *hci,
>  		xfer = xfer_list + i;
>  		if (!xfer->data)
>  			continue;
> -		dma_unmap_single(&hci->master.dev,
> -				 xfer->data_dma, xfer->data_len,
> -				 xfer->rnw ? DMA_FROM_DEVICE : DMA_TO_DEVICE);
> +		i3c_master_dma_unmap_single(xfer->dma);
>  	}
>  }
>
> @@ -362,7 +360,6 @@ static int hci_dma_queue_xfer(struct i3c_hci *hci,
>  	struct hci_rh_data *rh;
>  	unsigned int i, ring, enqueue_ptr;
>  	u32 op1_val, op2_val;
> -	void *buf;
>
>  	/* For now we only use ring 0 */
>  	ring = 0;
> @@ -373,6 +370,8 @@ static int hci_dma_queue_xfer(struct i3c_hci *hci,
>  	for (i = 0; i < n; i++) {
>  		struct hci_xfer *xfer = xfer_list + i;
>  		u32 *ring_data = rh->xfer + rh->xfer_struct_sz * enqueue_ptr;
> +		enum dma_data_direction dir = xfer->rnw ? DMA_FROM_DEVICE :
> +							  DMA_TO_DEVICE;
>
>  		/* store cmd descriptor */
>  		*ring_data++ = xfer->cmd_desc[0];
> @@ -391,21 +390,17 @@ static int hci_dma_queue_xfer(struct i3c_hci *hci,
>
>  		/* 2nd and 3rd words of Data Buffer Descriptor Structure */
>  		if (xfer->data) {
> -			buf = xfer->bounce_buf ? xfer->bounce_buf : xfer->data;
> -			xfer->data_dma =
> -				dma_map_single(&hci->master.dev,
> -					       buf,
> -					       xfer->data_len,
> -					       xfer->rnw ?
> -						  DMA_FROM_DEVICE :
> -						  DMA_TO_DEVICE);
> -			if (dma_mapping_error(&hci->master.dev,
> -					      xfer->data_dma)) {
> +			xfer->dma = i3c_master_dma_map_single(&hci->master.dev,
> +							      xfer->data,
> +							      xfer->data_len,
> +							      false,
> +							      dir);
> +			if (!xfer->dma) {
>  				hci_dma_unmap_xfer(hci, xfer_list, i);
>  				return -ENOMEM;
>  			}
> -			*ring_data++ = lower_32_bits(xfer->data_dma);
> -			*ring_data++ = upper_32_bits(xfer->data_dma);
> +			*ring_data++ = lower_32_bits(xfer->dma->addr);
> +			*ring_data++ = upper_32_bits(xfer->dma->addr);
>  		} else {
>  			*ring_data++ = 0;
>  			*ring_data++ = 0;
> diff --git a/drivers/i3c/master/mipi-i3c-hci/hci.h b/drivers/i3c/master/mipi-i3c-hci/hci.h
> index 69ea1d10414b..33bc4906df1f 100644
> --- a/drivers/i3c/master/mipi-i3c-hci/hci.h
> +++ b/drivers/i3c/master/mipi-i3c-hci/hci.h
> @@ -94,8 +94,7 @@ struct hci_xfer {
>  		};
>  		struct {
>  			/* DMA specific */
> -			dma_addr_t data_dma;
> -			void *bounce_buf;
> +			struct i3c_dma *dma;
>  			int ring_number;
>  			int ring_entry;
>  		};
> --
> 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