Re: [PATCH] clk: prevent erronous parsing of children during rate change

[Tero Kristo]

On 09/30/2014 10:07 AM, Mike Turquette wrote:
> 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.

No, this is not the DPLLs are modelled. Each DPLL has currently two 
parents, ref-clk and bypass-clk, which are both modelled as separate 
clock nodes, and the DPLL switches parents based on bypass/lock mode. 
The bypass clock is also usually a mux clock, which further selects 
separate bypass parent, resulting in 3 or more parents for a certain DPLL.

> > 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?

Well, its not kind of incorrectly modelled, it is just modelled in such 
way that clk_set_rate doesn't cope too well with it.

> 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.

Do we have implementation for clk_set_parent_and_rate someplace? I 
looked at rc7 and didn't find this. I think this would fix the issues I 
am seeing combined with determine_rate, if clk_set_rate would internally 
handle changing both rate + parent.

-Tero

> 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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/