[PATCH v2 0/6] OMAP IOMMU fixes

[PATCH v2 0/6] OMAP IOMMU fixes

[Laurent Pinchart]

Hello,

This patch set fixes miscellaneous issues with the OMAP IOMMU driver, found
when trying to port the OMAP3 ISP away from omap-iovmm to the ARM DMA API. The
biggest issue is fixed by patch 6/6, while the other patches fix smaller
problems that I've noticed when reading the code, without experiencing them at
runtime.

I believe I've addressed all comments received for v1. Suman, could you please
have a quick look at ack the patches you haven't acked yet ? I'll then send a
pull request.

Changes since v1:

- Dropped the "iommu/omap: Use the cache cleaning API" and "iommu/omap: Flush
  the TLB only after updating translation table entries" patches
- Reworked "iommu/omap: Fix 'no page for' debug message in flush_iotlb_page()"
  to just break out of the loop
- Added patches 1/6 to 3/6

Laurent Pinchart (3):
  iommu/omap: Fix 'no page for' debug message in flush_iotlb_page()
  iommu/omap: Remove comment about supporting single page mappings only
  iommu/omap: Fix map protection value handling

Suman Anna (3):
  iommu/omap: Correct init value of iotlb_entry valid field
  iommu/omap: Remove omap_iommu_domain_has_cap() function
  iommu/omap: Move to_iommu definition from omap-iopgtable.h

 drivers/iommu/omap-iommu.c     | 31 ++++++++++++-------------------
 drivers/iommu/omap-iopgtable.h |  3 ---
 2 files changed, 12 insertions(+), 22 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH v2 1/6] iommu/omap: Correct init value of iotlb_entry valid field

[Laurent Pinchart]

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

The iotlb_entry field values are used directly in omap2_alloc_cr,
a function used in preparing the MMU_CAM and MMU_RAM registers.
The iotlb_entry.valid value is being set incorrectly to 1 at the
moment, and this would result in overriding the PAGESIZE bit field
of the MMU_CAM register if prefetching of the entries were to be
supported.

The bug has not caused any MMU faults due to incorrect size
programming so far as the prefetching is disabled by default. Fix
this by using the correct init value for the iotlb_entry.valid
field.

Signed-off-by: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>
---
 drivers/iommu/omap-iommu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 8acea87..4716af1 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1043,7 +1043,7 @@ static u32 iotlb_init_entry(struct iotlb_entry *e, u32 da, u32 pa,
 
 	e->da		= da;
 	e->pa		= pa;
-	e->valid	= 1;
+	e->valid	= MMU_CAM_V;
 	/* FIXME: add OMAP1 support */
 	e->pgsz		= flags & MMU_CAM_PGSZ_MASK;
 	e->endian	= flags & MMU_RAM_ENDIAN_MASK;
-- 
1.8.3.2

[PATCH v2 3/6] iommu/omap: Move to_iommu definition from omap-iopgtable.h

[Laurent Pinchart]

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

The to_iommu definition is used only locally to the omap-iommu.c
source file, and it has nothing to do with the page attributes
defined in omap-iopgtable.h. So, move the definition out of
omap-iopgtable.h header file.

Signed-off-by: Suman Anna <s-anna-l0cyMroinI0@public.gmane.org>
---
 drivers/iommu/omap-iommu.c     | 3 +++
 drivers/iommu/omap-iopgtable.h | 3 ---
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index d987edb..fdf3dd0 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -34,6 +34,9 @@
 #include "omap-iopgtable.h"
 #include "omap-iommu.h"
 
+#define to_iommu(dev)							\
+	((struct omap_iommu *)platform_get_drvdata(to_platform_device(dev)))
+
 #define for_each_iotlb_cr(obj, n, __i, cr)				\
 	for (__i = 0;							\
 	     (__i < (n)) && (cr = __iotlb_read_cr((obj), __i), true);	\
diff --git a/drivers/iommu/omap-iopgtable.h b/drivers/iommu/omap-iopgtable.h
index b6f9a51..f891683 100644
--- a/drivers/iommu/omap-iopgtable.h
+++ b/drivers/iommu/omap-iopgtable.h
@@ -93,6 +93,3 @@ static inline phys_addr_t omap_iommu_translate(u32 d, u32 va, u32 mask)
 /* to find an entry in the second-level page table. */
 #define iopte_index(da)		(((da) >> IOPTE_SHIFT) & (PTRS_PER_IOPTE - 1))
 #define iopte_offset(iopgd, da)	(iopgd_page_vaddr(iopgd) + iopte_index(da))
-
-#define to_iommu(dev)							\
-	(platform_get_drvdata(to_platform_device(dev)))
-- 
1.8.3.2

[PATCH v2 4/6] iommu/omap: Fix 'no page for' debug message in flush_iotlb_page()

[Laurent Pinchart]

The flush_iotlb_page() function prints a debug message when no
corresponding page was found in the TLB. That condition is incorrectly
checked and always resolves to true, given that the for_each_iotlb_cr()
loop is never interrupted and always reaches obj->nr_tlb_entries.

Given that we can't have two TLB entries for the same VA, break from the
loop when a match is found.

Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
---
 drivers/iommu/omap-iommu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index fdf3dd0..428b47a 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -394,6 +394,7 @@ static void flush_iotlb_page(struct omap_iommu *obj, u32 da)
 				__func__, start, da, bytes);
 			iotlb_load_cr(obj, &cr);
 			iommu_write_reg(obj, 1, MMU_FLUSH_ENTRY);
+			break;
 		}
 	}
 	pm_runtime_put_sync(obj->dev);
