[RESEND PATCH 0/4] EDAC: support reduce bus width on 98dx3236

[RESEND PATCH 0/4] EDAC: support reduce bus width on 98dx3236

[Chris Packham]

(sorry I messed up sending this earlier, there is one additional patch and I'll
actually include the linux-arm and linux-edac mailing lists)

This series applies on top of Jan Lubbe's "EDAC drivers for Armada XP L2 and
DDR" series[1]. 1/4, 2/4 and 3/4 don't strictly depend on Jan's work so they
could go in through the ARM tree if that is preferred.

The 98dx3236 and similar switch chips with integrated CPUs have fewer pins
available for the SDRAM interface so the definition of "full" and "half" is
different to the Armada-XP SoC. In this series I introduce a
"marvell,reduced-width" device tree property and use this to identify such a
system.

I chose to use a new property instead of a new compatible string because the IP
block really is the Armada-XP one (at least according to the Marvell FAE I
spoke to) and because the scenario of requiring a reduced pin-count when going
from an external SoC to an integrated one will be reasonably common as we see
more an more of these switches with integrated ARM cores.


[1] - http://marc.info/?l=linux-edac&m=150167758312924

Chris Packham (4):
  ARM: dts: enable L2 cache parity and ecc on db-xc3-24g4xg board
  dt-bindings: add "reduced-width" property for Armada XP SDRAM
    controller
  ARM: dts: mvebu: set reduced-width property for SDRAM on 98dx3236
  EDAC: add support for reduced-width Armada-XP SDRAM

 .../bindings/memory-controllers/mvebu-sdram-controller.txt          | 6 ++++++
 arch/arm/boot/dts/armada-xp-98dx3236.dtsi                           | 1 +
 arch/arm/boot/dts/armada-xp-db-xc3-24g4xg.dts                       | 5 +++++
 drivers/edac/armada_xp_edac.c                                       | 3 +++
 4 files changed, 15 insertions(+)

-- 
2.13.0

[RESEND PATCH 1/4] ARM: dts: enable L2 cache parity and ecc on db-xc3-24g4xg board

[Chris Packham]

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 arch/arm/boot/dts/armada-xp-db-xc3-24g4xg.dts | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/armada-xp-db-xc3-24g4xg.dts b/arch/arm/boot/dts/armada-xp-db-xc3-24g4xg.dts
index 06fce35d7491..00ca489fc788 100644
--- a/arch/arm/boot/dts/armada-xp-db-xc3-24g4xg.dts
+++ b/arch/arm/boot/dts/armada-xp-db-xc3-24g4xg.dts
@@ -70,6 +70,11 @@
 	};
 };
 
+&L2 {
+	arm,parity-enable;
+	marvell,ecc-enable;
+};
+
 &devbus_bootcs {
 	status = "okay";
 
-- 
2.13.0

[RESEND PATCH 2/4] dt-bindings: add "reduced-width" property for Armada XP SDRAM controller

[Chris Packham]

Some SoC implementations that use this controller have a reduced pin
count so the meaning of "full" and "half" with change.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 .../bindings/memory-controllers/mvebu-sdram-controller.txt          | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt b/Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt
index 89657d1d4cd4..3041868321c8 100644
--- a/Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt
+++ b/Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt
@@ -13,6 +13,12 @@ Required properties:
  - reg: a resource specifier for the register space, which should
    include all SDRAM controller registers as per the datasheet.
 
+Optional properties:
+ - marvell,reduced-width: some SoCs that use this SDRAM controller have
+   a reduced pin count. On such systems "full" width is 32-bits and
+   "half" width is 16-bits. Set this property to indicate that the SoC
+   used is such a system.
+
 Example:
 
 sdramc@1400 {
-- 
2.13.0

[RESEND PATCH 3/4] ARM: dts: mvebu: set reduced-width property for SDRAM on 98dx3236

[Chris Packham]

Because the 98dx3236 and similar SoCs are switch chips with integrated
CPUs they use a reduced pin count for the SDRAM interface. As such
"full" with is 32-bits and "half" width is 16-bits (as opposed to 64/32
on the discrete SoC). Set the reduced-width property on the sdramc node
to indicate this.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 arch/arm/boot/dts/armada-xp-98dx3236.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/armada-xp-98dx3236.dtsi b/arch/arm/boot/dts/armada-xp-98dx3236.dtsi
index 0e12816d961e..4d6a2acc1b55 100644
--- a/arch/arm/boot/dts/armada-xp-98dx3236.dtsi
+++ b/arch/arm/boot/dts/armada-xp-98dx3236.dtsi
@@ -129,6 +129,7 @@
 			sdramc@1400 {
 				compatible = "marvell,armada-xp-sdram-controller";
 				reg = <0x1400 0x500>;
+				marvell,reduced-width;
 			};
 
 			L2: l2-cache@8000 {
-- 
2.13.0

[RESEND PATCH 4/4] EDAC: add support for reduced-width Armada-XP SDRAM

[Chris Packham]

Some integrated Armada XP SoCs use a reduced pin count so the width of
the SDRAM interface is smaller than the traditional discrete SoCs. This
means that the definition of "full" and "half" width is further reduced.

Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
---
 drivers/edac/armada_xp_edac.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/edac/armada_xp_edac.c b/drivers/edac/armada_xp_edac.c
index 68e88b180928..d8edcaac87c0 100644
--- a/drivers/edac/armada_xp_edac.c
+++ b/drivers/edac/armada_xp_edac.c
@@ -350,6 +350,9 @@ static int armada_xp_mc_edac_probe(struct platform_device *pdev)
 	if (armada_xp_mc_edac_read_config(mci))
 		return -EINVAL;
 
+	if (of_property_read_bool(pdev->dev.of_node, "marvell,reduced-width"))
+		drvdata->width /= 2;
+
 	/* configure SBE threshold */
 	/* it seems that SBEs are not captured otherwise */
 	writel(1 << SDRAM_ERR_CTRL_ERR_THR_OFFSET,
-- 
2.13.0

Re: [RESEND PATCH 2/4] dt-bindings: add "reduced-width" property for Armada XP SDRAM controller

[Rob Herring]

On Mon, Aug 07, 2017 at 01:46:39PM +1200, Chris Packham wrote:
> Some SoC implementations that use this controller have a reduced pin
> count so the meaning of "full" and "half" with change.

s/with/width/ ?

> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>  .../bindings/memory-controllers/mvebu-sdram-controller.txt          | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt b/Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt
> index 89657d1d4cd4..3041868321c8 100644
> --- a/Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt
> +++ b/Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt
> @@ -13,6 +13,12 @@ Required properties:
>   - reg: a resource specifier for the register space, which should
>     include all SDRAM controller registers as per the datasheet.
>  
> +Optional properties:
> + - marvell,reduced-width: some SoCs that use this SDRAM controller have
> +   a reduced pin count. On such systems "full" width is 32-bits and
> +   "half" width is 16-bits. Set this property to indicate that the SoC
> +   used is such a system.

Maybe you should just state what the width is.

Or your compatible string should just be specific enough to know the 
width.

Rob

Re: [RESEND PATCH 2/4] dt-bindings: add "reduced-width" property for Armada XP SDRAM controller

[Chris Packham]

On 11/08/17 08:38, Rob Herring wrote:
> On Mon, Aug 07, 2017 at 01:46:39PM +1200, Chris Packham wrote:
>> Some SoC implementations that use this controller have a reduced pin
>> count so the meaning of "full" and "half" with change.
> 
> s/with/width/ ?
> 

Yes will include in v2.

>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>   .../bindings/memory-controllers/mvebu-sdram-controller.txt          | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt b/Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt
>> index 89657d1d4cd4..3041868321c8 100644
>> --- a/Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt
>> +++ b/Documentation/devicetree/bindings/memory-controllers/mvebu-sdram-controller.txt
>> @@ -13,6 +13,12 @@ Required properties:
>>    - reg: a resource specifier for the register space, which should
>>      include all SDRAM controller registers as per the datasheet.
>>   
>> +Optional properties:
>> + - marvell,reduced-width: some SoCs that use this SDRAM controller have
>> +   a reduced pin count. On such systems "full" width is 32-bits and
>> +   "half" width is 16-bits. Set this property to indicate that the SoC
>> +   used is such a system.
> 
> Maybe you should just state what the width is.


Specifying a number like 64/32/16 is done in for some other properties I 
dismissed that because what this is about how we interpret a 
pin-strapping option. I guess "max-width = <64>;" and "max-width = 
<32>"; would achieve the same.


> Or your compatible string should just be specific enough to know the
> width.

I decided against a new compatible sting that because the IP block 
really is the Armada-XP one and the existing compatible string is used 
in other places (using multiple compatible strings would solve that).

I'm not too fussed which of the 3 options are used. Is there any 
particular preference?

Re: [RESEND PATCH 4/4] EDAC: add support for reduced-width Armada-XP SDRAM

[Borislav Petkov]

On Mon, Aug 07, 2017 at 01:46:41PM +1200, Chris Packham wrote:
> Some integrated Armada XP SoCs use a reduced pin count so the width of
> the SDRAM interface is smaller than the traditional discrete SoCs. This
> means that the definition of "full" and "half" width is further reduced.
> 
> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
> ---
>  drivers/edac/armada_xp_edac.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/edac/armada_xp_edac.c b/drivers/edac/armada_xp_edac.c
> index 68e88b180928..d8edcaac87c0 100644
> --- a/drivers/edac/armada_xp_edac.c
> +++ b/drivers/edac/armada_xp_edac.c
> @@ -350,6 +350,9 @@ static int armada_xp_mc_edac_probe(struct platform_device *pdev)
>  	if (armada_xp_mc_edac_read_config(mci))
>  		return -EINVAL;
>  
> +	if (of_property_read_bool(pdev->dev.of_node, "marvell,reduced-width"))
> +		drvdata->width /= 2;

If the compiler doesn't already convert it to a shift on ARM, you
probably should do

		>>= 1;

here, just in case.

With that you can add my

Acked-by: Borislav Petkov <bp@suse.de>

and route it through an ARM tree.

Thx.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--

Re: [RESEND PATCH 2/4] dt-bindings: add "reduced-width" property for Armada XP SDRAM controller

[Jan Lübbe]

On Thu, 2017-08-10 at 21:17 +0000, Chris Packham wrote:
> On 11/08/17 08:38, Rob Herring wrote:
> > On Mon, Aug 07, 2017 at 01:46:39PM +1200, Chris Packham wrote:
[...]  
> > > +Optional properties:
> > > + - marvell,reduced-width: some SoCs that use this SDRAM controller have
> > > +   a reduced pin count. On such systems "full" width is 32-bits and
> > > +   "half" width is 16-bits. Set this property to indicate that the SoC
> > > +   used is such a system.
> > 
> > Maybe you should just state what the width is.
> 
> Specifying a number like 64/32/16 is done in for some other properties I 
> dismissed that because what this is about how we interpret a 
> pin-strapping option. I guess "max-width = <64>;" and "max-width = 
> <32>"; would achieve the same.
> 
> > Or your compatible string should just be specific enough to know the
> > width.
> 
> I decided against a new compatible sting that because the IP block 
> really is the Armada-XP one and the existing compatible string is used 
> in other places (using multiple compatible strings would solve that).
> 
> I'm not too fussed which of the 3 options are used. Is there any 
> particular preference?

I'd prefer a specific compatible string, as it would avoid adding even
more properties if further difference turn up.

Rob, I seem to remember that some drivers match the top-level
compatible against a list of SoC variants to detect SoC-dependent
features in a generic IP block. Is that something you'd prefer instead?

Regards,
Jan

Re: [RESEND PATCH 4/4] EDAC: add support for reduced-width Armada-XP SDRAM

[Chris Packham]

On 11/08/17 21:14, Borislav Petkov wrote:
> On Mon, Aug 07, 2017 at 01:46:41PM +1200, Chris Packham wrote:
>> Some integrated Armada XP SoCs use a reduced pin count so the width of
>> the SDRAM interface is smaller than the traditional discrete SoCs. This
>> means that the definition of "full" and "half" width is further reduced.
>>
>> Signed-off-by: Chris Packham <chris.packham@alliedtelesis.co.nz>
>> ---
>>   drivers/edac/armada_xp_edac.c | 3 +++
>>   1 file changed, 3 insertions(+)
>>
>> diff --git a/drivers/edac/armada_xp_edac.c b/drivers/edac/armada_xp_edac.c
>> index 68e88b180928..d8edcaac87c0 100644
>> --- a/drivers/edac/armada_xp_edac.c
>> +++ b/drivers/edac/armada_xp_edac.c
>> @@ -350,6 +350,9 @@ static int armada_xp_mc_edac_probe(struct platform_device *pdev)
>>   	if (armada_xp_mc_edac_read_config(mci))
>>   		return -EINVAL;
>>   
>> +	if (of_property_read_bool(pdev->dev.of_node, "marvell,reduced-width"))
>> +		drvdata->width /= 2;
> 
> If the compiler doesn't already convert it to a shift on ARM, you
> probably should do
> 
> 		>>= 1;
> 
> here, just in case.

Based on discussions around the first patch in this series the final 
version will probably be something like

   if (of_device_is_compatible(pdev->dev.of_node,
                               "marvell,98dx3236-sdram-controller")
           drvdata->width >>= 1;

> 
> With that you can add my
> 
> Acked-by: Borislav Petkov <bp@suse.de>
> 
> and route it through an ARM tree.

That may depend on where Jan's series lands. This is is the only patch 
that is dependent on it. Regardless it should be inert so aside from 
triggering checkpatch warnings about dt-bindings there would be no harm 
in this patch taking the long way round.

> 
> Thx.
> 

Re: [RESEND PATCH 0/4] EDAC: support reduce bus width on 98dx3236

[Gregory CLEMENT]

Hi Chris,
 
 On lun., août 07 2017, Chris Packham <chris.packham@alliedtelesis.co.nz> wrote:

> (sorry I messed up sending this earlier, there is one additional patch and I'll
> actually include the linux-arm and linux-edac mailing lists)
>
> This series applies on top of Jan Lubbe's "EDAC drivers for Armada XP L2 and
> DDR" series[1]. 1/4, 2/4 and 3/4 don't strictly depend on Jan's work so they
> could go in through the ARM tree if that is preferred.
>
> The 98dx3236 and similar switch chips with integrated CPUs have fewer pins
> available for the SDRAM interface so the definition of "full" and "half" is
> different to the Armada-XP SoC. In this series I introduce a
> "marvell,reduced-width" device tree property and use this to identify such a
> system.
>
> I chose to use a new property instead of a new compatible string because the IP
> block really is the Armada-XP one (at least according to the Marvell FAE I
> spoke to) and because the scenario of requiring a reduced pin-count when going
> from an external SoC to an integrated one will be reasonably common as we see
> more an more of these switches with integrated ARM cores.

If I understood well a version 2 is expected. If I missed it, please
point me on it.

Once the binding will be acked I will be able to apply the dts
patch. For the first patch unless I am wrong the the series "EDAC
drivers for Armada XP L2 and DDR" was not applied or even formally
acked. Also while you will be at sending a new version please add a
commit log even a simple one.

For the 3rd patch, I also wait the new version of patch 2 with an
ack.

Thanks,

Gregory


>
>
> [1] - http://marc.info/?l=linux-edac&m=150167758312924
>
> Chris Packham (4):
>   ARM: dts: enable L2 cache parity and ecc on db-xc3-24g4xg board
>   dt-bindings: add "reduced-width" property for Armada XP SDRAM
>     controller
>   ARM: dts: mvebu: set reduced-width property for SDRAM on 98dx3236
>   EDAC: add support for reduced-width Armada-XP SDRAM
>
>  .../bindings/memory-controllers/mvebu-sdram-controller.txt          | 6 ++++++
>  arch/arm/boot/dts/armada-xp-98dx3236.dtsi                           | 1 +
>  arch/arm/boot/dts/armada-xp-db-xc3-24g4xg.dts                       | 5 +++++
>  drivers/edac/armada_xp_edac.c                                       | 3 +++
>  4 files changed, 15 insertions(+)
>
> -- 
> 2.13.0
>

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: [RESEND PATCH 0/4] EDAC: support reduce bus width on 98dx3236

[Chris Packham]

Hi Gregory,

Thanks for following up.

On 21/09/17 01:04, Gregory CLEMENT wrote:
> Hi Chris,
>   
>   On lun., août 07 2017, Chris Packham <chris.packham@alliedtelesis.co.nz> wrote:
> 
>> (sorry I messed up sending this earlier, there is one additional patch and I'll
>> actually include the linux-arm and linux-edac mailing lists)
>>
>> This series applies on top of Jan Lubbe's "EDAC drivers for Armada XP L2 and
>> DDR" series[1]. 1/4, 2/4 and 3/4 don't strictly depend on Jan's work so they
>> could go in through the ARM tree if that is preferred.
>>
>> The 98dx3236 and similar switch chips with integrated CPUs have fewer pins
>> available for the SDRAM interface so the definition of "full" and "half" is
>> different to the Armada-XP SoC. In this series I introduce a
>> "marvell,reduced-width" device tree property and use this to identify such a
>> system.
>>
>> I chose to use a new property instead of a new compatible string because the IP
>> block really is the Armada-XP one (at least according to the Marvell FAE I
>> spoke to) and because the scenario of requiring a reduced pin-count when going
>> from an external SoC to an integrated one will be reasonably common as we see
>> more an more of these switches with integrated ARM cores.
> 
> If I understood well a version 2 is expected. If I missed it, please
> point me on it.

Yes I owe a v2. I was actually waiting to see Jan's series applied 
before sending my additions which build upon it. I'm traveling right now 
so it'll probably be at least a week before I'm able to send the next 
iteration.

> Once the binding will be acked I will be able to apply the dts
> patch. For the first patch unless I am wrong the the series "EDAC
> drivers for Armada XP L2 and DDR" was not applied or even formally
> acked.

Last I saw there was some comments from Boris on v2 of that series that 
needed addressing. Not sure if Jan has time to look at them. If not I 
can have a go when I get back.

> Also while you will be at sending a new version please add a
> commit log even a simple one.

Will do.
> For the 3rd patch, I also wait the new version of patch 2 with an
> ack.

Yes that's fine. I'm now doing some work on and Armada-38x platform and 
I realize that that also has the narrower DDR interface (as does the 375 
I believe). I was planning on using that as the compatible string since 
it's much better known than the integrated switch asic + CPU I've been 
working with.

> 
> Thanks,
> 
> Gregory
> 
> 
>>
>>
>> [1] - http://marc.info/?l=linux-edac&m=150167758312924
>>
>> Chris Packham (4):
>>    ARM: dts: enable L2 cache parity and ecc on db-xc3-24g4xg board
>>    dt-bindings: add "reduced-width" property for Armada XP SDRAM
>>      controller
>>    ARM: dts: mvebu: set reduced-width property for SDRAM on 98dx3236
>>    EDAC: add support for reduced-width Armada-XP SDRAM
>>
>>   .../bindings/memory-controllers/mvebu-sdram-controller.txt          | 6 ++++++
>>   arch/arm/boot/dts/armada-xp-98dx3236.dtsi                           | 1 +
>>   arch/arm/boot/dts/armada-xp-db-xc3-24g4xg.dts                       | 5 +++++
>>   drivers/edac/armada_xp_edac.c                                       | 3 +++
>>   4 files changed, 15 insertions(+)
>>
>> -- 
>> 2.13.0
>>
>