Hi Matthias,
Thanks very much for reviewing.
On Thu, 2022-04-28 at 16:14 +0200, Matthias Brugger wrote:
>
> On 07/04/2022 09:57, Yong Wu wrote:
> > We preassign some ports in a special bank via the new defined
> > banks_portmsk. Put it in the plat_data means it is not expected to
> > be
> > adjusted dynamically.
> >
> > If the iommu id in the iommu consumer's dtsi node is inside this
> > banks_portmsk, then we switch it to this special iommu bank, and
> > initialise the IOMMU bank HW.
> >
> > Each a bank has the independent pgtable(4GB iova range). Each a
> > bank
> > is a independent iommu domain/group. Currently we don't separate
> > different
> > iova ranges inside a bank.
> >
> > Signed-off-by: Yong Wu <yong.wu@xxxxxxxxxxxx>
> > Reviewed-by: AngeloGioacchino Del Regno <
> > angelogioacchino.delregno@xxxxxxxxxxxxx>
> > ---
> > drivers/iommu/mtk_iommu.c | 39
> > ++++++++++++++++++++++++++++++++++++---
> > 1 file changed, 36 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > index 0828cff97625..d42b3d35a36e 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
[snip]
> > @@ -802,6 +828,7 @@ static struct iommu_group
> > *mtk_iommu_device_group(struct device *dev)
> > struct mtk_iommu_data *c_data = dev_iommu_priv_get(dev), *data;
> > struct list_head *hw_list = c_data->hw_list;
> > struct iommu_group *group;
> > + unsigned int bankid, groupid;
> > int regionid;
> >
> > data = mtk_iommu_get_frst_data(hw_list);
> > @@ -812,12 +839,18 @@ static struct iommu_group
> > *mtk_iommu_device_group(struct device *dev)
> > if (regionid < 0)
> > return ERR_PTR(regionid);
> >
> > + bankid = mtk_iommu_get_bank_id(dev, data->plat_data);
>
> I think code readability would be improved if we add a new function
> like
> mtk_iommu_get_id which call mtk_iommu_get_bankid and if necessary
> mtk_iommu_get_regionid.
OK, I will define a new function, like mtk_iommu_get_group_id for the
readability.
>
> > mutex_lock(&data->mutex);
> > - group = data->m4u_group[regionid];
> > + /*
> > + * If the bank function is enabled, each a bank is a iommu
> > group/domain.
> > + * otherwise, each a iova region is a iommu group/domain.
>
> While at it:
> "If the bank function is enabled, each bank is a iommu group/domain.
> Otherwise,
> each iova region is a iommu group/domain."
And move this comment into the new funtion.
Also of course, I will fix the other two comments and send v7.
Thanks.
>
> Regards,
> Matthias
>
> > + */
> > + groupid = bankid ? bankid : regionid;
> > + group = data->m4u_group[groupid];
> > if (!group) {
> > group = iommu_group_alloc();
> > if (!IS_ERR(group))
> > - data->m4u_group[regionid] = group;
> > + data->m4u_group[groupid] = group;
> > } else {
> > iommu_group_ref_get(group);
> > }
