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

[Stephen Boyd]

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.

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. 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, 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!

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.