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
Re: [PATCH -stable] amd_iommu: attach device fails on the last pci device
On Fri, 2012-10-12 at 12:35 -0700, Jonathan Nieder wrote: Shuah Khan wrote: On Fri, 2012-10-12 at 11:38 -0700, Jonathan Nieder wrote: To save Willy time: am I correct in guessing the upstream commit you are referring to is 98fc5a693bbdda498a556654c70d1e31a186c988 (x86/amd-iommu: Use get_device_id and check_device where appropriate, 2009-11-24)? Yes that is one. Great, thanks for the quick confirmation. Is this patch in the queue to be pulled into 2.6.32 tree? Thanks, -- Shuah ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu