From: Bjorn Andersson <bjorn.andersson@linaro.org>
To: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
Cc: rjw@rjwysocki.net, viresh.kumar@linaro.org, robh+dt@kernel.org,
agross@kernel.org, amitk@kernel.org, linux-pm@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-msm@vger.kernel.org, dmitry.baryshkov@linaro.org,
tdas@codeaurora.org
Subject: Re: [PATCH 4/7] cpufreq: qcom-hw: Make use of of_match data for offsets and row size
Date: Tue, 8 Sep 2020 10:31:28 -0500 [thread overview]
Message-ID: <20200908153128.GP3715@yoga> (raw)
In-Reply-To: <20200908075716.30357-5-manivannan.sadhasivam@linaro.org>
On Tue 08 Sep 02:57 CDT 2020, Manivannan Sadhasivam wrote:
> For preparing the driver to handle further SoC revisions, let's use the
> of_match data for getting the device specific offsets and row size instead
> of defining them globally.
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
> drivers/cpufreq/qcom-cpufreq-hw.c | 96 +++++++++++++++++++++----------
> 1 file changed, 66 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index ccea34f61152..41853db7c9b8 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -19,15 +19,21 @@
> #define LUT_L_VAL GENMASK(7, 0)
> #define LUT_CORE_COUNT GENMASK(18, 16)
> #define LUT_VOLT GENMASK(11, 0)
> -#define LUT_ROW_SIZE 32
> #define CLK_HW_DIV 2
> #define LUT_TURBO_IND 1
>
> -/* Register offsets */
> -#define REG_ENABLE 0x0
> -#define REG_FREQ_LUT 0x110
> -#define REG_VOLT_LUT 0x114
> -#define REG_PERF_STATE 0x920
> +struct qcom_cpufreq_soc_data {
> + u32 reg_enable;
> + u32 reg_freq_lut;
> + u32 reg_volt_lut;
> + u32 reg_perf_state;
> + u8 lut_row_size;
> +};
> +
> +struct qcom_cpufreq_data {
> + void __iomem *base;
> + const struct qcom_cpufreq_soc_data *soc_data;
> +};
>
> static unsigned long cpu_hw_rate, xo_rate;
> static bool icc_scaling_enabled;
> @@ -76,10 +82,11 @@ static int qcom_cpufreq_update_opp(struct device *cpu_dev,
> static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
> unsigned int index)
> {
> - void __iomem *perf_state_reg = policy->driver_data;
> + struct qcom_cpufreq_data *data = policy->driver_data;
> + const struct qcom_cpufreq_soc_data *soc_data = data->soc_data;
> unsigned long freq = policy->freq_table[index].frequency;
>
> - writel_relaxed(index, perf_state_reg);
> + writel_relaxed(index, data->base + soc_data->reg_perf_state);
>
> if (icc_scaling_enabled)
> qcom_cpufreq_set_bw(policy, freq);
> @@ -91,7 +98,8 @@ static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
>
> static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
> {
> - void __iomem *perf_state_reg;
> + struct qcom_cpufreq_data *data;
> + const struct qcom_cpufreq_soc_data *soc_data;
> struct cpufreq_policy *policy;
> unsigned int index;
>
> @@ -99,9 +107,10 @@ static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
> if (!policy)
> return 0;
>
> - perf_state_reg = policy->driver_data;
> + data = policy->driver_data;
> + soc_data = data->soc_data;
>
> - index = readl_relaxed(perf_state_reg);
> + index = readl_relaxed(data->base + soc_data->reg_perf_state);
> index = min(index, LUT_MAX_ENTRIES - 1);
>
> return policy->freq_table[index].frequency;
> @@ -110,12 +119,13 @@ static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
> static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
> unsigned int target_freq)
> {
> - void __iomem *perf_state_reg = policy->driver_data;
> + struct qcom_cpufreq_data *data = policy->driver_data;
> + const struct qcom_cpufreq_soc_data *soc_data = data->soc_data;
> unsigned int index;
> unsigned long freq;
>
> index = policy->cached_resolved_idx;
> - writel_relaxed(index, perf_state_reg);
> + writel_relaxed(index, data->base + soc_data->reg_perf_state);
>
> freq = policy->freq_table[index].frequency;
> arch_set_freq_scale(policy->related_cpus, freq,
> @@ -125,8 +135,7 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
> }
>
> static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev,
> - struct cpufreq_policy *policy,
> - void __iomem *base)
> + struct cpufreq_policy *policy)
> {
> u32 data, src, lval, i, core_count, prev_freq = 0, freq;
> u32 volt;
> @@ -134,6 +143,8 @@ static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev,
> struct dev_pm_opp *opp;
> unsigned long rate;
> int ret;
> + struct qcom_cpufreq_data *drv_data = policy->driver_data;
> + const struct qcom_cpufreq_soc_data *soc_data = drv_data->soc_data;
>
> table = kcalloc(LUT_MAX_ENTRIES + 1, sizeof(*table), GFP_KERNEL);
> if (!table)
> @@ -160,14 +171,14 @@ static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev,
> }
>
> for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> - data = readl_relaxed(base + REG_FREQ_LUT +
> - i * LUT_ROW_SIZE);
> + data = readl_relaxed(drv_data->base + soc_data->reg_freq_lut +
> + i * soc_data->lut_row_size);
> src = FIELD_GET(LUT_SRC, data);
> lval = FIELD_GET(LUT_L_VAL, data);
> core_count = FIELD_GET(LUT_CORE_COUNT, data);
>
> - data = readl_relaxed(base + REG_VOLT_LUT +
> - i * LUT_ROW_SIZE);
> + data = readl_relaxed(drv_data->base + soc_data->reg_volt_lut +
> + i * soc_data->lut_row_size);
> volt = FIELD_GET(LUT_VOLT, data) * 1000;
>
> if (src)
> @@ -237,6 +248,20 @@ static void qcom_get_related_cpus(int index, struct cpumask *m)
> }
> }
>
> +static const struct qcom_cpufreq_soc_data qcom_soc_data = {
> + .reg_enable = 0x0,
> + .reg_freq_lut = 0x110,
> + .reg_volt_lut = 0x114,
> + .reg_perf_state = 0x920,
> + .lut_row_size = 32,
> +};
> +
> +static const struct of_device_id qcom_cpufreq_hw_match[] = {
> + { .compatible = "qcom,cpufreq-hw", .data = &qcom_soc_data },
> + {}
> +};
> +MODULE_DEVICE_TABLE(of, qcom_cpufreq_hw_match);
> +
> static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> {
> struct platform_device *pdev = cpufreq_get_driver_data();
> @@ -246,6 +271,8 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> struct device *cpu_dev;
> struct resource *res;
> void __iomem *base;
> + struct qcom_cpufreq_data *data;
> + const struct of_device_id *match;
> int ret, index;
>
> cpu_dev = get_cpu_device(policy->cpu);
> @@ -275,8 +302,23 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> if (!base)
> return -ENOMEM;
>
> + data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> + if (!data) {
> + ret = -ENOMEM;
> + goto error;
> + }
> +
> + match = of_match_device(qcom_cpufreq_hw_match, &pdev->dev);
Rather than having to move the qcom_cpufreq_hw_match table and reference
it here, use the more succinct:
data->soc-data = of_device_get_match_data(&pdev->dev);
Apart from this, I think the patch looks good.
> + if (!match) {
> + ret = -ENODEV;
> + goto error;
> + }
> +
> + data->soc_data = match->data;
> + data->base = base;
> +
> /* HW should be in enabled state to proceed */
> - if (!(readl_relaxed(base + REG_ENABLE) & 0x1)) {
> + if (!(readl_relaxed(base + data->soc_data->reg_enable) & 0x1)) {
> dev_err(dev, "Domain-%d cpufreq hardware not enabled\n", index);
> ret = -ENODEV;
> goto error;
> @@ -289,9 +331,9 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> goto error;
> }
>
> - policy->driver_data = base + REG_PERF_STATE;
> + policy->driver_data = data;
>
> - ret = qcom_cpufreq_hw_read_lut(cpu_dev, policy, base);
> + ret = qcom_cpufreq_hw_read_lut(cpu_dev, policy);
> if (ret) {
> dev_err(dev, "Domain-%d failed to read LUT\n", index);
> goto error;
> @@ -315,13 +357,13 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> static int qcom_cpufreq_hw_cpu_exit(struct cpufreq_policy *policy)
> {
> struct device *cpu_dev = get_cpu_device(policy->cpu);
> - void __iomem *base = policy->driver_data - REG_PERF_STATE;
> + struct qcom_cpufreq_data *data = policy->driver_data;
> struct platform_device *pdev = cpufreq_get_driver_data();
>
> dev_pm_opp_remove_all_dynamic(cpu_dev);
> dev_pm_opp_of_cpumask_remove_table(policy->related_cpus);
> kfree(policy->freq_table);
> - devm_iounmap(&pdev->dev, base);
> + devm_iounmap(&pdev->dev, data->base);
PS. Unrelated to your series, I don't like the fact that we use devres
in code paths that's only indirectly tied to the device's life cycle.
Regards,
Bjorn
>
> return 0;
> }
> @@ -391,12 +433,6 @@ static int qcom_cpufreq_hw_driver_remove(struct platform_device *pdev)
> return cpufreq_unregister_driver(&cpufreq_qcom_hw_driver);
> }
>
> -static const struct of_device_id qcom_cpufreq_hw_match[] = {
> - { .compatible = "qcom,cpufreq-hw" },
> - {}
> -};
> -MODULE_DEVICE_TABLE(of, qcom_cpufreq_hw_match);
> -
> static struct platform_driver qcom_cpufreq_hw_driver = {
> .probe = qcom_cpufreq_hw_driver_probe,
> .remove = qcom_cpufreq_hw_driver_remove,
> --
> 2.17.1
>
next prev parent reply other threads:[~2020-09-08 19:20 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-09-08 7:57 [PATCH 0/7] Add CPUFreq support for SM8250 SoC Manivannan Sadhasivam
2020-09-08 7:57 ` [PATCH 1/7] dt-bindings: cpufreq: cpufreq-qcom-hw: Document SM8250 compatible Manivannan Sadhasivam
2020-09-08 11:56 ` Amit Kucheria
2020-09-08 14:58 ` Bjorn Andersson
2020-09-08 15:10 ` Manivannan Sadhasivam
2020-09-15 16:38 ` Rob Herring
2020-09-08 7:57 ` [PATCH 2/7] arm64: dts: qcom: sm8250: Add cpufreq hw node Manivannan Sadhasivam
2020-09-08 10:39 ` Viresh Kumar
2020-09-08 11:08 ` Manivannan Sadhasivam
2020-09-08 11:12 ` Viresh Kumar
2020-09-08 11:56 ` Amit Kucheria
2020-09-08 7:57 ` [PATCH 3/7] cpufreq: qcom-hw: Make use of cpufreq driver_data for passing pdev Manivannan Sadhasivam
2020-09-08 10:31 ` Viresh Kumar
2020-09-08 7:57 ` [PATCH 4/7] cpufreq: qcom-hw: Make use of of_match data for offsets and row size Manivannan Sadhasivam
2020-09-08 12:18 ` Amit Kucheria
2020-09-08 15:09 ` Manivannan Sadhasivam
2020-09-08 17:06 ` Amit Kucheria
2020-09-08 15:31 ` Bjorn Andersson [this message]
2020-09-08 7:57 ` [PATCH 5/7] cpufreq: qcom-hw: Use regmap for accessing hardware registers Manivannan Sadhasivam
2020-09-08 10:34 ` Viresh Kumar
2020-09-08 11:11 ` Manivannan Sadhasivam
2020-09-08 11:18 ` Viresh Kumar
2020-09-08 11:28 ` Manivannan Sadhasivam
2020-09-08 11:48 ` Amit Kucheria
2020-09-08 12:08 ` Amit Kucheria
2020-09-08 15:03 ` Manivannan Sadhasivam
2020-09-09 4:35 ` Viresh Kumar
2020-09-08 14:08 ` Sudeep Holla
2020-09-08 15:39 ` Bjorn Andersson
2020-09-08 7:57 ` [PATCH 6/7] cpufreq: qcom-hw: Add cpufreq support for SM8250 SoC Manivannan Sadhasivam
2020-09-08 12:10 ` Amit Kucheria
2020-09-08 15:22 ` Bjorn Andersson
2020-09-08 15:29 ` Manivannan Sadhasivam
2020-09-08 7:57 ` [PATCH 7/7] cpufreq: qcom-hw: Use devm_platform_ioremap_resource() to simplify code Manivannan Sadhasivam
2020-09-08 10:36 ` Viresh Kumar
2020-09-08 11:12 ` Manivannan Sadhasivam
2020-09-08 12:00 ` Amit Kucheria
2020-09-08 15:35 ` Bjorn Andersson
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=20200908153128.GP3715@yoga \
--to=bjorn.andersson@linaro.org \
--cc=agross@kernel.org \
--cc=amitk@kernel.org \
--cc=devicetree@vger.kernel.org \
--cc=dmitry.baryshkov@linaro.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-pm@vger.kernel.org \
--cc=manivannan.sadhasivam@linaro.org \
--cc=rjw@rjwysocki.net \
--cc=robh+dt@kernel.org \
--cc=tdas@codeaurora.org \
--cc=viresh.kumar@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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.