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

[Thor Thayer]

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.

Thanks,

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/