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

[AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>]

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@collabora.com>
>> ---
>>   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

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.

After all, it's just one single SoC.

I'll send a new version soon!

Cheers,
Angelo