[PATCH v3 00/09] iommu/ipmmu-vmsa: r8a7795 support V3

[PATCH v3 00/09] iommu/ipmmu-vmsa: r8a7795 support V3

[Magnus Damm]

iommu/ipmmu-vmsa: r8a7795 support V3

[PATCH v3 01/09] iommu/ipmmu-vmsa: Introduce features, break out alias
[PATCH v3 02/09] iommu/ipmmu-vmsa: Add optional root device feature
[PATCH v3 03/09] iommu/ipmmu-vmsa: Enable multi context support
[PATCH v3 04/09] iommu/ipmmu-vmsa: Make use of IOMMU_OF_DECLARE()
[PATCH v3 05/09] iommu/ipmmu-vmsa: IPMMU device is 40-bit bus master
[PATCH v3 06/09] iommu/ipmmu-vmsa: Write IMCTR twice
[PATCH v3 07/09] iommu/ipmmu-vmsa: Make IMBUSCTR setup optional
[PATCH v3 08/09] iommu/ipmmu-vmsa: Allow two bit SL0
[PATCH v3 09/09] iommu/ipmmu-vmsa: Hook up r8a7795 DT matching code

This series contains a much needed r8a7795 support update for the
IPMMU driver. Last series was earlier posted as:

[PATCH v2 00/11] iommu/ipmmu-vmsa: r8a7795 support V2

The DT binding for r8a7795 has been accepted for upstream merge
and this series implements support following such format:

d4e42e7 iommu/ipmmu-vmsa: Add r8a7795 DT binding

The r8a7795 IPMMU is almost register compatible with earlier devices
like r8a7790-r8a7794, however some bitfields have been shifted
slightly. On a grander scale topology has been added and interrupts
have been reworked. So now there are several "cache" IPMMU units
without interrupt that somehow communicate with IPMMU-MM that
is the only instance that supports interrupts. The code refers to
IPMMU-MM as a "root" device and the other ones as "leaf" nodes.

To make this more interesting the IPMMU driver needs to be shared
between 32-bit ARM for r8a7790-r8a7794 and 64-bit ARM for r8a7795.
In practice this means that two separate implementations are needed
inside the driver to attach to the rather different architecture
specific code.

CONFIG_IOMMU_DMA=y is needed on 64-bit ARM while on 32-bit ARM
the arch specific dma-mapping code is hooked up rather directly.
During init 64-bit ARM IPMMU support is relying on IOMMU_OF_DECLARE().

This version of the code is known to build on 32-bit and 64-bit ARM.

Some patches that existed in V1 have been folded into:
 [PATCH v7 00/07] iommu/ipmmu-vmsa: IPMMU multi-arch update V7

Changes since V2:
 - Patch 2/9 has been updated with a bug fix and to supply __ipmmu_find_root()
 - Patch 4/9 now makes use of iommu_device_* functions
 - Patch 5/9 sets the mask to 40 bits instead of 64 bits
 - Patch 9/9 implements white list handling via ->xlate() and fixes a bug

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Developed on top of v4.11-rc1 and:
 [PATCH v7 00/07] iommu/ipmmu-vmsa: IPMMU multi-arch update V7

 drivers/iommu/ipmmu-vmsa.c |  291 +++++++++++++++++++++++++++++++++++++-------
 1 file changed, 251 insertions(+), 40 deletions(-)

[PATCH v3 01/09] iommu/ipmmu-vmsa: Introduce features, break out alias

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Introduce struct ipmmu_features to track various hardware
and software implementation changes inside the driver for
different kinds of IPMMU hardware. Add use_ns_alias_offset
as a first example of a feature to control if the secure
register bank offset should be used or not.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Changes since V2:
 - None

 Changes since V1:
 - Moved patch to front of the series

 drivers/iommu/ipmmu-vmsa.c |   35 ++++++++++++++++++++++++++++-------
 1 file changed, 28 insertions(+), 7 deletions(-)

--- 0007/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2017-03-07 12:25:47.000000000 +0900
@@ -32,11 +32,15 @@
 
 #define IPMMU_CTX_MAX 1
 
