[PATCH] clk: tegra: Fix PLLE programming

[PATCH] clk: tegra: Fix PLLE programming

[Thierry Reding]

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.

Signed-off-by: Thierry Reding <treding-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 drivers/clk/tegra/clk-pll.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
index 0d20241e0770..357911303315 100644
--- a/drivers/clk/tegra/clk-pll.c
+++ b/drivers/clk/tegra/clk-pll.c
@@ -58,9 +58,9 @@
 #define PLLDU_LFCON_SET_DIVN 600
 
 #define PLLE_BASE_DIVCML_SHIFT 24
-#define PLLE_BASE_DIVCML_WIDTH 4
+#define PLLE_BASE_DIVCML_MASK 0xf
 #define PLLE_BASE_DIVP_SHIFT 16
-#define PLLE_BASE_DIVP_WIDTH 7
+#define PLLE_BASE_DIVP_WIDTH 6
 #define PLLE_BASE_DIVN_SHIFT 8
 #define PLLE_BASE_DIVN_WIDTH 8
 #define PLLE_BASE_DIVM_SHIFT 0
@@ -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);
+		val &= ~(PLLE_BASE_DIVCML_MASK << PLLE_BASE_DIVCML_SHIFT);
 		val |= sel.m << pll->params->div_nmp->divm_shift;
 		val |= sel.n << pll->params->div_nmp->divn_shift;
 		val |= sel.p << pll->params->div_nmp->divp_shift;
@@ -745,6 +747,7 @@ static int clk_plle_enable(struct clk_hw *hw)
 	pll_writel_misc(val, pll);
 
 	val = readl(pll->clk_base + PLLE_SS_CTRL);
+	val &= ~PLLE_SS_COEFFICIENTS_MASK;
 	val |= PLLE_SS_DISABLE;
 	writel(val, pll->clk_base + PLLE_SS_CTRL);
 
@@ -1292,8 +1295,10 @@ static int clk_plle_tegra114_enable(struct clk_hw *hw)
 	pll_writel(val, PLLE_SS_CTRL, pll);
 
 	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);
+	val &= ~(PLLE_BASE_DIVCML_MASK << PLLE_BASE_DIVCML_SHIFT);
 	val |= sel.m << pll->params->div_nmp->divm_shift;
 	val |= sel.n << pll->params->div_nmp->divn_shift;
 	val |= sel.cpcon << PLLE_BASE_DIVCML_SHIFT;
@@ -1410,6 +1415,15 @@ struct clk *tegra_clk_register_pll(const char *name, const char *parent_name,
 	return clk;
 }
 
+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,
+};
+
 struct clk *tegra_clk_register_plle(const char *name, const char *parent_name,
 		void __iomem *clk_base, void __iomem *pmc,
 		unsigned long flags, struct tegra_clk_pll_params *pll_params,
@@ -1420,6 +1434,10 @@ struct clk *tegra_clk_register_plle(const char *name, const char *parent_name,
 
 	pll_params->flags |= TEGRA_PLL_LOCK_MISC | TEGRA_PLL_BYPASS;
 	pll_params->flags |= TEGRA_PLL_HAS_LOCK_ENABLE;
+
+	if (!pll_params->div_nmp)
+		pll_params->div_nmp = &pll_e_nmp;
+
 	pll = _tegra_init_pll(clk_base, pmc, pll_params, lock);
 	if (IS_ERR(pll))
 		return ERR_CAST(pll);
-- 
1.9.1

[PATCH] clk: tegra: Fix PLLE programming

[Thierry Reding]

From: Thierry Reding <treding@nvidia.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.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 drivers/clk/tegra/clk-pll.c | 30 ++++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/tegra/clk-pll.c b/drivers/clk/tegra/clk-pll.c
index 0d20241e0770..357911303315 100644
--- a/drivers/clk/tegra/clk-pll.c
+++ b/drivers/clk/tegra/clk-pll.c
@@ -58,9 +58,9 @@
 #define PLLDU_LFCON_SET_DIVN 600
 
 #define PLLE_BASE_DIVCML_SHIFT 24
-#define PLLE_BASE_DIVCML_WIDTH 4
+#define PLLE_BASE_DIVCML_MASK 0xf
 #define PLLE_BASE_DIVP_SHIFT 16
-#define PLLE_BASE_DIVP_WIDTH 7
+#define PLLE_BASE_DIVP_WIDTH 6
 #define PLLE_BASE_DIVN_SHIFT 8
 #define PLLE_BASE_DIVN_WIDTH 8
 #define PLLE_BASE_DIVM_SHIFT 0
@@ -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);
+		val &= ~(PLLE_BASE_DIVCML_MASK << PLLE_BASE_DIVCML_SHIFT);
 		val |= sel.m << pll->params->div_nmp->divm_shift;
 		val |= sel.n << pll->params->div_nmp->divn_shift;
 		val |= sel.p << pll->params->div_nmp->divp_shift;
@@ -745,6 +747,7 @@ static int clk_plle_enable(struct clk_hw *hw)
 	pll_writel_misc(val, pll);
 
 	val = readl(pll->clk_base + PLLE_SS_CTRL);
+	val &= ~PLLE_SS_COEFFICIENTS_MASK;
 	val |= PLLE_SS_DISABLE;
 	writel(val, pll->clk_base + PLLE_SS_CTRL);
 
@@ -1292,8 +1295,10 @@ static int clk_plle_tegra114_enable(struct clk_hw *hw)
 	pll_writel(val, PLLE_SS_CTRL, pll);
 
 	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);
+	val &= ~(PLLE_BASE_DIVCML_MASK << PLLE_BASE_DIVCML_SHIFT);
 	val |= sel.m << pll->params->div_nmp->divm_shift;
 	val |= sel.n << pll->params->div_nmp->divn_shift;
 	val |= sel.cpcon << PLLE_BASE_DIVCML_SHIFT;
@@ -1410,6 +1415,15 @@ struct clk *tegra_clk_register_pll(const char *name, const char *parent_name,
 	return clk;
 }
 
+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,
+};
+
 struct clk *tegra_clk_register_plle(const char *name, const char *parent_name,
 		void __iomem *clk_base, void __iomem *pmc,
 		unsigned long flags, struct tegra_clk_pll_params *pll_params,
@@ -1420,6 +1434,10 @@ struct clk *tegra_clk_register_plle(const char *name, const char *parent_name,
 
 	pll_params->flags |= TEGRA_PLL_LOCK_MISC | TEGRA_PLL_BYPASS;
 	pll_params->flags |= TEGRA_PLL_HAS_LOCK_ENABLE;
+
+	if (!pll_params->div_nmp)
+		pll_params->div_nmp = &pll_e_nmp;
+
 	pll = _tegra_init_pll(clk_base, pmc, pll_params, lock);
 	if (IS_ERR(pll))
 		return ERR_CAST(pll);
-- 
1.9.1

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

[Stephen Warren]

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?

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

[Stephen Warren]

On 03/31/2014 08:45 AM, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.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?

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

[Thierry Reding]

[-- 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 --]

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

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1996 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@nvidia.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

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

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

[Peter De Schrijver]

On Mon, Mar 31, 2014 at 09:46:22PM +0200, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> 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.
> 

Problem is that there are 2 shift values in case the PLL has an override bit.
The corresponding PMC register is not always layed out in the same way than
the CAR register.

Cheers,

Peter.

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

[Peter De Schrijver]

On Mon, Mar 31, 2014 at 09:46:22PM +0200, Thierry Reding wrote:
> * PGP Signed by an unknown key
> 
> 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@nvidia.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.
> 

Problem is that there are 2 shift values in case the PLL has an override bit.
The corresponding PMC register is not always layed out in the same way than
the CAR register.

Cheers,

Peter.