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

[PATCH 1/2] 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 <rob.herring@calxeda.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..351ab7b
--- /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 and possibly other devices in the future.  See
+		also Documentation/devicetree/bindings/mfd/syscon.txt
+- reg : Should contain 1 register ranges(address and length)
+
+Example:
+	sdrctl@0xffc25000 {
+		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 9e78c1d..3634de7 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -528,6 +528,11 @@
 			reg-io-width = <4>;
 		};
 
+		sdrctl@0xffc25000 {
+			compatible = "altr,sdr-ctl", "syscon";
+			reg = <0xffc25000 0x1000>;
+		};
+
 		rstmgr@ffd05000 {
 			compatible = "altr,rst-mgr";
 			reg = <0xffd05000 0x1000>;
-- 
1.7.9.5

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

[Steffen Trumtrar]

Hi!

On Mon, Mar 31, 2014 at 05:07:06PM -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 <rob.herring@calxeda.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..351ab7b
> --- /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 and possibly other devices in the future.  See
> +		also Documentation/devicetree/bindings/mfd/syscon.txt

This note is really linux specific (and specific to some vendor kernel driver
for that matter). I think you should remove that.

> +- reg : Should contain 1 register ranges(address and length)
> +
> +Example:
> +	sdrctl@0xffc25000 {
> +		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 9e78c1d..3634de7 100644
> --- a/arch/arm/boot/dts/socfpga.dtsi
> +++ b/arch/arm/boot/dts/socfpga.dtsi
> @@ -528,6 +528,11 @@
>  			reg-io-width = <4>;
>  		};
>  
> +		sdrctl@0xffc25000 {
> +			compatible = "altr,sdr-ctl", "syscon";
> +			reg = <0xffc25000 0x1000>;
> +		};
> +
>  		rstmgr@ffd05000 {
>  			compatible = "altr,rst-mgr";
>  			reg = <0xffd05000 0x1000>;

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 1/2] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Thor Thayer - Sendmail]

On Tue, 2014-04-01 at 07:28 +0200, Steffen Trumtrar wrote:
> Hi!
> 
> On Mon, Mar 31, 2014 at 05:07:06PM -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>

> > 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..351ab7b
> > --- /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 and possibly other devices in the future.  See
> > +		also Documentation/devicetree/bindings/mfd/syscon.txt
> 
> This note is really linux specific (and specific to some vendor kernel driver
> for that matter). I think you should remove that.

Hi Steffen. Are you asking for the path to syscon.txt to be removed or
the entire note? 
The note is explaining why the "syscon" tag is used for this Altera IP
block (registers shared between multiple drivers).
Thanks for the clarification.

> 
> > +- reg : Should contain 1 register ranges(address and length)
> > +
> > +Example:
> > +	sdrctl@0xffc25000 {
> > +		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 9e78c1d..3634de7 100644
> > --- a/arch/arm/boot/dts/socfpga.dtsi
> > +++ b/arch/arm/boot/dts/socfpga.dtsi
> > @@ -528,6 +528,11 @@
> >  			reg-io-width = <4>;
> >  		};
> >  
> > +		sdrctl@0xffc25000 {
> > +			compatible = "altr,sdr-ctl", "syscon";
> > +			reg = <0xffc25000 0x1000>;
> > +		};
> > +
> >  		rstmgr@ffd05000 {
> >  			compatible = "altr,rst-mgr";
> >  			reg = <0xffd05000 0x1000>;
> 
> Regards,
> Steffen
> 

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

[Steffen Trumtrar]

Hi!

On Tue, Apr 01, 2014 at 03:11:41PM -0500, Thor Thayer - Sendmail wrote:
> On Tue, 2014-04-01 at 07:28 +0200, Steffen Trumtrar wrote:
> > Hi!
> > 
> > On Mon, Mar 31, 2014 at 05:07:06PM -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>
> 
> > > 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..351ab7b
> > > --- /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 and possibly other devices in the future.  See
> > > +		also Documentation/devicetree/bindings/mfd/syscon.txt
> > 
> > This note is really linux specific (and specific to some vendor kernel driver
> > for that matter). I think you should remove that.
> 
> Hi Steffen. Are you asking for the path to syscon.txt to be removed or
> the entire note? 
> The note is explaining why the "syscon" tag is used for this Altera IP
> block (registers shared between multiple drivers).
> Thanks for the clarification.
> 

I was referring to the note. I don't know what the current guideline is for
this type of "devices", but I think referring to the syscon documentation
is okay.
The first part however describes a DT binding with things that might or might
not happen in the future. What I want to say is, I would rather have this
reworded, to have it just describe the SDRAM controller node as simple as it may
be.

> > 
> > > +- reg : Should contain 1 register ranges(address and length)
> > > +
> > > +Example:
> > > +	sdrctl@0xffc25000 {
> > > +		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 9e78c1d..3634de7 100644
> > > --- a/arch/arm/boot/dts/socfpga.dtsi
> > > +++ b/arch/arm/boot/dts/socfpga.dtsi
> > > @@ -528,6 +528,11 @@
> > >  			reg-io-width = <4>;
> > >  		};
> > >  
> > > +		sdrctl@0xffc25000 {
                       ^^
Oh, and Dinh just mentioned in my patch, that the 0x shall be removed here.
But I'm not so sure about that. Just FYI.

> > > +			compatible = "altr,sdr-ctl", "syscon";
> > > +			reg = <0xffc25000 0x1000>;
> > > +		};
> > > +
> > >  		rstmgr@ffd05000 {
> > >  			compatible = "altr,rst-mgr";
> > >  			reg = <0xffd05000 0x1000>;

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 1/2] dts: socfpga: Add bindings for Altera SoC SDRAM controller

[Thor Thayer - Sendmail]

On Wed, 2014-04-02 at 00:19 +0200, Steffen Trumtrar wrote:
> Hi!
> 
> On Tue, Apr 01, 2014 at 03:11:41PM -0500, Thor Thayer - Sendmail wrote:
> > On Tue, 2014-04-01 at 07:28 +0200, Steffen Trumtrar wrote:
> > > Hi!
> > > 
> > > On Mon, Mar 31, 2014 at 05:07:06PM -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>
> > 
> > > > 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..351ab7b
> > > > --- /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 and possibly other devices in the future.  See
> > > > +		also Documentation/devicetree/bindings/mfd/syscon.txt
> > > 
> > > This note is really linux specific (and specific to some vendor kernel driver
> > > for that matter). I think you should remove that.
> > 
> > Hi Steffen. Are you asking for the path to syscon.txt to be removed or
> > the entire note? 
> > The note is explaining why the "syscon" tag is used for this Altera IP
> > block (registers shared between multiple drivers).
> > Thanks for the clarification.
> > 
> 
> I was referring to the note. I don't know what the current guideline is for
> this type of "devices", but I think referring to the syscon documentation
> is okay.
> The first part however describes a DT binding with things that might or might
> not happen in the future. What I want to say is, I would rather have this
> reworded, to have it just describe the SDRAM controller node as simple as it may
> be.

I see the confusion. It was not written well. I'll abandon this patch. 

I have another patch series ready that uses the "syscon" parameter of
the SDRAM controller so I'll include this patch in that series. The need
for the "syscon" will be more apparent. Thanks!
> 
> > > 
> > > > +- reg : Should contain 1 register ranges(address and length)
> > > > +
> > > > +Example:
> > > > +	sdrctl@0xffc25000 {
> > > > +		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 9e78c1d..3634de7 100644
> > > > --- a/arch/arm/boot/dts/socfpga.dtsi
> > > > +++ b/arch/arm/boot/dts/socfpga.dtsi
> > > > @@ -528,6 +528,11 @@
> > > >  			reg-io-width = <4>;
> > > >  		};
> > > >  
> > > > +		sdrctl@0xffc25000 {
>                        ^^
> Oh, and Dinh just mentioned in my patch, that the 0x shall be removed here.
> But I'm not so sure about that. Just FYI.
> 
Thanks.
> > > > +			compatible = "altr,sdr-ctl", "syscon";
> > > > +			reg = <0xffc25000 0x1000>;
> > > > +		};
> > > > +
> > > >  		rstmgr@ffd05000 {
> > > >  			compatible = "altr,rst-mgr";
> > > >  			reg = <0xffd05000 0x1000>;
> 
> Regards,
> Steffen
> 

[PATCH 2/2] arm: socfpga: Add support for Altera SoC SDRAM controller

[tthayer]

From: Thor Thayer <tthayer@altera.com>

Addition of the Altera SDRAM controller registers to the
Altera SoC project. These registers are shared by future
drivers such as ECC and the FPGA bridge.

Signed-off-by: Thor Thayer <tthayer@altera.com>
To: Rob Herring <rob.herring@calxeda.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
---
 arch/arm/mach-socfpga/socfpga.c |   10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c
index 21d6026..d514e8a 100644
--- a/arch/arm/mach-socfpga/socfpga.c
+++ b/arch/arm/mach-socfpga/socfpga.c
@@ -32,6 +32,7 @@
 void __iomem *socfpga_scu_base_addr = ((void __iomem *)(SOCFPGA_SCU_VIRT_BASE));
 void __iomem *sys_manager_base_addr;
 void __iomem *rst_manager_base_addr;
+void __iomem *sdr_ctl_base_addr;
 void __iomem *clk_mgr_base_addr;
 unsigned long cpu1start_addr;
 
@@ -150,6 +151,15 @@ void __init socfpga_sysmgr_init(void)
 
 	np = of_find_compatible_node(NULL, NULL, "altr,clk-mgr");
 	clk_mgr_base_addr = of_iomap(np, 0);
+
+	np = of_find_compatible_node(NULL, NULL, "altr,sdr-ctl");
+	if (!np) {
+		pr_err("SOCFPGA: Unable to find sdr-ctl\n");
+		return;
+	}
+
+	sdr_ctl_base_addr = of_iomap(np, 0);
+	WARN_ON(!sdr_ctl_base_addr);
 }
 
 static void __init socfpga_init_irq(void)
-- 
1.7.9.5

Re: [PATCH 2/2] arm: socfpga: Add support for Altera SoC SDRAM controller

[Steffen Trumtrar]

On Mon, Mar 31, 2014 at 05:07:07PM -0500, tthayer@altera.com wrote:
> From: Thor Thayer <tthayer@altera.com>
> 
> Addition of the Altera SDRAM controller registers to the
> Altera SoC project. These registers are shared by future
> drivers such as ECC and the FPGA bridge.
> 
> Signed-off-by: Thor Thayer <tthayer@altera.com>
> To: Rob Herring <rob.herring@calxeda.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
> ---
>  arch/arm/mach-socfpga/socfpga.c |   10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c
> index 21d6026..d514e8a 100644
> --- a/arch/arm/mach-socfpga/socfpga.c
> +++ b/arch/arm/mach-socfpga/socfpga.c
> @@ -32,6 +32,7 @@
>  void __iomem *socfpga_scu_base_addr = ((void __iomem *)(SOCFPGA_SCU_VIRT_BASE));
>  void __iomem *sys_manager_base_addr;
>  void __iomem *rst_manager_base_addr;
> +void __iomem *sdr_ctl_base_addr;
>  void __iomem *clk_mgr_base_addr;
>  unsigned long cpu1start_addr;
>  

Can't we get rid of all these global pointers instead of adding to them?

> @@ -150,6 +151,15 @@ void __init socfpga_sysmgr_init(void)
>  
>  	np = of_find_compatible_node(NULL, NULL, "altr,clk-mgr");
>  	clk_mgr_base_addr = of_iomap(np, 0);
> +
> +	np = of_find_compatible_node(NULL, NULL, "altr,sdr-ctl");
> +	if (!np) {
> +		pr_err("SOCFPGA: Unable to find sdr-ctl\n");
> +		return;
> +	}
> +
> +	sdr_ctl_base_addr = of_iomap(np, 0);
> +	WARN_ON(!sdr_ctl_base_addr);
>  }
>  
>  static void __init socfpga_init_irq(void)
> -- 
> 1.7.9.5
> 

