Skip to contentSkip to conversation

Add Unisoc iommu basic driver

PATCH In review Chunyan Zhang 3 participants 7 messages Jan 21, 2021 – Jan 27, 2021

Subsystems (3)
lore
Share & tools
lore

Chunyan Zhang

author reviewer
#0 lore raw

cover Add Unisoc iommu basic driver

From: Chunyan Zhang <[email protected]>

Changes since RFC v2:
* Addressed Robin's comments:
- Add COMPILE_TEST support;
- Use DMA allocator for PTE;
- Revised to avoid resource leak issue;
- Added ->iotlb_sync implemented;
- Moved iommu group allocation to probe;
- Changed some function names to make them sprd specific;
* Added support for more iommu instance;

Changes since RFC v1:
* Rebased on v5.11-rc1;
* Changed sprd-iommu to tristate;
* Removed check for args_count of iommu OF node, since there's no args for sprd-iommu device node;
* Added another IP version (i.e. vau);
* Removed unnecessary configs selection from CONFIG_SPRD_IOMMU;
* Changed to get zeroed pages.

Chunyan Zhang (2):
dt-bindings: iommu: add bindings for sprd iommu
iommu: add Unisoc iommu basic driver

 .../devicetree/bindings/iommu/sprd,iommu.yaml |  45 ++
 drivers/iommu/Kconfig                         |  12 +
 drivers/iommu/Makefile                        |   1 +
 drivers/iommu/sprd-iommu.c                    | 566 ++++++++++++++++++
 4 files changed, 624 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
 create mode 100644 drivers/iommu/sprd-iommu.c
Signature · 6 lines
2.25.1

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
author reviewer
#1 lore raw

PATCH 1/2 dt-bindings: iommu: add bindings for sprd iommu

From: Chunyan Zhang <[email protected]>

This patch adds bindings to support display and Image codec(jpeg) iommu instance.

The iommu support for others would be added once finished tests with those devices, such as a few signal processors, including VSP(video), GSP(graphic), ISP(image), and camera CPP, etc.

---
 .../devicetree/bindings/iommu/sprd,iommu.yaml | 45 +++++++++++++++++++
 1 file changed, 45 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
Documentation/devicetree/bindings/iommu/sprd,iommu.yaml new file +45 -0
new file mode 100644
index 000000000000..4b912857c112
@@ -0,0 +1,45 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2020 Unisoc Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/iommu/sprd,iommu.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Unisoc IOMMU and Multi-media MMU
+
+maintainers:
+  - Chunyan Zhang <[email protected]>
+
+properties:
+  compatible:
+    enum:
+      - sprd,iommu-v1-disp
+      - sprd,iommu-v1-jpg
+
+  reg:
+    maxItems: 1
+
+  "#iommu-cells":
+    const: 0
+    description:
+      Unisoc IOMMUs are all single-master IOMMU devices, therefore no
+      additional information needs to associate with its master device.
+      Please refer to the generic bindings document for more details,
+      Documentation/devicetree/bindings/iommu/iommu.txt
+
+required:
+  - compatible
+  - reg
+  - "#iommu-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    iommu_disp: iommu@63000000 {
+      compatible = "sprd,iommu-v1-disp";
+      reg = <0x63000000 0x880>;
+      #iommu-cells = <0>;
+    };
+
+...
Signature · 6 lines
2.25.1

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
author reviewer
#2 lore raw

PATCH 2/2 iommu: add Unisoc iommu basic driver

In reply to Chunyan Zhang

From: Chunyan Zhang <[email protected]>

This patch only adds display iommu support, the driver was tested with sprd dpu and image codec processor.

The iommu support for others would be added once finished tests with those devices, such as a few signal processors, including VSP(video), GSP(graphic), ISP(image), and camera CPP, etc.

---
 drivers/iommu/Kconfig      |  12 +
 drivers/iommu/Makefile     |   1 +
 drivers/iommu/sprd-iommu.c | 566 +++++++++++++++++++++++++++++++++++++
 3 files changed, 579 insertions(+)
 create mode 100644 drivers/iommu/sprd-iommu.c
drivers/iommu/Kconfig +12 -0
index 192ef8f61310..79af62c519ae 100644
@@ -408,4 +408,16 @@ config VIRTIO_IOMMU
 
 	  Say Y here if you intend to run this kernel as a guest.
 
+config SPRD_IOMMU
+	tristate "Unisoc IOMMU Support"
+	depends on ARCH_SPRD || COMPILE_TEST
+	select IOMMU_API
+	help
+	  Support for IOMMU on Unisoc's SoCs on which multi-media subsystems
+	  need IOMMU, such as DPU, Image codec(jpeg) processor, and a few
+	  signal processors, including VSP(video), GSP(graphic), ISP(image), and
+	  CPP, etc.
+
+	  Say Y here if you want multi-media functions.
+
 endif # IOMMU_SUPPORT
drivers/iommu/Makefile +1 -0
index 61bd30cd8369..5925b6af2123 100644
@@ -28,3 +28,4 @@ obj-$(CONFIG_S390_IOMMU) += s390-iommu.o
 obj-$(CONFIG_HYPERV_IOMMU) += hyperv-iommu.o
 obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
 obj-$(CONFIG_IOMMU_SVA_LIB) += iommu-sva-lib.o
+obj-$(CONFIG_SPRD_IOMMU) += sprd-iommu.o
drivers/iommu/sprd-iommu.c new file +566 -0

566 lines not on this page · raw message

Signature · 6 lines
2.25.1

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu
#3 lore raw

Re: [PATCH v1 2/2] iommu: add Unisoc iommu basic driver

I love your patch! Perhaps something to improve:

Hi Chunyan,

I love your patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on v5.11-rc4 next-20210121]
[cannot apply to iommu/next]
[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]

url:    https://github.com/0day-ci/linux/commits/Chunyan-Zhang/Add-Unisoc-iommu-basic-driver/20210121-194023
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arc-allyesconfig (attached as .config)
compiler: arceb-elf-gcc (GCC) 9.3.0

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/891db11d7229149235a02e5bc31a61188243a5d7
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Chunyan-Zhang/Add-Unisoc-iommu-basic-driver/20210121-194023 git checkout 891db11d7229149235a02e5bc31a61188243a5d7
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc

If you fix the issue, kindly add following tag as appropriate

All warnings (new ones prefixed by >>):

drivers/iommu/sprd-iommu.c: In function 'sprd_iommu_attach_device':
drivers/iommu/sprd-iommu.c:248:16: error: implicit declaration of function 'dma_alloc_coherent' [-Werror=implicit-function-declaration]

     248 |  dom->pgt_va = dma_alloc_coherent(sdev->dev, pgt_size, &dom->pgt_pa, GFP_KERNEL);
         |                ^~~~~~~~~~~~~~~~~~
>> drivers/iommu/sprd-iommu.c:248:14: warning: assignment to 'u32 *' {aka 'unsigned int *'} from 'int' makes pointer from integer without a cast [-Wint-conversion]
     248 |  dom->pgt_va = dma_alloc_coherent(sdev->dev, pgt_size, &dom->pgt_pa, GFP_KERNEL);
         |              ^
   drivers/iommu/sprd-iommu.c: In function 'sprd_iommu_detach_device':
   drivers/iommu/sprd-iommu.c:270:2: error: implicit declaration of function 'dma_free_coherent' [-Werror=implicit-function-declaration]
     270 |  dma_free_coherent(sdev->dev, pgt_size, dom->pgt_va, dom->pgt_pa);
         |  ^~~~~~~~~~~~~~~~~
   In file included from include/linux/device.h:15,
                    from drivers/iommu/sprd-iommu.c:10:
   drivers/iommu/sprd-iommu.c: In function 'sprd_iommu_map':
