[PATCH v9 07/16] iommu/exynos: support for device tree

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.

see

REG_INT_CLEAR);

REG_MMU_CTRL);

disabled\n",

TLB.\n");

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),
 	}
 };