Patchwork [1/2] ARM: mx51/53: correct misuse of _clk_max_enable and _clk_max_disable

login
register
mail settings
Submitter Richard Zhao
Date Nov. 7, 2011, 3:02 a.m.
Message ID <1320634936-7898-1-git-send-email-richard.zhao@linaro.org>
Download mbox | patch
Permalink /patch/123993/
State New
Headers show

Comments

Richard Zhao - Nov. 7, 2011, 3:02 a.m.
Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
---
 arch/arm/mach-mx5/clock-mx51-mx53.c |   70 +++++++++++++++++++---------------
 1 files changed, 39 insertions(+), 31 deletions(-)
Richard Zhao - Nov. 17, 2011, 1:26 a.m.
Hi Sascha,

Would you merge this patch? The second patch in this series can be dropped.

Thanks
Richard
Sascha Hauer - Nov. 17, 2011, 6:18 p.m.
On Mon, Nov 07, 2011 at 11:02:15AM +0800, Richard Zhao wrote:
> Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> ---
>  arch/arm/mach-mx5/clock-mx51-mx53.c |   70 +++++++++++++++++++---------------
>  1 files changed, 39 insertions(+), 31 deletions(-)

What exactly is the misuse of clk_max_enable/disable you are talking
about? What does this fix? Why is a clk_round_rate involved?
This patch seems to do more than the subject says. You should split
it and improve the description.

Sascha

> 
> diff --git a/arch/arm/mach-mx5/clock-mx51-mx53.c b/arch/arm/mach-mx5/clock-mx51-mx53.c
> index a3ca4ce..507d24c 100644
> --- a/arch/arm/mach-mx5/clock-mx51-mx53.c
> +++ b/arch/arm/mach-mx5/clock-mx51-mx53.c
> @@ -1013,20 +1013,6 @@ static struct clk mipi_hsp_clk = {
>  		.secondary	= s,			\
>  	}
>  
> -#define DEFINE_CLOCK_MAX(name, i, er, es, pfx, p, s)	\
> -	static struct clk name = {			\
> -		.id		= i,			\
> -		.enable_reg	= er,			\
> -		.enable_shift	= es,			\
> -		.get_rate	= pfx##_get_rate,	\
> -		.set_rate	= pfx##_set_rate,	\
> -		.set_parent	= pfx##_set_parent,	\
> -		.enable		= _clk_max_enable,	\
> -		.disable	= _clk_max_disable,	\
> -		.parent		= p,			\
> -		.secondary	= s,			\
> -	}
> -
>  #define CLK_GET_RATE(name, nr, bitsname)				\
>  static unsigned long clk_##name##_get_rate(struct clk *clk)		\
>  {									\
> @@ -1088,6 +1074,25 @@ static int clk_##name##_set_rate(struct clk *clk, unsigned long rate)	\
>  	return 0;							\
>  }
>  
> +#define CLK_ROUND_RATE(name , nr, bitsname)				\
> +static unsigned long clk_##name##_round_rate(struct clk *clk,		\
> +						unsigned long rate)	\
> +{									\
> +	u32 div, parent_rate;						\
> +	u32 pre = 0, post = 0;						\
> +									\
> +	parent_rate = clk_get_rate(clk->parent);			\
> +	div = DIV_ROUND_UP(parent_rate, rate);				\
> +									\
> +	__calc_pre_post_dividers(div, &pre, &post,			\
> +		(MXC_CCM_CSCDR##nr##_##bitsname##_CLK_PRED_MASK >>	\
> +		MXC_CCM_CSCDR##nr##_##bitsname##_CLK_PRED_OFFSET) + 1,	\
> +		(MXC_CCM_CSCDR##nr##_##bitsname##_CLK_PODF_MASK >>	\
> +		MXC_CCM_CSCDR##nr##_##bitsname##_CLK_PODF_OFFSET) + 1);	\
> +									\
> +	return parent_rate / pre / post;				\
> +}
> +
>  /* UART */
>  CLK_GET_RATE(uart, 1, UART)
>  CLK_SET_PARENT(uart, 1, UART)
> @@ -1157,11 +1162,13 @@ static struct clk ecspi_main_clk = {
>  CLK_GET_RATE(esdhc1, 1, ESDHC1_MSHC1)
>  CLK_SET_PARENT(esdhc1, 1, ESDHC1_MSHC1)
>  CLK_SET_RATE(esdhc1, 1, ESDHC1_MSHC1)
> +CLK_ROUND_RATE(esdhc1, 1, ESDHC1_MSHC1)
>  
>  /* mx51 specific */
>  CLK_GET_RATE(esdhc2, 1, ESDHC2_MSHC2)
>  CLK_SET_PARENT(esdhc2, 1, ESDHC2_MSHC2)
>  CLK_SET_RATE(esdhc2, 1, ESDHC2_MSHC2)
> +CLK_ROUND_RATE(esdhc2, 1, ESDHC2_MSHC2)
>  
>  static int clk_esdhc3_set_parent(struct clk *clk, struct clk *parent)
>  {
> @@ -1215,6 +1222,7 @@ static int clk_esdhc2_mx53_set_parent(struct clk *clk, struct clk *parent)
>  CLK_GET_RATE(esdhc3_mx53, 1, ESDHC3_MX53)
>  CLK_SET_PARENT(esdhc3_mx53, 1, ESDHC3_MX53)
>  CLK_SET_RATE(esdhc3_mx53, 1, ESDHC3_MX53)
> +CLK_ROUND_RATE(esdhc3_mx53, 1, ESDHC3_MX53)
>  
>  static int clk_esdhc4_mx53_set_parent(struct clk *clk, struct clk *parent)
>  {
> @@ -1341,18 +1349,18 @@ DEFINE_CLOCK(sdma_clk, 1, MXC_CCM_CCGR4, MXC_CCM_CCGRx_CG15_OFFSET,
>  
>  /* eSDHC */
>  DEFINE_CLOCK_FULL(esdhc1_ipg_clk, 0, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG0_OFFSET,
> -	NULL,  NULL, _clk_max_enable, _clk_max_disable, &ipg_clk, NULL);
> -DEFINE_CLOCK_MAX(esdhc1_clk, 0, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG1_OFFSET,
> +	NULL,  NULL, _clk_ccgr_enable, _clk_ccgr_disable, &ipg_clk, NULL);
> +DEFINE_CLOCK_CCGR(esdhc1_clk, 0, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG1_OFFSET,
>  	clk_esdhc1, &pll2_sw_clk, &esdhc1_ipg_clk);
>  DEFINE_CLOCK_FULL(esdhc2_ipg_clk, 1, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG2_OFFSET,
> -	NULL,  NULL, _clk_max_enable, _clk_max_disable, &ipg_clk, NULL);
> +	NULL,  NULL, _clk_ccgr_enable, _clk_ccgr_disable, &ipg_clk, NULL);
>  DEFINE_CLOCK_FULL(esdhc3_ipg_clk, 2, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG4_OFFSET,
> -	NULL,  NULL, _clk_max_enable, _clk_max_disable, &ipg_clk, NULL);
> +	NULL,  NULL, _clk_ccgr_enable, _clk_ccgr_disable, &ipg_clk, NULL);
>  DEFINE_CLOCK_FULL(esdhc4_ipg_clk, 3, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG6_OFFSET,
> -	NULL,  NULL, _clk_max_enable, _clk_max_disable, &ipg_clk, NULL);
> +	NULL,  NULL, _clk_ccgr_enable, _clk_ccgr_disable, &ipg_clk, NULL);
>  
>  /* mx51 specific */
> -DEFINE_CLOCK_MAX(esdhc2_clk, 1, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG3_OFFSET,
> +DEFINE_CLOCK_CCGR(esdhc2_clk, 1, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG3_OFFSET,
>  	clk_esdhc2, &pll2_sw_clk, &esdhc2_ipg_clk);
>  
>  static struct clk esdhc3_clk = {
> @@ -1361,8 +1369,8 @@ static struct clk esdhc3_clk = {
>  	.set_parent = clk_esdhc3_set_parent,
>  	.enable_reg = MXC_CCM_CCGR3,
>  	.enable_shift = MXC_CCM_CCGRx_CG5_OFFSET,
> -	.enable  = _clk_max_enable,
> -	.disable = _clk_max_disable,
> +	.enable  = _clk_ccgr_enable,
> +	.disable = _clk_ccgr_disable,
>  	.secondary = &esdhc3_ipg_clk,
>  };
>  static struct clk esdhc4_clk = {
> @@ -1371,8 +1379,8 @@ static struct clk esdhc4_clk = {
>  	.set_parent = clk_esdhc4_set_parent,
>  	.enable_reg = MXC_CCM_CCGR3,
>  	.enable_shift = MXC_CCM_CCGRx_CG7_OFFSET,
> -	.enable  = _clk_max_enable,
> -	.disable = _clk_max_disable,
> +	.enable  = _clk_ccgr_enable,
> +	.disable = _clk_ccgr_disable,
>  	.secondary = &esdhc4_ipg_clk,
>  };
>  
> @@ -1383,12 +1391,12 @@ static struct clk esdhc2_mx53_clk = {
>  	.set_parent = clk_esdhc2_mx53_set_parent,
>  	.enable_reg = MXC_CCM_CCGR3,
>  	.enable_shift = MXC_CCM_CCGRx_CG3_OFFSET,
> -	.enable  = _clk_max_enable,
> -	.disable = _clk_max_disable,
> +	.enable  = _clk_ccgr_enable,
> +	.disable = _clk_ccgr_disable,
>  	.secondary = &esdhc3_ipg_clk,
>  };
>  
> -DEFINE_CLOCK_MAX(esdhc3_mx53_clk, 2, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG5_OFFSET,
> +DEFINE_CLOCK_CCGR(esdhc3_mx53_clk, 2, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG5_OFFSET,
>  	clk_esdhc3_mx53, &pll2_sw_clk, &esdhc2_ipg_clk);
>  
>  static struct clk esdhc4_mx53_clk = {
> @@ -1397,17 +1405,17 @@ static struct clk esdhc4_mx53_clk = {
>  	.set_parent = clk_esdhc4_mx53_set_parent,
>  	.enable_reg = MXC_CCM_CCGR3,
>  	.enable_shift = MXC_CCM_CCGRx_CG7_OFFSET,
> -	.enable  = _clk_max_enable,
> -	.disable = _clk_max_disable,
> +	.enable  = _clk_ccgr_enable,
> +	.disable = _clk_ccgr_disable,
>  	.secondary = &esdhc4_ipg_clk,
>  };
>  
>  static struct clk sata_clk = {
>  	.parent = &ipg_clk,
> -	.enable = _clk_max_enable,
> +	.enable = _clk_ccgr_enable,
>  	.enable_reg = MXC_CCM_CCGR4,
>  	.enable_shift = MXC_CCM_CCGRx_CG1_OFFSET,
> -	.disable = _clk_max_disable,
> +	.disable = _clk_ccgr_disable,
>  };
>  
>  static struct clk ahci_phy_clk = {
> -- 
> 1.7.5.4
> 
> 
>
Richard Zhao - Nov. 18, 2011, 12:57 a.m.
Hi Sascha,
On Thu, Nov 17, 2011 at 07:18:53PM +0100, Sascha Hauer wrote:
> On Mon, Nov 07, 2011 at 11:02:15AM +0800, Richard Zhao wrote:
> > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > ---
> >  arch/arm/mach-mx5/clock-mx51-mx53.c |   70 +++++++++++++++++++---------------
> >  1 files changed, 39 insertions(+), 31 deletions(-)
> 
> What exactly is the misuse of clk_max_enable/disable you are talking
> about?
_clk_max_enable/_clk_max_enable adds extra ahb max handshake set.
It is only for ahb_max_clk. There's no reason for other clk to use it.
> What does this fix?
The patch change other misuse place to use more common _clk_ccgr_enable and
_clk_ccgr_disable.
> Why is a clk_round_rate involved?
I choose to use DEFINE_CLOCK_CCGR to define esdhc clocks, It needs
a round_rate. So I add a macro to help, like macro CLK_SET_RATE.
> This patch seems to do more than the subject says. You should split
> it and improve the description.
You see, it's hard to split. One macro patch? But I can add more comments.
What do you think?

Richard
> 
> Sascha
> 
> > 
> > diff --git a/arch/arm/mach-mx5/clock-mx51-mx53.c b/arch/arm/mach-mx5/clock-mx51-mx53.c
> > index a3ca4ce..507d24c 100644
> > --- a/arch/arm/mach-mx5/clock-mx51-mx53.c
> > +++ b/arch/arm/mach-mx5/clock-mx51-mx53.c
> > @@ -1013,20 +1013,6 @@ static struct clk mipi_hsp_clk = {
> >  		.secondary	= s,			\
> >  	}
> >  
> > -#define DEFINE_CLOCK_MAX(name, i, er, es, pfx, p, s)	\
> > -	static struct clk name = {			\
> > -		.id		= i,			\
> > -		.enable_reg	= er,			\
> > -		.enable_shift	= es,			\
> > -		.get_rate	= pfx##_get_rate,	\
> > -		.set_rate	= pfx##_set_rate,	\
> > -		.set_parent	= pfx##_set_parent,	\
> > -		.enable		= _clk_max_enable,	\
> > -		.disable	= _clk_max_disable,	\
> > -		.parent		= p,			\
> > -		.secondary	= s,			\
> > -	}
> > -
> >  #define CLK_GET_RATE(name, nr, bitsname)				\
> >  static unsigned long clk_##name##_get_rate(struct clk *clk)		\
> >  {									\
> > @@ -1088,6 +1074,25 @@ static int clk_##name##_set_rate(struct clk *clk, unsigned long rate)	\
> >  	return 0;							\
> >  }
> >  
> > +#define CLK_ROUND_RATE(name , nr, bitsname)				\
> > +static unsigned long clk_##name##_round_rate(struct clk *clk,		\
> > +						unsigned long rate)	\
> > +{									\
> > +	u32 div, parent_rate;						\
> > +	u32 pre = 0, post = 0;						\
> > +									\
> > +	parent_rate = clk_get_rate(clk->parent);			\
> > +	div = DIV_ROUND_UP(parent_rate, rate);				\
> > +									\
> > +	__calc_pre_post_dividers(div, &pre, &post,			\
> > +		(MXC_CCM_CSCDR##nr##_##bitsname##_CLK_PRED_MASK >>	\
> > +		MXC_CCM_CSCDR##nr##_##bitsname##_CLK_PRED_OFFSET) + 1,	\
> > +		(MXC_CCM_CSCDR##nr##_##bitsname##_CLK_PODF_MASK >>	\
> > +		MXC_CCM_CSCDR##nr##_##bitsname##_CLK_PODF_OFFSET) + 1);	\
> > +									\
> > +	return parent_rate / pre / post;				\
> > +}
> > +
> >  /* UART */
> >  CLK_GET_RATE(uart, 1, UART)
> >  CLK_SET_PARENT(uart, 1, UART)
> > @@ -1157,11 +1162,13 @@ static struct clk ecspi_main_clk = {
> >  CLK_GET_RATE(esdhc1, 1, ESDHC1_MSHC1)
> >  CLK_SET_PARENT(esdhc1, 1, ESDHC1_MSHC1)
> >  CLK_SET_RATE(esdhc1, 1, ESDHC1_MSHC1)
> > +CLK_ROUND_RATE(esdhc1, 1, ESDHC1_MSHC1)
> >  
> >  /* mx51 specific */
> >  CLK_GET_RATE(esdhc2, 1, ESDHC2_MSHC2)
> >  CLK_SET_PARENT(esdhc2, 1, ESDHC2_MSHC2)
> >  CLK_SET_RATE(esdhc2, 1, ESDHC2_MSHC2)
> > +CLK_ROUND_RATE(esdhc2, 1, ESDHC2_MSHC2)
> >  
> >  static int clk_esdhc3_set_parent(struct clk *clk, struct clk *parent)
> >  {
> > @@ -1215,6 +1222,7 @@ static int clk_esdhc2_mx53_set_parent(struct clk *clk, struct clk *parent)
> >  CLK_GET_RATE(esdhc3_mx53, 1, ESDHC3_MX53)
> >  CLK_SET_PARENT(esdhc3_mx53, 1, ESDHC3_MX53)
> >  CLK_SET_RATE(esdhc3_mx53, 1, ESDHC3_MX53)
> > +CLK_ROUND_RATE(esdhc3_mx53, 1, ESDHC3_MX53)
> >  
> >  static int clk_esdhc4_mx53_set_parent(struct clk *clk, struct clk *parent)
> >  {
> > @@ -1341,18 +1349,18 @@ DEFINE_CLOCK(sdma_clk, 1, MXC_CCM_CCGR4, MXC_CCM_CCGRx_CG15_OFFSET,
> >  
> >  /* eSDHC */
> >  DEFINE_CLOCK_FULL(esdhc1_ipg_clk, 0, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG0_OFFSET,
> > -	NULL,  NULL, _clk_max_enable, _clk_max_disable, &ipg_clk, NULL);
> > -DEFINE_CLOCK_MAX(esdhc1_clk, 0, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG1_OFFSET,
> > +	NULL,  NULL, _clk_ccgr_enable, _clk_ccgr_disable, &ipg_clk, NULL);
> > +DEFINE_CLOCK_CCGR(esdhc1_clk, 0, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG1_OFFSET,
> >  	clk_esdhc1, &pll2_sw_clk, &esdhc1_ipg_clk);
> >  DEFINE_CLOCK_FULL(esdhc2_ipg_clk, 1, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG2_OFFSET,
> > -	NULL,  NULL, _clk_max_enable, _clk_max_disable, &ipg_clk, NULL);
> > +	NULL,  NULL, _clk_ccgr_enable, _clk_ccgr_disable, &ipg_clk, NULL);
> >  DEFINE_CLOCK_FULL(esdhc3_ipg_clk, 2, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG4_OFFSET,
> > -	NULL,  NULL, _clk_max_enable, _clk_max_disable, &ipg_clk, NULL);
> > +	NULL,  NULL, _clk_ccgr_enable, _clk_ccgr_disable, &ipg_clk, NULL);
> >  DEFINE_CLOCK_FULL(esdhc4_ipg_clk, 3, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG6_OFFSET,
> > -	NULL,  NULL, _clk_max_enable, _clk_max_disable, &ipg_clk, NULL);
> > +	NULL,  NULL, _clk_ccgr_enable, _clk_ccgr_disable, &ipg_clk, NULL);
> >  
> >  /* mx51 specific */
> > -DEFINE_CLOCK_MAX(esdhc2_clk, 1, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG3_OFFSET,
> > +DEFINE_CLOCK_CCGR(esdhc2_clk, 1, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG3_OFFSET,
> >  	clk_esdhc2, &pll2_sw_clk, &esdhc2_ipg_clk);
> >  
> >  static struct clk esdhc3_clk = {
> > @@ -1361,8 +1369,8 @@ static struct clk esdhc3_clk = {
> >  	.set_parent = clk_esdhc3_set_parent,
> >  	.enable_reg = MXC_CCM_CCGR3,
> >  	.enable_shift = MXC_CCM_CCGRx_CG5_OFFSET,
> > -	.enable  = _clk_max_enable,
> > -	.disable = _clk_max_disable,
> > +	.enable  = _clk_ccgr_enable,
> > +	.disable = _clk_ccgr_disable,
> >  	.secondary = &esdhc3_ipg_clk,
> >  };
> >  static struct clk esdhc4_clk = {
> > @@ -1371,8 +1379,8 @@ static struct clk esdhc4_clk = {
> >  	.set_parent = clk_esdhc4_set_parent,
> >  	.enable_reg = MXC_CCM_CCGR3,
> >  	.enable_shift = MXC_CCM_CCGRx_CG7_OFFSET,
> > -	.enable  = _clk_max_enable,
> > -	.disable = _clk_max_disable,
> > +	.enable  = _clk_ccgr_enable,
> > +	.disable = _clk_ccgr_disable,
> >  	.secondary = &esdhc4_ipg_clk,
> >  };
> >  
> > @@ -1383,12 +1391,12 @@ static struct clk esdhc2_mx53_clk = {
> >  	.set_parent = clk_esdhc2_mx53_set_parent,
> >  	.enable_reg = MXC_CCM_CCGR3,
> >  	.enable_shift = MXC_CCM_CCGRx_CG3_OFFSET,
> > -	.enable  = _clk_max_enable,
> > -	.disable = _clk_max_disable,
> > +	.enable  = _clk_ccgr_enable,
> > +	.disable = _clk_ccgr_disable,
> >  	.secondary = &esdhc3_ipg_clk,
> >  };
> >  
> > -DEFINE_CLOCK_MAX(esdhc3_mx53_clk, 2, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG5_OFFSET,
> > +DEFINE_CLOCK_CCGR(esdhc3_mx53_clk, 2, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG5_OFFSET,
> >  	clk_esdhc3_mx53, &pll2_sw_clk, &esdhc2_ipg_clk);
> >  
> >  static struct clk esdhc4_mx53_clk = {
> > @@ -1397,17 +1405,17 @@ static struct clk esdhc4_mx53_clk = {
> >  	.set_parent = clk_esdhc4_mx53_set_parent,
> >  	.enable_reg = MXC_CCM_CCGR3,
> >  	.enable_shift = MXC_CCM_CCGRx_CG7_OFFSET,
> > -	.enable  = _clk_max_enable,
> > -	.disable = _clk_max_disable,
> > +	.enable  = _clk_ccgr_enable,
> > +	.disable = _clk_ccgr_disable,
> >  	.secondary = &esdhc4_ipg_clk,
> >  };
> >  
> >  static struct clk sata_clk = {
> >  	.parent = &ipg_clk,
> > -	.enable = _clk_max_enable,
> > +	.enable = _clk_ccgr_enable,
> >  	.enable_reg = MXC_CCM_CCGR4,
> >  	.enable_shift = MXC_CCM_CCGRx_CG1_OFFSET,
> > -	.disable = _clk_max_disable,
> > +	.disable = _clk_ccgr_disable,
> >  };
> >  
> >  static struct clk ahci_phy_clk = {
> > -- 
> > 1.7.5.4
> > 
> > 
> > 
> 
> -- 
> Pengutronix e.K.                           |                             |
> Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
Sascha Hauer - Nov. 18, 2011, 6:50 a.m.
On Fri, Nov 18, 2011 at 08:57:20AM +0800, Richard Zhao wrote:
> Hi Sascha,
> On Thu, Nov 17, 2011 at 07:18:53PM +0100, Sascha Hauer wrote:
> > On Mon, Nov 07, 2011 at 11:02:15AM +0800, Richard Zhao wrote:
> > > Signed-off-by: Richard Zhao <richard.zhao@linaro.org>
> > > ---
> > >  arch/arm/mach-mx5/clock-mx51-mx53.c |   70 +++++++++++++++++++---------------
> > >  1 files changed, 39 insertions(+), 31 deletions(-)
> > 
> > What exactly is the misuse of clk_max_enable/disable you are talking
> > about?
> _clk_max_enable/_clk_max_enable adds extra ahb max handshake set.
> It is only for ahb_max_clk. There's no reason for other clk to use it.

Ah, then please update the commit log.

> > What does this fix?
> The patch change other misuse place to use more common _clk_ccgr_enable and
> _clk_ccgr_disable.
> > Why is a clk_round_rate involved?
> I choose to use DEFINE_CLOCK_CCGR to define esdhc clocks, It needs
> a round_rate. So I add a macro to help, like macro CLK_SET_RATE.
> > This patch seems to do more than the subject says. You should split
> > it and improve the description.
> You see, it's hard to split. One macro patch? But I can add more comments.
> What do you think?

This one is easy to split into ~10 bisectable patches, but ok, we don't
want that. Please make two patches, one for the simple _clk_max_x ->
clk_ccgr_x and one with the round rate change involved.

> 
> Richard
> > 
> > Sascha
> > 
> > > 
> > > diff --git a/arch/arm/mach-mx5/clock-mx51-mx53.c b/arch/arm/mach-mx5/clock-mx51-mx53.c
> > > index a3ca4ce..507d24c 100644
> > > --- a/arch/arm/mach-mx5/clock-mx51-mx53.c
> > > +++ b/arch/arm/mach-mx5/clock-mx51-mx53.c
> > > @@ -1013,20 +1013,6 @@ static struct clk mipi_hsp_clk = {
> > >  		.secondary	= s,			\
> > >  	}
> > >  
> > > -#define DEFINE_CLOCK_MAX(name, i, er, es, pfx, p, s)	\
> > > -	static struct clk name = {			\
> > > -		.id		= i,			\
> > > -		.enable_reg	= er,			\
> > > -		.enable_shift	= es,			\
> > > -		.get_rate	= pfx##_get_rate,	\
> > > -		.set_rate	= pfx##_set_rate,	\
> > > -		.set_parent	= pfx##_set_parent,	\
> > > -		.enable		= _clk_max_enable,	\
> > > -		.disable	= _clk_max_disable,	\
> > > -		.parent		= p,			\
> > > -		.secondary	= s,			\
> > > -	}
> > > -
> > >  #define CLK_GET_RATE(name, nr, bitsname)				\
> > >  static unsigned long clk_##name##_get_rate(struct clk *clk)		\
> > >  {									\
> > > @@ -1088,6 +1074,25 @@ static int clk_##name##_set_rate(struct clk *clk, unsigned long rate)	\
> > >  	return 0;							\
> > >  }
> > >  
> > > +#define CLK_ROUND_RATE(name , nr, bitsname)				\
> > > +static unsigned long clk_##name##_round_rate(struct clk *clk,		\
> > > +						unsigned long rate)	\
> > > +{									\
> > > +	u32 div, parent_rate;						\
> > > +	u32 pre = 0, post = 0;						\
> > > +									\
> > > +	parent_rate = clk_get_rate(clk->parent);			\
> > > +	div = DIV_ROUND_UP(parent_rate, rate);				\
> > > +									\
> > > +	__calc_pre_post_dividers(div, &pre, &post,			\
> > > +		(MXC_CCM_CSCDR##nr##_##bitsname##_CLK_PRED_MASK >>	\
> > > +		MXC_CCM_CSCDR##nr##_##bitsname##_CLK_PRED_OFFSET) + 1,	\
> > > +		(MXC_CCM_CSCDR##nr##_##bitsname##_CLK_PODF_MASK >>	\
> > > +		MXC_CCM_CSCDR##nr##_##bitsname##_CLK_PODF_OFFSET) + 1);	\
> > > +									\
> > > +	return parent_rate / pre / post;				\
> > > +}
> > > +
> > >  /* UART */
> > >  CLK_GET_RATE(uart, 1, UART)
> > >  CLK_SET_PARENT(uart, 1, UART)
> > > @@ -1157,11 +1162,13 @@ static struct clk ecspi_main_clk = {
> > >  CLK_GET_RATE(esdhc1, 1, ESDHC1_MSHC1)
> > >  CLK_SET_PARENT(esdhc1, 1, ESDHC1_MSHC1)
> > >  CLK_SET_RATE(esdhc1, 1, ESDHC1_MSHC1)
> > > +CLK_ROUND_RATE(esdhc1, 1, ESDHC1_MSHC1)
> > >  
> > >  /* mx51 specific */
> > >  CLK_GET_RATE(esdhc2, 1, ESDHC2_MSHC2)
> > >  CLK_SET_PARENT(esdhc2, 1, ESDHC2_MSHC2)
> > >  CLK_SET_RATE(esdhc2, 1, ESDHC2_MSHC2)
> > > +CLK_ROUND_RATE(esdhc2, 1, ESDHC2_MSHC2)
> > >  
> > >  static int clk_esdhc3_set_parent(struct clk *clk, struct clk *parent)
> > >  {
> > > @@ -1215,6 +1222,7 @@ static int clk_esdhc2_mx53_set_parent(struct clk *clk, struct clk *parent)
> > >  CLK_GET_RATE(esdhc3_mx53, 1, ESDHC3_MX53)
> > >  CLK_SET_PARENT(esdhc3_mx53, 1, ESDHC3_MX53)
> > >  CLK_SET_RATE(esdhc3_mx53, 1, ESDHC3_MX53)
> > > +CLK_ROUND_RATE(esdhc3_mx53, 1, ESDHC3_MX53)
> > >  
> > >  static int clk_esdhc4_mx53_set_parent(struct clk *clk, struct clk *parent)
> > >  {
> > > @@ -1341,18 +1349,18 @@ DEFINE_CLOCK(sdma_clk, 1, MXC_CCM_CCGR4, MXC_CCM_CCGRx_CG15_OFFSET,
> > >  
> > >  /* eSDHC */
> > >  DEFINE_CLOCK_FULL(esdhc1_ipg_clk, 0, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG0_OFFSET,
> > > -	NULL,  NULL, _clk_max_enable, _clk_max_disable, &ipg_clk, NULL);
> > > -DEFINE_CLOCK_MAX(esdhc1_clk, 0, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG1_OFFSET,
> > > +	NULL,  NULL, _clk_ccgr_enable, _clk_ccgr_disable, &ipg_clk, NULL);
> > > +DEFINE_CLOCK_CCGR(esdhc1_clk, 0, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG1_OFFSET,
> > >  	clk_esdhc1, &pll2_sw_clk, &esdhc1_ipg_clk);
> > >  DEFINE_CLOCK_FULL(esdhc2_ipg_clk, 1, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG2_OFFSET,
> > > -	NULL,  NULL, _clk_max_enable, _clk_max_disable, &ipg_clk, NULL);
> > > +	NULL,  NULL, _clk_ccgr_enable, _clk_ccgr_disable, &ipg_clk, NULL);
> > >  DEFINE_CLOCK_FULL(esdhc3_ipg_clk, 2, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG4_OFFSET,
> > > -	NULL,  NULL, _clk_max_enable, _clk_max_disable, &ipg_clk, NULL);
> > > +	NULL,  NULL, _clk_ccgr_enable, _clk_ccgr_disable, &ipg_clk, NULL);
> > >  DEFINE_CLOCK_FULL(esdhc4_ipg_clk, 3, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG6_OFFSET,
> > > -	NULL,  NULL, _clk_max_enable, _clk_max_disable, &ipg_clk, NULL);
> > > +	NULL,  NULL, _clk_ccgr_enable, _clk_ccgr_disable, &ipg_clk, NULL);
> > >  
> > >  /* mx51 specific */
> > > -DEFINE_CLOCK_MAX(esdhc2_clk, 1, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG3_OFFSET,
> > > +DEFINE_CLOCK_CCGR(esdhc2_clk, 1, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG3_OFFSET,
> > >  	clk_esdhc2, &pll2_sw_clk, &esdhc2_ipg_clk);
> > >  
> > >  static struct clk esdhc3_clk = {
> > > @@ -1361,8 +1369,8 @@ static struct clk esdhc3_clk = {
> > >  	.set_parent = clk_esdhc3_set_parent,
> > >  	.enable_reg = MXC_CCM_CCGR3,
> > >  	.enable_shift = MXC_CCM_CCGRx_CG5_OFFSET,
> > > -	.enable  = _clk_max_enable,
> > > -	.disable = _clk_max_disable,
> > > +	.enable  = _clk_ccgr_enable,
> > > +	.disable = _clk_ccgr_disable,
> > >  	.secondary = &esdhc3_ipg_clk,
> > >  };
> > >  static struct clk esdhc4_clk = {
> > > @@ -1371,8 +1379,8 @@ static struct clk esdhc4_clk = {
> > >  	.set_parent = clk_esdhc4_set_parent,
> > >  	.enable_reg = MXC_CCM_CCGR3,
> > >  	.enable_shift = MXC_CCM_CCGRx_CG7_OFFSET,
> > > -	.enable  = _clk_max_enable,
> > > -	.disable = _clk_max_disable,
> > > +	.enable  = _clk_ccgr_enable,
> > > +	.disable = _clk_ccgr_disable,
> > >  	.secondary = &esdhc4_ipg_clk,
> > >  };
> > >  
> > > @@ -1383,12 +1391,12 @@ static struct clk esdhc2_mx53_clk = {
> > >  	.set_parent = clk_esdhc2_mx53_set_parent,
> > >  	.enable_reg = MXC_CCM_CCGR3,
> > >  	.enable_shift = MXC_CCM_CCGRx_CG3_OFFSET,
> > > -	.enable  = _clk_max_enable,
> > > -	.disable = _clk_max_disable,
> > > +	.enable  = _clk_ccgr_enable,
> > > +	.disable = _clk_ccgr_disable,
> > >  	.secondary = &esdhc3_ipg_clk,
> > >  };
> > >  
> > > -DEFINE_CLOCK_MAX(esdhc3_mx53_clk, 2, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG5_OFFSET,
> > > +DEFINE_CLOCK_CCGR(esdhc3_mx53_clk, 2, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG5_OFFSET,
> > >  	clk_esdhc3_mx53, &pll2_sw_clk, &esdhc2_ipg_clk);
> > >  
> > >  static struct clk esdhc4_mx53_clk = {
> > > @@ -1397,17 +1405,17 @@ static struct clk esdhc4_mx53_clk = {
> > >  	.set_parent = clk_esdhc4_mx53_set_parent,
> > >  	.enable_reg = MXC_CCM_CCGR3,
> > >  	.enable_shift = MXC_CCM_CCGRx_CG7_OFFSET,
> > > -	.enable  = _clk_max_enable,
> > > -	.disable = _clk_max_disable,
> > > +	.enable  = _clk_ccgr_enable,
> > > +	.disable = _clk_ccgr_disable,
> > >  	.secondary = &esdhc4_ipg_clk,
> > >  };
> > >  
> > >  static struct clk sata_clk = {
> > >  	.parent = &ipg_clk,
> > > -	.enable = _clk_max_enable,
> > > +	.enable = _clk_ccgr_enable,
> > >  	.enable_reg = MXC_CCM_CCGR4,
> > >  	.enable_shift = MXC_CCM_CCGRx_CG1_OFFSET,
> > > -	.disable = _clk_max_disable,
> > > +	.disable = _clk_ccgr_disable,
> > >  };
> > >  
> > >  static struct clk ahci_phy_clk = {
> > > -- 
> > > 1.7.5.4
> > > 
> > > 
> > > 
> > 
> > -- 
> > Pengutronix e.K.                           |                             |
> > Industrial Linux Solutions                 | http://www.pengutronix.de/  |
> > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
> > Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |
> > 
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> > 
> 
>

Patch

diff --git a/arch/arm/mach-mx5/clock-mx51-mx53.c b/arch/arm/mach-mx5/clock-mx51-mx53.c
index a3ca4ce..507d24c 100644
--- a/arch/arm/mach-mx5/clock-mx51-mx53.c
+++ b/arch/arm/mach-mx5/clock-mx51-mx53.c
@@ -1013,20 +1013,6 @@  static struct clk mipi_hsp_clk = {
 		.secondary	= s,			\
 	}
 
-#define DEFINE_CLOCK_MAX(name, i, er, es, pfx, p, s)	\
-	static struct clk name = {			\
-		.id		= i,			\
-		.enable_reg	= er,			\
-		.enable_shift	= es,			\
-		.get_rate	= pfx##_get_rate,	\
-		.set_rate	= pfx##_set_rate,	\
-		.set_parent	= pfx##_set_parent,	\
-		.enable		= _clk_max_enable,	\
-		.disable	= _clk_max_disable,	\
-		.parent		= p,			\
-		.secondary	= s,			\
-	}
-
 #define CLK_GET_RATE(name, nr, bitsname)				\
 static unsigned long clk_##name##_get_rate(struct clk *clk)		\
 {									\
@@ -1088,6 +1074,25 @@  static int clk_##name##_set_rate(struct clk *clk, unsigned long rate)	\
 	return 0;							\
 }
 
+#define CLK_ROUND_RATE(name , nr, bitsname)				\
+static unsigned long clk_##name##_round_rate(struct clk *clk,		\
+						unsigned long rate)	\
+{									\
+	u32 div, parent_rate;						\
+	u32 pre = 0, post = 0;						\
+									\
+	parent_rate = clk_get_rate(clk->parent);			\
+	div = DIV_ROUND_UP(parent_rate, rate);				\
+									\
+	__calc_pre_post_dividers(div, &pre, &post,			\
+		(MXC_CCM_CSCDR##nr##_##bitsname##_CLK_PRED_MASK >>	\
+		MXC_CCM_CSCDR##nr##_##bitsname##_CLK_PRED_OFFSET) + 1,	\
+		(MXC_CCM_CSCDR##nr##_##bitsname##_CLK_PODF_MASK >>	\
+		MXC_CCM_CSCDR##nr##_##bitsname##_CLK_PODF_OFFSET) + 1);	\
+									\
+	return parent_rate / pre / post;				\
+}
+
 /* UART */
 CLK_GET_RATE(uart, 1, UART)
 CLK_SET_PARENT(uart, 1, UART)
@@ -1157,11 +1162,13 @@  static struct clk ecspi_main_clk = {
 CLK_GET_RATE(esdhc1, 1, ESDHC1_MSHC1)
 CLK_SET_PARENT(esdhc1, 1, ESDHC1_MSHC1)
 CLK_SET_RATE(esdhc1, 1, ESDHC1_MSHC1)
+CLK_ROUND_RATE(esdhc1, 1, ESDHC1_MSHC1)
 
 /* mx51 specific */
 CLK_GET_RATE(esdhc2, 1, ESDHC2_MSHC2)
 CLK_SET_PARENT(esdhc2, 1, ESDHC2_MSHC2)
 CLK_SET_RATE(esdhc2, 1, ESDHC2_MSHC2)
+CLK_ROUND_RATE(esdhc2, 1, ESDHC2_MSHC2)
 
 static int clk_esdhc3_set_parent(struct clk *clk, struct clk *parent)
 {
@@ -1215,6 +1222,7 @@  static int clk_esdhc2_mx53_set_parent(struct clk *clk, struct clk *parent)
 CLK_GET_RATE(esdhc3_mx53, 1, ESDHC3_MX53)
 CLK_SET_PARENT(esdhc3_mx53, 1, ESDHC3_MX53)
 CLK_SET_RATE(esdhc3_mx53, 1, ESDHC3_MX53)
+CLK_ROUND_RATE(esdhc3_mx53, 1, ESDHC3_MX53)
 
 static int clk_esdhc4_mx53_set_parent(struct clk *clk, struct clk *parent)
 {
@@ -1341,18 +1349,18 @@  DEFINE_CLOCK(sdma_clk, 1, MXC_CCM_CCGR4, MXC_CCM_CCGRx_CG15_OFFSET,
 
 /* eSDHC */
 DEFINE_CLOCK_FULL(esdhc1_ipg_clk, 0, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG0_OFFSET,
-	NULL,  NULL, _clk_max_enable, _clk_max_disable, &ipg_clk, NULL);
-DEFINE_CLOCK_MAX(esdhc1_clk, 0, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG1_OFFSET,
+	NULL,  NULL, _clk_ccgr_enable, _clk_ccgr_disable, &ipg_clk, NULL);
+DEFINE_CLOCK_CCGR(esdhc1_clk, 0, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG1_OFFSET,
 	clk_esdhc1, &pll2_sw_clk, &esdhc1_ipg_clk);
 DEFINE_CLOCK_FULL(esdhc2_ipg_clk, 1, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG2_OFFSET,
-	NULL,  NULL, _clk_max_enable, _clk_max_disable, &ipg_clk, NULL);
+	NULL,  NULL, _clk_ccgr_enable, _clk_ccgr_disable, &ipg_clk, NULL);
 DEFINE_CLOCK_FULL(esdhc3_ipg_clk, 2, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG4_OFFSET,
-	NULL,  NULL, _clk_max_enable, _clk_max_disable, &ipg_clk, NULL);
+	NULL,  NULL, _clk_ccgr_enable, _clk_ccgr_disable, &ipg_clk, NULL);
 DEFINE_CLOCK_FULL(esdhc4_ipg_clk, 3, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG6_OFFSET,
-	NULL,  NULL, _clk_max_enable, _clk_max_disable, &ipg_clk, NULL);
+	NULL,  NULL, _clk_ccgr_enable, _clk_ccgr_disable, &ipg_clk, NULL);
 
 /* mx51 specific */
-DEFINE_CLOCK_MAX(esdhc2_clk, 1, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG3_OFFSET,
+DEFINE_CLOCK_CCGR(esdhc2_clk, 1, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG3_OFFSET,
 	clk_esdhc2, &pll2_sw_clk, &esdhc2_ipg_clk);
 
 static struct clk esdhc3_clk = {
@@ -1361,8 +1369,8 @@  static struct clk esdhc3_clk = {
 	.set_parent = clk_esdhc3_set_parent,
 	.enable_reg = MXC_CCM_CCGR3,
 	.enable_shift = MXC_CCM_CCGRx_CG5_OFFSET,
-	.enable  = _clk_max_enable,
-	.disable = _clk_max_disable,
+	.enable  = _clk_ccgr_enable,
+	.disable = _clk_ccgr_disable,
 	.secondary = &esdhc3_ipg_clk,
 };
 static struct clk esdhc4_clk = {
@@ -1371,8 +1379,8 @@  static struct clk esdhc4_clk = {
 	.set_parent = clk_esdhc4_set_parent,
 	.enable_reg = MXC_CCM_CCGR3,
 	.enable_shift = MXC_CCM_CCGRx_CG7_OFFSET,
-	.enable  = _clk_max_enable,
-	.disable = _clk_max_disable,
+	.enable  = _clk_ccgr_enable,
+	.disable = _clk_ccgr_disable,
 	.secondary = &esdhc4_ipg_clk,
 };
 
@@ -1383,12 +1391,12 @@  static struct clk esdhc2_mx53_clk = {
 	.set_parent = clk_esdhc2_mx53_set_parent,
 	.enable_reg = MXC_CCM_CCGR3,
 	.enable_shift = MXC_CCM_CCGRx_CG3_OFFSET,
-	.enable  = _clk_max_enable,
-	.disable = _clk_max_disable,
+	.enable  = _clk_ccgr_enable,
+	.disable = _clk_ccgr_disable,
 	.secondary = &esdhc3_ipg_clk,
 };
 
-DEFINE_CLOCK_MAX(esdhc3_mx53_clk, 2, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG5_OFFSET,
+DEFINE_CLOCK_CCGR(esdhc3_mx53_clk, 2, MXC_CCM_CCGR3, MXC_CCM_CCGRx_CG5_OFFSET,
 	clk_esdhc3_mx53, &pll2_sw_clk, &esdhc2_ipg_clk);
 
 static struct clk esdhc4_mx53_clk = {
@@ -1397,17 +1405,17 @@  static struct clk esdhc4_mx53_clk = {
 	.set_parent = clk_esdhc4_mx53_set_parent,
 	.enable_reg = MXC_CCM_CCGR3,
 	.enable_shift = MXC_CCM_CCGRx_CG7_OFFSET,
-	.enable  = _clk_max_enable,
-	.disable = _clk_max_disable,
+	.enable  = _clk_ccgr_enable,
+	.disable = _clk_ccgr_disable,
 	.secondary = &esdhc4_ipg_clk,
 };
 
 static struct clk sata_clk = {
 	.parent = &ipg_clk,
-	.enable = _clk_max_enable,
+	.enable = _clk_ccgr_enable,
 	.enable_reg = MXC_CCM_CCGR4,
 	.enable_shift = MXC_CCM_CCGRx_CG1_OFFSET,
-	.disable = _clk_max_disable,
+	.disable = _clk_ccgr_disable,
 };
 
 static struct clk ahci_phy_clk = {