On Thu, Feb 05, 2015 at 12:20:48PM +0000, Mark Rutland wrote:
> [Adding dtc folk]
>
> On Thu, Feb 05, 2015 at 12:09:20PM +0000, Will Deacon wrote:
> > On Thu, Feb 05, 2015 at 11:59:33AM +0000, Mark Rutland wrote:
> > > On Thu, Feb 05, 2015 at 11:54:16AM +0000, Will Deacon wrote:
> > > > On Thu, Feb 05, 2015 at 11:46:42AM +0000, Mark Rutland wrote:
> > > > > On Mon, Jan 26, 2015 at 05:54:15PM +0000, Will Deacon wrote:
> > > > > > diff --git a/arch/arm64/boot/dts/arm/juno.dts b/arch/arm64/boot/dts/arm/juno.dts
> > > > > > index cb3073e4e7a8..4ed9287aaef1 100644
> > > > > > --- a/arch/arm64/boot/dts/arm/juno.dts
> > > > > > +++ b/arch/arm64/boot/dts/arm/juno.dts
> > > > > > @@ -107,11 +107,11 @@
> > > > > > pmu {
> > > > > > compatible = "arm,armv8-pmuv3";
> > > > > > interrupts = <GIC_SPI 18 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > + <GIC_SPI 02 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > + <GIC_SPI 06 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 22 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > <GIC_SPI 26 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > - <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > - <GIC_SPI 02 IRQ_TYPE_LEVEL_HIGH>,
> > > > > > - <GIC_SPI 06 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > + <GIC_SPI 30 IRQ_TYPE_LEVEL_HIGH>;
> > > > > > };
> > > > >
> > > > > I am very much not keen on this. While this may get things working
> > > > > today, it completely relies on Linux-internal details (the order of CPU
> > > > > bringup, which in this case is different from the order of entries in
> > > > > /cpus).
> > > > >
> > > > > In all other dts that I am aware of, the order of entries in /cpus
> > > > > aligns with the order of interrupts in the PMU node, and the first entry
> > > > > is the boot CPU.
> > > > >
> > > > > I think that we should ensure that the ordering of CPU nodes matches the
> > > > > order of interrupts here. That way we can fall back to that ordering (if
> > > > > not explicitly overridden), and even after an arbitrary logical
> > > > > renumbering (e.g. after a kexec) the relationship should stay intact.
> > > >
> > > > There are a few problems with reordering the CPU nodes:
> > > >
> > > > (1) It breaks any existing users of taskset to pin on big/little
> > > > clusters.
> > >
> > > This is unfortunate, but this is also the case if the boot CPU is
> > > different.
> >
> > Right, so don't change the boot CPU. In that vain, we also shouldn't change
> > the CPU order in the .dts -- the current .dts is working for taskset and
> > we shouldn't break people's scripts just because they want to use the PMU.
>
> I think this is an orthogonal discussion. If Linux is booted on a
> different CPU, it's not the fault of Linux that CPU0 is different.
>
> > > > (2) It's not generally possible if, for example, the bootloader decides
> > > > to boot Linux on a different CPU then we have no choice but to
> > > > change the PMU interrupt order.
> > >
> > > In that case _this_ patch is broken.
> >
> > Why? I'm not denying that changing the boot CPU causes problems, I'm saying
> > that you *can't* fix that by changing the CPU node order. You still have
> > to change the interrupt order in that case, so why not just localise the
> > changes there in the first place?
>
> If we're going to try to maintain support for these DTs long-term (with
> kexec and whatever logical renumbering can occur there), then we need a
> consistent invariant that we can rely on to associate interrupts and
> CPUs correctly.
>
> The Linux logical ordering is not invariant, so we know that this _will_
> break.
>
> As far as I am aware, every other DT lists the boot CPU first, and the
> order of entries in /cpus mathes the logical order. Using the order of
> entries in /cpus will remain consistent in the face of arbitrary
> renumbering, and is (currently) consistent with logical numbering.
>
> So keeping the CPU nodes and interrupt entries in the same order
> provides us with a long-term consistent order, regardless of which CPU
> is the boot CPU.
>
> This DT is currently broken. If we're going to make it work we should do
> so in a manner that will continue to work. Anything else is a broken
> bodge that hurts us in the long-term as we'll have to hack around it.
>
> > > If we associate the interrupt with a CPU by node order, the relationship
> > > is preserved regardless of which CPU is the boot CPU (whether it was the
> > > bootloader's choice, kexec, or whatever).
> >
> > Sure, and that requires code changes. If we're going to change the code,
> > then I'd much rather we make the binding explicit, like I did in the
> > follow-up patches to this one. As I mentioned before, this is a .dts fix
> > to get things working with the current code. It's really too late to argue
> > about the existing binding, even if it sucks.
>
> Sure, the binding sucks.
>
> This DT has also _never_ worked.
>
> If we're going to fix things, let's not introduce a middle step that's
> broken in a different way.
>
> > > > (3) I didn't think that the ordering of CPU nodes was guaranteed to be
> > > > preserved by dtc, whereas the order of the interrupts will be.
> > >
> > > The order of nodes is presently preserved.
> >
> > It's not about the present behaviour; I need a _guarantee_ that dtc/libfdt
> > will *never* reorder CPU nodes. Today's working .dts file needs to continue
> > to work with future tools.
>
> Jon, David, Grant, thoughts?
As a general rule, neither dtc nor libfdt will re-order any nodes
unless you explicitly ask them to (e.g. dtc's "-s" option). That
said, you should try not to rely on dt order.
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
Attachment:
pgplBDBNHBFIU.pgp
Description: PGP signature
