[PATCH 0/5] Add support for Verisilicon IOMMU used by media codec blocks

[PATCH 0/5] Add support for Verisilicon IOMMU used by media codec blocks

[Benjamin Gaignard]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=y, Size: 1831 bytes --]

Hi all,

This patch series adds support for the Verisilicon IOMMU, which is found in front
of hardware encoder and decoder blocks in several SoCs using Verisilicon IP. 
A first implementation of this IOMMU is available on the Rockchip RK3588 SoC.

Rockchip provides a driver for this hardware in their 6.1 kernel branch:
https://github.com/rockchip-linux/kernel/blob/develop-6.1/drivers/iommu/rockchip-iommu-av1d.c

This series includes:
- a new binding for the Verisilicon IOMMU
- a basic driver implementation
- DT updates for RK3588
- integration with the Verisilicon VPU driver.

The driver was forward-ported from Rockchip’s 6.1 implementation, 
the prefix was renamed to vsi for generality, and several fixes were applied.

AV1 decoding was tested using the stateless VPU driver and Fluster.
The test results show a score of 205/239, which confirms that no regressions
were introduced by this series.

Feedback and testing welcome.

Thanks,
Benjamin

Benjamin Gaignard (5):
  dt-bindings: vendor-prefixes: Add Verisilicon
  dt-bindings: iommu: verisilicon: Add binding for VSI IOMMU
  iommu: Add verisilicon IOMMU driver
  arm64: dts: rockchip: Add verisilicon IOMMU node on RK3588
  media: verisilicon: Flush IOMMU before decoding a frame

 .../bindings/iommu/verisilicon,iommu.yaml     |  71 ++
 .../devicetree/bindings/vendor-prefixes.yaml  |   2 +
 arch/arm64/boot/dts/rockchip/rk3588-base.dtsi |  11 +
 drivers/iommu/Kconfig                         |   8 +
 drivers/iommu/Makefile                        |   1 +
 drivers/iommu/vsi-iommu.c                     | 900 ++++++++++++++++++
 .../media/platform/verisilicon/hantro_drv.c   |  11 +
 7 files changed, 1004 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml
 create mode 100644 drivers/iommu/vsi-iommu.c

-- 
2.43.0

[PATCH 1/5] dt-bindings: vendor-prefixes: Add Verisilicon

[Benjamin Gaignard]

Verisilicon Microelectronics is a company based in Shanghai, China,
developping hardware blocks for SoC.

https://verisilicon.com/

Add their name to the list of vendors.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
 Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
index 5d2a7a8d3ac6..1baf8304c9ac 100644
--- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
+++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
@@ -1635,6 +1635,8 @@ patternProperties:
     description: Variscite Ltd.
   "^vdl,.*":
     description: Van der Laan b.v.
+  "^verisilicon,.*":
+    description: VeriSilicon Microelectronics
   "^vertexcom,.*":
     description: Vertexcom Technologies, Inc.
   "^via,.*":
-- 
2.43.0

[PATCH 2/5] dt-bindings: iommu: verisilicon: Add binding for VSI IOMMU

[Benjamin Gaignard]

Add a device tree binding for the Verisilicon (VSI) IOMMU. This IOMMU sits
in front of hardware encoder and decoder blocks on SoCs using Verisilicon IP,
such as the Rockchip RK3588.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
 .../bindings/iommu/verisilicon,iommu.yaml     | 71 +++++++++++++++++++
 1 file changed, 71 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml

diff --git a/Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml b/Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml
new file mode 100644
index 000000000000..acef855fc61d
--- /dev/null
+++ b/Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml
@@ -0,0 +1,71 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iommu/verisilicon,iommu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Verisilicon IOMMU
+
+maintainers:
+  - Benjamin Gaignard <benjamin.gaignard@collabora.com>
+
+description: |+
+  A Versilicon iommu translates io virtual addresses to physical addresses for
+  its associated video decoder.
+
+properties:
+  compatible:
+    oneOf:
+      - items:
+          - const: verisilicon,iommu
+
+  reg:
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  clocks:
+    items:
+      - description: Core clock
+      - description: Interface clock
+
+  clock-names:
+    items:
+      - const: aclk
+      - const: iface
+
+  "#iommu-cells":
+    const: 0
+
+  power-domains:
+    maxItems: 1
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - clocks
+  - clock-names
+  - "#iommu-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/clock/rockchip,rk3588-cru.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+
+    bus {
+      #address-cells = <2>;
+      #size-cells = <2>;
+
+      vsi_mmu: iommu@fdca0000 {
+        compatible = "verisilicon,iommu";
+        reg = <0x0 0xfdca0000 0x0 0x600>;
+        interrupts = <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH 0>;
+        clocks = <&cru ACLK_AV1>, <&cru PCLK_AV1>;
+        clock-names = "aclk", "iface";
+        #iommu-cells = <0>;
+      };
+    };
-- 
2.43.0

[PATCH 3/5] iommu: Add verisilicon IOMMU driver

[Benjamin Gaignard]

Verisilicon IOMMU hardware block can be found in combination
with hardware video codecs (encoders or decoders) on
different SoCs.
Enable it will allows to use non contiguous memory
allocators for Verisilicon video codecs.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
 drivers/iommu/Kconfig     |   8 +
 drivers/iommu/Makefile    |   1 +
 drivers/iommu/vsi-iommu.c | 900 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 909 insertions(+)
 create mode 100644 drivers/iommu/vsi-iommu.c

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 0a33d995d15d..4cf4504dcc25 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -383,4 +383,12 @@ config SPRD_IOMMU
 
 	  Say Y here if you want to use the multimedia devices listed above.
 
+config VSI_IOMMU
+	bool "Verisilicon IOMMU Support"
+	depends on ARM64
+	select IOMMU_API
+	select ARM_DMA_USE_IOMMU
+	help
+	  Support for IOMMUs used by Verisilicon video decoders.
+
 endif # IOMMU_SUPPORT
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 355294fa9033..68aeff31af8b 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -34,3 +34,4 @@ obj-$(CONFIG_IOMMU_SVA) += iommu-sva.o
 obj-$(CONFIG_IOMMU_IOPF) += io-pgfault.o
 obj-$(CONFIG_SPRD_IOMMU) += sprd-iommu.o
 obj-$(CONFIG_APPLE_DART) += apple-dart.o
