Re: [PATCH 7/8] memory: mtk-smi: Rearrange some function position alphabetically

[Yong Wu]

On Fri, 2017-08-11 at 19:09 +0100, Robin Murphy wrote:
> On 11/08/17 10:56, Yong Wu wrote:
> > Only adjust some code position in Soc numerical order, from mt2701,
> > mt2712 to mt8173.
> > 
> > Besides, 3 minor changes:
> > 1) fix a coding style issue:
> > CHECK: Alignment should match open parenthesis
> > +   writel(reg_val,
> > +   common->smi_ao_base
> > 2) change from readl to readl_relaxed in gen1_config_port.
> > 3) change the type "larbid" from "int" to "unsigned int" to meet
> >the requirement of of_property_read_u32.
> 
> If moving existing code around is really necessary, do it as a
> preliminary patch *before* any material changes (and arguably separate
> even from the whitespace and comment updates) - those diffs are usually
> hard to review as-is, so being able to check you get binary-identical
> object files before and after is reassuring. A "cleanup" patch shouldn't
> need to touch code added in the same series, and it certainly shouldn't
> have significant things like the readl_relaxed() change hidden in it.

OK. This patch is really not easy to read.

This patch-set is mainly for supporting MT2712 IOMMU. thus, I will use a
patch "readl_relaxed replace readl" instead of this one.

About the cleanup patch, I can send it in future if necessary.

> 
> Robin.
> 
> > Signed-off-by: Yong Wu 
> > ---
> >  drivers/memory/mtk-smi.c | 105 
> > +++
> >  1 file changed, 52 insertions(+), 53 deletions(-)
> > 
[...]

Re: [PATCH 7/8] memory: mtk-smi: Rearrange some function position alphabetically

[Yong Wu]

On Fri, 2017-08-11 at 19:09 +0100, Robin Murphy wrote:
> On 11/08/17 10:56, Yong Wu wrote:
> > Only adjust some code position in Soc numerical order, from mt2701,
> > mt2712 to mt8173.
> > 
> > Besides, 3 minor changes:
> > 1) fix a coding style issue:
> > CHECK: Alignment should match open parenthesis
> > +   writel(reg_val,
> > +   common->smi_ao_base
> > 2) change from readl to readl_relaxed in gen1_config_port.
> > 3) change the type "larbid" from "int" to "unsigned int" to meet
> >the requirement of of_property_read_u32.
> 
> If moving existing code around is really necessary, do it as a
> preliminary patch *before* any material changes (and arguably separate
> even from the whitespace and comment updates) - those diffs are usually
> hard to review as-is, so being able to check you get binary-identical
> object files before and after is reassuring. A "cleanup" patch shouldn't
> need to touch code added in the same series, and it certainly shouldn't
> have significant things like the readl_relaxed() change hidden in it.

OK. This patch is really not easy to read.

This patch-set is mainly for supporting MT2712 IOMMU. thus, I will use a
patch "readl_relaxed replace readl" instead of this one.

About the cleanup patch, I can send it in future if necessary.

> 
> Robin.
> 
> > Signed-off-by: Yong Wu 
> > ---
> >  drivers/memory/mtk-smi.c | 105 
> > +++
> >  1 file changed, 52 insertions(+), 53 deletions(-)
> > 
[...]

Re: [PATCH 7/8] memory: mtk-smi: Rearrange some function position alphabetically

[Robin Murphy]

On 11/08/17 10:56, Yong Wu wrote:
> Only adjust some code position in Soc numerical order, from mt2701,
> mt2712 to mt8173.
> 
> Besides, 3 minor changes:
> 1) fix a coding style issue:
> CHECK: Alignment should match open parenthesis
> +   writel(reg_val,
> +   common->smi_ao_base
> 2) change from readl to readl_relaxed in gen1_config_port.
> 3) change the type "larbid" from "int" to "unsigned int" to meet
>the requirement of of_property_read_u32.

If moving existing code around is really necessary, do it as a
preliminary patch *before* any material changes (and arguably separate
even from the whitespace and comment updates) - those diffs are usually
hard to review as-is, so being able to check you get binary-identical
object files before and after is reassuring. A "cleanup" patch shouldn't
need to touch code added in the same series, and it certainly shouldn't
have significant things like the readl_relaxed() change hidden in it.

Robin.

