From mboxrd@z Thu Jan 1 00:00:00 1970 From: Suman Anna Subject: Re: [PATCH 5/5] iommu/omap: Fix map protection value handling Date: Fri, 14 Mar 2014 19:16:53 -0500 Message-ID: <53239BF5.1000006@ti.com> References: <1394239574-2389-1-git-send-email-laurent.pinchart@ideasonboard.com> <1394239574-2389-6-git-send-email-laurent.pinchart@ideasonboard.com> <53224845.1050901@ti.com> <13414141.7HHPA8i3Vs@avalon> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <13414141.7HHPA8i3Vs@avalon> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Laurent Pinchart Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, linux-omap-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, sakari.ailus-X3B1VOXEql0@public.gmane.org, Florian Vaussard List-Id: iommu@lists.linux-foundation.org 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 >>> --- >>> >>> 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