Until now the clock providers were initialized in the order found in
the device tree. This led to have the dependencies between the clocks
not respected: children clocks could be initialized before their
parent clocks.
Instead of forcing each platform to manage its own initialization order,
this patch adds this work inside the framework itself.
Using the data of the device tree the of_clk_init function now delayed
the initialization of a clock provider if its parent provider was not
ready yet.
Signed-off-by: Gregory CLEMENT <[email protected]>
---
Mike,
this patch could solve the issues we get on severals mvebu platform
since 3.14-rc1. This is an alternate solution of the patch set sent by
Sebastian. However as it modifies the clock framework itself, it is
more sensible.
I find this solution more elegant than changing the order of the
initialization of the clock at the platform level. However as it
should be tested on more platforms that only the mvebu ones, it would
take some time, and I don't want to still have "broken" platform
during more release candidate. So at the end this patch should be part
of the 3.15 kernel.
Thanks,
Gregory
drivers/clk/clk.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
1 file changed, 91 insertions(+), 3 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index 5517944495d8..beb0f8b0c2a0 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2526,24 +2526,112 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
}
EXPORT_SYMBOL_GPL(of_clk_get_parent_name);
+struct clock_provider {
+ of_clk_init_cb_t clk_init_cb;
+ struct device_node *np;
+ struct list_head node;
+};
+
+static LIST_HEAD(clk_provider_list);
+
+/*
+ * This function looks for a parent clock. If there is one, then it
+ * checks that the provider for this parent clock was initialized, in
+ * this case the parent clock will be ready.
+ */
+static int parent_ready(struct device_node *np)
+{
+ struct of_phandle_args clkspec;
+ struct of_clk_provider *provider;
+
+ /*
+ * If there is no clock parent, no need to wait for them, then
+ * we can consider their absence as being ready
+ */
+ if (of_parse_phandle_with_args(np, "clocks", "#clock-cells", 0,
+ &clkspec))
+ return 1;
+
+ /* Check if we have such a provider in our array */
+ list_for_each_entry(provider, &of_clk_providers, link) {
+ if (provider->node == clkspec.np)
+ return 1;
+ }
+
+ return 0;
+}
+
/**
* of_clk_init() - Scan and init clock providers from the DT
* @matches: array of compatible values and init functions for providers.
*
- * This function scans the device tree for matching clock providers and
- * calls their initialization functions
+ * This function scans the device tree for matching clock providers
+ * and calls their initialization functions. It also do it by trying
+ * to follow the dependencies.
*/
void __init of_clk_init(const struct of_device_id *matches)
{
const struct of_device_id *match;
struct device_node *np;
+ struct clock_provider *clk_provider, *next;
+ bool is_init_done;
if (!matches)
matches = &__clk_of_table;
for_each_matching_node_and_match(np, matches, &match) {
of_clk_init_cb_t clk_init_cb = match->data;
- clk_init_cb(np);
+
+
+ if (parent_ready(np)) {
+ /*
+ * The parent clock is ready or there is no
+ * clock parent at all, in this case the
+ * provider can be initialize immediately.
+ */
+ clk_init_cb(np);
+ } else {
+ /*
+ * The parent clock is not ready, this
+ * provider is moved to a list to be
+ * initialized later
+ */
+ struct clock_provider *parent = kzalloc(sizeof(struct clock_provider),
+ GFP_KERNEL);
+
+ parent->clk_init_cb = match->data;
+ parent->np = np;
+ list_add(&parent->node, &clk_provider_list);
+ }
+ }
+
+ while (!list_empty(&clk_provider_list)) {
+ is_init_done = false;
+ list_for_each_entry_safe(clk_provider, next,
+ &clk_provider_list, node) {
+ if (parent_ready(clk_provider->np)) {
+ clk_provider->clk_init_cb(clk_provider->np);
+ list_del(&clk_provider->node);
+ kfree(clk_provider);
+ is_init_done = true;
+ }
+ }
+
+ if (!is_init_done) {
+ /*
+ * We didn't managed to initialize any of the
+ * remaining providers during the last loop,
+ * so now we initialize all the remaining ones
+ * unconditionally in case the clock parent
+ * was not mandatory
+ */
+ list_for_each_entry_safe(clk_provider, next,
+ &clk_provider_list, node) {
+ clk_provider->clk_init_cb(clk_provider->np);
+ list_del(&clk_provider->node);
+ kfree(clk_provider);
+ }
+ }
}
}
#endif
--
1.8.1.2
Mike,
On Tue, Feb 04, 2014 at 11:59:26PM +0100, Gregory CLEMENT wrote:
> Until now the clock providers were initialized in the order found in
> the device tree. This led to have the dependencies between the clocks
> not respected: children clocks could be initialized before their
> parent clocks.
>
> Instead of forcing each platform to manage its own initialization order,
> this patch adds this work inside the framework itself.
>
> Using the data of the device tree the of_clk_init function now delayed
> the initialization of a clock provider if its parent provider was not
> ready yet.
>
> Signed-off-by: Gregory CLEMENT <[email protected]>
> ---
> Mike,
>
> this patch could solve the issues we get on severals mvebu platform
> since 3.14-rc1. This is an alternate solution of the patch set sent by
> Sebastian. However as it modifies the clock framework itself, it is
> more sensible.
>
> I find this solution more elegant than changing the order of the
> initialization of the clock at the platform level. However as it
> should be tested on more platforms that only the mvebu ones, it would
> take some time, and I don't want to still have "broken" platform
> during more release candidate. So at the end this patch should be part
> of the 3.15 kernel.
If we can get a response from Mike, I'd prefer to go with this approach
and let it bake in -next for a while. Hopefully, we could get it in
before -rc4...
thx,
Jason.
> drivers/clk/clk.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 91 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 5517944495d8..beb0f8b0c2a0 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2526,24 +2526,112 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
> }
> EXPORT_SYMBOL_GPL(of_clk_get_parent_name);
>
> +struct clock_provider {
> + of_clk_init_cb_t clk_init_cb;
> + struct device_node *np;
> + struct list_head node;
> +};
> +
> +static LIST_HEAD(clk_provider_list);
> +
> +/*
> + * This function looks for a parent clock. If there is one, then it
> + * checks that the provider for this parent clock was initialized, in
> + * this case the parent clock will be ready.
> + */
> +static int parent_ready(struct device_node *np)
> +{
> + struct of_phandle_args clkspec;
> + struct of_clk_provider *provider;
> +
> + /*
> + * If there is no clock parent, no need to wait for them, then
> + * we can consider their absence as being ready
> + */
> + if (of_parse_phandle_with_args(np, "clocks", "#clock-cells", 0,
> + &clkspec))
> + return 1;
> +
> + /* Check if we have such a provider in our array */
> + list_for_each_entry(provider, &of_clk_providers, link) {
> + if (provider->node == clkspec.np)
> + return 1;
> + }
> +
> + return 0;
> +}
> +
> /**
> * of_clk_init() - Scan and init clock providers from the DT
> * @matches: array of compatible values and init functions for providers.
> *
> - * This function scans the device tree for matching clock providers and
> - * calls their initialization functions
> + * This function scans the device tree for matching clock providers
> + * and calls their initialization functions. It also do it by trying
> + * to follow the dependencies.
> */
> void __init of_clk_init(const struct of_device_id *matches)
> {
> const struct of_device_id *match;
> struct device_node *np;
> + struct clock_provider *clk_provider, *next;
> + bool is_init_done;
>
> if (!matches)
> matches = &__clk_of_table;
>
> for_each_matching_node_and_match(np, matches, &match) {
> of_clk_init_cb_t clk_init_cb = match->data;
> - clk_init_cb(np);
> +
> +
> + if (parent_ready(np)) {
> + /*
> + * The parent clock is ready or there is no
> + * clock parent at all, in this case the
> + * provider can be initialize immediately.
> + */
> + clk_init_cb(np);
> + } else {
> + /*
> + * The parent clock is not ready, this
> + * provider is moved to a list to be
> + * initialized later
> + */
> + struct clock_provider *parent = kzalloc(sizeof(struct clock_provider),
> + GFP_KERNEL);
> +
> + parent->clk_init_cb = match->data;
> + parent->np = np;
> + list_add(&parent->node, &clk_provider_list);
> + }
> + }
> +
> + while (!list_empty(&clk_provider_list)) {
> + is_init_done = false;
> + list_for_each_entry_safe(clk_provider, next,
> + &clk_provider_list, node) {
> + if (parent_ready(clk_provider->np)) {
> + clk_provider->clk_init_cb(clk_provider->np);
> + list_del(&clk_provider->node);
> + kfree(clk_provider);
> + is_init_done = true;
> + }
> + }
> +
> + if (!is_init_done) {
> + /*
> + * We didn't managed to initialize any of the
> + * remaining providers during the last loop,
> + * so now we initialize all the remaining ones
> + * unconditionally in case the clock parent
> + * was not mandatory
> + */
> + list_for_each_entry_safe(clk_provider, next,
> + &clk_provider_list, node) {
> + clk_provider->clk_init_cb(clk_provider->np);
> + list_del(&clk_provider->node);
> + kfree(clk_provider);
> + }
> + }
> }
> }
> #endif
> --
> 1.8.1.2
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> [email protected]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
Hi Greg,
On 04/02/2014 23:59, Gregory CLEMENT wrote:
> Until now the clock providers were initialized in the order found in
> the device tree. This led to have the dependencies between the clocks
> not respected: children clocks could be initialized before their
> parent clocks.
>
> Instead of forcing each platform to manage its own initialization order,
> this patch adds this work inside the framework itself.
>
> Using the data of the device tree the of_clk_init function now delayed
> the initialization of a clock provider if its parent provider was not
> ready yet.
>
Great! I remember having some trouble with this "parent is not
registered yet" issue (but I don't recall how I solved it, maybe in
reordering clk nodes).
Anyway, this will help other platforms too (at least the at91 one).
> Signed-off-by: Gregory CLEMENT <[email protected]>
> ---
> Mike,
>
> this patch could solve the issues we get on severals mvebu platform
> since 3.14-rc1. This is an alternate solution of the patch set sent by
> Sebastian. However as it modifies the clock framework itself, it is
> more sensible.
>
> I find this solution more elegant than changing the order of the
> initialization of the clock at the platform level. However as it
> should be tested on more platforms that only the mvebu ones, it would
> take some time, and I don't want to still have "broken" platform
> during more release candidate. So at the end this patch should be part
> of the 3.15 kernel.
>
> Thanks,
>
> Gregory
>
> drivers/clk/clk.c | 94 +++++++++++++++++++++++++++++++++++++++++++++++++++++--
> 1 file changed, 91 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index 5517944495d8..beb0f8b0c2a0 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2526,24 +2526,112 @@ const char *of_clk_get_parent_name(struct device_node *np, int index)
> }
> EXPORT_SYMBOL_GPL(of_clk_get_parent_name);
>
> +struct clock_provider {
> + of_clk_init_cb_t clk_init_cb;
> + struct device_node *np;
> + struct list_head node;
> +};
> +
> +static LIST_HEAD(clk_provider_list);
> +
> +/*
> + * This function looks for a parent clock. If there is one, then it
> + * checks that the provider for this parent clock was initialized, in
> + * this case the parent clock will be ready.
> + */
> +static int parent_ready(struct device_node *np)
> +{
> + struct of_phandle_args clkspec;
> + struct of_clk_provider *provider;
> +
> + /*
> + * If there is no clock parent, no need to wait for them, then
> + * we can consider their absence as being ready
> + */
> + if (of_parse_phandle_with_args(np, "clocks", "#clock-cells", 0,
> + &clkspec))
> + return 1;
> +
> + /* Check if we have such a provider in our array */
> + list_for_each_entry(provider, &of_clk_providers, link) {
> + if (provider->node == clkspec.np)
> + return 1;
> + }
Shouldn't we wait for all parents to be ready (If I'm right, you're only
waiting for the first parent...) ?
And if you only request for one clk to be ready, why the first one (you
could loop over parent clks and stop when one of its parent is ready) ?
Best Regards,
Boris
> +
> + return 0;
> +}
> +
> /**
> * of_clk_init() - Scan and init clock providers from the DT
> * @matches: array of compatible values and init functions for providers.
> *
> - * This function scans the device tree for matching clock providers and
> - * calls their initialization functions
> + * This function scans the device tree for matching clock providers
> + * and calls their initialization functions. It also do it by trying
> + * to follow the dependencies.
> */
> void __init of_clk_init(const struct of_device_id *matches)
> {
> const struct of_device_id *match;
> struct device_node *np;
> + struct clock_provider *clk_provider, *next;
> + bool is_init_done;
>
> if (!matches)
> matches = &__clk_of_table;
>
> for_each_matching_node_and_match(np, matches, &match) {
> of_clk_init_cb_t clk_init_cb = match->data;
> - clk_init_cb(np);
> +
> +
> + if (parent_ready(np)) {
> + /*
> + * The parent clock is ready or there is no
> + * clock parent at all, in this case the
> + * provider can be initialize immediately.
> + */
> + clk_init_cb(np);
> + } else {
> + /*
> + * The parent clock is not ready, this
> + * provider is moved to a list to be
> + * initialized later
> + */
> + struct clock_provider *parent = kzalloc(sizeof(struct clock_provider),
> + GFP_KERNEL);
> +
> + parent->clk_init_cb = match->data;
> + parent->np = np;
> + list_add(&parent->node, &clk_provider_list);
> + }
> + }
> +
> + while (!list_empty(&clk_provider_list)) {
> + is_init_done = false;
> + list_for_each_entry_safe(clk_provider, next,
> + &clk_provider_list, node) {
> + if (parent_ready(clk_provider->np)) {
> + clk_provider->clk_init_cb(clk_provider->np);
> + list_del(&clk_provider->node);
> + kfree(clk_provider);
> + is_init_done = true;
> + }
> + }
> +
> + if (!is_init_done) {
> + /*
> + * We didn't managed to initialize any of the
> + * remaining providers during the last loop,
> + * so now we initialize all the remaining ones
> + * unconditionally in case the clock parent
> + * was not mandatory
> + */
> + list_for_each_entry_safe(clk_provider, next,
> + &clk_provider_list, node) {
> + clk_provider->clk_init_cb(clk_provider->np);
> + list_del(&clk_provider->node);
> + kfree(clk_provider);
> + }
> + }
> }
> }
> #endif
>
The parent dependency check is only available on the first parent of a given
clk.
Add support for strict dependency check: all parents of a given clk must be
initialized.
Signed-off-by: Boris BREZILLON <[email protected]>
---
Hello Gregory,
This patch adds support for strict check on clk dependencies (check if all
parents specified by an DT clk node are initialized).
I'm not sure this is what you were expecting (maybe testing the first parent
is what you really want), so please feel free to tell me if I'm wrong.
Best Regards,
Boris
drivers/clk/clk.c | 27 +++++++++++++++++++++------
1 file changed, 21 insertions(+), 6 deletions(-)
diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
index beb0f8b..6849769 100644
--- a/drivers/clk/clk.c
+++ b/drivers/clk/clk.c
@@ -2543,22 +2543,37 @@ static int parent_ready(struct device_node *np)
{
struct of_phandle_args clkspec;
struct of_clk_provider *provider;
+ int num_parents;
+ bool found;
+ int i;
/*
* If there is no clock parent, no need to wait for them, then
* we can consider their absence as being ready
*/
- if (of_parse_phandle_with_args(np, "clocks", "#clock-cells", 0,
- &clkspec))
+ num_parents = of_count_phandle_with_args(np, "clocks", "#clock-cells");
+ if (num_parents <= 0)
return 1;
- /* Check if we have such a provider in our array */
- list_for_each_entry(provider, &of_clk_providers, link) {
- if (provider->node == clkspec.np)
+ for (i = 0; i < num_parents; i++) {
+ if (of_parse_phandle_with_args(np, "clocks", "#clock-cells", i,
+ &clkspec))
return 1;
+
+ /* Check if we have such a provider in our array */
+ found = false;
+ list_for_each_entry(provider, &of_clk_providers, link) {
+ if (provider->node == clkspec.np) {
+ found = true;
+ break;
+ }
+ }
+
+ if (!found)
+ return 0;
}
- return 0;
+ return 1;
}
/**
--
1.7.9.5
Hi Boris,
On 05/02/2014 10:48, Boris BREZILLON wrote:
> The parent dependency check is only available on the first parent of a given
> clk.
>
> Add support for strict dependency check: all parents of a given clk must be
> initialized.
>
> Signed-off-by: Boris BREZILLON <[email protected]>
> ---
>
> Hello Gregory,
>
> This patch adds support for strict check on clk dependencies (check if all
> parents specified by an DT clk node are initialized).
>
> I'm not sure this is what you were expecting (maybe testing the first parent
> is what you really want), so please feel free to tell me if I'm wrong.
>
> Best Regards,
>
> Boris
>
> drivers/clk/clk.c | 27 +++++++++++++++++++++------
> 1 file changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
> index beb0f8b..6849769 100644
> --- a/drivers/clk/clk.c
> +++ b/drivers/clk/clk.c
> @@ -2543,22 +2543,37 @@ static int parent_ready(struct device_node *np)
> {
> struct of_phandle_args clkspec;
> struct of_clk_provider *provider;
> + int num_parents;
> + bool found;
> + int i;
>
> /*
> * If there is no clock parent, no need to wait for them, then
> * we can consider their absence as being ready
> */
> - if (of_parse_phandle_with_args(np, "clocks", "#clock-cells", 0,
> - &clkspec))
> + num_parents = of_count_phandle_with_args(np, "clocks", "#clock-cells");
> + if (num_parents <= 0)
> return 1;
>
> - /* Check if we have such a provider in our array */
> - list_for_each_entry(provider, &of_clk_providers, link) {
> - if (provider->node == clkspec.np)
> + for (i = 0; i < num_parents; i++) {
> + if (of_parse_phandle_with_args(np, "clocks", "#clock-cells", i,
> + &clkspec))
> return 1;
> +
> + /* Check if we have such a provider in our array */
> + found = false;
> + list_for_each_entry(provider, &of_clk_providers, link) {
> + if (provider->node == clkspec.np) {
> + found = true;
> + break;
Hum this means that as soon as you have one parent then you consider it
as ready. It is better of what I have done because I only test the 1st
parent. However I wondered if we should go further by ensuring all the
parents are ready.
If I am right, there is more than one parent only for the muxer. In this
case is it really expected that all the parent are ready?
Thanks,
Gregory
> + }
> + }
> +
> + if (!found)
> + return 0;
> }
>
> - return 0;
> + return 1;
> }
>
> /**
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
On 05/02/2014 15:48, Gregory CLEMENT wrote:
> Hi Boris,
>
> On 05/02/2014 10:48, Boris BREZILLON wrote:
>> The parent dependency check is only available on the first parent of a given
>> clk.
>>
>> Add support for strict dependency check: all parents of a given clk must be
>> initialized.
>>
>> Signed-off-by: Boris BREZILLON <[email protected]>
>> ---
>>
>> Hello Gregory,
>>
>> This patch adds support for strict check on clk dependencies (check if all
>> parents specified by an DT clk node are initialized).
>>
>> I'm not sure this is what you were expecting (maybe testing the first parent
>> is what you really want), so please feel free to tell me if I'm wrong.
>>
>> Best Regards,
>>
>> Boris
>>
>> drivers/clk/clk.c | 27 +++++++++++++++++++++------
>> 1 file changed, 21 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>> index beb0f8b..6849769 100644
>> --- a/drivers/clk/clk.c
>> +++ b/drivers/clk/clk.c
>> @@ -2543,22 +2543,37 @@ static int parent_ready(struct device_node *np)
>> {
>> struct of_phandle_args clkspec;
>> struct of_clk_provider *provider;
>> + int num_parents;
>> + bool found;
>> + int i;
>>
>> /*
>> * If there is no clock parent, no need to wait for them, then
>> * we can consider their absence as being ready
>> */
>> - if (of_parse_phandle_with_args(np, "clocks", "#clock-cells", 0,
>> - &clkspec))
>> + num_parents = of_count_phandle_with_args(np, "clocks", "#clock-cells");
>> + if (num_parents <= 0)
>> return 1;
>>
>> - /* Check if we have such a provider in our array */
>> - list_for_each_entry(provider, &of_clk_providers, link) {
>> - if (provider->node == clkspec.np)
>> + for (i = 0; i < num_parents; i++) {
>> + if (of_parse_phandle_with_args(np, "clocks", "#clock-cells", i,
>> + &clkspec))
>> return 1;
>> +
>> + /* Check if we have such a provider in our array */
>> + found = false;
>> + list_for_each_entry(provider, &of_clk_providers, link) {
>> + if (provider->node == clkspec.np) {
>> + found = true;
>> + break;
>
> Hum this means that as soon as you have one parent then you consider it
> as ready. It is better of what I have done because I only test the 1st
> parent. However I wondered if we should go further by ensuring all the
> parents are ready.
My bad, I read the code too fast. Your code already checks that all the
parents are ready.
So if you agree I will merge your code with mine and send a new patch.
>
> If I am right, there is more than one parent only for the muxer. In this
> case is it really expected that all the parent are ready?
>
> Thanks,
>
> Gregory
>
>> + }
>> + }
>> +
>> + if (!found)
>> + return 0;
>> }
>>
>> - return 0;
>> + return 1;
>> }
>>
>> /**
>>
>
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
On 05/02/2014 16:05, Gregory CLEMENT wrote:
> On 05/02/2014 15:48, Gregory CLEMENT wrote:
>> Hi Boris,
>>
>> On 05/02/2014 10:48, Boris BREZILLON wrote:
>>> The parent dependency check is only available on the first parent of a given
>>> clk.
>>>
>>> Add support for strict dependency check: all parents of a given clk must be
>>> initialized.
>>>
>>> Signed-off-by: Boris BREZILLON <[email protected]>
>>> ---
>>>
>>> Hello Gregory,
>>>
>>> This patch adds support for strict check on clk dependencies (check if all
>>> parents specified by an DT clk node are initialized).
>>>
>>> I'm not sure this is what you were expecting (maybe testing the first parent
>>> is what you really want), so please feel free to tell me if I'm wrong.
>>>
>>> Best Regards,
>>>
>>> Boris
>>>
>>> drivers/clk/clk.c | 27 +++++++++++++++++++++------
>>> 1 file changed, 21 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/drivers/clk/clk.c b/drivers/clk/clk.c
>>> index beb0f8b..6849769 100644
>>> --- a/drivers/clk/clk.c
>>> +++ b/drivers/clk/clk.c
>>> @@ -2543,22 +2543,37 @@ static int parent_ready(struct device_node *np)
>>> {
>>> struct of_phandle_args clkspec;
>>> struct of_clk_provider *provider;
>>> + int num_parents;
>>> + bool found;
>>> + int i;
>>>
>>> /*
>>> * If there is no clock parent, no need to wait for them, then
>>> * we can consider their absence as being ready
>>> */
>>> - if (of_parse_phandle_with_args(np, "clocks", "#clock-cells", 0,
>>> - &clkspec))
>>> + num_parents = of_count_phandle_with_args(np, "clocks", "#clock-cells");
>>> + if (num_parents <= 0)
>>> return 1;
>>>
>>> - /* Check if we have such a provider in our array */
>>> - list_for_each_entry(provider, &of_clk_providers, link) {
>>> - if (provider->node == clkspec.np)
>>> + for (i = 0; i < num_parents; i++) {
>>> + if (of_parse_phandle_with_args(np, "clocks", "#clock-cells", i,
>>> + &clkspec))
>>> return 1;
>>> +
>>> + /* Check if we have such a provider in our array */
>>> + found = false;
>>> + list_for_each_entry(provider, &of_clk_providers, link) {
>>> + if (provider->node == clkspec.np) {
>>> + found = true;
>>> + break;
>> Hum this means that as soon as you have one parent then you consider it
>> as ready. It is better of what I have done because I only test the 1st
>> parent. However I wondered if we should go further by ensuring all the
>> parents are ready.
> My bad, I read the code too fast. Your code already checks that all the
> parents are ready.
>
> So if you agree I will merge your code with mine and send a new patch.
That's fine by me.
>
>> If I am right, there is more than one parent only for the muxer. In this
>> case is it really expected that all the parent are ready?
>>
>> Thanks,
>>
>> Gregory
>>
>>> + }
>>> + }
>>> +
>>> + if (!found)
>>> + return 0;
>>> }
>>>
>>> - return 0;
>>> + return 1;
>>> }
>>>
>>> /**
>>>
>>
>
On 02/04/2014 11:59 PM, Gregory CLEMENT wrote:
> Until now the clock providers were initialized in the order found in
> the device tree. This led to have the dependencies between the clocks
> not respected: children clocks could be initialized before their
> parent clocks.
>
> Instead of forcing each platform to manage its own initialization order,
> this patch adds this work inside the framework itself.
>
> Using the data of the device tree the of_clk_init function now delayed
> the initialization of a clock provider if its parent provider was not
> ready yet.
>
> Signed-off-by: Gregory CLEMENT <[email protected]>
> [...]
> this patch could solve the issues we get on severals mvebu platform
> since 3.14-rc1. This is an alternate solution of the patch set sent by
> Sebastian. However as it modifies the clock framework itself, it is
> more sensible.
>
> I find this solution more elegant than changing the order of the
> initialization of the clock at the platform level. However as it
> should be tested on more platforms that only the mvebu ones, it would
> take some time, and I don't want to still have "broken" platform
> during more release candidate. So at the end this patch should be part
> of the 3.15 kernel.
Gregory,
I admit, your patch is more general and I am looking forward to revert
the reorder fix as soon as this is ready :)
BTW, what happened to the early device discussion? Couldn't this be
picked up here to allow -EPROBE_DEFER also for those early drivers?
Sebastian
Hi all,
El 04/02/14 19:59, Gregory CLEMENT escribi?:
> Until now the clock providers were initialized in the order found in
> the device tree. This led to have the dependencies between the clocks
> not respected: children clocks could be initialized before their
> parent clocks.
>
> Instead of forcing each platform to manage its own initialization order,
> this patch adds this work inside the framework itself.
>
> Using the data of the device tree the of_clk_init function now delayed
> the initialization of a clock provider if its parent provider was not
> ready yet.
>
> Signed-off-by: Gregory CLEMENT <[email protected]>
After discussing this on IRC with Ezequiel, I must say I'm not really
fond of this solution, as it adds unnecessary complexity to the
framework and slows down clock registration to solve one particular
problem that can be resolved in a simpler, less intrusive way.
The triggering issue which has resulted on this patch is really a simple
dependency on the name of the parent clock - something that should not
be an issue if DT is used to provide them (see of_clk_get_parent_name),
and even when this is not the case, it should not require poking the
clock tree to find out. It wouldn't be hard to fix however, even if you
cannot modify your device trees to provide proper naming; a tiny patch
like the one below should suffice (Ezequiel has kindly tested it today
on a a370-rd and found it working correctly).
In other words, I feel this patch is a pretty good demonstration of
over-engineering and bloating of an otherwise good framework to cover up
for bugs/bad design on other parts of the kernel. Think about it; why
does the framework require char *parent instead of struct clk *parent to
link a child with their parent? Randomly-ordered registration is a
pretty core principle of it; whether by design or not.
So, until someone has a real reason to enforce dependencies on clock
registration time, I would like this to stay uncommited. And, if the
time comes and something like this is ever needed, *please* make it
optional (ie, have a special macro and separate table to register clocks
which are dependency-sensitive, and do so after registering all the
non-dependency-sensitive clocks).
Cheers,
Emilio
-----8<------
From ffdb49506e3ce92090c15e1f9b37f4d465097ac1 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Emilio=20L=C3=B3pez?= <[email protected]>
Date: Thu, 6 Feb 2014 18:07:07 -0300
Subject: [PATCH] clk: mvebu: fix name dependency during registration time
Currently, mvebu_clk_gating_setup has a silly dependency on clock
registration order just to gather the parent clock name. This is
completely unnecesary, as it supports using an already provided name
via the clk_gating_soc_desc structs, and we can therefore solve this
issue with a 69+/- line patch. But, given that the parent name is
always "tclk" as default-hardcoded on mvebu_coreclk_setup(), we can
just default-hardcode it here too and get away with solving this
problem with a one-liner.
---
drivers/clk/mvebu/common.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/clk/mvebu/common.c b/drivers/clk/mvebu/common.c
index 25ceccf..6c63b43 100644
--- a/drivers/clk/mvebu/common.c
+++ b/drivers/clk/mvebu/common.c
@@ -121,7 +121,7 @@ void __init mvebu_clk_gating_setup(struct
device_node *np,
struct clk_gating_ctrl *ctrl;
struct clk *clk;
void __iomem *base;
- const char *default_parent = NULL;
+ const char *default_parent = "tclk";
int n;
base = of_iomap(np, 0);
--
1.8.5.3
On Fri, Feb 07, 2014 at 10:06:08AM -0300, Emilio L?pez wrote:
[snip a great explanation]
Guys, can I get some Tested-by's on this?
thx,
Jason.
> -----8<------
>
> From ffdb49506e3ce92090c15e1f9b37f4d465097ac1 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Emilio=20L=C3=B3pez?= <[email protected]>
> Date: Thu, 6 Feb 2014 18:07:07 -0300
> Subject: [PATCH] clk: mvebu: fix name dependency during registration time
>
> Currently, mvebu_clk_gating_setup has a silly dependency on clock
> registration order just to gather the parent clock name. This is
> completely unnecesary, as it supports using an already provided name
> via the clk_gating_soc_desc structs, and we can therefore solve this
> issue with a 69+/- line patch. But, given that the parent name is
> always "tclk" as default-hardcoded on mvebu_coreclk_setup(), we can
> just default-hardcode it here too and get away with solving this
> problem with a one-liner.
> ---
> drivers/clk/mvebu/common.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/clk/mvebu/common.c b/drivers/clk/mvebu/common.c
> index 25ceccf..6c63b43 100644
> --- a/drivers/clk/mvebu/common.c
> +++ b/drivers/clk/mvebu/common.c
> @@ -121,7 +121,7 @@ void __init mvebu_clk_gating_setup(struct
> device_node *np,
> struct clk_gating_ctrl *ctrl;
> struct clk *clk;
> void __iomem *base;
> - const char *default_parent = NULL;
> + const char *default_parent = "tclk";
> int n;
>
> base = of_iomap(np, 0);
> --
> 1.8.5.3
On Fri, Feb 07, 2014 at 09:24:30AM -0500, Jason Cooper wrote:
> On Fri, Feb 07, 2014 at 10:06:08AM -0300, Emilio López wrote:
>
> [snip a great explanation]
>
> Guys, can I get some Tested-by's on this?
>
In case someone missed Emilio's comment about it, I gave his oneliner
a test on A370 Reference Design. It worked just as well as Sebastian's.
Tested-by: Ezequiel Garcia <[email protected]>
> > -----8<------
> >
> > From ffdb49506e3ce92090c15e1f9b37f4d465097ac1 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Emilio=20L=C3=B3pez?= <[email protected]>
> > Date: Thu, 6 Feb 2014 18:07:07 -0300
> > Subject: [PATCH] clk: mvebu: fix name dependency during registration time
> >
> > Currently, mvebu_clk_gating_setup has a silly dependency on clock
> > registration order just to gather the parent clock name. This is
> > completely unnecesary, as it supports using an already provided name
> > via the clk_gating_soc_desc structs, and we can therefore solve this
> > issue with a 69+/- line patch. But, given that the parent name is
> > always "tclk" as default-hardcoded on mvebu_coreclk_setup(), we can
> > just default-hardcode it here too and get away with solving this
> > problem with a one-liner.
> > ---
> > drivers/clk/mvebu/common.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/clk/mvebu/common.c b/drivers/clk/mvebu/common.c
> > index 25ceccf..6c63b43 100644
> > --- a/drivers/clk/mvebu/common.c
> > +++ b/drivers/clk/mvebu/common.c
> > @@ -121,7 +121,7 @@ void __init mvebu_clk_gating_setup(struct
> > device_node *np,
> > struct clk_gating_ctrl *ctrl;
> > struct clk *clk;
> > void __iomem *base;
> > - const char *default_parent = NULL;
> > + const char *default_parent = "tclk";
> > int n;
> >
> > base = of_iomap(np, 0);
> > --
> > 1.8.5.3
--
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
On 07/02/2014 15:43, Ezequiel Garcia wrote:
> On Fri, Feb 07, 2014 at 09:24:30AM -0500, Jason Cooper wrote:
>> On Fri, Feb 07, 2014 at 10:06:08AM -0300, Emilio López wrote:
>>
>> [snip a great explanation]
>>
>> Guys, can I get some Tested-by's on this?
>>
>
> In case someone missed Emilio's comment about it, I gave his oneliner
> a test on A370 Reference Design. It worked just as well as Sebastian's.
Well ok it's working but this patch is not better than Sebastian, it is
even worth. I don't think it is a good idea at all to totally ignore the
information given by the device tree.
>
> Tested-by: Ezequiel Garcia <[email protected]>
>
>>> -----8<------
>>>
>>> From ffdb49506e3ce92090c15e1f9b37f4d465097ac1 Mon Sep 17 00:00:00 2001
>>> From: =?UTF-8?q?Emilio=20L=C3=B3pez?= <[email protected]>
>>> Date: Thu, 6 Feb 2014 18:07:07 -0300
>>> Subject: [PATCH] clk: mvebu: fix name dependency during registration time
>>>
>>> Currently, mvebu_clk_gating_setup has a silly dependency on clock
>>> registration order just to gather the parent clock name. This is
>>> completely unnecesary, as it supports using an already provided name
>>> via the clk_gating_soc_desc structs, and we can therefore solve this
>>> issue with a 69+/- line patch. But, given that the parent name is
>>> always "tclk" as default-hardcoded on mvebu_coreclk_setup(), we can
>>> just default-hardcode it here too and get away with solving this
>>> problem with a one-liner.
>>> ---
>>> drivers/clk/mvebu/common.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/clk/mvebu/common.c b/drivers/clk/mvebu/common.c
>>> index 25ceccf..6c63b43 100644
>>> --- a/drivers/clk/mvebu/common.c
>>> +++ b/drivers/clk/mvebu/common.c
>>> @@ -121,7 +121,7 @@ void __init mvebu_clk_gating_setup(struct
>>> device_node *np,
>>> struct clk_gating_ctrl *ctrl;
>>> struct clk *clk;
>>> void __iomem *base;
>>> - const char *default_parent = NULL;
>>> + const char *default_parent = "tclk";
>>> int n;
>>>
>>> base = of_iomap(np, 0);
>>> --
>>> 1.8.5.3
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
El 07/02/14 11:49, Gregory CLEMENT escribió:
> On 07/02/2014 15:43, Ezequiel Garcia wrote:
>> On Fri, Feb 07, 2014 at 09:24:30AM -0500, Jason Cooper wrote:
>>> On Fri, Feb 07, 2014 at 10:06:08AM -0300, Emilio López wrote:
>>>
>>> [snip a great explanation]
>>>
>>> Guys, can I get some Tested-by's on this?
>>>
>>
>> In case someone missed Emilio's comment about it, I gave his oneliner
>> a test on A370 Reference Design. It worked just as well as Sebastian's.
>
> Well ok it's working but this patch is not better than Sebastian, it is
> even worth. I don't think it is a good idea at all to totally ignore the
> information given by the device tree.
With a bit more work, you can replace the clk_get magic with a call to
of_clk_get_parent_name() or similar to be able to keep overriding stuff
from DT. This way it would completely match the behaviour on
mvebu_coreclk_setup (default to "tclk", allow overriding with DT).
Emilio
On 07/02/2014 16:00, Emilio López wrote:
> El 07/02/14 11:49, Gregory CLEMENT escribió:
>> On 07/02/2014 15:43, Ezequiel Garcia wrote:
>>> On Fri, Feb 07, 2014 at 09:24:30AM -0500, Jason Cooper wrote:
>>>> On Fri, Feb 07, 2014 at 10:06:08AM -0300, Emilio López wrote:
>>>>
>>>> [snip a great explanation]
>>>>
>>>> Guys, can I get some Tested-by's on this?
>>>>
>>>
>>> In case someone missed Emilio's comment about it, I gave his oneliner
>>> a test on A370 Reference Design. It worked just as well as Sebastian's.
>>
>> Well ok it's working but this patch is not better than Sebastian, it is
>> even worth. I don't think it is a good idea at all to totally ignore the
>> information given by the device tree.
>
> With a bit more work, you can replace the clk_get magic with a call to
> of_clk_get_parent_name() or similar to be able to keep overriding stuff
> from DT. This way it would completely match the behaviour on
> mvebu_coreclk_setup (default to "tclk", allow overriding with DT).
>
I think you didn't have a look on our implementation: the name of the clock
are created by the driver during the initialization. That's why we need that
the parent clock are initialized before the gating clock. I know that for the
sunxi clock you choose to list all your clock name in the device tree, but we
didn't make this choice on purpose. It is not as trivial as you suggested.
I didn't have a look on the atmel clocks, and I don't know if they have this
kind of issue, as they also have to deal with multiple parents, they may
have different issues.
Gregory
> Emilio
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
Hi Gregory,
El 07/02/14 12:12, Gregory CLEMENT escribió:
> On 07/02/2014 16:00, Emilio López wrote:
>> El 07/02/14 11:49, Gregory CLEMENT escribió:
>>> On 07/02/2014 15:43, Ezequiel Garcia wrote:
>>>> On Fri, Feb 07, 2014 at 09:24:30AM -0500, Jason Cooper wrote:
>>>>> On Fri, Feb 07, 2014 at 10:06:08AM -0300, Emilio López wrote:
>>>>>
>>>>> [snip a great explanation]
>>>>>
>>>>> Guys, can I get some Tested-by's on this?
>>>>>
>>>>
>>>> In case someone missed Emilio's comment about it, I gave his oneliner
>>>> a test on A370 Reference Design. It worked just as well as Sebastian's.
>>>
>>> Well ok it's working but this patch is not better than Sebastian, it is
>>> even worth. I don't think it is a good idea at all to totally ignore the
>>> information given by the device tree.
>>
>> With a bit more work, you can replace the clk_get magic with a call to
>> of_clk_get_parent_name() or similar to be able to keep overriding stuff
>> from DT. This way it would completely match the behaviour on
>> mvebu_coreclk_setup (default to "tclk", allow overriding with DT).
>>
>
> I think you didn't have a look on our implementation:
I did, several times in fact.
> the name of the clock
> are created by the driver during the initialization.
The name of the clock is always "tclk", as hardcoded on the driver,
unless overridden via clock-output-names from DT (see
mvebu_coreclk_setup). Currently none of your DT that I can see does
this, so in practice it's always "tclk".
> That's why we need that
> the parent clock are initialized before the gating clock.
You don't really *need* that. The driver just does that because it may
have been convenient at the time and it worked. Defaulting to "tclk" on
mvebu_clk_gating_setup and overriding it if it turns out it has some
other name on the DT (just like on mvebu_coreclk_setup!) should work as
well, and doesn't require complex, bloaty, dependency management.
Rough, untested patch below, so you get the idea.
Cheers,
Emilio
---
drivers/clk/mvebu/common.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/clk/mvebu/common.c b/drivers/clk/mvebu/common.c
index 25ceccf..730625b 100644
--- a/drivers/clk/mvebu/common.c
+++ b/drivers/clk/mvebu/common.c
@@ -119,19 +119,20 @@ void __init mvebu_clk_gating_setup(struct
device_node *np,
const struct clk_gating_soc_desc *desc)
{
struct clk_gating_ctrl *ctrl;
- struct clk *clk;
void __iomem *base;
- const char *default_parent = NULL;
+ struct of_phandle_args clkspec;
+ const char *default_parent = "tclk";
int n;
base = of_iomap(np, 0);
if (WARN_ON(!base))
return;
- clk = of_clk_get(np, 0);
- if (!IS_ERR(clk)) {
- default_parent = __clk_get_name(clk);
- clk_put(clk);
+ if (!of_parse_phandle_with_args(np, "clocks", "#clock-cells", 0,
&clkspec)) {
+ of_property_read_string_index(clkspec.np, "clock-output-names",
+ clkspec.args_count ? clkspec.args[0] : 0,
+ &default_parent);
+ of_node_put(clkspec.np);
}
ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
--
1.8.5.3
On 07/02/2014 17:16, Emilio López wrote:
> Hi Gregory,
>
> El 07/02/14 12:12, Gregory CLEMENT escribió:
>> On 07/02/2014 16:00, Emilio López wrote:
>>> El 07/02/14 11:49, Gregory CLEMENT escribió:
>>>> On 07/02/2014 15:43, Ezequiel Garcia wrote:
>>>>> On Fri, Feb 07, 2014 at 09:24:30AM -0500, Jason Cooper wrote:
>>>>>> On Fri, Feb 07, 2014 at 10:06:08AM -0300, Emilio López wrote:
>>>>>>
>>>>>> [snip a great explanation]
>>>>>>
>>>>>> Guys, can I get some Tested-by's on this?
>>>>>>
>>>>>
>>>>> In case someone missed Emilio's comment about it, I gave his oneliner
>>>>> a test on A370 Reference Design. It worked just as well as Sebastian's.
>>>>
>>>> Well ok it's working but this patch is not better than Sebastian, it is
>>>> even worth. I don't think it is a good idea at all to totally ignore the
>>>> information given by the device tree.
>>>
>>> With a bit more work, you can replace the clk_get magic with a call to
>>> of_clk_get_parent_name() or similar to be able to keep overriding stuff
>>> from DT. This way it would completely match the behaviour on
>>> mvebu_coreclk_setup (default to "tclk", allow overriding with DT).
>>>
>>
>> I think you didn't have a look on our implementation:
>
> I did, several times in fact.
>
>> the name of the clock
>> are created by the driver during the initialization.
>
> The name of the clock is always "tclk", as hardcoded on the driver,
> unless overridden via clock-output-names from DT (see
> mvebu_coreclk_setup). Currently none of your DT that I can see does
> this, so in practice it's always "tclk".
It's not because currently we always use "tclk" that we should not use
the information given by the device tree. Else it will be very
misleading to have a information written in the dts and doing something
else in the code.
>
>> That's why we need that
>> the parent clock are initialized before the gating clock.
>
> You don't really *need* that. The driver just does that because it may
We need it, see my comment in your code.
> have been convenient at the time and it worked. Defaulting to "tclk" on
> mvebu_clk_gating_setup and overriding it if it turns out it has some
> other name on the DT (just like on mvebu_coreclk_setup!) should work as
> well, and doesn't require complex, bloaty, dependency management.
>
> Rough, untested patch below, so you get the idea.
>
> Cheers,
>
> Emilio
>
>
>
> ---
> drivers/clk/mvebu/common.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/clk/mvebu/common.c b/drivers/clk/mvebu/common.c
> index 25ceccf..730625b 100644
> --- a/drivers/clk/mvebu/common.c
> +++ b/drivers/clk/mvebu/common.c
> @@ -119,19 +119,20 @@ void __init mvebu_clk_gating_setup(struct
> device_node *np,
> const struct clk_gating_soc_desc *desc)
> {
> struct clk_gating_ctrl *ctrl;
> - struct clk *clk;
> void __iomem *base;
> - const char *default_parent = NULL;
> + struct of_phandle_args clkspec;
> + const char *default_parent = "tclk";
> int n;
>
> base = of_iomap(np, 0);
> if (WARN_ON(!base))
> return;
>
> - clk = of_clk_get(np, 0);
> - if (!IS_ERR(clk)) {
> - default_parent = __clk_get_name(clk);
> - clk_put(clk);
> + if (!of_parse_phandle_with_args(np, "clocks", "#clock-cells", 0,
> &clkspec)) {
> + of_property_read_string_index(clkspec.np, "clock-output-names",
> + clkspec.args_count ? clkspec.args[0] : 0,
> + &default_parent);
> + of_node_put(clkspec.np);
OK here you duplicate the code from of_clk_get_parent_name, I wonder why
you didn't use the function.But whatever.
Here you will get default_parent = "mvebu-sar" which is the name of the node, you
can't have "tclk" because this name is not in the device tree, but it will be
created by the initialization of the core clocks.
> }
>
> ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL);
>
--
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com
Hi,
El 07/02/14 15:10, Gregory CLEMENT escribió:
(snip)
>> ---
>> drivers/clk/mvebu/common.c | 13 +++++++------
>> 1 file changed, 7 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/clk/mvebu/common.c b/drivers/clk/mvebu/common.c
>> index 25ceccf..730625b 100644
>> --- a/drivers/clk/mvebu/common.c
>> +++ b/drivers/clk/mvebu/common.c
>> @@ -119,19 +119,20 @@ void __init mvebu_clk_gating_setup(struct
>> device_node *np,
>> const struct clk_gating_soc_desc *desc)
>> {
>> struct clk_gating_ctrl *ctrl;
>> - struct clk *clk;
>> void __iomem *base;
>> - const char *default_parent = NULL;
>> + struct of_phandle_args clkspec;
>> + const char *default_parent = "tclk";
>> int n;
>>
>> base = of_iomap(np, 0);
>> if (WARN_ON(!base))
>> return;
>>
>> - clk = of_clk_get(np, 0);
>> - if (!IS_ERR(clk)) {
>> - default_parent = __clk_get_name(clk);
>> - clk_put(clk);
>> + if (!of_parse_phandle_with_args(np, "clocks", "#clock-cells", 0,
>> &clkspec)) {
>> + of_property_read_string_index(clkspec.np, "clock-output-names",
>> + clkspec.args_count ? clkspec.args[0] : 0,
>> + &default_parent);
>> + of_node_put(clkspec.np);
>
> OK here you duplicate the code from of_clk_get_parent_name, I wonder why
> you didn't use the function.But whatever.
I specifically duplicated it because of_clk_get_parent_name returns the
node name if there's no clock-output-names. That wouldn't be useful in
this case.
> Here you will get default_parent = "mvebu-sar" which is the name of the node, you
> can't have "tclk" because this name is not in the device tree, but it will be
> created by the initialization of the core clocks.
No, default_parent would be "tclk" still because clock-output-names
property doesn't exist. If you ever add such a property, it will
override "tclk".
Cheers,
Emilio
On 02/07/2014 03:49 PM, Gregory CLEMENT wrote:
> On 07/02/2014 15:43, Ezequiel Garcia wrote:
>> On Fri, Feb 07, 2014 at 09:24:30AM -0500, Jason Cooper wrote:
>>> On Fri, Feb 07, 2014 at 10:06:08AM -0300, Emilio López wrote:
>>>
>>> [snip a great explanation]
>>>
>>> Guys, can I get some Tested-by's on this?
>>>
>>
>> In case someone missed Emilio's comment about it, I gave his oneliner
>> a test on A370 Reference Design. It worked just as well as Sebastian's.
>
> Well ok it's working but this patch is not better than Sebastian, it is
> even worth. I don't think it is a good idea at all to totally ignore the
Tstststs.. Gregory please re-read the above slooooowly.. and think about
where you'd put me in.
Hint: "this patch", "not better than", "even worse". ;)
> information given by the device tree.
Actually, I have no strong opinion about how we fix the issue now.
Emilio's patch is short and very suitable for a fix. I'll test later
this weekend.
For the long run, I definitely prefer probe ordering within clk frame
work or even better some early_device stuff.
>> Tested-by: Ezequiel Garcia <[email protected]>
>>
>>>> -----8<------
>>>>
>>>> From ffdb49506e3ce92090c15e1f9b37f4d465097ac1 Mon Sep 17 00:00:00 2001
>>>> From: =?UTF-8?q?Emilio=20L=C3=B3pez?= <[email protected]>
>>>> Date: Thu, 6 Feb 2014 18:07:07 -0300
>>>> Subject: [PATCH] clk: mvebu: fix name dependency during registration time
>>>>
>>>> Currently, mvebu_clk_gating_setup has a silly dependency on clock
>>>> registration order just to gather the parent clock name. This is
Of course, the dependency is not silly but we are just ignoring that
some clocks actually have a different parent clock. As long as we are
not interested in the frequency and they all depend on a fixed-clock
it makes no difference. Also, on some gates, we do some tweaking, e.g.
on Dove there is a clock gate for the GBE ip that we loop back to the
GBE PHY gate.. some day I may be so bored to get it straight.
>>>> completely unnecesary, as it supports using an already provided name
>>>> via the clk_gating_soc_desc structs, and we can therefore solve this
>>>> issue with a 69+/- line patch. But, given that the parent name is
>>>> always "tclk" as default-hardcoded on mvebu_coreclk_setup(), we can
>>>> just default-hardcode it here too and get away with solving this
>>>> problem with a one-liner.
I agree with the default hard-coded "tclk" for now. But in general,
clocking can become very nasty and rather than coding the whole fscking
clock tree like the imx clock tree beast does, it would be much more
readable to be able separate the clock drivers into different parts.
Sebastian
>>>> ---
>>>> drivers/clk/mvebu/common.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/clk/mvebu/common.c b/drivers/clk/mvebu/common.c
>>>> index 25ceccf..6c63b43 100644
>>>> --- a/drivers/clk/mvebu/common.c
>>>> +++ b/drivers/clk/mvebu/common.c
>>>> @@ -121,7 +121,7 @@ void __init mvebu_clk_gating_setup(struct
>>>> device_node *np,
>>>> struct clk_gating_ctrl *ctrl;
>>>> struct clk *clk;
>>>> void __iomem *base;
>>>> - const char *default_parent = NULL;
>>>> + const char *default_parent = "tclk";
>>>> int n;
>>>>
>>>> base = of_iomap(np, 0);
>>>> --
>>>> 1.8.5.3