Skip to contentSkip to conversation

iommu/msm: Add DT adaptation and generic bindings support

PATCH Superseded Sricharan R 3 participants 19 messages Jun 13, 2016 – Aug 12, 2016

Subsystems (4)
lore
Share & tools
lore
#0 lore raw

v6 cover iommu/msm: Add DT adaptation and generic bindings support

The msm_iommu.c driver currently works based on platform data.
A single master device can be connected to more than one iommu and multiple contexts in each of the iommu. This association between master and iommus was represented from platform data using parent/child devices. The master drivers were responsible for attaching all of the iommus/context to a domain. Now the platform data support is removed and DT support is added. The master/iommus are added through generic iommu bindings.

This is essentially rework of the patch posted earlier by Rob Clark <[email protected]>. This series folds the changes in to the existing driver with the addition of generic bindings.

        http://www.spinics.net/lists/linux-arm-msm/msg10077.html

Tested this series on ifc6410 board.

[V6] After some discussions on patch 6 [1] from previous post,
it was concluded that the changes for using relaxed writes in all places should not be a part of this series, so should be moved it. So removed that patch and added Acked/Tested tags.
[1] https://patchwork.kernel.org/patch/9129231/

[V5] Changed the compatible binding name as per comments, added comments
for usage of barriers in patch 6.

[V4] Addressed comments for making the iommu compatible binding more soc
specific and updated the documentation for the iommu clocks.

[V3] Addressed comments to correct the usage
of the #iommu-cells binding, improve the flush_iotlb_range function, added a new patch to use writel_relaxed for register access and split up the documentation patch.

[V2] Adapted the driver to use generic ARMV7S short descriptor pagetable ops
and addressed comments.

[V1]
https://lists.linuxfoundation.org/pipermail/iommu/2015-August/014074.html

Sricharan R (6):
iommu/msm: Add DT adaptation
documentation: iommu: Add bindings for msm,iommu-v0 ip iommu/msm: Move the contents from msm_iommu_dev.c to msm_iommu.c iommu/msm: Add support for generic master bindings iommu/msm: use generic ARMV7S short descriptor pagetable ops iommu/msm: Remove driver BROKEN

 .../devicetree/bindings/iommu/msm,iommu-v0.txt     |  64 ++
 drivers/iommu/Kconfig                              |   2 +-
 drivers/iommu/Makefile                             |   2 +-
 drivers/iommu/msm_iommu.c                          | 870 +++++++++++----------
 drivers/iommu/msm_iommu.h                          |  73 +-
 drivers/iommu/msm_iommu_dev.c                      | 381 ---------
 6 files changed, 564 insertions(+), 828 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iommu/msm,iommu-v0.txt
 delete mode 100644 drivers/iommu/msm_iommu_dev.c
Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
#1 lore raw

PATCH v6 1/6 iommu/msm: Add DT adaptation

The driver currently works based on platform data. Remove this and add support for DT. A single master can have multiple ports connected to more than one iommu.

                      master
                        |
                        |
                        |
           ------------------------
           |                      |
         IOMMU0                 IOMMU1
           |                      |
      ctx0   ctx1            ctx0   ctx1

This association of master and iommus/contexts were previously represented by platform data parent/child device details. The client drivers were responsible for programming all of the iommus/contexts for the device. Now while adapting to generic DT bindings we maintain the list of iommus, contexts that each master domain is connected to and program all of them on attach/detach.

---
 drivers/iommu/msm_iommu.c     | 252 ++++++++++++++++++---------------
 drivers/iommu/msm_iommu.h     |  73 ++++------
 drivers/iommu/msm_iommu_dev.c | 315 ++++++++++--------------------------------
 3 files changed, 237 insertions(+), 403 deletions(-)
