Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1383969 > unrolled thread

[PATCH 10/10] cpufreq: mvebu: Use generic platdev driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 10/10] cpufreq: mvebu: Use generic platdev driver Viresh Kumar <viresh.kumar@linaro.org> - 2016-04-21 11:10 +0200
    Re: [PATCH 10/10] cpufreq: mvebu: Use generic platdev driver Arnd Bergmann <arnd@arndb.de> - 2016-04-23 00:50 +0200
      Re: [PATCH 10/10] cpufreq: mvebu: Use generic platdev driver Viresh Kumar <viresh.kumar@linaro.org> - 2016-04-25 05:10 +0200
        Re: [PATCH 10/10] cpufreq: mvebu: Use generic platdev driver Arnd Bergmann <arnd@arndb.de> - 2016-04-25 15:00 +0200
          Re: [PATCH 10/10] cpufreq: mvebu: Use generic platdev driver Viresh Kumar <viresh.kumar@linaro.org> - 2016-04-25 15:00 +0200
            Re: [PATCH 10/10] cpufreq: mvebu: Use generic platdev driver Arnd Bergmann <arnd@arndb.de> - 2016-04-25 17:30 +0200
              Re: [PATCH 10/10] cpufreq: mvebu: Use generic platdev driver Viresh Kumar <viresh.kumar@linaro.org> - 2016-04-25 17:30 +0200
                Re: [PATCH 10/10] cpufreq: mvebu: Use generic platdev driver Arnd Bergmann <arnd@arndb.de> - 2016-04-25 17:50 +0200
                  Re: [PATCH 10/10] cpufreq: mvebu: Use generic platdev driver Thomas Petazzoni <thomas.petazzoni@free-electrons.com> - 2016-04-25 18:00 +0200

#1383969 — [PATCH 10/10] cpufreq: mvebu: Use generic platdev driver

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 <thomas.petazzoni@free-electrons.com>
Cc: Jason Cooper <jason@lakedaemon.net>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Gregory Clement <gregory.clement@free-electrons.com>
Cc: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 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

[toc] | [next] | [standalone]

#1385519

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

[toc] | [prev] | [next] | [standalone]

#1385991

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

[toc] | [prev] | [next] | [standalone]

#1386386

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

[toc] | [prev] | [next] | [standalone]

#1386389

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

[toc] | [prev] | [next] | [standalone]

#1386581

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

[toc] | [prev] | [next] | [standalone]

#1386583

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

[toc] | [prev] | [next] | [standalone]

#1386604

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

[toc] | [prev] | [next] | [standalone]

#1386614

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web