diff mbox series

[v2,1/7] clk: meson: introduce 'INIT_ONCE' flag to eliminate init for enabled PLL

Message ID 20240510090933.19464-2-ddrokosov@salutedevices.com (mailing list archive)
State Changes Requested, archived
Headers show
Series clk: meson: introduce Amlogic A1 SoC Family CPU clock controller driver | expand

Commit Message

Dmitry Rokosov May 10, 2024, 9:08 a.m. UTC 
When dealing with certain PLLs, it is necessary to avoid modifying them
if they have already been initialized by lower levels. For instance, in
the A1 SoC Family, the sys_pll is enabled as the parent for the cpuclk,
and it cannot be disabled during the initialization sequence. Therefore,
initialization phase must be skipped.

Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
---
 drivers/clk/meson/clk-pll.c | 37 +++++++++++++++++++++----------------
 drivers/clk/meson/clk-pll.h |  1 +
 2 files changed, 22 insertions(+), 16 deletions(-)

Comments

Jerome Brunet May 13, 2024, 12:44 p.m. UTC | #1 
On Fri 10 May 2024 at 12:08, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:

> When dealing with certain PLLs, it is necessary to avoid modifying them
> if they have already been initialized by lower levels. For instance, in
> the A1 SoC Family, the sys_pll is enabled as the parent for the cpuclk,
> and it cannot be disabled during the initialization sequence. Therefore,
> initialization phase must be skipped.
>
> Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> ---
>  drivers/clk/meson/clk-pll.c | 37 +++++++++++++++++++++----------------
>  drivers/clk/meson/clk-pll.h |  1 +
>  2 files changed, 22 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
> index 78d17b2415af..47b22a6be2e4 100644
> --- a/drivers/clk/meson/clk-pll.c
> +++ b/drivers/clk/meson/clk-pll.c
> @@ -289,11 +289,32 @@ static int meson_clk_pll_wait_lock(struct clk_hw *hw)
>  	return -ETIMEDOUT;
>  }
>  
> +static int meson_clk_pll_is_enabled(struct clk_hw *hw)
> +{
> +	struct clk_regmap *clk = to_clk_regmap(hw);
> +	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
> +
> +	if (MESON_PARM_APPLICABLE(&pll->rst) &&
> +	    meson_parm_read(clk->map, &pll->rst))
> +		return 0;
> +
> +	if (!meson_parm_read(clk->map, &pll->en) ||
> +	    !meson_parm_read(clk->map, &pll->l))
> +		return 0;
> +
> +	return 1;
> +}
> +
>  static int meson_clk_pll_init(struct clk_hw *hw)
>  {
>  	struct clk_regmap *clk = to_clk_regmap(hw);
>  	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
>  
> +	/* Do not init already enabled PLL which marked with 'init_once'
> */

That is decribing the code, which we can read. So not really helpful
Saying why you do it, like "Keep the clock running from the bootloader
stage and avoid glitching it ..." gives more context about what you are
trying to do.

> +	if ((pll->flags & CLK_MESON_PLL_INIT_ONCE) &&

I don't like INIT_ONCE. It gives the false impression that

* The PLL is going to be initialized once in Linux if it has the flag
* Is initialised multiple times otherwise 

I agree that currently that carefully reading the code clears that up
but it is misleading

CLK_MESON_PLL_EN_NOINIT ?

> +	    meson_clk_pll_is_enabled(hw))
> +		return 0;
> +
>  	if (pll->init_count) {
>  		if (MESON_PARM_APPLICABLE(&pll->rst))
>  			meson_parm_write(clk->map, &pll->rst, 1);
> @@ -308,22 +329,6 @@ static int meson_clk_pll_init(struct clk_hw *hw)
>  	return 0;
>  }
>  
> -static int meson_clk_pll_is_enabled(struct clk_hw *hw)
> -{
> -	struct clk_regmap *clk = to_clk_regmap(hw);
> -	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
> -
> -	if (MESON_PARM_APPLICABLE(&pll->rst) &&
> -	    meson_parm_read(clk->map, &pll->rst))
> -		return 0;
> -
> -	if (!meson_parm_read(clk->map, &pll->en) ||
> -	    !meson_parm_read(clk->map, &pll->l))
> -		return 0;
> -
> -	return 1;
> -}
> -
>  static int meson_clk_pcie_pll_enable(struct clk_hw *hw)
>  {
>  	int retries = 10;
> diff --git a/drivers/clk/meson/clk-pll.h b/drivers/clk/meson/clk-pll.h
> index a2228c0fdce5..23195ea4eae1 100644
> --- a/drivers/clk/meson/clk-pll.h
> +++ b/drivers/clk/meson/clk-pll.h
> @@ -28,6 +28,7 @@ struct pll_mult_range {
>  	}
>  
>  #define CLK_MESON_PLL_ROUND_CLOSEST	BIT(0)
> +#define CLK_MESON_PLL_INIT_ONCE		BIT(1)
>  
>  struct meson_clk_pll_data {
>  	struct parm en;
Dmitry Rokosov May 13, 2024, 9:47 p.m. UTC | #2 
On Mon, May 13, 2024 at 02:44:06PM +0200, Jerome Brunet wrote:
> 
> On Fri 10 May 2024 at 12:08, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:
> 
> > When dealing with certain PLLs, it is necessary to avoid modifying them
> > if they have already been initialized by lower levels. For instance, in
> > the A1 SoC Family, the sys_pll is enabled as the parent for the cpuclk,
> > and it cannot be disabled during the initialization sequence. Therefore,
> > initialization phase must be skipped.
> >
> > Signed-off-by: Dmitry Rokosov <ddrokosov@salutedevices.com>
> > ---
> >  drivers/clk/meson/clk-pll.c | 37 +++++++++++++++++++++----------------
> >  drivers/clk/meson/clk-pll.h |  1 +
> >  2 files changed, 22 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
> > index 78d17b2415af..47b22a6be2e4 100644
> > --- a/drivers/clk/meson/clk-pll.c
> > +++ b/drivers/clk/meson/clk-pll.c
> > @@ -289,11 +289,32 @@ static int meson_clk_pll_wait_lock(struct clk_hw *hw)
> >  	return -ETIMEDOUT;
> >  }
> >  
> > +static int meson_clk_pll_is_enabled(struct clk_hw *hw)
> > +{
> > +	struct clk_regmap *clk = to_clk_regmap(hw);
> > +	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
> > +
> > +	if (MESON_PARM_APPLICABLE(&pll->rst) &&
> > +	    meson_parm_read(clk->map, &pll->rst))
> > +		return 0;
> > +
> > +	if (!meson_parm_read(clk->map, &pll->en) ||
> > +	    !meson_parm_read(clk->map, &pll->l))
> > +		return 0;
> > +
> > +	return 1;
> > +}
> > +
> >  static int meson_clk_pll_init(struct clk_hw *hw)
> >  {
> >  	struct clk_regmap *clk = to_clk_regmap(hw);
> >  	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
> >  
> > +	/* Do not init already enabled PLL which marked with 'init_once'
> > */
> 
> That is decribing the code, which we can read. So not really helpful
> Saying why you do it, like "Keep the clock running from the bootloader
> stage and avoid glitching it ..." gives more context about what you are
> trying to do.
> 

Yes, I agree with you.

"Instead of describing the action, provide the reasoning behind it."

I will incorporate your feedback in the upcoming version.

> > +	if ((pll->flags & CLK_MESON_PLL_INIT_ONCE) &&
> 
> I don't like INIT_ONCE. It gives the false impression that
> 
> * The PLL is going to be initialized once in Linux if it has the flag
> * Is initialised multiple times otherwise 

But that's how things happen. For previous clocks on other platforms, we
assumed that the PLL could be initialized multiple times: once from the
bootloader and once from Linux. We didn't have the ability to disable
initialization from the Linux side before, so it meant that multiple
initializations were potentially possible by default.

> 
> I agree that currently that carefully reading the code clears that up
> but it is misleading
> 
> CLK_MESON_PLL_EN_NOINIT ?
> 

I have been considering this name and its derivatives, such as:

    CLK_MESON_PLL_SKIP_ENABLED
    CLK_MESON_PLL_NOINIT_ENABLED
    CLK_MESON_PLL_INIT_DISABLED_ONLY

However, I find all of these names to be quite long and bulky. It
reminded me of the WARN_ONCE() function, which ensures that a warning
message is only printed once. In my opinion, the name "INIT_ONCE"
accurately reflects the situation.  Nevertheless, if it is your
requirement for me to change the flag name, I am more than willing to do
so, it's not a problem.

> > +	    meson_clk_pll_is_enabled(hw))
> > +		return 0;
> > +
> >  	if (pll->init_count) {
> >  		if (MESON_PARM_APPLICABLE(&pll->rst))
> >  			meson_parm_write(clk->map, &pll->rst, 1);
> > @@ -308,22 +329,6 @@ static int meson_clk_pll_init(struct clk_hw *hw)
> >  	return 0;
> >  }
> >  
> > -static int meson_clk_pll_is_enabled(struct clk_hw *hw)
> > -{
> > -	struct clk_regmap *clk = to_clk_regmap(hw);
> > -	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
> > -
> > -	if (MESON_PARM_APPLICABLE(&pll->rst) &&
> > -	    meson_parm_read(clk->map, &pll->rst))
> > -		return 0;
> > -
> > -	if (!meson_parm_read(clk->map, &pll->en) ||
> > -	    !meson_parm_read(clk->map, &pll->l))
> > -		return 0;
> > -
> > -	return 1;
> > -}
> > -
> >  static int meson_clk_pcie_pll_enable(struct clk_hw *hw)
> >  {
> >  	int retries = 10;
> > diff --git a/drivers/clk/meson/clk-pll.h b/drivers/clk/meson/clk-pll.h
> > index a2228c0fdce5..23195ea4eae1 100644
> > --- a/drivers/clk/meson/clk-pll.h
> > +++ b/drivers/clk/meson/clk-pll.h
> > @@ -28,6 +28,7 @@ struct pll_mult_range {
> >  	}
> >  
> >  #define CLK_MESON_PLL_ROUND_CLOSEST	BIT(0)
> > +#define CLK_MESON_PLL_INIT_ONCE		BIT(1)
> >  
> >  struct meson_clk_pll_data {
> >  	struct parm en;
> 
> 
> -- 
> Jerome
Jerome Brunet May 15, 2024, 1:12 p.m. UTC | #3 
On Tue 14 May 2024 at 00:47, Dmitry Rokosov <ddrokosov@salutedevices.com> wrote:

>> 
>> I agree that currently that carefully reading the code clears that up
>> but it is misleading
>> 
>> CLK_MESON_PLL_EN_NOINIT ?
>>                         
>
> I have been considering this name and its derivatives, such as:
>
>     CLK_MESON_PLL_SKIP_ENABLED

>     CLK_MESON_PLL_NOINIT_ENABLED

That one accurately describes what you do.
Use this one please
diff mbox series

Patch

diff --git a/drivers/clk/meson/clk-pll.c b/drivers/clk/meson/clk-pll.c
index 78d17b2415af..47b22a6be2e4 100644
--- a/drivers/clk/meson/clk-pll.c
+++ b/drivers/clk/meson/clk-pll.c
@@ -289,11 +289,32 @@  static int meson_clk_pll_wait_lock(struct clk_hw *hw)
 	return -ETIMEDOUT;
 }
 
+static int meson_clk_pll_is_enabled(struct clk_hw *hw)
+{
+	struct clk_regmap *clk = to_clk_regmap(hw);
+	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
+
+	if (MESON_PARM_APPLICABLE(&pll->rst) &&
+	    meson_parm_read(clk->map, &pll->rst))
+		return 0;
+
+	if (!meson_parm_read(clk->map, &pll->en) ||
+	    !meson_parm_read(clk->map, &pll->l))
+		return 0;
+
+	return 1;
+}
+
 static int meson_clk_pll_init(struct clk_hw *hw)
 {
 	struct clk_regmap *clk = to_clk_regmap(hw);
 	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
 
+	/* Do not init already enabled PLL which marked with 'init_once' */
+	if ((pll->flags & CLK_MESON_PLL_INIT_ONCE) &&
+	    meson_clk_pll_is_enabled(hw))
+		return 0;
+
 	if (pll->init_count) {
 		if (MESON_PARM_APPLICABLE(&pll->rst))
 			meson_parm_write(clk->map, &pll->rst, 1);
@@ -308,22 +329,6 @@  static int meson_clk_pll_init(struct clk_hw *hw)
 	return 0;
 }
 
-static int meson_clk_pll_is_enabled(struct clk_hw *hw)
-{
-	struct clk_regmap *clk = to_clk_regmap(hw);
-	struct meson_clk_pll_data *pll = meson_clk_pll_data(clk);
-
-	if (MESON_PARM_APPLICABLE(&pll->rst) &&
-	    meson_parm_read(clk->map, &pll->rst))
-		return 0;
-
-	if (!meson_parm_read(clk->map, &pll->en) ||
-	    !meson_parm_read(clk->map, &pll->l))
-		return 0;
-
-	return 1;
-}
-
 static int meson_clk_pcie_pll_enable(struct clk_hw *hw)
 {
 	int retries = 10;
diff --git a/drivers/clk/meson/clk-pll.h b/drivers/clk/meson/clk-pll.h
index a2228c0fdce5..23195ea4eae1 100644
--- a/drivers/clk/meson/clk-pll.h
+++ b/drivers/clk/meson/clk-pll.h
@@ -28,6 +28,7 @@  struct pll_mult_range {
 	}
 
 #define CLK_MESON_PLL_ROUND_CLOSEST	BIT(0)
+#define CLK_MESON_PLL_INIT_ONCE		BIT(1)
 
 struct meson_clk_pll_data {
 	struct parm en;