From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id BF324C432BE for ; Wed, 1 Sep 2021 15:54:15 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 8232561027 for ; Wed, 1 Sep 2021 15:54:15 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org 8232561027 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linaro.org Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=zhSLljV/+f8jeRCFaDyb3xtQIGQnCw5eINDG8C7qc4M=; b=VzKPxjVPocuRgZ KFZ8wySuMDyol8QLxTzrIQwZW/4F8j8DcAwp3O/fg0nzFCPTf/j0AK29QteAAfQhmL6/uitIPgO7l 7pe7wysy/JP18ykFGkVgZsu5LSTdgB7Bz39aeG0BPYinc2bZAPzs8jIgwdcDjabKjspWtSFhWptOn HOHL7fC5cYUS+DVVSOvVsmSg0e1Omuyq15ZUUH/Qn4tAzocppA2erINwTjEdxtpStpfYx16fVgjUH 2GQ8gM6vG5sEMtaXzvOhjKmrZ/ujw0lXmuW7P8PPYyYZGU/ULJwFk7IYkjaHKrAPqSMX3kHUVJExU ryTuwvpSWQpgOoOhjF0g==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1mLSW0-006QOa-OL; Wed, 01 Sep 2021 15:51:04 +0000 Received: from mail-pj1-x1030.google.com ([2607:f8b0:4864:20::1030]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1mLSVt-006QO5-W4 for linux-arm-kernel@lists.infradead.org; Wed, 01 Sep 2021 15:51:02 +0000 Received: by mail-pj1-x1030.google.com with SMTP id j1so2167585pjv.3 for ; Wed, 01 Sep 2021 08:50:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=qNB0tKe+YPZM3TdHiuuATVpTDN8fjKcxkbtEtf3WGZs=; b=D2VIIJH1pwcPd9oP8vDlWrHD7vywgcZDjqNwZHeE1fAtg4vzhJj6cWQRlSBtSkFJdk Ntq8DQXQgrmAeoxqmvrKRAvFQ7YduPAWOMeyjgN5U2Sc33oXmnvovWMi2mghrQqtvjDz X3RFYvT5u/rElfqxXmSsUjyK7+uCTNQgId67dl18kVbrcIlTLnFrDn7KdK/2FygGgmxy WHux9o83IwIT4sKzdRL7wrRS2/R+2vdHx+1Rv4ha8BCgWJ1lScOlu/Q8Xha7RAc78B6I QwOxrvvbhJhU8XPTmrNZw0Oyqb+j4zBgjCRDgUk83ItXwXxPXaiP0vwgvV7xPNdMkAz/ NVEw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=qNB0tKe+YPZM3TdHiuuATVpTDN8fjKcxkbtEtf3WGZs=; b=SA02KBj8T5VqFLlKt8ui3xpT9JRHL2f+6UXQl+rHAC0+TjDQ/BKdoBFA9+W67jERVL LGxNctnZrmnq6BuHm/4JU27aIHhmPDu6bftYLwGkB2TdjENrE16NjzFXHcParoIe7jX6 Yys8GAnV+MOxiXe2NprNwdZMlw5hKWiFx0vkrUC8g4duObWKrog0RgBkJUGqrRyxMGqN majltSTXPiWWwoPRCmFrIhCPSB+hqVIoTmQCr/cjKGBZgig8wO9tnttdftmluPQ5P2+G +BNTuqzYwZIoyNG0KpdW3Hgabt17dK2dQ1xXGJpm01CvnnP4SM9YgcQdCv9+YjwnV5QA XnXQ== X-Gm-Message-State: AOAM531XXF9L8P9I5iI/qeQUhFafsHjV/7EzNPc6ftdBbIWexW3Ejmv1 +PAKVoMt4CCDyaYl5v1CKnzJGA== X-Google-Smtp-Source: ABdhPJxkyySCUKlwAICIssEef5Y7Lhbgx2xQ/jE8094p4LyKSF+NE/+JoJJe4ABG4D4FQZbcMydWCg== X-Received: by 2002:a17:902:a586:b0:132:6a9c:f8d9 with SMTP id az6-20020a170902a58600b001326a9cf8d9mr128494plb.3.1630511455840; Wed, 01 Sep 2021 08:50:55 -0700 (PDT) Received: from p14s (S0106889e681aac74.cg.shawcable.net. [68.147.0.187]) by smtp.gmail.com with ESMTPSA id n41sm382751pfv.43.2021.09.01.08.50.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 01 Sep 2021 08:50:54 -0700 (PDT) Date: Wed, 1 Sep 2021 09:50:51 -0600 From: Mathieu Poirier To: Shengjiu Wang Cc: Shengjiu Wang , Ohad Ben Cohen , bjorn.andersson@linaro.org, Rob Herring , Shawn Guo , Sascha Hauer , Sascha Hauer , Fabio Estevam , NXP Linux Team , linux-remoteproc@vger.kernel.org, "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" , "moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE" , linux-kernel Subject: Re: [PATCH v2 2/2] remoteproc: imx_dsp_rproc: add remoteproc driver for dsp on i.MX Message-ID: <20210901155051.GA1033512@p14s> References: <1629876516-16513-1-git-send-email-shengjiu.wang@nxp.com> <1629876516-16513-2-git-send-email-shengjiu.wang@nxp.com> <20210831180156.GA981305@p14s> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20210901_085058_100991_A3485118 X-CRM114-Status: GOOD ( 39.58 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org [...] > > > > > > + > > > +/* 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 "); > > > -- > > > 2.17.1 > > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel