[PATCH v1] cpufreq: mediatek-hw: Add support for Mediatek cpufreq HW driver

[PATCH v1] cpufreq: mediatek-hw: Add support for Mediatek cpufreq HW driver

[Hector Yuan]

The CPUfreq HW present in some Mediatek chipsets offloads
the steps necessary for changing the frequency of CPUs.
The driver implements the cpufreq driver interface for
this hardware engine. 

This patch depends on the MT6779 DTS patch submitted by Hanks Chen
 https://lkml.org/lkml/2020/8/4/1094


Hector.Yuan (2):
  dt-bindings: cpufreq: add bindings for MediaTek cpufreq HW
  cpufreq: mediatek-hw: Add support for Mediatek cpufreq HW driver

 .../bindings/cpufreq/cpufreq-mediatek-hw.yaml      |   61 +++++
 arch/arm64/configs/defconfig                       |    1 +
 drivers/cpufreq/Kconfig.arm                        |   11 +
 drivers/cpufreq/Makefile                           |    1 +
 drivers/cpufreq/mediatek-cpufreq-hw.c              |  255 ++++++++++++++++++++
 5 files changed, 329 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek-hw.yaml
 create mode 100644 drivers/cpufreq/mediatek-cpufreq-hw.c

[PATCH v2 1/2] cpufreq: mediatek-hw: Add support for Mediatek cpufreq HW driver

[Hector Yuan]

From: "Hector.Yuan" <hector.yuan@mediatek.com>

Add MT6779 cpufreq HW support.

Signed-off-by: Hector.Yuan <hector.yuan@mediatek.com>
---
 arch/arm64/configs/defconfig          |    1 +
 drivers/cpufreq/Kconfig.arm           |   11 ++
 drivers/cpufreq/Makefile              |    1 +
 drivers/cpufreq/mediatek-cpufreq-hw.c |  255 +++++++++++++++++++++++++++++++++
 4 files changed, 268 insertions(+)
 create mode 100644 drivers/cpufreq/mediatek-cpufreq-hw.c

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 883e8ba..866a1bf 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -86,6 +86,7 @@ CONFIG_CPUFREQ_DT=y
 CONFIG_ACPI_CPPC_CPUFREQ=m
 CONFIG_ARM_ALLWINNER_SUN50I_CPUFREQ_NVMEM=m
 CONFIG_ARM_ARMADA_37XX_CPUFREQ=y
+CONFIG_ARM_MEDIATEK_CPUFREQ_HW=m
 CONFIG_ARM_SCPI_CPUFREQ=y
 CONFIG_ARM_IMX_CPUFREQ_DT=m
 CONFIG_ARM_QCOM_CPUFREQ_NVMEM=y
diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index c6cbfc8..81f1cc1 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -121,6 +121,17 @@ config ARM_MEDIATEK_CPUFREQ
 	help
 	  This adds the CPUFreq driver support for MediaTek SoCs.
 
+config ARM_MEDIATEK_CPUFREQ_HW
+	tristate "MediaTek CPUFreq HW driver"
+	depends on ARCH_MEDIATEK || COMPILE_TEST
+	help
+	  Support for the CPUFreq HW driver.
+	  Some MediaTek chipsets have a HW engine to offload the steps
+	  necessary for changing the frequency of the CPUs. Firmware loaded
+	  in this engine exposes a programming interface to the OS.
+	  The driver implements the cpufreq interface for this HW engine.
+	  Say Y if you want to support CPUFreq HW.
+
 config ARM_OMAP2PLUS_CPUFREQ
 	bool "TI OMAP2+"
 	depends on ARCH_OMAP2PLUS
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index f6670c4..dc1f371 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -57,6 +57,7 @@ obj-$(CONFIG_ARM_IMX6Q_CPUFREQ)		+= imx6q-cpufreq.o
 obj-$(CONFIG_ARM_IMX_CPUFREQ_DT)	+= imx-cpufreq-dt.o
 obj-$(CONFIG_ARM_KIRKWOOD_CPUFREQ)	+= kirkwood-cpufreq.o
 obj-$(CONFIG_ARM_MEDIATEK_CPUFREQ)	+= mediatek-cpufreq.o
+obj-$(CONFIG_ARM_MEDIATEK_CPUFREQ_HW)	+= mediatek-cpufreq-hw.o
 obj-$(CONFIG_MACH_MVEBU_V7)		+= mvebu-cpufreq.o
 obj-$(CONFIG_ARM_OMAP2PLUS_CPUFREQ)	+= omap-cpufreq.o
 obj-$(CONFIG_ARM_PXA2xx_CPUFREQ)	+= pxa2xx-cpufreq.o
diff --git a/drivers/cpufreq/mediatek-cpufreq-hw.c b/drivers/cpufreq/mediatek-cpufreq-hw.c
new file mode 100644
index 0000000..6752db9
--- /dev/null
+++ b/drivers/cpufreq/mediatek-cpufreq-hw.c
@@ -0,0 +1,255 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (c) 2020 MediaTek Inc.
+ */
+
+#include <linux/bitfield.h>
+#include <linux/cpufreq.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/of_address.h>
+#include <linux/of_platform.h>
+#include <linux/pm_opp.h>
+#include <linux/slab.h>
+
+#define LUT_MAX_ENTRIES			32U
+#define LUT_FREQ			GENMASK(11, 0)
+#define LUT_VOLT			GENMASK(28, 12)
+#define LUT_ROW_SIZE			0x4
+
+/* Register offsets */
+#define REG_ENABLE			0x84
+#define REG_PERF_STATE		0x88
+
+static struct platform_device *global_pdev;
+static int mtk_cpufreq_hw_target_index(struct cpufreq_policy *policy,
+				       unsigned int index)
+{
+	void __iomem *perf_state_reg = policy->driver_data;
+	unsigned long freq = policy->freq_table[index].frequency;
+
+	writel_relaxed(index, perf_state_reg);
+	arch_set_freq_scale(policy->related_cpus, freq,
+			    policy->cpuinfo.max_freq);
+	return 0;
+}
+
+static unsigned int mtk_cpufreq_hw_get(unsigned int cpu)
+{
+	void __iomem *perf_state_reg;
+	struct cpufreq_policy *policy;
+	unsigned int index;
+
+	policy = cpufreq_cpu_get_raw(cpu);
+	if (!policy)
+		return 0;
+
+	perf_state_reg = policy->driver_data;
+
+	index = readl_relaxed(perf_state_reg);
+	index = min(index, LUT_MAX_ENTRIES - 1);
+
+	return policy->freq_table[index].frequency;
+}
+
+static int mtk_cpufreq_hw_read_lut(struct device *cpu_dev,
+				   struct cpufreq_policy *policy,
+				   void __iomem *base)
+{
+	u32 data;
+	u32 freq, volt, prev_freq = 0;
+	int i = 0;
+	struct cpufreq_frequency_table	*table;
+
+	table = kcalloc(LUT_MAX_ENTRIES + 1, sizeof(*table), GFP_KERNEL);
+	if (!table)
+		return -ENOMEM;
+
+	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
+		data = readl_relaxed(base + (i * LUT_ROW_SIZE));
+		freq = FIELD_GET(LUT_FREQ, data) * 1000;
+		volt = FIELD_GET(LUT_VOLT, data);
+		if (freq != prev_freq) {
+			table[i].frequency = freq;
+			dev_pm_opp_add(cpu_dev, freq * 1000, volt);
+			dev_dbg(cpu_dev, "index=%d freq=%d, volt=%d\n", i,
+				freq, volt);
+		} else {
+			break;
+		}
+		prev_freq = freq;
+	}
+	table[i].frequency = CPUFREQ_TABLE_END;
+	policy->freq_table = table;
+	dev_pm_opp_set_sharing_cpus(cpu_dev, policy->cpus);
+
+	return 0;
+}
+
+static void mtk_get_related_cpus(int index, struct cpumask *m)
+{
+	struct device_node *cpu_np;
+	struct of_phandle_args args;
+	int cpu, ret;
+
+	for_each_possible_cpu(cpu) {
+		cpu_np = of_cpu_device_node_get(cpu);
+		if (!cpu_np)
+			continue;
+
+		ret = of_parse_phandle_with_args(cpu_np, "mtk,freq-domain",
+						 "#freq-domain-cells", 0,
+						 &args);
+		of_node_put(cpu_np);
+		if (ret < 0)
+			continue;
+
+		if (index == args.args[0])
+			cpumask_set_cpu(cpu, m);
+	}
+}
+
+static int mtk_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
+{
+	struct device *dev = &global_pdev->dev;
+	struct of_phandle_args args;
+	struct device_node *cpu_np;
+	struct device *cpu_dev;
+	struct resource *res;
+	void __iomem *base;
+	int ret, index;
+
+	cpu_dev = get_cpu_device(policy->cpu);
+	if (!cpu_dev) {
+		pr_err("%s: failed to get cpu%d device\n", __func__,
+		       policy->cpu);
+		return -ENODEV;
+	}
+
+	cpu_np = of_cpu_device_node_get(policy->cpu);
+	if (!cpu_np)
+		return -EINVAL;
+
+	ret = of_parse_phandle_with_args(cpu_np, "mtk,freq-domain",
+					 "#freq-domain-cells", 0, &args);
+	of_node_put(cpu_np);
+	if (ret)
+		return ret;
+
+	index = args.args[0];
+	res = platform_get_resource(global_pdev, IORESOURCE_MEM, index);
+	if (!res)
+		return -ENODEV;
+	base = devm_ioremap(dev, res->start, resource_size(res));
+	if (!base)
+		return -ENOMEM;
+
+	mtk_get_related_cpus(index, policy->cpus);
+	if (!cpumask_weight(policy->cpus)) {
+		dev_err(dev, "Domain-%d failed to get related CPUs\n", index);
+		ret = -ENOENT;
+		goto error;
+	}
+
+	policy->driver_data = base + REG_PERF_STATE;
+	ret = mtk_cpufreq_hw_read_lut(cpu_dev, policy, base);
+	if (ret) {
+		dev_err(dev, "Domain-%d failed to read LUT\n", index);
+		goto error;
+	}
+
+	ret = dev_pm_opp_get_opp_count(cpu_dev);
+	if (ret <= 0) {
+		dev_err(cpu_dev, "Failed to add OPPs\n");
+		ret = -ENODEV;
+		goto error;
+	}
+
+	dev_pm_opp_of_register_em(policy->cpus);
+
+	/* HW should be in enabled state to proceed now */
+	writel_relaxed(0x1, (base + REG_ENABLE));
+	return 0;
+error:
+	devm_iounmap(dev, base);
+	return ret;
+}
+
+static int mtk_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;
+
+	dev_pm_opp_remove_all_dynamic(cpu_dev);
+	kfree(policy->freq_table);
+	devm_iounmap(&global_pdev->dev, base);
+
+	return 0;
+}
+
+static struct freq_attr *mtk_cpufreq_hw_attr[] = {
+	&cpufreq_freq_attr_scaling_available_freqs,
+	NULL
+};
+
+static struct cpufreq_driver cpufreq_mtk_hw_driver = {
+	.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
+			  CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
+	.verify		= cpufreq_generic_frequency_table_verify,
+	.target_index	= mtk_cpufreq_hw_target_index,
+	.get		= mtk_cpufreq_hw_get,
+	.init		= mtk_cpufreq_hw_cpu_init,
+	.exit		= mtk_cpufreq_hw_cpu_exit,
+	.name		= "mtk-cpufreq-hw",
+	.attr		= mtk_cpufreq_hw_attr,
+};
+
+static int mtk_cpufreq_hw_driver_probe(struct platform_device *pdev)
+{
+	int ret;
+
+	global_pdev = pdev;
+	ret = cpufreq_register_driver(&cpufreq_mtk_hw_driver);
+	if (ret)
+		dev_err(&pdev->dev, "CPUFreq HW driver failed to register\n");
+	else
+		dev_dbg(&pdev->dev, "mtk CPUFreq HW driver initialized\n");
+
+	return ret;
+}
+
+static int mtk_cpufreq_hw_driver_remove(struct platform_device *pdev)
+{
+	return cpufreq_unregister_driver(&cpufreq_mtk_hw_driver);
+}
+
+static const struct of_device_id mtk_cpufreq_hw_match[] = {
+	{ .compatible = "mediatek,cpufreq-hw" },
+	{}
+};
+MODULE_DEVICE_TABLE(of, mtk_cpufreq_hw_match);
+
+static struct platform_driver mtk_cpufreq_hw_driver = {
+	.probe = mtk_cpufreq_hw_driver_probe,
+	.remove = mtk_cpufreq_hw_driver_remove,
+	.driver = {
+		.name = "mtk-cpufreq-hw",
+		.of_match_table = mtk_cpufreq_hw_match,
+	},
+};
+
+static int __init mtk_cpufreq_hw_init(void)
+{
+	return platform_driver_register(&mtk_cpufreq_hw_driver);
+}
+postcore_initcall(mtk_cpufreq_hw_init);
+
+static void __exit mtk_cpufreq_hw_exit(void)
+{
+	platform_driver_unregister(&mtk_cpufreq_hw_driver);
+}
+module_exit(mtk_cpufreq_hw_exit);
+
+MODULE_DESCRIPTION("mtk CPUFREQ HW Driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.9.5

[PATCH v2 2/2] dt-bindings: cpufreq: add bindings for MediaTek cpufreq HW

[Hector Yuan]

From: "Hector.Yuan" <hector.yuan@mediatek.com>

Add devicetree bindings for MediaTek HW driver.

Signed-off-by: Hector.Yuan <hector.yuan@mediatek.com>
---
 .../bindings/cpufreq/cpufreq-mediatek-hw.yaml      |   61 ++++++++++++++++++++
 1 file changed, 61 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek-hw.yaml

diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek-hw.yaml b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek-hw.yaml
new file mode 100644
index 0000000..59bb24e
--- /dev/null
+++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek-hw.yaml
@@ -0,0 +1,61 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/cpufreq/cpufreq-mediatek-hw.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: MediaTek's CPUFREQ Bindings
+
+maintainers:
+  - Hector Yuan <hector.yuan@mediatek.com>
+
+description:
+  CPUFREQ HW is a hardware engine used by MediaTek
+  SoCs to manage frequency in hardware. It is capable of controlling frequency
+  for multiple clusters.
+
+properties:
+  compatible:
+    const: mediatek,cpufreq-hw
+
+  reg:
+    minItems: 1
+    maxItems: 2
+    description: |
+      Addresses and sizes for the memory of the HW bases in each frequency domain.
+
+  reg-names:
+    items:
+      - const: "freq-domain0"
+      - const: "freq-domain1"
+    description: |
+      Frequency domain name.
+
+  "#freq-domain-cells":
+    const: 1
+    description: |
+      Number of cells in a freqency domain specifier.
+
+required:
+  - compatible
+  - reg
+  - reg-names
+  - "#freq-domain-cells"
+
+additionalProperties: false
+
+examples:
+  - |
+    soc {
+        #address-cells = <2>;
+        #size-cells = <2>;
+
+        cpufreq_hw: cpufreq@11bc00 {
+            compatible = "mediatek,cpufreq-hw";
+            reg = <0 0x11bc10 0 0x8c>,
+               <0 0x11bca0 0 0x8c>;
+            reg-names = "freq-domain0", "freq-domain1";
+            #freq-domain-cells = <1>;
+        };
+    };
+
-- 
1.7.9.5

Re: [PATCH v2 1/2] cpufreq: mediatek-hw: Add support for Mediatek cpufreq HW driver

[Viresh Kumar]

On 13-08-20, 15:07, Hector Yuan wrote:
> From: "Hector.Yuan" <hector.yuan@mediatek.com>
> 
> Add MT6779 cpufreq HW support.
> 
> Signed-off-by: Hector.Yuan <hector.yuan@mediatek.com>
> ---
>  arch/arm64/configs/defconfig          |    1 +
>  drivers/cpufreq/Kconfig.arm           |   11 ++
>  drivers/cpufreq/Makefile              |    1 +
>  drivers/cpufreq/mediatek-cpufreq-hw.c |  255 +++++++++++++++++++++++++++++++++
>  4 files changed, 268 insertions(+)
>  create mode 100644 drivers/cpufreq/mediatek-cpufreq-hw.c
> 
> diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> index 883e8ba..866a1bf 100644
> --- a/arch/arm64/configs/defconfig
> +++ b/arch/arm64/configs/defconfig
> @@ -86,6 +86,7 @@ CONFIG_CPUFREQ_DT=y
>  CONFIG_ACPI_CPPC_CPUFREQ=m
>  CONFIG_ARM_ALLWINNER_SUN50I_CPUFREQ_NVMEM=m
>  CONFIG_ARM_ARMADA_37XX_CPUFREQ=y
> +CONFIG_ARM_MEDIATEK_CPUFREQ_HW=m

What about a 'default m' in Kconfig itself ?

>  CONFIG_ARM_SCPI_CPUFREQ=y
>  CONFIG_ARM_IMX_CPUFREQ_DT=m
>  CONFIG_ARM_QCOM_CPUFREQ_NVMEM=y
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index c6cbfc8..81f1cc1 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -121,6 +121,17 @@ config ARM_MEDIATEK_CPUFREQ
>  	help
>  	  This adds the CPUFreq driver support for MediaTek SoCs.
>  
> +config ARM_MEDIATEK_CPUFREQ_HW
> +	tristate "MediaTek CPUFreq HW driver"
> +	depends on ARCH_MEDIATEK || COMPILE_TEST
> +	help
> +	  Support for the CPUFreq HW driver.
> +	  Some MediaTek chipsets have a HW engine to offload the steps
> +	  necessary for changing the frequency of the CPUs. Firmware loaded
> +	  in this engine exposes a programming interface to the OS.
> +	  The driver implements the cpufreq interface for this HW engine.
> +	  Say Y if you want to support CPUFreq HW.
> +
>  config ARM_OMAP2PLUS_CPUFREQ
>  	bool "TI OMAP2+"
>  	depends on ARCH_OMAP2PLUS
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index f6670c4..dc1f371 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -57,6 +57,7 @@ obj-$(CONFIG_ARM_IMX6Q_CPUFREQ)		+= imx6q-cpufreq.o
>  obj-$(CONFIG_ARM_IMX_CPUFREQ_DT)	+= imx-cpufreq-dt.o
>  obj-$(CONFIG_ARM_KIRKWOOD_CPUFREQ)	+= kirkwood-cpufreq.o
>  obj-$(CONFIG_ARM_MEDIATEK_CPUFREQ)	+= mediatek-cpufreq.o
> +obj-$(CONFIG_ARM_MEDIATEK_CPUFREQ_HW)	+= mediatek-cpufreq-hw.o
>  obj-$(CONFIG_MACH_MVEBU_V7)		+= mvebu-cpufreq.o
>  obj-$(CONFIG_ARM_OMAP2PLUS_CPUFREQ)	+= omap-cpufreq.o
>  obj-$(CONFIG_ARM_PXA2xx_CPUFREQ)	+= pxa2xx-cpufreq.o
> diff --git a/drivers/cpufreq/mediatek-cpufreq-hw.c b/drivers/cpufreq/mediatek-cpufreq-hw.c
> new file mode 100644
> index 0000000..6752db9
> --- /dev/null
> +++ b/drivers/cpufreq/mediatek-cpufreq-hw.c
> @@ -0,0 +1,255 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (c) 2020 MediaTek Inc.
> + */
> +
> +#include <linux/bitfield.h>
> +#include <linux/cpufreq.h>
> +#include <linux/init.h>
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/of_address.h>
> +#include <linux/of_platform.h>
> +#include <linux/pm_opp.h>
> +#include <linux/slab.h>
> +
> +#define LUT_MAX_ENTRIES			32U
> +#define LUT_FREQ			GENMASK(11, 0)
> +#define LUT_VOLT			GENMASK(28, 12)
> +#define LUT_ROW_SIZE			0x4
> +
> +/* Register offsets */
> +#define REG_ENABLE			0x84
> +#define REG_PERF_STATE		0x88
> +
> +static struct platform_device *global_pdev;

Use cpufreq_driver->driver_data for this, it is already used in other
drivers for similar use.

> +static int mtk_cpufreq_hw_target_index(struct cpufreq_policy *policy,
> +				       unsigned int index)
> +{
> +	void __iomem *perf_state_reg = policy->driver_data;
> +	unsigned long freq = policy->freq_table[index].frequency;
> +
> +	writel_relaxed(index, perf_state_reg);
> +	arch_set_freq_scale(policy->related_cpus, freq,
> +			    policy->cpuinfo.max_freq);
> +	return 0;
> +}
> +
> +static unsigned int mtk_cpufreq_hw_get(unsigned int cpu)
> +{
> +	void __iomem *perf_state_reg;
> +	struct cpufreq_policy *policy;
> +	unsigned int index;
> +
> +	policy = cpufreq_cpu_get_raw(cpu);
> +	if (!policy)
> +		return 0;
> +
> +	perf_state_reg = policy->driver_data;
> +
> +	index = readl_relaxed(perf_state_reg);
> +	index = min(index, LUT_MAX_ENTRIES - 1);
> +
> +	return policy->freq_table[index].frequency;
> +}
> +
> +static int mtk_cpufreq_hw_read_lut(struct device *cpu_dev,

This routine needs to be named better, it is creating the cpufreq
table after all.

> +				   struct cpufreq_policy *policy,
> +				   void __iomem *base)

Please make sure checkpatch --strict doesn't give any errors.

> +{
> +	u32 data;
> +	u32 freq, volt, prev_freq = 0;

Merge these two..

> +	int i = 0;
> +	struct cpufreq_frequency_table	*table;
> +
> +	table = kcalloc(LUT_MAX_ENTRIES + 1, sizeof(*table), GFP_KERNEL);
> +	if (!table)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> +		data = readl_relaxed(base + (i * LUT_ROW_SIZE));
> +		freq = FIELD_GET(LUT_FREQ, data) * 1000;
> +		volt = FIELD_GET(LUT_VOLT, data);
> +		if (freq != prev_freq) {
> +			table[i].frequency = freq;
> +			dev_pm_opp_add(cpu_dev, freq * 1000, volt);

Why are you adding OPPs here and rather why using OPP specific stuff
at all in the driver ?

> +			dev_dbg(cpu_dev, "index=%d freq=%d, volt=%d\n", i,
> +				freq, volt);
> +		} else {
> +			break;
> +		}
> +		prev_freq = freq;
> +	}
> +	table[i].frequency = CPUFREQ_TABLE_END;
> +	policy->freq_table = table;
> +	dev_pm_opp_set_sharing_cpus(cpu_dev, policy->cpus);
> +
> +	return 0;
> +}
> +
> +static void mtk_get_related_cpus(int index, struct cpumask *m)
> +{
> +	struct device_node *cpu_np;
> +	struct of_phandle_args args;
> +	int cpu, ret;
> +
> +	for_each_possible_cpu(cpu) {
> +		cpu_np = of_cpu_device_node_get(cpu);
> +		if (!cpu_np)
> +			continue;
> +
> +		ret = of_parse_phandle_with_args(cpu_np, "mtk,freq-domain",

Where are bindings of this node and how does this look ?

> +						 "#freq-domain-cells", 0,
> +						 &args);
> +		of_node_put(cpu_np);
> +		if (ret < 0)
> +			continue;
> +
> +		if (index == args.args[0])
> +			cpumask_set_cpu(cpu, m);
> +	}
> +}
> +
> +static int mtk_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> +{
> +	struct device *dev = &global_pdev->dev;
> +	struct of_phandle_args args;
> +	struct device_node *cpu_np;
> +	struct device *cpu_dev;
> +	struct resource *res;
> +	void __iomem *base;
> +	int ret, index;
> +
> +	cpu_dev = get_cpu_device(policy->cpu);
> +	if (!cpu_dev) {
> +		pr_err("%s: failed to get cpu%d device\n", __func__,
> +		       policy->cpu);
> +		return -ENODEV;
> +	}
> +
> +	cpu_np = of_cpu_device_node_get(policy->cpu);
> +	if (!cpu_np)
> +		return -EINVAL;
> +
> +	ret = of_parse_phandle_with_args(cpu_np, "mtk,freq-domain",
> +					 "#freq-domain-cells", 0, &args);
> +	of_node_put(cpu_np);
> +	if (ret)
> +		return ret;
> +
> +	index = args.args[0];
> +	res = platform_get_resource(global_pdev, IORESOURCE_MEM, index);
> +	if (!res)
> +		return -ENODEV;
> +	base = devm_ioremap(dev, res->start, resource_size(res));
> +	if (!base)
> +		return -ENOMEM;
> +
> +	mtk_get_related_cpus(index, policy->cpus);
> +	if (!cpumask_weight(policy->cpus)) {
> +		dev_err(dev, "Domain-%d failed to get related CPUs\n", index);
> +		ret = -ENOENT;
> +		goto error;
> +	}
> +
> +	policy->driver_data = base + REG_PERF_STATE;
> +	ret = mtk_cpufreq_hw_read_lut(cpu_dev, policy, base);
> +	if (ret) {
> +		dev_err(dev, "Domain-%d failed to read LUT\n", index);
> +		goto error;
> +	}
> +
> +	ret = dev_pm_opp_get_opp_count(cpu_dev);
> +	if (ret <= 0) {
> +		dev_err(cpu_dev, "Failed to add OPPs\n");
> +		ret = -ENODEV;
> +		goto error;
> +	}
> +
> +	dev_pm_opp_of_register_em(policy->cpus);
> +
> +	/* HW should be in enabled state to proceed now */
> +	writel_relaxed(0x1, (base + REG_ENABLE));
> +	return 0;
> +error:
> +	devm_iounmap(dev, base);
> +	return ret;
> +}
> +
> +static int mtk_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;
> +
> +	dev_pm_opp_remove_all_dynamic(cpu_dev);
> +	kfree(policy->freq_table);
> +	devm_iounmap(&global_pdev->dev, base);
> +
> +	return 0;
> +}
> +
> +static struct freq_attr *mtk_cpufreq_hw_attr[] = {
> +	&cpufreq_freq_attr_scaling_available_freqs,
> +	NULL
> +};
> +
> +static struct cpufreq_driver cpufreq_mtk_hw_driver = {
> +	.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
> +			  CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
> +	.verify		= cpufreq_generic_frequency_table_verify,
> +	.target_index	= mtk_cpufreq_hw_target_index,
> +	.get		= mtk_cpufreq_hw_get,
> +	.init		= mtk_cpufreq_hw_cpu_init,
> +	.exit		= mtk_cpufreq_hw_cpu_exit,
> +	.name		= "mtk-cpufreq-hw",
> +	.attr		= mtk_cpufreq_hw_attr,
> +};
> +
> +static int mtk_cpufreq_hw_driver_probe(struct platform_device *pdev)
> +{
> +	int ret;
> +
> +	global_pdev = pdev;
> +	ret = cpufreq_register_driver(&cpufreq_mtk_hw_driver);
> +	if (ret)
> +		dev_err(&pdev->dev, "CPUFreq HW driver failed to register\n");
> +	else
> +		dev_dbg(&pdev->dev, "mtk CPUFreq HW driver initialized\n");
> +
> +	return ret;
> +}
> +
> +static int mtk_cpufreq_hw_driver_remove(struct platform_device *pdev)
> +{
> +	return cpufreq_unregister_driver(&cpufreq_mtk_hw_driver);
> +}
> +
> +static const struct of_device_id mtk_cpufreq_hw_match[] = {
> +	{ .compatible = "mediatek,cpufreq-hw" },
> +	{}
> +};
> +MODULE_DEVICE_TABLE(of, mtk_cpufreq_hw_match);
> +
> +static struct platform_driver mtk_cpufreq_hw_driver = {
> +	.probe = mtk_cpufreq_hw_driver_probe,
> +	.remove = mtk_cpufreq_hw_driver_remove,
> +	.driver = {
> +		.name = "mtk-cpufreq-hw",
> +		.of_match_table = mtk_cpufreq_hw_match,
> +	},
> +};
> +
> +static int __init mtk_cpufreq_hw_init(void)
> +{
> +	return platform_driver_register(&mtk_cpufreq_hw_driver);
> +}
> +postcore_initcall(mtk_cpufreq_hw_init);
> +
> +static void __exit mtk_cpufreq_hw_exit(void)
> +{
> +	platform_driver_unregister(&mtk_cpufreq_hw_driver);
> +}
> +module_exit(mtk_cpufreq_hw_exit);
> +
> +MODULE_DESCRIPTION("mtk CPUFREQ HW Driver");
> +MODULE_LICENSE("GPL v2");
> -- 
> 1.7.9.5

-- 
viresh

Re: [PATCH v2 2/2] dt-bindings: cpufreq: add bindings for MediaTek cpufreq HW

[Rob Herring]

On Thu, Aug 13, 2020 at 03:07:55PM +0800, Hector Yuan wrote:
> From: "Hector.Yuan" <hector.yuan@mediatek.com>
> 
> Add devicetree bindings for MediaTek HW driver.
> 
> Signed-off-by: Hector.Yuan <hector.yuan@mediatek.com>
> ---
>  .../bindings/cpufreq/cpufreq-mediatek-hw.yaml      |   61 ++++++++++++++++++++
>  1 file changed, 61 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek-hw.yaml
> 
> diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek-hw.yaml b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek-hw.yaml
> new file mode 100644
> index 0000000..59bb24e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek-hw.yaml
> @@ -0,0 +1,61 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/cpufreq/cpufreq-mediatek-hw.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MediaTek's CPUFREQ Bindings
> +
> +maintainers:
> +  - Hector Yuan <hector.yuan@mediatek.com>
> +
> +description:
> +  CPUFREQ HW is a hardware engine used by MediaTek
> +  SoCs to manage frequency in hardware. It is capable of controlling frequency
> +  for multiple clusters.
> +
> +properties:
> +  compatible:
> +    const: mediatek,cpufreq-hw
> +
> +  reg:
> +    minItems: 1
> +    maxItems: 2
> +    description: |
> +      Addresses and sizes for the memory of the HW bases in each frequency domain.
> +
> +  reg-names:
> +    items:
> +      - const: "freq-domain0"
> +      - const: "freq-domain1"

Not all that useful of a name given it's based on the index.

> +    description: |
> +      Frequency domain name.
> +
> +  "#freq-domain-cells":
> +    const: 1
> +    description: |
> +      Number of cells in a freqency domain specifier.

What's this for?

> +
> +required:
> +  - compatible
> +  - reg
> +  - reg-names
> +  - "#freq-domain-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    soc {
> +        #address-cells = <2>;
> +        #size-cells = <2>;
> +
> +        cpufreq_hw: cpufreq@11bc00 {
> +            compatible = "mediatek,cpufreq-hw";
> +            reg = <0 0x11bc10 0 0x8c>,
> +               <0 0x11bca0 0 0x8c>;
> +            reg-names = "freq-domain0", "freq-domain1";
> +            #freq-domain-cells = <1>;
> +        };
> +    };
> +
> -- 
> 1.7.9.5

Re: [PATCH v2 2/2] dt-bindings: cpufreq: add bindings for MediaTek cpufreq HW

[Hector Yuan]

On Mon, 2020-08-24 at 20:04 -0600, Rob Herring wrote:
> On Thu, Aug 13, 2020 at 03:07:55PM +0800, Hector Yuan wrote:
> > From: "Hector.Yuan" <hector.yuan@mediatek.com>
> > 
> > Add devicetree bindings for MediaTek HW driver.
> > 
> > Signed-off-by: Hector.Yuan <hector.yuan@mediatek.com>
> > ---
> >  .../bindings/cpufreq/cpufreq-mediatek-hw.yaml      |   61 ++++++++++++++++++++
> >  1 file changed, 61 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek-hw.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek-hw.yaml b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek-hw.yaml
> > new file mode 100644
> > index 0000000..59bb24e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/cpufreq/cpufreq-mediatek-hw.yaml
> > @@ -0,0 +1,61 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/cpufreq/cpufreq-mediatek-hw.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: MediaTek's CPUFREQ Bindings
> > +
> > +maintainers:
> > +  - Hector Yuan <hector.yuan@mediatek.com>
> > +
> > +description:
> > +  CPUFREQ HW is a hardware engine used by MediaTek
> > +  SoCs to manage frequency in hardware. It is capable of controlling frequency
> > +  for multiple clusters.
> > +
> > +properties:
> > +  compatible:
> > +    const: mediatek,cpufreq-hw
> > +
> > +  reg:
> > +    minItems: 1
> > +    maxItems: 2
> > +    description: |
> > +      Addresses and sizes for the memory of the HW bases in each frequency domain.
> > +
> > +  reg-names:
> > +    items:
> > +      - const: "freq-domain0"
> > +      - const: "freq-domain1"
> 
> Not all that useful of a name given it's based on the index.
Let me explain this index is about to map cpus to each frequency control
domain. Will update details usage in V3. Thank you.
> > +    description: |
> > +      Frequency domain name.
> > +
> > +  "#freq-domain-cells":
> > +    const: 1
> > +    description: |
> > +      Number of cells in a freqency domain specifier.
> 
> What's this for?
> Like the previous mentioned, this is for index mapping. will update in V3. Thank you.
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - reg-names
> > +  - "#freq-domain-cells"
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    soc {
> > +        #address-cells = <2>;
> > +        #size-cells = <2>;
> > +
> > +        cpufreq_hw: cpufreq@11bc00 {
> > +            compatible = "mediatek,cpufreq-hw";
> > +            reg = <0 0x11bc10 0 0x8c>,
> > +               <0 0x11bca0 0 0x8c>;
> > +            reg-names = "freq-domain0", "freq-domain1";
> > +            #freq-domain-cells = <1>;
> > +        };
> > +    };
> > +
> > -- 
> > 1.7.9.5

