Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend
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. -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend
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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend
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. -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend
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", _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", _cpuidle_ops);
> drivers/soc/qcom/spm.c:CPUIDLE_METHOD_OF_DECLARE(qcom_idle_v2,
> "qcom,kpss-acc-v2", _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,
Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend
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. -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend
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. -- 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 majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend
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 -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend
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
Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend
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", _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", _cpuidle_ops);
drivers/soc/qcom/spm.c:CPUIDLE_METHOD_OF_DECLARE(qcom_idle_v2,
"qcom,kpss-acc-v2", _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?
--
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend
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?
--
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend
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
> +#include
> +
> +#include
> +
> +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", _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", _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
--
To unsubscribe from
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
+#include
+
+#include
+
+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", _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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend
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
--
To unsubscribe from this list: send the line
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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", _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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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", _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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend
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", _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.
--
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend
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
> > ---
> > 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 000..e7146d2
> > --- /dev/null
> > +++ b/arch/arm/kernel/psci_cpuidle.c
> > @@ -0,0 +1,19 @@
> > +/*
> > + * Copyright (C) 2015 Marvell Technology Group Ltd.
> > + * Author: Jisheng Zhang
> > + *
> > + * 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
> > +#include
> > +
> > +#include
> > +
> > +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", _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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend
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
> ---
> 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 000..e7146d2
> --- /dev/null
> +++ b/arch/arm/kernel/psci_cpuidle.c
> @@ -0,0 +1,19 @@
> +/*
> + * Copyright (C) 2015 Marvell Technology Group Ltd.
> + * Author: Jisheng Zhang
> + *
> + * 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
> +#include
> +
> +#include
> +
> +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", _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.
--
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend
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 jszh...@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 000..e7146d2
--- /dev/null
+++ b/arch/arm/kernel/psci_cpuidle.c
@@ -0,0 +1,19 @@
+/*
+ * Copyright (C) 2015 Marvell Technology Group Ltd.
+ * Author: Jisheng Zhang jszh...@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
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend
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 jszh...@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 000..e7146d2
--- /dev/null
+++ b/arch/arm/kernel/psci_cpuidle.c
@@ -0,0 +1,19 @@
+/*
+ * Copyright (C) 2015 Marvell Technology Group Ltd.
+ * Author: Jisheng Zhang jszh...@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.
--
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci backend
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.
--
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
