RE: [PATCH 4/7] clk: imx: add imx_hw_clk_cpuv2 for i.MX7ULP

[Peng Fan]

> Subject: Re: [PATCH 4/7] clk: imx: add imx_hw_clk_cpuv2 for i.MX7ULP

+ Viresh

Hi Stephen,

> 
> Quoting peng.fan@xxxxxxx (2020-02-04 05:34:34)
> > From: Peng Fan <peng.fan@xxxxxxx>
> >
> > Add a clk api for i.MX7ULP ARM core clk usage.
> > imx_hw_clk_cpu could not be reused, because i.MX7ULP ARM core clk has
> > totally different design. To simplify ARM core clk change logic, add a
> > new clk api.
> >
> > A draft picture to show the ARM core clock.
> >                                                       |-sirc
> >      |->   run(500MHz)    ->  div -> mux -------------|-firc
> >   ARM|                                                |
> >      |->   hsrun(720MHz)  ->  hs div -> hs mux -------|-spll pfd
> >                                                       |-....
> >
> > Need to configure PMC when ARM core runs in HSRUN or RUN mode.
> >
> > RUN and HSRUN related registers are not same, but their mux has same
> > clocks as input.
> >
> > The API takes arm core, div, hs div, mux, hs mux, mux parent, pfd,
> > step as params for switch clk freq.
> >
> > When set rate, need to switch mux to take firc as input, then set spll
> > pfd freq, then switch back mux to spll pfd as parent.
> >
> > Per i.MX7ULP requirement, when clk runs in HSRUN mode, it could only
> > support arm core wfi idle, so add pm qos to support it.
> >
> > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> > ---
> >  drivers/clk/imx/Makefile    |   1 +
> >  drivers/clk/imx/clk-cpuv2.c | 137
> ++++++++++++++++++++++++++++++++++++++++++++
> >  drivers/clk/imx/clk.h       |   9 +++
> >  3 files changed, 147 insertions(+)
> >  create mode 100644 drivers/clk/imx/clk-cpuv2.c
> >
> > diff --git a/drivers/clk/imx/Makefile b/drivers/clk/imx/Makefile index
> > 928f874c73d2..9707fef8da98 100644
> > --- a/drivers/clk/imx/Makefile
> > +++ b/drivers/clk/imx/Makefile
> > @@ -5,6 +5,7 @@ obj-$(CONFIG_MXC_CLK) += \
> >         clk-busy.o \
> >         clk-composite-8m.o \
> >         clk-cpu.o \
> > +       clk-cpuv2.o \
> >         clk-composite-7ulp.o \
> >         clk-divider-gate.o \
> >         clk-fixup-div.o \
> > diff --git a/drivers/clk/imx/clk-cpuv2.c b/drivers/clk/imx/clk-cpuv2.c
> > new file mode 100644 index 000000000000..a73d97a782aa
> > --- /dev/null
> > +++ b/drivers/clk/imx/clk-cpuv2.c
> > @@ -0,0 +1,137 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/*
> > + * Copyright 2020 NXP
> > + *
> > + * Peng Fan <peng.fan@xxxxxxx>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/slab.h>
> > +#include <linux/pm_qos.h>
> > +#include "clk.h"
> > +
> > +static struct pm_qos_request pm_qos_hsrun;
> > +
> > +#define MAX_NORMAL_RUN_FREQ    528000000
> > +
> > +struct clk_cpu {
> > +       struct clk_hw   hw;
> > +       struct clk_hw   *core;
> > +       struct clk_hw   *div_nor;
> > +       struct clk_hw   *div_hs;
> > +       struct clk_hw   *mux_nor;
> > +       struct clk_hw   *mux_hs;
> > +       struct clk_hw   *mux_parent;
> > +       struct clk_hw   *pfd;
> > +       struct clk_hw   *step;
> > +};
> > +
> > +static inline struct clk_cpu *to_clk_cpu(struct clk_hw *hw) {
> > +       return container_of(hw, struct clk_cpu, hw); }
> > +
> > +static unsigned long clk_cpu_recalc_rate(struct clk_hw *hw,
> > +                                        unsigned long
> parent_rate) {
> > +       struct clk_cpu *cpu = to_clk_cpu(hw);
> > +
> > +       return clk_hw_get_rate(cpu->core); }
> > +
> > +static long clk_cpu_round_rate(struct clk_hw *hw, unsigned long rate,
> > +                              unsigned long *prate) {
> > +       return rate;
> > +}
> > +
> > +static int clk_cpu_set_rate(struct clk_hw *hw, unsigned long rate,
> > +                           unsigned long parent_rate) {
> > +       struct clk_cpu *cpu = to_clk_cpu(hw);
> > +       int ret;
> > +       struct clk_hw *div, *mux_now;
> > +       unsigned long old_rate = clk_hw_get_rate(cpu->core);
> > +
> > +       div = clk_hw_get_parent(cpu->core);
> > +
> > +       if (div == cpu->div_nor)
> > +               mux_now = cpu->mux_nor;
> > +       else
> > +               mux_now = cpu->mux_hs;
> > +
> > +       ret = clk_hw_set_parent(mux_now, cpu->step);
> > +       if (ret)
> > +               return ret;
> > +
> > +       ret = clk_set_rate(cpu->pfd->clk, rate);
> > +       if (ret) {
> > +               clk_hw_set_parent(mux_now, cpu->mux_parent);
> > +               return ret;
> > +       }
> > +
> > +       if (rate > MAX_NORMAL_RUN_FREQ) {
> > +               pm_qos_add_request(&pm_qos_hsrun,
> PM_QOS_CPU_DMA_LATENCY, 0);
> > +               clk_hw_set_parent(cpu->mux_hs, cpu->mux_parent);
> > +               clk_hw_set_parent(cpu->core, cpu->div_hs);
> > +       } else {
> > +               clk_hw_set_parent(cpu->mux_nor, cpu->mux_parent);
> > +               clk_hw_set_parent(cpu->core, cpu->div_nor);
> > +               if (old_rate > MAX_NORMAL_RUN_FREQ)
> > +                       pm_qos_remove_request(&pm_qos_hsrun);
> > +       }
> > +
> 
> This is a cpufreq driver. Please write a cpufreq driver instead of trying to make
> "clk_set_rate()" conform to the requirements that cpufreq-dt mandates,
> which is that one clk exists and that clk rate change changes the frequency of
> the CPU.

There was an old thread, https://lkml.org/lkml/2017/9/20/931

Aisheng proposed a opp->set_clk implemented, but rejected by Viresh.
Viresh suggested resolve this issue in clk driver to let clk driver handle
the cpu clk freq change.

> 
> If cpufreq-dt can work with the clk framework is up to the implementation of
> the hardware and the software. From what I see here, the clk framework is
> being subverted through the use of
> clk_hw_set_parent() and clk_set_rate() calls from within the framework to
> make the cpufreq-dt driver happy. Don't do that.

ok

 Either write a proper clk
> driver for the clks that are there and have that manage the clk tree when
> clk_set_rate() is called, 

Do you have any suggestions if put the cpu clk freq change in clk driver?
i.MX7ULP has some special requirements, it has RUN and HSRUN registers
which match 500MHz and 720MHz, to run at 500MHz, need configure RUN
related registers; to run at 720MHz, need configure HSRUN related registers.
So the cpu clk is mux marked with CLK_IS_CRITICAL. However in its parent
tree, pll and pfd needs CLK_SET_RATE_GATE. So after pll/pfd prepared,
its freq could not be changed, because of protected.


or write a cpufreq driver that controls various clks and
> adjusts their frequencies.
> 
> I assume there is a mux or something that eventually clks the CPU, so that can
> certainly be modeled as a clk with the parents set some way.
> That will make clk_set_rate() mostly work as long as you can hardcode a
> min/max value to change the parents, etc. Should work!

As described above, the pll/pfd flag has CLK_SET_RATE_GATE, the mux
has flag CLK_IS_CRITICAL. So the pll/pfd freq could not be changed because
clk framework marked pll/pfd protected.

Anyway I'll try to find more to see any new solutions.

> 
> The use of pm_qos_add_request() is also pretty horrifying. We're deep in the
> clk framework implementation here and we're calling out to pm_qos.
> That shouldn't need to happen. If anything, we should do that from the core
> framework, but I don't know why we would. It's probably some sort of
> cpufreq thing.

When run at 720MHz, cpu are only allowed cpu core wfi. So I add pm qos
to block low power idle.

Thanks,
Peng.