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

Groups > linux.kernel > #1192836 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci  backend Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2015-07-27 11:20 +0200
    Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci  backend Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-07-27 11:50 +0200
      Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci  backend Lorenzo Pieralisi <lorenzo.pieralisi@arm.com> - 2015-07-27 12:10 +0200
        Re: [PATCH v3 3/3] arm: kernel: implement cpuidle_ops with psci  backend Russell King - ARM Linux <linux@arm.linux.org.uk> - 2015-07-27 12:20 +0200

#1192836 — 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 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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1192889

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1192918

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1192927

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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web