Message-ID: <1596163478.3932.17.camel@mtkswgap22>
Date:   Fri, 31 Jul 2020 10:44:38 +0800
From:   Neal Liu <neal.liu@...iatek.com>
To:     Chun-Kuang Hu <chunkuang.hu@...nel.org>
CC:     Neal Liu <neal.liu@...iatek.com>, Rob Herring <robh+dt@...nel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        <devicetree@...r.kernel.org>,
        wsd_upstream <wsd_upstream@...iatek.com>,
        lkml <linux-kernel@...r.kernel.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@...ts.infradead.org>,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver

Hi Chun-Kuang,


On Thu, 2020-07-30 at 00:38 +0800, Chun-Kuang Hu wrote:
> Hi, Neal:
> 
> Neal Liu <neal.liu@...iatek.com> 於 2020年7月29日 週三 下午4:29寫道：
> >
> > 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@...iatek.com>
> > ---
> 
> [snip]
> 
> > +
> > +static int get_shift_group(struct mtk_devapc_context *ctx)
> > +{
> > +       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 -EIO;
> > +}
> 
> get_shift_group() is a small function, I would like to merge this
> function into sync_vio_dbg() to make code more simple.

This function have a specific functionality. And it would make this
driver more readability. I would like to keep it as a function. Is that
okay for you?

> 
> > +
> 
> [snip]
> 
> > +
> > +#define PHY_DEVAPC_TIMEOUT     0x10000
> > +
> > +/*
> > + * 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;
> > +       void __iomem *pd_vio_shift_con_reg;
> > +       int ret;
> > +       u32 val;
> > +
> > +       pd_vio_shift_sta_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sta;
> > +       pd_vio_shift_sel_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_sel;
> > +       pd_vio_shift_con_reg = ctx->devapc_pd_base + ctx->offset->vio_shift_con;
> > +
> > +       /* Enable shift mechansim */
> > +       writel(0x1 << shift_bit, pd_vio_shift_sel_reg);
> > +       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__);
> > +
> > +       /* Disable shift mechanism */
> > +       writel(0x0, pd_vio_shift_con_reg);
> > +       writel(0x0, pd_vio_shift_sel_reg);
> > +       writel(0x1 << shift_bit, pd_vio_shift_sta_reg);
> > +
> > +       return ret;
> > +}
> > +
> 
> [snip]
> 
> > +
> > +/*
> > + * devapc_extract_vio_dbg - extract full violation information after doing
> > + *                          shift mechanism.
> > + */
> > +static void devapc_extract_vio_dbg(struct mtk_devapc_context *ctx)
> > +{
> > +       const struct mtk_devapc_vio_dbgs *vio_dbgs;
> > +       struct mtk_devapc_vio_info *vio_info;
> > +       void __iomem *vio_dbg0_reg;
> > +       void __iomem *vio_dbg1_reg;
> > +       u32 dbg0;
> > +
> > +       vio_dbg0_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg0;
> > +       vio_dbg1_reg = ctx->devapc_pd_base + ctx->offset->vio_dbg1;
> > +
> > +       vio_dbgs = ctx->vio_dbgs;
> > +       vio_info = ctx->vio_info;
> > +
> > +       /* Starts to extract violation information */
> > +       dbg0 = readl(vio_dbg0_reg);
> > +       vio_info->vio_addr = readl(vio_dbg1_reg);
> > +
> > +       vio_info->master_id = (dbg0 & vio_dbgs->mstid.mask) >>
> > +                             vio_dbgs->mstid.start;
> 
> What is master_id? How could we use it to debug? For example, if we
> get a master_id = 1, what should we do for this?
> 
> > +       vio_info->domain_id = (dbg0 & vio_dbgs->dmnid.mask) >>
> > +                             vio_dbgs->dmnid.start;
> 
> What is domain_id? How could we use it to debug? For example, if we
> get a domain_id = 2, what should we do for this?
> 

master_id and domain_id belongs our bus side-band signal info. It can
help us to find the violation master.

> > +       vio_info->write = ((dbg0 & vio_dbgs->vio_w.mask) >>
> > +                           vio_dbgs->vio_w.start) == 1;
> > +       vio_info->read = ((dbg0 & vio_dbgs->vio_r.mask) >>
> > +                         vio_dbgs->vio_r.start) == 1;
> > +       vio_info->vio_addr_high = (dbg0 & vio_dbgs->addr_h.mask) >>
> > +                                 vio_dbgs->addr_h.start;
> 
> What is vio_addr_high? As I know all register address are 32 bits, is
> vio_addr_high the address above 32 bits?

Yes, you are right. In MT6779, all register base are 32 bits. We can
ignore this info for current driver. I'll update on next patch.
Thanks !

> 
> > +
> > +       devapc_vio_info_print(ctx);
> > +}
> > +
> 
> [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 *ctx)
> > +{
> > +       u32 vio_idx;
> > +
> > +       /*
> > +        * Mask slave's irq before clearing vio status.
> > +        * Must do it to avoid nested interrupt and prevent
> > +        * unexpected behavior.
> > +        */
> > +       for (vio_idx = 0; vio_idx < ctx->vio_idx_num; vio_idx++)
> > +               mask_module_irq(ctx, vio_idx, true);
> 
> I would like to rewrite this for-loop as below to prevent too many
> function call in irq handler.
> 
> for (i = 0; i < VIO_MOD_TO_REG_IND(ctx->vio_idx_num); i++)
>     writel(0xffffffff, ctx->devapc_pd_base + ctx->offset->vio_mask + 4 * i);
> 

This idea is okay for me. Is there any macro to replace 0xffffffff?

> reg  = readl(ctx->devapc_pd_base + ctx->offset->vio_mask + 4 * i);
> reg |= 1 << (ctx->vio_idx_num - 32 * i + 1) - 1;
> writel(reg, ctx->devapc_pd_base + ctx->offset->vio_mask + 4 * i);

Are you trying to clear the bits which over vio_idx_num?
If yes, I think the second line should be:
reg &= 1 << (ctx->vio_idx_num - 32 * i) - 1;

For example, if vio_idx_num is 40:
after for loop:
vio_mask0 = 0xffffffff;
vio_mask1 = 0xffffffff;

reg = readl(vio_mask1);
reg &= (1 << 8) - 1; (which is 0x000000ff)
reg will be 0xff
writel(reg, vio_mask1);

Does it make sense?

Actually, it is okay to overwrite the unused register bits.
So it's no matter to do this step.

> 
> > +
> > +       devapc_dump_vio_dbg(ctx);
> > +
> > +       /*
> > +        * Ensure that violation info are written
> > +        * before further operations
> > +        */
> > +       smp_mb();
> > +
> > +       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);
> > +       }
> 
> Ditto for this for-loop.

