| 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
| ||
|
Message-ID: <CAPDyKFrn-CYoWV3f4g2HOTEEg6_mnx6wfoXsMxBd1_NwNaD4LA@mail.gmail.com>
Date: Thu, 12 Dec 2024 13:25:05 +0100
From: Ulf Hansson <ulf.hansson@...aro.org>
To: Christian Marangi <ansuelsmth@...il.com>, Viresh Kumar <viresh.kumar@...aro.org>
Cc: "Rafael J. Wysocki" <rafael@...nel.org>, Rob Herring <robh@...nel.org>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>, linux-pm@...r.kernel.org,
devicetree@...r.kernel.org, linux-kernel@...r.kernel.org, upstream@...oha.com
Subject: Re: [PATCH v7 2/2] cpufreq: airoha: Add EN7581 CPUFreq SMCCC driver
On Thu, 12 Dec 2024 at 13:01, Ulf Hansson <ulf.hansson@...aro.org> wrote:
>
> On Fri, 6 Dec 2024 at 22:16, Christian Marangi <ansuelsmth@...il.com> wrote:
> >
> > Add simple CPU Freq driver for Airoha EN7581 SoC that control CPU
> > frequency scaling with SMC APIs and register a generic "cpufreq-dt"
> > device.
> >
> > CPUFreq driver registers a get-only clock to get the current global CPU
> > frequency from SMC and a Power Domain to configure the performance state
> > for each OPP to apply the requested frequency from cpufreq-dt. This is
> > needed as SMC use index instead of raw frequency.
> >
> > All CPU share the same frequency and can't be controlled independently.
> > Current shared CPU frequency is returned by the related SMC command.
> >
> > Add SoC compatible to cpufreq-dt-plat block list as a dedicated cpufreq
> > driver is needed with OPP v2 nodes declared in DTS.
> >
> > Signed-off-by: Christian Marangi <ansuelsmth@...il.com>
> > ---
> > Changes v7:
> > - No changes
> > Changes v6:
> > - Improve Kconfig depends logic
> > - Select PM (PM_GENERIC_DOMAINS depends on it)
> > - Drop (int) cast for
> > Changes v5:
> > - Rename cpu_pd to perf for power domain name
> > - Use remove instead of remove_new
> > Changes v4:
> > - Rework to clk-only + PM set_performance_state implementation
> > Changes v3:
> > - Adapt to new cpufreq-dt APIs
> > - Register cpufreq-dt instead of custom freq driver
> > Changes v2:
> > - Fix kernel bot error with missing slab.h and bitfield.h header
> > - Limit COMPILE_TEST to ARM64 due to smcc 1.2
> >
> > drivers/cpufreq/Kconfig.arm | 10 ++
> > drivers/cpufreq/Makefile | 1 +
> > drivers/cpufreq/airoha-cpufreq.c | 222 +++++++++++++++++++++++++++
>
> Hmm, it looks like this needs to be moved and possibly split up.
>
> The provider part (for the clock and power-domain) belongs in
> /drivers/pmdomain/*, along with the other power-domain providers.
>
> Other than that, I was really expecting the cpufreq-dt to take care of the rest.
>
> > drivers/cpufreq/cpufreq-dt-platdev.c | 2 +
> > 4 files changed, 235 insertions(+)
> > create mode 100644 drivers/cpufreq/airoha-cpufreq.c
> >
> > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> > index 5f7e13e60c80..8494faac58ae 100644
> > --- a/drivers/cpufreq/Kconfig.arm
> > +++ b/drivers/cpufreq/Kconfig.arm
> > @@ -15,6 +15,16 @@ config ARM_ALLWINNER_SUN50I_CPUFREQ_NVMEM
> > To compile this driver as a module, choose M here: the
> > module will be called sun50i-cpufreq-nvmem.
> >
> > +config ARM_AIROHA_SOC_CPUFREQ
> > + tristate "Airoha EN7581 SoC CPUFreq support"
> > + depends on ARM64 && (ARCH_AIROHA || COMPILE_TEST)
> > + select PM
> > + select PM_OPP
> > + select PM_GENERIC_DOMAINS
> > + default ARCH_AIROHA
> > + help
> > + This adds the CPUFreq driver for Airoha EN7581 SoCs.
> > +
> > config ARM_APPLE_SOC_CPUFREQ
> > tristate "Apple Silicon SoC CPUFreq support"
> > depends on ARCH_APPLE || (COMPILE_TEST && 64BIT)
> > diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> > index d35a28dd9463..890fff99f37d 100644
> > --- a/drivers/cpufreq/Makefile
> > +++ b/drivers/cpufreq/Makefile
> > @@ -53,6 +53,7 @@ obj-$(CONFIG_X86_AMD_FREQ_SENSITIVITY) += amd_freq_sensitivity.o
> >
> > ##################################################################################
> > # ARM SoC drivers
> > +obj-$(CONFIG_ARM_AIROHA_SOC_CPUFREQ) += airoha-cpufreq.o
> > obj-$(CONFIG_ARM_APPLE_SOC_CPUFREQ) += apple-soc-cpufreq.o
> > obj-$(CONFIG_ARM_ARMADA_37XX_CPUFREQ) += armada-37xx-cpufreq.o
> > obj-$(CONFIG_ARM_ARMADA_8K_CPUFREQ) += armada-8k-cpufreq.o
> > diff --git a/drivers/cpufreq/airoha-cpufreq.c b/drivers/cpufreq/airoha-cpufreq.c
> > new file mode 100644
> > index 000000000000..29738f61f401
> > --- /dev/null
> > +++ b/drivers/cpufreq/airoha-cpufreq.c
> > @@ -0,0 +1,222 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +
> > +#include <linux/arm-smccc.h>
> > +#include <linux/bitfield.h>
> > +#include <linux/cpufreq.h>
> > +#include <linux/clk-provider.h>
> > +#include <linux/module.h>
> > +#include <linux/platform_device.h>
> > +#include <linux/pm_domain.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/slab.h>
> > +
> > +#include "cpufreq-dt.h"
> > +
> > +#define AIROHA_SIP_AVS_HANDLE 0x82000301
> > +#define AIROHA_AVS_OP_BASE 0xddddddd0
> > +#define AIROHA_AVS_OP_MASK GENMASK(1, 0)
> > +#define AIROHA_AVS_OP_FREQ_DYN_ADJ (AIROHA_AVS_OP_BASE | \
> > + FIELD_PREP(AIROHA_AVS_OP_MASK, 0x1))
> > +#define AIROHA_AVS_OP_GET_FREQ (AIROHA_AVS_OP_BASE | \
> > + FIELD_PREP(AIROHA_AVS_OP_MASK, 0x2))
> > +
> > +struct airoha_cpufreq_priv {
> > + struct clk_hw hw;
> > + struct generic_pm_domain pd;
> > +
> > + int opp_token;
> > + struct dev_pm_domain_list *pd_list;
> > + struct platform_device *cpufreq_dt;
> > +};
> > +
> > +static long airoha_cpufreq_clk_round(struct clk_hw *hw, unsigned long rate,
> > + unsigned long *parent_rate)
> > +{
> > + return rate;
> > +}
> > +
> > +static unsigned long airoha_cpufreq_clk_get(struct clk_hw *hw,
> > + unsigned long parent_rate)
> > +{
> > + const struct arm_smccc_1_2_regs args = {
> > + .a0 = AIROHA_SIP_AVS_HANDLE,
> > + .a1 = AIROHA_AVS_OP_GET_FREQ,
> > + };
> > + struct arm_smccc_1_2_regs res;
> > +
> > + arm_smccc_1_2_smc(&args, &res);
> > +
> > + /* SMCCC returns freq in MHz */
> > + return res.a0 * 1000 * 1000;
> > +}
> > +
> > +/* Airoha CPU clk SMCC is always enabled */
> > +static int airoha_cpufreq_clk_is_enabled(struct clk_hw *hw)
> > +{
> > + return true;
> > +}
> > +
> > +static const struct clk_ops airoha_cpufreq_clk_ops = {
> > + .recalc_rate = airoha_cpufreq_clk_get,
> > + .is_enabled = airoha_cpufreq_clk_is_enabled,
> > + .round_rate = airoha_cpufreq_clk_round,
> > +};
> > +
> > +static const char * const airoha_cpufreq_clk_names[] = { "cpu", NULL };
> > +
> > +/* NOP function to disable OPP from setting clock */
> > +static int airoha_cpufreq_config_clks_nop(struct device *dev,
> > + struct opp_table *opp_table,
> > + struct dev_pm_opp *opp,
> > + void *data, bool scaling_down)
> > +{
> > + return 0;
> > +}
> > +
> > +static const char * const airoha_cpufreq_pd_names[] = { "perf" };
> > +
> > +static int airoha_cpufreq_set_performance_state(struct generic_pm_domain *domain,
> > + unsigned int state)
> > +{
> > + const struct arm_smccc_1_2_regs args = {
> > + .a0 = AIROHA_SIP_AVS_HANDLE,
> > + .a1 = AIROHA_AVS_OP_FREQ_DYN_ADJ,
> > + .a3 = state,
> > + };
> > + struct arm_smccc_1_2_regs res;
> > +
> > + arm_smccc_1_2_smc(&args, &res);
> > +
> > + /* SMC signal correct apply by unsetting BIT 0 */
> > + return res.a0 & BIT(0) ? -EINVAL : 0;
> > +}
> > +
> > +static int airoha_cpufreq_probe(struct platform_device *pdev)
> > +{
> > + const struct dev_pm_domain_attach_data attach_data = {
> > + .pd_names = airoha_cpufreq_pd_names,
> > + .num_pd_names = ARRAY_SIZE(airoha_cpufreq_pd_names),
> > + .pd_flags = PD_FLAG_DEV_LINK_ON | PD_FLAG_REQUIRED_OPP,
> > + };
> > + struct dev_pm_opp_config config = {
> > + .clk_names = airoha_cpufreq_clk_names,
> > + .config_clks = airoha_cpufreq_config_clks_nop,
> > + };
> > + struct platform_device *cpufreq_dt;
> > + struct airoha_cpufreq_priv *priv;
> > + struct device *dev = &pdev->dev;
> > + const struct clk_init_data init = {
> > + .name = "cpu",
> > + .ops = &airoha_cpufreq_clk_ops,
> > + /* Clock with no set_rate, can't cache */
> > + .flags = CLK_GET_RATE_NOCACHE,
> > + };
> > + struct generic_pm_domain *pd;
> > + struct device *cpu_dev;
> > + int ret;
> > +
> > + /* CPUs refer to the same OPP table */
> > + cpu_dev = get_cpu_device(0);
> > + if (!cpu_dev)
> > + return -ENODEV;
> > +
> > + priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> > + if (!priv)
> > + return -ENOMEM;
> > +
> > + /* Init and register a get-only clk for Cpufreq */
> > + priv->hw.init = &init;
> > + ret = devm_clk_hw_register(dev, &priv->hw);
> > + if (ret)
> > + return ret;
> > +
> > + ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get,
> > + &priv->hw);
> > + if (ret)
> > + return ret;
> > +
> > + /* Init and register a PD for Cpufreq */
> > + pd = &priv->pd;
> > + pd->name = "cpu_pd";
> > + pd->flags = GENPD_FLAG_ALWAYS_ON;
> > + pd->set_performance_state = airoha_cpufreq_set_performance_state;
> > +
> > + ret = pm_genpd_init(pd, NULL, false);
> > + if (ret)
> > + return ret;
> > +
> > + ret = of_genpd_add_provider_simple(dev->of_node, pd);
> > + if (ret)
> > + goto err_add_provider;
> > +
>
> To me, the above code belongs in a power-domain provider driver. While
> the below should be taken care of in cpufreq-dt, except for the device
> registration of the cpufreq-dt device, I guess.
>
> Viresh, what's your view on this?
>
> > + /* Set OPP table conf with NOP config_clks */
> > + priv->opp_token = dev_pm_opp_set_config(cpu_dev, &config);
> > + if (priv->opp_token < 0) {
> > + ret = priv->opp_token;
> > + dev_err(dev, "Failed to set OPP config\n");
> > + goto err_set_config;
> > + }
> > +
> > + /* Attach PM for OPP */
> > + ret = dev_pm_domain_attach_list(cpu_dev, &attach_data,
> > + &priv->pd_list);
> > + if (ret)
> > + goto err_attach_pm;
> > +
> > + cpufreq_dt = platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
> > + ret = PTR_ERR_OR_ZERO(cpufreq_dt);
> > + if (ret) {
> > + dev_err(dev, "failed to create cpufreq-dt device: %d\n", ret);
> > + goto err_register_cpufreq;
> > + }
> > +
> > + priv->cpufreq_dt = cpufreq_dt;
> > + platform_set_drvdata(pdev, priv);
> > +
> > + return 0;
> > +
> > +err_register_cpufreq:
> > + dev_pm_domain_detach_list(priv->pd_list);
> > +err_attach_pm:
> > + dev_pm_opp_clear_config(priv->opp_token);
> > +err_set_config:
> > + of_genpd_del_provider(dev->of_node);
> > +err_add_provider:
> > + pm_genpd_remove(pd);
> > +
> > + return ret;
> > +}
> > +
> > +static void airoha_cpufreq_remove(struct platform_device *pdev)
> > +{
> > + struct airoha_cpufreq_priv *priv = platform_get_drvdata(pdev);
> > +
> > + platform_device_unregister(priv->cpufreq_dt);
> > +
> > + dev_pm_domain_detach_list(priv->pd_list);
> > +
> > + dev_pm_opp_clear_config(priv->opp_token);
> > +
> > + of_genpd_del_provider(pdev->dev.of_node);
> > + pm_genpd_remove(&priv->pd);
> > +}
> > +
> > +static const struct of_device_id airoha_cpufreq_of_match[] = {
> > + { .compatible = "airoha,en7581-cpufreq" },
> > + { },
> > +};
> > +MODULE_DEVICE_TABLE(of, airoha_cpufreq_of_match);
> > +
> > +static struct platform_driver airoha_cpufreq_driver = {
> > + .probe = airoha_cpufreq_probe,
> > + .remove = airoha_cpufreq_remove,
> > + .driver = {
> > + .name = "airoha-cpufreq",
> > + .of_match_table = airoha_cpufreq_of_match,
> > + },
> > +};
> > +module_platform_driver(airoha_cpufreq_driver);
> > +
> > +MODULE_AUTHOR("Christian Marangi <ansuelsmth@...il.com>");
> > +MODULE_DESCRIPTION("CPUfreq driver for Airoha SoCs");
> > +MODULE_LICENSE("GPL");
> > diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
> > index 9c198bd4f7e9..2aa00769cf09 100644
> > --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> > +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> > @@ -103,6 +103,8 @@ static const struct of_device_id allowlist[] __initconst = {
> > * platforms using "operating-points-v2" property.
> > */
> > static const struct of_device_id blocklist[] __initconst = {
> > + { .compatible = "airoha,en7581", },
> > +
I had a closer look at cpufreq-dt. Not sure if it makes sense, but to
me it seems like we should extend it to support attaching cpu-devices
to their performance domains (power-domains). In this way, we can
avoid adding the airoha compatible above to the blocklist.
Christian, do you want to take a stab at this - or just let me know
and I can try to draft a patch for cpufreq-dt with this?
> > { .compatible = "allwinner,sun50i-a100" },
> > { .compatible = "allwinner,sun50i-h6", },
> > { .compatible = "allwinner,sun50i-h616", },
> > --
> > 2.45.2
> >
Kind regards
Uffe
Powered by blists - more mailing lists