cpufreq: brcmstb-cpufreq: CPUfreq driver for older Broadcom STB SoCs

PATCH Superseded Markus Mayer 4 participants 8 messages Nov 22, 2016 – Dec 2, 2016

Subsystems (4)

Common clk CPU frequency scaling framework Broadcom BCM7XXX Arm architecture Broadcom stb avs cpufreq driver

lore

Share & tools

lore

Markus Mayer

author

2016-11-22 21:33 UTC #0 lore raw

PATCH v3 1/1 cpufreq: brcmstb-cpufreq: CPUfreq driver for older Broadcom STB SoCs

From: Markus Mayer <[email protected]>

This CPUfreq driver provides basic frequency scaling for older Broadcom STB SoCs that do not use AVS firmware with DVFS support. There is no support for voltage scaling.

Signed-off-by Markus Mayer <[email protected]>

Acked-by Viresh Kumar <[email protected]>

---

This patch is based on Rafael's linux-next.

Changes since v2:
- fixed naming-inconsistency for function brcmstb_cpufreq_init() (it was called brcmstb_cpu_init() before)

Changes since v1:
- removed brcmstb_cpufreq_get(), using cpufreq_generic_get() instead
- replaced calls to cpufreq_table_validate_and_show() and cpumask_setall() with a call to cpufreq_generic_init()
- removed code that would set policy->cur, leaving it up to the framework to do so
- simplified show_brcmstb_safe_freq(), re-using already existing data

 drivers/cpufreq/Kconfig.arm       |  12 ++
 drivers/cpufreq/Makefile          |   1 +
 drivers/cpufreq/brcmstb-cpufreq.c | 381 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 394 insertions(+)
 create mode 100644 drivers/cpufreq/brcmstb-cpufreq.c

drivers/cpufreq/Kconfig.arm +12 -0

index 920c469..36422af 100644
@@ -33,6 +33,18 @@ config ARM_BRCMSTB_AVS_CPUFREQ_DEBUG
 
 	  If in doubt, say N.
 
+config ARM_BRCMSTB_CPUFREQ
+	tristate "Broadcom STB CPUfreq driver"
+	depends on ARCH_BRCMSTB || COMPILE_TEST
+	default y
+	help
+	  Some Broadcom SoCs offer multiple operating frequencies that CPUfreq
+	  can take advantage of to improve energy efficiency.
+
+	  Say Y, if you have a supported Broadcom SoC. If your Broadcom SoC
+	  has AVS firmware with support for frequency and voltage scaling,
+	  say N here and enable ARM_BRCMSTB_AVS_CPUFREQ instead.
+
 config ARM_DT_BL_CPUFREQ
 	tristate "Generic probing via DT for ARM big LITTLE CPUfreq driver"
 	depends on ARM_BIG_LITTLE_CPUFREQ && OF

drivers/cpufreq/Makefile +1 -0

index 1e46c39..23700aa 100644
@@ -52,6 +52,7 @@ obj-$(CONFIG_ARM_BIG_LITTLE_CPUFREQ)	+= arm_big_little.o
 obj-$(CONFIG_ARM_DT_BL_CPUFREQ)		+= arm_big_little_dt.o
 
 obj-$(CONFIG_ARM_BRCMSTB_AVS_CPUFREQ)	+= brcmstb-avs-cpufreq.o
+obj-$(CONFIG_ARM_BRCMSTB_CPUFREQ)	+= brcmstb-cpufreq.o
 obj-$(CONFIG_ARCH_DAVINCI)		+= davinci-cpufreq.o
 obj-$(CONFIG_UX500_SOC_DB8500)		+= dbx500-cpufreq.o
 obj-$(CONFIG_ARM_EXYNOS5440_CPUFREQ)	+= exynos5440-cpufreq.o

drivers/cpufreq/brcmstb-cpufreq.c new file +381 -0

381 lines not on this page · raw message

Signature · 1 line

2.7.4

Arnd Bergmann

