| Message ID | 1400169891-29546-2-git-send-email-tthayer@altera.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi! On Thu, May 15, 2014 at 11:04:49AM -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. > > v2: Changes to SoC SDRAM EDAC code. > > v3: Implement code suggestions for SDRAM EDAC code. > > v4: Remove syscon from SDRAM controller bindings. > > v5: No Change, bump version for consistency. > > Signed-off-by: Thor Thayer <tthayer@altera.com> > --- > .../bindings/arm/altera/socfpga-sdram.txt | 11 +++++++++++ > arch/arm/boot/dts/socfpga.dtsi | 5 +++++ > 2 files changed, 16 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..8f8746b > --- /dev/null > +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt > @@ -0,0 +1,11 @@ > +Altera SOCFPGA SDRAM Controller > + > +Required properties: > +- compatible : "altr,sdr-ctl"; > +- reg : Should contain 1 register ranges(address and length) > + > +Example: > + sdrctl@ffc25000 { > + compatible = "altr,sdr-ctl"; > + 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"; ^^^^^^^^^^ Get rid of that, too, please. > + reg = <0xffc25000 0x1000>; > + }; > + How about sdrctl@ffc25000 { compatible = "altr,sdr-ctl"; reg = <0xffc25000 0x1000>; ranges; edac@ffc2502c { compatible = "altr,sdram-edac"; reg = <0xffc2502c 0x50>; interrupts = <0 39 4>; }; }; Then we can later add: sdr-ports: ports@ffc2507c { #reset-cells = <1>; compatible = "altr,sdr-ports"; reg = <0xffc2507c 0x10>; clocks = <&ddr_dqs_clk>; ... }; to use the reset-controller framework for the port resets. > rstmgr@ffd05000 { > compatible = "altr,rst-mgr"; > reg = <0xffd05000 0x1000>; > -- Regards, Steffen
On Fri, May 16, 2014 at 2:53 AM, Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote: > Hi! > > On Thu, May 15, 2014 at 11:04:49AM -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. >> >> v2: Changes to SoC SDRAM EDAC code. >> >> v3: Implement code suggestions for SDRAM EDAC code. >> >> v4: Remove syscon from SDRAM controller bindings. >> >> v5: No Change, bump version for consistency. >> >> Signed-off-by: Thor Thayer <tthayer@altera.com> >> --- >> .../bindings/arm/altera/socfpga-sdram.txt | 11 +++++++++++ >> arch/arm/boot/dts/socfpga.dtsi | 5 +++++ >> 2 files changed, 16 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..8f8746b >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt >> @@ -0,0 +1,11 @@ >> +Altera SOCFPGA SDRAM Controller >> + >> +Required properties: >> +- compatible : "altr,sdr-ctl"; >> +- reg : Should contain 1 register ranges(address and length) >> + >> +Example: >> + sdrctl@ffc25000 { >> + compatible = "altr,sdr-ctl"; >> + 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"; > ^^^^^^^^^^ > > Get rid of that, too, please. Hi Steffan, I believe I need to keep the "syscon". The same register (ctrlcfg) that has the ECC enable bitfield also includes the DRAM configuration bitfields that other drivers will want to access (specifically the FPGA bridge needs this information). Since this register will be shared between drivers, syscon seems like the best solution. I may be misunderstanding something so feel free to elaborate. Thanks for reviewing. Thor >> + reg = <0xffc25000 0x1000>; >> + }; >> + > > How about > > sdrctl@ffc25000 { > compatible = "altr,sdr-ctl"; > reg = <0xffc25000 0x1000>; > ranges; > > edac@ffc2502c { > compatible = "altr,sdram-edac"; > reg = <0xffc2502c 0x50>; > interrupts = <0 39 4>; > }; > }; > > Then we can later add: > > sdr-ports: ports@ffc2507c { > #reset-cells = <1>; > compatible = "altr,sdr-ports"; > reg = <0xffc2507c 0x10>; > clocks = <&ddr_dqs_clk>; > ... > }; > > to use the reset-controller framework for the port resets. > >> 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 |
Hi Thor! On Mon, May 19, 2014 at 01:36:30PM -0500, Thor Thayer wrote: > On Fri, May 16, 2014 at 2:53 AM, Steffen Trumtrar > <s.trumtrar@pengutronix.de> wrote: > > Hi! > > > > On Thu, May 15, 2014 at 11:04:49AM -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. > >> > >> v2: Changes to SoC SDRAM EDAC code. > >> > >> v3: Implement code suggestions for SDRAM EDAC code. > >> > >> v4: Remove syscon from SDRAM controller bindings. > >> > >> v5: No Change, bump version for consistency. > >> > >> Signed-off-by: Thor Thayer <tthayer@altera.com> > >> --- > >> .../bindings/arm/altera/socfpga-sdram.txt | 11 +++++++++++ > >> arch/arm/boot/dts/socfpga.dtsi | 5 +++++ > >> 2 files changed, 16 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..8f8746b > >> --- /dev/null > >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt > >> @@ -0,0 +1,11 @@ > >> +Altera SOCFPGA SDRAM Controller > >> + > >> +Required properties: > >> +- compatible : "altr,sdr-ctl"; > >> +- reg : Should contain 1 register ranges(address and length) > >> + > >> +Example: > >> + sdrctl@ffc25000 { > >> + compatible = "altr,sdr-ctl"; > >> + 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"; > > ^^^^^^^^^^ > > > > Get rid of that, too, please. > > Hi Steffan, > > I believe I need to keep the "syscon". The same register (ctrlcfg) > that has the ECC enable bitfield also includes the DRAM configuration > bitfields that other drivers will want to access (specifically the > FPGA bridge needs this information). Since this register will be > shared between drivers, syscon seems like the best solution. > Hm, from looking at the documentation of the ctrlcfg I can't really understand which bits you would need for the FPGA brigde and why. That all sounds like stuff you would want to set for the specific RAM you are dealing with on a specific board. What bridge are you talking about? The SDRAM bridge? I can see the problem with the ECC enable, though. Regards, Steffen > > sdrctl@ffc25000 { > > compatible = "altr,sdr-ctl"; > > reg = <0xffc25000 0x1000>; > > ranges; > > > > edac@ffc2502c { > > compatible = "altr,sdram-edac"; > > reg = <0xffc2502c 0x50>; > > interrupts = <0 39 4>; > > }; > > }; > > > > Then we can later add: > > > > sdr-ports: ports@ffc2507c { > > #reset-cells = <1>; > > compatible = "altr,sdr-ports"; > > reg = <0xffc2507c 0x10>; > > clocks = <&ddr_dqs_clk>; > > ... > > };
On Mon, May 19, 2014 at 2:12 PM, Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote: > Hi Thor! > > On Mon, May 19, 2014 at 01:36:30PM -0500, Thor Thayer wrote: >> On Fri, May 16, 2014 at 2:53 AM, Steffen Trumtrar >> <s.trumtrar@pengutronix.de> wrote: >> > Hi! >> > >> > On Thu, May 15, 2014 at 11:04:49AM -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. >> >> >> >> v2: Changes to SoC SDRAM EDAC code. >> >> >> >> v3: Implement code suggestions for SDRAM EDAC code. >> >> >> >> v4: Remove syscon from SDRAM controller bindings. >> >> >> >> v5: No Change, bump version for consistency. >> >> >> >> Signed-off-by: Thor Thayer <tthayer@altera.com> >> >> --- >> >> .../bindings/arm/altera/socfpga-sdram.txt | 11 +++++++++++ >> >> arch/arm/boot/dts/socfpga.dtsi | 5 +++++ >> >> 2 files changed, 16 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..8f8746b >> >> --- /dev/null >> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt >> >> @@ -0,0 +1,11 @@ >> >> +Altera SOCFPGA SDRAM Controller >> >> + >> >> +Required properties: >> >> +- compatible : "altr,sdr-ctl"; >> >> +- reg : Should contain 1 register ranges(address and length) >> >> + >> >> +Example: >> >> + sdrctl@ffc25000 { >> >> + compatible = "altr,sdr-ctl"; >> >> + 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"; >> > ^^^^^^^^^^ >> > >> > Get rid of that, too, please. >> >> Hi Steffan, >> >> I believe I need to keep the "syscon". The same register (ctrlcfg) >> that has the ECC enable bitfield also includes the DRAM configuration >> bitfields that other drivers will want to access (specifically the >> FPGA bridge needs this information). Since this register will be >> shared between drivers, syscon seems like the best solution. >> > > Hm, from looking at the documentation of the ctrlcfg I can't really > understand which bits you would need for the FPGA brigde and why. > That all sounds like stuff you would want to set for the specific > RAM you are dealing with on a specific board. > What bridge are you talking about? The SDRAM bridge? > > I can see the problem with the ECC enable, though. > > Regards, > Steffen > Hi Steffen, I'll get more details from the guy working on the FPGA bridge when he gets back in the office. When I started working on EDAC, that register had been allocated by the FPGA bridge driver so we decided to use the syscon to allow sharing of the register. My understanding was that the FPGA bridge as an SDRAM master would allow FPGA devices to access the SDRAM. As part of that process, they may want to read the SDRAM configuration. I'll need to get more details from the driver developer because the FPGA driver is in flux while the appropriate driver architecture is being discussed. Thor >> > sdrctl@ffc25000 { >> > compatible = "altr,sdr-ctl"; >> > reg = <0xffc25000 0x1000>; >> > ranges; >> > >> > edac@ffc2502c { >> > compatible = "altr,sdram-edac"; >> > reg = <0xffc2502c 0x50>; >> > interrupts = <0 39 4>; >> > }; >> > }; >> > >> > Then we can later add: >> > >> > sdr-ports: ports@ffc2507c { >> > #reset-cells = <1>; >> > compatible = "altr,sdr-ports"; >> > reg = <0xffc2507c 0x10>; >> > clocks = <&ddr_dqs_clk>; >> > ... >> > }; > > -- > 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 |
On Mon, May 19, 2014 at 2:37 PM, Thor Thayer <tthayer.linux@gmail.com> wrote: >>> >> 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..8f8746b >>> >> --- /dev/null >>> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt >>> >> @@ -0,0 +1,11 @@ >>> >> +Altera SOCFPGA SDRAM Controller >>> >> + >>> >> +Required properties: >>> >> +- compatible : "altr,sdr-ctl"; >>> >> +- reg : Should contain 1 register ranges(address and length) >>> >> + >>> >> +Example: >>> >> + sdrctl@ffc25000 { >>> >> + compatible = "altr,sdr-ctl"; >>> >> + 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"; >>> > ^^^^^^^^^^ >>> > >>> > Get rid of that, too, please. >>> >>> Hi Steffan, >>> >>> I believe I need to keep the "syscon". The same register (ctrlcfg) >>> that has the ECC enable bitfield also includes the DRAM configuration >>> bitfields that other drivers will want to access (specifically the >>> FPGA bridge needs this information). Since this register will be >>> shared between drivers, syscon seems like the best solution. >>> >> >> Hm, from looking at the documentation of the ctrlcfg I can't really >> understand which bits you would need for the FPGA brigde and why. Hi Steffen, Offset 0x80 in the sdr-ctl is the "fpgaportrst" register. 14 bits wide, defaults to 0. When appropriate bits set to 1 in that reg, it allows an FPGA port to come out of reset (enables that port). Has no other effect on SDRAM configuration. >> That all sounds like stuff you would want to set for the specific >> RAM you are dealing with on a specific board. >> What bridge are you talking about? The SDRAM bridge? Yes, the port allows the FPGA a direct path to the SDRAM. This one register the only register in the sdr that the bridge driver needs. >> >> I can see the problem with the ECC enable, though. >> >> Regards, >> Steffen >> > > Hi Steffen, > > I'll get more details from the guy working on the FPGA bridge when he > gets back in the office. When I started working on EDAC, that register > had been allocated by the FPGA bridge driver so we decided to use the > syscon to allow sharing of the register. > > My understanding was that the FPGA bridge as an SDRAM master would > allow FPGA devices to access the SDRAM. As part of that process, they > may want to read the SDRAM configuration. The FPGA bridge driver doesn't look at the SDRAM configuration. It just wants access to this one register so it can enable or disable the FPGA path to SDRAM. The only function of this bridge driver is to enable/disable the bridge. ~Alan > > I'll need to get more details from the driver developer because the > FPGA driver is in flux while the appropriate driver architecture is > being discussed. > > Thor > > >>> > sdrctl@ffc25000 { >>> > compatible = "altr,sdr-ctl"; >>> > reg = <0xffc25000 0x1000>; >>> > ranges; >>> > >>> > edac@ffc2502c { >>> > compatible = "altr,sdram-edac"; >>> > reg = <0xffc2502c 0x50>; >>> > interrupts = <0 39 4>; >>> > }; >>> > }; >>> > >>> > Then we can later add: >>> > >>> > sdr-ports: ports@ffc2507c { >>> > #reset-cells = <1>; >>> > compatible = "altr,sdr-ports"; >>> > reg = <0xffc2507c 0x10>; >>> > clocks = <&ddr_dqs_clk>; >>> > ... >>> > }; >> >> -- >> 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 | > -- > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/
Hi! On Tue, May 20, 2014 at 09:31:06AM -0500, Alan Tull wrote: > On Mon, May 19, 2014 at 2:37 PM, Thor Thayer <tthayer.linux@gmail.com> wrote: > > >>> >> 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..8f8746b > >>> >> --- /dev/null > >>> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt > >>> >> @@ -0,0 +1,11 @@ > >>> >> +Altera SOCFPGA SDRAM Controller > >>> >> + > >>> >> +Required properties: > >>> >> +- compatible : "altr,sdr-ctl"; > >>> >> +- reg : Should contain 1 register ranges(address and length) > >>> >> + > >>> >> +Example: > >>> >> + sdrctl@ffc25000 { > >>> >> + compatible = "altr,sdr-ctl"; > >>> >> + 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"; > >>> > ^^^^^^^^^^ > >>> > > >>> > Get rid of that, too, please. > >>> > >>> Hi Steffan, > >>> > >>> I believe I need to keep the "syscon". The same register (ctrlcfg) > >>> that has the ECC enable bitfield also includes the DRAM configuration > >>> bitfields that other drivers will want to access (specifically the > >>> FPGA bridge needs this information). Since this register will be > >>> shared between drivers, syscon seems like the best solution. > >>> > >> > >> Hm, from looking at the documentation of the ctrlcfg I can't really > >> understand which bits you would need for the FPGA brigde and why. > > Hi Steffen, > > Offset 0x80 in the sdr-ctl is the "fpgaportrst" register. 14 bits > wide, defaults to 0. When appropriate bits set to 1 in that reg, it > allows an FPGA port to come out of reset (enables that port). Has no > other effect on SDRAM configuration. > > >> That all sounds like stuff you would want to set for the specific > >> RAM you are dealing with on a specific board. > >> What bridge are you talking about? The SDRAM bridge? > > Yes, the port allows the FPGA a direct path to the SDRAM. This one > register the only register in the sdr that the bridge driver needs. > So, what I suggested down ... > >> > >> I can see the problem with the ECC enable, though. > >> > >> Regards, > >> Steffen > >> > > > >>> > sdrctl@ffc25000 { > >>> > compatible = "altr,sdr-ctl"; > >>> > reg = <0xffc25000 0x1000>; > >>> > ranges; > >>> > > >>> > edac@ffc2502c { > >>> > compatible = "altr,sdram-edac"; > >>> > reg = <0xffc2502c 0x50>; > >>> > interrupts = <0 39 4>; > >>> > }; > >>> > }; > >>> > > >>> > Then we can later add: > >>> > > >>> > sdr-ports: ports@ffc2507c { > >>> > #reset-cells = <1>; > >>> > compatible = "altr,sdr-ports"; > >>> > reg = <0xffc2507c 0x10>; > >>> > clocks = <&ddr_dqs_clk>; > >>> > ... > >>> > }; > ... here should work. No?! Just a simple driver that registers with the reset-framework. I would add 0x7c to that driver and than that driver could "configure" the port and let it out of reset. I have done the same thing for the other 3 bridges, but am not finished yet. Especially the GPV-stuff needs to at least be able to be added later if not now. Regard, Steffen
On Tue, May 20, 2014 at 9:44 AM, Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote: > Hi! > > On Tue, May 20, 2014 at 09:31:06AM -0500, Alan Tull wrote: >> On Mon, May 19, 2014 at 2:37 PM, Thor Thayer <tthayer.linux@gmail.com> wrote: >> >> >>> >> 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..8f8746b >> >>> >> --- /dev/null >> >>> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt >> >>> >> @@ -0,0 +1,11 @@ >> >>> >> +Altera SOCFPGA SDRAM Controller >> >>> >> + >> >>> >> +Required properties: >> >>> >> +- compatible : "altr,sdr-ctl"; >> >>> >> +- reg : Should contain 1 register ranges(address and length) >> >>> >> + >> >>> >> +Example: >> >>> >> + sdrctl@ffc25000 { >> >>> >> + compatible = "altr,sdr-ctl"; >> >>> >> + 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"; >> >>> > ^^^^^^^^^^ >> >>> > >> >>> > Get rid of that, too, please. >> >>> >> >>> Hi Steffan, >> >>> >> >>> I believe I need to keep the "syscon". The same register (ctrlcfg) >> >>> that has the ECC enable bitfield also includes the DRAM configuration >> >>> bitfields that other drivers will want to access (specifically the >> >>> FPGA bridge needs this information). Since this register will be >> >>> shared between drivers, syscon seems like the best solution. >> >>> >> >> >> >> Hm, from looking at the documentation of the ctrlcfg I can't really >> >> understand which bits you would need for the FPGA brigde and why. >> >> Hi Steffen, >> >> Offset 0x80 in the sdr-ctl is the "fpgaportrst" register. 14 bits >> wide, defaults to 0. When appropriate bits set to 1 in that reg, it >> allows an FPGA port to come out of reset (enables that port). Has no >> other effect on SDRAM configuration. >> >> >> That all sounds like stuff you would want to set for the specific >> >> RAM you are dealing with on a specific board. >> >> What bridge are you talking about? The SDRAM bridge? >> >> Yes, the port allows the FPGA a direct path to the SDRAM. This one >> register the only register in the sdr that the bridge driver needs. >> > > So, what I suggested down ... > >> >> >> >> I can see the problem with the ECC enable, though. >> >> >> >> Regards, >> >> Steffen >> >> >> > >> >>> > sdrctl@ffc25000 { >> >>> > compatible = "altr,sdr-ctl"; >> >>> > reg = <0xffc25000 0x1000>; >> >>> > ranges; >> >>> > >> >>> > edac@ffc2502c { >> >>> > compatible = "altr,sdram-edac"; >> >>> > reg = <0xffc2502c 0x50>; >> >>> > interrupts = <0 39 4>; >> >>> > }; >> >>> > }; >> >>> > >> >>> > Then we can later add: >> >>> > >> >>> > sdr-ports: ports@ffc2507c { >> >>> > #reset-cells = <1>; >> >>> > compatible = "altr,sdr-ports"; >> >>> > reg = <0xffc2507c 0x10>; >> >>> > clocks = <&ddr_dqs_clk>; >> >>> > ... >> >>> > }; >> > > ... here should work. No?! Just a simple driver that registers with the > reset-framework. I would add 0x7c to that driver and than that driver could > "configure" the port and let it out of reset. > > I have done the same thing for the other 3 bridges, but am not finished yet. > Especially the GPV-stuff needs to at least be able to be added later if not now. > > Regard, > 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 | Hi Steffen! I'm not clear on how the EDAC driver will interact with the registers allocated to the SDRAM controller. If the group of registers from 0xffc25000 to 0xffc26000 is exclusively allocated to the SDRAM controller, how does the EDAC driver cleanly access that single register inside this range? Is the solution that I don't use request_region() (and therefore not request exclusive access) when setting up the SDRAM controller? If you could point me to your drivers for the other bridges that you reference, your code may answer my question. Thanks for your comments and insight. Thor
On Wed, May 21, 2014 at 10:38:34AM -0500, Thor Thayer wrote: > On Tue, May 20, 2014 at 9:44 AM, Steffen Trumtrar > <s.trumtrar@pengutronix.de> wrote: > > Hi! > > > > On Tue, May 20, 2014 at 09:31:06AM -0500, Alan Tull wrote: > >> On Mon, May 19, 2014 at 2:37 PM, Thor Thayer <tthayer.linux@gmail.com> wrote: > >> > >> >>> >> 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..8f8746b > >> >>> >> --- /dev/null > >> >>> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt > >> >>> >> @@ -0,0 +1,11 @@ > >> >>> >> +Altera SOCFPGA SDRAM Controller > >> >>> >> + > >> >>> >> +Required properties: > >> >>> >> +- compatible : "altr,sdr-ctl"; > >> >>> >> +- reg : Should contain 1 register ranges(address and length) > >> >>> >> + > >> >>> >> +Example: > >> >>> >> + sdrctl@ffc25000 { > >> >>> >> + compatible = "altr,sdr-ctl"; > >> >>> >> + 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"; > >> >>> > ^^^^^^^^^^ > >> >>> > > >> >>> > Get rid of that, too, please. > >> >>> > >> >>> Hi Steffan, > >> >>> > >> >>> I believe I need to keep the "syscon". The same register (ctrlcfg) > >> >>> that has the ECC enable bitfield also includes the DRAM configuration > >> >>> bitfields that other drivers will want to access (specifically the > >> >>> FPGA bridge needs this information). Since this register will be > >> >>> shared between drivers, syscon seems like the best solution. > >> >>> > >> >> > >> >> Hm, from looking at the documentation of the ctrlcfg I can't really > >> >> understand which bits you would need for the FPGA brigde and why. > >> > >> Hi Steffen, > >> > >> Offset 0x80 in the sdr-ctl is the "fpgaportrst" register. 14 bits > >> wide, defaults to 0. When appropriate bits set to 1 in that reg, it > >> allows an FPGA port to come out of reset (enables that port). Has no > >> other effect on SDRAM configuration. > >> > >> >> That all sounds like stuff you would want to set for the specific > >> >> RAM you are dealing with on a specific board. > >> >> What bridge are you talking about? The SDRAM bridge? > >> > >> Yes, the port allows the FPGA a direct path to the SDRAM. This one > >> register the only register in the sdr that the bridge driver needs. > >> > > > > So, what I suggested down ... > > > >> >> > >> >> I can see the problem with the ECC enable, though. > >> >> > >> >> Regards, > >> >> Steffen > >> >> > >> > > >> >>> > sdrctl@ffc25000 { > >> >>> > compatible = "altr,sdr-ctl"; > >> >>> > reg = <0xffc25000 0x1000>; > >> >>> > ranges; > >> >>> > > >> >>> > edac@ffc2502c { > >> >>> > compatible = "altr,sdram-edac"; > >> >>> > reg = <0xffc2502c 0x50>; > >> >>> > interrupts = <0 39 4>; > >> >>> > }; > >> >>> > }; > >> >>> > > >> >>> > Then we can later add: > >> >>> > > >> >>> > sdr-ports: ports@ffc2507c { > >> >>> > #reset-cells = <1>; > >> >>> > compatible = "altr,sdr-ports"; > >> >>> > reg = <0xffc2507c 0x10>; > >> >>> > clocks = <&ddr_dqs_clk>; > >> >>> > ... > >> >>> > }; > >> > > > > ... here should work. No?! Just a simple driver that registers with the > > reset-framework. I would add 0x7c to that driver and than that driver could > > "configure" the port and let it out of reset. > > > > I have done the same thing for the other 3 bridges, but am not finished yet. > > Especially the GPV-stuff needs to at least be able to be added later if not now. > > Hi Thor! > I'm not clear on how the EDAC driver will interact with the registers > allocated to the SDRAM controller. If the group of registers from > 0xffc25000 to 0xffc26000 is exclusively allocated to the SDRAM > controller, how does the EDAC driver cleanly access that single > register inside this range? > The compatible in the example is wrong. I didn't mean to map the whole address space to some driver. I think for the configuration register syscon is the right approach. It is a bag of bits that don't necessitate an own driver, so syscon is perfect. So, let me change my proposal to sdr-ctl: sdram@ffc25000 { compatible = "altr,sdr-ctl", "syscon"; reg = <0xffc25000 0x1>; }; edac: edac@ffc2502c { compatible = "altr,sdram-edac"; reg = <0xffc2502c 0x50>; interrupts = <0 39 4>; config = <&sdr-ctl>; ... }; sdr-ports: ports@ffc2507c { compatible = "altr,sdr-ports"; reg = <0xffc2507c 0x10>; clocks = <&ddr_dqs_clk>; ... }; Maybe we can just skip the outer node that combines all the others. So, if we do it like that, you can still use syscon, but only for the register that needs it. And the EDAC definitely needs access to the config register, so all is good. > Is the solution that I don't use request_region() (and therefore not > request exclusive access) when setting up the SDRAM controller? > > If you could point me to your drivers for the other bridges that you > reference, your code may answer my question. > The other bridges don't need access to any SDRAM controller registers and I haven't even started to implement the GPV-stuff with the lw2hps-bridge :-( Regards, Steffen
On Tue, May 27, 2014 at 2:11 AM, Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote: > On Wed, May 21, 2014 at 10:38:34AM -0500, Thor Thayer wrote: >> On Tue, May 20, 2014 at 9:44 AM, Steffen Trumtrar >> <s.trumtrar@pengutronix.de> wrote: >> > Hi! >> > >> > On Tue, May 20, 2014 at 09:31:06AM -0500, Alan Tull wrote: >> >> On Mon, May 19, 2014 at 2:37 PM, Thor Thayer <tthayer.linux@gmail.com> wrote: >> >> >> >> >>> >> 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..8f8746b >> >> >>> >> --- /dev/null >> >> >>> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt >> >> >>> >> @@ -0,0 +1,11 @@ >> >> >>> >> +Altera SOCFPGA SDRAM Controller >> >> >>> >> + >> >> >>> >> +Required properties: >> >> >>> >> +- compatible : "altr,sdr-ctl"; >> >> >>> >> +- reg : Should contain 1 register ranges(address and length) >> >> >>> >> + >> >> >>> >> +Example: >> >> >>> >> + sdrctl@ffc25000 { >> >> >>> >> + compatible = "altr,sdr-ctl"; >> >> >>> >> + 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"; >> >> >>> > ^^^^^^^^^^ >> >> >>> > >> >> >>> > Get rid of that, too, please. >> >> >>> >> >> >>> Hi Steffan, >> >> >>> >> >> >>> I believe I need to keep the "syscon". The same register (ctrlcfg) >> >> >>> that has the ECC enable bitfield also includes the DRAM configuration >> >> >>> bitfields that other drivers will want to access (specifically the >> >> >>> FPGA bridge needs this information). Since this register will be >> >> >>> shared between drivers, syscon seems like the best solution. >> >> >>> >> >> >> >> >> >> Hm, from looking at the documentation of the ctrlcfg I can't really >> >> >> understand which bits you would need for the FPGA brigde and why. >> >> >> >> Hi Steffen, >> >> >> >> Offset 0x80 in the sdr-ctl is the "fpgaportrst" register. 14 bits >> >> wide, defaults to 0. When appropriate bits set to 1 in that reg, it >> >> allows an FPGA port to come out of reset (enables that port). Has no >> >> other effect on SDRAM configuration. >> >> >> >> >> That all sounds like stuff you would want to set for the specific >> >> >> RAM you are dealing with on a specific board. >> >> >> What bridge are you talking about? The SDRAM bridge? >> >> >> >> Yes, the port allows the FPGA a direct path to the SDRAM. This one >> >> register the only register in the sdr that the bridge driver needs. >> >> >> > >> > So, what I suggested down ... >> > >> >> >> >> >> >> I can see the problem with the ECC enable, though. >> >> >> >> >> >> Regards, >> >> >> Steffen >> >> >> >> >> > >> >> >>> > sdrctl@ffc25000 { >> >> >>> > compatible = "altr,sdr-ctl"; >> >> >>> > reg = <0xffc25000 0x1000>; >> >> >>> > ranges; >> >> >>> > >> >> >>> > edac@ffc2502c { >> >> >>> > compatible = "altr,sdram-edac"; >> >> >>> > reg = <0xffc2502c 0x50>; >> >> >>> > interrupts = <0 39 4>; >> >> >>> > }; >> >> >>> > }; >> >> >>> > >> >> >>> > Then we can later add: >> >> >>> > >> >> >>> > sdr-ports: ports@ffc2507c { >> >> >>> > #reset-cells = <1>; >> >> >>> > compatible = "altr,sdr-ports"; >> >> >>> > reg = <0xffc2507c 0x10>; >> >> >>> > clocks = <&ddr_dqs_clk>; >> >> >>> > ... >> >> >>> > }; >> >> >> > >> > ... here should work. No?! Just a simple driver that registers with the >> > reset-framework. I would add 0x7c to that driver and than that driver could >> > "configure" the port and let it out of reset. >> > >> > I have done the same thing for the other 3 bridges, but am not finished yet. >> > Especially the GPV-stuff needs to at least be able to be added later if not now. >> > > > Hi Thor! > >> I'm not clear on how the EDAC driver will interact with the registers >> allocated to the SDRAM controller. If the group of registers from >> 0xffc25000 to 0xffc26000 is exclusively allocated to the SDRAM >> controller, how does the EDAC driver cleanly access that single >> register inside this range? >> > > The compatible in the example is wrong. I didn't mean to map the whole address space > to some driver. > I think for the configuration register syscon is the right approach. It is a > bag of bits that don't necessitate an own driver, so syscon is perfect. > > So, let me change my proposal to > > sdr-ctl: sdram@ffc25000 { > compatible = "altr,sdr-ctl", "syscon"; > reg = <0xffc25000 0x1>; > }; > > edac: edac@ffc2502c { > compatible = "altr,sdram-edac"; > reg = <0xffc2502c 0x50>; > interrupts = <0 39 4>; > config = <&sdr-ctl>; > ... > }; > > sdr-ports: ports@ffc2507c { > compatible = "altr,sdr-ports"; > reg = <0xffc2507c 0x10>; > clocks = <&ddr_dqs_clk>; > ... > }; > > Maybe we can just skip the outer node that combines all the others. > So, if we do it like that, you can still use syscon, but only for the register > that needs it. And the EDAC definitely needs access to the config register, so > all is good. > >> Is the solution that I don't use request_region() (and therefore not >> request exclusive access) when setting up the SDRAM controller? >> >> If you could point me to your drivers for the other bridges that you >> reference, your code may answer my question. >> > > The other bridges don't need access to any SDRAM controller registers and > I haven't even started to implement the GPV-stuff with the lw2hps-bridge :-( > > 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 | Thank you for your help on this, Steffen. It is good to have your insight. I'll implement the changes and resubmit shortly. Thanks, Thor
On Tue, May 27, 2014 at 2:11 AM, Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote: > On Wed, May 21, 2014 at 10:38:34AM -0500, Thor Thayer wrote: >> On Tue, May 20, 2014 at 9:44 AM, Steffen Trumtrar >> <s.trumtrar@pengutronix.de> wrote: >> > Hi! >> > >> > On Tue, May 20, 2014 at 09:31:06AM -0500, Alan Tull wrote: >> >> On Mon, May 19, 2014 at 2:37 PM, Thor Thayer <tthayer.linux@gmail.com> wrote: >> >> >> >> >>> >> 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..8f8746b >> >> >>> >> --- /dev/null >> >> >>> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt >> >> >>> >> @@ -0,0 +1,11 @@ >> >> >>> >> +Altera SOCFPGA SDRAM Controller >> >> >>> >> + >> >> >>> >> +Required properties: >> >> >>> >> +- compatible : "altr,sdr-ctl"; >> >> >>> >> +- reg : Should contain 1 register ranges(address and length) >> >> >>> >> + >> >> >>> >> +Example: >> >> >>> >> + sdrctl@ffc25000 { >> >> >>> >> + compatible = "altr,sdr-ctl"; >> >> >>> >> + 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"; >> >> >>> > ^^^^^^^^^^ >> >> >>> > >> >> >>> > Get rid of that, too, please. >> >> >>> >> >> >>> Hi Steffan, >> >> >>> >> >> >>> I believe I need to keep the "syscon". The same register (ctrlcfg) >> >> >>> that has the ECC enable bitfield also includes the DRAM configuration >> >> >>> bitfields that other drivers will want to access (specifically the >> >> >>> FPGA bridge needs this information). Since this register will be >> >> >>> shared between drivers, syscon seems like the best solution. >> >> >>> >> >> >> >> >> >> Hm, from looking at the documentation of the ctrlcfg I can't really >> >> >> understand which bits you would need for the FPGA brigde and why. >> >> >> >> Hi Steffen, >> >> >> >> Offset 0x80 in the sdr-ctl is the "fpgaportrst" register. 14 bits >> >> wide, defaults to 0. When appropriate bits set to 1 in that reg, it >> >> allows an FPGA port to come out of reset (enables that port). Has no >> >> other effect on SDRAM configuration. >> >> >> >> >> That all sounds like stuff you would want to set for the specific >> >> >> RAM you are dealing with on a specific board. >> >> >> What bridge are you talking about? The SDRAM bridge? >> >> >> >> Yes, the port allows the FPGA a direct path to the SDRAM. This one >> >> register the only register in the sdr that the bridge driver needs. >> >> >> > >> > So, what I suggested down ... >> > >> >> >> >> >> >> I can see the problem with the ECC enable, though. >> >> >> >> >> >> Regards, >> >> >> Steffen >> >> >> >> >> > >> >> >>> > sdrctl@ffc25000 { >> >> >>> > compatible = "altr,sdr-ctl"; >> >> >>> > reg = <0xffc25000 0x1000>; >> >> >>> > ranges; >> >> >>> > >> >> >>> > edac@ffc2502c { >> >> >>> > compatible = "altr,sdram-edac"; >> >> >>> > reg = <0xffc2502c 0x50>; >> >> >>> > interrupts = <0 39 4>; >> >> >>> > }; >> >> >>> > }; >> >> >>> > >> >> >>> > Then we can later add: >> >> >>> > >> >> >>> > sdr-ports: ports@ffc2507c { >> >> >>> > #reset-cells = <1>; >> >> >>> > compatible = "altr,sdr-ports"; >> >> >>> > reg = <0xffc2507c 0x10>; >> >> >>> > clocks = <&ddr_dqs_clk>; >> >> >>> > ... >> >> >>> > }; >> >> >> > >> > ... here should work. No?! Just a simple driver that registers with the >> > reset-framework. I would add 0x7c to that driver and than that driver could >> > "configure" the port and let it out of reset. >> > >> > I have done the same thing for the other 3 bridges, but am not finished yet. >> > Especially the GPV-stuff needs to at least be able to be added later if not now. >> > > > Hi Thor! > >> I'm not clear on how the EDAC driver will interact with the registers >> allocated to the SDRAM controller. If the group of registers from >> 0xffc25000 to 0xffc26000 is exclusively allocated to the SDRAM >> controller, how does the EDAC driver cleanly access that single >> register inside this range? >> > > The compatible in the example is wrong. I didn't mean to map the whole address space > to some driver. > I think for the configuration register syscon is the right approach. It is a > bag of bits that don't necessitate an own driver, so syscon is perfect. > > So, let me change my proposal to > > sdr-ctl: sdram@ffc25000 { > compatible = "altr,sdr-ctl", "syscon"; > reg = <0xffc25000 0x1>; > }; > > edac: edac@ffc2502c { > compatible = "altr,sdram-edac"; > reg = <0xffc2502c 0x50>; > interrupts = <0 39 4>; > config = <&sdr-ctl>; > ... > }; > > sdr-ports: ports@ffc2507c { > compatible = "altr,sdr-ports"; > reg = <0xffc2507c 0x10>; > clocks = <&ddr_dqs_clk>; > ... > }; > > Maybe we can just skip the outer node that combines all the others. > So, if we do it like that, you can still use syscon, but only for the register > that needs it. And the EDAC definitely needs access to the config register, so > all is good. > >> Is the solution that I don't use request_region() (and therefore not >> request exclusive access) when setting up the SDRAM controller? >> >> If you could point me to your drivers for the other bridges that you >> reference, your code may answer my question. >> > > The other bridges don't need access to any SDRAM controller registers and > I haven't even started to implement the GPV-stuff with the lw2hps-bridge :-( > > 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 | OK now I understand. For register offset 0x0, that's all stuff set up in the bootloader. We won't be touching that register except for the EDAC. So syscon isn't needed here. Each driver that uses some sdr registers can specify which registers it uses in the device tree. So far we don't have any cases of two drivers that share a register. The ECC driver will need two ranges: offset 0x00 and 0x38 through 0x50. The fpga bridges just needs offset 0x80. Thanks for your feedback! Alan
On Tue, May 27, 2014 at 02:12:17PM -0500, Alan Tull wrote: > On Tue, May 27, 2014 at 2:11 AM, Steffen Trumtrar > <s.trumtrar@pengutronix.de> wrote: > > On Wed, May 21, 2014 at 10:38:34AM -0500, Thor Thayer wrote: > >> On Tue, May 20, 2014 at 9:44 AM, Steffen Trumtrar > >> <s.trumtrar@pengutronix.de> wrote: > >> > Hi! > >> > > >> > On Tue, May 20, 2014 at 09:31:06AM -0500, Alan Tull wrote: > >> >> On Mon, May 19, 2014 at 2:37 PM, Thor Thayer <tthayer.linux@gmail.com> wrote: > >> >> > >> >> >>> >> 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..8f8746b > >> >> >>> >> --- /dev/null > >> >> >>> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt > >> >> >>> >> @@ -0,0 +1,11 @@ > >> >> >>> >> +Altera SOCFPGA SDRAM Controller > >> >> >>> >> + > >> >> >>> >> +Required properties: > >> >> >>> >> +- compatible : "altr,sdr-ctl"; > >> >> >>> >> +- reg : Should contain 1 register ranges(address and length) > >> >> >>> >> + > >> >> >>> >> +Example: > >> >> >>> >> + sdrctl@ffc25000 { > >> >> >>> >> + compatible = "altr,sdr-ctl"; > >> >> >>> >> + 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"; > >> >> >>> > ^^^^^^^^^^ > >> >> >>> > > >> >> >>> > Get rid of that, too, please. > >> >> >>> > >> >> >>> Hi Steffan, > >> >> >>> > >> >> >>> I believe I need to keep the "syscon". The same register (ctrlcfg) > >> >> >>> that has the ECC enable bitfield also includes the DRAM configuration > >> >> >>> bitfields that other drivers will want to access (specifically the > >> >> >>> FPGA bridge needs this information). Since this register will be > >> >> >>> shared between drivers, syscon seems like the best solution. > >> >> >>> > >> >> >> > >> >> >> Hm, from looking at the documentation of the ctrlcfg I can't really > >> >> >> understand which bits you would need for the FPGA brigde and why. > >> >> > >> >> Hi Steffen, > >> >> > >> >> Offset 0x80 in the sdr-ctl is the "fpgaportrst" register. 14 bits > >> >> wide, defaults to 0. When appropriate bits set to 1 in that reg, it > >> >> allows an FPGA port to come out of reset (enables that port). Has no > >> >> other effect on SDRAM configuration. > >> >> > >> >> >> That all sounds like stuff you would want to set for the specific > >> >> >> RAM you are dealing with on a specific board. > >> >> >> What bridge are you talking about? The SDRAM bridge? > >> >> > >> >> Yes, the port allows the FPGA a direct path to the SDRAM. This one > >> >> register the only register in the sdr that the bridge driver needs. > >> >> > >> > > >> > So, what I suggested down ... > >> > > >> >> >> > >> >> >> I can see the problem with the ECC enable, though. > >> >> >> > >> >> >> Regards, > >> >> >> Steffen > >> >> >> > >> >> > > >> >> >>> > sdrctl@ffc25000 { > >> >> >>> > compatible = "altr,sdr-ctl"; > >> >> >>> > reg = <0xffc25000 0x1000>; > >> >> >>> > ranges; > >> >> >>> > > >> >> >>> > edac@ffc2502c { > >> >> >>> > compatible = "altr,sdram-edac"; > >> >> >>> > reg = <0xffc2502c 0x50>; > >> >> >>> > interrupts = <0 39 4>; > >> >> >>> > }; > >> >> >>> > }; > >> >> >>> > > >> >> >>> > Then we can later add: > >> >> >>> > > >> >> >>> > sdr-ports: ports@ffc2507c { > >> >> >>> > #reset-cells = <1>; > >> >> >>> > compatible = "altr,sdr-ports"; > >> >> >>> > reg = <0xffc2507c 0x10>; > >> >> >>> > clocks = <&ddr_dqs_clk>; > >> >> >>> > ... > >> >> >>> > }; > >> >> > >> > > >> > ... here should work. No?! Just a simple driver that registers with the > >> > reset-framework. I would add 0x7c to that driver and than that driver could > >> > "configure" the port and let it out of reset. > >> > > >> > I have done the same thing for the other 3 bridges, but am not finished yet. > >> > Especially the GPV-stuff needs to at least be able to be added later if not now. > >> > > > > > Hi Thor! > > > >> I'm not clear on how the EDAC driver will interact with the registers > >> allocated to the SDRAM controller. If the group of registers from > >> 0xffc25000 to 0xffc26000 is exclusively allocated to the SDRAM > >> controller, how does the EDAC driver cleanly access that single > >> register inside this range? > >> > > > > The compatible in the example is wrong. I didn't mean to map the whole address space > > to some driver. > > I think for the configuration register syscon is the right approach. It is a > > bag of bits that don't necessitate an own driver, so syscon is perfect. > > > > So, let me change my proposal to > > > > sdr-ctl: sdram@ffc25000 { > > compatible = "altr,sdr-ctl", "syscon"; > > reg = <0xffc25000 0x1>; > > }; > > > > edac: edac@ffc2502c { > > compatible = "altr,sdram-edac"; > > reg = <0xffc2502c 0x50>; > > interrupts = <0 39 4>; > > config = <&sdr-ctl>; > > ... > > }; > > > > sdr-ports: ports@ffc2507c { > > compatible = "altr,sdr-ports"; > > reg = <0xffc2507c 0x10>; > > clocks = <&ddr_dqs_clk>; > > ... > > }; > > > > Maybe we can just skip the outer node that combines all the others. > > So, if we do it like that, you can still use syscon, but only for the register > > that needs it. And the EDAC definitely needs access to the config register, so > > all is good. > > > >> Is the solution that I don't use request_region() (and therefore not > >> request exclusive access) when setting up the SDRAM controller? > >> > >> If you could point me to your drivers for the other bridges that you > >> reference, your code may answer my question. > >> > > > > The other bridges don't need access to any SDRAM controller registers and > > I haven't even started to implement the GPV-stuff with the lw2hps-bridge :-( > > > > 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 | > > OK now I understand. For register offset 0x0, that's all stuff set up > in the bootloader. We won't be touching that register except for the > EDAC. So syscon isn't needed here. > > Each driver that uses some sdr registers can specify which registers > it uses in the device tree. So far we don't have any cases of two > drivers that share a register. > > The ECC driver will need two ranges: offset 0x00 and 0x38 through 0x50. > The fpga bridges just needs offset 0x80. > Well, almost ;-) Use syscon for 0x0 and reference that in the ECC driver, which only is responsible for 0x38 to 0x50. Then flip the two EDAC bits via the syscon-phandle. Then a bootloader can probe with the same dts and have a driver match on the config-register and one for all the DRAM setup (< 0x38), which we don't need in the kernel. On the other hand the EDAC seems unneccessary for the bootloader. And this all matches the partitioning of the SDR register space, so I think it is also a correct hardware description. Regards, Steffen
On Tue, May 27, 2014 at 01:00:32PM -0500, Thor Thayer wrote: > On Tue, May 27, 2014 at 2:11 AM, Steffen Trumtrar > <s.trumtrar@pengutronix.de> wrote: > > On Wed, May 21, 2014 at 10:38:34AM -0500, Thor Thayer wrote: > >> On Tue, May 20, 2014 at 9:44 AM, Steffen Trumtrar > >> <s.trumtrar@pengutronix.de> wrote: > >> > Hi! > >> > > >> > On Tue, May 20, 2014 at 09:31:06AM -0500, Alan Tull wrote: > >> >> On Mon, May 19, 2014 at 2:37 PM, Thor Thayer <tthayer.linux@gmail.com> wrote: > >> >> > >> >> >>> >> 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..8f8746b > >> >> >>> >> --- /dev/null > >> >> >>> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt > >> >> >>> >> @@ -0,0 +1,11 @@ > >> >> >>> >> +Altera SOCFPGA SDRAM Controller > >> >> >>> >> + > >> >> >>> >> +Required properties: > >> >> >>> >> +- compatible : "altr,sdr-ctl"; > >> >> >>> >> +- reg : Should contain 1 register ranges(address and length) > >> >> >>> >> + > >> >> >>> >> +Example: > >> >> >>> >> + sdrctl@ffc25000 { > >> >> >>> >> + compatible = "altr,sdr-ctl"; > >> >> >>> >> + 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"; > >> >> >>> > ^^^^^^^^^^ > >> >> >>> > > >> >> >>> > Get rid of that, too, please. > >> >> >>> > >> >> >>> Hi Steffan, > >> >> >>> > >> >> >>> I believe I need to keep the "syscon". The same register (ctrlcfg) > >> >> >>> that has the ECC enable bitfield also includes the DRAM configuration > >> >> >>> bitfields that other drivers will want to access (specifically the > >> >> >>> FPGA bridge needs this information). Since this register will be > >> >> >>> shared between drivers, syscon seems like the best solution. > >> >> >>> > >> >> >> > >> >> >> Hm, from looking at the documentation of the ctrlcfg I can't really > >> >> >> understand which bits you would need for the FPGA brigde and why. > >> >> > >> >> Hi Steffen, > >> >> > >> >> Offset 0x80 in the sdr-ctl is the "fpgaportrst" register. 14 bits > >> >> wide, defaults to 0. When appropriate bits set to 1 in that reg, it > >> >> allows an FPGA port to come out of reset (enables that port). Has no > >> >> other effect on SDRAM configuration. > >> >> > >> >> >> That all sounds like stuff you would want to set for the specific > >> >> >> RAM you are dealing with on a specific board. > >> >> >> What bridge are you talking about? The SDRAM bridge? > >> >> > >> >> Yes, the port allows the FPGA a direct path to the SDRAM. This one > >> >> register the only register in the sdr that the bridge driver needs. > >> >> > >> > > >> > So, what I suggested down ... > >> > > >> >> >> > >> >> >> I can see the problem with the ECC enable, though. > >> >> >> > >> >> >> Regards, > >> >> >> Steffen > >> >> >> > >> >> > > >> >> >>> > sdrctl@ffc25000 { > >> >> >>> > compatible = "altr,sdr-ctl"; > >> >> >>> > reg = <0xffc25000 0x1000>; > >> >> >>> > ranges; > >> >> >>> > > >> >> >>> > edac@ffc2502c { > >> >> >>> > compatible = "altr,sdram-edac"; > >> >> >>> > reg = <0xffc2502c 0x50>; > >> >> >>> > interrupts = <0 39 4>; > >> >> >>> > }; > >> >> >>> > }; > >> >> >>> > > >> >> >>> > Then we can later add: > >> >> >>> > > >> >> >>> > sdr-ports: ports@ffc2507c { > >> >> >>> > #reset-cells = <1>; > >> >> >>> > compatible = "altr,sdr-ports"; > >> >> >>> > reg = <0xffc2507c 0x10>; > >> >> >>> > clocks = <&ddr_dqs_clk>; > >> >> >>> > ... > >> >> >>> > }; > >> >> > >> > > >> > ... here should work. No?! Just a simple driver that registers with the > >> > reset-framework. I would add 0x7c to that driver and than that driver could > >> > "configure" the port and let it out of reset. > >> > > >> > I have done the same thing for the other 3 bridges, but am not finished yet. > >> > Especially the GPV-stuff needs to at least be able to be added later if not now. > >> > > > > > Hi Thor! > > > >> I'm not clear on how the EDAC driver will interact with the registers > >> allocated to the SDRAM controller. If the group of registers from > >> 0xffc25000 to 0xffc26000 is exclusively allocated to the SDRAM > >> controller, how does the EDAC driver cleanly access that single > >> register inside this range? > >> > > > > The compatible in the example is wrong. I didn't mean to map the whole address space > > to some driver. > > I think for the configuration register syscon is the right approach. It is a > > bag of bits that don't necessitate an own driver, so syscon is perfect. > > > > So, let me change my proposal to > > > > sdr-ctl: sdram@ffc25000 { > > compatible = "altr,sdr-ctl", "syscon"; > > reg = <0xffc25000 0x1>; > > }; > > > > edac: edac@ffc2502c { > > compatible = "altr,sdram-edac"; > > reg = <0xffc2502c 0x50>; > > interrupts = <0 39 4>; > > config = <&sdr-ctl>; > > ... > > }; > > > > sdr-ports: ports@ffc2507c { > > compatible = "altr,sdr-ports"; > > reg = <0xffc2507c 0x10>; > > clocks = <&ddr_dqs_clk>; > > ... > > }; > > > > Maybe we can just skip the outer node that combines all the others. > > So, if we do it like that, you can still use syscon, but only for the register > > that needs it. And the EDAC definitely needs access to the config register, so > > all is good. > > > >> Is the solution that I don't use request_region() (and therefore not > >> request exclusive access) when setting up the SDRAM controller? > >> > >> If you could point me to your drivers for the other bridges that you > >> reference, your code may answer my question. > >> > > > > The other bridges don't need access to any SDRAM controller registers and > > I haven't even started to implement the GPV-stuff with the lw2hps-bridge :-( > > > > 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 | > > Thank you for your help on this, Steffen. It is good to have your > insight. I'll implement the changes and resubmit shortly. > Very good. I'm happy that we could come to a conclusion we are all okay with. (Unless someone else shows up and rains on your parade ;-)) Regards, Steffen
On Tue, May 27, 2014 at 2:42 PM, Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote: > On Tue, May 27, 2014 at 02:12:17PM -0500, Alan Tull wrote: >> On Tue, May 27, 2014 at 2:11 AM, Steffen Trumtrar >> <s.trumtrar@pengutronix.de> wrote: >> > On Wed, May 21, 2014 at 10:38:34AM -0500, Thor Thayer wrote: >> >> On Tue, May 20, 2014 at 9:44 AM, Steffen Trumtrar >> >> <s.trumtrar@pengutronix.de> wrote: >> >> > Hi! >> >> > >> >> > On Tue, May 20, 2014 at 09:31:06AM -0500, Alan Tull wrote: >> >> >> On Mon, May 19, 2014 at 2:37 PM, Thor Thayer <tthayer.linux@gmail.com> wrote: >> >> >> >> >> >> >>> >> 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..8f8746b >> >> >> >>> >> --- /dev/null >> >> >> >>> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt >> >> >> >>> >> @@ -0,0 +1,11 @@ >> >> >> >>> >> +Altera SOCFPGA SDRAM Controller >> >> >> >>> >> + >> >> >> >>> >> +Required properties: >> >> >> >>> >> +- compatible : "altr,sdr-ctl"; >> >> >> >>> >> +- reg : Should contain 1 register ranges(address and length) >> >> >> >>> >> + >> >> >> >>> >> +Example: >> >> >> >>> >> + sdrctl@ffc25000 { >> >> >> >>> >> + compatible = "altr,sdr-ctl"; >> >> >> >>> >> + 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"; >> >> >> >>> > ^^^^^^^^^^ >> >> >> >>> > >> >> >> >>> > Get rid of that, too, please. >> >> >> >>> >> >> >> >>> Hi Steffan, >> >> >> >>> >> >> >> >>> I believe I need to keep the "syscon". The same register (ctrlcfg) >> >> >> >>> that has the ECC enable bitfield also includes the DRAM configuration >> >> >> >>> bitfields that other drivers will want to access (specifically the >> >> >> >>> FPGA bridge needs this information). Since this register will be >> >> >> >>> shared between drivers, syscon seems like the best solution. >> >> >> >>> >> >> >> >> >> >> >> >> Hm, from looking at the documentation of the ctrlcfg I can't really >> >> >> >> understand which bits you would need for the FPGA brigde and why. >> >> >> >> >> >> Hi Steffen, >> >> >> >> >> >> Offset 0x80 in the sdr-ctl is the "fpgaportrst" register. 14 bits >> >> >> wide, defaults to 0. When appropriate bits set to 1 in that reg, it >> >> >> allows an FPGA port to come out of reset (enables that port). Has no >> >> >> other effect on SDRAM configuration. >> >> >> >> >> >> >> That all sounds like stuff you would want to set for the specific >> >> >> >> RAM you are dealing with on a specific board. >> >> >> >> What bridge are you talking about? The SDRAM bridge? >> >> >> >> >> >> Yes, the port allows the FPGA a direct path to the SDRAM. This one >> >> >> register the only register in the sdr that the bridge driver needs. >> >> >> >> >> > >> >> > So, what I suggested down ... >> >> > >> >> >> >> >> >> >> >> I can see the problem with the ECC enable, though. >> >> >> >> >> >> >> >> Regards, >> >> >> >> Steffen >> >> >> >> >> >> >> > >> >> >> >>> > sdrctl@ffc25000 { >> >> >> >>> > compatible = "altr,sdr-ctl"; >> >> >> >>> > reg = <0xffc25000 0x1000>; >> >> >> >>> > ranges; >> >> >> >>> > >> >> >> >>> > edac@ffc2502c { >> >> >> >>> > compatible = "altr,sdram-edac"; >> >> >> >>> > reg = <0xffc2502c 0x50>; >> >> >> >>> > interrupts = <0 39 4>; >> >> >> >>> > }; >> >> >> >>> > }; >> >> >> >>> > >> >> >> >>> > Then we can later add: >> >> >> >>> > >> >> >> >>> > sdr-ports: ports@ffc2507c { >> >> >> >>> > #reset-cells = <1>; >> >> >> >>> > compatible = "altr,sdr-ports"; >> >> >> >>> > reg = <0xffc2507c 0x10>; >> >> >> >>> > clocks = <&ddr_dqs_clk>; >> >> >> >>> > ... >> >> >> >>> > }; >> >> >> >> >> > >> >> > ... here should work. No?! Just a simple driver that registers with the >> >> > reset-framework. I would add 0x7c to that driver and than that driver could >> >> > "configure" the port and let it out of reset. >> >> > >> >> > I have done the same thing for the other 3 bridges, but am not finished yet. >> >> > Especially the GPV-stuff needs to at least be able to be added later if not now. >> >> > >> > >> > Hi Thor! >> > >> >> I'm not clear on how the EDAC driver will interact with the registers >> >> allocated to the SDRAM controller. If the group of registers from >> >> 0xffc25000 to 0xffc26000 is exclusively allocated to the SDRAM >> >> controller, how does the EDAC driver cleanly access that single >> >> register inside this range? >> >> >> > >> > The compatible in the example is wrong. I didn't mean to map the whole address space >> > to some driver. >> > I think for the configuration register syscon is the right approach. It is a >> > bag of bits that don't necessitate an own driver, so syscon is perfect. >> > >> > So, let me change my proposal to >> > >> > sdr-ctl: sdram@ffc25000 { >> > compatible = "altr,sdr-ctl", "syscon"; >> > reg = <0xffc25000 0x1>; >> > }; >> > >> > edac: edac@ffc2502c { >> > compatible = "altr,sdram-edac"; >> > reg = <0xffc2502c 0x50>; >> > interrupts = <0 39 4>; >> > config = <&sdr-ctl>; >> > ... >> > }; >> > >> > sdr-ports: ports@ffc2507c { >> > compatible = "altr,sdr-ports"; >> > reg = <0xffc2507c 0x10>; >> > clocks = <&ddr_dqs_clk>; >> > ... >> > }; >> > >> > Maybe we can just skip the outer node that combines all the others. >> > So, if we do it like that, you can still use syscon, but only for the register >> > that needs it. And the EDAC definitely needs access to the config register, so >> > all is good. >> > >> >> Is the solution that I don't use request_region() (and therefore not >> >> request exclusive access) when setting up the SDRAM controller? >> >> >> >> If you could point me to your drivers for the other bridges that you >> >> reference, your code may answer my question. >> >> >> > >> > The other bridges don't need access to any SDRAM controller registers and >> > I haven't even started to implement the GPV-stuff with the lw2hps-bridge :-( >> > >> > 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 | >> >> OK now I understand. For register offset 0x0, that's all stuff set up >> in the bootloader. We won't be touching that register except for the >> EDAC. So syscon isn't needed here. >> >> Each driver that uses some sdr registers can specify which registers >> it uses in the device tree. So far we don't have any cases of two >> drivers that share a register. >> >> The ECC driver will need two ranges: offset 0x00 and 0x38 through 0x50. >> The fpga bridges just needs offset 0x80. >> > > Well, almost ;-) > Use syscon for 0x0 and reference that in the ECC driver, which only is > responsible for 0x38 to 0x50. Then flip the two EDAC bits via the syscon-phandle. > Then a bootloader can probe with the same dts and have a driver match on the > config-register and one for all the DRAM setup (< 0x38), which we don't need in > the kernel. On the other hand the EDAC seems unneccessary for the bootloader. > And this all matches the partitioning of the SDR register space, so I think it > is also a correct hardware description. > > 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 | We could do that. But is syscon a concept the bootloader will know about? Actually it is a bit messier than what I originally said: ECC driver needs two ranges: offset 0x00 and 0x38 through 0x50. Machine layer needs control of 0x10, 0x14, 0x54, and 0x58 for putting SDRAM into self-refresh. fpga bridges needs offset 0x80. Alan
On Tue, May 27, 2014 at 03:57:47PM -0500, Alan Tull wrote: > On Tue, May 27, 2014 at 2:42 PM, Steffen Trumtrar > <s.trumtrar@pengutronix.de> wrote: > > On Tue, May 27, 2014 at 02:12:17PM -0500, Alan Tull wrote: > >> On Tue, May 27, 2014 at 2:11 AM, Steffen Trumtrar > >> <s.trumtrar@pengutronix.de> wrote: > >> > On Wed, May 21, 2014 at 10:38:34AM -0500, Thor Thayer wrote: > >> >> On Tue, May 20, 2014 at 9:44 AM, Steffen Trumtrar > >> >> <s.trumtrar@pengutronix.de> wrote: > >> >> > Hi! > >> >> > > >> >> > On Tue, May 20, 2014 at 09:31:06AM -0500, Alan Tull wrote: > >> >> >> On Mon, May 19, 2014 at 2:37 PM, Thor Thayer <tthayer.linux@gmail.com> wrote: > >> >> >> > >> >> >> >>> >> 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..8f8746b > >> >> >> >>> >> --- /dev/null > >> >> >> >>> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt > >> >> >> >>> >> @@ -0,0 +1,11 @@ > >> >> >> >>> >> +Altera SOCFPGA SDRAM Controller > >> >> >> >>> >> + > >> >> >> >>> >> +Required properties: > >> >> >> >>> >> +- compatible : "altr,sdr-ctl"; > >> >> >> >>> >> +- reg : Should contain 1 register ranges(address and length) > >> >> >> >>> >> + > >> >> >> >>> >> +Example: > >> >> >> >>> >> + sdrctl@ffc25000 { > >> >> >> >>> >> + compatible = "altr,sdr-ctl"; > >> >> >> >>> >> + 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"; > >> >> >> >>> > ^^^^^^^^^^ > >> >> >> >>> > > >> >> >> >>> > Get rid of that, too, please. > >> >> >> >>> > >> >> >> >>> Hi Steffan, > >> >> >> >>> > >> >> >> >>> I believe I need to keep the "syscon". The same register (ctrlcfg) > >> >> >> >>> that has the ECC enable bitfield also includes the DRAM configuration > >> >> >> >>> bitfields that other drivers will want to access (specifically the > >> >> >> >>> FPGA bridge needs this information). Since this register will be > >> >> >> >>> shared between drivers, syscon seems like the best solution. > >> >> >> >>> > >> >> >> >> > >> >> >> >> Hm, from looking at the documentation of the ctrlcfg I can't really > >> >> >> >> understand which bits you would need for the FPGA brigde and why. > >> >> >> > >> >> >> Hi Steffen, > >> >> >> > >> >> >> Offset 0x80 in the sdr-ctl is the "fpgaportrst" register. 14 bits > >> >> >> wide, defaults to 0. When appropriate bits set to 1 in that reg, it > >> >> >> allows an FPGA port to come out of reset (enables that port). Has no > >> >> >> other effect on SDRAM configuration. > >> >> >> > >> >> >> >> That all sounds like stuff you would want to set for the specific > >> >> >> >> RAM you are dealing with on a specific board. > >> >> >> >> What bridge are you talking about? The SDRAM bridge? > >> >> >> > >> >> >> Yes, the port allows the FPGA a direct path to the SDRAM. This one > >> >> >> register the only register in the sdr that the bridge driver needs. > >> >> >> > >> >> > > >> >> > So, what I suggested down ... > >> >> > > >> >> >> >> > >> >> >> >> I can see the problem with the ECC enable, though. > >> >> >> >> > >> >> >> >> Regards, > >> >> >> >> Steffen > >> >> >> >> > >> >> >> > > >> >> >> >>> > sdrctl@ffc25000 { > >> >> >> >>> > compatible = "altr,sdr-ctl"; > >> >> >> >>> > reg = <0xffc25000 0x1000>; > >> >> >> >>> > ranges; > >> >> >> >>> > > >> >> >> >>> > edac@ffc2502c { > >> >> >> >>> > compatible = "altr,sdram-edac"; > >> >> >> >>> > reg = <0xffc2502c 0x50>; > >> >> >> >>> > interrupts = <0 39 4>; > >> >> >> >>> > }; > >> >> >> >>> > }; > >> >> >> >>> > > >> >> >> >>> > Then we can later add: > >> >> >> >>> > > >> >> >> >>> > sdr-ports: ports@ffc2507c { > >> >> >> >>> > #reset-cells = <1>; > >> >> >> >>> > compatible = "altr,sdr-ports"; > >> >> >> >>> > reg = <0xffc2507c 0x10>; > >> >> >> >>> > clocks = <&ddr_dqs_clk>; > >> >> >> >>> > ... > >> >> >> >>> > }; > >> >> >> > >> >> > > >> >> > ... here should work. No?! Just a simple driver that registers with the > >> >> > reset-framework. I would add 0x7c to that driver and than that driver could > >> >> > "configure" the port and let it out of reset. > >> >> > > >> >> > I have done the same thing for the other 3 bridges, but am not finished yet. > >> >> > Especially the GPV-stuff needs to at least be able to be added later if not now. > >> >> > > >> > > >> > Hi Thor! > >> > > >> >> I'm not clear on how the EDAC driver will interact with the registers > >> >> allocated to the SDRAM controller. If the group of registers from > >> >> 0xffc25000 to 0xffc26000 is exclusively allocated to the SDRAM > >> >> controller, how does the EDAC driver cleanly access that single > >> >> register inside this range? > >> >> > >> > > >> > The compatible in the example is wrong. I didn't mean to map the whole address space > >> > to some driver. > >> > I think for the configuration register syscon is the right approach. It is a > >> > bag of bits that don't necessitate an own driver, so syscon is perfect. > >> > > >> > So, let me change my proposal to > >> > > >> > sdr-ctl: sdram@ffc25000 { > >> > compatible = "altr,sdr-ctl", "syscon"; > >> > reg = <0xffc25000 0x1>; > >> > }; > >> > > >> > edac: edac@ffc2502c { > >> > compatible = "altr,sdram-edac"; > >> > reg = <0xffc2502c 0x50>; > >> > interrupts = <0 39 4>; > >> > config = <&sdr-ctl>; > >> > ... > >> > }; > >> > > >> > sdr-ports: ports@ffc2507c { > >> > compatible = "altr,sdr-ports"; > >> > reg = <0xffc2507c 0x10>; > >> > clocks = <&ddr_dqs_clk>; > >> > ... > >> > }; > >> > > >> > Maybe we can just skip the outer node that combines all the others. > >> > So, if we do it like that, you can still use syscon, but only for the register > >> > that needs it. And the EDAC definitely needs access to the config register, so > >> > all is good. > >> > > >> >> Is the solution that I don't use request_region() (and therefore not > >> >> request exclusive access) when setting up the SDRAM controller? > >> >> > >> >> If you could point me to your drivers for the other bridges that you > >> >> reference, your code may answer my question. > >> >> > >> > > >> > The other bridges don't need access to any SDRAM controller registers and > >> > I haven't even started to implement the GPV-stuff with the lw2hps-bridge :-( > >> > > >> > 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 | > >> > >> OK now I understand. For register offset 0x0, that's all stuff set up > >> in the bootloader. We won't be touching that register except for the > >> EDAC. So syscon isn't needed here. > >> > >> Each driver that uses some sdr registers can specify which registers > >> it uses in the device tree. So far we don't have any cases of two > >> drivers that share a register. > >> > >> The ECC driver will need two ranges: offset 0x00 and 0x38 through 0x50. > >> The fpga bridges just needs offset 0x80. > >> > > > > Well, almost ;-) > > Use syscon for 0x0 and reference that in the ECC driver, which only is > > responsible for 0x38 to 0x50. Then flip the two EDAC bits via the syscon-phandle. > > Then a bootloader can probe with the same dts and have a driver match on the > > config-register and one for all the DRAM setup (< 0x38), which we don't need in > > the kernel. On the other hand the EDAC seems unneccessary for the bootloader. > > And this all matches the partitioning of the SDR register space, so I think it > > is also a correct hardware description. > > > > 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 | > > We could do that. But is syscon a concept the bootloader will know about? > Depends on the bootloader. I guess u-boot doesn't, but barebox does. Alternative would be to have the bootloader know that it has to look for an ECC node when it instead is looking for DRAM timing/width/... setup. Sounds wrong to me. > Actually it is a bit messier than what I originally said: > ECC driver needs two ranges: offset 0x00 and 0x38 through 0x50. > Machine layer needs control of 0x10, 0x14, 0x54, and 0x58 for putting > SDRAM into self-refresh. I didn't get that. Does the ECC driver need access to this "machine layer"? Regards, Steffen
On Wed, May 28, 2014 at 2:01 AM, Steffen Trumtrar <s.trumtrar@pengutronix.de> wrote: > On Tue, May 27, 2014 at 03:57:47PM -0500, Alan Tull wrote: >> On Tue, May 27, 2014 at 2:42 PM, Steffen Trumtrar >> <s.trumtrar@pengutronix.de> wrote: >> > On Tue, May 27, 2014 at 02:12:17PM -0500, Alan Tull wrote: >> >> On Tue, May 27, 2014 at 2:11 AM, Steffen Trumtrar >> >> <s.trumtrar@pengutronix.de> wrote: >> >> > On Wed, May 21, 2014 at 10:38:34AM -0500, Thor Thayer wrote: >> >> >> On Tue, May 20, 2014 at 9:44 AM, Steffen Trumtrar >> >> >> <s.trumtrar@pengutronix.de> wrote: >> >> >> > Hi! >> >> >> > >> >> >> > On Tue, May 20, 2014 at 09:31:06AM -0500, Alan Tull wrote: >> >> >> >> On Mon, May 19, 2014 at 2:37 PM, Thor Thayer <tthayer.linux@gmail.com> wrote: >> >> >> >> >> >> >> >> >>> >> 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..8f8746b >> >> >> >> >>> >> --- /dev/null >> >> >> >> >>> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt >> >> >> >> >>> >> @@ -0,0 +1,11 @@ >> >> >> >> >>> >> +Altera SOCFPGA SDRAM Controller >> >> >> >> >>> >> + >> >> >> >> >>> >> +Required properties: >> >> >> >> >>> >> +- compatible : "altr,sdr-ctl"; >> >> >> >> >>> >> +- reg : Should contain 1 register ranges(address and length) >> >> >> >> >>> >> + >> >> >> >> >>> >> +Example: >> >> >> >> >>> >> + sdrctl@ffc25000 { >> >> >> >> >>> >> + compatible = "altr,sdr-ctl"; >> >> >> >> >>> >> + 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"; >> >> >> >> >>> > ^^^^^^^^^^ >> >> >> >> >>> > >> >> >> >> >>> > Get rid of that, too, please. >> >> >> >> >>> >> >> >> >> >>> Hi Steffan, >> >> >> >> >>> >> >> >> >> >>> I believe I need to keep the "syscon". The same register (ctrlcfg) >> >> >> >> >>> that has the ECC enable bitfield also includes the DRAM configuration >> >> >> >> >>> bitfields that other drivers will want to access (specifically the >> >> >> >> >>> FPGA bridge needs this information). Since this register will be >> >> >> >> >>> shared between drivers, syscon seems like the best solution. >> >> >> >> >>> >> >> >> >> >> >> >> >> >> >> Hm, from looking at the documentation of the ctrlcfg I can't really >> >> >> >> >> understand which bits you would need for the FPGA brigde and why. >> >> >> >> >> >> >> >> Hi Steffen, >> >> >> >> >> >> >> >> Offset 0x80 in the sdr-ctl is the "fpgaportrst" register. 14 bits >> >> >> >> wide, defaults to 0. When appropriate bits set to 1 in that reg, it >> >> >> >> allows an FPGA port to come out of reset (enables that port). Has no >> >> >> >> other effect on SDRAM configuration. >> >> >> >> >> >> >> >> >> That all sounds like stuff you would want to set for the specific >> >> >> >> >> RAM you are dealing with on a specific board. >> >> >> >> >> What bridge are you talking about? The SDRAM bridge? >> >> >> >> >> >> >> >> Yes, the port allows the FPGA a direct path to the SDRAM. This one >> >> >> >> register the only register in the sdr that the bridge driver needs. >> >> >> >> >> >> >> > >> >> >> > So, what I suggested down ... >> >> >> > >> >> >> >> >> >> >> >> >> >> I can see the problem with the ECC enable, though. >> >> >> >> >> >> >> >> >> >> Regards, >> >> >> >> >> Steffen >> >> >> >> >> >> >> >> >> > >> >> >> >> >>> > sdrctl@ffc25000 { >> >> >> >> >>> > compatible = "altr,sdr-ctl"; >> >> >> >> >>> > reg = <0xffc25000 0x1000>; >> >> >> >> >>> > ranges; >> >> >> >> >>> > >> >> >> >> >>> > edac@ffc2502c { >> >> >> >> >>> > compatible = "altr,sdram-edac"; >> >> >> >> >>> > reg = <0xffc2502c 0x50>; >> >> >> >> >>> > interrupts = <0 39 4>; >> >> >> >> >>> > }; >> >> >> >> >>> > }; >> >> >> >> >>> > >> >> >> >> >>> > Then we can later add: >> >> >> >> >>> > >> >> >> >> >>> > sdr-ports: ports@ffc2507c { >> >> >> >> >>> > #reset-cells = <1>; >> >> >> >> >>> > compatible = "altr,sdr-ports"; >> >> >> >> >>> > reg = <0xffc2507c 0x10>; >> >> >> >> >>> > clocks = <&ddr_dqs_clk>; >> >> >> >> >>> > ... >> >> >> >> >>> > }; >> >> >> >> >> >> >> > >> >> >> > ... here should work. No?! Just a simple driver that registers with the >> >> >> > reset-framework. I would add 0x7c to that driver and than that driver could >> >> >> > "configure" the port and let it out of reset. >> >> >> > >> >> >> > I have done the same thing for the other 3 bridges, but am not finished yet. >> >> >> > Especially the GPV-stuff needs to at least be able to be added later if not now. >> >> >> > >> >> > >> >> > Hi Thor! >> >> > >> >> >> I'm not clear on how the EDAC driver will interact with the registers >> >> >> allocated to the SDRAM controller. If the group of registers from >> >> >> 0xffc25000 to 0xffc26000 is exclusively allocated to the SDRAM >> >> >> controller, how does the EDAC driver cleanly access that single >> >> >> register inside this range? >> >> >> >> >> > >> >> > The compatible in the example is wrong. I didn't mean to map the whole address space >> >> > to some driver. >> >> > I think for the configuration register syscon is the right approach. It is a >> >> > bag of bits that don't necessitate an own driver, so syscon is perfect. >> >> > >> >> > So, let me change my proposal to >> >> > >> >> > sdr-ctl: sdram@ffc25000 { >> >> > compatible = "altr,sdr-ctl", "syscon"; >> >> > reg = <0xffc25000 0x1>; >> >> > }; >> >> > >> >> > edac: edac@ffc2502c { >> >> > compatible = "altr,sdram-edac"; >> >> > reg = <0xffc2502c 0x50>; >> >> > interrupts = <0 39 4>; >> >> > config = <&sdr-ctl>; >> >> > ... >> >> > }; >> >> > >> >> > sdr-ports: ports@ffc2507c { >> >> > compatible = "altr,sdr-ports"; >> >> > reg = <0xffc2507c 0x10>; >> >> > clocks = <&ddr_dqs_clk>; >> >> > ... >> >> > }; >> >> > >> >> > Maybe we can just skip the outer node that combines all the others. >> >> > So, if we do it like that, you can still use syscon, but only for the register >> >> > that needs it. And the EDAC definitely needs access to the config register, so >> >> > all is good. >> >> > >> >> >> Is the solution that I don't use request_region() (and therefore not >> >> >> request exclusive access) when setting up the SDRAM controller? >> >> >> >> >> >> If you could point me to your drivers for the other bridges that you >> >> >> reference, your code may answer my question. >> >> >> >> >> > >> >> > The other bridges don't need access to any SDRAM controller registers and >> >> > I haven't even started to implement the GPV-stuff with the lw2hps-bridge :-( >> >> > >> >> > 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 | >> >> >> >> OK now I understand. For register offset 0x0, that's all stuff set up >> >> in the bootloader. We won't be touching that register except for the >> >> EDAC. So syscon isn't needed here. >> >> >> >> Each driver that uses some sdr registers can specify which registers >> >> it uses in the device tree. So far we don't have any cases of two >> >> drivers that share a register. >> >> >> >> The ECC driver will need two ranges: offset 0x00 and 0x38 through 0x50. >> >> The fpga bridges just needs offset 0x80. >> >> >> > >> > Well, almost ;-) >> > Use syscon for 0x0 and reference that in the ECC driver, which only is >> > responsible for 0x38 to 0x50. Then flip the two EDAC bits via the syscon-phandle. >> > Then a bootloader can probe with the same dts and have a driver match on the >> > config-register and one for all the DRAM setup (< 0x38), which we don't need in >> > the kernel. On the other hand the EDAC seems unneccessary for the bootloader. >> > And this all matches the partitioning of the SDR register space, so I think it >> > is also a correct hardware description. >> > >> > 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 | >> >> We could do that. But is syscon a concept the bootloader will know about? >> > > Depends on the bootloader. I guess u-boot doesn't, but barebox does. OK then we will want offset 0 to be 'syscon'. > Alternative would be to have the bootloader know that it has to look for an > ECC node when it instead is looking for DRAM timing/width/... setup. > Sounds wrong to me. > >> Actually it is a bit messier than what I originally said: >> ECC driver needs two ranges: offset 0x00 and 0x38 through 0x50. >> Machine layer needs control of 0x10, 0x14, 0x54, and 0x58 for putting >> SDRAM into self-refresh. > > I didn't get that. Does the ECC driver need access to this "machine layer"? Nope. Now that I think about it this machine layer will only need access to 0x54, 0x58 and nobody else will need them. So I agree with your suggestion. I think that besides offset 0, these registers will be not be shared between drivers. Thanks! Alan Tull > > 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 |
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..8f8746b --- /dev/null +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt @@ -0,0 +1,11 @@ +Altera SOCFPGA SDRAM Controller + +Required properties: +- compatible : "altr,sdr-ctl"; +- reg : Should contain 1 register ranges(address and length) + +Example: + sdrctl@ffc25000 { + compatible = "altr,sdr-ctl"; + 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>;