The cpufreq-dt-platdev driver supports creation of cpufreq-dt platform
device now, reuse that and remove similar code from platform code.
Cc: Thomas Petazzoni <[email protected]>
Cc: Jason Cooper <[email protected]>
Cc: Andrew Lunn <[email protected]>
Cc: Gregory Clement <[email protected]>
Cc: Sebastian Hesselbarth <[email protected]>
Signed-off-by: Viresh Kumar <[email protected]>
---
arch/arm/mach-mvebu/pmsu.c | 2 --
drivers/cpufreq/cpufreq-dt-platdev.c | 2 ++
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c
index 79d0a5d9da8e..f24f46776fbb 100644
--- a/arch/arm/mach-mvebu/pmsu.c
+++ b/arch/arm/mach-mvebu/pmsu.c
@@ -685,8 +685,6 @@ static int __init armada_xp_pmsu_cpufreq_init(void)
dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n",
__func__, ret);
}
-
- platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
return 0;
}
diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
index 69b2a222c84e..5704a92c52dc 100644
--- a/drivers/cpufreq/cpufreq-dt-platdev.c
+++ b/drivers/cpufreq/cpufreq-dt-platdev.c
@@ -33,6 +33,8 @@ static const struct of_device_id machines[] = {
{ .compatible = "marvell,berlin", },
+ { .compatible = "marvell,armadaxp", },
+
{ .compatible = "samsung,exynos3250", },
{ .compatible = "samsung,exynos4210", },
{ .compatible = "samsung,exynos4212", },
--
2.7.1.410.g6faf27b
On Thursday 21 April 2016 14:29:02 Viresh Kumar wrote:
> diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c
> index 79d0a5d9da8e..f24f46776fbb 100644
> --- a/arch/arm/mach-mvebu/pmsu.c
> +++ b/arch/arm/mach-mvebu/pmsu.c
> @@ -685,8 +685,6 @@ static int __init armada_xp_pmsu_cpufreq_init(void)
> dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n",
> __func__, ret);
> }
> -
> - platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
> return 0;
> }
>
> diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
> index 69b2a222c84e..5704a92c52dc 100644
> --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> @@ -33,6 +33,8 @@ static const struct of_device_id machines[] = {
>
> { .compatible = "marvell,berlin", },
>
> + { .compatible = "marvell,armadaxp", },
> +
> { .compatible = "samsung,exynos3250", },
> { .compatible = "samsung,exynos4210", },
> { .compatible = "samsung,exynos4212", },
I think to make it clear that the ordering is significant here, I would leave this
platform_device_register_simple() in armada_xp_pmsu_cpufreq_init().
However, it might be helpful to move that function into a new file in
drivers/cpufreq/ if that works.
Arnd
On 23-04-16, 00:42, Arnd Bergmann wrote:
> On Thursday 21 April 2016 14:29:02 Viresh Kumar wrote:
> > diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c
> > index 79d0a5d9da8e..f24f46776fbb 100644
> > --- a/arch/arm/mach-mvebu/pmsu.c
> > +++ b/arch/arm/mach-mvebu/pmsu.c
> > @@ -685,8 +685,6 @@ static int __init armada_xp_pmsu_cpufreq_init(void)
> > dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n",
> > __func__, ret);
> > }
> > -
> > - platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
> > return 0;
> > }
> >
> > diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
> > index 69b2a222c84e..5704a92c52dc 100644
> > --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> > +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> > @@ -33,6 +33,8 @@ static const struct of_device_id machines[] = {
> >
> > { .compatible = "marvell,berlin", },
> >
> > + { .compatible = "marvell,armadaxp", },
> > +
> > { .compatible = "samsung,exynos3250", },
> > { .compatible = "samsung,exynos4210", },
> > { .compatible = "samsung,exynos4212", },
>
> I think to make it clear that the ordering is significant here, I would leave this
> platform_device_register_simple() in armada_xp_pmsu_cpufreq_init().
>
> However, it might be helpful to move that function into a new file in
> drivers/cpufreq/ if that works.
We just can't get wrong with the ordering here, as this is done from
device_initcall() and so that point is out of question.
The other thing that can happen is that armada_xp_pmsu_cpufreq_init()
call can fail. In that case, most of the times cpufreq-dt ->init()
will fail as well, so even that is fine for me.
And, so I think we can keep this patch as is.
Do you agree ?
--
viresh
On Monday 25 April 2016 08:30:41 Viresh Kumar wrote:
> On 23-04-16, 00:42, Arnd Bergmann wrote:
> > On Thursday 21 April 2016 14:29:02 Viresh Kumar wrote:
> > > diff --git a/arch/arm/mach-mvebu/pmsu.c b/arch/arm/mach-mvebu/pmsu.c
> > > index 79d0a5d9da8e..f24f46776fbb 100644
> > > --- a/arch/arm/mach-mvebu/pmsu.c
> > > +++ b/arch/arm/mach-mvebu/pmsu.c
> > > @@ -685,8 +685,6 @@ static int __init armada_xp_pmsu_cpufreq_init(void)
> > > dev_err(cpu_dev, "%s: failed to mark OPPs as shared: %d\n",
> > > __func__, ret);
> > > }
> > > -
> > > - platform_device_register_simple("cpufreq-dt", -1, NULL, 0);
> > > return 0;
> > > }
> > >
> > > diff --git a/drivers/cpufreq/cpufreq-dt-platdev.c b/drivers/cpufreq/cpufreq-dt-platdev.c
> > > index 69b2a222c84e..5704a92c52dc 100644
> > > --- a/drivers/cpufreq/cpufreq-dt-platdev.c
> > > +++ b/drivers/cpufreq/cpufreq-dt-platdev.c
> > > @@ -33,6 +33,8 @@ static const struct of_device_id machines[] = {
> > >
> > > { .compatible = "marvell,berlin", },
> > >
> > > + { .compatible = "marvell,armadaxp", },
> > > +
> > > { .compatible = "samsung,exynos3250", },
> > > { .compatible = "samsung,exynos4210", },
> > > { .compatible = "samsung,exynos4212", },
> >
> > I think to make it clear that the ordering is significant here, I would leave this
> > platform_device_register_simple() in armada_xp_pmsu_cpufreq_init().
> >
> > However, it might be helpful to move that function into a new file in
> > drivers/cpufreq/ if that works.
>
> We just can't get wrong with the ordering here, as this is done from
> device_initcall() and so that point is out of question.
I realize that the ordering is fixed through the way that the kernel
is linked, my worry is more about someone changing the code in some
way because it's not obvious from reading the code that the
dependency exists. If either the armada_xp_pmsu_cpufreq_init()
initcall gets changed so it does not always get called, or the
cpufreq_dt_platdev_init() initcall gets changed so it comes a little
earlier, things will break.
> The other thing that can happen is that armada_xp_pmsu_cpufreq_init()
> call can fail. In that case, most of the times cpufreq-dt ->init()
> will fail as well, so even that is fine for me.
>
> And, so I think we can keep this patch as is.
What are the downsides of moving armada_xp_pmsu_cpufreq_init()
into drivers/cpufreq?
Arnd
On 25-04-16, 14:53, Arnd Bergmann wrote:
> On Monday 25 April 2016 08:30:41 Viresh Kumar wrote:
> I realize that the ordering is fixed through the way that the kernel
> is linked, my worry is more about someone changing the code in some
> way because it's not obvious from reading the code that the
> dependency exists. If either the armada_xp_pmsu_cpufreq_init()
> initcall gets changed so it does not always get called, or the
> cpufreq_dt_platdev_init() initcall gets changed so it comes a little
> earlier, things will break.
cpufreq-dt will just error out in that case, because it wouldn't find
any OPPs registered to the OPP-core. It *shouldn't* crash and if it
does, then we have a problem to fix.
> > The other thing that can happen is that armada_xp_pmsu_cpufreq_init()
> > call can fail. In that case, most of the times cpufreq-dt ->init()
> > will fail as well, so even that is fine for me.
> >
> > And, so I think we can keep this patch as is.
>
> What are the downsides of moving armada_xp_pmsu_cpufreq_init()
> into drivers/cpufreq?
More special code :)
--
viresh
On Monday 25 April 2016 18:26:05 Viresh Kumar wrote:
> On 25-04-16, 14:53, Arnd Bergmann wrote:
> > On Monday 25 April 2016 08:30:41 Viresh Kumar wrote:
>
> > I realize that the ordering is fixed through the way that the kernel
> > is linked, my worry is more about someone changing the code in some
> > way because it's not obvious from reading the code that the
> > dependency exists. If either the armada_xp_pmsu_cpufreq_init()
> > initcall gets changed so it does not always get called, or the
> > cpufreq_dt_platdev_init() initcall gets changed so it comes a little
> > earlier, things will break.
>
> cpufreq-dt will just error out in that case, because it wouldn't find
> any OPPs registered to the OPP-core. It *shouldn't* crash and if it
> does, then we have a problem to fix.
Ok.
> > > The other thing that can happen is that armada_xp_pmsu_cpufreq_init()
> > > call can fail. In that case, most of the times cpufreq-dt ->init()
> > > will fail as well, so even that is fine for me.
> > >
> > > And, so I think we can keep this patch as is.
> >
> > What are the downsides of moving armada_xp_pmsu_cpufreq_init()
> > into drivers/cpufreq?
>
> More special code :)
Of course the special code still exists, it seems more like neither of
us wants to have it in the portion of the kernel that he maintains ;-)
Maybe the mvebu maintainers have a preference where they'd like the
code to be, they are the ones that are most impacted if anything
goes wrong.
Arnd
On 25-04-16, 17:26, Arnd Bergmann wrote:
> On Monday 25 April 2016 18:26:05 Viresh Kumar wrote:
> > On 25-04-16, 14:53, Arnd Bergmann wrote:
> > > On Monday 25 April 2016 08:30:41 Viresh Kumar wrote:
> >
> > > I realize that the ordering is fixed through the way that the kernel
> > > is linked, my worry is more about someone changing the code in some
> > > way because it's not obvious from reading the code that the
> > > dependency exists. If either the armada_xp_pmsu_cpufreq_init()
> > > initcall gets changed so it does not always get called, or the
> > > cpufreq_dt_platdev_init() initcall gets changed so it comes a little
> > > earlier, things will break.
> >
> > cpufreq-dt will just error out in that case, because it wouldn't find
> > any OPPs registered to the OPP-core. It *shouldn't* crash and if it
> > does, then we have a problem to fix.
>
> Ok.
>
> > > > The other thing that can happen is that armada_xp_pmsu_cpufreq_init()
> > > > call can fail. In that case, most of the times cpufreq-dt ->init()
> > > > will fail as well, so even that is fine for me.
> > > >
> > > > And, so I think we can keep this patch as is.
> > >
> > > What are the downsides of moving armada_xp_pmsu_cpufreq_init()
> > > into drivers/cpufreq?
> >
> > More special code :)
>
> Of course the special code still exists, it seems more like neither of
> us wants to have it in the portion of the kernel that he maintains ;-)
Hehe.. But after $subject patch, we don't have any special code for
creating the device, isn't it?
> Maybe the mvebu maintainers have a preference where they'd like the
> code to be, they are the ones that are most impacted if anything
> goes wrong.
What code are you talking about? Initializing the OPPs or adding the
cpufreq-dt device? The first one (or whatever is left now in that
function) can stay anywhere, even as a cpufreq driver. I was talking
about the fact that we don't have a sequence problem to solve here.
--
viresh
On Monday 25 April 2016 20:59:14 Viresh Kumar wrote:
> On 25-04-16, 17:26, Arnd Bergmann wrote:
> > On Monday 25 April 2016 18:26:05 Viresh Kumar wrote:
> > > On 25-04-16, 14:53, Arnd Bergmann wrote:
> > > > What are the downsides of moving armada_xp_pmsu_cpufreq_init()
> > > > into drivers/cpufreq?
> > >
> > > More special code :)
> >
> > Of course the special code still exists, it seems more like neither of
> > us wants to have it in the portion of the kernel that he maintains ;-)
>
> Hehe.. But after $subject patch, we don't have any special code for
> creating the device, isn't it?
>
> > Maybe the mvebu maintainers have a preference where they'd like the
> > code to be, they are the ones that are most impacted if anything
> > goes wrong.
>
> What code are you talking about? Initializing the OPPs or adding the
> cpufreq-dt device? The first one (or whatever is left now in that
> function) can stay anywhere, even as a cpufreq driver. I was talking
> about the fact that we don't have a sequence problem to solve here.
My line of thinking was that the armada_xp_pmsu_cpufreq_init()
function makes sense by itself and feels like it should be
one file in drivers/cpufreq, including the creation of the device.
Even without the argument of the sequencing, they two parts sort
of belong together because the cpufreq-dt driver depends on both
of them being run before it can function. It's also the same amount
of code, as you are replacing one line in armada_xp_pmsu_cpufreq_init
with one line in "struct of_device_id machines".
It's not really that important, just a feeling I had that it could
be done better. Unless the mvebu maintainers feel strongly about
it, just do as you prefer.
Arnd
Hello,
On Mon, 25 Apr 2016 17:46:53 +0200, Arnd Bergmann wrote:
> > What code are you talking about? Initializing the OPPs or adding the
> > cpufreq-dt device? The first one (or whatever is left now in that
> > function) can stay anywhere, even as a cpufreq driver. I was talking
> > about the fact that we don't have a sequence problem to solve here.
>
> My line of thinking was that the armada_xp_pmsu_cpufreq_init()
> function makes sense by itself and feels like it should be
> one file in drivers/cpufreq, including the creation of the device.
>
> Even without the argument of the sequencing, they two parts sort
> of belong together because the cpufreq-dt driver depends on both
> of them being run before it can function. It's also the same amount
> of code, as you are replacing one line in armada_xp_pmsu_cpufreq_init
> with one line in "struct of_device_id machines".
>
> It's not really that important, just a feeling I had that it could
> be done better. Unless the mvebu maintainers feel strongly about
> it, just do as you prefer.
As a mvebu folk, I don't really have a strong opinion on this. We also
have some cpufreq device registration code in
arch/arm/mach-mvebu/kirkwood.c for the older Kirkwood platform, though
this one uses a custom cpufreq driver and not the generic cpufreq-dt
driver.
Ideally, in the grand direction of removing as much things as possible
from mach-<foo> directories, it would be great to move such
initializations somewhere else. But cpufreq is not by far not the only
reason why we still have code in mach-<foo>, at least in the mvebu land.
Thomas
--
Thomas Petazzoni, CTO, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com