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 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 90C52C433FE for ; Thu, 29 Sep 2022 23:24:34 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229522AbiI2XYd (ORCPT ); Thu, 29 Sep 2022 19:24:33 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51494 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229437AbiI2XYc (ORCPT ); Thu, 29 Sep 2022 19:24:32 -0400 Received: from mail-ej1-x62f.google.com (mail-ej1-x62f.google.com [IPv6:2a00:1450:4864:20::62f]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 0F6E213D865 for ; Thu, 29 Sep 2022 16:24:28 -0700 (PDT) Received: by mail-ej1-x62f.google.com with SMTP id lc7so5841416ejb.0 for ; Thu, 29 Sep 2022 16:24:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=paWDF9qEhQ/RAJTTlp1AiMs155DuVvDnadMu1zQFjD8=; b=GiOvoLK9vTgHI+OQp3NKWA9RpgH5YYwZxGsdVakeFuKwaiXT9kPp6+aHpQw81/Yz5e efOkS96KFaRuSobskSi476Uen9in4Wa8UX+IM8mNHiyhszgTFx/B+9i8VANIGAxa/6bO X4WfC9pWHmqg87pwkxbV7Uk2MAyUT0rjphWVHISsGxMqOys3Yp68qt+Qyx52Zy/1qAHS jNHaSxVo16rvu7BGR5sCpJ7FSG+Di271gZje1FQXBlL4DNxGwzwKy8UAG2Uj7hlU1Ze0 NAkjvr1Sq8sQbIlRktF2QXkWl2xf04Lf4Vk3pDYtk2EA0Y2jsKdi1qyrP2qt02fnjtke LqiQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=paWDF9qEhQ/RAJTTlp1AiMs155DuVvDnadMu1zQFjD8=; b=uIDGx7zodkTS63O9Fb92Wxh2U531CoKKFp3g7rdm0xpi3ovvnB1kHv9yEleo/3TWUT 1LO30sDqP/+3qe3mCN4ZATb+Ml9s9N4aZL8metELmfPWU7NHhUkynOkcgP0BBUu3MoMP MNqJnB+kKRKmc9xY7eCHB7WSwUyDvWMKb/2X/LIGb2q87+2noxjJ9XyPhbw5afU97jfy cNyfzxkYurMjFNK/1MS242F3hhHtRDgsIZ1KH30hFombpMbFF8Z3/OmzxqlYxjF9fRy9 +2G5SOt/BVLNmdYBSHohwbZB9NEqTjmh76/aOwQsm8g2EeAiYFDQmFpmF5Joj8pc4p/f NCnA== X-Gm-Message-State: ACrzQf30zeHJ97ERXcg0+DUfcKlxlKMGj74Xkq9/zPC0e4uPZBBW9Vgs USd8te+XYYxKJrKoc+QBB5gT3HJWGTk2S1RY3OmQ1w== X-Google-Smtp-Source: AMsMyM7vFGbQcpBkP4hVHjOTU931gKkPqfhNNZcFKL89EhXUD82qb2ybkuDXHBk8hzVJB/CI0Q3StbEnbp+nwlmGqd0= X-Received: by 2002:a17:907:78d:b0:740:33e1:998 with SMTP id xd13-20020a170907078d00b0074033e10998mr4623963ejb.162.1664493866522; Thu, 29 Sep 2022 16:24:26 -0700 (PDT) MIME-Version: 1.0 References: <20220824011023.1493050-1-peng.fan@oss.nxp.com> <20220824011023.1493050-4-peng.fan@oss.nxp.com> <20220926232127.GB2817947@p14s> <20220927175649.GB2883698@p14s> <64c6bdc2-583d-a2d0-f8b8-c4487f8a4d97@oss.nxp.com> <65b7224d-d3eb-4513-d733-ec781864fb7b@oss.nxp.com> <20220928173054.GC2990524@p14s> <484445ad-d2e4-c73d-aa87-85f0bbd73440@oss.nxp.com> In-Reply-To: <484445ad-d2e4-c73d-aa87-85f0bbd73440@oss.nxp.com> From: Mathieu Poirier Date: Thu, 29 Sep 2022 17:24:14 -0600 Message-ID: Subject: Re: [PATCH V5 3/6] remoteproc: imx_rproc: support attaching to i.MX8QXP M4 To: Peng Fan Cc: Peng Fan , "robh+dt@kernel.org" , "krzysztof.kozlowski+dt@linaro.org" , "shawnguo@kernel.org" , "s.hauer@pengutronix.de" , "kernel@pengutronix.de" , "festevam@gmail.com" , dl-linux-imx , "linux-remoteproc@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , Bjorn Andersson Content-Type: text/plain; charset="UTF-8" Precedence: bulk List-ID: X-Mailing-List: linux-remoteproc@vger.kernel.org On Thu, 29 Sept 2022 at 17:17, Peng Fan wrote: > > > > On 9/29/2022 10:58 PM, Mathieu Poirier wrote: > > On Wed, 28 Sept 2022 at 21:52, Peng Fan wrote: > >> > >>> Subject: Re: [PATCH V5 3/6] remoteproc: imx_rproc: support attaching to > >>> i.MX8QXP M4 > >>> > >>> On Wed, Sep 28, 2022 at 06:01:45PM +0800, Peng Fan wrote: > >>>> Hi Mathieu, > >>>> > >>>> On 9/28/2022 3:39 PM, Peng Fan wrote: > >>>>> > >>>>> > >>>>> On 9/28/2022 1:56 AM, Mathieu Poirier wrote: > >>>>>> On Tue, Sep 27, 2022 at 02:48:02AM +0000, Peng Fan wrote: > >>>>>>> Hi Mathieu, > >>>>>>> > >>>>>>> Thanks for reviewing this patchset. > >>>>>>>> Subject: Re: [PATCH V5 3/6] remoteproc: imx_rproc: support > >>>>>>>> attaching to i.MX8QXP M4 > >>>>>>>> > >>>>>>>> On Wed, Aug 24, 2022 at 09:10:20AM +0800, Peng Fan (OSS) wrote: > >>>>>>>>> From: Peng Fan > >>>>>>>>> > >>>>>>>>> When M4 is kicked by SCFW, M4 runs in its own hardware > >>>>>>>>> partition, Linux could only do IPC with M4, it could not > >>>>>>>>> start, stop, update image. > >>>>>>>>> > >>>>>>>>> We disable recovery reboot when M4 is managed by SCFW, > >>>>>>>>> because remoteproc core still not support M4 auto-recovery > >>>>>>>>> without loading image. > >>>>>>>>> > >>>>>>>>> Signed-off-by: Peng Fan > >>>>>>>>> --- > >>>>>>>>> drivers/remoteproc/imx_rproc.c | 108 > >>>>>>>>> ++++++++++++++++++++++++++++++++- > >>>>>>>>> 1 file changed, 107 insertions(+), 1 deletion(-) > >>>>>>>>> > >>>>>>>>> diff --git a/drivers/remoteproc/imx_rproc.c > >>>>>>>>> b/drivers/remoteproc/imx_rproc.c index > >>>>>>>>> 7cc4fd207e2d..bcba74e90020 > >>>>>>>>> 100644 > >>>>>>>>> --- a/drivers/remoteproc/imx_rproc.c > >>>>>>>>> +++ b/drivers/remoteproc/imx_rproc.c > >>>>>>>>> @@ -6,6 +6,7 @@ > >>>>>>>>> #include > >>>>>>>>> #include > >>>>>>>>> #include > >>>>>>>>> +#include > >>>>>>>>> #include > >>>>>>>>> #include > >>>>>>>>> #include @@ -59,6 +60,8 @@ > >>>>>>>>> #define IMX_SIP_RPROC_STARTED 0x01 > >>>>>>>>> #define IMX_SIP_RPROC_STOP 0x02 > >>>>>>>>> > >>>>>>>>> +#define IMX_SC_IRQ_GROUP_REBOOTED 5 > >>>>>>>>> + > >>>>>>>>> /** > >>>>>>>>> * struct imx_rproc_mem - slim internal memory structure > >>>>>>>>> * @cpu_addr: MPU virtual address of the memory region @@ > >>>>>>>>> -89,6 > >>>>>>>>> +92,10 @@ struct imx_rproc { > >>>>>>>>> struct work_struct rproc_work; > >>>>>>>>> struct workqueue_struct *workqueue; > >>>>>>>>> void __iomem *rsc_table; > >>>>>>>>> + struct imx_sc_ipc *ipc_handle; > >>>>>>>>> + struct notifier_block rproc_nb; > >>>>>>>>> + u32 rproc_pt; /* partition id */ > >>>>>>>>> + u32 rsrc_id; /* resource id */ > >>>>>>>>> }; > >>>>>>>>> > >>>>>>>>> static const struct imx_rproc_att imx_rproc_att_imx93[] = > >>>>>>>>> { @@ -117,6 > >>>>>>>>> +124,18 @@ static const struct imx_rproc_att > >>>>>>>>> +imx_rproc_att_imx93[] = { > >>>>>>>>> { 0xD0000000, 0xa0000000, 0x10000000, 0 }, }; > >>>>>>>>> > >>>>>>>>> +static const struct imx_rproc_att imx_rproc_att_imx8qxp[] = > >>>>>>>>> +{ > >>>>>>>>> + { 0x08000000, 0x08000000, 0x10000000, 0 }, > >>>>>>>>> + /* TCML/U */ > >>>>>>>>> + { 0x1FFE0000, 0x34FE0000, 0x00040000, ATT_OWN | > >>>>>>>>> +ATT_IOMEM }, > >>>>>>>>> + /* OCRAM(Low 96KB) */ > >>>>>>>>> + { 0x21000000, 0x00100000, 0x00018000, 0 }, > >>>>>>>>> + /* OCRAM */ > >>>>>>>>> + { 0x21100000, 0x00100000, 0x00040000, 0 }, > >>>>>>>>> + /* DDR (Data) */ > >>>>>>>>> + { 0x80000000, 0x80000000, 0x60000000, 0 }, }; > >>>>>>>>> + > >>>>>>>>> static const struct imx_rproc_att imx_rproc_att_imx8mn[] = > >>>>>>>>> { > >>>>>>>>> /* dev addr , sys addr , size , flags */ > >>>>>>>>> /* ITCM */ > >>>>>>>>> @@ -255,6 +274,12 @@ static const struct imx_rproc_dcfg > >>>>>>>> imx_rproc_cfg_imx8mq = { > >>>>>>>>> .method = IMX_RPROC_MMIO, > >>>>>>>>> }; > >>>>>>>>> > >>>>>>>>> +static const struct imx_rproc_dcfg imx_rproc_cfg_imx8qxp = > >>>>>>>>> +{ > >>>>>>>>> + .att = imx_rproc_att_imx8qxp, > >>>>>>>>> + .att_size = ARRAY_SIZE(imx_rproc_att_imx8qxp), > >>>>>>>>> + .method = IMX_RPROC_SCU_API, }; > >>>>>>>>> + > >>>>>>>>> static const struct imx_rproc_dcfg imx_rproc_cfg_imx8ulp = > >>>>>>>>> { > >>>>>>>>> .att = imx_rproc_att_imx8ulp, > >>>>>>>>> .att_size = ARRAY_SIZE(imx_rproc_att_imx8ulp), > >>>>>>>>> @@ -680,6 +705,37 @@ static void imx_rproc_free_mbox(struct > >>>>>>>>> rproc > >>>>>>>> *rproc) > >>>>>>>>> mbox_free_channel(priv->rx_ch); > >>>>>>>>> } > >>>>>>>>> > >>>>>>>>> +static void imx_rproc_put_scu(struct rproc *rproc) { > >>>>>>>>> + struct imx_rproc *priv = rproc->priv; > >>>>>>>>> + const struct imx_rproc_dcfg *dcfg = priv->dcfg; > >>>>>>>>> + > >>>>>>>>> + if (dcfg->method != IMX_RPROC_SCU_API) > >>>>>>>>> + return; > >>>>>>>>> + > >>>>>>>>> + if (!imx_sc_rm_is_resource_owned(priv->ipc_handle, > >>>>>>>>> priv->rsrc_id)) > >>>>>>>>> + return; > >>>>>>>>> + > >>>>>>>>> + imx_scu_irq_group_enable(IMX_SC_IRQ_GROUP_REBOOTED, > >>>>>>>> BIT(priv->rproc_pt), false); > >>>>>>>>> + imx_scu_irq_unregister_notifier(&priv->rproc_nb); > >>>>>>>>> +} > >>>>>>>>> + > >>>>>>>>> +static int imx_rproc_partition_notify(struct notifier_block > >>>>>>>>> +*nb, > >>>>>>>>> + unsigned long event, void *group) { > >>>>>>>>> + struct imx_rproc *priv = container_of(nb, struct > >>>>>>>>> +imx_rproc, rproc_nb); > >>>>>>>>> + > >>>>>>>>> + /* Ignore other irqs */ > >>>>>>>>> + if (!((event & BIT(priv->rproc_pt)) && (*(u8 *)group == > >>>>>>>> IMX_SC_IRQ_GROUP_REBOOTED))) > >>>>>>>>> + return 0; > >>>>>>>>> + > >>>>>>>>> + rproc_report_crash(priv->rproc, RPROC_WATCHDOG); > >>>>>>>>> + > >>>>>>>>> + pr_info("Partition%d reset!\n", priv->rproc_pt); > >>>>>>>>> + > >>>>>>>>> + return 0; > >>>>>>>>> +} > >>>>>>>>> + > >>>>>>>>> static int imx_rproc_detect_mode(struct imx_rproc *priv) > >>>>>>>>> { > >>>>>>>>> struct regmap_config config = { .name = "imx-rproc" }; > >>>>>>>>> @@ -689,6 > >>>>>>>>> +745,7 @@ static int imx_rproc_detect_mode(struct imx_rproc > >>>>>>>>> +*priv) > >>>>>>>>> struct arm_smccc_res res; > >>>>>>>>> int ret; > >>>>>>>>> u32 val; > >>>>>>>>> + u8 pt; > >>>>>>>>> > >>>>>>>>> switch (dcfg->method) { > >>>>>>>>> case IMX_RPROC_NONE: > >>>>>>>>> @@ -699,6 +756,51 @@ static int imx_rproc_detect_mode(struct > >>>>>>>> imx_rproc *priv) > >>>>>>>>> if (res.a0) > >>>>>>>>> priv->rproc->state = RPROC_DETACHED; > >>>>>>>>> return 0; > >>>>>>>>> + case IMX_RPROC_SCU_API: > >>>>>>>>> + ret = imx_scu_get_handle(&priv->ipc_handle); > >>>>>>>>> + if (ret) > >>>>>>>>> + return ret; > >>>>>>>>> + ret = of_property_read_u32(dev->of_node, > >>>>>>>>> +"fsl,resource-id", > >>>>>>>> &priv->rsrc_id); > >>>>>>>>> + if (ret) { > >>>>>>>>> + dev_err(dev, "No fsl,resource-id property\n"); > >>>>>>>>> + return ret; > >>>>>>>>> + } > >>>>>>>>> + > >>>>>>>>> + /* > >>>>>>>>> + * If Mcore resource is not owned by Acore > >>>>>>>>> +partition, It is > >>>>>>>> kicked by ROM, > >>>>>>>>> + * and Linux could only do IPC with Mcore and nothing else. > >>>>>>>>> + */ > >>>>>>>>> + if (imx_sc_rm_is_resource_owned(priv->ipc_handle, > >>>>>>>>> +priv- > >>>>>>>>> rsrc_id)) > >>>>>>>>> + return 0; > >>>>>>>> > >>>>>>>> If imx_sc_rm_is_resource_owned() return '1' than the remote > >>>>>>>> processor is under Linux's control and what follows below is > >>>>>>>> not needed. > >>>>>>>> That is also > >>>>>>>> coherent with the comment in [1]. > >>>>>>> > >>>>>>> Case 1: If M4 is owned by Linux, here directly return 0. > >>>>>>> Case 2: If M4 is not owned by Linux, the following code after > >>>>>>> this line will set state as RPROC_DETACHED. > >>>>>> > >>>>>> I understand that part. > >>>>>> > >>>>>>> > >>>>>>> Patch 3/6(this patch) is only to support case 2. > >>>>>>> Patch 4/6 is to support case 1. > >>>>>>> > >>>>>> > >>>>>> Let's leave the subsequent patches alone for now. > >>>>>> > >>>>>>>> > >>>>>>>> That is in contrast with what is happening in > >>>>>>>> imx_rproc_put_scu(). There, if the remote processor is _not_ > >>>>>>>> owned by Linux than the condition returns without calling > >>>>>>>> imx_scu_irq_group_enable() and > >>>>>>>> imx_scu_irq_unregister_notifier(). That seems to be a bug. > >>>>>>> > >>>>>>> No. The two functions only needed when M4 is in a separate > >>>>>>> hardware partition. > >>>>>>> > >>>>>>> The scu irq is only needed when M4 is out of linux control and > >>>>>>> need some notification such as M4 is reset by SCU(System Control > >>> Unit). > >>>>>>> That linux got > >>>>>>> notification that M4 is reset by SCU. > >>>>>> > >>>>>> I also understand that part. > >>>>>> > >>>>>> What I am underlining here is that when the M4 is independent, > >>>>>> function > >>>>>> imx_scu_irq_register_notifier() and imx_scu_irq_group_enable() are > >>>>>> called but their cleanup equivalent are not called in > >>>>>> imx_rproc_put_scu() because of the '!' > >>>>>> in the if() statement. > >>>>> > >>>>> you are right, this is bug in my side. It should be as below based > >>>>> on patch 3/6. > >>>>> > >>>>> diff --git a/drivers/remoteproc/imx_rproc.c > >>>>> b/drivers/remoteproc/imx_rproc.c index bcba74e90020..a56aecae00c6 > >>>>> 100644 > >>>>> --- a/drivers/remoteproc/imx_rproc.c > >>>>> +++ b/drivers/remoteproc/imx_rproc.c > >>>>> @@ -713,7 +713,7 @@ static void imx_rproc_put_scu(struct rproc > >>>>> *rproc) > >>>>> if (dcfg->method != IMX_RPROC_SCU_API) > >>>>> return; > >>>>> > >>>>> - if (!imx_sc_rm_is_resource_owned(priv->ipc_handle, > >>>>> priv->rsrc_id)) > >>>>> + if (imx_sc_rm_is_resource_owned(priv->ipc_handle, > >>>>> +priv->rsrc_id)) > >>> > >>> Indeed, which raises questions about how this patchset was tested. And it > >>> is not the first time we touch base on that. > >> > >> Patch 4/6 has this change, anyway I should not mix patch 3,4 here. > >> > >>> > >>>>> return; > >>>>> > >>>>> Thanks for detailed reviewing. > >>>> > >>>> If you are fine with this change, I could send out V6. Anyway, I'll > >>>> wait to see if you have other comments in this patchset. > >>>> > >>> > >>> I am out of time for this patchset and as such will not provide more > >>> comments on this revision. > >> > >> Sure. Thanks for your time. Since you have applied the attach recovery > >> part, next version will enable this feature in imx rproc. > >> > > > > I would rather not. Please introduce support for the recovery in > > another patchset, once this one has been merged. > > I pushed the button too early, sorry. V6 was out yesterday, but the new > patch 7/7 is an incremetenl patch to enable attach recovery. > Right, I noticed that after replying to you. > You could ignore patch 7/7 in V6, I will reply their to note. patch > [1-6]/7 is to achieve same as V5 patchset. > > If you need to send V7 which just drop patch 7/7 in V6, please let me know. It is not a problem if all the new functionality is in a patch on its own. At least the mental picture I have of the first 6 patches doesn't change. No need to send a new revision. > > Thanks, > Peng. > > > > >> Thanks, > >> Peng. > >> > >>> > >>>> Thanks, > >>>> Peng. > >>>> > >>>>> > >>>>> Thanks, > >>>>> Peng. > >>>>> > >>>>>> > >>>>>>> > >>>>>>>> > >>>>>>>> There is also a problem in patch 4/6 associated to that. > >>>>>>> > >>>>>>> If the upper explanation eliminate your concern, "a problem in > >>>>>>> patch 4/6" should not be a problem. > >>>>>>> > >>>>>>> When M4 is owned by Linux, Linux need handle the power domain. > >>>>>>> If M4 is not owned > >>>>>>> by Linux, SCU firmware will handle the power domain, and Linux > >>>>>>> has no permission to touch that. > >>>>>>> > >>>>>>> Thanks > >>>>>>> Peng > >>>>>>> > >>>>>>>> > >>>>>>>> Thanks, > >>>>>>>> Mathieu > >>>>>>>> > >>>>>>>> > >>>>>>>> [1]. > >>>>>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2 > >>>>>>>> F%2Felixir > >>>>>>>> .bootlin.com%2Flinux%2Fv6.0- > >>>>>>>> > >>> rc7%2Fsource%2Fdrivers%2Ffirmware%2Fimx%2Frm.c%23L24&data= > >>>>>>>> 0 > >>>>>>>> > >>> 5%7C01%7Cpeng.fan%40nxp.com%7Cbe679e9a409a48b834b908daa015d92 > >>>>>>>> > >>> c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637998312946913 > >>>>>>>> > >>> 710%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2lu > >>>>>>>> > >>> MzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata= > >>>>>>>> > >>> JDRvoDGGgEiSmbhj3410V2DNxamZbDmMS0U2GvBnI74%3D&reserved > >>>>>>>> =0 > >>>>>>>> > >>>>>>>>> + > >>>>>>>>> + priv->rproc->state = RPROC_DETACHED; > >>>>>>>>> + priv->rproc->recovery_disabled = true; > >>>>>>>>> + > >>>>>>>>> + /* Get partition id and enable irq in SCFW */ > >>>>>>>>> + ret = > >>>>>>>>> +imx_sc_rm_get_resource_owner(priv->ipc_handle, > >>>>>>>> priv->rsrc_id, &pt); > >>>>>>>>> + if (ret) { > >>>>>>>>> + dev_err(dev, "not able to get resource > >>>>>>>>> +owner\n"); > >>>>>>>>> + return ret; > >>>>>>>>> + } > >>>>>>>>> + > >>>>>>>>> + priv->rproc_pt = pt; > >>>>>>>>> + priv->rproc_nb.notifier_call = > >>>>>>>>> +imx_rproc_partition_notify; > >>>>>>>>> + > >>>>>>>>> + ret = > >>>>>>>>> +imx_scu_irq_register_notifier(&priv->rproc_nb); > >>>>>>>>> + if (ret) { > >>>>>>>>> + dev_warn(dev, "register scu notifier > >>>>>>>>> +failed.\n"); > >>>>>>>>> + return ret; > >>>>>>>>> + } > >>>>>>>>> + > >>>>>>>>> + ret = > >>>>>>>> imx_scu_irq_group_enable(IMX_SC_IRQ_GROUP_REBOOTED, > >>> BIT(priv- > >>>>>>>>> rproc_pt), > >>>>>>>>> + true); > >>>>>>>>> + if (ret) { > >>>>>>>>> + > >>>>>>>>> +imx_scu_irq_unregister_notifier(&priv->rproc_nb); > >>>>>>>>> + dev_warn(dev, "Enable irq failed.\n"); > >>>>>>>>> + return ret; > >>>>>>>>> + } > >>>>>>>>> + > >>>>>>>>> + return 0; > >>>>>>>>> default: > >>>>>>>>> break; > >>>>>>>>> } > >>>>>>>>> @@ -803,7 +905,7 @@ static int imx_rproc_probe(struct > >>>>>>>>> platform_device > >>>>>>>>> *pdev) > >>>>>>>>> > >>>>>>>>> ret = imx_rproc_clk_enable(priv); > >>>>>>>>> if (ret) > >>>>>>>>> - goto err_put_mbox; > >>>>>>>>> + goto err_put_scu; > >>>>>>>>> > >>>>>>>>> INIT_WORK(&priv->rproc_work, imx_rproc_vq_work); > >>>>>>>>> > >>>>>>>>> @@ -820,6 +922,8 @@ static int imx_rproc_probe(struct > >>>>>>>>> platform_device > >>>>>>>>> *pdev) > >>>>>>>>> > >>>>>>>>> err_put_clk: > >>>>>>>>> clk_disable_unprepare(priv->clk); > >>>>>>>>> +err_put_scu: > >>>>>>>>> + imx_rproc_put_scu(rproc); > >>>>>>>>> err_put_mbox: > >>>>>>>>> imx_rproc_free_mbox(rproc); > >>>>>>>>> err_put_wkq: > >>>>>>>>> @@ -837,6 +941,7 @@ static int imx_rproc_remove(struct > >>>>>>>> platform_device > >>>>>>>>> *pdev) > >>>>>>>>> > >>>>>>>>> clk_disable_unprepare(priv->clk); > >>>>>>>>> rproc_del(rproc); > >>>>>>>>> + imx_rproc_put_scu(rproc); > >>>>>>>>> imx_rproc_free_mbox(rproc); > >>>>>>>>> destroy_workqueue(priv->workqueue); > >>>>>>>>> rproc_free(rproc); > >>>>>>>>> @@ -852,6 +957,7 @@ static const struct of_device_id > >>>>>>>> imx_rproc_of_match[] = { > >>>>>>>>> { .compatible = "fsl,imx8mm-cm4", .data = > >>>>>>>> &imx_rproc_cfg_imx8mq }, > >>>>>>>>> { .compatible = "fsl,imx8mn-cm7", .data = > >>>>>>>> &imx_rproc_cfg_imx8mn }, > >>>>>>>>> { .compatible = "fsl,imx8mp-cm7", .data = > >>>>>>>> &imx_rproc_cfg_imx8mn }, > >>>>>>>>> + { .compatible = "fsl,imx8qxp-cm4", .data = > >>>>>>>> &imx_rproc_cfg_imx8qxp }, > >>>>>>>>> { .compatible = "fsl,imx8ulp-cm33", .data = > >>>>>>>> &imx_rproc_cfg_imx8ulp }, > >>>>>>>>> { .compatible = "fsl,imx93-cm33", .data = > >>>>>>>>> &imx_rproc_cfg_imx93 }, > >>>>>>>>> {}, > >>>>>>>>> -- > >>>>>>>>> 2.37.1 > >>>>>>>>> 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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id 25604C433F5 for ; Thu, 29 Sep 2022 23:25:40 +0000 (UTC) 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:Cc:To:Subject:Message-ID:Date:From: In-Reply-To:References:MIME-Version:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=b5JsDvvDJ8si0bY+ORtTXoHgxJJJg5PcldpAqnXaAis=; b=uxftleHRWy1J8l jUg1IdcwvVLudf8IcstlSjbTRvP62U7yXFbqH0Ypy+sAmeBQqFXSxIHDre6LJyYvuCbelOlnQJKhE uuD6mM1yEOJ4J/0mL8DpNopiSlvF7uVTQOYamYRk+vIa/8uZ0M8nxm36PTOm2JmCPjOVISKzTdD0g YtyYuKfZfwd5cv3mEQhiTVmuaFTCEPx7C+h7gSRcLg2swG6D7YhVi5oC80rwjzsmlRArdkaxtW++Z NmCecQXQYqu8vqSXH+pn6m68h1UjwwoK34W1nTxEeywPMU2SYh+At2iTeCCVQaY7pJUP4Z4buoFDr 03Ang87neQ94BIpiryXA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oe2tO-005vUR-Qo; Thu, 29 Sep 2022 23:24:34 +0000 Received: from mail-ej1-x62b.google.com ([2a00:1450:4864:20::62b]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oe2tK-005vSy-DZ for linux-arm-kernel@lists.infradead.org; Thu, 29 Sep 2022 23:24:32 +0000 Received: by mail-ej1-x62b.google.com with SMTP id r18so5748028eja.11 for ; Thu, 29 Sep 2022 16:24:27 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=paWDF9qEhQ/RAJTTlp1AiMs155DuVvDnadMu1zQFjD8=; b=GiOvoLK9vTgHI+OQp3NKWA9RpgH5YYwZxGsdVakeFuKwaiXT9kPp6+aHpQw81/Yz5e efOkS96KFaRuSobskSi476Uen9in4Wa8UX+IM8mNHiyhszgTFx/B+9i8VANIGAxa/6bO X4WfC9pWHmqg87pwkxbV7Uk2MAyUT0rjphWVHISsGxMqOys3Yp68qt+Qyx52Zy/1qAHS jNHaSxVo16rvu7BGR5sCpJ7FSG+Di271gZje1FQXBlL4DNxGwzwKy8UAG2Uj7hlU1Ze0 NAkjvr1Sq8sQbIlRktF2QXkWl2xf04Lf4Vk3pDYtk2EA0Y2jsKdi1qyrP2qt02fnjtke LqiQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=paWDF9qEhQ/RAJTTlp1AiMs155DuVvDnadMu1zQFjD8=; b=uGTUdaNI+KqjcPcxizf6yuLCE85grXBPNZLCL141qz+gQOXCnaWapP2TSC3m8gaHc7 yiL5CsqBcQnd9IEDi9dfOBFawdLosNOts7LXl9C1o8opjf4pz5qycIlvd0waJwAlDiS1 LsXcf3AtURei64UTwgwFbrMN1kPw0yDpS76PA1UeqoMK7SiTnGofA+BLeU6d0zibh6lC cbrJrtOp4sIpIwNebeB+mkU/GWWMe2d/ZHhnQyA46ck6I5jWIyQUOLtcXpou2nzWjNYF XI+X7+JiWviuyMYVxfXF4Br5RMzrK3eHshE20Li/3K8DR8yxtWsHzmQEBsvqY0cuJDap nG8w== X-Gm-Message-State: ACrzQf1R6hqf/BESLK79GOXVnU0UKeoDxKeuGnkGnmlndHaX6229PMpq DMT0DhxHW8XwR/S0K6PUJskR8GL+8XDb5Xy8lPdtUg== X-Google-Smtp-Source: AMsMyM7vFGbQcpBkP4hVHjOTU931gKkPqfhNNZcFKL89EhXUD82qb2ybkuDXHBk8hzVJB/CI0Q3StbEnbp+nwlmGqd0= X-Received: by 2002:a17:907:78d:b0:740:33e1:998 with SMTP id xd13-20020a170907078d00b0074033e10998mr4623963ejb.162.1664493866522; Thu, 29 Sep 2022 16:24:26 -0700 (PDT) MIME-Version: 1.0 References: <20220824011023.1493050-1-peng.fan@oss.nxp.com> <20220824011023.1493050-4-peng.fan@oss.nxp.com> <20220926232127.GB2817947@p14s> <20220927175649.GB2883698@p14s> <64c6bdc2-583d-a2d0-f8b8-c4487f8a4d97@oss.nxp.com> <65b7224d-d3eb-4513-d733-ec781864fb7b@oss.nxp.com> <20220928173054.GC2990524@p14s> <484445ad-d2e4-c73d-aa87-85f0bbd73440@oss.nxp.com> In-Reply-To: <484445ad-d2e4-c73d-aa87-85f0bbd73440@oss.nxp.com> From: Mathieu Poirier Date: Thu, 29 Sep 2022 17:24:14 -0600 Message-ID: Subject: Re: [PATCH V5 3/6] remoteproc: imx_rproc: support attaching to i.MX8QXP M4 To: Peng Fan Cc: Peng Fan , "robh+dt@kernel.org" , "krzysztof.kozlowski+dt@linaro.org" , "shawnguo@kernel.org" , "s.hauer@pengutronix.de" , "kernel@pengutronix.de" , "festevam@gmail.com" , dl-linux-imx , "linux-remoteproc@vger.kernel.org" , "devicetree@vger.kernel.org" , "linux-arm-kernel@lists.infradead.org" , "linux-kernel@vger.kernel.org" , Bjorn Andersson X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220929_162430_492366_833D7E26 X-CRM114-Status: GOOD ( 55.19 ) 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 On Thu, 29 Sept 2022 at 17:17, Peng Fan wrote: > > > > On 9/29/2022 10:58 PM, Mathieu Poirier wrote: > > On Wed, 28 Sept 2022 at 21:52, Peng Fan wrote: > >> > >>> Subject: Re: [PATCH V5 3/6] remoteproc: imx_rproc: support attaching to > >>> i.MX8QXP M4 > >>> > >>> On Wed, Sep 28, 2022 at 06:01:45PM +0800, Peng Fan wrote: > >>>> Hi Mathieu, > >>>> > >>>> On 9/28/2022 3:39 PM, Peng Fan wrote: > >>>>> > >>>>> > >>>>> On 9/28/2022 1:56 AM, Mathieu Poirier wrote: > >>>>>> On Tue, Sep 27, 2022 at 02:48:02AM +0000, Peng Fan wrote: > >>>>>>> Hi Mathieu, > >>>>>>> > >>>>>>> Thanks for reviewing this patchset. > >>>>>>>> Subject: Re: [PATCH V5 3/6] remoteproc: imx_rproc: support > >>>>>>>> attaching to i.MX8QXP M4 > >>>>>>>> > >>>>>>>> On Wed, Aug 24, 2022 at 09:10:20AM +0800, Peng Fan (OSS) wrote: > >>>>>>>>> From: Peng Fan > >>>>>>>>> > >>>>>>>>> When M4 is kicked by SCFW, M4 runs in its own hardware > >>>>>>>>> partition, Linux could only do IPC with M4, it could not > >>>>>>>>> start, stop, update image. > >>>>>>>>> > >>>>>>>>> We disable recovery reboot when M4 is managed by SCFW, > >>>>>>>>> because remoteproc core still not support M4 auto-recovery > >>>>>>>>> without loading image. > >>>>>>>>> > >>>>>>>>> Signed-off-by: Peng Fan > >>>>>>>>> --- > >>>>>>>>> drivers/remoteproc/imx_rproc.c | 108 > >>>>>>>>> ++++++++++++++++++++++++++++++++- > >>>>>>>>> 1 file changed, 107 insertions(+), 1 deletion(-) > >>>>>>>>> > >>>>>>>>> diff --git a/drivers/remoteproc/imx_rproc.c > >>>>>>>>> b/drivers/remoteproc/imx_rproc.c index > >>>>>>>>> 7cc4fd207e2d..bcba74e90020 > >>>>>>>>> 100644 > >>>>>>>>> --- a/drivers/remoteproc/imx_rproc.c > >>>>>>>>> +++ b/drivers/remoteproc/imx_rproc.c > >>>>>>>>> @@ -6,6 +6,7 @@ > >>>>>>>>> #include > >>>>>>>>> #include > >>>>>>>>> #include > >>>>>>>>> +#include > >>>>>>>>> #include > >>>>>>>>> #include > >>>>>>>>> #include @@ -59,6 +60,8 @@ > >>>>>>>>> #define IMX_SIP_RPROC_STARTED 0x01 > >>>>>>>>> #define IMX_SIP_RPROC_STOP 0x02 > >>>>>>>>> > >>>>>>>>> +#define IMX_SC_IRQ_GROUP_REBOOTED 5 > >>>>>>>>> + > >>>>>>>>> /** > >>>>>>>>> * struct imx_rproc_mem - slim internal memory structure > >>>>>>>>> * @cpu_addr: MPU virtual address of the memory region @@ > >>>>>>>>> -89,6 > >>>>>>>>> +92,10 @@ struct imx_rproc { > >>>>>>>>> struct work_struct rproc_work; > >>>>>>>>> struct workqueue_struct *workqueue; > >>>>>>>>> void __iomem *rsc_table; > >>>>>>>>> + struct imx_sc_ipc *ipc_handle; > >>>>>>>>> + struct notifier_block rproc_nb; > >>>>>>>>> + u32 rproc_pt; /* partition id */ > >>>>>>>>> + u32 rsrc_id; /* resource id */ > >>>>>>>>> }; > >>>>>>>>> > >>>>>>>>> static const struct imx_rproc_att imx_rproc_att_imx93[] = > >>>>>>>>> { @@ -117,6 > >>>>>>>>> +124,18 @@ static const struct imx_rproc_att > >>>>>>>>> +imx_rproc_att_imx93[] = { > >>>>>>>>> { 0xD0000000, 0xa0000000, 0x10000000, 0 }, }; > >>>>>>>>> > >>>>>>>>> +static const struct imx_rproc_att imx_rproc_att_imx8qxp[] = > >>>>>>>>> +{ > >>>>>>>>> + { 0x08000000, 0x08000000, 0x10000000, 0 }, > >>>>>>>>> + /* TCML/U */ > >>>>>>>>> + { 0x1FFE0000, 0x34FE0000, 0x00040000, ATT_OWN | > >>>>>>>>> +ATT_IOMEM }, > >>>>>>>>> + /* OCRAM(Low 96KB) */ > >>>>>>>>> + { 0x21000000, 0x00100000, 0x00018000, 0 }, > >>>>>>>>> + /* OCRAM */ > >>>>>>>>> + { 0x21100000, 0x00100000, 0x00040000, 0 }, > >>>>>>>>> + /* DDR (Data) */ > >>>>>>>>> + { 0x80000000, 0x80000000, 0x60000000, 0 }, }; > >>>>>>>>> + > >>>>>>>>> static const struct imx_rproc_att imx_rproc_att_imx8mn[] = > >>>>>>>>> { > >>>>>>>>> /* dev addr , sys addr , size , flags */ > >>>>>>>>> /* ITCM */ > >>>>>>>>> @@ -255,6 +274,12 @@ static const struct imx_rproc_dcfg > >>>>>>>> imx_rproc_cfg_imx8mq = { > >>>>>>>>> .method = IMX_RPROC_MMIO, > >>>>>>>>> }; > >>>>>>>>> > >>>>>>>>> +static const struct imx_rproc_dcfg imx_rproc_cfg_imx8qxp = > >>>>>>>>> +{ > >>>>>>>>> + .att = imx_rproc_att_imx8qxp, > >>>>>>>>> + .att_size = ARRAY_SIZE(imx_rproc_att_imx8qxp), > >>>>>>>>> + .method = IMX_RPROC_SCU_API, }; > >>>>>>>>> + > >>>>>>>>> static const struct imx_rproc_dcfg imx_rproc_cfg_imx8ulp = > >>>>>>>>> { > >>>>>>>>> .att = imx_rproc_att_imx8ulp, > >>>>>>>>> .att_size = ARRAY_SIZE(imx_rproc_att_imx8ulp), > >>>>>>>>> @@ -680,6 +705,37 @@ static void imx_rproc_free_mbox(struct > >>>>>>>>> rproc > >>>>>>>> *rproc) > >>>>>>>>> mbox_free_channel(priv->rx_ch); > >>>>>>>>> } > >>>>>>>>> > >>>>>>>>> +static void imx_rproc_put_scu(struct rproc *rproc) { > >>>>>>>>> + struct imx_rproc *priv = rproc->priv; > >>>>>>>>> + const struct imx_rproc_dcfg *dcfg = priv->dcfg; > >>>>>>>>> + > >>>>>>>>> + if (dcfg->method != IMX_RPROC_SCU_API) > >>>>>>>>> + return; > >>>>>>>>> + > >>>>>>>>> + if (!imx_sc_rm_is_resource_owned(priv->ipc_handle, > >>>>>>>>> priv->rsrc_id)) > >>>>>>>>> + return; > >>>>>>>>> + > >>>>>>>>> + imx_scu_irq_group_enable(IMX_SC_IRQ_GROUP_REBOOTED, > >>>>>>>> BIT(priv->rproc_pt), false); > >>>>>>>>> + imx_scu_irq_unregister_notifier(&priv->rproc_nb); > >>>>>>>>> +} > >>>>>>>>> + > >>>>>>>>> +static int imx_rproc_partition_notify(struct notifier_block > >>>>>>>>> +*nb, > >>>>>>>>> + unsigned long event, void *group) { > >>>>>>>>> + struct imx_rproc *priv = container_of(nb, struct > >>>>>>>>> +imx_rproc, rproc_nb); > >>>>>>>>> + > >>>>>>>>> + /* Ignore other irqs */ > >>>>>>>>> + if (!((event & BIT(priv->rproc_pt)) && (*(u8 *)group == > >>>>>>>> IMX_SC_IRQ_GROUP_REBOOTED))) > >>>>>>>>> + return 0; > >>>>>>>>> + > >>>>>>>>> + rproc_report_crash(priv->rproc, RPROC_WATCHDOG); > >>>>>>>>> + > >>>>>>>>> + pr_info("Partition%d reset!\n", priv->rproc_pt); > >>>>>>>>> + > >>>>>>>>> + return 0; > >>>>>>>>> +} > >>>>>>>>> + > >>>>>>>>> static int imx_rproc_detect_mode(struct imx_rproc *priv) > >>>>>>>>> { > >>>>>>>>> struct regmap_config config = { .name = "imx-rproc" }; > >>>>>>>>> @@ -689,6 > >>>>>>>>> +745,7 @@ static int imx_rproc_detect_mode(struct imx_rproc > >>>>>>>>> +*priv) > >>>>>>>>> struct arm_smccc_res res; > >>>>>>>>> int ret; > >>>>>>>>> u32 val; > >>>>>>>>> + u8 pt; > >>>>>>>>> > >>>>>>>>> switch (dcfg->method) { > >>>>>>>>> case IMX_RPROC_NONE: > >>>>>>>>> @@ -699,6 +756,51 @@ static int imx_rproc_detect_mode(struct > >>>>>>>> imx_rproc *priv) > >>>>>>>>> if (res.a0) > >>>>>>>>> priv->rproc->state = RPROC_DETACHED; > >>>>>>>>> return 0; > >>>>>>>>> + case IMX_RPROC_SCU_API: > >>>>>>>>> + ret = imx_scu_get_handle(&priv->ipc_handle); > >>>>>>>>> + if (ret) > >>>>>>>>> + return ret; > >>>>>>>>> + ret = of_property_read_u32(dev->of_node, > >>>>>>>>> +"fsl,resource-id", > >>>>>>>> &priv->rsrc_id); > >>>>>>>>> + if (ret) { > >>>>>>>>> + dev_err(dev, "No fsl,resource-id property\n"); > >>>>>>>>> + return ret; > >>>>>>>>> + } > >>>>>>>>> + > >>>>>>>>> + /* > >>>>>>>>> + * If Mcore resource is not owned by Acore > >>>>>>>>> +partition, It is > >>>>>>>> kicked by ROM, > >>>>>>>>> + * and Linux could only do IPC with Mcore and nothing else. > >>>>>>>>> + */ > >>>>>>>>> + if (imx_sc_rm_is_resource_owned(priv->ipc_handle, > >>>>>>>>> +priv- > >>>>>>>>> rsrc_id)) > >>>>>>>>> + return 0; > >>>>>>>> > >>>>>>>> If imx_sc_rm_is_resource_owned() return '1' than the remote > >>>>>>>> processor is under Linux's control and what follows below is > >>>>>>>> not needed. > >>>>>>>> That is also > >>>>>>>> coherent with the comment in [1]. > >>>>>>> > >>>>>>> Case 1: If M4 is owned by Linux, here directly return 0. > >>>>>>> Case 2: If M4 is not owned by Linux, the following code after > >>>>>>> this line will set state as RPROC_DETACHED. > >>>>>> > >>>>>> I understand that part. > >>>>>> > >>>>>>> > >>>>>>> Patch 3/6(this patch) is only to support case 2. > >>>>>>> Patch 4/6 is to support case 1. > >>>>>>> > >>>>>> > >>>>>> Let's leave the subsequent patches alone for now. > >>>>>> > >>>>>>>> > >>>>>>>> That is in contrast with what is happening in > >>>>>>>> imx_rproc_put_scu(). There, if the remote processor is _not_ > >>>>>>>> owned by Linux than the condition returns without calling > >>>>>>>> imx_scu_irq_group_enable() and > >>>>>>>> imx_scu_irq_unregister_notifier(). That seems to be a bug. > >>>>>>> > >>>>>>> No. The two functions only needed when M4 is in a separate > >>>>>>> hardware partition. > >>>>>>> > >>>>>>> The scu irq is only needed when M4 is out of linux control and > >>>>>>> need some notification such as M4 is reset by SCU(System Control > >>> Unit). > >>>>>>> That linux got > >>>>>>> notification that M4 is reset by SCU. > >>>>>> > >>>>>> I also understand that part. > >>>>>> > >>>>>> What I am underlining here is that when the M4 is independent, > >>>>>> function > >>>>>> imx_scu_irq_register_notifier() and imx_scu_irq_group_enable() are > >>>>>> called but their cleanup equivalent are not called in > >>>>>> imx_rproc_put_scu() because of the '!' > >>>>>> in the if() statement. > >>>>> > >>>>> you are right, this is bug in my side. It should be as below based > >>>>> on patch 3/6. > >>>>> > >>>>> diff --git a/drivers/remoteproc/imx_rproc.c > >>>>> b/drivers/remoteproc/imx_rproc.c index bcba74e90020..a56aecae00c6 > >>>>> 100644 > >>>>> --- a/drivers/remoteproc/imx_rproc.c > >>>>> +++ b/drivers/remoteproc/imx_rproc.c > >>>>> @@ -713,7 +713,7 @@ static void imx_rproc_put_scu(struct rproc > >>>>> *rproc) > >>>>> if (dcfg->method != IMX_RPROC_SCU_API) > >>>>> return; > >>>>> > >>>>> - if (!imx_sc_rm_is_resource_owned(priv->ipc_handle, > >>>>> priv->rsrc_id)) > >>>>> + if (imx_sc_rm_is_resource_owned(priv->ipc_handle, > >>>>> +priv->rsrc_id)) > >>> > >>> Indeed, which raises questions about how this patchset was tested. And it > >>> is not the first time we touch base on that. > >> > >> Patch 4/6 has this change, anyway I should not mix patch 3,4 here. > >> > >>> > >>>>> return; > >>>>> > >>>>> Thanks for detailed reviewing. > >>>> > >>>> If you are fine with this change, I could send out V6. Anyway, I'll > >>>> wait to see if you have other comments in this patchset. > >>>> > >>> > >>> I am out of time for this patchset and as such will not provide more > >>> comments on this revision. > >> > >> Sure. Thanks for your time. Since you have applied the attach recovery > >> part, next version will enable this feature in imx rproc. > >> > > > > I would rather not. Please introduce support for the recovery in > > another patchset, once this one has been merged. > > I pushed the button too early, sorry. V6 was out yesterday, but the new > patch 7/7 is an incremetenl patch to enable attach recovery. > Right, I noticed that after replying to you. > You could ignore patch 7/7 in V6, I will reply their to note. patch > [1-6]/7 is to achieve same as V5 patchset. > > If you need to send V7 which just drop patch 7/7 in V6, please let me know. It is not a problem if all the new functionality is in a patch on its own. At least the mental picture I have of the first 6 patches doesn't change. No need to send a new revision. > > Thanks, > Peng. > > > > >> Thanks, > >> Peng. > >> > >>> > >>>> Thanks, > >>>> Peng. > >>>> > >>>>> > >>>>> Thanks, > >>>>> Peng. > >>>>> > >>>>>> > >>>>>>> > >>>>>>>> > >>>>>>>> There is also a problem in patch 4/6 associated to that. > >>>>>>> > >>>>>>> If the upper explanation eliminate your concern, "a problem in > >>>>>>> patch 4/6" should not be a problem. > >>>>>>> > >>>>>>> When M4 is owned by Linux, Linux need handle the power domain. > >>>>>>> If M4 is not owned > >>>>>>> by Linux, SCU firmware will handle the power domain, and Linux > >>>>>>> has no permission to touch that. > >>>>>>> > >>>>>>> Thanks > >>>>>>> Peng > >>>>>>> > >>>>>>>> > >>>>>>>> Thanks, > >>>>>>>> Mathieu > >>>>>>>> > >>>>>>>> > >>>>>>>> [1]. > >>>>>>>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2 > >>>>>>>> F%2Felixir > >>>>>>>> .bootlin.com%2Flinux%2Fv6.0- > >>>>>>>> > >>> rc7%2Fsource%2Fdrivers%2Ffirmware%2Fimx%2Frm.c%23L24&data= > >>>>>>>> 0 > >>>>>>>> > >>> 5%7C01%7Cpeng.fan%40nxp.com%7Cbe679e9a409a48b834b908daa015d92 > >>>>>>>> > >>> c%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637998312946913 > >>>>>>>> > >>> 710%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2lu > >>>>>>>> > >>> MzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata= > >>>>>>>> > >>> JDRvoDGGgEiSmbhj3410V2DNxamZbDmMS0U2GvBnI74%3D&reserved > >>>>>>>> =0 > >>>>>>>> > >>>>>>>>> + > >>>>>>>>> + priv->rproc->state = RPROC_DETACHED; > >>>>>>>>> + priv->rproc->recovery_disabled = true; > >>>>>>>>> + > >>>>>>>>> + /* Get partition id and enable irq in SCFW */ > >>>>>>>>> + ret = > >>>>>>>>> +imx_sc_rm_get_resource_owner(priv->ipc_handle, > >>>>>>>> priv->rsrc_id, &pt); > >>>>>>>>> + if (ret) { > >>>>>>>>> + dev_err(dev, "not able to get resource > >>>>>>>>> +owner\n"); > >>>>>>>>> + return ret; > >>>>>>>>> + } > >>>>>>>>> + > >>>>>>>>> + priv->rproc_pt = pt; > >>>>>>>>> + priv->rproc_nb.notifier_call = > >>>>>>>>> +imx_rproc_partition_notify; > >>>>>>>>> + > >>>>>>>>> + ret = > >>>>>>>>> +imx_scu_irq_register_notifier(&priv->rproc_nb); > >>>>>>>>> + if (ret) { > >>>>>>>>> + dev_warn(dev, "register scu notifier > >>>>>>>>> +failed.\n"); > >>>>>>>>> + return ret; > >>>>>>>>> + } > >>>>>>>>> + > >>>>>>>>> + ret = > >>>>>>>> imx_scu_irq_group_enable(IMX_SC_IRQ_GROUP_REBOOTED, > >>> BIT(priv- > >>>>>>>>> rproc_pt), > >>>>>>>>> + true); > >>>>>>>>> + if (ret) { > >>>>>>>>> + > >>>>>>>>> +imx_scu_irq_unregister_notifier(&priv->rproc_nb); > >>>>>>>>> + dev_warn(dev, "Enable irq failed.\n"); > >>>>>>>>> + return ret; > >>>>>>>>> + } > >>>>>>>>> + > >>>>>>>>> + return 0; > >>>>>>>>> default: > >>>>>>>>> break; > >>>>>>>>> } > >>>>>>>>> @@ -803,7 +905,7 @@ static int imx_rproc_probe(struct > >>>>>>>>> platform_device > >>>>>>>>> *pdev) > >>>>>>>>> > >>>>>>>>> ret = imx_rproc_clk_enable(priv); > >>>>>>>>> if (ret) > >>>>>>>>> - goto err_put_mbox; > >>>>>>>>> + goto err_put_scu; > >>>>>>>>> > >>>>>>>>> INIT_WORK(&priv->rproc_work, imx_rproc_vq_work); > >>>>>>>>> > >>>>>>>>> @@ -820,6 +922,8 @@ static int imx_rproc_probe(struct > >>>>>>>>> platform_device > >>>>>>>>> *pdev) > >>>>>>>>> > >>>>>>>>> err_put_clk: > >>>>>>>>> clk_disable_unprepare(priv->clk); > >>>>>>>>> +err_put_scu: > >>>>>>>>> + imx_rproc_put_scu(rproc); > >>>>>>>>> err_put_mbox: > >>>>>>>>> imx_rproc_free_mbox(rproc); > >>>>>>>>> err_put_wkq: > >>>>>>>>> @@ -837,6 +941,7 @@ static int imx_rproc_remove(struct > >>>>>>>> platform_device > >>>>>>>>> *pdev) > >>>>>>>>> > >>>>>>>>> clk_disable_unprepare(priv->clk); > >>>>>>>>> rproc_del(rproc); > >>>>>>>>> + imx_rproc_put_scu(rproc); > >>>>>>>>> imx_rproc_free_mbox(rproc); > >>>>>>>>> destroy_workqueue(priv->workqueue); > >>>>>>>>> rproc_free(rproc); > >>>>>>>>> @@ -852,6 +957,7 @@ static const struct of_device_id > >>>>>>>> imx_rproc_of_match[] = { > >>>>>>>>> { .compatible = "fsl,imx8mm-cm4", .data = > >>>>>>>> &imx_rproc_cfg_imx8mq }, > >>>>>>>>> { .compatible = "fsl,imx8mn-cm7", .data = > >>>>>>>> &imx_rproc_cfg_imx8mn }, > >>>>>>>>> { .compatible = "fsl,imx8mp-cm7", .data = > >>>>>>>> &imx_rproc_cfg_imx8mn }, > >>>>>>>>> + { .compatible = "fsl,imx8qxp-cm4", .data = > >>>>>>>> &imx_rproc_cfg_imx8qxp }, > >>>>>>>>> { .compatible = "fsl,imx8ulp-cm33", .data = > >>>>>>>> &imx_rproc_cfg_imx8ulp }, > >>>>>>>>> { .compatible = "fsl,imx93-cm33", .data = > >>>>>>>>> &imx_rproc_cfg_imx93 }, > >>>>>>>>> {}, > >>>>>>>>> -- > >>>>>>>>> 2.37.1 > >>>>>>>>> _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel