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

From: Peng Fan
Date: Mon Dec 30 2019 - 21:51:09 EST

Next message: Al Viro: "Re: Why is CONFIG_VT forced on?"
Previous message: Yang Shi: "Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes"
Next in thread: Sudeep Holla: "Re: [PATCH] firmware: arm_scmi: Make scmi core independent of transport type"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

> Subject: [PATCH] firmware: arm_scmi: Make scmi core independent of
> transport type
>
> 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.

Will there be v2? will this be used to replace smc mailbox?

Thanks,
Peng.

>
> Signed-off-by: Viresh Kumar <viresh.kumar@xxxxxxxxxx>
> ---
> drivers/firmware/arm_scmi/Makefile | 3 +-
> drivers/firmware/arm_scmi/common.h | 39 ++++++++
> drivers/firmware/arm_scmi/driver.c | 143 ++++++++++-----------------
> drivers/firmware/arm_scmi/mailbox.c | 146
> ++++++++++++++++++++++++++++
> 4 files changed, 236 insertions(+), 95 deletions(-) create mode 100644
> drivers/firmware/arm_scmi/mailbox.c
>
> diff --git a/drivers/firmware/arm_scmi/Makefile
> b/drivers/firmware/arm_scmi/Makefile
> index 5f298f00a82e..df2c05a545d8 100644
> --- a/drivers/firmware/arm_scmi/Makefile
> +++ b/drivers/firmware/arm_scmi/Makefile
> @@ -1,6 +1,7 @@
> # SPDX-License-Identifier: GPL-2.0-only
> -obj-y = scmi-bus.o scmi-driver.o scmi-protocols.o
> +obj-y = scmi-bus.o scmi-driver.o scmi-protocols.o scmi-transport.o
> scmi-bus-y = bus.o
> scmi-driver-y = driver.o
> +scmi-transport-y = mailbox.o
> scmi-protocols-y = base.o clock.o perf.o power.o reset.o sensors.o
> obj-$(CONFIG_ARM_SCMI_POWER_DOMAIN) += scmi_pm_domain.o diff
> --git a/drivers/firmware/arm_scmi/common.h
> b/drivers/firmware/arm_scmi/common.h
> index 5237c2ff79fe..e11314146e67 100644
> --- a/drivers/firmware/arm_scmi/common.h
> +++ b/drivers/firmware/arm_scmi/common.h
> @@ -111,3 +111,42 @@ void scmi_setup_protocol_implemented(const
> struct scmi_handle *handle,
> u8 *prot_imp);
>
> int scmi_base_protocol_init(struct scmi_handle *h);
> +
> +/* SCMI Transport */
> +
> +/**
> + * 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;
> +};
> +
> +/**
> + * struct scmi_transport_ops - Structure representing a SCMI transport
> +ops
> + *
> + * @send_message: Callback to send a message
> + * @mark_txdone: Callback to mark tx as done
> + * @chan_setup: Callback to allocate and setup a channel
> + * @chan_free: Callback to free a channel */ struct scmi_transport_ops
> +{
> + bool (*chan_available)(struct device *dev, int idx);
> + int (*chan_setup)(struct scmi_chan_info *cinfo, bool tx);
> + int (*chan_free)(int id, void *p, void *data);
> + int (*send_message)(struct scmi_chan_info *cinfo, struct scmi_xfer
> *xfer);
> + void (*mark_txdone)(struct scmi_chan_info *cinfo, int ret); };
> +
> +void scmi_tx_prepare(struct scmi_chan_info *cinfo, struct scmi_xfer
> +*t); void scmi_rx_callback(struct scmi_chan_info *cinfo, struct
> +scmi_xfer *t); void scmi_free_channel(struct scmi_chan_info *cinfo,
> +struct idr *idr, int id); struct scmi_transport_ops
> +*scmi_mailbox_get_ops(struct device *dev);
> diff --git a/drivers/firmware/arm_scmi/driver.c
> b/drivers/firmware/arm_scmi/driver.c
> index 3eb0382491ce..dfdd12f3dd8b 100644
> --- a/drivers/firmware/arm_scmi/driver.c
> +++ b/drivers/firmware/arm_scmi/driver.c
> @@ -19,7 +19,6 @@
> #include <linux/io.h>
> #include <linux/kernel.h>
> #include <linux/ktime.h>
> -#include <linux/mailbox_client.h>
> #include <linux/module.h>
> #include <linux/of_address.h>
> #include <linux/of_device.h>
> @@ -91,24 +90,6 @@ struct scmi_desc {
> int max_msg_size;
> };
>
> -/**
> - * struct scmi_chan_info - Structure representing a SCMI channel information
> - *
> - * @cl: Mailbox Client
> - * @chan: Transmit/Receive mailbox channel
> - * @payload: Transmit/Receive mailbox channel payload area
> - * @dev: Reference to device in the SCMI hierarchy corresponding to this
> - * channel
> - * @handle: Pointer to SCMI entity handle
> - */
> -struct scmi_chan_info {
> - struct mbox_client cl;
> - struct mbox_chan *chan;
> - void __iomem *payload;
> - struct device *dev;
> - struct scmi_handle *handle;
> -};
> -
> /**
> * struct scmi_info - Structure representing a SCMI instance
> *
> @@ -128,6 +109,7 @@ struct scmi_chan_info { struct scmi_info {
> struct device *dev;
> const struct scmi_desc *desc;
> + struct scmi_transport_ops *transport_ops;
> struct scmi_revision_info version;
> struct scmi_handle handle;
> struct scmi_xfers_info tx_minfo;
> @@ -138,7 +120,6 @@ struct scmi_info {
> int users;
> };
>
> -#define client_to_scmi_chan_info(c) container_of(c, struct scmi_chan_info,
> cl)
> #define handle_to_scmi_info(h) container_of(h, struct scmi_info, handle)
>
> /*
> @@ -233,18 +214,16 @@ static inline void unpack_scmi_header(u32
> msg_hdr, struct scmi_msg_hdr *hdr) }
>
> /**
> - * scmi_tx_prepare() - mailbox client callback to prepare for the transfer
> + * scmi_tx_prepare() - callback to prepare for the transfer
> *
> - * @cl: client pointer
> - * @m: mailbox message
> + * @cinfo: SCMI channel info
> + * @t: transfer message
> *
> * This function prepares the shared memory which contains the header and
> the
> * payload.
> */
> -static void scmi_tx_prepare(struct mbox_client *cl, void *m)
> +void scmi_tx_prepare(struct scmi_chan_info *cinfo, struct scmi_xfer *t)
> {
> - struct scmi_xfer *t = m;
> - struct scmi_chan_info *cinfo = client_to_scmi_chan_info(cl);
> struct scmi_shared_mem __iomem *mem = cinfo->payload;
>
> /*
> @@ -332,10 +311,10 @@ __scmi_xfer_put(struct scmi_xfers_info *minfo,
> struct scmi_xfer *xfer) }
>
> /**
> - * scmi_rx_callback() - mailbox client callback for receive messages
> + * scmi_rx_callback() - callback for receive messages
> *
> - * @cl: client pointer
> - * @m: mailbox message
> + * @cinfo: SCMI channel info
> + * @t: transfer message
> *
> * Processes one received message to appropriate transfer information and
> * signals completion of the transfer.
> @@ -343,13 +322,12 @@ __scmi_xfer_put(struct scmi_xfers_info *minfo,
> struct scmi_xfer *xfer)
> * NOTE: This function will be invoked in IRQ context, hence should be
> * as optimal as possible.
> */
> -static void scmi_rx_callback(struct mbox_client *cl, void *m)
> +void scmi_rx_callback(struct scmi_chan_info *cinfo, struct scmi_xfer
> +*t)
> {
> u8 msg_type;
> u32 msg_hdr;
> u16 xfer_id;
> struct scmi_xfer *xfer;
> - struct scmi_chan_info *cinfo = client_to_scmi_chan_info(cl);
> struct device *dev = cinfo->dev;
> struct scmi_info *info = handle_to_scmi_info(cinfo->handle);
> struct scmi_xfers_info *minfo = &info->tx_minfo; @@ -439,15 +417,12
> @@ int scmi_do_xfer(const struct scmi_handle *handle, struct scmi_xfer
> *xfer)
> if (unlikely(!cinfo))
> return -EINVAL;
>
> - ret = mbox_send_message(cinfo->chan, xfer);
> + ret = info->transport_ops->send_message(cinfo, xfer);
> if (ret < 0) {
> - dev_dbg(dev, "mbox send fail %d\n", ret);
> + dev_dbg(dev, "Failed to send message %d\n", ret);
> return ret;
> }
>
> - /* mbox_send_message returns non-negative value on success, so reset
> */
> - ret = 0;
> -
> if (xfer->hdr.poll_completion) {
> ktime_t stop = ktime_add_ns(ktime_get(),
> SCMI_MAX_POLL_TO_NS);
>
> @@ -461,7 +436,7 @@ int scmi_do_xfer(const struct scmi_handle *handle,
> struct scmi_xfer *xfer)
> /* And we wait for the response. */
> timeout = msecs_to_jiffies(info->desc->max_rx_timeout_ms);
> if (!wait_for_completion_timeout(&xfer->done, timeout)) {
> - dev_err(dev, "mbox timed out in resp(caller: %pS)\n",
> + dev_err(dev, "timed out in resp(caller: %pS)\n",
> (void *)_RET_IP_);
> ret = -ETIMEDOUT;
> }
> @@ -470,13 +445,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->transport_ops->mark_txdone(cinfo, ret);
>
> return ret;
> }
> @@ -713,21 +682,14 @@ 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)
> {
> int ret, idx;
> struct resource res;
> resource_size_t size;
> - struct device_node *shmem, *np = dev->of_node;
> + struct device_node *shmem;
> struct scmi_chan_info *cinfo;
> - struct mbox_client *cl;
> struct idr *idr;
> const char *desc = tx ? "Tx" : "Rx";
>
> @@ -735,7 +697,7 @@ static int scmi_mbox_chan_setup(struct scmi_info
> *info, struct device *dev,
> idx = tx ? 0 : 1;
> idr = tx ? &info->tx_idr : &info->rx_idr;
>
> - if (scmi_mailbox_check(np, idx)) {
> + if (!info->transport_ops->chan_available(dev, idx)) {
> cinfo = idr_find(idr, SCMI_PROTOCOL_BASE);
> if (unlikely(!cinfo)) /* Possible only if platform has no Rx */
> return -EINVAL;
> @@ -748,14 +710,7 @@ static int scmi_mbox_chan_setup(struct scmi_info
> *info, struct device *dev,
>
> cinfo->dev = dev;
>
> - cl = &cinfo->cl;
> - cl->dev = dev;
> - cl->rx_callback = scmi_rx_callback;
> - cl->tx_prepare = tx ? scmi_tx_prepare : NULL;
> - cl->tx_block = false;
> - cl->knows_txdone = tx;
> -
> - shmem = of_parse_phandle(np, "shmem", idx);
> + shmem = of_parse_phandle(dev->of_node, "shmem", idx);
> ret = of_address_to_resource(shmem, 0, &res);
> of_node_put(shmem);
> if (ret) {
> @@ -770,14 +725,9 @@ static int scmi_mbox_chan_setup(struct scmi_info
> *info, struct device *dev,
> return -EADDRNOTAVAIL;
> }
>
> - cinfo->chan = mbox_request_channel(cl, idx);
> - if (IS_ERR(cinfo->chan)) {
> - ret = PTR_ERR(cinfo->chan);
> - if (ret != -EPROBE_DEFER)
> - dev_err(dev, "failed to request SCMI %s mailbox\n",
> - desc);
> + ret = info->transport_ops->chan_setup(cinfo, tx);
> + if (ret)
> return ret;
> - }
>
> idr_alloc:
> ret = idr_alloc(idr, cinfo, prot_id, prot_id + 1, GFP_KERNEL); @@ -791,12
> +741,12 @@ static int scmi_mbox_chan_setup(struct scmi_info *info, struct
> device *dev, }
>
> static inline int
> -scmi_mbox_txrx_setup(struct scmi_info *info, struct device *dev, int prot_id)
> +scmi_txrx_setup(struct scmi_info *info, struct device *dev, int
> +prot_id)
> {
> - int ret = scmi_mbox_chan_setup(info, dev, prot_id, true);
> + int ret = scmi_chan_setup(info, dev, prot_id, true);
>
> if (!ret) /* Rx is optional, hence no error check */
> - scmi_mbox_chan_setup(info, dev, prot_id, false);
> + scmi_chan_setup(info, dev, prot_id, false);
>
> return ret;
> }
> @@ -814,7 +764,7 @@ scmi_create_protocol_device(struct device_node *np,
> struct scmi_info *info,
> return;
> }
>
> - if (scmi_mbox_txrx_setup(info, &sdev->dev, prot_id)) {
> + if (scmi_txrx_setup(info, &sdev->dev, prot_id)) {
> dev_err(&sdev->dev, "failed to setup transport\n");
> scmi_device_destroy(sdev);
> return;
> @@ -824,6 +774,23 @@ scmi_create_protocol_device(struct device_node
> *np, struct scmi_info *info,
> scmi_set_handle(sdev);
> }
>
> +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;
> +}
> +
> static int scmi_probe(struct platform_device *pdev) {
> int ret;
> @@ -833,12 +800,6 @@ static int scmi_probe(struct platform_device *pdev)
> struct device *dev = &pdev->dev;
> struct device_node *child, *np = dev->of_node;
>
> - /* Only mailbox method supported, check for the presence of one */
> - if (scmi_mailbox_check(np, 0)) {
> - dev_err(dev, "no mailbox found in %pOF\n", np);
> - return -EINVAL;
> - }
> -
> desc = of_device_get_match_data(dev);
> if (!desc)
> return -EINVAL;
> @@ -851,6 +812,10 @@ static int scmi_probe(struct platform_device *pdev)
> info->desc = desc;
> INIT_LIST_HEAD(&info->node);
>
> + ret = scmi_set_transport_ops(info);
> + if (ret)
> + return ret;
> +
> ret = scmi_xfer_info_init(info);
> if (ret)
> return ret;
> @@ -863,7 +828,7 @@ static int scmi_probe(struct platform_device *pdev)
> handle->dev = info->dev;
> handle->version = &info->version;
>
> - ret = scmi_mbox_txrx_setup(info, dev, SCMI_PROTOCOL_BASE);
> + ret = scmi_txrx_setup(info, dev, SCMI_PROTOCOL_BASE);
> if (ret)
> return ret;
>
> @@ -898,19 +863,9 @@ static int scmi_probe(struct platform_device *pdev)
> return 0;
> }
>
> -static int scmi_mbox_free_channel(int id, void *p, void *data)
> +void scmi_free_channel(struct scmi_chan_info *cinfo, struct idr *idr,
> +int id)
> {
> - struct scmi_chan_info *cinfo = p;
> - struct idr *idr = data;
> -
> - if (!IS_ERR_OR_NULL(cinfo->chan)) {
> - mbox_free_channel(cinfo->chan);
> - cinfo->chan = NULL;
> - }
> -
> idr_remove(idr, id);
> -
> - return 0;
> }
>
> static int scmi_remove(struct platform_device *pdev) @@ -930,11 +885,11
> @@ static int scmi_remove(struct platform_device *pdev)
> return ret;
>
> /* Safe to free channels since no more users */
> - ret = idr_for_each(idr, scmi_mbox_free_channel, idr);
> + ret = idr_for_each(idr, info->transport_ops->chan_free, idr);
> idr_destroy(&info->tx_idr);
>
> idr = &info->rx_idr;
> - ret = idr_for_each(idr, scmi_mbox_free_channel, idr);
> + ret = idr_for_each(idr, info->transport_ops->chan_free, idr);
> idr_destroy(&info->rx_idr);
>
> return ret;
> diff --git a/drivers/firmware/arm_scmi/mailbox.c
> b/drivers/firmware/arm_scmi/mailbox.c
> new file mode 100644
> index 000000000000..d9d301df5cc9
> --- /dev/null
> +++ b/drivers/firmware/arm_scmi/mailbox.c
> @@ -0,0 +1,146 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * System Control and Management Interface (SCMI) Message Mailbox
> +Transport driver
> + *
> + * Copyright (C) 2019 ARM Ltd.
> + */
> +
> +#include <linux/err.h>
> +#include <linux/device.h>
> +#include <linux/mailbox_client.h>
> +#include <linux/of.h>
> +#include <linux/slab.h>
> +
> +#include "common.h"
> +
> +/**
> + * struct scmi_mailbox - Structure representing a SCMI mailbox
> +transport
> + *
> + * @cl: Mailbox Client
> + * @chan: Transmit/Receive mailbox channel
> + * @cinfo: SCMI channel info
> + */
> +struct scmi_mailbox {
> + struct mbox_client cl;
> + struct mbox_chan *chan;
> + struct scmi_chan_info *cinfo;
> +};
> +
> +#define client_to_scmi_mailbox(c) container_of(c, struct scmi_mailbox,
> +cl)
> +
> +static bool mailbox_chan_available(struct device *dev, int idx) {
> + return !of_parse_phandle_with_args(dev->of_node, "mboxes",
> + "#mbox-cells", idx, NULL);
> +}
> +
> +static void mailbox_tx_prepare(struct mbox_client *cl, void *m) {
> + struct scmi_mailbox *smbox = client_to_scmi_mailbox(cl);
> + struct scmi_chan_info *cinfo = smbox->cinfo;
> +
> + scmi_tx_prepare(cinfo, m);
> +}
> +
> +static void mailbox_rx_callback(struct mbox_client *cl, void *m) {
> + struct scmi_mailbox *smbox = client_to_scmi_mailbox(cl);
> + struct scmi_chan_info *cinfo = smbox->cinfo;
> +
> + scmi_rx_callback(cinfo, m);
> +}
> +
> +static int mailbox_chan_setup(struct scmi_chan_info *cinfo, bool tx) {
> + struct device *dev = cinfo->dev;
> + struct scmi_mailbox *smbox;
> + struct mbox_client *cl;
> + int ret;
> +
> + smbox = devm_kzalloc(dev, sizeof(*smbox), GFP_KERNEL);
> + if (!smbox)
> + return -ENOMEM;
> +
> + cl = &smbox->cl;
> + cl->dev = dev;
> + cl->tx_prepare = tx ? mailbox_tx_prepare : NULL;
> + cl->rx_callback = mailbox_rx_callback;
> + cl->tx_block = false;
> + cl->knows_txdone = tx;
> +
> + smbox->chan = mbox_request_channel(cl, tx ? 0 : 1);
> + if (IS_ERR(smbox->chan)) {
> + ret = PTR_ERR(smbox->chan);
> + if (ret != -EPROBE_DEFER)
> + dev_err(dev, "failed to request SCMI %s mailbox\n",
> + tx ? "Tx" : "Rx");
> + return ret;
> + }
> +
> + cinfo->transport_info = smbox;
> + smbox->cinfo = cinfo;
> +
> + return 0;
> +}
> +
> +static int mailbox_chan_free(int id, void *p, void *data) {
> + struct scmi_chan_info *cinfo = p;
> + struct scmi_mailbox *smbox = cinfo->transport_info;
> +
> + if (!IS_ERR_OR_NULL(smbox->chan)) {
> + mbox_free_channel(smbox->chan);
> + cinfo->transport_info = NULL;
> + smbox->chan = NULL;
> + smbox->cinfo = NULL;
> + }
> +
> + scmi_free_channel(cinfo, data, id);
> +
> + return 0;
> +}
> +
> +static int mailbox_send_message(struct scmi_chan_info *cinfo,
> + struct scmi_xfer *xfer)
> +{
> + struct scmi_mailbox *smbox = cinfo->transport_info;
> + int ret;
> +
> + ret = mbox_send_message(smbox->chan, xfer);
> +
> + /* mbox_send_message returns non-negative value on success, so reset
> */
> + if (ret > 0)
> + ret = 0;
> +
> + return ret;
> +}
> +
> +static void mailbox_mark_txdone(struct scmi_chan_info *cinfo, int ret)
> +{
> + struct scmi_mailbox *smbox = cinfo->transport_info;
> +
> + /*
> + * 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(smbox->chan, ret);
> +}
> +
> +static struct scmi_transport_ops scmi_mailbox_ops = {
> + .chan_available = mailbox_chan_available,
> + .chan_setup = mailbox_chan_setup,
> + .chan_free = mailbox_chan_free,
> + .send_message = mailbox_send_message,
> + .mark_txdone = mailbox_mark_txdone,
> +};
> +
> +struct scmi_transport_ops *scmi_mailbox_get_ops(struct device *dev) {
> + if (!of_parse_phandle_with_args(dev->of_node, "mboxes",
> "#mbox-cells",
> + 0, NULL))
> + return &scmi_mailbox_ops;
> +
> + return NULL;
> +}
> --
> 2.21.0.rc0.269.g1a574e7a288b
>
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@xxxxxxxxxxxxxxxxxxx
> https://eur01.safelinks.protection.outlook.com/?url=http%3A%2F%2Flists.infr
> adead.org%2Fmailman%2Flistinfo%2Flinux-arm-kernel&data=02%7C01
> %7Cpeng.fan%40nxp.com%7C8fe2d24d2ab54103882208d774af1406%7C686
> ea1d3bc2b4c6fa92cd99c5c301635%7C0%7C0%7C637106167622251045&a
> mp;sdata=q74lMDic9eolz1JhL2Iple8wz1KxNEDq0BbHkh1QfRU%3D&res
> erved=0

Next message: Al Viro: "Re: Why is CONFIG_VT forced on?"
Previous message: Yang Shi: "Re: [PATCH] move_pages.2: not return ENOENT if the page are already on the target nodes"
Next in thread: Sudeep Holla: "Re: [PATCH] firmware: arm_scmi: Make scmi core independent of transport type"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]