>> drivers/iommu/sprd-iommu.c:296:27: warning: format '%lx' expects argument of type 'long unsigned int', but argument 4 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
     296 |   dev_err(dom->sdev->dev, "(iova(0x%lx) + sixe(0x%lx)) are not in the range!\n",
         |                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:19:22: note: in definition of macro 'dev_fmt'
      19 | #define dev_fmt(fmt) fmt
         |                      ^~~
   drivers/iommu/sprd-iommu.c:296:3: note: in expansion of macro 'dev_err'
     296 |   dev_err(dom->sdev->dev, "(iova(0x%lx) + sixe(0x%lx)) are not in the range!\n",
         |   ^~~~~~~
   drivers/iommu/sprd-iommu.c:296:52: note: format string is defined here
     296 |   dev_err(dom->sdev->dev, "(iova(0x%lx) + sixe(0x%lx)) are not in the range!\n",
         |                                                  ~~^
         |                                                    |
         |                                                    long unsigned int
         |                                                  %x
>> drivers/iommu/sprd-iommu.c:279:38: warning: variable 'mdata' set but not used [-Wunused-but-set-variable]
     279 |  const struct sprd_iommu_match_data *mdata;
         |                                      ^~~~~
   drivers/iommu/sprd-iommu.c: In function 'sprd_iommu_probe':
   drivers/iommu/sprd-iommu.c:483:21: warning: assignment to 'u32 *' {aka 'unsigned int *'} from 'int' makes pointer from integer without a cast [-Wint-conversion]
     483 |  sdev->prot_page_va = dma_alloc_coherent(dev, SPRD_IOMMU_PAGE_SIZE,
         |                     ^

cc1: some warnings being treated as errors

vim +248 drivers/iommu/sprd-iommu.c

240
241 static int sprd_iommu_attach_device(struct iommu_domain *domain,
242 struct device *dev)
243 {
244 struct sprd_iommu_device *sdev = dev_iommu_priv_get(dev);
245 struct sprd_iommu_domain *dom = to_sprd_domain(domain);
246 size_t pgt_size = sprd_iommu_pgt_size(domain);
247

 > 248		dom->pgt_va = dma_alloc_coherent(sdev->dev, pgt_size, &dom->pgt_pa, GFP_KERNEL);
   249		if (!dom->pgt_va)
   250			return -ENOMEM;
   251	
   252		dom->sdev = sdev;
   253	
   254		sprd_iommu_first_ppn(dom);
   255		sprd_iommu_first_vpn(dom);
   256		sprd_iommu_vpn_range(dom);
   257		sprd_iommu_default_ppn(sdev);
   258		sprd_iommu_hw_en(sdev, true);
   259	
   260		return 0;
   261	}
   262	
   263	static void sprd_iommu_detach_device(struct iommu_domain *domain,
   264						     struct device *dev)
   265	{
   266		struct sprd_iommu_domain *dom = to_sprd_domain(domain);
   267		struct sprd_iommu_device *sdev = dom->sdev;
   268		size_t pgt_size = sprd_iommu_pgt_size(domain);
   269	
   270		dma_free_coherent(sdev->dev, pgt_size, dom->pgt_va, dom->pgt_pa);
   271		sprd_iommu_hw_en(sdev, false);
   272		dom->sdev = NULL;
   273	}
   274	
   275	static int sprd_iommu_map(struct iommu_domain *domain, unsigned long iova,
   276				  phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
   277	{
   278		struct sprd_iommu_domain *dom = to_sprd_domain(domain);
 > 279		const struct sprd_iommu_match_data *mdata;
   280		unsigned int page_num = size >> SPRD_IOMMU_PAGE_SHIFT;
   281		unsigned long flags;
   282		unsigned int i;
   283		u32 *pgt_base_iova;
   284		u32 pabase = (u32)paddr;
   285		int map_size = 0;
   286		unsigned long start = domain->geometry.aperture_start;
   287		unsigned long end = domain->geometry.aperture_end;
   288	
   289		if (!dom->sdev) {
   290			pr_err("No sprd_iommu_device attached to the domain\n");
   291			return -EINVAL;
   292		}
   293	
   294		mdata = dom->sdev->mdata;
   295		if (iova < start || (iova + size) > (end + 1)) {
 > 296			dev_err(dom->sdev->dev, "(iova(0x%lx) + sixe(0x%lx)) are not in the range!\n",

297 iova, size);
298 return -EINVAL;
299 }
300
301 pgt_base_iova = dom->pgt_va + ((iova - start) >> SPRD_IOMMU_PAGE_SHIFT);
302
303 spin_lock_irqsave(&dom->pgtlock, flags);
304 for (i = 0; i < page_num; i++) {
305 pgt_base_iova[i] = pabase >> SPRD_IOMMU_PAGE_SHIFT;
306 pabase += SPRD_IOMMU_PAGE_SIZE;
307 map_size += SPRD_IOMMU_PAGE_SIZE;
308 }
309 spin_unlock_irqrestore(&dom->pgtlock, flags);
310
311 return map_size == size ? 0 : -EEXIST;
312 }
313

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

#4 lore raw

Re: [PATCH v1 2/2] iommu: add Unisoc iommu basic driver

In reply to Chunyan Zhang
I love your patch! Yet something to improve:

Hi Chunyan,

I love your patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on v5.11-rc4 next-20210121]
[cannot apply to iommu/next]
[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]

url:    https://github.com/0day-ci/linux/commits/Chunyan-Zhang/Add-Unisoc-iommu-basic-driver/20210121-194023
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: arc-allyesconfig (attached as .config)
compiler: arceb-elf-gcc (GCC) 9.3.0

reproduce (this is a W=1 build):
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross
# https://github.com/0day-ci/linux/commit/891db11d7229149235a02e5bc31a61188243a5d7
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Chunyan-Zhang/Add-Unisoc-iommu-basic-driver/20210121-194023 git checkout 891db11d7229149235a02e5bc31a61188243a5d7
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=arc

If you fix the issue, kindly add following tag as appropriate

All errors (new ones prefixed by >>):

drivers/iommu/sprd-iommu.c: In function 'sprd_iommu_attach_device':

>> drivers/iommu/sprd-iommu.c:248:16: error: implicit declaration of function 'dma_alloc_coherent' [-Werror=implicit-function-declaration]
     248 |  dom->pgt_va = dma_alloc_coherent(sdev->dev, pgt_size, &dom->pgt_pa, GFP_KERNEL);
         |                ^~~~~~~~~~~~~~~~~~
   drivers/iommu/sprd-iommu.c:248:14: warning: assignment to 'u32 *' {aka 'unsigned int *'} from 'int' makes pointer from integer without a cast [-Wint-conversion]
     248 |  dom->pgt_va = dma_alloc_coherent(sdev->dev, pgt_size, &dom->pgt_pa, GFP_KERNEL);
         |              ^
   drivers/iommu/sprd-iommu.c: In function 'sprd_iommu_detach_device':
>> drivers/iommu/sprd-iommu.c:270:2: error: implicit declaration of function 'dma_free_coherent' [-Werror=implicit-function-declaration]
     270 |  dma_free_coherent(sdev->dev, pgt_size, dom->pgt_va, dom->pgt_pa);
         |  ^~~~~~~~~~~~~~~~~
   In file included from include/linux/device.h:15,
                    from drivers/iommu/sprd-iommu.c:10:
   drivers/iommu/sprd-iommu.c: In function 'sprd_iommu_map':
   drivers/iommu/sprd-iommu.c:296:27: warning: format '%lx' expects argument of type 'long unsigned int', but argument 4 has type 'size_t' {aka 'unsigned int'} [-Wformat=]
     296 |   dev_err(dom->sdev->dev, "(iova(0x%lx) + sixe(0x%lx)) are not in the range!\n",
         |                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
   include/linux/dev_printk.h:19:22: note: in definition of macro 'dev_fmt'
      19 | #define dev_fmt(fmt) fmt
         |                      ^~~
   drivers/iommu/sprd-iommu.c:296:3: note: in expansion of macro 'dev_err'
     296 |   dev_err(dom->sdev->dev, "(iova(0x%lx) + sixe(0x%lx)) are not in the range!\n",
         |   ^~~~~~~
   drivers/iommu/sprd-iommu.c:296:52: note: format string is defined here
     296 |   dev_err(dom->sdev->dev, "(iova(0x%lx) + sixe(0x%lx)) are not in the range!\n",
         |                                                  ~~^
         |                                                    |
         |                                                    long unsigned int
         |                                                  %x
   drivers/iommu/sprd-iommu.c:279:38: warning: variable 'mdata' set but not used [-Wunused-but-set-variable]
     279 |  const struct sprd_iommu_match_data *mdata;
         |                                      ^~~~~
   drivers/iommu/sprd-iommu.c: In function 'sprd_iommu_probe':
   drivers/iommu/sprd-iommu.c:483:21: warning: assignment to 'u32 *' {aka 'unsigned int *'} from 'int' makes pointer from integer without a cast [-Wint-conversion]
     483 |  sdev->prot_page_va = dma_alloc_coherent(dev, SPRD_IOMMU_PAGE_SIZE,
         |                     ^

cc1: some warnings being treated as errors

vim +/dma_alloc_coherent +248 drivers/iommu/sprd-iommu.c

240
241 static int sprd_iommu_attach_device(struct iommu_domain *domain,
242 struct device *dev)
243 {
244 struct sprd_iommu_device *sdev = dev_iommu_priv_get(dev);
245 struct sprd_iommu_domain *dom = to_sprd_domain(domain);
246 size_t pgt_size = sprd_iommu_pgt_size(domain);
247

 > 248		dom->pgt_va = dma_alloc_coherent(sdev->dev, pgt_size, &dom->pgt_pa, GFP_KERNEL);
   249		if (!dom->pgt_va)
   250			return -ENOMEM;
   251	
   252		dom->sdev = sdev;
   253	
   254		sprd_iommu_first_ppn(dom);
   255		sprd_iommu_first_vpn(dom);
   256		sprd_iommu_vpn_range(dom);
   257		sprd_iommu_default_ppn(sdev);
   258		sprd_iommu_hw_en(sdev, true);
   259	
   260		return 0;
   261	}
   262	
   263	static void sprd_iommu_detach_device(struct iommu_domain *domain,
   264						     struct device *dev)
   265	{
   266		struct sprd_iommu_domain *dom = to_sprd_domain(domain);
   267		struct sprd_iommu_device *sdev = dom->sdev;
   268		size_t pgt_size = sprd_iommu_pgt_size(domain);
   269	
 > 270		dma_free_coherent(sdev->dev, pgt_size, dom->pgt_va, dom->pgt_pa);

271 sprd_iommu_hw_en(sdev, false);
272 dom->sdev = NULL;
273 }
274

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

reviewer
#5 lore raw

Re: [PATCH v1 2/2] iommu: add Unisoc iommu basic driver

In reply to Chunyan Zhang
Quoted 54 lines · Chunyan Zhang

From: Chunyan Zhang <[email protected]>

This patch only adds display iommu support, the driver was tested with sprd dpu and image codec processor.

The iommu support for others would be added once finished tests with those devices, such as a few signal processors, including VSP(video), GSP(graphic), ISP(image), and camera CPP, etc.

---
  drivers/iommu/Kconfig      |  12 +
  drivers/iommu/Makefile     |   1 +
  drivers/iommu/sprd-iommu.c | 566 +++++++++++++++++++++++++++++++++++++
  3 files changed, 579 insertions(+)

create mode 100644 drivers/iommu/sprd-iommu.c

drivers/iommu/Kconfig +12 -0
index 192ef8f61310..79af62c519ae 100644
@@ -408,4 +408,16 @@ config VIRTIO_IOMMU
 
  	  Say Y here if you intend to run this kernel as a guest.
 
+config SPRD_IOMMU
+	tristate "Unisoc IOMMU Support"
+	depends on ARCH_SPRD || COMPILE_TEST
+	select IOMMU_API
+	help
+	  Support for IOMMU on Unisoc's SoCs on which multi-media subsystems
+	  need IOMMU, such as DPU, Image codec(jpeg) processor, and a few
+	  signal processors, including VSP(video), GSP(graphic), ISP(image), and
+	  CPP, etc.
+
+	  Say Y here if you want multi-media functions.
+
  endif # IOMMU_SUPPORT
drivers/iommu/Makefile +1 -0
index 61bd30cd8369..5925b6af2123 100644
@@ -28,3 +28,4 @@ obj-$(CONFIG_S390_IOMMU) += s390-iommu.o
  obj-$(CONFIG_HYPERV_IOMMU) += hyperv-iommu.o
  obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
  obj-$(CONFIG_IOMMU_SVA_LIB) += iommu-sva-lib.o
+obj-$(CONFIG_SPRD_IOMMU) += sprd-iommu.o
drivers/iommu/sprd-iommu.c new file +1 -0
new file mode 100644
index 000000000000..44cde44017fa
@@ -0,0 +1,566 @@
+// SPDX-License-Identifier: GPL-2.0-only

drivers/iommu/sprd-iommu.c line 11

Quoted 10 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+/*
+ * Unisoc IOMMU driver
+ *
+ * Copyright (C) 2020 Unisoc, Inc.
+ * Author: Chunyan Zhang <[email protected]>
+ */
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/dma-iommu.h>

You need <linux/dma-mapping.h> since you're using the DMA API.

Quoted 113 lines
drivers/iommu/sprd-iommu.c new file +113 -0
+#include <linux/errno.h>
+#include <linux/iommu.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/slab.h>
+
+/* SPRD IOMMU page is 4K size alignment */
+#define SPRD_IOMMU_PAGE_SHIFT	12
+#define SPRD_IOMMU_PAGE_SIZE	SZ_4K
+
+#define SPRD_EX_CFG		0x0
+#define SPRD_IOMMU_VAOR_BYPASS	BIT(4)
+#define SPRD_IOMMU_GATE_EN	BIT(1)
+#define SPRD_IOMMU_EN		BIT(0)
+#define SPRD_EX_UPDATE		0x4
+#define SPRD_EX_FIRST_VPN	0x8
+#define SPRD_EX_VPN_RANGE	0xc
+#define SPRD_EX_FIRST_PPN	0x10
+#define SPRD_EX_DEFAULT_PPN	0x14
+
+#define SPRD_IOMMU_VERSION	0x0
+#define SPRD_VERSION_MASK	GENMASK(15, 8)
+#define SPRD_VERSION_SHIFT	0x8
+#define SPRD_VAU_CFG		0x4
+#define SPRD_VAU_UPDATE		0x8
+#define SPRD_VAU_AUTH_CFG	0xc
+#define SPRD_VAU_FIRST_PPN	0x10
+#define SPRD_VAU_DEFAULT_PPN_RD	0x14
+#define SPRD_VAU_DEFAULT_PPN_WR	0x18
+#define SPRD_VAU_FIRST_VPN	0x1c
+#define SPRD_VAU_VPN_RANGE	0x20
+
+enum sprd_iommu_version {
+	SPRD_IOMMU_EX,
+	SPRD_IOMMU_VAU,
+};
+
+struct sprd_iommu_match_data {
+	unsigned long reg_offset;
+};
+
+/*
+ * struct sprd_iommu_device - high-level sprd iommu device representation,
+ * including hardware information and configuration, also driver data, etc
+ *
+ * @mdata:	hardware configuration and information
+ * @ver:	sprd iommu device version
+ * @prot_page:	protect page base address, data would be written to here
+ *		while translation fault
+ * @base:	mapped base address for accessing registers
+ * @dev:	pointer to basic device structure
+ * @iommu:	IOMMU core representation
+ * @group:	IOMMU group
+ */
+struct sprd_iommu_device {
+	const struct sprd_iommu_match_data *mdata;
+	enum sprd_iommu_version ver;
+	u32			*prot_page_va;
+	dma_addr_t		prot_page_pa;
+	void __iomem		*base;
+	struct device		*dev;
+	struct iommu_device	iommu;
+	struct iommu_group	*group;
+};
+
+struct sprd_iommu_domain {
+	spinlock_t		pgtlock; /* lock for page table */
+	struct iommu_domain	domain;
+	u32			*pgt_va; /* page table virtual address base */
+	dma_addr_t		pgt_pa; /* page table physical address base */
+	struct sprd_iommu_device	*sdev;
+};
+
+static const struct iommu_ops sprd_iommu_ops;
+
+static struct sprd_iommu_domain *to_sprd_domain(struct iommu_domain *dom)
+{
+	return container_of(dom, struct sprd_iommu_domain, domain);
+}
+
+static inline void
+sprd_iommu_writel(struct sprd_iommu_device *sdev, unsigned int reg, u32 val)
+{
+	writel_relaxed(val, sdev->base + sdev->mdata->reg_offset + reg);
+}
+
+static inline u32
+sprd_iommu_readl(struct sprd_iommu_device *sdev, unsigned int reg)
+{
+	return readl_relaxed(sdev->base + sdev->mdata->reg_offset + reg);
+}
+
+static inline void
+sprd_iommu_update_bits(struct sprd_iommu_device *sdev, unsigned int reg,
+		  u32 mask, u32 shift, u32 val)
+{
+	u32 t = sprd_iommu_readl(sdev, reg);
+
+	t = (t & (~(mask << shift))) | ((val & mask) << shift);
+	sprd_iommu_writel(sdev, reg, t);
+}
+
+static inline int
+sprd_iommu_get_version(struct sprd_iommu_device *sdev)
+{
+	int ver = (sprd_iommu_readl(sdev, SPRD_IOMMU_VERSION) &
+		   SPRD_VERSION_MASK) >> SPRD_VERSION_SHIFT;
+
+	switch (ver) {
+	case SPRD_IOMMU_EX:
+	case SPRD_IOMMU_VAU:
+		return ver;
+	default:

drivers/iommu/sprd-iommu.c line 134

Quoted 10 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+		return -EINVAL;
+	}
+}
+
+static size_t
+sprd_iommu_pgt_size(struct iommu_domain *domain)
+{
+	return (size_t)	(((domain->geometry.aperture_end -
+			   domain->geometry.aperture_start + 1) >>
+			  SPRD_IOMMU_PAGE_SHIFT) * 4);

Nit: it's *reasonably* obvious, but "sizeof(u32)" might be more foolproof than just "4".

Also the cast doesn't do anything that the implicit conversion in the return doesn't already.

Quoted 20 lines
drivers/iommu/sprd-iommu.c new file +20 -0
+}
+
+static struct iommu_domain *sprd_iommu_domain_alloc(unsigned int domain_type)
+{
+	struct sprd_iommu_domain *dom;
+
+	if (domain_type != IOMMU_DOMAIN_DMA && domain_type != IOMMU_DOMAIN_UNMANAGED)
+		return NULL;
+
+	dom = kzalloc(sizeof(*dom), GFP_KERNEL);
+	if (!dom)
+		return NULL;
+
+	if (iommu_get_dma_cookie(&dom->domain)) {
+		kfree(dom);
+		return NULL;
+	}
+
+	spin_lock_init(&dom->pgtlock);
+

drivers/iommu/sprd-iommu.c line 164

Quoted 10 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+	dom->domain.geometry.aperture_start = 0;
+	dom->domain.geometry.aperture_end = SZ_256M - 1;
+
+	return &dom->domain;
+}
+
+static void sprd_iommu_domain_free(struct iommu_domain *domain)
+{
+	struct sprd_iommu_domain *dom = to_sprd_domain(domain);
+

iommu_put_dma_cookie(domain);

Quoted 72 lines
drivers/iommu/sprd-iommu.c new file +72 -0
+	kfree(dom);
+}
+
+static void sprd_iommu_first_vpn(struct sprd_iommu_domain *dom)
+{
+	struct sprd_iommu_device *sdev = dom->sdev;
+	u32 val;
+	unsigned int reg;
+
+	if (sdev->ver == SPRD_IOMMU_EX)
+		reg = SPRD_EX_FIRST_VPN;
+	else
+		reg = SPRD_VAU_FIRST_VPN;
+
+	val = dom->domain.geometry.aperture_start >> SPRD_IOMMU_PAGE_SHIFT;
+	sprd_iommu_writel(sdev, reg, val);
+}
+
+static void sprd_iommu_vpn_range(struct sprd_iommu_domain *dom)
+{
+	struct sprd_iommu_device *sdev = dom->sdev;
+	u32 val;
+	unsigned int reg;
+
+	if (sdev->ver == SPRD_IOMMU_EX)
+		reg = SPRD_EX_VPN_RANGE;
+	else
+		reg = SPRD_VAU_VPN_RANGE;
+
+	val = (dom->domain.geometry.aperture_end -
+	       dom->domain.geometry.aperture_start) >> SPRD_IOMMU_PAGE_SHIFT;
+	sprd_iommu_writel(sdev, reg, val);
+}
+
+static void sprd_iommu_first_ppn(struct sprd_iommu_domain *dom)
+{
+	u32 val = dom->pgt_pa >> SPRD_IOMMU_PAGE_SHIFT;
+	struct sprd_iommu_device *sdev = dom->sdev;
+	unsigned int reg;
+
+	if (sdev->ver == SPRD_IOMMU_EX)
+		reg = SPRD_EX_FIRST_PPN;
+	else
+		reg = SPRD_VAU_FIRST_PPN;
+
+	sprd_iommu_writel(sdev, reg, val);
+}
+
+static void sprd_iommu_default_ppn(struct sprd_iommu_device *sdev)
+{
+	u32 val = sdev->prot_page_pa >> SPRD_IOMMU_PAGE_SHIFT;
+
+	if (sdev->ver == SPRD_IOMMU_EX) {
+		sprd_iommu_writel(sdev, SPRD_EX_DEFAULT_PPN, val);
+	} else if (sdev->ver == SPRD_IOMMU_VAU) {
+		sprd_iommu_writel(sdev, SPRD_VAU_DEFAULT_PPN_RD, val);
+		sprd_iommu_writel(sdev, SPRD_VAU_DEFAULT_PPN_WR, val);
+	}
+}
+
+static void sprd_iommu_hw_en(struct sprd_iommu_device *sdev, bool en)
+{
+	unsigned int reg_cfg;
+	u32 mask, val;
+
+	if (sdev->ver == SPRD_IOMMU_EX)
+		reg_cfg = SPRD_EX_CFG;
+	else
+		reg_cfg = SPRD_VAU_CFG;
+
+	/* enable mmu, clk gate, vaor bypass */
+	mask = SPRD_IOMMU_EN | SPRD_IOMMU_GATE_EN | SPRD_IOMMU_VAOR_BYPASS;

drivers/iommu/sprd-iommu.c line 246

Quoted 10 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+	val = en ? mask : 0;
+	sprd_iommu_update_bits(sdev, reg_cfg, mask, 0, val);
+}
+
+static int sprd_iommu_attach_device(struct iommu_domain *domain,
+				    struct device *dev)
+{
+	struct sprd_iommu_device *sdev = dev_iommu_priv_get(dev);
+	struct sprd_iommu_domain *dom = to_sprd_domain(domain);
+	size_t pgt_size = sprd_iommu_pgt_size(domain);

If you're only supporting a single device per domain you should bail out here if dom->sdev is already set.

Quoted 13 lines
drivers/iommu/sprd-iommu.c new file +13 -0
+	dom->pgt_va = dma_alloc_coherent(sdev->dev, pgt_size, &dom->pgt_pa, GFP_KERNEL);
+	if (!dom->pgt_va)
+		return -ENOMEM;
+
+	dom->sdev = sdev;
+
+	sprd_iommu_first_ppn(dom);
+	sprd_iommu_first_vpn(dom);
+	sprd_iommu_vpn_range(dom);
+	sprd_iommu_default_ppn(sdev);
+	sprd_iommu_hw_en(sdev, true);
+
+	return 0;

drivers/iommu/sprd-iommu.c line 270

Quoted 10 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+}
+
+static void sprd_iommu_detach_device(struct iommu_domain *domain,
+					     struct device *dev)
+{
+	struct sprd_iommu_domain *dom = to_sprd_domain(domain);
+	struct sprd_iommu_device *sdev = dom->sdev;
+	size_t pgt_size = sprd_iommu_pgt_size(domain);
+
+	dma_free_coherent(sdev->dev, pgt_size, dom->pgt_va, dom->pgt_pa);

sdev may be NULL here.

Quoted 16 lines
drivers/iommu/sprd-iommu.c new file +16 -0
+	sprd_iommu_hw_en(sdev, false);
+	dom->sdev = NULL;
+}
+
+static int sprd_iommu_map(struct iommu_domain *domain, unsigned long iova,
+			  phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
+{
+	struct sprd_iommu_domain *dom = to_sprd_domain(domain);
+	const struct sprd_iommu_match_data *mdata;
+	unsigned int page_num = size >> SPRD_IOMMU_PAGE_SHIFT;
+	unsigned long flags;
+	unsigned int i;
+	u32 *pgt_base_iova;
+	u32 pabase = (u32)paddr;
+	int map_size = 0;
+	unsigned long start = domain->geometry.aperture_start;

drivers/iommu/sprd-iommu.c line 296

Quoted 10 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+	unsigned long end = domain->geometry.aperture_end;
+
+	if (!dom->sdev) {
+		pr_err("No sprd_iommu_device attached to the domain\n");
+		return -EINVAL;
+	}
+
+	mdata = dom->sdev->mdata;
+	if (iova < start || (iova + size) > (end + 1)) {
+		dev_err(dom->sdev->dev, "(iova(0x%lx) + sixe(0x%lx)) are not in the range!\n",

%zx for printing size_t.

Quoted 5 lines
drivers/iommu/sprd-iommu.c new file +5 -0
+			iova, size);
+		return -EINVAL;
+	}
+
+	pgt_base_iova = dom->pgt_va + ((iova - start) >> SPRD_IOMMU_PAGE_SHIFT);

drivers/iommu/sprd-iommu.c line 311

Quoted 10 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+
+	spin_lock_irqsave(&dom->pgtlock, flags);
+	for (i = 0; i < page_num; i++) {
+		pgt_base_iova[i] = pabase >> SPRD_IOMMU_PAGE_SHIFT;
+		pabase += SPRD_IOMMU_PAGE_SIZE;
+		map_size += SPRD_IOMMU_PAGE_SIZE;
+	}
+	spin_unlock_irqrestore(&dom->pgtlock, flags);
+
+	return map_size == size ? 0 : -EEXIST;

Clearly this can never fail.

Quoted 53 lines
drivers/iommu/sprd-iommu.c new file +53 -0
+}
+
+static size_t sprd_iommu_unmap(struct iommu_domain *domain, unsigned long iova,
+			size_t size, struct iommu_iotlb_gather *iotlb_gather)
+{
+	struct sprd_iommu_domain *dom = to_sprd_domain(domain);
+	unsigned long flags;
+	u32 *pgt_base_iova;
+	unsigned int page_num = size >> SPRD_IOMMU_PAGE_SHIFT;
+	unsigned long start = domain->geometry.aperture_start;
+	unsigned long end = domain->geometry.aperture_end;
+
+	if (iova < start || (iova + size) > (end + 1))
+		return -EINVAL;
+
+	pgt_base_iova = dom->pgt_va + ((iova - start) >> SPRD_IOMMU_PAGE_SHIFT);
+
+	spin_lock_irqsave(&dom->pgtlock, flags);
+	memset(pgt_base_iova, 0, page_num * sizeof(u32));
+	spin_unlock_irqrestore(&dom->pgtlock, flags);
+
+	return 0;
+}
+
+static void sprd_iommu_sync_map(struct iommu_domain *domain)
+{
+	struct sprd_iommu_domain *dom = to_sprd_domain(domain);
+	unsigned int reg;
+
+	if (dom->sdev->ver == SPRD_IOMMU_EX)
+		reg = SPRD_EX_UPDATE;
+	else
+		reg = SPRD_VAU_UPDATE;
+
+	/* clear iommu TLB buffer after page table updated */
+	sprd_iommu_writel(dom->sdev, reg, 0xffffffff);
+}
+
+static void sprd_iommu_sync(struct iommu_domain *domain,
+				 struct iommu_iotlb_gather *iotlb_gather)
+{
+	sprd_iommu_sync_map(domain);
+}
+
+static phys_addr_t sprd_iommu_iova_to_phys(struct iommu_domain *domain,
+					   dma_addr_t iova)
+{
+	struct sprd_iommu_domain *dom = to_sprd_domain(domain);
+	unsigned long flags;
+	phys_addr_t pa;
+	unsigned long start = domain->geometry.aperture_start;
+	unsigned long end = domain->geometry.aperture_end;
+

drivers/iommu/sprd-iommu.c line 374

Quoted 10 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+	if (iova < start || iova > end)
+		pr_err("iova (0x%llx) exceed the vpn range[0x%lx-0x%lx]!\n",
+		       iova, start, end);
+
+	spin_lock_irqsave(&dom->pgtlock, flags);
+	pa = *(dom->pgt_va + ((iova - start) >> SPRD_IOMMU_PAGE_SHIFT));
+	pa = pa << SPRD_IOMMU_PAGE_SHIFT;
+	spin_unlock_irqrestore(&dom->pgtlock, flags);
+
+	return pa;

Don't forget to add back the offset - the input address isn't necessarily page-aligned (at least you don't have block mappings to worry about as well...)

Quoted 21 lines
drivers/iommu/sprd-iommu.c new file +21 -0
+}
+
+static struct iommu_device *sprd_iommu_probe_device(struct device *dev)
+{
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+	struct sprd_iommu_device *sdev;
+
+	if (!fwspec || fwspec->ops != &sprd_iommu_ops)
+		return ERR_PTR(-ENODEV);
+
+	sdev = dev_iommu_priv_get(dev);
+
+	return &sdev->iommu;
+}
+
+static void sprd_iommu_release_device(struct device *dev)
+{
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+
+	if (!fwspec || fwspec->ops != &sprd_iommu_ops)
+		return;

drivers/iommu/sprd-iommu.c line 405

Quoted 10 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+
+	iommu_fwspec_free(dev);
+}
+
+static struct iommu_group *sprd_iommu_device_group(struct device *dev)
+{
+	struct sprd_iommu_device *sdev = dev_iommu_priv_get(dev);
+
+	if (!sdev)
+		return ERR_PTR(-ENODEV);

No need for this check, since iommu_group_get_for_dev() can now only be called after you've already handled ->probe_device.

drivers/iommu/sprd-iommu.c line 416

Quoted 11 lines
drivers/iommu/sprd-iommu.c new file +11 -0
+
+	return iommu_group_ref_get(sdev->group);
+}
+
+static int sprd_iommu_of_xlate(struct device *dev, struct of_phandle_args *args)
+{
+	struct platform_device *pdev;
+
+	if (!dev_iommu_priv_get(dev)) {
+		pdev = of_find_device_by_node(args->np);
+		dev_iommu_priv_set(dev, platform_get_drvdata(pdev));

You leak a reference on pdev->dev here.

Quoted 20 lines
drivers/iommu/sprd-iommu.c new file +20 -0
+	}
+
+	return 0;
+}
+
+
+static const struct iommu_ops sprd_iommu_ops = {
+	.domain_alloc	= sprd_iommu_domain_alloc,
+	.domain_free	= sprd_iommu_domain_free,
+	.attach_dev	= sprd_iommu_attach_device,
+	.detach_dev	= sprd_iommu_detach_device,
+	.map		= sprd_iommu_map,
+	.unmap		= sprd_iommu_unmap,
+	.iotlb_sync_map	= sprd_iommu_sync_map,
+	.iotlb_sync	= sprd_iommu_sync,
+	.iova_to_phys	= sprd_iommu_iova_to_phys,
+	.probe_device	= sprd_iommu_probe_device,
+	.release_device	= sprd_iommu_release_device,
+	.device_group	= sprd_iommu_device_group,
+	.of_xlate	= sprd_iommu_of_xlate,

drivers/iommu/sprd-iommu.c line 446

Quoted 10 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+	.pgsize_bitmap	= ~0UL << SPRD_IOMMU_PAGE_SHIFT,
+};
+
+static const struct sprd_iommu_match_data sprd_iommu_disp = {
+	.reg_offset = 0x800,
+};
+
+static const struct sprd_iommu_match_data sprd_iommu_jpg = {
+	.reg_offset = 0x300,
+};

Shouldn't those just be part of the base address in the DT to begin with? The Rockchip IOMMUs, for example, are all over the place relative to the base of whichever media device they're embedded into, and they don't care.

FWIW you can still accommodate your debugging trick without needing an excuse for per-instance compatibles in the DT - you could match known base addresses at probe to assign your desired IOVA ranges, or dynamically assign an IOVA range to each new instance and keep track of who got what in debugfs, or wait until probe_device/attach_device and inspect the client device itself to see who you belong to.

Quoted 12 lines
drivers/iommu/sprd-iommu.c new file +12 -0
+
+static const struct of_device_id sprd_iommu_of_match[] = {
+	{ .compatible = "sprd,iommu-v1-disp",
+	  .data = &sprd_iommu_disp },
+	{ .compatible = "sprd,iommu-v1-jpg",
+	  .data = &sprd_iommu_jpg },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, sprd_iommu_of_match);
+
+static int sprd_iommu_clk_enable(struct device *dev)
+{

drivers/iommu/sprd-iommu.c line 468

Quoted 10 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+	struct clk *eb;
+
+	eb = of_clk_get(dev->of_node, 0);
+	if (IS_ERR_OR_NULL(eb))
+		return PTR_ERR(eb);
+
+	clk_prepare_enable(eb);
+
+	return 0;
+}

Unless you plan to add significant complexity to this very soon, I don't think it really needs a separate helper. Also shouldn't you relinquish this clock on probe failure and remove?

Quoted 31 lines
drivers/iommu/sprd-iommu.c new file +31 -0
+static int sprd_iommu_probe(struct platform_device *pdev)
+{
+	struct sprd_iommu_device *sdev;
+	struct device *dev = &pdev->dev;
+	int ret;
+
+	sdev = devm_kzalloc(dev, sizeof(*sdev), GFP_KERNEL);
+	if (!sdev)
+		return -ENOMEM;
+
+	sdev->base = devm_platform_ioremap_resource(pdev, 0);
+	sdev->mdata = device_get_match_data(dev);
+
+	sdev->prot_page_va = dma_alloc_coherent(dev, SPRD_IOMMU_PAGE_SIZE,
+						&sdev->prot_page_pa, GFP_KERNEL);
+	if (!sdev->prot_page_va)
+		return -ENOMEM;
+
+	platform_set_drvdata(pdev, sdev);
+	sdev->dev = dev;
+
+	/* All the client devices are in the same iommu-group */
+	sdev->group = iommu_group_alloc();
+	if (IS_ERR(sdev->group)) {
+		ret = PTR_ERR(sdev->group);
+		goto free_page;
+	}
+
+	ret = iommu_device_sysfs_add(&sdev->iommu, dev, NULL, dev_name(dev));
+	if (ret)
+		goto put_group;

drivers/iommu/sprd-iommu.c line 510

Quoted 10 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+
+	iommu_device_set_ops(&sdev->iommu, &sprd_iommu_ops);
+	iommu_device_set_fwnode(&sdev->iommu, &dev->of_node->fwnode);
+
+	ret = iommu_device_register(&sdev->iommu);
+	if (ret)
+		goto remove_sysfs;
+
+	if (!iommu_present(&platform_bus_type))
+		bus_set_iommu(&platform_bus_type,  &sprd_iommu_ops);

Nit: extra space after the comma.

drivers/iommu/sprd-iommu.c line 512

Quoted 2 lines
drivers/iommu/sprd-iommu.c new file +2 -0
+
+	/* access to some iommus are controlled by gate clock, others are not */

The binding doesn't say you can have clocks...

Quoted 17 lines
drivers/iommu/sprd-iommu.c new file +17 -0
+	sprd_iommu_clk_enable(dev);
+
+	ret = sprd_iommu_get_version(sdev);
+	if (ret < 0) {
+		dev_err(dev, "iommu version(%d) is invalid.\n", ret);
+		goto unregister_iommu;
+	}
+	sdev->ver = ret;
+
+	return 0;
+
+unregister_iommu:
+	iommu_device_unregister(&sdev->iommu);
+remove_sysfs:
+	iommu_device_sysfs_remove(&sdev->iommu);
+put_group:
+	iommu_group_put(sdev->group);

drivers/iommu/sprd-iommu.c line 539

Quoted 10 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+free_page:
+	dma_free_coherent(sdev->dev, SPRD_IOMMU_PAGE_SIZE, sdev->prot_page_va, sdev->prot_page_pa);
+	return ret;
+}
+
+static int sprd_iommu_remove(struct platform_device *pdev)
+{
+	struct sprd_iommu_device *sdev = platform_get_drvdata(pdev);
+
+	dma_free_coherent(sdev->dev, SPRD_IOMMU_PAGE_SIZE, sdev->prot_page_va, sdev->prot_page_pa);

Just to confirm, does the sprd_iommu_hw_en(sdev, false) call from sprd_iommu_detach_device() effectively guarantee that no more accesses can be made to prot_page after that?

Quoted 6 lines
drivers/iommu/sprd-iommu.c new file +6 -0
+	iommu_group_put(sdev->group);
+	sdev->group = NULL;
+
+	bus_set_iommu(&platform_bus_type, NULL);
+
+	platform_set_drvdata(pdev, NULL);

drivers/iommu/sprd-iommu.c line 556

Quoted 10 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+	iommu_device_sysfs_remove(&sdev->iommu);
+	iommu_device_unregister(&sdev->iommu);
+
+	return 0;
+}
+
+static struct platform_driver sprd_iommu_driver = {
+	.driver	= {
+		.name		= "sprd-iommu",
+		.of_match_table	= sprd_iommu_of_match,

You probably want ".suppress_bind_attrs = true" as well.

Robin.

drivers/iommu/sprd-iommu.c line 566

Quoted 11 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+
+	},
+	.probe	= sprd_iommu_probe,
+	.remove	= sprd_iommu_remove,
+};
+module_platform_driver(sprd_iommu_driver);
+
+MODULE_DESCRIPTION("IOMMU driver for Unisoc SoCs");
+MODULE_ALIAS("platform:sprd-iommu");
+MODULE_LICENSE("GPL v2");

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

author reviewer
#6 lore raw

Re: [PATCH v1 2/2] iommu: add Unisoc iommu basic driver

Quoted 450 lines · Robin Murphy
Quoted 54 lines · Chunyan Zhang

From: Chunyan Zhang <[email protected]>

This patch only adds display iommu support, the driver was tested with sprd dpu and image codec processor.

The iommu support for others would be added once finished tests with those devices, such as a few signal processors, including VSP(video), GSP(graphic), ISP(image), and camera CPP, etc.

---
  drivers/iommu/Kconfig      |  12 +
  drivers/iommu/Makefile     |   1 +
  drivers/iommu/sprd-iommu.c | 566 +++++++++++++++++++++++++++++++++++++
  3 files changed, 579 insertions(+)

create mode 100644 drivers/iommu/sprd-iommu.c

drivers/iommu/Kconfig +12 -0
index 192ef8f61310..79af62c519ae 100644
@@ -408,4 +408,16 @@ config VIRTIO_IOMMU
 
        Say Y here if you intend to run this kernel as a guest.
 
+config SPRD_IOMMU
+     tristate "Unisoc IOMMU Support"
+     depends on ARCH_SPRD || COMPILE_TEST
+     select IOMMU_API
+     help
+       Support for IOMMU on Unisoc's SoCs on which multi-media subsystems
+       need IOMMU, such as DPU, Image codec(jpeg) processor, and a few
+       signal processors, including VSP(video), GSP(graphic), ISP(image), and
+       CPP, etc.
+
+       Say Y here if you want multi-media functions.
+
  endif # IOMMU_SUPPORT
drivers/iommu/Makefile +1 -0
index 61bd30cd8369..5925b6af2123 100644
@@ -28,3 +28,4 @@ obj-$(CONFIG_S390_IOMMU) += s390-iommu.o
  obj-$(CONFIG_HYPERV_IOMMU) += hyperv-iommu.o
  obj-$(CONFIG_VIRTIO_IOMMU) += virtio-iommu.o
  obj-$(CONFIG_IOMMU_SVA_LIB) += iommu-sva-lib.o
+obj-$(CONFIG_SPRD_IOMMU) += sprd-iommu.o
drivers/iommu/sprd-iommu.c new file +1 -0
new file mode 100644
index 000000000000..44cde44017fa
@@ -0,0 +1,566 @@
+// SPDX-License-Identifier: GPL-2.0-only

drivers/iommu/sprd-iommu.c line 11

Quoted 10 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+/*
+ * Unisoc IOMMU driver
+ *
+ * Copyright (C) 2020 Unisoc, Inc.
+ * Author: Chunyan Zhang <[email protected]>
+ */
+
+#include <linux/clk.h>
+#include <linux/device.h>
+#include <linux/dma-iommu.h>

You need <linux/dma-mapping.h> since you're using the DMA API.

Quoted 113 lines
drivers/iommu/sprd-iommu.c new file +113 -0
+#include <linux/errno.h>
+#include <linux/iommu.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/slab.h>
+
+/* SPRD IOMMU page is 4K size alignment */
+#define SPRD_IOMMU_PAGE_SHIFT        12
+#define SPRD_IOMMU_PAGE_SIZE SZ_4K
+
+#define SPRD_EX_CFG          0x0
+#define SPRD_IOMMU_VAOR_BYPASS       BIT(4)
+#define SPRD_IOMMU_GATE_EN   BIT(1)
+#define SPRD_IOMMU_EN                BIT(0)
+#define SPRD_EX_UPDATE               0x4
+#define SPRD_EX_FIRST_VPN    0x8
+#define SPRD_EX_VPN_RANGE    0xc
+#define SPRD_EX_FIRST_PPN    0x10
+#define SPRD_EX_DEFAULT_PPN  0x14
+
+#define SPRD_IOMMU_VERSION   0x0
+#define SPRD_VERSION_MASK    GENMASK(15, 8)
+#define SPRD_VERSION_SHIFT   0x8
+#define SPRD_VAU_CFG         0x4
+#define SPRD_VAU_UPDATE              0x8
+#define SPRD_VAU_AUTH_CFG    0xc
+#define SPRD_VAU_FIRST_PPN   0x10
+#define SPRD_VAU_DEFAULT_PPN_RD      0x14
+#define SPRD_VAU_DEFAULT_PPN_WR      0x18
+#define SPRD_VAU_FIRST_VPN   0x1c
+#define SPRD_VAU_VPN_RANGE   0x20
+
+enum sprd_iommu_version {
+     SPRD_IOMMU_EX,
+     SPRD_IOMMU_VAU,
+};
+
+struct sprd_iommu_match_data {
+     unsigned long reg_offset;
+};
+
+/*
+ * struct sprd_iommu_device - high-level sprd iommu device representation,
+ * including hardware information and configuration, also driver data, etc
+ *
+ * @mdata:   hardware configuration and information
+ * @ver:     sprd iommu device version
+ * @prot_page:       protect page base address, data would be written to here
+ *           while translation fault
+ * @base:    mapped base address for accessing registers
+ * @dev:     pointer to basic device structure
+ * @iommu:   IOMMU core representation
+ * @group:   IOMMU group
+ */
+struct sprd_iommu_device {
+     const struct sprd_iommu_match_data *mdata;
+     enum sprd_iommu_version ver;
+     u32                     *prot_page_va;
+     dma_addr_t              prot_page_pa;
+     void __iomem            *base;
+     struct device           *dev;
+     struct iommu_device     iommu;
+     struct iommu_group      *group;
+};
+
+struct sprd_iommu_domain {
+     spinlock_t              pgtlock; /* lock for page table */
+     struct iommu_domain     domain;
+     u32                     *pgt_va; /* page table virtual address base */
+     dma_addr_t              pgt_pa; /* page table physical address base */
+     struct sprd_iommu_device        *sdev;
+};
+
+static const struct iommu_ops sprd_iommu_ops;
+
+static struct sprd_iommu_domain *to_sprd_domain(struct iommu_domain *dom)
+{
+     return container_of(dom, struct sprd_iommu_domain, domain);
+}
+
+static inline void
+sprd_iommu_writel(struct sprd_iommu_device *sdev, unsigned int reg, u32 val)
+{
+     writel_relaxed(val, sdev->base + sdev->mdata->reg_offset + reg);
+}
+
+static inline u32
+sprd_iommu_readl(struct sprd_iommu_device *sdev, unsigned int reg)
+{
+     return readl_relaxed(sdev->base + sdev->mdata->reg_offset + reg);
+}
+
+static inline void
+sprd_iommu_update_bits(struct sprd_iommu_device *sdev, unsigned int reg,
+               u32 mask, u32 shift, u32 val)
+{
+     u32 t = sprd_iommu_readl(sdev, reg);
+
+     t = (t & (~(mask << shift))) | ((val & mask) << shift);
+     sprd_iommu_writel(sdev, reg, t);
+}
+
+static inline int
+sprd_iommu_get_version(struct sprd_iommu_device *sdev)
+{
+     int ver = (sprd_iommu_readl(sdev, SPRD_IOMMU_VERSION) &
+                SPRD_VERSION_MASK) >> SPRD_VERSION_SHIFT;
+
+     switch (ver) {
+     case SPRD_IOMMU_EX:
+     case SPRD_IOMMU_VAU:
+             return ver;
+     default:

drivers/iommu/sprd-iommu.c line 134

Quoted 10 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+             return -EINVAL;
+     }
+}
+
+static size_t
+sprd_iommu_pgt_size(struct iommu_domain *domain)
+{
+     return (size_t) (((domain->geometry.aperture_end -
+                        domain->geometry.aperture_start + 1) >>
+                       SPRD_IOMMU_PAGE_SHIFT) * 4);

Nit: it's *reasonably* obvious, but "sizeof(u32)" might be more foolproof than just "4".

Also the cast doesn't do anything that the implicit conversion in the return doesn't already.

Quoted 20 lines
drivers/iommu/sprd-iommu.c new file +20 -0
+}
+
+static struct iommu_domain *sprd_iommu_domain_alloc(unsigned int domain_type)
+{
+     struct sprd_iommu_domain *dom;
+
+     if (domain_type != IOMMU_DOMAIN_DMA && domain_type != IOMMU_DOMAIN_UNMANAGED)
+             return NULL;
+
+     dom = kzalloc(sizeof(*dom), GFP_KERNEL);
+     if (!dom)
+             return NULL;
+
+     if (iommu_get_dma_cookie(&dom->domain)) {
+             kfree(dom);
+             return NULL;
+     }
+
+     spin_lock_init(&dom->pgtlock);
+

drivers/iommu/sprd-iommu.c line 164

Quoted 10 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+     dom->domain.geometry.aperture_start = 0;
+     dom->domain.geometry.aperture_end = SZ_256M - 1;
+
+     return &dom->domain;
+}
+
+static void sprd_iommu_domain_free(struct iommu_domain *domain)
+{
+     struct sprd_iommu_domain *dom = to_sprd_domain(domain);
+

iommu_put_dma_cookie(domain);

Quoted 72 lines
drivers/iommu/sprd-iommu.c new file +72 -0
+     kfree(dom);
+}
+
+static void sprd_iommu_first_vpn(struct sprd_iommu_domain *dom)
+{
+     struct sprd_iommu_device *sdev = dom->sdev;
+     u32 val;
+     unsigned int reg;
+
+     if (sdev->ver == SPRD_IOMMU_EX)
+             reg = SPRD_EX_FIRST_VPN;
+     else
+             reg = SPRD_VAU_FIRST_VPN;
+
+     val = dom->domain.geometry.aperture_start >> SPRD_IOMMU_PAGE_SHIFT;
+     sprd_iommu_writel(sdev, reg, val);
+}
+
+static void sprd_iommu_vpn_range(struct sprd_iommu_domain *dom)
+{
+     struct sprd_iommu_device *sdev = dom->sdev;
+     u32 val;
+     unsigned int reg;
+
+     if (sdev->ver == SPRD_IOMMU_EX)
+             reg = SPRD_EX_VPN_RANGE;
+     else
+             reg = SPRD_VAU_VPN_RANGE;
+
+     val = (dom->domain.geometry.aperture_end -
+            dom->domain.geometry.aperture_start) >> SPRD_IOMMU_PAGE_SHIFT;
+     sprd_iommu_writel(sdev, reg, val);
+}
+
+static void sprd_iommu_first_ppn(struct sprd_iommu_domain *dom)
+{
+     u32 val = dom->pgt_pa >> SPRD_IOMMU_PAGE_SHIFT;
+     struct sprd_iommu_device *sdev = dom->sdev;
+     unsigned int reg;
+
+     if (sdev->ver == SPRD_IOMMU_EX)
+             reg = SPRD_EX_FIRST_PPN;
+     else
+             reg = SPRD_VAU_FIRST_PPN;
+
+     sprd_iommu_writel(sdev, reg, val);
+}
+
+static void sprd_iommu_default_ppn(struct sprd_iommu_device *sdev)
+{
+     u32 val = sdev->prot_page_pa >> SPRD_IOMMU_PAGE_SHIFT;
+
+     if (sdev->ver == SPRD_IOMMU_EX) {
+             sprd_iommu_writel(sdev, SPRD_EX_DEFAULT_PPN, val);
+     } else if (sdev->ver == SPRD_IOMMU_VAU) {
+             sprd_iommu_writel(sdev, SPRD_VAU_DEFAULT_PPN_RD, val);
+             sprd_iommu_writel(sdev, SPRD_VAU_DEFAULT_PPN_WR, val);
+     }
+}
+
+static void sprd_iommu_hw_en(struct sprd_iommu_device *sdev, bool en)
+{
+     unsigned int reg_cfg;
+     u32 mask, val;
+
+     if (sdev->ver == SPRD_IOMMU_EX)
+             reg_cfg = SPRD_EX_CFG;
+     else
+             reg_cfg = SPRD_VAU_CFG;
+
+     /* enable mmu, clk gate, vaor bypass */
+     mask = SPRD_IOMMU_EN | SPRD_IOMMU_GATE_EN | SPRD_IOMMU_VAOR_BYPASS;

drivers/iommu/sprd-iommu.c line 246

Quoted 10 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+     val = en ? mask : 0;
+     sprd_iommu_update_bits(sdev, reg_cfg, mask, 0, val);
+}
+
+static int sprd_iommu_attach_device(struct iommu_domain *domain,
+                                 struct device *dev)
+{
+     struct sprd_iommu_device *sdev = dev_iommu_priv_get(dev);
+     struct sprd_iommu_domain *dom = to_sprd_domain(domain);
+     size_t pgt_size = sprd_iommu_pgt_size(domain);

If you're only supporting a single device per domain you should bail out here if dom->sdev is already set.

Quoted 13 lines
drivers/iommu/sprd-iommu.c new file +13 -0
+     dom->pgt_va = dma_alloc_coherent(sdev->dev, pgt_size, &dom->pgt_pa, GFP_KERNEL);
+     if (!dom->pgt_va)
+             return -ENOMEM;
+
+     dom->sdev = sdev;
+
+     sprd_iommu_first_ppn(dom);
+     sprd_iommu_first_vpn(dom);
+     sprd_iommu_vpn_range(dom);
+     sprd_iommu_default_ppn(sdev);
+     sprd_iommu_hw_en(sdev, true);
+
+     return 0;

drivers/iommu/sprd-iommu.c line 270

Quoted 10 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+}
+
+static void sprd_iommu_detach_device(struct iommu_domain *domain,
+                                          struct device *dev)
+{
+     struct sprd_iommu_domain *dom = to_sprd_domain(domain);
+     struct sprd_iommu_device *sdev = dom->sdev;
+     size_t pgt_size = sprd_iommu_pgt_size(domain);
+
+     dma_free_coherent(sdev->dev, pgt_size, dom->pgt_va, dom->pgt_pa);

sdev may be NULL here.

Quoted 16 lines
drivers/iommu/sprd-iommu.c new file +16 -0
+     sprd_iommu_hw_en(sdev, false);
+     dom->sdev = NULL;
+}
+
+static int sprd_iommu_map(struct iommu_domain *domain, unsigned long iova,
+                       phys_addr_t paddr, size_t size, int prot, gfp_t gfp)
+{
+     struct sprd_iommu_domain *dom = to_sprd_domain(domain);
+     const struct sprd_iommu_match_data *mdata;
+     unsigned int page_num = size >> SPRD_IOMMU_PAGE_SHIFT;
+     unsigned long flags;
+     unsigned int i;
+     u32 *pgt_base_iova;
+     u32 pabase = (u32)paddr;
+     int map_size = 0;
+     unsigned long start = domain->geometry.aperture_start;

drivers/iommu/sprd-iommu.c line 296

Quoted 10 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+     unsigned long end = domain->geometry.aperture_end;
+
+     if (!dom->sdev) {
+             pr_err("No sprd_iommu_device attached to the domain\n");
+             return -EINVAL;
+     }
+
+     mdata = dom->sdev->mdata;
+     if (iova < start || (iova + size) > (end + 1)) {
+             dev_err(dom->sdev->dev, "(iova(0x%lx) + sixe(0x%lx)) are not in the range!\n",

%zx for printing size_t.

Quoted 5 lines
drivers/iommu/sprd-iommu.c new file +5 -0
+                     iova, size);
+             return -EINVAL;
+     }
+
+     pgt_base_iova = dom->pgt_va + ((iova - start) >> SPRD_IOMMU_PAGE_SHIFT);

drivers/iommu/sprd-iommu.c line 311

Quoted 10 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+
+     spin_lock_irqsave(&dom->pgtlock, flags);
+     for (i = 0; i < page_num; i++) {
+             pgt_base_iova[i] = pabase >> SPRD_IOMMU_PAGE_SHIFT;
+             pabase += SPRD_IOMMU_PAGE_SIZE;
+             map_size += SPRD_IOMMU_PAGE_SIZE;
+     }
+     spin_unlock_irqrestore(&dom->pgtlock, flags);
+
+     return map_size == size ? 0 : -EEXIST;

Clearly this can never fail.

Quoted 59 lines
drivers/iommu/sprd-iommu.c new file +59 -0
+}
+
+static size_t sprd_iommu_unmap(struct iommu_domain *domain, unsigned long iova,
+                     size_t size, struct iommu_iotlb_gather *iotlb_gather)
+{
+     struct sprd_iommu_domain *dom = to_sprd_domain(domain);
+     unsigned long flags;
+     u32 *pgt_base_iova;
+     unsigned int page_num = size >> SPRD_IOMMU_PAGE_SHIFT;
+     unsigned long start = domain->geometry.aperture_start;
+     unsigned long end = domain->geometry.aperture_end;
+
+     if (iova < start || (iova + size) > (end + 1))
+             return -EINVAL;
+
+     pgt_base_iova = dom->pgt_va + ((iova - start) >> SPRD_IOMMU_PAGE_SHIFT);
+
+     spin_lock_irqsave(&dom->pgtlock, flags);
+     memset(pgt_base_iova, 0, page_num * sizeof(u32));
+     spin_unlock_irqrestore(&dom->pgtlock, flags);
+
+     return 0;
+}
+
+static void sprd_iommu_sync_map(struct iommu_domain *domain)
+{
+     struct sprd_iommu_domain *dom = to_sprd_domain(domain);
+     unsigned int reg;
+
+     if (dom->sdev->ver == SPRD_IOMMU_EX)
+             reg = SPRD_EX_UPDATE;
+     else
+             reg = SPRD_VAU_UPDATE;
+
+     /* clear iommu TLB buffer after page table updated */
+     sprd_iommu_writel(dom->sdev, reg, 0xffffffff);
+}
+
+static void sprd_iommu_sync(struct iommu_domain *domain,
+                              struct iommu_iotlb_gather *iotlb_gather)
+{
+     sprd_iommu_sync_map(domain);
+}
+
+static phys_addr_t sprd_iommu_iova_to_phys(struct iommu_domain *domain,
+                                        dma_addr_t iova)
+{
+     struct sprd_iommu_domain *dom = to_sprd_domain(domain);
+     unsigned long flags;
+     phys_addr_t pa;
+     unsigned long start = domain->geometry.aperture_start;
+     unsigned long end = domain->geometry.aperture_end;
+
+     if (iova < start || iova > end)
+             pr_err("iova (0x%llx) exceed the vpn range[0x%lx-0x%lx]!\n",
+                    iova, start, end);
+
+     spin_lock_irqsave(&dom->pgtlock, flags);
+     pa = *(dom->pgt_va + ((iova - start) >> SPRD_IOMMU_PAGE_SHIFT));
Quoted 8 lines

drivers/iommu/sprd-iommu.c line 374

Quoted 4 lines
drivers/iommu/sprd-iommu.c new file +4 -0
+     pa = pa << SPRD_IOMMU_PAGE_SHIFT;
+     spin_unlock_irqrestore(&dom->pgtlock, flags);
+
+     return pa;

Don't forget to add back the offset - the input address isn't necessarily page-aligned (at least you don't have block mappings to worry about as well...)

Ok.

Quoted 36 lines
Quoted 22 lines
drivers/iommu/sprd-iommu.c new file +21 -0
+}
+
+static struct iommu_device *sprd_iommu_probe_device(struct device *dev)
+{
+     struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+     struct sprd_iommu_device *sdev;
+
+     if (!fwspec || fwspec->ops != &sprd_iommu_ops)
+             return ERR_PTR(-ENODEV);
+
+     sdev = dev_iommu_priv_get(dev);
+
+     return &sdev->iommu;
+}
+
+static void sprd_iommu_release_device(struct device *dev)
+{
+     struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+
+     if (!fwspec || fwspec->ops != &sprd_iommu_ops)
+             return;

drivers/iommu/sprd-iommu.c line 405

Quoted 10 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+
+     iommu_fwspec_free(dev);
+}
+
+static struct iommu_group *sprd_iommu_device_group(struct device *dev)
+{
+     struct sprd_iommu_device *sdev = dev_iommu_priv_get(dev);
+
+     if (!sdev)
+             return ERR_PTR(-ENODEV);

No need for this check, since iommu_group_get_for_dev() can now only be called after you've already handled ->probe_device.

Quoted 13 lines

drivers/iommu/sprd-iommu.c line 416

Quoted 11 lines
drivers/iommu/sprd-iommu.c new file +11 -0
+
+     return iommu_group_ref_get(sdev->group);
+}
+
+static int sprd_iommu_of_xlate(struct device *dev, struct of_phandle_args *args)
+{
+     struct platform_device *pdev;
+
+     if (!dev_iommu_priv_get(dev)) {
+             pdev = of_find_device_by_node(args->np);
+             dev_iommu_priv_set(dev, platform_get_drvdata(pdev));

You leak a reference on pdev->dev here.

Will fix it in the next version.

Quoted 28 lines
Quoted 28 lines
drivers/iommu/sprd-iommu.c new file +27 -0
+     }
+
+     return 0;
+}
+
+
+static const struct iommu_ops sprd_iommu_ops = {
+     .domain_alloc   = sprd_iommu_domain_alloc,
+     .domain_free    = sprd_iommu_domain_free,
+     .attach_dev     = sprd_iommu_attach_device,
+     .detach_dev     = sprd_iommu_detach_device,
+     .map            = sprd_iommu_map,
+     .unmap          = sprd_iommu_unmap,
+     .iotlb_sync_map = sprd_iommu_sync_map,
+     .iotlb_sync     = sprd_iommu_sync,
+     .iova_to_phys   = sprd_iommu_iova_to_phys,
+     .probe_device   = sprd_iommu_probe_device,
+     .release_device = sprd_iommu_release_device,
+     .device_group   = sprd_iommu_device_group,
+     .of_xlate       = sprd_iommu_of_xlate,
+     .pgsize_bitmap  = ~0UL << SPRD_IOMMU_PAGE_SHIFT,
+};
+
+static const struct sprd_iommu_match_data sprd_iommu_disp = {
+     .reg_offset = 0x800,
+};
+
Quoted 8 lines

drivers/iommu/sprd-iommu.c line 446

Quoted 3 lines
drivers/iommu/sprd-iommu.c new file +3 -0
+static const struct sprd_iommu_match_data sprd_iommu_jpg = {
+     .reg_offset = 0x300,
+};

Shouldn't those just be part of the base address in the DT to begin with? The Rockchip IOMMUs, for example, are all over the place relative to the base of whichever media device they're embedded into, and they don't care.

I'm considering moving the iommu DT node under into the DT node of the media device which uses this iommu.

Quoted 7 lines

FWIW you can still accommodate your debugging trick without needing an excuse for per-instance compatibles in the DT - you could match known base addresses at probe to assign your desired IOVA ranges, or dynamically assign an IOVA range to each new instance and keep track of who got what in debugfs, or wait until probe_device/attach_device and inspect the client device itself to see who you belong to.

I considered letting the media devices set IOVA and range, I would also set a default IOVA start address when allocating the iommu domain.

Quoted 19 lines
Quoted 19 lines
drivers/iommu/sprd-iommu.c new file +18 -0
+
+static const struct of_device_id sprd_iommu_of_match[] = {
+     { .compatible = "sprd,iommu-v1-disp",
+       .data = &sprd_iommu_disp },
+     { .compatible = "sprd,iommu-v1-jpg",
+       .data = &sprd_iommu_jpg },
+     { },
+};
+MODULE_DEVICE_TABLE(of, sprd_iommu_of_match);
+
+static int sprd_iommu_clk_enable(struct device *dev)
+{
+     struct clk *eb;
+
+     eb = of_clk_get(dev->of_node, 0);
+     if (IS_ERR_OR_NULL(eb))
+             return PTR_ERR(eb);
+
Quoted 8 lines

drivers/iommu/sprd-iommu.c line 468

Quoted 4 lines
drivers/iommu/sprd-iommu.c new file +4 -0
+     clk_prepare_enable(eb);
+
+     return 0;
+}

Unless you plan to add significant complexity to this very soon, I don't think it really needs a separate helper. Also shouldn't you relinquish this clock on probe failure and remove?

I meant to do that since the clock is optional, I added comments in probe(), I should add comments here.
Also I found I should use clk_get_optional() to get the clock reference.

Quoted 45 lines
Quoted 32 lines
drivers/iommu/sprd-iommu.c new file +31 -0
+static int sprd_iommu_probe(struct platform_device *pdev)
+{
+     struct sprd_iommu_device *sdev;
+     struct device *dev = &pdev->dev;
+     int ret;
+
+     sdev = devm_kzalloc(dev, sizeof(*sdev), GFP_KERNEL);
+     if (!sdev)
+             return -ENOMEM;
+
+     sdev->base = devm_platform_ioremap_resource(pdev, 0);
+     sdev->mdata = device_get_match_data(dev);
+
+     sdev->prot_page_va = dma_alloc_coherent(dev, SPRD_IOMMU_PAGE_SIZE,
+                                             &sdev->prot_page_pa, GFP_KERNEL);
+     if (!sdev->prot_page_va)
+             return -ENOMEM;
+
+     platform_set_drvdata(pdev, sdev);
+     sdev->dev = dev;
+
+     /* All the client devices are in the same iommu-group */
+     sdev->group = iommu_group_alloc();
+     if (IS_ERR(sdev->group)) {
+             ret = PTR_ERR(sdev->group);
+             goto free_page;
+     }
+
+     ret = iommu_device_sysfs_add(&sdev->iommu, dev, NULL, dev_name(dev));
+     if (ret)
+             goto put_group;

drivers/iommu/sprd-iommu.c line 510

Quoted 10 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+
+     iommu_device_set_ops(&sdev->iommu, &sprd_iommu_ops);
+     iommu_device_set_fwnode(&sdev->iommu, &dev->of_node->fwnode);
+
+     ret = iommu_device_register(&sdev->iommu);
+     if (ret)
+             goto remove_sysfs;
+
+     if (!iommu_present(&platform_bus_type))
+             bus_set_iommu(&platform_bus_type,  &sprd_iommu_ops);

Nit: extra space after the comma.

Quoted 4 lines

drivers/iommu/sprd-iommu.c line 512

Quoted 2 lines
drivers/iommu/sprd-iommu.c new file +2 -0
+
+     /* access to some iommus are controlled by gate clock, others are not */

The binding doesn't say you can have clocks...

Forgot to add clock property..

Quoted 24 lines
Quoted 24 lines
drivers/iommu/sprd-iommu.c new file +23 -0
+     sprd_iommu_clk_enable(dev);
+
+     ret = sprd_iommu_get_version(sdev);
+     if (ret < 0) {
+             dev_err(dev, "iommu version(%d) is invalid.\n", ret);
+             goto unregister_iommu;
+     }
+     sdev->ver = ret;
+
+     return 0;
+
+unregister_iommu:
+     iommu_device_unregister(&sdev->iommu);
+remove_sysfs:
+     iommu_device_sysfs_remove(&sdev->iommu);
+put_group:
+     iommu_group_put(sdev->group);
+free_page:
+     dma_free_coherent(sdev->dev, SPRD_IOMMU_PAGE_SIZE, sdev->prot_page_va, sdev->prot_page_pa);
+     return ret;
+}
+
+static int sprd_iommu_remove(struct platform_device *pdev)
Quoted 8 lines

drivers/iommu/sprd-iommu.c line 539

Quoted 4 lines
drivers/iommu/sprd-iommu.c new file +4 -0
+{
+     struct sprd_iommu_device *sdev = platform_get_drvdata(pdev);
+
+     dma_free_coherent(sdev->dev, SPRD_IOMMU_PAGE_SIZE, sdev->prot_page_va, sdev->prot_page_pa);

Just to confirm, does the sprd_iommu_hw_en(sdev, false) call from sprd_iommu_detach_device() effectively guarantee that no more accesses can be made to prot_page after that?

From what I understand, prot_page is used for the case that if the virtual address is in the range of iova addresses, but not mapped to a physical address, then prot_page was provided for accessing, that would happen only if iommu is enabled.

Back to your question, prot_page wouldn't be accessed any more after iommu being disabled by calling sprd_iommu_hw_en(sdev, false).

Quoted 11 lines
Quoted 11 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+     iommu_group_put(sdev->group);
+     sdev->group = NULL;
+
+     bus_set_iommu(&platform_bus_type, NULL);
+
+     platform_set_drvdata(pdev, NULL);
+     iommu_device_sysfs_remove(&sdev->iommu);
+     iommu_device_unregister(&sdev->iommu);
+
+     return 0;
Quoted 8 lines

drivers/iommu/sprd-iommu.c line 556

Quoted 6 lines
drivers/iommu/sprd-iommu.c new file +6 -0
+}
+
+static struct platform_driver sprd_iommu_driver = {
+     .driver = {
+             .name           = "sprd-iommu",
+             .of_match_table = sprd_iommu_of_match,

You probably want ".suppress_bind_attrs = true" as well.

Thanks for your comments, I will address them in the next version.

Chunyan

Quoted 14 lines

Robin.

drivers/iommu/sprd-iommu.c line 566

Quoted 11 lines
drivers/iommu/sprd-iommu.c new file +10 -0
+
+     },
+     .probe  = sprd_iommu_probe,
+     .remove = sprd_iommu_remove,
+};
+module_platform_driver(sprd_iommu_driver);
+
+MODULE_DESCRIPTION("IOMMU driver for Unisoc SoCs");
+MODULE_ALIAS("platform:sprd-iommu");
+MODULE_LICENSE("GPL v2");

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu