| Message ID | 5c545c2866ba075ddb44907940a1dae1d823b8a1.1575019719.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | firmware: arm_scmi: Make scmi core independent of transport type | expand |
On Fri, Nov 29, 2019 at 03:01:39PM +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 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. > The implementation looks fine to me. > We can now implement more transport protocols to transport SCMI > messages. > I am more interested in this part. As I am aware the only 2 other transport being discussed is SMC/HVC and new/yet conceptual SPCI(built on top of SMC/HVC). There are already discussions on the list to make former as mailbox[1]. While I see both pros and cons with that approach, there's a need to converge. One main advantage I see with SMC/HVC mailbox is that it can be used with any other client and not just SCMI. Equally, the queuing in the mailbox may not be needed with fast SMC/HVC but may be needed for new SPCI(not yet fully analysed). > The transport protocols just need to provide struct scmi_transport_ops, > with its version of the callbacks to enable exchange of SCMI messages. > As I mentioned I am fine with implementation in this patch. But I would like to hear especially from Arnd and Jassi as the abstraction look more like mailbox APIs themselves and may look like duplication. I don't want people to realise late that this is not good idea for whatever reasons. If we have valid and enough reasons to do so, we can take this approach. I really need some feedback here. -- Regards, Sudeep [1] https://lore.kernel.org/lkml/1575281525-1549-1-git-send-email-peng.fan@nxp.com
Hi a one minor nit, and one question about scmi_desc usage in this new transport independent driver. On 29/11/2019 09:31, 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 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> > --- > 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 > [snip] > /** > * 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; > }; > Could we add also the related @transport_ops in the above comment block ? > -#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) > > /* [snip] > + > 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; This scmi_desc struct descriptor is retrieved from of_match_table .data and points to the driver-provided scmi_generic_desc static const struct scmi_desc scmi_generic_desc = { .max_rx_timeout_ms = 30, /* We may increase this if required */ .max_msg = 20, /* Limited by MBOX_TX_QUEUE_LEN */ .max_msg_size = 128, }; Is not this kind of information possibly (maybe partially) related to the selected transport, and as such it should be also provided dynamically by the chosen transport layer at probe time, like the transport_ops, instead of being hard-coded in this driver ? Thanks Cristian
On 09-12-19, 18:13, Cristian Marussi wrote: > On 29/11/2019 09:31, Viresh Kumar wrote: > > /** > > * 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; > > }; > > > > Could we add also the related @transport_ops in the above comment block ? Ah, I missed that. > > desc = of_device_get_match_data(dev); > > if (!desc) > > return -EINVAL; > > This scmi_desc struct descriptor is retrieved from of_match_table .data and points to > the driver-provided scmi_generic_desc > > static const struct scmi_desc scmi_generic_desc = { > .max_rx_timeout_ms = 30, /* We may increase this if required */ > .max_msg = 20, /* Limited by MBOX_TX_QUEUE_LEN */ > .max_msg_size = 128, > }; > > Is not this kind of information possibly (maybe partially) related to the selected > transport, and as such it should be also provided dynamically by the chosen transport > layer at probe time, like the transport_ops, instead of being hard-coded in > this driver ? I had my doubts about this thing and I missed checking it out. @Sudeep: Is this information completely mailbox specific ? Should I move it to mailbox.c here ?
On 03-12-19, 12:00, Sudeep Holla wrote: > I am more interested in this part. As I am aware the only 2 other > transport being discussed is SMC/HVC and new/yet conceptual SPCI(built > on top of SMC/HVC). There are already discussions on the list to make > former as mailbox[1]. While I see both pros and cons with that approach, > there's a need to converge. One main advantage I see with SMC/HVC mailbox > is that it can be used with any other client and not just SCMI. Equally, > the queuing in the mailbox may not be needed with fast SMC/HVC but may > be needed for new SPCI(not yet fully analysed). We were also looking for OPTEE based mailbox which is similar to SPCI.
On Tue, Dec 10, 2019 at 11:04:48AM +0530, Viresh Kumar wrote: > On 09-12-19, 18:13, Cristian Marussi wrote: > > On 29/11/2019 09:31, Viresh Kumar wrote: [...] > > > desc = of_device_get_match_data(dev); > > > if (!desc) > > > return -EINVAL; > > > > This scmi_desc struct descriptor is retrieved from of_match_table .data and points to > > the driver-provided scmi_generic_desc > > > > static const struct scmi_desc scmi_generic_desc = { > > .max_rx_timeout_ms = 30, /* We may increase this if required */ > > .max_msg = 20, /* Limited by MBOX_TX_QUEUE_LEN */ > > .max_msg_size = 128, > > }; > > > > Is not this kind of information possibly (maybe partially) related to the selected > > transport, and as such it should be also provided dynamically by the chosen transport > > layer at probe time, like the transport_ops, instead of being hard-coded in > > this driver ? > > I had my doubts about this thing and I missed checking it out. > > @Sudeep: Is this information completely mailbox specific ? Should I move it to > mailbox.c here ? > May be to some/small extent. 1. max_rx_timeout_ms is firmware dependent, maximum time it expects to complete a synchronous request or acknowledge async request(worst case value). 2. max_msg_size is maximum size of the buffer we need to allocate, mostly based on the specification and we don't have any more that 0x80. But the custom/vendor specific protocols may wary and hence I thought of keeping it configurable for platforms. 3. max_msg is the maximum number of messages the transport can support. This is currently based on the mailbox layer. For SMC/HVC we can have upto nr_cpus, something different for spci/optee. We must be able to make it transport independent if required. This is mainly used to pre-allocate number of (tx/rx) buffers. -- Regards, Sudeep
On 10-12-19, 18:46, Sudeep Holla wrote: > May be to some/small extent. > > 1. max_rx_timeout_ms is firmware dependent, maximum time it expects to > complete a synchronous request or acknowledge async request(worst case value). > 2. max_msg_size is maximum size of the buffer we need to allocate, mostly > based on the specification and we don't have any more that 0x80. But > the custom/vendor specific protocols may wary and hence I thought of > keeping it configurable for platforms. > 3. max_msg is the maximum number of messages the transport can support. > This is currently based on the mailbox layer. For SMC/HVC we can have > upto nr_cpus, something different for spci/optee. We must be able to > make it transport independent if required. > > This is mainly used to pre-allocate number of (tx/rx) buffers. So we can only move max_msg to mailbox file and read it along with ops, right ?
> 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@linaro.org> > --- > 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@lists.infradead.org > 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
On Tue, Dec 31, 2019 at 02:50:40AM +0000, Peng Fan wrote: > > > 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? > There's a requirement for virtio based transport too. I need to do a thorough review once I am able to gather the details. Feel free to add SMC based transport based on this patch if you can, you need not wait for me. I am fine with the approach as such. Also I was waiting to get some feedback from Arnd or Jassi. -- Regards, Sudeep
On Fri, Nov 29, 2019 at 3: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. > We can either add new transport layer between SCMI and Mailbox layers, or we can write new transport as a mailbox driver (which I always thought could be a usecase). Right now I am of no strong opinion either way. Depends, what other transport do you have in mind? Cheers!
On Tue, Dec 31, 2019 at 02:09:27PM -0600, Jassi Brar wrote: > On Fri, Nov 29, 2019 at 3: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. > > > We can either add new transport layer between SCMI and Mailbox layers, > or we can write new transport as a mailbox driver (which I always > thought could be a usecase). Right now I am of no strong opinion > either way. Depends, what other transport do you have in mind? > To be more clear, this patch abstracts the SCMI transport so that mailbox can be one of the transport. The plan is to add SMC/HVC, SMC/HVC over SPCI, vitio based transport as alternative to mailbox. These are neither added as mailbox driver nor transport layer between SCMI and Mailbox. E.g.: we either use Peng's SMC based mailbox driver as is or add a new transport independent of mailbox framework here as SCMI transport. -- Regards, Sudeep
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. > +/** > + * 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 > +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. > +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; > + } There is something wrong if smbox->chan can be be one of three things (a valid pointer, a NULL pointer, or an error value). I see this is a preexisting problem, but please add a patch to make it consistently use either NULL pointers or error codes and remove all instances of IS_ERR_OR_NULL() from this subsystem. Arnd
On 09-01-20, 09:18, Arnd Bergmann wrote: > > +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; > > + } > > There is something wrong if smbox->chan can be be one of > three things (a valid pointer, a NULL pointer, or an error value). > > I see this is a preexisting problem, but please add a patch to > make it consistently use either NULL pointers or error codes > and remove all instances of IS_ERR_OR_NULL() from this > subsystem. This isn't a subsystem problem actually. mbox_request_channel() never returns NULL on error. @Sudeep, do we really need the IS_ERR_OR_NULL() check in scmi_mbox_free_channel() helper ? Or can it just be IS_ERR() ?
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. So should this be done like: compatible = "arm,scmi", "arm,scmi-mailbox"; or just compatible = "arm,scmi-mailbox"; ?
On Thu, Jan 9, 2020 at 10:34 AM Viresh Kumar <viresh.kumar@linaro.org> 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: > > 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"; I would keep compatibility with the existing binding and make a plain "arm,scmi" mean the version with the mailbox, while for new transports, I would require them to have both the existing compatible string and a more specific one. Arnd
On Thu, Jan 09, 2020 at 02:46:13PM +0530, Viresh Kumar wrote: > On 09-01-20, 09:18, Arnd Bergmann wrote: > > > +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; > > > + } > > > > There is something wrong if smbox->chan can be be one of > > three things (a valid pointer, a NULL pointer, or an error value). > > > > I see this is a preexisting problem, but please add a patch to > > make it consistently use either NULL pointers or error codes > > and remove all instances of IS_ERR_OR_NULL() from this > > subsystem. > > This isn't a subsystem problem actually. mbox_request_channel() never > returns NULL on error. > > @Sudeep, do we really need the IS_ERR_OR_NULL() check in > scmi_mbox_free_channel() helper ? Or can it just be IS_ERR() ? > It can be just IS_ERR, just not noticed it so far I believe. -- Regards, Sudeep
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 ? > Even if we don't add new transport right away, I would like to see if the requirements are met. I will take a look at you v2 with that in mind anyways. We need not wait, we I want to see people think it meets their requirement. I will also add couple of guys working on virtio transport for SCMI when I respond to your v2. Thanks for posting it. > @Peng ? > Peng, Did you get a chance to try this with SMC ? If SCMI was the only usecase, you can try this approach instead of mailbox, now that no one has any objects to this approach conceptually. Please use v2 as base and update us. -- Regards, Sudeep
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
Hi Sudeep, > Subject: Re: [PATCH] firmware: arm_scmi: Make scmi core independent of > transport type > > 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 ? > > > > Even if we don't add new transport right away, I would like to see if the > requirements are met. I will take a look at you v2 with that in mind anyways. > We need not wait, we I want to see people think it meets their requirement. I > will also add couple of guys working on virtio transport for SCMI when I > respond to your v2. Thanks for posting it. > > > @Peng ? > > > Peng, Did you get a chance to try this with SMC ? If SCMI was the only > usecase, you can try this approach instead of mailbox, now that no one has > any objects to this approach conceptually. Please use v2 as base and update > us. I will try that, but might be a bit later. Thanks, Peng. > > -- > Regards, > Sudeep
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; +}
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> --- 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