Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1300997 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC  Support Borislav Petkov <bp@alien8.de> - 2016-01-04 20:50 +0100
    Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC  Support Borislav Petkov <bp@alien8.de> - 2016-01-04 21:40 +0100
      Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC  Support Borislav Petkov <bp@alien8.de> - 2016-01-04 22:00 +0100
        Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC  Support Borislav Petkov <bp@alien8.de> - 2016-01-04 22:10 +0100
          Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC  Support Borislav Petkov <bp@alien8.de> - 2016-01-04 23:10 +0100
            Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC  Support Borislav Petkov <bp@alien8.de> - 2016-01-05 12:00 +0100
              Re: [PATCHv7] EDAC, altera: Add Altera L2 Cache and OCRAM EDAC  Support Borislav Petkov <bp@alien8.de> - 2016-01-05 22:20 +0100

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

On Mon, Jan 04, 2016 at 11:17:29AM -0600, Dinh Nguyen wrote:
> We tried to jam the L2 and OCRAM EDAC functionality in the same
> altr_edac.c file. It looks like it might be clean if we split out the L2
> and OCRAM functions into their appropriate files(altr_edac_l2.c and
> altr_edac_ocram.c). Do you agree?

"Clean" in what sense? To me clean is when there's a single compilation
unit altera_edac.c which contains all Altera-specific code.

> These bindings were in a separate patch that you were not CC'd. I'll
> keep you on the entire patch set in the future.

Yes, I believe the devicetree definitions need to go hand-in-hand with
its user(s).

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1301066

On Mon, Jan 04, 2016 at 02:04:08PM -0600, Dinh Nguyen wrote:
> altr_edac.c originally added support for SDRAM. Now we're adding support
> for L2 and OCRAM into the same file by using #ifdef
> CONFIG_EDAC_ALTERA_OCRAM and CONFIG_EDAC_ALTERA_L2C. So "clean" was to
> move the l2 and ocram implementation into separate files.

So what's wrong with adding those to the same file and using different
registration functions? Like xgene_edac does, for example.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1301084

On Mon, Jan 04, 2016 at 02:46:26PM -0600, Dinh Nguyen wrote:
> I don't see a way for the xgene to manually build for each configuration
> using Kconfig? For SoCFGPA, we would like to keep the option to build
> for each type of ECC.

xgene builds everything in and unconditionally.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1301096

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

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1301138

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.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1301432

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.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1302218

On Tue, Jan 05, 2016 at 02:37:48PM -0600, Thor Thayer wrote:
> 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?

Sounds good to me.

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

Thanks.

-- 
Regards/Gruss,
    Boris.

ECO tip #101: Trim your mails when you reply.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web