Re: [PATCH 3/3] edac: altera: Add SDRAM EDAC support for CycloneV/ArriaV

Steffen Trumtrar
2014-04-08T12:46:11Z

On Tue, Apr 08, 2014 at 11:45:25AM +0100, Mark Rutland wrote:
> On Mon, Apr 07, 2014 at 10:54:09PM +0100, tthayer@altera.com wrote:
> > From: Thor Thayer <tthayer@altera.com>
> >
> > Added EDAC support for reporting ECC errors of CycloneV
> > and ArriaV SDRAM controller.
> > - The SDRAM Controller registers are used by the FPGA bridge so
> > these are accessed through the syscon interface.
> > - The configuration of the SDRAM memory size for the EDAC framework
> > is discovered from the memory node of the device tree.
> > - Documentation of the bindings in devicetree/bindings/arm/altera/
> > socfpga-sdram-edac.txt
> > - Correction of single bit errors, detection of double bit errors.
> >
> > Signed-off-by: Thor Thayer <tthayer@altera.com>
> > To: Rob Herring <robherring2@gmail.com>
> > To: Doug Thompson <dougthompson@xmission.com>
> > To: Grant Likely <grant.likely@linaro.org>
> > Cc: Dinh Nguyen <dinguyen@altera.com>
> > Cc: devicetree@vger.kernel.org
> > Cc: linux-edac@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > ---
> > drivers/edac/Kconfig | 9 ++
> > drivers/edac/Makefile | 2 +
> > drivers/edac/altera_mc_edac.c | 360 +++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 371 insertions(+)
> > create mode 100644 drivers/edac/altera_mc_edac.c
>
> [...]
>
> > +/* Get total memory size from Open Firmware DTB */
> > +static u32 altr_sdram_get_total_mem_size(void)
> > +{
> > + struct device_node *np;
> > + u32 retcode, reg_array[2];
> > +
> > + np = of_find_node_by_type(NULL, "memory");
> > + if (!np)
> > + return 0;
> > +
> > + retcode = of_property_read_u32_array(np, "reg",
> > + reg_array, ARRAY_SIZE(reg_array));
>
> There's no requirement that #address-cells = <1> or #size-cells = <1>,
> even if any values in either would fit into 32 bits. If we must read
> this from the DTB rather than elsewhere, please check
> of_n_{addr,size}_cells.
>
> Additionally, it's possible that the physical memory might be described
> over multiple reg entries, or multiple memory nodes for some arbitrary
> reason.
>
> Can we not get this info from elsewhere rather than having to parse the
> memory node within a driver?
>

It should be possible to calculate this from the dramaddrw register in the
SDRAM controller.

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 |