Re: [PATCH 5/5] iommu/omap: Fix map protection value handling
Hi Suman,
Thank you for the review.
On Thursday 13 March 2014 19:07:33 Suman Anna wrote:
On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
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.pinch...@ideasonboard.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 59de3fc..9f7f1d4 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1016,8 +1016,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));
@@ -1025,10 +1024,10 @@ static u32 iotlb_init_entry(struct iotlb_entry *e,
u32 da, u32 pa,
e-pa = pa;
e-valid= 1;
As I was looking through this, I found a small bug here. This is unrelated
to this patch, but you can fix it in the same patch. The e-valid value is
directly used in omap2_alloc_cr, so it should be initialized to MMU_CAM_V.
It is not a problem currently as PREFETCH_IOTLB is not used, if used, this
is overriding the size bit-fields when it prefetches the entry, which may
lead to an MMU fault.
Good catch. As it's a separate issue, do you mind if I fix that in a separate
patch in v2 ?
/* 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;
Thanks, the change of these settings looks good overall. The only ones for
which it may apply anyway is just the Camera in OMAP2/OMAP3 (as per TRM,
endianness conversion supported only on writes anyway, so I am assuming that
we never use the combination), and OMAP2420 DSP, for which there is no
driver currently nor is the MMU being instantiated. All other processors are
pure little endian, and they even ignore the values written in these fields.
Please recheck the Camera subsystem once, and see if the e-elsz should be
set as MMU_RAM_ELSZ_NONE (I am not too familiar with the Camera usage, so
you are probably in a better place than me).
I've tested MMU_RAM_ELSZ_NONE and it works fine. I'll update the patch in v2.
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] iommu/omap: Fix map protection value handling
Hi Laurent,
On 03/14/2014 04:46 AM, Laurent Pinchart wrote:
Hi Suman,
Thank you for the review.
On Thursday 13 March 2014 19:07:33 Suman Anna wrote:
On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
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.pinch...@ideasonboard.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 59de3fc..9f7f1d4 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1016,8 +1016,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));
@@ -1025,10 +1024,10 @@ static u32 iotlb_init_entry(struct iotlb_entry *e,
u32 da, u32 pa,
e-pa= pa;
e-valid = 1;
As I was looking through this, I found a small bug here. This is unrelated
to this patch, but you can fix it in the same patch. The e-valid value is
directly used in omap2_alloc_cr, so it should be initialized to MMU_CAM_V.
It is not a problem currently as PREFETCH_IOTLB is not used, if used, this
is overriding the size bit-fields when it prefetches the entry, which may
lead to an MMU fault.
Good catch. As it's a separate issue, do you mind if I fix that in a separate
patch in v2 ?
I actually have couple of minor cleanup patches as well. I will send
them out and you can include them with your series when you post v2.
/* 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;
Thanks, the change of these settings looks good overall. The only ones for
which it may apply anyway is just the Camera in OMAP2/OMAP3 (as per TRM,
endianness conversion supported only on writes anyway, so I am assuming that
we never use the combination), and OMAP2420 DSP, for which there is no
driver currently nor is the MMU being instantiated. All other processors are
pure little endian, and they even ignore the values written in these fields.
Please recheck the Camera subsystem once, and see if the e-elsz should be
set as MMU_RAM_ELSZ_NONE (I am not too familiar with the Camera usage, so
you are probably in a better place than me).
I've tested MMU_RAM_ELSZ_NONE and it works fine. I'll update the patch in v2.
Thanks for checking and confirming this.
regards
Suman
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/5] iommu/omap: Fix map protection value handling
Hi Laurent,
On 03/07/2014 06:46 PM, Laurent Pinchart wrote:
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.pinch...@ideasonboard.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 59de3fc..9f7f1d4 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1016,8 +1016,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));
@@ -1025,10 +1024,10 @@ static u32 iotlb_init_entry(struct iotlb_entry *e, u32
da, u32 pa,
e-pa= pa;
e-valid = 1;
As I was looking through this, I found a small bug here. This is
unrelated to this patch, but you can fix it in the same patch. The
e-valid value is directly used in omap2_alloc_cr, so it should be
initialized to MMU_CAM_V. It is not a problem currently as
PREFETCH_IOTLB is not used, if used, this is overriding the size
bit-fields when it prefetches the entry, which may lead to an MMU fault.
/* 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;
Thanks, the change of these settings looks good overall. The only ones
for which it may apply anyway is just the Camera in OMAP2/OMAP3 (as per
TRM, endianness conversion supported only on writes anyway, so I am
assuming that we never use the combination), and OMAP2420 DSP, for which
there is no driver currently nor is the MMU being instantiated. All
other processors are pure little endian, and they even ignore the values
written in these fields.
Please recheck the Camera subsystem once, and see if the e-elsz should
be set as MMU_RAM_ELSZ_NONE (I am not too familiar with the Camera
usage, so you are probably in a better place than me).
If you are good, then with the e-valid change,
Acked-by: Suman Anna s-a...@ti.com
regards
Suman
return iopgsz_to_bytes(e-pgsz);
}
@@ -1041,7 +1040,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) {
@@ -1051,9 +1050,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)
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 5/5] iommu/omap: Fix map protection value handling
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.pinch...@ideasonboard.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 59de3fc..9f7f1d4 100644
--- a/drivers/iommu/omap-iommu.c
+++ b/drivers/iommu/omap-iommu.c
@@ -1016,8 +1016,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));
@@ -1025,10 +1024,10 @@ static u32 iotlb_init_entry(struct iotlb_entry *e, u32
da, u32 pa,
e-pa = pa;
e-valid= 1;
/* 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);
}
@@ -1041,7 +1040,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) {
@@ -1051,9 +1050,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
--
To unsubscribe from this list: send the line unsubscribe linux-omap in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
