LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Shengjiu Wang <shengjiu.wang@gmail.com>
To: Mathieu Poirier <mathieu.poirier@linaro.org>
Cc: Shengjiu Wang <shengjiu.wang@nxp.com>,
	Ohad Ben Cohen <ohad@wizery.com>,
	bjorn.andersson@linaro.org, Rob Herring <robh+dt@kernel.org>,
	Shawn Guo <shawnguo@kernel.org>,
	Sascha Hauer <s.hauer@pengutronix.de>,
	Sascha Hauer <kernel@pengutronix.de>,
	Fabio Estevam <festevam@gmail.com>,
	NXP Linux Team <linux-imx@nxp.com>,
	linux-remoteproc@vger.kernel.org,
	"open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
	<devicetree@vger.kernel.org>,
	"moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" 
	<linux-arm-kernel@lists.infradead.org>,
	linux-kernel <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v2 2/2] remoteproc: imx_dsp_rproc: add remoteproc driver for dsp on i.MX
Date: Thu, 2 Sep 2021 09:46:04 +0800	[thread overview]
Message-ID: <CAA+D8APDU8H-nC2xnp+dpa8sLALAnYQFDCb2stv4=o0Hp6NOvw@mail.gmail.com> (raw)
In-Reply-To: <20210901155051.GA1033512@p14s>

On Wed, Sep 1, 2021 at 11:50 PM Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
>
> [...]
>
> >
> > >
> > > > +
> > > > +/* address translation table */
> > > > +struct imx_dsp_rproc_att {
> > > > +     u32 da; /* device address (From Cortex M4 view)*/
> > > > +     u32 sa; /* system bus address */
> > > > +     u32 size; /* size of reg range */
> > > > +     int flags;
> > > > +};
> > >
> > > This is already defined in imx_rproc.c - why do we need another definition here?
> >
> > I just want to avoid to modify imx_rproc.c driver.
> > So with this comments, should I add imx_rproc.h for extracting the common
> > structure in it?
> >
>
> Yes, that is probably the best way to proceed.

Ok, I will do it in the next version.

>
> > >
> > > > +
> > > > +/* Remote core start/stop method */
> > > > +enum imx_dsp_rproc_method {
> > > > +     /* Through syscon regmap */
> > > > +     IMX_DSP_MMIO,
> > > > +     IMX_DSP_SCU_API,
> > > > +};
> > >
> > > From where I stand it would be worth merging the above with imx_rproc_method
> > > found in imx_rproc.c.  I don't see a need for duplication.
> >
> > Should I add imx_rproc.h for extracting the common structure in it?
> >
>
> See my comment above.
>
> > >
> > > > +
> > > > +struct imx_dsp_rproc {
> > > > +     struct device                   *dev;
> > > > +     struct regmap                   *regmap;
> > > > +     struct rproc                    *rproc;
> > > > +     const struct imx_dsp_rproc_dcfg *dcfg;
> > > > +     struct clk                      *clks[DSP_RPROC_CLK_MAX];
> > > > +     struct mbox_client              cl;
> > > > +     struct mbox_client              cl_rxdb;
> > > > +     struct mbox_chan                *tx_ch;
> > > > +     struct mbox_chan                *rx_ch;
> > > > +     struct mbox_chan                *rxdb_ch;
> > > > +     struct device                   **pd_dev;
> > > > +     struct device_link              **pd_dev_link;
> > > > +     struct imx_sc_ipc               *ipc_handle;
> > > > +     struct work_struct              rproc_work;
> > > > +     struct workqueue_struct         *workqueue;
> > > > +     struct completion               pm_comp;
> > > > +     spinlock_t                      mbox_lock;    /* lock for mbox */
> > > > +     int                             num_domains;
> > > > +     u32                             flags;
> > > > +};
> > > > +
> > > > +struct imx_dsp_rproc_dcfg {
> > > > +     u32                             src_reg;
> > > > +     u32                             src_mask;
> > > > +     u32                             src_start;
> > > > +     u32                             src_stop;
> > > > +     const struct imx_dsp_rproc_att  *att;
> > > > +     size_t                          att_size;
> > > > +     enum imx_dsp_rproc_method       method;
> > >
> > > The above is similar to imx_rproc_cfg.  As such:
> > >
> > > struct imx_dsp_rproc_dcfg {
> > >         struct imx_rproc_cfg            *dcfg;
> > >         int (*reset)(struct imx_dsp_rproc *priv);
> > > };
> > >
> >
> > Yes, seems need to add imx_rproc.h header file.
> >
>
> [...]
>
> > > > +
> > > > +/* pm runtime */
> > > > +static int imx_dsp_runtime_resume(struct device *dev)
> > > > +{
> > > > +     struct rproc *rproc = dev_get_drvdata(dev);
> > > > +     struct imx_dsp_rproc *priv = rproc->priv;
> > > > +     const struct imx_dsp_rproc_dcfg *dcfg = priv->dcfg;
> > > > +     int ret;
> > > > +
> > > > +     ret = imx_dsp_rproc_mbox_init(priv);
> > >
> > > Why is the mailbox setup and destroyed with every PM cycle?  I find an overall
> > > lack of comments makes this driver difficult to review, resulting in having to
> > > spend more time to look at and understand the code.  I will have to continue
> > > tomorrow because, well, I ran out of time.
> > >
> > > Mathieu
> > >
> >
> > There is power domain attached with mailbox, if request the mailbox
> > channel, the power is enabled. so if setup mailbox in probe(), then
> > the power is enabled always which is no benefit for saving power.
> > so setup mailbox in pm_runtime_resume().
>
> This is the kind of very useful information that should be in comments.

All right,  I will add it in the next version.

>
> >
> > Sorry for lack of comments, I will add more in next version.
>
> That will be much appreciated.
>
> >
> > Again, Thanks your time for reviewing this patch.
>
> More comments to come in a minute...
>
> >
> > Best regards
> > Wang shengjiu
> >
> > > > +     if (ret) {
> > > > +             dev_err(dev, "failed on imx_dsp_rproc_mbox_init\n");
> > > > +             return ret;
> > > > +     }
> > > > +
> > > > +     ret = imx_dsp_rproc_clk_enable(priv);
> > > > +     if (ret) {
> > > > +             dev_err(dev, "failed on imx_dsp_rproc_clk_enable\n");
> > > > +             return ret;
> > > > +     }
> > > > +
> > > > +     /* reset DSP if needed */
> > > > +     if (dcfg->reset)
> > > > +             dcfg->reset(priv);
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static int imx_dsp_runtime_suspend(struct device *dev)
> > > > +{
> > > > +     struct rproc *rproc = dev_get_drvdata(dev);
> > > > +     struct imx_dsp_rproc *priv = rproc->priv;
> > > > +
> > > > +     imx_dsp_rproc_clk_disable(priv);
> > > > +
> > > > +     imx_dsp_rproc_free_mbox(priv);
> > > > +
> > > > +     return 0;
> > > > +}
> > > > +
> > > > +static void imx_dsp_load_firmware(const struct firmware *fw, void *context)
> > > > +{
> > > > +     struct rproc *rproc = context;
> > > > +     int ret;
> > > > +
> > > > +     /* load the ELF segments to memory */
> > > > +     ret = rproc_load_segments(rproc, fw);
> > > > +     if (ret)
> > > > +             goto out;
> > > > +
> > > > +     /* power up the remote processor */
> > > > +     ret = rproc->ops->start(rproc);
> > > > +     if (ret)
> > > > +             goto out;
> > > > +
> > > > +     /* same flow as first start */
> > > > +     rproc->ops->kick(rproc, 0);
> > > > +
> > > > +out:
> > > > +     release_firmware(fw);
> > > > +}
> > > > +
> > > > +static int imx_dsp_suspend(struct device *dev)
> > > > +{
> > > > +     struct rproc *rproc = dev_get_drvdata(dev);
> > > > +     struct imx_dsp_rproc *priv = rproc->priv;
> > > > +     __u32 mmsg = RP_MBOX_SUSPEND_SYSTEM;
> > > > +     int ret;
> > > > +
> > > > +     if (rproc->state == RPROC_RUNNING) {
> > > > +             reinit_completion(&priv->pm_comp);
> > > > +
> > > > +             ret = mbox_send_message(priv->tx_ch, (void *)&mmsg);
> > > > +             if (ret < 0) {
> > > > +                     dev_err(dev, "PM mbox_send_message failed: %d\n", ret);
> > > > +                     return ret;
> > > > +             }
> > > > +
> > > > +             if (!wait_for_completion_timeout(&priv->pm_comp, msecs_to_jiffies(100)))
> > > > +                     return -EBUSY;
> > > > +     }
> > > > +
> > > > +     return pm_runtime_force_suspend(dev);
> > > > +}
> > > > +
> > > > +static int imx_dsp_resume(struct device *dev)
> > > > +{
> > > > +     struct rproc *rproc = dev_get_drvdata(dev);
> > > > +     int ret = 0;
> > > > +
> > > > +     ret = pm_runtime_force_resume(dev);
> > > > +     if (ret)
> > > > +             return ret;
> > > > +
> > > > +     if (rproc->state == RPROC_RUNNING) {
> > > > +             /*TODO: load firmware and start */
> > > > +             ret = request_firmware_nowait(THIS_MODULE,
> > > > +                                           FW_ACTION_UEVENT,
> > > > +                                           rproc->firmware,
> > > > +                                           dev,
> > > > +                                           GFP_KERNEL,
> > > > +                                           rproc,
> > > > +                                           imx_dsp_load_firmware);
> > > > +             if (ret < 0) {
> > > > +                     dev_err(dev, "load firmware failed: %d\n", ret);
> > > > +                     goto err;
> > > > +             }
> > > > +     }
> > > > +
> > > > +     return 0;
> > > > +
> > > > +err:
> > > > +     pm_runtime_force_suspend(dev);
> > > > +
> > > > +     return ret;
> > > > +}
> > > > +
> > > > +static const struct dev_pm_ops imx_dsp_rproc_pm_ops = {
> > > > +     SET_SYSTEM_SLEEP_PM_OPS(imx_dsp_suspend, imx_dsp_resume)
> > > > +     SET_RUNTIME_PM_OPS(imx_dsp_runtime_suspend,
> > > > +                        imx_dsp_runtime_resume, NULL)
> > > > +};
> > > > +
> > > > +static const struct of_device_id imx_dsp_rproc_of_match[] = {
> > > > +     { .compatible = "fsl,imx8qxp-hifi4", .data = &imx_dsp_rproc_cfg_imx8qxp },
> > > > +     { .compatible = "fsl,imx8qm-hifi4",  .data = &imx_dsp_rproc_cfg_imx8qm },
> > > > +     { .compatible = "fsl,imx8mp-hifi4",  .data = &imx_dsp_rproc_cfg_imx8mp },
> > > > +     { .compatible = "fsl,imx8ulp-hifi4", .data = &imx_dsp_rproc_cfg_imx8ulp },
> > > > +     {},
> > > > +};
> > > > +MODULE_DEVICE_TABLE(of, imx_dsp_rproc_of_match);
> > > > +
> > > > +static struct platform_driver imx_dsp_rproc_driver = {
> > > > +     .probe = imx_dsp_rproc_probe,
> > > > +     .remove = imx_dsp_rproc_remove,
> > > > +     .driver = {
> > > > +             .name = "imx-dsp-rproc",
> > > > +             .of_match_table = imx_dsp_rproc_of_match,
> > > > +             .pm = &imx_dsp_rproc_pm_ops,
> > > > +     },
> > > > +};
> > > > +module_platform_driver(imx_dsp_rproc_driver);
> > > > +
> > > > +MODULE_LICENSE("GPL v2");
> > > > +MODULE_DESCRIPTION("i.MX HiFi Core Remote Processor Control Driver");
> > > > +MODULE_AUTHOR("Shengjiu Wang <shengjiu.wang@nxp.com>");
> > > > --
> > > > 2.17.1
> > > >

  reply	other threads:[~2021-09-02  1:46 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-25  7:28 [PATCH v2 1/2] dt-bindings: remoteproc: Add fsl,imx-dsp-rproc binding document Shengjiu Wang
2021-08-25  7:28 ` [PATCH v2 2/2] remoteproc: imx_dsp_rproc: add remoteproc driver for dsp on i.MX Shengjiu Wang
2021-08-31 18:01   ` Mathieu Poirier
2021-09-01  8:11     ` Shengjiu Wang
2021-09-01 15:50       ` Mathieu Poirier
2021-09-02  1:46         ` Shengjiu Wang [this message]
2021-09-01 16:22   ` Mathieu Poirier
2021-09-02  2:54     ` Shengjiu Wang
2021-09-02 15:17 ` [PATCH v2 1/2] dt-bindings: remoteproc: Add fsl,imx-dsp-rproc binding document Rob Herring
2021-09-03  7:54   ` Shengjiu Wang

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='CAA+D8APDU8H-nC2xnp+dpa8sLALAnYQFDCb2stv4=o0Hp6NOvw@mail.gmail.com' \
    --to=shengjiu.wang@gmail.com \
    --cc=bjorn.andersson@linaro.org \
    --cc=devicetree@vger.kernel.org \
    --cc=festevam@gmail.com \
    --cc=kernel@pengutronix.de \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-imx@nxp.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-remoteproc@vger.kernel.org \
    --cc=mathieu.poirier@linaro.org \
    --cc=ohad@wizery.com \
    --cc=robh+dt@kernel.org \
    --cc=s.hauer@pengutronix.de \
    --cc=shawnguo@kernel.org \
    --cc=shengjiu.wang@nxp.com \
    --subject='Re: [PATCH v2 2/2] remoteproc: imx_dsp_rproc: add remoteproc driver for dsp on i.MX' \
    /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

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