[...]
>
> >
> > > +
> > > +/* 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.
> >
> > > +
> > > +/* 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.
>
> 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
> > >
