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

[Lorenzo Pieralisi]

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