[v3,2/2] soc: mediatek: add mtk-devapc driver

Commit Message

Neal Liu July 21, 2020, 3:59 a.m. UTC

MediaTek bus fabric provides TrustZone security support and data
protection to prevent slaves from being accessed by unexpected
masters.
The security violation is logged and sent to the processor for
further analysis or countermeasures.

Any occurrence of security violation would raise an interrupt, and
it will be handled by mtk-devapc driver. The violation
information is printed in order to find the murderer.

Signed-off-by: Neal Liu <[e-mail]>
---
 drivers/soc/mediatek/Kconfig      |    9 +
 drivers/soc/mediatek/Makefile     |    1 +
 drivers/soc/mediatek/mtk-devapc.c |  372 +++++++++++++++++++++++++++++++++++++
 drivers/soc/mediatek/mtk-devapc.h |   54 ++++++
 4 files changed, 436 insertions(+)
 create mode 100644 drivers/soc/mediatek/mtk-devapc.c
 create mode 100644 drivers/soc/mediatek/mtk-devapc.h

Comments

Chun-Kuang Hu July 21, 2020, 11:21 p.m. UTC | #1

Hi, Neal:

Neal Liu <[e-mail]> 於 2020年7月21日 週二 下午12:00寫道：
>
> MediaTek bus fabric provides TrustZone security support and data
> protection to prevent slaves from being accessed by unexpected
> masters.
> The security violation is logged and sent to the processor for
> further analysis or countermeasures.
>
> Any occurrence of security violation would raise an interrupt, and
> it will be handled by mtk-devapc driver. The violation
> information is printed in order to find the murderer.
>
> Signed-off-by: Neal Liu <[e-mail]>
> ---

[snip]

> +
> +static u32 get_shift_group(struct mtk_devapc_context *ctx, u32 vio_idx)

vio_idx is useless, so remove it.

> +{
> +       u32 vio_shift_sta;
> +       void __iomem *reg;
> +
> +       reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
> +       vio_shift_sta = readl(reg);
> +
> +       if (vio_shift_sta)
> +               return __ffs(vio_shift_sta);
> +
> +       return 31;
> +}
> +

[snip]

> +
> +/*
> + * mtk_devapc_dump_vio_dbg - get the violation index and dump the full violation
> + *                           debug information.
> + */
> +static bool mtk_devapc_dump_vio_dbg(struct mtk_devapc_context *ctx, u32 vio_idx)
> +{
> +       u32 shift_bit;
> +
> +       if (check_vio_mask(ctx, vio_idx))
> +               return false;
> +
> +       if (!check_vio_status(ctx, vio_idx))
> +               return false;
> +
> +       shift_bit = get_shift_group(ctx, vio_idx);
> +
> +       if (sync_vio_dbg(ctx, shift_bit))
> +               return false;
> +
> +       devapc_extract_vio_dbg(ctx);

I think get_shift_group(), sync_vio_dbg(), and
devapc_extract_vio_dbg() should be moved out of vio_idx for-loop (the
loop in devapc_violation_irq()) because these three function is not
related to vio_idx.
Another question: when multiple vio_idx violation occur, vio_addr is
related to which one vio_idx? The latest happened one?

> +
> +       return true;
> +}
> +
> +/*
> + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> + *                        violation information including which master violates
> + *                        access slave.
> + */
> +static irqreturn_t devapc_violation_irq(int irq_number,
> +                                       struct mtk_devapc_context *ctx)
> +{
> +       u32 vio_idx;
> +
> +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
> +               if (!mtk_devapc_dump_vio_dbg(ctx, vio_idx))
> +                       continue;
> +
> +               /* Ensure that violation info are written before
> +                * further operations
> +                */
> +               smp_mb();
> +
> +               /*
> +                * Mask slave's irq before clearing vio status.
> +                * Must do it to avoid nested interrupt and prevent
> +                * unexpected behavior.
> +                */
> +               mask_module_irq(ctx, vio_idx, true);
> +
> +               clear_vio_status(ctx, vio_idx);
> +
> +               mask_module_irq(ctx, vio_idx, false);
> +       }
> +
> +       return IRQ_HANDLED;
> +}
> +
> +/*
> + * start_devapc - initialize devapc status and start receiving interrupt
> + *                while devapc violation is triggered.
> + */
> +static int start_devapc(struct mtk_devapc_context *ctx)
> +{
> +       void __iomem *pd_vio_shift_sta_reg;
> +       void __iomem *pd_apc_con_reg;
> +       u32 vio_shift_sta;
> +       u32 vio_idx;
> +
> +       pd_apc_con_reg = ctx->devapc_pd_base + ctx->offset->apc_con;
> +       pd_vio_shift_sta_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
> +       if (!pd_apc_con_reg || !pd_vio_shift_sta_reg)
> +               return -EINVAL;
> +
> +       /* Clear devapc violation status */
> +       writel(BIT(31), pd_apc_con_reg);
> +
> +       /* Clear violation shift status */
> +       vio_shift_sta = readl(pd_vio_shift_sta_reg);
> +       if (vio_shift_sta)
> +               writel(vio_shift_sta, pd_vio_shift_sta_reg);
> +
> +       /* Clear slave violation status */
> +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
> +               clear_vio_status(ctx, vio_idx);
> +               mask_module_irq(ctx, vio_idx, false);
> +       }
> +

Why do you clear these? After power on hardware, I think these
register status are correct. If the default value of these register
are not correct, add a comment for this.

Regards,
Chun-Kuang.

> +       return 0;
> +}
> +

Neal Liu July 22, 2020, 3:49 a.m. UTC | #2

Hi Chun-Kuang,

On Wed, 2020-07-22 at 07:21 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
> 
> Neal Liu <[e-mail]> 於 2020年7月21日 週二 下午12:00寫道：
> >
> > MediaTek bus fabric provides TrustZone security support and data
> > protection to prevent slaves from being accessed by unexpected
> > masters.
> > The security violation is logged and sent to the processor for
> > further analysis or countermeasures.
> >
> > Any occurrence of security violation would raise an interrupt, and
> > it will be handled by mtk-devapc driver. The violation
> > information is printed in order to find the murderer.
> >
> > Signed-off-by: Neal Liu <[e-mail]>
> > ---
> 
> [snip]
> 
> > +
> > +static u32 get_shift_group(struct mtk_devapc_context *ctx, u32 vio_idx)
> 
> vio_idx is useless, so remove it.

Okay, I'll remove it in next patch.

> 
> > +{
> > +       u32 vio_shift_sta;
> > +       void __iomem *reg;
> > +
> > +       reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
> > +       vio_shift_sta = readl(reg);
> > +
> > +       if (vio_shift_sta)
> > +               return __ffs(vio_shift_sta);
> > +
> > +       return 31;
> > +}
> > +
> 
> [snip]
> 
> > +
> > +/*
> > + * mtk_devapc_dump_vio_dbg - get the violation index and dump the full violation
> > + *                           debug information.
> > + */
> > +static bool mtk_devapc_dump_vio_dbg(struct mtk_devapc_context *ctx, u32 vio_idx)
> > +{
> > +       u32 shift_bit;
> > +
> > +       if (check_vio_mask(ctx, vio_idx))
> > +               return false;
> > +
> > +       if (!check_vio_status(ctx, vio_idx))
> > +               return false;
> > +
> > +       shift_bit = get_shift_group(ctx, vio_idx);
> > +
> > +       if (sync_vio_dbg(ctx, shift_bit))
> > +               return false;
> > +
> > +       devapc_extract_vio_dbg(ctx);
> 
> I think get_shift_group(), sync_vio_dbg(), and
> devapc_extract_vio_dbg() should be moved out of vio_idx for-loop (the
> loop in devapc_violation_irq()) because these three function is not
> related to vio_idx.
> Another question: when multiple vio_idx violation occur, vio_addr is
> related to which one vio_idx? The latest happened one?
> 

Actually, it's related to vio_idx. But we don't use it directly on these
function. I think below snip code might be better way to understand it.

for (...)
{
	check_vio_mask()
	check_vio_status()

	// if get vio_idx, mask it temporarily
	mask_module_irq(true)
	clear_vio_status()

	// dump violation info
	get_shift_group()
	sync_vio_dbg()
	devapc_extract_vio_dbg()

	// unmask
	mask_module_irq(false)
}

About your question, vio_addr would be the first one.

> > +
> > +       return true;
> > +}
> > +
> > +/*
> > + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> > + *                        violation information including which master violates
> > + *                        access slave.
> > + */
> > +static irqreturn_t devapc_violation_irq(int irq_number,
> > +                                       struct mtk_devapc_context *ctx)
> > +{
> > +       u32 vio_idx;
> > +
> > +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
> > +               if (!mtk_devapc_dump_vio_dbg(ctx, vio_idx))
> > +                       continue;
> > +
> > +               /* Ensure that violation info are written before
> > +                * further operations
> > +                */
> > +               smp_mb();
> > +
> > +               /*
> > +                * Mask slave's irq before clearing vio status.
> > +                * Must do it to avoid nested interrupt and prevent
> > +                * unexpected behavior.
> > +                */
> > +               mask_module_irq(ctx, vio_idx, true);
> > +
> > +               clear_vio_status(ctx, vio_idx);
> > +
> > +               mask_module_irq(ctx, vio_idx, false);
> > +       }
> > +
> > +       return IRQ_HANDLED;
> > +}
> > +
> > +/*
> > + * start_devapc - initialize devapc status and start receiving interrupt
> > + *                while devapc violation is triggered.
> > + */
> > +static int start_devapc(struct mtk_devapc_context *ctx)
> > +{
> > +       void __iomem *pd_vio_shift_sta_reg;
> > +       void __iomem *pd_apc_con_reg;
> > +       u32 vio_shift_sta;
> > +       u32 vio_idx;
> > +
> > +       pd_apc_con_reg = ctx->devapc_pd_base + ctx->offset->apc_con;
> > +       pd_vio_shift_sta_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
> > +       if (!pd_apc_con_reg || !pd_vio_shift_sta_reg)
> > +               return -EINVAL;
> > +
> > +       /* Clear devapc violation status */
> > +       writel(BIT(31), pd_apc_con_reg);
> > +
> > +       /* Clear violation shift status */
> > +       vio_shift_sta = readl(pd_vio_shift_sta_reg);
> > +       if (vio_shift_sta)
> > +               writel(vio_shift_sta, pd_vio_shift_sta_reg);
> > +
> > +       /* Clear slave violation status */
> > +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
> > +               clear_vio_status(ctx, vio_idx);
> > +               mask_module_irq(ctx, vio_idx, false);
> > +       }
> > +
> 
> Why do you clear these? After power on hardware, I think these
> register status are correct. If the default value of these register
> are not correct, add a comment for this.
> 

The register default value would be correct after power on.
But there are many things have to do before kernel driver probe.
During that time, devapc register status might be changed. But we are
focusing on handling violation after driver probe instead.
So clearing all reg status to make it as initial state.

> Regards,
> Chun-Kuang.
> 
> > +       return 0;
> > +}
> > +

Chun-Kuang Hu July 22, 2020, 2:25 p.m. UTC | #3

Hi, Neal:

Neal Liu <[e-mail]> 於 2020年7月22日 週三 上午11:49寫道：
>
> Hi Chun-Kuang,
>
> On Wed, 2020-07-22 at 07:21 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu <[e-mail]> 於 2020年7月21日 週二 下午12:00寫道：
> > >
> >
> > > +
> > > +/*
> > > + * mtk_devapc_dump_vio_dbg - get the violation index and dump the full violation
> > > + *                           debug information.
> > > + */
> > > +static bool mtk_devapc_dump_vio_dbg(struct mtk_devapc_context *ctx, u32 vio_idx)
> > > +{
> > > +       u32 shift_bit;
> > > +
> > > +       if (check_vio_mask(ctx, vio_idx))
> > > +               return false;
> > > +
> > > +       if (!check_vio_status(ctx, vio_idx))
> > > +               return false;
> > > +
> > > +       shift_bit = get_shift_group(ctx, vio_idx);
> > > +
> > > +       if (sync_vio_dbg(ctx, shift_bit))
> > > +               return false;
> > > +
> > > +       devapc_extract_vio_dbg(ctx);
> >
> > I think get_shift_group(), sync_vio_dbg(), and
> > devapc_extract_vio_dbg() should be moved out of vio_idx for-loop (the
> > loop in devapc_violation_irq()) because these three function is not
> > related to vio_idx.
> > Another question: when multiple vio_idx violation occur, vio_addr is
> > related to which one vio_idx? The latest happened one?
> >
>
> Actually, it's related to vio_idx. But we don't use it directly on these
> function. I think below snip code might be better way to understand it.
>
> for (...)
> {
>         check_vio_mask()
>         check_vio_status()
>
>         // if get vio_idx, mask it temporarily
>         mask_module_irq(true)
>         clear_vio_status()
>
>         // dump violation info
>         get_shift_group()
>         sync_vio_dbg()
>         devapc_extract_vio_dbg()
>
>         // unmask
>         mask_module_irq(false)
> }

This snip code does not explain any thing. I could rewrite this code as:

for (...)
{
    check_vio_mask()
    check_vio_status()

    // if get vio_idx, mask it temporarily
    mask_module_irq(true)
    clear_vio_status()
    // unmask
    mask_module_irq(false)
}

// dump violation info
get_shift_group()
sync_vio_dbg()
devapc_extract_vio_dbg()

And my version is identical with your version, isn't it?

>
> About your question, vio_addr would be the first one.

So other vio_addr would be dropped? Or hardware would keep all
vio_addr and you have some way to get all vio_addr?

>
> > > +
> > > +       return true;
> > > +}
> > > +
> > > +/*
> > > + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> > > + *                        violation information including which master violates
> > > + *                        access slave.
> > > + */
> > > +static irqreturn_t devapc_violation_irq(int irq_number,
> > > +                                       struct mtk_devapc_context *ctx)
> > > +{
> > > +       u32 vio_idx;
> > > +
> > > +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
> > > +               if (!mtk_devapc_dump_vio_dbg(ctx, vio_idx))
> > > +                       continue;
> > > +
> > > +               /* Ensure that violation info are written before
> > > +                * further operations
> > > +                */
> > > +               smp_mb();
> > > +
> > > +               /*
> > > +                * Mask slave's irq before clearing vio status.
> > > +                * Must do it to avoid nested interrupt and prevent
> > > +                * unexpected behavior.
> > > +                */
> > > +               mask_module_irq(ctx, vio_idx, true);
> > > +
> > > +               clear_vio_status(ctx, vio_idx);
> > > +
> > > +               mask_module_irq(ctx, vio_idx, false);
> > > +       }
> > > +
> > > +       return IRQ_HANDLED;
> > > +}
> > > +
> > > +/*
> > > + * start_devapc - initialize devapc status and start receiving interrupt
> > > + *                while devapc violation is triggered.
> > > + */
> > > +static int start_devapc(struct mtk_devapc_context *ctx)
> > > +{
> > > +       void __iomem *pd_vio_shift_sta_reg;
> > > +       void __iomem *pd_apc_con_reg;
> > > +       u32 vio_shift_sta;
> > > +       u32 vio_idx;
> > > +
> > > +       pd_apc_con_reg = ctx->devapc_pd_base + ctx->offset->apc_con;
> > > +       pd_vio_shift_sta_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
> > > +       if (!pd_apc_con_reg || !pd_vio_shift_sta_reg)
> > > +               return -EINVAL;
> > > +
> > > +       /* Clear devapc violation status */
> > > +       writel(BIT(31), pd_apc_con_reg);
> > > +
> > > +       /* Clear violation shift status */
> > > +       vio_shift_sta = readl(pd_vio_shift_sta_reg);
> > > +       if (vio_shift_sta)
> > > +               writel(vio_shift_sta, pd_vio_shift_sta_reg);
> > > +
> > > +       /* Clear slave violation status */
> > > +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
> > > +               clear_vio_status(ctx, vio_idx);
> > > +               mask_module_irq(ctx, vio_idx, false);
> > > +       }
> > > +
> >
> > Why do you clear these? After power on hardware, I think these
> > register status are correct. If the default value of these register
> > are not correct, add a comment for this.
> >
>
> The register default value would be correct after power on.
> But there are many things have to do before kernel driver probe.
> During that time, devapc register status might be changed. But we are
> focusing on handling violation after driver probe instead.
> So clearing all reg status to make it as initial state.

After hardware is powered on and some violation happen before this
driver init, why do you not care about it? That is a violation in this
system.
For one application, I could build this driver as a ko (kernel
module). I do not insert this ko in normal, but I insert it after
something is wrong. So I need to get the information happened before
this driver init.

Regards,
Chun-Kuang.

>
> > Regards,
> > Chun-Kuang.
> >
> > > +       return 0;
> > > +}
> > > +
>

Neal Liu July 23, 2020, 6:11 a.m. UTC | #4

Hi Chun-Kuang,

