Re: [PATCHv3 3/3] edac: altera: Add EDAC support for Altera SDRAM

[Thor Thayer]

On Thu, May 8, 2014 at 7:05 AM, Borislav Petkov b...@alien8.de wrote:
 On Mon, May 05, 2014 at 05:52:17PM -0500, ttha...@altera.com wrote:
 From: Thor Thayer ttha...@altera.com

 Missing commit message.

Whoops. I don't know what happened there. I'll fix it.


 ---
 v2: Use the SDRAM controller registers to calculate memory size
 instead of the Device Tree. Update To  Cc list. Add maintainer
 information.

 v3: EDAC driver cleanup based on comments from Mailing list.

 Signed-off-by: Thor Thayer ttha...@altera.com
 ---
  MAINTAINERS|5 +
  drivers/edac/Kconfig   |9 +
  drivers/edac/Makefile  |2 +
  drivers/edac/altera_edac.c |  411 
 
  4 files changed, 427 insertions(+)
  create mode 100644 drivers/edac/altera_edac.c


snip

 --- /dev/null
 +++ b/drivers/edac/altera_edac.c
 @@ -0,0 +1,411 @@
 +/*
 + *  Copyright Altera Corporation (C) 2014. All rights reserved.
 + *  Copyright 2011-2012 Calxeda, Inc.
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms and conditions of the GNU General Public License,
 + * version 2, as published by the Free Software Foundation.
 + *
 + * This program is distributed in the hope it will be useful, but WITHOUT
 + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
 + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
 + * more details.
 + *
 + * This file is subject to the terms and conditions of the GNU General 
 Public
 + * License.  See the file COPYING in the main directory of this archive
 + * for more details.

 I'm guessing your lawyers want this boilerplate after all?


Yes. Their reasoning is that they want to retain the rights and
warranty language with the file (just in case the COPYING file
changes).

 ...

 +static irqreturn_t altr_sdram_mc_err_handler(int irq, void *dev_id)
 +{
 + struct mem_ctl_info *mci = dev_id;
 + struct altr_sdram_mc_data *drvdata = mci-pvt_info;
 + u32 status = 0, err_count = 0, err_addr = 0;
 +
 + /* Error Address is shared by both SBE  DBE */
 + regmap_read(drvdata-mc_vbase, ERRADDR, err_addr);
 +
 + regmap_read(drvdata-mc_vbase, DRAMSTS, status);
 +
 + if (status  DRAMSTS_DBEERR) {
 + regmap_read(drvdata-mc_vbase, DBECOUNT, err_count);
 + edac_mc_handle_error(HW_EVENT_ERR_UNCORRECTED, mci, err_count,
 +  err_addr  PAGE_SHIFT,
 +  err_addr  ~PAGE_MASK, 0,
 +  0, 0, -1, mci-ctl_name, );

 So, are we setting edac_mc_panic_on_ue now or what are we planning to do
 for uncorrectable errors?

Yes. I tested using edac_core.edac_mc_panic_on_ue=1 from the command
line and it worked fine. I'll add a comment to clarify. BTW, thanks
for your help on that.


 + if (status  DRAMSTS_SBEERR) {
 + regmap_read(drvdata-mc_vbase, SBECOUNT, err_count);

snip

 + if (count == 3) {
 + edac_printk(KERN_ALERT, EDAC_MC,
 + EDAC Inject Double bit error\n);
 + regmap_write(drvdata-mc_vbase, CTLCFG,
 +  (read_reg | CTLCFG_GEN_DB_ERR));
 + } else {
 + edac_printk(KERN_ALERT, EDAC_MC,
 + EDAC Inject Single bit error\n);
 + regmap_write(drvdata-mc_vbase, CTLCFG,
 +  (read_reg | CTLCFG_GEN_SB_ERR));

 You can remove the EDAC  string from those printks above as
 edac_printk already adds the prefix.

Great. Will do.


 + }
 +
 + ptemp[0] = 0x5A5A5A5A;
 + ptemp[1] = 0xA5A5A5A5;
 + regmap_write(drvdata-mc_vbase, CTLCFG, read_reg);
 + /* Ensure it has been written out */
 + wmb();
 +
 + reg = ptemp[0];
 + read_reg = ptemp[1];
 + /* Force Read */
 + rmb();

 Have you checked whether those reads don't get optimized away by the
 compiler? I know, you need them for triggering the error condition.

 Also, you should add a comment here to explain why the reads are being
 done.

I considered using volatile variables, but decided against it after
I read Documentation/volatile-considered-harmful.txt and my situation
doesn't fit into the exemptions. Is there a better way to handle this?

I'll add the comment.

Thanks for reviewing and your comments.

Thor


 --
 Regards/Gruss,
 Boris.

 Sent from a fat crate under my desk. Formatting is fine.
 --
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv3 3/3] edac: altera: Add EDAC support for Altera SDRAM

[Thor Thayer]

On Thu, May 8, 2014 at 5:44 PM, Dinh Nguyen dinh.li...@gmail.com wrote:

 On 5/5/14 5:52 PM, ttha...@altera.com wrote:
 From: Thor Thayer ttha...@altera.com

 ---
 v2: Use the SDRAM controller registers to calculate memory size
 instead of the Device Tree. Update To  Cc list. Add maintainer
 information.

snip

 +
 +err2er altr_sdram_edac_driver = {
 + .probe = altr_sdram_probe,
 + .remove = altr_sdram_remove,
 + .driver = {
 + .name = altr_sdram_edac,
 + .of_match_table = of_match_ptr(altr_sdram_ctrl_of_match),
 I don't think you need of_match_ptr here for this platform as SOCFPGA is
 only a DT platform. All that of_match_ptr() does for a CONFIG_OF platform
 is just return the pointer.

 Dinh

Thanks Dinh. I'll remove it in my next submission.

 + },
 +};
 +
 +module_platform_driver(altr_sdram_edac_driver);
 +
 +MODULE_LICENSE(GPL v2);
 +MODULE_AUTHOR(Altera Corporation);
 +MODULE_DESCRIPTION(EDAC Driver for Altera SDRAM Controller);

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv3 3/3] edac: altera: Add EDAC support for Altera SDRAM

[Thor Thayer]

On Fri, May 9, 2014 at 8:52 AM, Borislav Petkov b...@alien8.de wrote:
 On Thu, May 08, 2014 at 03:37:19PM -0500, Thor Thayer wrote:
 Yes. Their reasoning is that they want to retain the rights and
 warranty language with the file (just in case the COPYING file
 changes).

 Ok, thanks for checking up on this.

 Yes. I tested using edac_core.edac_mc_panic_on_ue=1 from the command
 line and it worked fine. I'll add a comment to clarify. BTW, thanks
 for your help on that.

 Sure, but the question still remains: do you want to panic on
 uncorrectable errors by default or want the user to decide? I guess this
 is something you can answer for your hardware...

Yes, good point. Our hardware can't recover from Double Bit Errors so
I'll go back to the panic() in that path. I like the flexibility of
the command line parameter though...


 I considered using volatile variables, but decided against it after
 I read Documentation/volatile-considered-harmful.txt and my situation
 doesn't fit into the exemptions. Is there a better way to handle this?

 Off the top of my head, I'd first look at compiler asm output to
 check what my compiler does with those writes and then take a look at
 employing the ACCESS_ONCE macro or something similar where we use the
 asm volatile() as an optimization stop for the compiler, among others.

 And then I'll look at asm again to make sure it does what it is supposed
 to do. Something to that effect, in any case...

 HTH.

The reads aren't optimized out now but I'd like to protect against
future optimization changes. I implemented ACCESS_ONCE and checked the
resulting asm output - it looks clean. Thanks.


 --
 Regards/Gruss,
 Boris.

 Sent from a fat crate under my desk. Formatting is fine.
 --
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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.trumt...@pengutronix.de 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.li...@gmail.com 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 000..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- |

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

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

[Thor Thayer]

On Fri, May 16, 2014 at 2:53 AM, Steffen Trumtrar
s.trumt...@pengutronix.de wrote:
 Hi!

 On Thu, May 15, 2014 at 11:04:49AM -0500, ttha...@altera.com wrote:
 From: Thor Thayer ttha...@altera.com

 Addition of the Altera SDRAM controller bindings and device
 tree changes to the Altera SoC project.

 v2: Changes to SoC SDRAM EDAC code.

 v3: Implement code suggestions for SDRAM EDAC code.

 v4: Remove syscon from SDRAM controller bindings.

 v5: No Change, bump version for consistency.

 Signed-off-by: Thor Thayer ttha...@altera.com
 ---
  .../bindings/arm/altera/socfpga-sdram.txt  |   11 +++
  arch/arm/boot/dts/socfpga.dtsi |5 +
  2 files changed, 16 insertions(+)
  create mode 100644 
 Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt

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

I may be misunderstanding something so feel free to elaborate. Thanks
for reviewing.

Thor


 + reg = 0xffc25000 0x1000;
 + };
 +

 How about

 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;
 ...
 };

 to use the reset-controller framework for the port resets.

   rstmgr@ffd05000 {
   compatible = altr,rst-mgr;
   reg = 0xffd05000 0x1000;
 --

 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- |
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Thor Thayer]

On Mon, May 19, 2014 at 2:12 PM, Steffen Trumtrar
s.trumt...@pengutronix.de wrote:
 Hi Thor!

 On Mon, May 19, 2014 at 01:36:30PM -0500, Thor Thayer wrote:
 On Fri, May 16, 2014 at 2:53 AM, Steffen Trumtrar
 s.trumt...@pengutronix.de wrote:
  Hi!
 
  On Thu, May 15, 2014 at 11:04:49AM -0500, ttha...@altera.com wrote:
  From: Thor Thayer ttha...@altera.com
 
  Addition of the Altera SDRAM controller bindings and device
  tree changes to the Altera SoC project.
 
  v2: Changes to SoC SDRAM EDAC code.
 
  v3: Implement code suggestions for SDRAM EDAC code.
 
  v4: Remove syscon from SDRAM controller bindings.
 
  v5: No Change, bump version for consistency.
 
  Signed-off-by: Thor Thayer ttha...@altera.com
  ---
   .../bindings/arm/altera/socfpga-sdram.txt  |   11 +++
   arch/arm/boot/dts/socfpga.dtsi |5 +
   2 files changed, 16 insertions(+)
   create mode 100644 
  Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
 
  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 000..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.
 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?

 I can see the problem with the ECC enable, though.

 Regards,
 Steffen


Hi Steffen,

I'll get more details from the guy working on the FPGA bridge when he
gets back in the office. When I started working on EDAC, that register
had been allocated by the FPGA bridge driver so we decided to use the
syscon to allow sharing of the register.

My understanding was that the FPGA bridge as an SDRAM master would
allow FPGA devices to access the SDRAM.  As part of that process, they
may want to read the SDRAM configuration.

I'll need to get more details from the driver developer because the
FPGA driver is in flux while the appropriate driver architecture is
being discussed.

Thor


  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;
  ...
  };

 --
 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- |
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv5 3/3] edac: altera: Add EDAC support for Altera SoC SDRAM Controller

[Thor Thayer]

On Mon, May 26, 2014 at 4:57 AM, Borislav Petkov b...@alien8.de wrote:
 On Thu, May 15, 2014 at 11:04:51AM -0500, ttha...@altera.com wrote:
 From: Thor Thayer ttha...@altera.com

 v2: Use the SDRAM controller registers to calculate memory size
 instead of the Device Tree. Update To  Cc list. Add maintainer
 information.

 v3: EDAC driver cleanup based on comments from Mailing list.

 v4: Panic on DBE. Add macro around inject-error reads to prevent
 them from being optimized out. Remove of_match_ptr since this
 will always use Device Tree.

 v5: Addition of printk to trigger function to ensure read vars
 are not optimized out.

 Yeah, you could turn those vX: messages into a real commit message -
 much better than none at all :-)

 Other than that, the edac bits look ok. I'll wait out until you guys've
 sorted the devicetree issues so ping me when it is ready to pick up.

 Thanks.

 --
 Regards/Gruss,
 Boris.

 Sent from a fat crate under my desk. Formatting is fine.
 --

Hi Boris,

Sorry about the commit message - I thought I'd fixed it. It will be
fixed in the next version.

I think Steffen and I have worked out how the devicetree should look.
I'll submit the changes soon.

Thanks,

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

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.trumt...@pengutronix.de 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.trumt...@pengutronix.de 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.li...@gmail.com 
  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 000..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

Re: [PATCHv3 3/3] edac: altera: Add EDAC support for Altera SDRAM

[Thor Thayer]

On Fri, May 9, 2014 at 3:52 PM, Borislav Petkov b...@alien8.de wrote:
 On Fri, May 09, 2014 at 03:31:53PM -0500, Thor Thayer wrote:
 Yes, good point. Our hardware can't recover from Double Bit Errors so
 I'll go back to the panic() in that path. I like the flexibility of
 the command line parameter though...

 Like to panic by default when the machine is booted normally but to be
 able to turn off the panicking with a module parameter?

 If so, then you could probably implement a trivial setter called

 edac_mc_set_panic_on_ue() in a separate patch.

 Then, you call it at the end of altr_sdram_probe().

 If you want to turn it off again, you do

 echo 0  /sys/module/edac_core/parameters/edac_mc_panic_on_ue

 Something like that...

 --
 Regards/Gruss,
 Boris.

 Sent from a fat crate under my desk. Formatting is fine.
 --

Hi Boris. Yes. this would be a nice solution. I'll use the panic for
now in my next revision of the patch - we know a DBE is bad news.
Thanks!
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv4 3/3] edac: altera: Add EDAC support for Altera SoC SDRAM Controller

[Thor Thayer]

On Mon, May 12, 2014 at 7:12 PM, Borislav Petkov b...@alien8.de wrote:
 On Mon, May 12, 2014 at 06:36:57PM -0500, ttha...@altera.com wrote:
 + ptemp[0] = 0x5A5A5A5A;
 + ptemp[1] = 0xA5A5A5A5;
 + /* Clear the error injection bits */
 + regmap_write(drvdata-mc_vbase, CTLCFG, read_reg);
 + /* Ensure it has been written out */
 + wmb();
 +
 + /*
 +  * To trigger the error, we need to read the data back
 +  * (the data was written with errors above)
 +  * The ACCESS_ONCE macros are used to prevent the
 +  * compiler optimizing these reads out.
 +  */
 + reg = ACCESS_ONCE(ptemp[0]);
 + read_reg = ACCESS_ONCE(ptemp[1]);
 + /* Force Read */
 + rmb();

 Right, I still am a bit unsure about this thing. So sure, we funnel
 ptemp through an asm() block which stops the optimizer but we assign the
 results to reg and read_reg, i.e. two local variables which aren't used
 in this function anymore.

 Thus, I don't see what stops the compiler from discarding them along
 with the ACCESS_ONCE() reads at some point, when it becomes really
 smart. Basically killing

 + reg = ACCESS_ONCE(ptemp[0]);
 + read_reg = ACCESS_ONCE(ptemp[1]);

 as they have to effect.

 Maybe we can do the reads in asm - this should be pretty safe.

 I.e., something like this:

   asm volatile( : =r (reg), =r (read_reg),
   : 0 (ptemp[0]), 1 (ptemp[1]));

 and it does it on x86 (did a small test program) by shuffling the values
 through registers so we definitely have the reads.

 .loc 1 28 0
 movl-48(%rbp), %edx # ptemp, D.2240
 movl-44(%rbp), %eax # ptemp, D.2240
 .loc 1 27 0
 movl%edx, -28(%rbp) # reg, reg
 movl%eax, -32(%rbp) # read_reg, read_reg

 Btw, if you only want to do the reads, you can simply tell gcc to put them in
 registers

   asm volatile( :: r (ptemp[0]), r (ptemp[1]));

 and be done with it but I don't know how smart it is with register
 allocation so as to recognize that they're already in registers (when
 they already are in some registers) and discard the reads.

 Fun stuff! :-)

 --
 Regards/Gruss,
 Boris.

 Sent from a fat crate under my desk. Formatting is fine.
 --

Hi Boris.

I'll look into that. I think I will use the registers in a
edac_printk(). Since this is a debugging trigger function, the
printout may be useful anyway.

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

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

[Thor Thayer]

Hi Sören

On Mon, May 5, 2014 at 6:16 PM, Sören Brinkmann
soren.brinkm...@xilinx.com wrote:

 Hi Thor,

 On Mon, 2014-05-05 at 05:52PM -0500, ttha...@altera.com wrote:
  From: Thor Thayer ttha...@altera.com
 
  Addition of the Altera SDRAM controller bindings and device
  tree changes to the Altera SoC project. The syscon parameter
  is included here because the SDRAM EDAC bits are shared with the SDRAM
  configuration bits.
  ---
  v2: Changes to SoC SDRAM EDAC code.
 
  V3: Implement code suggestions for SDRAM EDAC code.
 
  Signed-off-by: Thor Thayer ttha...@altera.com
  ---
   .../bindings/arm/altera/socfpga-sdram.txt  |   14 ++
   arch/arm/boot/dts/socfpga.dtsi |5 +
   2 files changed, 19 insertions(+)
   create mode 100644 
  Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
 
  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 000..525cb76
  --- /dev/null
  +++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt
  @@ -0,0 +1,14 @@
  +Altera SOCFPGA SDRAM Controller
  +
  +Required properties:
  +- compatible : altr,sdr-ctl, syscon;
  +Note that syscon is invoked for this device to support the 
  FPGA
  + bridge driver, EDAC driver and other devices that share the
  + registers.

 This sounds like implementation specifics, which shouldn't be part of
 the bindings.

 Sören

I see your point and will remove. Thanks!
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv3 3/3] edac: altera: Add EDAC support for Altera SDRAM

[Thor Thayer]

On Tue, May 6, 2014 at 10:42 AM, Dinh Nguyen dingu...@altera.com wrote:
 On Mon, 2014-05-05 at 17:52 -0500, Thor Thayer wrote:
 From: Thor Thayer ttha...@altera.com

 ---
 v2: Use the SDRAM controller registers to calculate memory size
 instead of the Device Tree. Update To  Cc list. Add maintainer
 information.

 v3: EDAC driver cleanup based on comments from Mailing list.

 Signed-off-by: Thor Thayer ttha...@altera.com
 ---
  MAINTAINERS|5 +
  drivers/edac/Kconfig   |9 +
  drivers/edac/Makefile  |2 +
  drivers/edac/altera_edac.c |  411 
 
  4 files changed, 427 insertions(+)
  create mode 100644 drivers/edac/altera_edac.c

 diff --git a/MAINTAINERS b/MAINTAINERS
 index e67ea24..ecd1277 100644
 --- a/MAINTAINERS
 +++ b/MAINTAINERS
 @@ -1290,6 +1290,11 @@ M: Dinh Nguyen dingu...@altera.com
  S:   Maintained
  F:   drivers/clk/socfpga/

 +ARM/SOCFPGA SDRAM EDAC SUPPORT
 +M:   Thor Thayer ttha...@altera.com
 +S:   Maintained
 +F:   drivers/edac/altera_edac.c
 +
  ARM/STI ARCHITECTURE
  M:   Srinivas Kandagatla srinivas.kandaga...@gmail.com
  M:   Maxime Coquelin maxime.coque...@st.com
 diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
 index 878f090..4f4d379 100644
 --- a/drivers/edac/Kconfig
 +++ b/drivers/edac/Kconfig
 @@ -368,4 +368,13 @@ config EDAC_OCTEON_PCI
 Support for error detection and correction on the
 Cavium Octeon family of SOCs.

 +config EDAC_ALTERA_MC
 + bool Altera SDRAM Memory Controller EDAC

 Can this be tristate?

 Dinh

Hi Dinh,

We elected to make this a compile time option because it requires a
special preloader to setup the SDRAM for EDAC.

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

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

[Thor Thayer]

On Tue, 2014-04-08 at 12:08 +0200, Borislav Petkov wrote:
 On Mon, Apr 07, 2014 at 04:54:09PM -0500, ttha...@altera.com wrote:
  From: Thor Thayer ttha...@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.
 
 Before I go and take a look at this further, is anyone at Altera going
 to maintain this driver in case of bug reports and issues with it?

Yes, Altera has a group specifically supporting Linux drivers on the
Altera SoCs.

 
 Also, I see patch 3/3. Are the other two related? I see they are
 devicetree additions and, as such, all three should go together...
 

I was told that the device tree additions should be separate patches in
a series since the device tree additions go to a separate group for
approval.

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Thor Thayer]

On Tue, 2014-04-08 at 14:45 +0200, Steffen Trumtrar wrote:
 On Tue, Apr 08, 2014 at 11:45:25AM +0100, Mark Rutland wrote:
  On Mon, Apr 07, 2014 at 10:54:09PM +0100, ttha...@altera.com wrote:
   From: Thor Thayer ttha...@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 ttha...@altera.com
   To: Rob Herring robherri...@gmail.com
   To: Doug Thompson dougthomp...@xmission.com
   To: Grant Likely grant.lik...@linaro.org
   Cc: Dinh Nguyen dingu...@altera.com
   Cc: devicet...@vger.kernel.org
   Cc: linux-e...@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.

Thank you all for the comments. I will look into this further.

 
 Regards,
 Steffen
 

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Thor Thayer]

On Tue, 2014-04-08 at 15:38 +0200, Steffen Trumtrar wrote:
 Hi!
 
 On Mon, Apr 07, 2014 at 04:54:07PM -0500, ttha...@altera.com wrote:
  From: Thor Thayer ttha...@altera.com
  
  Addition of the Altera SDRAM controller bindings and device
  tree changes to the Altera SoC project.
  
[snip]
  +
  +Required properties:
  +- compatible : altr,sdr-ctl, syscon;
  +Note that syscon is invoked for this device to support the 
  FPGA
  +   bridge driver, EDAC driver and other devices that share the
  +   registers.
  +- reg : Should contain 1 register ranges(address and length)
 
 I haven't really thought this through, but why would the FPGA bridge driver
 access the sdram controller? For releasing the resets in fpgaportrst ? Or is
 there more?

Hi Steffan. No, not for resets. We need to enable the FPGA to SDRAM
path. Our SDRAM controller allows FPGA master access to the SDRAM.

 Wouldn't it be more appropriate to represent those bits as a reset-controller 
 to
 some hypothetical IP core driver?
 Then you could have something like
 
   hps2fpga@c000 {
   ipcore@0 {
   resets = sdr 1;
   reset-names = foo;
   resets = rst 96;
   reset-names = bar;
   (...)
   };
 
   ipcore@1000 {
   resets = rst 96;
   reset-names = baz;
   (...)
   };
   };
 
 And you would always have the correct bridges released out of reset for your
 IP core. Does the FPGA bridge driver do more? I think that is basically it.
 Where we maybe could run into problems though is the early_init stuff.
 
 I think syscon is nice for some things, but we should try not to overuse it.

Understood. In this case, syscon seems to be appropriate.
 
 Regards,
 Steffen
 
  +Example:
  +   sdrctl@ffc25000 {
  +   compatible = altr,sdr-ctl, syscon;
  +   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;
  +   reg = 0xffc25000 0x1000;
  +   };
  +
  rstmgr@ffd05000 {
  compatible = altr,rst-mgr;
  reg = 0xffd05000 0x1000;
 

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Thor Thayer]

On Tue, 2014-04-08 at 18:22 +0200, Borislav Petkov wrote:
 On Tue, Apr 08, 2014 at 05:10:54PM +0100, Mark Rutland wrote:
  Typically the bindings would go with the driver via the appropriate
  subsystem maintainer. That way we don't get bindings without drivers
  or vice-versa if there's a problem part-way, and we don't end up with
  every other driver going via a dt tree.
 
 Ok, I can pick them up then, once review is done.
 
 @Thor, please CC linux-edac on the whole patchset for your next
 submission.

OK. Sorry. I'll add that on V2. Thanks for the gentle pointers.
 
 Thanks.
 

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Thor Thayer]

On Wed, 2014-04-23 at 16:54 +0200, Borislav Petkov wrote:
 On Tue, Apr 15, 2014 at 06:30:10PM -0500, ttha...@altera.com wrote:
  From: Thor Thayer ttha...@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 SDRAM Controller registers.
  - Documentation of the bindings in devicetree/bindings/arm/altera/
socfpga-sdram-edac.txt
  - Correction of single bit errors, detection of double bit errors.
  
  ---
  v2: Use the SDRAM controller registers to calculate memory size
  instead of the Device Tree. Update To  Cc list. Add maintainer 
  information.
  
  Signed-off-by: Thor Thayer ttha...@altera.com

[snip]

  @@ -0,0 +1,393 @@
  +/*
  + *  Copyright Altera Corporation (C) 2014. All rights reserved.
  + *  Copyright 2011-2012 Calxeda, Inc.
  + *
  + * This program is free software; you can redistribute it and/or modify
  + * it under the terms and conditions of the GNU General Public License,
  + * version 2, as published by the Free Software Foundation.
  + *
  + * This program is distributed in the hope it will be useful, but WITHOUT
  + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
  + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License 
  for
  + * more details.
  + *
  + * You should have received a copy of the GNU General Public License along 
  with
  + * this program.  If not, see http://www.gnu.org/licenses/.
 
 Please drop this boilerplate and point to COPYING in a single sentence
 stating that it is licensed under GPLv2.

Thank you for reviewing. This is the only review item that may be a
problem. 

  + *
  + * Adapted from the highbank_mc_edac driver
  + *
  + */
  +#include linux/types.h
  +#include linux/kernel.h
  +#include linux/ctype.h
  +#include linux/edac.h
  +#include linux/interrupt.h
  +#include linux/platform_device.h
  +#include linux/of_platform.h
  +#include linux/uaccess.h
  +#include linux/mfd/syscon.h
  +#include linux/regmap.h
  +
  +#include edac_core.h
  +#include edac_module.h
  +
  +#define ALTR_EDAC_MOD_STR  altera_edac
 
 and yet the filename is called altera_mc_edac.c. Please change it to
 altera_edac.c too.
 
  +
  +/* SDRAM Controller CtrlCfg Register */

[snip]

  +
  +/* SDRAM Controller ECC AutoCorrect Error Address Register Bit Masks */
  +#define ALTR_SDR_DROPADDR_MASK 0x
 
 Right, those defines are pefectly fine 'n all but they're used only
 here, in thie file locally. So you probably could drop this ALTR_SDR_
 prefix and thus make them substantially shorter and as a result, the
 code more readable. It'll also shorten the code below, for example:
 
  +   regmap_write(drvdata-mc_vbase, ALTR_SDR_DRAMINTR,
  +(ALTR_SDR_DRAMINTR_INTRCLR | ALTR_SDR_DRAMINTR_INTREN));
 
 would become
 
 
   regmap_write(drvdata-mc_vbase, DRAMINTR, (DRAMINTR_INTRCLR |
  DRAMINTR_INTREN));
 
 which one can read even with one eye opened. :-)
 

Noted. I will make the change. Thanks.

  +
  +/* Altera SDRAM Memory Controller data */
  +struct altr_sdram_mc_data {
  +   struct regmap *mc_vbase;
  +};
  +
  +static irqreturn_t altr_sdram_mc_err_handler(int irq, void *dev_id)
  +{
  +   struct mem_ctl_info *mci = dev_id;
  +   struct altr_sdram_mc_data *drvdata = mci-pvt_info;
  +   u32 status = 0, err_count = 0, err_addr = 0;
  +
  +   /* Error Address is shared by both SBE  DBE */
  +   regmap_read(drvdata-mc_vbase, ALTR_SDR_ERRADDR, err_addr);
  +
  +   regmap_read(drvdata-mc_vbase, ALTR_SDR_DRAMSTS, status);
  +
  +   if (status  ALTR_SDR_DRAMSTS_DBEERR) {
  +   regmap_read(drvdata-mc_vbase, ALTR_SDR_DBECOUNT, err_count);
  +   panic(\nEDAC: [%d Uncorrectable errors @ 0x%08X]\n,
  + err_count, err_addr);
  +   }
 
 Right, ok, I guess you know what you're doing here. I'm guessing there's
 no more graceful recovery than panic when encountering UEs on this
 platform...
 

The concern is that we could execute invalid instructions. I noticed the
'edac_mc_panic_on_ue' module parameter but wanted this to be obvious. I
will revisit the module parameter though. Thank you.

  +   if (status  ALTR_SDR_DRAMSTS_SBEERR) {
  +   regmap_read(drvdata-mc_vbase, ALTR_SDR_SBECOUNT, err_count);
  +   edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci, err_count,
  +err_addr  PAGE_SHIFT,
  +err_addr  ~PAGE_MASK, 0,
  +0, 0, -1, mci-ctl_name, );
  +   }
  +
  +   regmap_write(drvdata-mc_vbase, ALTR_SDR_DRAMINTR,
  +(ALTR_SDR_DRAMINTR_INTRCLR | ALTR_SDR_DRAMINTR_INTREN));
  +
  +   return IRQ_HANDLED;
  +}
  +
  +#ifdef CONFIG_EDAC_DEBUG
  +static

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

[Thor Thayer]

On Mon, 2014-04-21 at 12:27 +0200, Pavel Machek wrote:
 Hi!
 
  From: Thor Thayer ttha...@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 SDRAM Controller registers.
  - Documentation of the bindings in devicetree/bindings/arm/altera/
socfpga-sdram-edac.txt
  - Correction of single bit errors, detection of double bit errors.
  
  ---
  v2: Use the SDRAM controller registers to calculate memory size
  instead of the Device Tree. Update To  Cc list. Add maintainer 
  information.
 
 I'd reduce number of *s in the messages, otherwise
 
 Reviewed-by: Pavel Machek pa...@ucw.cz
 
 for whole series.
   Pavel
 
Hi Pavel.

Noted - I will make the change. Thank you for reviewing.

Thor

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Thor Thayer]

On Tue, 2014-04-08 at 13:52 -0500, Rob Herring wrote:
 On Tue, Apr 8, 2014 at 11:02 AM, delicious quinoa
 delicious.qui...@gmail.com wrote:
  On Tue, Apr 8, 2014 at 9:33 AM, Steffen Trumtrar
  s.trumt...@pengutronix.de wrote:
  On Tue, Apr 08, 2014 at 09:29:50AM -0500, Thor Thayer wrote:
  On Tue, 2014-04-08 at 15:38 +0200, Steffen Trumtrar wrote:
   Hi!
  
   On Mon, Apr 07, 2014 at 04:54:07PM -0500, ttha...@altera.com wrote:
