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

[Tero Kristo]

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

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