[PATCH 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[PATCH 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[tthayer]

From: Thor Thayer <tthayer@altera.com>

Addition of the Altera SDRAM controller bindings and device
tree changes to the Altera SoC project.

Signed-off-by: Thor Thayer <tthayer@altera.com>
To: Rob Herring <robherring2@gmail.com>
To: Pawel Moll <pawel.moll@arm.com>
To: Mark Rutland <mark.rutland@arm.com>
To: Ian Campbell <ijc+devicetree@hellion.org.uk>
To: Kumar Gala <galak@codeaurora.org>
To: Rob Landley <rob@landley.net>
To: Russell King <linux@arm.linux.org.uk>
To: Dinh Nguyen <dinguyen@altera.com>
Cc: devicetree@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
---
 .../bindings/arm/altera/socfpga-sdram.txt          |   14 ++++++++++++++
 arch/arm/boot/dts/socfpga.dtsi                     |    5 +++++
 2 files changed, 19 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt

diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
new file mode 100644
index 0000000..525cb76
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
@@ -0,0 +1,14 @@
+Altera SOCFPGA SDRAM Controller
+
+Required properties:
+- compatible : "altr,sdr-ctl", "syscon";
+                Note that syscon is invoked for this device to support the FPGA
+		bridge driver, EDAC driver and other devices that share the
+		registers.
+- reg : Should contain 1 register ranges(address and length)
+
+Example:
+	sdrctl@ffc25000 {
+		compatible = "altr,sdr-ctl", "syscon";
+		reg = <0xffc25000 0x1000>;
+	};
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index df43702..6ce912e 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -676,6 +676,11 @@
 			clocks = <&l4_sp_clk>;
 		};
 
+		sdrctl@ffc25000 {
+			compatible = "altr,sdr-ctl", "syscon";
+			reg = <0xffc25000 0x1000>;
+		};
+
 		rstmgr@ffd05000 {
 			compatible = "altr,rst-mgr";
 			reg = <0xffd05000 0x1000>;
-- 
1.7.9.5

Re: [PATCH 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Mark Rutland]

On Mon, Apr 07, 2014 at 10:54:07PM +0100, tthayer@altera.com wrote:
> From: Thor Thayer <tthayer@altera.com>
> 
> Addition of the Altera SDRAM controller bindings and device
> tree changes to the Altera SoC project.
> 
> Signed-off-by: Thor Thayer <tthayer@altera.com>
> To: Rob Herring <robherring2@gmail.com>
> To: Pawel Moll <pawel.moll@arm.com>
> To: Mark Rutland <mark.rutland@arm.com>
> To: Ian Campbell <ijc+devicetree@hellion.org.uk>
> To: Kumar Gala <galak@codeaurora.org>
> To: Rob Landley <rob@landley.net>
> To: Russell King <linux@arm.linux.org.uk>
> To: Dinh Nguyen <dinguyen@altera.com>
> Cc: devicetree@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> ---
>  .../bindings/arm/altera/socfpga-sdram.txt          |   14 ++++++++++++++
>  arch/arm/boot/dts/socfpga.dtsi                     |    5 +++++
>  2 files changed, 19 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
> new file mode 100644
> index 0000000..525cb76
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
> @@ -0,0 +1,14 @@
> +Altera SOCFPGA SDRAM Controller
> +
> +Required properties:
> +- compatible : "altr,sdr-ctl", "syscon";
> +                Note that syscon is invoked for this device to support the FPGA
> +		bridge driver, EDAC driver and other devices that share the
> +		registers.

Is the SDRAM controller really a bag of bits that necessitates the use
of syscon? Or are the "other devices" just sub-components of the SDRAM
controller?

If they are, just describe the SDRAM controller and related interrupts
as a single node, and only use the EDAC portion in the Linux driver.

Cheers,
Mark.

> +- reg : Should contain 1 register ranges(address and length)
> +
> +Example:
> +	sdrctl@ffc25000 {
> +		compatible = "altr,sdr-ctl", "syscon";
> +		reg = <0xffc25000 0x1000>;
> +	};
> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> index df43702..6ce912e 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -676,6 +676,11 @@
>  			clocks = <&l4_sp_clk>;
>  		};
>  
> +		sdrctl@ffc25000 {
> +			compatible = "altr,sdr-ctl", "syscon";
> +			reg = <0xffc25000 0x1000>;
> +		};
> +
>  		rstmgr@ffd05000 {
>  			compatible = "altr,rst-mgr";
>  			reg = <0xffd05000 0x1000>;
> -- 
> 1.7.9.5
> 
> 

Re: [PATCH 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Steffen Trumtrar]

Hi!

On Mon, Apr 07, 2014 at 04:54:07PM -0500, tthayer@altera.com wrote:
> From: Thor Thayer <tthayer@altera.com>
> 
> Addition of the Altera SDRAM controller bindings and device
> tree changes to the Altera SoC project.
> 
> Signed-off-by: Thor Thayer <tthayer@altera.com>
> To: Rob Herring <robherring2@gmail.com>
> To: Pawel Moll <pawel.moll@arm.com>
> To: Mark Rutland <mark.rutland@arm.com>
> To: Ian Campbell <ijc+devicetree@hellion.org.uk>
> To: Kumar Gala <galak@codeaurora.org>
> To: Rob Landley <rob@landley.net>
> To: Russell King <linux@arm.linux.org.uk>
> To: Dinh Nguyen <dinguyen@altera.com>
> Cc: devicetree@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> ---
>  .../bindings/arm/altera/socfpga-sdram.txt          |   14 ++++++++++++++
>  arch/arm/boot/dts/socfpga.dtsi                     |    5 +++++
>  2 files changed, 19 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
> 
> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
> new file mode 100644
> index 0000000..525cb76
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
> @@ -0,0 +1,14 @@
> +Altera SOCFPGA SDRAM Controller
> +
> +Required properties:
> +- compatible : "altr,sdr-ctl", "syscon";
> +                Note that syscon is invoked for this device to support the FPGA
> +		bridge driver, EDAC driver and other devices that share the
> +		registers.
> +- reg : Should contain 1 register ranges(address and length)

I haven't really thought this through, but why would the FPGA bridge driver
access the sdram controller? For releasing the resets in fpgaportrst ? Or is
there more?
Wouldn't it be more appropriate to represent those bits as a reset-controller to
some hypothetical IP core driver?
Then you could have something like

	hps2fpga@c0000000 {
		ipcore@0 {
			resets = <&sdr 1>;
			reset-names = "foo";
			resets = <&rst 96>;
			reset-names = "bar";
			(...)
		};

		ipcore@1000 {
			resets = <&rst 96>;
			reset-names = "baz";
			(...)
		};
	};

And you would always have the correct bridges released out of reset for your
IP core. Does the FPGA bridge driver do more? I think that is basically it.
Where we maybe could run into problems though is the early_init stuff.

I think syscon is nice for some things, but we should try not to overuse it.

Regards,
Steffen

> +Example:
> +	sdrctl@ffc25000 {
> +		compatible = "altr,sdr-ctl", "syscon";
> +		reg = <0xffc25000 0x1000>;
> +	};
> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> index df43702..6ce912e 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -676,6 +676,11 @@
>  			clocks = <&l4_sp_clk>;
>  		};
>  
> +		sdrctl@ffc25000 {
> +			compatible = "altr,sdr-ctl", "syscon";
> +			reg = <0xffc25000 0x1000>;
> +		};
> +
>  		rstmgr@ffd05000 {
>  			compatible = "altr,rst-mgr";
>  			reg = <0xffd05000 0x1000>;

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

Re: [PATCH 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Thor Thayer]

On Tue, 2014-04-08 at 15:38 +0200, Steffen Trumtrar wrote:
> Hi!
> 
> On Mon, Apr 07, 2014 at 04:54:07PM -0500, tthayer@altera.com wrote:
> > From: Thor Thayer <tthayer@altera.com>
> > 
> > Addition of the Altera SDRAM controller bindings and device
> > tree changes to the Altera SoC project.
> > 
[snip]
> > +
> > +Required properties:
> > +- compatible : "altr,sdr-ctl", "syscon";
> > +                Note that syscon is invoked for this device to support the FPGA
> > +		bridge driver, EDAC driver and other devices that share the
> > +		registers.
> > +- reg : Should contain 1 register ranges(address and length)
> 
> I haven't really thought this through, but why would the FPGA bridge driver
> access the sdram controller? For releasing the resets in fpgaportrst ? Or is
> there more?

Hi Steffan. No, not for resets. We need to enable the FPGA to SDRAM
path. Our SDRAM controller allows FPGA master access to the SDRAM.

> Wouldn't it be more appropriate to represent those bits as a reset-controller to
> some hypothetical IP core driver?
> Then you could have something like
> 
> 	hps2fpga@c0000000 {
> 		ipcore@0 {
> 			resets = <&sdr 1>;
> 			reset-names = "foo";
> 			resets = <&rst 96>;
> 			reset-names = "bar";
> 			(...)
> 		};
> 
> 		ipcore@1000 {
> 			resets = <&rst 96>;
> 			reset-names = "baz";
> 			(...)
> 		};
> 	};
> 
> And you would always have the correct bridges released out of reset for your
> IP core. Does the FPGA bridge driver do more? I think that is basically it.
> Where we maybe could run into problems though is the early_init stuff.
> 
> I think syscon is nice for some things, but we should try not to overuse it.

Understood. In this case, syscon seems to be appropriate.
> 
> Regards,
> Steffen
> 
> > +Example:
> > +	sdrctl@ffc25000 {
> > +		compatible = "altr,sdr-ctl", "syscon";
> > +		reg = <0xffc25000 0x1000>;
> > +	};
> > diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> > index df43702..6ce912e 100644
> > --- a/arch/arm/boot/dts/socfpga.dtsi
> > +++ b/arch/arm/boot/dts/socfpga.dtsi
> > @@ -676,6 +676,11 @@
> >  			clocks = <&l4_sp_clk>;
> >  		};
> >  
> > +		sdrctl@ffc25000 {
> > +			compatible = "altr,sdr-ctl", "syscon";
> > +			reg = <0xffc25000 0x1000>;
> > +		};
> > +
> >  		rstmgr@ffd05000 {
> >  			compatible = "altr,rst-mgr";
> >  			reg = <0xffd05000 0x1000>;
> 

Re: [PATCH 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Steffen Trumtrar]

On Tue, Apr 08, 2014 at 09:29:50AM -0500, Thor Thayer wrote:
> On Tue, 2014-04-08 at 15:38 +0200, Steffen Trumtrar wrote:
> > Hi!
> > 
> > On Mon, Apr 07, 2014 at 04:54:07PM -0500, tthayer@altera.com wrote:
> > > From: Thor Thayer <tthayer@altera.com>
> > > 
> > > Addition of the Altera SDRAM controller bindings and device
> > > tree changes to the Altera SoC project.
> > > 
> [snip]
> > > +
> > > +Required properties:
> > > +- compatible : "altr,sdr-ctl", "syscon";
> > > +                Note that syscon is invoked for this device to support the FPGA
> > > +		bridge driver, EDAC driver and other devices that share the
> > > +		registers.
> > > +- reg : Should contain 1 register ranges(address and length)
> > 
> > I haven't really thought this through, but why would the FPGA bridge driver
> > access the sdram controller? For releasing the resets in fpgaportrst ? Or is
> > there more?
> 
> Hi Steffan. No, not for resets. We need to enable the FPGA to SDRAM
> path. Our SDRAM controller allows FPGA master access to the SDRAM.
>

Yes. But what you have to do to enable the path is let the FPGA port you use
out of reset. And that is it as far as I can see. The rest happens in the
bitstream. Or is there more to enable the path?
The FPGA2SDRAM bridge is the one I didn't use as of yet, so if I miss something
please elaborate.

> > Wouldn't it be more appropriate to represent those bits as a reset-controller to
> > some hypothetical IP core driver?
> > Then you could have something like
> > 
> > 	hps2fpga@c0000000 {
> > 		ipcore@0 {
> > 			resets = <&sdr 1>;
> > 			reset-names = "foo";
> > 			resets = <&rst 96>;
> > 			reset-names = "bar";
> > 			(...)
> > 		};
> > 
> > 		ipcore@1000 {
> > 			resets = <&rst 96>;
> > 			reset-names = "baz";
> > 			(...)
> > 		};
> > 	};
> > 
> > And you would always have the correct bridges released out of reset for your
> > IP core. Does the FPGA bridge driver do more? I think that is basically it.
> > Where we maybe could run into problems though is the early_init stuff.
> > 
> > I think syscon is nice for some things, but we should try not to overuse it.
> 
> Understood. In this case, syscon seems to be appropriate.

I'm not convinced yet.

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-5555 |

Re: [PATCH 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[delicious quinoa]

On Tue, Apr 8, 2014 at 9:33 AM, Steffen Trumtrar
<s.trumtrar@pengutronix.de> wrote:
> On Tue, Apr 08, 2014 at 09:29:50AM -0500, Thor Thayer wrote:
>> On Tue, 2014-04-08 at 15:38 +0200, Steffen Trumtrar wrote:
>> > Hi!
>> >
>> > On Mon, Apr 07, 2014 at 04:54:07PM -0500, tthayer@altera.com wrote:
>> > > From: Thor Thayer <tthayer@altera.com>
>> > >
>> > > Addition of the Altera SDRAM controller bindings and device
>> > > tree changes to the Altera SoC project.
>> > >
>> [snip]
>> > > +
>> > > +Required properties:
>> > > +- compatible : "altr,sdr-ctl", "syscon";
>> > > +                Note that syscon is invoked for this device to support the FPGA
>> > > +         bridge driver, EDAC driver and other devices that share the
>> > > +         registers.
>> > > +- reg : Should contain 1 register ranges(address and length)
>> >
>> > I haven't really thought this through, but why would the FPGA bridge driver
>> > access the sdram controller? For releasing the resets in fpgaportrst ? Or is
>> > there more?
>>
>> Hi Steffan. No, not for resets. We need to enable the FPGA to SDRAM
>> path. Our SDRAM controller allows FPGA master access to the SDRAM.
>>
>
> Yes. But what you have to do to enable the path is let the FPGA port you use
> out of reset. And that is it as far as I can see. The rest happens in the
> bitstream. Or is there more to enable the path?
> The FPGA2SDRAM bridge is the one I didn't use as of yet, so if I miss something
> please elaborate.

Hi Steffen,

The sdram controller is used by two drivers.  That's why we want to
specify "syscon" here.  The other driver is the FPGA bridge driver.
Its functionality is very separate from what this driver is doing (we
are not enabling the bridge in this driver; we are enabling the
monitoring and resetting the interrupt bit of the EDAC).  We wanted to
specify "syscon" her so that we don't have to have to change it for
the other driver.

Alan Tull

Re: [PATCH 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Rob Herring]

On Tue, Apr 8, 2014 at 11:02 AM, delicious quinoa
<delicious.quinoa@gmail.com> wrote:
> On Tue, Apr 8, 2014 at 9:33 AM, Steffen Trumtrar
> <s.trumtrar@pengutronix.de> wrote:
>> On Tue, Apr 08, 2014 at 09:29:50AM -0500, Thor Thayer wrote:
>>> On Tue, 2014-04-08 at 15:38 +0200, Steffen Trumtrar wrote:
>>> > Hi!
>>> >
>>> > On Mon, Apr 07, 2014 at 04:54:07PM -0500, tthayer@altera.com wrote:
>>> > > From: Thor Thayer <tthayer@altera.com>
>>> > >
>>> > > Addition of the Altera SDRAM controller bindings and device
>>> > > tree changes to the Altera SoC project.
>>> > >
>>> [snip]
>>> > > +
>>> > > +Required properties:
>>> > > +- compatible : "altr,sdr-ctl", "syscon";
>>> > > +                Note that syscon is invoked for this device to support the FPGA
>>> > > +         bridge driver, EDAC driver and other devices that share the
>>> > > +         registers.
>>> > > +- reg : Should contain 1 register ranges(address and length)
>>> >
>>> > I haven't really thought this through, but why would the FPGA bridge driver
>>> > access the sdram controller? For releasing the resets in fpgaportrst ? Or is
>>> > there more?
>>>
>>> Hi Steffan. No, not for resets. We need to enable the FPGA to SDRAM
>>> path. Our SDRAM controller allows FPGA master access to the SDRAM.
>>>
>>
>> Yes. But what you have to do to enable the path is let the FPGA port you use
>> out of reset. And that is it as far as I can see. The rest happens in the
>> bitstream. Or is there more to enable the path?
>> The FPGA2SDRAM bridge is the one I didn't use as of yet, so if I miss something
>> please elaborate.
>
> Hi Steffen,
>
> The sdram controller is used by two drivers.  That's why we want to
> specify "syscon" here.  The other driver is the FPGA bridge driver.
> Its functionality is very separate from what this driver is doing (we
> are not enabling the bridge in this driver; we are enabling the
> monitoring and resetting the interrupt bit of the EDAC).  We wanted to
> specify "syscon" her so that we don't have to have to change it for
> the other driver.

But are there actually overlapping registers which are accessed by
both drivers and need the protection of regmap?

Perhaps MFD is more appropriate than syscon?

Rob

Re: [PATCH 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Thor Thayer]

On Tue, 2014-04-08 at 13:52 -0500, Rob Herring wrote:
> On Tue, Apr 8, 2014 at 11:02 AM, delicious quinoa
> <delicious.quinoa@gmail.com> wrote:
> > On Tue, Apr 8, 2014 at 9:33 AM, Steffen Trumtrar
> > <s.trumtrar@pengutronix.de> wrote:
> >> On Tue, Apr 08, 2014 at 09:29:50AM -0500, Thor Thayer wrote:
> >>> On Tue, 2014-04-08 at 15:38 +0200, Steffen Trumtrar wrote:
> >>> > Hi!
> >>> >
> >>> > On Mon, Apr 07, 2014 at 04:54:07PM -0500, tthayer@altera.com wrote:
> >>> > > From: Thor Thayer <tthayer@altera.com>
> >>> > >
> >>> > > Addition of the Altera SDRAM controller bindings and device
> >>> > > tree changes to the Altera SoC project.
> >>> > >
> >>> [snip]
> >>> > > +
> >>> > > +Required properties:
> >>> > > +- compatible : "altr,sdr-ctl", "syscon";
> >>> > > +                Note that syscon is invoked for this device to support the FPGA
> >>> > > +         bridge driver, EDAC driver and other devices that share the
> >>> > > +         registers.
> >>> > > +- reg : Should contain 1 register ranges(address and length)
> >>> >
> >>> > I haven't really thought this through, but why would the FPGA bridge driver
> >>> > access the sdram controller? For releasing the resets in fpgaportrst ? Or is
> >>> > there more?
> >>>
> >>> Hi Steffan. No, not for resets. We need to enable the FPGA to SDRAM
> >>> path. Our SDRAM controller allows FPGA master access to the SDRAM.
> >>>
> >>
> >> Yes. But what you have to do to enable the path is let the FPGA port you use
> >> out of reset. And that is it as far as I can see. The rest happens in the
> >> bitstream. Or is there more to enable the path?
> >> The FPGA2SDRAM bridge is the one I didn't use as of yet, so if I miss something
> >> please elaborate.
> >
> > Hi Steffen,
> >
> > The sdram controller is used by two drivers.  That's why we want to
> > specify "syscon" here.  The other driver is the FPGA bridge driver.
> > Its functionality is very separate from what this driver is doing (we
> > are not enabling the bridge in this driver; we are enabling the
> > monitoring and resetting the interrupt bit of the EDAC).  We wanted to
> > specify "syscon" her so that we don't have to have to change it for
> > the other driver.
> 
> But are there actually overlapping registers which are accessed by
> both drivers and need the protection of regmap?
> 
> Perhaps MFD is more appropriate than syscon?

Hi Rob,

We are accessing bits in the SDRAM Controller's Control register which
has other bits that configure the SDRAM controller. Since this main
control register may be accessed by other drivers (more likely for
reading the current SDRAM configuration setup than for writing), the
syscon still seems like an appropriate use.

Thor


> 
> Rob

Re: [PATCH 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Thor Thayer]

On Tue, 2014-04-08 at 13:52 -0500, Rob Herring wrote:
> On Tue, Apr 8, 2014 at 11:02 AM, delicious quinoa
> <delicious.quinoa@gmail.com> wrote:
> > On Tue, Apr 8, 2014 at 9:33 AM, Steffen Trumtrar
> > <s.trumtrar@pengutronix.de> wrote:
> >> On Tue, Apr 08, 2014 at 09:29:50AM -0500, Thor Thayer wrote:
> >>> On Tue, 2014-04-08 at 15:38 +0200, Steffen Trumtrar wrote:
> >>> > Hi!
> >>> >
> >>> > On Mon, Apr 07, 2014 at 04:54:07PM -0500, tthayer@altera.com wrote:
> >>> > > From: Thor Thayer <tthayer@altera.com>
> >>> > >
> >>> > > Addition of the Altera SDRAM controller bindings and device
> >>> > > tree changes to the Altera SoC project.
> >>> > >
> >>> [snip]
> >>> > > +
> >>> > > +Required properties:
> >>> > > +- compatible : "altr,sdr-ctl", "syscon";
> >>> > > +                Note that syscon is invoked for this device to support the FPGA
> >>> > > +         bridge driver, EDAC driver and other devices that share the
> >>> > > +         registers.
> >>> > > +- reg : Should contain 1 register ranges(address and length)
> >>> >
> >>> > I haven't really thought this through, but why would the FPGA bridge driver
> >>> > access the sdram controller? For releasing the resets in fpgaportrst ? Or is
> >>> > there more?
> >>>
> >>> Hi Steffan. No, not for resets. We need to enable the FPGA to SDRAM
> >>> path. Our SDRAM controller allows FPGA master access to the SDRAM.
> >>>
> >>
> >> Yes. But what you have to do to enable the path is let the FPGA port you use
> >> out of reset. And that is it as far as I can see. The rest happens in the
> >> bitstream. Or is there more to enable the path?
> >> The FPGA2SDRAM bridge is the one I didn't use as of yet, so if I miss something
> >> please elaborate.
> >
> > Hi Steffen,
> >
> > The sdram controller is used by two drivers.  That's why we want to
> > specify "syscon" here.  The other driver is the FPGA bridge driver.
> > Its functionality is very separate from what this driver is doing (we
> > are not enabling the bridge in this driver; we are enabling the
> > monitoring and resetting the interrupt bit of the EDAC).  We wanted to
> > specify "syscon" her so that we don't have to have to change it for
> > the other driver.
> 
> But are there actually overlapping registers which are accessed by
> both drivers and need the protection of regmap?
> 
> Perhaps MFD is more appropriate than syscon?

Hi Rob,

We are accessing bits in the SDRAM Controller's Control register which
has other bits that configure the SDRAM controller. Since this main
control register may be accessed by other drivers (more likely for
reading the current SDRAM configuration setup than for writing), the
syscon still seems like an appropriate use.

Thor


> 
> Rob

Re: [PATCH 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Thor Thayer]

On Tue, 2014-04-08 at 13:52 -0500, Rob Herring wrote:
> On Tue, Apr 8, 2014 at 11:02 AM, delicious quinoa
> <delicious.quinoa@gmail.com> wrote:
> > On Tue, Apr 8, 2014 at 9:33 AM, Steffen Trumtrar
> > <s.trumtrar@pengutronix.de> wrote:
> >> On Tue, Apr 08, 2014 at 09:29:50AM -0500, Thor Thayer wrote:
> >>> On Tue, 2014-04-08 at 15:38 +0200, Steffen Trumtrar wrote:
> >>> > Hi!
> >>> >
> >>> > On Mon, Apr 07, 2014 at 04:54:07PM -0500, tthayer@altera.com wrote:
> >>> > > From: Thor Thayer <tthayer@altera.com>
> >>> > >
> >>> > > Addition of the Altera SDRAM controller bindings and device
> >>> > > tree changes to the Altera SoC project.
> >>> > >
> >>> [snip]
> >>> > > +
> >>> > > +Required properties:
> >>> > > +- compatible : "altr,sdr-ctl", "syscon";
> >>> > > +                Note that syscon is invoked for this device to support the FPGA
> >>> > > +         bridge driver, EDAC driver and other devices that share the
> >>> > > +         registers.
> >>> > > +- reg : Should contain 1 register ranges(address and length)
> >>> >
> >>> > I haven't really thought this through, but why would the FPGA bridge driver
> >>> > access the sdram controller? For releasing the resets in fpgaportrst ? Or is
> >>> > there more?
> >>>
> >>> Hi Steffan. No, not for resets. We need to enable the FPGA to SDRAM
> >>> path. Our SDRAM controller allows FPGA master access to the SDRAM.
> >>>
> >>
> >> Yes. But what you have to do to enable the path is let the FPGA port you use
> >> out of reset. And that is it as far as I can see. The rest happens in the
> >> bitstream. Or is there more to enable the path?
> >> The FPGA2SDRAM bridge is the one I didn't use as of yet, so if I miss something
> >> please elaborate.
> >
> > Hi Steffen,
> >
> > The sdram controller is used by two drivers.  That's why we want to
> > specify "syscon" here.  The other driver is the FPGA bridge driver.
> > Its functionality is very separate from what this driver is doing (we
> > are not enabling the bridge in this driver; we are enabling the
> > monitoring and resetting the interrupt bit of the EDAC).  We wanted to
> > specify "syscon" her so that we don't have to have to change it for
> > the other driver.
> 
> But are there actually overlapping registers which are accessed by
> both drivers and need the protection of regmap?
> 
> Perhaps MFD is more appropriate than syscon?

Hi Rob,

We are accessing bits in the SDRAM Controller's Configuration register which
has other bits that configure the SDRAM controller. Since this main
configuration register may be accessed by other drivers (more likely for
reading the current SDRAM configuration setup than for writing), the
syscon still seems like an appropriate use.

Thor


> 
> Rob

Re: [PATCH 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Alan Tull]

On Tue, Apr 8, 2014 at 1:52 PM, Rob Herring <robherring2@gmail.com> wrote:
> On Tue, Apr 8, 2014 at 11:02 AM, delicious quinoa
> <delicious.quinoa@gmail.com> wrote:
>> On Tue, Apr 8, 2014 at 9:33 AM, Steffen Trumtrar
>> <s.trumtrar@pengutronix.de> wrote:
>>> On Tue, Apr 08, 2014 at 09:29:50AM -0500, Thor Thayer wrote:
>>>> On Tue, 2014-04-08 at 15:38 +0200, Steffen Trumtrar wrote:
>>>> > Hi!
>>>> >
>>>> > On Mon, Apr 07, 2014 at 04:54:07PM -0500, tthayer@altera.com wrote:
>>>> > > From: Thor Thayer <tthayer@altera.com>
>>>> > >
>>>> > > Addition of the Altera SDRAM controller bindings and device
>>>> > > tree changes to the Altera SoC project.
>>>> > >
>>>> [snip]
>>>> > > +
>>>> > > +Required properties:
>>>> > > +- compatible : "altr,sdr-ctl", "syscon";
>>>> > > +                Note that syscon is invoked for this device to support the FPGA
>>>> > > +         bridge driver, EDAC driver and other devices that share the
>>>> > > +         registers.
>>>> > > +- reg : Should contain 1 register ranges(address and length)
>>>> >
>>>> > I haven't really thought this through, but why would the FPGA bridge driver
>>>> > access the sdram controller? For releasing the resets in fpgaportrst ? Or is
>>>> > there more?
>>>>
>>>> Hi Steffan. No, not for resets. We need to enable the FPGA to SDRAM
>>>> path. Our SDRAM controller allows FPGA master access to the SDRAM.
>>>>
>>>
>>> Yes. But what you have to do to enable the path is let the FPGA port you use
>>> out of reset. And that is it as far as I can see. The rest happens in the
>>> bitstream. Or is there more to enable the path?
>>> The FPGA2SDRAM bridge is the one I didn't use as of yet, so if I miss something
>>> please elaborate.
>>
>> Hi Steffen,
>>
>> The sdram controller is used by two drivers.  That's why we want to
>> specify "syscon" here.  The other driver is the FPGA bridge driver.
>> Its functionality is very separate from what this driver is doing (we
>> are not enabling the bridge in this driver; we are enabling the
>> monitoring and resetting the interrupt bit of the EDAC).  We wanted to
>> specify "syscon" her so that we don't have to have to change it for
>> the other driver.
>
> But are there actually overlapping registers which are accessed by
> both drivers and need the protection of regmap?

No overlapping registers here.  Just various registers that are used
by: edac driver, fpga bridge, low power modes.  So no special
protection needed.

>
> Perhaps MFD is more appropriate than syscon?
>
> Rob

A syscon will do fine here.  If we did a MFD, all it would be doing
would be providing register access for this range of registers to a
few drivers, so syscon does that without any trouble.

Alan Tull
aka
delicious quinoa

[PATCH 2/3] dts: socfpga: Add bindings for Altera SoC SDRAM EDAC

[tthayer]

From: Thor Thayer <tthayer@altera.com>

Addition of the Altera SDRAM EDAC bindings and device
tree changes to the Altera SoC project.

Signed-off-by: Thor Thayer <tthayer@altera.com>
To: Rob Herring <robherring2@gmail.com>
To: Pawel Moll <pawel.moll@arm.com>
To: Mark Rutland <mark.rutland@arm.com>
To: Ian Campbell <ijc+devicetree@hellion.org.uk>
To: Kumar Gala <galak@codeaurora.org>
To: Rob Landley <rob@landley.net>
To: Russell King <linux@arm.linux.org.uk>
To: Dinh Nguyen <dinguyen@altera.com>
Cc: devicetree@vger.kernel.org
Cc: linux-doc@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
---
 .../bindings/arm/altera/socfpga-sdram-edac.txt     |   12 ++++++++++++
 arch/arm/boot/dts/socfpga.dtsi                     |    5 +++++
 2 files changed, 17 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 0000000..9348c53
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
@@ -0,0 +1,12 @@
+Altera SOCFPGA SDRAM Error Detection & Correction [EDAC]
+
+Required properties:
+- compatible : should contain "altr,sdr-edac";
+- interrupts : Should contain the SDRAM ECC IRQ in the
+	appropriate format for the IRQ controller.
+
+Example:
+	sdramedac@0 {
+		compatible = "altr,sdram-edac";
+		interrupts = <0 39 4>;
+	};
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 6ce912e..a0ea69b 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -681,6 +681,11 @@
 			reg = <0xffc25000 0x1000>;
 		};
 
+		sdramedac@0 {
+			compatible = "altr,sdram-edac";
+			interrupts = <0 39 4>;
+		};
+
 		rstmgr@ffd05000 {
 			compatible = "altr,rst-mgr";
 			reg = <0xffd05000 0x1000>;
-- 
1.7.9.5

Re: [PATCH 2/3] dts: socfpga: Add bindings for Altera SoC SDRAM EDAC

[Mark Rutland]

On Mon, Apr 07, 2014 at 10:54:08PM +0100, tthayer@altera.com wrote:
> From: Thor Thayer <tthayer@altera.com>
> 
> Addition of the Altera SDRAM EDAC bindings and device
> tree changes to the Altera SoC project.
> 
> Signed-off-by: Thor Thayer <tthayer@altera.com>
> To: Rob Herring <robherring2@gmail.com>
> To: Pawel Moll <pawel.moll@arm.com>
> To: Mark Rutland <mark.rutland@arm.com>
> To: Ian Campbell <ijc+devicetree@hellion.org.uk>
> To: Kumar Gala <galak@codeaurora.org>
> To: Rob Landley <rob@landley.net>
> To: Russell King <linux@arm.linux.org.uk>
> To: Dinh Nguyen <dinguyen@altera.com>
> Cc: devicetree@vger.kernel.org
> Cc: linux-doc@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-arm-kernel@lists.infradead.org
> ---
>  .../bindings/arm/altera/socfpga-sdram-edac.txt     |   12 ++++++++++++
>  arch/arm/boot/dts/socfpga.dtsi                     |    5 +++++
>  2 files changed, 17 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 0000000..9348c53
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
> @@ -0,0 +1,12 @@
> +Altera SOCFPGA SDRAM Error Detection & Correction [EDAC]
> +
> +Required properties:
> +- compatible : should contain "altr,sdr-edac";
> +- interrupts : Should contain the SDRAM ECC IRQ in the
> +	appropriate format for the IRQ controller.
> +
> +Example:
> +	sdramedac@0 {

Nit: If there's no reg, there shouldn't be a unit-address (the "@0").

> +		compatible = "altr,sdram-edac";
> +		interrupts = <0 39 4>;
> +	};

No phandle to the actual SDRAM controller node? Is there a guaranteed
limitation of a single SDRAM controller?

I don't see the point in describing this separately from the main SDRAM
controller node, given this seems to be a subcomponent.

> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> index 6ce912e..a0ea69b 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -681,6 +681,11 @@
>  			reg = <0xffc25000 0x1000>;
>  		};
>  
> +		sdramedac@0 {

Nit: get rid of the unit-address here too.

Cheers,
Mark.

> +			compatible = "altr,sdram-edac";
> +			interrupts = <0 39 4>;
> +		};
> +
>  		rstmgr@ffd05000 {
>  			compatible = "altr,rst-mgr";
>  			reg = <0xffd05000 0x1000>;
> -- 
> 1.7.9.5
> 
> 

[PATCH 3/3] edac: altera: Add SDRAM EDAC support for CycloneV/ArriaV

[tthayer]

From: Thor Thayer <tthayer@altera.com>

Added EDAC support for reporting ECC errors of CycloneV
and ArriaV SDRAM controller.
- The SDRAM Controller registers are used by the FPGA bridge so
  these are accessed through the syscon interface.
- The configuration of the SDRAM memory size for the EDAC framework
  is discovered from the memory node of the device tree.
- Documentation of the bindings in devicetree/bindings/arm/altera/
  socfpga-sdram-edac.txt
- Correction of single bit errors, detection of double bit errors.

Signed-off-by: Thor Thayer <tthayer@altera.com>
To: Rob Herring <robherring2@gmail.com>
To: Doug Thompson <dougthompson@xmission.com>
To: Grant Likely <grant.likely@linaro.org>
Cc: Dinh Nguyen <dinguyen@altera.com>
Cc: devicetree@vger.kernel.org
Cc: linux-edac@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/edac/Kconfig          |    9 ++
 drivers/edac/Makefile         |    2 +
 drivers/edac/altera_mc_edac.c |  360 +++++++++++++++++++++++++++++++++++++++++
 3 files changed, 371 insertions(+)
 create mode 100644 drivers/edac/altera_mc_edac.c

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 878f090..4f4d379 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -368,4 +368,13 @@ config EDAC_OCTEON_PCI
 	  Support for error detection and correction on the
 	  Cavium Octeon family of SOCs.
 
+config EDAC_ALTERA_MC
+	bool "Altera SDRAM Memory Controller EDAC"
+	depends on EDAC_MM_EDAC && ARCH_SOCFPGA
+	help
+	  Support for error detection and correction on the
+	  Altera SDRAM memory controller. Note that the
+	  preloader must initialize the SDRAM before loading
+	  the kernel.
+
 endif # EDAC
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index 4154ed6..e15d05f 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -64,3 +64,5 @@ obj-$(CONFIG_EDAC_OCTEON_PC)		+= octeon_edac-pc.o
 obj-$(CONFIG_EDAC_OCTEON_L2C)		+= octeon_edac-l2c.o
 obj-$(CONFIG_EDAC_OCTEON_LMC)		+= octeon_edac-lmc.o
 obj-$(CONFIG_EDAC_OCTEON_PCI)		+= octeon_edac-pci.o
+
+obj-$(CONFIG_EDAC_ALTERA_MC)	        += altera_mc_edac.o
diff --git a/drivers/edac/altera_mc_edac.c b/drivers/edac/altera_mc_edac.c
new file mode 100644
index 0000000..7d15196
--- /dev/null
+++ b/drivers/edac/altera_mc_edac.c
@@ -0,0 +1,360 @@
+/*
+ *  Copyright Altera Corporation (C) 2014. All rights reserved.
+ *  Copyright 2011-2012 Calxeda, Inc.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Adapted from the highbank_mc_edac driver
+ *
+ */
+#include <linux/types.h>
+#include <linux/kernel.h>
+#include <linux/ctype.h>
+#include <linux/edac.h>
+#include <linux/interrupt.h>
+#include <linux/platform_device.h>
+#include <linux/of_platform.h>
+#include <linux/uaccess.h>
+#include <linux/mfd/syscon.h>
+#include <linux/regmap.h>
+
+#include "edac_core.h"
+#include "edac_module.h"
+
+#define ALTR_EDAC_MOD_STR	"altera_edac"
+
+/* SDRAM Controller CtrlCfg Register */
+#define ALTR_SDR_CTLCFG                 0x00
+
+/* SDRAM Controller CtrlCfg Register Bit Masks */
+#define ALTR_SDR_CTLCFG_ECC_EN          0x400
+#define ALTR_SDR_CTLCFG_ECC_CORR_EN     0x800
+#define ALTR_SDR_CTLCFG_GEN_SB_ERR      0x2000
+#define ALTR_SDR_CTLCFG_GEN_DB_ERR      0x4000
+
+#define ALTR_SDR_CTLCFG_ECC_AUTO_EN     (ALTR_SDR_CTLCFG_ECC_EN | \
+					ALTR_SDR_CTLCFG_ECC_CORR_EN)
+
+/* SDRAM Controller DRAM Status Register */
+#define ALTR_SDR_DRAMSTS                0x38
+
+/* SDRAM Controller DRAM Status Register Bit Masks */
+#define ALTR_SDR_DRAMSTS_SBEERR         0x04
+#define ALTR_SDR_DRAMSTS_DBEERR         0x08
+#define ALTR_SDR_DRAMSTS_CORR_DROP      0x10
+
+/* SDRAM Controller DRAM IRQ Register */
+#define ALTR_SDR_DRAMINTR                0x3C
+
+/* SDRAM Controller DRAM IRQ Register Bit Masks */
+#define ALTR_SDR_DRAMINTR_INTREN        0x01
+#define ALTR_SDR_DRAMINTR_SBEMASK       0x02
+#define ALTR_SDR_DRAMINTR_DBEMASK       0x04
+#define ALTR_SDR_DRAMINTR_CORRDROPMASK  0x08
+#define ALTR_SDR_DRAMINTR_INTRCLR       0x10
+
+/* SDRAM Controller Single Bit Error Count Register */
+#define ALTR_SDR_SBECOUNT               0x40
+
+/* SDRAM Controller Single Bit Error Count Register Bit Masks */
+#define ALTR_SDR_SBECOUNT_COUNT         0x0F
+
+/* SDRAM Controller Double Bit Error Count Register */
+#define ALTR_SDR_DBECOUNT               0x44
+
+/* SDRAM Controller Double Bit Error Count Register Bit Masks */
+#define ALTR_SDR_DBECOUNT_COUNT         0x0F
+
+/* SDRAM Controller ECC Error Address Register */
+#define ALTR_SDR_ERRADDR                0x48
+
+/* SDRAM Controller ECC Error Address Register Bit Masks */
+#define ALTR_SDR_ERRADDR_ADDR           0xFFFFFFFF
+
+/* SDRAM Controller ECC Autocorrect Drop Count Register */
+#define ALTR_SDR_DROPCOUNT              0x4C
+
+/* SDRAM Controller ECC Autocorrect Drop Count Register Bit Masks */
+#define ALTR_SDR_DROPCOUNT_CORR         0x0F
+
+/* SDRAM Controller ECC AutoCorrect Address Register */
+#define ALTR_SDR_DROPADDR               0x50
+
+/* SDRAM Controller ECC AutoCorrect Error Address Register Bit Masks */
+#define ALTR_SDR_DROPADDR_ADDR          0xFFFFFFFF
+
+/* Altera SDRAM Memory Controller data */
+struct altr_sdram_mc_data {
+	struct regmap *mc_vbase;
+};
+
+static irqreturn_t altr_sdram_mc_err_handler(int irq, void *dev_id)
+{
+	struct mem_ctl_info *mci = dev_id;
+	struct altr_sdram_mc_data *drvdata = mci->pvt_info;
+	u32 status = 0, err_count = 0, err_addr = 0;
+
+	/* Error Address is shared by both SBE & DBE */
+	regmap_read(drvdata->mc_vbase, ALTR_SDR_ERRADDR, &err_addr);
+
+	regmap_read(drvdata->mc_vbase, ALTR_SDR_DRAMSTS, &status);
+
+	if (status & ALTR_SDR_DRAMSTS_DBEERR) {
+		regmap_read(drvdata->mc_vbase, ALTR_SDR_DBECOUNT, &err_count);
+		panic("\nEDAC: [%d Uncorrectable errors @ 0x%08X]\n",
+		      err_count, err_addr);
+	}
+	if (status & ALTR_SDR_DRAMSTS_SBEERR) {
+		regmap_read(drvdata->mc_vbase, ALTR_SDR_SBECOUNT, &err_count);
+		edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, err_count,
+				     err_addr >> PAGE_SHIFT,
+				     err_addr & ~PAGE_MASK, 0,
+				     0, 0, -1, mci->ctl_name, "");
+	}
+
+	regmap_write(drvdata->mc_vbase,	ALTR_SDR_DRAMINTR,
+		     (ALTR_SDR_DRAMINTR_INTRCLR | ALTR_SDR_DRAMINTR_INTREN));
+
+	return IRQ_HANDLED;
+}
+
+#ifdef CONFIG_EDAC_DEBUG
+static ssize_t altr_sdr_mc_err_inject_write(struct file *file,
+					const char __user *data,
+					size_t count, loff_t *ppos)
+{
+	struct mem_ctl_info *mci = file->private_data;
+	struct altr_sdram_mc_data *drvdata = mci->pvt_info;
+	u32 *ptemp;
+	dma_addr_t dma_handle;
+	u32 reg, read_reg = 0;
+
+	ptemp = dma_alloc_coherent(mci->pdev, 16, &dma_handle, GFP_KERNEL);
+	if (IS_ERR(ptemp)) {
+		dma_free_coherent(mci->pdev, 16, ptemp, dma_handle);
+		dev_err(mci->pdev, "**EDAC Inject: Buffer Allocation error\n");
+		return -ENOMEM;
+	}
+
+	regmap_read(drvdata->mc_vbase, ALTR_SDR_CTLCFG, &read_reg);
+	read_reg &= ~(ALTR_SDR_CTLCFG_GEN_SB_ERR | ALTR_SDR_CTLCFG_GEN_DB_ERR);
+
+	if (count == 3) {
+		dev_alert(mci->pdev, "** EDAC Inject Double bit error\n");
+		regmap_write(drvdata->mc_vbase, ALTR_SDR_CTLCFG,
+			     (read_reg | ALTR_SDR_CTLCFG_GEN_DB_ERR));
+	} else {
+		dev_alert(mci->pdev, "** EDAC Inject Single bit error\n");
+		regmap_write(drvdata->mc_vbase,	ALTR_SDR_CTLCFG,
+			     (read_reg | ALTR_SDR_CTLCFG_GEN_SB_ERR));
+	}
+
+	ptemp[0] = 0x5A5A5A5A;
+	ptemp[1] = 0xA5A5A5A5;
+	regmap_write(drvdata->mc_vbase,	ALTR_SDR_CTLCFG, read_reg);
+	/* Ensure it has been written out */
+	wmb();
+
+	reg = ptemp[0];
+	read_reg = ptemp[1];
+
+	dma_free_coherent(mci->pdev, 16, ptemp, dma_handle);
+
+	return count;
+}
+
+static const struct file_operations altr_sdr_mc_debug_inject_fops = {
+	.open = simple_open,
+	.write = altr_sdr_mc_err_inject_write,
+	.llseek = generic_file_llseek,
+};
+
+static void altr_sdr_mc_create_debugfs_nodes(struct mem_ctl_info *mci)
+{
+	if (mci->debugfs)
+		debugfs_create_file("inject_ctrl", S_IWUSR, mci->debugfs, mci,
+				    &altr_sdr_mc_debug_inject_fops);
+}
+#else
+static void altr_sdr_mc_create_debugfs_nodes(struct mem_ctl_info *mci)
+{}
+#endif
+
+/* Get total memory size from Open Firmware DTB */
+static u32 altr_sdram_get_total_mem_size(void)
+{
+	struct device_node *np;
+	u32 retcode, reg_array[2];
+
+	np = of_find_node_by_type(NULL, "memory");
+	if (!np)
+		return 0;
+
+	retcode = of_property_read_u32_array(np, "reg",
+					     reg_array, ARRAY_SIZE(reg_array));
+
+	of_node_put(np);
+
+	if (retcode)
+		return 0;
+
+	return reg_array[1];
+}
+
+static int altr_sdram_mc_probe(struct platform_device *pdev)
+{
+	struct edac_mc_layer layers[2];
+	struct mem_ctl_info *mci;
+	struct altr_sdram_mc_data *drvdata;
+	struct dimm_info *dimm;
+	u32 read_reg, mem_size;
+	int irq;
+	int res = 0, retcode;
+
+	layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
+	layers[0].size = 1;
+	layers[0].is_virt_csrow = true;
+	layers[1].type = EDAC_MC_LAYER_CHANNEL;
+	layers[1].size = 1;
+	layers[1].is_virt_csrow = false;
+	mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers,
+			    sizeof(struct altr_sdram_mc_data));
+	if (!mci)
+		return -ENOMEM;
+
+	mci->pdev = &pdev->dev;
+	drvdata = mci->pvt_info;
+	platform_set_drvdata(pdev, mci);
+
+	if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) {
+		edac_mc_free(mci);
+		return -ENOMEM;
+	}
+
+	/* Grab the register values from the sdr-ctl in device tree */
+	drvdata->mc_vbase = syscon_regmap_lookup_by_compatible("altr,sdr-ctl");
+	if (IS_ERR(drvdata->mc_vbase)) {
+		dev_err(&pdev->dev,
+			"regmap for altr,sdr-ctl lookup failed.\n");
+		res = -ENODEV;
+		goto err;
+	}
+
+	retcode = regmap_read(drvdata->mc_vbase, ALTR_SDR_CTLCFG, &read_reg);
+	if (retcode || ((read_reg & ALTR_SDR_CTLCFG_ECC_AUTO_EN) !=
+		ALTR_SDR_CTLCFG_ECC_AUTO_EN)) {
+		dev_err(&pdev->dev, "No ECC present / ECC disabled - 0x%08X\n",
+			read_reg);
+		res = -ENODEV;
+		goto err;
+	}
+
+	mci->mtype_cap = MEM_FLAG_DDR3;
+	mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED;
+	mci->edac_cap = EDAC_FLAG_SECDED;
+	mci->mod_name = ALTR_EDAC_MOD_STR;
+	mci->mod_ver = "1";
+	mci->ctl_name = dev_name(&pdev->dev);
+	mci->scrub_mode = SCRUB_SW_SRC;
+	mci->dev_name = dev_name(&pdev->dev);
+
+	/* Grab memory size from device tree. */
+	mem_size = altr_sdram_get_total_mem_size();
+	dev_dbg(&pdev->dev, "EDAC Memory Size = 0x%08x\n", mem_size);
+	dimm = *mci->dimms;
+	if (mem_size <= 0) {
+		dev_err(&pdev->dev, "Unable to find memory size (dts)\n");
+		res = -ENODEV;
+		goto err;
+	}
+	dimm->nr_pages = ((mem_size - 1) >> PAGE_SHIFT) + 1;
+	dimm->grain = 8;
+	dimm->dtype = DEV_X8;
+	dimm->mtype = MEM_DDR3;
+	dimm->edac_mode = EDAC_SECDED;
+
+	res = edac_mc_add_mc(mci);
+	if (res < 0)
+		goto err;
+
+	retcode = regmap_write(drvdata->mc_vbase, ALTR_SDR_DRAMINTR,
+			ALTR_SDR_DRAMINTR_INTRCLR);
+	if (retcode) {
+		dev_err(&pdev->dev, "Error clearing SDRAM ECC IRQ\n");
+		res = -ENODEV;
+		goto err;
+	}
+
+	irq = platform_get_irq(pdev, 0);
+	res = devm_request_irq(&pdev->dev, irq, altr_sdram_mc_err_handler,
+				0, dev_name(&pdev->dev), mci);
+	if (res < 0) {
+		dev_err(&pdev->dev, "Unable to request irq %d\n", irq);
+		res = -ENODEV;
+		goto err;
+	}
+
+	retcode = regmap_write(drvdata->mc_vbase, ALTR_SDR_DRAMINTR,
+		(ALTR_SDR_DRAMINTR_INTRCLR | ALTR_SDR_DRAMINTR_INTREN));
+	if (retcode) {
+		dev_err(&pdev->dev, "Error enabling SDRAM ECC IRQ\n");
+		res = -ENODEV;
+		goto err2;
+	}
+
+	altr_sdr_mc_create_debugfs_nodes(mci);
+
+	devres_close_group(&pdev->dev, NULL);
+
+	return 0;
+err2:
+	edac_mc_del_mc(&pdev->dev);
+err:
+	dev_err(&pdev->dev, "EDAC Probe Failed; Error %d\n", res);
+	devres_release_group(&pdev->dev, NULL);
+	edac_mc_free(mci);
+
+	return res;
+}
+
+static int altr_sdram_mc_remove(struct platform_device *pdev)
+{
+	struct mem_ctl_info *mci = platform_get_drvdata(pdev);
+
+	edac_mc_del_mc(&pdev->dev);
+	edac_mc_free(mci);
+	platform_set_drvdata(pdev, NULL);
+
+	return 0;
+}
+
+static const struct of_device_id altr_sdram_ctrl_of_match[] = {
+	{ .compatible = "altr,sdram-edac", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, altr_sdram_ctrl_of_match);
+
+static struct platform_driver altr_sdram_mc_edac_driver = {
+	.probe = altr_sdram_mc_probe,
+	.remove = altr_sdram_mc_remove,
+	.driver = {
+		.name = "altr_sdram_mc_edac",
+		.of_match_table = of_match_ptr(altr_sdram_ctrl_of_match),
+	},
+};
+
+module_platform_driver(altr_sdram_mc_edac_driver);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Altera Corporation");
+MODULE_DESCRIPTION("EDAC Driver for Altera SDRAM Controller");
-- 
1.7.9.5

Re: [PATCH 3/3] edac: altera: Add SDRAM EDAC support for CycloneV/ArriaV

[Borislav Petkov]

On Mon, Apr 07, 2014 at 04:54:09PM -0500, tthayer@altera.com wrote:
> From: Thor Thayer <tthayer@altera.com>
> 
> Added EDAC support for reporting ECC errors of CycloneV
> and ArriaV SDRAM controller.
> - The SDRAM Controller registers are used by the FPGA bridge so
>   these are accessed through the syscon interface.
> - The configuration of the SDRAM memory size for the EDAC framework
>   is discovered from the memory node of the device tree.
> - Documentation of the bindings in devicetree/bindings/arm/altera/
>   socfpga-sdram-edac.txt
> - Correction of single bit errors, detection of double bit errors.

Before I go and take a look at this further, is anyone at Altera going
to maintain this driver in case of bug reports and issues with it?

Also, I see patch 3/3. Are the other two related? I see they are
devicetree additions and, as such, all three should go together...

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3/3] edac: altera: Add SDRAM EDAC support for CycloneV/ArriaV

[Thor Thayer]

On Tue, 2014-04-08 at 12:08 +0200, Borislav Petkov wrote:
> On Mon, Apr 07, 2014 at 04:54:09PM -0500, tthayer@altera.com wrote:
> > From: Thor Thayer <tthayer@altera.com>
> > 
> > Added EDAC support for reporting ECC errors of CycloneV
> > and ArriaV SDRAM controller.
> > - The SDRAM Controller registers are used by the FPGA bridge so
> >   these are accessed through the syscon interface.
> > - The configuration of the SDRAM memory size for the EDAC framework
> >   is discovered from the memory node of the device tree.
> > - Documentation of the bindings in devicetree/bindings/arm/altera/
> >   socfpga-sdram-edac.txt
> > - Correction of single bit errors, detection of double bit errors.
> 
> Before I go and take a look at this further, is anyone at Altera going
> to maintain this driver in case of bug reports and issues with it?

Yes, Altera has a group specifically supporting Linux drivers on the
Altera SoCs.

> 
> Also, I see patch 3/3. Are the other two related? I see they are
> devicetree additions and, as such, all three should go together...
> 

I was told that the device tree additions should be separate patches in
a series since the device tree additions go to a separate group for
approval.

Re: [PATCH 3/3] edac: altera: Add SDRAM EDAC support for CycloneV/ArriaV

