Messages in this thread | | | Date | Fri, 17 Jun 2022 12:32:42 +0200 | Subject | Re: [PATCH v3 6/6] iommu: mtk_iommu: Lookup phandle to retrieve syscon to pericfg | From | 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@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 >
| |