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