From: Thor Thayer ttha...@altera.com
   
Addition of the Altera SDRAM controller bindings and device
tree changes to the Altera SoC project.
   
  [snip]
+
+Required properties:
+- compatible : altr,sdr-ctl, syscon;
+Note that syscon is invoked for this device to 
support the FPGA
+ bridge driver, EDAC driver and other devices that share the
+ registers.
+- reg : Should contain 1 register ranges(address and length)
  
   I haven't really thought this through, but why would the FPGA bridge 
   driver
   access the sdram controller? For releasing the resets in fpgaportrst ? 
   Or is
   there more?
 
  Hi Steffan. No, not for resets. We need to enable the FPGA to SDRAM
  path. Our SDRAM controller allows FPGA master access to the SDRAM.
 
 
  Yes. But what you have to do to enable the path is let the FPGA port you 
  use
  out of reset. And that is it as far as I can see. The rest happens in the
  bitstream. Or is there more to enable the path?
  The FPGA2SDRAM bridge is the one I didn't use as of yet, so if I miss 
  something
  please elaborate.
 
  Hi Steffen,
 
  The sdram controller is used by two drivers.  That's why we want to
  specify syscon here.  The other driver is the FPGA bridge driver.
  Its functionality is very separate from what this driver is doing (we
  are not enabling the bridge in this driver; we are enabling the
  monitoring and resetting the interrupt bit of the EDAC).  We wanted to
  specify syscon her so that we don't have to have to change it for
  the other driver.
 
 But are there actually overlapping registers which are accessed by
 both drivers and need the protection of regmap?
 
 Perhaps MFD is more appropriate than syscon?

Hi Rob,

We are accessing bits in the SDRAM Controller's Control register which
has other bits that configure the SDRAM controller. Since this main
control register may be accessed by other drivers (more likely for
reading the current SDRAM configuration setup than for writing), the
syscon still seems like an appropriate use.

Thor


 
 Rob


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Thor Thayer]

On Tue, 2014-04-08 at 13:52 -0500, Rob Herring wrote:
 On Tue, Apr 8, 2014 at 11:02 AM, delicious quinoa
 delicious.qui...@gmail.com wrote:
  On Tue, Apr 8, 2014 at 9:33 AM, Steffen Trumtrar
  s.trumt...@pengutronix.de wrote:
  On Tue, Apr 08, 2014 at 09:29:50AM -0500, Thor Thayer wrote:
  On Tue, 2014-04-08 at 15:38 +0200, Steffen Trumtrar wrote:
   Hi!
  
   On Mon, Apr 07, 2014 at 04:54:07PM -0500, ttha...@altera.com wrote:
From: Thor Thayer ttha...@altera.com
   
Addition of the Altera SDRAM controller bindings and device
tree changes to the Altera SoC project.
   
  [snip]
+
+Required properties:
+- compatible : altr,sdr-ctl, syscon;
+Note that syscon is invoked for this device to 
support the FPGA
+ bridge driver, EDAC driver and other devices that share the
+ registers.
+- reg : Should contain 1 register ranges(address and length)
  
   I haven't really thought this through, but why would the FPGA bridge 
   driver
   access the sdram controller? For releasing the resets in fpgaportrst ? 
   Or is
   there more?
 
  Hi Steffan. No, not for resets. We need to enable the FPGA to SDRAM
  path. Our SDRAM controller allows FPGA master access to the SDRAM.
 
 
  Yes. But what you have to do to enable the path is let the FPGA port you 
  use
  out of reset. And that is it as far as I can see. The rest happens in the
  bitstream. Or is there more to enable the path?
  The FPGA2SDRAM bridge is the one I didn't use as of yet, so if I miss 
  something
  please elaborate.
 
  Hi Steffen,
 
  The sdram controller is used by two drivers.  That's why we want to
  specify syscon here.  The other driver is the FPGA bridge driver.
  Its functionality is very separate from what this driver is doing (we
  are not enabling the bridge in this driver; we are enabling the
  monitoring and resetting the interrupt bit of the EDAC).  We wanted to
  specify syscon her so that we don't have to have to change it for
  the other driver.
 
 But are there actually overlapping registers which are accessed by
 both drivers and need the protection of regmap?
 
 Perhaps MFD is more appropriate than syscon?

Hi Rob,

We are accessing bits in the SDRAM Controller's Control register which
has other bits that configure the SDRAM controller. Since this main
control register may be accessed by other drivers (more likely for
reading the current SDRAM configuration setup than for writing), the
syscon still seems like an appropriate use.

Thor


 
 Rob


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

[Thor Thayer]

On Tue, 2014-04-08 at 13:52 -0500, Rob Herring wrote:
 On Tue, Apr 8, 2014 at 11:02 AM, delicious quinoa
 delicious.qui...@gmail.com wrote:
  On Tue, Apr 8, 2014 at 9:33 AM, Steffen Trumtrar
  s.trumt...@pengutronix.de wrote:
  On Tue, Apr 08, 2014 at 09:29:50AM -0500, Thor Thayer wrote:
  On Tue, 2014-04-08 at 15:38 +0200, Steffen Trumtrar wrote:
   Hi!
  
   On Mon, Apr 07, 2014 at 04:54:07PM -0500, ttha...@altera.com wrote:
From: Thor Thayer ttha...@altera.com
   
Addition of the Altera SDRAM controller bindings and device
tree changes to the Altera SoC project.
   
  [snip]
+
+Required properties:
+- compatible : altr,sdr-ctl, syscon;
+Note that syscon is invoked for this device to 
support the FPGA
+ bridge driver, EDAC driver and other devices that share the
+ registers.
+- reg : Should contain 1 register ranges(address and length)
  
   I haven't really thought this through, but why would the FPGA bridge 
   driver
   access the sdram controller? For releasing the resets in fpgaportrst ? 
   Or is
   there more?
 
  Hi Steffan. No, not for resets. We need to enable the FPGA to SDRAM
  path. Our SDRAM controller allows FPGA master access to the SDRAM.
 
 
  Yes. But what you have to do to enable the path is let the FPGA port you 
  use
  out of reset. And that is it as far as I can see. The rest happens in the
  bitstream. Or is there more to enable the path?
  The FPGA2SDRAM bridge is the one I didn't use as of yet, so if I miss 
  something
  please elaborate.
 
  Hi Steffen,
 
  The sdram controller is used by two drivers.  That's why we want to
  specify syscon here.  The other driver is the FPGA bridge driver.
  Its functionality is very separate from what this driver is doing (we
  are not enabling the bridge in this driver; we are enabling the
  monitoring and resetting the interrupt bit of the EDAC).  We wanted to
  specify syscon her so that we don't have to have to change it for
  the other driver.
 
 But are there actually overlapping registers which are accessed by
 both drivers and need the protection of regmap?
 
 Perhaps MFD is more appropriate than syscon?

Hi Rob,

We are accessing bits in the SDRAM Controller's Configuration register which
has other bits that configure the SDRAM controller. Since this main
configuration register may be accessed by other drivers (more likely for
reading the current SDRAM configuration setup than for writing), the
syscon still seems like an appropriate use.

Thor


 
 Rob


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv7 2/3] devicetree: Addition of the Altera SDRAM EDAC.

[Thor Thayer]

On Thu, Jun 26, 2014 at 4:45 AM, Mark Rutland mark.rutl...@arm.com wrote:
 On Wed, Jun 25, 2014 at 10:15:26PM +0100, ttha...@altera.com wrote:
 From: Thor Thayer ttha...@altera.com

 Add the Altera SDRAM EDAC bindings and device tree changes to the Altera SoC 
 project.

 Signed-off-by: Thor Thayer ttha...@altera.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 000..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@ffc2502c {
 + 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@ffc2502c {
 + 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.

Thanks for your help.

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

Re: [PATCHv7 3/3] edac: altera: Add EDAC support for Altera SoC SDRAM Controller.

[Thor Thayer]

Hi Dinh,

On Wed, Jun 25, 2014 at 4:12 PM, Dinh Nguyen dinh.li...@gmail.com wrote:
 Hi Thor,


 On 06/25/2014 04:15 PM, ttha...@altera.com wrote:

 From: Thor Thayer ttha...@altera.com

 This patch adds support for the CycloneV and ArriaV SDRAM controllers.
 Correction and reporting of SBEs, Panic on DBEs.

 Signed-off-by: Thor Thayer ttha...@altera.com
 ---
 v2: Use the SDRAM controller registers to calculate memory size
  instead of the Device Tree. Update To  Cc list. Add maintainer
  information.

 v3: EDAC driver cleanup based on comments from Mailing list.

 v4: Panic on DBE. Add macro around inject-error reads to prevent
  them from being optimized out. Remove of_match_ptr since this
  will always use Device Tree.

 v5: Addition of printk to trigger function to ensure read vars
  are not optimized out.

 v6: Changes to split out shared SDRAM controller reg (offset 0x00)
  as a syscon device and allocate ECC specific SDRAM registers
  to EDAC.

 v7: No change. Bump version for consistency.
 ---
   drivers/edac/Kconfig   |9 +
   drivers/edac/Makefile  |2 +
   drivers/edac/altera_edac.c |  448
 
   3 files changed, 459 insertions(+)
   create mode 100644 drivers/edac/altera_edac.c

 diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
 index 878f090..4f4d379 100644
 --- a/drivers/edac/Kconfig
 +++ b/drivers/edac/Kconfig
 @@ -368,4 +368,13 @@ config EDAC_OCTEON_PCI
   Support for error detection and correction on the
   Cavium Octeon family of SOCs.

 +config EDAC_ALTERA_MC
 +   bool Altera SDRAM Memory Controller EDAC
 +   depends on EDAC_MM_EDAC  ARCH_SOCFPGA
 +   help
 + Support for error detection and correction on the
 + Altera SDRAM memory controller. Note that the
 + preloader must initialize the SDRAM before loading
 + the kernel.
 +
   endif # EDAC
 diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
 index 4154ed6..9741336 100644
 --- a/drivers/edac/Makefile
 +++ b/drivers/edac/Makefile
 @@ -64,3 +64,5 @@ obj-$(CONFIG_EDAC_OCTEON_PC)  +=
 octeon_edac-pc.o
   obj-$(CONFIG_EDAC_OCTEON_L2C) += octeon_edac-l2c.o
   obj-$(CONFIG_EDAC_OCTEON_LMC) += octeon_edac-lmc.o
   obj-$(CONFIG_EDAC_OCTEON_PCI) += octeon_edac-pci.o
 +
 +obj-$(CONFIG_EDAC_ALTERA_MC)   += altera_edac.o
 diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
 new file mode 100644
 index 000..e3fcd27
 --- /dev/null
 +++ b/drivers/edac/altera_edac.c
 @@ -0,0 +1,448 @@
 +/*
 + *  Copyright Altera Corporation (C) 2014. All rights reserved.
 + *  Copyright 2011-2012 Calxeda, Inc.
 + *
 + * This program is free software; you can redistribute it and/or modify
 + * it under the terms and conditions of the GNU General Public License,
 + * version 2, as published by the Free Software Foundation.
 + *
 + * This program is distributed in the hope it will be useful, but WITHOUT
 + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
 + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License
 for
 + * more details.
 + *
 + * This file is subject to the terms and conditions of the GNU General
 Public
 + * License.  See the file COPYING in the main directory of this archive
 + * for more details.
 +
 + *
 + * Adapted from the highbank_mc_edac driver
 + *
 + */
 +#include linux/types.h
 +#include linux/kernel.h
 +#include linux/ctype.h
 +#include linux/edac.h
 +#include linux/interrupt.h
 +#include linux/platform_device.h
 +#include linux/of_platform.h
 +#include linux/uaccess.h
 +#include linux/mfd/syscon.h
 +#include linux/regmap.h


 Did you miss my comment to put these headers in alpabetical order?

 Dinh

Yep, I missed your comment - looking at it now.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv6 1/3] devicetree: Addition of the Altera SDRAM controller

[Thor Thayer]

On Sat, Jun 21, 2014 at 4:04 AM, Steffen Trumtrar
s.trumt...@pengutronix.de wrote:
 Hi!

 On Fri, Jun 20, 2014 at 06:22:01PM -0500, ttha...@altera.com wrote:
 From: Thor Thayer ttha...@altera.com

 Addition of the Altera SDRAM Controller bindings and device tree changes.

 v2: Changes to SoC SDRAM EDAC code.

 v3: Implement code suggestions for SDRAM EDAC code.

 v4: Remove syscon from SDRAM controller bindings.

 v5: No Change, bump version for consistency.

 v6: Only map the ctrlcfg register as syscon.

 Signed-off-by: Thor Thayer ttha...@altera.com
 ---
  .../bindings/arm/altera/socfpga-sdram.txt  |   11 +++
  arch/arm/boot/dts/socfpga.dtsi |5 +
  2 files changed, 16 insertions(+)
  create mode 100644 
 Documentation/devicetree/bindings/arm/altera/socfpga-sdram.txt

 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 000..5027026
 --- /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 0x4;
 + };
 diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
 index 4676f25..310292e 100644
 --- a/arch/arm/boot/dts/socfpga.dtsi
 +++ b/arch/arm/boot/dts/socfpga.dtsi
 @@ -682,6 +682,11 @@
   clocks = l4_sp_clk;
   };

 + sdrctl@ffc25000 {
 + compatible = altr,sdr-ctl, syscon;

 Did you forget to add the syscon to the binding documentation?

Hi Steffen,

I guess I'm confused about when to use syscon. In my last series, I
was advised not to include syscon in the bindings but to use it in the
device tree. I had been including it in the binding document until
this patch.

Thanks,

Thor
 + reg = 0xffc25000 0x4;
 + };
 +
   rst: rstmgr@ffd05000 {
   compatible = altr,rst-mgr;
   reg = 0xffd05000 0x1000;

 --
 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- |
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv6 2/3] devicetree: Addition of the Altera SDRAM EDAC

[Thor Thayer]

On Sat, Jun 21, 2014 at 4:06 AM, Steffen Trumtrar
s.trumt...@pengutronix.de wrote:
 On Fri, Jun 20, 2014 at 06:22:02PM -0500, ttha...@altera.com wrote:
 From: Thor Thayer ttha...@altera.com

 Addition of the Altera SDRAM EDAC bindings and device tree changes

 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

 Signed-off-by: Thor Thayer ttha...@altera.com
 ---
  .../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 000..540c9cf
 --- /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@0 {
 + 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..fe9832e 100644
 --- a/arch/arm/boot/dts/socfpga.dtsi
 +++ b/arch/arm/boot/dts/socfpga.dtsi
 @@ -687,6 +687,12 @@
   reg = 0xffc25000 0x4;
   };

 + sdramedac@0 {
  ^^^

 Please fix the baseaddress (also in the binding doc).


Thanks. Good catch. I will fix this.

 + compatible = altr,sdram-edac;
 + reg = 0xffc2502C 0x28;
 + interrupts = 0 39 4;
 + };
 +
   rst: rstmgr@ffd05000 {
   compatible = altr,rst-mgr;
   reg = 0xffd05000 0x1000;

 --
 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- |
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv7 2/3] devicetree: Addition of the Altera SDRAM EDAC.

[Thor Thayer]

Hi Mark

On Thu, Jun 26, 2014 at 4:45 AM, Mark Rutland mark.rutl...@arm.com wrote:
 On Wed, Jun 25, 2014 at 10:15:26PM +0100, ttha...@altera.com wrote:
 From: Thor Thayer ttha...@altera.com

 Add the Altera SDRAM EDAC bindings and device tree changes to the Altera SoC 
 project.

 Signed-off-by: Thor Thayer ttha...@altera.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 000..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@ffc2502c {
 + 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@ffc2502c {
 + 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.

Sorry, I missed your reply. Luckily Dinh pointed it out to me.

The SDRAM Controller binding is just 1 register but it includes
bitfields that describe the SDRAM configuration as well as some
bitfields for ECC. Ideally the ECC bitfields would be in a separate
register and could be included in the SDRAM EDAC block.

It is anticipated that other drivers and U-Boot may need to read the
SDRAM configuration which is why this register contains a syscon
designation.

The SDRAM EDAC block is a set of registers in the SDRAM Controller IP
register space that are ECC specific. I was thinking this should be a
separate binding since it encompasses 1 complete task but I see your
point about just having 1 binding.

I appreciate your help on figuring the proper way to handle this.
Sorry about reposting without addressing your comment.

Thanks,

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

Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings devicetree entries.

[Thor Thayer]

On 08/14/2014 01:49 PM, Pavel Machek wrote:

On Mon 2014-08-11 10:18:13, ttha...@opensource.altera.com wrote:

From: Thor Thayer ttha...@opensource.altera.com

Add the Altera SDRAM EDAC bindings and device tree changes to the Altera SoC 
project.

Signed-off-by: Thor Thayer ttha...@opensource.altera.com

Acked-by: Pavel Machek pa...@denx.de



Hi All,

Any further comments or suggestions on this patch series? Thanks for the 
review, Pavel!


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

Re: [PATCHv10 1/2] edac: altera: Add Altera SDRAM EDAC support.

[Thor Thayer]

On 08/14/2014 01:49 PM, Pavel Machek wrote:

On Mon 2014-08-11 10:18:12, ttha...@opensource.altera.com wrote:

From: Thor Thayer ttha...@opensource.altera.com

This patch adds support for the CycloneV and ArriaV SDRAM controllers.
Correction and reporting of SBEs, Panic on DBEs.

Signed-off-by: Thor Thayer ttha...@opensource.altera.com

Acked-by: Pavel Machek pa...@denx.de


Hi All,

Any further comments or suggestions on this patch series? Thanks for 
reviewing, Pavel!


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

Re: [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller

[Thor Thayer]

On 07/31/2014 03:26 AM, Lee Jones wrote:

On Wed, 30 Jul 2014, ttha...@opensource.altera.com wrote:


From: Thor Thayer ttha...@opensource.altera.com

Add a simple MFD for the Altera SDRAM Controller.

Signed-off-by: Alan Tull at...@opensource.altera.com
Signed-off-by: Thor Thayer ttha...@opensource.altera.com
---
v1-8: The MFD implementation was not included in the original series.

v9: New MFD implementation.
---
  MAINTAINERS|5 ++
  drivers/mfd/Kconfig|7 ++
  drivers/mfd/Makefile   |1 +
  drivers/mfd/altera-sdr.c   |  162 
  include/linux/mfd/altera-sdr.h |  102 +
  5 files changed, 277 insertions(+)
  create mode 100644 drivers/mfd/altera-sdr.c
  create mode 100644 include/linux/mfd/altera-sdr.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 86efa7e..48a8923 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1340,6 +1340,11 @@ M:   Dinh Nguyen dingu...@altera.com
  S:Maintained
  F:drivers/clk/socfpga/
  
+ARM/SOCFPGA SDRAM CONTROLLER SUPPORT

+M: Thor Thayer ttha...@altera.com
+S: Maintained
+F: drivers/mfd/altera-sdr.c
+
  ARM/STI ARCHITECTURE
  M:Srinivas Kandagatla srinivas.kandaga...@gmail.com
  M:Maxime Coquelin maxime.coque...@st.com

This should be in a separate patch.
OK. Thanks for your comments and for reviewing. I will move this into a 
separate patch.

diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
index 6cc4b6a..8ce4961 100644
--- a/drivers/mfd/Kconfig
+++ b/drivers/mfd/Kconfig
@@ -719,6 +719,13 @@ config MFD_STMPE
Keypad: stmpe-keypad
Touchscreen: stmpe-ts
  
+config MFD_ALTERA_SDR

+   bool Altera SDRAM Controller MFD
+   depends on ARCH_SOCFPGA
+   select MFD_CORE
+   help
+ Support for Altera SDRAM Controller (SDR) MFD.
+
  menu STMicroelectronics STMPE Interface Drivers
  depends on MFD_STMPE
  
diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile

index 8afedba..24cc2b7 100644
--- a/drivers/mfd/Makefile
+++ b/drivers/mfd/Makefile
@@ -169,3 +169,4 @@ obj-$(CONFIG_MFD_AS3711)+= as3711.o
  obj-$(CONFIG_MFD_AS3722)  += as3722.o
  obj-$(CONFIG_MFD_STW481X) += stw481x.o
  obj-$(CONFIG_MFD_IPAQ_MICRO)  += ipaq-micro.o
+obj-$(CONFIG_MFD_ALTERA_SDR)   += altera-sdr.o
diff --git a/drivers/mfd/altera-sdr.c b/drivers/mfd/altera-sdr.c
new file mode 100644
index 000..b5c6646
--- /dev/null
+++ b/drivers/mfd/altera-sdr.c
@@ -0,0 +1,162 @@
+/*
+ * SDRAM Controller (SDR) MFD
+ *
+ * Copyright (C) 2014 Altera Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see http://www.gnu.org/licenses/.

Can you use the shorter version of the licence?
Hi. This seems to be the shorter version of the license agreement and is 
fairly common in the kernel, right?

+ */

'\n' here.

OK.

+#include linux/io.h
+#include linux/kernel.h
+#include linux/mfd/altera-sdr.h
+#include linux/mfd/core.h
+#include linux/module.h
+#include linux/of.h
+#include linux/platform_device.h
+
+static const struct mfd_cell altera_sdr_devs[] = {
+#if defined(CONFIG_EDAC_ALTERA_MC)

No need to do this, as it will only be matched if the driver is
enabled.  Please remove the #iffery.

OK. Will remove.

+   {
+   .name = altr_sdram_edac,
+   .of_compatible = altr,sdram-edac,

What other devices will there be?

There will be an FPGA bridge and a power control driver that will need 
access to the SDR Controller registers.

+   },
+#endif
+};
+
+u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset)
+{
+   return readl(sdr-reg_base + reg_offset);
+}
+EXPORT_SYMBOL_GPL(altera_sdr_readl);
+
+void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
+{
+   writel(value, sdr-reg_base + reg_offset);
+}
+EXPORT_SYMBOL_GPL(altera_sdr_writel);

Why are you abstracting these?

Might be better to use Regmap even.
regmap seems unnecessarily complex for what we're doing which is why 
this method was chosen.


Future drivers will access different sets of registers in the device. 
These drivers won't share bitfields in the same register so the MFD 
seemed like the best solution. Originally we implemented this using 
syscon but that seems to be frowned upon so we changed to using a MFD.

+/* Get total memory size in bytes */
+u32 altera_sdr_mem_size(struct altera_sdr *sdr)
+{
+   u32 size;
+   u32 read_reg, row, bank, col, cs, width;

Weird

Re: [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller

[Thor Thayer]

On 08/01/2014 03:13 AM, Lee Jones wrote:

On Thu, 31 Jul 2014, Thor Thayer wrote:

On 07/31/2014 03:26 AM, Lee Jones wrote:

On Wed, 30 Jul 2014, ttha...@opensource.altera.com wrote:


From: Thor Thayer ttha...@opensource.altera.com

Add a simple MFD for the Altera SDRAM Controller.

Signed-off-by: Alan Tull at...@opensource.altera.com
Signed-off-by: Thor Thayer ttha...@opensource.altera.com
---
v1-8: The MFD implementation was not included in the original series.

v9: New MFD implementation.
---
  MAINTAINERS|5 ++
  drivers/mfd/Kconfig|7 ++
  drivers/mfd/Makefile   |1 +
  drivers/mfd/altera-sdr.c   |  162 
  include/linux/mfd/altera-sdr.h |  102 +
  5 files changed, 277 insertions(+)
  create mode 100644 drivers/mfd/altera-sdr.c
  create mode 100644 include/linux/mfd/altera-sdr.h

[...]


+++ b/drivers/mfd/altera-sdr.c
@@ -0,0 +1,162 @@
+/*
+ * SDRAM Controller (SDR) MFD
+ *
+ * Copyright (C) 2014 Altera Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see http://www.gnu.org/licenses/.

Can you use the shorter version of the licence?

Hi. This seems to be the shorter version of the license agreement
and is fairly common in the kernel, right?

This is the long(ish) version.  Many programs have the Copyright line
and the This program is free software; line.  It'll also be a good
idea to keep the link to the full licence.

[...]

Hi Lee.

Our legal department requires this header.  Their reasoning is that they 
want to retain the rights and warranty language with the file (just in 
case the COPYING file changes).



+   {
+   .name = altr_sdram_edac,
+   .of_compatible = altr,sdram-edac,

What other devices will there be?


There will be an FPGA bridge and a power control driver that will
need access to the SDR Controller registers.

Okay.  Do you know when they'll be upstreamed?

The other drivers are waiting on this file as a pre-requisite.

+u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset)
+{
+   return readl(sdr-reg_base + reg_offset);
+}
+EXPORT_SYMBOL_GPL(altera_sdr_readl);
+
+void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
+{
+   writel(value, sdr-reg_base + reg_offset);
+}
+EXPORT_SYMBOL_GPL(altera_sdr_writel);

Why are you abstracting these?

Might be better to use Regmap even.

regmap seems unnecessarily complex for what we're doing which is why
this method was chosen.

Future drivers will access different sets of registers in the
device. These drivers won't share bitfields in the same register so
the MFD seemed like the best solution. Originally we implemented
this using syscon but that seems to be frowned upon so we changed to
using a MFD.

Why was the use of syscon frowned upon?  Can you link me to the
thread?  Writing directly to the registers sounds to me a lot worse
than using infrastructure which was designed for these kinds of
accesses.

If you do choose to fiddle with the registers in this manner, is there
any reason why you're calling back into here, rather than using
readl() and writel() directly?

We'd prefer to use syscon and that is what we started with. If you'd 
like to be our advocate, I will return to that because it was pretty 
clean. My primary concern is to get it upstreamed and if it is MFD then 
I'll make the changes.


Here are the threads.
http://marc.info/?l=linux-kernelm=140128791902800w=2
and
http://article.gmane.org/gmane.linux.kernel/1679601

+u32 altera_sdr_mem_size(struct altera_sdr *sdr)
+{
+   u32 size;
+   u32 read_reg, row, bank, col, cs, width;

Weird that size is on its own.  Either place on a single line or
separate them all.

OK. I will change this.

+   read_reg = altera_sdr_readl(sdr, SDR_DRAMADDRW_OFST);
+   if (read_reg  0)
+   return 0;
+
+   width = altera_sdr_readl(sdr, SDR_DRAMIFWIDTH_OFST);
+   if (width  0)
+   return 0;

u32s cant be  0.  The 'u' means 'unsigned'.

Whoops. Good catch.

+   col = (read_reg  SDR_DRAMADDRW_COLBITS_MASK) 
+   SDR_DRAMADDRW_COLBITS_LSB;
+   row = (read_reg  SDR_DRAMADDRW_ROWBITS_MASK) 
+   SDR_DRAMADDRW_ROWBITS_LSB;
+   bank = (read_reg  SDR_DRAMADDRW_BANKBITS_MASK) 
+   SDR_DRAMADDRW_BANKBITS_LSB;
+   cs = (read_reg  SDR_DRAMADDRW_CSBITS_MASK) 
+   SDR_DRAMADDRW_CSBITS_LSB;

These would

Re: [PATCHv9 1/3] mfd: altera: Add Altera SDRAM Controller

[Thor Thayer]

On 08/02/2014 12:08 PM, Steffen Trumtrar wrote:

Hi!

On Fri, Aug 01, 2014 at 05:27:57PM -0500, Thor Thayer wrote:

On 08/01/2014 03:13 AM, Lee Jones wrote:

On Thu, 31 Jul 2014, Thor Thayer wrote:

On 07/31/2014 03:26 AM, Lee Jones wrote:

On Wed, 30 Jul 2014, ttha...@opensource.altera.com wrote:

[...]


+u32 altera_sdr_readl(struct altera_sdr *sdr, u32 reg_offset)
+{
+   return readl(sdr-reg_base + reg_offset);
+}
+EXPORT_SYMBOL_GPL(altera_sdr_readl);
+
+void altera_sdr_writel(struct altera_sdr *sdr, u32 reg_offset, u32 value)
+{
+   writel(value, sdr-reg_base + reg_offset);
+}
+EXPORT_SYMBOL_GPL(altera_sdr_writel);

We'd prefer to use syscon and that is what we started with. If you'd
like to be our advocate, I will return to that because it was pretty
clean. My primary concern is to get it upstreamed and if it is MFD
then I'll make the changes.

Here are the threads.
http://marc.info/?l=linux-kernelm=140128791902800w=2

The conclusion of this thread was syscon for offset 0x0, no ?!
And if you decide to have new writel/readl functions, I'd prefer if you don't
change the order of parameters just because. That always weirds me out, when
there are vendorname_writel functions, that only change the API of writel and
nothing else (not exactly the case here).

Hi Steffen,

Yes, I see your point on the order of parameters. i will change it.

As for the syscon only at offset 0, I submitted that in version 7  8. 
It wasn't as clean as the version using syscon for the entire range.  It 
could also require changes to the SDRAM controller binding as things are 
added in the future.  My understanding is the bindings should not change 
significantly and changes are frowned upon.


After going through this exercise in a couple of different ways, I'm 
leaning toward using syscon for the entire SDRAM controller register range.


Is there a good reason not to use syscon on the entire register range?

Thanks,

Thor


Regards,
Steffen



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv10 2/2] arm: dts: Add Altera SDRAM EDAC bindings devicetree entries.

[Thor Thayer]

On 08/15/2014 11:07 AM, atull wrote:

On Fri, 15 Aug 2014, Steffen Trumtrar wrote:


Hi!

Hello

Thanks for the feedback...


ttha...@opensource.altera.com writes:


From: Thor Thayer ttha...@opensource.altera.com

Add the Altera SDRAM EDAC bindings and device tree changes to the Altera SoC 
project.

Signed-off-by: Thor Thayer ttha...@opensource.altera.com
---
v2: Changes to SoC SDRAM EDAC code.

v3: Implement code suggestions for SDRAM EDAC code.

v4: Remove syscon from SDRAM controller bindings.

v5: No Change, bump version for consistency.

v6: Only map the ctrlcfg register as syscon.

v7: No change. Bump for consistency.

v8: No change. Bump for consistency.

v9: Changes to support a MFD SDRAM controller with nested EDAC.

v10: Revert to using syscon based on feedback.
---
  .../bindings/arm/altera/socfpga-sdram-edac.txt |   15 +++
  arch/arm/boot/dts/socfpga.dtsi |   11 +++
  2 files changed, 26 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 000..d0ce01d
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdram-edac.txt
@@ -0,0 +1,15 @@
+Altera SOCFPGA SDRAM Error Detection  Correction [EDAC]
+The EDAC accesses a range of registers in the SDRAM controller.
+
+Required properties:
+- compatible : should contain altr,sdram-edac;
+- altr,sdr-syscon : phandle of the sdr module
+- interrupts : Should contain the SDRAM ECC IRQ in the
+   appropriate format for the IRQ controller.
+
+Example:
+   sdramedac {
+   compatible = altr,sdram-edac;
+   altr,sdr-syscon = sdr;
+   interrupts = 0 39 4;
+   };
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 4676f25..45b361e 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -603,6 +603,17 @@
};
};
  
+		sdr: sdr@ffc25000 {

+   compatible = syscon;
+   reg = 0xffc25000 0x1000;
+   };
+
+   sdramedac {
+   compatible = altr,sdram-edac;
+   altr,sdr-syscon = sdr;
+   interrupts = 0 39 4;
+   };
+
L2: l2-cache@fffef000 {
compatible = arm,pl310-cache;
reg = 0xfffef000 0x1000;

Sorry, but I personally still don't like this approach.

This is a normal use of syscon.  Syscon is great for this case
where multiple drivers need access to an ip block's register
range but there is otherwise no need to write a MFD from scratch
to allow that.  I think that's the purpose of syscon in the first
place.


If we do it like this however, shouldn't the different regions of the
SDR (EDAC, FPGAPORTRST, ...) at least be child nodes of the syscon?
That seems to match the syscon binding and describes the actual hardware
better IMHO.

I like that; it would be clean and clear, but I don't think syscon is
written such that that would actually work.  I haven't seen anybody else
do it that way.

Hi Steffen!

Building on Alan's points, I don't see the reference to the child nodes 
in the syscon.txt binding documentation - can you point out what I'm 
missing?


I based this patch on other examples of syscon, and there aren't child 
nodes (exynos5250.dtsi, omap3.dtsi, etc). According to my understanding 
of the device tree parsing, the parent needs to register child nodes. 
Since syscon is the parent and doesn't register the child nodes, they 
won't be automatically probed. The other syscon implementations are at 
the same level as the syscon and use a phandle to the syscon (for 
instance the omap3.dtsi).


Thanks for reviewing!

Thor

Oh, and reg = ... for the sdram-edac, maybe ?

How would that work?  Syscon is already mapping the whole register range
for the sdr block.  Are you proposing a device tree binding that this
Linux driver would ignore, but some other driver in some other OS might
find useful in the future?  I'd rather not put duplicate information
in two places, too easy for it to get out of sync.


How would I know where the start address of the EDAC is, if I would use
this DT on another OS where I don't have the same driver?

The start address of the EDAC is an offset into the range mapped by
syscon here.  The driver knows what the register offsets are.

If we are talking about this device tree being used by some other OS
that is not aware of syscon, then that OS won't know what's going on and
some modification of the DT will be needed.  I have been advised that
u-boot will need a significantly different DT, though I don't know
what the issues are there.


Regards,
Steffen

--
Pengutronix e.K.   | Steffen

Re: [PATCHv9 3/3] arm: dts: Add Altera SDRAM controller bindings

[Thor Thayer]

On 08/17/2014 07:50 PM, Rob Herring wrote:

On 07/30/2014 01:22 PM, ttha...@opensource.altera.com wrote:

From: Thor Thayer ttha...@opensource.altera.com

Add the Altera SDRAM controller bindings and device tree changes to the Altera 
SoC project.

Signed-off-by: Thor Thayer ttha...@opensource.altera.com
---
v2: Changes to SoC SDRAM EDAC code.

v3: Implement code suggestions for SDRAM EDAC code.

v4: Remove syscon from SDRAM controller bindings.

v5: No Change, bump version for consistency.

v6: Only map the ctrlcfg register as syscon.

v7: No change. Bump for consistency.

v8: No change. Bump for consistency.

v9: Changes to support a MFD SDRAM controller with nested EDAC.
---
  .../devicetree/bindings/arm/altera/socfpga-sdr.txt |   13 +
  arch/arm/boot/dts/socfpga.dtsi |   10 ++
  2 files changed, 23 insertions(+)
  create mode 100644 
Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt

diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt 
b/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
new file mode 100644
index 000..2bb1ddf
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-sdr.txt
@@ -0,0 +1,13 @@
+Altera SOCFPGA SDRAM Controller
+The SDRAM controller is implemented as a MFD so various drivers may
+nest under this main SDRAM controller binding.
+
+Required properties:
+- compatible : altr,sdr;
+- reg : Should contain 1 register range(address and length)
+
+Example:
+   sdr@0xffc25000 {
+   compatible = altr,sdr;
+   reg = 0xffc25000 0x1000;
+   };
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 4676f25..ecb306d 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -603,6 +603,16 @@
};
};
  
+		sdr@0xffc25000 {

+   compatible = altr,sdr;
+   reg = 0xffc25000 0x1000;
+
+   sdramedac@0 {
+   compatible = altr,sdram-edac;
+   interrupts = 0 39 4;
+   };

This doesn't match the documentation, but I don't think this is a move
in the right direction anyway. Because Linux has/wants an MFD driver is
not a reason to add a sub node. It is a single h/w block and DT should
reflect that.

Rob

Hi Rob,

Thanks for reviewing. After discussions with the community and 
internally, I reverted to using the syscon case in revision 10. I 
apologize for the confusion but the syscon method seems to be a cleaner 
solution. I submitted the sycon version on 8/11/14.


Thanks,

Thor

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv2 0/4] Add Altera peripheral memories to EDAC framework

[Thor Thayer]

Hi Borislav,

On 10/17/2014 03:33 PM, ttha...@opensource.altera.com wrote:

From: Thor Thayer ttha...@opensource.altera.com

This patch adds the L2 cache and OCRAM peripherals to the EDAC framework
using the EDAC device framework. The ECC is enabled early in the boot
process in the platform specific code.

Thor Thayer (4):
   arm: socfpga: Enable L2 Cache ECC on startup.
   arm: socfpga: Enable OCRAM ECC on startup.
   edac: altera: Add Altera L2 Cache and OCRAM EDAC Support
   arm: dts: Add Altera L2 Cache and OCRAM EDAC

  .../bindings/arm/altera/socfpga-l2-edac.txt|   15 ++
  .../bindings/arm/altera/socfpga-ocram-edac.txt |   16 ++
  MAINTAINERS|4 +-
  arch/arm/boot/dts/socfpga.dtsi |   15 +-
  arch/arm/mach-socfpga/Makefile |2 +
  arch/arm/mach-socfpga/l2_cache.c   |   44 
  arch/arm/mach-socfpga/l2_cache.h   |   28 +++
  arch/arm/mach-socfpga/ocram.c  |   90 +++
  arch/arm/mach-socfpga/ocram.h  |   28 +++
  arch/arm/mach-socfpga/socfpga.c|   13 +-
  drivers/edac/Kconfig   |   14 ++
  drivers/edac/Makefile  |3 +
  drivers/edac/altera_edac_mgr.c |  261 
  drivers/edac/altera_edac_mgr.h |   56 +
  drivers/edac/altera_l2_edac.c  |  130 ++
  drivers/edac/altera_ocram_edac.c   |  107 
  16 files changed, 823 insertions(+), 3 deletions(-)
  create mode 100644 
Documentation/devicetree/bindings/arm/altera/socfpga-l2-edac.txt
  create mode 100644 
Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt
  create mode 100644 arch/arm/mach-socfpga/l2_cache.c
  create mode 100644 arch/arm/mach-socfpga/l2_cache.h
  create mode 100644 arch/arm/mach-socfpga/ocram.c
  create mode 100644 arch/arm/mach-socfpga/ocram.h
  create mode 100644 drivers/edac/altera_edac_mgr.c
  create mode 100644 drivers/edac/altera_edac_mgr.h
  create mode 100644 drivers/edac/altera_l2_edac.c
  create mode 100644 drivers/edac/altera_ocram_edac.c


Do you have any comments about this driver?

Thanks,

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

Re: [PATCHv2 0/4] Add Altera peripheral memories to EDAC framework

[Thor Thayer]

On 10/27/2014 03:43 PM, Borislav Petkov wrote:

On Mon, Oct 27, 2014 at 01:50:24PM -0500, Thor Thayer wrote:

Do you have any comments about this driver?

Just a question: why do you have three .c files for something which
does only error injection and nothing else AFAICT? Why isn't this part
of altera_edac.c?

There are 2 files for doing the error injection (altera_l2_edac.c and 
altera_ocram_edac.c) and then 1 file for the irq handling and probe 
(altera_edac_mgr.c).


The L2 cache and the On-Chip RAM drivers were based on the Calxeda L2 
cache driver and when written as 2 separate files, the resulting code 
was very similar from the probe and error handling standpoint so the 
common code was combined (altera_edac_mgr.c).


The Memory Controller model was used for the SDRAM EDAC (altera_edac.c) 
since it matches the DIMM model.  The MC model didn't seem to fit the 
discrete memories like OCRAM and L2 cache (these files) so I used the 
EDAC device model which agreed with the Calxeda L2 cache driver.


Should I move the EDAC Device probe and error handling from 
altera_edac_mgr.c to altera_edac.c? Can I mix the MC and Device models 
in the same file?


Thanks for reviewing and for commenting.

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

Re: [PATCHv2 0/4] Add Altera peripheral memories to EDAC framework

[Thor Thayer]

On 10/27/2014 04:59 PM, Borislav Petkov wrote:

On Mon, Oct 27, 2014 at 04:35:00PM -0500, Thor Thayer wrote:

Should I move the EDAC Device probe and error handling from
altera_edac_mgr.c to altera_edac.c? Can I mix the MC and Device models
in the same file?

Right, for basic practical reasons, I'd like to keep all functionality
pertaining to one hw flavour in one compilation unit/one driver. We can
always split them later if a compelling reason emerges.

Thanks.


OK. I will make the changes.

Would the L2 cache and OCRAM specific functions also be in 
altera_edac.c? Each of these EDAC pieces is independent and can be 
compiled in without the others. I've read that the use of #ifdef's is 
discouraged and having separate files in the Makefile solves that.


Thanks for the clarification.

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

[RESEND PATCHv5 5/5] arm: dts: Add Altera L2 Cache and OCRAM EDAC

[Thor Thayer]

Hi all,

On 11/11/2014 06:14 PM, ttha...@opensource.altera.com wrote:

From: Thor Thayerttha...@opensource.altera.com

Adding the device tree entries and bindings needed to support
the Altera L2 cache and On-Chip RAM EDAC. This patch relies upon
an earlier patch to declare and setup On-chip RAM properly.
http://www.spinics.net/lists/devicetree/msg51117.html


Any comments on these devicetree additions?

Thanks,
Thor

Signed-off-by: Thor Thayerttha...@opensource.altera.com
---
v2: Remove OCRAM declaration and reference prior patch.

v3-5: No Change
---
  .../bindings/arm/altera/socfpga-l2-edac.txt|   15 +++
  .../bindings/arm/altera/socfpga-ocram-edac.txt |   16 
  arch/arm/boot/dts/socfpga.dtsi |   15 ++-
  3 files changed, 45 insertions(+), 1 deletion(-)
  create mode 100644 
Documentation/devicetree/bindings/arm/altera/socfpga-l2-edac.txt
  create mode 100644 
Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt

diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-l2-edac.txt 
b/Documentation/devicetree/bindings/arm/altera/socfpga-l2-edac.txt
new file mode 100644
index 000..35b19e3
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-l2-edac.txt
@@ -0,0 +1,15 @@
+Altera SoCFPGA L2 cache Error Detection and Correction [EDAC]
+
+Required Properties:
+- compatible : Should be altr,l2-edac
+- reg : Address and size for ECC error interrupt clear registers.
+- interrupts : Should be single bit error interrupt, then double bit error
+   interrupt. Note the rising edge type.
+
+Example:
+
+   l2edac@ffd08140 {
+   compatible = altr,l2-edac;
+   reg = 0xffd08140 0x4;
+   interrupts = 0 36 1, 0 37 1;
+   };
diff --git 
a/Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt 
b/Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt
new file mode 100644
index 000..31ab205
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt
@@ -0,0 +1,16 @@
+Altera SoCFPGA On-Chip RAM Error Detection and Correction [EDAC]
+
+OCRAM ECC Required Properties:
+- compatible : Should be altr,ocram-edac
+- reg : Address and size for ECC error interrupt clear registers.
+- iram : phandle to On-Chip RAM definition.
+- interrupts : Should be single bit error interrupt, then double bit error
+   interrupt. Note the rising edge type.
+
+Example:
+   ocramedac@ffd08144 {
+   compatible = altr,ocram-edac;
+   reg = 0xffd08144 0x4;
+   iram = ocram;
+   interrupts = 0 178 1, 0 179 1;
+   };
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 6af96ed..32c63a3 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -618,8 +618,21 @@
interrupts = 0 39 4;
};
  
+		l2edac@ffd08140 {

+   compatible = altr,l2-edac;
+   reg = 0xffd08140 0x4;
+   interrupts = 0 36 1, 0 37 1;
+   };
+
+   ocramedac@ffd08144 {
+   compatible = altr,ocram-edac;
+   reg = 0xffd08144 0x4;
+   iram = ocram;
+   interrupts = 0 178 1, 0 179 1;
+   };
+
L2: l2-cache@fffef000 {
-   compatible = arm,pl310-cache;
+   compatible = arm,pl310-cache, syscon;
reg = 0xfffef000 0x1000;
interrupts = 0 38 0x04;
cache-unified;


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] spi: spidev: Don't mangle max_speed_hz in underlying spi device

[Thor Thayer]

Hi Mark,

On 11/08/2014 04:29 AM, Mark Brown wrote:

Currently spidev allows callers to set the default speed by overriding the
max_speed_hz in the underlying device. This achieves the immediate goal but
is not what devices expect and can easily lead to userspace trying to set
unsupported speeds and succeeding, apart from anything else drivers can't
set a limit on the speed using max_speed_hz as they'd expect and any other
devices on the bus will be affected.

Instead store the default speed in the spidev struct and fill this in on
each transfer.

Signed-off-by: Mark Brownbroo...@kernel.org
---

I've not tested this at all yet.

  drivers/spi/spidev.c | 12 ++--
  1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spidev.c b/drivers/spi/spidev.c
index e50039f..42239fd 100644
--- a/drivers/spi/spidev.c
+++ b/drivers/spi/spidev.c
@@ -87,6 +87,7 @@ struct spidev_data {
unsignedusers;
u8  *tx_buffer;
u8  *rx_buffer;
+   u32 speed_hz;
  };
  
  static LIST_HEAD(device_list);

@@ -274,6 +275,8 @@ static int spidev_message(struct spidev_data *spidev,
k_tmp-bits_per_word = u_tmp-bits_per_word;
k_tmp-delay_usecs = u_tmp-delay_usecs;
k_tmp-speed_hz = u_tmp-speed_hz;
+   if (!k_tmp-speed_hz)
+   k_tmp-speed_hz = spidev-speed_hz;
  #ifdef VERBOSE
dev_dbg(spidev-spi-dev,
  xfer len %zd %s%s%s%dbits %u usec %uHz\n,
@@ -377,7 +380,7 @@ spidev_ioctl(struct file *filp, unsigned int cmd, unsigned 
long arg)
retval = __put_user(spi-bits_per_word, (__u8 __user *)arg);
break;
case SPI_IOC_RD_MAX_SPEED_HZ:
-   retval = __put_user(spi-max_speed_hz, (__u32 __user *)arg);
+   retval = __put_user(spidev-speed_hz, (__u32 __user *)arg);
break;
  
  	/* write requests */

@@ -442,9 +445,10 @@ spidev_ioctl(struct file *filp, unsigned int cmd, unsigned 
long arg)
spi-max_speed_hz = tmp;
retval = spi_setup(spi);
if (retval  0)
-   spi-max_speed_hz = save;
+   spidev-speed_hz = tmp;
else
dev_dbg(spi-dev, %d Hz (max)\n, tmp);
+   spi-max_speed_hz = save;


I think the test should be if (retval = 0) otherwise the value is only 
updated on an error. With this change, I was able to successfully change 
the SPI speed without affecting the max speed. This was tested using 
spidev_test.c on a Altera Cyclone V which includes the DesignWare SPI IP 
(spi-dw.c).



}
break;
  
@@ -570,6 +574,8 @@ static int spidev_release(struct inode *inode, struct file *filp)

kfree(spidev-rx_buffer);
spidev-rx_buffer = NULL;
  
+		spidev-speed_hz = spidev-spi-max_speed_hz;

+
/* ... after we unbound from the underlying device? */
spin_lock_irq(spidev-spi_lock);
dofree = (spidev-spi == NULL);
@@ -650,6 +656,8 @@ static int spidev_probe(struct spi_device *spi)
}
mutex_unlock(device_list_lock);
  
+	spidev-speed_hz = spi-max_speed_hz;

+
if (status == 0)
spi_set_drvdata(spi, spidev);
else


The echo command calls spidev_write() directly. Although the speed can't 
be specified in the echo command, it did prompt me to look at that path.


In that case I think we'd need to add the .speed_hz element in 
spidev_sync_write() and spidev_sync_read().

struct spi_transfert = {
.rx_buf= spidev-rx_buffer,
.len= len,
.speed_hz= spidev-speed_hz,
};
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv4 1/5] arm: socfpga: Enable L2 Cache ECC on startup.

[Thor Thayer]

Hi Dinh,

On 11/07/2014 02:13 PM, Dinh Nguyen wrote:

Hi Thor,

On 11/07/2014 10:54 AM, ttha...@opensource.altera.com wrote:

From: Thor Thayer ttha...@opensource.altera.com

This patch enables the ECC for L2 cache on machine
startup.  The ECC has to be enabled before data is
is stored in memory otherwise the ECC will fail on
reads.

Signed-off-by: Thor Thayer ttha...@opensource.altera.com
---
v2: Split OCRAM initialization into separate patch.

v3/4: No change
---
  MAINTAINERS  |1 +
  arch/arm/mach-socfpga/Makefile   |1 +
  arch/arm/mach-socfpga/l2_cache.c |   44 ++
  arch/arm/mach-socfpga/l2_cache.h |   28 
  arch/arm/mach-socfpga/socfpga.c  |5 -
  5 files changed, 78 insertions(+), 1 deletion(-)
  create mode 100644 arch/arm/mach-socfpga/l2_cache.c
  create mode 100644 arch/arm/mach-socfpga/l2_cache.h

diff --git a/MAINTAINERS b/MAINTAINERS
index ee1bc5b..d0c7752 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1407,6 +1407,7 @@ ARM/SOCFPGA EDAC SUPPORT
  M:Thor Thayer ttha...@opensource.altera.com
  S:Maintained
  F:drivers/edac/altera_edac.
+F: arch/arm/mach-socfpga/l2_cache.*
  
  ARM/STI ARCHITECTURE

  M:Srinivas Kandagatla srinivas.kandaga...@gmail.com
diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-socfpga/Makefile
index 6dd7a93..142609e 100644
--- a/arch/arm/mach-socfpga/Makefile
+++ b/arch/arm/mach-socfpga/Makefile
@@ -4,3 +4,4 @@
  
  obj-y	:= socfpga.o

  obj-$(CONFIG_SMP) += headsmp.o platsmp.o
+obj-$(CONFIG_EDAC_ALTERA_L2C) += l2_cache.o
diff --git a/arch/arm/mach-socfpga/l2_cache.c b/arch/arm/mach-socfpga/l2_cache.c
new file mode 100644
index 000..8e109f3
--- /dev/null
+++ b/arch/arm/mach-socfpga/l2_cache.c
@@ -0,0 +1,44 @@
+/*
+ * Copyright Altera Corporation (C) 2014. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see http://www.gnu.org/licenses/.
+ */
+#include linux/clk-provider.h

Why clk-provider.h and not io.h?

+#include linux/of_platform.h
+#include linux/of_address.h
+
+#include l2_cache.h
+
+void socfpga_init_l2_ecc(void)
+{
+   struct device_node *np;
+   void __iomem  *mapped_l2_edac_addr;
+
+   np = of_find_compatible_node(NULL, NULL, altr,l2-edac);
+   if (!np) {
+   pr_err(SOCFPGA: Unable to find altr,l2-edac in dtb\n);
+   return;
+   }
+
+   mapped_l2_edac_addr = of_iomap(np, 0);
+   if (!mapped_l2_edac_addr) {
+   pr_err(SOCFPGA: Unable to find L2 ECC mapping in dtb\n);
+   return;
+   }
+
+   /* Enable ECC */
+   writel(0x01, mapped_l2_edac_addr);
+
+   pr_debug(SOCFPGA: Success Initializing L2 cache ECC\n);
+}
+

Extra line here...


diff --git a/arch/arm/mach-socfpga/l2_cache.h b/arch/arm/mach-socfpga/l2_cache.h
new file mode 100644
index 000..58e140d
--- /dev/null
+++ b/arch/arm/mach-socfpga/l2_cache.h
@@ -0,0 +1,28 @@
+/*
+ * Copyright Altera Corporation (C) 2014. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see http://www.gnu.org/licenses/.
+ */
+
+#ifndef MACH_SOCFPGA_L2_CACHE_H
+#define MACH_SOCFPGA_L2_CACHE_H
+
+#ifdef CONFIG_EDAC_ALTERA_L2C
+void socfpga_init_l2_ecc(void);
+#else
+inline void socfpga_init_l2_ecc(void)
+{
+}
+#endif
+
+#endif /* #ifndef MACH_SOCFPGA_L2_CACHE_H */
diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c
index adbf383..af6413a 100644
--- a/arch/arm/mach-socfpga/socfpga.c
+++ b/arch/arm/mach-socfpga/socfpga.c
@@ -1,5 +1,5 @@
  /*
- *  Copyright (C) 2012 Altera Corporation
+ *  Copyright (C) 2012;2014 Altera Corporation

Should be 2012-2014.


   *
   * This program is free software; you can redistribute it and/or modify
   * it under the terms of the GNU General Public License as published by
@@ -25,6 +25,8 @@
  #include asm/mach/map.h
  
  #include core.h

+#include l2_cache.h
+#include ocram.h

Re: [PATCHv4 2/5] arm: socfpga: Enable OCRAM ECC on startup.

[Thor Thayer]

Hi Dinh,

On 11/07/2014 02:32 PM, Dinh Nguyen wrote:

Hi Thor,

On 11/07/2014 10:54 AM, ttha...@opensource.altera.com wrote:

From: Thor Thayer ttha...@opensource.altera.com

This patch enables the ECC for On-Chip RAM on machine
startup.  The ECC has to be enabled before data is
is stored in memory otherwise the ECC will fail on
reads.

Signed-off-by: Thor Thayer ttha...@opensource.altera.com
---
v2: Split OCRAM ECC portion separately. Addition of iounmap()
and reorganization of gen_pool_free. Remove defconfig from patch.

v3/4: No change
---
  MAINTAINERS |1 +
  arch/arm/mach-socfpga/Makefile  |1 +
  arch/arm/mach-socfpga/ocram.c   |   90 +++
  arch/arm/mach-socfpga/ocram.h   |   28 
  arch/arm/mach-socfpga/socfpga.c |8 
  5 files changed, 128 insertions(+)
  create mode 100644 arch/arm/mach-socfpga/ocram.c
  create mode 100644 arch/arm/mach-socfpga/ocram.h

diff --git a/MAINTAINERS b/MAINTAINERS
index d0c7752..c6d390e 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -1408,6 +1408,7 @@ M:Thor Thayer ttha...@opensource.altera.com
  S:Maintained
  F:drivers/edac/altera_edac.
  F:arch/arm/mach-socfpga/l2_cache.*
+F: arch/arm/mach-socfpga/ocram.*

I think you can remove these entries for l2_cache.c and ocram.c since it
already falls under mach-socfpga/, which I already maintain.


Thanks. I will remove them.
  
  ARM/STI ARCHITECTURE

  M:Srinivas Kandagatla srinivas.kandaga...@gmail.com
diff --git a/arch/arm/mach-socfpga/Makefile b/arch/arm/mach-socfpga/Makefile
index 142609e..1552ca5 100644
--- a/arch/arm/mach-socfpga/Makefile
+++ b/arch/arm/mach-socfpga/Makefile
@@ -5,3 +5,4 @@
  obj-y := socfpga.o
  obj-$(CONFIG_SMP) += headsmp.o platsmp.o
  obj-$(CONFIG_EDAC_ALTERA_L2C) += l2_cache.o
+obj-$(CONFIG_EDAC_ALTERA_OCRAM) += ocram.o
diff --git a/arch/arm/mach-socfpga/ocram.c b/arch/arm/mach-socfpga/ocram.c
new file mode 100644
index 000..9136009
--- /dev/null
+++ b/arch/arm/mach-socfpga/ocram.c
@@ -0,0 +1,90 @@
+/*
+ * Copyright Altera Corporation (C) 2014. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see http://www.gnu.org/licenses/.
+ */
+#include linux/clk-provider.h

Why clk-provider.h and not io.h?


I will change to io.h.

+#include linux/genalloc.h
+#include linux/of_platform.h
+
+#include ocram.h
+
+void socfpga_init_ocram_ecc(void)
+{
+   struct device_node *np;
+   const __be32 *prop;
+   u32 ocr_edac_addr, iram_addr, len;
+   void __iomem  *mapped_ocr_edac_addr;
+   size_t size;
+   struct gen_pool *gp;
+
+   np = of_find_compatible_node(NULL, NULL, altr,ocram-edac);
+   if (!np) {
+   pr_err(SOCFPGA: Unable to find altr,ocram-edac in dtb\n);
+   return;
+   }
+
+   prop = of_get_property(np, reg, size);
+   ocr_edac_addr = be32_to_cpup(prop++);
+   len = be32_to_cpup(prop);

You should be checking for prop before calling be32_to_cpup.


+   if (!prop || size  sizeof(*prop)) {

Then you don't need check for prop here.


+   pr_err(SOCFPGA: Unable to find OCRAM ECC mapping in dtb\n);
+   return;
+   }
+
+   gp = of_get_named_gen_pool(np, iram, 0);
+   if (!gp) {
+   pr_err(SOCFPGA: OCRAM cannot find gen pool\n);
+   return;
+   }
+
+   np = of_find_compatible_node(NULL, NULL, mmio-sram);
+   if (!np) {
+   pr_err(SOCFPGA: Unable to find mmio-sram in dtb\n);
+   return;
+   }
+   /* Determine the OCRAM address and size */
+   prop = of_get_property(np, reg, size);
+   iram_addr = be32_to_cpup(prop++);
+   len = be32_to_cpup(prop);

Same comment here for checking prop before calling be32_to_cpu.


Good catch. Thanks.

+
+   if (!prop || size  sizeof(*prop)) {
+   pr_err(SOCFPGA: Unable to find OCRAM mapping in dtb\n);
+   return;
+   }
+
+   iram_addr = gen_pool_alloc(gp, len);
+   if (iram_addr == 0) {
+   pr_err(SOCFPGA: cannot alloc from gen pool\n);
+   return;
+   }
+
+   memset((void *)iram_addr, 0, len);
+
+   gen_pool_free(gp, iram_addr, len);
+
+   mapped_ocr_edac_addr = ioremap(ocr_edac_addr, 4);
+   if (!mapped_ocr_edac_addr) {
+   pr_err(SOCFPGA: Unable to map OCRAM ecc regs.\n);
+   return

Re: [RESEND PATCHv5 5/5] arm: dts: Add Altera L2 Cache and OCRAM EDAC

[Thor Thayer]

On 12/02/2014 09:01 AM, Mark Rutland wrote:

Hi Thor,

On Mon, Dec 01, 2014 at 08:47:41PM +, Thor Thayer wrote:

Hi Boris,

On 11/18/2014 02:56 PM, Thor Thayer wrote:

Hi all,

On 11/11/2014 06:14 PM, ttha...@opensource.altera.com wrote:

From: Thor Thayerttha...@opensource.altera.com

Adding the device tree entries and bindings needed to support
the Altera L2 cache and On-Chip RAM EDAC. This patch relies upon
an earlier patch to declare and setup On-chip RAM properly.
http://www.spinics.net/lists/devicetree/msg51117.html


Any comments on these devicetree additions?

Thanks,
Thor

Signed-off-by: Thor Thayerttha...@opensource.altera.com
---
v2: Remove OCRAM declaration and reference prior patch.

v3-5: No Change
---


I originally submitted this series on November 11, 2014.

I haven't received any comments or ACKs on the device tree patch portion
(patch 5 of 5). According to
Documentation/devicetree/bindings/submitting-patches.txt, if the patch
hasn't been Acked after a couple of weeks, the maintainer can pull the
changes if they are comfortable with those changes.


Apologies it has taken so long for this to be looked at. Unfortunately
my inbox is overflowing (as are all of ours) and things fall by the
wayside. I am sorry that this has lead to such a delayed response,
especially given that it was negative.

I'll try to keep up with this a bit more actively.

Thanks,
Mark.



Hi Mark,

Thank you for reviewing this. I have seen how busy the device tree ML is 
so I completely understand.


You have reviewed previous patch revisions in this series - thanks. I 
thought that I'd addressed most of the issues (in some cases by 
explaining my implementation) and since I hadn't heard anything, I 
assumed there weren't concerns.


Thanks again,

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

Re: [PATCHv5 2/5] arm: socfpga: Enable OCRAM ECC on startup.

[Thor Thayer]

On 12/02/2014 09:11 AM, Mark Rutland wrote:

On Wed, Nov 12, 2014 at 12:14:20AM +, ttha...@opensource.altera.com wrote:

From: Thor Thayer ttha...@opensource.altera.com

This patch enables the ECC for On-Chip RAM on machine
startup.  The ECC has to be enabled before data is
is stored in memory otherwise the ECC will fail on
reads.

Signed-off-by: Thor Thayer ttha...@opensource.altera.com
---
v2: Split OCRAM ECC portion separately. Addition of iounmap()
and reorganization of gen_pool_free. Remove defconfig from patch.

v3/4: No change

v5: Remove ocram.h, use io.h instead of clk-provider.h
 Check prop in correct place. Add ECC EN defines.
---

snip


+
+void socfpga_init_ocram_ecc(void)
+{
+   struct device_node *np;
+   const __be32 *prop;


Please don't use accessors which return raw __be32s in drivers,
typically it's the wrong thing to do and leaves horrible bugs and/or
quirks that are painful to fix up.


OK. Thanks.


+   u32 ocr_edac_addr, iram_addr, len;
+   void __iomem  *mapped_ocr_edac_addr;
+   size_t size;
+   struct gen_pool *gp;
+
+   np = of_find_compatible_node(NULL, NULL, altr,ocram-edac);
+   if (!np) {
+   pr_err(SOCFPGA: Unable to find altr,ocram-edac in dtb\n);
+   return;
+   }
+
+   prop = of_get_property(np, reg, size);
+   if (!prop || size  sizeof(*prop)) {
+   pr_err(SOCFPGA: Unable to find OCRAM ECC mapping in dtb\n);
+   return;
+   }
+   ocr_edac_addr = be32_to_cpup(prop++);
+   len = be32_to_cpup(prop);


Use of_iomap(np, 0). You don't seem to pass the address around, so that
should be sufficient.


+
+   gp = of_get_named_gen_pool(np, iram, 0);
+   if (!gp) {
+   pr_err(SOCFPGA: OCRAM cannot find gen pool\n);
+   return;
+   }
+
+   np = of_find_compatible_node(NULL, NULL, mmio-sram);
+   if (!np) {
+   pr_err(SOCFPGA: Unable to find mmio-sram in dtb\n);
+   return;
+   }
+
+   /* Determine the OCRAM address and size */
+   prop = of_get_property(np, reg, size);
+   if (!prop || size  sizeof(*prop)) {
+   pr_err(SOCFPGA: Unable to find OCRAM mapping in dtb\n);
+   return;
+   }
+   iram_addr = be32_to_cpup(prop++);
+   len = be32_to_cpup(prop);


This address is overwritten below. What's going on?



Thanks, leftovers from debugging. I will look for a different way of 
getting the information without the be32 stuff.



+
+   iram_addr = gen_pool_alloc(gp, len);
+   if (iram_addr == 0) {
+   pr_err(SOCFPGA: cannot alloc from gen pool\n);
+   return;
+   }
+
+   memset((void *)iram_addr, 0, len);


How is the iram mapped? Is memset to it safe (e.g. it unaligned accesses
are made)?



I will need to look at this further - good points. The entire iram 
should be used as a pool so I didn't expect anything unusual even though 
I just realized allocations are not contiguous.



+
+   gen_pool_free(gp, iram_addr, len);
+
+   mapped_ocr_edac_addr = ioremap(ocr_edac_addr, 4);
+   if (!mapped_ocr_edac_addr) {
+   pr_err(SOCFPGA: Unable to map OCRAM ecc regs.\n);
+   return;
+   }
+
+   /* Clear any pending OCRAM ECC interrupts, then enable ECC */
+   writel(ALTR_OCRAM_CLEAR_ECC, mapped_ocr_edac_addr);
+   writel(ALTR_OCRAM_ECC_EN, mapped_ocr_edac_addr);
+
+   iounmap(mapped_ocr_edac_addr);
+
+   pr_debug(SOCFPGA: Success Initializing OCRAM\n);
+}
diff --git a/arch/arm/mach-socfpga/socfpga.c b/arch/arm/mach-socfpga/socfpga.c
index 0954011..065d80d 100644
--- a/arch/arm/mach-socfpga/socfpga.c
+++ b/arch/arm/mach-socfpga/socfpga.c
@@ -100,6 +100,14 @@ static void socfpga_cyclone5_restart(enum reboot_mode 
mode, const char *cmd)
writel(temp, rst_manager_base_addr + SOCFPGA_RSTMGR_CTRL);
  }

+static void __init socfpga_cyclone5_init(void)
+{
+   of_platform_populate(NULL, of_default_bus_match_table,
+NULL, NULL);
+   if (IS_ENABLED(CONFIG_EDAC_ALTERA_OCRAM))
+   socfpga_init_ocram_ecc();


If it's safe to do this after probing everything else, why can't this be
a normal device probe?

Thanks,
Mark.


Good point. Thank you.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv5 4/5] edac: altera: Add Altera L2 Cache and OCRAM EDAC Support

[Thor Thayer]

On 12/02/2014 09:25 AM, Mark Rutland wrote:

Hi,


+/* MPU L2 Register Defines */
+#define ALTR_MPUL2_CONTROL_OFFSET   0x100
+#define ALTR_MPUL2_CTL_CACHE_EN_MASKBIT(0)


These are just standard PL310 register definitions, no?


Yes.


[...]


+static void *ocram_alloc_mem(size_t size, void **other)
+{
+   struct device_node *np;
+   struct gen_pool *gp;
+   void *sram_addr;
+
+   np = of_find_compatible_node(NULL, NULL, altr,ocram-edac);
+   if (!np)
+   return NULL;


Don't you need to have a corresponding of_node_put?

Is this ever called before the code above has probed? Can't you keep
this information around rather than probing it every time?



This is only used for testing so it happens infrequently and always 
after the probe has finished.


These error injection functions will not be part of the standard 
distribution for our board so it was cleaner to leave it out.



+
+   gp = of_get_named_gen_pool(np, iram, 0);
+   if (!gp)
+   return NULL;
+
+   sram_addr = (void *)gen_pool_alloc(gp, size);
+   if (!sram_addr)
+   return NULL;
+
+   memset(sram_addr, 0, size);


Is it safe to do a memset to the sram? How is it mapped?



I checked the gen_pool and the memory may not be contiguous. I will fix 
this. Thanks!



[...]


+static void *l2_alloc_mem(size_t size, void **other)
+{
+   struct device *dev = *other;
+   void *ptemp = devm_kzalloc(dev, size, GFP_KERNEL);
+
+   if (!ptemp)
+   return NULL;
+
+   /* Make sure everything is written out */
+   wmb();
+   flush_cache_all();


Doesn't this just flush out _to_ the L2 (and not beyond)? Even then I
don't think that's safe with the MMU enabled.

Why is the flush necessary?



flush_cache_all() maps to flush_kern_all() which cleans all the cache 
levels up to the level of coherency which includes L2 in it.


This flush ensures the data is in a known state (zero) before I inject 
the errors for testing L2 ECC.



[...]


+static int altr_l2_dependencies(struct platform_device *pdev,
+   void __iomem *base)
+{
+   u32 control;
+   struct regmap *l2_vbase;
+
+   control = readl(base)  ALTR_L2_ECC_EN_MASK;
+   if (!control) {
+   edac_printk(KERN_ERR, EDAC_DEVICE,
+   L2: No ECC present, or ECC disabled\n);
+   return -ENODEV;
+   }
+
+   l2_vbase = syscon_regmap_lookup_by_compatible(arm,pl310-cache);
+   if (IS_ERR(l2_vbase)) {
+   edac_printk(KERN_ERR, EDAC_DEVICE,
+   L2:regmap for arm,pl310-cache lookup failed.\n);
+   return -ENODEV;
+   }


I must NAK any use of the L2 as a syscon device. It's simply not a
register file that was intended to be shared. I appreciate that you only
want to check something very simple, but we should use a higher level
API for that (and we should add one if we do not already have one)



OK. I didn't know of a way to read that register otherwise. I will 
rework this.



+
+   regmap_read(l2_vbase, ALTR_MPUL2_CONTROL_OFFSET, control);
+   if (!(control  ALTR_MPUL2_CTL_CACHE_EN_MASK)) {
+   edac_printk(KERN_ERR, EDAC_DEVICE, L2: Cache disabled\n);
+   return -ENODEV;
+   }
+
+   return 0;
+}
+
+const struct edac_device_prv_data l2ecc_data = {
+   .setup = altr_l2_dependencies,
+   .ce_clear_mask = 0,
+   .ue_clear_mask = 0,
+#ifdef CONFIG_EDAC_DEBUG
+   .eccmgr_sysfs_attr = altr_l2_sysfs_attributes,
+   .alloc_mem = l2_alloc_mem,
+   .free_mem = l2_free_mem,
+   .ecc_enable_mask = ALTR_L2_ECC_EN_MASK,
+   .ce_set_mask = (ALTR_L2_ECC_EN_MASK | ALTR_L2_ECC_INJS_MASK),
+   .ue_set_mask = (ALTR_L2_ECC_EN_MASK | ALTR_L2_ECC_INJD_MASK),
+   .trig_alloc_sz = ALTR_TRIG_L2C_BYTE_SIZE,
+#endif
+};
+
+#endif /* #ifdef CONFIG_EDAC_ALTERA_L2C */
+
  MODULE_LICENSE(GPL v2);
  MODULE_AUTHOR(Thor Thayer);
-MODULE_DESCRIPTION(EDAC Driver for Altera SDRAM Controller);
+MODULE_DESCRIPTION(EDAC Driver for Altera Memories);
--
1.7.9.5



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv5 5/5] arm: dts: Add Altera L2 Cache and OCRAM EDAC

[Thor Thayer]

On 12/02/2014 08:57 AM, Mark Rutland wrote:

On Wed, Nov 12, 2014 at 12:14:23AM +, ttha...@opensource.altera.com wrote:

From: Thor Thayer ttha...@opensource.altera.com

Adding the device tree entries and bindings needed to support
the Altera L2 cache and On-Chip RAM EDAC. This patch relies upon
an earlier patch to declare and setup On-chip RAM properly.
http://www.spinics.net/lists/devicetree/msg51117.html

Signed-off-by: Thor Thayer ttha...@opensource.altera.com


snip


+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-l2-edac.txt
@@ -0,0 +1,15 @@
+Altera SoCFPGA L2 cache Error Detection and Correction [EDAC]
+
+Required Properties:
+- compatible : Should be altr,l2-edac
+- reg : Address and size for ECC error interrupt clear registers.
+- interrupts : Should be single bit error interrupt, then double bit error
+   interrupt. Note the rising edge type.
+
+Example:
+
+   l2edac@ffd08140 {
+   compatible = altr,l2-edac;
+   reg = 0xffd08140 0x4;
+   interrupts = 0 36 1, 0 37 1;
+   };


Judging by the size of the reg entry, this is part of a larger block
(the same one the OCRAM EDAC lives in). Why isn't that larger block
described?

EDAC is a Linux subsystem name, but typically not the HW block name.
What HW block does this live in?



Yes, this register is part of the ECC block of registers. In order to 
get each probe function to run, the L2 EDAC and OCRAM EDAC had to be at 
the top level of the device tree. If they are children of a parent node, 
the probe functions aren't executed.

Is there a better way or example I should follow?


diff --git 
a/Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt 
b/Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt
new file mode 100644
index 000..31ab205
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-ocram-edac.txt


snip


+
L2: l2-cache@fffef000 {
-   compatible = arm,pl310-cache;
+   compatible = arm,pl310-cache, syscon;


NAK.

Why are you marking the PL310 as a syscon device? It is most definitely
_NOT_ a shared set of registers lumped together.



Unfortunately, the register is locked for exclusive access by the L2 
cache driver. I read your comment on patch 4 and will find a better way.



Thanks,
Mark.


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv3 4/5] edac: altera: Add Altera L2 Cache and OCRAM EDAC Support

[Thor Thayer]

Hi Boris!

On 11/04/2014 09:12 AM, Borislav Petkov wrote:

On Thu, Oct 30, 2014 at 10:32:10AM -0500, ttha...@opensource.altera.com wrote:

From: Thor Thayer ttha...@opensource.altera.com

Adding L2 Cache and On-Chip RAM EDAC support for the
Altera SoCs using the EDAC device  model. The SDRAM
controller is using the Memory Controller model. All
Altera EDAC functions live in altera_edac.c.

Signed-off-by: Thor Thayer ttha...@opensource.altera.com
---
v2: Fix L2 dependency comments.

v3: Move OCRAM and L2 cache EDAC functions into altera_edac.c
 instead of separate files.
---
  drivers/edac/Kconfig   |   14 ++
  drivers/edac/Makefile  |5 +-
  drivers/edac/altera_edac.c |  475 +++-
  3 files changed, 492 insertions(+), 2 deletions(-)

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 1719975..b145a52 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -385,4 +385,18 @@ config EDAC_ALTERA_MC
  preloader must initialize the SDRAM before loading
  the kernel.
  
+config EDAC_ALTERA_L2C

+   bool Altera L2 Cache EDAC
+   depends on EDAC_MM_EDAC=y  ARCH_SOCFPGA  CACHE_L2X0
+   help
+ Support for error detection and correction on the
+ Altera L2 cache Memory for Altera SoCs.
+
+config EDAC_ALTERA_OCRAM
+   bool Altera On-Chip RAM EDAC
+   depends on EDAC_MM_EDAC=y  ARCH_SOCFPGA  SRAM  GENERIC_ALLOCATOR
+   help
+ Support for error detection and correction on the
+ Altera On-Chip RAM Memory for Altera SoCs.

Why are those separate config items? Why not switch on EDAC_ALTERA_MC
and get it all?

We can always split them out later, if needed but I don't think
we should burden the user with unnecessary questions if it is not
absolutely necessary.

We want to at least separate L2/OCRAM ECC from the SDRAM ECC because
1) the SDRAM preparation can take almost 2 seconds on boot and some 
customers need a faster boot time.
2) the SDRAM has an ECC initialization dependency on the preloader which 
is outside the kernel. It is desirable to be able to turn the SDRAM on  
off separately.


