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

[Daniel Walker <dwalker@codeaurora.org>]

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.