Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1423614 > unrolled thread

Re: [PATCH 1/3] clk: Add new function of_clk_is_provider()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 1/3] clk: Add new function of_clk_is_provider() Stephen Boyd <sboyd@codeaurora.org> - 2016-06-16 02:50 +0200
    Re: [PATCH 1/3] clk: Add new function of_clk_is_provider() Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> - 2016-06-20 16:00 +0200
      Re: [PATCH 1/3] clk: Add new function of_clk_is_provider() Stephen Boyd <sboyd@codeaurora.org> - 2016-06-21 03:40 +0200
        Re: [PATCH 1/3] clk: Add new function of_clk_is_provider() Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com> - 2016-06-21 10:40 +0200

#1423614 — Re: [PATCH 1/3] clk: Add new function of_clk_is_provider()

On 06/08, Ricardo Ribalda Delgado wrote:
> of_clk_is_provider() checks if a device_node has already been added to
> the clk provider list. This can be used to avoid adding the same clock
> provider twice.
> 
> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>

While I don't disagree with the concept, I'd like to do this
outside of the clk framework checking for nodes, because the
problem doesn't seem clk specific. From digging in the OF
platform layer I see of_node_test_and_set_flag(OF_POPULATED) may
be what we should be using. It looks like this can be used to
make sure that any clk provider nodes aren't populated as
platform devices when we've initialized them early.

The only problem now is that we have drivers using a hybrid
approach with of_clk_init(). Sometimes drivers need to get clks
up early for timers, so they have CLK_OF_DECLARE() in their
driver, but then they also use a platform driver to handle the
non-timer related clks. If we mark all nodes as populated in
of_clk_init() we'll preclude these drivers from working. The
solution there is to make those drivers specifically clear the
populated flag in the clk init callback. Or we can automatically
do that with some new CLK_OF_DECLARE_EARLY() macro that hides
this clearing from them. Either way, the drivers will need to
indicate they're using this hybrid style so that we still
populate platform devices.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [next] | [standalone]

#1426632

Hi Stephen

When the device tree is populated or when an overlay is added, all its
nodes have the flag OF_POPULATED set. The flag is enabled recursively
in
of_platform_bus_create->of_platform_device_create_pdata()
So we cannot use that flag to mark what is enabled and what is not.

The other issue that I see is of_clk_mutex. Whatever final
implementation that we decide to do, it should take into consideration
that mutex, otherwise it will not be thread-safe.
of_clk_is_provider() is already taking care of it.

Another advantage of of_clk_is_provider() is that it opens the door to
implement something like:  CLK_OF_DECLARE_EARLY_PLATFORM(probe,remove)
 That allows a driver to implement early clk and platform clk at the
same time and automatically, following a logic similar to what I have
done in fixed-clk.

I will send a v2 with the changes you proposed to fixed-clk

Thanks and best regards!

On Thu, Jun 16, 2016 at 2:44 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> On 06/08, Ricardo Ribalda Delgado wrote:
>> of_clk_is_provider() checks if a device_node has already been added to
>> the clk provider list. This can be used to avoid adding the same clock
>> provider twice.
>>
>> Signed-off-by: Ricardo Ribalda Delgado <ricardo.ribalda@gmail.com>
>
> While I don't disagree with the concept, I'd like to do this
> outside of the clk framework checking for nodes, because the
> problem doesn't seem clk specific. From digging in the OF
> platform layer I see of_node_test_and_set_flag(OF_POPULATED) may
> be what we should be using. It looks like this can be used to
> make sure that any clk provider nodes aren't populated as
> platform devices when we've initialized them early.
>
> The only problem now is that we have drivers using a hybrid
> approach with of_clk_init(). Sometimes drivers need to get clks
> up early for timers, so they have CLK_OF_DECLARE() in their
> driver, but then they also use a platform driver to handle the
> non-timer related clks. If we mark all nodes as populated in
> of_clk_init() we'll preclude these drivers from working. The
> solution there is to make those drivers specifically clear the
> populated flag in the clk init callback. Or we can automatically
> do that with some new CLK_OF_DECLARE_EARLY() macro that hides
> this clearing from them. Either way, the drivers will need to
> indicate they're using this hybrid style so that we still
> populate platform devices.
>
> --
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
> a Linux Foundation Collaborative Project



-- 
Ricardo Ribalda

[toc] | [prev] | [next] | [standalone]

#1427181

(Please don't top post)

On 06/20, Ricardo Ribalda Delgado wrote:
> Hi Stephen
> 
> When the device tree is populated or when an overlay is added, all its
> nodes have the flag OF_POPULATED set. The flag is enabled recursively
> in
> of_platform_bus_create->of_platform_device_create_pdata()
> So we cannot use that flag to mark what is enabled and what is not.

Sorry I don't follow the reasoning here. I'm not asking to test
that flag in an of_clk_is_provider() API. The goal is to not have
an of_clk_is_provider() API.

I was thinking that of_clk_init() would mark any nodes that
matched and provided clk providers as OF_POPULATED. That way,
when of_platform_populate() ran, it would *not* add platform
devices for clk providers that we registered during the
of_clk_init() phase. Then we could have platform drivers and
CLK_OF_DECLARE drivers for the same compatible strings, but we
wouldn't probe random platform drivers for the nodes that we
handled early on and we wouldn't need to litter
of_clk_is_provider() in driver probe routines.

> 
> The other issue that I see is of_clk_mutex. Whatever final
> implementation that we decide to do, it should take into consideration
> that mutex, otherwise it will not be thread-safe.
> of_clk_is_provider() is already taking care of it.
> 
> Another advantage of of_clk_is_provider() is that it opens the door to
> implement something like:  CLK_OF_DECLARE_EARLY_PLATFORM(probe,remove)
>  That allows a driver to implement early clk and platform clk at the
> same time and automatically, following a logic similar to what I have
> done in fixed-clk.

Something like CLK_OF_DECLARE_EARLY_PLATFORM() sounds like it may
be good (I haven't looked at the patch). We'll need something to
say that there's CLK_OF_DECLARE and a platform driver for the
same node.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project

[toc] | [prev] | [next] | [standalone]

#1427452

Hi Stephen

On Tue, Jun 21, 2016 at 3:30 AM, Stephen Boyd <sboyd@codeaurora.org> wrote:
> (Please don't top post)

Sorry about that

>
> I was thinking that of_clk_init() would mark any nodes that
> matched and provided clk providers as OF_POPULATED. That way,
> when of_platform_populate() ran, it would *not* add platform
> devices for clk providers that we registered during the
> of_clk_init() phase. Then we could have platform drivers and
> CLK_OF_DECLARE drivers for the same compatible strings, but we
> wouldn't probe random platform drivers for the nodes that we
> handled early on and we wouldn't need to litter
> of_clk_is_provider() in driver probe routines.
>

Ok, now I get it. I didn't realised that you wanted to set the flag.

I have prepared two new patches.  now setup does something like:

void __init of_fixed_factor_clk_setup(struct device_node *node)
{
        if (!_of_fixed_factor_clk_setup(node))
                  of_node_set_flag(node, OF_POPULATED);
}

If we probe this method to be valid, on a future stage I can make a MACRO like:

CLK_OF_DECLARE_PLATFORM(fixed_factor_clk, "fixed-factor-clock",
_of_fixed_factor_clk_setup, clk_unregister_fixed_factor);

That sets the flag, instantiate MODULE_DEVICE_TABLE,
builtin_platform_driver,  and does the platform_set_drvdata.....

Driver developers can choose to use CLK_OF_DECLARE_PLATFORM or CLK_OF_DECLARE


Thanks!

-- 
Ricardo Ribalda

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web