* Add EDAC support for Altera SoC SDRAM Controller
@ 2014-05-15 16:04 tthayer at altera.com
2014-05-15 16:04 ` [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller tthayer at altera.com
` (2 more replies)
0 siblings, 3 replies; 21+ messages in thread
From: tthayer at altera.com @ 2014-05-15 16:04 UTC (permalink / raw)
To: linux-arm-kernel
[PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM
[PATCHv5 2/3] dts: socfpga: Add bindings for Altera SoC SDRAM EDAC
[PATCHv5 3/3] edac: altera: Add EDAC support for Altera SoC SDRAM
^ permalink raw reply [flat|nested] 21+ messages in thread* [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller 2014-05-15 16:04 Add EDAC support for Altera SoC SDRAM Controller tthayer at altera.com @ 2014-05-15 16:04 ` tthayer at altera.com 2014-05-16 7:53 ` Steffen Trumtrar 2014-05-15 16:04 ` [PATCHv5 2/3] dts: socfpga: Add bindings for Altera SoC SDRAM EDAC tthayer at altera.com 2014-05-15 16:04 ` [PATCHv5 3/3] edac: altera: Add EDAC support for Altera SoC SDRAM Controller tthayer at altera.com 2 siblings, 1 reply; 21+ messages in thread From: tthayer at altera.com @ 2014-05-15 16:04 UTC (permalink / raw) To: linux-arm-kernel 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 at 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 at ffc25000 { + compatible = "altr,sdr-ctl", "syscon"; + reg = <0xffc25000 0x1000>; + }; + rstmgr at ffd05000 { compatible = "altr,rst-mgr"; reg = <0xffd05000 0x1000>; -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller 2014-05-15 16:04 ` [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller tthayer at altera.com @ 2014-05-16 7:53 ` Steffen Trumtrar 2014-05-19 18:36 ` Thor Thayer 0 siblings, 1 reply; 21+ messages in thread From: Steffen Trumtrar @ 2014-05-16 7:53 UTC (permalink / raw) To: linux-arm-kernel Hi! On Thu, May 15, 2014 at 11:04:49AM -0500, tthayer at altera.com wrote: > From: Thor Thayer <tthayer@altera.com> > > Addition of the Altera SDRAM controller bindings and device > tree changes to the Altera SoC project. > > 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 at 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 at ffc25000 { > + compatible = "altr,sdr-ctl", "syscon"; ^^^^^^^^^^ Get rid of that, too, please. > + reg = <0xffc25000 0x1000>; > + }; > + How about sdrctl at ffc25000 { compatible = "altr,sdr-ctl"; reg = <0xffc25000 0x1000>; ranges; edac at ffc2502c { compatible = "altr,sdram-edac"; reg = <0xffc2502c 0x50>; interrupts = <0 39 4>; }; }; Then we can later add: sdr-ports: ports at 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 at 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 | ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller 2014-05-16 7:53 ` Steffen Trumtrar @ 2014-05-19 18:36 ` Thor Thayer 2014-05-19 19:12 ` Steffen Trumtrar 0 siblings, 1 reply; 21+ messages in thread From: Thor Thayer @ 2014-05-19 18:36 UTC (permalink / raw) To: linux-arm-kernel 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 at altera.com wrote: >> From: Thor Thayer <tthayer@altera.com> >> >> Addition of the Altera SDRAM controller bindings and device >> tree changes to the Altera SoC project. >> >> 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 at 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 at 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 at ffc25000 { > compatible = "altr,sdr-ctl"; > reg = <0xffc25000 0x1000>; > ranges; > > edac at ffc2502c { > compatible = "altr,sdram-edac"; > reg = <0xffc2502c 0x50>; > interrupts = <0 39 4>; > }; > }; > > Then we can later add: > > sdr-ports: ports at 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 at 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 | ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller 2014-05-19 18:36 ` Thor Thayer @ 2014-05-19 19:12 ` Steffen Trumtrar 2014-05-19 19:37 ` Thor Thayer 0 siblings, 1 reply; 21+ messages in thread From: Steffen Trumtrar @ 2014-05-19 19:12 UTC (permalink / raw) To: linux-arm-kernel 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 at altera.com wrote: > >> From: Thor Thayer <tthayer@altera.com> > >> > >> Addition of the Altera SDRAM controller bindings and device > >> tree changes to the Altera SoC project. > >> > >> 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 at 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 at 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 at ffc25000 { > > compatible = "altr,sdr-ctl"; > > reg = <0xffc25000 0x1000>; > > ranges; > > > > edac at ffc2502c { > > compatible = "altr,sdram-edac"; > > reg = <0xffc2502c 0x50>; > > interrupts = <0 39 4>; > > }; > > }; > > > > Then we can later add: > > > > sdr-ports: ports at 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 | ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller 2014-05-19 19:12 ` Steffen Trumtrar @ 2014-05-19 19:37 ` Thor Thayer 2014-05-20 14:31 ` Alan Tull 0 siblings, 1 reply; 21+ messages in thread From: Thor Thayer @ 2014-05-19 19:37 UTC (permalink / raw) To: linux-arm-kernel 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 at altera.com wrote: >> >> From: Thor Thayer <tthayer@altera.com> >> >> >> >> Addition of the Altera SDRAM controller bindings and device >> >> tree changes to the Altera SoC project. >> >> >> >> 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 at 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 at 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 at ffc25000 { >> > compatible = "altr,sdr-ctl"; >> > reg = <0xffc25000 0x1000>; >> > ranges; >> > >> > edac at ffc2502c { >> > compatible = "altr,sdram-edac"; >> > reg = <0xffc2502c 0x50>; >> > interrupts = <0 39 4>; >> > }; >> > }; >> > >> > Then we can later add: >> > >> > sdr-ports: ports at 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 | ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller 2014-05-19 19:37 ` Thor Thayer @ 2014-05-20 14:31 ` Alan Tull 2014-05-20 14:44 ` Steffen Trumtrar 0 siblings, 1 reply; 21+ messages in thread From: Alan Tull @ 2014-05-20 14:31 UTC (permalink / raw) To: linux-arm-kernel 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 at 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 at 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 at ffc25000 { >>> > compatible = "altr,sdr-ctl"; >>> > reg = <0xffc25000 0x1000>; >>> > ranges; >>> > >>> > edac at ffc2502c { >>> > compatible = "altr,sdram-edac"; >>> > reg = <0xffc2502c 0x50>; >>> > interrupts = <0 39 4>; >>> > }; >>> > }; >>> > >>> > Then we can later add: >>> > >>> > sdr-ports: ports at 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 at vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > Please read the FAQ at http://www.tux.org/lkml/ ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller 2014-05-20 14:31 ` Alan Tull @ 2014-05-20 14:44 ` Steffen Trumtrar 2014-05-21 15:38 ` Thor Thayer 0 siblings, 1 reply; 21+ messages in thread From: Steffen Trumtrar @ 2014-05-20 14:44 UTC (permalink / raw) To: linux-arm-kernel 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 at 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 at 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 at ffc25000 { > >>> > compatible = "altr,sdr-ctl"; > >>> > reg = <0xffc25000 0x1000>; > >>> > ranges; > >>> > > >>> > edac at ffc2502c { > >>> > compatible = "altr,sdram-edac"; > >>> > reg = <0xffc2502c 0x50>; > >>> > interrupts = <0 39 4>; > >>> > }; > >>> > }; > >>> > > >>> > Then we can later add: > >>> > > >>> > sdr-ports: ports at 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 | ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller 2014-05-20 14:44 ` Steffen Trumtrar @ 2014-05-21 15:38 ` Thor Thayer 2014-05-27 7:11 ` Steffen Trumtrar 0 siblings, 1 reply; 21+ messages in thread From: Thor Thayer @ 2014-05-21 15:38 UTC (permalink / raw) To: linux-arm-kernel 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 at 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 at 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 at ffc25000 { >> >>> > compatible = "altr,sdr-ctl"; >> >>> > reg = <0xffc25000 0x1000>; >> >>> > ranges; >> >>> > >> >>> > edac at ffc2502c { >> >>> > compatible = "altr,sdram-edac"; >> >>> > reg = <0xffc2502c 0x50>; >> >>> > interrupts = <0 39 4>; >> >>> > }; >> >>> > }; >> >>> > >> >>> > Then we can later add: >> >>> > >> >>> > sdr-ports: ports at 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller 2014-05-21 15:38 ` Thor Thayer @ 2014-05-27 7:11 ` Steffen Trumtrar 2014-05-27 18:00 ` Thor Thayer 2014-05-27 19:12 ` Alan Tull 0 siblings, 2 replies; 21+ messages in thread From: Steffen Trumtrar @ 2014-05-27 7:11 UTC (permalink / raw) To: linux-arm-kernel 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 at 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 at 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 at ffc25000 { > >> >>> > compatible = "altr,sdr-ctl"; > >> >>> > reg = <0xffc25000 0x1000>; > >> >>> > ranges; > >> >>> > > >> >>> > edac at ffc2502c { > >> >>> > compatible = "altr,sdram-edac"; > >> >>> > reg = <0xffc2502c 0x50>; > >> >>> > interrupts = <0 39 4>; > >> >>> > }; > >> >>> > }; > >> >>> > > >> >>> > Then we can later add: > >> >>> > > >> >>> > sdr-ports: ports at 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 at ffc25000 { compatible = "altr,sdr-ctl", "syscon"; reg = <0xffc25000 0x1>; }; edac: edac at ffc2502c { compatible = "altr,sdram-edac"; reg = <0xffc2502c 0x50>; interrupts = <0 39 4>; config = <&sdr-ctl>; ... }; sdr-ports: ports at 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 | ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller 2014-05-27 7:11 ` Steffen Trumtrar @ 2014-05-27 18:00 ` Thor Thayer 2014-05-27 19:51 ` Steffen Trumtrar 2014-05-27 19:12 ` Alan Tull 1 sibling, 1 reply; 21+ messages in thread From: Thor Thayer @ 2014-05-27 18:00 UTC (permalink / raw) To: linux-arm-kernel 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 at 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 at 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 at ffc25000 { >> >> >>> > compatible = "altr,sdr-ctl"; >> >> >>> > reg = <0xffc25000 0x1000>; >> >> >>> > ranges; >> >> >>> > >> >> >>> > edac at ffc2502c { >> >> >>> > compatible = "altr,sdram-edac"; >> >> >>> > reg = <0xffc2502c 0x50>; >> >> >>> > interrupts = <0 39 4>; >> >> >>> > }; >> >> >>> > }; >> >> >>> > >> >> >>> > Then we can later add: >> >> >>> > >> >> >>> > sdr-ports: ports at 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 at ffc25000 { > compatible = "altr,sdr-ctl", "syscon"; > reg = <0xffc25000 0x1>; > }; > > edac: edac at ffc2502c { > compatible = "altr,sdram-edac"; > reg = <0xffc2502c 0x50>; > interrupts = <0 39 4>; > config = <&sdr-ctl>; > ... > }; > > sdr-ports: ports at 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller 2014-05-27 18:00 ` Thor Thayer @ 2014-05-27 19:51 ` Steffen Trumtrar 0 siblings, 0 replies; 21+ messages in thread From: Steffen Trumtrar @ 2014-05-27 19:51 UTC (permalink / raw) To: linux-arm-kernel 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 at 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 at 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 at ffc25000 { > >> >> >>> > compatible = "altr,sdr-ctl"; > >> >> >>> > reg = <0xffc25000 0x1000>; > >> >> >>> > ranges; > >> >> >>> > > >> >> >>> > edac at ffc2502c { > >> >> >>> > compatible = "altr,sdram-edac"; > >> >> >>> > reg = <0xffc2502c 0x50>; > >> >> >>> > interrupts = <0 39 4>; > >> >> >>> > }; > >> >> >>> > }; > >> >> >>> > > >> >> >>> > Then we can later add: > >> >> >>> > > >> >> >>> > sdr-ports: ports at 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 at ffc25000 { > > compatible = "altr,sdr-ctl", "syscon"; > > reg = <0xffc25000 0x1>; > > }; > > > > edac: edac at ffc2502c { > > compatible = "altr,sdram-edac"; > > reg = <0xffc2502c 0x50>; > > interrupts = <0 39 4>; > > config = <&sdr-ctl>; > > ... > > }; > > > > sdr-ports: ports at 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 -- 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 | ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller 2014-05-27 7:11 ` Steffen Trumtrar 2014-05-27 18:00 ` Thor Thayer @ 2014-05-27 19:12 ` Alan Tull 2014-05-27 19:42 ` Steffen Trumtrar 1 sibling, 1 reply; 21+ messages in thread From: Alan Tull @ 2014-05-27 19:12 UTC (permalink / raw) To: linux-arm-kernel 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 at 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 at 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 at ffc25000 { >> >> >>> > compatible = "altr,sdr-ctl"; >> >> >>> > reg = <0xffc25000 0x1000>; >> >> >>> > ranges; >> >> >>> > >> >> >>> > edac at ffc2502c { >> >> >>> > compatible = "altr,sdram-edac"; >> >> >>> > reg = <0xffc2502c 0x50>; >> >> >>> > interrupts = <0 39 4>; >> >> >>> > }; >> >> >>> > }; >> >> >>> > >> >> >>> > Then we can later add: >> >> >>> > >> >> >>> > sdr-ports: ports at 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 at ffc25000 { > compatible = "altr,sdr-ctl", "syscon"; > reg = <0xffc25000 0x1>; > }; > > edac: edac at ffc2502c { > compatible = "altr,sdram-edac"; > reg = <0xffc2502c 0x50>; > interrupts = <0 39 4>; > config = <&sdr-ctl>; > ... > }; > > sdr-ports: ports at 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller 2014-05-27 19:12 ` Alan Tull @ 2014-05-27 19:42 ` Steffen Trumtrar 2014-05-27 20:57 ` Alan Tull 0 siblings, 1 reply; 21+ messages in thread From: Steffen Trumtrar @ 2014-05-27 19:42 UTC (permalink / raw) To: linux-arm-kernel 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 at 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 at 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 at ffc25000 { > >> >> >>> > compatible = "altr,sdr-ctl"; > >> >> >>> > reg = <0xffc25000 0x1000>; > >> >> >>> > ranges; > >> >> >>> > > >> >> >>> > edac at ffc2502c { > >> >> >>> > compatible = "altr,sdram-edac"; > >> >> >>> > reg = <0xffc2502c 0x50>; > >> >> >>> > interrupts = <0 39 4>; > >> >> >>> > }; > >> >> >>> > }; > >> >> >>> > > >> >> >>> > Then we can later add: > >> >> >>> > > >> >> >>> > sdr-ports: ports at 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 at ffc25000 { > > compatible = "altr,sdr-ctl", "syscon"; > > reg = <0xffc25000 0x1>; > > }; > > > > edac: edac at ffc2502c { > > compatible = "altr,sdram-edac"; > > reg = <0xffc2502c 0x50>; > > interrupts = <0 39 4>; > > config = <&sdr-ctl>; > > ... > > }; > > > > sdr-ports: ports at 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 | ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller 2014-05-27 19:42 ` Steffen Trumtrar @ 2014-05-27 20:57 ` Alan Tull 2014-05-28 7:01 ` Steffen Trumtrar 0 siblings, 1 reply; 21+ messages in thread From: Alan Tull @ 2014-05-27 20:57 UTC (permalink / raw) To: linux-arm-kernel 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 at 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 at 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 at ffc25000 { >> >> >> >>> > compatible = "altr,sdr-ctl"; >> >> >> >>> > reg = <0xffc25000 0x1000>; >> >> >> >>> > ranges; >> >> >> >>> > >> >> >> >>> > edac at ffc2502c { >> >> >> >>> > compatible = "altr,sdram-edac"; >> >> >> >>> > reg = <0xffc2502c 0x50>; >> >> >> >>> > interrupts = <0 39 4>; >> >> >> >>> > }; >> >> >> >>> > }; >> >> >> >>> > >> >> >> >>> > Then we can later add: >> >> >> >>> > >> >> >> >>> > sdr-ports: ports at 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 at ffc25000 { >> > compatible = "altr,sdr-ctl", "syscon"; >> > reg = <0xffc25000 0x1>; >> > }; >> > >> > edac: edac at ffc2502c { >> > compatible = "altr,sdram-edac"; >> > reg = <0xffc2502c 0x50>; >> > interrupts = <0 39 4>; >> > config = <&sdr-ctl>; >> > ... >> > }; >> > >> > sdr-ports: ports at 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller 2014-05-27 20:57 ` Alan Tull @ 2014-05-28 7:01 ` Steffen Trumtrar 2014-05-28 14:38 ` Alan Tull 0 siblings, 1 reply; 21+ messages in thread From: Steffen Trumtrar @ 2014-05-28 7:01 UTC (permalink / raw) To: linux-arm-kernel 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 at 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 at 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 at ffc25000 { > >> >> >> >>> > compatible = "altr,sdr-ctl"; > >> >> >> >>> > reg = <0xffc25000 0x1000>; > >> >> >> >>> > ranges; > >> >> >> >>> > > >> >> >> >>> > edac at ffc2502c { > >> >> >> >>> > compatible = "altr,sdram-edac"; > >> >> >> >>> > reg = <0xffc2502c 0x50>; > >> >> >> >>> > interrupts = <0 39 4>; > >> >> >> >>> > }; > >> >> >> >>> > }; > >> >> >> >>> > > >> >> >> >>> > Then we can later add: > >> >> >> >>> > > >> >> >> >>> > sdr-ports: ports at 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 at ffc25000 { > >> > compatible = "altr,sdr-ctl", "syscon"; > >> > reg = <0xffc25000 0x1>; > >> > }; > >> > > >> > edac: edac at ffc2502c { > >> > compatible = "altr,sdram-edac"; > >> > reg = <0xffc2502c 0x50>; > >> > interrupts = <0 39 4>; > >> > config = <&sdr-ctl>; > >> > ... > >> > }; > >> > > >> > sdr-ports: ports at 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 -- 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 | ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller 2014-05-28 7:01 ` Steffen Trumtrar @ 2014-05-28 14:38 ` Alan Tull 0 siblings, 0 replies; 21+ messages in thread From: Alan Tull @ 2014-05-28 14:38 UTC (permalink / raw) To: linux-arm-kernel 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 at 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 at 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 at ffc25000 { >> >> >> >> >>> > compatible = "altr,sdr-ctl"; >> >> >> >> >>> > reg = <0xffc25000 0x1000>; >> >> >> >> >>> > ranges; >> >> >> >> >>> > >> >> >> >> >>> > edac at ffc2502c { >> >> >> >> >>> > compatible = "altr,sdram-edac"; >> >> >> >> >>> > reg = <0xffc2502c 0x50>; >> >> >> >> >>> > interrupts = <0 39 4>; >> >> >> >> >>> > }; >> >> >> >> >>> > }; >> >> >> >> >>> > >> >> >> >> >>> > Then we can later add: >> >> >> >> >>> > >> >> >> >> >>> > sdr-ports: ports at 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 at ffc25000 { >> >> > compatible = "altr,sdr-ctl", "syscon"; >> >> > reg = <0xffc25000 0x1>; >> >> > }; >> >> > >> >> > edac: edac at ffc2502c { >> >> > compatible = "altr,sdram-edac"; >> >> > reg = <0xffc2502c 0x50>; >> >> > interrupts = <0 39 4>; >> >> > config = <&sdr-ctl>; >> >> > ... >> >> > }; >> >> > >> >> > sdr-ports: ports at 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 | ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCHv5 2/3] dts: socfpga: Add bindings for Altera SoC SDRAM EDAC 2014-05-15 16:04 Add EDAC support for Altera SoC SDRAM Controller tthayer at altera.com 2014-05-15 16:04 ` [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller tthayer at altera.com @ 2014-05-15 16:04 ` tthayer at altera.com 2014-05-15 16:04 ` [PATCHv5 3/3] edac: altera: Add EDAC support for Altera SoC SDRAM Controller tthayer at altera.com 2 siblings, 0 replies; 21+ messages in thread From: tthayer at altera.com @ 2014-05-15 16:04 UTC (permalink / raw) To: linux-arm-kernel From: Thor Thayer <tthayer@altera.com> Addition of the Altera SDRAM EDAC bindings and device tree changes to the Altera SoC project. v2: Changes to SoC EDAC source code. v3: Fix typo in device tree documentation. v4,v5: No changes - bump version for consistency. Signed-off-by: Thor Thayer <tthayer@altera.com> --- .../bindings/arm/altera/socfpga-sdram-edac.txt | 12 ++++++++++++ arch/arm/boot/dts/socfpga.dtsi | 5 +++++ 2 files changed, 17 insertions(+) create mode 100644 Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt new file mode 100644 index 0000000..431e98b --- /dev/null +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt @@ -0,0 +1,12 @@ +Altera SOCFPGA SDRAM Error Detection & Correction [EDAC] + +Required properties: +- compatible : should contain "altr,sdram-edac"; +- interrupts : Should contain the SDRAM ECC IRQ in the + appropriate format for the IRQ controller. + +Example: + sdramedac at 0 { + compatible = "altr,sdram-edac"; + interrupts = <0 39 4>; + }; diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi index 6ce912e..a0ea69b 100644 --- a/arch/arm/boot/dts/socfpga.dtsi +++ b/arch/arm/boot/dts/socfpga.dtsi @@ -681,6 +681,11 @@ reg = <0xffc25000 0x1000>; }; + sdramedac at 0 { + compatible = "altr,sdram-edac"; + interrupts = <0 39 4>; + }; + rstmgr at ffd05000 { compatible = "altr,rst-mgr"; reg = <0xffd05000 0x1000>; -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCHv5 3/3] edac: altera: Add EDAC support for Altera SoC SDRAM Controller 2014-05-15 16:04 Add EDAC support for Altera SoC SDRAM Controller tthayer at altera.com 2014-05-15 16:04 ` [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller tthayer at altera.com 2014-05-15 16:04 ` [PATCHv5 2/3] dts: socfpga: Add bindings for Altera SoC SDRAM EDAC tthayer at altera.com @ 2014-05-15 16:04 ` tthayer at altera.com 2014-05-26 9:57 ` Borislav Petkov 2 siblings, 1 reply; 21+ messages in thread From: tthayer at altera.com @ 2014-05-15 16:04 UTC (permalink / raw) To: linux-arm-kernel From: Thor Thayer <tthayer@altera.com> v2: Use the SDRAM controller registers to calculate memory size instead of the Device Tree. Update To & Cc list. Add maintainer information. v3: EDAC driver cleanup based on comments from Mailing list. v4: Panic on DBE. Add macro around inject-error reads to prevent them from being optimized out. Remove of_match_ptr since this will always use Device Tree. v5: Addition of printk to trigger function to ensure read vars are not optimized out. Signed-off-by: Thor Thayer <tthayer@altera.com> --- MAINTAINERS | 5 + drivers/edac/Kconfig | 9 + drivers/edac/Makefile | 2 + drivers/edac/altera_edac.c | 419 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 435 insertions(+) create mode 100644 drivers/edac/altera_edac.c diff --git a/MAINTAINERS b/MAINTAINERS index e67ea24..ecd1277 100644 --- a/MAINTAINERS +++ b/MAINTAINERS @@ -1290,6 +1290,11 @@ M: Dinh Nguyen <dinguyen@altera.com> S: Maintained F: drivers/clk/socfpga/ +ARM/SOCFPGA SDRAM EDAC SUPPORT +M: Thor Thayer <tthayer@altera.com> +S: Maintained +F: drivers/edac/altera_edac.c + ARM/STI ARCHITECTURE M: Srinivas Kandagatla <srinivas.kandagatla@gmail.com> M: Maxime Coquelin <maxime.coquelin@st.com> diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig index 878f090..4f4d379 100644 --- a/drivers/edac/Kconfig +++ b/drivers/edac/Kconfig @@ -368,4 +368,13 @@ config EDAC_OCTEON_PCI Support for error detection and correction on the Cavium Octeon family of SOCs. +config EDAC_ALTERA_MC + bool "Altera SDRAM Memory Controller EDAC" + depends on EDAC_MM_EDAC && ARCH_SOCFPGA + help + Support for error detection and correction on the + Altera SDRAM memory controller. Note that the + preloader must initialize the SDRAM before loading + the kernel. + endif # EDAC diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile index 4154ed6..9741336 100644 --- a/drivers/edac/Makefile +++ b/drivers/edac/Makefile @@ -64,3 +64,5 @@ obj-$(CONFIG_EDAC_OCTEON_PC) += octeon_edac-pc.o obj-$(CONFIG_EDAC_OCTEON_L2C) += octeon_edac-l2c.o obj-$(CONFIG_EDAC_OCTEON_LMC) += octeon_edac-lmc.o obj-$(CONFIG_EDAC_OCTEON_PCI) += octeon_edac-pci.o + +obj-$(CONFIG_EDAC_ALTERA_MC) += altera_edac.o diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c new file mode 100644 index 0000000..39d3dd4 --- /dev/null +++ b/drivers/edac/altera_edac.c @@ -0,0 +1,419 @@ +/* + * Copyright Altera Corporation (C) 2014. All rights reserved. + * Copyright 2011-2012 Calxeda, Inc. + * + * This program is free software; you can redistribute it and/or modify + * it under the terms and conditions of the GNU General Public License, + * version 2, as published by the Free Software Foundation. + * + * This program is distributed in the hope it will be useful, but WITHOUT + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for + * more details. + * + * This file is subject to the terms and conditions of the GNU General Public + * License. See the file "COPYING" in the main directory of this archive + * for more details. + + * + * Adapted from the highbank_mc_edac driver + * + */ +#include <linux/types.h> +#include <linux/kernel.h> +#include <linux/ctype.h> +#include <linux/edac.h> +#include <linux/interrupt.h> +#include <linux/platform_device.h> +#include <linux/of_platform.h> +#include <linux/uaccess.h> +#include <linux/mfd/syscon.h> +#include <linux/regmap.h> + +#include "edac_core.h" +#include "edac_module.h" + +#define EDAC_MOD_STR "altera_edac" +#define EDAC_VERSION "1" + +/* SDRAM Controller CtrlCfg Register */ +#define CTLCFG 0x00 + +/* SDRAM Controller CtrlCfg Register Bit Masks */ +#define CTLCFG_ECC_EN 0x400 +#define CTLCFG_ECC_CORR_EN 0x800 +#define CTLCFG_GEN_SB_ERR 0x2000 +#define CTLCFG_GEN_DB_ERR 0x4000 + +#define CTLCFG_ECC_AUTO_EN (CTLCFG_ECC_EN | \ + CTLCFG_ECC_CORR_EN) + +/* SDRAM Controller Address Width Register */ +#define DRAMADDRW 0x2C + +/* SDRAM Controller Address Widths Field Register */ +#define DRAMADDRW_COLBIT_MASK 0x001F +#define DRAMADDRW_COLBIT_LSB 0 +#define DRAMADDRW_ROWBIT_MASK 0x03E0 +#define DRAMADDRW_ROWBIT_LSB 5 +#define DRAMADDRW_BANKBIT_MASK 0x1C00 +#define DRAMADDRW_BANKBIT_LSB 10 +#define DRAMADDRW_CSBIT_MASK 0xE000 +#define DRAMADDRW_CSBIT_LSB 13 + +/* SDRAM Controller Interface Data Width Register */ +#define DRAMIFWIDTH 0x30 + +/* SDRAM Controller Interface Data Width Defines */ +#define DRAMIFWIDTH_16B_ECC 24 +#define DRAMIFWIDTH_32B_ECC 40 + +/* SDRAM Controller DRAM Status Register */ +#define DRAMSTS 0x38 + +/* SDRAM Controller DRAM Status Register Bit Masks */ +#define DRAMSTS_SBEERR 0x04 +#define DRAMSTS_DBEERR 0x08 +#define DRAMSTS_CORR_DROP 0x10 + +/* SDRAM Controller DRAM IRQ Register */ +#define DRAMINTR 0x3C + +/* SDRAM Controller DRAM IRQ Register Bit Masks */ +#define DRAMINTR_INTREN 0x01 +#define DRAMINTR_SBEMASK 0x02 +#define DRAMINTR_DBEMASK 0x04 +#define DRAMINTR_CORRDROPMASK 0x08 +#define DRAMINTR_INTRCLR 0x10 + +/* SDRAM Controller Single Bit Error Count Register */ +#define SBECOUNT 0x40 + +/* SDRAM Controller Single Bit Error Count Register Bit Masks */ +#define SBECOUNT_MASK 0x0F + +/* SDRAM Controller Double Bit Error Count Register */ +#define DBECOUNT 0x44 + +/* SDRAM Controller Double Bit Error Count Register Bit Masks */ +#define DBECOUNT_MASK 0x0F + +/* SDRAM Controller ECC Error Address Register */ +#define ERRADDR 0x48 + +/* SDRAM Controller ECC Error Address Register Bit Masks */ +#define ERRADDR_MASK 0xFFFFFFFF + +/* SDRAM Controller ECC Autocorrect Drop Count Register */ +#define DROPCOUNT 0x4C + +/* SDRAM Controller ECC Autocorrect Drop Count Register Bit Masks */ +#define DROPCOUNT_MASK 0x0F + +/* SDRAM Controller ECC AutoCorrect Address Register */ +#define DROPADDR 0x50 + +/* SDRAM Controller ECC AutoCorrect Error Address Register Bit Masks */ +#define DROPADDR_MASK 0xFFFFFFFF + +/* Altera SDRAM Memory Controller data */ +struct altr_sdram_mc_data { + struct regmap *mc_vbase; +}; + +static irqreturn_t altr_sdram_mc_err_handler(int irq, void *dev_id) +{ + struct mem_ctl_info *mci = dev_id; + struct altr_sdram_mc_data *drvdata = mci->pvt_info; + u32 status, err_count, err_addr; + + /* Error Address is shared by both SBE & DBE */ + regmap_read(drvdata->mc_vbase, ERRADDR, &err_addr); + + regmap_read(drvdata->mc_vbase, DRAMSTS, &status); + + if (status & DRAMSTS_DBEERR) { + regmap_read(drvdata->mc_vbase, DBECOUNT, &err_count); + panic("\nEDAC: [%d Uncorrectable errors @ 0x%08X]\n", + err_count, err_addr); + } + if (status & DRAMSTS_SBEERR) { + regmap_read(drvdata->mc_vbase, SBECOUNT, &err_count); + edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, err_count, + err_addr >> PAGE_SHIFT, + err_addr & ~PAGE_MASK, 0, + 0, 0, -1, mci->ctl_name, ""); + } + + regmap_write(drvdata->mc_vbase, DRAMINTR, + (DRAMINTR_INTRCLR | DRAMINTR_INTREN)); + + return IRQ_HANDLED; +} + +#ifdef CONFIG_EDAC_DEBUG +static ssize_t altr_sdr_mc_err_inject_write(struct file *file, + const char __user *data, + size_t count, loff_t *ppos) +{ + struct mem_ctl_info *mci = file->private_data; + struct altr_sdram_mc_data *drvdata = mci->pvt_info; + u32 *ptemp; + dma_addr_t dma_handle; + u32 reg, read_reg; + + ptemp = dma_alloc_coherent(mci->pdev, 16, &dma_handle, GFP_KERNEL); + if (IS_ERR(ptemp)) { + dma_free_coherent(mci->pdev, 16, ptemp, dma_handle); + edac_printk(KERN_ERR, EDAC_MC, + "Inject: Buffer Allocation error\n"); + return -ENOMEM; + } + + regmap_read(drvdata->mc_vbase, CTLCFG, &read_reg); + read_reg &= ~(CTLCFG_GEN_SB_ERR | CTLCFG_GEN_DB_ERR); + + if (count == 3) { + edac_printk(KERN_ALERT, EDAC_MC, + "Inject Double bit error\n"); + regmap_write(drvdata->mc_vbase, CTLCFG, + (read_reg | CTLCFG_GEN_DB_ERR)); + } else { + edac_printk(KERN_ALERT, EDAC_MC, + "Inject Single bit error\n"); + regmap_write(drvdata->mc_vbase, CTLCFG, + (read_reg | CTLCFG_GEN_SB_ERR)); + } + + ptemp[0] = 0x5A5A5A5A; + ptemp[1] = 0xA5A5A5A5; + /* Clear the error injection bits */ + regmap_write(drvdata->mc_vbase, CTLCFG, read_reg); + /* Ensure it has been written out */ + wmb(); + + /* + * To trigger the error, we need to read the data back + * (the data was written with errors above) + * The ACCESS_ONCE macros are used to prevent the + * compiler optimizing these reads out. + */ + reg = ACCESS_ONCE(ptemp[0]); + read_reg = ACCESS_ONCE(ptemp[1]); + /* Force Read */ + rmb(); + + edac_printk(KERN_ALERT, EDAC_MC, "Read Data [0x%X, 0x%X]\n", + reg, read_reg); + + dma_free_coherent(mci->pdev, 16, ptemp, dma_handle); + + return count; +} + +static const struct file_operations altr_sdr_mc_debug_inject_fops = { + .open = simple_open, + .write = altr_sdr_mc_err_inject_write, + .llseek = generic_file_llseek, +}; + +static void altr_sdr_mc_create_debugfs_nodes(struct mem_ctl_info *mci) +{ + if (mci->debugfs) + debugfs_create_file("inject_ctrl", S_IWUSR, mci->debugfs, mci, + &altr_sdr_mc_debug_inject_fops); +} +#else +static void altr_sdr_mc_create_debugfs_nodes(struct mem_ctl_info *mci) +{} +#endif + +/* Get total memory size in bytes */ +static u32 altr_sdram_get_total_mem_size(struct regmap *mc_vbase) +{ + u32 size; + u32 read_reg, row, bank, col, cs, width; + + if (regmap_read(mc_vbase, DRAMADDRW, &read_reg) < 0) + return 0; + + if (regmap_read(mc_vbase, DRAMIFWIDTH, &width) < 0) + return 0; + + col = (read_reg & DRAMADDRW_COLBIT_MASK) >> + DRAMADDRW_COLBIT_LSB; + row = (read_reg & DRAMADDRW_ROWBIT_MASK) >> + DRAMADDRW_ROWBIT_LSB; + bank = (read_reg & DRAMADDRW_BANKBIT_MASK) >> + DRAMADDRW_BANKBIT_LSB; + cs = (read_reg & DRAMADDRW_CSBIT_MASK) >> + DRAMADDRW_CSBIT_LSB; + + /* Correct for ECC as its not addressible */ + if (width == DRAMIFWIDTH_32B_ECC) + width = 32; + if (width == DRAMIFWIDTH_16B_ECC) + width = 16; + + /* calculate the SDRAM size base on this info */ + size = 1 << (row + bank + col); + size = size * cs * (width / 8); + return size; +} + +static int altr_sdram_probe(struct platform_device *pdev) +{ + struct edac_mc_layer layers[2]; + struct mem_ctl_info *mci; + struct altr_sdram_mc_data *drvdata; + struct regmap *mc_vbase; + struct dimm_info *dimm; + u32 read_reg, mem_size; + int irq; + int res = 0; + + /* Validate the SDRAM controller has ECC enabled */ + /* Grab the register values from the sdr-ctl in device tree */ + mc_vbase = syscon_regmap_lookup_by_compatible("altr,sdr-ctl"); + if (IS_ERR(mc_vbase)) { + edac_printk(KERN_ERR, EDAC_MC, + "regmap for altr,sdr-ctl lookup failed.\n"); + return -ENODEV; + } + + if (regmap_read(mc_vbase, CTLCFG, &read_reg) || + ((read_reg & CTLCFG_ECC_AUTO_EN) != CTLCFG_ECC_AUTO_EN)) { + edac_printk(KERN_ERR, EDAC_MC, + "No ECC/ECC disabled [0x%08X]\n", read_reg); + return -ENODEV; + } + + /* Grab memory size from device tree. */ + mem_size = altr_sdram_get_total_mem_size(mc_vbase); + if (mem_size <= 0) { + edac_printk(KERN_ERR, EDAC_MC, + "Unable to calculate memory size\n"); + return -ENODEV; + } + + /* Ensure the SDRAM Interrupt is disabled and cleared */ + if (regmap_write(mc_vbase, DRAMINTR, DRAMINTR_INTRCLR)) { + edac_printk(KERN_ERR, EDAC_MC, + "Error clearing SDRAM ECC IRQ\n"); + return -ENODEV; + } + + irq = platform_get_irq(pdev, 0); + if (irq < 0) { + edac_printk(KERN_ERR, EDAC_MC, + "No irq %d in DT\n", irq); + return -ENODEV; + } + + layers[0].type = EDAC_MC_LAYER_CHIP_SELECT; + layers[0].size = 1; + layers[0].is_virt_csrow = true; + layers[1].type = EDAC_MC_LAYER_CHANNEL; + layers[1].size = 1; + layers[1].is_virt_csrow = false; + mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, + sizeof(struct altr_sdram_mc_data)); + if (!mci) + return -ENOMEM; + + mci->pdev = &pdev->dev; + drvdata = mci->pvt_info; + drvdata->mc_vbase = mc_vbase; + platform_set_drvdata(pdev, mci); + + if (!devres_open_group(&pdev->dev, NULL, GFP_KERNEL)) { + res = -ENOMEM; + goto free; + } + + mci->mtype_cap = MEM_FLAG_DDR3; + mci->edac_ctl_cap = EDAC_FLAG_NONE | EDAC_FLAG_SECDED; + mci->edac_cap = EDAC_FLAG_SECDED; + mci->mod_name = EDAC_MOD_STR; + mci->mod_ver = EDAC_VERSION; + mci->ctl_name = dev_name(&pdev->dev); + mci->scrub_mode = SCRUB_SW_SRC; + mci->dev_name = dev_name(&pdev->dev); + + dimm = *mci->dimms; + dimm->nr_pages = ((mem_size - 1) >> PAGE_SHIFT) + 1; + dimm->grain = 8; + dimm->dtype = DEV_X8; + dimm->mtype = MEM_DDR3; + dimm->edac_mode = EDAC_SECDED; + + res = edac_mc_add_mc(mci); + if (res < 0) + goto err; + + res = devm_request_irq(&pdev->dev, irq, altr_sdram_mc_err_handler, + 0, dev_name(&pdev->dev), mci); + if (res < 0) { + edac_mc_printk(mci, KERN_ERR, + "Unable to request irq %d\n", irq); + res = -ENODEV; + goto err2; + } + + if (regmap_write(drvdata->mc_vbase, DRAMINTR, + (DRAMINTR_INTRCLR | DRAMINTR_INTREN))) { + edac_mc_printk(mci, KERN_ERR, + "Error enabling SDRAM ECC IRQ\n"); + res = -ENODEV; + goto err2; + } + + altr_sdr_mc_create_debugfs_nodes(mci); + + devres_close_group(&pdev->dev, NULL); + + return 0; + +err2: + edac_mc_del_mc(&pdev->dev); +err: + devres_release_group(&pdev->dev, NULL); +free: + edac_mc_free(mci); + edac_printk(KERN_ERR, EDAC_MC, + "EDAC Probe Failed; Error %d\n", res); + + return res; +} + +static int altr_sdram_remove(struct platform_device *pdev) +{ + struct mem_ctl_info *mci = platform_get_drvdata(pdev); + + edac_mc_del_mc(&pdev->dev); + edac_mc_free(mci); + platform_set_drvdata(pdev, NULL); + + return 0; +} + +static const struct of_device_id altr_sdram_ctrl_of_match[] = { + { .compatible = "altr,sdram-edac", }, + {}, +}; +MODULE_DEVICE_TABLE(of, altr_sdram_ctrl_of_match); + +static struct platform_driver altr_sdram_edac_driver = { + .probe = altr_sdram_probe, + .remove = altr_sdram_remove, + .driver = { + .name = "altr_sdram_edac", + .of_match_table = altr_sdram_ctrl_of_match, + }, +}; + +module_platform_driver(altr_sdram_edac_driver); + +MODULE_LICENSE("GPL v2"); +MODULE_AUTHOR("Altera Corporation"); +MODULE_DESCRIPTION("EDAC Driver for Altera SDRAM Controller"); -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCHv5 3/3] edac: altera: Add EDAC support for Altera SoC SDRAM Controller 2014-05-15 16:04 ` [PATCHv5 3/3] edac: altera: Add EDAC support for Altera SoC SDRAM Controller tthayer at altera.com @ 2014-05-26 9:57 ` Borislav Petkov 2014-05-27 17:58 ` Thor Thayer 0 siblings, 1 reply; 21+ messages in thread From: Borislav Petkov @ 2014-05-26 9:57 UTC (permalink / raw) To: linux-arm-kernel On Thu, May 15, 2014 at 11:04:51AM -0500, tthayer at altera.com wrote: > From: Thor Thayer <tthayer@altera.com> > > v2: Use the SDRAM controller registers to calculate memory size > instead of the Device Tree. Update To & Cc list. Add maintainer > information. > > v3: EDAC driver cleanup based on comments from Mailing list. > > v4: Panic on DBE. Add macro around inject-error reads to prevent > them from being optimized out. Remove of_match_ptr since this > will always use Device Tree. > > v5: Addition of printk to trigger function to ensure read vars > are not optimized out. Yeah, you could turn those vX: messages into a real commit message - much better than none at all :-) Other than that, the edac bits look ok. I'll wait out until you guys've sorted the devicetree issues so ping me when it is ready to pick up. Thanks. -- Regards/Gruss, Boris. Sent from a fat crate under my desk. Formatting is fine. -- ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCHv5 3/3] edac: altera: Add EDAC support for Altera SoC SDRAM Controller 2014-05-26 9:57 ` Borislav Petkov @ 2014-05-27 17:58 ` Thor Thayer 0 siblings, 0 replies; 21+ messages in thread From: Thor Thayer @ 2014-05-27 17:58 UTC (permalink / raw) To: linux-arm-kernel On Mon, May 26, 2014 at 4:57 AM, Borislav Petkov <bp@alien8.de> wrote: > On Thu, May 15, 2014 at 11:04:51AM -0500, tthayer at altera.com wrote: >> From: Thor Thayer <tthayer@altera.com> >> >> v2: Use the SDRAM controller registers to calculate memory size >> instead of the Device Tree. Update To & Cc list. Add maintainer >> information. >> >> v3: EDAC driver cleanup based on comments from Mailing list. >> >> v4: Panic on DBE. Add macro around inject-error reads to prevent >> them from being optimized out. Remove of_match_ptr since this >> will always use Device Tree. >> >> v5: Addition of printk to trigger function to ensure read vars >> are not optimized out. > > Yeah, you could turn those vX: messages into a real commit message - > much better than none at all :-) > > Other than that, the edac bits look ok. I'll wait out until you guys've > sorted the devicetree issues so ping me when it is ready to pick up. > > Thanks. > > -- > Regards/Gruss, > Boris. > > Sent from a fat crate under my desk. Formatting is fine. > -- Hi Boris, Sorry about the commit message - I thought I'd fixed it. It will be fixed in the next version. I think Steffen and I have worked out how the devicetree should look. I'll submit the changes soon. Thanks, Thor ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2014-05-28 14:38 UTC | newest] Thread overview: 21+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2014-05-15 16:04 Add EDAC support for Altera SoC SDRAM Controller tthayer at altera.com 2014-05-15 16:04 ` [PATCHv5 1/3] dts: socfpga: Add bindings for Altera SoC SDRAM controller tthayer at altera.com 2014-05-16 7:53 ` Steffen Trumtrar 2014-05-19 18:36 ` Thor Thayer 2014-05-19 19:12 ` Steffen Trumtrar 2014-05-19 19:37 ` Thor Thayer 2014-05-20 14:31 ` Alan Tull 2014-05-20 14:44 ` Steffen Trumtrar 2014-05-21 15:38 ` Thor Thayer 2014-05-27 7:11 ` Steffen Trumtrar 2014-05-27 18:00 ` Thor Thayer 2014-05-27 19:51 ` Steffen Trumtrar 2014-05-27 19:12 ` Alan Tull 2014-05-27 19:42 ` Steffen Trumtrar 2014-05-27 20:57 ` Alan Tull 2014-05-28 7:01 ` Steffen Trumtrar 2014-05-28 14:38 ` Alan Tull 2014-05-15 16:04 ` [PATCHv5 2/3] dts: socfpga: Add bindings for Altera SoC SDRAM EDAC tthayer at altera.com 2014-05-15 16:04 ` [PATCHv5 3/3] edac: altera: Add EDAC support for Altera SoC SDRAM Controller tthayer at altera.com 2014-05-26 9:57 ` Borislav Petkov 2014-05-27 17:58 ` Thor Thayer
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).