[PATCH 0/7] Fix omap-iommu probe and convert to DT for 3.14

[PATCH 0/7] Fix omap-iommu probe and convert to DT for 3.14

[Florian Vaussard]

OMAP2+ is heading towards a full device tree boot for 3.14. Currently,
the iommu used by the OMAP3 camera subsystem is not yet converted. It
cannot be probed as necessary data are only passed through device tree.

Patches 1 and 2 are small fixes for problems encountered while developing
this series.

Patches 3 to 5 add the device tree logic to omap-iommu, and complete iommu
data in omap3.dtsi. Patches 6 and 7 remove unused iommu hwmod data and
platform code from OMAP2+.

This was tested on Overo (OMAP36xx) with an MT9V032 sensor connected
to the isp interface. The full testing tree can be found here [2] (not
safe for merging).

Patches are based on 3.13-rc3. OMAP-related patches are based on Tony's
omap-for-v3.14/omap3-board-removal branch [1].

Regards,

Florian

[1] git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git
      omap-for-v3.14/omap3-board-removal
[2] git@github.com:vaussard/linux.git overo-for-3.14/iommu/dt

Florian Vaussard (7):
  iommu/omap: Do bus_set_iommu() only if probe() succeeds
  iommu/omap: omap_iommu_attach() should return ENODEV, not NULL
  iommu/omap: Convert to devicetree
  iommu/omap: Allow enable/disable even without pdata
  ARM: dts: Complete data for isp iommu
  ARM: OMAP2+: Remove legacy data from hwmod for omap3 isp iommu
  ARM: OMAP2+: Remove platform-specific omap-iommu

 .../devicetree/bindings/iommu/ti,omap-iommu.txt    |  19 ++
 arch/arm/boot/dts/omap3.dtsi                       |   4 +-
 arch/arm/mach-omap2/Makefile                       |   3 -
 arch/arm/mach-omap2/omap-iommu.c                   |  74 ------
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c         |   8 -
 drivers/iommu/omap-iommu.c                         | 247 +++++++++++----------
 6 files changed, 156 insertions(+), 199 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iommu/ti,omap-iommu.txt
 delete mode 100644 arch/arm/mach-omap2/omap-iommu.c

-- 
1.8.1.2

[PATCH 1/7] iommu/omap: Do bus_set_iommu() only if probe() succeeds

[Florian Vaussard]

Currently, bus_set_iommu() is done in omap_iommu_init(). However,
omap_iommu_probe() can fail in a number of ways, leaving the platform
bus with a dangling reference to a non-initialized iommu. Perform
bus_set_iommu() only if omap_iommu_probe() succeed.

Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
---
 drivers/iommu/omap-iommu.c | 207 +++++++++++++++++++++++----------------------
 1 file changed, 104 insertions(+), 103 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index bcd78a7..ee83bcc 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -930,107 +930,6 @@ static void omap_iommu_detach(struct omap_iommu *obj)
 	dev_dbg(obj->dev, "%s: %s\n", __func__, obj->name);
 }
 
-/*
- *	OMAP Device MMU(IOMMU) detection
- */
-static int omap_iommu_probe(struct platform_device *pdev)
-{
-	int err = -ENODEV;
-	int irq;
-	struct omap_iommu *obj;
-	struct resource *res;
-	struct iommu_platform_data *pdata = pdev->dev.platform_data;
-
-	obj = kzalloc(sizeof(*obj) + MMU_REG_SIZE, GFP_KERNEL);
-	if (!obj)
-		return -ENOMEM;
-
-	obj->nr_tlb_entries = pdata->nr_tlb_entries;
-	obj->name = pdata->name;
-	obj->dev = &pdev->dev;
-	obj->ctx = (void *)obj + sizeof(*obj);
-	obj->da_start = pdata->da_start;
-	obj->da_end = pdata->da_end;
-
-	spin_lock_init(&obj->iommu_lock);
-	mutex_init(&obj->mmap_lock);
-	spin_lock_init(&obj->page_table_lock);
-	INIT_LIST_HEAD(&obj->mmap);
-
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	if (!res) {
-		err = -ENODEV;
-		goto err_mem;
-	}
-
-	res = request_mem_region(res->start, resource_size(res),
-				 dev_name(&pdev->dev));
-	if (!res) {
-		err = -EIO;
-		goto err_mem;
-	}
-
-	obj->regbase = ioremap(res->start, resource_size(res));
-	if (!obj->regbase) {
-		err = -ENOMEM;
-		goto err_ioremap;
-	}
-
-	irq = platform_get_irq(pdev, 0);
-	if (irq < 0) {
-		err = -ENODEV;
-		goto err_irq;
-	}
-	err = request_irq(irq, iommu_fault_handler, IRQF_SHARED,
-			  dev_name(&pdev->dev), obj);
-	if (err < 0)
-		goto err_irq;
-	platform_set_drvdata(pdev, obj);
-
-	pm_runtime_irq_safe(obj->dev);
-	pm_runtime_enable(obj->dev);
-
-	dev_info(&pdev->dev, "%s registered\n", obj->name);
-	return 0;
-
-err_irq:
-	iounmap(obj->regbase);
-err_ioremap:
-	release_mem_region(res->start, resource_size(res));
-err_mem:
-	kfree(obj);
-	return err;
-}
-
-static int omap_iommu_remove(struct platform_device *pdev)
-{
-	int irq;
-	struct resource *res;
-	struct omap_iommu *obj = platform_get_drvdata(pdev);
-
-	iopgtable_clear_entry_all(obj);
-
-	irq = platform_get_irq(pdev, 0);
-	free_irq(irq, obj);
-	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-	release_mem_region(res->start, resource_size(res));
-	iounmap(obj->regbase);
-
-	pm_runtime_disable(obj->dev);
-
-	dev_info(&pdev->dev, "%s removed\n", obj->name);
-	kfree(obj);
-	return 0;
-}
-
-static struct platform_driver omap_iommu_driver = {
-	.probe	= omap_iommu_probe,
-	.remove	= omap_iommu_remove,
-	.driver	= {
-		.name	= "omap-iommu",
-	},
-};
-
 static void iopte_cachep_ctor(void *iopte)
 {
 	clean_dcache_area(iopte, IOPTE_TABLE_SIZE);
@@ -1265,6 +1164,110 @@ static struct iommu_ops omap_iommu_ops = {
 	.pgsize_bitmap	= OMAP_IOMMU_PGSIZES,
 };
 
+/*
+ *	OMAP Device MMU(IOMMU) detection
+ */
+static int omap_iommu_probe(struct platform_device *pdev)
+{
+	int err = -ENODEV;
+	int irq;
+	struct omap_iommu *obj;
+	struct resource *res;
+	struct iommu_platform_data *pdata = pdev->dev.platform_data;
+
+	obj = kzalloc(sizeof(*obj) + MMU_REG_SIZE, GFP_KERNEL);
+	if (!obj)
+		return -ENOMEM;
+
+	obj->nr_tlb_entries = pdata->nr_tlb_entries;
+	obj->name = pdata->name;
+	obj->dev = &pdev->dev;
+	obj->ctx = (void *)obj + sizeof(*obj);
+	obj->da_start = pdata->da_start;
+	obj->da_end = pdata->da_end;
+
+	spin_lock_init(&obj->iommu_lock);
+	mutex_init(&obj->mmap_lock);
+	spin_lock_init(&obj->page_table_lock);
+	INIT_LIST_HEAD(&obj->mmap);
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		err = -ENODEV;
+		goto err_mem;
+	}
+
+	res = request_mem_region(res->start, resource_size(res),
+				 dev_name(&pdev->dev));
+	if (!res) {
+		err = -EIO;
+		goto err_mem;
+	}
+
+	obj->regbase = ioremap(res->start, resource_size(res));
+	if (!obj->regbase) {
+		err = -ENOMEM;
+		goto err_ioremap;
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	if (irq < 0) {
+		err = -ENODEV;
+		goto err_irq;
+	}
+
+	err = request_irq(irq, iommu_fault_handler, IRQF_SHARED,
+			  dev_name(&pdev->dev), obj);
+	if (err < 0)
+		goto err_irq;
+	platform_set_drvdata(pdev, obj);
+
+	bus_set_iommu(&platform_bus_type, &omap_iommu_ops);
+
+	pm_runtime_irq_safe(obj->dev);
+	pm_runtime_enable(obj->dev);
+
+	dev_info(&pdev->dev, "%s registered\n", obj->name);
+	return 0;
+
+err_irq:
+	iounmap(obj->regbase);
+err_ioremap:
+	release_mem_region(res->start, resource_size(res));
+err_mem:
+	kfree(obj);
+	return err;
+}
+
+static int omap_iommu_remove(struct platform_device *pdev)
+{
+	int irq;
+	struct resource *res;
+	struct omap_iommu *obj = platform_get_drvdata(pdev);
+
+	iopgtable_clear_entry_all(obj);
+
+	irq = platform_get_irq(pdev, 0);
+	free_irq(irq, obj);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	release_mem_region(res->start, resource_size(res));
+	iounmap(obj->regbase);
+
+	pm_runtime_disable(obj->dev);
+
+	dev_info(&pdev->dev, "%s removed\n", obj->name);
+	kfree(obj);
+	return 0;
+}
+
+static struct platform_driver omap_iommu_driver = {
+	.probe	= omap_iommu_probe,
+	.remove	= omap_iommu_remove,
+	.driver	= {
+		.name	= "omap-iommu",
+	},
+};
+
 static int __init omap_iommu_init(void)
 {
 	struct kmem_cache *p;
@@ -1277,8 +1280,6 @@ static int __init omap_iommu_init(void)
 		return -ENOMEM;
 	iopte_cachep = p;
 
-	bus_set_iommu(&platform_bus_type, &omap_iommu_ops);
-
 	return platform_driver_register(&omap_iommu_driver);
 }
 /* must be ready before omap3isp is probed */
-- 
1.8.1.2

Re: [PATCH 1/7] iommu/omap: Do bus_set_iommu() only if probe() succeeds

[Anna, Suman]

Hi Florian,

On 12/17/2013 06:53 AM, Florian Vaussard wrote:
> Currently, bus_set_iommu() is done in omap_iommu_init(). However,
> omap_iommu_probe() can fail in a number of ways, leaving the platform
> bus with a dangling reference to a non-initialized iommu. Perform
> bus_set_iommu() only if omap_iommu_probe() succeed.

Can you clarify a bit more on what kind of issues you were seeing 
specifically? In general, there can be multiple instances of the iommu, 
so setting it in probe may not be fixing whatever issue you were seeing. 
The current OMAP3 code has only the ISP MMU enabled, but there is also 
another one for the IVA MMU (currently not configured by default). 
Moving the bus_set_iommu to probe makes sense if only one iommu is 
present, so this patch may not be needed at all.

Also, the main change in this patch is moving the bus_set_iommu from
omap_iommu_init to omap_iommu_probe, so you should probably leave out 
moving the function. The omap_iommu_probe function would anyway need 
conversion to using devm_ functions.

regards
Suman

>
> Signed-off-by: Florian Vaussard <florian.vaussard-p8DiymsW2f8@public.gmane.org>
> ---
>   drivers/iommu/omap-iommu.c | 207 +++++++++++++++++++++++----------------------
>   1 file changed, 104 insertions(+), 103 deletions(-)
>
> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> index bcd78a7..ee83bcc 100644
> --- a/drivers/iommu/omap-iommu.c
> +++ b/drivers/iommu/omap-iommu.c
> @@ -930,107 +930,6 @@ static void omap_iommu_detach(struct omap_iommu *obj)
>   	dev_dbg(obj->dev, "%s: %s\n", __func__, obj->name);
>   }
>
> -/*
> - *	OMAP Device MMU(IOMMU) detection
> - */
> -static int omap_iommu_probe(struct platform_device *pdev)
> -{
> -	int err = -ENODEV;
> -	int irq;
> -	struct omap_iommu *obj;
> -	struct resource *res;
> -	struct iommu_platform_data *pdata = pdev->dev.platform_data;
> -
> -	obj = kzalloc(sizeof(*obj) + MMU_REG_SIZE, GFP_KERNEL);
> -	if (!obj)
> -		return -ENOMEM;
> -
> -	obj->nr_tlb_entries = pdata->nr_tlb_entries;
> -	obj->name = pdata->name;
> -	obj->dev = &pdev->dev;
> -	obj->ctx = (void *)obj + sizeof(*obj);
> -	obj->da_start = pdata->da_start;
> -	obj->da_end = pdata->da_end;
> -
> -	spin_lock_init(&obj->iommu_lock);
> -	mutex_init(&obj->mmap_lock);
> -	spin_lock_init(&obj->page_table_lock);
> -	INIT_LIST_HEAD(&obj->mmap);
> -
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!res) {
> -		err = -ENODEV;
> -		goto err_mem;
> -	}
> -
> -	res = request_mem_region(res->start, resource_size(res),
> -				 dev_name(&pdev->dev));
> -	if (!res) {
> -		err = -EIO;
> -		goto err_mem;
> -	}
> -
> -	obj->regbase = ioremap(res->start, resource_size(res));
> -	if (!obj->regbase) {
> -		err = -ENOMEM;
> -		goto err_ioremap;
> -	}
> -
> -	irq = platform_get_irq(pdev, 0);
> -	if (irq < 0) {
> -		err = -ENODEV;
> -		goto err_irq;
> -	}
> -	err = request_irq(irq, iommu_fault_handler, IRQF_SHARED,
> -			  dev_name(&pdev->dev), obj);
> -	if (err < 0)
> -		goto err_irq;
> -	platform_set_drvdata(pdev, obj);
> -
> -	pm_runtime_irq_safe(obj->dev);
> -	pm_runtime_enable(obj->dev);
> -
> -	dev_info(&pdev->dev, "%s registered\n", obj->name);
> -	return 0;
> -
> -err_irq:
> -	iounmap(obj->regbase);
> -err_ioremap:
> -	release_mem_region(res->start, resource_size(res));
> -err_mem:
> -	kfree(obj);
> -	return err;
> -}
> -
> -static int omap_iommu_remove(struct platform_device *pdev)
> -{
> -	int irq;
> -	struct resource *res;
> -	struct omap_iommu *obj = platform_get_drvdata(pdev);
> -
> -	iopgtable_clear_entry_all(obj);
> -
> -	irq = platform_get_irq(pdev, 0);
> -	free_irq(irq, obj);
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	release_mem_region(res->start, resource_size(res));
> -	iounmap(obj->regbase);
> -
> -	pm_runtime_disable(obj->dev);
> -
> -	dev_info(&pdev->dev, "%s removed\n", obj->name);
> -	kfree(obj);
> -	return 0;
> -}
> -
> -static struct platform_driver omap_iommu_driver = {
> -	.probe	= omap_iommu_probe,
> -	.remove	= omap_iommu_remove,
> -	.driver	= {
> -		.name	= "omap-iommu",
> -	},
> -};
> -
>   static void iopte_cachep_ctor(void *iopte)
>   {
>   	clean_dcache_area(iopte, IOPTE_TABLE_SIZE);
> @@ -1265,6 +1164,110 @@ static struct iommu_ops omap_iommu_ops = {
>   	.pgsize_bitmap	= OMAP_IOMMU_PGSIZES,
>   };
>
> +/*
> + *	OMAP Device MMU(IOMMU) detection
> + */
> +static int omap_iommu_probe(struct platform_device *pdev)
> +{
> +	int err = -ENODEV;
> +	int irq;
> +	struct omap_iommu *obj;
> +	struct resource *res;
> +	struct iommu_platform_data *pdata = pdev->dev.platform_data;
> +
> +	obj = kzalloc(sizeof(*obj) + MMU_REG_SIZE, GFP_KERNEL);
> +	if (!obj)
> +		return -ENOMEM;
> +
> +	obj->nr_tlb_entries = pdata->nr_tlb_entries;
> +	obj->name = pdata->name;
> +	obj->dev = &pdev->dev;
> +	obj->ctx = (void *)obj + sizeof(*obj);
> +	obj->da_start = pdata->da_start;
> +	obj->da_end = pdata->da_end;
> +
> +	spin_lock_init(&obj->iommu_lock);
> +	mutex_init(&obj->mmap_lock);
> +	spin_lock_init(&obj->page_table_lock);
> +	INIT_LIST_HEAD(&obj->mmap);
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		err = -ENODEV;
> +		goto err_mem;
> +	}
> +
> +	res = request_mem_region(res->start, resource_size(res),
> +				 dev_name(&pdev->dev));
> +	if (!res) {
> +		err = -EIO;
> +		goto err_mem;
> +	}
> +
> +	obj->regbase = ioremap(res->start, resource_size(res));
> +	if (!obj->regbase) {
> +		err = -ENOMEM;
> +		goto err_ioremap;
> +	}
> +
> +	irq = platform_get_irq(pdev, 0);
> +	if (irq < 0) {
> +		err = -ENODEV;
> +		goto err_irq;
> +	}
> +
> +	err = request_irq(irq, iommu_fault_handler, IRQF_SHARED,
> +			  dev_name(&pdev->dev), obj);
> +	if (err < 0)
> +		goto err_irq;
> +	platform_set_drvdata(pdev, obj);
> +
> +	bus_set_iommu(&platform_bus_type, &omap_iommu_ops);
> +
> +	pm_runtime_irq_safe(obj->dev);
> +	pm_runtime_enable(obj->dev);
> +
> +	dev_info(&pdev->dev, "%s registered\n", obj->name);
> +	return 0;
> +
> +err_irq:
> +	iounmap(obj->regbase);
> +err_ioremap:
> +	release_mem_region(res->start, resource_size(res));
> +err_mem:
> +	kfree(obj);
> +	return err;
> +}
> +
> +static int omap_iommu_remove(struct platform_device *pdev)
> +{
> +	int irq;
> +	struct resource *res;
> +	struct omap_iommu *obj = platform_get_drvdata(pdev);
> +
> +	iopgtable_clear_entry_all(obj);
> +
> +	irq = platform_get_irq(pdev, 0);
> +	free_irq(irq, obj);
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	release_mem_region(res->start, resource_size(res));
> +	iounmap(obj->regbase);
> +
> +	pm_runtime_disable(obj->dev);
> +
> +	dev_info(&pdev->dev, "%s removed\n", obj->name);
> +	kfree(obj);
> +	return 0;
> +}
> +
> +static struct platform_driver omap_iommu_driver = {
> +	.probe	= omap_iommu_probe,
> +	.remove	= omap_iommu_remove,
> +	.driver	= {
> +		.name	= "omap-iommu",
> +	},
> +};
> +
>   static int __init omap_iommu_init(void)
>   {
>   	struct kmem_cache *p;
> @@ -1277,8 +1280,6 @@ static int __init omap_iommu_init(void)
>   		return -ENOMEM;
>   	iopte_cachep = p;
>
> -	bus_set_iommu(&platform_bus_type, &omap_iommu_ops);
> -
>   	return platform_driver_register(&omap_iommu_driver);
>   }
>   /* must be ready before omap3isp is probed */
>

Re: [PATCH 1/7] iommu/omap: Do bus_set_iommu() only if probe() succeeds

[Florian Vaussard]

Hi Suman,

On 12/23/2013 08:02 PM, Anna, Suman wrote:
> Hi Florian,
> 
> On 12/17/2013 06:53 AM, Florian Vaussard wrote:
>> Currently, bus_set_iommu() is done in omap_iommu_init(). However,
>> omap_iommu_probe() can fail in a number of ways, leaving the platform
>> bus with a dangling reference to a non-initialized iommu. Perform
>> bus_set_iommu() only if omap_iommu_probe() succeed.
> 
> Can you clarify a bit more on what kind of issues you were seeing
> specifically? In general, there can be multiple instances of the iommu,
> so setting it in probe may not be fixing whatever issue you were seeing.
> The current OMAP3 code has only the ISP MMU enabled, but there is also
> another one for the IVA MMU (currently not configured by default).
> Moving the bus_set_iommu to probe makes sense if only one iommu is
> present, so this patch may not be needed at all.
> 

If omap_iommu_probe() fails, the init will have called bus_set_iommu()
anyways. Thus, when a driver request the iommu by calling
iommu_domain_alloc(), it will succeed (but iommu_attach_device() will
fail if I remember). Leaving a driver with a dangling reference to
a phantom iommu is not good IMHO. It will lead to strange behaviours
one day or another.

As for the multiple iommu case, as I do not think it is currently
possible, as bus_type (in this case &platform_bus_type) has only
one struct iommu_ops. bus_set_iommu() will return EBUSY on the
second call. Am I missing something?

> Also, the main change in this patch is moving the bus_set_iommu from
> omap_iommu_init to omap_iommu_probe, so you should probably leave out
> moving the function. The omap_iommu_probe function would anyway need
> conversion to using devm_ functions.
> 

This was my first try also, but as bus_set_iommu() needs omap_iommu_ops
(itself depending on several functions), its call must come after the
declaration of omap_iommu_ops. Thus I moved omap_iommu_probe() after
the declaration of omap_iommu_ops. But I can probably use a forward
declaration for omap_iommu_ops, this would be better.

Indeed, we can also convert to devm_.

Regards,

Florian

Re: [PATCH 1/7] iommu/omap: Do bus_set_iommu() only if probe() succeeds

[Anna, Suman]

Hi Florian,

>>
>> On 12/17/2013 06:53 AM, Florian Vaussard wrote:
>>> Currently, bus_set_iommu() is done in omap_iommu_init(). However,
>>> omap_iommu_probe() can fail in a number of ways, leaving the platform
>>> bus with a dangling reference to a non-initialized iommu. Perform
>>> bus_set_iommu() only if omap_iommu_probe() succeed.
>>
>> Can you clarify a bit more on what kind of issues you were seeing
>> specifically? In general, there can be multiple instances of the iommu,
>> so setting it in probe may not be fixing whatever issue you were seeing.
>> The current OMAP3 code has only the ISP MMU enabled, but there is also
>> another one for the IVA MMU (currently not configured by default).
>> Moving the bus_set_iommu to probe makes sense if only one iommu is
>> present, so this patch may not be needed at all.
>>
>
> If omap_iommu_probe() fails, the init will have called bus_set_iommu()
> anyways. Thus, when a driver request the iommu by calling
> iommu_domain_alloc(), it will succeed (but iommu_attach_device() will
> fail if I remember).

Yeah, thats the behavior I expected anyway.

> Leaving a driver with a dangling reference to
> a phantom iommu is not good IMHO. It will lead to strange behaviours
> one day or another.
>
> As for the multiple iommu case, as I do not think it is currently
> possible, as bus_type (in this case &platform_bus_type) has only
> one struct iommu_ops. bus_set_iommu() will return EBUSY on the
> second call. Am I missing something?

What I meant was the problem you cited will still exist, say if ISP MMU 
probe failed, but the IVA MMU probe succeeded. The bus_set_iommu() can 
only be called once anyway, so moving it from init to probe would not 
help much.

regards
Suman

>
>> Also, the main change in this patch is moving the bus_set_iommu from
>> omap_iommu_init to omap_iommu_probe, so you should probably leave out
>> moving the function. The omap_iommu_probe function would anyway need
>> conversion to using devm_ functions.
>>
>
> This was my first try also, but as bus_set_iommu() needs omap_iommu_ops
> (itself depending on several functions), its call must come after the
> declaration of omap_iommu_ops. Thus I moved omap_iommu_probe() after
> the declaration of omap_iommu_ops. But I can probably use a forward
> declaration for omap_iommu_ops, this would be better.
>
> Indeed, we can also convert to devm_.
>
> Regards,
>
> Florian
>

Re: [PATCH 1/7] iommu/omap: Do bus_set_iommu() only if probe() succeeds

[Florian Vaussard]

Hi Suman,

So back to this...

On 12/24/2013 12:35 AM, Anna, Suman wrote:
> Hi Florian,
> 

[...]

>>
>> If omap_iommu_probe() fails, the init will have called bus_set_iommu()
>> anyways. Thus, when a driver request the iommu by calling
>> iommu_domain_alloc(), it will succeed (but iommu_attach_device() will
>> fail if I remember).
> 
> Yeah, thats the behavior I expected anyway.
> 
>> Leaving a driver with a dangling reference to
>> a phantom iommu is not good IMHO. It will lead to strange behaviours
>> one day or another.
>>
>> As for the multiple iommu case, as I do not think it is currently
>> possible, as bus_type (in this case &platform_bus_type) has only
>> one struct iommu_ops. bus_set_iommu() will return EBUSY on the
>> second call. Am I missing something?
> 
> What I meant was the problem you cited will still exist, say if ISP MMU
> probe failed, but the IVA MMU probe succeeded. The bus_set_iommu() can
> only be called once anyway, so moving it from init to probe would not
> help much.
> 

Ok I see your point. Similar IPs share the same ops, but with different
omap_iommu_arch_data, even if currently we only have one registered
IOMMU for OMAP3.

So I will drop this patch.

Regards,
Florian

[PATCH 2/7] iommu/omap: omap_iommu_attach() should return ENODEV, not NULL

[Florian Vaussard]

omap_iommu_attach() returns NULL or ERR_PTR in case of error, but
omap_iommu_attach_dev() only checks for IS_ERR. Thus a NULL return value (in
case driver_find_device fails) will cause the kernel to panic when
omap_iommu_attach_dev() dereferences the pointer.

In such case, omap_iommu_attach() should return ENODEV, not NULL.

Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
---
 drivers/iommu/omap-iommu.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index ee83bcc..385bf5e 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -863,7 +863,7 @@ static int device_match_by_alias(struct device *dev, void *data)
  **/
 static struct omap_iommu *omap_iommu_attach(const char *name, u32 *iopgd)
 {
-	int err = -ENOMEM;
+	int err = -ENODEV;
 	struct device *dev;
 	struct omap_iommu *obj;
 
@@ -871,7 +871,7 @@ static struct omap_iommu *omap_iommu_attach(const char *name, u32 *iopgd)
 				(void *)name,
 				device_match_by_alias);
 	if (!dev)
-		return NULL;
+		return ERR_PTR(err);
 
 	obj = to_iommu(dev);
 
-- 
1.8.1.2

Re: [PATCH 2/7] iommu/omap: omap_iommu_attach() should return ENODEV, not NULL

[Anna, Suman]

On 12/17/2013 06:53 AM, Florian Vaussard wrote:
> omap_iommu_attach() returns NULL or ERR_PTR in case of error, but
> omap_iommu_attach_dev() only checks for IS_ERR. Thus a NULL return value (in
> case driver_find_device fails) will cause the kernel to panic when
> omap_iommu_attach_dev() dereferences the pointer.
>
> In such case, omap_iommu_attach() should return ENODEV, not NULL.
>
> Signed-off-by: Florian Vaussard <florian.vaussard-p8DiymsW2f8@public.gmane.org>

Acked-by: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>

> ---
>   drivers/iommu/omap-iommu.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> index ee83bcc..385bf5e 100644
> --- a/drivers/iommu/omap-iommu.c
> +++ b/drivers/iommu/omap-iommu.c
> @@ -863,7 +863,7 @@ static int device_match_by_alias(struct device *dev, void *data)
>    **/
>   static struct omap_iommu *omap_iommu_attach(const char *name, u32 *iopgd)
>   {
> -	int err = -ENOMEM;
> +	int err = -ENODEV;
>   	struct device *dev;
>   	struct omap_iommu *obj;
>
> @@ -871,7 +871,7 @@ static struct omap_iommu *omap_iommu_attach(const char *name, u32 *iopgd)
>   				(void *)name,
>   				device_match_by_alias);
>   	if (!dev)
> -		return NULL;
> +		return ERR_PTR(err);
>
>   	obj = to_iommu(dev);
>
>

[PATCH 3/7] iommu/omap: Convert to devicetree

[Florian Vaussard]

As OMAP2+ is moving to a full DT boot for 3.14, commit 7ce93f3
"ARM: OMAP2+: Fix more missing data for omap3.dtsi file" adds
basic DT bits. But the driver is not yet converted, so this will
not work and driver will not be probed. Convert it!

Apart from standard bindings, this patch uses 'dma-window' (already
used by Tegra SMMU) and adds a custom 'ti,#tlb-entries' binding.

Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
---
 .../devicetree/bindings/iommu/ti,omap-iommu.txt    | 19 ++++++++++++
 arch/arm/mach-omap2/omap-iommu.c                   |  5 +++
 drivers/iommu/omap-iommu.c                         | 36 +++++++++++++++++++---
 3 files changed, 55 insertions(+), 5 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/iommu/ti,omap-iommu.txt

diff --git a/Documentation/devicetree/bindings/iommu/ti,omap-iommu.txt b/Documentation/devicetree/bindings/iommu/ti,omap-iommu.txt
new file mode 100644
index 0000000..4e5027c
--- /dev/null
+++ b/Documentation/devicetree/bindings/iommu/ti,omap-iommu.txt
@@ -0,0 +1,19 @@
+OMAP3 IOMMU used by camera subsystem
+
+Required properties:
+- compatible : "ti,omap3-mmu-isp"
+- ti,hwmods : "mmu_isp"
+- reg : address space for the configuration registers
+- interrupts : interrupt line
+- dma-window : IOVA start address and length.
+- ti,#tlb-entries : number of entries in the translation look-aside buffer
+
+Example:
+	mmu_isp: mmu_isp@480bd400 {
+		compatible = "ti,omap3-mmu-isp";
+		ti,hwmods = "mmu_isp";
+		reg = <0x480bd400 0x80>;
+		interrupts = <8>;
+		dma-window = <0 0xfffff000>;
+		ti,#tlb-entries = <8>;
+	};
diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
index f6daae8..f1fab56 100644
--- a/arch/arm/mach-omap2/omap-iommu.c
+++ b/arch/arm/mach-omap2/omap-iommu.c
@@ -10,6 +10,7 @@
  * published by the Free Software Foundation.
  */
 
+#include <linux/of.h>
 #include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/err.h>
@@ -58,6 +59,10 @@ static int __init omap_iommu_dev_init(struct omap_hwmod *oh, void *unused)
 
 static int __init omap_iommu_init(void)
 {
+	/* If dtb is there, the devices will be created dynamically */
+	if (of_have_populated_dt())
+		return -ENODEV;
+
 	return omap_hwmod_for_each_by_class("mmu", omap_iommu_dev_init, NULL);
 }
 /* must be ready before omap3isp is probed */
diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 385bf5e..51efcc4 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -23,6 +23,9 @@
 #include <linux/spinlock.h>
 #include <linux/io.h>
 #include <linux/pm_runtime.h>
+#include <linux/of.h>
+#include <linux/of_iommu.h>
+#include <linux/of_irq.h>
 
 #include <asm/cacheflush.h>
 
@@ -1171,20 +1174,30 @@ static int omap_iommu_probe(struct platform_device *pdev)
 {
 	int err = -ENODEV;
 	int irq;
+	size_t len;
 	struct omap_iommu *obj;
 	struct resource *res;
 	struct iommu_platform_data *pdata = pdev->dev.platform_data;
+	struct device_node *of = pdev->dev.of_node;
 
 	obj = kzalloc(sizeof(*obj) + MMU_REG_SIZE, GFP_KERNEL);
 	if (!obj)
 		return -ENOMEM;
 
-	obj->nr_tlb_entries = pdata->nr_tlb_entries;
-	obj->name = pdata->name;
+	if (of) {
+		obj->name = of->name;
+		of_property_read_u32(of, "ti,#tlb-entries",
+				     &obj->nr_tlb_entries);
+		of_get_dma_window(of, NULL, 0, NULL, &obj->da_start, &len);
+		obj->da_end = obj->da_start + len;
+	} else {
+		obj->nr_tlb_entries = pdata->nr_tlb_entries;
+		obj->name = pdata->name;
+		obj->da_start = pdata->da_start;
+		obj->da_end = pdata->da_end;
+	}
 	obj->dev = &pdev->dev;
 	obj->ctx = (void *)obj + sizeof(*obj);
-	obj->da_start = pdata->da_start;
-	obj->da_end = pdata->da_end;
 
 	spin_lock_init(&obj->iommu_lock);
 	mutex_init(&obj->mmap_lock);
@@ -1210,7 +1223,11 @@ static int omap_iommu_probe(struct platform_device *pdev)
 		goto err_ioremap;
 	}
 
