From: Viresh Kumar <viresh.kumar@linaro.org>
To: Hector Martin <marcan@marcan.st>
Cc: "Rafael J. Wysocki" <rafael@kernel.org>,
Matthias Brugger <matthias.bgg@gmail.com>,
Sven Peter <sven@svenpeter.dev>,
Alyssa Rosenzweig <alyssa@rosenzweig.io>,
Rob Herring <robh+dt@kernel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
Stephen Boyd <sboyd@kernel.org>,
Ulf Hansson <ulf.hansson@linaro.org>,
Marc Zyngier <maz@kernel.org>,
Mark Kettenis <mark.kettenis@xs4all.nl>,
asahi@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
linux-pm@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v3 4/5] cpufreq: apple-soc: Add new driver to control Apple SoC CPU P-states
Date: Wed, 2 Nov 2022 11:48:19 +0530 [thread overview]
Message-ID: <20221102061819.dyl5ah6qffntqieh@vireshk-i7> (raw)
In-Reply-To: <20221024043925.25379-5-marcan@marcan.st>
On 24-10-22, 13:39, Hector Martin wrote:
> diff --git a/drivers/cpufreq/apple-soc-cpufreq.c b/drivers/cpufreq/apple-soc-cpufreq.c
> +struct apple_soc_cpufreq_info {
> + u64 max_pstate;
> + u64 cur_pstate_mask;
> + u64 cur_pstate_shift;
> +};
> +
> +struct apple_cpu_priv {
> + struct device *cpu_dev;
> + void __iomem *reg_base;
> + const struct apple_soc_cpufreq_info *info;
> +};
> +
> +static struct cpufreq_driver apple_soc_cpufreq_driver;
> +
> +const struct apple_soc_cpufreq_info soc_t8103_info = {
static ? For other instances too.
> + .max_pstate = 15,
> + .cur_pstate_mask = APPLE_DVFS_STATUS_CUR_PS_T8103,
> + .cur_pstate_shift = APPLE_DVFS_STATUS_CUR_PS_SHIFT_T8103,
> +};
> +
> +const struct apple_soc_cpufreq_info soc_t8112_info = {
> + .max_pstate = 31,
> + .cur_pstate_mask = APPLE_DVFS_STATUS_CUR_PS_T8112,
> + .cur_pstate_shift = APPLE_DVFS_STATUS_CUR_PS_SHIFT_T8112,
> +};
> +
> +const struct apple_soc_cpufreq_info soc_default_info = {
> + .max_pstate = 15,
> + .cur_pstate_mask = 0, /* fallback */
> +};
> +
> +static const struct of_device_id apple_soc_cpufreq_of_match[] = {
> + {
> + .compatible = "apple,t8103-cluster-cpufreq",
> + .data = &soc_t8103_info,
> + },
> + {
Isn't the preferred way for this is "}, {" instead ?
I couldn't find this in Coding Guidelines, but somehow remember that
to be the preferred format.
> + .compatible = "apple,t8112-cluster-cpufreq",
> + .data = &soc_t8112_info,
> + },
> + {
> + .compatible = "apple,cluster-cpufreq",
> + .data = &soc_default_info,
> + },
> + {}
> +};
> +
> +static unsigned int apple_soc_cpufreq_get_rate(unsigned int cpu)
> +{
> + struct cpufreq_policy *policy = cpufreq_cpu_get_raw(cpu);
> + struct apple_cpu_priv *priv = policy->driver_data;
> + unsigned int pstate;
> + unsigned int i;
Merge these two ?
> +
> + if (priv->info->cur_pstate_mask) {
> + u64 reg = readq_relaxed(priv->reg_base + APPLE_DVFS_STATUS);
> +
> + pstate = (reg & priv->info->cur_pstate_mask) >> priv->info->cur_pstate_shift;
> + } else {
> + /*
> + * For the fallback case we might not know the layout of DVFS_STATUS,
> + * so just use the command register value (which ignores boost limitations).
> + */
> + u64 reg = readq_relaxed(priv->reg_base + APPLE_DVFS_CMD);
> +
> + pstate = FIELD_GET(APPLE_DVFS_CMD_PS1, reg);
> + }
> +
> + for (i = 0; policy->freq_table[i].frequency != CPUFREQ_TABLE_END; i++)
You may want to use, cpufreq_for_each_valid_entry(), or some other
generic iterator here.
> + if (policy->freq_table[i].driver_data == pstate)
> + return policy->freq_table[i].frequency;
> +
> + dev_err(priv->cpu_dev, "could not find frequency for pstate %d\n",
> + pstate);
> + return 0;
> +}
> +
> +static int apple_soc_cpufreq_set_target(struct cpufreq_policy *policy,
> + unsigned int index)
> +{
> + struct apple_cpu_priv *priv = policy->driver_data;
> + unsigned int pstate = policy->freq_table[index].driver_data;
> + u64 reg;
> +
> + /* Fallback for newer SoCs */
> + if (index > priv->info->max_pstate)
> + index = priv->info->max_pstate;
> +
> + if (readq_poll_timeout_atomic(priv->reg_base + APPLE_DVFS_CMD, reg,
> + !(reg & APPLE_DVFS_CMD_BUSY), 2,
> + APPLE_DVFS_TRANSITION_TIMEOUT)) {
> + return -EIO;
> + }
> +
> + reg &= ~(APPLE_DVFS_CMD_PS1 | APPLE_DVFS_CMD_PS2);
> + reg |= FIELD_PREP(APPLE_DVFS_CMD_PS1, pstate);
> + reg |= FIELD_PREP(APPLE_DVFS_CMD_PS2, pstate);
> + reg |= APPLE_DVFS_CMD_SET;
> +
> + writeq_relaxed(reg, priv->reg_base + APPLE_DVFS_CMD);
> +
> + return 0;
> +}
> +
> +static unsigned int apple_soc_cpufreq_fast_switch(struct cpufreq_policy *policy,
> + unsigned int target_freq)
> +{
> + if (apple_soc_cpufreq_set_target(policy, policy->cached_resolved_idx) < 0)
> + return 0;
> +
> + return policy->freq_table[policy->cached_resolved_idx].frequency;
> +}
> +
> +static int apple_soc_cpufreq_find_cluster(struct cpufreq_policy *policy,
> + void __iomem **reg_base,
> + const struct apple_soc_cpufreq_info **info)
> +{
> + struct of_phandle_args args;
> + const struct of_device_id *match;
> + int ret = 0;
> +
> + ret = of_perf_domain_get_sharing_cpumask(policy->cpu, "performance-domains",
> + "#performance-domain-cells",
> + policy->cpus, &args);
> + if (ret < 0)
> + return ret;
> +
> + match = of_match_node(apple_soc_cpufreq_of_match, args.np);
> + of_node_put(args.np);
> + if (!match)
> + return -ENODEV;
> +
> + *info = match->data;
> +
> + *reg_base = of_iomap(args.np, 0);
> + if (IS_ERR(*reg_base))
> + return PTR_ERR(*reg_base);
> +
> + return 0;
> +}
> +
> +static struct freq_attr *apple_soc_cpufreq_hw_attr[] = {
> + &cpufreq_freq_attr_scaling_available_freqs,
> + NULL,
> + NULL,
> +};
> +
> +static int apple_soc_cpufreq_init(struct cpufreq_policy *policy)
> +{
> + int ret, i;
> + unsigned int transition_latency;
> + void __iomem *reg_base;
> + struct device *cpu_dev;
> + struct apple_cpu_priv *priv;
> + const struct apple_soc_cpufreq_info *info;
> + struct cpufreq_frequency_table *freq_table;
> +
> + cpu_dev = get_cpu_device(policy->cpu);
> + if (!cpu_dev) {
> + pr_err("failed to get cpu%d device\n", policy->cpu);
> + return -ENODEV;
> + }
> +
> + ret = dev_pm_opp_of_add_table(cpu_dev);
> + if (ret < 0) {
> + dev_err(cpu_dev, "%s: failed to add OPP table: %d\n", __func__, ret);
> + return ret;
> + }
> +
> + ret = apple_soc_cpufreq_find_cluster(policy, ®_base, &info);
> + if (ret) {
> + dev_err(cpu_dev, "%s: failed to get cluster info: %d\n", __func__, ret);
> + return ret;
> + }
> +
> + ret = dev_pm_opp_set_sharing_cpus(cpu_dev, policy->cpus);
Why do you need this ? The OPP core should be able to find this
information by itself in your case AFAIU. The OPP core will refer
"operating-points-v2 = <&pcluster_opp>" and find that the cores are
related.
> + if (ret) {
> + dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n", __func__, ret);
> + goto out_iounmap;
> + }
> +
> + ret = dev_pm_opp_get_opp_count(cpu_dev);
> + if (ret <= 0) {
> + dev_dbg(cpu_dev, "OPP table is not ready, deferring probe\n");
Why would this happen in your case ?
> + ret = -EPROBE_DEFER;
> + goto out_free_opp;
> + }
> +
> + priv = kzalloc(sizeof(*priv), GFP_KERNEL);
> + if (!priv) {
> + ret = -ENOMEM;
> + goto out_free_opp;
> + }
> +
> + ret = dev_pm_opp_init_cpufreq_table(cpu_dev, &freq_table);
> + if (ret) {
> + dev_err(cpu_dev, "failed to init cpufreq table: %d\n", ret);
> + goto out_free_priv;
> + }
> +
> + /* Get OPP levels (p-state indexes) and stash them in driver_data */
> + for (i = 0; freq_table[i].frequency != CPUFREQ_TABLE_END; i++) {
> + unsigned long rate = freq_table[i].frequency * 1000;
> + struct dev_pm_opp *opp = dev_pm_opp_find_freq_floor(cpu_dev, &rate);
Shouldn't you use dev_pm_opp_find_freq_exact() here ?
--
viresh
next prev parent reply other threads:[~2022-11-02 6:18 UTC|newest]
Thread overview: 29+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-24 4:39 [PATCH v3 0/5] Apple SoC cpufreq driver Hector Martin
2022-10-24 4:39 ` [PATCH v3 1/5] MAINTAINERS: Add entries for " Hector Martin
2022-10-24 4:39 ` [PATCH v3 2/5] dt-bindings: cpufreq: apple,soc-cpufreq: Add binding for Apple SoC cpufreq Hector Martin
2022-10-25 16:01 ` Krzysztof Kozlowski
2022-10-25 17:22 ` Hector Martin
2022-10-25 18:56 ` Krzysztof Kozlowski
2022-10-26 4:18 ` Hector Martin
2022-10-26 14:13 ` Krzysztof Kozlowski
2022-10-25 23:12 ` Rob Herring
2022-10-26 4:26 ` Hector Martin
2022-10-24 4:39 ` [PATCH v3 3/5] cpufreq: Generalize of_perf_domain_get_sharing_cpumask phandle format Hector Martin
2022-11-02 5:37 ` Viresh Kumar
2022-10-24 4:39 ` [PATCH v3 4/5] cpufreq: apple-soc: Add new driver to control Apple SoC CPU P-states Hector Martin
2022-10-24 8:27 ` Marc Zyngier
2022-11-09 12:13 ` Hector Martin
2022-11-09 14:20 ` Marc Zyngier
2022-11-09 15:39 ` Hector Martin
2022-11-01 15:16 ` Ulf Hansson
2022-11-01 18:17 ` Hector Martin
2022-11-14 19:49 ` Ulf Hansson
2022-11-02 6:18 ` Viresh Kumar [this message]
2022-11-09 12:36 ` Hector Martin
2022-11-14 6:51 ` Viresh Kumar
2022-11-14 6:57 ` Hector Martin
2022-11-14 7:03 ` Viresh Kumar
2022-11-14 11:06 ` Hector Martin
2022-10-24 4:39 ` [PATCH v3 5/5] arm64: dts: apple: Add CPU topology & cpufreq nodes for t8103 Hector Martin
2022-10-25 16:02 ` Krzysztof Kozlowski
2022-10-24 8:28 ` [PATCH v3 0/5] Apple SoC cpufreq driver Marc Zyngier
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20221102061819.dyl5ah6qffntqieh@vireshk-i7 \
--to=viresh.kumar@linaro.org \
--cc=alyssa@rosenzweig.io \
--cc=asahi@lists.linux.dev \
--cc=devicetree@vger.kernel.org \
--cc=krzysztof.kozlowski+dt@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=marcan@marcan.st \
--cc=mark.kettenis@xs4all.nl \
--cc=matthias.bgg@gmail.com \
--cc=maz@kernel.org \
--cc=rafael@kernel.org \
--cc=robh+dt@kernel.org \
--cc=sboyd@kernel.org \
--cc=sven@svenpeter.dev \
--cc=ulf.hansson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).