Linux-mediatek Archive on lore.kernel.org
 help / color / Atom feed
From: Yong Wu <yong.wu@mediatek.com>
To: chao hao <Chao.Hao@mediatek.com>
Cc: "Anan Sun (孙安安)" <Anan.Sun@mediatek.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"Cui Zhang (张翠)" <Cui.Zhang@mediatek.com>,
	"Jun Yan (颜军)" <Jun.Yan@mediatek.com>,
	wsd_upstream <wsd_upstream@mediatek.com>,
	"Joerg Roedel" <joro@8bytes.org>,
	youlin.pei@mediatek.com,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"iommu@lists.linux-foundation.org"
	<iommu@lists.linux-foundation.org>,
	"Rob Herring" <robh+dt@kernel.org>,
	"linux-mediatek@lists.infradead.org"
	<linux-mediatek@lists.infradead.org>,
	"Miles Chen (陳民樺)" <Miles.Chen@mediatek.com>,
	"Matthias Brugger" <matthias.bgg@gmail.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"Guangming Cao (曹光明)" <Guangming.Cao@mediatek.com>
Subject: Re: [RESEND,PATCH 03/13] iommu/mediatek: Add mtk_iommu_pgtable structure
Date: Sat, 15 Feb 2020 20:17:26 +0800
Message-ID: <1581769046.32039.27.camel@mhfsdcap03> (raw)
In-Reply-To: <1577785148.30177.5.camel@mbjsdccf07>

On Tue, 2019-12-31 at 17:39 +0800, chao hao wrote:
> 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

What's your plan if the 4GB iova range is not enough for us in future?
Do you plan to add a new global mtk_iommu_pgtable again?

> and all the IOMMU attach it, I think that it is more clear and
> unambiguous. For beginners, it maybe more easily explicable? 

I still don't get the necessity of this change. it is only for making
code clear from your point for view, right?

This code has been reviewed for many years, I don't know why you think
it is ambiguous. it is clear for me at lease. and I will complain that
you add a new global variable in this change.

> > 
> > 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 understand you have to do this change after you adjust the structure.
I mean that it may be not proper since allocating pagetable should not
be done in device_group logically. From here, Could we get this change
looks not good?.

> > 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.

Could you help try this way if it could meet your requirement? Then
let's compare which one is better.


BTW, your patches(including v2) cause hangup as below since
"data->m4u_dom" was uninitialized.


Unable to handle kernel NULL pointer dereference at virtual address
0000000000000010
...
pc : mtk_iommu_tlb_flush_page_nosync+0x38/0xb8
lr : __arm_v7s_unmap+0x174/0x598
...
Call trace:
 mtk_iommu_tlb_flush_page_nosync+0x38/0xb8
 __arm_v7s_unmap+0x174/0x598
 arm_v7s_unmap+0x30/0x48
 mtk_iommu_unmap+0x20/0x28
 __iommu_unmap+0xa4/0xf8
 iommu_unmap+0x44/0x90

> > 
> > > 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 <chao.hao@mediatek.com>
> > > ---
> > >  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;
> > 
> > 
> 
> 

_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek

  reply index

Thread overview: 22+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-11-04 11:52 [RESEND,PATCH 00/13] MT6779 IOMMU SUPPORT Chao Hao
2019-11-04 11:52 ` [RESEND,PATCH 01/13] dt-bindings: mediatek: Add bindings for MT6779 Chao Hao
2019-11-06 23:40   ` Rob Herring
2019-12-16 12:05   ` Yong Wu
2019-12-20 11:01     ` chao hao
2019-11-04 11:52 ` [RESEND,PATCH 02/13] iommu/mediatek: Add mt6779 IOMMU basic support Chao Hao
2019-12-16 12:07   ` Yong Wu
2019-12-25  6:58     ` chao hao
2019-11-04 11:52 ` [RESEND,PATCH 03/13] iommu/mediatek: Add mtk_iommu_pgtable structure Chao Hao
2019-12-16 12:13   ` Yong Wu
2019-12-31  9:39     ` chao hao
2020-02-15 12:17       ` Yong Wu [this message]
2019-11-04 11:52 ` [RESEND, PATCH 04/13] iommu/mediatek: Remove mtk_iommu_domain_finalise Chao Hao
2019-11-04 11:52 ` [RESEND, PATCH 05/13] iommu/mediatek: Remove pgtable info in mtk_iommu_domain Chao Hao
2019-11-04 11:52 ` [RESEND,PATCH 06/13] iommu/mediatek: Change get the way of m4u_group Chao Hao
2019-11-04 11:52 ` [RESEND,PATCH 07/13] iommu/mediatek: Add smi_larb info about device Chao Hao
2019-11-04 11:52 ` [RESEND,PATCH 08/13] iommu/mediatek: Add mtk_domain_data structure Chao Hao
2019-11-04 11:52 ` [RESEND, PATCH 09/13] iommu/mediatek: Remove the usage of m4u_dom variable Chao Hao
2019-11-04 11:52 ` [RESEND, PATCH 10/13] iommu/mediatek: Remove mtk_iommu_get_m4u_data api Chao Hao
2019-11-04 11:52 ` [RESEND,PATCH 11/13] iommu/mediatek: Add iova reserved function Chao Hao
2019-11-04 11:52 ` [RESEND, PATCH 12/13] iommu/mediatek: Change single domain to multiple domains Chao Hao
2019-11-04 11:52 ` [RESEND, PATCH 13/13] iommu/mediatek: Add multiple mtk_iommu_domain support for mt6779 Chao Hao

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1581769046.32039.27.camel@mhfsdcap03 \
    --to=yong.wu@mediatek.com \
    --cc=Anan.Sun@mediatek.com \
    --cc=Chao.Hao@mediatek.com \
    --cc=Cui.Zhang@mediatek.com \
    --cc=Guangming.Cao@mediatek.com \
    --cc=Jun.Yan@mediatek.com \
    --cc=Miles.Chen@mediatek.com \
    --cc=devicetree@vger.kernel.org \
    --cc=iommu@lists.linux-foundation.org \
    --cc=joro@8bytes.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=matthias.bgg@gmail.com \
    --cc=robh+dt@kernel.org \
    --cc=wsd_upstream@mediatek.com \
    --cc=youlin.pei@mediatek.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-mediatek Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mediatek/0 linux-mediatek/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mediatek linux-mediatek/ https://lore.kernel.org/linux-mediatek \
		linux-mediatek@lists.infradead.org
	public-inbox-index linux-mediatek

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-mediatek


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git