-	irq = platform_get_irq(pdev, 0);
+	if (of)
+		irq = irq_of_parse_and_map(of, 0);
+	else
+		irq = platform_get_irq(pdev, 0);
+
 	if (irq < 0) {
 		err = -ENODEV;
 		goto err_irq;
@@ -1260,11 +1277,20 @@ static int omap_iommu_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#if defined(CONFIG_OF)
+static struct of_device_id omap_iommu_of_match[] = {
+	{ .compatible = "ti,omap3-mmu-isp" },
+	{},
+};
+MODULE_DEVICE_TABLE(of, omap_iommu_of_match);
+#endif
+
 static struct platform_driver omap_iommu_driver = {
 	.probe	= omap_iommu_probe,
 	.remove	= omap_iommu_remove,
 	.driver	= {
 		.name	= "omap-iommu",
+		.of_match_table = omap_iommu_of_match,
 	},
 };
 
-- 
1.8.1.2

Re: [PATCH 3/7] iommu/omap: Convert to devicetree

[Anna, Suman]

Hi Florian,

On 12/17/2013 06:53 AM, Florian Vaussard wrote:
> As OMAP2+ is moving to a full DT boot for 3.14, commit 7ce93f3
> "ARM: OMAP2+: Fix more missing data for omap3.dtsi file" adds
> basic DT bits. But the driver is not yet converted, so this will
> not work and driver will not be probed. Convert it!
>
> Apart from standard bindings, this patch uses 'dma-window' (already
> used by Tegra SMMU) and adds a custom 'ti,#tlb-entries' binding.
>
> Signed-off-by: Florian Vaussard <florian.vaussard-p8DiymsW2f8@public.gmane.org>
> ---
>   .../devicetree/bindings/iommu/ti,omap-iommu.txt    | 19 ++++++++++++
>   arch/arm/mach-omap2/omap-iommu.c                   |  5 +++
>   drivers/iommu/omap-iommu.c                         | 36 +++++++++++++++++++---
>   3 files changed, 55 insertions(+), 5 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/iommu/ti,omap-iommu.txt
>
> diff --git a/Documentation/devicetree/bindings/iommu/ti,omap-iommu.txt b/Documentation/devicetree/bindings/iommu/ti,omap-iommu.txt
> new file mode 100644
> index 0000000..4e5027c
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/iommu/ti,omap-iommu.txt
> @@ -0,0 +1,19 @@
> +OMAP3 IOMMU used by camera subsystem

As I mentioned in comments on your cover-letter, the current binding is 
only limited to OMAP3 ISP, but the driver is common for all OMAP2+ SoCs. 
This binding an update to handle all SoCs.

To summarize the differences, OMAP2/3 has the same MMU register set for 
all remote processor MMUs (IVA/DSP/ISP) with the only difference being 
the number of TLBs supported (8 for ISP on OMAP2/3 and 32 for all other 
instances on all OMAP2+ SoCs). Depending on whether the compatibility 
property is defined for an SoC or for the processor, "ti,#tlb-entries" 
can be retrieved directly from the match data or made as an optional 
property, with the default value being 32.

OMAP4 and OMAP5 have some additional registers (MMU_FAULT_PC, 
MMU_FAULT_STATUS, MMU_GP_REG/DSPSS_MMU_GPR) on top of the OMAP2/OMAP3 
ones, and DRA7 a further superset of OMAP4/OMAP5. The only difference is 
the definition of the MMU_GPR register, which is different between the 
IPU and DSP processors, but present at the same offset, and dictates the 
validity of the MMU_FAULT_PC and MMU_FAULT_STATUS registers. The MMU_GPR 
register is also not defined on OMAP4430 ES1.0, but is present on all 
other newer SoCs.

> +
> +Required properties:
> +- compatible : "ti,omap3-mmu-isp"
My suggestion would be to use
	"ti,omap2-iommu" for OMAP2/OMAP3 MMUs with the optional 
"ti,#tlb-entries" to distinguish ISP MMU.
	"ti,omap4-iommu" for OMAP4/OMAP5 MMUs with an additional boolean 
property to distinguish the presence/functionality of the MMU_GPR register
	"ti,dra7-iommu" for DRA7 MMUs

Mark, Tony,
Do you have any other recommendations given the above summary?

> +- ti,hwmods : "mmu_isp"
Replace with general description

> +- reg : address space for the configuration registers
> +- interrupts : interrupt line
> +- dma-window : IOVA start address and length.
> +- ti,#tlb-entries : number of entries in the translation look-aside buffer
> +
> +Example:
> +	mmu_isp: mmu_isp@480bd400 {
> +		compatible = "ti,omap3-mmu-isp";
> +		ti,hwmods = "mmu_isp";
> +		reg = <0x480bd400 0x80>;
> +		interrupts = <8>;
> +		dma-window = <0 0xfffff000>;
> +		ti,#tlb-entries = <8>;
> +	};
> diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
> index f6daae8..f1fab56 100644
> --- a/arch/arm/mach-omap2/omap-iommu.c
> +++ b/arch/arm/mach-omap2/omap-iommu.c
> @@ -10,6 +10,7 @@
>    * published by the Free Software Foundation.
>    */
>
> +#include <linux/of.h>
>   #include <linux/module.h>
>   #include <linux/platform_device.h>
>   #include <linux/err.h>
> @@ -58,6 +59,10 @@ static int __init omap_iommu_dev_init(struct omap_hwmod *oh, void *unused)
>
>   static int __init omap_iommu_init(void)
>   {
> +	/* If dtb is there, the devices will be created dynamically */
> +	if (of_have_populated_dt())
> +		return -ENODEV;
> +
>   	return omap_hwmod_for_each_by_class("mmu", omap_iommu_dev_init, NULL);
>   }
>   /* must be ready before omap3isp is probed */
> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> index 385bf5e..51efcc4 100644
> --- a/drivers/iommu/omap-iommu.c
> +++ b/drivers/iommu/omap-iommu.c
> @@ -23,6 +23,9 @@
>   #include <linux/spinlock.h>
>   #include <linux/io.h>
>   #include <linux/pm_runtime.h>
> +#include <linux/of.h>
> +#include <linux/of_iommu.h>
> +#include <linux/of_irq.h>
>
>   #include <asm/cacheflush.h>
>
> @@ -1171,20 +1174,30 @@ static int omap_iommu_probe(struct platform_device *pdev)
>   {
>   	int err = -ENODEV;
>   	int irq;
> +	size_t len;
>   	struct omap_iommu *obj;
>   	struct resource *res;
>   	struct iommu_platform_data *pdata = pdev->dev.platform_data;
> +	struct device_node *of = pdev->dev.of_node;
>
>   	obj = kzalloc(sizeof(*obj) + MMU_REG_SIZE, GFP_KERNEL);
>   	if (!obj)
>   		return -ENOMEM;
>
> -	obj->nr_tlb_entries = pdata->nr_tlb_entries;
> -	obj->name = pdata->name;
> +	if (of) {
> +		obj->name = of->name;
> +		of_property_read_u32(of, "ti,#tlb-entries",
> +				     &obj->nr_tlb_entries);
> +		of_get_dma_window(of, NULL, 0, NULL, &obj->da_start, &len);

There is no error checking performed on both the of functions.

> +		obj->da_end = obj->da_start + len;
> +	} else {
> +		obj->nr_tlb_entries = pdata->nr_tlb_entries;
> +		obj->name = pdata->name;
> +		obj->da_start = pdata->da_start;
> +		obj->da_end = pdata->da_end;
> +	}
>   	obj->dev = &pdev->dev;
>   	obj->ctx = (void *)obj + sizeof(*obj);
> -	obj->da_start = pdata->da_start;
> -	obj->da_end = pdata->da_end;
>
>   	spin_lock_init(&obj->iommu_lock);
>   	mutex_init(&obj->mmap_lock);
> @@ -1210,7 +1223,11 @@ static int omap_iommu_probe(struct platform_device *pdev)
>   		goto err_ioremap;
>   	}
>
> -	irq = platform_get_irq(pdev, 0);
> +	if (of)
> +		irq = irq_of_parse_and_map(of, 0);
> +	else
> +		irq = platform_get_irq(pdev, 0);

The platform_get_irq should still work just fine, so this change can 
probably be left out. We can switch once the driver supports DT-only 
devices completely.

regards
Suman


> +
>   	if (irq < 0) {
>   		err = -ENODEV;
>   		goto err_irq;
> @@ -1260,11 +1277,20 @@ static int omap_iommu_remove(struct platform_device *pdev)
>   	return 0;
>   }
>
> +#if defined(CONFIG_OF)
> +static struct of_device_id omap_iommu_of_match[] = {
> +	{ .compatible = "ti,omap3-mmu-isp" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, omap_iommu_of_match);
> +#endif
> +
>   static struct platform_driver omap_iommu_driver = {
>   	.probe	= omap_iommu_probe,
>   	.remove	= omap_iommu_remove,
>   	.driver	= {
>   		.name	= "omap-iommu",
> +		.of_match_table = omap_iommu_of_match,
>   	},
>   };
>
>

Re: [PATCH 3/7] iommu/omap: Convert to devicetree

[Florian Vaussard]

Hi Suman,

On 12/23/2013 08:48 PM, Anna, Suman wrote:
> Hi Florian,
> 
> On 12/17/2013 06:53 AM, Florian Vaussard wrote:
>> As OMAP2+ is moving to a full DT boot for 3.14, commit 7ce93f3
>> "ARM: OMAP2+: Fix more missing data for omap3.dtsi file" adds
>> basic DT bits. But the driver is not yet converted, so this will
>> not work and driver will not be probed. Convert it!
>>
>> Apart from standard bindings, this patch uses 'dma-window' (already
>> used by Tegra SMMU) and adds a custom 'ti,#tlb-entries' binding.
>>
>> Signed-off-by: Florian Vaussard <florian.vaussard-p8DiymsW2f8@public.gmane.org>
>> ---
>>   .../devicetree/bindings/iommu/ti,omap-iommu.txt    | 19 ++++++++++++
>>   arch/arm/mach-omap2/omap-iommu.c                   |  5 +++
>>   drivers/iommu/omap-iommu.c                         | 36
>> +++++++++++++++++++---
>>   3 files changed, 55 insertions(+), 5 deletions(-)
>>   create mode 100644
>> Documentation/devicetree/bindings/iommu/ti,omap-iommu.txt
>>
>> diff --git a/Documentation/devicetree/bindings/iommu/ti,omap-iommu.txt
>> b/Documentation/devicetree/bindings/iommu/ti,omap-iommu.txt
>> new file mode 100644
>> index 0000000..4e5027c
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/iommu/ti,omap-iommu.txt
>> @@ -0,0 +1,19 @@
>> +OMAP3 IOMMU used by camera subsystem
> 
> As I mentioned in comments on your cover-letter, the current binding is
> only limited to OMAP3 ISP, but the driver is common for all OMAP2+ SoCs.
> This binding an update to handle all SoCs.
> 
> To summarize the differences, OMAP2/3 has the same MMU register set for
> all remote processor MMUs (IVA/DSP/ISP) with the only difference being
> the number of TLBs supported (8 for ISP on OMAP2/3 and 32 for all other
> instances on all OMAP2+ SoCs). Depending on whether the compatibility
> property is defined for an SoC or for the processor, "ti,#tlb-entries"
> can be retrieved directly from the match data or made as an optional
> property, with the default value being 32.
> 

It makes sense.

> OMAP4 and OMAP5 have some additional registers (MMU_FAULT_PC,
> MMU_FAULT_STATUS, MMU_GP_REG/DSPSS_MMU_GPR) on top of the OMAP2/OMAP3
> ones, and DRA7 a further superset of OMAP4/OMAP5. The only difference is
> the definition of the MMU_GPR register, which is different between the
> IPU and DSP processors, but present at the same offset, and dictates the
> validity of the MMU_FAULT_PC and MMU_FAULT_STATUS registers. The MMU_GPR
> register is also not defined on OMAP4430 ES1.0, but is present on all
> other newer SoCs.
> 

Thank you for this detailed comparison ! I do not know much about
OMAP4 and OMAP5. In the current implementation, I was unable to find
references to these specificities. Is this currently supported?

>> +
>> +Required properties:
>> +- compatible : "ti,omap3-mmu-isp"
> My suggestion would be to use
>     "ti,omap2-iommu" for OMAP2/OMAP3 MMUs with the optional
> "ti,#tlb-entries" to distinguish ISP MMU.
>     "ti,omap4-iommu" for OMAP4/OMAP5 MMUs with an additional boolean
> property to distinguish the presence/functionality of the MMU_GPR register
>     "ti,dra7-iommu" for DRA7 MMUs
> 
> Mark, Tony,
> Do you have any other recommendations given the above summary?
> 
>> +- ti,hwmods : "mmu_isp"
> Replace with general description
> 

You mean something like "Name of the hwmod associated to the IOMMU"?

>> +- reg : address space for the configuration registers
>> +- interrupts : interrupt line
>> +- dma-window : IOVA start address and length.
>> +- ti,#tlb-entries : number of entries in the translation look-aside
>> buffer
>> +
>> +Example:
>> +    mmu_isp: mmu_isp@480bd400 {
>> +        compatible = "ti,omap3-mmu-isp";
>> +        ti,hwmods = "mmu_isp";
>> +        reg = <0x480bd400 0x80>;
>> +        interrupts = <8>;
>> +        dma-window = <0 0xfffff000>;
>> +        ti,#tlb-entries = <8>;
>> +    };
>> diff --git a/arch/arm/mach-omap2/omap-iommu.c
>> b/arch/arm/mach-omap2/omap-iommu.c
>> index f6daae8..f1fab56 100644
>> --- a/arch/arm/mach-omap2/omap-iommu.c
>> +++ b/arch/arm/mach-omap2/omap-iommu.c
>> @@ -10,6 +10,7 @@
>>    * published by the Free Software Foundation.
>>    */
>>
>> +#include <linux/of.h>
>>   #include <linux/module.h>
>>   #include <linux/platform_device.h>
>>   #include <linux/err.h>
>> @@ -58,6 +59,10 @@ static int __init omap_iommu_dev_init(struct
>> omap_hwmod *oh, void *unused)
>>
>>   static int __init omap_iommu_init(void)
>>   {
>> +    /* If dtb is there, the devices will be created dynamically */
>> +    if (of_have_populated_dt())
>> +        return -ENODEV;
>> +
>>       return omap_hwmod_for_each_by_class("mmu", omap_iommu_dev_init,
>> NULL);
>>   }
>>   /* must be ready before omap3isp is probed */
>> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
>> index 385bf5e..51efcc4 100644
>> --- a/drivers/iommu/omap-iommu.c
>> +++ b/drivers/iommu/omap-iommu.c
>> @@ -23,6 +23,9 @@
>>   #include <linux/spinlock.h>
>>   #include <linux/io.h>
>>   #include <linux/pm_runtime.h>
>> +#include <linux/of.h>
>> +#include <linux/of_iommu.h>
>> +#include <linux/of_irq.h>
>>
>>   #include <asm/cacheflush.h>
>>
>> @@ -1171,20 +1174,30 @@ static int omap_iommu_probe(struct
>> platform_device *pdev)
>>   {
>>       int err = -ENODEV;
>>       int irq;
>> +    size_t len;
>>       struct omap_iommu *obj;
>>       struct resource *res;
>>       struct iommu_platform_data *pdata = pdev->dev.platform_data;
>> +    struct device_node *of = pdev->dev.of_node;
>>
>>       obj = kzalloc(sizeof(*obj) + MMU_REG_SIZE, GFP_KERNEL);
>>       if (!obj)
>>           return -ENOMEM;
>>
>> -    obj->nr_tlb_entries = pdata->nr_tlb_entries;
>> -    obj->name = pdata->name;
>> +    if (of) {
>> +        obj->name = of->name;
>> +        of_property_read_u32(of, "ti,#tlb-entries",
>> +                     &obj->nr_tlb_entries);
>> +        of_get_dma_window(of, NULL, 0, NULL, &obj->da_start, &len);
> 
> There is no error checking performed on both the of functions.
> 

Indeed.

>> +        obj->da_end = obj->da_start + len;
>> +    } else {
>> +        obj->nr_tlb_entries = pdata->nr_tlb_entries;
>> +        obj->name = pdata->name;
>> +        obj->da_start = pdata->da_start;
>> +        obj->da_end = pdata->da_end;
>> +    }
>>       obj->dev = &pdev->dev;
>>       obj->ctx = (void *)obj + sizeof(*obj);
>> -    obj->da_start = pdata->da_start;
>> -    obj->da_end = pdata->da_end;
>>
>>       spin_lock_init(&obj->iommu_lock);
>>       mutex_init(&obj->mmap_lock);
>> @@ -1210,7 +1223,11 @@ static int omap_iommu_probe(struct
>> platform_device *pdev)
>>           goto err_ioremap;
>>       }
>>
>> -    irq = platform_get_irq(pdev, 0);
>> +    if (of)
>> +        irq = irq_of_parse_and_map(of, 0);
>> +    else
>> +        irq = platform_get_irq(pdev, 0);
> 
> The platform_get_irq should still work just fine, so this change can
> probably be left out. We can switch once the driver supports DT-only
> devices completely.
> 

Ok I will test when back at my office.

Thank you!

Regards,
Florian

Re: [PATCH 3/7] iommu/omap: Convert to devicetree

[Anna, Suman]

Hi Florian,

 >>
>> On 12/17/2013 06:53 AM, Florian Vaussard wrote:
>>> As OMAP2+ is moving to a full DT boot for 3.14, commit 7ce93f3
>>> "ARM: OMAP2+: Fix more missing data for omap3.dtsi file" adds
>>> basic DT bits. But the driver is not yet converted, so this will
>>> not work and driver will not be probed. Convert it!
>>>
>>> Apart from standard bindings, this patch uses 'dma-window' (already
>>> used by Tegra SMMU) and adds a custom 'ti,#tlb-entries' binding.
>>>
>>> Signed-off-by: Florian Vaussard <florian.vaussard-p8DiymsW2f8@public.gmane.org>
>>> ---
>>>    .../devicetree/bindings/iommu/ti,omap-iommu.txt    | 19 ++++++++++++
>>>    arch/arm/mach-omap2/omap-iommu.c                   |  5 +++
>>>    drivers/iommu/omap-iommu.c                         | 36
>>> +++++++++++++++++++---
>>>    3 files changed, 55 insertions(+), 5 deletions(-)
>>>    create mode 100644
>>> Documentation/devicetree/bindings/iommu/ti,omap-iommu.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/iommu/ti,omap-iommu.txt
>>> b/Documentation/devicetree/bindings/iommu/ti,omap-iommu.txt
>>> new file mode 100644
>>> index 0000000..4e5027c
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/iommu/ti,omap-iommu.txt
>>> @@ -0,0 +1,19 @@
>>> +OMAP3 IOMMU used by camera subsystem
>>
>> As I mentioned in comments on your cover-letter, the current binding is
>> only limited to OMAP3 ISP, but the driver is common for all OMAP2+ SoCs.
>> This binding an update to handle all SoCs.
>>
>> To summarize the differences, OMAP2/3 has the same MMU register set for
>> all remote processor MMUs (IVA/DSP/ISP) with the only difference being
>> the number of TLBs supported (8 for ISP on OMAP2/3 and 32 for all other
>> instances on all OMAP2+ SoCs). Depending on whether the compatibility
>> property is defined for an SoC or for the processor, "ti,#tlb-entries"
>> can be retrieved directly from the match data or made as an optional
>> property, with the default value being 32.
>>
>
> It makes sense.
>
>> OMAP4 and OMAP5 have some additional registers (MMU_FAULT_PC,
>> MMU_FAULT_STATUS, MMU_GP_REG/DSPSS_MMU_GPR) on top of the OMAP2/OMAP3
>> ones, and DRA7 a further superset of OMAP4/OMAP5. The only difference is
>> the definition of the MMU_GPR register, which is different between the
>> IPU and DSP processors, but present at the same offset, and dictates the
>> validity of the MMU_FAULT_PC and MMU_FAULT_STATUS registers. The MMU_GPR
>> register is also not defined on OMAP4430 ES1.0, but is present on all
>> other newer SoCs.
>>
>
> Thank you for this detailed comparison ! I do not know much about
> OMAP4 and OMAP5. In the current implementation, I was unable to find
> references to these specificities. Is this currently supported?

OMAP4 iommu devices are instantiated today in legacy form, using the 
hwmod data. These are used by the remoteproc driver. OMAP5 data has not 
yet been added, as there is no point in adding it in legacy form at the 
moment.

>
>>> +
>>> +Required properties:
>>> +- compatible : "ti,omap3-mmu-isp"
>> My suggestion would be to use
>>      "ti,omap2-iommu" for OMAP2/OMAP3 MMUs with the optional
>> "ti,#tlb-entries" to distinguish ISP MMU.
>>      "ti,omap4-iommu" for OMAP4/OMAP5 MMUs with an additional boolean
>> property to distinguish the presence/functionality of the MMU_GPR register
>>      "ti,dra7-iommu" for DRA7 MMUs
>>
>> Mark, Tony,
>> Do you have any other recommendations given the above summary?
>>
>>> +- ti,hwmods : "mmu_isp"
>> Replace with general description
>>
>
> You mean something like "Name of the hwmod associated to the IOMMU"?

Yes.

regards
Suman

[snip]

Re: [PATCH 3/7] iommu/omap: Convert to devicetree

[Laurent Pinchart]

Hi Florian,

Thank you for the patch.

On Tuesday 17 December 2013 13:53:34 Florian Vaussard wrote:
> As OMAP2+ is moving to a full DT boot for 3.14, commit 7ce93f3
> "ARM: OMAP2+: Fix more missing data for omap3.dtsi file" adds
> basic DT bits. But the driver is not yet converted, so this will
> not work and driver will not be probed. Convert it!
> 
> Apart from standard bindings, this patch uses 'dma-window' (already
> used by Tegra SMMU) and adds a custom 'ti,#tlb-entries' binding.
> 
> Signed-off-by: Florian Vaussard <florian.vaussard-p8DiymsW2f8@public.gmane.org>
> ---
>  .../devicetree/bindings/iommu/ti,omap-iommu.txt    | 19 ++++++++++++
>  arch/arm/mach-omap2/omap-iommu.c                   |  5 +++
>  drivers/iommu/omap-iommu.c                         | 36 ++++++++++++++++---
>  3 files changed, 55 insertions(+), 5 deletions(-)
>  create mode 100644
> Documentation/devicetree/bindings/iommu/ti,omap-iommu.txt

[snip]

> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> index 385bf5e..51efcc4 100644
> --- a/drivers/iommu/omap-iommu.c
> +++ b/drivers/iommu/omap-iommu.c

[snip]

> @@ -1260,11 +1277,20 @@ static int omap_iommu_remove(struct platform_device
> *pdev) return 0;
>  }
> 
> +#if defined(CONFIG_OF)
> +static struct of_device_id omap_iommu_of_match[] = {
> +	{ .compatible = "ti,omap3-mmu-isp" },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, omap_iommu_of_match);
> +#endif
> +
>  static struct platform_driver omap_iommu_driver = {
>  	.probe	= omap_iommu_probe,
>  	.remove	= omap_iommu_remove,
>  	.driver	= {
>  		.name	= "omap-iommu",
> +		.of_match_table = omap_iommu_of_match,

If CONFIG_OF isn't defined (pretty unlikely I agree, but a possibility you 
seem to be prepared for nonetheless given the above #if), this will fail to 
compile.

>  	},
>  };

-- 
Regards,

Laurent Pinchart

Re: [PATCH 3/7] iommu/omap: Convert to devicetree

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 476 bytes --]

Hi,

On Thu, Jan 02, 2014 at 01:13:42AM +0100, Laurent Pinchart wrote:
> > +		.of_match_table = omap_iommu_of_match,
> 
> If CONFIG_OF isn't defined (pretty unlikely I agree, but a possibility you 
> seem to be prepared for nonetheless given the above #if), this will fail to 
> compile.

FYI: This is avoided in other drivers by using
of_match_ptr(omap_iommu_of_match), which is replaced with NULL by
the preprocessor if CONFIG_OF is not defined.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 3/7] iommu/omap: Convert to devicetree

[Florian Vaussard]

Hi

On 01/02/2014 02:01 AM, Sebastian Reichel wrote:
> Hi,
> 
> On Thu, Jan 02, 2014 at 01:13:42AM +0100, Laurent Pinchart wrote:
>>> +		.of_match_table = omap_iommu_of_match,
>>
>> If CONFIG_OF isn't defined (pretty unlikely I agree, but a possibility you 
>> seem to be prepared for nonetheless given the above #if), this will fail to 
>> compile.
> 
> FYI: This is avoided in other drivers by using
> of_match_ptr(omap_iommu_of_match), which is replaced with NULL by
> the preprocessor if CONFIG_OF is not defined.
> 

Thank you for the hint.

Florian

