Re: [PATCH v2 03/10] iommu/mediatek: Get regionid from larb/port id

From: "Yong Wu (吴勇)" <Yong.Wu@mediatek.com>
To: "joro@8bytes.org" <joro@8bytes.org>,
	"matthias.bgg@gmail.com" <matthias.bgg@gmail.com>,
	"mchehab@kernel.org" <mchehab@kernel.org>,
	"angelogioacchino.delregno@collabora.com" 
	<angelogioacchino.delregno@collabora.com>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>
Cc: "linux-mediatek@lists.infradead.org"
	<linux-mediatek@lists.infradead.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"Youlin Pei (裴友林)" <youlin.pei@mediatek.com>,
	"linux-media@vger.kernel.org" <linux-media@vger.kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"Anan Sun (孙安安)" <Anan.Sun@mediatek.com>,
	"Libo Kang (康利波)" <Libo.Kang@mediatek.com>,
	"kyrie.wu@mediatek.corp-partner.google.com"
	<kyrie.wu@mediatek.corp-partner.google.com>,
	"robin.murphy@arm.com" <robin.murphy@arm.com>,
	"hverkuil@xs4all.nl" <hverkuil@xs4all.nl>,
	"Chengci Xu (许承赐)" <Chengci.Xu@mediatek.com>,
	"Yunfei Dong (董云飞)" <Yunfei.Dong@mediatek.com>,
	"YF Wang (王云飞)" <YF.Wang@mediatek.com>,
	"linux-arm-kernel@lists.infradead.org"
	<linux-arm-kernel@lists.infradead.org>,
	"krzysztof.kozlowski+dt@linaro.org"
	<krzysztof.kozlowski+dt@linaro.org>,
	"iommu@lists.linux.dev" <iommu@lists.linux.dev>,
	"Mingyuan Ma (马鸣远)" <Mingyuan.Ma@mediatek.com>,
	"will@kernel.org" <will@kernel.org>,
	"nfraprado@collabora.com" <nfraprado@collabora.com>
Subject: Re: [PATCH v2 03/10] iommu/mediatek: Get regionid from larb/port id
Date: Fri, 10 Feb 2023 06:13:17 +0000	[thread overview]
Message-ID: <538959e38bbb694d4e0bae7078fa34cd3d23c65b.camel@mediatek.com> (raw)
In-Reply-To: <cab40506-8b3a-0b53-b45d-a8000e953289@collabora.com>

On Thu, 2023-02-09 at 14:39 +0100, AngeloGioacchino Del Regno wrote:
> Il 08/02/23 06:36, Yong Wu ha scritto:
> > After commit f1ad5338a4d5 ("of: Fix "dma-ranges" handling for bus
> > controllers"), the dma-ranges is not allowed for dts leaf node.
> > but we still would like to separate to different masters
> > into different iova regions.
> > 
> > Thus we have to separate it by the HW larbid and portid. For
> > example,
> > larb1/2 are in region2 and larb3 is in region3. The problem is that
> > some ports inside a larb are in region4 while some ports inside
> > this
> > larb are in region5. Therefore I define a "larb_region_msk" to help
> > record the information for each a port. Take a example for a larb:
> >   [1] = ~0: means all ports in this larb are in region1;
> >   [2] = BIT(3) | BIT(4): means port3/4 in this larb are region2;
> >   [3] = ~(BIT(3) | BIT(4)): means all the other ports except
> > port3/4
> >                             in this larb are region3.
> > 
> > This method also avoids the users forget/abuse the iova regions.
> > 
> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > ---
> >   drivers/iommu/mtk_iommu.c | 43 +++++++++++++++++++++-------------
> > -----
> >   1 file changed, 23 insertions(+), 20 deletions(-)
> > 
> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > index d5a4955910ff..fc3d9be120a0 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> > @@ -8,7 +8,6 @@
> >   #include <linux/clk.h>
> >   #include <linux/component.h>
> >   #include <linux/device.h>
> > -#include <linux/dma-direct.h>
> >   #include <linux/err.h>
> >   #include <linux/interrupt.h>
> >   #include <linux/io.h>
> > @@ -194,6 +193,7 @@ struct mtk_iommu_plat_data {
> >   	enum mtk_iommu_plat	m4u_plat;
> >   	u32			flags;
> >   	u32			inv_sel_reg;
> > +	const u32		(*larb_region_msk)[32];
> 
> Can you please document this larb region mask in code, other than the
> commit
> description?
> 
> I can see this being essential for the next person reading this
> driver's code
> without digging through the commit history. At least some comment on
> top of
> the pointer, or on top of the struct declaration... and perhaps also
> describe
> briefly that the array is "indexed by region" (so 1 = region 1; 2 =
> region 2)
> and that the region index corresponds to the same index as
> `mtk_iommu_iova_region`.

Thanks for this suggestion. I will comment these in the code in next
version.

> 
> Before doing that, I'd like to check if anyone else has a better
> solution for
> that... because when looking at data for one of the SoCs in here, it
> looks a bit intimidating!
> 
> Copy-paste from patch [04/10] of this series for the reader's
> commodity:
> 
> static const unsigned int mt8195_larb_region_msk[][32] = {
> 	[0] = {~0, ~0, ~0, ~0},               /* Region0: all ports for
> larb0/1/2/3 */
> 	[1] = {0, 0, 0, 0, 0, 0, 0, 0,
> 	       0, 0, 0, 0, 0, 0, 0, 0,
> 	       0, 0, 0, ~0, ~0, ~0, ~0, ~0,   /* Region1:
> larb19/20/21/22/23/24 */
> 	       ~0},
> 	[2] = {0, 0, 0, 0, ~0, ~0, ~0, ~0,    /* Region2: the other
> larbs. */
> 	       ~0, ~0, ~0, ~0, ~0, ~0, ~0, ~0,
> 	       ~0, ~0, 0, 0, 0, 0, 0, 0,
> 	       0, ~0, ~0, ~0, ~0},
> 	[3] = {0},
> 	[4] = {[18] = BIT(0) | BIT(1)},       /* Only larb18 port0/1 */
> 	[5] = {[18] = BIT(2) | BIT(3)},       /* Only larb18 port2/3 */
> };
> 
> ^^^^ That's what I actually mean by "intimidating"... :-P
> 
> It's just looks though, there's nothing much complicated here.

Thanks.

> 
> Regards,
> Angelo
> 
> 