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

[Neal Liu <neal.liu@mediatek.com>]

Hi Chun-Kuang,

On Mon, 2020-06-15 at 23:51 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
> 
> Neal Liu <neal.liu@mediatek.com> 於 2020年6月9日 週二 下午6:25寫道：
> >
> > MT6873 bus frabric provides TrustZone security support and data
> > protection to prevent slaves from being accessed by unexpected
> > masters.
> > The security violations are 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 devapc-mt6873 driver. The violation
> > information is printed in order to find the murderer.
> >
> > Signed-off-by: Neal Liu <neal.liu@mediatek.com>
> > ---
> 
> [snip]
> 
> > +       {1, 0, 22, "MMSYS", true},
> > +       {1, 1, 23, "MMSYS_DISP", true},
> > +       {1, 2, 24, "SMI", true},
> > +       {1, 3, 25, "SMI", true},
> > +       {1, 4, 26, "SMI", true},
> > +       {1, 5, 27, "MMSYS_DISP", true},
> > +       {1, 6, 28, "MMSYS_DISP", true},
> > +
> > +       /* 30 */
> > +       {1, 7, 29, "MMSYS_DISP", true},
> > +       {1, 8, 30, "MMSYS_DISP", true},
> > +       {1, 9, 31, "MMSYS_DISP", true},
> > +       {1, 10, 32, "MMSYS_DISP", true},
> > +       {1, 11, 33, "MMSYS_DISP", true},
> > +       {1, 12, 34, "MMSYS_DISP", true},
> > +       {1, 13, 35, "MMSYS_DISP", true},
> > +       {1, 14, 36, "MMSYS_DISP", true},
> > +       {1, 15, 37, "MMSYS_DISP", true},
> > +       {1, 16, 38, "MMSYS_DISP", true},
> > +
> > +       /* 40 */
> > +       {1, 17, 39, "MMSYS_DISP", true},
> > +       {1, 18, 40, "MMSYS_DISP", true},
> > +       {1, 19, 41, "MMSYS_DISP", true},
> > +       {1, 20, 42, "MMSYS_DISP", true},
> > +       {1, 21, 43, "MMSYS_DISP", true},
> > +       {1, 22, 44, "MMSYS_DISP", true},
> 
> I think the device name, such as "MMSYS_DISP" does not help for debug.
> When DevAPC print "MMSYS_DISP" has error, how does us know that to do
> next? WHERE is the code may related to this error, and WHO should us
> to find help? I think we just need vio address. Using mt8173 for
> example, if the vio address is 0x1400d03c, we could find the device in
> mt8173.dtsi [1],
> 
> ovl1: ovl@1400d000 {
>         compatible = "mediatek,mt8173-disp-ovl";
>         reg = <0 0x1400d000 0 0x1000>;
> };
> 
> we could know error occur in ovl1, and we could find the compatible
> string "mediatek,mt8173-disp-ovl" in
> drivers/gpu/drm/mediatek/mtk_drm_drv.c, so we know WHERE is the code
> may related to this error. And we could find maintainer list [2] to
> find out the maintainer of this code:
> 
> DRM DRIVERS FOR MEDIATEK
> M: Chun-Kuang Hu <chunkuang.hu@kernel.org>
> M: Philipp Zabel <p.zabel@pengutronix.de>
> L: dri-devel@lists.freedesktop.org
> S: Supported
> F: Documentation/devicetree/bindings/display/mediatek/
> F: drivers/gpu/drm/mediatek/
> 
> and we know find WHO for help.
> So I think we should drop device name and just print vio address is
> enough for debug.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/arch/arm64/boot/dts/mediatek/mt8173.dtsi?h=v5.8-rc1
> [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/MAINTAINERS?h=v5.8-rc1
> 

You are right. This is a way to find WHO can help this issue.
We integrated our internal debug system with device name, and it can
help us to find module owner automatically instead of searching dts and
source code manually.
But this kinds of debugging flow is not necessary for upstream. I'll try
to remove it.

> > +       {1, 23, 45, "MMSYS_MDP", true},
> > +       {1, 24, 46, "MMSYS_MDP", true},
> > +       {1, 25, 47, "MMSYS_MDP", true},
> > +       {1, 26, 48, "MMSYS_MDP", true},
> > +
> 
> [snip]
> 
> > +
> > +int mtk_devapc_probe(struct platform_device *pdev, struct mtk_devapc_soc *soc)
> > +{
> > +       struct device_node *node = pdev->dev.of_node;
> > +       u32 slave_type_num;
> > +       int slave_type;
> > +       int ret;
> > +
> > +       if (IS_ERR(node))
> > +               return -ENODEV;
> > +
> > +       mtk_devapc_ctx->soc = soc;
> > +       slave_type_num = mtk_devapc_ctx->soc->slave_type_num;
> > +
> > +       for (slave_type = 0; slave_type < slave_type_num; slave_type++) {
> > +               mtk_devapc_ctx->devapc_pd_base[slave_type] =
> > +                       of_iomap(node, slave_type);
> > +               if (!mtk_devapc_ctx->devapc_pd_base[slave_type])
> > +                       return -EINVAL;
> > +       }
> > +
> > +       mtk_devapc_ctx->infracfg_base = of_iomap(node, slave_type_num + 1);
> > +       if (!mtk_devapc_ctx->infracfg_base)
> > +               return -EINVAL;
> > +
> > +       mtk_devapc_ctx->devapc_irq = irq_of_parse_and_map(node, 0);
> > +       if (!mtk_devapc_ctx->devapc_irq)
> > +               return -EINVAL;
> > +
> > +       /* CCF (Common Clock Framework) */
> > +       mtk_devapc_ctx->devapc_infra_clk = devm_clk_get(&pdev->dev,
> > +                                                       "devapc-infra-clock");
> > +
> > +       if (IS_ERR(mtk_devapc_ctx->devapc_infra_clk))
> > +               return -EINVAL;
> > +
> > +       proc_create("devapc_dbg", 0664, NULL, &devapc_dbg_fops);
> 
> I think debugfs is not a basic function, so move debugfs function to
> another patch.
> 

Okay, I'll try to remove and upstream again.

> Regards,
> Chun-Kuang.
> 
> > +
> > +       if (clk_prepare_enable(mtk_devapc_ctx->devapc_infra_clk))
> > +               return -EINVAL;
> > +
> > +       start_devapc();
> > +
> > +       ret = devm_request_irq(&pdev->dev, mtk_devapc_ctx->devapc_irq,
> > +                              (irq_handler_t)devapc_violation_irq,
> > +                              IRQF_TRIGGER_NONE, "devapc", NULL);
> > +       if (ret) {
> > +               pr_err(PFX "request devapc irq failed, ret:%d\n", ret);
> > +               return ret;
> > +       }
> > +
> > +       return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(mtk_devapc_probe);
> > +
> >