Re: [RFC PATCH V3 3/5] powerpc/cpuidle: Generic powerpc backend cpuidle driver.

[Deepthi Dharwar <deepthi@linux.vnet.ibm.com>]

On 08/22/2013 01:38 AM, Scott Wood wrote:
> On Wed, 2013-08-21 at 10:23 +0530, Deepthi Dharwar wrote:
>> On 08/19/2013 11:47 PM, Scott Wood wrote:
>>> On Mon, 2013-08-19 at 15:48 +0530, Deepthi Dharwar wrote:
>>>> Hi Dongsheng,
>>>>
>>>> On 08/19/2013 11:22 AM, Wang Dongsheng-B40534 wrote:
>>>>> I think we should move the states and handle function to arch/power/platform*
>>>>> The states and handle function is belong to backend driver, not for this, different platform have different state.
>>>>> Different platforms to make their own deal with these states.
>>>>>
>>>>> I think we cannot put all the status of different platforms and handler in this driver.
>>>>
>>>> The idea here is a single powerpc back-end driver, which does a runtime
>>>> detection of the platform it is running and choose the right
>>>> idle states table. This was one of outcome of V2 discussion.
>>>
>>> I see a lot more in there than just detecting a platform and choosing a
>>> driver.
>>>
>>>> I feel there is no harm in keeping the state information in the same
>>>> file. We do have x86, which has all its variants information in one
>>>> file. One place will have all the idle consolidated information of
>>>> all the platform variants. If community does feel, we need to
>>>> have just the states information in arch specific file, we can do so.
>>>
>>> What actual functionality is common to all powerpc but not common to
>>> other arches?
> 

The functionality here is idle states on powerpc  like the snooze loop
that is common.
Also, the basic registration of the driver, hotplug notifier etc for
powerpc.

> 
>>>>>> +config CPU_IDLE_POWERPC
>>>>>> +	bool "CPU Idle driver for POWERPC platforms"
>>>>>> +	depends on PPC64
>>>>>
>>>>> Why not PPC?
>>>>
>>>> PPC64 seems to a good place to began the consolidation work. This
>>>> patch-set has not been tested for PPC32 currently.
>>>
>>> PPC64 is a bad place to start if you want it to be generic, because it
>>> means you'll end up growing dependencies on other things that are PPC64
>>> only.  There are too many arbitrary 32/64 differences as is.
>>
>> Hi Scott,
>>
>> From my understanding, PPC64 includes BOOK3E and BOOK3S archs.
>> PPC includes PPC32 and PPC64.
>>
>> It seemed logical to start consolidating at PPC64 as
>> one does not want to get into 32/64 bit differences.
> 
> I don't want to "get into" a file that claims to be generic PPC but is
> loaded with 64-bit dependencies.
> 
>> From your comments above,  I just wanted to clarify if PPC or PPC64 is
>> bad place to start. If PPC64 is bad place to start, then whats the way
>> forward ?  Can you please throw some more light on it.
> 
> The way forward is to give this file a more appropriate name based on
> the hardware that it actually targets -- and to refactor it so that the
> answer to that question is not complicated.

Sure, thanks.
Our idea was to have POWER archs idle states merged at the first go.
Only that is what is enabled in the current version (V4 posted out)
( Code is enabled for PSERIES and POWERNV only)
If needed, other POWERPC archs might benefit by extending the same
driver, that is why it is named cpuidle-powerpc.c

But if having cpuidle backend-driver separately for other powerpc arcs
makes sense such that each one have their own state information etc
then it makes sense to name the files as cpuidle-power.c,
cpuilde-ppc32.c and so on.

Regards,
Deepthi


> 
> -Scott
> 
> 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
> 
>