[PATCH 4/7] iommu/omap: Allow enable/disable even without pdata

[Florian Vaussard]

When booting with a devietree, no platform data is provided. Do not prematurely
exit iommu_enable() and iommu_disable() in such a case.

Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
---
 drivers/iommu/omap-iommu.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 51efcc4..0a9854d 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -149,13 +149,10 @@ static int iommu_enable(struct omap_iommu *obj)
 	struct platform_device *pdev = to_platform_device(obj->dev);
 	struct iommu_platform_data *pdata = pdev->dev.platform_data;
 
-	if (!pdata)
-		return -EINVAL;
-
 	if (!arch_iommu)
 		return -ENODEV;
 
-	if (pdata->deassert_reset) {
+	if (pdata && pdata->deassert_reset) {
 		err = pdata->deassert_reset(pdev, pdata->reset_name);
 		if (err) {
 			dev_err(obj->dev, "deassert_reset failed: %d\n", err);
@@ -175,14 +172,11 @@ static void iommu_disable(struct omap_iommu *obj)
 	struct platform_device *pdev = to_platform_device(obj->dev);
 	struct iommu_platform_data *pdata = pdev->dev.platform_data;
 
-	if (!pdata)
-		return;
-
 	arch_iommu->disable(obj);
 
 	pm_runtime_put_sync(obj->dev);
 
-	if (pdata->assert_reset)
+	if (pdata && pdata->assert_reset)
 		pdata->assert_reset(pdev, pdata->reset_name);
 }
 
-- 
1.8.1.2

Re: [PATCH 4/7] iommu/omap: Allow enable/disable even without pdata

[Anna, Suman]

Hi Florian,

On 12/17/2013 06:53 AM, Florian Vaussard wrote:
> When booting with a devietree, no platform data is provided. Do not prematurely
> exit iommu_enable() and iommu_disable() in such a case.

Platform data may still be needed if we were to go with the pdata quirks 
approach for handling resets. You may need to revise the patch 
description then, but the change itself may be ok if supporting only DT 
devices.

regards
Suman

>
> Signed-off-by: Florian Vaussard <florian.vaussard-p8DiymsW2f8@public.gmane.org>
> ---
>   drivers/iommu/omap-iommu.c | 10 ++--------
>   1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> index 51efcc4..0a9854d 100644
> --- a/drivers/iommu/omap-iommu.c
> +++ b/drivers/iommu/omap-iommu.c
> @@ -149,13 +149,10 @@ static int iommu_enable(struct omap_iommu *obj)
>   	struct platform_device *pdev = to_platform_device(obj->dev);
>   	struct iommu_platform_data *pdata = pdev->dev.platform_data;
>
> -	if (!pdata)
> -		return -EINVAL;
> -
>   	if (!arch_iommu)
>   		return -ENODEV;
>
> -	if (pdata->deassert_reset) {
> +	if (pdata && pdata->deassert_reset) {
>   		err = pdata->deassert_reset(pdev, pdata->reset_name);
>   		if (err) {
>   			dev_err(obj->dev, "deassert_reset failed: %d\n", err);
> @@ -175,14 +172,11 @@ static void iommu_disable(struct omap_iommu *obj)
>   	struct platform_device *pdev = to_platform_device(obj->dev);
>   	struct iommu_platform_data *pdata = pdev->dev.platform_data;
>
> -	if (!pdata)
> -		return;
> -
>   	arch_iommu->disable(obj);
>
>   	pm_runtime_put_sync(obj->dev);
>
> -	if (pdata->assert_reset)
> +	if (pdata && pdata->assert_reset)
>   		pdata->assert_reset(pdev, pdata->reset_name);
>   }
>
>

Re: [PATCH 4/7] iommu/omap: Allow enable/disable even without pdata

[Florian Vaussard]

Hi Suman,

On 12/23/2013 08:05 PM, Anna, Suman wrote:
> Hi Florian,
> 
> On 12/17/2013 06:53 AM, Florian Vaussard wrote:
>> When booting with a devietree, no platform data is provided. Do not
>> prematurely
>> exit iommu_enable() and iommu_disable() in such a case.
> 
> Platform data may still be needed if we were to go with the pdata quirks
> approach for handling resets. You may need to revise the patch
> description then, but the change itself may be ok if supporting only DT
> devices.
> 

Indeed, this should be mentioned in the commit message. I would go for
a pdata-quirk until we have a proper reset-controller.

Regards,

Florian

[PATCH 5/7] ARM: dts: Complete data for isp iommu

[Florian Vaussard]

Add missing information required to probe the iommu for the camera
subsystem.

Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
---
 arch/arm/boot/dts/omap3.dtsi | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/arch/arm/boot/dts/omap3.dtsi b/arch/arm/boot/dts/omap3.dtsi
index daabf99..610d084 100644
--- a/arch/arm/boot/dts/omap3.dtsi
+++ b/arch/arm/boot/dts/omap3.dtsi
@@ -371,11 +371,13 @@
 			dma-names = "tx", "rx";
 		};
 
-		mmu_isp: mmu@480bd400 {
+		mmu_isp: mmu_isp@480bd400 {
 			compatible = "ti,omap3-mmu-isp";
 			ti,hwmods = "mmu_isp";
 			reg = <0x480bd400 0x80>;
 			interrupts = <8>;
+			ti,#tlb-entries = <8>;
+			dma-window = <0 0xfffff000>;	/* IOVA start & length */
 		};
 
 		wdt2: wdt@48314000 {
-- 
1.8.1.2

Re: [PATCH 5/7] ARM: dts: Complete data for isp iommu

[Anna, Suman]

Hi Florian,

On 12/17/2013 06:53 AM, Florian Vaussard wrote:
> Add missing information required to probe the iommu for the camera
> subsystem.
>
> Signed-off-by: Florian Vaussard <florian.vaussard-p8DiymsW2f8@public.gmane.org>
> ---
>   arch/arm/boot/dts/omap3.dtsi | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm/boot/dts/omap3.dtsi b/arch/arm/boot/dts/omap3.dtsi
> index daabf99..610d084 100644
> --- a/arch/arm/boot/dts/omap3.dtsi
> +++ b/arch/arm/boot/dts/omap3.dtsi
> @@ -371,11 +371,13 @@
>   			dma-names = "tx", "rx";
>   		};
>
> -		mmu_isp: mmu@480bd400 {
> +		mmu_isp: mmu_isp@480bd400 {

Any reason for switching the name to mmu_isp?

>   			compatible = "ti,omap3-mmu-isp";
>   			ti,hwmods = "mmu_isp";
>   			reg = <0x480bd400 0x80>;
>   			interrupts = <8>;

As I was testing the series, I found that this interrupt number is 
wrong. The interrupt number should be 24, you can fix it in this patch.
I will post couple of patches to correct the interrupt numbers for 
couple of other occurrences.

regards
Suman

> +			ti,#tlb-entries = <8>;
> +			dma-window = <0 0xfffff000>;	/* IOVA start & length */
>   		};
>
>   		wdt2: wdt@48314000 {
>

Re: [PATCH 5/7] ARM: dts: Complete data for isp iommu

[Florian Vaussard]

Hi Suman,

On 12/23/2013 08:12 PM, Anna, Suman wrote:
> Hi Florian,
> 
> On 12/17/2013 06:53 AM, Florian Vaussard wrote:
>> Add missing information required to probe the iommu for the camera
>> subsystem.
>>
>> Signed-off-by: Florian Vaussard <florian.vaussard-p8DiymsW2f8@public.gmane.org>
>> ---
>>   arch/arm/boot/dts/omap3.dtsi | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/arm/boot/dts/omap3.dtsi b/arch/arm/boot/dts/omap3.dtsi
>> index daabf99..610d084 100644
>> --- a/arch/arm/boot/dts/omap3.dtsi
>> +++ b/arch/arm/boot/dts/omap3.dtsi
>> @@ -371,11 +371,13 @@
>>               dma-names = "tx", "rx";
>>           };
>>
>> -        mmu_isp: mmu@480bd400 {
>> +        mmu_isp: mmu_isp@480bd400 {
> 
> Any reason for switching the name to mmu_isp?
> 

The name of the hwmod is "mmu_isp". This was not working otherwise,
but I cannot tell you for sure why without getting back at my office.

>>               compatible = "ti,omap3-mmu-isp";
>>               ti,hwmods = "mmu_isp";
>>               reg = <0x480bd400 0x80>;
>>               interrupts = <8>;
> 
> As I was testing the series, I found that this interrupt number is
> wrong. The interrupt number should be 24, you can fix it in this patch.
> I will post couple of patches to correct the interrupt numbers for
> couple of other occurrences.
> 

Really? Oh yes, this is CAM_IRQ0, thus M_IRQ_24. Nice catch.

Regards,

Florian

Re: [PATCH 5/7] ARM: dts: Complete data for isp iommu

[Anna, Suman]

Hi Florian,

On 12/23/2013 03:34 PM, Florian Vaussard wrote:
> Hi Suman,
>
> On 12/23/2013 08:12 PM, Anna, Suman wrote:
>> Hi Florian,
>>
>> On 12/17/2013 06:53 AM, Florian Vaussard wrote:
>>> Add missing information required to probe the iommu for the camera
>>> subsystem.
>>>
>>> Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
>>> ---
>>>    arch/arm/boot/dts/omap3.dtsi | 4 +++-
>>>    1 file changed, 3 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/arch/arm/boot/dts/omap3.dtsi b/arch/arm/boot/dts/omap3.dtsi
>>> index daabf99..610d084 100644
>>> --- a/arch/arm/boot/dts/omap3.dtsi
>>> +++ b/arch/arm/boot/dts/omap3.dtsi
>>> @@ -371,11 +371,13 @@
>>>                dma-names = "tx", "rx";
>>>            };
>>>
>>> -        mmu_isp: mmu@480bd400 {
>>> +        mmu_isp: mmu_isp@480bd400 {
>>
>> Any reason for switching the name to mmu_isp?
>>
>
> The name of the hwmod is "mmu_isp". This was not working otherwise,
> but I cannot tell you for sure why without getting back at my office.

Ok, did a bit of digging, this is due to the name tie-in for iommu arch 
data (look at the omap3_camera_init in mach-omap2/devices.c), and the 
obj->name assignment of of->name. This is another thing that needs to be 
looked into since it would be preferable to move away from the name 
based lookup towards using a phandle approach by the iommu consumer drivers.

regards
Suman

[PATCH 6/7] ARM: OMAP2+: Remove legacy data from hwmod for omap3 isp iommu

[Florian Vaussard]

The data are now passed using the devicetree.

Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
---
 arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
index 0477131..6dccd46 100644
--- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
+++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
@@ -2469,15 +2469,8 @@ static struct omap_hwmod_class omap3xxx_mmu_hwmod_class = {
 
 /* mmu isp */
 
-static struct omap_mmu_dev_attr mmu_isp_dev_attr = {
-	.da_start	= 0x0,
-	.da_end		= 0xfffff000,
-	.nr_tlb_entries = 8,
-};
-
 static struct omap_hwmod omap3xxx_mmu_isp_hwmod;
 
-
 /* l4_core -> mmu isp */
 static struct omap_hwmod_ocp_if omap3xxx_l4_core__mmu_isp = {
 	.master		= &omap3xxx_l4_core_hwmod,
@@ -2489,7 +2482,6 @@ static struct omap_hwmod omap3xxx_mmu_isp_hwmod = {
 	.name		= "mmu_isp",
 	.class		= &omap3xxx_mmu_hwmod_class,
 	.main_clk	= "cam_ick",
-	.dev_attr	= &mmu_isp_dev_attr,
 	.flags		= HWMOD_NO_IDLEST,
 };
 
-- 
1.8.1.2

Re: [PATCH 6/7] ARM: OMAP2+: Remove legacy data from hwmod for omap3 isp iommu

[Anna, Suman]

Hi Florian,

On 12/17/2013 06:53 AM, Florian Vaussard wrote:
> The data are now passed using the devicetree.

Patch is good by itself. A similar change is needed for the IVA
MMU as well.

regards
Suman

>
> Signed-off-by: Florian Vaussard <florian.vaussard-p8DiymsW2f8@public.gmane.org>
> ---
>   arch/arm/mach-omap2/omap_hwmod_3xxx_data.c | 8 --------
>   1 file changed, 8 deletions(-)
>
> diff --git a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> index 0477131..6dccd46 100644
> --- a/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> +++ b/arch/arm/mach-omap2/omap_hwmod_3xxx_data.c
> @@ -2469,15 +2469,8 @@ static struct omap_hwmod_class omap3xxx_mmu_hwmod_class = {
>
>   /* mmu isp */
>
> -static struct omap_mmu_dev_attr mmu_isp_dev_attr = {
> -	.da_start	= 0x0,
> -	.da_end		= 0xfffff000,
> -	.nr_tlb_entries = 8,
> -};
> -
>   static struct omap_hwmod omap3xxx_mmu_isp_hwmod;
>
> -
>   /* l4_core -> mmu isp */
>   static struct omap_hwmod_ocp_if omap3xxx_l4_core__mmu_isp = {
>   	.master		= &omap3xxx_l4_core_hwmod,
> @@ -2489,7 +2482,6 @@ static struct omap_hwmod omap3xxx_mmu_isp_hwmod = {
>   	.name		= "mmu_isp",
>   	.class		= &omap3xxx_mmu_hwmod_class,
>   	.main_clk	= "cam_ick",
> -	.dev_attr	= &mmu_isp_dev_attr,
>   	.flags		= HWMOD_NO_IDLEST,
>   };
>
>

Re: [PATCH 6/7] ARM: OMAP2+: Remove legacy data from hwmod for omap3 isp iommu

[Florian Vaussard]

Hi Suman,

On 12/23/2013 08:08 PM, Anna, Suman wrote:
> Hi Florian,
> 
> On 12/17/2013 06:53 AM, Florian Vaussard wrote:
>> The data are now passed using the devicetree.
> 
> Patch is good by itself. A similar change is needed for the IVA
> MMU as well.
> 

As I understood, the IVA MMU is still not integrated into the iommu
core, as its implementation lives under staging/tidspbridge/. But
correct me if I am wrong.

Regards,

Florian

Re: [PATCH 6/7] ARM: OMAP2+: Remove legacy data from hwmod for omap3 isp iommu

[Anna, Suman]

Hi Florian,

 >>
>> On 12/17/2013 06:53 AM, Florian Vaussard wrote:
>>> The data are now passed using the devicetree.
>>
>> Patch is good by itself. A similar change is needed for the IVA
>> MMU as well.
>>
>
> As I understood, the IVA MMU is still not integrated into the iommu
> core, as its implementation lives under staging/tidspbridge/. But
> correct me if I am wrong.
>

Yes, that is correct. That shouldn't stop us though from adding the IVA 
MMU node to the DT file and the associated cleanup. To begin with, we 
can have the DT node added with status=disabled.

regards
Suman

[PATCH 7/7] ARM: OMAP2+: Remove platform-specific omap-iommu

[Florian Vaussard]

With full DT boot, the platform specific part of the OMAP iommu
is not useful anymore.

Signed-off-by: Florian Vaussard <florian.vaussard@epfl.ch>
---
 arch/arm/mach-omap2/Makefile     |  3 --
 arch/arm/mach-omap2/omap-iommu.c | 79 ----------------------------------------
 2 files changed, 82 deletions(-)
 delete mode 100644 arch/arm/mach-omap2/omap-iommu.c

diff --git a/arch/arm/mach-omap2/Makefile b/arch/arm/mach-omap2/Makefile
index 35be79f..4e01b61 100644
--- a/arch/arm/mach-omap2/Makefile
+++ b/arch/arm/mach-omap2/Makefile
@@ -219,9 +219,6 @@ obj-$(CONFIG_SOC_DRA7XX)		+= omap_hwmod_7xx_data.o
 obj-$(CONFIG_OMAP3_EMU)			+= emu.o
 obj-$(CONFIG_HW_PERF_EVENTS)		+= pmu.o
 
-iommu-$(CONFIG_OMAP_IOMMU)		:= omap-iommu.o
-obj-y					+= $(iommu-m) $(iommu-y)
-
 ifneq ($(CONFIG_TIDSPBRIDGE),)
 obj-y					+= dsp.o
 endif
diff --git a/arch/arm/mach-omap2/omap-iommu.c b/arch/arm/mach-omap2/omap-iommu.c
deleted file mode 100644
index f1fab56..0000000
--- a/arch/arm/mach-omap2/omap-iommu.c
+++ /dev/null
@@ -1,79 +0,0 @@
-/*
- * omap iommu: omap device registration
- *
- * Copyright (C) 2008-2009 Nokia Corporation
- *
- * Written by Hiroshi DOYU <Hiroshi.DOYU@nokia.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
-
-#include <linux/of.h>
-#include <linux/module.h>
-#include <linux/platform_device.h>
-#include <linux/err.h>
-#include <linux/slab.h>
-
-#include <linux/platform_data/iommu-omap.h>
-#include "soc.h"
-#include "omap_hwmod.h"
-#include "omap_device.h"
-
-static int __init omap_iommu_dev_init(struct omap_hwmod *oh, void *unused)
-{
-	struct platform_device *pdev;
-	struct iommu_platform_data *pdata;
-	struct omap_mmu_dev_attr *a = (struct omap_mmu_dev_attr *)oh->dev_attr;
-	static int i;
-
-	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
-	if (!pdata)
-		return -ENOMEM;
-
-	pdata->name = oh->name;
-	pdata->nr_tlb_entries = a->nr_tlb_entries;
-	pdata->da_start = a->da_start;
-	pdata->da_end = a->da_end;
-
-	if (oh->rst_lines_cnt == 1) {
-		pdata->reset_name = oh->rst_lines->name;
-		pdata->assert_reset = omap_device_assert_hardreset;
-		pdata->deassert_reset = omap_device_deassert_hardreset;
-	}
-
-	pdev = omap_device_build("omap-iommu", i, oh, pdata, sizeof(*pdata));
-
-	kfree(pdata);
-
-	if (IS_ERR(pdev)) {
-		pr_err("%s: device build err: %ld\n", __func__, PTR_ERR(pdev));
-		return PTR_ERR(pdev);
-	}
-
-	i++;
-
-	return 0;
-}
-
-static int __init omap_iommu_init(void)
-{
-	/* If dtb is there, the devices will be created dynamically */
-	if (of_have_populated_dt())
-		return -ENODEV;
-
-	return omap_hwmod_for_each_by_class("mmu", omap_iommu_dev_init, NULL);
-}
-/* must be ready before omap3isp is probed */
-omap_subsys_initcall(omap_iommu_init);
-
-static void __exit omap_iommu_exit(void)
-{
-	/* Do nothing */
-}
-module_exit(omap_iommu_exit);
-
-MODULE_AUTHOR("Hiroshi DOYU");
-MODULE_DESCRIPTION("omap iommu: omap device registration");
-MODULE_LICENSE("GPL v2");
-- 
1.8.1.2

Re: [PATCH 0/7] Fix omap-iommu probe and convert to DT for 3.14

[Anna, Suman]

Hi Florian,

On 12/17/2013 06:53 AM, Florian Vaussard wrote:
> OMAP2+ is heading towards a full device tree boot for 3.14. Currently,
> the iommu used by the OMAP3 camera subsystem is not yet converted. It
> cannot be probed as necessary data are only passed through device tree.
>
> Patches 1 and 2 are small fixes for problems encountered while developing
> this series.
>
> Patches 3 to 5 add the device tree logic to omap-iommu, and complete iommu
> data in omap3.dtsi. Patches 6 and 7 remove unused iommu hwmod data and
> platform code from OMAP2+.

This is a good starting patch series for the OMAP iommu DT conversion, 
but it only handles OMAP3 ISP MMU. The OMAP3 ISP MMU is not the only MMU 
handled by the OMAP iommu driver. There is also an OMAP3 IVA MMU and 
MMUs associated with DSP and IPU in OMAP4/OMAP5. The conversion is 
simpler just with the OMAP3 ISP MMU, as it doesn't have any reset lines 
associated with it. But all the other MMUs would require 
asserting/deasserting the resets (performed currently through the pdata 
function pointers). Your patch series removes that functionality 
completely, and if this were to go into 3.14, this has to be handled 
through some pdata quirks until all the resets in hwmod data are 
converted to a reset driver.

I have provided some more comments directly in the respective patches.

regards
Suman

>
> This was tested on Overo (OMAP36xx) with an MT9V032 sensor connected
> to the isp interface. The full testing tree can be found here [2] (not
> safe for merging).
>
> Patches are based on 3.13-rc3. OMAP-related patches are based on Tony's
> omap-for-v3.14/omap3-board-removal branch [1].
>
> Regards,
>
> Florian
>
> [1] git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap.git
>        omap-for-v3.14/omap3-board-removal
> [2] git-9UaJU3cA/F/QT0dZR+AlfA@public.gmane.org:vaussard/linux.git overo-for-3.14/iommu/dt
>
> Florian Vaussard (7):
>    iommu/omap: Do bus_set_iommu() only if probe() succeeds
>    iommu/omap: omap_iommu_attach() should return ENODEV, not NULL
>    iommu/omap: Convert to devicetree
>    iommu/omap: Allow enable/disable even without pdata
>    ARM: dts: Complete data for isp iommu
>    ARM: OMAP2+: Remove legacy data from hwmod for omap3 isp iommu
>    ARM: OMAP2+: Remove platform-specific omap-iommu
>
>   .../devicetree/bindings/iommu/ti,omap-iommu.txt    |  19 ++
>   arch/arm/boot/dts/omap3.dtsi                       |   4 +-
>   arch/arm/mach-omap2/Makefile                       |   3 -
>   arch/arm/mach-omap2/omap-iommu.c                   |  74 ------
>   arch/arm/mach-omap2/omap_hwmod_3xxx_data.c         |   8 -
>   drivers/iommu/omap-iommu.c                         | 247 +++++++++++----------
>   6 files changed, 156 insertions(+), 199 deletions(-)
>   create mode 100644 Documentation/devicetree/bindings/iommu/ti,omap-iommu.txt
>   delete mode 100644 arch/arm/mach-omap2/omap-iommu.c
>

Re: [PATCH 0/7] Fix omap-iommu probe and convert to DT for 3.14

[Florian Vaussard]

Hi Suman,

On 12/23/2013 07:52 PM, Anna, Suman wrote:
> Hi Florian,
>
> On 12/17/2013 06:53 AM, Florian Vaussard wrote:
>> OMAP2+ is heading towards a full device tree boot for 3.14. Currently,
>> the iommu used by the OMAP3 camera subsystem is not yet converted. It
>> cannot be probed as necessary data are only passed through device tree.
>>
>> Patches 1 and 2 are small fixes for problems encountered while developing
>> this series.
>>
>> Patches 3 to 5 add the device tree logic to omap-iommu, and complete
>> iommu
>> data in omap3.dtsi. Patches 6 and 7 remove unused iommu hwmod data and
>> platform code from OMAP2+.
>
> This is a good starting patch series for the OMAP iommu DT conversion,
> but it only handles OMAP3 ISP MMU. The OMAP3 ISP MMU is not the only MMU
> handled by the OMAP iommu driver. There is also an OMAP3 IVA MMU and
> MMUs associated with DSP and IPU in OMAP4/OMAP5. The conversion is
> simpler just with the OMAP3 ISP MMU, as it doesn't have any reset lines
> associated with it. But all the other MMUs would require
> asserting/deasserting the resets (performed currently through the pdata
> function pointers). Your patch series removes that functionality
> completely, and if this were to go into 3.14, this has to be handled
> through some pdata quirks until all the resets in hwmod data are
> converted to a reset driver.
>

Indeed, this patchset currently addresses only the OMAP ISP IOMMU.
For the OMAP3 IVA, the current implementation looks completely
different (inside drivers/staging/tidspbridge/). And to the best of
my knowledge, the current omap-iommu driver can handle only one
instance. By calling

bus_set_iommu(&platform_bus_type, &omap_iommu_ops);

the driver register the iommu on the platform bus (which is maybe
not the best choice). This call will fill the struct iommu_ops of
the bus_type, but if an iommu is already present, it will fail
and return EBUSY. Thus only one iommu can be set on the same bus.

But for the OMAP4 and OMAP5, I understand your concern. If a reset
is needed for these IPs, then a solution must be found. pdata-quirks
can be a temporary solution for 3.14. Otherwise a proper reset
controller should be devised from the current PRM module. Even if
I already looked at the reset core, I do not know the amount of work
necessary for this solution. And I do not have the hardware to
test the solution. But I can have a look after the XMAS break.

> I have provided some more comments directly in the respective patches.
>

And I will answer inline. Thank you very much!

Regards,

Florian

Re: [PATCH 0/7] Fix omap-iommu probe and convert to DT for 3.14

[Anna, Suman]

Hi Florian,

 >>
>> On 12/17/2013 06:53 AM, Florian Vaussard wrote:
>>> OMAP2+ is heading towards a full device tree boot for 3.14. Currently,
>>> the iommu used by the OMAP3 camera subsystem is not yet converted. It
>>> cannot be probed as necessary data are only passed through device tree.
>>>
>>> Patches 1 and 2 are small fixes for problems encountered while developing
>>> this series.
>>>
>>> Patches 3 to 5 add the device tree logic to omap-iommu, and complete
>>> iommu
>>> data in omap3.dtsi. Patches 6 and 7 remove unused iommu hwmod data and
>>> platform code from OMAP2+.
>>
>> This is a good starting patch series for the OMAP iommu DT conversion,
>> but it only handles OMAP3 ISP MMU. The OMAP3 ISP MMU is not the only MMU
>> handled by the OMAP iommu driver. There is also an OMAP3 IVA MMU and
>> MMUs associated with DSP and IPU in OMAP4/OMAP5. The conversion is
>> simpler just with the OMAP3 ISP MMU, as it doesn't have any reset lines
>> associated with it. But all the other MMUs would require
>> asserting/deasserting the resets (performed currently through the pdata
>> function pointers). Your patch series removes that functionality
>> completely, and if this were to go into 3.14, this has to be handled
>> through some pdata quirks until all the resets in hwmod data are
>> converted to a reset driver.
>>
>
> Indeed, this patchset currently addresses only the OMAP ISP IOMMU.
> For the OMAP3 IVA, the current implementation looks completely
> different (inside drivers/staging/tidspbridge/). And to the best of
> my knowledge, the current omap-iommu driver can handle only one
> instance. By calling
>
> bus_set_iommu(&platform_bus_type, &omap_iommu_ops);

Not really, the omap_iommu_ops are identical for all instances. The 
usage would be by different drivers, with each of them doing a attach 
for the respective device.

> the driver register the iommu on the platform bus (which is maybe
> not the best choice). This call will fill the struct iommu_ops of
> the bus_type, but if an iommu is already present, it will fail
> and return EBUSY. Thus only one iommu can be set on the same bus.
>
> But for the OMAP4 and OMAP5, I understand your concern. If a reset
> is needed for these IPs, then a solution must be found. pdata-quirks
> can be a temporary solution for 3.14. Otherwise a proper reset
> controller should be devised from the current PRM module. Even if
> I already looked at the reset core, I do not know the amount of work
> necessary for this solution. And I do not have the hardware to
> test the solution. But I can have a look after the XMAS break.

Yeah, the DT conversion has been on my list, and wanted to do this after 
the TI reset framework changes. That would probably take some time as it 
also involves the hwmod framework, so the only short term solution to 
enable this would be to use the pdata-quirks.

Tony, do you have any objections to this approach?

regards
Suman