The initial idea of creating the cpufreq-dt-platdev.c file was to keep a
list of platforms that use the "operating-points" (V1) bindings and
create cpufreq device for them only, as we weren't sure which platforms
would want the device to get created automatically as some had their own
cpufreq drivers as well, or wanted to initialize cpufreq after doing
some stuff from platform code.
But that wasn't the case with platforms using "operating-points-v2"
property. We wanted the device to get created automatically without the
need of adding them to the whitelist. Though, we will still have some
exceptions where we don't want to create the device automatically.
Rename the earlier platform list as *whitelist* and create a new
*blacklist* as well.
The cpufreq-dt device will get created if:
- The platform is there in the whitelist OR
- The platform has "operating-points-v2" property in CPU0's DT node and
isn't part of the blacklist .
Reported-by: Geert Uytterhoeven <[email protected]>
Signed-off-by: Viresh Kumar <[email protected]>
I have exercised this on the r8a7795 and r8a7795 with the following reverted:
* 034def597bb7 ("cpufreq: rcar: Add support for R8A7795 SoC")
* bea2ebca6b91 ("cpufreq: dt: Add r8a7796 support to to use generic cpufreq driver")
Tested-by: Simon Horman <[email protected]>
---
drivers/cpufreq/cpufreq-dt-platdev.c | 45 ++++++++++++++++++++++++++++++++----
1 file changed, 40 insertions(+), 5 deletions(-)
diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
index bcee384b3251..061b468512a2 100644
--- a/drivers/cpufreq/cpufreq-dt-platdev.c
+++ b/drivers/cpufreq/cpufreq-dt-platdev.c
@@ -9,11 +9,16 @@
#include <linux/err.h>
#include <linux/of.h>
+#include <linux/of_device.h>
#include <linux/platform_device.h>
#include "cpufreq-dt.h"
-static const struct of_device_id machines[] __initconst = {
+/*
+ * Machines for which the cpufreq device is *always* created, mostly used for
+ * platforms using "operating-points" (V1) property.
+ */
+static const struct of_device_id whitelist[] __initconst = {
{ .compatible = "allwinner,sun4i-a10", },
{ .compatible = "allwinner,sun5i-a10s", },
{ .compatible = "allwinner,sun5i-a13", },
@@ -101,21 +106,51 @@ static const struct of_device_id machines[] __initconst = {
{ }
};
+/*
+ * Machines for which the cpufreq device is *not* created, mostly used for
+ * platforms using "operating-points-v2" property.
+ */
+static const struct of_device_id blacklist[] __initconst = {
+ { }
+};
+
+static bool __init cpu0_node_has_opp_v2_prop(void)
+{
+ struct device_node *np = of_cpu_device_node_get(0);
+ bool ret = false;
+
+ if (of_get_property(np, "operating-points-v2", NULL))
+ ret = true;
+
+ of_node_put(np);
+ return ret;
+}
+
static int __init cpufreq_dt_platdev_init(void)
{
struct device_node *np = of_find_node_by_path("/");
const struct of_device_id *match;
+ const void *data = NULL;
if (!np)
return -ENODEV;
- match = of_match_node(machines, np);
+ match = of_match_node(whitelist, np);
+ if (match) {
+ data = match->data;
+ goto create_pdev;
+ }
+
+ if (cpu0_node_has_opp_v2_prop() && !of_match_node(blacklist, np))
+ goto create_pdev;
+
of_node_put(np);
- if (!match)
- return -ENODEV;
+ return -ENODEV;
+create_pdev:
+ of_node_put(np);
return PTR_ERR_OR_ZERO(platform_device_register_data(NULL, "cpufreq-dt",
- -1, match->data,
+ -1, data,
sizeof(struct cpufreq_dt_platform_data)));
}
device_initcall(cpufreq_dt_platdev_init);
On Mon, Aug 21, 2017 at 03:13:32PM +0200, Viresh Kumar wrote:
> The initial idea of creating the cpufreq-dt-platdev.c file was to keep a
> list of platforms that use the "operating-points" (V1) bindings and
> create cpufreq device for them only, as we weren't sure which platforms
> would want the device to get created automatically as some had their own
> cpufreq drivers as well, or wanted to initialize cpufreq after doing
> some stuff from platform code.
>
> But that wasn't the case with platforms using "operating-points-v2"
> property. We wanted the device to get created automatically without the
> need of adding them to the whitelist. Though, we will still have some
> exceptions where we don't want to create the device automatically.
>
> Rename the earlier platform list as *whitelist* and create a new
> *blacklist* as well.
>
> The cpufreq-dt device will get created if:
> - The platform is there in the whitelist OR
> - The platform has "operating-points-v2" property in CPU0's DT node and
> isn't part of the blacklist .
>
> Reported-by: Geert Uytterhoeven <[email protected]>
> Signed-off-by: Viresh Kumar <[email protected]>
>
> I have exercised this on the r8a7795 and r8a7795 with the following reverted:
>
> * 034def597bb7 ("cpufreq: rcar: Add support for R8A7795 SoC")
> * bea2ebca6b91 ("cpufreq: dt: Add r8a7796 support to to use generic cpufreq driver")
>
> Tested-by: Simon Horman <[email protected]>
Sorry, I seem to have accidently sent this email as Virish rather than
myself. I will try again.
>
>
> ---
> drivers/cpufreq/cpufreq-dt-platdev.c | 45 ++++++++++++++++++++++++++++++++----
> 1 file changed, 40 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
> index bcee384b3251..061b468512a2 100644
> --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> @@ -9,11 +9,16 @@
>
> #include <linux/err.h>
> #include <linux/of.h>
> +#include <linux/of_device.h>
> #include <linux/platform_device.h>
>
> #include "cpufreq-dt.h"
>
> -static const struct of_device_id machines[] __initconst = {
> +/*
> + * Machines for which the cpufreq device is *always* created, mostly used for
> + * platforms using "operating-points" (V1) property.
> + */
> +static const struct of_device_id whitelist[] __initconst = {
> { .compatible = "allwinner,sun4i-a10", },
> { .compatible = "allwinner,sun5i-a10s", },
> { .compatible = "allwinner,sun5i-a13", },
> @@ -101,21 +106,51 @@ static const struct of_device_id machines[] __initconst = {
> { }
> };
>
> +/*
> + * Machines for which the cpufreq device is *not* created, mostly used for
> + * platforms using "operating-points-v2" property.
> + */
> +static const struct of_device_id blacklist[] __initconst = {
> + { }
> +};
> +
> +static bool __init cpu0_node_has_opp_v2_prop(void)
> +{
> + struct device_node *np = of_cpu_device_node_get(0);
> + bool ret = false;
> +
> + if (of_get_property(np, "operating-points-v2", NULL))
> + ret = true;
> +
> + of_node_put(np);
> + return ret;
> +}
> +
> static int __init cpufreq_dt_platdev_init(void)
> {
> struct device_node *np = of_find_node_by_path("/");
> const struct of_device_id *match;
> + const void *data = NULL;
>
> if (!np)
> return -ENODEV;
>
> - match = of_match_node(machines, np);
> + match = of_match_node(whitelist, np);
> + if (match) {
> + data = match->data;
> + goto create_pdev;
> + }
> +
> + if (cpu0_node_has_opp_v2_prop() && !of_match_node(blacklist, np))
> + goto create_pdev;
> +
> of_node_put(np);
> - if (!match)
> - return -ENODEV;
> + return -ENODEV;
>
> +create_pdev:
> + of_node_put(np);
> return PTR_ERR_OR_ZERO(platform_device_register_data(NULL, "cpufreq-dt",
> - -1, match->data,
> + -1, data,
> sizeof(struct cpufreq_dt_platform_data)));
> }
> device_initcall(cpufreq_dt_platdev_init);
On 21-08-17, 15:17, Simon Horman wrote:
> Sorry, I seem to have accidently sent this email as Virish rather than
> myself. I will try again.
No issues. I see that Rafael has already applied my patches, you can send
additional patches over that to make sure everything is clear.
--
viresh
On Tuesday 29 August 2017 03:41 PM, Viresh Kumar wrote:
> On 21-08-17, 15:17, Simon Horman wrote:
>> Sorry, I seem to have accidently sent this email as Virish rather than
>> myself. I will try again.
>
> No issues. I see that Rafael has already applied my patches, you can send
> additional patches over that to make sure everything is clear.
Viresh,
I see below warning with this patch on am57xx-beagle
[ 3.545247] WARNING: CPU: 1 PID: 1 at fs/sysfs/dir.c:31
sysfs_warn_dup+0x58/0x78
[ 3.553099] sysfs: cannot create duplicate filename
'/devices/platform/cpufreq-dt'
[ 3.561126] Modules linked in:
[ 3.564402] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G W
4.13.0-next-20170911-12690-ga31cc45-dirty #7
[ 3.575071] Hardware name: Generic DRA74X (Flattened Device Tree)
[ 3.581484] [<c0110a88>] (unwind_backtrace) from [<c010cae4>]
(show_stack+0x10/0x14)
[ 3.589620] [<c010cae4>] (show_stack) from [<c0828184>]
(dump_stack+0xac/0xe0)
[ 3.597209] [<c0828184>] (dump_stack) from [<c01374d0>]
(__warn+0xd8/0x104)
[ 3.604532] [<c01374d0>] (__warn) from [<c0137530>]
(warn_slowpath_fmt+0x34/0x44)
[ 3.612390] [<c0137530>] (warn_slowpath_fmt) from [<c0345b30>]
(sysfs_warn_dup+0x58/0x78)
[ 3.620988] [<c0345b30>] (sysfs_warn_dup) from [<c0345c18>]
(sysfs_create_dir_ns+0x80/0x98)
[ 3.629773] [<c0345c18>] (sysfs_create_dir_ns) from [<c082c974>]
(kobject_add_internal+0x9c/0x2d4)
[ 3.639177] [<c082c974>] (kobject_add_internal) from [<c082cbf8>]
(kobject_add+0x4c/0x9c)
[ 3.647764] [<c082cbf8>] (kobject_add) from [<c05805a8>]
(device_add+0xcc/0x57c)
[ 3.655542] [<c05805a8>] (device_add) from [<c0584d38>]
(platform_device_add+0x100/0x220)
[ 3.664138] [<c0584d38>] (platform_device_add) from [<c058576c>]
(platform_device_register_full+0xf4/0x118)
[ 3.674378] [<c058576c>] (platform_device_register_full) from
[<c0670514>] (ti_cpufreq_init+0x150/0x22c)
[ 3.684326] [<c0670514>] (ti_cpufreq_init) from [<c0101df4>]
(do_one_initcall+0x3c/0x170)
[ 3.692916] [<c0101df4>] (do_one_initcall) from [<c0c00eb4>]
(kernel_init_freeable+0x1fc/0x2c4)
[ 3.702049] [<c0c00eb4>] (kernel_init_freeable) from [<c083c0cc>]
(kernel_init+0x8/0x110)
[ 3.710639] [<c083c0cc>] (kernel_init) from [<c0107d78>]
(ret_from_fork+0x14/0x3c)
[ 3.718680] ---[ end trace 33482508cbb50156 ]---
[ 3.720850] ata1: SATA link down (SStatus 0 SControl 300)
[ 3.729278] ------------[ cut here ]------------
[ 3.734147] WARNING: CPU: 1 PID: 1 at lib/kobject.c:240
kobject_add_internal+0x254/0x2d4
[ 3.742709] kobject_add_internal failed for cpufreq-dt with -EEXIST,
don't try to register things with the same name in the same directory.
[ 3.755900] Modules linked in:
[ 3.759239] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G W
4.13.0-next-20170911-12690-ga31cc45-dirty #7
[ 3.769907] Hardware name: Generic DRA74X (Flattened Device Tree)
[ 3.776321] [<c0110a88>] (unwind_backtrace) from [<c010cae4>]
(show_stack+0x10/0x14)
[ 3.784454] [<c010cae4>] (show_stack) from [<c0828184>]
(dump_stack+0xac/0xe0)
[ 3.792043] [<c0828184>] (dump_stack) from [<c01374d0>]
(__warn+0xd8/0x104)
[ 3.799364] [<c01374d0>] (__warn) from [<c0137530>]
(warn_slowpath_fmt+0x34/0x44)
[ 3.807227] [<c0137530>] (warn_slowpath_fmt) from [<c082cb2c>]
(kobject_add_internal+0x254/0x2d4)
[ 3.816552] [<c082cb2c>] (kobject_add_internal) from [<c082cbf8>]
(kobject_add+0x4c/0x9c)
[ 3.825139] [<c082cbf8>] (kobject_add) from [<c05805a8>]
(device_add+0xcc/0x57c)
[ 3.832913] [<c05805a8>] (device_add) from [<c0584d38>]
(platform_device_add+0x100/0x220)
[ 3.841504] [<c0584d38>] (platform_device_add) from [<c058576c>]
(platform_device_register_full+0xf4/0x118)
[ 3.851729] [<c058576c>] (platform_device_register_full) from
[<c0670514>] (ti_cpufreq_init+0x150/0x22c)
[ 3.861686] [<c0670514>] (ti_cpufreq_init) from [<c0101df4>]
(do_one_initcall+0x3c/0x170)
[ 3.870273] [<c0101df4>] (do_one_initcall) from [<c0c00eb4>]
(kernel_init_freeable+0x1fc/0x2c4)
[ 3.879412] [<c0c00eb4>] (kernel_init_freeable) from [<c083c0cc>]
(kernel_init+0x8/0x110)
[ 3.888002] [<c083c0cc>] (kernel_init) from [<c0107d78>]
(ret_from_fork+0x14/0x3c)
[ 3.896035] ---[ end trace 33482508cbb50157 ]---
I believe in two places platform_device_register_* calls are made with
the same name 'cpufreq-dt'
First place is: cpufreq_dt_platdev_init and the other i believe is
ti_cpufreq_init.
Dave,
Should the ti-cpufreq driver also be calling
platform_device_register_simple?
Regards,
Keerthy
>
Hi,
On 09/11/2017 07:13 AM, Keerthy wrote:
>
>
> On Tuesday 29 August 2017 03:41 PM, Viresh Kumar wrote:
>> On 21-08-17, 15:17, Simon Horman wrote:
>>> Sorry, I seem to have accidently sent this email as Virish rather than
>>> myself. I will try again.
>>
>> No issues. I see that Rafael has already applied my patches, you can send
>> additional patches over that to make sure everything is clear.
>
> Viresh,
>
> I see below warning with this patch on am57xx-beagle
>
> [ 3.545247] WARNING: CPU: 1 PID: 1 at fs/sysfs/dir.c:31
> sysfs_warn_dup+0x58/0x78
> [ 3.553099] sysfs: cannot create duplicate filename
> '/devices/platform/cpufreq-dt'
> [ 3.561126] Modules linked in:
> [ 3.564402] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G W
> 4.13.0-next-20170911-12690-ga31cc45-dirty #7
> [ 3.575071] Hardware name: Generic DRA74X (Flattened Device Tree)
> [ 3.581484] [<c0110a88>] (unwind_backtrace) from [<c010cae4>]
> (show_stack+0x10/0x14)
> [ 3.589620] [<c010cae4>] (show_stack) from [<c0828184>]
> (dump_stack+0xac/0xe0)
> [ 3.597209] [<c0828184>] (dump_stack) from [<c01374d0>]
> (__warn+0xd8/0x104)
> [ 3.604532] [<c01374d0>] (__warn) from [<c0137530>]
> (warn_slowpath_fmt+0x34/0x44)
> [ 3.612390] [<c0137530>] (warn_slowpath_fmt) from [<c0345b30>]
> (sysfs_warn_dup+0x58/0x78)
> [ 3.620988] [<c0345b30>] (sysfs_warn_dup) from [<c0345c18>]
> (sysfs_create_dir_ns+0x80/0x98)
> [ 3.629773] [<c0345c18>] (sysfs_create_dir_ns) from [<c082c974>]
> (kobject_add_internal+0x9c/0x2d4)
> [ 3.639177] [<c082c974>] (kobject_add_internal) from [<c082cbf8>]
> (kobject_add+0x4c/0x9c)
> [ 3.647764] [<c082cbf8>] (kobject_add) from [<c05805a8>]
> (device_add+0xcc/0x57c)
> [ 3.655542] [<c05805a8>] (device_add) from [<c0584d38>]
> (platform_device_add+0x100/0x220)
> [ 3.664138] [<c0584d38>] (platform_device_add) from [<c058576c>]
> (platform_device_register_full+0xf4/0x118)
> [ 3.674378] [<c058576c>] (platform_device_register_full) from
> [<c0670514>] (ti_cpufreq_init+0x150/0x22c)
> [ 3.684326] [<c0670514>] (ti_cpufreq_init) from [<c0101df4>]
> (do_one_initcall+0x3c/0x170)
> [ 3.692916] [<c0101df4>] (do_one_initcall) from [<c0c00eb4>]
> (kernel_init_freeable+0x1fc/0x2c4)
> [ 3.702049] [<c0c00eb4>] (kernel_init_freeable) from [<c083c0cc>]
> (kernel_init+0x8/0x110)
> [ 3.710639] [<c083c0cc>] (kernel_init) from [<c0107d78>]
> (ret_from_fork+0x14/0x3c)
> [ 3.718680] ---[ end trace 33482508cbb50156 ]---
> [ 3.720850] ata1: SATA link down (SStatus 0 SControl 300)
> [ 3.729278] ------------[ cut here ]------------
> [ 3.734147] WARNING: CPU: 1 PID: 1 at lib/kobject.c:240
> kobject_add_internal+0x254/0x2d4
> [ 3.742709] kobject_add_internal failed for cpufreq-dt with -EEXIST,
> don't try to register things with the same name in the same directory.
> [ 3.755900] Modules linked in:
> [ 3.759239] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G W
> 4.13.0-next-20170911-12690-ga31cc45-dirty #7
> [ 3.769907] Hardware name: Generic DRA74X (Flattened Device Tree)
> [ 3.776321] [<c0110a88>] (unwind_backtrace) from [<c010cae4>]
> (show_stack+0x10/0x14)
> [ 3.784454] [<c010cae4>] (show_stack) from [<c0828184>]
> (dump_stack+0xac/0xe0)
> [ 3.792043] [<c0828184>] (dump_stack) from [<c01374d0>]
> (__warn+0xd8/0x104)
> [ 3.799364] [<c01374d0>] (__warn) from [<c0137530>]
> (warn_slowpath_fmt+0x34/0x44)
> [ 3.807227] [<c0137530>] (warn_slowpath_fmt) from [<c082cb2c>]
> (kobject_add_internal+0x254/0x2d4)
> [ 3.816552] [<c082cb2c>] (kobject_add_internal) from [<c082cbf8>]
> (kobject_add+0x4c/0x9c)
> [ 3.825139] [<c082cbf8>] (kobject_add) from [<c05805a8>]
> (device_add+0xcc/0x57c)
> [ 3.832913] [<c05805a8>] (device_add) from [<c0584d38>]
> (platform_device_add+0x100/0x220)
> [ 3.841504] [<c0584d38>] (platform_device_add) from [<c058576c>]
> (platform_device_register_full+0xf4/0x118)
> [ 3.851729] [<c058576c>] (platform_device_register_full) from
> [<c0670514>] (ti_cpufreq_init+0x150/0x22c)
> [ 3.861686] [<c0670514>] (ti_cpufreq_init) from [<c0101df4>]
> (do_one_initcall+0x3c/0x170)
> [ 3.870273] [<c0101df4>] (do_one_initcall) from [<c0c00eb4>]
> (kernel_init_freeable+0x1fc/0x2c4)
> [ 3.879412] [<c0c00eb4>] (kernel_init_freeable) from [<c083c0cc>]
> (kernel_init+0x8/0x110)
> [ 3.888002] [<c083c0cc>] (kernel_init) from [<c0107d78>]
> (ret_from_fork+0x14/0x3c)
> [ 3.896035] ---[ end trace 33482508cbb50157 ]---
>
> I believe in two places platform_device_register_* calls are made with
> the same name 'cpufreq-dt'
>
> First place is: cpufreq_dt_platdev_init and the other i believe is
> ti_cpufreq_init.
>
> Dave,
>
> Should the ti-cpufreq driver also be calling
> platform_device_register_simple?
Yes, ti-cpufreq registers the cpufreq-dt platdev in order to ensure the
ti-cpufreq driver probes first and provides the necessary opp-supported-hw for
cpufreq-dt. This applies to am335x, am437x, dra7xx, and am57xx. I suppose these
platforms will need to be added to the 'blacklist' to prevent the cpufreq-dt
platdev from being created automatically, unless there is a better way to handle
this dependency...
Regards,
Dave
>
> Regards,
> Keerthy
>
>>
On 11-09-17, 09:18, Dave Gerlach wrote:
> Yes, ti-cpufreq registers the cpufreq-dt platdev in order to ensure the
> ti-cpufreq driver probes first and provides the necessary opp-supported-hw for
> cpufreq-dt. This applies to am335x, am437x, dra7xx, and am57xx. I suppose these
> platforms will need to be added to the 'blacklist' to prevent the cpufreq-dt
> platdev from being created automatically, unless there is a better way to handle
> this dependency...
Right. Patches are already on the list to fix all platforms that had such
issues.
--
viresh