From: Neal Liu <neal.liu@mediatek.com>
To: Matthias Brugger <matthias.bgg@gmail.com>
Cc: Neal Liu <neal.liu@mediatek.com>,
Rob Herring <robh+dt@kernel.org>,
Chun-Kuang Hu <chunkuang.hu@kernel.org>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>,
"linux-mediatek@lists.infradead.org"
<linux-mediatek@lists.infradead.org>,
lkml <linux-kernel@vger.kernel.org>,
wsd_upstream <wsd_upstream@mediatek.com>
Subject: Re: [PATCH v7 2/2] soc: mediatek: add mt6779 devapc driver
Date: Thu, 15 Oct 2020 10:13:37 +0800 [thread overview]
Message-ID: <1602728017.11536.5.camel@mtkswgap22> (raw)
In-Reply-To: <1602124514.28301.17.camel@mtkswgap22>
On Thu, 2020-10-08 at 10:35 +0800, Neal Liu wrote:
> On Wed, 2020-10-07 at 12:44 +0200, Matthias Brugger wrote:
> >
> > On 27/08/2020 05:06, Neal Liu wrote:
[...]
> > > +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;
> >
> > So bit 0 of selection register (pd_vio_shift_sel_reg) does not represent a
> > violation group?
> > I don't know how the HW works, but is seems odd to me. In case that's bit 0
> > actually doesn't represent anything: how can an interrupt be triggered without
> > any debug information present (means val == 0)?
>
> This check implies HW status has something wrong. It cannot get any
> debug information for this case.
> It won't happen in normal scenario. Should we remove this check?
>
Sorry, I missed the most common part. Is function is in the while loop:
while (devapc_sync_vio_dbg(ctx))
...
We keep find the minimum bit in pd_vio_shift_sta_reg to get the
violation information, (pd_vio_shift_sta_reg might raise multiple bits)
until all raised bit (shift group) has been handled.
So I don't think it's necessary to add WARN message in this case.
Thanks
> >
> > > +
> > > + 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__);
> >
> > In which case this can happen? I'm asking, because we are calling
> > devapc_sync_vio_dbg() in a while loop that could make the kernel hang here.
> >
> > Do I understand correctly, that we are using the while loop, because there can
> > be more then one violation group which got triggered (read, more then one bit is
> > set in pd_vio_shift_sta_reg)? Would it make more sense then to read the register
> > once and do all the shift operation for all groups which bit set to 1 in the
> > shift status register?
>
> Yes, your understanding is correct.
> This check also implies HW status has something wrong. We return false
> to skip further violation info dump.
> How could this case make the kernel hang?
>
> >
> > > + return false;
> > > + }
> > > +
> > > + /* Stop syncing */
> > > + writel(0x0, pd_vio_shift_con_reg);
> > > +
> > > + /* Write clear */
> > > + writel(0x1 << min_shift_group, pd_vio_shift_sta_reg);
> > > +
> > > + 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;
> > > + void __iomem *vio_dbg0_reg;
> > > + void __iomem *vio_dbg1_reg;
> > > +
> > > + 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);
> > > +}
> > > +
> > > +/*
> > > + * 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)
> >
> > static irqreturn_t devapc_violation_irq(int irq_number, void *data)
> > {
> > struct mtk_devapc_context *ctx = data;
>
> Okay, I'll fix it on next patch.
> Thanks
>
> >
> > > +{
> > > + while (devapc_sync_vio_dbg(ctx))
> > > + devapc_extract_vio_dbg(ctx);
> > > +
> > > + clear_vio_status(ctx);
> > > +
> > > + return IRQ_HANDLED;
> > > +}
> > > +
> > > +/*
> > > + * start_devapc - unmask slave's irq to start receiving devapc violation.
> > > + */
> > > +static void start_devapc(struct mtk_devapc_context *ctx)
> > > +{
> > > + writel(BIT(31), ctx->infra_base + ctx->data->apc_con_offset);
> > > +
> > > + mask_module_irq(ctx, false);
> > > +}
> > > +
> > > +/*
> > > + * stop_devapc - mask slave's irq to stop service.
> > > + */
> > > +static void stop_devapc(struct mtk_devapc_context *ctx)
> > > +{
> > > + mask_module_irq(ctx, true);
> > > +
> > > + writel(BIT(2), ctx->infra_base + ctx->data->apc_con_offset);
> > > +}
> > > +
> > > +static const struct mtk_devapc_data devapc_mt6779 = {
> > > + .vio_idx_num = 511,
> > > + .vio_mask_offset = 0x0,
> > > + .vio_sta_offset = 0x400,
> > > + .vio_dbg0_offset = 0x900,
> > > + .vio_dbg1_offset = 0x904,
> > > + .apc_con_offset = 0xF00,
> > > + .vio_shift_sta_offset = 0xF10,
> > > + .vio_shift_sel_offset = 0xF14,
> > > + .vio_shift_con_offset = 0xF20,
> > > +};
> > > +
> > > +static const struct of_device_id mtk_devapc_dt_match[] = {
> > > + {
> > > + .compatible = "mediatek,mt6779-devapc",
> > > + .data = &devapc_mt6779,
> > > + }, {
> > > + },
> > > +};
> > > +
> > > +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);
> >
> > Does this mean the device is part of the infracfg block?
> > I wasn't able to find any information about it.
>
> I'm not sure why you would ask infracfg block. devapc is parts of our
> SoC infra, it's different with infracfg.
>
> >
> > > + 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,
> >
> > No cast should be needed.
>
> Okay, I'll remove it on next patch.
> Thanks
>
> >
> > > + 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);
> > > +
> > > + clk_disable_unprepare(ctx->infra_clk);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static struct platform_driver mtk_devapc_driver = {
> > > + .probe = mtk_devapc_probe,
> > > + .remove = mtk_devapc_remove,
> > > + .driver = {
> > > + .name = KBUILD_MODNAME,
> >
> > .name = "mtk-devapc",
>
> Okay, I'll add it on next patch.
> Thanks
>
> >
> > Regards,
> > Matthias
>
>
next prev parent reply other threads:[~2020-10-15 2:13 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-27 3:06 [PATCH v7] Add MediaTek MT6779 devapc driver Neal Liu
2020-08-27 3:06 ` [PATCH v7 1/2] dt-bindings: devapc: add bindings for mtk-devapc Neal Liu
2020-08-27 3:06 ` [PATCH v7 2/2] soc: mediatek: add mt6779 devapc driver Neal Liu
2020-10-02 16:24 ` Chun-Kuang Hu
2020-10-05 7:20 ` Neal Liu
2020-10-07 10:44 ` Matthias Brugger
2020-10-08 2:35 ` Neal Liu
2020-10-08 8:45 ` Matthias Brugger
2020-10-08 9:39 ` Neal Liu
2020-10-09 12:34 ` Matthias Brugger
2020-10-12 3:24 ` Neal Liu
2020-10-15 2:13 ` Neal Liu [this message]
2020-10-15 9:13 ` Matthias Brugger
2020-09-02 6:40 ` [PATCH v7] Add MediaTek MT6779 " Neal Liu
2020-09-09 8:37 ` Neal Liu
2020-09-16 8:58 ` Neal Liu
2020-09-22 7:13 ` Neal Liu
2020-09-30 7:10 ` Neal Liu
2020-10-02 16:17 ` 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=1602728017.11536.5.camel@mtkswgap22 \
--to=neal.liu@mediatek.com \
--cc=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=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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).