Re: [PATCH v3 6/6] iommu: mtk_iommu: Lookup phandle to retrieve syscon to pericfg

[Matthias Brugger]

On 15/06/2022 14:28, AngeloGioacchino Del Regno wrote:
> Il 15/06/22 14:09, Matthias Brugger ha scritto:
> > On 09/06/2022 12:08, AngeloGioacchino Del Regno wrote:
> > > On some SoCs (of which only MT8195 is supported at the time of writing),
> > > the "R" and "W" (I/O) enable bits for the IOMMUs are in the pericfg_ao
> > > register space and not in the IOMMU space: as it happened already with
> > > infracfg, it is expected that this list will grow.
> > >
> > > Instead of specifying pericfg compatibles on a per-SoC basis, following
> > > what was done with infracfg, let's lookup the syscon by phandle instead.
> > >
> > > Signed-off-by: AngeloGioacchino Del Regno 
> > > <angelogioacchino.delregno@xxxxxxxxxxxxx>
> > > ---
> > >   drivers/iommu/mtk_iommu.c | 23 +++++++++++++----------
> > >   1 file changed, 13 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > > index 90685946fcbe..0ea0848581e9 100644
> > > --- a/drivers/iommu/mtk_iommu.c
> > > +++ b/drivers/iommu/mtk_iommu.c
> > > @@ -138,6 +138,8 @@
> > >   /* PM and clock always on. e.g. infra iommu */
> > >   #define PM_CLK_AO            BIT(15)
> > >   #define IFA_IOMMU_PCIE_SUPPORT        BIT(16)
> > > +/* IOMMU I/O (r/w) is enabled using PERICFG_IOMMU_1 register */
> > > +#define HAS_PERI_IOMMU1_REG        BIT(17)
> >
> >  From what I can see MTK_IOMMU_TYPE_INFRA is only set in MT8195 which uses 
> > pericfg. So we don't need a new flag here. For me the flag name 
> > MTK_IOMMU_TYPE_INFRA was confusing as it has nothing to do with the use of 
> > infracfg. I'll hijack this patch to provide some feedback on the actual code, 
> > please see below.
> >
> > >   #define MTK_IOMMU_HAS_FLAG_MASK(pdata, _x, mask)    \
> > >                   ((((pdata)->flags) & (mask)) == (_x))
> > > @@ -187,7 +189,6 @@ struct mtk_iommu_plat_data {
> > >       u32            flags;
> > >       u32            inv_sel_reg;
> > > -    char            *pericfg_comp_str;
> > >       struct list_head    *hw_list;
> > >       unsigned int        iova_region_nr;
> > >       const struct mtk_iommu_iova_region    *iova_region;
> > > @@ -1218,14 +1219,16 @@ static int mtk_iommu_probe(struct platform_device *pdev)
> > >               goto out_runtime_disable;
> > >           }
> > >       } else if (MTK_IOMMU_IS_TYPE(data->plat_data, MTK_IOMMU_TYPE_INFRA) &&
> > > -           data->plat_data->pericfg_comp_str) {
> >
> > Check for pericfg_comp_str is not needed, we only have one platform that uses 
> > MTK_IOMMU_TYPE_INFRA.
>
> Fair enough. I agree.
>
> > > -        infracfg = 
> > > syscon_regmap_lookup_by_compatible(data->plat_data->pericfg_comp_str);
> >
> > We can do something like this to make the code clearer:
> > data->pericfg = 
> > syscon_regmap_lookup_by_compatible(data->plat_data->pericfg_comp_str);
> >          if (IS_ERR(data->pericfg)) {
> >
> > Using infracfg variable here is confusing as it has nothing to do with 
> > infracfg used with HAS_4GB_MODE flag.
>
> Yes Matthias, using the infracfg variable is confusing - that's why I changed that
> already....
>
> > Regards,
> > Matthias
> >
> > > -        if (IS_ERR(infracfg)) {
> > > -            ret = PTR_ERR(infracfg);
> > > -            goto out_runtime_disable;
> > > +           MTK_IOMMU_HAS_FLAG(data->plat_data, HAS_PERI_IOMMU1_REG)) {
>
>
>
> > > +        data->pericfg = syscon_regmap_lookup_by_phandle(dev->of_node, 
> > > "mediatek,pericfg");
>
> ....Here, where I'm assigning directly to data->pericfg :-P

Uuuups, sorry, did look too much on the existing code and not enough on your patch.

> By the way, since it was only about one platform, my intention was to remove the
> pericfg_comp_str from struct iommu_plat_data (as you can see), but then, with the
> current code, I had to assign .....
>
>
> > > +        if (IS_ERR(data->pericfg)) {
> > > +            p = "mediatek,mt8195-pericfg_ao";
>
> ...the string to 'p', because otherwise it would go over 100 columns.
>
> In any case, I just checked and, apparently, MT8195 is really the one and only SoC
> that needs this pericfg register to be managed by Linux... even the latest and
> greatest smartphone chip (Dimensity 9000, MT6983) doesn't need this (at least,
> from what I can read on a downstream kernel).
>
> On an afterthought, perhaps the best idea is to just leave this as it is and, as
> you proposed, avoid using that confusing infracfg variable, without adding the
> pericfg handle at all.

Either this or get also rid of the pericfg_comp_str in the _plat_data. I'm 
unemotional about this :)

Regards,
Matthias

> After all, it's just one single SoC.
>
> I'll send a new version soon!
>
> Cheers,
> Angelo