[PATCH 0/3] msm: iommu: Further improvements to the MSM IOMMU driver

[PATCH 0/3] msm: iommu: Further improvements to the MSM IOMMU driver

[Stepan Moskovchenko]

This depends on the clock control driver. Please hold off
on this until the clock driver is in.

Simplify the IOMMU clock control, probe functions, and
initialization, since we no longer have to work around
having a dummy clock driver. As a result, all the IOMMU
devices can now be safely be probed and initialized. Also
add clock control for the IOMMU bus interconnect, which is
only needed for access to IOMMU registers and now can be
disabled for most of the time.

Stepan Moskovchenko (3):
  msm: iommu: Add bus clocks to platform data
  msm: iommu: Clock control for the IOMMU driver
  msm: iommu: Simplify the platform clock code

 arch/arm/mach-msm/devices-msm8x60-iommu.c |    5 -
 arch/arm/mach-msm/include/mach/iommu.h    |   12 +-
 arch/arm/mach-msm/iommu.c                 |   82 ++++++++++--
 arch/arm/mach-msm/iommu_dev.c             |  204 +++++++++++++++++------------
 4 files changed, 196 insertions(+), 107 deletions(-)

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 1/3] msm: iommu: Add bus clocks to platform data

[Stepan Moskovchenko]

Add the IOMMU bus clock and the IOMMU bus interconnect
clocks to the platform data for an IOMMU device. These
clocks are needed to access the IOMMU registers.

Signed-off-by: Stepan Moskovchenko <stepanm@codeaurora.org>
---
Please hold off on this until the clock driver is in.
 arch/arm/mach-msm/include/mach/iommu.h |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/arch/arm/mach-msm/include/mach/iommu.h b/arch/arm/mach-msm/include/mach/iommu.h
index 296c0f1..62f6699 100644
--- a/arch/arm/mach-msm/include/mach/iommu.h
+++ b/arch/arm/mach-msm/include/mach/iommu.h
@@ -19,6 +19,7 @@
 #define MSM_IOMMU_H

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

 /* Sharability attributes of MSM IOMMU mappings */
 #define MSM_IOMMU_ATTR_NON_SH		0x0
@@ -74,13 +75,17 @@ struct msm_iommu_ctx_dev {
  * struct msm_iommu_drvdata - A single IOMMU hardware instance
  * @base:	IOMMU config port base address (VA)
  * @irq:	Interrupt number
-  *
+ * @clk:	The bus clock for this IOMMU hardware instance
+ * @pclk:	The clock for the IOMMU bus interconnect
+ *
  * A msm_iommu_drvdata holds the global driver data about a single piece
  * of an IOMMU hardware instance.
  */
 struct msm_iommu_drvdata {
 	void __iomem *base;
 	int irq;
+	struct clk *clk;
+	struct clk *pclk;
 };

 /**
--
1.7.0.2

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 2/3] msm: iommu: Clock control for the IOMMU driver

[Stepan Moskovchenko]

Add clock control to the IOMMU driver. The IOMMU bus clock
(and potentially an AXI clock) need to be on to gain access
to IOMMU registers. Actively control these clocks when
needed instead of leaving them on.

Signed-off-by: Stepan Moskovchenko <stepanm@codeaurora.org>
---
Please hold off on this until the clock driver is in.
 arch/arm/mach-msm/iommu.c |   82 ++++++++++++++++++++++++++++++++++++++------
 1 files changed, 70 insertions(+), 12 deletions(-)

diff --git a/arch/arm/mach-msm/iommu.c b/arch/arm/mach-msm/iommu.c
index a468ee3..83381c3 100644
--- a/arch/arm/mach-msm/iommu.c
+++ b/arch/arm/mach-msm/iommu.c
@@ -26,6 +26,7 @@
 #include <linux/spinlock.h>
 #include <linux/slab.h>
 #include <linux/iommu.h>
+#include <linux/clk.h>

 #include <asm/cacheflush.h>
 #include <asm/sizes.h>
@@ -50,12 +51,36 @@ struct msm_priv {
 	struct list_head list_attached;
 };

-static void __flush_iotlb(struct iommu_domain *domain)
+static int __enable_clocks(struct msm_iommu_drvdata *drvdata)
+{
+	int ret;
+
+	ret = clk_enable(drvdata->pclk);
+	if (ret)
+		goto fail;
+
+	if (drvdata->clk) {
+		ret = clk_enable(drvdata->clk);
+		if (ret)
+			clk_disable(drvdata->pclk);
+	}
+fail:
+	return ret;
+}
+
+static void __disable_clocks(struct msm_iommu_drvdata *drvdata)
+{
+	if (drvdata->clk)
+		clk_disable(drvdata->clk);
+	clk_disable(drvdata->pclk);
+}
+
+static int __flush_iotlb(struct iommu_domain *domain)
 {
 	struct msm_priv *priv = domain->priv;
 	struct msm_iommu_drvdata *iommu_drvdata;
 	struct msm_iommu_ctx_drvdata *ctx_drvdata;
-
+	int ret = 0;
 #ifndef CONFIG_IOMMU_PGTABLES_L2
 	unsigned long *fl_table = priv->pgtable;
 	int i;
@@ -77,8 +102,18 @@ static void __flush_iotlb(struct iommu_domain *domain)
 			BUG();

 		iommu_drvdata = dev_get_drvdata(ctx_drvdata->pdev->dev.parent);
+		if (!iommu_drvdata)
+			BUG();
+
+		ret = __enable_clocks(iommu_drvdata);
+		if (ret)
+			goto fail;
+
 		SET_CTX_TLBIALL(iommu_drvdata->base, ctx_drvdata->num, 0);
+		__disable_clocks(iommu_drvdata);
 	}
+fail:
+	return ret;
 }

 static void __reset_context(void __iomem *base, int ctx)
@@ -263,11 +298,16 @@ static int msm_iommu_attach_dev(struct iommu_domain *domain, struct device *dev)
 			goto fail;
 		}

+	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);
-	__flush_iotlb(domain);
+	ret = __flush_iotlb(domain);

 fail:
 	spin_unlock_irqrestore(&msm_iommu_lock, flags);
@@ -282,6 +322,7 @@ static void msm_iommu_detach_dev(struct iommu_domain *domain,
 	struct msm_iommu_drvdata *iommu_drvdata;
 	struct msm_iommu_ctx_drvdata *ctx_drvdata;
 	unsigned long flags;
+	int ret;

 	spin_lock_irqsave(&msm_iommu_lock, flags);
 	priv = domain->priv;
@@ -296,8 +337,16 @@ static void msm_iommu_detach_dev(struct iommu_domain *domain,
 	if (!iommu_drvdata || !ctx_drvdata || !ctx_dev)
 		goto fail;

-	__flush_iotlb(domain);
+	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);

 fail:
@@ -410,7 +459,7 @@ static int msm_iommu_map(struct iommu_domain *domain, unsigned long va,
 				SL_AP1 | SL_SHARED | SL_TYPE_LARGE | pgprot;
 	}

-	__flush_iotlb(domain);
+	ret = __flush_iotlb(domain);
 fail:
 	spin_unlock_irqrestore(&msm_iommu_lock, flags);
 	return ret;
@@ -495,7 +544,7 @@ static int msm_iommu_unmap(struct iommu_domain *domain, unsigned long va,
 		}
 	}

-	__flush_iotlb(domain);
+	ret = __flush_iotlb(domain);
 fail:
 	spin_unlock_irqrestore(&msm_iommu_lock, flags);
 	return ret;
@@ -526,13 +575,14 @@ static phys_addr_t msm_iommu_iova_to_phys(struct iommu_domain *domain,
 	base = iommu_drvdata->base;
 	ctx = ctx_drvdata->num;

+	ret = __enable_clocks(iommu_drvdata);
+	if (ret)
+		goto fail;
+
 	/* Invalidate context TLB */
 	SET_CTX_TLBIALL(base, ctx, 0);
 	SET_V2PPR_VA(base, ctx, va >> V2Pxx_VA_SHIFT);

-	if (GET_FAULT(base, ctx))
-		goto fail;
-
 	par = GET_PAR(base, ctx);

 	/* We are dealing with a supersection */
@@ -541,6 +591,10 @@ static phys_addr_t msm_iommu_iova_to_phys(struct iommu_domain *domain,
 	else	/* Upper 20 bits from PAR, lower 12 from VA */
 		ret = (par & 0xFFFFF000) | (va & 0x00000FFF);

+	if (GET_FAULT(base, ctx))
+		ret = 0;
+
+	__disable_clocks(iommu_drvdata);
 fail:
 	spin_unlock_irqrestore(&msm_iommu_lock, flags);
 	return ret;
@@ -583,8 +637,8 @@ irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)
 {
 	struct msm_iommu_drvdata *drvdata = dev_id;
 	void __iomem *base;
-	unsigned int fsr = 0;
-	int ncb = 0, i = 0;
+	unsigned int fsr;
+	int ncb, i, ret;

 	spin_lock(&msm_iommu_lock);

@@ -595,10 +649,13 @@ irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)

 	base = drvdata->base;

-	pr_err("===== WOAH! =====\n");
 	pr_err("Unexpected IOMMU page fault!\n");
 	pr_err("base = %08x\n", (unsigned int) base);

+	ret = __enable_clocks(drvdata);
+	if (ret)
+		goto fail;
+
 	ncb = GET_NCB(base)+1;
 	for (i = 0; i < ncb; i++) {
 		fsr = GET_FSR(base, i);
@@ -609,6 +666,7 @@ irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)
 			SET_FSR(base, i, 0x4000000F);
 		}
 	}
