linux-kernel - Re: [PATCH 2/2] iommu: mtk_iommu: Add support for MT6795 Helio X10 M4Us

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <8a3079d2-1a2f-43ff-9072-91083a025c7b@gmail.com>
Date:   Tue, 17 May 2022 11:37:34 +0200
From:   Matthias Brugger <matthias.bgg@...il.com>
To:     AngeloGioacchino Del Regno 
        <angelogioacchino.delregno@...labora.com>,
        Yong Wu <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,
        konrad.dybcio@...ainline.org, marijn.suijten@...ainline.org,
        martin.botka@...ainline.org, ~postmarketos/upstreaming@...ts.sr.ht,
        phone-devel@...r.kernel.org, paul.bouchara@...ainline.org,
        yf.wang@...iatek.com, mingyuan.ma@...iatek.com
Subject: Re: [PATCH 2/2] iommu: mtk_iommu: Add support for MT6795 Helio X10
 M4Us



On 17/05/2022 11:26, AngeloGioacchino Del Regno wrote:
> Il 17/05/22 11:08, Yong Wu ha scritto:
>> On Fri, 2022-05-13 at 17:14 +0200, AngeloGioacchino Del Regno wrote:
>>> Add support for the M4Us found in the MT6795 Helio X10 SoC.
>>>
>>> Signed-off-by: AngeloGioacchino Del Regno <
>>> angelogioacchino.delregno@...labora.com>
>>> ---
>>>   drivers/iommu/mtk_iommu.c | 20 +++++++++++++++++++-
>>>   1 file changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
>>> index 71b2ace74cd6..3d802dd3f377 100644
>>> --- a/drivers/iommu/mtk_iommu.c
>>> +++ b/drivers/iommu/mtk_iommu.c
>>> @@ -157,6 +157,7 @@
>>>   enum mtk_iommu_plat {
>>>       M4U_MT2712,
>>>       M4U_MT6779,
>>> +    M4U_MT6795,
>>>       M4U_MT8167,
>>>       M4U_MT8173,
>>>       M4U_MT8183,
>>> @@ -953,7 +954,8 @@ static int mtk_iommu_hw_init(const struct
>>> mtk_iommu_data *data, unsigned int ban
>>>        * Global control settings are in bank0. May re-init these
>>> global registers
>>>        * since no sure if there is bank0 consumers.
>>>        */
>>> -    if (data->plat_data->m4u_plat == M4U_MT8173) {
>>> +    if (data->plat_data->m4u_plat == M4U_MT6795 ||
>>> +        data->plat_data->m4u_plat == M4U_MT8173) {
>>>           regval = F_MMU_PREFETCH_RT_REPLACE_MOD |
>>>                F_MMU_TF_PROT_TO_PROGRAM_ADDR_MT8173;
>>>       } else {
>>> @@ -1138,6 +1140,9 @@ static int mtk_iommu_probe(struct
>>> platform_device *pdev)
>>>           case M4U_MT2712:
>>>               p = "mediatek,mt2712-infracfg";
>>>               break;
>>> +        case M4U_MT6795:
>>> +            p = "mediatek,mt6795-infracfg";
>>> +            break;
>>>           case M4U_MT8173:
>>>               p = "mediatek,mt8173-infracfg";
>>>               break;
>>> @@ -1404,6 +1409,18 @@ static const struct mtk_iommu_plat_data
>>> mt6779_data = {
>>>       .larbid_remap  = {{0}, {1}, {2}, {3}, {5}, {7, 8}, {10}, {9}},
>>>   };
>>> +static const struct mtk_iommu_plat_data mt6795_data = {
>>> +    .m4u_plat     = M4U_MT6795,
>>> +    .flags          = HAS_4GB_MODE | HAS_BCLK | RESET_AXI |
>>> +            HAS_LEGACY_IVRP_PADDR | MTK_IOMMU_TYPE_MM,
>>> +    .inv_sel_reg  = REG_MMU_INV_SEL_GEN1,
>>> +    .banks_num    = 1,
>>> +    .banks_enable = {true},
>>> +    .iova_region  = single_domain,
>>> +    .iova_region_nr = ARRAY_SIZE(single_domain),
>>> +    .larbid_remap = {{0}, {1}, {2}, {3}, {4}}, /* Linear mapping.
>>> */
>>> +};
>>
>> This is nearly same with mt8173_data. mt8173 has one more larb than
>> mt6795, its larbid_remap is also ok for mt6795.
>>
> 
> I think that we should be explicit about the larbid_remap property,
> since mt6795 has one less larb, we should explicitly say that like
> I did there... that's only for human readability I admit ... but,
> still, I wouldn't want to see people thinking that MT6795 has 6 LARBs
> because they've read that larbid_remap having 6 entries.
> 
>> thus it looks we could use mt8173 as the backward compatible.
>>      compatible = "mediatek,mt6795-m4u",
>>                   "mediatek,mt8173-m4u";
>>
>> After this, the only thing is about "mediatek,mt6795-infracfg". we have
>> to try again with mediatek,mt6795-infracfg after mediatek,mt8173-
>> infracfg fail. I think we should allow the backward case in 4GB mode
>> judgment if we have.
>>
>> What's your opinion? or some other suggestion?
>> Thanks.
> 
> I know, I may have a plan for that, but I wanted to have a good reason to
> propose such a thing, as if it's just about two SoCs needing that, there
> would be no good reason to get things done differently.
> 
> ...so, in order to provide a good cleanup, we have two possible roads to
> follow here: either we add a generic "mediatek,infracfg" compatible to the
> infra node (but I don't like that), or we can do it like it was previously
> done in mtk-pm-domains.c (I prefer that approach):
> 
> iommu: iommu@...ewhere {
>      ... something ...
>      mediatek,infracfg = <&infracfg>;
> };
> 
> infracfg = syscon_regmap_lookup_by_compatible(node, "mediatek,infracfg");
> if (IS_ERR(infracfg)) {
>      /* try with the older way */
>      switch (...) {
>      case .... p = "mediatek,mt2712-infracfg";
>      ... blah blah ...
>      }
>      /* legacy also failed, ouch! */
>      if (IS_ERR(infracfg))
>          return PTR_ERR(infracfg);
> }
> 
> ret = regmap_read ... etc etc etc
> 

I prefer that approach as well.

Regards,
Matthias

> Cheers,
> Angelo