Re: [PATCH v5 03/10] iommu/mediatek: Modify the usage of mtk_iommu_plat_data structure

Re: [PATCH v5 03/10] iommu/mediatek: Modify the usage of mtk_iommu_plat_data structure

[Matthias Brugger]

On 29/06/2020 09:13, Chao Hao wrote:
> Given the fact that we are adding more and more plat_data bool values,
> it would make sense to use a u32 flags register and add the appropriate
> macro definitions to set and check for a flag present.
> No functional change.
> 
> Suggested-by: Matthias Brugger <matthias.bgg@gmail.com>
> Signed-off-by: Chao Hao <chao.hao@mediatek.com>

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

> ---
>  drivers/iommu/mtk_iommu.c | 23 ++++++++++++-----------
>  drivers/iommu/mtk_iommu.h | 16 ++++++++++------
>  2 files changed, 22 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 88d3df5b91c2..8f81df6cbe51 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -563,7 +563,8 @@ static int mtk_iommu_hw_init(const struct mtk_iommu_data *data)
>  			 upper_32_bits(data->protect_base);
>  	writel_relaxed(regval, data->base + REG_MMU_IVRP_PADDR);
>  
> -	if (data->enable_4GB && data->plat_data->has_vld_pa_rng) {
> +	if (data->enable_4GB &&
> +	    MTK_IOMMU_HAS_FLAG(data->plat_data, HAS_VLD_PA_RNG)) {
>  		/*
>  		 * If 4GB mode is enabled, the validate PA range is from
>  		 * 0x1_0000_0000 to 0x1_ffff_ffff. here record bit[32:30].
> @@ -573,7 +574,7 @@ static int mtk_iommu_hw_init(const struct mtk_iommu_data *data)
>  	}
>  	writel_relaxed(0, data->base + REG_MMU_DCM_DIS);
>  
> -	if (data->plat_data->reset_axi) {
> +	if (MTK_IOMMU_HAS_FLAG(data->plat_data, RESET_AXI)) {
>  		/* The register is called STANDARD_AXI_MODE in this case */
>  		writel_relaxed(0, data->base + REG_MMU_MISC_CTRL);
>  	}
> @@ -618,7 +619,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
>  
>  	/* Whether the current dram is over 4GB */
>  	data->enable_4GB = !!(max_pfn > (BIT_ULL(32) >> PAGE_SHIFT));
> -	if (!data->plat_data->has_4gb_mode)
> +	if (!MTK_IOMMU_HAS_FLAG(data->plat_data, HAS_4GB_MODE))
>  		data->enable_4GB = false;
>  
>  	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -631,7 +632,7 @@ static int mtk_iommu_probe(struct platform_device *pdev)
>  	if (data->irq < 0)
>  		return data->irq;
>  
> -	if (data->plat_data->has_bclk) {
> +	if (MTK_IOMMU_HAS_FLAG(data->plat_data, HAS_BCLK)) {
>  		data->bclk = devm_clk_get(dev, "bclk");
>  		if (IS_ERR(data->bclk))
>  			return PTR_ERR(data->bclk);
> @@ -763,23 +764,23 @@ static const struct dev_pm_ops mtk_iommu_pm_ops = {
>  
>  static const struct mtk_iommu_plat_data mt2712_data = {
>  	.m4u_plat     = M4U_MT2712,
> -	.has_4gb_mode = true,
> -	.has_bclk     = true,
> -	.has_vld_pa_rng   = true,
> +	.flags        = HAS_4GB_MODE |
> +			HAS_BCLK |
> +			HAS_VLD_PA_RNG,
>  	.larbid_remap = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9},
>  };
>  
>  static const struct mtk_iommu_plat_data mt8173_data = {
>  	.m4u_plat     = M4U_MT8173,
> -	.has_4gb_mode = true,
> -	.has_bclk     = true,
> -	.reset_axi    = true,
> +	.flags	      = HAS_4GB_MODE |
> +			HAS_BCLK |
> +			RESET_AXI,
>  	.larbid_remap = {0, 1, 2, 3, 4, 5}, /* Linear mapping. */
>  };
>  
>  static const struct mtk_iommu_plat_data mt8183_data = {
>  	.m4u_plat     = M4U_MT8183,
> -	.reset_axi    = true,
> +	.flags        = RESET_AXI,
>  	.larbid_remap = {0, 4, 5, 6, 7, 2, 3, 1},
>  };
>  
> diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
> index 1b6ea839b92c..7cc39f729263 100644
> --- a/drivers/iommu/mtk_iommu.h
> +++ b/drivers/iommu/mtk_iommu.h
> @@ -17,6 +17,15 @@
>  #include <linux/spinlock.h>
>  #include <soc/mediatek/smi.h>
>  
> +#define HAS_4GB_MODE			BIT(0)
> +/* HW will use the EMI clock if there isn't the "bclk". */
> +#define HAS_BCLK			BIT(1)
> +#define HAS_VLD_PA_RNG			BIT(2)
> +#define RESET_AXI			BIT(3)
> +
> +#define MTK_IOMMU_HAS_FLAG(pdata, _x) \
> +		((((pdata)->flags) & (_x)) == (_x))
> +
>  struct mtk_iommu_suspend_reg {
>  	u32				misc_ctrl;
>  	u32				dcm_dis;
> @@ -36,12 +45,7 @@ enum mtk_iommu_plat {
>  
>  struct mtk_iommu_plat_data {
>  	enum mtk_iommu_plat m4u_plat;
> -	bool                has_4gb_mode;
> -
> -	/* HW will use the EMI clock if there isn't the "bclk". */
> -	bool                has_bclk;
> -	bool                has_vld_pa_rng;
> -	bool                reset_axi;
> +	u32                 flags;
>  	unsigned char       larbid_remap[MTK_LARB_NR_MAX];
>  };
>  
> 

Re: [PATCH v5 03/10] iommu/mediatek: Modify the usage of mtk_iommu_plat_data structure

[Yong Wu]

Hi Chao,

This is also ok for me. Only two format nitpick.

On Mon, 2020-06-29 at 15:13 +0800, Chao Hao wrote:
> Given the fact that we are adding more and more plat_data bool values,
> it would make sense to use a u32 flags register and add the appropriate
> macro definitions to set and check for a flag present.
> No functional change.
> 
> Suggested-by: Matthias Brugger <matthias.bgg@gmail.com>
> Signed-off-by: Chao Hao <chao.hao@mediatek.com>
> ---

[snip]

>  static const struct mtk_iommu_plat_data mt2712_data = {
>  	.m4u_plat     = M4U_MT2712,
> -	.has_4gb_mode = true,
> -	.has_bclk     = true,
> -	.has_vld_pa_rng   = true,
> +	.flags        = HAS_4GB_MODE |
> +			HAS_BCLK |
> +			HAS_VLD_PA_RNG,

short enough. we can put it in one line?

>  	.larbid_remap = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9},
>  };
>  
>  static const struct mtk_iommu_plat_data mt8173_data = {
>  	.m4u_plat     = M4U_MT8173,
> -	.has_4gb_mode = true,
> -	.has_bclk     = true,
> -	.reset_axi    = true,
> +	.flags	      = HAS_4GB_MODE |
> +			HAS_BCLK |
> +			RESET_AXI,
>  	.larbid_remap = {0, 1, 2, 3, 4, 5}, /* Linear mapping. */
>  };
>  
>  static const struct mtk_iommu_plat_data mt8183_data = {
>  	.m4u_plat     = M4U_MT8183,
> -	.reset_axi    = true,
> +	.flags        = RESET_AXI,
>  	.larbid_remap = {0, 4, 5, 6, 7, 2, 3, 1},
>  };
>  
> diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
> index 1b6ea839b92c..7cc39f729263 100644
> --- a/drivers/iommu/mtk_iommu.h
> +++ b/drivers/iommu/mtk_iommu.h
> @@ -17,6 +17,15 @@
>  #include <linux/spinlock.h>
>  #include <soc/mediatek/smi.h>
>  
> +#define HAS_4GB_MODE			BIT(0)
> +/* HW will use the EMI clock if there isn't the "bclk". */
> +#define HAS_BCLK			BIT(1)
> +#define HAS_VLD_PA_RNG			BIT(2)
> +#define RESET_AXI			BIT(3)
> +
> +#define MTK_IOMMU_HAS_FLAG(pdata, _x) \
> +		((((pdata)->flags) & (_x)) == (_x))

If these definitions are not used in mtk_iommu_v1.c(also no this plan),
then we can put them in the mtk_iommu.c.


BTW, the patch title "modify the usage of mtk_iommu_plat_data structure"
isn't so clear, we could write what the detailed modification is.
something like:
iommu/mediatek: Use a u32 flags to describe different HW features

> +
>  struct mtk_iommu_suspend_reg {
>  	u32				misc_ctrl;
>  	u32				dcm_dis;
> @@ -36,12 +45,7 @@ enum mtk_iommu_plat {
>  
>  struct mtk_iommu_plat_data {
>  	enum mtk_iommu_plat m4u_plat;
> -	bool                has_4gb_mode;
> -
> -	/* HW will use the EMI clock if there isn't the "bclk". */
> -	bool                has_bclk;
> -	bool                has_vld_pa_rng;
> -	bool                reset_axi;
> +	u32                 flags;
>  	unsigned char       larbid_remap[MTK_LARB_NR_MAX];
>  };
>  

Re: [PATCH v5 03/10] iommu/mediatek: Modify the usage of mtk_iommu_plat_data structure

[chao hao]

On Tue, 2020-06-30 at 18:56 +0800, Yong Wu wrote:
> Hi Chao,
> 
> This is also ok for me. Only two format nitpick.
> 
> On Mon, 2020-06-29 at 15:13 +0800, Chao Hao wrote:
> > Given the fact that we are adding more and more plat_data bool values,
> > it would make sense to use a u32 flags register and add the appropriate
> > macro definitions to set and check for a flag present.
> > No functional change.
> > 
> > Suggested-by: Matthias Brugger <matthias.bgg@gmail.com>
> > Signed-off-by: Chao Hao <chao.hao@mediatek.com>
> > ---
> 
> [snip]
> 
> >  static const struct mtk_iommu_plat_data mt2712_data = {
> >  	.m4u_plat     = M4U_MT2712,
> > -	.has_4gb_mode = true,
> > -	.has_bclk     = true,
> > -	.has_vld_pa_rng   = true,
> > +	.flags        = HAS_4GB_MODE |
> > +			HAS_BCLK |
> > +			HAS_VLD_PA_RNG,
> 
> short enough. we can put it in one line?

ok, I will try to put it in one line in next version, thanks

> 
> >  	.larbid_remap = {0, 1, 2, 3, 4, 5, 6, 7, 8, 9},
> >  };
> >  
> >  static const struct mtk_iommu_plat_data mt8173_data = {
> >  	.m4u_plat     = M4U_MT8173,
> > -	.has_4gb_mode = true,
> > -	.has_bclk     = true,
> > -	.reset_axi    = true,
> > +	.flags	      = HAS_4GB_MODE |
> > +			HAS_BCLK |
> > +			RESET_AXI,
> >  	.larbid_remap = {0, 1, 2, 3, 4, 5}, /* Linear mapping. */
> >  };
> >  
> >  static const struct mtk_iommu_plat_data mt8183_data = {
> >  	.m4u_plat     = M4U_MT8183,
> > -	.reset_axi    = true,
> > +	.flags        = RESET_AXI,
> >  	.larbid_remap = {0, 4, 5, 6, 7, 2, 3, 1},
> >  };
> >  
> > diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
> > index 1b6ea839b92c..7cc39f729263 100644
> > --- a/drivers/iommu/mtk_iommu.h
> > +++ b/drivers/iommu/mtk_iommu.h
> > @@ -17,6 +17,15 @@
> >  #include <linux/spinlock.h>
> >  #include <soc/mediatek/smi.h>
> >  
> > +#define HAS_4GB_MODE			BIT(0)
> > +/* HW will use the EMI clock if there isn't the "bclk". */
> > +#define HAS_BCLK			BIT(1)
> > +#define HAS_VLD_PA_RNG			BIT(2)
> > +#define RESET_AXI			BIT(3)
> > +
> > +#define MTK_IOMMU_HAS_FLAG(pdata, _x) \
> > +		((((pdata)->flags) & (_x)) == (_x))
> 
> If these definitions are not used in mtk_iommu_v1.c(also no this plan),
> then we can put them in the mtk_iommu.c.
> 

ok, mtk_iommu_v1.c doesn't use these definitions.
I will move them to mtk_iommu.c in next version, thanks.

> 
> BTW, the patch title "modify the usage of mtk_iommu_plat_data structure"
> isn't so clear, we could write what the detailed modification is.
> something like:
> iommu/mediatek: Use a u32 flags to describe different HW features
> 
got it , thanks for you advice.


> > +
> >  struct mtk_iommu_suspend_reg {
> >  	u32				misc_ctrl;
> >  	u32				dcm_dis;
> > @@ -36,12 +45,7 @@ enum mtk_iommu_plat {
> >  
> >  struct mtk_iommu_plat_data {
> >  	enum mtk_iommu_plat m4u_plat;
> > -	bool                has_4gb_mode;
> > -
> > -	/* HW will use the EMI clock if there isn't the "bclk". */
> > -	bool                has_bclk;
> > -	bool                has_vld_pa_rng;
> > -	bool                reset_axi;
> > +	u32                 flags;
> >  	unsigned char       larbid_remap[MTK_LARB_NR_MAX];
> >  };
> >  
> 
> 

Re: [PATCH v5 04/10] iommu/mediatek: Setting MISC_CTRL register

[Matthias Brugger]

On 29/06/2020 09:13, Chao Hao wrote:
> Add F_MMU_IN_ORDER_WR_EN and F_MMU_STANDARD_AXI_MODE_BIT definition
> in MISC_CTRL register.
> F_MMU_STANDARD_AXI_MODE_BIT:
>   If we set F_MMU_STANDARD_AXI_MODE_BIT(bit[3][19] = 0, not follow
> standard AXI protocol), iommu will send urgent read command firstly
> compare with normal read command to improve performance.

Can you please help me to understand the phrase. Sorry I'm not a AXI specialist.
Does this mean that you will send a 'urgent read command' which is not described
in the specifications instead of a normal read command?

> F_MMU_IN_ORDER_WR_EN:
>   If we set F_MMU_IN_ORDER_WR_EN(bit[1][17] = 0, out-of-order write), iommu
> will re-order write command and send more higher priority write command
> instead of sending write command in order. The feature be controlled
> by OUT_ORDER_EN macro definition.
> 
> Cc: Matthias Brugger <matthias.bgg@gmail.com>
> Suggested-by: Yong Wu <yong.wu@mediatek.com>
> Signed-off-by: Chao Hao <chao.hao@mediatek.com>
> ---
>  drivers/iommu/mtk_iommu.c | 12 +++++++++++-
>  drivers/iommu/mtk_iommu.h |  1 +
>  2 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 8f81df6cbe51..67b46b5d83d9 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -42,6 +42,9 @@
>  #define F_INVLD_EN1				BIT(1)
>  
>  #define REG_MMU_MISC_CTRL			0x048
> +#define F_MMU_IN_ORDER_WR_EN			(BIT(1) | BIT(17))
> +#define F_MMU_STANDARD_AXI_MODE_BIT		(BIT(3) | BIT(19))

Wouldn't it make more sense to name it F_MMU_STANDARD_AXI_MODE_EN?

> +
>  #define REG_MMU_DCM_DIS				0x050
>  
>  #define REG_MMU_CTRL_REG			0x110
> @@ -574,10 +577,17 @@ static int mtk_iommu_hw_init(const struct mtk_iommu_data *data)
>  	}
>  	writel_relaxed(0, data->base + REG_MMU_DCM_DIS);
>  
> +	regval = readl_relaxed(data->base + REG_MMU_MISC_CTRL);

We only need to read regval in the else branch.

>  	if (MTK_IOMMU_HAS_FLAG(data->plat_data, RESET_AXI)) {
>  		/* The register is called STANDARD_AXI_MODE in this case */
> -		writel_relaxed(0, data->base + REG_MMU_MISC_CTRL);
> +		regval = 0;
> +	} else {
> +		/* For mm_iommu, it can improve performance by the setting */
> +		regval &= ~F_MMU_STANDARD_AXI_MODE_BIT;
> +		if (MTK_IOMMU_HAS_FLAG(data->plat_data, OUT_ORDER_EN))
> +			regval &= ~F_MMU_IN_ORDER_WR_EN;
>  	}
> +	writel_relaxed(regval, data->base + REG_MMU_MISC_CTRL);
>  
>  	if (devm_request_irq(data->dev, data->irq, mtk_iommu_isr, 0,
>  			     dev_name(data->dev), (void *)data)) {
> diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
> index 7cc39f729263..4b780b651ef4 100644
> --- a/drivers/iommu/mtk_iommu.h
> +++ b/drivers/iommu/mtk_iommu.h
> @@ -22,6 +22,7 @@
>  #define HAS_BCLK			BIT(1)
>  #define HAS_VLD_PA_RNG			BIT(2)
>  #define RESET_AXI			BIT(3)
> +#define OUT_ORDER_EN			BIT(4)

Maybe something like OUT_ORDER_WR_EN, to make clear that it's about the the
write path.

>  
>  #define MTK_IOMMU_HAS_FLAG(pdata, _x) \
>  		((((pdata)->flags) & (_x)) == (_x))
> 

Re: [PATCH v5 04/10] iommu/mediatek: Setting MISC_CTRL register

[chao hao]

On Mon, 2020-06-29 at 11:28 +0200, Matthias Brugger wrote:
> 
> On 29/06/2020 09:13, Chao Hao wrote:
> > Add F_MMU_IN_ORDER_WR_EN and F_MMU_STANDARD_AXI_MODE_BIT definition
> > in MISC_CTRL register.
> > F_MMU_STANDARD_AXI_MODE_BIT:
> >   If we set F_MMU_STANDARD_AXI_MODE_BIT(bit[3][19] = 0, not follow
> > standard AXI protocol), iommu will send urgent read command firstly
> > compare with normal read command to improve performance.
> 
> Can you please help me to understand the phrase. Sorry I'm not a AXI specialist.
> Does this mean that you will send a 'urgent read command' which is not described
> in the specifications instead of a normal read command?

ok.
iommu sends read command to next bus_node normally(we can name it to
cmd1), when cmd1 isn't handled by next bus_node, iommu has a urgent read
command is needed to be sent(we can name it to cmd2), iommu will send
cmd2 and replace cmd1. So cmd2 is handled by next bus_node firstly and
cmd2 will be handled secondly.
But for standard AXI protocol, it will ignore the priority of read
command and only be handled in order. So cmd2 is handled by next
bus_node after cmd1 is done.

> 
> > F_MMU_IN_ORDER_WR_EN:
> >   If we set F_MMU_IN_ORDER_WR_EN(bit[1][17] = 0, out-of-order write), iommu
> > will re-order write command and send more higher priority write command
> > instead of sending write command in order. The feature be controlled
> > by OUT_ORDER_EN macro definition.
> > 
> > Cc: Matthias Brugger <matthias.bgg@gmail.com>
> > Suggested-by: Yong Wu <yong.wu@mediatek.com>
> > Signed-off-by: Chao Hao <chao.hao@mediatek.com>
> > ---
> >  drivers/iommu/mtk_iommu.c | 12 +++++++++++-
> >  drivers/iommu/mtk_iommu.h |  1 +
> >  2 files changed, 12 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > index 8f81df6cbe51..67b46b5d83d9 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> > @@ -42,6 +42,9 @@
> >  #define F_INVLD_EN1				BIT(1)
> >  
> >  #define REG_MMU_MISC_CTRL			0x048
> > +#define F_MMU_IN_ORDER_WR_EN			(BIT(1) | BIT(17))
> > +#define F_MMU_STANDARD_AXI_MODE_BIT		(BIT(3) | BIT(19))
> 
> Wouldn't it make more sense to name it F_MMU_STANDARD_AXI_MODE_EN?
ok, you are right.
1'b1: follow standard axi protocol

> 
> > +
> >  #define REG_MMU_DCM_DIS				0x050
> >  
> >  #define REG_MMU_CTRL_REG			0x110
> > @@ -574,10 +577,17 @@ static int mtk_iommu_hw_init(const struct mtk_iommu_data *data)
> >  	}
> >  	writel_relaxed(0, data->base + REG_MMU_DCM_DIS);
> >  
> > +	regval = readl_relaxed(data->base + REG_MMU_MISC_CTRL);
> 
> We only need to read regval in the else branch.

ok, I got it. thanks

> 
> >  	if (MTK_IOMMU_HAS_FLAG(data->plat_data, RESET_AXI)) {
> >  		/* The register is called STANDARD_AXI_MODE in this case */
> > -		writel_relaxed(0, data->base + REG_MMU_MISC_CTRL);
> > +		regval = 0;
> > +	} else {
> > +		/* For mm_iommu, it can improve performance by the setting */
> > +		regval &= ~F_MMU_STANDARD_AXI_MODE_BIT;
> > +		if (MTK_IOMMU_HAS_FLAG(data->plat_data, OUT_ORDER_EN))
> > +			regval &= ~F_MMU_IN_ORDER_WR_EN;
> >  	}
> > +	writel_relaxed(regval, data->base + REG_MMU_MISC_CTRL);
> >  
> >  	if (devm_request_irq(data->dev, data->irq, mtk_iommu_isr, 0,
> >  			     dev_name(data->dev), (void *)data)) {
> > diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
> > index 7cc39f729263..4b780b651ef4 100644
> > --- a/drivers/iommu/mtk_iommu.h
> > +++ b/drivers/iommu/mtk_iommu.h
> > @@ -22,6 +22,7 @@
> >  #define HAS_BCLK			BIT(1)
> >  #define HAS_VLD_PA_RNG			BIT(2)
> >  #define RESET_AXI			BIT(3)
> > +#define OUT_ORDER_EN			BIT(4)
> 
> Maybe something like OUT_ORDER_WR_EN, to make clear that it's about the the
> write path.
> 
ok, thanks for your advice.

> >  
> >  #define MTK_IOMMU_HAS_FLAG(pdata, _x) \
> >  		((((pdata)->flags) & (_x)) == (_x))
> > 

Re: [PATCH v5 04/10] iommu/mediatek: Setting MISC_CTRL register

[Matthias Brugger]

On 30/06/2020 12:53, chao hao wrote:
> On Mon, 2020-06-29 at 11:28 +0200, Matthias Brugger wrote:
>>
>> On 29/06/2020 09:13, Chao Hao wrote:
>>> Add F_MMU_IN_ORDER_WR_EN and F_MMU_STANDARD_AXI_MODE_BIT definition
>>> in MISC_CTRL register.
>>> F_MMU_STANDARD_AXI_MODE_BIT:
>>>   If we set F_MMU_STANDARD_AXI_MODE_BIT(bit[3][19] = 0, not follow
>>> standard AXI protocol), iommu will send urgent read command firstly
>>> compare with normal read command to improve performance.
>>
>> Can you please help me to understand the phrase. Sorry I'm not a AXI specialist.
>> Does this mean that you will send a 'urgent read command' which is not described
>> in the specifications instead of a normal read command?
> 
> ok.
> iommu sends read command to next bus_node normally(we can name it to
> cmd1), when cmd1 isn't handled by next bus_node, iommu has a urgent read
> command is needed to be sent(we can name it to cmd2), iommu will send
> cmd2 and replace cmd1. So cmd2 is handled by next bus_node firstly and
> cmd2 will be handled secondly.
> But for standard AXI protocol, it will ignore the priority of read
> command and only be handled in order. So cmd2 is handled by next
> bus_node after cmd1 is done.
> 

Thanks. So I propose change this part of the commit message to something like:
F_MMU_STANDARD_AXI_MODE_BIT:
If we set F_MMU_STANDARD_AXI_MODE_EN_MASK (bit[3][19] = 0, not follow standard
AXI protocol), the iommu will priorize sending of urgent read command over a
normal read command. This improves the performance.

>>
>>> F_MMU_IN_ORDER_WR_EN:
>>>   If we set F_MMU_IN_ORDER_WR_EN(bit[1][17] = 0, out-of-order write), iommu
>>> will re-order write command and send more higher priority write command
>>> instead of sending write command in order. The feature be controlled
>>> by OUT_ORDER_EN macro definition.

F_MMU_IN_ORDER_WR_EN:
If we set F_MMU_IN_ORDER_WR_EN_MASK (bit[1][17] = 0, out-of-order write), the
iommu will re-order write commands and send the write command with higher
priority. Otherwise the sending of write commands will be done in order. The
feature is controlled by OUT_ORDER_WR_EN platform data flag.


>>>
>>> Cc: Matthias Brugger <matthias.bgg@gmail.com>
>>> Suggested-by: Yong Wu <yong.wu@mediatek.com>
>>> Signed-off-by: Chao Hao <chao.hao@mediatek.com>
>>> ---
>>>  drivers/iommu/mtk_iommu.c | 12 +++++++++++-
>>>  drivers/iommu/mtk_iommu.h |  1 +
>>>  2 files changed, 12 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
>>> index 8f81df6cbe51..67b46b5d83d9 100644
>>> --- a/drivers/iommu/mtk_iommu.c
>>> +++ b/drivers/iommu/mtk_iommu.c
>>> @@ -42,6 +42,9 @@
>>>  #define F_INVLD_EN1				BIT(1)
>>>  
>>>  #define REG_MMU_MISC_CTRL			0x048
>>> +#define F_MMU_IN_ORDER_WR_EN			(BIT(1) | BIT(17))
>>> +#define F_MMU_STANDARD_AXI_MODE_BIT		(BIT(3) | BIT(19))
>>
>> Wouldn't it make more sense to name it F_MMU_STANDARD_AXI_MODE_EN?
> ok, you are right.
> 1'b1: follow standard axi protocol
> 

What about
F_MMU_IN_ORDER_WR_EN_MASK
F_MMU_STANDARD_AXI_MODE_EN_MASK

Background is that we have to set/unset two bits to enable or disable the
feature, so it's a mask we have to apply to the register.

Regards,
Matthias

>>
>>> +
>>>  #define REG_MMU_DCM_DIS				0x050
>>>  
>>>  #define REG_MMU_CTRL_REG			0x110
>>> @@ -574,10 +577,17 @@ static int mtk_iommu_hw_init(const struct mtk_iommu_data *data)
>>>  	}
>>>  	writel_relaxed(0, data->base + REG_MMU_DCM_DIS);
>>>  
>>> +	regval = readl_relaxed(data->base + REG_MMU_MISC_CTRL);
>>
>> We only need to read regval in the else branch.
> 
> ok, I got it. thanks
> 
>>
>>>  	if (MTK_IOMMU_HAS_FLAG(data->plat_data, RESET_AXI)) {
>>>  		/* The register is called STANDARD_AXI_MODE in this case */
>>> -		writel_relaxed(0, data->base + REG_MMU_MISC_CTRL);
>>> +		regval = 0;
>>> +	} else {
>>> +		/* For mm_iommu, it can improve performance by the setting */
>>> +		regval &= ~F_MMU_STANDARD_AXI_MODE_BIT;
>>> +		if (MTK_IOMMU_HAS_FLAG(data->plat_data, OUT_ORDER_EN))
>>> +			regval &= ~F_MMU_IN_ORDER_WR_EN;
>>>  	}
>>> +	writel_relaxed(regval, data->base + REG_MMU_MISC_CTRL);
>>>  
>>>  	if (devm_request_irq(data->dev, data->irq, mtk_iommu_isr, 0,
>>>  			     dev_name(data->dev), (void *)data)) {
>>> diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
>>> index 7cc39f729263..4b780b651ef4 100644
>>> --- a/drivers/iommu/mtk_iommu.h
>>> +++ b/drivers/iommu/mtk_iommu.h
>>> @@ -22,6 +22,7 @@
>>>  #define HAS_BCLK			BIT(1)
>>>  #define HAS_VLD_PA_RNG			BIT(2)
>>>  #define RESET_AXI			BIT(3)
>>> +#define OUT_ORDER_EN			BIT(4)
>>
>> Maybe something like OUT_ORDER_WR_EN, to make clear that it's about the the
>> write path.
>>
> ok, thanks for your advice.
> 
>>>  
>>>  #define MTK_IOMMU_HAS_FLAG(pdata, _x) \
>>>  		((((pdata)->flags) & (_x)) == (_x))
>>>
> 

Re: [PATCH v5 04/10] iommu/mediatek: Setting MISC_CTRL register

[chao hao]

On Wed, 2020-07-01 at 16:58 +0200, Matthias Brugger wrote:
> 
> On 30/06/2020 12:53, chao hao wrote:
> > On Mon, 2020-06-29 at 11:28 +0200, Matthias Brugger wrote:
> >>
> >> On 29/06/2020 09:13, Chao Hao wrote:
> >>> Add F_MMU_IN_ORDER_WR_EN and F_MMU_STANDARD_AXI_MODE_BIT definition
> >>> in MISC_CTRL register.
> >>> F_MMU_STANDARD_AXI_MODE_BIT:
> >>>   If we set F_MMU_STANDARD_AXI_MODE_BIT(bit[3][19] = 0, not follow
> >>> standard AXI protocol), iommu will send urgent read command firstly
> >>> compare with normal read command to improve performance.
> >>
> >> Can you please help me to understand the phrase. Sorry I'm not a AXI specialist.
> >> Does this mean that you will send a 'urgent read command' which is not described
> >> in the specifications instead of a normal read command?
> > 
> > ok.
> > iommu sends read command to next bus_node normally(we can name it to
> > cmd1), when cmd1 isn't handled by next bus_node, iommu has a urgent read
> > command is needed to be sent(we can name it to cmd2), iommu will send
> > cmd2 and replace cmd1. So cmd2 is handled by next bus_node firstly and
> > cmd2 will be handled secondly.
> > But for standard AXI protocol, it will ignore the priority of read
> > command and only be handled in order. So cmd2 is handled by next
> > bus_node after cmd1 is done.
> > 
> 
> Thanks. So I propose change this part of the commit message to something like:
> F_MMU_STANDARD_AXI_MODE_BIT:
> If we set F_MMU_STANDARD_AXI_MODE_EN_MASK (bit[3][19] = 0, not follow standard
> AXI protocol), the iommu will priorize sending of urgent read command over a
> normal read command. This improves the performance.
> 
ok, thanks

> >>
> >>> F_MMU_IN_ORDER_WR_EN:
> >>>   If we set F_MMU_IN_ORDER_WR_EN(bit[1][17] = 0, out-of-order write), iommu
> >>> will re-order write command and send more higher priority write command
> >>> instead of sending write command in order. The feature be controlled
> >>> by OUT_ORDER_EN macro definition.
> 
> F_MMU_IN_ORDER_WR_EN:
> If we set F_MMU_IN_ORDER_WR_EN_MASK (bit[1][17] = 0, out-of-order write), the
> iommu will re-order write commands and send the write command with higher
> priority. Otherwise the sending of write commands will be done in order. The
> feature is controlled by OUT_ORDER_WR_EN platform data flag.
> 
> 
> >>>
> >>> Cc: Matthias Brugger <matthias.bgg@gmail.com>
> >>> Suggested-by: Yong Wu <yong.wu@mediatek.com>
> >>> Signed-off-by: Chao Hao <chao.hao@mediatek.com>
> >>> ---
> >>>  drivers/iommu/mtk_iommu.c | 12 +++++++++++-
> >>>  drivers/iommu/mtk_iommu.h |  1 +
> >>>  2 files changed, 12 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> >>> index 8f81df6cbe51..67b46b5d83d9 100644
> >>> --- a/drivers/iommu/mtk_iommu.c
> >>> +++ b/drivers/iommu/mtk_iommu.c
> >>> @@ -42,6 +42,9 @@
> >>>  #define F_INVLD_EN1				BIT(1)
> >>>  
> >>>  #define REG_MMU_MISC_CTRL			0x048
> >>> +#define F_MMU_IN_ORDER_WR_EN			(BIT(1) | BIT(17))
> >>> +#define F_MMU_STANDARD_AXI_MODE_BIT		(BIT(3) | BIT(19))
> >>
> >> Wouldn't it make more sense to name it F_MMU_STANDARD_AXI_MODE_EN?
> > ok, you are right.
> > 1'b1: follow standard axi protocol
> > 
> 
> What about
> F_MMU_IN_ORDER_WR_EN_MASK
> F_MMU_STANDARD_AXI_MODE_EN_MASK
> 
> Background is that we have to set/unset two bits to enable or disable the
> feature, so it's a mask we have to apply to the register.
> 

ok, thanks for your advice

> Regards,
> Matthias
> 
> >>
> >>> +
> >>>  #define REG_MMU_DCM_DIS				0x050
> >>>  
> >>>  #define REG_MMU_CTRL_REG			0x110
> >>> @@ -574,10 +577,17 @@ static int mtk_iommu_hw_init(const struct mtk_iommu_data *data)
> >>>  	}
> >>>  	writel_relaxed(0, data->base + REG_MMU_DCM_DIS);
> >>>  
> >>> +	regval = readl_relaxed(data->base + REG_MMU_MISC_CTRL);
> >>
> >> We only need to read regval in the else branch.
> > 
> > ok, I got it. thanks
> > 
> >>
> >>>  	if (MTK_IOMMU_HAS_FLAG(data->plat_data, RESET_AXI)) {
> >>>  		/* The register is called STANDARD_AXI_MODE in this case */
> >>> -		writel_relaxed(0, data->base + REG_MMU_MISC_CTRL);
> >>> +		regval = 0;
> >>> +	} else {
> >>> +		/* For mm_iommu, it can improve performance by the setting */
> >>> +		regval &= ~F_MMU_STANDARD_AXI_MODE_BIT;
> >>> +		if (MTK_IOMMU_HAS_FLAG(data->plat_data, OUT_ORDER_EN))
> >>> +			regval &= ~F_MMU_IN_ORDER_WR_EN;
> >>>  	}
> >>> +	writel_relaxed(regval, data->base + REG_MMU_MISC_CTRL);
> >>>  
> >>>  	if (devm_request_irq(data->dev, data->irq, mtk_iommu_isr, 0,
> >>>  			     dev_name(data->dev), (void *)data)) {
> >>> diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
> >>> index 7cc39f729263..4b780b651ef4 100644
> >>> --- a/drivers/iommu/mtk_iommu.h
> >>> +++ b/drivers/iommu/mtk_iommu.h
> >>> @@ -22,6 +22,7 @@
> >>>  #define HAS_BCLK			BIT(1)
> >>>  #define HAS_VLD_PA_RNG			BIT(2)
> >>>  #define RESET_AXI			BIT(3)
> >>> +#define OUT_ORDER_EN			BIT(4)
> >>
> >> Maybe something like OUT_ORDER_WR_EN, to make clear that it's about the the
> >> write path.
> >>
> > ok, thanks for your advice.
> > 
> >>>  
> >>>  #define MTK_IOMMU_HAS_FLAG(pdata, _x) \
> >>>  		((((pdata)->flags) & (_x)) == (_x))
> >>>
> > 

Re: [PATCH v5 07/10] iommu/mediatek: Add REG_MMU_WR_LEN register definition

[Matthias Brugger]

On 29/06/2020 09:13, Chao Hao wrote:
> Some platforms(ex: mt6779) need to improve performance by setting
> REG_MMU_WR_LEN register. And we can use WR_THROT_EN macro to control
> whether we need to set the register. If the register uses default value,
> iommu will send command to EMI without restriction, when the number of
> commands become more and more, it will drop the EMI performance. So when
> more than ten_commands(default value) don't be handled for EMI, iommu will
> stop send command to EMI for keeping EMI's performace by enabling write
> throttling mechanism(bit[5][21]=0) in MMU_WR_LEN_CTRL register.
> 
> Cc: Matthias Brugger <matthias.bgg@gmail.com>
> Signed-off-by: Chao Hao <chao.hao@mediatek.com>
> ---
>  drivers/iommu/mtk_iommu.c | 10 ++++++++++
>  drivers/iommu/mtk_iommu.h |  2 ++
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index ec1f86913739..92316c4175a9 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -46,6 +46,8 @@
>  #define F_MMU_STANDARD_AXI_MODE_BIT		(BIT(3) | BIT(19))
>  
>  #define REG_MMU_DCM_DIS				0x050
> +#define REG_MMU_WR_LEN				0x054

The register name is confusing. For me it seems to describe the length of a
write but it is used for controlling the write throttling. Is this the name
that's used in the datasheet?

> +#define F_MMU_WR_THROT_DIS_BIT			(BIT(5) |  BIT(21))

There are two spaces between '|' and 'BIT(21)', should be one.

Regarding the name of the define, what does the 'F_' statnds for? Also I think
it should be called '_MASK' instead of '_BIT' as it defines a mask of bits.

Regards,
Matthias

>  
>  #define REG_MMU_CTRL_REG			0x110
>  #define F_MMU_TF_PROT_TO_PROGRAM_ADDR		(2 << 4)
> @@ -582,6 +584,12 @@ static int mtk_iommu_hw_init(const struct mtk_iommu_data *data)
>  		writel_relaxed(regval, data->base + REG_MMU_VLD_PA_RNG);
>  	}
>  	writel_relaxed(0, data->base + REG_MMU_DCM_DIS);
> +	if (MTK_IOMMU_HAS_FLAG(data->plat_data, WR_THROT_EN)) {
> +		/* write command throttling mode */
> +		regval = readl_relaxed(data->base + REG_MMU_WR_LEN);
> +		regval &= ~F_MMU_WR_THROT_DIS_BIT;
> +		writel_relaxed(regval, data->base + REG_MMU_WR_LEN);
> +	}
>  
>  	regval = readl_relaxed(data->base + REG_MMU_MISC_CTRL);
>  	if (MTK_IOMMU_HAS_FLAG(data->plat_data, RESET_AXI)) {
> @@ -737,6 +745,7 @@ static int __maybe_unused mtk_iommu_suspend(struct device *dev)
>  	struct mtk_iommu_suspend_reg *reg = &data->reg;
>  	void __iomem *base = data->base;
>  
> +	reg->wr_len = readl_relaxed(base + REG_MMU_WR_LEN);
>  	reg->misc_ctrl = readl_relaxed(base + REG_MMU_MISC_CTRL);
>  	reg->dcm_dis = readl_relaxed(base + REG_MMU_DCM_DIS);
>  	reg->ctrl_reg = readl_relaxed(base + REG_MMU_CTRL_REG);
> @@ -761,6 +770,7 @@ static int __maybe_unused mtk_iommu_resume(struct device *dev)
>  		dev_err(data->dev, "Failed to enable clk(%d) in resume\n", ret);
>  		return ret;
>  	}
> +	writel_relaxed(reg->wr_len, base + REG_MMU_WR_LEN);
>  	writel_relaxed(reg->misc_ctrl, base + REG_MMU_MISC_CTRL);
>  	writel_relaxed(reg->dcm_dis, base + REG_MMU_DCM_DIS);
>  	writel_relaxed(reg->ctrl_reg, base + REG_MMU_CTRL_REG);
> diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
> index be6d32ee5bda..ce4f4e8f03aa 100644
> --- a/drivers/iommu/mtk_iommu.h
> +++ b/drivers/iommu/mtk_iommu.h
> @@ -24,6 +24,7 @@
>  #define RESET_AXI			BIT(3)
>  #define OUT_ORDER_EN			BIT(4)
>  #define HAS_SUB_COMM			BIT(5)
> +#define WR_THROT_EN			BIT(6)
>  
>  #define MTK_IOMMU_HAS_FLAG(pdata, _x) \
>  		((((pdata)->flags) & (_x)) == (_x))
> @@ -36,6 +37,7 @@ struct mtk_iommu_suspend_reg {
>  	u32				int_main_control;
>  	u32				ivrp_paddr;
>  	u32				vld_pa_rng;
> +	u32				wr_len;
>  };
>  
>  enum mtk_iommu_plat {
> 

Re: [PATCH v5 07/10] iommu/mediatek: Add REG_MMU_WR_LEN register definition

[chao hao]

On Mon, 2020-06-29 at 12:16 +0200, Matthias Brugger wrote:
> 
> On 29/06/2020 09:13, Chao Hao wrote:
> > Some platforms(ex: mt6779) need to improve performance by setting
> > REG_MMU_WR_LEN register. And we can use WR_THROT_EN macro to control
> > whether we need to set the register. If the register uses default value,
> > iommu will send command to EMI without restriction, when the number of
> > commands become more and more, it will drop the EMI performance. So when
> > more than ten_commands(default value) don't be handled for EMI, iommu will
> > stop send command to EMI for keeping EMI's performace by enabling write
> > throttling mechanism(bit[5][21]=0) in MMU_WR_LEN_CTRL register.
> > 
> > Cc: Matthias Brugger <matthias.bgg@gmail.com>
> > Signed-off-by: Chao Hao <chao.hao@mediatek.com>
> > ---
> >  drivers/iommu/mtk_iommu.c | 10 ++++++++++
> >  drivers/iommu/mtk_iommu.h |  2 ++
> >  2 files changed, 12 insertions(+)
> > 
> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > index ec1f86913739..92316c4175a9 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> > @@ -46,6 +46,8 @@
> >  #define F_MMU_STANDARD_AXI_MODE_BIT		(BIT(3) | BIT(19))
> >  
> >  #define REG_MMU_DCM_DIS				0x050
> > +#define REG_MMU_WR_LEN				0x054
> 
> The register name is confusing. For me it seems to describe the length of a
> write but it is used for controlling the write throttling. Is this the name
> that's used in the datasheet?
> 

Thanks for your review carefully, we can name it to REG_MMU_WR_LEN_CTRL


> > +#define F_MMU_WR_THROT_DIS_BIT			(BIT(5) |  BIT(21))
> 
> There are two spaces between '|' and 'BIT(21)', should be one.
> 
> Regarding the name of the define, what does the 'F_' statnds for? 

F_ is used to described some bits in register and doesn't have other
meanings. The format is refer to other bits definition

> Also I think
> it should be called '_MASK' instead of '_BIT' as it defines a mask of bits.
> 

Thanks for your advice.
For F_MMU_WR_THROT_DIS_BIT:
1'b0: Enable write throttling mechanism
1'b1: Disable write throttling mechanism
So I think we can name "F_MMU_WR_THROT_DIS  BIT(5) | BIT(21)" directly,
it maybe more clearer.

> Regards,
> Matthias
> 
> >  
> >  #define REG_MMU_CTRL_REG			0x110
> >  #define F_MMU_TF_PROT_TO_PROGRAM_ADDR		(2 << 4)
> > @@ -582,6 +584,12 @@ static int mtk_iommu_hw_init(const struct mtk_iommu_data *data)
> >  		writel_relaxed(regval, data->base + REG_MMU_VLD_PA_RNG);
> >  	}
> >  	writel_relaxed(0, data->base + REG_MMU_DCM_DIS);
> > +	if (MTK_IOMMU_HAS_FLAG(data->plat_data, WR_THROT_EN)) {
> > +		/* write command throttling mode */
> > +		regval = readl_relaxed(data->base + REG_MMU_WR_LEN);
> > +		regval &= ~F_MMU_WR_THROT_DIS_BIT;
> > +		writel_relaxed(regval, data->base + REG_MMU_WR_LEN);
> > +	}
> >  
> >  	regval = readl_relaxed(data->base + REG_MMU_MISC_CTRL);
> >  	if (MTK_IOMMU_HAS_FLAG(data->plat_data, RESET_AXI)) {
> > @@ -737,6 +745,7 @@ static int __maybe_unused mtk_iommu_suspend(struct device *dev)
> >  	struct mtk_iommu_suspend_reg *reg = &data->reg;
> >  	void __iomem *base = data->base;
> >  
> > +	reg->wr_len = readl_relaxed(base + REG_MMU_WR_LEN);
> >  	reg->misc_ctrl = readl_relaxed(base + REG_MMU_MISC_CTRL);
> >  	reg->dcm_dis = readl_relaxed(base + REG_MMU_DCM_DIS);
> >  	reg->ctrl_reg = readl_relaxed(base + REG_MMU_CTRL_REG);
> > @@ -761,6 +770,7 @@ static int __maybe_unused mtk_iommu_resume(struct device *dev)
> >  		dev_err(data->dev, "Failed to enable clk(%d) in resume\n", ret);
> >  		return ret;
> >  	}
> > +	writel_relaxed(reg->wr_len, base + REG_MMU_WR_LEN);
> >  	writel_relaxed(reg->misc_ctrl, base + REG_MMU_MISC_CTRL);
> >  	writel_relaxed(reg->dcm_dis, base + REG_MMU_DCM_DIS);
> >  	writel_relaxed(reg->ctrl_reg, base + REG_MMU_CTRL_REG);
> > diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
> > index be6d32ee5bda..ce4f4e8f03aa 100644
> > --- a/drivers/iommu/mtk_iommu.h
> > +++ b/drivers/iommu/mtk_iommu.h
> > @@ -24,6 +24,7 @@
> >  #define RESET_AXI			BIT(3)
> >  #define OUT_ORDER_EN			BIT(4)
> >  #define HAS_SUB_COMM			BIT(5)
> > +#define WR_THROT_EN			BIT(6)
> >  
> >  #define MTK_IOMMU_HAS_FLAG(pdata, _x) \
> >  		((((pdata)->flags) & (_x)) == (_x))
> > @@ -36,6 +37,7 @@ struct mtk_iommu_suspend_reg {
> >  	u32				int_main_control;
> >  	u32				ivrp_paddr;
> >  	u32				vld_pa_rng;
> > +	u32				wr_len;
> >  };
> >  
> >  enum mtk_iommu_plat {
> > 

Re: [PATCH v5 07/10] iommu/mediatek: Add REG_MMU_WR_LEN register definition

[Matthias Brugger]

On 30/06/2020 12:59, chao hao wrote:
> On Mon, 2020-06-29 at 12:16 +0200, Matthias Brugger wrote:
>>
>> On 29/06/2020 09:13, Chao Hao wrote:
>>> Some platforms(ex: mt6779) need to improve performance by setting
>>> REG_MMU_WR_LEN register. And we can use WR_THROT_EN macro to control
>>> whether we need to set the register. If the register uses default value,
>>> iommu will send command to EMI without restriction, when the number of
>>> commands become more and more, it will drop the EMI performance. So when
>>> more than ten_commands(default value) don't be handled for EMI, iommu will
>>> stop send command to EMI for keeping EMI's performace by enabling write
>>> throttling mechanism(bit[5][21]=0) in MMU_WR_LEN_CTRL register.
>>>
>>> Cc: Matthias Brugger <matthias.bgg@gmail.com>
>>> Signed-off-by: Chao Hao <chao.hao@mediatek.com>
>>> ---
>>>  drivers/iommu/mtk_iommu.c | 10 ++++++++++
>>>  drivers/iommu/mtk_iommu.h |  2 ++
>>>  2 files changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
>>> index ec1f86913739..92316c4175a9 100644
>>> --- a/drivers/iommu/mtk_iommu.c
>>> +++ b/drivers/iommu/mtk_iommu.c
>>> @@ -46,6 +46,8 @@
>>>  #define F_MMU_STANDARD_AXI_MODE_BIT		(BIT(3) | BIT(19))
>>>  
>>>  #define REG_MMU_DCM_DIS				0x050
>>> +#define REG_MMU_WR_LEN				0x054
>>
>> The register name is confusing. For me it seems to describe the length of a
>> write but it is used for controlling the write throttling. Is this the name
>> that's used in the datasheet?
>>
> 
> Thanks for your review carefully, we can name it to REG_MMU_WR_LEN_CTRL

Yes, that's mbetter, thanks.

> 
> 
>>> +#define F_MMU_WR_THROT_DIS_BIT			(BIT(5) |  BIT(21))
>>
>> There are two spaces between '|' and 'BIT(21)', should be one.
>>
>> Regarding the name of the define, what does the 'F_' statnds for? 
> 
> F_ is used to described some bits in register and doesn't have other
> meanings. The format is refer to other bits definition
> 
>> Also I think
>> it should be called '_MASK' instead of '_BIT' as it defines a mask of bits.
>>
> 
> Thanks for your advice.
> For F_MMU_WR_THROT_DIS_BIT:
> 1'b0: Enable write throttling mechanism
> 1'b1: Disable write throttling mechanism
> So I think we can name "F_MMU_WR_THROT_DIS  BIT(5) | BIT(21)" directly,
> it maybe more clearer.
> 

Is this what the datasheet names it? If not then I'd prefer F_MMU_WR_THROT_DIS_MASK.

Regards,
Matthias

>> Regards,
>> Matthias
>>
>>>  
>>>  #define REG_MMU_CTRL_REG			0x110
>>>  #define F_MMU_TF_PROT_TO_PROGRAM_ADDR		(2 << 4)
>>> @@ -582,6 +584,12 @@ static int mtk_iommu_hw_init(const struct mtk_iommu_data *data)
>>>  		writel_relaxed(regval, data->base + REG_MMU_VLD_PA_RNG);
>>>  	}
>>>  	writel_relaxed(0, data->base + REG_MMU_DCM_DIS);
>>> +	if (MTK_IOMMU_HAS_FLAG(data->plat_data, WR_THROT_EN)) {
>>> +		/* write command throttling mode */
>>> +		regval = readl_relaxed(data->base + REG_MMU_WR_LEN);
>>> +		regval &= ~F_MMU_WR_THROT_DIS_BIT;
>>> +		writel_relaxed(regval, data->base + REG_MMU_WR_LEN);
>>> +	}
>>>  
>>>  	regval = readl_relaxed(data->base + REG_MMU_MISC_CTRL);
>>>  	if (MTK_IOMMU_HAS_FLAG(data->plat_data, RESET_AXI)) {
>>> @@ -737,6 +745,7 @@ static int __maybe_unused mtk_iommu_suspend(struct device *dev)
>>>  	struct mtk_iommu_suspend_reg *reg = &data->reg;
>>>  	void __iomem *base = data->base;
>>>  
>>> +	reg->wr_len = readl_relaxed(base + REG_MMU_WR_LEN);
>>>  	reg->misc_ctrl = readl_relaxed(base + REG_MMU_MISC_CTRL);
>>>  	reg->dcm_dis = readl_relaxed(base + REG_MMU_DCM_DIS);
>>>  	reg->ctrl_reg = readl_relaxed(base + REG_MMU_CTRL_REG);
>>> @@ -761,6 +770,7 @@ static int __maybe_unused mtk_iommu_resume(struct device *dev)
>>>  		dev_err(data->dev, "Failed to enable clk(%d) in resume\n", ret);
>>>  		return ret;
>>>  	}
>>> +	writel_relaxed(reg->wr_len, base + REG_MMU_WR_LEN);
>>>  	writel_relaxed(reg->misc_ctrl, base + REG_MMU_MISC_CTRL);
>>>  	writel_relaxed(reg->dcm_dis, base + REG_MMU_DCM_DIS);
>>>  	writel_relaxed(reg->ctrl_reg, base + REG_MMU_CTRL_REG);
>>> diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
>>> index be6d32ee5bda..ce4f4e8f03aa 100644
>>> --- a/drivers/iommu/mtk_iommu.h
>>> +++ b/drivers/iommu/mtk_iommu.h
>>> @@ -24,6 +24,7 @@
>>>  #define RESET_AXI			BIT(3)
>>>  #define OUT_ORDER_EN			BIT(4)
>>>  #define HAS_SUB_COMM			BIT(5)
>>> +#define WR_THROT_EN			BIT(6)
>>>  
>>>  #define MTK_IOMMU_HAS_FLAG(pdata, _x) \
>>>  		((((pdata)->flags) & (_x)) == (_x))
>>> @@ -36,6 +37,7 @@ struct mtk_iommu_suspend_reg {
>>>  	u32				int_main_control;
>>>  	u32				ivrp_paddr;
>>>  	u32				vld_pa_rng;
>>> +	u32				wr_len;
>>>  };
>>>  
>>>  enum mtk_iommu_plat {
>>>
> 

Re: [PATCH v5 08/10] iommu/mediatek: Extend protect pa alignment value

[Matthias Brugger]

On 29/06/2020 09:13, Chao Hao wrote:
> Starting with mt6779, iommu needs to extend to 256 bytes from 128
> bytes which can send the max number of data for memory protection
> pa alignment. So we can use a separate patch to modify it.
> 
> Suggested-by: Matthias Brugger <matthias.bgg@gmail.com>
> Signed-off-by: Chao Hao <chao.hao@mediatek.com>

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

> ---
>  drivers/iommu/mtk_iommu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 92316c4175a9..8299a3299090 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -98,7 +98,7 @@
>  #define F_MMU_INT_ID_LARB_ID(a)			(((a) >> 7) & 0x7)
>  #define F_MMU_INT_ID_PORT_ID(a)			(((a) >> 2) & 0x1f)
>  
> -#define MTK_PROTECT_PA_ALIGN			128
> +#define MTK_PROTECT_PA_ALIGN			256
>  
>  /*
>   * Get the local arbiter ID and the portid within the larb arbiter
> 

Re: [PATCH v5 09/10] iommu/mediatek: Modify MMU_CTRL register setting

[Matthias Brugger]

On 29/06/2020 09:13, Chao Hao wrote:
> MT8173 is different from other SoCs for MMU_CTRL register.
> For mt8173, its bit9 is in_order_write_en and doesn't use its
> default 1'b1.> For other SoCs, bit[12] represents victim_tlb_en feature and
> victim_tlb is enable defaultly(bit[12]=1), if we use
> "regval = F_MMU_TF_PROT_TO_PROGRAM_ADDR", victim_tlb will be
> disabled, it will drop iommu performace.
> So we need to deal with the setting of MMU_CTRL separately
> for mt8173 and others.
> 

My proposal to rewrite the commit message:

The MMU_CTRL regiser of MT8173 is different from other SoCs. The in_order_wr_en
is bit[9] which is zero by default.
Other SoCs have the vitcim_tlb_en feature mapped to bit[12]. This bit is set to
one by default. We need to preserve the bit when setting
F_MMU_TF_PROT_TO_PROGRAM_ADDR as otherwise the bit will be cleared and IOMMU
performance will drop.


> Suggested-by: Matthias Brugger <matthias.bgg@gmail.com>
> Suggested-by: Yong Wu <yong.wu@mediatek.com>
> Signed-off-by: Chao Hao <chao.hao@mediatek.com>
> ---
>  drivers/iommu/mtk_iommu.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index 8299a3299090..e46e2deee3fd 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -543,11 +543,12 @@ static int mtk_iommu_hw_init(const struct mtk_iommu_data *data)
>  		return ret;
>  	}
>  
> +	regval = readl_relaxed(data->base + REG_MMU_CTRL_REG);

The read is only needed in the else branch.

>  	if (data->plat_data->m4u_plat == M4U_MT8173)
>  		regval = F_MMU_PREFETCH_RT_REPLACE_MOD |
>  			 F_MMU_TF_PROT_TO_PROGRAM_ADDR_MT8173;
>  	else
> -		regval = F_MMU_TF_PROT_TO_PROGRAM_ADDR;
> +		regval |= F_MMU_TF_PROT_TO_PROGRAM_ADDR;
>  	writel_relaxed(regval, data->base + REG_MMU_CTRL_REG);
>  
>  	regval = F_L2_MULIT_HIT_EN |
> 

Re: [PATCH v5 09/10] iommu/mediatek: Modify MMU_CTRL register setting

[chao hao]

On Mon, 2020-06-29 at 12:28 +0200, Matthias Brugger wrote:
> 
> On 29/06/2020 09:13, Chao Hao wrote:
> > MT8173 is different from other SoCs for MMU_CTRL register.
> > For mt8173, its bit9 is in_order_write_en and doesn't use its
> > default 1'b1.> For other SoCs, bit[12] represents victim_tlb_en feature and
> > victim_tlb is enable defaultly(bit[12]=1), if we use
> > "regval = F_MMU_TF_PROT_TO_PROGRAM_ADDR", victim_tlb will be
> > disabled, it will drop iommu performace.
> > So we need to deal with the setting of MMU_CTRL separately
> > for mt8173 and others.
> > 
> 
> My proposal to rewrite the commit message:
> 
> The MMU_CTRL regiser of MT8173 is different from other SoCs. The in_order_wr_en
> is bit[9] which is zero by default.
> Other SoCs have the vitcim_tlb_en feature mapped to bit[12]. This bit is set to
> one by default. We need to preserve the bit when setting
> F_MMU_TF_PROT_TO_PROGRAM_ADDR as otherwise the bit will be cleared and IOMMU
> performance will drop.

got it, thanks for your advice very much.

> 
> 
> > Suggested-by: Matthias Brugger <matthias.bgg@gmail.com>
> > Suggested-by: Yong Wu <yong.wu@mediatek.com>
> > Signed-off-by: Chao Hao <chao.hao@mediatek.com>
> > ---
> >  drivers/iommu/mtk_iommu.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > index 8299a3299090..e46e2deee3fd 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> > @@ -543,11 +543,12 @@ static int mtk_iommu_hw_init(const struct mtk_iommu_data *data)
> >  		return ret;
> >  	}
> >  
> > +	regval = readl_relaxed(data->base + REG_MMU_CTRL_REG);
> 
> The read is only needed in the else branch.
> 
ok, thanks

> >  	if (data->plat_data->m4u_plat == M4U_MT8173)
> >  		regval = F_MMU_PREFETCH_RT_REPLACE_MOD |
> >  			 F_MMU_TF_PROT_TO_PROGRAM_ADDR_MT8173;
> >  	else
> > -		regval = F_MMU_TF_PROT_TO_PROGRAM_ADDR;
> > +		regval |= F_MMU_TF_PROT_TO_PROGRAM_ADDR;
> >  	writel_relaxed(regval, data->base + REG_MMU_CTRL_REG);
> >  
> >  	regval = F_L2_MULIT_HIT_EN |
> > 

Re: [PATCH v5 10/10] iommu/mediatek: Add mt6779 basic support

[Matthias Brugger]

On 29/06/2020 09:13, Chao Hao wrote:
> 1. Start from mt6779, INVLDT_SEL move to offset=0x2c, so we add
>    REG_MMU_INV_SEL_GEN2 definition and mt6779 uses it.
> 2. Add mt6779_data to support mm_iommu HW init.
> 
> Cc: Yong Wu <yong.wu@mediatek.com>
> Cc: Matthias Brugger <matthias.bgg@gmail.com>
> Signed-off-by: Chao Hao <chao.hao@mediatek.com>

Reviewed by: Matthias Brugger <matthias.bgg@gmail.com>

> ---
>  drivers/iommu/mtk_iommu.c | 11 +++++++++++
>  drivers/iommu/mtk_iommu.h |  1 +
>  2 files changed, 12 insertions(+)
> 
> diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> index e46e2deee3fd..1575196d9cd5 100644
> --- a/drivers/iommu/mtk_iommu.c
> +++ b/drivers/iommu/mtk_iommu.c
> @@ -37,6 +37,7 @@
>  #define REG_MMU_INVLD_START_A			0x024
>  #define REG_MMU_INVLD_END_A			0x028
>  
> +#define REG_MMU_INV_SEL_GEN2			0x02c
>  #define REG_MMU_INV_SEL_GEN1			0x038
>  #define F_INVLD_EN0				BIT(0)
>  #define F_INVLD_EN1				BIT(1)
> @@ -798,6 +799,15 @@ static const struct mtk_iommu_plat_data mt2712_data = {
>  	.larbid_remap = {{0}, {1}, {2}, {3}, {4}, {5}, {6}, {7}},
>  };
>  
> +static const struct mtk_iommu_plat_data mt6779_data = {
> +	.m4u_plat      = M4U_MT6779,
> +	.flags         = HAS_SUB_COMM |
> +			 OUT_ORDER_EN |
> +			 WR_THROT_EN,
> +	.inv_sel_reg   = REG_MMU_INV_SEL_GEN2,
> +	.larbid_remap  = {{0}, {1}, {2}, {3}, {5}, {7, 8}, {10}, {9}},
> +};
> +
>  static const struct mtk_iommu_plat_data mt8173_data = {
>  	.m4u_plat     = M4U_MT8173,
>  	.flags	      = HAS_4GB_MODE |
> @@ -816,6 +826,7 @@ static const struct mtk_iommu_plat_data mt8183_data = {
>  
>  static const struct of_device_id mtk_iommu_of_ids[] = {
>  	{ .compatible = "mediatek,mt2712-m4u", .data = &mt2712_data},
> +	{ .compatible = "mediatek,mt6779-m4u", .data = &mt6779_data},
>  	{ .compatible = "mediatek,mt8173-m4u", .data = &mt8173_data},
>  	{ .compatible = "mediatek,mt8183-m4u", .data = &mt8183_data},
>  	{}
> diff --git a/drivers/iommu/mtk_iommu.h b/drivers/iommu/mtk_iommu.h
> index ce4f4e8f03aa..a080db2e8a93 100644
> --- a/drivers/iommu/mtk_iommu.h
> +++ b/drivers/iommu/mtk_iommu.h
> @@ -43,6 +43,7 @@ struct mtk_iommu_suspend_reg {
>  enum mtk_iommu_plat {
>  	M4U_MT2701,
>  	M4U_MT2712,
> +	M4U_MT6779,
>  	M4U_MT8173,
>  	M4U_MT8183,
>  };
> 

Re: [PATCH v5 06/10] iommu/mediatek: Add sub_comm id in translation fault

[Yong Wu]

On Mon, 2020-06-29 at 15:13 +0800, Chao Hao wrote:
> The max larb number that a iommu HW support is 8(larb0~larb7 in the below
> diagram).
> If the larb's number is over 8, we use a sub_common for merging
> several larbs into one larb. At this case, we will extend larb_id:
> bit[11:9] means common-id;
> bit[8:7] means subcommon-id;
> From these two variables, we could get the real larb number when
> translation fault happen.
> The diagram is as below:
> 		 EMI
> 		  |
> 		IOMMU
> 		  |
>            -----------------
> 	   |               |
> 	common1   	common0
> 	   |		   |
> 	   -----------------
> 		  |
>              smi common
> 		  |
>   ------------------------------------
>   |       |       |       |     |    |
>  3'd0    3'd1    3'd2    3'd3  ...  3'd7   <-common_id(max is 8)
>   |       |       |       |     |    |
> Larb0   Larb1     |     Larb3  ... Larb7
> 		  |
> 	    smi sub common
> 		  |
>      --------------------------
>      |        |       |       |
>     2'd0     2'd1    2'd2    2'd3   <-sub_common_id(max is 4)
>      |        |       |       |
>    Larb8    Larb9   Larb10  Larb11
> 
> In this patch we extend larb_remap[] to larb_remap[8][4] for this.
> larb_remap[x][y]: x means common-id above, y means subcommon_id above.
> 
> We can also distinguish if the M4U HW has sub_common by HAS_SUB_COMM
> macro.
> 
> Cc: Matthias Brugger <matthias.bgg@gmail.com>
> Signed-off-by: Chao Hao <chao.hao@mediatek.com>
> Reviewed-by: Yong Wu <yong.wu@mediatek.com>
> ---
>  drivers/iommu/mtk_iommu.c  | 20 +++++++++++++-------
>  drivers/iommu/mtk_iommu.h  |  3 ++-
>  include/soc/mediatek/smi.h |  2 ++
>  3 files changed, 17 insertions(+), 8 deletions(-)

[snip]

> @@ -48,7 +49,7 @@ struct mtk_iommu_plat_data {
>  	enum mtk_iommu_plat m4u_plat;
>  	u32                 flags;
>  	u32                 inv_sel_reg;
> -	unsigned char       larbid_remap[MTK_LARB_NR_MAX];
> +	unsigned char       larbid_remap[MTK_LARB_COM_MAX][MTK_LARB_SUBCOM_MAX];
>  };
>  
>  struct mtk_iommu_domain;
> diff --git a/include/soc/mediatek/smi.h b/include/soc/mediatek/smi.h
> index 5a34b87d89e3..fa65a55468e2 100644
> --- a/include/soc/mediatek/smi.h
> +++ b/include/soc/mediatek/smi.h
> @@ -12,6 +12,8 @@
>  #ifdef CONFIG_MTK_SMI
>  
>  #define MTK_LARB_NR_MAX		16
> +#define MTK_LARB_COM_MAX	8
> +#define MTK_LARB_SUBCOM_MAX	4

Both are only used in mtk_iommu.h, and I don't think smi has plan to use
them. thus we could move them into mtk_iommu.h

>  
>  #define MTK_SMI_MMU_EN(port)	BIT(port)
>  

Re: [PATCH v5 06/10] iommu/mediatek: Add sub_comm id in translation fault

[chao hao]

On Tue, 2020-06-30 at 18:55 +0800, Yong Wu wrote:
> On Mon, 2020-06-29 at 15:13 +0800, Chao Hao wrote:
> > The max larb number that a iommu HW support is 8(larb0~larb7 in the below
> > diagram).
> > If the larb's number is over 8, we use a sub_common for merging
> > several larbs into one larb. At this case, we will extend larb_id:
> > bit[11:9] means common-id;
> > bit[8:7] means subcommon-id;
> > From these two variables, we could get the real larb number when
> > translation fault happen.
> > The diagram is as below:
> > 		 EMI
> > 		  |
> > 		IOMMU
> > 		  |
> >            -----------------
> > 	   |               |
> > 	common1   	common0
> > 	   |		   |
> > 	   -----------------
> > 		  |
> >              smi common
> > 		  |
> >   ------------------------------------
> >   |       |       |       |     |    |
> >  3'd0    3'd1    3'd2    3'd3  ...  3'd7   <-common_id(max is 8)
> >   |       |       |       |     |    |
> > Larb0   Larb1     |     Larb3  ... Larb7
> > 		  |
> > 	    smi sub common
> > 		  |
> >      --------------------------
> >      |        |       |       |
> >     2'd0     2'd1    2'd2    2'd3   <-sub_common_id(max is 4)
> >      |        |       |       |
> >    Larb8    Larb9   Larb10  Larb11
> > 
> > In this patch we extend larb_remap[] to larb_remap[8][4] for this.
> > larb_remap[x][y]: x means common-id above, y means subcommon_id above.
> > 
> > We can also distinguish if the M4U HW has sub_common by HAS_SUB_COMM
> > macro.
> > 
> > Cc: Matthias Brugger <matthias.bgg@gmail.com>
> > Signed-off-by: Chao Hao <chao.hao@mediatek.com>
> > Reviewed-by: Yong Wu <yong.wu@mediatek.com>
> > ---
> >  drivers/iommu/mtk_iommu.c  | 20 +++++++++++++-------
> >  drivers/iommu/mtk_iommu.h  |  3 ++-
> >  include/soc/mediatek/smi.h |  2 ++
> >  3 files changed, 17 insertions(+), 8 deletions(-)
> 
> [snip]
> 
> > @@ -48,7 +49,7 @@ struct mtk_iommu_plat_data {
> >  	enum mtk_iommu_plat m4u_plat;
> >  	u32                 flags;
> >  	u32                 inv_sel_reg;
> > -	unsigned char       larbid_remap[MTK_LARB_NR_MAX];
> > +	unsigned char       larbid_remap[MTK_LARB_COM_MAX][MTK_LARB_SUBCOM_MAX];
> >  };
> >  
> >  struct mtk_iommu_domain;
> > diff --git a/include/soc/mediatek/smi.h b/include/soc/mediatek/smi.h
> > index 5a34b87d89e3..fa65a55468e2 100644
> > --- a/include/soc/mediatek/smi.h
> > +++ b/include/soc/mediatek/smi.h
> > @@ -12,6 +12,8 @@
> >  #ifdef CONFIG_MTK_SMI
> >  
> >  #define MTK_LARB_NR_MAX		16
> > +#define MTK_LARB_COM_MAX	8
> > +#define MTK_LARB_SUBCOM_MAX	4
> 
> Both are only used in mtk_iommu.h, and I don't think smi has plan to use
> them. thus we could move them into mtk_iommu.h
> 
ok, got it. Thanks for your advice.

> >  
> >  #define MTK_SMI_MMU_EN(port)	BIT(port)
> >  
> 
>