Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings & devicetree entries.
Hi DTS maintainers,
If possible, can I please get an Acked-by for this patch?
Many thanks...
Dinh
On 8/11/14, 10:18 AM, ttha...@opensource.altera.com wrote:
> From: Thor Thayer
>
> Add the Altera SDRAM EDAC bindings and device tree changes to the Altera SoC
> project.
>
> Signed-off-by: Thor Thayer
> ---
> v2: Changes to SoC SDRAM EDAC code.
>
> v3: Implement code suggestions for SDRAM EDAC code.
>
> v4: Remove syscon from SDRAM controller bindings.
>
> v5: No Change, bump version for consistency.
>
> v6: Only map the ctrlcfg register as syscon.
>
> v7: No change. Bump for consistency.
>
> v8: No change. Bump for consistency.
>
> v9: Changes to support a MFD SDRAM controller with nested EDAC.
>
> v10: Revert to using syscon based on feedback.
> ---
> .../bindings/arm/altera/socfpga-sdram-edac.txt | 15 +++
> arch/arm/boot/dts/socfpga.dtsi | 11 +++
> 2 files changed, 26 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
>
> diff --git
> a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
> b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
> new file mode 100644
> index 000..d0ce01d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
> @@ -0,0 +1,15 @@
> +Altera SOCFPGA SDRAM Error Detection & Correction [EDAC]
> +The EDAC accesses a range of registers in the SDRAM controller.
> +
> +Required properties:
> +- compatible : should contain "altr,sdram-edac";
> +- altr,sdr-syscon : phandle of the sdr module
> +- interrupts : Should contain the SDRAM ECC IRQ in the
> + appropriate format for the IRQ controller.
> +
> +Example:
> + sdramedac {
> + compatible = "altr,sdram-edac";
> + altr,sdr-syscon = <>;
> + interrupts = <0 39 4>;
> + };
> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> index 4676f25..45b361e 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -603,6 +603,17 @@
> };
> };
>
> + sdr: sdr@ffc25000 {
> + compatible = "syscon";
> + reg = <0xffc25000 0x1000>;
> + };
> +
> + sdramedac {
> + compatible = "altr,sdram-edac";
> + altr,sdr-syscon = <>;
> + interrupts = <0 39 4>;
> + };
> +
> L2: l2-cache@fffef000 {
> compatible = "arm,pl310-cache";
> reg = <0xfffef000 0x1000>;
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings devicetree entries.
Hi DTS maintainers,
If possible, can I please get an Acked-by for this patch?
Many thanks...
Dinh
On 8/11/14, 10:18 AM, ttha...@opensource.altera.com wrote:
From: Thor Thayer ttha...@opensource.altera.com
Add the Altera SDRAM EDAC bindings and device tree changes to the Altera SoC
project.
Signed-off-by: Thor Thayer ttha...@opensource.altera.com
---
v2: Changes to SoC SDRAM EDAC code.
v3: Implement code suggestions for SDRAM EDAC code.
v4: Remove syscon from SDRAM controller bindings.
v5: No Change, bump version for consistency.
v6: Only map the ctrlcfg register as syscon.
v7: No change. Bump for consistency.
v8: No change. Bump for consistency.
v9: Changes to support a MFD SDRAM controller with nested EDAC.
v10: Revert to using syscon based on feedback.
---
.../bindings/arm/altera/socfpga-sdram-edac.txt | 15 +++
arch/arm/boot/dts/socfpga.dtsi | 11 +++
2 files changed, 26 insertions(+)
create mode 100644
Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
diff --git
a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
new file mode 100644
index 000..d0ce01d
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
@@ -0,0 +1,15 @@
+Altera SOCFPGA SDRAM Error Detection Correction [EDAC]
+The EDAC accesses a range of registers in the SDRAM controller.
+
+Required properties:
+- compatible : should contain altr,sdram-edac;
+- altr,sdr-syscon : phandle of the sdr module
+- interrupts : Should contain the SDRAM ECC IRQ in the
+ appropriate format for the IRQ controller.
+
+Example:
+ sdramedac {
+ compatible = altr,sdram-edac;
+ altr,sdr-syscon = sdr;
+ interrupts = 0 39 4;
+ };
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 4676f25..45b361e 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -603,6 +603,17 @@
};
};
+ sdr: sdr@ffc25000 {
+ compatible = syscon;
+ reg = 0xffc25000 0x1000;
+ };
+
+ sdramedac {
+ compatible = altr,sdram-edac;
+ altr,sdr-syscon = sdr;
+ interrupts = 0 39 4;
+ };
+
L2: l2-cache@fffef000 {
compatible = arm,pl310-cache;
reg = 0xfffef000 0x1000;
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings & devicetree entries.
Hi Boris On 8/27/14, 1:36 AM, Borislav Petkov wrote: > On Tue, Aug 26, 2014 at 03:28:10PM -0500, Dinh Nguyen wrote: >> If Boris is okay with driver part and everyone else is OK with the DTS >> portion, then I can apply the DTS patch to my tree, and Boris take the >> driver patch into his tree? > > Actually, it would be easier for everyone involved if those patches go > together due to their dependency. So if you want me to take a look at > the EDAC parts again and give you my ack so that you can pick them all > up together and they go through your tree, let me know. > That would be great! Thanks, Dinh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings & devicetree entries.
On Tue, Aug 26, 2014 at 03:28:10PM -0500, Dinh Nguyen wrote: > If Boris is okay with driver part and everyone else is OK with the DTS > portion, then I can apply the DTS patch to my tree, and Boris take the > driver patch into his tree? Actually, it would be easier for everyone involved if those patches go together due to their dependency. So if you want me to take a look at the EDAC parts again and give you my ack so that you can pick them all up together and they go through your tree, let me know. -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings devicetree entries.
On Tue, Aug 26, 2014 at 03:28:10PM -0500, Dinh Nguyen wrote: If Boris is okay with driver part and everyone else is OK with the DTS portion, then I can apply the DTS patch to my tree, and Boris take the driver patch into his tree? Actually, it would be easier for everyone involved if those patches go together due to their dependency. So if you want me to take a look at the EDAC parts again and give you my ack so that you can pick them all up together and they go through your tree, let me know. -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings devicetree entries.
Hi Boris On 8/27/14, 1:36 AM, Borislav Petkov wrote: On Tue, Aug 26, 2014 at 03:28:10PM -0500, Dinh Nguyen wrote: If Boris is okay with driver part and everyone else is OK with the DTS portion, then I can apply the DTS patch to my tree, and Boris take the driver patch into his tree? Actually, it would be easier for everyone involved if those patches go together due to their dependency. So if you want me to take a look at the EDAC parts again and give you my ack so that you can pick them all up together and they go through your tree, let me know. That would be great! Thanks, Dinh -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings & devicetree entries.
On Tue, Aug 26, 2014 at 3:28 PM, Dinh Nguyen wrote: > On 08/26/2014 01:39 PM, Thor Thayer wrote: >> >> On 08/14/2014 01:49 PM, Pavel Machek wrote: >>> On Mon 2014-08-11 10:18:13, ttha...@opensource.altera.com wrote: From: Thor Thayer Add the Altera SDRAM EDAC bindings and device tree changes to the Altera SoC project. Signed-off-by: Thor Thayer >>> Acked-by: Pavel Machek >>> >>> >> Hi All, >> >> Any further comments or suggestions on this patch series? Thanks for the >> review, Pavel! > > If Boris is okay with driver part and everyone else is OK with the DTS > portion, then I can apply the DTS patch to my tree, and Boris take the > driver patch into his tree? > Actually, I think I need to get an Ack-by from a DTS maintainer before I can apply this as it is introducing a new binding. Sorry for the noise. Dinh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings & devicetree entries.
On 08/26/2014 01:39 PM, Thor Thayer wrote: > > On 08/14/2014 01:49 PM, Pavel Machek wrote: >> On Mon 2014-08-11 10:18:13, ttha...@opensource.altera.com wrote: >>> From: Thor Thayer >>> >>> Add the Altera SDRAM EDAC bindings and device tree changes to the >>> Altera SoC project. >>> >>> Signed-off-by: Thor Thayer >> Acked-by: Pavel Machek >> >> > Hi All, > > Any further comments or suggestions on this patch series? Thanks for the > review, Pavel! If Boris is okay with driver part and everyone else is OK with the DTS portion, then I can apply the DTS patch to my tree, and Boris take the driver patch into his tree? Thanks, Dinh -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings & devicetree entries.
On 08/14/2014 01:49 PM, Pavel Machek wrote: On Mon 2014-08-11 10:18:13, ttha...@opensource.altera.com wrote: From: Thor Thayer Add the Altera SDRAM EDAC bindings and device tree changes to the Altera SoC project. Signed-off-by: Thor Thayer Acked-by: Pavel Machek Hi All, Any further comments or suggestions on this patch series? Thanks for the review, Pavel! Thor -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings devicetree entries.
On 08/14/2014 01:49 PM, Pavel Machek wrote: On Mon 2014-08-11 10:18:13, ttha...@opensource.altera.com wrote: From: Thor Thayer ttha...@opensource.altera.com Add the Altera SDRAM EDAC bindings and device tree changes to the Altera SoC project. Signed-off-by: Thor Thayer ttha...@opensource.altera.com Acked-by: Pavel Machek pa...@denx.de Hi All, Any further comments or suggestions on this patch series? Thanks for the review, Pavel! Thor -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings devicetree entries.
On 08/26/2014 01:39 PM, Thor Thayer wrote: On 08/14/2014 01:49 PM, Pavel Machek wrote: On Mon 2014-08-11 10:18:13, ttha...@opensource.altera.com wrote: From: Thor Thayer ttha...@opensource.altera.com Add the Altera SDRAM EDAC bindings and device tree changes to the Altera SoC project. Signed-off-by: Thor Thayer ttha...@opensource.altera.com Acked-by: Pavel Machek pa...@denx.de Hi All, Any further comments or suggestions on this patch series? Thanks for the review, Pavel! If Boris is okay with driver part and everyone else is OK with the DTS portion, then I can apply the DTS patch to my tree, and Boris take the driver patch into his tree? Thanks, Dinh -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings devicetree entries.
On Tue, Aug 26, 2014 at 3:28 PM, Dinh Nguyen dingu...@opensource.altera.com wrote: On 08/26/2014 01:39 PM, Thor Thayer wrote: On 08/14/2014 01:49 PM, Pavel Machek wrote: On Mon 2014-08-11 10:18:13, ttha...@opensource.altera.com wrote: From: Thor Thayer ttha...@opensource.altera.com Add the Altera SDRAM EDAC bindings and device tree changes to the Altera SoC project. Signed-off-by: Thor Thayer ttha...@opensource.altera.com Acked-by: Pavel Machek pa...@denx.de Hi All, Any further comments or suggestions on this patch series? Thanks for the review, Pavel! If Boris is okay with driver part and everyone else is OK with the DTS portion, then I can apply the DTS patch to my tree, and Boris take the driver patch into his tree? Actually, I think I need to get an Ack-by from a DTS maintainer before I can apply this as it is introducing a new binding. Sorry for the noise. Dinh -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings & devicetree entries.
On Fri, Aug 15, 2014 at 11:47:09AM -0500, Thor Thayer wrote: > Hi Steffen! > > Building on Alan's points, I don't see the reference to the child > nodes in the syscon.txt binding documentation - can you point out > what I'm missing? > > I based this patch on other examples of syscon, and there aren't > child nodes (exynos5250.dtsi, omap3.dtsi, etc). According to my > understanding of the device tree parsing, the parent needs to > register child nodes. Since syscon is the parent and doesn't > register the child nodes, they won't be automatically probed. The > other syscon implementations are at the same level as the syscon and > use a phandle to the syscon (for instance the omap3.dtsi). > > Thanks for reviewing! > > Thor Hm, I looked in one of my many directories with some version of the kernel tree checked out, but ... you are right. I can't find where I read that. Seems like I got confused somewhere... There is now version of syscon.txt that mentions anything about childs. Should have drank my morning coffee first. Regards, Steffen -- 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- | -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings & devicetree entries.
On Fri, Aug 15, 2014 at 11:07:31AM -0500, atull wrote:
> On Fri, 15 Aug 2014, Steffen Trumtrar wrote:
>
> >
> > Hi!
>
> Hello
>
> Thanks for the feedback...
>
> >
> > ttha...@opensource.altera.com writes:
> >
> > > From: Thor Thayer
> > >
> > > Add the Altera SDRAM EDAC bindings and device tree changes to the Altera
> > > SoC project.
> > >
> > > Signed-off-by: Thor Thayer
> > > ---
> > > v2: Changes to SoC SDRAM EDAC code.
> > >
> > > v3: Implement code suggestions for SDRAM EDAC code.
> > >
> > > v4: Remove syscon from SDRAM controller bindings.
> > >
> > > v5: No Change, bump version for consistency.
> > >
> > > v6: Only map the ctrlcfg register as syscon.
> > >
> > > v7: No change. Bump for consistency.
> > >
> > > v8: No change. Bump for consistency.
> > >
> > > v9: Changes to support a MFD SDRAM controller with nested EDAC.
> > >
> > > v10: Revert to using syscon based on feedback.
> > > ---
> > > .../bindings/arm/altera/socfpga-sdram-edac.txt | 15 +++
> > > arch/arm/boot/dts/socfpga.dtsi | 11 +++
> > > 2 files changed, 26 insertions(+)
> > > create mode 100644
> > > Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
> > >
> > > diff --git
> > > a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
> > > b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
> > > new file mode 100644
> > > index 000..d0ce01d
> > > --- /dev/null
> > > +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
> > > @@ -0,0 +1,15 @@
> > > +Altera SOCFPGA SDRAM Error Detection & Correction [EDAC]
> > > +The EDAC accesses a range of registers in the SDRAM controller.
> > > +
> > > +Required properties:
> > > +- compatible : should contain "altr,sdram-edac";
> > > +- altr,sdr-syscon : phandle of the sdr module
> > > +- interrupts : Should contain the SDRAM ECC IRQ in the
> > > + appropriate format for the IRQ controller.
> > > +
> > > +Example:
> > > + sdramedac {
> > > + compatible = "altr,sdram-edac";
> > > + altr,sdr-syscon = <>;
> > > + interrupts = <0 39 4>;
> > > + };
> > > diff --git a/arch/arm/boot/dts/socfpga.dtsi
> > > b/arch/arm/boot/dts/socfpga.dtsi
> > > index 4676f25..45b361e 100644
> > > --- a/arch/arm/boot/dts/socfpga.dtsi
> > > +++ b/arch/arm/boot/dts/socfpga.dtsi
> > > @@ -603,6 +603,17 @@
> > > };
> > > };
> > >
> > > + sdr: sdr@ffc25000 {
> > > + compatible = "syscon";
> > > + reg = <0xffc25000 0x1000>;
> > > + };
> > > +
> > > + sdramedac {
> > > + compatible = "altr,sdram-edac";
> > > + altr,sdr-syscon = <>;
> > > + interrupts = <0 39 4>;
> > > + };
> > > +
> > > L2: l2-cache@fffef000 {
> > > compatible = "arm,pl310-cache";
> > > reg = <0xfffef000 0x1000>;
> >
> > Sorry, but I personally still don't like this approach.
>
> This is a normal use of syscon. Syscon is great for this case
> where multiple drivers need access to an ip block's register
> range but there is otherwise no need to write a MFD from scratch
> to allow that. I think that's the purpose of syscon in the first
> place.
>
I talked with a co-worker about this. And we came to the conclusion,
that syscon is wrong and has issues, but we sadly had no better idea.
I'd rather have the whole range be a syscon than some bad MFD implementation.
> >
> > If we do it like this however, shouldn't the different regions of the
> > SDR (EDAC, FPGAPORTRST, ...) at least be child nodes of the syscon?
> > That seems to match the syscon binding and describes the actual hardware
> > better IMHO.
>
> I like that; it would be clean and clear, but I don't think syscon is
> written such that that would actually work. I haven't seen anybody else
> do it that way.
Don't actually know. The documentation says this is possible.
>
> > Oh, and "reg = <...>" for the sdram-edac, maybe ?
>
> How would that work? Syscon is already mapping the whole register range
> for the sdr block. Are you proposing a device tree binding that this
> Linux driver would ignore, but some other driver in some other OS might
> find useful in the future? I'd rather not put duplicate information
> in two places, too easy for it to get out of sync.
>
Yes. I think you are actually right. This won't work.
> > How would I know where the start address of the EDAC is, if I would use
> > this DT on another OS where I don't have the same driver?
>
> The start address of the EDAC is an offset into the range mapped by
> syscon here. The driver knows what the register offsets are.
>
> If we are talking about this device tree being used by some other OS
> that is not aware of syscon, then that OS won't know what's going on and
> some modification of the DT will be needed. I have been advised that
> u-boot will need a significantly
Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings & devicetree entries.
On 08/15/2014 11:07 AM, atull wrote:
On Fri, 15 Aug 2014, Steffen Trumtrar wrote:
Hi!
Hello
Thanks for the feedback...
ttha...@opensource.altera.com writes:
From: Thor Thayer
Add the Altera SDRAM EDAC bindings and device tree changes to the Altera SoC
project.
Signed-off-by: Thor Thayer
---
v2: Changes to SoC SDRAM EDAC code.
v3: Implement code suggestions for SDRAM EDAC code.
v4: Remove syscon from SDRAM controller bindings.
v5: No Change, bump version for consistency.
v6: Only map the ctrlcfg register as syscon.
v7: No change. Bump for consistency.
v8: No change. Bump for consistency.
v9: Changes to support a MFD SDRAM controller with nested EDAC.
v10: Revert to using syscon based on feedback.
---
.../bindings/arm/altera/socfpga-sdram-edac.txt | 15 +++
arch/arm/boot/dts/socfpga.dtsi | 11 +++
2 files changed, 26 insertions(+)
create mode 100644
Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
diff --git
a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
new file mode 100644
index 000..d0ce01d
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
@@ -0,0 +1,15 @@
+Altera SOCFPGA SDRAM Error Detection & Correction [EDAC]
+The EDAC accesses a range of registers in the SDRAM controller.
+
+Required properties:
+- compatible : should contain "altr,sdram-edac";
+- altr,sdr-syscon : phandle of the sdr module
+- interrupts : Should contain the SDRAM ECC IRQ in the
+ appropriate format for the IRQ controller.
+
+Example:
+ sdramedac {
+ compatible = "altr,sdram-edac";
+ altr,sdr-syscon = <>;
+ interrupts = <0 39 4>;
+ };
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 4676f25..45b361e 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -603,6 +603,17 @@
};
};
+ sdr: sdr@ffc25000 {
+ compatible = "syscon";
+ reg = <0xffc25000 0x1000>;
+ };
+
+ sdramedac {
+ compatible = "altr,sdram-edac";
+ altr,sdr-syscon = <>;
+ interrupts = <0 39 4>;
+ };
+
L2: l2-cache@fffef000 {
compatible = "arm,pl310-cache";
reg = <0xfffef000 0x1000>;
Sorry, but I personally still don't like this approach.
This is a normal use of syscon. Syscon is great for this case
where multiple drivers need access to an ip block's register
range but there is otherwise no need to write a MFD from scratch
to allow that. I think that's the purpose of syscon in the first
place.
If we do it like this however, shouldn't the different regions of the
SDR (EDAC, FPGAPORTRST, ...) at least be child nodes of the syscon?
That seems to match the syscon binding and describes the actual hardware
better IMHO.
I like that; it would be clean and clear, but I don't think syscon is
written such that that would actually work. I haven't seen anybody else
do it that way.
Hi Steffen!
Building on Alan's points, I don't see the reference to the child nodes
in the syscon.txt binding documentation - can you point out what I'm
missing?
I based this patch on other examples of syscon, and there aren't child
nodes (exynos5250.dtsi, omap3.dtsi, etc). According to my understanding
of the device tree parsing, the parent needs to register child nodes.
Since syscon is the parent and doesn't register the child nodes, they
won't be automatically probed. The other syscon implementations are at
the same level as the syscon and use a phandle to the syscon (for
instance the omap3.dtsi).
Thanks for reviewing!
Thor
Oh, and "reg = <...>" for the sdram-edac, maybe ?
How would that work? Syscon is already mapping the whole register range
for the sdr block. Are you proposing a device tree binding that this
Linux driver would ignore, but some other driver in some other OS might
find useful in the future? I'd rather not put duplicate information
in two places, too easy for it to get out of sync.
How would I know where the start address of the EDAC is, if I would use
this DT on another OS where I don't have the same driver?
The start address of the EDAC is an offset into the range mapped by
syscon here. The driver knows what the register offsets are.
If we are talking about this device tree being used by some other OS
that is not aware of syscon, then that OS won't know what's going on and
some modification of the DT will be needed. I have been advised that
u-boot will need a significantly different DT, though I don't know
what the issues are there.
Regards,
Steffen
--
Pengutronix e.K. | Steffen Trumtrar|
Industrial Linux
Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings & devicetree entries.
On Fri, 15 Aug 2014, Steffen Trumtrar wrote:
>
> Hi!
Hello
Thanks for the feedback...
>
> ttha...@opensource.altera.com writes:
>
> > From: Thor Thayer
> >
> > Add the Altera SDRAM EDAC bindings and device tree changes to the Altera
> > SoC project.
> >
> > Signed-off-by: Thor Thayer
> > ---
> > v2: Changes to SoC SDRAM EDAC code.
> >
> > v3: Implement code suggestions for SDRAM EDAC code.
> >
> > v4: Remove syscon from SDRAM controller bindings.
> >
> > v5: No Change, bump version for consistency.
> >
> > v6: Only map the ctrlcfg register as syscon.
> >
> > v7: No change. Bump for consistency.
> >
> > v8: No change. Bump for consistency.
> >
> > v9: Changes to support a MFD SDRAM controller with nested EDAC.
> >
> > v10: Revert to using syscon based on feedback.
> > ---
> > .../bindings/arm/altera/socfpga-sdram-edac.txt | 15 +++
> > arch/arm/boot/dts/socfpga.dtsi | 11 +++
> > 2 files changed, 26 insertions(+)
> > create mode 100644
> > Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
> >
> > diff --git
> > a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
> > b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
> > new file mode 100644
> > index 000..d0ce01d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
> > @@ -0,0 +1,15 @@
> > +Altera SOCFPGA SDRAM Error Detection & Correction [EDAC]
> > +The EDAC accesses a range of registers in the SDRAM controller.
> > +
> > +Required properties:
> > +- compatible : should contain "altr,sdram-edac";
> > +- altr,sdr-syscon : phandle of the sdr module
> > +- interrupts : Should contain the SDRAM ECC IRQ in the
> > + appropriate format for the IRQ controller.
> > +
> > +Example:
> > + sdramedac {
> > + compatible = "altr,sdram-edac";
> > + altr,sdr-syscon = <>;
> > + interrupts = <0 39 4>;
> > + };
> > diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> > index 4676f25..45b361e 100644
> > --- a/arch/arm/boot/dts/socfpga.dtsi
> > +++ b/arch/arm/boot/dts/socfpga.dtsi
> > @@ -603,6 +603,17 @@
> > };
> > };
> >
> > + sdr: sdr@ffc25000 {
> > + compatible = "syscon";
> > + reg = <0xffc25000 0x1000>;
> > + };
> > +
> > + sdramedac {
> > + compatible = "altr,sdram-edac";
> > + altr,sdr-syscon = <>;
> > + interrupts = <0 39 4>;
> > + };
> > +
> > L2: l2-cache@fffef000 {
> > compatible = "arm,pl310-cache";
> > reg = <0xfffef000 0x1000>;
>
> Sorry, but I personally still don't like this approach.
This is a normal use of syscon. Syscon is great for this case
where multiple drivers need access to an ip block's register
range but there is otherwise no need to write a MFD from scratch
to allow that. I think that's the purpose of syscon in the first
place.
>
> If we do it like this however, shouldn't the different regions of the
> SDR (EDAC, FPGAPORTRST, ...) at least be child nodes of the syscon?
> That seems to match the syscon binding and describes the actual hardware
> better IMHO.
I like that; it would be clean and clear, but I don't think syscon is
written such that that would actually work. I haven't seen anybody else
do it that way.
> Oh, and "reg = <...>" for the sdram-edac, maybe ?
How would that work? Syscon is already mapping the whole register range
for the sdr block. Are you proposing a device tree binding that this
Linux driver would ignore, but some other driver in some other OS might
find useful in the future? I'd rather not put duplicate information
in two places, too easy for it to get out of sync.
> How would I know where the start address of the EDAC is, if I would use
> this DT on another OS where I don't have the same driver?
The start address of the EDAC is an offset into the range mapped by
syscon here. The driver knows what the register offsets are.
If we are talking about this device tree being used by some other OS
that is not aware of syscon, then that OS won't know what's going on and
some modification of the DT will be needed. I have been advised that
u-boot will need a significantly different DT, though I don't know
what the issues are there.
>
> Regards,
> Steffen
>
> --
> Pengutronix e.K. | Steffen Trumtrar|
> 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- |
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please
Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings & devicetree entries.
Hi!
ttha...@opensource.altera.com writes:
> From: Thor Thayer
>
> Add the Altera SDRAM EDAC bindings and device tree changes to the Altera SoC
> project.
>
> Signed-off-by: Thor Thayer
> ---
> v2: Changes to SoC SDRAM EDAC code.
>
> v3: Implement code suggestions for SDRAM EDAC code.
>
> v4: Remove syscon from SDRAM controller bindings.
>
> v5: No Change, bump version for consistency.
>
> v6: Only map the ctrlcfg register as syscon.
>
> v7: No change. Bump for consistency.
>
> v8: No change. Bump for consistency.
>
> v9: Changes to support a MFD SDRAM controller with nested EDAC.
>
> v10: Revert to using syscon based on feedback.
> ---
> .../bindings/arm/altera/socfpga-sdram-edac.txt | 15 +++
> arch/arm/boot/dts/socfpga.dtsi | 11 +++
> 2 files changed, 26 insertions(+)
> create mode 100644
> Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
>
> diff --git
> a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
> b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
> new file mode 100644
> index 000..d0ce01d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
> @@ -0,0 +1,15 @@
> +Altera SOCFPGA SDRAM Error Detection & Correction [EDAC]
> +The EDAC accesses a range of registers in the SDRAM controller.
> +
> +Required properties:
> +- compatible : should contain "altr,sdram-edac";
> +- altr,sdr-syscon : phandle of the sdr module
> +- interrupts : Should contain the SDRAM ECC IRQ in the
> + appropriate format for the IRQ controller.
> +
> +Example:
> + sdramedac {
> + compatible = "altr,sdram-edac";
> + altr,sdr-syscon = <>;
> + interrupts = <0 39 4>;
> + };
> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> index 4676f25..45b361e 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -603,6 +603,17 @@
> };
> };
>
> + sdr: sdr@ffc25000 {
> + compatible = "syscon";
> + reg = <0xffc25000 0x1000>;
> + };
> +
> + sdramedac {
> + compatible = "altr,sdram-edac";
> + altr,sdr-syscon = <>;
> + interrupts = <0 39 4>;
> + };
> +
> L2: l2-cache@fffef000 {
> compatible = "arm,pl310-cache";
> reg = <0xfffef000 0x1000>;
Sorry, but I personally still don't like this approach.
If we do it like this however, shouldn't the different regions of the
SDR (EDAC, FPGAPORTRST, ...) at least be child nodes of the syscon?
That seems to match the syscon binding and describes the actual hardware
better IMHO.
Oh, and "reg = <...>" for the sdram-edac, maybe ?
How would I know where the start address of the EDAC is, if I would use
this DT on another OS where I don't have the same driver?
Regards,
Steffen
--
Pengutronix e.K. | Steffen Trumtrar|
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- |
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings devicetree entries.
Hi!
ttha...@opensource.altera.com writes:
From: Thor Thayer ttha...@opensource.altera.com
Add the Altera SDRAM EDAC bindings and device tree changes to the Altera SoC
project.
Signed-off-by: Thor Thayer ttha...@opensource.altera.com
---
v2: Changes to SoC SDRAM EDAC code.
v3: Implement code suggestions for SDRAM EDAC code.
v4: Remove syscon from SDRAM controller bindings.
v5: No Change, bump version for consistency.
v6: Only map the ctrlcfg register as syscon.
v7: No change. Bump for consistency.
v8: No change. Bump for consistency.
v9: Changes to support a MFD SDRAM controller with nested EDAC.
v10: Revert to using syscon based on feedback.
---
.../bindings/arm/altera/socfpga-sdram-edac.txt | 15 +++
arch/arm/boot/dts/socfpga.dtsi | 11 +++
2 files changed, 26 insertions(+)
create mode 100644
Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
diff --git
a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
new file mode 100644
index 000..d0ce01d
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
@@ -0,0 +1,15 @@
+Altera SOCFPGA SDRAM Error Detection Correction [EDAC]
+The EDAC accesses a range of registers in the SDRAM controller.
+
+Required properties:
+- compatible : should contain altr,sdram-edac;
+- altr,sdr-syscon : phandle of the sdr module
+- interrupts : Should contain the SDRAM ECC IRQ in the
+ appropriate format for the IRQ controller.
+
+Example:
+ sdramedac {
+ compatible = altr,sdram-edac;
+ altr,sdr-syscon = sdr;
+ interrupts = 0 39 4;
+ };
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 4676f25..45b361e 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -603,6 +603,17 @@
};
};
+ sdr: sdr@ffc25000 {
+ compatible = syscon;
+ reg = 0xffc25000 0x1000;
+ };
+
+ sdramedac {
+ compatible = altr,sdram-edac;
+ altr,sdr-syscon = sdr;
+ interrupts = 0 39 4;
+ };
+
L2: l2-cache@fffef000 {
compatible = arm,pl310-cache;
reg = 0xfffef000 0x1000;
Sorry, but I personally still don't like this approach.
If we do it like this however, shouldn't the different regions of the
SDR (EDAC, FPGAPORTRST, ...) at least be child nodes of the syscon?
That seems to match the syscon binding and describes the actual hardware
better IMHO.
Oh, and reg = ... for the sdram-edac, maybe ?
How would I know where the start address of the EDAC is, if I would use
this DT on another OS where I don't have the same driver?
Regards,
Steffen
--
Pengutronix e.K. | Steffen Trumtrar|
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- |
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings devicetree entries.
On Fri, 15 Aug 2014, Steffen Trumtrar wrote:
Hi!
Hello
Thanks for the feedback...
ttha...@opensource.altera.com writes:
From: Thor Thayer ttha...@opensource.altera.com
Add the Altera SDRAM EDAC bindings and device tree changes to the Altera
SoC project.
Signed-off-by: Thor Thayer ttha...@opensource.altera.com
---
v2: Changes to SoC SDRAM EDAC code.
v3: Implement code suggestions for SDRAM EDAC code.
v4: Remove syscon from SDRAM controller bindings.
v5: No Change, bump version for consistency.
v6: Only map the ctrlcfg register as syscon.
v7: No change. Bump for consistency.
v8: No change. Bump for consistency.
v9: Changes to support a MFD SDRAM controller with nested EDAC.
v10: Revert to using syscon based on feedback.
---
.../bindings/arm/altera/socfpga-sdram-edac.txt | 15 +++
arch/arm/boot/dts/socfpga.dtsi | 11 +++
2 files changed, 26 insertions(+)
create mode 100644
Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
diff --git
a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
new file mode 100644
index 000..d0ce01d
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
@@ -0,0 +1,15 @@
+Altera SOCFPGA SDRAM Error Detection Correction [EDAC]
+The EDAC accesses a range of registers in the SDRAM controller.
+
+Required properties:
+- compatible : should contain altr,sdram-edac;
+- altr,sdr-syscon : phandle of the sdr module
+- interrupts : Should contain the SDRAM ECC IRQ in the
+ appropriate format for the IRQ controller.
+
+Example:
+ sdramedac {
+ compatible = altr,sdram-edac;
+ altr,sdr-syscon = sdr;
+ interrupts = 0 39 4;
+ };
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 4676f25..45b361e 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -603,6 +603,17 @@
};
};
+ sdr: sdr@ffc25000 {
+ compatible = syscon;
+ reg = 0xffc25000 0x1000;
+ };
+
+ sdramedac {
+ compatible = altr,sdram-edac;
+ altr,sdr-syscon = sdr;
+ interrupts = 0 39 4;
+ };
+
L2: l2-cache@fffef000 {
compatible = arm,pl310-cache;
reg = 0xfffef000 0x1000;
Sorry, but I personally still don't like this approach.
This is a normal use of syscon. Syscon is great for this case
where multiple drivers need access to an ip block's register
range but there is otherwise no need to write a MFD from scratch
to allow that. I think that's the purpose of syscon in the first
place.
If we do it like this however, shouldn't the different regions of the
SDR (EDAC, FPGAPORTRST, ...) at least be child nodes of the syscon?
That seems to match the syscon binding and describes the actual hardware
better IMHO.
I like that; it would be clean and clear, but I don't think syscon is
written such that that would actually work. I haven't seen anybody else
do it that way.
Oh, and reg = ... for the sdram-edac, maybe ?
How would that work? Syscon is already mapping the whole register range
for the sdr block. Are you proposing a device tree binding that this
Linux driver would ignore, but some other driver in some other OS might
find useful in the future? I'd rather not put duplicate information
in two places, too easy for it to get out of sync.
How would I know where the start address of the EDAC is, if I would use
this DT on another OS where I don't have the same driver?
The start address of the EDAC is an offset into the range mapped by
syscon here. The driver knows what the register offsets are.
If we are talking about this device tree being used by some other OS
that is not aware of syscon, then that OS won't know what's going on and
some modification of the DT will be needed. I have been advised that
u-boot will need a significantly different DT, though I don't know
what the issues are there.
Regards,
Steffen
--
Pengutronix e.K. | Steffen Trumtrar|
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- |
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to
Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings devicetree entries.
On 08/15/2014 11:07 AM, atull wrote:
On Fri, 15 Aug 2014, Steffen Trumtrar wrote:
Hi!
Hello
Thanks for the feedback...
ttha...@opensource.altera.com writes:
From: Thor Thayer ttha...@opensource.altera.com
Add the Altera SDRAM EDAC bindings and device tree changes to the Altera SoC
project.
Signed-off-by: Thor Thayer ttha...@opensource.altera.com
---
v2: Changes to SoC SDRAM EDAC code.
v3: Implement code suggestions for SDRAM EDAC code.
v4: Remove syscon from SDRAM controller bindings.
v5: No Change, bump version for consistency.
v6: Only map the ctrlcfg register as syscon.
v7: No change. Bump for consistency.
v8: No change. Bump for consistency.
v9: Changes to support a MFD SDRAM controller with nested EDAC.
v10: Revert to using syscon based on feedback.
---
.../bindings/arm/altera/socfpga-sdram-edac.txt | 15 +++
arch/arm/boot/dts/socfpga.dtsi | 11 +++
2 files changed, 26 insertions(+)
create mode 100644
Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
diff --git
a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
new file mode 100644
index 000..d0ce01d
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
@@ -0,0 +1,15 @@
+Altera SOCFPGA SDRAM Error Detection Correction [EDAC]
+The EDAC accesses a range of registers in the SDRAM controller.
+
+Required properties:
+- compatible : should contain altr,sdram-edac;
+- altr,sdr-syscon : phandle of the sdr module
+- interrupts : Should contain the SDRAM ECC IRQ in the
+ appropriate format for the IRQ controller.
+
+Example:
+ sdramedac {
+ compatible = altr,sdram-edac;
+ altr,sdr-syscon = sdr;
+ interrupts = 0 39 4;
+ };
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 4676f25..45b361e 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -603,6 +603,17 @@
};
};
+ sdr: sdr@ffc25000 {
+ compatible = syscon;
+ reg = 0xffc25000 0x1000;
+ };
+
+ sdramedac {
+ compatible = altr,sdram-edac;
+ altr,sdr-syscon = sdr;
+ interrupts = 0 39 4;
+ };
+
L2: l2-cache@fffef000 {
compatible = arm,pl310-cache;
reg = 0xfffef000 0x1000;
Sorry, but I personally still don't like this approach.
This is a normal use of syscon. Syscon is great for this case
where multiple drivers need access to an ip block's register
range but there is otherwise no need to write a MFD from scratch
to allow that. I think that's the purpose of syscon in the first
place.
If we do it like this however, shouldn't the different regions of the
SDR (EDAC, FPGAPORTRST, ...) at least be child nodes of the syscon?
That seems to match the syscon binding and describes the actual hardware
better IMHO.
I like that; it would be clean and clear, but I don't think syscon is
written such that that would actually work. I haven't seen anybody else
do it that way.
Hi Steffen!
Building on Alan's points, I don't see the reference to the child nodes
in the syscon.txt binding documentation - can you point out what I'm
missing?
I based this patch on other examples of syscon, and there aren't child
nodes (exynos5250.dtsi, omap3.dtsi, etc). According to my understanding
of the device tree parsing, the parent needs to register child nodes.
Since syscon is the parent and doesn't register the child nodes, they
won't be automatically probed. The other syscon implementations are at
the same level as the syscon and use a phandle to the syscon (for
instance the omap3.dtsi).
Thanks for reviewing!
Thor
Oh, and reg = ... for the sdram-edac, maybe ?
How would that work? Syscon is already mapping the whole register range
for the sdr block. Are you proposing a device tree binding that this
Linux driver would ignore, but some other driver in some other OS might
find useful in the future? I'd rather not put duplicate information
in two places, too easy for it to get out of sync.
How would I know where the start address of the EDAC is, if I would use
this DT on another OS where I don't have the same driver?
The start address of the EDAC is an offset into the range mapped by
syscon here. The driver knows what the register offsets are.
If we are talking about this device tree being used by some other OS
that is not aware of syscon, then that OS won't know what's going on and
some modification of the DT will be needed. I have been advised that
u-boot will need a significantly different DT, though I don't know
what the issues are there.
Regards,
Steffen
--
Pengutronix e.K. | Steffen
Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings devicetree entries.
On Fri, Aug 15, 2014 at 11:07:31AM -0500, atull wrote:
On Fri, 15 Aug 2014, Steffen Trumtrar wrote:
Hi!
Hello
Thanks for the feedback...
ttha...@opensource.altera.com writes:
From: Thor Thayer ttha...@opensource.altera.com
Add the Altera SDRAM EDAC bindings and device tree changes to the Altera
SoC project.
Signed-off-by: Thor Thayer ttha...@opensource.altera.com
---
v2: Changes to SoC SDRAM EDAC code.
v3: Implement code suggestions for SDRAM EDAC code.
v4: Remove syscon from SDRAM controller bindings.
v5: No Change, bump version for consistency.
v6: Only map the ctrlcfg register as syscon.
v7: No change. Bump for consistency.
v8: No change. Bump for consistency.
v9: Changes to support a MFD SDRAM controller with nested EDAC.
v10: Revert to using syscon based on feedback.
---
.../bindings/arm/altera/socfpga-sdram-edac.txt | 15 +++
arch/arm/boot/dts/socfpga.dtsi | 11 +++
2 files changed, 26 insertions(+)
create mode 100644
Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
diff --git
a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
new file mode 100644
index 000..d0ce01d
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
@@ -0,0 +1,15 @@
+Altera SOCFPGA SDRAM Error Detection Correction [EDAC]
+The EDAC accesses a range of registers in the SDRAM controller.
+
+Required properties:
+- compatible : should contain altr,sdram-edac;
+- altr,sdr-syscon : phandle of the sdr module
+- interrupts : Should contain the SDRAM ECC IRQ in the
+ appropriate format for the IRQ controller.
+
+Example:
+ sdramedac {
+ compatible = altr,sdram-edac;
+ altr,sdr-syscon = sdr;
+ interrupts = 0 39 4;
+ };
diff --git a/arch/arm/boot/dts/socfpga.dtsi
b/arch/arm/boot/dts/socfpga.dtsi
index 4676f25..45b361e 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -603,6 +603,17 @@
};
};
+ sdr: sdr@ffc25000 {
+ compatible = syscon;
+ reg = 0xffc25000 0x1000;
+ };
+
+ sdramedac {
+ compatible = altr,sdram-edac;
+ altr,sdr-syscon = sdr;
+ interrupts = 0 39 4;
+ };
+
L2: l2-cache@fffef000 {
compatible = arm,pl310-cache;
reg = 0xfffef000 0x1000;
Sorry, but I personally still don't like this approach.
This is a normal use of syscon. Syscon is great for this case
where multiple drivers need access to an ip block's register
range but there is otherwise no need to write a MFD from scratch
to allow that. I think that's the purpose of syscon in the first
place.
I talked with a co-worker about this. And we came to the conclusion,
that syscon is wrong and has issues, but we sadly had no better idea.
I'd rather have the whole range be a syscon than some bad MFD implementation.
If we do it like this however, shouldn't the different regions of the
SDR (EDAC, FPGAPORTRST, ...) at least be child nodes of the syscon?
That seems to match the syscon binding and describes the actual hardware
better IMHO.
I like that; it would be clean and clear, but I don't think syscon is
written such that that would actually work. I haven't seen anybody else
do it that way.
Don't actually know. The documentation says this is possible.
Oh, and reg = ... for the sdram-edac, maybe ?
How would that work? Syscon is already mapping the whole register range
for the sdr block. Are you proposing a device tree binding that this
Linux driver would ignore, but some other driver in some other OS might
find useful in the future? I'd rather not put duplicate information
in two places, too easy for it to get out of sync.
Yes. I think you are actually right. This won't work.
How would I know where the start address of the EDAC is, if I would use
this DT on another OS where I don't have the same driver?
The start address of the EDAC is an offset into the range mapped by
syscon here. The driver knows what the register offsets are.
If we are talking about this device tree being used by some other OS
that is not aware of syscon, then that OS won't know what's going on and
some modification of the DT will be needed. I have been advised that
u-boot will need a significantly different DT, though I don't know
what the issues are there.
That is one issue I personally have with syscon. We are always told to
only describe the hardware and don't mix Linux specifics in the bindings.
How is syscon not a Linux specific thing? Maybe I see
Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings devicetree entries.
On Fri, Aug 15, 2014 at 11:47:09AM -0500, Thor Thayer wrote: Hi Steffen! Building on Alan's points, I don't see the reference to the child nodes in the syscon.txt binding documentation - can you point out what I'm missing? I based this patch on other examples of syscon, and there aren't child nodes (exynos5250.dtsi, omap3.dtsi, etc). According to my understanding of the device tree parsing, the parent needs to register child nodes. Since syscon is the parent and doesn't register the child nodes, they won't be automatically probed. The other syscon implementations are at the same level as the syscon and use a phandle to the syscon (for instance the omap3.dtsi). Thanks for reviewing! Thor Hm, I looked in one of my many directories with some version of the kernel tree checked out, but ... you are right. I can't find where I read that. Seems like I got confused somewhere... There is now version of syscon.txt that mentions anything about childs. Should have drank my morning coffee first. Regards, Steffen -- 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- | -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings & devicetree entries.
On Mon 2014-08-11 10:18:13, ttha...@opensource.altera.com wrote: > From: Thor Thayer > > Add the Altera SDRAM EDAC bindings and device tree changes to the Altera SoC > project. > > Signed-off-by: Thor Thayer Acked-by: Pavel Machek -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings devicetree entries.
On Mon 2014-08-11 10:18:13, ttha...@opensource.altera.com wrote: From: Thor Thayer ttha...@opensource.altera.com Add the Altera SDRAM EDAC bindings and device tree changes to the Altera SoC project. Signed-off-by: Thor Thayer ttha...@opensource.altera.com Acked-by: Pavel Machek pa...@denx.de -- (english) http://www.livejournal.com/~pavelmachek (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings & devicetree entries.
From: Thor Thayer
Add the Altera SDRAM EDAC bindings and device tree changes to the Altera SoC
project.
Signed-off-by: Thor Thayer
---
v2: Changes to SoC SDRAM EDAC code.
v3: Implement code suggestions for SDRAM EDAC code.
v4: Remove syscon from SDRAM controller bindings.
v5: No Change, bump version for consistency.
v6: Only map the ctrlcfg register as syscon.
v7: No change. Bump for consistency.
v8: No change. Bump for consistency.
v9: Changes to support a MFD SDRAM controller with nested EDAC.
v10: Revert to using syscon based on feedback.
---
.../bindings/arm/altera/socfpga-sdram-edac.txt | 15 +++
arch/arm/boot/dts/socfpga.dtsi | 11 +++
2 files changed, 26 insertions(+)
create mode 100644
Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
diff --git
a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
new file mode 100644
index 000..d0ce01d
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
@@ -0,0 +1,15 @@
+Altera SOCFPGA SDRAM Error Detection & Correction [EDAC]
+The EDAC accesses a range of registers in the SDRAM controller.
+
+Required properties:
+- compatible : should contain "altr,sdram-edac";
+- altr,sdr-syscon : phandle of the sdr module
+- interrupts : Should contain the SDRAM ECC IRQ in the
+ appropriate format for the IRQ controller.
+
+Example:
+ sdramedac {
+ compatible = "altr,sdram-edac";
+ altr,sdr-syscon = <>;
+ interrupts = <0 39 4>;
+ };
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 4676f25..45b361e 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -603,6 +603,17 @@
};
};
+ sdr: sdr@ffc25000 {
+ compatible = "syscon";
+ reg = <0xffc25000 0x1000>;
+ };
+
+ sdramedac {
+ compatible = "altr,sdram-edac";
+ altr,sdr-syscon = <>;
+ interrupts = <0 39 4>;
+ };
+
L2: l2-cache@fffef000 {
compatible = "arm,pl310-cache";
reg = <0xfffef000 0x1000>;
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings devicetree entries.
From: Thor Thayer ttha...@opensource.altera.com
Add the Altera SDRAM EDAC bindings and device tree changes to the Altera SoC
project.
Signed-off-by: Thor Thayer ttha...@opensource.altera.com
---
v2: Changes to SoC SDRAM EDAC code.
v3: Implement code suggestions for SDRAM EDAC code.
v4: Remove syscon from SDRAM controller bindings.
v5: No Change, bump version for consistency.
v6: Only map the ctrlcfg register as syscon.
v7: No change. Bump for consistency.
v8: No change. Bump for consistency.
v9: Changes to support a MFD SDRAM controller with nested EDAC.
v10: Revert to using syscon based on feedback.
---
.../bindings/arm/altera/socfpga-sdram-edac.txt | 15 +++
arch/arm/boot/dts/socfpga.dtsi | 11 +++
2 files changed, 26 insertions(+)
create mode 100644
Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
diff --git
a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
new file mode 100644
index 000..d0ce01d
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
@@ -0,0 +1,15 @@
+Altera SOCFPGA SDRAM Error Detection Correction [EDAC]
+The EDAC accesses a range of registers in the SDRAM controller.
+
+Required properties:
+- compatible : should contain altr,sdram-edac;
+- altr,sdr-syscon : phandle of the sdr module
+- interrupts : Should contain the SDRAM ECC IRQ in the
+ appropriate format for the IRQ controller.
+
+Example:
+ sdramedac {
+ compatible = altr,sdram-edac;
+ altr,sdr-syscon = sdr;
+ interrupts = 0 39 4;
+ };
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 4676f25..45b361e 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -603,6 +603,17 @@
};
};
+ sdr: sdr@ffc25000 {
+ compatible = syscon;
+ reg = 0xffc25000 0x1000;
+ };
+
+ sdramedac {
+ compatible = altr,sdram-edac;
+ altr,sdr-syscon = sdr;
+ interrupts = 0 39 4;
+ };
+
L2: l2-cache@fffef000 {
compatible = arm,pl310-cache;
reg = 0xfffef000 0x1000;
--
1.7.9.5
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