2016-11-28 10:14 UTC #1 lore raw

Quoted 8 lines · CET Markus Mayer

From: Markus Mayer <[email protected]>
This CPUfreq driver provides basic frequency scaling for older Broadcom STB SoCs that do not use AVS firmware with DVFS support. There is no support for voltage scaling.
Signed-off-by Markus Mayer <[email protected]>
Acked-by Viresh Kumar <[email protected]>

This causes multiple build errors in linux-next, please fix asap or drop the patch again. My feeling is that it's probably too late to fix it for v4.10, but that's up to Viresh and Rafael of course.

drivers/cpufreq/brcmstb-cpufreq.c line 34

Quoted 12 lines

drivers/cpufreq/brcmstb-cpufreq.c new file +12 -0

+#define BRCMSTB_CPUFREQ_PREFIX	"brcmstb"
+#define BRCMSTB_CPUFREQ_NAME	BRCMSTB_CPUFREQ_PREFIX "-cpufreq"
+
+/* We search for these compatible strings. */
+#define BRCMSTB_DT_CPU_CLK_CTRL	"brcm,brcmstb-cpu-clk-div"
+#define BRCMSTB_DT_MEMC_DDR	"brcm,brcmstb-memc-ddr"
+#define BRCM_AVS_CPU_DATA	"brcm,avs-cpu-data-mem"
+
+/* We also need a few clocks in device tree. These are node names. */
+#define BRCMSTB_CLK_MDIV_CH0	"cpu_mdiv_ch0"
+#define BRCMSTB_CLK_NDIV_INT	"cpu_ndiv_int"
+#define BRCMSTB_CLK_SW_SCB	"sw_scb"

Not critical but the use of those macros obfuscates the DT interfaces here and made it harder to analyse what was going on.

Also, a couple of them are lacking a DT binding.

Quoted 11 lines

drivers/cpufreq/brcmstb-cpufreq.c new file +11 -0

+static int get_frequencies(const struct cpufreq_policy *policy,
+			   unsigned int *vco_freq, unsigned int *cpu_freq,
+			   unsigned int *scb_freq)
+{
+	struct clk *cpu_ndiv_int, *sw_scb;
+
+	cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
+	if (!cpu_ndiv_int)
+		return -ENODEV;
+
+	sw_scb = __clk_lookup(BRCMSTB_CLK_SW_SCB);

drivers/cpufreq/brcmstb-cpufreq.c line 85

Quoted 10 lines

drivers/cpufreq/brcmstb-cpufreq.c new file +10 -0

+	if (!sw_scb)
+		return -ENODEV;
+
+	/* return frequencies in kHz */
+	*vco_freq = clk_get_rate(cpu_ndiv_int) / 1000;
+	*cpu_freq = clk_get_rate(policy->clk) / 1000;
+	*scb_freq = clk_get_rate(sw_scb) / 1000;
+
+	return 0;
+}

You really can't do this:

../drivers/cpufreq/brcmstb-cpufreq.c: In function 'get_frequencies':
../drivers/cpufreq/brcmstb-cpufreq.c:71:17: error: implicit declaration of function '__clk_lookup';did you mean 'key_lookup'? [-Werror=implicit-function-declaration]

  cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
                 ^~~~~~~~~~~~

__clk_lookup is an internal API for the clk providers.

In particular, relying on undocumented internal names of the clk provider in a device driver is inappropriate.

drivers/cpufreq/brcmstb-cpufreq.c line 367

Quoted 5 lines

drivers/cpufreq/brcmstb-cpufreq.c new file +5 -0

+static const struct of_device_id brcmstb_cpufreq_match[] = {
+	{ .compatible = BRCMSTB_DT_CPU_CLK_CTRL },
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, brcmstb_cpufreq_match);

This is a simple typo, also causing the build to fail:

FATAL: drivers/cpufreq/brcmstb-cpufreq: sizeof(struct platform_device_id)=24 is not a modulo of the size of section __mod_platform__<identifier>_device_table=392.

