RE: [PATCH v3 7/8] EDAC/amd64: Support Asymmetric Dual-Rank DIMMs

From: "Ghannam, Yazen" <Yazen.Ghannam@amd.com>
To: Borislav Petkov <bp@alien8.de>
Cc: "linux-edac@vger.kernel.org" <linux-edac@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v3 7/8] EDAC/amd64: Support Asymmetric Dual-Rank DIMMs
Date: Fri, 23 Aug 2019 13:27:50 +0000	[thread overview]
Message-ID: <SN6PR12MB2639B5F8445D90154BA8860AF8A40@SN6PR12MB2639.namprd12.prod.outlook.com> (raw)
In-Reply-To: <20190823112628.GA28379@zn.tnic>

> -----Original Message-----
> From: linux-edac-owner@vger.kernel.org <linux-edac-owner@vger.kernel.org> On Behalf Of Borislav Petkov
> Sent: Friday, August 23, 2019 6:26 AM
> To: Ghannam, Yazen <Yazen.Ghannam@amd.com>
> Cc: linux-edac@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v3 7/8] EDAC/amd64: Support Asymmetric Dual-Rank DIMMs
> 
> On Thu, Aug 22, 2019 at 12:00:02AM +0000, Ghannam, Yazen wrote:
> > From: Yazen Ghannam <yazen.ghannam@amd.com>
> >
> > Future AMD systems will support "Asymmetric" Dual-Rank DIMMs. These are
> > DIMMs where the ranks are of different sizes.
> >
> > The even rank will use the Primary Even Chip Select registers and the
> > odd rank will use the Secondary Odd Chip Select registers.
> >
> > Recognize if a Secondary Odd Chip Select is being used. Use the
> > Secondary Odd Address Mask when calculating the chip select size.
> >
> > Signed-off-by: Yazen Ghannam <yazen.ghannam@amd.com>
> > ---
> > Link:
> > https://lkml.kernel.org/r/20190709215643.171078-8-Yazen.Ghannam@amd.com
> >
> > v2->v3:
> > * Add check of csrow_nr before using secondary mask.
> >
> > v1->v2:
> > * No change.
> >
> >  drivers/edac/amd64_edac.c | 18 +++++++++++++++---
> >  1 file changed, 15 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/edac/amd64_edac.c b/drivers/edac/amd64_edac.c
> > index 26ce48fcaf00..4d1e6daa7ec4 100644
> > --- a/drivers/edac/amd64_edac.c
> > +++ b/drivers/edac/amd64_edac.c
> > @@ -790,9 +790,13 @@ static void debug_dump_dramcfg_low(struct amd64_pvt *pvt, u32 dclr, int chan)
> >
> >  #define CS_EVEN_PRIMARY		BIT(0)
> >  #define CS_ODD_PRIMARY		BIT(1)
> > +#define CS_EVEN_SECONDARY	BIT(2)
> > +#define CS_ODD_SECONDARY	BIT(3)
> >
> > -#define CS_EVEN			CS_EVEN_PRIMARY
> > -#define CS_ODD			CS_ODD_PRIMARY
> > +#define CS_EVEN			(CS_EVEN_PRIMARY | CS_EVEN_SECONDARY)
> > +#define CS_ODD			(CS_ODD_PRIMARY | CS_EVEN_SECONDARY)
> 
> That's just my urge to have stuff ballanced but shouldn't that last line be:
> 
> #define CS_ODD			(CS_ODD_PRIMARY | CS_ODD_SECONDARY)
> 
> i.e., not have "even" as in CS_EVEN_SECONDARY in there but only "odd"s? :)
> 

Yes, sorry I missed that.

> > +#define csrow_sec_enabled(i, dct, pvt)	((pvt)->csels[(dct)].csbases_sec[(i)] & DCSB_CS_ENABLE)
> 
> I moved that to the header, under csrow_enabled().
> 

Okay, thank you.

-Yazen