On Fri, Sep 30, 2022 at 04:42:31PM +0200, Borislav Petkov wrote: > On Fri, Sep 30, 2022 at 02:27:09AM +0300, Serge Semin wrote: > > It was a bad idea in the first place to combine two absolutely different > > controllers support in a single driver [1]. It caused having an additional > > level of abstraction, which obviously have needlessly overcomplicated the > > driver and as such caused many problems in the new main controller > > features support implementation. The solution looks even more unreasonable > > now seeing the justification of having both controllers support in a > > single driver hasn't been implemented by the original code author [2]. > > Yeah, no, you need to give more concrete details here. > > Why exactly is this a problem? 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]). 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. Regarding this particular case. This series and two more patchsets mentioned in the cover letter are about extending the Synopsys uMCTL2 DDRC controller functionality support in the driver. Here is just the most significant part of the being added features: 1. DW uCMTL2 IP-core parameters auto-detection (DDR bus-width, frequency ratio, burst-length, ECC-mode, etc). 2. Common SDRAM<->phys address translation. 3. Multi-ranked memory support. 4. ECC-scrubber and Scrubber IRQ support. 5. Individual IRQ signals support. 6. DFI alert_n IRQ support. 7. Add reference clock sources support. None of the above can be utilized for the Zynq A05 DDR controller, but is of the Synopsys uMCTL2 DDRC specific. Seeing the controllers are not even of the same vendor and are absolutely incompatible, considering all the new features, moving the Zynq A05 DDRC support away to another driver was the best choice in order to create a coherent and easy-to-read code, provide simple-enough patches for review. The standard kernel bus-device-driver model is perfectly suitable to differentiate these controllers. There is no point in creating an additional abstraction. > > Are you saying that if synopsys puts out 10 incompatible memory > controllers, we should have 10 separate EDAC drivers? I said "absolutely different controllers" in the patchlog. The level of incompatibility can be different and can vary from case to case. In this particular case there is nothing in common in Synopsys uMCTL2 DDRC and Zynq A05 DDRC. So if there are two absolutely different devices at consideration then in general they should be handled in different drivers as per the kernel bus-device-driver model. Note having absolutely different devices detectable on the same platform doesn't mean they should be handled by the same driver otherwise the kernel would have had tons of common platform-drivers instead of the device-specific ones. Even in current synopsys EDAC driver state (without my patches applied) should you detach the Zynq A05 DDRC support to another driver, the code would have got to be much easier to read and maintain. I one more time draw your attention to the fact that the original reason of having both Synopsys uMCTL2 and Zynq A05 DDRC support in the same driver has never been implemented [2]. See edac_mc_alloc() is always called with index zero in the driver. Even despite of that I still provide a way to solve the problem denoted in [2] in the patch "EDAC/mc: Add MC unique index allocation procedure" in this patchset. > > Hell no. > > synopsys_edac.c is not a huge file and the probe logic which matches > which synps_platform_data to load is pretty straight-forward to me. The synps_platform_data interface is so abstractive so you could have added any basic EDAC device support to the driver. It doesn't mean you should even if the devices can be detected on the same platform, especially if you expect the code to evolve further being extended with new more complex features. > > But maybe I'm missing something so please explain in detail what the > actual problems with this are? In this case I'd say the KISS principle applies. Simplification will be reached by splitting the driver up only. I understand that it couldn't have been done back then due to the original driver author claiming the EDAC core to require the MC auto-index generation [2]. That's why I have added such functionality in the framework of this series. [1] https://lore.kernel.org/all/808655a9-77eb-4e3a-9781-2b059ad9517b@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ [2] https://lore.kernel.org/all/9dc2a947-d2ab-4f00-8ed3-d2499cb6fdfd@xxxxxxxxxxxxxxxxxxxxxxxxxxxxx/ -Sergey > > Thx. > > -- > Regards/Gruss, > Boris. > > https://people.kernel.org/tglx/notes-about-netiquette