You bring up a good point about the L2 and OCRAM being combined though.

If we do want granular control, maybe I should use a submenu? Or isn't 
that desirable either?

+
  endif # EDAC
diff --git a/drivers/edac/Makefile b/drivers/edac/Makefile
index 359aa49..fa8aebc 100644
--- a/drivers/edac/Makefile
+++ b/drivers/edac/Makefile
@@ -66,4 +66,7 @@ obj-$(CONFIG_EDAC_OCTEON_L2C) += octeon_edac-l2c.o
  obj-$(CONFIG_EDAC_OCTEON_LMC) += octeon_edac-lmc.o
  obj-$(CONFIG_EDAC_OCTEON_PCI) += octeon_edac-pci.o
  
-obj-$(CONFIG_EDAC_ALTERA_MC)		+= altera_edac.o

+alt_edac-y := altera_edac.o
+obj-$(CONFIG_EDAC_ALTERA_MC)   += alt_edac.o
+obj-$(CONFIG_EDAC_ALTERA_L2C)  += alt_edac.o
+obj-$(CONFIG_EDAC_ALTERA_OCRAM)+= alt_edac.o
diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index 3c4929f..1ee8d94 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -17,8 +17,10 @@
   * Adapted from the highbank_mc_edac driver.
   */
  
+#include asm/cacheflush.h

  #include linux/ctype.h
  #include linux/edac.h
+#include linux/genalloc.h
  #include linux/interrupt.h
  #include linux/kernel.h
  #include linux/mfd/syscon.h
@@ -33,6 +35,7 @@
  
  #define EDAC_MOD_STR		altera_edac

  #define EDAC_VERSION  1
+#define EDAC_DEVICEDEVICE

That name is not very descriptive.

I will change to using Altera EDAC Device

  /* SDRAM Controller CtrlCfg Register */
  #define CTLCFG_OFST 0x00
@@ -107,6 +110,30 @@ struct altr_sdram_mc_data {
struct regmap *mc_vbase;
  };
  
+/** EDAC Device Defines **/

+
+/* OCRAM ECC Management Group Defines */
+#define ALTR_MAN_GRP_OCRAM_ECC_OFFSET   0x04
+#define ALTR_OCR_ECC_EN_MASK0x0001
+#define ALTR_OCR_ECC_INJS_MASK  0x0002
+#define ALTR_OCR_ECC_INJD_MASK  0x0004
+#define ALTR_OCR_ECC_SERR_MASK  0x0008
+#define ALTR_OCR_ECC_DERR_MASK  0x0010
+
+/* MPU L2 Register Defines */
+#define ALTR_MPUL2_CONTROL_OFFSET   0x100
+#define ALTR_MPUL2_CTL_CACHE_EN_MASK0x0001
+
+/* L2 ECC Management Group Defines */
+#define ALTR_MAN_GRP_L2_ECC_OFFSET  0x00
+#define ALTR_L2_ECC_EN_MASK 0x0001
+#define ALTR_L2_ECC_INJS_MASK   0x0002
+#define ALTR_L2_ECC_INJD_MASK   0x0004

You can use BIT() for those.

Thanks. I will change this.

+
+/*** EDAC Memory Controller Functions /
+
+/* The SDRAM controller uses the EDAC Memory Controller framework.   */
+
  static irqreturn_t altr_sdram_mc_err_handler(int irq, void *dev_id)
  {
struct mem_ctl_info *mci = dev_id;
@@ -405,6 +432,452 @@ static struct platform_driver altr_sdram_edac_driver

Re: [RESEND PATCHv5 5/5] arm: dts: Add Altera L2 Cache and OCRAM EDAC

[Thor Thayer]

Hi Boris,

On 11/18/2014 02:56 PM, Thor Thayer wrote:

Hi all,

On 11/11/2014 06:14 PM, ttha...@opensource.altera.com wrote:

From: Thor Thayerttha...@opensource.altera.com

Adding the device tree entries and bindings needed to support
the Altera L2 cache and On-Chip RAM EDAC. This patch relies upon
an earlier patch to declare and setup On-chip RAM properly.
http://www.spinics.net/lists/devicetree/msg51117.html


Any comments on these devicetree additions?

Thanks,
Thor

Signed-off-by: Thor Thayerttha...@opensource.altera.com
---
v2: Remove OCRAM declaration and reference prior patch.

v3-5: No Change
---


I originally submitted this series on November 11, 2014.

I haven't received any comments or ACKs on the device tree patch portion 
(patch 5 of 5). According to 
Documentation/devicetree/bindings/submitting-patches.txt, if the patch 
hasn't been Acked after a couple of weeks, the maintainer can pull the 
changes if they are comfortable with those changes.


Dinh checked for merge conflicts in the DTS files in his November 7 
email (queued patches for 3.19).


Dinh acked patches 1  2 on November 12.

Thanks,

Thor

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv6 2/5] arm: socfpga: Enable OCRAM ECC on startup.

[Thor Thayer]

Hi Mark,

On 02/06/2015 12:45 PM, Mark Rutland wrote:

On Fri, Jan 09, 2015 at 02:53:53AM +, ttha...@opensource.altera.com wrote:

From: Thor Thayer ttha...@opensource.altera.com

This patch enables the ECC for On-Chip RAM on machine
startup.  The ECC has to be enabled before data is
is stored in memory otherwise the ECC will fail on
reads.


Where else is this OCRAM used?

If we need the ECC to be enabled before use, a module_init call that
seems to be unrelated to any other usage doesn't seem right to me.
Hopefully I've just misunderstood something here.



Thank you for reviewing this.

The OCRAM will be used to store data and functions for putting and 
removing the SoC from sleep. However, in this scenario, we won't have 
the ECC enabled.


To initialize ECC, the OCRAM needs to enable ECC then clear the entire 
memory to zero before using it. Doing this early in the startup sequence 
seemed appropriate. Maybe I'm misunderstanding your concern.


 snip


+static int __init socfpga_init_ocram_ecc(void)
+{
+   struct device_node *np;
+   struct resourceres;
+   u32iram_addr;
+   void __iomem   *mapped_ocr_edac_addr;
+   resource_size_tsize;
+   struct gen_pool*gp;
+   intret;
+
+   /* Get the size of the on-chip RAM */
+   np = of_find_compatible_node(NULL, NULL, mmio-sram);
+   if (!np) {
+   pr_err(%s: Unable to find mmio-sram in dtb\n, __func__);
+   return -ENODEV;
+   }
+
+   ret = of_address_to_resource(np, 0, res);
+   if (ret) {
+   of_node_put(np);
+   pr_err(%s: Problem getting SRAM address in dtb\n, __func__);
+   return -ENODEV;
+   }
+   size = resource_size(res);
+   of_node_put(np);
+
+   /* Find the OCRAM EDAC device tree node */
+   np = of_find_compatible_node(NULL, NULL, altr,ocram-edac);
+   if (!np) {
+   pr_err(%s: Unable to find altr,ocram-edac\n, __func__);
+   return -ENODEV;
+   }
+
+   mapped_ocr_edac_addr = of_iomap(np, 0);
+   if (!mapped_ocr_edac_addr) {
+   of_node_put(np);
+   pr_err(%s: Unable to map OCRAM ecc regs.\n, __func__);
+   return -ENODEV;
+   }
+
+   gp = of_get_named_gen_pool(np, iram, 0);
+   if (!gp) {
+   of_node_put(np);
+   pr_err(%s: OCRAM cannot find gen pool\n, __func__);
+   return -ENODEV;
+   }
+   of_node_put(np);
+
+   iram_addr = gen_pool_alloc(gp, size / sizeof(size_t));


Why divide by sizeof(size_t) here? As far as I am aware, resource_size
gives you a size in bytes...



Yes, you are right. I shouldn't have changed this - for some reason when 
I re-read the function prototype, I thought this function was allocating 
integers. Thank you.




+   if (iram_addr == 0) {
+   pr_err(%s: cannot alloc from gen pool\n, __func__);
+   return -ENODEV;
+   }
+
+   /* Clear any pending OCRAM ECC interrupts, then enable ECC */
+   writel(ALTR_OCRAM_CLEAR_ECC, mapped_ocr_edac_addr);
+   writel(ALTR_OCRAM_ECC_EN, mapped_ocr_edac_addr);
+
+   memset((void *)iram_addr, 0, size);


...and here we write size bytes, not (size / sizeof(size_t)) bytes, so
we're poking memory we weren't allocated.

How is this memory mapped exactly? Is memset safe?

Thanks,
Mark.



Yes, thank you for catching that.

OK. I think I understand your point now. I need to enable the OCRAM ECC 
before the gen_pool_create() which is called by the 
ocram:sram@ node.


In that case, I should move this OCRAM ECC enable into the same place as 
L2 ecc is enabled.


Thor


+
+   gen_pool_free(gp, iram_addr, size / sizeof(size_t));
+
+   iounmap(mapped_ocr_edac_addr);
+
+   return 0;
+}
+
+static void __exit socfpga_exit_ocram_ecc(void)
+{
+}
+
+module_init(socfpga_init_ocram_ecc);
+module_exit(socfpga_exit_ocram_ecc);
--
1.7.9.5



--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv6 5/5] arm: dts: Add Altera L2 Cache and OCRAM EDAC entries

[Thor Thayer]

Hi Mark

On 02/06/2015 01:24 PM, Mark Rutland wrote:

On Fri, Jan 09, 2015 at 02:53:56AM +, ttha...@opensource.altera.com wrote:

From: Thor Thayer ttha...@opensource.altera.com

Adding the device tree entries and bindings needed to support
the Altera L2 cache and On-Chip RAM EDAC. This patch relies upon
an earlier patch to declare and setup On-chip RAM properly.
http://www.spinics.net/lists/devicetree/msg51117.html

Signed-off-by: Thor Thayer ttha...@opensource.altera.com
---
v2: Remove OCRAM declaration and reference prior patch.

v3-5: No Change

v6: Change to nested EDAC device nodes based on community
 feedback. Remove L2 syscon. Use consolidated binding.
---
  .../bindings/arm/altera/socfpga-edac.txt   |   46 
  arch/arm/boot/dts/socfpga.dtsi |   20 +
  2 files changed, 66 insertions(+)
  create mode 100644 
Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt

diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt 
b/Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt
new file mode 100644
index 000..4bf32e1
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt
@@ -0,0 +1,46 @@
+Altera SoCFPGA Error Detection and Correction [EDAC]
+
+Required Properties:
+- compatible : Should be altr,edac


Is there a single large EDAC block that happens to perform EDAC
functions for various components, or various small EDAC blocks that
happen to be close to each other in the MMIO space?



There is a sequential grouping of ECC registers containing IP specific 
ECC enable and ECC error injection bits. A number of these registers 
have the same bit definitions but a few such as L2 cache are different.


If I understand your question, I'd characterize these as various small 
EDAC blocks that happen to be close to each other in the MMIO space.


There is no master - these IP memories are meant to be individually enabled.


+- #address-cells: must be 1
+- #size-cells: must be 1
+- ranges : standard definition, should translate from local addresses


Surely these just need to be suitable for mapping the child nodes, and
you shouldn't care about their precise values?



If you are saying that I place the base address in the parent and then 
just include the offset from the parent registers [0 for L2 cache and 4 
for OCRAM] in each child then I'll need to research how to do that. I 
think our clock manager may do this.



+
+Subcomponents:
+
+L2 Cache ECC
+Required Properties:
+- compatible : Should be altr,l2-edac
+- reg : Address and size for ECC error interrupt clear registers.
+- interrupts : Should be single bit error interrupt, then double bit error
+   interrupt. Note the rising edge type.
+
+On Chip RAM ECC
+Required Properties:
+- compatible : Should be altr,ocram-edac
+- reg : Address and size for ECC error interrupt clear registers.
+- iram : phandle to On-Chip RAM definition.
+- interrupts : Should be single bit error interrupt, then double bit error
+   interrupt. Note the rising edge type.


Do these actually differ in programming interface, or just w.r.t. the
component they are used to monitor?


The interface is similar but the bit definitions for ECC Enable and bit 
injection are different between these two IPs. However, these 
differences are taken care of with different const data structures in 
altera_edac.c.




It would be good to have an explicit link to what they monitor rather
than relying on there being single instances with particular compatible
strings. That will make this easier to generalise for multiple instances
in future SoCs that may reuse the EDAC block.



You're suggesting a phandle to the L2 cache similar to the phandle the 
OCRAM uses? The probe function would then grab the appropriate const 
data structure based on where the phandle pointed, correct?



This doesn't look too bad, but I'd like to hear some response to this
and my other queries before I can say whether this is a good way of
describing the hardware; it all looks a bit vague.

As an aside, please place the documentation at the start of the series,
before the driver rework. The dts patches can come after the docs and
code patches.



I will do that. Thanks for reviewing.


Thanks,
Mark.


+
+Example:
+
+   soc_ecc {
+   compatible = altr,edac;
+   #address-cells = 1;
+   #size-cells = 1;
+   ranges;
+
+   l2edac@ffd08140 {
+   compatible = altr,l2-edac;
+   reg = 0xffd08140 0x4;
+   interrupts = 0 36 1, 0 37 1;
+   };
+
+   ocramedac@ffd08144 {
+   compatible = altr,ocram-edac;
+   reg = 0xffd08144 0x4;
+   iram = ocram;
+   interrupts = 0 178 1, 0 179 1;
+   };
+   };
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts

Re: [PATCHv6 4/5] edac: altera: Add Altera L2 Cache and OCRAM EDAC Support

[Thor Thayer]

On 02/06/2015 01:17 PM, Mark Rutland wrote:

On Fri, Jan 09, 2015 at 02:53:55AM +, ttha...@opensource.altera.com wrote:

From: Thor Thayer ttha...@opensource.altera.com

Adding L2 Cache and On-Chip RAM EDAC support for the
Altera SoCs using the EDAC device  model. The SDRAM
controller is using the Memory Controller model.

Each type of ECC is individually configurable.

The SDRAM ECC is a separate Kconfig option because:
1) the SDRAM preparation can take almost 2 seconds on boot and some
customers need a faster boot time.
2) the SDRAM has an ECC initialization dependency on the preloader
which is outside the kernel. It is desirable to be able to turn the
SDRAM on  off separately.

Signed-off-by: Thor Thayer ttha...@opensource.altera.com
---
v2: Fix L2 dependency comments.

v3: Move OCRAM and L2 cache EDAC functions into altera_edac.c
 instead of separate files.

v4: Change mask defines to use BIT().
 Fix comment style to agree with kernel coding style.
 Better printk description for read != write in trigger.
 Remove SysFS debugging message.
 Better dci-mod_name
 Move gen_pool pointer assignment to end of function.
 Invert logic to reduce indent in ocram depenency check.
 Change from dev_err() to edac_printk()
 Replace magic numbers with defines  comments.
 Improve error injection test.
 Change Makefile intermediary name to altr (from alt)

v5: No change.

v6: Convert to nested EDAC in device tree. Force L2 cache
 on for L2Cache ECC  remove L2 cache syscon for checking
 enable bit. Update year in header.
---
  drivers/edac/Kconfig   |   16 ++
  drivers/edac/Makefile  |5 +-
  drivers/edac/altera_edac.c |  506 +++-
  3 files changed, 524 insertions(+), 3 deletions(-)


[...]


+/* EDAC Parent Probe */
+
+static const struct of_device_id altr_edac_device_of_match[];


Huh? What's this for?


I needed this as a parameter for the of_platform_populate() function in 
altr_edac_probe(). I will change the naming to prevent misunderstanding.





+static const struct of_device_id altr_edac_of_match[] = {
+   { .compatible = altr,edac },
+   {},
+};
+MODULE_DEVICE_TABLE(of, altr_edac_of_match);


I know it may seem like a minor thing, but the documentation really
should have come in an earlier patch. It's painful to review a patch
series when you have to randomly just to the end of hte series to see
the documentation.

The name is _very_ generic here. Do we not have a more specific name for
the EDAC block in your SoC?

Is there actually a specific EDAC device, or are you just grouping some
portions of HW blocks into an EDAC device to match what the Linux EDAC
framework wants?

[...]



Yes, I can move the documentation - I understand better from the 5/5 
email what you are looking for.


I can change the name to ECC Manager. There are a group of consecutive 
registers to enable and disable ECC for peripherals. Unfortunately, the 
SDRAM register is in a completely different area (not grouped with these 
peripherals) and seems better suited to the Memory Controller EDAC 
instead of a device EDAC for the peripherals.


There is not a specific EDAC device. I grouped these HW blocks into 
different instances of an EDAC device to match what I though the Linux 
EDAC framework wanted.



+ssize_t altr_edac_device_trig(struct edac_device_ctl_info *edac_dci,
+ const char *buffer, size_t count)
+{
+   u32 *ptemp, i, error_mask;
+   int result = 0;
+   unsigned long flags;
+   struct altr_edac_device_dev *drvdata = edac_dci-pvt_info;
+   const struct edac_device_prv_data *priv = drvdata-data;
+   void *generic_ptr = edac_dci-dev;


Huh? What's hidden behind this generic_ptr?



There are 2 memory allocation functions (ocram  L2) that return a 
pointer to the data. The OCRAM allocation function returns the gen_pool 
pointer in the generic_ptr so that it can be freed later.



+
+   if (!priv-alloc_mem)
+   return -ENOMEM;
+
+   /*
+* Note that generic_ptr is initialized to the device * but in
+* some alloc_functions, this is overridden and returns data.
+*/
+   ptemp = priv-alloc_mem(priv-trig_alloc_sz, generic_ptr);
+   if (!ptemp) {
+   edac_printk(KERN_ERR, EDAC_DEVICE,
+   Inject: Buffer Allocation error\n);
+   return -ENOMEM;
+   }
+
+   if (buffer[0] == ALTR_UE_TRIGGER_CHAR)
+   error_mask = priv-ue_set_mask;
+   else
+   error_mask = priv-ce_set_mask;
+
+   edac_printk(KERN_ALERT, EDAC_DEVICE,
+   Trigger Error Mask (0x%X)\n, error_mask);
+
+   local_irq_save(flags);
+   /* write ECC corrupted data out. */
+   for (i = 0; i  (priv-trig_alloc_sz / sizeof(*ptemp)); i++) {
+   /* Read data so we're in the correct state

[RESEND PATCHv6 5/5] arm: dts: Add Altera L2 Cache and OCRAM EDAC entries

[Thor Thayer]

Hi Device Tree Maintainers,

On 01/08/2015 08:53 PM, ttha...@opensource.altera.com wrote:

From: Thor Thayer ttha...@opensource.altera.com

Adding the device tree entries and bindings needed to support
the Altera L2 cache and On-Chip RAM EDAC. This patch relies upon
an earlier patch to declare and setup On-chip RAM properly.
http://www.spinics.net/lists/devicetree/msg51117.html

Signed-off-by: Thor Thayer ttha...@opensource.altera.com
---
v2: Remove OCRAM declaration and reference prior patch.

v3-5: No Change

v6: Change to nested EDAC device nodes based on community
 feedback. Remove L2 syscon. Use consolidated binding.


I'm requesting comments on this patch series. The changes in this patch 
series are based upon feedback from Mark Rutland for patch series 
version 5 on December 2, 2014.


I believe this patch set addresses the concerns that Mark had with my 
previous patch. Primarily, syscon was removed from the L2 cache and a 
top level device tree node with L2 and OCRAM children is used instead of 
individual top level nodes. Some concerns were addressed in an email 
reply on December 2, 2014.


This change also created a new edac parent probe function which is in 
[PATCHv6 4/5] which should be reviewed along with this device tree change.


Thanks,

Thor


---
  .../bindings/arm/altera/socfpga-edac.txt   |   46 
  arch/arm/boot/dts/socfpga.dtsi |   20 +
  2 files changed, 66 insertions(+)
  create mode 100644 
Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt

diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt 
b/Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt
new file mode 100644
index 000..4bf32e1
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt
@@ -0,0 +1,46 @@
+Altera SoCFPGA Error Detection and Correction [EDAC]
+
+Required Properties:
+- compatible : Should be altr,edac
+- #address-cells: must be 1
+- #size-cells: must be 1
+- ranges : standard definition, should translate from local addresses
+
+Subcomponents:
+
+L2 Cache ECC
+Required Properties:
+- compatible : Should be altr,l2-edac
+- reg : Address and size for ECC error interrupt clear registers.
+- interrupts : Should be single bit error interrupt, then double bit error
+   interrupt. Note the rising edge type.
+
+On Chip RAM ECC
+Required Properties:
+- compatible : Should be altr,ocram-edac
+- reg : Address and size for ECC error interrupt clear registers.
+- iram : phandle to On-Chip RAM definition.
+- interrupts : Should be single bit error interrupt, then double bit error
+   interrupt. Note the rising edge type.
+
+Example:
+
+   soc_ecc {
+   compatible = altr,edac;
+   #address-cells = 1;
+   #size-cells = 1;
+   ranges;
+
+   l2edac@ffd08140 {
+   compatible = altr,l2-edac;
+   reg = 0xffd08140 0x4;
+   interrupts = 0 36 1, 0 37 1;
+   };
+
+   ocramedac@ffd08144 {
+   compatible = altr,ocram-edac;
+   reg = 0xffd08144 0x4;
+   iram = ocram;
+   interrupts = 0 178 1, 0 179 1;
+   };
+   };
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 252c3d1..e546e47 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -618,6 +618,26 @@
interrupts = 0 39 4;
};

+   soc_ecc {
+   compatible = altr,edac;
+   #address-cells = 1;
+   #size-cells = 1;
+   ranges;
+
+   l2edac@ffd08140 {
+   compatible = altr,l2-edac;
+   reg = 0xffd08140 0x4;
+   interrupts = 0 36 1, 0 37 1;
+   };
+
+   ocramedac@ffd08144 {
+   compatible = altr,ocram-edac;
+   reg = 0xffd08144 0x4;
+   iram = ocram;
+   interrupts = 0 178 1, 0 179 1;
+   };
+   };
+
L2: l2-cache@fffef000 {
compatible = arm,pl310-cache;
reg = 0xfffef000 0x1000;


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv6 0/5] Add Altera peripheral memories to EDAC framework

[Thor Thayer]

Hi Device Tree Maintainers,

On 01/08/2015 08:53 PM, ttha...@opensource.altera.com wrote:

From: Thor Thayer ttha...@opensource.altera.com

This patch adds the L2 cache and OCRAM peripherals to the EDAC framework
using the EDAC device framework. The ECC is enabled early in the boot
process in the platform specific code.



The changes in this patch series revision were mainly to address device 
tree concerns. There were changes in other areas of the code to address 
these changes but I believe the other maintainers are waiting to see if 
these changes are accepted before they will review (they had approved 
the previous patch changes).


How does the this patch series appear from a device tree perspective?

Thank you for your time,

Thor



v2 changes:
- Split On-Chip RAM ECC platform initialization into separate patch from
   L2 ECC platform initialization.
- Fix L2 cache dependency comments.
- Remove OCRAM node from dts and reference prior patch.

v3 changes:
- Move L2 cache  On-Chip RAM EDAC code into altera_edac.c
- Remove SDRAM module compile.

v4 changes:
- Change mask defines to use BIT().
- Fix comment style to agree with kernel coding style.
- Better printk description for read != write in trigger.
- Remove SysFS debugging message.
- Better dci-mod_name
- Move gen_pool pointer assignment to end of function.
- Invert logic to reduce indent in ocram depenency check.
- Change from dev_err() to edac_printk()
- Replace magic numbers with defines  comments.
- Improve error injection test.
- Change Makefile intermediary name to altr (from alt)

v5 changes:
- Remove l2cache.h by using if (IS_ENABLED(CONFIG_EDAC_ALTERA_L2C))
- Remove ocram.h by using if (IS_ENABLED(CONFIG_EDAC_ALTERA_OCRAM))
- Check prop variable before using. Include io.h.
- Add defines for better readability. Remove MAINTAINERS changes.

v6 changes:
- Simplify OCRAM initialization. Remove be32_to_cpup() calls.
- Remove syscon from L2 Cache. Force L2 Cache on if ECC enabled.
- Convert to nested ECC in device tree.
- Additional comments to clarify debug error injection.

Thor Thayer (5):
   arm: socfpga: Enable L2 Cache ECC on startup.
   arm: socfpga: Enable OCRAM ECC on startup.
   edac: altera: Remove SDRAM module compile
   edac: altera: Add Altera L2 Cache and OCRAM EDAC Support
   arm: dts: Add Altera L2 Cache and OCRAM EDAC entries

  .../bindings/arm/altera/socfpga-edac.txt   |   46 ++
  arch/arm/boot/dts/socfpga.dtsi |   20 +
  arch/arm/mach-socfpga/Makefile |2 +
  arch/arm/mach-socfpga/core.h   |2 +
  arch/arm/mach-socfpga/l2_cache.c   |   39 ++
  arch/arm/mach-socfpga/ocram.c  |   97 
  arch/arm/mach-socfpga/socfpga.c|4 +-
  drivers/edac/Kconfig   |   20 +-
  drivers/edac/Makefile  |5 +-
  drivers/edac/altera_edac.c |  506 +++-
  10 files changed, 735 insertions(+), 6 deletions(-)
  create mode 100644 
Documentation/devicetree/bindings/arm/altera/socfpga-edac.txt
  create mode 100644 arch/arm/mach-socfpga/l2_cache.c
  create mode 100644 arch/arm/mach-socfpga/ocram.c


--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 3/4] edac, altera: Addition of Arria10 EDAC

[Thor Thayer]

On 05/14/2015 03:20 PM, Dinh Nguyen wrote:

On 05/13/2015 04:49 PM, ttha...@opensource.altera.com wrote:

From: Thor Thayer ttha...@opensource.altera.com

The Arria10 SDRAM and ECC system differs significantly from the
Cyclone5 and Arria5 SoCs. This patch adds support for the Arria10
SoC.
1) IRQ handler needs to support SHARED IRQ
2) Support sberr and dberr address reporting.

Signed-off-by: Thor Thayer ttha...@opensource.altera.com
---
  drivers/edac/altera_edac.c |  132 ++--
  drivers/edac/altera_edac.h |   85 
  2 files changed, 201 insertions(+), 16 deletions(-)

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index 204ad2d..735a180 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -42,6 +42,7 @@ const struct altr_sdram_prv_data c5_data = {
.ecc_stat_ce_mask   = CV_DRAMSTS_SBEERR,
.ecc_stat_ue_mask   = CV_DRAMSTS_DBEERR,
.ecc_saddr_offset   = CV_ERRADDR_OFST,
+   .ecc_daddr_offset   = CV_ERRADDR_OFST,
.ecc_cecnt_offset   = CV_SBECOUNT_OFST,
.ecc_uecnt_offset   = CV_DBECOUNT_OFST,
.ecc_irq_en_offset  = CV_DRAMINTR_OFST,
@@ -57,37 +58,62 @@ const struct altr_sdram_prv_data c5_data = {
  #endif
  };

+const struct altr_sdram_prv_data a10_data = {\


This should be static.


+   .ecc_ctrl_offset= A10_ECCCTRL1_OFST,
+   .ecc_ctl_en_mask= A10_ECCCTRL1_ECC_EN,
+   .ecc_stat_offset= A10_INTSTAT_OFST,
+   .ecc_stat_ce_mask   = A10_INTSTAT_SBEERR,
+   .ecc_stat_ue_mask   = A10_INTSTAT_DBEERR,
+   .ecc_saddr_offset   = A10_SERRADDR_OFST,
+   .ecc_daddr_offset   = A10_DERRADDR_OFST,
+   .ecc_irq_en_offset  = A10_ERRINTEN_OFST,
+   .ecc_irq_en_mask= A10_ECC_IRQ_EN_MASK,
+   .ecc_irq_clr_offset = A10_INTSTAT_OFST,
+   .ecc_irq_clr_mask   = (A10_INTSTAT_SBEERR | A10_INTSTAT_DBEERR),
+   .ecc_cnt_rst_offset = A10_ECCCTRL1_OFST,
+   .ecc_cnt_rst_mask   = A10_ECC_CNT_RESET_MASK,
+#ifdef CONFIG_EDAC_DEBUG
+   .ce_ue_trgr_offset  = A10_DIAGINTTEST_OFST,
+   .ce_set_mask= A10_DIAGINT_TSERRA_MASK,
+   .ue_set_mask= A10_DIAGINT_TDERRA_MASK,
+#endif
+};
+


snip


+
  static int altr_sdram_probe(struct platform_device *pdev)
  {
const struct of_device_id *id;
@@ -221,8 +295,8 @@ static int altr_sdram_probe(struct platform_device *pdev)
struct regmap *mc_vbase;
struct dimm_info *dimm;
u32 read_reg;
-   int irq, res = 0;
-   unsigned long mem_size;
+   int irq, irq2, res = 0;
+   unsigned long mem_size, irqflags;

id = of_match_device(altr_sdram_ctrl_of_match, pdev-dev);
if (!id)
@@ -288,6 +362,9 @@ static int altr_sdram_probe(struct platform_device *pdev)
return -ENODEV;
}

+   /* Arria10 has a 2nd IRQ */
+   irq2 = platform_get_irq(pdev, 1);
+
layers[0].type = EDAC_MC_LAYER_CHIP_SELECT;
layers[0].size = 1;
layers[0].is_virt_csrow = true;
@@ -332,8 +409,31 @@ static int altr_sdram_probe(struct platform_device *pdev)
if (res  0)
goto err;

+   /* Only the Arria10 has separate IRQs */
+   if (irq2  0) {
+   /* Arria10 specific initialization */
+   res = a10_init(mc_vbase);
+   if (res  0)
+   goto err2;
+
+   res = a10_unmask_irq(pdev, A10_DDR0_IRQ_MASK);
+   if (res  0)
+   goto err2;
+
+   res = devm_request_irq(pdev-dev, irq2,
+  altr_sdram_mc_err_handler,
+  IRQF_SHARED, dev_name(pdev-dev), mci);
+   if (res  0) {
+   edac_mc_printk(mci, KERN_ERR,
+  Unable to request irq %d\n, irq2);
+   res = -ENODEV;
+   goto err2;
+   }
+   irqflags = IRQF_SHARED;
+   }
+
res = devm_request_irq(pdev-dev, irq, altr_sdram_mc_err_handler,
-  0, dev_name(pdev-dev), mci);
+  irqflags, dev_name(pdev-dev), mci);


irqflags was never set for the case of !(irq2  0).

Dinh



Correct. So irqflags will be 0 for the CycloneV case.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 4/4] dts, altera: Arria10 SDRAM EDAC DTS additions.

[Thor Thayer]

Hi Arnd,

On 05/15/2015 05:55 AM, Arnd Bergmann wrote:

On Wednesday 13 May 2015 16:49:47 ttha...@opensource.altera.com wrote:

+   sdr: sdr@ffc25000 {
+   compatible = syscon;
+   reg = 0xffcfb100 0x80;
+   };
+



A syscon node with just 128 bytes seems very odd. Can you check the
hardware manual to see if this is part of some bigger unit?

Arnd



This is an unfortunate legacy of our previous SDRAM controller (in the 
CycloneV) which had ECC registers interspersed with registers other 
drivers needed - thus the use of syscon.


In the Arria10 chip, the ECC registers are in their own partitioned 
group but I kept the syscon to remain consistent with the Device Tree 
bindings from the CycloneV family.


I've implemented your other suggestions. Thank you for reviewing!

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

Re: [PATCHv2 2/4] edac, altera: Refactor EDAC for Altera CycloneV SoC.

[Thor Thayer]

On 06/04/2015 10:26 AM, Dinh Nguyen wrote:

On 06/04/2015 09:28 AM, ttha...@opensource.altera.com wrote:

From: Thor Thayer ttha...@opensource.altera.com

The Arria10 SOC uses a completely different SDRAM controller from the
earlier CycloneV and ArriaV SoCs. This patch abstracts the SDRAM bits
for the CycloneV/ArriaV SoCs in preparation for the Arria10 support.

Signed-off-by: Thor Thayer ttha...@opensource.altera.com
---
v2: Make c5_data static.
---
  drivers/edac/altera_edac.c |  194 
  drivers/edac/altera_edac.h |  116 ++
  2 files changed, 206 insertions(+), 104 deletions(-)
  create mode 100644 drivers/edac/altera_edac.h

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index a9e7c69..a8350f6 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c


What branch are you basing this patch on? I get the following error when
applied to both v4.1-rc6, linux-next, and linux-edac/linux-next.

Applying: edac, altera: Refactor EDAC for Altera CycloneV SoC.
error: patch failed: drivers/edac/altera_edac.c:400
error: drivers/edac/altera_edac.c: patch does not apply
Patch failed at 0001 edac, altera: Refactor EDAC for Altera CycloneV SoC.

Dinh



OK. I'll refactor and resend. I was using Altera's internal for-next branch.
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] EDAC, altera: wrap edac pm with a CONFIG_PM

[Thor Thayer]

Hi Boris,

On 06/05/2015 10:15 AM, Borislav Petkov wrote:

On Fri, Jun 05, 2015 at 08:49:15AM -0500, dingu...@opensource.altera.com wrote:

From: Alan Tull at...@opensource.altera.com

Suspend-to-RAM and EDAC support are mutually exclusive on
SOCFPGA.  If the EDAC is enabled, it will prevent the
platform from going into suspend.


Btw, what is exactly the problem with EDAC being enabled and not being
able to suspend? Is it a hardware issue?

Because if it is sw one, we probably could change the EDAC core to
accomodate power management. On x86 we don't need to do anything special
to EDAC wrt PM though - we simply suspend.

Thanks.



Yes, in our case, it is a hardware issue but I'm still gathering 
information.


The IRQ vectors for OCRAM reside on DDR and in Suspend-to-RAM mode we're 
executing out of OCRAM. If an ECC error occurs, we can't handle it so it 
was decided to make them mutually exclusive.


Thor

--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support

[Thor Thayer]

On 01/04/2016 03:07 PM, Borislav Petkov wrote:

On Mon, Jan 04, 2016 at 02:55:43PM -0600, Dinh Nguyen wrote:

Right. So for us, if we build in SDRAM ECC unconditionally, there is a
requirement with the bootloader to turn on ECC and scrub the memory.


Huh, how does a built-in piece of code cause the bootloader to do
something?!?

And how would the bootloader know what's in the kernel? The bootloader
runs first and hands off to the kernel...


Hi Boris,

The decision about ECC or non-ECC SDRAM is made before building the 
Linux image and must be matched to the appropriate bootloader (ECC or 
non-ECC).


If ECC is desired for SDRAM, the bootloader enables SDRAM ECC and then 
initializes the memory contents (scrub) before the Linux image is loaded 
into SDRAM.


The ECC syndromes are calculated and stored in SDRAM only when SDRAM ECC 
is enabled and the SDRAM data is written (in the bootloader case, this 
is the Linux image). If we suddenly switched ECC on during Linux 
initialization, we'd be flooded with ECC errors since the ECC syndromes 
won't match the data for the Linux image.


The scrubbing process takes more time to boot which some of our 
customers don't want. This is what Dinh was referring to.

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

Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support

[Thor Thayer]

On 01/04/2016 04:01 PM, Borislav Petkov wrote:

On Mon, Jan 04, 2016 at 03:33:23PM -0600, Thor Thayer wrote:

The decision about ECC or non-ECC SDRAM is made before building the Linux
image and must be matched to the appropriate bootloader (ECC or non-ECC).

If ECC is desired for SDRAM, the bootloader enables SDRAM ECC and then
initializes the memory contents (scrub) before the Linux image is loaded
into SDRAM.

The ECC syndromes are calculated and stored in SDRAM only when SDRAM ECC is
enabled and the SDRAM data is written (in the bootloader case, this is the
Linux image). If we suddenly switched ECC on during Linux initialization,
we'd be flooded with ECC errors since the ECC syndromes won't match the data
for the Linux image.

The scrubbing process takes more time to boot which some of our customers
don't want. This is what Dinh was referring to.


So that still doesn't have any effect on what's compiled in the EDAC
module, AFAICT. You simply build everything in and depending on
whether ECC is enabled or not in the bootloader, altera_edac behaves
accordingly. On a system with ECC *not* enabled, it would simply have
the SDRAM ECC functionality inactive.

This is no different than an x86 system where you enter the BIOS and
enable or disable ECC. The EDAC module queries whether ECC has been
enabled or not and behaves accordingly.


OK. I see your point for SDRAM.

However, in the case of OCRAM and L2 cache ECC, we want to be able to 
enable them individually which is what the following does.


>> -obj-$(CONFIG_EDAC_ALTERA_MC)  += altera_edac.o
>> +altr_edac-y   := altera_edac.o
>> +obj-$(CONFIG_EDAC_ALTERA_MC)  += altr_edac.o
>> +obj-$(CONFIG_EDAC_ALTERA_L2C) += altr_edac.o
>> +obj-$(CONFIG_EDAC_ALTERA_OCRAM)   += altr_edac.o
>
> What are those supposed to accomplish?
>

and then the defines are also used to conditionally include the L2 or 
OCRAM ECC functions because everything is in one file.


However, the highbank and octeon edacs are split into separate files for 
L2 which Dinh pointed may be cleaner for individual control.


Thanks,

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

Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support

[Thor Thayer]

On 01/05/2016 04:58 AM, Borislav Petkov wrote:

On Mon, Jan 04, 2016 at 05:42:40PM -0600, Thor Thayer wrote:

and then the defines are also used to conditionally include the L2 or OCRAM
ECC functions because everything is in one file.


So?

You don't have to do those funny games in the Makefile. Instead, you
have your main CONFIG_EDAC_ALTERA_MC option and all the other CONFIG_*
options depend on it. The ifdeffery in altera_edac.c then takes care of
what needs to be enabled or not.

However(!), your driver is not huge or something. So I still don't
understand why you need that split and those additional config options
and why not keep it all together in one file. What is the compelling use
case for that split and additional complexity?


However, the highbank and octeon edacs are split into separate files for L2
which Dinh pointed may be cleaner for individual control.


I should've nacked that split at the time.


OK, Thanks for the clarification.

The CONFIG_EDAC_ALTERA_MC is a little confusing because it refers to the 
Memory Controller (SDRAM) and uses that in the menu string(Altera SDRAM 
Memory Controller EDAC).


Would it be confusing to rename this CONFIG_EDAC_ALTERA, update the 
SDRAM code to check the ECC Enable bit instead of this config option and 
update the string in the menu?


I'll keep this as one file and implement your suggested changes and 
resubmit.

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

Re: [PATCHv4 4/7] Documentation: dt: socfpga: Add Arria10 Ethernet binding

[Thor Thayer]

Hi Rob,

On 06/21/2016 08:33 AM, Rob Herring wrote:

On Mon, Jun 20, 2016 at 09:50:49AM -0500, ttha...@opensource.altera.com wrote:

From: Thor Thayer <ttha...@opensource.altera.com>

Add the device tree bindings needed to support the Altera Ethernet
FIFO buffers on the Arria10 chip.

Signed-off-by: Thor Thayer <ttha...@opensource.altera.com>
---
v2  No Change
v3  Change to common compatible string based on maintainer comments
 Add local IRQ values.
v4  Add compatible string for parent node.
---
  .../bindings/arm/altera/socfpga-eccmgr.txt |   24 
  1 file changed, 24 insertions(+)

diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt 
b/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
index 15eb0df..7c714ba 100644
--- a/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
@@ -82,6 +82,14 @@ Required Properties:
  - interrupts : Should be single bit error interrupt, then double bit error
interrupt, in this order.

+Ethernet FIFO ECC
+Required Properties:
+- compatible : Should be "altr,socfpga-eth-mac-ecc"
+- reg: Address and size for ECC block registers.
+- parent : phandle to parent (altr,socfpga-stmmac) Ethernet node.


Sorry if I wasn't clear before, but I was suggesting changing 'parent'
to 'altr,ethernet-mac':

altr,ethernet-mac = <>;

Rob


Ahh, I see what you're saying.

I used parent as the tag because I have a generic function for 
validating that the parent status is "okay" using the "parent" string in 
my validate_parent_available() function (see below).


I will be submitting other peripheral FIFOs with EDAC protection in 
future patches (USB, DMA, etc).


static int validate_parent_available(struct device_node *np)
{
struct device_node *parent;
int ret = 0;

/* Ensure parent device is enabled if parent node exists */
parent = of_parse_phandle(np, "parent", 0);
if (parent && !of_device_is_available(parent))
ret = -ENODEV;
of_node_put(parent);
return ret;
}


I can change this to using a passed in data string but the code won't be 
as straightforward.


Thanks for reviewing,

Thor

Re: [PATCHv4 4/7] Documentation: dt: socfpga: Add Arria10 Ethernet binding

[Thor Thayer]

On 06/21/2016 10:48 AM, Rob Herring wrote:

On Tue, Jun 21, 2016 at 9:46 AM, Thor Thayer
<ttha...@opensource.altera.com> wrote:

Hi Rob,


On 06/21/2016 08:33 AM, Rob Herring wrote:


On Mon, Jun 20, 2016 at 09:50:49AM -0500, ttha...@opensource.altera.com
wrote:


From: Thor Thayer <ttha...@opensource.altera.com>

Add the device tree bindings needed to support the Altera Ethernet
FIFO buffers on the Arria10 chip.

Signed-off-by: Thor Thayer <ttha...@opensource.altera.com>
---
v2  No Change
v3  Change to common compatible string based on maintainer comments
  Add local IRQ values.
v4  Add compatible string for parent node.
---
   .../bindings/arm/altera/socfpga-eccmgr.txt |   24

   1 file changed, 24 insertions(+)

diff --git
a/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
b/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
index 15eb0df..7c714ba 100644
--- a/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
@@ -82,6 +82,14 @@ Required Properties:
   - interrupts : Should be single bit error interrupt, then double bit
error
 interrupt, in this order.

+Ethernet FIFO ECC
+Required Properties:
+- compatible : Should be "altr,socfpga-eth-mac-ecc"
+- reg: Address and size for ECC block registers.
+- parent : phandle to parent (altr,socfpga-stmmac) Ethernet node.



Sorry if I wasn't clear before, but I was suggesting changing 'parent'
to 'altr,ethernet-mac':

altr,ethernet-mac = <>;

Rob


Ahh, I see what you're saying.

I used parent as the tag because I have a generic function for validating
that the parent status is "okay" using the "parent" string in my
validate_parent_available() function (see below).


Ah, so common ecc-mgr code is parsing it. Then how about 'altr,ecc-parent'?

Rob


That's clean - I'll change to that string. Thanks!

Re: [PATCHv9 1/4] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support

[Thor Thayer]

Hi Boris.

On 02/08/2016 05:39 AM, Borislav Petkov wrote:

On Wed, Jan 27, 2016 at 10:13:20AM -0600, ttha...@opensource.altera.com wrote:

From: Thor Thayer <ttha...@opensource.altera.com>

Adding L2 Cache and On-Chip RAM EDAC support for the
Altera SoCs using the EDAC device  model. The SDRAM
controller is using the Memory Controller model.

Each type of ECC is individually configurable.

Signed-off-by: Thor Thayer <ttha...@opensource.altera.com>
---
v9: Improve device tree node release. Free managed resources
 on error path. Fix ocram memory leak.
v8: Remove MASK from single bit mask names.
 s/altr,edac/altr,socfpga-ecc-manager
 Use debugfs instead of sysfs.
 Add chip family name to match string.
 Fix header year.
 Fix build dependencies & change commit accordingly.
 s/CONFIG_EDAC_ALTERA_MC/CONFIG_EDAC_ALTERA
v7: s/of_get_named_gen_pool/of_gen_pool_get
 Remove #ifdef for EDAC_DEBUG
 Use -ENODEV instead of EPROBE_DEFER
v6: Convert to nested EDAC in device tree. Force L2 cache
 on for L2Cache ECC & remove L2 cache syscon for checking
 enable bit. Update year in header.
v5: No change.
v4: Change mask defines to use BIT().
 Fix comment style to agree with kernel coding style.
 Better printk description for read != write in trigger.
 Remove SysFS debugging message.
 Better dci->mod_name
 Move gen_pool pointer assignment to end of function.
 Invert logic to reduce indent in ocram depenency check.
 Change from dev_err() to edac_printk()
 Replace magic numbers with defines & comments.
 Improve error injection test.
 Change Makefile intermediary name to altr (from alt)
v3: Move OCRAM and L2 cache EDAC functions into altera_edac.c
 instead of separate files.
v2: Fix L2 dependency comments.
---
  drivers/edac/Kconfig   |   26 ++-
  drivers/edac/Makefile  |2 +-
  drivers/edac/altera_edac.c |  487 +++-
  3 files changed, 507 insertions(+), 8 deletions(-)


I'm still waiting for the people on CC to confirm the DT changes. A
couple of comments on the EDAC bits below.




Understood. I did get a conditional ACK from Rob Herring on the DT 
portion of the patch from the last revision (as long as I made the 
changes he suggested which I did in this patch). There may be other 
comments though.







+static irqreturn_t altr_edac_device_handler(int irq, void *dev_id)
+{
+   struct edac_device_ctl_info *dci = dev_id;
+   struct altr_edac_device_dev *drvdata = dci->pvt_info;
+   const struct edac_device_prv_data *priv = drvdata->data;
+
+   if (irq == drvdata->sb_irq) {
+   if (priv->ce_clear_mask)
+   writel(priv->ce_clear_mask, drvdata->base);
+   edac_device_handle_ce(dci, 0, 0, drvdata->edac_dev_name);
+   }
+   if (irq == drvdata->db_irq) {
+   if (priv->ue_clear_mask)
+   writel(priv->ue_clear_mask, drvdata->base);
+   edac_device_handle_ue(dci, 0, 0, drvdata->edac_dev_name);
+   panic("\nEDAC:ECC_DEVICE[Uncorrectable errors]\n");
+   }


And irq is guaranteed to always be ->sb_irq or ->db_irq?

Otherwise, you can do

else
WARN_ON(1);

just in case.


Those are the only cases of irq but it would be good to be alerted if 
that is not the case. I will add. Thanks!





+
+   return IRQ_HANDLED;
+}


...


+/*
+ * altr_edac_device_probe()
+ * This is a generic EDAC device driver that will support
+ * various Altera memory devices such as the L2 cache ECC and
+ * OCRAM ECC as well as the memories for other peripherals.
+ * Module specific initialization is done by passing the
+ * function index in the device tree.
+ */
+static int altr_edac_device_probe(struct platform_device *pdev)
+{





+
+   dci->mod_name = "Altera ECC Manager";
+   dci->dev_name = drvdata->edac_dev_name;
+
+   debugfs = edac_debugfs_create_dir(ecc_name);
+   if (debugfs)
+   altr_create_edacdev_dbgfs(dci, drvdata->data, debugfs);
+
+   if (edac_device_add_device(dci))


<--- if you end up here and debugfs nodes have been created, you need to
destroy them here. You probably could change edac_debugfs_exit() to call
debugfs_remove_recursive() and make sure your driver calls it.

Right now we're calling edac_debugfs_exit() in edac_exit() and I *think*
that is ok for a platform device driver as this one but I haven't
actually *verified* that.


Yes, thanks. I was using the xgene code as an example but I missed the 
unregister (although it looks like the xgene's unregister affects sysfs 
instead of debugfs). I'm also moving the debugfs creation to the end of 
the probe since it is not critical and avoids an error path if creation 
fails.


I'll make the debugfs_remove_recursive() change as a sepa

Re: [PATCHv8 1/4] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support

[Thor Thayer]

Hi Vladimir,


On 01/22/2016 12:02 AM, Vladimir Zapolskiy wrote:

Hi Thor,

On 21.01.2016 19:34, ttha...@opensource.altera.com wrote:

From: Thor Thayer <ttha...@opensource.altera.com>

Adding L2 Cache and On-Chip RAM EDAC support for the
Altera SoCs using the EDAC device  model. The SDRAM
controller is using the Memory Controller model.

Each type of ECC is individually configurable.

Signed-off-by: Thor Thayer <ttha...@opensource.altera.com>
Signed-off-by: Dinh Nguyen <dingu...@opensource.altera.com>


You are sending a change authored by yourself for review, but you add Dinh's
SoB, what's his role here?

See Documentation/SubmittingPatches "Sign your work".

[snip]


While I was working in a different group at Altera last year, Dinh 
submitted the previous patch revision so I kept his signed off by for 
continuity. I'll just use myself in the future. Thank you for clarifying.





+/*
+ * altr_edac_device_probe()
+ * This is a generic EDAC device driver that will support
+ * various Altera memory devices such as the L2 cache ECC and
+ * OCRAM ECC as well as the memories for other peripherals.
+ * Module specific initialization is done by passing the
+ * function index in the device tree.
+ */
+static int altr_edac_device_probe(struct platform_device *pdev)
+{
+   struct edac_device_ctl_info *dci;
+   struct altr_edac_device_dev *drvdata;
+   struct resource *r;
+   int res = 0;
+   struct device_node *np = pdev->dev.of_node;
+   char *ecc_name = (char *)np->name;
+   static int dev_instance;
+   struct dentry *debugfs;
+
+   if (!devres_open_group(>dev, NULL, GFP_KERNEL)) {
+   edac_printk(KERN_ERR, EDAC_DEVICE,
+   "Unable to open devm\n");
+   return -ENOMEM;
+   }
+
+   r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+   if (!r) {
+   edac_printk(KERN_ERR, EDAC_DEVICE,
+   "Unable to get mem resource\n");


Missing devres_release_group(>dev, NULL) on error path.


Yes. Thank you.


+   return -ENODEV;
+   }
+
+   if (!devm_request_mem_region(>dev, r->start, resource_size(r),
+dev_name(>dev))) {
+   edac_printk(KERN_ERR, EDAC_DEVICE,
+   "%s:Error requesting mem region\n", ecc_name);


See above.


+   return -EBUSY;
+   }
+
+   dci = edac_device_alloc_ctl_info(sizeof(*drvdata), ecc_name,
+1, ecc_name, 1, 0, NULL, 0,
+dev_instance++);
+
+   if (!dci) {
+   edac_printk(KERN_ERR, EDAC_DEVICE,
+   "%s: Unable to allocate EDAC device\n", ecc_name);


See above.


+   return -ENOMEM;
+   }
+
+   drvdata = dci->pvt_info;
+   dci->dev = >dev;
+   platform_set_drvdata(pdev, dci);
+   drvdata->edac_dev_name = ecc_name;
+
+   drvdata->base = devm_ioremap(>dev, r->start, resource_size(r));
+   if (!drvdata->base)
+   goto err;
+
+   /* Get driver specific data for this EDAC device */
+   drvdata->data = of_match_node(altr_edac_device_of_match, np)->data;
+
+   /* Check specific dependencies for the module */
+   if (drvdata->data->setup) {
+   res = drvdata->data->setup(pdev, drvdata->base);
+   if (res < 0)
+   goto err;
+   }
+
+   drvdata->sb_irq = platform_get_irq(pdev, 0);
+   res = devm_request_irq(>dev, drvdata->sb_irq,
+  altr_edac_device_handler,
+  0, dev_name(>dev), dci);
+   if (res < 0)
+   goto err;
+
+   drvdata->db_irq = platform_get_irq(pdev, 1);
+   res = devm_request_irq(>dev, drvdata->db_irq,
+  altr_edac_device_handler,
+  0, dev_name(>dev), dci);
+   if (res < 0)
+   goto err;
+
+   dci->mod_name = "Altera ECC Manager";
+   dci->dev_name = drvdata->edac_dev_name;
+
+   debugfs = edac_debugfs_create_dir(ecc_name);
+   if (debugfs)
+   altr_create_edacdev_dbgfs(dci, drvdata->data, debugfs);
+
+   if (edac_device_add_device(dci))
+   goto err;
+
+   devres_close_group(>dev, NULL);
+
+   return 0;
+err:
+   edac_printk(KERN_ERR, EDAC_DEVICE,
+   "%s:Error setting up EDAC device: %d\n", ecc_name, res);
+   devres_release_group(>dev, NULL);
+   edac_device_free_ctl_info(dci);
+
+   return res;
+}
+
+static int altr_edac_device_remove(struct platform_device *pdev)
+{
+   struct edac_device_ctl_info *dci = platform_get_drvdata(pdev);
+
+   edac_d

Re: [PATCHv8 1/4] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC Support

[Thor Thayer]

On 01/22/2016 12:08 PM, Borislav Petkov wrote:

On Fri, Jan 22, 2016 at 06:56:57PM +0200, Vladimir Zapolskiy wrote:

it sounds like the author of the original change is Dinh, but if you agreed
about authorship transfer, then "From: Thor Thayer" statement should be
correct, but in any case your SoB should follow Dinh's SoB, if you decide to
keep the latter one.

This consideration may apply to the other changes in the changeset as well.


So the patch author should be in the From:

If Thor has changed the original patch considerably, then you Thor and
Dinh could decide amongst each other who should be the author.

If Thor becomes the author and lands in From:, then the commit message
could state something like "based on original work from Dinh" or
"Originally-from: Dinh" and so on. "git log" has some examples.

The SOB chain shows who handled the patch on its way upstream. So in
this case, it should be:

SOB: Dinh (if From: is Dinh - otherwise Originally-by:)
SOB: Thor
SOB: Boris

if I'm going to pick it up and send it to Linus.

Ok?

OK. Thank you for the explanation, it is much clearer now. I apologize 
for creating this confusion.


I was the original author for the previous versions of patches (1-6) but 
since I was out for a portion of last year, Dinh picked up version 7 and 
submitted it.


Since I'm taking it back over, Dinh and I agreed that I'll be the only 
signoff from now on. Additionally, I'll make sure I'm the From: going 
forward.


Thor

Re: [PATCHv8 2/4] ARM: dts: Add Altera L2 Cache and OCRAM EDAC entries

[Thor Thayer]

On 01/22/2016 08:35 PM, Rob Herring wrote:

On Thu, Jan 21, 2016 at 11:34:26AM -0600, ttha...@opensource.altera.com wrote:

From: Thor Thayer <ttha...@opensource.altera.com>

Adding the device tree entries and bindings needed to support
the Altera L2 cache and On-Chip RAM EDAC. This patch relies upon
an earlier patch to declare and setup On-chip RAM properly.
http://www.spinics.net/lists/devicetree/msg51117.html

Signed-off-by: Thor Thayer <ttha...@opensource.altera.com>
Signed-off-by: Dinh Nguyen <dingu...@opensource.altera.com>
---
v8: Fix node names to include chip family and use ecc manager
 to better describe the driver. Rename socfpga-edac.txt to
 socfpga-eccmgr.txt.
v7: No Change
v6: Change to nested EDAC device nodes based on community
 feedback. Remove L2 syscon. Use consolidated binding.
v3-5: No Change
v2: Remove OCRAM declaration and reference prior patch.
---
  .../bindings/arm/altera/socfpga-eccmgr.txt |   49 
  arch/arm/boot/dts/socfpga.dtsi |   20 
  2 files changed, 69 insertions(+)
  create mode 100644 
Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt


A couple of nits, otherwise:

Acked-by: Rob Herring <r...@kernel.org>



Great! I will make these changes for my next version that fixes some 
coding issue. Thank you for reviewing.




diff --git a/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt 
b/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
new file mode 100644
index 000..4f45690
--- /dev/null
+++ b/Documentation/devicetree/bindings/arm/altera/socfpga-eccmgr.txt
@@ -0,0 +1,49 @@
+Altera SoCFPGA ECC Manager
+This driver uses the EDAC framework to implement the SOCFPGA ECC Manager.
+The ECC Manager counts and corrects single bit errors and counts/handles
+double bit errors which are uncorrectable.
+
+Required Properties:
+- compatible : Should be "altr,socfpga-ecc-manager"
+- #address-cells: must be 1
+- #size-cells: must be 1
+- ranges : standard definition, should translate from local addresses
+
+Subcomponents:
+
+L2 Cache ECC
+Required Properties:
+- compatible : Should be "altr,socfpga-l2-ecc"
+- reg : Address and size for ECC error interrupt clear registers.
+- interrupts : Should be single bit error interrupt, then double bit error
+   interrupt. Note the rising edge type.
+
+On Chip RAM ECC
+Required Properties:
+- compatible : Should be "altr,socfpga-ocram-ecc"
+- reg : Address and size for ECC error interrupt clear registers.
+- iram : phandle to On-Chip RAM definition.
+- interrupts : Should be single bit error interrupt, then double bit error
+   interrupt. Note the rising edge type.
+
+Example:
+
+   eccmgr: eccmgr@0xffd08140 {


drop the '0x'


+   compatible = "altr,socfpga-ecc-manager";
+   #address-cells = <1>;
+   #size-cells = <1>;
+   ranges;
+
+   l2-ecc@ffd08140 {
+   compatible = "altr,socfpga-l2-ecc";
+   reg = <0xffd08140 0x4>;
+   interrupts = <0 36 1>, <0 37 1>;
+   };
+
+   ocram-ecc@ffd08144 {
+   compatible = "altr,socfpga-ocram-ecc";
+   reg = <0xffd08144 0x4>;
+   iram = <>;
+   interrupts = <0 178 1>, <0 179 1>;
+   };
+   };
diff --git a/arch/arm/boot/dts/socfpga.dtsi b/arch/arm/boot/dts/socfpga.dtsi
index 39c470e..9bb383e 100644
--- a/arch/arm/boot/dts/socfpga.dtsi
+++ b/arch/arm/boot/dts/socfpga.dtsi
@@ -656,6 +656,26 @@
status = "disabled";
};

+   eccmgr: eccmgr@0xffd08140 {


and here.


+   compatible = "altr,socfpga-ecc-manager";
+   #address-cells = <1>;
+   #size-cells = <1>;
+   ranges;
+
+   l2-ecc@ffd08140 {
+   compatible = "altr,socfpga-l2-ecc";
+   reg = <0xffd08140 0x4>;
+   interrupts = <0 36 1>, <0 37 1>;
+   };
+
+   ocram-ecc@ffd08144 {
+   compatible = "altr,socfpga-ocram-ecc";
+   reg = <0xffd08144 0x4>;
+   iram = <>;
+   interrupts = <0 178 1>, <0 179 1>;
+   };
+   };
+
L2: l2-cache@fffef000 {
compatible = "arm,pl310-cache";
reg = <0xfffef000 0x1000>;
--
1.7.9.5


___
linux-arm-kernel mailing list
linux-arm-ker...@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCHv2 6/7] ARM: socfpga: Enable Arria10 OCRAM ECC on startup

[Thor Thayer]

Hi Boris,

On 04/05/2016 12:31 AM, Borislav Petkov wrote:

On Tue, Apr 05, 2016 at 12:25:33AM -0500, Thor Thayer wrote:

I realize that I'm not calling iounmap(ecc_block_base) and I'll fix that in
the next revision with a goto.


I'm assuming nothing else changes. Because I've applied 1-4 already.

Yes, no?

If no, then please send only an updated version of this patch as a reply
to this thread here.

Thanks.


Yes, nothing else changes. The rest was OK. Thanks!

Re: [PATCHv2 11/11] ARM: dts: Add Altera Arria10 L2 Cache EDAC devicetree entry

[Thor Thayer]

Hi Dinh,

On 03/08/2016 08:50 AM, Dinh Nguyen wrote:



On 03/07/2016 01:43 PM, ttha...@opensource.altera.com wrote:

From: Thor Thayer <ttha...@opensource.altera.com>

Add the device tree entries needed to support the Altera L2
cache EDAC on the Arria10 chip.

Signed-off-by: Thor Thayer <ttha...@opensource.altera.com>
---
v2 Match register value (l2-ecc@ffd06010)
---
  arch/arm/boot/dts/socfpga_arria10.dtsi |   14 ++
  1 file changed, 14 insertions(+)

diff --git a/arch/arm/boot/dts/socfpga_arria10.dtsi 
b/arch/arm/boot/dts/socfpga_arria10.dtsi
index cce9e50..44aeb3f 100644
--- a/arch/arm/boot/dts/socfpga_arria10.dtsi
+++ b/arch/arm/boot/dts/socfpga_arria10.dtsi
@@ -599,6 +599,20 @@
reg = <0xffe0 0x4>;
};

+   eccmgr: eccmgr@ffd06090 {
+   compatible = "altr,socfpga-ecc-manager";
+   #address-cells = <1>;
+   #size-cells = <1>;
+   ranges;
+
+   l2-ecc@ffd06010 {
+   compatible = "altr,socfpga-a10-l2-ecc";
+   reg = <0xffd06010 0x4>;
+   interrupts = <0 2 IRQ_TYPE_LEVEL_HIGH>,
+<0 0 IRQ_TYPE_LEVEL_HIGH>;
+   };
+   };
+


Just checking if these addresses are correct. The eccmgr is at
0xffd06090, but the l2-ecc is at 0xffd06010? I would have thought from
the placement the l2-ecc address would be inside the eccmgr's address?

Dinh



Yes, this is confusing and I'll clarify/reorganize in the next series. 
The eccmgr is pointing to the ECC IRQ mask bits. These registers and the 
L2 ECC registers are organized in different areas within the system manager.


I'm actually redoing the series since the Arria10 IRQ handling is 
significantly different.


Since this change will affect the bindings and dti (the eccmgr will have 
the IRQs), please disregard this series.


Sorry for the noise.

Thor

Re: [PATCHv2 07/11] EDAC, altera: Add status offset & masks

[Thor Thayer]

Hi Boris,

On 03/07/2016 01:43 PM, ttha...@opensource.altera.com wrote:

From: Thor Thayer <ttha...@opensource.altera.com>

In preparation for the Arria10 peripheral ECCs, the IRQ
status needs to be determined because the IRQs are shared.
The IRQ status register is read to determine if the IRQ
was for this ECC peripheral. Cyclone5 and Arria5 have
dedicated IRQs so the confirmation mechanism is not
required and the mask is set to 0.

Signed-off-by: Thor Thayer <ttha...@opensource.altera.com>
---
v2: Split large patch into smaller patches. Determine
 if IRQ matches this ECC peripheral before handling it.
---
  drivers/edac/altera_edac.c |   41 +++--
  drivers/edac/altera_edac.h |3 +++
  2 files changed, 34 insertions(+), 10 deletions(-)

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index fd73a77..11b7291 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -556,19 +556,32 @@ static irqreturn_t altr_edac_device_handler(int irq, void 
*dev_id)
struct edac_device_ctl_info *dci = dev_id;
struct altr_edac_device_dev *drvdata = dci->pvt_info;
const struct edac_device_prv_data *priv = drvdata->data;
+   void __iomem *status_addr = drvdata->status + priv->err_status_ofst;
void __iomem *clear_addr = drvdata->status + priv->clear_err_ofst;

+   /*
+* CycloneV is directly mapped to a specific IRQ. Arria10
+* shares the IRQ with other ECCs so we must match first.
+*/
if (irq == drvdata->sb_irq) {
-   if (priv->ce_clear_mask)
-   writel(priv->ce_clear_mask, clear_addr);
-   edac_device_handle_ce(dci, 0, 0, drvdata->edac_dev_name);
-   ret_value = IRQ_HANDLED;
+   if (!priv->ce_status_mask ||
+   (priv->ce_status_mask & readl(status_addr))) {
+   if (priv->ce_clear_mask)
+   writel(priv->ce_clear_mask, clear_addr);
+   edac_device_handle_ce(dci, 0, 0,
+ drvdata->edac_dev_name);
+   ret_value = IRQ_HANDLED;
+   }
} else if (irq == drvdata->db_irq) {
-   if (priv->ue_clear_mask)
-   writel(priv->ue_clear_mask, clear_addr);
-   edac_device_handle_ue(dci, 0, 0, drvdata->edac_dev_name);
-   panic("\nEDAC:ECC_DEVICE[Uncorrectable errors]\n");
-   ret_value = IRQ_HANDLED;
+   if (!priv->ue_status_mask ||
+   (priv->ue_status_mask & readl(status_addr))) {
+   if (priv->ue_clear_mask)
+   writel(priv->ue_clear_mask, clear_addr);
+   edac_device_handle_ue(dci, 0, 0,
+ drvdata->edac_dev_name);
+   panic("\nEDAC:ECC_DEVICE[Uncorrectable errors]\n");
+   ret_value = IRQ_HANDLED;
+   }
} else {
WARN_ON(1);
}



While working on subsequent ECC components to upstream, I realized that 
the above is not an optimal solution for Arria10.


The Arria10 is significantly different from the Cyclone5/Arria5 and 
therefore should be it's own implementation.


Please disregard this patch series. I'll redo the series with a 
different IRQ implementation that is cleaner - it will be closer to the 
Xgene driver.


Sorry for the noise.

Thor



@@ -882,6 +895,10 @@ const struct edac_device_prv_data ocramecc_data = {
.ce_clear_mask = (ALTR_OCR_ECC_EN | ALTR_OCR_ECC_SERR),
.ue_clear_mask = (ALTR_OCR_ECC_EN | ALTR_OCR_ECC_DERR),
.clear_err_ofst = ALTR_OCR_ECC_REG_OFFSET,
+   /* Cyclone5 & Arria5 have separate IRQs so status = 0 */
+   .ce_status_mask = 0,
+   .ue_status_mask = 0,
+   .err_status_ofst = 0,
.dbgfs_name = "altr_ocram_trigger",
.alloc_mem = ocram_alloc_mem,
.free_mem = ocram_free_mem,
@@ -957,7 +974,11 @@ const struct edac_device_prv_data l2ecc_data = {
.setup = altr_l2_check_deps,
.ce_clear_mask = 0,
.ue_clear_mask = 0,
-   .clear_err_ofst = ALTR_L2_ECC_REG_OFFSET,
+   .clear_err_ofst = ALTR_MAN_GRP_L2_ECC_OFFSET,
+   /* Cyclone5 & Arria5 have separate IRQs so status = 0 */
+   .ce_status_mask = 0,
+   .ue_status_mask = 0,
+   .err_status_ofst = 0,
.dbgfs_name = "altr_l2_trigger",
.alloc_mem = l2_alloc_mem,
.free_mem = l2_free_mem,
diff --git a/drivers/edac/altera_edac.h b/drivers/edac/altera_edac.h
index b262f74..43e0dae 100644
--- a/drivers/edac/altera_edac.h
+++ b/drivers/edac/altera_edac.h
@@ -226,6 +226,9 @@ struct edac_device_prv_data {
int ce_clear_mask;
int 

Re: [PATCH 3/5] EDAC, altera: Addition of Arria10 L2 Cache ECC

[Thor Thayer]

Hi Boris,

On 03/04/2016 04:38 AM, Borislav Petkov wrote:

On Tue, Mar 01, 2016 at 10:38:19AM -0600, ttha...@opensource.altera.com wrote:

From: Thor Thayer <ttha...@opensource.altera.com>

Addition of the Arria10 L2 Cache ECC handling. The major
changes affect the L2 ECC registers not being grouped
together. The Arria10 IRQ status needs to be mapped into
a different region. The mapping occurs in the L2 specific
function.
Important changes include:



1) Move private data structure definition to altera_edac.h
2) Move Cyclone5 device defines to altera_edac.h


This should be a separate patch.


3) Split IRQ status and ECC enable/control into separate
memory areas.


Ditto.


4) Add IRQ status mapping in L2 ECC dependency checks
function.


Ditto...


5) Addition of register offsets in private data structure.
6) Changes to code to use register offset define.
7) Addition of Arria10 L2 cache private data.
8) Add IRQ flags to indicate Exclusive/Shared.


Do you see where I'm going with this?

Each patch should countain one logical change: add defines and move
struct, change functionality A, change functionality B, ...

The fact that you have to make a list of 8 important changes should
already give you a hint that it needs to be split.

As always, I'm going to need ACKs for the ARM stuff.



OK. I'll split up the changes and resubmit. Thanks!

Re: [PATCHv2 6/7] ARM: socfpga: Enable Arria10 OCRAM ECC on startup

[Thor Thayer]

Hi,

On 03/31/2016 01:48 PM, ttha...@opensource.altera.com wrote:

From: Thor Thayer <ttha...@opensource.altera.com>

Enable ECC for Arria10 On-Chip RAM on machine startup. The ECC has to
be enabled and memory initialized before data is stored in memory
otherwise the ECC will fail on reads.

Signed-off-by: Thor Thayer <ttha...@opensource.altera.com>
---
v2: Add Arria10 ECC block initialization locally.
---
  arch/arm/mach-socfpga/ocram.c |  128 +
  1 file changed, 128 insertions(+)

diff --git a/arch/arm/mach-socfpga/ocram.c b/arch/arm/mach-socfpga/ocram.c
index 60ec643..d4a524c 100644
--- a/arch/arm/mach-socfpga/ocram.c
+++ b/arch/arm/mach-socfpga/ocram.c
@@ -13,12 +13,15 @@
   * You should have received a copy of the GNU General Public License along 
with
   * this program.  If not, see <http://www.gnu.org/licenses/>.
   */
+#include 
  #include 
  #include 
  #include 
  #include 
  #include 

+#include "core.h"
+
  #define ALTR_OCRAM_CLEAR_ECC  0x0018
  #define ALTR_OCRAM_ECC_EN 0x0019

@@ -47,3 +50,128 @@ void socfpga_init_ocram_ecc(void)

iounmap(mapped_ocr_edac_addr);
  }
+
+/* Arria10 OCRAM Section */
+#define ALTR_A10_ECC_CTRL_OFST  0x08
+#define ALTR_A10_OCRAM_ECC_EN_CTL   (BIT(1) | BIT(0))
+#define ALTR_A10_ECC_INITA  BIT(16)
+
+#define ALTR_A10_ECC_INITSTAT_OFST  0x0C
+#define ALTR_A10_ECC_INITCOMPLETEA  BIT(0)
+#define ALTR_A10_ECC_INITCOMPLETEB  BIT(8)
+
+#define ALTR_A10_ECC_ERRINTEN_OFST  0x10
+#define ALTR_A10_ECC_SERRINTEN  BIT(0)
+
+#define ALTR_A10_ECC_INTSTAT_OFST   0x20
+#define ALTR_A10_ECC_SERRPENA   BIT(0)
+#define ALTR_A10_ECC_DERRPENA   BIT(8)
+#define ALTR_A10_ECC_ERRPENA_MASK   (ALTR_A10_ECC_SERRPENA | \
+ALTR_A10_ECC_DERRPENA)
+/* ECC Manager Defines */
+#define A10_SYSMGR_ECC_INTMASK_SET_OFST   0x94
+#define A10_SYSMGR_ECC_INTMASK_CLR_OFST   0x98
+#define A10_SYSMGR_ECC_INTMASK_OCRAM  BIT(1)
+
+#define ALTR_A10_ECC_INIT_WATCHDOG_10US   1
+
+static void ecc_set_bits(u32 bit_mask, void __iomem *ioaddr)
+{
+   u32 value = readl(ioaddr);
+
+   value |= bit_mask;
+   writel(value, ioaddr);
+}
+
+static void ecc_clear_bits(u32 bit_mask, void __iomem *ioaddr)
+{
+   u32 value = readl(ioaddr);
+
+   value &= ~bit_mask;
+   writel(value, ioaddr);
+}
+
+static int ecc_test_bits(u32 bit_mask, void __iomem *ioaddr)
+{
+   u32 value = readl(ioaddr);
+
+   return (value & bit_mask) ? 1 : 0;
+}
+
+/*
+ * This function uses the memory initialization block in the Arria10 ECC
+ * controller to initialize/clear the entire memory data and ECC data.
+ */
+static int altr_init_memory_port(void __iomem *ioaddr)
+{
+   int limit = ALTR_A10_ECC_INIT_WATCHDOG_10US;
+
+   ecc_set_bits(ALTR_A10_ECC_INITA, (ioaddr + ALTR_A10_ECC_CTRL_OFST));
+   while (limit--) {
+   if (ecc_test_bits(ALTR_A10_ECC_INITCOMPLETEA,
+ (ioaddr + ALTR_A10_ECC_INITSTAT_OFST)))
+   break;
+   udelay(1);
+   }
+   if (limit < 0)
+   return -EBUSY;
+
+   /* Clear any pending ECC interrupts */
+   writel(ALTR_A10_ECC_ERRPENA_MASK,
+  (ioaddr + ALTR_A10_ECC_INTSTAT_OFST));
+
+   return 0;
+}
+
+void socfpga_init_arria10_ocram_ecc(void)
+{
+   struct device_node *np;
+   int ret = 0;
+   void __iomem *ecc_block_base;
+
+   if (!sys_manager_base_addr) {
+   pr_err("SOCFPGA: sys-mgr is not initialized\n");
+   return;
+   }
+
+   /* Find the OCRAM EDAC device tree node */
+   np = of_find_compatible_node(NULL, NULL, "altr,socfpga-a10-ocram-ecc");
+   if (!np) {
+   pr_err("Unable to find socfpga-a10-ocram-ecc\n");
+   return;
+   }
+
+   /* Map the ECC Block */
+   ecc_block_base = of_iomap(np, 0);
+   of_node_put(np);
+   if (!ecc_block_base) {
+   pr_err("Unable to map OCRAM ECC block\n");
+   return;
+   }
+
+   /* Disable ECC */
+   writel(ALTR_A10_OCRAM_ECC_EN_CTL,
+  sys_manager_base_addr + A10_SYSMGR_ECC_INTMASK_SET_OFST);
+   ecc_clear_bits(ALTR_A10_ECC_SERRINTEN,
+  (ecc_block_base + ALTR_A10_ECC_ERRINTEN_OFST));
+   ecc_clear_bits(ALTR_A10_OCRAM_ECC_EN_CTL,
+  (ecc_block_base + ALTR_A10_ECC_CTRL_OFST));
+
+   /* Use HW initialization block to initialize memory for ECC */
+   ret = altr_init_memory_port(ecc_block_base);
+   if (ret) {
+   pr_err("ECC: cannot init OCRAM PORTA memory\n");
+   return;


I realize that I'm not calling iounmap(ecc_block_base) and I'll fix that 
in the next revision with a goto.



+   }
+
+   /* Enable EC

Re: [PATCH 6/7] ARM: socfpga: Enable Arria10 OCRAM ECC on startup

[Thor Thayer]

Hi Dinh,

On 03/30/2016 12:11 PM, Dinh Nguyen wrote:

On Wed, 30 Mar 2016, ttha...@opensource.altera.com wrote:


From: Thor Thayer <ttha...@opensource.altera.com>

Enable ECC for Arria10 On-Chip RAM on machine startup. The ECC has to be
enabled before data is stored in memory otherwise the ECC will fail
on reads.

Signed-off-by: Thor Thayer <ttha...@opensource.altera.com>
---


[snip]


+
+void socfpga_init_arria10_ocram_ecc(void)
+{
+   struct device_node *np;
+   int ret;
+
+   /* Find the OCRAM EDAC device tree node */
+   np = of_find_compatible_node(NULL, NULL, "altr,socfpga-a10-ocram-ecc");
+   if (!np) {
+   pr_err("Unable to find socfpga-a10-ocram-ecc\n");
+   return;
+   }
+
+   ret = altr_init_a10_ecc_block(np, A10_SYSMGR_ECC_INTSTAT_OCRAM,
+ ALTR_A10_OCRAM_ECC_EN_CTL, 0);


I think this is a no-no, you shouldn't be making a call directly into the driver
from here.

BR,
Dinh


OK. I'll make the initialization local and resubmit. Thanks for reviewing.

Re: [PATCH] Add EDAC peripheral init functions & Ethernet EDAC.

[Thor Thayer]

On 04/15/2016 04:40 AM, Mauro Carvalho Chehab wrote:

Em Thu, 14 Apr 2016 09:35:01 -0500
Rob Herring  escreveu:


On Tue, Apr 12, 2016 at 05:12:55PM -0500, ttha...@opensource.altera.com wrote:

This patch set adds the memory initialization functions for Altera's
Arria10 peripherals, the first of which is the Ethernet EDAC. The
first 3 patches add the memory initialization functionality. The
last 3 patches add Ethernet EDAC support.


The ethernet part seems a bit strange to me to put under EDAC as EDAC
is primarily memory controller ECC (and caches to some extent). Also you
would not halt the system in case of an UC, but rather just drop the
frame. This would need to be part of the ethernet driver in that case.

Of course, given that ethernet frames already have a CRC, ECC of the
FIFO seems a bit redundant.


Actually, EDAC was conceived to be a way to report hardware errors, and,
although the main use case is for memory and CPU errors, there are a few
drivers that report errors at PCI bus. So, I don't see much problems using
it to report other hardware errors, like the ones associated with the
Ethernet hardware.

That's said, things like Ethernet frame errors are better handled via the
network drivers. I would report via EDAC only errors associated with the
Ethernet hardware that would cause the hardware to malfunction.

Btw, an UC error won't cause the system to halt, except if a UC memory
error happens and the EDAC core is loaded with an special modprobe
parameter (edac_mc_panic_on_ue = 1).

Thank you for the clarification. Rob's comment was logical and made me 
re-think this. He pointed out that I was causing a kernel panic in the 
case of Uncorrectable errors which is not the desired response and will 
need to change.


I'll update this patch to only count errors. I'll need to re-think how 
the network driver can be alerted that there was an uncorrectable error 
but that could be a later patch.


Great feedback. Thank you Mauro and Rob for reviewing and commenting!

Re: [PATCH] Add EDAC peripheral init functions & Ethernet EDAC.

[Thor Thayer]

On 04/18/2016 03:02 PM, Borislav Petkov wrote:

On Mon, Apr 18, 2016 at 09:27:16AM -0500, Thor Thayer wrote:

We're still getting the single bit correction


By that you mean, you get that by enabling ECC on the FIFO block?

Yes, you are correct. I'd still get the single bit correction by 
enabling ECC on the FIFO which is a win.



which makes the entire system more stable and the ability to see both
single bit errors corrected and the number of uncorrectable errors is
useful from a system point of view.


If so, that makes sense, yes.

Thanks.

Re: [PATCH] Add EDAC peripheral init functions & Ethernet EDAC.

[Thor Thayer]

On 04/18/2016 03:06 PM, Borislav Petkov wrote:

On Mon, Apr 18, 2016 at 10:02:27PM +0200, Borislav Petkov wrote:

  the number of uncorrectable errors is useful from a system point of
view.


I forgot: so altr_edac_a10_ecc_irq() panics on uncorrectable errors. Do we want
to do that even for UEs coming from the network...?

Seems a bit overboard to me...



Yes, as currently submitted, it is overboard and I'll fix this in the 
next patch submission. In the uncorrectable error case, I plan to just 
count the error. I'll add a constant flag in the Ethernet module's 
edac_device_prv_data to determine if the panic should be bypassed.

Re: [PATCH 1/2] EDAC, altera: remove useless casts

[Thor Thayer]

On 04/16/2016 03:13 PM, Arnd Bergmann wrote:

The altera EDAC driver refers to its per-device data
using a cast to '(void *)', which makes the pointer
non-const, though both the source and destination are
actually const.

Removing the annotation makes the reference (almost)
fit into a single line for improved readability, and
ensures that it is actually defined as const.

Signed-off-by: Arnd Bergmann <a...@arndb.de>
---
  drivers/edac/altera_edac.c | 15 ++-
  1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index 11775dc0b139..cc987b4ce908 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -232,8 +232,8 @@ static unsigned long get_total_mem(void)
  }

  static const struct of_device_id altr_sdram_ctrl_of_match[] = {
-   { .compatible = "altr,sdram-edac", .data = (void *)_data},
-   { .compatible = "altr,sdram-edac-a10", .data = (void *)_data},
+   { .compatible = "altr,sdram-edac", .data = _data},
+   { .compatible = "altr,sdram-edac-a10", .data = _data},
{},
  };
  MODULE_DEVICE_TABLE(of, altr_sdram_ctrl_of_match);
