Hello Jason, I currently merge your and our patch set. Will follow up with the result hopefully later today.
On Fri, Sep 03, 2010 at 02:22:08PM +0800, Jason Wang wrote: >>> @@ -52,6 +53,18 @@ static int _clk_ccgr_enable(struct clk *clk) >>> return 0; >>> } >>> +static int _clk_ccgr_enable_inrun(struct clk *clk) >>> +{ >>> + u32 reg; >>> + >>> + reg = __raw_readl(clk->enable_reg); >>> + reg &= ~(MXC_CCM_CCGRx_CG_MASK << clk->enable_shift); >>> + reg |= MXC_CCM_CCGRx_MOD_IDLE << clk->enable_shift; >>> + __raw_writel(reg, clk->enable_reg); >>> + >>> + return 0; >>> +} >>> + >>> >> imho this should be consolidated in something like: >> >> static int _clk_ccgr_setclk(struct clk *clk, unsigned mode) >> { >> ... >> } >> >> #define _clk_ccgr_enable(clk) _clk_ccgr_setclk(clk, MXC_CCM_CCGRx_MOD_ON) >> #define _clk_ccgr_disable(clk) _clk_ccgr_setclk(clk, MXC_CCM_CCGRx_MOD_OFF) >> #define _clk_ccgr_enable_inrun(clk) _clk_ccgr_setclk(clk, >> MXC_CCM_CCGRx_MOD_IDLE) >> >> > It makes code more concise. On the other hand, too many macros will > add troubles when we use kgdb to perform sourcecode-level debug. Using macros doesn't work here, as they are used as callbacks. Still made it with functions. Then (apart from the return value) _clk_ccgr_enable_inrun and _clk_ccgr_disable_inwait are identically. I wonder if this is intended? > Anyway, i agree your suggestion. >>> static void _clk_ccgr_disable(struct clk *clk) >>> { >>> u32 reg; >>> @@ -762,6 +775,61 @@ static struct clk kpp_clk = { >>> .id = 0, >>> }; >>> +/* eCSPI */ >>> +static unsigned long _clk_ecspi_getrate(struct clk *clk) >>> +{ >>> + u32 reg, prediv, podf; >>> + unsigned long ret; >>> + >>> + reg = __raw_readl(MXC_CCM_CSCDR2); >>> + prediv = ((reg & MXC_CCM_CSCDR2_CSPI_CLK_PRED_MASK) >> >>> + MXC_CCM_CSCDR2_CSPI_CLK_PRED_OFFSET) + 1; >>> + if (prediv == 1) >>> + BUG(); >>> + podf = ((reg & MXC_CCM_CSCDR2_CSPI_CLK_PODF_MASK) >> >>> + MXC_CCM_CSCDR2_CSPI_CLK_PODF_OFFSET) + 1; >>> + >>> + ret = clk_get_rate(clk->parent) / (prediv * podf); >>> + return ret; >>> +} >>> + >>> +static int _clk_ecspi_set_parent(struct clk *clk, struct clk *parent) >>> +{ >>> + u32 reg, mux; >>> + >>> + mux = _get_mux(parent, &pll1_sw_clk, &pll2_sw_clk, &pll3_sw_clk, >>> + &lp_apm_clk); >>> + reg = __raw_readl(MXC_CCM_CSCMR1) & ~MXC_CCM_CSCMR1_CSPI_CLK_SEL_MASK; >>> + reg |= mux << MXC_CCM_CSCMR1_CSPI_CLK_SEL_OFFSET; >>> + __raw_writel(reg, MXC_CCM_CSCMR1); >>> + >>> + return 0; >>> +} >>> + >>> +static struct clk ecspi_main_clk = { >>> + .parent = &pll3_sw_clk, >>> + .get_rate = _clk_ecspi_getrate, >>> + .set_parent = _clk_ecspi_set_parent, >>> >> Sascha didn't implement set_parent >> >> > ecspi really can change parent root clock. >>> +}; >>> + >>> +static struct clk ecspi1_ipg_clk = { >>> + .parent = &ipg_clk, >>> + .secondary = &spba_clk, >>> + .enable_reg = MXC_CCM_CCGR4, >>> + .enable_shift = MXC_CCM_CCGRx_CG9_OFFSET, >>> + .enable = _clk_ccgr_enable_inrun, >>> + .disable = _clk_ccgr_disable, >>> +}; >>> + >>> +static struct clk ecspi2_ipg_clk = { >>> + .parent = &ipg_clk, >>> + .secondary = &aips_tz2_clk, >>> + .enable_reg = MXC_CCM_CCGR4, >>> + .enable_shift = MXC_CCM_CCGRx_CG11_OFFSET, >>> + .enable = _clk_ccgr_enable_inrun, >>> + .disable = _clk_ccgr_disable, >>> +}; aips_tz2_clk is wrong here, no? Sascha used spba_clk here, too. I didn't found out yet how to read that out of the reference manual. Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | ------------------------------------------------------------------------------ Automate Storage Tiering Simply Optimize IT performance and efficiency through flexible, powerful, automated storage tiering capabilities. View this brief to learn how you can reduce costs and improve performance. http://p.sf.net/sfu/dell-sfdev2dev _______________________________________________ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general