[RFC v2 4/5] clk: per-user clock accounting for debug
Tomeu Vizoso
tomeu.vizoso at collabora.com
Thu Jul 10 03:51:56 PDT 2014
On 07/09/2014 10:01 PM, Tomasz Figa wrote:
> Hi Tomeu, Rabin,
>
> Please see my comments inline.
>
> On 03.07.2014 16:38, Tomeu Vizoso wrote:
>> From: Rabin Vincent <rabin.vincent at stericsson.com>
>>
>> When a clock has multiple users, the WARNING on imbalance of
>> enable/disable may not show the guilty party since although they may
>> have commited the error earlier, the warning is emitted later when some
>> other user, presumably innocent, disables the clock.
>
> [snip]
>
>> diff --git a/drivers/clk/clk-devres.c b/drivers/clk/clk-devres.c
>> index 5f2aba9..d6a5e59 100644
>> --- a/drivers/clk/clk-devres.c
>> +++ b/drivers/clk/clk-devres.c
>> @@ -37,7 +37,8 @@ EXPORT_SYMBOL(devm_clk_provider_get);
>>
>> struct clk *devm_clk_get(struct device *dev, const char *id)
>> {
>> - return clk_core_to_clk(devm_clk_provider_get(dev, id));
>> + const char *dev_id = dev ? dev_name(dev) : NULL;
>> + return clk_core_to_clk(devm_clk_provider_get(dev, id), dev_id, id);
>
> Hmm, correct me if I'm missing something but you're just calling
> devm_clk_provider() which guarantees calling of clk_provider_put() on
> device removal, but what about the consumer struct being created here?
>
> Shouldn't you rather call normal clk_provider_get() here, then create a
> consumer struct for it and then wrap it up using devres, providing
> appropriate release function?
You are totally right.
>> }
>> EXPORT_SYMBOL(devm_clk_get);
>>
>> diff --git a/drivers/clk/clk-highbank.c b/drivers/clk/clk-highbank.c
>> index cc1bae1..4e38856 100644
>> --- a/drivers/clk/clk-highbank.c
>> +++ b/drivers/clk/clk-highbank.c
>> @@ -331,7 +331,7 @@ CLK_OF_DECLARE(hb_a9periph, "calxeda,hb-a9periph-clock", hb_a9periph_init);
>> static void __init hb_a9bus_init(struct device_node *node)
>> {
>> struct clk_core *clk = hb_clk_init(node, &a9bclk_ops);
>> - clk_prepare_enable(clk_core_to_clk(clk));
>> + clk_prepare_enable(clk_core_to_clk(clk, node->full_name, NULL));
>> }
>> CLK_OF_DECLARE(hb_a9bus, "calxeda,hb-a9bus-clock", hb_a9bus_init);
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index 644a824..b887c69 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -568,6 +568,28 @@ static int clk_disable_unused(void)
>> }
>> late_initcall_sync(clk_disable_unused);
>>
>> +struct clk *clk_core_to_clk(struct clk_core *clk_core, const char *dev,
>> + const char *con)
>
> Name of this function suggests that this is just a simple conversion,
> while it creates new object. I would suggest calling this
> clk_create_consumer() or something along these lines.
Agreed, that sounds better.
>> +{
>> + struct clk *clk;
>> +
>> + clk = kzalloc(sizeof(*clk), GFP_KERNEL);
>> + if (!clk)
>> + return ERR_PTR(-ENOMEM);
>
> [snip]
>
>> +static void clk_free_clk(struct clk *clk)
>> +{
>> + kfree(clk);
>> +}
>
> Any need for this one-liner? Seems to be used just once below.
Yeah, should probably remove it from this patch and only reintroduce it
later when it's actually needed.
>> +
>> void clk_put(struct clk *clk)
>> {
>> - __clk_put(clk_to_clk_core(clk));
>> + clk_core_t *core = clk_to_clk_core(clk);
>> +
>> + clk_free_clk(clk);
>> + __clk_put(core);
>> }
>> EXPORT_SYMBOL(clk_put);
>>
>> diff --git a/drivers/clk/qcom/mmcc-msm8960.c b/drivers/clk/qcom/mmcc-msm8960.c
>> index 28a5d30..47f21ea 100644
>> --- a/drivers/clk/qcom/mmcc-msm8960.c
>> +++ b/drivers/clk/qcom/mmcc-msm8960.c
>> @@ -470,7 +470,8 @@ static int pix_rdi_set_parent(struct clk_hw *hw, u8 index)
>> * needs to be on at what time.
>> */
>> for (i = 0; i < num_parents; i++) {
>> - ret = clk_prepare_enable(clk_core_to_clk(clk_get_parent_by_index(clk, i)));
>> + struct clk_core *parent = clk_get_parent_by_index(clk, i);
>> + ret = clk_prepare_enable(clk_core_to_clk(parent, NULL, __clk_get_name(parent)));
>
> Shouldn't this use clk_provider_*() instead? IMHO clk_core_to_clk() and
> clk_to_clk_core() shouldn't be available to clock drivers, just the core
> clock code. Consumers should operate on struct clks alone, clock drivers
> should operate only on struct clk_cores and only the core code should be
> responsible for necessary conversions.
Totally, this is already fixed in my local version.
>> if (ret)
>> goto err;
>> }
>> @@ -498,8 +499,10 @@ static int pix_rdi_set_parent(struct clk_hw *hw, u8 index)
>> udelay(1);
>>
>> err:
>> - for (i--; i >= 0; i--)
>> - clk_disable_unprepare(clk_core_to_clk(clk_get_parent_by_index(clk, i)));
>> + for (i--; i >= 0; i--) {
>> + struct clk_core *parent = clk_get_parent_by_index(clk, i);
>> + clk_disable_unprepare(clk_core_to_clk(parent, NULL, __clk_get_name(parent)));
>
> Hmm. Memory leak? Every time this operation is called you create a
> consumer struct for each parent two times.
Yes, fortunately this won't be needed as we'll have
clk_provider_disable_unprepare and friends.
>> + }
>>
>> return ret;
>> }
>> diff --git a/drivers/clk/sunxi/clk-sunxi.c b/drivers/clk/sunxi/clk-sunxi.c
>> index 5de44c1..550a691 100644
>> --- a/drivers/clk/sunxi/clk-sunxi.c
>> +++ b/drivers/clk/sunxi/clk-sunxi.c
>> @@ -1196,7 +1196,7 @@ static void __init sunxi_init_clocks(const char *clocks[], int nclocks)
>> struct clk_core *clk = clk_provider_get(NULL, clocks[i]);
>>
>> if (!IS_ERR(clk))
>> - clk_prepare_enable(clk_core_to_clk(clk));
>> + clk_prepare_enable(clk_core_to_clk(clk, NULL, clocks[i]));
>
> Probably should use clk_provider_*()?
>
>> }
>> }
>>
>> diff --git a/drivers/clk/tegra/clk.c b/drivers/clk/tegra/clk.c
>> index 385d101..2a03d47 100644
>> --- a/drivers/clk/tegra/clk.c
>> +++ b/drivers/clk/tegra/clk.c
>> @@ -228,7 +228,7 @@ void __init tegra_init_from_table(struct tegra_clk_init_table *tbl,
>> }
>>
>> if (tbl->state)
>> - if (clk_prepare_enable(clk_core_to_clk(clk))) {
>> + if (clk_prepare_enable(clk_core_to_clk(clk, NULL, __clk_get_name(clk)))) {
>
> Ditto. And all similar cases below which I skipped due to lack of any
> added value to this review.
>
>> pr_err("%s: Failed to enable %s\n", __func__,
>> __clk_get_name(clk));
>> WARN_ON(1);
>
> [snip]
>
>> diff --git a/include/linux/clk-private.h b/include/linux/clk-private.h
>> index 2c1ece9..9657fc8 100644
>> --- a/include/linux/clk-private.h
>> +++ b/include/linux/clk-private.h
>> @@ -57,6 +57,10 @@ struct clk_core {
>>
>> struct clk {
>> struct clk_core *core;
>> + unsigned int enable_count;
>> + const char *dev_id;
>> + const char *con_id;
>> + void *last_disable;
>
> Probably the same as for enables should be done for prepares.
Yes, I think it would be useful as well.
>> };
>>
>> /*
>> diff --git a/include/linux/clk-provider.h b/include/linux/clk-provider.h
>> index 87de32c..abfc31a 100644
>> --- a/include/linux/clk-provider.h
>> +++ b/include/linux/clk-provider.h
>> @@ -533,7 +533,8 @@ struct clk_core *clk_provider_get_sys(const char *dev_id, const char *con_id);
>> struct clk_core *clk_provider_get(struct device *dev, const char *con_id);
>> struct clk_core *devm_clk_provider_get(struct device *dev, const char *id);
>>
>> -#define clk_core_to_clk(core) ((struct clk *)(core))
>
> Btw. this should have been a static inline.
Agreed.
Thanks,
Tomeu
> Best regards,
> Tomasz
>
More information about the linux-arm-kernel
mailing list