Messages in this thread | | | Date | Fri, 14 Mar 2014 14:07:32 +0100 | From | Tomasz Figa <> | Subject | Re: [PATCH v11 08/27] iommu/exynos: always use a single clock descriptor |
| |
Hi KyongHo,
On 14.03.2014 06:05, Cho KyongHo wrote: > System MMU driver is changed to control only a single instance of > System MMU at a time. Since a single instance of System MMU has only > a single clock descriptor for its clock gating, there is no need to > obtain two or more clock descriptors. >
This patch does much more than just making the driver use a single clock descriptor. Please update the subject and description accordingly.
> Signed-off-by: Cho KyongHo <pullip.cho@samsung.com> > --- > drivers/iommu/exynos-iommu.c | 223 ++++++++++++++---------------------------- > 1 file changed, 72 insertions(+), 151 deletions(-) > > diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c > index 8dc7031..a4499b2 100644 > --- a/drivers/iommu/exynos-iommu.c > +++ b/drivers/iommu/exynos-iommu.c > @@ -171,9 +171,8 @@ struct sysmmu_drvdata { > struct device *sysmmu; /* System MMU's device descriptor */ > struct device *dev; /* Owner of system MMU */ > char *dbgname; > - int nsfrs; > - void __iomem **sfrbases; > - struct clk *clk[2]; > + void __iomem *sfrbase; > + struct clk *clk; > int activations; > rwlock_t lock; > struct iommu_domain *domain; > @@ -294,56 +293,39 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id) > { > /* SYSMMU is in blocked when interrupt occurred. */ > struct sysmmu_drvdata *data = dev_id; > - struct resource *irqres; > - struct platform_device *pdev; > enum exynos_sysmmu_inttype itype; > unsigned long addr = -1; > - > - int i, ret = -ENOSYS; > + int ret = -ENOSYS; > > read_lock(&data->lock); > > WARN_ON(!is_sysmmu_active(data)); > > - pdev = to_platform_device(data->sysmmu); > - for (i = 0; i < (pdev->num_resources / 2); i++) { > - irqres = platform_get_resource(pdev, IORESOURCE_IRQ, i); > - if (irqres && ((int)irqres->start == irq)) > - break; > - } > - > - if (i == pdev->num_resources) { > + itype = (enum exynos_sysmmu_inttype) > + __ffs(__raw_readl(data->sfrbase + REG_INT_STATUS)); > + if (WARN_ON(!((itype >= 0) && (itype < SYSMMU_FAULT_UNKNOWN)))) > itype = SYSMMU_FAULT_UNKNOWN; > - } else { > - itype = (enum exynos_sysmmu_inttype) > - __ffs(__raw_readl(data->sfrbases[i] + REG_INT_STATUS)); > - if (WARN_ON(!((itype >= 0) && (itype < SYSMMU_FAULT_UNKNOWN)))) > - itype = SYSMMU_FAULT_UNKNOWN; > - else > - addr = __raw_readl( > - data->sfrbases[i] + fault_reg_offset[itype]); > - } > + else > + addr = __raw_readl(data->sfrbase + fault_reg_offset[itype]); > > if (data->domain) > - ret = report_iommu_fault(data->domain, data->dev, > - addr, itype); > + ret = report_iommu_fault(data->domain, data->dev, addr, itype); > > if ((ret == -ENOSYS) && data->fault_handler) { > unsigned long base = data->pgtable; > if (itype != SYSMMU_FAULT_UNKNOWN) > - base = __raw_readl( > - data->sfrbases[i] + REG_PT_BASE_ADDR); > + base = __raw_readl(data->sfrbase + REG_PT_BASE_ADDR); > ret = data->fault_handler(itype, base, addr); > } > > if (!ret && (itype != SYSMMU_FAULT_UNKNOWN)) > - __raw_writel(1 << itype, data->sfrbases[i] + REG_INT_CLEAR); > + __raw_writel(1 << itype, data->sfrbase + REG_INT_CLEAR); > else > dev_dbg(data->sysmmu, "(%s) %s is not handled.\n", > data->dbgname, sysmmu_fault_name[itype]); > > if (itype != SYSMMU_FAULT_UNKNOWN) > - sysmmu_unblock(data->sfrbases[i]); > + sysmmu_unblock(data->sfrbase); > > read_unlock(&data->lock); > > @@ -354,20 +336,16 @@ static bool __exynos_sysmmu_disable(struct sysmmu_drvdata *data) > { > unsigned long flags; > bool disabled = false; > - int i; > > write_lock_irqsave(&data->lock, flags); > > if (!set_sysmmu_inactive(data)) > goto finish; > > - for (i = 0; i < data->nsfrs; i++) > - __raw_writel(CTRL_DISABLE, data->sfrbases[i] + REG_MMU_CTRL); > + __raw_writel(CTRL_DISABLE, data->sfrbase + REG_MMU_CTRL); > > - if (data->clk[1]) > - clk_disable(data->clk[1]); > - if (data->clk[0]) > - clk_disable(data->clk[0]); > + if (data->clk)
I know this is already in the driver, but checking (struct clk *) for NULL is incorrect. NULL is a valid pointer for dummy clocks on platforms which do not provide particular clocks, to make this transparent to drivers. IS_ERR() should be used to check whether a clock pointer is valid.
This patch is changing all the clock code anyway, so this change could be squashed into it to fix this.
> + clk_disable(data->clk); > > disabled = true; > data->pgtable = 0; > @@ -393,7 +371,7 @@ finish: > static int __exynos_sysmmu_enable(struct sysmmu_drvdata *data, > unsigned long pgtable, struct iommu_domain *domain) > { > - int i, ret = 0; > + int ret = 0; > unsigned long flags; > > write_lock_irqsave(&data->lock, flags); > @@ -410,17 +388,14 @@ static int __exynos_sysmmu_enable(struct sysmmu_drvdata *data, > goto finish; > } > > - if (data->clk[0]) > - clk_enable(data->clk[0]); > - if (data->clk[1]) > - clk_enable(data->clk[1]); > + if (data->clk) > + clk_enable(data->clk); > > data->pgtable = pgtable; > > - for (i = 0; i < data->nsfrs; i++) { > - __sysmmu_set_ptbase(data->sfrbases[i], pgtable); > - __raw_writel(CTRL_ENABLE, data->sfrbases[i] + REG_MMU_CTRL); > - } > + __sysmmu_set_ptbase(data->sfrbase, pgtable); > + > + __raw_writel(CTRL_ENABLE, data->sfrbase + REG_MMU_CTRL); > > data->domain = domain; > > @@ -477,28 +452,26 @@ static void sysmmu_tlb_invalidate_entry(struct device *dev, unsigned long iova, > read_lock_irqsave(&data->lock, flags); > > if (is_sysmmu_active(data)) { > - int i; > - for (i = 0; i < data->nsfrs; i++) { > - unsigned int maj; > - unsigned int num_inv = 1; > - maj = __raw_readl(data->sfrbases[i] + REG_MMU_VERSION); > - /* > - * L2TLB invalidation required > - * 4KB page: 1 invalidation > - * 64KB page: 16 invalidation > - * 1MB page: 64 invalidation > - * because it is set-associative TLB > - * with 8-way and 64 sets. > - * 1MB page can be cached in one of all sets. > - * 64KB page can be one of 16 consecutive sets. > - */ > - if ((maj >> 28) == 2) /* major version number */ > - num_inv = min_t(unsigned int, size / PAGE_SIZE, 64); > - if (sysmmu_block(data->sfrbases[i])) { > - __sysmmu_tlb_invalidate_entry( > - data->sfrbases[i], iova, num_inv); > - sysmmu_unblock(data->sfrbases[i]); > - } > + unsigned int maj; > + unsigned int num_inv = 1; > + maj = __raw_readl(data->sfrbase + REG_MMU_VERSION); > + /* > + * L2TLB invalidation required > + * 4KB page: 1 invalidation > + * 64KB page: 16 invalidation > + * 1MB page: 64 invalidation > + * because it is set-associative TLB > + * with 8-way and 64 sets. > + * 1MB page can be cached in one of all sets. > + * 64KB page can be one of 16 consecutive sets. > + */ > + if ((maj >> 28) == 2) /* major version number */ > + num_inv = min_t(unsigned int, size / PAGE_SIZE, 64); > + > + if (sysmmu_block(data->sfrbase)) { > + __sysmmu_tlb_invalidate_entry(data->sfrbase, iova, > + num_inv); > + sysmmu_unblock(data->sfrbase); > } > } else { > dev_dbg(data->sysmmu, > @@ -517,12 +490,9 @@ void exynos_sysmmu_tlb_invalidate(struct device *dev) > read_lock_irqsave(&data->lock, flags); > > if (is_sysmmu_active(data)) { > - int i; > - for (i = 0; i < data->nsfrs; i++) { > - if (sysmmu_block(data->sfrbases[i])) { > - __sysmmu_tlb_invalidate(data->sfrbases[i]); > - sysmmu_unblock(data->sfrbases[i]); > - } > + if (sysmmu_block(data->sfrbase)) { > + __sysmmu_tlb_invalidate(data->sfrbase); > + sysmmu_unblock(data->sfrbase); > } > } else { > dev_dbg(data->sysmmu, > @@ -535,11 +505,10 @@ void exynos_sysmmu_tlb_invalidate(struct device *dev) > > static int exynos_sysmmu_probe(struct platform_device *pdev) > { > - int i, ret; > - struct device *dev; > + int ret; > + struct device *dev = &pdev->dev; > struct sysmmu_drvdata *data; > - > - dev = &pdev->dev; > + struct resource *res; > > data = kzalloc(sizeof(*data), GFP_KERNEL); > if (!data) { > @@ -548,82 +517,39 @@ static int exynos_sysmmu_probe(struct platform_device *pdev) > goto err_alloc; > } > > - ret = dev_set_drvdata(dev, data); > - if (ret) { > - dev_dbg(dev, "Unabled to initialize driver data\n"); > + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > + if (!res) { > + dev_dbg(dev, "Unable to find IOMEM region\n"); > + ret = -ENOENT; > goto err_init; > } > > - data->nsfrs = pdev->num_resources / 2; > - data->sfrbases = kmalloc(sizeof(*data->sfrbases) * data->nsfrs, > - GFP_KERNEL); > - if (data->sfrbases == NULL) { > - dev_dbg(dev, "Not enough memory\n"); > - ret = -ENOMEM; > - goto err_init; > + data->sfrbase = ioremap(res->start, resource_size(res)); > + if (!data->sfrbase) { > + dev_dbg(dev, "Unable to map IOMEM @ PA:%#x\n", res->start); > + ret = -ENOENT; > + goto err_res; > } > > - for (i = 0; i < data->nsfrs; i++) { > - struct resource *res; > - res = platform_get_resource(pdev, IORESOURCE_MEM, i); > - if (!res) { > - dev_dbg(dev, "Unable to find IOMEM region\n"); > - ret = -ENOENT; > - goto err_res; > - } > - > - data->sfrbases[i] = ioremap(res->start, resource_size(res)); > - if (!data->sfrbases[i]) { > - dev_dbg(dev, "Unable to map IOMEM @ PA:%#x\n", > - res->start); > - ret = -ENOENT; > - goto err_res; > - } > + ret = platform_get_irq(pdev, 0); > + if (ret <= 0) { > + dev_dbg(dev, "Unable to find IRQ resource\n"); > + goto err_irq; > } > > - for (i = 0; i < data->nsfrs; i++) { > - ret = platform_get_irq(pdev, i); > - if (ret <= 0) { > - dev_dbg(dev, "Unable to find IRQ resource\n"); > - goto err_irq; > - } > - > - ret = request_irq(ret, exynos_sysmmu_irq, 0, > - dev_name(dev), data); > - if (ret) { > - dev_dbg(dev, "Unabled to register interrupt handler\n"); > - goto err_irq; > - } > + ret = request_irq(ret, exynos_sysmmu_irq, 0, > + dev_name(dev), data); > + if (ret) { > + dev_dbg(dev, "Unabled to register interrupt handler\n"); > + goto err_irq; > } > > if (dev_get_platdata(dev)) { > - char *deli, *beg; > - struct sysmmu_platform_data *platdata = dev_get_platdata(dev); > - > - beg = platdata->clockname; > - > - for (deli = beg; (*deli != '\0') && (*deli != ','); deli++) > - /* NOTHING */; > - > - if (*deli == '\0') > - deli = NULL; > - else > - *deli = '\0'; > - > - data->clk[0] = clk_get(dev, beg); > - if (IS_ERR(data->clk[0])) { > - data->clk[0] = NULL; > + data->clk = clk_get(dev, "sysmmu"); > + if (IS_ERR(data->clk)) { > + data->clk = NULL;
This is incorrect. IS_ERR() should be used through the driver and the error value here should not be replaced with NULL.
-- Best regards, Tomasz
| |