From: houlong wei <houlong.wei@mediatek.com>
To: CK Hu <ck.hu@mediatek.com>
Cc: "Jassi Brar" <jassisinghbrar@gmail.com>,
"Matthias Brugger" <matthias.bgg@gmail.com>,
"Rob Herring" <robh+dt@kernel.org>,
"Daniel Kurtz" <djkurtz@chromium.org>,
"Sascha Hauer" <s.hauer@pengutronix.de>,
"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@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>,
srv_heupstream <srv_heupstream@mediatek.com>,
"Sascha Hauer" <kernel@pengutronix.de>,
"Philipp Zabel" <p.zabel@pengutronix.de>,
"Nicolas Boichat" <drinkcat@chromium.org>,
"Bibby Hsieh (謝濟遠)" <Bibby.Hsieh@mediatek.com>,
"YT Shen (沈岳霆)" <Yt.Shen@mediatek.com>,
"Daoyuan Huang (黃道原)" <Daoyuan.Huang@mediatek.com>,
"Jiaguang Zhang (张加广)" <Jiaguang.Zhang@mediatek.com>,
"Dennis-YC Hsieh (謝宇哲)" <Dennis-YC.Hsieh@mediatek.com>,
"Monica Wang (王孟婷)" <monica.wang@mediatek.com>,
"Hs Liao (廖宏祥)" <Hs.Liao@mediatek.com>,
"Ginny Chen (陳治傑)" <ginny.chen@mediatek.com>,
"Enzhu Wang (王恩柱)" <enzhu.wang@mediatek.com>,
houlong.wei@mediatek.com
Subject: Re: [PATCH v22 4/4] soc: mediatek: Add Mediatek CMDQ helper
Date: Wed, 4 Jul 2018 08:47:37 +0800 [thread overview]
Message-ID: <1530665257.24157.7.camel@mhfsdcap03> (raw)
In-Reply-To: <1530264123.22098.12.camel@mtksdaap41>
On Fri, 2018-06-29 at 17:22 +0800, CK Hu wrote:
> Hi, Houlong:
>
> On Fri, 2018-06-29 at 07:32 +0800, houlong wei wrote:
> > On Thu, 2018-06-28 at 18:41 +0800, CK Hu wrote:
> > > Hi, Houlong:
> > >
> > > Some inline comment.
> > >
> > > On Wed, 2018-06-27 at 19:16 +0800, Houlong Wei wrote:
> > > > Add Mediatek CMDQ helper to create CMDQ packet and assemble GCE op code.
> > > >
> > > > Signed-off-by: Houlong Wei <houlong.wei@mediatek.com>
> > > > Signed-off-by: HS Liao <hs.liao@mediatek.com>
> > > > ---
> > > > drivers/soc/mediatek/Kconfig | 12 ++
> > > > drivers/soc/mediatek/Makefile | 1 +
> > > > drivers/soc/mediatek/mtk-cmdq-helper.c | 258 ++++++++++++++++++++++++++++++++
> > > > include/linux/soc/mediatek/mtk-cmdq.h | 132 ++++++++++++++++
> > > > 4 files changed, 403 insertions(+)
> > > > create mode 100644 drivers/soc/mediatek/mtk-cmdq-helper.c
> > > > create mode 100644 include/linux/soc/mediatek/mtk-cmdq.h
> > > >
> > > > diff --git a/drivers/soc/mediatek/Kconfig b/drivers/soc/mediatek/Kconfig
> > > > index a7d0667..17bd759 100644
> > > > --- a/drivers/soc/mediatek/Kconfig
> > > > +++ b/drivers/soc/mediatek/Kconfig
> > > > @@ -4,6 +4,18 @@
> > > > menu "MediaTek SoC drivers"
> > > > depends on ARCH_MEDIATEK || COMPILE_TEST
> > > >
> > >
>
> [...]
>
> > > > +
> > > > +static int cmdq_pkt_finalize(struct cmdq_pkt *pkt)
> > > > +{
> > > > + int err;
> > > > +
> > > > + if (cmdq_pkt_is_finalized(pkt))
> > > > + return 0;
> > > > +
> > > > + /* insert EOC and generate IRQ for each command iteration */
> > > > + err = cmdq_pkt_append_command(pkt, CMDQ_CODE_EOC, 0, CMDQ_EOC_IRQ_EN);
> > > > + if (err < 0)
> > > > + return err;
> > > > +
> > > > + /* JUMP to end */
> > > > + err = cmdq_pkt_append_command(pkt, CMDQ_CODE_JUMP, 0, CMDQ_JUMP_PASS);
> > > > + if (err < 0)
> > > > + return err;
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +int cmdq_pkt_flush_async(struct cmdq_client *client, struct cmdq_pkt *pkt,
> > > > + cmdq_async_flush_cb cb, void *data)
> > > > +{
> > > > + int err;
> > > > + struct device *dev;
> > > > + dma_addr_t dma_addr;
> > > > +
> > > > + err = cmdq_pkt_finalize(pkt);
> > > > + if (err < 0)
> > > > + return err;
> > > > +
> > > > + dev = client->chan->mbox->dev;
> > > > + dma_addr = dma_map_single(dev, pkt->va_base, pkt->cmd_buf_size,
> > > > + DMA_TO_DEVICE);
> > >
> > > You map here, but I could not find unmap, so the unmap should be done in
> > > client driver. I would prefer a symmetric map/unmap which means that
> > > both map and unmap are done in client driver. I think you put map here
> > > because you should map after finalize.
> >
> > The unmap is done before callback to client, in function
> > cmdq_task_exec_done, mtk-cmdq-mailbox.c.
>
> You put unmap in mailbox controller, and map here (here would be mailbox
> client), so this is not symmetric. If the code is asymmetric, it's easy
> to cause bug and not easy to maintain. So I would like move both
> map/unmap to client driver.
>
Since map/unmap is common code for client drivers, can we move unmap to
CMDQ helper and do not put in client driver?
> >
> > > Therefore, export
> > > cmdq_pkt_finalize() to client driver and let client do finalize, so
> > > there is no finalize in flush function. This method have a benefit that
> > > if client reuse command buffer, it need not to map/unmap frequently.
> >
> > If client reuse command buffer or cmdq_pkt(command queue packet), client
> > will add new commands to the cmdq_pkt, so map/unmap are necessary for
> > each cmdq_pkt flush.
>
> If the buffer size is 4K bytes, client driver could map the whole 4K at
> initialization. Before it write new command, it call
> dma_sync_single_for_cpu(), after it write new command, it call
> dma_sync_single_for_device(). And then it could flush this buffer to
> mailbox controller. So client driver just call dma sync function when it
> reuse the command buffer. dma sync function is much lightweight then dma
> map because map would search the memory area which could be mapped.
>
> Regards,
> CK
Maybe we can do dma map/unmap/sync in cmdq helper, and make client
driver simple.
>
> >
> > >
> > > Regards,
> > > CK
> > >
> > > > + if (dma_mapping_error(dev, dma_addr)) {
> > > > + dev_err(client->chan->mbox->dev, "dma map failed\n");
> > > > + return -ENOMEM;
> > > > + }
> > > > +
> > > > + pkt->pa_base = dma_addr;
> > > > + pkt->cb.cb = cb;
> > > > + pkt->cb.data = data;
> > > > +
> > > > + mbox_send_message(client->chan, pkt);
> > > > + /* We can send next packet immediately, so just call txdone. */
> > > > + mbox_client_txdone(client->chan, 0);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +EXPORT_SYMBOL(cmdq_pkt_flush_async);
> > > > +
> > > > +struct cmdq_flush_completion {
> > > > + struct completion cmplt;
> > > > + bool err;
> > > > +};
> > > > +
> > > > +static void cmdq_pkt_flush_cb(struct cmdq_cb_data data)
> > > > +{
> > > > + struct cmdq_flush_completion *cmplt = data.data;
> > > > +
> > > > + cmplt->err = data.err;
> > > > + complete(&cmplt->cmplt);
> > > > +}
> > > > +
> > > > +int cmdq_pkt_flush(struct cmdq_client *client, struct cmdq_pkt *pkt)
> > > > +{
> > > > + struct cmdq_flush_completion cmplt;
> > > > + int err;
> > > > +
> > > > + init_completion(&cmplt.cmplt);
> > > > + err = cmdq_pkt_flush_async(client, pkt, cmdq_pkt_flush_cb, &cmplt);
> > > > + if (err < 0)
> > > > + return err;
> > > > + wait_for_completion(&cmplt.cmplt);
> > > > +
> > > > + return cmplt.err ? -EFAULT : 0;
> > > > +}
> > > > +EXPORT_SYMBOL(cmdq_pkt_flush);
> > >
> > > [...]
> > >
> > >
> >
> >
>
>
next prev parent reply other threads:[~2018-07-04 0:47 UTC|newest]
Thread overview: 19+ messages / expand[flat|nested] mbox.gz Atom feed top
2018-06-27 11:16 [PATCH v22 0/4] MediaTek MT8173 CMDQ support Houlong Wei
2018-06-27 11:16 ` [PATCH v22 1/4] dt-bindings: soc: Add documentation for the MediaTek GCE unit Houlong Wei
2018-07-03 2:30 ` Rob Herring
2018-07-03 23:39 ` houlong wei
2018-07-05 20:08 ` Rob Herring
2018-07-06 2:29 ` houlong wei
2018-06-27 11:16 ` [PATCH v22 2/4] mailbox: mediatek: Add Mediatek CMDQ driver Houlong Wei
2018-06-29 7:08 ` CK Hu
2018-07-04 0:10 ` houlong wei
2018-07-04 9:03 ` CK Hu
2018-07-06 2:04 ` houlong wei
2018-06-27 11:16 ` [PATCH v22 3/4] arm64: dts: mt8173: Add GCE node Houlong Wei
2018-06-27 11:16 ` [PATCH v22 4/4] soc: mediatek: Add Mediatek CMDQ helper Houlong Wei
2018-06-28 10:41 ` CK Hu
2018-06-28 23:32 ` houlong wei
2018-06-29 9:22 ` CK Hu
2018-07-04 0:47 ` houlong wei [this message]
2018-07-04 2:39 ` CK Hu
2018-07-06 1:22 ` houlong wei
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=1530665257.24157.7.camel@mhfsdcap03 \
--to=houlong.wei@mediatek.com \
--cc=Bibby.Hsieh@mediatek.com \
--cc=Daoyuan.Huang@mediatek.com \
--cc=Dennis-YC.Hsieh@mediatek.com \
--cc=Hs.Liao@mediatek.com \
--cc=Jiaguang.Zhang@mediatek.com \
--cc=Yt.Shen@mediatek.com \
--cc=ck.hu@mediatek.com \
--cc=devicetree@vger.kernel.org \
--cc=djkurtz@chromium.org \
--cc=drinkcat@chromium.org \
--cc=enzhu.wang@mediatek.com \
--cc=ginny.chen@mediatek.com \
--cc=jassisinghbrar@gmail.com \
--cc=kernel@pengutronix.de \
--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=monica.wang@mediatek.com \
--cc=p.zabel@pengutronix.de \
--cc=robh+dt@kernel.org \
--cc=s.hauer@pengutronix.de \
--cc=srv_heupstream@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).