lkml.org 
[lkml]   [2020]   [Aug]   [2]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver
    From
    Date
    Hi Chun-Kuang,

    On Fri, 2020-07-31 at 23:03 +0800, Chun-Kuang Hu wrote:
    > Hi, Neal:
    >
    > Neal Liu <neal.liu@mediatek.com> 於 2020年7月31日 週五 上午10:44寫道:
    > >
    > > Hi Chun-Kuang,
    > >
    > >
    > > On Thu, 2020-07-30 at 00:38 +0800, Chun-Kuang Hu wrote:
    > > > Hi, Neal:
    > > >
    > > > Neal Liu <neal.liu@mediatek.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@mediatek.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?
    >
    > After merge, the function would be:
    >
    > static bool sync_min_shift_group_vio_dbg(struct mtk_devapc_context *ctx)
    > {
    > int min_shift_group;
    > int ret;
    > u32 val;
    >
    > /* find the minimum shift group which has violation */
    > val = readl(ctx->devapc_pd_base + ctx->offset->vio_shift_sta);
    > if (!val)
    > return false;
    >
    > min_shift_group = __ffs(val);
    >
    > /* Assign the group to sync */
    > writel(0x1 << min_shift_group, ctx->devapc_pd_base +
    > ctx->offset->vio_shift_sel);
    >
    > /* Start syncing */
    > writel(0x1, ctx->devapc_pd_base + ctx->offset->vio_shift_con);
    >
    > 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, ctx->devapc_pd_base + ctx->offset->vio_shift_con);
    >
    > /* ? */
    > writel(0x1 << min_shift_group, ctx->devapc_pd_base +
    > ctx->offset->vio_shift_sta);
    >
    > return true;
    > }
    >
    > The whole function is to sync min_shift_group violation info, I don't
    > know why separate any part to an independent function? Any function
    > call would cause penalty on CPU performance, so I does not like to
    > break this function. After good comment, I think every body could
    > understand the function of each register.
    > After the merge, the code would be so simple as:
    >
    > while(sync_min_shift_group_vio_dbg(ctx))
    > devapc_extract_vio_dbg(ctx);
    >

    Okay, this looks good to me. I'll apply this on next patch.
    Thanks !

    > >
    > > >
    > > > > +
    > > >
    > > > [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.
    >
    > Does 'violation master' means the hardware could access the protected
    > register? (ex. CPU, GCE, ...) If so, I think it's better to add
    > comment to explain how to map (master_id, domain_id) to a hardware
    > (maybe the device in device tree) because every body does not know
    > what the number means. Don't try to translate the number to a string
    > because this would cost much time to do this. Just print a number and
    > we could find out the master by the comment.

    'violation master' means the master which violates the permission
    control. For example, if we set permission 'Secure R/W only' as CPU to
    spi register. When violation is triggered, it means CPU access spi
    register through normal world instead of secure world, which is not
    allowed.

    'master_id' cannot use the simple comments to describe which master it
    is. It depends on violation slaves. For example, if there are two
    violations:
    1. CPU access spi reg
    2. CPU access timer reg
    It might be different 'master_id' for CPU on these two cases.
    I would prefer to remain the id number if translate to a string is a bad
    idea.
    Thanks !

    >
    > >
    > > > > + 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 !
    >
    > Such a strange hardware, all register is 32 bits but it has a
    > vio_addr_high in its register. OK, just drop this.
    >
    > >
    > > >
    > > > > +
    > > > > + 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?
    >
    > GENMASK(31, 0);
    >
    > >
    > > > 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;
    >
    > when vio_idx_num is 40, VIO_MOD_TO_REG_IND(ctx->vio_idx_num) is 1, so
    > after for-loop:
    > vio_mask0 = 0xffffffff;
    >
    > And the code after for-loop just to do this:
    > vio_mask1 = 0xff;
    >
    > >
    > > 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.
    >
    > OK, the code would be
    >
    > for (i = 0; i <= VIO_MOD_TO_REG_IND(ctx->vio_idx_num - 1); i++)
    > writel(GENMASK(31, 0), ctx->devapc_pd_base + ctx->offset->vio_mask + 4 * i);
    >

    Yes, this is okay for me.
    Thanks !

    > Regards,
    > Chun-Kuang.
    >
    > >
    > > >
    > > > > +
    > > > > + 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);
    > > > > +
    > >

    \
     
     \ /
      Last update: 2020-08-03 05:33    [W:3.169 / U:0.224 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site