Date:	Thu, 10 Jul 2014 15:07:04 -0500
From:	Alan Tull <delicious.quinoa@...il.com>
To:	Thor Thayer <tthayer.linux@...il.com>
Cc:	Mark Rutland <mark.rutland@....com>,
	"tthayer@...era.com" <tthayer@...era.com>,
	"robherring2@...il.com" <robherring2@...il.com>,
	Pawel Moll <Pawel.Moll@....com>,
	"ijc+devicetree@...lion.org.uk" <ijc+devicetree@...lion.org.uk>,
	"galak@...eaurora.org" <galak@...eaurora.org>,
	"rob@...dley.net" <rob@...dley.net>,
	"linux@....linux.org.uk" <linux@....linux.org.uk>,
	"dinguyen@...era.com" <dinguyen@...era.com>,
	"dougthompson@...ssion.com" <dougthompson@...ssion.com>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	"bp@...en8.de" <bp@...en8.de>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
	"linux-edac@...r.kernel.org" <linux-edac@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCHv7 2/3] devicetree: Addition of the Altera SDRAM EDAC.

On Wed, Jul 9, 2014 at 3:07 PM, Thor Thayer <tthayer.linux@...il.com> wrote:
>
> On Thu, Jun 26, 2014 at 4:45 AM, Mark Rutland <mark.rutland@....com> wrote:
> > On Wed, Jun 25, 2014 at 10:15:26PM +0100, tthayer@...era.com wrote:
> >> From: Thor Thayer <tthayer@...era.com>
> >>
> >> Add the Altera SDRAM EDAC bindings and device tree changes to the Altera SoC project.
> >>
> >> Signed-off-by: Thor Thayer <tthayer@...era.com>
> >> ---
> >> v2: Changes to SoC EDAC source code.
> >>
> >> v3: Fix typo in device tree documentation.
> >>
> >> v4,v5: No changes - bump version for consistency.
> >>
> >> v6: Assign ECC registers in SDRAM controller to EDAC
> >>
> >> v7: Fix SDRAM EDAC base address.
> >> ---
> >>  .../bindings/arm/altera/socfpga-sdram-edac.txt     |   15 +++++++++++++++
> >>  arch/arm/boot/dts/socfpga.dtsi                     |    6 ++++++
> >>  2 files changed, 21 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..d68e033
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
> >> @@ -0,0 +1,15 @@
> >> +Altera SOCFPGA SDRAM Error Detection & Correction [EDAC]
> >> +
> >> +Required properties:
> >> +- compatible : should contain "altr,sdram-edac";
> >> +- reg : should contain the ECC register range in sdram
> >> +        controller (address and length).
> >> +- interrupts : Should contain the SDRAM ECC IRQ in the
> >> +     appropriate format for the IRQ controller.
> >> +
> >> +Example:
> >> +     sdramedac@...2502c {
> >> +             compatible = "altr,sdram-edac";
> >> +             reg = <0xffc2502c 0x28>;
> >> +             interrupts = <0 39 4>;
> >> +     };
> >> diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
> >> index 310292e..da0785d 100644
> >> --- a/arch/arm/boot/dts/socfpga.dtsi
> >> +++ b/arch/arm/boot/dts/socfpga.dtsi
> >> @@ -687,6 +687,12 @@
> >>                       reg = <0xffc25000 0x4>;
> >>               };
> >>
> >> +             sdramedac@...2502c {
> >> +                     compatible = "altr,sdram-edac";
> >> +                     reg = <0xffc2502c 0x28>;
> >> +                     interrupts = <0 39 4>;
> >> +             };
> >
> > I'm not sure I understand this. The ECC register existing within the
> > SDRAM controller, which we have a binding for. Why do we need a separate
> > binding for a subset of registers within an IP block?
> >
> > Why can we not have a single binding for the entire SDRAM controlelr and
> > decompse that within Linux as it makes sense for the appropriate
> > subsystyems?
> >
> > Leaking Linux design into bindings is a bad idea; it makes it harder to
> > change things.
> >
> > Mark.
>
> Hi Mark,
>
> How would we decompose this within Linux. MFD? Is there an example
> that I can look at?
>
> We originally used syscon for the entire sdram controller register
> block but we got dinged on it.

I think it is good feedback that we don't want the devicetree to break
up this register range (unless someone wants to argue that).

The problem we need to solve here is that the sdram controller
register range has registers that are needed for at least 3 different
functions/drivers: edac, fpga bridges, and loe power mode (putting ddr
in self-refresh).  We would not want to do that in one driver.
Originally we had the whole sdram controller as 'syscon' and that was
frowned upon.  I think someone suggested a MFD.  We can do that and
will do that if that will move things forward.  But that would just be
a mfd that ioremaps the range and provides read/write functions for
other drivers to use (which starts looking very similar to syscon
except that it adds more code to the kernel to do what syscon does).
It seems like that's what syscon was for (looking at
Documentation/devicetree/bindings/mfd/syscon.txt).

So can we have a consensus here on syscon or a very minimal mfd or
some other clear way of moving forward?

Alan

>
> Thanks for your help.
>
> Thor
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists