Message-ID: <20140930070732.19023.37829@quantum>
Date:	Tue, 30 Sep 2014 00:07:32 -0700
From:	Mike Turquette <mturquette@...aro.org>
To:	Tero Kristo <t-kristo@...com>,
	"Stephen Boyd" <sboyd@...eaurora.org>
Cc:	linux-kernel@...r.kernel.org, linux-omap@...r.kernel.org,
	tony@...mide.com, nm@...com, linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH] clk: prevent erronous parsing of children during rate change

Quoting Tero Kristo (2014-09-29 01:09:24)
> On 09/27/2014 02:24 AM, Mike Turquette wrote:
> > Quoting Tero Kristo (2014-09-26 00:18:55)
> >> On 09/26/2014 04:35 AM, Stephen Boyd wrote:
> >>> On 09/23/14 06:38, Tero Kristo wrote:
> >>>> On 09/22/2014 10:18 PM, Stephen Boyd wrote:
> >>>>> On 08/21, Tero Kristo wrote:
> >>>>>>             /* Skip children who will be reparented to another clock */
> >>>>>>             if (child->new_parent && child->new_parent != clk)
> >>>>>>                 continue;
> >>>>>
> >>>>> Are we not hitting the new_parent check here? I don't understand
> >>>>> how we can be changing parents here unless the check is being
> >>>>> avoided, in which case I wonder why determine_rate isn't being
> >>>>> used.
> >>>>>
> >>>>
> >>>> It depends how the clock underneath handles the situation. The error I
> >>>> am seeing actually happens with a SoC specific compound clock (DPLL)
> >>>> which integrates set_rate + mux functionality into a single clock
> >>>> node. A call to the clk_set_rate changes the parent of this clock
> >>>> (from bypass clock to reference clock), in addition to changing the
> >>>> rate (tune the mul+div.) I looked at using the determine rate call
> >>>> with this type but it breaks everything up... the parent gets changed
> >>>> but not the clock rate, in addition to some other issues.
> >>>
> >>> Ok. Is this omap3_noncore_dpll_set_rate()?
> >>
> >> Yes.
> >>
> >>   > Can we use determine_rate +
> >>> clk_set_parent_and_rate()? At least clk_set_parent_and_rate() would
> >>> allow us to do the mult+div and the parent in the same op call, although
> >>> I don't understand why setting the parent and then setting the rate is
> >>> not going to work.
> >>
> >> Well, setting parent first, then rate later causes problems with the
> >> DPLL ending up running with illegal (non-specified) rate, the M+N values
> >> are most likely wrong if you just switch from bypass clock to reference
> >> clock first without programming the M+N first.
> >
> > I took a quick look and it still seems to me that the OMAP DPLLs are
> > still not modeled properly as mux clocks. Is this correct?
> 
> Yeah, they are not mux clocks, but rather a compound of mux + DPLL 
> multiplier/divider logic. Changing the DPLL to be a separate mux + DPLL 
> div/mult clock will still have overlapping usage of the DPLL_EN field, 

I'm not talking about splitting up the clock into two separate clocks.
If memory serves the DPLL clock implementation "cheats" and hides the
bypass_clk info from the clock framework. To be explicit, from the
perspective of Linux clock framework DPLL clocks only have one parent.

In reality a typical DPLL should have at least 2 parents (and in some
cases starting with OMAP4, some of the DPLL output clocks should have a
second HSD parent). But the implementation does not reflect this.

> as the DPLL must be in bypass mode during M+N change. Or, should the 
> DPLL rate change only be allowed if the mux is in bypass setting? 
> Several drivers still depend on direct dpll clk_set_rate working 
> 'properly' (there are some other issues currently present also which 
> have nothing to do with the mux behavior.)
> 
> > This issue has been lingering for a long time and we can't use
> > determine_rate unless that clock has multiple parents. Simply hacking
> > knowledge of the parent bypass clock into the .set_rate callback is not
> > enough.
> 
> If you believe this _must_ be changed, I can take a look at this for 
> next merge window, but this will cause a DT data compatibility break if 
> nothing else (personally I don't care about this as I always rebuild DT 
> blob with kernel, but lots of other people seem to do.)

Well I guess the question is how long will we put up with the many small
headaches caused by incorrectly modeling the clock?

determine_rate and clk_set_parent_and_rate should be sufficient for the
OMAP DPLLs but only if they are correctly modeled in the framework.

Regards,
Mike

> 
> -Tero
> 
> >
> > Regards,
> > Mike
> >
> >>
> >>    I'm interested in the other issues that you mentioned
> >>> too.
> >>
> >> Mostly these were side-effects from the illegal DPLL setup I guess, like
> >> boot hang, failed drivers etc. I didn't really investigate this that
> >> much as it is much more simpler just to use safe list iteration here.
> >>
> >> -Tero
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists