| Message ID | 20201008143722.21888-1-etienne.carriere@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] firmware: arm_scmi: always initialize protocols | expand |
On Thu, Oct 08, 2020 at 04:37:18PM +0200, Etienne Carriere wrote: > Remove the IDR replacement that prevent initializing an SCMI protocol > when it has already been initialized. This is needed when there are > several SCMI agents that do implement a given SCMI protocol unless > what only the related SCMI protocol communication is initialized only > for first probed agent. > Can you please elaborate on your usecase please. What do you mean by several SCMI agents here. OSPM is the only agent we are interested here. What other agents is this driver supposed to handle here. We allocate memory in init and calling init multiple times messes up the allocated and initialised structures. So NACK for this patch as it needs more work if we need this at all.
On Thu, 8 Oct 2020 at 21:17, Sudeep Holla <sudeep.holla@arm.com> wrote: > > On Thu, Oct 08, 2020 at 04:37:18PM +0200, Etienne Carriere wrote: > > Remove the IDR replacement that prevent initializing an SCMI protocol > > when it has already been initialized. This is needed when there are > > several SCMI agents that do implement a given SCMI protocol unless > > what only the related SCMI protocol communication is initialized only > > for first probed agent. > > > > Can you please elaborate on your usecase please. What do you mean by several > SCMI agents here. OSPM is the only agent we are interested here. What > other agents is this driver supposed to handle here. We allocate memory > in init and calling init multiple times messes up the allocated and > initialised structures. > > So NACK for this patch as it needs more work if we need this at all. > Hello Sudeep, Considering a device with several SCMI servers spread over several co-processor and possibly also in the Arm TZ secure world, each of these servers uses a specific SCMI channel. Without this change, each SCMI protocol gets initialized only for the first agent device that is probed. My setup is also a bit specific. My device has several secure configuration features that can individually be enabled or not. For example, configuring domain X as secure makes some clocks reachable by Linux only through SCMI, and configuring domain Y as secure makes other clocks reachable by Linux only through SCMI. For flexibility, I expose domain X resources (here clocks) to an Linux agent whereas domain Y resources (here clocks also) are exposed to another agent, each agent with its specific transport/channel. Enabling each agent node in the Linux FDT allows to define which SCMI clocks get exposed and hence registered in the kernel. Without the change proposed here, I cannot get the clocks exposed to both agents when enabled as the SCMI clock protocol is initialized only for the 1st probbed agent device. Regards, Etienne > -- > Regards, > Sudeep
Hi Etienne, On Fri, Oct 09, 2020 at 02:31:55PM +0200, Etienne Carriere wrote: > On Thu, 8 Oct 2020 at 21:17, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Thu, Oct 08, 2020 at 04:37:18PM +0200, Etienne Carriere wrote: > > > Remove the IDR replacement that prevent initializing an SCMI protocol > > > when it has already been initialized. This is needed when there are > > > several SCMI agents that do implement a given SCMI protocol unless > > > what only the related SCMI protocol communication is initialized only > > > for first probed agent. > > > > > > > Can you please elaborate on your usecase please. What do you mean by several > > SCMI agents here. OSPM is the only agent we are interested here. What > > other agents is this driver supposed to handle here. We allocate memory > > in init and calling init multiple times messes up the allocated and > > initialised structures. > > > > So NACK for this patch as it needs more work if we need this at all. > > > > Hello Sudeep, > > Considering a device with several SCMI servers spread over several co-processor > and possibly also in the Arm TZ secure world, each of these servers > uses a specific > SCMI channel. Without this change, each SCMI protocol gets initialized > only for the > first agent device that is probed. > > My setup is also a bit specific. My device has several secure configuration > features that can individually be enabled or not. For example, configuring > domain X as secure makes some clocks reachable by Linux only through SCMI, > and configuring domain Y as secure makes other clocks reachable by Linux > only through SCMI. For flexibility, I expose domain X resources (here clocks) > to an Linux agent whereas domain Y resources (here clocks also) are > exposed to another agent, each agent with its specific transport/channel. > Enabling each agent node in the Linux FDT allows to define which SCMI clocks > get exposed and hence registered in the kernel. > Without the change proposed here, I cannot get the clocks exposed to both > agents when enabled as the SCMI clock protocol is initialized only for the 1st > probbed agent device. > Just a heads up that I have a series to be posted soon(ish) that is meant to address vendor protocols support and more in general protocols modularization by simplifying and reorganizing 'a bit' the whole initialization process and the handle interface itself: in that context I've got rid as a whole of the above protocol initialization code inside device probing and moved it at the time of first usage of the protocol, i.e. when the first SCMI driver, inside its probe, attempts to get hold of a protocol issuing something like: perf_ops = handle->get_ops(handle, SCMI_PROTOCOL_PERF); This will effectively trigger a run of the usual protocol initialization code if the protocol was still NOT initialized (no previous get_ops) for that SCMI instance (server): so if your SCMI driver/device gets instantiated multiple times against multiple different servers, the needed protocol(s) will be initialized multiple times one for each instance (handle) upon first usage, while any other further instance of another SCMI driver requesting the same, already initialized, protocol will find it ready to go. Cleanup/deinitialization is delegated to a corresponding handle->put_ops(), while the general notifications will still be available using the current .notify_ops() interface, but those requests will be now tracked internally against the relevant protocol in order to avoid premature deinitialization or removal of still in-use protocols. Thanks Cristian > Regards, > Etienne > > > -- > > Regards, > > Sudeep
On Fri, Oct 09, 2020 at 02:31:55PM +0200, Etienne Carriere wrote: > On Thu, 8 Oct 2020 at 21:17, Sudeep Holla <sudeep.holla@arm.com> wrote: > > > > On Thu, Oct 08, 2020 at 04:37:18PM +0200, Etienne Carriere wrote: > > > Remove the IDR replacement that prevent initializing an SCMI protocol > > > when it has already been initialized. This is needed when there are > > > several SCMI agents that do implement a given SCMI protocol unless > > > what only the related SCMI protocol communication is initialized only > > > for first probed agent. > > > > > > > Can you please elaborate on your usecase please. What do you mean by several > > SCMI agents here. OSPM is the only agent we are interested here. What > > other agents is this driver supposed to handle here. We allocate memory > > in init and calling init multiple times messes up the allocated and > > initialised structures. > > > > So NACK for this patch as it needs more work if we need this at all. > > > > Hello Sudeep, > > Considering a device with several SCMI servers spread over several co-processor > and possibly also in the Arm TZ secure world, each of these servers > uses a specific SCMI channel. Without this change, each SCMI protocol gets > initialized only for the first agent device that is probed. > Fair enough, but it also adds complexity. Do you have a master SCMI server implementation in that case. As some protocols like system might need to be broadcast to all servers and master server needs to take appropriate action. > My setup is also a bit specific. My device has several secure configuration > features that can individually be enabled or not. For example, configuring > domain X as secure makes some clocks reachable by Linux only through SCMI, > and configuring domain Y as secure makes other clocks reachable by Linux > only through SCMI. For flexibility, I expose domain X resources (here clocks) > to an Linux agent whereas domain Y resources (here clocks also) are > exposed to another agent, each agent with its specific transport/channel. > Enabling each agent node in the Linux FDT allows to define which SCMI clocks > get exposed and hence registered in the kernel. > Without the change proposed here, I cannot get the clocks exposed to both > agents when enabled as the SCMI clock protocol is initialized only for the 1st > probbed agent device. > OK, as Cristian has already mentioned we need to clean up a bit on these initcalls and Cristian has some WIP patches, I would like to wait and look at them instead of breaking other usecases with patch(multiple devices per protocol within one scmi server)
On Thu, 8 Oct 2020 16:37:18 +0200, Etienne Carriere wrote: > Remove the IDR replacement that prevent initializing an SCMI protocol > when it has already been initialized. This is needed when there are > several SCMI agents that do implement a given SCMI protocol unless > what only the related SCMI protocol communication is initialized only > for first probed agent. I am picking up 4-5. Applied to sudeep.holla/linux (for-next/scmi), thanks! [4/5] firmware: arm_scmi: Expand SMC/HVC message pool to more than one https://git.kernel.org/sudeep.holla/c/7adb2c8aaa [5/5] firmware: arm_scmi: Fix ARCH_COLD_RESET https://git.kernel.org/sudeep.holla/c/45b9e04d5b -- Regards, Sudeep
diff --git a/drivers/firmware/arm_scmi/bus.c b/drivers/firmware/arm_scmi/bus.c index 1377ec76a45d..8ea04b069129 100644 --- a/drivers/firmware/arm_scmi/bus.c +++ b/drivers/firmware/arm_scmi/bus.c @@ -60,11 +60,6 @@ static int scmi_protocol_init(int protocol_id, struct scmi_handle *handle) return fn(handle); } -static int scmi_protocol_dummy_init(struct scmi_handle *handle) -{ - return 0; -} - static int scmi_dev_probe(struct device *dev) { struct scmi_driver *scmi_drv = to_scmi_driver(dev->driver); @@ -83,10 +78,6 @@ static int scmi_dev_probe(struct device *dev) if (ret) return ret; - /* Skip protocol initialisation for additional devices */ - idr_replace(&scmi_protocols, &scmi_protocol_dummy_init, - scmi_dev->protocol_id); - return scmi_drv->probe(scmi_dev); }
Remove the IDR replacement that prevent initializing an SCMI protocol when it has already been initialized. This is needed when there are several SCMI agents that do implement a given SCMI protocol unless what only the related SCMI protocol communication is initialized only for first probed agent. Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org> --- drivers/firmware/arm_scmi/bus.c | 9 --------- 1 file changed, 9 deletions(-)