drivers/iommu/msm_iommu.c +138 -114
index e321fa5..bc1a4e3 100644
@@ -48,6 +48,7 @@ __asm__ __volatile__ (							\
 static int msm_iommu_tex_class[4];
 
 DEFINE_SPINLOCK(msm_iommu_lock);
+static LIST_HEAD(qcom_iommu_devices);
 
 struct msm_priv {
 	unsigned long *pgtable;
@@ -60,35 +61,37 @@ static struct msm_priv *to_msm_priv(struct iommu_domain *dom)
 	return container_of(dom, struct msm_priv, domain);
 }
 
-static int __enable_clocks(struct msm_iommu_drvdata *drvdata)
+static int __enable_clocks(struct msm_iommu_dev *iommu)
 {
 	int ret;
 
-	ret = clk_enable(drvdata->pclk);
+	ret = clk_enable(iommu->pclk);
 	if (ret)
 		goto fail;
 
-	if (drvdata->clk) {
-		ret = clk_enable(drvdata->clk);
+	if (iommu->clk) {
+		ret = clk_enable(iommu->clk);
 		if (ret)
-			clk_disable(drvdata->pclk);
+			clk_disable(iommu->pclk);
 	}
 fail:
 	return ret;
 }
 
-static void __disable_clocks(struct msm_iommu_drvdata *drvdata)
+static void __disable_clocks(struct msm_iommu_dev *iommu)
 {
-	clk_disable(drvdata->clk);
-	clk_disable(drvdata->pclk);
+	if (iommu->clk)
+		clk_disable(iommu->clk);
+	clk_disable(iommu->pclk);
 }
 
 static int __flush_iotlb(struct iommu_domain *domain)
 {
 	struct msm_priv *priv = to_msm_priv(domain);
-	struct msm_iommu_drvdata *iommu_drvdata;
-	struct msm_iommu_ctx_drvdata *ctx_drvdata;
+	struct msm_iommu_dev *iommu = NULL;
+	struct msm_iommu_ctx_dev *master;
 	int ret = 0;
+
 #ifndef CONFIG_IOMMU_PGTABLES_L2
 	unsigned long *fl_table = priv->pgtable;
 	int i;
@@ -105,24 +108,67 @@ static int __flush_iotlb(struct iommu_domain *domain)
 	}
 #endif
 
-	list_for_each_entry(ctx_drvdata, &priv->list_attached, attached_elm) {
-
-		BUG_ON(!ctx_drvdata->pdev || !ctx_drvdata->pdev->dev.parent);
-
-		iommu_drvdata = dev_get_drvdata(ctx_drvdata->pdev->dev.parent);
-		BUG_ON(!iommu_drvdata);
-
-		ret = __enable_clocks(iommu_drvdata);
+	list_for_each_entry(iommu, &priv->list_attached, dom_node) {
+		ret = __enable_clocks(iommu);
 		if (ret)
 			goto fail;
 
-		SET_CTX_TLBIALL(iommu_drvdata->base, ctx_drvdata->num, 0);
-		__disable_clocks(iommu_drvdata);
+		list_for_each_entry(master, &iommu->ctx_list, list)
+			SET_CTX_TLBIALL(iommu->base, master->num, 0);
+
+		__disable_clocks(iommu);
 	}
 fail:
 	return ret;
 }
 
+static int msm_iommu_alloc_ctx(unsigned long *map, int start, int end)
+{
+	int idx;
+
+	do {
+		idx = find_next_zero_bit(map, end, start);
+		if (idx == end)
+			return -ENOSPC;
+	} while (test_and_set_bit(idx, map));
+
+	return idx;
+}
+
+static void msm_iommu_free_ctx(unsigned long *map, int idx)
+{
+	clear_bit(idx, map);
+}
+
+static void config_mids(struct msm_iommu_dev *iommu,
+			struct msm_iommu_ctx_dev *master)
+{
+	int mid, ctx, i;
+
+	for (i = 0; i < master->num_mids; i++) {
+		mid = master->mids[i];
+		ctx = master->num;
+
+		SET_M2VCBR_N(iommu->base, mid, 0);
+		SET_CBACR_N(iommu->base, ctx, 0);
+
+		/* Set VMID = 0 */
+		SET_VMID(iommu->base, mid, 0);
+
+		/* Set the context number for that MID to this context */
+		SET_CBNDX(iommu->base, mid, ctx);
+
+		/* Set MID associated with this context bank to 0*/
+		SET_CBVMID(iommu->base, ctx, 0);
+
+		/* Set the ASID for TLB tagging for this context */
+		SET_CONTEXTIDR_ASID(iommu->base, ctx, ctx);
+
+		/* Set security bit override to be Non-secure */
+		SET_NSCFG(iommu->base, mid, 3);
+	}
+}
+
 static void __reset_context(void __iomem *base, int ctx)
 {
 	SET_BPRCOSH(base, ctx, 0);
@@ -272,94 +318,76 @@ static void msm_iommu_domain_free(struct iommu_domain *domain)
 
 static int msm_iommu_attach_dev(struct iommu_domain *domain, struct device *dev)
 {
-	struct msm_priv *priv;
-	struct msm_iommu_ctx_dev *ctx_dev;
-	struct msm_iommu_drvdata *iommu_drvdata;
-	struct msm_iommu_ctx_drvdata *ctx_drvdata;
-	struct msm_iommu_ctx_drvdata *tmp_drvdata;
 	int ret = 0;
 	unsigned long flags;
+	struct msm_iommu_dev *iommu;
+	struct msm_priv *priv = to_msm_priv(domain);
+	struct msm_iommu_ctx_dev *master;
 
 	spin_lock_irqsave(&msm_iommu_lock, flags);
-
-	priv = to_msm_priv(domain);
-
-	if (!dev) {
-		ret = -EINVAL;
-		goto fail;
-	}
-
-	iommu_drvdata = dev_get_drvdata(dev->parent);
-	ctx_drvdata = dev_get_drvdata(dev);
-	ctx_dev = dev->platform_data;
-
-	if (!iommu_drvdata || !ctx_drvdata || !ctx_dev) {
-		ret = -EINVAL;
-		goto fail;
-	}
-
-	if (!list_empty(&ctx_drvdata->attached_elm)) {
-		ret = -EBUSY;
-		goto fail;
-	}
-
-	list_for_each_entry(tmp_drvdata, &priv->list_attached, attached_elm)
-		if (tmp_drvdata == ctx_drvdata) {
-			ret = -EBUSY;
-			goto fail;
+	list_for_each_entry(iommu, &qcom_iommu_devices, dev_node) {
+		master = list_first_entry(&iommu->ctx_list,
+					  struct msm_iommu_ctx_dev,
+					  list);
+		if (master->of_node == dev->of_node) {
+			ret = __enable_clocks(iommu);
+			if (ret)
+				goto fail;
+
+			list_for_each_entry(master, &iommu->ctx_list, list) {
+				if (master->num) {
+					dev_err(dev, "domain already attached");
+					ret = -EEXIST;
+					goto fail;
+				}
+				master->num =
+					msm_iommu_alloc_ctx(iommu->context_map,
+							    0, iommu->ncb);
+					if (IS_ERR_VALUE(master->num)) {
+						ret = -ENODEV;
+						goto fail;
+					}
+				config_mids(iommu, master);
+				__program_context(iommu->base, master->num,
+						  __pa(priv->pgtable));
+			}
+			__disable_clocks(iommu);
+			list_add(&iommu->dom_node, &priv->list_attached);
 		}
+	}
 
-	ret = __enable_clocks(iommu_drvdata);
-	if (ret)
-		goto fail;
-
-	__program_context(iommu_drvdata->base, ctx_dev->num,
-			  __pa(priv->pgtable));
-
-	__disable_clocks(iommu_drvdata);
-	list_add(&(ctx_drvdata->attached_elm), &priv->list_attached);
 	ret = __flush_iotlb(domain);
-
 fail:
 	spin_unlock_irqrestore(&msm_iommu_lock, flags);
+
 	return ret;
 }
 
 static void msm_iommu_detach_dev(struct iommu_domain *domain,
 				 struct device *dev)
 {
-	struct msm_priv *priv;
-	struct msm_iommu_ctx_dev *ctx_dev;
-	struct msm_iommu_drvdata *iommu_drvdata;
-	struct msm_iommu_ctx_drvdata *ctx_drvdata;
+	struct msm_priv *priv = to_msm_priv(domain);
 	unsigned long flags;
+	struct msm_iommu_dev *iommu;
+	struct msm_iommu_ctx_dev *master;
 	int ret;
 
 	spin_lock_irqsave(&msm_iommu_lock, flags);
-	priv = to_msm_priv(domain);
-
-	if (!dev)
-		goto fail;
-
-	iommu_drvdata = dev_get_drvdata(dev->parent);
-	ctx_drvdata = dev_get_drvdata(dev);
-	ctx_dev = dev->platform_data;
-
-	if (!iommu_drvdata || !ctx_drvdata || !ctx_dev)
-		goto fail;
-
 	ret = __flush_iotlb(domain);
 	if (ret)
 		goto fail;
 
-	ret = __enable_clocks(iommu_drvdata);
-	if (ret)
-		goto fail;
-
-	__reset_context(iommu_drvdata->base, ctx_dev->num);
-	__disable_clocks(iommu_drvdata);
-	list_del_init(&ctx_drvdata->attached_elm);
+	list_for_each_entry(iommu, &priv->list_attached, dom_node) {
+		ret = __enable_clocks(iommu);
+		if (ret)
+			goto fail;
 
+		list_for_each_entry(master, &iommu->ctx_list, list) {
+			msm_iommu_free_ctx(iommu->context_map, master->num);
+			__reset_context(iommu->base, master->num);
+		}
+		__disable_clocks(iommu);
+	}
 fail:
 	spin_unlock_irqrestore(&msm_iommu_lock, flags);
 }
@@ -555,47 +583,46 @@ static phys_addr_t msm_iommu_iova_to_phys(struct iommu_domain *domain,
 					  dma_addr_t va)
 {
 	struct msm_priv *priv;
-	struct msm_iommu_drvdata *iommu_drvdata;
-	struct msm_iommu_ctx_drvdata *ctx_drvdata;
+	struct msm_iommu_dev *iommu;
+	struct msm_iommu_ctx_dev *master;
 	unsigned int par;
 	unsigned long flags;
-	void __iomem *base;
 	phys_addr_t ret = 0;
-	int ctx;
 
 	spin_lock_irqsave(&msm_iommu_lock, flags);
 
 	priv = to_msm_priv(domain);
-	if (list_empty(&priv->list_attached))
-		goto fail;
+	iommu = list_first_entry(&priv->list_attached,
+				 struct msm_iommu_dev, dom_node);
 
-	ctx_drvdata = list_entry(priv->list_attached.next,
-				 struct msm_iommu_ctx_drvdata, attached_elm);
-	iommu_drvdata = dev_get_drvdata(ctx_drvdata->pdev->dev.parent);
+	if (list_empty(&iommu->ctx_list))
+		goto fail;
 
-	base = iommu_drvdata->base;
-	ctx = ctx_drvdata->num;
+	master = list_first_entry(&iommu->ctx_list,
+				  struct msm_iommu_ctx_dev, list);
+	if (!master)
+		goto fail;
 
-	ret = __enable_clocks(iommu_drvdata);
+	ret = __enable_clocks(iommu);
 	if (ret)
 		goto fail;
 
 	/* Invalidate context TLB */
-	SET_CTX_TLBIALL(base, ctx, 0);
-	SET_V2PPR(base, ctx, va & V2Pxx_VA);
+	SET_CTX_TLBIALL(iommu->base, master->num, 0);
+	SET_V2PPR(iommu->base, master->num, va & V2Pxx_VA);
 
-	par = GET_PAR(base, ctx);
+	par = GET_PAR(iommu->base, master->num);
 
 	/* We are dealing with a supersection */
-	if (GET_NOFAULT_SS(base, ctx))
+	if (GET_NOFAULT_SS(iommu->base, master->num))
 		ret = (par & 0xFF000000) | (va & 0x00FFFFFF);
 	else	/* Upper 20 bits from PAR, lower 12 from VA */
 		ret = (par & 0xFFFFF000) | (va & 0x00000FFF);
 
-	if (GET_FAULT(base, ctx))
+	if (GET_FAULT(iommu->base, master->num))
 		ret = 0;
 
-	__disable_clocks(iommu_drvdata);
+	__disable_clocks(iommu);
 fail:
 	spin_unlock_irqrestore(&msm_iommu_lock, flags);
 	return ret;
@@ -635,37 +662,34 @@ static void print_ctx_regs(void __iomem *base, int ctx)
 
 irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)
 {
-	struct msm_iommu_drvdata *drvdata = dev_id;
-	void __iomem *base;
+	struct msm_iommu_dev *iommu = dev_id;
 	unsigned int fsr;
 	int i, ret;
 
 	spin_lock(&msm_iommu_lock);
 
-	if (!drvdata) {
+	if (!iommu) {
 		pr_err("Invalid device ID in context interrupt handler\n");
 		goto fail;
 	}
 
-	base = drvdata->base;
-
 	pr_err("Unexpected IOMMU page fault!\n");
-	pr_err("base = %08x\n", (unsigned int) base);
+	pr_err("base = %08x\n", (unsigned int)iommu->base);
 
-	ret = __enable_clocks(drvdata);
+	ret = __enable_clocks(iommu);
 	if (ret)
 		goto fail;
 
-	for (i = 0; i < drvdata->ncb; i++) {
-		fsr = GET_FSR(base, i);
+	for (i = 0; i < iommu->ncb; i++) {
+		fsr = GET_FSR(iommu->base, i);
 		if (fsr) {
 			pr_err("Fault occurred in context %d.\n", i);
 			pr_err("Interesting registers:\n");
-			print_ctx_regs(base, i);
-			SET_FSR(base, i, 0x4000000F);
+			print_ctx_regs(iommu->base, i);
+			SET_FSR(iommu->base, i, 0x4000000F);
 		}
 	}
-	__disable_clocks(drvdata);
+	__disable_clocks(iommu);
 fail:
 	spin_unlock(&msm_iommu_lock);
 	return 0;
drivers/iommu/msm_iommu.h +26 -47

100 lines not on this page · raw message

drivers/iommu/msm_iommu_dev.c +73 -242

370 lines not on this page · raw message

Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
#2 lore raw

PATCH v6 2/6 documentation: iommu: Add bindings for msm, iommu-v0 ip

In reply to Sricharan R

The MSM IOMMU is an implementation compatible with the ARM VMSA short descriptor page tables. It provides address translation for bus masters outside of the CPU, each connected to the IOMMU through a port called micro-TLB. Adding the DT bindings for the same.

---
 .../devicetree/bindings/iommu/msm,iommu-v0.txt     | 64 ++++++++++++++++++++++
 1 file changed, 64 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/iommu/msm,iommu-v0.txt
Documentation/devicetree/bindings/iommu/msm,iommu-v0.txt new file +64 -0
new file mode 100644
index 0000000..2023638
@@ -0,0 +1,64 @@
+* QCOM IOMMU
+
+The MSM IOMMU is an implementation compatible with the ARM VMSA short
+descriptor page tables. It provides address translation for bus masters outside
+of the CPU, each connected to the IOMMU through a port called micro-TLB.
+
+Required Properties:
+
+  - compatible: Must contain "qcom,apq8064-iommu".
+  - reg: Base address and size of the IOMMU registers.
+  - interrupts: Specifiers for the MMU fault interrupts. For instances that
+    support secure mode two interrupts must be specified, for non-secure and
+    secure mode, in that order. For instances that don't support secure mode a
+    single interrupt must be specified.
+  - #iommu-cells: The number of cells needed to specify the stream id. This
+		  is always 1.
+  - qcom,ncb:	  The total number of context banks in the IOMMU.
+  - clocks	: List of clocks to be used during SMMU register access. See
+		  Documentation/devicetree/bindings/clock/clock-bindings.txt
+		  for information about the format. For each clock specified
+		  here, there must be a corresponding entry in clock-names
+		  (see below).
+
+  - clock-names	: List of clock names corresponding to the clocks specified in
+		  the "clocks" property (above).
+		  Should be "smmu_pclk" for specifying the interface clock
+		  required for iommu's register accesses.
+		  Should be "smmu_clk" for specifying the functional clock
+		  required by iommu for bus accesses.
+
+Each bus master connected to an IOMMU must reference the IOMMU in its device
+node with the following property:
+
+  - iommus: A reference to the IOMMU in multiple cells. The first cell is a
+	    phandle to the IOMMU and the second cell is the stream id.
+	    A single master device can be connected to more than one iommu
+	    and multiple contexts in each of the iommu. So multiple entries
+	    are required to list all the iommus and the stream ids that the
+	    master is connected to.
+
+Example: mdp iommu and its bus master
+
+                mdp_port0: iommu at 7500000 {
+			compatible = "qcom,apq8064-iommu";
+			#iommu-cells = <1>;
+			clock-names =
+			    "smmu_pclk",
+			    "smmu_clk";
+			clocks =
+			    <&mmcc SMMU_AHB_CLK>,
+			    <&mmcc MDP_AXI_CLK>;
+			reg = <0x07500000 0x100000>;
+			interrupts =
+			    <GIC_SPI 63 0>,
+			    <GIC_SPI 64 0>;
+			qcom,ncb = <2>;
+		};
+
+		mdp: qcom,mdp at 5100000 {
+			compatible = "qcom,mdp";
+			...
+			iommus = <&mdp_port0 0
+				  &mdp_port0 2>;
+		};
Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
#3 lore raw

PATCH v6 3/6 iommu/msm: Move the contents from msm_iommu_dev.c to msm_iommu.c

In reply to Sricharan R

There are only two functions left in msm_iommu_dev.c. Move it to msm_iommu.c and delete the file.

---
 drivers/iommu/Makefile        |   2 +-
 drivers/iommu/msm_iommu.c     | 182 ++++++++++++++++++++++++++++++++++++
 drivers/iommu/msm_iommu_dev.c | 212 ------------------------------------------
 3 files changed, 183 insertions(+), 213 deletions(-)
 delete mode 100644 drivers/iommu/msm_iommu_dev.c
drivers/iommu/Makefile +1 -1
index e7b64d1..6de2fb1 100644
@@ -7,7 +7,7 @@ obj-$(CONFIG_IOMMU_IO_PGTABLE_ARMV7S) += io-pgtable-arm-v7s.o
 obj-$(CONFIG_IOMMU_IO_PGTABLE_LPAE) += io-pgtable-arm.o
 obj-$(CONFIG_IOMMU_IOVA) += iova.o
 obj-$(CONFIG_OF_IOMMU)	+= of_iommu.o
-obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o msm_iommu_dev.o
+obj-$(CONFIG_MSM_IOMMU) += msm_iommu.o
 obj-$(CONFIG_QCOM_IOMMU_V1) += qcom/
 obj-$(CONFIG_AMD_IOMMU) += amd_iommu.o amd_iommu_init.o
 obj-$(CONFIG_AMD_IOMMU_V2) += amd_iommu_v2.o
drivers/iommu/msm_iommu.c +182 -0

200 lines not on this page · raw message

drivers/iommu/msm_iommu_dev.c deleted +0 -212

212 lines not on this page · raw message

Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
#4 lore raw

PATCH v6 4/6 iommu/msm: Add support for generic master bindings

In reply to Sricharan R

This adds the xlate callback which gets invoked during device registration from DT. The master devices gets added through this.

---
 drivers/iommu/msm_iommu.c | 61 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 59 insertions(+), 2 deletions(-)
drivers/iommu/msm_iommu.c +59 -2
index 792b352..8ab0643 100644
@@ -28,6 +28,7 @@
 #include <linux/iommu.h>
 #include <linux/clk.h>
 #include <linux/err.h>
+#include <linux/of_iommu.h>
 
 #include <asm/cacheflush.h>
 #include <asm/sizes.h>
@@ -702,6 +703,54 @@ static void print_ctx_regs(void __iomem *base, int ctx)
 	       GET_PRRR(base, ctx), GET_NMRR(base, ctx));
 }
 
+static void insert_iommu_master(struct device *dev,
+				struct msm_iommu_dev **iommu,
+				struct of_phandle_args *spec)
+{
+	struct msm_iommu_ctx_dev *master = dev->archdata.iommu;
+	int sid;
+
+	if (list_empty(&(*iommu)->ctx_list)) {
+		master = kzalloc(sizeof(*master), GFP_ATOMIC);
+		master->of_node = dev->of_node;
+		list_add(&master->list, &(*iommu)->ctx_list);
+		dev->archdata.iommu = master;
+	}
+
+	for (sid = 0; sid < master->num_mids; sid++)
+		if (master->mids[sid] == spec->args[0]) {
+			dev_warn(dev, "Stream ID 0x%hx repeated; ignoring\n",
+				 sid);
+			return;
+		}
+
+	master->mids[master->num_mids++] = spec->args[0];
+}
+
+static int qcom_iommu_of_xlate(struct device *dev,
+			       struct of_phandle_args *spec)
+{
+	struct msm_iommu_dev *iommu;
+	unsigned long flags;
+	int ret = 0;
+
+	spin_lock_irqsave(&msm_iommu_lock, flags);
+	list_for_each_entry(iommu, &qcom_iommu_devices, dev_node)
+		if (iommu->dev->of_node == spec->np)
+			break;
+
+	if (!iommu || iommu->dev->of_node != spec->np) {
+		ret = -ENODEV;
+		goto fail;
+	}
+
+	insert_iommu_master(dev, &iommu, spec);
+fail:
+	spin_unlock_irqrestore(&msm_iommu_lock, flags);
+
+	return ret;
+}
+
 irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)
 {
 	struct msm_iommu_dev *iommu = dev_id;
@@ -737,7 +786,7 @@ fail:
 	return 0;
 }
 
-static const struct iommu_ops msm_iommu_ops = {
+static struct iommu_ops msm_iommu_ops = {
 	.capable = msm_iommu_capable,
 	.domain_alloc = msm_iommu_domain_alloc,
 	.domain_free = msm_iommu_domain_free,
@@ -748,6 +797,7 @@ static const struct iommu_ops msm_iommu_ops = {
 	.map_sg = default_iommu_map_sg,
 	.iova_to_phys = msm_iommu_iova_to_phys,
 	.pgsize_bitmap = MSM_IOMMU_PGSIZES,
+	.of_xlate = qcom_iommu_of_xlate,
 };
 
 static int msm_iommu_probe(struct platform_device *pdev)
@@ -837,6 +887,7 @@ static int msm_iommu_probe(struct platform_device *pdev)
 	}
 
 	list_add(&iommu->dev_node, &qcom_iommu_devices);
+	of_iommu_set_ops(pdev->dev.of_node, &msm_iommu_ops);
 
 	pr_info("device mapped at %p, irq %d with %d ctx banks\n",
 		iommu->base, iommu->irq, iommu->ncb);
@@ -935,7 +986,13 @@ static int __init msm_iommu_init(void)
 	return 0;
 }
 
-subsys_initcall(msm_iommu_init);
+static int __init msm_iommu_of_setup(struct device_node *np)
+{
+	msm_iommu_init();
+	return 0;
+}
+
+IOMMU_OF_DECLARE(msm_iommu_of, "qcom,apq8064-iommu", msm_iommu_of_setup);
 
 MODULE_LICENSE("GPL v2");
 MODULE_AUTHOR("Stepan Moskovchenko <[email protected]>");
Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
#5 lore raw

PATCH v6 5/6 iommu/msm: use generic ARMV7S short descriptor pagetable ops

In reply to Sricharan R

This iommu uses the armv7 short descriptor format. So use the generic ARMV7S pagetable ops instead of rewriting the same stuff in the driver.

---
 drivers/iommu/Kconfig     |   1 +
 drivers/iommu/msm_iommu.c | 405 +++++++++++++---------------------------------
 2 files changed, 109 insertions(+), 297 deletions(-)
drivers/iommu/Kconfig +1 -0
index 55b2b3a..1f5d496 100644
@@ -92,6 +92,7 @@ config MSM_IOMMU
 	depends on ARCH_MSM8X60 || ARCH_MSM8960 || COMPILE_TEST
 	depends on BROKEN
 	select IOMMU_API
+	select IOMMU_IO_PGTABLE_ARMV7S
 	help
 	  Support for the IOMMUs found on certain Qualcomm SOCs.
 	  These IOMMUs allow virtualization of the address space used by most
drivers/iommu/msm_iommu.c +108 -297

558 lines not on this page · raw message

Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
#6 lore raw

PATCH v6 6/6 iommu/msm: Remove driver BROKEN

In reply to Sricharan R

Now that the driver is DT adapted, bus_set_iommu gets called only when on compatible matching. So the driver should not break multiplatform builds now. So remove the BROKEN config.

---
 drivers/iommu/Kconfig | 1 -
 1 file changed, 1 deletion(-)
drivers/iommu/Kconfig +0 -1
index 1f5d496..b81a0bb 100644
@@ -90,7 +90,6 @@ config MSM_IOMMU
 	bool "MSM IOMMU Support"
 	depends on ARM
 	depends on ARCH_MSM8X60 || ARCH_MSM8960 || COMPILE_TEST
-	depends on BROKEN
 	select IOMMU_API
 	select IOMMU_IO_PGTABLE_ARMV7S
 	help
Signature · 1 line
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
maintainer Applied
#7 lore raw

v6 cover iommu/msm: Add DT adaptation and generic bindings support

In reply to Sricharan R
Quoted 8 lines · Sricharan R

Sricharan R (6):
iommu/msm: Add DT adaptation
documentation: iommu: Add bindings for msm,iommu-v0 ip iommu/msm: Move the contents from msm_iommu_dev.c to msm_iommu.c iommu/msm: Add support for generic master bindings iommu/msm: use generic ARMV7S short descriptor pagetable ops iommu/msm: Remove driver BROKEN

Quoted 9 lines
 .../devicetree/bindings/iommu/msm,iommu-v0.txt     |  64 ++
 drivers/iommu/Kconfig                              |   2 +-
 drivers/iommu/Makefile                             |   2 +-
 drivers/iommu/msm_iommu.c                          | 870 +++++++++++----------
 drivers/iommu/msm_iommu.h                          |  73 +-
 drivers/iommu/msm_iommu_dev.c                      | 381 ---------
 6 files changed, 564 insertions(+), 828 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iommu/msm,iommu-v0.txt
 delete mode 100644 drivers/iommu/msm_iommu_dev.c

Applied this series, thanks.

#8 lore raw

v6 cover iommu/msm: Add DT adaptation and generic bindings support

In reply to Sricharan R
Quoted 8 lines · Sricharan R

The msm_iommu.c driver currently works based on platform data.
A single master device can be connected to more than one iommu and multiple contexts in each of the iommu. This association between master and iommus was represented from platform data using parent/child devices. The master drivers were responsible for attaching all of the iommus/context to a domain. Now the platform data support is removed and DT support is added. The master/iommus are added through generic iommu bindings.

Quoted 7 lines

This is essentially rework of the patch posted earlier by Rob Clark <[email protected]>. This series folds the changes in to the existing driver with the addition of generic bindings.

        http://www.spinics.net/lists/linux-arm-msm/msg10077.html

Tested this series on ifc6410 board.

btw, the current state, at least on linaro integration branch, fault handling doesn't work so well (ie. device never gets resumed).. which is a bit unfortunate for a gpu (and results in a *lot* of rebooting on my part when debugging userspace). I haven't had time yet to compare to the ancient downstream driver, but not sure if you have any ideas?

I guess probably disabling stall on fault would help. But I'm not even getting the "Fault occurred in context.." prints. Seeing the fault iova is pretty useful since that plus gpu cmdstream trace helps me figure out which texture/etc is being accessed out of bounds.

BR,
-R

Quoted 44 lines

[V6] After some discussions on patch 6 [1] from previous post,
it was concluded that the changes for using relaxed writes in all places should not be a part of this series, so should be moved it. So removed that patch and added Acked/Tested tags.
[1] https://patchwork.kernel.org/patch/9129231/

[V5] Changed the compatible binding name as per comments, added comments
for usage of barriers in patch 6.

[V4] Addressed comments for making the iommu compatible binding more soc
specific and updated the documentation for the iommu clocks.

[V3] Addressed comments to correct the usage
of the #iommu-cells binding, improve the flush_iotlb_range function, added a new patch to use writel_relaxed for register access and split up the documentation patch.

[V2] Adapted the driver to use generic ARMV7S short descriptor pagetable ops
and addressed comments.

[V1]
https://lists.linuxfoundation.org/pipermail/iommu/2015-August/014074.html

Sricharan R (6):
iommu/msm: Add DT adaptation
documentation: iommu: Add bindings for msm,iommu-v0 ip iommu/msm: Move the contents from msm_iommu_dev.c to msm_iommu.c iommu/msm: Add support for generic master bindings iommu/msm: use generic ARMV7S short descriptor pagetable ops iommu/msm: Remove driver BROKEN

 .../devicetree/bindings/iommu/msm,iommu-v0.txt     |  64 ++
 drivers/iommu/Kconfig                              |   2 +-
 drivers/iommu/Makefile                             |   2 +-
 drivers/iommu/msm_iommu.c                          | 870 +++++++++++----------
 drivers/iommu/msm_iommu.h                          |  73 +-
 drivers/iommu/msm_iommu_dev.c                      | 381 ---------
 6 files changed, 564 insertions(+), 828 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iommu/msm,iommu-v0.txt
 delete mode 100644 drivers/iommu/msm_iommu_dev.c

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

#9 lore raw

v6 cover iommu/msm: Add DT adaptation and generic bindings support

Quoted 23 lines · Rob Clark
Quoted 8 lines · Sricharan R

The msm_iommu.c driver currently works based on platform data.
A single master device can be connected to more than one iommu and multiple contexts in each of the iommu. This association between master and iommus was represented from platform data using parent/child devices. The master drivers were responsible for attaching all of the iommus/context to a domain. Now the platform data support is removed and DT support is added. The master/iommus are added through generic iommu bindings.

Quoted 7 lines

This is essentially rework of the patch posted earlier by Rob Clark <[email protected]>. This series folds the changes in to the existing driver with the addition of generic bindings.

        http://www.spinics.net/lists/linux-arm-msm/msg10077.html

Tested this series on ifc6410 board.

btw, the current state, at least on linaro integration branch, fault handling doesn't work so well (ie. device never gets resumed).. which is a bit unfortunate for a gpu (and results in a *lot* of rebooting on my part when debugging userspace). I haven't had time yet to compare to the ancient downstream driver, but not sure if you have any ideas?

Quoted 4 lines

I guess probably disabling stall on fault would help. But I'm not even getting the "Fault occurred in context.." prints. Seeing the fault iova is pretty useful since that plus gpu cmdstream trace helps me figure out which texture/etc is being accessed out of bounds.

fyi, it looks like it is not getting any fault irq.. it's *possible* that I screwed up the irq #'s when translating from downstream, so you might want to double check that. I thought I had it right, I assume I would have noticed during piglit runs if fault recovery wasn't working (since the result is that *everything* after the faulting test would have failed since gpu is wedged with no access to memory), but it was long enough ago that I can't claim that definitively.

If you need an easy way to trigger a gpu fault, msmtest is a good way, change this line:

https://github.com/freedreno/msmtest/blob/master/msmtest.c#L247

from OUT_RELOC() to OUT_RING(ring, 0x00000000) will trigger a fault.

BR,
-R

Quoted 45 lines
Quoted 45 lines

[V6] After some discussions on patch 6 [1] from previous post,
it was concluded that the changes for using relaxed writes in all places should not be a part of this series, so should be moved it. So removed that patch and added Acked/Tested tags.
[1] https://patchwork.kernel.org/patch/9129231/

[V5] Changed the compatible binding name as per comments, added comments
for usage of barriers in patch 6.

[V4] Addressed comments for making the iommu compatible binding more soc
specific and updated the documentation for the iommu clocks.

[V3] Addressed comments to correct the usage
of the #iommu-cells binding, improve the flush_iotlb_range function, added a new patch to use writel_relaxed for register access and split up the documentation patch.

[V2] Adapted the driver to use generic ARMV7S short descriptor pagetable ops
and addressed comments.

[V1]
https://lists.linuxfoundation.org/pipermail/iommu/2015-August/014074.html

Sricharan R (6):
iommu/msm: Add DT adaptation
documentation: iommu: Add bindings for msm,iommu-v0 ip iommu/msm: Move the contents from msm_iommu_dev.c to msm_iommu.c iommu/msm: Add support for generic master bindings iommu/msm: use generic ARMV7S short descriptor pagetable ops iommu/msm: Remove driver BROKEN

 .../devicetree/bindings/iommu/msm,iommu-v0.txt     |  64 ++
 drivers/iommu/Kconfig                              |   2 +-
 drivers/iommu/Makefile                             |   2 +-
 drivers/iommu/msm_iommu.c                          | 870 +++++++++++----------
 drivers/iommu/msm_iommu.h                          |  73 +-
 drivers/iommu/msm_iommu_dev.c                      | 381 ---------
 6 files changed, 564 insertions(+), 828 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iommu/msm,iommu-v0.txt
 delete mode 100644 drivers/iommu/msm_iommu_dev.c

--
QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation

author
#10 lore raw

v6 cover iommu/msm: Add DT adaptation and generic bindings support

Hi Rob,

Quoted 19 lines
Quoted 10 lines

btw, the current state, at least on linaro integration branch, fault handling doesn't work so well (ie. device never gets resumed).. which is a bit unfortunate for a gpu (and results in a *lot* of rebooting on my part when debugging userspace). I haven't had time yet to compare to the ancient downstream driver, but not sure if you have any ideas?

I guess probably disabling stall on fault would help. But I'm not even getting the "Fault occurred in context.." prints. Seeing the fault iova is pretty useful since that plus gpu cmdstream trace helps me figure out which texture/etc is being accessed out of bounds.

fyi, it looks like it is not getting any fault irq.. it's *possible* that I screwed up the irq #'s when translating from downstream, so you might want to double check that. I thought I had it right, I assume I would have noticed during piglit runs if fault recovery wasn't working (since the result is that *everything* after the faulting test would have failed since gpu is wedged with no access to memory), but it was long enough ago that I can't claim that definitively.

Quoted 7 lines

If you need an easy way to trigger a gpu fault, msmtest is a good way, change this line:

https://github.com/freedreno/msmtest/blob/master/msmtest.c#L247

from OUT_RELOC() to OUT_RING(ring, 0x00000000) will trigger a fault.

So for the irq to be triggered, 'non-secure' irq line has to be
populated in DT. There is a 'secure'and 'non-secure' irq lines for these iommus and non-secure irq number is secure + 1. I tested this by having a 'return 0'
from the msm_iommu_map (no mapping), and the faults were getting triggered.

Can you share me your dts data ?

Regards,
Sricharan

#11 lore raw

v6 cover iommu/msm: Add DT adaptation and generic bindings support

Quoted 26 lines · Sricharan

Hi Rob,

Quoted 24 lines
Quoted 10 lines

btw, the current state, at least on linaro integration branch, fault handling doesn't work so well (ie. device never gets resumed).. which is a bit unfortunate for a gpu (and results in a *lot* of rebooting on my part when debugging userspace). I haven't had time yet to compare to the ancient downstream driver, but not sure if you have any ideas?

I guess probably disabling stall on fault would help. But I'm not even getting the "Fault occurred in context.." prints. Seeing the fault iova is pretty useful since that plus gpu cmdstream trace helps me figure out which texture/etc is being accessed out of bounds.

fyi, it looks like it is not getting any fault irq.. it's *possible* that I screwed up the irq #'s when translating from downstream, so you might want to double check that. I thought I had it right, I assume I would have noticed during piglit runs if fault recovery wasn't working (since the result is that *everything* after the faulting test would have failed since gpu is wedged with no access to memory), but it was long enough ago that I can't claim that definitively.

If you need an easy way to trigger a gpu fault, msmtest is a good way, change this line:

https://github.com/freedreno/msmtest/blob/master/msmtest.c#L247

Quoted 9 lines
Quoted 2 lines

from OUT_RELOC() to OUT_RING(ring, 0x00000000) will trigger a fault.

So for the irq to be triggered, 'non-secure' irq line has to be
populated in DT. There is a 'secure'and 'non-secure' irq lines for these iommus and non-secure irq number is secure + 1. I tested this by having a 'return 0'
from the msm_iommu_map (no mapping), and the faults were getting triggered.

Can you share me your dts data ?

I think this is what you want:

https://github.com/freedreno/kernel-msm/blob/integration-linux-qcomlt/arch/arm/boot/dts/qcom-apq8064.dtsi#L2008

I haven't tested a display fault, so I suppose it is possible that irq's are working for some iommu instances but not others?

BR,
-R

Quoted 4 lines

Regards,
Sricharan

author
#12 lore raw

v6 cover iommu/msm: Add DT adaptation and generic bindings support

Hi Rob,

Quoted 34 lines
Quoted 24 lines
Quoted 24 lines
Quoted 10 lines

btw, the current state, at least on linaro integration branch, fault handling doesn't work so well (ie. device never gets resumed).. which is a bit unfortunate for a gpu (and results in a *lot* of rebooting on my part when debugging userspace). I haven't had time yet to compare to the ancient downstream driver, but not sure if you have any ideas?

I guess probably disabling stall on fault would help. But I'm not even getting the "Fault occurred in context.." prints. Seeing the fault iova is pretty useful since that plus gpu cmdstream trace helps me figure out which texture/etc is being accessed out of bounds.

fyi, it looks like it is not getting any fault irq.. it's *possible* that I screwed up the irq #'s when translating from downstream, so you might want to double check that. I thought I had it right, I assume I would have noticed during piglit runs if fault recovery wasn't working (since the result is that *everything* after the faulting test would have failed since gpu is wedged with no access to memory), but it was long enough ago that I can't claim that definitively.

If you need an easy way to trigger a gpu fault, msmtest is a good way, change this line:

https://github.com/freedreno/msmtest/blob/master/msmtest.c#L247

Quoted 9 lines
Quoted 2 lines

from OUT_RELOC() to OUT_RING(ring, 0x00000000) will trigger a fault.

So for the irq to be triggered, 'non-secure' irq line has to be
populated in DT. There is a 'secure'and 'non-secure' irq lines for these iommus and non-secure irq number is secure + 1. I tested this by having a 'return 0'
from the msm_iommu_map (no mapping), and the faults were getting triggered.

Can you share me your dts data ?

Quoted 6 lines

I think this is what you want:

https://github.com/freedreno/kernel-msm/blob/integration-linux-qcomlt/arch/arm/boot/dts/qcom-apq8064.dtsi#L2008

I haven't tested a display fault, so I suppose it is possible that irq's are working for some iommu instances but not others?

So in your DT, for gfx3d, the non-secure line is '70' and not '69' (This is secure) .
Infact only '70' should be populated. The driver sets the irq line based on resource 0. This applies for all iommu nodes in your DT. (only the second irq line is needed).

Regards,
Sricharan

#13 lore raw

v6 cover iommu/msm: Add DT adaptation and generic bindings support

Quoted 38 lines · Sricharan

Hi Rob,

Quoted 36 lines
Quoted 24 lines
Quoted 24 lines
Quoted 10 lines

btw, the current state, at least on linaro integration branch, fault handling doesn't work so well (ie. device never gets resumed).. which is a bit unfortunate for a gpu (and results in a *lot* of rebooting on my part when debugging userspace). I haven't had time yet to compare to the ancient downstream driver, but not sure if you have any ideas?

I guess probably disabling stall on fault would help. But I'm not even getting the "Fault occurred in context.." prints. Seeing the fault iova is pretty useful since that plus gpu cmdstream trace helps me figure out which texture/etc is being accessed out of bounds.

fyi, it looks like it is not getting any fault irq.. it's *possible* that I screwed up the irq #'s when translating from downstream, so you might want to double check that. I thought I had it right, I assume I would have noticed during piglit runs if fault recovery wasn't working (since the result is that *everything* after the faulting test would have failed since gpu is wedged with no access to memory), but it was long enough ago that I can't claim that definitively.

If you need an easy way to trigger a gpu fault, msmtest is a good way, change this line:

https://github.com/freedreno/msmtest/blob/master/msmtest.c#L247

Quoted 9 lines
Quoted 2 lines

from OUT_RELOC() to OUT_RING(ring, 0x00000000) will trigger a fault.

So for the irq to be triggered, 'non-secure' irq line has to be
populated in DT. There is a 'secure'and 'non-secure' irq lines for these iommus and non-secure irq number is secure + 1. I tested this by having a 'return 0'
from the msm_iommu_map (no mapping), and the faults were getting triggered.

Can you share me your dts data ?

I think this is what you want:

Quoted 8 lines
Quoted 4 lines

https://github.com/freedreno/kernel-msm/blob/integration-linux-qcomlt/arch/arm/boot/dts/qcom-apq8064.dtsi#L2008

I haven't tested a display fault, so I suppose it is possible that irq's are working for some iommu instances but not others?

So in your DT, for gfx3d, the non-secure line is '70' and not '69' (This is secure) .
Infact only '70' should be populated. The driver sets the irq line based on resource 0. This applies for all iommu nodes in your DT. (only the second irq line is needed).

ahh, that would explain.

Is it better to remove the extra entry, or should I just swap them all? Ie. might there be some point in the future where the driver would want both?

BR,
-R

author
#14 lore raw

v6 cover iommu/msm: Add DT adaptation and generic bindings support

Hi,

Quoted 45 lines
Quoted 36 lines
Quoted 36 lines
Quoted 24 lines
Quoted 24 lines
Quoted 10 lines

btw, the current state, at least on linaro integration branch, fault handling doesn't work so well (ie. device never gets resumed).. which is a bit unfortunate for a gpu (and results in a *lot* of rebooting on my part when debugging userspace). I haven't had time yet to compare to the ancient downstream driver, but not sure if you have any ideas?

I guess probably disabling stall on fault would help. But I'm not even getting the "Fault occurred in context.." prints. Seeing the fault iova is pretty useful since that plus gpu cmdstream trace helps me figure out which texture/etc is being accessed out of bounds.

fyi, it looks like it is not getting any fault irq.. it's *possible* that I screwed up the irq #'s when translating from downstream, so you might want to double check that. I thought I had it right, I assume I would have noticed during piglit runs if fault recovery wasn't working (since the result is that *everything* after the faulting test would have failed since gpu is wedged with no access to memory), but it was long enough ago that I can't claim that definitively.

If you need an easy way to trigger a gpu fault, msmtest is a good way, change this line:

https://github.com/freedreno/msmtest/blob/master/msmtest.c#L247

Quoted 9 lines
Quoted 2 lines

from OUT_RELOC() to OUT_RING(ring, 0x00000000) will trigger a fault.

So for the irq to be triggered, 'non-secure' irq line has to be
populated in DT. There is a 'secure'and 'non-secure' irq lines for these iommus and non-secure irq number is secure + 1. I tested this by having a 'return 0'
from the msm_iommu_map (no mapping), and the faults were getting triggered.

Can you share me your dts data ?

I think this is what you want:

Quoted 8 lines
Quoted 4 lines

https://github.com/freedreno/kernel-msm/blob/integration-linux-qcomlt/arch/arm/boot/dts/qcom-apq8064.dtsi#L2008

I haven't tested a display fault, so I suppose it is possible that irq's are working for some iommu instances but not others?

So in your DT, for gfx3d, the non-secure line is '70' and not '69' (This is secure) .
Infact only '70' should be populated. The driver sets the irq line based on resource 0. This applies for all iommu nodes in your DT. (only the second irq line is needed).

Quoted 5 lines

ahh, that would explain.

Is it better to remove the extra entry, or should I just swap them all? Ie. might there be some point in the future where the driver would want both?

I feel better to have one. Not sure why the secure irq was added in first place in the downstream data and it would setup/handled by the TZ

Regards,
Sricharan

#15 lore raw

v6 cover iommu/msm: Add DT adaptation and generic bindings support

Quoted 47 lines · Sricharan

Hi,

Quoted 45 lines
Quoted 44 lines
Quoted 34 lines
Quoted 24 lines
Quoted 24 lines
Quoted 10 lines

btw, the current state, at least on linaro integration branch, fault handling doesn't work so well (ie. device never gets resumed).. which is a bit unfortunate for a gpu (and results in a *lot* of rebooting on my part when debugging userspace). I haven't had time yet to compare to the ancient downstream driver, but not sure if you have any ideas?

I guess probably disabling stall on fault would help. But I'm not even getting the "Fault occurred in context.." prints. Seeing the fault iova is pretty useful since that plus gpu cmdstream trace helps me figure out which texture/etc is being accessed out of bounds.

fyi, it looks like it is not getting any fault irq.. it's *possible* that I screwed up the irq #'s when translating from downstream, so you might want to double check that. I thought I had it right, I assume I would have noticed during piglit runs if fault recovery wasn't working (since the result is that *everything* after the faulting test would have failed since gpu is wedged with no access to memory), but it was long enough ago that I can't claim that definitively.

If you need an easy way to trigger a gpu fault, msmtest is a good way, change this line:

https://github.com/freedreno/msmtest/blob/master/msmtest.c#L247

Quoted 9 lines
Quoted 2 lines

from OUT_RELOC() to OUT_RING(ring, 0x00000000) will trigger a fault.

So for the irq to be triggered, 'non-secure' irq line has to be
populated in DT. There is a 'secure'and 'non-secure' irq lines for these iommus and non-secure irq number is secure + 1. I tested this by having a 'return 0'
from the msm_iommu_map (no mapping), and the faults were getting triggered.

Can you share me your dts data ?

Quoted 6 lines

I think this is what you want:

https://github.com/freedreno/kernel-msm/blob/integration-linux-qcomlt/arch/arm/boot/dts/qcom-apq8064.dtsi#L2008

I haven't tested a display fault, so I suppose it is possible that irq's are working for some iommu instances but not others?

So in your DT, for gfx3d, the non-secure line is '70' and not '69' (This is secure) .
Infact only '70' should be populated. The driver sets the irq line based on resource 0. This applies for all iommu nodes in your DT. (only the second irq line is needed).

Quoted 7 lines
Quoted 5 lines

ahh, that would explain.

Is it better to remove the extra entry, or should I just swap them all? Ie. might there be some point in the future where the driver would want both?

I feel better to have one. Not sure why the secure irq was added in first place in the downstream data and it would setup/handled by the TZ

Ok, getting further.. still seems like the gpu is not getting resumed, but at least we are getting a fault interrupt..

BR,
-R

#16 lore raw

v6 cover iommu/msm: Add DT adaptation and generic bindings support

Quoted 50 lines · Rob Clark
Quoted 49 lines · Sricharan

Hi,

Quoted 47 lines
Quoted 36 lines
Quoted 36 lines
Quoted 24 lines
Quoted 24 lines
Quoted 10 lines

btw, the current state, at least on linaro integration branch, fault handling doesn't work so well (ie. device never gets resumed).. which is a bit unfortunate for a gpu (and results in a *lot* of rebooting on my part when debugging userspace). I haven't had time yet to compare to the ancient downstream driver, but not sure if you have any ideas?

I guess probably disabling stall on fault would help. But I'm not even getting the "Fault occurred in context.." prints. Seeing the fault iova is pretty useful since that plus gpu cmdstream trace helps me figure out which texture/etc is being accessed out of bounds.

fyi, it looks like it is not getting any fault irq.. it's *possible* that I screwed up the irq #'s when translating from downstream, so you might want to double check that. I thought I had it right, I assume I would have noticed during piglit runs if fault recovery wasn't working (since the result is that *everything* after the faulting test would have failed since gpu is wedged with no access to memory), but it was long enough ago that I can't claim that definitively.

If you need an easy way to trigger a gpu fault, msmtest is a good way, change this line:

https://github.com/freedreno/msmtest/blob/master/msmtest.c#L247

Quoted 9 lines
Quoted 2 lines

from OUT_RELOC() to OUT_RING(ring, 0x00000000) will trigger a fault.

So for the irq to be triggered, 'non-secure' irq line has to be
populated in DT. There is a 'secure'and 'non-secure' irq lines for these iommus and non-secure irq number is secure + 1. I tested this by having a 'return 0'
from the msm_iommu_map (no mapping), and the faults were getting triggered.

Can you share me your dts data ?

I think this is what you want:

Quoted 8 lines
Quoted 4 lines

https://github.com/freedreno/kernel-msm/blob/integration-linux-qcomlt/arch/arm/boot/dts/qcom-apq8064.dtsi#L2008

I haven't tested a display fault, so I suppose it is possible that irq's are working for some iommu instances but not others?

So in your DT, for gfx3d, the non-secure line is '70' and not '69' (This is secure) .
Infact only '70' should be populated. The driver sets the irq line based on resource 0. This applies for all iommu nodes in your DT. (only the second irq line is needed).

ahh, that would explain.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

Is it better to remove the extra entry, or should I just swap them all? Ie. might there be some point in the future where the driver would want both?

I feel better to have one. Not sure why the secure irq was added in first place in the downstream data and it would setup/handled by the TZ

Ok, getting further.. still seems like the gpu is not getting resumed, but at least we are getting a fault interrupt..

ok, seems like we need:

-------

drivers/iommu/msm_iommu.c +1 -0
index b09692b..f6f596f 100644
@@ -628,6 +628,7 @@ irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)
             pr_err("Interesting registers:\n");
             print_ctx_regs(iommu->base, i);
             SET_FSR(iommu->base, i, 0x4000000F);
+            SET_RESUME(iommu->base, i, 1);
         }
     }
     __disable_clocks(iommu);

-------

with that it seems like it kinda/mostly recovers, although there is still a bit of strangeness..

BR,
-R

author
#17 lore raw

v6 cover iommu/msm: Add DT adaptation and generic bindings support

Hi,

Quoted 66 lines
Quoted 48 lines
Quoted 48 lines
Quoted 48 lines
Quoted 37 lines
Quoted 37 lines
Quoted 25 lines
Quoted 25 lines
Quoted 11 lines

btw, the current state, at least on linaro integration branch, fault handling doesn't work so well (ie. device never gets resumed).. which is a bit unfortunate for a gpu (and results in a *lot* of rebooting on my part when debugging userspace). I haven't had time yet to compare to the ancient downstream driver, but not sure if you have any ideas?

I guess probably disabling stall on fault would help. But I'm not even getting the "Fault occurred in context.." prints. Seeing the fault iova is pretty useful since that plus gpu cmdstream trace helps me figure out which texture/etc is being accessed out of bounds.

fyi, it looks like it is not getting any fault irq.. it's *possible* that I screwed up the irq #'s when translating from downstream, so you might want to double check that. I thought I had it right, I assume I would have noticed during piglit runs if fault recovery wasn't working (since the result is that *everything* after the faulting test would have failed since gpu is wedged with no access to memory), but it was long enough ago that I can't claim that definitively.

If you need an easy way to trigger a gpu fault, msmtest is a good way, change this line:

https://github.com/freedreno/msmtest/blob/master/msmtest.c#L247

Quoted 9 lines
Quoted 2 lines

from OUT_RELOC() to OUT_RING(ring, 0x00000000) will trigger a fault.

So for the irq to be triggered, 'non-secure' irq line has to be
populated in DT. There is a 'secure'and 'non-secure' irq lines for these iommus and non-secure irq number is secure + 1. I tested this by having a 'return 0'
from the msm_iommu_map (no mapping), and the faults were getting triggered.

Can you share me your dts data ?

I think this is what you want:

Quoted 8 lines
Quoted 4 lines

https://github.com/freedreno/kernel-msm/blob/integration-linux-qcomlt/arch/arm/boot/dts/qcom-apq8064.dtsi#L2008

I haven't tested a display fault, so I suppose it is possible that irq's are working for some iommu instances but not others?

So in your DT, for gfx3d, the non-secure line is '70' and not '69' (This is secure) .
Infact only '70' should be populated. The driver sets the irq line based on resource 0. This applies for all iommu nodes in your DT. (only the second irq line is needed).

ahh, that would explain.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

Is it better to remove the extra entry, or should I just swap them all? Ie. might there be some point in the future where the driver would want both?

I feel better to have one. Not sure why the secure irq was added in first place in the downstream data and it would setup/handled by the TZ

Ok, getting further.. still seems like the gpu is not getting resumed, but at least we are getting a fault interrupt..

ok, seems like we need:

-------

drivers/iommu/msm_iommu.c +0 -0
index b09692b..f6f596f 100644

drivers/iommu/msm_iommu.c irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id) · line 632

