Re: [PATCH v5 2/2] soc: mediatek: add mt6779 devapc driver

[Neal Liu]

Hi Chun-Kuang,

On Fri, 2020-08-07 at 23:52 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
> 
> Neal Liu <neal.liu@xxxxxxxxxxxx> 於 2020年8月7日 週五 上午10:34寫道：
> >
> > 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]
> 
> > +
> > +#define PHY_DEVAPC_TIMEOUT     0x10000
> > +
> > +/*
> > + * devapc_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 sync full violation
> > + *                       info to VIO_DBGs registers.
> > + *
> > + */
> > +static int devapc_sync_vio_dbg(struct mtk_devapc_context *ctx)
> > +{
> > +       void __iomem *pd_vio_shift_sta_reg;
> > +       void __iomem *pd_vio_shift_sel_reg;
> > +       void __iomem *pd_vio_shift_con_reg;
> > +       int min_shift_group;
> > +       int ret;
> > +       u32 val;
> > +
> > +       pd_vio_shift_sta_reg = ctx->infra_base +
> > +                              ctx->data->vio_shift_sta_offset;
> > +       pd_vio_shift_sel_reg = ctx->infra_base +
> > +                              ctx->data->vio_shift_sel_offset;
> > +       pd_vio_shift_con_reg = ctx->infra_base +
> > +                              ctx->data->vio_shift_con_offset;
> > +
> > +       /* Find the minimum shift group which has violation */
> > +       val = readl(pd_vio_shift_sta_reg);
> > +       if (!val)
> > +               return false;
> > +
> > +       min_shift_group = __ffs(val);
> > +
> > +       /* Assign the group to sync */
> > +       writel(0x1 << min_shift_group, pd_vio_shift_sel_reg);
> > +
> > +       /* Start syncing */
> > +       writel(0x1, pd_vio_shift_con_reg);
> > +
> > +       ret = readl_poll_timeout(pd_vio_shift_con_reg, val, val == 0x3, 0,
> > +                                PHY_DEVAPC_TIMEOUT);
> > +       if (ret) {
> > +               dev_err(ctx->dev, "%s: Shift violation info failed\n", __func__);
> > +               return false;
> > +       }
> > +
> > +       /* Stop syncing */
> > +       writel(0x0, pd_vio_shift_con_reg);
> > +       writel(0x0, pd_vio_shift_sel_reg);
> 
> This is redundant because you set this register before start syncing.

No, we don't set this reg before start syncing.

> 
> > +       writel(0x1 << min_shift_group, pd_vio_shift_sta_reg);
> 
> You read this register to find minimum shift group, but you write it
> back into this register, so this function would get the same minimum
> shift group in next time, isn't it?

No. The operation means write clear. We won't get the same minimum shift
group after clear this bit.

> 
> > +
> > +       return true;
> > +}
> > +
> > +/*
> > + * devapc_extract_vio_dbg - extract full violation information after doing
> > + *                          shift mechanism.
> > + */
> > +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> > +{
> > +       struct mtk_devapc_vio_dbgs *vio_dbgs;
> 
> struct mtk_devapc_vio_dbgs vio_dbgs;
> 
> Use stack instead of allocating from heap.

Why it cannot use heap if the memory is handled correctly?

> 
> > +       void __iomem *vio_dbg0_reg;
> > +       void __iomem *vio_dbg1_reg;
> > +
> > +       vio_dbgs = devm_kzalloc(ctx->dev, sizeof(struct mtk_devapc_vio_dbgs),
> > +                               GFP_KERNEL);
> > +       if (!vio_dbgs)
> > +               return;
> > +
> > +       vio_dbg0_reg = ctx->infra_base + ctx->data->vio_dbg0_offset;
> > +       vio_dbg1_reg = ctx->infra_base + ctx->data->vio_dbg1_offset;
> > +
> > +       vio_dbgs->vio_dbg0 = readl(vio_dbg0_reg);
> > +       vio_dbgs->vio_dbg1 = readl(vio_dbg1_reg);
> > +
> > +       /* Print violation information */
> > +       if (vio_dbgs->dbg0_bits.vio_w)
> > +               dev_info(ctx->dev, "Write Violation\n");
> > +       else if (vio_dbgs->dbg0_bits.vio_r)
> > +               dev_info(ctx->dev, "Read Violation\n");
> > +
> > +       dev_info(ctx->dev, "Bus ID:0x%x, Dom ID:0x%x, Vio Addr:0x%x\n",
> > +                vio_dbgs->dbg0_bits.mstid, vio_dbgs->dbg0_bits.dmnid,
> > +                vio_dbgs->vio_dbg1);
> > +}
> > +
> 
> [snip]
> 
> > +
> > +/*
> > + * start_devapc - unmask slave's irq to start receiving devapc violation.
> > + */
> > +static void start_devapc(struct mtk_devapc_context *ctx)
> > +{
> > +       void __iomem *pd_apc_con_reg;
> > +
> > +       pd_apc_con_reg = ctx->infra_base + ctx->data->apc_con_offset;
> > +       writel(BIT(31), pd_apc_con_reg);
> 
> pd_apc_con_reg is used once, so
> 
> writel(BIT(31), ctx->infra_base + ctx->data->apc_con_offset);

Okay, I'll merge it on next patch.
Thanks !

> 
> > +
> > +       mask_module_irq(ctx, false);
> > +}
> > +
> 
> [snip]
> 
> > +
> > +static int mtk_devapc_probe(struct platform_device *pdev)
> > +{
> > +       struct device_node *node = pdev->dev.of_node;
> > +       struct mtk_devapc_context *ctx;
> > +       u32 devapc_irq;
> > +       int ret;
> > +
> > +       if (IS_ERR(node))
> > +               return -ENODEV;
> > +
> > +       ctx = devm_kzalloc(&pdev->dev, sizeof(*ctx), GFP_KERNEL);
> > +       if (!ctx)
> > +               return -ENOMEM;
> > +
> > +       ctx->data = of_device_get_match_data(&pdev->dev);
> > +       ctx->dev = &pdev->dev;
> > +
> > +       ctx->infra_base = of_iomap(node, 0);
> > +       if (!ctx->infra_base)
> > +               return -EINVAL;
> > +
> > +       devapc_irq = irq_of_parse_and_map(node, 0);
> > +       if (!devapc_irq)
> > +               return -EINVAL;
> > +
> > +       ctx->infra_clk = devm_clk_get(&pdev->dev, "devapc-infra-clock");
> > +       if (IS_ERR(ctx->infra_clk))
> > +               return -EINVAL;
> > +
> > +       if (clk_prepare_enable(ctx->infra_clk))
> > +               return -EINVAL;
> > +
> > +       ret = devm_request_irq(&pdev->dev, devapc_irq,
> > +                              (irq_handler_t)devapc_violation_irq,
> > +                              IRQF_TRIGGER_NONE, "devapc", ctx);
> > +       if (ret) {
> > +               clk_disable_unprepare(ctx->infra_clk);
> > +               return ret;
> > +       }
> > +
> > +       platform_set_drvdata(pdev, ctx);
> > +
> > +       start_devapc(ctx);
> > +
> > +       return 0;
> > +}
> > +
> > +static int mtk_devapc_remove(struct platform_device *pdev)
> > +{
> > +       struct mtk_devapc_context *ctx = platform_get_drvdata(pdev);
> > +
> 
> stop_devapc(ctx);

We don't have to do any further operations to stop devapc hw.

> 
> Regards,
> Chun-Kuang.
> 
> > +       if (ctx->infra_clk)
> > +               clk_disable_unprepare(ctx->infra_clk);
> > +
> > +       return 0;
> > +}
> > +