Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <1629876516-16513-1-git-send-email-shengjiu.wang@nxp.com>
 <1629876516-16513-2-git-send-email-shengjiu.wang@nxp.com> <20210831180156.GA981305@p14s>
 <CAA+D8AO8hGoQEngq-7f7H9oFwQzoxNkCcEA5fpr5ifuVuEkyxQ@mail.gmail.com> <20210901155051.GA1033512@p14s>
In-Reply-To: <20210901155051.GA1033512@p14s>
From:   Shengjiu Wang <shengjiu.wang@gmail.com>
Date:   Thu, 2 Sep 2021 09:46:04 +0800
Message-ID: <CAA+D8APDU8H-nC2xnp+dpa8sLALAnYQFDCb2stv4=o0Hp6NOvw@mail.gmail.com>
Subject: Re: [PATCH v2 2/2] remoteproc: imx_dsp_rproc: add remoteproc driver
 for dsp on i.MX
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>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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