Re: [PATCH] clk: tegra: Fix PLLE programming

[Thierry Reding <thierry.reding-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>]

[-- Attachment #1: Type: text/plain, Size: 2025 bytes --]

On Mon, Mar 31, 2014 at 10:51:38AM -0600, Stephen Warren wrote:
> On 03/31/2014 08:45 AM, Thierry Reding wrote:
> > From: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> > 
> > PLLE has M, N and P divider shift and width parameters that differ from
> > the defaults. Furthermore, when clearing the M, N and P divider fields
> > the corresponding masks were never shifted, thereby clearing only the
> > lowest bits of the register. This lead to a situation where the PLLE
> > programming would only work if the register hadn't been touched before.
> 
> > diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
> 
> > @@ -730,8 +730,10 @@ static int clk_plle_enable(struct clk_hw *hw)
> >  	if (pll->params->flags & TEGRA_PLLE_CONFIGURE) {
> >  		/* configure dividers */
> >  		val = pll_readl_base(pll);
> > -		val &= ~(divm_mask(pll) | divn_mask(pll) | divp_mask(pll));
> > -		val &= ~(PLLE_BASE_DIVCML_WIDTH << PLLE_BASE_DIVCML_SHIFT);
> > +		val &= ~(divp_mask(pll) << PLLE_BASE_DIVP_SHIFT |
> > +			 divn_mask(pll) << PLLE_BASE_DIVN_SHIFT |
> > +			 divm_mask(pll) << PLLE_BASE_DIVM_SHIFT);
> 
> Shouldn't those shift values also be a macro/inline like
> divm_shift(pll), since ...
> 
> > +static struct div_nmp pll_e_nmp = {
> > +	.divn_shift = PLLE_BASE_DIVN_SHIFT,
> > +	.divn_width = PLLE_BASE_DIVN_WIDTH,
> > +	.divm_shift = PLLE_BASE_DIVM_SHIFT,
> > +	.divm_width = PLLE_BASE_DIVM_WIDTH,
> > +	.divp_shift = PLLE_BASE_DIVP_SHIFT,
> > +	.divp_width = PLLE_BASE_DIVP_WIDTH,
> > +};
> 
> ... that table contains parameters for both width and shift values, not
> just width values?

Yes, that could be done. At some point I was also thinking about simply
converting the masks to be shifted masks, but then realized that the
patche would become rather intrusive.

Perhaps, though, to make the code slightly more terse, in addition to
div{n,m,p}_shift() macros I could add div{n,m,p}_mask_shifted() macros
as well to combine both.

Thierry

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]