LinuxLists.cc - [PATCH v3 0/2] Add Unisoc iommu basic driver

2021-02-03 09:12:29

Subject: [PATCH v3 0/2] Add Unisoc iommu basic driver

From: Chunyan Zhang <[email protected]>

Changes since v2:
* Added a WARN and return 0 if an invalid iova was passed to sprd_iommu_iova_to_phys();
* Changed the name of sprd_iommu_write();
* Revised CONFIG_SPRD_IOMMU help graph in Kconfig.
* Revised comments for the struct sprd_iommu_device;
* Converted to use "GPL" instread of "GPL v2", they are same as license-rules.rst shows.

Changes since v1:
* Fixed compile errors reported by kernel test robot <[email protected]>.
* Changed to use syscon to get mapped registers for iommu and media devices to avoid double map issue.
* Addressed Robin's comments:
- Added including offset in the returned physical address if the input virtual address isn't page-aligned;
- Added platform_device_put() after calling of_find_device_by_node();
- Removed iommu register offset from driver, it will be defined as the cell of DT reference to syscon phandle;
- Removed multi compatible strings which are not needed;
- Added comments for the function sprd_iommu_clk_enable();
- Added clocks property in bindings;
- Set device_driver.suppress_bind_attrs to disable unbind the devices via sysfs;
- A few trivial fixes.

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 | 72 +++
drivers/iommu/Kconfig | 12 +
drivers/iommu/Makefile | 1 +
drivers/iommu/sprd-iommu.c | 600 ++++++++++++++++++
4 files changed, 685 insertions(+)
create mode 100644 Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
create mode 100644 drivers/iommu/sprd-iommu.c

--
2.25.1

2021-02-03 09:13:37

by Chunyan Zhang

[permalink] [raw]

Subject: [PATCH v3 1/2] dt-bindings: iommu: add bindings for sprd iommu

2021-02-03 09:14:30

by Chunyan Zhang

[permalink] [raw]

Subject: [PATCH v3 2/2] iommu: add Unisoc iommu basic driver

From: Chunyan Zhang <[email protected]>

This iommu module can be used by Unisoc's multimedia devices, such as
display, Image codec(jpeg) and a few signal processors, including
VSP(video), GSP(graphic), ISP(image), and CPP(camera pixel processor), etc.

Signed-off-by: Chunyan Zhang <[email protected]>
---
drivers/iommu/Kconfig | 12 +
drivers/iommu/Makefile | 1 +
drivers/iommu/sprd-iommu.c | 600 +++++++++++++++++++++++++++++++++++++
3 files changed, 613 insertions(+)
create mode 100644 drivers/iommu/sprd-iommu.c

diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 192ef8f61310..99e7712f3903 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -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, this iommu can be used by
+ Unisoc's multimedia devices, such as display, Image codec(jpeg)
+ and a few signal processors, including VSP(video), GSP(graphic),
+ ISP(image), and CPP(camera pixel processor), etc.
+
+ Say Y here if you want to use the multimedia devices listed above.
+
endif # IOMMU_SUPPORT
diff --git a/drivers/iommu/Makefile b/drivers/iommu/Makefile
index 61bd30cd8369..5925b6af2123 100644
--- a/drivers/iommu/Makefile
+++ b/drivers/iommu/Makefile
@@ -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
diff --git a/drivers/iommu/sprd-iommu.c b/drivers/iommu/sprd-iommu.c
new file mode 100644
index 000000000000..6b3c36785c7c
--- /dev/null
+++ b/drivers/iommu/sprd-iommu.c
@@ -0,0 +1,600 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * 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>
+#include <linux/dma-mapping.h>
+#include <linux/errno.h>
+#include <linux/iommu.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
+#include <linux/of_platform.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
+
+#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_device - high-level sprd iommu device representation,
+ * including hardware information and configuration, also driver data, etc
+ *
+ * @ver: sprd iommu device version
+ * @prot_page_va: protect page base virtual address
+ * @prot_page_pa: protect page base physical address, data would be
+ * written to here while translation fault
+ * @base: mapped base address for accessing registers
+ * @reg_offset: some IOMMU shares the same range of registers with the multimedia
+ * device which use it, this represents the iommu register offset
+ * @dev: pointer to basic device structure
+ * @iommu: IOMMU core representation
+ * @group: IOMMU group
+ * @eb: gate clock which controls iommu access
+ */
+struct sprd_iommu_device {
+ enum sprd_iommu_version ver;
+ u32 *prot_page_va;
+ dma_addr_t prot_page_pa;
+ struct regmap *base;
+ unsigned int reg_offset;
+ struct device *dev;
+ struct iommu_device iommu;
+ struct iommu_group *group;
+ struct clk *eb;
+};
+
+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_write(struct sprd_iommu_device *sdev, unsigned int reg, u32 val)
+{
+ regmap_write(sdev->base, sdev->reg_offset + reg, val);
+}
+
+static inline u32
+sprd_iommu_read(struct sprd_iommu_device *sdev, unsigned int reg)
+{
+ u32 val;
+
+ regmap_read(sdev->base, sdev->reg_offset + reg, &val);
+ return val;
+}
+
+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_read(sdev, reg);
+
+ t = (t & (~(mask << shift))) | ((val & mask) << shift);
+ sprd_iommu_write(sdev, reg, t);
+}
+
+static inline int
+sprd_iommu_get_version(struct sprd_iommu_device *sdev)
+{
+ int ver = (sprd_iommu_read(sdev, SPRD_IOMMU_VERSION) &
+ SPRD_VERSION_MASK) >> SPRD_VERSION_SHIFT;
+
+ switch (ver) {
+ case SPRD_IOMMU_EX:
+ case SPRD_IOMMU_VAU:
+ return ver;
+ default:
+ return -EINVAL;
+ }
+}
+
+static size_t
+sprd_iommu_pgt_size(struct iommu_domain *domain)
+{
+ return ((domain->geometry.aperture_end -
+ domain->geometry.aperture_start + 1) >>
+ SPRD_IOMMU_PAGE_SHIFT) * sizeof(u32);
+}
+
+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);
+
+ 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);
+ 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_write(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_write(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_write(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_write(sdev, SPRD_EX_DEFAULT_PPN, val);
+ } else if (sdev->ver == SPRD_IOMMU_VAU) {
+ sprd_iommu_write(sdev, SPRD_VAU_DEFAULT_PPN_RD, val);
+ sprd_iommu_write(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;
+
+ mask = SPRD_IOMMU_EN | SPRD_IOMMU_GATE_EN;
+ 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 (dom->sdev) {
+ pr_err("There's already a device attached to this domain.\n");
+ return -EINVAL;
+ }
+
+ 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;
+}
+
+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);
+
+ if (!sdev)
+ return;
+
+ dma_free_coherent(sdev->dev, pgt_size, dom->pgt_va, dom->pgt_pa);
+ 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);
+ unsigned int page_num = size >> SPRD_IOMMU_PAGE_SHIFT;
+ unsigned long flags;
+ unsigned int i;
+ u32 *pgt_base_iova;
+ u32 pabase = (u32)paddr;
+ unsigned long start = domain->geometry.aperture_start;
+ unsigned long end = domain->geometry.aperture_end;
+
+ if (!dom->sdev) {
+ pr_err("No sprd_iommu_device attached to the domain\n");
+ return -EINVAL;
+ }
+
+ if (iova < start || (iova + size) > (end + 1)) {
+ dev_err(dom->sdev->dev, "(iova(0x%lx) + sixe(%zx)) are not in the range!\n",
+ iova, size);
+ return -EINVAL;
+ }
+
+ pgt_base_iova = dom->pgt_va + ((iova - start) >> SPRD_IOMMU_PAGE_SHIFT);
+
+ 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;
+ }
+ spin_unlock_irqrestore(&dom->pgtlock, flags);
+
+ return 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_write(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 (WARN(iova < start || iova > end,
+ "iova (0x%llx) exceeds the vpn range[0x%lx-0x%lx]\n",
+ iova, start, end))
+ return 0;
+
+ spin_lock_irqsave(&dom->pgtlock, flags);
+ pa = *(dom->pgt_va + ((iova - start) >> SPRD_IOMMU_PAGE_SHIFT));
+ pa = (pa << SPRD_IOMMU_PAGE_SHIFT) + ((iova - start) & (SPRD_IOMMU_PAGE_SIZE - 1));
+ spin_unlock_irqrestore(&dom->pgtlock, flags);
+
+ return pa;
+}
+
+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;
+
+ 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);
+
+ 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));
+ platform_device_put(pdev);
+ }
+
+ 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 of_device_id sprd_iommu_of_match[] = {
+ { .compatible = "sprd,iommu-v1" },
+ { },
+};
+MODULE_DEVICE_TABLE(of, sprd_iommu_of_match);
+
+/*
+ * Clock is not required, access to some of IOMMUs is controlled by gate
+ * clk, enabled clocks for that kind of IOMMUs before accessing.
+ * Return 0 for success or no clocks found.
+ */
+static int sprd_iommu_clk_enable(struct sprd_iommu_device *sdev)
+{
+ struct clk *eb;
+
+ eb = clk_get_optional(sdev->dev, 0);
+ if (!eb)
+ return 0;
+
+ if (IS_ERR(eb))
+ return PTR_ERR(eb);
+
+ sdev->eb = eb;
+ return clk_prepare_enable(eb);
+}
+
+static void sprd_iommu_clk_disable(struct sprd_iommu_device *sdev)
+{
+ if (sdev->eb)
+ clk_disable_unprepare(sdev->eb);
+}
+
+static struct regmap_config reg_config = {
+ .reg_bits = 32,
+ .val_bits = 32,
+ .reg_stride = 4,
+};
+
+static int sprd_iommu_probe(struct platform_device *pdev)
+{
+ struct sprd_iommu_device *sdev;
+ struct device *dev = &pdev->dev;
+ void __iomem *base;
+ int ret;
+
+ sdev = devm_kzalloc(dev, sizeof(*sdev), GFP_KERNEL);
+ if (!sdev)
+ return -ENOMEM;
+
+ sdev->base = syscon_regmap_lookup_by_phandle_args(dev->of_node,
+ "sprd,iommu-regs", 1, &sdev->reg_offset);
+ if (IS_ERR(sdev->base)) {
+ sdev->reg_offset = 0;
+ base = devm_platform_ioremap_resource(pdev, 0);
+ if (IS_ERR(base)) {
+ dev_err(dev, "Failed to get ioremap resource.\n");
+ return PTR_ERR(base);
+ }
+
+ sdev->base = regmap_init_mmio(NULL, base, &reg_config);
+ if (IS_ERR(sdev->base)) {
+ dev_err(dev, "Failed to init regmap.\n");
+ return PTR_ERR(sdev->base);
+ }
+ }
+
+ 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;
+
+ 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);
+
+ ret = sprd_iommu_clk_enable(sdev);
+ if (ret)
+ goto unregister_iommu;
+
+ ret = sprd_iommu_get_version(sdev);
+ if (ret < 0) {
+ dev_err(dev, "iommu version(%d) is invalid.\n", ret);
+ goto disable_clk;
+ }
+ sdev->ver = ret;
+
+ return 0;
+
+disable_clk:
+ sprd_iommu_clk_disable(sdev);
+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)
+{
+ 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);
+
+ 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;
+}
+
+static struct platform_driver sprd_iommu_driver = {
+ .driver = {
+ .name = "sprd-iommu",
+ .of_match_table = sprd_iommu_of_match,
+ .suppress_bind_attrs = true,
+ },
+ .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");
--
2.25.1

2021-02-03 13:06:08

by kernel test robot

[permalink] [raw]

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

Hi Chunyan,

I love your patch! Perhaps something to improve:

[auto build test WARNING on iommu/next]
[also build test WARNING on robh/for-next v5.11-rc6 next-20210125]
[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/20210203-171459
base: https://git.kernel.org/pub/scm/linux/kernel/git/joro/iommu.git next
config: nios2-allyesconfig (attached as .config)
compiler: nios2-linux-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/02726f17be90f0d6226117f44cef3497250e378f
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Chunyan-Zhang/Add-Unisoc-iommu-basic-driver/20210203-171459
git checkout 02726f17be90f0d6226117f44cef3497250e378f
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=nios2

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <[email protected]>

All warnings (new ones prefixed by >>):

In file included from ./arch/nios2/include/generated/asm/bug.h:1,
from include/linux/bug.h:5,
from include/linux/thread_info.h:12,
from include/asm-generic/current.h:5,
from ./arch/nios2/include/generated/asm/current.h:1,
from include/linux/mutex.h:14,
from include/linux/notifier.h:14,
from include/linux/clk.h:14,
from drivers/iommu/sprd-iommu.c:9:
drivers/iommu/sprd-iommu.c: In function 'sprd_iommu_iova_to_phys':
>> drivers/iommu/sprd-iommu.c:375:4: warning: format '%llx' expects argument of type 'long long unsigned int', but argument 5 has type 'dma_addr_t' {aka 'unsigned int'} [-Wformat=]
375 | "iova (0x%llx) exceeds the vpn range[0x%lx-0x%lx]\n",
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
376 | iova, start, end))
| ~~~~
| |
| dma_addr_t {aka unsigned int}
include/asm-generic/bug.h:89:48: note: in definition of macro '__WARN_printf'
89 | warn_slowpath_fmt(__FILE__, __LINE__, taint, arg); \
| ^~~
drivers/iommu/sprd-iommu.c:374:6: note: in expansion of macro 'WARN'
374 | if (WARN(iova < start || iova > end,
| ^~~~
drivers/iommu/sprd-iommu.c:375:16: note: format string is defined here
375 | "iova (0x%llx) exceeds the vpn range[0x%lx-0x%lx]\n",
| ~~~^
| |
| long long unsigned int
| %x
drivers/iommu/sprd-iommu.c: At top level:
drivers/iommu/sprd-iommu.c:438:20: error: initialization of 'void (*)(struct iommu_domain *, long unsigned int, size_t)' {aka 'void (*)(struct iommu_domain *, long unsigned int, unsigned int)'} from incompatible pointer type 'void (*)(struct iommu_domain *)' [-Werror=incompatible-pointer-types]
438 | .iotlb_sync_map = sprd_iommu_sync_map,
| ^~~~~~~~~~~~~~~~~~~
drivers/iommu/sprd-iommu.c:438:20: note: (near initialization for 'sprd_iommu_ops.iotlb_sync_map')
cc1: some warnings being treated as errors

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

364
365 static phys_addr_t sprd_iommu_iova_to_phys(struct iommu_domain *domain,
366 dma_addr_t iova)
367 {
368 struct sprd_iommu_domain *dom = to_sprd_domain(domain);
369 unsigned long flags;
370 phys_addr_t pa;
371 unsigned long start = domain->geometry.aperture_start;
372 unsigned long end = domain->geometry.aperture_end;
373
374 if (WARN(iova < start || iova > end,
> 375 "iova (0x%llx) exceeds the vpn range[0x%lx-0x%lx]\n",
376 iova, start, end))
377 return 0;
378
379 spin_lock_irqsave(&dom->pgtlock, flags);
380 pa = *(dom->pgt_va + ((iova - start) >> SPRD_IOMMU_PAGE_SHIFT));
381 pa = (pa << SPRD_IOMMU_PAGE_SHIFT) + ((iova - start) & (SPRD_IOMMU_PAGE_SIZE - 1));
382 spin_unlock_irqrestore(&dom->pgtlock, flags);
383
384 return pa;
385 }
386

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/[email protected]

Attachments:

(No filename) (4.64 kB)
.config.gz (57.42 kB)
Download all attachments

2021-02-03 17:47:21

by Randy Dunlap

[permalink] [raw]

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

On 2/3/21 1:07 AM, Chunyan Zhang wrote:
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 192ef8f61310..99e7712f3903 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -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, this iommu can be used by

s/iommu/IOMMU/ please

> + Unisoc's multimedia devices, such as display, Image codec(jpeg)
> + and a few signal processors, including VSP(video), GSP(graphic),
> + ISP(image), and CPP(camera pixel processor), etc.
> +
> + Say Y here if you want to use the multimedia devices listed above.

--
~Randy

2021-02-04 03:21:48

by Chunyan Zhang

[permalink] [raw]

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

On Thu, 4 Feb 2021 at 01:44, Randy Dunlap <[email protected]> wrote:
>
> On 2/3/21 1:07 AM, Chunyan Zhang wrote:
> > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > index 192ef8f61310..99e7712f3903 100644
> > --- a/drivers/iommu/Kconfig
> > +++ b/drivers/iommu/Kconfig
> > @@ -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, this iommu can be used by
>
> s/iommu/IOMMU/ please

Sure, thanks.

Chunyan

>
> > + Unisoc's multimedia devices, such as display, Image codec(jpeg)
> > + and a few signal processors, including VSP(video), GSP(graphic),
> > + ISP(image), and CPP(camera pixel processor), etc.
> > +
> > + Say Y here if you want to use the multimedia devices listed above.
>
>
> --
> ~Randy

2021-02-05 01:51:53

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] dt-bindings: iommu: add bindings for sprd iommu

On Wed, Feb 03, 2021 at 05:07:26PM +0800, Chunyan Zhang wrote:
> From: Chunyan Zhang <[email protected]>
>
> This iommu module can be used by Unisoc's multimedia devices, such as
> display, Image codec(jpeg) and a few signal processors, including
> VSP(video), GSP(graphic), ISP(image), and CPP(camera pixel processor), etc.
>
> Signed-off-by: Chunyan Zhang <[email protected]>
> ---
> .../devicetree/bindings/iommu/sprd,iommu.yaml | 72 +++++++++++++++++++
> 1 file changed, 72 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
>
> diff --git a/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
> new file mode 100644
> index 000000000000..4fc99e81fa66
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
> @@ -0,0 +1,72 @@
> +# 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
> +
> + "#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
> +
> + reg:
> + maxItems: 1
> + description:
> + Not required if 'sprd,iommu-regs' is defined.
> +
> + clocks:
> + description:
> + Reference to a gate clock phandle, since access to some of IOMMUs are
> + controlled by gate clock, but this is not required.
> +
> + sprd,iommu-regs:
> + $ref: /schemas/types.yaml#/definitions/phandle-array
> + description:
> + Reference to a syscon phandle plus 1 cell, the syscon defines the
> + register range used by the iommu and the media device, the cell
> + defines the offset for iommu registers. Since iommu module shares
> + the same register range with the media device which uses it.
> +
> +required:
> + - compatible
> + - "#iommu-cells"
> +
> +oneOf:
> + - required:
> + - reg
> + - required:
> + - sprd,iommu-regs
> +
> +additionalProperties: false
> +
> +examples:
> + - |
> + iommu_disp: iommu-disp {
> + compatible = "sprd,iommu-v1";
> + sprd,iommu-regs = <&dpu_regs 0x800>;

If the IOMMU is contained within another device, then it should just be
a child node of that device. Or just make 'dpu_regs' an IOMMU provider
(i.e. just add #iommu-cells to it).

> + #iommu-cells = <0>;
> + };
> +
> + - |
> + iommu_jpg: iommu-jpg {
> + compatible = "sprd,iommu-v1";
> + sprd,iommu-regs = <&jpg_regs 0x300>;
> + #iommu-cells = <0>;
> + clocks = <&mm_gate 1>;
> + };
> +
> +...
> --
> 2.25.1
>

2021-02-05 07:25:25

by Chunyan Zhang

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] dt-bindings: iommu: add bindings for sprd iommu

Hi Rob,

On Fri, 5 Feb 2021 at 07:25, Rob Herring <[email protected]> wrote:
>
> On Wed, Feb 03, 2021 at 05:07:26PM +0800, Chunyan Zhang wrote:
> > From: Chunyan Zhang <[email protected]>
> >
> > This iommu module can be used by Unisoc's multimedia devices, such as
> > display, Image codec(jpeg) and a few signal processors, including
> > VSP(video), GSP(graphic), ISP(image), and CPP(camera pixel processor), etc.
> >
> > Signed-off-by: Chunyan Zhang <[email protected]>
> > ---
> > .../devicetree/bindings/iommu/sprd,iommu.yaml | 72 +++++++++++++++++++
> > 1 file changed, 72 insertions(+)
> > create mode 100644 Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
> > new file mode 100644
> > index 000000000000..4fc99e81fa66
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
> > @@ -0,0 +1,72 @@
> > +# 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
> > +
> > + "#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
> > +
> > + reg:
> > + maxItems: 1
> > + description:
> > + Not required if 'sprd,iommu-regs' is defined.
> > +
> > + clocks:
> > + description:
> > + Reference to a gate clock phandle, since access to some of IOMMUs are
> > + controlled by gate clock, but this is not required.
> > +
> > + sprd,iommu-regs:
> > + $ref: /schemas/types.yaml#/definitions/phandle-array
> > + description:
> > + Reference to a syscon phandle plus 1 cell, the syscon defines the
> > + register range used by the iommu and the media device, the cell
> > + defines the offset for iommu registers. Since iommu module shares
> > + the same register range with the media device which uses it.
> > +
> > +required:
> > + - compatible
> > + - "#iommu-cells"
> > +
> > +oneOf:
> > + - required:
> > + - reg
> > + - required:
> > + - sprd,iommu-regs
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > + - |
> > + iommu_disp: iommu-disp {
> > + compatible = "sprd,iommu-v1";
> > + sprd,iommu-regs = <&dpu_regs 0x800>;
>
> If the IOMMU is contained within another device, then it should just be
> a child node of that device.

Yes, actually IOMMU can be seen as a child of multimedia devices, I
considered moving IOMMU under into multimedia device node, but
multimedia devices need IOMMU when probe[1], so I dropped that idea.

And they share the same register base, e.g.

+ mm {
+ compatible = "simple-bus";
+ #address-cells = <2>;
+ #size-cells = <2>;
+ ranges;
+
+ dpu_regs: syscon@63000000 {
+ compatible = "sprd,sc9863a-dpuregs", "syscon";
+ reg = <0 0x63000000 0 0x1000>;
+ };
+
+ dpu: dpu@63000000 {
+ compatible = "sprd,sharkl3-dpu";
+ sprd,disp-regs = <&dpu_regs>;
+ iommus = <&iommu_dispc>;
+ };
+
+ iommu_dispc: iommu@63000000 {
+ compatible = "sprd,iommu-v1";
+ sprd,iommu-regs = <&dpu_regs 0x800>;
+ #iommu-cells = <0>;
+ };

DPU use the registers from 0, IOMMU from 0x800, the purpose of using
syscon node was to avoid remapping register physical address.

> Or just make 'dpu_regs' an IOMMU provider
> (i.e. just add #iommu-cells to it).

xxx_regs(syscon node) defines the register range for IOMMU and a
multimedia device (such as dpu, image codec, etc.)

Hope I've explained the relationship of xxx_regs, multimedia device,
and iommu clearly :)

Any suggestion for this kind of cases?

Thanks,
Chunyan

[1] https://elixir.bootlin.com/linux/v5.11-rc6/source/drivers/iommu/of_iommu.c#L145
>
> > + #iommu-cells = <0>;
> > + };
> > +
> > + - |
> > + iommu_jpg: iommu-jpg {
> > + compatible = "sprd,iommu-v1";
> > + sprd,iommu-regs = <&jpg_regs 0x300>;
> > + #iommu-cells = <0>;
> > + clocks = <&mm_gate 1>;
> > + };
> > +
> > +...
> > --
> > 2.25.1
> >

2021-02-10 19:24:26

by Rob Herring (Arm)

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] dt-bindings: iommu: add bindings for sprd iommu

On Fri, Feb 5, 2021 at 1:21 AM Chunyan Zhang <[email protected]> wrote:
>
> Hi Rob,
>
> On Fri, 5 Feb 2021 at 07:25, Rob Herring <[email protected]> wrote:
> >
> > On Wed, Feb 03, 2021 at 05:07:26PM +0800, Chunyan Zhang wrote:
> > > From: Chunyan Zhang <[email protected]>
> > >
> > > This iommu module can be used by Unisoc's multimedia devices, such as
> > > display, Image codec(jpeg) and a few signal processors, including
> > > VSP(video), GSP(graphic), ISP(image), and CPP(camera pixel processor), etc.
> > >
> > > Signed-off-by: Chunyan Zhang <[email protected]>
> > > ---
> > > .../devicetree/bindings/iommu/sprd,iommu.yaml | 72 +++++++++++++++++++
> > > 1 file changed, 72 insertions(+)
> > > create mode 100644 Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
> > >
> > > diff --git a/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
> > > new file mode 100644
> > > index 000000000000..4fc99e81fa66
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
> > > @@ -0,0 +1,72 @@
> > > +# 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
> > > +
> > > + "#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
> > > +
> > > + reg:
> > > + maxItems: 1
> > > + description:
> > > + Not required if 'sprd,iommu-regs' is defined.
> > > +
> > > + clocks:
> > > + description:
> > > + Reference to a gate clock phandle, since access to some of IOMMUs are
> > > + controlled by gate clock, but this is not required.
> > > +
> > > + sprd,iommu-regs:
> > > + $ref: /schemas/types.yaml#/definitions/phandle-array
> > > + description:
> > > + Reference to a syscon phandle plus 1 cell, the syscon defines the
> > > + register range used by the iommu and the media device, the cell
> > > + defines the offset for iommu registers. Since iommu module shares
> > > + the same register range with the media device which uses it.
> > > +
> > > +required:
> > > + - compatible
> > > + - "#iommu-cells"
> > > +
> > > +oneOf:
> > > + - required:
> > > + - reg
> > > + - required:
> > > + - sprd,iommu-regs
> > > +
> > > +additionalProperties: false
> > > +
> > > +examples:
> > > + - |
> > > + iommu_disp: iommu-disp {
> > > + compatible = "sprd,iommu-v1";
> > > + sprd,iommu-regs = <&dpu_regs 0x800>;
> >
> > If the IOMMU is contained within another device, then it should just be
> > a child node of that device.
>
> Yes, actually IOMMU can be seen as a child of multimedia devices, I
> considered moving IOMMU under into multimedia device node, but
> multimedia devices need IOMMU when probe[1], so I dropped that idea.

Don't design your binding around working-around linux issues.

> And they share the same register base, e.g.
>
> + mm {
> + compatible = "simple-bus";
> + #address-cells = <2>;
> + #size-cells = <2>;
> + ranges;
> +
> + dpu_regs: syscon@63000000 {

Drop this node.

> + compatible = "sprd,sc9863a-dpuregs", "syscon";
> + reg = <0 0x63000000 0 0x1000>;
> + };
> +
> + dpu: dpu@63000000 {
> + compatible = "sprd,sharkl3-dpu";
> + sprd,disp-regs = <&dpu_regs>;

reg = <0 0x63000000 0 0x800>;

> + iommus = <&iommu_dispc>;
> + };
> +
> + iommu_dispc: iommu@63000000 {
> + compatible = "sprd,iommu-v1";
> + sprd,iommu-regs = <&dpu_regs 0x800>;

reg = <0 0x63000800 0 0x800>;

> + #iommu-cells = <0>;

Though given it seems there is only 1 client and this might really be
just 1 h/w block, you don't really need to use the iommu binding at
all. The DPU should be able to instantiate it's own IOMMU device.
There's other examples of this such as mali GPU though that is all one
driver, but that's a Linux implementation detail.

Rob

2021-02-16 15:13:52

by Robin Murphy

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] dt-bindings: iommu: add bindings for sprd iommu

On 2021-02-10 19:21, Rob Herring wrote:
> On Fri, Feb 5, 2021 at 1:21 AM Chunyan Zhang <[email protected]> wrote:
>>
>> Hi Rob,
>>
>> On Fri, 5 Feb 2021 at 07:25, Rob Herring <[email protected]> wrote:
>>>
>>> On Wed, Feb 03, 2021 at 05:07:26PM +0800, Chunyan Zhang wrote:
>>>> From: Chunyan Zhang <[email protected]>
>>>>
>>>> This iommu module can be used by Unisoc's multimedia devices, such as
>>>> display, Image codec(jpeg) and a few signal processors, including
>>>> VSP(video), GSP(graphic), ISP(image), and CPP(camera pixel processor), etc.
>>>>
>>>> Signed-off-by: Chunyan Zhang <[email protected]>
>>>> ---
>>>> .../devicetree/bindings/iommu/sprd,iommu.yaml | 72 +++++++++++++++++++
>>>> 1 file changed, 72 insertions(+)
>>>> create mode 100644 Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
>>>> new file mode 100644
>>>> index 000000000000..4fc99e81fa66
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
>>>> @@ -0,0 +1,72 @@
>>>> +# 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
>>>> +
>>>> + "#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
>>>> +
>>>> + reg:
>>>> + maxItems: 1
>>>> + description:
>>>> + Not required if 'sprd,iommu-regs' is defined.
>>>> +
>>>> + clocks:
>>>> + description:
>>>> + Reference to a gate clock phandle, since access to some of IOMMUs are
>>>> + controlled by gate clock, but this is not required.
>>>> +
>>>> + sprd,iommu-regs:
>>>> + $ref: /schemas/types.yaml#/definitions/phandle-array
>>>> + description:
>>>> + Reference to a syscon phandle plus 1 cell, the syscon defines the
>>>> + register range used by the iommu and the media device, the cell
>>>> + defines the offset for iommu registers. Since iommu module shares
>>>> + the same register range with the media device which uses it.
>>>> +
>>>> +required:
>>>> + - compatible
>>>> + - "#iommu-cells"

OK, so apparently the hardware is not quite as trivial as my initial
impression, and you should have interrupts as well.

>>>> +
>>>> +oneOf:
>>>> + - required:
>>>> + - reg
>>>> + - required:
>>>> + - sprd,iommu-regs
>>>> +
>>>> +additionalProperties: false
>>>> +
>>>> +examples:
>>>> + - |
>>>> + iommu_disp: iommu-disp {
>>>> + compatible = "sprd,iommu-v1";
>>>> + sprd,iommu-regs = <&dpu_regs 0x800>;
>>>
>>> If the IOMMU is contained within another device, then it should just be
>>> a child node of that device.
>>
>> Yes, actually IOMMU can be seen as a child of multimedia devices, I
>> considered moving IOMMU under into multimedia device node, but
>> multimedia devices need IOMMU when probe[1], so I dropped that idea.
>
> Don't design your binding around working-around linux issues.

Having stumbled across the DRM driver patches the other day, I now see
where this is coming from, and it's even worse than that - this whole
binding seems to be largely working around bad driver design.

>> And they share the same register base, e.g.
>>
>> + mm {
>> + compatible = "simple-bus";
>> + #address-cells = <2>;
>> + #size-cells = <2>;
>> + ranges;
>> +
>> + dpu_regs: syscon@63000000 {
>
> Drop this node.
>
>> + compatible = "sprd,sc9863a-dpuregs", "syscon";
>> + reg = <0 0x63000000 0 0x1000>;
>> + };
>> +
>> + dpu: dpu@63000000 {
>> + compatible = "sprd,sharkl3-dpu";
>> + sprd,disp-regs = <&dpu_regs>;
>
> reg = <0 0x63000000 0 0x800>;

In fact judging by the other driver it looks like the length only needs
to be 0x200 here (but maybe there's more to come in future).

>> + iommus = <&iommu_dispc>;
>> + };
>> +
>> + iommu_dispc: iommu@63000000 {
>> + compatible = "sprd,iommu-v1";
>> + sprd,iommu-regs = <&dpu_regs 0x800>;
>
> reg = <0 0x63000800 0 0x800>;

...and this one looks to need less than 0x80, even :)

>
>> + #iommu-cells = <0>;
>
> Though given it seems there is only 1 client and this might really be
> just 1 h/w block, you don't really need to use the iommu binding at
> all. The DPU should be able to instantiate it's own IOMMU device.
> There's other examples of this such as mali GPU though that is all one
> driver, but that's a Linux implementation detail.

FWIW that's really a very different situation - the MMUs in a Mali GPU
are fundamental parts of its internal pipelines and would never make
sense to handle as separate devices (if it were even feasible to try).
An IOMMU like this one is typically a logically-distinct block stuck to
the external bus interface of any old device, rewriting transactions
that said device has already issued - it's telling that it needs to
allocate the prot_page scratchpad for "faulting" transactions to still
flow somewhere, implying that it's not even involved enough to be able
to terminate them.

As such I think it *does* make complete sense to describe even
"dedicated" IOMMUs like this one, Rockchip, Exynos, etc. in DT.
Otherwise you'd be effectively forcing OSes to turn half their
display/media drivers into mini board files with secret knowledge of
which blocks are integrated with IOMMUs on which SoCs.

Robin.

2021-02-26 06:14:50

by Chunyan Zhang

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] dt-bindings: iommu: add bindings for sprd iommu

On Thu, 11 Feb 2021 at 03:21, Rob Herring <[email protected]> wrote:
>
> On Fri, Feb 5, 2021 at 1:21 AM Chunyan Zhang <[email protected]> wrote:
> >
> > Hi Rob,
> >
> > On Fri, 5 Feb 2021 at 07:25, Rob Herring <[email protected]> wrote:
> > >
> > > On Wed, Feb 03, 2021 at 05:07:26PM +0800, Chunyan Zhang wrote:
> > > > From: Chunyan Zhang <[email protected]>
> > > >
> > > > This iommu module can be used by Unisoc's multimedia devices, such as
> > > > display, Image codec(jpeg) and a few signal processors, including
> > > > VSP(video), GSP(graphic), ISP(image), and CPP(camera pixel processor), etc.
> > > >
> > > > Signed-off-by: Chunyan Zhang <[email protected]>
> > > > ---
> > > > .../devicetree/bindings/iommu/sprd,iommu.yaml | 72 +++++++++++++++++++
> > > > 1 file changed, 72 insertions(+)
> > > > create mode 100644 Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
> > > > new file mode 100644
> > > > index 000000000000..4fc99e81fa66
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
> > > > @@ -0,0 +1,72 @@
> > > > +# 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
> > > > +
> > > > + "#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
> > > > +
> > > > + reg:
> > > > + maxItems: 1
> > > > + description:
> > > > + Not required if 'sprd,iommu-regs' is defined.
> > > > +
> > > > + clocks:
> > > > + description:
> > > > + Reference to a gate clock phandle, since access to some of IOMMUs are
> > > > + controlled by gate clock, but this is not required.
> > > > +
> > > > + sprd,iommu-regs:
> > > > + $ref: /schemas/types.yaml#/definitions/phandle-array
> > > > + description:
> > > > + Reference to a syscon phandle plus 1 cell, the syscon defines the
> > > > + register range used by the iommu and the media device, the cell
> > > > + defines the offset for iommu registers. Since iommu module shares
> > > > + the same register range with the media device which uses it.
> > > > +
> > > > +required:
> > > > + - compatible
> > > > + - "#iommu-cells"
> > > > +
> > > > +oneOf:
> > > > + - required:
> > > > + - reg
> > > > + - required:
> > > > + - sprd,iommu-regs
> > > > +
> > > > +additionalProperties: false
> > > > +
> > > > +examples:
> > > > + - |
> > > > + iommu_disp: iommu-disp {
> > > > + compatible = "sprd,iommu-v1";
> > > > + sprd,iommu-regs = <&dpu_regs 0x800>;
> > >
> > > If the IOMMU is contained within another device, then it should just be
> > > a child node of that device.
> >
> > Yes, actually IOMMU can be seen as a child of multimedia devices, I
> > considered moving IOMMU under into multimedia device node, but
> > multimedia devices need IOMMU when probe[1], so I dropped that idea.
>
> Don't design your binding around working-around linux issues.
>
> > And they share the same register base, e.g.
> >
> > + mm {
> > + compatible = "simple-bus";
> > + #address-cells = <2>;
> > + #size-cells = <2>;
> > + ranges;
> > +
> > + dpu_regs: syscon@63000000 {
>
> Drop this node.
>
> > + compatible = "sprd,sc9863a-dpuregs", "syscon";
> > + reg = <0 0x63000000 0 0x1000>;
> > + };
> > +
> > + dpu: dpu@63000000 {
> > + compatible = "sprd,sharkl3-dpu";
> > + sprd,disp-regs = <&dpu_regs>;
>
> reg = <0 0x63000000 0 0x800>;
>
> > + iommus = <&iommu_dispc>;
> > + };
> > +
> > + iommu_dispc: iommu@63000000 {
> > + compatible = "sprd,iommu-v1";
> > + sprd,iommu-regs = <&dpu_regs 0x800>;
>
> reg = <0 0x63000800 0 0x800>;

Alright, considering you deprecate using syscon to map registers here,
I will drop that.
But that would cause the same physical address to be mapped two times at least.
And this is not a single case, since there are a few media devices and
their IOMMUs which all have this issue.

Thanks,
Chunyan

>
> > + #iommu-cells = <0>;
>
> Though given it seems there is only 1 client and this might really be
> just 1 h/w block, you don't really need to use the iommu binding at
> all. The DPU should be able to instantiate it's own IOMMU device.
> There's other examples of this such as mali GPU though that is all one
> driver, but that's a Linux implementation detail.
>
> Rob

2021-02-26 06:51:49

by Chunyan Zhang

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] dt-bindings: iommu: add bindings for sprd iommu

On Tue, 16 Feb 2021 at 23:10, Robin Murphy <[email protected]> wrote:
>
> On 2021-02-10 19:21, Rob Herring wrote:
> > On Fri, Feb 5, 2021 at 1:21 AM Chunyan Zhang <[email protected]> wrote:
> >>
> >> Hi Rob,
> >>
> >> On Fri, 5 Feb 2021 at 07:25, Rob Herring <[email protected]> wrote:
> >>>
> >>> On Wed, Feb 03, 2021 at 05:07:26PM +0800, Chunyan Zhang wrote:
> >>>> From: Chunyan Zhang <[email protected]>
> >>>>
> >>>> This iommu module can be used by Unisoc's multimedia devices, such as
> >>>> display, Image codec(jpeg) and a few signal processors, including
> >>>> VSP(video), GSP(graphic), ISP(image), and CPP(camera pixel processor), etc.
> >>>>
> >>>> Signed-off-by: Chunyan Zhang <[email protected]>
> >>>> ---
> >>>> .../devicetree/bindings/iommu/sprd,iommu.yaml | 72 +++++++++++++++++++
> >>>> 1 file changed, 72 insertions(+)
> >>>> create mode 100644 Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
> >>>> new file mode 100644
> >>>> index 000000000000..4fc99e81fa66
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
> >>>> @@ -0,0 +1,72 @@
> >>>> +# 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
> >>>> +
> >>>> + "#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
> >>>> +
> >>>> + reg:
> >>>> + maxItems: 1
> >>>> + description:
> >>>> + Not required if 'sprd,iommu-regs' is defined.
> >>>> +
> >>>> + clocks:
> >>>> + description:
> >>>> + Reference to a gate clock phandle, since access to some of IOMMUs are
> >>>> + controlled by gate clock, but this is not required.
> >>>> +
> >>>> + sprd,iommu-regs:
> >>>> + $ref: /schemas/types.yaml#/definitions/phandle-array
> >>>> + description:
> >>>> + Reference to a syscon phandle plus 1 cell, the syscon defines the
> >>>> + register range used by the iommu and the media device, the cell
> >>>> + defines the offset for iommu registers. Since iommu module shares
> >>>> + the same register range with the media device which uses it.
> >>>> +
> >>>> +required:
> >>>> + - compatible
> >>>> + - "#iommu-cells"
>
> OK, so apparently the hardware is not quite as trivial as my initial
> impression, and you should have interrupts as well.

Ok, I will have a look.

>
> >>>> +
> >>>> +oneOf:
> >>>> + - required:
> >>>> + - reg
> >>>> + - required:
> >>>> + - sprd,iommu-regs
> >>>> +
> >>>> +additionalProperties: false
> >>>> +
> >>>> +examples:
> >>>> + - |
> >>>> + iommu_disp: iommu-disp {
> >>>> + compatible = "sprd,iommu-v1";
> >>>> + sprd,iommu-regs = <&dpu_regs 0x800>;
> >>>
> >>> If the IOMMU is contained within another device, then it should just be
> >>> a child node of that device.
> >>
> >> Yes, actually IOMMU can be seen as a child of multimedia devices, I
> >> considered moving IOMMU under into multimedia device node, but
> >> multimedia devices need IOMMU when probe[1], so I dropped that idea.
> >
> > Don't design your binding around working-around linux issues.
>
> Having stumbled across the DRM driver patches the other day, I now see
> where this is coming from, and it's even worse than that - this whole
> binding seems to be largely working around bad driver design.
>

I guess you mean bad h/w design (not bad driver design)?
Having this syscon node just because I don't want a same register
range to be mapped to multiple virtual address ranges, and that's the
case for many media devices and their IOMMUs. If this issue exsists
for one device only, I can even endure, but that's not unfortunately.
But anyway, as you all think this is not a good way, I will change to
use reg property.

> >> And they share the same register base, e.g.
> >>
> >> + mm {
> >> + compatible = "simple-bus";
> >> + #address-cells = <2>;
> >> + #size-cells = <2>;
> >> + ranges;
> >> +
> >> + dpu_regs: syscon@63000000 {
> >
> > Drop this node.
> >
> >> + compatible = "sprd,sc9863a-dpuregs", "syscon";
> >> + reg = <0 0x63000000 0 0x1000>;
> >> + };
> >> +
> >> + dpu: dpu@63000000 {
> >> + compatible = "sprd,sharkl3-dpu";
> >> + sprd,disp-regs = <&dpu_regs>;
> >
> > reg = <0 0x63000000 0 0x800>;
>
> In fact judging by the other driver it looks like the length only needs
> to be 0x200 here (but maybe there's more to come in future).
>
> >> + iommus = <&iommu_dispc>;
> >> + };
> >> +
> >> + iommu_dispc: iommu@63000000 {
> >> + compatible = "sprd,iommu-v1";
> >> + sprd,iommu-regs = <&dpu_regs 0x800>;
> >
> > reg = <0 0x63000800 0 0x800>;
>
> ...and this one looks to need less than 0x80, even :)

There're some registers not be added in the current driver indeed. The
specification defines registers up to 0x7c.

>
> >
> >> + #iommu-cells = <0>;
> >
> > Though given it seems there is only 1 client and this might really be
> > just 1 h/w block, you don't really need to use the iommu binding at
> > all. The DPU should be able to instantiate it's own IOMMU device.
> > There's other examples of this such as mali GPU though that is all one
> > driver, but that's a Linux implementation detail.
>
> FWIW that's really a very different situation - the MMUs in a Mali GPU
> are fundamental parts of its internal pipelines and would never make
> sense to handle as separate devices (if it were even feasible to try).
> An IOMMU like this one is typically a logically-distinct block stuck to
> the external bus interface of any old device, rewriting transactions
> that said device has already issued - it's telling that it needs to
> allocate the prot_page scratchpad for "faulting" transactions to still
> flow somewhere, implying that it's not even involved enough to be able
> to terminate them.
>
> As such I think it *does* make complete sense to describe even
> "dedicated" IOMMUs like this one, Rockchip, Exynos, etc. in DT.
> Otherwise you'd be effectively forcing OSes to turn half their
> display/media drivers into mini board files with secret knowledge of
> which blocks are integrated with IOMMUs on which SoCs.
>

Thanks for helping me explain the situation.

Regards,
Chunyan

> Robin.

2021-03-04 15:20:12

by Chunyan Zhang

[permalink] [raw]

Subject: Re: [PATCH v3 1/2] dt-bindings: iommu: add bindings for sprd iommu

Hi Robin,

On Tue, 16 Feb 2021 at 23:10, Robin Murphy <[email protected]> wrote:
>
> >>>
> >>> On Wed, Feb 03, 2021 at 05:07:26PM +0800, Chunyan Zhang wrote:
> >>>> From: Chunyan Zhang <[email protected]>
> >>>>
> >>>> This iommu module can be used by Unisoc's multimedia devices, such as
> >>>> display, Image codec(jpeg) and a few signal processors, including
> >>>> VSP(video), GSP(graphic), ISP(image), and CPP(camera pixel processor), etc.
> >>>>
> >>>> Signed-off-by: Chunyan Zhang <[email protected]>
> >>>> ---
> >>>> .../devicetree/bindings/iommu/sprd,iommu.yaml | 72 +++++++++++++++++++
> >>>> 1 file changed, 72 insertions(+)
> >>>> create mode 100644 Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
> >>>> new file mode 100644
> >>>> index 000000000000..4fc99e81fa66
> >>>> --- /dev/null
> >>>> +++ b/Documentation/devicetree/bindings/iommu/sprd,iommu.yaml
> >>>> @@ -0,0 +1,72 @@
> >>>> +# 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
> >>>> +
> >>>> + "#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
> >>>> +
> >>>> + reg:
> >>>> + maxItems: 1
> >>>> + description:
> >>>> + Not required if 'sprd,iommu-regs' is defined.
> >>>> +
> >>>> + clocks:
> >>>> + description:
> >>>> + Reference to a gate clock phandle, since access to some of IOMMUs are
> >>>> + controlled by gate clock, but this is not required.
> >>>> +
> >>>> + sprd,iommu-regs:
> >>>> + $ref: /schemas/types.yaml#/definitions/phandle-array
> >>>> + description:
> >>>> + Reference to a syscon phandle plus 1 cell, the syscon defines the
> >>>> + register range used by the iommu and the media device, the cell
> >>>> + defines the offset for iommu registers. Since iommu module shares
> >>>> + the same register range with the media device which uses it.
> >>>> +
> >>>> +required:
> >>>> + - compatible
> >>>> + - "#iommu-cells"
>
> OK, so apparently the hardware is not quite as trivial as my initial
> impression, and you should have interrupts as well.

I've checked with my colleagues for this issue. And like I explained
before, one sprd IOMMU serves one master device only, so interrupts
are handled by master devices rather than IOMMUs.

Chunyan