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

On 10-01-20, 12:15, Arnd Bergmann wrote:

On Fri, Jan 10, 2020 at 10:43 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, some of the transport protocols getting discussed currently
are SMC/HVC, SPCI (built on top of SMC/HVC), OPTEE based mailbox
(similar to SPCI), and vitio based transport as alternative to mailbox.

The transport protocols just need to provide struct scmi_desc, which
also implements the struct scmi_transport_ops.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V2:
- Dropped __iomem from payload data.

Simply dropping the __iomem isn't much better, now you get other
type mismatches.

Right. So what exactly do you suggest I should do now? Drop __iomem
from the structure's payload field but keep all local variables and
function arguments with __iomem ?

quoted

- Moved transport ops to scmi_desc, and that has a per transport
  instance now which is differentiated using the compatible string.
- Converted IS_ERR_OR_NULL to IS_ERR.

These look good to me.

quoted

+ * @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 *payload;
+       struct device *dev;
+       struct scmi_handle *handle;
+       void *transport_info;
+};

Maybe you can wrap the scmi_chan_info inside of another
structure that contains  the payload pointer, and use container_of
to convert between them?

We don't need to convert between the two of them, isn't it ? Are you
referring some other field here ?

It's not obvious which parts of the structure should be shared and
which are transport specific.

All transport specific information is kept in the transport specific
structure which is saved here in the transport_info field. Is there
something else that isn't clear ?

-- 
viresh

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