Message ID | 20221111095313.2010815-1-sumit.garg@linaro.org |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:6687:0:0:0:0:0 with SMTP id l7csp648567wru; Fri, 11 Nov 2022 02:00:44 -0800 (PST) X-Google-Smtp-Source: AA0mqf6zSvsxj+VZbvQ2O9QtARtTJ1jI0NcjLY4/L97S1YMU+tJCDmdD8wqB/LU+j2xV8vWUb9UL X-Received: by 2002:a65:590b:0:b0:462:8605:7fe7 with SMTP id f11-20020a65590b000000b0046286057fe7mr984227pgu.445.1668160844060; Fri, 11 Nov 2022 02:00:44 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1668160844; cv=none; d=google.com; s=arc-20160816; b=YZVD7obh0Dr+2DxJMWRRHHB7XkcyUJcvszXDdEh2C0ZogxjwAeoIz79DbfSLrCwOQ2 G2syXGApcpZZI1BCP+WwTQBFZlGwqqfOt4nDRjcrXjgYFdEOu49owAq1m34SIRIFE0Wt QWDnmGurydhO8DhhTwcFQtRIh4UtCgY+CZB4PjgHz97kjqE9d+KwPBwsRTOk+aYP0KDT FvT09BYrJ/1NQ0SBm81iKqbEhutDqFoEiWoQX+EOuZ+bNK+mnSkAuUyEamQhiHSPrxoF A6R9f6fiinQirC7E5+MlYhnFw08FDfz3lX0EgrhQz8MhV7hDXAvYT0SYPWWmMQcvCpNn eKzA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=l8aDxDsNnGd9Cti6vqMoMN7YUwle0jy3PVcJaHD2rW4=; b=ln0Um/8WSPf8yep7isB98uwLE2eH5i3/rsk3G6FzP8DrgilTczeiYEY43asyqX9mzH 9SGxuJ5HsKpkPoDA7SD7K8ixlOW+Ugc/TDUIV6Fc84zH/gnqYVpxILKURRdxhwUf20yS tuW/k0o2xpvrYoqyvn9y1yKOXFaN2RnEiGCorvffzj/biTknN2fDwJ1FK5SqFVVnkE9b ygmY9Z7rblhzQh0gJSyujiZ/+De7P4RRN/AIPQTHJ9cVMMpBCAdhmZrS1Na1OxTI0OqV a2K2nAgMOLMG6IP06PA5o8rFKBOJAnTuNZDudPL0BJNU+bA+29WSvj+X44RalXCbJO8e jH/w== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=Wu1zQP8E; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id i6-20020a17090332c600b0017840d9d42esi2396414plr.582.2022.11.11.02.00.27; Fri, 11 Nov 2022 02:00:44 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@linaro.org header.s=google header.b=Wu1zQP8E; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=linaro.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233602AbiKKJx1 (ORCPT <rfc822;winker.wchi@gmail.com> + 99 others); Fri, 11 Nov 2022 04:53:27 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:37504 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233470AbiKKJxZ (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Fri, 11 Nov 2022 04:53:25 -0500 Received: from mail-pl1-x62d.google.com (mail-pl1-x62d.google.com [IPv6:2607:f8b0:4864:20::62d]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 731F2BED for <linux-kernel@vger.kernel.org>; Fri, 11 Nov 2022 01:53:24 -0800 (PST) Received: by mail-pl1-x62d.google.com with SMTP id io19so3847725plb.8 for <linux-kernel@vger.kernel.org>; Fri, 11 Nov 2022 01:53:24 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=l8aDxDsNnGd9Cti6vqMoMN7YUwle0jy3PVcJaHD2rW4=; b=Wu1zQP8EzPVqNDzRo8j8C0Xo3hhPg6iViTQGEnZepBOg9p03rjrgpYMlPw7aOlaK/q FDHsTrtL1xO2WzglqEjy87RlpbE+i/BUqCkNAdDaJ1Tr0R2uVE3Lmi+diDNQeBVolnFc Z9qkQN7QtqGhEGvREKwBJ18z5qn66ZZgDkUZL0QX9lmhFkp28DpLKtz2elrieaQf600O jK6v2uWD1h4fdJyPQr54ISGEVsIcFaZU16OSDGCkCGoXW2qGLunTeHVPeuPw+FmAjvAa LWO3SDH+6mRNli12b14lNoWw8X7JSEJOIA0lTw57OCvvX9b+UWjA9roZtxhJnWsCDUVC 4g8g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=l8aDxDsNnGd9Cti6vqMoMN7YUwle0jy3PVcJaHD2rW4=; b=Muhzq7HCQNq1xnTZfNfaMY6678bFLAs/HkEq/Bn0hEWEF2C7kaTqg0htErPVUQavv1 SW68FbyIeVV+dzex/dRa0PPnGBFTajXTr2C2HR2MgHtivW3cT9SBM8iMCChlBf5zZrgH pJVOHx6TXa7+ZkyylVJlW/PgMQZSHzdel9JDh+sCrE14TGb9xWFxBzcImkVsdpg3L3Lq NXpN58R4Fe+nR/F4dUqU5dt2IvgijMcoY+Wy6wfokN0wO4S1FC0xGD2LE7kDGMYCk1K+ wzToLf32FMz212ZWOxzMnHOUzfq+KgXjNwvc/cBZNnHSMIsVn7ejcVjy5EPbrtSvGHOC G6Ug== X-Gm-Message-State: ANoB5pnwypfUrgb0LKE+Phg3J/UMhMMwVIMKz2LAJqTk5xxXWlz+dv1p XUMdXZC2Ai6ix8V4evNOV+oRpQ== X-Received: by 2002:a17:902:8d8f:b0:179:f94a:6fda with SMTP id v15-20020a1709028d8f00b00179f94a6fdamr1778751plo.118.1668160404006; Fri, 11 Nov 2022 01:53:24 -0800 (PST) Received: from sumit-X1.. ([223.178.212.236]) by smtp.gmail.com with ESMTPSA id b14-20020a170902650e00b00177e5d83d3esm1263094plk.88.2022.11.11.01.53.20 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 11 Nov 2022 01:53:23 -0800 (PST) From: Sumit Garg <sumit.garg@linaro.org> To: linux-arm-kernel@lists.infradead.org Cc: sudeep.holla@arm.com, cristian.marussi@arm.com, Ludvig.Parsson@axis.com, jens.wiklander@linaro.org, etienne.carriere@linaro.org, vincent.guittot@linaro.org, linux-kernel@vger.kernel.org, Sumit Garg <sumit.garg@linaro.org> Subject: [PATCH] firmware: arm_scmi: Resolve dependency with TEE subsystem Date: Fri, 11 Nov 2022 15:23:13 +0530 Message-Id: <20221111095313.2010815-1-sumit.garg@linaro.org> X-Mailer: git-send-email 2.34.1 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1749193425382332648?= X-GMAIL-MSGID: =?utf-8?q?1749193425382332648?= |
Series |
firmware: arm_scmi: Resolve dependency with TEE subsystem
|
|
Commit Message
Sumit Garg
Nov. 11, 2022, 9:53 a.m. UTC
The OP-TEE SCMI transport channel is dependent on TEE subsystem to be
initialized first. But currently the Arm SCMI subsystem and TEE
subsystem are invoked on the same initcall level as subsystem_init().
It is observed that the SCMI subsystem initcall is invoked prior to TEE
subsystem initcall. This leads to unwanted error messages regarding TEE
bus is not present yet. Although, -EPROBE_DEFER tries to workaround that
problem.
Lets try to resolve inter subsystem dependency problem via shifting Arm
SCMI subsystem to subsystem_init_sync() initcall level.
Signed-off-by: Sumit Garg <sumit.garg@linaro.org>
---
drivers/firmware/arm_scmi/driver.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
Comments
On Fri, Nov 11, 2022 at 03:23:13PM +0530, Sumit Garg wrote: > The OP-TEE SCMI transport channel is dependent on TEE subsystem to be > initialized first. But currently the Arm SCMI subsystem and TEE > subsystem are invoked on the same initcall level as subsystem_init(). > > It is observed that the SCMI subsystem initcall is invoked prior to TEE > subsystem initcall. This leads to unwanted error messages regarding TEE > bus is not present yet. Although, -EPROBE_DEFER tries to workaround that > problem. > > Lets try to resolve inter subsystem dependency problem via shifting Arm > SCMI subsystem to subsystem_init_sync() initcall level. > I would avoid doing that. We already have some implicit dependency with subsys_initcall because this driver creates/registers bus and need to be done early. Now in order to solve the dependency between SCMI and TEE, both of which creates/registers bus and are at same subsys_initcall, we are relying on subsys_initcall_sync. Me and Ludvig discussed this in private and I suggested him to do something like below patch snippet. He mentioned he did post a patch on the list but I couldn't find it. For this the scmi node must be child node of OPTEE as it is providing the transport. @Ludvig, ? Regards, Sudeep -- diff --git i/drivers/tee/optee/smc_abi.c w/drivers/tee/optee/smc_abi.c index a1c1fa1a9c28..839feca0def4 100644 --- i/drivers/tee/optee/smc_abi.c +++ w/drivers/tee/optee/smc_abi.c @@ -1534,7 +1534,9 @@ static int optee_probe(struct platform_device *pdev) goto err_disable_shm_cache; pr_info("initialized driver\n"); - return 0; + + /* Populate any dependent child node(if any) */ + return devm_of_platform_populate(&pdev->dev); err_disable_shm_cache: if (!optee->rpc_param_count)
On Fri, 2022-11-11 at 14:38 +0000, Sudeep Holla wrote: > On Fri, Nov 11, 2022 at 03:23:13PM +0530, Sumit Garg wrote: > > The OP-TEE SCMI transport channel is dependent on TEE subsystem to > > be > > initialized first. But currently the Arm SCMI subsystem and TEE > > subsystem are invoked on the same initcall level as > > subsystem_init(). > > > > It is observed that the SCMI subsystem initcall is invoked prior to > > TEE > > subsystem initcall. This leads to unwanted error messages regarding > > TEE > > bus is not present yet. Although, -EPROBE_DEFER tries to workaround > > that > > problem. > > > > Lets try to resolve inter subsystem dependency problem via shifting > > Arm > > SCMI subsystem to subsystem_init_sync() initcall level. > > > > I would avoid doing that. We already have some implicit dependency > with > subsys_initcall because this driver creates/registers bus and need to > be > done early. Now in order to solve the dependency between SCMI and > TEE, > both of which creates/registers bus and are at same subsys_initcall, > we are relying on subsys_initcall_sync. > > Me and Ludvig discussed this in private and I suggested him to do > something > like below patch snippet. He mentioned he did post a patch on the > list but > I couldn't find it. For this the scmi node must be child node of > OPTEE as > it is providing the transport. > > @Ludvig, ? > > Regards, > Sudeep > > -- > diff --git i/drivers/tee/optee/smc_abi.c > w/drivers/tee/optee/smc_abi.c > index a1c1fa1a9c28..839feca0def4 100644 > --- i/drivers/tee/optee/smc_abi.c > +++ w/drivers/tee/optee/smc_abi.c > @@ -1534,7 +1534,9 @@ static int optee_probe(struct platform_device > *pdev) > goto err_disable_shm_cache; > > pr_info("initialized driver\n"); > - return 0; > + > + /* Populate any dependent child node(if any) */ > + return devm_of_platform_populate(&pdev->dev); > > err_disable_shm_cache: > if (!optee->rpc_param_count) > I have answered something similar in my submit [1]. Maybe I should have CCed you, or atleast sent you this link when I told you I made the submission. [1] https://lkml.org/lkml/2022/11/9/803 BR, Ludvig
On Fri, Nov 11, 2022 at 03:00:29PM +0000, Ludvig Pärsson wrote: > On Fri, 2022-11-11 at 14:38 +0000, Sudeep Holla wrote: > > On Fri, Nov 11, 2022 at 03:23:13PM +0530, Sumit Garg wrote: > > > The OP-TEE SCMI transport channel is dependent on TEE subsystem to > > > be > > > initialized first. But currently the Arm SCMI subsystem and TEE > > > subsystem are invoked on the same initcall level as > > > subsystem_init(). > > > > > > It is observed that the SCMI subsystem initcall is invoked prior to > > > TEE > > > subsystem initcall. This leads to unwanted error messages regarding > > > TEE > > > bus is not present yet. Although, -EPROBE_DEFER tries to workaround > > > that > > > problem. > > > > > > Lets try to resolve inter subsystem dependency problem via shifting > > > Arm > > > SCMI subsystem to subsystem_init_sync() initcall level. > > > > > > > I would avoid doing that. We already have some implicit dependency > > with > > subsys_initcall because this driver creates/registers bus and need to > > be > > done early. Now in order to solve the dependency between SCMI and > > TEE, > > both of which creates/registers bus and are at same subsys_initcall, > > we are relying on subsys_initcall_sync. > > > > Me and Ludvig discussed this in private and I suggested him to do > > something > > like below patch snippet. He mentioned he did post a patch on the > > list but > > I couldn't find it. For this the scmi node must be child node of > > OPTEE as > > it is providing the transport. > > > > @Ludvig, ? > > > > Regards, > > Sudeep > > > > -- > > diff --git i/drivers/tee/optee/smc_abi.c > > w/drivers/tee/optee/smc_abi.c > > index a1c1fa1a9c28..839feca0def4 100644 > > --- i/drivers/tee/optee/smc_abi.c > > +++ w/drivers/tee/optee/smc_abi.c > > @@ -1534,7 +1534,9 @@ static int optee_probe(struct platform_device > > *pdev) > > goto err_disable_shm_cache; > > > > pr_info("initialized driver\n"); > > - return 0; > > + > > + /* Populate any dependent child node(if any) */ > > + return devm_of_platform_populate(&pdev->dev); > > > > err_disable_shm_cache: > > if (!optee->rpc_param_count) > > > I have answered something similar in my submit [1]. Maybe I should have > CCed you, or atleast sent you this link when I told you I made the > submission. > > [1] https://lkml.org/lkml/2022/11/9/803 > Thanks for the reference.
Hi Sudeep, On Fri, 11 Nov 2022 at 20:08, Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Fri, Nov 11, 2022 at 03:23:13PM +0530, Sumit Garg wrote: > > The OP-TEE SCMI transport channel is dependent on TEE subsystem to be > > initialized first. But currently the Arm SCMI subsystem and TEE > > subsystem are invoked on the same initcall level as subsystem_init(). > > > > It is observed that the SCMI subsystem initcall is invoked prior to TEE > > subsystem initcall. This leads to unwanted error messages regarding TEE > > bus is not present yet. Although, -EPROBE_DEFER tries to workaround that > > problem. > > > > Lets try to resolve inter subsystem dependency problem via shifting Arm > > SCMI subsystem to subsystem_init_sync() initcall level. > > > > I would avoid doing that. We already have some implicit dependency with > subsys_initcall because this driver creates/registers bus and need to be > done early. Yeah but that should work fine with subsystem_init_sync() too unless you have drivers being registered on the bus at subsystem_init_sync() initcall which doesn't seem to be the case in mainline. Have a look at usage of subsystem_init_sync() elsewhere to see its similar usage to resolve dependencies among different subsystems. However, if you are too skeptical regarding this change then we can limit it to OP-TEE transport only as follows: diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index f43e52541da4..19c1222b3dfc 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -2667,7 +2667,11 @@ static int __init scmi_driver_init(void) return platform_driver_register(&scmi_driver); } +#ifdef CONFIG_ARM_SCMI_TRANSPORT_OPTEE subsys_initcall_sync(scmi_driver_init); +#else +subsys_initcall(scmi_driver_init); +#endif static void __exit scmi_driver_exit(void) { > Now in order to solve the dependency between SCMI and TEE, > both of which creates/registers bus and are at same subsys_initcall, > we are relying on subsys_initcall_sync. True. > > Me and Ludvig discussed this in private and I suggested him to do something > like below patch snippet. He mentioned he did post a patch on the list but > I couldn't find it. For this the scmi node must be child node of OPTEE as > it is providing the transport. TBH, the first thought that came to mind after looking at SCMI OP-TEE DT node was that why do we need it when those properties can be probed from SCMI pseudo TA at runtime? Maybe I am missing something as I wasn't involved in its review process. The whole idea of TEE bus evolved from the idea that if the firmware bits can be probed at runtime then we shouldn't use DT as a dumping ground for those. I hope you remember discussions around discoverable FF-A bus too. However, the change below is simply an incorrect way to fix the actual inter subsystem dependency problem via DT. How would this fix the problem in the case OP-TEE driver registers on FF-A bus? There won't be any DT node for OP-TEE in that case. -Sumit > > @Ludvig, ? > > Regards, > Sudeep > > -- > diff --git i/drivers/tee/optee/smc_abi.c w/drivers/tee/optee/smc_abi.c > index a1c1fa1a9c28..839feca0def4 100644 > --- i/drivers/tee/optee/smc_abi.c > +++ w/drivers/tee/optee/smc_abi.c > @@ -1534,7 +1534,9 @@ static int optee_probe(struct platform_device *pdev) > goto err_disable_shm_cache; > > pr_info("initialized driver\n"); > - return 0; > + > + /* Populate any dependent child node(if any) */ > + return devm_of_platform_populate(&pdev->dev); > > err_disable_shm_cache: > if (!optee->rpc_param_count) >
On Mon, Nov 14, 2022 at 12:01:32PM +0530, Sumit Garg wrote: > Hi Sudeep, > > On Fri, 11 Nov 2022 at 20:08, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Fri, Nov 11, 2022 at 03:23:13PM +0530, Sumit Garg wrote: > > > The OP-TEE SCMI transport channel is dependent on TEE subsystem to be > > > initialized first. But currently the Arm SCMI subsystem and TEE > > > subsystem are invoked on the same initcall level as subsystem_init(). > > > > > > It is observed that the SCMI subsystem initcall is invoked prior to TEE > > > subsystem initcall. This leads to unwanted error messages regarding TEE > > > bus is not present yet. Although, -EPROBE_DEFER tries to workaround that > > > problem. > > > > > > Lets try to resolve inter subsystem dependency problem via shifting Arm > > > SCMI subsystem to subsystem_init_sync() initcall level. > > > > > > > I would avoid doing that. We already have some implicit dependency with > > subsys_initcall because this driver creates/registers bus and need to be > > done early. > > Yeah but that should work fine with subsystem_init_sync() too unless > you have drivers being registered on the bus at subsystem_init_sync() > initcall which doesn't seem to be the case in mainline. Have a look at > usage of subsystem_init_sync() elsewhere to see its similar usage to > resolve dependencies among different subsystems. > > However, if you are too skeptical regarding this change then we can > limit it to OP-TEE transport only as follows: > > diff --git a/drivers/firmware/arm_scmi/driver.c > b/drivers/firmware/arm_scmi/driver.c > index f43e52541da4..19c1222b3dfc 100644 > --- a/drivers/firmware/arm_scmi/driver.c > +++ b/drivers/firmware/arm_scmi/driver.c > @@ -2667,7 +2667,11 @@ static int __init scmi_driver_init(void) > > return platform_driver_register(&scmi_driver); > } > +#ifdef CONFIG_ARM_SCMI_TRANSPORT_OPTEE > subsys_initcall_sync(scmi_driver_init); > +#else > +subsys_initcall(scmi_driver_init); > +#endif > If this is the only way to solve, I would prefer to keep it unconditional. > static void __exit scmi_driver_exit(void) > { > > > Now in order to solve the dependency between SCMI and TEE, > > both of which creates/registers bus and are at same subsys_initcall, > > we are relying on subsys_initcall_sync. > > True. > > > > > Me and Ludvig discussed this in private and I suggested him to do something > > like below patch snippet. He mentioned he did post a patch on the list but > > I couldn't find it. For this the scmi node must be child node of OPTEE as > > it is providing the transport. > > TBH, the first thought that came to mind after looking at SCMI OP-TEE > DT node was that why do we need it when those properties can be probed > from SCMI pseudo TA at runtime? Maybe I am missing something as I > wasn't involved in its review process. > I don't have internal details OPTEE and may be it could be probed. Etienne can comment on that. But we need SCMI node in general as the consumers of the SCMI are in the DT and they need to link to the provider. > The whole idea of TEE bus evolved from the idea that if the firmware > bits can be probed at runtime then we shouldn't use DT as a dumping > ground for those. I hope you remember discussions around discoverable > FF-A bus too. > Exactly this is what I thought of initially when I proposed the solution. And yes we won't even have DT node for TEE in that case, so it shouldn't be a problem. When both SCMI and TEE are present in DT and SCMI used OPTEE as a transport I see it is correct to represent them as child and parent and it can be utilised here to solved the problem with respect to the driver model without having to play around with the initcall levels which is always going to bite us back with one extra dependency. And with FF-A, TEE and SCMI all in the mix we might have that dependency already, so I really want to avoid playing with initcall levels just to solve this problem. > However, the change below is simply an incorrect way to fix the actual > inter subsystem dependency problem via DT. How would this fix the > problem in the case OP-TEE driver registers on FF-A bus? There won't > be any DT node for OP-TEE in that case. > Agreed and this is why I thought it in the first place. As I mention in this case there are no DT nodes and hence we can't use this at all. I am suggesting this only when both DT nodes are present and SCMI depends on OPTEE transport which fits the child/parent hierarchy irrespective of this solution. This is just ensuring any dependent DT nodes are populated only after OPTEE is ready. I don't see this to be an issue or see this as incorrect. Also I am not sure this initcall juggling will help if there are 3 or more at the same level, we need to rely on driver model and/or proper hierarchy in the DT node. The whole links between devices is modelled on that and I don't see that as any issue and we are not dumping any more information that it is already in DT. We are just using the correct hierarchical representation here IMO.
Hello all, On Mon, 14 Nov 2022 at 11:26, Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Mon, Nov 14, 2022 at 12:01:32PM +0530, Sumit Garg wrote: > > Hi Sudeep, > > > > On Fri, 11 Nov 2022 at 20:08, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > On Fri, Nov 11, 2022 at 03:23:13PM +0530, Sumit Garg wrote: > > > > The OP-TEE SCMI transport channel is dependent on TEE subsystem to be > > > > initialized first. But currently the Arm SCMI subsystem and TEE > > > > subsystem are invoked on the same initcall level as subsystem_init(). > > > > > > > > It is observed that the SCMI subsystem initcall is invoked prior to TEE > > > > subsystem initcall. This leads to unwanted error messages regarding TEE > > > > bus is not present yet. Although, -EPROBE_DEFER tries to workaround that > > > > problem. > > > > > > > > Lets try to resolve inter subsystem dependency problem via shifting Arm > > > > SCMI subsystem to subsystem_init_sync() initcall level. > > > > > > > > > > I would avoid doing that. We already have some implicit dependency with > > > subsys_initcall because this driver creates/registers bus and need to be > > > done early. > > > > Yeah but that should work fine with subsystem_init_sync() too unless > > you have drivers being registered on the bus at subsystem_init_sync() > > initcall which doesn't seem to be the case in mainline. Have a look at > > usage of subsystem_init_sync() elsewhere to see its similar usage to > > resolve dependencies among different subsystems. > > > > However, if you are too skeptical regarding this change then we can > > limit it to OP-TEE transport only as follows: > > > > diff --git a/drivers/firmware/arm_scmi/driver.c > > b/drivers/firmware/arm_scmi/driver.c > > index f43e52541da4..19c1222b3dfc 100644 > > --- a/drivers/firmware/arm_scmi/driver.c > > +++ b/drivers/firmware/arm_scmi/driver.c > > @@ -2667,7 +2667,11 @@ static int __init scmi_driver_init(void) > > > > return platform_driver_register(&scmi_driver); > > } > > +#ifdef CONFIG_ARM_SCMI_TRANSPORT_OPTEE > > subsys_initcall_sync(scmi_driver_init); > > +#else > > +subsys_initcall(scmi_driver_init); > > +#endif > > > > If this is the only way to solve, I would prefer to keep it unconditional. > > > static void __exit scmi_driver_exit(void) > > { > > > > > Now in order to solve the dependency between SCMI and TEE, > > > both of which creates/registers bus and are at same subsys_initcall, > > > we are relying on subsys_initcall_sync. > > > > True. > > > > > > > > Me and Ludvig discussed this in private and I suggested him to do something > > > like below patch snippet. He mentioned he did post a patch on the list but > > > I couldn't find it. For this the scmi node must be child node of OPTEE as > > > it is providing the transport. > > > > TBH, the first thought that came to mind after looking at SCMI OP-TEE > > DT node was that why do we need it when those properties can be probed > > from SCMI pseudo TA at runtime? Maybe I am missing something as I > > wasn't involved in its review process. > > > > I don't have internal details OPTEE and may be it could be probed. Etienne > can comment on that. But we need SCMI node in general as the consumers of > the SCMI are in the DT and they need to link to the provider. Indeed the SCMI OP-TEE service is currently designed to be discovered by Linux but it does not allow Linux to discover which resources are related to the exposed SCMI channels. As Sudeep said, these information are provided by the DT. Moreover, consumer devices of the SCMI services in Linux are using DT to reference the SCMI resource used, as phandles on SCMI clock provider, SCMI regulator provider etc... For the consumers, we need these DT descriptions. > > > The whole idea of TEE bus evolved from the idea that if the firmware > > bits can be probed at runtime then we shouldn't use DT as a dumping > > ground for those. I hope you remember discussions around discoverable > > FF-A bus too. > > > > Exactly this is what I thought of initially when I proposed the solution. > And yes we won't even have DT node for TEE in that case, so it shouldn't > be a problem. When both SCMI and TEE are present in DT and SCMI used OPTEE > as a transport I see it is correct to represent them as child and parent > and it can be utilised here to solved the problem with respect to the driver > model without having to play around with the initcall levels which is always > going to bite us back with one extra dependency. > > And with FF-A, TEE and SCMI all in the mix we might have that dependency > already, so I really want to avoid playing with initcall levels just to solve > this problem. Even with FFA, the optee driver still registers from module_init level (== device_init level initcall), as when using legacy OP-TE SMC ABI. SCMI firmware driver is initialized from subsys_init level hence before optee driver. So I think SCMI optee transport relies on the same dependencies whatever OP-TEE is using FFA ABI or its legacy SCM ABI. Device discovery from OP-TEE bus will always need to wait for the OP-TEE bus to be ready. This is currently archived for scmi/optee by returning -EPROBE_DEFER from scmi_optee_link_supplier() (scmi_transport_ops::link handler from scmi/optee). @Luvig, your initial issue is that driver_register() prints an error trace when one registers a driver for a bus device that is not setup, not an issue with dependencies, right? Regards, Etienne > > > However, the change below is simply an incorrect way to fix the actual > > inter subsystem dependency problem via DT. How would this fix the > > problem in the case OP-TEE driver registers on FF-A bus? There won't > > be any DT node for OP-TEE in that case. > > > > Agreed and this is why I thought it in the first place. As I mention in this > case there are no DT nodes and hence we can't use this at all. I am suggesting > this only when both DT nodes are present and SCMI depends on OPTEE transport > which fits the child/parent hierarchy irrespective of this solution. This > is just ensuring any dependent DT nodes are populated only after OPTEE is > ready. I don't see this to be an issue or see this as incorrect. > > > Also I am not sure this initcall juggling will help if there are 3 or more > at the same level, we need to rely on driver model and/or proper hierarchy > in the DT node. The whole links between devices is modelled on that and > I don't see that as any issue and we are not dumping any more information > that it is already in DT. We are just using the correct hierarchical > representation here IMO. > > -- > Regards, > Sudeep
On Mon, 2022-11-14 at 12:29 +0100, Etienne Carriere wrote: > Hello all, > > On Mon, 14 Nov 2022 at 11:26, Sudeep Holla <sudeep.holla@arm.com> > wrote: > > > > On Mon, Nov 14, 2022 at 12:01:32PM +0530, Sumit Garg wrote: > > > Hi Sudeep, > > > > > > On Fri, 11 Nov 2022 at 20:08, Sudeep Holla <sudeep.holla@arm.com> > > > wrote: > > > > > > > > On Fri, Nov 11, 2022 at 03:23:13PM +0530, Sumit Garg wrote: > > > > > The OP-TEE SCMI transport channel is dependent on TEE > > > > > subsystem to be > > > > > initialized first. But currently the Arm SCMI subsystem and > > > > > TEE > > > > > subsystem are invoked on the same initcall level as > > > > > subsystem_init(). > > > > > > > > > > It is observed that the SCMI subsystem initcall is invoked > > > > > prior to TEE > > > > > subsystem initcall. This leads to unwanted error messages > > > > > regarding TEE > > > > > bus is not present yet. Although, -EPROBE_DEFER tries to > > > > > workaround that > > > > > problem. > > > > > > > > > > Lets try to resolve inter subsystem dependency problem via > > > > > shifting Arm > > > > > SCMI subsystem to subsystem_init_sync() initcall level. > > > > > > > > > > > > > I would avoid doing that. We already have some implicit > > > > dependency with > > > > subsys_initcall because this driver creates/registers bus and > > > > need to be > > > > done early. > > > > > > Yeah but that should work fine with subsystem_init_sync() too > > > unless > > > you have drivers being registered on the bus at > > > subsystem_init_sync() > > > initcall which doesn't seem to be the case in mainline. Have a > > > look at > > > usage of subsystem_init_sync() elsewhere to see its similar usage > > > to > > > resolve dependencies among different subsystems. > > > > > > However, if you are too skeptical regarding this change then we > > > can > > > limit it to OP-TEE transport only as follows: > > > > > > diff --git a/drivers/firmware/arm_scmi/driver.c > > > b/drivers/firmware/arm_scmi/driver.c > > > index f43e52541da4..19c1222b3dfc 100644 > > > --- a/drivers/firmware/arm_scmi/driver.c > > > +++ b/drivers/firmware/arm_scmi/driver.c > > > @@ -2667,7 +2667,11 @@ static int __init scmi_driver_init(void) > > > > > > return platform_driver_register(&scmi_driver); > > > } > > > +#ifdef CONFIG_ARM_SCMI_TRANSPORT_OPTEE > > > subsys_initcall_sync(scmi_driver_init); > > > +#else > > > +subsys_initcall(scmi_driver_init); > > > +#endif > > > > > > > If this is the only way to solve, I would prefer to keep it > > unconditional. > > > > > static void __exit scmi_driver_exit(void) > > > { > > > > > > > Now in order to solve the dependency between SCMI and TEE, > > > > both of which creates/registers bus and are at same > > > > subsys_initcall, > > > > we are relying on subsys_initcall_sync. > > > > > > True. > > > > > > > > > > > Me and Ludvig discussed this in private and I suggested him to > > > > do something > > > > like below patch snippet. He mentioned he did post a patch on > > > > the list but > > > > I couldn't find it. For this the scmi node must be child node > > > > of OPTEE as > > > > it is providing the transport. > > > > > > TBH, the first thought that came to mind after looking at SCMI > > > OP-TEE > > > DT node was that why do we need it when those properties can be > > > probed > > > from SCMI pseudo TA at runtime? Maybe I am missing something as I > > > wasn't involved in its review process. > > > > > > > I don't have internal details OPTEE and may be it could be probed. > > Etienne > > can comment on that. But we need SCMI node in general as the > > consumers of > > the SCMI are in the DT and they need to link to the provider. > > Indeed the SCMI OP-TEE service is currently designed to be discovered > by Linux but it does not allow Linux to discover which resources are > related to the exposed SCMI channels. As Sudeep said, these > information are provided by the DT. Moreover, consumer devices of the > SCMI services in Linux are using DT to reference the SCMI resource > used, as phandles on SCMI clock provider, SCMI regulator provider > etc... For the consumers, we need these DT descriptions. > > > > > > > The whole idea of TEE bus evolved from the idea that if the > > > firmware > > > bits can be probed at runtime then we shouldn't use DT as a > > > dumping > > > ground for those. I hope you remember discussions around > > > discoverable > > > FF-A bus too. > > > > > > > Exactly this is what I thought of initially when I proposed the > > solution. > > And yes we won't even have DT node for TEE in that case, so it > > shouldn't > > be a problem. When both SCMI and TEE are present in DT and SCMI > > used OPTEE > > as a transport I see it is correct to represent them as child and > > parent > > and it can be utilised here to solved the problem with respect to > > the driver > > model without having to play around with the initcall levels which > > is always > > going to bite us back with one extra dependency. > > > > And with FF-A, TEE and SCMI all in the mix we might have that > > dependency > > already, so I really want to avoid playing with initcall levels > > just to solve > > this problem. > > Even with FFA, the optee driver still registers from module_init > level > (== device_init level initcall), as when using legacy OP-TE SMC ABI. > SCMI firmware driver is initialized from subsys_init level hence > before optee driver. So I think SCMI optee transport relies on the > same dependencies whatever OP-TEE is using FFA ABI or its legacy SCM > ABI. > > Device discovery from OP-TEE bus will always need to wait for the > OP-TEE bus to be ready. > This is currently archived for scmi/optee by returning -EPROBE_DEFER > from scmi_optee_link_supplier() (scmi_transport_ops::link handler > from scmi/optee). > @Luvig, your initial issue is that driver_register() prints an error > trace when one registers a driver for a bus device that is not setup, > not an issue with dependencies, right? > > Regards, > Etienne > Yes, exactly. We don't want to call driver_register() before the bus is initialized. I guess you can say that there should be a dependency here, but there isn't one. BR, Ludvig > > > > > However, the change below is simply an incorrect way to fix the > > > actual > > > inter subsystem dependency problem via DT. How would this fix the > > > problem in the case OP-TEE driver registers on FF-A bus? There > > > won't > > > be any DT node for OP-TEE in that case. > > > > > > > Agreed and this is why I thought it in the first place. As I > > mention in this > > case there are no DT nodes and hence we can't use this at all. I am > > suggesting > > this only when both DT nodes are present and SCMI depends on OPTEE > > transport > > which fits the child/parent hierarchy irrespective of this > > solution. This > > is just ensuring any dependent DT nodes are populated only after > > OPTEE is > > ready. I don't see this to be an issue or see this as incorrect. > > > > > > Also I am not sure this initcall juggling will help if there are 3 > > or more > > at the same level, we need to rely on driver model and/or proper > > hierarchy > > in the DT node. The whole links between devices is modelled on that > > and > > I don't see that as any issue and we are not dumping any more > > information > > that it is already in DT. We are just using the correct > > hierarchical > > representation here IMO. > > > > -- > > Regards, > > Sudeep
On Mon, 14 Nov 2022 at 17:00, Etienne Carriere <etienne.carriere@linaro.org> wrote: > > Hello all, > > On Mon, 14 Nov 2022 at 11:26, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Mon, Nov 14, 2022 at 12:01:32PM +0530, Sumit Garg wrote: > > > Hi Sudeep, > > > > > > On Fri, 11 Nov 2022 at 20:08, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > > > > > On Fri, Nov 11, 2022 at 03:23:13PM +0530, Sumit Garg wrote: > > > > > The OP-TEE SCMI transport channel is dependent on TEE subsystem to be > > > > > initialized first. But currently the Arm SCMI subsystem and TEE > > > > > subsystem are invoked on the same initcall level as subsystem_init(). > > > > > > > > > > It is observed that the SCMI subsystem initcall is invoked prior to TEE > > > > > subsystem initcall. This leads to unwanted error messages regarding TEE > > > > > bus is not present yet. Although, -EPROBE_DEFER tries to workaround that > > > > > problem. > > > > > > > > > > Lets try to resolve inter subsystem dependency problem via shifting Arm > > > > > SCMI subsystem to subsystem_init_sync() initcall level. > > > > > > > > > > > > > I would avoid doing that. We already have some implicit dependency with > > > > subsys_initcall because this driver creates/registers bus and need to be > > > > done early. > > > > > > Yeah but that should work fine with subsystem_init_sync() too unless > > > you have drivers being registered on the bus at subsystem_init_sync() > > > initcall which doesn't seem to be the case in mainline. Have a look at > > > usage of subsystem_init_sync() elsewhere to see its similar usage to > > > resolve dependencies among different subsystems. > > > > > > However, if you are too skeptical regarding this change then we can > > > limit it to OP-TEE transport only as follows: > > > > > > diff --git a/drivers/firmware/arm_scmi/driver.c > > > b/drivers/firmware/arm_scmi/driver.c > > > index f43e52541da4..19c1222b3dfc 100644 > > > --- a/drivers/firmware/arm_scmi/driver.c > > > +++ b/drivers/firmware/arm_scmi/driver.c > > > @@ -2667,7 +2667,11 @@ static int __init scmi_driver_init(void) > > > > > > return platform_driver_register(&scmi_driver); > > > } > > > +#ifdef CONFIG_ARM_SCMI_TRANSPORT_OPTEE > > > subsys_initcall_sync(scmi_driver_init); > > > +#else > > > +subsys_initcall(scmi_driver_init); > > > +#endif > > > > > > > If this is the only way to solve, I would prefer to keep it unconditional. > > > > > static void __exit scmi_driver_exit(void) > > > { > > > > > > > Now in order to solve the dependency between SCMI and TEE, > > > > both of which creates/registers bus and are at same subsys_initcall, > > > > we are relying on subsys_initcall_sync. > > > > > > True. > > > > > > > > > > > Me and Ludvig discussed this in private and I suggested him to do something > > > > like below patch snippet. He mentioned he did post a patch on the list but > > > > I couldn't find it. For this the scmi node must be child node of OPTEE as > > > > it is providing the transport. > > > > > > TBH, the first thought that came to mind after looking at SCMI OP-TEE > > > DT node was that why do we need it when those properties can be probed > > > from SCMI pseudo TA at runtime? Maybe I am missing something as I > > > wasn't involved in its review process. > > > > > > > I don't have internal details OPTEE and may be it could be probed. Etienne > > can comment on that. But we need SCMI node in general as the consumers of > > the SCMI are in the DT and they need to link to the provider. > > Indeed the SCMI OP-TEE service is currently designed to be discovered > by Linux but it does not allow Linux to discover which resources are > related to the exposed SCMI channels. As Sudeep said, these > information are provided by the DT. Moreover, consumer devices of the > SCMI services in Linux are using DT to reference the SCMI resource > used, as phandles on SCMI clock provider, SCMI regulator provider > etc... For the consumers, we need these DT descriptions. > So it's the DT legacy we want to maintain for the SCMI subsystem even if the underlying transport is discoverable? This simply undermines the benefits of a discoverable TEE bus over the non-discoverable platform bus. Also, the reluctance to carry forward SCMI subsystem DT legacy has resulted in misrepresentation of SCMI OP-TEE transport as follows: First represented as a platform device via DT (compatible = "linaro,scmi-optee";) and then Migrated to being a TEE bus device (UUID: 0xa8cfe406, 0xd4f5, 0x4a2e, 0x9f, 0x8d, 0xa2, 0x5d, 0xc7, 0x54, 0xc0, 0x99) This misrepresentation is the reason for the indirect DT hack that Sudeep suggested to fix the error message while registering a driver on TEE bus. Is it not possible for SCMI subsystem to evolve and support underlying transport on a discoverable TEE bus? > > > > > > The whole idea of TEE bus evolved from the idea that if the firmware > > > bits can be probed at runtime then we shouldn't use DT as a dumping > > > ground for those. I hope you remember discussions around discoverable > > > FF-A bus too. > > > > > > > Exactly this is what I thought of initially when I proposed the solution. > > And yes we won't even have DT node for TEE in that case, so it shouldn't > > be a problem. Why? The SCMI OP-TEE transport driver will still be registered on TEE bus via subsys_initcall() prior to TEE bus being registered via subsys_initcall(). > > When both SCMI and TEE are present in DT and SCMI used OPTEE > > as a transport I see it is correct to represent them as child and parent No it's not the correct representation. Devices on the TEE bus have nothing to do with DT. The OP-TEE node in DT represents a particular TEE implementation whereas there are other ways to represent OP-TEE implementation as a device on FF-A bus. Your suggested change only works due to misrepresentation of SCMI OP-TEE transport as highlighted above while it won't fix the case with OP-TEE device on FF-A bus. > > and it can be utilised here to solved the problem with respect to the driver > > model without having to play around with the initcall levels which is always > > going to bite us back with one extra dependency. > > As I mentioned in the patch description, it's an inter-subsystem dependency. It has to be solved via initcall levels. I am unsure which extra dependency you have in mind but the one mentioned below doesn't fall into that category. Have you looked at other places within the kernel for usage of subsys_initcall_sync()? > > And with FF-A, TEE and SCMI all in the mix we might have that dependency > > already, so I really want to avoid playing with initcall levels just to solve > > this problem. There isn't a three level dependency here. The only dependency we have to solve is that the SCMI OP-TEE transport shouldn't register on TEE bus prior to registration of TEE bus. And switching SCMI subsystem to subsys_initcall_sync() fixes that dependency even with OP-TEE FF-A ABI. > > Even with FFA, the optee driver still registers from module_init level > (== device_init level initcall), as when using legacy OP-TE SMC ABI. > SCMI firmware driver is initialized from subsys_init level hence > before optee driver. So I think SCMI optee transport relies on the > same dependencies whatever OP-TEE is using FFA ABI or its legacy SCM > ABI. > I guess here you are confusing the TEE subsystem driver invoked at subsys_initcall() versus OP-TEE driver invoked at module_init(). The TEE bus is registered by the TEE subsystem driver rather than the OP-TEE driver. So there is *no* dependency among SCMI firmware driver and OP-TEE driver but rather the dependency is with TEE subsystem driver instead. > Device discovery from OP-TEE bus will always need to wait for the > OP-TEE bus to be ready. It isn't an OP-TEE bus but rather a TEE bus with underlying TEE implementations like OP-TEE etc. registering their corresponding devices. -Sumit
On Thu, Nov 17, 2022 at 03:11:32PM +0530, Sumit Garg wrote: > > So it's the DT legacy we want to maintain for the SCMI subsystem even > if the underlying transport is discoverable? Not sure what exactly you mean here by "DT legacy". The SCMI consumers needing SCMI info from DT is not legacy for sure. The creation of SCMI platform device from DT for TEE can be considered as legacy. > This simply undermines the benefits of a discoverable TEE bus over the > non-discoverable platform bus. Also, the reluctance to carry forward SCMI > subsystem DT legacy has resulted in misrepresentation of SCMI OP-TEE > transport as follows: > Agreed on using DT for creation of SCMI platform device. > First represented as a platform device via DT (compatible = > "linaro,scmi-optee";) and then > Migrated to being a TEE bus device (UUID: 0xa8cfe406, 0xd4f5, > 0x4a2e, 0x9f, 0x8d, 0xa2, 0x5d, 0xc7, 0x54, 0xc0, 0x99) > I am fine with SCMI platform device not created by TEE/OPTEE by of_populate_device(). It can create one by matching the service(IIUC). > This misrepresentation is the reason for the indirect DT hack that > Sudeep suggested to fix the error message while registering a driver > on TEE bus. > Again, sorry. We are mixing up things here I think. The DT representation of SCMI node as child of TEE/OPTEE is *not* hack. I agree creating the platform device from DT w/o checking the service is hack. Let me know if we are on the same page here. > Is it not possible for SCMI subsystem to evolve and support underlying > transport on a discoverable TEE bus? > It must, and as I mentioned we need to create SCMI platform device only once the SCMI service is discovered. For that reason, the SCMI node in that case must not be in /firmware but inside /firmware/optee. Now it is up to OPTEE or even FF-A to create the devices for the service it provides when ready. > > Why? The SCMI OP-TEE transport driver will still be registered on TEE > bus via subsys_initcall() prior to TEE bus being registered via > subsys_initcall(). > Not if SCMI platform device is not created before TEE has discovered SCMI service. > > > When both SCMI and TEE are present in DT and SCMI used OPTEE > > > as a transport I see it is correct to represent them as child and parent > > No it's not the correct representation. Devices on the TEE bus have > nothing to do with DT. I disagree, the whole child/parent hierarchy in DT is for such a dependency. That said, I am not asking to create SCMI device via of_populate_devices() from TEE. It can choose to do in whatever is the right ways especially if it can be discovered. > The OP-TEE node in DT represents a particular > TEE implementation whereas there are other ways to represent OP-TEE > implementation as a device on FF-A bus. > Agreed again. > Your suggested change only works due to misrepresentation of SCMI > OP-TEE transport as highlighted above while it won't fix the case with > OP-TEE device on FF-A bus. > Incorrect. As I mentioned before I don't care how we create SCMI device, whether it has to be SCMI platform device or it can be SCMI TEE device with all these managed within probe. > > > and it can be utilised here to solved the problem with respect to the driver > > > model without having to play around with the initcall levels which is always > > > going to bite us back with one extra dependency. > > > > > As I mentioned in the patch description, it's an inter-subsystem > dependency. It has to be solved via initcall levels. A big NACK again. What you think will solve problem on this problem may cause issue on some other platform or some other config. We definitely need a solution which is not fiddling with initcall levels. I am sure others agree with that as we have had enough issues with that in the past and the whole probe deferral is result of that. But in this case it is not working and we can't go back to initcall for sure. > I am unsure which > extra dependency you have in mind but the one mentioned below doesn't > fall into that category. Have you looked at other places within the > kernel for usage of subsys_initcall_sync()? > Yes, but still I don't agree with that. For me it is hack just to solve the issue at hand instead of thinking what is wrong with the design. The whole point of all these discoverable buses was to get rid of such initcall and deferred probe dependency and we must not use them to solve the issue here. > > > And with FF-A, TEE and SCMI all in the mix we might have that dependency > > > already, so I really want to avoid playing with initcall levels just to solve > > > this problem. > > There isn't a three level dependency here. The only dependency we have > to solve is that the SCMI OP-TEE transport shouldn't register on TEE > bus prior to registration of TEE bus. And switching SCMI subsystem to > subsys_initcall_sync() fixes that dependency even with OP-TEE FF-A > ABI. > NACK again. I won't accept that unless we have explore right options and get agreement with Greg or others that this is the only way. > > > > Even with FFA, the optee driver still registers from module_init level > > (== device_init level initcall), as when using legacy OP-TE SMC ABI. > > SCMI firmware driver is initialized from subsys_init level hence > > before optee driver. So I think SCMI optee transport relies on the > > same dependencies whatever OP-TEE is using FFA ABI or its legacy SCM > > ABI. > > > > I guess here you are confusing the TEE subsystem driver invoked at > subsys_initcall() versus OP-TEE driver invoked at module_init(). The > TEE bus is registered by the TEE subsystem driver rather than the > OP-TEE driver. > I agree. One option I could think of as I read this is to have SCMI optee to be proper driver at appropriate level and the probe can add the platform SCMI device needed to prove SCMI driver from scmi_optee_service_probe. That should work IMO. I will try to hack up something and share. > So there is *no* dependency among SCMI firmware driver and OP-TEE > driver but rather the dependency is with TEE subsystem driver instead. > Agreed. > > Device discovery from OP-TEE bus will always need to wait for the > > OP-TEE bus to be ready. > > It isn't an OP-TEE bus but rather a TEE bus with underlying TEE > implementations like OP-TEE etc. registering their corresponding > devices. > +1
On Mon, Nov 14, 2022 at 01:47:25PM +0000, Ludvig Pärsson wrote: > On Mon, 2022-11-14 at 12:29 +0100, Etienne Carriere wrote: > > Hello all, > > Hi Ludvig, following up on the issues raised by this thread and a few proposals that were flying around (online and offline), in the past days I took the chance to have a go at a substantial rework of the init/probe sequences in the SCMI core to address the issue you faced with SCMI TEE transport while trying to untangle a bit the SCMI core startup sequences (... while also possibly not breaking it all :P...) In a nutshell, building on an idea from an offline chat with Etienne ad Sudeep, now the SCMI bus initialization is split on its own and initialized at subsys_initcall level, while the SCMI core stack, including the the SCMI TEE transport layer, is moved at module_init layer together with the SCMI driver users. This *should* theoretically solve your issue ... (and it seems like all the rest it's still working :P) ... so I was wondering if you can give a go at the following pachset on your setup: https://gitlab.arm.com/linux-arm/linux-cm/-/commits/scmi_rework_stack_init_draft/ ... note that this is just a draft at the moment, which has undergone a reasonable amount of testing on mailbox/virtio transports only in both a SCMI builtin and/or modules scenario, but is no where ready for review. The top three patches are really what you need BUT these are probably tightly bound to that bunch of early fixes you can see in the branch...so in other words better if you pick the whole branch for testing :D Once you've confirmed me that this solves your issues I'll start the final cleanup for posting in the next cycle. Thanks, Cristian
On Tue, 2022-11-22 at 17:48 +0000, Cristian Marussi wrote: > On Mon, Nov 14, 2022 at 01:47:25PM +0000, Ludvig Pärsson wrote: > > On Mon, 2022-11-14 at 12:29 +0100, Etienne Carriere wrote: > > > Hello all, > > > > > Hi Ludvig, > > following up on the issues raised by this thread and a few proposals > that > were flying around (online and offline), in the past days I took the > chance > to have a go at a substantial rework of the init/probe sequences in > the SCMI > core to address the issue you faced with SCMI TEE transport while > trying to > untangle a bit the SCMI core startup sequences (... while also > possibly not > breaking it all :P...) > > In a nutshell, building on an idea from an offline chat with Etienne > ad > Sudeep, now the SCMI bus initialization is split on its own and > initialized at > subsys_initcall level, while the SCMI core stack, including the the > SCMI TEE > transport layer, is moved at module_init layer together with the SCMI > driver users. > > This *should* theoretically solve your issue ... (and it seems like > all the > rest it's still working :P) ... so I was wondering if you can give a > go > at the following pachset on your setup: > > https://gitlab.arm.com/linux-arm/linux-cm/-/commits/scmi_rework_stack_init_draft/ > > ... note that this is just a draft at the moment, which has undergone > a > reasonable amount of testing on mailbox/virtio transports only in > both a > SCMI builtin and/or modules scenario, but is no where ready for > review. > > The top three patches are really what you need BUT these are probably > tightly bound to that bunch of early fixes you can see in the > branch...so in other words better if you pick the whole branch for > testing :D > > Once you've confirmed me that this solves your issues I'll start the > final cleanup for posting in the next cycle. > > Thanks, > Cristian Hi Cristian, I tried my best to get the patchset to work somehow on my version of the kernel, and it seems to be working great. I played around with some things, for example changing order of some drivers that were on the same init levels, and it still worked. Only tested with voltage domain protocol and optee transport. Thanks for your great work! BR, Ludvig
On Tue, Nov 29, 2022 at 10:49:10AM +0000, Ludvig Pärsson wrote: > On Tue, 2022-11-22 at 17:48 +0000, Cristian Marussi wrote: > > On Mon, Nov 14, 2022 at 01:47:25PM +0000, Ludvig Pärsson wrote: > > > On Mon, 2022-11-14 at 12:29 +0100, Etienne Carriere wrote: > > > > Hello all, > > > > > > > > Hi Ludvig, > > > > following up on the issues raised by this thread and a few proposals > > that > > were flying around (online and offline), in the past days I took the > > chance > > to have a go at a substantial rework of the init/probe sequences in > > the SCMI > > core to address the issue you faced with SCMI TEE transport while > > trying to > > untangle a bit the SCMI core startup sequences (... while also > > possibly not > > breaking it all :P...) > > > > In a nutshell, building on an idea from an offline chat with Etienne > > ad > > Sudeep, now the SCMI bus initialization is split on its own and > > initialized at > > subsys_initcall level, while the SCMI core stack, including the the > > SCMI TEE > > transport layer, is moved at module_init layer together with the SCMI > > driver users. > > > > This *should* theoretically solve your issue ... (and it seems like > > all the > > rest it's still working :P) ... so I was wondering if you can give a > > go > > at the following pachset on your setup: > > > > https://gitlab.arm.com/linux-arm/linux-cm/-/commits/scmi_rework_stack_init_draft/ > > > > ... note that this is just a draft at the moment, which has undergone > > a > > reasonable amount of testing on mailbox/virtio transports only in > > both a > > SCMI builtin and/or modules scenario, but is no where ready for > > review. > > > > The top three patches are really what you need BUT these are probably > > tightly bound to that bunch of early fixes you can see in the > > branch...so in other words better if you pick the whole branch for > > testing :D > > > > Once you've confirmed me that this solves your issues I'll start the > > final cleanup for posting in the next cycle. > > > > Thanks, > > Cristian > > Hi Cristian, Hi, > > I tried my best to get the patchset to work somehow on my version of > the kernel, and it seems to be working great. I played around with some > things, for example changing order of some drivers that were on the > same init levels, and it still worked. Only tested with voltage domain > protocol and optee transport. > > Thanks for your great work! > Great, thanks for testing it. I'll post shortly a cleaned up series aiming at the next release cycle. Thanks, Cristian
Hi Cristian, Ludvig, For info, I've tested your patches Cristian on my setup. they did the job for probing optee transport at module initcall level. br, etienne On Tue, 29 Nov 2022 at 13:15, Cristian Marussi <cristian.marussi@arm.com> wrote: > > On Tue, Nov 29, 2022 at 10:49:10AM +0000, Ludvig Pärsson wrote: > > On Tue, 2022-11-22 at 17:48 +0000, Cristian Marussi wrote: > > > On Mon, Nov 14, 2022 at 01:47:25PM +0000, Ludvig Pärsson wrote: > > > > On Mon, 2022-11-14 at 12:29 +0100, Etienne Carriere wrote: > > > > > Hello all, > > > > > > > > > > > Hi Ludvig, > > > > > > following up on the issues raised by this thread and a few proposals > > > that > > > were flying around (online and offline), in the past days I took the > > > chance > > > to have a go at a substantial rework of the init/probe sequences in > > > the SCMI > > > core to address the issue you faced with SCMI TEE transport while > > > trying to > > > untangle a bit the SCMI core startup sequences (... while also > > > possibly not > > > breaking it all :P...) > > > > > > In a nutshell, building on an idea from an offline chat with Etienne > > > ad > > > Sudeep, now the SCMI bus initialization is split on its own and > > > initialized at > > > subsys_initcall level, while the SCMI core stack, including the the > > > SCMI TEE > > > transport layer, is moved at module_init layer together with the SCMI > > > driver users. > > > > > > This *should* theoretically solve your issue ... (and it seems like > > > all the > > > rest it's still working :P) ... so I was wondering if you can give a > > > go > > > at the following pachset on your setup: > > > > > > https://gitlab.arm.com/linux-arm/linux-cm/-/commits/scmi_rework_stack_init_draft/ > > > > > > ... note that this is just a draft at the moment, which has undergone > > > a > > > reasonable amount of testing on mailbox/virtio transports only in > > > both a > > > SCMI builtin and/or modules scenario, but is no where ready for > > > review. > > > > > > The top three patches are really what you need BUT these are probably > > > tightly bound to that bunch of early fixes you can see in the > > > branch...so in other words better if you pick the whole branch for > > > testing :D > > > > > > Once you've confirmed me that this solves your issues I'll start the > > > final cleanup for posting in the next cycle. > > > > > > Thanks, > > > Cristian > > > > Hi Cristian, > > Hi, > > > > > I tried my best to get the patchset to work somehow on my version of > > the kernel, and it seems to be working great. I played around with some > > things, for example changing order of some drivers that were on the > > same init levels, and it still worked. Only tested with voltage domain > > protocol and optee transport. > > > > Thanks for your great work! > > > > Great, thanks for testing it. > I'll post shortly a cleaned up series aiming at the next release cycle. > > Thanks, > Cristian
On Tue, Nov 29, 2022 at 03:57:50PM +0100, Etienne Carriere wrote: > Hi Cristian, Ludvig, > Hi Etienne, > For info, I've tested your patches Cristian on my setup. > they did the job for probing optee transport at module initcall level. > Thanks for testing, I'll keep you in CC when posting the final series. Cristian
diff --git a/drivers/firmware/arm_scmi/driver.c b/drivers/firmware/arm_scmi/driver.c index f818d00bb2c6..f43e52541da4 100644 --- a/drivers/firmware/arm_scmi/driver.c +++ b/drivers/firmware/arm_scmi/driver.c @@ -2667,7 +2667,7 @@ static int __init scmi_driver_init(void) return platform_driver_register(&scmi_driver); } -subsys_initcall(scmi_driver_init); +subsys_initcall_sync(scmi_driver_init); static void __exit scmi_driver_exit(void) {