Message-ID: <1502532287.11148.70.camel@mhfsdcap03>
Date:   Sat, 12 Aug 2017 18:04:47 +0800
From:   Yong Wu <yong.wu@...iatek.com>
To:     Robin Murphy <robin.murphy@....com>
CC:     Joerg Roedel <joro@...tes.org>, Rob Herring <robh+dt@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Will Deacon <will.deacon@....com>,
        Daniel Kurtz <djkurtz@...gle.com>,
        Tomasz Figa <tfiga@...gle.com>,
        Mark Rutland <mark.rutland@....com>,
        Catalin Marinas <catalin.marinas@....com>,
        <linux-mediatek@...ts.infradead.org>,
        <srv_heupstream@...iatek.com>, <devicetree@...r.kernel.org>,
        <linux-kernel@...r.kernel.org>,
        <linux-arm-kernel@...ts.infradead.org>,
        <iommu@...ts.linux-foundation.org>, <arnd@...db.de>,
        <honghui.zhang@...iatek.com>, <k.zhang@...iatek.com>,
        <cloud.chou@...iatek.com>,
        Arvind Yadav <arvind.yadav.cs@...il.com>,
        <youlin.pei@...iatek.com>
Subject: Re: [PATCH 2/8] iommu/mediatek: Add mt2712 IOMMU support

On Fri, 2017-08-11 at 18:24 +0100, Robin Murphy wrote:
> On 11/08/17 10:56, Yong Wu wrote:
> > The M4U IP blocks in mt2712 is MTK's generation2 M4U which use the
> > Short-descriptor like mt8173, and most of the HW registers are the
> > same.
> > 
> > The difference is that there are 2 M4U HWs in mt2712 while there's
> > only one in mt8173. The purpose of 2 M4U HWs is for balance the
> > bandwidth.
> 
> Heh, I never imagined that theoretical argument I made against global
> data in the original driver would become reality so soon :D

Sorry for this.

The global data you said should be "static LIST_HEAD(m4ulist); "

Actually, After this patch, the mt2712 IOMMU can work successfully.

In order to simplify the IOMMU client code, we add that global data to
merge 2 IOMMU domains into one, then they don't need map the iova range
from a iommu domain into another.

I'm not familiar with SMMU. The SMMU should have so many iommu
domains(each context bank has one.). So how the SMMU deal with this
case: Does the modules in different context banks need share the iova?
How does they share the iova?

> 
> > Normally if there are 2 M4U HWs, there should be 2 iommu domains,
> > each M4U has a iommu domain.
> > 
> > Signed-off-by: Yong Wu <yong.wu@...iatek.com>
> > ---
> > This patch also include a minor issue:
> > suspend while there is no iommu client. it will hang because
> > there is no iommu domain at that time.
> > ---
> >  drivers/iommu/mtk_iommu.c | 48 ++++++++++++++++++++++++++++++++---------------
> >  drivers/iommu/mtk_iommu.h |  7 +++++++
> >  drivers/memory/mtk-smi.c  | 40 ++++++++++++++++++++++++++++++++++++---
> >  3 files changed, 77 insertions(+), 18 deletions(-)
> > 
> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > index 91c6d36..da6cedb 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> > @@ -54,7 +54,9 @@
> >  
> >  #define REG_MMU_CTRL_REG			0x110
> >  #define F_MMU_PREFETCH_RT_REPLACE_MOD		BIT(4)
> > +/* The TF-protect-select is bit[5:4] in mt2712 while it's bit[6:5] in mt8173.*/
> >  #define F_MMU_TF_PROTECT_SEL(prot)		(((prot) & 0x3) << 5)
> > +#define F_MMU_TF_PROT_SEL(prot)			(((prot) & 0x3) << 4)
> 
> In my opinion PROTECT vs. PROT here is too confusing for its own good...

.Both the strings are from the coda released by our HW DE..

And your suggestion looks better. I will use:

/* mt2712 is named by F_MMU_TF_PROT_SEL */
#define F_MMU_TF_PROTECT_SEL_SHIFT(data) \
 	((data)->m4u_type == MT2172 ? 4 : 5)
#define F_MMU_TF_PROTECT_SEL(prot, data) \
 	((prot) & 0x3) << F_MMU_TF_PROTECT_SEL_SHIFT(data))