+obj-$(CONFIG_VSI_IOMMU) += vsi-iommu.o
diff --git a/drivers/iommu/vsi-iommu.c b/drivers/iommu/vsi-iommu.c
new file mode 100644
index 000000000000..f2fa1197916c
--- /dev/null
+++ b/drivers/iommu/vsi-iommu.c
@@ -0,0 +1,900 @@
+// SPDX-License-Identifier: GPL-2.0-only
+
+#include <linux/clk.h>
+#include <linux/compiler.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/dma-mapping.h>
+#include <linux/dma-map-ops.h>
+#include <linux/errno.h>
+#include <linux/init.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/iommu.h>
+#include <linux/iopoll.h>
+#include <linux/list.h>
+#include <linux/mm.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_iommu.h>
+#include <linux/of_platform.h>
+#include <linux/platform_device.h>
+#include <linux/pm_runtime.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+#include "iommu-pages.h"
+
+struct vsi_iommu_domain {
+	struct list_head iommus;
+	u32 *dt; /* page directory table */
+	dma_addr_t dt_dma;
+	spinlock_t iommus_lock; /* lock for iommus list */
+	spinlock_t dt_lock; /* lock for modifying page directory table */
+	struct iommu_domain domain;
+	/* for vsi iommu */
+	u64 *pta; /* page directory table */
+	dma_addr_t pta_dma;
+};
+
+struct vsi_iommu {
+	struct device *dev;
+	void __iomem **bases;
+	int num_mmu;
+	int num_irq;
+	struct clk_bulk_data *clocks;
+	int num_clocks;
+	struct iommu_device iommu;
+	struct list_head node; /* entry in vsi_iommu_domain.iommus */
+	struct iommu_domain *domain; /* domain to which iommu is attached */
+	struct iommu_group *group;
+};
+
+struct vsi_iommudata {
+	struct device_link *link; /* runtime PM link from IOMMU to master */
+	struct vsi_iommu *iommu;
+	bool defer_attach;
+};
+
+#define NUM_DT_ENTRIES	1024
+#define NUM_PT_ENTRIES	1024
+
+#define SPAGE_SIZE	BIT(12)
+
+/* vsi iommu regs address */
+#define VSI_MMU_CONFIG1_BASE			0x1ac
+#define VSI_MMU_AHB_EXCEPTION_BASE		0x380
+#define VSI_MMU_AHB_CONTROL_BASE		0x388
+#define VSI_MMU_AHB_TBL_ARRAY_BASE_L_BASE	0x38C
+
+/* MMU register offsets */
+#define VSI_MMU_FLUSH_BASE		0x184
+#define VSI_MMU_BIT_FLUSH		BIT(4)
+
+#define VSI_MMU_PAGE_FAULT_ADDR		0x380
+#define VSI_MMU_STATUS_BASE		0x384	/* IRQ status */
+
+#define VSI_MMU_BIT_ENABLE		BIT(0)
+
+#define VSI_MMU_OUT_OF_BOUND		BIT(28)
+/* Irq mask */
+#define VSI_MMU_IRQ_MASK		0x7
+
+#define VSI_DTE_PT_ADDRESS_MASK		0xffffffc0
+#define VSI_DTE_PT_VALID		BIT(0)
+
+#define VSI_PAGE_DESC_LO_MASK		0xfffff000
+#define VSI_PAGE_DESC_HI_MASK		GENMASK_ULL(39, 32)
+#define VSI_PAGE_DESC_HI_SHIFT		(32 - 4)
+
+#define VSI_IOMMU_PGSIZE_BITMAP 0x007ff000
+
+static inline phys_addr_t vsi_dte_pt_address(u32 dte)
+{
+	return (phys_addr_t)dte & VSI_DTE_PT_ADDRESS_MASK;
+}
+
+static inline u32 vsi_mk_dte(dma_addr_t pt_dma)
+{
+	return (pt_dma) | VSI_DTE_PT_VALID;
+}
+
+#define VSI_PTE_PAGE_ADDRESS_MASK	0xfffffff0
+#define VSI_PTE_PAGE_WRITABLE		BIT(2)
+#define VSI_PTE_PAGE_VALID		BIT(0)
+
+static struct device *dma_dev;
+
+static inline phys_addr_t vsi_pte_page_address(u32 pte)
+{
+	u64 pte_vsi = pte;
+
+	pte_vsi = ((pte_vsi & VSI_PAGE_DESC_HI_MASK) << VSI_PAGE_DESC_HI_SHIFT) |
+		  (pte_vsi & VSI_PAGE_DESC_LO_MASK);
+
+	return (phys_addr_t)pte_vsi;
+}
+
+static u32 vsi_mk_pte(phys_addr_t page, int prot)
+{
+	u32 flags = 0;
+
+	flags |= (prot & IOMMU_WRITE) ? VSI_PTE_PAGE_WRITABLE : 0;
+	page = (page & VSI_PAGE_DESC_LO_MASK) |
+	       ((page & VSI_PAGE_DESC_HI_MASK) >> VSI_PAGE_DESC_HI_SHIFT);
+	page &= VSI_PTE_PAGE_ADDRESS_MASK;
+
+	return page | flags | VSI_PTE_PAGE_VALID;
+}
+
+#define VSI_DTE_PT_VALID	BIT(0)
+
+static inline bool vsi_dte_is_pt_valid(u32 dte)
+{
+	return dte & VSI_DTE_PT_VALID;
+}
+
+static inline bool vsi_pte_is_page_valid(u32 pte)
+{
+	return pte & VSI_PTE_PAGE_VALID;
+}
+
+static u32 vsi_mk_pte_invalid(u32 pte)
+{
+	return pte & ~VSI_PTE_PAGE_VALID;
+}
+
+#define VSI_MASTER_TLB_MASK	GENMASK_ULL(31, 10)
+/* mode 0 : 4k */
+#define VSI_PTA_4K_MODE	0
+
+static struct vsi_iommu *vsi_iommu_from_dev(struct device *dev)
+{
+	struct vsi_iommudata *data = dev_iommu_priv_get(dev);
+
+	return data ? data->iommu : NULL;
+}
+
+static u64 vsi_mk_pta(dma_addr_t dt_dma)
+{
+	u64 val = (dt_dma & VSI_MASTER_TLB_MASK) | VSI_PTA_4K_MODE;
+
+	return val;
+}
+
+static struct vsi_iommu_domain *to_vsi_domain(struct iommu_domain *dom)
+{
+	return container_of(dom, struct vsi_iommu_domain, domain);
+}
+
+static void vsi_iommu_disable(struct vsi_iommu *iommu)
+{
+	int i;
+
+	/* Ignore error while disabling, just keep going */
+	WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks));
+	for (i = 0; i < iommu->num_mmu; i++)
+		writel(0, iommu->bases[i] + VSI_MMU_AHB_CONTROL_BASE);
+
+	clk_bulk_disable(iommu->num_clocks, iommu->clocks);
+}
+
+static int vsi_iommu_enable(struct vsi_iommu *iommu)
+{
+	struct iommu_domain *domain = iommu->domain;
+	struct vsi_iommu_domain *vsi_domain = to_vsi_domain(domain);
+	int ret, i;
+
+	ret = clk_bulk_enable(iommu->num_clocks, iommu->clocks);
+	if (ret)
+		return ret;
+
+	for (i = 0; i < iommu->num_mmu; i++) {
+		u32 val = readl(iommu->bases[i] + VSI_MMU_AHB_CONTROL_BASE);
+
+		if (!(val & VSI_MMU_BIT_ENABLE)) {
+			writel(vsi_domain->pta_dma,
+			       iommu->bases[i] + VSI_MMU_AHB_TBL_ARRAY_BASE_L_BASE);
+			writel(VSI_MMU_OUT_OF_BOUND, iommu->bases[i] + VSI_MMU_CONFIG1_BASE);
+			writel(VSI_MMU_BIT_ENABLE, iommu->bases[i] + VSI_MMU_AHB_EXCEPTION_BASE);
+			writel(VSI_MMU_BIT_ENABLE, iommu->bases[i] + VSI_MMU_AHB_CONTROL_BASE);
+		}
+	}
+	clk_bulk_disable(iommu->num_clocks, iommu->clocks);
+
+	return ret;
+}
+
+static inline void vsi_table_flush(struct vsi_iommu_domain *dom, dma_addr_t dma,
+				   unsigned int count)
+{
+	size_t size = count * sizeof(u32); /* count of u32 entry */
+
+	dma_sync_single_for_device(dma_dev, dma, size, DMA_TO_DEVICE);
+}
+
+#define VSI_IOVA_DTE_MASK	0xffc00000
+#define VSI_IOVA_DTE_SHIFT	22
+#define VSI_IOVA_PTE_MASK	0x003ff000
+#define VSI_IOVA_PTE_SHIFT	12
+#define VSI_IOVA_PAGE_MASK	0x00000fff
+#define VSI_IOVA_PAGE_SHIFT	0
+
+static u32 vsi_iova_dte_index(dma_addr_t iova)
+{
+	return (u32)(iova & VSI_IOVA_DTE_MASK) >> VSI_IOVA_DTE_SHIFT;
+}
+
+static u32 vsi_iova_pte_index(dma_addr_t iova)
+{
+	return (u32)(iova & VSI_IOVA_PTE_MASK) >> VSI_IOVA_PTE_SHIFT;
+}
+
+static u32 vsi_iova_page_offset(dma_addr_t iova)
+{
+	return (u32)(iova & VSI_IOVA_PAGE_MASK) >> VSI_IOVA_PAGE_SHIFT;
+}
+
+static u32 vsi_iommu_read(void __iomem *base, u32 offset)
+{
+	return readl(base + offset);
+}
+
+static void vsi_iommu_write(void __iomem *base, u32 offset, u32 value)
+{
+	writel(value, base + offset);
+}
+
+static void vsi_iommu_flush_tlb_all(struct iommu_domain *domain)
+{
+	struct vsi_iommu_domain *vsi_domain = to_vsi_domain(domain);
+	struct list_head *pos;
+	unsigned long flags;
+	int i;
+
+	spin_lock_irqsave(&vsi_domain->iommus_lock, flags);
+	list_for_each(pos, &vsi_domain->iommus) {
+		struct vsi_iommu *iommu;
+		int ret;
+
+		iommu = list_entry(pos, struct vsi_iommu, node);
+		ret = pm_runtime_get_if_in_use(iommu->dev);
+		if (WARN_ON_ONCE(ret < 0))
+			continue;
+		if (ret) {
+			WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks));
+			for (i = 0; i < iommu->num_mmu; i++) {
+				writel(VSI_MMU_BIT_FLUSH,
+				       iommu->bases[i] + VSI_MMU_FLUSH_BASE);
+				writel(0, iommu->bases[i] + VSI_MMU_FLUSH_BASE);
+			}
+			clk_bulk_disable(iommu->num_clocks, iommu->clocks);
+			pm_runtime_put(iommu->dev);
+		}
+	}
+	spin_unlock_irqrestore(&vsi_domain->iommus_lock, flags);
+}
+
+static irqreturn_t vsi_iommu_irq(int irq, void *dev_id)
+{
+	struct vsi_iommu *iommu = dev_id;
+	u32 int_status;
+	dma_addr_t iova;
+	irqreturn_t ret = IRQ_NONE;
+	int i, err;
+
+	err = pm_runtime_get_if_in_use(iommu->dev);
+	if (!err || WARN_ON_ONCE(err < 0))
+		return ret;
+
+	if (WARN_ON(clk_bulk_enable(iommu->num_clocks, iommu->clocks)))
+		goto out;
+
+	for (i = 0; i < iommu->num_mmu; i++) {
+		int_status = vsi_iommu_read(iommu->bases[i], VSI_MMU_STATUS_BASE);
+		if (int_status & VSI_MMU_IRQ_MASK) {
+			dev_err(iommu->dev, "unexpected int_status=%08x\n", int_status);
+			iova = vsi_iommu_read(iommu->bases[i], VSI_MMU_PAGE_FAULT_ADDR);
+
+			if (iommu->domain)
+				report_iommu_fault(iommu->domain, iommu->dev, iova, int_status);
+			else
+				dev_err(iommu->dev,
+					"Page fault while iommu not attached to domain?\n");
+		}
+		vsi_iommu_write(iommu->bases[i], VSI_MMU_STATUS_BASE, 0);
+		ret = IRQ_HANDLED;
+	}
+
+	clk_bulk_disable(iommu->num_clocks, iommu->clocks);
+
+out:
+	pm_runtime_put(iommu->dev);
+	return ret;
+}
+
+static struct iommu_domain *vsi_iommu_domain_alloc_paging(struct device *dev)
+{
+	struct vsi_iommu_domain *vsi_domain;
+
+	if (!dma_dev)
+		return NULL;
+
+	vsi_domain = kzalloc(sizeof(*vsi_domain), GFP_KERNEL);
+	if (!vsi_domain)
+		return NULL;
+
+	/*
+	 * iommu use a 2 level pagetable.
+	 * Each level1 (dt) and level2 (pt) table has 1024 4-byte entries.
+	 * Allocate one 4 KiB page for each table.
+	 */
+	vsi_domain->dt = iommu_alloc_pages_sz(GFP_KERNEL | GFP_DMA32,
+					      SPAGE_SIZE);
+	if (!vsi_domain->dt)
+		goto err_free_domain;
+
+	vsi_domain->dt_dma = dma_map_single(dma_dev, vsi_domain->dt,
+					    SPAGE_SIZE, DMA_TO_DEVICE);
+	if (dma_mapping_error(dma_dev, vsi_domain->dt_dma)) {
+		dev_err(dma_dev, "DMA map error for DT\n");
+		goto err_free_dt;
+	}
+
+	vsi_domain->pta = iommu_alloc_pages_sz(GFP_KERNEL | GFP_DMA32,
+					       SPAGE_SIZE);
+	if (!vsi_domain->pta)
+		goto err_unmap_dt;
+
+	vsi_domain->pta_dma = dma_map_single(dma_dev, vsi_domain->pta,
+					     SPAGE_SIZE, DMA_TO_DEVICE);
+	if (dma_mapping_error(dma_dev, vsi_domain->pta_dma)) {
+		dev_err(dma_dev, "DMA map error for PTA\n");
+		goto err_free_pta;
+	}
+	vsi_domain->pta[0] = vsi_mk_pta(vsi_domain->dt_dma);
+
+	vsi_table_flush(vsi_domain, vsi_domain->pta_dma, 1024);
+	vsi_table_flush(vsi_domain, vsi_domain->dt_dma, NUM_DT_ENTRIES);
+
+	spin_lock_init(&vsi_domain->iommus_lock);
+	spin_lock_init(&vsi_domain->dt_lock);
+	INIT_LIST_HEAD(&vsi_domain->iommus);
+
+	vsi_domain->domain.geometry.aperture_start = 0;
+	vsi_domain->domain.geometry.aperture_end   = DMA_BIT_MASK(32);
+	vsi_domain->domain.geometry.force_aperture = true;
+
+	return &vsi_domain->domain;
+
+err_free_pta:
+	iommu_free_pages(vsi_domain->pta);
+err_unmap_dt:
+	dma_unmap_single(dma_dev, vsi_domain->dt_dma,
+			 SPAGE_SIZE, DMA_TO_DEVICE);
+err_free_dt:
+	iommu_free_pages(vsi_domain->dt);
+err_free_domain:
+	kfree(vsi_domain);
+
+	return NULL;
+}
+
+static phys_addr_t vsi_iommu_iova_to_phys(struct iommu_domain *domain,
+					  dma_addr_t iova)
+{
+	struct vsi_iommu_domain *vsi_domain = to_vsi_domain(domain);
+	unsigned long flags;
+	phys_addr_t pt_phys, phys = 0;
+	u32 dte, pte;
+	u32 *page_table;
+
+	spin_lock_irqsave(&vsi_domain->dt_lock, flags);
+
+	dte = vsi_domain->dt[vsi_iova_dte_index(iova)];
+	if (!vsi_dte_is_pt_valid(dte))
+		goto out;
+
+	pt_phys = vsi_dte_pt_address(dte);
+	page_table = (u32 *)phys_to_virt(pt_phys);
+	pte = page_table[vsi_iova_pte_index(iova)];
+	if (!vsi_pte_is_page_valid(pte))
+		goto out;
+
+	phys = vsi_pte_page_address(pte) + vsi_iova_page_offset(iova);
+out:
+	spin_unlock_irqrestore(&vsi_domain->dt_lock, flags);
+
+	return phys;
+}
+
+static u32 *vsi_dte_get_page_table(struct vsi_iommu_domain *vsi_domain, dma_addr_t iova)
+{
+	u32 *page_table, *dte_addr;
+	u32 dte_index, dte;
+	phys_addr_t pt_phys;
+	dma_addr_t pt_dma;
+
+	assert_spin_locked(&vsi_domain->dt_lock);
+
+	dte_index = vsi_iova_dte_index(iova);
+	dte_addr = &vsi_domain->dt[dte_index];
+	dte = *dte_addr;
+	if (vsi_dte_is_pt_valid(dte))
+		goto done;
+
+	page_table = (u32 *)get_zeroed_page(GFP_ATOMIC | GFP_DMA32);
+	if (!page_table)
+		return ERR_PTR(-ENOMEM);
+
+	pt_dma = dma_map_single(dma_dev, page_table, SPAGE_SIZE, DMA_TO_DEVICE);
+	if (dma_mapping_error(dma_dev, pt_dma)) {
+		dev_err(dma_dev, "DMA mapping error while allocating page table\n");
+		free_page((unsigned long)page_table);
+		return ERR_PTR(-ENOMEM);
+	}
+
+	dte = vsi_mk_dte(pt_dma);
+	*dte_addr = dte;
+
+	vsi_table_flush(vsi_domain, pt_dma, NUM_PT_ENTRIES);
+	vsi_table_flush(vsi_domain,
+			vsi_domain->dt_dma + dte_index * sizeof(u32), 1);
+done:
+	pt_phys = vsi_dte_pt_address(dte);
+	return (u32 *)phys_to_virt(pt_phys);
+}
+
+static size_t vsi_iommu_unmap_iova(struct vsi_iommu_domain *vsi_domain,
+				   u32 *pte_addr, dma_addr_t pte_dma,
+				   size_t size)
+{
+	unsigned int pte_count;
+	unsigned int pte_total = size / SPAGE_SIZE;
+
+	assert_spin_locked(&vsi_domain->dt_lock);
+
+	for (pte_count = 0; pte_count < pte_total; pte_count++) {
+		u32 pte = pte_addr[pte_count];
+
+		if (!vsi_pte_is_page_valid(pte))
+			break;
+
+		pte_addr[pte_count] = vsi_mk_pte_invalid(pte);
+	}
+
+	vsi_table_flush(vsi_domain, pte_dma, pte_count);
+
+	return pte_count * SPAGE_SIZE;
+}
+
+static int vsi_iommu_map_iova(struct vsi_iommu_domain *vsi_domain, u32 *pte_addr,
+			      dma_addr_t pte_dma, dma_addr_t iova,
+			      phys_addr_t paddr, size_t size, int prot)
+{
+	unsigned int pte_count;
+	unsigned int pte_total = size / SPAGE_SIZE;
+	phys_addr_t page_phys;
+
+	assert_spin_locked(&vsi_domain->dt_lock);
+
+	for (pte_count = 0; pte_count < pte_total; pte_count++) {
+		u32 pte = pte_addr[pte_count];
+
+		if (vsi_pte_is_page_valid(pte))
+			goto unwind;
+
+		pte_addr[pte_count] = vsi_mk_pte(paddr, prot);
+
+		paddr += SPAGE_SIZE;
+	}
+
+	vsi_table_flush(vsi_domain, pte_dma, pte_total);
+
+	return 0;
+unwind:
+	/* Unmap the range of iovas that we just mapped */
+	vsi_iommu_unmap_iova(vsi_domain, pte_addr, pte_dma,
+			     pte_count * SPAGE_SIZE);
+
+	iova += pte_count * SPAGE_SIZE;
+	page_phys = vsi_pte_page_address(pte_addr[pte_count]);
+	pr_err("iova: %pad already mapped to %pa cannot remap to phys: %pa prot: %#x\n",
+	       &iova, &page_phys, &paddr, prot);
+
+	return -EADDRINUSE;
+}
+
+static size_t vsi_iommu_unmap(struct iommu_domain *domain, unsigned long _iova,
+			      size_t size, size_t count, struct iommu_iotlb_gather *gather)
+{
+	struct vsi_iommu_domain *vsi_domain = to_vsi_domain(domain);
+	unsigned long flags;
+	dma_addr_t pte_dma, iova = (dma_addr_t)_iova;
+	phys_addr_t pt_phys;
+	u32 dte;
+	u32 *pte_addr;
+	size_t unmap_size;
+
+	spin_lock_irqsave(&vsi_domain->dt_lock, flags);
+
+	dte = vsi_domain->dt[vsi_iova_dte_index(iova)];
+	/* Just return 0 if iova is unmapped */
+	if (!vsi_dte_is_pt_valid(dte)) {
+		spin_unlock_irqrestore(&vsi_domain->dt_lock, flags);
+		return 0;
+	}
+
+	pt_phys = vsi_dte_pt_address(dte);
+	pte_addr = (u32 *)phys_to_virt(pt_phys) + vsi_iova_pte_index(iova);
+	pte_dma = pt_phys + vsi_iova_pte_index(iova) * sizeof(u32);
+	unmap_size = vsi_iommu_unmap_iova(vsi_domain, pte_addr, pte_dma, size);
+
+	spin_unlock_irqrestore(&vsi_domain->dt_lock, flags);
+
+	return unmap_size;
+}
+
+static int vsi_iommu_map(struct iommu_domain *domain, unsigned long _iova,
+			 phys_addr_t paddr, size_t size, size_t count,
+			 int prot, gfp_t gfp, size_t *mapped)
+{
+	struct vsi_iommu_domain *vsi_domain = to_vsi_domain(domain);
+	unsigned long flags;
+	dma_addr_t pte_dma, iova = (dma_addr_t)_iova;
+	u32 *page_table, *pte_addr;
+	u32 dte, pte_index;
+	int ret;
+
+	spin_lock_irqsave(&vsi_domain->dt_lock, flags);
+
+	page_table = vsi_dte_get_page_table(vsi_domain, iova);
+	if (IS_ERR(page_table)) {
+		spin_unlock_irqrestore(&vsi_domain->dt_lock, flags);
+		return PTR_ERR(page_table);
+	}
+
+	dte = vsi_domain->dt[vsi_iova_dte_index(iova)];
+	pte_index = vsi_iova_pte_index(iova);
+	pte_addr = &page_table[pte_index];
+	pte_dma = vsi_dte_pt_address(dte) + pte_index * sizeof(u32);
+	ret = vsi_iommu_map_iova(vsi_domain, pte_addr, pte_dma, iova,
+				 paddr, size, prot);
+
+	spin_unlock_irqrestore(&vsi_domain->dt_lock, flags);
+	if (!ret)
+		*mapped = size;
+
+	return ret;
+}
+
+static void vsi_iommu_detach_device(struct iommu_domain *domain,
+				    struct device *dev)
+{
+	struct vsi_iommu *iommu;
+	struct vsi_iommu_domain *vsi_domain = to_vsi_domain(domain);
+	unsigned long flags;
+	int ret;
+
+	/* Allow 'virtual devices' (eg drm) to detach from domain */
+	iommu = vsi_iommu_from_dev(dev);
+	if (WARN_ON(!iommu))
+		return;
+
+	dev_dbg(dev, "Detaching from iommu domain\n");
+
+	if (!iommu->domain)
+		return;
+
+	spin_lock_irqsave(&vsi_domain->iommus_lock, flags);
+	list_del_init(&iommu->node);
+	spin_unlock_irqrestore(&vsi_domain->iommus_lock, flags);
+
+	ret = pm_runtime_get_if_in_use(iommu->dev);
+	WARN_ON_ONCE(ret < 0);
+	if (ret > 0) {
+		vsi_iommu_disable(iommu);
+		pm_runtime_put(iommu->dev);
+	}
+	iommu->domain = NULL;
+}
+
+static int vsi_iommu_attach_device(struct iommu_domain *domain,
+				   struct device *dev)
+{
+	struct vsi_iommu *iommu;
+	struct vsi_iommu_domain *vsi_domain = to_vsi_domain(domain);
+	unsigned long flags;
+	int ret;
+
+	iommu = vsi_iommu_from_dev(dev);
+	if (WARN_ON(!iommu))
+		return -ENODEV;
+
+	if (iommu->domain == domain)
+		return 0;
+
+	iommu->domain = domain;
+
+	spin_lock_irqsave(&vsi_domain->iommus_lock, flags);
+	list_add_tail(&iommu->node, &vsi_domain->iommus);
+	spin_unlock_irqrestore(&vsi_domain->iommus_lock, flags);
+
+	ret = pm_runtime_get_if_in_use(iommu->dev);
+	if (!ret || WARN_ON_ONCE(ret < 0))
+		return 0;
+
+	ret = vsi_iommu_enable(iommu);
+	if (ret)
+		vsi_iommu_detach_device(iommu->domain, dev);
+
+	pm_runtime_put(iommu->dev);
+
+	return ret;
+}
+
+static void vsi_iommu_domain_free(struct iommu_domain *domain)
+{
+	struct vsi_iommu_domain *vsi_domain = to_vsi_domain(domain);
+	int i;
+
+	WARN_ON(!list_empty(&vsi_domain->iommus));
+
+	for (i = 0; i < NUM_DT_ENTRIES; i++) {
+		u32 dte = vsi_domain->dt[i];
+
+		if (vsi_dte_is_pt_valid(dte)) {
+			phys_addr_t pt_phys = vsi_dte_pt_address(dte);
+			u32 *page_table = phys_to_virt(pt_phys);
+
+			dma_unmap_single(dma_dev, pt_phys,
+					 SPAGE_SIZE, DMA_TO_DEVICE);
+			iommu_free_pages(page_table);
+		}
+	}
+
+	dma_unmap_single(dma_dev, vsi_domain->dt_dma,
+			 SPAGE_SIZE, DMA_TO_DEVICE);
+	iommu_free_pages(vsi_domain->dt);
+
+	dma_unmap_single(dma_dev, vsi_domain->pta_dma,
+			 SPAGE_SIZE, DMA_TO_DEVICE);
+	iommu_free_pages(vsi_domain->pta);
+
+	kfree(vsi_domain);
+}
+
+static struct iommu_device *vsi_iommu_probe_device(struct device *dev)
+{
+	struct vsi_iommudata *data;
+	struct vsi_iommu *iommu;
+
+	data = dev_iommu_priv_get(dev);
+	if (!data)
+		return ERR_PTR(-ENODEV);
+
+	iommu = vsi_iommu_from_dev(dev);
+
+	pr_info("%s,%d, consumer : %s, supplier : %s\n",
+		__func__, __LINE__, dev_name(dev), dev_name(iommu->dev));
+
+	/*
+	 * link will free by platform_device_del(master) via
+	 * BUS_NOTIFY_REMOVED_DEVICE
+	 */
+	data->link = device_link_add(dev, iommu->dev,
+				     DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME);
+
+	/* set max segment size for dev, needed for single chunk map */
+	if (!dev->dma_parms)
+		dev->dma_parms = kzalloc(sizeof(*dev->dma_parms), GFP_KERNEL);
+	if (!dev->dma_parms)
+		return ERR_PTR(-ENOMEM);
+
+	dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
+
+	return &iommu->iommu;
+}
+
+static void vsi_iommu_release_device(struct device *dev)
+{
+	struct vsi_iommudata *data = dev_iommu_priv_get(dev);
+
+	device_link_del(data->link);
+}
+
+static int vsi_iommu_of_xlate(struct device *dev,
+			      const struct of_phandle_args *args)
+{
+	struct platform_device *iommu_dev;
+	struct vsi_iommudata *data;
+
+	data = devm_kzalloc(dma_dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	dev_info(dev, "%s,%d\n", __func__, __LINE__);
+	iommu_dev = of_find_device_by_node(args->np);
+
+	data->iommu = platform_get_drvdata(iommu_dev);
+
+	dev_iommu_priv_set(dev, data);
+
+	platform_device_put(iommu_dev);
+
+	return 0;
+}
+
+static struct iommu_ops vsi_iommu_ops = {
+	.domain_alloc_paging = vsi_iommu_domain_alloc_paging,
+	.probe_device = vsi_iommu_probe_device,
+	.release_device = vsi_iommu_release_device,
+	.device_group = generic_single_device_group,
+	.pgsize_bitmap = VSI_IOMMU_PGSIZE_BITMAP,
+	.of_xlate = vsi_iommu_of_xlate,
+	.default_domain_ops = &(const struct iommu_domain_ops) {
+		.attach_dev		= vsi_iommu_attach_device,
+		.map_pages		= vsi_iommu_map,
+		.unmap_pages		= vsi_iommu_unmap,
+		.flush_iotlb_all	= vsi_iommu_flush_tlb_all,
+		.iova_to_phys		= vsi_iommu_iova_to_phys,
+		.free			= vsi_iommu_domain_free,
+	}
+};
+
+static const struct of_device_id vsi_iommu_dt_ids[] = {
+	{
+		.compatible = "verisilicon,iommu",
+	},
+	{ /* sentinel */ }
+};
+
+static int vsi_iommu_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct vsi_iommu *iommu;
+	struct resource *res;
+	int num_res = pdev->num_resources;
+	int err, i;
+
+	iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
+	if (!iommu)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, iommu);
+	iommu->dev = dev;
+	iommu->num_mmu = 0;
+
+	iommu->bases = devm_kcalloc(dev, num_res, sizeof(*iommu->bases),
+				    GFP_KERNEL);
+	if (!iommu->bases)
+		return -ENOMEM;
+
+	for (i = 0; i < num_res; i++) {
+		res = platform_get_resource(pdev, IORESOURCE_MEM, i);
+		if (!res)
+			continue;
+		iommu->bases[i] = devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(iommu->bases[i]))
+			continue;
+		iommu->num_mmu++;
+	}
+	if (iommu->num_mmu == 0)
+		return PTR_ERR(iommu->bases[0]);
+
+	iommu->num_irq = platform_irq_count(pdev);
+	if (iommu->num_irq < 0)
+		return iommu->num_irq;
+
+	err = devm_clk_bulk_get_all(dev, &iommu->clocks);
+	if (err >= 0)
+		iommu->num_clocks = err;
+	else if (err == -ENOENT)
+		iommu->num_clocks = 0;
+	else
+		return err;
+
+	err = clk_bulk_prepare(iommu->num_clocks, iommu->clocks);
+	if (err)
+		return err;
+
+	iommu->group = iommu_group_alloc();
+	if (IS_ERR(iommu->group)) {
+		err = PTR_ERR(iommu->group);
+		goto err_unprepare_clocks;
+	}
+
+	/*
+	 * use the first registered sysmmu device for performing
+	 * dma mapping operations on iommu page tables (cpu cache flush)
+	 */
+	if (!dma_dev)
+		dma_dev = &pdev->dev;
+
+	pm_runtime_enable(dev);
+
+	err = iommu_device_sysfs_add(&iommu->iommu, dev, NULL, dev_name(dev));
+	if (err)
+		goto err_put_group;
+
+	err = iommu_device_register(&iommu->iommu, &vsi_iommu_ops, dev);
+	if (err)
+		goto err_remove_sysfs;
+
+	for (i = 0; i < iommu->num_irq; i++) {
+		int irq = platform_get_irq(pdev, i);
+
+		if (irq < 0)
+			return irq;
+
+		err = devm_request_irq(iommu->dev, irq, vsi_iommu_irq,
+				       IRQF_SHARED, dev_name(dev), iommu);
+		if (err)
+			goto err_unregister;
+	}
+
+	return 0;
+err_unregister:
+	iommu_device_unregister(&iommu->iommu);
+err_remove_sysfs:
+	iommu_device_sysfs_remove(&iommu->iommu);
+err_put_group:
+	iommu_group_put(iommu->group);
+	pm_runtime_disable(dev);
+err_unprepare_clocks:
+	clk_bulk_unprepare(iommu->num_clocks, iommu->clocks);
+	return err;
+}
+
+static void vsi_iommu_shutdown(struct platform_device *pdev)
+{
+	struct vsi_iommu *iommu = platform_get_drvdata(pdev);
+	int i;
+
+	for (i = 0; i < iommu->num_irq; i++) {
+		int irq = platform_get_irq(pdev, i);
+
+		devm_free_irq(iommu->dev, irq, iommu);
+	}
+
+	pm_runtime_force_suspend(&pdev->dev);
+}
+
+static int __maybe_unused vsi_iommu_suspend(struct device *dev)
+{
+	struct vsi_iommu *iommu = dev_get_drvdata(dev);
+
+	if (!iommu->domain)
+		return 0;
+
+	vsi_iommu_disable(iommu);
+	return 0;
+}
+
+static int __maybe_unused vsi_iommu_resume(struct device *dev)
+{
+	struct vsi_iommu *iommu = dev_get_drvdata(dev);
+
+	if (!iommu->domain)
+		return 0;
+
+	return vsi_iommu_enable(iommu);
+}
+
+static const struct dev_pm_ops vsi_iommu_pm_ops = {
+	SET_RUNTIME_PM_OPS(vsi_iommu_suspend, vsi_iommu_resume, NULL)
+	SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend,
+				pm_runtime_force_resume)
+};
+
+struct platform_driver rockchip_vsi_iommu_driver = {
+	.probe = vsi_iommu_probe,
+	.shutdown = vsi_iommu_shutdown,
+	.driver = {
+		   .name = "vsi_iommu",
+		   .of_match_table = vsi_iommu_dt_ids,
+		   .pm = &vsi_iommu_pm_ops,
+		   .suppress_bind_attrs = true,
+	},
+};
+builtin_platform_driver(rockchip_vsi_iommu_driver);
-- 
2.43.0

