| Message ID | 20190325111200.15940-2-jbrunet@baylibre.com |
|---|---|
| State | New |
| Headers | show |
| Series | clk: meson: fixup g12a mpll | expand |
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); >
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); > >
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.
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
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.
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
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.
On Fri, 2019-04-05 at 13:43 -0700, Stephen Boyd wrote: > 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. > If removing .init() is important for you, I would prefer to help guys. That being said, we need a decent solution to some use case if this is going to work. I'll illustrate with examples from the meson code but I think they represent fairly common cases: 1) clk-pll: Without the work done init(), the pll just won't lock ... I agree that we would see the problem when the clock is finally enabled, so we could do "init" sequence in .prepare() each time it is called. The sequence might be "long" (with a couple of delays in it) so doing it on each call to .prepare() is wasting time but it could work. Something like .prepare_once would help for this. 2) clk-mpll: Without the work done in .init(), the fractional part of the divider will not work, only the integer part works => the rate calculated is off by a significant margin. IOW, until the initial setup is done, the result of .recalc_rate() is off and cannot be trusted. .prepare() (and .prepare_once() if called a the same stage) is too late. We would need something that runs before any call to .recalc_rate() is made. We could also think about clocks with the ability to observe and feedback (read-only) on the actual output signal. Such thing might need an initial() setup as well. 3) sclk: This is a kind of divider which gates when the value is zero. We need to save the divider value on .disable() to restore it on .enable(). In .init(), if divider value is 0 (gated) we init cached value to the maximum divider value. This done so a call to .enable() works, even without prior calls to .set_rate(). Here, again, I think .prepare() is a too late. Maybe it is a bit extreme but we could also think about weird muxes not reporting the parent accurately until some prior setting is done. For those you need something that runs before all the orphan mechanism in the clock register. Whatever the name we give it, It think it would help if we had a (not discouraged) callback that is guaranteed to run before anything else is called on the clock. This is what I tried to do with 541debae0adf ("clk: call the clock init() callback before any other ops callback"). Having the counterpart callback, guaranteed to run last, would allow a clock to allocate (and de-allocate) stuff. It would be an interesting addition IMO. For clocks which needs to store things (such as sclk), it would allow to take the runtime data out of the init data. What about .register() and .deregister() ? It would map nicely to the CCF API ?
Hi Jerome, On Mon, Apr 8, 2019 at 9:38 AM Jerome Brunet <jbrunet@baylibre.com> wrote: > > On Fri, 2019-04-05 at 13:43 -0700, Stephen Boyd wrote: > > 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. > > > > If removing .init() is important for you, I would prefer to help guys. That > being said, we need a decent solution to some use case if this is going to > work. > > I'll illustrate with examples from the meson code but I think they represent > fairly common cases: > > 1) clk-pll: Without the work done init(), the pll just won't lock ... > > I agree that we would see the problem when the clock is finally enabled, so we > could do "init" sequence in .prepare() each time it is called. The sequence > might be "long" (with a couple of delays in it) so doing it on each call to > .prepare() is wasting time but it could work. > > Something like .prepare_once would help for this. > > 2) clk-mpll: Without the work done in .init(), the fractional part of the > divider will not work, only the integer part works => the rate calculated is > off by a significant margin. IOW, until the initial setup is done, the result > of .recalc_rate() is off and cannot be trusted. > > .prepare() (and .prepare_once() if called a the same stage) is too late. We > would need something that runs before any call to .recalc_rate() is made. > > We could also think about clocks with the ability to observe and feedback > (read-only) on the actual output signal. Such thing might need an initial() > setup as well. > > 3) sclk: This is a kind of divider which gates when the value is zero. We need > to save the divider value on .disable() to restore it on .enable(). In > .init(), if divider value is 0 (gated) we init cached value to the maximum > divider value. This done so a call to .enable() works, even without prior > calls to .set_rate(). > > Here, again, I think .prepare() is a too late. > > Maybe it is a bit extreme but we could also think about weird muxes not > reporting the parent accurately until some prior setting is done. For those > you need something that runs before all the orphan mechanism in the clock > register. > > Whatever the name we give it, It think it would help if we had a (not > discouraged) callback that is guaranteed to run before anything else is called > on the clock. This is what I tried to do with 541debae0adf ("clk: call the > clock init() callback before any other ops callback"). > > Having the counterpart callback, guaranteed to run last, would allow a clock > to allocate (and de-allocate) stuff. It would be an interesting addition IMO. > For clocks which needs to store things (such as sclk), it would allow to take > the runtime data out of the init data. > > What about .register() and .deregister() ? It would map nicely to the CCF API > ? I like .register & .deregister. I propose that we merge your .init to keep things moving BUT ONLY if you pinky swear to follow up with .register & .deregister conversion and convert all existing .init users over to .register. Deal? Stephen, thoughts? Thanks, Mike > > > > -- Michael Turquette CEO BayLibre - At the Heart of Embedded Linux http://baylibre.com/ Schedule a meeting: https://calendly.com/mturquette
Quoting Michael Turquette (2019-04-23 10:34:13) > On Mon, Apr 8, 2019 at 9:38 AM Jerome Brunet <jbrunet@baylibre.com> wrote: > > > > If removing .init() is important for you, I would prefer to help guys. That > > being said, we need a decent solution to some use case if this is going to > > work. > > > > I'll illustrate with examples from the meson code but I think they represent > > fairly common cases: > > > > 1) clk-pll: Without the work done init(), the pll just won't lock ... > > > > I agree that we would see the problem when the clock is finally enabled, so we > > could do "init" sequence in .prepare() each time it is called. The sequence > > might be "long" (with a couple of delays in it) so doing it on each call to > > .prepare() is wasting time but it could work. > > > > Something like .prepare_once would help for this. > > > > 2) clk-mpll: Without the work done in .init(), the fractional part of the > > divider will not work, only the integer part works => the rate calculated is > > off by a significant margin. IOW, until the initial setup is done, the result > > of .recalc_rate() is off and cannot be trusted. > > > > .prepare() (and .prepare_once() if called a the same stage) is too late. We > > would need something that runs before any call to .recalc_rate() is made. > > > > We could also think about clocks with the ability to observe and feedback > > (read-only) on the actual output signal. Such thing might need an initial() > > setup as well. > > > > 3) sclk: This is a kind of divider which gates when the value is zero. We need > > to save the divider value on .disable() to restore it on .enable(). In > > .init(), if divider value is 0 (gated) we init cached value to the maximum > > divider value. This done so a call to .enable() works, even without prior > > calls to .set_rate(). > > > > Here, again, I think .prepare() is a too late. > > > > Maybe it is a bit extreme but we could also think about weird muxes not > > reporting the parent accurately until some prior setting is done. For those > > you need something that runs before all the orphan mechanism in the clock > > register. Well I wonder if it would work to do something on clk_get() path. At that point, we've decided that a consumer is going to be able to use the clk and so we can clearly define that this is the last moment to do any final setup/configuration before handing out a pointer to a consumer. There are still the other cases you talk about though where .recalc_rate() wouldn't work, and we would need to do this configuration before we calculate the rate of this clk when registering it. Seems to be an argument for .init or the rename of it sticking around. > > > > Whatever the name we give it, It think it would help if we had a (not > > discouraged) callback that is guaranteed to run before anything else is called > > on the clock. This is what I tried to do with 541debae0adf ("clk: call the > > clock init() callback before any other ops callback"). > > > > Having the counterpart callback, guaranteed to run last, would allow a clock > > to allocate (and de-allocate) stuff. It would be an interesting addition IMO. > > For clocks which needs to store things (such as sclk), it would allow to take > > the runtime data out of the init data. > > > > What about .register() and .deregister() ? It would map nicely to the CCF API > > ? > > I like .register & .deregister. > > I propose that we merge your .init to keep things moving BUT ONLY if > you pinky swear to follow up with .register & .deregister conversion > and convert all existing .init users over to .register. Deal? > > Stephen, thoughts? > Ok. I'd like to see the semantics of .register and .deregister (.unregister?) be well defined, especially around what locks are held when the op is called (do both prepare and enable locks need to be held?) and what state the clk is in (can it be an orphan?). Alternatively, we can retroactively do that for the .init op and then introduce the other ops if drivers don't need this. Then we're just making .init more official and not shunned, which is also fine. This crude grep only finds a few clks that use the init op. $ git grep -W 'struct clk_ops .*=' | grep "\.init =" drivers/clk/mmp/clk-frac.c- .init = clk_factor_init, drivers/clk/mmp/clk-mix.c- .init = mmp_clk_mix_init, drivers/clk/qcom/clk-hfpll.c- .init = clk_hfpll_init, drivers/clk/rockchip/clk-pll.c- .init = rockchip_rk3036_pll_init, drivers/clk/rockchip/clk-pll.c- .init = rockchip_rk3066_pll_init, drivers/clk/rockchip/clk-pll.c- .init = rockchip_rk3399_pll_init, drivers/clk/rockchip/clk.c- frac_mux->hw.init = &init;
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; };
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(-) -- 2.20.1