| Message ID | 20220125085634.17972-1-yong.wu@mediatek.com |
|---|---|
| Headers | show |
| Series | MT8195 IOMMU SUPPORT | expand |
Il 25/01/22 09:56, Yong Wu ha scritto: > In the commit 4f956c97d26b ("iommu/mediatek: Move domain_finalise into > attach_device"), I overlooked the sharing pgtable case. > After that commit, the "data" in the mtk_iommu_domain_finalise always is > the data of the current IOMMU HW. Fix this for the sharing pgtable case. > > Only affect mt2712 which is the only SoC that share pgtable currently. > > Fixes: 4f956c97d26b ("iommu/mediatek: Move domain_finalise into attach_device") > Signed-off-by: Yong Wu <yong.wu@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 25/01/22 09:56, Yong Wu ha scritto: > Add a mutex to protect the data in the structure mtk_iommu_data, > like ->"m4u_group" ->"m4u_dom". For the internal data, we should > protect it in ourselves driver. Add a mutex for this. > This could be a fix for the multi-groups support. > > Fixes: c3045f39244e ("iommu/mediatek: Support for multi domains") > Signed-off-by: Yunfei Wang <yf.wang@mediatek.com> > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > --- > drivers/iommu/mtk_iommu.c | 13 +++++++++++-- > drivers/iommu/mtk_iommu.h | 2 ++ > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c > index ec2c387abf60..095736bfb7b4 100644 > --- a/drivers/iommu/mtk_iommu.c > +++ b/drivers/iommu/mtk_iommu.c > @@ -464,15 +464,16 @@ static int mtk_iommu_attach_device(struct iommu_domain *domain, > dom->data = data; > } > > + mutex_lock(&data->mutex); > if (!data->m4u_dom) { /* Initialize the M4U HW */ > ret = pm_runtime_resume_and_get(m4udev); > if (ret < 0) > - return ret; > + goto data_unlock; In order to enhance human readability, I would rather propose: goto err_unlock; Apart from this, Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> > > ret = mtk_iommu_hw_init(data); > if (ret) { > pm_runtime_put(m4udev); > - return ret; > + goto data_unlock; > } > data->m4u_dom = dom; > writel(dom->cfg.arm_v7s_cfg.ttbr & MMU_PT_ADDR_MASK, > @@ -480,9 +481,14 @@ static int mtk_iommu_attach_device(struct iommu_domain *domain, > > pm_runtime_put(m4udev); > } > + mutex_unlock(&data->mutex); > > mtk_iommu_config(data, dev, true, domid); > return 0; > + > +data_unlock: > + mutex_unlock(&data->mutex); > + return ret; > } > > static void mtk_iommu_detach_device(struct iommu_domain *domain, > @@ -592,6 +598,7 @@ static struct iommu_group *mtk_iommu_device_group(struct device *dev) > if (domid < 0) > return ERR_PTR(domid); > > + mutex_lock(&data->mutex); > group = data->m4u_group[domid]; > if (!group) { > group = iommu_group_alloc(); > @@ -600,6 +607,7 @@ static struct iommu_group *mtk_iommu_device_group(struct device *dev) > } else { > iommu_group_ref_get(group); > } > + mutex_unlock(&data->mutex); > return group; > } > > @@ -874,6 +882,7 @@ static int mtk_iommu_probe(struct platform_device *pdev) > } > > platform_set_drvdata(pdev, data); > + mutex_init(&data->mutex); > > ret = iommu_device_sysfs_add(&data->iommu, dev, NULL, > "mtk-iommu.%pa", &ioaddr); > diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h > index f81fa8862ed0..f413546ac6e5 100644 > --- a/drivers/iommu/mtk_iommu.h > +++ b/drivers/iommu/mtk_iommu.h > @@ -80,6 +80,8 @@ struct mtk_iommu_data { > > struct dma_iommu_mapping *mapping; /* For mtk_iommu_v1.c */ > > + struct mutex mutex; /* Protect m4u_group/m4u_dom above */ > + > struct list_head list; > struct mtk_smi_larb_iommu larb_imu[MTK_LARB_NR_MAX]; > }; >
Il 25/01/22 09:56, Yong Wu ha scritto: > Same with the previous patch, add a mutex for the "data" in the > mtk_iommu_domain. Just improve the safety for multi devices > enter attach_device at the same time. We don't get the real issue > for this. > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 25/01/22 09:56, Yong Wu ha scritto: > No need zero for the protect buffer that is only accessed by the IOMMU HW > translation fault happened. > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> I would rather keep this a devm_kzalloc instead... the cost is very minimal and this will be handy when new hardware will be introduced, as it may require a bigger buffer: in that case, "older" platforms will use only part of it and we may get garbage data at the end. Regards, Angelo
Il 25/01/22 09:56, Yong Wu ha scritto: > Currently there is only compare_of/release_of/a suspend structure in the > header file. I think it is no need to keep a header file only for these. > Move these into the c file and rm this header file. > > I think there should be a common helper for compare_of and release_of. > There is many copy in drm, it should be another topic. > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > --- > drivers/iommu/mtk_iommu.c | 25 ++++++++++++++++++++- > drivers/iommu/mtk_iommu.h | 42 ------------------------------------ > drivers/iommu/mtk_iommu_v1.c | 21 +++++++++++++++--- > 3 files changed, 42 insertions(+), 46 deletions(-) > delete mode 100644 drivers/iommu/mtk_iommu.h > > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c > index 80c1e5a75868..f88c7bb235bf 100644 > --- a/drivers/iommu/mtk_iommu.c > +++ b/drivers/iommu/mtk_iommu.c > @@ -14,6 +14,7 @@ > #include <linux/io.h> > #include <linux/iommu.h> > #include <linux/iopoll.h> > +#include <linux/io-pgtable.h> > #include <linux/list.h> > #include <linux/mfd/syscon.h> > #include <linux/module.h> > @@ -30,7 +31,7 @@ > #include <asm/barrier.h> > #include <soc/mediatek/smi.h> > > -#include "mtk_iommu.h" > +#include <dt-bindings/memory/mtk-memory-port.h> > > #define REG_MMU_PT_BASE_ADDR 0x000 > #define MMU_PT_ADDR_MASK GENMASK(31, 7) > @@ -166,6 +167,17 @@ struct mtk_iommu_iova_region { > unsigned long long size; > }; > > +struct mtk_iommu_suspend_reg { > + u32 misc_ctrl; > + u32 dcm_dis; > + u32 ctrl_reg; > + u32 int_control0; > + u32 int_main_control; > + u32 ivrp_paddr; > + u32 vld_pa_rng; > + u32 wr_len_ctrl; > +}; > + > struct mtk_iommu_plat_data { > enum mtk_iommu_plat m4u_plat; > u32 flags; > @@ -219,6 +231,17 @@ struct mtk_iommu_domain { > struct mutex mutex; /* Protect "data" in this structure */ > }; > > +/* TODO: A common helper is expected. */ > +static inline int compare_of(struct device *dev, void *data) > +{ > + return dev->of_node == data; > +} > + > +static inline void release_of(struct device *dev, void *data) > +{ > + of_node_put(data); > +} > + Since it's just one line, at this point you should also open-code these, as in you can then remove the two helper functions entirely. So, please do that. > static inline int mtk_iommu_bind(struct device *dev) > { > struct mtk_iommu_data *data = dev_get_drvdata(dev); > diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h > deleted file mode 100644 > index d332f9769f83..000000000000 > --- a/drivers/iommu/mtk_iommu.h > +++ /dev/null > @@ -1,42 +0,0 @@ > -/* SPDX-License-Identifier: GPL-2.0-only */ > -/* > - * Copyright (c) 2015-2016 MediaTek Inc. > - * Author: Honghui Zhang <honghui.zhang@mediatek.com> > - */ > - > -#ifndef _MTK_IOMMU_H_ > -#define _MTK_IOMMU_H_ > - > -#include <linux/device.h> > -#include <linux/io.h> > -#include <linux/io-pgtable.h> > -#include <linux/iommu.h> > -#include <linux/spinlock.h> > -#include <soc/mediatek/smi.h> > -#include <dt-bindings/memory/mtk-memory-port.h> > - > -struct mtk_iommu_suspend_reg { > - union { > - u32 standard_axi_mode;/* v1 */ > - u32 misc_ctrl;/* v2 */ > - }; > - u32 dcm_dis; > - u32 ctrl_reg; > - u32 int_control0; > - u32 int_main_control; > - u32 ivrp_paddr; > - u32 vld_pa_rng; > - u32 wr_len_ctrl; > -}; > - > -static inline int compare_of(struct device *dev, void *data) > -{ > - return dev->of_node == data; > -} > - > -static inline void release_of(struct device *dev, void *data) > -{ > - of_node_put(data); > -} > - > -#endif > diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c > index b762a05328d4..23c3bc175153 100644 > --- a/drivers/iommu/mtk_iommu_v1.c > +++ b/drivers/iommu/mtk_iommu_v1.c > @@ -7,7 +7,6 @@ > * > * Based on driver/iommu/mtk_iommu.c > */ > -#include <linux/memblock.h> > #include <linux/bug.h> > #include <linux/clk.h> > #include <linux/component.h> > @@ -28,10 +27,9 @@ > #include <linux/spinlock.h> > #include <asm/barrier.h> > #include <asm/dma-iommu.h> > -#include <linux/init.h> > +#include <dt-bindings/memory/mtk-memory-port.h> > #include <dt-bindings/memory/mt2701-larb-port.h> > #include <soc/mediatek/smi.h> > -#include "mtk_iommu.h" > > #define REG_MMU_PT_BASE_ADDR 0x000 > > @@ -87,6 +85,13 @@ > */ > #define M2701_IOMMU_PGT_SIZE SZ_4M > > +struct mtk_iommu_suspend_reg { > + u32 standard_axi_mode; > + u32 dcm_dis; > + u32 ctrl_reg; > + u32 int_control0; > +}; > + > struct mtk_iommu_data { > void __iomem *base; > int irq; > @@ -110,6 +115,16 @@ struct mtk_iommu_domain { > struct mtk_iommu_data *data; > }; > > +static inline int compare_of(struct device *dev, void *data) > +{ > + return dev->of_node == data; > +} > + > +static inline void release_of(struct device *dev, void *data) > +{ > + of_node_put(data); > +} > + ....And the same comment applies here too. > static inline int mtk_iommu_bind(struct device *dev) > { > struct mtk_iommu_data *data = dev_get_drvdata(dev); >
Il 25/01/22 09:56, Yong Wu ha scritto: > Prepare for supporting multi-banks for the IOMMU HW, No functional change. > > Add a new structure(mtk_iommu_bank_data) for each a bank. Each a bank have > the independent HW base/IRQ/tlb-range ops, and each a bank has its special > iommu-domain(independent pgtable), thus, also move the domain information > into it. > > In previous SoC, we have only one bank which could be treated as bank0( > bankid always is 0 for the previous SoC). > > After adding this structure, the tlb operations and irq could use > bank_data as parameter. > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 25/01/22 09:56, Yong Wu ha scritto: > We preassign some ports in a special bank via the new defined > banks_portmsk. Put it in the plat_data means it is not expected to be > adjusted dynamically. > > If the iommu id in the iommu consumer's dtsi node is inside this > banks_portmsk, then we switch it to this special iommu bank, and > initialise the IOMMU bank HW. > > Each a bank has the independent pgtable(4GB iova range). Each a bank > is a independent iommu domain/group. Currently we don't separate different > iova ranges inside a bank. > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > --- > drivers/iommu/mtk_iommu.c | 39 ++++++++++++++++++++++++++++++++++++--- > 1 file changed, 36 insertions(+), 3 deletions(-) > > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c > index 22586d1aed72..c6de9304bbc6 100644 > --- a/drivers/iommu/mtk_iommu.c > +++ b/drivers/iommu/mtk_iommu.c > @@ -191,6 +191,7 @@ struct mtk_iommu_plat_data { > > u8 banks_num; > bool banks_enable[MTK_IOMMU_BANK_MAX]; > + unsigned int banks_portmsk[MTK_IOMMU_BANK_MAX]; I would prefer to see u32 here instead... but maybe that's just a personal preference, it doesn't really matter. In any case: Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 25/01/22 09:56, Yong Wu ha scritto: > Each bank has some independent registers. thus backup/restore them for > each a bank when suspend and resume. > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
Il 25/01/22 09:56, Yong Wu ha scritto: > Enable the multi-bank functions for infra-iommu. We put PCIE in bank0 > and USB in the last bank(bank4). and we don't use the other banks > currently, disable them. > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
On Thu, 2022-01-27 at 12:08 +0100, AngeloGioacchino Del Regno wrote: > Il 25/01/22 09:56, Yong Wu ha scritto: > > No need zero for the protect buffer that is only accessed by the > > IOMMU HW > > translation fault happened. > > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > > I would rather keep this a devm_kzalloc instead... the cost is very > minimal and > this will be handy when new hardware will be introduced, as it may > require a bigger > buffer: in that case, "older" platforms will use only part of it and > we may get > garbage data at the end. Currently this is to avoid zero 512 bytes for all the platforms. Sorry, I don't understand why it is unnecessary when the new hardware requires a bigger buffer. If the buffer becomes bigger, then clearing it to 0 need more cost. then this patch is more helpful? The content in this buffer is garbage, we won't care about or analyse it. > > Regards, > Angelo
On Wed, Feb 16, 2022 at 2:55 PM Yong Wu <yong.wu@mediatek.com> wrote: > > On Thu, 2022-01-27 at 12:08 +0100, AngeloGioacchino Del Regno wrote: > > Il 25/01/22 09:56, Yong Wu ha scritto: > > > No need zero for the protect buffer that is only accessed by the > > > IOMMU HW > > > translation fault happened. > > > > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > > > > I would rather keep this a devm_kzalloc instead... the cost is very > > minimal and > > this will be handy when new hardware will be introduced, as it may > > require a bigger > > buffer: in that case, "older" platforms will use only part of it and > > we may get > > garbage data at the end. > > Currently this is to avoid zero 512 bytes for all the platforms. > > Sorry, I don't understand why it is unnecessary when the new hardware > requires a bigger buffer. If the buffer becomes bigger, then clearing > it to 0 need more cost. then this patch is more helpful? > > The content in this buffer is garbage, we won't care about or analyse > it. I think we should zero it for security reasons regardless of any other aspects. With this patch it's leaking kernel data to the hardware. At the same time, we're talking here about something executed just 1 time when the driver probes. I don't think the cost would really matter. Best regards, Tomasz
On Wed, 2022-02-16 at 14:59 +0900, Tomasz Figa wrote: > On Wed, Feb 16, 2022 at 2:55 PM Yong Wu <yong.wu@mediatek.com> wrote: > > > > On Thu, 2022-01-27 at 12:08 +0100, AngeloGioacchino Del Regno > > wrote: > > > Il 25/01/22 09:56, Yong Wu ha scritto: > > > > No need zero for the protect buffer that is only accessed by > > > > the > > > > IOMMU HW > > > > translation fault happened. > > > > > > > > Signed-off-by: Yong Wu <yong.wu@mediatek.com> > > > > > > I would rather keep this a devm_kzalloc instead... the cost is > > > very > > > minimal and > > > this will be handy when new hardware will be introduced, as it > > > may > > > require a bigger > > > buffer: in that case, "older" platforms will use only part of it > > > and > > > we may get > > > garbage data at the end. > > > > Currently this is to avoid zero 512 bytes for all the platforms. > > > > Sorry, I don't understand why it is unnecessary when the new > > hardware > > requires a bigger buffer. If the buffer becomes bigger, then > > clearing > > it to 0 need more cost. then this patch is more helpful? > > > > The content in this buffer is garbage, we won't care about or > > analyse > > it. > > I think we should zero it for security reasons regardless of any > other > aspects. With this patch it's leaking kernel data to the hardware. > > At the same time, we're talking here about something executed just 1 > time when the driver probes. I don't think the cost would really > matter. OK. I will remove this patch in next version. Thanks. > > Best regards, > Tomasz > > _______________________________________________ > Linux-mediatek mailing list > Linux-mediatek@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/linux-mediatek
This patchset adds MT8195 iommu support. MT8195 have 3 IOMMU HWs. 2 IOMMU HW is for multimedia, and 1 IOMMU HW is for infra-master, like PCIe/USB. About the 2 MM IOMMU HW, something like this: IOMMU(VDO) IOMMU(VPP) | | SMI_COMMON(VDO) SMI_COMMON(VPP) --------------- ---------------- | | ... | | ... larb0 larb2 ... larb1 larb3 ... these two MM IOMMU HW share a pgtable. About the INFRA IOMMU, it don't have larbs, the master connects the iommu directly. It use a independent pgtable. Also, mt8195 IOMMU bank supports. Normally the IOMMU register size only is 0x1000. In this IOMMU HW, the register size is 5 * 0x1000. each 0x1000 is a bank. the banks' register look like this: ---------------------------------------- |bank0 | bank1 | bank2 | bank3 | bank4| ---------------------------------------- |global | |control| null |regs | ----------------------------------------- |bank |bank |bank |bank |bank | |regs |regs |regs |regs |regs | | | | | | | ----------------------------------------- All the banks share some global control registers, and each bank have its special bank registers, like pgtable base register, tlb operation registers, the fault status registers. In mt8195, we enable this bank feature for infra iommu, We put PCIe in bank0 and USB in bank4. they have independent pgtable. Change note: v4:1) Base on v5.16-rc1 2) Base on tlb logic 2, some patches in v3 has already gone through in that patchset. https://lore.kernel.org/linux-mediatek/20211208120744.2415-1-dafna.hirschfeld@collabora.com/ 3) Due to the unreadable union for v1/v2(comment in 26/33 of v3), I separate mtk_iommu_data for v1 and v2 totally, then remove mtk_iommu.h. please see patch[26/35][27/35]. 4) add two mutex for the internal data. patch[6/35][7/35]. 5) add a new flag PM_CLK_AO. v3: https://lore.kernel.org/linux-mediatek/20210923115840.17813-1-yong.wu@mediatek.com/ 1) base on v5.15-rc1 2) Adjust devlink with smi-common, not use the property(sub-sommon). 3) Adjust tlb_flush_all flow, a) Fix tlb_flush_all only is supported in bank0. b) add tlb-flush-all in the resume callback. c) remove the pm status checking in tlb-flush-all. The reason are showed in the commit message. 4) Allow IOMMU_DOMAIN_UNMANAGED since PCIe VFIO use that. 5) Fix a clk warning and a null abort when unbind the iommu driver. v2: https://lore.kernel.org/linux-mediatek/20210813065324.29220-1-yong.wu@mediatek.com/ 1) Base on v5.14-rc1. 2) Fix build fail for arm32. 3) Fix dt-binding issue from Rob. 4) Fix the bank issue when tlb flush. v1 always use bank->base. 5) adjust devlink with smi-common since the node may be smi-sub-common. 6) other changes: like reword some commit message(removing many "This patch..."); seperate serveral patches. v1: https://lore.kernel.org/linux-mediatek/20210630023504.18177-1-yong.wu@mediatek.com/ Base on v5.13-rc1 Yong Wu (35): dt-bindings: mediatek: mt8195: Add binding for MM IOMMU dt-bindings: mediatek: mt8195: Add binding for infra IOMMU iommu/mediatek: Fix 2 HW sharing pgtable issue iommu/mediatek: Add list_del in mtk_iommu_remove iommu/mediatek: Remove clk_disable in mtk_iommu_remove iommu/mediatek: Add mutex for m4u_group and m4u_dom in data iommu/mediatek: Add mutex for data in the mtk_iommu_domain iommu/mediatek: Use kmalloc for protect buffer iommu/mediatek: Adapt sharing and non-sharing pgtable case iommu/mediatek: Add 12G~16G support for multi domains iommu/mediatek: Add a flag DCM_DISABLE iommu/mediatek: Add a flag NON_STD_AXI iommu/mediatek: Remove the granule in the tlb flush iommu/mediatek: Always enable output PA over 32bits in isr iommu/mediatek: Add SUB_COMMON_3BITS flag iommu/mediatek: Add IOMMU_TYPE flag iommu/mediatek: Contain MM IOMMU flow with the MM TYPE iommu/mediatek: Adjust device link when it is sub-common iommu/mediatek: Allow IOMMU_DOMAIN_UNMANAGED for PCIe VFIO iommu/mediatek: Add a PM_CLK_AO flag for infra iommu iommu/mediatek: Add infra iommu support iommu/mediatek: Add PCIe support iommu/mediatek: Add mt8195 support iommu/mediatek: Only adjust code about register base iommu/mediatek: Just move code position in hw_init iommu/mediatek: Separate mtk_iommu_data for v1 and v2 iommu/mediatek: Remove mtk_iommu.h iommu/mediatek-v1: Just rename mtk_iommu to mtk_iommu_v1 iommu/mediatek: Add mtk_iommu_bank_data structure iommu/mediatek: Initialise bank HW for each a bank iommu/mediatek: Change the domid to iova_region_id iommu/mediatek: Get the proper bankid for multi banks iommu/mediatek: Initialise/Remove for multi bank dev iommu/mediatek: Backup/restore regsiters for multi banks iommu/mediatek: mt8195: Enable multi banks for infra iommu .../bindings/iommu/mediatek,iommu.yaml | 20 +- drivers/iommu/mtk_iommu.c | 951 +++++++++++++----- drivers/iommu/mtk_iommu.h | 111 -- drivers/iommu/mtk_iommu_v1.c | 243 +++-- .../dt-bindings/memory/mt8195-memory-port.h | 408 ++++++++ include/dt-bindings/memory/mtk-memory-port.h | 2 + 6 files changed, 1255 insertions(+), 480 deletions(-) delete mode 100644 drivers/iommu/mtk_iommu.h create mode 100644 include/dt-bindings/memory/mt8195-memory-port.h