[PATCH RESEND] ARM: l2x0: add three special L210 aux control flags

[PATCH RESEND] ARM: l2x0: add three special L210 aux control flags

[Linus Walleij]

This adds support for setting the flag bits "wrap access disable",
"write allocate override" and "exclusive abort disable" to the L210
variant from the device tree.

The "write allocate override" bit was used on the Nomadik and
necessary to properly make use of the L210 cache on that machine
without sporadic crashes.

I took this oppportunity to add all three flags so we avoid excess
patching for each one.

Cc: Rob Herring <robh+dt@kernel.org>
Cc: Pawel Moll <pawel.moll@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>
Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
Cc: Kumar Gala <galak@codeaurora.org>
Cc: Russell King <linux@arm.linux.org.uk>
Cc: devicetree at vger.kernel.org
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
---
 Documentation/devicetree/bindings/arm/l2c2x0.txt | 10 ++++++++++
 arch/arm/mm/cache-l2x0.c                         | 18 ++++++++++++++++++
 2 files changed, 28 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/l2c2x0.txt b/Documentation/devicetree/bindings/arm/l2c2x0.txt
index fe0398c5c77b..975d1ae304d4 100644
--- a/Documentation/devicetree/bindings/arm/l2c2x0.txt
+++ b/Documentation/devicetree/bindings/arm/l2c2x0.txt
@@ -74,6 +74,16 @@ Optional properties:
   or "write through no write allocate" (for writes).
   On systems where this may cause DMA buffer corruption, this property must be
   specified to indicate that such transforms are precluded.
+- arm,wrap-access-disable : (L210 only) if this flag is set, wrap accesses
+  requested on slave ports are converted to linear acesses on master ports.
+- arm,write-allocate-override : (L210 only) normally the L210 will use HPROT
+  attributes, setting this bit overrides that behaviour and cause the cache
+  to make all write-through and write-back accesses into read-write-allocate
+  accesses.
+- arm,exclusive-abort-disable : (L210 only) normally the cache controller sends
+  an ERROR response back to exclusive access in a cacheable, shared memory
+  region with the shared override bit set, but setting this flag disables the
+  abort generation and the access is instead treated as cacheable, non-shared.
 - arm,parity-enable : enable parity checking on the L2 cache (L220 or PL310).
 - arm,parity-disable : disable parity checking on the L2 cache (L220 or PL310).
 - arm,outer-sync-disable : disable the outer sync operation on the L2 cache.
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 9f9d54271aad..301c3c2dc09f 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -1076,6 +1076,24 @@ static void __init l2x0_of_parse(const struct device_node *np,
 		val |= L2C_AUX_CTRL_SHARED_OVERRIDE;
 	}
 
+	/* L210-specific aux control flags */
+	if (of_device_is_compatible(np, "arm,l210-cache")) {
+		if (of_property_read_bool(np, "arm,wrap-access-disable")) {
+			mask &= ~L210_AUX_CTRL_WRAP_DISABLE;
+			val |= L210_AUX_CTRL_WRAP_DISABLE;
+		}
+
+		if (of_property_read_bool(np, "arm,write-allocate-override")) {
+			mask &= ~L210_AUX_CTRL_WA_OVERRIDE;
+			val |= L210_AUX_CTRL_WA_OVERRIDE;
+		}
+
+		if (of_property_read_bool(np, "arm,exclusive-abort-disable")) {
+			mask &= ~L210_AUX_CTRL_EXCLUSIVE_ABORT;
+			val |= L210_AUX_CTRL_EXCLUSIVE_ABORT;
+		}
+	}
+
 	ret = l2x0_cache_size_of_parse(np, aux_val, aux_mask, &assoc, SZ_256K);
 	if (ret)
 		return;
-- 
2.4.3

[PATCH RESEND] ARM: l2x0: add three special L210 aux control flags

[Rob Herring]