[Borislav Petkov]

On Tue, Apr 08, 2014 at 08:57:39AM -0500, Thor Thayer wrote:
> Yes, Altera has a group specifically supporting Linux drivers on the
> Altera SoCs.

Then please add MAINTAINERS file entry for this EDAC driver so that
people can send issues/reports to that group.

> I was told that the device tree additions should be separate patches
> in a series since the device tree additions go to a separate group for
> approval.

Well, the EDAC driver depends on the devicetree stuff, right? If so,
they should go together.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3/3] edac: altera: Add SDRAM EDAC support for CycloneV/ArriaV

[Mark Rutland]

On Tue, Apr 08, 2014 at 04:24:06PM +0100, Borislav Petkov wrote:
> On Tue, Apr 08, 2014 at 08:57:39AM -0500, Thor Thayer wrote:
> > Yes, Altera has a group specifically supporting Linux drivers on the
> > Altera SoCs.
> 
> Then please add MAINTAINERS file entry for this EDAC driver so that
> people can send issues/reports to that group.
> 
> > I was told that the device tree additions should be separate patches
> > in a series since the device tree additions go to a separate group for
> > approval.
> 
> Well, the EDAC driver depends on the devicetree stuff, right? If so,
> they should go together.

The patches should be in the same series, but for review purposes it's
nicer if the bindings are separate patches from the code within that
series.

I usually look at the drivers implementing bindings and prefer to be
Cc'd on the whole series, with both the binding and driver.

Cheers,
Mark.

Re: [PATCH 3/3] edac: altera: Add SDRAM EDAC support for CycloneV/ArriaV

[Borislav Petkov]

On Tue, Apr 08, 2014 at 04:40:17PM +0100, Mark Rutland wrote:
> The patches should be in the same series, but for review purposes it's
> nicer if the bindings are separate patches from the code within that
> series.
>
> I usually look at the drivers implementing bindings and prefer to be
> Cc'd on the whole series, with both the binding and driver.

Right, but I got only the 3/3 patch on linux-edac which made me wonder.

So I'm guessing you or someone else would be picking the whole set once
I've acked the EDAC part?

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3/3] edac: altera: Add SDRAM EDAC support for CycloneV/ArriaV

[Mark Rutland]

On Tue, Apr 08, 2014 at 05:03:51PM +0100, Borislav Petkov wrote:
> On Tue, Apr 08, 2014 at 04:40:17PM +0100, Mark Rutland wrote:
> > The patches should be in the same series, but for review purposes it's
> > nicer if the bindings are separate patches from the code within that
> > series.
> >
> > I usually look at the drivers implementing bindings and prefer to be
> > Cc'd on the whole series, with both the binding and driver.
> 
> Right, but I got only the 3/3 patch on linux-edac which made me wonder.

