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

[Thor Thayer]

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.
>
> 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
--
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/