From: Serge Semin <fancer.lancer@gmail.com> To: Borislav Petkov <bp@alien8.de> Cc: Serge Semin <Sergey.Semin@baikalelectronics.ru>, Michal Simek <michal.simek@xilinx.com>, Mauro Carvalho Chehab <mchehab@kernel.org>, Tony Luck <tony.luck@intel.com>, James Morse <james.morse@arm.com>, Robert Richter <rric@kernel.org>, Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>, Michail Ivanov <Michail.Ivanov@baikalelectronics.ru>, Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>, Punnaiah Choudary Kalluri <punnaiah.choudary.kalluri@xilinx.com>, Manish Narani <manish.narani@xilinx.com>, Dinh Nguyen <dinguyen@kernel.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH RESEND v3 14/17] EDAC/synopsys: Detach Zynq DDRC controller support Date: Sat, 8 Oct 2022 03:42:24 +0300 [thread overview] Message-ID: <20221008004224.owqbzbdctclua2hb@mobilestation> (raw) In-Reply-To: <Yz7XVqeopgGVR7+3@zn.tnic> On Thu, Oct 06, 2022 at 03:25:42PM +0200, Borislav Petkov wrote: > On Thu, Oct 06, 2022 at 03:17:40PM +0300, Serge Semin wrote: > > In general because it needlessly overcomplicates the driver, worsen > > it scalability, maintainability and readability, makes it much harder > > to add new controller features. Moreover even if you still able to add > > some more features support the driver will get to be more and more messy > > (as Michal correctly said in the original thread [1]). > > Did you read that thread until the end? Of course I did. Here is a short summary: 1. @Punnaiah described that he got a Zynq system with two different DDR controllers. One of them was Synopsys DW uMCTL2 DDRC (ARM cortex A9 PS) and another one - Zynq A05 DDRC (Xilinx PL). All of these DDR controllers could be probed in Linux. It was required to detect the ECC errors for both of them. 2. @Punnaiah suggested that both of these controllers should have been handled by two different drivers since the controllers were different. But he was reasonably afraid that there could be a race condition for the mc_num assignment since it was the drivers responsibility to allocate one. 3. @Punnaiah suggested two solutions: 1) Keep two drivers in single file and use static global variable for tracking the mc_num. 2) Let the framework assign the mc_num to the edac driver. 4. @Punnaiah preferred the solution 2, but you said that the solution 1 would do it. 5. @Michal was against mixing up the support of two different devices in a single driver. He reasonably assumed that the driver would get to look messy. 6. @Boris disagreed saying that it won't be messy and any communication between the two memory controllers could be done internally in that driver. 7. So you all decided to keep both controllers support in a single file, but would get back to a discussion of having separate drivers for them later. But after all these years of the Synopsys EDAC driver living in kernel we can conclude that combining the two different devices support in a single driver was pointless. First by the driver design nobody ever used the driver to handle both of them at the same time (MC index is fixed to zero). Second there is no even a glimpse of communications between two devices in the system. Third there are two different set of macros and multiple platform-specific methods and if-flag-else statements fully abstracting out the devices implementation from the EDAC core. All of that has made the driver overcomplicated in-vain seeing none of the reasons of these complications have been realised. Meanwhile extending further the Synopsys uMCTL2 DDR controller functionality support would mean adding new macros, functions, platform-specific flags, parameters and device-specific data. That would have made the driver even more complicated. Dropping the abstraction layer out and splitting up the drivers was the best choice in the current circumstances especially seeing the driver author and @Michal preferred that solution in the first place. Moreover in order to cover a still possible case of having both Synopsys uMCTL2 DDRC and Zynq A05 DDRC used on the same system in future I have implemented the solution 2. > > > It will get to be messy since you'll need to add more if-flag-else > > conditionals or define new device-specific callbacks to select the > > right code path for different controllers; you'll need to define > > some private data specific to one controller and unused in another. > > All of that you already have in the driver and all of that would be > > unneeded should the driver author have followed the standard kernel > > bus-device-driver model. > > So lemme ask this again because the last time it all sounded weird and I > don't think it got clarified fully: you cannot have more than one memory > controller type at the same time on those systems, can you? To be clear I don't work with the Xilinx systems. So I can only say based on your discussion with @Punnaiah on the initial driver review. @Punnaiah said they could have more than one memory controller type on their systems. That's why he described the problem with the MC indexes allocation and suggested to combine the devices support in a single driver. > > Because if you can and you want to support that, the current EDAC > "design" allows to have only a single EDAC driver per system. So you > can't do two drivers now. I do understand that. > > If your answer to that is > > Subject: [PATCH RESEND v3 13/17] EDAC/mc: Add MC unique index allocation procedure > > then I'm sceptical but I'd need to look at that first. Em, if there is something else which makes the EDAC drivers to be impossible to co-exist on the same system, then it greatly violates the bus-device-driver model. > > And reading your commit messages, you're talking a lot about what you're > doing. But that should be visible from the patch. What I wanna read is > *why* you're doing it. Each patchlog has a thorough enough description of "why". > > Like in this patch above, what's that "unique index allocation > procedure" for? Have you read the patchlog? > > edac_mc_alloc() already gets a mc_num as the MC number. > > And yes, if you want to do multiple driver instances like x86 does per > NUMA node instances, then that is done with edac_mc_alloc() which gives > you a memory controller descriptor and then you can deal with those. > See the "EDAC/mc: Add MC unique index allocation procedure" patch. It also provides a way to assign the indexes based on the OF-aliases. > From all the text it sounds to me you want to add a separate driver for > that Zynq A05 thing but I might still be missing something... I do want to detach the Zynq A05 device support from out of the Synopsys uMCTL2 driver. BTW have you read the cover letter? It contains a short summary of the changes and their justification. It seems as if you started your review straight from this patch. -Sergey > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette
WARNING: multiple messages have this Message-ID (diff)
From: Serge Semin <fancer.lancer@gmail.com> To: Borislav Petkov <bp@alien8.de> Cc: Serge Semin <Sergey.Semin@baikalelectronics.ru>, Michal Simek <michal.simek@xilinx.com>, Mauro Carvalho Chehab <mchehab@kernel.org>, Tony Luck <tony.luck@intel.com>, James Morse <james.morse@arm.com>, Robert Richter <rric@kernel.org>, Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>, Michail Ivanov <Michail.Ivanov@baikalelectronics.ru>, Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>, Punnaiah Choudary Kalluri <punnaiah.choudary.kalluri@xilinx.com>, Manish Narani <manish.narani@xilinx.com>, Dinh Nguyen <dinguyen@kernel.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Rob Herring <robh@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>, devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-edac@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH RESEND v3 14/17] EDAC/synopsys: Detach Zynq DDRC controller support Date: Sat, 8 Oct 2022 03:42:24 +0300 [thread overview] Message-ID: <20221008004224.owqbzbdctclua2hb@mobilestation> (raw) In-Reply-To: <Yz7XVqeopgGVR7+3@zn.tnic> On Thu, Oct 06, 2022 at 03:25:42PM +0200, Borislav Petkov wrote: > On Thu, Oct 06, 2022 at 03:17:40PM +0300, Serge Semin wrote: > > In general because it needlessly overcomplicates the driver, worsen > > it scalability, maintainability and readability, makes it much harder > > to add new controller features. Moreover even if you still able to add > > some more features support the driver will get to be more and more messy > > (as Michal correctly said in the original thread [1]). > > Did you read that thread until the end? Of course I did. Here is a short summary: 1. @Punnaiah described that he got a Zynq system with two different DDR controllers. One of them was Synopsys DW uMCTL2 DDRC (ARM cortex A9 PS) and another one - Zynq A05 DDRC (Xilinx PL). All of these DDR controllers could be probed in Linux. It was required to detect the ECC errors for both of them. 2. @Punnaiah suggested that both of these controllers should have been handled by two different drivers since the controllers were different. But he was reasonably afraid that there could be a race condition for the mc_num assignment since it was the drivers responsibility to allocate one. 3. @Punnaiah suggested two solutions: 1) Keep two drivers in single file and use static global variable for tracking the mc_num. 2) Let the framework assign the mc_num to the edac driver. 4. @Punnaiah preferred the solution 2, but you said that the solution 1 would do it. 5. @Michal was against mixing up the support of two different devices in a single driver. He reasonably assumed that the driver would get to look messy. 6. @Boris disagreed saying that it won't be messy and any communication between the two memory controllers could be done internally in that driver. 7. So you all decided to keep both controllers support in a single file, but would get back to a discussion of having separate drivers for them later. But after all these years of the Synopsys EDAC driver living in kernel we can conclude that combining the two different devices support in a single driver was pointless. First by the driver design nobody ever used the driver to handle both of them at the same time (MC index is fixed to zero). Second there is no even a glimpse of communications between two devices in the system. Third there are two different set of macros and multiple platform-specific methods and if-flag-else statements fully abstracting out the devices implementation from the EDAC core. All of that has made the driver overcomplicated in-vain seeing none of the reasons of these complications have been realised. Meanwhile extending further the Synopsys uMCTL2 DDR controller functionality support would mean adding new macros, functions, platform-specific flags, parameters and device-specific data. That would have made the driver even more complicated. Dropping the abstraction layer out and splitting up the drivers was the best choice in the current circumstances especially seeing the driver author and @Michal preferred that solution in the first place. Moreover in order to cover a still possible case of having both Synopsys uMCTL2 DDRC and Zynq A05 DDRC used on the same system in future I have implemented the solution 2. > > > It will get to be messy since you'll need to add more if-flag-else > > conditionals or define new device-specific callbacks to select the > > right code path for different controllers; you'll need to define > > some private data specific to one controller and unused in another. > > All of that you already have in the driver and all of that would be > > unneeded should the driver author have followed the standard kernel > > bus-device-driver model. > > So lemme ask this again because the last time it all sounded weird and I > don't think it got clarified fully: you cannot have more than one memory > controller type at the same time on those systems, can you? To be clear I don't work with the Xilinx systems. So I can only say based on your discussion with @Punnaiah on the initial driver review. @Punnaiah said they could have more than one memory controller type on their systems. That's why he described the problem with the MC indexes allocation and suggested to combine the devices support in a single driver. > > Because if you can and you want to support that, the current EDAC > "design" allows to have only a single EDAC driver per system. So you > can't do two drivers now. I do understand that. > > If your answer to that is > > Subject: [PATCH RESEND v3 13/17] EDAC/mc: Add MC unique index allocation procedure > > then I'm sceptical but I'd need to look at that first. Em, if there is something else which makes the EDAC drivers to be impossible to co-exist on the same system, then it greatly violates the bus-device-driver model. > > And reading your commit messages, you're talking a lot about what you're > doing. But that should be visible from the patch. What I wanna read is > *why* you're doing it. Each patchlog has a thorough enough description of "why". > > Like in this patch above, what's that "unique index allocation > procedure" for? Have you read the patchlog? > > edac_mc_alloc() already gets a mc_num as the MC number. > > And yes, if you want to do multiple driver instances like x86 does per > NUMA node instances, then that is done with edac_mc_alloc() which gives > you a memory controller descriptor and then you can deal with those. > See the "EDAC/mc: Add MC unique index allocation procedure" patch. It also provides a way to assign the indexes based on the OF-aliases. > From all the text it sounds to me you want to add a separate driver for > that Zynq A05 thing but I might still be missing something... I do want to detach the Zynq A05 device support from out of the Synopsys uMCTL2 driver. BTW have you read the cover letter? It contains a short summary of the changes and their justification. It seems as if you started your review straight from this patch. -Sergey > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-10-08 0:42 UTC|newest] Thread overview: 76+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-09-29 23:26 [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Serge Semin 2022-09-29 23:26 ` Serge Semin 2022-09-29 23:26 ` [PATCH RESEND v3 01/17] EDAC/synopsys: Fix native uMCTL2 IRQs handling procedure Serge Semin 2022-09-29 23:26 ` Serge Semin 2022-10-31 17:24 ` Borislav Petkov 2022-10-31 17:24 ` Borislav Petkov 2023-09-01 11:24 ` Serge Semin 2023-09-01 11:24 ` Serge Semin 2022-09-29 23:26 ` [PATCH RESEND v3 02/17] EDAC/synopsys: Fix generic device type detection procedure Serge Semin 2022-09-29 23:26 ` Serge Semin 2022-09-29 23:26 ` [PATCH RESEND v3 03/17] EDAC/synopsys: Fix mci->scrub_cap field setting Serge Semin 2022-09-29 23:26 ` Serge Semin 2022-09-29 23:26 ` [PATCH RESEND v3 04/17] EDAC/synopsys: Drop erroneous ADDRMAP4.addrmap_col_b10 parse Serge Semin 2022-09-29 23:26 ` Serge Semin 2022-09-29 23:27 ` [PATCH RESEND v3 05/17] EDAC/synopsys: Fix reading errors count before ECC status Serge Semin 2022-09-29 23:27 ` Serge Semin 2022-09-29 23:27 ` [PATCH RESEND v3 06/17] EDAC/synopsys: Use platform device devm ioremap method Serge Semin 2022-09-29 23:27 ` Serge Semin 2022-09-29 23:27 ` [PATCH RESEND v3 07/17] EDAC/synopsys: Drop internal CE and UE counters Serge Semin 2022-09-29 23:27 ` Serge Semin 2022-09-29 23:27 ` [PATCH RESEND v3 08/17] EDAC/synopsys: Drop local to_mci macro implementation Serge Semin 2022-09-29 23:27 ` Serge Semin 2022-09-29 23:27 ` [PATCH RESEND v3 09/17] EDAC/synopsys: Drop struct ecc_error_info.blknr field Serge Semin 2022-09-29 23:27 ` Serge Semin 2022-09-29 23:27 ` [PATCH RESEND v3 10/17] EDAC/synopsys: Shorten out struct ecc_error_info.bankgrpnr field name Serge Semin 2022-09-29 23:27 ` Serge Semin 2022-09-29 23:27 ` [PATCH RESEND v3 11/17] EDAC/synopsys: Drop redundant info from error message Serge Semin 2022-09-29 23:27 ` Serge Semin 2022-09-29 23:27 ` [PATCH RESEND v3 12/17] EDAC/mc: Init DIMM labels in MC registration method Serge Semin 2022-09-29 23:27 ` Serge Semin 2022-09-29 23:27 ` [PATCH RESEND v3 13/17] EDAC/mc: Add MC unique index allocation procedure Serge Semin 2022-09-29 23:27 ` Serge Semin 2022-10-12 17:29 ` Borislav Petkov 2022-10-12 17:29 ` Borislav Petkov 2022-10-12 20:01 ` Serge Semin 2022-10-12 20:01 ` Serge Semin 2022-10-12 20:33 ` Borislav Petkov 2022-10-12 20:33 ` Borislav Petkov 2022-10-12 20:44 ` Tony Luck 2022-10-12 20:44 ` Tony Luck 2022-10-12 21:31 ` Serge Semin 2022-10-12 21:31 ` Serge Semin 2022-10-12 22:30 ` Serge Semin 2022-10-12 22:30 ` Serge Semin 2022-10-13 9:38 ` Borislav Petkov 2022-10-13 9:38 ` Borislav Petkov 2022-09-29 23:27 ` [PATCH RESEND v3 14/17] EDAC/synopsys: Detach Zynq DDRC controller support Serge Semin 2022-09-29 23:27 ` Serge Semin 2022-09-30 14:42 ` Borislav Petkov 2022-09-30 14:42 ` Borislav Petkov 2022-10-06 12:17 ` Serge Semin 2022-10-06 12:17 ` Serge Semin 2022-10-06 13:25 ` Borislav Petkov 2022-10-06 13:25 ` Borislav Petkov 2022-10-08 0:42 ` Serge Semin [this message] 2022-10-08 0:42 ` Serge Semin 2022-10-12 17:28 ` Borislav Petkov 2022-10-12 17:28 ` Borislav Petkov 2022-10-12 19:27 ` Serge Semin 2022-10-12 19:27 ` Serge Semin 2022-10-12 19:44 ` Borislav Petkov 2022-10-12 19:44 ` Borislav Petkov 2022-10-12 20:55 ` Serge Semin 2022-10-12 20:55 ` Serge Semin 2022-09-29 23:27 ` [PATCH RESEND v3 15/17] EDAC/synopsys: Drop unused platform-specific setup API Serge Semin 2022-09-29 23:27 ` Serge Semin 2022-09-29 23:27 ` [PATCH RESEND v3 16/17] EDAC/synopsys: Unify the driver entities naming Serge Semin 2022-09-29 23:27 ` Serge Semin 2022-09-29 23:27 ` [PATCH RESEND v3 17/17] EDAC/synopsys: Convert to using BIT/GENMASK/FIELD_x macros Serge Semin 2022-09-29 23:27 ` Serge Semin 2022-09-30 14:29 ` [PATCH RESEND v3 00/17] EDAC/mc/synopsys: Various fixes and cleanups Borislav Petkov 2022-09-30 14:29 ` Borislav Petkov 2022-10-06 7:13 ` Michal Simek 2022-10-06 7:13 ` Michal Simek 2023-08-18 23:06 ` Serge Semin 2023-08-18 23:06 ` Serge Semin
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20221008004224.owqbzbdctclua2hb@mobilestation \ --to=fancer.lancer@gmail.com \ --cc=Alexey.Malahov@baikalelectronics.ru \ --cc=Michail.Ivanov@baikalelectronics.ru \ --cc=Pavel.Parkhomenko@baikalelectronics.ru \ --cc=Sergey.Semin@baikalelectronics.ru \ --cc=bp@alien8.de \ --cc=devicetree@vger.kernel.org \ --cc=dinguyen@kernel.org \ --cc=james.morse@arm.com \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=krzysztof.kozlowski@linaro.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-edac@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=manish.narani@xilinx.com \ --cc=mchehab@kernel.org \ --cc=michal.simek@xilinx.com \ --cc=punnaiah.choudary.kalluri@xilinx.com \ --cc=robh+dt@kernel.org \ --cc=robh@kernel.org \ --cc=rric@kernel.org \ --cc=tony.luck@intel.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.