+	__disable_clocks(drvdata);
 fail:
 	spin_unlock(&msm_iommu_lock);
 	return 0;
--
1.7.0.2

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 2/3] msm: iommu: Clock control for the IOMMU driver

[Daniel Walker]

On Fri, 2010-11-19 at 19:02 -0800, Stepan Moskovchenko wrote:
> Add clock control to the IOMMU driver. The IOMMU bus clock
> (and potentially an AXI clock) need to be on to gain access
> to IOMMU registers. Actively control these clocks when
> needed instead of leaving them on.
> 
> Signed-off-by: Stepan Moskovchenko <stepanm@codeaurora.org>
> ---
> Please hold off on this until the clock driver is in.
>  arch/arm/mach-msm/iommu.c |   82 ++++++++++++++++++++++++++++++++++++++------
>  1 files changed, 70 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm/mach-msm/iommu.c b/arch/arm/mach-msm/iommu.c
> index a468ee3..83381c3 100644
> --- a/arch/arm/mach-msm/iommu.c
> +++ b/arch/arm/mach-msm/iommu.c
> @@ -26,6 +26,7 @@
>  #include <linux/spinlock.h>
>  #include <linux/slab.h>
>  #include <linux/iommu.h>
> +#include <linux/clk.h>
> 
>  #include <asm/cacheflush.h>
>  #include <asm/sizes.h>
> @@ -50,12 +51,36 @@ struct msm_priv {
>  	struct list_head list_attached;
>  };
> 
> -static void __flush_iotlb(struct iommu_domain *domain)
> +static int __enable_clocks(struct msm_iommu_drvdata *drvdata)
> +{
> +	int ret;
> +
> +	ret = clk_enable(drvdata->pclk);

You don't need to check if pclk is null ?

> +	if (ret)
> +		goto fail;
> +
> +	if (drvdata->clk) {
> +		ret = clk_enable(drvdata->clk);
> +		if (ret)
> +			clk_disable(drvdata->pclk);
> +	}
> +fail:
> +	return ret;
> +}
> +
> +static void __disable_clocks(struct msm_iommu_drvdata *drvdata)
> +{
> +	if (drvdata->clk)
> +		clk_disable(drvdata->clk);
> +	clk_disable(drvdata->pclk);
> +}
> +
> +static int __flush_iotlb(struct iommu_domain *domain)
>  {
>  	struct msm_priv *priv = domain->priv;
>  	struct msm_iommu_drvdata *iommu_drvdata;
>  	struct msm_iommu_ctx_drvdata *ctx_drvdata;
> -
> +	int ret = 0;
>  #ifndef CONFIG_IOMMU_PGTABLES_L2
>  	unsigned long *fl_table = priv->pgtable;
>  	int i;
> @@ -77,8 +102,18 @@ static void __flush_iotlb(struct iommu_domain *domain)
>  			BUG();
> 
>  		iommu_drvdata = dev_get_drvdata(ctx_drvdata->pdev->dev.parent);
> +		if (!iommu_drvdata)
> +			BUG();

Just do,

BUG_ON(!iommu_drvdata);

> +		ret = __enable_clocks(iommu_drvdata);
> +		if (ret)
> +			goto fail;
> +
>  		SET_CTX_TLBIALL(iommu_drvdata->base, ctx_drvdata->num, 0);
> +		__disable_clocks(iommu_drvdata);
>  	}
> +fail:
> +	return ret;
>  }
> 
>  static void __reset_context(void __iomem *base, int ctx)
> @@ -263,11 +298,16 @@ static int msm_iommu_attach_dev(struct iommu_domain *domain, struct device *dev)
>  			goto fail;
>  		}
> 
> +	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);
> -	__flush_iotlb(domain);
> +	ret = __flush_iotlb(domain);
> 
>  fail:
>  	spin_unlock_irqrestore(&msm_iommu_lock, flags);
> @@ -282,6 +322,7 @@ static void msm_iommu_detach_dev(struct iommu_domain *domain,
>  	struct msm_iommu_drvdata *iommu_drvdata;
>  	struct msm_iommu_ctx_drvdata *ctx_drvdata;
>  	unsigned long flags;
> +	int ret;
> 
>  	spin_lock_irqsave(&msm_iommu_lock, flags);
>  	priv = domain->priv;
> @@ -296,8 +337,16 @@ static void msm_iommu_detach_dev(struct iommu_domain *domain,
>  	if (!iommu_drvdata || !ctx_drvdata || !ctx_dev)
>  		goto fail;
> 
> -	__flush_iotlb(domain);
> +	ret = __flush_iotlb(domain);

What the relationship between this __flush_iotlb() and turning the
clocks on/off.

> +	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);
> 
>  fail:
> @@ -410,7 +459,7 @@ static int msm_iommu_map(struct iommu_domain *domain, unsigned long va,
>  				SL_AP1 | SL_SHARED | SL_TYPE_LARGE | pgprot;
>  	}
> 
> -	__flush_iotlb(domain);
> +	ret = __flush_iotlb(domain);
>  fail:
>  	spin_unlock_irqrestore(&msm_iommu_lock, flags);
>  	return ret;
> @@ -495,7 +544,7 @@ static int msm_iommu_unmap(struct iommu_domain *domain, unsigned long va,
>  		}
>  	}
> 
> -	__flush_iotlb(domain);
> +	ret = __flush_iotlb(domain);
>  fail:
>  	spin_unlock_irqrestore(&msm_iommu_lock, flags);
>  	return ret;
> @@ -526,13 +575,14 @@ static phys_addr_t msm_iommu_iova_to_phys(struct iommu_domain *domain,
>  	base = iommu_drvdata->base;
>  	ctx = ctx_drvdata->num;
> 
> +	ret = __enable_clocks(iommu_drvdata);
> +	if (ret)
> +		goto fail;
> +
>  	/* Invalidate context TLB */
>  	SET_CTX_TLBIALL(base, ctx, 0);
>  	SET_V2PPR_VA(base, ctx, va >> V2Pxx_VA_SHIFT);
> 
> -	if (GET_FAULT(base, ctx))
> -		goto fail;
> -
>  	par = GET_PAR(base, ctx);
> 
>  	/* We are dealing with a supersection */
> @@ -541,6 +591,10 @@ static phys_addr_t msm_iommu_iova_to_phys(struct iommu_domain *domain,
>  	else	/* Upper 20 bits from PAR, lower 12 from VA */
>  		ret = (par & 0xFFFFF000) | (va & 0x00000FFF);
> 
> +	if (GET_FAULT(base, ctx))
> +		ret = 0;
> +
> +	__disable_clocks(iommu_drvdata);
>  fail:
>  	spin_unlock_irqrestore(&msm_iommu_lock, flags);
>  	return ret;
> @@ -583,8 +637,8 @@ irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)
>  {
>  	struct msm_iommu_drvdata *drvdata = dev_id;
>  	void __iomem *base;
> -	unsigned int fsr = 0;
> -	int ncb = 0, i = 0;
> +	unsigned int fsr;
> +	int ncb, i, ret;
> 
>  	spin_lock(&msm_iommu_lock);
> 
> @@ -595,10 +649,13 @@ irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)
> 
>  	base = drvdata->base;
> 
> -	pr_err("===== WOAH! =====\n");

Cleanup right? It doesn't need it's own patch, but you could mention in
the description that you've done "minor cleanups" or something to that
effect.

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH 2/3] msm: iommu: Clock control for the IOMMU driver

[Stepan Moskovchenko]

On 11/22/2010 3:32 PM, Daniel Walker wrote:
> On Fri, 2010-11-19 at 19:02 -0800, Stepan Moskovchenko wrote:
>> +	int ret;
>> +
>> +	ret = clk_enable(drvdata->pclk);
> You don't need to check if pclk is null ?
>
Nope. If we are here, the pclk will always be non-null, which is 
something that may not necessarily be said for for AXI clock.

