[PATCH 0/4] clk: meson: fixup g12a mpll

[PATCH 0/4] clk: meson: fixup g12a mpll

[Jerome Brunet]

When the g12a support has been initially submitted, the MPLL appeared
(overall) fine. At the time, the board I used was falshed with amlogic
vendor u-boot. Since then, I moved to an early version on mainline
u-boot.

While debugging audio support, I noticed that MPLL based clocks were way
above target. It appeared the fractional part of the divider was not
working.

To work properly, the MPLLs each needs an initial setting in addition to
a common one. No one likes those register sequences but sometimes, like
here for PLL clocks, there is no way around it.

The series adds the possibility to set initial register sequence for the
ee clock controller and the MPLL driver. It is then used to enable the
fractional part of the g12a MPLL.

With this series applied, the fractional part works again but we are still
seeing a significant clock jitter (+/- 1,6% for 294912KHz). Discussion
are ongoing to explain and, hopefully, solve this as well.

Jerome Brunet (4):
  clk: meson: mpll: add init callback and regs
  clk: meson: g12a: add mpll register init sequences
  clk: meson: eeclk: add init regs
  clk: meson: g12a: add controller register init

 drivers/clk/meson/clk-mpll.c    | 33 +++++++++++++++++++++++----------
 drivers/clk/meson/clk-mpll.h    |  2 ++
 drivers/clk/meson/g12a.c        | 32 +++++++++++++++++++++++++++++++-
 drivers/clk/meson/meson-eeclk.c |  3 +++
 drivers/clk/meson/meson-eeclk.h |  2 ++
 5 files changed, 61 insertions(+), 11 deletions(-)

-- 
2.20.1

[PATCH 1/4] clk: meson: mpll: add init callback and regs

[Jerome Brunet]

Until now (gx and axg), the mpll setting on boot (whatever the
bootloader) was good enough generate a clean fractional division.

It is not the case on the g12a. While moving away from the vendor u-boot,
it was noticed the fractional part of the divider was no longer applied.
Like on the pll, some magic settings need to applied on the mpll
register.

This change adds the ability to do that on the mpll driver.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/meson/clk-mpll.c | 33 +++++++++++++++++++++++----------
 drivers/clk/meson/clk-mpll.h |  2 ++
 2 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/clk/meson/clk-mpll.c b/drivers/clk/meson/clk-mpll.c
