| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Mon, 10 Feb 2020 14:38:37 -0800
From: Stephen Boyd <sboyd@...nel.org>
To: abel.vesa@....com, aisheng.dong@....com, leonard.crestez@....com,
peng.fan@....com, s.hauer@...gutronix.de, shawnguo@...nel.org
Cc: kernel@...gutronix.de, festevam@...il.com, linux-imx@....com,
ping.bai@....com, Anson.Huang@....com,
linux-arm-kernel@...ts.infradead.org, linux-kernel@...r.kernel.org,
linux-clk@...r.kernel.org, Peng Fan <peng.fan@....com>
Subject: Re: [PATCH 4/7] clk: imx: add imx_hw_clk_cpuv2 for i.MX7ULP
Quoting peng.fan@....com (2020-02-04 05:34:34)
> From: Peng Fan <peng.fan@....com>
>
> 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@....com>
> ---
> 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@....com>
> + */
> +
> +#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.
Powered by blists - more mailing lists