Move of powerpc/cell cpufreq drivers

Move of powerpc/cell cpufreq drivers

[Jean Delvare]

Hi all,

In this commit:

commit 6eb1c377423572374518f5be93214d139d113090
Author: Viresh Kumar
Date:   Mon Mar 25 11:20:23 2013 +0530

    cpufreq: powerpc/platforms/cell: move cpufreq driver to drivers/cpufreq

the cbe_cpufreq_pmi and cbe-cpufreq drivers were moved and renamed,
together with their respective Kconfig symbols. In the move,
tristate CBE_CPUFREQ_PMI became bool CPU_FREQ_CBE_PMI. This makes it
impossible to build the ppc_cbe_cpufreq_pmi driver as a module, while
that was possible before the move.

I see nothing in the commit message that would suggest that this change
was intentional. Instead I suspect this is a mistake and
CPU_FREQ_CBE_PMI was actually supposed to be a tristate. As a matter of
fact, ppc_cbe_cpufreq.h includes this check:
  #if defined(CONFIG_CPU_FREQ_CBE_PMI) || defined(CONFIG_CPU_FREQ_CBE_PMI_MODULE)
so it is seems expected that CPU_FREQ_CBE_PMI can be set to m.

The current situation is kind of odd, because CPU_FREQ_CBE_PMI (bool)
depends on CPU_FREQ_CBE (tristate) so it is possible to have
CPU_FREQ_CBE=m and CPU_FREQ_CBE_PMI=y. If ppc_cbe_cpufreq_pmi really
depends on ppc_cbe_cpufreq, then that will fail, as ppc_cbe_cpufreq as
a module may not be loaded when ppc_cbe_cpufreq_pmi is initialized. The
compiler would complain about missing symbols. So I suspect this
dependency doesn't actually exist, otherwise after one year someone
would have noticed the build breakage, right?

As a conclusion I believe the following changes should be applied:
* CPU_FREQ_CBE_PMI should be changed to a tristate.
* CPU_FREQ_CBE_PMI should not depend on CPU_FREQ_CBE.

I can write and submit the patches once it is confirmed that this
changes are the right way to fix the problem.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: Move of powerpc/cell cpufreq drivers

[Arnd Bergmann]

On Friday 28 March 2014, Jean Delvare wrote:

> The current situation is kind of odd, because CPU_FREQ_CBE_PMI (bool)
> depends on CPU_FREQ_CBE (tristate) so it is possible to have
> CPU_FREQ_CBE=m and CPU_FREQ_CBE_PMI=y. If ppc_cbe_cpufreq_pmi really
> depends on ppc_cbe_cpufreq, then that will fail, as ppc_cbe_cpufreq as
> a module may not be loaded when ppc_cbe_cpufreq_pmi is initialized. The
> compiler would complain about missing symbols. So I suspect this
> dependency doesn't actually exist, otherwise after one year someone
> would have noticed the build breakage, right?

I believe CPU_FREQ_CBE can be built with support for CPU_FREQ_CBE_PMI
or without, but if the PMI code is a module, then CPU_FREQ_CBE
cannot be 'y'.	

> As a conclusion I believe the following changes should be applied:
> * CPU_FREQ_CBE_PMI should be changed to a tristate.
> * CPU_FREQ_CBE_PMI should not depend on CPU_FREQ_CBE.
> 
> I can write and submit the patches once it is confirmed that this
> changes are the right way to fix the problem.

If you do this, you should also add

	depends on CPU_FREQ_CBE_PMI || !CPU_FREQ_CBE_PMI

to the CPU_FREQ_CBE option, to correctly handle the dependency.

I don't actually see a problem with the current code, other than
that CPU_FREQ_CBE_PMI could in theory be a module. This code is
used only for an IBM machine that has been end-of-service for
a couple of years. If anyone still wants to build kernels for
it, they probably want it built-in anyway, and not have
the same kernel run on other machines.

	Arnd

Re: Move of powerpc/cell cpufreq drivers