> 
> >  #define REG_MMU_IVRP_PADDR			0x114
> >  #define F_MMU_IVRP_PA_SET(pa, ext)		(((pa) >> 1) | ((!!(ext)) << 31))
> > @@ -301,10 +303,6 @@ static int mtk_iommu_attach_device(struct iommu_domain *domain,
> >  			data->m4u_dom = NULL;
> >  			return ret;
> >  		}
> > -	} else if (data->m4u_dom != dom) {
> > -		/* All the client devices should be in the same m4u domain */
> > -		dev_err(dev, "try to attach into the error iommu domain\n");
> > -		return -EPERM;
> >  	}
> >  
> >  	mtk_iommu_config(data, dev, true);
> > @@ -464,8 +462,12 @@ static int mtk_iommu_hw_init(const struct mtk_iommu_data *data)
> >  		return ret;
> >  	}
> >  
> > -	regval = F_MMU_PREFETCH_RT_REPLACE_MOD |
> > -		F_MMU_TF_PROTECT_SEL(2);
> > +	if (data->m4u_type == M4U_MT8173) {
> > +		regval = F_MMU_PREFETCH_RT_REPLACE_MOD |
> > +			F_MMU_TF_PROTECT_SEL(2);
> > +	} else {
> > +		regval = F_MMU_TF_PROT_SEL(2);
> 
> ...and it would be a bit more obvious to just use
> F_MMU_TF_PROTECT_SEL(2) >> 1 here (with the comment from above).
> Alternatively, the really bullet-proof option would be something like:
> 
> #define F_MMU_TF_PROTECT_SEL_SHIFT(data) \
> 	((data)->m4u_type == MT2172 ? 4 : 5)
> #define F_MMU_TF_PROTECT_SEL(prot, data) \
> 	((prot) & 0x3) << F_MMU_TF_PROTECT_SEL_SHIFT(data))
> 
> > +	}
> >  	writel_relaxed(regval, data->base + REG_MMU_CTRL_REG);
> >  
> >  	regval = F_L2_MULIT_HIT_EN |
> > @@ -487,9 +489,11 @@ static int mtk_iommu_hw_init(const struct mtk_iommu_data *data)
> >  
> >  	writel_relaxed(F_MMU_IVRP_PA_SET(data->protect_base, data->enable_4GB),
> >  		       data->base + REG_MMU_IVRP_PADDR);
> > -
> >  	writel_relaxed(0, data->base + REG_MMU_DCM_DIS);
> > -	writel_relaxed(0, data->base + REG_MMU_STANDARD_AXI_MODE);
> > +
> > +	/* It's MISC control register whose default value is ok except mt8173.*/
> > +	if (data->m4u_type == M4U_MT8173)
> > +		writel_relaxed(0, data->base + REG_MMU_STANDARD_AXI_MODE);
> >  
> >  	if (devm_request_irq(data->dev, data->irq, mtk_iommu_isr, 0,
> >  			     dev_name(data->dev), (void *)data)) {
> > @@ -521,6 +525,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
> >  	if (!data)
> >  		return -ENOMEM;
> >  	data->dev = dev;
> > +	data->m4u_type = (enum mtk_iommu_type)of_device_get_match_data(dev);
> >  
> >  	/* Protect memory. HW will access here while translation fault.*/
> >  	protect = devm_kzalloc(dev, MTK_PROTECT_PA_ALIGN * 2, GFP_KERNEL);
> > @@ -554,6 +559,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
> >  	for (i = 0; i < larb_nr; i++) {
> >  		struct device_node *larbnode;
> >  		struct platform_device *plarbdev;
> > +		unsigned int id;
> 
> Strictly, this should be u32...

OK.

> 
> >  
> >  		larbnode = of_parse_phandle(dev->of_node, "mediatek,larbs", i);
> >  		if (!larbnode)
> > @@ -562,6 +568,10 @@ static int mtk_iommu_probe(struct platform_device *pdev)
> >  		if (!of_device_is_available(larbnode))
> >  			continue;
> >  
> > +		ret = of_property_read_u32(larbnode, "mediatek,larb-id", &id);
> > +		if (ret)/* The id is consecutive if there is no this property */
> > +			id = i;
> 
> ...but wouldn't it make more sense for the SMI driver to handle this?
> Admittedly it looks like only this driver knows the default IDs thanks
> to the ordering of the phandle args, but the SMI driver could at least
> initialise larb->larbid to some sentinel value which could be detected
> and replaced with i here.

>From the HW diagram before, the "mediatek,larbs" is
"mediatek,larbs = <&larb0 &larb1 &larb2 &larb3 &larb6>" in M4U0. and
"mediatek,larbs = <&larb4 &larb5 &larb7>" in M4U1.

Both are not consecutive..this is different with mt8173 and mt2701.

The problem is that: In the mtk_iommu_config, it will get the larbid via
"larbid = MTK_M4U_TO_LARB(fwspec->ids[i])".
This function will get the right larbid.

Thus I have to initialize the array "data->smi_imu.larb_imu" with the
real lardid here.

SMI driver will probe defer because it is delayed by MTK power-domain.
It will be very late to initialize the larb-id. So currently IOMMU
initialize the array "smi_imu.larb_imu" according to the real larbid,
and later SMI driver read the array to get the iommu information about
each larb.
Thus, is there some other way the SMI driver can help handle this?

About "the ordering of the phandle args", I have requested it must 
sort in the binding description of "mediatek,larbs".

> 
> > +
> >  		plarbdev = of_find_device_by_node(larbnode);
> >  		if (!plarbdev) {
> >  			plarbdev = of_platform_device_create(
> > @@ -572,7 +582,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
> >  				return -EPROBE_DEFER;
> >  			}
> >  		}
> > -		data->smi_imu.larb_imu[i].dev = &plarbdev->dev;
> > +		data->smi_imu.larb_imu[id].dev = &plarbdev->dev;
> 
> Changing the way the larb_imu array is indexed also seems to create a
> worrying inconsistency with mtk_iommu_v1.
> 
> >  		component_match_add_release(dev, &match, release_of,
> >  					    compare_of, larbnode);
> > @@ -640,8 +650,6 @@ static int __maybe_unused mtk_iommu_resume(struct device *dev)
> >  	struct mtk_iommu_suspend_reg *reg = &data->reg;
> >  	void __iomem *base = data->base;
> >  
> > -	writel_relaxed(data->m4u_dom->cfg.arm_v7s_cfg.ttbr[0],
> > -		       base + REG_MMU_PT_BASE_ADDR);
> >  	writel_relaxed(reg->standard_axi_mode,
> >  		       base + REG_MMU_STANDARD_AXI_MODE);
> >  	writel_relaxed(reg->dcm_dis, base + REG_MMU_DCM_DIS);
> > @@ -650,15 +658,19 @@ static int __maybe_unused mtk_iommu_resume(struct device *dev)
> >  	writel_relaxed(reg->int_main_control, base + REG_MMU_INT_MAIN_CONTROL);
> >  	writel_relaxed(F_MMU_IVRP_PA_SET(data->protect_base, data->enable_4GB),
> >  		       base + REG_MMU_IVRP_PADDR);
> > +	if (data->m4u_dom)
> > +		writel(data->m4u_dom->cfg.arm_v7s_cfg.ttbr[0],
> > +		       base + REG_MMU_PT_BASE_ADDR);
> >  	return 0;
> >  }
> >  
> > -const struct dev_pm_ops mtk_iommu_pm_ops = {
> > +static const struct dev_pm_ops mtk_iommu_pm_ops = {
> >  	SET_SYSTEM_SLEEP_PM_OPS(mtk_iommu_suspend, mtk_iommu_resume)
> >  };
> >  
> >  static const struct of_device_id mtk_iommu_of_ids[] = {
> > -	{ .compatible = "mediatek,mt8173-m4u", },
> > +	{ .compatible = "mediatek,mt2712-m4u", .data = (void *)M4U_MT2712},
> > +	{ .compatible = "mediatek,mt8173-m4u", .data = (void *)M4U_MT8173},
> >  	{}
> >  };
> >  
> > @@ -667,16 +679,20 @@ static int __maybe_unused mtk_iommu_resume(struct device *dev)
> >  	.remove	= mtk_iommu_remove,
> >  	.driver	= {
> >  		.name = "mtk-iommu",
> > -		.of_match_table = mtk_iommu_of_ids,
> > +		.of_match_table = of_match_ptr(mtk_iommu_of_ids),
> >  		.pm = &mtk_iommu_pm_ops,
> >  	}
> >  };
> >  
> >  static int mtk_iommu_init_fn(struct device_node *np)
> >  {
> > +	static bool init_done;
> >  	int ret;
> >  	struct platform_device *pdev;
> >  
> > +	if (init_done)
> > +		return 0;
> > +
> 
> Actually, you can simply get rid of the whole init_fn now - the

Thanks. It looks ok after I change here to subsys_initcall.

> IOMMU_OF_DECLARE() table only remains as a way to identify built-in
> drivers for the probe-deferral decision. Hopefully the dodgy-looking

Do you means that IOMMU_OF_DECLARE is needed by probe-deferral drivers?

I tested a deferal driver without IOMMU_OF_DEDCALRE.It could also enter
our of_late. all the flow looks normal.

> forced creation of plarbdev in probe could go away as well.

Yes. I can get rid of it.

> 
> >  	pdev = of_platform_device_create(np, NULL, platform_bus_type.dev_root);
> >  	if (!pdev)
> >  		return -ENOMEM;
> > @@ -686,8 +702,10 @@ static int mtk_iommu_init_fn(struct device_node *np)
> >  		pr_err("%s: Failed to register driver\n", __func__);
> >  		return ret;
> >  	}
> > +	init_done = true;
> >  
> >  	return 0;
> >  }
> >  
> > -IOMMU_OF_DECLARE(mtkm4u, "mediatek,mt8173-m4u", mtk_iommu_init_fn);
> > +IOMMU_OF_DECLARE(mt8173m4u, "mediatek,mt8173-m4u", mtk_iommu_init_fn);
> > +IOMMU_OF_DECLARE(mt2712m4u, "mediatek,mt2712-m4u", mtk_iommu_init_fn);
> > diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
> > index c06cc91..cd729a3 100644
> > --- a/drivers/iommu/mtk_iommu.h
> > +++ b/drivers/iommu/mtk_iommu.h
> > @@ -34,6 +34,12 @@ struct mtk_iommu_suspend_reg {
> >  	u32				int_main_control;
> >  };
> >  
> > +enum mtk_iommu_type {
> > +	M4U_MT2701,
> > +	M4U_MT2712,
> > +	M4U_MT8173,
> > +};
> > +
> >  struct mtk_iommu_domain;
> >  
> >  struct mtk_iommu_data {
> > @@ -50,6 +56,7 @@ struct mtk_iommu_data {
> >  	bool				tlb_flush_active;
> >  
> >  	struct iommu_device		iommu;
> > +	enum mtk_iommu_type		m4u_type;
> >  };
> >  
> >  static inline int compare_of(struct device *dev, void *data)
> > diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
> > index 13f8c45..ec06d2b 100644
> > --- a/drivers/memory/mtk-smi.c
> > +++ b/drivers/memory/mtk-smi.c
> > @@ -23,7 +23,10 @@
> >  #include <soc/mediatek/smi.h>
> >  #include <dt-bindings/memory/mt2701-larb-port.h>
> >  
> > +/* mt8173 */
> >  #define SMI_LARB_MMU_EN		0xf00
> > +
> > +/* mt2701 */
> >  #define REG_SMI_SECUR_CON_BASE		0x5c0
> >  
> >  /* every register control 8 port, register offset 0x4 */
> > @@ -41,6 +44,10 @@
> >  /* mt2701 domain should be set to 3 */
> >  #define SMI_SECUR_CON_VAL_DOMAIN(id)	(0x3 << ((((id) & 0x7) << 2) + 1))
> >  
> > +/* mt2712 */
> > +#define SMI_LARB_NONSEC_CON(id)	(0x380 + (id * 4))
> > +#define F_MMU_EN		BIT(0)
> > +
> >  struct mtk_smi_larb_gen {
> >  	bool need_larbid;
> >  	int port_in_larb[MTK_LARB_NR_MAX + 1];
> > @@ -149,7 +156,7 @@ void mtk_smi_larb_put(struct device *larbdev)
> >  	struct mtk_smi_iommu *smi_iommu = data;
> >  	unsigned int         i;
> >  
> > -	for (i = 0; i < smi_iommu->larb_nr; i++) {
> > +	for (i = 0; i < MTK_LARB_NR_MAX; i++) {
> 
> This initially looked suspicious, but I guess it's related to the
> earlier change to indexing. As a result we seem to have a bit of a
> redundant mess where some things are using larb->larbid and others are
> relying on inferring it from the index in larb_imu.
> 
> I'm not sure whether it would end up better to use larbid consistently
> everywhere, or to convert everything to make make larb_imu officially a
> sparse array indexed by ID (and thus remove smi_iommu->larb_nr and
> larb->larbid), but a weird mix of both is not a great idea.

see above.

> 
> >  		if (dev == smi_iommu->larb_imu[i].dev) {
> >  			/* The 'mmu' may be updated in iommu-attach/detach. */
> >  			larb->mmu = &smi_iommu->larb_imu[i].mmu;
> > @@ -159,13 +166,28 @@ void mtk_smi_larb_put(struct device *larbdev)
> >  	return -ENODEV;
> >  }
> >  
> > -static void mtk_smi_larb_config_port(struct device *dev)
> > +static void mtk_smi_larb_config_port_mt8173(struct device *dev)
> >  {
> >  	struct mtk_smi_larb *larb = dev_get_drvdata(dev);
> >  
> >  	writel(*larb->mmu, larb->base + SMI_LARB_MMU_EN);
> >  }
> >  
> > +static void mtk_smi_larb_config_port_mt2712(struct device *dev)
> > +{
> > +	struct mtk_smi_larb *larb = dev_get_drvdata(dev);
> > +	u32 reg;
> > +	int i;
> > +
> > +	for (i = 0; i < 32; i++) {
> > +		if (*larb->mmu & BIT(i)) {
> 
> Seeing this immediately make me think of:
> 
> 	unsigned long enable = *larb->mmu;
> 
> 	for_each_set_bit(i, &enable, 32) {
> 		...
> 	}
> 
> but maybe that's overkill :/

Good enough..I will use "u32" to instead of "unsigned long" above.

> 
> Robin.
> 
> > +			reg = readl_relaxed(larb->base +
> > +					    SMI_LARB_NONSEC_CON(i));
> > +			reg |= F_MMU_EN;
> > +			writel(reg, larb->base + SMI_LARB_NONSEC_CON(i));
> > +		}
> > +	}
> > +}
> >  
> >  static void mtk_smi_larb_config_port_gen1(struct device *dev)
> >  {
> > @@ -211,7 +233,11 @@ static void mtk_smi_larb_config_port_gen1(struct device *dev)
> >  
> >  static const struct mtk_smi_larb_gen mtk_smi_larb_mt8173 = {
> >  	/* mt8173 do not need the port in larb */
> > -	.config_port = mtk_smi_larb_config_port,
> > +	.config_port = mtk_smi_larb_config_port_mt8173,
> > +};
> > +
> > +static const struct mtk_smi_larb_gen mtk_smi_larb_mt2712 = {
> > +	.config_port = mtk_smi_larb_config_port_mt2712,
> >  };
> >  
> >  static const struct mtk_smi_larb_gen mtk_smi_larb_mt2701 = {
> > @@ -232,6 +258,10 @@ static void mtk_smi_larb_config_port_gen1(struct device *dev)
> >  		.compatible = "mediatek,mt2701-smi-larb",
> >  		.data = &mtk_smi_larb_mt2701
> >  	},
> > +	{
> > +		.compatible = "mediatek,mt2712-smi-larb",
> > +		.data = &mtk_smi_larb_mt2712
> > +	},
> >  	{}
> >  };
> >  
> > @@ -318,6 +348,10 @@ static int mtk_smi_larb_remove(struct platform_device *pdev)
> >  		.compatible = "mediatek,mt2701-smi-common",
> >  		.data = (void *)MTK_SMI_GEN1
> >  	},
> > +	{
> > +		.compatible = "mediatek,mt2712-smi-common",
> > +		.data = (void *)MTK_SMI_GEN2
> > +	},
> >  	{}
> >  };
> >  
> > 
> 

Powered by blists - more mailing lists