| Message ID | 20221018-clk-range-checks-fixes-v4-65-971d5077e7d2@cerno.tech (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | clk: Make determine_rate mandatory for muxes | expand |
05.05.2023 14:26, Maxime Ripard пишет: > The Tegra super clocks implements a mux with a set_parent hook, but > doesn't provide a determine_rate implementation. > > This is a bit odd, since set_parent() is there to, as its name implies, > change the parent of a clock. However, the most likely candidate to > trigger that parent change is a call to clk_set_rate(), with > determine_rate() figuring out which parent is the best suited for a > given rate. > > The other trigger would be a call to clk_set_parent(), but it's far less > used, and it doesn't look like there's any obvious user for that clock. > > So, the set_parent hook is effectively unused, possibly because of an > oversight. However, it could also be an explicit decision by the > original author to avoid any reparenting but through an explicit call to > clk_set_parent(). > > The driver does implement round_rate() though, which means that we can > change the rate of the clock, but we will never get to change the > parent. > > However, It's hard to tell whether it's been done on purpose or not. > > Since we'll start mandating a determine_rate() implementation, let's > convert the round_rate() implementation to a determine_rate(), which > will also make the current behavior explicit. And if it was an > oversight, the clock behaviour can be adjusted later on. > > Cc: Jonathan Hunter <jonathanh@nvidia.com> > Cc: Peter De Schrijver <pdeschrijver@nvidia.com> > Cc: Prashant Gaikwad <pgaikwad@nvidia.com> > Cc: Thierry Reding <thierry.reding@gmail.com> > Cc: linux-tegra@vger.kernel.org > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > --- > drivers/clk/tegra/clk-super.c | 15 +++++++++++---- > 1 file changed, 11 insertions(+), 4 deletions(-) > > diff --git a/drivers/clk/tegra/clk-super.c b/drivers/clk/tegra/clk-super.c > index 3f3a7a203c5f..7ec47942720c 100644 > --- a/drivers/clk/tegra/clk-super.c > +++ b/drivers/clk/tegra/clk-super.c > @@ -142,15 +142,22 @@ static const struct clk_ops tegra_clk_super_mux_ops = { > .restore_context = clk_super_mux_restore_context, > }; > > -static long clk_super_round_rate(struct clk_hw *hw, unsigned long rate, > - unsigned long *parent_rate) > +static int clk_super_determine_rate(struct clk_hw *hw, > + struct clk_rate_request *req) > { > struct tegra_clk_super_mux *super = to_clk_super_mux(hw); > struct clk_hw *div_hw = &super->frac_div.hw; > + unsigned long rate; > > __clk_hw_set_clk(div_hw, hw); > > - return super->div_ops->round_rate(div_hw, rate, parent_rate); > + rate = super->div_ops->round_rate(div_hw, req->rate, > + &req->best_parent_rate); > + if (rate < 0) > + return rate; > + > + req->rate = rate; > + return 0; > } > > static unsigned long clk_super_recalc_rate(struct clk_hw *hw, > @@ -193,7 +200,7 @@ const struct clk_ops tegra_clk_super_ops = { > .get_parent = clk_super_get_parent, > .set_parent = clk_super_set_parent, > .set_rate = clk_super_set_rate, > - .round_rate = clk_super_round_rate, > + .determine_rate = clk_super_determine_rate, > .recalc_rate = clk_super_recalc_rate, > .restore_context = clk_super_restore_context, > }; > Tegra30 doesn't boot anymore with this change. Best would be to keep the old behaviour for both sclk and periph tegra clocks.
Hi Dmitry, On Mon, Jun 19, 2023 at 02:38:59AM +0300, Dmitry Osipenko wrote: > 05.05.2023 14:26, Maxime Ripard пишет: > > The Tegra super clocks implements a mux with a set_parent hook, but > > doesn't provide a determine_rate implementation. > > > > This is a bit odd, since set_parent() is there to, as its name implies, > > change the parent of a clock. However, the most likely candidate to > > trigger that parent change is a call to clk_set_rate(), with > > determine_rate() figuring out which parent is the best suited for a > > given rate. > > > > The other trigger would be a call to clk_set_parent(), but it's far less > > used, and it doesn't look like there's any obvious user for that clock. > > > > So, the set_parent hook is effectively unused, possibly because of an > > oversight. However, it could also be an explicit decision by the > > original author to avoid any reparenting but through an explicit call to > > clk_set_parent(). > > > > The driver does implement round_rate() though, which means that we can > > change the rate of the clock, but we will never get to change the > > parent. > > > > However, It's hard to tell whether it's been done on purpose or not. > > > > Since we'll start mandating a determine_rate() implementation, let's > > convert the round_rate() implementation to a determine_rate(), which > > will also make the current behavior explicit. And if it was an > > oversight, the clock behaviour can be adjusted later on. > > > > Cc: Jonathan Hunter <jonathanh@nvidia.com> > > Cc: Peter De Schrijver <pdeschrijver@nvidia.com> > > Cc: Prashant Gaikwad <pgaikwad@nvidia.com> > > Cc: Thierry Reding <thierry.reding@gmail.com> > > Cc: linux-tegra@vger.kernel.org > > Signed-off-by: Maxime Ripard <maxime@cerno.tech> > > --- > > drivers/clk/tegra/clk-super.c | 15 +++++++++++---- > > 1 file changed, 11 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/clk/tegra/clk-super.c b/drivers/clk/tegra/clk-super.c > > index 3f3a7a203c5f..7ec47942720c 100644 > > --- a/drivers/clk/tegra/clk-super.c > > +++ b/drivers/clk/tegra/clk-super.c > > @@ -142,15 +142,22 @@ static const struct clk_ops tegra_clk_super_mux_ops = { > > .restore_context = clk_super_mux_restore_context, > > }; > > > > -static long clk_super_round_rate(struct clk_hw *hw, unsigned long rate, > > - unsigned long *parent_rate) > > +static int clk_super_determine_rate(struct clk_hw *hw, > > + struct clk_rate_request *req) > > { > > struct tegra_clk_super_mux *super = to_clk_super_mux(hw); > > struct clk_hw *div_hw = &super->frac_div.hw; > > + unsigned long rate; > > > > __clk_hw_set_clk(div_hw, hw); > > > > - return super->div_ops->round_rate(div_hw, rate, parent_rate); > > + rate = super->div_ops->round_rate(div_hw, req->rate, > > + &req->best_parent_rate); > > + if (rate < 0) > > + return rate; > > + > > + req->rate = rate; > > + return 0; > > } > > > > static unsigned long clk_super_recalc_rate(struct clk_hw *hw, > > @@ -193,7 +200,7 @@ const struct clk_ops tegra_clk_super_ops = { > > .get_parent = clk_super_get_parent, > > .set_parent = clk_super_set_parent, > > .set_rate = clk_super_set_rate, > > - .round_rate = clk_super_round_rate, > > + .determine_rate = clk_super_determine_rate, > > .recalc_rate = clk_super_recalc_rate, > > .restore_context = clk_super_restore_context, > > }; > > > > Tegra30 doesn't boot anymore with this change. Best would be to keep the > old behaviour for both sclk and periph tegra clocks. I took a closer look at the patch and can't find anything different to what the core is doing if there's a round_rate implementation: https://elixir.bootlin.com/linux/latest/source/drivers/clk/clk.c#L1459 Also, it's not clear to me how that driver is used. It looks like div_ops is always supposed to be set, and super clocks are registered with either tegra_clk_register_super_clk() or tegra_clk_register_super_mux() tegra_clk_register_super_clk() sets the div_ops pointer to tegra_clk_super_ops, but tegra30 doesn't seem to call it. tegra_clk_register_super_mux() doesn't set the div_ops pointer, but is used by tegra30, so I would assume that it's the broken one. But I'm confused, since div_ops doesn't seem to be set anywhere? Maxime
Quoting Maxime Ripard (2023-06-19 00:26:19) > On Mon, Jun 19, 2023 at 02:38:59AM +0300, Dmitry Osipenko wrote: > > 05.05.2023 14:26, Maxime Ripard пишет: > > > > > > diff --git a/drivers/clk/tegra/clk-super.c b/drivers/clk/tegra/clk-super.c > > > index 3f3a7a203c5f..7ec47942720c 100644 > > > --- a/drivers/clk/tegra/clk-super.c > > > +++ b/drivers/clk/tegra/clk-super.c > > > @@ -142,15 +142,22 @@ static const struct clk_ops tegra_clk_super_mux_ops = { > > > .restore_context = clk_super_mux_restore_context, > > > }; > > > > > > -static long clk_super_round_rate(struct clk_hw *hw, unsigned long rate, > > > - unsigned long *parent_rate) > > > +static int clk_super_determine_rate(struct clk_hw *hw, > > > + struct clk_rate_request *req) > > > { > > > struct tegra_clk_super_mux *super = to_clk_super_mux(hw); > > > struct clk_hw *div_hw = &super->frac_div.hw; > > > + unsigned long rate; > > > > > > __clk_hw_set_clk(div_hw, hw); > > > > > > - return super->div_ops->round_rate(div_hw, rate, parent_rate); > > > + rate = super->div_ops->round_rate(div_hw, req->rate, > > > + &req->best_parent_rate); > > > + if (rate < 0) There's the report that this condition is never possible. Maybe the previous code was relying on an error value sometimes. Can we add determine_rate to the div_ops and simplify this code? I asked on the list for that earlier.
On Tue, Jun 20, 2023 at 12:09:09PM -0700, Stephen Boyd wrote: > Quoting Maxime Ripard (2023-06-19 00:26:19) > > On Mon, Jun 19, 2023 at 02:38:59AM +0300, Dmitry Osipenko wrote: > > > 05.05.2023 14:26, Maxime Ripard пишет: > > > > > > > > diff --git a/drivers/clk/tegra/clk-super.c b/drivers/clk/tegra/clk-super.c > > > > index 3f3a7a203c5f..7ec47942720c 100644 > > > > --- a/drivers/clk/tegra/clk-super.c > > > > +++ b/drivers/clk/tegra/clk-super.c > > > > @@ -142,15 +142,22 @@ static const struct clk_ops tegra_clk_super_mux_ops = { > > > > .restore_context = clk_super_mux_restore_context, > > > > }; > > > > > > > > -static long clk_super_round_rate(struct clk_hw *hw, unsigned long rate, > > > > - unsigned long *parent_rate) > > > > +static int clk_super_determine_rate(struct clk_hw *hw, > > > > + struct clk_rate_request *req) > > > > { > > > > struct tegra_clk_super_mux *super = to_clk_super_mux(hw); > > > > struct clk_hw *div_hw = &super->frac_div.hw; > > > > + unsigned long rate; > > > > > > > > __clk_hw_set_clk(div_hw, hw); > > > > > > > > - return super->div_ops->round_rate(div_hw, rate, parent_rate); > > > > + rate = super->div_ops->round_rate(div_hw, req->rate, > > > > + &req->best_parent_rate); > > > > + if (rate < 0) > > There's the report that this condition is never possible. Maybe the > previous code was relying on an error value sometimes. Can we add > determine_rate to the div_ops and simplify this code? I asked on the > list for that earlier. I was able to reproduce this on a Tegra30 Beaver, but the problem is more straightforward than this. The crash I was seeing during boot was because cclk_super_determine_rate() was still calling the round_rate() callback from tegra_clk_super_ops, which this patch removed (and added determine_rate() instead). The following fixes the problem for me. It's basically converting the round_rate() call to an equivalent determine_rate() call. Dmitry, can you verify that this fixes the issue that you were seeing? Thierry --- >8 --- diff --git a/drivers/clk/tegra/clk-tegra-super-cclk.c b/drivers/clk/tegra/clk-tegra-super-cclk.c index 68d7bcd5fc8a..8a2bb4ae4fd2 100644 --- a/drivers/clk/tegra/clk-tegra-super-cclk.c +++ b/drivers/clk/tegra/clk-tegra-super-cclk.c @@ -86,9 +86,16 @@ static int cclk_super_determine_rate(struct clk_hw *hw, if (rate <= pllp_rate) { if (super->flags & TEGRA20_SUPER_CLK) rate = pllp_rate; - else - rate = tegra_clk_super_ops.round_rate(hw, rate, - &pllp_rate); + else { + struct clk_rate_request parent = { + .rate = req->rate, + .best_parent_rate = pllp_rate, + }; + + tegra_clk_super_ops.determine_rate(hw, &parent); + pllp_rate = parent.best_parent_rate; + rate = parent.rate; + } req->best_parent_rate = pllp_rate; req->best_parent_hw = pllp_hw; --- >8 ---
Hi, On Wed, Jun 21, 2023 at 05:35:09PM +0200, Thierry Reding wrote: > On Tue, Jun 20, 2023 at 12:09:09PM -0700, Stephen Boyd wrote: > > Quoting Maxime Ripard (2023-06-19 00:26:19) > > > On Mon, Jun 19, 2023 at 02:38:59AM +0300, Dmitry Osipenko wrote: > > > > 05.05.2023 14:26, Maxime Ripard пишет: > > > > > > > > > > diff --git a/drivers/clk/tegra/clk-super.c b/drivers/clk/tegra/clk-super.c > > > > > index 3f3a7a203c5f..7ec47942720c 100644 > > > > > --- a/drivers/clk/tegra/clk-super.c > > > > > +++ b/drivers/clk/tegra/clk-super.c > > > > > @@ -142,15 +142,22 @@ static const struct clk_ops tegra_clk_super_mux_ops = { > > > > > .restore_context = clk_super_mux_restore_context, > > > > > }; > > > > > > > > > > -static long clk_super_round_rate(struct clk_hw *hw, unsigned long rate, > > > > > - unsigned long *parent_rate) > > > > > +static int clk_super_determine_rate(struct clk_hw *hw, > > > > > + struct clk_rate_request *req) > > > > > { > > > > > struct tegra_clk_super_mux *super = to_clk_super_mux(hw); > > > > > struct clk_hw *div_hw = &super->frac_div.hw; > > > > > + unsigned long rate; > > > > > > > > > > __clk_hw_set_clk(div_hw, hw); > > > > > > > > > > - return super->div_ops->round_rate(div_hw, rate, parent_rate); > > > > > + rate = super->div_ops->round_rate(div_hw, req->rate, > > > > > + &req->best_parent_rate); > > > > > + if (rate < 0) > > > > There's the report that this condition is never possible. Maybe the > > previous code was relying on an error value sometimes. Can we add > > determine_rate to the div_ops and simplify this code? I asked on the > > list for that earlier. > > I was able to reproduce this on a Tegra30 Beaver, but the problem is > more straightforward than this. The crash I was seeing during boot was > because cclk_super_determine_rate() was still calling the round_rate() > callback from tegra_clk_super_ops, which this patch removed (and added > determine_rate() instead). > > The following fixes the problem for me. It's basically converting the > round_rate() call to an equivalent determine_rate() call. Thanks for figuring it out :) > Dmitry, can you verify that this fixes the issue that you were seeing? > > Thierry > > --- >8 --- > diff --git a/drivers/clk/tegra/clk-tegra-super-cclk.c b/drivers/clk/tegra/clk-tegra-super-cclk.c > index 68d7bcd5fc8a..8a2bb4ae4fd2 100644 > --- a/drivers/clk/tegra/clk-tegra-super-cclk.c > +++ b/drivers/clk/tegra/clk-tegra-super-cclk.c > @@ -86,9 +86,16 @@ static int cclk_super_determine_rate(struct clk_hw *hw, > if (rate <= pllp_rate) { > if (super->flags & TEGRA20_SUPER_CLK) > rate = pllp_rate; > - else > - rate = tegra_clk_super_ops.round_rate(hw, rate, > - &pllp_rate); > + else { > + struct clk_rate_request parent = { > + .rate = req->rate, > + .best_parent_rate = pllp_rate, > + }; If it works and you submit a patch later, this needs to be changed to clk_hw_init_rate_request() Maxime
21.06.2023 18:35, Thierry Reding пишет: > On Tue, Jun 20, 2023 at 12:09:09PM -0700, Stephen Boyd wrote: >> Quoting Maxime Ripard (2023-06-19 00:26:19) >>> On Mon, Jun 19, 2023 at 02:38:59AM +0300, Dmitry Osipenko wrote: >>>> 05.05.2023 14:26, Maxime Ripard пишет: >>>>> >>>>> diff --git a/drivers/clk/tegra/clk-super.c b/drivers/clk/tegra/clk-super.c >>>>> index 3f3a7a203c5f..7ec47942720c 100644 >>>>> --- a/drivers/clk/tegra/clk-super.c >>>>> +++ b/drivers/clk/tegra/clk-super.c >>>>> @@ -142,15 +142,22 @@ static const struct clk_ops tegra_clk_super_mux_ops = { >>>>> .restore_context = clk_super_mux_restore_context, >>>>> }; >>>>> >>>>> -static long clk_super_round_rate(struct clk_hw *hw, unsigned long rate, >>>>> - unsigned long *parent_rate) >>>>> +static int clk_super_determine_rate(struct clk_hw *hw, >>>>> + struct clk_rate_request *req) >>>>> { >>>>> struct tegra_clk_super_mux *super = to_clk_super_mux(hw); >>>>> struct clk_hw *div_hw = &super->frac_div.hw; >>>>> + unsigned long rate; >>>>> >>>>> __clk_hw_set_clk(div_hw, hw); >>>>> >>>>> - return super->div_ops->round_rate(div_hw, rate, parent_rate); >>>>> + rate = super->div_ops->round_rate(div_hw, req->rate, >>>>> + &req->best_parent_rate); >>>>> + if (rate < 0) >> >> There's the report that this condition is never possible. Maybe the >> previous code was relying on an error value sometimes. Can we add >> determine_rate to the div_ops and simplify this code? I asked on the >> list for that earlier. > > I was able to reproduce this on a Tegra30 Beaver, but the problem is > more straightforward than this. The crash I was seeing during boot was > because cclk_super_determine_rate() was still calling the round_rate() > callback from tegra_clk_super_ops, which this patch removed (and added > determine_rate() instead). > > The following fixes the problem for me. It's basically converting the > round_rate() call to an equivalent determine_rate() call. > > Dmitry, can you verify that this fixes the issue that you were seeing? > > Thierry > > --- >8 --- > diff --git a/drivers/clk/tegra/clk-tegra-super-cclk.c b/drivers/clk/tegra/clk-tegra-super-cclk.c > index 68d7bcd5fc8a..8a2bb4ae4fd2 100644 > --- a/drivers/clk/tegra/clk-tegra-super-cclk.c > +++ b/drivers/clk/tegra/clk-tegra-super-cclk.c > @@ -86,9 +86,16 @@ static int cclk_super_determine_rate(struct clk_hw *hw, > if (rate <= pllp_rate) { > if (super->flags & TEGRA20_SUPER_CLK) > rate = pllp_rate; > - else > - rate = tegra_clk_super_ops.round_rate(hw, rate, > - &pllp_rate); > + else { > + struct clk_rate_request parent = { > + .rate = req->rate, > + .best_parent_rate = pllp_rate, > + }; > + > + tegra_clk_super_ops.determine_rate(hw, &parent); > + pllp_rate = parent.best_parent_rate; > + rate = parent.rate; > + } > > req->best_parent_rate = pllp_rate; > req->best_parent_hw = pllp_hw; > --- >8 --- Thank you for the fix, it works! Feel free to add my t-b :)
On Thu, Jun 22, 2023 at 01:24:12PM +0200, Maxime Ripard wrote: > Hi, > > On Wed, Jun 21, 2023 at 05:35:09PM +0200, Thierry Reding wrote: > > On Tue, Jun 20, 2023 at 12:09:09PM -0700, Stephen Boyd wrote: > > > Quoting Maxime Ripard (2023-06-19 00:26:19) > > > > On Mon, Jun 19, 2023 at 02:38:59AM +0300, Dmitry Osipenko wrote: > > > > > 05.05.2023 14:26, Maxime Ripard пишет: > > > > > > > > > > > > diff --git a/drivers/clk/tegra/clk-super.c b/drivers/clk/tegra/clk-super.c > > > > > > index 3f3a7a203c5f..7ec47942720c 100644 > > > > > > --- a/drivers/clk/tegra/clk-super.c > > > > > > +++ b/drivers/clk/tegra/clk-super.c > > > > > > @@ -142,15 +142,22 @@ static const struct clk_ops tegra_clk_super_mux_ops = { > > > > > > .restore_context = clk_super_mux_restore_context, > > > > > > }; > > > > > > > > > > > > -static long clk_super_round_rate(struct clk_hw *hw, unsigned long rate, > > > > > > - unsigned long *parent_rate) > > > > > > +static int clk_super_determine_rate(struct clk_hw *hw, > > > > > > + struct clk_rate_request *req) > > > > > > { > > > > > > struct tegra_clk_super_mux *super = to_clk_super_mux(hw); > > > > > > struct clk_hw *div_hw = &super->frac_div.hw; > > > > > > + unsigned long rate; > > > > > > > > > > > > __clk_hw_set_clk(div_hw, hw); > > > > > > > > > > > > - return super->div_ops->round_rate(div_hw, rate, parent_rate); > > > > > > + rate = super->div_ops->round_rate(div_hw, req->rate, > > > > > > + &req->best_parent_rate); > > > > > > + if (rate < 0) > > > > > > There's the report that this condition is never possible. Maybe the > > > previous code was relying on an error value sometimes. Can we add > > > determine_rate to the div_ops and simplify this code? I asked on the > > > list for that earlier. > > > > I was able to reproduce this on a Tegra30 Beaver, but the problem is > > more straightforward than this. The crash I was seeing during boot was > > because cclk_super_determine_rate() was still calling the round_rate() > > callback from tegra_clk_super_ops, which this patch removed (and added > > determine_rate() instead). > > > > The following fixes the problem for me. It's basically converting the > > round_rate() call to an equivalent determine_rate() call. > > Thanks for figuring it out :) > > > Dmitry, can you verify that this fixes the issue that you were seeing? > > > > Thierry > > > > --- >8 --- > > diff --git a/drivers/clk/tegra/clk-tegra-super-cclk.c b/drivers/clk/tegra/clk-tegra-super-cclk.c > > index 68d7bcd5fc8a..8a2bb4ae4fd2 100644 > > --- a/drivers/clk/tegra/clk-tegra-super-cclk.c > > +++ b/drivers/clk/tegra/clk-tegra-super-cclk.c > > @@ -86,9 +86,16 @@ static int cclk_super_determine_rate(struct clk_hw *hw, > > if (rate <= pllp_rate) { > > if (super->flags & TEGRA20_SUPER_CLK) > > rate = pllp_rate; > > - else > > - rate = tegra_clk_super_ops.round_rate(hw, rate, > > - &pllp_rate); > > + else { > > + struct clk_rate_request parent = { > > + .rate = req->rate, > > + .best_parent_rate = pllp_rate, > > + }; > > If it works and you submit a patch later, this needs to be changed to > clk_hw_init_rate_request() I've tried this and while it seems to work, this doesn't seem to be exactly the same as what the original code does. From what I understand the parent clock can be either pll-p or pll-x, but what we want to do in this branch is check what a configuration would look like for pll-p as the parent. clk_hw_init_rate_request() would initialize the request with data for the current parent, even if that's not pll-p, so I'm a bit hesitant to go with that instead of manually hard-coding this to pll-p. Thierry
On Fri, Jun 23, 2023 at 04:51:27PM +0200, Thierry Reding wrote: > On Thu, Jun 22, 2023 at 01:24:12PM +0200, Maxime Ripard wrote: > > Hi, > > > > On Wed, Jun 21, 2023 at 05:35:09PM +0200, Thierry Reding wrote: > > > On Tue, Jun 20, 2023 at 12:09:09PM -0700, Stephen Boyd wrote: > > > > Quoting Maxime Ripard (2023-06-19 00:26:19) > > > > > On Mon, Jun 19, 2023 at 02:38:59AM +0300, Dmitry Osipenko wrote: > > > > > > 05.05.2023 14:26, Maxime Ripard пишет: > > > > > > > > > > > > > > diff --git a/drivers/clk/tegra/clk-super.c b/drivers/clk/tegra/clk-super.c > > > > > > > index 3f3a7a203c5f..7ec47942720c 100644 > > > > > > > --- a/drivers/clk/tegra/clk-super.c > > > > > > > +++ b/drivers/clk/tegra/clk-super.c > > > > > > > @@ -142,15 +142,22 @@ static const struct clk_ops tegra_clk_super_mux_ops = { > > > > > > > .restore_context = clk_super_mux_restore_context, > > > > > > > }; > > > > > > > > > > > > > > -static long clk_super_round_rate(struct clk_hw *hw, unsigned long rate, > > > > > > > - unsigned long *parent_rate) > > > > > > > +static int clk_super_determine_rate(struct clk_hw *hw, > > > > > > > + struct clk_rate_request *req) > > > > > > > { > > > > > > > struct tegra_clk_super_mux *super = to_clk_super_mux(hw); > > > > > > > struct clk_hw *div_hw = &super->frac_div.hw; > > > > > > > + unsigned long rate; > > > > > > > > > > > > > > __clk_hw_set_clk(div_hw, hw); > > > > > > > > > > > > > > - return super->div_ops->round_rate(div_hw, rate, parent_rate); > > > > > > > + rate = super->div_ops->round_rate(div_hw, req->rate, > > > > > > > + &req->best_parent_rate); > > > > > > > + if (rate < 0) > > > > > > > > There's the report that this condition is never possible. Maybe the > > > > previous code was relying on an error value sometimes. Can we add > > > > determine_rate to the div_ops and simplify this code? I asked on the > > > > list for that earlier. > > > > > > I was able to reproduce this on a Tegra30 Beaver, but the problem is > > > more straightforward than this. The crash I was seeing during boot was > > > because cclk_super_determine_rate() was still calling the round_rate() > > > callback from tegra_clk_super_ops, which this patch removed (and added > > > determine_rate() instead). > > > > > > The following fixes the problem for me. It's basically converting the > > > round_rate() call to an equivalent determine_rate() call. > > > > Thanks for figuring it out :) > > > > > Dmitry, can you verify that this fixes the issue that you were seeing? > > > > > > Thierry > > > > > > --- >8 --- > > > diff --git a/drivers/clk/tegra/clk-tegra-super-cclk.c b/drivers/clk/tegra/clk-tegra-super-cclk.c > > > index 68d7bcd5fc8a..8a2bb4ae4fd2 100644 > > > --- a/drivers/clk/tegra/clk-tegra-super-cclk.c > > > +++ b/drivers/clk/tegra/clk-tegra-super-cclk.c > > > @@ -86,9 +86,16 @@ static int cclk_super_determine_rate(struct clk_hw *hw, > > > if (rate <= pllp_rate) { > > > if (super->flags & TEGRA20_SUPER_CLK) > > > rate = pllp_rate; > > > - else > > > - rate = tegra_clk_super_ops.round_rate(hw, rate, > > > - &pllp_rate); > > > + else { > > > + struct clk_rate_request parent = { > > > + .rate = req->rate, > > > + .best_parent_rate = pllp_rate, > > > + }; > > > > If it works and you submit a patch later, this needs to be changed to > > clk_hw_init_rate_request() > > I've tried this and while it seems to work, this doesn't seem to be > exactly the same as what the original code does. From what I understand > the parent clock can be either pll-p or pll-x, but what we want to do in > this branch is check what a configuration would look like for pll-p as > the parent. clk_hw_init_rate_request() would initialize the request with > data for the current parent, even if that's not pll-p, so I'm a bit > hesitant to go with that instead of manually hard-coding this to pll-p. Ah, yes, sorry. Maybe we need some kind of variant to address this then, but for the time being you can at least set req->min_rate and req->max_rate using clk_hw_get_rate_range. Maxime
Quoting Thierry Reding (2023-06-21 08:35:09) > > I was able to reproduce this on a Tegra30 Beaver, but the problem is > more straightforward than this. The crash I was seeing during boot was > because cclk_super_determine_rate() was still calling the round_rate() > callback from tegra_clk_super_ops, which this patch removed (and added > determine_rate() instead). > > The following fixes the problem for me. It's basically converting the > round_rate() call to an equivalent determine_rate() call. > > Dmitry, can you verify that this fixes the issue that you were seeing? > Can you send this as a proper patch? I'd like to send this early next week.
diff --git a/drivers/clk/tegra/clk-super.c b/drivers/clk/tegra/clk-super.c index 3f3a7a203c5f..7ec47942720c 100644 --- a/drivers/clk/tegra/clk-super.c +++ b/drivers/clk/tegra/clk-super.c @@ -142,15 +142,22 @@ static const struct clk_ops tegra_clk_super_mux_ops = { .restore_context = clk_super_mux_restore_context, }; -static long clk_super_round_rate(struct clk_hw *hw, unsigned long rate, - unsigned long *parent_rate) +static int clk_super_determine_rate(struct clk_hw *hw, + struct clk_rate_request *req) { struct tegra_clk_super_mux *super = to_clk_super_mux(hw); struct clk_hw *div_hw = &super->frac_div.hw; + unsigned long rate; __clk_hw_set_clk(div_hw, hw); - return super->div_ops->round_rate(div_hw, rate, parent_rate); + rate = super->div_ops->round_rate(div_hw, req->rate, + &req->best_parent_rate); + if (rate < 0) + return rate; + + req->rate = rate; + return 0; } static unsigned long clk_super_recalc_rate(struct clk_hw *hw, @@ -193,7 +200,7 @@ const struct clk_ops tegra_clk_super_ops = { .get_parent = clk_super_get_parent, .set_parent = clk_super_set_parent, .set_rate = clk_super_set_rate, - .round_rate = clk_super_round_rate, + .determine_rate = clk_super_determine_rate, .recalc_rate = clk_super_recalc_rate, .restore_context = clk_super_restore_context, };
The Tegra super clocks implements a mux with a set_parent hook, but doesn't provide a determine_rate implementation. This is a bit odd, since set_parent() is there to, as its name implies, change the parent of a clock. However, the most likely candidate to trigger that parent change is a call to clk_set_rate(), with determine_rate() figuring out which parent is the best suited for a given rate. The other trigger would be a call to clk_set_parent(), but it's far less used, and it doesn't look like there's any obvious user for that clock. So, the set_parent hook is effectively unused, possibly because of an oversight. However, it could also be an explicit decision by the original author to avoid any reparenting but through an explicit call to clk_set_parent(). The driver does implement round_rate() though, which means that we can change the rate of the clock, but we will never get to change the parent. However, It's hard to tell whether it's been done on purpose or not. Since we'll start mandating a determine_rate() implementation, let's convert the round_rate() implementation to a determine_rate(), which will also make the current behavior explicit. And if it was an oversight, the clock behaviour can be adjusted later on. Cc: Jonathan Hunter <jonathanh@nvidia.com> Cc: Peter De Schrijver <pdeschrijver@nvidia.com> Cc: Prashant Gaikwad <pgaikwad@nvidia.com> Cc: Thierry Reding <thierry.reding@gmail.com> Cc: linux-tegra@vger.kernel.org Signed-off-by: Maxime Ripard <maxime@cerno.tech> --- drivers/clk/tegra/clk-super.c | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-)