[Jean Delvare]

Hi Arnd,

Thanks for the quick reply.

Le Friday 28 March 2014 à 19:02 +0100, Arnd Bergmann a écrit :
> On Friday 28 March 2014, Jean Delvare wrote:
> 
> > The current situation is kind of odd, because CPU_FREQ_CBE_PMI (bool)
> > depends on CPU_FREQ_CBE (tristate) so it is possible to have
> > CPU_FREQ_CBE=m and CPU_FREQ_CBE_PMI=y. If ppc_cbe_cpufreq_pmi really
> > depends on ppc_cbe_cpufreq, then that will fail, as ppc_cbe_cpufreq as
> > a module may not be loaded when ppc_cbe_cpufreq_pmi is initialized. The
> > compiler would complain about missing symbols. So I suspect this
> > dependency doesn't actually exist, otherwise after one year someone
> > would have noticed the build breakage, right?
> 
> I believe CPU_FREQ_CBE can be built with support for CPU_FREQ_CBE_PMI
> or without, but if the PMI code is a module, then CPU_FREQ_CBE
> cannot be 'y'.	
> 
> > As a conclusion I believe the following changes should be applied:
> > * CPU_FREQ_CBE_PMI should be changed to a tristate.
> > * CPU_FREQ_CBE_PMI should not depend on CPU_FREQ_CBE.
> > 
> > I can write and submit the patches once it is confirmed that this
> > changes are the right way to fix the problem.
> 
> If you do this, you should also add
> 
> 	depends on CPU_FREQ_CBE_PMI || !CPU_FREQ_CBE_PMI
> 
> to the CPU_FREQ_CBE option, to correctly handle the dependency.

Ah, my bad, I thought both modules were more or less independent, I
didn't realize ppc_cbe_cpufreq_pmi was actually a helper "module" for
ppc_cbe_cpufreq when enabled.

Thinking about it some more, I think this would be much more simple to
build ppc_cbe_cpufreq_pmi.c as an object that would part of
ppc_cbe_cpufreq.ko. That is, build everything as a single module in the
end. That would solve the cross-dependency issue, and the code in
ppc_cbe_cpufreq_pmi would no longer have to be built-in.

But well, I don't really have the time to look into this, especially as
I don't even have a powerpc machine at hand to test the changes. So if
nobody cares, things will have to stay the way they are.

> I don't actually see a problem with the current code, other than
> that CPU_FREQ_CBE_PMI could in theory be a module. This code is
> used only for an IBM machine that has been end-of-service for
> a couple of years. If anyone still wants to build kernels for
> it, they probably want it built-in anyway, and not have
> the same kernel run on other machines.

Thanks for pointing this out. I don't know much about powerpc hardware
so I didn't realize that. We indeed ended up disabling this driver
altogether in our default ppc kernel config. Which was the easiest way
to solve our problem after all :-)

-- 
Jean Delvare
SUSE L3 Support

Re: Move of powerpc/cell cpufreq drivers

[Arnd Bergmann]

