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

[Matthias Brugger <matthias.bgg@gmail.com>]

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@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
> 

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
>