On Wed, Sep 1, 2021 at 11:50 PM Mathieu Poirier
<mathieu.poirier@xxxxxxxxxx> 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@xxxxxxx>");
> > > > --
> > > > 2.17.1
> > > >
