On 07/30/2014 05:41 PM, Punnaiah Choudary Kalluri wrote: > Hi Boris, > >> -----Original Message----- >> From: Borislav Petkov [mailto:bp@xxxxxxxxx] >> Sent: Monday, July 28, 2014 11:32 PM >> To: Punnaiah Choudary Kalluri >> Cc: Punnaiah Choudary Kalluri; dougthompson@xxxxxxxxxxxx; >> robh+dt@xxxxxxxxxx; pawel.moll@xxxxxxx; Michal Simek; >> mark.rutland@xxxxxxx; ijc+devicetree@xxxxxxxxxxxxxx; Kumar Gala; Rob >> Landley; devicetree@xxxxxxxxxxxxxxx; linux-doc@xxxxxxxxxxxxxxx; linux- >> edac@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; linux-arm- >> kernel@xxxxxxxxxxxxxxxxxxx; Punnaiah Choudary; Anirudha Sarangi; Srikanth >> Vemula >> Subject: Re: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr >> ecc controller >> >> On Mon, Jul 28, 2014 at 10:53:26PM +0530, punnaiah choudary kalluri wrote: >>> I can agree with you that we can use shorter names.But ZYNQ has >>> programmable logic next to processing system where one can add soft >>> memory controller in the same system and may use different driver. So, >>> the edac driver for zynq may include multiple drivers for different >>> memory controllers in the same file. In this case it is difficult to >>> maintain generic macros as you suggested above. >> >> So? >> >> You can still shorten them a bit - the current names are awfully long. >> SYNPS_DDRC_ECC_ is too much information, for example. We know it is DDR >> and we know it is about ECC. So do SYNPS and ZYNQ or OTHER or whatever >> you wanna call it prefix and then the rest. I.e., >> >> SYNPS_<reg>_<bits*> >> ZYNQ_<reg>_bits*> >> >> You can even use single letter prefixes like S_ and Z_ and add a comment >> explaining what those mean. Still much more readable than the long >> repeating prefixes currently. > > Ok. >> >>> Also the current edac framework for edac memory controllers is >>> expecting the mc_num from the driver while allocating the memory >>> controller instance using the edac_mc_alloc function. This requirement >>> mandates that if the system contains two different memory controllers >>> then the corresponding edac drivers should be in single file. >> >> So you're telling me that you want one edac driver for *two* memory >> controllers which can be present on a single system *at* *the* *same* >> *time*? Is that it? > > Yes. >> >> How exactly is that topology supposed to look like, work, etc, etc? What >> kind of error reporting do you imagine you want to do with EDAC? > > Zynq (All programmable SOC) contains a dual core ARM cortex A9 based processing > System(PS) and Xilinx programmable logic(PL) in a single device. > > Assume the application is a broadcast camera. The design for this system use PS as > Control plane and use PL as data plane for processing the video data. So, the design > may have two different memory controllers one in PS and another one in PL. > PS is running with Linux OS and PL doesn't have the OS and it is controlled by the > OS running on PS. > Now the requirement is to develop a monitoring system for all the hardware failures > Including ddr ecc errors. Since the broadcast camera involves more data processing and > DDR memory access, there is a high probability of getting ddr ecc errors over the period. > So, the user should be intimated with these errors when they occur and if the error rate > is high, then the user can consider the preventive methods. Without this error reporting > mechanism it is difficult to debug the issues like memory corruption, kernel oops which > may occur due to ddr ecc failures. > > Since, the memory controllers are different, it need two edac drivers for reporting the ecc > errors and also maintaining the statistics of that particular memory controller. With the current > framework, there is a chance that both the drivers get mc_num as zero and malfunction. > Assume the code for the two drivers looks like below > > Driver 1: > mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, > sizeof(struct ctrl1_drvdata)); > > Driver 2: > mci = edac_mc_alloc(0, ARRAY_SIZE(layers), layers, > sizeof(struct ctrl2_drvdata)); > > Issue: > Since driver is providing the mc_num to framework, now there is chance that only one device active as > both the drivers claiming the same number. > > Solution 1: > Keep two drivers in single file and use static global variable for tracking the mc_num. This solution looks > good but the drivers may not be generic as these driver would be in a zynq_edac.c file. So others may not > reuse these drivers > static int mc_num = 0; > > Driver 1: > mci = edac_mc_alloc(mc_num, ARRAY_SIZE(layers), layers, > sizeof(struct ctrl1_drvdata)); > mc_num++; > > Driver 2: > mci = edac_mc_alloc(mc_num, ARRAY_SIZE(layers), layers, > sizeof(struct ctrl2_drvdata)); > mc_num++; > > Solution 2: > Let the framework assign the mc_num to the edac driver > > mc_num = edac_mc_get_id(); /* returns the next available mci slot */ > mci = edac_mc_alloc(mc_num,...); > > > In my opinion solution 2 looks neat and it eliminates the dependency on tracking > the mc_num. Isn't there also 3rd option which is provide mc_num via DTS? Thanks, Michal -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
