[PATCH 0/2] brcmstb-avs-cpufreq changes

[PATCH 0/2] brcmstb-avs-cpufreq changes

[Markus Mayer]

From: Markus Mayer <mmayer@broadcom.com>

These changes are based on v4.17-rc1. The two patches are unrelated.

Patch 1/2 removes unused code that was only useful during driver
development and that has been disabled by default for a long time.

Patch 2/2 allows the driver to detect if the system supports SCMI cpufreq

Jim Quinlan (1):
  cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

Markus Mayer (1):
  cpufreq: brcmstb-avs-cpufreq: remove development debug support

 drivers/cpufreq/Kconfig.arm           |  10 -
 drivers/cpufreq/brcmstb-avs-cpufreq.c | 339 ++--------------------------------
 2 files changed, 17 insertions(+), 332 deletions(-)

-- 
2.7.4

[PATCH 0/2] brcmstb-avs-cpufreq changes

[Markus Mayer]

From: Markus Mayer <mmayer@broadcom.com>

These changes are based on v4.17-rc1. The two patches are unrelated.

Patch 1/2 removes unused code that was only useful during driver
development and that has been disabled by default for a long time.

Patch 2/2 allows the driver to detect if the system supports SCMI cpufreq

Jim Quinlan (1):
  cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

Markus Mayer (1):
  cpufreq: brcmstb-avs-cpufreq: remove development debug support

 drivers/cpufreq/Kconfig.arm           |  10 -
 drivers/cpufreq/brcmstb-avs-cpufreq.c | 339 ++--------------------------------
 2 files changed, 17 insertions(+), 332 deletions(-)

-- 
2.7.4

[PATCH 0/2] brcmstb-avs-cpufreq changes

[Markus Mayer]

From: Markus Mayer <mmayer@broadcom.com>

These changes are based on v4.17-rc1. The two patches are unrelated.

Patch 1/2 removes unused code that was only useful during driver
development and that has been disabled by default for a long time.

Patch 2/2 allows the driver to detect if the system supports SCMI cpufreq

Jim Quinlan (1):
  cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

Markus Mayer (1):
  cpufreq: brcmstb-avs-cpufreq: remove development debug support

 drivers/cpufreq/Kconfig.arm           |  10 -
 drivers/cpufreq/brcmstb-avs-cpufreq.c | 339 ++--------------------------------
 2 files changed, 17 insertions(+), 332 deletions(-)

-- 
2.7.4

[PATCH 1/2] cpufreq: brcmstb-avs-cpufreq: remove development debug support

[Markus Mayer]

From: Markus Mayer <mmayer@broadcom.com>

This debug code was helpful while developing the driver, but it isn't
being used for anything anymore.

Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 drivers/cpufreq/Kconfig.arm           |  10 --
 drivers/cpufreq/brcmstb-avs-cpufreq.c | 323 +---------------------------------
 2 files changed, 1 insertion(+), 332 deletions(-)

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 7f56fe5183f2..de55c7d57438 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -71,16 +71,6 @@ config ARM_BRCMSTB_AVS_CPUFREQ
 
 	  Say Y, if you have a Broadcom SoC with AVS support for DFS or DVFS.
 
-config ARM_BRCMSTB_AVS_CPUFREQ_DEBUG
-	bool "Broadcom STB AVS CPUfreq driver sysfs debug capability"
-	depends on ARM_BRCMSTB_AVS_CPUFREQ
-	help
-	  Enabling this option turns on debug support via sysfs under
-	  /sys/kernel/debug/brcmstb-avs-cpufreq. It is possible to read all and
-	  write some AVS mailbox registers through sysfs entries.
-
-	  If in doubt, say N.
-
 config ARM_EXYNOS5440_CPUFREQ
 	tristate "SAMSUNG EXYNOS5440"
 	depends on SOC_EXYNOS5440
diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
index 6cdac1aaf23c..b07559b9ed99 100644
--- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
+++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
@@ -49,13 +49,6 @@
 #include <linux/platform_device.h>
 #include <linux/semaphore.h>
 
-#ifdef CONFIG_ARM_BRCMSTB_AVS_CPUFREQ_DEBUG
-#include <linux/ctype.h>
-#include <linux/debugfs.h>
-#include <linux/slab.h>
-#include <linux/uaccess.h>
-#endif
-
 /* Max number of arguments AVS calls take */
 #define AVS_MAX_CMD_ARGS	4
 /*
@@ -182,88 +175,11 @@ struct private_data {
 	void __iomem *base;
 	void __iomem *avs_intr_base;
 	struct device *dev;
-#ifdef CONFIG_ARM_BRCMSTB_AVS_CPUFREQ_DEBUG
-	struct dentry *debugfs;
-#endif
 	struct completion done;
 	struct semaphore sem;
 	struct pmap pmap;
 };
 
-#ifdef CONFIG_ARM_BRCMSTB_AVS_CPUFREQ_DEBUG
-
-enum debugfs_format {
-	DEBUGFS_NORMAL,
-	DEBUGFS_FLOAT,
-	DEBUGFS_REV,
-};
-
-struct debugfs_data {
-	struct debugfs_entry *entry;
-	struct private_data *priv;
-};
-
-struct debugfs_entry {
-	char *name;
-	u32 offset;
-	fmode_t mode;
-	enum debugfs_format format;
-};
-
-#define DEBUGFS_ENTRY(name, mode, format)	{ \
-	#name, AVS_MBOX_##name, mode, format \
-}
-
-/*
- * These are used for debugfs only. Otherwise we use AVS_MBOX_PARAM() directly.
- */
-#define AVS_MBOX_PARAM1		AVS_MBOX_PARAM(0)
-#define AVS_MBOX_PARAM2		AVS_MBOX_PARAM(1)
-#define AVS_MBOX_PARAM3		AVS_MBOX_PARAM(2)
-#define AVS_MBOX_PARAM4		AVS_MBOX_PARAM(3)
-
-/*
- * This table stores the name, access permissions and offset for each hardware
- * register and is used to generate debugfs entries.
- */
-static struct debugfs_entry debugfs_entries[] = {
-	DEBUGFS_ENTRY(COMMAND, S_IWUSR, DEBUGFS_NORMAL),
-	DEBUGFS_ENTRY(STATUS, S_IWUSR, DEBUGFS_NORMAL),
-	DEBUGFS_ENTRY(VOLTAGE0, 0, DEBUGFS_FLOAT),
-	DEBUGFS_ENTRY(TEMP0, 0, DEBUGFS_FLOAT),
-	DEBUGFS_ENTRY(PV0, 0, DEBUGFS_FLOAT),
-	DEBUGFS_ENTRY(MV0, 0, DEBUGFS_FLOAT),
-	DEBUGFS_ENTRY(PARAM1, S_IWUSR, DEBUGFS_NORMAL),
-	DEBUGFS_ENTRY(PARAM2, S_IWUSR, DEBUGFS_NORMAL),
-	DEBUGFS_ENTRY(PARAM3, S_IWUSR, DEBUGFS_NORMAL),
-	DEBUGFS_ENTRY(PARAM4, S_IWUSR, DEBUGFS_NORMAL),
-	DEBUGFS_ENTRY(REVISION, 0, DEBUGFS_REV),
-	DEBUGFS_ENTRY(PSTATE, 0, DEBUGFS_NORMAL),
-	DEBUGFS_ENTRY(HEARTBEAT, 0, DEBUGFS_NORMAL),
-	DEBUGFS_ENTRY(MAGIC, S_IWUSR, DEBUGFS_NORMAL),
-	DEBUGFS_ENTRY(SIGMA_HVT, 0, DEBUGFS_NORMAL),
-	DEBUGFS_ENTRY(SIGMA_SVT, 0, DEBUGFS_NORMAL),
-	DEBUGFS_ENTRY(VOLTAGE1, 0, DEBUGFS_FLOAT),
-	DEBUGFS_ENTRY(TEMP1, 0, DEBUGFS_FLOAT),
-	DEBUGFS_ENTRY(PV1, 0, DEBUGFS_FLOAT),
-	DEBUGFS_ENTRY(MV1, 0, DEBUGFS_FLOAT),
-	DEBUGFS_ENTRY(FREQUENCY, 0, DEBUGFS_NORMAL),
-};
-
-static int brcm_avs_target_index(struct cpufreq_policy *, unsigned int);
-
-static char *__strtolower(char *s)
-{
-	char *p;
-
-	for (p = s; *p; p++)
-		*p = tolower(*p);
-
-	return s;
-}
-
-#endif /* CONFIG_ARM_BRCMSTB_AVS_CPUFREQ_DEBUG */
-
 static void __iomem *__map_region(const char *name)
 {
 	struct device_node *np;
@@ -516,238 +432,6 @@ brcm_avs_get_freq_table(struct device *dev, struct private_data *priv)
 	return table;
 }
 
