(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
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 <[email protected]>
---
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
Signed-off-by: Chris Packham <[email protected]>
---
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
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 <[email protected]>
---
.../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
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> .../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
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 <[email protected]>
>> ---
>> .../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?
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 <[email protected]>
> ---
> 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 <[email protected]>
and route it through an ARM tree.
Thx.
--
Regards/Gruss,
Boris.
ECO tip #101: Trim your mails when you reply.
--
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
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 <[email protected]>
>> ---
>> 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 <[email protected]>
>
> 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.
>
Hi Chris,
On lun., août 07 2017, Chris Packham <[email protected]> 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
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 <[email protected]> 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
>>
>