Arnd

Rafael J. Wysocki

maintainer

2016-11-28 13:29 UTC #2 lore raw

Quoted 74 lines · Arnd Bergmann

Quoted 8 lines · CET Markus Mayer
From: Markus Mayer <[email protected]>
This CPUfreq driver provides basic frequency scaling for older Broadcom STB SoCs that do not use AVS firmware with DVFS support. There is no support for voltage scaling.
Signed-off-by Markus Mayer <[email protected]>
Acked-by Viresh Kumar <[email protected]>
This causes multiple build errors in linux-next, please fix asap or drop the patch again. My feeling is that it's probably too late to fix it for v4.10, but that's up to Viresh and Rafael of course.
drivers/cpufreq/brcmstb-cpufreq.c line 34
Quoted 12 lines
drivers/cpufreq/brcmstb-cpufreq.c new file +12 -0
+#define BRCMSTB_CPUFREQ_PREFIX       "brcmstb"
+#define BRCMSTB_CPUFREQ_NAME BRCMSTB_CPUFREQ_PREFIX "-cpufreq"
+
+/* We search for these compatible strings. */
+#define BRCMSTB_DT_CPU_CLK_CTRL      "brcm,brcmstb-cpu-clk-div"
+#define BRCMSTB_DT_MEMC_DDR  "brcm,brcmstb-memc-ddr"
+#define BRCM_AVS_CPU_DATA    "brcm,avs-cpu-data-mem"
+
+/* We also need a few clocks in device tree. These are node names. */
+#define BRCMSTB_CLK_MDIV_CH0 "cpu_mdiv_ch0"
+#define BRCMSTB_CLK_NDIV_INT "cpu_ndiv_int"
+#define BRCMSTB_CLK_SW_SCB   "sw_scb"
Not critical but the use of those macros obfuscates the DT interfaces here and made it harder to analyse what was going on.
Also, a couple of them are lacking a DT binding.
Quoted 11 lines
drivers/cpufreq/brcmstb-cpufreq.c new file +11 -0
+static int get_frequencies(const struct cpufreq_policy *policy,
+                        unsigned int *vco_freq, unsigned int *cpu_freq,
+                        unsigned int *scb_freq)
+{
+     struct clk *cpu_ndiv_int, *sw_scb;
+
+     cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
+     if (!cpu_ndiv_int)
+             return -ENODEV;
+
+     sw_scb = __clk_lookup(BRCMSTB_CLK_SW_SCB);
drivers/cpufreq/brcmstb-cpufreq.c line 85
Quoted 10 lines
drivers/cpufreq/brcmstb-cpufreq.c new file +10 -0
+     if (!sw_scb)
+             return -ENODEV;
+
+     /* return frequencies in kHz */
+     *vco_freq = clk_get_rate(cpu_ndiv_int) / 1000;
+     *cpu_freq = clk_get_rate(policy->clk) / 1000;
+     *scb_freq = clk_get_rate(sw_scb) / 1000;
+
+     return 0;
+}
You really can't do this:
../drivers/cpufreq/brcmstb-cpufreq.c: In function 'get_frequencies':
../drivers/cpufreq/brcmstb-cpufreq.c:71:17: error: implicit declaration of function '__clk_lookup';did you mean 'key_lookup'? [-Werror=implicit-function-declaration]
  cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
                 ^~~~~~~~~~~~
__clk_lookup is an internal API for the clk providers.
In particular, relying on undocumented internal names of the clk provider in a device driver is inappropriate.
drivers/cpufreq/brcmstb-cpufreq.c line 367
Quoted 5 lines
drivers/cpufreq/brcmstb-cpufreq.c new file +5 -0
+static const struct of_device_id brcmstb_cpufreq_match[] = {
+     { .compatible = BRCMSTB_DT_CPU_CLK_CTRL },
+     { }
+};
+MODULE_DEVICE_TABLE(platform, brcmstb_cpufreq_match);
This is a simple typo, also causing the build to fail:

