[RFC PATCH 3/4] ARM: l2x0: add arm,ecc-enable property for aurora

[RFC PATCH 3/4] ARM: l2x0: add arm,ecc-enable property for aurora

[Chris Packham]

The aurora cache on the Marvell Armada-XP SoC supports ECC protection
for the L2 data arrays. Add a "arm,ecc-enable" device tree property
which can be used to enable this.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 Documentation/devicetree/bindings/arm/l2c2x0.txt | 2 ++
 arch/arm/mm/cache-l2x0.c                         | 7 +++++++
 2 files changed, 9 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/l2c2x0.txt b/Documentation/devicetree/bindings/arm/l2c2x0.txt
index d9650c1788f4..6316e673307a 100644
--- a/Documentation/devicetree/bindings/arm/l2c2x0.txt
+++ b/Documentation/devicetree/bindings/arm/l2c2x0.txt
@@ -76,6 +76,8 @@ Optional properties:
   specified to indicate that such transforms are precluded.
 - 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,ecc-enable : enable ECC protection on the L2 cache
+- arm,ecc-disable : disable ECC protection on the L2 cache
 - arm,outer-sync-disable : disable the outer sync operation on the L2 cache.
   Some core tiles, especially ARM PB11MPCore have a faulty L220 cache that
   will randomly hang unless outer sync operations are disabled.
diff --git a/arch/arm/mm/cache-l2x0.c b/arch/arm/mm/cache-l2x0.c
index 2cc2653b046f..4f0e6d9b151d 100644
--- a/arch/arm/mm/cache-l2x0.c
+++ b/arch/arm/mm/cache-l2x0.c
@@ -1505,6 +1505,13 @@ static void __init aurora_of_parse(const struct device_node *np,
 		mask |= AURORA_ACR_FORCE_WRITE_POLICY_MASK;
 	}
 
+	if (of_property_read_bool(np, "arm,ecc-enable")) {
+		mask |= L2C_AUX_CTRL_EVTMON_ENABLE;
+		val |= L2C_AUX_CTRL_EVTMON_ENABLE;
+	} else if (of_property_read_bool(np, "arm,ecc-disable")) {
+		mask |= L2C_AUX_CTRL_EVTMON_ENABLE;
+	}
+
 	if (of_property_read_bool(np, "arm,parity-enable")) {
 		mask |= L2C_AUX_CTRL_PARITY_ENABLE;
 		val |= L2C_AUX_CTRL_PARITY_ENABLE;
-- 
2.13.0

[RFC PATCH 4/4] ARM: dts: enable l2c parity and ecc protection on 98dx3236

[Chris Packham]

Signed-off-by: Chris Packham <chris.packham-6g8wRflRTwXFdCa3tKVlE6U/zSkkHjvu@public.gmane.org>
---
 arch/arm/boot/dts/armada-xp-98dx3236.dtsi | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/arm/boot/dts/armada-xp-98dx3236.dtsi b/arch/arm/boot/dts/armada-xp-98dx3236.dtsi
index 84cc232a29e9..32d05ec88ffd 100644
--- a/arch/arm/boot/dts/armada-xp-98dx3236.dtsi
+++ b/arch/arm/boot/dts/armada-xp-98dx3236.dtsi
@@ -138,6 +138,8 @@
 				cache-level = <2>;
 				cache-unified;
 				wt-override;
+				arm,parity-enable;
+				arm,ecc-enable;
 			};
 
 			gpio0: gpio@18100 {
-- 
2.13.0

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 3/4] ARM: l2x0: add arm,ecc-enable property for aurora

[Jan Lübbe]

On Do, 2017-06-08 at 16:11 +1200, Chris Packham wrote:
> +       if (of_property_read_bool(np, "arm,ecc-enable")) {
> +               mask |= L2C_AUX_CTRL_EVTMON_ENABLE;
> +               val |= L2C_AUX_CTRL_EVTMON_ENABLE;
> +       } else if (of_property_read_bool(np, "arm,ecc-disable")) {
> +               mask |= L2C_AUX_CTRL_EVTMON_ENABLE;
> +       }

Unless I misunderstand the code in __l2c_init(), the mask is used to
specify the bits to preserve:
        old_aux = aux = readl_relaxed(l2x0_base + L2X0_AUX_CTRL);
        aux &= aux_mask;
        aux |= aux_val;

        if (old_aux != aux)
                pr_warn("L2C: DT/platform modifies aux control register: 0x%08x -> 0x%08x\n",
                        old_aux, aux);

So the arm,ecc-disable property will have no effect. This probably also
applies to patch 2/4. The existing property *-disable code removes the
corresponding bit from the mask.

Regards,
Jan
-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [RFC PATCH 3/4] ARM: l2x0: add arm,ecc-enable property for aurora

[Rob Herring]

On Thu, Jun 08, 2017 at 04:11:23PM +1200, Chris Packham wrote:
> The aurora cache on the Marvell Armada-XP SoC supports ECC protection
> for the L2 data arrays. Add a "arm,ecc-enable" device tree property
> which can be used to enable this.
> 
> Signed-off-by: Chris Packham <chris.packham-6g8wRflRTwXFdCa3tKVlE6U/zSkkHjvu@public.gmane.org>
> ---
>  Documentation/devicetree/bindings/arm/l2c2x0.txt | 2 ++
>  arch/arm/mm/cache-l2x0.c                         | 7 +++++++
>  2 files changed, 9 insertions(+)

Acked-by: Rob Herring <robh-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 3/4] ARM: l2x0: add arm,ecc-enable property for aurora

[Chris Packham]

On 09/06/17 20:58, Jan Lübbe wrote:
> On Do, 2017-06-08 at 16:11 +1200, Chris Packham wrote:
>> +       if (of_property_read_bool(np, "arm,ecc-enable")) {
>> +               mask |= L2C_AUX_CTRL_EVTMON_ENABLE;
>> +               val |= L2C_AUX_CTRL_EVTMON_ENABLE;
>> +       } else if (of_property_read_bool(np, "arm,ecc-disable")) {
>> +               mask |= L2C_AUX_CTRL_EVTMON_ENABLE;
>> +       }
> 
> Unless I misunderstand the code in __l2c_init(), the mask is used to
> specify the bits to preserve:
>          old_aux = aux = readl_relaxed(l2x0_base + L2X0_AUX_CTRL);
>          aux &= aux_mask;
>          aux |= aux_val;
> 
>          if (old_aux != aux)
>                  pr_warn("L2C: DT/platform modifies aux control register: 0x%08x -> 0x%08x\n",
>                          old_aux, aux);
> 
> So the arm,ecc-disable property will have no effect. This probably also
> applies to patch 2/4. The existing property *-disable code removes the
> corresponding bit from the mask.

Indeed the disable version should be mask &= ~L2C_AUX_CTRL_EVTMON_ENABLE 
and I was probably a little lazy to have used the L2C EVTMON instead of 
adding AURORA specific ones like you have in your series.

I'll rebase my series on top of yours and send it direct to you so you 
can include it in the overall submission.

> 
> Regards,
> Jan
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 3/4] ARM: l2x0: add arm,ecc-enable property for aurora

[Jan Lübbe]

Chris,

