Varun Sethi wrote:

I think you can move most of macros in this .h file into one of the .c files.

The code in this file is generally hard to read because you

1) have very few comments
2) you have a lot of expressions that span several lines, like this one:

		if ((iova >= geom_attr->aperture_start &&
			iova < geom_attr->aperture_end - 1 &&
			size <= geom_size) &&
			!align_check) {

Try adding a few more comments (e.g. each function should be commented)
and maybe try to break up a few of these lines.

You don't use this macro.

+
+#include <linux/init.h>
+#include <linux/iommu.h>
+#include <linux/notifier.h>
+#include <linux/slab.h>
+#include <linux/module.h>
+#include <linux/types.h>
+#include <linux/mm.h>
+#include <linux/interrupt.h>
+#include <linux/device.h>
+#include <linux/of_platform.h>
+#include <linux/bootmem.h>
+#include <linux/err.h>
+#include <asm/io.h>
+#include <asm/bitops.h>
+
+#include "fsl_pamu_domain.h"
+
+/* This bitmap advertises the page sizes supported by PAMU hardware
+ * to the IOMMU API.
+ */
+#define FSL_PAMU_PGSIZES	(~0xFFFUL)
+
+/* global spinlock that needs to be held while
+ * configuring PAMU.
+ */
+static DEFINE_SPINLOCK(iommu_lock);
+
+static struct kmem_cache *fsl_pamu_domain_cache;
+static struct kmem_cache *iommu_devinfo_cache;
+static DEFINE_SPINLOCK(device_domain_lock);
+
+int __init iommu_init_mempool(void)
+{
+
+	fsl_pamu_domain_cache = kmem_cache_create("fsl_pamu_domain",
+					 sizeof(struct fsl_dma_domain),
+					 0,
+					 SLAB_HWCACHE_ALIGN,
+
+					 NULL);
+	if (!fsl_pamu_domain_cache) {
+		pr_err("Couldn't create fsl iommu_domain cache\n");

ALL of these pr_xxx functions (in the entire file) need to have a
"fsl-pamu-domain:" prefix.  Maybe that's why you created pr_fmt(), but you
forgot to use it.

The above message should look like this:

	pr_err("fsl-pamu-domain: could not create iommu domain cache\n");

+		return -ENOMEM;
+	}
+
+	iommu_devinfo_cache = kmem_cache_create("iommu_devinfo",
+					 sizeof(struct device_domain_info),
+					 0,
+					 SLAB_HWCACHE_ALIGN,
+					 NULL);
+	if (!iommu_devinfo_cache) {
+		pr_err("Couldn't create devinfo cache\n");
+		kmem_cache_destroy(fsl_pamu_domain_cache);
+		return -ENOMEM;
+	}
+
+	return 0;
+}
+
+
+static int reconfig_win(int liodn, struct fsl_dma_domain *domain)
+{
+	int ret;
+
+	spin_lock(&iommu_lock);
+	ret = pamu_config_ppaace(liodn, domain->mapped_iova,
+				 domain->mapped_size,
+				 ~(u32)0,
+				 domain->paddr >> PAMU_PAGE_SHIFT,
+				 domain->snoop_id, domain->stash_id,
+				 0, domain->prot);
+	spin_unlock(&iommu_lock);
+	if (ret) {
+		pr_err("PAMU PAACE configuration failed for liodn %d\n",
+			 liodn);
+	}
+	return ret;
+}
+
+static void update_domain_subwin(struct fsl_dma_domain *dma_domain,
+				unsigned long iova, size_t size,
+				phys_addr_t paddr, int prot, int status)
+{
+	struct iommu_domain *domain = dma_domain->iommu_domain;
+	u32 subwin_cnt = dma_domain->subwin_cnt;
+	dma_addr_t geom_size = dma_domain->geom_size;
+	u32 subwin_size;
+	u32 mapped_subwin;
+	u32 mapped_subwin_cnt;
+	struct dma_subwindow *sub_win_ptr;
+	int i;
+
+	subwin_size = geom_size >> ilog2(subwin_cnt);
+	mapped_subwin = (iova - domain->geometry.aperture_start)
+				 >> ilog2(subwin_size);
+	sub_win_ptr = &dma_domain->sub_win_arr[mapped_subwin];
+	mapped_subwin_cnt = (size < subwin_size) ? 1 :
+				size >> ilog2(subwin_size);
+	for (i = 0; i < mapped_subwin_cnt; i++) {
+		if (status) {
+			sub_win_ptr[i].paddr = paddr;
+			sub_win_ptr[i].size = (size < subwin_size) ?
+						 size : subwin_size;
+			paddr += subwin_size;
+			sub_win_ptr[i].iova = iova;
+			iova += subwin_size;
+		}
+		sub_win_ptr[i].valid = status;
+		sub_win_ptr[i].prot = prot;
+	}
+
+	dma_domain->mapped_subwin = mapped_subwin;
+	dma_domain->mapped_subwin_cnt =  mapped_subwin_cnt;
+}
+
+static int reconfig_subwin(int liodn, struct fsl_dma_domain *dma_domain)
+{
+	u32 subwin_cnt = dma_domain->subwin_cnt;
+	int ret = 0;
+	u32 mapped_subwin;
+	u32 mapped_subwin_cnt;
+	struct dma_subwindow *sub_win_ptr;
+	unsigned long rpn;
+	int i;
+
+	mapped_subwin = dma_domain->mapped_subwin;
+	mapped_subwin_cnt = dma_domain->mapped_subwin_cnt;
+	sub_win_ptr = &dma_domain->sub_win_arr[mapped_subwin];
+
+	for (i = 0; i < mapped_subwin_cnt; i++) {
+		rpn = sub_win_ptr[i].paddr >> PAMU_PAGE_SHIFT,
+	
+		spin_lock(&iommu_lock);
+		ret = pamu_config_spaace(liodn, subwin_cnt, mapped_subwin,
+					 sub_win_ptr[i].size,
+					 ~(u32)0, 
+					 rpn, dma_domain->snoop_id,
+					 dma_domain->stash_id, 
+					 (mapped_subwin == 0 &&
+					 !dma_domain->enabled) ?
+					 0 : sub_win_ptr[i].valid,

Break out this expression into another variable.  This code is illegible.

	int enabled = 0;
	
	for (...
		if (mapped_subwin || dma_domain->enabled)
			enabled = sub_win_ptr[i].valid;

+					 sub_win_ptr[i].prot);
+		spin_unlock(&iommu_lock);
+		if (ret) {
+			pr_err("PAMU SPAACE configuration failed for liodn %d\n",liodn);
+			return ret;
+		}
+		mapped_subwin++;
+	}
+
+	return ret;
+}
+
+static phys_addr_t get_phys_addr(struct fsl_dma_domain *dma_domain, unsigned long iova)
+{
+	u32 subwin_cnt = dma_domain->subwin_cnt;
+
+	if (subwin_cnt) {
+		int i;
+		struct dma_subwindow *sub_win_ptr = 
+					&dma_domain->sub_win_arr[0];
+
+		for (i = 0; i < subwin_cnt; i++) {
+			if (sub_win_ptr[i].valid &&
+			    iova >= sub_win_ptr[i].iova &&
+			    iova < (sub_win_ptr[i].iova +
+			    sub_win_ptr[i].size - 1))
+				return (sub_win_ptr[i].paddr + (iova &
+					(sub_win_ptr[i].size - 1)));
+		}
+	} else {
+		return (dma_domain->paddr + (iova & (dma_domain->mapped_size - 1)));
+	}
+
+	return 0;
+}
+
+static int map_liodn_subwins(int liodn, struct fsl_dma_domain *dma_domain, u32 subwin_cnt)
+{
+	struct dma_subwindow *sub_win_ptr = 
+				&dma_domain->sub_win_arr[0];
+	int i, ret;
+	unsigned long rpn;
+
+	for (i = 0; i < subwin_cnt; i++) {
+		if (sub_win_ptr[i].valid) {
+			rpn = sub_win_ptr[i].paddr >>
+				 PAMU_PAGE_SHIFT,
+			spin_lock(&iommu_lock);
+			ret = pamu_config_spaace(liodn, subwin_cnt, i,
+						 sub_win_ptr[i].size,
+						 ~(u32)0,
+						 rpn,
+						 dma_domain->snoop_id,
+						 dma_domain->stash_id,
+						 (i > 0) ? 1 : 0,
+						 sub_win_ptr[i].prot);
+			spin_unlock(&iommu_lock);
+			if (ret) {
+				pr_err("PAMU SPAACE configuration failed for liodn %d\n",
+					 liodn);
+				return ret;
+			}
+		}
+	}
+
+	return ret;
+}
+
+static int map_liodn_win(int liodn, struct fsl_dma_domain *dma_domain)
+{
+	unsigned long rpn;
+	int ret;
+
+	rpn = dma_domain->paddr >> PAMU_PAGE_SHIFT;
+	spin_lock(&iommu_lock);
+	ret = pamu_config_ppaace(liodn, dma_domain->mapped_iova,
+				 dma_domain->mapped_size,
+				 ~(u32)0,
+				 rpn,
+				dma_domain->snoop_id, dma_domain->stash_id,
+				0, dma_domain->prot);

Indentation problem

+	spin_unlock(&iommu_lock);
+	if (ret)
+		pr_err("PAMU PAACE configuration failed for liodn %d\n",
+			liodn);
+
+	return ret;
+}
+
+static int map_liodn(int liodn, struct fsl_dma_domain *dma_domain)
+{
+	u32 subwin_cnt = dma_domain->subwin_cnt;
+
+	if (subwin_cnt)
+		return map_liodn_subwins(liodn, dma_domain, subwin_cnt);
+	else
+		return map_liodn_win(liodn, dma_domain);
+
+}
+
+static int update_liodn(int liodn, struct fsl_dma_domain *dma_domain)
+{
+	int ret;
+
+	if (dma_domain->subwin_cnt) {
+		ret = reconfig_subwin(liodn, dma_domain);
+		if (ret)
+			pr_err("Subwindow reconfiguration failed for liodn %d\n", liodn);
+	} else {
+		ret = reconfig_win(liodn, dma_domain);
+		if (ret)
+			pr_err("Window reconfiguration failed for liodn %d\n", liodn);
+	}
+
+	return ret;
+}
+
+static int update_liodn_stash(int liodn, struct fsl_dma_domain *dma_domain,
+				 u32 val)
+{
+	int ret = 0, i;
+
+	spin_lock(&iommu_lock);
+	if (!dma_domain->subwin_cnt) {
+		ret = pamu_update_paace_stash(liodn, 0, val);
+		if (ret) {
+			pr_err("Failed to update PAACE field for liodn %d\n ", liodn);
+			spin_unlock(&iommu_lock);
+			return ret;
+		}
+	} else {
+		for (i = 0; i < dma_domain->subwin_cnt; i++) {
+			ret = pamu_update_paace_stash(liodn, i, val);
+			if (ret) {
+				pr_err("Failed to update SPAACE %d field for liodn %d\n ", i, liodn);
+				spin_unlock(&iommu_lock);
+				return ret;
+			}
+		}
+	}
+	spin_unlock(&iommu_lock);
+
+	return ret;
+}
+
+static int configure_liodn(int liodn, struct device *dev,
+			   struct fsl_dma_domain *dma_domain,
+			   struct iommu_domain_geometry *geom_attr,
+			   u32 subwin_cnt)
+{
+	phys_addr_t window_addr, window_size;
+	phys_addr_t subwin_size;
+	int ret = 0, i;
+	u32 omi_index = ~(u32)0;
+
+	/* Configure the omi_index at the geometry setup time.
+	 * This is a static value which depends on the type of
+	 * device and would not change thereafter.
+	 */

	/*
	 * multi-line comments
	 * look like this
	 */

-- 
Timur Tabi
Linux kernel developer at Freescale