Quoted 5 lines
drivers/iommu/msm_iommu.c +1 -0
@@ -628,6 +628,7 @@ irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)
            pr_err("Interesting registers:\n");
            print_ctx_regs(iommu->base, i);
            SET_FSR(iommu->base, i, 0x4000000F);
+            SET_RESUME(iommu->base, i, 1);
        }

ok, ya this is required for disabling the stall. I can send a patch to change this.

Quoted 6 lines
drivers/iommu/msm_iommu.c +0 -1
    }
    __disable_clocks(iommu);
-------

with that it seems like it kinda/mostly recovers, although there is still a bit of strangeness..

hmm ok, can test it and check something particular ?

Regards,
Sricharan

#18 lore raw

v6 cover iommu/msm: Add DT adaptation and generic bindings support

In reply to Rob Clark
Quoted 78 lines · Rob Clark
Quoted 50 lines · Rob Clark
Quoted 49 lines · Sricharan

Hi,

Quoted 47 lines
Quoted 36 lines
Quoted 36 lines
Quoted 24 lines
Quoted 24 lines
Quoted 10 lines

btw, the current state, at least on linaro integration branch, fault handling doesn't work so well (ie. device never gets resumed).. which is a bit unfortunate for a gpu (and results in a *lot* of rebooting on my part when debugging userspace). I haven't had time yet to compare to the ancient downstream driver, but not sure if you have any ideas?

