Re: [PATCH 0/6] ARM: cpuidle: Unify the ARM64/ARM DT approach

[Lorenzo Pieralisi]

On Fri, Mar 13, 2015 at 08:51:38PM +0000, Rob Herring wrote:
> On Thu, Mar 12, 2015 at 9:25 AM, Daniel Lezcano
> <daniel.lezcano@xxxxxxxxxx> wrote:
> >
> > Catalin, Rob,
> >
> > do you agree with this patchset ?
> 
> There's very little to do with DT, but looks fine to me.

There are key points related to DT, that are not necessarily
visible within this set, that I fully support BTW.

(1) enable-method: we do need platforms to define an enable-method
    to match cpuidle ops. Nothing new, but we should keep that in mind.
(2) platform back-ends will match idle states according to idle state
    compatible string (to know what function to execute on idle state
    entry for a given idle state the compatible string is used, in DT
    standard way, to match the compatible string and get corresponding
    enter function and parameters/data). The cpuidle_ops suspend method
    will get an index from the idle driver that through a look-up will be
    translated into a idle entry call with respective parameters that are
    back-end specific. PSCI does that already on ARM64.

If we agree on these two points there is not much to debate anymore,
I will review and provide tags according this week, apart from a couple
of nits on functions naming and cpuidle_ops structures replication
the set seems good to go to me.

Lorenzo

> 
> >
> > Thanks
> >   -- Daniel
> >
> >
> > On 03/03/2015 01:29 PM, Daniel Lezcano wrote:
> >>
> >> There is a big number of cpuidle drivers for the ARM architecture.
> >>
> >> These drivers have been cleaned up and grouped into the drivers/cpuidle
> >> directory to keep track of the changes more easily and ensure the code
> >> is following the same scheme across the drivers.
> >>
> >> That had the benefit of simplifying the code and factor out a lot of
> >> common
> >> parts. Beside that, as the drivers belong to the 'drivers' directory, we
> >> had
> >> to split the arch specific bits and the generic code in order to keep
> >> everything self contained. The platform driver paradigm was used for this
> >> purpose.
> >>
> >> Unfortunately, this approach is now no longer accepted and a different
> >> solution
> >> must be provided to reach the same goal: one example is the Qualcomm
> >> cpuidle
> >> driver upstreaming attempt [1].
> >>
> >> In the meantime, ARM64 developed a generic cpuidle driver based on DT
> >> definition.
> >>
> >> The DT definition provides an 'enable-method' to specify one of the cpu
> >> operations (PSCI, ...).
> >>
> >> This patchset unify this driver with ARM32, using the same DT definition.
> >>
> >> Thanks with this patchset we can use the 'enable-method' to specify a cpu
> >> operations, hence get rid of the platform driver approach and go further
> >> in the
> >> cpuidle driver flexibility via the DT.
> >>
> >> Daniel Lezcano (6):
> >>    ARM: cpuidle: Remove duplicate header inclusion
> >>    ARM: cpuidle: Add a cpuidle ops structure to be used for DT
> >>    ARM64: cpuidle: Replace cpu_suspend by the common ARM/ARM64 function
> >>    ARM64: cpuidle: Rename cpu_init_idle to a common function name
> >>    ARM64: cpuidle: Remove arm64 reference
> >>    ARM: cpuidle: Enable the ARM64 driver for both ARM32/ARM64
> >>
> >>   arch/arm/include/asm/cpuidle.h                     | 12 +++
> >>   arch/arm/include/asm/cpuidle_ops.h                 |  3 +
> >>   arch/arm/kernel/cpuidle.c                          | 87
> >> +++++++++++++++++++++-
> >>   arch/arm/mach-davinci/cpuidle.c                    |  1 -
> >>   arch/arm/mach-imx/cpuidle-imx6q.c                  |  1 -
> >>   arch/arm/mach-imx/cpuidle-imx6sl.c                 |  1 -
> >>   arch/arm/mach-imx/cpuidle-imx6sx.c                 |  1 -
> >>   arch/arm/mach-omap2/cpuidle44xx.c                  |  1 -
> >>   arch/arm/mach-tegra/cpuidle-tegra20.c              |  1 -
> >>   arch/arm/mach-tegra/cpuidle-tegra30.c              |  1 -
> >>   arch/arm64/include/asm/cpuidle.h                   |  9 ++-
> >>   arch/arm64/kernel/cpuidle.c                        |  2 +-
> >>   drivers/cpuidle/Kconfig                            |  7 +-
> >>   drivers/cpuidle/Kconfig.arm                        | 10 +++
> >>   drivers/cpuidle/Kconfig.arm64                      | 13 ----
> >>   drivers/cpuidle/Makefile                           |  5 +-
> >>   drivers/cpuidle/{cpuidle-arm64.c => cpuidle-arm.c} | 38 +++++-----
> >>   drivers/cpuidle/cpuidle-at91.c                     |  1 -
> >>   drivers/cpuidle/cpuidle-exynos.c                   |  1 -
> >>   drivers/cpuidle/cpuidle-kirkwood.c                 |  1 -
> >>   drivers/cpuidle/cpuidle-ux500.c                    |  1 -
> >>   drivers/cpuidle/cpuidle-zynq.c                     |  1 -
> >>   22 files changed, 139 insertions(+), 59 deletions(-)
> >>   create mode 100644 arch/arm/include/asm/cpuidle_ops.h
> >>   delete mode 100644 drivers/cpuidle/Kconfig.arm64
> >>   rename drivers/cpuidle/{cpuidle-arm64.c => cpuidle-arm.c} (72%)
> >>
> >
> >
> > --
> >  <http://www.linaro.org/> Linaro.org | Open source software for ARM SoCs
> >
> > Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
> > <http://twitter.com/#!/linaroorg> Twitter |
> > <http://www.linaro.org/linaro-blog/> Blog
> >
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/