Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
From:   Etienne Carriere <etienne.carriere@linaro.org>
Date:   Thu, 30 Jan 2020 10:25:58 +0100
Message-ID: <CAN5uoS_A9TYU2aWf3WVq3KGna6oVswfut+hC7sJWvhfYggMwTA@mail.gmail.com>
Subject: Re: [PATCH V5] firmware: arm_scmi: Make scmi core independent of the
 transport type
To:     viresh.kumar@linaro.org, linux-kernel@vger.kernel.org,
        Peng Fan <peng.fan@nxp.com>,
        Sudeep Holla <sudeep.holla@arm.com>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hello Viresh,

On Tue, Jan 28, 2020 at 04:24:19PM +0530, 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.
>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
> @Sudeep: Please help getting this tested as well :)
>
> V4->V5:
> - struct scmi_shared_mem is moved to mailbox.c and it is completely
>   handled by transport layer now.
> - And so lots of ops change due to this.
> - Fixed a bug from previous version where wrong dev structure was
>   getting passed to devm_kzalloc().
>

Hello Viresh, Sudeep,
I've made a first port (draft) for adding new transport channels, next
to existing mailbox channel, on top of your change.
You can find it here: https://github.com/etienne-lms/linux/pull/1.

I don't have specific comments on your change but the one below.
I think SMT header should move out of mailbox.c, but that may be a bit
out of the scope of your change.


> (...)
>
> @@ -470,13 +310,7 @@ int scmi_do_xfer(const struct scmi_handle *handle, struct scmi_xfer *xfer)
>  if (!ret && xfer->hdr.status)
>   ret = scmi_to_linux_errno(xfer->hdr.status);
>
> - /*
> - * NOTE: we might prefer not to need the mailbox ticker to manage the
> - * transfer queueing since the protocol layer queues things by itself.
> - * Unfortunately, we have to kick the mailbox framework after we have
> - * received our message.
> - */
> - mbox_client_txdone(cinfo->chan, ret);
> + info->desc->ops->mark_txdone(cinfo, ret);
>
>  return ret;
> }

I would prefer an optional mak_txdone callback:

    if (info->desc->ops->mark_txdone)
       info->desc->ops->mark_txdone(cinfo, ret);


> @@ -713,29 +547,18 @@ static int scmi_xfer_info_init(struct scmi_info *sinfo)
>   return 0;
>  }
>
> -static int scmi_mailbox_check(struct device_node *np, int idx)
> -{
> - return of_parse_phandle_with_args(np, "mboxes", "#mbox-cells",
> -   idx, NULL);
> -}
> -
> -static int scmi_mbox_chan_setup(struct scmi_info *info, struct device *dev,
> - int prot_id, bool tx)
> +static int scmi_chan_setup(struct scmi_info *info, struct device *dev,
> +    int prot_id, bool tx)
>
> (...)


Regards,
Etienne