From mboxrd@z Thu Jan 1 00:00:00 1970 From: Loc Ho Subject: Re: [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver Date: Wed, 6 May 2015 11:43:36 -0700 Message-ID: References: <1430884947-16787-1-git-send-email-lho@apm.com> <2150970.xGYLhDU9BR@wuerfel> <20150506182900.GI22949@pd.tnic> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Return-path: In-Reply-To: <20150506182900.GI22949-fF5Pk5pvG8Y@public.gmane.org> Sender: devicetree-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Borislav Petkov Cc: Arnd Bergmann , Doug Thompson , Mauro Carvalho Chehab , Rob Herring , Mark Rutland , Ian Campbell , linux-edac-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, "devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org" , "linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org" , Jon Masters , "patches-qTEPVZfXA3Y@public.gmane.org" List-Id: devicetree@vger.kernel.org Hi Borislav, On Wed, May 6, 2015 at 11:29 AM, Borislav Petkov 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. > >> 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. > 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. -Loc -- 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: lho@apm.com (Loc Ho) Date: Wed, 6 May 2015 11:43:36 -0700 Subject: [PATCH v8 0/4] edac: Add APM X-Gene SoC memory controller EDAC driver In-Reply-To: <20150506182900.GI22949@pd.tnic> References: <1430884947-16787-1-git-send-email-lho@apm.com> <2150970.xGYLhDU9BR@wuerfel> <20150506182900.GI22949@pd.tnic> Message-ID: To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Hi Borislav, On Wed, May 6, 2015 at 11:29 AM, Borislav Petkov 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. > >> 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. > 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. -Loc