RE: [PATCH 3/3 v3] iommu/fsl: Freescale PAMU driver and IOMMU API implementation.
-Original Message-
From: Tabi Timur-B04825
Sent: Tuesday, October 23, 2012 2:48 AM
To: Sethi Varun-B16395
Cc: joerg.roe...@amd.com; iommu@lists.linux-foundation.org; linuxppc-
d...@lists.ozlabs.org; linux-ker...@vger.kernel.org
Subject: Re: [PATCH 3/3 v3] iommu/fsl: Freescale PAMU driver and IOMMU
API implementation.
On Wed, Oct 17, 2012 at 12:32 PM, Varun Sethi varun.se...@freescale.com
wrote:
+ * Copyright (C) 2012 Freescale Semiconductor, Inc.
Copyright 2012 Freescale Semiconductor, Inc.
[Sethi Varun-B16395] Have followed similar convention elsewhere i.e. added (C).
+ *
+ */
+
+#include linux/init.h
+#include linux/iommu.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/genalloc.h
+#include asm/io.h
+#include asm/bitops.h
+
+#include fsl_pamu.h
+
+/* PAMU bypass enbale register contains control bits for
+ * enabling bypass mode on each PAMU.
+ */
/*
* Linux multi-line comments
* look like this.
*/
+#define PAMUBYPENR 0x604
Update struct ccsr_guts instead.
http://patchwork.ozlabs.org/patch/141649/
[Sethi Varun-B16395] Ok.
+
+/* define indexes for each operation mapping scenario */
+#define OMI_QMAN0x00
+#define OMI_FMAN0x01
+#define OMI_QMAN_PRIV 0x02
+#define OMI_CAAM0x03
+
+static paace_t *ppaact;
+static paace_t *spaact;
+static struct ome *omt;
+unsigned int max_subwindow_count;
+
+struct gen_pool *spaace_pool;
+
+static paace_t *pamu_get_ppaace(int liodn) {
+ if (!ppaact) {
+ pr_err(PPAACT doesn't exist\n);
pr_err(fsl-pamu: PPAACT has not been initialized\n);
Make sure ALL pr_xxx() messages in this file start with fsl-pamu:
+ return NULL;
+ }
+
+ return ppaact[liodn];
Bounds checking?
[Sethi Varun-B16395] Ok.
+}
+
+/** Sets validation bit of PACCE
+ *
+ * @parm[in] liodn PAACT index for desired PAACE
+ *
+ * @return Returns 0 upon success else error code 0 returned */
+int pamu_enable_liodn(int liodn) {
+ paace_t *ppaace;
+
+ ppaace = pamu_get_ppaace(liodn);
+ if (!ppaace)
+ return -ENOENT;
error message?
[Sethi Varun-B16395] Have error messages at places from where the function is
invoked.
+
+ if (!get_bf(ppaace-addr_bitfields, PPAACE_AF_WSE)) {
+ pr_err(liodn %d not configured\n, liodn);
+ return -EINVAL;
+ }
+
+ /* Ensure that all other stores to the ppaace complete first */
+ mb();
+
+ ppaace-addr_bitfields |= PAACE_V_VALID;
+ mb();
+
+ return 0;
+}
+
+/** Clears validation bit of PACCE
+ *
+ * @parm[in] liodn PAACT index for desired PAACE
+ *
+ * @return Returns 0 upon success else error code 0 returned
This is not proper kerneldoc format.
[Sethi Varun-B16395] What format must be used? Can you point me to a relevant
file.
+ */
+int pamu_disable_liodn(int liodn)
+{
+ paace_t *ppaace;
+
+ ppaace = pamu_get_ppaace(liodn);
+ if (!ppaace)
+ return -ENOENT;
error message?
[Sethi Varun-B16395] Error message at the point of invocation.
+
+ set_bf(ppaace-addr_bitfields, PAACE_AF_V, PAACE_V_INVALID);
+ mb();
+
+ return 0;
+}
+
+
+static unsigned int map_addrspace_size_to_wse(phys_addr_t
+addrspace_size) {
+ BUG_ON((addrspace_size (addrspace_size - 1)));
+
+ /* window size is 2^(WSE+1) bytes */
+ return __ffs(addrspace_size PAMU_PAGE_SHIFT) +
+PAMU_PAGE_SHIFT - 1; }
+
+static unsigned int map_subwindow_cnt_to_wce(u32 subwindow_cnt) {
+ /* window count is 2^(WCE+1) bytes */
+ return __ffs(subwindow_cnt) - 1; }
+
+static void pamu_setup_default_xfer_to_host_ppaace(paace_t *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); }
+
+static void pamu_setup_default_xfer_to_host_spaace(paace_t *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); }
+
+static paace_t *pamu_get_spaace(u32 fspi_index, u32 wnum) {
+ return spaact[fspi_index + wnum];
bounds checking?
[Sethi Varun-B16395] Ok.
+}
+
+static unsigned long pamu_get_fspi_and_allocate(u32 subwin_cnt) {
subwin_cnt should probably be an unsigned int.
[Sethi Varun-B16395] Why?
This function needs to be documented. What value is being returned?
+ unsigned long spaace_addr
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; joerg.roe...@amd.com; iommu@lists.linux-
foundation.org; linuxppc-...@lists.ozlabs.org; linux-
ker...@vger.kernel.org
Subject: Re: [PATCH 3/3 v3] iommu/fsl: Freescale PAMU driver and IOMMU
API implementation.
On 10/22/2012 04:18:07 PM, Tabi Timur-B04825 wrote:
On Wed, Oct 17, 2012 at 12:32 PM, Varun Sethi
varun.se...@freescale.com wrote:
+}
+
+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?
spaact offset (yes, this needs to be documented)
[Sethi Varun-B16395] Ok.
+/* 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?
Is it even true? We don't support IOMMU pages larger than the SoC can
address.
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.
[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).
+static int map_liodn(int liodn, struct fsl_dma_domain *dma_domain)
+{
+ u32 subwin_cnt = dma_domain-subwin_cnt;
+ unsigned long rpn;
+ int ret = 0, i;
+
+ if (subwin_cnt) {
+ 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) {
+ 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,
+-1,
+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;
+ }
+ }
+ }
Break up that nesting with some subfunctions.
+ while (!list_empty(dma_domain-devices)) {
+ info = 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.
The above is simpler if you're destroying the entire list.
+}
+
+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.
Me too.
[Sethi Varun-B16395] Sure.
-Varun
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
