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

[Neal Liu <neal.liu@mediatek.com>]

Hi Chun-Kuang,

On Wed, 2020-07-22 at 22:25 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
> 
> Neal Liu <neal.liu@mediatek.com> 於 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 <neal.liu@mediatek.com> 於 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;
> > > > +}
> > > > +
> >