diff mbox series

[v4,01/12] clk: mediatek: Disable tuner_en before change PLL rate

Message ID 20190201083016.25856-3-weiyi.lu@mediatek.com (mailing list archive)
State New, archived
Headers show
Series Mediatek MT8183 clock and scpsys support | expand

Commit Message

Weiyi Lu Feb. 1, 2019, 8:30 a.m. UTC 
From: Owen Chen <owen.chen@mediatek.com>

PLLs with tuner_en bit, such as APLL1, need to disable
tuner_en before apply new frequency settings, or the new frequency
settings (pcw) will not be applied.
The tuner_en bit will be disabled during changing PLL rate
and be restored after new settings applied.
Another minor change is to correct the macro name of pcw change bit
to CON1_PCW_CHG because PCW_CHG(BIT31) is on CON1.

Fixes: e2f744a82d725 (clk: mediatek: Add MT2712 clock support)
Cc: <stable@vger.kernel.org>
Signed-off-by: Owen Chen <owen.chen@mediatek.com>
---
 drivers/clk/mediatek/clk-pll.c | 33 +++++++++++++++++++++++++++++++--
 1 file changed, 31 insertions(+), 2 deletions(-)

Comments

Matthias Brugger Feb. 26, 2019, 3:59 p.m. UTC | #1 
On 01/02/2019 09:30, Weiyi Lu wrote:
> From: Owen Chen <owen.chen@mediatek.com>
> 
> PLLs with tuner_en bit, such as APLL1, need to disable
> tuner_en before apply new frequency settings, or the new frequency
> settings (pcw) will not be applied.
> The tuner_en bit will be disabled during changing PLL rate
> and be restored after new settings applied.
> Another minor change is to correct the macro name of pcw change bit
> to CON1_PCW_CHG because PCW_CHG(BIT31) is on CON1.
> 
> Fixes: e2f744a82d725 (clk: mediatek: Add MT2712 clock support)
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Owen Chen <owen.chen@mediatek.com>
> ---
>  drivers/clk/mediatek/clk-pll.c | 33 +++++++++++++++++++++++++++++++--
>  1 file changed, 31 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
> index f54e4015b0b1..f0ff5f535c7e 100644
> --- a/drivers/clk/mediatek/clk-pll.c
> +++ b/drivers/clk/mediatek/clk-pll.c
> @@ -27,7 +27,7 @@
>  #define CON0_BASE_EN		BIT(0)
>  #define CON0_PWR_ON		BIT(0)
>  #define CON0_ISO_EN		BIT(1)
> -#define CON0_PCW_CHG		BIT(31)
> +#define CON1_PCW_CHG		BIT(31)
>  
>  #define AUDPLL_TUNER_EN		BIT(31)
>  
> @@ -93,9 +93,31 @@ static void mtk_pll_set_rate_regs(struct mtk_clk_pll *pll, u32 pcw,
>  {
>  	u32 con1, val;
>  	int pll_en;
> +	u32 tuner_en = 0;
> +	u32 tuner_en_mask;
> +	void __iomem *tuner_en_addr = NULL;
>  
>  	pll_en = readl(pll->base_addr + REG_CON0) & CON0_BASE_EN;
>  
> +	/* disable tuner */
> +	if (pll->tuner_en_addr) {
> +		tuner_en_addr = pll->tuner_en_addr;
> +		tuner_en_mask = BIT(pll->data->tuner_en_bit);
> +	} else if (pll->tuner_addr) {
> +		tuner_en_addr = pll->tuner_addr;
> +		tuner_en_mask = AUDPLL_TUNER_EN;
> +	}
> +
> +	if (tuner_en_addr) {
> +		val = readl(tuner_en_addr);
> +		tuner_en = val & tuner_en_mask;
> +
> +		if (tuner_en) {
> +			val &= ~tuner_en_mask;
> +			writel(val, tuner_en_addr);
> +		}
> +	}
> +

Why don't we use a flag here, it would make the code easier to understand.
I think it would also help if you put this code in a separate function.

Regards,
Matthias


>  	/* set postdiv */
>  	val = readl(pll->pd_addr);
>  	val &= ~(POSTDIV_MASK << pll->data->pd_shift);
> @@ -116,12 +138,19 @@ static void mtk_pll_set_rate_regs(struct mtk_clk_pll *pll, u32 pcw,
>  	con1 = readl(pll->base_addr + REG_CON1);
>  
>  	if (pll_en)
> -		con1 |= CON0_PCW_CHG;
> +		con1 |= CON1_PCW_CHG;
>  
>  	writel(con1, pll->base_addr + REG_CON1);
>  	if (pll->tuner_addr)
>  		writel(con1 + 1, pll->tuner_addr);
>  
> +	/* restore tuner_en */
> +	if (tuner_en_addr && tuner_en) {
> +		val = readl(tuner_en_addr);
> +		val |= tuner_en_mask;
> +		writel(val, tuner_en_addr);
> +	}
> +
>  	if (pll_en)
>  		udelay(20);
>  }
>
Weiyi Lu Feb. 27, 2019, 3:51 a.m. UTC | #2 
On Tue, 2019-02-26 at 16:59 +0100, Matthias Brugger wrote:
> 
> On 01/02/2019 09:30, Weiyi Lu wrote:
> > From: Owen Chen <owen.chen@mediatek.com>
> > 
> > PLLs with tuner_en bit, such as APLL1, need to disable
> > tuner_en before apply new frequency settings, or the new frequency
> > settings (pcw) will not be applied.
> > The tuner_en bit will be disabled during changing PLL rate
> > and be restored after new settings applied.
> > Another minor change is to correct the macro name of pcw change bit
> > to CON1_PCW_CHG because PCW_CHG(BIT31) is on CON1.
> > 
> > Fixes: e2f744a82d725 (clk: mediatek: Add MT2712 clock support)
> > Cc: <stable@vger.kernel.org>
> > Signed-off-by: Owen Chen <owen.chen@mediatek.com>
> > ---
> >  drivers/clk/mediatek/clk-pll.c | 33 +++++++++++++++++++++++++++++++--
> >  1 file changed, 31 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
> > index f54e4015b0b1..f0ff5f535c7e 100644
> > --- a/drivers/clk/mediatek/clk-pll.c
> > +++ b/drivers/clk/mediatek/clk-pll.c
> > @@ -27,7 +27,7 @@
> >  #define CON0_BASE_EN		BIT(0)
> >  #define CON0_PWR_ON		BIT(0)
> >  #define CON0_ISO_EN		BIT(1)
> > -#define CON0_PCW_CHG		BIT(31)
> > +#define CON1_PCW_CHG		BIT(31)
> >  
> >  #define AUDPLL_TUNER_EN		BIT(31)
> >  
> > @@ -93,9 +93,31 @@ static void mtk_pll_set_rate_regs(struct mtk_clk_pll *pll, u32 pcw,
> >  {
> >  	u32 con1, val;
> >  	int pll_en;
> > +	u32 tuner_en = 0;
> > +	u32 tuner_en_mask;
> > +	void __iomem *tuner_en_addr = NULL;
> >  
> >  	pll_en = readl(pll->base_addr + REG_CON0) & CON0_BASE_EN;
> >  
> > +	/* disable tuner */
> > +	if (pll->tuner_en_addr) {
> > +		tuner_en_addr = pll->tuner_en_addr;
> > +		tuner_en_mask = BIT(pll->data->tuner_en_bit);
> > +	} else if (pll->tuner_addr) {
> > +		tuner_en_addr = pll->tuner_addr;
> > +		tuner_en_mask = AUDPLL_TUNER_EN;
> > +	}
> > +
> > +	if (tuner_en_addr) {
> > +		val = readl(tuner_en_addr);
> > +		tuner_en = val & tuner_en_mask;
> > +
> > +		if (tuner_en) {
> > +			val &= ~tuner_en_mask;
> > +			writel(val, tuner_en_addr);
> > +		}
> > +	}
> > +
> 
> Why don't we use a flag here, it would make the code easier to understand.
> I think it would also help if you put this code in a separate function.
> 
> Regards,
> Matthias
> 

Hi Matthias,

I guess you suggest to add a flag (e.g. CLK_TUNER_SUPPORT) and declare
the PLL clock with this flag. And inside this mtk_pll_set_rate_regs()
function, we could modify it as below. Am I right?

if (pll->data->flags & CLK_TUNER_SUPPORT)
    disable_tuner();
[...]
if (pll->data->flags & CLK_TUNER_SUPPORT)
    enable_tuner();


But I thought this flag might be a customized flags and can only be put
in clk-mtk.h not clk-provider.h so far. And it might cause 
potential problem of clock flag re-definition. May I have more
suggestion?

> 
> >  	/* set postdiv */
> >  	val = readl(pll->pd_addr);
> >  	val &= ~(POSTDIV_MASK << pll->data->pd_shift);
> > @@ -116,12 +138,19 @@ static void mtk_pll_set_rate_regs(struct mtk_clk_pll *pll, u32 pcw,
> >  	con1 = readl(pll->base_addr + REG_CON1);
> >  
> >  	if (pll_en)
> > -		con1 |= CON0_PCW_CHG;
> > +		con1 |= CON1_PCW_CHG;
> >  
> >  	writel(con1, pll->base_addr + REG_CON1);
> >  	if (pll->tuner_addr)
> >  		writel(con1 + 1, pll->tuner_addr);
> >  
> > +	/* restore tuner_en */
> > +	if (tuner_en_addr && tuner_en) {
> > +		val = readl(tuner_en_addr);
> > +		val |= tuner_en_mask;
> > +		writel(val, tuner_en_addr);
> > +	}
> > +
> >  	if (pll_en)
> >  		udelay(20);
> >  }
> >
Weiyi Lu Feb. 27, 2019, 4:39 a.m. UTC | #3 
On Wed, 2019-02-27 at 11:51 +0800, Weiyi Lu wrote:
> On Tue, 2019-02-26 at 16:59 +0100, Matthias Brugger wrote:
> > 
> > On 01/02/2019 09:30, Weiyi Lu wrote:
> > > From: Owen Chen <owen.chen@mediatek.com>
> > > 
> > > PLLs with tuner_en bit, such as APLL1, need to disable
> > > tuner_en before apply new frequency settings, or the new frequency
> > > settings (pcw) will not be applied.
> > > The tuner_en bit will be disabled during changing PLL rate
> > > and be restored after new settings applied.
> > > Another minor change is to correct the macro name of pcw change bit
> > > to CON1_PCW_CHG because PCW_CHG(BIT31) is on CON1.
> > > 
> > > Fixes: e2f744a82d725 (clk: mediatek: Add MT2712 clock support)
> > > Cc: <stable@vger.kernel.org>
> > > Signed-off-by: Owen Chen <owen.chen@mediatek.com>
> > > ---
> > >  drivers/clk/mediatek/clk-pll.c | 33 +++++++++++++++++++++++++++++++--
> > >  1 file changed, 31 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
> > > index f54e4015b0b1..f0ff5f535c7e 100644
> > > --- a/drivers/clk/mediatek/clk-pll.c
> > > +++ b/drivers/clk/mediatek/clk-pll.c
> > > @@ -27,7 +27,7 @@
> > >  #define CON0_BASE_EN		BIT(0)
> > >  #define CON0_PWR_ON		BIT(0)
> > >  #define CON0_ISO_EN		BIT(1)
> > > -#define CON0_PCW_CHG		BIT(31)
> > > +#define CON1_PCW_CHG		BIT(31)
> > >  
> > >  #define AUDPLL_TUNER_EN		BIT(31)
> > >  
> > > @@ -93,9 +93,31 @@ static void mtk_pll_set_rate_regs(struct mtk_clk_pll *pll, u32 pcw,
> > >  {
> > >  	u32 con1, val;
> > >  	int pll_en;
> > > +	u32 tuner_en = 0;
> > > +	u32 tuner_en_mask;
> > > +	void __iomem *tuner_en_addr = NULL;
> > >  
> > >  	pll_en = readl(pll->base_addr + REG_CON0) & CON0_BASE_EN;
> > >  
> > > +	/* disable tuner */
> > > +	if (pll->tuner_en_addr) {
> > > +		tuner_en_addr = pll->tuner_en_addr;
> > > +		tuner_en_mask = BIT(pll->data->tuner_en_bit);
> > > +	} else if (pll->tuner_addr) {
> > > +		tuner_en_addr = pll->tuner_addr;
> > > +		tuner_en_mask = AUDPLL_TUNER_EN;
> > > +	}
> > > +
> > > +	if (tuner_en_addr) {
> > > +		val = readl(tuner_en_addr);
> > > +		tuner_en = val & tuner_en_mask;
> > > +
> > > +		if (tuner_en) {
> > > +			val &= ~tuner_en_mask;
> > > +			writel(val, tuner_en_addr);
> > > +		}
> > > +	}
> > > +
> > 
> > Why don't we use a flag here, it would make the code easier to understand.
> > I think it would also help if you put this code in a separate function.
> > 
> > Regards,
> > Matthias
> > 
> 
> Hi Matthias,
> 
> I guess you suggest to add a flag (e.g. CLK_TUNER_SUPPORT) and declare
> the PLL clock with this flag. And inside this mtk_pll_set_rate_regs()
> function, we could modify it as below. Am I right?
> 
> if (pll->data->flags & CLK_TUNER_SUPPORT)
>     disable_tuner();
> [...]
> if (pll->data->flags & CLK_TUNER_SUPPORT)
>     enable_tuner();
> 
> 
> But I thought this flag might be a customized flags and can only be put
> in clk-mtk.h not clk-provider.h so far. And it might cause 
> potential problem of clock flag re-definition. May I have more
> suggestion?
> 

Please ignore the potential problem I mentioned, I just checked it
carefully and that redefinition problem won't happen in current flow.
Sorry for having you confused.
But I still need to confirm with you if the method in previous mail is
correct? Thanks.

> > 
> > >  	/* set postdiv */
> > >  	val = readl(pll->pd_addr);
> > >  	val &= ~(POSTDIV_MASK << pll->data->pd_shift);
> > > @@ -116,12 +138,19 @@ static void mtk_pll_set_rate_regs(struct mtk_clk_pll *pll, u32 pcw,
> > >  	con1 = readl(pll->base_addr + REG_CON1);
> > >  
> > >  	if (pll_en)
> > > -		con1 |= CON0_PCW_CHG;
> > > +		con1 |= CON1_PCW_CHG;
> > >  
> > >  	writel(con1, pll->base_addr + REG_CON1);
> > >  	if (pll->tuner_addr)
> > >  		writel(con1 + 1, pll->tuner_addr);
> > >  
> > > +	/* restore tuner_en */
> > > +	if (tuner_en_addr && tuner_en) {
> > > +		val = readl(tuner_en_addr);
> > > +		val |= tuner_en_mask;
> > > +		writel(val, tuner_en_addr);
> > > +	}
> > > +
> > >  	if (pll_en)
> > >  		udelay(20);
> > >  }
> > > 
>
diff mbox series

Patch

diff --git a/drivers/clk/mediatek/clk-pll.c b/drivers/clk/mediatek/clk-pll.c
index f54e4015b0b1..f0ff5f535c7e 100644
--- a/drivers/clk/mediatek/clk-pll.c
+++ b/drivers/clk/mediatek/clk-pll.c
@@ -27,7 +27,7 @@ 
 #define CON0_BASE_EN		BIT(0)
 #define CON0_PWR_ON		BIT(0)
 #define CON0_ISO_EN		BIT(1)
-#define CON0_PCW_CHG		BIT(31)
+#define CON1_PCW_CHG		BIT(31)
 
 #define AUDPLL_TUNER_EN		BIT(31)
 
@@ -93,9 +93,31 @@  static void mtk_pll_set_rate_regs(struct mtk_clk_pll *pll, u32 pcw,
 {
 	u32 con1, val;
 	int pll_en;
+	u32 tuner_en = 0;
+	u32 tuner_en_mask;
+	void __iomem *tuner_en_addr = NULL;
 
 	pll_en = readl(pll->base_addr + REG_CON0) & CON0_BASE_EN;
 
+	/* disable tuner */
+	if (pll->tuner_en_addr) {
+		tuner_en_addr = pll->tuner_en_addr;
+		tuner_en_mask = BIT(pll->data->tuner_en_bit);
+	} else if (pll->tuner_addr) {
+		tuner_en_addr = pll->tuner_addr;
+		tuner_en_mask = AUDPLL_TUNER_EN;
+	}
+
+	if (tuner_en_addr) {
+		val = readl(tuner_en_addr);
+		tuner_en = val & tuner_en_mask;
+
+		if (tuner_en) {
+			val &= ~tuner_en_mask;
+			writel(val, tuner_en_addr);
+		}
+	}
+
 	/* set postdiv */
 	val = readl(pll->pd_addr);
 	val &= ~(POSTDIV_MASK << pll->data->pd_shift);
@@ -116,12 +138,19 @@  static void mtk_pll_set_rate_regs(struct mtk_clk_pll *pll, u32 pcw,
 	con1 = readl(pll->base_addr + REG_CON1);
 
 	if (pll_en)
-		con1 |= CON0_PCW_CHG;
+		con1 |= CON1_PCW_CHG;
 
 	writel(con1, pll->base_addr + REG_CON1);
 	if (pll->tuner_addr)
 		writel(con1 + 1, pll->tuner_addr);
 
+	/* restore tuner_en */
+	if (tuner_en_addr && tuner_en) {
+		val = readl(tuner_en_addr);
+		val |= tuner_en_mask;
+		writel(val, tuner_en_addr);
+	}
+
 	if (pll_en)
 		udelay(20);
 }