On Thu, Mar 02, 2023 at 05:24:16AM +0000, Datta, Shubhrajyoti wrote:
> > > +
> > > +#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.
$ grep -E "GENMASK\(5, 0\)" drivers/edac/xilinx_ddrmc_edac.c
#define RANK_0_MASK GENMASK(5, 0)
#define ROW_0_MASK GENMASK(5, 0)
#define ROW_5_MASK GENMASK(5, 0)
#define ROW_10_MASK GENMASK(5, 0)
#define ROW_15_MASK GENMASK(5, 0)
#define COL_1_MASK GENMASK(5, 0)
#define COL_6_MASK GENMASK(5, 0)
#define BANK_1_MASK GENMASK(5, 0)
Ditto for the other ones.
Can't you use a single
#define MASK_0 GENMASK(5, 0)
and then use MASK_0 everywhere?
And the same for the other ones?
Better yet, you can define a function which does that repeated block:
priv->row_bit[0] = regval & ROW_0_MASK;
priv->row_bit[1] = (regval & ROW_1_MASK) >> ROW_1_SHIFT;
priv->row_bit[2] = (regval & ROW_2_MASK) >> ROW_2_SHIFT;
priv->row_bit[3] = (regval & ROW_3_MASK) >> ROW_3_SHIFT;
priv->row_bit[4] = (regval & ROW_4_MASK) >> ROW_4_SHIFT;
in one:
static inline void process_bit(priv, unsigned int start, ... regval)
{
priv->row_bit[start] = regval & MASK_0;
priv->row_bit[start + 1] = (regval & MASK_1) >> ROW_1_SHIFT;
...
}
and then you don't have to define the same masks for every 5 bits but
have the function do it for ya, for each group of 5 bits?
>
> <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.
Let's take the first one. Isn't what you're doing equivalent to simply:
err_addr = pinf.row & GENMASK_ULL(0, XDDR_MAX_ROW_CNT);
err_addr <<= XDDR_MAX_ROW_CNT;
?
You basically stick in the error address each segment one after the
other...
err_addr = [XDDR_MAX_ROW_CNT ... XDDR_MAX_COL_CNT ... XDDR_MAX_BANK_CNT .. ]
and so on?
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