On Tue, Mar 15, 2016 at 01:11:22PM +0100, Linus Walleij wrote:
> This adds support for setting the flag bits "wrap access disable",
> "write allocate override" and "exclusive abort disable" to the L210
> variant from the device tree.
> 
> The "write allocate override" bit was used on the Nomadik and
> necessary to properly make use of the L210 cache on that machine
> without sporadic crashes.
> 
> I took this oppportunity to add all three flags so we avoid excess
> patching for each one.
> 
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: Kumar Gala <galak@codeaurora.org>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: devicetree at vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> ---
>  Documentation/devicetree/bindings/arm/l2c2x0.txt | 10 ++++++++++
>  arch/arm/mm/cache-l2x0.c                         | 18 ++++++++++++++++++
>  2 files changed, 28 insertions(+)

Acked-by: Rob Herring <robh@kernel.org>

[PATCH RESEND] ARM: l2x0: add three special L210 aux control flags

[Linus Walleij]

On Tue, Mar 15, 2016 at 1:11 PM, Linus Walleij <linus.walleij@linaro.org> wrote:

> This adds support for setting the flag bits "wrap access disable",
> "write allocate override" and "exclusive abort disable" to the L210
> variant from the device tree.
>
> The "write allocate override" bit was used on the Nomadik and
> necessary to properly make use of the L210 cache on that machine
> without sporadic crashes.
>
> I took this oppportunity to add all three flags so we avoid excess
> patching for each one.
>
> Cc: Rob Herring <robh+dt@kernel.org>
> Cc: Pawel Moll <pawel.moll@arm.com>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> Cc: Kumar Gala <galak@codeaurora.org>
> Cc: Russell King <linux@arm.linux.org.uk>
> Cc: devicetree at vger.kernel.org
> Signed-off-by: Linus Walleij <linus.walleij@linaro.org>

Russell, this patch is in the patch tracker with Rob's ACK from
the DT side, do you think you could apply it?

For Nomadik I actually only need the write-allocate-override
flag to be selectable, so if you think it is increasing complexity
too much I'd be happy to make a diet version which only
adds this flag.

Yours,
Linus Walleij

[PATCH RESEND] ARM: l2x0: add three special L210 aux control flags

[Josh Cartwright]

On Sun, Apr 03, 2016 at 06:22:44PM +0200, Linus Walleij wrote:
> On Tue, Mar 15, 2016 at 1:11 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> > This adds support for setting the flag bits "wrap access disable",
> > "write allocate override" and "exclusive abort disable" to the L210
> > variant from the device tree.
> >
> > The "write allocate override" bit was used on the Nomadik and
> > necessary to properly make use of the L210 cache on that machine
> > without sporadic crashes.
> >
> > I took this oppportunity to add all three flags so we avoid excess
> > patching for each one.
> >
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Pawel Moll <pawel.moll@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> > Cc: Kumar Gala <galak@codeaurora.org>
> > Cc: Russell King <linux@arm.linux.org.uk>
> > Cc: devicetree at vger.kernel.org
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Russell, this patch is in the patch tracker with Rob's ACK from
> the DT side, do you think you could apply it?

Similarly, we're waiting on feedback for:

  http://lkml.kernel.org/r/1456761716-10174-1-git-send-email-brad.mouring at ni.com

Also in the queue; also w/ Rob's Ack.  There was a change awhile back to
unconditionally enable the power-management features of the PL310 which
caused a noticeable performance degradation on our boards; placing the
PM options behind a togglable DT property provides us an out.

  Josh

PS. Sorry for the threadjack, but it seemed similar enough.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 473 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160404/3f2a0f30/attachment-0001.sig>

[PATCH RESEND] ARM: l2x0: add three special L210 aux control flags

[Russell King - ARM Linux]

On Mon, Apr 04, 2016 at 06:29:26PM -0500, Josh Cartwright wrote:
> Similarly, we're waiting on feedback for:
> 
>   http://lkml.kernel.org/r/1456761716-10174-1-git-send-email-brad.mouring at ni.com
> 
> Also in the queue; also w/ Rob's Ack.  There was a change awhile back to
> unconditionally enable the power-management features of the PL310 which
> caused a noticeable performance degradation on our boards; placing the
> PM options behind a togglable DT property provides us an out.

