Re: [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver

[Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>]

On Wednesday 06 May 2015 11:43:36 Loc Ho wrote:
> Hi Borislav,
> 
> On Wed, May 6, 2015 at 11:29 AM, Borislav Petkov <bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org> wrote:
> > On Wed, May 06, 2015 at 11:12:20AM -0700, Loc Ho wrote:
> >> 1. Whether to have an single driver for APM EDAC or multiple instance
> >> of 4 different drivers. With single driver, it does not scale in the
> >> future when we add/remove memory controllers and CPU domains. This is
> >
> > Why doesn't it scale? Please explain this to me more verbosely.
> 
> Let me explain a bit more. We have four memory controller today.
> Therefore, I would like to have 4 DTS node and the same driver probe
> function called 4 times. If there is only one driver for the entire
> APM EDAC, I would have to merge all the resource registers into an
> single DTS node and its probe function called one time. In this one
> driver design, what would I do in future chip or variant of the chip
> in which it remove or add an addition memory controller? I would have
> to change the driver as well as the DTS node. In the four instance
> probe design, all I need is to add an additional DTS node.

There are lots of possible representations in DT that would solve this.

First of all, a device node can be turned into a platform_device but
does not have to, so you you could have one DT node for the common
parts (the pcp), and then subnodes underneath it, and have the driver
attach to the main device but parse the subnodes manually to see
what registers are there. There is no magic involved here, and this
would address the concerns that Boris and I have.

Another option would be to have multiple drivers: have one driver
that handles the common parts here, and make that export a shared
interface to which the other drivers can register, then create
five drivers that each do one thing. In my opinion that would work
just as well, and be a nice abstraction, but I suspect that Boris
would prefer doing it the other way.


> >> also agreed by Rob Herring from review of the DTS nodes. For L3 and
> >> SoC EDAC, they are less of an issue as I don't see a situation that we
> >> would have multiple instances.
> >>
> >> 2. With regard to the top level PCP interrupt, they are just for
> >> status and once configured, it will not be touch. Therefore, I keep
> >> the current implementation. With an single driver, there is no need to
> >> worry about read/modify/write as it will be guarded with an lock. For
> >> multiple instance, I am thinking that the xgene_edac_mc module will
> >> provide exported lock functions for the other drivers.
> >
> > Doing this would be a very bad design and it would be a homegrown case
> > only for this driver. Then other ARM drivers will appear which will do
> > their own locking too. No no no. No ad-hoc hackery.

I don't think there is a huge risk that other ARM drivers would copy a bad
design, when it's obviously bad. In particular, other chips will most
likely have other requirements in this area.

> What if I move the locking function into a common module to be
> included by the EDAC framework? Or would you prefer that I go and
> write an common driver that all it does is provide locking?
> 
> Any suggestion as all I need is an way to share access to an CSR which
> can't use atomic operations? It isn't an actual interrupt controller
> either.

If this is something that does not fit into existing categories, you
probably want to create your own high-level abstraction between the
accesses to the shared parts of the driver and the parts that can
exist in multiple instances. This will work for either of the approaches
I've described above.

	Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html