@@ -705,15 +705,12 @@ static void altr_create_edacdev_dbgfs(struct 
edac_device_ctl_info *edac_dci,

  static const struct of_device_id altr_edac_device_of_match[] = {
  #ifdef CONFIG_EDAC_ALTERA_L2C
-   { .compatible = "altr,socfpga-l2-ecc", .data = (void *)_data },
-   { .compatible = "altr,socfpga-a10-l2-ecc",
- .data = (void *)_l2ecc_data },
+   { .compatible = "altr,socfpga-l2-ecc", .data = _data },
+   { .compatible = "altr,socfpga-a10-l2-ecc", .data = _l2ecc_data },
  #endif
  #ifdef CONFIG_EDAC_ALTERA_OCRAM
-   { .compatible = "altr,socfpga-ocram-ecc",
- .data = (void *)_data },
-   { .compatible = "altr,socfpga-a10-ocram-ecc",
- .data = (void *)_ocramecc_data },
+   { .compatible = "altr,socfpga-ocram-ecc", .data = _data },
+   { .compatible = "altr,socfpga-a10-ocram-ecc", .data = 
_ocramecc_data },
  #endif
{},
  };



Acked-by: Thor Thayer <ttha...@opensource.altera.com>

Re: [PATCH 2/2] EDAC, altera: avoid unused function warnings

[Thor Thayer]

else {
-   writel(ALTR_A10_ECC_DERRPENA,
-  base + ALTR_A10_ECC_INTSTAT_OFST);
-   edac_device_handle_ue(dci->edac_dev, 0, 0, dci->edac_dev_name);
-   panic("\nEDAC:ECC_DEVICE[Uncorrectable errors]\n");
-   }
-   return IRQ_HANDLED;
-}
-
  static irqreturn_t altr_edac_a10_irq_handler(int irq, void *dev_id)
  {
irqreturn_t rc = IRQ_NONE;



Acked-by: Thor Thayer <ttha...@opensource.altera.com>

Re: [PATCH] Add EDAC peripheral init functions & Ethernet EDAC.

[Thor Thayer]

Hi Boris,

On 04/15/2016 04:46 PM, Borislav Petkov wrote:

On Fri, Apr 15, 2016 at 10:27:54AM -0500, Thor Thayer wrote:

I'll update this patch to only count errors.


... and also think about what that counting is going to bring. If it is
only going to be there to show how many network errors happened and we
can't do anything about it except stare at that number, then adding all
that code is probably waste of time an electrons...

I'm just sayin'.



Yes, valid point about the uncorrectable errors.

We're still getting the single bit correction which makes the entire 
system more stable and the ability to see both single bit errors 
corrected and the number of uncorrectable errors is useful from a system 
point of view.

Re: [PATCH 05/10] EDAC, altera: Add Arria10 NAND EDAC support

[Thor Thayer]

On 07/27/2016 12:10 PM, Borislav Petkov wrote:

On Thu, Jul 14, 2016 at 11:06:43AM -0500, ttha...@opensource.altera.com wrote:

From: Thor Thayer <ttha...@opensource.altera.com>

Add Altera Arria10 NAND FIFO memory EDAC support.

Signed-off-by: Thor Thayer <ttha...@opensource.altera.com>
---
  drivers/edac/Kconfig   |7 +++
  drivers/edac/altera_edac.c |   34 +-
  2 files changed, 40 insertions(+), 1 deletion(-)


...


@@ -1589,7 +1619,9 @@ static int altr_edac_a10_probe(struct platform_device 
*pdev)
else if ((of_device_is_compatible(child,
"altr,socfpga-a10-ocram-ecc")) ||
 (of_device_is_compatible(child,
-   "altr,socfpga-eth-mac-ecc")))
+   "altr,socfpga-eth-mac-ecc")) ||
+(of_device_is_compatible(child,
+ "altr,socfpga-nand-ecc")))
altr_edac_a10_device_add(edac, child);
else if (of_device_is_compatible(child,
 "altr,sdram-edac-a10"))


Can we simplify this loop like this?

for_each_child_of_node(pdev->dev.of_node, child) {
if (!of_device_is_available(child))
continue;

if (of_device_is_compatible(child, "altr,socfpga-a10-l2-ecc") ||
of_device_is_compatible(child, 
"altr,socfpga-a10-ocram-ecc") ||
of_device_is_compatible(child, "altr,socfpga-eth-mac-ecc") 
||
of_device_is_compatible(child, "altr,socfpga-nand-ecc"))

altr_edac_a10_device_add(edac, child);

else if (of_device_is_compatible(child, "altr,sdram-edac-a10"))
of_platform_populate(pdev->dev.of_node,
 altr_sdram_ctrl_of_match,
 NULL, >dev);
}

I've merged the first "if" and subsequent "else if" because they all
do altr_edac_a10_device_add(edac, child) and added spacing for better
readability.

Look ok?

Or have I fatfingered it?



Yes, that's better. I was trying to stay within the 80 character limit 
but missed the if/else if improvement. Thanks, Boris!


Should I re-submit?

Thanks,

Thor

Re: [PATCH 2/3] EDAC, altera: Add Arria10 SD-MMC EDAC support

[Thor Thayer]

On 08/08/2016 08:36 AM, Borislav Petkov wrote:

On Tue, Aug 02, 2016 at 10:56:20AM -0500, ttha...@opensource.altera.com wrote:

From: Thor Thayer <ttha...@opensource.altera.com>

Add Altera Arria10 SD-MMC FIFO memory EDAC support. The SD-MMC
is a dual port RAM implementation which is different than any
of the other peripherals and therefore requires additional code.

Signed-off-by: Thor Thayer <ttha...@opensource.altera.com>
---
 drivers/edac/Kconfig   |7 ++
 drivers/edac/altera_edac.c |  188 +++-
 drivers/edac/altera_edac.h |5 ++
 3 files changed, 199 insertions(+), 1 deletion(-)

diff --git a/drivers/edac/Kconfig b/drivers/edac/Kconfig
index 72752f4..394cd16 100644
--- a/drivers/edac/Kconfig
+++ b/drivers/edac/Kconfig
@@ -426,6 +426,13 @@ config EDAC_ALTERA_QSPI
  Support for error detection and correction on the
  Altera QSPI FIFO Memory for Altera SoCs.

+config EDAC_ALTERA_SDMMC
+   bool "Altera SDMMC FIFO ECC"
+   depends on EDAC_ALTERA=y && MMC_DW
+   help
+ Support for error detection and correction on the
+ Altera SDMMC FIFO Memory for Altera SoCs.
+
 config EDAC_SYNOPSYS
tristate "Synopsys DDR Memory Controller"
depends on EDAC_MM_EDAC && ARCH_ZYNQ
diff --git a/drivers/edac/altera_edac.c b/drivers/edac/altera_edac.c
index 28247f8..8b5177e 100644
--- a/drivers/edac/altera_edac.c
+++ b/drivers/edac/altera_edac.c
@@ -1393,6 +1393,188 @@ early_initcall(socfpga_init_qspi_ecc);

 #endif /* CONFIG_EDAC_ALTERA_QSPI */

+/* SDMMC Device Functions **/
+
+#ifdef CONFIG_EDAC_ALTERA_SDMMC
+
+static const struct edac_device_prv_data a10_sdmmceccb_data;
+static int altr_portb_setup(struct altr_edac_device_dev *device)
+{
+   struct edac_device_ctl_info *dci;
+   struct altr_edac_device_dev *altdev;
+   char *ecc_name = "sdmmcb-ecc";
+   int edac_idx, rc;
+   struct device_node *np;
+   const struct edac_device_prv_data *prv = _sdmmceccb_data;
+
+   rc = altr_check_ecc_deps(device);
+   if (rc)
+   return rc;
+
+   /* Create the PortB EDAC device */
+   edac_idx = edac_device_alloc_index();
+   dci = edac_device_alloc_ctl_info(sizeof(*altdev), ecc_name, 1,
+ecc_name, 1, 0, NULL, 0, edac_idx);
+   if (!dci) {
+   edac_printk(KERN_ERR, EDAC_DEVICE,
+   "%s: Unable to allocate PortB EDAC device\n",
+   ecc_name);
+   return -ENOMEM;
+   }
+
+   /* Initialize the PortB EDAC device structure from PortA structure */
+   altdev = dci->pvt_info;
+   *altdev = *device;
+
+   if (!devres_open_group(>ddev, altr_portb_setup, GFP_KERNEL))
+   return -ENOMEM;
+
+   /* Update PortB specific values */
+   altdev->edac_dev_name = ecc_name;
+   altdev->edac_idx = edac_idx;
+   altdev->edac_dev = dci;
+   altdev->data = prv;
+   dci->dev = >ddev;
+   dci->ctl_name = "Altera ECC Manager";
+   dci->mod_name = ecc_name;
+   dci->dev_name = ecc_name;
+
+   /* Find the SD/MMC device tree Node then update the IRQs for PortB */
+   np = of_find_compatible_node(NULL, NULL, "altr,socfpga-sdmmc-ecc");


So why aren't you doing this thing first in the function so that...


+   if (!np) {
+   rc = -ENODEV;
+   goto err_release_group_1;


... you can save yourself the unwind work in err_release_group_1?

In general, make sure you're doing all the work of poking at the
hardware so that you make sure you have the right resources *before* you
go and allocate and init stuff here.

Should make the error paths simpler and the function body smaller.



Argh. Missed that. The rest of the IRQ queries require the DCI 
allocation so the functions below will need to unwind but yes, the 
of_find_compatible_node should move. Thanks!



+   }
+
+   altdev->sb_irq = irq_of_parse_and_map(np, 2);
+   if (!altdev->sb_irq) {
+   edac_printk(KERN_ERR, EDAC_DEVICE, "Error PortB SBIRQ alloc\n");
+   rc = -ENODEV;
+   goto err_release_group_1;
+   }
+   rc = devm_request_irq(>ddev, altdev->sb_irq,
+ prv->ecc_irq_handler,
+ IRQF_SHARED, ecc_name, altdev);
+   if (rc) {
+   edac_printk(KERN_ERR, EDAC_DEVICE, "PortB SBERR IRQ error\n");
+   goto err_release_group_1;
+   }
+
+   altdev->db_irq = irq_of_parse_and_map(np, 3);
+   if (!altdev->db_irq) {
+   edac_printk(KERN_ERR, EDAC_DEVICE, "Error PortB DBIRQ alloc\n");
+   rc = -ENODEV;
+   goto err_release_group_1;
+   }
+   r

[PATCHv2] ARM: dts: Add EMAC AXI settings for Arria10

[thor . thayer]

From: Thor Thayer <thor.tha...@linux.intel.com>

Add the device tree entries needed to support the EMAC AXI
bus settings on the Arria10 SoCFPGA chip.

Signed-off-by: Thor Thayer <thor.tha...@linux.intel.com>
---
v2 Add the AXI configuration to the other DW EMACs in the chip.
---
 arch/arm/boot/dts/socfpga_arria10.dtsi | 9 +
 1 file changed, 9 insertions(+)

diff --git a/arch/arm/boot/dts/socfpga_arria10.dtsi 
b/arch/arm/boot/dts/socfpga_arria10.dtsi
index f520cbf..9abc7d8 100644
--- a/arch/arm/boot/dts/socfpga_arria10.dtsi
+++ b/arch/arm/boot/dts/socfpga_arria10.dtsi
@@ -400,6 +400,12 @@
};
};
 
+   socfpga_axi_setup: stmmac-axi-config {
+   snps,wr_osr_lmt = <0xf>;
+   snps,rd_osr_lmt = <0xf>;
+   snps,blen = <0 0 0 0 16 0 0>;
+   };
+
gmac0: ethernet@ff80 {
compatible = "altr,socfpga-stmmac", "snps,dwmac-3.72a", 
"snps,dwmac";
altr,sysmgr-syscon = < 0x44 0>;
@@ -416,6 +422,7 @@
clock-names = "stmmaceth";
resets = < EMAC0_RESET>;
reset-names = "stmmaceth";
+   snps,axi-config = <_axi_setup>;
status = "disabled";
};
 
@@ -435,6 +442,7 @@
clock-names = "stmmaceth";
resets = < EMAC1_RESET>;
reset-names = "stmmaceth";
+   snps,axi-config = <_axi_setup>;
status = "disabled";
};
 
@@ -452,6 +460,7 @@
rx-fifo-depth = <16384>;
clocks = <_mp_clk>;
clock-names = "stmmaceth";
+   snps,axi-config = <_axi_setup>;
status = "disabled";
};
 
-- 
2.7.4

[PATCH] ARM: dts: Add EMAC AXI settings for Arria10

[thor . thayer]

From: Thor Thayer <thor.tha...@linux.intel.com>

Add the device tree entries needed to support the EMAC AXI
bus settings on the Arria10 SoCFPGA chip.

Signed-off-by: Thor Thayer <thor.tha...@linux.intel.com>
---
 arch/arm/boot/dts/socfpga_arria10.dtsi | 7 +++
 1 file changed, 7 insertions(+)

diff --git a/arch/arm/boot/dts/socfpga_arria10.dtsi 
b/arch/arm/boot/dts/socfpga_arria10.dtsi
index f520cbf..f05dd15 100644
--- a/arch/arm/boot/dts/socfpga_arria10.dtsi
+++ b/arch/arm/boot/dts/socfpga_arria10.dtsi
@@ -400,6 +400,12 @@
};
};
 
+   socfpga_axi_setup: stmmac-axi-config {
+   snps,wr_osr_lmt = <0xf>;
+   snps,rd_osr_lmt = <0xf>;
+   snps,blen = <0 0 0 0 16 0 0>;
+   };
+
gmac0: ethernet@ff80 {
compatible = "altr,socfpga-stmmac", "snps,dwmac-3.72a", 
"snps,dwmac";
altr,sysmgr-syscon = < 0x44 0>;
@@ -416,6 +422,7 @@
clock-names = "stmmaceth";
resets = < EMAC0_RESET>;
reset-names = "stmmaceth";
+   snps,axi-config = <_axi_setup>;
status = "disabled";
};
 
-- 
2.7.4

[PATCHv2 4/5] mfd: altr_a10sr: Add Arria10 DevKit Reset Controller

[thor . thayer]

From: Thor Thayer <thor.tha...@linux.intel.com>

Add Peripheral PHY Reset Controller to the Arria10
Development Kit System Resource Chip's MFD.

Signed-off-by: Thor Thayer <thor.tha...@linux.intel.com>
---
v2  Changes to commit header & body for clarification.
---
 drivers/mfd/altera-a10sr.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/mfd/altera-a10sr.c b/drivers/mfd/altera-a10sr.c
index 06e1f7f..96e7d2c 100644
--- a/drivers/mfd/altera-a10sr.c
+++ b/drivers/mfd/altera-a10sr.c
@@ -33,6 +33,10 @@
.name = "altr_a10sr_gpio",
.of_compatible = "altr,a10sr-gpio",
},
+   {
+   .name = "altr_a10sr_reset",
+   .of_compatible = "altr,a10sr-reset",
+   },
 };
 
 static bool altr_a10sr_reg_readable(struct device *dev, unsigned int reg)
-- 
1.9.1

[PATCHv2 1/5] dt-bindings: mfd: Add Altera Arria10 SR Reset Controller bindings

[thor . thayer]

From: Thor Thayer <thor.tha...@linux.intel.com>

This patch adds documentation for the Altera A10-SR Reset
Controller DT bindings.

Signed-off-by: Thor Thayer <thor.tha...@linux.intel.com>
---
v2  No change
---
 Documentation/devicetree/bindings/mfd/altera-a10sr.txt | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/altera-a10sr.txt 
b/Documentation/devicetree/bindings/mfd/altera-a10sr.txt
index ea151f2..c8a7365 100644
--- a/Documentation/devicetree/bindings/mfd/altera-a10sr.txt
+++ b/Documentation/devicetree/bindings/mfd/altera-a10sr.txt
@@ -18,6 +18,7 @@ The A10SR consists of these sub-devices:
 Device   Description
 --   --
 a10sr_gpio   GPIO Controller
+a10sr_rstReset Controller
 
 Arria10 GPIO
 Required Properties:
@@ -27,6 +28,11 @@ Required Properties:
   the second cell is used to specify flags.
   See ../gpio/gpio.txt for more information.
 
+Arria10 Peripheral PHY Reset
+Required Properties:
+- compatible: Should be "altr,a10sr-reset"
+- #reset-cells  : Should be one.
+
 Example:
 
 resource-manager@0 {
@@ -43,4 +49,9 @@ Example:
gpio-controller;
#gpio-cells = <2>;
};
+
+   a10sr_rst: reset-controller {
+   compatible = "altr,a10sr-reset";
+   #reset-cells = <1>;
+   };
};
-- 
1.9.1

[PATCHv2 0/5] Add Arria10 System Manager Reset Controller

[thor . thayer]

From: Thor Thayer <thor.tha...@linux.intel.com>

This series of patches adds the Altera Arria10 Development Kit System
Resource Chip's Reset Controller.

Thor Thayer (5):
  dt-bindings: mfd: Add Altera Arria10 SR Reset Controller bindings
  dt-bindings: reset: a10sr: Add Arria10 SR Reset Controller offsets
  reset: Add Altera Arria10 SR Reset Controller
  mfd: altr_a10sr: Add Arria10 DevKit Reset Controller
  ARM: dts: socfpga: Add Devkit A10-SR Reset Controller

 .../devicetree/bindings/mfd/altera-a10sr.txt   |  11 ++
 MAINTAINERS|   2 +
 arch/arm/boot/dts/socfpga_arria10_socdk.dtsi   |   5 +
 drivers/mfd/altera-a10sr.c |   4 +
 drivers/reset/Kconfig  |   7 ++
 drivers/reset/Makefile |   1 +
 drivers/reset/reset-a10sr.c| 138 +
 include/dt-bindings/reset/altr,rst-mgr-a10sr.h |  33 +
 8 files changed, 201 insertions(+)
 create mode 100644 drivers/reset/reset-a10sr.c
 create mode 100644 include/dt-bindings/reset/altr,rst-mgr-a10sr.h

-- 
1.9.1

[PATCHv2 2/5] dt-bindings: reset: a10sr: Add Arria10 SR Reset Controller offsets

[thor . thayer]

From: Thor Thayer <thor.tha...@linux.intel.com>

The Arria10 System Resource Chip reset controller handles the
Arria10 peripheral PHYs. This patch adds the offsets for
these PHYs.

Signed-off-by: Thor Thayer <thor.tha...@linux.intel.com>
---
v2  Add NUM_RESETs to altr,rst-mgr-a10sr.h for maximum count.
---
 MAINTAINERS|  1 +
 include/dt-bindings/reset/altr,rst-mgr-a10sr.h | 33 ++
 2 files changed, 34 insertions(+)
 create mode 100644 include/dt-bindings/reset/altr,rst-mgr-a10sr.h

diff --git a/MAINTAINERS b/MAINTAINERS
index b9af886..4b714bd 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -654,6 +654,7 @@ S:  Maintained
 F: drivers/gpio/gpio-altera-a10sr.c
 F: drivers/mfd/altera-a10sr.c
 F: include/linux/mfd/altera-a10sr.h
+F: include/dt-bindings/reset/altr,rst-mgr-a10sr.h
 
 ALTERA TRIPLE SPEED ETHERNET DRIVER
 M: Vince Bridgers <vbrid...@opensource.altera.com>
diff --git a/include/dt-bindings/reset/altr,rst-mgr-a10sr.h 
b/include/dt-bindings/reset/altr,rst-mgr-a10sr.h
new file mode 100644
index 000..9855925
--- /dev/null
+++ b/include/dt-bindings/reset/altr,rst-mgr-a10sr.h
@@ -0,0 +1,33 @@
+/*
+ *  Copyright Intel Corporation (C) 2017. All Rights Reserved
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Reset binding definitions for Altera Arria10 MAX5 System Resource Chip
+ *
+ * Adapted from altr,rst-mgr-a10.h
+ */
+
+#ifndef _DT_BINDINGS_RESET_ALTR_RST_MGR_A10SR_H
+#define _DT_BINDINGS_RESET_ALTR_RST_MGR_A10SR_H
+
+/* Peripheral PHY resets */
+#define A10SR_RESET_ENET_HPS   0
+#define A10SR_RESET_PCIE   1
+#define A10SR_RESET_FILE   2
+#define A10SR_RESET_BQSPI  3
+#define A10SR_RESET_USB4
+
+#define A10SR_RESET_NUM5
+
+#endif
-- 
1.9.1

[PATCHv2 5/5] ARM: dts: socfpga: Add Devkit A10-SR Reset Controller

[thor . thayer]

From: Thor Thayer <thor.tha...@linux.intel.com>

Add the Altera Arria10 System Resource Reset Controller to the MFD

Signed-off-by: Thor Thayer <thor.tha...@linux.intel.com>
Acked-by: Dinh Nguyen <dingu...@kernel.org>
---
v2  change commit header to ARM: dts: socfpga.
---
 arch/arm/boot/dts/socfpga_arria10_socdk.dtsi | 5 +
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/socfpga_arria10_socdk.dtsi 
b/arch/arm/boot/dts/socfpga_arria10_socdk.dtsi
index c57e6ce..9329025 100644
--- a/arch/arm/boot/dts/socfpga_arria10_socdk.dtsi
+++ b/arch/arm/boot/dts/socfpga_arria10_socdk.dtsi
@@ -121,6 +121,11 @@
gpio-controller;
#gpio-cells = <2>;
};
+
+   a10sr_rst: reset-controller {
+   compatible = "altr,a10sr-reset";
+   #reset-cells = <1>;
+   };
};
 };
 
-- 
1.9.1

[PATCHv2 3/5] reset: Add Altera Arria10 SR Reset Controller

[thor . thayer]

From: Thor Thayer <thor.tha...@linux.intel.com>

This patch adds the reset controller functionality for
Peripheral PHYs to the Arria10 System Resource Chip.

Signed-off-by: Thor Thayer <thor.tha...@linux.intel.com>
---
V2  Rename and move new Kconfig to correct alphabetical order.
Update Makefile with better Kconfig name.
Alphabetize MAINTAINER drivers entry.
Remove unused include files.
Replace 16 reset size with actual size. Enumerate size in
header file.
Remove extra id in-range checks since framework checks this.
Return error code from regmap_read() fail instead of fixed error.
Remove device_node validation since framework handles this.
Remove device_node and device local variables in cleanup.
Delete a10sr_reset_remove() since using devm_ functions.
Delete .remove function and .owner from platform_driver struct.
Minor cleanup of commit header and commit description.
---
 MAINTAINERS |   1 +
 drivers/reset/Kconfig   |   7 +++
 drivers/reset/Makefile  |   1 +
 drivers/reset/reset-a10sr.c | 138 
 4 files changed, 147 insertions(+)
 create mode 100644 drivers/reset/reset-a10sr.c

diff --git a/MAINTAINERS b/MAINTAINERS
index 4b714bd..35ffb92 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -653,6 +653,7 @@ M:  Thor Thayer <thor.tha...@linux.intel.com>
 S: Maintained
 F: drivers/gpio/gpio-altera-a10sr.c
 F: drivers/mfd/altera-a10sr.c
+F: drivers/reset/reset-a10sr.c
 F: include/linux/mfd/altera-a10sr.h
 F: include/dt-bindings/reset/altr,rst-mgr-a10sr.h
 
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index f4cdfe9..54e4b8b 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -14,6 +14,13 @@ menuconfig RESET_CONTROLLER
 
 if RESET_CONTROLLER
 
+config RESET_A10SR
+   tristate "Altera Arria10 System Resource Reset"
+   depends on MFD_ALTERA_A10SR
+   help
+ This option enables support for the external reset functions for
+ peripheral PHYs on the Altera Arria10 System Resource Chip.
+
 config RESET_ATH79
bool "AR71xx Reset Driver" if COMPILE_TEST
default ATH79
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 2cd3f6c..8f6eb57 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -2,6 +2,7 @@ obj-y += core.o
 obj-y += hisilicon/
 obj-$(CONFIG_ARCH_STI) += sti/
 obj-$(CONFIG_ARCH_TEGRA) += tegra/
+obj-$(CONFIG_RESET_A10SR) += reset-a10sr.o
 obj-$(CONFIG_RESET_ATH79) += reset-ath79.o
 obj-$(CONFIG_RESET_BERLIN) += reset-berlin.o
 obj-$(CONFIG_RESET_LPC18XX) += reset-lpc18xx.o
