From: Maxime Ripard <hidden> Date: 2014-09-11 20:36:23
Hi Chen-Yu,
On Sat, Sep 06, 2014 at 06:47:21PM +0800, Chen-Yu Tsai wrote:
Hi everyone,
This series unifies the mux and divider parts of the AHB1 clock found
on sun6i and sun8i, while also adding support for the pre-divider on
the PLL6 input.
The rate calculation logic must factor in which parent it is using to
calculate the rate, to decide whether to use the pre-divider or not.
This is beyond the original factors clk design in sunxi. To avoid
feature bloat, this is implemented as a seperate composite clk.
The new clock driver is registered with a separate OF_CLK_DECLARE.
This is done so that assigned-clocks* properties on the clk provider
node can actually work. The clock framework arranges the clock setup
order by checking whether all clock parents are available, by checking
the node matching OF_CLK_DECLARE.
However, the sunxi clk driver is based on the root node compatible,
has no defined dependencies (parents), and is setup before the fixed-rate
clocks. Thus when the ahb1 clock is added, all parents have rate = 0.
There is no way to calculate the required clock factors to set a default
clock rate under these circumstances. This happens when we set the
defaults in the clock node (provider), rather than a clock consumer.
I can think of 2 ways to solve the dependency issue, but neither is
pretty. One would be to move the root fixed-rate clocks into the sunxi
clk driver. The other would be separating all the clocks into individual
OF_CLK_DECLARE statements, which adds a lot of boilerplate code.
I don't know what Mike thinks of this, but I'd prefer the second.
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
From: Mike Turquette <hidden> Date: 2014-09-26 00:25:44
Quoting Maxime Ripard (2014-09-11 13:36:23)
Hi Chen-Yu,
On Sat, Sep 06, 2014 at 06:47:21PM +0800, Chen-Yu Tsai wrote:
quoted
Hi everyone,
This series unifies the mux and divider parts of the AHB1 clock found
on sun6i and sun8i, while also adding support for the pre-divider on
the PLL6 input.
The rate calculation logic must factor in which parent it is using to
calculate the rate, to decide whether to use the pre-divider or not.
This is beyond the original factors clk design in sunxi. To avoid
feature bloat, this is implemented as a seperate composite clk.
The new clock driver is registered with a separate OF_CLK_DECLARE.
This is done so that assigned-clocks* properties on the clk provider
node can actually work. The clock framework arranges the clock setup
order by checking whether all clock parents are available, by checking
the node matching OF_CLK_DECLARE.
However, the sunxi clk driver is based on the root node compatible,
has no defined dependencies (parents), and is setup before the fixed-rate
clocks. Thus when the ahb1 clock is added, all parents have rate = 0.
There is no way to calculate the required clock factors to set a default
clock rate under these circumstances. This happens when we set the
defaults in the clock node (provider), rather than a clock consumer.
I can think of 2 ways to solve the dependency issue, but neither is
pretty. One would be to move the root fixed-rate clocks into the sunxi
clk driver. The other would be separating all the clocks into individual
OF_CLK_DECLARE statements, which adds a lot of boilerplate code.
I don't know what Mike thinks of this, but I'd prefer the second.
I do not fully understand the problem. Ideally the clock driver should
have some way to fail with EPROBE_DEFER until the fixed-rate clocks are
registered. Those fixed-rate parents are enumerated in your dts, right?
Why isn't this enough?
Thanks,
Mike
Maxime
--
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com
On Fri, Sep 26, 2014 at 8:25 AM, Mike Turquette [off-list ref] wrote:
Quoting Maxime Ripard (2014-09-11 13:36:23)
quoted
Hi Chen-Yu,
On Sat, Sep 06, 2014 at 06:47:21PM +0800, Chen-Yu Tsai wrote:
quoted
Hi everyone,
This series unifies the mux and divider parts of the AHB1 clock found
on sun6i and sun8i, while also adding support for the pre-divider on
the PLL6 input.
The rate calculation logic must factor in which parent it is using to
calculate the rate, to decide whether to use the pre-divider or not.
This is beyond the original factors clk design in sunxi. To avoid
feature bloat, this is implemented as a seperate composite clk.
The new clock driver is registered with a separate OF_CLK_DECLARE.
This is done so that assigned-clocks* properties on the clk provider
node can actually work. The clock framework arranges the clock setup
order by checking whether all clock parents are available, by checking
the node matching OF_CLK_DECLARE.
However, the sunxi clk driver is based on the root node compatible,
has no defined dependencies (parents), and is setup before the fixed-rate
clocks. Thus when the ahb1 clock is added, all parents have rate = 0.
There is no way to calculate the required clock factors to set a default
clock rate under these circumstances. This happens when we set the
defaults in the clock node (provider), rather than a clock consumer.
I can think of 2 ways to solve the dependency issue, but neither is
pretty. One would be to move the root fixed-rate clocks into the sunxi
clk driver. The other would be separating all the clocks into individual
OF_CLK_DECLARE statements, which adds a lot of boilerplate code.
I don't know what Mike thinks of this, but I'd prefer the second.
I do not fully understand the problem. Ideally the clock driver should
have some way to fail with EPROBE_DEFER until the fixed-rate clocks are
registered. Those fixed-rate parents are enumerated in your dts, right?
Why isn't this enough?
This is due to the way the sunxi clock driver is setup. The clock driver's
OF_CLK_DECLARE matches against the "soc" node, not the individual clock
nodes. When the setup function is called, it just registers all the
supported clocks. There are no dependencies listed.
Unfortunately, the fixed-factor clock is in the middle of the whole clock
tree. The sunxi clock driver provides its parents _and_ its children.
Naturally the clock framework then probes the fixed-factor clock after
the sunxi ones. Any attempts to change the state of clocks under the
unavailable fixed-factor clock, such as done by of_clk_set_defaults(),
would get an incomplete clock, likely with no parents and parent_rate = 0.
That is until of_clk_init() finishes and all clocks are properly hooked
up.
Anyway, this problem only occurred when I added clk-assigned-* defaults
to the clock provider node, which is not the case anymore.
The second method i proposed is to have OF_CLK_DECLAREs for each individual
clock. An example can be found here:
https://github.com/wens/linux/commit/1276898da02a93da4af163ed5bdc88cdead565dc
This does add a lot of boilerplate code. Not really happy about it. But
it seems the proper way to split up the driver.
Cheers
ChenYu
From: Mike Turquette <hidden> Date: 2014-09-26 18:53:34
Quoting Chen-Yu Tsai (2014-09-25 17:55:27)
On Fri, Sep 26, 2014 at 8:25 AM, Mike Turquette [off-list ref] wrote:
quoted
Quoting Maxime Ripard (2014-09-11 13:36:23)
quoted
Hi Chen-Yu,
On Sat, Sep 06, 2014 at 06:47:21PM +0800, Chen-Yu Tsai wrote:
quoted
Hi everyone,
This series unifies the mux and divider parts of the AHB1 clock found
on sun6i and sun8i, while also adding support for the pre-divider on
the PLL6 input.
The rate calculation logic must factor in which parent it is using to
calculate the rate, to decide whether to use the pre-divider or not.
This is beyond the original factors clk design in sunxi. To avoid
feature bloat, this is implemented as a seperate composite clk.
The new clock driver is registered with a separate OF_CLK_DECLARE.
This is done so that assigned-clocks* properties on the clk provider
node can actually work. The clock framework arranges the clock setup
order by checking whether all clock parents are available, by checking
the node matching OF_CLK_DECLARE.
However, the sunxi clk driver is based on the root node compatible,
has no defined dependencies (parents), and is setup before the fixed-rate
clocks. Thus when the ahb1 clock is added, all parents have rate = 0.
There is no way to calculate the required clock factors to set a default
clock rate under these circumstances. This happens when we set the
defaults in the clock node (provider), rather than a clock consumer.
I can think of 2 ways to solve the dependency issue, but neither is
pretty. One would be to move the root fixed-rate clocks into the sunxi
clk driver. The other would be separating all the clocks into individual
OF_CLK_DECLARE statements, which adds a lot of boilerplate code.
I don't know what Mike thinks of this, but I'd prefer the second.
I do not fully understand the problem. Ideally the clock driver should
have some way to fail with EPROBE_DEFER until the fixed-rate clocks are
registered. Those fixed-rate parents are enumerated in your dts, right?
Why isn't this enough?
This is due to the way the sunxi clock driver is setup. The clock driver's
OF_CLK_DECLARE matches against the "soc" node, not the individual clock
nodes. When the setup function is called, it just registers all the
supported clocks. There are no dependencies listed.
Unfortunately, the fixed-factor clock is in the middle of the whole clock
tree. The sunxi clock driver provides its parents _and_ its children.
Naturally the clock framework then probes the fixed-factor clock after
the sunxi ones. Any attempts to change the state of clocks under the
unavailable fixed-factor clock, such as done by of_clk_set_defaults(),
would get an incomplete clock, likely with no parents and parent_rate = 0.
That is until of_clk_init() finishes and all clocks are properly hooked
up.
Anyway, this problem only occurred when I added clk-assigned-* defaults
to the clock provider node, which is not the case anymore.
Makes sense. I guess you could ignore the problem until you need to use
the assigned defaults.
The second method i proposed is to have OF_CLK_DECLAREs for each individual
clock. An example can be found here:
https://github.com/wens/linux/commit/1276898da02a93da4af163ed5bdc88cdead565dc
This does add a lot of boilerplate code. Not really happy about it. But
it seems the proper way to split up the driver.
On Sat, Sep 27, 2014 at 2:53 AM, Mike Turquette [off-list ref] wrote:
Quoting Chen-Yu Tsai (2014-09-25 17:55:27)
quoted
On Fri, Sep 26, 2014 at 8:25 AM, Mike Turquette [off-list ref] wrote:
quoted
Quoting Maxime Ripard (2014-09-11 13:36:23)
quoted
Hi Chen-Yu,
On Sat, Sep 06, 2014 at 06:47:21PM +0800, Chen-Yu Tsai wrote:
quoted
Hi everyone,
This series unifies the mux and divider parts of the AHB1 clock found
on sun6i and sun8i, while also adding support for the pre-divider on
the PLL6 input.
The rate calculation logic must factor in which parent it is using to
calculate the rate, to decide whether to use the pre-divider or not.
This is beyond the original factors clk design in sunxi. To avoid
feature bloat, this is implemented as a seperate composite clk.
The new clock driver is registered with a separate OF_CLK_DECLARE.
This is done so that assigned-clocks* properties on the clk provider
node can actually work. The clock framework arranges the clock setup
order by checking whether all clock parents are available, by checking
the node matching OF_CLK_DECLARE.
However, the sunxi clk driver is based on the root node compatible,
has no defined dependencies (parents), and is setup before the fixed-rate
clocks. Thus when the ahb1 clock is added, all parents have rate = 0.
There is no way to calculate the required clock factors to set a default
clock rate under these circumstances. This happens when we set the
defaults in the clock node (provider), rather than a clock consumer.
I can think of 2 ways to solve the dependency issue, but neither is
pretty. One would be to move the root fixed-rate clocks into the sunxi
clk driver. The other would be separating all the clocks into individual
OF_CLK_DECLARE statements, which adds a lot of boilerplate code.
I don't know what Mike thinks of this, but I'd prefer the second.
I do not fully understand the problem. Ideally the clock driver should
have some way to fail with EPROBE_DEFER until the fixed-rate clocks are
registered. Those fixed-rate parents are enumerated in your dts, right?
Why isn't this enough?
This is due to the way the sunxi clock driver is setup. The clock driver's
OF_CLK_DECLARE matches against the "soc" node, not the individual clock
nodes. When the setup function is called, it just registers all the
supported clocks. There are no dependencies listed.
Unfortunately, the fixed-factor clock is in the middle of the whole clock
tree. The sunxi clock driver provides its parents _and_ its children.
Naturally the clock framework then probes the fixed-factor clock after
the sunxi ones. Any attempts to change the state of clocks under the
unavailable fixed-factor clock, such as done by of_clk_set_defaults(),
would get an incomplete clock, likely with no parents and parent_rate = 0.
That is until of_clk_init() finishes and all clocks are properly hooked
up.
Anyway, this problem only occurred when I added clk-assigned-* defaults
to the clock provider node, which is not the case anymore.
Makes sense. I guess you could ignore the problem until you need to use
the assigned defaults.
An update on this. Improper ordering of clock probing also affects
sunxi's clock protection code.
Currently we have 2 mechanisms for protecting clocks.
a) A list of clock names in sunxi/clk-sunxi.c, fetched and enabled
using clkdev.
b) Enabling clocks right after they are registered. Used for separated
clock drivers like sun5i-a13-mbus and sun8i-a23-mbus.
One issue I ran across was when most of the clock tree is registered using
independent CLK_OF_DECLAREs, as I'm doing for the A80, if the protected
clocks list is handled before the clock tree is complete, the prepare
and enable calls are not correctly propagated to the parents that arrive
later on.
This happens to the ahb*_sdram gates.
quoted
The second method i proposed is to have OF_CLK_DECLAREs for each individual
clock. An example can be found here:
https://github.com/wens/linux/commit/1276898da02a93da4af163ed5bdc88cdead565dc
This does add a lot of boilerplate code. Not really happy about it. But
it seems the proper way to split up the driver.