Re: [PATCH v2 1/2] cpufreq: mediatek-hw: Add support for Mediatek cpufreq HW driver

[Hector Yuan]

On Mon, 2020-08-24 at 15:36 +0530, Viresh Kumar wrote:
> On 13-08-20, 15:07, Hector Yuan wrote:
> > From: "Hector.Yuan" <hector.yuan@mediatek.com>
> > 
> > Add MT6779 cpufreq HW support.
> > 
> > Signed-off-by: Hector.Yuan <hector.yuan@mediatek.com>
> > ---
> >  arch/arm64/configs/defconfig          |    1 +
> >  drivers/cpufreq/Kconfig.arm           |   11 ++
> >  drivers/cpufreq/Makefile              |    1 +
> >  drivers/cpufreq/mediatek-cpufreq-hw.c |  255 +++++++++++++++++++++++++++++++++
> >  4 files changed, 268 insertions(+)
> >  create mode 100644 drivers/cpufreq/mediatek-cpufreq-hw.c
> > 
> > diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
> > index 883e8ba..866a1bf 100644
> > --- a/arch/arm64/configs/defconfig
> > +++ b/arch/arm64/configs/defconfig
> > @@ -86,6 +86,7 @@ CONFIG_CPUFREQ_DT=y
> >  CONFIG_ACPI_CPPC_CPUFREQ=m
> >  CONFIG_ARM_ALLWINNER_SUN50I_CPUFREQ_NVMEM=m
> >  CONFIG_ARM_ARMADA_37XX_CPUFREQ=y
> > +CONFIG_ARM_MEDIATEK_CPUFREQ_HW=m
> 
> What about a 'default m' in Kconfig itself ?
> OK, will update in V3.
> >  CONFIG_ARM_SCPI_CPUFREQ=y
> >  CONFIG_ARM_IMX_CPUFREQ_DT=m
> >  CONFIG_ARM_QCOM_CPUFREQ_NVMEM=y
> > diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> > index c6cbfc8..81f1cc1 100644
> > --- a/drivers/cpufreq/Kconfig.arm
> > +++ b/drivers/cpufreq/Kconfig.arm
> > @@ -121,6 +121,17 @@ config ARM_MEDIATEK_CPUFREQ
> >  	help
> >  	  This adds the CPUFreq driver support for MediaTek SoCs.
> >  
> > +config ARM_MEDIATEK_CPUFREQ_HW
> > +	tristate "MediaTek CPUFreq HW driver"
> > +	depends on ARCH_MEDIATEK || COMPILE_TEST
> > +	help
> > +	  Support for the CPUFreq HW driver.
> > +	  Some MediaTek chipsets have a HW engine to offload the steps
> > +	  necessary for changing the frequency of the CPUs. Firmware loaded
> > +	  in this engine exposes a programming interface to the OS.
> > +	  The driver implements the cpufreq interface for this HW engine.
> > +	  Say Y if you want to support CPUFreq HW.
> > +
> >  config ARM_OMAP2PLUS_CPUFREQ
> >  	bool "TI OMAP2+"
> >  	depends on ARCH_OMAP2PLUS
> > diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> > index f6670c4..dc1f371 100644
> > --- a/drivers/cpufreq/Makefile
> > +++ b/drivers/cpufreq/Makefile
> > @@ -57,6 +57,7 @@ obj-$(CONFIG_ARM_IMX6Q_CPUFREQ)		+= imx6q-cpufreq.o
> >  obj-$(CONFIG_ARM_IMX_CPUFREQ_DT)	+= imx-cpufreq-dt.o
> >  obj-$(CONFIG_ARM_KIRKWOOD_CPUFREQ)	+= kirkwood-cpufreq.o
> >  obj-$(CONFIG_ARM_MEDIATEK_CPUFREQ)	+= mediatek-cpufreq.o
> > +obj-$(CONFIG_ARM_MEDIATEK_CPUFREQ_HW)	+= mediatek-cpufreq-hw.o
> >  obj-$(CONFIG_MACH_MVEBU_V7)		+= mvebu-cpufreq.o
> >  obj-$(CONFIG_ARM_OMAP2PLUS_CPUFREQ)	+= omap-cpufreq.o
> >  obj-$(CONFIG_ARM_PXA2xx_CPUFREQ)	+= pxa2xx-cpufreq.o
> > diff --git a/drivers/cpufreq/mediatek-cpufreq-hw.c b/drivers/cpufreq/mediatek-cpufreq-hw.c
> > new file mode 100644
> > index 0000000..6752db9
> > --- /dev/null
> > +++ b/drivers/cpufreq/mediatek-cpufreq-hw.c
> > @@ -0,0 +1,255 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Copyright (c) 2020 MediaTek Inc.
> > + */
> > +
> > +#include <linux/bitfield.h>
> > +#include <linux/cpufreq.h>
> > +#include <linux/init.h>
> > +#include <linux/kernel.h>
> > +#include <linux/module.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_platform.h>
> > +#include <linux/pm_opp.h>
> > +#include <linux/slab.h>
> > +
> > +#define LUT_MAX_ENTRIES			32U
> > +#define LUT_FREQ			GENMASK(11, 0)
> > +#define LUT_VOLT			GENMASK(28, 12)
> > +#define LUT_ROW_SIZE			0x4
> > +
> > +/* Register offsets */
> > +#define REG_ENABLE			0x84
> > +#define REG_PERF_STATE		0x88
> > +
> > +static struct platform_device *global_pdev;
> 
> Use cpufreq_driver->driver_data for this, it is already used in other
> drivers for similar use.
> OK, I see.will update in V3.
> > +static int mtk_cpufreq_hw_target_index(struct cpufreq_policy *policy,
> > +				       unsigned int index)
> > +{
> > +	void __iomem *perf_state_reg = policy->driver_data;
> > +	unsigned long freq = policy->freq_table[index].frequency;
> > +
> > +	writel_relaxed(index, perf_state_reg);
> > +	arch_set_freq_scale(policy->related_cpus, freq,
> > +			    policy->cpuinfo.max_freq);
> > +	return 0;
> > +}
> > +
> > +static unsigned int mtk_cpufreq_hw_get(unsigned int cpu)
> > +{
> > +	void __iomem *perf_state_reg;
> > +	struct cpufreq_policy *policy;
> > +	unsigned int index;
> > +
> > +	policy = cpufreq_cpu_get_raw(cpu);
> > +	if (!policy)
> > +		return 0;
> > +
> > +	perf_state_reg = policy->driver_data;
> > +
> > +	index = readl_relaxed(perf_state_reg);
> > +	index = min(index, LUT_MAX_ENTRIES - 1);
> > +
> > +	return policy->freq_table[index].frequency;
> > +}
> > +
> > +static int mtk_cpufreq_hw_read_lut(struct device *cpu_dev,
> 
> This routine needs to be named better, it is creating the cpufreq
> table after all.
> OK, will update in V3. rename to xxx_opp_create.
> > +				   struct cpufreq_policy *policy,
> > +				   void __iomem *base)
> 
> Please make sure checkpatch --strict doesn't give any errors.
> 
> > +{
> > +	u32 data;
> > +	u32 freq, volt, prev_freq = 0;
> 
> Merge these two..
> OK
> > +	int i = 0;
> > +	struct cpufreq_frequency_table	*table;
> > +
> > +	table = kcalloc(LUT_MAX_ENTRIES + 1, sizeof(*table), GFP_KERNEL);
> > +	if (!table)
> > +		return -ENOMEM;
> > +
> > +	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> > +		data = readl_relaxed(base + (i * LUT_ROW_SIZE));
> > +		freq = FIELD_GET(LUT_FREQ, data) * 1000;
> > +		volt = FIELD_GET(LUT_VOLT, data);
> > +		if (freq != prev_freq) {
> > +			table[i].frequency = freq;
> > +			dev_pm_opp_add(cpu_dev, freq * 1000, volt);
> 
> Why are you adding OPPs here and rather why using OPP specific stuff
> at all in the driver ?
> yes, the opp information is read from CPU HW engine.Then add it to the CPU dev OPP one by one.  
> > +			dev_dbg(cpu_dev, "index=%d freq=%d, volt=%d\n", i,
> > +				freq, volt);
> > +		} else {
> > +			break;
> > +		}
> > +		prev_freq = freq;
> > +	}
> > +	table[i].frequency = CPUFREQ_TABLE_END;
> > +	policy->freq_table = table;
> > +	dev_pm_opp_set_sharing_cpus(cpu_dev, policy->cpus);
> > +
> > +	return 0;
> > +}
> > +
> > +static void mtk_get_related_cpus(int index, struct cpumask *m)
> > +{
> > +	struct device_node *cpu_np;
> > +	struct of_phandle_args args;
> > +	int cpu, ret;
> > +
> > +	for_each_possible_cpu(cpu) {
> > +		cpu_np = of_cpu_device_node_get(cpu);
> > +		if (!cpu_np)
> > +			continue;
> > +
> > +		ret = of_parse_phandle_with_args(cpu_np, "mtk,freq-domain",
> 
> Where are bindings of this node and how does this look ?
> Can refer to the same patch series, I split it to another patch.Each cpu will be group into one frequency domain for the CPU DVFS. 
> > +						 "#freq-domain-cells", 0,
> > +						 &args);
> > +		of_node_put(cpu_np);
> > +		if (ret < 0)
> > +			continue;
> > +
> > +		if (index == args.args[0])
> > +			cpumask_set_cpu(cpu, m);
> > +	}
> > +}
> > +
> > +static int mtk_cpufreq_hw_cpu_init(struct cpufreq_policy *policy)
> > +{
> > +	struct device *dev = &global_pdev->dev;
> > +	struct of_phandle_args args;
> > +	struct device_node *cpu_np;
> > +	struct device *cpu_dev;
> > +	struct resource *res;
> > +	void __iomem *base;
> > +	int ret, index;
> > +
> > +	cpu_dev = get_cpu_device(policy->cpu);
> > +	if (!cpu_dev) {
> > +		pr_err("%s: failed to get cpu%d device\n", __func__,
> > +		       policy->cpu);
> > +		return -ENODEV;
> > +	}
> > +
> > +	cpu_np = of_cpu_device_node_get(policy->cpu);
> > +	if (!cpu_np)
> > +		return -EINVAL;
> > +
> > +	ret = of_parse_phandle_with_args(cpu_np, "mtk,freq-domain",
> > +					 "#freq-domain-cells", 0, &args);
> > +	of_node_put(cpu_np);
> > +	if (ret)
> > +		return ret;
> > +
> > +	index = args.args[0];
> > +	res = platform_get_resource(global_pdev, IORESOURCE_MEM, index);
> > +	if (!res)
> > +		return -ENODEV;
> > +	base = devm_ioremap(dev, res->start, resource_size(res));
> > +	if (!base)
> > +		return -ENOMEM;
> > +
> > +	mtk_get_related_cpus(index, policy->cpus);
> > +	if (!cpumask_weight(policy->cpus)) {
> > +		dev_err(dev, "Domain-%d failed to get related CPUs\n", index);
> > +		ret = -ENOENT;
> > +		goto error;
> > +	}
> > +
> > +	policy->driver_data = base + REG_PERF_STATE;
> > +	ret = mtk_cpufreq_hw_read_lut(cpu_dev, policy, base);
> > +	if (ret) {
> > +		dev_err(dev, "Domain-%d failed to read LUT\n", index);
> > +		goto error;
> > +	}
> > +
> > +	ret = dev_pm_opp_get_opp_count(cpu_dev);
> > +	if (ret <= 0) {
> > +		dev_err(cpu_dev, "Failed to add OPPs\n");
> > +		ret = -ENODEV;
> > +		goto error;
> > +	}
> > +
> > +	dev_pm_opp_of_register_em(policy->cpus);
> > +
> > +	/* HW should be in enabled state to proceed now */
> > +	writel_relaxed(0x1, (base + REG_ENABLE));
> > +	return 0;
> > +error:
> > +	devm_iounmap(dev, base);
> > +	return ret;
> > +}
> > +
> > +static int mtk_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;
> > +
> > +	dev_pm_opp_remove_all_dynamic(cpu_dev);
> > +	kfree(policy->freq_table);
> > +	devm_iounmap(&global_pdev->dev, base);
> > +
> > +	return 0;
> > +}
> > +
> > +static struct freq_attr *mtk_cpufreq_hw_attr[] = {
> > +	&cpufreq_freq_attr_scaling_available_freqs,
> > +	NULL
> > +};
> > +
> > +static struct cpufreq_driver cpufreq_mtk_hw_driver = {
> > +	.flags		= CPUFREQ_STICKY | CPUFREQ_NEED_INITIAL_FREQ_CHECK |
> > +			  CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
> > +	.verify		= cpufreq_generic_frequency_table_verify,
> > +	.target_index	= mtk_cpufreq_hw_target_index,
> > +	.get		= mtk_cpufreq_hw_get,
> > +	.init		= mtk_cpufreq_hw_cpu_init,
> > +	.exit		= mtk_cpufreq_hw_cpu_exit,
> > +	.name		= "mtk-cpufreq-hw",
> > +	.attr		= mtk_cpufreq_hw_attr,
> > +};
> > +
> > +static int mtk_cpufreq_hw_driver_probe(struct platform_device *pdev)
> > +{
> > +	int ret;
> > +
> > +	global_pdev = pdev;
> > +	ret = cpufreq_register_driver(&cpufreq_mtk_hw_driver);
> > +	if (ret)
> > +		dev_err(&pdev->dev, "CPUFreq HW driver failed to register\n");
> > +	else
> > +		dev_dbg(&pdev->dev, "mtk CPUFreq HW driver initialized\n");
> > +
> > +	return ret;
> > +}
> > +
> > +static int mtk_cpufreq_hw_driver_remove(struct platform_device *pdev)
> > +{
> > +	return cpufreq_unregister_driver(&cpufreq_mtk_hw_driver);
> > +}
> > +
> > +static const struct of_device_id mtk_cpufreq_hw_match[] = {
> > +	{ .compatible = "mediatek,cpufreq-hw" },
> > +	{}
> > +};
> > +MODULE_DEVICE_TABLE(of, mtk_cpufreq_hw_match);
> > +
> > +static struct platform_driver mtk_cpufreq_hw_driver = {
> > +	.probe = mtk_cpufreq_hw_driver_probe,
> > +	.remove = mtk_cpufreq_hw_driver_remove,
> > +	.driver = {
> > +		.name = "mtk-cpufreq-hw",
> > +		.of_match_table = mtk_cpufreq_hw_match,
> > +	},
> > +};
> > +
> > +static int __init mtk_cpufreq_hw_init(void)
> > +{
> > +	return platform_driver_register(&mtk_cpufreq_hw_driver);
> > +}
> > +postcore_initcall(mtk_cpufreq_hw_init);
> > +
> > +static void __exit mtk_cpufreq_hw_exit(void)
> > +{
> > +	platform_driver_unregister(&mtk_cpufreq_hw_driver);
> > +}
> > +module_exit(mtk_cpufreq_hw_exit);
> > +
> > +MODULE_DESCRIPTION("mtk CPUFREQ HW Driver");
> > +MODULE_LICENSE("GPL v2");
> > -- 
> > 1.7.9.5
> 

Re: [PATCH v2 1/2] cpufreq: mediatek-hw: Add support for Mediatek cpufreq HW driver

[Viresh Kumar]

On 26-08-20, 20:57, Hector Yuan wrote:
> On Mon, 2020-08-24 at 15:36 +0530, Viresh Kumar wrote:
> > On 13-08-20, 15:07, Hector Yuan wrote:
> > >  CONFIG_ARM_ALLWINNER_SUN50I_CPUFREQ_NVMEM=m
> > >  CONFIG_ARM_ARMADA_37XX_CPUFREQ=y
> > > +CONFIG_ARM_MEDIATEK_CPUFREQ_HW=m
> > 
> > What about a 'default m' in Kconfig itself ?
> > OK, will update in V3.

Hector, you need to remove (or not add) the right bracket (>) before the
beginning of your lines. This makes it incredibly difficult to read.

> > > +	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> > > +		data = readl_relaxed(base + (i * LUT_ROW_SIZE));
> > > +		freq = FIELD_GET(LUT_FREQ, data) * 1000;
> > > +		volt = FIELD_GET(LUT_VOLT, data);
> > > +		if (freq != prev_freq) {
> > > +			table[i].frequency = freq;
> > > +			dev_pm_opp_add(cpu_dev, freq * 1000, volt);
> > 
> > Why are you adding OPPs here and rather why using OPP specific stuff
> > at all in the driver ?
> > yes, the opp information is read from CPU HW engine.Then add it to the CPU dev OPP one by one.  

I asked a different question, why are you adding OPPs ? You don't need the OPPs
at all in my opinion. You can just create the frequency table and that's it.

> > > +	for_each_possible_cpu(cpu) {
> > > +		cpu_np = of_cpu_device_node_get(cpu);
> > > +		if (!cpu_np)
> > > +			continue;
> > > +
> > > +		ret = of_parse_phandle_with_args(cpu_np, "mtk,freq-domain",
> > 
> > Where are bindings of this node and how does this look ?
> > Can refer to the same patch series, I split it to another patch.Each cpu will be group into one frequency domain for the CPU DVFS. 

That binding only defines "mediatek,cpufreq-hw" and not "mtk,freq-domain".

-- 
viresh

Re: [PATCH v2 1/2] cpufreq: mediatek-hw: Add support for Mediatek cpufreq HW driver

[Hector Yuan]

On Thu, 2020-08-27 at 09:56 +0530, Viresh Kumar wrote:
> On 26-08-20, 20:57, Hector Yuan wrote:
> > On Mon, 2020-08-24 at 15:36 +0530, Viresh Kumar wrote:
> > > On 13-08-20, 15:07, Hector Yuan wrote:
> > > >  CONFIG_ARM_ALLWINNER_SUN50I_CPUFREQ_NVMEM=m
> > > >  CONFIG_ARM_ARMADA_37XX_CPUFREQ=y
> > > > +CONFIG_ARM_MEDIATEK_CPUFREQ_HW=m
> > > 
> > > What about a 'default m' in Kconfig itself ?
> > > OK, will update in V3.
> 
> Hector, you need to remove (or not add) the right bracket (>) before the
> beginning of your lines. This makes it incredibly difficult to read.

OK, I get it. Sorry for the inconvenience.
> > > > +	for (i = 0; i < LUT_MAX_ENTRIES; i++) {
> > > > +		data = readl_relaxed(base + (i * LUT_ROW_SIZE));
> > > > +		freq = FIELD_GET(LUT_FREQ, data) * 1000;
> > > > +		volt = FIELD_GET(LUT_VOLT, data);
> > > > +		if (freq != prev_freq) {
> > > > +			table[i].frequency = freq;
> > > > +			dev_pm_opp_add(cpu_dev, freq * 1000, volt);
> > > 
> > > Why are you adding OPPs here and rather why using OPP specific stuff
> > > at all in the driver ?
> > > yes, the opp information is read from CPU HW engine.Then add it to the CPU dev OPP one by one.  
> 
> I asked a different question, why are you adding OPPs ? You don't need the OPPs
> at all in my opinion. You can just create the frequency table and that's it.

I just add OPP info to OPP framework so that others modules can get it
from OPP framework.
But like you said, I don't need it in this driver. I will remove this
code segment in V4.
I already send V3 yesterday but not including this modification.

> > > > +	for_each_possible_cpu(cpu) {
> > > > +		cpu_np = of_cpu_device_node_get(cpu);
> > > > +		if (!cpu_np)
> > > > +			continue;
> > > > +
> > > > +		ret = of_parse_phandle_with_args(cpu_np, "mtk,freq-domain",
> > > 
> > > Where are bindings of this node and how does this look ?
> > > Can refer to the same patch series, I split it to another patch.Each cpu will be group into one frequency domain for the CPU DVFS. 
> 
> That binding only defines "mediatek,cpufreq-hw" and not "mtk,freq-domain".

Please refer to the dt binding in V3, thank you.  (lkml not show up yet,
so I post the below link instead)
https://www.spinics.net/lists/arm-kernel/msg832592.html
>