On Tue, Mar 15, 2011 at 02:02:56AM -0600, Grant Likely wrote: > On Tue, Mar 15, 2011 at 03:50:29PM +0800, Shawn Guo wrote: > > On Tue, Mar 15, 2011 at 01:41:01AM -0600, Grant Likely wrote: > > > On Tue, Mar 08, 2011 at 11:56:34AM +0800, Shawn Guo wrote: > > > > On Mon, Mar 07, 2011 at 10:53:37AM -0700, Grant Likely wrote: > > > > > On Mon, Mar 7, 2011 at 9:22 AM, Shawn Guo <shawn....@linaro.org> > > > > > wrote: > > > > > > The 'rate' is added for fixed-clock support, while 'pll_base' is for > > > > > > pll clock. These two particular type of clocks are supposed to be > > > > > > gracefully supported by the common clk api when it gets ready. > > > > > > > > > > How does the current imx clock code handle fixed and pll clocks? > > > > > > > > For fixed-clock, the current code gets several variables holding the > > > > rate and then return the rate from several get_rate functions. > > > > > > > > static unsigned long external_high_reference, external_low_reference; > > > > static unsigned long oscillator_reference, ckih2_reference; > > > > > > > > static unsigned long get_high_reference_clock_rate(struct clk *clk) > > > > { > > > > return external_high_reference; > > > > } > > > > > > > > static unsigned long get_low_reference_clock_rate(struct clk *clk) > > > > { > > > > return external_low_reference; > > > > } > > > > > > > > static unsigned long get_oscillator_reference_clock_rate(struct clk > > > > *clk) > > > > { > > > > return oscillator_reference; > > > > } > > > > > > > > static unsigned long get_ckih2_reference_clock_rate(struct clk *clk) > > > > { > > > > return ckih2_reference; > > > > } > > > > > > > > With this new rate member added, all these can be consolidated into one. > > > > > > > > For base address of pll, the current code uses the reference to clocks > > > > statically defined to know which pll is the one. > > > > > > > > static inline void __iomem *_mx51_get_pll_base(struct clk *pll) > > > > { > > > > #ifdef CONFIG_OF > > > > return pll->pll_base; > > > > #else > > > > if (pll == &pll1_main_clk) > > > > return MX51_DPLL1_BASE; > > > > else if (pll == &pll2_sw_clk) > > > > return MX51_DPLL2_BASE; > > > > else if (pll == &pll3_sw_clk) > > > > return MX51_DPLL3_BASE; > > > > else > > > > BUG(); > > > > > > > > return NULL; > > > > #endif > > > > > > Be careful about stuff like this. Remember that enabling CONFIG_OF > > > must *not break* board support that does not use the device tree. The > > > above #ifdef block will break existing users. > > > > > Though the code has been killed in the latest version I just sent > > yesterday I sent last night, I do not understand how it will break > > the existing users. The existing code is: > > > > static inline void __iomem *_mx51_get_pll_base(struct clk *pll) > > { > > if (pll == &pll1_main_clk) > > return MX51_DPLL1_BASE; > > else if (pll == &pll2_sw_clk) > > return MX51_DPLL2_BASE; > > else if (pll == &pll3_sw_clk) > > return MX51_DPLL3_BASE; > > else > > BUG(); > > > > return NULL; > > } > > What you wrote wrapped the current implementation with #ifdef > CONFIG_OF ... #else [existing code] #endif. That says to me that when > CONFIG_OF is enabled, the old code gets compiled out, which means the > function no longer works on non-dt platforms. > > The goal is to support both dt and non-dt machines with a single > kernel image. > Ah, I missed this point. Thanks.
-- Regards, Shawn _______________________________________________ linaro-dev mailing list linaro-dev@lists.linaro.org http://lists.linaro.org/mailman/listinfo/linaro-dev