[PATCH 4/5] arm64: dts: rockchip: Add verisilicon IOMMU node on RK3588

[Benjamin Gaignard]

Add the device tree node for the Verisilicon IOMMU present in the RK3588 SoC.
This IOMMU handles address translation for the VPU hardware blocks.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
 arch/arm64/boot/dts/rockchip/rk3588-base.dtsi | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
index 70f03e68ba55..20af8d461c69 100644
--- a/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
+++ b/arch/arm64/boot/dts/rockchip/rk3588-base.dtsi
@@ -1263,6 +1263,17 @@ av1d: video-codec@fdc70000 {
 		clock-names = "aclk", "hclk";
 		power-domains = <&power RK3588_PD_AV1>;
 		resets = <&cru SRST_A_AV1>, <&cru SRST_P_AV1>, <&cru SRST_A_AV1_BIU>, <&cru SRST_P_AV1_BIU>;
+		iommus = <&av1d_mmu>;
+	};
+
+	av1d_mmu: iommu@fdca0000 {
+		compatible = "verisilicon,iommu";
+		reg = <0x0 0xfdca0000 0x0 0x600>;
+		interrupts = <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH 0>;
+		clocks = <&cru ACLK_AV1>, <&cru PCLK_AV1>;
+		clock-names = "aclk", "iface";
+		#iommu-cells = <0>;
+		power-domains = <&power RK3588_PD_AV1>;
 	};
 
 	vop: vop@fdd90000 {
-- 
2.43.0

[PATCH 5/5] media: verisilicon: Flush IOMMU before decoding a frame

[Benjamin Gaignard]

Flush the IOMMU mapping before decoding a frame to ensure that all memory
translations are properly applied.

Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
---
 drivers/media/platform/verisilicon/hantro_drv.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c
index 8542238e0fb1..5a672467d83f 100644
--- a/drivers/media/platform/verisilicon/hantro_drv.c
+++ b/drivers/media/platform/verisilicon/hantro_drv.c
@@ -11,6 +11,9 @@
  */
 
 #include <linux/clk.h>
+#include <linux/dma-map-ops.h>
+#include <linux/iommu.h>
+#include <linux/iommu-dma.h>
 #include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
@@ -186,6 +189,14 @@ static void device_run(void *priv)
 
 	v4l2_m2m_buf_copy_metadata(src, dst, true);
 
+	if (use_dma_iommu(ctx->dev->v4l2_dev.dev)) {
+		struct iommu_domain *mmu_dom;
+
+		mmu_dom = iommu_get_domain_for_dev(ctx->dev->v4l2_dev.dev);
+		if (mmu_dom)
+			iommu_flush_iotlb_all(mmu_dom);
+	}
+
 	if (ctx->codec_ops->run(ctx))
 		goto err_cancel_job;
 
-- 
2.43.0

Re: [PATCH 2/5] dt-bindings: iommu: verisilicon: Add binding for VSI IOMMU

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2921 bytes --]