For that one, I've been wondering why it's seemingly acceptable to
start throwing *errors* for *new* DT properties which weren't required
before.  Also, there's no DT documentation for the new properties,
which is a fundamental requirement - and Rob should not have given
his ack without there being a DT documentation patch.

Obviously, the kernel review process has broken on this one... or
I must remember this sneaky trick for getting new DT properties in
without the required documentation...

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH RESEND] ARM: l2x0: add three special L210 aux control flags

[Russell King - ARM Linux]

On Sun, Apr 03, 2016 at 06:22:44PM +0200, Linus Walleij wrote:
> On Tue, Mar 15, 2016 at 1:11 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> 
> > This adds support for setting the flag bits "wrap access disable",
> > "write allocate override" and "exclusive abort disable" to the L210
> > variant from the device tree.
> >
> > The "write allocate override" bit was used on the Nomadik and
> > necessary to properly make use of the L210 cache on that machine
> > without sporadic crashes.
> >
> > I took this oppportunity to add all three flags so we avoid excess
> > patching for each one.
> >
> > Cc: Rob Herring <robh+dt@kernel.org>
> > Cc: Pawel Moll <pawel.moll@arm.com>
> > Cc: Mark Rutland <mark.rutland@arm.com>
> > Cc: Ian Campbell <ijc+devicetree@hellion.org.uk>
> > Cc: Kumar Gala <galak@codeaurora.org>
> > Cc: Russell King <linux@arm.linux.org.uk>
> > Cc: devicetree at vger.kernel.org
> > Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
> 
> Russell, this patch is in the patch tracker with Rob's ACK from
> the DT side, do you think you could apply it?
> 
> For Nomadik I actually only need the write-allocate-override
> flag to be selectable, so if you think it is increasing complexity
> too much I'd be happy to make a diet version which only
> adds this flag.

I haven't forgotten it, but I do wonder whether this endless shovelling
of more and more DT properties is really the way forward, or whether
some of this stuff should be handled by the boot loader/firmware.

That's the reason that you get the:

"L2C: DT/platform modifies aux control register: 0x%08x -> 0x%08x"

message at _warning_ level.

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH RESEND] ARM: l2x0: add three special L210 aux control flags

[Linus Walleij]

On Tue, Apr 5, 2016 at 9:43 AM, Russell King - ARM Linux
<linux@arm.linux.org.uk> wrote:

> I haven't forgotten it, but I do wonder whether this endless shovelling
> of more and more DT properties is really the way forward, or whether
> some of this stuff should be handled by the boot loader/firmware.
>
> That's the reason that you get the:
>
> "L2C: DT/platform modifies aux control register: 0x%08x -> 0x%08x"
>
> message at _warning_ level.

I see your concern.

