Linux-EDAC Archive on lore.kernel.org
 help / color / Atom feed
From: Yash Shah <yash.shah@openfive.com>
To: Palmer Dabbelt <palmer@dabbelt.com>,
	Christoph Hellwig <hch@infradead.org>,
	"dkangude@cadence.com" <dkangude@cadence.com>
Cc: "robh+dt@kernel.org" <robh+dt@kernel.org>,
	"Paul Walmsley ( Sifive)" <paul.walmsley@sifive.com>,
	"bp@alien8.de" <bp@alien8.de>,
	"mchehab@kernel.org" <mchehab@kernel.org>,
	"tony.luck@intel.com" <tony.luck@intel.com>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"aou@eecs.berkeley.edu" <aou@eecs.berkeley.edu>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	Sachin Ghadi <sachin.ghadi@openfive.com>,
	"rrichter@marvell.com" <rrichter@marvell.com>,
	"james.morse@arm.com" <james.morse@arm.com>,
	"linux-riscv@lists.infradead.org"
	<linux-riscv@lists.infradead.org>,
	"linux-edac@vger.kernel.org" <linux-edac@vger.kernel.org>
Subject: RE: [PATCH v2 2/3] soc: sifive: Add SiFive specific Cadence DDR controller driver
Date: Wed, 9 Sep 2020 03:56:54 +0000
Message-ID: <BN6PR1301MB20201D1CF5DE19A48306D28182260@BN6PR1301MB2020.namprd13.prod.outlook.com> (raw)
In-Reply-To: <mhng-d2a95187-c772-4c5d-b30b-b053a3195177@palmerdabbelt-glaptop1>

> -----Original Message-----
> From: Palmer Dabbelt <palmer@dabbelt.com>
> Sent: 09 September 2020 08:42
> To: Christoph Hellwig <hch@infradead.org>; dkangude@cadence.com
> Cc: Yash Shah <yash.shah@openfive.com>; robh+dt@kernel.org; Paul
> Walmsley ( Sifive) <paul.walmsley@sifive.com>; bp@alien8.de;
> mchehab@kernel.org; tony.luck@intel.com; devicetree@vger.kernel.org;
> aou@eecs.berkeley.edu; linux-kernel@vger.kernel.org; Sachin Ghadi
> <sachin.ghadi@openfive.com>; rrichter@marvell.com;
> james.morse@arm.com; linux-riscv@lists.infradead.org; linux-
> edac@vger.kernel.org
> Subject: Re: [PATCH v2 2/3] soc: sifive: Add SiFive specific Cadence DDR
> controller driver
> 
> [External Email] Do not click links or attachments unless you recognize the
> sender and know the content is safe
> 
> On Sun, 06 Sep 2020 23:11:26 PDT (-0700), Christoph Hellwig wrote:
> > On Mon, Sep 07, 2020 at 11:17:58AM +0530, Yash Shah wrote:
> >> Add a driver to manage the Cadence DDR controller present on SiFive
> >> SoCs At present the driver manages the EDAC feature of the DDR
> controller.
> >> Additional features may be added to the driver in future to control
> >> other aspects of the DDR controller.
> >
> > So if this is a generic(ish) Cadence IP block shouldn't it be named
> > Cadence and made generic?  Or is the frontend somehow SiFive specific?
> 
> For some reason I thought we had a SiFive-specific interface to this, but I may
> have gotten that confused with something else as it's been a while.  Someone
> from SiFive would probably have a better idea, but it looks like the person I'd
> ask isn't thereany more so I'm all out of options ;)
> 
> It looks like there was a very similar driver posted by Dhananjay Kangude
> from Cadence in April: https://lkml.org/lkml/2020/4/6/358 .  Some of the
> register definitions seem to be different, but the code I looked at is very
> similar so there's at least some bits that could be shared.  I found a v4 of that
> patch set, but that was back in May: https://lkml.org/lkml/2020/5/11/912 .  It
> alludes to a v5, but I can't find one.  I've added Dhananjay, maybe he knows
> what's up?
> 

I consulted with Dhananjay before posting this patch. From what I understood, Cadence provide highly configurable and customised DDR IP blocks based on the SoC vendor's need. This impacts the register configuration and probably the offsets too.
I had also refer the v4 patch posted by Dhananjay mentioned above and found that the registers offsets are not matching with that of Cadence DDR IP in SiFive SoC. Therefore it seems this DDR IP block has SiFive specific configurations and hence this Sifive specific driver.

> I don't know enough about the block to know if the subtle difference in
> register names/offsets means.  They look properly jumbled up (ie, not just an
> offset), so maybe there's just different versions or that's the SiFive-specific
> part I had bouncing around my head?  Either way, it seems like one driver
> with some simple configuration could handle both of these -- either sticking
> the offsets in the DT (if they're going to be different everywhere) or by
> coming up with some version sort of thing (if there's a handful of these).
> 
> I'm now also a bit worried about the provenace of this code.  The two drivers
> are errily similar -- for example, the variable definitions in handle_ce()
> 
>        u64 err_c_addr = 0x0;
>        u64 err_c_data = 0x0;
>        u32 err_c_synd, err_c_id;
>        u32 sig_val_l, sig_val_h;
> 
> are exactly the same.

I apologized, I forgot to mention it in cover-letter. I have based my work on Dhananjay's v4 patch[0]. 

- Yash

[0]: https://lkml.org/lkml/2020/4/24/183

  reply index

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-07  5:47 [PATCH v2 0/3] SiFive DDR controller and EDAC support Yash Shah
2020-09-07  5:47 ` [PATCH v2 1/3] dt-bindings: riscv: Add DT documentation for DDR Controller in SiFive SoCs Yash Shah
2020-09-15 15:24   ` Rob Herring
2020-09-07  5:47 ` [PATCH v2 2/3] soc: sifive: Add SiFive specific Cadence DDR controller driver Yash Shah
2020-09-07  5:54   ` Randy Dunlap
2020-09-07  6:11   ` Christoph Hellwig
2020-09-09  3:12     ` Palmer Dabbelt
2020-09-09  3:56       ` Yash Shah [this message]
2020-09-09  6:00       ` Christoph Hellwig
2020-09-09 20:31         ` Palmer Dabbelt
2020-09-17  9:56       ` Dhananjay Vilasrao Kangude
2020-09-07  5:47 ` [PATCH v2 3/3] EDAC/sifive: Add EDAC support for Memory Controller in SiFive SoCs Yash Shah
2020-09-23 17:10   ` Borislav Petkov
2020-09-15 15:22 ` [PATCH v2 0/3] SiFive DDR controller and EDAC support Rob Herring

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=BN6PR1301MB20201D1CF5DE19A48306D28182260@BN6PR1301MB2020.namprd13.prod.outlook.com \
    --to=yash.shah@openfive.com \
    --cc=aou@eecs.berkeley.edu \
    --cc=bp@alien8.de \
    --cc=devicetree@vger.kernel.org \
    --cc=dkangude@cadence.com \
    --cc=hch@infradead.org \
    --cc=james.morse@arm.com \
    --cc=linux-edac@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-riscv@lists.infradead.org \
    --cc=mchehab@kernel.org \
    --cc=palmer@dabbelt.com \
    --cc=paul.walmsley@sifive.com \
    --cc=robh+dt@kernel.org \
    --cc=rrichter@marvell.com \
    --cc=sachin.ghadi@openfive.com \
    --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: link

Linux-EDAC Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-edac/0 linux-edac/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-edac linux-edac/ https://lore.kernel.org/linux-edac \
		linux-edac@vger.kernel.org
	public-inbox-index linux-edac

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-edac


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git