On Mon, Jun 16, 2025 at 04:55:50PM +0200, Benjamin Gaignard wrote:
> Add a device tree binding for the Verisilicon (VSI) IOMMU. This IOMMU sits
> in front of hardware encoder and decoder blocks on SoCs using Verisilicon IP,
> such as the Rockchip RK3588.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
>  .../bindings/iommu/verisilicon,iommu.yaml     | 71 +++++++++++++++++++
>  1 file changed, 71 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml
> 
> diff --git a/Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml b/Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml
> new file mode 100644
> index 000000000000..acef855fc61d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml
> @@ -0,0 +1,71 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/iommu/verisilicon,iommu.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Verisilicon IOMMU
> +
> +maintainers:
> +  - Benjamin Gaignard <benjamin.gaignard@collabora.com>
> +
> +description: |+
> +  A Versilicon iommu translates io virtual addresses to physical addresses for
> +  its associated video decoder.
> +
> +properties:
> +  compatible:
> +    oneOf:
> +      - items:
> +          - const: verisilicon,iommu

You're missing a soc-specific compatible at the very least here, but is
there really no versioning on the IP at all? I'd be surprised if
verisilicon only produced exactly one version of an iommu IP.

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    items:
> +      - description: Core clock
> +      - description: Interface clock
> +
> +  clock-names:
> +    items:
> +      - const: aclk
> +      - const: iface

Why "aclk" rather than core, to match the description?

> +
> +  "#iommu-cells":
> +    const: 0
> +
> +  power-domains:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - clock-names
> +  - "#iommu-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/clock/rockchip,rk3588-cru.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    bus {
> +      #address-cells = <2>;
> +      #size-cells = <2>;
> +
> +      vsi_mmu: iommu@fdca0000 {

The "vsi_mmu" label can be dropped here, it has no users.

Cheers,
Conor.

> +        compatible = "verisilicon,iommu";
> +        reg = <0x0 0xfdca0000 0x0 0x600>;
> +        interrupts = <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH 0>;
> +        clocks = <&cru ACLK_AV1>, <&cru PCLK_AV1>;
> +        clock-names = "aclk", "iface";
> +        #iommu-cells = <0>;
> +      };
> +    };
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 1/5] dt-bindings: vendor-prefixes: Add Verisilicon

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1122 bytes --]

On Mon, Jun 16, 2025 at 04:55:49PM +0200, Benjamin Gaignard wrote:
> Verisilicon Microelectronics is a company based in Shanghai, China,
> developping hardware blocks for SoC.
> 
> https://verisilicon.com/
> 
> Add their name to the list of vendors.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>

Acked-by: Conor Dooley <conor.dooley@microchip.com>

> ---
>  Documentation/devicetree/bindings/vendor-prefixes.yaml | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/vendor-prefixes.yaml b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> index 5d2a7a8d3ac6..1baf8304c9ac 100644
> --- a/Documentation/devicetree/bindings/vendor-prefixes.yaml
> +++ b/Documentation/devicetree/bindings/vendor-prefixes.yaml
> @@ -1635,6 +1635,8 @@ patternProperties:
>      description: Variscite Ltd.
>    "^vdl,.*":
>      description: Van der Laan b.v.
> +  "^verisilicon,.*":
> +    description: VeriSilicon Microelectronics
>    "^vertexcom,.*":
>      description: Vertexcom Technologies, Inc.
>    "^via,.*":
> -- 
> 2.43.0
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/5] dt-bindings: iommu: verisilicon: Add binding for VSI IOMMU

[Benjamin Gaignard]

Le 16/06/2025 à 17:14, Conor Dooley a écrit :
> On Mon, Jun 16, 2025 at 04:55:50PM +0200, Benjamin Gaignard wrote:
>> Add a device tree binding for the Verisilicon (VSI) IOMMU. This IOMMU sits
>> in front of hardware encoder and decoder blocks on SoCs using Verisilicon IP,
>> such as the Rockchip RK3588.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>>   .../bindings/iommu/verisilicon,iommu.yaml     | 71 +++++++++++++++++++
>>   1 file changed, 71 insertions(+)
>>   create mode 100644 Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml
>>
>> diff --git a/Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml b/Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml
>> new file mode 100644
>> index 000000000000..acef855fc61d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml
>> @@ -0,0 +1,71 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/iommu/verisilicon,iommu.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Verisilicon IOMMU
>> +
>> +maintainers:
>> +  - Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> +
>> +description: |+
>> +  A Versilicon iommu translates io virtual addresses to physical addresses for
>> +  its associated video decoder.
>> +
>> +properties:
>> +  compatible:
>> +    oneOf:
>> +      - items:
>> +          - const: verisilicon,iommu
> You're missing a soc-specific compatible at the very least here, but is
> there really no versioning on the IP at all? I'd be surprised if
> verisilicon only produced exactly one version of an iommu IP.

I only aware this version of the iommu for the moment.
Does adding verisilicon,rk3588-iommu sound good for you ?

>
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    items:
>> +      - description: Core clock
>> +      - description: Interface clock
>> +
>> +  clock-names:
>> +    items:
>> +      - const: aclk
>> +      - const: iface
> Why "aclk" rather than core, to match the description?

I will change that, the driver doesn't care of the clock name anyway

>
>> +
>> +  "#iommu-cells":
>> +    const: 0
>> +
>> +  power-domains:
>> +    maxItems: 1
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +  - clock-names
>> +  - "#iommu-cells"
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/clock/rockchip,rk3588-cru.h>
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +
>> +    bus {
>> +      #address-cells = <2>;
>> +      #size-cells = <2>;
>> +
>> +      vsi_mmu: iommu@fdca0000 {
> The "vsi_mmu" label can be dropped here, it has no users.

ok.

Thanks,
Benjamin

>
> Cheers,
> Conor.
>
>> +        compatible = "verisilicon,iommu";
>> +        reg = <0x0 0xfdca0000 0x0 0x600>;
>> +        interrupts = <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH 0>;
>> +        clocks = <&cru ACLK_AV1>, <&cru PCLK_AV1>;
>> +        clock-names = "aclk", "iface";
>> +        #iommu-cells = <0>;
>> +      };
>> +    };
>> -- 
>> 2.43.0
>>

Re: [PATCH 2/5] dt-bindings: iommu: verisilicon: Add binding for VSI IOMMU

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2197 bytes --]

On Mon, Jun 16, 2025 at 05:30:44PM +0200, Benjamin Gaignard wrote:
> 
> Le 16/06/2025 à 17:14, Conor Dooley a écrit :
> > On Mon, Jun 16, 2025 at 04:55:50PM +0200, Benjamin Gaignard wrote:
> > > Add a device tree binding for the Verisilicon (VSI) IOMMU. This IOMMU sits
> > > in front of hardware encoder and decoder blocks on SoCs using Verisilicon IP,
> > > such as the Rockchip RK3588.
> > > 
> > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > > ---
> > >   .../bindings/iommu/verisilicon,iommu.yaml     | 71 +++++++++++++++++++
> > >   1 file changed, 71 insertions(+)
> > >   create mode 100644 Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml
> > > 
> > > diff --git a/Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml b/Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml
> > > new file mode 100644
> > > index 000000000000..acef855fc61d
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml
> > > @@ -0,0 +1,71 @@
> > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > +%YAML 1.2
> > > +---
> > > +$id: http://devicetree.org/schemas/iommu/verisilicon,iommu.yaml#
> > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > +
> > > +title: Verisilicon IOMMU
> > > +
> > > +maintainers:
> > > +  - Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > > +
> > > +description: |+
> > > +  A Versilicon iommu translates io virtual addresses to physical addresses for
> > > +  its associated video decoder.
> > > +
> > > +properties:
> > > +  compatible:
> > > +    oneOf:
> > > +      - items:
> > > +          - const: verisilicon,iommu
> > You're missing a soc-specific compatible at the very least here, but is
> > there really no versioning on the IP at all? I'd be surprised if
> > verisilicon only produced exactly one version of an iommu IP.
> 
> I only aware this version of the iommu for the moment.

"for the moment", yeah. Is there any information that could be used to
version this available?

> Does adding verisilicon,rk3588-iommu sound good for you ?

It'd be "rockchip,rk3588-iommu", but sure.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/5] dt-bindings: iommu: verisilicon: Add binding for VSI IOMMU

[Benjamin Gaignard]