index f76850d99e59..64d31c8ba3d0 100644
--- a/drivers/clk/meson/clk-mpll.c
+++ b/drivers/clk/meson/clk-mpll.c
@@ -115,21 +115,12 @@ static int mpll_set_rate(struct clk_hw *hw,
 	else
 		__acquire(mpll->lock);
 
-	/* Enable and set the fractional part */
+	/* Set the fractional part */
 	meson_parm_write(clk->map, &mpll->sdm, sdm);
-	meson_parm_write(clk->map, &mpll->sdm_en, 1);
-
-	/* Set additional fractional part enable if required */
-	if (MESON_PARM_APPLICABLE(&mpll->ssen))
-		meson_parm_write(clk->map, &mpll->ssen, 1);
 
 	/* Set the integer divider part */
 	meson_parm_write(clk->map, &mpll->n2, n2);
 
-	/* Set the magic misc bit if required */
-	if (MESON_PARM_APPLICABLE(&mpll->misc))
-		meson_parm_write(clk->map, &mpll->misc, 1);
-
 	if (mpll->lock)
 		spin_unlock_irqrestore(mpll->lock, flags);
 	else
@@ -138,6 +129,27 @@ static int mpll_set_rate(struct clk_hw *hw,
 	return 0;
 }
 
+static void mpll_init(struct clk_hw *hw)
+{
+	struct clk_regmap *clk = to_clk_regmap(hw);
+	struct meson_clk_mpll_data *mpll = meson_clk_mpll_data(clk);
+
+	if (mpll->init_count)
+		regmap_multi_reg_write(clk->map, mpll->init_regs,
+				       mpll->init_count);
+
+	/* Enable the fractional part */
+	meson_parm_write(clk->map, &mpll->sdm_en, 1);
+
+	/* Set additional fractional part enable if required */
+	if (MESON_PARM_APPLICABLE(&mpll->ssen))
+		meson_parm_write(clk->map, &mpll->ssen, 1);
+
+	/* Set the magic misc bit if required */
+	if (MESON_PARM_APPLICABLE(&mpll->misc))
+		meson_parm_write(clk->map, &mpll->misc, 1);
+}
+
 const struct clk_ops meson_clk_mpll_ro_ops = {
 	.recalc_rate	= mpll_recalc_rate,
 	.round_rate	= mpll_round_rate,
@@ -148,6 +160,7 @@ const struct clk_ops meson_clk_mpll_ops = {
 	.recalc_rate	= mpll_recalc_rate,
 	.round_rate	= mpll_round_rate,
 	.set_rate	= mpll_set_rate,
+	.init		= mpll_init,
 };
 EXPORT_SYMBOL_GPL(meson_clk_mpll_ops);
 
diff --git a/drivers/clk/meson/clk-mpll.h b/drivers/clk/meson/clk-mpll.h
index cf79340006dd..2925fb939fdd 100644
--- a/drivers/clk/meson/clk-mpll.h
+++ b/drivers/clk/meson/clk-mpll.h
@@ -18,6 +18,8 @@ struct meson_clk_mpll_data {
 	struct parm n2;
 	struct parm ssen;
 	struct parm misc;
+	const struct reg_sequence *init_regs;
+	unsigned int init_count;
 	spinlock_t *lock;
 	u8 flags;
 };
-- 
2.20.1

[PATCH 2/4] clk: meson: g12a: add mpll register init sequences

[Jerome Brunet]

Add the required init of each MPLL of the g12a.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/meson/g12a.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
index d3f53a9b97dc..6a01f8fd8114 100644
--- a/drivers/clk/meson/g12a.c
+++ b/drivers/clk/meson/g12a.c
@@ -577,6 +577,10 @@ static struct clk_fixed_factor g12a_mpll_prediv = {
 	},
 };
 
+static const struct reg_sequence g12a_mpll0_init_regs[] = {
+	{ .reg = HHI_MPLL_CNTL2,	.def = 0x40000033 },
+};
+
 static struct clk_regmap g12a_mpll0_div = {
 	.data = &(struct meson_clk_mpll_data){
 		.sdm = {
@@ -600,6 +604,8 @@ static struct clk_regmap g12a_mpll0_div = {
 			.width	 = 1,
 		},
 		.lock = &meson_clk_lock,
+		.init_regs = g12a_mpll0_init_regs,
+		.init_count = ARRAY_SIZE(g12a_mpll0_init_regs),
 	},
 	.hw.init = &(struct clk_init_data){
 		.name = "mpll0_div",
@@ -623,6 +629,10 @@ static struct clk_regmap g12a_mpll0 = {
 	},
 };
 
+static const struct reg_sequence g12a_mpll1_init_regs[] = {
+	{ .reg = HHI_MPLL_CNTL4,	.def = 0x40000033 },
+};
+
 static struct clk_regmap g12a_mpll1_div = {
 	.data = &(struct meson_clk_mpll_data){
 		.sdm = {
@@ -646,6 +656,8 @@ static struct clk_regmap g12a_mpll1_div = {
 			.width	 = 1,
 		},
 		.lock = &meson_clk_lock,
+		.init_regs = g12a_mpll1_init_regs,
+		.init_count = ARRAY_SIZE(g12a_mpll1_init_regs),
 	},
 	.hw.init = &(struct clk_init_data){
 		.name = "mpll1_div",
@@ -669,6 +681,10 @@ static struct clk_regmap g12a_mpll1 = {
 	},
 };
 
+static const struct reg_sequence g12a_mpll2_init_regs[] = {
+	{ .reg = HHI_MPLL_CNTL6,	.def = 0x40000033 },
+};
+
 static struct clk_regmap g12a_mpll2_div = {
 	.data = &(struct meson_clk_mpll_data){
 		.sdm = {
@@ -692,6 +708,8 @@ static struct clk_regmap g12a_mpll2_div = {
 			.width	 = 1,
 		},
 		.lock = &meson_clk_lock,
+		.init_regs = g12a_mpll2_init_regs,
+		.init_count = ARRAY_SIZE(g12a_mpll2_init_regs),
 	},
 	.hw.init = &(struct clk_init_data){
 		.name = "mpll2_div",
@@ -715,6 +733,10 @@ static struct clk_regmap g12a_mpll2 = {
 	},
 };
 
+static const struct reg_sequence g12a_mpll3_init_regs[] = {
+	{ .reg = HHI_MPLL_CNTL8,	.def = 0x40000033 },
+};
+
 static struct clk_regmap g12a_mpll3_div = {
 	.data = &(struct meson_clk_mpll_data){
 		.sdm = {
@@ -738,6 +760,8 @@ static struct clk_regmap g12a_mpll3_div = {
 			.width	 = 1,
 		},
 		.lock = &meson_clk_lock,
+		.init_regs = g12a_mpll3_init_regs,
+		.init_count = ARRAY_SIZE(g12a_mpll3_init_regs),
 	},
 	.hw.init = &(struct clk_init_data){
 		.name = "mpll3_div",
-- 
2.20.1

[PATCH 3/4] clk: meson: eeclk: add init regs

[Jerome Brunet]

Like the PLL and MPLL, the controller may require some magic setting to
be applied on startup.

This is needed when the initial setting is not applied by the boot ROM.
The controller need to do it when the setting applies to several clock,
like all the MPLLs in the case of g12a.

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/meson/meson-eeclk.c | 3 +++
 drivers/clk/meson/meson-eeclk.h | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/clk/meson/meson-eeclk.c b/drivers/clk/meson/meson-eeclk.c
index 37a34c9c3885..6ba2094be257 100644
--- a/drivers/clk/meson/meson-eeclk.c
+++ b/drivers/clk/meson/meson-eeclk.c
@@ -34,6 +34,9 @@ int meson_eeclkc_probe(struct platform_device *pdev)
 		return PTR_ERR(map);
 	}
 
+	if (data->init_count)
+		regmap_multi_reg_write(map, data->init_regs, data->init_count);
+
 	input = meson_clk_hw_register_input(dev, "xtal", IN_PREFIX "xtal", 0);
 	if (IS_ERR(input)) {
 		ret = PTR_ERR(input);
diff --git a/drivers/clk/meson/meson-eeclk.h b/drivers/clk/meson/meson-eeclk.h
index 1b809b1419fe..9ab5d6fa7ccb 100644
--- a/drivers/clk/meson/meson-eeclk.h
+++ b/drivers/clk/meson/meson-eeclk.h
@@ -17,6 +17,8 @@ struct platform_device;
 struct meson_eeclkc_data {
 	struct clk_regmap *const	*regmap_clks;
 	unsigned int			regmap_clk_num;
+	const struct reg_sequence	*init_regs;
+	unsigned int			init_count;
 	struct clk_hw_onecell_data	*hw_onecell_data;
 };
 
-- 
2.20.1

[PATCH 4/4] clk: meson: g12a: add controller register init

[Jerome Brunet]

Add the MPLL common register initial setting

Signed-off-by: Jerome Brunet <jbrunet@baylibre.com>
---
 drivers/clk/meson/g12a.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/clk/meson/g12a.c b/drivers/clk/meson/g12a.c
index 6a01f8fd8114..493db74270ac 100644
--- a/drivers/clk/meson/g12a.c
+++ b/drivers/clk/meson/g12a.c
@@ -2361,10 +2361,16 @@ static struct clk_regmap *const g12a_clk_regmaps[] = {
 	&g12a_mpll_50m,
 };
 
+static const struct reg_sequence g12a_init_regs[] = {
+	{ .reg = HHI_MPLL_CNTL0,	.def = 0x00000543 },
+};
+
 static const struct meson_eeclkc_data g12a_clkc_data = {
 	.regmap_clks = g12a_clk_regmaps,
 	.regmap_clk_num = ARRAY_SIZE(g12a_clk_regmaps),
-	.hw_onecell_data = &g12a_hw_onecell_data
+	.hw_onecell_data = &g12a_hw_onecell_data,
+	.init_regs = g12a_init_regs,
+	.init_count = ARRAY_SIZE(g12a_init_regs),
 };
 
 static const struct of_device_id clkc_match_table[] = {
-- 
2.20.1

Re: [PATCH 1/4] clk: meson: mpll: add init callback and regs

[Stephen Boyd]

Quoting Jerome Brunet (2019-03-25 04:11:57)
> @@ -138,6 +129,27 @@ static int mpll_set_rate(struct clk_hw *hw,
>         return 0;
>  }
>  
> +static void mpll_init(struct clk_hw *hw)
> +{
> +       struct clk_regmap *clk = to_clk_regmap(hw);
> +       struct meson_clk_mpll_data *mpll = meson_clk_mpll_data(clk);
> +
> +       if (mpll->init_count)
> +               regmap_multi_reg_write(clk->map, mpll->init_regs,
> +                                      mpll->init_count);
> +
> +       /* Enable the fractional part */
> +       meson_parm_write(clk->map, &mpll->sdm_en, 1);
> +
> +       /* Set additional fractional part enable if required */
> +       if (MESON_PARM_APPLICABLE(&mpll->ssen))
> +               meson_parm_write(clk->map, &mpll->ssen, 1);
> +
> +       /* Set the magic misc bit if required */
> +       if (MESON_PARM_APPLICABLE(&mpll->misc))
> +               meson_parm_write(clk->map, &mpll->misc, 1);
> +}
> +
>  const struct clk_ops meson_clk_mpll_ro_ops = {
>         .recalc_rate    = mpll_recalc_rate,
>         .round_rate     = mpll_round_rate,
> @@ -148,6 +160,7 @@ const struct clk_ops meson_clk_mpll_ops = {
>         .recalc_rate    = mpll_recalc_rate,
>         .round_rate     = mpll_round_rate,
>         .set_rate       = mpll_set_rate,
> +       .init           = mpll_init,

We actively discourage using init callbacks. Can you do this some other
way?

>  };
>  EXPORT_SYMBOL_GPL(meson_clk_mpll_ops);
>  

Re: [PATCH 1/4] clk: meson: mpll: add init callback and regs

[Jerome Brunet]

On Mon, 2019-03-25 at 10:10 -0700, Stephen Boyd wrote:
> Quoting Jerome Brunet (2019-03-25 04:11:57)
> > @@ -138,6 +129,27 @@ static int mpll_set_rate(struct clk_hw *hw,
> >         return 0;
> >  }
> >  
> > +static void mpll_init(struct clk_hw *hw)
> > +{
> > +       struct clk_regmap *clk = to_clk_regmap(hw);
> > +       struct meson_clk_mpll_data *mpll = meson_clk_mpll_data(clk);
> > +
> > +       if (mpll->init_count)
> > +               regmap_multi_reg_write(clk->map, mpll->init_regs,
> > +                                      mpll->init_count);
> > +
> > +       /* Enable the fractional part */
> > +       meson_parm_write(clk->map, &mpll->sdm_en, 1);
> > +
> > +       /* Set additional fractional part enable if required */
> > +       if (MESON_PARM_APPLICABLE(&mpll->ssen))
> > +               meson_parm_write(clk->map, &mpll->ssen, 1);
> > +
> > +       /* Set the magic misc bit if required */
> > +       if (MESON_PARM_APPLICABLE(&mpll->misc))
> > +               meson_parm_write(clk->map, &mpll->misc, 1);
> > +}
> > +
> >  const struct clk_ops meson_clk_mpll_ro_ops = {
> >         .recalc_rate    = mpll_recalc_rate,
> >         .round_rate     = mpll_round_rate,
> > @@ -148,6 +160,7 @@ const struct clk_ops meson_clk_mpll_ops = {
> >         .recalc_rate    = mpll_recalc_rate,
> >         .round_rate     = mpll_round_rate,
> >         .set_rate       = mpll_set_rate,
> > +       .init           = mpll_init,
> 
> We actively discourage using init callbacks. Can you do this some other
> way?

Yes I'm aware of that but init it the right place to do this.
To be clear, this is not initializing the clock to some particular rate, the
rate is preserved.

It just applies the necessary settings that needs to be done only once to make
sure the clock is in working order and that the rate calculated is actually
accurate. 

> 
> >  };
> >  EXPORT_SYMBOL_GPL(meson_clk_mpll_ops);
> >  

Re: [PATCH 1/4] clk: meson: mpll: add init callback and regs

[Stephen Boyd]

Quoting Jerome Brunet (2019-03-26 00:53:15)
> On Mon, 2019-03-25 at 10:10 -0700, Stephen Boyd wrote:
> > Quoting Jerome Brunet (2019-03-25 04:11:57)
> > > @@ -138,6 +129,27 @@ static int mpll_set_rate(struct clk_hw *hw,
> > >         return 0;
> > >  }
> > >  
> > > +static void mpll_init(struct clk_hw *hw)
> > > +{
> > > +       struct clk_regmap *clk = to_clk_regmap(hw);
> > > +       struct meson_clk_mpll_data *mpll = meson_clk_mpll_data(clk);
> > > +
> > > +       if (mpll->init_count)
> > > +               regmap_multi_reg_write(clk->map, mpll->init_regs,
> > > +                                      mpll->init_count);
> > > +
> > > +       /* Enable the fractional part */
> > > +       meson_parm_write(clk->map, &mpll->sdm_en, 1);
> > > +
> > > +       /* Set additional fractional part enable if required */
> > > +       if (MESON_PARM_APPLICABLE(&mpll->ssen))
> > > +               meson_parm_write(clk->map, &mpll->ssen, 1);
> > > +
> > > +       /* Set the magic misc bit if required */
> > > +       if (MESON_PARM_APPLICABLE(&mpll->misc))
> > > +               meson_parm_write(clk->map, &mpll->misc, 1);
> > > +}
> > > +
> > >  const struct clk_ops meson_clk_mpll_ro_ops = {
> > >         .recalc_rate    = mpll_recalc_rate,
> > >         .round_rate     = mpll_round_rate,
> > > @@ -148,6 +160,7 @@ const struct clk_ops meson_clk_mpll_ops = {
> > >         .recalc_rate    = mpll_recalc_rate,
> > >         .round_rate     = mpll_round_rate,
> > >         .set_rate       = mpll_set_rate,
> > > +       .init           = mpll_init,
> > 
> > We actively discourage using init callbacks. Can you do this some other
> > way?
> 
> Yes I'm aware of that but init it the right place to do this.
> To be clear, this is not initializing the clock to some particular rate, the
> rate is preserved.
> 
> It just applies the necessary settings that needs to be done only once to make
> sure the clock is in working order and that the rate calculated is actually
> accurate. 

Ok, but can you do that in your driver's probe routine instead of
attaching to the init callback? We want to get rid of "init" at some
point so throwing the init sequence stuff into the driver probe around
registration is a solution. Or we should think about not discouraging
the init callback.

Re: [PATCH 1/4] clk: meson: mpll: add init callback and regs

[Jerome Brunet]

On Fri, 2019-03-29 at 15:14 -0700, Stephen Boyd wrote:
> > > We actively discourage using init callbacks. Can you do this some other
> > > way?
> > 
> > Yes I'm aware of that but init it the right place to do this.
> > To be clear, this is not initializing the clock to some particular rate, the
> > rate is preserved.
> > 
> > It just applies the necessary settings that needs to be done only once to make
> > sure the clock is in working order and that the rate calculated is actually
> > accurate. 
> 
> Ok, but can you do that in your driver's probe routine instead of
> attaching to the init callback? We want to get rid of "init" at some
> point so throwing the init sequence stuff into the driver probe around
> registration is a solution. Or we should think about not discouraging
> the init callback

Is is callback really a problem after all ?
I think we should actively prevent using it to set a particular rate.

Here, the goal is put the clock in working order. The bootloader does not
always do that for us. I could put this in controller driver, but I would have
to repeat the init pattern for each instance of the clock...not nice 
Using the callback clearly shows the relationship between the init and the
clock. I think it is a lot better.

In the same series, I have added some init for the controller. In this case
the init target a group of clocks, so having the init in the controller makes
sense for that

Re: [PATCH 1/4] clk: meson: mpll: add init callback and regs

[Jerome Brunet]

On Fri, 2019-03-29 at 23:58 +0100, Jerome Brunet wrote:
> On Fri, 2019-03-29 at 15:14 -0700, Stephen Boyd wrote:
> > > > We actively discourage using init callbacks. Can you do this some other
> > > > way?
> > > 
> > > Yes I'm aware of that but init it the right place to do this.
> > > To be clear, this is not initializing the clock to some particular rate, the
> > > rate is preserved.
> > > 
> > > It just applies the necessary settings that needs to be done only once to make
> > > sure the clock is in working order and that the rate calculated is actually
> > > accurate. 
> > 
> > Ok, but can you do that in your driver's probe routine instead of
> > attaching to the init callback? We want to get rid of "init" at some
> > point so throwing the init sequence stuff into the driver probe around
> > registration is a solution. Or we should think about not discouraging
> > the init callback
> 
> Is is callback really a problem after all ?
> I think we should actively prevent using it to set a particular rate.
> 
> Here, the goal is put the clock in working order. The bootloader does not
> always do that for us. I could put this in controller driver, but I would have
> to repeat the init pattern for each instance of the clock...not nice 
> Using the callback clearly shows the relationship between the init and the
> clock. I think it is a lot better.
> 
> In the same series, I have added some init for the controller. In this case
> the init target a group of clocks, so having the init in the controller makes
> sense for that
> 
> 

Hi Stephen, Is it ok if we go ahead with this ?

BTW, it is not the only Amlogic clock using the .init callback (the pll and
sclk use it as well). It is not an excuse and I agree, there is always another
way to do things.

Still having an init() callback is really convenient for several cases.
Reworking those 3 drivers to do without it would not help maintainability.

Re: [PATCH 1/4] clk: meson: mpll: add init callback and regs

[Michael Turquette]

Hi Jerome,

On Fri, Mar 29, 2019 at 3:58 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
>
> On Fri, 2019-03-29 at 15:14 -0700, Stephen Boyd wrote:
> > > > We actively discourage using init callbacks. Can you do this some other
> > > > way?
> > >
> > > Yes I'm aware of that but init it the right place to do this.
> > > To be clear, this is not initializing the clock to some particular rate, the
> > > rate is preserved.
> > >
> > > It just applies the necessary settings that needs to be done only once to make
> > > sure the clock is in working order and that the rate calculated is actually
> > > accurate.
> >
> > Ok, but can you do that in your driver's probe routine instead of
> > attaching to the init callback? We want to get rid of "init" at some
> > point so throwing the init sequence stuff into the driver probe around
> > registration is a solution. Or we should think about not discouraging
> > the init callback
>
> Is is callback really a problem after all ?

It is a problem, insofar as Stephen and I want to remove .init some day.

> I think we should actively prevent using it to set a particular rate.
>
> Here, the goal is put the clock in working order. The bootloader does not
> always do that for us.

The above two sentences make it sound like this sequence belongs in .prepare().

I know that you're concerned with setting rate, but I guess it is safe
to assume that you'll always complete .prepare() before you begin to
execute .set_rate(), no? This also avoids the ugliness of putting it
in the .probe(), which I agree doesn't feel like an accurate thing to
do for a clock's own programming sequence.

.probe() is an OK place to put policy (turn these clocks on or set
them to this rate, for a known-good configuration).

Thanks,
Mike

> I could put this in controller driver, but I would have
> to repeat the init pattern for each instance of the clock...not nice
> Using the callback clearly shows the relationship between the init and the
> clock. I think it is a lot better.
>
> In the same series, I have added some init for the controller. In this case
> the init target a group of clocks, so having the init in the controller makes
> sense for that
>
>
>


-- 
Michael Turquette
CEO
BayLibre - At the Heart of Embedded Linux
http://baylibre.com/
Schedule a meeting: https://calendly.com/mturquette

Re: [PATCH 1/4] clk: meson: mpll: add init callback and regs

[Stephen Boyd]

Quoting Michael Turquette (2019-04-05 08:43:40)
> Hi Jerome,
> 
> On Fri, Mar 29, 2019 at 3:58 PM Jerome Brunet <jbrunet@baylibre.com> wrote:
> >
> > On Fri, 2019-03-29 at 15:14 -0700, Stephen Boyd wrote:
> > > > > We actively discourage using init callbacks. Can you do this some other
> > > > > way?
> > > >
> > > > Yes I'm aware of that but init it the right place to do this.
> > > > To be clear, this is not initializing the clock to some particular rate, the
> > > > rate is preserved.
> > > >
> > > > It just applies the necessary settings that needs to be done only once to make
> > > > sure the clock is in working order and that the rate calculated is actually
> > > > accurate.
> > >
> > > Ok, but can you do that in your driver's probe routine instead of
> > > attaching to the init callback? We want to get rid of "init" at some
> > > point so throwing the init sequence stuff into the driver probe around
> > > registration is a solution. Or we should think about not discouraging
> > > the init callback
> >
> > Is is callback really a problem after all ?
> 
> It is a problem, insofar as Stephen and I want to remove .init some day.
> 
> > I think we should actively prevent using it to set a particular rate.
> >
> > Here, the goal is put the clock in working order. The bootloader does not
> > always do that for us.
> 
> The above two sentences make it sound like this sequence belongs in .prepare().
> 
> I know that you're concerned with setting rate, but I guess it is safe
> to assume that you'll always complete .prepare() before you begin to
> execute .set_rate(), no? This also avoids the ugliness of putting it
> in the .probe(), which I agree doesn't feel like an accurate thing to
> do for a clock's own programming sequence.
> 
> .probe() is an OK place to put policy (turn these clocks on or set
> them to this rate, for a known-good configuration).
> 

Following along with this reasoning, maybe we need a 'prepare_once'
callback that does this the first time the clk is prepared or set_rate
is called on it. The problem I have with the init callback is that the
semantics of when it's called and what has happened before it's called
isn't clearly defined. I'm afraid to remove it and also afraid to move
around where it's called from. I've been itching to get it out of under
all the locks we're holding at registration time, but I don't know if
that's feasible, for example.