Re: [PATCH v3 0/8] Introduce atomic support for SCMI transports

[Peter Hilber]

On 12.07.21 18:26, Cristian Marussi wrote:
> Hi all,
>
> This series mainly aims to introduce atomic support for transports
> that can support it.
>
> At first in [1/8], as a closely related addition, it is introduced a
> common way for a transport to signal to the SCMI core that it does not
> offer completion interrupts, so that the usual polling behaviour based
> on .poll_done() will be required: this can be done enabling statically
> a global polling behaviour for the whole transport with flag
> scmi_desc.force_polling OR dynamically enabling at runtime such polling
> behaviour on a per-channel basis with scmi_chan_info.needs_polling,
> typically during .chan_setup(). The usual per-command polling selection
> behaviour based on hdr.poll_completion is preserved as before.
>
> Then in [2/8], a transport that supports atomic operations on its TX path
> can now declare itself as .atomic_capable and as a consequence the SCMI
> core will refrain itself from sleeping on the correspondent RX-path.
>
> In [5/8] a simple method is introduced so that an SCMI driver can easily
> query the core to check if the currently used transport is configured to
> behave in an atomic manner: in this way, interested SCMI driver users, like
> Clock framework [6/8], can optionally support atomic operations when
> operating on an atomically configured transport.
>
> Finally there are 2 *tentative" RFC patch for SMC transport: at first [7/8]
> ports SMC to use the common core completions when completion interrupt is
> available or otherwise revert to use common core polling mechanism above
> introduced; then in [8/8] SMC is converted to be .atomic_capable by
> substituting the mutexes with busy-waiting to keep the channel 'locked'.
>
> SMC changes have NOT been tested so far (I cannot), AND they are just a
> proposal at this stage to try to better abstract and unify behaviour with
> the SCMI core; both patches are completely intended as RFCs, though, not
> only regarding their implementation but even their mere existence is RFC:
> I mean maybe we just don't want to do such kind of unification/abstraction,
> and I can just drop those SMC patches if unwanted; any feedback welcome.
>
> Atomic support has been minimally tested against the upcoming virtio
> transport V6 series, while polling has been tested with mailbox transports.
>
> The series is based on SCMI VirtIO Transport support V6 [1] (since it will
> be the main prospective user of atomic mode) and, as such, it is also
> publicly available on top of SCMI VirtIO V6 from ARM GitLab [2].
> (Note that in order to use/test atomic mode on virtio you'll have to enable
>   it setting .atomic_capable = true in virtio.c::scmi_virtio_desc)

I think the .atomic_capable setting should become configurable, not 
always-on, for the scmi-virtio transport (and the timeout should become 
configurable as well). Not sure if it might make sense to add a 
corresponding new feature for the (OASIS) Virtio SCMI device definition.

Best regards,

Peter

> Given I'm still gathering feedback on this, I still not have CCed any
> maintainer out of SCMI subsystem.
>
> Any feedback welcome.
>
> Thanks,
>
> Cristian
>
> ---
>
> [1]:https://lore.kernel.org/linux-arm-kernel/20210712141833.6628-1-cristian.marussi@xxxxxxx/
> [2]:https://gitlab.arm.com/linux-arm/linux-cm/-/commits/scmi_atomic_transport_V3_on_virtio_V6
>
> V2 --> v3
> - rebased on SCMI VirtIO V6 which in turn is based on v5.14-rc1
>
>
> Cristian Marussi (8):
>    firmware: arm_scmi: Add configurable polling mode for transports
>    firmware: arm_scmi: Add support for atomic transports
>    include: trace: Add new scmi_xfer_response_wait event
>    firmware: arm_scmi: Use new trace event scmi_xfer_response_wait
>    firmware: arm_scmi: Add is_transport_atomic() handle method
>    clk: scmi: Support atomic enable/disable API
>    [RFC] firmware: arm_scmi: Make smc transport use common completions
>    [RFC] firmware: arm_scmi: Make smc transport atomic
>
>   drivers/clk/clk-scmi.c             |  44 ++++--
>   drivers/firmware/arm_scmi/common.h |  13 ++
>   drivers/firmware/arm_scmi/driver.c | 223 ++++++++++++++++++++++-------
>   drivers/firmware/arm_scmi/smc.c    |  61 +++++---
>   include/linux/scmi_protocol.h      |   8 ++
>   include/trace/events/scmi.h        |  28 ++++
>   6 files changed, 297 insertions(+), 80 deletions(-)