>>   		iommu_drvdata = dev_get_drvdata(ctx_drvdata->pdev->dev.parent);
>> +		if (!iommu_drvdata)
>> +			BUG();
> Just do,
>
> BUG_ON(!iommu_drvdata);
Will fix in v2.

>> -	__flush_iotlb(domain);
>> +	ret = __flush_iotlb(domain);
> What the relationship between this __flush_iotlb() and turning the
> clocks on/off.
>
The flush_iotlb function needs to handle clock control as well, which 
means it can now fail. As a result, we now need to check its return 
value, instead of assuming it succeeded.
>> -	pr_err("===== WOAH! =====\n");
> Cleanup right? It doesn't need it's own patch, but you could mention in
> the description that you've done "minor cleanups" or something to that
> effect.
>
Will fix in v2.

[PATCH v2 2/3] msm: iommu: Clock control for the IOMMU driver

[Stepan Moskovchenko]

Add clock control to the IOMMU driver. The IOMMU bus clock
(and potentially an AXI clock) need to be on to gain access
to IOMMU registers. Actively control these clocks when
needed instead of leaving them on. Additionally, perform
minor code cleanups.

Signed-off-by: Stepan Moskovchenko <stepanm@codeaurora.org>
---
Please hold off on this until the clock driver is in.
Changes in v2:
 * Two minor cleanups of the form of changing if (..) BUG() to BUG_ON()
 * Amend the commit text to that effect

 arch/arm/mach-msm/iommu.c |   84 +++++++++++++++++++++++++++++++++++++-------
 1 files changed, 70 insertions(+), 14 deletions(-)

diff --git a/arch/arm/mach-msm/iommu.c b/arch/arm/mach-msm/iommu.c
index a468ee3..3cdecb9 100644
--- a/arch/arm/mach-msm/iommu.c
+++ b/arch/arm/mach-msm/iommu.c
@@ -26,6 +26,7 @@
 #include <linux/spinlock.h>
 #include <linux/slab.h>
 #include <linux/iommu.h>
+#include <linux/clk.h>

 #include <asm/cacheflush.h>
 #include <asm/sizes.h>
@@ -50,12 +51,36 @@ struct msm_priv {
 	struct list_head list_attached;
 };

-static void __flush_iotlb(struct iommu_domain *domain)
+static int __enable_clocks(struct msm_iommu_drvdata *drvdata)
+{
+	int ret;
+
+	ret = clk_enable(drvdata->pclk);
+	if (ret)
+		goto fail;
+
+	if (drvdata->clk) {
+		ret = clk_enable(drvdata->clk);
+		if (ret)
+			clk_disable(drvdata->pclk);
+	}
+fail:
+	return ret;
+}
+
+static void __disable_clocks(struct msm_iommu_drvdata *drvdata)
+{
+	if (drvdata->clk)
+		clk_disable(drvdata->clk);
+	clk_disable(drvdata->pclk);
+}
+
+static int __flush_iotlb(struct iommu_domain *domain)
 {
 	struct msm_priv *priv = domain->priv;
 	struct msm_iommu_drvdata *iommu_drvdata;
 	struct msm_iommu_ctx_drvdata *ctx_drvdata;
-
+	int ret = 0;
 #ifndef CONFIG_IOMMU_PGTABLES_L2
 	unsigned long *fl_table = priv->pgtable;
 	int i;
@@ -73,12 +98,20 @@ static void __flush_iotlb(struct iommu_domain *domain)
 #endif

 	list_for_each_entry(ctx_drvdata, &priv->list_attached, attached_elm) {
-		if (!ctx_drvdata->pdev || !ctx_drvdata->pdev->dev.parent)
-			BUG();
+		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);
+		if (ret)
+			goto fail;
+
 		SET_CTX_TLBIALL(iommu_drvdata->base, ctx_drvdata->num, 0);
+		__disable_clocks(iommu_drvdata);
 	}
+fail:
+	return ret;
 }

 static void __reset_context(void __iomem *base, int ctx)
@@ -263,11 +296,16 @@ static int msm_iommu_attach_dev(struct iommu_domain *domain, struct device *dev)
 			goto fail;
 		}

+	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);
-	__flush_iotlb(domain);
+	ret = __flush_iotlb(domain);

 fail:
 	spin_unlock_irqrestore(&msm_iommu_lock, flags);
@@ -282,6 +320,7 @@ static void msm_iommu_detach_dev(struct iommu_domain *domain,
 	struct msm_iommu_drvdata *iommu_drvdata;
 	struct msm_iommu_ctx_drvdata *ctx_drvdata;
 	unsigned long flags;
+	int ret;

 	spin_lock_irqsave(&msm_iommu_lock, flags);
 	priv = domain->priv;
@@ -296,8 +335,16 @@ static void msm_iommu_detach_dev(struct iommu_domain *domain,
 	if (!iommu_drvdata || !ctx_drvdata || !ctx_dev)
 		goto fail;

-	__flush_iotlb(domain);
+	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);

 fail:
@@ -410,7 +457,7 @@ static int msm_iommu_map(struct iommu_domain *domain, unsigned long va,
 				SL_AP1 | SL_SHARED | SL_TYPE_LARGE | pgprot;
 	}

-	__flush_iotlb(domain);
+	ret = __flush_iotlb(domain);
 fail:
 	spin_unlock_irqrestore(&msm_iommu_lock, flags);
 	return ret;
@@ -495,7 +542,7 @@ static int msm_iommu_unmap(struct iommu_domain *domain, unsigned long va,
 		}
 	}

-	__flush_iotlb(domain);
+	ret = __flush_iotlb(domain);
 fail:
 	spin_unlock_irqrestore(&msm_iommu_lock, flags);
 	return ret;
@@ -526,13 +573,14 @@ static phys_addr_t msm_iommu_iova_to_phys(struct iommu_domain *domain,
 	base = iommu_drvdata->base;
 	ctx = ctx_drvdata->num;

+	ret = __enable_clocks(iommu_drvdata);
+	if (ret)
+		goto fail;
+
 	/* Invalidate context TLB */
 	SET_CTX_TLBIALL(base, ctx, 0);
 	SET_V2PPR_VA(base, ctx, va >> V2Pxx_VA_SHIFT);

-	if (GET_FAULT(base, ctx))
-		goto fail;
-
 	par = GET_PAR(base, ctx);

 	/* We are dealing with a supersection */
@@ -541,6 +589,10 @@ static phys_addr_t msm_iommu_iova_to_phys(struct iommu_domain *domain,
 	else	/* Upper 20 bits from PAR, lower 12 from VA */
 		ret = (par & 0xFFFFF000) | (va & 0x00000FFF);

+	if (GET_FAULT(base, ctx))
+		ret = 0;
+
+	__disable_clocks(iommu_drvdata);
 fail:
 	spin_unlock_irqrestore(&msm_iommu_lock, flags);
 	return ret;
@@ -583,8 +635,8 @@ irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)
 {
 	struct msm_iommu_drvdata *drvdata = dev_id;
 	void __iomem *base;
-	unsigned int fsr = 0;
-	int ncb = 0, i = 0;
+	unsigned int fsr;
+	int ncb, i, ret;

 	spin_lock(&msm_iommu_lock);

@@ -595,10 +647,13 @@ irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)

 	base = drvdata->base;

-	pr_err("===== WOAH! =====\n");
 	pr_err("Unexpected IOMMU page fault!\n");
 	pr_err("base = %08x\n", (unsigned int) base);

+	ret = __enable_clocks(drvdata);
+	if (ret)
+		goto fail;
+
 	ncb = GET_NCB(base)+1;
 	for (i = 0; i < ncb; i++) {
 		fsr = GET_FSR(base, i);
@@ -609,6 +664,7 @@ irqreturn_t msm_iommu_fault_handler(int irq, void *dev_id)
 			SET_FSR(base, i, 0x4000000F);
 		}
 	}
+	__disable_clocks(drvdata);
 fail:
 	spin_unlock(&msm_iommu_lock);
 	return 0;
--
1.7.0.2

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH v2 3/3] msm: iommu: Rework clock logic and add IOMMU bus clock control

[Stepan Moskovchenko]

Clean up the clock control code in the probe calls, and add
support for controlling the clock for the IOMMU bus
interconnect. With the (proper) clock driver in place, the
clock control logic in the probe function can be made much
cleaner since it does not have to deal with the placeholder
driver anymore.

Signed-off-by: Stepan Moskovchenko <stepanm@codeaurora.org>
---
Please hold off on this until the clock driver is in.
The patch seems like it is a lot of changes, but a lot of
it comes from removing one condition, which causes a bunch
of code to be unindented by one level. This implementation
is a lot cleaner IMO.

Changes in v2:
 * Remove two unnecessart memset calls
 * Fix alignment / whitespace

 arch/arm/mach-msm/devices-msm8x60-iommu.c |    5 -
 arch/arm/mach-msm/include/mach/iommu.h    |    5 -
 arch/arm/mach-msm/iommu_dev.c             |  204 +++++++++++++++++------------
 3 files changed, 119 insertions(+), 95 deletions(-)

