linux-kernel - Re: [PATCH] clk: tegra: Fix PLLE programming

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20140331194612.GC3431@mithrandir>
Date:	Mon, 31 Mar 2014 21:46:22 +0200
From:	Thierry Reding <thierry.reding@...il.com>
To:	Stephen Warren <swarren@...dotorg.org>
Cc:	Peter De Schrijver <pdeschrijver@...dia.com>,
	Prashant Gaikwad <pgaikwad@...dia.com>,
	Mike Turquette <mturquette@...aro.org>,
	linux-tegra@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] clk: tegra: Fix PLLE programming

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@...dia.com>
> > 
> > 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

Content of type "application/pgp-signature" skipped