-- 
1.8.3.2

Re: [PATCH v2 4/6] iommu/omap: Fix 'no page for' debug message in flush_iotlb_page()

[Suman Anna]

Hi Laurent,

On 04/03/2014 03:01 PM, Laurent Pinchart wrote:
> The flush_iotlb_page() function prints a debug message when no
> corresponding page was found in the TLB. That condition is incorrectly
> checked and always resolves to true, given that the for_each_iotlb_cr()
> loop is never interrupted and always reaches obj->nr_tlb_entries.
> 
> Given that we can't have two TLB entries for the same VA, break from the
> loop when a match is found.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>

Thanks for the reworked and simplified patch.

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

> ---
>  drivers/iommu/omap-iommu.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
> index fdf3dd0..428b47a 100644
> --- a/drivers/iommu/omap-iommu.c
> +++ b/drivers/iommu/omap-iommu.c
> @@ -394,6 +394,7 @@ static void flush_iotlb_page(struct omap_iommu *obj, u32 da)
>  				__func__, start, da, bytes);
>  			iotlb_load_cr(obj, &cr);
>  			iommu_write_reg(obj, 1, MMU_FLUSH_ENTRY);
> +			break;
>  		}
>  	}
>  	pm_runtime_put_sync(obj->dev);
> 

[PATCH v2 5/6] iommu/omap: Remove comment about supporting single page mappings only

[Laurent Pinchart]

The IOMMU core breaks out mappings into pages already, there's no
need to support mapping multiple pages in one go.