Ah. The whole series should've been Cc'd to linux-edac; I didn't realise
that wasn't the case.

> So I'm guessing you or someone else would be picking the whole set once
> I've acked the EDAC part?

Typically the bindings would go with the driver via the appropriate
subsystem maintainer. That way we don't get bindings without drivers or
vice-versa if there's a problem part-way, and we don't end up with every
other driver going via a dt tree.

Cheers,
Mark.

Re: [PATCH 3/3] edac: altera: Add SDRAM EDAC support for CycloneV/ArriaV

[Borislav Petkov]

On Tue, Apr 08, 2014 at 05:10:54PM +0100, Mark Rutland wrote:
> Typically the bindings would go with the driver via the appropriate
> subsystem maintainer. That way we don't get bindings without drivers
> or vice-versa if there's a problem part-way, and we don't end up with
> every other driver going via a dt tree.

Ok, I can pick them up then, once review is done.

@Thor, please CC linux-edac on the whole patchset for your next
submission.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 3/3] edac: altera: Add SDRAM EDAC support for CycloneV/ArriaV

[Thor Thayer]

On Tue, 2014-04-08 at 18:22 +0200, Borislav Petkov wrote:
> On Tue, Apr 08, 2014 at 05:10:54PM +0100, Mark Rutland wrote:
> > Typically the bindings would go with the driver via the appropriate
> > subsystem maintainer. That way we don't get bindings without drivers
> > or vice-versa if there's a problem part-way, and we don't end up with
> > every other driver going via a dt tree.
> 
> Ok, I can pick them up then, once review is done.
> 
> @Thor, please CC linux-edac on the whole patchset for your next
> submission.

