[AMD Official Use Only - General]
HI Boris,
Thanks for the review.
> -----Original Message-----
> From: Borislav Petkov <bp@xxxxxxxxx>
> Sent: Monday, January 30, 2023 3:41 AM
> To: Datta, Shubhrajyoti <shubhrajyoti.datta@xxxxxxx>
> Cc: linux-edac@xxxxxxxxxxxxxxx; git (AMD-Xilinx) <git@xxxxxxx>;
> michal.simek@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> krzysztof.kozlowski@xxxxxxxxxx; robh+dt@xxxxxxxxxx; mchehab@xxxxxxxxxx;
> tony.luck@xxxxxxxxx
> Subject: Re: [PATCH v3 2/2] edac: xilinx: Added EDAC support for Xilinx DDR
> controller
>
<snip>
> > +
> > +#define PCSR_UNLOCK_VAL 0xF9E8D7C6
> > +#define XDDR_ERR_TYPE_CE 0
> > +#define XDDR_ERR_TYPE_UE 1
> > +
> > +#define XILINX_DRAM_SIZE_4G 0
> > +#define XILINX_DRAM_SIZE_6G 1
> > +#define XILINX_DRAM_SIZE_8G 2
> > +#define XILINX_DRAM_SIZE_12G 3
> > +#define XILINX_DRAM_SIZE_16G 4
> > +#define XILINX_DRAM_SIZE_32G 5
>
> Oh wow, that's a *lot* of defines!
>
> How about unifying them?
>
> All those rank masks look the same.
I did not understand the comment. Could you please clarify.
The size difference is not uniform so a offset jump strategy may not work.
<snip>
> > + ulong err_addr = 0;
> > + u32 index;
> > +
> > + for (index = 0; index < XDDR_MAX_ROW_CNT; index++) {
> > + err_addr |= (pinf.row & BIT(0)) << priv->row_bit[index];
> > + pinf.row >>= 1;
> > + }
> > +
> > + for (index = 0; index < XDDR_MAX_COL_CNT; index++) {
> > + err_addr |= (pinf.col & BIT(0)) << priv->col_bit[index];
> > + pinf.col >>= 1;
> > + }
> > +
> > + for (index = 0; index < XDDR_MAX_BANK_CNT; index++) {
> > + err_addr |= (pinf.bank & BIT(0)) << priv->bank_bit[index];
> > + pinf.bank >>= 1;
> > + }
> > +
> > + for (index = 0; index < XDDR_MAX_GRP_CNT; index++) {
> > + err_addr |= (pinf.group & BIT(0)) << priv->grp_bit[index];
> > + pinf.group >>= 1;
> > + }
> > +
> > + for (index = 0; index < XDDR_MAX_RANK_CNT; index++) {
> > + err_addr |= (pinf.rank & BIT(0)) << priv->rank_bit[index];
> > + pinf.rank >>= 1;
> > + }
> > +
> > + for (index = 0; index < XDDR_MAX_LRANK_CNT; index++) {
> > + err_addr |= (pinf.lrank & BIT(0)) << priv->lrank_bit[index];
> > + pinf.lrank >>= 1;
> > + }
>
> Oh wow, 6 loops!
>
> I'm wondering if you could "unroll" those loops and work on each component
> with a single mask and such...
I did not understand this one . The loop are running for different indixes.
>
> > +