diff --git a/drivers/reset/reset-a10sr.c b/drivers/reset/reset-a10sr.c
new file mode 100644
index 000..37496bd
--- /dev/null
+++ b/drivers/reset/reset-a10sr.c
@@ -0,0 +1,138 @@
+/*
+ *  Copyright Intel Corporation (C) 2017. All Rights Reserved
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Reset driver for Altera Arria10 MAX5 System Resource Chip
+ *
+ * Adapted from reset-socfpga.c
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+struct a10sr_reset {
+   struct reset_controller_dev rcdev;
+   struct regmap *regmap;
+};
+
+static inline struct a10sr_reset *to_a10sr_rst(struct reset_controller_dev *rc)
+{
+   return container_of(rc, struct a10sr_reset, rcdev);
+}
+
+static inline int a10sr_reset_shift(unsigned long id)
+{
+   switch (id) {
+   case A10SR_RESET_ENET_HPS:
+   return 1;
+   case A10SR_RESET_PCIE:
+   case A10SR_RESET_FILE:
+   case A10SR_RESET_BQSPI:
+   case A10SR_RESET_USB:
+   return id + 11;
+   default:
+   return -EINVAL;
+   }
+}
+
+static int a10sr_reset_update(struct reset_controller_dev *rcdev,
+ unsigned long id, bool assert)
+{
+   struct a10sr_reset *a10r = to_a10sr_rst(rcdev);
+   int offset = a10sr_reset_shift(id);
+   u8 mask = ALTR_A10SR_REG_BIT_MASK(offset);
+   int index = ALTR_A10SR_HPS_RST_REG + ALTR_A10SR_REG_OFFSET(offset);
+
+   return regmap_update_bits(a10r->regmap, index, mask, assert ? 0 : mask);
+}
+
+static int a10sr_reset_assert(struct reset_controller_dev *rcdev,
+ unsigned long id)
+{
+   return a10sr_reset_update(rcdev, id, true);
+}
+
+static int a10sr_re

[PATCH] mfd: altr-a10sr: Add Arria10 SR sysfs attributes

[thor . thayer]

From: Thor Thayer <thor.tha...@linux.intel.com>

Add the Altera Arria10 DevKit sysfs attributes to the
MFD device. Update copyright and email.

Signed-off-by: Thor Thayer <thor.tha...@linux.intel.com>
---
 drivers/mfd/altera-a10sr.c | 98 --
 1 file changed, 95 insertions(+), 3 deletions(-)

diff --git a/drivers/mfd/altera-a10sr.c b/drivers/mfd/altera-a10sr.c
index 06e1f7f..8c51818 100644
--- a/drivers/mfd/altera-a10sr.c
+++ b/drivers/mfd/altera-a10sr.c
@@ -1,9 +1,9 @@
 /*
  * Altera Arria10 DevKit System Resource MFD Driver
  *
- * Author: Thor Thayer <ttha...@opensource.altera.com>
+ * Author: Thor Thayer <thor.tha...@linux.intel.com>
  *
- * Copyright Intel Corporation (C) 2014-2016. All Rights Reserved
+ * Copyright Intel Corporation (C) 2014-2017. All Rights Reserved
  *
  * This program is free software; you can redistribute it and/or modify it
  * under the terms and conditions of the GNU General Public License,
@@ -35,6 +35,89 @@
},
 };
 
+/* Add sysfs interface for MAX5 System Resource Controller */
+static unsigned int a10sr_reg_addr;
+static ssize_t a10sr_reg_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+   return sprintf(buf, "0x%02x\n", a10sr_reg_addr);
+}
+
+static ssize_t a10sr_reg_store(struct device *dev,
+  struct device_attribute *attr,
+  const char *buf, size_t count)
+{
+   int ret = kstrtouint(buf, 0, _reg_addr);
+
+   if (ret || (a10sr_reg_addr < 0) ||
+   (a10sr_reg_addr > ALTR_A10SR_PMBUS_REG)) {
+   a10sr_reg_addr = 0;
+   dev_err(dev, "Invalid register address\n");
+   return -EINVAL;
+   }
+   return (ssize_t)count;
+}
+
+static ssize_t a10sr_val_show(struct device *dev,
+ struct device_attribute *attr, char *buf)
+{
+   int ret;
+   unsigned int val;
+   struct altr_a10sr *a10sr_regs = dev_get_drvdata(dev);
+
+   ret = regmap_read(a10sr_regs->regmap, a10sr_reg_addr, );
+   if (ret < 0) {
+   dev_err(dev, "Failed to read 0x%x\n", a10sr_reg_addr);
+   return -EIO;
+   }
+
+   return sprintf(buf, "0x%02x\n", val);
+}
+
+static ssize_t a10sr_val_store(struct device *dev,
+  struct device_attribute *attr,
+  const char *buf, size_t count)
+{
+   int ret;
+   unsigned int val;
+   struct altr_a10sr *a10sr_regs = dev_get_drvdata(dev);
+
+   ret = kstrtouint(buf, 0, );
+   if (ret)
+   return ret;
+
+   ret = regmap_write(a10sr_regs->regmap, a10sr_reg_addr, val);
+   if (ret) {
+   dev_err(dev, "Failed to write value 0x%02x to address 0x%x",
+   val, a10sr_reg_addr);
+   return -EIO;
+   }
+   return count;
+}
+
+static ssize_t a10sr_version(struct device *dev,
+struct device_attribute *devattr, char *buf)
+{
+   a10sr_reg_addr = ALTR_A10SR_VERSION_READ;
+   return a10sr_val_show(dev, devattr, buf);
+}
+
+/* Define FS entries */
+static DEVICE_ATTR(max5_version, 0444, a10sr_version, NULL);
+static DEVICE_ATTR(max5_address, 0644, a10sr_reg_show, a10sr_reg_store);
+static DEVICE_ATTR(max5_value, 0644, a10sr_val_show, a10sr_val_store);
+
+static struct attribute *altr_a10sr_attr[] = {
+   _attr_max5_version.attr,
+   _attr_max5_address.attr,
+   _attr_max5_value.attr,
+   NULL
+};
+
+static const struct attribute_group a10sr_attr_group = {
+   .attrs = altr_a10sr_attr,
+};
+
 static bool altr_a10sr_reg_readable(struct device *dev, unsigned int reg)
 {
switch (reg) {
@@ -141,13 +224,22 @@ static int altr_a10sr_spi_probe(struct spi_device *spi)
return ret;
}
 
+   /* Add the A10SR Registers to the device's sysfs */
+   ret = sysfs_create_group(>dev->kobj, _attr_group);
+   if (ret) {
+   dev_err(>dev, "unable to create sysfs attributes\n");
+   return ret;
+   }
+
ret = devm_mfd_add_devices(a10sr->dev, PLATFORM_DEVID_AUTO,
   altr_a10sr_subdev_info,
   ARRAY_SIZE(altr_a10sr_subdev_info),
   NULL, 0, NULL);
-   if (ret)
+   if (ret) {
dev_err(a10sr->dev, "Failed to register sub-devices: %d\n",
ret);
+   sysfs_remove_group(>dev->kobj, _attr_group);
+   }
 
return ret;
 }
-- 
1.9.1

Re: [PATCH 3/5] reset: Add Altera Arria10 System Resource Reset Controller

[Thor Thayer]

Hi Philipp,

On 02/16/2017 04:30 AM, Philipp Zabel wrote:

Hi Thor,

thank you for the patch. A few comments below:

On Wed, 2017-02-15 at 15:50 -0600, thor.tha...@linux.intel.com wrote:

From: Thor Thayer <thor.tha...@linux.intel.com>

This patch adds the reset controller functionality to the Arria10
System Resource Manager.

Signed-off-by: Thor Thayer <thor.tha...@linux.intel.com>
---
 MAINTAINERS |   1 +
 drivers/reset/Kconfig   |   7 ++
 drivers/reset/Makefile  |   1 +
 drivers/reset/reset-a10sr.c | 176 
 4 files changed, 185 insertions(+)
 create mode 100644 drivers/reset/reset-a10sr.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a2c74db..3265cb2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -655,6 +655,7 @@ F:  drivers/gpio/gpio-altera-a10sr.c
 F: drivers/mfd/altera-a10sr.c
 F: include/linux/mfd/altera-a10sr.h
 F: include/dt-bindings/reset/altr,rst-mgr-a10sr.h
+F: drivers/reset/reset-a10sr.c

 ALTERA TRIPLE SPEED ETHERNET DRIVER
 M: Vince Bridgers <vbrid...@opensource.altera.com>
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index f4cdfe9..b821d1b 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -39,6 +39,13 @@ config RESET_MESON
help
  This enables the reset driver for Amlogic Meson SoCs.

+config ALTERA_A10SR_RESET


Please follow the RESET_ convention and sort alphabetically.


Whoops. Thanks.


+   tristate "Altera Arria10 System Resource Reset"
+   depends on MFD_ALTERA_A10SR
+   help
+ This option enables support for the external reset functions for
+ peripheral PHYs on the Altera Arria10 System Resource Chip.
+
 config RESET_OXNAS
bool

diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 2cd3f6c..d6a2a87 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -15,3 +15,4 @@ obj-$(CONFIG_TI_SYSCON_RESET) += reset-ti-syscon.o
 obj-$(CONFIG_RESET_UNIPHIER) += reset-uniphier.o
 obj-$(CONFIG_RESET_ZX2967) += reset-zx2967.o
 obj-$(CONFIG_RESET_ZYNQ) += reset-zynq.o
+obj-$(CONFIG_ALTERA_A10SR_RESET) += reset-a10sr.o
diff --git a/drivers/reset/reset-a10sr.c b/drivers/reset/reset-a10sr.c
new file mode 100644
index 000..ed058f3
--- /dev/null
+++ b/drivers/reset/reset-a10sr.c
@@ -0,0 +1,176 @@
+/*
+ *  Copyright Intel Corporation (C) 2017. All Rights Reserved
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Reset driver for Altera Arria10 MAX5 System Resource Chip
+ *
+ * Adapted from reset-socfpga.c
+ */
+
+#include 
+#include 


This is not used.


OK, thanks.


+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+/* Number of A10 System Controller Resets */
+#define A10SR_RESETS   16


So 16 resets...


+struct a10sr_reset {
+   struct reset_controller_dev rcdev;
+   struct regmap *regmap;
+};
+
+static inline struct a10sr_reset *to_a10sr_rst(struct reset_controller_dev *rc)
+{
+   return container_of(rc, struct a10sr_reset, rcdev);
+}
+
+static inline int a10sr_reset_shift(unsigned long id)
+{
+   switch (id) {
+   case A10SR_RESET_ENET_HPS:
+   return 1;
+   case A10SR_RESET_PCIE:
+   case A10SR_RESET_FILE:
+   case A10SR_RESET_BQSPI:
+   case A10SR_RESET_USB:
+   return id + 11;


... but only 5 are handled. What about the other 11? Could you point me
to documentation for this reset controller?

Whoops. Good catch, I will fix this. Remnants of my first implementation 
before realizing a switch statement was cleaner.


I looked and apparently we don't publish the System Resource 
documentation yet. This is for a CPLD programmed as a system manager so 
there is some sharing of reset/enable and status bits.


There were two 8-bit registers but only 5 bits are writable while others 
only show status. Bit 2 of the first register and bits 12-15 of the 2nd 
register are writable resets.



+   default:
+   return -EINVAL;
+   }
+}
+
+static int a10sr_reset_update(struct reset_controller_dev *rcdev,
+ unsigned long id, bool assert)
+{
+   struct a10sr_reset *a10r = to_a10sr_rst(rcdev);
+   int offset = a10sr_reset_shift(id);
+   u8 mask = ALTR_A10SR_REG_BIT_MASK(offset);
+   int index = ALTR_A10SR_HPS_RST_REG + ALTR_A10SR_REG_OFFSET(offset);
+

Re: [PATCH 5/5] ARM: socfpga: dts: Add Devkit A10-SR Reset Controller

[Thor Thayer]

On 02/16/2017 10:12 AM, Dinh Nguyen wrote:



On 02/15/2017 03:50 PM, thor.tha...@linux.intel.com wrote:

From: Thor Thayer <thor.tha...@linux.intel.com>

Add the Altera Arria10 System Resource Reset Controller to the MFD

Signed-off-by: Thor Thayer <thor.tha...@linux.intel.com>


Nit:

Please have the commit header as "ARM: dts: socfpga:"

Otherwise,

Acked-by: Dinh Nguyen <dingu...@kernel.org>

Dinh


OK. I'll change it for my next revision. Thanks for reviewing!

[PATCH 4/5] mfd: altr_a10sr: Add Arria10 DevKit Resource Controller

[thor . thayer]

From: Thor Thayer <thor.tha...@linux.intel.com>

Add Arria10 System Resource Manager Reset Controller to MFD.

Signed-off-by: Thor Thayer <thor.tha...@linux.intel.com>
---
 drivers/mfd/altera-a10sr.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/mfd/altera-a10sr.c b/drivers/mfd/altera-a10sr.c
index 06e1f7f..96e7d2c 100644
--- a/drivers/mfd/altera-a10sr.c
+++ b/drivers/mfd/altera-a10sr.c
@@ -33,6 +33,10 @@
.name = "altr_a10sr_gpio",
.of_compatible = "altr,a10sr-gpio",
},
+   {
+   .name = "altr_a10sr_reset",
+   .of_compatible = "altr,a10sr-reset",
+   },
 };
 
 static bool altr_a10sr_reg_readable(struct device *dev, unsigned int reg)
-- 
1.9.1

[PATCH 0/5] Add Arria10 System Manager Reset Controller

[thor . thayer]

From: Thor Thayer <thor.tha...@linux.intel.com>

This series of patches adds the Altera Arria10 Development Kit System
Manager Reset Controller.

Thor Thayer (5):
  dt-bindings: mfd: Add Altera Arria10 SR Reset Controller bindings
  dt-bindings: Add Arria10 System Resource reset manager offsets
  reset: Add Altera Arria10 System Resource Reset Controller
  mfd: altr_a10sr: Add Arria10 DevKit Resource Controller
  ARM: socfpga: dts: Add Devkit A10-SR Reset Controller

 .../devicetree/bindings/mfd/altera-a10sr.txt   |  11 ++
 MAINTAINERS|   2 +
 arch/arm/boot/dts/socfpga_arria10_socdk.dtsi   |   5 +
 drivers/mfd/altera-a10sr.c |   4 +
 drivers/reset/Kconfig  |   7 +
 drivers/reset/Makefile |   1 +
 drivers/reset/reset-a10sr.c| 176 +
 include/dt-bindings/reset/altr,rst-mgr-a10sr.h |  31 
 8 files changed, 237 insertions(+)
 create mode 100644 drivers/reset/reset-a10sr.c
 create mode 100644 include/dt-bindings/reset/altr,rst-mgr-a10sr.h

-- 
1.9.1

[PATCH 5/5] ARM: socfpga: dts: Add Devkit A10-SR Reset Controller

[thor . thayer]

From: Thor Thayer <thor.tha...@linux.intel.com>

Add the Altera Arria10 System Resource Reset Controller to the MFD

Signed-off-by: Thor Thayer <thor.tha...@linux.intel.com>
---
 arch/arm/boot/dts/socfpga_arria10_socdk.dtsi | 5 +
 1 file changed, 5 insertions(+)

diff --git a/arch/arm/boot/dts/socfpga_arria10_socdk.dtsi 
b/arch/arm/boot/dts/socfpga_arria10_socdk.dtsi
index c57e6ce..9329025 100644
--- a/arch/arm/boot/dts/socfpga_arria10_socdk.dtsi
+++ b/arch/arm/boot/dts/socfpga_arria10_socdk.dtsi
@@ -121,6 +121,11 @@
gpio-controller;
#gpio-cells = <2>;
};
+
+   a10sr_rst: reset-controller {
+   compatible = "altr,a10sr-reset";
+   #reset-cells = <1>;
+   };
};
 };
 
-- 
1.9.1

[PATCH 2/5] dt-bindings: Add Arria10 System Resource reset manager offsets

[thor . thayer]

From: Thor Thayer <thor.tha...@linux.intel.com>

The Arria10 System Resource reset manager handles the Arria10
peripheral PHYs. This patch adds the offsets for these PHYs.

Signed-off-by: Thor Thayer <thor.tha...@linux.intel.com>
---
 MAINTAINERS|  1 +
 include/dt-bindings/reset/altr,rst-mgr-a10sr.h | 31 ++
 2 files changed, 32 insertions(+)
 create mode 100644 include/dt-bindings/reset/altr,rst-mgr-a10sr.h

diff --git a/MAINTAINERS b/MAINTAINERS
index 9daf28b..a2c74db 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -654,6 +654,7 @@ S:  Maintained
 F: drivers/gpio/gpio-altera-a10sr.c
 F: drivers/mfd/altera-a10sr.c
 F: include/linux/mfd/altera-a10sr.h
+F: include/dt-bindings/reset/altr,rst-mgr-a10sr.h
 
 ALTERA TRIPLE SPEED ETHERNET DRIVER
 M: Vince Bridgers <vbrid...@opensource.altera.com>
diff --git a/include/dt-bindings/reset/altr,rst-mgr-a10sr.h 
b/include/dt-bindings/reset/altr,rst-mgr-a10sr.h
new file mode 100644
index 000..252f71a7
--- /dev/null
+++ b/include/dt-bindings/reset/altr,rst-mgr-a10sr.h
@@ -0,0 +1,31 @@
+/*
+ *  Copyright Intel Corporation (C) 2017. All Rights Reserved
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Reset binding definitions for Altera Arria10 MAX5 System Resource Chip
+ *
+ * Adapted from altr,rst-mgr-a10.h
+ */
+
+#ifndef _DT_BINDINGS_RESET_ALTR_RST_MGR_A10SR_H
+#define _DT_BINDINGS_RESET_ALTR_RST_MGR_A10SR_H
+
+/* Peripheral PHY resets */
+#define A10SR_RESET_ENET_HPS   0
+#define A10SR_RESET_PCIE   1
+#define A10SR_RESET_FILE   2
+#define A10SR_RESET_BQSPI  3
+#define A10SR_RESET_USB4
+
+#endif
-- 
1.9.1

[PATCH 3/5] reset: Add Altera Arria10 System Resource Reset Controller

[thor . thayer]

From: Thor Thayer <thor.tha...@linux.intel.com>

This patch adds the reset controller functionality to the Arria10
System Resource Manager.

Signed-off-by: Thor Thayer <thor.tha...@linux.intel.com>
---
 MAINTAINERS |   1 +
 drivers/reset/Kconfig   |   7 ++
 drivers/reset/Makefile  |   1 +
 drivers/reset/reset-a10sr.c | 176 
 4 files changed, 185 insertions(+)
 create mode 100644 drivers/reset/reset-a10sr.c

diff --git a/MAINTAINERS b/MAINTAINERS
index a2c74db..3265cb2 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -655,6 +655,7 @@ F:  drivers/gpio/gpio-altera-a10sr.c
 F: drivers/mfd/altera-a10sr.c
 F: include/linux/mfd/altera-a10sr.h
 F: include/dt-bindings/reset/altr,rst-mgr-a10sr.h
+F: drivers/reset/reset-a10sr.c
 
 ALTERA TRIPLE SPEED ETHERNET DRIVER
 M: Vince Bridgers <vbrid...@opensource.altera.com>
diff --git a/drivers/reset/Kconfig b/drivers/reset/Kconfig
index f4cdfe9..b821d1b 100644
--- a/drivers/reset/Kconfig
+++ b/drivers/reset/Kconfig
@@ -39,6 +39,13 @@ config RESET_MESON
help
  This enables the reset driver for Amlogic Meson SoCs.
 
+config ALTERA_A10SR_RESET
+   tristate "Altera Arria10 System Resource Reset"
+   depends on MFD_ALTERA_A10SR
+   help
+ This option enables support for the external reset functions for
+ peripheral PHYs on the Altera Arria10 System Resource Chip.
+
 config RESET_OXNAS
bool
 
diff --git a/drivers/reset/Makefile b/drivers/reset/Makefile
index 2cd3f6c..d6a2a87 100644
--- a/drivers/reset/Makefile
+++ b/drivers/reset/Makefile
@@ -15,3 +15,4 @@ obj-$(CONFIG_TI_SYSCON_RESET) += reset-ti-syscon.o
 obj-$(CONFIG_RESET_UNIPHIER) += reset-uniphier.o
 obj-$(CONFIG_RESET_ZX2967) += reset-zx2967.o
 obj-$(CONFIG_RESET_ZYNQ) += reset-zynq.o
+obj-$(CONFIG_ALTERA_A10SR_RESET) += reset-a10sr.o
diff --git a/drivers/reset/reset-a10sr.c b/drivers/reset/reset-a10sr.c
new file mode 100644
index 000..ed058f3
--- /dev/null
+++ b/drivers/reset/reset-a10sr.c
@@ -0,0 +1,176 @@
+/*
+ *  Copyright Intel Corporation (C) 2017. All Rights Reserved
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ * Reset driver for Altera Arria10 MAX5 System Resource Chip
+ *
+ * Adapted from reset-socfpga.c
+ */
+
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+#include 
+
+/* Number of A10 System Controller Resets */
+#define A10SR_RESETS   16
+
+struct a10sr_reset {
+   struct reset_controller_dev rcdev;
+   struct regmap *regmap;
+};
+
+static inline struct a10sr_reset *to_a10sr_rst(struct reset_controller_dev *rc)
+{
+   return container_of(rc, struct a10sr_reset, rcdev);
+}
+
+static inline int a10sr_reset_shift(unsigned long id)
+{
+   switch (id) {
+   case A10SR_RESET_ENET_HPS:
+   return 1;
+   case A10SR_RESET_PCIE:
+   case A10SR_RESET_FILE:
+   case A10SR_RESET_BQSPI:
+   case A10SR_RESET_USB:
+   return id + 11;
+   default:
+   return -EINVAL;
+   }
+}
+
+static int a10sr_reset_update(struct reset_controller_dev *rcdev,
+ unsigned long id, bool assert)
+{
+   struct a10sr_reset *a10r = to_a10sr_rst(rcdev);
+   int offset = a10sr_reset_shift(id);
+   u8 mask = ALTR_A10SR_REG_BIT_MASK(offset);
+   int index = ALTR_A10SR_HPS_RST_REG + ALTR_A10SR_REG_OFFSET(offset);
+
+   if (id >= rcdev->nr_resets)
+   return -EINVAL;
+
+   return regmap_update_bits(a10r->regmap, index, mask, assert ? 0 : mask);
+}
+
+static int a10sr_reset_assert(struct reset_controller_dev *rcdev,
+ unsigned long id)
+{
+   return a10sr_reset_update(rcdev, id, true);
+}
+
+static int a10sr_reset_deassert(struct reset_controller_dev *rcdev,
+   unsigned long id)
+{
+   return a10sr_reset_update(rcdev, id, false);
+}
+
+static int a10sr_reset_status(struct reset_controller_dev *rcdev,
+ unsigned long id)
+{
+   int error;
+   struct a10sr_reset *a10r = to_a10sr_rst(rcdev);
+   int offset = a10sr_reset_shift(id);
+   u8 mask = ALTR_A10SR_REG_BIT_MASK(offset);
+   int index = ALTR_A10SR_HPS_RST_REG + ALTR_A10SR_REG_OFFSET(offset);
+   unsigned int value;
+
+   if (id >= rc

[PATCH 1/5] dt-bindings: mfd: Add Altera Arria10 SR Reset Controller bindings

[thor . thayer]

From: Thor Thayer <thor.tha...@linux.intel.com>

This patch adds documentation for the Altera A10-SR Reset
Controller DT bindings.

Signed-off-by: Thor Thayer <thor.tha...@linux.intel.com>
---
 Documentation/devicetree/bindings/mfd/altera-a10sr.txt | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/devicetree/bindings/mfd/altera-a10sr.txt 
b/Documentation/devicetree/bindings/mfd/altera-a10sr.txt
index ea151f2..c8a7365 100644
--- a/Documentation/devicetree/bindings/mfd/altera-a10sr.txt
+++ b/Documentation/devicetree/bindings/mfd/altera-a10sr.txt
@@ -18,6 +18,7 @@ The A10SR consists of these sub-devices:
 Device   Description
 --   --
 a10sr_gpio   GPIO Controller
+a10sr_rstReset Controller
 
 Arria10 GPIO
 Required Properties:
@@ -27,6 +28,11 @@ Required Properties:
   the second cell is used to specify flags.
   See ../gpio/gpio.txt for more information.
 
+Arria10 Peripheral PHY Reset
+Required Properties:
+- compatible: Should be "altr,a10sr-reset"
+- #reset-cells  : Should be one.
+
 Example:
 
 resource-manager@0 {
@@ -43,4 +49,9 @@ Example:
gpio-controller;
#gpio-cells = <2>;
};
+
+   a10sr_rst: reset-controller {
+   compatible = "altr,a10sr-reset";
+   #reset-cells = <1>;
+   };
};
-- 
1.9.1

Re: [PATCH] mfd: altera-a10sr: Make altr_a10sr_regmap_config static const

[Thor Thayer]

On 08/05/2016 01:40 AM, Axel Lin wrote:

It's only used in this driver and never get modified, make it static const.

Signed-off-by: Axel Lin <axel@ingics.com>
---
 drivers/mfd/altera-a10sr.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/mfd/altera-a10sr.c b/drivers/mfd/altera-a10sr.c
index c05aa4f..40ed65e 100644
--- a/drivers/mfd/altera-a10sr.c
+++ b/drivers/mfd/altera-a10sr.c
@@ -94,7 +94,7 @@ static bool altr_a10sr_reg_volatile(struct device *dev, 
unsigned int reg)
}
 }

-const struct regmap_config altr_a10sr_regmap_config = {
+static const struct regmap_config altr_a10sr_regmap_config = {
.reg_bits = 8,
.val_bits = 8,



Reviewed-by: Thor Thayer <ttha...@opensource.altera.com>

Re: [PATCH] spi: dw: Enable Slave Select with GPIO Chip Select.

[Thor Thayer]

On 10/06/2016 04:37 AM, Mark Brown wrote:

On Wed, Oct 05, 2016 at 04:38:58PM -0500, ttha...@opensource.altera.com wrote:


This patch adds the Slave Select locally so that the transfer will
start and complete. The GPIO CS is taken care of earlier in the SPI
framework (spi_set_cs).


This seems like something that other devices might need - it's not the
first time I heard of a device with the data transfer start and chip
select being tied together.  Why not make this a generic feature that
the core implements and drivers can enable?

OK. Thanks for the suggestion and review. I'll add a new boolean and 
device tree binding to the SPI core. Thanks.

Re: [PATCHv2 1/2] Documentation: dt: spi: Add GPIO Slave Select Parameter

[Thor Thayer]

On 10/07/2016 02:33 PM, Geert Uytterhoeven wrote:

On Fri, Oct 7, 2016 at 4:56 PM,  <ttha...@opensource.altera.com> wrote:

From: Thor Thayer <ttha...@opensource.altera.com>

Some SPI masters require the slave to be selected before a
transaction can occur - even in the case of GPIO chip select.

This patch adds a GPIO slave select parameter to indicate
the slave needs to be selected in the GPIO CS case.

Signed-off-by: Thor Thayer <ttha...@opensource.altera.com>
---
v2  Add to SPI framework - add bindings.
---
 Documentation/devicetree/bindings/spi/spi-bus.txt |1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/spi/spi-bus.txt 
b/Documentation/devicetree/bindings/spi/spi-bus.txt
index 4b1d6e7..3a006bc 100644
--- a/Documentation/devicetree/bindings/spi/spi-bus.txt
+++ b/Documentation/devicetree/bindings/spi/spi-bus.txt
@@ -24,6 +24,7 @@ support describing the chip select layout.
 Optional properties:
 - cs-gpios   - gpios chip select.
 - num-cs - total number of chipselects.
+- gpio-ss- use internal slave select with gpio chip select.

 If cs-gpios is used the number of chip selects will be increased automatically
 with max(cs-gpios > hw cs).


To me, this looks more like a new flag the SPI controller driver should
set in spi_master.flags, instead of a DT property.

Yes, I see your point since this is SPI master specific. I'll respin as 
you suggest.


Thanks for reviewing!


Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Re: [PATCH 2/4] serial: 8250: of: Load TX FIFO Load Size from DT

[Thor Thayer]

Hi Rob,

On 09/16/2016 02:20 PM, Rob Herring wrote:

On Thu, Sep 08, 2016 at 11:12:19AM -0500, ttha...@opensource.altera.com wrote:

From: Thor Thayer <ttha...@opensource.altera.com>

Initialize the tx_loadsz parameter if it is defined in the
device tree.

Signed-off-by: Thor Thayer <ttha...@opensource.altera.com>
---
 drivers/tty/serial/8250/8250_of.c |6 ++
 1 file changed, 6 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_of.c 
b/drivers/tty/serial/8250/8250_of.c
index 38963d7..3e4be2f 100644
--- a/drivers/tty/serial/8250/8250_of.c
+++ b/drivers/tty/serial/8250/8250_of.c
@@ -195,6 +195,7 @@ static int of_platform_serial_probe(struct platform_device 
*ofdev)
switch (port_type) {
case PORT_8250 ... PORT_MAX_8250:
{
+   u32 prop;
struct uart_8250_port port8250;
memset(, 0, sizeof(port8250));
port8250.port = port;
@@ -202,6 +203,11 @@ static int of_platform_serial_probe(struct platform_device 
*ofdev)
if (port.fifosize)
port8250.capabilities = UART_CAP_FIFO;

+   /* Check for TX fifo load size */
+   if (of_property_read_u32(ofdev->dev.of_node,
+"tx-loadsz", ) == 0)
+   port8250.tx_loadsz = prop;


This can be simplified to:

of_property_read_u32(ofdev->dev.of_node, "tx-loadsz", _loadsz);


Yes, I originally had it implemented using that function but then I 
realized the port8250.tx_loadsz is an unsigned int and therefore can 
change size on different platforms.


The assignment handles that. In retrospect, I should probably change 
this to a of_property_read_u8() anyway so that it won't cause compile 
warnings in the smaller architectures.


Thank you for reviewing and the comments.

Thor


+
if (of_property_read_bool(ofdev->dev.of_node,
  "auto-flow-control"))
port8250.capabilities |= UART_CAP_AFE;
--
1.7.9.5

Re: [PATCH 1/4] Documentation: dt: serial: Add TX FIFO load size

[Thor Thayer]

Hi Rob,

On 09/16/2016 02:19 PM, Rob Herring wrote:

On Thu, Sep 08, 2016 at 11:12:18AM -0500, ttha...@opensource.altera.com wrote:

From: Thor Thayer <ttha...@opensource.altera.com>

Add the device tree bindings needed to support the TX FIFO
load size.

Signed-off-by: Thor Thayer <ttha...@opensource.altera.com>
---
 Documentation/devicetree/bindings/serial/8250.txt |1 +
 1 file changed, 1 insertion(+)

diff --git a/Documentation/devicetree/bindings/serial/8250.txt 
b/Documentation/devicetree/bindings/serial/8250.txt
index f5561ac..93de5ef 100644
--- a/Documentation/devicetree/bindings/serial/8250.txt
+++ b/Documentation/devicetree/bindings/serial/8250.txt
@@ -45,6 +45,7 @@ Optional properties:
 - {rts,cts,dtr,dsr,rng,dcd}-gpios: specify a GPIO for RTS/CTS/DTR/DSR/RI/DCD
   line respectively. It will use specified GPIO instead of the peripheral
   function pin for the UART feature. If unsure, don't specify this property.
+- tx-loadsz: the number of bytes that can be loaded into the TX FIFO.


It seems you don't really need to know the loadsz, but just need the
threshold trigger level. So just use that directly. And please check if
any other UARTs have threshold properties. i.MX has programmable
thresholds.

Rob



Yes, it is correct that I only really need the loadsz, but there is a 
tx-loadsz parameter in the uart_8250_port structure. Since there 
currently isn't a way to update the default value of tx-loadsz from 
device tree, I thought this would be useful for other 8250 UARTs as well.


Yes, there are a couple of 8250 compatible UARTs that have programmable 
thresholds. The SC16654 and SC16C650 are 2 examples. However, I didn't 
see the programmable TX threshold implemented.


I looked at the i.MX UARTs but I don't see a programmable threshold 
parameter in the bindings( 
Documentation/devicetree/bindings/serial/fsl-imx-uart.txt).


I'm assuming you're saying that I should use the same binding names for 
this transmit threshold if they are available?


Thank you for reviewing and for the comments.

Thor