Le 16/06/2025 à 17:42, Conor Dooley a écrit :
> On Mon, Jun 16, 2025 at 05:30:44PM +0200, Benjamin Gaignard wrote:
>> Le 16/06/2025 à 17:14, Conor Dooley a écrit :
>>> On Mon, Jun 16, 2025 at 04:55:50PM +0200, Benjamin Gaignard wrote:
>>>> Add a device tree binding for the Verisilicon (VSI) IOMMU. This IOMMU sits
>>>> in front of hardware encoder and decoder blocks on SoCs using Verisilicon IP,
>>>> such as the Rockchip RK3588.
>>>>
>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>>> ---
>>>>    .../bindings/iommu/verisilicon,iommu.yaml     | 71 +++++++++++++++++++
>>>>    1 file changed, 71 insertions(+)
>>>>    create mode 100644 Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml b/Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml
>>>> new file mode 100644
>>>> index 000000000000..acef855fc61d
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml
>>>> @@ -0,0 +1,71 @@
>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>> +%YAML 1.2
>>>> +---
>>>> +$id: http://devicetree.org/schemas/iommu/verisilicon,iommu.yaml#
>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>> +
>>>> +title: Verisilicon IOMMU
>>>> +
>>>> +maintainers:
>>>> +  - Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>>> +
>>>> +description: |+
>>>> +  A Versilicon iommu translates io virtual addresses to physical addresses for
>>>> +  its associated video decoder.
>>>> +
>>>> +properties:
>>>> +  compatible:
>>>> +    oneOf:
>>>> +      - items:
>>>> +          - const: verisilicon,iommu
>>> You're missing a soc-specific compatible at the very least here, but is
>>> there really no versioning on the IP at all? I'd be surprised if
>>> verisilicon only produced exactly one version of an iommu IP.
>> I only aware this version of the iommu for the moment.
> "for the moment", yeah. Is there any information that could be used to
> version this available?

The hardware block isn't documented in the TRM so I don't know if there is a version
field or something like that.

>
>> Does adding verisilicon,rk3588-iommu sound good for you ?
> It'd be "rockchip,rk3588-iommu", but sure.

"rockchip,rk3588-iommu" is already use for other MMUs in rk3588.

Re: [PATCH 2/5] dt-bindings: iommu: verisilicon: Add binding for VSI IOMMU

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2897 bytes --]

On Mon, Jun 16, 2025 at 05:50:50PM +0200, Benjamin Gaignard wrote:
> 
> Le 16/06/2025 à 17:42, Conor Dooley a écrit :
> > On Mon, Jun 16, 2025 at 05:30:44PM +0200, Benjamin Gaignard wrote:
> > > Le 16/06/2025 à 17:14, Conor Dooley a écrit :
> > > > On Mon, Jun 16, 2025 at 04:55:50PM +0200, Benjamin Gaignard wrote:
> > > > > Add a device tree binding for the Verisilicon (VSI) IOMMU. This IOMMU sits
> > > > > in front of hardware encoder and decoder blocks on SoCs using Verisilicon IP,
> > > > > such as the Rockchip RK3588.
> > > > > 
> > > > > Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > > > > ---
> > > > >    .../bindings/iommu/verisilicon,iommu.yaml     | 71 +++++++++++++++++++
> > > > >    1 file changed, 71 insertions(+)
> > > > >    create mode 100644 Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml
> > > > > 
> > > > > diff --git a/Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml b/Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml
> > > > > new file mode 100644
> > > > > index 000000000000..acef855fc61d
> > > > > --- /dev/null
> > > > > +++ b/Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml
> > > > > @@ -0,0 +1,71 @@
> > > > > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > > > > +%YAML 1.2
> > > > > +---
> > > > > +$id: http://devicetree.org/schemas/iommu/verisilicon,iommu.yaml#
> > > > > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > > > > +
> > > > > +title: Verisilicon IOMMU
> > > > > +
> > > > > +maintainers:
> > > > > +  - Benjamin Gaignard <benjamin.gaignard@collabora.com>
> > > > > +
> > > > > +description: |+
> > > > > +  A Versilicon iommu translates io virtual addresses to physical addresses for
> > > > > +  its associated video decoder.
> > > > > +
> > > > > +properties:
> > > > > +  compatible:
> > > > > +    oneOf:
> > > > > +      - items:
> > > > > +          - const: verisilicon,iommu
> > > > You're missing a soc-specific compatible at the very least here, but is
> > > > there really no versioning on the IP at all? I'd be surprised if
> > > > verisilicon only produced exactly one version of an iommu IP.
> > > I only aware this version of the iommu for the moment.
> > "for the moment", yeah. Is there any information that could be used to
> > version this available?
> 
> The hardware block isn't documented in the TRM so I don't know if there is a version
> field or something like that.
> 
> > 
> > > Does adding verisilicon,rk3588-iommu sound good for you ?
> > It'd be "rockchip,rk3588-iommu", but sure.
> 
> "rockchip,rk3588-iommu" is already use for other MMUs in rk3588.

"rockchip,rk3588-video-iommu" then? Instances of an IP in an SoC get a
specific compatible with the SoC vendor's prefix, so having verisilicon
there isn't suitable unless they made the SoC.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 2/5] dt-bindings: iommu: verisilicon: Add binding for VSI IOMMU

[Benjamin Gaignard]

Le 16/06/2025 à 17:58, Conor Dooley a écrit :
> On Mon, Jun 16, 2025 at 05:50:50PM +0200, Benjamin Gaignard wrote:
>> Le 16/06/2025 à 17:42, Conor Dooley a écrit :
>>> On Mon, Jun 16, 2025 at 05:30:44PM +0200, Benjamin Gaignard wrote:
>>>> Le 16/06/2025 à 17:14, Conor Dooley a écrit :
>>>>> On Mon, Jun 16, 2025 at 04:55:50PM +0200, Benjamin Gaignard wrote:
>>>>>> Add a device tree binding for the Verisilicon (VSI) IOMMU. This IOMMU sits
>>>>>> in front of hardware encoder and decoder blocks on SoCs using Verisilicon IP,
>>>>>> such as the Rockchip RK3588.
>>>>>>
>>>>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>>>>> ---
>>>>>>     .../bindings/iommu/verisilicon,iommu.yaml     | 71 +++++++++++++++++++
>>>>>>     1 file changed, 71 insertions(+)
>>>>>>     create mode 100644 Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml
>>>>>>
>>>>>> diff --git a/Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml b/Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml
>>>>>> new file mode 100644
>>>>>> index 000000000000..acef855fc61d
>>>>>> --- /dev/null
>>>>>> +++ b/Documentation/devicetree/bindings/iommu/verisilicon,iommu.yaml
>>>>>> @@ -0,0 +1,71 @@
>>>>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>>>>> +%YAML 1.2
>>>>>> +---
>>>>>> +$id: http://devicetree.org/schemas/iommu/verisilicon,iommu.yaml#
>>>>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>>>>> +
>>>>>> +title: Verisilicon IOMMU
>>>>>> +
>>>>>> +maintainers:
>>>>>> +  - Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>>>>> +
>>>>>> +description: |+
>>>>>> +  A Versilicon iommu translates io virtual addresses to physical addresses for
>>>>>> +  its associated video decoder.
>>>>>> +
>>>>>> +properties:
>>>>>> +  compatible:
>>>>>> +    oneOf:
>>>>>> +      - items:
>>>>>> +          - const: verisilicon,iommu
>>>>> You're missing a soc-specific compatible at the very least here, but is
>>>>> there really no versioning on the IP at all? I'd be surprised if
>>>>> verisilicon only produced exactly one version of an iommu IP.
>>>> I only aware this version of the iommu for the moment.
>>> "for the moment", yeah. Is there any information that could be used to
>>> version this available?
>> The hardware block isn't documented in the TRM so I don't know if there is a version
>> field or something like that.
>>
>>>> Does adding verisilicon,rk3588-iommu sound good for you ?
>>> It'd be "rockchip,rk3588-iommu", but sure.
>> "rockchip,rk3588-iommu" is already use for other MMUs in rk3588.
> "rockchip,rk3588-video-iommu" then? Instances of an IP in an SoC get a
> specific compatible with the SoC vendor's prefix, so having verisilicon
> there isn't suitable unless they made the SoC.

Other hardware video codecs have a different IOMMU so I will suggest
"rockchip,rk3588-av1-iommu" which is specific to this video hardware block.

Re: [PATCH 2/5] dt-bindings: iommu: verisilicon: Add binding for VSI IOMMU

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 988 bytes --]

Hi,

Le lundi 16 juin 2025 à 16:14 +0100, Conor Dooley a écrit :
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - items:
> > +          - const: verisilicon,iommu
> 
> You're missing a soc-specific compatible at the very least here, but is
> there really no versioning on the IP at all? I'd be surprised if
> verisilicon only produced exactly one version of an iommu IP.

I've dumped the HW ID (base + 6*4), and it reports this IP as an "MM 1.2.0"
(0x4d4d1200).

Note, all VSI IP for which rockchip did not rewrite the register
interface expose a HW ID register, but the from and location can vary.
This one is following the old school H1/G1/G2 style, using ascii to
idenity the core type. Interesting fact too, the register layout seem
to be the same as the Vivante MMU (which is hidden inside the etnaviv
driver).

I'm fine with having a soc specific compatible, just documenting
some fact I could dump.

cheers,
Nicolas

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 195 bytes --]

Re: [PATCH 3/5] iommu: Add verisilicon IOMMU driver

[Diederik de Haas]

[-- Attachment #1: Type: text/plain, Size: 2556 bytes --]

Hi,

On Mon Jun 16, 2025 at 4:55 PM CEST, Benjamin Gaignard wrote:
> Verisilicon IOMMU hardware block can be found in combination

Can there be only 1 hardware block or is multiple possible?
If only 1, then I'd start with "The Verisilicon ...".
If more then one, then I'd use "blocks".

> with hardware video codecs (encoders or decoders) on
> different SoCs.

This makes it sound like it can also be used with non-Verisilicon codecs
and based on the DT binding description, I get the _impression_ that
that is not the case?
But it's actually not clear to me if that's the case or not.

> Enable it will allows to use non contiguous memory

"Enabling it will allow us to use non ..." or "Enable it to allow [the]
use of non ..." or "Enabling allows the use of non ..."

> allocators for Verisilicon video codecs.

And the wrapping of the whole commit message is not following the
standards.

> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
>  drivers/iommu/Kconfig     |   8 +
>  drivers/iommu/Makefile    |   1 +
>  drivers/iommu/vsi-iommu.c | 900 ++++++++++++++++++++++++++++++++++++++
>  3 files changed, 909 insertions(+)
>  create mode 100644 drivers/iommu/vsi-iommu.c
>
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 0a33d995d15d..4cf4504dcc25 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -383,4 +383,12 @@ config SPRD_IOMMU
>  
>  	  Say Y here if you want to use the multimedia devices listed above.
>  
> +config VSI_IOMMU
> +	bool "Verisilicon IOMMU Support"
> +	depends on ARM64
> +	select IOMMU_API
> +	select ARM_DMA_USE_IOMMU
> +	help
> +	  Support for IOMMUs used by Verisilicon video decoders.
> +
>  endif # IOMMU_SUPPORT
> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
> index 355294fa9033..68aeff31af8b 100644
> --- a/drivers/iommu/Makefile
> +++ b/drivers/iommu/Makefile
> @@ -34,3 +34,4 @@ obj-$(CONFIG_IOMMU_SVA) += iommu-sva.o
>  obj-$(CONFIG_IOMMU_IOPF) += io-pgfault.o
>  obj-$(CONFIG_SPRD_IOMMU) += sprd-iommu.o
>  obj-$(CONFIG_APPLE_DART) += apple-dart.o
> +obj-$(CONFIG_VSI_IOMMU) += vsi-iommu.o
> diff --git a/drivers/iommu/vsi-iommu.c b/drivers/iommu/vsi-iommu.c
> new file mode 100644
> index 000000000000..f2fa1197916c
> --- /dev/null
> +++ b/drivers/iommu/vsi-iommu.c
> @@ -0,0 +1,900 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +

No copyright statement? (or an informational header block?)

Cheers,
  Diederik

> +#include <linux/clk.h>
> + <snip>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH 3/5] iommu: Add verisilicon IOMMU driver

[Benjamin Gaignard]

Le 17/06/2025 à 15:04, Diederik de Haas a écrit :
> Hi,
>
> On Mon Jun 16, 2025 at 4:55 PM CEST, Benjamin Gaignard wrote:
>> Verisilicon IOMMU hardware block can be found in combination
> Can there be only 1 hardware block or is multiple possible?
> If only 1, then I'd start with "The Verisilicon ...".
> If more then one, then I'd use "blocks".
>
>> with hardware video codecs (encoders or decoders) on
>> different SoCs.
> This makes it sound like it can also be used with non-Verisilicon codecs
> and based on the DT binding description, I get the _impression_ that
> that is not the case?
> But it's actually not clear to me if that's the case or not.

Only one hardware block exists on rk3588 and it can only be used
by Verisilicon AV1 codec.

>
>> Enable it will allows to use non contiguous memory
> "Enabling it will allow us to use non ..." or "Enable it to allow [the]
> use of non ..." or "Enabling allows the use of non ..."
>
>> allocators for Verisilicon video codecs.
> And the wrapping of the whole commit message is not following the
> standards.
>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>>   drivers/iommu/Kconfig     |   8 +
>>   drivers/iommu/Makefile    |   1 +
>>   drivers/iommu/vsi-iommu.c | 900 ++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 909 insertions(+)
>>   create mode 100644 drivers/iommu/vsi-iommu.c
>>
>> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
>> index 0a33d995d15d..4cf4504dcc25 100644
>> --- a/drivers/iommu/Kconfig
>> +++ b/drivers/iommu/Kconfig
>> @@ -383,4 +383,12 @@ config SPRD_IOMMU
>>   
>>   	  Say Y here if you want to use the multimedia devices listed above.
>>   
>> +config VSI_IOMMU
>> +	bool "Verisilicon IOMMU Support"
>> +	depends on ARM64
>> +	select IOMMU_API
>> +	select ARM_DMA_USE_IOMMU
>> +	help
>> +	  Support for IOMMUs used by Verisilicon video decoders.
>> +
>>   endif # IOMMU_SUPPORT
>> diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
>> index 355294fa9033..68aeff31af8b 100644
>> --- a/drivers/iommu/Makefile
>> +++ b/drivers/iommu/Makefile
>> @@ -34,3 +34,4 @@ obj-$(CONFIG_IOMMU_SVA) += iommu-sva.o
>>   obj-$(CONFIG_IOMMU_IOPF) += io-pgfault.o
>>   obj-$(CONFIG_SPRD_IOMMU) += sprd-iommu.o
>>   obj-$(CONFIG_APPLE_DART) += apple-dart.o
>> +obj-$(CONFIG_VSI_IOMMU) += vsi-iommu.o
>> diff --git a/drivers/iommu/vsi-iommu.c b/drivers/iommu/vsi-iommu.c
>> new file mode 100644
>> index 000000000000..f2fa1197916c
>> --- /dev/null
>> +++ b/drivers/iommu/vsi-iommu.c
>> @@ -0,0 +1,900 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +
> No copyright statement? (or an informational header block?)
>
> Cheers,
>    Diederik
>
>> +#include <linux/clk.h>
>> + <snip>

Re: [PATCH 5/5] media: verisilicon: Flush IOMMU before decoding a frame

[Jason Gunthorpe]

On Mon, Jun 16, 2025 at 04:55:53PM +0200, Benjamin Gaignard wrote:
> Flush the IOMMU mapping before decoding a frame to ensure that all memory
> translations are properly applied.
> 
> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
> ---
>  drivers/media/platform/verisilicon/hantro_drv.c | 11 +++++++++++
>  1 file changed, 11 insertions(+)

This is a really big red flag.

iommu translations are supposed to be controlled by the iommu driver
and should be flushed as part of the iommu map/unmap flows. It should
never be necessary to do something like this.

Jason

Re: [PATCH 5/5] media: verisilicon: Flush IOMMU before decoding a frame

[Benjamin Gaignard]

Le 17/06/2025 à 17:58, Jason Gunthorpe a écrit :
> On Mon, Jun 16, 2025 at 04:55:53PM +0200, Benjamin Gaignard wrote:
>> Flush the IOMMU mapping before decoding a frame to ensure that all memory
>> translations are properly applied.
>>
>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>> ---
>>   drivers/media/platform/verisilicon/hantro_drv.c | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
> This is a really big red flag.
>
> iommu translations are supposed to be controlled by the iommu driver
> and should be flushed as part of the iommu map/unmap flows. It should
> never be necessary to do something like this.

I have redone tests without this patch and the decode is perform correctly.
I will drop it in the next version.
I think I have fix the reference frame management in the driver so it becomes
useless.

Benjamin

>
> Jason

Re: [PATCH 5/5] media: verisilicon: Flush IOMMU before decoding a frame

[Robin Murphy]

On 17/06/2025 5:01 pm, Benjamin Gaignard wrote:
> 
> Le 17/06/2025 à 17:58, Jason Gunthorpe a écrit :
>> On Mon, Jun 16, 2025 at 04:55:53PM +0200, Benjamin Gaignard wrote:
>>> Flush the IOMMU mapping before decoding a frame to ensure that all 
>>> memory
>>> translations are properly applied.
>>>
>>> Signed-off-by: Benjamin Gaignard <benjamin.gaignard@collabora.com>
>>> ---
>>>   drivers/media/platform/verisilicon/hantro_drv.c | 11 +++++++++++
>>>   1 file changed, 11 insertions(+)
>> This is a really big red flag.
>>
>> iommu translations are supposed to be controlled by the iommu driver
>> and should be flushed as part of the iommu map/unmap flows. It should
>> never be necessary to do something like this.
> 
> I have redone tests without this patch and the decode is perform correctly.
> I will drop it in the next version.
> I think I have fix the reference frame management in the driver so it 
> becomes
> useless.

If it turns out the hardware does require a TLBI to guarantee new 
mappings are visible, though, then the correct way is to implement the 
iotlb_sync_map op.

Thanks,
Robin.

Re: [PATCH 3/5] iommu: Add verisilicon IOMMU driver

[Jason Gunthorpe]

On Mon, Jun 16, 2025 at 04:55:51PM +0200, Benjamin Gaignard wrote:

