Hi, Neal: Neal Liu <neal....@mediatek.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....@mediatek.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....@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....@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. > > > > 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; > > > > > > > +} > > > > > > > + > > > > > > > +/* >