Re: [PATCH 2/8] iommu/mediatek: Add mt2712 IOMMU support

2017-08-12 Thread Yong Wu 
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 
> > ---
> > 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_

Re: [PATCH 2/8] iommu/mediatek: Add mt2712 IOMMU support

2017-08-11 Thread Robin Murphy 
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

> Normally if there are 2 M4U HWs, there should be 2 iommu domains,
> each M4U has a iommu domain.
> 
> Signed-off-by: Yong Wu 
> ---
> 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_MODBIT(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...

>  #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...

>  
>   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 */
> +

[PATCH 2/8] iommu/mediatek: Add mt2712 IOMMU support

2017-08-11 Thread Yong Wu 
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.

Normally if there are 2 M4U HWs, there should be 2 iommu domains,
each M4U has a iommu domain.

Signed-off-by: Yong Wu 
---
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)
 
 #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);
+   }
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;
 
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;
+
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;
 
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;