OK. Sorry. I'll add that on V2. Thanks for the gentle pointers.
> 
> Thanks.
> 

Re: [PATCH 3/3] edac: altera: Add SDRAM EDAC support for CycloneV/ArriaV

[Mark Rutland]

On Mon, Apr 07, 2014 at 10:54:09PM +0100, tthayer@altera.com wrote:
> From: Thor Thayer <tthayer@altera.com>
> 
> Added EDAC support for reporting ECC errors of CycloneV
> and ArriaV SDRAM controller.
> - The SDRAM Controller registers are used by the FPGA bridge so
>   these are accessed through the syscon interface.
> - The configuration of the SDRAM memory size for the EDAC framework
>   is discovered from the memory node of the device tree.
> - Documentation of the bindings in devicetree/bindings/arm/altera/
>   socfpga-sdram-edac.txt
> - Correction of single bit errors, detection of double bit errors.
> 
> Signed-off-by: Thor Thayer <tthayer@altera.com>
> To: Rob Herring <robherring2@gmail.com>
> To: Doug Thompson <dougthompson@xmission.com>
> To: Grant Likely <grant.likely@linaro.org>
> Cc: Dinh Nguyen <dinguyen@altera.com>
> Cc: devicetree@vger.kernel.org
> Cc: linux-edac@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/edac/Kconfig          |    9 ++
>  drivers/edac/Makefile         |    2 +
>  drivers/edac/altera_mc_edac.c |  360 +++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 371 insertions(+)
>  create mode 100644 drivers/edac/altera_mc_edac.c

