Re: [PATCH v4 1/4] dt-bindings: mediatek: mt8188: Add binding for MM & INFRA IOMMU

Re: [PATCH v4 1/4] dt-bindings: mediatek: mt8188: Add binding for MM & INFRA IOMMU

[Yong Wu (吴勇)]

On Tue, 2022-10-18 at 14:42 +0800, Chengci.Xu wrote:
> Adds descriptions for mt8188 IOMMU which also use ARM Short-
> Descriptor
> translation table format.
> 
> In mt8188, there are two smi-common HW and IOMMU, one is for
> vdo(video
> output), the other is for vpp(video processing pipe). They connects
> with different smi-larbs, then some setting(larbid_remap) is
> different.
> Differentiate them with the compatible string.
> 
> Something like this:
> 
>   IOMMU(VDO)          IOMMU(VPP)
>       |                   |
> SMI_COMMON_VDO      SMI_COMMON_VPP
> ---------------     ----------------
>   |     |    ...      |     |    ...
> larb0 larb2  ...    larb1 larb3  ...
> 
> We also have an IOMMU that is for infra master like PCIe.
> And infra master don't have the larb and ports.
> 
> Signed-off-by: Chengci.Xu <chengci.xu@mediatek.com>
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>
> ---
>  .../bindings/iommu/mediatek,iommu.yaml        |  12 +-
>  .../memory/mediatek,mt8188-memory-port.h      | 482
> ++++++++++++++++++

Reviewed-by: Yong Wu <yong.wu@mediatek.com>

Re: [PATCH v4 4/4] iommu/mediatek: Add MT8188 IOMMU Support

[Yong Wu (吴勇)]

On Tue, 2022-10-18 at 14:42 +0800, Chengci.Xu wrote:
> MT8188 has 3 IOMMU, containing 2 MM IOMMUs, one is for vdo, the other
> is for vpp. and 1 INFRA IOMMU.
> 
> Signed-off-by: Chengci.Xu <chengci.xu@mediatek.com>
> Reviewed-by: AngeloGioacchino Del Regno <
> angelogioacchino.delregno@collabora.com>
> ---
>  drivers/iommu/mtk_iommu.c | 47
> +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index cb62cb89a2f4..3de8bb5d8404 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -166,6 +166,7 @@ enum mtk_iommu_plat {
>  	M4U_MT8173,
>  	M4U_MT8183,
>  	M4U_MT8186,
> +	M4U_MT8188,
>  	M4U_MT8192,
>  	M4U_MT8195,
>  };
> @@ -1475,6 +1476,49 @@ static const struct mtk_iommu_plat_data
> mt8186_data_mm = {
>  	.iova_region_nr = ARRAY_SIZE(mt8192_multi_dom),
>  };
>  
> +static const struct mtk_iommu_plat_data mt8188_data_infra = {
> +	.m4u_plat	  = M4U_MT8188,
> +	.flags            = WR_THROT_EN | DCM_DISABLE | STD_AXI_MODE |
> PM_CLK_AO |
> +			    MTK_IOMMU_TYPE_INFRA |
> IFA_IOMMU_PCIE_SUPPORT |
> +			    CFG_IFA_MASTER_IN_ATF,
> +	.inv_sel_reg      = REG_MMU_INV_SEL_GEN2,
> +	.banks_num	  = 1,
> +	.banks_enable     = {true},
> +	.iova_region      = single_domain,
> +	.iova_region_nr   = ARRAY_SIZE(single_domain),
> +};
> +
> +static const struct mtk_iommu_plat_data mt8188_data_vdo = {
> +	.m4u_plat	= M4U_MT8188,
> +	.flags          = HAS_BCLK | HAS_SUB_COMM_3BITS |
> OUT_ORDER_WR_EN |
> +			  WR_THROT_EN | IOVA_34_EN | SHARE_PGTABLE |
> MTK_IOMMU_TYPE_MM,
> +	.hw_list        = &m4ulist,
> +	.inv_sel_reg    = REG_MMU_INV_SEL_GEN2,
> +	.banks_num      = 1,
> +	.banks_enable   = {true},
> +	.iova_region	= mt8192_multi_dom,
> +	.iova_region_nr	= ARRAY_SIZE(mt8192_multi_dom),
> +	.larbid_remap   = {{2}, {0}, {21}, {0}, {19}, {9, 10,
> +			   11 /* 11a */, 25 /* 11c */},
> +			   {13, 0, 29 /* 16b */, 30 /* 17b */, 0},
> {5}},
> +};
> +
> +static const struct mtk_iommu_plat_data mt8188_data_vpp = {
> +	.m4u_plat	= M4U_MT8188,
> +	.flags          = HAS_BCLK | HAS_SUB_COMM_3BITS |
> OUT_ORDER_WR_EN |
> +			  WR_THROT_EN | IOVA_34_EN | SHARE_PGTABLE |
> MTK_IOMMU_TYPE_MM,
> +	.hw_list        = &m4ulist,
> +	.inv_sel_reg    = REG_MMU_INV_SEL_GEN2,
> +	.banks_num      = 1,
> +	.banks_enable   = {true},
> +	.iova_region	= mt8192_multi_dom,
> +	.iova_region_nr	= ARRAY_SIZE(mt8192_multi_dom),
> +	.larbid_remap   = {{1}, {3}, {23}, {7}, {MTK_INVALID_LARBID},
> +			   {12, 15, 24 /* 11b */}, {14,
> MTK_INVALID_LARBID,
> +			   16 /* 16a */, 17 /* 17a */,
> MTK_INVALID_LARBID,
> +			   27, 28 /* ccu0 */, MTK_INVALID_LARBID}, {4,
> 6}},
> +};
> +