> +static struct vsi_iommu *vsi_iommu_from_dev(struct device *dev)
> +{
> +	struct vsi_iommudata *data = dev_iommu_priv_get(dev);
> +
> +	return data ? data->iommu : NULL;

It would be a serious bug if dev_iommu_priv_get() is null, don't check
for it, let it crash.

> +static struct iommu_domain *vsi_iommu_domain_alloc_paging(struct device *dev)
> +{
> +	struct vsi_iommu_domain *vsi_domain;
> +
> +	if (!dma_dev)
> +		return NULL;
> +
> +	vsi_domain = kzalloc(sizeof(*vsi_domain), GFP_KERNEL);
> +	if (!vsi_domain)
> +		return NULL;
> +
> +	/*
> +	 * iommu use a 2 level pagetable.
> +	 * Each level1 (dt) and level2 (pt) table has 1024 4-byte entries.
> +	 * Allocate one 4 KiB page for each table.
> +	 */
> +	vsi_domain->dt = iommu_alloc_pages_sz(GFP_KERNEL | GFP_DMA32,
> +					      SPAGE_SIZE);
> +	if (!vsi_domain->dt)
> +		goto err_free_domain;
> +
> +	vsi_domain->dt_dma = dma_map_single(dma_dev, vsi_domain->dt,
> +					    SPAGE_SIZE, DMA_TO_DEVICE);
> +	if (dma_mapping_error(dma_dev, vsi_domain->dt_dma)) {
> +		dev_err(dma_dev, "DMA map error for DT\n");
> +		goto err_free_dt;
> +	}
> +
> +	vsi_domain->pta = iommu_alloc_pages_sz(GFP_KERNEL | GFP_DMA32,
> +					       SPAGE_SIZE);
> +	if (!vsi_domain->pta)
> +		goto err_unmap_dt;
> +
> +	vsi_domain->pta_dma = dma_map_single(dma_dev, vsi_domain->pta,
> +					     SPAGE_SIZE, DMA_TO_DEVICE);
> +	if (dma_mapping_error(dma_dev, vsi_domain->pta_dma)) {
> +		dev_err(dma_dev, "DMA map error for PTA\n");
> +		goto err_free_pta;
> +	}
> +	vsi_domain->pta[0] = vsi_mk_pta(vsi_domain->dt_dma);

> +
> +	vsi_table_flush(vsi_domain, vsi_domain->pta_dma, 1024);
> +	vsi_table_flush(vsi_domain, vsi_domain->dt_dma, NUM_DT_ENTRIES);

dma_map_single already flushes, put things in the write order and no
need to double flush.

> +	spin_lock_init(&vsi_domain->iommus_lock);
> +	spin_lock_init(&vsi_domain->dt_lock);
> +	INIT_LIST_HEAD(&vsi_domain->iommus);
> +
> +	vsi_domain->domain.geometry.aperture_start = 0;
> +	vsi_domain->domain.geometry.aperture_end   = DMA_BIT_MASK(32);
> +	vsi_domain->domain.geometry.force_aperture = true;

Initialize domain.pgsize_bitmap here and remove this:

+       .pgsize_bitmap = VSI_IOMMU_PGSIZE_BITMAP,

It is going away.

> +	return &vsi_domain->domain;
> +
> +err_free_pta:
> +	iommu_free_pages(vsi_domain->pta);
> +err_unmap_dt:
> +	dma_unmap_single(dma_dev, vsi_domain->dt_dma,
> +			 SPAGE_SIZE, DMA_TO_DEVICE);
> +err_free_dt:
> +	iommu_free_pages(vsi_domain->dt);
> +err_free_domain:
> +	kfree(vsi_domain);
> +
> +	return NULL;
> +}
> +
> +static phys_addr_t vsi_iommu_iova_to_phys(struct iommu_domain *domain,
> +					  dma_addr_t iova)
> +{
> +	struct vsi_iommu_domain *vsi_domain = to_vsi_domain(domain);
> +	unsigned long flags;
> +	phys_addr_t pt_phys, phys = 0;
> +	u32 dte, pte;
> +	u32 *page_table;
> +
> +	spin_lock_irqsave(&vsi_domain->dt_lock, flags);

No locking should be here. Drivers are supposed to use cmpxchg to set
the new tables to avoid races, however there is complexity around the
cache flushing that this locking solves.

IDK, you might be better to use the new iommupt stuff since all these
algorithms including the complicated lockless cache flushing
optmization are sorted out there.

https://lore.kernel.org/linux-iommu/0-v3-a93aab628dbc+521-iommu_pt_jgg@nvidia.com/

> +	dte_index = vsi_iova_dte_index(iova);
> +	dte_addr = &vsi_domain->dt[dte_index];
> +	dte = *dte_addr;
> +	if (vsi_dte_is_pt_valid(dte))
> +		goto done;
> +
> +	page_table = (u32 *)get_zeroed_page(GFP_ATOMIC | GFP_DMA32);
> +	if (!page_table)
> +		return ERR_PTR(-ENOMEM);

Don't use get_zeroed_page for page table memory.

> +
> +	pt_dma = dma_map_single(dma_dev, page_table, SPAGE_SIZE, DMA_TO_DEVICE);
> +	if (dma_mapping_error(dma_dev, pt_dma)) {
> +		dev_err(dma_dev, "DMA mapping error while allocating page table\n");
> +		free_page((unsigned long)page_table);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	dte = vsi_mk_dte(pt_dma);
> +	*dte_addr = dte;
> +
> +	vsi_table_flush(vsi_domain, pt_dma, NUM_PT_ENTRIES);
> +	vsi_table_flush(vsi_domain,
> +			vsi_domain->dt_dma + dte_index * sizeof(u32), 1);

Double flushing again.

> +static int vsi_iommu_map_iova(struct vsi_iommu_domain *vsi_domain, u32 *pte_addr,
> +			      dma_addr_t pte_dma, dma_addr_t iova,
> +			      phys_addr_t paddr, size_t size, int prot)
> +{
> +	unsigned int pte_count;
> +	unsigned int pte_total = size / SPAGE_SIZE;
> +	phys_addr_t page_phys;
> +
> +	assert_spin_locked(&vsi_domain->dt_lock);
> +
> +	for (pte_count = 0; pte_count < pte_total; pte_count++) {
> +		u32 pte = pte_addr[pte_count];
> +
> +		if (vsi_pte_is_page_valid(pte))
> +			goto unwind;
> +
> +		pte_addr[pte_count] = vsi_mk_pte(paddr, prot);

So why is this:

#define VSI_IOMMU_PGSIZE_BITMAP 0x007ff000

If the sizes don't become encoded in the PTE? The bits beyond 4k
should reflect actual ability to store those sizes in PTEs, eg using
contiguous bits or something.

> +		paddr += SPAGE_SIZE;
> +	}
> +
> +	vsi_table_flush(vsi_domain, pte_dma, pte_total);
> +
> +	return 0;
> +unwind:
> +	/* Unmap the range of iovas that we just mapped */
> +	vsi_iommu_unmap_iova(vsi_domain, pte_addr, pte_dma,
> +			     pte_count * SPAGE_SIZE);
> +
> +	iova += pte_count * SPAGE_SIZE;
> +	page_phys = vsi_pte_page_address(pte_addr[pte_count]);
> +	pr_err("iova: %pad already mapped to %pa cannot remap to phys: %pa prot: %#x\n",
> +	       &iova, &page_phys, &paddr, prot);

No pr_errs prints on normal errors.

> +static void vsi_iommu_detach_device(struct iommu_domain *domain,
> +				    struct device *dev)
> +{
> +	struct vsi_iommu *iommu;
> +	struct vsi_iommu_domain *vsi_domain = to_vsi_domain(domain);
> +	unsigned long flags;
> +	int ret;
> +
> +	/* Allow 'virtual devices' (eg drm) to detach from domain */
> +	iommu = vsi_iommu_from_dev(dev);
> +	if (WARN_ON(!iommu))
> +		return;
> +
> +	dev_dbg(dev, "Detaching from iommu domain\n");
> +
> +	if (!iommu->domain)
> +		return;
> +
> +	spin_lock_irqsave(&vsi_domain->iommus_lock, flags);
> +	list_del_init(&iommu->node);
> +	spin_unlock_irqrestore(&vsi_domain->iommus_lock, flags);

The list del should probably be after the vsi_iommu_disable()? We
expect invalidations to continue to keep the IOTLB consistent until
the HW is no longer walking the page table.

> +static struct iommu_device *vsi_iommu_probe_device(struct device *dev)
> +{
> +	struct vsi_iommudata *data;
> +	struct vsi_iommu *iommu;
> +
> +	data = dev_iommu_priv_get(dev);
> +	if (!data)
> +		return ERR_PTR(-ENODEV);
> +
> +	iommu = vsi_iommu_from_dev(dev);
> +
> +	pr_info("%s,%d, consumer : %s, supplier : %s\n",
> +		__func__, __LINE__, dev_name(dev), dev_name(iommu->dev));

No prints

> +	/*
> +	 * link will free by platform_device_del(master) via
> +	 * BUS_NOTIFY_REMOVED_DEVICE
> +	 */
> +	data->link = device_link_add(dev, iommu->dev,
> +				     DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME);
> +
> +	/* set max segment size for dev, needed for single chunk map */
> +	if (!dev->dma_parms)
> +		dev->dma_parms = kzalloc(sizeof(*dev->dma_parms), GFP_KERNEL);
> +	if (!dev->dma_parms)
> +		return ERR_PTR(-ENOMEM);
> +
> +	dma_set_max_seg_size(dev, DMA_BIT_MASK(32));

I'm not sure this should be in an iommu driver??? Doesn't dma-iommu.c
deal with this stuff

> +static void vsi_iommu_release_device(struct device *dev)
> +{
> +	struct vsi_iommudata *data = dev_iommu_priv_get(dev);
> +
> +	device_link_del(data->link);

Leaking data..

> +static int vsi_iommu_of_xlate(struct device *dev,
> +			      const struct of_phandle_args *args)
> +{
> +	struct platform_device *iommu_dev;
> +	struct vsi_iommudata *data;
> +
> +	data = devm_kzalloc(dma_dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	dev_info(dev, "%s,%d\n", __func__, __LINE__);
> +	iommu_dev = of_find_device_by_node(args->np);
> +
> +	data->iommu = platform_get_drvdata(iommu_dev);
> +
> +	dev_iommu_priv_set(dev, data);

Can you use iommu_fwspec_add_ids() instead of this?

The only thing 'data' is doing here is to pass the iommu_dev, it is
much better if you can get that from the fwspec in probe, like say ARM
does it:

	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);

	smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);

And then don't allocate memory in the of_xlate.

> +static struct iommu_ops vsi_iommu_ops = {
> +	.domain_alloc_paging = vsi_iommu_domain_alloc_paging,
> +	.probe_device = vsi_iommu_probe_device,
> +	.release_device = vsi_iommu_release_device,
> +	.device_group = generic_single_device_group,
> +	.pgsize_bitmap = VSI_IOMMU_PGSIZE_BITMAP,

move pgsize_bitmap to alloc paging


> +static int vsi_iommu_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct vsi_iommu *iommu;
> +	struct resource *res;
> +	int num_res = pdev->num_resources;
> +	int err, i;
> +
> +	iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
> +	if (!iommu)
> +		return -ENOMEM;
> +
> +	platform_set_drvdata(pdev, iommu);

This should be done last after the function can't fail

> +
> +	iommu->group = iommu_group_alloc();
> +	if (IS_ERR(iommu->group)) {
> +		err = PTR_ERR(iommu->group);
> +		goto err_unprepare_clocks;
> +	}

This should not be in iommu drivers. I'm guessing you want
generic_single_device_group() ?

> +	/*
> +	 * use the first registered sysmmu device for performing
> +	 * dma mapping operations on iommu page tables (cpu cache flush)
> +	 */
> +	if (!dma_dev)
> +		dma_dev = &pdev->dev;

Huh? This is racey, and why? What is the reason not to use the current
device always? Put the dev in the iommu_domain during
domain_alloc_paging and get it from  dev->iommu->iommu_dev->dev.

> +
> +	pm_runtime_enable(dev);
> +
> +	err = iommu_device_sysfs_add(&iommu->iommu, dev, NULL, dev_name(dev));
> +	if (err)
> +		goto err_put_group;
> +
> +	err = iommu_device_register(&iommu->iommu, &vsi_iommu_ops, dev);
> +	if (err)
> +		goto err_remove_sysfs;

Register should usually be last.

> +	for (i = 0; i < iommu->num_irq; i++) {
> +		int irq = platform_get_irq(pdev, i);
> +
> +		if (irq < 0)
> +			return irq;
> +
> +		err = devm_request_irq(iommu->dev, irq, vsi_iommu_irq,
> +				       IRQF_SHARED, dev_name(dev), iommu);
> +		if (err)
> +			goto err_unregister;
> +	}

Why allocate interrupts after registration?

Jason

Re: [PATCH 3/5] iommu: Add verisilicon IOMMU driver

[kernel test robot]

Hi Benjamin,

kernel test robot noticed the following build warnings:

[auto build test WARNING on robh/for-next]
[also build test WARNING on rockchip/for-next linus/master v6.16-rc2 next-20250617]
[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/Benjamin-Gaignard/dt-bindings-vendor-prefixes-Add-Verisilicon/20250616-225842
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
patch link:    https://lore.kernel.org/r/20250616145607.116639-4-benjamin.gaignard%40collabora.com
patch subject: [PATCH 3/5] iommu: Add verisilicon IOMMU driver
config: arm64-randconfig-r111-20250618 (https://download.01.org/0day-ci/archive/20250618/202506181230.D53QwmdL-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 15.1.0
reproduce: (https://download.01.org/0day-ci/archive/20250618/202506181230.D53QwmdL-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/202506181230.D53QwmdL-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/iommu/vsi-iommu.c:890:24: sparse: sparse: symbol 'rockchip_vsi_iommu_driver' was not declared. Should it be static?

vim +/rockchip_vsi_iommu_driver +890 drivers/iommu/vsi-iommu.c

   889	
 > 890	struct platform_driver rockchip_vsi_iommu_driver = {

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

Re: [PATCH 3/5] iommu: Add verisilicon IOMMU driver

[Benjamin Gaignard]

Le 17/06/2025 à 18:32, Jason Gunthorpe a écrit :
> On Mon, Jun 16, 2025 at 04:55:51PM +0200, Benjamin Gaignard wrote:
>
>> +static struct vsi_iommu *vsi_iommu_from_dev(struct device *dev)
>> +{
>> +	struct vsi_iommudata *data = dev_iommu_priv_get(dev);
>> +
>> +	return data ? data->iommu : NULL;
> It would be a serious bug if dev_iommu_priv_get() is null, don't check
> for it, let it crash.

Ok, I will simplify/merge the driver internal structure and remove this function.

>> +static struct iommu_domain *vsi_iommu_domain_alloc_paging(struct device *dev)
>> +{
>> +	struct vsi_iommu_domain *vsi_domain;
>> +
>> +	if (!dma_dev)
>> +		return NULL;
>> +
>> +	vsi_domain = kzalloc(sizeof(*vsi_domain), GFP_KERNEL);
>> +	if (!vsi_domain)
>> +		return NULL;
>> +
>> +	/*
>> +	 * iommu use a 2 level pagetable.
>> +	 * Each level1 (dt) and level2 (pt) table has 1024 4-byte entries.
>> +	 * Allocate one 4 KiB page for each table.
>> +	 */
>> +	vsi_domain->dt = iommu_alloc_pages_sz(GFP_KERNEL | GFP_DMA32,
>> +					      SPAGE_SIZE);
>> +	if (!vsi_domain->dt)
>> +		goto err_free_domain;
>> +
>> +	vsi_domain->dt_dma = dma_map_single(dma_dev, vsi_domain->dt,
>> +					    SPAGE_SIZE, DMA_TO_DEVICE);
>> +	if (dma_mapping_error(dma_dev, vsi_domain->dt_dma)) {
>> +		dev_err(dma_dev, "DMA map error for DT\n");
>> +		goto err_free_dt;
>> +	}
>> +
>> +	vsi_domain->pta = iommu_alloc_pages_sz(GFP_KERNEL | GFP_DMA32,
>> +					       SPAGE_SIZE);
>> +	if (!vsi_domain->pta)
>> +		goto err_unmap_dt;
>> +
>> +	vsi_domain->pta_dma = dma_map_single(dma_dev, vsi_domain->pta,
>> +					     SPAGE_SIZE, DMA_TO_DEVICE);
>> +	if (dma_mapping_error(dma_dev, vsi_domain->pta_dma)) {
>> +		dev_err(dma_dev, "DMA map error for PTA\n");
>> +		goto err_free_pta;
>> +	}
>> +	vsi_domain->pta[0] = vsi_mk_pta(vsi_domain->dt_dma);
>> +
>> +	vsi_table_flush(vsi_domain, vsi_domain->pta_dma, 1024);
>> +	vsi_table_flush(vsi_domain, vsi_domain->dt_dma, NUM_DT_ENTRIES);
> dma_map_single already flushes, put things in the write order and no
> need to double flush.

I don't get your point here, for me it flush two different pieces of memory.

>
>> +	spin_lock_init(&vsi_domain->iommus_lock);
>> +	spin_lock_init(&vsi_domain->dt_lock);
>> +	INIT_LIST_HEAD(&vsi_domain->iommus);
>> +
>> +	vsi_domain->domain.geometry.aperture_start = 0;
>> +	vsi_domain->domain.geometry.aperture_end   = DMA_BIT_MASK(32);
>> +	vsi_domain->domain.geometry.force_aperture = true;
> Initialize domain.pgsize_bitmap here and remove this:
>
> +       .pgsize_bitmap = VSI_IOMMU_PGSIZE_BITMAP,
>
> It is going away.

Will be in v2.

>
>> +	return &vsi_domain->domain;
>> +
>> +err_free_pta:
>> +	iommu_free_pages(vsi_domain->pta);
>> +err_unmap_dt:
>> +	dma_unmap_single(dma_dev, vsi_domain->dt_dma,
>> +			 SPAGE_SIZE, DMA_TO_DEVICE);
>> +err_free_dt:
>> +	iommu_free_pages(vsi_domain->dt);
>> +err_free_domain:
>> +	kfree(vsi_domain);
>> +
>> +	return NULL;
>> +}
>> +
>> +static phys_addr_t vsi_iommu_iova_to_phys(struct iommu_domain *domain,
>> +					  dma_addr_t iova)
>> +{
>> +	struct vsi_iommu_domain *vsi_domain = to_vsi_domain(domain);
>> +	unsigned long flags;
>> +	phys_addr_t pt_phys, phys = 0;
>> +	u32 dte, pte;
>> +	u32 *page_table;
>> +
>> +	spin_lock_irqsave(&vsi_domain->dt_lock, flags);
> No locking should be here. Drivers are supposed to use cmpxchg to set
> the new tables to avoid races, however there is complexity around the
> cache flushing that this locking solves.
>
> IDK, you might be better to use the new iommupt stuff since all these
> algorithms including the complicated lockless cache flushing
> optmization are sorted out there.
>
> https://lore.kernel.org/linux-iommu/0-v3-a93aab628dbc+521-iommu_pt_jgg@nvidia.com/

This series is not merged on mainline (at least I don't see it) so I will keep
it like it is for the moment.

>
>> +	dte_index = vsi_iova_dte_index(iova);
>> +	dte_addr = &vsi_domain->dt[dte_index];
>> +	dte = *dte_addr;
>> +	if (vsi_dte_is_pt_valid(dte))
>> +		goto done;
>> +
>> +	page_table = (u32 *)get_zeroed_page(GFP_ATOMIC | GFP_DMA32);
>> +	if (!page_table)
>> +		return ERR_PTR(-ENOMEM);
> Don't use get_zeroed_page for page table memory.

I will use kmem_cache in v2

>
>> +
>> +	pt_dma = dma_map_single(dma_dev, page_table, SPAGE_SIZE, DMA_TO_DEVICE);
>> +	if (dma_mapping_error(dma_dev, pt_dma)) {
>> +		dev_err(dma_dev, "DMA mapping error while allocating page table\n");
>> +		free_page((unsigned long)page_table);
>> +		return ERR_PTR(-ENOMEM);
>> +	}
>> +
>> +	dte = vsi_mk_dte(pt_dma);
>> +	*dte_addr = dte;
>> +
>> +	vsi_table_flush(vsi_domain, pt_dma, NUM_PT_ENTRIES);
>> +	vsi_table_flush(vsi_domain,
>> +			vsi_domain->dt_dma + dte_index * sizeof(u32), 1);
> Double flushing again.

Same here, for me I flushing two different memory area.

>
>> +static int vsi_iommu_map_iova(struct vsi_iommu_domain *vsi_domain, u32 *pte_addr,
>> +			      dma_addr_t pte_dma, dma_addr_t iova,
>> +			      phys_addr_t paddr, size_t size, int prot)
>> +{
>> +	unsigned int pte_count;
>> +	unsigned int pte_total = size / SPAGE_SIZE;
>> +	phys_addr_t page_phys;
>> +
>> +	assert_spin_locked(&vsi_domain->dt_lock);
>> +
>> +	for (pte_count = 0; pte_count < pte_total; pte_count++) {
>> +		u32 pte = pte_addr[pte_count];
>> +
>> +		if (vsi_pte_is_page_valid(pte))
>> +			goto unwind;
>> +
>> +		pte_addr[pte_count] = vsi_mk_pte(paddr, prot);
> So why is this:
>
> #define VSI_IOMMU_PGSIZE_BITMAP 0x007ff000
>
> If the sizes don't become encoded in the PTE? The bits beyond 4k
> should reflect actual ability to store those sizes in PTEs, eg using
> contiguous bits or something.

The iommu use arrays to store up to 1024 4k pages indexes so the size
isn't coded in the PTE bits but the numbers of used indexes for each arrays.

>
>> +		paddr += SPAGE_SIZE;
>> +	}
>> +
>> +	vsi_table_flush(vsi_domain, pte_dma, pte_total);
>> +
>> +	return 0;
>> +unwind:
>> +	/* Unmap the range of iovas that we just mapped */
>> +	vsi_iommu_unmap_iova(vsi_domain, pte_addr, pte_dma,
>> +			     pte_count * SPAGE_SIZE);
>> +
>> +	iova += pte_count * SPAGE_SIZE;
>> +	page_phys = vsi_pte_page_address(pte_addr[pte_count]);
>> +	pr_err("iova: %pad already mapped to %pa cannot remap to phys: %pa prot: %#x\n",
>> +	       &iova, &page_phys, &paddr, prot);
> No pr_errs prints on normal errors.

I will remove it in v2

>
>> +static void vsi_iommu_detach_device(struct iommu_domain *domain,
>> +				    struct device *dev)
>> +{
>> +	struct vsi_iommu *iommu;
>> +	struct vsi_iommu_domain *vsi_domain = to_vsi_domain(domain);
>> +	unsigned long flags;
>> +	int ret;
>> +
>> +	/* Allow 'virtual devices' (eg drm) to detach from domain */
>> +	iommu = vsi_iommu_from_dev(dev);
>> +	if (WARN_ON(!iommu))
>> +		return;
>> +
>> +	dev_dbg(dev, "Detaching from iommu domain\n");
>> +
>> +	if (!iommu->domain)
>> +		return;
>> +
>> +	spin_lock_irqsave(&vsi_domain->iommus_lock, flags);
>> +	list_del_init(&iommu->node);
>> +	spin_unlock_irqrestore(&vsi_domain->iommus_lock, flags);
> The list del should probably be after the vsi_iommu_disable()? We
> expect invalidations to continue to keep the IOTLB consistent until
> the HW is no longer walking the page table.

I will do that in v2.

>
>> +static struct iommu_device *vsi_iommu_probe_device(struct device *dev)
>> +{
>> +	struct vsi_iommudata *data;
>> +	struct vsi_iommu *iommu;
>> +
>> +	data = dev_iommu_priv_get(dev);
>> +	if (!data)
>> +		return ERR_PTR(-ENODEV);
>> +
>> +	iommu = vsi_iommu_from_dev(dev);
>> +
>> +	pr_info("%s,%d, consumer : %s, supplier : %s\n",
>> +		__func__, __LINE__, dev_name(dev), dev_name(iommu->dev));
> No prints
>
>> +	/*
>> +	 * link will free by platform_device_del(master) via
>> +	 * BUS_NOTIFY_REMOVED_DEVICE
>> +	 */
>> +	data->link = device_link_add(dev, iommu->dev,
>> +				     DL_FLAG_STATELESS | DL_FLAG_PM_RUNTIME);
>> +
>> +	/* set max segment size for dev, needed for single chunk map */
>> +	if (!dev->dma_parms)
>> +		dev->dma_parms = kzalloc(sizeof(*dev->dma_parms), GFP_KERNEL);
>> +	if (!dev->dma_parms)
>> +		return ERR_PTR(-ENOMEM);
>> +
>> +	dma_set_max_seg_size(dev, DMA_BIT_MASK(32));
> I'm not sure this should be in an iommu driver??? Doesn't dma-iommu.c
> deal with this stuff

You are right, I will remove it.

>
>> +static void vsi_iommu_release_device(struct device *dev)
>> +{
>> +	struct vsi_iommudata *data = dev_iommu_priv_get(dev);
>> +
>> +	device_link_del(data->link);
> Leaking data..

I will change the driver internal structures and remove data->link.

>
>> +static int vsi_iommu_of_xlate(struct device *dev,
>> +			      const struct of_phandle_args *args)
>> +{
>> +	struct platform_device *iommu_dev;
>> +	struct vsi_iommudata *data;
>> +
>> +	data = devm_kzalloc(dma_dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	dev_info(dev, "%s,%d\n", __func__, __LINE__);
>> +	iommu_dev = of_find_device_by_node(args->np);
>> +
>> +	data->iommu = platform_get_drvdata(iommu_dev);
>> +
>> +	dev_iommu_priv_set(dev, data);
> Can you use iommu_fwspec_add_ids() instead of this?
>
> The only thing 'data' is doing here is to pass the iommu_dev, it is
> much better if you can get that from the fwspec in probe, like say ARM
> does it:
>
> 	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>
> 	smmu = arm_smmu_get_by_fwnode(fwspec->iommu_fwnode);
>
> And then don't allocate memory in the of_xlate.

I will rework driver probe and the internal structure to avoid allocate here.

>
>> +static struct iommu_ops vsi_iommu_ops = {
>> +	.domain_alloc_paging = vsi_iommu_domain_alloc_paging,
>> +	.probe_device = vsi_iommu_probe_device,
>> +	.release_device = vsi_iommu_release_device,
>> +	.device_group = generic_single_device_group,
>> +	.pgsize_bitmap = VSI_IOMMU_PGSIZE_BITMAP,
> move pgsize_bitmap to alloc paging

ok

>
>
>> +static int vsi_iommu_probe(struct platform_device *pdev)
>> +{
>> +	struct device *dev = &pdev->dev;
>> +	struct vsi_iommu *iommu;
>> +	struct resource *res;
>> +	int num_res = pdev->num_resources;
>> +	int err, i;
>> +
>> +	iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
>> +	if (!iommu)
>> +		return -ENOMEM;
>> +
>> +	platform_set_drvdata(pdev, iommu);
> This should be done last after the function can't fail

sure I will fix it.

>
>> +
>> +	iommu->group = iommu_group_alloc();
>> +	if (IS_ERR(iommu->group)) {
>> +		err = PTR_ERR(iommu->group);
>> +		goto err_unprepare_clocks;
>> +	}
> This should not be in iommu drivers. I'm guessing you want
> generic_single_device_group() ?

I will remove it.

>
>> +	/*
>> +	 * use the first registered sysmmu device for performing
>> +	 * dma mapping operations on iommu page tables (cpu cache flush)
>> +	 */
>> +	if (!dma_dev)
>> +		dma_dev = &pdev->dev;
> Huh? This is racey, and why? What is the reason not to use the current
> device always? Put the dev in the iommu_domain during
> domain_alloc_paging and get it from  dev->iommu->iommu_dev->dev.

I will change that too.

>
>> +
>> +	pm_runtime_enable(dev);
>> +
>> +	err = iommu_device_sysfs_add(&iommu->iommu, dev, NULL, dev_name(dev));
>> +	if (err)
>> +		goto err_put_group;
>> +
>> +	err = iommu_device_register(&iommu->iommu, &vsi_iommu_ops, dev);
>> +	if (err)
>> +		goto err_remove_sysfs;
> Register should usually be last.
>
>> +	for (i = 0; i < iommu->num_irq; i++) {
>> +		int irq = platform_get_irq(pdev, i);
>> +
>> +		if (irq < 0)
>> +			return irq;
>> +
>> +		err = devm_request_irq(iommu->dev, irq, vsi_iommu_irq,
>> +				       IRQF_SHARED, dev_name(dev), iommu);
>> +		if (err)
>> +			goto err_unregister;
>> +	}
> Why allocate interrupts after registration?

I will rework probe function to fix all that.

Thanks for your remarks.
Benjamin

>
> Jason
>

Re: [PATCH 3/5] iommu: Add verisilicon IOMMU driver

[Jason Gunthorpe]

On Wed, Jun 18, 2025 at 02:04:19PM +0200, Benjamin Gaignard wrote:
> 
> Le 17/06/2025 à 18:32, Jason Gunthorpe a écrit :
> > > +	vsi_domain->dt_dma = dma_map_single(dma_dev, vsi_domain->dt,
> > > +					    SPAGE_SIZE, DMA_TO_DEVICE);
> > > +	if (dma_mapping_error(dma_dev, vsi_domain->dt_dma)) {
> > > +		dev_err(dma_dev, "DMA map error for DT\n");
> > > +		goto err_free_dt;
> > > +	}
> > > +
> > > +	vsi_domain->pta = iommu_alloc_pages_sz(GFP_KERNEL | GFP_DMA32,
> > > +					       SPAGE_SIZE);
> > > +	if (!vsi_domain->pta)
> > > +		goto err_unmap_dt;
> > > +
> > > +	vsi_domain->pta_dma = dma_map_single(dma_dev, vsi_domain->pta,
> > > +					     SPAGE_SIZE, DMA_TO_DEVICE);
> > > +	if (dma_mapping_error(dma_dev, vsi_domain->pta_dma)) {
> > > +		dev_err(dma_dev, "DMA map error for PTA\n");
> > > +		goto err_free_pta;
> > > +	}
> > > +	vsi_domain->pta[0] = vsi_mk_pta(vsi_domain->dt_dma);
> > > +
> > > +	vsi_table_flush(vsi_domain, vsi_domain->pta_dma, 1024);
> > > +	vsi_table_flush(vsi_domain, vsi_domain->dt_dma, NUM_DT_ENTRIES);
> > dma_map_single already flushes, put things in the write order and no
> > need to double flush.
> 
> I don't get your point here, for me it flush two different pieces of memory.

dma_map_single() already flushes the cache, you don't need to do it
again.

Do your memory writes then call dma_map_signle().

> > > +	dte_index = vsi_iova_dte_index(iova);
> > > +	dte_addr = &vsi_domain->dt[dte_index];
> > > +	dte = *dte_addr;
> > > +	if (vsi_dte_is_pt_valid(dte))
> > > +		goto done;
> > > +
> > > +	page_table = (u32 *)get_zeroed_page(GFP_ATOMIC | GFP_DMA32);
> > > +	if (!page_table)
> > > +		return ERR_PTR(-ENOMEM);
> > Don't use get_zeroed_page for page table memory.
> 
> I will use kmem_cache in v2

I mean you are supposed to iommu-pages.h for page table memory.

> > > +	pt_dma = dma_map_single(dma_dev, page_table, SPAGE_SIZE, DMA_TO_DEVICE);
> > > +	if (dma_mapping_error(dma_dev, pt_dma)) {
> > > +		dev_err(dma_dev, "DMA mapping error while allocating page table\n");
> > > +		free_page((unsigned long)page_table);
> > > +		return ERR_PTR(-ENOMEM);
> > > +	}
> > > +
> > > +	dte = vsi_mk_dte(pt_dma);
> > > +	*dte_addr = dte;
> > > +
> > > +	vsi_table_flush(vsi_domain, pt_dma, NUM_PT_ENTRIES);
> > > +	vsi_table_flush(vsi_domain,
> > > +			vsi_domain->dt_dma + dte_index * sizeof(u32), 1);
> > Double flushing again.
>
> Same here, for me I flushing two different memory area.

write to the page-table, then call dma_map_single(), don't flush it again.

> > > +static int vsi_iommu_map_iova(struct vsi_iommu_domain *vsi_domain, u32 *pte_addr,
> > > +			      dma_addr_t pte_dma, dma_addr_t iova,
> > > +			      phys_addr_t paddr, size_t size, int prot)
> > > +{
> > > +	unsigned int pte_count;
> > > +	unsigned int pte_total = size / SPAGE_SIZE;
> > > +	phys_addr_t page_phys;
> > > +
> > > +	assert_spin_locked(&vsi_domain->dt_lock);
> > > +
> > > +	for (pte_count = 0; pte_count < pte_total; pte_count++) {
> > > +		u32 pte = pte_addr[pte_count];
> > > +
> > > +		if (vsi_pte_is_page_valid(pte))
> > > +			goto unwind;
> > > +
> > > +		pte_addr[pte_count] = vsi_mk_pte(paddr, prot);
> > So why is this:
> > 
> > #define VSI_IOMMU_PGSIZE_BITMAP 0x007ff000
> > 
> > If the sizes don't become encoded in the PTE? The bits beyond 4k
> > should reflect actual ability to store those sizes in PTEs, eg using
> > contiguous bits or something.
> 
> The iommu use arrays to store up to 1024 4k pages indexes so the size
> isn't coded in the PTE bits but the numbers of used indexes for each arrays.

That isn't how it works, if the PTE bits don't code the size then you
don't set the VSI_IOMMU_PGSIZE_BITMAP. You just want SZ_4K for the way
this driver is written.

Jason