I guess probably disabling stall on fault would help. But I'm not even getting the "Fault occurred in context.." prints. Seeing the fault iova is pretty useful since that plus gpu cmdstream trace helps me figure out which texture/etc is being accessed out of bounds.

fyi, it looks like it is not getting any fault irq.. it's *possible* that I screwed up the irq #'s when translating from downstream, so you might want to double check that. I thought I had it right, I assume I would have noticed during piglit runs if fault recovery wasn't working (since the result is that *everything* after the faulting test would have failed since gpu is wedged with no access to memory), but it was long enough ago that I can't claim that definitively.

If you need an easy way to trigger a gpu fault, msmtest is a good way, change this line:

https://github.com/freedreno/msmtest/blob/master/msmtest.c#L247

Quoted 9 lines
Quoted 2 lines

from OUT_RELOC() to OUT_RING(ring, 0x00000000) will trigger a fault.

So for the irq to be triggered, 'non-secure' irq line has to be
populated in DT. There is a 'secure'and 'non-secure' irq lines for these iommus and non-secure irq number is secure + 1. I tested this by having a 'return 0'
from the msm_iommu_map (no mapping), and the faults were getting triggered.

Can you share me your dts data ?

I think this is what you want:

Quoted 8 lines
Quoted 4 lines

https://github.com/freedreno/kernel-msm/blob/integration-linux-qcomlt/arch/arm/boot/dts/qcom-apq8064.dtsi#L2008

