Messages in this thread | | | Date | Thu, 2 Apr 2020 09:20:06 +0200 | From | Robert Richter <> | Subject | Re: [PATCH v2 4/4] EDAC: synopsys: Add useful debug and output information for 64bit systems |
| |
On 01.04.20 14:32:58, Sherry Sun wrote: > > From: Robert Richter <rrichter@marvell.com> > > On 01.04.20 15:39:09, Sherry Sun wrote:
> > > --- a/drivers/edac/synopsys_edac.c > > > +++ b/drivers/edac/synopsys_edac.c > > > @@ -278,18 +278,22 @@ > > > * @col: Column number. > > > * @bank: Bank number. > > > * @bitpos: Bit position. > > > - * @data: Data causing the error. > > > + * @data_low: Low bit data causing the error. > > > + * @data_high: High bit data causing the error(used for 64 bit > > systems). > > > * @bankgrpnr: Bank group number. > > > * @blknr: Block number. > > > + * @syndrome: Syndrome of the error. > > > */ > > > struct ecc_error_info { > > > u32 row; > > > u32 col; > > > u32 bank; > > > u32 bitpos; > > > - u32 data; > > > + u32 data_low; > > > + u32 data_high; > > > > Where are 16, 32 and 64 bit widths. You could handle them all the same in a > > u64. > > > > If I am not wrong, the width is fix for the whole mci. So you could create > > various .get_error_info() functions depending on the data width without run > > time width checks. > > > > Yes, here we can use u64. Will correct it in v3. > For the zynq, it's 16-bit bus width, for imx8mp, it's 64-bit bus width. > But for zynqmp, it's optional between 32-bit and 64-bit bus width, > So we had better run time check the bus width, what do you think?
I am wondering a bit here. *get_dtype() is used only in init_csrows() which sets up the mci. So it will be a fix value all the time after init. With that you easily can setup functions depending on the width, alternatively the width could be stored in struct synps_platform_data or struct synps_edac_priv.
> > > > u32 bankgrpnr; > > > u32 blknr; > > > + u32 syndrome; > > > };
> > @@ -480,20 > > > +559,27 @@ static void handle_error(struct mem_ctl_info *mci, struct > > > synps_ecc_status *p) { > > > struct synps_edac_priv *priv = mci->pvt_info; > > > struct ecc_error_info *pinf; > > > + int n; > > > > > > if (p->ce_cnt) { > > > pinf = &p->ceinfo; > > > if (priv->p_data->quirks & DDR_ECC_INTR_SUPPORT) { > > > - snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, > > > - "DDR ECC error type:%s Row %d Bank %d > > BankGroup Number %d Block Number %d Bit Position: %d Data: 0x%08x", > > > - "CE", pinf->row, pinf->bank, > > > - pinf->bankgrpnr, pinf->blknr, > > > - pinf->bitpos, pinf->data); > > > + n = snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, > > > + "DDR ECC error type:%s Row %d Bank %d > > BankGroup Number %d Block Number %d Bit Position: %d Data: 0x%08x", > > > + "CE", pinf->row, pinf->bank, > > > + pinf->bankgrpnr, pinf->blknr, > > > + pinf->bitpos, pinf->data_low); > > > + > > > + if (zynqmp_get_dtype(priv->baseaddr) == DEV_X8) > > > > This is zynqmp specific, right? but you call it in the generic function > > handle_error(). > > Yes, although handle_error() is a generic function, > but this operation is checked by priv->p_data->quirks, > so here is zynqmp and imx8mp specific.
Naah, this is a bit hacky, just make 2 variants of handlers and set them up during init. Storing the width in some private data would be an alternative.
Thanks,
-Robert
> > Best regards > Sherry Sun > > > > > -Robert > > > > > + snprintf(priv->message + n, > > > + SYNPS_EDAC_MSG_SIZE - n, > > > + " Data_high: 0x%08x Syndrome: > > 0x%08x", > > > + pinf->data_high, pinf->syndrome); > > > } else { > > > snprintf(priv->message, SYNPS_EDAC_MSG_SIZE, > > > "DDR ECC error type:%s Row %d Bank %d > > Col %d Bit Position: %d Data: 0x%08x", > > > "CE", pinf->row, pinf->bank, pinf->col, > > > - pinf->bitpos, pinf->data); > > > + pinf->bitpos, pinf->data_low); > > > } > > > > > > edac_mc_handle_error(HW_EVENT_ERR_CORRECTED, mci,
| |