Message-ID: <1595988652.9265.8.camel@mtkswgap22>
Date:   Wed, 29 Jul 2020 10:10:52 +0800
From:   Neal Liu <neal.liu@...iatek.com>
To:     Chun-Kuang Hu <chunkuang.hu@...nel.org>
CC:     Neal Liu <neal.liu@...iatek.com>, Rob Herring <robh+dt@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        wsd_upstream <wsd_upstream@...iatek.com>,
        lkml <linux-kernel@...r.kernel.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@...ts.infradead.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v3 2/2] soc: mediatek: add mtk-devapc driver

Hi Chun-Kuang,

On Tue, 2020-07-28 at 23:35 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
> 
> Neal Liu <neal.liu@...iatek.com> 於 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 <neal.liu@...iatek.com> 於 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 <neal.liu@...iatek.com> 於 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 <neal.liu@...iatek.com> 於 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 <neal.liu@...iatek.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@...iatek.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.
> > > > > > >
> > > > > > > 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;
> > > > > > > > > > > > +}
> > > > > > > > > > > > +
> > > > > > > > > > > > +/*
> > > > > >
> > > >
> >

Powered by blists - more mailing lists