Re: [PATCH 0/7] clk: sun6i: Unify AHB1 clock and fix rate calculation

[Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>]

Quoting Chen-Yu Tsai (2014-09-25 17:55:27)
> On Fri, Sep 26, 2014 at 8:25 AM, Mike Turquette <mturquette-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > 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:
> >> > 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.

Yeah, this is OK. Ugly, but OK.

Regards,
Mike

> 
> 
> Cheers
> ChenYu