On Friday 28 March 2014, Jean Delvare wrote:
> Le Friday 28 March 2014 à 19:02 +0100, Arnd Bergmann a écrit :
> > On Friday 28 March 2014, Jean Delvare wrote:
> > 
> > > The current situation is kind of odd, because CPU_FREQ_CBE_PMI (bool)
> > > depends on CPU_FREQ_CBE (tristate) so it is possible to have
> > > CPU_FREQ_CBE=m and CPU_FREQ_CBE_PMI=y. If ppc_cbe_cpufreq_pmi really
> > > depends on ppc_cbe_cpufreq, then that will fail, as ppc_cbe_cpufreq as
> > > a module may not be loaded when ppc_cbe_cpufreq_pmi is initialized. The
> > > compiler would complain about missing symbols. So I suspect this
> > > dependency doesn't actually exist, otherwise after one year someone
> > > would have noticed the build breakage, right?
> > 
> > I believe CPU_FREQ_CBE can be built with support for CPU_FREQ_CBE_PMI
> > or without, but if the PMI code is a module, then CPU_FREQ_CBE
> > cannot be 'y'.        
> > 
> > > As a conclusion I believe the following changes should be applied:
> > > * CPU_FREQ_CBE_PMI should be changed to a tristate.
> > > * CPU_FREQ_CBE_PMI should not depend on CPU_FREQ_CBE.
> > > 
> > > I can write and submit the patches once it is confirmed that this
> > > changes are the right way to fix the problem.
> > 
> > If you do this, you should also add
> > 
> >       depends on CPU_FREQ_CBE_PMI || !CPU_FREQ_CBE_PMI
> > 
> > to the CPU_FREQ_CBE option, to correctly handle the dependency.
> 
> Ah, my bad, I thought both modules were more or less independent, I
> didn't realize ppc_cbe_cpufreq_pmi was actually a helper "module" for
> ppc_cbe_cpufreq when enabled.
> 
> Thinking about it some more, I think this would be much more simple to
> build ppc_cbe_cpufreq_pmi.c as an object that would part of
> ppc_cbe_cpufreq.ko. That is, build everything as a single module in the
> end. That would solve the cross-dependency issue, and the code in
> ppc_cbe_cpufreq_pmi would no longer have to be built-in.
> 
> But well, I don't really have the time to look into this, especially as
> I don't even have a powerpc machine at hand to test the changes. So if
> nobody cares, things will have to stay the way they are.

Fine with me.

> > I don't actually see a problem with the current code, other than
> > that CPU_FREQ_CBE_PMI could in theory be a module. This code is
> > used only for an IBM machine that has been end-of-service for
> > a couple of years. If anyone still wants to build kernels for
> > it, they probably want it built-in anyway, and not have
> > the same kernel run on other machines.
> 
> Thanks for pointing this out. I don't know much about powerpc hardware
> so I didn't realize that. We indeed ended up disabling this driver
> altogether in our default ppc kernel config. Which was the easiest way
> to solve our problem after all :-)

You probably want to disable all of PPC_CELL anyway, which implies
disabling this driver. There are a few hacks that get enabled if you
want Cell support that have a small impact on performance on other
platforms, and I don't expect anyone to run distro kernels on
these machines any more.

	Arnd

Re: Move of powerpc/cell cpufreq drivers

[Jean Delvare]

Hi Arnd,

Le Saturday 29 March 2014 à 03:46 +0100, Arnd Bergmann a écrit :
> On Friday 28 March 2014, Jean Delvare wrote:
> > Thanks for pointing this out. I don't know much about powerpc hardware
> > so I didn't realize that. We indeed ended up disabling this driver
> > altogether in our default ppc kernel config. Which was the easiest way
> > to solve our problem after all :-)
> 
> You probably want to disable all of PPC_CELL anyway, which implies
> disabling this driver. There are a few hacks that get enabled if you
> want Cell support that have a small impact on performance on other
> platforms, and I don't expect anyone to run distro kernels on
> these machines any more.

That's exactly what we did.

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: Move of powerpc/cell cpufreq drivers

[Viresh Kumar]

On 28 March 2014 14:56, Jean Delvare <jdelvare@suse.de> wrote:
> the cbe_cpufreq_pmi and cbe-cpufreq drivers were moved and renamed,
> together with their respective Kconfig symbols. In the move,
> tristate CBE_CPUFREQ_PMI became bool CPU_FREQ_CBE_PMI. This makes it
> impossible to build the ppc_cbe_cpufreq_pmi driver as a module, while
> that was possible before the move.
>
> I see nothing in the commit message that would suggest that this change
> was intentional. Instead I suspect this is a mistake and
> CPU_FREQ_CBE_PMI was actually supposed to be a tristate.

Yes, that was a mistake and it happened because I removed
CBE_CPUFREQ_PMI_ENABLE completely and reused its definition for
CBE_CPUFREQ_PMI and didn't notice the bool things :(