On Wed, 2015-12-16 at 12:48 +0000, Robin Murphy wrote:

On 16/12/15 05:59, Yong Wu wrote:

quoted

On Tue, 2015-12-15 at 12:37 +0000, Robin Murphy wrote:

quoted

On 15/12/15 03:28, Yong Wu wrote:

quoted

On Mon, 2015-12-14 at 15:16 +0100, Joerg Roedel wrote:

quoted

On Tue, Dec 08, 2015 at 05:49:12PM +0800, Yong Wu wrote:

[...]

quoted

Following your comment above, I test as below. Then the flows seems meet
the "best case" that the iommu core will help create default DMA domain.

@@ -664,19 +636,41 @@ static int mtk_iommu_probe(struct platform_device

*pdev)
for (i = 0; i < larb_nr; i++) {
		struct device_node *larbnode;
		struct platform_device *plarbdev;

		larbnode = of_parse_phandle(dev->of_node, "mediatek,larbs", i);
		if (!larbnode)
			return -EINVAL;

                 plarbdev = of_find_device_by_node(larbnode);
                 of_node_put(larbnode);
-               if (!plarbdev)
-                       return -EPROBE_DEFER;
+               if (!plarbdev) {
+                       plarbdev = of_platform_device_create(larbnode,
NULL, platform_bus_type.dev_root);
+                       if (IS_ERR(pdev))
+                               return -EPROBE_DEFER;
+               }
}

I only add of_platform_device_create for the SMI local arbiter devices
here.

This is a big improvement for us. If this is ok, I will send a quick
next version for this.

In my opinion it's reasonable - we need the whole "IOMMU" to be ready, 

  Thanks.

so if we already have to short-cut the creation of the M4U part it only 
seems fair to do the same for the SMI part. That said, would it work to 
just unconditionally poke the larbs in mtk_iommu_init_fn() before you 
poke the M4U itself? It would be nice to keep all that stuff together in 
the same place.

    mtk_iommu_init_fn don't have the larb's "struct device_node". So I
cann't create its platform_device directly.

    I have tried 2 method:
   a) add a mtk_smi_larb_init_fn in the SMI patch.

static int mtk_smi_larb_init_fn(struct device_node *np)
{
	struct platform_device *pdev;

	pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root);

	return IS_ERR(pdev) ? PTR_ERR(pdev) :  0;
}
IOMMU_OF_DECLARE(mtk_smi_larb, "mediatek,mt8173-smi-larb",
mtk_smi_larb_init_fn);

   This don't work. It will run after mtk_iommu_init_fn. then the larb's
platform_device also don't exist while m4u's probe.

  b) Copy the code below to mtk_iommu_init_fn.

   for (i = 0; i < larb_nr; i++) {
      xxx
      plarbdev = of_platform_device_create(larbnode,
                      NULL, platform_bus_type.dev_root);
   }

   It works. But then there are 2 same code of parsing the SMI local
arbiter(one is in mtk_iommu_init_fn, the other is in mtk_iommu_init_fn).
It looks not good. I think that the one I wrote in the previous mail is
better, It only add 3 lines, What's your opinion?

quoted

quoted

quoted

quoted

quoted

+static struct iommu_group *mtk_iommu_device_group(struct device *dev)
+{
+	struct mtk_iommu_data *data;
+	struct mtk_iommu_client_priv *priv;
+
+	priv = dev->archdata.iommu;
+	if (!priv)
+		return ERR_PTR(-ENODEV);
+
+	/* All the client devices are in the same m4u iommu-group */
+	data = dev_get_drvdata(priv->m4udev);
+	if (!data->m4u_group) {
+		data->m4u_group = iommu_group_alloc();
+		if (IS_ERR(data->m4u_group))
+			dev_err(dev, "Failed to allocate M4U IOMMU group\n");
+	}
+	return data->m4u_group;
+}

As long as this works as expected, then AFAICS you shouldn't have to
have *any* special-case behaviour or tracking of domains at all.

We only need one iommu-group, one iommu domain here.

What's the special-case behavior, how can we track of domains.
Could you help give me a example?

The beauty of it is that you don't need to. If you guarantee all of an 
IOMMU's client devices are in the same group, you know you've only got 
one thing which can be attached to that IOMMU's domains. Therefore, you 
can freely allow as many domains as you like to *exist*, because there 
can never be more than one *active* at any given time - the core code 
enforces that the group is detached from one domain before being 
attached to another, and the driver's attach and detach calls just 
become responsible for switching the given domain's page table in and 
out of the actual hardware. I think it's pretty neat.

It seems that mtk-iommu can not detach/attach dynamically. the iommu
core don't support iommu_detach_device/iommu_attach_device whose
iommu-group have many devices.(Normally there is only one device in a
iommu-group). 

So currently we only iommu_attach_device while probe, it will never
attach/detach again.

All our multimedia modules are in the m4u'domain and share m4u's
pagetable, They won't change pagetable.

Robin.