| Message ID | 20230919140408.2608521-1-u-kumar1@ti.com |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [v2] clk: ti: Notify AVS driver upon setting clock rate | expand |
On 19:34-20230919, Udit Kumar wrote: > AVS is enabled at R5 SPL stage, on few platforms like J721E > and J7200 clk-k3 is used instead if clk-sci driver. > > Add support in clk-k3 driver as well to notify AVS driver > upon setting clock rate so that voltage is changed accordingly. > > Cc: Keerthy <j-keerthy@ti.com> > Signed-off-by: Udit Kumar <u-kumar1@ti.com> > --- > Change log: > Changes in v2: > - Kept clk-sci.c as is because this is used by > AM64 and AM65 platforms > - v1 link > https://lore.kernel.org/all/20230919060649.2518147-1-u-kumar1@ti.com/ > > drivers/clk/ti/clk-k3.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/clk/ti/clk-k3.c b/drivers/clk/ti/clk-k3.c > index ba925fa3c4..8016c3784a 100644 > --- a/drivers/clk/ti/clk-k3.c > +++ b/drivers/clk/ti/clk-k3.c > @@ -11,6 +11,7 @@ > #include <errno.h> > #include <soc.h> > #include <clk-uclass.h> > +#include <k3-avs.h> > #include "k3-clk.h" > > #define PLL_MIN_FREQ 800000000 > @@ -339,6 +340,10 @@ static ulong ti_clk_set_rate(struct clk *clk, ulong rate) > } > } > > + if (IS_ENABLED(CONFIG_K3_AVS0)) > + k3_avs_notify_freq(data->map[clk->id].dev_id, > + data->map[clk->id].clk_id, rate); > + Don't you want to do this prior to actual clock change? F1, V1 -> F2, V2 Seq 1: F1, V1 F2, V1 F2, V2 Seq 2: F1, V1 F1, V2 F2, V2 If V2 < V1 (implying F2 < F1), then Seq 1 is valid. But if V2 > V1 (F2 > F1) Seq 2 is valid. We seem to ignore this constraint. Can you explain this in the commit message? clk-sci.c seems to use Seq 2, vs this patch seems to take Seq 1. > return new_rate; > } > > -- > 2.34.1 >
Hi Nishanth, On 9/19/2023 9:07 PM, Nishanth Menon wrote: > On 19:34-20230919, Udit Kumar wrote: >> AVS is enabled at R5 SPL stage, on few platforms like J721E >> and J7200 clk-k3 is used instead if clk-sci driver. >> >> Add support in clk-k3 driver as well to notify AVS driver >> upon setting clock rate so that voltage is changed accordingly. >> >> Cc: Keerthy <j-keerthy@ti.com> >> Signed-off-by: Udit Kumar <u-kumar1@ti.com> >> --- >> Change log: >> Changes in v2: >> - Kept clk-sci.c as is because this is used by >> AM64 and AM65 platforms >> - v1 link >> https://lore.kernel.org/all/20230919060649.2518147-1-u-kumar1@ti.com/ >> >> drivers/clk/ti/clk-k3.c | 5 +++++ >> 1 file changed, 5 insertions(+) >> >> diff --git a/drivers/clk/ti/clk-k3.c b/drivers/clk/ti/clk-k3.c >> index ba925fa3c4..8016c3784a 100644 >> --- a/drivers/clk/ti/clk-k3.c >> +++ b/drivers/clk/ti/clk-k3.c >> @@ -11,6 +11,7 @@ >> #include <errno.h> >> #include <soc.h> >> #include <clk-uclass.h> >> +#include <k3-avs.h> >> #include "k3-clk.h" >> >> #define PLL_MIN_FREQ 800000000 >> @@ -339,6 +340,10 @@ static ulong ti_clk_set_rate(struct clk *clk, ulong rate) >> } >> } >> >> + if (IS_ENABLED(CONFIG_K3_AVS0)) >> + k3_avs_notify_freq(data->map[clk->id].dev_id, >> + data->map[clk->id].clk_id, rate); >> + > Don't you want to do this prior to actual clock change? Not really, As i see new freq to be less than current one. > F1, V1 -> F2, V2 > > Seq 1: > F1, V1 > F2, V1 > F2, V2 > > Seq 2: > F1, V1 > F1, V2 > F2, V2 > > If V2 < V1 (implying F2 < F1), then Seq 1 is valid. But if V2 > V1 (F2 > > F1) Seq 2 is valid. > > We seem to ignore this constraint. Can you explain this in the commit > message? Sure, will do , Below is reasoning, Please see if make sense On J7 devices, In default device tree, A72 freq is set to 2Ghz, few SOCs can operate at lower freq. So while setting OPP freq (which is less than default one), First freq needs to set before changing voltage. On other hand, I am thinking to fix properly for both cases, irrespective of seq 1 is valid for current use case. > > clk-sci.c seems to use Seq 2, vs this patch seems to take Seq 1. I think, something to be fixed for clk-sci.c as well. As I see three freq point https://elixir.bootlin.com/u-boot/latest/source/drivers/misc/k3_avs.c#L339 So we can choose either way new-freq > current-freq or new-freq < current-freq, Based upon new-freq , voltage to be set before/after clock change. > >> return new_rate; >> } >> >> -- >> 2.34.1 >>
On 22:58-20230919, Kumar, Udit wrote: > Hi Nishanth, > > On 9/19/2023 9:07 PM, Nishanth Menon wrote: > > On 19:34-20230919, Udit Kumar wrote: > > > AVS is enabled at R5 SPL stage, on few platforms like J721E > > > and J7200 clk-k3 is used instead if clk-sci driver. > > > > > > Add support in clk-k3 driver as well to notify AVS driver > > > upon setting clock rate so that voltage is changed accordingly. > > > > > > Cc: Keerthy <j-keerthy@ti.com> > > > Signed-off-by: Udit Kumar <u-kumar1@ti.com> > > > --- > > > Change log: > > > Changes in v2: > > > - Kept clk-sci.c as is because this is used by > > > AM64 and AM65 platforms > > > - v1 link > > > https://lore.kernel.org/all/20230919060649.2518147-1-u-kumar1@ti.com/ > > > > > > drivers/clk/ti/clk-k3.c | 5 +++++ > > > 1 file changed, 5 insertions(+) > > > > > > diff --git a/drivers/clk/ti/clk-k3.c b/drivers/clk/ti/clk-k3.c > > > index ba925fa3c4..8016c3784a 100644 > > > --- a/drivers/clk/ti/clk-k3.c > > > +++ b/drivers/clk/ti/clk-k3.c > > > @@ -11,6 +11,7 @@ > > > #include <errno.h> > > > #include <soc.h> > > > #include <clk-uclass.h> > > > +#include <k3-avs.h> > > > #include "k3-clk.h" > > > #define PLL_MIN_FREQ 800000000 > > > @@ -339,6 +340,10 @@ static ulong ti_clk_set_rate(struct clk *clk, ulong rate) > > > } > > > } > > > + if (IS_ENABLED(CONFIG_K3_AVS0)) > > > + k3_avs_notify_freq(data->map[clk->id].dev_id, > > > + data->map[clk->id].clk_id, rate); > > > + > > Don't you want to do this prior to actual clock change? > > Not really, As i see new freq to be less than current one. > > > F1, V1 -> F2, V2 > > > > Seq 1: > > F1, V1 > > F2, V1 > > F2, V2 > > > > Seq 2: > > F1, V1 > > F1, V2 > > F2, V2 > > > > If V2 < V1 (implying F2 < F1), then Seq 1 is valid. But if V2 > V1 (F2 > > > F1) Seq 2 is valid. > > > > We seem to ignore this constraint. Can you explain this in the commit > > message? > > Sure, will do , Below is reasoning, Please see if make sense > > On J7 devices, In default device tree, A72 freq is set to 2Ghz, few SOCs > can operate at lower freq. > > So while setting OPP freq (which is less than default one), First freq needs > to set > > before changing voltage. > > > On other hand, I am thinking to fix properly for both cases, irrespective of > seq 1 is > > valid for current use case. Thanks. > > > > clk-sci.c seems to use Seq 2, vs this patch seems to take Seq 1. > > I think, something to be fixed for clk-sci.c as well. > As I see three freq point > https://elixir.bootlin.com/u-boot/latest/source/drivers/misc/k3_avs.c#L339 > > So we can choose either way new-freq > current-freq or new-freq < > current-freq, > > Based upon new-freq , voltage to be set before/after clock change. We should handle both cases, ideally.
diff --git a/drivers/clk/ti/clk-k3.c b/drivers/clk/ti/clk-k3.c index ba925fa3c4..8016c3784a 100644 --- a/drivers/clk/ti/clk-k3.c +++ b/drivers/clk/ti/clk-k3.c @@ -11,6 +11,7 @@ #include <errno.h> #include <soc.h> #include <clk-uclass.h> +#include <k3-avs.h> #include "k3-clk.h" #define PLL_MIN_FREQ 800000000 @@ -339,6 +340,10 @@ static ulong ti_clk_set_rate(struct clk *clk, ulong rate) } } + if (IS_ENABLED(CONFIG_K3_AVS0)) + k3_avs_notify_freq(data->map[clk->id].dev_id, + data->map[clk->id].clk_id, rate); + return new_rate; }
AVS is enabled at R5 SPL stage, on few platforms like J721E and J7200 clk-k3 is used instead if clk-sci driver. Add support in clk-k3 driver as well to notify AVS driver upon setting clock rate so that voltage is changed accordingly. Cc: Keerthy <j-keerthy@ti.com> Signed-off-by: Udit Kumar <u-kumar1@ti.com> --- Change log: Changes in v2: - Kept clk-sci.c as is because this is used by AM64 and AM65 platforms - v1 link https://lore.kernel.org/all/20230919060649.2518147-1-u-kumar1@ti.com/ drivers/clk/ti/clk-k3.c | 5 +++++ 1 file changed, 5 insertions(+)