On Wed, Sep 1, 2021 at 11:50 PM Mathieu Poirier
[off-list ref] wrote:

[...]

quoted

quoted

quoted

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

quoted

quoted

quoted

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

quoted

quoted

quoted

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

[...]

quoted

quoted

quoted

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

quoted

Sorry for lack of comments, I will add more in next version.

That will be much appreciated.

quoted

Again, Thanks your time for reviewing this patch.

More comments to come in a minute...

quoted

Best regards
Wang shengjiu

quoted

quoted

+     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 [off-list ref]");
--
2.17.1