Re: [PATCH RESEND v3 14/17] EDAC/synopsys: Detach Zynq DDRC controller support

[Borislav Petkov]

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?

> 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?

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.

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.

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.

Like in this patch above, what's that "unique index allocation
procedure" for?

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.

>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...

-- 
Regards/Gruss,
    Boris.

https://people.kernel.org/tglx/notes-about-netiquette