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.