Ditto.

> 
> > +
> > +       return IRQ_HANDLED;
> > +}
> > +
> 
> [snip]
> 
> > +
> > +static int mtk_devapc_probe(struct platform_device *pdev)
> > +{
> > +       struct device_node *node = pdev->dev.of_node;
> > +       struct mtk_devapc_context *ctx;
> > +       struct clk *devapc_infra_clk;
> > +       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 = (struct mtk_devapc_context *)of_device_get_match_data(&pdev->dev);
> > +       ctx->dev = &pdev->dev;
> > +
> > +       ctx->vio_info = devm_kzalloc(&pdev->dev,
> > +                                    sizeof(struct mtk_devapc_vio_info),
> > +                                    GFP_KERNEL);
> > +       if (!ctx->vio_info)
> > +               return -ENOMEM;
> > +
> > +       ctx->devapc_pd_base = of_iomap(node, 0);
> > +       if (!ctx->devapc_pd_base)
> > +               return -EINVAL;
> > +
> > +       devapc_irq = irq_of_parse_and_map(node, 0);
> > +       if (!devapc_irq)
> > +               return -EINVAL;
> > +
> > +       devapc_infra_clk = devm_clk_get(&pdev->dev, "devapc-infra-clock");
> > +       if (IS_ERR(devapc_infra_clk))
> > +               return -EINVAL;
> > +
> > +       if (clk_prepare_enable(devapc_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)
> 
> You should clk_disable_unprepare(devapc_infra_clk);

Yes, I miss this part. Thanks for your remind.
I'll update it on next patch.

> 
> > +               return ret;
> > +
> > +       start_devapc(ctx);
> > +
> > +       return 0;
> > +}
> > +
> > +static int mtk_devapc_remove(struct platform_device *dev)
> > +{
> 
> Ditto.

Ditto.

> 
> Regards,
> Chun-Kuang.
> 
> > +       return 0;
> > +}
> > +
> > +static struct platform_driver mtk_devapc_driver = {
> > +       .probe = mtk_devapc_probe,
> > +       .remove = mtk_devapc_remove,
> > +       .driver = {
> > +               .name = KBUILD_MODNAME,
> > +               .of_match_table = mtk_devapc_dt_match,
> > +       },
> > +};
> > +
> > +module_platform_driver(mtk_devapc_driver);
> > +

Powered by blists - more mailing lists