On So, 2017-06-11 at 22:55 +0000, Chris Packham wrote:
> On 09/06/17 20:58, Jan Lübbe wrote:
> > On Do, 2017-06-08 at 16:11 +1200, Chris Packham wrote:
> >> +       if (of_property_read_bool(np, "arm,ecc-enable")) {
> >> +               mask |= L2C_AUX_CTRL_EVTMON_ENABLE;
> >> +               val |= L2C_AUX_CTRL_EVTMON_ENABLE;
> >> +       } else if (of_property_read_bool(np, "arm,ecc-disable")) {
> >> +               mask |= L2C_AUX_CTRL_EVTMON_ENABLE;
> >> +       }
> > 
> > Unless I misunderstand the code in __l2c_init(), the mask is used to
> > specify the bits to preserve:
> >          old_aux = aux = readl_relaxed(l2x0_base + L2X0_AUX_CTRL);
> >          aux &= aux_mask;
> >          aux |= aux_val;
> > 
> >          if (old_aux != aux)
> >                  pr_warn("L2C: DT/platform modifies aux control register: 0x%08x -> 0x%08x\n",
> >                          old_aux, aux);
> > 
> > So the arm,ecc-disable property will have no effect. This probably also
> > applies to patch 2/4. The existing property *-disable code removes the
> > corresponding bit from the mask.
> 
> Indeed the disable version should be mask &= ~L2C_AUX_CTRL_EVTMON_ENABLE 
> and I was probably a little lazy to have used the L2C EVTMON instead of 
> adding AURORA specific ones like you have in your series.
> 
> I'll rebase my series on top of yours and send it direct to you so you 
> can include it in the overall submission.

While picking up your arm,parity-enable and arm,ecc-enable patches for
my series, I noticed that the mask variable is inverted when merging the
local changes into aux_val/aux_mask at the end of aurora_of_parse. So I
now believe your code is correct. ;) I'd appreciate a close look,
nevertheless.

Regards,
Jan
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC PATCH 3/4] ARM: l2x0: add arm,ecc-enable property for aurora

[Chris Packham]

On 23/06/17 02:46, Jan Lübbe wrote:
> Chris,
> 
> On So, 2017-06-11 at 22:55 +0000, Chris Packham wrote:
>> On 09/06/17 20:58, Jan Lübbe wrote:
>>> On Do, 2017-06-08 at 16:11 +1200, Chris Packham wrote:
>>>> +       if (of_property_read_bool(np, "arm,ecc-enable")) {
>>>> +               mask |= L2C_AUX_CTRL_EVTMON_ENABLE;
>>>> +               val |= L2C_AUX_CTRL_EVTMON_ENABLE;
>>>> +       } else if (of_property_read_bool(np, "arm,ecc-disable")) {
>>>> +               mask |= L2C_AUX_CTRL_EVTMON_ENABLE;
>>>> +       }
>>>
>>> Unless I misunderstand the code in __l2c_init(), the mask is used to
>>> specify the bits to preserve:
>>>           old_aux = aux = readl_relaxed(l2x0_base + L2X0_AUX_CTRL);
>>>           aux &= aux_mask;
>>>           aux |= aux_val;
>>>
>>>           if (old_aux != aux)
>>>                   pr_warn("L2C: DT/platform modifies aux control register: 0x%08x -> 0x%08x\n",
>>>                           old_aux, aux);
>>>
>>> So the arm,ecc-disable property will have no effect. This probably also
>>> applies to patch 2/4. The existing property *-disable code removes the
>>> corresponding bit from the mask.
>>
>> Indeed the disable version should be mask &= ~L2C_AUX_CTRL_EVTMON_ENABLE
>> and I was probably a little lazy to have used the L2C EVTMON instead of
>> adding AURORA specific ones like you have in your series.
>>
>> I'll rebase my series on top of yours and send it direct to you so you
>> can include it in the overall submission.
> 
> While picking up your arm,parity-enable and arm,ecc-enable patches for
> my series, I noticed that the mask variable is inverted when merging the
> local changes into aux_val/aux_mask at the end of aurora_of_parse. So I
> now believe your code is correct. ;) I'd appreciate a close look,
> nevertheless.

For my part I tested the enable case but I never tested the disable case 
(because u-boot doesn't enable it there's nothing to disable). I can 
probably just poke the register before starting the kernel to confirm 
the disable case works with whatever masking logic we end up with.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html