I haven't tested a display fault, so I suppose it is possible that irq's are working for some iommu instances but not others?

So in your DT, for gfx3d, the non-secure line is '70' and not '69' (This is secure) .
Infact only '70' should be populated. The driver sets the irq line based on resource 0. This applies for all iommu nodes in your DT. (only the second irq line is needed).

ahh, that would explain.

Quoted 8 lines
Quoted 5 lines
Quoted 3 lines

Is it better to remove the extra entry, or should I just swap them all? Ie. might there be some point in the future where the driver would want both?

I feel better to have one. Not sure why the secure irq was added in first place in the downstream data and it would setup/handled by the TZ

Ok, getting further.. still seems like the gpu is not getting resumed, but at least we are getting a fault interrupt..

ok, seems like we need:

-------

drivers/iommu/msm_iommu.c +1 -1
index b09692b..f6f596f 100644
@@ -628,6 +628,7 @@ irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)
             pr_err("Interesting registers:\n");
             print_ctx_regs(iommu->base, i);
             SET_FSR(iommu->base, i, 0x4000000F);
+            SET_RESUME(iommu->base, i, 1);
         }
     }
     __disable_clocks(iommu);
-------
Quoted 2 lines

with that it seems like it kinda/mostly recovers, although there is still a bit of strangeness..

ok, the "still a bit of strangeness" was a false alarm (unrelated problem).. I'll send a patch to add the SET_RESUME().

And also one to wire up fault reporting. When things go wrong we can get thousands of faults from the GPU, so I'd rather it hit my simpler ratelimited print from the fault handler ;-)

BR,
-R

Quoted 2 lines

BR,
-R