Quoted 2 lines

FATAL: drivers/cpufreq/brcmstb-cpufreq: sizeof(struct platform_device_id)=24 is not a modulo of the size of section __mod_platform__<identifier>_device_table=392.

I've dropped the patch.

Markus, please fix the problems pointed out by Arnd and resend.

Thanks,
Rafael

Markus Mayer

2016-11-28 17:12 UTC #3 lore raw

In reply to Arnd Bergmann

Quoted 58 lines · Arnd Bergmann

Quoted 8 lines · CET Markus Mayer
From: Markus Mayer <[email protected]>
This CPUfreq driver provides basic frequency scaling for older Broadcom STB SoCs that do not use AVS firmware with DVFS support. There is no support for voltage scaling.
Signed-off-by Markus Mayer <[email protected]>
Acked-by Viresh Kumar <[email protected]>
This causes multiple build errors in linux-next, please fix asap or drop the patch again. My feeling is that it's probably too late to fix it for v4.10, but that's up to Viresh and Rafael of course.
drivers/cpufreq/brcmstb-cpufreq.c line 34
Quoted 12 lines
drivers/cpufreq/brcmstb-cpufreq.c new file +12 -0
+#define BRCMSTB_CPUFREQ_PREFIX       "brcmstb"
+#define BRCMSTB_CPUFREQ_NAME BRCMSTB_CPUFREQ_PREFIX "-cpufreq"
+
+/* We search for these compatible strings. */
+#define BRCMSTB_DT_CPU_CLK_CTRL      "brcm,brcmstb-cpu-clk-div"
+#define BRCMSTB_DT_MEMC_DDR  "brcm,brcmstb-memc-ddr"
+#define BRCM_AVS_CPU_DATA    "brcm,avs-cpu-data-mem"
+
+/* We also need a few clocks in device tree. These are node names. */
+#define BRCMSTB_CLK_MDIV_CH0 "cpu_mdiv_ch0"
+#define BRCMSTB_CLK_NDIV_INT "cpu_ndiv_int"
+#define BRCMSTB_CLK_SW_SCB   "sw_scb"
Not critical but the use of those macros obfuscates the DT interfaces here and made it harder to analyse what was going on.
Also, a couple of them are lacking a DT binding.
Quoted 11 lines
drivers/cpufreq/brcmstb-cpufreq.c new file +11 -0
+static int get_frequencies(const struct cpufreq_policy *policy,
+                        unsigned int *vco_freq, unsigned int *cpu_freq,
+                        unsigned int *scb_freq)
+{
+     struct clk *cpu_ndiv_int, *sw_scb;
+
+     cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
+     if (!cpu_ndiv_int)
+             return -ENODEV;
+
+     sw_scb = __clk_lookup(BRCMSTB_CLK_SW_SCB);
drivers/cpufreq/brcmstb-cpufreq.c line 85
Quoted 10 lines
drivers/cpufreq/brcmstb-cpufreq.c new file +10 -0
+     if (!sw_scb)
+             return -ENODEV;
+
+     /* return frequencies in kHz */
+     *vco_freq = clk_get_rate(cpu_ndiv_int) / 1000;
+     *cpu_freq = clk_get_rate(policy->clk) / 1000;
+     *scb_freq = clk_get_rate(sw_scb) / 1000;
+
+     return 0;
+}
You really can't do this:
../drivers/cpufreq/brcmstb-cpufreq.c: In function 'get_frequencies':
../drivers/cpufreq/brcmstb-cpufreq.c:71:17: error: implicit declaration of function '__clk_lookup';did you mean 'key_lookup'? [-Werror=implicit-function-declaration]

Quoted 7 lines

  cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
                 ^~~~~~~~~~~~
__clk_lookup is an internal API for the clk providers.
In particular, relying on undocumented internal names of the clk provider in a device driver is inappropriate.

What compiler are you using? I didn't get any warnings. Otherwise I would have known right away that something isn't right.

Quoted 9 lines

