On Wed, 2015-12-16 at 12:48 +0000, Robin Murphy wrote: > On 16/12/15 05:59, Yong Wu wrote: > > On Tue, 2015-12-15 at 12:37 +0000, Robin Murphy wrote: > >> On 15/12/15 03:28, Yong Wu wrote: > >>> On Mon, 2015-12-14 at 15:16 +0100, Joerg Roedel wrote: > >>>> On Tue, Dec 08, 2015 at 05:49:12PM +0800, Yong Wu wrote: [...] > > 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? > >>>>> +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. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html