[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <3c2ac364-5eaf-b26f-2f93-d4cd93b53828@gmail.com>
Date: Fri, 17 Jun 2022 12:32:42 +0200
From: Matthias Brugger <matthias.bgg@...il.com>
To: AngeloGioacchino Del Regno
<angelogioacchino.delregno@...labora.com>, yong.wu@...iatek.com
Cc: joro@...tes.org, will@...nel.org, robh+dt@...nel.org,
krzysztof.kozlowski+dt@...aro.org,
iommu@...ts.linux-foundation.org,
linux-mediatek@...ts.infradead.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
krzysztof.kozlowski@...aro.org
Subject: Re: [PATCH v3 6/6] iommu: mtk_iommu: Lookup phandle to retrieve
syscon to pericfg
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@...labora.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
>
Powered by blists - more mailing lists