Re: [PATCH v7 3/5] Documentation: Add documentation for the APM X-Gene SoC EDAC DTS binding

Rob Herring
2015-04-29T23:02:14Z

On Wed, Apr 29, 2015 at 5:08 PM, Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org> wrote:
> On Wed, Apr 29, 2015 at 02:56:25PM -0700, Loc Ho wrote:
>> Hi,
>>
>> >> > Similar comments for the rest. I would define memory controller
>> >> > bindings and EDAC driver, then worry about the rest.
>> >>
>> >> Okay.. As comment in following emails, I will break up the driver into
>> >> multiple drivers and focus only on the memory controller driver first.
>> >
>> > Please no multiple EDAC drivers. Or do you mean something else here?
>>
>> We will have the following:
>>
>> xgene-edac-mc.c
>> xgene-edac-pmd.c
>> xgene-edac-l3.c
>> xgene-edac-soc.c
>>
>> Or what would you suggest.
>
> xgene-edac.c
>
> This granularity is insane. On x86 we have single EDAC drivers for
> multiple CPU families and platforms.

It may make sense on x86 as there are only 2 vendors, some level of
architecture definition at least within a vendor, some commonality
among generations, and some amount of abstraction by ACPI. For the
most part none of that applies to ARM based systems (although that is
changing).

> You can start carving out stuff when this is absolutely needed, i.e.
> when an IP block is in multiple hw incarnations. And even then I'd be
> sceptical and would really want to know whether it is even worth it to
> have an EDAC module for it.

One platform device corresponds to 1 DT node which corresponds to 1
h/w block. This is a well defined pattern throughout the kernel. There
are cases of grouping, but those are places where we need coordination
between h/w blocks such as audio and display and that is still
multiple platform devices with a single parent device. If you want
this all in 1 file or 1 module that is fine. I think that it is silly
to group otherwise independent things together and generally not what
we do anywhere else in the kernel. They all likely have different
capabilities and control mechanisms.

Rob