On 09-01-20, 09:18, Arnd Bergmann wrote:

On Fri, Nov 29, 2019 at 10:32 AM Viresh Kumar [off-list ref] wrote:

quoted

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 of 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.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Conceptually I think this is fine, but as others have said, it would be
better to have another transport implementation posted along with this
to see if the interfaces actually work out.

@Sudeep/Vincent: Do you think we can add another transport
implementation something right away for it ?

@Peng ?

quoted

+/**
+ * struct scmi_chan_info - Structure representing a SCMI channel information
+ *
+ * @payload: Transmit/Receive payload area
+ * @dev: Reference to device in the SCMI hierarchy corresponding to this
+ *      channel
+ * @handle: Pointer to SCMI entity handle
+ * @transport_info: Transport layer related information
+ */
+struct scmi_chan_info {
+       void __iomem *payload;
+       struct device *dev;
+       struct scmi_handle *handle;
+       void *transport_info;
+};

I would assume that with another transport, the 'payload' pointer would
not be __iomem

Hmm, okay. I just separated things based on the current transport and
didn't add much changes on top of it as I wasn't sure how things are
going to look with next transport and so left the changes for then.

I can now drop it though.

quoted

+static int scmi_set_transport_ops(struct scmi_info *info)
+{
+       struct scmi_transport_ops *ops;
+       struct device *dev = info->dev;
+
+       /* Only mailbox method supported for now */
+       ops = scmi_mailbox_get_ops(dev);
+       if (!ops) {
+               dev_err(dev, "Transport protocol not found in %pOF\n",
+                       dev->of_node);
+               return -EINVAL;
+       }
+
+       info->transport_ops = ops;
+       return 0;
+}

This looks odd: rather than guessing the transport type based on
random DT properties, I would prefer to have it determined by
the device compatible string, and have different drivers bind
to one of them each, with each driver linking against a common
base implementation, either as separate modules or in one file.

Since there are no platforms using the scmi binding in mainline kernel
for now, it won't be difficult to add new compatible strings. So
should this be done like:

        compatible = "arm,scmi", "arm,scmi-mailbox";

or just
        compatible = "arm,scmi-mailbox";

?
-- 
viresh

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel