[PATCH 1/3] cpufreq: Add Tegra186 cpufreq driver

[PATCH 1/3] cpufreq: Add Tegra186 cpufreq driver

[Mikko Perttunen]

Add a new cpufreq driver for Tegra186 (and likely later).
The CPUs are organized into two clusters, Denver and A57,
with two and four cores respectively. CPU frequency can be
adjusted by writing the desired rate divisor and a voltage
hint to a special per-core register.

The frequency of each core can be set individually; however,
this is just a hint as all CPUs in a cluster will run at
the maximum rate of non-idle CPUs in the cluster.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/cpufreq/Kconfig.arm        |   7 +
 drivers/cpufreq/Makefile           |   1 +
 drivers/cpufreq/tegra186-cpufreq.c | 277 +++++++++++++++++++++++++++++++++++++
 3 files changed, 285 insertions(+)
 create mode 100644 drivers/cpufreq/tegra186-cpufreq.c

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 74fa5c5904d3..168d36fa4a58 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -247,6 +247,13 @@ config ARM_TEGRA124_CPUFREQ
 	help
 	  This adds the CPUFreq driver support for Tegra124 SOCs.
 
+config ARM_TEGRA186_CPUFREQ
+	tristate "Tegra186 CPUFreq support"
+	depends on ARCH_TEGRA && TEGRA_BPMP
+	default y
+	help
+	  This adds the CPUFreq driver support for Tegra186 SOCs.
+
 config ARM_TI_CPUFREQ
 	bool "Texas Instruments CPUFreq support"
 	depends on ARCH_OMAP2PLUS
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 9f5a8045f36d..b7e78f063c4f 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -77,6 +77,7 @@ obj-$(CONFIG_ARM_SPEAR_CPUFREQ)		+= spear-cpufreq.o
 obj-$(CONFIG_ARM_STI_CPUFREQ)		+= sti-cpufreq.o
 obj-$(CONFIG_ARM_TEGRA20_CPUFREQ)	+= tegra20-cpufreq.o
 obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
+obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)	+= tegra186-cpufreq.o
 obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
 obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
 obj-$(CONFIG_ACPI_CPPC_CPUFREQ) += cppc_cpufreq.o
diff --git a/drivers/cpufreq/tegra186-cpufreq.c b/drivers/cpufreq/tegra186-cpufreq.c
new file mode 100644
index 000000000000..794c1f2d8231
--- /dev/null
+++ b/drivers/cpufreq/tegra186-cpufreq.c
@@ -0,0 +1,277 @@
+/*
+ * Copyright (c) 2017, NVIDIA CORPORATION. All rights reserved
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/cpufreq.h>
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#include <soc/tegra/bpmp.h>
+#include <soc/tegra/bpmp-abi.h>
+
+#define EDVD_CORE_VOLT_FREQ(core)		(0x20 + (core) * 0x4)
+#define EDVD_CORE_VOLT_FREQ_F_SHIFT		0
+#define EDVD_CORE_VOLT_FREQ_F_MASK		0xff
+#define EDVD_CORE_VOLT_FREQ_V_SHIFT		16
+#define EDVD_CORE_VOLT_FREQ_V_MASK		0xff
+
+#define CLUSTER_DENVER				0
+#define CLUSTER_A57				1
+#define NUM_CLUSTERS				2
+
+struct tegra186_cpufreq_cluster {
+	const char *name;
+	unsigned int num_cores;
+};
+
+static const struct tegra186_cpufreq_cluster CLUSTERS[] = {
+	{
+		.name = "denver",
+		.num_cores = 2,
+	},
+	{
+		.name = "a57",
+		.num_cores = 4,
+	}
+};
+
+struct tegra186_cpufreq_data {
+	void __iomem *regs[NUM_CLUSTERS];
+	struct cpufreq_frequency_table *tables[NUM_CLUSTERS];
+};
+
+static void get_cluster_core(int cpu, int *cluster, int *core)
+{
+	switch (cpu) {
+	case 0:
+		*cluster = CLUSTER_A57; *core = 0; break;
+	case 3:
+		*cluster = CLUSTER_A57; *core = 1; break;
+	case 4:
+		*cluster = CLUSTER_A57; *core = 2; break;
+	case 5:
+		*cluster = CLUSTER_A57; *core = 3; break;
+	case 1:
+		*cluster = CLUSTER_DENVER; *core = 0; break;
+	case 2:
+		*cluster = CLUSTER_DENVER; *core = 1; break;
+	}
+}
+
+static int tegra186_cpufreq_init(struct cpufreq_policy *policy)
+{
+	struct tegra186_cpufreq_data *data = cpufreq_get_driver_data();
+	struct cpufreq_frequency_table *table;
+	int cluster, core;
+
+	get_cluster_core(policy->cpu, &cluster, &core);
+
+	table = data->tables[cluster];
+	cpufreq_table_validate_and_show(policy, table);
+
+	policy->cpuinfo.transition_latency = 300 * 1000;
+
+	return 0;
+}
+
+static void write_volt_freq(uint8_t vidx, uint8_t ndiv, void __iomem *regs,
+			unsigned int core)
+{
+	u32 val = 0;
+
+	val |= ndiv << EDVD_CORE_VOLT_FREQ_F_SHIFT;
+	val |= vidx << EDVD_CORE_VOLT_FREQ_V_SHIFT;
+
+	writel(val, regs + EDVD_CORE_VOLT_FREQ(core));
+}
+
+static int tegra186_cpufreq_set_target(struct cpufreq_policy *policy,
+				       unsigned int index)
+{
+	struct cpufreq_frequency_table *tbl = policy->freq_table + index;
+	struct tegra186_cpufreq_data *data = cpufreq_get_driver_data();
+	uint16_t vidx = tbl->driver_data >> 16;
+	uint16_t ndiv = tbl->driver_data & 0xffff;
+	int cluster, core;
+
+	get_cluster_core(policy->cpu, &cluster, &core);
+	write_volt_freq(vidx, ndiv, data->regs[cluster], core);
+
+	return 0;
+}
+
+static struct cpufreq_driver tegra186_cpufreq_driver = {
+	.name = "tegra186",
+	.flags = CPUFREQ_STICKY | CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
+	.verify = cpufreq_generic_frequency_table_verify,
+	.target_index = tegra186_cpufreq_set_target,
+	.init = tegra186_cpufreq_init,
+	.attr = cpufreq_generic_attr,
+};
+
+static int init_vhint_table(struct platform_device *pdev,
+			    struct tegra_bpmp *bpmp, uint32_t cluster_id,
+			    struct cpufreq_frequency_table **table)
+{
+	struct mrq_cpu_vhint_request req;
+	struct tegra_bpmp_message msg;
+	struct cpu_vhint_data *data;
+	int err, i, j, num_rates;
+	dma_addr_t phys;
+	void *virt;
+
+	virt = dma_alloc_coherent(bpmp->dev, MSG_DATA_MIN_SZ, &phys,
+				  GFP_KERNEL | GFP_DMA32);
+	if (!virt)
+		return -ENOMEM;
+
+	data = (struct cpu_vhint_data *)virt;
+
+	memset(&req, 0, sizeof(req));
+	req.addr = phys;
+	req.cluster_id = cluster_id;
+
+	memset(&msg, 0, sizeof(msg));
+	msg.mrq = MRQ_CPU_VHINT;
+	msg.tx.data = &req;
+	msg.tx.size = sizeof(req);
+
+	err = tegra_bpmp_transfer(bpmp, &msg);
+	if (err)
+		goto end;
+
+	num_rates = 0;
+
+	for (i = data->vfloor; i < data->vceil + 1; ++i) {
+		uint16_t ndiv = data->ndiv[i];
+
+		if (ndiv < data->ndiv_min || ndiv > data->ndiv_max)
+			continue;
+
+		/* Only store lowest voltage index for each rate */
+		if (i > 0 && ndiv == data->ndiv[i-1])
+			continue;
+
+		++num_rates;
+	}
+
+	*table = devm_kcalloc(&pdev->dev, num_rates + 1, sizeof(**table),
+			      GFP_KERNEL);
+	if (!*table) {
+		err = -ENOMEM;
+		goto end;
+	}
+
+	for (i = data->vfloor, j = 0; i < data->vceil + 1; ++i) {
+		struct cpufreq_frequency_table *point;
+		uint16_t ndiv = data->ndiv[i];
+
+		if (ndiv < data->ndiv_min || ndiv > data->ndiv_max)
+			continue;
+
+		/* Only store lowest voltage index for each rate */
+		if (i > 0 && ndiv == data->ndiv[i-1])
+			continue;
+
+		point = &(*table)[j++];
+		point->driver_data = (i << 16) | (ndiv);
+		point->frequency = data->ref_clk_hz * ndiv / data->pdiv /
+			data->mdiv / 1000;
+	}
+
+	(*table)[j].frequency = CPUFREQ_TABLE_END;
+
+end:
+	dma_free_coherent(bpmp->dev, MSG_DATA_MIN_SZ, virt, phys);
+
+	return err;
+}
+
+static int tegra186_cpufreq_probe(struct platform_device *pdev)
+{
+	struct tegra186_cpufreq_data *data;
+	struct tegra_bpmp *bpmp;
+	int i, err;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	bpmp = tegra_bpmp_get(&pdev->dev);
+	if (IS_ERR(bpmp))
+		return PTR_ERR(bpmp);
+
+	for (i = 0; i < NUM_CLUSTERS; ++i) {
+		struct resource *res;
+
+		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+						   CLUSTERS[i].name);
+		if (!res) {
+			err = -ENXIO;
+			goto put_bpmp;
+		}
+
+		data->regs[i] = devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(data->regs[i])) {
+			err = PTR_ERR(data->regs[i]);
+			goto put_bpmp;
+		}
+
+		err = init_vhint_table(pdev, bpmp, i, &data->tables[i]);
+		if (err)
+			goto put_bpmp;
+	}
+
+	tegra_bpmp_put(bpmp);
+
+	tegra186_cpufreq_driver.driver_data = data;
+
+	err = cpufreq_register_driver(&tegra186_cpufreq_driver);
+	if (err)
+		return err;
+
+	return 0;
+
+put_bpmp:
+	tegra_bpmp_put(bpmp);
+
+	return err;
+}
+
+static int tegra186_cpufreq_remove(struct platform_device *pdev)
+{
+	cpufreq_unregister_driver(&tegra186_cpufreq_driver);
+
+	return 0;
+}
+
+static const struct of_device_id tegra186_cpufreq_of_match[] = {
+	{ .compatible = "nvidia,tegra186-ccplex-cluster", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, tegra186_cpufreq_of_match);
+
+static struct platform_driver tegra186_cpufreq_platform_driver = {
+	.driver = {
+		.name = "tegra186-cpufreq",
+		.of_match_table = tegra186_cpufreq_of_match,
+	},
+	.probe = tegra186_cpufreq_probe,
+	.remove = tegra186_cpufreq_remove,
+};
+module_platform_driver(tegra186_cpufreq_platform_driver);
+
+MODULE_AUTHOR("Mikko Perttunen <mperttunen@nvidia.com>");
+MODULE_DESCRIPTION("Tegra186 cpufreq driver");
+MODULE_LICENSE("GPL v2");
-- 
2.1.4

[PATCH 1/3] cpufreq: Add Tegra186 cpufreq driver

[Mikko Perttunen]

Add a new cpufreq driver for Tegra186 (and likely later).
The CPUs are organized into two clusters, Denver and A57,
with two and four cores respectively. CPU frequency can be
adjusted by writing the desired rate divisor and a voltage
hint to a special per-core register.

The frequency of each core can be set individually; however,
this is just a hint as all CPUs in a cluster will run at
the maximum rate of non-idle CPUs in the cluster.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 drivers/cpufreq/Kconfig.arm        |   7 +
 drivers/cpufreq/Makefile           |   1 +
 drivers/cpufreq/tegra186-cpufreq.c | 277 +++++++++++++++++++++++++++++++++++++
 3 files changed, 285 insertions(+)
 create mode 100644 drivers/cpufreq/tegra186-cpufreq.c

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 74fa5c5904d3..168d36fa4a58 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -247,6 +247,13 @@ config ARM_TEGRA124_CPUFREQ
 	help
 	  This adds the CPUFreq driver support for Tegra124 SOCs.
 
+config ARM_TEGRA186_CPUFREQ
+	tristate "Tegra186 CPUFreq support"
+	depends on ARCH_TEGRA && TEGRA_BPMP
+	default y
+	help
+	  This adds the CPUFreq driver support for Tegra186 SOCs.
+
 config ARM_TI_CPUFREQ
 	bool "Texas Instruments CPUFreq support"
 	depends on ARCH_OMAP2PLUS
diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
index 9f5a8045f36d..b7e78f063c4f 100644
--- a/drivers/cpufreq/Makefile
+++ b/drivers/cpufreq/Makefile
@@ -77,6 +77,7 @@ obj-$(CONFIG_ARM_SPEAR_CPUFREQ)		+= spear-cpufreq.o
 obj-$(CONFIG_ARM_STI_CPUFREQ)		+= sti-cpufreq.o
 obj-$(CONFIG_ARM_TEGRA20_CPUFREQ)	+= tegra20-cpufreq.o
 obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
+obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)	+= tegra186-cpufreq.o
 obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
 obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
 obj-$(CONFIG_ACPI_CPPC_CPUFREQ) += cppc_cpufreq.o
diff --git a/drivers/cpufreq/tegra186-cpufreq.c b/drivers/cpufreq/tegra186-cpufreq.c
new file mode 100644
index 000000000000..794c1f2d8231
--- /dev/null
+++ b/drivers/cpufreq/tegra186-cpufreq.c
@@ -0,0 +1,277 @@
+/*
+ * Copyright (c) 2017, NVIDIA CORPORATION. All rights reserved
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/cpufreq.h>
+#include <linux/dma-mapping.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+
+#include <soc/tegra/bpmp.h>
+#include <soc/tegra/bpmp-abi.h>
+
+#define EDVD_CORE_VOLT_FREQ(core)		(0x20 + (core) * 0x4)
+#define EDVD_CORE_VOLT_FREQ_F_SHIFT		0
+#define EDVD_CORE_VOLT_FREQ_F_MASK		0xff
+#define EDVD_CORE_VOLT_FREQ_V_SHIFT		16
+#define EDVD_CORE_VOLT_FREQ_V_MASK		0xff
+
+#define CLUSTER_DENVER				0
+#define CLUSTER_A57				1
+#define NUM_CLUSTERS				2
+
+struct tegra186_cpufreq_cluster {
+	const char *name;
+	unsigned int num_cores;
+};
+
+static const struct tegra186_cpufreq_cluster CLUSTERS[] = {
+	{
+		.name = "denver",
+		.num_cores = 2,
+	},
+	{
+		.name = "a57",
+		.num_cores = 4,
+	}
+};
+
+struct tegra186_cpufreq_data {
+	void __iomem *regs[NUM_CLUSTERS];
+	struct cpufreq_frequency_table *tables[NUM_CLUSTERS];
+};
+
+static void get_cluster_core(int cpu, int *cluster, int *core)
+{
+	switch (cpu) {
+	case 0:
+		*cluster = CLUSTER_A57; *core = 0; break;
+	case 3:
+		*cluster = CLUSTER_A57; *core = 1; break;
+	case 4:
+		*cluster = CLUSTER_A57; *core = 2; break;
+	case 5:
+		*cluster = CLUSTER_A57; *core = 3; break;
+	case 1:
+		*cluster = CLUSTER_DENVER; *core = 0; break;
+	case 2:
+		*cluster = CLUSTER_DENVER; *core = 1; break;
+	}
+}
+
+static int tegra186_cpufreq_init(struct cpufreq_policy *policy)
+{
+	struct tegra186_cpufreq_data *data = cpufreq_get_driver_data();
+	struct cpufreq_frequency_table *table;
+	int cluster, core;
+
+	get_cluster_core(policy->cpu, &cluster, &core);
+
+	table = data->tables[cluster];
+	cpufreq_table_validate_and_show(policy, table);
+
+	policy->cpuinfo.transition_latency = 300 * 1000;
+
+	return 0;
+}
+
+static void write_volt_freq(uint8_t vidx, uint8_t ndiv, void __iomem *regs,
+			unsigned int core)
+{
+	u32 val = 0;
+
+	val |= ndiv << EDVD_CORE_VOLT_FREQ_F_SHIFT;
+	val |= vidx << EDVD_CORE_VOLT_FREQ_V_SHIFT;
+
+	writel(val, regs + EDVD_CORE_VOLT_FREQ(core));
+}
+
+static int tegra186_cpufreq_set_target(struct cpufreq_policy *policy,
+				       unsigned int index)
+{
+	struct cpufreq_frequency_table *tbl = policy->freq_table + index;
+	struct tegra186_cpufreq_data *data = cpufreq_get_driver_data();
+	uint16_t vidx = tbl->driver_data >> 16;
+	uint16_t ndiv = tbl->driver_data & 0xffff;
+	int cluster, core;
+
+	get_cluster_core(policy->cpu, &cluster, &core);
+	write_volt_freq(vidx, ndiv, data->regs[cluster], core);
+
+	return 0;
+}
+
+static struct cpufreq_driver tegra186_cpufreq_driver = {
+	.name = "tegra186",
+	.flags = CPUFREQ_STICKY | CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
+	.verify = cpufreq_generic_frequency_table_verify,
+	.target_index = tegra186_cpufreq_set_target,
+	.init = tegra186_cpufreq_init,
+	.attr = cpufreq_generic_attr,
+};
+
+static int init_vhint_table(struct platform_device *pdev,
+			    struct tegra_bpmp *bpmp, uint32_t cluster_id,
+			    struct cpufreq_frequency_table **table)
+{
+	struct mrq_cpu_vhint_request req;
+	struct tegra_bpmp_message msg;
+	struct cpu_vhint_data *data;
+	int err, i, j, num_rates;
+	dma_addr_t phys;
+	void *virt;
+
+	virt = dma_alloc_coherent(bpmp->dev, MSG_DATA_MIN_SZ, &phys,
+				  GFP_KERNEL | GFP_DMA32);
+	if (!virt)
+		return -ENOMEM;
+
+	data = (struct cpu_vhint_data *)virt;
+
+	memset(&req, 0, sizeof(req));
+	req.addr = phys;
+	req.cluster_id = cluster_id;
+
+	memset(&msg, 0, sizeof(msg));
+	msg.mrq = MRQ_CPU_VHINT;
+	msg.tx.data = &req;
+	msg.tx.size = sizeof(req);
+
+	err = tegra_bpmp_transfer(bpmp, &msg);
+	if (err)
+		goto end;
+
+	num_rates = 0;
+
+	for (i = data->vfloor; i < data->vceil + 1; ++i) {
+		uint16_t ndiv = data->ndiv[i];
+
+		if (ndiv < data->ndiv_min || ndiv > data->ndiv_max)
+			continue;
+
+		/* Only store lowest voltage index for each rate */
+		if (i > 0 && ndiv == data->ndiv[i-1])
+			continue;
+
+		++num_rates;
+	}
+
+	*table = devm_kcalloc(&pdev->dev, num_rates + 1, sizeof(**table),
+			      GFP_KERNEL);
+	if (!*table) {
+		err = -ENOMEM;
+		goto end;
+	}
+
+	for (i = data->vfloor, j = 0; i < data->vceil + 1; ++i) {
+		struct cpufreq_frequency_table *point;
+		uint16_t ndiv = data->ndiv[i];
+
+		if (ndiv < data->ndiv_min || ndiv > data->ndiv_max)
+			continue;
+
+		/* Only store lowest voltage index for each rate */
+		if (i > 0 && ndiv == data->ndiv[i-1])
+			continue;
+
+		point = &(*table)[j++];
+		point->driver_data = (i << 16) | (ndiv);
+		point->frequency = data->ref_clk_hz * ndiv / data->pdiv /
+			data->mdiv / 1000;
+	}
+
+	(*table)[j].frequency = CPUFREQ_TABLE_END;
+
+end:
+	dma_free_coherent(bpmp->dev, MSG_DATA_MIN_SZ, virt, phys);
+
+	return err;
+}
+
+static int tegra186_cpufreq_probe(struct platform_device *pdev)
+{
+	struct tegra186_cpufreq_data *data;
+	struct tegra_bpmp *bpmp;
+	int i, err;
+
+	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	bpmp = tegra_bpmp_get(&pdev->dev);
+	if (IS_ERR(bpmp))
+		return PTR_ERR(bpmp);
+
+	for (i = 0; i < NUM_CLUSTERS; ++i) {
+		struct resource *res;
+
+		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
+						   CLUSTERS[i].name);
+		if (!res) {
+			err = -ENXIO;
+			goto put_bpmp;
+		}
+
+		data->regs[i] = devm_ioremap_resource(&pdev->dev, res);
+		if (IS_ERR(data->regs[i])) {
+			err = PTR_ERR(data->regs[i]);
+			goto put_bpmp;
+		}
+
+		err = init_vhint_table(pdev, bpmp, i, &data->tables[i]);
+		if (err)
+			goto put_bpmp;
+	}
+
+	tegra_bpmp_put(bpmp);
+
+	tegra186_cpufreq_driver.driver_data = data;
+
+	err = cpufreq_register_driver(&tegra186_cpufreq_driver);
+	if (err)
+		return err;
+
+	return 0;
+
+put_bpmp:
+	tegra_bpmp_put(bpmp);
+
+	return err;
+}
+
+static int tegra186_cpufreq_remove(struct platform_device *pdev)
+{
+	cpufreq_unregister_driver(&tegra186_cpufreq_driver);
+
+	return 0;
+}
+
+static const struct of_device_id tegra186_cpufreq_of_match[] = {
+	{ .compatible = "nvidia,tegra186-ccplex-cluster", },
+	{ }
+};
+MODULE_DEVICE_TABLE(of, tegra186_cpufreq_of_match);
+
+static struct platform_driver tegra186_cpufreq_platform_driver = {
+	.driver = {
+		.name = "tegra186-cpufreq",
+		.of_match_table = tegra186_cpufreq_of_match,
+	},
+	.probe = tegra186_cpufreq_probe,
+	.remove = tegra186_cpufreq_remove,
+};
+module_platform_driver(tegra186_cpufreq_platform_driver);
+
+MODULE_AUTHOR("Mikko Perttunen <mperttunen@nvidia.com>");
+MODULE_DESCRIPTION("Tegra186 cpufreq driver");
+MODULE_LICENSE("GPL v2");
-- 
2.1.4

[PATCH 2/3] dt-bindings: Add bindings for nvidia,tegra186-ccplex-cluster

[Mikko Perttunen]

The Tegra186 CCPLEX_CLUSTER area contains memory-mapped
registers that initiate CPU frequency/voltage transitions.

Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
---
 .../arm/tegra/nvidia,tegra186-ccplex-cluster.txt   | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt

diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt
new file mode 100644
index 000000000000..50cd615219e9
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt
@@ -0,0 +1,22 @@
+NVIDIA Tegra CCPLEX_CLUSTER area
+
+Required properties:
+- compatible: Should contain one of the following:
+  - "nvidia,tegra186-ccplex-cluster": for Tegra186
+- reg: Must contain an (offset, length) pair of the register set for each
+  entry in reg-names.
+- reg-names: Must include the following entries:
+  - "a57": Public aperture for A57 CPU cluster
+  - "denver": Public aperture for Denver CPU cluster
+- nvidia,bpmp: Phandle to BPMP device that can be queried for OPP tables
+
+Example:
+
+	ccplex@e000000 {
+		compatible = "nvidia,tegra186-ccplex-cluster";
+		reg = <0x0 0x0e060000 0x0 0x1000>,
+		      <0x0 0x0e070000 0x0 0x1000>;
+		reg-names = "a57", "denver";
+
+		nvidia,bpmp = <&bpmp>;
+	};
-- 
2.1.4

[PATCH 2/3] dt-bindings: Add bindings for nvidia,tegra186-ccplex-cluster

[Mikko Perttunen]

The Tegra186 CCPLEX_CLUSTER area contains memory-mapped
registers that initiate CPU frequency/voltage transitions.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 .../arm/tegra/nvidia,tegra186-ccplex-cluster.txt   | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt

diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt
new file mode 100644
index 000000000000..50cd615219e9
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt
@@ -0,0 +1,22 @@
+NVIDIA Tegra CCPLEX_CLUSTER area
+
+Required properties:
+- compatible: Should contain one of the following:
+  - "nvidia,tegra186-ccplex-cluster": for Tegra186
+- reg: Must contain an (offset, length) pair of the register set for each
+  entry in reg-names.
+- reg-names: Must include the following entries:
+  - "a57": Public aperture for A57 CPU cluster
+  - "denver": Public aperture for Denver CPU cluster
+- nvidia,bpmp: Phandle to BPMP device that can be queried for OPP tables
+
+Example:
+
+	ccplex@e000000 {
+		compatible = "nvidia,tegra186-ccplex-cluster";
+		reg = <0x0 0x0e060000 0x0 0x1000>,
+		      <0x0 0x0e070000 0x0 0x1000>;
+		reg-names = "a57", "denver";
+
+		nvidia,bpmp = <&bpmp>;
+	};
-- 
2.1.4

[PATCH 3/3] arm64: tegra: Add CCPLEX_CLUSTER area in Tegra186

[Mikko Perttunen]

The Tegra186 CCPLEX_CLUSTER area contains memory-mapped
registers that initiate CPU frequency/voltage transitions.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 arch/arm64/boot/dts/nvidia/tegra186.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
index 3ea5e6369bc3..765b840933ac 100644
--- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
@@ -347,6 +347,15 @@
 		reg-names = "pmc", "wake", "aotag", "scratch";
 	};
 
+	ccplex@e000000 {
+		compatible = "nvidia,tegra186-ccplex-cluster";
+		reg = <0x0 0x0e060000 0x0 0x1000>,
+		      <0x0 0x0e070000 0x0 0x1000>;
+		reg-names = "a57", "denver";
+
+		nvidia,bpmp = <&bpmp>;
+	};
+
 	sysram@30000000 {
 		compatible = "nvidia,tegra186-sysram", "mmio-sram";
 		reg = <0x0 0x30000000 0x0 0x50000>;
-- 
2.1.4

[PATCH 3/3] arm64: tegra: Add CCPLEX_CLUSTER area in Tegra186

[Mikko Perttunen]

The Tegra186 CCPLEX_CLUSTER area contains memory-mapped
registers that initiate CPU frequency/voltage transitions.

Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
---
 arch/arm64/boot/dts/nvidia/tegra186.dtsi | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/arch/arm64/boot/dts/nvidia/tegra186.dtsi b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
index 3ea5e6369bc3..765b840933ac 100644
--- a/arch/arm64/boot/dts/nvidia/tegra186.dtsi
+++ b/arch/arm64/boot/dts/nvidia/tegra186.dtsi
@@ -347,6 +347,15 @@
 		reg-names = "pmc", "wake", "aotag", "scratch";
 	};
 
+	ccplex@e000000 {
+		compatible = "nvidia,tegra186-ccplex-cluster";
+		reg = <0x0 0x0e060000 0x0 0x1000>,
+		      <0x0 0x0e070000 0x0 0x1000>;
+		reg-names = "a57", "denver";
+
+		nvidia,bpmp = <&bpmp>;
+	};
+
 	sysram@30000000 {
 		compatible = "nvidia,tegra186-sysram", "mmio-sram";
 		reg = <0x0 0x30000000 0x0 0x50000>;
-- 
2.1.4

Re: [PATCH 2/3] dt-bindings: Add bindings for nvidia,tegra186-ccplex-cluster

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 1795 bytes --]

On Mon, Apr 03, 2017 at 03:42:24PM +0300, Mikko Perttunen wrote:
> The Tegra186 CCPLEX_CLUSTER area contains memory-mapped
> registers that initiate CPU frequency/voltage transitions.
> 
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
>  .../arm/tegra/nvidia,tegra186-ccplex-cluster.txt   | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt
> new file mode 100644
> index 000000000000..50cd615219e9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt
> @@ -0,0 +1,22 @@
> +NVIDIA Tegra CCPLEX_CLUSTER area
> +
> +Required properties:
> +- compatible: Should contain one of the following:
> +  - "nvidia,tegra186-ccplex-cluster": for Tegra186
> +- reg: Must contain an (offset, length) pair of the register set for each
> +  entry in reg-names.
> +- reg-names: Must include the following entries:
> +  - "a57": Public aperture for A57 CPU cluster
> +  - "denver": Public aperture for Denver CPU cluster
> +- nvidia,bpmp: Phandle to BPMP device that can be queried for OPP tables

"phandle"

> +Example:
> +
> +	ccplex@e000000 {
> +		compatible = "nvidia,tegra186-ccplex-cluster";
> +		reg = <0x0 0x0e060000 0x0 0x1000>,
> +		      <0x0 0x0e070000 0x0 0x1000>;
> +		reg-names = "a57", "denver";
> +
> +		nvidia,bpmp = <&bpmp>;
> +	};

Where's the information about the register offsets coming from? The TRM
says that CCPLEX_CLUSTER has a single aperture from 0x0e000000 to
0x0e3fffff.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/3] dt-bindings: Add bindings for nvidia,tegra186-ccplex-cluster

[Jon Hunter]

On 03/04/17 13:42, Mikko Perttunen wrote:
> The Tegra186 CCPLEX_CLUSTER area contains memory-mapped
> registers that initiate CPU frequency/voltage transitions.
> 
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
>  .../arm/tegra/nvidia,tegra186-ccplex-cluster.txt   | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt
> new file mode 100644
> index 000000000000..50cd615219e9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt
> @@ -0,0 +1,22 @@
> +NVIDIA Tegra CCPLEX_CLUSTER area
> +
> +Required properties:
> +- compatible: Should contain one of the following:
> +  - "nvidia,tegra186-ccplex-cluster": for Tegra186

Nit pick ... any reason why we append 'cluster' here? The TRM just says
the "CPU Complex" consists of two CPU clusters. So
"nvidia,tegra186-cpu-complex" or "nvidia,tegra186-ccplex" seems fine.

BTW, I do see references in the TRM to CCPLEX_CLUSTER0/1, but never
CCPLEX_CLUSTER in reference to both. I think it is just CCPLEX AFAICT.

Cheers
Jon

-- 
nvpublic

Re: [PATCH 2/3] dt-bindings: Add bindings for nvidia,tegra186-ccplex-cluster

[Jon Hunter]

On 03/04/17 13:42, Mikko Perttunen wrote:
> The Tegra186 CCPLEX_CLUSTER area contains memory-mapped
> registers that initiate CPU frequency/voltage transitions.
> 
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
>  .../arm/tegra/nvidia,tegra186-ccplex-cluster.txt   | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt
> new file mode 100644
> index 000000000000..50cd615219e9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt
> @@ -0,0 +1,22 @@
> +NVIDIA Tegra CCPLEX_CLUSTER area
> +
> +Required properties:
> +- compatible: Should contain one of the following:
> +  - "nvidia,tegra186-ccplex-cluster": for Tegra186

Nit pick ... any reason why we append 'cluster' here? The TRM just says
the "CPU Complex" consists of two CPU clusters. So
"nvidia,tegra186-cpu-complex" or "nvidia,tegra186-ccplex" seems fine.

BTW, I do see references in the TRM to CCPLEX_CLUSTER0/1, but never
CCPLEX_CLUSTER in reference to both. I think it is just CCPLEX AFAICT.

Cheers
Jon

-- 
nvpublic

Re: [PATCH 1/3] cpufreq: Add Tegra186 cpufreq driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 13057 bytes --]

On Mon, Apr 03, 2017 at 03:42:23PM +0300, Mikko Perttunen wrote:
> Add a new cpufreq driver for Tegra186 (and likely later).
> The CPUs are organized into two clusters, Denver and A57,
> with two and four cores respectively. CPU frequency can be
> adjusted by writing the desired rate divisor and a voltage
> hint to a special per-core register.
> 
> The frequency of each core can be set individually; however,
> this is just a hint as all CPUs in a cluster will run at
> the maximum rate of non-idle CPUs in the cluster.
> 
> Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/cpufreq/Kconfig.arm        |   7 +
>  drivers/cpufreq/Makefile           |   1 +
>  drivers/cpufreq/tegra186-cpufreq.c | 277 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 285 insertions(+)
>  create mode 100644 drivers/cpufreq/tegra186-cpufreq.c
> 
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 74fa5c5904d3..168d36fa4a58 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -247,6 +247,13 @@ config ARM_TEGRA124_CPUFREQ
>  	help
>  	  This adds the CPUFreq driver support for Tegra124 SOCs.
>  
> +config ARM_TEGRA186_CPUFREQ
> +	tristate "Tegra186 CPUFreq support"
> +	depends on ARCH_TEGRA && TEGRA_BPMP
> +	default y

I'd rather not default this to "y". We can use the defconfig to enable
this.

> +	help
> +	  This adds the CPUFreq driver support for Tegra186 SOCs.
> +
>  config ARM_TI_CPUFREQ
>  	bool "Texas Instruments CPUFreq support"
>  	depends on ARCH_OMAP2PLUS
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index 9f5a8045f36d..b7e78f063c4f 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -77,6 +77,7 @@ obj-$(CONFIG_ARM_SPEAR_CPUFREQ)		+= spear-cpufreq.o
>  obj-$(CONFIG_ARM_STI_CPUFREQ)		+= sti-cpufreq.o
>  obj-$(CONFIG_ARM_TEGRA20_CPUFREQ)	+= tegra20-cpufreq.o
>  obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
> +obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)	+= tegra186-cpufreq.o
>  obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
>  obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
>  obj-$(CONFIG_ACPI_CPPC_CPUFREQ) += cppc_cpufreq.o
> diff --git a/drivers/cpufreq/tegra186-cpufreq.c b/drivers/cpufreq/tegra186-cpufreq.c
> new file mode 100644
> index 000000000000..794c1f2d8231
> --- /dev/null
> +++ b/drivers/cpufreq/tegra186-cpufreq.c
> @@ -0,0 +1,277 @@
> +/*
> + * Copyright (c) 2017, NVIDIA CORPORATION. All rights reserved
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +#include <linux/cpufreq.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +#include <soc/tegra/bpmp.h>
> +#include <soc/tegra/bpmp-abi.h>
> +
> +#define EDVD_CORE_VOLT_FREQ(core)		(0x20 + (core) * 0x4)
> +#define EDVD_CORE_VOLT_FREQ_F_SHIFT		0
> +#define EDVD_CORE_VOLT_FREQ_F_MASK		0xff
> +#define EDVD_CORE_VOLT_FREQ_V_SHIFT		16
> +#define EDVD_CORE_VOLT_FREQ_V_MASK		0xff
> +
> +#define CLUSTER_DENVER				0
> +#define CLUSTER_A57				1
> +#define NUM_CLUSTERS				2
> +
> +struct tegra186_cpufreq_cluster {
> +	const char *name;
> +	unsigned int num_cores;
> +};
> +
> +static const struct tegra186_cpufreq_cluster CLUSTERS[] = {

We don't usually use uppercase letters for variable names.

> +	{
> +		.name = "denver",
> +		.num_cores = 2,
> +	},
> +	{
> +		.name = "a57",
> +		.num_cores = 4,
> +	}
> +};
> +
> +struct tegra186_cpufreq_data {
> +	void __iomem *regs[NUM_CLUSTERS];
> +	struct cpufreq_frequency_table *tables[NUM_CLUSTERS];
> +};

Given my comments regarding the aperture, perhaps it would be useful to
only have a single range of memory-mapped registers and add an offset to
struct tegra186_cpufreq_cluster that points into that region?

Also, you're somewhat mixing arrays of NUM_CLUSTERS elements and
dynamically sized ones (CLUSTERS[]). Probably best to just stick to one
of them.

Might be worth considering to dynamically allocate based on the cluster
table, but that could be done in a separate patch if we ever get a
configuration where NUM_CLUSTERS != 2.

> +static void get_cluster_core(int cpu, int *cluster, int *core)

These can all be unsigned int.

> +{
> +	switch (cpu) {
> +	case 0:
> +		*cluster = CLUSTER_A57; *core = 0; break;
> +	case 3:
> +		*cluster = CLUSTER_A57; *core = 1; break;
> +	case 4:
> +		*cluster = CLUSTER_A57; *core = 2; break;
> +	case 5:
> +		*cluster = CLUSTER_A57; *core = 3; break;
> +	case 1:
> +		*cluster = CLUSTER_DENVER; *core = 0; break;
> +	case 2:
> +		*cluster = CLUSTER_DENVER; *core = 1; break;
> +	}
> +}

This is weirdly formatted. Also, what if cpu > 5? Do we need to be
careful and WARN_ON()?

Perhaps in order to make this more easily extensible this could go
into struct tegra186_cpufreq_cluster? Or a separate table that has
information about the cluster and a list of CPUs?

Again, probably not necessary right away, but something to keep in
mind for parameterization if we ever need to support other configs.

> +static int tegra186_cpufreq_init(struct cpufreq_policy *policy)
> +{
> +	struct tegra186_cpufreq_data *data = cpufreq_get_driver_data();
> +	struct cpufreq_frequency_table *table;
> +	int cluster, core;
> +
> +	get_cluster_core(policy->cpu, &cluster, &core);
> +
> +	table = data->tables[cluster];
> +	cpufreq_table_validate_and_show(policy, table);
> +
> +	policy->cpuinfo.transition_latency = 300 * 1000;
> +
> +	return 0;
> +}
> +
> +static void write_volt_freq(uint8_t vidx, uint8_t ndiv, void __iomem *regs,

uint8_t -> u8

> +			unsigned int core)
> +{
> +	u32 val = 0;
> +
> +	val |= ndiv << EDVD_CORE_VOLT_FREQ_F_SHIFT;
> +	val |= vidx << EDVD_CORE_VOLT_FREQ_V_SHIFT;
> +
> +	writel(val, regs + EDVD_CORE_VOLT_FREQ(core));
> +}
> +
> +static int tegra186_cpufreq_set_target(struct cpufreq_policy *policy,
> +				       unsigned int index)
> +{
> +	struct cpufreq_frequency_table *tbl = policy->freq_table + index;
> +	struct tegra186_cpufreq_data *data = cpufreq_get_driver_data();
> +	uint16_t vidx = tbl->driver_data >> 16;
> +	uint16_t ndiv = tbl->driver_data & 0xffff;

uint16_t -> u16

Also it's slightly strange that you store u16 here and pass it to a
function that takes

> +	int cluster, core;
> +
> +	get_cluster_core(policy->cpu, &cluster, &core);
> +	write_volt_freq(vidx, ndiv, data->regs[cluster], core);
> +
> +	return 0;
> +}
> +
> +static struct cpufreq_driver tegra186_cpufreq_driver = {
> +	.name = "tegra186",
> +	.flags = CPUFREQ_STICKY | CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
> +	.verify = cpufreq_generic_frequency_table_verify,
> +	.target_index = tegra186_cpufreq_set_target,
> +	.init = tegra186_cpufreq_init,
> +	.attr = cpufreq_generic_attr,
> +};
> +
> +static int init_vhint_table(struct platform_device *pdev,
> +			    struct tegra_bpmp *bpmp, uint32_t cluster_id,
> +			    struct cpufreq_frequency_table **table)
> +{
> +	struct mrq_cpu_vhint_request req;
> +	struct tegra_bpmp_message msg;
> +	struct cpu_vhint_data *data;
> +	int err, i, j, num_rates;
> +	dma_addr_t phys;
> +	void *virt;
> +
> +	virt = dma_alloc_coherent(bpmp->dev, MSG_DATA_MIN_SZ, &phys,
> +				  GFP_KERNEL | GFP_DMA32);
> +	if (!virt)
> +		return -ENOMEM;

This seems wrong. MSG_DATA_MIN_SZ is 120 but struct cpu_vhint_data is a
lot larger than that (836 bytes, if I'm not mistaken). It probably works
because dma_alloc_coherent() will always give you at least a whole page,
but you should still use the correct size here.

> +
> +	data = (struct cpu_vhint_data *)virt;
> +
> +	memset(&req, 0, sizeof(req));
> +	req.addr = phys;
> +	req.cluster_id = cluster_id;
> +
> +	memset(&msg, 0, sizeof(msg));
> +	msg.mrq = MRQ_CPU_VHINT;
> +	msg.tx.data = &req;
> +	msg.tx.size = sizeof(req);
> +
> +	err = tegra_bpmp_transfer(bpmp, &msg);
> +	if (err)
> +		goto end;
> +
> +	num_rates = 0;

This could be initialized when it is declared.

> +
> +	for (i = data->vfloor; i < data->vceil + 1; ++i) {

i <= data->vceil? That seems more intuitive to me. Also, please only use
pre-decrement if really necessary.

> +		uint16_t ndiv = data->ndiv[i];
> +
> +		if (ndiv < data->ndiv_min || ndiv > data->ndiv_max)
> +			continue;
> +
> +		/* Only store lowest voltage index for each rate */
> +		if (i > 0 && ndiv == data->ndiv[i-1])

Needs spaces around '-', I think checkpatch would complain otherwise.

Also, why is this even happening? Why would MRQ_CPU_VHINT return the
same ndiv value twice?

> +			continue;
> +
> +		++num_rates;

Again, post-increment is preferred over pre-increment.

> +	}
> +
> +	*table = devm_kcalloc(&pdev->dev, num_rates + 1, sizeof(**table),
> +			      GFP_KERNEL);

There's a lot of dereferencing here and in the code below. Why not
simply return a struct cpufreq_frequency_table * from this function, and
an ERR_PTR()-encoded error on failure, rather than returning this in a
parameter, requiring the repeated deref?

> +	if (!*table) {
> +		err = -ENOMEM;
> +		goto end;
> +	}
> +
> +	for (i = data->vfloor, j = 0; i < data->vceil + 1; ++i) {
> +		struct cpufreq_frequency_table *point;
> +		uint16_t ndiv = data->ndiv[i];
> +
> +		if (ndiv < data->ndiv_min || ndiv > data->ndiv_max)
> +			continue;
> +
> +		/* Only store lowest voltage index for each rate */
> +		if (i > 0 && ndiv == data->ndiv[i-1])
> +			continue;
> +
> +		point = &(*table)[j++];
> +		point->driver_data = (i << 16) | (ndiv);
> +		point->frequency = data->ref_clk_hz * ndiv / data->pdiv /
> +			data->mdiv / 1000;
> +	}
> +
> +	(*table)[j].frequency = CPUFREQ_TABLE_END;
> +
> +end:

free: would be a more accurate name for this label.

> +	dma_free_coherent(bpmp->dev, MSG_DATA_MIN_SZ, virt, phys);
> +
> +	return err;
> +}
> +
> +static int tegra186_cpufreq_probe(struct platform_device *pdev)
> +{
> +	struct tegra186_cpufreq_data *data;
> +	struct tegra_bpmp *bpmp;
> +	int i, err;

unsigned int i?

> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	bpmp = tegra_bpmp_get(&pdev->dev);
> +	if (IS_ERR(bpmp))
> +		return PTR_ERR(bpmp);
> +
> +	for (i = 0; i < NUM_CLUSTERS; ++i) {
> +		struct resource *res;
> +
> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +						   CLUSTERS[i].name);
> +		if (!res) {
> +			err = -ENXIO;
> +			goto put_bpmp;
> +		}

No need for this check, devm_ioremap_resource() will take care of it.

> +
> +		data->regs[i] = devm_ioremap_resource(&pdev->dev, res);
> +		if (IS_ERR(data->regs[i])) {
> +			err = PTR_ERR(data->regs[i]);
> +			goto put_bpmp;
> +		}
> +
> +		err = init_vhint_table(pdev, bpmp, i, &data->tables[i]);
> +		if (err)
> +			goto put_bpmp;

This could become something along the lines of:

		data->tables[i] = init_vhint_table(pdev, bpmp, i);
		if (IS_ERR(data->tables[i])) {
			err = PTR_ERR(data->tables[i]);
			goto put_bpmp;
		}

Which is slightly more verbose than the original, but you get a lot more
clarity in return because you can just deal with straight pointers above
in init_vhint_table().

> +	}
> +
> +	tegra_bpmp_put(bpmp);
> +
> +	tegra186_cpufreq_driver.driver_data = data;
> +
> +	err = cpufreq_register_driver(&tegra186_cpufreq_driver);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +
> +put_bpmp:
> +	tegra_bpmp_put(bpmp);
> +
> +	return err;
> +}
> +
> +static int tegra186_cpufreq_remove(struct platform_device *pdev)
> +{
> +	cpufreq_unregister_driver(&tegra186_cpufreq_driver);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id tegra186_cpufreq_of_match[] = {
> +	{ .compatible = "nvidia,tegra186-ccplex-cluster", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, tegra186_cpufreq_of_match);
> +
> +static struct platform_driver tegra186_cpufreq_platform_driver = {
> +	.driver = {
> +		.name = "tegra186-cpufreq",
> +		.of_match_table = tegra186_cpufreq_of_match,
> +	},
> +	.probe = tegra186_cpufreq_probe,
> +	.remove = tegra186_cpufreq_remove,
> +};
> +module_platform_driver(tegra186_cpufreq_platform_driver);
> +
> +MODULE_AUTHOR("Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>");
> +MODULE_DESCRIPTION("Tegra186 cpufreq driver");

NVIDIA Tegra186?

I very much like how simple this driver is compared to previous
generations.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] cpufreq: Add Tegra186 cpufreq driver

[Thierry Reding]

[-- Attachment #1: Type: text/plain, Size: 12999 bytes --]

On Mon, Apr 03, 2017 at 03:42:23PM +0300, Mikko Perttunen wrote:
> Add a new cpufreq driver for Tegra186 (and likely later).
> The CPUs are organized into two clusters, Denver and A57,
> with two and four cores respectively. CPU frequency can be
> adjusted by writing the desired rate divisor and a voltage
> hint to a special per-core register.
> 
> The frequency of each core can be set individually; however,
> this is just a hint as all CPUs in a cluster will run at
> the maximum rate of non-idle CPUs in the cluster.
> 
> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
> ---
>  drivers/cpufreq/Kconfig.arm        |   7 +
>  drivers/cpufreq/Makefile           |   1 +
>  drivers/cpufreq/tegra186-cpufreq.c | 277 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 285 insertions(+)
>  create mode 100644 drivers/cpufreq/tegra186-cpufreq.c
> 
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 74fa5c5904d3..168d36fa4a58 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -247,6 +247,13 @@ config ARM_TEGRA124_CPUFREQ
>  	help
>  	  This adds the CPUFreq driver support for Tegra124 SOCs.
>  
> +config ARM_TEGRA186_CPUFREQ
> +	tristate "Tegra186 CPUFreq support"
> +	depends on ARCH_TEGRA && TEGRA_BPMP
> +	default y

I'd rather not default this to "y". We can use the defconfig to enable
this.

> +	help
> +	  This adds the CPUFreq driver support for Tegra186 SOCs.
> +
>  config ARM_TI_CPUFREQ
>  	bool "Texas Instruments CPUFreq support"
>  	depends on ARCH_OMAP2PLUS
> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
> index 9f5a8045f36d..b7e78f063c4f 100644
> --- a/drivers/cpufreq/Makefile
> +++ b/drivers/cpufreq/Makefile
> @@ -77,6 +77,7 @@ obj-$(CONFIG_ARM_SPEAR_CPUFREQ)		+= spear-cpufreq.o
>  obj-$(CONFIG_ARM_STI_CPUFREQ)		+= sti-cpufreq.o
>  obj-$(CONFIG_ARM_TEGRA20_CPUFREQ)	+= tegra20-cpufreq.o
>  obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
> +obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)	+= tegra186-cpufreq.o
>  obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
>  obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
>  obj-$(CONFIG_ACPI_CPPC_CPUFREQ) += cppc_cpufreq.o
> diff --git a/drivers/cpufreq/tegra186-cpufreq.c b/drivers/cpufreq/tegra186-cpufreq.c
> new file mode 100644
> index 000000000000..794c1f2d8231
> --- /dev/null
> +++ b/drivers/cpufreq/tegra186-cpufreq.c
> @@ -0,0 +1,277 @@
> +/*
> + * Copyright (c) 2017, NVIDIA CORPORATION. All rights reserved
> + *
> + * This program is free software; you can redistribute it and/or modify it
> + * under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + */
> +
> +#include <linux/cpufreq.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +
> +#include <soc/tegra/bpmp.h>
> +#include <soc/tegra/bpmp-abi.h>
> +
> +#define EDVD_CORE_VOLT_FREQ(core)		(0x20 + (core) * 0x4)
> +#define EDVD_CORE_VOLT_FREQ_F_SHIFT		0
> +#define EDVD_CORE_VOLT_FREQ_F_MASK		0xff
> +#define EDVD_CORE_VOLT_FREQ_V_SHIFT		16
> +#define EDVD_CORE_VOLT_FREQ_V_MASK		0xff
> +
> +#define CLUSTER_DENVER				0
> +#define CLUSTER_A57				1
> +#define NUM_CLUSTERS				2
> +
> +struct tegra186_cpufreq_cluster {
> +	const char *name;
> +	unsigned int num_cores;
> +};
> +
> +static const struct tegra186_cpufreq_cluster CLUSTERS[] = {

We don't usually use uppercase letters for variable names.

> +	{
> +		.name = "denver",
> +		.num_cores = 2,
> +	},
> +	{
> +		.name = "a57",
> +		.num_cores = 4,
> +	}
> +};
> +
> +struct tegra186_cpufreq_data {
> +	void __iomem *regs[NUM_CLUSTERS];
> +	struct cpufreq_frequency_table *tables[NUM_CLUSTERS];
> +};

Given my comments regarding the aperture, perhaps it would be useful to
only have a single range of memory-mapped registers and add an offset to
struct tegra186_cpufreq_cluster that points into that region?

Also, you're somewhat mixing arrays of NUM_CLUSTERS elements and
dynamically sized ones (CLUSTERS[]). Probably best to just stick to one
of them.

Might be worth considering to dynamically allocate based on the cluster
table, but that could be done in a separate patch if we ever get a
configuration where NUM_CLUSTERS != 2.

> +static void get_cluster_core(int cpu, int *cluster, int *core)

These can all be unsigned int.

> +{
> +	switch (cpu) {
> +	case 0:
> +		*cluster = CLUSTER_A57; *core = 0; break;
> +	case 3:
> +		*cluster = CLUSTER_A57; *core = 1; break;
> +	case 4:
> +		*cluster = CLUSTER_A57; *core = 2; break;
> +	case 5:
> +		*cluster = CLUSTER_A57; *core = 3; break;
> +	case 1:
> +		*cluster = CLUSTER_DENVER; *core = 0; break;
> +	case 2:
> +		*cluster = CLUSTER_DENVER; *core = 1; break;
> +	}
> +}

This is weirdly formatted. Also, what if cpu > 5? Do we need to be
careful and WARN_ON()?

Perhaps in order to make this more easily extensible this could go
into struct tegra186_cpufreq_cluster? Or a separate table that has
information about the cluster and a list of CPUs?

Again, probably not necessary right away, but something to keep in
mind for parameterization if we ever need to support other configs.

> +static int tegra186_cpufreq_init(struct cpufreq_policy *policy)
> +{
> +	struct tegra186_cpufreq_data *data = cpufreq_get_driver_data();
> +	struct cpufreq_frequency_table *table;
> +	int cluster, core;
> +
> +	get_cluster_core(policy->cpu, &cluster, &core);
> +
> +	table = data->tables[cluster];
> +	cpufreq_table_validate_and_show(policy, table);
> +
> +	policy->cpuinfo.transition_latency = 300 * 1000;
> +
> +	return 0;
> +}
> +
> +static void write_volt_freq(uint8_t vidx, uint8_t ndiv, void __iomem *regs,

uint8_t -> u8

> +			unsigned int core)
> +{
> +	u32 val = 0;
> +
> +	val |= ndiv << EDVD_CORE_VOLT_FREQ_F_SHIFT;
> +	val |= vidx << EDVD_CORE_VOLT_FREQ_V_SHIFT;
> +
> +	writel(val, regs + EDVD_CORE_VOLT_FREQ(core));
> +}
> +
> +static int tegra186_cpufreq_set_target(struct cpufreq_policy *policy,
> +				       unsigned int index)
> +{
> +	struct cpufreq_frequency_table *tbl = policy->freq_table + index;
> +	struct tegra186_cpufreq_data *data = cpufreq_get_driver_data();
> +	uint16_t vidx = tbl->driver_data >> 16;
> +	uint16_t ndiv = tbl->driver_data & 0xffff;

uint16_t -> u16

Also it's slightly strange that you store u16 here and pass it to a
function that takes

> +	int cluster, core;
> +
> +	get_cluster_core(policy->cpu, &cluster, &core);
> +	write_volt_freq(vidx, ndiv, data->regs[cluster], core);
> +
> +	return 0;
> +}
> +
> +static struct cpufreq_driver tegra186_cpufreq_driver = {
> +	.name = "tegra186",
> +	.flags = CPUFREQ_STICKY | CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
> +	.verify = cpufreq_generic_frequency_table_verify,
> +	.target_index = tegra186_cpufreq_set_target,
> +	.init = tegra186_cpufreq_init,
> +	.attr = cpufreq_generic_attr,
> +};
> +
> +static int init_vhint_table(struct platform_device *pdev,
> +			    struct tegra_bpmp *bpmp, uint32_t cluster_id,
> +			    struct cpufreq_frequency_table **table)
> +{
> +	struct mrq_cpu_vhint_request req;
> +	struct tegra_bpmp_message msg;
> +	struct cpu_vhint_data *data;
> +	int err, i, j, num_rates;
> +	dma_addr_t phys;
> +	void *virt;
> +
> +	virt = dma_alloc_coherent(bpmp->dev, MSG_DATA_MIN_SZ, &phys,
> +				  GFP_KERNEL | GFP_DMA32);
> +	if (!virt)
> +		return -ENOMEM;

This seems wrong. MSG_DATA_MIN_SZ is 120 but struct cpu_vhint_data is a
lot larger than that (836 bytes, if I'm not mistaken). It probably works
because dma_alloc_coherent() will always give you at least a whole page,
but you should still use the correct size here.

> +
> +	data = (struct cpu_vhint_data *)virt;
> +
> +	memset(&req, 0, sizeof(req));
> +	req.addr = phys;
> +	req.cluster_id = cluster_id;
> +
> +	memset(&msg, 0, sizeof(msg));
> +	msg.mrq = MRQ_CPU_VHINT;
> +	msg.tx.data = &req;
> +	msg.tx.size = sizeof(req);
> +
> +	err = tegra_bpmp_transfer(bpmp, &msg);
> +	if (err)
> +		goto end;
> +
> +	num_rates = 0;

This could be initialized when it is declared.

> +
> +	for (i = data->vfloor; i < data->vceil + 1; ++i) {

i <= data->vceil? That seems more intuitive to me. Also, please only use
pre-decrement if really necessary.

> +		uint16_t ndiv = data->ndiv[i];
> +
> +		if (ndiv < data->ndiv_min || ndiv > data->ndiv_max)
> +			continue;
> +
> +		/* Only store lowest voltage index for each rate */
> +		if (i > 0 && ndiv == data->ndiv[i-1])

Needs spaces around '-', I think checkpatch would complain otherwise.

Also, why is this even happening? Why would MRQ_CPU_VHINT return the
same ndiv value twice?

> +			continue;
> +
> +		++num_rates;

Again, post-increment is preferred over pre-increment.

> +	}
> +
> +	*table = devm_kcalloc(&pdev->dev, num_rates + 1, sizeof(**table),
> +			      GFP_KERNEL);

There's a lot of dereferencing here and in the code below. Why not
simply return a struct cpufreq_frequency_table * from this function, and
an ERR_PTR()-encoded error on failure, rather than returning this in a
parameter, requiring the repeated deref?

> +	if (!*table) {
> +		err = -ENOMEM;
> +		goto end;
> +	}
> +
> +	for (i = data->vfloor, j = 0; i < data->vceil + 1; ++i) {
> +		struct cpufreq_frequency_table *point;
> +		uint16_t ndiv = data->ndiv[i];
> +
> +		if (ndiv < data->ndiv_min || ndiv > data->ndiv_max)
> +			continue;
> +
> +		/* Only store lowest voltage index for each rate */
> +		if (i > 0 && ndiv == data->ndiv[i-1])
> +			continue;
> +
> +		point = &(*table)[j++];
> +		point->driver_data = (i << 16) | (ndiv);
> +		point->frequency = data->ref_clk_hz * ndiv / data->pdiv /
> +			data->mdiv / 1000;
> +	}
> +
> +	(*table)[j].frequency = CPUFREQ_TABLE_END;
> +
> +end:

free: would be a more accurate name for this label.

> +	dma_free_coherent(bpmp->dev, MSG_DATA_MIN_SZ, virt, phys);
> +
> +	return err;
> +}
> +
> +static int tegra186_cpufreq_probe(struct platform_device *pdev)
> +{
> +	struct tegra186_cpufreq_data *data;
> +	struct tegra_bpmp *bpmp;
> +	int i, err;

unsigned int i?

> +
> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
> +	if (!data)
> +		return -ENOMEM;
> +
> +	bpmp = tegra_bpmp_get(&pdev->dev);
> +	if (IS_ERR(bpmp))
> +		return PTR_ERR(bpmp);
> +
> +	for (i = 0; i < NUM_CLUSTERS; ++i) {
> +		struct resource *res;
> +
> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
> +						   CLUSTERS[i].name);
> +		if (!res) {
> +			err = -ENXIO;
> +			goto put_bpmp;
> +		}

No need for this check, devm_ioremap_resource() will take care of it.

> +
> +		data->regs[i] = devm_ioremap_resource(&pdev->dev, res);
> +		if (IS_ERR(data->regs[i])) {
> +			err = PTR_ERR(data->regs[i]);
> +			goto put_bpmp;
> +		}
> +
> +		err = init_vhint_table(pdev, bpmp, i, &data->tables[i]);
> +		if (err)
> +			goto put_bpmp;

This could become something along the lines of:

		data->tables[i] = init_vhint_table(pdev, bpmp, i);
		if (IS_ERR(data->tables[i])) {
			err = PTR_ERR(data->tables[i]);
			goto put_bpmp;
		}

Which is slightly more verbose than the original, but you get a lot more
clarity in return because you can just deal with straight pointers above
in init_vhint_table().

> +	}
> +
> +	tegra_bpmp_put(bpmp);
> +
> +	tegra186_cpufreq_driver.driver_data = data;
> +
> +	err = cpufreq_register_driver(&tegra186_cpufreq_driver);
> +	if (err)
> +		return err;
> +
> +	return 0;
> +
> +put_bpmp:
> +	tegra_bpmp_put(bpmp);
> +
> +	return err;
> +}
> +
> +static int tegra186_cpufreq_remove(struct platform_device *pdev)
> +{
> +	cpufreq_unregister_driver(&tegra186_cpufreq_driver);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id tegra186_cpufreq_of_match[] = {
> +	{ .compatible = "nvidia,tegra186-ccplex-cluster", },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, tegra186_cpufreq_of_match);
> +
> +static struct platform_driver tegra186_cpufreq_platform_driver = {
> +	.driver = {
> +		.name = "tegra186-cpufreq",
> +		.of_match_table = tegra186_cpufreq_of_match,
> +	},
> +	.probe = tegra186_cpufreq_probe,
> +	.remove = tegra186_cpufreq_remove,
> +};
> +module_platform_driver(tegra186_cpufreq_platform_driver);
> +
> +MODULE_AUTHOR("Mikko Perttunen <mperttunen@nvidia.com>");
> +MODULE_DESCRIPTION("Tegra186 cpufreq driver");

NVIDIA Tegra186?

I very much like how simple this driver is compared to previous
generations.

Thierry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/3] dt-bindings: Add bindings for nvidia,tegra186-ccplex-cluster

[Mikko Perttunen]

On 04/03/2017 05:06 PM, Thierry Reding wrote:
> On Mon, Apr 03, 2017 at 03:42:24PM +0300, Mikko Perttunen wrote:
>> The Tegra186 CCPLEX_CLUSTER area contains memory-mapped
>> registers that initiate CPU frequency/voltage transitions.
>>
>> Signed-off-by: Mikko Perttunen <mperttunen-DDmLM1+adcrQT0dZR+AlfA@public.gmane.org>
>> ---
>>  .../arm/tegra/nvidia,tegra186-ccplex-cluster.txt   | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt
>> new file mode 100644
>> index 000000000000..50cd615219e9
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt
>> @@ -0,0 +1,22 @@
>> +NVIDIA Tegra CCPLEX_CLUSTER area
>> +
>> +Required properties:
>> +- compatible: Should contain one of the following:
>> +  - "nvidia,tegra186-ccplex-cluster": for Tegra186
>> +- reg: Must contain an (offset, length) pair of the register set for each
>> +  entry in reg-names.
>> +- reg-names: Must include the following entries:
>> +  - "a57": Public aperture for A57 CPU cluster
>> +  - "denver": Public aperture for Denver CPU cluster
>> +- nvidia,bpmp: Phandle to BPMP device that can be queried for OPP tables
>
> "phandle"

Will fix.

>
>> +Example:
>> +
>> +	ccplex@e000000 {
>> +		compatible = "nvidia,tegra186-ccplex-cluster";
>> +		reg = <0x0 0x0e060000 0x0 0x1000>,
>> +		      <0x0 0x0e070000 0x0 0x1000>;
>> +		reg-names = "a57", "denver";
>> +
>> +		nvidia,bpmp = <&bpmp>;
>> +	};
>
> Where's the information about the register offsets coming from? The TRM
> says that CCPLEX_CLUSTER has a single aperture from 0x0e000000 to
> 0x0e3fffff.

Some internal document with a name related to Denver power management, 
IIRC. I'll link it to you tomorrow.

>
> Thierry
>

Re: [PATCH 2/3] dt-bindings: Add bindings for nvidia,tegra186-ccplex-cluster

[Mikko Perttunen]

On 04/03/2017 05:06 PM, Thierry Reding wrote:
> On Mon, Apr 03, 2017 at 03:42:24PM +0300, Mikko Perttunen wrote:
>> The Tegra186 CCPLEX_CLUSTER area contains memory-mapped
>> registers that initiate CPU frequency/voltage transitions.
>>
>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>> ---
>>  .../arm/tegra/nvidia,tegra186-ccplex-cluster.txt   | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt
>> new file mode 100644
>> index 000000000000..50cd615219e9
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt
>> @@ -0,0 +1,22 @@
>> +NVIDIA Tegra CCPLEX_CLUSTER area
>> +
>> +Required properties:
>> +- compatible: Should contain one of the following:
>> +  - "nvidia,tegra186-ccplex-cluster": for Tegra186
>> +- reg: Must contain an (offset, length) pair of the register set for each
>> +  entry in reg-names.
>> +- reg-names: Must include the following entries:
>> +  - "a57": Public aperture for A57 CPU cluster
>> +  - "denver": Public aperture for Denver CPU cluster
>> +- nvidia,bpmp: Phandle to BPMP device that can be queried for OPP tables
>
> "phandle"

Will fix.

>
>> +Example:
>> +
>> +	ccplex@e000000 {
>> +		compatible = "nvidia,tegra186-ccplex-cluster";
>> +		reg = <0x0 0x0e060000 0x0 0x1000>,
>> +		      <0x0 0x0e070000 0x0 0x1000>;
>> +		reg-names = "a57", "denver";
>> +
>> +		nvidia,bpmp = <&bpmp>;
>> +	};
>
> Where's the information about the register offsets coming from? The TRM
> says that CCPLEX_CLUSTER has a single aperture from 0x0e000000 to
> 0x0e3fffff.

Some internal document with a name related to Denver power management, 
IIRC. I'll link it to you tomorrow.

>
> Thierry
>

Re: [PATCH 2/3] dt-bindings: Add bindings for nvidia,tegra186-ccplex-cluster

[Mikko Perttunen]

On 04/03/2017 05:24 PM, Jon Hunter wrote:
>
> On 03/04/17 13:42, Mikko Perttunen wrote:
>> The Tegra186 CCPLEX_CLUSTER area contains memory-mapped
>> registers that initiate CPU frequency/voltage transitions.
>>
>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>> ---
>>  .../arm/tegra/nvidia,tegra186-ccplex-cluster.txt   | 22 ++++++++++++++++++++++
>>  1 file changed, 22 insertions(+)
>>  create mode 100644 Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt
>>
>> diff --git a/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt
>> new file mode 100644
>> index 000000000000..50cd615219e9
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/arm/tegra/nvidia,tegra186-ccplex-cluster.txt
>> @@ -0,0 +1,22 @@
>> +NVIDIA Tegra CCPLEX_CLUSTER area
>> +
>> +Required properties:
>> +- compatible: Should contain one of the following:
>> +  - "nvidia,tegra186-ccplex-cluster": for Tegra186
>
> Nit pick ... any reason why we append 'cluster' here? The TRM just says
> the "CPU Complex" consists of two CPU clusters. So
> "nvidia,tegra186-cpu-complex" or "nvidia,tegra186-ccplex" seems fine.
>
> BTW, I do see references in the TRM to CCPLEX_CLUSTER0/1, but never
> CCPLEX_CLUSTER in reference to both. I think it is just CCPLEX AFAICT.

The reason was that the MMIO aperture is called "CCPLEX_CLUSTER" in the 
address map. But I agree it's not a very descriptive name.

>
> Cheers
> Jon
>

Re: [PATCH 1/3] cpufreq: Add Tegra186 cpufreq driver

[Mikko Perttunen]

On 04/03/2017 05:47 PM, Thierry Reding wrote:
> On Mon, Apr 03, 2017 at 03:42:23PM +0300, Mikko Perttunen wrote:
>> Add a new cpufreq driver for Tegra186 (and likely later).
>> The CPUs are organized into two clusters, Denver and A57,
>> with two and four cores respectively. CPU frequency can be
>> adjusted by writing the desired rate divisor and a voltage
>> hint to a special per-core register.
>>
>> The frequency of each core can be set individually; however,
>> this is just a hint as all CPUs in a cluster will run at
>> the maximum rate of non-idle CPUs in the cluster.
>>
>> Signed-off-by: Mikko Perttunen <mperttunen@nvidia.com>
>> ---
>>  drivers/cpufreq/Kconfig.arm        |   7 +
>>  drivers/cpufreq/Makefile           |   1 +
>>  drivers/cpufreq/tegra186-cpufreq.c | 277 +++++++++++++++++++++++++++++++++++++
>>  3 files changed, 285 insertions(+)
>>  create mode 100644 drivers/cpufreq/tegra186-cpufreq.c
>>
>> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
>> index 74fa5c5904d3..168d36fa4a58 100644
>> --- a/drivers/cpufreq/Kconfig.arm
>> +++ b/drivers/cpufreq/Kconfig.arm
>> @@ -247,6 +247,13 @@ config ARM_TEGRA124_CPUFREQ
>>  	help
>>  	  This adds the CPUFreq driver support for Tegra124 SOCs.
>>
>> +config ARM_TEGRA186_CPUFREQ
>> +	tristate "Tegra186 CPUFreq support"
>> +	depends on ARCH_TEGRA && TEGRA_BPMP
>> +	default y
>
> I'd rather not default this to "y". We can use the defconfig to enable
> this.

Sure.

>
>> +	help
>> +	  This adds the CPUFreq driver support for Tegra186 SOCs.
>> +
>>  config ARM_TI_CPUFREQ
>>  	bool "Texas Instruments CPUFreq support"
>>  	depends on ARCH_OMAP2PLUS
>> diff --git a/drivers/cpufreq/Makefile b/drivers/cpufreq/Makefile
>> index 9f5a8045f36d..b7e78f063c4f 100644
>> --- a/drivers/cpufreq/Makefile
>> +++ b/drivers/cpufreq/Makefile
>> @@ -77,6 +77,7 @@ obj-$(CONFIG_ARM_SPEAR_CPUFREQ)		+= spear-cpufreq.o
>>  obj-$(CONFIG_ARM_STI_CPUFREQ)		+= sti-cpufreq.o
>>  obj-$(CONFIG_ARM_TEGRA20_CPUFREQ)	+= tegra20-cpufreq.o
>>  obj-$(CONFIG_ARM_TEGRA124_CPUFREQ)	+= tegra124-cpufreq.o
>> +obj-$(CONFIG_ARM_TEGRA186_CPUFREQ)	+= tegra186-cpufreq.o
>>  obj-$(CONFIG_ARM_TI_CPUFREQ)		+= ti-cpufreq.o
>>  obj-$(CONFIG_ARM_VEXPRESS_SPC_CPUFREQ)	+= vexpress-spc-cpufreq.o
>>  obj-$(CONFIG_ACPI_CPPC_CPUFREQ) += cppc_cpufreq.o
>> diff --git a/drivers/cpufreq/tegra186-cpufreq.c b/drivers/cpufreq/tegra186-cpufreq.c
>> new file mode 100644
>> index 000000000000..794c1f2d8231
>> --- /dev/null
>> +++ b/drivers/cpufreq/tegra186-cpufreq.c
>> @@ -0,0 +1,277 @@
>> +/*
>> + * Copyright (c) 2017, NVIDIA CORPORATION. All rights reserved
>> + *
>> + * This program is free software; you can redistribute it and/or modify it
>> + * under the terms and conditions of the GNU General Public License,
>> + * version 2, as published by the Free Software Foundation.
>> + *
>> + * This program is distributed in the hope it will be useful, but WITHOUT
>> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
>> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
>> + * more details.
>> + */
>> +
>> +#include <linux/cpufreq.h>
>> +#include <linux/dma-mapping.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +
>> +#include <soc/tegra/bpmp.h>
>> +#include <soc/tegra/bpmp-abi.h>
>> +
>> +#define EDVD_CORE_VOLT_FREQ(core)		(0x20 + (core) * 0x4)
>> +#define EDVD_CORE_VOLT_FREQ_F_SHIFT		0
>> +#define EDVD_CORE_VOLT_FREQ_F_MASK		0xff
>> +#define EDVD_CORE_VOLT_FREQ_V_SHIFT		16
>> +#define EDVD_CORE_VOLT_FREQ_V_MASK		0xff
>> +
>> +#define CLUSTER_DENVER				0
>> +#define CLUSTER_A57				1
>> +#define NUM_CLUSTERS				2
>> +
>> +struct tegra186_cpufreq_cluster {
>> +	const char *name;
>> +	unsigned int num_cores;
>> +};
>> +
>> +static const struct tegra186_cpufreq_cluster CLUSTERS[] = {
>
> We don't usually use uppercase letters for variable names.

You're right. Looks like I've misapplied some style rules from other 
projects :e

>
>> +	{
>> +		.name = "denver",
>> +		.num_cores = 2,
>> +	},
>> +	{
>> +		.name = "a57",
>> +		.num_cores = 4,
>> +	}
>> +};
>> +
>> +struct tegra186_cpufreq_data {
>> +	void __iomem *regs[NUM_CLUSTERS];
>> +	struct cpufreq_frequency_table *tables[NUM_CLUSTERS];
>> +};
>
> Given my comments regarding the aperture, perhaps it would be useful to
> only have a single range of memory-mapped registers and add an offset to
> struct tegra186_cpufreq_cluster that points into that region?

Yes, that might be cleaner. Would slightly simplify the probe code as well.

>
> Also, you're somewhat mixing arrays of NUM_CLUSTERS elements and
> dynamically sized ones (CLUSTERS[]). Probably best to just stick to one
> of them.
>
> Might be worth considering to dynamically allocate based on the cluster
> table, but that could be done in a separate patch if we ever get a
> configuration where NUM_CLUSTERS != 2.

I think we will have that eventually, so I'll do this for v2.

>
>> +static void get_cluster_core(int cpu, int *cluster, int *core)
>
> These can all be unsigned int.

Yes.

>
>> +{
>> +	switch (cpu) {
>> +	case 0:
>> +		*cluster = CLUSTER_A57; *core = 0; break;
>> +	case 3:
>> +		*cluster = CLUSTER_A57; *core = 1; break;
>> +	case 4:
>> +		*cluster = CLUSTER_A57; *core = 2; break;
>> +	case 5:
>> +		*cluster = CLUSTER_A57; *core = 3; break;
>> +	case 1:
>> +		*cluster = CLUSTER_DENVER; *core = 0; break;
>> +	case 2:
>> +		*cluster = CLUSTER_DENVER; *core = 1; break;
>> +	}
>> +}
>
> This is weirdly formatted. Also, what if cpu > 5? Do we need to be
> careful and WARN_ON()?

Each case was on a single line but checkpatch didn't like that.

>
> Perhaps in order to make this more easily extensible this could go
> into struct tegra186_cpufreq_cluster? Or a separate table that has
> information about the cluster and a list of CPUs?
>
> Again, probably not necessary right away, but something to keep in
> mind for parameterization if we ever need to support other configs.

I'll try to think of a better way for v2.

>
>> +static int tegra186_cpufreq_init(struct cpufreq_policy *policy)
>> +{
>> +	struct tegra186_cpufreq_data *data = cpufreq_get_driver_data();
>> +	struct cpufreq_frequency_table *table;
>> +	int cluster, core;
>> +
>> +	get_cluster_core(policy->cpu, &cluster, &core);
>> +
>> +	table = data->tables[cluster];
>> +	cpufreq_table_validate_and_show(policy, table);
>> +
>> +	policy->cpuinfo.transition_latency = 300 * 1000;
>> +
>> +	return 0;
>> +}
>> +
>> +static void write_volt_freq(uint8_t vidx, uint8_t ndiv, void __iomem *regs,
>
> uint8_t -> u8

Will do.

>
>> +			unsigned int core)
>> +{
>> +	u32 val = 0;
>> +
>> +	val |= ndiv << EDVD_CORE_VOLT_FREQ_F_SHIFT;
>> +	val |= vidx << EDVD_CORE_VOLT_FREQ_V_SHIFT;
>> +
>> +	writel(val, regs + EDVD_CORE_VOLT_FREQ(core));
>> +}
>> +
>> +static int tegra186_cpufreq_set_target(struct cpufreq_policy *policy,
>> +				       unsigned int index)
>> +{
>> +	struct cpufreq_frequency_table *tbl = policy->freq_table + index;
>> +	struct tegra186_cpufreq_data *data = cpufreq_get_driver_data();
>> +	uint16_t vidx = tbl->driver_data >> 16;
>> +	uint16_t ndiv = tbl->driver_data & 0xffff;
>
> uint16_t -> u16

Will do.

>
> Also it's slightly strange that you store u16 here and pass it to a
> function that takes

Ugh, yeah, write_volt_freq should take u16 (that's what the register 
size is), although in practice 8 bits is enough.

>
>> +	int cluster, core;
>> +
>> +	get_cluster_core(policy->cpu, &cluster, &core);
>> +	write_volt_freq(vidx, ndiv, data->regs[cluster], core);
>> +
>> +	return 0;
>> +}
>> +
>> +static struct cpufreq_driver tegra186_cpufreq_driver = {
>> +	.name = "tegra186",
>> +	.flags = CPUFREQ_STICKY | CPUFREQ_HAVE_GOVERNOR_PER_POLICY,
>> +	.verify = cpufreq_generic_frequency_table_verify,
>> +	.target_index = tegra186_cpufreq_set_target,
>> +	.init = tegra186_cpufreq_init,
>> +	.attr = cpufreq_generic_attr,
>> +};
>> +
>> +static int init_vhint_table(struct platform_device *pdev,
>> +			    struct tegra_bpmp *bpmp, uint32_t cluster_id,
>> +			    struct cpufreq_frequency_table **table)
>> +{
>> +	struct mrq_cpu_vhint_request req;
>> +	struct tegra_bpmp_message msg;
>> +	struct cpu_vhint_data *data;
>> +	int err, i, j, num_rates;
>> +	dma_addr_t phys;
>> +	void *virt;
>> +
>> +	virt = dma_alloc_coherent(bpmp->dev, MSG_DATA_MIN_SZ, &phys,
>> +				  GFP_KERNEL | GFP_DMA32);
>> +	if (!virt)
>> +		return -ENOMEM;
>
> This seems wrong. MSG_DATA_MIN_SZ is 120 but struct cpu_vhint_data is a
> lot larger than that (836 bytes, if I'm not mistaken). It probably works
> because dma_alloc_coherent() will always give you at least a whole page,
> but you should still use the correct size here.

Oops! Well spotted.

>
>> +
>> +	data = (struct cpu_vhint_data *)virt;
>> +
>> +	memset(&req, 0, sizeof(req));
>> +	req.addr = phys;
>> +	req.cluster_id = cluster_id;
>> +
>> +	memset(&msg, 0, sizeof(msg));
>> +	msg.mrq = MRQ_CPU_VHINT;
>> +	msg.tx.data = &req;
>> +	msg.tx.size = sizeof(req);
>> +
>> +	err = tegra_bpmp_transfer(bpmp, &msg);
>> +	if (err)
>> +		goto end;
>> +
>> +	num_rates = 0;
>
> This could be initialized when it is declared.

Will fix.

>
>> +
>> +	for (i = data->vfloor; i < data->vceil + 1; ++i) {
>
> i <= data->vceil? That seems more intuitive to me. Also, please only use
> pre-decrement if really necessary.

Will fix.

>
>> +		uint16_t ndiv = data->ndiv[i];
>> +
>> +		if (ndiv < data->ndiv_min || ndiv > data->ndiv_max)
>> +			continue;
>> +
>> +		/* Only store lowest voltage index for each rate */
>> +		if (i > 0 && ndiv == data->ndiv[i-1])
>
> Needs spaces around '-', I think checkpatch would complain otherwise.

Sure; checkpatch did not complain though.

>
> Also, why is this even happening? Why would MRQ_CPU_VHINT return the
> same ndiv value twice?

The array is indexed by voltage values, so if the achievable frequency 
doesn't increase when the voltage is increased by one step, there are 
two or more successive entries with the same ndiv value.

>
>> +			continue;
>> +
>> +		++num_rates;
>
> Again, post-increment is preferred over pre-increment.

Will fix.

>
>> +	}
>> +
>> +	*table = devm_kcalloc(&pdev->dev, num_rates + 1, sizeof(**table),
>> +			      GFP_KERNEL);
>
> There's a lot of dereferencing here and in the code below. Why not
> simply return a struct cpufreq_frequency_table * from this function, and
> an ERR_PTR()-encoded error on failure, rather than returning this in a
> parameter, requiring the repeated deref?

Yep, that's a good idea.

>
>> +	if (!*table) {
>> +		err = -ENOMEM;
>> +		goto end;
>> +	}
>> +
>> +	for (i = data->vfloor, j = 0; i < data->vceil + 1; ++i) {
>> +		struct cpufreq_frequency_table *point;
>> +		uint16_t ndiv = data->ndiv[i];
>> +
>> +		if (ndiv < data->ndiv_min || ndiv > data->ndiv_max)
>> +			continue;
>> +
>> +		/* Only store lowest voltage index for each rate */
>> +		if (i > 0 && ndiv == data->ndiv[i-1])
>> +			continue;
>> +
>> +		point = &(*table)[j++];
>> +		point->driver_data = (i << 16) | (ndiv);
>> +		point->frequency = data->ref_clk_hz * ndiv / data->pdiv /
>> +			data->mdiv / 1000;
>> +	}
>> +
>> +	(*table)[j].frequency = CPUFREQ_TABLE_END;
>> +
>> +end:
>
> free: would be a more accurate name for this label.

Will change.

>
>> +	dma_free_coherent(bpmp->dev, MSG_DATA_MIN_SZ, virt, phys);
>> +
>> +	return err;
>> +}
>> +
>> +static int tegra186_cpufreq_probe(struct platform_device *pdev)
>> +{
>> +	struct tegra186_cpufreq_data *data;
>> +	struct tegra_bpmp *bpmp;
>> +	int i, err;
>
> unsigned int i?

Will change.

>
>> +
>> +	data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	bpmp = tegra_bpmp_get(&pdev->dev);
>> +	if (IS_ERR(bpmp))
>> +		return PTR_ERR(bpmp);
>> +
>> +	for (i = 0; i < NUM_CLUSTERS; ++i) {
>> +		struct resource *res;
>> +
>> +		res = platform_get_resource_byname(pdev, IORESOURCE_MEM,
>> +						   CLUSTERS[i].name);
>> +		if (!res) {
>> +			err = -ENXIO;
>> +			goto put_bpmp;
>> +		}
>
> No need for this check, devm_ioremap_resource() will take care of it.

Will fix.

>
>> +
>> +		data->regs[i] = devm_ioremap_resource(&pdev->dev, res);
>> +		if (IS_ERR(data->regs[i])) {
>> +			err = PTR_ERR(data->regs[i]);
>> +			goto put_bpmp;
>> +		}
>> +
>> +		err = init_vhint_table(pdev, bpmp, i, &data->tables[i]);
>> +		if (err)
>> +			goto put_bpmp;
>
> This could become something along the lines of:
>
> 		data->tables[i] = init_vhint_table(pdev, bpmp, i);
> 		if (IS_ERR(data->tables[i])) {
> 			err = PTR_ERR(data->tables[i]);
> 			goto put_bpmp;
> 		}
>
> Which is slightly more verbose than the original, but you get a lot more
> clarity in return because you can just deal with straight pointers above
> in init_vhint_table().

Yep.

>
>> +	}
>> +
>> +	tegra_bpmp_put(bpmp);
>> +
>> +	tegra186_cpufreq_driver.driver_data = data;
>> +
>> +	err = cpufreq_register_driver(&tegra186_cpufreq_driver);
>> +	if (err)
>> +		return err;
>> +
>> +	return 0;
>> +
>> +put_bpmp:
>> +	tegra_bpmp_put(bpmp);
>> +
>> +	return err;
>> +}
>> +
>> +static int tegra186_cpufreq_remove(struct platform_device *pdev)
>> +{
>> +	cpufreq_unregister_driver(&tegra186_cpufreq_driver);
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id tegra186_cpufreq_of_match[] = {
>> +	{ .compatible = "nvidia,tegra186-ccplex-cluster", },
>> +	{ }
>> +};
>> +MODULE_DEVICE_TABLE(of, tegra186_cpufreq_of_match);
>> +
>> +static struct platform_driver tegra186_cpufreq_platform_driver = {
>> +	.driver = {
>> +		.name = "tegra186-cpufreq",
>> +		.of_match_table = tegra186_cpufreq_of_match,
>> +	},
>> +	.probe = tegra186_cpufreq_probe,
>> +	.remove = tegra186_cpufreq_remove,
>> +};
>> +module_platform_driver(tegra186_cpufreq_platform_driver);
>> +
>> +MODULE_AUTHOR("Mikko Perttunen <mperttunen@nvidia.com>");
>> +MODULE_DESCRIPTION("Tegra186 cpufreq driver");
>
> NVIDIA Tegra186?

Will change.

>
> I very much like how simple this driver is compared to previous
> generations.

I agree :) Though as discussed, it's not perfect - I skipped the clock 
rate get function because of how difficult it is to implement and how (I 
believe) poor the result is. But we should be able to live without that.

>
> Thierry
>

Thanks for reviewing!

Mikko