> Signed-off-by: Yong Wu 
> ---
>  drivers/memory/mtk-smi.c | 105 
> +++
>  1 file changed, 52 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
> index ec06d2b..ce27bdf 100644
> --- a/drivers/memory/mtk-smi.c
> +++ b/drivers/memory/mtk-smi.c
> @@ -23,9 +23,6 @@
>  #include 
>  #include 
>  
> -/* mt8173 */
> -#define SMI_LARB_MMU_EN  0xf00
> -
>  /* mt2701 */
>  #define REG_SMI_SECUR_CON_BASE   0x5c0
>  
> @@ -48,16 +45,19 @@
>  #define SMI_LARB_NONSEC_CON(id)  (0x380 + (id * 4))
>  #define F_MMU_EN BIT(0)
>  
> +/* mt8173 */
> +#define SMI_LARB_MMU_EN  0xf00
> +
>  struct mtk_smi_larb_gen {
> - bool need_larbid;
> - int port_in_larb[MTK_LARB_NR_MAX + 1];
> - void (*config_port)(struct device *);
> + boolneed_larbid;
> + int port_in_larb[MTK_LARB_NR_MAX + 1];/* Only needed by smi gen1 */
> + void(*config_port)(struct device *);
>  };
>  
>  struct mtk_smi {
>   struct device   *dev;
>   struct clk  *clk_apb, *clk_smi;
> - struct clk  *clk_async; /*only needed by mt2701*/
> + struct clk  *clk_async; /*only needed by smi gen1*/
>   void __iomem*smi_ao_base;
>  };
>  
> @@ -66,7 +66,7 @@ struct mtk_smi_larb { /* larb: local arbiter */
>   void __iomem*base;
>   struct device   *smi_common_dev;
>   const struct mtk_smi_larb_gen   *larb_gen;
> - int larbid;
> + unsigned intlarbid;
>   u32 *mmu;
>  };
>  
> @@ -166,28 +166,16 @@ void mtk_smi_larb_put(struct device *larbdev)
>   return -ENODEV;
>  }
>  
> -static void mtk_smi_larb_config_port_mt8173(struct device *dev)
> +static void
> +mtk_smi_larb_unbind(struct device *dev, struct device *master, void *data)
>  {
> - struct mtk_smi_larb *larb = dev_get_drvdata(dev);
> -
> - writel(*larb->mmu, larb->base + SMI_LARB_MMU_EN);
> + /* Do nothing as the iommu is always enabled. */
>  }
>  
> -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)) {
> - reg = readl_relaxed(larb->base +
> - SMI_LARB_NONSEC_CON(i));
> - reg |= F_MMU_EN;
> - writel(reg, larb->base + SMI_LARB_NONSEC_CON(i));
> - }
> - }
> -}
> +static const struct component_ops mtk_smi_larb_component_ops = {
> + .bind = mtk_smi_larb_bind,
> + .unbind = mtk_smi_larb_unbind,
> +};
>  
>  static void mtk_smi_larb_config_port_gen1(struct device *dev)
>  {
> @@ -209,36 +197,39 @@ static void mtk_smi_larb_config_port_gen1(struct device 
> *dev)
>   /* do not need to enable m4u for this port */
>   continue;
>   }
> - reg_val = readl(common->smi_ao_base
> + reg_val = readl_relaxed(common->smi_ao_base
>   + REG_SMI_SECUR_CON_ADDR(m4u_port_id));
>   reg_val &= SMI_SECUR_CON_VAL_MSK(m4u_port_id);
>   reg_val |= sec_con_val;
>   reg_val |= SMI_SECUR_CON_VAL_DOMAIN(m4u_port_id);
>   writel(reg_val,
> - common->smi_ao_base
> - + REG_SMI_SECUR_CON_ADDR(m4u_port_id));
> +common->smi_ao_base +
> +REG_SMI_SECUR_CON_ADDR(m4u_port_id));
>   }
>  }
>  
> -static void
> -mtk_smi_larb_unbind(struct device *dev, struct device *master, void *data)
> +static void mtk_smi_larb_config_port_mt2712(struct device *dev)
>  {
> - /* Do nothing as the iommu is 

Re: [PATCH 7/8] memory: mtk-smi: Rearrange some function position alphabetically

[Robin Murphy]

On 11/08/17 10:56, Yong Wu wrote:
> Only adjust some code position in Soc numerical order, from mt2701,
> mt2712 to mt8173.
> 
> Besides, 3 minor changes:
> 1) fix a coding style issue:
> CHECK: Alignment should match open parenthesis
> +   writel(reg_val,
> +   common->smi_ao_base
> 2) change from readl to readl_relaxed in gen1_config_port.
> 3) change the type "larbid" from "int" to "unsigned int" to meet
>the requirement of of_property_read_u32.

If moving existing code around is really necessary, do it as a
preliminary patch *before* any material changes (and arguably separate
even from the whitespace and comment updates) - those diffs are usually
hard to review as-is, so being able to check you get binary-identical
object files before and after is reassuring. A "cleanup" patch shouldn't
need to touch code added in the same series, and it certainly shouldn't
have significant things like the readl_relaxed() change hidden in it.

Robin.

> Signed-off-by: Yong Wu 
> ---
>  drivers/memory/mtk-smi.c | 105 
> +++
>  1 file changed, 52 insertions(+), 53 deletions(-)
> 
> diff --git a/drivers/memory/mtk-smi.c b/drivers/memory/mtk-smi.c
> index ec06d2b..ce27bdf 100644
> --- a/drivers/memory/mtk-smi.c
> +++ b/drivers/memory/mtk-smi.c
> @@ -23,9 +23,6 @@
>  #include 
>  #include 
>  
> -/* mt8173 */
> -#define SMI_LARB_MMU_EN  0xf00
> -
>  /* mt2701 */
>  #define REG_SMI_SECUR_CON_BASE   0x5c0
>  
> @@ -48,16 +45,19 @@
>  #define SMI_LARB_NONSEC_CON(id)  (0x380 + (id * 4))
>  #define F_MMU_EN BIT(0)
>  
> +/* mt8173 */
> +#define SMI_LARB_MMU_EN  0xf00
> +
>  struct mtk_smi_larb_gen {
> - bool need_larbid;
> - int port_in_larb[MTK_LARB_NR_MAX + 1];
> - void (*config_port)(struct device *);
> + boolneed_larbid;
> + int port_in_larb[MTK_LARB_NR_MAX + 1];/* Only needed by smi gen1 */
> + void(*config_port)(struct device *);
>  };
>  
>  struct mtk_smi {
>   struct device   *dev;
>   struct clk  *clk_apb, *clk_smi;
> - struct clk  *clk_async; /*only needed by mt2701*/
> + struct clk  *clk_async; /*only needed by smi gen1*/
>   void __iomem*smi_ao_base;
>  };
>  
> @@ -66,7 +66,7 @@ struct mtk_smi_larb { /* larb: local arbiter */
>   void __iomem*base;
>   struct device   *smi_common_dev;
>   const struct mtk_smi_larb_gen   *larb_gen;
> - int larbid;
> + unsigned intlarbid;
>   u32 *mmu;
>  };
>  
> @@ -166,28 +166,16 @@ void mtk_smi_larb_put(struct device *larbdev)
>   return -ENODEV;
>  }
>  
> -static void mtk_smi_larb_config_port_mt8173(struct device *dev)
> +static void
> +mtk_smi_larb_unbind(struct device *dev, struct device *master, void *data)
>  {
> - struct mtk_smi_larb *larb = dev_get_drvdata(dev);
> -
> - writel(*larb->mmu, larb->base + SMI_LARB_MMU_EN);
> + /* Do nothing as the iommu is always enabled. */
>  }
>  
> -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)) {
> - reg = readl_relaxed(larb->base +
> - SMI_LARB_NONSEC_CON(i));
> - reg |= F_MMU_EN;
> - writel(reg, larb->base + SMI_LARB_NONSEC_CON(i));
> - }
> - }
> -}
> +static const struct component_ops mtk_smi_larb_component_ops = {
> + .bind = mtk_smi_larb_bind,
> + .unbind = mtk_smi_larb_unbind,
> +};
>  
>  static void mtk_smi_larb_config_port_gen1(struct device *dev)
>  {
> @@ -209,36 +197,39 @@ static void mtk_smi_larb_config_port_gen1(struct device 
> *dev)
>   /* do not need to enable m4u for this port */
>   continue;
>   }
> - reg_val = readl(common->smi_ao_base
> + reg_val = readl_relaxed(common->smi_ao_base
>   + REG_SMI_SECUR_CON_ADDR(m4u_port_id));
>   reg_val &= SMI_SECUR_CON_VAL_MSK(m4u_port_id);
>   reg_val |= sec_con_val;
>   reg_val |= SMI_SECUR_CON_VAL_DOMAIN(m4u_port_id);
>   writel(reg_val,
> - common->smi_ao_base
> - + REG_SMI_SECUR_CON_ADDR(m4u_port_id));
> +common->smi_ao_base +
> +REG_SMI_SECUR_CON_ADDR(m4u_port_id));
>   }
>  }
>  
> -static void
> -mtk_smi_larb_unbind(struct device *dev, struct device *master, void *data)
> +static void mtk_smi_larb_config_port_mt2712(struct device *dev)
>  {
> - /* Do nothing as the iommu is always enabled. */
>