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

[Yong Wu (吴勇)]

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@xxxxxxxxxxxx>
> > ---
> >   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
> 
>