diff --git a/arch/arm/mach-msm/devices-msm8x60-iommu.c b/arch/arm/mach-msm/devices-msm8x60-iommu.c
index f9e7bd3..e12d7e2 100644
--- a/arch/arm/mach-msm/devices-msm8x60-iommu.c
+++ b/arch/arm/mach-msm/devices-msm8x60-iommu.c
@@ -282,7 +282,6 @@ static struct platform_device msm_root_iommu_dev = {

 static struct msm_iommu_dev jpegd_iommu = {
 	.name = "jpegd",
-	.clk_rate = -1
 };

 static struct msm_iommu_dev vpe_iommu = {
@@ -307,7 +306,6 @@ static struct msm_iommu_dev ijpeg_iommu = {

 static struct msm_iommu_dev vfe_iommu = {
 	.name = "vfe",
-	.clk_rate = -1
 };

 static struct msm_iommu_dev vcodec_a_iommu = {
@@ -320,17 +318,14 @@ static struct msm_iommu_dev vcodec_b_iommu = {

 static struct msm_iommu_dev gfx3d_iommu = {
 	.name = "gfx3d",
-	.clk_rate = 27000000
 };

 static struct msm_iommu_dev gfx2d0_iommu = {
 	.name = "gfx2d0",
-	.clk_rate = 27000000
 };

 static struct msm_iommu_dev gfx2d1_iommu = {
 	.name = "gfx2d1",
-	.clk_rate = 27000000
 };

 static struct platform_device msm_device_iommu_jpegd = {
diff --git a/arch/arm/mach-msm/include/mach/iommu.h b/arch/arm/mach-msm/include/mach/iommu.h
index 62f6699..10811ba 100644
--- a/arch/arm/mach-msm/include/mach/iommu.h
+++ b/arch/arm/mach-msm/include/mach/iommu.h
@@ -45,14 +45,9 @@
 /**
  * struct msm_iommu_dev - a single IOMMU hardware instance
  * name		Human-readable name given to this IOMMU HW instance
- * clk_rate	Rate to set for this IOMMU's clock, if applicable to this
- *		particular IOMMU. 0 means don't set a rate.
- *		-1 means it is an AXI clock with no valid rate
- *
  */
 struct msm_iommu_dev {
 	const char *name;
-	int clk_rate;
 };

 /**
diff --git a/arch/arm/mach-msm/iommu_dev.c b/arch/arm/mach-msm/iommu_dev.c
index b83c73b..7819ed2 100644
--- a/arch/arm/mach-msm/iommu_dev.c
+++ b/arch/arm/mach-msm/iommu_dev.c
@@ -29,6 +29,7 @@

 #include <mach/iommu_hw-8xxx.h>
 #include <mach/iommu.h>
+#include <mach/clk.h>

 struct iommu_ctx_iter_data {
 	/* input */
@@ -130,117 +131,134 @@ static int msm_iommu_probe(struct platform_device *pdev)
 {
 	struct resource *r, *r2;
 	struct clk *iommu_clk;
+	struct clk *iommu_pclk;
 	struct msm_iommu_drvdata *drvdata;
 	struct msm_iommu_dev *iommu_dev = pdev->dev.platform_data;
 	void __iomem *regs_base;
 	resource_size_t	len;
-	int ret = 0, ncb, nm2v, irq;
+	int ret, ncb, nm2v, irq;

-	if (pdev->id != -1) {
-		drvdata = kzalloc(sizeof(*drvdata), GFP_KERNEL);
+	if (pdev->id == -1) {
+		msm_iommu_root_dev = pdev;
+		return 0;
+	}

-		if (!drvdata) {
-			ret = -ENOMEM;
-			goto fail;
-		}
+	drvdata = kzalloc(sizeof(*drvdata), GFP_KERNEL);

-		if (!iommu_dev) {
-			ret = -ENODEV;
-			goto fail;
-		}
+	if (!drvdata) {
+		ret = -ENOMEM;
+		goto fail;
+	}
+
+	if (!iommu_dev) {
+		ret = -ENODEV;
+		goto fail;
+	}

-		if (iommu_dev->clk_rate != 0) {
-			iommu_clk = clk_get(&pdev->dev, "iommu_clk");
-
-			if (IS_ERR(iommu_clk)) {
-				ret = -ENODEV;
-				goto fail;
-			}
-
-			if (iommu_dev->clk_rate > 0) {
-				ret = clk_set_rate(iommu_clk,
-							iommu_dev->clk_rate);
-				if (ret) {
-					clk_put(iommu_clk);
-					goto fail;
-				}
-			}
-
-			ret = clk_enable(iommu_clk);
-			if (ret) {
-				clk_put(iommu_clk);
-				goto fail;
-			}
+	iommu_pclk = clk_get(NULL, "smmu_pclk");
+	if (IS_ERR(iommu_pclk)) {
+		ret = -ENODEV;
+		goto fail;
+	}
+
+	ret = clk_enable(iommu_pclk);
+	if (ret)
+		goto fail_enable;
+
+	iommu_clk = clk_get(&pdev->dev, "iommu_clk");
+
+	if (!IS_ERR(iommu_clk))	{
+		if (clk_get_rate(iommu_clk) == 0)
+			clk_set_min_rate(iommu_clk, 1);
+
+		ret = clk_enable(iommu_clk);
+		if (ret) {
 			clk_put(iommu_clk);
+			goto fail_pclk;
 		}
+	} else
+		iommu_clk = NULL;

-		r = platform_get_resource_byname(pdev, IORESOURCE_MEM,
-						 "physbase");
-		if (!r) {
-			ret = -ENODEV;
-			goto fail;
-		}
+	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "physbase");

-		len = r->end - r->start + 1;
+	if (!r) {
+		ret = -ENODEV;
+		goto fail_clk;
+	}

-		r2 = request_mem_region(r->start, len, r->name);
-		if (!r2) {
-			pr_err("Could not request memory region: "
-			"start=%p, len=%d\n", (void *) r->start, len);
-			ret = -EBUSY;
-			goto fail;
-		}
+	len = r->end - r->start + 1;

-		regs_base = ioremap(r2->start, len);
+	r2 = request_mem_region(r->start, len, r->name);
+	if (!r2) {
+		pr_err("Could not request memory region: start=%p, len=%d\n",
+			(void *) r->start, len);
+		ret = -EBUSY;
+		goto fail_clk;
+	}

-		if (!regs_base) {
-			pr_err("Could not ioremap: start=%p, len=%d\n",
-				 (void *) r2->start, len);
-			ret = -EBUSY;
-			goto fail_mem;
-		}
+	regs_base = ioremap(r2->start, len);

-		irq = platform_get_irq_byname(pdev, "secure_irq");
-		if (irq < 0) {
-			ret = -ENODEV;
-			goto fail_io;
-		}
+	if (!regs_base) {
+		pr_err("Could not ioremap: start=%p, len=%d\n",
+			 (void *) r2->start, len);
+		ret = -EBUSY;
+		goto fail_mem;
+	}
+
+	irq = platform_get_irq_byname(pdev, "secure_irq");
+	if (irq < 0) {
+		ret = -ENODEV;
+		goto fail_io;
+	}

-		mb();
+	mb();

-		if (GET_IDR(regs_base) == 0) {
-			pr_err("Invalid IDR value detected\n");
-			ret = -ENODEV;
-			goto fail_io;
-		}
+	if (GET_IDR(regs_base) == 0) {
+		pr_err("Invalid IDR value detected\n");
+		ret = -ENODEV;
+		goto fail_io;
+	}

-		ret = request_irq(irq, msm_iommu_fault_handler, 0,
-				"msm_iommu_secure_irpt_handler", drvdata);
-		if (ret) {
-			pr_err("Request IRQ %d failed with ret=%d\n", irq, ret);
-			goto fail_io;
-		}
+	ret = request_irq(irq, msm_iommu_fault_handler, 0,
+			"msm_iommu_secure_irpt_handler", drvdata);
+	if (ret) {
+		pr_err("Request IRQ %d failed with ret=%d\n", irq, ret);
+		goto fail_io;
+	}

-		msm_iommu_reset(regs_base);
-		drvdata->base = regs_base;
-		drvdata->irq = irq;
+	msm_iommu_reset(regs_base);
+	drvdata->pclk = iommu_pclk;
+	drvdata->clk = iommu_clk;
+	drvdata->base = regs_base;
+	drvdata->irq = irq;

-		nm2v = GET_NM2VCBMT((unsigned long) regs_base);
-		ncb = GET_NCB((unsigned long) regs_base);
+	nm2v = GET_NM2VCBMT((unsigned long) regs_base);
+	ncb = GET_NCB((unsigned long) regs_base);

-		pr_info("device %s mapped at %p, irq %d with %d ctx banks\n",
+	pr_info("device %s mapped at %p, irq %d with %d ctx banks\n",
 			iommu_dev->name, regs_base, irq, ncb+1);

-		platform_set_drvdata(pdev, drvdata);
-	} else
-		msm_iommu_root_dev = pdev;
+	platform_set_drvdata(pdev, drvdata);

-	return 0;
+	if (iommu_clk)
+		clk_disable(iommu_clk);
+
+	clk_disable(iommu_pclk);

+	return 0;
 fail_io:
 	iounmap(regs_base);
 fail_mem:
 	release_mem_region(r->start, len);
+fail_clk:
+	if (iommu_clk) {
+		clk_disable(iommu_clk);
+		clk_put(iommu_clk);
+	}
+fail_pclk:
+	clk_disable(iommu_pclk);
+fail_enable:
+	clk_put(iommu_pclk);
 fail:
 	kfree(drvdata);
 	return ret;
@@ -252,7 +270,9 @@ static int msm_iommu_remove(struct platform_device *pdev)

 	drv = platform_get_drvdata(pdev);
 	if (drv) {
-		memset(drv, 0, sizeof(struct msm_iommu_drvdata));
+		if (drv->clk)
+			clk_put(drv->clk);
+		clk_put(drv->pclk);
 		kfree(drv);
 		platform_set_drvdata(pdev, NULL);
 	}
@@ -264,7 +284,7 @@ static int msm_iommu_ctx_probe(struct platform_device *pdev)
 	struct msm_iommu_ctx_dev *c = pdev->dev.platform_data;
 	struct msm_iommu_drvdata *drvdata;
 	struct msm_iommu_ctx_drvdata *ctx_drvdata = NULL;
-	int i, ret = 0;
+	int i, ret;
 	if (!c || !pdev->dev.parent) {
 		ret = -EINVAL;
 		goto fail;
@@ -288,6 +308,18 @@ static int msm_iommu_ctx_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&ctx_drvdata->attached_elm);
 	platform_set_drvdata(pdev, ctx_drvdata);

+	ret = clk_enable(drvdata->pclk);
+	if (ret)
+		goto fail;
+
+	if (drvdata->clk) {
+		ret = clk_enable(drvdata->clk);
+		if (ret) {
+			clk_disable(drvdata->pclk);
+			goto fail;
+		}
+	}
+
 	/* Program the M2V tables for this context */
 	for (i = 0; i < MAX_NUM_MIDS; i++) {
 		int mid = c->mids[i];
@@ -310,8 +342,11 @@ static int msm_iommu_ctx_probe(struct platform_device *pdev)
 		SET_NSCFG(drvdata->base, mid, 3);
 	}

-	pr_info("context device %s with bank index %d\n", c->name, c->num);
+	if (drvdata->clk)
+		clk_disable(drvdata->clk);
+	clk_disable(drvdata->pclk);

+	dev_info(&pdev->dev, "context %s using bank %d\n", c->name, c->num);
 	return 0;
 fail:
 	kfree(ctx_drvdata);
@@ -323,7 +358,6 @@ static int msm_iommu_ctx_remove(struct platform_device *pdev)
 	struct msm_iommu_ctx_drvdata *drv = NULL;
 	drv = platform_get_drvdata(pdev);
 	if (drv) {
-		memset(drv, 0, sizeof(struct msm_iommu_ctx_drvdata));
 		kfree(drv);
 		platform_set_drvdata(pdev, NULL);
 	}
--
1.7.0.2

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 3/3] msm: iommu: Rework clock logic and add IOMMU bus clock control

[Stepan Moskovchenko]

Clean up the clock control code in the probe calls, and add
support for controlling the clock for the IOMMU bus
interconnect. With the (proper) clock driver in place, the
clock control logic in the probe function can be made much
cleaner since it does not have to deal with the placeholder
driver anymore.

Change-Id: I1040bc4e18f4ab4b7cc0dd5fe667f9df83b9f1f5
Signed-off-by: Stepan Moskovchenko <stepanm@codeaurora.org>
---
Please hold off on this until the clock driver is in.
The patch seems like it is a lot of changes, but a lot of
it comes from removing one condition, which causes a bunch
of code to be unindented by one level. This implementation
is a lot cleaner IMO.
 arch/arm/mach-msm/devices-msm8x60-iommu.c |    5 -
 arch/arm/mach-msm/include/mach/iommu.h    |    5 -
 arch/arm/mach-msm/iommu_dev.c             |  204 +++++++++++++++++------------
 3 files changed, 120 insertions(+), 94 deletions(-)

diff --git a/arch/arm/mach-msm/devices-msm8x60-iommu.c b/arch/arm/mach-msm/devices-msm8x60-iommu.c
index f9e7bd3..e12d7e2 100644
--- a/arch/arm/mach-msm/devices-msm8x60-iommu.c
+++ b/arch/arm/mach-msm/devices-msm8x60-iommu.c
@@ -282,7 +282,6 @@ static struct platform_device msm_root_iommu_dev = {

 static struct msm_iommu_dev jpegd_iommu = {
 	.name = "jpegd",
-	.clk_rate = -1
 };

 static struct msm_iommu_dev vpe_iommu = {
@@ -307,7 +306,6 @@ static struct msm_iommu_dev ijpeg_iommu = {

 static struct msm_iommu_dev vfe_iommu = {
 	.name = "vfe",
-	.clk_rate = -1
 };

 static struct msm_iommu_dev vcodec_a_iommu = {
@@ -320,17 +318,14 @@ static struct msm_iommu_dev vcodec_b_iommu = {

 static struct msm_iommu_dev gfx3d_iommu = {
 	.name = "gfx3d",
-	.clk_rate = 27000000
 };

 static struct msm_iommu_dev gfx2d0_iommu = {
 	.name = "gfx2d0",
-	.clk_rate = 27000000
 };

 static struct msm_iommu_dev gfx2d1_iommu = {
 	.name = "gfx2d1",
-	.clk_rate = 27000000
 };

 static struct platform_device msm_device_iommu_jpegd = {
diff --git a/arch/arm/mach-msm/include/mach/iommu.h b/arch/arm/mach-msm/include/mach/iommu.h
index 62f6699..10811ba 100644
--- a/arch/arm/mach-msm/include/mach/iommu.h
+++ b/arch/arm/mach-msm/include/mach/iommu.h
@@ -45,14 +45,9 @@
 /**
  * struct msm_iommu_dev - a single IOMMU hardware instance
  * name		Human-readable name given to this IOMMU HW instance
- * clk_rate	Rate to set for this IOMMU's clock, if applicable to this
- *		particular IOMMU. 0 means don't set a rate.
- *		-1 means it is an AXI clock with no valid rate
- *
  */
 struct msm_iommu_dev {
 	const char *name;
-	int clk_rate;
 };

 /**
diff --git a/arch/arm/mach-msm/iommu_dev.c b/arch/arm/mach-msm/iommu_dev.c
index b83c73b..7f2b730 100644
--- a/arch/arm/mach-msm/iommu_dev.c
+++ b/arch/arm/mach-msm/iommu_dev.c
@@ -29,6 +29,7 @@

 #include <mach/iommu_hw-8xxx.h>
 #include <mach/iommu.h>
+#include <mach/clk.h>

 struct iommu_ctx_iter_data {
 	/* input */
@@ -130,117 +131,134 @@ static int msm_iommu_probe(struct platform_device *pdev)
 {
 	struct resource *r, *r2;
 	struct clk *iommu_clk;
+	struct clk *iommu_pclk;
 	struct msm_iommu_drvdata *drvdata;
 	struct msm_iommu_dev *iommu_dev = pdev->dev.platform_data;
 	void __iomem *regs_base;
 	resource_size_t	len;
-	int ret = 0, ncb, nm2v, irq;
+	int ret, ncb, nm2v, irq;

-	if (pdev->id != -1) {
-		drvdata = kzalloc(sizeof(*drvdata), GFP_KERNEL);
+	if (pdev->id == -1) {
+		msm_iommu_root_dev = pdev;
+		return 0;
+	}

-		if (!drvdata) {
-			ret = -ENOMEM;
-			goto fail;
-		}
+	drvdata = kzalloc(sizeof(*drvdata), GFP_KERNEL);

-		if (!iommu_dev) {
-			ret = -ENODEV;
-			goto fail;
-		}
+	if (!drvdata) {
+		ret = -ENOMEM;
+		goto fail;
+	}
+
+	if (!iommu_dev) {
+		ret = -ENODEV;
+		goto fail;
+	}

-		if (iommu_dev->clk_rate != 0) {
-			iommu_clk = clk_get(&pdev->dev, "iommu_clk");
-
-			if (IS_ERR(iommu_clk)) {
-				ret = -ENODEV;
-				goto fail;
-			}
-
-			if (iommu_dev->clk_rate > 0) {
-				ret = clk_set_rate(iommu_clk,
-							iommu_dev->clk_rate);
-				if (ret) {
-					clk_put(iommu_clk);
-					goto fail;
-				}
-			}
-
-			ret = clk_enable(iommu_clk);
-			if (ret) {
-				clk_put(iommu_clk);
-				goto fail;
-			}
+	iommu_pclk = clk_get(NULL, "smmu_pclk");
+	if (IS_ERR(iommu_pclk)) {
+		ret = -ENODEV;
+		goto fail;
+	}
+
+	ret = clk_enable(iommu_pclk);
+	if (ret)
+		goto fail_enable;
+
+	iommu_clk = clk_get(&pdev->dev, "iommu_clk");
+
+	if (!IS_ERR(iommu_clk))	{
+		if (clk_get_rate(iommu_clk) == 0)
+			clk_set_min_rate(iommu_clk, 1);
+
+		ret = clk_enable(iommu_clk);
+		if (ret) {
 			clk_put(iommu_clk);
+			goto fail_pclk;
 		}
+	} else
+		iommu_clk = NULL;

-		r = platform_get_resource_byname(pdev, IORESOURCE_MEM,
-						 "physbase");
-		if (!r) {
-			ret = -ENODEV;
-			goto fail;
-		}
+	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "physbase");

-		len = r->end - r->start + 1;
+	if (!r) {
+		ret = -ENODEV;
+		goto fail_clk;
+	}

-		r2 = request_mem_region(r->start, len, r->name);
-		if (!r2) {
-			pr_err("Could not request memory region: "
-			"start=%p, len=%d\n", (void *) r->start, len);
-			ret = -EBUSY;
-			goto fail;
-		}
+	len = r->end - r->start + 1;

-		regs_base = ioremap(r2->start, len);
+	r2 = request_mem_region(r->start, len, r->name);
+	if (!r2) {
+		pr_err("Could not request memory region: start=%p, len=%d\n",
+							(void *) r->start, len);
+		ret = -EBUSY;
+		goto fail_clk;
+	}

-		if (!regs_base) {
-			pr_err("Could not ioremap: start=%p, len=%d\n",
-				 (void *) r2->start, len);
-			ret = -EBUSY;
-			goto fail_mem;
-		}
+	regs_base = ioremap(r2->start, len);

-		irq = platform_get_irq_byname(pdev, "secure_irq");
-		if (irq < 0) {
-			ret = -ENODEV;
-			goto fail_io;
-		}
+	if (!regs_base) {
+		pr_err("Could not ioremap: start=%p, len=%d\n",
+			 (void *) r2->start, len);
+		ret = -EBUSY;
+		goto fail_mem;
+	}
+
+	irq = platform_get_irq_byname(pdev, "secure_irq");
+	if (irq < 0) {
+		ret = -ENODEV;
+		goto fail_io;
+	}

-		mb();
+	mb();

-		if (GET_IDR(regs_base) == 0) {
-			pr_err("Invalid IDR value detected\n");
-			ret = -ENODEV;
-			goto fail_io;
-		}
+	if (GET_IDR(regs_base) == 0) {
+		pr_err("Invalid IDR value detected\n");
+		ret = -ENODEV;
+		goto fail_io;
+	}

-		ret = request_irq(irq, msm_iommu_fault_handler, 0,
-				"msm_iommu_secure_irpt_handler", drvdata);
-		if (ret) {
-			pr_err("Request IRQ %d failed with ret=%d\n", irq, ret);
-			goto fail_io;
-		}
+	ret = request_irq(irq, msm_iommu_fault_handler, 0,
+			"msm_iommu_secure_irpt_handler", drvdata);
+	if (ret) {
+		pr_err("Request IRQ %d failed with ret=%d\n", irq, ret);
+		goto fail_io;
+	}

-		msm_iommu_reset(regs_base);
-		drvdata->base = regs_base;
-		drvdata->irq = irq;
+	msm_iommu_reset(regs_base);
+	drvdata->pclk = iommu_pclk;
+	drvdata->clk = iommu_clk;
+	drvdata->base = regs_base;
+	drvdata->irq = irq;

-		nm2v = GET_NM2VCBMT((unsigned long) regs_base);
-		ncb = GET_NCB((unsigned long) regs_base);
+	nm2v = GET_NM2VCBMT((unsigned long) regs_base);
+	ncb = GET_NCB((unsigned long) regs_base);

-		pr_info("device %s mapped at %p, irq %d with %d ctx banks\n",
+	pr_info("device %s mapped at %p, irq %d with %d ctx banks\n",
 			iommu_dev->name, regs_base, irq, ncb+1);

-		platform_set_drvdata(pdev, drvdata);
-	} else
-		msm_iommu_root_dev = pdev;
+	platform_set_drvdata(pdev, drvdata);

-	return 0;
+	if (iommu_clk)
+		clk_disable(iommu_clk);
+
+	clk_disable(iommu_pclk);

+	return 0;
 fail_io:
 	iounmap(regs_base);
 fail_mem:
 	release_mem_region(r->start, len);
+fail_clk:
+	if (iommu_clk) {
+		clk_disable(iommu_clk);
+		clk_put(iommu_clk);
+	}
+fail_pclk:
+	clk_disable(iommu_pclk);
+fail_enable:
+	clk_put(iommu_pclk);
 fail:
 	kfree(drvdata);
 	return ret;
@@ -252,7 +270,10 @@ static int msm_iommu_remove(struct platform_device *pdev)

 	drv = platform_get_drvdata(pdev);
 	if (drv) {
-		memset(drv, 0, sizeof(struct msm_iommu_drvdata));
+		if (drv->clk)
+			clk_put(drv->clk);
+		clk_put(drv->pclk);
+		memset(drv, 0, sizeof(*drv));
 		kfree(drv);
 		platform_set_drvdata(pdev, NULL);
 	}
@@ -264,7 +285,7 @@ static int msm_iommu_ctx_probe(struct platform_device *pdev)
 	struct msm_iommu_ctx_dev *c = pdev->dev.platform_data;
 	struct msm_iommu_drvdata *drvdata;
 	struct msm_iommu_ctx_drvdata *ctx_drvdata = NULL;
-	int i, ret = 0;
+	int i, ret;
 	if (!c || !pdev->dev.parent) {
 		ret = -EINVAL;
 		goto fail;
@@ -288,6 +309,18 @@ static int msm_iommu_ctx_probe(struct platform_device *pdev)
 	INIT_LIST_HEAD(&ctx_drvdata->attached_elm);
 	platform_set_drvdata(pdev, ctx_drvdata);

+	ret = clk_enable(drvdata->pclk);
+	if (ret)
+		goto fail;
+
+	if (drvdata->clk) {
+		ret = clk_enable(drvdata->clk);
+		if (ret) {
+			clk_disable(drvdata->pclk);
+			goto fail;
+		}
+	}
+
 	/* Program the M2V tables for this context */
 	for (i = 0; i < MAX_NUM_MIDS; i++) {
 		int mid = c->mids[i];
@@ -310,8 +343,11 @@ static int msm_iommu_ctx_probe(struct platform_device *pdev)
 		SET_NSCFG(drvdata->base, mid, 3);
 	}

-	pr_info("context device %s with bank index %d\n", c->name, c->num);
+	if (drvdata->clk)
+		clk_disable(drvdata->clk);
+	clk_disable(drvdata->pclk);

+	dev_info(&pdev->dev, "context %s using bank %d\n", c->name, c->num);
 	return 0;
 fail:
 	kfree(ctx_drvdata);
--
1.7.0.2

Sent by an employee of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 3/3] msm: iommu: Rework clock logic and add IOMMU bus clock control

[Daniel Walker]

On Fri, 2010-11-19 at 19:02 -0800, Stepan Moskovchenko wrote:
> Clean up the clock control code in the probe calls, and add
> support for controlling the clock for the IOMMU bus
> interconnect. With the (proper) clock driver in place, the
> clock control logic in the probe function can be made much
> cleaner since it does not have to deal with the placeholder
> driver anymore.
> 
> Change-Id: I1040bc4e18f4ab4b7cc0dd5fe667f9df83b9f1f5

You need to remove this Change-Id ..

> Signed-off-by: Stepan Moskovchenko <stepanm@codeaurora.org>
> ---
> Please hold off on this until the clock driver is in.
> The patch seems like it is a lot of changes, but a lot of
> it comes from removing one condition, which causes a bunch
> of code to be unindented by one level. This implementation
> is a lot cleaner IMO.
>  arch/arm/mach-msm/devices-msm8x60-iommu.c |    5 -
>  arch/arm/mach-msm/include/mach/iommu.h    |    5 -
>  arch/arm/mach-msm/iommu_dev.c             |  204 +++++++++++++++++------------
>  3 files changed, 120 insertions(+), 94 deletions(-)
> 
> diff --git a/arch/arm/mach-msm/devices-msm8x60-iommu.c b/arch/arm/mach-msm/devices-msm8x60-iommu.c
> index f9e7bd3..e12d7e2 100644
> --- a/arch/arm/mach-msm/devices-msm8x60-iommu.c
> +++ b/arch/arm/mach-msm/devices-msm8x60-iommu.c
> @@ -282,7 +282,6 @@ static struct platform_device msm_root_iommu_dev = {
> 
>  static struct msm_iommu_dev jpegd_iommu = {
>  	.name = "jpegd",
> -	.clk_rate = -1
>  };
> 
>  static struct msm_iommu_dev vpe_iommu = {
> @@ -307,7 +306,6 @@ static struct msm_iommu_dev ijpeg_iommu = {
> 
>  static struct msm_iommu_dev vfe_iommu = {
>  	.name = "vfe",
> -	.clk_rate = -1
>  };
> 
>  static struct msm_iommu_dev vcodec_a_iommu = {
> @@ -320,17 +318,14 @@ static struct msm_iommu_dev vcodec_b_iommu = {
> 
>  static struct msm_iommu_dev gfx3d_iommu = {
>  	.name = "gfx3d",
> -	.clk_rate = 27000000
>  };
> 
>  static struct msm_iommu_dev gfx2d0_iommu = {
>  	.name = "gfx2d0",
> -	.clk_rate = 27000000
>  };
> 
>  static struct msm_iommu_dev gfx2d1_iommu = {
>  	.name = "gfx2d1",
> -	.clk_rate = 27000000
>  };
> 
>  static struct platform_device msm_device_iommu_jpegd = {
> diff --git a/arch/arm/mach-msm/include/mach/iommu.h b/arch/arm/mach-msm/include/mach/iommu.h
> index 62f6699..10811ba 100644
> --- a/arch/arm/mach-msm/include/mach/iommu.h
> +++ b/arch/arm/mach-msm/include/mach/iommu.h
> @@ -45,14 +45,9 @@
>  /**
>   * struct msm_iommu_dev - a single IOMMU hardware instance
>   * name		Human-readable name given to this IOMMU HW instance
> - * clk_rate	Rate to set for this IOMMU's clock, if applicable to this
> - *		particular IOMMU. 0 means don't set a rate.
> - *		-1 means it is an AXI clock with no valid rate
> - *
>   */
>  struct msm_iommu_dev {
>  	const char *name;
> -	int clk_rate;
>  };
> 
>  /**
> diff --git a/arch/arm/mach-msm/iommu_dev.c b/arch/arm/mach-msm/iommu_dev.c
> index b83c73b..7f2b730 100644
> --- a/arch/arm/mach-msm/iommu_dev.c
> +++ b/arch/arm/mach-msm/iommu_dev.c
> @@ -29,6 +29,7 @@
> 
>  #include <mach/iommu_hw-8xxx.h>
>  #include <mach/iommu.h>
> +#include <mach/clk.h>
> 
>  struct iommu_ctx_iter_data {
>  	/* input */
> @@ -130,117 +131,134 @@ static int msm_iommu_probe(struct platform_device *pdev)
>  {
>  	struct resource *r, *r2;
>  	struct clk *iommu_clk;
> +	struct clk *iommu_pclk;
>  	struct msm_iommu_drvdata *drvdata;
>  	struct msm_iommu_dev *iommu_dev = pdev->dev.platform_data;
>  	void __iomem *regs_base;
>  	resource_size_t	len;
> -	int ret = 0, ncb, nm2v, irq;
> +	int ret, ncb, nm2v, irq;
> 
> -	if (pdev->id != -1) {
> -		drvdata = kzalloc(sizeof(*drvdata), GFP_KERNEL);
> +	if (pdev->id == -1) {
> +		msm_iommu_root_dev = pdev;
> +		return 0;
> +	}
> 
> -		if (!drvdata) {
> -			ret = -ENOMEM;
> -			goto fail;
> -		}
> +	drvdata = kzalloc(sizeof(*drvdata), GFP_KERNEL);
> 
> -		if (!iommu_dev) {
> -			ret = -ENODEV;
> -			goto fail;
> -		}
> +	if (!drvdata) {
> +		ret = -ENOMEM;
> +		goto fail;
> +	}
> +
> +	if (!iommu_dev) {
> +		ret = -ENODEV;
> +		goto fail;
> +	}
> 
> -		if (iommu_dev->clk_rate != 0) {
> -			iommu_clk = clk_get(&pdev->dev, "iommu_clk");
> -
> -			if (IS_ERR(iommu_clk)) {
> -				ret = -ENODEV;
> -				goto fail;
> -			}
> -
> -			if (iommu_dev->clk_rate > 0) {
> -				ret = clk_set_rate(iommu_clk,
> -							iommu_dev->clk_rate);
> -				if (ret) {
> -					clk_put(iommu_clk);
> -					goto fail;
> -				}
> -			}
> -
> -			ret = clk_enable(iommu_clk);
> -			if (ret) {
> -				clk_put(iommu_clk);
> -				goto fail;
> -			}
> +	iommu_pclk = clk_get(NULL, "smmu_pclk");
> +	if (IS_ERR(iommu_pclk)) {
> +		ret = -ENODEV;
> +		goto fail;
> +	}
> +
> +	ret = clk_enable(iommu_pclk);
> +	if (ret)
> +		goto fail_enable;
> +
> +	iommu_clk = clk_get(&pdev->dev, "iommu_clk");
> +
> +	if (!IS_ERR(iommu_clk))	{
> +		if (clk_get_rate(iommu_clk) == 0)
> +			clk_set_min_rate(iommu_clk, 1);
> +
> +		ret = clk_enable(iommu_clk);
> +		if (ret) {
>  			clk_put(iommu_clk);
> +			goto fail_pclk;
>  		}
> +	} else
> +		iommu_clk = NULL;
> 
> -		r = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> -						 "physbase");
> -		if (!r) {
> -			ret = -ENODEV;
> -			goto fail;
> -		}
> +	r = platform_get_resource_byname(pdev, IORESOURCE_MEM, "physbase");
> 
> -		len = r->end - r->start + 1;
> +	if (!r) {
> +		ret = -ENODEV;
> +		goto fail_clk;
> +	}
> 
> -		r2 = request_mem_region(r->start, len, r->name);
> -		if (!r2) {
> -			pr_err("Could not request memory region: "
> -			"start=%p, len=%d\n", (void *) r->start, len);
> -			ret = -EBUSY;
> -			goto fail;
> -		}
> +	len = r->end - r->start + 1;
> 
> -		regs_base = ioremap(r2->start, len);
> +	r2 = request_mem_region(r->start, len, r->name);
> +	if (!r2) {
> +		pr_err("Could not request memory region: start=%p, len=%d\n",
> +							(void *) r->start, len);(void *) r->start, len);

You usually just tab over till you get to the " like this,

pr_err("Could not request memory region: start=%p, len=%d\n",
	(void *) r->start, len);

> +		ret = -EBUSY;
> +		goto fail_clk;
> +	}
> 
> -		if (!regs_base) {
> -			pr_err("Could not ioremap: start=%p, len=%d\n",
> -				 (void *) r2->start, len);
> -			ret = -EBUSY;
> -			goto fail_mem;
> -		}
> +	regs_base = ioremap(r2->start, len);
> 
> -		irq = platform_get_irq_byname(pdev, "secure_irq");
> -		if (irq < 0) {
> -			ret = -ENODEV;
> -			goto fail_io;
> -		}
> +	if (!regs_base) {
> +		pr_err("Could not ioremap: start=%p, len=%d\n",
> +			 (void *) r2->start, len);
> +		ret = -EBUSY;
> +		goto fail_mem;
> +	}
> +
> +	irq = platform_get_irq_byname(pdev, "secure_irq");
> +	if (irq < 0) {
> +		ret = -ENODEV;
> +		goto fail_io;
> +	}
> 
> -		mb();
> +	mb();
> 
> -		if (GET_IDR(regs_base) == 0) {
> -			pr_err("Invalid IDR value detected\n");
> -			ret = -ENODEV;
> -			goto fail_io;
> -		}
> +	if (GET_IDR(regs_base) == 0) {
> +		pr_err("Invalid IDR value detected\n");
> +		ret = -ENODEV;
> +		goto fail_io;
> +	}
> 
> -		ret = request_irq(irq, msm_iommu_fault_handler, 0,
> -				"msm_iommu_secure_irpt_handler", drvdata);
> -		if (ret) {
> -			pr_err("Request IRQ %d failed with ret=%d\n", irq, ret);
> -			goto fail_io;
> -		}
> +	ret = request_irq(irq, msm_iommu_fault_handler, 0,
> +			"msm_iommu_secure_irpt_handler", drvdata);
> +	if (ret) {
> +		pr_err("Request IRQ %d failed with ret=%d\n", irq, ret);
> +		goto fail_io;
> +	}
> 
> -		msm_iommu_reset(regs_base);
> -		drvdata->base = regs_base;
> -		drvdata->irq = irq;
> +	msm_iommu_reset(regs_base);
> +	drvdata->pclk = iommu_pclk;
> +	drvdata->clk = iommu_clk;
> +	drvdata->base = regs_base;
> +	drvdata->irq = irq;
> 
> -		nm2v = GET_NM2VCBMT((unsigned long) regs_base);
> -		ncb = GET_NCB((unsigned long) regs_base);
> +	nm2v = GET_NM2VCBMT((unsigned long) regs_base);
> +	ncb = GET_NCB((unsigned long) regs_base);
> 
> -		pr_info("device %s mapped at %p, irq %d with %d ctx banks\n",
> +	pr_info("device %s mapped at %p, irq %d with %d ctx banks\n",
>  			iommu_dev->name, regs_base, irq, ncb+1);
> 
> -		platform_set_drvdata(pdev, drvdata);
> -	} else
> -		msm_iommu_root_dev = pdev;
> +	platform_set_drvdata(pdev, drvdata);
> 
> -	return 0;
> +	if (iommu_clk)
> +		clk_disable(iommu_clk);
> +
> +	clk_disable(iommu_pclk);
> 
> +	return 0;
>  fail_io:
>  	iounmap(regs_base);
>  fail_mem:
>  	release_mem_region(r->start, len);
> +fail_clk:
> +	if (iommu_clk) {
> +		clk_disable(iommu_clk);
> +		clk_put(iommu_clk);
> +	}
> +fail_pclk:
> +	clk_disable(iommu_pclk);
> +fail_enable:
> +	clk_put(iommu_pclk);
>  fail:
>  	kfree(drvdata);
>  	return ret;
> @@ -252,7 +270,10 @@ static int msm_iommu_remove(struct platform_device *pdev)
> 
>  	drv = platform_get_drvdata(pdev);
>  	if (drv) {
> -		memset(drv, 0, sizeof(struct msm_iommu_drvdata));
> +		if (drv->clk)
> +			clk_put(drv->clk);
> +		clk_put(drv->pclk);
> +		memset(drv, 0, sizeof(*drv));

Do you really need the memset ?

>  		kfree(drv);
>  		platform_set_drvdata(pdev, NULL);
>  	}
> @@ -264,7 +285,7 @@ static int msm_iommu_ctx_probe(struct platform_device *pdev)
>  	struct msm_iommu_ctx_dev *c = pdev->dev.platform_data;
>  	struct msm_iommu_drvdata *drvdata;
>  	struct msm_iommu_ctx_drvdata *ctx_drvdata = NULL;
> -	int i, ret = 0;
> +	int i, ret;
>  	if (!c || !pdev->dev.parent) {
>  		ret = -EINVAL;
>  		goto fail;
> @@ -288,6 +309,18 @@ static int msm_iommu_ctx_probe(struct platform_device *pdev)
>  	INIT_LIST_HEAD(&ctx_drvdata->attached_elm);
>  	platform_set_drvdata(pdev, ctx_drvdata);
> 
> +	ret = clk_enable(drvdata->pclk);
> +	if (ret)
> +		goto fail;
> +
> +	if (drvdata->clk) {
> +		ret = clk_enable(drvdata->clk);
> +		if (ret) {
> +			clk_disable(drvdata->pclk);
> +			goto fail;
> +		}
> +	}

You did this in a prior patch also, you could combine them into a single
helper function. Maybe do the same for the disable side too.

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.

[PATCH 3/3] msm: iommu: Rework clock logic and add IOMMU bus clock control

[Stepan Moskovchenko]

On 11/22/2010 3:51 PM, Daniel Walker wrote:
> On Fri, 2010-11-19 at 19:02 -0800, Stepan Moskovchenko wrote:
>> Clean up the clock control code in the probe calls, and add
>> support for controlling the clock for the IOMMU bus
>> interconnect. With the (proper) clock driver in place, the
>> clock control logic in the probe function can be made much
>> cleaner since it does not have to deal with the placeholder
>> driver anymore.
>>
>> Change-Id: I1040bc4e18f4ab4b7cc0dd5fe667f9df83b9f1f5
> You need to remove this Change-Id ..
Fixed in v2.

>
>> +		pr_err("Could not request memory region: start=%p, len=%d\n",
>> +							(void *) r->start, len);(void *) r->start, len);
> You usually just tab over till you get to the " like this,
>
> pr_err("Could not request memory region: start=%p, len=%d\n",
> 	(void *) r->start, len);
Fixed in v2.

>>   	drv = platform_get_drvdata(pdev);
>>   	if (drv) {
>> -		memset(drv, 0, sizeof(struct msm_iommu_drvdata));
>> +		if (drv->clk)
>> +			clk_put(drv->clk);
>> +		clk_put(drv->pclk);
>> +		memset(drv, 0, sizeof(*drv));
> Do you really need the memset ?
I guess not.. it seemed like good practice to poison the memory in case 
someone else had a stale reference to it. I guess I can remove them.

>> +	if (ret)
>> +		goto fail;
>> +
>> +	if (drvdata->clk) {
>> +		ret = clk_enable(drvdata->clk);
>> +		if (ret) {
>> +			clk_disable(drvdata->pclk);
>> +			goto fail;
>> +		}
>> +	}
> You did this in a prior patch also, you could combine them into a single
> helper function. Maybe do the same for the disable side too.
That was in a different file (where it gets used much more extensively 
than just in the one case here). I would rather not make a bunch of my 
internal stuff visible to the global namespace if I can help it, and 
it's a trivial enough operation to enable two clocks.

I will send out v2 in a few minutes.

Steve

---

  Sent by an employee of the Qualcomm Innovation Center, Inc.
  The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum.

[PATCH 3/3] msm: iommu: Rework clock logic and add IOMMU bus clock control

[Daniel Walker]

On Mon, 2010-11-22 at 19:06 -0800, Stepan Moskovchenko wrote:
> On 11/22/2010 3:51 PM, Daniel Walker wrote:
> > On Fri, 2010-11-19 at 19:02 -0800, Stepan Moskovchenko wrote:
> >> Clean up the clock control code in the probe calls, and add
> >> support for controlling the clock for the IOMMU bus
> >> interconnect. With the (proper) clock driver in place, the
> >> clock control logic in the probe function can be made much
> >> cleaner since it does not have to deal with the placeholder
> >> driver anymore.
> >>
> >> Change-Id: I1040bc4e18f4ab4b7cc0dd5fe667f9df83b9f1f5
> > You need to remove this Change-Id ..
> Fixed in v2.
> 
> >
> >> +		pr_err("Could not request memory region: start=%p, len=%d\n",
> >> +							(void *) r->start, len);(void *) r->start, len);
> > You usually just tab over till you get to the " like this,
> >
> > pr_err("Could not request memory region: start=%p, len=%d\n",
> > 	(void *) r->start, len);
> Fixed in v2.
> 
> >>   	drv = platform_get_drvdata(pdev);
> >>   	if (drv) {
> >> -		memset(drv, 0, sizeof(struct msm_iommu_drvdata));
> >> +		if (drv->clk)
> >> +			clk_put(drv->clk);
> >> +		clk_put(drv->pclk);
> >> +		memset(drv, 0, sizeof(*drv));
> > Do you really need the memset ?
> I guess not.. it seemed like good practice to poison the memory in case 
> someone else had a stale reference to it. I guess I can remove them.

The memory allocator already has a poison debugging option. If you
suspect memory issues you can always turn that on.

> >> +	if (ret)
> >> +		goto fail;
> >> +
> >> +	if (drvdata->clk) {
> >> +		ret = clk_enable(drvdata->clk);
> >> +		if (ret) {
> >> +			clk_disable(drvdata->pclk);
> >> +			goto fail;
> >> +		}
> >> +	}
> > You did this in a prior patch also, you could combine them into a single
> > helper function. Maybe do the same for the disable side too.
> That was in a different file (where it gets used much more extensively 
> than just in the one case here). I would rather not make a bunch of my 
> internal stuff visible to the global namespace if I can help it, and 
> it's a trivial enough operation to enable two clocks.

I was meaning making it a static inline inside a header.. You wouldn't
want this to be a global function. If you can it's usually a good idea
to combine code that's similar, that way there's only one place to make
modifications.

Daniel

-- 
Sent by an consultant of the Qualcomm Innovation Center, Inc.
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora
Forum.