Linux-Devicetree Archive on lore.kernel.org
 help / color / Atom feed
From: Chun-Kuang Hu <chunkuang.hu@kernel.org>
To: Neal Liu <neal.liu@mediatek.com>
Cc: Rob Herring <robh+dt@kernel.org>,
	Matthias Brugger <matthias.bgg@gmail.com>,
	devicetree@vger.kernel.org,
	wsd_upstream <wsd_upstream@mediatek.com>,
	lkml <linux-kernel@vger.kernel.org>,
	"moderated list:ARM/Mediatek SoC support" 
	<linux-mediatek@lists.infradead.org>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver
Date: Thu, 30 Jul 2020 00:38:34 +0800
Message-ID: <CAAOTY_8bE8Qo5-0MA9J1gcEbN4DP=Wf2O6vOUVos=VkoODeayA@mail.gmail.com> (raw)
In-Reply-To: <1596010690-13178-3-git-send-email-neal.liu@mediatek.com>

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.

> +

[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?

> +       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?

> +
> +       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);

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);

> +
> +       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.

> +
> +       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);

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

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);
> +

  reply index

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-29  8:18 [PATCH v4] Add MediaTek MT6779 devapc driver Neal Liu
2020-07-29  8:18 ` [PATCH v4 1/2] dt-bindings: devapc: add bindings for mtk-devapc Neal Liu
2020-07-29  8:18 ` [PATCH v4 2/2] soc: mediatek: add mtk-devapc driver Neal Liu
2020-07-29 16:38   ` Chun-Kuang Hu [this message]
2020-07-31  2:44     ` Neal Liu
2020-07-31 15:03       ` Chun-Kuang Hu
2020-08-03  3:32         ` Neal Liu
2020-08-03 16:13           ` Chun-Kuang Hu
2020-08-04  2:18             ` Neal Liu
2020-08-04 15:27               ` Chun-Kuang Hu
2020-07-29 22:47   ` Chun-Kuang Hu
2020-07-31  2:47     ` Neal Liu
2020-07-30 16:14   ` Chun-Kuang Hu
2020-07-31  2:52     ` Neal Liu
2020-07-31 15:55       ` Chun-Kuang Hu
2020-08-03  3:41         ` Neal Liu
2020-08-01  0:12   ` Chun-Kuang Hu
2020-08-03  4:01     ` Neal Liu
2020-08-03 16:04       ` Chun-Kuang Hu
2020-08-04  2:08         ` Neal Liu
2020-08-04 15:55           ` Chun-Kuang Hu
2020-08-01 23:50   ` Chun-Kuang Hu
2020-08-03  4:05     ` Neal Liu
2020-08-03 15:38       ` Chun-Kuang Hu

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAAOTY_8bE8Qo5-0MA9J1gcEbN4DP=Wf2O6vOUVos=VkoODeayA@mail.gmail.com' \
    --to=chunkuang.hu@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=matthias.bgg@gmail.com \
    --cc=neal.liu@mediatek.com \
    --cc=robh+dt@kernel.org \
    --cc=wsd_upstream@mediatek.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Devicetree Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-devicetree/0 linux-devicetree/git/0.git
	git clone --mirror https://lore.kernel.org/linux-devicetree/1 linux-devicetree/git/1.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-devicetree linux-devicetree/ https://lore.kernel.org/linux-devicetree \
		devicetree@vger.kernel.org
	public-inbox-index linux-devicetree

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-devicetree


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git