[PATCH 2/5] arm/mxc: add clk members to ease dt clock support

[Shawn Guo]

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.guo at 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