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

[Chun-Kuang Hu]

Hi, Neal:

Neal Liu <neal.liu@xxxxxxxxxxxx> æ 2020å7æ10æ éä äå11:23åéï
>
> Hi Chun-Kuang,
>
> Thanks for your review.
>
> On Thu, 2020-07-09 at 21:01 +0800, Chun-Kuang Hu wrote:
> > Hi, Neal:
> >
> > Neal Liu <neal.liu@xxxxxxxxxxxx> æ 2020å7æ9æ éå äå5:13åéï
> > >
> > > 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 <neal.liu@xxxxxxxxxxxx>
> >
> > [snip]
> >
> > > +
> > > +static u32 get_shift_group(struct mtk_devapc_context *devapc_ctx,
> > > +                          int slave_type, int vio_idx)
> >
> > vio_idx  is useless, so remove it.
> >
>
> yes, my mistake. I'll remove it on next patch.
>
> > > +{
> > > +       u32 vio_shift_sta;
> > > +       void __iomem *reg;
> > > +       int bit;
> > > +
> > > +       reg = mtk_devapc_pd_get(devapc_ctx, slave_type, VIO_SHIFT_STA, 0);
> > > +       vio_shift_sta = readl(reg);
> > > +
> > > +       for (bit = 0; bit < 32; bit++) {
> > > +               if ((vio_shift_sta >> bit) & 0x1)
> > > +                       break;
> > > +       }
> > > +
> > > +       return bit;
> > > +}
> > > +
> >
> > [snip]
> >
> > > +
> > > +/*
> > > + * 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 *devapc_ctx)
> > > +{
> > > +       const struct mtk_device_info **device_info;
> > > +       int slave_type_num;
> > > +       int vio_idx = -1;
> > > +       int slave_type;
> > > +
> > > +       slave_type_num = devapc_ctx->slave_type_num;
> > > +       device_info = devapc_ctx->device_info;
> > > +
> > > +       for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
> >
> > If slave_type_num is 1, I think the code should be simpler.
>
> slave_type_num is depends on DT data, it's not always 1.

Please change commit title to "add mt6779 mtk-devapc driver". This
patch is just for mt6779. If slave_type_num = 1 in mt6779, there is
only one slave and we don't need a slave_type variable. Add
slave_type_num in the patch of adding one SoC which has multiple
slaves.

>
> >
> > > +               if (!mtk_devapc_dump_vio_dbg(devapc_ctx, slave_type, &vio_idx))
> > > +                       continue;
> > > +
> > > +               /* Ensure that violation info are written before
> > > +                * further operations
> > > +                */
> > > +               smp_mb();
> > > +
> > > +               mask_module_irq(devapc_ctx, slave_type, vio_idx, true);
> >
> > Why do you mask irq?
>
> It has to mask slave's irq before clear violation status.
> It's one of hardware design.

If don't do this before clear_vio_status, what would happen? The clear
would fail?

>
> >
> > > +
> > > +               clear_vio_status(devapc_ctx, slave_type, vio_idx);
> > > +
> > > +               mask_module_irq(devapc_ctx, slave_type, vio_idx, false);
> > > +       }
> > > +
> > > +       return IRQ_HANDLED;
> > > +}
> > > +
> > > +/*
> > > + * start_devapc - initialize devapc status and start receiving interrupt
> > > + *               while devapc violation is triggered.
> > > + */
> >
> > [snip]
> >
> > > +
> > > +struct mtk_device_info {
> > > +       int sys_index;
> >
> > Useless, so remove it.
>
> We need to print it as our debug information.
> But I did not apply it on this patch, I'll add it on next patch.

I think vio address is enough to find out the murder, so remove it in
this patch. If it provide another information, add it in another patch
and describe clear about what is this and how to use this information.

>
> >
> > > +       int ctrl_index;
> >
> > Ditto.
> >
> > Regards,
> > Chun-Kuang.
> >
> > > +       int vio_index;
> > > +};
> > > +
>