On Wed, Jun 25, 2014 at 04:13:33PM +0100, Mark Rutland wrote:
> On Wed, Jun 25, 2014 at 03:10:20PM +0100, Lorenzo Pieralisi wrote:
> > With the introduction of DT based idle states, CPUidle drivers for
> > ARM can now initialize idle states data through properties in the device
> > tree.
> >
> > This patch adds code to the Exynos CPUidle driver to dynamically
> > initialize idle states data through the updated device tree source
> > files.
> >
> > Cc: Kukjin Kim <kgene.kim@xxxxxxxxxxx>
> > Cc: Tomasz Figa <t.figa@xxxxxxxxxxx>
> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi@xxxxxxx>
> > ---
> > Compile tested, I am not sure I patched the right dts files, please check.
> >
> > .../devicetree/bindings/arm/exynos/idle-states.txt | 27 ++++++++++++++++++++
> > arch/arm/boot/dts/exynos3250.dtsi | 16 ++++++++++++
> > arch/arm/boot/dts/exynos5250.dtsi | 15 +++++++++++
> > arch/arm/boot/dts/exynos5410.dtsi | 17 +++++++++++++
> > drivers/cpuidle/Kconfig.arm | 1 +
> > drivers/cpuidle/cpuidle-exynos.c | 29 +++++++++++++---------
> > 6 files changed, 93 insertions(+), 12 deletions(-)
> > create mode 100644 Documentation/devicetree/bindings/arm/exynos/idle-states.txt
> >
> > diff --git a/Documentation/devicetree/bindings/arm/exynos/idle-states.txt b/Documentation/devicetree/bindings/arm/exynos/idle-states.txt
> > new file mode 100644
> > index 0000000..342ecb4
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/exynos/idle-states.txt
> > @@ -0,0 +1,27 @@
> > +idle-states node
> > +----------------
> > +
> > +On Exynos platforms with power management capabilities, the device
> > +tree source file must contain the idle-states node[1]. As defined in [1] the
> > +idle-states node must contain an entry-method property that for Exynos
> > +platforms can be one of:
> > +
> > + - "samsung,exynos"
>
> Similarly to the TC2 binding, what does this mean?
>
> What is a kernel expected to do when it sees this entry-method?
>
> Using "samsung,exynos" as the entry-method feels like something that's
> going to bite us; it sounds far too wide-reaching.
Same story as TC2, it adds nothing to the patch, it is just there for
compliance with current DT bindings, but useless and will disappear.
> > static int exynos_cpuidle_probe(struct platform_device *pdev)
> > {
> > - int ret;
> > + int ret, i;
> > + struct cpuidle_driver *drv = &exynos_idle_driver;
> >
> > exynos_enter_aftr = (void *)(pdev->dev.platform_data);
> >
> > - ret = cpuidle_register(&exynos_idle_driver, NULL);
> > + drv->cpumask = (struct cpumask *) cpu_possible_mask;
>
> This assignment looks scary to me. Why do we need to do this, and why
> are we throwing away the constness of cpu_possible_mask?
Yes, that's how it is done in CPUidle core if the idle driver does not
initialize cpumask pointer, I guess it is to save some bytes, but I agree
with you, I do not like that either, I will allocate the mask and copy.
Thanks,
Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
