Subject: [RFC 05/24] ARM: omap: clk: Nuke plat clock.c & clock.h if CONFIG_COMMON_CLK

From: [email-masked] (Rajendra Nayak)
Date: Tue, 05 Jun 2012 10:28:05 +0530

Hi Jon,

> On 06/04/2012 09:16 AM, Rajendra Nayak wrote:
>>
>>>> +/* struct clksel_rate.flags possibilities */
>>>> +#define RATE_IN_242X        (1<<   0)
>>>> +#define RATE_IN_243X        (1<<   1)
>>>> +#define RATE_IN_3430ES1        (1<<   2)    /* 3430ES1 rates only */
>>>> +#define RATE_IN_3430ES2PLUS    (1<<   3)    /* 3430 ES>= 2 rates only */
>>>> +#define RATE_IN_36XX        (1<<   4)
>>>> +#define RATE_IN_4430        (1<<   5)
>>>> +#define RATE_IN_TI816X        (1<<   6)
>>>> +#define RATE_IN_4460        (1<<   7)
>>>> +#define RATE_IN_AM33XX        (1<<   8)
>>>> +#define RATE_IN_TI814X        (1<<   9)
>>>> +
>>>> +#define RATE_IN_24XX        (RATE_IN_242X | RATE_IN_243X)
>>>> +#define RATE_IN_34XX        (RATE_IN_3430ES1 | RATE_IN_3430ES2PLUS)
>>>> +#define RATE_IN_3XXX        (RATE_IN_34XX | RATE_IN_36XX)
>>>> +#define RATE_IN_44XX        (RATE_IN_4430 | RATE_IN_4460)
>>>> +
>>>> +/* RATE_IN_3430ES2PLUS_36XX includes 34xx/35xx with ES>=2, and all
>>>> 36xx/37xx */
>>>> +#define RATE_IN_3430ES2PLUS_36XX    (RATE_IN_3430ES2PLUS |
>>>> RATE_IN_36XX)
>>>> +
>>>> +/* Platform flags for the clkdev-OMAP integration code */
>>>> +#define CK_242X        (1<<   4)
>>>> +#define CK_243X        (1<<   5)        /* 243x, 253x */
>>>> +#define CK_3430ES1    (1<<   6)        /* 34xxES1 only */
>>>> +#define CK_3430ES2PLUS    (1<<   7)        /* 34xxES2, ES3,
>>>> non-Sitara 35xx only */
>>>> +#define CK_3505        (1<<   8)
>>>> +#define CK_3517        (1<<   9)
>>>> +#define CK_36XX        (1<<   10)       /* 36xx/37xx-specific clocks */
>>>> +#define CK_443X        (1<<   11)
>>>> +#define CK_TI816X    (1<<   12)
>>>> +#define CK_446X        (1<<   13)
>>>> +
>>>> +#define CK_34XX        (CK_3430ES1 | CK_3430ES2PLUS)
>>>> +#define CK_AM35XX    (CK_3505 | CK_3517)     /* all Sitara AM35xx */
>>>> +#define CK_3XXX        (CK_34XX | CK_AM35XX | CK_36XX)
>>>
>>> I am not sure why we should duplicate these defines in an OMAP2 specific
>>> header. What not just leave in plat clock.h where we have all the
>>> RATE_IN_xxx and CK_xxx for all OMAP devices?
>>
>> These get removed from the file which is used for OMAP1 in a later
>> patch. Like I said the idea was to separate out whats needed for OMAP1
>> (using legacy struct clk) and OMAP2+ (using common struct clk) with
>> both headers residing in respective mach-omap folders. (The RFC I posted
>> still had the OMAP1 file in plat-omap)
>
> But these definitions are unrelated to whether you use common clock or
> legacy. So why move them?

I am really fine either way, I don;t see a big issue in keeping these
in plat clock.h and some others with ifdefs around for COMMON_CLK.
The bigger issue is moving OMAP1 to common clk and I don't plan to do
it as part of this series.

