diff mbox series

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

Message ID 20211011165707.138157-8-marcan@marcan.st (mailing list archive)
State New, archived
Headers show
Series Apple SoC CPU P-state switching | expand

Commit Message

Hector Martin Oct. 11, 2021, 4:57 p.m. UTC 
This driver exposes the CPU performance state switching hardware in
Apple SoCs as a clock controller that can be used together with the
generic cpufreq-dt mechanism to implement cpufreq support. It also
supports binding to an apple-mcc instance, to increase memory controller
performance when the CPUs are in the highest performance states.

Signed-off-by: Hector Martin <marcan@marcan.st>
---
 drivers/clk/Kconfig             |   9 ++
 drivers/clk/Makefile            |   1 +
 drivers/clk/clk-apple-cluster.c | 184 ++++++++++++++++++++++++++++++++
 3 files changed, 194 insertions(+)
 create mode 100644 drivers/clk/clk-apple-cluster.c

Comments

Stephen Boyd Oct. 14, 2021, 10:07 p.m. UTC | #1 
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?
Hector Martin Oct. 17, 2021, 9:16 a.m. UTC | #2 
On 15/10/2021 07.07, Stephen Boyd wrote:
> 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.

It seems this is all going away given the other discussion threads point 
towards handling this directly via OPP in the cpufreq-dt driver. I'll 
run whatever I end up with for v2 through lockdep though, good call!

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

I wasn't too sure about this approach; I thought using a clk provider 
would end up simplifying things since I could use the cpufreq-dt 
machinery to take care of all the OPP stuff, and a lot of SoCs seemed to 
be going that way, but it seems cpufreq might be a better approach for 
this SoC?

There can only be one cpufreq driver instance, while I used two clock 
controllers to model the two clusters. So in the cpufreq case, the 
driver itself would have to deal with all potential CPU cluster 
instances/combinations itself. Not sure how much more code that will be, 
hopefully not too much...

I see qcom-cpufreq-hw uses a qcom,freq-domain prop to link CPUs to the 
cpufreq domains. cpufreq-dt and vexpress-spc-cpufreq instead use 
dev_pm_opp_get_sharing_cpus to look for shared OPP tables. Is there a 
reason not to do it that way and avoid the vendor prop? I guess the prop 
is more explicit while the sharing approach would have an implicit order 
dependency (i.e. CPUs are always grouped by cluster and clusters are 
listed in /cpus in the same order as in the cpufreq node)...

(Ack on the other comments, but if this becomes a cpufreq driver most of 
it is going to end up rewritten... :))

For the cpufreq case, do you have any suggestions as to how to relate it 
to the memory controller configuration tweaks? Ideally this would go 
through the OPP tables so it can be customized for future SoCs without 
stuff hardcoded in the driver... it seems the configuration affects 
power saving behavior / latencies, so it doesn't quite match the 
interconnect framework bandwidth request stuff. I'm also not sure how 
this would affect fast_switch, since going through those frameworks 
might imply locks... we might even find ourselves with a situation in 
the near future where multiple cpufreq policies can request memory 
controller latency reduction independently; I can come up with how to do 
this locklessly using atomics, but I can't imagine that being workable 
with higher-level frameworks, it would have to be a vendor-specific 
mechanism at that point...
diff mbox series

Patch

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
+	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
 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));
+
+	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;
+	int ret;
+
+	memset(&init, 0, sizeof(init));
+	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;
+	init.ops = &apple_cluster_clk_ops;
+	init.flags = 0;
+
+	ret = dev_pm_opp_of_add_table_noclk(dev, 0);
+	if (ret < 0) {
+		dev_err(dev, "failed to get opp table\n");
+		return ret;
+	}
+
+	cluster->has_pd = of_property_read_bool(node, "power-domains");
+
+	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_simple_get, hw);
+	if (ret < 0)
+		return ret;
+
+	ret = devm_clk_hw_register(dev, hw);
+	if (ret) {
+		dev_err(dev, "failed to register clock\n");
+		return ret;
+	}
+
+	return 0;
+}
+
+static const struct of_device_id apple_cluster_clk_of_match[] = {
+	{ .compatible = "apple,cluster-clk" },
+	{}
+};
+
+MODULE_DEVICE_TABLE(of, apple_cluster_clk_of_match);
+
+static struct platform_driver apple_cluster_clk_driver = {
+	.probe = apple_cluster_clk_probe,
+	.driver = {
+		.name = "apple-cluster-clk",
+		.of_match_table = apple_cluster_clk_of_match,
+	},
+};
+
+MODULE_AUTHOR("Hector Martin <marcan@marcan.st>");
+MODULE_DESCRIPTION("CPU cluster performance state driver for Apple SoCs");
+MODULE_LICENSE("GPL v2");
+
+module_platform_driver(apple_cluster_clk_driver);