[...]

> +/* Get total memory size from Open Firmware DTB */
> +static u32 altr_sdram_get_total_mem_size(void)
> +{
> +       struct device_node *np;
> +       u32 retcode, reg_array[2];
> +
> +       np = of_find_node_by_type(NULL, "memory");
> +       if (!np)
> +               return 0;
> +
> +       retcode = of_property_read_u32_array(np, "reg",
> +                                            reg_array, ARRAY_SIZE(reg_array));

There's no requirement that #address-cells = <1> or #size-cells = <1>,
even if any values in either would fit into 32 bits. If we must read
this from the DTB rather than elsewhere, please check
of_n_{addr,size}_cells.

Additionally, it's possible that the physical memory might be described
over multiple reg entries, or multiple memory nodes for some arbitrary
reason.

Can we not get this info from elsewhere rather than having to parse the
memory node within a driver?

Cheers,
Mark.

Re: [PATCH 3/3] edac: altera: Add SDRAM EDAC support for CycloneV/ArriaV

[Steffen Trumtrar]

On Tue, Apr 08, 2014 at 11:45:25AM +0100, Mark Rutland wrote:
> On Mon, Apr 07, 2014 at 10:54:09PM +0100, tthayer@altera.com wrote:
> > From: Thor Thayer <tthayer@altera.com>
> > 
> > Added EDAC support for reporting ECC errors of CycloneV
> > and ArriaV SDRAM controller.
> > - The SDRAM Controller registers are used by the FPGA bridge so
> >   these are accessed through the syscon interface.
> > - The configuration of the SDRAM memory size for the EDAC framework
> >   is discovered from the memory node of the device tree.
> > - Documentation of the bindings in devicetree/bindings/arm/altera/
> >   socfpga-sdram-edac.txt
> > - Correction of single bit errors, detection of double bit errors.
> > 
> > Signed-off-by: Thor Thayer <tthayer@altera.com>
> > To: Rob Herring <robherring2@gmail.com>
> > To: Doug Thompson <dougthompson@xmission.com>
> > To: Grant Likely <grant.likely@linaro.org>
> > Cc: Dinh Nguyen <dinguyen@altera.com>
> > Cc: devicetree@vger.kernel.org
> > Cc: linux-edac@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> >  drivers/edac/Kconfig          |    9 ++
> >  drivers/edac/Makefile         |    2 +
> >  drivers/edac/altera_mc_edac.c |  360 +++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 371 insertions(+)
> >  create mode 100644 drivers/edac/altera_mc_edac.c
> 
> [...]
> 
> > +/* Get total memory size from Open Firmware DTB */
> > +static u32 altr_sdram_get_total_mem_size(void)
> > +{
> > +       struct device_node *np;
> > +       u32 retcode, reg_array[2];
> > +
> > +       np = of_find_node_by_type(NULL, "memory");
> > +       if (!np)
> > +               return 0;
> > +
> > +       retcode = of_property_read_u32_array(np, "reg",
> > +                                            reg_array, ARRAY_SIZE(reg_array));
> 
> There's no requirement that #address-cells = <1> or #size-cells = <1>,
> even if any values in either would fit into 32 bits. If we must read
> this from the DTB rather than elsewhere, please check
> of_n_{addr,size}_cells.
> 
> Additionally, it's possible that the physical memory might be described
> over multiple reg entries, or multiple memory nodes for some arbitrary
> reason.
> 
> Can we not get this info from elsewhere rather than having to parse the
> memory node within a driver?
> 

It should be possible to calculate this from the dramaddrw register in the
SDRAM controller.

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-5555 |

Re: [PATCH 3/3] edac: altera: Add SDRAM EDAC support for CycloneV/ArriaV

[Thor Thayer]

On Tue, 2014-04-08 at 14:45 +0200, Steffen Trumtrar wrote:
> On Tue, Apr 08, 2014 at 11:45:25AM +0100, Mark Rutland wrote:
> > On Mon, Apr 07, 2014 at 10:54:09PM +0100, tthayer@altera.com wrote:
> > > From: Thor Thayer <tthayer@altera.com>
> > > 
> > > Added EDAC support for reporting ECC errors of CycloneV
> > > and ArriaV SDRAM controller.
> > > - The SDRAM Controller registers are used by the FPGA bridge so
> > >   these are accessed through the syscon interface.
> > > - The configuration of the SDRAM memory size for the EDAC framework
> > >   is discovered from the memory node of the device tree.
> > > - Documentation of the bindings in devicetree/bindings/arm/altera/
> > >   socfpga-sdram-edac.txt
> > > - Correction of single bit errors, detection of double bit errors.
> > > 
> > > Signed-off-by: Thor Thayer <tthayer@altera.com>
> > > To: Rob Herring <robherring2@gmail.com>
> > > To: Doug Thompson <dougthompson@xmission.com>
> > > To: Grant Likely <grant.likely@linaro.org>
> > > Cc: Dinh Nguyen <dinguyen@altera.com>
> > > Cc: devicetree@vger.kernel.org
> > > Cc: linux-edac@vger.kernel.org
> > > Cc: linux-kernel@vger.kernel.org
> > > ---
> > >  drivers/edac/Kconfig          |    9 ++
> > >  drivers/edac/Makefile         |    2 +
> > >  drivers/edac/altera_mc_edac.c |  360 +++++++++++++++++++++++++++++++++++++++++
> > >  3 files changed, 371 insertions(+)
> > >  create mode 100644 drivers/edac/altera_mc_edac.c
> > 
> > [...]
> > 
> > > +/* Get total memory size from Open Firmware DTB */
> > > +static u32 altr_sdram_get_total_mem_size(void)
> > > +{
> > > +       struct device_node *np;
> > > +       u32 retcode, reg_array[2];
> > > +
> > > +       np = of_find_node_by_type(NULL, "memory");
> > > +       if (!np)
> > > +               return 0;
> > > +
> > > +       retcode = of_property_read_u32_array(np, "reg",
> > > +                                            reg_array, ARRAY_SIZE(reg_array));
> > 
> > There's no requirement that #address-cells = <1> or #size-cells = <1>,
> > even if any values in either would fit into 32 bits. If we must read
> > this from the DTB rather than elsewhere, please check
> > of_n_{addr,size}_cells.
> > 
> > Additionally, it's possible that the physical memory might be described
> > over multiple reg entries, or multiple memory nodes for some arbitrary
> > reason.
> > 
> > Can we not get this info from elsewhere rather than having to parse the
> > memory node within a driver?
> > 
> 
> It should be possible to calculate this from the dramaddrw register in the
> SDRAM controller.

Thank you all for the comments. I will look into this further.

> 
> Regards,
> Steffen
>