On Wed, 2020-07-22 at 22:25 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
> 
> Neal Liu <[e-mail]> 於 2020年7月22日 週三 上午11:49寫道：
> >
> > Hi Chun-Kuang,
> >
> > On Wed, 2020-07-22 at 07:21 +0800, Chun-Kuang Hu wrote:
> > > Hi, Neal:
> > >
> > > Neal Liu <[e-mail]> 於 2020年7月21日 週二 下午12:00寫道：
> > > >
> > >
> > > > +
> > > > +/*
> > > > + * mtk_devapc_dump_vio_dbg - get the violation index and dump the full violation
> > > > + *                           debug information.
> > > > + */
> > > > +static bool mtk_devapc_dump_vio_dbg(struct mtk_devapc_context *ctx, u32 vio_idx)
> > > > +{
> > > > +       u32 shift_bit;
> > > > +
> > > > +       if (check_vio_mask(ctx, vio_idx))
> > > > +               return false;
> > > > +
> > > > +       if (!check_vio_status(ctx, vio_idx))
> > > > +               return false;
> > > > +
> > > > +       shift_bit = get_shift_group(ctx, vio_idx);
> > > > +
> > > > +       if (sync_vio_dbg(ctx, shift_bit))
> > > > +               return false;
> > > > +
> > > > +       devapc_extract_vio_dbg(ctx);
> > >
> > > I think get_shift_group(), sync_vio_dbg(), and
> > > devapc_extract_vio_dbg() should be moved out of vio_idx for-loop (the
> > > loop in devapc_violation_irq()) because these three function is not
> > > related to vio_idx.
> > > Another question: when multiple vio_idx violation occur, vio_addr is
> > > related to which one vio_idx? The latest happened one?
> > >
> >
> > Actually, it's related to vio_idx. But we don't use it directly on these
> > function. I think below snip code might be better way to understand it.
> >
> > for (...)
> > {
> >         check_vio_mask()
> >         check_vio_status()
> >
> >         // if get vio_idx, mask it temporarily
> >         mask_module_irq(true)
> >         clear_vio_status()
> >
> >         // dump violation info
> >         get_shift_group()
> >         sync_vio_dbg()
> >         devapc_extract_vio_dbg()
> >
> >         // unmask
> >         mask_module_irq(false)
> > }
> 
> This snip code does not explain any thing. I could rewrite this code as:
> 
> for (...)
> {
>     check_vio_mask()
>     check_vio_status()
> 
>     // if get vio_idx, mask it temporarily
>     mask_module_irq(true)
>     clear_vio_status()
>     // unmask
>     mask_module_irq(false)
> }
> 
> // dump violation info
> get_shift_group()
> sync_vio_dbg()
> devapc_extract_vio_dbg()
> 
> And my version is identical with your version, isn't it?

Sorry, I did not explain it clearly. Let's me try again.
The reason why I put "dump violation info" between mask & unmask context
is because it has to stop interrupt first before dump violation info,
and then unmask it to prepare next violation.
These sequence guarantee that if multiple violation is triggered, we
still have information to debug.
If the code sequence in your version and multiple violation is
triggered, there might be no any information but keeps entering ISR.
Finally, system might be abnormal and watchdog timeout.
In this case, we still don't have any information to debug.

> 
> >
> > About your question, vio_addr would be the first one.
> 
> So other vio_addr would be dropped? Or hardware would keep all
> vio_addr and you have some way to get all vio_addr?
> 

In this case, hardware will drop other violation info and keep the first
one until it been handled.

> >
> > > > +
> > > > +       return true;
> > > > +}
> > > > +
> > > > +/*
> > > > + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> > > > + *                        violation information including which master violates
> > > > + *                        access slave.
> > > > + */
> > > > +static irqreturn_t devapc_violation_irq(int irq_number,
> > > > +                                       struct mtk_devapc_context *ctx)
> > > > +{
> > > > +       u32 vio_idx;
> > > > +
> > > > +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
> > > > +               if (!mtk_devapc_dump_vio_dbg(ctx, vio_idx))
> > > > +                       continue;
> > > > +
> > > > +               /* Ensure that violation info are written before
> > > > +                * further operations
> > > > +                */
> > > > +               smp_mb();
> > > > +
> > > > +               /*
> > > > +                * Mask slave's irq before clearing vio status.
> > > > +                * Must do it to avoid nested interrupt and prevent
> > > > +                * unexpected behavior.
> > > > +                */
> > > > +               mask_module_irq(ctx, vio_idx, true);
> > > > +
> > > > +               clear_vio_status(ctx, vio_idx);
> > > > +
> > > > +               mask_module_irq(ctx, vio_idx, false);
> > > > +       }
> > > > +
> > > > +       return IRQ_HANDLED;
> > > > +}
> > > > +
> > > > +/*
> > > > + * start_devapc - initialize devapc status and start receiving interrupt
> > > > + *                while devapc violation is triggered.
> > > > + */
> > > > +static int start_devapc(struct mtk_devapc_context *ctx)
> > > > +{
> > > > +       void __iomem *pd_vio_shift_sta_reg;
> > > > +       void __iomem *pd_apc_con_reg;
> > > > +       u32 vio_shift_sta;
> > > > +       u32 vio_idx;
> > > > +
> > > > +       pd_apc_con_reg = ctx->devapc_pd_base + ctx->offset->apc_con;
> > > > +       pd_vio_shift_sta_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
> > > > +       if (!pd_apc_con_reg || !pd_vio_shift_sta_reg)
> > > > +               return -EINVAL;
> > > > +
> > > > +       /* Clear devapc violation status */
> > > > +       writel(BIT(31), pd_apc_con_reg);
> > > > +
> > > > +       /* Clear violation shift status */
> > > > +       vio_shift_sta = readl(pd_vio_shift_sta_reg);
> > > > +       if (vio_shift_sta)
> > > > +               writel(vio_shift_sta, pd_vio_shift_sta_reg);
> > > > +
> > > > +       /* Clear slave violation status */
> > > > +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
> > > > +               clear_vio_status(ctx, vio_idx);
> > > > +               mask_module_irq(ctx, vio_idx, false);
> > > > +       }
> > > > +
> > >
> > > Why do you clear these? After power on hardware, I think these
> > > register status are correct. If the default value of these register
> > > are not correct, add a comment for this.
> > >
> >
> > The register default value would be correct after power on.
> > But there are many things have to do before kernel driver probe.
> > During that time, devapc register status might be changed. But we are
> > focusing on handling violation after driver probe instead.
> > So clearing all reg status to make it as initial state.
> 
> After hardware is powered on and some violation happen before this
> driver init, why do you not care about it? That is a violation in this
> system.
> For one application, I could build this driver as a ko (kernel
> module). I do not insert this ko in normal, but I insert it after
> something is wrong. So I need to get the information happened before
> this driver init.

Yes, you are right. I think it's a better way for upstream patch.
I'll remove it in next patch.
Thanks

> 
> Regards,
> Chun-Kuang.
> 
> >
> > > Regards,
> > > Chun-Kuang.
> > >
> > > > +       return 0;
> > > > +}
> > > > +
> >

Chun-Kuang Hu July 23, 2020, 4:32 p.m. UTC | #5

Hi, Neal:

