by Prashant Gaikwad

[permalink] [raw]

Subject: Re: [PATCH 4/6] ARM: tegra: Add clk_tegra structure and helper functions

On Thursday 28 June 2012 11:58 PM, Mike Turquette wrote:
> On 20120628-16:07, Prashant Gaikwad wrote:
>> Add Tegra platform specific clock structure clk_tegra and
>> some helper functions for generic clock framework.
>>
>> struct clk_tegra is the single strcture used for all types of
>> clocks. reset and cfg_ex ops moved to clk_tegra from clk_ops.
>>
>> Signed-off-by: Prashant Gaikwad<[email protected]>
> Hi Prashant,
>
> I'm happy to see Tegra getting ported over to the common clk framework.

Thanks Mike!! It will help to get in-line with recent developments.

> <snip>
>> +void tegra_clk_add(struct clk *clk)
>> +{
>> + struct clk_tegra *c = to_clk_tegra(__clk_get_hw(clk));
>> +
>> + mutex_lock(&clock_list_lock);
>> + list_add(&c->node,&clocks);
>> + mutex_unlock(&clock_list_lock);
>> +}
>> +
>> +struct clk *tegra_get_clock_by_name(const char *name)
>> +{
>> + struct clk_tegra *c;
>> + struct clk *ret = NULL;
>> + mutex_lock(&clock_list_lock);
>> + list_for_each_entry(c,&clocks, node) {
>> + if (strcmp(__clk_get_name(c->hw.clk), name) == 0) {
>> + ret = c->hw.clk;
>> + break;
>> + }
>> + }
>> + mutex_unlock(&clock_list_lock);
>> + return ret;
>> +}
>> +
> Are you planning to continue using an internal list of struct clk_tegra?
> OMAP had a similar list for the legacy clock framework but that was
> since removed and now lookups are done solely through clkdev.

No, will move to clkdev later.

> <snip>
>> +void tegra_periph_reset_deassert(struct clk *c)
>> +{
>> + struct clk_tegra *clk = to_clk_tegra(__clk_get_hw(c));
>> + BUG_ON(!clk->reset);
>> + clk->reset(__clk_get_hw(c), false);
>> +}
>> +EXPORT_SYMBOL(tegra_periph_reset_deassert);
>> +
>> +void tegra_periph_reset_assert(struct clk *c)
>> +{
>> + struct clk_tegra *clk = to_clk_tegra(__clk_get_hw(c));
>> + BUG_ON(!clk->reset);
>> + clk->reset(__clk_get_hw(c), true);
>> +}
>> +EXPORT_SYMBOL(tegra_periph_reset_assert);
>> +
>> +/* Several extended clock configuration bits (e.g., clock routing, clock
>> + * phase control) are included in PLL and peripheral clock source
>> + * registers. */
>> +int tegra_clk_cfg_ex(struct clk *c, enum tegra_clk_ex_param p, u32 setting)
>> +{
>> + int ret = 0;
>> + struct clk_tegra *clk = to_clk_tegra(__clk_get_hw(c));
>> +
>> + if (!clk->clk_cfg_ex) {
>> + ret = -ENOSYS;
>> + goto out;
>> + }
>> + ret = clk->clk_cfg_ex(__clk_get_hw(c), p, setting);
>> +
>> +out:
>> + return ret;
>> +}
> We had some discussions in the past on your clock reset and external
> line request operations which you've had to put into struct clk_tegra.
>
> Do you need to expose those ops to code in drivers/*? I consider that a
> reasonable litmus test to start considering if something should be moved
> into the generic clk.h api.

Yes, we need these ops in drivers. Peter has sent a patch proposing to
move these ops to generic clk.

In addition, we also need mechanism/ops to change rate and parent from
clk_ops implementation. There was some discussion but I do not know the
latest status.

> <snip>
>> +enum clk_state {
>> + UNINITIALIZED = 0,
>> + ON,
>> + OFF,
>> +};
> How is clk_state used? Might it be helpful to you if that was in the
> generic code in drivers/clk/clk.c?

clk_state is used to store the clock state. I am planning to remove this
since is_enabled ops should work.

> <snip>
>> +struct clk_tegra {
>> + /* node for master clocks list */
>> + struct list_head node; /* node for list of all clocks */
>> + struct clk_lookup lookup;
>> + struct clk_hw hw;
>> +
>> + bool set;
>> + unsigned long fixed_rate;
>> + unsigned long max_rate;
>> + unsigned long min_rate;
>> + u32 flags;
>> + const char *name;
>> +
>> + enum clk_state state;
>> + u32 div;
>> + u32 mul;
>> +
>> + u32 reg;
>> + u32 reg_shift;
>> +
>> + struct list_head shared_bus_list;
>> +
>> + union {
>> + struct {
>> + unsigned int clk_num;
>> + } periph;
>> + struct {
>> + unsigned long input_min;
>> + unsigned long input_max;
>> + unsigned long cf_min;
>> + unsigned long cf_max;
>> + unsigned long vco_min;
>> + unsigned long vco_max;
>> + const struct clk_pll_freq_table *freq_table;
>> + int lock_delay;
>> + unsigned long fixed_rate;
>> + } pll;
>> + struct {
>> + u32 sel;
>> + u32 reg_mask;
>> + } mux;
>> + struct {
>> + struct clk *main;
>> + struct clk *backup;
>> + } cpu;
>> + struct {
>> + struct list_head node;
>> + bool enabled;
>> + unsigned long rate;
>> + } shared_bus_user;
>> + } u;
>> +
>> + void (*reset)(struct clk_hw *, bool);
>> + int (*clk_cfg_ex)(struct clk_hw *, enum tegra_clk_ex_param, u32);
>> +};
> I know this is an initial effort but I hope in time that struct
> clk_tegra can be broken up into separate clk types. It would be even
> better if some of the Tegra clocks code be converted over to the common
> clk types in drivers/clk/clk-*.c

Yes, that is next in my action list. One approach I am working on is to
model the clock as Mux -> Divider -> Gate by which we can use some of
common clock types.
But this will take time.

> Regards,
> Mike

2012-06-29 17:24:13

by Stephen Warren

[permalink] [raw]

Subject: Re: [PATCH 0/6] Port Tegra to generic clk framework

On 06/28/2012 04:37 AM, Prashant Gaikwad wrote:
> This patch set ports Tegra clock code to generic clock framework.
>
> Depends on
> [PATCH v3 0/9] Add clk_prepare/clk_unprepare series
> [PATCH] ARM: tegra: Remove second instance of uart clk
> [PATCH] dma: tegra: add clk_prepare/clk_unprepare
> [PATCH] ARM: tegra: dma: rename driver name for clock to "tegra-apbdma"

Prashant, this doesn't apply either to my local development tree
(roughly what's in next-20120629), nor Tegra's for-3.6/common-clk
branch. Can you please regenerate the series on top of
for-3.6/common-clk. Thanks.