Re: [PATCH v3 2/2] soc: mediatek: add mtk-devapc driver
On Wed, 2020-07-29 at 10:22 +0800, Chun-Kuang Hu wrote:
> Neal Liu 於 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 於 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 於 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 於 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 於 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 於 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 於 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)
> > > > > > > > > > > }
> > > > > >
Re: [PATCH v3 2/2] soc: mediatek: add mtk-devapc driver
Neal Liu 於 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 於 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 於 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 於 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 於 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 於 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 於 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
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 於 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 於 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 於 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 於 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 於 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 於 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
Re: [PATCH v3 2/2] soc: mediatek: add mtk-devapc driver
Hi, Neal:
Neal Liu 於 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 於 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 於 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 於 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 於 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 於 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.
Re: [PATCH v3 2/2] soc: mediatek: add mtk-devapc driver
Hi Chun-Kuang,
On Mon, 2020-07-27 at 22:47 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
>
> Neal Liu 於 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 於 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 於 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 於 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 於 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,
Re: [PATCH v3 2/2] soc: mediatek: add mtk-devapc driver
Hi, Neal:
Neal Liu 於 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 於 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 於 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 於 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 於 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.
Re: [PATCH v3 2/2] soc: mediatek: add mtk-devapc driver
Hi Chun-Kuang,
On Fri, 2020-07-24 at 23:55 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
>
> Neal Liu 於 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 於 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 於 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 於 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
> >
Re: [PATCH v3 2/2] soc: mediatek: add mtk-devapc driver
Hi, Neal:
Neal Liu 於 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 於 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 於 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 於 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
Re: [PATCH v3 2/2] soc: mediatek: add mtk-devapc driver
Hi Chun-Kuang,
On Fri, 2020-07-24 at 00:32 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
>
> Neal Liu 於 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 於 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 於 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;
> > > > > > +}
> > > > > > +
> > > > > > +/*
> > > > > > + *
Re: [PATCH v3 2/2] soc: mediatek: add mtk-devapc driver
Hi, Neal:
Neal Liu 於 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 於 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 於 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();
> > > > > +
> > > > > +
Re: [PATCH v3 2/2] soc: mediatek: add mtk-devapc driver
Hi Chun-Kuang,
On Wed, 2020-07-22 at 22:25 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
>
> Neal Liu 於 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 於 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
Re: [PATCH v3 2/2] soc: mediatek: add mtk-devapc driver
Hi, Neal:
Neal Liu 於 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 於 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
Re: [PATCH v3 2/2] soc: mediatek: add mtk-devapc driver
Hi Chun-Kuang,
On Wed, 2020-07-22 at 07:21 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
>
> Neal Liu 於 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
> > ---
>
> [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 <
Re: [PATCH v3 2/2] soc: mediatek: add mtk-devapc driver
Hi, Neal:
Neal Liu 於 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
> ---
[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;
> +}
> +
[PATCH v3 2/2] soc: mediatek: add mtk-devapc driver
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
---
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
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 000..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
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#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 0x1
+
+/*
+ * 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;
+
