Re: [PATCHv6 6/8] fpga: add intel stratix10 soc fpga manager driver

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Richard + Alan,

couple of small stuff inline. Sorry for the super late reply.

On Tue, Jul 3, 2018 at 7:46 AM,  <richard.gong@xxxxxxxxxxxxxxx> wrote:
> From: Alan Tull <atull@xxxxxxxxxx>
>
> Add driver for reconfiguring Intel Stratix10 SoC FPGA devices.
> This driver communicates through the Intel Service Driver which
> does communication with privileged hardware (that does the
> FPGA programming) through a secure mailbox.
>
> Signed-off-by: Alan Tull <atull@xxxxxxxxxx>
> Signed-off-by: Richard Gong <richard.gong@xxxxxxxxx>
> ---
> v2: this patch is added in patch set version 2
> v3: change to align to the update of service client APIs, and the
>     update of fpga_mgr device node
> v4: changes to align with stratix10-svc-client API updates
>     add Richard's signed-off-by
> v5: update to align changes at service layer to minimize service
>     layer thread usages
> v6: add S10_RECONFIG_TIMEOUT
>     rename s/S10_BUF_TIMEOUT/S10_BUFFER_TIMEOUT/
>     fix klocwork errors
> ---
>  drivers/fpga/Kconfig         |   6 +
>  drivers/fpga/Makefile        |   1 +
>  drivers/fpga/stratix10-soc.c | 553 +++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 560 insertions(+)
>  create mode 100644 drivers/fpga/stratix10-soc.c
>
> diff --git a/drivers/fpga/Kconfig b/drivers/fpga/Kconfig
> index ee9c542..7d20743 100644
> --- a/drivers/fpga/Kconfig
> +++ b/drivers/fpga/Kconfig
> @@ -56,6 +56,12 @@ config FPGA_MGR_ZYNQ_FPGA
>         help
>           FPGA manager driver support for Xilinx Zynq FPGAs.
>
> +config FPGA_MGR_STRATIX10_SOC
> +       tristate "Intel Stratix10 SoC FPGA Manager"
> +       depends on (ARCH_STRATIX10 && STRATIX10_SERVICE)
> +       help
> +         FPGA manager driver support for the Intel Stratix10 SoC.
> +
>  config FPGA_MGR_XILINX_SPI
>         tristate "Xilinx Configuration over Slave Serial (SPI)"
>         depends on SPI
> diff --git a/drivers/fpga/Makefile b/drivers/fpga/Makefile
> index f9803da..9f17b7f 100644
> --- a/drivers/fpga/Makefile
> +++ b/drivers/fpga/Makefile
> @@ -13,6 +13,7 @@ obj-$(CONFIG_FPGA_MGR_ICE40_SPI)      += ice40-spi.o
>  obj-$(CONFIG_FPGA_MGR_MACHXO2_SPI)     += machxo2-spi.o
>  obj-$(CONFIG_FPGA_MGR_SOCFPGA)         += socfpga.o
>  obj-$(CONFIG_FPGA_MGR_SOCFPGA_A10)     += socfpga-a10.o
> +obj-$(CONFIG_FPGA_MGR_STRATIX10_SOC)   += stratix10-soc.o
>  obj-$(CONFIG_FPGA_MGR_TS73XX)          += ts73xx-fpga.o
>  obj-$(CONFIG_FPGA_MGR_XILINX_SPI)      += xilinx-spi.o
>  obj-$(CONFIG_FPGA_MGR_ZYNQ_FPGA)       += zynq-fpga.o
> diff --git a/drivers/fpga/stratix10-soc.c b/drivers/fpga/stratix10-soc.c
> new file mode 100644
> index 0000000..74e4830
> --- /dev/null
> +++ b/drivers/fpga/stratix10-soc.c
> @@ -0,0 +1,553 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * FPGA Manager Driver for Intel Stratix10 SoC
> + *
> + *  Copyright (C) 2018 Intel Corporation
> + */
> +#include <linux/completion.h>
> +#include <linux/fpga/fpga-mgr.h>
> +#include <linux/stratix10-svc-client.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_platform.h>
> +
> +/*
> + * FPGA programming requires a higher level of privilege (EL3), per the SoC
> + * design.
> + */
> +#define NUM_SVC_BUFS   4
> +#define SVC_BUF_SIZE   SZ_512K
> +
> +/* Indicates buffer is in use if set */
> +#define SVC_BUF_LOCK   0
> +
> +#define S10_BUFFER_TIMEOUT (msecs_to_jiffies(SVC_RECONFIG_BUFFER_TIMEOUT_MS))
> +#define S10_RECONFIG_TIMEOUT (msecs_to_jiffies(SVC_RECONFIG_REQUEST_TIMEOUT_MS))
> +
> +/**
> + * struct s10_svc_buf
> + * @buf: virtual address of buf provided by service layer
> + * @lock: locked if buffer is in use
> + */
> +struct s10_svc_buf {
> +       char *buf;
> +       unsigned long lock;
> +};
> +
> +struct s10_priv {
> +       struct stratix10_svc_chan *chan;
> +       struct stratix10_svc_client client;
> +       struct completion status_return_completion;
> +       struct s10_svc_buf svc_bufs[NUM_SVC_BUFS];
> +       unsigned long status;
> +};
> +
> +static int s10_svc_send_msg(struct s10_priv *priv,
> +                           enum stratix10_svc_command_code command,
> +                           void *payload, u32 payload_length)
> +{
> +       struct stratix10_svc_chan *chan = priv->chan;
> +       struct stratix10_svc_client_msg msg;
> +       int ret;
> +
> +       pr_debug("%s cmd=%d payload=%p legnth=%d\n",
> +                __func__, command, payload, payload_length);

Can we make those dev_dbg() ? Or do we not have a device available?
> +
> +       msg.command = command;
> +       msg.payload = payload;
> +       msg.payload_length = payload_length;
> +
> +       ret = stratix10_svc_send(chan, &msg);
> +       pr_debug("stratix10_svc_send returned status %d\n", ret);
> +
> +       return ret;
> +}
> +
> +/**
> + * s10_free_buffers
> + * Free buffers allocated from the service layer's pool that are not in use.
> + * @mgr: fpga manager struct
> + * Free all buffers that are not in use.
> + * Return true when all buffers are freed.
> + */
> +static bool s10_free_buffers(struct fpga_manager *mgr)
> +{
> +       struct s10_priv *priv = mgr->priv;
> +       uint num_free = 0;
> +       uint i;
> +
> +       for (i = 0; i < NUM_SVC_BUFS; i++) {
> +               if (!priv->svc_bufs[i].buf) {
> +                       num_free++;
> +                       continue;
> +               }
> +
> +               if (!test_and_set_bit_lock(SVC_BUF_LOCK,
> +                                          &priv->svc_bufs[i].lock)) {
> +                       stratix10_svc_free_memory(priv->chan,
> +                                             priv->svc_bufs[i].buf);
> +                       priv->svc_bufs[i].buf = NULL;
> +                       num_free++;
> +               }
> +       }
> +
> +       return num_free == NUM_SVC_BUFS;
> +}
> +
> +/**
> + * s10_free_buffer_count
> + * Count how many buffers are not in use.
> + * @mgr: fpga manager struct
> + * Return # of buffers that are not in use.
> + */
> +static uint s10_free_buffer_count(struct fpga_manager *mgr)
> +{
> +       struct s10_priv *priv = mgr->priv;
> +       uint num_free = 0;
> +       uint i;
> +
> +       for (i = 0; i < NUM_SVC_BUFS; i++)
> +               if (!priv->svc_bufs[i].buf)
> +                       num_free++;
> +
> +       return num_free;
> +}
> +
> +/**
> + * s10_unlock_bufs
> + * Given the returned buffer address, match that address to our buffer struct
> + * and unlock that buffer.  This marks it as available to be refilled and sent
> + * (or freed).
> + * @priv: private data
> + * @kaddr: kernel address of buffer that was returned from service layer
> + */
> +static void s10_unlock_bufs(struct s10_priv *priv, void *kaddr)
> +{
> +       uint i;
> +
> +       if (!kaddr)
> +               return;
> +
> +       for (i = 0; i < NUM_SVC_BUFS; i++)
> +               if (priv->svc_bufs[i].buf == kaddr) {
> +                       clear_bit_unlock(SVC_BUF_LOCK,
> +                                        &priv->svc_bufs[i].lock);
> +                       return;
> +               }
> +
> +       WARN(1, "Unknown buffer returned from service layer %p\n", kaddr);
> +}
> +
> +/**
> + * s10_receive_callback
> + * Callback for service layer to use to provide client (this driver) messages
> + * received through the mailbox.
> + * @client: service layer client struct
> + * @data: message
> + */
> +static void s10_receive_callback(struct stratix10_svc_client *client,
> +                                struct stratix10_svc_cb_data *data)
> +{
> +       struct s10_priv *priv = client->priv;
> +       u32 status;
> +       int i;
> +
> +       WARN_ONCE(!data, "%s: stratix10_svc_rc_data = NULL", __func__);
> +
> +       status = data->status;
> +
> +       /*
> +        * Here we set status bits as we receive them.  Elsewhere, we always use
> +        * test_and_clear_bit() to check status in priv->status
> +        */
> +       for (i = 0; i <= SVC_STATUS_RECONFIG_ERROR; i++)
> +               if (status & (1 << i))
> +                       set_bit(i, &priv->status);
> +
> +       if (status & BIT(SVC_STATUS_RECONFIG_BUFFER_DONE)) {
> +               s10_unlock_bufs(priv, data->kaddr1);
> +               s10_unlock_bufs(priv, data->kaddr2);
> +               s10_unlock_bufs(priv, data->kaddr3);
> +       }
> +
> +       complete(&priv->status_return_completion);
> +}
> +
> +/**
> + * s10_ops_write_init
> + * Prepare for FPGA reconfiguration by requesting partial reconfig and
> + * allocating buffers from the service layer.
> + * @mgr: fpga manager
> + * @info: fpga image info
> + * @buf: fpga image buffer
> + * @count: size of buf in bytes
> + */
> +static int s10_ops_write_init(struct fpga_manager *mgr,
> +                             struct fpga_image_info *info,
> +                             const char *buf, size_t count)
> +{
> +       struct s10_priv *priv = mgr->priv;
> +       struct device *dev = priv->client.dev;
> +       struct stratix10_svc_command_reconfig_payload payload;
> +       char *kbuf;
> +       uint i;
> +       int ret;
> +
> +       payload.flags = 0;
> +       if (info->flags & FPGA_MGR_PARTIAL_RECONFIG) {
> +               dev_info(dev, "Requesting partial reconfiguration.\n");
Do we need these to be _info() or can the by _dbg()? The Framework
already prints ("Writing blah.foo to ...")
> +               payload.flags |= BIT(COMMAND_RECONFIG_FLAG_PARTIAL);
> +       } else {
> +               dev_info(dev, "Requesting full reconfiguration.\n");
Same here.
> +       }
> +
> +       reinit_completion(&priv->status_return_completion);
> +       ret = s10_svc_send_msg(priv, COMMAND_RECONFIG,
> +                              &payload, sizeof(payload));
> +       if (ret < 0)
> +               goto init_done;
> +
> +       ret = wait_for_completion_interruptible_timeout(
> +               &priv->status_return_completion, S10_RECONFIG_TIMEOUT);
> +       if (!ret) {
> +               dev_err(dev, "timeout waiting for RECONFIG_REQUEST\n");
> +               ret = -ETIMEDOUT;
> +               goto init_done;
> +       }
> +       if (ret < 0) {
> +               dev_err(dev, "error (%d) waiting for RECONFIG_REQUEST\n", ret);
> +               goto init_done;
> +       }
> +
> +       ret = 0;
> +       if (!test_and_clear_bit(SVC_STATUS_RECONFIG_REQUEST_OK,
> +                               &priv->status)) {
> +               ret = -ETIMEDOUT;
> +               goto init_done;
> +       }
> +
> +       /* Allocate buffers from the service layer's pool. */
> +       for (i = 0; i < NUM_SVC_BUFS; i++) {
> +               kbuf = stratix10_svc_allocate_memory(priv->chan, SVC_BUF_SIZE);
> +               if (!kbuf) {
> +                       s10_free_buffers(mgr);
> +                       ret = -ENOMEM;
> +                       goto init_done;
> +               }
> +
> +               priv->svc_bufs[i].buf = kbuf;
> +               priv->svc_bufs[i].lock = 0;
> +       }
> +
> +init_done:
> +       stratix10_svc_done(priv->chan);
> +       return ret;
> +}
> +
> +/**
> + * s10_send_buf
> + * Send a buffer to the service layer queue
> + * @mgr: fpga manager struct
> + * @buf: fpga image buffer
> + * @count: size of buf in bytes
> + * Returns # of bytes transferred or -ENOBUFS if the all the buffers are in use
> + * or if the service queue is full. Never returns 0.
> + */
> +static int s10_send_buf(struct fpga_manager *mgr, const char *buf, size_t count)
> +{
> +       struct s10_priv *priv = mgr->priv;
> +       struct device *dev = priv->client.dev;
> +       void *svc_buf;
> +       size_t xfer_sz;
> +       int ret;
> +       uint i;
> +
> +       /* get/lock a buffer that that's not being used */
> +       for (i = 0; i < NUM_SVC_BUFS; i++)
> +               if (!test_and_set_bit_lock(SVC_BUF_LOCK,
> +                                          &priv->svc_bufs[i].lock))
> +                       break;
> +
> +       if (i == NUM_SVC_BUFS)
> +               return -ENOBUFS;
> +
> +       xfer_sz = count < SVC_BUF_SIZE ? count : SVC_BUF_SIZE;
> +
> +       svc_buf = priv->svc_bufs[i].buf;
> +       memcpy(svc_buf, buf, xfer_sz);
> +       ret = s10_svc_send_msg(priv, COMMAND_RECONFIG_DATA_SUBMIT,
> +                              svc_buf, xfer_sz);
> +       if (ret < 0) {
> +               dev_err(dev,
> +                       "Error while sending data to service layer (%d)", ret);
> +               return ret;
> +       }
Do we need to clean up anything here, or is the buffer unlocked if we fail?
> +
> +       return xfer_sz;
> +}
> +
> +/**
> + * s10_ops_write
> + * Send a FPGA image to privileged layers to write to the FPGA.  When done
> + * sending, free all service layer buffers we allocated in write_init.
> + * @mgr: fpga manager
> + * @buf: fpga image buffer
> + * @count: size of buf in bytes
> + * Returns 0 for success or negative errno.
> + */
> +static int s10_ops_write(struct fpga_manager *mgr, const char *buf,
> +                        size_t count)
> +{
> +       struct s10_priv *priv = mgr->priv;
> +       struct device *dev = priv->client.dev;
> +       long wait_status;
> +       int sent = 0;
> +       int ret = 0;
> +
> +       /*
> +        * Loop waiting for buffers to be returned.  When a buffer is returned,
> +        * reuse it to send more data or free if if all data has been sent.
> +        */
> +       while (count > 0 || s10_free_buffer_count(mgr) != NUM_SVC_BUFS) {
> +               reinit_completion(&priv->status_return_completion);
> +
> +               if (count > 0) {
> +                       sent = s10_send_buf(mgr, buf, count);
> +                       if (sent < 0)
> +                               continue;
> +
> +                       count -= sent;
> +                       buf += sent;
> +               } else {
> +                       if (s10_free_buffers(mgr))
> +                               return 0;
> +
> +                       ret = s10_svc_send_msg(
> +                               priv, COMMAND_RECONFIG_DATA_CLAIM,
> +                               NULL, 0);
> +                       if (ret < 0)
> +                               break;
> +               }
> +
> +               /*
> +                * If callback hasn't already happened, wait for buffers to be
> +                * returned from service layer
> +                */
> +               wait_status = 1; /* not timed out */
> +               if (!priv->status)
> +                       wait_status = wait_for_completion_interruptible_timeout(
> +                               &priv->status_return_completion,
> +                               S10_BUFFER_TIMEOUT);
> +
> +               if (test_and_clear_bit(SVC_STATUS_RECONFIG_BUFFER_DONE,
> +                                      &priv->status) ||
> +                   test_and_clear_bit(SVC_STATUS_RECONFIG_BUFFER_SUBMITTED,
> +                                      &priv->status)) {
> +                       ret = 0;
> +                       continue;
> +               }
> +
> +               if (test_and_clear_bit(SVC_STATUS_RECONFIG_ERROR,
> +                                      &priv->status)) {
> +                       dev_err(dev, "ERROR - giving up - SVC_STATUS_RECONFIG_ERROR\n");
> +                       ret = -EFAULT;
> +                       break;
> +               }
> +
> +               if (!wait_status) {
> +                       dev_err(dev, "timeout waiting for svc layer buffers\n");
> +                       ret = -ETIMEDOUT;
> +                       break;
> +               }
> +               if (wait_status < 0) {
> +                       ret = wait_status;
> +                       dev_err(dev,
> +                               "error (%d) waiting for svc layer buffers\n",
> +                               ret);
> +                       break;
> +               }
> +       }
> +
> +       if (!s10_free_buffers(mgr))
> +               dev_err(dev, "%s not all buffers were freed\n", __func__);
> +
> +       return ret;
> +}
> +
> +/**
> + * s10_ops_write_complete
> + * Wait for FPGA configuration to be done
> + * @mgr: fpga manager
> + * @info: fpga image info
> + * Returns 0 for success negative errno.
> + */
> +static int s10_ops_write_complete(struct fpga_manager *mgr,
> +                                 struct fpga_image_info *info)
> +{
> +       struct s10_priv *priv = mgr->priv;
> +       struct device *dev = priv->client.dev;
> +       unsigned long timeout;
> +       int ret;
> +
> +       timeout = usecs_to_jiffies(info->config_complete_timeout_us);
> +
> +       do {
> +               reinit_completion(&priv->status_return_completion);
> +
> +               ret = s10_svc_send_msg(priv, COMMAND_RECONFIG_STATUS, NULL, 0);
> +               if (ret < 0)
> +                       break;
> +
> +               ret = wait_for_completion_interruptible_timeout(
> +                       &priv->status_return_completion, timeout);
> +               if (!ret) {
> +                       dev_err(dev,
> +                               "timeout waiting for RECONFIG_COMPLETED\n");
> +                       ret = -ETIMEDOUT;
> +                       break;
> +               }
> +               if (ret < 0) {
> +                       dev_err(dev,
> +                               "error (%d) waiting for RECONFIG_COMPLETED\n",
> +                               ret);
> +                       break;
> +               }
> +               /* Not error or timeout, so ret is # of jiffies until timeout */
> +               timeout = ret;
> +               ret = 0;
> +
> +               if (test_and_clear_bit(SVC_STATUS_RECONFIG_COMPLETED,
> +                                      &priv->status))
> +                       break;
> +
> +               if (test_and_clear_bit(SVC_STATUS_RECONFIG_ERROR,
> +                                      &priv->status)) {
> +                       dev_err(dev, "ERROR - giving up - SVC_STATUS_RECONFIG_ERROR\n");
> +                       ret = -EFAULT;
> +                       break;
> +               }
> +       } while (1);
> +
> +       stratix10_svc_done(priv->chan);
> +
> +       return ret;
> +}
> +
> +static enum fpga_mgr_states s10_ops_state(struct fpga_manager *mgr)
> +{
> +       return FPGA_MGR_STATE_UNKNOWN;
> +}
> +
> +static const struct fpga_manager_ops s10_ops = {
> +       .state = s10_ops_state,
> +       .write_init = s10_ops_write_init,
> +       .write = s10_ops_write,
> +       .write_complete = s10_ops_write_complete,
> +};
> +
> +static int s10_probe(struct platform_device *pdev)
> +{
> +       struct device *dev = &pdev->dev;
> +       struct s10_priv *priv;
> +       struct fpga_manager *mgr;
> +       int ret;
> +
> +       priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +       if (!priv)
> +               return -ENOMEM;
> +
> +       priv->client.dev = dev;
> +       priv->client.receive_cb = s10_receive_callback;
> +       priv->client.priv = priv;
> +
> +       priv->chan = stratix10_svc_request_channel_byname(&priv->client,
> +                                               SVC_CLIENT_FPGA);
> +       if (IS_ERR(priv->chan)) {
> +               dev_err(dev, "couldn't get service channel (%s)\n",
> +                       SVC_CLIENT_FPGA);
> +               return PTR_ERR(priv->chan);
> +       }
> +
> +       init_completion(&priv->status_return_completion);
> +
> +       mgr = fpga_mgr_create(dev, "Stratix10 SOC FPGA Manager",
> +                             &s10_ops, priv);
> +       if (!mgr)
> +               return -ENOMEM;
Does this release the channel above?
> +
> +       ret = fpga_mgr_register(mgr);
> +       if (ret) {
> +               dev_err(dev, "unable to register FPGA manager\n");
> +               stratix10_svc_free_channel(priv->chan);
> +               fpga_mgr_free(mgr);
> +               return ret;
> +       }
> +
> +       platform_set_drvdata(pdev, mgr);
> +
> +       return ret;
> +}
> +
> +static int s10_remove(struct platform_device *pdev)
> +{
> +       struct fpga_manager *mgr = platform_get_drvdata(pdev);
> +       struct s10_priv *priv = mgr->priv;
> +
> +       fpga_mgr_unregister(mgr);
> +       stratix10_svc_free_channel(priv->chan);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id s10_of_match[] = {
> +       { .compatible = "intel,stratix10-soc-fpga-mgr", },
> +       {},
> +};
> +
> +MODULE_DEVICE_TABLE(of, s10_of_match);
> +
> +static struct platform_driver s10_driver = {
> +       .probe = s10_probe,
> +       .remove = s10_remove,
> +       .driver = {
> +               .name   = "Stratix10 SoC FPGA manager",
> +               .of_match_table = of_match_ptr(s10_of_match),
> +       },
> +};
> +
> +static int __init s10_init(void)
> +{
> +       struct device_node *fw_np;
> +       struct device_node *np;
> +       int ret;
> +
> +       fw_np = of_find_node_by_name(NULL, "svc");
> +       if (!fw_np)
> +               return -ENODEV;
> +
> +       np = of_find_matching_node(fw_np, s10_of_match);
> +       if (!np) {
> +               of_node_put(fw_np);
> +               return -ENODEV;
> +       }
> +
> +       of_node_put(np);
> +       ret = of_platform_populate(fw_np, s10_of_match, NULL, NULL);
> +       of_node_put(fw_np);
> +       if (ret)
> +               return ret;
> +
> +       return platform_driver_register(&s10_driver);
> +}
> +
> +static void __exit s10_exit(void)
> +{
> +       return platform_driver_unregister(&s10_driver);
> +}
> +
> +module_init(s10_init);
> +module_exit(s10_exit);
> +
> +MODULE_AUTHOR("Alan Tull <atull@xxxxxxxxxx>");
> +MODULE_DESCRIPTION("Intel Stratix 10 SOC FPGA Manager");
> +MODULE_LICENSE("GPL v2");
> --
> 2.7.4
>

Cheers,
Moritz
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Device Tree Compilter]     [Device Tree Spec]     [Linux Driver Backports]     [Video for Linux]     [Linux USB Devel]     [Linux PCI Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Yosemite Backpacking]


  Powered by Linux