-- 
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 2/2] arm: socfpga: Add support for Altera SoC SDRAM controller

[Thor Thayer - Sendmail]

On Tue, 2014-04-01 at 07:33 +0200, Steffen Trumtrar wrote:
> On Mon, Mar 31, 2014 at 05:07:07PM -0500, tthayer@altera.com wrote:
> > From: Thor Thayer <tthayer@altera.com>
> > 
<snip>
> >  
> 
> Can't we get rid of all these global pointers instead of adding to them?

Yes. I will remove this file from the patch. Thanks.

> 
> > @@ -150,6 +151,15 @@ void __init socfpga_sysmgr_init(void)
> >  
> >  	np = of_find_compatible_node(NULL, NULL, "altr,clk-mgr");
> >  	clk_mgr_base_addr = of_iomap(np, 0);
> > +
> > +	np = of_find_compatible_node(NULL, NULL, "altr,sdr-ctl");
> > +	if (!np) {
> > +		pr_err("SOCFPGA: Unable to find sdr-ctl\n");
> > +		return;
> > +	}
> > +
> > +	sdr_ctl_base_addr = of_iomap(np, 0);
> > +	WARN_ON(!sdr_ctl_base_addr);
> >  }
> >  
> >  static void __init socfpga_init_irq(void)
> > -- 
> > 1.7.9.5
> > 
>