Re: [PATCH 2/3] msm: iommu: Clock control for the IOMMU driver
From: Daniel Walker
Date: Mon Nov 22 2010 - 18:32:49 EST
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@xxxxxxxxxxxxxx>
> ---
> 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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/