Messages in this thread | | | Subject | Re: [PATCH v3 09/33] iommu/mediatek: Remove for_each_m4u in tlb_sync_all | From | AngeloGioacchino Del Regno <> | Date | Mon, 10 Jan 2022 12:40:18 +0100 |
| |
Il 10/01/22 11:59, Yong Wu ha scritto: > On Mon, 2022-01-10 at 10:16 +0100, AngeloGioacchino Del Regno wrote: >> Il 09/01/22 03:48, Yong Wu ha scritto: >>> On Tue, 2022-01-04 at 16:55 +0100, AngeloGioacchino Del Regno >>> wrote: >>>> Il 23/09/21 13:58, Yong Wu ha scritto: >>>>> The tlb_sync_all is called from these three functions: >>>>> a) flush_iotlb_all: it will be called for each a iommu HW. >>>>> b) tlb_flush_range_sync: it already has for_each_m4u. >>>>> c) in irq: When IOMMU HW translation fault, Only need flush >>>>> itself. >>>>> >>>>> Thus, No need for_each_m4u in this tlb_sync_all. Remove it. >>>>> >>>>> Signed-off-by: Yong Wu <yong.wu@mediatek.com> >>>>> --- >>>>> drivers/iommu/mtk_iommu.c | 18 +++++++----------- >>>>> 1 file changed, 7 insertions(+), 11 deletions(-) >>>>> >>>>> diff --git a/drivers/iommu/mtk_iommu.c >>>>> b/drivers/iommu/mtk_iommu.c >>>>> index 6f4f6624e3ac..0b4c30baa864 100644 >>>>> --- a/drivers/iommu/mtk_iommu.c >>>>> +++ b/drivers/iommu/mtk_iommu.c >>>>> @@ -206,19 +206,15 @@ static struct mtk_iommu_domain >>>>> *to_mtk_domain(struct iommu_domain *dom) >>>>> >>>>> static void mtk_iommu_tlb_flush_all(struct mtk_iommu_data >>>>> *data) >>>>> { >>>>> - struct list_head *head = data->hw_list; >>>>> - >>>>> - for_each_m4u(data, head) { >>>>> - if (pm_runtime_get_if_in_use(data->dev) <= 0) >>>>> - continue; >>>>> + if (pm_runtime_get_if_in_use(data->dev) <= 0) >>>>> + return; >>>>> >>>>> - writel_relaxed(F_INVLD_EN1 | F_INVLD_EN0, >>>>> - data->base + data->plat_data- >>>>>> inv_sel_reg); >>>>> >>>>> - writel_relaxed(F_ALL_INVLD, data->base + >>>>> REG_MMU_INVALIDATE); >>>>> - wmb(); /* Make sure the tlb flush all done */ >>>>> + writel_relaxed(F_INVLD_EN1 | F_INVLD_EN0, >>>>> + data->base + data->plat_data- >>>>>> inv_sel_reg); >>>>> + writel_relaxed(F_ALL_INVLD, data->base + >>>>> REG_MMU_INVALIDATE); >>>>> + wmb(); /* Make sure the tlb flush all done */ >>>> >>>> There aren't a lot of writes here - not anymore, since you are no >>>> longer doing >>>> this for_each_m4u()... >>>> ...so, please change writel_relaxed() to writel() calls, allowing >>>> you >>>> to also >>>> remove the write barrier at the end (since in the non relaxed >>>> version, order is already ensured). >>> >>> In the "writel", the "__iowmb()" runs before "write_relaxed". Then >>> how >>> to guarantee the last register was wrote into the HW. Here the >>> flush >>> all don't have sync(waiting it complete) >>> >> >> That's right, I'm sorry for the invalid proposal. >> >> Though, there's something else to mention here... if writing >> (F_INVLD_EN1 | F_INVLD_EN0) to inv_sel_reg is *required* to happen >> before >> writing F_ALL_INVLD to REG_MMU_INVALIDATE (which I think is exactly >> the >> case here), then, in order to ensure write ordering, you should still >> use >> writel() instead of the relaxed accessor; after which, since (as you >> mentioned) >> there is no sync readback loop, you can keep that wmb() at the end. > > The writel_relaxed also makes sure the order. I did try this: > > > https://patchwork.kernel.org/project/linux-mediatek/patch/1570627143-29441-3-git-send-email-yong.wu@mediatek.com/ >
Ok, that's fair. Means that this patch is fine as it is. I'll release by R-b on Dafna's patch, as suggested.
Thank you, - Angelo
| |