This commit adds device tree support for System MMU.
This also include the following changes and enhancements:
* use managed device helper functions.
Simplyfies System MMU device driver.
* use only a single clock descriptor.
System MMU device descriptor is seperate if it is imposible to make
a single clock descriptor to make a device descriptor for a group of
System MMUs.
* removed dbgname member from sysmmu_drvdata structure.
debugging kernel message for a System MMU is distinguisheable with the
name of device descroptors.
Signed-off-by: Cho KyongHo <[email protected]>
---
drivers/iommu/Kconfig | 5 +-
drivers/iommu/exynos-iommu.c | 186 ++++++++++++++++-------------------------
2 files changed, 75 insertions(+), 116 deletions(-)
diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
index 820d85c..9ad45f1 100644
--- a/drivers/iommu/Kconfig
+++ b/drivers/iommu/Kconfig
@@ -168,16 +168,15 @@ config TEGRA_IOMMU_SMMU
config EXYNOS_IOMMU
bool "Exynos IOMMU Support"
- depends on ARCH_EXYNOS && EXYNOS_DEV_SYSMMU
+ depends on ARCH_EXYNOS
select IOMMU_API
+ default n
help
Support for the IOMMU(System MMU) of Samsung Exynos application
processor family. This enables H/W multimedia accellerators to see
non-linear physical memory chunks as a linear memory in their
address spaces
- If unsure, say N here.
-
config EXYNOS_IOMMU_DEBUG
bool "Debugging log for Exynos IOMMU"
depends on EXYNOS_IOMMU
diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
index a318049..0ee73e8 100644
--- a/drivers/iommu/exynos-iommu.c
+++ b/drivers/iommu/exynos-iommu.c
@@ -26,6 +26,7 @@
#include <linux/list.h>
#include <linux/memblock.h>
#include <linux/export.h>
+#include <linux/of.h>
#include <asm/cacheflush.h>
#include <asm/pgtable.h>
@@ -170,15 +171,14 @@ struct sysmmu_drvdata {
struct list_head node; /* entry of exynos_iommu_domain.clients */
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];
+ struct clk *clk;
int activations;
rwlock_t lock;
struct iommu_domain *domain;
sysmmu_fault_handler_t fault_handler;
unsigned long pgtable;
+ void __iomem *sfrbases[0];
};
static bool set_sysmmu_active(struct sysmmu_drvdata *data)
@@ -385,8 +385,8 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void *dev_id)
if (!ret && (itype != SYSMMU_FAULT_UNKNOWN))
__raw_writel(1 << itype, data->sfrbases[i] + REG_INT_CLEAR);
else
- dev_dbg(data->sysmmu, "(%s) %s is not handled.\n",
- data->dbgname, sysmmu_fault_name[itype]);
+ dev_dbg(data->sysmmu, "%s is not handled.\n",
+ sysmmu_fault_name[itype]);
if (itype != SYSMMU_FAULT_UNKNOWN)
sysmmu_unblock(data->sfrbases[i]);
@@ -410,10 +410,8 @@ static bool __exynos_sysmmu_disable(struct sysmmu_drvdata *data)
for (i = 0; i < data->nsfrs; i++)
__raw_writel(CTRL_DISABLE, data->sfrbases[i] + REG_MMU_CTRL);
- if (data->clk[1])
- clk_disable(data->clk[1]);
- if (data->clk[0])
- clk_disable(data->clk[0]);
+ if (data->clk)
+ clk_disable(data->clk);
disabled = true;
data->pgtable = 0;
@@ -422,10 +420,10 @@ finish:
write_unlock_irqrestore(&data->lock, flags);
if (disabled)
- dev_dbg(data->sysmmu, "(%s) Disabled\n", data->dbgname);
+ dev_dbg(data->sysmmu, "Disabled\n");
else
- dev_dbg(data->sysmmu, "(%s) %d times left to be disabled\n",
- data->dbgname, data->activations);
+ dev_dbg(data->sysmmu, "%d times left to be disabled\n",
+ data->activations);
return disabled;
}
@@ -452,14 +450,12 @@ static int __exynos_sysmmu_enable(struct sysmmu_drvdata *data,
ret = 1;
}
- dev_dbg(data->sysmmu, "(%s) Already enabled\n", data->dbgname);
+ dev_dbg(data->sysmmu, "Already enabled\n");
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;
@@ -479,7 +475,7 @@ static int __exynos_sysmmu_enable(struct sysmmu_drvdata *data,
data->domain = domain;
- dev_dbg(data->sysmmu, "(%s) Enabled\n", data->dbgname);
+ dev_dbg(data->sysmmu, "Enabled\n");
finish:
write_unlock_irqrestore(&data->lock, flags);
@@ -495,7 +491,7 @@ int exynos_sysmmu_enable(struct device *dev, unsigned long pgtable)
ret = pm_runtime_get_sync(data->sysmmu);
if (ret < 0) {
- dev_dbg(data->sysmmu, "(%s) Failed to enable\n", data->dbgname);
+ dev_dbg(data->sysmmu, "Failed to enable\n");
return ret;
}
@@ -503,8 +499,8 @@ int exynos_sysmmu_enable(struct device *dev, unsigned long pgtable)
if (WARN_ON(ret < 0)) {
pm_runtime_put(data->sysmmu);
dev_err(data->sysmmu,
- "(%s) Already enabled with page table %#lx\n",
- data->dbgname, data->pgtable);
+ "Already enabled with page table %#lx\n",
+ data->pgtable);
} else {
data->dev = dev;
}
@@ -540,9 +536,7 @@ static void sysmmu_tlb_invalidate_entry(struct device *dev, unsigned long iova)
}
}
} else {
- dev_dbg(data->sysmmu,
- "(%s) Disabled. Skipping invalidating TLB.\n",
- data->dbgname);
+ dev_dbg(data->sysmmu, "Disabled. Skipping invalidating TLB.\n");
}
read_unlock_irqrestore(&data->lock, flags);
@@ -564,141 +558,107 @@ void exynos_sysmmu_tlb_invalidate(struct device *dev)
}
}
} else {
- dev_dbg(data->sysmmu,
- "(%s) Disabled. Skipping invalidating TLB.\n",
- data->dbgname);
+ dev_dbg(data->sysmmu, "Disabled. Skipping invalidating TLB.\n");
}
read_unlock_irqrestore(&data->lock, flags);
}
-static int exynos_sysmmu_probe(struct platform_device *pdev)
+static int __init exynos_sysmmu_probe(struct platform_device *pdev)
{
int i, ret;
- struct device *dev;
+ struct device *dev = &pdev->dev;
struct sysmmu_drvdata *data;
- dev = &pdev->dev;
-
- data = kzalloc(sizeof(*data), GFP_KERNEL);
- if (!data) {
- dev_dbg(dev, "Not enough memory\n");
- ret = -ENOMEM;
- goto err_alloc;
+ if (pdev->num_resources == 0) {
+ dev_err(dev, "No System MMU resource defined\n");
+ return -ENODEV;
}
- ret = dev_set_drvdata(dev, data);
- if (ret) {
- dev_dbg(dev, "Unabled to initialize driver data\n");
- goto err_init;
+ data = devm_kzalloc(dev,
+ sizeof(*data) +
+ sizeof(*data->sfrbases) * (pdev->num_resources / 2),
+ GFP_KERNEL);
+ if (!data) {
+ dev_err(dev, "Not enough memory for initialization\n");
+ return -ENOMEM;
}
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;
- }
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;
+ dev_err(dev, "Unable to find IOMEM region\n");
+ return -ENOENT;
}
- data->sfrbases[i] = ioremap(res->start, resource_size(res));
+ data->sfrbases[i] = devm_request_and_ioremap(dev, res);
if (!data->sfrbases[i]) {
- dev_dbg(dev, "Unable to map IOMEM @ PA:%#x\n",
- res->start);
- ret = -ENOENT;
- goto err_res;
+ dev_err(dev, "Unable to map IOMEM @ %#x\n", res->start);
+ return -EBUSY;
}
}
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;
+ int irq;
+
+ irq = platform_get_irq(pdev, i);
+ if (irq <= 0) {
+ dev_err(dev, "Unable to find IRQ resource\n");
+ return -ENOENT;
}
- ret = request_irq(ret, exynos_sysmmu_irq, 0,
- dev_name(dev), data);
+ ret = devm_request_irq(dev, irq, exynos_sysmmu_irq,
+ 0, dev_name(dev), data);
if (ret) {
- dev_dbg(dev, "Unabled to register interrupt handler\n");
- goto err_irq;
+ dev_err(dev, "Unable to register handler to irq %d\n",
+ irq);
+ return ret;
}
}
- 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 */;
+ pm_runtime_enable(dev);
- 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;
- dev_dbg(dev, "No clock descriptor registered\n");
- }
+ __set_fault_handler(data, &default_fault_handler);
- if (data->clk[0] && deli) {
- *deli = ',';
- data->clk[1] = clk_get(dev, deli + 1);
- if (IS_ERR(data->clk[1]))
- data->clk[1] = NULL;
- }
+ data->sysmmu = dev;
+ data->clk = devm_clk_get(dev, "sysmmu");
+ if (IS_ERR(data->clk)) {
+ dev_info(dev, "No gate clock found!\n");
+ data->clk = NULL;
+ }
- data->dbgname = platdata->dbgname;
+ ret = clk_prepare(data->clk);
+ if (ret) {
+ dev_err(dev, "Failed to prepare clk\n");
+ return ret;
}
- data->sysmmu = dev;
rwlock_init(&data->lock);
INIT_LIST_HEAD(&data->node);
- __set_fault_handler(data, &default_fault_handler);
-
- if (dev->parent)
- pm_runtime_enable(dev);
+ platform_set_drvdata(pdev, data);
+ dev_dbg(dev, "Probed and initialized\n");
- dev_dbg(dev, "(%s) Initialized\n", data->dbgname);
- return 0;
-err_irq:
- while (i-- > 0) {
- int irq;
-
- irq = platform_get_irq(pdev, i);
- free_irq(irq, data);
- }
-err_res:
- while (data->nsfrs-- > 0)
- iounmap(data->sfrbases[data->nsfrs]);
- kfree(data->sfrbases);
-err_init:
- kfree(data);
-err_alloc:
- dev_err(dev, "Failed to initialize\n");
return ret;
}
-static struct platform_driver exynos_sysmmu_driver = {
- .probe = exynos_sysmmu_probe,
- .driver = {
+#ifdef CONFIG_OF
+static struct of_device_id sysmmu_of_match[] __initconst = {
+ { .compatible = "samsung,exynos4210-sysmmu", },
+ { },
+};
+#endif
+
+static struct platform_driver exynos_sysmmu_driver __refdata = {
+ .probe = exynos_sysmmu_probe,
+ .driver = {
.owner = THIS_MODULE,
.name = "exynos-sysmmu",
+ .of_match_table = of_match_ptr(sysmmu_of_match),
}
};
--
1.7.2.5
Hi KyongHo,
On Thursday 08 of August 2013 18:38:49 Cho KyongHo wrote:
> This commit adds device tree support for System MMU.
> This also include the following changes and enhancements:
>
> * use managed device helper functions.
> Simplyfies System MMU device driver.
>
> * use only a single clock descriptor.
> System MMU device descriptor is seperate if it is imposible to make
> a single clock descriptor to make a device descriptor for a group of
> System MMUs.
>
> * removed dbgname member from sysmmu_drvdata structure.
> debugging kernel message for a System MMU is distinguisheable with the
> name of device descroptors.
Please put all these three changes in separate patches. This patch is hard
to review with all the changes mixed together...
In addition, I believe this is the patch that should be adding device tree
binding documentation, not the 6/16 one, as this is where actually support
for this binding gets added to the kernel.
> Signed-off-by: Cho KyongHo <[email protected]>
> ---
> drivers/iommu/Kconfig | 5 +-
> drivers/iommu/exynos-iommu.c | 186
> ++++++++++++++++------------------------- 2 files changed, 75
> insertions(+), 116 deletions(-)
>
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 820d85c..9ad45f1 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -168,16 +168,15 @@ config TEGRA_IOMMU_SMMU
>
> config EXYNOS_IOMMU
> bool "Exynos IOMMU Support"
> - depends on ARCH_EXYNOS && EXYNOS_DEV_SYSMMU
> + depends on ARCH_EXYNOS
> select IOMMU_API
> + default n
> help
> Support for the IOMMU(System MMU) of Samsung Exynos application
> processor family. This enables H/W multimedia accellerators to
see
> non-linear physical memory chunks as a linear memory in their
> address spaces
>
> - If unsure, say N here.
> -
> config EXYNOS_IOMMU_DEBUG
> bool "Debugging log for Exynos IOMMU"
> depends on EXYNOS_IOMMU
> diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> index a318049..0ee73e8 100644
> --- a/drivers/iommu/exynos-iommu.c
> +++ b/drivers/iommu/exynos-iommu.c
> @@ -26,6 +26,7 @@
> #include <linux/list.h>
> #include <linux/memblock.h>
> #include <linux/export.h>
> +#include <linux/of.h>
>
> #include <asm/cacheflush.h>
> #include <asm/pgtable.h>
> @@ -170,15 +171,14 @@ struct sysmmu_drvdata {
> struct list_head node; /* entry of exynos_iommu_domain.clients */
> 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];
> + struct clk *clk;
> int activations;
> rwlock_t lock;
> struct iommu_domain *domain;
> sysmmu_fault_handler_t fault_handler;
> unsigned long pgtable;
> + void __iomem *sfrbases[0];
> };
>
> static bool set_sysmmu_active(struct sysmmu_drvdata *data)
> @@ -385,8 +385,8 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void
> *dev_id) if (!ret && (itype != SYSMMU_FAULT_UNKNOWN))
> __raw_writel(1 << itype, data->sfrbases[i] +
REG_INT_CLEAR);
> else
> - dev_dbg(data->sysmmu, "(%s) %s is not handled.\n",
> - data->dbgname, sysmmu_fault_name[itype]);
> + dev_dbg(data->sysmmu, "%s is not handled.\n",
> + sysmmu_fault_name[itype]);
>
> if (itype != SYSMMU_FAULT_UNKNOWN)
> sysmmu_unblock(data->sfrbases[i]);
> @@ -410,10 +410,8 @@ static bool __exynos_sysmmu_disable(struct
> sysmmu_drvdata *data) for (i = 0; i < data->nsfrs; i++)
> __raw_writel(CTRL_DISABLE, data->sfrbases[i] +
REG_MMU_CTRL);
>
> - if (data->clk[1])
> - clk_disable(data->clk[1]);
> - if (data->clk[0])
> - clk_disable(data->clk[0]);
> + if (data->clk)
> + clk_disable(data->clk);
>
> disabled = true;
> data->pgtable = 0;
> @@ -422,10 +420,10 @@ finish:
> write_unlock_irqrestore(&data->lock, flags);
>
> if (disabled)
> - dev_dbg(data->sysmmu, "(%s) Disabled\n", data->dbgname);
> + dev_dbg(data->sysmmu, "Disabled\n");
> else
> - dev_dbg(data->sysmmu, "(%s) %d times left to be
disabled\n",
> - data->dbgname, data->activations);
> + dev_dbg(data->sysmmu, "%d times left to be disabled\n",
> + data->activations);
>
> return disabled;
> }
> @@ -452,14 +450,12 @@ static int __exynos_sysmmu_enable(struct
> sysmmu_drvdata *data, ret = 1;
> }
>
> - dev_dbg(data->sysmmu, "(%s) Already enabled\n", data-
>dbgname);
> + dev_dbg(data->sysmmu, "Already enabled\n");
> 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;
>
> @@ -479,7 +475,7 @@ static int __exynos_sysmmu_enable(struct
> sysmmu_drvdata *data,
>
> data->domain = domain;
>
> - dev_dbg(data->sysmmu, "(%s) Enabled\n", data->dbgname);
> + dev_dbg(data->sysmmu, "Enabled\n");
> finish:
> write_unlock_irqrestore(&data->lock, flags);
>
> @@ -495,7 +491,7 @@ int exynos_sysmmu_enable(struct device *dev,
> unsigned long pgtable)
>
> ret = pm_runtime_get_sync(data->sysmmu);
> if (ret < 0) {
> - dev_dbg(data->sysmmu, "(%s) Failed to enable\n", data-
>dbgname);
> + dev_dbg(data->sysmmu, "Failed to enable\n");
> return ret;
> }
>
> @@ -503,8 +499,8 @@ int exynos_sysmmu_enable(struct device *dev,
> unsigned long pgtable) if (WARN_ON(ret < 0)) {
> pm_runtime_put(data->sysmmu);
> dev_err(data->sysmmu,
> - "(%s) Already enabled with page table %#lx\n",
> - data->dbgname, data->pgtable);
> + "Already enabled with page table %#lx\n",
> + data->pgtable);
> } else {
> data->dev = dev;
> }
> @@ -540,9 +536,7 @@ static void sysmmu_tlb_invalidate_entry(struct
> device *dev, unsigned long iova) }
> }
> } else {
> - dev_dbg(data->sysmmu,
> - "(%s) Disabled. Skipping invalidating TLB.\n",
> - data->dbgname);
> + dev_dbg(data->sysmmu, "Disabled. Skipping invalidating
TLB.\n");
> }
>
> read_unlock_irqrestore(&data->lock, flags);
> @@ -564,141 +558,107 @@ void exynos_sysmmu_tlb_invalidate(struct device
> *dev) }
> }
> } else {
> - dev_dbg(data->sysmmu,
> - "(%s) Disabled. Skipping invalidating TLB.\n",
> - data->dbgname);
> + dev_dbg(data->sysmmu, "Disabled. Skipping invalidating
TLB.\n");
> }
>
> read_unlock_irqrestore(&data->lock, flags);
> }
>
> -static int exynos_sysmmu_probe(struct platform_device *pdev)
> +static int __init exynos_sysmmu_probe(struct platform_device *pdev)
> {
> int i, ret;
> - struct device *dev;
> + struct device *dev = &pdev->dev;
> struct sysmmu_drvdata *data;
>
> - dev = &pdev->dev;
> -
> - data = kzalloc(sizeof(*data), GFP_KERNEL);
> - if (!data) {
> - dev_dbg(dev, "Not enough memory\n");
> - ret = -ENOMEM;
> - goto err_alloc;
> + if (pdev->num_resources == 0) {
> + dev_err(dev, "No System MMU resource defined\n");
> + return -ENODEV;
> }
>
> - ret = dev_set_drvdata(dev, data);
> - if (ret) {
> - dev_dbg(dev, "Unabled to initialize driver data\n");
> - goto err_init;
> + data = devm_kzalloc(dev,
> + sizeof(*data) +
> + sizeof(*data->sfrbases) * (pdev->num_resources /
2),
> + GFP_KERNEL);
> + if (!data) {
> + dev_err(dev, "Not enough memory for initialization\n");
nit: This message is not necessary, as kmalloc() already prints a message
if allocation error occurs.
> + return -ENOMEM;
> }
>
> data->nsfrs = pdev->num_resources / 2;
As Marek and Bart already mentioned before, please (oh please, really
please) finally remove this brokenness from this driver.
A device driver should not do this kind of abstraction, emulating one
instance of particular IP from multiple instances. This is a task for the
IOMMU core and this is already supported by IOMMU domains, which is the
right way to achieve a single virtual address space for multiple IOMMUs.
> - 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;
> - }
>
> 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;
> + dev_err(dev, "Unable to find IOMEM region\n");
> + return -ENOENT;
> }
>
> - data->sfrbases[i] = ioremap(res->start,
resource_size(res));
> + data->sfrbases[i] = devm_request_and_ioremap(dev, res);
devm_request_and_ioremap() is deprecated. Please use
devm_ioremap_resource().
> if (!data->sfrbases[i]) {
In case of devm_ioremap_resource() remember to adjust this check to check
for IS_ERR, not for NULL.
> - dev_dbg(dev, "Unable to map IOMEM @ PA:%#x\n",
> - res->start);
> - ret = -ENOENT;
> - goto err_res;
> + dev_err(dev, "Unable to map IOMEM @ %#x\n", res-
>start);
This message will not be needed with devm_ioremap_resource() as it already
prints a message if an error occurs.
> + return -EBUSY;
> }
> }
>
> 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;
> + int irq;
> +
> + irq = platform_get_irq(pdev, i);
> + if (irq <= 0) {
> + dev_err(dev, "Unable to find IRQ resource\n");
> + return -ENOENT;
> }
>
> - ret = request_irq(ret, exynos_sysmmu_irq, 0,
> - dev_name(dev), data);
> + ret = devm_request_irq(dev, irq, exynos_sysmmu_irq,
> + 0, dev_name(dev), data);
> if (ret) {
> - dev_dbg(dev, "Unabled to register interrupt
handler\n");
> - goto err_irq;
> + dev_err(dev, "Unable to register handler to irq
%d\n",
> + irq);
> + return ret;
> }
> }
>
> - 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 */;
> + pm_runtime_enable(dev);
>
> - 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;
> - dev_dbg(dev, "No clock descriptor registered\n");
> - }
> + __set_fault_handler(data, &default_fault_handler);
>
> - if (data->clk[0] && deli) {
> - *deli = ',';
> - data->clk[1] = clk_get(dev, deli + 1);
> - if (IS_ERR(data->clk[1]))
> - data->clk[1] = NULL;
> - }
> + data->sysmmu = dev;
> + data->clk = devm_clk_get(dev, "sysmmu");
> + if (IS_ERR(data->clk)) {
> + dev_info(dev, "No gate clock found!\n");
> + data->clk = NULL;
Just a note: In common clock framework NULL is a valid value for a struct
clk *, which should not be interpreted as a pointer, but as an opaque
cookie. This is material for separate patch, but this should be fixed all
around this driver so data->clk is for IS_ERR instead.
Best regards,
Tomasz
> + }
>
> - data->dbgname = platdata->dbgname;
> + ret = clk_prepare(data->clk);
> + if (ret) {
> + dev_err(dev, "Failed to prepare clk\n");
> + return ret;
> }
>
> - data->sysmmu = dev;
> rwlock_init(&data->lock);
> INIT_LIST_HEAD(&data->node);
>
> - __set_fault_handler(data, &default_fault_handler);
> -
> - if (dev->parent)
> - pm_runtime_enable(dev);
> + platform_set_drvdata(pdev, data);
> + dev_dbg(dev, "Probed and initialized\n");
>
> - dev_dbg(dev, "(%s) Initialized\n", data->dbgname);
> - return 0;
> -err_irq:
> - while (i-- > 0) {
> - int irq;
> -
> - irq = platform_get_irq(pdev, i);
> - free_irq(irq, data);
> - }
> -err_res:
> - while (data->nsfrs-- > 0)
> - iounmap(data->sfrbases[data->nsfrs]);
> - kfree(data->sfrbases);
> -err_init:
> - kfree(data);
> -err_alloc:
> - dev_err(dev, "Failed to initialize\n");
> return ret;
> }
>
> -static struct platform_driver exynos_sysmmu_driver = {
> - .probe = exynos_sysmmu_probe,
> - .driver = {
> +#ifdef CONFIG_OF
> +static struct of_device_id sysmmu_of_match[] __initconst = {
> + { .compatible = "samsung,exynos4210-sysmmu", },
> + { },
> +};
> +#endif
> +
> +static struct platform_driver exynos_sysmmu_driver __refdata = {
> + .probe = exynos_sysmmu_probe,
> + .driver = {
> .owner = THIS_MODULE,
> .name = "exynos-sysmmu",
> + .of_match_table = of_match_ptr(sysmmu_of_match),
> }
> };
On Fri, 09 Aug 2013 00:41:25 +0200, Tomasz Figa wrote:
> Hi KyongHo,
>
> On Thursday 08 of August 2013 18:38:49 Cho KyongHo wrote:
> > This commit adds device tree support for System MMU.
> > This also include the following changes and enhancements:
> >
> > * use managed device helper functions.
> > Simplyfies System MMU device driver.
> >
> > * use only a single clock descriptor.
> > System MMU device descriptor is seperate if it is imposible to make
> > a single clock descriptor to make a device descriptor for a group of
> > System MMUs.
> >
> > * removed dbgname member from sysmmu_drvdata structure.
> > debugging kernel message for a System MMU is distinguisheable with the
> > name of device descroptors.
>
> Please put all these three changes in separate patches. This patch is hard
> to review with all the changes mixed together...
>
Ok.
> In addition, I believe this is the patch that should be adding device tree
> binding documentation, not the 6/16 one, as this is where actually support
> for this binding gets added to the kernel.
Oh, I didn't know that devicetree binding description and implementation need
to be in the same patch.
I will do as you advised.
>
> > Signed-off-by: Cho KyongHo <[email protected]>
> > ---
> > drivers/iommu/Kconfig | 5 +-
> > drivers/iommu/exynos-iommu.c | 186
> > ++++++++++++++++------------------------- 2 files changed, 75
> > insertions(+), 116 deletions(-)
> >
> > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > index 820d85c..9ad45f1 100644
> > --- a/drivers/iommu/Kconfig
> > +++ b/drivers/iommu/Kconfig
> > @@ -168,16 +168,15 @@ config TEGRA_IOMMU_SMMU
> >
> > config EXYNOS_IOMMU
> > bool "Exynos IOMMU Support"
> > - depends on ARCH_EXYNOS && EXYNOS_DEV_SYSMMU
> > + depends on ARCH_EXYNOS
> > select IOMMU_API
> > + default n
> > help
> > Support for the IOMMU(System MMU) of Samsung Exynos application
> > processor family. This enables H/W multimedia accellerators to
> see
> > non-linear physical memory chunks as a linear memory in their
> > address spaces
> >
> > - If unsure, say N here.
> > -
> > config EXYNOS_IOMMU_DEBUG
> > bool "Debugging log for Exynos IOMMU"
> > depends on EXYNOS_IOMMU
> > diff --git a/drivers/iommu/exynos-iommu.c b/drivers/iommu/exynos-iommu.c
> > index a318049..0ee73e8 100644
> > --- a/drivers/iommu/exynos-iommu.c
> > +++ b/drivers/iommu/exynos-iommu.c
> > @@ -26,6 +26,7 @@
> > #include <linux/list.h>
> > #include <linux/memblock.h>
> > #include <linux/export.h>
> > +#include <linux/of.h>
> >
> > #include <asm/cacheflush.h>
> > #include <asm/pgtable.h>
> > @@ -170,15 +171,14 @@ struct sysmmu_drvdata {
> > struct list_head node; /* entry of exynos_iommu_domain.clients */
> > 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];
> > + struct clk *clk;
> > int activations;
> > rwlock_t lock;
> > struct iommu_domain *domain;
> > sysmmu_fault_handler_t fault_handler;
> > unsigned long pgtable;
> > + void __iomem *sfrbases[0];
> > };
> >
> > static bool set_sysmmu_active(struct sysmmu_drvdata *data)
> > @@ -385,8 +385,8 @@ static irqreturn_t exynos_sysmmu_irq(int irq, void
> > *dev_id) if (!ret && (itype != SYSMMU_FAULT_UNKNOWN))
> > __raw_writel(1 << itype, data->sfrbases[i] +
> REG_INT_CLEAR);
> > else
> > - dev_dbg(data->sysmmu, "(%s) %s is not handled.\n",
> > - data->dbgname, sysmmu_fault_name[itype]);
> > + dev_dbg(data->sysmmu, "%s is not handled.\n",
> > + sysmmu_fault_name[itype]);
> >
> > if (itype != SYSMMU_FAULT_UNKNOWN)
> > sysmmu_unblock(data->sfrbases[i]);
> > @@ -410,10 +410,8 @@ static bool __exynos_sysmmu_disable(struct
> > sysmmu_drvdata *data) for (i = 0; i < data->nsfrs; i++)
> > __raw_writel(CTRL_DISABLE, data->sfrbases[i] +
> REG_MMU_CTRL);
> >
> > - if (data->clk[1])
> > - clk_disable(data->clk[1]);
> > - if (data->clk[0])
> > - clk_disable(data->clk[0]);
> > + if (data->clk)
> > + clk_disable(data->clk);
> >
> > disabled = true;
> > data->pgtable = 0;
> > @@ -422,10 +420,10 @@ finish:
> > write_unlock_irqrestore(&data->lock, flags);
> >
> > if (disabled)
> > - dev_dbg(data->sysmmu, "(%s) Disabled\n", data->dbgname);
> > + dev_dbg(data->sysmmu, "Disabled\n");
> > else
> > - dev_dbg(data->sysmmu, "(%s) %d times left to be
> disabled\n",
> > - data->dbgname, data->activations);
> > + dev_dbg(data->sysmmu, "%d times left to be disabled\n",
> > + data->activations);
> >
> > return disabled;
> > }
> > @@ -452,14 +450,12 @@ static int __exynos_sysmmu_enable(struct
> > sysmmu_drvdata *data, ret = 1;
> > }
> >
> > - dev_dbg(data->sysmmu, "(%s) Already enabled\n", data-
> >dbgname);
> > + dev_dbg(data->sysmmu, "Already enabled\n");
> > 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;
> >
> > @@ -479,7 +475,7 @@ static int __exynos_sysmmu_enable(struct
> > sysmmu_drvdata *data,
> >
> > data->domain = domain;
> >
> > - dev_dbg(data->sysmmu, "(%s) Enabled\n", data->dbgname);
> > + dev_dbg(data->sysmmu, "Enabled\n");
> > finish:
> > write_unlock_irqrestore(&data->lock, flags);
> >
> > @@ -495,7 +491,7 @@ int exynos_sysmmu_enable(struct device *dev,
> > unsigned long pgtable)
> >
> > ret = pm_runtime_get_sync(data->sysmmu);
> > if (ret < 0) {
> > - dev_dbg(data->sysmmu, "(%s) Failed to enable\n", data-
> >dbgname);
> > + dev_dbg(data->sysmmu, "Failed to enable\n");
> > return ret;
> > }
> >
> > @@ -503,8 +499,8 @@ int exynos_sysmmu_enable(struct device *dev,
> > unsigned long pgtable) if (WARN_ON(ret < 0)) {
> > pm_runtime_put(data->sysmmu);
> > dev_err(data->sysmmu,
> > - "(%s) Already enabled with page table %#lx\n",
> > - data->dbgname, data->pgtable);
> > + "Already enabled with page table %#lx\n",
> > + data->pgtable);
> > } else {
> > data->dev = dev;
> > }
> > @@ -540,9 +536,7 @@ static void sysmmu_tlb_invalidate_entry(struct
> > device *dev, unsigned long iova) }
> > }
> > } else {
> > - dev_dbg(data->sysmmu,
> > - "(%s) Disabled. Skipping invalidating TLB.\n",
> > - data->dbgname);
> > + dev_dbg(data->sysmmu, "Disabled. Skipping invalidating
> TLB.\n");
> > }
> >
> > read_unlock_irqrestore(&data->lock, flags);
> > @@ -564,141 +558,107 @@ void exynos_sysmmu_tlb_invalidate(struct device
> > *dev) }
> > }
> > } else {
> > - dev_dbg(data->sysmmu,
> > - "(%s) Disabled. Skipping invalidating TLB.\n",
> > - data->dbgname);
> > + dev_dbg(data->sysmmu, "Disabled. Skipping invalidating
> TLB.\n");
> > }
> >
> > read_unlock_irqrestore(&data->lock, flags);
> > }
> >
> > -static int exynos_sysmmu_probe(struct platform_device *pdev)
> > +static int __init exynos_sysmmu_probe(struct platform_device *pdev)
> > {
> > int i, ret;
> > - struct device *dev;
> > + struct device *dev = &pdev->dev;
> > struct sysmmu_drvdata *data;
> >
> > - dev = &pdev->dev;
> > -
> > - data = kzalloc(sizeof(*data), GFP_KERNEL);
> > - if (!data) {
> > - dev_dbg(dev, "Not enough memory\n");
> > - ret = -ENOMEM;
> > - goto err_alloc;
> > + if (pdev->num_resources == 0) {
> > + dev_err(dev, "No System MMU resource defined\n");
> > + return -ENODEV;
> > }
> >
> > - ret = dev_set_drvdata(dev, data);
> > - if (ret) {
> > - dev_dbg(dev, "Unabled to initialize driver data\n");
> > - goto err_init;
> > + data = devm_kzalloc(dev,
> > + sizeof(*data) +
> > + sizeof(*data->sfrbases) * (pdev->num_resources /
> 2),
> > + GFP_KERNEL);
> > + if (!data) {
> > + dev_err(dev, "Not enough memory for initialization\n");
>
> nit: This message is not necessary, as kmalloc() already prints a message
> if allocation error occurs.
Ok.
>
> > + return -ENOMEM;
> > }
> >
> > data->nsfrs = pdev->num_resources / 2;
>
> As Marek and Bart already mentioned before, please (oh please, really
> please) finally remove this brokenness from this driver.
>
> A device driver should not do this kind of abstraction, emulating one
> instance of particular IP from multiple instances. This is a task for the
> IOMMU core and this is already supported by IOMMU domains, which is the
> right way to achieve a single virtual address space for multiple IOMMUs.
>
Let me show some example.
FIMD in Exynos5250 has a single System MMU.
FIMD in Exynos5420 has 2 System MMUs.
I wanted to show the FIMD driver the same view of System MMU.
Otherwise, FIMD driver must aware of how the System MMUs are connected
with FIMD.
If it is responsible for the device drivers of master H/Ws, System MMU
driver can be much more simpler which I also prefer :)
> > - 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;
> > - }
> >
> > 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;
> > + dev_err(dev, "Unable to find IOMEM region\n");
> > + return -ENOENT;
> > }
> >
> > - data->sfrbases[i] = ioremap(res->start,
> resource_size(res));
> > + data->sfrbases[i] = devm_request_and_ioremap(dev, res);
>
> devm_request_and_ioremap() is deprecated. Please use
> devm_ioremap_resource().
>
Oh. Thanks.
> > if (!data->sfrbases[i]) {
>
> In case of devm_ioremap_resource() remember to adjust this check to check
> for IS_ERR, not for NULL.
Ok. Thanks.
>
> > - dev_dbg(dev, "Unable to map IOMEM @ PA:%#x\n",
> > - res->start);
> > - ret = -ENOENT;
> > - goto err_res;
> > + dev_err(dev, "Unable to map IOMEM @ %#x\n", res-
> >start);
>
> This message will not be needed with devm_ioremap_resource() as it already
> prints a message if an error occurs.
Ok.
>
> > + return -EBUSY;
> > }
> > }
> >
> > 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;
> > + int irq;
> > +
> > + irq = platform_get_irq(pdev, i);
> > + if (irq <= 0) {
> > + dev_err(dev, "Unable to find IRQ resource\n");
> > + return -ENOENT;
> > }
> >
> > - ret = request_irq(ret, exynos_sysmmu_irq, 0,
> > - dev_name(dev), data);
> > + ret = devm_request_irq(dev, irq, exynos_sysmmu_irq,
> > + 0, dev_name(dev), data);
> > if (ret) {
> > - dev_dbg(dev, "Unabled to register interrupt
> handler\n");
> > - goto err_irq;
> > + dev_err(dev, "Unable to register handler to irq
> %d\n",
> > + irq);
> > + return ret;
> > }
> > }
> >
> > - 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 */;
> > + pm_runtime_enable(dev);
> >
> > - 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;
> > - dev_dbg(dev, "No clock descriptor registered\n");
> > - }
> > + __set_fault_handler(data, &default_fault_handler);
> >
> > - if (data->clk[0] && deli) {
> > - *deli = ',';
> > - data->clk[1] = clk_get(dev, deli + 1);
> > - if (IS_ERR(data->clk[1]))
> > - data->clk[1] = NULL;
> > - }
> > + data->sysmmu = dev;
> > + data->clk = devm_clk_get(dev, "sysmmu");
> > + if (IS_ERR(data->clk)) {
> > + dev_info(dev, "No gate clock found!\n");
> > + data->clk = NULL;
>
> Just a note: In common clock framework NULL is a valid value for a struct
> clk *, which should not be interpreted as a pointer, but as an opaque
> cookie. This is material for separate patch, but this should be fixed all
> around this driver so data->clk is for IS_ERR instead.
Error returned by devm_clk_get("sysmmu") is not an error actually.
More specifically, -ENOENT returned by devm_clk_get("sysmmu") is not an error.
It just means that a System MMU does not have to gating its clock.
However, it is required to check if the error retruned is -ENOENT.
data->clk must be NULL if 'sysmmu' clock is not specified for a System MMU
because clk_enable(data->clk) is called regardless of the value in data->clk.
Thank you for review :)
KyongHo.
>
> Best regards,
> Tomasz
>
> > + }
> >
> > - data->dbgname = platdata->dbgname;
> > + ret = clk_prepare(data->clk);
> > + if (ret) {
> > + dev_err(dev, "Failed to prepare clk\n");
> > + return ret;
> > }
> >
> > - data->sysmmu = dev;
> > rwlock_init(&data->lock);
> > INIT_LIST_HEAD(&data->node);
> >
> > - __set_fault_handler(data, &default_fault_handler);
> > -
> > - if (dev->parent)
> > - pm_runtime_enable(dev);
> > + platform_set_drvdata(pdev, data);
> > + dev_dbg(dev, "Probed and initialized\n");
> >
> > - dev_dbg(dev, "(%s) Initialized\n", data->dbgname);
> > - return 0;
> > -err_irq:
> > - while (i-- > 0) {
> > - int irq;
> > -
> > - irq = platform_get_irq(pdev, i);
> > - free_irq(irq, data);
> > - }
> > -err_res:
> > - while (data->nsfrs-- > 0)
> > - iounmap(data->sfrbases[data->nsfrs]);
> > - kfree(data->sfrbases);
> > -err_init:
> > - kfree(data);
> > -err_alloc:
> > - dev_err(dev, "Failed to initialize\n");
> > return ret;
> > }
> >
> > -static struct platform_driver exynos_sysmmu_driver = {
> > - .probe = exynos_sysmmu_probe,
> > - .driver = {
> > +#ifdef CONFIG_OF
> > +static struct of_device_id sysmmu_of_match[] __initconst = {
> > + { .compatible = "samsung,exynos4210-sysmmu", },
> > + { },
> > +};
> > +#endif
> > +
> > +static struct platform_driver exynos_sysmmu_driver __refdata = {
> > + .probe = exynos_sysmmu_probe,
> > + .driver = {
> > .owner = THIS_MODULE,
> > .name = "exynos-sysmmu",
> > + .of_match_table = of_match_ptr(sysmmu_of_match),
> > }
> > };
On Friday 09 of August 2013 15:37:30 Cho KyongHo wrote:
> On Fri, 09 Aug 2013 00:41:25 +0200, Tomasz Figa wrote:
> > Hi KyongHo,
> >
> > On Thursday 08 of August 2013 18:38:49 Cho KyongHo wrote:
> > > This commit adds device tree support for System MMU.
> > > This also include the following changes and enhancements:
> > >
> > > * use managed device helper functions.
> > > Simplyfies System MMU device driver.
> > >
> > > * use only a single clock descriptor.
> > > System MMU device descriptor is seperate if it is imposible to make
> > > a single clock descriptor to make a device descriptor for a group of
> > > System MMUs.
> > >
> > > * removed dbgname member from sysmmu_drvdata structure.
> > > debugging kernel message for a System MMU is distinguisheable with
> > > the
> > > name of device descroptors.
> >
> > Please put all these three changes in separate patches. This patch is
> > hard to review with all the changes mixed together...
>
> Ok.
>
> > In addition, I believe this is the patch that should be adding device
> > tree binding documentation, not the 6/16 one, as this is where
> > actually support for this binding gets added to the kernel.
>
> Oh, I didn't know that devicetree binding description and implementation
> need to be in the same patch.
> I will do as you advised.
Thanks. We are still missing a "best practices" document about device tree
binding development, so it might not be clear.
> > > Signed-off-by: Cho KyongHo <[email protected]>
> > > ---
> > >
> > > drivers/iommu/Kconfig | 5 +-
> > > drivers/iommu/exynos-iommu.c | 186
> > >
> > > ++++++++++++++++------------------------- 2 files changed, 75
> > > insertions(+), 116 deletions(-)
> > >
> > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > > index 820d85c..9ad45f1 100644
> > > --- a/drivers/iommu/Kconfig
> > > +++ b/drivers/iommu/Kconfig
> > > @@ -168,16 +168,15 @@ config TEGRA_IOMMU_SMMU
> > >
> > > config EXYNOS_IOMMU
> > >
> > > bool "Exynos IOMMU Support"
> > >
> > > - depends on ARCH_EXYNOS && EXYNOS_DEV_SYSMMU
> > > + depends on ARCH_EXYNOS
> > >
> > > select IOMMU_API
> > >
> > > + default n
> > >
> > > help
> > >
> > > Support for the IOMMU(System MMU) of Samsung Exynos application
> > > processor family. This enables H/W multimedia accellerators to
> >
> > see
> >
> > > non-linear physical memory chunks as a linear memory in their
> > > address spaces
> > >
> > > - If unsure, say N here.
> > > -
> > >
> > > config EXYNOS_IOMMU_DEBUG
> > >
> > > bool "Debugging log for Exynos IOMMU"
> > > depends on EXYNOS_IOMMU
> > >
> > > diff --git a/drivers/iommu/exynos-iommu.c
> > > b/drivers/iommu/exynos-iommu.c index a318049..0ee73e8 100644
> > > --- a/drivers/iommu/exynos-iommu.c
> > > +++ b/drivers/iommu/exynos-iommu.c
> > > @@ -26,6 +26,7 @@
> > >
> > > #include <linux/list.h>
> > > #include <linux/memblock.h>
> > > #include <linux/export.h>
> > >
> > > +#include <linux/of.h>
> > >
> > > #include <asm/cacheflush.h>
> > > #include <asm/pgtable.h>
> > >
> > > @@ -170,15 +171,14 @@ struct sysmmu_drvdata {
> > >
> > > struct list_head node; /* entry of exynos_iommu_domain.clients */
> > > 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];
> > > + struct clk *clk;
> > >
> > > int activations;
> > > rwlock_t lock;
> > > struct iommu_domain *domain;
> > > sysmmu_fault_handler_t fault_handler;
> > > unsigned long pgtable;
> > >
> > > + void __iomem *sfrbases[0];
> > >
> > > };
> > >
> > > static bool set_sysmmu_active(struct sysmmu_drvdata *data)
> > >
> > > @@ -385,8 +385,8 @@ static irqreturn_t exynos_sysmmu_irq(int irq,
> > > void
> > > *dev_id) if (!ret && (itype != SYSMMU_FAULT_UNKNOWN))
> > >
> > > __raw_writel(1 << itype, data->sfrbases[i] +
> >
> > REG_INT_CLEAR);
> >
> > > else
> > >
> > > - dev_dbg(data->sysmmu, "(%s) %s is not handled.\n",
> > > - data->dbgname, sysmmu_fault_name[itype]);
> > > + dev_dbg(data->sysmmu, "%s is not handled.\n",
> > > + sysmmu_fault_name[itype]);
> > >
> > > if (itype != SYSMMU_FAULT_UNKNOWN)
> > >
> > > sysmmu_unblock(data->sfrbases[i]);
> > >
> > > @@ -410,10 +410,8 @@ static bool __exynos_sysmmu_disable(struct
> > > sysmmu_drvdata *data) for (i = 0; i < data->nsfrs; i++)
> > >
> > > __raw_writel(CTRL_DISABLE, data->sfrbases[i] +
> >
> > REG_MMU_CTRL);
> >
> > > - if (data->clk[1])
> > > - clk_disable(data->clk[1]);
> > > - if (data->clk[0])
> > > - clk_disable(data->clk[0]);
> > > + if (data->clk)
> > > + clk_disable(data->clk);
> > >
> > > disabled = true;
> > > data->pgtable = 0;
> > >
> > > @@ -422,10 +420,10 @@ finish:
> > > write_unlock_irqrestore(&data->lock, flags);
> > >
> > > if (disabled)
> > >
> > > - dev_dbg(data->sysmmu, "(%s) Disabled\n", data->dbgname);
> > > + dev_dbg(data->sysmmu, "Disabled\n");
> > >
> > > else
> > >
> > > - dev_dbg(data->sysmmu, "(%s) %d times left to be
> >
> > disabled\n",
> >
> > > - data->dbgname, data->activations);
> > > + dev_dbg(data->sysmmu, "%d times left to be disabled\n",
> > > + data->activations);
> > >
> > > return disabled;
> > >
> > > }
> > >
> > > @@ -452,14 +450,12 @@ static int __exynos_sysmmu_enable(struct
> > > sysmmu_drvdata *data, ret = 1;
> > >
> > > }
> > >
> > > - dev_dbg(data->sysmmu, "(%s) Already enabled\n", data-
> > >
> > >dbgname);
> > >
> > > + dev_dbg(data->sysmmu, "Already enabled\n");
> > >
> > > 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;
> > >
> > > @@ -479,7 +475,7 @@ static int __exynos_sysmmu_enable(struct
> > > sysmmu_drvdata *data,
> > >
> > > data->domain = domain;
> > >
> > > - dev_dbg(data->sysmmu, "(%s) Enabled\n", data->dbgname);
> > > + dev_dbg(data->sysmmu, "Enabled\n");
> > >
> > > finish:
> > > write_unlock_irqrestore(&data->lock, flags);
> > >
> > > @@ -495,7 +491,7 @@ int exynos_sysmmu_enable(struct device *dev,
> > > unsigned long pgtable)
> > >
> > > ret = pm_runtime_get_sync(data->sysmmu);
> > > if (ret < 0) {
> > >
> > > - dev_dbg(data->sysmmu, "(%s) Failed to enable\n", data-
> > >
> > >dbgname);
> > >
> > > + dev_dbg(data->sysmmu, "Failed to enable\n");
> > >
> > > return ret;
> > >
> > > }
> > >
> > > @@ -503,8 +499,8 @@ int exynos_sysmmu_enable(struct device *dev,
> > > unsigned long pgtable) if (WARN_ON(ret < 0)) {
> > >
> > > pm_runtime_put(data->sysmmu);
> > > dev_err(data->sysmmu,
> > >
> > > - "(%s) Already enabled with page table %#lx\n",
> > > - data->dbgname, data->pgtable);
> > > + "Already enabled with page table %#lx\n",
> > > + data->pgtable);
> > >
> > > } else {
> > >
> > > data->dev = dev;
> > >
> > > }
> > >
> > > @@ -540,9 +536,7 @@ static void sysmmu_tlb_invalidate_entry(struct
> > > device *dev, unsigned long iova) }
> > >
> > > }
> > >
> > > } else {
> > >
> > > - dev_dbg(data->sysmmu,
> > > - "(%s) Disabled. Skipping invalidating TLB.\n",
> > > - data->dbgname);
> > > + dev_dbg(data->sysmmu, "Disabled. Skipping invalidating
> >
> > TLB.\n");
> >
> > > }
> > >
> > > read_unlock_irqrestore(&data->lock, flags);
> > >
> > > @@ -564,141 +558,107 @@ void exynos_sysmmu_tlb_invalidate(struct
> > > device
> > > *dev) }
> > >
> > > }
> > >
> > > } else {
> > >
> > > - dev_dbg(data->sysmmu,
> > > - "(%s) Disabled. Skipping invalidating TLB.\n",
> > > - data->dbgname);
> > > + dev_dbg(data->sysmmu, "Disabled. Skipping invalidating
> >
> > TLB.\n");
> >
> > > }
> > >
> > > read_unlock_irqrestore(&data->lock, flags);
> > >
> > > }
> > >
> > > -static int exynos_sysmmu_probe(struct platform_device *pdev)
> > > +static int __init exynos_sysmmu_probe(struct platform_device *pdev)
> > >
> > > {
> > >
> > > int i, ret;
> > >
> > > - struct device *dev;
> > > + struct device *dev = &pdev->dev;
> > >
> > > struct sysmmu_drvdata *data;
> > >
> > > - dev = &pdev->dev;
> > > -
> > > - data = kzalloc(sizeof(*data), GFP_KERNEL);
> > > - if (!data) {
> > > - dev_dbg(dev, "Not enough memory\n");
> > > - ret = -ENOMEM;
> > > - goto err_alloc;
> > > + if (pdev->num_resources == 0) {
> > > + dev_err(dev, "No System MMU resource defined\n");
> > > + return -ENODEV;
> > >
> > > }
> > >
> > > - ret = dev_set_drvdata(dev, data);
> > > - if (ret) {
> > > - dev_dbg(dev, "Unabled to initialize driver data\n");
> > > - goto err_init;
> > > + data = devm_kzalloc(dev,
> > > + sizeof(*data) +
> > > + sizeof(*data->sfrbases) * (pdev->num_resources /
> >
> > 2),
> >
> > > + GFP_KERNEL);
> > > + if (!data) {
> > > + dev_err(dev, "Not enough memory for initialization\n");
> >
> > nit: This message is not necessary, as kmalloc() already prints a
> > message if allocation error occurs.
>
> Ok.
>
> > > + return -ENOMEM;
> > >
> > > }
> > >
> > > data->nsfrs = pdev->num_resources / 2;
> >
> > As Marek and Bart already mentioned before, please (oh please, really
> > please) finally remove this brokenness from this driver.
> >
> > A device driver should not do this kind of abstraction, emulating one
> > instance of particular IP from multiple instances. This is a task for
> > the IOMMU core and this is already supported by IOMMU domains, which
> > is the right way to achieve a single virtual address space for
> > multiple IOMMUs.
> Let me show some example.
> FIMD in Exynos5250 has a single System MMU.
> FIMD in Exynos5420 has 2 System MMUs.
> I wanted to show the FIMD driver the same view of System MMU.
> Otherwise, FIMD driver must aware of how the System MMUs are connected
> with FIMD.
>
> If it is responsible for the device drivers of master H/Ws, System MMU
> driver can be much more simpler which I also prefer :)
Yes, it is responsibility of FIMD driver to add all its IOMMUs into the
same domain or into any other configuration it wants.
For example by some extension, the driver might want to have separate
address spaces for each IOMMU and this would not be possible with current
configuration of SysMMU driver.
> > > - 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;
> > > - }
> > >
> > > 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;
> > > + dev_err(dev, "Unable to find IOMEM region\n");
> > > + return -ENOENT;
> > >
> > > }
> > >
> > > - data->sfrbases[i] = ioremap(res->start,
> >
> > resource_size(res));
> >
> > > + data->sfrbases[i] = devm_request_and_ioremap(dev, res);
> >
> > devm_request_and_ioremap() is deprecated. Please use
> > devm_ioremap_resource().
>
> Oh. Thanks.
>
> > > if (!data->sfrbases[i]) {
> >
> > In case of devm_ioremap_resource() remember to adjust this check to
> > check for IS_ERR, not for NULL.
>
> Ok. Thanks.
>
> > > - dev_dbg(dev, "Unable to map IOMEM @ PA:%#x\n",
> > > - res->start);
> > > - ret = -ENOENT;
> > > - goto err_res;
> > > + dev_err(dev, "Unable to map IOMEM @ %#x\n", res-
> > >
> > >start);
> >
> > This message will not be needed with devm_ioremap_resource() as it
> > already prints a message if an error occurs.
>
> Ok.
>
> > > + return -EBUSY;
> > >
> > > }
> > >
> > > }
> > >
> > > 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;
> > > + int irq;
> > > +
> > > + irq = platform_get_irq(pdev, i);
> > > + if (irq <= 0) {
> > > + dev_err(dev, "Unable to find IRQ resource\n");
> > > + return -ENOENT;
> > >
> > > }
> > >
> > > - ret = request_irq(ret, exynos_sysmmu_irq, 0,
> > > - dev_name(dev), data);
> > > + ret = devm_request_irq(dev, irq, exynos_sysmmu_irq,
> > > + 0, dev_name(dev), data);
> > >
> > > if (ret) {
> > >
> > > - dev_dbg(dev, "Unabled to register interrupt
> >
> > handler\n");
> >
> > > - goto err_irq;
> > > + dev_err(dev, "Unable to register handler to irq
> >
> > %d\n",
> >
> > > + irq);
> > > + return ret;
> > >
> > > }
> > >
> > > }
> > >
> > > - 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 */;
> > > + pm_runtime_enable(dev);
> > >
> > > - 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;
> > > - dev_dbg(dev, "No clock descriptor registered\n");
> > > - }
> > > + __set_fault_handler(data, &default_fault_handler);
> > >
> > > - if (data->clk[0] && deli) {
> > > - *deli = ',';
> > > - data->clk[1] = clk_get(dev, deli + 1);
> > > - if (IS_ERR(data->clk[1]))
> > > - data->clk[1] = NULL;
> > > - }
> > > + data->sysmmu = dev;
> > > + data->clk = devm_clk_get(dev, "sysmmu");
> > > + if (IS_ERR(data->clk)) {
> > > + dev_info(dev, "No gate clock found!\n");
> > > + data->clk = NULL;
> >
> > Just a note: In common clock framework NULL is a valid value for a
> > struct clk *, which should not be interpreted as a pointer, but as an
> > opaque cookie. This is material for separate patch, but this should
> > be fixed all around this driver so data->clk is for IS_ERR instead.
>
> Error returned by devm_clk_get("sysmmu") is not an error actually.
> More specifically, -ENOENT returned by devm_clk_get("sysmmu") is not an
> error. It just means that a System MMU does not have to gating its
> clock. However, it is required to check if the error retruned is
> -ENOENT.
I agree with this. Since the clock is optional for this driver, the -
ENOENT error case is not an error for it.
> data->clk must be NULL if 'sysmmu' clock is not specified for a System
> MMU because clk_enable(data->clk) is called regardless of the value in
> data->clk.
Still, the return value convention for Common Clock Framework is that
clk_get() can return a NULL and the driver must interpret this as a valid
clock handle (for a dummy clock for example) that can be passed to further
clock operations, such as clk_prepare() or clk_enable().
This is why the driver should not reserve NULL as its own value meaning
that the clock is not present, but should rather check for IS_ERR
everywhere to find out whether the clock was acquired at probe.
Separate patch fixing this would be nice.
> Thank you for review :)
You're welcome. Thank you for your work on this series. :)
Best regards,
Tomasz
On Fri, Aug 9, 2013 at 1:37 AM, Cho KyongHo <[email protected]> wrote:
> On Fri, 09 Aug 2013 00:41:25 +0200, Tomasz Figa wrote:
>> Hi KyongHo,
>>
>> On Thursday 08 of August 2013 18:38:49 Cho KyongHo wrote:
>> > This commit adds device tree support for System MMU.
>> > This also include the following changes and enhancements:
>> >
>> > * use managed device helper functions.
>> > Simplyfies System MMU device driver.
>> >
>> > * use only a single clock descriptor.
>> > System MMU device descriptor is seperate if it is imposible to make
>> > a single clock descriptor to make a device descriptor for a group of
>> > System MMUs.
>> >
>> > * removed dbgname member from sysmmu_drvdata structure.
>> > debugging kernel message for a System MMU is distinguisheable with the
>> > name of device descroptors.
>>
>> Please put all these three changes in separate patches. This patch is hard
>> to review with all the changes mixed together...
Agreed.
>>
>
> Ok.
>
>> In addition, I believe this is the patch that should be adding device tree
>> binding documentation, not the 6/16 one, as this is where actually support
>> for this binding gets added to the kernel.
>
> Oh, I didn't know that devicetree binding description and implementation need
> to be in the same patch.
> I will do as you advised.
Actually, I prefer the binding docs be separate patches. The reason
being so we can get closer to having them in a separate repository.
Also, then the binding can be acked separately from the kernel
implementation using the binding.
Rob
On Friday 09 of August 2013 08:28:09 Rob Herring wrote:
> On Fri, Aug 9, 2013 at 1:37 AM, Cho KyongHo <[email protected]>
wrote:
> > On Fri, 09 Aug 2013 00:41:25 +0200, Tomasz Figa wrote:
> >> Hi KyongHo,
> >>
> >> On Thursday 08 of August 2013 18:38:49 Cho KyongHo wrote:
> >> > This commit adds device tree support for System MMU.
> >> > This also include the following changes and enhancements:
> >> >
> >> > * use managed device helper functions.
> >> > Simplyfies System MMU device driver.
> >> >
> >> > * use only a single clock descriptor.
> >> > System MMU device descriptor is seperate if it is imposible to make
> >> > a single clock descriptor to make a device descriptor for a group of
> >> > System MMUs.
> >> >
> >> > * removed dbgname member from sysmmu_drvdata structure.
> >> > debugging kernel message for a System MMU is distinguisheable with
> >> > the
> >> > name of device descroptors.
> >>
> >> Please put all these three changes in separate patches. This patch is
> >> hard to review with all the changes mixed together...
>
> Agreed.
>
> > Ok.
> >
> >> In addition, I believe this is the patch that should be adding device
> >> tree binding documentation, not the 6/16 one, as this is where
> >> actually support for this binding gets added to the kernel.
> >
> > Oh, I didn't know that devicetree binding description and
> > implementation need to be in the same patch.
> > I will do as you advised.
>
> Actually, I prefer the binding docs be separate patches. The reason
> being so we can get closer to having them in a separate repository.
> Also, then the binding can be acked separately from the kernel
> implementation using the binding.
OK. This is a good point. So maybe let's fully define this order to be as
following (and have first best practice for the DT binding best practices
that need to be defined):
Bindings documentation should be added with a separate patch, before any
user of these bindings is introduced.
What do you think?
Best regards,
Tomasz
On Fri, 09 Aug 2013 10:30:11 +0200, Tomasz Figa wrote:
> On Friday 09 of August 2013 15:37:30 Cho KyongHo wrote:
> > On Fri, 09 Aug 2013 00:41:25 +0200, Tomasz Figa wrote:
> > > Hi KyongHo,
> > >
> > > On Thursday 08 of August 2013 18:38:49 Cho KyongHo wrote:
> > > > This commit adds device tree support for System MMU.
> > > > This also include the following changes and enhancements:
> > > >
> > > > * use managed device helper functions.
> > > > Simplyfies System MMU device driver.
> > > >
> > > > * use only a single clock descriptor.
> > > > System MMU device descriptor is seperate if it is imposible to make
> > > > a single clock descriptor to make a device descriptor for a group of
> > > > System MMUs.
> > > >
> > > > * removed dbgname member from sysmmu_drvdata structure.
> > > > debugging kernel message for a System MMU is distinguisheable with
> > > > the
> > > > name of device descroptors.
> > >
> > > Please put all these three changes in separate patches. This patch is
> > > hard to review with all the changes mixed together...
> >
> > Ok.
> >
> > > In addition, I believe this is the patch that should be adding device
> > > tree binding documentation, not the 6/16 one, as this is where
> > > actually support for this binding gets added to the kernel.
> >
> > Oh, I didn't know that devicetree binding description and implementation
> > need to be in the same patch.
> > I will do as you advised.
>
> Thanks. We are still missing a "best practices" document about device tree
> binding development, so it might not be clear.
>
I saw Rob Herring has different idea about the seperation of patches.
Let's talk about it more.
> > > > Signed-off-by: Cho KyongHo <[email protected]>
> > > > ---
> > > >
> > > > drivers/iommu/Kconfig | 5 +-
> > > > drivers/iommu/exynos-iommu.c | 186
> > > >
> > > > ++++++++++++++++------------------------- 2 files changed, 75
> > > > insertions(+), 116 deletions(-)
> > > >
> > > > diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> > > > index 820d85c..9ad45f1 100644
> > > > --- a/drivers/iommu/Kconfig
> > > > +++ b/drivers/iommu/Kconfig
> > > > @@ -168,16 +168,15 @@ config TEGRA_IOMMU_SMMU
> > > >
> > > > config EXYNOS_IOMMU
> > > >
> > > > bool "Exynos IOMMU Support"
> > > >
> > > > - depends on ARCH_EXYNOS && EXYNOS_DEV_SYSMMU
> > > > + depends on ARCH_EXYNOS
> > > >
> > > > select IOMMU_API
> > > >
> > > > + default n
> > > >
> > > > help
> > > >
> > > > Support for the IOMMU(System MMU) of Samsung Exynos application
> > > > processor family. This enables H/W multimedia accellerators to
> > >
> > > see
> > >
> > > > non-linear physical memory chunks as a linear memory in their
> > > > address spaces
> > > >
> > > > - If unsure, say N here.
> > > > -
> > > >
> > > > config EXYNOS_IOMMU_DEBUG
> > > >
> > > > bool "Debugging log for Exynos IOMMU"
> > > > depends on EXYNOS_IOMMU
> > > >
> > > > diff --git a/drivers/iommu/exynos-iommu.c
> > > > b/drivers/iommu/exynos-iommu.c index a318049..0ee73e8 100644
> > > > --- a/drivers/iommu/exynos-iommu.c
> > > > +++ b/drivers/iommu/exynos-iommu.c
> > > > @@ -26,6 +26,7 @@
> > > >
> > > > #include <linux/list.h>
> > > > #include <linux/memblock.h>
> > > > #include <linux/export.h>
> > > >
> > > > +#include <linux/of.h>
> > > >
> > > > #include <asm/cacheflush.h>
> > > > #include <asm/pgtable.h>
> > > >
> > > > @@ -170,15 +171,14 @@ struct sysmmu_drvdata {
> > > >
> > > > struct list_head node; /* entry of exynos_iommu_domain.clients */
> > > > 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];
> > > > + struct clk *clk;
> > > >
> > > > int activations;
> > > > rwlock_t lock;
> > > > struct iommu_domain *domain;
> > > > sysmmu_fault_handler_t fault_handler;
> > > > unsigned long pgtable;
> > > >
> > > > + void __iomem *sfrbases[0];
> > > >
> > > > };
> > > >
> > > > static bool set_sysmmu_active(struct sysmmu_drvdata *data)
> > > >
> > > > @@ -385,8 +385,8 @@ static irqreturn_t exynos_sysmmu_irq(int irq,
> > > > void
> > > > *dev_id) if (!ret && (itype != SYSMMU_FAULT_UNKNOWN))
> > > >
> > > > __raw_writel(1 << itype, data->sfrbases[i] +
> > >
> > > REG_INT_CLEAR);
> > >
> > > > else
> > > >
> > > > - dev_dbg(data->sysmmu, "(%s) %s is not handled.\n",
> > > > - data->dbgname, sysmmu_fault_name[itype]);
> > > > + dev_dbg(data->sysmmu, "%s is not handled.\n",
> > > > + sysmmu_fault_name[itype]);
> > > >
> > > > if (itype != SYSMMU_FAULT_UNKNOWN)
> > > >
> > > > sysmmu_unblock(data->sfrbases[i]);
> > > >
> > > > @@ -410,10 +410,8 @@ static bool __exynos_sysmmu_disable(struct
> > > > sysmmu_drvdata *data) for (i = 0; i < data->nsfrs; i++)
> > > >
> > > > __raw_writel(CTRL_DISABLE, data->sfrbases[i] +
> > >
> > > REG_MMU_CTRL);
> > >
> > > > - if (data->clk[1])
> > > > - clk_disable(data->clk[1]);
> > > > - if (data->clk[0])
> > > > - clk_disable(data->clk[0]);
> > > > + if (data->clk)
> > > > + clk_disable(data->clk);
> > > >
> > > > disabled = true;
> > > > data->pgtable = 0;
> > > >
> > > > @@ -422,10 +420,10 @@ finish:
> > > > write_unlock_irqrestore(&data->lock, flags);
> > > >
> > > > if (disabled)
> > > >
> > > > - dev_dbg(data->sysmmu, "(%s) Disabled\n", data->dbgname);
> > > > + dev_dbg(data->sysmmu, "Disabled\n");
> > > >
> > > > else
> > > >
> > > > - dev_dbg(data->sysmmu, "(%s) %d times left to be
> > >
> > > disabled\n",
> > >
> > > > - data->dbgname, data->activations);
> > > > + dev_dbg(data->sysmmu, "%d times left to be disabled\n",
> > > > + data->activations);
> > > >
> > > > return disabled;
> > > >
> > > > }
> > > >
> > > > @@ -452,14 +450,12 @@ static int __exynos_sysmmu_enable(struct
> > > > sysmmu_drvdata *data, ret = 1;
> > > >
> > > > }
> > > >
> > > > - dev_dbg(data->sysmmu, "(%s) Already enabled\n", data-
> > > >
> > > >dbgname);
> > > >
> > > > + dev_dbg(data->sysmmu, "Already enabled\n");
> > > >
> > > > 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;
> > > >
> > > > @@ -479,7 +475,7 @@ static int __exynos_sysmmu_enable(struct
> > > > sysmmu_drvdata *data,
> > > >
> > > > data->domain = domain;
> > > >
> > > > - dev_dbg(data->sysmmu, "(%s) Enabled\n", data->dbgname);
> > > > + dev_dbg(data->sysmmu, "Enabled\n");
> > > >
> > > > finish:
> > > > write_unlock_irqrestore(&data->lock, flags);
> > > >
> > > > @@ -495,7 +491,7 @@ int exynos_sysmmu_enable(struct device *dev,
> > > > unsigned long pgtable)
> > > >
> > > > ret = pm_runtime_get_sync(data->sysmmu);
> > > > if (ret < 0) {
> > > >
> > > > - dev_dbg(data->sysmmu, "(%s) Failed to enable\n", data-
> > > >
> > > >dbgname);
> > > >
> > > > + dev_dbg(data->sysmmu, "Failed to enable\n");
> > > >
> > > > return ret;
> > > >
> > > > }
> > > >
> > > > @@ -503,8 +499,8 @@ int exynos_sysmmu_enable(struct device *dev,
> > > > unsigned long pgtable) if (WARN_ON(ret < 0)) {
> > > >
> > > > pm_runtime_put(data->sysmmu);
> > > > dev_err(data->sysmmu,
> > > >
> > > > - "(%s) Already enabled with page table %#lx\n",
> > > > - data->dbgname, data->pgtable);
> > > > + "Already enabled with page table %#lx\n",
> > > > + data->pgtable);
> > > >
> > > > } else {
> > > >
> > > > data->dev = dev;
> > > >
> > > > }
> > > >
> > > > @@ -540,9 +536,7 @@ static void sysmmu_tlb_invalidate_entry(struct
> > > > device *dev, unsigned long iova) }
> > > >
> > > > }
> > > >
> > > > } else {
> > > >
> > > > - dev_dbg(data->sysmmu,
> > > > - "(%s) Disabled. Skipping invalidating TLB.\n",
> > > > - data->dbgname);
> > > > + dev_dbg(data->sysmmu, "Disabled. Skipping invalidating
> > >
> > > TLB.\n");
> > >
> > > > }
> > > >
> > > > read_unlock_irqrestore(&data->lock, flags);
> > > >
> > > > @@ -564,141 +558,107 @@ void exynos_sysmmu_tlb_invalidate(struct
> > > > device
> > > > *dev) }
> > > >
> > > > }
> > > >
> > > > } else {
> > > >
> > > > - dev_dbg(data->sysmmu,
> > > > - "(%s) Disabled. Skipping invalidating TLB.\n",
> > > > - data->dbgname);
> > > > + dev_dbg(data->sysmmu, "Disabled. Skipping invalidating
> > >
> > > TLB.\n");
> > >
> > > > }
> > > >
> > > > read_unlock_irqrestore(&data->lock, flags);
> > > >
> > > > }
> > > >
> > > > -static int exynos_sysmmu_probe(struct platform_device *pdev)
> > > > +static int __init exynos_sysmmu_probe(struct platform_device *pdev)
> > > >
> > > > {
> > > >
> > > > int i, ret;
> > > >
> > > > - struct device *dev;
> > > > + struct device *dev = &pdev->dev;
> > > >
> > > > struct sysmmu_drvdata *data;
> > > >
> > > > - dev = &pdev->dev;
> > > > -
> > > > - data = kzalloc(sizeof(*data), GFP_KERNEL);
> > > > - if (!data) {
> > > > - dev_dbg(dev, "Not enough memory\n");
> > > > - ret = -ENOMEM;
> > > > - goto err_alloc;
> > > > + if (pdev->num_resources == 0) {
> > > > + dev_err(dev, "No System MMU resource defined\n");
> > > > + return -ENODEV;
> > > >
> > > > }
> > > >
> > > > - ret = dev_set_drvdata(dev, data);
> > > > - if (ret) {
> > > > - dev_dbg(dev, "Unabled to initialize driver data\n");
> > > > - goto err_init;
> > > > + data = devm_kzalloc(dev,
> > > > + sizeof(*data) +
> > > > + sizeof(*data->sfrbases) * (pdev->num_resources /
> > >
> > > 2),
> > >
> > > > + GFP_KERNEL);
> > > > + if (!data) {
> > > > + dev_err(dev, "Not enough memory for initialization\n");
> > >
> > > nit: This message is not necessary, as kmalloc() already prints a
> > > message if allocation error occurs.
> >
> > Ok.
> >
> > > > + return -ENOMEM;
> > > >
> > > > }
> > > >
> > > > data->nsfrs = pdev->num_resources / 2;
> > >
> > > As Marek and Bart already mentioned before, please (oh please, really
> > > please) finally remove this brokenness from this driver.
> > >
> > > A device driver should not do this kind of abstraction, emulating one
> > > instance of particular IP from multiple instances. This is a task for
> > > the IOMMU core and this is already supported by IOMMU domains, which
> > > is the right way to achieve a single virtual address space for
> > > multiple IOMMUs.
> > Let me show some example.
> > FIMD in Exynos5250 has a single System MMU.
> > FIMD in Exynos5420 has 2 System MMUs.
> > I wanted to show the FIMD driver the same view of System MMU.
> > Otherwise, FIMD driver must aware of how the System MMUs are connected
> > with FIMD.
> >
> > If it is responsible for the device drivers of master H/Ws, System MMU
> > driver can be much more simpler which I also prefer :)
>
> Yes, it is responsibility of FIMD driver to add all its IOMMUs into the
> same domain or into any other configuration it wants.
>
> For example by some extension, the driver might want to have separate
> address spaces for each IOMMU and this would not be possible with current
> configuration of SysMMU driver.
>
Ok. I will post seperate series of patches for this issue later.
I think it is better to discuss because it is change in design.
> > > > - 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;
> > > > - }
> > > >
> > > > 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;
> > > > + dev_err(dev, "Unable to find IOMEM region\n");
> > > > + return -ENOENT;
> > > >
> > > > }
> > > >
> > > > - data->sfrbases[i] = ioremap(res->start,
> > >
> > > resource_size(res));
> > >
> > > > + data->sfrbases[i] = devm_request_and_ioremap(dev, res);
> > >
> > > devm_request_and_ioremap() is deprecated. Please use
> > > devm_ioremap_resource().
> >
> > Oh. Thanks.
> >
> > > > if (!data->sfrbases[i]) {
> > >
> > > In case of devm_ioremap_resource() remember to adjust this check to
> > > check for IS_ERR, not for NULL.
> >
> > Ok. Thanks.
> >
> > > > - dev_dbg(dev, "Unable to map IOMEM @ PA:%#x\n",
> > > > - res->start);
> > > > - ret = -ENOENT;
> > > > - goto err_res;
> > > > + dev_err(dev, "Unable to map IOMEM @ %#x\n", res-
> > > >
> > > >start);
> > >
> > > This message will not be needed with devm_ioremap_resource() as it
> > > already prints a message if an error occurs.
> >
> > Ok.
> >
> > > > + return -EBUSY;
> > > >
> > > > }
> > > >
> > > > }
> > > >
> > > > 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;
> > > > + int irq;
> > > > +
> > > > + irq = platform_get_irq(pdev, i);
> > > > + if (irq <= 0) {
> > > > + dev_err(dev, "Unable to find IRQ resource\n");
> > > > + return -ENOENT;
> > > >
> > > > }
> > > >
> > > > - ret = request_irq(ret, exynos_sysmmu_irq, 0,
> > > > - dev_name(dev), data);
> > > > + ret = devm_request_irq(dev, irq, exynos_sysmmu_irq,
> > > > + 0, dev_name(dev), data);
> > > >
> > > > if (ret) {
> > > >
> > > > - dev_dbg(dev, "Unabled to register interrupt
> > >
> > > handler\n");
> > >
> > > > - goto err_irq;
> > > > + dev_err(dev, "Unable to register handler to irq
> > >
> > > %d\n",
> > >
> > > > + irq);
> > > > + return ret;
> > > >
> > > > }
> > > >
> > > > }
> > > >
> > > > - 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 */;
> > > > + pm_runtime_enable(dev);
> > > >
> > > > - 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;
> > > > - dev_dbg(dev, "No clock descriptor registered\n");
> > > > - }
> > > > + __set_fault_handler(data, &default_fault_handler);
> > > >
> > > > - if (data->clk[0] && deli) {
> > > > - *deli = ',';
> > > > - data->clk[1] = clk_get(dev, deli + 1);
> > > > - if (IS_ERR(data->clk[1]))
> > > > - data->clk[1] = NULL;
> > > > - }
> > > > + data->sysmmu = dev;
> > > > + data->clk = devm_clk_get(dev, "sysmmu");
> > > > + if (IS_ERR(data->clk)) {
> > > > + dev_info(dev, "No gate clock found!\n");
> > > > + data->clk = NULL;
> > >
> > > Just a note: In common clock framework NULL is a valid value for a
> > > struct clk *, which should not be interpreted as a pointer, but as an
> > > opaque cookie. This is material for separate patch, but this should
> > > be fixed all around this driver so data->clk is for IS_ERR instead.
> >
> > Error returned by devm_clk_get("sysmmu") is not an error actually.
> > More specifically, -ENOENT returned by devm_clk_get("sysmmu") is not an
> > error. It just means that a System MMU does not have to gating its
> > clock. However, it is required to check if the error retruned is
> > -ENOENT.
>
> I agree with this. Since the clock is optional for this driver, the -
> ENOENT error case is not an error for it.
>
> > data->clk must be NULL if 'sysmmu' clock is not specified for a System
> > MMU because clk_enable(data->clk) is called regardless of the value in
> > data->clk.
>
> Still, the return value convention for Common Clock Framework is that
> clk_get() can return a NULL and the driver must interpret this as a valid
> clock handle (for a dummy clock for example) that can be passed to further
> clock operations, such as clk_prepare() or clk_enable().
>
> This is why the driver should not reserve NULL as its own value meaning
> that the clock is not present, but should rather check for IS_ERR
> everywhere to find out whether the clock was acquired at probe.
>
> Separate patch fixing this would be nice.
>
Ok. I understand that it must not be assumed that NULL as the arguement
of clk_enable()/prepare() is ignored.
I will change this patch not to call clk_enable/disable
if IS_ERR(data->clk) is true.
Thank you.
> > Thank you for review :)
>
> You're welcome. Thank you for your work on this series. :)
>
> Best regards,
> Tomasz
>