+struct ipmmu_features {
+	bool use_ns_alias_offset;
+};
+
 struct ipmmu_vmsa_device {
 	struct device *dev;
 	void __iomem *base;
 	struct list_head list;
-
+	const struct ipmmu_features *features;
 	unsigned int num_utlbs;
 	spinlock_t lock;			/* Protects ctx and domains[] */
 	DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
@@ -999,13 +1003,33 @@ static void ipmmu_device_reset(struct ip
 		ipmmu_write(mmu, i * IM_CTX_SIZE + IMCTR, 0);
 }
 
+static const struct ipmmu_features ipmmu_features_default = {
+	.use_ns_alias_offset = true,
+};
+
+static const struct of_device_id ipmmu_of_ids[] = {
+	{
+		.compatible = "renesas,ipmmu-vmsa",
+		.data = &ipmmu_features_default,
+	}, {
+		/* Terminator */
+	},
+};
+
+MODULE_DEVICE_TABLE(of, ipmmu_of_ids);
+
 static int ipmmu_probe(struct platform_device *pdev)
 {
 	struct ipmmu_vmsa_device *mmu;
+	const struct of_device_id *match;
 	struct resource *res;
 	int irq;
 	int ret;
 
+	match = of_match_node(ipmmu_of_ids, pdev->dev.of_node);
+	if (!match)
+		return -EINVAL;
+
 	mmu = devm_kzalloc(&pdev->dev, sizeof(*mmu), GFP_KERNEL);
 	if (!mmu) {
 		dev_err(&pdev->dev, "cannot allocate device data\n");
@@ -1016,6 +1040,7 @@ static int ipmmu_probe(struct platform_d
 	mmu->num_utlbs = 32;
 	spin_lock_init(&mmu->lock);
 	bitmap_zero(mmu->ctx, IPMMU_CTX_MAX);
+	mmu->features = match->data;
 
 	/* Map I/O memory and request IRQ. */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -1035,7 +1060,8 @@ static int ipmmu_probe(struct platform_d
 	 * Offset the registers base unconditionally to point to the non-secure
 	 * alias space for now.
 	 */
-	mmu->base += IM_NS_ALIAS_OFFSET;
+	if (mmu->features->use_ns_alias_offset)
+		mmu->base += IM_NS_ALIAS_OFFSET;
 
 	irq = platform_get_irq(pdev, 0);
 	if (irq < 0) {
@@ -1084,11 +1110,6 @@ static int ipmmu_remove(struct platform_
 	return 0;
 }
 
-static const struct of_device_id ipmmu_of_ids[] = {
-	{ .compatible = "renesas,ipmmu-vmsa", },
-	{ }
-};
-
 static struct platform_driver ipmmu_driver = {
 	.driver = {
 		.name = "ipmmu-vmsa",

Re: [PATCH v3 01/09] iommu/ipmmu-vmsa: Introduce features, break out alias

[Robin Murphy]

Hi Magnus,

On 08/03/17 11:01, Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
> 
> Introduce struct ipmmu_features to track various hardware
> and software implementation changes inside the driver for
> different kinds of IPMMU hardware. Add use_ns_alias_offset
> as a first example of a feature to control if the secure
> register bank offset should be used or not.
> 
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
> 
>  Changes since V2:
>  - None
> 
>  Changes since V1:
>  - Moved patch to front of the series
> 
>  drivers/iommu/ipmmu-vmsa.c |   35 ++++++++++++++++++++++++++++-------
>  1 file changed, 28 insertions(+), 7 deletions(-)
> 
> --- 0007/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c	2017-03-07 12:25:47.000000000 +0900
> @@ -32,11 +32,15 @@
>  
>  #define IPMMU_CTX_MAX 1
>  
> +struct ipmmu_features {
> +	bool use_ns_alias_offset;
> +};
> +
>  struct ipmmu_vmsa_device {
>  	struct device *dev;
>  	void __iomem *base;
>  	struct list_head list;
> -
> +	const struct ipmmu_features *features;
>  	unsigned int num_utlbs;
>  	spinlock_t lock;			/* Protects ctx and domains[] */
>  	DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
> @@ -999,13 +1003,33 @@ static void ipmmu_device_reset(struct ip
>  		ipmmu_write(mmu, i * IM_CTX_SIZE + IMCTR, 0);
>  }
>  
> +static const struct ipmmu_features ipmmu_features_default = {
> +	.use_ns_alias_offset = true,
> +};
> +
> +static const struct of_device_id ipmmu_of_ids[] = {
> +	{
> +		.compatible = "renesas,ipmmu-vmsa",
> +		.data = &ipmmu_features_default,
> +	}, {
> +		/* Terminator */
> +	},
> +};
> +
> +MODULE_DEVICE_TABLE(of, ipmmu_of_ids);
> +
>  static int ipmmu_probe(struct platform_device *pdev)
>  {
>  	struct ipmmu_vmsa_device *mmu;
> +	const struct of_device_id *match;
>  	struct resource *res;
>  	int irq;
>  	int ret;
>  
> +	match = of_match_node(ipmmu_of_ids, pdev->dev.of_node);

of_device_get_match_data() makes this a lot easier.

> +	if (!match)
> +		return -EINVAL;

Also, if the driver is DT-only per the other series, note that this
cannot happen anyway, since of_driver_match_device() would have to have
found a match for your probe function to be called in the first place.

Robin.

> +
>  	mmu = devm_kzalloc(&pdev->dev, sizeof(*mmu), GFP_KERNEL);
>  	if (!mmu) {
>  		dev_err(&pdev->dev, "cannot allocate device data\n");
> @@ -1016,6 +1040,7 @@ static int ipmmu_probe(struct platform_d
>  	mmu->num_utlbs = 32;
>  	spin_lock_init(&mmu->lock);
>  	bitmap_zero(mmu->ctx, IPMMU_CTX_MAX);
> +	mmu->features = match->data;
>  
>  	/* Map I/O memory and request IRQ. */
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -1035,7 +1060,8 @@ static int ipmmu_probe(struct platform_d
>  	 * Offset the registers base unconditionally to point to the non-secure
>  	 * alias space for now.
>  	 */
> -	mmu->base += IM_NS_ALIAS_OFFSET;
> +	if (mmu->features->use_ns_alias_offset)
> +		mmu->base += IM_NS_ALIAS_OFFSET;
>  
>  	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0) {
> @@ -1084,11 +1110,6 @@ static int ipmmu_remove(struct platform_
>  	return 0;
>  }
>  
> -static const struct of_device_id ipmmu_of_ids[] = {
> -	{ .compatible = "renesas,ipmmu-vmsa", },
> -	{ }
> -};
> -
>  static struct platform_driver ipmmu_driver = {
>  	.driver = {
>  		.name = "ipmmu-vmsa",
> 

Re: [PATCH v3 01/09] iommu/ipmmu-vmsa: Introduce features, break out alias

[Magnus Damm]

Hi Robin,

On Wed, Mar 8, 2017 at 8:53 PM, Robin Murphy <robin.murphy@arm.com> wrote:
> Hi Magnus,
>
> On 08/03/17 11:01, Magnus Damm wrote:
>> From: Magnus Damm <damm+renesas@opensource.se>
>>
>> Introduce struct ipmmu_features to track various hardware
>> and software implementation changes inside the driver for
>> different kinds of IPMMU hardware. Add use_ns_alias_offset
>> as a first example of a feature to control if the secure
>> register bank offset should be used or not.
>>
>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>> ---
>>
>>  Changes since V2:
>>  - None
>>
>>  Changes since V1:
>>  - Moved patch to front of the series
>>
>>  drivers/iommu/ipmmu-vmsa.c |   35 ++++++++++++++++++++++++++++-------
>>  1 file changed, 28 insertions(+), 7 deletions(-)
>>
>> --- 0007/drivers/iommu/ipmmu-vmsa.c
>> +++ work/drivers/iommu/ipmmu-vmsa.c   2017-03-07 12:25:47.000000000 +0900
>> @@ -32,11 +32,15 @@
>>
>>  #define IPMMU_CTX_MAX 1
>>
>> +struct ipmmu_features {
>> +     bool use_ns_alias_offset;
>> +};
>> +
>>  struct ipmmu_vmsa_device {
>>       struct device *dev;
>>       void __iomem *base;
>>       struct list_head list;
>> -
>> +     const struct ipmmu_features *features;
>>       unsigned int num_utlbs;
>>       spinlock_t lock;                        /* Protects ctx and domains[] */
>>       DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
>> @@ -999,13 +1003,33 @@ static void ipmmu_device_reset(struct ip
>>               ipmmu_write(mmu, i * IM_CTX_SIZE + IMCTR, 0);
>>  }
>>
>> +static const struct ipmmu_features ipmmu_features_default = {
>> +     .use_ns_alias_offset = true,
>> +};
>> +
>> +static const struct of_device_id ipmmu_of_ids[] = {
>> +     {
>> +             .compatible = "renesas,ipmmu-vmsa",
>> +             .data = &ipmmu_features_default,
>> +     }, {
>> +             /* Terminator */
>> +     },
>> +};
>> +
>> +MODULE_DEVICE_TABLE(of, ipmmu_of_ids);
>> +
>>  static int ipmmu_probe(struct platform_device *pdev)
>>  {
>>       struct ipmmu_vmsa_device *mmu;
>> +     const struct of_device_id *match;
>>       struct resource *res;
>>       int irq;
>>       int ret;
>>
>> +     match = of_match_node(ipmmu_of_ids, pdev->dev.of_node);
>
> of_device_get_match_data() makes this a lot easier.
>
>> +     if (!match)
>> +             return -EINVAL;
>
> Also, if the driver is DT-only per the other series, note that this
> cannot happen anyway, since of_driver_match_device() would have to have
> found a match for your probe function to be called in the first place.

Yeah, you are right. As you know, in the IPMMU driver (with the
r8a7795 V3 series applied) the init handling is a bit special with
ARM32 and ARM64 being treated differently. I would like to clean it up
and share a common implementation.

Until that happens, how do you think we should handle the (!match)
case? BUG_ON()?

Cheers,

/ magnus

[PATCH v3 02/09] iommu/ipmmu-vmsa: Add optional root device feature

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Add root device handling to the IPMMU driver by allowing certain
DT compat strings to enable has_cache_leaf_nodes that in turn will
support both root devices with interrupts and leaf devices that
face the actual IPMMU consumer devices.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Changes since V2:
 - Fixed a bug in ipmmu_find_root() when only leaf devices are present
 - Broke out __ipmmu_find_root() to allow ->xlate() check for root devices

 Changes since V1:
 - Moved patch to earlier in the series
 - Updated code to work with recent changes in:
   [PATCH v3 00/06] iommu/ipmmu-vmsa: IPMMU multi-arch update V3

 drivers/iommu/ipmmu-vmsa.c |   95 ++++++++++++++++++++++++++++++++++++--------
 1 file changed, 78 insertions(+), 17 deletions(-)

--- 0011/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2017-03-08 17:56:51.770607110 +0900
@@ -34,6 +34,7 @@
 
 struct ipmmu_features {
 	bool use_ns_alias_offset;
+	bool has_cache_leaf_nodes;
 };
 
 struct ipmmu_vmsa_device {
@@ -41,6 +42,7 @@ struct ipmmu_vmsa_device {
 	void __iomem *base;
 	struct list_head list;
 	const struct ipmmu_features *features;
+	bool is_leaf;
 	unsigned int num_utlbs;
 	spinlock_t lock;			/* Protects ctx and domains[] */
 	DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
@@ -51,6 +53,7 @@ struct ipmmu_vmsa_device {
 
 struct ipmmu_vmsa_domain {
 	struct ipmmu_vmsa_device *mmu;
+	struct ipmmu_vmsa_device *root;
 	struct iommu_domain io_domain;
 
 	struct io_pgtable_cfg cfg;
@@ -216,6 +219,44 @@ static void set_archdata(struct device *
 #define IMUASID_ASID0_SHIFT		0
 
 /* -----------------------------------------------------------------------------
+ * Root device handling
+ */
+
+static bool ipmmu_is_root(struct ipmmu_vmsa_device *mmu)
+{
+	if (mmu->features->has_cache_leaf_nodes)
+		return mmu->is_leaf ? false : true;
+	else
+		return true; /* older IPMMU hardware treated as single root */
+}
+
+static struct ipmmu_vmsa_device *__ipmmu_find_root(void)
+{
+	struct ipmmu_vmsa_device *mmu;
+	bool found = false;
+
+	spin_lock(&ipmmu_devices_lock);
+
+	list_for_each_entry(mmu, &ipmmu_devices, list) {
+		if (ipmmu_is_root(mmu)) {
+			found = true;
+			break;
+		}
+	}
+
+	spin_unlock(&ipmmu_devices_lock);
+	return found ? mmu : NULL;
+}
+
+static struct ipmmu_vmsa_device *ipmmu_find_root(struct ipmmu_vmsa_device *leaf)
+{
+	if (ipmmu_is_root(leaf))
+		return leaf;
+	else
+		return __ipmmu_find_root();
+}
+
+/* -----------------------------------------------------------------------------
  * Read/Write Access
  */
 
@@ -232,13 +273,13 @@ static void ipmmu_write(struct ipmmu_vms
 
 static u32 ipmmu_ctx_read(struct ipmmu_vmsa_domain *domain, unsigned int reg)
 {
-	return ipmmu_read(domain->mmu, domain->context_id * IM_CTX_SIZE + reg);
+	return ipmmu_read(domain->root, domain->context_id * IM_CTX_SIZE + reg);
 }
 
 static void ipmmu_ctx_write(struct ipmmu_vmsa_domain *domain, unsigned int reg,
 			    u32 data)
 {
-	ipmmu_write(domain->mmu, domain->context_id * IM_CTX_SIZE + reg, data);
+	ipmmu_write(domain->root, domain->context_id * IM_CTX_SIZE + reg, data);
 }
 
 /* -----------------------------------------------------------------------------
@@ -373,7 +414,7 @@ static int ipmmu_domain_init_context(str
 	 * TODO: Add support for coherent walk through CCI with DVM and remove
 	 * cache handling. For now, delegate it to the io-pgtable code.
 	 */
-	domain->cfg.iommu_dev = domain->mmu->dev;
+	domain->cfg.iommu_dev = domain->root->dev;
 
 	domain->iop = alloc_io_pgtable_ops(ARM_32_LPAE_S1, &domain->cfg,
 					   domain);
@@ -383,7 +424,7 @@ static int ipmmu_domain_init_context(str
 	/*
 	 * Find an unused context.
 	 */
-	ret = ipmmu_domain_allocate_context(domain->mmu, domain);
+	ret = ipmmu_domain_allocate_context(domain->root, domain);
 	if (ret == IPMMU_CTX_MAX) {
 		free_io_pgtable_ops(domain->iop);
 		return -EBUSY;
@@ -454,7 +495,7 @@ static void ipmmu_domain_destroy_context
 	 */
 	ipmmu_ctx_write(domain, IMCTR, IMCTR_FLUSH);
 	ipmmu_tlb_sync(domain);
-	ipmmu_domain_free_context(domain->mmu, domain->context_id);
+	ipmmu_domain_free_context(domain->root, domain->context_id);
 }
 
 /* -----------------------------------------------------------------------------
@@ -567,7 +608,7 @@ static int ipmmu_attach_device(struct io
 			       struct device *dev)
 {
 	struct ipmmu_vmsa_archdata *archdata = to_archdata(dev);
-	struct ipmmu_vmsa_device *mmu = archdata->mmu;
+	struct ipmmu_vmsa_device *root, *mmu = archdata->mmu;
 	struct ipmmu_vmsa_domain *domain = to_vmsa_domain(io_domain);
 	unsigned long flags;
 	unsigned int i;
@@ -578,11 +619,18 @@ static int ipmmu_attach_device(struct io
 		return -ENXIO;
 	}
 
+	root = ipmmu_find_root(archdata->mmu);
+	if (!root) {
+		dev_err(dev, "Unable to locate root IPMMU\n");
+		return -EAGAIN;
+	}
+
 	spin_lock_irqsave(&domain->lock, flags);
 
 	if (!domain->mmu) {
 		/* The domain hasn't been used yet, initialize it. */
 		domain->mmu = mmu;
+		domain->root = root;
 		ret = ipmmu_domain_init_context(domain);
 	} else if (domain->mmu != mmu) {
 		/*
@@ -1005,6 +1053,7 @@ static void ipmmu_device_reset(struct ip
 
 static const struct ipmmu_features ipmmu_features_default = {
 	.use_ns_alias_offset = true,
+	.has_cache_leaf_nodes = false,
 };
 
 static const struct of_device_id ipmmu_of_ids[] = {
@@ -1064,19 +1113,31 @@ static int ipmmu_probe(struct platform_d
 		mmu->base += IM_NS_ALIAS_OFFSET;
 
 	irq = platform_get_irq(pdev, 0);
-	if (irq < 0) {
-		dev_err(&pdev->dev, "no IRQ found\n");
-		return irq;
-	}
 
-	ret = devm_request_irq(&pdev->dev, irq, ipmmu_irq, 0,
-			       dev_name(&pdev->dev), mmu);
-	if (ret < 0) {
-		dev_err(&pdev->dev, "failed to request IRQ %d\n", irq);
-		return ret;
-	}
+	/*
+	 * Determine if this IPMMU instance is a leaf device by checking
+	 * if the renesas,ipmmu-main property exists or not.
+	 */
+	if (mmu->features->has_cache_leaf_nodes &&
+	    of_find_property(pdev->dev.of_node, "renesas,ipmmu-main", NULL))
+		mmu->is_leaf = true;
+
+	/* Root devices have mandatory IRQs */
+	if (ipmmu_is_root(mmu)) {
+		if (irq < 0) {
+			dev_err(&pdev->dev, "no IRQ found\n");
+			return irq;
+		}
 
-	ipmmu_device_reset(mmu);
+		ret = devm_request_irq(&pdev->dev, irq, ipmmu_irq, 0,
+				       dev_name(&pdev->dev), mmu);
+		if (ret < 0) {
+			dev_err(&pdev->dev, "failed to request IRQ %d\n", irq);
+			return ret;
+		}
+
+		ipmmu_device_reset(mmu);
+	}
 
 	/*
 	 * We can't create the ARM mapping here as it requires the bus to have

Re: [PATCH v3 02/09] iommu/ipmmu-vmsa: Add optional root device feature

[Geert Uytterhoeven]

Hi Magnus,

On Wed, Mar 8, 2017 at 12:01 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
>
> Add root device handling to the IPMMU driver by allowing certain
> DT compat strings to enable has_cache_leaf_nodes that in turn will
> support both root devices with interrupts and leaf devices that
> face the actual IPMMU consumer devices.
>
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>

> --- 0011/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c     2017-03-08 17:56:51.770607110 +0900

> @@ -216,6 +219,44 @@ static void set_archdata(struct device *
>  #define IMUASID_ASID0_SHIFT            0
>
>  /* -----------------------------------------------------------------------------
> + * Root device handling
> + */
> +
> +static bool ipmmu_is_root(struct ipmmu_vmsa_device *mmu)
> +{
> +       if (mmu->features->has_cache_leaf_nodes)
> +               return mmu->is_leaf ? false : true;

Expressions using the ternary operator are sometimes hard to read.
In this case, you want negation, so why not use that?

return !mmu->is_leaf;

> +       else

I'd drop the else.

> +               return true; /* older IPMMU hardware treated as single root */
> +}
> +
> +static struct ipmmu_vmsa_device *__ipmmu_find_root(void)
> +{
> +       struct ipmmu_vmsa_device *mmu;
> +       bool found = false;

struct ipmmu_vmsa_device *root = NULL;

> +
> +       spin_lock(&ipmmu_devices_lock);
> +
> +       list_for_each_entry(mmu, &ipmmu_devices, list) {
> +               if (ipmmu_is_root(mmu)) {
> +                       found = true;

root = mmu;

> +                       break;
> +               }
> +       }
> +
> +       spin_unlock(&ipmmu_devices_lock);
> +       return found ? mmu : NULL;

return root;

> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 02/09] iommu/ipmmu-vmsa: Add optional root device feature

[Magnus Damm]

Hi Geert,

On Wed, Mar 8, 2017 at 10:47 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Magnus,
>
> On Wed, Mar 8, 2017 at 12:01 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
>> From: Magnus Damm <damm+renesas@opensource.se>
>>
>> Add root device handling to the IPMMU driver by allowing certain
>> DT compat strings to enable has_cache_leaf_nodes that in turn will
>> support both root devices with interrupts and leaf devices that
>> face the actual IPMMU consumer devices.
>>
>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>
>> --- 0011/drivers/iommu/ipmmu-vmsa.c
>> +++ work/drivers/iommu/ipmmu-vmsa.c     2017-03-08 17:56:51.770607110 +0900
>
>> @@ -216,6 +219,44 @@ static void set_archdata(struct device *
>>  #define IMUASID_ASID0_SHIFT            0
>>
>>  /* -----------------------------------------------------------------------------
>> + * Root device handling
>> + */
>> +
>> +static bool ipmmu_is_root(struct ipmmu_vmsa_device *mmu)
>> +{
>> +       if (mmu->features->has_cache_leaf_nodes)
>> +               return mmu->is_leaf ? false : true;
>
> Expressions using the ternary operator are sometimes hard to read.
> In this case, you want negation, so why not use that?
>
> return !mmu->is_leaf;
>
>> +       else
>
> I'd drop the else.

Yeah, your suggestion makes the code easier to read. Will fix.

>> +               return true; /* older IPMMU hardware treated as single root */
>> +}
>> +
>> +static struct ipmmu_vmsa_device *__ipmmu_find_root(void)
>> +{
>> +       struct ipmmu_vmsa_device *mmu;
>> +       bool found = false;
>
> struct ipmmu_vmsa_device *root = NULL;

I used to have it initialized to NULL and not use any found variable
and only return the variable. But then I ran into the error case when
devices exist on the ipmmu_devices list however none of them are root.
I returned the last one on the list regardless if they were root or
not. So I updated the code to use the found variable, and because of
that I thought I could simply drop the NULL assignment.

>> +
>> +       spin_lock(&ipmmu_devices_lock);
>> +
>> +       list_for_each_entry(mmu, &ipmmu_devices, list) {
>> +               if (ipmmu_is_root(mmu)) {
>> +                       found = true;
>
> root = mmu;
>
>> +                       break;
>> +               }
>> +       }
>> +
>> +       spin_unlock(&ipmmu_devices_lock);
>> +       return found ? mmu : NULL;
>
> return root;

I agree it makes sense to use root as variable name, will fix. Not
sure about the NULL assignment though, can you enlighten me?

Cheers,

/ magnus

[PATCH v3 03/09] iommu/ipmmu-vmsa: Enable multi context support

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Add support for up to 8 contexts. Each context is mapped to one
domain. One domain is assigned one or more slave devices. Contexts
are allocated dynamically and slave devices are grouped together
based on which IPMMU device they are connected to. This makes slave
devices tied to the same IPMMU device share the same IOVA space.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Changes since V2:
 - Updated patch description to reflect code included in:
   [PATCH v7 00/07] iommu/ipmmu-vmsa: IPMMU multi-arch update V7

 Changes since V1:
 - Support up to 8 contexts instead of 4
 - Use feature flag and runtime handling
 - Default to single context

 drivers/iommu/ipmmu-vmsa.c |   38 ++++++++++++++++++++++++++++++--------
 1 file changed, 30 insertions(+), 8 deletions(-)

--- 0012/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2017-03-08 17:59:19.900607110 +0900
@@ -30,11 +30,12 @@
 
 #include "io-pgtable.h"
 
-#define IPMMU_CTX_MAX 1
+#define IPMMU_CTX_MAX 8
 
 struct ipmmu_features {
 	bool use_ns_alias_offset;
 	bool has_cache_leaf_nodes;
+	bool has_eight_ctx;
 };
 
 struct ipmmu_vmsa_device {
@@ -44,6 +45,7 @@ struct ipmmu_vmsa_device {
 	const struct ipmmu_features *features;
 	bool is_leaf;
 	unsigned int num_utlbs;
+	unsigned int num_ctx;
 	spinlock_t lock;			/* Protects ctx and domains[] */
 	DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
 	struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
@@ -376,11 +378,12 @@ static int ipmmu_domain_allocate_context
 
 	spin_lock_irqsave(&mmu->lock, flags);
 
-	ret = find_first_zero_bit(mmu->ctx, IPMMU_CTX_MAX);
-	if (ret != IPMMU_CTX_MAX) {
+	ret = find_first_zero_bit(mmu->ctx, mmu->num_ctx);
+	if (ret != mmu->num_ctx) {
 		mmu->domains[ret] = domain;
 		set_bit(ret, mmu->ctx);
-	}
+	} else
+		ret = -EBUSY;
 
 	spin_unlock_irqrestore(&mmu->lock, flags);
 
@@ -425,9 +428,9 @@ static int ipmmu_domain_init_context(str
 	 * Find an unused context.
 	 */
 	ret = ipmmu_domain_allocate_context(domain->root, domain);
-	if (ret == IPMMU_CTX_MAX) {
+	if (ret < 0) {
 		free_io_pgtable_ops(domain->iop);
-		return -EBUSY;
+		return ret;
 	}
 
 	domain->context_id = ret;
@@ -562,7 +565,7 @@ static irqreturn_t ipmmu_irq(int irq, vo
 	/*
 	 * Check interrupts for all active contexts.
 	 */
-	for (i = 0; i < IPMMU_CTX_MAX; i++) {
+	for (i = 0; i < mmu->num_ctx; i++) {
 		if (!mmu->domains[i])
 			continue;
 		if (ipmmu_domain_irq(mmu->domains[i]) == IRQ_HANDLED)
@@ -632,6 +635,13 @@ static int ipmmu_attach_device(struct io
 		domain->mmu = mmu;
 		domain->root = root;
 		ret = ipmmu_domain_init_context(domain);
+		if (ret < 0) {
+			dev_err(dev, "Unable to initialize IPMMU context\n");
+			domain->mmu = NULL;
+		} else {
+			dev_info(dev, "Using IPMMU context %u\n",
+				 domain->context_id);
+		}
 	} else if (domain->mmu != mmu) {
 		/*
 		 * Something is wrong, we can't attach two devices using
@@ -1047,13 +1057,14 @@ static void ipmmu_device_reset(struct ip
 	unsigned int i;
 
 	/* Disable all contexts. */
-	for (i = 0; i < 4; ++i)
+	for (i = 0; i < mmu->num_ctx; ++i)
 		ipmmu_write(mmu, i * IM_CTX_SIZE + IMCTR, 0);
 }
 
 static const struct ipmmu_features ipmmu_features_default = {
 	.use_ns_alias_offset = true,
 	.has_cache_leaf_nodes = false,
+	.has_eight_ctx = false,
 };
 
 static const struct of_device_id ipmmu_of_ids[] = {
@@ -1112,6 +1123,17 @@ static int ipmmu_probe(struct platform_d
 	if (mmu->features->use_ns_alias_offset)
 		mmu->base += IM_NS_ALIAS_OFFSET;
 
+	/*
+	 * The number of contexts varies with generation and instance.
+	 * Newer SoCs get a total of 8 contexts enabled, older ones just one.
+	 */
+	if (mmu->features->has_eight_ctx)
+		mmu->num_ctx = 8;
+	else
+		mmu->num_ctx = 1;
+
+	WARN_ON(mmu->num_ctx > IPMMU_CTX_MAX);
+
 	irq = platform_get_irq(pdev, 0);
 
 	/*

Re: [PATCH v3 03/09] iommu/ipmmu-vmsa: Enable multi context support

[Robin Murphy]

On 08/03/17 11:01, Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
> 
> Add support for up to 8 contexts. Each context is mapped to one
> domain. One domain is assigned one or more slave devices. Contexts
> are allocated dynamically and slave devices are grouped together
> based on which IPMMU device they are connected to. This makes slave
> devices tied to the same IPMMU device share the same IOVA space.
> 
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
> 
>  Changes since V2:
>  - Updated patch description to reflect code included in:
>    [PATCH v7 00/07] iommu/ipmmu-vmsa: IPMMU multi-arch update V7
> 
>  Changes since V1:
>  - Support up to 8 contexts instead of 4
>  - Use feature flag and runtime handling
>  - Default to single context
> 
>  drivers/iommu/ipmmu-vmsa.c |   38 ++++++++++++++++++++++++++++++--------
>  1 file changed, 30 insertions(+), 8 deletions(-)
> 
> --- 0012/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c	2017-03-08 17:59:19.900607110 +0900
> @@ -30,11 +30,12 @@
>  
>  #include "io-pgtable.h"
>  
> -#define IPMMU_CTX_MAX 1
> +#define IPMMU_CTX_MAX 8
>  
>  struct ipmmu_features {
>  	bool use_ns_alias_offset;
>  	bool has_cache_leaf_nodes;
> +	bool has_eight_ctx;

Wouldn't it be more sensible to just encode a number of contexts
directly, if it isn't reported by the hardware itself? I'm just
imagining future hardware generations... :P

bool also_has_another_eight_ctx_on_top_of_that;
bool wait_no_this_is_the_one_where_ctx_15_isnt_usable;

>  };
>  
>  struct ipmmu_vmsa_device {
> @@ -44,6 +45,7 @@ struct ipmmu_vmsa_device {
>  	const struct ipmmu_features *features;
>  	bool is_leaf;
>  	unsigned int num_utlbs;
> +	unsigned int num_ctx;
>  	spinlock_t lock;			/* Protects ctx and domains[] */
>  	DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
>  	struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
> @@ -376,11 +378,12 @@ static int ipmmu_domain_allocate_context
>  
>  	spin_lock_irqsave(&mmu->lock, flags);
>  
> -	ret = find_first_zero_bit(mmu->ctx, IPMMU_CTX_MAX);
> -	if (ret != IPMMU_CTX_MAX) {
> +	ret = find_first_zero_bit(mmu->ctx, mmu->num_ctx);
> +	if (ret != mmu->num_ctx) {
>  		mmu->domains[ret] = domain;
>  		set_bit(ret, mmu->ctx);

Using test_and_set_bit() in a loop would avoid having to take a lock here.

> -	}
> +	} else
> +		ret = -EBUSY;
>  
>  	spin_unlock_irqrestore(&mmu->lock, flags);
>  
> @@ -425,9 +428,9 @@ static int ipmmu_domain_init_context(str
>  	 * Find an unused context.
>  	 */
>  	ret = ipmmu_domain_allocate_context(domain->root, domain);
> -	if (ret == IPMMU_CTX_MAX) {
> +	if (ret < 0) {
>  		free_io_pgtable_ops(domain->iop);
> -		return -EBUSY;
> +		return ret;
>  	}
>  
>  	domain->context_id = ret;
> @@ -562,7 +565,7 @@ static irqreturn_t ipmmu_irq(int irq, vo
>  	/*
>  	 * Check interrupts for all active contexts.
>  	 */
> -	for (i = 0; i < IPMMU_CTX_MAX; i++) {
> +	for (i = 0; i < mmu->num_ctx; i++) {
>  		if (!mmu->domains[i])
>  			continue;
>  		if (ipmmu_domain_irq(mmu->domains[i]) == IRQ_HANDLED)
> @@ -632,6 +635,13 @@ static int ipmmu_attach_device(struct io
>  		domain->mmu = mmu;
>  		domain->root = root;
>  		ret = ipmmu_domain_init_context(domain);
> +		if (ret < 0) {
> +			dev_err(dev, "Unable to initialize IPMMU context\n");
> +			domain->mmu = NULL;
> +		} else {
> +			dev_info(dev, "Using IPMMU context %u\n",
> +				 domain->context_id);
> +		}
>  	} else if (domain->mmu != mmu) {
>  		/*
>  		 * Something is wrong, we can't attach two devices using
> @@ -1047,13 +1057,14 @@ static void ipmmu_device_reset(struct ip
>  	unsigned int i;
>  
>  	/* Disable all contexts. */
> -	for (i = 0; i < 4; ++i)
> +	for (i = 0; i < mmu->num_ctx; ++i)
>  		ipmmu_write(mmu, i * IM_CTX_SIZE + IMCTR, 0);
>  }
>  
>  static const struct ipmmu_features ipmmu_features_default = {
>  	.use_ns_alias_offset = true,
>  	.has_cache_leaf_nodes = false,
> +	.has_eight_ctx = false,
>  };
>  
>  static const struct of_device_id ipmmu_of_ids[] = {
> @@ -1112,6 +1123,17 @@ static int ipmmu_probe(struct platform_d
>  	if (mmu->features->use_ns_alias_offset)
>  		mmu->base += IM_NS_ALIAS_OFFSET;
>  
> +	/*
> +	 * The number of contexts varies with generation and instance.
> +	 * Newer SoCs get a total of 8 contexts enabled, older ones just one.
> +	 */
> +	if (mmu->features->has_eight_ctx)
> +		mmu->num_ctx = 8;
> +	else
> +		mmu->num_ctx = 1;
> +
> +	WARN_ON(mmu->num_ctx > IPMMU_CTX_MAX);

The likelihood of that happening doesn't appear to warrant a runtime
check. Especially one which probably isn't even generated because it's
trivially resolvable to "if (false)..." at compile time.

Robin.

> +
>  	irq = platform_get_irq(pdev, 0);
>  
>  	/*
> 

Re: [PATCH v3 03/09] iommu/ipmmu-vmsa: Enable multi context support

[Magnus Damm]

Hi Robin,

Thanks for your feedback!

On Wed, Mar 8, 2017 at 9:21 PM, Robin Murphy <robin.murphy@arm.com> wrote:
> On 08/03/17 11:01, Magnus Damm wrote:
>> From: Magnus Damm <damm+renesas@opensource.se>
>>
>> Add support for up to 8 contexts. Each context is mapped to one
>> domain. One domain is assigned one or more slave devices. Contexts
>> are allocated dynamically and slave devices are grouped together
>> based on which IPMMU device they are connected to. This makes slave
>> devices tied to the same IPMMU device share the same IOVA space.
>>
>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>> ---
>>
>>  Changes since V2:
>>  - Updated patch description to reflect code included in:
>>    [PATCH v7 00/07] iommu/ipmmu-vmsa: IPMMU multi-arch update V7
>>
>>  Changes since V1:
>>  - Support up to 8 contexts instead of 4
>>  - Use feature flag and runtime handling
>>  - Default to single context
>>
>>  drivers/iommu/ipmmu-vmsa.c |   38 ++++++++++++++++++++++++++++++--------
>>  1 file changed, 30 insertions(+), 8 deletions(-)
>>
>> --- 0012/drivers/iommu/ipmmu-vmsa.c
>> +++ work/drivers/iommu/ipmmu-vmsa.c   2017-03-08 17:59:19.900607110 +0900
>> @@ -30,11 +30,12 @@
>>
>>  #include "io-pgtable.h"
>>
>> -#define IPMMU_CTX_MAX 1
>> +#define IPMMU_CTX_MAX 8
>>
>>  struct ipmmu_features {
>>       bool use_ns_alias_offset;
>>       bool has_cache_leaf_nodes;
>> +     bool has_eight_ctx;
>
> Wouldn't it be more sensible to just encode a number of contexts
> directly, if it isn't reported by the hardware itself? I'm just
> imagining future hardware generations... :P
>
> bool also_has_another_eight_ctx_on_top_of_that;
> bool wait_no_this_is_the_one_where_ctx_15_isnt_usable;

=)

Sure, I agree with you!

Please note that this is currently a mix of software and hardware
policy. On R-Car Gen2 (ARM32) the legacy code only uses a single
context for now but 4 contexts are supported by hardware according to
the data sheet. The remaining 3 contexts are untested at this point.
For R-Car Gen3 (ARM64) the hardware supports 8 contexts and this patch
enables all of them.

>>  };
>>
>>  struct ipmmu_vmsa_device {
>> @@ -44,6 +45,7 @@ struct ipmmu_vmsa_device {
>>       const struct ipmmu_features *features;
>>       bool is_leaf;
>>       unsigned int num_utlbs;
>> +     unsigned int num_ctx;
>>       spinlock_t lock;                        /* Protects ctx and domains[] */
>>       DECLARE_BITMAP(ctx, IPMMU_CTX_MAX);
>>       struct ipmmu_vmsa_domain *domains[IPMMU_CTX_MAX];
>> @@ -376,11 +378,12 @@ static int ipmmu_domain_allocate_context
>>
>>       spin_lock_irqsave(&mmu->lock, flags);
>>
>> -     ret = find_first_zero_bit(mmu->ctx, IPMMU_CTX_MAX);
>> -     if (ret != IPMMU_CTX_MAX) {
>> +     ret = find_first_zero_bit(mmu->ctx, mmu->num_ctx);
>> +     if (ret != mmu->num_ctx) {
>>               mmu->domains[ret] = domain;
>>               set_bit(ret, mmu->ctx);
>
> Using test_and_set_bit() in a loop would avoid having to take a lock here.

So you mean that in case of test_and_set_bit() returns 1 then we try
find_first_zero_bit() again?

This is not really a performance sensitive part of the driver, so I'm
currently optimizing for code readability. I'm of course all for
dropping the lock, but I have a hard time figuring out how your
suggestion could result in semi-readable code. Any pointers? =)

>> @@ -1112,6 +1123,17 @@ static int ipmmu_probe(struct platform_d
>>       if (mmu->features->use_ns_alias_offset)
>>               mmu->base += IM_NS_ALIAS_OFFSET;
>>
>> +     /*
>> +      * The number of contexts varies with generation and instance.
>> +      * Newer SoCs get a total of 8 contexts enabled, older ones just one.
>> +      */
>> +     if (mmu->features->has_eight_ctx)
>> +             mmu->num_ctx = 8;
>> +     else
>> +             mmu->num_ctx = 1;
>> +
>> +     WARN_ON(mmu->num_ctx > IPMMU_CTX_MAX);
>
> The likelihood of that happening doesn't appear to warrant a runtime
> check. Especially one which probably isn't even generated because it's
> trivially resolvable to "if (false)..." at compile time.

Sure, I agree. Will drop.

Thanks,

/ magnus

[PATCH v3 04/09] iommu/ipmmu-vmsa: Make use of IOMMU_OF_DECLARE()

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Hook up IOMMU_OF_DECLARE() support in case CONFIG_IOMMU_DMA
is enabled. The only current supported case for 32-bit ARM
is disabled, however for 64-bit ARM usage of OF is required.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Changes since V2:
 - Reworked registration code to make use of recently introduced:
     iommu_device_register()
     iommu_device_set_ops()
     iommu_device_set_fwnode()

 Changes since V1:
 - Reworked slightly to fit updated patch order and
   [PATCH v3 00/06] iommu/ipmmu-vmsa: IPMMU multi-arch update V3

 drivers/iommu/ipmmu-vmsa.c |   39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

--- 0012/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2017-03-08 19:08:31.610607110 +0900
@@ -19,6 +19,7 @@
 #include <linux/iommu.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_iommu.h>
 #include <linux/platform_device.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
@@ -41,6 +42,7 @@ struct ipmmu_features {
 struct ipmmu_vmsa_device {
 	struct device *dev;
 	void __iomem *base;
+	struct iommu_device iommu;
 	struct list_head list;
 	const struct ipmmu_features *features;
 	bool is_leaf;
@@ -1161,6 +1163,25 @@ static int ipmmu_probe(struct platform_d
 		ipmmu_device_reset(mmu);
 	}
 
+#if defined(CONFIG_IOMMU_DMA)
+	/*
+	 * Register the IPMMU to the IOMMU subsystem in the following cases:
+	 * - R-Car Gen2 IPMMU (all devices registered)
+	 * - R-Car Gen3 IPMMU (leaf devices only - skip root IPMMU-MM device)
+	 */
+	if (!mmu->features->has_cache_leaf_nodes || mmu->is_leaf) {
+		ret = iommu_device_register(&mmu->iommu);
+		if (ret)
+			return ret;
+
+		iommu_device_set_ops(&mmu->iommu, &ipmmu_ops);
+		iommu_device_set_fwnode(&mmu->iommu,
+					&pdev->dev.of_node->fwnode);
+
+		if (!iommu_present(&platform_bus_type))
+			bus_set_iommu(&platform_bus_type, &ipmmu_ops);
+	}
+#endif
 	/*
 	 * We can't create the ARM mapping here as it requires the bus to have
 	 * an IOMMU, which only happens when bus_set_iommu() is called in
@@ -1204,15 +1225,22 @@ static struct platform_driver ipmmu_driv
 
 static int __init ipmmu_init(void)
 {
+	static bool setup_done;
 	int ret;
 
+	if (setup_done)
+		return 0;
+
 	ret = platform_driver_register(&ipmmu_driver);
 	if (ret < 0)
 		return ret;
 
+#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
 	if (!iommu_present(&platform_bus_type))
 		bus_set_iommu(&platform_bus_type, &ipmmu_ops);
+#endif
 
+	setup_done = true;
 	return 0;
 }
 
@@ -1224,6 +1252,17 @@ static void __exit ipmmu_exit(void)
 subsys_initcall(ipmmu_init);
 module_exit(ipmmu_exit);
 
+#ifdef CONFIG_IOMMU_DMA
+static int __init ipmmu_vmsa_iommu_of_setup(struct device_node *np)
+{
+	ipmmu_init();
+	return 0;
+}
+
+IOMMU_OF_DECLARE(ipmmu_vmsa_iommu_of, "renesas,ipmmu-vmsa",
+		 ipmmu_vmsa_iommu_of_setup);
+#endif
+
 MODULE_DESCRIPTION("IOMMU API for Renesas VMSA-compatible IPMMU");
 MODULE_AUTHOR("Laurent Pinchart <laurent.pinchart@ideasonboard.com>");
 MODULE_LICENSE("GPL v2");

Re: [PATCH v3 04/09] iommu/ipmmu-vmsa: Make use of IOMMU_OF_DECLARE()

[Geert Uytterhoeven]

Hi Magnus,

On Wed, Mar 8, 2017 at 12:02 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
>
> Hook up IOMMU_OF_DECLARE() support in case CONFIG_IOMMU_DMA
> is enabled. The only current supported case for 32-bit ARM
> is disabled, however for 64-bit ARM usage of OF is required.
>
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se

While I'm not such a big fan of *_OF_DECLARE() (it doesn't support deferred
probing, which is needed for any device with dependencies, like clocks and
power domains), what's the rationale for not using IOMMU_OF_DECLARE()
on arm32, and thus the need for setup_done?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 04/09] iommu/ipmmu-vmsa: Make use of IOMMU_OF_DECLARE()

[Magnus Damm]

Hi Geert,

On Wed, Mar 8, 2017 at 10:52 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Magnus,
>
> On Wed, Mar 8, 2017 at 12:02 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
>> From: Magnus Damm <damm+renesas@opensource.se>
>>
>> Hook up IOMMU_OF_DECLARE() support in case CONFIG_IOMMU_DMA
>> is enabled. The only current supported case for 32-bit ARM
>> is disabled, however for 64-bit ARM usage of OF is required.
>>
>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se
>
> While I'm not such a big fan of *_OF_DECLARE() (it doesn't support deferred
> probing, which is needed for any device with dependencies, like clocks and
> power domains), what's the rationale for not using IOMMU_OF_DECLARE()
> on arm32, and thus the need for setup_done?

ARM32 could (and should) be converted over to IOMMU_OF_DECLARE(), but
it is just a matter of timing. If we try to do it before ARM32 is
converted over to CONFIG_IOMMU_DMA=y then we have to handle all the
hairy legacy implementation details of IOMMU support in case of
CONFIG_IOMMU_DMA=n _and_ deal with just moving over the init order
bits to OF. Testing and keeping all the combinations working is a lot
of work.

I prefer to kill two birds with one stone and do a larger feature jump
and move over ARM32 to same state of ARM64 (with OF init) once
CONFIG_IOMMU_DMA=y is ready for 32-bit ARM. Just changing the init
order bits to OF while keeping legacy CONFIG_IOMMU_DMA=n code is
introducing potential errors with not much upside. Unless there is
some other reason to do it that I can't see that is. =)

Cheers,

/ magnus

[PATCH v3 05/09] iommu/ipmmu-vmsa: IPMMU device is 40-bit bus master

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

The r8a7795 IPMMU supports 40-bit bus mastering. Both
the coherent DMA mask and the streaming DMA mask are
set to unlock the 40-bit address space for coherent
allocations and streaming operations.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Changes since V2:
 - Updated the code and commit message to use 40 bits instead of 64 bits

 Changes since V1:
 - Updated the commit message

 drivers/iommu/ipmmu-vmsa.c |    1 +
 1 file changed, 1 insertion(+)

--- 0016/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2017-03-08 18:29:56.390607110 +0900
@@ -1103,6 +1103,7 @@ static int ipmmu_probe(struct platform_d
 	spin_lock_init(&mmu->lock);
 	bitmap_zero(mmu->ctx, IPMMU_CTX_MAX);
 	mmu->features = match->data;
+	dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(40));
 
 	/* Map I/O memory and request IRQ. */
 	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);

[PATCH v3 06/09] iommu/ipmmu-vmsa: Write IMCTR twice

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Write IMCTR both in the root device and the leaf node.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Changes since V2:
 - None

 Changes since V1:
 - None

 drivers/iommu/ipmmu-vmsa.c |   17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

--- 0018/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2017-03-08 18:30:36.870607110 +0900
@@ -286,6 +286,16 @@ static void ipmmu_ctx_write(struct ipmmu
 	ipmmu_write(domain->root, domain->context_id * IM_CTX_SIZE + reg, data);
 }
 
+static void ipmmu_ctx_write2(struct ipmmu_vmsa_domain *domain, unsigned int reg,
+			     u32 data)
+{
+	if (domain->mmu != domain->root)
+		ipmmu_write(domain->mmu,
+			    domain->context_id * IM_CTX_SIZE + reg, data);
+
+	ipmmu_write(domain->root, domain->context_id * IM_CTX_SIZE + reg, data);
+}
+
 /* -----------------------------------------------------------------------------
  * TLB and microTLB Management
  */
@@ -312,7 +322,7 @@ static void ipmmu_tlb_invalidate(struct
 
 	reg = ipmmu_ctx_read(domain, IMCTR);
 	reg |= IMCTR_FLUSH;
-	ipmmu_ctx_write(domain, IMCTR, reg);
+	ipmmu_ctx_write2(domain, IMCTR, reg);
 
 	ipmmu_tlb_sync(domain);
 }
@@ -472,7 +482,8 @@ static int ipmmu_domain_init_context(str
 	 * software management as we have no use for it. Flush the TLB as
 	 * required when modifying the context registers.
 	 */
-	ipmmu_ctx_write(domain, IMCTR, IMCTR_INTEN | IMCTR_FLUSH | IMCTR_MMUEN);
+	ipmmu_ctx_write2(domain, IMCTR,
+			 IMCTR_INTEN | IMCTR_FLUSH | IMCTR_MMUEN);
 
 	return 0;
 }
@@ -498,7 +509,7 @@ static void ipmmu_domain_destroy_context
 	 *
 	 * TODO: Is TLB flush really needed ?
 	 */
-	ipmmu_ctx_write(domain, IMCTR, IMCTR_FLUSH);
+	ipmmu_ctx_write2(domain, IMCTR, IMCTR_FLUSH);
 	ipmmu_tlb_sync(domain);
 	ipmmu_domain_free_context(domain->root, domain->context_id);
 }

Re: [PATCH v3 06/09] iommu/ipmmu-vmsa: Write IMCTR twice

[Robin Murphy]

On 08/03/17 11:02, Magnus Damm wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
> 
> Write IMCTR both in the root device and the leaf node.
> 
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
> ---
> 
>  Changes since V2:
>  - None
> 
>  Changes since V1:
>  - None
> 
>  drivers/iommu/ipmmu-vmsa.c |   17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> --- 0018/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c	2017-03-08 18:30:36.870607110 +0900
> @@ -286,6 +286,16 @@ static void ipmmu_ctx_write(struct ipmmu
>  	ipmmu_write(domain->root, domain->context_id * IM_CTX_SIZE + reg, data);
>  }
>  
> +static void ipmmu_ctx_write2(struct ipmmu_vmsa_domain *domain, unsigned int reg,
> +			     u32 data)

That's pretty cryptic. Maybe both functions could do with less ambiguous
names - something like ipmmu_ctx_write_root() vs. ipmmu_ctx_write_all(),
perhaps? (and if there's a more specific hardware term than "all" that
describes this kind of configuration, even better).

Robin.

> +{
> +	if (domain->mmu != domain->root)
> +		ipmmu_write(domain->mmu,
> +			    domain->context_id * IM_CTX_SIZE + reg, data);
> +
> +	ipmmu_write(domain->root, domain->context_id * IM_CTX_SIZE + reg, data);
> +}
> +
>  /* -----------------------------------------------------------------------------
>   * TLB and microTLB Management
>   */
> @@ -312,7 +322,7 @@ static void ipmmu_tlb_invalidate(struct
>  
>  	reg = ipmmu_ctx_read(domain, IMCTR);
>  	reg |= IMCTR_FLUSH;
> -	ipmmu_ctx_write(domain, IMCTR, reg);
> +	ipmmu_ctx_write2(domain, IMCTR, reg);
>  
>  	ipmmu_tlb_sync(domain);
>  }
> @@ -472,7 +482,8 @@ static int ipmmu_domain_init_context(str
>  	 * software management as we have no use for it. Flush the TLB as
>  	 * required when modifying the context registers.
>  	 */
> -	ipmmu_ctx_write(domain, IMCTR, IMCTR_INTEN | IMCTR_FLUSH | IMCTR_MMUEN);
> +	ipmmu_ctx_write2(domain, IMCTR,
> +			 IMCTR_INTEN | IMCTR_FLUSH | IMCTR_MMUEN);
>  
>  	return 0;
>  }
> @@ -498,7 +509,7 @@ static void ipmmu_domain_destroy_context
>  	 *
>  	 * TODO: Is TLB flush really needed ?
>  	 */
> -	ipmmu_ctx_write(domain, IMCTR, IMCTR_FLUSH);
> +	ipmmu_ctx_write2(domain, IMCTR, IMCTR_FLUSH);
>  	ipmmu_tlb_sync(domain);
>  	ipmmu_domain_free_context(domain->root, domain->context_id);
>  }
> 

Re: [PATCH v3 06/09] iommu/ipmmu-vmsa: Write IMCTR twice

[Magnus Damm]

Hi Robin,

On Wed, Mar 8, 2017 at 9:34 PM, Robin Murphy <robin.murphy@arm.com> wrote:
> On 08/03/17 11:02, Magnus Damm wrote:
>> From: Magnus Damm <damm+renesas@opensource.se>
>>
>> Write IMCTR both in the root device and the leaf node.
>>
>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>> ---
>>
>>  Changes since V2:
>>  - None
>>
>>  Changes since V1:
>>  - None
>>
>>  drivers/iommu/ipmmu-vmsa.c |   17 ++++++++++++++---
>>  1 file changed, 14 insertions(+), 3 deletions(-)
>>
>> --- 0018/drivers/iommu/ipmmu-vmsa.c
>> +++ work/drivers/iommu/ipmmu-vmsa.c   2017-03-08 18:30:36.870607110 +0900
>> @@ -286,6 +286,16 @@ static void ipmmu_ctx_write(struct ipmmu
>>       ipmmu_write(domain->root, domain->context_id * IM_CTX_SIZE + reg, data);
>>  }
>>
>> +static void ipmmu_ctx_write2(struct ipmmu_vmsa_domain *domain, unsigned int reg,
>> +                          u32 data)
>
> That's pretty cryptic. Maybe both functions could do with less ambiguous
> names - something like ipmmu_ctx_write_root() vs. ipmmu_ctx_write_all(),
> perhaps? (and if there's a more specific hardware term than "all" that
> describes this kind of configuration, even better).

Yeah I agree. Will fix in next version!

Thanks,

/ magnus

[PATCH v3 07/09] iommu/ipmmu-vmsa: Make IMBUSCTR setup optional

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Introduce a feature to allow opt-out of setting up
IMBUSCR. The default case is unchanged.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Changes since V2:
 - None

 Changes since V1:
 - Updated the commit message
 - Reworked patch to coexist with the multi context feature

 drivers/iommu/ipmmu-vmsa.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

--- 0020/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2017-03-08 18:32:26.280607110 +0900
@@ -37,6 +37,7 @@ struct ipmmu_features {
 	bool use_ns_alias_offset;
 	bool has_cache_leaf_nodes;
 	bool has_eight_ctx;
+	bool setup_imbuscr;
 };
 
 struct ipmmu_vmsa_device {
@@ -465,10 +466,10 @@ static int ipmmu_domain_init_context(str
 	ipmmu_ctx_write(domain, IMMAIR0, domain->cfg.arm_lpae_s1_cfg.mair[0]);
 
 	/* IMBUSCR */
-	ipmmu_ctx_write(domain, IMBUSCR,
-			ipmmu_ctx_read(domain, IMBUSCR) &
-			~(IMBUSCR_DVM | IMBUSCR_BUSSEL_MASK));
-
+	if (domain->root->features->setup_imbuscr)
+		ipmmu_ctx_write(domain, IMBUSCR,
+				ipmmu_ctx_read(domain, IMBUSCR) &
+				~(IMBUSCR_DVM | IMBUSCR_BUSSEL_MASK));
 	/*
 	 * IMSTR
 	 * Clear all interrupt flags.
@@ -1078,6 +1079,7 @@ static const struct ipmmu_features ipmmu
 	.use_ns_alias_offset = true,
 	.has_cache_leaf_nodes = false,
 	.has_eight_ctx = false,
+	.setup_imbuscr = true,
 };
 
 static const struct of_device_id ipmmu_of_ids[] = {

[PATCH v3 08/09] iommu/ipmmu-vmsa: Allow two bit SL0

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Introduce support for two bit SL0 bitfield in IMTTBCR
by using a separate feature flag.

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Changes since V2:
 - None

 Changes since V1:
 - None

 drivers/iommu/ipmmu-vmsa.c |   15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

--- 0022/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2017-03-08 18:33:07.630607110 +0900
@@ -38,6 +38,7 @@ struct ipmmu_features {
 	bool has_cache_leaf_nodes;
 	bool has_eight_ctx;
 	bool setup_imbuscr;
+	bool twobit_imttbcr_sl0;
 };
 
 struct ipmmu_vmsa_device {
@@ -163,6 +164,10 @@ static void set_archdata(struct device *
 #define IMTTBCR_TSZ0_MASK		(7 << 0)
 #define IMTTBCR_TSZ0_SHIFT		O
 
+#define IMTTBCR_SL0_TWOBIT_LVL_3	(0 << 6)
+#define IMTTBCR_SL0_TWOBIT_LVL_2	(1 << 6)
+#define IMTTBCR_SL0_TWOBIT_LVL_1	(2 << 6)
+
 #define IMBUSCR				0x000c
 #define IMBUSCR_DVM			(1 << 2)
 #define IMBUSCR_BUSSEL_SYS		(0 << 0)
@@ -406,6 +411,7 @@ static int ipmmu_domain_allocate_context
 static int ipmmu_domain_init_context(struct ipmmu_vmsa_domain *domain)
 {
 	u64 ttbr;
+	u32 tmp;
 	int ret;
 
 	/*
@@ -458,9 +464,15 @@ static int ipmmu_domain_init_context(str
 	 * We use long descriptors with inner-shareable WBWA tables and allocate
 	 * the whole 32-bit VA space to TTBR0.
 	 */
+
+	if (domain->root->features->twobit_imttbcr_sl0)
+		tmp = IMTTBCR_SL0_TWOBIT_LVL_1;
+	else
+		tmp = IMTTBCR_SL0_LVL_1;
+
 	ipmmu_ctx_write(domain, IMTTBCR, IMTTBCR_EAE |
 			IMTTBCR_SH0_INNER_SHAREABLE | IMTTBCR_ORGN0_WB_WA |
-			IMTTBCR_IRGN0_WB_WA | IMTTBCR_SL0_LVL_1);
+			IMTTBCR_IRGN0_WB_WA | tmp);
 
 	/* MAIR0 */
 	ipmmu_ctx_write(domain, IMMAIR0, domain->cfg.arm_lpae_s1_cfg.mair[0]);
@@ -1080,6 +1092,7 @@ static const struct ipmmu_features ipmmu
 	.has_cache_leaf_nodes = false,
 	.has_eight_ctx = false,
 	.setup_imbuscr = true,
+	.twobit_imttbcr_sl0 = false,
 };
 
 static const struct of_device_id ipmmu_of_ids[] = {

[PATCH v3 09/09] iommu/ipmmu-vmsa: Hook up r8a7795 DT matching code

[Magnus Damm]

From: Magnus Damm <damm+renesas@opensource.se>

Tie in r8a7795 features and update the IOMMU_OF_DECLARE
compat string to include the updated compat string.

TODO:
 - Consider making use of iommu_fwspec_add_ids() for uTLB handling
     Needed to coexist with non-OF R-Car Gen2 somehow...
 - Break out stuff useful for R-Car Gen2 from this series
     Fix up the Gen2 IPMMU support code
       and/or
     Fold more stuff into the multi-arch series
 - Add support for sysfs and iommu_device_link()/unlink()

Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
---

 Changes since V2:
 - Check for lack of root device in ->xlate()
   This fixed a bug when IPMMU-MM is disabled in DT the system hangs on boot
 - Added code to ipmmu_init_platform_device() to handle multiple ->xlate() calls
 - Include empty white list by default
 - Updated TODO list

 Changes since V1:
 - Enable multi context feature
 - Update TODO list

 drivers/iommu/ipmmu-vmsa.c |   41 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

--- 0018/drivers/iommu/ipmmu-vmsa.c
+++ work/drivers/iommu/ipmmu-vmsa.c	2017-03-08 19:11:53.600607110 +0900
@@ -23,6 +23,7 @@
 #include <linux/platform_device.h>
 #include <linux/sizes.h>
 #include <linux/slab.h>
+#include <linux/sys_soc.h>
 
 #if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
 #include <asm/dma-iommu.h>
@@ -770,6 +771,10 @@ static int ipmmu_init_platform_device(st
 	int num_utlbs;
 	int ret = -ENODEV;
 
+	/* Initialize once - xlate() will call multiple times */
+	if (to_archdata(dev))
+		return 0;
+
 	/* Find the master corresponding to the device. */
 
 	num_utlbs = of_count_phandle_with_args(dev->of_node, "iommus",
@@ -1043,6 +1048,17 @@ static struct iommu_group *ipmmu_find_gr
 	return group;
 }
 
+static bool ipmmu_slave_whitelist(struct device *dev)
+{
+	/* By default, do not allow use of IPMMU */
+	return false;
+}
+
+static const struct soc_device_attribute soc_r8a7795[] = {
+	{ .soc_id = "r8a7795", },
+	{ /* sentinel */ }
+};
+
 static int ipmmu_of_xlate_dma(struct device *dev,
 			      struct of_phandle_args *spec)
 {
@@ -1053,6 +1069,18 @@ static int ipmmu_of_xlate_dma(struct dev
 	if (!of_device_is_available(spec->np))
 		return -ENODEV;
 
+	/* Failing in ->attach_device() results in a hang, so make
+	 * sure the root device is installed before going there
+	 */
+	if (!__ipmmu_find_root()) {
+		dev_info(dev, "Unable to locate IPMMU root device\n");
+		return -ENODEV;
+	}
+
+	/* For R-Car Gen3 use a white list to opt-in slave devices */
+	if (soc_device_match(soc_r8a7795) && !ipmmu_slave_whitelist(dev))
+		return -ENODEV;
+
 	return ipmmu_init_platform_device(dev);
 }
 
@@ -1095,11 +1123,22 @@ static const struct ipmmu_features ipmmu
 	.twobit_imttbcr_sl0 = false,
 };
 
+static const struct ipmmu_features ipmmu_features_r8a7795 = {
+	.use_ns_alias_offset = false,
+	.has_cache_leaf_nodes = true,
+	.has_eight_ctx = true,
+	.setup_imbuscr = false,
+	.twobit_imttbcr_sl0 = true,
+};
+
 static const struct of_device_id ipmmu_of_ids[] = {
 	{
 		.compatible = "renesas,ipmmu-vmsa",
 		.data = &ipmmu_features_default,
 	}, {
+		.compatible = "renesas,ipmmu-r8a7795",
+		.data = &ipmmu_features_r8a7795,
+	}, {
 		/* Terminator */
 	},
 };
@@ -1288,6 +1327,8 @@ static int __init ipmmu_vmsa_iommu_of_se
 
 IOMMU_OF_DECLARE(ipmmu_vmsa_iommu_of, "renesas,ipmmu-vmsa",
 		 ipmmu_vmsa_iommu_of_setup);
+IOMMU_OF_DECLARE(ipmmu_r8a7795_iommu_of, "renesas,ipmmu-r8a7795",
+		 ipmmu_vmsa_iommu_of_setup);
 #endif
 
 MODULE_DESCRIPTION("IOMMU API for Renesas VMSA-compatible IPMMU");

Re: [PATCH v3 09/09] iommu/ipmmu-vmsa: Hook up r8a7795 DT matching code

[Geert Uytterhoeven]

Hi Magnus,

On Wed, Mar 8, 2017 at 12:02 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
> From: Magnus Damm <damm+renesas@opensource.se>
>
> Tie in r8a7795 features and update the IOMMU_OF_DECLARE
> compat string to include the updated compat string.
>
> TODO:
>  - Consider making use of iommu_fwspec_add_ids() for uTLB handling
>      Needed to coexist with non-OF R-Car Gen2 somehow...
>  - Break out stuff useful for R-Car Gen2 from this series
>      Fix up the Gen2 IPMMU support code
>        and/or
>      Fold more stuff into the multi-arch series
>  - Add support for sysfs and iommu_device_link()/unlink()
>
> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>

> --- 0018/drivers/iommu/ipmmu-vmsa.c
> +++ work/drivers/iommu/ipmmu-vmsa.c     2017-03-08 19:11:53.600607110 +0900

> @@ -1043,6 +1048,17 @@ static struct iommu_group *ipmmu_find_gr
>         return group;
>  }
>
> +static bool ipmmu_slave_whitelist(struct device *dev)
> +{
> +       /* By default, do not allow use of IPMMU */
> +       return false;
> +}
> +
> +static const struct soc_device_attribute soc_r8a7795[] = {
> +       { .soc_id = "r8a7795", },

If/when the whitelist is/becomes device/revision specific, you probably want
to store a  pointer to the *_slave_whitelist() function in the .data member?

> +       { /* sentinel */ }
> +};
> +
>  static int ipmmu_of_xlate_dma(struct device *dev,
>                               struct of_phandle_args *spec)
>  {
> @@ -1053,6 +1069,18 @@ static int ipmmu_of_xlate_dma(struct dev
>         if (!of_device_is_available(spec->np))
>                 return -ENODEV;
>
> +       /* Failing in ->attach_device() results in a hang, so make
> +        * sure the root device is installed before going there
> +        */
> +       if (!__ipmmu_find_root()) {
> +               dev_info(dev, "Unable to locate IPMMU root device\n");

dev_err?

> +               return -ENODEV;
> +       }
> +
> +       /* For R-Car Gen3 use a white list to opt-in slave devices */
> +       if (soc_device_match(soc_r8a7795) && !ipmmu_slave_whitelist(dev))
> +               return -ENODEV;

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v3 09/09] iommu/ipmmu-vmsa: Hook up r8a7795 DT matching code

[Magnus Damm]

Hi Geert,

On Wed, Mar 8, 2017 at 10:58 PM, Geert Uytterhoeven
<geert@linux-m68k.org> wrote:
> Hi Magnus,
>
> On Wed, Mar 8, 2017 at 12:02 PM, Magnus Damm <magnus.damm@gmail.com> wrote:
>> From: Magnus Damm <damm+renesas@opensource.se>
>>
>> Tie in r8a7795 features and update the IOMMU_OF_DECLARE
>> compat string to include the updated compat string.
>>
>> TODO:
>>  - Consider making use of iommu_fwspec_add_ids() for uTLB handling
>>      Needed to coexist with non-OF R-Car Gen2 somehow...
>>  - Break out stuff useful for R-Car Gen2 from this series
>>      Fix up the Gen2 IPMMU support code
>>        and/or
>>      Fold more stuff into the multi-arch series
>>  - Add support for sysfs and iommu_device_link()/unlink()
>>
>> Signed-off-by: Magnus Damm <damm+renesas@opensource.se>
>
>> --- 0018/drivers/iommu/ipmmu-vmsa.c
>> +++ work/drivers/iommu/ipmmu-vmsa.c     2017-03-08 19:11:53.600607110 +0900
>
>> @@ -1043,6 +1048,17 @@ static struct iommu_group *ipmmu_find_gr
>>         return group;
>>  }
>>
>> +static bool ipmmu_slave_whitelist(struct device *dev)
>> +{
>> +       /* By default, do not allow use of IPMMU */
>> +       return false;
>> +}
>> +
>> +static const struct soc_device_attribute soc_r8a7795[] = {
>> +       { .soc_id = "r8a7795", },
>
> If/when the whitelist is/becomes device/revision specific, you probably want
> to store a  pointer to the *_slave_whitelist() function in the .data member?

Yeah, for sure. It is a bit early to tell exactly how the code will
look like at this point, but I think it will become more clear in the
future. Just want to send out a new version of r8a7796 IPMMU support
and some r8a7795 DT integration to get a coherent working set of patch
series out of the door first.

>> +       { /* sentinel */ }
>> +};
>> +
>>  static int ipmmu_of_xlate_dma(struct device *dev,
>>                               struct of_phandle_args *spec)
>>  {
>> @@ -1053,6 +1069,18 @@ static int ipmmu_of_xlate_dma(struct dev
>>         if (!of_device_is_available(spec->np))
>>                 return -ENODEV;
>>
>> +       /* Failing in ->attach_device() results in a hang, so make
>> +        * sure the root device is installed before going there
>> +        */
>> +       if (!__ipmmu_find_root()) {
>> +               dev_info(dev, "Unable to locate IPMMU root device\n");
>
> dev_err?

Good idea. Will fix.

>> +               return -ENODEV;
>> +       }
>> +
>> +       /* For R-Car Gen3 use a white list to opt-in slave devices */
>> +       if (soc_device_match(soc_r8a7795) && !ipmmu_slave_whitelist(dev))
>> +               return -ENODEV;

This will have to be updated for r8a7796 somehow as well.

Thanks for your help!

Cheers,

/ magnus