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