On Mon, 2019-12-16 at 20:13 +0800, Yong Wu wrote: > On Mon, 2019-11-04 at 19:52 +0800, Chao Hao wrote: > > Start with this patch, we will change the SW architecture > > to support multiple domains. SW architecture will has a big change, > > so we need to modify a little bit by more than one patch. > > The new SW overall architecture is as below: > > > > iommu0 iommu1 > > | | > > ----------- > > | > > mtk_iommu_pgtable > > | > > ------------------------------------------ > > | | | > > mtk_iommu_domain1 mtk_iommu_domain2 mtk_iommu_domain3 > > | | | > > iommu_group1 iommu_group2 iommu_group3 > > | | | > > iommu_domain1 iommu_domain2 iommu_domain3 > > | | | > > iova region1(normal) iova region2(CCU) iova region3(VPU) > > > > For current structure, no matter how many iommus there are, > > they use the same page table to simplify the usage of module. > > In order to make the software architecture more explicit, this > > patch will create a global mtk_iommu_pgtable structure to describe > > page table and all the iommus use it. > > Thanks for the hard work of this file. Actually this patch and the later > ones confuse me. Why do you make this flow change? > for making the code "more explicit" or for adding multi-domain support > in 13/13. > > IMHO, the change is unnecessary. > a) For me, this change has no improvement. currently we use a global > mtk_iommu_get_m4u_data to get the M4U data. I will be very glad if you > could get rid of it. But in this patchset, You use a another global > mtk_iommu_pgtable to instead. For me. It has no improvement.
Thanks for you advice! For current SW arch, all the IOMMU HW use the same page table, we can use a global mtk_iommu_pgtable to discribe the information of page table and all the IOMMU attach it, I think that it is more clear and unambiguous. For beginners, it maybe more easily explicable? > > b) This patchset break the original flow. device_group give you a > software chance for initializing, then you move pagetable allocating > code into it. But it isn't device_group job. > As is shown above diagram, mtk_iommu_pgtable includes iommu_group and iommu_domain,so we need to allocate mtk_iommu_pgtable and initialize it in device_group firstly,and then execute the original flow, it only changes place for creating mtk_iommu_pgtable and don't break original device_group flow. > I can not decide if your flow is right. But if you only want to add > support multi-domain, I guess you could extend the current "m4u_group" > to a array "m4u_group[N]". It may be more simple. To make mt6779 > progress easily, I suggest you can use this way to support multi-domain > firstly. Then you could send this new mtk_iommu_pgtable patchset for the > code "more explicit" if you insist. > > > The diagram is as below: > > > > mtk_iommu_data1(MM) mtk_iommu_data2(APU) > > | | > > | | > > ------mtk_iommu_pgtable----- > > > > We need to create global mtk_iommu_pgtable to include all the iova > > regions firstly and special iova regions by divided based on it, > > so the information of pgtable needs to be created in device_group. > > > > Signed-off-by: Chao Hao <[email protected]> > > --- > > drivers/iommu/mtk_iommu.c | 84 +++++++++++++++++++++++++++++++++++++++ > > drivers/iommu/mtk_iommu.h | 1 + > > 2 files changed, 85 insertions(+) > > > > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c > > index f2847e661137..fcbde6b0f58d 100644 > > --- a/drivers/iommu/mtk_iommu.c > > +++ b/drivers/iommu/mtk_iommu.c > > @@ -123,6 +123,12 @@ struct mtk_iommu_domain { > > struct iommu_domain domain; > > }; > > > > +struct mtk_iommu_pgtable { > > + struct io_pgtable_cfg cfg; > > + struct io_pgtable_ops *iop; > > +}; > > + > > +static struct mtk_iommu_pgtable *share_pgtable; > > static const struct iommu_ops mtk_iommu_ops; > > > > /* > > @@ -170,6 +176,11 @@ static struct mtk_iommu_data > > *mtk_iommu_get_m4u_data(void) > > return NULL; > > } > > > > +static struct mtk_iommu_pgtable *mtk_iommu_get_pgtable(void) > > +{ > > + return share_pgtable; > > +} > > + > > static struct mtk_iommu_domain *to_mtk_domain(struct iommu_domain *dom) > > { > > return container_of(dom, struct mtk_iommu_domain, domain); > > @@ -322,6 +333,13 @@ static int mtk_iommu_domain_finalise(struct > > mtk_iommu_domain *dom) > > { > > struct mtk_iommu_data *data = mtk_iommu_get_m4u_data(); > > > > + if (data->pgtable) { > > + dom->cfg = data->pgtable->cfg; > > + dom->iop = data->pgtable->iop; > > + dom->domain.pgsize_bitmap = data->pgtable->cfg.pgsize_bitmap; > > + return 0; > > + } > > + > > dom->cfg = (struct io_pgtable_cfg) { > > .quirks = IO_PGTABLE_QUIRK_ARM_NS | > > IO_PGTABLE_QUIRK_NO_PERMS | > > @@ -345,6 +363,61 @@ static int mtk_iommu_domain_finalise(struct > > mtk_iommu_domain *dom) > > return 0; > > } > > > > +static struct mtk_iommu_pgtable *create_pgtable(struct mtk_iommu_data > > *data) > > +{ > > + struct mtk_iommu_pgtable *pgtable; > > + > > + pgtable = kzalloc(sizeof(*pgtable), GFP_KERNEL); > > + if (!pgtable) > > + return ERR_PTR(-ENOMEM); > > + > > + pgtable->cfg = (struct io_pgtable_cfg) { > > + .quirks = IO_PGTABLE_QUIRK_ARM_NS | > > + IO_PGTABLE_QUIRK_NO_PERMS | > > + IO_PGTABLE_QUIRK_TLBI_ON_MAP | > > + IO_PGTABLE_QUIRK_ARM_MTK_EXT, > > + .pgsize_bitmap = mtk_iommu_ops.pgsize_bitmap, > > + .ias = 32, > > + .oas = 34, > > + .tlb = &mtk_iommu_flush_ops, > > + .iommu_dev = data->dev, > > + }; > > + > > + pgtable->iop = alloc_io_pgtable_ops(ARM_V7S, &pgtable->cfg, data); > > + if (!pgtable->iop) { > > + dev_err(data->dev, "Failed to alloc io pgtable\n"); > > + return ERR_PTR(-EINVAL); > > + } > > + > > + dev_info(data->dev, "%s create pgtable done\n", __func__); > > + > > + return pgtable; > > +} > > + > > +static int mtk_iommu_attach_pgtable(struct mtk_iommu_data *data, > > + struct device *dev) > > +{ > > + struct mtk_iommu_pgtable *pgtable = mtk_iommu_get_pgtable(); > > + > > + /* create share pgtable */ > > + if (!pgtable) { > > + pgtable = create_pgtable(data); > > + if (IS_ERR(pgtable)) { > > + dev_err(data->dev, "Failed to create pgtable\n"); > > + return -ENOMEM; > > + } > > + > > + share_pgtable = pgtable; > > + } > > + > > + /* binding to pgtable */ > > + data->pgtable = pgtable; > > + > > + dev_info(data->dev, "m4u%d attach_pgtable done!\n", data->m4u_id); > > + > > + return 0; > > +} > > + > > static struct iommu_domain *mtk_iommu_domain_alloc(unsigned type) > > { > > struct mtk_iommu_domain *dom; > > @@ -508,10 +581,21 @@ static void mtk_iommu_remove_device(struct device > > *dev) > > static struct iommu_group *mtk_iommu_device_group(struct device *dev) > > { > > struct mtk_iommu_data *data = mtk_iommu_get_m4u_data(); > > + struct mtk_iommu_pgtable *pgtable; > > + int ret = 0; > > > > if (!data) > > return ERR_PTR(-ENODEV); > > > > + pgtable = data->pgtable; > > + if (!pgtable) { > > + ret = mtk_iommu_attach_pgtable(data, dev); > > + if (ret) { > > + dev_err(data->dev, "Failed to device_group\n"); > > + return NULL; > > + } > > + } > > + > > /* All the client devices are in the same m4u iommu-group */ > > if (!data->m4u_group) { > > data->m4u_group = iommu_group_alloc(); > > diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h > > index 132dc765a40b..dd5f19f78b62 100644 > > --- a/drivers/iommu/mtk_iommu.h > > +++ b/drivers/iommu/mtk_iommu.h > > @@ -61,6 +61,7 @@ struct mtk_iommu_data { > > struct clk *bclk; > > phys_addr_t protect_base; /* protect memory base */ > > struct mtk_iommu_suspend_reg reg; > > + struct mtk_iommu_pgtable *pgtable; > > struct mtk_iommu_domain *m4u_dom; > > struct iommu_group *m4u_group; > > bool enable_4GB; > > _______________________________________________ iommu mailing list [email protected] https://lists.linuxfoundation.org/mailman/listinfo/iommu
