[AMD Official Use Only - General]
Hi Boris,
Thanks for the review.
> -----Original Message-----
> From: Borislav Petkov <bp@xxxxxxxxx>
> Sent: Monday, July 17, 2023 1:49 PM
> To: Datta, Shubhrajyoti <shubhrajyoti.datta@xxxxxxx>
> Cc: linux-edac@xxxxxxxxxxxxxxx; git (AMD-Xilinx) <git@xxxxxxx>;
> krzysztof.kozlowski@xxxxxxxxxx; devicetree@xxxxxxxxxxxxxxx;
> tony.luck@xxxxxxxxx; james.morse@xxxxxxx; mchehab@xxxxxxxxxx;
> rric@xxxxxxxxxx; Simek, Michal <michal.simek@xxxxxxx>;
> sboyd@xxxxxxxxxx; robh+dt@xxxxxxxxxx; krzysztof.kozlowski+dt@xxxxxxxxxx;
> conor+dt@xxxxxxxxxx
> Subject: Re: [PATCH v7 2/2] EDAC/versal: Add a Xilinx Versal memory
> controller driver
>
> Caution: This message originated from an External Source. Use proper
> caution when opening attachments, clicking links, or responding.
>
>
> On Wed, Jun 14, 2023 at 09:58:52AM +0530, Shubhrajyoti Datta wrote:
> > MAINTAINERS | 7 +
> > drivers/edac/Kconfig | 11 +
> > drivers/edac/Makefile | 1 +
> > drivers/edac/versal_edac.c | 1065 ++++++++++++++++++++++++++
> > include/linux/firmware/xlnx-zynqmp.h | 10 +
> > 5 files changed, 1094 insertions(+)
> > create mode 100644 drivers/edac/versal_edac.c
>
> I've done some changes ontop, see below:
>
> That toggling of interrupts at the end of mc_probe() happens only for
> debugfs's sake so they can move inside the ifdef.
>
> Right?
>
> ---
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 4f3514e8116a..569c48368458 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -23410,7 +23410,7 @@ M: Shubhrajyoti Datta
> <shubhrajyoti.datta@xxxxxxx>
> M: Sai Krishna Potthuri <sai.krishna.potthuri@xxxxxxx>
> S: Maintained
> F: Documentation/devicetree/bindings/memory-controllers/xlnx,versal-
> ddrmc-edac.yaml
> -F: drivers/edac/xilinx_ddrmc_edac.c
> +F: drivers/edac/versal_edac.c
>
> XILINX WATCHDOG DRIVER
> M: Srinivas Neeli <srinivas.neeli@xxxxxxx>
> diff --git a/drivers/edac/versal_edac.c b/drivers/edac/versal_edac.c index
> 4aa073ffa827..07a07641172c 100644
> --- a/drivers/edac/versal_edac.c
> +++ b/drivers/edac/versal_edac.c
> @@ -195,7 +195,7 @@ union edac_info {
> struct ecc_status {
> union ecc_error_info ceinfo[2];
> union ecc_error_info ueinfo[2];
> - bool channel;
> + u8 channel;
> u8 error_type;
> };
>
> @@ -637,38 +637,38 @@ static void mc_init(struct mem_ctl_info *mci, struct
> platform_device *pdev)
> init_csrows(mci);
> }
>
> -static void enable_intr(struct edac_priv *priv)
> +static void disable_all_intr(struct edac_priv *priv)
> {
> /* Unlock the PCSR registers */
> writel(PCSR_UNLOCK_VAL, priv->ddrmc_baseaddr +
> XDDR_PCSR_OFFSET);
>
> - /* Enable UE and CE Interrupts to support the interrupt case */
> - writel(XDDR_IRQ_CE_MASK | XDDR_IRQ_UE_MASK,
> - priv->ddrmc_baseaddr + XDDR_IRQ_EN_OFFSET);
> + writel(XDDR_IRQ_ALL,
> + priv->ddrmc_baseaddr + XDDR_IRQ_DIS_OFFSET);
> + writel(XDDR_IRQ_ALL,
> + priv->ddrmc_baseaddr + XDDR_IRQ1_DIS_OFFSET);
>
> - writel(XDDR_IRQ_UE_MASK,
> - priv->ddrmc_baseaddr + XDDR_IRQ1_EN_OFFSET);
> /* Lock the PCSR registers */
> writel(1, priv->ddrmc_baseaddr + XDDR_PCSR_OFFSET); }
>
> -static void disable_all_intr(struct edac_priv *priv)
> +#ifdef CONFIG_EDAC_DEBUG
> +#define to_mci(k) container_of(k, struct mem_ctl_info, dev)
> +
> +static void enable_intr(struct edac_priv *priv)
> {
> /* Unlock the PCSR registers */
> writel(PCSR_UNLOCK_VAL, priv->ddrmc_baseaddr +
> XDDR_PCSR_OFFSET);
>
> - writel(XDDR_IRQ_ALL,
> - priv->ddrmc_baseaddr + XDDR_IRQ_DIS_OFFSET);
> - writel(XDDR_IRQ_ALL,
> - priv->ddrmc_baseaddr + XDDR_IRQ1_DIS_OFFSET);
> + /* Enable UE and CE Interrupts to support the interrupt case */
> + writel(XDDR_IRQ_CE_MASK | XDDR_IRQ_UE_MASK,
> + priv->ddrmc_baseaddr + XDDR_IRQ_EN_OFFSET);
>
> + writel(XDDR_IRQ_UE_MASK,
> + priv->ddrmc_baseaddr + XDDR_IRQ1_EN_OFFSET);
> /* Lock the PCSR registers */
> writel(1, priv->ddrmc_baseaddr + XDDR_PCSR_OFFSET); }
>
> -#ifdef CONFIG_EDAC_DEBUG
> -#define to_mci(k) container_of(k, struct mem_ctl_info, dev)
> -
> /**
> * poison_setup - Update poison registers.
> * @priv: DDR memory controller private instance data.
> @@ -787,7 +787,7 @@ static const struct file_operations
> xddr_inject_enable_fops = {
> .llseek = generic_file_llseek,
> };
>
> -static void edac_create_debugfs_attributes(struct mem_ctl_info *mci)
> +static void create_debugfs_attributes(struct mem_ctl_info *mci)
> {
> struct edac_priv *priv = mci->pvt_info;
>
> @@ -1012,13 +1012,12 @@ static int mc_probe(struct platform_device
> *pdev)
> goto del_mc;
> }
>
> - disable_all_intr(priv);
> #ifdef CONFIG_EDAC_DEBUG
> - edac_create_debugfs_attributes(mci);
> -
> + disable_all_intr(priv);
> + create_debugfs_attributes(mci);
> setup_address_map(priv);
> -#endif
> enable_intr(priv);
> +#endif
We disable all the interrupts and enable only the correctable and non-correctable errors.
The disable takes care of the issue that if other interrupts are enabled then there is no one to
handle the interrupts.
Also the enable interrupts are needed for the driver to work. The debug for error injection to
test the notification.
> return rc;
>
> del_mc:
>
> --
> Regards/Gruss,
> Boris.
>
> https://people.kernel.org/tglx/notes-about-netiquette