Add PGTABLE_PA_35_EN for them. Then,

Reviewed-by: Yong Wu <yong.wu@mediatek.com>

>  static const struct mtk_iommu_plat_data mt8192_data = {
>  	.m4u_plat       = M4U_MT8192,
>  	.flags          = HAS_BCLK | HAS_SUB_COMM_2BITS |
> OUT_ORDER_WR_EN |
> @@ -1543,6 +1587,9 @@ static const struct of_device_id
> mtk_iommu_of_ids[] = {
>  	{ .compatible = "mediatek,mt8173-m4u", .data = &mt8173_data},
>  	{ .compatible = "mediatek,mt8183-m4u", .data = &mt8183_data},
>  	{ .compatible = "mediatek,mt8186-iommu-mm",    .data =
> &mt8186_data_mm}, /* mm: m4u */
> +	{ .compatible = "mediatek,mt8188-iommu-infra", .data =
> &mt8188_data_infra},
> +	{ .compatible = "mediatek,mt8188-iommu-vdo",   .data =
> &mt8188_data_vdo},
> +	{ .compatible = "mediatek,mt8188-iommu-vpp",   .data =
> &mt8188_data_vpp},
>  	{ .compatible = "mediatek,mt8192-m4u", .data = &mt8192_data},
>  	{ .compatible = "mediatek,mt8195-iommu-infra", .data =
> &mt8195_data_infra},
>  	{ .compatible = "mediatek,mt8195-iommu-vdo",   .data =
> &mt8195_data_vdo},

Re: [PATCH v4 3/4] iommu/mediatek: Add enable IOMMU SMC command for INFRA master

[Yong Wu (吴勇)]

On Tue, 2022-10-18 at 14:42 +0800, Chengci.Xu wrote:
> The register which can enable IOMMU for INFRA master should be setted
> in secure world for security concerns. Therefore, we add a SMC
> command
> for INFRA master to enable/disable INFRA IOMMU in ATF. This function
> is
> prepared for MT8188.
> 
> Signed-off-by: Chengci.Xu <chengci.xu@mediatek.com>

Reviewed-by: Yong Wu <yong.wu@mediatek.com>

Re: [PATCH v4 2/4] iommu/mediatek: Adjust mtk_iommu_config flow

[Yong Wu (吴勇)]

On Tue, 2022-10-18 at 14:42 +0800, Chengci.Xu wrote:
> For reduce indention without functional change, prepare for MT8188.
> If there are many port in a same larb, current flow will update
> larb_mmu->mmu or update INFRA register for too many times.
> So we save all port to portid_msk in the front of mtk_iommu_config(),
> and then update only once for IOMMU configure. By this modification,
> we can prevent MT8188 from sending to many SMC calls, avoiding enter
> ATF for each port.
> 
> Signed-off-by: Chengci.Xu <chengci.xu@mediatek.com>
> ---
>  drivers/iommu/mtk_iommu.c | 60 ++++++++++++++++++++++---------------
> --
>  1 file changed, 34 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 5a4e00e4bbbc..fbaf401f34e0 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -534,41 +534,49 @@ static int mtk_iommu_config(struct
> mtk_iommu_data *data, struct device *dev,
>  	unsigned int                 larbid, portid;
>  	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>  	const struct mtk_iommu_iova_region *region;
> -	u32 peri_mmuen, peri_mmuen_msk;
> +	unsigned long portid_msk_ext;
> +	u32 portid_msk = 0;
>  	int i, ret = 0;
>  
>  	for (i = 0; i < fwspec->num_ids; ++i) {
> -		larbid = MTK_M4U_TO_LARB(fwspec->ids[i]);
>  		portid = MTK_M4U_TO_PORT(fwspec->ids[i]);
> +		portid_msk |= BIT(portid);
> +	}
>  
> -		if (MTK_IOMMU_IS_TYPE(data->plat_data,
> MTK_IOMMU_TYPE_MM)) {
> -			larb_mmu = &data->larb_imu[larbid];
> +	if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_MM)) {
> +		/* All ports should be in the same larb. just use 0
> here */
> +		larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
> +		larb_mmu = &data->larb_imu[larbid];
> +		region = data->plat_data->iova_region + regionid;
>  
> -			region = data->plat_data->iova_region +
> regionid;
> +		portid_msk_ext = portid_msk;
> +		for_each_set_bit(portid, &portid_msk_ext, 32)

Why do we need define a new portid_msk_ext? Can't we use portid_msk
directly?

>  			larb_mmu->bank[portid] = upper_32_bits(region-
> >iova_base);
>  
> -			dev_dbg(dev, "%s iommu for larb(%s) port %d
> region %d rgn-bank %d.\n",
> -				enable ? "enable" : "disable",
> dev_name(larb_mmu->dev),
> -				portid, regionid, larb_mmu-
> >bank[portid]);
> -
> -			if (enable)
> -				larb_mmu->mmu |=
> MTK_SMI_MMU_EN(portid);
> -			else
> -				larb_mmu->mmu &=
> ~MTK_SMI_MMU_EN(portid);
> -		} else if (MTK_IOMMU_IS_TYPE(data->plat_data,
> MTK_IOMMU_TYPE_INFRA)) {
> -			peri_mmuen_msk = BIT(portid);
> -			/* PCI dev has only one output id, enable the
> next writing bit for PCIe */
> -			if (dev_is_pci(dev))
> -				peri_mmuen_msk |= BIT(portid + 1);
> -
> -			peri_mmuen = enable ? peri_mmuen_msk : 0;
> -			ret = regmap_update_bits(data->pericfg,
> PERICFG_IOMMU_1,
> -						 peri_mmuen_msk,
> peri_mmuen);
> -			if (ret)
> -				dev_err(dev, "%s iommu(%s) inframaster
> 0x%x fail(%d).\n",
> -					enable ? "enable" : "disable",
> -					dev_name(data->dev),
> peri_mmuen_msk, ret);
> +		dev_dbg(dev, "%s iommu for larb(%s) port 0x%x region %d
> rgn-bank %d.\n",
> +			enable ? "enable" : "disable",
> dev_name(larb_mmu->dev),
> +			portid_msk, regionid, upper_32_bits(region-
> >iova_base));
> +
> +		if (enable)
> +			larb_mmu->mmu |= portid_msk;
> +		else
> +			larb_mmu->mmu &= ~portid_msk;
> +	} else if (MTK_IOMMU_IS_TYPE(data->plat_data,
> MTK_IOMMU_TYPE_INFRA)) {
> +		/* PCI dev has only one output id, enable the next
> writing bit for PCIe */
> +		if (dev_is_pci(dev)) {
> +			if (fwspec->num_ids != 1) {
> +				dev_err(dev, "PCI dev can only have one
> port.\n");
> +				return -ENODEV;
> +			}
> +			portid_msk |= BIT(portid + 1);
>  		}
> +
> +		ret = regmap_update_bits(data->pericfg,
> PERICFG_IOMMU_1,
> +					 portid_msk, enable ?
> portid_msk : 0);
> +		if (ret)
> +			dev_err(dev, "%s iommu(%s) inframaster 0x%x
> fail(%d).\n",
> +				enable ? "enable" : "disable",
> +				dev_name(data->dev), portid_msk, ret);
>  	}
>  	return ret;
>  }

Re: [PATCH v4 2/4] iommu/mediatek: Adjust mtk_iommu_config flow

[Chengci Xu (许承赐)]

On Mon, 2022-11-21 at 05:17 +0000, Yong Wu (吴勇) wrote:
> On Tue, 2022-10-18 at 14:42 +0800, Chengci.Xu wrote:
> > For reduce indention without functional change, prepare for MT8188.
> > If there are many port in a same larb, current flow will update
> > larb_mmu->mmu or update INFRA register for too many times.
> > So we save all port to portid_msk in the front of
> > mtk_iommu_config(),
> > and then update only once for IOMMU configure. By this
> > modification,
> > we can prevent MT8188 from sending to many SMC calls, avoiding
> > enter
> > ATF for each port.
> > 
> > Signed-off-by: Chengci.Xu <chengci.xu@mediatek.com>
> > ---
> >  drivers/iommu/mtk_iommu.c | 60 ++++++++++++++++++++++-------------
> > --
> > --
> >  1 file changed, 34 insertions(+), 26 deletions(-)
> > 
> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > index 5a4e00e4bbbc..fbaf401f34e0 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> > @@ -534,41 +534,49 @@ static int mtk_iommu_config(struct
> > mtk_iommu_data *data, struct device *dev,
> >  	unsigned int                 larbid, portid;
> >  	struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> >  	const struct mtk_iommu_iova_region *region;
> > -	u32 peri_mmuen, peri_mmuen_msk;
> > +	unsigned long portid_msk_ext;
> > +	u32 portid_msk = 0;
> >  	int i, ret = 0;
> >  
> >  	for (i = 0; i < fwspec->num_ids; ++i) {
> > -		larbid = MTK_M4U_TO_LARB(fwspec->ids[i]);
> >  		portid = MTK_M4U_TO_PORT(fwspec->ids[i]);
> > +		portid_msk |= BIT(portid);
> > +	}
> >  
> > -		if (MTK_IOMMU_IS_TYPE(data->plat_data,
> > MTK_IOMMU_TYPE_MM)) {
> > -			larb_mmu = &data->larb_imu[larbid];
> > +	if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_MM)) {
> > +		/* All ports should be in the same larb. just use 0
> > here */
> > +		larbid = MTK_M4U_TO_LARB(fwspec->ids[0]);
> > +		larb_mmu = &data->larb_imu[larbid];
> > +		region = data->plat_data->iova_region + regionid;
> >  
> > -			region = data->plat_data->iova_region +
> > regionid;
> > +		portid_msk_ext = portid_msk;
> > +		for_each_set_bit(portid, &portid_msk_ext, 32)
> 
> Why do we need define a new portid_msk_ext? Can't we use portid_msk
> directly?

Thanks for your review.

The second parameter of for_each_set_bit is an address of "ulong",
which is shown as "const unsigned long *", but portid_msk is "u32".

I have tried following two solutions to get correct address of ulong
from portid_msk:
(1) (unsigned long *)&portid_msk
	If we get the address of portid_msk by "&" and cast it to
"unsigned long *", "build error will happened. The fail reason we can
find in build_allmodconfig.arm64.log is"
/tmp/src_kernel/kernel/linux-next/drivers/iommu/mtk_iommu.c: In
function 'mtk_iommu_config': /tmp/src_kernel/kernel/linux-
next/include/linux/find.h:58:23: error: array subscript 'long unsigned
int[0]' is partly outside array bounds of 'u32[1]' {aka 'unsigned
int[1]'} [-Werror=array-bounds]
   58 |                 val = *addr & GENMASK(size - 1, offset);
(2) &((unsigned long)portid_msk)
	This is not allowed beacuse "(unsigned long)portid_msk" is a
right value and geting the address of a right value is illegal.

So I choose to define a new variable "portid_msk_ext" whose type is
"unsigned long". I know it is a ugly soultion just to make function ok
and build pass, but it's hard for me to catch up with other soultions.

May be we can change the type of portid_msk from "u32" to "u64", is
this OK for you?