>
>>>
>>>> +/**
>>>> + * struct clksel_rate - register bitfield values corresponding to
>>>> clk divisors
>>>> + * @val: register bitfield value (shifted to bit 0)
>>>> + * @div: clock divisor corresponding to @val
>>>> + * @flags: (see "struct clksel_rate.flags possibilities" above)
>>>> + *
>>>> + * @val should match the value of a read from struct clk.clksel_reg
>>>> + * AND'ed with struct clk.clksel_mask, shifted right to bit 0.
>>>> + *
>>>> + * @div is the divisor that should be applied to the parent clock's
>>>> rate
>>>> + * to produce the current clock's rate.
>>>> + */
>>>> +struct clksel_rate {
>>>> +    u32    val;
>>>> +    u8    div;
>>>> +    u8    flags;
>>>> +};
>>>> +
>>>> +/**
>>>> + * struct clksel - available parent clocks, and a pointer to their
>>>> divisors
>>>> + * @parent: struct clk * to a possible parent clock
>>>> + * @rates: available divisors for this parent clock
>>>> + *
>>>> + * A struct clksel is always associated with one or more struct clks
>>>> + * and one or more struct clksel_rates.
>>>> + */
>>>> +struct clksel {
>>>> +    struct clk            *parent;
>>>> +    const struct clksel_rate    *rates;
>>>> +};
>>>
>>> These above clksel structures would be need for omap1 devices so that we
>>> could use the clock framework to set the parent clock. So why not keep
>>> in plat clock.h?
>>
>> We could, but that alone wouldn't be enough if we move OMAP2+ alone to
>> common clk, it would mean we duplicate the clksel handling code too,
>> and if we do that, maybe its not that bad to just duplicate a couple
>> more struct definitions.
>
> So I have tested the legacy clksel code on omap1 and it works. So I
> would hope that the common clock version of clksel would work too for
> omap1 (if we move omap1 to the common clock framework).

Yes, *if we move omap1 to common clock* :-)

regards,
Rajendra

>
>>>> +
>>>> +struct clk_hw_omap_ops;
>>>> +
>>>> +struct clk_hw_omap {
>>>> +    struct clk_hw        hw;
>>>> +    struct list_head    node;
>>>> +    unsigned long        fixed_rate;
>>>> +    u8            fixed_div;
>>>> +    void __iomem        *enable_reg;
>>>> +    u8            enable_bit;
>>>> +    u8            flags;
>>>> +#ifdef CONFIG_ARCH_OMAP2PLUS
>>>> +    void __iomem        *clksel_reg;
>>>> +    u32            clksel_mask;
>>>> +    const struct clksel    *clksel;
>>>> +    struct dpll_data    *dpll_data;
>>>> +    const char        *clkdm_name;
>>>> +    struct clockdomain    *clkdm;
>>>> +#else
>>>> +    u8            rate_offset;
>>>> +    u8            src_offset;
>>>> +#endif
>>>> +    const struct clk_hw_omap_ops    *ops;
>>>> +};
>>>> +
>>>> +struct clk_hw_omap_ops {
>>>> +    void            (*find_idlest)(struct clk_hw_omap *oclk,
>>>> +                    void __iomem **idlest_reg,
>>>> +                    u8 *idlest_bit, u8 *idlest_val);
>>>> +    void            (*find_companion)(struct clk_hw_omap *oclk,
>>>> +                    void __iomem **other_reg,
>>>> +                    u8 *other_bit);
>>>> +    void            (*allow_idle)(struct clk_hw_omap *oclk);
>>>> +    void            (*deny_idle)(struct clk_hw_omap *oclk);
>>>> +};
>>>
>>> The above clk_hw_xxx would also be needed for omap1 too, right?
>>
>> Yes, when OMAP1 moves to common clk *and* if we find enough
>> commonalities in clk_hw_xxxx accross OMAP1 and OMAP2+.
>> Else it would make sense to keep them in separate mach folders.
>
> I guess I was hoping that these would be put somewhere in plat-omap, may
> be cclock.h so that the intent would be so that omap1 could use them. By
> putting them in mach-omap2, then if we find that omap1 can use them we
> are going to need to move them.
>
> Moving all this into mach-omap2, appears to be going the other direction
> I was expecting. In other words, really separating omap1 and omap2 code.
> I was hoping we could consolidate it more.
>
> Cheers
> Jon