[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 at altera.com]

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 at vger.kernel.org
Cc: linux-doc at vger.kernel.org
Cc: linux-kernel at vger.kernel.org
Cc: linux-arm-kernel at 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 at 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 at ffc25000 {
+			compatible = "altr,sdr-ctl", "syscon";
+			reg = <0xffc25000 0x1000>;
+		};
+
 		rstmgr at ffd05000 {
 			compatible = "altr,rst-mgr";
 			reg = <0xffd05000 0x1000>;
-- 
1.7.9.5

[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 at 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 at vger.kernel.org
> Cc: linux-doc at vger.kernel.org
> Cc: linux-kernel at vger.kernel.org
> Cc: linux-arm-kernel at 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 at 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 at ffc25000 {
> +			compatible = "altr,sdr-ctl", "syscon";
> +			reg = <0xffc25000 0x1000>;
> +		};
> +
>  		rstmgr at ffd05000 {
>  			compatible = "altr,rst-mgr";
>  			reg = <0xffd05000 0x1000>;
> -- 
> 1.7.9.5
> 
> 

[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 at 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 at vger.kernel.org
> Cc: linux-doc at vger.kernel.org
> Cc: linux-kernel at vger.kernel.org
> Cc: linux-arm-kernel at 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 at c0000000 {
		ipcore at 0 {
			resets = <&sdr 1>;
			reset-names = "foo";
			resets = <&rst 96>;
			reset-names = "bar";
			(...)
		};

		ipcore at 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 at 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 at ffc25000 {
> +			compatible = "altr,sdr-ctl", "syscon";
> +			reg = <0xffc25000 0x1000>;
> +		};
> +
>  		rstmgr at 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 |

[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 at 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 at c0000000 {
> 		ipcore at 0 {
> 			resets = <&sdr 1>;
> 			reset-names = "foo";
> 			resets = <&rst 96>;
> 			reset-names = "bar";
> 			(...)
> 		};
> 
> 		ipcore at 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 at 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 at ffc25000 {
> > +			compatible = "altr,sdr-ctl", "syscon";
> > +			reg = <0xffc25000 0x1000>;
> > +		};
> > +
> >  		rstmgr at ffd05000 {
> >  			compatible = "altr,rst-mgr";
> >  			reg = <0xffd05000 0x1000>;
> 

[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 at 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 at c0000000 {
> > 		ipcore at 0 {
> > 			resets = <&sdr 1>;
> > 			reset-names = "foo";
> > 			resets = <&rst 96>;
> > 			reset-names = "bar";
> > 			(...)
> > 		};
> > 
> > 		ipcore at 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 |

[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 at 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

[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 at 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

[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 at 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

[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 at 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

[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 at 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

[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 at 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 at altera.com]

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 at vger.kernel.org
Cc: linux-doc at vger.kernel.org
Cc: linux-kernel at vger.kernel.org
Cc: linux-arm-kernel at 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 at 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 at 0 {
+			compatible = "altr,sdram-edac";
+			interrupts = <0 39 4>;
+		};
+
 		rstmgr at ffd05000 {
 			compatible = "altr,rst-mgr";
 			reg = <0xffd05000 0x1000>;
-- 
1.7.9.5

[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 at 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 at vger.kernel.org
> Cc: linux-doc at vger.kernel.org
> Cc: linux-kernel at vger.kernel.org
> Cc: linux-arm-kernel at 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 at 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 at 0 {

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

Cheers,
Mark.

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