drivers/cpufreq/brcmstb-cpufreq.c line 367
Quoted 5 lines
drivers/cpufreq/brcmstb-cpufreq.c new file +5 -0
+static const struct of_device_id brcmstb_cpufreq_match[] = {
+     { .compatible = BRCMSTB_DT_CPU_CLK_CTRL },
+     { }
+};
+MODULE_DEVICE_TABLE(platform, brcmstb_cpufreq_match);
This is a simple typo, also causing the build to fail:
FATAL: drivers/cpufreq/brcmstb-cpufreq: sizeof(struct platform_device_id)=24 is not a modulo of the size of section __mod_platform__<identifier>_device_table=392.

What is the typo, if I may ask. Again strange, since the build doesn't fail for me. What was the configuration you used?

Quoted 1 line

Arnd

Arnd Bergmann

2016-11-28 20:59 UTC #4 lore raw

Quoted 29 lines · CET Markus Mayer

Quoted 29 lines

Quoted 11 lines

drivers/cpufreq/brcmstb-cpufreq.c new file +11 -0

+static int get_frequencies(const struct cpufreq_policy *policy,
+                        unsigned int *vco_freq, unsigned int *cpu_freq,
+                        unsigned int *scb_freq)
+{
+     struct clk *cpu_ndiv_int, *sw_scb;
+
+     cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
+     if (!cpu_ndiv_int)
+             return -ENODEV;
+
+     sw_scb = __clk_lookup(BRCMSTB_CLK_SW_SCB);

drivers/cpufreq/brcmstb-cpufreq.c line 85

Quoted 10 lines

drivers/cpufreq/brcmstb-cpufreq.c new file +10 -0

+     if (!sw_scb)
+             return -ENODEV;
+
+     /* return frequencies in kHz */
+     *vco_freq = clk_get_rate(cpu_ndiv_int) / 1000;
+     *cpu_freq = clk_get_rate(policy->clk) / 1000;
+     *scb_freq = clk_get_rate(sw_scb) / 1000;
+
+     return 0;
+}

You really can't do this:

  cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
                 ^~~~~~~~~~~~

Quoted 7 lines

Quoted 4 lines
__clk_lookup is an internal API for the clk providers.
In particular, relying on undocumented internal names of the clk provider in a device driver is inappropriate.
What compiler are you using? I didn't get any warnings. Otherwise I would have known right away that something isn't right.

This is a randconfig build with CONFIG_COMMON_CLK=n. There is a different problem with COMMON_CLK=y and the cpufreq driver as a loadable module, where the symbol causes a link error.

I did not get any warnings either, these are both hard errors.

Quoted 8 lines

Quoted 8 lines
drivers/cpufreq/brcmstb-cpufreq.c line 367
Quoted 5 lines
drivers/cpufreq/brcmstb-cpufreq.c new file +5 -0
+static const struct of_device_id brcmstb_cpufreq_match[] = {
+     { .compatible = BRCMSTB_DT_CPU_CLK_CTRL },
+     { }
+};
+MODULE_DEVICE_TABLE(platform, brcmstb_cpufreq_match);
This is a simple typo, also causing the build to fail:

Quoted 4 lines

Quoted 1 line
FATAL: drivers/cpufreq/brcmstb-cpufreq: sizeof(struct platform_device_id)=24 is not a modulo of the size of section __mod_platform__<identifier>_device_table=392.
What is the typo, if I may ask. Again strange, since the build doesn't fail for me. What was the configuration you used?

MODULE_DEVICE_TABLE() is only used when building a loadable module, e.g. in allmodconfig.

MODULE_DEVICE_TABLE(platform, ...) is for 'struct platform_device_id'. You need to use MODULE_DEVICE_TABLE(of, ...) for 'struct of_device_id'.

Arnd

Markus Mayer

2016-11-28 21:07 UTC #5 lore raw

Quoted 54 lines · Arnd Bergmann

Quoted 29 lines · CET Markus Mayer
Quoted 29 lines
Quoted 11 lines
drivers/cpufreq/brcmstb-cpufreq.c new file +11 -0
+static int get_frequencies(const struct cpufreq_policy *policy,
+                        unsigned int *vco_freq, unsigned int *cpu_freq,
+                        unsigned int *scb_freq)
+{
+     struct clk *cpu_ndiv_int, *sw_scb;
+
+     cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
+     if (!cpu_ndiv_int)
+             return -ENODEV;
+
+     sw_scb = __clk_lookup(BRCMSTB_CLK_SW_SCB);
drivers/cpufreq/brcmstb-cpufreq.c line 85
Quoted 10 lines
drivers/cpufreq/brcmstb-cpufreq.c new file +10 -0
+     if (!sw_scb)
+             return -ENODEV;
+
+     /* return frequencies in kHz */
+     *vco_freq = clk_get_rate(cpu_ndiv_int) / 1000;
+     *cpu_freq = clk_get_rate(policy->clk) / 1000;
+     *scb_freq = clk_get_rate(sw_scb) / 1000;
+
+     return 0;
+}
You really can't do this:
../drivers/cpufreq/brcmstb-cpufreq.c: In function 'get_frequencies':
../drivers/cpufreq/brcmstb-cpufreq.c:71:17: error: implicit declaration of function '__clk_lookup';did you mean 'key_lookup'? [-Werror=implicit-function-declaration]
  cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
                 ^~~~~~~~~~~~
Quoted 7 lines
Quoted 4 lines
__clk_lookup is an internal API for the clk providers.
In particular, relying on undocumented internal names of the clk provider in a device driver is inappropriate.
What compiler are you using? I didn't get any warnings. Otherwise I would have known right away that something isn't right.
This is a randconfig build with CONFIG_COMMON_CLK=n. There is a different problem with COMMON_CLK=y and the cpufreq driver as a loadable module, where the symbol causes a link error.
I did not get any warnings either, these are both hard errors.
Quoted 10 lines
Quoted 9 lines
drivers/cpufreq/brcmstb-cpufreq.c line 367
Quoted 5 lines
drivers/cpufreq/brcmstb-cpufreq.c new file +5 -0
+static const struct of_device_id brcmstb_cpufreq_match[] = {
+     { .compatible = BRCMSTB_DT_CPU_CLK_CTRL },
+     { }
+};
+MODULE_DEVICE_TABLE(platform, brcmstb_cpufreq_match);
This is a simple typo, also causing the build to fail:
FATAL: drivers/cpufreq/brcmstb-cpufreq: sizeof(struct platform_device_id)=24 is not a modulo of the size of section __mod_platform__<identifier>_device_table=392.

Quoted 8 lines

Quoted 2 lines
What is the typo, if I may ask. Again strange, since the build doesn't fail for me. What was the configuration you used?
MODULE_DEVICE_TABLE() is only used when building a loadable module, e.g. in allmodconfig.
MODULE_DEVICE_TABLE(platform, ...) is for 'struct platform_device_id'. You need to use MODULE_DEVICE_TABLE(of, ...) for 'struct of_device_id'.

Got it. Thanks.

I fixed this problem and the IS_ENABLED(CONFIG_ARM_BRCMSTB_AVS_CPUFREQ) issue in my copy. I'll need to look a bit more what I can do instead of calling __clk_lookup().

Regards,
-Markus

Markus Mayer

2016-12-02 00:50 UTC #6 lore raw

In reply to Arnd Bergmann

Quoted 58 lines · Arnd Bergmann

Quoted 8 lines · CET Markus Mayer
From: Markus Mayer <[email protected]>
This CPUfreq driver provides basic frequency scaling for older Broadcom STB SoCs that do not use AVS firmware with DVFS support. There is no support for voltage scaling.
Signed-off-by Markus Mayer <[email protected]>
Acked-by Viresh Kumar <[email protected]>
This causes multiple build errors in linux-next, please fix asap or drop the patch again. My feeling is that it's probably too late to fix it for v4.10, but that's up to Viresh and Rafael of course.
drivers/cpufreq/brcmstb-cpufreq.c line 34
Quoted 12 lines
drivers/cpufreq/brcmstb-cpufreq.c new file +12 -0
+#define BRCMSTB_CPUFREQ_PREFIX       "brcmstb"
+#define BRCMSTB_CPUFREQ_NAME BRCMSTB_CPUFREQ_PREFIX "-cpufreq"
+
+/* We search for these compatible strings. */
+#define BRCMSTB_DT_CPU_CLK_CTRL      "brcm,brcmstb-cpu-clk-div"
+#define BRCMSTB_DT_MEMC_DDR  "brcm,brcmstb-memc-ddr"
+#define BRCM_AVS_CPU_DATA    "brcm,avs-cpu-data-mem"
+
+/* We also need a few clocks in device tree. These are node names. */
+#define BRCMSTB_CLK_MDIV_CH0 "cpu_mdiv_ch0"
+#define BRCMSTB_CLK_NDIV_INT "cpu_ndiv_int"
+#define BRCMSTB_CLK_SW_SCB   "sw_scb"
Not critical but the use of those macros obfuscates the DT interfaces here and made it harder to analyse what was going on.
Also, a couple of them are lacking a DT binding.
Quoted 11 lines
drivers/cpufreq/brcmstb-cpufreq.c new file +11 -0
+static int get_frequencies(const struct cpufreq_policy *policy,
+                        unsigned int *vco_freq, unsigned int *cpu_freq,
+                        unsigned int *scb_freq)
+{
+     struct clk *cpu_ndiv_int, *sw_scb;
+
+     cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
+     if (!cpu_ndiv_int)
+             return -ENODEV;
+
+     sw_scb = __clk_lookup(BRCMSTB_CLK_SW_SCB);
drivers/cpufreq/brcmstb-cpufreq.c line 85
Quoted 10 lines
drivers/cpufreq/brcmstb-cpufreq.c new file +10 -0
+     if (!sw_scb)
+             return -ENODEV;
+
+     /* return frequencies in kHz */
+     *vco_freq = clk_get_rate(cpu_ndiv_int) / 1000;
+     *cpu_freq = clk_get_rate(policy->clk) / 1000;
+     *scb_freq = clk_get_rate(sw_scb) / 1000;
+
+     return 0;
+}
You really can't do this:
../drivers/cpufreq/brcmstb-cpufreq.c: In function 'get_frequencies':
../drivers/cpufreq/brcmstb-cpufreq.c:71:17: error: implicit declaration of function '__clk_lookup';did you mean 'key_lookup'? [-Werror=implicit-function-declaration]

Quoted 7 lines

  cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
                 ^~~~~~~~~~~~
__clk_lookup is an internal API for the clk providers.
In particular, relying on undocumented internal names of the clk provider in a device driver is inappropriate.

Do you happen to know of an "approved" way of looking up a clock node? Everything we need is in device tree. We can certainly add bindings for the missing nodes. It just seems somewhat difficult to get at the information in a clean way.

Quoted 11 lines

drivers/cpufreq/brcmstb-cpufreq.c line 367
Quoted 5 lines
drivers/cpufreq/brcmstb-cpufreq.c new file +5 -0
+static const struct of_device_id brcmstb_cpufreq_match[] = {
+     { .compatible = BRCMSTB_DT_CPU_CLK_CTRL },
+     { }
+};
+MODULE_DEVICE_TABLE(platform, brcmstb_cpufreq_match);
This is a simple typo, also causing the build to fail:
FATAL: drivers/cpufreq/brcmstb-cpufreq: sizeof(struct platform_device_id)=24 is not a modulo of the size of section __mod_platform__<identifier>_device_table=392.
Arnd

Arnd Bergmann

2016-11-28 10:16 UTC #7 lore raw

fwd, Re: [PATCH v3] cpufreq: brcmstb-cpufreq: CPUfreq driver for older Broadcom STB SoCs

