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

Groups > linux.kernel > #1180547 > unrolled thread

[PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend

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 v3 3/3] arm: kernel: implement cpuidle_ops with psci backend Jisheng Zhang <jszhang@marvell.com> - 2015-07-09 10:40 +0200
    Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci  backend Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2015-07-14 17:00 +0200
      Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci  backend Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-07-14 22:50 +0200
        Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci  backend Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2015-07-15 15:50 +0200
          Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci  backend Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-07-15 16:50 +0200

#1180547 — [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend

This patch implements cpuidle_ops using psci. After this patch, we can
use cpuidle-arm.c with psci backend for both arm and arm64.

Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
 arch/arm/kernel/Makefile       |  1 +
 arch/arm/kernel/psci_cpuidle.c | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)
 create mode 100644 arch/arm/kernel/psci_cpuidle.c

diff --git a/arch/arm/kernel/Makefile b/arch/arm/kernel/Makefile
index 3b995f5..96383d8 100644
--- a/arch/arm/kernel/Makefile
+++ b/arch/arm/kernel/Makefile
@@ -91,6 +91,7 @@ obj-$(CONFIG_ARM_VIRT_EXT)	+= hyp-stub.o
 ifeq ($(CONFIG_ARM_PSCI),y)
 obj-y				+= psci-call.o
 obj-$(CONFIG_SMP)		+= psci_smp.o
+obj-$(CONFIG_CPU_IDLE)		+= psci_cpuidle.o
 endif
 
 extra-y := $(head-y) vmlinux.lds
