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 <[email protected]>
> ---
> 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;
> }
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 <[email protected]>
> > ---
> > 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?