Signed-off-by: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/C1BVhZhaw@public.gmane.org>
Reviewed-by: Sakari Ailus <sakari.ailus-X3B1VOXEql0@public.gmane.org>
---
 drivers/iommu/omap-iommu.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 428b47a..a47dbdc 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1067,7 +1067,6 @@ static int omap_iommu_map(struct iommu_domain *domain, unsigned long da,
 	int omap_pgsz;
 	u32 ret, flags;
 
-	/* we only support mapping a single iommu page for now */
 	omap_pgsz = bytes_to_iopgsz(bytes);
 	if (omap_pgsz < 0) {
 		dev_err(dev, "invalid size to map: %d\n", bytes);
-- 
1.8.3.2

Re: [PATCH v2 0/6] OMAP IOMMU fixes

[Suman Anna]

Hi Laurent,

On 04/03/2014 03:01 PM, Laurent Pinchart wrote:
> Hello,
> 
> This patch set fixes miscellaneous issues with the OMAP IOMMU driver, found
> when trying to port the OMAP3 ISP away from omap-iovmm to the ARM DMA API. The
> biggest issue is fixed by patch 6/6, while the other patches fix smaller
> problems that I've noticed when reading the code, without experiencing them at
> runtime.
> 
> I believe I've addressed all comments received for v1. Suman, could you please
> have a quick look at ack the patches you haven't acked yet ? I'll then send a
> pull request.

Thanks for the revised series.

Just acked Patch 4/6. Patch 3/6 is a simple comment removal, and I am
good with that change. First 3 are already are from me, so I guess you
need to add your sign-off before you send out the pull request.

regards
Suman


> 
> Changes since v1:
> 
> - Dropped the "iommu/omap: Use the cache cleaning API" and "iommu/omap: Flush
>   the TLB only after updating translation table entries" patches
> - Reworked "iommu/omap: Fix 'no page for' debug message in flush_iotlb_page()"
>   to just break out of the loop
> - Added patches 1/6 to 3/6
> 
> Laurent Pinchart (3):
>   iommu/omap: Fix 'no page for' debug message in flush_iotlb_page()
>   iommu/omap: Remove comment about supporting single page mappings only
>   iommu/omap: Fix map protection value handling
> 
> Suman Anna (3):
>   iommu/omap: Correct init value of iotlb_entry valid field
>   iommu/omap: Remove omap_iommu_domain_has_cap() function
>   iommu/omap: Move to_iommu definition from omap-iopgtable.h
> 
>  drivers/iommu/omap-iommu.c     | 31 ++++++++++++-------------------
>  drivers/iommu/omap-iopgtable.h |  3 ---
>  2 files changed, 12 insertions(+), 22 deletions(-)
> 

Re: [PATCH v2 0/6] OMAP IOMMU fixes

[Suman Anna]

On 04/03/2014 04:02 PM, Anna, Suman wrote:
> Hi Laurent,
> 
> On 04/03/2014 03:01 PM, Laurent Pinchart wrote:
>> Hello,
>>
>> This patch set fixes miscellaneous issues with the OMAP IOMMU driver, found
>> when trying to port the OMAP3 ISP away from omap-iovmm to the ARM DMA API. The
>> biggest issue is fixed by patch 6/6, while the other patches fix smaller
>> problems that I've noticed when reading the code, without experiencing them at
>> runtime.
>>
>> I believe I've addressed all comments received for v1. Suman, could you please
>> have a quick look at ack the patches you haven't acked yet ? I'll then send a
>> pull request.
> 
> Thanks for the revised series.
> 
> Just acked Patch 4/6. Patch 3/6 is a simple comment removal, and I am

Sorry, had a minor typo, I meant 5/6 w.r.t comment removal.

> good with that change. First 3 are already are from me, so I guess you
> need to add your sign-off before you send out the pull request.
> 
> regards
> Suman
> 
> 
>>
>> Changes since v1:
>>
>> - Dropped the "iommu/omap: Use the cache cleaning API" and "iommu/omap: Flush
>>   the TLB only after updating translation table entries" patches
>> - Reworked "iommu/omap: Fix 'no page for' debug message in flush_iotlb_page()"
>>   to just break out of the loop
>> - Added patches 1/6 to 3/6
>>
>> Laurent Pinchart (3):
>>   iommu/omap: Fix 'no page for' debug message in flush_iotlb_page()
>>   iommu/omap: Remove comment about supporting single page mappings only
>>   iommu/omap: Fix map protection value handling
>>
>> Suman Anna (3):
>>   iommu/omap: Correct init value of iotlb_entry valid field
>>   iommu/omap: Remove omap_iommu_domain_has_cap() function
>>   iommu/omap: Move to_iommu definition from omap-iopgtable.h
>>
>>  drivers/iommu/omap-iommu.c     | 31 ++++++++++++-------------------
>>  drivers/iommu/omap-iopgtable.h |  3 ---
>>  2 files changed, 12 insertions(+), 22 deletions(-)
>>
> 
> _______________________________________________
> iommu mailing list
> iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linuxfoundation.org/mailman/listinfo/iommu
> 

Re: [PATCH v2 0/6] OMAP IOMMU fixes

[Laurent Pinchart]

Hi Suman,

On Thursday 03 April 2014 16:02:09 Suman Anna wrote:
> On 04/03/2014 03:01 PM, Laurent Pinchart wrote:
> > Hello,
> > 
> > This patch set fixes miscellaneous issues with the OMAP IOMMU driver,
> > found when trying to port the OMAP3 ISP away from omap-iovmm to the ARM
> > DMA API. The biggest issue is fixed by patch 6/6, while the other patches
> > fix smaller problems that I've noticed when reading the code, without
> > experiencing them at runtime.
> > 
> > I believe I've addressed all comments received for v1. Suman, could you
> > please have a quick look at ack the patches you haven't acked yet ? I'll
> > then send a pull request.
> 
> Thanks for the revised series.
> 
> Just acked Patch 4/6. Patch 3/6 is a simple comment removal, and I am
> good with that change. First 3 are already are from me, so I guess you
> need to add your sign-off before you send out the pull request.

Sure, that was planned. Thank you for your ack.

-- 
Regards,

Laurent Pinchart

[PATCH v2 2/6] iommu/omap: Remove omap_iommu_domain_has_cap() function

[Laurent Pinchart]

From: Suman Anna <s-anna@ti.com>

The current OMAP IOMMU ops for .domain_has_cap is a stub,
and the iommu core already returns a value of 0 if the
domain doesn't have a .domain_has_cap ops plugged in. So,
clean up this stub function.

Signed-off-by: Suman Anna <s-anna@ti.com>
---
 drivers/iommu/omap-iommu.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index 4716af1..d987edb 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1247,12 +1247,6 @@ static phys_addr_t omap_iommu_iova_to_phys(struct iommu_domain *domain,
 	return ret;
 }
 
-static int omap_iommu_domain_has_cap(struct iommu_domain *domain,
-				    unsigned long cap)
-{
-	return 0;
-}
-
 static int omap_iommu_add_device(struct device *dev)
 {
 	struct omap_iommu_arch_data *arch_data;
@@ -1304,7 +1298,6 @@ static struct iommu_ops omap_iommu_ops = {
 	.map		= omap_iommu_map,
 	.unmap		= omap_iommu_unmap,
 	.iova_to_phys	= omap_iommu_iova_to_phys,
-	.domain_has_cap	= omap_iommu_domain_has_cap,
 	.add_device	= omap_iommu_add_device,
 	.remove_device	= omap_iommu_remove_device,
 	.pgsize_bitmap	= OMAP_IOMMU_PGSIZES,
-- 
1.8.3.2

[PATCH v2 6/6] iommu/omap: Fix map protection value handling

[Laurent Pinchart]

The prot flags passed to the IOMMU map handler are defined in
include/linux/iommu.h as IOMMU_(READ|WRITE|CACHE|EXEC). However, the
driver expects to receive MMU_RAM_* OMAP-specific flags. This causes
IOMMU flags being interpreted as page sizes, leading to failures.

Hardcode the OMAP mapping parameters to little-endian, 8-bits and
non-mixed page attributes. Furthermore, as the OMAP IOMMU doesn't
support read-only or write-only mappings, ignore the prot value.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Sakari Ailus <sakari.ailus@iki.fi>
Acked-by: Suman Anna <s-anna@ti.com>
---
 drivers/iommu/omap-iommu.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/drivers/iommu/omap-iommu.c b/drivers/iommu/omap-iommu.c
index a47dbdc..6923217 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1040,8 +1040,7 @@ static void iopte_cachep_ctor(void *iopte)
 	clean_dcache_area(iopte, IOPTE_TABLE_SIZE);
 }
 
-static u32 iotlb_init_entry(struct iotlb_entry *e, u32 da, u32 pa,
-				   u32 flags)
+static u32 iotlb_init_entry(struct iotlb_entry *e, u32 da, u32 pa, int pgsz)
 {
 	memset(e, 0, sizeof(*e));
 
@@ -1049,10 +1048,10 @@ static u32 iotlb_init_entry(struct iotlb_entry *e, u32 da, u32 pa,
 	e->pa		= pa;
 	e->valid	= MMU_CAM_V;
 	/* FIXME: add OMAP1 support */
-	e->pgsz		= flags & MMU_CAM_PGSZ_MASK;
-	e->endian	= flags & MMU_RAM_ENDIAN_MASK;
-	e->elsz		= flags & MMU_RAM_ELSZ_MASK;
-	e->mixed	= flags & MMU_RAM_MIXED_MASK;
+	e->pgsz		= pgsz;
+	e->endian	= MMU_RAM_ENDIAN_LITTLE;
+	e->elsz		= MMU_RAM_ELSZ_8;
+	e->mixed	= 0;
 
 	return iopgsz_to_bytes(e->pgsz);
 }
@@ -1065,7 +1064,7 @@ static int omap_iommu_map(struct iommu_domain *domain, unsigned long da,
 	struct device *dev = oiommu->dev;
 	struct iotlb_entry e;
 	int omap_pgsz;
-	u32 ret, flags;
+	u32 ret;
 
 	omap_pgsz = bytes_to_iopgsz(bytes);
 	if (omap_pgsz < 0) {
@@ -1075,9 +1074,7 @@ static int omap_iommu_map(struct iommu_domain *domain, unsigned long da,
 
 	dev_dbg(dev, "mapping da 0x%lx to pa 0x%x size 0x%x\n", da, pa, bytes);
 
-	flags = omap_pgsz | prot;
-
-	iotlb_init_entry(&e, da, pa, flags);
+	iotlb_init_entry(&e, da, pa, omap_pgsz);
 
 	ret = omap_iopgtable_store_entry(oiommu, &e);
 	if (ret)
-- 
1.8.3.2