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

Viresh Kumar viresh.kumar at linaro.org
Thu Jan 9 01:34:42 PST 2020


On 09-01-20, 09:18, Arnd Bergmann wrote:
> On Fri, Nov 29, 2019 at 10:32 AM Viresh Kumar <viresh.kumar at linaro.org> 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 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 at 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 ?

> > +/**
> > + * 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.

> > +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



More information about the linux-arm-kernel mailing list