Neal Liu <[e-mail]> 於 2020年7月23日 週四 下午2:11寫道：
>
> Hi Chun-Kuang,
>
> On Wed, 2020-07-22 at 22:25 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu <[e-mail]> 於 2020年7月22日 週三 上午11:49寫道：
> > >
> > > Hi Chun-Kuang,
> > >
> > > On Wed, 2020-07-22 at 07:21 +0800, Chun-Kuang Hu wrote:
> > > > Hi, Neal:
> > > >
> > > > Neal Liu <[e-mail]> 於 2020年7月21日 週二 下午12:00寫道：
> > > > >
> > > >
> > > > > +
> > > > > +/*
> > > > > + * mtk_devapc_dump_vio_dbg - get the violation index and dump the full violation
> > > > > + *                           debug information.
> > > > > + */
> > > > > +static bool mtk_devapc_dump_vio_dbg(struct mtk_devapc_context *ctx, u32 vio_idx)
> > > > > +{
> > > > > +       u32 shift_bit;
> > > > > +
> > > > > +       if (check_vio_mask(ctx, vio_idx))
> > > > > +               return false;
> > > > > +
> > > > > +       if (!check_vio_status(ctx, vio_idx))
> > > > > +               return false;
> > > > > +
> > > > > +       shift_bit = get_shift_group(ctx, vio_idx);
> > > > > +
> > > > > +       if (sync_vio_dbg(ctx, shift_bit))
> > > > > +               return false;
> > > > > +
> > > > > +       devapc_extract_vio_dbg(ctx);
> > > >
> > > > I think get_shift_group(), sync_vio_dbg(), and
> > > > devapc_extract_vio_dbg() should be moved out of vio_idx for-loop (the
> > > > loop in devapc_violation_irq()) because these three function is not
> > > > related to vio_idx.
> > > > Another question: when multiple vio_idx violation occur, vio_addr is
> > > > related to which one vio_idx? The latest happened one?
> > > >
> > >
> > > Actually, it's related to vio_idx. But we don't use it directly on these
> > > function. I think below snip code might be better way to understand it.
> > >
> > > for (...)
> > > {
> > >         check_vio_mask()
> > >         check_vio_status()
> > >
> > >         // if get vio_idx, mask it temporarily
> > >         mask_module_irq(true)
> > >         clear_vio_status()
> > >
> > >         // dump violation info
> > >         get_shift_group()
> > >         sync_vio_dbg()
> > >         devapc_extract_vio_dbg()
> > >
> > >         // unmask
> > >         mask_module_irq(false)
> > > }
> >
> > This snip code does not explain any thing. I could rewrite this code as:
> >
> > for (...)
> > {
> >     check_vio_mask()
> >     check_vio_status()
> >
> >     // if get vio_idx, mask it temporarily
> >     mask_module_irq(true)
> >     clear_vio_status()
> >     // unmask
> >     mask_module_irq(false)
> > }
> >
> > // dump violation info
> > get_shift_group()
> > sync_vio_dbg()
> > devapc_extract_vio_dbg()
> >
> > And my version is identical with your version, isn't it?
>
> Sorry, I did not explain it clearly. Let's me try again.
> The reason why I put "dump violation info" between mask & unmask context
> is because it has to stop interrupt first before dump violation info,
> and then unmask it to prepare next violation.
> These sequence guarantee that if multiple violation is triggered, we
> still have information to debug.
> If the code sequence in your version and multiple violation is
> triggered, there might be no any information but keeps entering ISR.
> Finally, system might be abnormal and watchdog timeout.
> In this case, we still don't have any information to debug.

I still don't understand why no information to debug. For example when
vio_idx 5, 10, 15 has violation,
You would mask vio_idx 5 to get information, but vio_idx 10, 15 does
not mask yet.
In your words, when vio_idx 10, 15 not mask, you would not get any
debug information when you process vio_idx 5.

In my version, I would clear all status, why keeps entering ISR?

>
> >
> > >
> > > About your question, vio_addr would be the first one.
> >
> > So other vio_addr would be dropped? Or hardware would keep all
> > vio_addr and you have some way to get all vio_addr?
> >
>
> In this case, hardware will drop other violation info and keep the first
> one until it been handled.

Does 'handled' mean status is cleared?

Regards,
Chun-Kuang.

>
> > >
> > > > > +
> > > > > +       return true;
> > > > > +}
> > > > > +
> > > > > +/*
> > > > > + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> > > > > + *                        violation information including which master violates
> > > > > + *                        access slave.
> > > > > + */
> > > > > +static irqreturn_t devapc_violation_irq(int irq_number,
> > > > > +                                       struct mtk_devapc_context *ctx)
> > > > > +{
> > > > > +       u32 vio_idx;
> > > > > +
> > > > > +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
> > > > > +               if (!mtk_devapc_dump_vio_dbg(ctx, vio_idx))
> > > > > +                       continue;
> > > > > +
> > > > > +               /* Ensure that violation info are written before
> > > > > +                * further operations
> > > > > +                */
> > > > > +               smp_mb();
> > > > > +
> > > > > +               /*
> > > > > +                * Mask slave's irq before clearing vio status.
> > > > > +                * Must do it to avoid nested interrupt and prevent
> > > > > +                * unexpected behavior.
> > > > > +                */
> > > > > +               mask_module_irq(ctx, vio_idx, true);
> > > > > +
> > > > > +               clear_vio_status(ctx, vio_idx);
> > > > > +
> > > > > +               mask_module_irq(ctx, vio_idx, false);
> > > > > +       }
> > > > > +
> > > > > +       return IRQ_HANDLED;
> > > > > +}
> > > > > +
> > > > > +/*

Neal Liu July 24, 2020, 6:55 a.m. UTC | #6

Hi Chun-Kuang,

On Fri, 2020-07-24 at 00:32 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
> 
> Neal Liu <[e-mail]> 於 2020年7月23日 週四 下午2:11寫道：
> >
> > Hi Chun-Kuang,
> >
> > On Wed, 2020-07-22 at 22:25 +0800, Chun-Kuang Hu wrote:
> > > Hi, Neal:
> > >
> > > Neal Liu <[e-mail]> 於 2020年7月22日 週三 上午11:49寫道：
> > > >
> > > > Hi Chun-Kuang,
> > > >
> > > > On Wed, 2020-07-22 at 07:21 +0800, Chun-Kuang Hu wrote:
> > > > > Hi, Neal:
> > > > >
> > > > > Neal Liu <[e-mail]> 於 2020年7月21日 週二 下午12:00寫道：
> > > > > >
> > > > >
> > > > > > +
> > > > > > +/*
> > > > > > + * mtk_devapc_dump_vio_dbg - get the violation index and dump the full violation
> > > > > > + *                           debug information.
> > > > > > + */
> > > > > > +static bool mtk_devapc_dump_vio_dbg(struct mtk_devapc_context *ctx, u32 vio_idx)
> > > > > > +{
> > > > > > +       u32 shift_bit;
> > > > > > +
> > > > > > +       if (check_vio_mask(ctx, vio_idx))
> > > > > > +               return false;
> > > > > > +
> > > > > > +       if (!check_vio_status(ctx, vio_idx))
> > > > > > +               return false;
> > > > > > +
> > > > > > +       shift_bit = get_shift_group(ctx, vio_idx);
> > > > > > +
> > > > > > +       if (sync_vio_dbg(ctx, shift_bit))
> > > > > > +               return false;
> > > > > > +
> > > > > > +       devapc_extract_vio_dbg(ctx);
> > > > >
> > > > > I think get_shift_group(), sync_vio_dbg(), and
> > > > > devapc_extract_vio_dbg() should be moved out of vio_idx for-loop (the
> > > > > loop in devapc_violation_irq()) because these three function is not
> > > > > related to vio_idx.
> > > > > Another question: when multiple vio_idx violation occur, vio_addr is
> > > > > related to which one vio_idx? The latest happened one?
> > > > >
> > > >
> > > > Actually, it's related to vio_idx. But we don't use it directly on these
> > > > function. I think below snip code might be better way to understand it.
> > > >
> > > > for (...)
> > > > {
> > > >         check_vio_mask()
> > > >         check_vio_status()
> > > >
> > > >         // if get vio_idx, mask it temporarily
> > > >         mask_module_irq(true)
> > > >         clear_vio_status()
> > > >
> > > >         // dump violation info
> > > >         get_shift_group()
> > > >         sync_vio_dbg()
> > > >         devapc_extract_vio_dbg()
> > > >
> > > >         // unmask
> > > >         mask_module_irq(false)
> > > > }
> > >
> > > This snip code does not explain any thing. I could rewrite this code as:
> > >
> > > for (...)
> > > {
> > >     check_vio_mask()
> > >     check_vio_status()
> > >
> > >     // if get vio_idx, mask it temporarily
> > >     mask_module_irq(true)
> > >     clear_vio_status()
> > >     // unmask
> > >     mask_module_irq(false)
> > > }
> > >
> > > // dump violation info
> > > get_shift_group()
> > > sync_vio_dbg()
> > > devapc_extract_vio_dbg()
> > >
> > > And my version is identical with your version, isn't it?
> >
> > Sorry, I did not explain it clearly. Let's me try again.
> > The reason why I put "dump violation info" between mask & unmask context
> > is because it has to stop interrupt first before dump violation info,
> > and then unmask it to prepare next violation.
> > These sequence guarantee that if multiple violation is triggered, we
> > still have information to debug.
> > If the code sequence in your version and multiple violation is
> > triggered, there might be no any information but keeps entering ISR.
> > Finally, system might be abnormal and watchdog timeout.
> > In this case, we still don't have any information to debug.
> 
> I still don't understand why no information to debug. For example when
> vio_idx 5, 10, 15 has violation,
> You would mask vio_idx 5 to get information, but vio_idx 10, 15 does
> not mask yet.
> In your words, when vio_idx 10, 15 not mask, you would not get any
> debug information when you process vio_idx 5.
> 
> In my version, I would clear all status, why keeps entering ISR?

Think about this case, if someone tries to dump "AAA" module's register.
It would keep read reg base, base+0x4, base+0x8, ...
All these registers are in the same slave, which would be same vio_idx.
(Take vio_idx 5 as example)
In this case, vio_idx 5 will keep triggering interrupt. If you did not
do "dump violation info" between mask & unmask, you cannot get any
violation info until the last interrupt being handled.
Normally, system will crash before last interrupt coming.

> 
> >
> > >
> > > >
> > > > About your question, vio_addr would be the first one.
> > >
> > > So other vio_addr would be dropped? Or hardware would keep all
> > > vio_addr and you have some way to get all vio_addr?
> > >
> >
> > In this case, hardware will drop other violation info and keep the first
> > one until it been handled.
> 
> Does 'handled' mean status is cleared?

"handled" means clear status and dump violation info.

> 
> Regards,
> Chun-Kuang.
> 
> >
> > > >
> > > > > > +
> > > > > > +       return true;
> > > > > > +}
> > > > > > +
> > > > > > +/*
> > > > > > + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> > > > > > + *                        violation information including which master violates
> > > > > > + *                        access slave.
> > > > > > + */
> > > > > > +static irqreturn_t devapc_violation_irq(int irq_number,
> > > > > > +                                       struct mtk_devapc_context *ctx)
> > > > > > +{
> > > > > > +       u32 vio_idx;
> > > > > > +
> > > > > > +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
> > > > > > +               if (!mtk_devapc_dump_vio_dbg(ctx, vio_idx))
> > > > > > +                       continue;
> > > > > > +
> > > > > > +               /* Ensure that violation info are written before
> > > > > > +                * further operations
> > > > > > +                */
> > > > > > +               smp_mb();
> > > > > > +
> > > > > > +               /*
> > > > > > +                * Mask slave's irq before clearing vio status.
> > > > > > +                * Must do it to avoid nested interrupt and prevent
> > > > > > +                * unexpected behavior.
> > > > > > +                */
> > > > > > +               mask_module_irq(ctx, vio_idx, true);
> > > > > > +
> > > > > > +               clear_vio_status(ctx, vio_idx);
> > > > > > +
> > > > > > +               mask_module_irq(ctx, vio_idx, false);
> > > > > > +       }
> > > > > > +
> > > > > > +       return IRQ_HANDLED;
> > > > > > +}
> > > > > > +
> > > > > > +/*

Chun-Kuang Hu July 24, 2020, 3:55 p.m. UTC | #7

Hi, Neal:

Neal Liu <[e-mail]> 於 2020年7月24日 週五 下午2:55寫道：
>
> Hi Chun-Kuang,
>
> On Fri, 2020-07-24 at 00:32 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu <[e-mail]> 於 2020年7月23日 週四 下午2:11寫道：
> > >
> > > Hi Chun-Kuang,
> > >
> > > On Wed, 2020-07-22 at 22:25 +0800, Chun-Kuang Hu wrote:
> > > > Hi, Neal:
> > > >
> > > > Neal Liu <[e-mail]> 於 2020年7月22日 週三 上午11:49寫道：
> > > > >
> > > > > Hi Chun-Kuang,
> > > > >
> > > > > On Wed, 2020-07-22 at 07:21 +0800, Chun-Kuang Hu wrote:
> > > > > > Hi, Neal:
> > > > > >
> > > > > > Neal Liu <[e-mail]> 於 2020年7月21日 週二 下午12:00寫道：
> > > > > > >
> > > > > >
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * mtk_devapc_dump_vio_dbg - get the violation index and dump the full violation
> > > > > > > + *                           debug information.
> > > > > > > + */
> > > > > > > +static bool mtk_devapc_dump_vio_dbg(struct mtk_devapc_context *ctx, u32 vio_idx)
> > > > > > > +{
> > > > > > > +       u32 shift_bit;
> > > > > > > +
> > > > > > > +       if (check_vio_mask(ctx, vio_idx))
> > > > > > > +               return false;
> > > > > > > +
> > > > > > > +       if (!check_vio_status(ctx, vio_idx))
> > > > > > > +               return false;
> > > > > > > +
> > > > > > > +       shift_bit = get_shift_group(ctx, vio_idx);
> > > > > > > +
> > > > > > > +       if (sync_vio_dbg(ctx, shift_bit))
> > > > > > > +               return false;
> > > > > > > +
> > > > > > > +       devapc_extract_vio_dbg(ctx);
> > > > > >
> > > > > > I think get_shift_group(), sync_vio_dbg(), and
> > > > > > devapc_extract_vio_dbg() should be moved out of vio_idx for-loop (the
> > > > > > loop in devapc_violation_irq()) because these three function is not
> > > > > > related to vio_idx.
> > > > > > Another question: when multiple vio_idx violation occur, vio_addr is
> > > > > > related to which one vio_idx? The latest happened one?
> > > > > >
> > > > >
> > > > > Actually, it's related to vio_idx. But we don't use it directly on these
> > > > > function. I think below snip code might be better way to understand it.
> > > > >
> > > > > for (...)
> > > > > {
> > > > >         check_vio_mask()
> > > > >         check_vio_status()
> > > > >
> > > > >         // if get vio_idx, mask it temporarily
> > > > >         mask_module_irq(true)
> > > > >         clear_vio_status()
> > > > >
> > > > >         // dump violation info
> > > > >         get_shift_group()
> > > > >         sync_vio_dbg()
> > > > >         devapc_extract_vio_dbg()
> > > > >
> > > > >         // unmask
> > > > >         mask_module_irq(false)
> > > > > }
> > > >
> > > > This snip code does not explain any thing. I could rewrite this code as:
> > > >
> > > > for (...)
> > > > {
> > > >     check_vio_mask()
> > > >     check_vio_status()
> > > >
> > > >     // if get vio_idx, mask it temporarily
> > > >     mask_module_irq(true)
> > > >     clear_vio_status()
> > > >     // unmask
> > > >     mask_module_irq(false)
> > > > }
> > > >
> > > > // dump violation info
> > > > get_shift_group()
> > > > sync_vio_dbg()
> > > > devapc_extract_vio_dbg()
> > > >
> > > > And my version is identical with your version, isn't it?
> > >
> > > Sorry, I did not explain it clearly. Let's me try again.
> > > The reason why I put "dump violation info" between mask & unmask context
> > > is because it has to stop interrupt first before dump violation info,
> > > and then unmask it to prepare next violation.
> > > These sequence guarantee that if multiple violation is triggered, we
> > > still have information to debug.
> > > If the code sequence in your version and multiple violation is
> > > triggered, there might be no any information but keeps entering ISR.
> > > Finally, system might be abnormal and watchdog timeout.
> > > In this case, we still don't have any information to debug.
> >
> > I still don't understand why no information to debug. For example when
> > vio_idx 5, 10, 15 has violation,
> > You would mask vio_idx 5 to get information, but vio_idx 10, 15 does
> > not mask yet.
> > In your words, when vio_idx 10, 15 not mask, you would not get any
> > debug information when you process vio_idx 5.
> >
> > In my version, I would clear all status, why keeps entering ISR?
>
> Think about this case, if someone tries to dump "AAA" module's register.
> It would keep read reg base, base+0x4, base+0x8, ...
> All these registers are in the same slave, which would be same vio_idx.
> (Take vio_idx 5 as example)
> In this case, vio_idx 5 will keep triggering interrupt. If you did not
> do "dump violation info" between mask & unmask, you cannot get any
> violation info until the last interrupt being handled.
> Normally, system will crash before last interrupt coming.

You have said that first vio_addr would be kept until it's 'handled'.
So the first vio_addr reg_base would be kept even though other
violation happen. And I could handle (clear status and dump info) it
then vio_addr would next violation's address. I'm confused with your
statement. If AAA is dumping register of vio_idx 5, BBB is dumping
register of vio_idx 10, CCC is dumping register of vio_idx 15, I think
you should mask all vio_idx not only one. So the code would be

for all vio_idx {
    mask_module_irq(true)
}

devapc_extract_vio_dbg()

for all vio_idx {
    clear_vio_status()
    mask_module_irq(false)
}

>
> >
> > >
> > > >
> > > > >
> > > > > About your question, vio_addr would be the first one.
> > > >
> > > > So other vio_addr would be dropped? Or hardware would keep all
> > > > vio_addr and you have some way to get all vio_addr?
> > > >
> > >
> > > In this case, hardware will drop other violation info and keep the first
> > > one until it been handled.
> >
> > Does 'handled' mean status is cleared?
>
> "handled" means clear status and dump violation info.
>
> >
> > Regards,
> > Chun-Kuang.
> >
> > >
> > > > >
> > > > > > > +
> > > > > > > +       return true;
> > > > > > > +}
> > > > > > > +
> > > > > > > +/*
> > > > > > > + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> > > > > > > + *                        violation information including which master violates
> > > > > > > + *                        access slave.
> > > > > > > + */
> > > > > > > +static irqreturn_t devapc_violation_irq(int irq_number,
> > > > > > > +                                       struct mtk_devapc_context *ctx)
> > > > > > > +{
> > > > > > > +       u32 vio_idx;
> > > > > > > +
> > > > > > > +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
> > > > > > > +               if (!mtk_devapc_dump_vio_dbg(ctx, vio_idx))
> > > > > > > +                       continue;
> > > > > > > +
> > > > > > > +               /* Ensure that violation info are written before
> > > > > > > +                * further operations
> > > > > > > +                */
> > > > > > > +               smp_mb();
> > > > > > > +
> > > > > > > +               /*
> > > > > > > +                * Mask slave's irq before clearing vio status.
> > > > > > > +                * Must do it to avoid nested interrupt and prevent
> > > > > > > +                * unexpected behavior.
> > > > > > > +                */
> > > > > > > +               mask_module_irq(ctx, vio_idx, true);
> > > > > > > +
> > > > > > > +               clear_vio_status(ctx, vio_idx);
> > > > > > > +
> > > > > > > +               mask_module_irq(ctx, vio_idx, false);
> > > > > > > +       }
> > > > > > > +
> > > > > > > +       return IRQ_HANDLED;
> > > > > > > +}
> > > > > > > +
> > > > > > > +/*
>

Neal Liu July 27, 2020, 3:05 a.m. UTC | #8

Hi Chun-Kuang,

On Fri, 2020-07-24 at 23:55 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
> 
> Neal Liu <[e-mail]> 於 2020年7月24日 週五 下午2:55寫道：
> >
> > Hi Chun-Kuang,
> >
> > On Fri, 2020-07-24 at 00:32 +0800, Chun-Kuang Hu wrote:
> > > Hi, Neal:
> > >
> > > Neal Liu <[e-mail]> 於 2020年7月23日 週四 下午2:11寫道：
> > > >
> > > > Hi Chun-Kuang,
> > > >
> > > > On Wed, 2020-07-22 at 22:25 +0800, Chun-Kuang Hu wrote:
> > > > > Hi, Neal:
> > > > >
> > > > > Neal Liu <[e-mail]> 於 2020年7月22日 週三 上午11:49寫道：
> > > > > >
> > > > > > Hi Chun-Kuang,
> > > > > >
> > > > > > On Wed, 2020-07-22 at 07:21 +0800, Chun-Kuang Hu wrote:
> > > > > > > Hi, Neal:
> > > > > > >
> > > > > > > Neal Liu <[e-mail]> 於 2020年7月21日 週二 下午12:00寫道：
> > > > > > > >
> > > > > > >
> > > > > > > > +
> > > > > > > > +/*
> > > > > > > > + * mtk_devapc_dump_vio_dbg - get the violation index and dump the full violation
> > > > > > > > + *                           debug information.
> > > > > > > > + */
> > > > > > > > +static bool mtk_devapc_dump_vio_dbg(struct mtk_devapc_context *ctx, u32 vio_idx)
> > > > > > > > +{
> > > > > > > > +       u32 shift_bit;
> > > > > > > > +
> > > > > > > > +       if (check_vio_mask(ctx, vio_idx))
> > > > > > > > +               return false;
> > > > > > > > +
> > > > > > > > +       if (!check_vio_status(ctx, vio_idx))
> > > > > > > > +               return false;
> > > > > > > > +
> > > > > > > > +       shift_bit = get_shift_group(ctx, vio_idx);
> > > > > > > > +
> > > > > > > > +       if (sync_vio_dbg(ctx, shift_bit))
> > > > > > > > +               return false;
> > > > > > > > +
> > > > > > > > +       devapc_extract_vio_dbg(ctx);
> > > > > > >
> > > > > > > I think get_shift_group(), sync_vio_dbg(), and
> > > > > > > devapc_extract_vio_dbg() should be moved out of vio_idx for-loop (the
> > > > > > > loop in devapc_violation_irq()) because these three function is not
> > > > > > > related to vio_idx.
> > > > > > > Another question: when multiple vio_idx violation occur, vio_addr is
> > > > > > > related to which one vio_idx? The latest happened one?
> > > > > > >
> > > > > >
> > > > > > Actually, it's related to vio_idx. But we don't use it directly on these
> > > > > > function. I think below snip code might be better way to understand it.
> > > > > >
> > > > > > for (...)
> > > > > > {
> > > > > >         check_vio_mask()
> > > > > >         check_vio_status()
> > > > > >
> > > > > >         // if get vio_idx, mask it temporarily
> > > > > >         mask_module_irq(true)
> > > > > >         clear_vio_status()
> > > > > >
> > > > > >         // dump violation info
> > > > > >         get_shift_group()
> > > > > >         sync_vio_dbg()
> > > > > >         devapc_extract_vio_dbg()
> > > > > >
> > > > > >         // unmask
> > > > > >         mask_module_irq(false)
> > > > > > }
> > > > >
> > > > > This snip code does not explain any thing. I could rewrite this code as:
> > > > >
> > > > > for (...)
> > > > > {
> > > > >     check_vio_mask()
> > > > >     check_vio_status()
> > > > >
> > > > >     // if get vio_idx, mask it temporarily
> > > > >     mask_module_irq(true)
> > > > >     clear_vio_status()
> > > > >     // unmask
> > > > >     mask_module_irq(false)
> > > > > }
> > > > >
> > > > > // dump violation info
> > > > > get_shift_group()
> > > > > sync_vio_dbg()
> > > > > devapc_extract_vio_dbg()
> > > > >
> > > > > And my version is identical with your version, isn't it?
> > > >
> > > > Sorry, I did not explain it clearly. Let's me try again.
> > > > The reason why I put "dump violation info" between mask & unmask context
> > > > is because it has to stop interrupt first before dump violation info,
> > > > and then unmask it to prepare next violation.
> > > > These sequence guarantee that if multiple violation is triggered, we
> > > > still have information to debug.
> > > > If the code sequence in your version and multiple violation is
> > > > triggered, there might be no any information but keeps entering ISR.
> > > > Finally, system might be abnormal and watchdog timeout.
> > > > In this case, we still don't have any information to debug.
> > >
> > > I still don't understand why no information to debug. For example when
> > > vio_idx 5, 10, 15 has violation,
> > > You would mask vio_idx 5 to get information, but vio_idx 10, 15 does
> > > not mask yet.
> > > In your words, when vio_idx 10, 15 not mask, you would not get any
> > > debug information when you process vio_idx 5.
> > >
> > > In my version, I would clear all status, why keeps entering ISR?
> >
> > Think about this case, if someone tries to dump "AAA" module's register.
> > It would keep read reg base, base+0x4, base+0x8, ...
> > All these registers are in the same slave, which would be same vio_idx.
> > (Take vio_idx 5 as example)
> > In this case, vio_idx 5 will keep triggering interrupt. If you did not
> > do "dump violation info" between mask & unmask, you cannot get any
> > violation info until the last interrupt being handled.
> > Normally, system will crash before last interrupt coming.
> 
> You have said that first vio_addr would be kept until it's 'handled'.
> So the first vio_addr reg_base would be kept even though other
> violation happen. And I could handle (clear status and dump info) it
> then vio_addr would next violation's address. I'm confused with your
> statement. If AAA is dumping register of vio_idx 5, BBB is dumping
> register of vio_idx 10, CCC is dumping register of vio_idx 15, I think
> you should mask all vio_idx not only one. So the code would be
> 
> for all vio_idx {
>     mask_module_irq(true)
> }
> 
> devapc_extract_vio_dbg()
> 
> for all vio_idx {
>     clear_vio_status()
>     mask_module_irq(false)
> }
> 

I'm also consider this solution and I think it's much better to
understand hardware behavior.

devapc_dump_vio_dbg()
{
	while(1) {
		// might have multiple shift_bit raised
		shift_bit = get_shift_group()
		if (shift_bit >= 0 && shift bit <= 31)
			sync_vio_dbg(shift_bit)
			extract_vio_dbg()
		else
			break
	}
}

devapc_violation_irq()
{
	for all vio_idx {
		mask_module_irq(true)
	}

	devapc_dump_vio_dbg()

	for all vio_idx {
		clear_vio_status()
		mask_module_irq(false)
	}
}

Is it more clear for this control flow?
Thanks !

> >
> > >
> > > >
> > > > >
> > > > > >
> > > > > > About your question, vio_addr would be the first one.
> > > > >
> > > > > So other vio_addr would be dropped? Or hardware would keep all
> > > > > vio_addr and you have some way to get all vio_addr?
> > > > >
> > > >
> > > > In this case, hardware will drop other violation info and keep the first
> > > > one until it been handled.
> > >
> > > Does 'handled' mean status is cleared?
> >
> > "handled" means clear status and dump violation info.
> >
> > >
> > > Regards,
> > > Chun-Kuang.
> > >
> > > >
> > > > > >
> > > > > > > > +
> > > > > > > > +       return true;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +/*
> > > > > > > > + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> > > > > > > > + *                        violation information including which master violates
> > > > > > > > + *                        access slave.
> > > > > > > > + */
> > > > > > > > +static irqreturn_t devapc_violation_irq(int irq_number,
> > > > > > > > +                                       struct mtk_devapc_context *ctx)
> > > > > > > > +{
> > > > > > > > +       u32 vio_idx;
> > > > > > > > +
> > > > > > > > +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
> > > > > > > > +               if (!mtk_devapc_dump_vio_dbg(ctx, vio_idx))
> > > > > > > > +                       continue;
> > > > > > > > +
> > > > > > > > +               /* Ensure that violation info are written before
> > > > > > > > +                * further operations
> > > > > > > > +                */
> > > > > > > > +               smp_mb();
> > > > > > > > +
> > > > > > > > +               /*
> > > > > > > > +                * Mask slave's irq before clearing vio status.
> > > > > > > > +                * Must do it to avoid nested interrupt and prevent
> > > > > > > > +                * unexpected behavior.
> > > > > > > > +                */
> > > > > > > > +               mask_module_irq(ctx, vio_idx, true);
> > > > > > > > +
> > > > > > > > +               clear_vio_status(ctx, vio_idx);
> > > > > > > > +
> > > > > > > > +               mask_module_irq(ctx, vio_idx, false);
> > > > > > > > +       }
> > > > > > > > +
> > > > > > > > +       return IRQ_HANDLED;
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > +/*
> >

Chun-Kuang Hu July 27, 2020, 2:47 p.m. UTC | #9

Hi, Neal:

Neal Liu <[e-mail]> 於 2020年7月27日 週一 上午11:06寫道：
>
> Hi Chun-Kuang,
>
> On Fri, 2020-07-24 at 23:55 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu <[e-mail]> 於 2020年7月24日 週五 下午2:55寫道：
> > >
> > > Hi Chun-Kuang,
> > >
> > > On Fri, 2020-07-24 at 00:32 +0800, Chun-Kuang Hu wrote:
> > > > Hi, Neal:
> > > >
> > > > Neal Liu <[e-mail]> 於 2020年7月23日 週四 下午2:11寫道：
> > > > >
> > > > > Hi Chun-Kuang,
> > > > >
> > > > > On Wed, 2020-07-22 at 22:25 +0800, Chun-Kuang Hu wrote:
> > > > > > Hi, Neal:
> > > > > >
> > > > > > Neal Liu <[e-mail]> 於 2020年7月22日 週三 上午11:49寫道：
> > > > > > >
> > > > > > > Hi Chun-Kuang,
> > > > > > >
> > > > > > > On Wed, 2020-07-22 at 07:21 +0800, Chun-Kuang Hu wrote:
> > > > > > > > Hi, Neal:
> > > > > > > >
> > > > > > > > Neal Liu <[e-mail]> 於 2020年7月21日 週二 下午12:00寫道：
> > > > > > > > >
> > > > > > > >
> > > > > > > > > +
> > > > > > > > > +/*
> > > > > > > > > + * mtk_devapc_dump_vio_dbg - get the violation index and dump the full violation
> > > > > > > > > + *                           debug information.
> > > > > > > > > + */
> > > > > > > > > +static bool mtk_devapc_dump_vio_dbg(struct mtk_devapc_context *ctx, u32 vio_idx)
> > > > > > > > > +{
> > > > > > > > > +       u32 shift_bit;
> > > > > > > > > +
> > > > > > > > > +       if (check_vio_mask(ctx, vio_idx))
> > > > > > > > > +               return false;
> > > > > > > > > +
> > > > > > > > > +       if (!check_vio_status(ctx, vio_idx))
> > > > > > > > > +               return false;
> > > > > > > > > +
> > > > > > > > > +       shift_bit = get_shift_group(ctx, vio_idx);
> > > > > > > > > +
> > > > > > > > > +       if (sync_vio_dbg(ctx, shift_bit))
> > > > > > > > > +               return false;
> > > > > > > > > +
> > > > > > > > > +       devapc_extract_vio_dbg(ctx);
> > > > > > > >
> > > > > > > > I think get_shift_group(), sync_vio_dbg(), and
> > > > > > > > devapc_extract_vio_dbg() should be moved out of vio_idx for-loop (the
> > > > > > > > loop in devapc_violation_irq()) because these three function is not
> > > > > > > > related to vio_idx.
> > > > > > > > Another question: when multiple vio_idx violation occur, vio_addr is
> > > > > > > > related to which one vio_idx? The latest happened one?
> > > > > > > >
> > > > > > >
> > > > > > > Actually, it's related to vio_idx. But we don't use it directly on these
> > > > > > > function. I think below snip code might be better way to understand it.
> > > > > > >
> > > > > > > for (...)
> > > > > > > {
> > > > > > >         check_vio_mask()
> > > > > > >         check_vio_status()
> > > > > > >
> > > > > > >         // if get vio_idx, mask it temporarily
> > > > > > >         mask_module_irq(true)
> > > > > > >         clear_vio_status()
> > > > > > >
> > > > > > >         // dump violation info
> > > > > > >         get_shift_group()
> > > > > > >         sync_vio_dbg()
> > > > > > >         devapc_extract_vio_dbg()
> > > > > > >
> > > > > > >         // unmask
> > > > > > >         mask_module_irq(false)
> > > > > > > }
> > > > > >
> > > > > > This snip code does not explain any thing. I could rewrite this code as:
> > > > > >
> > > > > > for (...)
> > > > > > {
> > > > > >     check_vio_mask()
> > > > > >     check_vio_status()
> > > > > >
> > > > > >     // if get vio_idx, mask it temporarily
> > > > > >     mask_module_irq(true)
> > > > > >     clear_vio_status()
> > > > > >     // unmask
> > > > > >     mask_module_irq(false)
> > > > > > }
> > > > > >
> > > > > > // dump violation info
> > > > > > get_shift_group()
> > > > > > sync_vio_dbg()
> > > > > > devapc_extract_vio_dbg()
> > > > > >
> > > > > > And my version is identical with your version, isn't it?
> > > > >
> > > > > Sorry, I did not explain it clearly. Let's me try again.
> > > > > The reason why I put "dump violation info" between mask & unmask context
> > > > > is because it has to stop interrupt first before dump violation info,
> > > > > and then unmask it to prepare next violation.
> > > > > These sequence guarantee that if multiple violation is triggered, we
> > > > > still have information to debug.
> > > > > If the code sequence in your version and multiple violation is
> > > > > triggered, there might be no any information but keeps entering ISR.
> > > > > Finally, system might be abnormal and watchdog timeout.
> > > > > In this case, we still don't have any information to debug.
> > > >
> > > > I still don't understand why no information to debug. For example when
> > > > vio_idx 5, 10, 15 has violation,
> > > > You would mask vio_idx 5 to get information, but vio_idx 10, 15 does
> > > > not mask yet.
> > > > In your words, when vio_idx 10, 15 not mask, you would not get any
> > > > debug information when you process vio_idx 5.
> > > >
> > > > In my version, I would clear all status, why keeps entering ISR?
> > >
> > > Think about this case, if someone tries to dump "AAA" module's register.
> > > It would keep read reg base, base+0x4, base+0x8, ...
> > > All these registers are in the same slave, which would be same vio_idx.
> > > (Take vio_idx 5 as example)
> > > In this case, vio_idx 5 will keep triggering interrupt. If you did not
> > > do "dump violation info" between mask & unmask, you cannot get any
> > > violation info until the last interrupt being handled.
> > > Normally, system will crash before last interrupt coming.
> >
> > You have said that first vio_addr would be kept until it's 'handled'.
> > So the first vio_addr reg_base would be kept even though other
> > violation happen. And I could handle (clear status and dump info) it
> > then vio_addr would next violation's address. I'm confused with your
> > statement. If AAA is dumping register of vio_idx 5, BBB is dumping
> > register of vio_idx 10, CCC is dumping register of vio_idx 15, I think
> > you should mask all vio_idx not only one. So the code would be
> >
> > for all vio_idx {
> >     mask_module_irq(true)
> > }
> >
> > devapc_extract_vio_dbg()
> >
> > for all vio_idx {
> >     clear_vio_status()
> >     mask_module_irq(false)
> > }
> >
>
> I'm also consider this solution and I think it's much better to
> understand hardware behavior.
>
> devapc_dump_vio_dbg()
> {
>         while(1) {
>                 // might have multiple shift_bit raised
>                 shift_bit = get_shift_group()
>                 if (shift_bit >= 0 && shift bit <= 31)
>                         sync_vio_dbg(shift_bit)
>                         extract_vio_dbg()

According to your statement, when multiple violation occur, only the
first one is kept, others are dropped. I think we just need to dump
debug info once.

Because only one violation information would be kept, why not only one
group (equal to no group)?

Regards,
Chun-Kuang.

>                 else
>                         break
>         }
> }
>
> devapc_violation_irq()
> {
>         for all vio_idx {
>                 mask_module_irq(true)
>         }
>
>         devapc_dump_vio_dbg()
>
>         for all vio_idx {
>                 clear_vio_status()
>                 mask_module_irq(false)
>         }
> }
>
> Is it more clear for this control flow?
> Thanks !
>
> > >
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > About your question, vio_addr would be the first one.
> > > > > >
> > > > > > So other vio_addr would be dropped? Or hardware would keep all
> > > > > > vio_addr and you have some way to get all vio_addr?
> > > > > >
> > > > >
> > > > > In this case, hardware will drop other violation info and keep the first
> > > > > one until it been handled.
> > > >
> > > > Does 'handled' mean status is cleared?
> > >
> > > "handled" means clear status and dump violation info.
> > >
> > > >
> > > > Regards,
> > > > Chun-Kuang.
> > > >
> > > > >
> > > > > > >
> > > > > > > > > +
> > > > > > > > > +       return true;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +/*
> > > > > > > > > + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> > > > > > > > > + *                        violation information including which master violates
> > > > > > > > > + *                        access slave.
> > > > > > > > > + */
> > > > > > > > > +static irqreturn_t devapc_violation_irq(int irq_number,
> > > > > > > > > +                                       struct mtk_devapc_context *ctx)
> > > > > > > > > +{
> > > > > > > > > +       u32 vio_idx;
> > > > > > > > > +
> > > > > > > > > +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
> > > > > > > > > +               if (!mtk_devapc_dump_vio_dbg(ctx, vio_idx))
> > > > > > > > > +                       continue;
> > > > > > > > > +
> > > > > > > > > +               /* Ensure that violation info are written before
> > > > > > > > > +                * further operations
> > > > > > > > > +                */
> > > > > > > > > +               smp_mb();
> > > > > > > > > +
> > > > > > > > > +               /*
> > > > > > > > > +                * Mask slave's irq before clearing vio status.
> > > > > > > > > +                * Must do it to avoid nested interrupt and prevent
> > > > > > > > > +                * unexpected behavior.
> > > > > > > > > +                */
> > > > > > > > > +               mask_module_irq(ctx, vio_idx, true);
> > > > > > > > > +
> > > > > > > > > +               clear_vio_status(ctx, vio_idx);
> > > > > > > > > +
> > > > > > > > > +               mask_module_irq(ctx, vio_idx, false);
> > > > > > > > > +       }
> > > > > > > > > +
> > > > > > > > > +       return IRQ_HANDLED;
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > +/*
> > >
>

Neal Liu July 28, 2020, 3:52 a.m. UTC | #10

Hi Chun-Kuang,

On Mon, 2020-07-27 at 22:47 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
> 
> Neal Liu <[e-mail]> 於 2020年7月27日 週一 上午11:06寫道：
> >
> > Hi Chun-Kuang,
> >
> > On Fri, 2020-07-24 at 23:55 +0800, Chun-Kuang Hu wrote:
> > > Hi, Neal:
> > >
> > > Neal Liu <[e-mail]> 於 2020年7月24日 週五 下午2:55寫道：
> > > >
> > > > Hi Chun-Kuang,
> > > >
> > > > On Fri, 2020-07-24 at 00:32 +0800, Chun-Kuang Hu wrote:
> > > > > Hi, Neal:
> > > > >
> > > > > Neal Liu <[e-mail]> 於 2020年7月23日 週四 下午2:11寫道：
> > > > > >
> > > > > > Hi Chun-Kuang,
> > > > > >
> > > > > > On Wed, 2020-07-22 at 22:25 +0800, Chun-Kuang Hu wrote:
> > > > > > > Hi, Neal:
> > > > > > >
> > > > > > > Neal Liu <[e-mail]> 於 2020年7月22日 週三 上午11:49寫道：
> > > > > > > >
> > > > > > > > Hi Chun-Kuang,
> > > > > > > >
> > > > > > > > On Wed, 2020-07-22 at 07:21 +0800, Chun-Kuang Hu wrote:
> > > > > > > > > Hi, Neal:
> > > > > > > > >
> > > > > > > > > Neal Liu <[e-mail]> 於 2020年7月21日 週二 下午12:00寫道：
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > > +
> > > > > > > > > > +/*
> > > > > > > > > > + * mtk_devapc_dump_vio_dbg - get the violation index and dump the full violation
> > > > > > > > > > + *                           debug information.
> > > > > > > > > > + */
> > > > > > > > > > +static bool mtk_devapc_dump_vio_dbg(struct mtk_devapc_context *ctx, u32 vio_idx)
> > > > > > > > > > +{
> > > > > > > > > > +       u32 shift_bit;
> > > > > > > > > > +
> > > > > > > > > > +       if (check_vio_mask(ctx, vio_idx))
> > > > > > > > > > +               return false;
> > > > > > > > > > +
> > > > > > > > > > +       if (!check_vio_status(ctx, vio_idx))
> > > > > > > > > > +               return false;
> > > > > > > > > > +
> > > > > > > > > > +       shift_bit = get_shift_group(ctx, vio_idx);
> > > > > > > > > > +
> > > > > > > > > > +       if (sync_vio_dbg(ctx, shift_bit))
> > > > > > > > > > +               return false;
> > > > > > > > > > +
> > > > > > > > > > +       devapc_extract_vio_dbg(ctx);
> > > > > > > > >
> > > > > > > > > I think get_shift_group(), sync_vio_dbg(), and
> > > > > > > > > devapc_extract_vio_dbg() should be moved out of vio_idx for-loop (the
> > > > > > > > > loop in devapc_violation_irq()) because these three function is not
> > > > > > > > > related to vio_idx.
> > > > > > > > > Another question: when multiple vio_idx violation occur, vio_addr is
> > > > > > > > > related to which one vio_idx? The latest happened one?
> > > > > > > > >
> > > > > > > >
> > > > > > > > Actually, it's related to vio_idx. But we don't use it directly on these
> > > > > > > > function. I think below snip code might be better way to understand it.
> > > > > > > >
> > > > > > > > for (...)
> > > > > > > > {
> > > > > > > >         check_vio_mask()
> > > > > > > >         check_vio_status()
> > > > > > > >
> > > > > > > >         // if get vio_idx, mask it temporarily
> > > > > > > >         mask_module_irq(true)
> > > > > > > >         clear_vio_status()
> > > > > > > >
> > > > > > > >         // dump violation info
> > > > > > > >         get_shift_group()
> > > > > > > >         sync_vio_dbg()
> > > > > > > >         devapc_extract_vio_dbg()
> > > > > > > >
> > > > > > > >         // unmask
> > > > > > > >         mask_module_irq(false)
> > > > > > > > }
> > > > > > >
> > > > > > > This snip code does not explain any thing. I could rewrite this code as:
> > > > > > >
> > > > > > > for (...)
> > > > > > > {
> > > > > > >     check_vio_mask()
> > > > > > >     check_vio_status()
> > > > > > >
> > > > > > >     // if get vio_idx, mask it temporarily
> > > > > > >     mask_module_irq(true)
> > > > > > >     clear_vio_status()
> > > > > > >     // unmask
> > > > > > >     mask_module_irq(false)
> > > > > > > }
> > > > > > >
> > > > > > > // dump violation info
> > > > > > > get_shift_group()
> > > > > > > sync_vio_dbg()
> > > > > > > devapc_extract_vio_dbg()
> > > > > > >
> > > > > > > And my version is identical with your version, isn't it?
> > > > > >
> > > > > > Sorry, I did not explain it clearly. Let's me try again.
> > > > > > The reason why I put "dump violation info" between mask & unmask context
> > > > > > is because it has to stop interrupt first before dump violation info,
> > > > > > and then unmask it to prepare next violation.
> > > > > > These sequence guarantee that if multiple violation is triggered, we
> > > > > > still have information to debug.
> > > > > > If the code sequence in your version and multiple violation is
> > > > > > triggered, there might be no any information but keeps entering ISR.
> > > > > > Finally, system might be abnormal and watchdog timeout.
> > > > > > In this case, we still don't have any information to debug.
> > > > >
> > > > > I still don't understand why no information to debug. For example when
> > > > > vio_idx 5, 10, 15 has violation,
> > > > > You would mask vio_idx 5 to get information, but vio_idx 10, 15 does
> > > > > not mask yet.
> > > > > In your words, when vio_idx 10, 15 not mask, you would not get any
> > > > > debug information when you process vio_idx 5.
> > > > >
> > > > > In my version, I would clear all status, why keeps entering ISR?
> > > >
> > > > Think about this case, if someone tries to dump "AAA" module's register.
> > > > It would keep read reg base, base+0x4, base+0x8, ...
> > > > All these registers are in the same slave, which would be same vio_idx.
> > > > (Take vio_idx 5 as example)
> > > > In this case, vio_idx 5 will keep triggering interrupt. If you did not
> > > > do "dump violation info" between mask & unmask, you cannot get any
> > > > violation info until the last interrupt being handled.
> > > > Normally, system will crash before last interrupt coming.
> > >
> > > You have said that first vio_addr would be kept until it's 'handled'.
> > > So the first vio_addr reg_base would be kept even though other
> > > violation happen. And I could handle (clear status and dump info) it
> > > then vio_addr would next violation's address. I'm confused with your
> > > statement. If AAA is dumping register of vio_idx 5, BBB is dumping
> > > register of vio_idx 10, CCC is dumping register of vio_idx 15, I think
> > > you should mask all vio_idx not only one. So the code would be
> > >
> > > for all vio_idx {
> > >     mask_module_irq(true)
> > > }
> > >
> > > devapc_extract_vio_dbg()
> > >
> > > for all vio_idx {
> > >     clear_vio_status()
> > >     mask_module_irq(false)
> > > }
> > >
> >
> > I'm also consider this solution and I think it's much better to
> > understand hardware behavior.
> >
> > devapc_dump_vio_dbg()
> > {
> >         while(1) {
> >                 // might have multiple shift_bit raised
> >                 shift_bit = get_shift_group()
> >                 if (shift_bit >= 0 && shift bit <= 31)
> >                         sync_vio_dbg(shift_bit)
> >                         extract_vio_dbg()
> 
> According to your statement, when multiple violation occur, only the
> first one is kept, others are dropped. I think we just need to dump
> debug info once.
> 
> Because only one violation information would be kept, why not only one
> group (equal to no group)?
> 
> Regards,
> Chun-Kuang.

Let's me give you an example of devapc design.
vio_idx: 0, 1, 2 -> group 0 (shift_bit: 0)
vio_idx: 3, 4, 5 -> group 1 (shift_bit: 1)
...

Each group violation will keep one violation (the first one). If vio_idx
0 is triggered first, vio_idx 1 is triggered next, then group 0 will
just keep vio_idx 0 violation info.
If vio_idx 2 is triggered first, vio_idx 3 is triggered next, then group
0 will keep vio_idx 2 violation info, group 1 will keep vio_idx 3's.

We have to scan all groups and dump everything we have.
Thanks !

> 
> >                 else
> >                         break
> >         }
> > }
> >
> > devapc_violation_irq()
> > {
> >         for all vio_idx {
> >                 mask_module_irq(true)
> >         }
> >
> >         devapc_dump_vio_dbg()
> >
> >         for all vio_idx {
> >                 clear_vio_status()
> >                 mask_module_irq(false)
> >         }
> > }
> >
> > Is it more clear for this control flow?
> > Thanks !
> >
> > > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > About your question, vio_addr would be the first one.
> > > > > > >
> > > > > > > So other vio_addr would be dropped? Or hardware would keep all
> > > > > > > vio_addr and you have some way to get all vio_addr?
> > > > > > >
> > > > > >
> > > > > > In this case, hardware will drop other violation info and keep the first
> > > > > > one until it been handled.
> > > > >
> > > > > Does 'handled' mean status is cleared?
> > > >
> > > > "handled" means clear status and dump violation info.
> > > >
> > > > >
> > > > > Regards,
> > > > > Chun-Kuang.
> > > > >
> > > > > >
> > > > > > > >
> > > > > > > > > > +
> > > > > > > > > > +       return true;
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +/*
> > > > > > > > > > + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> > > > > > > > > > + *                        violation information including which master violates
> > > > > > > > > > + *                        access slave.
> > > > > > > > > > + */
> > > > > > > > > > +static irqreturn_t devapc_violation_irq(int irq_number,
> > > > > > > > > > +                                       struct mtk_devapc_context *ctx)
> > > > > > > > > > +{
> > > > > > > > > > +       u32 vio_idx;
> > > > > > > > > > +
> > > > > > > > > > +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
> > > > > > > > > > +               if (!mtk_devapc_dump_vio_dbg(ctx, vio_idx))
> > > > > > > > > > +                       continue;
> > > > > > > > > > +
> > > > > > > > > > +               /* Ensure that violation info are written before
> > > > > > > > > > +                * further operations
> > > > > > > > > > +                */
> > > > > > > > > > +               smp_mb();
> > > > > > > > > > +
> > > > > > > > > > +               /*
> > > > > > > > > > +                * Mask slave's irq before clearing vio status.
> > > > > > > > > > +                * Must do it to avoid nested interrupt and prevent
> > > > > > > > > > +                * unexpected behavior.
> > > > > > > > > > +                */
> > > > > > > > > > +               mask_module_irq(ctx, vio_idx, true);
> > > > > > > > > > +
> > > > > > > > > > +               clear_vio_status(ctx, vio_idx);
> > > > > > > > > > +
> > > > > > > > > > +               mask_module_irq(ctx, vio_idx, false);
> > > > > > > > > > +       }
> > > > > > > > > > +
> > > > > > > > > > +       return IRQ_HANDLED;
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > +/*
> > > >
> >

Chun-Kuang Hu July 28, 2020, 3:35 p.m. UTC | #11

Hi, Neal:

Neal Liu <[e-mail]> 於 2020年7月28日 週二 上午11:52寫道：
>
> Hi Chun-Kuang,
>
> On Mon, 2020-07-27 at 22:47 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu <[e-mail]> 於 2020年7月27日 週一 上午11:06寫道：
> > >
> > > Hi Chun-Kuang,
> > >
> > > On Fri, 2020-07-24 at 23:55 +0800, Chun-Kuang Hu wrote:
> > > > Hi, Neal:
> > > >
> > > > Neal Liu <[e-mail]> 於 2020年7月24日 週五 下午2:55寫道：
> > > > >
> > > > > Hi Chun-Kuang,
> > > > >
> > > > > On Fri, 2020-07-24 at 00:32 +0800, Chun-Kuang Hu wrote:
> > > > > > Hi, Neal:
> > > > > >
> > > > > > Neal Liu <[e-mail]> 於 2020年7月23日 週四 下午2:11寫道：
> > > > > > >
> > > > > > > Hi Chun-Kuang,
> > > > > > >
> > > > > > > On Wed, 2020-07-22 at 22:25 +0800, Chun-Kuang Hu wrote:
> > > > > > > > Hi, Neal:
> > > > > > > >
> > > > > > > > Neal Liu <[e-mail]> 於 2020年7月22日 週三 上午11:49寫道：
> > > > > > > > >
> > > > > > > > > Hi Chun-Kuang,
> > > > > > > > >
> > > > > > > > > On Wed, 2020-07-22 at 07:21 +0800, Chun-Kuang Hu wrote:
> > > > > > > > > > Hi, Neal:
> > > > > > > > > >
> > > > > > > > > > Neal Liu <[e-mail]> 於 2020年7月21日 週二 下午12:00寫道：
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > +
> > > > > > > > > > > +/*
> > > > > > > > > > > + * mtk_devapc_dump_vio_dbg - get the violation index and dump the full violation
> > > > > > > > > > > + *                           debug information.
> > > > > > > > > > > + */
> > > > > > > > > > > +static bool mtk_devapc_dump_vio_dbg(struct mtk_devapc_context *ctx, u32 vio_idx)
> > > > > > > > > > > +{
> > > > > > > > > > > +       u32 shift_bit;
> > > > > > > > > > > +
> > > > > > > > > > > +       if (check_vio_mask(ctx, vio_idx))
> > > > > > > > > > > +               return false;
> > > > > > > > > > > +
> > > > > > > > > > > +       if (!check_vio_status(ctx, vio_idx))
> > > > > > > > > > > +               return false;
> > > > > > > > > > > +
> > > > > > > > > > > +       shift_bit = get_shift_group(ctx, vio_idx);
> > > > > > > > > > > +
> > > > > > > > > > > +       if (sync_vio_dbg(ctx, shift_bit))
> > > > > > > > > > > +               return false;
> > > > > > > > > > > +
> > > > > > > > > > > +       devapc_extract_vio_dbg(ctx);
> > > > > > > > > >
> > > > > > > > > > I think get_shift_group(), sync_vio_dbg(), and
> > > > > > > > > > devapc_extract_vio_dbg() should be moved out of vio_idx for-loop (the
> > > > > > > > > > loop in devapc_violation_irq()) because these three function is not
> > > > > > > > > > related to vio_idx.
> > > > > > > > > > Another question: when multiple vio_idx violation occur, vio_addr is
> > > > > > > > > > related to which one vio_idx? The latest happened one?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > Actually, it's related to vio_idx. But we don't use it directly on these
> > > > > > > > > function. I think below snip code might be better way to understand it.
> > > > > > > > >
> > > > > > > > > for (...)
> > > > > > > > > {
> > > > > > > > >         check_vio_mask()
> > > > > > > > >         check_vio_status()
> > > > > > > > >
> > > > > > > > >         // if get vio_idx, mask it temporarily
> > > > > > > > >         mask_module_irq(true)
> > > > > > > > >         clear_vio_status()
> > > > > > > > >
> > > > > > > > >         // dump violation info
> > > > > > > > >         get_shift_group()
> > > > > > > > >         sync_vio_dbg()
> > > > > > > > >         devapc_extract_vio_dbg()
> > > > > > > > >
> > > > > > > > >         // unmask
> > > > > > > > >         mask_module_irq(false)
> > > > > > > > > }
> > > > > > > >
> > > > > > > > This snip code does not explain any thing. I could rewrite this code as:
> > > > > > > >
> > > > > > > > for (...)
> > > > > > > > {
> > > > > > > >     check_vio_mask()
> > > > > > > >     check_vio_status()
> > > > > > > >
> > > > > > > >     // if get vio_idx, mask it temporarily
> > > > > > > >     mask_module_irq(true)
> > > > > > > >     clear_vio_status()
> > > > > > > >     // unmask
> > > > > > > >     mask_module_irq(false)
> > > > > > > > }
> > > > > > > >
> > > > > > > > // dump violation info
> > > > > > > > get_shift_group()
> > > > > > > > sync_vio_dbg()
> > > > > > > > devapc_extract_vio_dbg()
> > > > > > > >
> > > > > > > > And my version is identical with your version, isn't it?
> > > > > > >
> > > > > > > Sorry, I did not explain it clearly. Let's me try again.
> > > > > > > The reason why I put "dump violation info" between mask & unmask context
> > > > > > > is because it has to stop interrupt first before dump violation info,
> > > > > > > and then unmask it to prepare next violation.
> > > > > > > These sequence guarantee that if multiple violation is triggered, we
> > > > > > > still have information to debug.
> > > > > > > If the code sequence in your version and multiple violation is
> > > > > > > triggered, there might be no any information but keeps entering ISR.
> > > > > > > Finally, system might be abnormal and watchdog timeout.
> > > > > > > In this case, we still don't have any information to debug.
> > > > > >
> > > > > > I still don't understand why no information to debug. For example when
> > > > > > vio_idx 5, 10, 15 has violation,
> > > > > > You would mask vio_idx 5 to get information, but vio_idx 10, 15 does
> > > > > > not mask yet.
> > > > > > In your words, when vio_idx 10, 15 not mask, you would not get any
> > > > > > debug information when you process vio_idx 5.
> > > > > >
> > > > > > In my version, I would clear all status, why keeps entering ISR?
> > > > >
> > > > > Think about this case, if someone tries to dump "AAA" module's register.
> > > > > It would keep read reg base, base+0x4, base+0x8, ...
> > > > > All these registers are in the same slave, which would be same vio_idx.
> > > > > (Take vio_idx 5 as example)
> > > > > In this case, vio_idx 5 will keep triggering interrupt. If you did not
> > > > > do "dump violation info" between mask & unmask, you cannot get any
> > > > > violation info until the last interrupt being handled.
> > > > > Normally, system will crash before last interrupt coming.
> > > >
> > > > You have said that first vio_addr would be kept until it's 'handled'.
> > > > So the first vio_addr reg_base would be kept even though other
> > > > violation happen. And I could handle (clear status and dump info) it
> > > > then vio_addr would next violation's address. I'm confused with your
> > > > statement. If AAA is dumping register of vio_idx 5, BBB is dumping
> > > > register of vio_idx 10, CCC is dumping register of vio_idx 15, I think
> > > > you should mask all vio_idx not only one. So the code would be
> > > >
> > > > for all vio_idx {
> > > >     mask_module_irq(true)
> > > > }
> > > >
> > > > devapc_extract_vio_dbg()
> > > >
> > > > for all vio_idx {
> > > >     clear_vio_status()
> > > >     mask_module_irq(false)
> > > > }
> > > >
> > >
> > > I'm also consider this solution and I think it's much better to
> > > understand hardware behavior.
> > >
> > > devapc_dump_vio_dbg()
> > > {
> > >         while(1) {
> > >                 // might have multiple shift_bit raised
> > >                 shift_bit = get_shift_group()
> > >                 if (shift_bit >= 0 && shift bit <= 31)
> > >                         sync_vio_dbg(shift_bit)
> > >                         extract_vio_dbg()
> >
> > According to your statement, when multiple violation occur, only the
> > first one is kept, others are dropped. I think we just need to dump
> > debug info once.
> >
> > Because only one violation information would be kept, why not only one
> > group (equal to no group)?
> >
> > Regards,
> > Chun-Kuang.
>
> Let's me give you an example of devapc design.
> vio_idx: 0, 1, 2 -> group 0 (shift_bit: 0)
> vio_idx: 3, 4, 5 -> group 1 (shift_bit: 1)
> ...
>
> Each group violation will keep one violation (the first one). If vio_idx
> 0 is triggered first, vio_idx 1 is triggered next, then group 0 will
> just keep vio_idx 0 violation info.
> If vio_idx 2 is triggered first, vio_idx 3 is triggered next, then group
> 0 will keep vio_idx 2 violation info, group 1 will keep vio_idx 3's.
>
> We have to scan all groups and dump everything we have.
> Thanks !
>

Could we let all vio_idx be group 0 so that we could just sync one
group? It's bad to spend too much time in irq handler.
When we set pd_vio_shift_sel_reg, it seems we could set multiple group
together, couldn't it?

Regards,
Chun-Kuang.

> >
> > >                 else
> > >                         break
> > >         }
> > > }
> > >
> > > devapc_violation_irq()
> > > {
> > >         for all vio_idx {
> > >                 mask_module_irq(true)
> > >         }
> > >
> > >         devapc_dump_vio_dbg()
> > >
> > >         for all vio_idx {
> > >                 clear_vio_status()
> > >                 mask_module_irq(false)
> > >         }
> > > }
> > >
> > > Is it more clear for this control flow?
> > > Thanks !
> > >
> > > > >
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > About your question, vio_addr would be the first one.
> > > > > > > >
> > > > > > > > So other vio_addr would be dropped? Or hardware would keep all
> > > > > > > > vio_addr and you have some way to get all vio_addr?
> > > > > > > >
> > > > > > >
> > > > > > > In this case, hardware will drop other violation info and keep the first
> > > > > > > one until it been handled.
> > > > > >
> > > > > > Does 'handled' mean status is cleared?
> > > > >
> > > > > "handled" means clear status and dump violation info.
> > > > >
> > > > > >
> > > > > > Regards,
> > > > > > Chun-Kuang.
> > > > > >
> > > > > > >
> > > > > > > > >
> > > > > > > > > > > +
> > > > > > > > > > > +       return true;
> > > > > > > > > > > +}
> > > > > > > > > > > +
> > > > > > > > > > > +/*
> > > > > > > > > > > + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> > > > > > > > > > > + *                        violation information including which master violates
> > > > > > > > > > > + *                        access slave.
> > > > > > > > > > > + */
> > > > > > > > > > > +static irqreturn_t devapc_violation_irq(int irq_number,
> > > > > > > > > > > +                                       struct mtk_devapc_context *ctx)
> > > > > > > > > > > +{
> > > > > > > > > > > +       u32 vio_idx;
> > > > > > > > > > > +
> > > > > > > > > > > +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
> > > > > > > > > > > +               if (!mtk_devapc_dump_vio_dbg(ctx, vio_idx))
> > > > > > > > > > > +                       continue;
> > > > > > > > > > > +
> > > > > > > > > > > +               /* Ensure that violation info are written before
> > > > > > > > > > > +                * further operations
> > > > > > > > > > > +                */
> > > > > > > > > > > +               smp_mb();
> > > > > > > > > > > +
> > > > > > > > > > > +               /*
> > > > > > > > > > > +                * Mask slave's irq before clearing vio status.
> > > > > > > > > > > +                * Must do it to avoid nested interrupt and prevent
> > > > > > > > > > > +                * unexpected behavior.
> > > > > > > > > > > +                */
> > > > > > > > > > > +               mask_module_irq(ctx, vio_idx, true);
> > > > > > > > > > > +
> > > > > > > > > > > +               clear_vio_status(ctx, vio_idx);
> > > > > > > > > > > +
> > > > > > > > > > > +               mask_module_irq(ctx, vio_idx, false);
> > > > > > > > > > > +       }
> > > > > > > > > > > +
> > > > > > > > > > > +       return IRQ_HANDLED;
> > > > > > > > > > > +}
> > > > > > > > > > > +
> > > > > > > > > > > +/*
> > > > >
> > >
>

Neal Liu July 29, 2020, 2:10 a.m. UTC | #12

Hi Chun-Kuang,

On Tue, 2020-07-28 at 23:35 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
> 
> Neal Liu <[e-mail]> 於 2020年7月28日 週二 上午11:52寫道：
> >
> > Hi Chun-Kuang,
> >
> > On Mon, 2020-07-27 at 22:47 +0800, Chun-Kuang Hu wrote:
> > > Hi, Neal:
> > >
> > > Neal Liu <[e-mail]> 於 2020年7月27日 週一 上午11:06寫道：
> > > >
> > > > Hi Chun-Kuang,
> > > >
> > > > On Fri, 2020-07-24 at 23:55 +0800, Chun-Kuang Hu wrote:
> > > > > Hi, Neal:
> > > > >
> > > > > Neal Liu <[e-mail]> 於 2020年7月24日 週五 下午2:55寫道：
> > > > > >
> > > > > > Hi Chun-Kuang,
> > > > > >
> > > > > > On Fri, 2020-07-24 at 00:32 +0800, Chun-Kuang Hu wrote:
> > > > > > > Hi, Neal:
> > > > > > >
> > > > > > > Neal Liu <[e-mail]> 於 2020年7月23日 週四 下午2:11寫道：
> > > > > > > >
> > > > > > > > Hi Chun-Kuang,
> > > > > > > >
> > > > > > > > On Wed, 2020-07-22 at 22:25 +0800, Chun-Kuang Hu wrote:
> > > > > > > > > Hi, Neal:
> > > > > > > > >
> > > > > > > > > Neal Liu <[e-mail]> 於 2020年7月22日 週三 上午11:49寫道：
> > > > > > > > > >
> > > > > > > > > > Hi Chun-Kuang,
> > > > > > > > > >
> > > > > > > > > > On Wed, 2020-07-22 at 07:21 +0800, Chun-Kuang Hu wrote:
> > > > > > > > > > > Hi, Neal:
> > > > > > > > > > >
> > > > > > > > > > > Neal Liu <[e-mail]> 於 2020年7月21日 週二 下午12:00寫道：
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > +
> > > > > > > > > > > > +/*
> > > > > > > > > > > > + * mtk_devapc_dump_vio_dbg - get the violation index and dump the full violation
> > > > > > > > > > > > + *                           debug information.
> > > > > > > > > > > > + */
> > > > > > > > > > > > +static bool mtk_devapc_dump_vio_dbg(struct mtk_devapc_context *ctx, u32 vio_idx)
> > > > > > > > > > > > +{
> > > > > > > > > > > > +       u32 shift_bit;
> > > > > > > > > > > > +
> > > > > > > > > > > > +       if (check_vio_mask(ctx, vio_idx))
> > > > > > > > > > > > +               return false;
> > > > > > > > > > > > +
> > > > > > > > > > > > +       if (!check_vio_status(ctx, vio_idx))
> > > > > > > > > > > > +               return false;
> > > > > > > > > > > > +
> > > > > > > > > > > > +       shift_bit = get_shift_group(ctx, vio_idx);
> > > > > > > > > > > > +
> > > > > > > > > > > > +       if (sync_vio_dbg(ctx, shift_bit))
> > > > > > > > > > > > +               return false;
> > > > > > > > > > > > +
> > > > > > > > > > > > +       devapc_extract_vio_dbg(ctx);
> > > > > > > > > > >
> > > > > > > > > > > I think get_shift_group(), sync_vio_dbg(), and
> > > > > > > > > > > devapc_extract_vio_dbg() should be moved out of vio_idx for-loop (the
> > > > > > > > > > > loop in devapc_violation_irq()) because these three function is not
> > > > > > > > > > > related to vio_idx.
> > > > > > > > > > > Another question: when multiple vio_idx violation occur, vio_addr is
> > > > > > > > > > > related to which one vio_idx? The latest happened one?
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Actually, it's related to vio_idx. But we don't use it directly on these
> > > > > > > > > > function. I think below snip code might be better way to understand it.
> > > > > > > > > >
> > > > > > > > > > for (...)
> > > > > > > > > > {
> > > > > > > > > >         check_vio_mask()
> > > > > > > > > >         check_vio_status()
> > > > > > > > > >
> > > > > > > > > >         // if get vio_idx, mask it temporarily
> > > > > > > > > >         mask_module_irq(true)
> > > > > > > > > >         clear_vio_status()
> > > > > > > > > >
> > > > > > > > > >         // dump violation info
> > > > > > > > > >         get_shift_group()
> > > > > > > > > >         sync_vio_dbg()
> > > > > > > > > >         devapc_extract_vio_dbg()
> > > > > > > > > >
> > > > > > > > > >         // unmask
> > > > > > > > > >         mask_module_irq(false)
> > > > > > > > > > }
> > > > > > > > >
> > > > > > > > > This snip code does not explain any thing. I could rewrite this code as:
> > > > > > > > >
> > > > > > > > > for (...)
> > > > > > > > > {
> > > > > > > > >     check_vio_mask()
> > > > > > > > >     check_vio_status()
> > > > > > > > >
> > > > > > > > >     // if get vio_idx, mask it temporarily
> > > > > > > > >     mask_module_irq(true)
> > > > > > > > >     clear_vio_status()
> > > > > > > > >     // unmask
> > > > > > > > >     mask_module_irq(false)
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > // dump violation info
> > > > > > > > > get_shift_group()
> > > > > > > > > sync_vio_dbg()
> > > > > > > > > devapc_extract_vio_dbg()
> > > > > > > > >
> > > > > > > > > And my version is identical with your version, isn't it?
> > > > > > > >
> > > > > > > > Sorry, I did not explain it clearly. Let's me try again.
> > > > > > > > The reason why I put "dump violation info" between mask & unmask context
> > > > > > > > is because it has to stop interrupt first before dump violation info,
> > > > > > > > and then unmask it to prepare next violation.
> > > > > > > > These sequence guarantee that if multiple violation is triggered, we
> > > > > > > > still have information to debug.
> > > > > > > > If the code sequence in your version and multiple violation is
> > > > > > > > triggered, there might be no any information but keeps entering ISR.
> > > > > > > > Finally, system might be abnormal and watchdog timeout.
> > > > > > > > In this case, we still don't have any information to debug.
> > > > > > >
> > > > > > > I still don't understand why no information to debug. For example when
> > > > > > > vio_idx 5, 10, 15 has violation,
> > > > > > > You would mask vio_idx 5 to get information, but vio_idx 10, 15 does
> > > > > > > not mask yet.
> > > > > > > In your words, when vio_idx 10, 15 not mask, you would not get any
> > > > > > > debug information when you process vio_idx 5.
> > > > > > >
> > > > > > > In my version, I would clear all status, why keeps entering ISR?
> > > > > >
> > > > > > Think about this case, if someone tries to dump "AAA" module's register.
> > > > > > It would keep read reg base, base+0x4, base+0x8, ...
> > > > > > All these registers are in the same slave, which would be same vio_idx.
> > > > > > (Take vio_idx 5 as example)
> > > > > > In this case, vio_idx 5 will keep triggering interrupt. If you did not
> > > > > > do "dump violation info" between mask & unmask, you cannot get any
> > > > > > violation info until the last interrupt being handled.
> > > > > > Normally, system will crash before last interrupt coming.
> > > > >
> > > > > You have said that first vio_addr would be kept until it's 'handled'.
> > > > > So the first vio_addr reg_base would be kept even though other
> > > > > violation happen. And I could handle (clear status and dump info) it
> > > > > then vio_addr would next violation's address. I'm confused with your
> > > > > statement. If AAA is dumping register of vio_idx 5, BBB is dumping
> > > > > register of vio_idx 10, CCC is dumping register of vio_idx 15, I think
> > > > > you should mask all vio_idx not only one. So the code would be
> > > > >
> > > > > for all vio_idx {
> > > > >     mask_module_irq(true)
> > > > > }
> > > > >
> > > > > devapc_extract_vio_dbg()
> > > > >
> > > > > for all vio_idx {
> > > > >     clear_vio_status()
> > > > >     mask_module_irq(false)
> > > > > }
> > > > >
> > > >
> > > > I'm also consider this solution and I think it's much better to
> > > > understand hardware behavior.
> > > >
> > > > devapc_dump_vio_dbg()
> > > > {
> > > >         while(1) {
> > > >                 // might have multiple shift_bit raised
> > > >                 shift_bit = get_shift_group()
> > > >                 if (shift_bit >= 0 && shift bit <= 31)
> > > >                         sync_vio_dbg(shift_bit)
> > > >                         extract_vio_dbg()
> > >
> > > According to your statement, when multiple violation occur, only the
> > > first one is kept, others are dropped. I think we just need to dump
> > > debug info once.
> > >
> > > Because only one violation information would be kept, why not only one
> > > group (equal to no group)?
> > >
> > > Regards,
> > > Chun-Kuang.
> >
> > Let's me give you an example of devapc design.
> > vio_idx: 0, 1, 2 -> group 0 (shift_bit: 0)
> > vio_idx: 3, 4, 5 -> group 1 (shift_bit: 1)
> > ...
> >
> > Each group violation will keep one violation (the first one). If vio_idx
> > 0 is triggered first, vio_idx 1 is triggered next, then group 0 will
> > just keep vio_idx 0 violation info.
> > If vio_idx 2 is triggered first, vio_idx 3 is triggered next, then group
> > 0 will keep vio_idx 2 violation info, group 1 will keep vio_idx 3's.
> >
> > We have to scan all groups and dump everything we have.
> > Thanks !
> >
> 
> Could we let all vio_idx be group 0 so that we could just sync one
> group? It's bad to spend too much time in irq handler.
> When we set pd_vio_shift_sel_reg, it seems we could set multiple group
> together, couldn't it?
> 
> Regards,
> Chun-Kuang.
> 

No, Which group vio_idx belongs to is determined by hardware. Software
cannot change its group.
There is very low possibility that multiple groups has violation at the
same time, so it would not spend much time to handle it.
It also cannot shift multiple groups at the same time since there is
only one vio_info(rw, vio_addr, master_id, ...) exist at a time.
devapc_extract_vio_dbg() function is doing this step.

Thanks !

> > >
> > > >                 else
> > > >                         break
> > > >         }
> > > > }
> > > >
> > > > devapc_violation_irq()
> > > > {
> > > >         for all vio_idx {
> > > >                 mask_module_irq(true)
> > > >         }
> > > >
> > > >         devapc_dump_vio_dbg()
> > > >
> > > >         for all vio_idx {
> > > >                 clear_vio_status()
> > > >                 mask_module_irq(false)
> > > >         }
> > > > }
> > > >
> > > > Is it more clear for this control flow?
> > > > Thanks !
> > > >
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > About your question, vio_addr would be the first one.
> > > > > > > > >
> > > > > > > > > So other vio_addr would be dropped? Or hardware would keep all
> > > > > > > > > vio_addr and you have some way to get all vio_addr?
> > > > > > > > >
> > > > > > > >
> > > > > > > > In this case, hardware will drop other violation info and keep the first
> > > > > > > > one until it been handled.
> > > > > > >
> > > > > > > Does 'handled' mean status is cleared?
> > > > > >
> > > > > > "handled" means clear status and dump violation info.
> > > > > >
> > > > > > >
> > > > > > > Regards,
> > > > > > > Chun-Kuang.
> > > > > > >
> > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > > +
> > > > > > > > > > > > +       return true;
> > > > > > > > > > > > +}
> > > > > > > > > > > > +
> > > > > > > > > > > > +/*
> > > > > > > > > > > > + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> > > > > > > > > > > > + *                        violation information including which master violates
> > > > > > > > > > > > + *                        access slave.
> > > > > > > > > > > > + */
> > > > > > > > > > > > +static irqreturn_t devapc_violation_irq(int irq_number,
> > > > > > > > > > > > +                                       struct mtk_devapc_context *ctx)
> > > > > > > > > > > > +{
> > > > > > > > > > > > +       u32 vio_idx;
> > > > > > > > > > > > +
> > > > > > > > > > > > +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
> > > > > > > > > > > > +               if (!mtk_devapc_dump_vio_dbg(ctx, vio_idx))
> > > > > > > > > > > > +                       continue;
> > > > > > > > > > > > +
> > > > > > > > > > > > +               /* Ensure that violation info are written before
> > > > > > > > > > > > +                * further operations
> > > > > > > > > > > > +                */
> > > > > > > > > > > > +               smp_mb();
> > > > > > > > > > > > +
> > > > > > > > > > > > +               /*
> > > > > > > > > > > > +                * Mask slave's irq before clearing vio status.
> > > > > > > > > > > > +                * Must do it to avoid nested interrupt and prevent
> > > > > > > > > > > > +                * unexpected behavior.
> > > > > > > > > > > > +                */
> > > > > > > > > > > > +               mask_module_irq(ctx, vio_idx, true);
> > > > > > > > > > > > +
> > > > > > > > > > > > +               clear_vio_status(ctx, vio_idx);
> > > > > > > > > > > > +
> > > > > > > > > > > > +               mask_module_irq(ctx, vio_idx, false);
> > > > > > > > > > > > +       }
> > > > > > > > > > > > +
> > > > > > > > > > > > +       return IRQ_HANDLED;
> > > > > > > > > > > > +}
> > > > > > > > > > > > +
> > > > > > > > > > > > +/*
> > > > > >
> > > >
> >

Chun-Kuang Hu July 29, 2020, 2:22 a.m. UTC | #13

Neal Liu <[e-mail]> 於 2020年7月29日 週三 上午10:10寫道：
>
> Hi Chun-Kuang,
>
> On Tue, 2020-07-28 at 23:35 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu <[e-mail]> 於 2020年7月28日 週二 上午11:52寫道：
> > >
> > > Hi Chun-Kuang,
> > >
> > > On Mon, 2020-07-27 at 22:47 +0800, Chun-Kuang Hu wrote:
> > > > Hi, Neal:
> > > >
> > > > Neal Liu <[e-mail]> 於 2020年7月27日 週一 上午11:06寫道：
> > > > >
> > > > > Hi Chun-Kuang,
> > > > >
> > > > > On Fri, 2020-07-24 at 23:55 +0800, Chun-Kuang Hu wrote:
> > > > > > Hi, Neal:
> > > > > >
> > > > > > Neal Liu <[e-mail]> 於 2020年7月24日 週五 下午2:55寫道：
> > > > > > >
> > > > > > > Hi Chun-Kuang,
> > > > > > >
> > > > > > > On Fri, 2020-07-24 at 00:32 +0800, Chun-Kuang Hu wrote:
> > > > > > > > Hi, Neal:
> > > > > > > >
> > > > > > > > Neal Liu <[e-mail]> 於 2020年7月23日 週四 下午2:11寫道：
> > > > > > > > >
> > > > > > > > > Hi Chun-Kuang,
> > > > > > > > >
> > > > > > > > > On Wed, 2020-07-22 at 22:25 +0800, Chun-Kuang Hu wrote:
> > > > > > > > > > Hi, Neal:
> > > > > > > > > >
> > > > > > > > > > Neal Liu <[e-mail]> 於 2020年7月22日 週三 上午11:49寫道：
> > > > > > > > > > >
> > > > > > > > > > > Hi Chun-Kuang,
> > > > > > > > > > >
> > > > > > > > > > > On Wed, 2020-07-22 at 07:21 +0800, Chun-Kuang Hu wrote:
> > > > > > > > > > > > Hi, Neal:
> > > > > > > > > > > >
> > > > > > > > > > > > Neal Liu <[e-mail]> 於 2020年7月21日 週二 下午12:00寫道：
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +/*
> > > > > > > > > > > > > + * mtk_devapc_dump_vio_dbg - get the violation index and dump the full violation
> > > > > > > > > > > > > + *                           debug information.
> > > > > > > > > > > > > + */
> > > > > > > > > > > > > +static bool mtk_devapc_dump_vio_dbg(struct mtk_devapc_context *ctx, u32 vio_idx)
> > > > > > > > > > > > > +{
> > > > > > > > > > > > > +       u32 shift_bit;
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +       if (check_vio_mask(ctx, vio_idx))
> > > > > > > > > > > > > +               return false;
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +       if (!check_vio_status(ctx, vio_idx))
> > > > > > > > > > > > > +               return false;
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +       shift_bit = get_shift_group(ctx, vio_idx);
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +       if (sync_vio_dbg(ctx, shift_bit))
> > > > > > > > > > > > > +               return false;
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +       devapc_extract_vio_dbg(ctx);
> > > > > > > > > > > >
> > > > > > > > > > > > I think get_shift_group(), sync_vio_dbg(), and
> > > > > > > > > > > > devapc_extract_vio_dbg() should be moved out of vio_idx for-loop (the
> > > > > > > > > > > > loop in devapc_violation_irq()) because these three function is not
> > > > > > > > > > > > related to vio_idx.
> > > > > > > > > > > > Another question: when multiple vio_idx violation occur, vio_addr is
> > > > > > > > > > > > related to which one vio_idx? The latest happened one?
> > > > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > Actually, it's related to vio_idx. But we don't use it directly on these
> > > > > > > > > > > function. I think below snip code might be better way to understand it.
> > > > > > > > > > >
> > > > > > > > > > > for (...)
> > > > > > > > > > > {
> > > > > > > > > > >         check_vio_mask()
> > > > > > > > > > >         check_vio_status()
> > > > > > > > > > >
> > > > > > > > > > >         // if get vio_idx, mask it temporarily
> > > > > > > > > > >         mask_module_irq(true)
> > > > > > > > > > >         clear_vio_status()
> > > > > > > > > > >
> > > > > > > > > > >         // dump violation info
> > > > > > > > > > >         get_shift_group()
> > > > > > > > > > >         sync_vio_dbg()
> > > > > > > > > > >         devapc_extract_vio_dbg()
> > > > > > > > > > >
> > > > > > > > > > >         // unmask
> > > > > > > > > > >         mask_module_irq(false)
> > > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > This snip code does not explain any thing. I could rewrite this code as:
> > > > > > > > > >
> > > > > > > > > > for (...)
> > > > > > > > > > {
> > > > > > > > > >     check_vio_mask()
> > > > > > > > > >     check_vio_status()
> > > > > > > > > >
> > > > > > > > > >     // if get vio_idx, mask it temporarily
> > > > > > > > > >     mask_module_irq(true)
> > > > > > > > > >     clear_vio_status()
> > > > > > > > > >     // unmask
> > > > > > > > > >     mask_module_irq(false)
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > // dump violation info
> > > > > > > > > > get_shift_group()
> > > > > > > > > > sync_vio_dbg()
> > > > > > > > > > devapc_extract_vio_dbg()
> > > > > > > > > >
> > > > > > > > > > And my version is identical with your version, isn't it?
> > > > > > > > >
> > > > > > > > > Sorry, I did not explain it clearly. Let's me try again.
> > > > > > > > > The reason why I put "dump violation info" between mask & unmask context
> > > > > > > > > is because it has to stop interrupt first before dump violation info,
> > > > > > > > > and then unmask it to prepare next violation.
> > > > > > > > > These sequence guarantee that if multiple violation is triggered, we
> > > > > > > > > still have information to debug.
> > > > > > > > > If the code sequence in your version and multiple violation is
> > > > > > > > > triggered, there might be no any information but keeps entering ISR.
> > > > > > > > > Finally, system might be abnormal and watchdog timeout.
> > > > > > > > > In this case, we still don't have any information to debug.
> > > > > > > >
> > > > > > > > I still don't understand why no information to debug. For example when
> > > > > > > > vio_idx 5, 10, 15 has violation,
> > > > > > > > You would mask vio_idx 5 to get information, but vio_idx 10, 15 does
> > > > > > > > not mask yet.
> > > > > > > > In your words, when vio_idx 10, 15 not mask, you would not get any
> > > > > > > > debug information when you process vio_idx 5.
> > > > > > > >
> > > > > > > > In my version, I would clear all status, why keeps entering ISR?
> > > > > > >
> > > > > > > Think about this case, if someone tries to dump "AAA" module's register.
> > > > > > > It would keep read reg base, base+0x4, base+0x8, ...
> > > > > > > All these registers are in the same slave, which would be same vio_idx.
> > > > > > > (Take vio_idx 5 as example)
> > > > > > > In this case, vio_idx 5 will keep triggering interrupt. If you did not
> > > > > > > do "dump violation info" between mask & unmask, you cannot get any
> > > > > > > violation info until the last interrupt being handled.
> > > > > > > Normally, system will crash before last interrupt coming.
> > > > > >
> > > > > > You have said that first vio_addr would be kept until it's 'handled'.
> > > > > > So the first vio_addr reg_base would be kept even though other
> > > > > > violation happen. And I could handle (clear status and dump info) it
> > > > > > then vio_addr would next violation's address. I'm confused with your
> > > > > > statement. If AAA is dumping register of vio_idx 5, BBB is dumping
> > > > > > register of vio_idx 10, CCC is dumping register of vio_idx 15, I think
> > > > > > you should mask all vio_idx not only one. So the code would be
> > > > > >
> > > > > > for all vio_idx {
> > > > > >     mask_module_irq(true)
> > > > > > }
> > > > > >
> > > > > > devapc_extract_vio_dbg()
> > > > > >
> > > > > > for all vio_idx {
> > > > > >     clear_vio_status()
> > > > > >     mask_module_irq(false)
> > > > > > }
> > > > > >
> > > > >
> > > > > I'm also consider this solution and I think it's much better to
> > > > > understand hardware behavior.
> > > > >
> > > > > devapc_dump_vio_dbg()
> > > > > {
> > > > >         while(1) {
> > > > >                 // might have multiple shift_bit raised
> > > > >                 shift_bit = get_shift_group()
> > > > >                 if (shift_bit >= 0 && shift bit <= 31)
> > > > >                         sync_vio_dbg(shift_bit)
> > > > >                         extract_vio_dbg()
> > > >
> > > > According to your statement, when multiple violation occur, only the
> > > > first one is kept, others are dropped. I think we just need to dump
> > > > debug info once.
> > > >
> > > > Because only one violation information would be kept, why not only one
> > > > group (equal to no group)?
> > > >
> > > > Regards,
> > > > Chun-Kuang.
> > >
> > > Let's me give you an example of devapc design.
> > > vio_idx: 0, 1, 2 -> group 0 (shift_bit: 0)
> > > vio_idx: 3, 4, 5 -> group 1 (shift_bit: 1)
> > > ...
> > >
> > > Each group violation will keep one violation (the first one). If vio_idx
> > > 0 is triggered first, vio_idx 1 is triggered next, then group 0 will
> > > just keep vio_idx 0 violation info.
> > > If vio_idx 2 is triggered first, vio_idx 3 is triggered next, then group
> > > 0 will keep vio_idx 2 violation info, group 1 will keep vio_idx 3's.
> > >
> > > We have to scan all groups and dump everything we have.
> > > Thanks !
> > >
> >
> > Could we let all vio_idx be group 0 so that we could just sync one
> > group? It's bad to spend too much time in irq handler.
> > When we set pd_vio_shift_sel_reg, it seems we could set multiple group
> > together, couldn't it?
> >
> > Regards,
> > Chun-Kuang.
> >
>
> No, Which group vio_idx belongs to is determined by hardware. Software
> cannot change its group.
> There is very low possibility that multiple groups has violation at the
> same time, so it would not spend much time to handle it.
> It also cannot shift multiple groups at the same time since there is
> only one vio_info(rw, vio_addr, master_id, ...) exist at a time.
> devapc_extract_vio_dbg() function is doing this step.
>

So this flow is OK for me. Would you please add comment for this
information so that we could understand how hardware work.

Regards,
Chun-Kuang.

> Thanks !
>
> > > >
> > > > >                 else
> > > > >                         break
> > > > >         }
> > > > > }
> > > > >
> > > > > devapc_violation_irq()
> > > > > {
> > > > >         for all vio_idx {
> > > > >                 mask_module_irq(true)
> > > > >         }
> > > > >
> > > > >         devapc_dump_vio_dbg()
> > > > >
> > > > >         for all vio_idx {
> > > > >                 clear_vio_status()
> > > > >                 mask_module_irq(false)
> > > > >         }
> > > > > }
> > > > >
> > > > > Is it more clear for this control flow?
> > > > > Thanks !
> > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > About your question, vio_addr would be the first one.
> > > > > > > > > >
> > > > > > > > > > So other vio_addr would be dropped? Or hardware would keep all
> > > > > > > > > > vio_addr and you have some way to get all vio_addr?
> > > > > > > > > >
> > > > > > > > >
> > > > > > > > > In this case, hardware will drop other violation info and keep the first
> > > > > > > > > one until it been handled.
> > > > > > > >
> > > > > > > > Does 'handled' mean status is cleared?
> > > > > > >
> > > > > > > "handled" means clear status and dump violation info.
> > > > > > >
> > > > > > > >
> > > > > > > > Regards,
> > > > > > > > Chun-Kuang.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +       return true;
> > > > > > > > > > > > > +}
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +/*
> > > > > > > > > > > > > + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> > > > > > > > > > > > > + *                        violation information including which master violates
> > > > > > > > > > > > > + *                        access slave.
> > > > > > > > > > > > > + */
> > > > > > > > > > > > > +static irqreturn_t devapc_violation_irq(int irq_number,
> > > > > > > > > > > > > +                                       struct mtk_devapc_context *ctx)
> > > > > > > > > > > > > +{
> > > > > > > > > > > > > +       u32 vio_idx;
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
> > > > > > > > > > > > > +               if (!mtk_devapc_dump_vio_dbg(ctx, vio_idx))
> > > > > > > > > > > > > +                       continue;
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +               /* Ensure that violation info are written before
> > > > > > > > > > > > > +                * further operations
> > > > > > > > > > > > > +                */
> > > > > > > > > > > > > +               smp_mb();
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +               /*
> > > > > > > > > > > > > +                * Mask slave's irq before clearing vio status.
> > > > > > > > > > > > > +                * Must do it to avoid nested interrupt and prevent
> > > > > > > > > > > > > +                * unexpected behavior.
> > > > > > > > > > > > > +                */
> > > > > > > > > > > > > +               mask_module_irq(ctx, vio_idx, true);
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +               clear_vio_status(ctx, vio_idx);
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +               mask_module_irq(ctx, vio_idx, false);
> > > > > > > > > > > > > +       }
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +       return IRQ_HANDLED;
> > > > > > > > > > > > > +}
> > > > > > > > > > > > > +
> > > > > > > > > > > > > +/*
> > > > > > >
> > > > >
> > >
>

Neal Liu July 29, 2020, 3:15 a.m. UTC | #14

On Wed, 2020-07-29 at 10:22 +0800, Chun-Kuang Hu wrote:
> Neal Liu <[e-mail]> 於 2020年7月29日 週三 上午10:10寫道：
> >
> > Hi Chun-Kuang,
> >
> > On Tue, 2020-07-28 at 23:35 +0800, Chun-Kuang Hu wrote:
> > > Hi, Neal:
> > >
> > > Neal Liu <[e-mail]> 於 2020年7月28日 週二 上午11:52寫道：
> > > >
> > > > Hi Chun-Kuang,
> > > >
> > > > On Mon, 2020-07-27 at 22:47 +0800, Chun-Kuang Hu wrote:
> > > > > Hi, Neal:
> > > > >
> > > > > Neal Liu <[e-mail]> 於 2020年7月27日 週一 上午11:06寫道：
> > > > > >
> > > > > > Hi Chun-Kuang,
> > > > > >
> > > > > > On Fri, 2020-07-24 at 23:55 +0800, Chun-Kuang Hu wrote:
> > > > > > > Hi, Neal:
> > > > > > >
> > > > > > > Neal Liu <[e-mail]> 於 2020年7月24日 週五 下午2:55寫道：
> > > > > > > >
> > > > > > > > Hi Chun-Kuang,
> > > > > > > >
> > > > > > > > On Fri, 2020-07-24 at 00:32 +0800, Chun-Kuang Hu wrote:
> > > > > > > > > Hi, Neal:
> > > > > > > > >
> > > > > > > > > Neal Liu <[e-mail]> 於 2020年7月23日 週四 下午2:11寫道：
> > > > > > > > > >
> > > > > > > > > > Hi Chun-Kuang,
> > > > > > > > > >
> > > > > > > > > > On Wed, 2020-07-22 at 22:25 +0800, Chun-Kuang Hu wrote:
> > > > > > > > > > > Hi, Neal:
> > > > > > > > > > >
> > > > > > > > > > > Neal Liu <[e-mail]> 於 2020年7月22日 週三 上午11:49寫道：
> > > > > > > > > > > >
> > > > > > > > > > > > Hi Chun-Kuang,
> > > > > > > > > > > >
> > > > > > > > > > > > On Wed, 2020-07-22 at 07:21 +0800, Chun-Kuang Hu wrote:
> > > > > > > > > > > > > Hi, Neal:
> > > > > > > > > > > > >
> > > > > > > > > > > > > Neal Liu <[e-mail]> 於 2020年7月21日 週二 下午12:00寫道：
> > > > > > > > > > > > > >
> > > > > > > > > > > > >
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +/*
> > > > > > > > > > > > > > + * mtk_devapc_dump_vio_dbg - get the violation index and dump the full violation
> > > > > > > > > > > > > > + *                           debug information.
> > > > > > > > > > > > > > + */
> > > > > > > > > > > > > > +static bool mtk_devapc_dump_vio_dbg(struct mtk_devapc_context *ctx, u32 vio_idx)
> > > > > > > > > > > > > > +{
> > > > > > > > > > > > > > +       u32 shift_bit;
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +       if (check_vio_mask(ctx, vio_idx))
> > > > > > > > > > > > > > +               return false;
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +       if (!check_vio_status(ctx, vio_idx))
> > > > > > > > > > > > > > +               return false;
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +       shift_bit = get_shift_group(ctx, vio_idx);
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +       if (sync_vio_dbg(ctx, shift_bit))
> > > > > > > > > > > > > > +               return false;
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +       devapc_extract_vio_dbg(ctx);
> > > > > > > > > > > > >
> > > > > > > > > > > > > I think get_shift_group(), sync_vio_dbg(), and
> > > > > > > > > > > > > devapc_extract_vio_dbg() should be moved out of vio_idx for-loop (the
> > > > > > > > > > > > > loop in devapc_violation_irq()) because these three function is not
> > > > > > > > > > > > > related to vio_idx.
> > > > > > > > > > > > > Another question: when multiple vio_idx violation occur, vio_addr is
> > > > > > > > > > > > > related to which one vio_idx? The latest happened one?
> > > > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > Actually, it's related to vio_idx. But we don't use it directly on these
> > > > > > > > > > > > function. I think below snip code might be better way to understand it.
> > > > > > > > > > > >
> > > > > > > > > > > > for (...)
> > > > > > > > > > > > {
> > > > > > > > > > > >         check_vio_mask()
> > > > > > > > > > > >         check_vio_status()
> > > > > > > > > > > >
> > > > > > > > > > > >         // if get vio_idx, mask it temporarily
> > > > > > > > > > > >         mask_module_irq(true)
> > > > > > > > > > > >         clear_vio_status()
> > > > > > > > > > > >
> > > > > > > > > > > >         // dump violation info
> > > > > > > > > > > >         get_shift_group()
> > > > > > > > > > > >         sync_vio_dbg()
> > > > > > > > > > > >         devapc_extract_vio_dbg()
> > > > > > > > > > > >
> > > > > > > > > > > >         // unmask
> > > > > > > > > > > >         mask_module_irq(false)
> > > > > > > > > > > > }
> > > > > > > > > > >
> > > > > > > > > > > This snip code does not explain any thing. I could rewrite this code as:
> > > > > > > > > > >
> > > > > > > > > > > for (...)
> > > > > > > > > > > {
> > > > > > > > > > >     check_vio_mask()
> > > > > > > > > > >     check_vio_status()
> > > > > > > > > > >
> > > > > > > > > > >     // if get vio_idx, mask it temporarily
> > > > > > > > > > >     mask_module_irq(true)
> > > > > > > > > > >     clear_vio_status()
> > > > > > > > > > >     // unmask
> > > > > > > > > > >     mask_module_irq(false)
> > > > > > > > > > > }
> > > > > > > > > > >
> > > > > > > > > > > // dump violation info
> > > > > > > > > > > get_shift_group()
> > > > > > > > > > > sync_vio_dbg()
> > > > > > > > > > > devapc_extract_vio_dbg()
> > > > > > > > > > >
> > > > > > > > > > > And my version is identical with your version, isn't it?
> > > > > > > > > >
> > > > > > > > > > Sorry, I did not explain it clearly. Let's me try again.
> > > > > > > > > > The reason why I put "dump violation info" between mask & unmask context
> > > > > > > > > > is because it has to stop interrupt first before dump violation info,
> > > > > > > > > > and then unmask it to prepare next violation.
> > > > > > > > > > These sequence guarantee that if multiple violation is triggered, we
> > > > > > > > > > still have information to debug.
> > > > > > > > > > If the code sequence in your version and multiple violation is
> > > > > > > > > > triggered, there might be no any information but keeps entering ISR.
> > > > > > > > > > Finally, system might be abnormal and watchdog timeout.
> > > > > > > > > > In this case, we still don't have any information to debug.
> > > > > > > > >
> > > > > > > > > I still don't understand why no information to debug. For example when
> > > > > > > > > vio_idx 5, 10, 15 has violation,
> > > > > > > > > You would mask vio_idx 5 to get information, but vio_idx 10, 15 does
> > > > > > > > > not mask yet.
> > > > > > > > > In your words, when vio_idx 10, 15 not mask, you would not get any
> > > > > > > > > debug information when you process vio_idx 5.
> > > > > > > > >
> > > > > > > > > In my version, I would clear all status, why keeps entering ISR?
> > > > > > > >
> > > > > > > > Think about this case, if someone tries to dump "AAA" module's register.
> > > > > > > > It would keep read reg base, base+0x4, base+0x8, ...
> > > > > > > > All these registers are in the same slave, which would be same vio_idx.
> > > > > > > > (Take vio_idx 5 as example)
> > > > > > > > In this case, vio_idx 5 will keep triggering interrupt. If you did not
> > > > > > > > do "dump violation info" between mask & unmask, you cannot get any
> > > > > > > > violation info until the last interrupt being handled.
> > > > > > > > Normally, system will crash before last interrupt coming.
> > > > > > >
> > > > > > > You have said that first vio_addr would be kept until it's 'handled'.
> > > > > > > So the first vio_addr reg_base would be kept even though other
> > > > > > > violation happen. And I could handle (clear status and dump info) it
> > > > > > > then vio_addr would next violation's address. I'm confused with your
> > > > > > > statement. If AAA is dumping register of vio_idx 5, BBB is dumping
> > > > > > > register of vio_idx 10, CCC is dumping register of vio_idx 15, I think
> > > > > > > you should mask all vio_idx not only one. So the code would be
> > > > > > >
> > > > > > > for all vio_idx {
> > > > > > >     mask_module_irq(true)
> > > > > > > }
> > > > > > >
> > > > > > > devapc_extract_vio_dbg()
> > > > > > >
> > > > > > > for all vio_idx {
> > > > > > >     clear_vio_status()
> > > > > > >     mask_module_irq(false)
> > > > > > > }
> > > > > > >
> > > > > >
> > > > > > I'm also consider this solution and I think it's much better to
> > > > > > understand hardware behavior.
> > > > > >
> > > > > > devapc_dump_vio_dbg()
> > > > > > {
> > > > > >         while(1) {
> > > > > >                 // might have multiple shift_bit raised
> > > > > >                 shift_bit = get_shift_group()
> > > > > >                 if (shift_bit >= 0 && shift bit <= 31)
> > > > > >                         sync_vio_dbg(shift_bit)
> > > > > >                         extract_vio_dbg()
> > > > >
> > > > > According to your statement, when multiple violation occur, only the
> > > > > first one is kept, others are dropped. I think we just need to dump
> > > > > debug info once.
> > > > >
> > > > > Because only one violation information would be kept, why not only one
> > > > > group (equal to no group)?
> > > > >
> > > > > Regards,
> > > > > Chun-Kuang.
> > > >
> > > > Let's me give you an example of devapc design.
> > > > vio_idx: 0, 1, 2 -> group 0 (shift_bit: 0)
> > > > vio_idx: 3, 4, 5 -> group 1 (shift_bit: 1)
> > > > ...
> > > >
> > > > Each group violation will keep one violation (the first one). If vio_idx
> > > > 0 is triggered first, vio_idx 1 is triggered next, then group 0 will
> > > > just keep vio_idx 0 violation info.
> > > > If vio_idx 2 is triggered first, vio_idx 3 is triggered next, then group
> > > > 0 will keep vio_idx 2 violation info, group 1 will keep vio_idx 3's.
> > > >
> > > > We have to scan all groups and dump everything we have.
> > > > Thanks !
> > > >
> > >
> > > Could we let all vio_idx be group 0 so that we could just sync one
> > > group? It's bad to spend too much time in irq handler.
> > > When we set pd_vio_shift_sel_reg, it seems we could set multiple group
> > > together, couldn't it?
> > >
> > > Regards,
> > > Chun-Kuang.
> > >
> >
> > No, Which group vio_idx belongs to is determined by hardware. Software
> > cannot change its group.
> > There is very low possibility that multiple groups has violation at the
> > same time, so it would not spend much time to handle it.
> > It also cannot shift multiple groups at the same time since there is
> > only one vio_info(rw, vio_addr, master_id, ...) exist at a time.
> > devapc_extract_vio_dbg() function is doing this step.
> >
> 
> So this flow is OK for me. Would you please add comment for this
> information so that we could understand how hardware work.
> 
> Regards,
> Chun-Kuang.
> 

Okay, I'll add it in next patch. Thanks !

> > Thanks !
> >
> > > > >
> > > > > >                 else
> > > > > >                         break
> > > > > >         }
> > > > > > }
> > > > > >
> > > > > > devapc_violation_irq()
> > > > > > {
> > > > > >         for all vio_idx {
> > > > > >                 mask_module_irq(true)
> > > > > >         }
> > > > > >
> > > > > >         devapc_dump_vio_dbg()
> > > > > >
> > > > > >         for all vio_idx {
> > > > > >                 clear_vio_status()
> > > > > >                 mask_module_irq(false)
> > > > > >         }
> > > > > > }
> > > > > >
> > > > > > Is it more clear for this control flow?
> > > > > > Thanks !
> > > > > >
> > > > > > > >
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > About your question, vio_addr would be the first one.
> > > > > > > > > > >
> > > > > > > > > > > So other vio_addr would be dropped? Or hardware would keep all
> > > > > > > > > > > vio_addr and you have some way to get all vio_addr?
> > > > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > In this case, hardware will drop other violation info and keep the first
> > > > > > > > > > one until it been handled.
> > > > > > > > >
> > > > > > > > > Does 'handled' mean status is cleared?
> > > > > > > >
> > > > > > > > "handled" means clear status and dump violation info.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Regards,
> > > > > > > > > Chun-Kuang.
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > > >
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +       return true;
> > > > > > > > > > > > > > +}
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +/*
> > > > > > > > > > > > > > + * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
> > > > > > > > > > > > > > + *                        violation information including which master violates
> > > > > > > > > > > > > > + *                        access slave.
> > > > > > > > > > > > > > + */
> > > > > > > > > > > > > > +static irqreturn_t devapc_violation_irq(int irq_number,
> > > > > > > > > > > > > > +                                       struct mtk_devapc_context *ctx)
> > > > > > > > > > > > > > +{
> > > > > > > > > > > > > > +       u32 vio_idx;
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
> > > > > > > > > > > > > > +               if (!mtk_devapc_dump_vio_dbg(ctx, vio_idx))
> > > > > > > > > > > > > > +                       continue;
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +               /* Ensure that violation info are written before
> > > > > > > > > > > > > > +                * further operations
> > > > > > > > > > > > > > +                */
> > > > > > > > > > > > > > +               smp_mb();
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +               /*
> > > > > > > > > > > > > > +                * Mask slave's irq before clearing vio status.
> > > > > > > > > > > > > > +                * Must do it to avoid nested interrupt and prevent
> > > > > > > > > > > > > > +                * unexpected behavior.
> > > > > > > > > > > > > > +                */
> > > > > > > > > > > > > > +               mask_module_irq(ctx, vio_idx, true);
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +               clear_vio_status(ctx, vio_idx);
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +               mask_module_irq(ctx, vio_idx, false);
> > > > > > > > > > > > > > +       }
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +       return IRQ_HANDLED;
> > > > > > > > > > > > > > +}
> > > > > > > > > > > > > > +
> > > > > > > > > > > > > > +/*
> > > > > > > >
> > > > > >
> > > >
> >

Patch

diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
index 59a56cd..1177c98 100644
--- a/drivers/soc/mediatek/Kconfig
+++ b/drivers/soc/mediatek/Kconfig
@@ -17,6 +17,15 @@  config MTK_CMDQ
 	  time limitation, such as updating display configuration during the
 	  vblank.
 
+config MTK_DEVAPC
+	tristate "Mediatek Device APC Support"
+	help
+	  Say yes here to enable support for Mediatek Device APC driver.
+	  This driver is mainly used to handle the violation which catches
+	  unexpected transaction.
+	  The violation information is logged for further analysis or
+	  countermeasures.
+
 config MTK_INFRACFG
 	bool "MediaTek INFRACFG Support"
 	select REGMAP
diff --git a/drivers/soc/mediatek/Makefile b/drivers/soc/mediatek/Makefile
index 01f9f87..abfd4ba 100644
--- a/drivers/soc/mediatek/Makefile
+++ b/drivers/soc/mediatek/Makefile
@@ -1,5 +1,6 @@ 
 # SPDX-License-Identifier: GPL-2.0-only
 obj-$(CONFIG_MTK_CMDQ) += mtk-cmdq-helper.o
+obj-$(CONFIG_MTK_DEVAPC) += mtk-devapc.o
 obj-$(CONFIG_MTK_INFRACFG) += mtk-infracfg.o
 obj-$(CONFIG_MTK_PMIC_WRAP) += mtk-pmic-wrap.o
 obj-$(CONFIG_MTK_SCPSYS) += mtk-scpsys.o
diff --git a/drivers/soc/mediatek/mtk-devapc.c b/drivers/soc/mediatek/mtk-devapc.c
new file mode 100644
index 0000000..1397e98
--- /dev/null
+++ b/drivers/soc/mediatek/mtk-devapc.c
@@ -0,0 +1,372 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2020 MediaTek Inc.
+ */
+
+#include <linux/clk.h>
+#include <linux/interrupt.h>
+#include <linux/iopoll.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/of_device.h>
+#include <linux/of_irq.h>
+#include <linux/of_address.h>
+#include "mtk-devapc.h"
+
+static u32 get_shift_group(struct mtk_devapc_context *ctx, u32 vio_idx)
+{
+	u32 vio_shift_sta;
+	void __iomem *reg;
+
+	reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
+	vio_shift_sta = readl(reg);
+
+	if (vio_shift_sta)
+		return __ffs(vio_shift_sta);
+
+	return 31;
+}
+
+static int check_vio_mask_sta(struct mtk_devapc_context *ctx, u32 module,
+			      u32 offset)
+{
+	void __iomem *reg;
+	u32 value;
+
+	reg = ctx->devapc_pd_base + offset;
+	reg += 0x4 * VIO_MOD_TO_REG_IND(module);
+
+	value = readl(reg);
+
+	return ((value >> VIO_MOD_TO_REG_OFF(module)) & 0x1);
+}
+
+static int check_vio_mask(struct mtk_devapc_context *ctx, u32 module)
+{
+	return check_vio_mask_sta(ctx, module, ctx->offset->vio_mask);
+}
+
+static int check_vio_status(struct mtk_devapc_context *ctx, u32 module)
+{
+	return check_vio_mask_sta(ctx, module, ctx->offset->vio_sta);
+}
+
+static void clear_vio_status(struct mtk_devapc_context *ctx, u32 module)
+{
+	void __iomem *reg;
+
+	reg = ctx->devapc_pd_base + ctx->offset->vio_sta;
+	reg += 0x4 * VIO_MOD_TO_REG_IND(module);
+
+	writel(0x1 << VIO_MOD_TO_REG_OFF(module), reg);
+
+	if (check_vio_status(ctx, module))
+		dev_err(ctx->dev, "%s: Clear failed, module_index:0x%x\n",
+			__func__, module);
+}
+
+static void mask_module_irq(struct mtk_devapc_context *ctx, u32 module,
+			    bool mask)
+{
+	void __iomem *reg;
+	u32 value;
+
+	reg = ctx->devapc_pd_base + ctx->offset->vio_mask;
+	reg += 0x4 * VIO_MOD_TO_REG_IND(module);
+
+	value = readl(reg);
+	if (mask)
+		value |= (0x1 << VIO_MOD_TO_REG_OFF(module));
+	else
+		value &= ~(0x1 << VIO_MOD_TO_REG_OFF(module));
+
+	writel(value, reg);
+}
+
+#define PHY_DEVAPC_TIMEOUT	0x10000
+
+/*
+ * sync_vio_dbg - do "shift" mechansim" to get full violation information.
+ *                shift mechanism is depends on devapc hardware design.
+ *                Mediatek devapc set multiple slaves as a group. When violation
+ *                is triggered, violation info is kept inside devapc hardware.
+ *                Driver should do shift mechansim to "shift" full violation
+ *                info to VIO_DBGs registers.
+ *
+ */
+static int sync_vio_dbg(struct mtk_devapc_context *ctx, u32 shift_bit)
+{
+	void __iomem *pd_vio_shift_sta_reg;
+	void __iomem *pd_vio_shift_sel_reg;
+	void __iomem *pd_vio_shift_con_reg;
+	int ret;
+	u32 val;
+
+	pd_vio_shift_sta_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
+	pd_vio_shift_sel_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sel;
+	pd_vio_shift_con_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_con;
+
+	/* Enable shift mechansim */
+	writel(0x1 << shift_bit, pd_vio_shift_sel_reg);
+	writel(0x1, pd_vio_shift_con_reg);
+
+	ret = readl_poll_timeout(pd_vio_shift_con_reg, val, val & 0x3, 1000,
+				 PHY_DEVAPC_TIMEOUT);
+	if (ret)
+		dev_err(ctx->dev, "%s: Shift violation info failed\n",
+			__func__);
+
+	/* Disable shift mechanism */
+	writel(0x0, pd_vio_shift_con_reg);
+	writel(0x0, pd_vio_shift_sel_reg);
+	writel(0x1 << shift_bit, pd_vio_shift_sta_reg);
+
+	return ret;
+}
+
+static void devapc_vio_info_print(struct mtk_devapc_context *ctx)
+{
+	struct mtk_devapc_vio_info *vio_info = ctx->vio_info;
+
+	/* Print violation information */
+	if (vio_info->write)
+		dev_info(ctx->dev, "Write Violation\n");
+	else if (vio_info->read)
+		dev_info(ctx->dev, "Read Violation\n");
+
+	dev_info(ctx->dev, "Vio Addr:0x%x, High:0x%x, Bus ID:0x%x, Dom ID:%x\n",
+		 vio_info->vio_addr, vio_info->vio_addr_high,
+		 vio_info->master_id, vio_info->domain_id);
+}
+
+/*
+ * devapc_extract_vio_dbg - extract full violation information after doing
+ *                          shift mechanism.
+ */
+static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
+{
+	const struct mtk_devapc_vio_dbgs *vio_dbgs;
+	struct mtk_devapc_vio_info *vio_info;
+	void __iomem *vio_dbg0_reg;
+	void __iomem *vio_dbg1_reg;
+	u32 dbg0;
+
+	vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
+	vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
+
+	vio_dbgs = ctx->vio_dbgs;
+	vio_info = ctx->vio_info;
+
+	/* Starts to extract violation information */
+	dbg0 = readl(vio_dbg0_reg);
+	vio_info->vio_addr = readl(vio_dbg1_reg);
+
+	vio_info->master_id = (dbg0 & vio_dbgs->mstid.mask) >>
+			      vio_dbgs->mstid.start;
+	vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
+			      vio_dbgs->dmnid.start;
+	vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
+			    vio_dbgs->vio_w.start) == 1;
+	vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
+			  vio_dbgs->vio_r.start) == 1;
+	vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
+				  vio_dbgs->addr_h.start;
+
+	devapc_vio_info_print(ctx);
+}
+
+/*
+ * mtk_devapc_dump_vio_dbg - get the violation index and dump the full violation
+ *                           debug information.
+ */
+static bool mtk_devapc_dump_vio_dbg(struct mtk_devapc_context *ctx, u32 vio_idx)
+{
+	u32 shift_bit;
+
+	if (check_vio_mask(ctx, vio_idx))
+		return false;
+
+	if (!check_vio_status(ctx, vio_idx))
+		return false;
+
+	shift_bit = get_shift_group(ctx, vio_idx);
+
+	if (sync_vio_dbg(ctx, shift_bit))
+		return false;
+
+	devapc_extract_vio_dbg(ctx);
+
+	return true;
+}
+
+/*
+ * devapc_violation_irq - the devapc Interrupt Service Routine (ISR) will dump
+ *                        violation information including which master violates
+ *                        access slave.
+ */
+static irqreturn_t devapc_violation_irq(int irq_number,
+					struct mtk_devapc_context *ctx)
+{
+	u32 vio_idx;
+
+	for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
+		if (!mtk_devapc_dump_vio_dbg(ctx, vio_idx))
+			continue;
+
+		/* Ensure that violation info are written before
+		 * further operations
+		 */
+		smp_mb();
+
+		/*
+		 * Mask slave's irq before clearing vio status.
+		 * Must do it to avoid nested interrupt and prevent
+		 * unexpected behavior.
+		 */
+		mask_module_irq(ctx, vio_idx, true);
+
+		clear_vio_status(ctx, vio_idx);
+
+		mask_module_irq(ctx, vio_idx, false);
+	}
+
+	return IRQ_HANDLED;
+}
+
+/*
+ * start_devapc - initialize devapc status and start receiving interrupt
+ *                while devapc violation is triggered.
+ */
+static int start_devapc(struct mtk_devapc_context *ctx)
+{
+	void __iomem *pd_vio_shift_sta_reg;
+	void __iomem *pd_apc_con_reg;
+	u32 vio_shift_sta;
+	u32 vio_idx;
+
+	pd_apc_con_reg = ctx->devapc_pd_base + ctx->offset->apc_con;
+	pd_vio_shift_sta_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
+	if (!pd_apc_con_reg || !pd_vio_shift_sta_reg)
+		return -EINVAL;
+
+	/* Clear devapc violation status */
+	writel(BIT(31), pd_apc_con_reg);
+
+	/* Clear violation shift status */
+	vio_shift_sta = readl(pd_vio_shift_sta_reg);
+	if (vio_shift_sta)
+		writel(vio_shift_sta, pd_vio_shift_sta_reg);
+
+	/* Clear slave violation status */
+	for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++) {
+		clear_vio_status(ctx, vio_idx);
+		mask_module_irq(ctx, vio_idx, false);
+	}
+
+	return 0;
+}
+
+static const struct mtk_devapc_pd_offset mt6779_pd_offset = {
+	.vio_mask = 0x0,
+	.vio_sta = 0x400,
+	.vio_dbg0 = 0x900,
+	.vio_dbg1 = 0x904,
+	.apc_con = 0xF00,
+	.vio_shift_sta = 0xF10,
+	.vio_shift_sel = 0xF14,
+	.vio_shift_con = 0xF20,
+};
+
+static const struct mtk_devapc_vio_dbgs mt6779_vio_dbgs = {
+	.mstid =  {0x0000FFFF, 0x0},
+	.dmnid =  {0x003F0000, 0x10},
+	.vio_w =  {0x00400000, 0x16},
+	.vio_r =  {0x00800000, 0x17},
+	.addr_h = {0x0F000000, 0x18},
+};
+
+static const struct mtk_devapc_context devapc_mt6779 = {
+	.vio_idx_num = 510,
+	.offset = &mt6779_pd_offset,
+	.vio_dbgs = &mt6779_vio_dbgs,
+};
+
+static const struct of_device_id mtk_devapc_dt_match[] = {
+	{
+		.compatible = "mediatek,mt6779-devapc",
+		.data = &devapc_mt6779,
+	}, {
+	},
+};
+
+static int mtk_devapc_probe(struct platform_device *pdev)
+{
+	struct device_node *node = pdev->dev.of_node;
+	struct mtk_devapc_context *ctx;
+	struct clk *devapc_infra_clk;
+	u32 devapc_irq;
+	int ret;
+
+	if (IS_ERR(node))
+		return -ENODEV;
+
+	ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
+	if (!ctx)
+		return -ENOMEM;
+
+	ctx = (struct mtk_devapc_context *)of_device_get_match_data(&pdev->dev);
+	ctx->dev = &pdev->dev;
+
+	ctx->vio_info = devm_kzalloc(&pdev->dev,
+				     sizeof(struct mtk_devapc_vio_info),
+				     GFP_KERNEL);
+	if (!ctx->vio_info)
+		return -ENOMEM;
+
+	ctx->devapc_pd_base = of_iomap(node, 0);
+	if (!ctx->devapc_pd_base)
+		return -EINVAL;
+
+	devapc_irq = irq_of_parse_and_map(node, 0);
+	if (!devapc_irq)
+		return -EINVAL;
+
+	devapc_infra_clk = devm_clk_get(&pdev->dev, "devapc-infra-clock");
+	if (IS_ERR(devapc_infra_clk))
+		return -EINVAL;
+
+	if (clk_prepare_enable(devapc_infra_clk))
+		return -EINVAL;
+
+	ret = start_devapc(ctx);
+	if (ret)
+		return ret;
+
+	ret = devm_request_irq(&pdev->dev, devapc_irq,
+			       (irq_handler_t)devapc_violation_irq,
+			       IRQF_TRIGGER_NONE, "devapc", ctx);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+static int mtk_devapc_remove(struct platform_device *dev)
+{
+	return 0;
+}
+
+static struct platform_driver mtk_devapc_driver = {
+	.probe = mtk_devapc_probe,
+	.remove = mtk_devapc_remove,
+	.driver = {
+		.name = KBUILD_MODNAME,
+		.of_match_table = mtk_devapc_dt_match,
+	},
+};
+
+module_platform_driver(mtk_devapc_driver);
+
+MODULE_DESCRIPTION("Mediatek Device APC Driver");
+MODULE_AUTHOR("Neal Liu <[e-mail]>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/soc/mediatek/mtk-devapc.h b/drivers/soc/mediatek/mtk-devapc.h
new file mode 100644
index 0000000..7bd7e66
--- /dev/null
+++ b/drivers/soc/mediatek/mtk-devapc.h
@@ -0,0 +1,54 @@ 
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2020 MediaTek Inc.
+ */
+
+#ifndef __MTK_DEVAPC_H__
+#define __MTK_DEVAPC_H__
+
+#define VIO_MOD_TO_REG_IND(m)	((m) / 32)
+#define VIO_MOD_TO_REG_OFF(m)	((m) % 32)
+
+struct mtk_devapc_pd_offset {
+	u32 vio_mask;
+	u32 vio_sta;
+	u32 vio_dbg0;
+	u32 vio_dbg1;
+	u32 apc_con;
+	u32 vio_shift_sta;
+	u32 vio_shift_sel;
+	u32 vio_shift_con;
+};
+
+struct mtk_devapc_vio_dbgs_desc {
+	u32 mask;
+	u32 start;
+};
+
+struct mtk_devapc_vio_dbgs {
+	struct mtk_devapc_vio_dbgs_desc mstid;
+	struct mtk_devapc_vio_dbgs_desc dmnid;
+	struct mtk_devapc_vio_dbgs_desc vio_w;
+	struct mtk_devapc_vio_dbgs_desc vio_r;
+	struct mtk_devapc_vio_dbgs_desc addr_h;
+};
+
+struct mtk_devapc_vio_info {
+	bool read;
+	bool write;
+	u32 vio_addr;
+	u32 vio_addr_high;
+	u32 master_id;
+	u32 domain_id;
+};
+
+struct mtk_devapc_context {
+	struct device *dev;
+	u32 vio_idx_num;
+	void __iomem *devapc_pd_base;
+	struct mtk_devapc_vio_info *vio_info;
+	const struct mtk_devapc_pd_offset *offset;
+	const struct mtk_devapc_vio_dbgs *vio_dbgs;
+};
+
+#endif /* __MTK_DEVAPC_H__ */