Re: [PATCH v2 4/9] clk: rockchip: add new clock type and controller for rk3036

[Stephen Boyd]

On 09/23, Heiko Stübner wrote:
> Hi Stephen,
> 
> Am Dienstag, 22. September 2015, 15:41:25 schrieb Stephen Boyd:
> > On 09/17, Xing Zheng wrote:
> > > +
> > > +static void rockchip_rk3036_pll_init(struct clk_hw *hw)
> > 
> > init ops are "discouraged". Could we do this through assigned
> > rates instead?
> 
> really? According to Mike that was a valid use-case when we looked for an 
> initial place for that on the rk3288 :-) .

A comment in clk.c indicates init ops are discouraged. Maybe this
is a valid use-case on other platforms so it was allowed, but
pretty much every time we see a new init op we have to think
about it and justify it. Hooray!

> 
> 
> > > +{
> > > +	struct rockchip_clk_pll *pll = to_rockchip_clk_pll(hw);
> > > +	const struct rockchip_pll_rate_table *rate;
> > > +	unsigned int fbdiv, postdiv1, refdiv, postdiv2, dsmpd, frac;
> > > +	unsigned long drate;
> > > +	u32 pllcon;
> > > +
> > > +	if (!(pll->flags & ROCKCHIP_PLL_SYNC_RATE))
> > > +		return;
> > 
> > I don't understand what this one does though. This check isn't in
> > the set rate ops.
> 
> And it shouldn't be :-)
> 
> The issue this whole thing is trying to solve is aligning the pll settings 
> which what we have in the rate table, not what the bootloader set.
> 
> For example the bootloader could set up a pll at 594MHz with one set of 
> parameters and after some time - when you don't want to exchange bootloaders 
> on shipping devices anymore - it comes to light that a different set of 
> parameters for the same frequency produces for example a more stable hdmi 
> signal [I think that was the main reason for the initial change].
> 
> So we're not changing the frequency x -> y, which could be easily done [and is 
> done already] via assigned-rates, but instead
> 	x {params a,b,c} -> x {params d,e,f}
> so the rate itself stays the same, only the frequency generation is adapted.

Ok. It would be nice if this sort of information was made into a
comment and put in the code. Or at least the commit text for the
change.

And is there any reason that we need to get the parent clock and
parent rate to align the PLL settings? It would be nice if we
avoided using clk_* APIs in here, by extracting the pll set rate
code into another function that we can call from init to make the
values the same without all the fallback to old rates, etc.

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
a Linux Foundation Collaborative Project
--
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/