-#ifdef CONFIG_ARM_BRCMSTB_AVS_CPUFREQ_DEBUG
-
-#define MANT(x)	(unsigned int)(abs((x)) / 1000)
-#define FRAC(x)	(unsigned int)(abs((x)) - abs((x)) / 1000 * 1000)
-
-static int brcm_avs_debug_show(struct seq_file *s, void *data)
-{
-	struct debugfs_data *dbgfs = s->private;
-	void __iomem *base;
-	u32 val, offset;
-
-	if (!dbgfs) {
-		seq_puts(s, "No device pointer\n");
-		return 0;
-	}
-
-	base = dbgfs->priv->base;
-	offset = dbgfs->entry->offset;
-	val = readl(base + offset);
-	switch (dbgfs->entry->format) {
-	case DEBUGFS_NORMAL:
-		seq_printf(s, "%u\n", val);
-		break;
-	case DEBUGFS_FLOAT:
-		seq_printf(s, "%d.%03d\n", MANT(val), FRAC(val));
-		break;
-	case DEBUGFS_REV:
-		seq_printf(s, "%c.%c.%c.%c\n", (val >> 24 & 0xff),
-			   (val >> 16 & 0xff), (val >> 8 & 0xff),
-			   val & 0xff);
-		break;
-	}
-	seq_printf(s, "0x%08x\n", val);
-
-	return 0;
-}
-
-#undef MANT
-#undef FRAC
-
-static ssize_t brcm_avs_seq_write(struct file *file, const char __user *buf,
-				  size_t size, loff_t *ppos)
-{
-	struct seq_file *s = file->private_data;
-	struct debugfs_data *dbgfs = s->private;
-	struct private_data *priv = dbgfs->priv;
-	void __iomem *base, *avs_intr_base;
-	bool use_issue_command = false;
-	unsigned long val, offset;
-	char str[128];
-	int ret;
-	char *str_ptr = str;
-
-	if (size >= sizeof(str))
-		return -E2BIG;
-
-	memset(str, 0, sizeof(str));
-	ret = copy_from_user(str, buf, size);
-	if (ret)
-		return ret;
-
-	base = priv->base;
-	avs_intr_base = priv->avs_intr_base;
-	offset = dbgfs->entry->offset;
-	/*
-	 * Special case writing to "command" entry only: if the string starts
-	 * with a 'c', we use the driver's __issue_avs_command() function.
-	 * Otherwise, we perform a raw write. This should allow testing of raw
-	 * access as well as using the higher level function. (Raw access
-	 * doesn't clear the firmware return status after issuing the command.)
-	 */
-	if (str_ptr[0] == 'c' && offset == AVS_MBOX_COMMAND) {
-		use_issue_command = true;
-		str_ptr++;
-	}
-	if (kstrtoul(str_ptr, 0, &val) != 0)
-		return -EINVAL;
-
-	/*
-	 * Setting the P-state is a special case. We need to update the CPU
-	 * frequency we report.
-	 */
-	if (val == AVS_CMD_SET_PSTATE) {
-		struct cpufreq_policy *policy;
-		unsigned int pstate;
-
-		policy = cpufreq_cpu_get(smp_processor_id());
-		/* Read back the P-state we are about to set */
-		pstate = readl(base + AVS_MBOX_PARAM(0));
-		if (use_issue_command) {
-			ret = brcm_avs_target_index(policy, pstate);
-			return ret ? ret : size;
-		}
-		policy->cur = policy->freq_table[pstate].frequency;
-	}
-
-	if (use_issue_command) {
-		ret = __issue_avs_command(priv, val, false, NULL);
-	} else {
-		/* Locking here is not perfect, but is only for debug. */
-		ret = down_interruptible(&priv->sem);
-		if (ret)
-			return ret;
-
-		writel(val, base + offset);
-		/* We have to wake up the firmware to process a command. */
-		if (offset == AVS_MBOX_COMMAND)
-			writel(AVS_CPU_L2_INT_MASK,
-			       avs_intr_base + AVS_CPU_L2_SET0);
-		up(&priv->sem);
-	}
-
-	return ret ? ret : size;
-}
-
-static struct debugfs_entry *__find_debugfs_entry(const char *name)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(debugfs_entries); i++)
-		if (strcasecmp(debugfs_entries[i].name, name) == 0)
-			return &debugfs_entries[i];
-
-	return NULL;
-}
-
-static int brcm_avs_debug_open(struct inode *inode, struct file *file)
-{
-	struct debugfs_data *data;
-	fmode_t fmode;
-	int ret;
-
-	/*
-	 * seq_open(), which is called by single_open(), clears "write" access.
-	 * We need write access to some files, so we preserve our access mode
-	 * and restore it.
-	 */
-	fmode = file->f_mode;
-	/*
-	 * Check access permissions even for root. We don't want to be writing
-	 * to read-only registers. Access for regular users has already been
-	 * checked by the VFS layer.
-	 */
-	if ((fmode & FMODE_WRITER) && !(inode->i_mode & S_IWUSR))
-		return -EACCES;
-
-	data = kmalloc(sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-	/*
-	 * We use the same file system operations for all our debug files. To
-	 * produce specific output, we look up the file name upon opening a
-	 * debugfs entry and map it to a memory offset. This offset is then used
-	 * in the generic "show" function to read a specific register.
-	 */
-	data->entry = __find_debugfs_entry(file->f_path.dentry->d_iname);
-	data->priv = inode->i_private;
-
-	ret = single_open(file, brcm_avs_debug_show, data);
-	if (ret)
-		kfree(data);
-	file->f_mode = fmode;
-
-	return ret;
-}
-
-static int brcm_avs_debug_release(struct inode *inode, struct file *file)
-{
-	struct seq_file *seq_priv = file->private_data;
-	struct debugfs_data *data = seq_priv->private;
-
-	kfree(data);
-	return single_release(inode, file);
-}
-
-static const struct file_operations brcm_avs_debug_ops = {
-	.open		= brcm_avs_debug_open,
-	.read		= seq_read,
-	.write		= brcm_avs_seq_write,
-	.llseek		= seq_lseek,
-	.release	= brcm_avs_debug_release,
-};
-
-static void brcm_avs_cpufreq_debug_init(struct platform_device *pdev)
-{
-	struct private_data *priv = platform_get_drvdata(pdev);
-	struct dentry *dir;
-	int i;
-
-	if (!priv)
-		return;
-
-	dir = debugfs_create_dir(BRCM_AVS_CPUFREQ_NAME, NULL);
-	if (IS_ERR_OR_NULL(dir))
-		return;
-	priv->debugfs = dir;
-
-	for (i = 0; i < ARRAY_SIZE(debugfs_entries); i++) {
-		/*
-		 * The DEBUGFS_ENTRY macro generates uppercase strings. We
-		 * convert them to lowercase before creating the debugfs
-		 * entries.
-		 */
-		char *entry = __strtolower(debugfs_entries[i].name);
-		fmode_t mode = debugfs_entries[i].mode;
-
-		if (!debugfs_create_file(entry, S_IFREG | S_IRUGO | mode,
-					 dir, priv, &brcm_avs_debug_ops)) {
-			priv->debugfs = NULL;
-			debugfs_remove_recursive(dir);
-			break;
-		}
-	}
-}
-
-static void brcm_avs_cpufreq_debug_exit(struct platform_device *pdev)
-{
-	struct private_data *priv = platform_get_drvdata(pdev);
-
-	if (priv && priv->debugfs) {
-		debugfs_remove_recursive(priv->debugfs);
-		priv->debugfs = NULL;
-	}
-}
-
-#else
-
-static void brcm_avs_cpufreq_debug_init(struct platform_device *pdev) {}
-static void brcm_avs_cpufreq_debug_exit(struct platform_device *pdev) {}
-
-#endif /* CONFIG_ARM_BRCMSTB_AVS_CPUFREQ_DEBUG */
-
 /*
  * To ensure the right firmware is running we need to
  *    - check the MAGIC matches what we expect
@@ -1016,11 +700,8 @@ static int brcm_avs_cpufreq_probe(struct platform_device *pdev)
 		return ret;
 
 	brcm_avs_driver.driver_data = pdev;
-	ret = cpufreq_register_driver(&brcm_avs_driver);
-	if (!ret)
-		brcm_avs_cpufreq_debug_init(pdev);
 
-	return ret;
+	return cpufreq_register_driver(&brcm_avs_driver);
 }
 
 static int brcm_avs_cpufreq_remove(struct platform_device *pdev)
@@ -1032,8 +713,6 @@ static int brcm_avs_cpufreq_remove(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	brcm_avs_cpufreq_debug_exit(pdev);
-
 	priv = platform_get_drvdata(pdev);
 	iounmap(priv->base);
 	iounmap(priv->avs_intr_base);
-- 
2.7.4

[PATCH 1/2] cpufreq: brcmstb-avs-cpufreq: remove development debug support

[Markus Mayer]

From: Markus Mayer <mmayer@broadcom.com>

This debug code was helpful while developing the driver, but it isn't
being used for anything anymore.

Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 drivers/cpufreq/Kconfig.arm           |  10 --
 drivers/cpufreq/brcmstb-avs-cpufreq.c | 323 +---------------------------------
 2 files changed, 1 insertion(+), 332 deletions(-)

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 7f56fe5183f2..de55c7d57438 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -71,16 +71,6 @@ config ARM_BRCMSTB_AVS_CPUFREQ
 
 	  Say Y, if you have a Broadcom SoC with AVS support for DFS or DVFS.
 
-config ARM_BRCMSTB_AVS_CPUFREQ_DEBUG
-	bool "Broadcom STB AVS CPUfreq driver sysfs debug capability"
-	depends on ARM_BRCMSTB_AVS_CPUFREQ
-	help
-	  Enabling this option turns on debug support via sysfs under
-	  /sys/kernel/debug/brcmstb-avs-cpufreq. It is possible to read all and
-	  write some AVS mailbox registers through sysfs entries.
-
-	  If in doubt, say N.
-
 config ARM_EXYNOS5440_CPUFREQ
 	tristate "SAMSUNG EXYNOS5440"
 	depends on SOC_EXYNOS5440
diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
index 6cdac1aaf23c..b07559b9ed99 100644
--- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
+++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
@@ -49,13 +49,6 @@
 #include <linux/platform_device.h>
 #include <linux/semaphore.h>
 
-#ifdef CONFIG_ARM_BRCMSTB_AVS_CPUFREQ_DEBUG
-#include <linux/ctype.h>
-#include <linux/debugfs.h>
-#include <linux/slab.h>
-#include <linux/uaccess.h>
-#endif
-
 /* Max number of arguments AVS calls take */
 #define AVS_MAX_CMD_ARGS	4
 /*
@@ -182,88 +175,11 @@ struct private_data {
 	void __iomem *base;
 	void __iomem *avs_intr_base;
 	struct device *dev;
-#ifdef CONFIG_ARM_BRCMSTB_AVS_CPUFREQ_DEBUG
-	struct dentry *debugfs;
-#endif
 	struct completion done;
 	struct semaphore sem;
 	struct pmap pmap;
 };
 
-#ifdef CONFIG_ARM_BRCMSTB_AVS_CPUFREQ_DEBUG
-
-enum debugfs_format {
-	DEBUGFS_NORMAL,
-	DEBUGFS_FLOAT,
-	DEBUGFS_REV,
-};
-
-struct debugfs_data {
-	struct debugfs_entry *entry;
-	struct private_data *priv;
-};
-
-struct debugfs_entry {
-	char *name;
-	u32 offset;
-	fmode_t mode;
-	enum debugfs_format format;
-};
-
-#define DEBUGFS_ENTRY(name, mode, format)	{ \
-	#name, AVS_MBOX_##name, mode, format \
-}
-
-/*
- * These are used for debugfs only. Otherwise we use AVS_MBOX_PARAM() directly.
- */
-#define AVS_MBOX_PARAM1		AVS_MBOX_PARAM(0)
-#define AVS_MBOX_PARAM2		AVS_MBOX_PARAM(1)
-#define AVS_MBOX_PARAM3		AVS_MBOX_PARAM(2)
-#define AVS_MBOX_PARAM4		AVS_MBOX_PARAM(3)
-
-/*
- * This table stores the name, access permissions and offset for each hardware
- * register and is used to generate debugfs entries.
- */
-static struct debugfs_entry debugfs_entries[] = {
-	DEBUGFS_ENTRY(COMMAND, S_IWUSR, DEBUGFS_NORMAL),
-	DEBUGFS_ENTRY(STATUS, S_IWUSR, DEBUGFS_NORMAL),
-	DEBUGFS_ENTRY(VOLTAGE0, 0, DEBUGFS_FLOAT),
-	DEBUGFS_ENTRY(TEMP0, 0, DEBUGFS_FLOAT),
-	DEBUGFS_ENTRY(PV0, 0, DEBUGFS_FLOAT),
-	DEBUGFS_ENTRY(MV0, 0, DEBUGFS_FLOAT),
-	DEBUGFS_ENTRY(PARAM1, S_IWUSR, DEBUGFS_NORMAL),
-	DEBUGFS_ENTRY(PARAM2, S_IWUSR, DEBUGFS_NORMAL),
-	DEBUGFS_ENTRY(PARAM3, S_IWUSR, DEBUGFS_NORMAL),
-	DEBUGFS_ENTRY(PARAM4, S_IWUSR, DEBUGFS_NORMAL),
-	DEBUGFS_ENTRY(REVISION, 0, DEBUGFS_REV),
-	DEBUGFS_ENTRY(PSTATE, 0, DEBUGFS_NORMAL),
-	DEBUGFS_ENTRY(HEARTBEAT, 0, DEBUGFS_NORMAL),
-	DEBUGFS_ENTRY(MAGIC, S_IWUSR, DEBUGFS_NORMAL),
-	DEBUGFS_ENTRY(SIGMA_HVT, 0, DEBUGFS_NORMAL),
-	DEBUGFS_ENTRY(SIGMA_SVT, 0, DEBUGFS_NORMAL),
-	DEBUGFS_ENTRY(VOLTAGE1, 0, DEBUGFS_FLOAT),
-	DEBUGFS_ENTRY(TEMP1, 0, DEBUGFS_FLOAT),
-	DEBUGFS_ENTRY(PV1, 0, DEBUGFS_FLOAT),
-	DEBUGFS_ENTRY(MV1, 0, DEBUGFS_FLOAT),
-	DEBUGFS_ENTRY(FREQUENCY, 0, DEBUGFS_NORMAL),
-};
-
-static int brcm_avs_target_index(struct cpufreq_policy *, unsigned int);
-
-static char *__strtolower(char *s)
-{
-	char *p;
-
-	for (p = s; *p; p++)
-		*p = tolower(*p);
-
-	return s;
-}
-
-#endif /* CONFIG_ARM_BRCMSTB_AVS_CPUFREQ_DEBUG */
-
 static void __iomem *__map_region(const char *name)
 {
 	struct device_node *np;
@@ -516,238 +432,6 @@ brcm_avs_get_freq_table(struct device *dev, struct private_data *priv)
 	return table;
 }
 
-#ifdef CONFIG_ARM_BRCMSTB_AVS_CPUFREQ_DEBUG
-
-#define MANT(x)	(unsigned int)(abs((x)) / 1000)
-#define FRAC(x)	(unsigned int)(abs((x)) - abs((x)) / 1000 * 1000)
-
-static int brcm_avs_debug_show(struct seq_file *s, void *data)
-{
-	struct debugfs_data *dbgfs = s->private;
-	void __iomem *base;
-	u32 val, offset;
-
-	if (!dbgfs) {
-		seq_puts(s, "No device pointer\n");
-		return 0;
-	}
-
-	base = dbgfs->priv->base;
-	offset = dbgfs->entry->offset;
-	val = readl(base + offset);
-	switch (dbgfs->entry->format) {
-	case DEBUGFS_NORMAL:
-		seq_printf(s, "%u\n", val);
-		break;
-	case DEBUGFS_FLOAT:
-		seq_printf(s, "%d.%03d\n", MANT(val), FRAC(val));
-		break;
-	case DEBUGFS_REV:
-		seq_printf(s, "%c.%c.%c.%c\n", (val >> 24 & 0xff),
-			   (val >> 16 & 0xff), (val >> 8 & 0xff),
-			   val & 0xff);
-		break;
-	}
-	seq_printf(s, "0x%08x\n", val);
-
-	return 0;
-}
-
-#undef MANT
-#undef FRAC
-
-static ssize_t brcm_avs_seq_write(struct file *file, const char __user *buf,
-				  size_t size, loff_t *ppos)
-{
-	struct seq_file *s = file->private_data;
-	struct debugfs_data *dbgfs = s->private;
-	struct private_data *priv = dbgfs->priv;
-	void __iomem *base, *avs_intr_base;
-	bool use_issue_command = false;
-	unsigned long val, offset;
-	char str[128];
-	int ret;
-	char *str_ptr = str;
-
-	if (size >= sizeof(str))
-		return -E2BIG;
-
-	memset(str, 0, sizeof(str));
-	ret = copy_from_user(str, buf, size);
-	if (ret)
-		return ret;
-
-	base = priv->base;
-	avs_intr_base = priv->avs_intr_base;
-	offset = dbgfs->entry->offset;
-	/*
-	 * Special case writing to "command" entry only: if the string starts
-	 * with a 'c', we use the driver's __issue_avs_command() function.
-	 * Otherwise, we perform a raw write. This should allow testing of raw
-	 * access as well as using the higher level function. (Raw access
-	 * doesn't clear the firmware return status after issuing the command.)
-	 */
-	if (str_ptr[0] == 'c' && offset == AVS_MBOX_COMMAND) {
-		use_issue_command = true;
-		str_ptr++;
-	}
-	if (kstrtoul(str_ptr, 0, &val) != 0)
-		return -EINVAL;
-
-	/*
-	 * Setting the P-state is a special case. We need to update the CPU
-	 * frequency we report.
-	 */
-	if (val == AVS_CMD_SET_PSTATE) {
-		struct cpufreq_policy *policy;
-		unsigned int pstate;
-
-		policy = cpufreq_cpu_get(smp_processor_id());
-		/* Read back the P-state we are about to set */
-		pstate = readl(base + AVS_MBOX_PARAM(0));
-		if (use_issue_command) {
-			ret = brcm_avs_target_index(policy, pstate);
-			return ret ? ret : size;
-		}
-		policy->cur = policy->freq_table[pstate].frequency;
-	}
-
-	if (use_issue_command) {
-		ret = __issue_avs_command(priv, val, false, NULL);
-	} else {
-		/* Locking here is not perfect, but is only for debug. */
-		ret = down_interruptible(&priv->sem);
-		if (ret)
-			return ret;
-
-		writel(val, base + offset);
-		/* We have to wake up the firmware to process a command. */
-		if (offset == AVS_MBOX_COMMAND)
-			writel(AVS_CPU_L2_INT_MASK,
-			       avs_intr_base + AVS_CPU_L2_SET0);
-		up(&priv->sem);
-	}
-
-	return ret ? ret : size;
-}
-
-static struct debugfs_entry *__find_debugfs_entry(const char *name)
-{
-	int i;
-
-	for (i = 0; i < ARRAY_SIZE(debugfs_entries); i++)
-		if (strcasecmp(debugfs_entries[i].name, name) == 0)
-			return &debugfs_entries[i];
-
-	return NULL;
-}
-
-static int brcm_avs_debug_open(struct inode *inode, struct file *file)
-{
-	struct debugfs_data *data;
-	fmode_t fmode;
-	int ret;
-
-	/*
-	 * seq_open(), which is called by single_open(), clears "write" access.
-	 * We need write access to some files, so we preserve our access mode
-	 * and restore it.
-	 */
-	fmode = file->f_mode;
-	/*
-	 * Check access permissions even for root. We don't want to be writing
-	 * to read-only registers. Access for regular users has already been
-	 * checked by the VFS layer.
-	 */
-	if ((fmode & FMODE_WRITER) && !(inode->i_mode & S_IWUSR))
-		return -EACCES;
-
-	data = kmalloc(sizeof(*data), GFP_KERNEL);
-	if (!data)
-		return -ENOMEM;
-	/*
-	 * We use the same file system operations for all our debug files. To
-	 * produce specific output, we look up the file name upon opening a
-	 * debugfs entry and map it to a memory offset. This offset is then used
-	 * in the generic "show" function to read a specific register.
-	 */
-	data->entry = __find_debugfs_entry(file->f_path.dentry->d_iname);
-	data->priv = inode->i_private;
-
-	ret = single_open(file, brcm_avs_debug_show, data);
-	if (ret)
-		kfree(data);
-	file->f_mode = fmode;
-
-	return ret;
-}
-
-static int brcm_avs_debug_release(struct inode *inode, struct file *file)
-{
-	struct seq_file *seq_priv = file->private_data;
-	struct debugfs_data *data = seq_priv->private;
-
-	kfree(data);
-	return single_release(inode, file);
-}
-
-static const struct file_operations brcm_avs_debug_ops = {
-	.open		= brcm_avs_debug_open,
-	.read		= seq_read,
-	.write		= brcm_avs_seq_write,
-	.llseek		= seq_lseek,
-	.release	= brcm_avs_debug_release,
-};
-
-static void brcm_avs_cpufreq_debug_init(struct platform_device *pdev)
-{
-	struct private_data *priv = platform_get_drvdata(pdev);
-	struct dentry *dir;
-	int i;
-
-	if (!priv)
-		return;
-
-	dir = debugfs_create_dir(BRCM_AVS_CPUFREQ_NAME, NULL);
-	if (IS_ERR_OR_NULL(dir))
-		return;
-	priv->debugfs = dir;
-
-	for (i = 0; i < ARRAY_SIZE(debugfs_entries); i++) {
-		/*
-		 * The DEBUGFS_ENTRY macro generates uppercase strings. We
-		 * convert them to lowercase before creating the debugfs
-		 * entries.
-		 */
-		char *entry = __strtolower(debugfs_entries[i].name);
-		fmode_t mode = debugfs_entries[i].mode;
-
-		if (!debugfs_create_file(entry, S_IFREG | S_IRUGO | mode,
-					 dir, priv, &brcm_avs_debug_ops)) {
-			priv->debugfs = NULL;
-			debugfs_remove_recursive(dir);
-			break;
-		}
-	}
-}
-
-static void brcm_avs_cpufreq_debug_exit(struct platform_device *pdev)
-{
-	struct private_data *priv = platform_get_drvdata(pdev);
-
-	if (priv && priv->debugfs) {
-		debugfs_remove_recursive(priv->debugfs);
-		priv->debugfs = NULL;
-	}
-}
-
-#else
-
-static void brcm_avs_cpufreq_debug_init(struct platform_device *pdev) {}
-static void brcm_avs_cpufreq_debug_exit(struct platform_device *pdev) {}
-
-#endif /* CONFIG_ARM_BRCMSTB_AVS_CPUFREQ_DEBUG */
-
 /*
  * To ensure the right firmware is running we need to
  *    - check the MAGIC matches what we expect
@@ -1016,11 +700,8 @@ static int brcm_avs_cpufreq_probe(struct platform_device *pdev)
 		return ret;
 
 	brcm_avs_driver.driver_data = pdev;
-	ret = cpufreq_register_driver(&brcm_avs_driver);
-	if (!ret)
-		brcm_avs_cpufreq_debug_init(pdev);
 
-	return ret;
+	return cpufreq_register_driver(&brcm_avs_driver);
 }
 
 static int brcm_avs_cpufreq_remove(struct platform_device *pdev)
@@ -1032,8 +713,6 @@ static int brcm_avs_cpufreq_remove(struct platform_device *pdev)
 	if (ret)
 		return ret;
 
-	brcm_avs_cpufreq_debug_exit(pdev);
-
 	priv = platform_get_drvdata(pdev);
 	iounmap(priv->base);
 	iounmap(priv->avs_intr_base);
-- 
2.7.4

[PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Markus Mayer]

From: Jim Quinlan <jim2101024@gmail.com>

If the SCMI cpufreq driver is supported, we bail, so that the new
approach can be used.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
index b07559b9ed99..b4861a730162 100644
--- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
+++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
@@ -164,6 +164,8 @@
 #define BRCM_AVS_CPU_INTR	"brcm,avs-cpu-l2-intr"
 #define BRCM_AVS_HOST_INTR	"sw_intr"
 
+#define ARM_SCMI_COMPAT		"arm,scmi"
+
 struct pmap {
 	unsigned int mode;
 	unsigned int p1;
@@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device *pdev)
 	struct device *dev;
 	int host_irq, ret;
 
+	/*
+	 * If the SCMI cpufreq driver is supported, we bail, so that the more
+	 * modern approach can be used.
+	 */
+	if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
+		struct device_node *np;
+
+		np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
+		if (np) {
+			of_node_put(np);
+			return -ENXIO;
+		}
+	}
+
 	dev = &pdev->dev;
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
-- 
2.7.4

[PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Markus Mayer]

From: Jim Quinlan <jim2101024@gmail.com>

If the SCMI cpufreq driver is supported, we bail, so that the new
approach can be used.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
index b07559b9ed99..b4861a730162 100644
--- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
+++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
@@ -164,6 +164,8 @@
 #define BRCM_AVS_CPU_INTR	"brcm,avs-cpu-l2-intr"
 #define BRCM_AVS_HOST_INTR	"sw_intr"
 
+#define ARM_SCMI_COMPAT		"arm,scmi"
+
 struct pmap {
 	unsigned int mode;
 	unsigned int p1;
@@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device *pdev)
 	struct device *dev;
 	int host_irq, ret;
 
+	/*
+	 * If the SCMI cpufreq driver is supported, we bail, so that the more
+	 * modern approach can be used.
+	 */
+	if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
+		struct device_node *np;
+
+		np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
+		if (np) {
+			of_node_put(np);
+			return -ENXIO;
+		}
+	}
+
 	dev = &pdev->dev;
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
-- 
2.7.4

[PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Markus Mayer]

From: Jim Quinlan <jim2101024@gmail.com>

If the SCMI cpufreq driver is supported, we bail, so that the new
approach can be used.

Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
Signed-off-by: Markus Mayer <mmayer@broadcom.com>
---
 drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
index b07559b9ed99..b4861a730162 100644
--- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
+++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
@@ -164,6 +164,8 @@
 #define BRCM_AVS_CPU_INTR	"brcm,avs-cpu-l2-intr"
 #define BRCM_AVS_HOST_INTR	"sw_intr"
 
+#define ARM_SCMI_COMPAT		"arm,scmi"
+
 struct pmap {
 	unsigned int mode;
 	unsigned int p1;
@@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device *pdev)
 	struct device *dev;
 	int host_irq, ret;
 
+	/*
+	 * If the SCMI cpufreq driver is supported, we bail, so that the more
+	 * modern approach can be used.
+	 */
+	if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
+		struct device_node *np;
+
+		np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
+		if (np) {
+			of_node_put(np);
+			return -ENXIO;
+		}
+	}
+
 	dev = &pdev->dev;
 	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
 	if (!priv)
-- 
2.7.4

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Florian Fainelli]

On 04/18/2018 08:56 AM, Markus Mayer wrote:
> From: Jim Quinlan <jim2101024@gmail.com>
> 
> If the SCMI cpufreq driver is supported, we bail, so that the new
> approach can be used.
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> index b07559b9ed99..b4861a730162 100644
> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> @@ -164,6 +164,8 @@
>  #define BRCM_AVS_CPU_INTR	"brcm,avs-cpu-l2-intr"
>  #define BRCM_AVS_HOST_INTR	"sw_intr"
>  
> +#define ARM_SCMI_COMPAT		"arm,scmi"
> +
>  struct pmap {
>  	unsigned int mode;
>  	unsigned int p1;
> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device *pdev)
>  	struct device *dev;
>  	int host_irq, ret;
>  
> +	/*
> +	 * If the SCMI cpufreq driver is supported, we bail, so that the more
> +	 * modern approach can be used.
> +	 */
> +	if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
> +		struct device_node *np;
> +
> +		np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
> +		if (np) {
> +			of_node_put(np);
> +			return -ENXIO;
> +		}

We would probably want to make sure that the node is also enabled (that
is, does not have a status = "disabled" property) otherwise the check
can be defeated. Something like:

		if (np && of_device_is_available(np))

should be good for that.

Thanks!
-- 
Florian

[PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Florian Fainelli]

On 04/18/2018 08:56 AM, Markus Mayer wrote:
> From: Jim Quinlan <jim2101024@gmail.com>
> 
> If the SCMI cpufreq driver is supported, we bail, so that the new
> approach can be used.
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> index b07559b9ed99..b4861a730162 100644
> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> @@ -164,6 +164,8 @@
>  #define BRCM_AVS_CPU_INTR	"brcm,avs-cpu-l2-intr"
>  #define BRCM_AVS_HOST_INTR	"sw_intr"
>  
> +#define ARM_SCMI_COMPAT		"arm,scmi"
> +
>  struct pmap {
>  	unsigned int mode;
>  	unsigned int p1;
> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device *pdev)
>  	struct device *dev;
>  	int host_irq, ret;
>  
> +	/*
> +	 * If the SCMI cpufreq driver is supported, we bail, so that the more
> +	 * modern approach can be used.
> +	 */
> +	if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
> +		struct device_node *np;
> +
> +		np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
> +		if (np) {
> +			of_node_put(np);
> +			return -ENXIO;
> +		}

We would probably want to make sure that the node is also enabled (that
is, does not have a status = "disabled" property) otherwise the check
can be defeated. Something like:

		if (np && of_device_is_available(np))

should be good for that.

Thanks!
-- 
Florian

Re: [PATCH 1/2] cpufreq: brcmstb-avs-cpufreq: remove development debug support

[Viresh Kumar]

On 18-04-18, 08:56, Markus Mayer wrote:
> From: Markus Mayer <mmayer@broadcom.com>
> 
> This debug code was helpful while developing the driver, but it isn't
> being used for anything anymore.
> 
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  drivers/cpufreq/Kconfig.arm           |  10 --
>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 323 +---------------------------------
>  2 files changed, 1 insertion(+), 332 deletions(-)

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

-- 
viresh

[PATCH 1/2] cpufreq: brcmstb-avs-cpufreq: remove development debug support

[Viresh Kumar]

On 18-04-18, 08:56, Markus Mayer wrote:
> From: Markus Mayer <mmayer@broadcom.com>
> 
> This debug code was helpful while developing the driver, but it isn't
> being used for anything anymore.
> 
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  drivers/cpufreq/Kconfig.arm           |  10 --
>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 323 +---------------------------------
>  2 files changed, 1 insertion(+), 332 deletions(-)

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

-- 
viresh

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Viresh Kumar]

On 18-04-18, 08:56, Markus Mayer wrote:
> From: Jim Quinlan <jim2101024@gmail.com>
> 
> If the SCMI cpufreq driver is supported, we bail, so that the new
> approach can be used.
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> index b07559b9ed99..b4861a730162 100644
> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> @@ -164,6 +164,8 @@
>  #define BRCM_AVS_CPU_INTR	"brcm,avs-cpu-l2-intr"
>  #define BRCM_AVS_HOST_INTR	"sw_intr"
>  
> +#define ARM_SCMI_COMPAT		"arm,scmi"
> +
>  struct pmap {
>  	unsigned int mode;
>  	unsigned int p1;
> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device *pdev)
>  	struct device *dev;
>  	int host_irq, ret;
>  
> +	/*
> +	 * If the SCMI cpufreq driver is supported, we bail, so that the more
> +	 * modern approach can be used.
> +	 */
> +	if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
> +		struct device_node *np;
> +
> +		np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
> +		if (np) {
> +			of_node_put(np);
> +			return -ENXIO;
> +		}
> +	}
> +

What about adding !CONFIG_ARM_SCMI_PROTOCOL in Kconfig dependency and don't
compile the driver at all ?

-- 
viresh

[PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Viresh Kumar]

On 18-04-18, 08:56, Markus Mayer wrote:
> From: Jim Quinlan <jim2101024@gmail.com>
> 
> If the SCMI cpufreq driver is supported, we bail, so that the new
> approach can be used.
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> index b07559b9ed99..b4861a730162 100644
> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> @@ -164,6 +164,8 @@
>  #define BRCM_AVS_CPU_INTR	"brcm,avs-cpu-l2-intr"
>  #define BRCM_AVS_HOST_INTR	"sw_intr"
>  
> +#define ARM_SCMI_COMPAT		"arm,scmi"
> +
>  struct pmap {
>  	unsigned int mode;
>  	unsigned int p1;
> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device *pdev)
>  	struct device *dev;
>  	int host_irq, ret;
>  
> +	/*
> +	 * If the SCMI cpufreq driver is supported, we bail, so that the more
> +	 * modern approach can be used.
> +	 */
> +	if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
> +		struct device_node *np;
> +
> +		np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
> +		if (np) {
> +			of_node_put(np);
> +			return -ENXIO;
> +		}
> +	}
> +

What about adding !CONFIG_ARM_SCMI_PROTOCOL in Kconfig dependency and don't
compile the driver at all ?

-- 
viresh

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Sudeep Holla]

On 18/04/18 16:56, Markus Mayer wrote:
> From: Jim Quinlan <jim2101024@gmail.com>
> 
> If the SCMI cpufreq driver is supported, we bail, so that the new
> approach can be used.
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> index b07559b9ed99..b4861a730162 100644
> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> @@ -164,6 +164,8 @@
>  #define BRCM_AVS_CPU_INTR	"brcm,avs-cpu-l2-intr"
>  #define BRCM_AVS_HOST_INTR	"sw_intr"
>  
> +#define ARM_SCMI_COMPAT		"arm,scmi"
> +
>  struct pmap {
>  	unsigned int mode;
>  	unsigned int p1;
> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device *pdev)
>  	struct device *dev;
>  	int host_irq, ret;
>  

Will this platform have both SCMI and BRCM_AVS_CPU_DATA nodes enabled ?
If so, is it not better to just keep only the preferred node enabled
instead ?

> +	/*
> +	 * If the SCMI cpufreq driver is supported, we bail, so that the more
> +	 * modern approach can be used.
> +	 */
> +	if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
> +		struct device_node *np;
> +
> +		np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
> +		if (np) {
> +			of_node_put(np);
> +			return -ENXIO;
> +		}
> +	}
> +

Clearly not a good approach.

-- 
Regards,
Sudeep

[PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Sudeep Holla]

On 18/04/18 16:56, Markus Mayer wrote:
> From: Jim Quinlan <jim2101024@gmail.com>
> 
> If the SCMI cpufreq driver is supported, we bail, so that the new
> approach can be used.
> 
> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> index b07559b9ed99..b4861a730162 100644
> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> @@ -164,6 +164,8 @@
>  #define BRCM_AVS_CPU_INTR	"brcm,avs-cpu-l2-intr"
>  #define BRCM_AVS_HOST_INTR	"sw_intr"
>  
> +#define ARM_SCMI_COMPAT		"arm,scmi"
> +
>  struct pmap {
>  	unsigned int mode;
>  	unsigned int p1;
> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device *pdev)
>  	struct device *dev;
>  	int host_irq, ret;
>  

Will this platform have both SCMI and BRCM_AVS_CPU_DATA nodes enabled ?
If so, is it not better to just keep only the preferred node enabled
instead ?

> +	/*
> +	 * If the SCMI cpufreq driver is supported, we bail, so that the more
> +	 * modern approach can be used.
> +	 */
> +	if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
> +		struct device_node *np;
> +
> +		np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
> +		if (np) {
> +			of_node_put(np);
> +			return -ENXIO;
> +		}
> +	}
> +

Clearly not a good approach.

-- 
Regards,
Sudeep

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Sudeep Holla]

On 19/04/18 05:16, Viresh Kumar wrote:
> On 18-04-18, 08:56, Markus Mayer wrote:
>> From: Jim Quinlan <jim2101024@gmail.com>
>>
>> If the SCMI cpufreq driver is supported, we bail, so that the new
>> approach can be used.
>>
>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
>> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
>> ---
>>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> index b07559b9ed99..b4861a730162 100644
>> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> @@ -164,6 +164,8 @@
>>  #define BRCM_AVS_CPU_INTR	"brcm,avs-cpu-l2-intr"
>>  #define BRCM_AVS_HOST_INTR	"sw_intr"
>>  
>> +#define ARM_SCMI_COMPAT		"arm,scmi"
>> +
>>  struct pmap {
>>  	unsigned int mode;
>>  	unsigned int p1;
>> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device *pdev)
>>  	struct device *dev;
>>  	int host_irq, ret;
>>  
>> +	/*
>> +	 * If the SCMI cpufreq driver is supported, we bail, so that the more
>> +	 * modern approach can be used.
>> +	 */
>> +	if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
>> +		struct device_node *np;
>> +
>> +		np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
>> +		if (np) {
>> +			of_node_put(np);
>> +			return -ENXIO;
>> +		}
>> +	}
>> +
> 
> What about adding !CONFIG_ARM_SCMI_PROTOCOL in Kconfig dependency and don't
> compile the driver at all ?
> 

Unfortunately, that may not be good idea with single image needing both
configs to be enabled.

-- 
Regards,
Sudeep

[PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Sudeep Holla]

On 19/04/18 05:16, Viresh Kumar wrote:
> On 18-04-18, 08:56, Markus Mayer wrote:
>> From: Jim Quinlan <jim2101024@gmail.com>
>>
>> If the SCMI cpufreq driver is supported, we bail, so that the new
>> approach can be used.
>>
>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
>> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
>> ---
>>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> index b07559b9ed99..b4861a730162 100644
>> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> @@ -164,6 +164,8 @@
>>  #define BRCM_AVS_CPU_INTR	"brcm,avs-cpu-l2-intr"
>>  #define BRCM_AVS_HOST_INTR	"sw_intr"
>>  
>> +#define ARM_SCMI_COMPAT		"arm,scmi"
>> +
>>  struct pmap {
>>  	unsigned int mode;
>>  	unsigned int p1;
>> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device *pdev)
>>  	struct device *dev;
>>  	int host_irq, ret;
>>  
>> +	/*
>> +	 * If the SCMI cpufreq driver is supported, we bail, so that the more
>> +	 * modern approach can be used.
>> +	 */
>> +	if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
>> +		struct device_node *np;
>> +
>> +		np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
>> +		if (np) {
>> +			of_node_put(np);
>> +			return -ENXIO;
>> +		}
>> +	}
>> +
> 
> What about adding !CONFIG_ARM_SCMI_PROTOCOL in Kconfig dependency and don't
> compile the driver at all ?
> 

Unfortunately, that may not be good idea with single image needing both
configs to be enabled.

-- 
Regards,
Sudeep

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Florian Fainelli]

On 04/19/2018 03:35 AM, Sudeep Holla wrote:
> 
> 
> On 18/04/18 16:56, Markus Mayer wrote:
>> From: Jim Quinlan <jim2101024@gmail.com>
>>
>> If the SCMI cpufreq driver is supported, we bail, so that the new
>> approach can be used.
>>
>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
>> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
>> ---
>>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> index b07559b9ed99..b4861a730162 100644
>> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> @@ -164,6 +164,8 @@
>>  #define BRCM_AVS_CPU_INTR	"brcm,avs-cpu-l2-intr"
>>  #define BRCM_AVS_HOST_INTR	"sw_intr"
>>  
>> +#define ARM_SCMI_COMPAT		"arm,scmi"
>> +
>>  struct pmap {
>>  	unsigned int mode;
>>  	unsigned int p1;
>> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device *pdev)
>>  	struct device *dev;
>>  	int host_irq, ret;
>>  
> 
> Will this platform have both SCMI and BRCM_AVS_CPU_DATA nodes enabled ?
> If so, is it not better to just keep only the preferred node enabled
> instead ?

The kernel image has both drivers enabled, the Device Tree blob we pass
contains both nodes, and should flip the status properties based on what
is available. We had some internal discussion about that specific
change, and we ended up having the patch being submitted to seek
external advice, I guess we have an answer now this is not desired.
-- 
Florian

[PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Florian Fainelli]

On 04/19/2018 03:35 AM, Sudeep Holla wrote:
> 
> 
> On 18/04/18 16:56, Markus Mayer wrote:
>> From: Jim Quinlan <jim2101024@gmail.com>
>>
>> If the SCMI cpufreq driver is supported, we bail, so that the new
>> approach can be used.
>>
>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
>> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
>> ---
>>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> index b07559b9ed99..b4861a730162 100644
>> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> @@ -164,6 +164,8 @@
>>  #define BRCM_AVS_CPU_INTR	"brcm,avs-cpu-l2-intr"
>>  #define BRCM_AVS_HOST_INTR	"sw_intr"
>>  
>> +#define ARM_SCMI_COMPAT		"arm,scmi"
>> +
>>  struct pmap {
>>  	unsigned int mode;
>>  	unsigned int p1;
>> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device *pdev)
>>  	struct device *dev;
>>  	int host_irq, ret;
>>  
> 
> Will this platform have both SCMI and BRCM_AVS_CPU_DATA nodes enabled ?
> If so, is it not better to just keep only the preferred node enabled
> instead ?

The kernel image has both drivers enabled, the Device Tree blob we pass
contains both nodes, and should flip the status properties based on what
is available. We had some internal discussion about that specific
change, and we ended up having the patch being submitted to seek
external advice, I guess we have an answer now this is not desired.
-- 
Florian

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Markus Mayer]

On 18 April 2018 at 09:37, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 04/18/2018 08:56 AM, Markus Mayer wrote:
>> From: Jim Quinlan <jim2101024@gmail.com>
>>
>> If the SCMI cpufreq driver is supported, we bail, so that the new
>> approach can be used.
>>
>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
>> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
>> ---
>>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> index b07559b9ed99..b4861a730162 100644
>> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> @@ -164,6 +164,8 @@
>>  #define BRCM_AVS_CPU_INTR    "brcm,avs-cpu-l2-intr"
>>  #define BRCM_AVS_HOST_INTR   "sw_intr"
>>
>> +#define ARM_SCMI_COMPAT              "arm,scmi"
>> +
>>  struct pmap {
>>       unsigned int mode;
>>       unsigned int p1;
>> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device *pdev)
>>       struct device *dev;
>>       int host_irq, ret;
>>
>> +     /*
>> +      * If the SCMI cpufreq driver is supported, we bail, so that the more
>> +      * modern approach can be used.
>> +      */
>> +     if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
>> +             struct device_node *np;
>> +
>> +             np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
>> +             if (np) {
>> +                     of_node_put(np);
>> +                     return -ENXIO;
>> +             }
>
> We would probably want to make sure that the node is also enabled (that
> is, does not have a status = "disabled" property) otherwise the check
> can be defeated. Something like:
>
>                 if (np && of_device_is_available(np))

Would we want something like this instead?

                 if (np) {
                                  bool bail_early =
(of_device_is_available(np) > 0);

                                  of_node_put(np);
                                  if (bail_early)
                                          return -ENXIO;
                 }

To ensure of_node_put() is called?

> should be good for that.
>
> Thanks!
> --
> Florian

[PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Markus Mayer]

On 18 April 2018 at 09:37, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 04/18/2018 08:56 AM, Markus Mayer wrote:
>> From: Jim Quinlan <jim2101024@gmail.com>
>>
>> If the SCMI cpufreq driver is supported, we bail, so that the new
>> approach can be used.
>>
>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
>> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
>> ---
>>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 ++++++++++++++++
>>  1 file changed, 16 insertions(+)
>>
>> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> index b07559b9ed99..b4861a730162 100644
>> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>> @@ -164,6 +164,8 @@
>>  #define BRCM_AVS_CPU_INTR    "brcm,avs-cpu-l2-intr"
>>  #define BRCM_AVS_HOST_INTR   "sw_intr"
>>
>> +#define ARM_SCMI_COMPAT              "arm,scmi"
>> +
>>  struct pmap {
>>       unsigned int mode;
>>       unsigned int p1;
>> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device *pdev)
>>       struct device *dev;
>>       int host_irq, ret;
>>
>> +     /*
>> +      * If the SCMI cpufreq driver is supported, we bail, so that the more
>> +      * modern approach can be used.
>> +      */
>> +     if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
>> +             struct device_node *np;
>> +
>> +             np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
>> +             if (np) {
>> +                     of_node_put(np);
>> +                     return -ENXIO;
>> +             }
>
> We would probably want to make sure that the node is also enabled (that
> is, does not have a status = "disabled" property) otherwise the check
> can be defeated. Something like:
>
>                 if (np && of_device_is_available(np))

Would we want something like this instead?

                 if (np) {
                                  bool bail_early =
(of_device_is_available(np) > 0);

                                  of_node_put(np);
                                  if (bail_early)
                                          return -ENXIO;
                 }

To ensure of_node_put() is called?

> should be good for that.
>
> Thanks!
> --
> Florian

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Viresh Kumar]

On 19-04-18, 11:37, Sudeep Holla wrote:
> 
> 
> On 19/04/18 05:16, Viresh Kumar wrote:
> > On 18-04-18, 08:56, Markus Mayer wrote:
> >> From: Jim Quinlan <jim2101024@gmail.com>
> >>
> >> If the SCMI cpufreq driver is supported, we bail, so that the new
> >> approach can be used.
> >>
> >> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> >> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> >> ---
> >>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 ++++++++++++++++
> >>  1 file changed, 16 insertions(+)
> >>
> >> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> >> index b07559b9ed99..b4861a730162 100644
> >> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> >> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> >> @@ -164,6 +164,8 @@
> >>  #define BRCM_AVS_CPU_INTR	"brcm,avs-cpu-l2-intr"
> >>  #define BRCM_AVS_HOST_INTR	"sw_intr"
> >>  
> >> +#define ARM_SCMI_COMPAT		"arm,scmi"
> >> +
> >>  struct pmap {
> >>  	unsigned int mode;
> >>  	unsigned int p1;
> >> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device *pdev)
> >>  	struct device *dev;
> >>  	int host_irq, ret;
> >>  
> >> +	/*
> >> +	 * If the SCMI cpufreq driver is supported, we bail, so that the more
> >> +	 * modern approach can be used.
> >> +	 */
> >> +	if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
> >> +		struct device_node *np;
> >> +
> >> +		np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
> >> +		if (np) {
> >> +			of_node_put(np);
> >> +			return -ENXIO;
> >> +		}
> >> +	}
> >> +
> > 
> > What about adding !CONFIG_ARM_SCMI_PROTOCOL in Kconfig dependency and don't
> > compile the driver at all ?
> > 
> 
> Unfortunately, that may not be good idea with single image needing both
> configs to be enabled.

Sure, but looking at the above code, it looked like they don't need the other
config if SCMI is enabled.

-- 
viresh

[PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Viresh Kumar]

On 19-04-18, 11:37, Sudeep Holla wrote:
> 
> 
> On 19/04/18 05:16, Viresh Kumar wrote:
> > On 18-04-18, 08:56, Markus Mayer wrote:
> >> From: Jim Quinlan <jim2101024@gmail.com>
> >>
> >> If the SCMI cpufreq driver is supported, we bail, so that the new
> >> approach can be used.
> >>
> >> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
> >> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> >> ---
> >>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 ++++++++++++++++
> >>  1 file changed, 16 insertions(+)
> >>
> >> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> >> index b07559b9ed99..b4861a730162 100644
> >> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> >> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> >> @@ -164,6 +164,8 @@
> >>  #define BRCM_AVS_CPU_INTR	"brcm,avs-cpu-l2-intr"
> >>  #define BRCM_AVS_HOST_INTR	"sw_intr"
> >>  
> >> +#define ARM_SCMI_COMPAT		"arm,scmi"
> >> +
> >>  struct pmap {
> >>  	unsigned int mode;
> >>  	unsigned int p1;
> >> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device *pdev)
> >>  	struct device *dev;
> >>  	int host_irq, ret;
> >>  
> >> +	/*
> >> +	 * If the SCMI cpufreq driver is supported, we bail, so that the more
> >> +	 * modern approach can be used.
> >> +	 */
> >> +	if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
> >> +		struct device_node *np;
> >> +
> >> +		np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
> >> +		if (np) {
> >> +			of_node_put(np);
> >> +			return -ENXIO;
> >> +		}
> >> +	}
> >> +
> > 
> > What about adding !CONFIG_ARM_SCMI_PROTOCOL in Kconfig dependency and don't
> > compile the driver at all ?
> > 
> 
> Unfortunately, that may not be good idea with single image needing both
> configs to be enabled.

Sure, but looking at the above code, it looked like they don't need the other
config if SCMI is enabled.

-- 
viresh

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Sudeep Holla]

On 20/04/18 05:42, Viresh Kumar wrote:
> On 19-04-18, 11:37, Sudeep Holla wrote:
>>
>>
>> On 19/04/18 05:16, Viresh Kumar wrote:
>>> On 18-04-18, 08:56, Markus Mayer wrote:
>>>> From: Jim Quinlan <jim2101024@gmail.com>
>>>>
>>>> If the SCMI cpufreq driver is supported, we bail, so that the new
>>>> approach can be used.
>>>>
>>>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
>>>> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
>>>> ---
>>>>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 ++++++++++++++++
>>>>  1 file changed, 16 insertions(+)
>>>>
>>>> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>>>> index b07559b9ed99..b4861a730162 100644
>>>> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
>>>> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>>>> @@ -164,6 +164,8 @@
>>>>  #define BRCM_AVS_CPU_INTR	"brcm,avs-cpu-l2-intr"
>>>>  #define BRCM_AVS_HOST_INTR	"sw_intr"
>>>>  
>>>> +#define ARM_SCMI_COMPAT		"arm,scmi"
>>>> +
>>>>  struct pmap {
>>>>  	unsigned int mode;
>>>>  	unsigned int p1;
>>>> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device *pdev)
>>>>  	struct device *dev;
>>>>  	int host_irq, ret;
>>>>  
>>>> +	/*
>>>> +	 * If the SCMI cpufreq driver is supported, we bail, so that the more
>>>> +	 * modern approach can be used.
>>>> +	 */
>>>> +	if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
>>>> +		struct device_node *np;
>>>> +
>>>> +		np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
>>>> +		if (np) {
>>>> +			of_node_put(np);
>>>> +			return -ENXIO;
>>>> +		}
>>>> +	}
>>>> +
>>>
>>> What about adding !CONFIG_ARM_SCMI_PROTOCOL in Kconfig dependency and don't
>>> compile the driver at all ?
>>>
>>
>> Unfortunately, that may not be good idea with single image needing both
>> configs to be enabled.
> 
> Sure, but looking at the above code, it looked like they don't need the other
> config if SCMI is enabled.
> 

Yes, I understand that. But if they just want to run a distro kernel or
a defconfig with all the options enabled, then it's not possible. But if
they always build kernel with some custom config options, then fine.

It still doesn't give the flexibility to switch between the two
implementations boot time based on some firmware config(e.g. DT status
property).

-- 
Regards,
Sudeep

[PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Sudeep Holla]

On 20/04/18 05:42, Viresh Kumar wrote:
> On 19-04-18, 11:37, Sudeep Holla wrote:
>>
>>
>> On 19/04/18 05:16, Viresh Kumar wrote:
>>> On 18-04-18, 08:56, Markus Mayer wrote:
>>>> From: Jim Quinlan <jim2101024@gmail.com>
>>>>
>>>> If the SCMI cpufreq driver is supported, we bail, so that the new
>>>> approach can be used.
>>>>
>>>> Signed-off-by: Jim Quinlan <jim2101024@gmail.com>
>>>> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
>>>> ---
>>>>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 16 ++++++++++++++++
>>>>  1 file changed, 16 insertions(+)
>>>>
>>>> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>>>> index b07559b9ed99..b4861a730162 100644
>>>> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
>>>> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
>>>> @@ -164,6 +164,8 @@
>>>>  #define BRCM_AVS_CPU_INTR	"brcm,avs-cpu-l2-intr"
>>>>  #define BRCM_AVS_HOST_INTR	"sw_intr"
>>>>  
>>>> +#define ARM_SCMI_COMPAT		"arm,scmi"
>>>> +
>>>>  struct pmap {
>>>>  	unsigned int mode;
>>>>  	unsigned int p1;
>>>> @@ -511,6 +513,20 @@ static int brcm_avs_prepare_init(struct platform_device *pdev)
>>>>  	struct device *dev;
>>>>  	int host_irq, ret;
>>>>  
>>>> +	/*
>>>> +	 * If the SCMI cpufreq driver is supported, we bail, so that the more
>>>> +	 * modern approach can be used.
>>>> +	 */
>>>> +	if (IS_ENABLED(CONFIG_ARM_SCMI_PROTOCOL)) {
>>>> +		struct device_node *np;
>>>> +
>>>> +		np = of_find_compatible_node(NULL, NULL, ARM_SCMI_COMPAT);
>>>> +		if (np) {
>>>> +			of_node_put(np);
>>>> +			return -ENXIO;
>>>> +		}
>>>> +	}
>>>> +
>>>
>>> What about adding !CONFIG_ARM_SCMI_PROTOCOL in Kconfig dependency and don't
>>> compile the driver at all ?
>>>
>>
>> Unfortunately, that may not be good idea with single image needing both
>> configs to be enabled.
> 
> Sure, but looking at the above code, it looked like they don't need the other
> config if SCMI is enabled.
> 

Yes, I understand that. But if they just want to run a distro kernel or
a defconfig with all the options enabled, then it's not possible. But if
they always build kernel with some custom config options, then fine.

It still doesn't give the flexibility to switch between the two
implementations boot time based on some firmware config(e.g. DT status
property).

-- 
Regards,
Sudeep

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Viresh Kumar]

On 20-04-18, 10:15, Sudeep Holla wrote:
> It still doesn't give the flexibility to switch between the two
> implementations boot time based on some firmware config(e.g. DT status
> property).

I agree, but it didn't look like they need flexibility :)

Lets see how the intend to use it. If they are *always* going to use SCPI if
that is available, then it should be solved at Kconfig level only. Else they
shouldn't put such code in the driver to quit early.

-- 
viresh

[PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Viresh Kumar]

On 20-04-18, 10:15, Sudeep Holla wrote:
> It still doesn't give the flexibility to switch between the two
> implementations boot time based on some firmware config(e.g. DT status
> property).

I agree, but it didn't look like they need flexibility :)

Lets see how the intend to use it. If they are *always* going to use SCPI if
that is available, then it should be solved at Kconfig level only. Else they
shouldn't put such code in the driver to quit early.

-- 
viresh

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Florian Fainelli]

On 04/20/2018 02:35 AM, Viresh Kumar wrote:
> On 20-04-18, 10:15, Sudeep Holla wrote:
>> It still doesn't give the flexibility to switch between the two
>> implementations boot time based on some firmware config(e.g. DT status
>> property).
> 
> I agree, but it didn't look like they need flexibility :)
> 
> Lets see how the intend to use it. If they are *always* going to use SCPI if
> that is available, then it should be solved at Kconfig level only. Else they
> shouldn't put such code in the driver to quit early.

We have both drivers (brcmstb-avs-cpufreq and scmi-cpufreq) enabled in
our kernel configuration, however, depending on the firmware version, we
may have a number of combinations:

- arm,scmi DT node is present and enabled (status = okay) as well as
brcmstb-avs-cpufreq being present and enabled
- arm,scmi DT node is present but disabled (status = disabled) and
brcmstb-avs-cpufreq is being present and enabled

If you think this is a self inflicted, downstream and backwards/forwards
compatible relevant only change, I suppose we are fine with that too.
-- 
Florian

[PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Florian Fainelli]

On 04/20/2018 02:35 AM, Viresh Kumar wrote:
> On 20-04-18, 10:15, Sudeep Holla wrote:
>> It still doesn't give the flexibility to switch between the two
>> implementations boot time based on some firmware config(e.g. DT status
>> property).
> 
> I agree, but it didn't look like they need flexibility :)
> 
> Lets see how the intend to use it. If they are *always* going to use SCPI if
> that is available, then it should be solved at Kconfig level only. Else they
> shouldn't put such code in the driver to quit early.

We have both drivers (brcmstb-avs-cpufreq and scmi-cpufreq) enabled in
our kernel configuration, however, depending on the firmware version, we
may have a number of combinations:

- arm,scmi DT node is present and enabled (status = okay) as well as
brcmstb-avs-cpufreq being present and enabled
- arm,scmi DT node is present but disabled (status = disabled) and
brcmstb-avs-cpufreq is being present and enabled

If you think this is a self inflicted, downstream and backwards/forwards
compatible relevant only change, I suppose we are fine with that too.
-- 
Florian

Re: [PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Viresh Kumar]

On 20-04-18, 09:50, Florian Fainelli wrote:
> On 04/20/2018 02:35 AM, Viresh Kumar wrote:
> > On 20-04-18, 10:15, Sudeep Holla wrote:
> >> It still doesn't give the flexibility to switch between the two
> >> implementations boot time based on some firmware config(e.g. DT status
> >> property).
> > 
> > I agree, but it didn't look like they need flexibility :)
> > 
> > Lets see how the intend to use it. If they are *always* going to use SCPI if
> > that is available, then it should be solved at Kconfig level only. Else they
> > shouldn't put such code in the driver to quit early.
> 
> We have both drivers (brcmstb-avs-cpufreq and scmi-cpufreq) enabled in
> our kernel configuration, however, depending on the firmware version, we
> may have a number of combinations:
> 
> - arm,scmi DT node is present and enabled (status = okay) as well as
> brcmstb-avs-cpufreq being present and enabled
> - arm,scmi DT node is present but disabled (status = disabled) and
> brcmstb-avs-cpufreq is being present and enabled

In this case the Kconfig thing I have been talking about doesn't apply anymore.

-- 
viresh

[PATCH 2/2] cpufreq: brcmstb-avs-cpufreq: prefer SCMI cpufreq if supported

[Viresh Kumar]

On 20-04-18, 09:50, Florian Fainelli wrote:
> On 04/20/2018 02:35 AM, Viresh Kumar wrote:
> > On 20-04-18, 10:15, Sudeep Holla wrote:
> >> It still doesn't give the flexibility to switch between the two
> >> implementations boot time based on some firmware config(e.g. DT status
> >> property).
> > 
> > I agree, but it didn't look like they need flexibility :)
> > 
> > Lets see how the intend to use it. If they are *always* going to use SCPI if
> > that is available, then it should be solved at Kconfig level only. Else they
> > shouldn't put such code in the driver to quit early.
> 
> We have both drivers (brcmstb-avs-cpufreq and scmi-cpufreq) enabled in
> our kernel configuration, however, depending on the firmware version, we
> may have a number of combinations:
> 
> - arm,scmi DT node is present and enabled (status = okay) as well as
> brcmstb-avs-cpufreq being present and enabled
> - arm,scmi DT node is present but disabled (status = disabled) and
> brcmstb-avs-cpufreq is being present and enabled

In this case the Kconfig thing I have been talking about doesn't apply anymore.

-- 
viresh

Re: [PATCH 1/2] cpufreq: brcmstb-avs-cpufreq: remove development debug support

[Rafael J. Wysocki]

On Wednesday, April 18, 2018 5:56:42 PM CEST Markus Mayer wrote:
> From: Markus Mayer <mmayer@broadcom.com>
> 
> This debug code was helpful while developing the driver, but it isn't
> being used for anything anymore.
> 
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  drivers/cpufreq/Kconfig.arm           |  10 --
>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 323 +---------------------------------
>  2 files changed, 1 insertion(+), 332 deletions(-)
> 
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 7f56fe5183f2..de55c7d57438 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -71,16 +71,6 @@ config ARM_BRCMSTB_AVS_CPUFREQ
>  
>  	  Say Y, if you have a Broadcom SoC with AVS support for DFS or DVFS.
>  
> -config ARM_BRCMSTB_AVS_CPUFREQ_DEBUG
> -	bool "Broadcom STB AVS CPUfreq driver sysfs debug capability"
> -	depends on ARM_BRCMSTB_AVS_CPUFREQ
> -	help
> -	  Enabling this option turns on debug support via sysfs under
> -	  /sys/kernel/debug/brcmstb-avs-cpufreq. It is possible to read all and
> -	  write some AVS mailbox registers through sysfs entries.
> -
> -	  If in doubt, say N.
> -
>  config ARM_EXYNOS5440_CPUFREQ
>  	tristate "SAMSUNG EXYNOS5440"
>  	depends on SOC_EXYNOS5440
> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> index 6cdac1aaf23c..b07559b9ed99 100644
> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> @@ -49,13 +49,6 @@
>  #include <linux/platform_device.h>
>  #include <linux/semaphore.h>
>  
> -#ifdef CONFIG_ARM_BRCMSTB_AVS_CPUFREQ_DEBUG
> -#include <linux/ctype.h>
> -#include <linux/debugfs.h>
> -#include <linux/slab.h>
> -#include <linux/uaccess.h>
> -#endif
> -
>  /* Max number of arguments AVS calls take */
>  #define AVS_MAX_CMD_ARGS	4
>  /*
> @@ -182,88 +175,11 @@ struct private_data {
>  	void __iomem *base;
>  	void __iomem *avs_intr_base;
>  	struct device *dev;
> -#ifdef CONFIG_ARM_BRCMSTB_AVS_CPUFREQ_DEBUG
> -	struct dentry *debugfs;
> -#endif
>  	struct completion done;
>  	struct semaphore sem;
>  	struct pmap pmap;
>  };
>  
> -#ifdef CONFIG_ARM_BRCMSTB_AVS_CPUFREQ_DEBUG
> -
> -enum debugfs_format {
> -	DEBUGFS_NORMAL,
> -	DEBUGFS_FLOAT,
> -	DEBUGFS_REV,
> -};
> -
> -struct debugfs_data {
> -	struct debugfs_entry *entry;
> -	struct private_data *priv;
> -};
> -
> -struct debugfs_entry {
> -	char *name;
> -	u32 offset;
> -	fmode_t mode;
> -	enum debugfs_format format;
> -};
> -
> -#define DEBUGFS_ENTRY(name, mode, format)	{ \
> -	#name, AVS_MBOX_##name, mode, format \
> -}
> -
> -/*
> - * These are used for debugfs only. Otherwise we use AVS_MBOX_PARAM() directly.
> - */
> -#define AVS_MBOX_PARAM1		AVS_MBOX_PARAM(0)
> -#define AVS_MBOX_PARAM2		AVS_MBOX_PARAM(1)
> -#define AVS_MBOX_PARAM3		AVS_MBOX_PARAM(2)
> -#define AVS_MBOX_PARAM4		AVS_MBOX_PARAM(3)
> -
> -/*
> - * This table stores the name, access permissions and offset for each hardware
> - * register and is used to generate debugfs entries.
> - */
> -static struct debugfs_entry debugfs_entries[] = {
> -	DEBUGFS_ENTRY(COMMAND, S_IWUSR, DEBUGFS_NORMAL),
> -	DEBUGFS_ENTRY(STATUS, S_IWUSR, DEBUGFS_NORMAL),
> -	DEBUGFS_ENTRY(VOLTAGE0, 0, DEBUGFS_FLOAT),
> -	DEBUGFS_ENTRY(TEMP0, 0, DEBUGFS_FLOAT),
> -	DEBUGFS_ENTRY(PV0, 0, DEBUGFS_FLOAT),
> -	DEBUGFS_ENTRY(MV0, 0, DEBUGFS_FLOAT),
> -	DEBUGFS_ENTRY(PARAM1, S_IWUSR, DEBUGFS_NORMAL),
> -	DEBUGFS_ENTRY(PARAM2, S_IWUSR, DEBUGFS_NORMAL),
> -	DEBUGFS_ENTRY(PARAM3, S_IWUSR, DEBUGFS_NORMAL),
> -	DEBUGFS_ENTRY(PARAM4, S_IWUSR, DEBUGFS_NORMAL),
> -	DEBUGFS_ENTRY(REVISION, 0, DEBUGFS_REV),
> -	DEBUGFS_ENTRY(PSTATE, 0, DEBUGFS_NORMAL),
> -	DEBUGFS_ENTRY(HEARTBEAT, 0, DEBUGFS_NORMAL),
> -	DEBUGFS_ENTRY(MAGIC, S_IWUSR, DEBUGFS_NORMAL),
> -	DEBUGFS_ENTRY(SIGMA_HVT, 0, DEBUGFS_NORMAL),
> -	DEBUGFS_ENTRY(SIGMA_SVT, 0, DEBUGFS_NORMAL),
> -	DEBUGFS_ENTRY(VOLTAGE1, 0, DEBUGFS_FLOAT),
> -	DEBUGFS_ENTRY(TEMP1, 0, DEBUGFS_FLOAT),
> -	DEBUGFS_ENTRY(PV1, 0, DEBUGFS_FLOAT),
> -	DEBUGFS_ENTRY(MV1, 0, DEBUGFS_FLOAT),
> -	DEBUGFS_ENTRY(FREQUENCY, 0, DEBUGFS_NORMAL),
> -};
> -
> -static int brcm_avs_target_index(struct cpufreq_policy *, unsigned int);
> -
> -static char *__strtolower(char *s)
> -{
> -	char *p;
> -
> -	for (p = s; *p; p++)
> -		*p = tolower(*p);
> -
> -	return s;
> -}
> -
> -#endif /* CONFIG_ARM_BRCMSTB_AVS_CPUFREQ_DEBUG */
> -
>  static void __iomem *__map_region(const char *name)
>  {
>  	struct device_node *np;
> @@ -516,238 +432,6 @@ brcm_avs_get_freq_table(struct device *dev, struct private_data *priv)
>  	return table;
>  }
>  
> -#ifdef CONFIG_ARM_BRCMSTB_AVS_CPUFREQ_DEBUG
> -
> -#define MANT(x)	(unsigned int)(abs((x)) / 1000)
> -#define FRAC(x)	(unsigned int)(abs((x)) - abs((x)) / 1000 * 1000)
> -
> -static int brcm_avs_debug_show(struct seq_file *s, void *data)
> -{
> -	struct debugfs_data *dbgfs = s->private;
> -	void __iomem *base;
> -	u32 val, offset;
> -
> -	if (!dbgfs) {
> -		seq_puts(s, "No device pointer\n");
> -		return 0;
> -	}
> -
> -	base = dbgfs->priv->base;
> -	offset = dbgfs->entry->offset;
> -	val = readl(base + offset);
> -	switch (dbgfs->entry->format) {
> -	case DEBUGFS_NORMAL:
> -		seq_printf(s, "%u\n", val);
> -		break;
> -	case DEBUGFS_FLOAT:
> -		seq_printf(s, "%d.%03d\n", MANT(val), FRAC(val));
> -		break;
> -	case DEBUGFS_REV:
> -		seq_printf(s, "%c.%c.%c.%c\n", (val >> 24 & 0xff),
> -			   (val >> 16 & 0xff), (val >> 8 & 0xff),
> -			   val & 0xff);
> -		break;
> -	}
> -	seq_printf(s, "0x%08x\n", val);
> -
> -	return 0;
> -}
> -
> -#undef MANT
> -#undef FRAC
> -
> -static ssize_t brcm_avs_seq_write(struct file *file, const char __user *buf,
> -				  size_t size, loff_t *ppos)
> -{
> -	struct seq_file *s = file->private_data;
> -	struct debugfs_data *dbgfs = s->private;
> -	struct private_data *priv = dbgfs->priv;
> -	void __iomem *base, *avs_intr_base;
> -	bool use_issue_command = false;
> -	unsigned long val, offset;
> -	char str[128];
> -	int ret;
> -	char *str_ptr = str;
> -
> -	if (size >= sizeof(str))
> -		return -E2BIG;
> -
> -	memset(str, 0, sizeof(str));
> -	ret = copy_from_user(str, buf, size);
> -	if (ret)
> -		return ret;
> -
> -	base = priv->base;
> -	avs_intr_base = priv->avs_intr_base;
> -	offset = dbgfs->entry->offset;
> -	/*
> -	 * Special case writing to "command" entry only: if the string starts
> -	 * with a 'c', we use the driver's __issue_avs_command() function.
> -	 * Otherwise, we perform a raw write. This should allow testing of raw
> -	 * access as well as using the higher level function. (Raw access
> -	 * doesn't clear the firmware return status after issuing the command.)
> -	 */
> -	if (str_ptr[0] == 'c' && offset == AVS_MBOX_COMMAND) {
> -		use_issue_command = true;
> -		str_ptr++;
> -	}
> -	if (kstrtoul(str_ptr, 0, &val) != 0)
> -		return -EINVAL;
> -
> -	/*
> -	 * Setting the P-state is a special case. We need to update the CPU
> -	 * frequency we report.
> -	 */
> -	if (val == AVS_CMD_SET_PSTATE) {
> -		struct cpufreq_policy *policy;
> -		unsigned int pstate;
> -
> -		policy = cpufreq_cpu_get(smp_processor_id());
> -		/* Read back the P-state we are about to set */
> -		pstate = readl(base + AVS_MBOX_PARAM(0));
> -		if (use_issue_command) {
> -			ret = brcm_avs_target_index(policy, pstate);
> -			return ret ? ret : size;
> -		}
> -		policy->cur = policy->freq_table[pstate].frequency;
> -	}
> -
> -	if (use_issue_command) {
> -		ret = __issue_avs_command(priv, val, false, NULL);
> -	} else {
> -		/* Locking here is not perfect, but is only for debug. */
> -		ret = down_interruptible(&priv->sem);
> -		if (ret)
> -			return ret;
> -
> -		writel(val, base + offset);
> -		/* We have to wake up the firmware to process a command. */
> -		if (offset == AVS_MBOX_COMMAND)
> -			writel(AVS_CPU_L2_INT_MASK,
> -			       avs_intr_base + AVS_CPU_L2_SET0);
> -		up(&priv->sem);
> -	}
> -
> -	return ret ? ret : size;
> -}
> -
> -static struct debugfs_entry *__find_debugfs_entry(const char *name)
> -{
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(debugfs_entries); i++)
> -		if (strcasecmp(debugfs_entries[i].name, name) == 0)
> -			return &debugfs_entries[i];
> -
> -	return NULL;
> -}
> -
> -static int brcm_avs_debug_open(struct inode *inode, struct file *file)
> -{
> -	struct debugfs_data *data;
> -	fmode_t fmode;
> -	int ret;
> -
> -	/*
> -	 * seq_open(), which is called by single_open(), clears "write" access.
> -	 * We need write access to some files, so we preserve our access mode
> -	 * and restore it.
> -	 */
> -	fmode = file->f_mode;
> -	/*
> -	 * Check access permissions even for root. We don't want to be writing
> -	 * to read-only registers. Access for regular users has already been
> -	 * checked by the VFS layer.
> -	 */
> -	if ((fmode & FMODE_WRITER) && !(inode->i_mode & S_IWUSR))
> -		return -EACCES;
> -
> -	data = kmalloc(sizeof(*data), GFP_KERNEL);
> -	if (!data)
> -		return -ENOMEM;
> -	/*
> -	 * We use the same file system operations for all our debug files. To
> -	 * produce specific output, we look up the file name upon opening a
> -	 * debugfs entry and map it to a memory offset. This offset is then used
> -	 * in the generic "show" function to read a specific register.
> -	 */
> -	data->entry = __find_debugfs_entry(file->f_path.dentry->d_iname);
> -	data->priv = inode->i_private;
> -
> -	ret = single_open(file, brcm_avs_debug_show, data);
> -	if (ret)
> -		kfree(data);
> -	file->f_mode = fmode;
> -
> -	return ret;
> -}
> -
> -static int brcm_avs_debug_release(struct inode *inode, struct file *file)
> -{
> -	struct seq_file *seq_priv = file->private_data;
> -	struct debugfs_data *data = seq_priv->private;
> -
> -	kfree(data);
> -	return single_release(inode, file);
> -}
> -
> -static const struct file_operations brcm_avs_debug_ops = {
> -	.open		= brcm_avs_debug_open,
> -	.read		= seq_read,
> -	.write		= brcm_avs_seq_write,
> -	.llseek		= seq_lseek,
> -	.release	= brcm_avs_debug_release,
> -};
> -
> -static void brcm_avs_cpufreq_debug_init(struct platform_device *pdev)
> -{
> -	struct private_data *priv = platform_get_drvdata(pdev);
> -	struct dentry *dir;
> -	int i;
> -
> -	if (!priv)
> -		return;
> -
> -	dir = debugfs_create_dir(BRCM_AVS_CPUFREQ_NAME, NULL);
> -	if (IS_ERR_OR_NULL(dir))
> -		return;
> -	priv->debugfs = dir;
> -
> -	for (i = 0; i < ARRAY_SIZE(debugfs_entries); i++) {
> -		/*
> -		 * The DEBUGFS_ENTRY macro generates uppercase strings. We
> -		 * convert them to lowercase before creating the debugfs
> -		 * entries.
> -		 */
> -		char *entry = __strtolower(debugfs_entries[i].name);
> -		fmode_t mode = debugfs_entries[i].mode;
> -
> -		if (!debugfs_create_file(entry, S_IFREG | S_IRUGO | mode,
> -					 dir, priv, &brcm_avs_debug_ops)) {
> -			priv->debugfs = NULL;
> -			debugfs_remove_recursive(dir);
> -			break;
> -		}
> -	}
> -}
> -
> -static void brcm_avs_cpufreq_debug_exit(struct platform_device *pdev)
> -{
> -	struct private_data *priv = platform_get_drvdata(pdev);
> -
> -	if (priv && priv->debugfs) {
> -		debugfs_remove_recursive(priv->debugfs);
> -		priv->debugfs = NULL;
> -	}
> -}
> -
> -#else
> -
> -static void brcm_avs_cpufreq_debug_init(struct platform_device *pdev) {}
> -static void brcm_avs_cpufreq_debug_exit(struct platform_device *pdev) {}
> -
> -#endif /* CONFIG_ARM_BRCMSTB_AVS_CPUFREQ_DEBUG */
> -
>  /*
>   * To ensure the right firmware is running we need to
>   *    - check the MAGIC matches what we expect
> @@ -1016,11 +700,8 @@ static int brcm_avs_cpufreq_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	brcm_avs_driver.driver_data = pdev;
> -	ret = cpufreq_register_driver(&brcm_avs_driver);
> -	if (!ret)
> -		brcm_avs_cpufreq_debug_init(pdev);
>  
> -	return ret;
> +	return cpufreq_register_driver(&brcm_avs_driver);
>  }
>  
>  static int brcm_avs_cpufreq_remove(struct platform_device *pdev)
> @@ -1032,8 +713,6 @@ static int brcm_avs_cpufreq_remove(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	brcm_avs_cpufreq_debug_exit(pdev);
> -
>  	priv = platform_get_drvdata(pdev);
>  	iounmap(priv->base);
>  	iounmap(priv->avs_intr_base);
> 

Applied and merged into 4.17-rc3, thanks!

[PATCH 1/2] cpufreq: brcmstb-avs-cpufreq: remove development debug support

[Rafael J. Wysocki]

On Wednesday, April 18, 2018 5:56:42 PM CEST Markus Mayer wrote:
> From: Markus Mayer <mmayer@broadcom.com>
> 
> This debug code was helpful while developing the driver, but it isn't
> being used for anything anymore.
> 
> Signed-off-by: Markus Mayer <mmayer@broadcom.com>
> ---
>  drivers/cpufreq/Kconfig.arm           |  10 --
>  drivers/cpufreq/brcmstb-avs-cpufreq.c | 323 +---------------------------------
>  2 files changed, 1 insertion(+), 332 deletions(-)
> 
> diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
> index 7f56fe5183f2..de55c7d57438 100644
> --- a/drivers/cpufreq/Kconfig.arm
> +++ b/drivers/cpufreq/Kconfig.arm
> @@ -71,16 +71,6 @@ config ARM_BRCMSTB_AVS_CPUFREQ
>  
>  	  Say Y, if you have a Broadcom SoC with AVS support for DFS or DVFS.
>  
> -config ARM_BRCMSTB_AVS_CPUFREQ_DEBUG
> -	bool "Broadcom STB AVS CPUfreq driver sysfs debug capability"
> -	depends on ARM_BRCMSTB_AVS_CPUFREQ
> -	help
> -	  Enabling this option turns on debug support via sysfs under
> -	  /sys/kernel/debug/brcmstb-avs-cpufreq. It is possible to read all and
> -	  write some AVS mailbox registers through sysfs entries.
> -
> -	  If in doubt, say N.
> -
>  config ARM_EXYNOS5440_CPUFREQ
>  	tristate "SAMSUNG EXYNOS5440"
>  	depends on SOC_EXYNOS5440
> diff --git a/drivers/cpufreq/brcmstb-avs-cpufreq.c b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> index 6cdac1aaf23c..b07559b9ed99 100644
> --- a/drivers/cpufreq/brcmstb-avs-cpufreq.c
> +++ b/drivers/cpufreq/brcmstb-avs-cpufreq.c
> @@ -49,13 +49,6 @@
>  #include <linux/platform_device.h>
>  #include <linux/semaphore.h>
>  
> -#ifdef CONFIG_ARM_BRCMSTB_AVS_CPUFREQ_DEBUG
> -#include <linux/ctype.h>
> -#include <linux/debugfs.h>
> -#include <linux/slab.h>
> -#include <linux/uaccess.h>
> -#endif
> -
>  /* Max number of arguments AVS calls take */
>  #define AVS_MAX_CMD_ARGS	4
>  /*
> @@ -182,88 +175,11 @@ struct private_data {
>  	void __iomem *base;
>  	void __iomem *avs_intr_base;
>  	struct device *dev;
> -#ifdef CONFIG_ARM_BRCMSTB_AVS_CPUFREQ_DEBUG
> -	struct dentry *debugfs;
> -#endif
>  	struct completion done;
>  	struct semaphore sem;
>  	struct pmap pmap;
>  };
>  
> -#ifdef CONFIG_ARM_BRCMSTB_AVS_CPUFREQ_DEBUG
> -
> -enum debugfs_format {
> -	DEBUGFS_NORMAL,
> -	DEBUGFS_FLOAT,
> -	DEBUGFS_REV,
> -};
> -
> -struct debugfs_data {
> -	struct debugfs_entry *entry;
> -	struct private_data *priv;
> -};
> -
> -struct debugfs_entry {
> -	char *name;
> -	u32 offset;
> -	fmode_t mode;
> -	enum debugfs_format format;
> -};
> -
> -#define DEBUGFS_ENTRY(name, mode, format)	{ \
> -	#name, AVS_MBOX_##name, mode, format \
> -}
> -
> -/*
> - * These are used for debugfs only. Otherwise we use AVS_MBOX_PARAM() directly.
> - */
> -#define AVS_MBOX_PARAM1		AVS_MBOX_PARAM(0)
> -#define AVS_MBOX_PARAM2		AVS_MBOX_PARAM(1)
> -#define AVS_MBOX_PARAM3		AVS_MBOX_PARAM(2)
> -#define AVS_MBOX_PARAM4		AVS_MBOX_PARAM(3)
> -
> -/*
> - * This table stores the name, access permissions and offset for each hardware
> - * register and is used to generate debugfs entries.
> - */
> -static struct debugfs_entry debugfs_entries[] = {
> -	DEBUGFS_ENTRY(COMMAND, S_IWUSR, DEBUGFS_NORMAL),
> -	DEBUGFS_ENTRY(STATUS, S_IWUSR, DEBUGFS_NORMAL),
> -	DEBUGFS_ENTRY(VOLTAGE0, 0, DEBUGFS_FLOAT),
> -	DEBUGFS_ENTRY(TEMP0, 0, DEBUGFS_FLOAT),
> -	DEBUGFS_ENTRY(PV0, 0, DEBUGFS_FLOAT),
> -	DEBUGFS_ENTRY(MV0, 0, DEBUGFS_FLOAT),
> -	DEBUGFS_ENTRY(PARAM1, S_IWUSR, DEBUGFS_NORMAL),
> -	DEBUGFS_ENTRY(PARAM2, S_IWUSR, DEBUGFS_NORMAL),
> -	DEBUGFS_ENTRY(PARAM3, S_IWUSR, DEBUGFS_NORMAL),
> -	DEBUGFS_ENTRY(PARAM4, S_IWUSR, DEBUGFS_NORMAL),
> -	DEBUGFS_ENTRY(REVISION, 0, DEBUGFS_REV),
> -	DEBUGFS_ENTRY(PSTATE, 0, DEBUGFS_NORMAL),
> -	DEBUGFS_ENTRY(HEARTBEAT, 0, DEBUGFS_NORMAL),
> -	DEBUGFS_ENTRY(MAGIC, S_IWUSR, DEBUGFS_NORMAL),
> -	DEBUGFS_ENTRY(SIGMA_HVT, 0, DEBUGFS_NORMAL),
> -	DEBUGFS_ENTRY(SIGMA_SVT, 0, DEBUGFS_NORMAL),
> -	DEBUGFS_ENTRY(VOLTAGE1, 0, DEBUGFS_FLOAT),
> -	DEBUGFS_ENTRY(TEMP1, 0, DEBUGFS_FLOAT),
> -	DEBUGFS_ENTRY(PV1, 0, DEBUGFS_FLOAT),
> -	DEBUGFS_ENTRY(MV1, 0, DEBUGFS_FLOAT),
> -	DEBUGFS_ENTRY(FREQUENCY, 0, DEBUGFS_NORMAL),
> -};
> -
> -static int brcm_avs_target_index(struct cpufreq_policy *, unsigned int);
> -
> -static char *__strtolower(char *s)
> -{
> -	char *p;
> -
> -	for (p = s; *p; p++)
> -		*p = tolower(*p);
> -
> -	return s;
> -}
> -
> -#endif /* CONFIG_ARM_BRCMSTB_AVS_CPUFREQ_DEBUG */
> -
>  static void __iomem *__map_region(const char *name)
>  {
>  	struct device_node *np;
> @@ -516,238 +432,6 @@ brcm_avs_get_freq_table(struct device *dev, struct private_data *priv)
>  	return table;
>  }
>  
> -#ifdef CONFIG_ARM_BRCMSTB_AVS_CPUFREQ_DEBUG
> -
> -#define MANT(x)	(unsigned int)(abs((x)) / 1000)
> -#define FRAC(x)	(unsigned int)(abs((x)) - abs((x)) / 1000 * 1000)
> -
> -static int brcm_avs_debug_show(struct seq_file *s, void *data)
> -{
> -	struct debugfs_data *dbgfs = s->private;
> -	void __iomem *base;
> -	u32 val, offset;
> -
> -	if (!dbgfs) {
> -		seq_puts(s, "No device pointer\n");
> -		return 0;
> -	}
> -
> -	base = dbgfs->priv->base;
> -	offset = dbgfs->entry->offset;
> -	val = readl(base + offset);
> -	switch (dbgfs->entry->format) {
> -	case DEBUGFS_NORMAL:
> -		seq_printf(s, "%u\n", val);
> -		break;
> -	case DEBUGFS_FLOAT:
> -		seq_printf(s, "%d.%03d\n", MANT(val), FRAC(val));
> -		break;
> -	case DEBUGFS_REV:
> -		seq_printf(s, "%c.%c.%c.%c\n", (val >> 24 & 0xff),
> -			   (val >> 16 & 0xff), (val >> 8 & 0xff),
> -			   val & 0xff);
> -		break;
> -	}
> -	seq_printf(s, "0x%08x\n", val);
> -
> -	return 0;
> -}
> -
> -#undef MANT
> -#undef FRAC
> -
> -static ssize_t brcm_avs_seq_write(struct file *file, const char __user *buf,
> -				  size_t size, loff_t *ppos)
> -{
> -	struct seq_file *s = file->private_data;
> -	struct debugfs_data *dbgfs = s->private;
> -	struct private_data *priv = dbgfs->priv;
> -	void __iomem *base, *avs_intr_base;
> -	bool use_issue_command = false;
> -	unsigned long val, offset;
> -	char str[128];
> -	int ret;
> -	char *str_ptr = str;
> -
> -	if (size >= sizeof(str))
> -		return -E2BIG;
> -
> -	memset(str, 0, sizeof(str));
> -	ret = copy_from_user(str, buf, size);
> -	if (ret)
> -		return ret;
> -
> -	base = priv->base;
> -	avs_intr_base = priv->avs_intr_base;
> -	offset = dbgfs->entry->offset;
> -	/*
> -	 * Special case writing to "command" entry only: if the string starts
> -	 * with a 'c', we use the driver's __issue_avs_command() function.
> -	 * Otherwise, we perform a raw write. This should allow testing of raw
> -	 * access as well as using the higher level function. (Raw access
> -	 * doesn't clear the firmware return status after issuing the command.)
> -	 */
> -	if (str_ptr[0] == 'c' && offset == AVS_MBOX_COMMAND) {
> -		use_issue_command = true;
> -		str_ptr++;
> -	}
> -	if (kstrtoul(str_ptr, 0, &val) != 0)
> -		return -EINVAL;
> -
> -	/*
> -	 * Setting the P-state is a special case. We need to update the CPU
> -	 * frequency we report.
> -	 */
> -	if (val == AVS_CMD_SET_PSTATE) {
> -		struct cpufreq_policy *policy;
> -		unsigned int pstate;
> -
> -		policy = cpufreq_cpu_get(smp_processor_id());
> -		/* Read back the P-state we are about to set */
> -		pstate = readl(base + AVS_MBOX_PARAM(0));
> -		if (use_issue_command) {
> -			ret = brcm_avs_target_index(policy, pstate);
> -			return ret ? ret : size;
> -		}
> -		policy->cur = policy->freq_table[pstate].frequency;
> -	}
> -
> -	if (use_issue_command) {
> -		ret = __issue_avs_command(priv, val, false, NULL);
> -	} else {
> -		/* Locking here is not perfect, but is only for debug. */
> -		ret = down_interruptible(&priv->sem);
> -		if (ret)
> -			return ret;
> -
> -		writel(val, base + offset);
> -		/* We have to wake up the firmware to process a command. */
> -		if (offset == AVS_MBOX_COMMAND)
> -			writel(AVS_CPU_L2_INT_MASK,
> -			       avs_intr_base + AVS_CPU_L2_SET0);
> -		up(&priv->sem);
> -	}
> -
> -	return ret ? ret : size;
> -}
> -
> -static struct debugfs_entry *__find_debugfs_entry(const char *name)
> -{
> -	int i;
> -
> -	for (i = 0; i < ARRAY_SIZE(debugfs_entries); i++)
> -		if (strcasecmp(debugfs_entries[i].name, name) == 0)
> -			return &debugfs_entries[i];
> -
> -	return NULL;
> -}
> -
> -static int brcm_avs_debug_open(struct inode *inode, struct file *file)
> -{
> -	struct debugfs_data *data;
> -	fmode_t fmode;
> -	int ret;
> -
> -	/*
> -	 * seq_open(), which is called by single_open(), clears "write" access.
> -	 * We need write access to some files, so we preserve our access mode
> -	 * and restore it.
> -	 */
> -	fmode = file->f_mode;
> -	/*
> -	 * Check access permissions even for root. We don't want to be writing
> -	 * to read-only registers. Access for regular users has already been
> -	 * checked by the VFS layer.
> -	 */
> -	if ((fmode & FMODE_WRITER) && !(inode->i_mode & S_IWUSR))
> -		return -EACCES;
> -
> -	data = kmalloc(sizeof(*data), GFP_KERNEL);
> -	if (!data)
> -		return -ENOMEM;
> -	/*
> -	 * We use the same file system operations for all our debug files. To
> -	 * produce specific output, we look up the file name upon opening a
> -	 * debugfs entry and map it to a memory offset. This offset is then used
> -	 * in the generic "show" function to read a specific register.
> -	 */
> -	data->entry = __find_debugfs_entry(file->f_path.dentry->d_iname);
> -	data->priv = inode->i_private;
> -
> -	ret = single_open(file, brcm_avs_debug_show, data);
> -	if (ret)
> -		kfree(data);
> -	file->f_mode = fmode;
> -
> -	return ret;
> -}
> -
> -static int brcm_avs_debug_release(struct inode *inode, struct file *file)
> -{
> -	struct seq_file *seq_priv = file->private_data;
> -	struct debugfs_data *data = seq_priv->private;
> -
> -	kfree(data);
> -	return single_release(inode, file);
> -}
> -
> -static const struct file_operations brcm_avs_debug_ops = {
> -	.open		= brcm_avs_debug_open,
> -	.read		= seq_read,
> -	.write		= brcm_avs_seq_write,
> -	.llseek		= seq_lseek,
> -	.release	= brcm_avs_debug_release,
> -};
> -
> -static void brcm_avs_cpufreq_debug_init(struct platform_device *pdev)
> -{
> -	struct private_data *priv = platform_get_drvdata(pdev);
> -	struct dentry *dir;
> -	int i;
> -
> -	if (!priv)
> -		return;
> -
> -	dir = debugfs_create_dir(BRCM_AVS_CPUFREQ_NAME, NULL);
> -	if (IS_ERR_OR_NULL(dir))
> -		return;
> -	priv->debugfs = dir;
> -
> -	for (i = 0; i < ARRAY_SIZE(debugfs_entries); i++) {
> -		/*
> -		 * The DEBUGFS_ENTRY macro generates uppercase strings. We
> -		 * convert them to lowercase before creating the debugfs
> -		 * entries.
> -		 */
> -		char *entry = __strtolower(debugfs_entries[i].name);
> -		fmode_t mode = debugfs_entries[i].mode;
> -
> -		if (!debugfs_create_file(entry, S_IFREG | S_IRUGO | mode,
> -					 dir, priv, &brcm_avs_debug_ops)) {
> -			priv->debugfs = NULL;
> -			debugfs_remove_recursive(dir);
> -			break;
> -		}
> -	}
> -}
> -
> -static void brcm_avs_cpufreq_debug_exit(struct platform_device *pdev)
> -{
> -	struct private_data *priv = platform_get_drvdata(pdev);
> -
> -	if (priv && priv->debugfs) {
> -		debugfs_remove_recursive(priv->debugfs);
> -		priv->debugfs = NULL;
> -	}
> -}
> -
> -#else
> -
> -static void brcm_avs_cpufreq_debug_init(struct platform_device *pdev) {}
> -static void brcm_avs_cpufreq_debug_exit(struct platform_device *pdev) {}
> -
> -#endif /* CONFIG_ARM_BRCMSTB_AVS_CPUFREQ_DEBUG */
> -
>  /*
>   * To ensure the right firmware is running we need to
>   *    - check the MAGIC matches what we expect
> @@ -1016,11 +700,8 @@ static int brcm_avs_cpufreq_probe(struct platform_device *pdev)
>  		return ret;
>  
>  	brcm_avs_driver.driver_data = pdev;
> -	ret = cpufreq_register_driver(&brcm_avs_driver);
> -	if (!ret)
> -		brcm_avs_cpufreq_debug_init(pdev);
>  
> -	return ret;
> +	return cpufreq_register_driver(&brcm_avs_driver);
>  }
>  
>  static int brcm_avs_cpufreq_remove(struct platform_device *pdev)
> @@ -1032,8 +713,6 @@ static int brcm_avs_cpufreq_remove(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> -	brcm_avs_cpufreq_debug_exit(pdev);
> -
>  	priv = platform_get_drvdata(pdev);
>  	iounmap(priv->base);
>  	iounmap(priv->avs_intr_base);
> 

Applied and merged into 4.17-rc3, thanks!