Hi Magnus,
Thank you for the patch.
On Tuesday 15 March 2016 13:22:04 Magnus Damm wrote:
> From: Magnus Damm <damm+rene...@opensource.se>
>
> Make the driver compile on more than just 32-bit ARM
> by breaking out and wrapping ARM specific functions
> in #ifdefs. Not pretty, but needed to be able to use
> the driver on other architectures like ARM64.
Given that the callers of the new ARM-specific functions will, after patch
"[PATCH 04/04] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops", be used only
on !CONFIG_IOMMU_DMA, and that ARM64 selects CONFIG_IOMMU_DMA, wouldn't it
make more sense not to extract the ARM-specific code in separate functions but
compile-out the whole ipmmu_ops structure and all the related operation
handlers when !CONFIG_IOMMU_DMA ?
I would order the patches as follows.
[PATCH v2 01/04] iommu/ipmmu-vmsa: Remove platform data handling
[PATCH v2 02/04] iommu/ipmmu-vmsa: Rework interrupt code and use bitmap for
context
[PATCH v2 04/04] iommu/ipmmu-vmsa: Drop LPAE Kconfig dependency
[PATCH 02/04] iommu/ipmmu-vmsa: Break out utlb parsing code
[PATCH 03/04] iommu/ipmmu-vmsa: Break out domain allocation code
and then squash the following two patches, with conditional compilation for
ipmmu_ops.
[PATCH 01/04] iommu/ipmmu-vmsa: 32-bit ARM may have CONFIG_IOMMU_DMA=y
[PATCH 04/04] iommu/ipmmu-vmsa: Add new IOMMU_DOMAIN_DMA ops
I think the result will be cleaner.
Please see below for a couple of other comments.
> Signed-off-by: Magnus Damm <damm+rene...@opensource.se>
> ---
>
> Changes since V1:
> - Rebased to work without patch 2 and 3 from V1 series
>
> drivers/iommu/ipmmu-vmsa.c | 94 +++++++++++++++++++++++++++-------------
> 1 file changed, 62 insertions(+), 32 deletions(-)
>
> --- 0004/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c 2016-03-15 12:25:45.040513000 +0900
> @@ -22,8 +22,10 @@
> #include <linux/sizes.h>
> #include <linux/slab.h>
>
> +#ifdef CONFIG_ARM
> #include <asm/dma-iommu.h>
> #include <asm/pgalloc.h>
> +#endif
>
> #include "io-pgtable.h"
>
> @@ -38,7 +40,9 @@ struct ipmmu_vmsa_device {
> DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
> struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
>
> +#ifdef CONFIG_ARM
> struct dma_iommu_mapping *mapping;
> +#endif
> };
>
> struct ipmmu_vmsa_domain {
> @@ -615,6 +619,60 @@ static int ipmmu_find_utlbs(struct ipmmu
> return 0;
> }
>
> +#ifdef CONFIG_ARM
> +static int ipmmu_map_attach(struct device *dev, struct ipmmu_vmsa_device
> *mmu)
> +{
> + int ret;
> +
> + /*
> + * Create the ARM mapping, used by the ARM DMA mapping core to allocate
> + * VAs. This will allocate a corresponding IOMMU domain.
> + *
> + * TODO:
> + * - Create one mapping per context (TLB).
> + * - Make the mapping size configurable ? We currently use a 2GB mapping
> + * at a 1GB offset to ensure that NULL VAs will fault.
> + */
> + if (!mmu->mapping) {
> + struct dma_iommu_mapping *mapping;
> +
> + mapping = arm_iommu_create_mapping(&platform_bus_type,
> + SZ_1G, SZ_2G);
> + if (IS_ERR(mapping)) {
> + dev_err(mmu->dev, "failed to create ARM IOMMU
> mapping\n");
> + return PTR_ERR(mapping);
> + }
> +
> + mmu->mapping = mapping;
> + }
> +
> + /* Attach the ARM VA mapping to the device. */
> + ret = arm_iommu_attach_device(dev, mmu->mapping);
> + if (ret < 0) {
> + dev_err(dev, "Failed to attach device to VA mapping\n");
> + arm_iommu_release_mapping(mmu->mapping);
> + }
> +
> + return ret;
> +}
How about adding a blank line here ?
> +static inline void ipmmu_detach(struct device *dev)
> +{
> + arm_iommu_detach_device(dev);
> +}
And another one here ?
> +static inline void ipmmu_release_mapping(struct ipmmu_vmsa_device *mmu)
> +{
> + arm_iommu_release_mapping(mmu->mapping);
> +}
> +#else
> +static inline int ipmmu_map_attach(struct device *dev,
> + struct ipmmu_vmsa_device *mmu)
> +{
> + return 0;
> +}
> +static inline void ipmmu_detach(struct device *dev) {}
> +static inline void ipmmu_release_mapping(struct ipmmu_vmsa_device *mmu) {}
The compiler should be smart enough to inline all these functions if needed.
> +#endif
> +
> static int ipmmu_add_device(struct device *dev)
> {
> struct ipmmu_vmsa_archdata *archdata;
> @@ -695,41 +753,13 @@ static int ipmmu_add_device(struct devic
> archdata->num_utlbs = num_utlbs;
> dev->archdata.iommu = archdata;
>
> - /*
> - * Create the ARM mapping, used by the ARM DMA mapping core to allocate
> - * VAs. This will allocate a corresponding IOMMU domain.
> - *
> - * TODO:
> - * - Create one mapping per context (TLB).
> - * - Make the mapping size configurable ? We currently use a 2GB mapping
> - * at a 1GB offset to ensure that NULL VAs will fault.
> - */
> - if (!mmu->mapping) {
> - struct dma_iommu_mapping *mapping;
> -
> - mapping = arm_iommu_create_mapping(&platform_bus_type,
> - SZ_1G, SZ_2G);
> - if (IS_ERR(mapping)) {
> - dev_err(mmu->dev, "failed to create ARM IOMMU
> mapping\n");
> - ret = PTR_ERR(mapping);
> - goto error;
> - }
> -
> - mmu->mapping = mapping;
> - }
> -
> - /* Attach the ARM VA mapping to the device. */
> - ret = arm_iommu_attach_device(dev, mmu->mapping);
> - if (ret < 0) {
> - dev_err(dev, "Failed to attach device to VA mapping\n");
> + ret = ipmmu_map_attach(dev, mmu);
> + if (ret < 0)
> goto error;
> - }
>
> return 0;
>
> error:
> - arm_iommu_release_mapping(mmu->mapping);
> -
> kfree(dev->archdata.iommu);
> kfree(utlbs);
>
> @@ -745,7 +775,7 @@ static void ipmmu_remove_device(struct d
> {
> struct ipmmu_vmsa_archdata *archdata = dev->archdata.iommu;
>
> - arm_iommu_detach_device(dev);
> + ipmmu_detach(dev);
> iommu_group_remove_device(dev);
>
> kfree(archdata->utlbs);
> @@ -856,7 +886,7 @@ static int ipmmu_remove(struct platform_
> list_del(&mmu->list);
> spin_unlock(&ipmmu_devices_lock);
>
> - arm_iommu_release_mapping(mmu->mapping);
> + ipmmu_release_mapping(mmu);
>
> ipmmu_device_reset(mmu);
--
Regards,
Laurent Pinchart
