| Message ID | 20200629071310.1557-8-chao.hao@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | MT6779 IOMMU SUPPORT | expand |
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 { >
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 { > >
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 { >>> >
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 +#define F_MMU_WR_THROT_DIS_BIT (BIT(5) | BIT(21)) #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 {
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(+)