Messages in this thread | | | From | Ludvig Pärsson <> | Subject | Re: [PATCH] firmware: arm_scmi: Resolve dependency with TEE subsystem | Date | Mon, 14 Nov 2022 13:47:25 +0000 |
| |
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
| |