diff --git a/arch/arm/kernel/psci_cpuidle.c b/arch/arm/kernel/psci_cpuidle.c
new file mode 100644
index 0000000..e7146d2
--- /dev/null
+++ b/arch/arm/kernel/psci_cpuidle.c
@@ -0,0 +1,19 @@
+/*
+ * Copyright (C) 2015 Marvell Technology Group Ltd.
+ * Author: Jisheng Zhang <jszhang@marvell.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include <linux/cpuidle.h>
+#include <linux/psci.h>
+
+#include <asm/cpuidle.h>
+
+static struct cpuidle_ops psci_cpuidle_ops __initdata = {
+	.suspend = cpu_psci_cpu_suspend,
+	.init = cpu_psci_cpu_init_idle,
+};
+CPUIDLE_METHOD_OF_DECLARE(psci_cpuidle, "psci", &psci_cpuidle_ops);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1183761 — Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend

On Tue, Jul 14, 2015 at 01:29:04PM +0100, Russell King - ARM Linux wrote:
> On Tue, Jul 14, 2015 at 12:03:02PM +0100, Lorenzo Pieralisi wrote:
> > On Tue, Jul 14, 2015 at 11:34:21AM +0100, Russell King - ARM Linux wrote:
> > > > +static struct cpuidle_ops psci_cpuidle_ops __initdata = {
> > > > +	.suspend = cpu_psci_cpu_suspend,
> > > > +	.init = cpu_psci_cpu_init_idle,
> > > > +};
> > > > +CPUIDLE_METHOD_OF_DECLARE(psci_cpuidle, "psci", &psci_cpuidle_ops);
> > 
> > I take this as an ACK to M.Rutland's PSCI code move to drivers/firmware,
> > right ?
> 
> No, that's not something I've particularly looked at.  PSCI doesn't really
> interest me because I have no systems which (afaik) support it.

PSCI implementation is currently part of arch/arm which means it is
used on arch/arm (and related platforms) and you should be interested in it
for that specific reason.

It is not about what you are interested on, it is about the code you
maintain, so please have a look at it, thank you.

> > > We don't do this for other stuff - we don't have IRQ_CHIP_OF_DECLARE
> > > stuff in arch/arm but have the IRQ chip drivers in drivers/irqchip.
> > > We keep it all togehter in drivers/irqchip, even when the IRQ chip
> > > driver is only useful on ARM.
> > 
> > CPUidle operations are ARM only, they are not used on ARM64, so
> > they belong in arch/arm (that's the same thing as SMP ops, on ARM64
> > SMP ops and CPUidle ops are unified through CPU operations).
> 
> I don't agree with that.  CPU idle isn't an "ARM thing" at all, it's
> generic kernel infrastructure.  OF is generic kernel infrastructure too.

I said "CPUidle operations", not CPUidle, we know CPUidle is not an
ARM thing.

> Yet, we're stuffing _all_ the PSCI CPU idle code into
> drivers/firmware/psci.c, but then stuffing the PSCI OF data structures
> into arch/arm.  This is utterly _insane_.

Ok, so we will copy the ARM64 PSCI idle related code to arch/arm
and we are done with this, or we will have to ifdef drivers/firmware
code, take your pick.

> There is nothing ARM specific about these CPU idle ops.  They don't
> belong on arch/arm.

See above.

> NAK on this series (and the move of the PSCI code to drivers/firmware)

I do not accept that. You may NAK this series but you can't object to
moving PSCI firmware code to drivers/firmware for that reason, so
please have a look at Mark's patches again and ACK/NAK them for
a valid reason, it has been a while since he asked.

> until people start seeing sense with stuff like this.  Having stuff split
> between arch/arm/ and drivers/ like this is totally crap.  It makes code
> unnecessary complex for no reason what so ever.

That's rather vague and just your opinion. If you have a solution better
than this one you tell us about it otherwise you keep your comments for
yourself, thank you.

> Find a solution which doesn't involve leaving _just_ data structures to
> connect stuff to OF in arch/arm.

I will copy the PSCI CPUidle related functions from arm64 to arch/arm so that
it is not _just_ OF data structures and I hope we are done with this.

Lorenzo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1183989 — Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend

On Tue, Jul 14, 2015 at 03:55:46PM +0100, Lorenzo Pieralisi wrote:
> On Tue, Jul 14, 2015 at 01:29:04PM +0100, Russell King - ARM Linux wrote:
> > On Tue, Jul 14, 2015 at 12:03:02PM +0100, Lorenzo Pieralisi wrote:
> > > On Tue, Jul 14, 2015 at 11:34:21AM +0100, Russell King - ARM Linux wrote:
> > > > > +static struct cpuidle_ops psci_cpuidle_ops __initdata = {
> > > > > +	.suspend = cpu_psci_cpu_suspend,
> > > > > +	.init = cpu_psci_cpu_init_idle,
> > > > > +};
> > > > > +CPUIDLE_METHOD_OF_DECLARE(psci_cpuidle, "psci", &psci_cpuidle_ops);
> > > 
> > > I take this as an ACK to M.Rutland's PSCI code move to drivers/firmware,
> > > right ?
> > 
> > No, that's not something I've particularly looked at.  PSCI doesn't really
> > interest me because I have no systems which (afaik) support it.
> 
> PSCI implementation is currently part of arch/arm which means it is
> used on arch/arm (and related platforms) and you should be interested in it
> for that specific reason.
> 
> It is not about what you are interested on, it is about the code you
> maintain, so please have a look at it, thank you.

I don't maintain it, ARM Ltd does.  ARM Ltd submitted it.  ARM Ltd does
the testing of it.  I'm only someone who passes patches through when
necessary.

"Maintaining" a chunk of code when you have no way to test it is no way
to do maintanence.

If ARM Ltd want me to maintain it, provide me with the tools and knowledge
to do so.

> > > > We don't do this for other stuff - we don't have IRQ_CHIP_OF_DECLARE
> > > > stuff in arch/arm but have the IRQ chip drivers in drivers/irqchip.
> > > > We keep it all togehter in drivers/irqchip, even when the IRQ chip
> > > > driver is only useful on ARM.
> > > 
> > > CPUidle operations are ARM only, they are not used on ARM64, so
> > > they belong in arch/arm (that's the same thing as SMP ops, on ARM64
> > > SMP ops and CPUidle ops are unified through CPU operations).
> > 
> > I don't agree with that.  CPU idle isn't an "ARM thing" at all, it's
> > generic kernel infrastructure.  OF is generic kernel infrastructure too.
> 
> I said "CPUidle operations", not CPUidle, we know CPUidle is not an
> ARM thing.
> 
> > Yet, we're stuffing _all_ the PSCI CPU idle code into
> > drivers/firmware/psci.c, but then stuffing the PSCI OF data structures
> > into arch/arm.  This is utterly _insane_.
> 
> Ok, so we will copy the ARM64 PSCI idle related code to arch/arm
> and we are done with this, or we will have to ifdef drivers/firmware
> code, take your pick.

What the fsck is up with you.  You're talking utter nonsense.

> > There is nothing ARM specific about these CPU idle ops.  They don't
> > belong on arch/arm.
> 
> See above.
> 
> > NAK on this series (and the move of the PSCI code to drivers/firmware)
> 
> I do not accept that. You may NAK this series but you can't object to
> moving PSCI firmware code to drivers/firmware for that reason, so
> please have a look at Mark's patches again and ACK/NAK them for
> a valid reason, it has been a while since he asked.

Sorry, NAK, and end of discussion.  There is nothing more to be said
here.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1184811 — Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend

On Tue, Jul 14, 2015 at 09:41:38PM +0100, Russell King - ARM Linux wrote:

[...]

> > > Yet, we're stuffing _all_ the PSCI CPU idle code into
> > > drivers/firmware/psci.c, but then stuffing the PSCI OF data structures
> > > into arch/arm.  This is utterly _insane_.
> > 
> > Ok, so we will copy the ARM64 PSCI idle related code to arch/arm
> > and we are done with this, or we will have to ifdef drivers/firmware
> > code, take your pick.
> 
> What the fsck is up with you.  You're talking utter nonsense.
> 
> > > There is nothing ARM specific about these CPU idle ops.  They don't
> > > belong on arch/arm.
> > 
> > See above.
> > 
> > > NAK on this series (and the move of the PSCI code to drivers/firmware)
> > 
> > I do not accept that. You may NAK this series but you can't object to
> > moving PSCI firmware code to drivers/firmware for that reason, so
> > please have a look at Mark's patches again and ACK/NAK them for
> > a valid reason, it has been a while since he asked.
> 
> Sorry, NAK, and end of discussion.  There is nothing more to be said
> here.

I beg to differ. To solve the issue that you brought up with this series,
we can create an arch function that allows to set CPUidle operations, which
would remove the need for the OF construct you did not like, this mirrors
what's done for PSCI smp operations (something similar to smp_set_ops),
does that sound a reasonable approach to you ?

It is not an issue related to CPUidle only, other PSCI functions
(eg psci_cpu_{die/kill} arch/arm/kernel/psci_smp.c) can be shared between
ARM/ARM64 - so moved to drivers/firmware), we would end up with SMP
operations that are initialized with functions that live in
drivers/firmware, if it is done for SMP ops I do not see why it
can't be done for CPUidle operations.

Is the problem related to renaming psci_smp.c to psci.c and adding
CPU_IDLE and SMP ifdeffery in there - as it is done on arm64:

arch/arm64/kernel/psci.c

?

Please let us know, I think we can easily find a way that is acceptable
to you.

As for M.Rutland's series[1], and the patch that moves common PSCI code to
drivers/firmware I reiterate my point, please review it[1] and provide us
with feedback if any otherwise acknowledge the code move, which is the
basis on top of which everything else can be developed, I do not understand
why you are stopping [1] from getting into the kernel since it moves
code from arch/arm to drivers/firmware to have it in a common and shared
place between ARM and ARM64, what's the issue you have with that or put
it differently why are you NAKing it ?

Thank you,
Lorenzo

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/355643.html 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1184845 — Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend

On Wed, Jul 15, 2015 at 02:46:03PM +0100, Lorenzo Pieralisi wrote:
> On Tue, Jul 14, 2015 at 09:41:38PM +0100, Russell King - ARM Linux wrote:
> > Sorry, NAK, and end of discussion.  There is nothing more to be said
> > here.
> 
> I beg to differ. To solve the issue that you brought up with this series,
> we can create an arch function that allows to set CPUidle operations, which
> would remove the need for the OF construct you did not like, this mirrors
> what's done for PSCI smp operations (something similar to smp_set_ops),
> does that sound a reasonable approach to you ?

What's the point of that?

This is _all_ that arch/arm/kernel/psci_cpuidle.c will contain after this
series:

+#include <linux/cpuidle.h>
+#include <linux/psci.h>
+
+#include <asm/cpuidle.h>
+
+static struct cpuidle_ops psci_cpuidle_ops __initdata = {
+       .suspend = cpu_psci_cpu_suspend,
+       .init = cpu_psci_cpu_init_idle,
+};
+CPUIDLE_METHOD_OF_DECLARE(psci_cpuidle, "psci", &psci_cpuidle_ops);

There is _nothing_ ARM specific there.  All it's doing is providing an
entry in the DT linker-built table for that data structure, and that
data structure contains a pair of function pointers to code which _will_
be located in drivers/firmware/psci.c.

I'm saying _that_ is totally pointless, that data structure and
CPUIDLE_METHOD_OF_DECLARE() _should_ live with the code in
drivers/firmware/psci.c.

There's nothing in the above quoted code which points anything to any
32bit ARM specific stuff at all (so there's no need for a smp_set_ops()
type thing), it's all about giving DT a way to specify the CPU idle
operations which should be used, in this case, allowing DT to specify
that the _generic_ PSCI CPU idle operations are to be used.

> As for M.Rutland's series[1], and the patch that moves common PSCI code to
> drivers/firmware I reiterate my point, please review it[1] and provide us
> with feedback if any otherwise acknowledge the code move, which is the
> basis on top of which everything else can be developed, I do not understand
> why you are stopping [1] from getting into the kernel since it moves
> code from arch/arm to drivers/firmware to have it in a common and shared
> place between ARM and ARM64, what's the issue you have with that or put
> it differently why are you NAKing it ?

I'm NAKing everything related to moving the PSCI code around until
someone in the PSCI maintainer pool (that's not me - that's ARM Ltd)
clearly demonstrates that they know what they're talking about, and
has a plan behind this reorganisation.

Right now, I'm getting the impression that there is _no_ plan, or if
there is, it's an absurd plan which results in data structures which
should not be in arch/arm being left behind there.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web