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

[Sudeep Holla <sudeep.holla@arm.com>]

On Thu, Jan 09, 2020 at 03:04:42PM +0530, Viresh Kumar wrote:
> On 09-01-20, 09:18, Arnd Bergmann wrote:
> > On Fri, Nov 29, 2019 at 10:32 AM Viresh Kumar <viresh.kumar@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@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.

I am fine adding new compatible but since the binding is present in the
mainline for several releases now, we may have to have fallback to mailbox
as default if any of the new compatibles added is missing.

--
Regards,
Sudeep