| Message ID | 1436430685-2202-4-git-send-email-jszhang@marvell.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jul 09, 2015 at 04:31:25PM +0800, Jisheng Zhang wrote: > 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); Is there any reason this can't live in the drivers sub-tree? Is there anything specific to 32-bit ARM about this? It looks to me like the right thing to do is to have it as part of drivers/firmware/psci.c. 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.
On Tue, Jul 14, 2015 at 11:34:21AM +0100, Russell King - ARM Linux wrote: > On Thu, Jul 09, 2015 at 04:31:25PM +0800, Jisheng Zhang wrote: > > 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); > > Is there any reason this can't live in the drivers sub-tree? Is there > anything specific to 32-bit ARM about this? > > It looks to me like the right thing to do is to have it as part of > drivers/firmware/psci.c. I take this as an ACK to M.Rutland's PSCI code move to drivers/firmware, right ? http://lists.infradead.org/pipermail/linux-arm-kernel/2015-July/355643.html > 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). Thanks, Lorenzo
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. > > 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. 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_. There is nothing ARM specific about these CPU idle ops. They don't belong on arch/arm. NAK on this series (and the move of the PSCI code to drivers/firmware) 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. Find a solution which doesn't involve leaving _just_ data structures to connect stuff to OF in arch/arm.
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
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.
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
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.
On Wed, Jul 15, 2015 at 03:45:07PM +0100, Russell King - ARM Linux wrote: > 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. No it is not. struct cpuidle_ops (as struct smp_operations) is ARM specific. You can't add that to generic code (unless guarded by CONFIG_ARM). You can't move that struct declaration above to drivers/firmware for the same reason you can't move CPU_METHOD_OF_DECLARE code (that is used as a mechanism to initialize SMP ops on ARM) to drivers/firmware. On ARM64 this is done differently. On ARM64 SMP operations and CPUidle operations are merged into struct cpu_operations, that is initialized through DT at boot (it does not use linker script mechanism because all enable-method are statically defined in the kernel in an array to avoid SMP methods du jour proliferation - see arch/arm64/kernel/cpu_ops.c and the supported_cpu_ops array). Now, we have to initialize cpuidle_ops on ARM (and SMP ops too) if those operations are implemented with PSCI. For SMP operations, this is done in arch/arm/kernel/setup.c through smp_set_ops. What I was saying is that we can do the same, without resorting to the linker script based approch above for CPUidle operations. We can't move: 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); to drivers/firmware unless it is guarded by CONFIG_ARM, that's what I am saying, is that clear ? How do you want us to do it ? > > 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. M.Rutland's patch series represents the code reorganisation and as far as I am concerned that's a complete and well defined plan, I still do not understand why you are NAKing that piece of code, it is completely orthogonal to what we are debating above, please have a look at it otherwise we are going around in circles here. > 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. Mark's series does not leave any data structure behind, it is moving code to a common place in the kernel so that the _current_ PSCI implementation can be shared between ARM and ARM64, again please have a look at it, we can tackle how to define cpuidle_ops in a way that you deem reasonable later, it is a separate issue, please understand. Thank you, Lorenzo
On Wed, Jul 15, 2015 at 04:40:56PM +0100, Lorenzo Pieralisi 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); > > to drivers/firmware unless it is guarded by CONFIG_ARM, that's what > I am saying, is that clear ? I'd appreciate it if you didn't go bitching to Philippe, who then phones me to discuss this problem. That's not the way "business" is done. You definitely _can_ move this into drivers/. There's absolutely nothing stopping that. Yes, CPUIDLE_METHOD_OF_DECLARE() may be an ARM-only thing, that doesn't stop drivers using it - and the hint there is in the name. Drivers. They belong in the drivers/ subdirectory, not the arch/ subdirectory. What's so difficult to get about that? All two _existing_ users of CPUIDLE_METHOD_OF_DECLARE() are in drivers/ already: $ git grep CPUIDLE_METHOD_OF_DECLARE arch/arm/include/asm/cpuidle.h:#define CPUIDLE_METHOD_OF_DECLARE(name, _method,_ops) \ drivers/soc/qcom/spm.c:CPUIDLE_METHOD_OF_DECLARE(qcom_idle_v1, "qcom,kpss-acc-v1", &qcom_cpuidle_ops); drivers/soc/qcom/spm.c:CPUIDLE_METHOD_OF_DECLARE(qcom_idle_v2, "qcom,kpss-acc-v2", &qcom_cpuidle_ops); So there's absolutely no argument you can possibly make about "it's in asm/ so code using it must be in arch/arm." > How do you want us to do it ? I want this under drivers/, like I've said already several times in this thread. > M.Rutland's patch series represents the code reorganisation and as far > as I am concerned that's a complete and well defined plan, I still do > not understand why you are NAKing that piece of code, it is completely > orthogonal to what we are debating above, please have a look at it > otherwise we are going around in circles here. It's not a well-defined plan if it involves dispersing related code or data structures across the kernel tree, especially when there is very little reason for it to be dispersed. > > 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. > > Mark's series does not leave any data structure behind, it is moving > code to a common place in the kernel so that the _current_ PSCI > implementation can be shared between ARM and ARM64, again please > have a look at it, we can tackle how to define cpuidle_ops in a way > that you deem reasonable later, it is a separate issue, please > understand. You misunderstand me. Yes, Mark's series _on its own_ doesn't leave it behind, but it _has the effect_ that when combined with subsequent patches, it _causes_ that to happen. Look, there's a simple solution to this: if the plan is to move CPU idle PSCI support into drivers/ then lets move the whole thing into drivers. That's a sane plan. What isn't a sane plan is to mvoe all the CPU idle PSCI code into drivers/ and then have a PSCI data structure left in arch/arm/. It can live in a separate file which is only built for ARM, rather than having ifdefs surround it, but the important thing is that it is localised with the rest of the code, so as changes happen, it gets noticed. No one in years to come will remember to look in arch/arm/ when making changes to the PSCI CPU idle code. This is no different to what drivers/soc/qcom/spm.c is doing. So. Sane plan: "let's move all the PSCI stuff into drivers/whatever/psci/". That's something I'm happy with. Insane plan: "let's move the PSCI code into drivers/whatever/psci/, let's keep PSCI data structures using that code in arch/arm". That plan (in its entirety) I'm NAKing, because it is _no_ plan. Lastly, I didn't realise that is an ARM only thing - that was merged without my involvement or ACK, the change wasn't even CC'd to me (so it's no wonder I know little about it.) I'd have NAKed that change too had I seen it, suggesting that the contents of it (which are used by drivers/soc/qcom/spm.c) should be located elsewhere - something which I've done in the past when people have tried to shove stuff into arch/arm/include/asm which gets used by stuff outside of arch/arm. Are we clear?
On Sun, Jul 26, 2015 at 10:45:54PM +0100, Russell King - ARM Linux wrote: > On Wed, Jul 15, 2015 at 04:40:56PM +0100, Lorenzo Pieralisi 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); > > > > to drivers/firmware unless it is guarded by CONFIG_ARM, that's what > > I am saying, is that clear ? > > I'd appreciate it if you didn't go bitching to Philippe, who then phones > me to discuss this problem. That's not the way "business" is done. > > You definitely _can_ move this into drivers/. There's absolutely nothing > stopping that. Yes, CPUIDLE_METHOD_OF_DECLARE() may be an ARM-only thing, > that doesn't stop drivers using it - and the hint there is in the name. > Drivers. They belong in the drivers/ subdirectory, not the arch/ > subdirectory. > > What's so difficult to get about that? > > All two _existing_ users of CPUIDLE_METHOD_OF_DECLARE() are in drivers/ > already: > > $ git grep CPUIDLE_METHOD_OF_DECLARE > arch/arm/include/asm/cpuidle.h:#define CPUIDLE_METHOD_OF_DECLARE(name, _method,_ops) \ > drivers/soc/qcom/spm.c:CPUIDLE_METHOD_OF_DECLARE(qcom_idle_v1, "qcom,kpss-acc-v1", &qcom_cpuidle_ops); > drivers/soc/qcom/spm.c:CPUIDLE_METHOD_OF_DECLARE(qcom_idle_v2, "qcom,kpss-acc-v2", &qcom_cpuidle_ops); > > So there's absolutely no argument you can possibly make about "it's in > asm/ so code using it must be in arch/arm." > > > How do you want us to do it ? > > I want this under drivers/, like I've said already several times in > this thread.o We will move it there. > > M.Rutland's patch series represents the code reorganisation and as far > > as I am concerned that's a complete and well defined plan, I still do > > not understand why you are NAKing that piece of code, it is completely > > orthogonal to what we are debating above, please have a look at it > > otherwise we are going around in circles here. > > It's not a well-defined plan if it involves dispersing related code or > data structures across the kernel tree, especially when there is very > little reason for it to be dispersed. > > > > 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. > > > > Mark's series does not leave any data structure behind, it is moving > > code to a common place in the kernel so that the _current_ PSCI > > implementation can be shared between ARM and ARM64, again please > > have a look at it, we can tackle how to define cpuidle_ops in a way > > that you deem reasonable later, it is a separate issue, please > > understand. > > You misunderstand me. Yes, Mark's series _on its own_ doesn't leave it > behind, but it _has the effect_ that when combined with subsequent > patches, it _causes_ that to happen. Let's forget the issues related to CPUidle, we have an agreement on the way forward, I will make sure it is implemented. I just wanted to say, please do not block Mark's series because of this patch, that series makes sense standalone and is a step in the right direction, I will make sure we implement the changes you requested in this thread on top of it if you allow it to get into the kernel. > Look, there's a simple solution to this: if the plan is to move CPU idle > PSCI support into drivers/ then lets move the whole thing into drivers. > That's a sane plan. What isn't a sane plan is to mvoe all the CPU idle > PSCI code into drivers/ and then have a PSCI data structure left in > arch/arm/. > > It can live in a separate file which is only built for ARM, rather than > having ifdefs surround it, but the important thing is that it is localised > with the rest of the code, so as changes happen, it gets noticed. No one > in years to come will remember to look in arch/arm/ when making changes to > the PSCI CPU idle code. > > This is no different to what drivers/soc/qcom/spm.c is doing. > > So. Sane plan: "let's move all the PSCI stuff into drivers/whatever/psci/". > That's something I'm happy with. I am ok with that too, so I think we are in agreement. > Insane plan: "let's move the PSCI code into drivers/whatever/psci/, let's > keep PSCI data structures using that code in arch/arm". That plan (in its > entirety) I'm NAKing, because it is _no_ plan. > > Lastly, I didn't realise that is an ARM only thing - that was merged > without my involvement or ACK, the change wasn't even CC'd to me (so > it's no wonder I know little about it.) I'd have NAKed that change too > had I seen it, suggesting that the contents of it (which are used by > drivers/soc/qcom/spm.c) should be located elsewhere - something which > I've done in the past when people have tried to shove stuff into > arch/arm/include/asm which gets used by stuff outside of arch/arm. > > Are we clear? Yes, I would only ask you, if the plan above (which can be implemented in two steps) makes sense to you please consider accepting Mark's change to consolidate PSCI code into drivers/firmware/psci, it is a stepping stone without which the changes above can't happen, I will take charge of completing the move of CPUidle code and create the required shim layer into drivers to make this happen. Are you ok with this ? Thanks !! Lorenzo
On Mon, Jul 27, 2015 at 10:16:02AM +0100, Lorenzo Pieralisi wrote: > Yes, I would only ask you, if the plan above (which can be implemented > in two steps) makes sense to you please consider accepting Mark's change > to consolidate PSCI code into drivers/firmware/psci, it is a stepping stone > without which the changes above can't happen, I will take charge of completing > the move of CPUidle code and create the required shim layer into > drivers to make this happen. Why can't Jisheng Zhang base his patches on top of Mark's changes and place the new file directly under drivers/ ? Why do it as a two-step process with it first appearing in arch/arm, and then having to generate another patch at a later date to move it elsewhere. That just creates more noise, and we should be avoiding generating noise in arch/arm. This is what Linus has said in his -rc4 release notes yesterday: Other than that issue, it's mostly drivers and networking. USB, gpu, mmc, network drivers, sound. With some ARM noise (but even that is mostly driver-related: dts updates due to MMC fixes). And a few small filesystem fixes. and we can infer from the phrase "ARM noise" that Linus' opinion of arch/arm is still fairly low, and still doesn't regard the "churn" in arch/arm as being useful.
On Mon, Jul 27, 2015 at 10:45:07AM +0100, Russell King - ARM Linux wrote: > On Mon, Jul 27, 2015 at 10:16:02AM +0100, Lorenzo Pieralisi wrote: > > Yes, I would only ask you, if the plan above (which can be implemented > > in two steps) makes sense to you please consider accepting Mark's change > > to consolidate PSCI code into drivers/firmware/psci, it is a stepping stone > > without which the changes above can't happen, I will take charge of completing > > the move of CPUidle code and create the required shim layer into > > drivers to make this happen. > > Why can't Jisheng Zhang base his patches on top of Mark's changes and > place the new file directly under drivers/ ? > > Why do it as a two-step process with it first appearing in arch/arm, > and then having to generate another patch at a later date to move it > elsewhere. That just creates more noise, and we should be avoiding > generating noise in arch/arm. Nothing will appear in arch/arm, I promise, I said two-step process because Mark's series is standalone/ready-to-be-merged while Jisheng patch series has still some bits to be debated (that won't affect what we discussed in relation to the code split and do not require any change in Mark's series at all). No code move, nothing in arch/arm, we will stick to the plan as I said before and I fully agree with that, please do not block one mature patch series for another one that still has some bits to be settled, and they are totally independent. > This is what Linus has said in his -rc4 release notes yesterday: > > Other than that issue, it's mostly drivers and networking. USB, gpu, > mmc, network drivers, sound. With some ARM noise (but even that is > mostly driver-related: dts updates due to MMC fixes). And a few small > filesystem fixes. > > and we can infer from the phrase "ARM noise" that Linus' opinion of > arch/arm is still fairly low, and still doesn't regard the "churn" in > arch/arm as being useful. Mark's series consolidate ARM/ARM64 PSCI implementations, it does not require creating anything in arch/arm actually it moves code in arch/arm to drivers/firmware, consolidating it, it is definitely the right thing to do in this respect. The CPUidle code comes as a second series on top of Mark's one and it will _not_ add anything in arch/arm (if you allow Mark to proceed), you have my word :) Does it sound ok to you ? Thanks ! Lorenzo
On Mon, Jul 27, 2015 at 11:01:03AM +0100, Lorenzo Pieralisi wrote: > Mark's series consolidate ARM/ARM64 PSCI implementations, it does not > require creating anything in arch/arm actually it moves code in arch/arm > to drivers/firmware, consolidating it, it is definitely the right > thing to do in this respect. > > The CPUidle code comes as a second series on top of Mark's one and it > will _not_ add anything in arch/arm (if you allow Mark to proceed), you > have my word :) > > Does it sound ok to you ? Yes. Please let Philippe know that there's no reason to have a phone call over this now.
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);
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