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

From: Steffen Trumtrar
Date: Tue May 27 2014 - 15:52:13 EST

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@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> 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@xxxxxxxxx> wrote:
> >> >>
> >> >> >>> >> diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
> >> >> >>> >> new file mode 100644
> >> >> >>> >> index 0000000..8f8746b
> >> >> >>> >> --- /dev/null
> >> >> >>> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
> >> >> >>> >> @@ -0,0 +1,11 @@
> >> >> >>> >> +Altera SOCFPGA SDRAM Controller
> >> >> >>> >> +
> >> >> >>> >> +Required properties:
> >> >> >>> >> +- compatible : "altr,sdr-ctl";
> >> >> >>> >> +- reg : Should contain 1 register ranges(address and length)
> >> >> >>> >> +
> >> >> >>> >> +Example:
> >> >> >>> >> + sdrctl@ffc25000 {
> >> >> >>> >> + compatible = "altr,sdr-ctl";
> >> >> >>> >> + reg = <0xffc25000 0x1000>;
> >> >> >>> >> + };
> >> >> >>> >> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> >> >> >>> >> index df43702..6ce912e 100644
> >> >> >>> >> --- a/arch/arm/boot/dts/socfpga.dtsi
> >> >> >>> >> +++ b/arch/arm/boot/dts/socfpga.dtsi
> >> >> >>> >> @@ -676,6 +676,11 @@
> >> >> >>> >> clocks = <&l4_sp_clk>;
> >> >> >>> >> };
> >> >> >>> >>
> >> >> >>> >> + sdrctl@ffc25000 {
> >> >> >>> >> + compatible = "altr,sdr-ctl", "syscon";
> >> >> >>> > ^^^^^^^^^^
> >> >> >>> >
> >> >> >>> > Get rid of that, too, please.
> >> >> >>>
> >> >> >>> Hi Steffan,
> >> >> >>>
> >> >> >>> I believe I need to keep the "syscon". The same register (ctrlcfg)
> >> >> >>> that has the ECC enable bitfield also includes the DRAM configuration
> >> >> >>> bitfields that other drivers will want to access (specifically the
> >> >> >>> FPGA bridge needs this information). Since this register will be
> >> >> >>> shared between drivers, syscon seems like the best solution.
> >> >> >>>
> >> >> >>
> >> >> >> Hm, from looking at the documentation of the ctrlcfg I can't really
> >> >> >> understand which bits you would need for the FPGA brigde and why.
> >> >>
> >> >> Hi Steffen,
> >> >>
> >> >> Offset 0x80 in the sdr-ctl is the "fpgaportrst" register. 14 bits
> >> >> wide, defaults to 0. When appropriate bits set to 1 in that reg, it
> >> >> allows an FPGA port to come out of reset (enables that port). Has no
> >> >> other effect on SDRAM configuration.
> >> >>
> >> >> >> That all sounds like stuff you would want to set for the specific
> >> >> >> RAM you are dealing with on a specific board.
> >> >> >> What bridge are you talking about? The SDRAM bridge?
> >> >>
> >> >> Yes, the port allows the FPGA a direct path to the SDRAM. This one
> >> >> register the only register in the sdr that the bridge driver needs.
> >> >>
> >> >
> >> > So, what I suggested down ...
> >> >
> >> >> >>
> >> >> >> I can see the problem with the ECC enable, though.
> >> >> >>
> >> >> >> Regards,
> >> >> >> Steffen
> >> >> >>
> >> >> >
> >> >> >>> > sdrctl@ffc25000 {
> >> >> >>> > compatible = "altr,sdr-ctl";
> >> >> >>> > reg = <0xffc25000 0x1000>;
> >> >> >>> > ranges;
> >> >> >>> >
> >> >> >>> > edac@ffc2502c {
> >> >> >>> > compatible = "altr,sdram-edac";
> >> >> >>> > reg = <0xffc2502c 0x50>;
> >> >> >>> > interrupts = <0 39 4>;
> >> >> >>> > };
> >> >> >>> > };
> >> >> >>> >
> >> >> >>> > Then we can later add:
> >> >> >>> >
> >> >> >>> > sdr-ports: ports@ffc2507c {
> >> >> >>> > #reset-cells = <1>;
> >> >> >>> > compatible = "altr,sdr-ports";
> >> >> >>> > reg = <0xffc2507c 0x10>;
> >> >> >>> > clocks = <&ddr_dqs_clk>;
> >> >> >>> > ...
> >> >> >>> > };
> >> >>
> >> >
> >> > ... here should work. No?! Just a simple driver that registers with the
> >> > reset-framework. I would add 0x7c to that driver and than that driver could
> >> > "configure" the port and let it out of reset.
> >> >
> >> > I have done the same thing for the other 3 bridges, but am not finished yet.
> >> > Especially the GPV-stuff needs to at least be able to be added later if not now.
> >> >
> >
> > Hi Thor!
> >
> >> I'm not clear on how the EDAC driver will interact with the registers
> >> allocated to the SDRAM controller. If the group of registers from
> >> 0xffc25000 to 0xffc26000 is exclusively allocated to the SDRAM
> >> controller, how does the EDAC driver cleanly access that single
> >> register inside this range?
> >>
> >
> > The compatible in the example is wrong. I didn't mean to map the whole address space
> > to some driver.
> > I think for the configuration register syscon is the right approach. It is a
> > bag of bits that don't necessitate an own driver, so syscon is perfect.
> >
> > So, let me change my proposal to
> >
> > sdr-ctl: sdram@ffc25000 {
> > compatible = "altr,sdr-ctl", "syscon";
> > reg = <0xffc25000 0x1>;
> > };
> >
> > edac: edac@ffc2502c {
> > compatible = "altr,sdram-edac";
> > reg = <0xffc2502c 0x50>;
> > interrupts = <0 39 4>;
> > config = <&sdr-ctl>;
> > ...
> > };
> >
> > sdr-ports: ports@ffc2507c {
> > compatible = "altr,sdr-ports";
> > reg = <0xffc2507c 0x10>;
> > clocks = <&ddr_dqs_clk>;
> > ...
> > };
> >
> > Maybe we can just skip the outer node that combines all the others.
> > So, if we do it like that, you can still use syscon, but only for the register
> > that needs it. And the EDAC definitely needs access to the config register, so
> > all is good.
> >
> >> Is the solution that I don't use request_region() (and therefore not
> >> request exclusive access) when setting up the SDRAM controller?
> >>
> >> If you could point me to your drivers for the other bridges that you
> >> reference, your code may answer my question.
> >>
> >
> > The other bridges don't need access to any SDRAM controller registers and
> > I haven't even started to implement the GPV-stuff with the lw2hps-bridge :-(
> >
> > Regards,
> > Steffen
> >
> > --
> > Pengutronix e.K. | |
> > Industrial Linux Solutions | http://www.pengutronix.de/ |
> > Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> > Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
>
> Thank you for your help on this, Steffen. It is good to have your
> insight. I'll implement the changes and resubmit shortly.
>

Very good. I'm happy that we could come to a conclusion we are all okay with.
(Unless someone else shows up and rains on your parade ;-))

Regards,
Steffen

--
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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/