In reply to Markus Mayer

[resending my mail, this time with devicetree, linux-clk, and linux-arm-kernel on cc]

Quoted 8 lines · CET Markus Mayer

From: Markus Mayer <[email protected]>
This CPUfreq driver provides basic frequency scaling for older Broadcom STB SoCs that do not use AVS firmware with DVFS support. There is no support for voltage scaling.
Signed-off-by Markus Mayer <[email protected]>
Acked-by Viresh Kumar <[email protected]>

This causes multiple build errors in linux-next, please fix asap or drop the patch again. My feeling is that it's probably too late to fix it for v4.10, but that's up to Viresh and Rafael of course.

drivers/cpufreq/brcmstb-cpufreq.c line 34

Quoted 12 lines

drivers/cpufreq/brcmstb-cpufreq.c new file +12 -0

+#define BRCMSTB_CPUFREQ_PREFIX	"brcmstb"
+#define BRCMSTB_CPUFREQ_NAME	BRCMSTB_CPUFREQ_PREFIX "-cpufreq"
+
+/* We search for these compatible strings. */
+#define BRCMSTB_DT_CPU_CLK_CTRL	"brcm,brcmstb-cpu-clk-div"
+#define BRCMSTB_DT_MEMC_DDR	"brcm,brcmstb-memc-ddr"
+#define BRCM_AVS_CPU_DATA	"brcm,avs-cpu-data-mem"
+
+/* We also need a few clocks in device tree. These are node names. */
+#define BRCMSTB_CLK_MDIV_CH0	"cpu_mdiv_ch0"
+#define BRCMSTB_CLK_NDIV_INT	"cpu_ndiv_int"
+#define BRCMSTB_CLK_SW_SCB	"sw_scb"

Not critical but the use of those macros obfuscates the DT interfaces here and made it harder to analyse what was going on.

Also, a couple of them are lacking a DT binding.

Quoted 11 lines

drivers/cpufreq/brcmstb-cpufreq.c new file +11 -0

+static int get_frequencies(const struct cpufreq_policy *policy,
+			   unsigned int *vco_freq, unsigned int *cpu_freq,
+			   unsigned int *scb_freq)
+{
+	struct clk *cpu_ndiv_int, *sw_scb;
+
+	cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
+	if (!cpu_ndiv_int)
+		return -ENODEV;
+
+	sw_scb = __clk_lookup(BRCMSTB_CLK_SW_SCB);

drivers/cpufreq/brcmstb-cpufreq.c line 85

Quoted 10 lines

drivers/cpufreq/brcmstb-cpufreq.c new file +10 -0

+	if (!sw_scb)
+		return -ENODEV;
+
+	/* return frequencies in kHz */
+	*vco_freq = clk_get_rate(cpu_ndiv_int) / 1000;
+	*cpu_freq = clk_get_rate(policy->clk) / 1000;
+	*scb_freq = clk_get_rate(sw_scb) / 1000;
+
+	return 0;
+}

You really can't do this:

  cpu_ndiv_int = __clk_lookup(BRCMSTB_CLK_NDIV_INT);
                 ^~~~~~~~~~~~

__clk_lookup is an internal API for the clk providers.

In particular, relying on undocumented internal names of the clk provider in a device driver is inappropriate.

drivers/cpufreq/brcmstb-cpufreq.c line 367

Quoted 5 lines

drivers/cpufreq/brcmstb-cpufreq.c new file +5 -0

+static const struct of_device_id brcmstb_cpufreq_match[] = {
+	{ .compatible = BRCMSTB_DT_CPU_CLK_CTRL },
+	{ }
+};
+MODULE_DEVICE_TABLE(platform, brcmstb_cpufreq_match);

This is a simple typo, also causing the build to fail:

FATAL: drivers/cpufreq/brcmstb-cpufreq: sizeof(struct platform_device_id)=24 is not a modulo of the size of section __mod_platform__<identifier>_device_table=392.

Arnd