Re: [RFC PATCH 7/9] clk: apple: Add clk-apple-cluster driver to manage CPU p-states

[Stephen Boyd <sboyd@kernel.org>]

Quoting Hector Martin (2021-10-11 09:57:05)
> diff --git a/drivers/clk/Kconfig b/drivers/clk/Kconfig
> index c5b3dc97396a..f3c8ad041f91 100644
> --- a/drivers/clk/Kconfig
> +++ b/drivers/clk/Kconfig
> @@ -390,6 +390,15 @@ config COMMON_CLK_K210
>         help
>           Support for the Canaan Kendryte K210 RISC-V SoC clocks.
>  
> +config COMMON_CLK_APPLE_CLUSTER

Please place in alphabetical sort order of config name

> +       bool "Clock driver for Apple SoC CPU clusters"
> +       depends on ARCH_APPLE || COMPILE_TEST
> +       select CPUFREQ_DT
> +       default ARCH_APPLE
> +       help
> +         This driver supports CPU cluster frequency switching on Apple SoC
> +         platforms.
> +
>  source "drivers/clk/actions/Kconfig"
>  source "drivers/clk/analogbits/Kconfig"
>  source "drivers/clk/baikal-t1/Kconfig"
> diff --git a/drivers/clk/Makefile b/drivers/clk/Makefile
> index e42312121e51..6dba8c2052c7 100644
> --- a/drivers/clk/Makefile
> +++ b/drivers/clk/Makefile
> @@ -31,6 +31,7 @@ obj-$(CONFIG_COMMON_CLK_FIXED_MMIO)   += clk-fixed-mmio.o
>  obj-$(CONFIG_COMMON_CLK_FSL_FLEXSPI)   += clk-fsl-flexspi.o
>  obj-$(CONFIG_COMMON_CLK_FSL_SAI)       += clk-fsl-sai.o
>  obj-$(CONFIG_COMMON_CLK_GEMINI)                += clk-gemini.o
> +obj-$(CONFIG_COMMON_CLK_APPLE_CLUSTER) += clk-apple-cluster.o

Please put this in sort order on file name. I don't know why aspeed is
in the wrong place. Sigh.

>  obj-$(CONFIG_COMMON_CLK_ASPEED)                += clk-aspeed.o
>  obj-$(CONFIG_MACH_ASPEED_G6)           += clk-ast2600.o
>  obj-$(CONFIG_ARCH_HIGHBANK)            += clk-highbank.o
> diff --git a/drivers/clk/clk-apple-cluster.c b/drivers/clk/clk-apple-cluster.c
> new file mode 100644
> index 000000000000..9e9be38f13b2
> --- /dev/null
> +++ b/drivers/clk/clk-apple-cluster.c
> @@ -0,0 +1,184 @@
> +// SPDX-License-Identifier: GPL-2.0-only OR MIT
> +/*
> + * Apple SoC CPU cluster performance state driver
> + *
> + * Copyright The Asahi Linux Contributors
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/bitops.h>
> +#include <linux/clk-provider.h>
> +#include <linux/delay.h>
> +#include <linux/err.h>
> +#include <linux/io.h>
> +#include <linux/iopoll.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/pm_domain.h>
> +#include <linux/pm_opp.h>
> +
> +#define APPLE_CLUSTER_PSTATE    0x20
> +#define APPLE_CLUSTER_PSTATE_BUSY      BIT(31)
> +#define APPLE_CLUSTER_PSTATE_SET       BIT(25)
> +#define APPLE_CLUSTER_PSTATE_DESIRED2  GENMASK(15, 12)
> +#define APPLE_CLUSTER_PSTATE_DESIRED1  GENMASK(3, 0)
> +
> +struct apple_cluster_clk {
> +       struct clk_hw hw;
> +       struct device *dev;
> +       void __iomem *reg_base;
> +       bool has_pd;
> +};
> +
> +#define to_apple_cluster_clk(_hw) container_of(_hw, struct apple_cluster_clk, hw)
> +
> +#define APPLE_CLUSTER_SWITCH_TIMEOUT 100
> +
> +static int apple_cluster_clk_set_rate(struct clk_hw *hw, unsigned long rate,
> +                                        unsigned long parent_rate)
> +{
> +       struct apple_cluster_clk *cluster = to_apple_cluster_clk(hw);
> +       struct dev_pm_opp *opp;
> +       unsigned int level;
> +       u64 reg;
> +
> +       opp = dev_pm_opp_find_freq_floor(cluster->dev, &rate);
> +
> +       if (IS_ERR(opp))
> +               return PTR_ERR(opp);
> +
> +       level = dev_pm_opp_get_level(opp);
> +
> +       dev_dbg(cluster->dev, "set_rate: %ld -> %d\n", rate, level);
> +
> +       if (readq_poll_timeout(cluster->reg_base + APPLE_CLUSTER_PSTATE, reg,
> +                              !(reg & APPLE_CLUSTER_PSTATE_BUSY), 2,
> +                              APPLE_CLUSTER_SWITCH_TIMEOUT)) {
> +               dev_err(cluster->dev, "timed out waiting for busy flag\n");
> +               return -EIO;
> +       }
> +
> +       reg &= ~(APPLE_CLUSTER_PSTATE_DESIRED1 | APPLE_CLUSTER_PSTATE_DESIRED2);
> +       reg |= FIELD_PREP(APPLE_CLUSTER_PSTATE_DESIRED1, level);
> +       reg |= FIELD_PREP(APPLE_CLUSTER_PSTATE_DESIRED2, level);
> +       reg |= APPLE_CLUSTER_PSTATE_SET;
> +
> +       writeq_relaxed(reg, cluster->reg_base + APPLE_CLUSTER_PSTATE);
> +
> +       if (cluster->has_pd)
> +               dev_pm_genpd_set_performance_state(cluster->dev,
> +                                                  dev_pm_opp_get_required_pstate(opp, 0));

This looks bad from a locking perspective. How is lockdep holding up
with this driver? We're underneath the prepare lock here and we're
setting a couple level registers which is all good but now we're calling
into genpd code and who knows what's going to happen locking wise.

I don't actually see anything in here that indicates this is supposed to
be a clk provider. Is it being modeled as a clk so that it can use
cpufreq-dt? If it was a clk provider I'd expect it to be looking at
parent clk rates, and reading hardware to calculate frequencies based on
dividers and multipliers, etc. None of that is happening here.

Why not write a cpufreq driver, similar to qcom-cpufreq-hw.c that looks
through the OPP table and then writes the value into the pstate
registers? The registers in here look awfully similar to the qcom
hardware. I don't know what the DESIRED1 and DESIRED2 registers are for
though. Maybe they're so that one or the other frequency can be used if
available? Like a min/max?

Either way, writing this as a cpufreq driver avoids the clk framework
entirely which is super great for me :) It also avoids locking headaches
from the clk prepare lock, and it also lets you support lockless cpufreq
transitions by implementing the fast_switch function. I don't see any
downsides to the cpufreq driver approach.

> +
> +       return 0;
> +}
> +
> +static unsigned long apple_cluster_clk_recalc_rate(struct clk_hw *hw, unsigned long parent_rate)
> +{
> +       struct apple_cluster_clk *cluster = to_apple_cluster_clk(hw);
> +       struct dev_pm_opp *opp;
> +       u64 reg;
> +
> +       reg = readq_relaxed(cluster->reg_base + APPLE_CLUSTER_PSTATE);
> +
> +       opp = dev_pm_opp_find_level_exact(cluster->dev,
> +                                         FIELD_GET(APPLE_CLUSTER_PSTATE_DESIRED1, reg));
> +
> +       if (IS_ERR(opp)) {
> +               dev_err(cluster->dev, "failed to find level: 0x%llx (%ld)\n", reg, PTR_ERR(opp));
> +               return 0;
> +       }
> +
> +       return dev_pm_opp_get_freq(opp);
> +}
> +
> +static long apple_cluster_clk_round_rate(struct clk_hw *hw, unsigned long rate,
> +                                                 unsigned long *parent_rate)
> +{
> +       struct apple_cluster_clk *cluster = to_apple_cluster_clk(hw);
> +       struct dev_pm_opp *opp;
> +
> +       opp = dev_pm_opp_find_freq_floor(cluster->dev, &rate);
> +
> +       if (IS_ERR(opp)) {
> +               dev_err(cluster->dev, "failed to find rate: %ld (%ld)\n", rate, PTR_ERR(opp));
> +               return PTR_ERR(opp);
> +       }
> +
> +       return rate;
> +}
> +
> +static const struct clk_ops apple_cluster_clk_ops = {
> +       .set_rate = apple_cluster_clk_set_rate,
> +       .recalc_rate = apple_cluster_clk_recalc_rate,
> +       .round_rate = apple_cluster_clk_round_rate,
> +};
> +
> +static int apple_cluster_clk_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct device_node *node = dev->of_node;
> +       struct apple_cluster_clk *cluster;
> +       struct clk_hw *hw;
> +       struct clk_init_data init;

init = { };

is more kernel style.

> +       int ret;
> +
> +       memset(&init, 0, sizeof(init));

And then memset is dropped.

> +       cluster = devm_kzalloc(dev, sizeof(*cluster), GFP_KERNEL);
> +       if (!cluster)
> +               return -ENOMEM;
> +
> +       cluster->dev = dev;
> +       cluster->reg_base = devm_platform_ioremap_resource(pdev, 0);
> +       if (IS_ERR(cluster->reg_base))
> +               return PTR_ERR(cluster->reg_base);
> +
> +       hw = &cluster->hw;
> +       hw->init = &init;
> +
> +       init.name = pdev->name;
> +       init.num_parents = 0;

Drop?

> +       init.ops = &apple_cluster_clk_ops;
> +       init.flags = 0;

Drop?

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel