RE: [RFC PATCH v3] edac: synps: Added EDAC support for zynq ddr ecc controller

[Punnaiah Choudary Kalluri]

Hi Boris,

>-----Original Message-----
>From: Borislav Petkov [mailto:bp@xxxxxxxxx]
>Sent: Thursday, July 31, 2014 7:27 PM
>To: Michal Simek
>Cc: Punnaiah Choudary Kalluri; dougthompson@xxxxxxxxxxxx;
>robh+dt@xxxxxxxxxx; pawel.moll@xxxxxxx; 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 Thu, Jul 31, 2014 at 03:36:35PM +0200, Michal Simek wrote:
>> Mixing functions for two/more different controller seems to me
>> really messy.
>
>Just stating that something is "really messy" without giving at least
>one technical reason for it is not going to get you any further. So let
>me give you mine:
>
>I think it is not messy because one file contains all the EDAC code for
>that system. Like every other system out there uses one EDAC driver.
>Everything is nicely contained in one place, any communication between
>the two memory controllers is done internally in that driver.
>
>Having two drivers would make that much more complicated. But hey, if
>you want to make your life more complicated because it is not enough
>complicated, feel free to do so. Just make sure you don't impose any
>restrictions on the rest of the subsystem.

Here we are also considering reusability of the driver. Let's say if some other
SOC contains Synopsys ddr controller then this driver can be reused. In this case,
Having separate driver would be good. But as you said in your previous comment,
We will talk about this later. 

Thanks,
Punnaiah
>
>> > -ENOPARSE for this sentence.
>>
>> I meant by this this scenario - please correct me if I am completely wrong
>> because I am not playing with this subsystem. My expectation is that
>> memory controller number should be same all the time.
>>
>> two memory controllers with the same edac driver.
>>
>> Normal calling sequence:
>> X probe - number 0
>> Y probe - number 1
>>
>> Different order/name (I think order is taken at the first place)
>> in DTS should caused that X will have number 1 and Y number 0.
>>
>> Another scenario:
>> two controllers and one with external gpio reset for example.
>>
>> X probe is called but gpio controller is not ready and gpio driver returns
>> -EPROBE_DEFER then Y probe is called and get number 0. Then X probe is
>called
>> again and get number 1.
>
>Right, since you have a heterogeneous design, you'd need to account
>for all that in your driver and deal with probing order. Like having a
>way to ask a controller what kind of a controller it is and so on. I'm
>afraid I don't know anything about your design to be of more help there.
>
>However, doing this in one <name>_edac.c file is almost trivial than
>doing it with two separate drivers. Unless you don't need to do any
>meaningful communication between the PS and PL. Then it doesn't matter.
>
>If I were you, I'd spend a more time on a proper design now so that all
>my work later is easy. Adding two drivers just because it seems messy is
>not a very good idea. What I would do is analyze all my use cases and
>come up with a proper design then. But this is just me.
>
>HTH.
>
>--
>Regards/Gruss,
>    Boris.
>
>Sent from a fat crate under my desk. Formatting is fine.
>--