RE: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
> -Original Message-
> From: Sethi Varun-B16395
> Sent: Monday, March 04, 2013 5:31 AM
> To: Stuart Yoder
> Cc: io...@lists.linux-foundation.org; linuxppc-...@lists.ozlabs.org;
> linux-kernel@vger.kernel.org; Wood
> Scott-B07421; Joerg Roedel; Yoder Stuart-B08248
> Subject: RE: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API
> implementation.
>
>
>
> > -Original Message-
> > From: Stuart Yoder [mailto:b08...@gmail.com]
> > Sent: Saturday, March 02, 2013 4:58 AM
> > To: Sethi Varun-B16395
> > Cc: io...@lists.linux-foundation.org; linuxppc-...@lists.ozlabs.org;
> > linux-kernel@vger.kernel.org; Wood Scott-B07421; Joerg Roedel; Yoder
> > Stuart-B08248
> > Subject: Re: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU
> > API implementation.
> >
> > On Mon, Feb 18, 2013 at 6:52 AM, Varun Sethi
> > wrote:
> > [cut]
> > > +static phys_addr_t get_phys_addr(struct fsl_dma_domain *dma_domain,
> > > +unsigned long iova) {
> > > + u32 win_cnt = dma_domain->win_cnt;
> > > + struct dma_window *win_ptr =
> > > + _domain->win_arr[0];
> > > + struct iommu_domain_geometry *geom;
> > > +
> > > + geom = _domain->iommu_domain->geometry;
> > > +
> > > + if (!win_cnt || !dma_domain->geom_size) {
> > > + pr_err("Number of windows/geometry not configured for
> > the domain\n");
> > > + return 0;
> > > + }
> > > +
> > > + if (win_cnt > 1) {
> > > + u64 subwin_size;
> > > + unsigned long subwin_iova;
> > > + u32 wnd;
> > > +
> > > + subwin_size = dma_domain->geom_size >> ilog2(win_cnt);
> >
> > Could it be just geom_size / win_cnt ??
> [Sethi Varun-B16395] You would run in to linking issues with respect to u64
> division on 32 bit kernels.
>
> >
> > > + subwin_iova = iova & ~(subwin_size - 1);
> > > + wnd = (subwin_iova - geom->aperture_start) >>
> > ilog2(subwin_size);
> > > + win_ptr = _domain->win_arr[wnd];
> > > + }
> > > +
> > > + if (win_ptr->valid)
> > > + return (win_ptr->paddr + (iova & (win_ptr->size -
> > > + 1)));
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int map_liodn_subwins(int liodn, struct fsl_dma_domain
> > > +*dma_domain)
> >
> > Just call it map_subwins(). They are just sub windows, not "liodn sub
> > windows".
> >
> [Sethi Varun-B16395] This would be called per liodn.
>
> > [cut]
> >
> > > +static int map_liodn_win(int liodn, struct fsl_dma_domain
> > > +*dma_domain)
> >
> > Call it map_win().
> [Sethi Varun-B16395] This would again be called per liodn.
On the function names-- function names are typically named
with a noun describing some object and a verb describing
the action...and sometimes a subsystem identifier:
kmem_cache + zalloc
spin + lock
I don't think inserting liodn in the function name to indicates that it
operates per liodn makes it more clear and reads a little awkward to me:
map + liodn_win
...it's almost like there is a "liodn_win" object.
I think plain map_win() is more clear and the function prototype makes
it pretty clear that you are operating on an liodn.
> > [cut]
> > > +static bool check_pci_ctl_endpt_part(struct pci_controller *pci_ctl)
> > > +{
> > > + u32 version;
> > > +
> > > + /* Check the PCI controller version number by readding BRR1
> > register */
> > > + version = in_be32(pci_ctl->cfg_addr + (PCI_FSL_BRR1 >> 2));
> > > + version &= PCI_FSL_BRR1_VER;
> > > + /* If PCI controller version is >= 0x204 we can partition
> > endpoints*/
> > > + if (version >= 0x204)
> > > + return 1;
> > > +
> > > + return 0;
> > > +}
> >
> > Can we just use the compatible string here to identify the different
> > kinds of PCI
> > controllers? Reading the actual device registers is ugly right now
> > because
> > you are #defining the BRR1 stuff in a generic powerpc header.
> >
> [Sethi Varun-B16395] hmmm.., I would have to check for various different
> compatible strings in that
> case. May be I can move the defines to a different file. What if I move them
> to some other header?
Don't personally feel too strongly about version register or header...but it
should be some fsl
PCI header that you define those regs.
You'll either need to check for a hardware version number or a compatible,
so a compatible check may be just as easy...look for "fsl,qoriq-pcie-v2.4"?
Stuart
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
> -Original Message-
> From: Stuart Yoder [mailto:b08...@gmail.com]
> Sent: Saturday, March 02, 2013 4:58 AM
> To: Sethi Varun-B16395
> Cc: io...@lists.linux-foundation.org; linuxppc-...@lists.ozlabs.org;
> linux-kernel@vger.kernel.org; Wood Scott-B07421; Joerg Roedel; Yoder
> Stuart-B08248
> Subject: Re: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU
> API implementation.
>
> On Mon, Feb 18, 2013 at 6:52 AM, Varun Sethi
> wrote:
> [cut]
> > +static phys_addr_t get_phys_addr(struct fsl_dma_domain *dma_domain,
> > +unsigned long iova) {
> > + u32 win_cnt = dma_domain->win_cnt;
> > + struct dma_window *win_ptr =
> > + _domain->win_arr[0];
> > + struct iommu_domain_geometry *geom;
> > +
> > + geom = _domain->iommu_domain->geometry;
> > +
> > + if (!win_cnt || !dma_domain->geom_size) {
> > + pr_err("Number of windows/geometry not configured for
> the domain\n");
> > + return 0;
> > + }
> > +
> > + if (win_cnt > 1) {
> > + u64 subwin_size;
> > + unsigned long subwin_iova;
> > + u32 wnd;
> > +
> > + subwin_size = dma_domain->geom_size >> ilog2(win_cnt);
>
> Could it be just geom_size / win_cnt ??
[Sethi Varun-B16395] You would run in to linking issues with respect to u64
division on 32 bit kernels.
>
> > + subwin_iova = iova & ~(subwin_size - 1);
> > + wnd = (subwin_iova - geom->aperture_start) >>
> ilog2(subwin_size);
> > + win_ptr = _domain->win_arr[wnd];
> > + }
> > +
> > + if (win_ptr->valid)
> > + return (win_ptr->paddr + (iova & (win_ptr->size -
> > + 1)));
> > +
> > + return 0;
> > +}
> > +
> > +static int map_liodn_subwins(int liodn, struct fsl_dma_domain
> > +*dma_domain)
>
> Just call it map_subwins(). They are just sub windows, not "liodn sub
> windows".
>
[Sethi Varun-B16395] This would be called per liodn.
> [cut]
>
> > +static int map_liodn_win(int liodn, struct fsl_dma_domain
> > +*dma_domain)
>
> Call it map_win().
[Sethi Varun-B16395] This would again be called per liodn.
>
> [cut]
> > +static struct fsl_dma_domain *iommu_alloc_dma_domain(void) {
> > + struct fsl_dma_domain *domain;
> > +
> > + domain = kmem_cache_zalloc(fsl_pamu_domain_cache, GFP_KERNEL);
> > + if (!domain)
> > + return NULL;
> > +
> > + domain->stash_id = ~(u32)0;
> > + domain->snoop_id = ~(u32)0;
> > + domain->win_cnt = max_subwindow_count;
>
> To align with my previous comments on fsl_pamu.c, I think instead of
> referencing a global variable (in fsl_pamu.c) you should be making an
> accessor API call here to get the max subwindow _count.
>
[Sethi Varun-B16395] ok, I will make a accessor API call.
> > + domain->geom_size = 0;
> > +
> > + INIT_LIST_HEAD(>devices);
> > +
> > + spin_lock_init(>domain_lock);
> > +
> > + return domain;
> > +}
> > +
> > +static inline struct device_domain_info *find_domain(struct device
> > +*dev) {
> > + return dev->archdata.iommu_domain; }
> > +
> > +static void remove_domain_ref(struct device_domain_info *info, u32
> > +win_cnt) {
> > + list_del(>link);
> > + spin_lock(_lock);
> > + if (win_cnt)
> > + pamu_free_subwins(info->liodn);
> > + pamu_disable_liodn(info->liodn);
> > + spin_unlock(_lock);
> > + spin_lock(_domain_lock);
> > + info->dev->archdata.iommu_domain = NULL;
> > + kmem_cache_free(iommu_devinfo_cache, info);
> > + spin_unlock(_domain_lock); }
>
> The above function is literally removing the _device_ reference from the
> domain.
> The name implies that it is removing a "domain reference". Suggestion
> is
> to call it "remove_device_ref".
>
> Also, the whitespace is messed up there. You have 2 tabs instead of 1.
[Sethi Varun-B16395] ok will make the change.
>
> > +static void destroy_domain(struct fsl_dma_domain *dma_domain) {
> > + struct device_domain_info *info;
> > +
> > + /* Dissociate all the devices from this domain */
> > + while (!list_empty(_domain->devices
RE: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
-Original Message-
From: Stuart Yoder [mailto:b08...@gmail.com]
Sent: Saturday, March 02, 2013 4:58 AM
To: Sethi Varun-B16395
Cc: io...@lists.linux-foundation.org; linuxppc-...@lists.ozlabs.org;
linux-kernel@vger.kernel.org; Wood Scott-B07421; Joerg Roedel; Yoder
Stuart-B08248
Subject: Re: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU
API implementation.
On Mon, Feb 18, 2013 at 6:52 AM, Varun Sethi varun.se...@freescale.com
wrote:
[cut]
+static phys_addr_t get_phys_addr(struct fsl_dma_domain *dma_domain,
+unsigned long iova) {
+ u32 win_cnt = dma_domain-win_cnt;
+ struct dma_window *win_ptr =
+ dma_domain-win_arr[0];
+ struct iommu_domain_geometry *geom;
+
+ geom = dma_domain-iommu_domain-geometry;
+
+ if (!win_cnt || !dma_domain-geom_size) {
+ pr_err(Number of windows/geometry not configured for
the domain\n);
+ return 0;
+ }
+
+ if (win_cnt 1) {
+ u64 subwin_size;
+ unsigned long subwin_iova;
+ u32 wnd;
+
+ subwin_size = dma_domain-geom_size ilog2(win_cnt);
Could it be just geom_size / win_cnt ??
[Sethi Varun-B16395] You would run in to linking issues with respect to u64
division on 32 bit kernels.
+ subwin_iova = iova ~(subwin_size - 1);
+ wnd = (subwin_iova - geom-aperture_start)
ilog2(subwin_size);
+ win_ptr = dma_domain-win_arr[wnd];
+ }
+
+ if (win_ptr-valid)
+ return (win_ptr-paddr + (iova (win_ptr-size -
+ 1)));
+
+ return 0;
+}
+
+static int map_liodn_subwins(int liodn, struct fsl_dma_domain
+*dma_domain)
Just call it map_subwins(). They are just sub windows, not liodn sub
windows.
[Sethi Varun-B16395] This would be called per liodn.
[cut]
+static int map_liodn_win(int liodn, struct fsl_dma_domain
+*dma_domain)
Call it map_win().
[Sethi Varun-B16395] This would again be called per liodn.
[cut]
+static struct fsl_dma_domain *iommu_alloc_dma_domain(void) {
+ struct fsl_dma_domain *domain;
+
+ domain = kmem_cache_zalloc(fsl_pamu_domain_cache, GFP_KERNEL);
+ if (!domain)
+ return NULL;
+
+ domain-stash_id = ~(u32)0;
+ domain-snoop_id = ~(u32)0;
+ domain-win_cnt = max_subwindow_count;
To align with my previous comments on fsl_pamu.c, I think instead of
referencing a global variable (in fsl_pamu.c) you should be making an
accessor API call here to get the max subwindow _count.
[Sethi Varun-B16395] ok, I will make a accessor API call.
+ domain-geom_size = 0;
+
+ INIT_LIST_HEAD(domain-devices);
+
+ spin_lock_init(domain-domain_lock);
+
+ return domain;
+}
+
+static inline struct device_domain_info *find_domain(struct device
+*dev) {
+ return dev-archdata.iommu_domain; }
+
+static void remove_domain_ref(struct device_domain_info *info, u32
+win_cnt) {
+ list_del(info-link);
+ spin_lock(iommu_lock);
+ if (win_cnt)
+ pamu_free_subwins(info-liodn);
+ pamu_disable_liodn(info-liodn);
+ spin_unlock(iommu_lock);
+ spin_lock(device_domain_lock);
+ info-dev-archdata.iommu_domain = NULL;
+ kmem_cache_free(iommu_devinfo_cache, info);
+ spin_unlock(device_domain_lock); }
The above function is literally removing the _device_ reference from the
domain.
The name implies that it is removing a domain reference. Suggestion
is
to call it remove_device_ref.
Also, the whitespace is messed up there. You have 2 tabs instead of 1.
[Sethi Varun-B16395] ok will make the change.
+static void destroy_domain(struct fsl_dma_domain *dma_domain) {
+ struct device_domain_info *info;
+
+ /* Dissociate all the devices from this domain */
+ while (!list_empty(dma_domain-devices)) {
+ info = list_entry(dma_domain-devices.next,
+ struct device_domain_info, link);
+ remove_domain_ref(info, dma_domain-win_cnt);
+ }
+}
This function is removing all devices from a domain...maybe to be
consistent with my suggestion below on detach_domain(), call this
detach_all_devices().
We have 2 functions
doing almost the same thingone detaches a single device, one detaches
all devices.
The current names destroy_domain and detach_domain are not as clear
to me.
[Sethi Varun-B16395]ok
+static void detach_domain(struct device *dev, struct fsl_dma_domain
+*dma_domain) {
+ struct device_domain_info *info;
+ struct list_head *entry, *tmp;
+ unsigned long flags;
+
+ spin_lock_irqsave(dma_domain-domain_lock, flags);
+ /* Remove
RE: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
-Original Message-
From: Sethi Varun-B16395
Sent: Monday, March 04, 2013 5:31 AM
To: Stuart Yoder
Cc: io...@lists.linux-foundation.org; linuxppc-...@lists.ozlabs.org;
linux-kernel@vger.kernel.org; Wood
Scott-B07421; Joerg Roedel; Yoder Stuart-B08248
Subject: RE: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API
implementation.
-Original Message-
From: Stuart Yoder [mailto:b08...@gmail.com]
Sent: Saturday, March 02, 2013 4:58 AM
To: Sethi Varun-B16395
Cc: io...@lists.linux-foundation.org; linuxppc-...@lists.ozlabs.org;
linux-kernel@vger.kernel.org; Wood Scott-B07421; Joerg Roedel; Yoder
Stuart-B08248
Subject: Re: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU
API implementation.
On Mon, Feb 18, 2013 at 6:52 AM, Varun Sethi varun.se...@freescale.com
wrote:
[cut]
+static phys_addr_t get_phys_addr(struct fsl_dma_domain *dma_domain,
+unsigned long iova) {
+ u32 win_cnt = dma_domain-win_cnt;
+ struct dma_window *win_ptr =
+ dma_domain-win_arr[0];
+ struct iommu_domain_geometry *geom;
+
+ geom = dma_domain-iommu_domain-geometry;
+
+ if (!win_cnt || !dma_domain-geom_size) {
+ pr_err(Number of windows/geometry not configured for
the domain\n);
+ return 0;
+ }
+
+ if (win_cnt 1) {
+ u64 subwin_size;
+ unsigned long subwin_iova;
+ u32 wnd;
+
+ subwin_size = dma_domain-geom_size ilog2(win_cnt);
Could it be just geom_size / win_cnt ??
[Sethi Varun-B16395] You would run in to linking issues with respect to u64
division on 32 bit kernels.
+ subwin_iova = iova ~(subwin_size - 1);
+ wnd = (subwin_iova - geom-aperture_start)
ilog2(subwin_size);
+ win_ptr = dma_domain-win_arr[wnd];
+ }
+
+ if (win_ptr-valid)
+ return (win_ptr-paddr + (iova (win_ptr-size -
+ 1)));
+
+ return 0;
+}
+
+static int map_liodn_subwins(int liodn, struct fsl_dma_domain
+*dma_domain)
Just call it map_subwins(). They are just sub windows, not liodn sub
windows.
[Sethi Varun-B16395] This would be called per liodn.
[cut]
+static int map_liodn_win(int liodn, struct fsl_dma_domain
+*dma_domain)
Call it map_win().
[Sethi Varun-B16395] This would again be called per liodn.
On the function names-- function names are typically named
with a noun describing some object and a verb describing
the action...and sometimes a subsystem identifier:
kmem_cache + zalloc
spin + lock
I don't think inserting liodn in the function name to indicates that it
operates per liodn makes it more clear and reads a little awkward to me:
map + liodn_win
...it's almost like there is a liodn_win object.
I think plain map_win() is more clear and the function prototype makes
it pretty clear that you are operating on an liodn.
[cut]
+static bool check_pci_ctl_endpt_part(struct pci_controller *pci_ctl)
+{
+ u32 version;
+
+ /* Check the PCI controller version number by readding BRR1
register */
+ version = in_be32(pci_ctl-cfg_addr + (PCI_FSL_BRR1 2));
+ version = PCI_FSL_BRR1_VER;
+ /* If PCI controller version is = 0x204 we can partition
endpoints*/
+ if (version = 0x204)
+ return 1;
+
+ return 0;
+}
Can we just use the compatible string here to identify the different
kinds of PCI
controllers? Reading the actual device registers is ugly right now
because
you are #defining the BRR1 stuff in a generic powerpc header.
[Sethi Varun-B16395] hmmm.., I would have to check for various different
compatible strings in that
case. May be I can move the defines to a different file. What if I move them
to some other header?
Don't personally feel too strongly about version register or header...but it
should be some fsl
PCI header that you define those regs.
You'll either need to check for a hardware version number or a compatible,
so a compatible check may be just as easy...look for fsl,qoriq-pcie-v2.4?
Stuart
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
On Mon, Feb 18, 2013 at 6:52 AM, Varun Sethi wrote:
[cut]
> +static phys_addr_t get_phys_addr(struct fsl_dma_domain *dma_domain, unsigned
> long iova)
> +{
> + u32 win_cnt = dma_domain->win_cnt;
> + struct dma_window *win_ptr =
> + _domain->win_arr[0];
> + struct iommu_domain_geometry *geom;
> +
> + geom = _domain->iommu_domain->geometry;
> +
> + if (!win_cnt || !dma_domain->geom_size) {
> + pr_err("Number of windows/geometry not configured for the
> domain\n");
> + return 0;
> + }
> +
> + if (win_cnt > 1) {
> + u64 subwin_size;
> + unsigned long subwin_iova;
> + u32 wnd;
> +
> + subwin_size = dma_domain->geom_size >> ilog2(win_cnt);
Could it be just geom_size / win_cnt ??
> + subwin_iova = iova & ~(subwin_size - 1);
> + wnd = (subwin_iova - geom->aperture_start) >>
> ilog2(subwin_size);
> + win_ptr = _domain->win_arr[wnd];
> + }
> +
> + if (win_ptr->valid)
> + return (win_ptr->paddr + (iova & (win_ptr->size - 1)));
> +
> + return 0;
> +}
> +
> +static int map_liodn_subwins(int liodn, struct fsl_dma_domain *dma_domain)
Just call it map_subwins(). They are just sub windows, not "liodn sub windows".
[cut]
> +static int map_liodn_win(int liodn, struct fsl_dma_domain *dma_domain)
Call it map_win().
[cut]
> +static struct fsl_dma_domain *iommu_alloc_dma_domain(void)
> +{
> + struct fsl_dma_domain *domain;
> +
> + domain = kmem_cache_zalloc(fsl_pamu_domain_cache, GFP_KERNEL);
> + if (!domain)
> + return NULL;
> +
> + domain->stash_id = ~(u32)0;
> + domain->snoop_id = ~(u32)0;
> + domain->win_cnt = max_subwindow_count;
To align with my previous comments on fsl_pamu.c, I think instead of referencing
a global variable (in fsl_pamu.c) you should be making an accessor API
call here to get
the max subwindow _count.
> + domain->geom_size = 0;
> +
> + INIT_LIST_HEAD(>devices);
> +
> + spin_lock_init(>domain_lock);
> +
> + return domain;
> +}
> +
> +static inline struct device_domain_info *find_domain(struct device *dev)
> +{
> + return dev->archdata.iommu_domain;
> +}
> +
> +static void remove_domain_ref(struct device_domain_info *info, u32 win_cnt)
> +{
> + list_del(>link);
> + spin_lock(_lock);
> + if (win_cnt)
> + pamu_free_subwins(info->liodn);
> + pamu_disable_liodn(info->liodn);
> + spin_unlock(_lock);
> + spin_lock(_domain_lock);
> + info->dev->archdata.iommu_domain = NULL;
> + kmem_cache_free(iommu_devinfo_cache, info);
> + spin_unlock(_domain_lock);
> +}
The above function is literally removing the _device_ reference from the domain.
The name implies that it is removing a "domain reference". Suggestion is
to call it "remove_device_ref".
Also, the whitespace is messed up there. You have 2 tabs instead of 1.
> +static void destroy_domain(struct fsl_dma_domain *dma_domain)
> +{
> + struct device_domain_info *info;
> +
> + /* Dissociate all the devices from this domain */
> + while (!list_empty(_domain->devices)) {
> + info = list_entry(dma_domain->devices.next,
> + struct device_domain_info, link);
> + remove_domain_ref(info, dma_domain->win_cnt);
> + }
> +}
This function is removing all devices from a domain...maybe to be
consistent with my
suggestion below on detach_domain(), call this detach_all_devices().
We have 2 functions
doing almost the same thingone detaches a single device, one
detaches all devices.
The current names "destroy_domain" and "detach_domain" are not as clear to me.
> +static void detach_domain(struct device *dev, struct fsl_dma_domain
> *dma_domain)
> +{
> + struct device_domain_info *info;
> + struct list_head *entry, *tmp;
> + unsigned long flags;
> +
> + spin_lock_irqsave(_domain->domain_lock, flags);
> + /* Remove the device from the domain device list */
> + if (!list_empty(_domain->devices)) {
> + list_for_each_safe(entry, tmp, _domain->devices) {
> + info = list_entry(entry, struct device_domain_info,
> link);
> + if (info->dev == dev)
> + remove_domain_ref(info, dma_domain->win_cnt);
> + }
> + }
> + spin_unlock_irqrestore(_domain->domain_lock, flags);
> +}
This function is not "detaching a domain", but is detaching a device.
Call it detach_device().
> +static void attach_domain(struct fsl_dma_domain *dma_domain, int liodn,
> struct device *dev)
> +{
Same thing here. This is not attaching a domain, but attaching a
device. Call it attach_device.
> + struct
Re: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
On Mon, Feb 18, 2013 at 6:52 AM, Varun Sethi varun.se...@freescale.com wrote:
[cut]
+static phys_addr_t get_phys_addr(struct fsl_dma_domain *dma_domain, unsigned
long iova)
+{
+ u32 win_cnt = dma_domain-win_cnt;
+ struct dma_window *win_ptr =
+ dma_domain-win_arr[0];
+ struct iommu_domain_geometry *geom;
+
+ geom = dma_domain-iommu_domain-geometry;
+
+ if (!win_cnt || !dma_domain-geom_size) {
+ pr_err(Number of windows/geometry not configured for the
domain\n);
+ return 0;
+ }
+
+ if (win_cnt 1) {
+ u64 subwin_size;
+ unsigned long subwin_iova;
+ u32 wnd;
+
+ subwin_size = dma_domain-geom_size ilog2(win_cnt);
Could it be just geom_size / win_cnt ??
+ subwin_iova = iova ~(subwin_size - 1);
+ wnd = (subwin_iova - geom-aperture_start)
ilog2(subwin_size);
+ win_ptr = dma_domain-win_arr[wnd];
+ }
+
+ if (win_ptr-valid)
+ return (win_ptr-paddr + (iova (win_ptr-size - 1)));
+
+ return 0;
+}
+
+static int map_liodn_subwins(int liodn, struct fsl_dma_domain *dma_domain)
Just call it map_subwins(). They are just sub windows, not liodn sub windows.
[cut]
+static int map_liodn_win(int liodn, struct fsl_dma_domain *dma_domain)
Call it map_win().
[cut]
+static struct fsl_dma_domain *iommu_alloc_dma_domain(void)
+{
+ struct fsl_dma_domain *domain;
+
+ domain = kmem_cache_zalloc(fsl_pamu_domain_cache, GFP_KERNEL);
+ if (!domain)
+ return NULL;
+
+ domain-stash_id = ~(u32)0;
+ domain-snoop_id = ~(u32)0;
+ domain-win_cnt = max_subwindow_count;
To align with my previous comments on fsl_pamu.c, I think instead of referencing
a global variable (in fsl_pamu.c) you should be making an accessor API
call here to get
the max subwindow _count.
+ domain-geom_size = 0;
+
+ INIT_LIST_HEAD(domain-devices);
+
+ spin_lock_init(domain-domain_lock);
+
+ return domain;
+}
+
+static inline struct device_domain_info *find_domain(struct device *dev)
+{
+ return dev-archdata.iommu_domain;
+}
+
+static void remove_domain_ref(struct device_domain_info *info, u32 win_cnt)
+{
+ list_del(info-link);
+ spin_lock(iommu_lock);
+ if (win_cnt)
+ pamu_free_subwins(info-liodn);
+ pamu_disable_liodn(info-liodn);
+ spin_unlock(iommu_lock);
+ spin_lock(device_domain_lock);
+ info-dev-archdata.iommu_domain = NULL;
+ kmem_cache_free(iommu_devinfo_cache, info);
+ spin_unlock(device_domain_lock);
+}
The above function is literally removing the _device_ reference from the domain.
The name implies that it is removing a domain reference. Suggestion is
to call it remove_device_ref.
Also, the whitespace is messed up there. You have 2 tabs instead of 1.
+static void destroy_domain(struct fsl_dma_domain *dma_domain)
+{
+ struct device_domain_info *info;
+
+ /* Dissociate all the devices from this domain */
+ while (!list_empty(dma_domain-devices)) {
+ info = list_entry(dma_domain-devices.next,
+ struct device_domain_info, link);
+ remove_domain_ref(info, dma_domain-win_cnt);
+ }
+}
This function is removing all devices from a domain...maybe to be
consistent with my
suggestion below on detach_domain(), call this detach_all_devices().
We have 2 functions
doing almost the same thingone detaches a single device, one
detaches all devices.
The current names destroy_domain and detach_domain are not as clear to me.
+static void detach_domain(struct device *dev, struct fsl_dma_domain
*dma_domain)
+{
+ struct device_domain_info *info;
+ struct list_head *entry, *tmp;
+ unsigned long flags;
+
+ spin_lock_irqsave(dma_domain-domain_lock, flags);
+ /* Remove the device from the domain device list */
+ if (!list_empty(dma_domain-devices)) {
+ list_for_each_safe(entry, tmp, dma_domain-devices) {
+ info = list_entry(entry, struct device_domain_info,
link);
+ if (info-dev == dev)
+ remove_domain_ref(info, dma_domain-win_cnt);
+ }
+ }
+ spin_unlock_irqrestore(dma_domain-domain_lock, flags);
+}
This function is not detaching a domain, but is detaching a device.
Call it detach_device().
+static void attach_domain(struct fsl_dma_domain *dma_domain, int liodn,
struct device *dev)
+{
Same thing here. This is not attaching a domain, but attaching a
device. Call it attach_device.
+ struct device_domain_info *info, *old_domain_info;
+
+ spin_lock(device_domain_lock);
Re: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
Some more comments...
On Mon, Feb 18, 2013 at 6:52 AM, Varun Sethi wrote:
> +/* Handling access violations */
> +#define make64(high, low) (((u64)(high) << 32) | (low))
> +
> +struct pamu_isr_data {
> + void __iomem *pamu_reg_base;/* Base address of PAMU regs*/
> + unsigned int count; /* The number of PAMUs */
> +};
> +
> +static struct paace *ppaact;
> +static struct paace *spaact;
> +static struct ome *omt;
> +
> +/* maximum subwindows permitted per liodn */
> +unsigned int max_subwindow_count;
> +/* Number of SPAACT entries */
> +unsigned long max_subwins;
I don't like that these variables are not static... and they are
referenced directly
from code in fsl_pamu_domain.c. It would be better if fsl_pamu_domain.c
called an accessor function-- like pamu_get_max_subwins.
> +/* Pool for fspi allocation */
> +struct gen_pool *spaace_pool;
spaace_pool should be static?
I'm wondering if you should change pamu_isr_data into a more
general struct analagous to struct intel_iommu. You could
put in there the max # of subwins, etc. You could then
provide an accessor to get at that data.
[cut]
> +/**
> + * pamu_get_fspi_and_allocate() - Allocates fspi index and reserves
> subwindows
> + *required for primary PAACE in the secondary
> + *PAACE table.
> + * @subwin_cnt: Number of subwindows to be reserved.
> + *
> + * A PPAACE entry may have a number of associated subwindows. A subwindow
> + * corresponds to a SPAACE entry in the SPAACT table. Each PAACE entry stores
> + * the index (fspi) of the first SPAACE entry in the SPAACT table. This
> + * function returns the index of the first SPAACE entry. The remaining
> + * SPAACE entries are reserved contiguously from that index.
> + *
> + * Returns a valid fspi index in the range of 0 - max_subwins on success.
> + * If no SPAACE entry is available or the allocator can not reserve the
> required
> + * number of contiguous entries function returns ULONG_MAX indicating a
> failure.
> + *
> +*/
> +static unsigned long pamu_get_fspi_and_allocate(u32 subwin_cnt)
> +{
> + unsigned long spaace_addr;
> +
> + spaace_addr = gen_pool_alloc(spaace_pool, subwin_cnt * sizeof(struct
> paace));
> + if (!spaace_addr)
> + return ULONG_MAX;
> +
> + return (spaace_addr - (unsigned long)spaact) / (sizeof(struct paace));
> +}
In order to keep things symmetric (with the free function) can we just
call the above
function:
pamu_alloc_subwins()
> +/* Release the subwindows reserved for a particular LIODN */
> +void pamu_free_subwins(int liodn)
> +{
> + struct paace *ppaace;
> + u32 subwin_cnt, size;
> +
> + ppaace = pamu_get_ppaace(liodn);
> + if (!ppaace) {
> + pr_err("Invalid liodn entry\n");
> + return;
> + }
> +
> + if (get_bf(ppaace->addr_bitfields, PPAACE_AF_MW)) {
> + subwin_cnt = 1UL << (get_bf(ppaace->impl_attr, PAACE_IA_WCE)
> + 1);
> + size = (subwin_cnt - 1) * sizeof(struct paace);
> + gen_pool_free(spaace_pool, (unsigned
> long)[ppaace->fspi], size);
> + set_bf(ppaace->addr_bitfields, PPAACE_AF_MW, 0);
> + }
> +}
[cut]
> +/**
> + * get_stash_id - Returns stash destination id corresponding to a
> + *cache type and vcpu.
> + * @stash_dest_hint: L1, L2 or L3
> + * @vcpu: vpcu target for a particular cache type.
> + *
> + * Returs stash on success or ~(u32)0 on failure.
> + *
> + */
> +u32 get_stash_id(u32 stash_dest_hint, u32 vcpu)
> +{
The stash dest is really not a hint, right? It's the requested stash
destination. So maybe just drop 'hint' from the name.
The CPU here is really a physical CPU number and has nothing to do
with vcpus I think. vcpu implies the index is inside a virtual machine...but
this API is generic and may or may not be used with KVM.
> +
> +/*
> + * Get the maximum number of PAACT table entries
> + * and subwindows supported by PAMU
> + */
> +static void get_pamu_cap_values(unsigned long pamu_reg_base)
> +{
> + u32 pc_val;
> +
> + pc_val = in_be32((u32 *)(pamu_reg_base + PAMU_PC3));
> + /* Maximum number of subwindows per liodn */
> + max_subwindow_count = 1 << (1 + PAMU_PC3_MWCE(pc_val));
> + /* Total number of SPACCT entries */
> + max_subwins = PAACE_NUMBER_ENTRIES * max_subwindow_count;
> +}
If you follow the suggestion at the top of this file, this function
would become something like-- init_pamu_capabilities(). And then
create an accessor function to access max subwins, etc.
Also, BTW, I don't see any support for the DOMAIN_ATTR_WINDOWS
attribute in your patch. Was that coming in a later patch?
[cut
> +static int __init fsl_pamu_probe(struct platform_device *pdev)
> +{
> + void __iomem *pamu_regs = NULL;
> + struct ccsr_guts __iomem *guts_regs = NULL;
> + u32 pamubypenr, pamu_counter;
> + unsigned
RE: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
> -Original Message-
> From: Stuart Yoder [mailto:b08...@gmail.com]
> Sent: Wednesday, February 27, 2013 4:03 AM
> To: Sethi Varun-B16395
> Cc: io...@lists.linux-foundation.org; linuxppc-...@lists.ozlabs.org;
> linux-kernel@vger.kernel.org; Wood Scott-B07421; Joerg Roedel; Yoder
> Stuart-B08248
> Subject: Re: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU
> API implementation.
>
> Have not got through the entire file, but have a few comments...
>
> +/*
> + * Set the PAACE type as primary and set the coherency required domain
> + * attribute
> + */
> +static void pamu_setup_default_xfer_to_host_ppaace(struct paace
> +*ppaace) {
> + set_bf(ppaace->addr_bitfields, PAACE_AF_PT, PAACE_PT_PRIMARY);
> +
> + set_bf(ppaace->domain_attr.to_host.coherency_required,
> PAACE_DA_HOST_CR,
> + PAACE_M_COHERENCE_REQ);
> +}
> +
> +/*
> + * Set the PAACE type as secondary and set the coherency required
> +domain
> + * attribute.
> + */
> +static void pamu_setup_default_xfer_to_host_spaace(struct paace
> +*spaace) {
> + set_bf(spaace->addr_bitfields, PAACE_AF_PT, PAACE_PT_SECONDARY);
> + set_bf(spaace->domain_attr.to_host.coherency_required,
> PAACE_DA_HOST_CR,
> + PAACE_M_COHERENCE_REQ);
> +}
>
> Can we change the names of the above functions... I know there is some
> history
> with the name, but "xfer_to_host" is confusing.
>
> Maybe just call them:
>
> pamu_init_paace()
> pamu_init_spaace()
>
[Sethi Varun-B16395] ok, will change the function names.
> > +/**
> > + * pamu_config_spaace() - Sets up SPAACE entry for specified
> > +subwindow
> > + *
> > + * @liodn: Logical IO device number
> > + * @subwin_cnt: number of sub-windows associated with dma-window
> > + * @subwin_addr: starting address of subwindow
> > + * @subwin_size: size of subwindow
> > + * @omi: Operation mapping index
> > + * @rpn: real (true physical) page number
> > + * @snoopid: snoop id for hardware coherency -- if ~snoopid == 0 then
> > + * snoopid not defined
> > + * @stashid: cache stash id for associated cpu
> > + * @enable: enable/disable subwindow after reconfiguration
> > + * @prot: sub window permissions
> > + *
> > + * Returns 0 upon success else error code < 0 returned */ int
> > +pamu_config_spaace(int liodn, u32 subwin_cnt, u32 subwin_addr,
> > + phys_addr_t subwin_size, u32 omi, unsigned long
> rpn,
> > + u32 snoopid, u32 stashid, int enable, int prot)
> > +{
> > + struct paace *paace;
> > +
> > + /* setup sub-windows */
> > + if (!subwin_cnt) {
> > + pr_err("Invalid subwindow count\n");
> > + return -EINVAL;
> > + }
> > +
> > + paace = pamu_get_ppaace(liodn);
> > + if (subwin_addr > 0 && subwin_addr < subwin_cnt && paace) {
>
> Why is the comparison subwin_addr < subwin_cnt? Seems wrong...
>
[Sethi Varun-B16395] It's actually the subwindow index. I will rename the
variable.
> > + paace = pamu_get_spaace(paace, subwin_addr - 1);
> > +
> > + if (paace && !(paace->addr_bitfields & PAACE_V_VALID))
> {
> > + pamu_setup_default_xfer_to_host_spaace(paace);
> > + set_bf(paace->addr_bitfields, SPAACE_AF_LIODN,
> liodn);
> > + }
> > + }
> > +
> > + if (!paace) {
> > + pr_err("Invalid liodn entry\n");
> > + return -ENOENT;
> > + }
> > +
> > + if (!enable && prot == PAACE_AP_PERMS_DENIED) {
> > + if (subwin_addr > 0)
> > + set_bf(paace->addr_bitfields, PAACE_AF_V,
> > +PAACE_V_INVALID);
> > + else
> > + set_bf(paace->addr_bitfields, PAACE_AF_AP,
> > +prot);
> > + mb();
> > + return 0;
> > + }
>
> Can you add a comment to the above if statement...when is this function
> called with PAACE_AP_PERMS_DENIED?
>
[Sethi Varun-B16395] Actually, this piece of code is redundant in case of the
window based API. I will remove this. PAACE_AP_PERMS_DENIED is primarily used
for disabling the primary subwindow.
-Varun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
-Original Message-
From: Stuart Yoder [mailto:b08...@gmail.com]
Sent: Wednesday, February 27, 2013 4:03 AM
To: Sethi Varun-B16395
Cc: io...@lists.linux-foundation.org; linuxppc-...@lists.ozlabs.org;
linux-kernel@vger.kernel.org; Wood Scott-B07421; Joerg Roedel; Yoder
Stuart-B08248
Subject: Re: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU
API implementation.
Have not got through the entire file, but have a few comments...
+/*
+ * Set the PAACE type as primary and set the coherency required domain
+ * attribute
+ */
+static void pamu_setup_default_xfer_to_host_ppaace(struct paace
+*ppaace) {
+ set_bf(ppaace-addr_bitfields, PAACE_AF_PT, PAACE_PT_PRIMARY);
+
+ set_bf(ppaace-domain_attr.to_host.coherency_required,
PAACE_DA_HOST_CR,
+ PAACE_M_COHERENCE_REQ);
+}
+
+/*
+ * Set the PAACE type as secondary and set the coherency required
+domain
+ * attribute.
+ */
+static void pamu_setup_default_xfer_to_host_spaace(struct paace
+*spaace) {
+ set_bf(spaace-addr_bitfields, PAACE_AF_PT, PAACE_PT_SECONDARY);
+ set_bf(spaace-domain_attr.to_host.coherency_required,
PAACE_DA_HOST_CR,
+ PAACE_M_COHERENCE_REQ);
+}
Can we change the names of the above functions... I know there is some
history
with the name, but xfer_to_host is confusing.
Maybe just call them:
pamu_init_paace()
pamu_init_spaace()
[Sethi Varun-B16395] ok, will change the function names.
+/**
+ * pamu_config_spaace() - Sets up SPAACE entry for specified
+subwindow
+ *
+ * @liodn: Logical IO device number
+ * @subwin_cnt: number of sub-windows associated with dma-window
+ * @subwin_addr: starting address of subwindow
+ * @subwin_size: size of subwindow
+ * @omi: Operation mapping index
+ * @rpn: real (true physical) page number
+ * @snoopid: snoop id for hardware coherency -- if ~snoopid == 0 then
+ * snoopid not defined
+ * @stashid: cache stash id for associated cpu
+ * @enable: enable/disable subwindow after reconfiguration
+ * @prot: sub window permissions
+ *
+ * Returns 0 upon success else error code 0 returned */ int
+pamu_config_spaace(int liodn, u32 subwin_cnt, u32 subwin_addr,
+ phys_addr_t subwin_size, u32 omi, unsigned long
rpn,
+ u32 snoopid, u32 stashid, int enable, int prot)
+{
+ struct paace *paace;
+
+ /* setup sub-windows */
+ if (!subwin_cnt) {
+ pr_err(Invalid subwindow count\n);
+ return -EINVAL;
+ }
+
+ paace = pamu_get_ppaace(liodn);
+ if (subwin_addr 0 subwin_addr subwin_cnt paace) {
Why is the comparison subwin_addr subwin_cnt? Seems wrong...
[Sethi Varun-B16395] It's actually the subwindow index. I will rename the
variable.
+ paace = pamu_get_spaace(paace, subwin_addr - 1);
+
+ if (paace !(paace-addr_bitfields PAACE_V_VALID))
{
+ pamu_setup_default_xfer_to_host_spaace(paace);
+ set_bf(paace-addr_bitfields, SPAACE_AF_LIODN,
liodn);
+ }
+ }
+
+ if (!paace) {
+ pr_err(Invalid liodn entry\n);
+ return -ENOENT;
+ }
+
+ if (!enable prot == PAACE_AP_PERMS_DENIED) {
+ if (subwin_addr 0)
+ set_bf(paace-addr_bitfields, PAACE_AF_V,
+PAACE_V_INVALID);
+ else
+ set_bf(paace-addr_bitfields, PAACE_AF_AP,
+prot);
+ mb();
+ return 0;
+ }
Can you add a comment to the above if statement...when is this function
called with PAACE_AP_PERMS_DENIED?
[Sethi Varun-B16395] Actually, this piece of code is redundant in case of the
window based API. I will remove this. PAACE_AP_PERMS_DENIED is primarily used
for disabling the primary subwindow.
-Varun
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
Some more comments...
On Mon, Feb 18, 2013 at 6:52 AM, Varun Sethi varun.se...@freescale.com wrote:
+/* Handling access violations */
+#define make64(high, low) (((u64)(high) 32) | (low))
+
+struct pamu_isr_data {
+ void __iomem *pamu_reg_base;/* Base address of PAMU regs*/
+ unsigned int count; /* The number of PAMUs */
+};
+
+static struct paace *ppaact;
+static struct paace *spaact;
+static struct ome *omt;
+
+/* maximum subwindows permitted per liodn */
+unsigned int max_subwindow_count;
+/* Number of SPAACT entries */
+unsigned long max_subwins;
I don't like that these variables are not static... and they are
referenced directly
from code in fsl_pamu_domain.c. It would be better if fsl_pamu_domain.c
called an accessor function-- like pamu_get_max_subwins.
+/* Pool for fspi allocation */
+struct gen_pool *spaace_pool;
spaace_pool should be static?
I'm wondering if you should change pamu_isr_data into a more
general struct analagous to struct intel_iommu. You could
put in there the max # of subwins, etc. You could then
provide an accessor to get at that data.
[cut]
+/**
+ * pamu_get_fspi_and_allocate() - Allocates fspi index and reserves
subwindows
+ *required for primary PAACE in the secondary
+ *PAACE table.
+ * @subwin_cnt: Number of subwindows to be reserved.
+ *
+ * A PPAACE entry may have a number of associated subwindows. A subwindow
+ * corresponds to a SPAACE entry in the SPAACT table. Each PAACE entry stores
+ * the index (fspi) of the first SPAACE entry in the SPAACT table. This
+ * function returns the index of the first SPAACE entry. The remaining
+ * SPAACE entries are reserved contiguously from that index.
+ *
+ * Returns a valid fspi index in the range of 0 - max_subwins on success.
+ * If no SPAACE entry is available or the allocator can not reserve the
required
+ * number of contiguous entries function returns ULONG_MAX indicating a
failure.
+ *
+*/
+static unsigned long pamu_get_fspi_and_allocate(u32 subwin_cnt)
+{
+ unsigned long spaace_addr;
+
+ spaace_addr = gen_pool_alloc(spaace_pool, subwin_cnt * sizeof(struct
paace));
+ if (!spaace_addr)
+ return ULONG_MAX;
+
+ return (spaace_addr - (unsigned long)spaact) / (sizeof(struct paace));
+}
In order to keep things symmetric (with the free function) can we just
call the above
function:
pamu_alloc_subwins()
+/* Release the subwindows reserved for a particular LIODN */
+void pamu_free_subwins(int liodn)
+{
+ struct paace *ppaace;
+ u32 subwin_cnt, size;
+
+ ppaace = pamu_get_ppaace(liodn);
+ if (!ppaace) {
+ pr_err(Invalid liodn entry\n);
+ return;
+ }
+
+ if (get_bf(ppaace-addr_bitfields, PPAACE_AF_MW)) {
+ subwin_cnt = 1UL (get_bf(ppaace-impl_attr, PAACE_IA_WCE)
+ 1);
+ size = (subwin_cnt - 1) * sizeof(struct paace);
+ gen_pool_free(spaace_pool, (unsigned
long)spaact[ppaace-fspi], size);
+ set_bf(ppaace-addr_bitfields, PPAACE_AF_MW, 0);
+ }
+}
[cut]
+/**
+ * get_stash_id - Returns stash destination id corresponding to a
+ *cache type and vcpu.
+ * @stash_dest_hint: L1, L2 or L3
+ * @vcpu: vpcu target for a particular cache type.
+ *
+ * Returs stash on success or ~(u32)0 on failure.
+ *
+ */
+u32 get_stash_id(u32 stash_dest_hint, u32 vcpu)
+{
The stash dest is really not a hint, right? It's the requested stash
destination. So maybe just drop 'hint' from the name.
The CPU here is really a physical CPU number and has nothing to do
with vcpus I think. vcpu implies the index is inside a virtual machine...but
this API is generic and may or may not be used with KVM.
+
+/*
+ * Get the maximum number of PAACT table entries
+ * and subwindows supported by PAMU
+ */
+static void get_pamu_cap_values(unsigned long pamu_reg_base)
+{
+ u32 pc_val;
+
+ pc_val = in_be32((u32 *)(pamu_reg_base + PAMU_PC3));
+ /* Maximum number of subwindows per liodn */
+ max_subwindow_count = 1 (1 + PAMU_PC3_MWCE(pc_val));
+ /* Total number of SPACCT entries */
+ max_subwins = PAACE_NUMBER_ENTRIES * max_subwindow_count;
+}
If you follow the suggestion at the top of this file, this function
would become something like-- init_pamu_capabilities(). And then
create an accessor function to access max subwins, etc.
Also, BTW, I don't see any support for the DOMAIN_ATTR_WINDOWS
attribute in your patch. Was that coming in a later patch?
[cut
+static int __init fsl_pamu_probe(struct platform_device *pdev)
+{
+ void __iomem *pamu_regs = NULL;
+ struct ccsr_guts __iomem *guts_regs = NULL;
+ u32 pamubypenr, pamu_counter;
+ unsigned long pamu_reg_off;
+ unsigned long pamu_reg_base;
+ struct
Re: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
Have not got through the entire file, but have a few comments...
+/*
+ * Set the PAACE type as primary and set the coherency required domain
+ * attribute
+ */
+static void pamu_setup_default_xfer_to_host_ppaace(struct paace *ppaace)
+{
+ set_bf(ppaace->addr_bitfields, PAACE_AF_PT, PAACE_PT_PRIMARY);
+
+ set_bf(ppaace->domain_attr.to_host.coherency_required, PAACE_DA_HOST_CR,
+ PAACE_M_COHERENCE_REQ);
+}
+
+/*
+ * Set the PAACE type as secondary and set the coherency required domain
+ * attribute.
+ */
+static void pamu_setup_default_xfer_to_host_spaace(struct paace *spaace)
+{
+ set_bf(spaace->addr_bitfields, PAACE_AF_PT, PAACE_PT_SECONDARY);
+ set_bf(spaace->domain_attr.to_host.coherency_required, PAACE_DA_HOST_CR,
+ PAACE_M_COHERENCE_REQ);
+}
Can we change the names of the above functions... I know there is some history
with the name, but "xfer_to_host" is confusing.
Maybe just call them:
pamu_init_paace()
pamu_init_spaace()
> +/**
> + * pamu_config_spaace() - Sets up SPAACE entry for specified subwindow
> + *
> + * @liodn: Logical IO device number
> + * @subwin_cnt: number of sub-windows associated with dma-window
> + * @subwin_addr: starting address of subwindow
> + * @subwin_size: size of subwindow
> + * @omi: Operation mapping index
> + * @rpn: real (true physical) page number
> + * @snoopid: snoop id for hardware coherency -- if ~snoopid == 0 then
> + * snoopid not defined
> + * @stashid: cache stash id for associated cpu
> + * @enable: enable/disable subwindow after reconfiguration
> + * @prot: sub window permissions
> + *
> + * Returns 0 upon success else error code < 0 returned
> + */
> +int pamu_config_spaace(int liodn, u32 subwin_cnt, u32 subwin_addr,
> + phys_addr_t subwin_size, u32 omi, unsigned long rpn,
> + u32 snoopid, u32 stashid, int enable, int prot)
> +{
> + struct paace *paace;
> +
> + /* setup sub-windows */
> + if (!subwin_cnt) {
> + pr_err("Invalid subwindow count\n");
> + return -EINVAL;
> + }
> +
> + paace = pamu_get_ppaace(liodn);
> + if (subwin_addr > 0 && subwin_addr < subwin_cnt && paace) {
Why is the comparison subwin_addr < subwin_cnt? Seems wrong...
> + paace = pamu_get_spaace(paace, subwin_addr - 1);
> +
> + if (paace && !(paace->addr_bitfields & PAACE_V_VALID)) {
> + pamu_setup_default_xfer_to_host_spaace(paace);
> + set_bf(paace->addr_bitfields, SPAACE_AF_LIODN, liodn);
> + }
> + }
> +
> + if (!paace) {
> + pr_err("Invalid liodn entry\n");
> + return -ENOENT;
> + }
> +
> + if (!enable && prot == PAACE_AP_PERMS_DENIED) {
> + if (subwin_addr > 0)
> + set_bf(paace->addr_bitfields, PAACE_AF_V,
> +PAACE_V_INVALID);
> + else
> + set_bf(paace->addr_bitfields, PAACE_AF_AP,
> +prot);
> + mb();
> + return 0;
> + }
Can you add a comment to the above if statement...when is this function called
with PAACE_AP_PERMS_DENIED?
> + if (subwin_size & (subwin_size - 1) || subwin_size < PAMU_PAGE_SIZE) {
> + pr_err("subwindow size out of range, or not a power of 2\n");
> + return -EINVAL;
> + }
> +
> + if (rpn == ULONG_MAX) {
> + pr_err("real page number out of range\n");
> + return -EINVAL;
> + }
> +
> + /* window size is 2^(WSE+1) bytes */
> + set_bf(paace->win_bitfields, PAACE_WIN_SWSE,
> + map_addrspace_size_to_wse(subwin_size));
> +
> + set_bf(paace->impl_attr, PAACE_IA_ATM, PAACE_ATM_WINDOW_XLATE);
> + paace->twbah = rpn >> 20;
> + set_bf(paace->win_bitfields, PAACE_WIN_TWBAL, rpn);
> + set_bf(paace->addr_bitfields, PAACE_AF_AP, prot);
> +
> + /* configure snoop id */
> + if (~snoopid != 0)
> + paace->domain_attr.to_host.snpid = snoopid;
> +
> + /* set up operation mapping if it's configured */
> + if (omi < OME_NUMBER_ENTRIES) {
> + set_bf(paace->impl_attr, PAACE_IA_OTM, PAACE_OTM_INDEXED);
> + paace->op_encode.index_ot.omi = omi;
> + } else if (~omi != 0) {
> + pr_err("bad operation mapping index: %d\n", omi);
> + return -EINVAL;
> + }
> +
> + if (~stashid != 0)
> + set_bf(paace->impl_attr, PAACE_IA_CID, stashid);
> +
> + smp_wmb();
> +
> + if (enable)
> + paace->addr_bitfields |= PAACE_V_VALID;
> +
> + mb();
> +
> + return 0;
> +}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at
Re: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
Have not got through the entire file, but have a few comments...
+/*
+ * Set the PAACE type as primary and set the coherency required domain
+ * attribute
+ */
+static void pamu_setup_default_xfer_to_host_ppaace(struct paace *ppaace)
+{
+ set_bf(ppaace-addr_bitfields, PAACE_AF_PT, PAACE_PT_PRIMARY);
+
+ set_bf(ppaace-domain_attr.to_host.coherency_required, PAACE_DA_HOST_CR,
+ PAACE_M_COHERENCE_REQ);
+}
+
+/*
+ * Set the PAACE type as secondary and set the coherency required domain
+ * attribute.
+ */
+static void pamu_setup_default_xfer_to_host_spaace(struct paace *spaace)
+{
+ set_bf(spaace-addr_bitfields, PAACE_AF_PT, PAACE_PT_SECONDARY);
+ set_bf(spaace-domain_attr.to_host.coherency_required, PAACE_DA_HOST_CR,
+ PAACE_M_COHERENCE_REQ);
+}
Can we change the names of the above functions... I know there is some history
with the name, but xfer_to_host is confusing.
Maybe just call them:
pamu_init_paace()
pamu_init_spaace()
+/**
+ * pamu_config_spaace() - Sets up SPAACE entry for specified subwindow
+ *
+ * @liodn: Logical IO device number
+ * @subwin_cnt: number of sub-windows associated with dma-window
+ * @subwin_addr: starting address of subwindow
+ * @subwin_size: size of subwindow
+ * @omi: Operation mapping index
+ * @rpn: real (true physical) page number
+ * @snoopid: snoop id for hardware coherency -- if ~snoopid == 0 then
+ * snoopid not defined
+ * @stashid: cache stash id for associated cpu
+ * @enable: enable/disable subwindow after reconfiguration
+ * @prot: sub window permissions
+ *
+ * Returns 0 upon success else error code 0 returned
+ */
+int pamu_config_spaace(int liodn, u32 subwin_cnt, u32 subwin_addr,
+ phys_addr_t subwin_size, u32 omi, unsigned long rpn,
+ u32 snoopid, u32 stashid, int enable, int prot)
+{
+ struct paace *paace;
+
+ /* setup sub-windows */
+ if (!subwin_cnt) {
+ pr_err(Invalid subwindow count\n);
+ return -EINVAL;
+ }
+
+ paace = pamu_get_ppaace(liodn);
+ if (subwin_addr 0 subwin_addr subwin_cnt paace) {
Why is the comparison subwin_addr subwin_cnt? Seems wrong...
+ paace = pamu_get_spaace(paace, subwin_addr - 1);
+
+ if (paace !(paace-addr_bitfields PAACE_V_VALID)) {
+ pamu_setup_default_xfer_to_host_spaace(paace);
+ set_bf(paace-addr_bitfields, SPAACE_AF_LIODN, liodn);
+ }
+ }
+
+ if (!paace) {
+ pr_err(Invalid liodn entry\n);
+ return -ENOENT;
+ }
+
+ if (!enable prot == PAACE_AP_PERMS_DENIED) {
+ if (subwin_addr 0)
+ set_bf(paace-addr_bitfields, PAACE_AF_V,
+PAACE_V_INVALID);
+ else
+ set_bf(paace-addr_bitfields, PAACE_AF_AP,
+prot);
+ mb();
+ return 0;
+ }
Can you add a comment to the above if statement...when is this function called
with PAACE_AP_PERMS_DENIED?
+ if (subwin_size (subwin_size - 1) || subwin_size PAMU_PAGE_SIZE) {
+ pr_err(subwindow size out of range, or not a power of 2\n);
+ return -EINVAL;
+ }
+
+ if (rpn == ULONG_MAX) {
+ pr_err(real page number out of range\n);
+ return -EINVAL;
+ }
+
+ /* window size is 2^(WSE+1) bytes */
+ set_bf(paace-win_bitfields, PAACE_WIN_SWSE,
+ map_addrspace_size_to_wse(subwin_size));
+
+ set_bf(paace-impl_attr, PAACE_IA_ATM, PAACE_ATM_WINDOW_XLATE);
+ paace-twbah = rpn 20;
+ set_bf(paace-win_bitfields, PAACE_WIN_TWBAL, rpn);
+ set_bf(paace-addr_bitfields, PAACE_AF_AP, prot);
+
+ /* configure snoop id */
+ if (~snoopid != 0)
+ paace-domain_attr.to_host.snpid = snoopid;
+
+ /* set up operation mapping if it's configured */
+ if (omi OME_NUMBER_ENTRIES) {
+ set_bf(paace-impl_attr, PAACE_IA_OTM, PAACE_OTM_INDEXED);
+ paace-op_encode.index_ot.omi = omi;
+ } else if (~omi != 0) {
+ pr_err(bad operation mapping index: %d\n, omi);
+ return -EINVAL;
+ }
+
+ if (~stashid != 0)
+ set_bf(paace-impl_attr, PAACE_IA_CID, stashid);
+
+ smp_wmb();
+
+ if (enable)
+ paace-addr_bitfields |= PAACE_V_VALID;
+
+ mb();
+
+ return 0;
+}
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
> -Original Message-
> From: Craciun Diana Madalina-STFD002
> Sent: Tuesday, February 19, 2013 9:30 PM
> To: Sethi Varun-B16395
> Cc: io...@lists.linux-foundation.org; linuxppc-...@lists.ozlabs.org;
> linux-kernel@vger.kernel.org; Wood Scott-B07421; j...@8bytes.org; Yoder
> Stuart-B08248
> Subject: Re: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU
> API implementation.
>
> On 02/18/2013 02:52 PM, Varun Sethi wrote:
> > +/**
> > + * pamu_get_ppaace() - Return the primary PACCE
> > + * @liodn: liodn PAACT index for desired PAACE
> > + *
> > + * Returns the ppace pointer upon success else return
> > + * null.
> > + */
> > +static struct paace *pamu_get_ppaace(int liodn) {
> > + if (!ppaact || liodn > PAACE_NUMBER_ENTRIES) {
>
> Shouldn't be "liodn >= PAACE_NUMBER_ENTRIES" ?
Yes, will fix this.
-Varun
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
-Original Message-
From: Craciun Diana Madalina-STFD002
Sent: Tuesday, February 19, 2013 9:30 PM
To: Sethi Varun-B16395
Cc: io...@lists.linux-foundation.org; linuxppc-...@lists.ozlabs.org;
linux-kernel@vger.kernel.org; Wood Scott-B07421; j...@8bytes.org; Yoder
Stuart-B08248
Subject: Re: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU
API implementation.
On 02/18/2013 02:52 PM, Varun Sethi wrote:
+/**
+ * pamu_get_ppaace() - Return the primary PACCE
+ * @liodn: liodn PAACT index for desired PAACE
+ *
+ * Returns the ppace pointer upon success else return
+ * null.
+ */
+static struct paace *pamu_get_ppaace(int liodn) {
+ if (!ppaact || liodn PAACE_NUMBER_ENTRIES) {
Shouldn't be liodn = PAACE_NUMBER_ENTRIES ?
Yes, will fix this.
-Varun
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
On 02/18/2013 02:52 PM, Varun Sethi wrote:
+/**
+ * pamu_get_ppaace() - Return the primary PACCE
+ * @liodn: liodn PAACT index for desired PAACE
+ *
+ * Returns the ppace pointer upon success else return
+ * null.
+ */
+static struct paace *pamu_get_ppaace(int liodn)
+{
+ if (!ppaact || liodn > PAACE_NUMBER_ENTRIES) {
Shouldn't be "liodn >= PAACE_NUMBER_ENTRIES" ?
+ pr_err("PPAACT doesn't exist\n");
+ return NULL;
+ }
+
+ return [liodn];
+}
+
Diana
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
> -Original Message- > From: Craciun Diana Madalina-STFD002 > Sent: Tuesday, February 19, 2013 3:34 PM > To: Sethi Varun-B16395 > Cc: io...@lists.linux-foundation.org; linuxppc-...@lists.ozlabs.org; > linux-kernel@vger.kernel.org; Wood Scott-B07421; j...@8bytes.org; Yoder > Stuart-B08248 > Subject: Re: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU > API implementation. > > On 02/18/2013 02:52 PM, Varun Sethi wrote: > > + > > +#define PAACE_TCEF_FORMAT0_8B 0x00 > > +#define PAACE_TCEF_FORMAT1_RSVD 0x01 > > + > > +#define PAACE_NUMBER_ENTRIES0x1FF > > Where is this number coming from? > This is currently hard coded. We will not require these many entries once we implement the LIODN allocation scheme. -Varun -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
On 02/18/2013 02:52 PM, Varun Sethi wrote: + +#define PAACE_TCEF_FORMAT0_8B 0x00 +#define PAACE_TCEF_FORMAT1_RSVD 0x01 + +#define PAACE_NUMBER_ENTRIES0x1FF Where is this number coming from? Diana -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
On 02/18/2013 02:52 PM, Varun Sethi wrote: + +#define PAACE_TCEF_FORMAT0_8B 0x00 +#define PAACE_TCEF_FORMAT1_RSVD 0x01 + +#define PAACE_NUMBER_ENTRIES0x1FF Where is this number coming from? Diana -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
-Original Message- From: Craciun Diana Madalina-STFD002 Sent: Tuesday, February 19, 2013 3:34 PM To: Sethi Varun-B16395 Cc: io...@lists.linux-foundation.org; linuxppc-...@lists.ozlabs.org; linux-kernel@vger.kernel.org; Wood Scott-B07421; j...@8bytes.org; Yoder Stuart-B08248 Subject: Re: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API implementation. On 02/18/2013 02:52 PM, Varun Sethi wrote: + +#define PAACE_TCEF_FORMAT0_8B 0x00 +#define PAACE_TCEF_FORMAT1_RSVD 0x01 + +#define PAACE_NUMBER_ENTRIES0x1FF Where is this number coming from? This is currently hard coded. We will not require these many entries once we implement the LIODN allocation scheme. -Varun -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH 6/6 v8] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
On 02/18/2013 02:52 PM, Varun Sethi wrote:
+/**
+ * pamu_get_ppaace() - Return the primary PACCE
+ * @liodn: liodn PAACT index for desired PAACE
+ *
+ * Returns the ppace pointer upon success else return
+ * null.
+ */
+static struct paace *pamu_get_ppaace(int liodn)
+{
+ if (!ppaact || liodn PAACE_NUMBER_ENTRIES) {
Shouldn't be liodn = PAACE_NUMBER_ENTRIES ?
+ pr_err(PPAACT doesn't exist\n);
+ return NULL;
+ }
+
+ return ppaact[liodn];
+}
+
Diana
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
