[PATCH 0/7] Add CPUFreq support for SM8250 SoC

[PATCH 0/7] Add CPUFreq support for SM8250 SoC

[Manivannan Sadhasivam]

Hello,

This series adds CPUFreq support for Qualcomm SM8250 SoC. The existing
qcom-hw driver is reworked to support the EPSS block on this SoC which
handles the CPUFreq duties.

The EPSS block supports additional features for which incremental patches
will be submitted on top of this series!

Thanks,
Mani

Bjorn Andersson (1):
  arm64: dts: qcom: sm8250: Add cpufreq hw node

Manivannan Sadhasivam (6):
  dt-bindings: cpufreq: cpufreq-qcom-hw: Document SM8250 compatible
  cpufreq: qcom-hw: Make use of cpufreq driver_data for passing pdev
  cpufreq: qcom-hw: Make use of of_match data for offsets and row size
  cpufreq: qcom-hw: Use regmap for accessing hardware registers
  cpufreq: qcom-hw: Add cpufreq support for SM8250 SoC
  cpufreq: qcom-hw: Use devm_platform_ioremap_resource() to simplify
    code

 .../bindings/cpufreq/cpufreq-qcom-hw.txt      |   2 +-
 arch/arm64/boot/dts/qcom/sm8250.dtsi          |  22 +++
 drivers/cpufreq/qcom-cpufreq-hw.c             | 160 +++++++++++++-----
 3 files changed, 143 insertions(+), 41 deletions(-)

-- 
2.17.1

[PATCH 1/7] dt-bindings: cpufreq: cpufreq-qcom-hw: Document SM8250 compatible

[Manivannan Sadhasivam]

Document the SM8250 SoC specific compatible for Qualcomm Cpufreq HW. The
hardware block which carries out CPUFreq operations on SM8250 SoC is
called EPSS.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
index 33856947c561..aea4ddb2b9e8 100644
--- a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
@@ -8,7 +8,7 @@ Properties:
 - compatible
 	Usage:		required
 	Value type:	<string>
-	Definition:	must be "qcom,cpufreq-hw".
+	Definition:	must be "qcom,cpufreq-hw" or "qcom,sm8250-epss".
 
 - clocks
 	Usage:		required
-- 
2.17.1

[PATCH 2/7] arm64: dts: qcom: sm8250: Add cpufreq hw node

[Manivannan Sadhasivam]

From: Bjorn Andersson <bjorn.andersson@linaro.org>

Add cpufreq HW device node to scale 4-Silver/3-Gold/1-Gold+ cores
on SM8250 SoCs.

Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 arch/arm64/boot/dts/qcom/sm8250.dtsi | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
index e7d139e1a6ce..aafb46a26a9c 100644
--- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
+++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
@@ -87,6 +87,7 @@
 			reg = <0x0 0x0>;
 			enable-method = "psci";
 			next-level-cache = <&L2_0>;
+			qcom,freq-domain = <&cpufreq_hw 0>;
 			L2_0: l2-cache {
 			      compatible = "cache";
 			      next-level-cache = <&L3_0>;
@@ -102,6 +103,7 @@
 			reg = <0x0 0x100>;
 			enable-method = "psci";
 			next-level-cache = <&L2_100>;
+			qcom,freq-domain = <&cpufreq_hw 0>;
 			L2_100: l2-cache {
 			      compatible = "cache";
 			      next-level-cache = <&L3_0>;
@@ -114,6 +116,7 @@
 			reg = <0x0 0x200>;
 			enable-method = "psci";
 			next-level-cache = <&L2_200>;
+			qcom,freq-domain = <&cpufreq_hw 0>;
 			L2_200: l2-cache {
 			      compatible = "cache";
 			      next-level-cache = <&L3_0>;
@@ -126,6 +129,7 @@
 			reg = <0x0 0x300>;
 			enable-method = "psci";
 			next-level-cache = <&L2_300>;
+			qcom,freq-domain = <&cpufreq_hw 0>;
 			L2_300: l2-cache {
 			      compatible = "cache";
 			      next-level-cache = <&L3_0>;
@@ -138,6 +142,7 @@
 			reg = <0x0 0x400>;
 			enable-method = "psci";
 			next-level-cache = <&L2_400>;
+			qcom,freq-domain = <&cpufreq_hw 1>;
 			L2_400: l2-cache {
 			      compatible = "cache";
 			      next-level-cache = <&L3_0>;
@@ -150,6 +155,7 @@
 			reg = <0x0 0x500>;
 			enable-method = "psci";
 			next-level-cache = <&L2_500>;
+			qcom,freq-domain = <&cpufreq_hw 1>;
 			L2_500: l2-cache {
 			      compatible = "cache";
 			      next-level-cache = <&L3_0>;
@@ -163,6 +169,7 @@
 			reg = <0x0 0x600>;
 			enable-method = "psci";
 			next-level-cache = <&L2_600>;
+			qcom,freq-domain = <&cpufreq_hw 1>;
 			L2_600: l2-cache {
 			      compatible = "cache";
 			      next-level-cache = <&L3_0>;
@@ -175,6 +182,7 @@
 			reg = <0x0 0x700>;
 			enable-method = "psci";
 			next-level-cache = <&L2_700>;
+			qcom,freq-domain = <&cpufreq_hw 2>;
 			L2_700: l2-cache {
 			      compatible = "cache";
 			      next-level-cache = <&L3_0>;
@@ -2076,6 +2084,20 @@
 				};
 			};
 		};
+
+		cpufreq_hw: cpufreq@18591000 {
+			compatible = "qcom,sm8250-epss";
+			reg = <0 0x18591000 0 0x1000>,
+			      <0 0x18592000 0 0x1000>,
+			      <0 0x18593000 0 0x1000>;
+			reg-names = "freq-domain0", "freq-domain1",
+				    "freq-domain2";
+
+			clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GPLL0>;
+			clock-names = "xo", "alternate";
+
+			#freq-domain-cells = <1>;
+		};
 	};
 
 	timer {
-- 
2.17.1

[PATCH 3/7] cpufreq: qcom-hw: Make use of cpufreq driver_data for passing pdev

[Manivannan Sadhasivam]

Get rid of global_pdev pointer and make use of cpufreq driver_data for
passing the reference of pdev. This aligns with what other cpufreq drivers
are doing.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/cpufreq/qcom-cpufreq-hw.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index 3fb044b907a8..ccea34f61152 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -30,7 +30,6 @@
 #define REG_PERF_STATE			0x920
 
 static unsigned long cpu_hw_rate, xo_rate;
-static struct platform_device *global_pdev;
 static bool icc_scaling_enabled;
 
 static int qcom_cpufreq_set_bw(struct cpufreq_policy *policy,
@@ -240,7 +239,8 @@ static void qcom_get_related_cpus(int index, struct cpumask *m)
 
 static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 {
-	struct device *dev = &global_pdev->dev;
+	struct platform_device *pdev = cpufreq_get_driver_data();
+	struct device *dev = &pdev->dev;
 	struct of_phandle_args args;
 	struct device_node *cpu_np;
 	struct device *cpu_dev;
@@ -267,7 +267,7 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 
 	index = args.args[0];
 
-	res = platform_get_resource(global_pdev, IORESOURCE_MEM, index);
+	res = platform_get_resource(pdev, IORESOURCE_MEM, index);
 	if (!res)
 		return -ENODEV;
 
@@ -316,11 +316,12 @@ 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 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(&global_pdev->dev, base);
+	devm_iounmap(&pdev->dev, base);
 
 	return 0;
 }
@@ -365,7 +366,7 @@ static int qcom_cpufreq_hw_driver_probe(struct platform_device *pdev)
 	cpu_hw_rate = clk_get_rate(clk) / CLK_HW_DIV;
 	clk_put(clk);
 
-	global_pdev = pdev;
+	cpufreq_qcom_hw_driver.driver_data = pdev;
 
 	/* Check for optional interconnect paths on CPU0 */
 	cpu_dev = get_cpu_device(0);
-- 
2.17.1

[PATCH 4/7] cpufreq: qcom-hw: Make use of of_match data for offsets and row size

[Manivannan Sadhasivam]

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);
+	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);
 
 	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

[PATCH 5/7] cpufreq: qcom-hw: Use regmap for accessing hardware registers

[Manivannan Sadhasivam]

Use regmap for accessing cpufreq registers in hardware.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/cpufreq/qcom-cpufreq-hw.c | 55 ++++++++++++++++++++++++++-----
 1 file changed, 47 insertions(+), 8 deletions(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index 41853db7c9b8..de816bcafd33 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -12,6 +12,7 @@
 #include <linux/of_address.h>
 #include <linux/of_platform.h>
 #include <linux/pm_opp.h>
+#include <linux/regmap.h>
 #include <linux/slab.h>
 
 #define LUT_MAX_ENTRIES			40U
@@ -32,6 +33,7 @@ struct qcom_cpufreq_soc_data {
 
 struct qcom_cpufreq_data {
 	void __iomem *base;
+	struct regmap *regmap;
 	const struct qcom_cpufreq_soc_data *soc_data;
 };
 
@@ -85,8 +87,11 @@ static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
 	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;
+	int ret;
 
-	writel_relaxed(index, data->base + soc_data->reg_perf_state);
+	ret = regmap_write(data->regmap, soc_data->reg_perf_state, index);
+	if (ret)
+		return ret;
 
 	if (icc_scaling_enabled)
 		qcom_cpufreq_set_bw(policy, freq);
@@ -102,6 +107,7 @@ static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
 	const struct qcom_cpufreq_soc_data *soc_data;
 	struct cpufreq_policy *policy;
 	unsigned int index;
+	int ret;
 
 	policy = cpufreq_cpu_get_raw(cpu);
 	if (!policy)
@@ -110,7 +116,10 @@ static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
 	data = policy->driver_data;
 	soc_data = data->soc_data;
 
-	index = readl_relaxed(data->base + soc_data->reg_perf_state);
+	ret = regmap_read(data->regmap, soc_data->reg_perf_state, &index);
+	if (ret)
+		return 0;
+
 	index = min(index, LUT_MAX_ENTRIES - 1);
 
 	return policy->freq_table[index].frequency;
@@ -123,9 +132,12 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
 	const struct qcom_cpufreq_soc_data *soc_data = data->soc_data;
 	unsigned int index;
 	unsigned long freq;
+	int ret;
 
 	index = policy->cached_resolved_idx;
-	writel_relaxed(index, data->base + soc_data->reg_perf_state);
+	ret = regmap_write(data->regmap, soc_data->reg_perf_state, index);
+	if (ret)
+		return 0;
 
 	freq = policy->freq_table[index].frequency;
 	arch_set_freq_scale(policy->related_cpus, freq,
@@ -171,14 +183,24 @@ static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev,
 	}
 
 	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
-		data = readl_relaxed(drv_data->base + soc_data->reg_freq_lut +
-				      i * soc_data->lut_row_size);
+		ret = regmap_read(drv_data->regmap, soc_data->reg_freq_lut +
+				  i * soc_data->lut_row_size, &data);
+		if (ret) {
+			kfree(table);
+			return ret;
+		}
+
 		src = FIELD_GET(LUT_SRC, data);
 		lval = FIELD_GET(LUT_L_VAL, data);
 		core_count = FIELD_GET(LUT_CORE_COUNT, data);
 
-		data = readl_relaxed(drv_data->base + soc_data->reg_volt_lut +
-				      i * soc_data->lut_row_size);
+		ret = regmap_read(drv_data->regmap, soc_data->reg_volt_lut +
+				  i * soc_data->lut_row_size, &data);
+		if (ret) {
+			kfree(table);
+			return ret;
+		}
+
 		volt = FIELD_GET(LUT_VOLT, data) * 1000;
 
 		if (src)
@@ -248,6 +270,13 @@ static void qcom_get_related_cpus(int index, struct cpumask *m)
 	}
 }
 
+static struct regmap_config qcom_cpufreq_regmap = {
+	.reg_bits = 32,
+	.reg_stride = 4,
+	.val_bits = 32,
+	.fast_io = true,
+};
+
 static const struct qcom_cpufreq_soc_data qcom_soc_data = {
 	.reg_enable = 0x0,
 	.reg_freq_lut = 0x110,
@@ -274,6 +303,7 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 	struct qcom_cpufreq_data *data;
 	const struct of_device_id *match;
 	int ret, index;
+	u32 val;
 
 	cpu_dev = get_cpu_device(policy->cpu);
 	if (!cpu_dev) {
@@ -316,9 +346,18 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 
 	data->soc_data = match->data;
 	data->base = base;
+	data->regmap = devm_regmap_init_mmio(dev, base, &qcom_cpufreq_regmap);
+	if (IS_ERR(data->regmap)) {
+		ret = PTR_ERR(data->regmap);
+		goto error;
+	}
 
 	/* HW should be in enabled state to proceed */
-	if (!(readl_relaxed(base + data->soc_data->reg_enable) & 0x1)) {
+	ret = regmap_read(data->regmap, data->soc_data->reg_enable, &val);
+	if (ret)
+		goto error;
+
+	if (!(val & 0x1)) {
 		dev_err(dev, "Domain-%d cpufreq hardware not enabled\n", index);
 		ret = -ENODEV;
 		goto error;
-- 
2.17.1

[PATCH 6/7] cpufreq: qcom-hw: Add cpufreq support for SM8250 SoC

[Manivannan Sadhasivam]

SM8250 SoC uses EPSS block for carrying out the cpufreq duties. Hence, add
support for it in the driver with relevant of_match data.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/cpufreq/qcom-cpufreq-hw.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index de816bcafd33..c3c397cc3dc6 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -285,8 +285,17 @@ static const struct qcom_cpufreq_soc_data qcom_soc_data = {
 	.lut_row_size = 32,
 };
 
+static const struct qcom_cpufreq_soc_data sm8250_soc_data = {
+	.reg_enable = 0x0,
+	.reg_freq_lut = 0x100,
+	.reg_volt_lut = 0x200,
+	.reg_perf_state = 0x320,
+	.lut_row_size = 4,
+};
+
 static const struct of_device_id qcom_cpufreq_hw_match[] = {
 	{ .compatible = "qcom,cpufreq-hw", .data = &qcom_soc_data },
+	{ .compatible = "qcom,sm8250-epss", .data = &sm8250_soc_data },
 	{}
 };
 MODULE_DEVICE_TABLE(of, qcom_cpufreq_hw_match);
-- 
2.17.1

[PATCH 7/7] cpufreq: qcom-hw: Use devm_platform_ioremap_resource() to simplify code

[Manivannan Sadhasivam]

devm_platform_ioremap_resource() is the combination of
platform_get_resource() and devm_ioremap_resource(). Hence, use it to
simplify the code a bit.

Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
---
 drivers/cpufreq/qcom-cpufreq-hw.c | 11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
index c3c397cc3dc6..6eeeb2bd4dfa 100644
--- a/drivers/cpufreq/qcom-cpufreq-hw.c
+++ b/drivers/cpufreq/qcom-cpufreq-hw.c
@@ -307,7 +307,6 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 	struct of_phandle_args args;
 	struct device_node *cpu_np;
 	struct device *cpu_dev;
-	struct resource *res;
 	void __iomem *base;
 	struct qcom_cpufreq_data *data;
 	const struct of_device_id *match;
@@ -333,13 +332,9 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
 
 	index = args.args[0];
 
-	res = platform_get_resource(pdev, IORESOURCE_MEM, index);
-	if (!res)
-		return -ENODEV;
-
-	base = devm_ioremap(dev, res->start, resource_size(res));
-	if (!base)
-		return -ENOMEM;
+	base = devm_platform_ioremap_resource(pdev, index);
+	if (IS_ERR(base))
+		return PTR_ERR(base);
 
 	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
 	if (!data) {
-- 
2.17.1

Re: [PATCH 3/7] cpufreq: qcom-hw: Make use of cpufreq driver_data for passing pdev

[Viresh Kumar]

On 08-09-20, 13:27, Manivannan Sadhasivam wrote:
> Get rid of global_pdev pointer and make use of cpufreq driver_data for
> passing the reference of pdev. This aligns with what other cpufreq drivers
> are doing.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/cpufreq/qcom-cpufreq-hw.c | 11 ++++++-----
>  1 file changed, 6 insertions(+), 5 deletions(-)

Applied. Thanks.

-- 
viresh

Re: [PATCH 5/7] cpufreq: qcom-hw: Use regmap for accessing hardware registers

[Viresh Kumar]

On 08-09-20, 13:27, Manivannan Sadhasivam wrote:
> Use regmap for accessing cpufreq registers in hardware.

Why ? Please mention why a change is required in the log.

> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/cpufreq/qcom-cpufreq-hw.c | 55 ++++++++++++++++++++++++++-----
>  1 file changed, 47 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index 41853db7c9b8..de816bcafd33 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -12,6 +12,7 @@
>  #include <linux/of_address.h>
>  #include <linux/of_platform.h>
>  #include <linux/pm_opp.h>
> +#include <linux/regmap.h>
>  #include <linux/slab.h>
>  
>  #define LUT_MAX_ENTRIES			40U
> @@ -32,6 +33,7 @@ struct qcom_cpufreq_soc_data {
>  
>  struct qcom_cpufreq_data {
>  	void __iomem *base;
> +	struct regmap *regmap;
>  	const struct qcom_cpufreq_soc_data *soc_data;
>  };
>  
> @@ -85,8 +87,11 @@ static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
>  	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;
> +	int ret;
>  
> -	writel_relaxed(index, data->base + soc_data->reg_perf_state);
> +	ret = regmap_write(data->regmap, soc_data->reg_perf_state, index);
> +	if (ret)
> +		return ret;
>  
>  	if (icc_scaling_enabled)
>  		qcom_cpufreq_set_bw(policy, freq);
> @@ -102,6 +107,7 @@ static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
>  	const struct qcom_cpufreq_soc_data *soc_data;
>  	struct cpufreq_policy *policy;
>  	unsigned int index;
> +	int ret;
>  
>  	policy = cpufreq_cpu_get_raw(cpu);
>  	if (!policy)
> @@ -110,7 +116,10 @@ static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
>  	data = policy->driver_data;
>  	soc_data = data->soc_data;
>  
> -	index = readl_relaxed(data->base + soc_data->reg_perf_state);
> +	ret = regmap_read(data->regmap, soc_data->reg_perf_state, &index);
> +	if (ret)
> +		return 0;
> +
>  	index = min(index, LUT_MAX_ENTRIES - 1);
>  
>  	return policy->freq_table[index].frequency;
> @@ -123,9 +132,12 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
>  	const struct qcom_cpufreq_soc_data *soc_data = data->soc_data;
>  	unsigned int index;
>  	unsigned long freq;
> +	int ret;
>  
>  	index = policy->cached_resolved_idx;
> -	writel_relaxed(index, data->base + soc_data->reg_perf_state);
> +	ret = regmap_write(data->regmap, soc_data->reg_perf_state, index);
> +	if (ret)
> +		return 0;
>  
>  	freq = policy->freq_table[index].frequency;
>  	arch_set_freq_scale(policy->related_cpus, freq,
> @@ -171,14 +183,24 @@ static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev,
>  	}
>  
>  	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> -		data = readl_relaxed(drv_data->base + soc_data->reg_freq_lut +
> -				      i * soc_data->lut_row_size);
> +		ret = regmap_read(drv_data->regmap, soc_data->reg_freq_lut +
> +				  i * soc_data->lut_row_size, &data);
> +		if (ret) {
> +			kfree(table);
> +			return ret;
> +		}
> +
>  		src = FIELD_GET(LUT_SRC, data);
>  		lval = FIELD_GET(LUT_L_VAL, data);
>  		core_count = FIELD_GET(LUT_CORE_COUNT, data);
>  
> -		data = readl_relaxed(drv_data->base + soc_data->reg_volt_lut +
> -				      i * soc_data->lut_row_size);
> +		ret = regmap_read(drv_data->regmap, soc_data->reg_volt_lut +
> +				  i * soc_data->lut_row_size, &data);
> +		if (ret) {
> +			kfree(table);
> +			return ret;
> +		}
> +
>  		volt = FIELD_GET(LUT_VOLT, data) * 1000;
>  
>  		if (src)
> @@ -248,6 +270,13 @@ static void qcom_get_related_cpus(int index, struct cpumask *m)
>  	}
>  }
>  
> +static struct regmap_config qcom_cpufreq_regmap = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.fast_io = true,
> +};
> +
>  static const struct qcom_cpufreq_soc_data qcom_soc_data = {
>  	.reg_enable = 0x0,
>  	.reg_freq_lut = 0x110,
> @@ -274,6 +303,7 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>  	struct qcom_cpufreq_data *data;
>  	const struct of_device_id *match;
>  	int ret, index;
> +	u32 val;
>  
>  	cpu_dev = get_cpu_device(policy->cpu);
>  	if (!cpu_dev) {
> @@ -316,9 +346,18 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>  
>  	data->soc_data = match->data;
>  	data->base = base;
> +	data->regmap = devm_regmap_init_mmio(dev, base, &qcom_cpufreq_regmap);
> +	if (IS_ERR(data->regmap)) {
> +		ret = PTR_ERR(data->regmap);
> +		goto error;
> +	}
>  
>  	/* HW should be in enabled state to proceed */
> -	if (!(readl_relaxed(base + data->soc_data->reg_enable) & 0x1)) {
> +	ret = regmap_read(data->regmap, data->soc_data->reg_enable, &val);
> +	if (ret)
> +		goto error;
> +
> +	if (!(val & 0x1)) {
>  		dev_err(dev, "Domain-%d cpufreq hardware not enabled\n", index);
>  		ret = -ENODEV;
>  		goto error;
> -- 
> 2.17.1

-- 
viresh

Re: [PATCH 7/7] cpufreq: qcom-hw: Use devm_platform_ioremap_resource() to simplify code

[Viresh Kumar]

On 08-09-20, 13:27, Manivannan Sadhasivam wrote:
> devm_platform_ioremap_resource() is the combination of
> platform_get_resource() and devm_ioremap_resource(). Hence, use it to
> simplify the code a bit.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/cpufreq/qcom-cpufreq-hw.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index c3c397cc3dc6..6eeeb2bd4dfa 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -307,7 +307,6 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>  	struct of_phandle_args args;
>  	struct device_node *cpu_np;
>  	struct device *cpu_dev;
> -	struct resource *res;
>  	void __iomem *base;
>  	struct qcom_cpufreq_data *data;
>  	const struct of_device_id *match;
> @@ -333,13 +332,9 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>  
>  	index = args.args[0];
>  
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, index);
> -	if (!res)
> -		return -ENODEV;
> -
> -	base = devm_ioremap(dev, res->start, resource_size(res));
> -	if (!base)
> -		return -ENOMEM;
> +	base = devm_platform_ioremap_resource(pdev, index);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
>  
>  	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>  	if (!data) {

Keep such patches at the top, so they could be independently applied.

-- 
viresh

Re: [PATCH 2/7] arm64: dts: qcom: sm8250: Add cpufreq hw node

[Viresh Kumar]

On 08-09-20, 13:27, Manivannan Sadhasivam wrote:
> From: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> Add cpufreq HW device node to scale 4-Silver/3-Gold/1-Gold+ cores
> on SM8250 SoCs.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sm8250.dtsi | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)

You want this to go through my tree or ARM Soc ?

-- 
viresh

Re: [PATCH 2/7] arm64: dts: qcom: sm8250: Add cpufreq hw node

[Manivannan Sadhasivam]

On 0908, Viresh Kumar wrote:
> On 08-09-20, 13:27, Manivannan Sadhasivam wrote:
> > From: Bjorn Andersson <bjorn.andersson@linaro.org>
> > 
> > Add cpufreq HW device node to scale 4-Silver/3-Gold/1-Gold+ cores
> > on SM8250 SoCs.
> > 
> > Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  arch/arm64/boot/dts/qcom/sm8250.dtsi | 22 ++++++++++++++++++++++
> >  1 file changed, 22 insertions(+)
> 
> You want this to go through my tree or ARM Soc ?
> 

Bjorn will take this through arm-soc. But it would be good if you can provide
the review.

Thanks,
Mani

> -- 
> viresh

Re: [PATCH 5/7] cpufreq: qcom-hw: Use regmap for accessing hardware registers

[Manivannan Sadhasivam]

On 0908, Viresh Kumar wrote:
> On 08-09-20, 13:27, Manivannan Sadhasivam wrote:
> > Use regmap for accessing cpufreq registers in hardware.
> 
> Why ? Please mention why a change is required in the log.
> 

Only because it is recommended to use regmap for abstracting the hw access.
Moreover it handles the proper locking for us in the core (spinlock vs mutex).
I've seen many subsystem maintainers prefer regmap over plain readl/writel
calls. I'll add the reason in commit log.

Thanks,
Mani

> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/cpufreq/qcom-cpufreq-hw.c | 55 ++++++++++++++++++++++++++-----
> >  1 file changed, 47 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> > index 41853db7c9b8..de816bcafd33 100644
> > --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> > @@ -12,6 +12,7 @@
> >  #include <linux/of_address.h>
> >  #include <linux/of_platform.h>
> >  #include <linux/pm_opp.h>
> > +#include <linux/regmap.h>
> >  #include <linux/slab.h>
> >  
> >  #define LUT_MAX_ENTRIES			40U
> > @@ -32,6 +33,7 @@ struct qcom_cpufreq_soc_data {
> >  
> >  struct qcom_cpufreq_data {
> >  	void __iomem *base;
> > +	struct regmap *regmap;
> >  	const struct qcom_cpufreq_soc_data *soc_data;
> >  };
> >  
> > @@ -85,8 +87,11 @@ static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
> >  	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;
> > +	int ret;
> >  
> > -	writel_relaxed(index, data->base + soc_data->reg_perf_state);
> > +	ret = regmap_write(data->regmap, soc_data->reg_perf_state, index);
> > +	if (ret)
> > +		return ret;
> >  
> >  	if (icc_scaling_enabled)
> >  		qcom_cpufreq_set_bw(policy, freq);
> > @@ -102,6 +107,7 @@ static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
> >  	const struct qcom_cpufreq_soc_data *soc_data;
> >  	struct cpufreq_policy *policy;
> >  	unsigned int index;
> > +	int ret;
> >  
> >  	policy = cpufreq_cpu_get_raw(cpu);
> >  	if (!policy)
> > @@ -110,7 +116,10 @@ static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
> >  	data = policy->driver_data;
> >  	soc_data = data->soc_data;
> >  
> > -	index = readl_relaxed(data->base + soc_data->reg_perf_state);
> > +	ret = regmap_read(data->regmap, soc_data->reg_perf_state, &index);
> > +	if (ret)
> > +		return 0;
> > +
> >  	index = min(index, LUT_MAX_ENTRIES - 1);
> >  
> >  	return policy->freq_table[index].frequency;
> > @@ -123,9 +132,12 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
> >  	const struct qcom_cpufreq_soc_data *soc_data = data->soc_data;
> >  	unsigned int index;
> >  	unsigned long freq;
> > +	int ret;
> >  
> >  	index = policy->cached_resolved_idx;
> > -	writel_relaxed(index, data->base + soc_data->reg_perf_state);
> > +	ret = regmap_write(data->regmap, soc_data->reg_perf_state, index);
> > +	if (ret)
> > +		return 0;
> >  
> >  	freq = policy->freq_table[index].frequency;
> >  	arch_set_freq_scale(policy->related_cpus, freq,
> > @@ -171,14 +183,24 @@ static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev,
> >  	}
> >  
> >  	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> > -		data = readl_relaxed(drv_data->base + soc_data->reg_freq_lut +
> > -				      i * soc_data->lut_row_size);
> > +		ret = regmap_read(drv_data->regmap, soc_data->reg_freq_lut +
> > +				  i * soc_data->lut_row_size, &data);
> > +		if (ret) {
> > +			kfree(table);
> > +			return ret;
> > +		}
> > +
> >  		src = FIELD_GET(LUT_SRC, data);
> >  		lval = FIELD_GET(LUT_L_VAL, data);
> >  		core_count = FIELD_GET(LUT_CORE_COUNT, data);
> >  
> > -		data = readl_relaxed(drv_data->base + soc_data->reg_volt_lut +
> > -				      i * soc_data->lut_row_size);
> > +		ret = regmap_read(drv_data->regmap, soc_data->reg_volt_lut +
> > +				  i * soc_data->lut_row_size, &data);
> > +		if (ret) {
> > +			kfree(table);
> > +			return ret;
> > +		}
> > +
> >  		volt = FIELD_GET(LUT_VOLT, data) * 1000;
> >  
> >  		if (src)
> > @@ -248,6 +270,13 @@ static void qcom_get_related_cpus(int index, struct cpumask *m)
> >  	}
> >  }
> >  
> > +static struct regmap_config qcom_cpufreq_regmap = {
> > +	.reg_bits = 32,
> > +	.reg_stride = 4,
> > +	.val_bits = 32,
> > +	.fast_io = true,
> > +};
> > +
> >  static const struct qcom_cpufreq_soc_data qcom_soc_data = {
> >  	.reg_enable = 0x0,
> >  	.reg_freq_lut = 0x110,
> > @@ -274,6 +303,7 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> >  	struct qcom_cpufreq_data *data;
> >  	const struct of_device_id *match;
> >  	int ret, index;
> > +	u32 val;
> >  
> >  	cpu_dev = get_cpu_device(policy->cpu);
> >  	if (!cpu_dev) {
> > @@ -316,9 +346,18 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> >  
> >  	data->soc_data = match->data;
> >  	data->base = base;
> > +	data->regmap = devm_regmap_init_mmio(dev, base, &qcom_cpufreq_regmap);
> > +	if (IS_ERR(data->regmap)) {
> > +		ret = PTR_ERR(data->regmap);
> > +		goto error;
> > +	}
> >  
> >  	/* HW should be in enabled state to proceed */
> > -	if (!(readl_relaxed(base + data->soc_data->reg_enable) & 0x1)) {
> > +	ret = regmap_read(data->regmap, data->soc_data->reg_enable, &val);
> > +	if (ret)
> > +		goto error;
> > +
> > +	if (!(val & 0x1)) {
> >  		dev_err(dev, "Domain-%d cpufreq hardware not enabled\n", index);
> >  		ret = -ENODEV;
> >  		goto error;
> > -- 
> > 2.17.1
> 
> -- 
> viresh

Re: [PATCH 2/7] arm64: dts: qcom: sm8250: Add cpufreq hw node

[Viresh Kumar]

On 08-09-20, 13:27, Manivannan Sadhasivam wrote:
> From: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> Add cpufreq HW device node to scale 4-Silver/3-Gold/1-Gold+ cores
> on SM8250 SoCs.
> 
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  arch/arm64/boot/dts/qcom/sm8250.dtsi | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh

Re: [PATCH 7/7] cpufreq: qcom-hw: Use devm_platform_ioremap_resource() to simplify code

[Manivannan Sadhasivam]

On 0908, Viresh Kumar wrote:
> On 08-09-20, 13:27, Manivannan Sadhasivam wrote:
> > devm_platform_ioremap_resource() is the combination of
> > platform_get_resource() and devm_ioremap_resource(). Hence, use it to
> > simplify the code a bit.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > ---
> >  drivers/cpufreq/qcom-cpufreq-hw.c | 11 +++--------
> >  1 file changed, 3 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> > index c3c397cc3dc6..6eeeb2bd4dfa 100644
> > --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> > @@ -307,7 +307,6 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> >  	struct of_phandle_args args;
> >  	struct device_node *cpu_np;
> >  	struct device *cpu_dev;
> > -	struct resource *res;
> >  	void __iomem *base;
> >  	struct qcom_cpufreq_data *data;
> >  	const struct of_device_id *match;
> > @@ -333,13 +332,9 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> >  
> >  	index = args.args[0];
> >  
> > -	res = platform_get_resource(pdev, IORESOURCE_MEM, index);
> > -	if (!res)
> > -		return -ENODEV;
> > -
> > -	base = devm_ioremap(dev, res->start, resource_size(res));
> > -	if (!base)
> > -		return -ENOMEM;
> > +	base = devm_platform_ioremap_resource(pdev, index);
> > +	if (IS_ERR(base))
> > +		return PTR_ERR(base);
> >  
> >  	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
> >  	if (!data) {
> 
> Keep such patches at the top, so they could be independently applied.
> 

Okay.

Thanks,
Mani

> -- 
> viresh

Re: [PATCH 5/7] cpufreq: qcom-hw: Use regmap for accessing hardware registers

[Viresh Kumar]

On 08-09-20, 16:41, Manivannan Sadhasivam wrote:
> On 0908, Viresh Kumar wrote:
> > On 08-09-20, 13:27, Manivannan Sadhasivam wrote:
> > > Use regmap for accessing cpufreq registers in hardware.
> > 
> > Why ? Please mention why a change is required in the log.
> > 
> 
> Only because it is recommended to use regmap for abstracting the hw access.

Yes it can be very useful in abstracting the hw access in case of
busses like SPI/I2C, others, but in this case there is only one way of
doing it with the exact same registers. I am not sure it is worth it
here. FWIW, I have never played with regmaps personally, and so every
chance I can be wrong here.

> Moreover it handles the proper locking for us in the core (spinlock vs mutex).

What locking do you need here ?

> I've seen many subsystem maintainers prefer regmap over plain readl/writel
> calls. I'll add the reason in commit log.

I am not sure if it is worth it here.

-- 
viresh

Re: [PATCH 5/7] cpufreq: qcom-hw: Use regmap for accessing hardware registers

[Manivannan Sadhasivam]

On 0908, Viresh Kumar wrote:
> On 08-09-20, 16:41, Manivannan Sadhasivam wrote:
> > On 0908, Viresh Kumar wrote:
> > > On 08-09-20, 13:27, Manivannan Sadhasivam wrote:
> > > > Use regmap for accessing cpufreq registers in hardware.
> > > 
> > > Why ? Please mention why a change is required in the log.
> > > 
> > 
> > Only because it is recommended to use regmap for abstracting the hw access.
> 
> Yes it can be very useful in abstracting the hw access in case of
> busses like SPI/I2C, others, but in this case there is only one way of
> doing it with the exact same registers. I am not sure it is worth it
> here. FWIW, I have never played with regmaps personally, and so every
> chance I can be wrong here.
> 
> > Moreover it handles the proper locking for us in the core (spinlock vs mutex).
> 
> What locking do you need here ?
> 

I was just referring the case where if we need the locking in future, regmap
handles it nicely in the core.

> > I've seen many subsystem maintainers prefer regmap over plain readl/writel
> > calls. I'll add the reason in commit log.
> 
> I am not sure if it is worth it here.
> 

Hmm, I thought it is recommended to use regmap for MMIO access as well. I can
drop the patch if you want but let's wait for Bjorn/Amit to get their views.

Thanks,
Mani

> -- 
> viresh

Re: [PATCH 5/7] cpufreq: qcom-hw: Use regmap for accessing hardware registers

[Amit Kucheria]

On Tue, Sep 8, 2020 at 4:48 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
> On 08-09-20, 16:41, Manivannan Sadhasivam wrote:
> > On 0908, Viresh Kumar wrote:
> > > On 08-09-20, 13:27, Manivannan Sadhasivam wrote:
> > > > Use regmap for accessing cpufreq registers in hardware.
> > >
> > > Why ? Please mention why a change is required in the log.
> > >
> >
> > Only because it is recommended to use regmap for abstracting the hw access.
>
> Yes it can be very useful in abstracting the hw access in case of
> busses like SPI/I2C, others, but in this case there is only one way of
> doing it with the exact same registers. I am not sure it is worth it
> here. FWIW, I have never played with regmaps personally, and so every
> chance I can be wrong here.

One could handle the reg offsets through a struct initialisation, but
then you end up with lots of #defines for bitmasks and bits for each
version of the IP. And the core code becomes a bit convoluted IMO,
trying to handle the differences.

regmap hides the differences of the bit positions and register offsets
between several IP versions.

> > Moreover it handles the proper locking for us in the core (spinlock vs mutex).
>
> What locking do you need here ?

Right, locking isn't the main reason here.

>
> > I've seen many subsystem maintainers prefer regmap over plain readl/writel
> > calls. I'll add the reason in commit log.
>
> I am not sure if it is worth it here.

Re: [PATCH 1/7] dt-bindings: cpufreq: cpufreq-qcom-hw: Document SM8250 compatible

[Amit Kucheria]

On Tue, Sep 8, 2020 at 1:27 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> Document the SM8250 SoC specific compatible for Qualcomm Cpufreq HW. The
> hardware block which carries out CPUFreq operations on SM8250 SoC is
> called EPSS.
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Reviewed-by: Amit Kucheria <amitk@kernel.org>


> ---
>  Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> index 33856947c561..aea4ddb2b9e8 100644
> --- a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> @@ -8,7 +8,7 @@ Properties:
>  - compatible
>         Usage:          required
>         Value type:     <string>
> -       Definition:     must be "qcom,cpufreq-hw".
> +       Definition:     must be "qcom,cpufreq-hw" or "qcom,sm8250-epss".
>
>  - clocks
>         Usage:          required
> --
> 2.17.1
>

Re: [PATCH 2/7] arm64: dts: qcom: sm8250: Add cpufreq hw node

[Amit Kucheria]

On Tue, Sep 8, 2020 at 1:27 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> From: Bjorn Andersson <bjorn.andersson@linaro.org>
>
> Add cpufreq HW device node to scale 4-Silver/3-Gold/1-Gold+ cores
> on SM8250 SoCs.
>
> Signed-off-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>


Reviewed-by: Amit Kucheria <amitk@kernel.org>


> ---
>  arch/arm64/boot/dts/qcom/sm8250.dtsi | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sm8250.dtsi b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> index e7d139e1a6ce..aafb46a26a9c 100644
> --- a/arch/arm64/boot/dts/qcom/sm8250.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sm8250.dtsi
> @@ -87,6 +87,7 @@
>                         reg = <0x0 0x0>;
>                         enable-method = "psci";
>                         next-level-cache = <&L2_0>;
> +                       qcom,freq-domain = <&cpufreq_hw 0>;
>                         L2_0: l2-cache {
>                               compatible = "cache";
>                               next-level-cache = <&L3_0>;
> @@ -102,6 +103,7 @@
>                         reg = <0x0 0x100>;
>                         enable-method = "psci";
>                         next-level-cache = <&L2_100>;
> +                       qcom,freq-domain = <&cpufreq_hw 0>;
>                         L2_100: l2-cache {
>                               compatible = "cache";
>                               next-level-cache = <&L3_0>;
> @@ -114,6 +116,7 @@
>                         reg = <0x0 0x200>;
>                         enable-method = "psci";
>                         next-level-cache = <&L2_200>;
> +                       qcom,freq-domain = <&cpufreq_hw 0>;
>                         L2_200: l2-cache {
>                               compatible = "cache";
>                               next-level-cache = <&L3_0>;
> @@ -126,6 +129,7 @@
>                         reg = <0x0 0x300>;
>                         enable-method = "psci";
>                         next-level-cache = <&L2_300>;
> +                       qcom,freq-domain = <&cpufreq_hw 0>;
>                         L2_300: l2-cache {
>                               compatible = "cache";
>                               next-level-cache = <&L3_0>;
> @@ -138,6 +142,7 @@
>                         reg = <0x0 0x400>;
>                         enable-method = "psci";
>                         next-level-cache = <&L2_400>;
> +                       qcom,freq-domain = <&cpufreq_hw 1>;
>                         L2_400: l2-cache {
>                               compatible = "cache";
>                               next-level-cache = <&L3_0>;
> @@ -150,6 +155,7 @@
>                         reg = <0x0 0x500>;
>                         enable-method = "psci";
>                         next-level-cache = <&L2_500>;
> +                       qcom,freq-domain = <&cpufreq_hw 1>;
>                         L2_500: l2-cache {
>                               compatible = "cache";
>                               next-level-cache = <&L3_0>;
> @@ -163,6 +169,7 @@
>                         reg = <0x0 0x600>;
>                         enable-method = "psci";
>                         next-level-cache = <&L2_600>;
> +                       qcom,freq-domain = <&cpufreq_hw 1>;
>                         L2_600: l2-cache {
>                               compatible = "cache";
>                               next-level-cache = <&L3_0>;
> @@ -175,6 +182,7 @@
>                         reg = <0x0 0x700>;
>                         enable-method = "psci";
>                         next-level-cache = <&L2_700>;
> +                       qcom,freq-domain = <&cpufreq_hw 2>;
>                         L2_700: l2-cache {
>                               compatible = "cache";
>                               next-level-cache = <&L3_0>;
> @@ -2076,6 +2084,20 @@
>                                 };
>                         };
>                 };
> +
> +               cpufreq_hw: cpufreq@18591000 {
> +                       compatible = "qcom,sm8250-epss";
> +                       reg = <0 0x18591000 0 0x1000>,
> +                             <0 0x18592000 0 0x1000>,
> +                             <0 0x18593000 0 0x1000>;
> +                       reg-names = "freq-domain0", "freq-domain1",
> +                                   "freq-domain2";
> +
> +                       clocks = <&rpmhcc RPMH_CXO_CLK>, <&gcc GPLL0>;
> +                       clock-names = "xo", "alternate";
> +
> +                       #freq-domain-cells = <1>;
> +               };
>         };
>
>         timer {
> --
> 2.17.1
>

Re: [PATCH 7/7] cpufreq: qcom-hw: Use devm_platform_ioremap_resource() to simplify code

[Amit Kucheria]

On Tue, Sep 8, 2020 at 1:27 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> devm_platform_ioremap_resource() is the combination of
> platform_get_resource() and devm_ioremap_resource(). Hence, use it to
> simplify the code a bit.
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Reviewed-by: Amit Kucheria <amitk@kernel.org>

> ---
>  drivers/cpufreq/qcom-cpufreq-hw.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index c3c397cc3dc6..6eeeb2bd4dfa 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -307,7 +307,6 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>         struct of_phandle_args args;
>         struct device_node *cpu_np;
>         struct device *cpu_dev;
> -       struct resource *res;
>         void __iomem *base;
>         struct qcom_cpufreq_data *data;
>         const struct of_device_id *match;
> @@ -333,13 +332,9 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>
>         index = args.args[0];
>
> -       res = platform_get_resource(pdev, IORESOURCE_MEM, index);
> -       if (!res)
> -               return -ENODEV;
> -
> -       base = devm_ioremap(dev, res->start, resource_size(res));
> -       if (!base)
> -               return -ENOMEM;
> +       base = devm_platform_ioremap_resource(pdev, index);
> +       if (IS_ERR(base))
> +               return PTR_ERR(base);
>
>         data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>         if (!data) {
> --
> 2.17.1
>

Re: [PATCH 5/7] cpufreq: qcom-hw: Use regmap for accessing hardware registers

[Amit Kucheria]

On Tue, Sep 8, 2020 at 5:18 PM Amit Kucheria <amitk@kernel.org> wrote:
>
> On Tue, Sep 8, 2020 at 4:48 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 08-09-20, 16:41, Manivannan Sadhasivam wrote:
> > > On 0908, Viresh Kumar wrote:
> > > > On 08-09-20, 13:27, Manivannan Sadhasivam wrote:
> > > > > Use regmap for accessing cpufreq registers in hardware.
> > > >
> > > > Why ? Please mention why a change is required in the log.
> > > >
> > >
> > > Only because it is recommended to use regmap for abstracting the hw access.
> >
> > Yes it can be very useful in abstracting the hw access in case of
> > busses like SPI/I2C, others, but in this case there is only one way of
> > doing it with the exact same registers. I am not sure it is worth it
> > here. FWIW, I have never played with regmaps personally, and so every
> > chance I can be wrong here.
>
> One could handle the reg offsets through a struct initialisation, but
> then you end up with lots of #defines for bitmasks and bits for each
> version of the IP. And the core code becomes a bit convoluted IMO,
> trying to handle the differences.
>
> regmap hides the differences of the bit positions and register offsets
> between several IP versions.
>
> > > Moreover it handles the proper locking for us in the core (spinlock vs mutex).
> >
> > What locking do you need here ?
>
> Right, locking isn't the main reason here.

Having said this, perhaps this patch can be held back for now, since
we're not yet using some of the features of regmap to abstract away
bit fields and such.

We don't strictly need it for just different register offsets.

Regards,
Amit

Re: [PATCH 6/7] cpufreq: qcom-hw: Add cpufreq support for SM8250 SoC

[Amit Kucheria]

On Tue, Sep 8, 2020 at 1:27 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> SM8250 SoC uses EPSS block for carrying out the cpufreq duties. Hence, add
> support for it in the driver with relevant of_match data.
>
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Reviewed-by: Amit Kucheria <amitk@kernel.org>


> ---
>  drivers/cpufreq/qcom-cpufreq-hw.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index de816bcafd33..c3c397cc3dc6 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -285,8 +285,17 @@ static const struct qcom_cpufreq_soc_data qcom_soc_data = {
>         .lut_row_size = 32,
>  };
>
> +static const struct qcom_cpufreq_soc_data sm8250_soc_data = {
> +       .reg_enable = 0x0,
> +       .reg_freq_lut = 0x100,
> +       .reg_volt_lut = 0x200,
> +       .reg_perf_state = 0x320,
> +       .lut_row_size = 4,
> +};
> +
>  static const struct of_device_id qcom_cpufreq_hw_match[] = {
>         { .compatible = "qcom,cpufreq-hw", .data = &qcom_soc_data },
> +       { .compatible = "qcom,sm8250-epss", .data = &sm8250_soc_data },
>         {}
>  };
>  MODULE_DEVICE_TABLE(of, qcom_cpufreq_hw_match);
> --
> 2.17.1
>

Re: [PATCH 4/7] cpufreq: qcom-hw: Make use of of_match data for offsets and row size

[Amit Kucheria]

On Tue, Sep 8, 2020 at 1:27 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> 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 = {

rename this to sdm845_soc_data?

Or even better, maybe just use the IP version number for this IP block
so that all SoCs using that IP version can use this struct?

> +       .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);
> +       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);
>
>         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
>

Re: [PATCH 5/7] cpufreq: qcom-hw: Use regmap for accessing hardware registers

[Sudeep Holla]

On Tue, Sep 08, 2020 at 05:18:35PM +0530, Amit Kucheria wrote:
> On Tue, Sep 8, 2020 at 4:48 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> >
> > On 08-09-20, 16:41, Manivannan Sadhasivam wrote:
> > > On 0908, Viresh Kumar wrote:
> > > > On 08-09-20, 13:27, Manivannan Sadhasivam wrote:
> > > > > Use regmap for accessing cpufreq registers in hardware.
> > > >
> > > > Why ? Please mention why a change is required in the log.
> > > >
> > >
> > > Only because it is recommended to use regmap for abstracting the hw access.
> >
> > Yes it can be very useful in abstracting the hw access in case of
> > busses like SPI/I2C, others, but in this case there is only one way of
> > doing it with the exact same registers. I am not sure it is worth it
> > here. FWIW, I have never played with regmaps personally, and so every
> > chance I can be wrong here.
> 
> One could handle the reg offsets through a struct initialisation, but
> then you end up with lots of #defines for bitmasks and bits for each
> version of the IP. And the core code becomes a bit convoluted IMO,
> trying to handle the differences.
> 
> regmap hides the differences of the bit positions and register offsets
> between several IP versions.
> 
> > > Moreover it handles the proper locking for us in the core (spinlock vs mutex).
> >
> > What locking do you need here ?
> 
> Right, locking isn't the main reason here.

If that is the case, IMO it is better to set disable_lock or something
in config especially as this regmap_write is used in qcom_cpufreq_hw_fast_switch

-- 
Regards,
Sudeep

Re: [PATCH 1/7] dt-bindings: cpufreq: cpufreq-qcom-hw: Document SM8250 compatible

[Bjorn Andersson]

On Tue 08 Sep 02:57 CDT 2020, Manivannan Sadhasivam wrote:

> Document the SM8250 SoC specific compatible for Qualcomm Cpufreq HW. The
> hardware block which carries out CPUFreq operations on SM8250 SoC is
> called EPSS.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

Please follow up, after this has been accepted, with a conversion of
this binding to yaml.

Regards,
Bjorn

> ---
>  Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> index 33856947c561..aea4ddb2b9e8 100644
> --- a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> @@ -8,7 +8,7 @@ Properties:
>  - compatible
>  	Usage:		required
>  	Value type:	<string>
> -	Definition:	must be "qcom,cpufreq-hw".
> +	Definition:	must be "qcom,cpufreq-hw" or "qcom,sm8250-epss".
>  
>  - clocks
>  	Usage:		required
> -- 
> 2.17.1
> 

Re: [PATCH 5/7] cpufreq: qcom-hw: Use regmap for accessing hardware registers

[Manivannan Sadhasivam]

On 0908, Amit Kucheria wrote:
> On Tue, Sep 8, 2020 at 5:18 PM Amit Kucheria <amitk@kernel.org> wrote:
> >
> > On Tue, Sep 8, 2020 at 4:48 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > On 08-09-20, 16:41, Manivannan Sadhasivam wrote:
> > > > On 0908, Viresh Kumar wrote:
> > > > > On 08-09-20, 13:27, Manivannan Sadhasivam wrote:
> > > > > > Use regmap for accessing cpufreq registers in hardware.
> > > > >
> > > > > Why ? Please mention why a change is required in the log.
> > > > >
> > > >
> > > > Only because it is recommended to use regmap for abstracting the hw access.
> > >
> > > Yes it can be very useful in abstracting the hw access in case of
> > > busses like SPI/I2C, others, but in this case there is only one way of
> > > doing it with the exact same registers. I am not sure it is worth it
> > > here. FWIW, I have never played with regmaps personally, and so every
> > > chance I can be wrong here.
> >
> > One could handle the reg offsets through a struct initialisation, but
> > then you end up with lots of #defines for bitmasks and bits for each
> > version of the IP. And the core code becomes a bit convoluted IMO,
> > trying to handle the differences.
> >
> > regmap hides the differences of the bit positions and register offsets
> > between several IP versions.
> >
> > > > Moreover it handles the proper locking for us in the core (spinlock vs mutex).
> > >
> > > What locking do you need here ?
> >
> > Right, locking isn't the main reason here.
> 
> Having said this, perhaps this patch can be held back for now, since
> we're not yet using some of the features of regmap to abstract away
> bit fields and such.
> 

Okay. Dropping this patch for now (in v2)!

Thanks,
Mani

> We don't strictly need it for just different register offsets.
> 
> Regards,
> Amit

Re: [PATCH 4/7] cpufreq: qcom-hw: Make use of of_match data for offsets and row size

[Manivannan Sadhasivam]

On 0908, Amit Kucheria wrote:
> On Tue, Sep 8, 2020 at 1:27 PM Manivannan Sadhasivam
> <manivannan.sadhasivam@linaro.org> 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 = {
> 
> rename this to sdm845_soc_data?
> 

Nah, this is not specific to SDM845. Atleast in mainline, there are 3 SoCs
using this compatible.

> Or even better, maybe just use the IP version number for this IP block
> so that all SoCs using that IP version can use this struct?
> 

Since the SoCs are using the same compatible it makes sense to use the same
name for the of_data. I don't think it is a good idea to use different name
for the of_data since the differentiation has to happen at compatible level.

Thanks,
Mani

> > +       .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);
> > +       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);
> >
> >         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
> >

Re: [PATCH 1/7] dt-bindings: cpufreq: cpufreq-qcom-hw: Document SM8250 compatible

[Manivannan Sadhasivam]

On 0908, Bjorn Andersson wrote:
> On Tue 08 Sep 02:57 CDT 2020, Manivannan Sadhasivam wrote:
> 
> > Document the SM8250 SoC specific compatible for Qualcomm Cpufreq HW. The
> > hardware block which carries out CPUFreq operations on SM8250 SoC is
> > called EPSS.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> Please follow up, after this has been accepted, with a conversion of
> this binding to yaml.
> 

Sure.

Thanks,
Mani

> Regards,
> Bjorn
> 
> > ---
> >  Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> > index 33856947c561..aea4ddb2b9e8 100644
> > --- a/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> > +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt
> > @@ -8,7 +8,7 @@ Properties:
> >  - compatible
> >  	Usage:		required
> >  	Value type:	<string>
> > -	Definition:	must be "qcom,cpufreq-hw".
> > +	Definition:	must be "qcom,cpufreq-hw" or "qcom,sm8250-epss".
> >  
> >  - clocks
> >  	Usage:		required
> > -- 
> > 2.17.1
> > 

Re: [PATCH 6/7] cpufreq: qcom-hw: Add cpufreq support for SM8250 SoC

[Bjorn Andersson]

On Tue 08 Sep 02:57 CDT 2020, Manivannan Sadhasivam wrote:

> SM8250 SoC uses EPSS block for carrying out the cpufreq duties. Hence, add
> support for it in the driver with relevant of_match data.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> ---
>  drivers/cpufreq/qcom-cpufreq-hw.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index de816bcafd33..c3c397cc3dc6 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -285,8 +285,17 @@ static const struct qcom_cpufreq_soc_data qcom_soc_data = {
>  	.lut_row_size = 32,
>  };
>  
> +static const struct qcom_cpufreq_soc_data sm8250_soc_data = {

Could it be that this is the "epss_soc_data" (i.e. not sm8250 specific)?
(We should still use/include the platform specific compatible though).

Regards,
Bjorn

> +	.reg_enable = 0x0,
> +	.reg_freq_lut = 0x100,
> +	.reg_volt_lut = 0x200,
> +	.reg_perf_state = 0x320,
> +	.lut_row_size = 4,
> +};
> +
>  static const struct of_device_id qcom_cpufreq_hw_match[] = {
>  	{ .compatible = "qcom,cpufreq-hw", .data = &qcom_soc_data },
> +	{ .compatible = "qcom,sm8250-epss", .data = &sm8250_soc_data },
>  	{}
>  };
>  MODULE_DEVICE_TABLE(of, qcom_cpufreq_hw_match);
> -- 
> 2.17.1
> 

Re: [PATCH 6/7] cpufreq: qcom-hw: Add cpufreq support for SM8250 SoC

[Manivannan Sadhasivam]

On 0908, Bjorn Andersson wrote:
> On Tue 08 Sep 02:57 CDT 2020, Manivannan Sadhasivam wrote:
> 
> > SM8250 SoC uses EPSS block for carrying out the cpufreq duties. Hence, add
> > support for it in the driver with relevant of_match data.
> > 
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> 
> Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>
> 
> > ---
> >  drivers/cpufreq/qcom-cpufreq-hw.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> > index de816bcafd33..c3c397cc3dc6 100644
> > --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> > +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> > @@ -285,8 +285,17 @@ static const struct qcom_cpufreq_soc_data qcom_soc_data = {
> >  	.lut_row_size = 32,
> >  };
> >  
> > +static const struct qcom_cpufreq_soc_data sm8250_soc_data = {
> 
> Could it be that this is the "epss_soc_data" (i.e. not sm8250 specific)?
> (We should still use/include the platform specific compatible though).
> 

Hmm, makes sense. Will change it.

Thanks,
Mani

> Regards,
> Bjorn
> 
> > +	.reg_enable = 0x0,
> > +	.reg_freq_lut = 0x100,
> > +	.reg_volt_lut = 0x200,
> > +	.reg_perf_state = 0x320,
> > +	.lut_row_size = 4,
> > +};
> > +
> >  static const struct of_device_id qcom_cpufreq_hw_match[] = {
> >  	{ .compatible = "qcom,cpufreq-hw", .data = &qcom_soc_data },
> > +	{ .compatible = "qcom,sm8250-epss", .data = &sm8250_soc_data },
> >  	{}
> >  };
> >  MODULE_DEVICE_TABLE(of, qcom_cpufreq_hw_match);
> > -- 
> > 2.17.1
> > 

Re: [PATCH 4/7] cpufreq: qcom-hw: Make use of of_match data for offsets and row size

[Bjorn Andersson]

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
> 

Re: [PATCH 7/7] cpufreq: qcom-hw: Use devm_platform_ioremap_resource() to simplify code

[Bjorn Andersson]

On Tue 08 Sep 02:57 CDT 2020, Manivannan Sadhasivam wrote:

> devm_platform_ioremap_resource() is the combination of
> platform_get_resource() and devm_ioremap_resource(). Hence, use it to
> simplify the code a bit.
> 

Reviewed-by: Bjorn Andersson <bjorn.andersson@linaro.org>

> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/cpufreq/qcom-cpufreq-hw.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index c3c397cc3dc6..6eeeb2bd4dfa 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -307,7 +307,6 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>  	struct of_phandle_args args;
>  	struct device_node *cpu_np;
>  	struct device *cpu_dev;
> -	struct resource *res;
>  	void __iomem *base;
>  	struct qcom_cpufreq_data *data;
>  	const struct of_device_id *match;
> @@ -333,13 +332,9 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>  
>  	index = args.args[0];
>  
> -	res = platform_get_resource(pdev, IORESOURCE_MEM, index);
> -	if (!res)
> -		return -ENODEV;
> -
> -	base = devm_ioremap(dev, res->start, resource_size(res));
> -	if (!base)
> -		return -ENOMEM;
> +	base = devm_platform_ioremap_resource(pdev, index);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
>  
>  	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>  	if (!data) {
> -- 
> 2.17.1
> 

Re: [PATCH 5/7] cpufreq: qcom-hw: Use regmap for accessing hardware registers

[Bjorn Andersson]

On Tue 08 Sep 02:57 CDT 2020, Manivannan Sadhasivam wrote:

> Use regmap for accessing cpufreq registers in hardware.
> 

The content of the patch looks good, but in itself I don't see the
reason for migrating to regmap.

If you have subsequent patches, that would benefit from describing the
hardware differences using reg_fields then it might be a good idea, but
I would suggest that you postpone this patch until there's an actual
beneficiary.

Regards,
Bjorn

> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  drivers/cpufreq/qcom-cpufreq-hw.c | 55 ++++++++++++++++++++++++++-----
>  1 file changed, 47 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/cpufreq/qcom-cpufreq-hw.c b/drivers/cpufreq/qcom-cpufreq-hw.c
> index 41853db7c9b8..de816bcafd33 100644
> --- a/drivers/cpufreq/qcom-cpufreq-hw.c
> +++ b/drivers/cpufreq/qcom-cpufreq-hw.c
> @@ -12,6 +12,7 @@
>  #include <linux/of_address.h>
>  #include <linux/of_platform.h>
>  #include <linux/pm_opp.h>
> +#include <linux/regmap.h>
>  #include <linux/slab.h>
>  
>  #define LUT_MAX_ENTRIES			40U
> @@ -32,6 +33,7 @@ struct qcom_cpufreq_soc_data {
>  
>  struct qcom_cpufreq_data {
>  	void __iomem *base;
> +	struct regmap *regmap;
>  	const struct qcom_cpufreq_soc_data *soc_data;
>  };
>  
> @@ -85,8 +87,11 @@ static int qcom_cpufreq_hw_target_index(struct cpufreq_policy *policy,
>  	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;
> +	int ret;
>  
> -	writel_relaxed(index, data->base + soc_data->reg_perf_state);
> +	ret = regmap_write(data->regmap, soc_data->reg_perf_state, index);
> +	if (ret)
> +		return ret;
>  
>  	if (icc_scaling_enabled)
>  		qcom_cpufreq_set_bw(policy, freq);
> @@ -102,6 +107,7 @@ static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
>  	const struct qcom_cpufreq_soc_data *soc_data;
>  	struct cpufreq_policy *policy;
>  	unsigned int index;
> +	int ret;
>  
>  	policy = cpufreq_cpu_get_raw(cpu);
>  	if (!policy)
> @@ -110,7 +116,10 @@ static unsigned int qcom_cpufreq_hw_get(unsigned int cpu)
>  	data = policy->driver_data;
>  	soc_data = data->soc_data;
>  
> -	index = readl_relaxed(data->base + soc_data->reg_perf_state);
> +	ret = regmap_read(data->regmap, soc_data->reg_perf_state, &index);
> +	if (ret)
> +		return 0;
> +
>  	index = min(index, LUT_MAX_ENTRIES - 1);
>  
>  	return policy->freq_table[index].frequency;
> @@ -123,9 +132,12 @@ static unsigned int qcom_cpufreq_hw_fast_switch(struct cpufreq_policy *policy,
>  	const struct qcom_cpufreq_soc_data *soc_data = data->soc_data;
>  	unsigned int index;
>  	unsigned long freq;
> +	int ret;
>  
>  	index = policy->cached_resolved_idx;
> -	writel_relaxed(index, data->base + soc_data->reg_perf_state);
> +	ret = regmap_write(data->regmap, soc_data->reg_perf_state, index);
> +	if (ret)
> +		return 0;
>  
>  	freq = policy->freq_table[index].frequency;
>  	arch_set_freq_scale(policy->related_cpus, freq,
> @@ -171,14 +183,24 @@ static int qcom_cpufreq_hw_read_lut(struct device *cpu_dev,
>  	}
>  
>  	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> -		data = readl_relaxed(drv_data->base + soc_data->reg_freq_lut +
> -				      i * soc_data->lut_row_size);
> +		ret = regmap_read(drv_data->regmap, soc_data->reg_freq_lut +
> +				  i * soc_data->lut_row_size, &data);
> +		if (ret) {
> +			kfree(table);
> +			return ret;
> +		}
> +
>  		src = FIELD_GET(LUT_SRC, data);
>  		lval = FIELD_GET(LUT_L_VAL, data);
>  		core_count = FIELD_GET(LUT_CORE_COUNT, data);
>  
> -		data = readl_relaxed(drv_data->base + soc_data->reg_volt_lut +
> -				      i * soc_data->lut_row_size);
> +		ret = regmap_read(drv_data->regmap, soc_data->reg_volt_lut +
> +				  i * soc_data->lut_row_size, &data);
> +		if (ret) {
> +			kfree(table);
> +			return ret;
> +		}
> +
>  		volt = FIELD_GET(LUT_VOLT, data) * 1000;
>  
>  		if (src)
> @@ -248,6 +270,13 @@ static void qcom_get_related_cpus(int index, struct cpumask *m)
>  	}
>  }
>  
> +static struct regmap_config qcom_cpufreq_regmap = {
> +	.reg_bits = 32,
> +	.reg_stride = 4,
> +	.val_bits = 32,
> +	.fast_io = true,
> +};
> +
>  static const struct qcom_cpufreq_soc_data qcom_soc_data = {
>  	.reg_enable = 0x0,
>  	.reg_freq_lut = 0x110,
> @@ -274,6 +303,7 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>  	struct qcom_cpufreq_data *data;
>  	const struct of_device_id *match;
>  	int ret, index;
> +	u32 val;
>  
>  	cpu_dev = get_cpu_device(policy->cpu);
>  	if (!cpu_dev) {
> @@ -316,9 +346,18 @@ static int qcom_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
>  
>  	data->soc_data = match->data;
>  	data->base = base;
> +	data->regmap = devm_regmap_init_mmio(dev, base, &qcom_cpufreq_regmap);
> +	if (IS_ERR(data->regmap)) {
> +		ret = PTR_ERR(data->regmap);
> +		goto error;
> +	}
>  
>  	/* HW should be in enabled state to proceed */
> -	if (!(readl_relaxed(base + data->soc_data->reg_enable) & 0x1)) {
> +	ret = regmap_read(data->regmap, data->soc_data->reg_enable, &val);
> +	if (ret)
> +		goto error;
> +
> +	if (!(val & 0x1)) {
>  		dev_err(dev, "Domain-%d cpufreq hardware not enabled\n", index);
>  		ret = -ENODEV;
>  		goto error;
> -- 
> 2.17.1
> 

Re: [PATCH 4/7] cpufreq: qcom-hw: Make use of of_match data for offsets and row size

[Amit Kucheria]

On Tue, Sep 8, 2020 at 8:40 PM Manivannan Sadhasivam
<manivannan.sadhasivam@linaro.org> wrote:
>
> On 0908, Amit Kucheria wrote:
> > On Tue, Sep 8, 2020 at 1:27 PM Manivannan Sadhasivam
> > <manivannan.sadhasivam@linaro.org> 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 = {
> >
> > rename this to sdm845_soc_data?
> >
>
> Nah, this is not specific to SDM845. Atleast in mainline, there are 3 SoCs
> using this compatible.
>
> > Or even better, maybe just use the IP version number for this IP block
> > so that all SoCs using that IP version can use this struct?
> >
>
> Since the SoCs are using the same compatible it makes sense to use the same
> name for the of_data. I don't think it is a good idea to use different name
> for the of_data since the differentiation has to happen at compatible level.

You are using the name sm8250_soc_data in a subsequent patch, though ;-)

So I think it would make sense for compatible "qcom,cpufreq-hw" to use
data "osm_soc_data" and compatible "qcom,sm8250-epss" to use data
"epss_soc_data" as suggested by Bjorn.

Regards,
Amit


>
> > > +       .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 },
> > > +       {}
> > > +};

Re: [PATCH 5/7] cpufreq: qcom-hw: Use regmap for accessing hardware registers

[Viresh Kumar]

On 08-09-20, 17:38, Amit Kucheria wrote:
> On Tue, Sep 8, 2020 at 5:18 PM Amit Kucheria <amitk@kernel.org> wrote:
> >
> > On Tue, Sep 8, 2020 at 4:48 PM Viresh Kumar <viresh.kumar@linaro.org> wrote:
> > >
> > > On 08-09-20, 16:41, Manivannan Sadhasivam wrote:
> > > > On 0908, Viresh Kumar wrote:
> > > > > On 08-09-20, 13:27, Manivannan Sadhasivam wrote:
> > > > > > Use regmap for accessing cpufreq registers in hardware.
> > > > >
> > > > > Why ? Please mention why a change is required in the log.
> > > > >
> > > >
> > > > Only because it is recommended to use regmap for abstracting the hw access.
> > >
> > > Yes it can be very useful in abstracting the hw access in case of
> > > busses like SPI/I2C, others, but in this case there is only one way of
> > > doing it with the exact same registers. I am not sure it is worth it
> > > here. FWIW, I have never played with regmaps personally, and so every
> > > chance I can be wrong here.
> >
> > One could handle the reg offsets through a struct initialisation, but
> > then you end up with lots of #defines for bitmasks and bits for each
> > version of the IP. And the core code becomes a bit convoluted IMO,
> > trying to handle the differences.
> >
> > regmap hides the differences of the bit positions and register offsets
> > between several IP versions.

Right and I agree that is another useful aspect of it which I missed
mentioning.

> > > > Moreover it handles the proper locking for us in the core (spinlock vs mutex).
> > >
> > > What locking do you need here ?
> >
> > Right, locking isn't the main reason here.
> 
> Having said this, perhaps this patch can be held back for now, since
> we're not yet using some of the features of regmap to abstract away
> bit fields and such.
> 
> We don't strictly need it for just different register offsets.

Right, I just didn't understood why it was required currently as it
wasn't all that complex at all.

-- 
viresh

Re: [PATCH 1/7] dt-bindings: cpufreq: cpufreq-qcom-hw: Document SM8250 compatible

[Rob Herring]

On Tue, 08 Sep 2020 13:27:10 +0530, Manivannan Sadhasivam wrote:
> Document the SM8250 SoC specific compatible for Qualcomm Cpufreq HW. The
> hardware block which carries out CPUFreq operations on SM8250 SoC is
> called EPSS.
> 
> Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> ---
>  Documentation/devicetree/bindings/cpufreq/cpufreq-qcom-hw.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 

Acked-by: Rob Herring <robh@kernel.org>