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

Message ID 1299514932-13558-3-git-send-email-shawn.guo@linaro.org
State New
Headers show

Commit Message

Shawn Guo March 7, 2011, 4:22 p.m.
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.

Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
---
 arch/arm/plat-mxc/include/mach/clock.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

Comments

Grant Likely March 7, 2011, 5:53 p.m. | #1
On Mon, Mar 7, 2011 at 9:22 AM, Shawn Guo <shawn.guo@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?
Using the dt shouldn't require any special treatment in this regard.

g.

>
> Signed-off-by: Shawn Guo <shawn.guo@linaro.org>
> ---
>  arch/arm/plat-mxc/include/mach/clock.h |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
>
> diff --git a/arch/arm/plat-mxc/include/mach/clock.h b/arch/arm/plat-mxc/include/mach/clock.h
> index 753a598..a29dc45 100644
> --- a/arch/arm/plat-mxc/include/mach/clock.h
> +++ b/arch/arm/plat-mxc/include/mach/clock.h
> @@ -38,6 +38,10 @@ struct clk {
>        /* Register address for clock's enable/disable control. */
>        void __iomem *enable_reg;
>        u32 flags;
> +       /* clock rate used by fixed-clock */
> +       unsigned long rate;
> +       /* base address of pll */
> +       void __iomem *pll_base;
>        /* get the current clock rate (always a fresh value) */
>        unsigned long (*get_rate) (struct clk *);
>        /* Function ptr to set the clock to a new rate. The rate must match a
> --
> 1.7.1
>
>
> _______________________________________________
> linaro-dev mailing list
> linaro-dev@lists.linaro.org
> http://lists.linaro.org/mailman/listinfo/linaro-dev
>
Shawn Guo March 8, 2011, 3:56 a.m. | #2
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@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
}

static inline void __iomem *_get_pll_base(struct clk *pll)
{
        if (cpu_is_mx51())
                return _mx51_get_pll_base(pll);
        else
                return _mx53_get_pll_base(pll);
}

> Using the dt shouldn't require any special treatment in this regard.
> 
I would say these two members were added to make dt clock code simple
and good.
Shawn Guo March 13, 2011, 3:19 p.m. | #3
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@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.
> 
I just noticed that the references to clocks statically created are 
being used in the current clock code widely, and I need to work around
it 'globally' anyway, so I will not add the new member 'pll_base'.

> 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
> }
> 
> static inline void __iomem *_get_pll_base(struct clk *pll)
> {
>         if (cpu_is_mx51())
>                 return _mx51_get_pll_base(pll);
>         else
>                 return _mx53_get_pll_base(pll);
> }
> 
> > Using the dt shouldn't require any special treatment in this regard.
> > 
> I would say these two members were added to make dt clock code simple
> and good.
Grant Likely March 15, 2011, 7:41 a.m. | #4
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@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.

g.
Shawn Guo March 15, 2011, 7:50 a.m. | #5
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@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;
}
Grant Likely March 15, 2011, 8:02 a.m. | #6
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@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.

g.

> 
> -- 
> Regards,
> Shawn
>
Shawn Guo March 15, 2011, 8:05 a.m. | #7
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@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.

Patch

diff --git a/arch/arm/plat-mxc/include/mach/clock.h b/arch/arm/plat-mxc/include/mach/clock.h
index 753a598..a29dc45 100644
--- a/arch/arm/plat-mxc/include/mach/clock.h
+++ b/arch/arm/plat-mxc/include/mach/clock.h
@@ -38,6 +38,10 @@  struct clk {
 	/* Register address for clock's enable/disable control. */
 	void __iomem *enable_reg;
 	u32 flags;
+	/* clock rate used by fixed-clock */
+	unsigned long rate;
+	/* base address of pll */
+	void __iomem *pll_base;
 	/* get the current clock rate (always a fresh value) */
 	unsigned long (*get_rate) (struct clk *);
 	/* Function ptr to set the clock to a new rate. The rate must match a