diff mbox series

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

Message ID 20230208053643.28249-4-yong.wu@mediatek.com
State New
Headers
Series Adjust the dma-ranges for MTK IOMMU |

Commit Message

Yong Wu Feb. 8, 2023, 5:36 a.m. UTC 
  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(-)

Comments

AngeloGioacchino Del Regno Feb. 9, 2023, 1:39 p.m. UTC | #1 
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`.


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.

Regards,
Angelo
Yong Wu Feb. 10, 2023, 6:13 a.m. UTC | #2 
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
> 
>
diff mbox series

Patch

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];
 
 	char			*pericfg_comp_str;
 	struct list_head	*hw_list;
@@ -508,30 +508,33 @@  static unsigned int mtk_iommu_get_bank_id(struct device *dev,
 static int mtk_iommu_get_iova_region_id(struct device *dev,
 					const struct mtk_iommu_plat_data *plat_data)
 {
-	const struct mtk_iommu_iova_region *rgn = plat_data->iova_region;
-	const struct bus_dma_region *dma_rgn = dev->dma_range_map;
-	int i, candidate = -1;
-	dma_addr_t dma_end;
+	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
+	unsigned int portidmsk = 0, larbid;
+	const u32 *rgn_larb_msk;
+	int i, region_id = -1;
 
-	if (!dma_rgn || plat_data->iova_region_nr == 1)
+	if (plat_data->iova_region_nr == 1)
 		return 0;
 
-	dma_end = dma_rgn->dma_start + dma_rgn->size - 1;
-	for (i = 0; i < plat_data->iova_region_nr; i++, rgn++) {
-		/* Best fit. */
-		if (dma_rgn->dma_start == rgn->iova_base &&
-		    dma_end == rgn->iova_base + rgn->size - 1)
-			return i;
-		/* ok if it is inside this region. */
-		if (dma_rgn->dma_start >= rgn->iova_base &&
-		    dma_end < rgn->iova_base + rgn->size)
-			candidate = i;
+	larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
+	for (i = 0; i < fwspec->num_ids; i++)
+		portidmsk |= BIT(MTK_M4U_TO_PORT(fwspec->ids[i]));
+
+	for (i = 0; i < plat_data->iova_region_nr; i++) {
+		rgn_larb_msk = plat_data->larb_region_msk[i];
+		if (!rgn_larb_msk)
+			continue;
+
+		if  ((rgn_larb_msk[larbid] & portidmsk) == portidmsk) {
+			region_id = i;
+			break;
+		}
 	}
 
-	if (candidate >= 0)
-		return candidate;
-	dev_err(dev, "Can NOT find the iommu domain id(%pad 0x%llx).\n",
-		&dma_rgn->dma_start, dma_rgn->size);
+	if (region_id >= 0)
+		return region_id;
+	dev_err(dev, "Can NOT find the region for larb(%d-%x).\n",
+		larbid, portidmsk);
 	return -EINVAL;
 }