RE: [PATCH 3/3 v3] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.

-----Original Message-----
From: Wood Scott-B07421
Sent: Tuesday, October 23, 2012 5:23 AM
To: Tabi Timur-B04825
Cc: Sethi Varun-B16395; <redacted>; <redacted>
foundation.org; linuxppc-dev@lists.ozlabs.org; linux-
kernel@vger.kernel.org
Subject: Re: [PATCH 3/3 v3] iommu/fsl: Freescale PAMU driver and IOMMU
API implementation.
=20
On 10/22/2012 04:18:07 PM, Tabi Timur-B04825 wrote:

quoted

On Wed, Oct 17, 2012 at 12:32 PM, Varun Sethi
[off-list ref] wrote:

quoted

+}
+
+static unsigned long pamu_get_fspi_and_allocate(u32 subwin_cnt) {

subwin_cnt should probably be an unsigned int.

This function needs to be documented.  What value is being returned?

=20
spaact offset (yes, this needs to be documented)

[Sethi Varun-B16395] Ok.

=20

quoted

quoted

+/* This bitmap advertises the page sizes supported by PAMU hardware
+ * to the IOMMU API.
+ */
+#define FSL_PAMU_PGSIZES       (~0xFFFUL)

There should be a better way to define this.  ~(PAMU_PAGE_SIZE-1)
maybe?

=20
Is it even true?  We don't support IOMMU pages larger than the SoC can
address.
=20
The (~0xFFFUL) version also discards some valid IOMMU page sizes on 32-
bit kernels.  One use case for windows larger than the CPU virtual
address space is creating one big identity-map window to effectively
disable translation.  If we're to support that, the size of pgsize_bitmap
will need to change as well.
=20

[Sethi Varun-B16395] Correct, this needs to be fixed. I will try to address=
 this
In a separate patch (would require changes to iommu_map).

quoted

quoted

+static int map_liodn(int liodn, struct fsl_dma_domain *dma_domain)
+{
+       u32 subwin_cnt =3D dma_domain->subwin_cnt;
+       unsigned long rpn;
+       int ret =3D 0, i;
+
+       if (subwin_cnt) {
+               struct dma_subwindow *sub_win_ptr =3D
+                                       &dma_domain->sub_win_arr[0];
+               for (i =3D 0; i < subwin_cnt; i++) {
+                       if (sub_win_ptr[i].valid) {
+                               rpn =3D sub_win_ptr[i].paddr >>
+                                        PAMU_PAGE_SHIFT,
+                               spin_lock(&iommu_lock);
+                               ret =3D pamu_config_spaace(liodn,

subwin_cnt, i,

quoted

+

sub_win_ptr[i].size,

quoted

+                                                        -1,
+                                                        rpn,
+

dma_domain->snoop_id,

quoted

+

dma_domain->stash_id,

quoted

+                                                        (i > 0) ?

1 : 0,

quoted

+

sub_win_ptr[i].prot);

quoted

+                               spin_unlock(&iommu_lock);
+                               if (ret) {
+                                       pr_err("PAMU SPAACE

configuration failed for liodn %d\n",

quoted

+                                                liodn);
+                                       return ret;
+                               }
+                       }
+               }

=20
Break up that nesting with some subfunctions.
=20

quoted

quoted

+       while (!list_empty(&dma_domain->devices)) {
+               info =3D list_entry(dma_domain->devices.next,
+                       struct device_domain_info, link);
+               remove_domain_ref(info, dma_domain->subwin_cnt);
+       }

I wonder if you should use list_for_each_safe() instead.

=20
The above is simpler if you're destroying the entire list.
=20

quoted

quoted

+}
+
+static int configure_domain_dma_state(struct fsl_dma_domain

*dma_domain, int enable)

bool enable

Finally, please CC: me on all IOMMU and PAMU patches you post
upstream.

=20
Me too.

[Sethi Varun-B16395] Sure.

-Varun