Re: [PATCH V4] firmware: arm_scmi: Make scmi core independent of the transport type

[Peter Hilber <peter.hilber@opensynergy.com>]

On 22.01.20 03:36, Viresh Kumar wrote:
> The SCMI specification is fairly independent of the transport protocol,
> which can be a simple mailbox (already implemented) or anything else.
> The current Linux implementation however is very much dependent on the
> mailbox transport layer.
>
> This patch makes the SCMI core code (driver.c) independent of the
> mailbox transport layer and moves all mailbox related code to a new
> file: mailbox.c.
>
> We can now implement more transport protocols to transport SCMI
> messages.
>
> The transport protocols just need to provide struct scmi_transport_ops,
> with its version of the callbacks to enable exchange of SCMI messages.

Sorry for being a bit late with my feedback.

Accessing struct scmi_shared_mem members from driver.c forces any
transport to also use the struct scmi_shared_mem layout (or at least
pretend to do so). IMHO this does not work very well for transports
which are not using a fixed memory region to transmit and receive. But I
think the current approach will still work out.

virtio transfers each message in a separate buffer, and always uses
different parts of the buffer for transmit data and receive data, which
is contrary to the assumptions of the struct scmi_shared_mem.

The virtio transport will probably have no use for the struct
scmi_shared_mem.channel_status and .flags. The check for
SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE in scmi_tx_prepare() is not required
for the virtio transport.

I would have preferred (to have an option) to use as data passing
interface to the transport just the struct scmi_xfer. A transport using
this option would not implement ops (read|write)32 and memcpy_(from|to).
The transport would also not call scmi_tx_prepare(), but instead take
data from struct scmi_xfer directly. The transport would use a modified
scmi_rx_callback() to notify that it updated the struct scmi_xfer. A
helper to derive the struct scmi_xfer * from the message header would be
extracted from scmi_rx_callback(). The scmi_xfer_poll_done() would
become an (optional) transport op.

Other remarks:

If staying with this approach, it would be more elegant to add an
abstraction through which the transport can set the
SCMI_SHMEM_CHAN_STAT_CHANNEL_FREE bit in the struct scmi_shared_mem.

The SCMI spec allows both interrupt-based and polling-based completion
notification. The transport should be able to indicate which
notification methods it supports. The virtio transport would not want to
support polling.

> -static void scmi_rx_callback(struct mbox_client *cl, void *m)
> +void scmi_rx_callback(struct scmi_chan_info *cinfo, struct scmi_xfer *t)

scmi_rx_callback() doesn't need the struct scmi_xfer * parameter any
more ATM (and the transport might also not know about it).

Best regards,

Peter

Please mind our privacy notice<https://www.opensynergy.com/datenschutzerklaerung/privacy-notice-for-business-partners-pursuant-to-article-13-of-the-general-data-protection-regulation-gdpr/> pursuant to Art. 13 GDPR. // Unsere Hinweise zum Datenschutz gem. Art. 13 DSGVO finden Sie hier.<https://www.opensynergy.com/de/datenschutzerklaerung/datenschutzhinweise-fuer-geschaeftspartner-gem-art-13-dsgvo/>


[tech_days_munchen]

OpenSynergy TechDay München

am 11. Februar 2020, ab 12:00Uhr, im Studio Balan, Moosacherstr. 86.

Anmeldung bitte hier<mailto:sabine.mutumba@opensynergy.com>