| 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 Jun 2024 16:08:24 +0300
From: Dmitry Rokosov <ddrokosov@...utedevices.com>
To: Jerome Brunet <jbrunet@...libre.com>
CC: <neil.armstrong@...aro.org>, <mturquette@...libre.com>,
<sboyd@...nel.org>, <robh+dt@...nel.org>,
<krzysztof.kozlowski+dt@...aro.org>, <khilman@...libre.com>,
<martin.blumenstingl@...glemail.com>, <jian.hu@...ogic.com>,
<kernel@...rdevices.ru>, <rockosov@...il.com>,
<linux-amlogic@...ts.infradead.org>, <linux-clk@...r.kernel.org>,
<devicetree@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v3 7/7] clk: meson: a1: add Amlogic A1 CPU clock
controller driver
On Mon, Jun 10, 2024 at 12:06:31PM +0200, Jerome Brunet wrote:
> On Wed 15 May 2024 at 21:47, Dmitry Rokosov <ddrokosov@...utedevices.com> wrote:
>
> > The CPU clock controller plays a general role in the Amlogic A1 SoC
> > family by generating CPU clocks. As an APB slave module, it offers the
> > capability to inherit the CPU clock from two sources: the internal fixed
> > clock known as 'cpu fixed clock' and the external input provided by the
> > A1 PLL clock controller, referred to as 'syspll'.
> >
> > It is important for the driver to handle cpu_clk rate switching
> > effectively by transitioning to the CPU fixed clock to avoid any
> > potential execution freezes.
> >
> > Signed-off-by: Dmitry Rokosov <ddrokosov@...utedevices.com>
> > ---
> > drivers/clk/meson/Kconfig | 10 ++
> > drivers/clk/meson/Makefile | 1 +
> > drivers/clk/meson/a1-cpu.c | 331 +++++++++++++++++++++++++++++++++++++
> > 3 files changed, 342 insertions(+)
> > create mode 100644 drivers/clk/meson/a1-cpu.c
> >
> > diff --git a/drivers/clk/meson/Kconfig b/drivers/clk/meson/Kconfig
> > index 80c4a18c83d2..148d4495eee3 100644
> > --- a/drivers/clk/meson/Kconfig
> > +++ b/drivers/clk/meson/Kconfig
> > @@ -111,6 +111,16 @@ config COMMON_CLK_AXG_AUDIO
> > Support for the audio clock controller on AmLogic A113D devices,
> > aka axg, Say Y if you want audio subsystem to work.
> >
> > +config COMMON_CLK_A1_CPU
> > + tristate "Amlogic A1 SoC CPU controller support"
> > + depends on ARM64
> > + select COMMON_CLK_MESON_REGMAP
> > + select COMMON_CLK_MESON_CLKC_UTILS
> > + help
> > + Support for the CPU clock controller on Amlogic A113L based
> > + device, A1 SoC Family. Say Y if you want A1 CPU clock controller
> > + to work.
> > +
> > config COMMON_CLK_A1_PLL
> > tristate "Amlogic A1 SoC PLL controller support"
> > depends on ARM64
> > diff --git a/drivers/clk/meson/Makefile b/drivers/clk/meson/Makefile
> > index 4968fc7ad555..2a06eb0303d6 100644
> > --- a/drivers/clk/meson/Makefile
> > +++ b/drivers/clk/meson/Makefile
> > @@ -18,6 +18,7 @@ obj-$(CONFIG_COMMON_CLK_MESON_AUDIO_RSTC) += meson-audio-rstc.o
> >
> > obj-$(CONFIG_COMMON_CLK_AXG) += axg.o axg-aoclk.o
> > obj-$(CONFIG_COMMON_CLK_AXG_AUDIO) += axg-audio.o
> > +obj-$(CONFIG_COMMON_CLK_A1_CPU) += a1-cpu.o
> > obj-$(CONFIG_COMMON_CLK_A1_PLL) += a1-pll.o
> > obj-$(CONFIG_COMMON_CLK_A1_PERIPHERALS) += a1-peripherals.o
> > obj-$(CONFIG_COMMON_CLK_A1_AUDIO) += a1-audio.o
> > diff --git a/drivers/clk/meson/a1-cpu.c b/drivers/clk/meson/a1-cpu.c
> > new file mode 100644
> > index 000000000000..a9edabeafea9
> > --- /dev/null
> > +++ b/drivers/clk/meson/a1-cpu.c
> > @@ -0,0 +1,331 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + * Amlogic A1 SoC family CPU Clock Controller driver.
> > + *
> > + * Copyright (c) 2024, SaluteDevices. All Rights Reserved.
> > + * Author: Dmitry Rokosov <ddrokosov@...utedevices.com>
> > + */
> > +
> > +#include <linux/clk.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/mod_devicetable.h>
> > +#include <linux/platform_device.h>
> > +#include "clk-regmap.h"
> > +#include "meson-clkc-utils.h"
> > +
> > +#include <dt-bindings/clock/amlogic,a1-cpu-clkc.h>
> > +
> > +/* CPU Clock Controller register offset */
> > +#define CPUCTRL_CLK_CTRL0 0x0
> > +#define CPUCTRL_CLK_CTRL1 0x4
> > +
> > +static u32 cpu_fsource_sel_table[] = { 0, 1, 2 };
> > +static const struct clk_parent_data cpu_fsource_sel_parents[] = {
> > + { .fw_name = "xtal" },
> > + { .fw_name = "fclk_div2" },
> > + { .fw_name = "fclk_div3" },
> > +};
> > +
> > +static struct clk_regmap cpu_fsource_sel0 = {
> > + .data = &(struct clk_regmap_mux_data) {
> > + .offset = CPUCTRL_CLK_CTRL0,
> > + .mask = 0x3,
> > + .shift = 0,
> > + .table = cpu_fsource_sel_table,
> > + },
> > + .hw.init = &(struct clk_init_data) {
> > + .name = "cpu_fsource_sel0",
> > + .ops = &clk_regmap_mux_ops,
> > + .parent_data = cpu_fsource_sel_parents,
> > + .num_parents = ARRAY_SIZE(cpu_fsource_sel_parents),
> > + .flags = CLK_SET_RATE_PARENT,
>
> I don't think setting the rates of controller parents is appropriate
>
> > + },
> > +};
> > +
> > +static struct clk_regmap cpu_fsource_div0 = {
> > + .data = &(struct clk_regmap_div_data) {
> > + .offset = CPUCTRL_CLK_CTRL0,
> > + .shift = 4,
> > + .width = 6,
> > + },
> > + .hw.init = &(struct clk_init_data) {
> > + .name = "cpu_fsource_div0",
> > + .ops = &clk_regmap_divider_ops,
> > + .parent_hws = (const struct clk_hw *[]) {
> > + &cpu_fsource_sel0.hw
> > + },
> > + .num_parents = 1,
> > + .flags = CLK_SET_RATE_PARENT,
> > + },
> > +};
> > +
> > +static struct clk_regmap cpu_fsel0 = {
> > + .data = &(struct clk_regmap_mux_data) {
> > + .offset = CPUCTRL_CLK_CTRL0,
> > + .mask = 0x1,
> > + .shift = 2,
> > + },
> > + .hw.init = &(struct clk_init_data) {
> > + .name = "cpu_fsel0",
> > + .ops = &clk_regmap_mux_ops,
> > + .parent_hws = (const struct clk_hw *[]) {
> > + &cpu_fsource_sel0.hw,
> > + &cpu_fsource_div0.hw,
> > + },
> > + .num_parents = 2,
> > + .flags = CLK_SET_RATE_GATE | CLK_SET_RATE_PARENT,
> > + },
> > +};
> > +
> > +static struct clk_regmap cpu_fsource_sel1 = {
> > + .data = &(struct clk_regmap_mux_data) {
> > + .offset = CPUCTRL_CLK_CTRL0,
> > + .mask = 0x3,
> > + .shift = 16,
> > + .table = cpu_fsource_sel_table,
> > + },
> > + .hw.init = &(struct clk_init_data) {
> > + .name = "cpu_fsource_sel1",
> > + .ops = &clk_regmap_mux_ops,
> > + .parent_data = cpu_fsource_sel_parents,
> > + .num_parents = ARRAY_SIZE(cpu_fsource_sel_parents),
> > + .flags = CLK_SET_RATE_PARENT,
> > + },
> > +};
> > +
> > +static struct clk_regmap cpu_fsource_div1 = {
> > + .data = &(struct clk_regmap_div_data) {
> > + .offset = CPUCTRL_CLK_CTRL0,
> > + .shift = 20,
> > + .width = 6,
> > + },
> > + .hw.init = &(struct clk_init_data) {
> > + .name = "cpu_fsource_div1",
> > + .ops = &clk_regmap_divider_ops,
> > + .parent_hws = (const struct clk_hw *[]) {
> > + &cpu_fsource_sel1.hw
> > + },
> > + .num_parents = 1,
> > + .flags = CLK_SET_RATE_PARENT,
> > + },
> > +};
> > +
> > +static struct clk_regmap cpu_fsel1 = {
> > + .data = &(struct clk_regmap_mux_data) {
> > + .offset = CPUCTRL_CLK_CTRL0,
> > + .mask = 0x1,
> > + .shift = 18,
> > + },
> > + .hw.init = &(struct clk_init_data) {
> > + .name = "cpu_fsel1",
> > + .ops = &clk_regmap_mux_ops,
> > + .parent_hws = (const struct clk_hw *[]) {
> > + &cpu_fsource_sel1.hw,
> > + &cpu_fsource_div1.hw,
> > + },
> > + .num_parents = 2,
> > + .flags = CLK_SET_RATE_GATE | CLK_SET_RATE_PARENT,
> > + },
> > +};
> > +
> > +static struct clk_regmap cpu_fclk = {
> > + .data = &(struct clk_regmap_mux_data) {
> > + .offset = CPUCTRL_CLK_CTRL0,
> > + .mask = 0x1,
> > + .shift = 10,
> > + },
> > + .hw.init = &(struct clk_init_data) {
> > + .name = "cpu_fclk",
> > + .ops = &clk_regmap_mux_ops,
> > + .parent_hws = (const struct clk_hw *[]) {
> > + &cpu_fsel0.hw,
> > + &cpu_fsel1.hw,
> > + },
> > + .num_parents = 2,
> > + .flags = CLK_SET_RATE_PARENT,
> > + },
> > +};
> > +
> > +static struct clk_regmap cpu_clk = {
> > + .data = &(struct clk_regmap_mux_data) {
> > + .offset = CPUCTRL_CLK_CTRL0,
> > + .mask = 0x1,
> > + .shift = 11,
> > + },
> > + .hw.init = &(struct clk_init_data) {
> > + .name = "cpu_clk",
> > + .ops = &clk_regmap_mux_ops,
> > + .parent_data = (const struct clk_parent_data []) {
> > + { .hw = &cpu_fclk.hw },
> > + { .fw_name = "sys_pll", },
> > + },
>
> You've put CLK_SET_RATE_GATE on fixed clock path but not the SYS_PLL
> ... that is odd. IMO there should be a bypass input clock to the sys_pll
> with that flag.
>
Apologies for any confusion caused. To clarify, are you proposing the
idea of creating an additional sys_pll_input clock object with the
CLK_SET_RATE_PARENT property, and then using it as the parent clock for
cpu_clk?
> > + .num_parents = 2,
> > + .flags = CLK_SET_RATE_PARENT | CLK_IS_CRITICAL,
> > + },
> > +};
> > +
> > +/* Array of all clocks registered by this provider */
> > +static struct clk_hw *a1_cpu_hw_clks[] = {
> > + [CLKID_CPU_FSOURCE_SEL0] = &cpu_fsource_sel0.hw,
> > + [CLKID_CPU_FSOURCE_DIV0] = &cpu_fsource_div0.hw,
> > + [CLKID_CPU_FSEL0] = &cpu_fsel0.hw,
> > + [CLKID_CPU_FSOURCE_SEL1] = &cpu_fsource_sel1.hw,
> > + [CLKID_CPU_FSOURCE_DIV1] = &cpu_fsource_div1.hw,
> > + [CLKID_CPU_FSEL1] = &cpu_fsel1.hw,
> > + [CLKID_CPU_FCLK] = &cpu_fclk.hw,
> > + [CLKID_CPU_CLK] = &cpu_clk.hw,
> > +};
> > +
> > +static struct clk_regmap *const a1_cpu_regmaps[] = {
> > + &cpu_fsource_sel0,
> > + &cpu_fsource_div0,
> > + &cpu_fsel0,
> > + &cpu_fsource_sel1,
> > + &cpu_fsource_div1,
> > + &cpu_fsel1,
> > + &cpu_fclk,
> > + &cpu_clk,
> > +};
> > +
> > +static struct regmap_config a1_cpu_regmap_cfg = {
> > + .reg_bits = 32,
> > + .val_bits = 32,
> > + .reg_stride = 4,
> > + .max_register = CPUCTRL_CLK_CTRL1,
> > +};
> > +
> > +static struct meson_clk_hw_data a1_cpu_clks = {
> > + .hws = a1_cpu_hw_clks,
> > + .num = ARRAY_SIZE(a1_cpu_hw_clks),
> > +};
> > +
> > +struct a1_sys_pll_nb_data {
> > + struct notifier_block nb;
> > + struct clk_hw *cpu_clk;
> > + struct clk_hw *cpu_fclk;
> > + struct clk *sys_pll;
> > +};
>
> There are number of things which are wrong with this notifier.
>
> First, and foremost, this is a clock controller driver ... it should not
> handle cpufreq policy. There is subsystem for that
>
> > +
> > +static int meson_a1_sys_pll_notifier_cb(struct notifier_block *nb,
> > + unsigned long event, void *data)
> > +{
> > + struct a1_sys_pll_nb_data *nbd;
> > + int ret = 0;
> > +
> > + nbd = container_of(nb, struct a1_sys_pll_nb_data, nb);
> > +
> > + switch (event) {
> > + case PRE_RATE_CHANGE:
> > + /*
> > + * Clock sys_pll will be changed to feed cpu_clk,
> > + * configure cpu_clk to use cpu_fclk fixed clock.
> > + */
> > + ret = clk_hw_set_parent(nbd->cpu_clk, nbd->cpu_fclk);
>
>
> This jumps to whatever was the last frequency below 768MHz ... that does
> not seems deterministic or safe.
Ah, that's an aspect I hadn't considered. You make a valid point. So,
this implies that the g12a clock driver could potentially encounter the
same issue, correct?
> > +
> > + /* Wait for clock propagation */
> > + if (!ret)
> > + udelay(100);
> > +
> > + break;
> > +
> > + case POST_RATE_CHANGE:
> > + /*
> > + * Clock sys_pll rate has ben calculated,
> > + * switch back cpu_clk to sys_pll
> > + */
> > + ret = clk_set_parent(nbd->cpu_clk->clk, nbd->sys_pll);
>
> So whenever sys_pll changes, even if was not used by the CPU at that
> time, this will change back to the sys_pll. Again, that seems fragile
>
>From what I comprehend, only the GEN clock is capable of using sys_pll
as its parent clock. The GEN clock seems more comparable to a diagnostic
clock, implying that when utilized, it should be done with full
awareness and control over its operations.
> > +
> > + /* Wait for clock propagation */
> > + if (!ret)
> > + udelay(100);
> > + break;
> > +
> > + default:
> > + pr_warn("Unknown event %lu for sys_pll notifier\n", event);
> > + break;
> > + }
> > +
> > + return notifier_from_errno(ret);
> > +}
> > +
> > +static struct a1_sys_pll_nb_data a1_sys_pll_nb_data = {
> > + .nb.notifier_call = meson_a1_sys_pll_notifier_cb,
> > + .cpu_clk = &cpu_clk.hw,
> > + .cpu_fclk = &cpu_fclk.hw,
> > +};
> > +
> > +static int meson_a1_dvfs_setup(struct platform_device *pdev)
> > +{
> > + struct device *dev = &pdev->dev;
> > + struct clk *sys_pll;
> > + int ret;
> > +
> > + /* Setup clock notifier for sys_pll clk */
> > + sys_pll = devm_clk_get(dev, "sys_pll");
> > + if (IS_ERR(sys_pll))
> > + return dev_err_probe(dev, PTR_ERR(sys_pll),
> > + "can't get sys_pll as notifier clock\n");
> > +
> > + a1_sys_pll_nb_data.sys_pll = sys_pll;
> > + ret = devm_clk_notifier_register(dev, sys_pll,
> > + &a1_sys_pll_nb_data.nb);
> > + if (ret)
> > + return dev_err_probe(dev, ret,
> > + "can't register sys_pll notifier\n");
> > +
> > + return ret;
> > +}
>
> I don't think these notifiers are appropriate to handle CPU frequency
> change. Cpufreq has a .target_intermediate() callback that seems more
> appropriate to switch the CPU to a safe clock while relocking a PLL.
>
> You should have a look at it and probably at the imx-cpufreq-dt.c which
> improves on cpufreq-dt.c to handle platform quirks
>
I believed that the same approach was employed with the g12a clock,
which uses a sys_pll <-> cpu fixed clock transition to ensure stable CPU
clocking. Am I overlooking something? Or does the g12a cpu clock
maintain a fixed frequency, thus indicating it is not fragile?
[...]
--
Thank you,
Dmitry
Powered by blists - more mailing lists