Unfortunately it's one of those bootloaders that I cannot replace.
There is nothing in the way of official support for it, and
you have to use a custom protocol to replace the boot loader,
so if I start to experiment with it I may end up bricking my
device beyond recovery :(

Yours,
Linus Walleij

[PATCH RESEND] ARM: l2x0: add three special L210 aux control flags

[Russell King - ARM Linux]

On Tue, Apr 05, 2016 at 12:36:40AM +0100, Russell King - ARM Linux wrote:
> On Mon, Apr 04, 2016 at 06:29:26PM -0500, Josh Cartwright wrote:
> > Similarly, we're waiting on feedback for:
> > 
> >   http://lkml.kernel.org/r/1456761716-10174-1-git-send-email-brad.mouring at ni.com
> > 
> > Also in the queue; also w/ Rob's Ack.  There was a change awhile back to
> > unconditionally enable the power-management features of the PL310 which
> > caused a noticeable performance degradation on our boards; placing the
> > PM options behind a togglable DT property provides us an out.
> 
> For that one, I've been wondering why it's seemingly acceptable to
> start throwing *errors* for *new* DT properties which weren't required
> before.  Also, there's no DT documentation for the new properties,
> which is a fundamental requirement - and Rob should not have given
> his ack without there being a DT documentation patch.
> 
> Obviously, the kernel review process has broken on this one... or
> I must remember this sneaky trick for getting new DT properties in
> without the required documentation...

There's another issue here - we go from enabling these options by
default to leaving them as the firmware specified them.  While that's
something I approve of, I'm not happy because it means all platforms
that have these PM bits enabled today could end up losing them - so
it's a PM regression waiting to happen.  Who's going to add them to
all the dt blobs where platforms _can_ cope with having these PM bits
enabled?

-- 
RMK's Patch system: http://www.arm.linux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH RESEND] ARM: l2x0: add three special L210 aux control flags

[Josh Cartwright]

On Tue, Apr 05, 2016 at 12:36:40AM +0100, Russell King - ARM Linux wrote:
> On Mon, Apr 04, 2016 at 06:29:26PM -0500, Josh Cartwright wrote:
> > Similarly, we're waiting on feedback for:
> > 
> >   http://lkml.kernel.org/r/1456761716-10174-1-git-send-email-brad.mouring at ni.com
> > 
> > Also in the queue; also w/ Rob's Ack.  There was a change awhile back to
> > unconditionally enable the power-management features of the PL310 which
> > caused a noticeable performance degradation on our boards; placing the
> > PM options behind a togglable DT property provides us an out.
> 
> For that one, I've been wondering why it's seemingly acceptable to
> start throwing *errors* for *new* DT properties which weren't required
> before.

Yes, this doesn't sound appropriate and should be revisited.

> Also, there's no DT documentation for the new properties, which is a
> fundamental requirement - and Rob should not have given his ack
> without there being a DT documentation patch.

Yeah, I partly mispoke here.  The patch I linked to is the first of a
two part series, the DT bindings document is the second patch, which had
Rob's ack (not the one I linked).

However, it looks like what made it into your patch tracker was a
squashed version of both patches, which is probably not appropriate.
It's probably best we drop what's in the patch queue, and we can get a
new rev sent out addressing your concerns.

Thanks,
  Josh

[PATCH RESEND] ARM: l2x0: add three special L210 aux control flags

[Josh Cartwright]

On Tue, Apr 05, 2016 at 09:08:27AM +0100, Russell King - ARM Linux wrote:
> On Tue, Apr 05, 2016 at 12:36:40AM +0100, Russell King - ARM Linux wrote:
> > On Mon, Apr 04, 2016 at 06:29:26PM -0500, Josh Cartwright wrote:
> > > Similarly, we're waiting on feedback for:
> > > 
> > >   http://lkml.kernel.org/r/1456761716-10174-1-git-send-email-brad.mouring at ni.com
> > > 
> > > Also in the queue; also w/ Rob's Ack.  There was a change awhile back to
> > > unconditionally enable the power-management features of the PL310 which
> > > caused a noticeable performance degradation on our boards; placing the
> > > PM options behind a togglable DT property provides us an out.
> > 
> > For that one, I've been wondering why it's seemingly acceptable to
> > start throwing *errors* for *new* DT properties which weren't required
> > before.  Also, there's no DT documentation for the new properties,
> > which is a fundamental requirement - and Rob should not have given
> > his ack without there being a DT documentation patch.
> > 
> > Obviously, the kernel review process has broken on this one... or
> > I must remember this sneaky trick for getting new DT properties in
> > without the required documentation...
> 
> There's another issue here - we go from enabling these options by
> default to leaving them as the firmware specified them.  While that's
> something I approve of, I'm not happy because it means all platforms
> that have these PM bits enabled today could end up losing them - so
> it's a PM regression waiting to happen.  Who's going to add them to
> all the dt blobs where platforms _can_ cope with having these PM bits
> enabled?

Viewing the pending patch as causing a PM regression is one way to look
at it, however another way to look at is is that the commit which
unconditionally enabled the PM features caused a performance regression;
in particular commit 3a43b581dac1 ("ARM: l2c: always enable low power
modes") which landed in v3.16.  (Perhaps, though, the distinction isn't
important, as it doesn't necessarily help dictate a course of action.)

So, I think we could do several things:

  1. Keep proposed opt-in power mode DT enablement.  Enumerate platforms
     which currently benefit from the existing auto-enabling, opt-in on
     those platforms in their upstream device trees

  2. Change proposed patch to opt-out of power modes in DT,
     "arm,no-standby-mode", "arm,no-dynamic-clock-gating"

Option #1 has a few benefits; the flags which dictate power mode control
are consistent with the other mode flags, it prevents the performance
regression.  However, it isn't clear how to go about doing this
enumeration, especially given the few ARM boards we have available for
testing :(.

Option #2 means that the existing performance regression will exist for
users across a 3.16 upgrade, however, the PM regression is still
avoided.  The mode control flags are inconsistent.

I'm assuming that, because #1 is effectively what you had suggested,
you'd consider that to be how we should proceed?

Thanks for the feedback.

  Josh

[PATCH RESEND] ARM: l2x0: add three special L210 aux control flags

[Brad Mouring]

On Tue, Apr 05, 2016 at 08:12:54AM -0500, Josh Cartwright wrote:
> On Tue, Apr 05, 2016 at 09:08:27AM +0100, Russell King - ARM Linux wrote:
> > On Tue, Apr 05, 2016 at 12:36:40AM +0100, Russell King - ARM Linux wrote:
> > > On Mon, Apr 04, 2016 at 06:29:26PM -0500, Josh Cartwright wrote:
> > > > Similarly, we're waiting on feedback for:
> > > > 
> > > >   http://lkml.kernel.org/r/1456761716-10174-1-git-send-email-brad.mouring at ni.com
> > > > 
> > > > Also in the queue; also w/ Rob's Ack.  There was a change awhile back to
> > > > unconditionally enable the power-management features of the PL310 which
> > > > caused a noticeable performance degradation on our boards; placing the
> > > > PM options behind a togglable DT property provides us an out.
> > > 
> > > For that one, I've been wondering why it's seemingly acceptable to
> > > start throwing *errors* for *new* DT properties which weren't required
> > > before.  Also, there's no DT documentation for the new properties,
> > > which is a fundamental requirement - and Rob should not have given
> > > his ack without there being a DT documentation patch.

2/2 Is the documentation. The patch that sits in the queue is the combination
of those two, including the documentation since the help of the patch queue
recommends submitting a single feature as a single patch

Queued patch: http://www.arm.linux.org.uk/developer/patches/viewpatch.php?id=8549/1

> > > Obviously, the kernel review process has broken on this one... or
> > > I must remember this sneaky trick for getting new DT properties in
> > > without the required documentation...
> > 
> > There's another issue here - we go from enabling these options by
> > default to leaving them as the firmware specified them.  While that's
> > something I approve of, I'm not happy because it means all platforms
> > that have these PM bits enabled today could end up losing them - so
> > it's a PM regression waiting to happen.  Who's going to add them to
> > all the dt blobs where platforms _can_ cope with having these PM bits
> > enabled?
> 
> Viewing the pending patch as causing a PM regression is one way to look
> at it, however another way to look at is is that the commit which
> unconditionally enabled the PM features caused a performance regression;
> in particular commit 3a43b581dac1 ("ARM: l2c: always enable low power
> modes") which landed in v3.16.  (Perhaps, though, the distinction isn't
> important, as it doesn't necessarily help dictate a course of action.)

I initially wrote the patch maintaining the "on-by-default" behavior. So
many of the other DT-controlled features, however, take the approach of
defaulting to the firmware's configuration, it seems odd and out-of-place
to suddenly impose a default setting.

Of course, I am open to feedback, and thanks for giving this some attention
Russell.

Brad