[PATCHv1] Add Intel Stratix10 service layer driver

[PATCHv1] Add Intel Stratix10 service layer driver

[richard.gong]

From: Richard Gong <richard.gong@intel.com>

Intel Stratix10 SoC is composed of a 64 bit quad-core ARM Cortex A53 hard
processor system (HPS) and Secure Device Manager (SDM). SDM is the hardware
which does the FPGA configuration, QSPI, Crypto and warm reset.

When the FPGA is configured from HPS, there needs to be a way for HPS to
notify SDM the location and size of the configuration data. Then SDM will
get the configuration data from that location and perform the FPGA configuration.

To meet the whole system security needs and support virtual machine
requesting communication with SDM, only the secure world of software (EL3,
Exception Level 3) can interface with SDM. All software entities running
on other exception levels must channel through the EL3 software whenever it
needs service from SDM.

Intel Stratix10 service layer driver is added to provide the service for
FPGA configuration. Running at privileged exception level (EL1, Exception
Level 1), Intel Stratix10 service layer driver interfaces with the service
provider at EL1 (Intel Stratix10 FPGA Manager) and manages secure monitor
call (SMC) to communicate with secure monitor software at secure monitor
exception level (EL3).

Later the Intel Stratix10 service layer driver will be extended to provide
services for QSPI, Crypto and warm reset.

Richard Gong (1):
  driver: misc: add Intel Stratix10 service layer driver

 drivers/misc/Kconfig                       |   3 +-
 drivers/misc/Makefile                      |   3 +-
 drivers/misc/intel-service/Kconfig         |   9 +
 drivers/misc/intel-service/Makefile        |   2 +
 drivers/misc/intel-service/intel_service.c | 703 +++++++++++++++++++++++++++++
 include/linux/intel-service-client.h       | 227 ++++++++++
 include/linux/intel-service.h              | 122 +++++
 include/linux/intel-smc.h                  | 246 ++++++++++
 8 files changed, 1313 insertions(+), 2 deletions(-)
 create mode 100644 drivers/misc/intel-service/Kconfig
 create mode 100644 drivers/misc/intel-service/Makefile
 create mode 100644 drivers/misc/intel-service/intel_service.c
 create mode 100644 include/linux/intel-service-client.h
 create mode 100644 include/linux/intel-service.h
 create mode 100644 include/linux/intel-smc.h

-- 
2.7.4

[PATCHv1] driver: misc: add Intel Stratix10 service layer driver

[richard.gong]

From: Richard Gong <richard.gong@intel.com>

Intel Stratix10 SoC is composed of a 64 bit quad-core ARM Cortex A53 hard
processor system (HPS) and Secure Device Manager (SDM). SDM is the
hardware which does the FPGA configuration, QSPI, Crypto and warm reset.

When the FPGA is configured from HPS, there needs to be a way for HPS to
notify SDM the location and size of the configuration data. Then SDM will
get the configuration data from that location and perform the FPGA
configuration.

To meet the whole system security needs and support virtual machine
requesting communication with SDM, only the secure world of software (EL3,
Exception Level 3) can interface with SDM. All software entities running
on other exception levels must channel through the EL3 software whenever
it needs service from SDM.

Intel Stratix10 service layer driver is added to provide the service for
FPGA configuration. Running at privileged exception level (EL1, Exception
Level 1), Intel Stratix10 service layer driver interfaces with the service
provider at EL1 (Intel Stratix10 FPGA Manager) and manages secure monitor
call (SMC) to communicate with secure monitor software at secure monitor
exception level (EL3).

Signed-off-by: Richard Gong <richard.gong@intel.com>
---
 drivers/misc/Kconfig                       |   3 +-
 drivers/misc/Makefile                      |   3 +-
 drivers/misc/intel-service/Kconfig         |   9 +
 drivers/misc/intel-service/Makefile        |   2 +
 drivers/misc/intel-service/intel_service.c | 703 +++++++++++++++++++++++++++++
 include/linux/intel-service-client.h       | 227 ++++++++++
 include/linux/intel-service.h              | 122 +++++
 include/linux/intel-smc.h                  | 246 ++++++++++
 8 files changed, 1313 insertions(+), 2 deletions(-)
 create mode 100644 drivers/misc/intel-service/Kconfig
 create mode 100644 drivers/misc/intel-service/Makefile
 create mode 100644 drivers/misc/intel-service/intel_service.c
 create mode 100644 include/linux/intel-service-client.h
 create mode 100644 include/linux/intel-service.h
 create mode 100644 include/linux/intel-smc.h

diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
index 4c842e8..eaca4bb 100644
--- a/drivers/misc/Kconfig
+++ b/drivers/misc/Kconfig
@@ -1,4 +1,4 @@
-#
+# SPDX-License-Identifier: GPL-2.0
 # Misc strange devices
 #
 
@@ -528,6 +528,7 @@ source "drivers/misc/eeprom/Kconfig"
 source "drivers/misc/cb710/Kconfig"
 source "drivers/misc/ti-st/Kconfig"
 source "drivers/misc/lis3lv02d/Kconfig"
+source "drivers/misc/intel-service/Kconfig"
 source "drivers/misc/altera-stapl/Kconfig"
 source "drivers/misc/mei/Kconfig"
 source "drivers/misc/vmw_vmci/Kconfig"
diff --git a/drivers/misc/Makefile b/drivers/misc/Makefile
index e07c116..6ee779d 100644
--- a/drivers/misc/Makefile
+++ b/drivers/misc/Makefile
@@ -1,4 +1,4 @@
-#
+# SPDX-License-Identifier: GPL-2.0
 # Makefile for misc devices that really don't fit anywhere else.
 #
 
@@ -58,6 +58,7 @@ obj-$(CONFIG_CXL_BASE)		+= cxl/
 obj-$(CONFIG_ASPEED_LPC_CTRL)	+= aspeed-lpc-ctrl.o
 obj-$(CONFIG_ASPEED_LPC_SNOOP)	+= aspeed-lpc-snoop.o
 obj-$(CONFIG_PCI_ENDPOINT_TEST)	+= pci_endpoint_test.o
+obj-$(CONFIG_INTEL_SERVICE)    	+= intel-service/
 
 lkdtm-$(CONFIG_LKDTM)		+= lkdtm_core.o
 lkdtm-$(CONFIG_LKDTM)		+= lkdtm_bugs.o
diff --git a/drivers/misc/intel-service/Kconfig b/drivers/misc/intel-service/Kconfig
new file mode 100644
index 0000000..69ce397
--- /dev/null
+++ b/drivers/misc/intel-service/Kconfig
@@ -0,0 +1,9 @@
+# SPDX-License-Identifier: GPL-2.0
+config INTEL_SERVICE
+	tristate "Intel Service Controller"
+	default y
+	depends on ARCH_STRATIX10
+	help
+	 An implementation of Intel service controller. It is used to provide service to
+	 FPGA manager on Intel Stratix 10 SOC for FPGA configuration. Say Y here if you
+	 want Intel service controller support
diff --git a/drivers/misc/intel-service/Makefile b/drivers/misc/intel-service/Makefile
new file mode 100644
index 0000000..9509b91
--- /dev/null
+++ b/drivers/misc/intel-service/Makefile
@@ -0,0 +1,2 @@
+# SPDX-License-Identifier: GPL-2.0
+obj-y += intel_service.o
diff --git a/drivers/misc/intel-service/intel_service.c b/drivers/misc/intel-service/intel_service.c
new file mode 100644
index 0000000..90f883c
--- /dev/null
+++ b/drivers/misc/intel-service/intel_service.c
@@ -0,0 +1,703 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Copyright (C) 2017-2018, Intel Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#include <linux/bitops.h>
+#include <linux/completion.h>
+#include <linux/delay.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/intel-service.h>
+#include <linux/intel-service-client.h>
+#include <linux/intel-smc.h>
+#include <linux/interrupt.h>
+#include <linux/io.h>
+#include <linux/kthread.h>
+#include <linux/module.h>
+#include <linux/mutex.h>
+#include <linux/of_address.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+#define SVC_FO_LEN	32
+#define SVC_MAX_CHANNEL	2
+
+static LIST_HEAD(svc_cons);
+static LIST_HEAD(svc_private_mem);
+static DEFINE_MUTEX(svc_mutex);
+
+svc_invoke_fn *invoke_fn;
+
+struct intel_svc_chan *request_svc_channel_byname(
+	struct intel_svc_client *client, const char *name)
+{
+	struct device *dev = client->dev;
+	struct intel_svc_controller *controller;
+	struct intel_svc_chan *chan;
+	unsigned long flag;
+	int i;
+
+	chan = ERR_PTR(-EPROBE_DEFER);
+	controller = list_first_entry(&svc_cons,
+				      struct intel_svc_controller, node);
+	for (i = 0; i < SVC_MAX_CHANNEL; i++) {
+		if (!strcmp(controller->chans[i].name, name)) {
+			chan = &controller->chans[i];
+			break;
+		}
+	}
+
+	if (chan->scl || !try_module_get(controller->dev->driver->owner)) {
+		dev_dbg(dev, "%s: svc not free\n", __func__);
+		return ERR_PTR(-EBUSY);
+	}
+
+	spin_lock_irqsave(&chan->lock, flag);
+	chan->scl = client;
+	spin_unlock_irqrestore(&chan->lock, flag);
+
+	return chan;
+}
+EXPORT_SYMBOL_GPL(request_svc_channel_byname);
+
+void free_svc_channel(struct intel_svc_chan *chan)
+{
+	unsigned long flag;
+
+	spin_lock_irqsave(&chan->lock, flag);
+	chan->scl = NULL;
+	module_put(chan->ctrl->dev->driver->owner);
+	spin_unlock_irqrestore(&chan->lock, flag);
+}
+EXPORT_SYMBOL_GPL(free_svc_channel);
+
+int intel_svc_send(struct intel_svc_chan *chan, void *data)
+{
+	struct intel_svc_data *svc_data = (struct intel_svc_data *)data;
+	struct intel_svc_private_mem *pmem;
+	int ret = 0;
+	unsigned long flag;
+
+	pr_debug("%s: sent P-va=%p, P-com=%x, P-size=%u\n", __func__,
+		 svc_data->payload, svc_data->command,
+		 (unsigned int)svc_data->payload_length);
+
+	list_for_each_entry(pmem, &svc_private_mem, node) {
+		if (pmem->kaddr == svc_data->payload) {
+			svc_p_data->paddr = pmem->paddr;
+			break;
+		}
+	}
+
+	svc_p_data->command = svc_data->command;
+	svc_p_data->size = svc_data->payload_length;
+	svc_p_data->chan = chan;
+	pr_debug("%s: put to FIFO pa=0x%016x, cmd=%x, size=%u\n", __func__,
+		 (unsigned int)svc_p_data->paddr, svc_p_data->command,
+		 (unsigned int)svc_p_data->size);
+	spin_lock_irqsave(&chan->ctrl->svc_fifo_lock, flag);
+	ret = kfifo_put(&chan->ctrl->intel_svc_fifo, svc_p_data);
+	spin_unlock_irqrestore(&chan->ctrl->svc_fifo_lock, flag);
+
+	if (!ret)
+		return -ENOBUFS;
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(intel_svc_send);
+
+void *intel_svc_allocate_memory(struct intel_svc_chan *chan, size_t size)
+{
+	struct intel_svc_private_mem *pmem;
+	unsigned long va;
+	phys_addr_t pa;
+	struct gen_pool *genpool = chan->ctrl->genpool;
+	size_t s = roundup(size, 1 << genpool->min_alloc_order);
+
+	pmem = devm_kzalloc(chan->ctrl->dev,
+			    sizeof(struct ntel_svc_private_mem *), GFP_KERNEL);
+	if (!pmem)
+		return ERR_PTR(-ENOMEM);
+
+	va = gen_pool_alloc(genpool, s);
+	if (!va)
+		return ERR_PTR(-ENOMEM);
+
+	memset((void *)va, 0, s);
+	pa = gen_pool_virt_to_phys(genpool, va);
+
+	pmem->kaddr = (void *)va;
+	pmem->paddr = pa;
+	pmem->size = s;
+	list_add_tail(&pmem->node, &svc_private_mem);
+	pr_debug("%s: va=%p, pa=0x%016x\n", __func__,
+		 pmem->kaddr, (unsigned int)pmem->paddr);
+	return (void *)va;
+}
+EXPORT_SYMBOL_GPL(intel_svc_allocate_memory);
+
+void intel_svc_free_memory(struct intel_svc_chan *chan, void *kaddr)
+{
+	struct intel_svc_private_mem *pmem;
+	size_t size = 0;
+
+	list_for_each_entry(pmem, &svc_private_mem, node)
+		if (pmem->kaddr == kaddr) {
+			size = pmem->size;
+			break;
+		}
+
+	gen_pool_free(chan->ctrl->genpool, (unsigned long)kaddr, size);
+	pmem->kaddr = NULL;
+	list_del(&pmem->node);
+}
+EXPORT_SYMBOL_GPL(intel_svc_free_memory);
+
+static void *svc_pa_to_va(unsigned long addr)
+{
+	struct intel_svc_private_mem *pmem;
+
+	pr_debug("claim back P-addr=0x%016x\n", (unsigned int)addr);
+	list_for_each_entry(pmem, &svc_private_mem, node) {
+		if (pmem->paddr == addr)
+			return pmem->kaddr;
+	}
+
+	/* physical address is not found */
+	return NULL;
+}
+
+static void thread_command_data_claim(struct intel_svc_private_data *msg,
+				      struct intel_svc_rc_data *data)
+{
+	struct arm_smccc_res res;
+	unsigned long a0, a1, a2;
+
+	do {
+		/* repeatedly query to claim back all submitted buffers */
+		a0 = INTEL_SIP_SMC_FPGA_CONFIG_COMPLETED_WRITE;
+		a1 = 0;
+		a2 = 0;
+		pr_debug("%s: claim back the submitted buffer\n", __func__);
+		invoke_fn(a0, a1, a2, 0, 0, 0, 0, 0, &res);
+		switch (res.a0) {
+		case INTEL_SIP_SMC_STATUS_OK:
+			data->status = BIT(SVC_STATUS_RECONFIG_BUFFER_DONE);
+			if (!res.a1)
+				break;
+			data->kaddr1 = svc_pa_to_va(res.a1);
+			data->kaddr2 = (res.a2) ? svc_pa_to_va(res.a2) : NULL;
+			data->kaddr3 = (res.a3) ? svc_pa_to_va(res.a3) : NULL;
+			msg->chan->scl->receive_cb(msg->chan->scl, data);
+			break;
+		case INTEL_SIP_SMC_FPGA_CONFIG_STATUS_BUSY:
+			pr_debug("%s: EL3 busy, claim again\n", __func__);
+			break;
+		default:
+			data->status = BIT(SVC_STATUS_RECONFIG_ERROR);
+			data->kaddr1 = NULL;
+			data->kaddr2 = NULL;
+			data->kaddr3 = NULL;
+			msg->chan->scl->receive_cb(msg->chan->scl, data);
+			break;
+		}
+	} while ((res.a0 == INTEL_SIP_SMC_STATUS_OK) && res.a1);
+}
+
+static void thread_command_data_submit(struct intel_svc_private_data *msg,
+				       struct intel_svc_rc_data *data)
+{
+	struct arm_smccc_res res;
+	unsigned long a0, a1, a2;
+
+	pr_debug("%s: EL3 buffer full\n", __func__);
+	do {
+		/* send query to see the completed data block */
+		pr_debug("%s: EL1 polling for completed D-block\n", __func__);
+		a0 = INTEL_SIP_SMC_FPGA_CONFIG_COMPLETED_WRITE;
+		a1 = 0;
+		a2 = 0;
+		invoke_fn(a0, a1, a2, 0, 0, 0, 0, 0, &res);
+		switch (res.a0) {
+		case INTEL_SIP_SMC_STATUS_OK:
+			data->status = BIT(SVC_STATUS_RECONFIG_BUFFER_DONE);
+			if (!res.a1)
+				break;
+			data->kaddr1 = svc_pa_to_va(res.a1);
+			data->kaddr2 = (res.a2) ? svc_pa_to_va(res.a2) : NULL;
+			data->kaddr3 = (res.a3) ? svc_pa_to_va(res.a3) : NULL;
+			msg->chan->scl->receive_cb(msg->chan->scl, data);
+			break;
+		case INTEL_SIP_SMC_FPGA_CONFIG_STATUS_BUSY:
+			pr_debug("%s: EL3 busy, polling again\n", __func__);
+			break;
+		default:
+			data->status = BIT(SVC_STATUS_RECONFIG_ERROR);
+			data->kaddr1 = NULL;
+			data->kaddr2 = NULL;
+			data->kaddr3 = NULL;
+			msg->chan->scl->receive_cb(msg->chan->scl, data);
+			break;
+		}
+	} while ((res.a0 == INTEL_SIP_SMC_STATUS_OK) && res.a1);
+}
+
+static void thread_command_reconfig_status(struct intel_svc_private_data *msg,
+					   struct intel_svc_rc_data *data)
+{
+	struct arm_smccc_res res;
+	unsigned long a0, a1, a2;
+	int count;
+
+	pr_debug("%s: EL1 polling config status for up to 30 sec\n", __func__);
+
+	/* timeout for polling FPGA configuration status
+	 * it is set to 30 seconds (30 * 1000) at Intel Stratix10 SoC
+	 */
+	count = 30;
+	while (count) {
+		a0 = INTEL_SIP_SMC_FPGA_CONFIG_ISDONE;
+		a1 = 0;
+		a2 = 0;
+		invoke_fn(a0, a1, a2, 0, 0, 0, 0, 0, &res);
+		if ((res.a0 == INTEL_SIP_SMC_STATUS_OK) ||
+		    (res.a0 == INTEL_SIP_SMC_FPGA_CONFIG_STATUS_ERROR))
+			break;
+
+		/* configuration is still in progress, wait one second then
+		 * poll again
+		 */
+		count--;
+		msleep(1000);
+	}
+
+	switch (res.a0) {
+	case INTEL_SIP_SMC_STATUS_OK:
+		data->status = BIT(SVC_STATUS_RECONFIG_COMPLETED);
+		break;
+	case SVC_STATUS_RECONFIG_ERROR:
+		data->status = BIT(SVC_STATUS_RECONFIG_ERROR);
+		break;
+	case INTEL_SIP_SMC_FPGA_CONFIG_STATUS_BUSY:
+		data->status = BIT(SVC_STATUS_RECONFIG_ERROR);
+		break;
+	default:
+		/* this shouldn't happen */
+		break;
+	}
+
+	data->kaddr1 = NULL;
+	data->kaddr2 = NULL;
+	data->kaddr3 = NULL;
+	msg->chan->scl->receive_cb(msg->chan->scl, data);
+}
+
+static void thread_recv_status_ok(struct intel_svc_private_data *msg,
+				  struct intel_svc_rc_data *data,
+				  struct arm_smccc_res res)
+{
+	data->kaddr1 = NULL;
+	data->kaddr2 = NULL;
+	data->kaddr3 = NULL;
+
+	switch (msg->command) {
+	case COMMAND_RECONFIG:
+		data->status = BIT(SVC_STATUS_RECONFIG_REQUEST_OK);
+		break;
+	case COMMAND_RECONFIG_DATA_SUBMIT:
+		data->status = BIT(SVC_STATUS_RECONFIG_BUFFER_SUBMITTED);
+		break;
+	case COMMAND_NOOP:
+		data->status = BIT(SVC_STATUS_RECONFIG_BUFFER_SUBMITTED);
+		data->kaddr1 = svc_pa_to_va(res.a1);
+		break;
+	case COMMAND_RECONFIG_STATUS:
+		data->status = BIT(SVC_STATUS_RECONFIG_COMPLETED);
+		break;
+	default:
+		break;
+	}
+
+	pr_debug("%s: call receive_cb\n", __func__);
+	msg->chan->scl->receive_cb(msg->chan->scl, data);
+}
+
+static int intel_svc_smc_thread(void *data)
+{
+	struct intel_svc_controller *ctrl = (struct intel_svc_controller *)data;
+	struct intel_svc_private_data *svc_msg;
+	struct intel_svc_rc_data *rcdata;
+	struct arm_smccc_res res;
+	unsigned long a0, a1, a2;
+	int ret = 0;
+	int ret_fifo = 0;
+	unsigned long flag;
+
+	rcdata = kmalloc(sizeof(struct intel_svc_rc_data *), GFP_KERNEL);
+	if (!rcdata)
+		return -ENOMEM;
+
+	/* default set, to remove build warning */
+	a0 = INTEL_SIP_SMC_FPGA_CONFIG_LOOPBACK;
+	a1 = 0;
+	a2 = 0;
+
+	while (!kthread_should_stop()) {
+		spin_lock_irqsave(&ctrl->svc_fifo_lock, flag);
+		ret_fifo = kfifo_get(&ctrl->intel_svc_fifo, &svc_msg);
+		spin_unlock_irqrestore(&ctrl->svc_fifo_lock, flag);
+		if (!ret_fifo)
+			continue;
+
+		pr_debug("get from FIFO pa=0x%016x, command=%u, size=%u\n",
+			 (unsigned int)svc_msg->paddr, svc_msg->command,
+			 (unsigned int)svc_msg->size);
+		if (svc_msg->command == COMMAND_RECONFIG_DATA_CLAIM) {
+			thread_command_data_claim(svc_msg, rcdata);
+		} else {
+			switch (svc_msg->command) {
+			case COMMAND_NOOP:
+				a0 = INTEL_SIP_SMC_FPGA_CONFIG_LOOPBACK;
+				a1 = (unsigned long)svc_msg->paddr;
+				a2 = (unsigned long)svc_msg->size;
+				break;
+			case COMMAND_RECONFIG:
+				a0 = INTEL_SIP_SMC_FPGA_CONFIG_START;
+				a1 = 0;
+				a2 = 0;
+				break;
+			case COMMAND_RECONFIG_DATA_SUBMIT:
+				a0 = INTEL_SIP_SMC_FPGA_CONFIG_WRITE;
+				a1 = (unsigned long)svc_msg->paddr;
+				a2 = (unsigned long)svc_msg->size;
+				break;
+			case COMMAND_RECONFIG_STATUS:
+				a0 = INTEL_SIP_SMC_FPGA_CONFIG_ISDONE;
+				a1 = 0;
+				a2 = 0;
+				break;
+			default:
+				/* it shouldn't happen */
+				break;
+			}
+			pr_debug("%s: before SMC call -- a0=0x%016x a1=0x%016x",
+				 __func__, (unsigned int)a0, (unsigned int)a1);
+			pr_debug(" a2=0x%016x\n", (unsigned int)a2);
+			invoke_fn(a0, a1, a2, 0, 0, 0, 0, 0, &res);
+
+			pr_debug("%s: after SMC call -- res.a0=0x%016x",
+				 __func__, (unsigned int)res.a0);
+			pr_debug(" res.a1=0x%016x, res.a2=0x%016x",
+				 (unsigned int)res.a1, (unsigned int)res.a2);
+			pr_debug(" res.a3=0x%016x\n", (unsigned int)res.a3);
+
+			switch (res.a0) {
+			case INTEL_SIP_SMC_STATUS_OK:
+				thread_recv_status_ok(svc_msg, rcdata, res);
+				break;
+			case INTEL_SIP_SMC_FPGA_CONFIG_STATUS_BUSY:
+				switch (svc_msg->command) {
+				case COMMAND_RECONFIG_DATA_SUBMIT:
+					thread_command_data_submit(svc_msg,
+								   rcdata);
+					break;
+				case COMMAND_RECONFIG_STATUS:
+					thread_command_reconfig_status(svc_msg,
+								       rcdata);
+					break;
+				default:
+					break;
+				}
+				break;
+			case INTEL_SIP_SMC_FPGA_CONFIG_STATUS_REJECTED:
+				pr_debug("%s: STATUS_REJECTED\n", __func__);
+				break;
+			case INTEL_SIP_SMC_FPGA_CONFIG_STATUS_ERROR:
+				pr_err("%s: STATUS_ERROR\n", __func__);
+				rcdata->status = BIT(SVC_STATUS_RECONFIG_ERROR);
+				rcdata->kaddr1 = NULL;
+				rcdata->kaddr2 = NULL;
+				rcdata->kaddr3 = NULL;
+				svc_msg->chan->scl->receive_cb
+					(svc_msg->chan->scl, rcdata);
+				break;
+			default:
+				break;
+			}
+		}
+	};
+
+	kfree(rcdata);
+	return ret;
+}
+
+static int intel_svc_smc_shmemory_thread(void *data)
+{
+	struct intel_svc_sh_memory *sh_mem = (struct intel_svc_sh_memory *)data;
+	struct arm_smccc_res res;
+
+	/* SMC call to get shared memory info from secure world */
+	invoke_fn(INTEL_SIP_SMC_FPGA_CONFIG_GET_MEM,
+		  0, 0, 0, 0, 0, 0, 0, &res);
+	if (res.a0 == INTEL_SIP_SMC_STATUS_OK) {
+		sh_mem->start_addr = res.a1;
+		sh_mem->size = res.a2;
+	} else {
+		sh_mem->start_addr = 0;
+		sh_mem->size = 0;
+	}
+
+	complete(&sh_mem->sync_complete);
+	do_exit(0);
+}
+
+static void svc_get_sh_memory_param(struct platform_device *pdev,
+				    struct intel_svc_sh_memory *param)
+{
+	struct task_struct *sh_memory_task;
+
+	init_completion(&param->sync_complete);
+
+	sh_memory_task = kthread_create_on_cpu(intel_svc_smc_shmemory_thread,
+					       (void *)param,
+						0, "intel_svc_smc_shm_thread");
+	if (IS_ERR(sh_memory_task))
+		dev_err(&pdev->dev, "fail create intel_svc_smc_shm_thread\n");
+
+	wake_up_process(sh_memory_task);
+}
+
+static void svc_smccc_smc(unsigned long a0, unsigned long a1,
+			  unsigned long a2, unsigned long a3,
+			  unsigned long a4, unsigned long a5,
+			  unsigned long a6, unsigned long a7,
+			  struct arm_smccc_res *res)
+{
+	arm_smccc_smc(a0, a1, a2, a3, a4, a5, a6, a7, res);
+}
+
+static void svc_smccc_hvc(unsigned long a0, unsigned long a1,
+			  unsigned long a2, unsigned long a3,
+			  unsigned long a4, unsigned long a5,
+			  unsigned long a6, unsigned long a7,
+			  struct arm_smccc_res *res)
+{
+	arm_smccc_hvc(a0, a1, a2, a3, a4, a5, a6, a7, res);
+}
+
+static svc_invoke_fn *get_invoke_func(struct device *dev,
+				      struct device_node *np)
+{
+	const char *method;
+
+	if (of_property_read_string(np, "method", &method)) {
+		dev_warn(dev, "missing \"method\" property\n");
+		return ERR_PTR(-ENXIO);
+	}
+
+	if (!strcmp(method, "smc"))
+		return svc_smccc_smc;
+	if (!strcmp(method, "hvc"))
+		return svc_smccc_hvc;
+
+	dev_warn(dev, "invalid \"method\" property: %s\n", method);
+	return ERR_PTR(-EINVAL);
+}
+
+static const struct of_device_id intel_svc_drv_match[] = {
+	{.compatible = "intc,svc-1.0"},
+	{},
+};
+
+static int intel_svc_drv_probe(struct platform_device *pdev)
+{
+	struct intel_svc_controller *controller;
+	struct intel_svc_chan *chans;
+	unsigned long vaddr;
+	phys_addr_t paddr;
+	size_t size;
+	phys_addr_t begin;
+	phys_addr_t end;
+	void *va;
+	size_t page_mask = PAGE_SIZE - 1;
+	int min_alloc_order = 3;
+	struct gen_pool *genpool = NULL;
+	int ret = 0;
+	struct intel_svc_sh_memory *sh_memory;
+
+	/* get SMC or HVC function */
+	invoke_fn = get_invoke_func(&pdev->dev, pdev->dev.of_node);
+	if (IS_ERR(invoke_fn))
+		return -EINVAL;
+
+	sh_memory = devm_kzalloc(&pdev->dev,
+				 sizeof(struct intel_svc_sh_memory *),
+				 GFP_KERNEL);
+	if (!sh_memory)
+		return -ENOMEM;
+
+	svc_get_sh_memory_param(pdev, sh_memory);
+	if (!wait_for_completion_timeout(&sh_memory->sync_complete, 10 * HZ)) {
+		dev_err(&pdev->dev,
+			"timeout to get sh-memory paras from secure world\n");
+		return -ETIMEDOUT;
+	}
+
+	if (!sh_memory->start_addr || !sh_memory->size) {
+		dev_err(&pdev->dev,
+			"fails to get shared memory info from secure world\n");
+		return -ENOMEM;
+	}
+
+	dev_dbg(&pdev->dev, "SM software provides pa: 0x%016x, size: 0x%08x\n",
+		(unsigned int)sh_memory->start_addr,
+		(unsigned int)sh_memory->size);
+
+	begin = roundup(sh_memory->start_addr, PAGE_SIZE);
+	end = rounddown(sh_memory->start_addr + sh_memory->size, PAGE_SIZE);
+	paddr = begin;
+	size = end - begin;
+	va = memremap(paddr, size, MEMREMAP_WC);
+	if (!va) {
+		dev_err(&pdev->dev, "fail to remap shared memory\n");
+		return -EINVAL;
+	}
+	vaddr = (unsigned long)va;
+
+	dev_dbg(&pdev->dev,
+		"reserved memory vaddr: %p, paddr: 0x%16x size: 0x%8x\n",
+		va, (unsigned int)paddr, (unsigned int)size);
+	if ((vaddr & page_mask) || (paddr & page_mask) ||
+	    (size & page_mask)) {
+		dev_err(&pdev->dev, "page is not aligned\n");
+		return -EINVAL;
+	}
+	genpool = gen_pool_create(min_alloc_order, -1);
+	if (!genpool) {
+		dev_err(&pdev->dev, "fail to create genpool\n");
+		return -ENOMEM;
+	}
+
+	gen_pool_set_algo(genpool, gen_pool_best_fit, NULL);
+	ret = gen_pool_add_virt(genpool, vaddr, paddr, size, -1);
+	if (ret) {
+		dev_err(&pdev->dev, "fail to add memory chunk to the pool\n");
+		gen_pool_destroy(genpool);
+		return ret;
+	}
+
+	/* allocate service controller and supporting channel */
+	controller = devm_kzalloc(&pdev->dev,
+				  sizeof(struct intel_svc_controller *),
+				  GFP_KERNEL);
+	if (!controller)
+		return -ENOMEM;
+
+	chans = devm_kzalloc(&pdev->dev,
+			     SVC_MAX_CHANNEL * sizeof(struct intel_svc_chan *),
+			     GFP_KERNEL);
+	if (!chans)
+		return -ENOMEM;
+
+	controller->dev = &pdev->dev;
+	controller->num_chans = SVC_MAX_CHANNEL;
+	controller->chans = chans;
+	controller->genpool = genpool;
+	ret = kfifo_alloc(&controller->intel_svc_fifo,
+			  sizeof(struct intel_svc_private_data *) * SVC_FO_LEN,
+			  GFP_KERNEL);
+	if (ret) {
+		dev_err(&pdev->dev, "fails to allocate FIFO\n");
+		return ret;
+	}
+	spin_lock_init(&controller->svc_fifo_lock);
+
+	chans[0].scl = NULL;
+	chans[0].ctrl = controller;
+	chans[0].name = "fpga";
+	spin_lock_init(&chans[0].lock);
+
+	chans[1].scl = NULL;
+	chans[1].ctrl = controller;
+	chans[1].name = "dummy";
+	spin_lock_init(&chans[1].lock);
+
+	svc_p_data = kmalloc(sizeof(struct intel_svc_private_data *),
+			     GFP_KERNEL);
+	if (!svc_p_data)
+		return -ENOMEM;
+
+	 /* create a kthread to handle SMC or HVC call */
+	task = kthread_create_on_cpu(intel_svc_smc_thread,
+				     (void *)controller, 0,
+				     "intel_svc_smc_thread");
+	if (IS_ERR(task)) {
+		dev_err(&pdev->dev, "fails to create svc_smc_thread\n");
+		return -EINVAL;
+	}
+	wake_up_process(task);
+
+	list_add_tail(&controller->node, &svc_cons);
+	platform_set_drvdata(pdev, controller);
+
+	pr_info("Intel Service Driver Initialized\n");
+
+	return ret;
+}
+
+static int intel_svc_drv_remove(struct platform_device *pdev)
+{
+	struct intel_svc_controller *ctrl = platform_get_drvdata(pdev);
+
+	kfifo_free(&ctrl->intel_svc_fifo);
+	kfree(svc_p_data);
+	kthread_stop(task);
+	if (ctrl->genpool)
+		gen_pool_destroy(ctrl->genpool);
+	list_del(&ctrl->node);
+
+	return 0;
+}
+
+static struct platform_driver intel_svc_driver = {
+	.probe = intel_svc_drv_probe,
+	.remove = intel_svc_drv_remove,
+	.driver = {
+		.name = "intel-svc",
+		.of_match_table = intel_svc_drv_match,
+	},
+};
+
+static int __init intel_svc_init(void)
+{
+	return platform_driver_register(&intel_svc_driver);
+}
+
+static void __exit intel_svc_exit(void)
+{
+	return platform_driver_unregister(&intel_svc_driver);
+}
+
+subsys_initcall(intel_svc_init);
+module_exit(intel_svc_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_DESCRIPTION("Intel Stratix10 Service Driver");
+MODULE_AUTHOR("Richard Gong <richard.gong@intel.com>");
+MODULE_ALIAS("platform:intel-svc");
+
diff --git a/include/linux/intel-service-client.h b/include/linux/intel-service-client.h
new file mode 100644
index 0000000..5bb6513
--- /dev/null
+++ b/include/linux/intel-service-client.h
@@ -0,0 +1,227 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2017-2018, Intel Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __INTEL_SERVICE_CLIENT_H
+#define __INTEL_SERVICE_CLIENT_H
+
+#include <linux/of.h>
+#include <linux/device.h>
+
+/**
+ * Service driver supports client names
+ * @fpga: for FPGA configuration
+ * @dummy: for integration/debug/trouble-shooting
+ */
+#define SVC_CLIENT_FPGA		"fpga"
+#define SVC_CLIENT_DUMMY	"dummy"
+
+/**
+ * Status of the sent command, in bit number
+ * @SVC_COMMAND_STATUS_RECONFIG_REQUEST_OK
+ *	SDM accepts the request of FPGA reconfiguration
+ * @SVC_STATUS_RECONFIG_BUFFER_SUBMITTED
+ *	Service client successfully submits FPGA configuration
+ *	data buffer to SDM
+ * @SVC_COMMAND_STATUS_RECONFIG_BUFFER_DONE
+ *	SDM completes data process, ready to accept the
+ *      next WRITE transaction
+ * @SVC_COMMAND_STATUS_RECONFIG_COMPLETED
+ *	SDM completes FPGA configuration successfully, FPGA should
+ *	be in user mode
+ * @SVC_COMMAND_STATUS_RECONFIG_BUSY
+ *	FPGA configuration is still in process
+ * @SVC_COMMAND_STATUS_RECONFIG_ERROR
+ *	Error encountered during FPGA configuration
+ */
+#define SVC_STATUS_RECONFIG_REQUEST_OK		0
+#define SVC_STATUS_RECONFIG_BUFFER_SUBMITTED	1
+#define SVC_STATUS_RECONFIG_BUFFER_DONE		2
+#define SVC_STATUS_RECONFIG_COMPLETED		3
+#define SVC_STATUS_RECONFIG_BUSY			4
+#define SVC_STATUS_RECONFIG_ERROR			5
+
+struct intel_svc_chan;
+
+/**
+ * enum intel_svc_command_code
+ *
+ * supporting service commands
+ */
+enum intel_svc_command_code {
+	/*
+	 * do 'dummy' request for integration/debug/trouble-shooting
+	 */
+	COMMAND_NOOP = 0,
+
+	/*
+	 * COMMAND_RECONFIG - ask for FPGA configuration preparation,
+	 * return status is SVC_STATUS_RECONFIG_REQUEST_OK
+	 */
+	COMMAND_RECONFIG,
+
+	/*
+	 * COMMAND_RECONFIG_DATA_SUBMIT - submit a buffer of bit-stream data
+	 * for FPGA configuration
+	 * return status is SVC_STATUS_RECONFIG_BUFFER_SUBMITTED, or
+	 * SVC_STATUS_RECONFIG_ERROR
+	 */
+	COMMAND_RECONFIG_DATA_SUBMIT,
+
+	/*
+	 * COMMAND_RECONFIG_DATA_CLAIM - claim back the submitted bit-stream
+	 * data buffer(s)
+	 * return status is SVC_STATUS_RECONFIG_BUFFER_DONE, or
+	 * SVC_STATUS_RECONFIG_ERROR
+	 */
+	COMMAND_RECONFIG_DATA_CLAIM,
+
+	/*
+	 * COMMAND_RECONFIG_STATUS - check the status of the configuration
+	 * return status is SVC_STATUS_RECONFIG_COMPLETED, or
+	 * SVC_STATUS_RECONFIG_BUSY, or SVC_STATUS_RECONFIG_ERROR
+	 */
+	COMMAND_RECONFIG_STATUS
+
+	/* To-Do:
+	 * need be added for QSPI and other
+	 */
+};
+
+/**
+ * struct intel_svc_data - service data structure
+ *
+ * @command: service command
+ * @payload: starting address of data need be processed if command is
+ *           COMMAND_RECONFIG_DATA
+ * @payload_length:to data size in bytes of command is COMMAND_RECONFIG_DATA
+ */
+struct intel_svc_data {
+	void *payload;
+	size_t payload_length;
+	enum intel_svc_command_code command;
+};
+
+/**
+ * Flag bits for COMMAND_RECONFIG
+ * @COMMAND_RECONFIG_FLAG_PARTIAL
+ *	Set to request partial reconfig.  Default is full reconfig.
+ */
+#define COMMAND_RECONFIG_FLAG_PARTIAL	0
+
+/**
+ * struct intel_command_reconfig_payload - payload for COMMAND_RECONFIG
+ *
+ * @flags: flag bits for COMMAND_RECONFIG
+ */
+struct intel_command_reconfig_payload {
+	u32 flags;
+};
+
+/**
+ * struct intel_svc_rc_data - data structure from service layer
+ *
+ * @status: the status of sent command
+ * @kaddr1-3: used when status is SVC_COMMAND_STATUS_RECONFIG_BUFFER_DONE
+ * kaddr1 - address of 1st completed data block
+ * kaddr2 - address of 2nd completed data block
+ * kaddr3 - address of 3rd completed data block
+ */
+struct intel_svc_rc_data {
+	u32 status;
+	void *kaddr1;
+	void *kaddr2;
+	void *kaddr3;
+};
+
+/**
+ * struct intel_svc_client
+ *
+ * @dev: the client device
+ * @receive_callback: callback to provide service client of data received,
+ * @priv: client private data
+ */
+struct intel_svc_client {
+	struct device *dev;
+	void (*receive_cb)(struct intel_svc_client *client,
+			   struct intel_svc_rc_data *data);
+	void *priv;
+};
+
+/*
+ * request_svc_channel_byname
+ *
+ * request service channel by providing one of names
+ *
+ * @client: identity of the client requesting the channel
+ * @name: supporting client name defined above
+ *
+ * return a pointer to channel assigned to the client if successful,
+ *	or -ERR_PTR for request failure
+ */
+struct intel_svc_chan
+*request_svc_channel_byname(struct intel_svc_client *client,
+	const char *name);
+
+/**
+ * free_svc_channel
+ *
+ * free service channel
+ *
+ * @chan: service channel to be freed
+ */
+void free_svc_channel(struct intel_svc_chan *chan);
+
+/**
+ * intel_svc_allocate_memory
+ *
+ * request memory from the pool
+ *	Controller allocates the requested number of bytes from the memory pool
+ *	for this client
+ *
+ * @chan: service channel assigned to this client
+ * @size: number of bytes client requests
+ *
+ * return the starting address of allocated memory
+ */
+void *intel_svc_allocate_memory(struct intel_svc_chan *chan, size_t size);
+
+/**
+ * intel_svc_free_memory
+ *
+ * free allocated memory from the pool
+ *
+ * @chan: service channel assigned to this client
+ * @kaddr: starting address of memory to be free back to pool
+ */
+void intel_svc_free_memory(struct intel_svc_chan *chan, void *kaddr);
+
+/**
+ * intel_svc_send
+ *
+ * send a message to the remote
+ *
+ * @chan: service channel assigned to this client
+ * @msg: message data to be sent, in the format of struct intel_svc_data
+ *
+ * return positive value (which is the number of elements put into the FIFO)
+ * for successful submission to the data queue created by service driver,
+ * or -ENOBUFS if the data queue FIFO is full
+ */
+int intel_svc_send(struct intel_svc_chan *chan, void *data);
+
+#endif
+
diff --git a/include/linux/intel-service.h b/include/linux/intel-service.h
new file mode 100644
index 0000000..fdf21b3
--- /dev/null
+++ b/include/linux/intel-service.h
@@ -0,0 +1,122 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * Copyright (C) 2017-2018, Intel Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __INTEL_SERVICE_H
+#define __INTEL_SERVICE_H
+
+#include <linux/of.h>
+#include <linux/types.h>
+#include <linux/device.h>
+#include <linux/kfifo.h>
+#include <linux/genalloc.h>
+#include <linux/arm-smccc.h>
+
+typedef void (svc_invoke_fn)(unsigned long, unsigned long, unsigned long,
+			unsigned long, unsigned long, unsigned long,
+			unsigned long, unsigned long, struct arm_smccc_res *);
+
+struct task_struct *task;
+struct intel_svc_private_data *svc_p_data;
+struct intel_svc_chan;
+
+/**
+ * struct intel_svc_sh_memory - service shared memory info
+ *
+ * @sync_complete: maintain state for a completion
+ * @start_addr: starting address of shared memory
+ * @size: size of shared memory
+ */
+struct intel_svc_sh_memory {
+	struct completion sync_complete;
+	unsigned long start_addr;
+	unsigned long size;
+};
+
+/**
+ * struct intel_svc_private_mem - service private memory info
+ *
+ * @kaddr: virtual address
+ * @paddr: physical address
+ * @size: size of memory
+ * @node: link list head node
+ */
+struct intel_svc_private_mem {
+	void *kaddr;
+	phys_addr_t paddr;
+	size_t size;
+	struct list_head node;
+};
+
+/**
+ * struct intel_svc_private_data - service private data
+ *
+ * @chan: service channel
+ * @paddr: play-load physical address
+ * @size: play-load size
+ * @ommand: service request command
+ */
+struct intel_svc_private_data {
+	struct intel_svc_chan *chan;
+	phys_addr_t paddr;
+	size_t size;
+	u32 command;
+};
+
+/**
+ * struct intel_svc_controller - service controller
+ *
+ * @dev: device backing this controller
+ * @chans: array of service channels
+ * $num_chans: number of channels in 'chans' array
+ * @node: list management
+ * @intel_svc_fifo: a queue for storing service message data
+ * @svc_fifo_lock: protect access to service message data queue
+ */
+struct intel_svc_controller {
+	struct device *dev;
+	struct intel_svc_chan *chans;
+	int num_chans;
+
+	struct list_head node;
+
+	struct gen_pool *genpool;
+	struct intel_svc_prviate_mem *private_mem;
+
+	DECLARE_KFIFO_PTR(intel_svc_fifo, struct intel_svc_private_data *);
+	/* protect access to the service message data queue*/
+	spinlock_t svc_fifo_lock;
+};
+
+/**
+ * struct intel_svc_chan - service communication channel
+ *
+ * @ctrl: pointer to the provider of this channel
+ * @scl: pointer to service client which owns this channel
+ * $name: name associated with this service channel
+ * @lock: protect access to the channel
+ */
+struct intel_svc_chan {
+	struct intel_svc_controller *ctrl;
+	struct intel_svc_client *scl;
+	char *name;
+	/* protect access to the channel*/
+	spinlock_t lock;
+};
+
+static int intel_svc_smc_thread(void *data);
+
+#endif
diff --git a/include/linux/intel-smc.h b/include/linux/intel-smc.h
new file mode 100644
index 0000000..5c4486c
--- /dev/null
+++ b/include/linux/intel-smc.h
@@ -0,0 +1,246 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/**
+ * Copyright (C) 2017-2018, Intel Corporation
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+ * more details.
+ *
+ * You should have received a copy of the GNU General Public License along with
+ * this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+#ifndef __INTEL_SMC_H
+#define __INTEL_SMC_H
+
+#include <linux/arm-smccc.h>
+#include <linux/bitops.h>
+
+/*
+ * This file defines the Secure Monitor Call (SMC) message protocol used for
+ * service driver in normal world (EL1) to communicate with boot-loader
+ * in Secure Monitor Exception level 3 (EL3)
+ *
+ * An ARM SMC instruction takes a function identifier and up to 6 64-bit
+ * register values as arguments, and can return up to 4 64-bit register
+ * value. The operation of the secure monitor is determined by the parameter
+ * values passed in through registers.
+
+ * EL1 and EL3 communicates pointer as physical address rather than the
+ * virtual address
+ */
+
+/*
+ * Functions specified by ARM SMC Calling convention
+ *
+ * FAST call executes atomic operations, returns when the requested operation
+ * has completed
+ * STD call starts a operation which can be preempted by a non-secure
+ * interrupt. The call can return before the requested operation has
+ * completed
+ *
+ * a0..a7 is used as register names in the descriptions below, on arm32
+ * that translates to r0..r7 and on arm64 to w0..w7
+ */
+
+#define INTEL_SIP_SMC_STD_CALL_VAL(func_num) \
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_STD_CALL, ARM_SMCCC_SMC_64, \
+	ARM_SMCCC_OWNER_SIP, (func_num))
+
+#define INTEL_SIP_SMC_FAST_CALL_VAL(func_num) \
+	ARM_SMCCC_CALL_VAL(ARM_SMCCC_FAST_CALL, ARM_SMCCC_SMC_64, \
+	ARM_SMCCC_OWNER_SIP, (func_num))
+
+/*
+ * Return values in INTEL_SIP_SMC_* call
+ *
+ * INTEL_SIP_SMC_RETURN_UNKNOWN_FUNCTION:
+ *		Secure world does not recognize this request
+ *
+ * INTEL_SIP_SMC_STATUS_OK:
+ * FPGA configuration completed successfully,
+ * In case of FPGA configuration write operation, it means boot-loader
+ * can accept the next chunk of FPGA configuration data
+ *
+ * INTEL_SIP_SMC_FPGA_CONFIG_STATUS_BUSY:
+ * In case of FPGA configuration write operation, it means boot-loader
+ * is still processing previous data & can't accept the next chunk
+ * of data. Service driver needs to issue
+ * INTEL_SIP_SMC_FPGA_CONFIG_COMPLETED_WRITE call to query the
+ * completed block(s)
+ *
+ * INTEL_SIP_SMC_FPGA_CONFIG_STATUS_ERROR:
+ * There is error during the FPGA configuration process
+ */
+#define INTEL_SIP_SMC_RETURN_UNKNOWN_FUNCTION		0xFFFFFFFF
+#define INTEL_SIP_SMC_STATUS_OK				0x0
+#define INTEL_SIP_SMC_FPGA_CONFIG_STATUS_BUSY		0x1
+#define INTEL_SIP_SMC_FPGA_CONFIG_STATUS_REJECTED       0x2
+#define INTEL_SIP_SMC_FPGA_CONFIG_STATUS_ERROR		0x4
+
+/*
+ * a0..a7 is used as register names in the descriptions below, on arm32
+ * that translates to r0..r7 and on arm64 to w0..w7.
+ */
+
+/*
+ * Request INTEL_SIP_SMC_FPGA_CONFIG_START
+ *
+ * Sync call used by service driver at EL1 to request the FPGA in EL3 to
+ * be prepare to receive a new configuration
+ *
+ * Call register usage:
+ * a0 INTEL_SIP_SMC_FPGA_CONFIG_START
+ * a1 flag for full or partial configuration
+ *    0 full reconfiguration
+ *    1 partial reconfiguration
+ * a2-7 not used
+ *
+ * Return status
+ * a0 INTEL_SIP_SMC_STATUS_OK indicates secure world is ready for
+ *	FPGA configuration, or negative value for failure
+ * a1-3: not used
+ */
+#define INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_START 1
+#define INTEL_SIP_SMC_FPGA_CONFIG_START \
+	INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_START)
+
+/*
+ * Request INTEL_SIP_SMC_FPGA_CONFIG_WRITE
+ *
+ * Async call used by service driver at EL1 to provide FPGA configuration data
+ * to secure world
+ *
+ * Call register usage:
+ * a0 INTEL_SIP_SMC_FPGA_CONFIG_WRITE
+ * a1 64bit physical address of the configuration data memory block
+ * a2 Size of configuration data block
+ * a3-7 not used
+ *
+ * Return status
+ * a0 INTEL_SIP_SMC_STATUS_OK
+ * a1 64bit physical address of 1st completed memory block if
+ *    any completed block, otherwise zero value
+ * a2 64bit physical address of 2nd completed memory block if
+ *    any completed block, otherwise zero value
+ * a3 64bit physical address of 3rd completed memory block if
+ *    any completed block, otherwise zero value
+ *
+ * Or
+ * a0 INTEL_SIP_SMC_FPGA_CONFIG_STATUS_BUSY
+ * a1-3 not used
+ *
+ * Or
+ * a0 INTEL_SIP_SMC_FPGA_CONFIG_STATUS_ERROR
+ * a1-3 not used
+ */
+#define INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_WRITE 2
+#define INTEL_SIP_SMC_FPGA_CONFIG_WRITE \
+	INTEL_SIP_SMC_STD_CALL_VAL(INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_WRITE)
+
+/*
+ * Request INTEL_SIP_SMC_FPGA_CONFIG_COMPLETED_WRITE
+ *
+ * Sync call used by service driver at EL1 to track the completed write
+ * transactions. This request is called after INTEL_SIP_SMC_FPGA_CONFIG_WRITE
+ * call with INTEL_SIP_SMC_FPGA_CONFIG_STATUS_BUSY
+ *
+ * Call register usage:
+ * a0 INTEL_SIP_SMC_FPGA_CONFIG_COMPLETED_WRITE
+ * a1-7 not used
+ *
+ * Return status
+ * a0 INTEL_SIP_SMC_STATUS_OK
+ * a1 64bit physical address of 1st completed memory block
+ * a2 64bit physical address of 2nd completed memory block if
+ *    any completed block, otherwise zero value
+ * a3 64bit physical address of 3rd completed memory block if
+ *    any completed block, otherwise zero value
+ *
+ * Or
+ * a0 INTEL_SIP_SMC_FPGA_CONFIG_STATUS_BUSY
+ * a1-3 not used
+ *
+ * Or
+ * a0 INTEL_SIP_SMC_FPGA_CONFIG_STATUS_ERROR
+ * a1-3 not used
+ */
+#define INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_COMPLETED_WRITE 3
+#define INTEL_SIP_SMC_FPGA_CONFIG_COMPLETED_WRITE \
+INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_COMPLETED_WRITE)
+
+/*
+ * Request INTEL_SIP_SMC_FPGA_CONFIG_ISDONE
+ *
+ * Sync call used by service driver at EL1 to inform secure world that all
+ * data are sent, to check whether or not the secure world completes
+ * the FPGA configuration process
+ *
+ * Call register usage:
+ * a0 INTEL_SIP_SMC_FPGA_CONFIG_ISDONE
+ * a1-7 not used
+ *
+ * Return status
+ * a0 INTEL_SIP_SMC_STATUS_OK for FPGA configuration process completed
+ *    successfully
+ *
+ *    or
+ *	 INTEL_SIP_SMC_FPGA_CONFIG_STATUS_BUSY for FPGA configuration is still
+ *	 in process
+ *
+ *    or
+ *	 INTEL_SIP_SMC_FPGA_CONFIG_STATUS_ERROR for error encountered during
+ *	 FPGA configuration
+ *
+ * a1-3: not used
+ */
+#define INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_ISDONE 4
+#define INTEL_SIP_SMC_FPGA_CONFIG_ISDONE \
+	INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_ISDONE)
+
+/*
+ * Request INTEL_SIP_SMC_FPGA_CONFIG_GET_MEM
+ *
+ * Sync call used by service driver at EL1 to query the physical address of
+ * memory block
+ * reserved by boot-loader
+ *
+ * Call register usage:
+ * a0 INTEL_SIP_SMC_FPGA_CONFIG_GET_MEM
+ * a1-7 not used
+ *
+ * Return status
+ * a0 INTEL_SIP_SMC_STATUS_OK
+ * a1: start of physical address of reserved memory block
+ * a2: size of reserved memory block
+ * a3: not used
+ */
+#define INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_GET_MEM 5
+#define INTEL_SIP_SMC_FPGA_CONFIG_GET_MEM \
+	INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_GET_MEM)
+
+/*
+ * Request INTEL_SIP_SMC_FPGA_CONFIG_LOOPBACK
+ *
+ * For SMC loop-back mode only, used for internal integration, debugging
+ * or troubleshooting,
+ *
+ * Call register usage:
+ * a0 INTEL_SIP_SMC_FPGA_CONFIG_LOOPBACK
+ * a1-7 not used
+ *
+ * Return status
+ * a0 INTEL_SIP_SMC_STATUS_OK for OK, or INTEL_SIP_SMC_FPGA_CONFIG_STATUS_ERROR
+ * for a failure
+ * a1-3: not used
+ */
+#define INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_LOOPBACK 6
+#define INTEL_SIP_SMC_FPGA_CONFIG_LOOPBACK \
+	INTEL_SIP_SMC_FAST_CALL_VAL(INTEL_SIP_SMC_FUNCID_FPGA_CONFIG_LOOPBACK)
+
+#endif
-- 
2.7.4

Re: [PATCHv1] Add Intel Stratix10 service layer driver

[Greg KH]

On Thu, Jan 25, 2018 at 10:39:03AM -0600, richard.gong@linux.intel.com wrote:
> From: Richard Gong <richard.gong@intel.com>
> 
> Intel Stratix10 SoC is composed of a 64 bit quad-core ARM Cortex A53 hard
> processor system (HPS) and Secure Device Manager (SDM). SDM is the hardware
> which does the FPGA configuration, QSPI, Crypto and warm reset.
> 
> When the FPGA is configured from HPS, there needs to be a way for HPS to
> notify SDM the location and size of the configuration data. Then SDM will
> get the configuration data from that location and perform the FPGA configuration.
> 
> To meet the whole system security needs and support virtual machine
> requesting communication with SDM, only the secure world of software (EL3,
> Exception Level 3) can interface with SDM. All software entities running
> on other exception levels must channel through the EL3 software whenever it
> needs service from SDM.
> 
> Intel Stratix10 service layer driver is added to provide the service for
> FPGA configuration. Running at privileged exception level (EL1, Exception
> Level 1), Intel Stratix10 service layer driver interfaces with the service
> provider at EL1 (Intel Stratix10 FPGA Manager) and manages secure monitor
> call (SMC) to communicate with secure monitor software at secure monitor
> exception level (EL3).
> 
> Later the Intel Stratix10 service layer driver will be extended to provide
> services for QSPI, Crypto and warm reset.
> 
> Richard Gong (1):
>   driver: misc: add Intel Stratix10 service layer driver
> 
>  drivers/misc/Kconfig                       |   3 +-
>  drivers/misc/Makefile                      |   3 +-
>  drivers/misc/intel-service/Kconfig         |   9 +
>  drivers/misc/intel-service/Makefile        |   2 +
>  drivers/misc/intel-service/intel_service.c | 703 +++++++++++++++++++++++++++++
>  include/linux/intel-service-client.h       | 227 ++++++++++
>  include/linux/intel-service.h              | 122 +++++
>  include/linux/intel-smc.h                  | 246 ++++++++++

Simple questions first:
 - why do you have 3 different .h files for a single .c file?
 - why do you have any public .h files for a single .c file?
 - use the correct SPDX markers for your file licenses, Intel legal
   knows all about this, please follow their rules.
 - why is this in a subdirectory for a single .c file?

thanks,

greg k-h

Re: [PATCHv1] driver: misc: add Intel Stratix10 service layer driver

[Greg KH]

On Thu, Jan 25, 2018 at 10:39:04AM -0600, richard.gong@linux.intel.com wrote:
> diff --git a/drivers/misc/Kconfig b/drivers/misc/Kconfig
> index 4c842e8..eaca4bb 100644
> --- a/drivers/misc/Kconfig
> +++ b/drivers/misc/Kconfig
> @@ -1,4 +1,4 @@
> -#
> +# SPDX-License-Identifier: GPL-2.0
>  # Misc strange devices
>  #
>  
> @@ -528,6 +528,7 @@ source "drivers/misc/eeprom/Kconfig"

Why do this?

Re: [PATCHv1] driver: misc: add Intel Stratix10 service layer driver

[Greg KH]

On Thu, Jan 25, 2018 at 10:39:04AM -0600, richard.gong@linux.intel.com wrote:
> --- /dev/null
> +++ b/drivers/misc/intel-service/Kconfig
> @@ -0,0 +1,9 @@
> +# SPDX-License-Identifier: GPL-2.0
> +config INTEL_SERVICE
> +	tristate "Intel Service Controller"
> +	default y

Unless the machine can not boot without it, it needs to be 'default n'
or no default at all (which is default n).

> +	depends on ARCH_STRATIX10

Why can this not be built on any other configuration?  Why not test
built at the least?

> +++ b/drivers/misc/intel-service/intel_service.c
> @@ -0,0 +1,703 @@
> +// SPDX-License-Identifier: GPL-2.0

Ok, you use SPDX, but then you do this:

> +/*
> + * Copyright (C) 2017-2018, Intel Corporation
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms and conditions of the GNU General Public License,
> + * version 2, as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope it will be useful, but WITHOUT
> + * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
> + * FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
> + * more details.
> + *
> + * You should have received a copy of the GNU General Public License along with
> + * this program.  If not, see <http://www.gnu.org/licenses/>.

Nothing except that copyright line needs to be there.  Please delete the
rest of the boilerplate crud.

thanks,

greg k-h

Re: [PATCHv1] driver: misc: add Intel Stratix10 service layer driver

[Greg KH]

On Thu, Jan 25, 2018 at 10:39:04AM -0600, richard.gong@linux.intel.com wrote:
> +static LIST_HEAD(svc_cons);
> +static LIST_HEAD(svc_private_mem);
> +static DEFINE_MUTEX(svc_mutex);
> +
> +svc_invoke_fn *invoke_fn;

A global variable for a single .c file?  No.

> +
> +struct intel_svc_chan *request_svc_channel_byname(
> +	struct intel_svc_client *client, const char *name)
> +{
> +	struct device *dev = client->dev;
> +	struct intel_svc_controller *controller;
> +	struct intel_svc_chan *chan;
> +	unsigned long flag;
> +	int i;
> +
> +	chan = ERR_PTR(-EPROBE_DEFER);
> +	controller = list_first_entry(&svc_cons,
> +				      struct intel_svc_controller, node);
> +	for (i = 0; i < SVC_MAX_CHANNEL; i++) {
> +		if (!strcmp(controller->chans[i].name, name)) {
> +			chan = &controller->chans[i];
> +			break;
> +		}
> +	}
> +
> +	if (chan->scl || !try_module_get(controller->dev->driver->owner)) {
> +		dev_dbg(dev, "%s: svc not free\n", __func__);
> +		return ERR_PTR(-EBUSY);
> +	}
> +
> +	spin_lock_irqsave(&chan->lock, flag);
> +	chan->scl = client;
> +	spin_unlock_irqrestore(&chan->lock, flag);
> +
> +	return chan;
> +}
> +EXPORT_SYMBOL_GPL(request_svc_channel_byname);

Exporting functions with no users?  No.

Please fix up and make this stand-alone.  Or, if you are going to have
framework that others use, then submit those others at the same time.

I'm stopping reviewing here, sorry.

Please fix up and resend.

Also, please get some signed-off-by: from some internal Intel people.
Don't rely on me to do this type of work when you have lots of good
people inside that could have told you all of this already.  Without
other developer's sign-offs, I'm not going to look at future
submissions, sorry.

thanks,

greg k-h

Re: [PATCHv1] driver: misc: add Intel Stratix10 service layer driver

[Randy Dunlap]

On 01/25/2018 08:39 AM, richard.gong@linux.intel.com wrote:
> From: Richard Gong <richard.gong@intel.com>
> 
> 
> Signed-off-by: Richard Gong <richard.gong@intel.com>
> ---
>  drivers/misc/Kconfig                       |   3 +-
>  drivers/misc/Makefile                      |   3 +-
>  drivers/misc/intel-service/Kconfig         |   9 +
>  drivers/misc/intel-service/Makefile        |   2 +
>  drivers/misc/intel-service/intel_service.c | 703 +++++++++++++++++++++++++++++
>  include/linux/intel-service-client.h       | 227 ++++++++++
>  include/linux/intel-service.h              | 122 +++++
>  include/linux/intel-smc.h                  | 246 ++++++++++
>  8 files changed, 1313 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/misc/intel-service/Kconfig
>  create mode 100644 drivers/misc/intel-service/Makefile
>  create mode 100644 drivers/misc/intel-service/intel_service.c
>  create mode 100644 include/linux/intel-service-client.h
>  create mode 100644 include/linux/intel-service.h
>  create mode 100644 include/linux/intel-smc.h
> 
> diff --git a/drivers/misc/intel-service/intel_service.c b/drivers/misc/intel-service/intel_service.c
> new file mode 100644
> index 0000000..90f883c
> --- /dev/null
> +++ b/drivers/misc/intel-service/intel_service.c
> @@ -0,0 +1,703 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2017-2018, Intel Corporation

> +
> +#include <linux/bitops.h>
> +#include <linux/completion.h>
> +#include <linux/delay.h>
> +#include <linux/device.h>
> +#include <linux/err.h>
> +#include <linux/intel-service.h>
> +#include <linux/intel-service-client.h>
> +#include <linux/intel-smc.h>
> +#include <linux/interrupt.h>
> +#include <linux/io.h>
> +#include <linux/kthread.h>
> +#include <linux/module.h>
> +#include <linux/mutex.h>
> +#include <linux/of_address.h>
> +#include <linux/of.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#define SVC_FO_LEN	32
> +#define SVC_MAX_CHANNEL	2
> +
> +static LIST_HEAD(svc_cons);
> +static LIST_HEAD(svc_private_mem);
> +static DEFINE_MUTEX(svc_mutex);
> +
> +svc_invoke_fn *invoke_fn;

Is that calling into firmware or what?

> +	/* timeout for polling FPGA configuration status
> +	 * it is set to 30 seconds (30 * 1000) at Intel Stratix10 SoC
> +	 */

Fix multi-line comment style (several places).






> diff --git a/include/linux/intel-service-client.h b/include/linux/intel-service-client.h
> new file mode 100644
> index 0000000..5bb6513
> --- /dev/null
> +++ b/include/linux/intel-service-client.h
> @@ -0,0 +1,227 @@
> +
> +#ifndef __INTEL_SERVICE_CLIENT_H
> +#define __INTEL_SERVICE_CLIENT_H
> +
> +#include <linux/of.h>
> +#include <linux/device.h>
> +
> +/**

In kernel source files, /** means that the following is a kernel-doc formatted
comment.  This is close, but not quite.  Others also are just close.

> + * Service driver supports client names
> + * @fpga: for FPGA configuration
> + * @dummy: for integration/debug/trouble-shooting
> + */
> +#define SVC_CLIENT_FPGA		"fpga"
> +#define SVC_CLIENT_DUMMY	"dummy"
> +
> +/**

Hm, these comments would be close to kernel-doc notation for an enum type,
but not for a list of #defines.

> + * Status of the sent command, in bit number
> + * @SVC_COMMAND_STATUS_RECONFIG_REQUEST_OK
> + *	SDM accepts the request of FPGA reconfiguration
> + * @SVC_STATUS_RECONFIG_BUFFER_SUBMITTED
> + *	Service client successfully submits FPGA configuration
> + *	data buffer to SDM
> + * @SVC_COMMAND_STATUS_RECONFIG_BUFFER_DONE
> + *	SDM completes data process, ready to accept the
> + *      next WRITE transaction
> + * @SVC_COMMAND_STATUS_RECONFIG_COMPLETED
> + *	SDM completes FPGA configuration successfully, FPGA should
> + *	be in user mode
> + * @SVC_COMMAND_STATUS_RECONFIG_BUSY
> + *	FPGA configuration is still in process
> + * @SVC_COMMAND_STATUS_RECONFIG_ERROR
> + *	Error encountered during FPGA configuration
> + */
> +#define SVC_STATUS_RECONFIG_REQUEST_OK		0
> +#define SVC_STATUS_RECONFIG_BUFFER_SUBMITTED	1
> +#define SVC_STATUS_RECONFIG_BUFFER_DONE		2
> +#define SVC_STATUS_RECONFIG_COMPLETED		3
> +#define SVC_STATUS_RECONFIG_BUSY			4
> +#define SVC_STATUS_RECONFIG_ERROR			5
> +
> +struct intel_svc_chan;
> +

> diff --git a/include/linux/intel-service.h b/include/linux/intel-service.h
> new file mode 100644
> index 0000000..fdf21b3
> --- /dev/null
> +++ b/include/linux/intel-service.h
> @@ -0,0 +1,122 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +#ifndef __INTEL_SERVICE_H
> +#define __INTEL_SERVICE_H
> +
> +#include <linux/of.h>
> +#include <linux/types.h>
> +#include <linux/device.h>
> +#include <linux/kfifo.h>
> +#include <linux/genalloc.h>
> +#include <linux/arm-smccc.h>
> +
> +
> +/**

Ah, this one is formatted correctly as kernel-doc.

> + * struct intel_svc_sh_memory - service shared memory info
> + *
> + * @sync_complete: maintain state for a completion
> + * @start_addr: starting address of shared memory
> + * @size: size of shared memory
> + */
> +struct intel_svc_sh_memory {
> +	struct completion sync_complete;
> +	unsigned long start_addr;
> +	unsigned long size;
> +};
> +
> +/**
> + * struct intel_svc_private_mem - service private memory info
> + *
> + * @kaddr: virtual address
> + * @paddr: physical address
> + * @size: size of memory
> + * @node: link list head node
> + */
> +struct intel_svc_private_mem {
> +	void *kaddr;
> +	phys_addr_t paddr;
> +	size_t size;
> +	struct list_head node;
> +};
> +
> +/**
> + * struct intel_svc_private_data - service private data
> + *
> + * @chan: service channel
> + * @paddr: play-load physical address
> + * @size: play-load size
> + * @ommand: service request command

      @command:

> + */
> +struct intel_svc_private_data {
> +	struct intel_svc_chan *chan;
> +	phys_addr_t paddr;
> +	size_t size;
> +	u32 command;
> +};
> +
> +/**
> + * struct intel_svc_controller - service controller
> + *
> + * @dev: device backing this controller
> + * @chans: array of service channels
> + * $num_chans: number of channels in 'chans' array
> + * @node: list management

missing 2 entries.

> + * @intel_svc_fifo: a queue for storing service message data
> + * @svc_fifo_lock: protect access to service message data queue
> + */
> +struct intel_svc_controller {
> +	struct device *dev;
> +	struct intel_svc_chan *chans;
> +	int num_chans;
> +
> +	struct list_head node;
> +
> +	struct gen_pool *genpool;
> +	struct intel_svc_prviate_mem *private_mem;
> +
> +	DECLARE_KFIFO_PTR(intel_svc_fifo, struct intel_svc_private_data *);
> +	/* protect access to the service message data queue*/
> +	spinlock_t svc_fifo_lock;
> +};
> +
> +/**
> + * struct intel_svc_chan - service communication channel
> + *
> + * @ctrl: pointer to the provider of this channel
> + * @scl: pointer to service client which owns this channel
> + * $name: name associated with this service channel

      @name:

> + * @lock: protect access to the channel
> + */
> +struct intel_svc_chan {
> +	struct intel_svc_controller *ctrl;
> +	struct intel_svc_client *scl;
> +	char *name;
> +	/* protect access to the channel*/
> +	spinlock_t lock;
> +};
> +
> +static int intel_svc_smc_thread(void *data);
> +
> +#endif



-- 
~Randy

Re: [PATCHv1] Add Intel Stratix10 service layer driver

[Richard Gong]

Hi Greg,

Many thanks for your reviews.


On 01/25/2018 10:53 AM, Greg KH wrote:
> On Thu, Jan 25, 2018 at 10:39:03AM -0600, richard.gong@linux.intel.com wrote:
>> From: Richard Gong <richard.gong@intel.com>
>>
>> Intel Stratix10 SoC is composed of a 64 bit quad-core ARM Cortex A53 hard
>> processor system (HPS) and Secure Device Manager (SDM). SDM is the hardware
>> which does the FPGA configuration, QSPI, Crypto and warm reset.
>>
>> When the FPGA is configured from HPS, there needs to be a way for HPS to
>> notify SDM the location and size of the configuration data. Then SDM will
>> get the configuration data from that location and perform the FPGA configuration.
>>
>> To meet the whole system security needs and support virtual machine
>> requesting communication with SDM, only the secure world of software (EL3,
>> Exception Level 3) can interface with SDM. All software entities running
>> on other exception levels must channel through the EL3 software whenever it
>> needs service from SDM.
>>
>> Intel Stratix10 service layer driver is added to provide the service for
>> FPGA configuration. Running at privileged exception level (EL1, Exception
>> Level 1), Intel Stratix10 service layer driver interfaces with the service
>> provider at EL1 (Intel Stratix10 FPGA Manager) and manages secure monitor
>> call (SMC) to communicate with secure monitor software at secure monitor
>> exception level (EL3).
>>
>> Later the Intel Stratix10 service layer driver will be extended to provide
>> services for QSPI, Crypto and warm reset.
>>
>> Richard Gong (1):
>>    driver: misc: add Intel Stratix10 service layer driver
>>
>>   drivers/misc/Kconfig                       |   3 +-
>>   drivers/misc/Makefile                      |   3 +-
>>   drivers/misc/intel-service/Kconfig         |   9 +
>>   drivers/misc/intel-service/Makefile        |   2 +
>>   drivers/misc/intel-service/intel_service.c | 703 +++++++++++++++++++++++++++++
>>   include/linux/intel-service-client.h       | 227 ++++++++++
>>   include/linux/intel-service.h              | 122 +++++
>>   include/linux/intel-smc.h                  | 246 ++++++++++
> Simple questions first:
>   - why do you have 3 different .h files for a single .c file?
This is because service layer driver interface with both the service 
provider and secure monitor SW.
intel-service-client.h is created to define interface between service 
providers (FPGA manager is one of them) and service layer. Alan Tull's 
FPGA manager .c file includes this header file
intel-smc.h defines the secure monitor call (SMC) message protocols used 
for service layer driver in normal world (EL1) to communicate with 
secure monitor SW in secure monitor exception level 3 (EL3). Also this 
header file is shared with firmware since both (FW, service layer) 
utilizes the same SMC message protocol.
intel-sevice.h is created to define service layer's own data structures 
(service controller, channel for communicating with service provider, 
shared memory region, private data etc)

>   - why do you have any public .h files for a single .c file?
intel-service-client.h is public .h and should be at include/linux/
intel-service.h and intel-smc.h are private .h files, should be in 
driver/misc/ (assume I move .c file from driver/misc/intel-service/ to 
driver/misc/)
>   - use the correct SPDX markers for your file licenses, Intel legal
>     knows all about this, please follow their rules.
I will follow those rules.
>   - why is this in a subdirectory for a single .c file?
Currently service layer is implemented to support FPGA configuration 
only. We have the new requirements and need to extend service layer to 
support additional features such as QSPI, Crypto and warm reset. It is 
expected that a few new files will be added later.
For now I can move the single .c file from driver/misc/intel-service/ to 
driver/misc/.

Regards,
Richard
> thanks,
>
> greg k-h

Re: [PATCHv1] Add Intel Stratix10 service layer driver

[Greg KH]

On Mon, Jan 29, 2018 at 08:08:11PM -0600, Richard Gong wrote:
> Hi Greg,
> 
> Many thanks for your reviews.
> 
> 
> On 01/25/2018 10:53 AM, Greg KH wrote:
> > On Thu, Jan 25, 2018 at 10:39:03AM -0600, richard.gong@linux.intel.com wrote:
> > > From: Richard Gong <richard.gong@intel.com>
> > > 
> > > Intel Stratix10 SoC is composed of a 64 bit quad-core ARM Cortex A53 hard
> > > processor system (HPS) and Secure Device Manager (SDM). SDM is the hardware
> > > which does the FPGA configuration, QSPI, Crypto and warm reset.
> > > 
> > > When the FPGA is configured from HPS, there needs to be a way for HPS to
> > > notify SDM the location and size of the configuration data. Then SDM will
> > > get the configuration data from that location and perform the FPGA configuration.
> > > 
> > > To meet the whole system security needs and support virtual machine
> > > requesting communication with SDM, only the secure world of software (EL3,
> > > Exception Level 3) can interface with SDM. All software entities running
> > > on other exception levels must channel through the EL3 software whenever it
> > > needs service from SDM.
> > > 
> > > Intel Stratix10 service layer driver is added to provide the service for
> > > FPGA configuration. Running at privileged exception level (EL1, Exception
> > > Level 1), Intel Stratix10 service layer driver interfaces with the service
> > > provider at EL1 (Intel Stratix10 FPGA Manager) and manages secure monitor
> > > call (SMC) to communicate with secure monitor software at secure monitor
> > > exception level (EL3).
> > > 
> > > Later the Intel Stratix10 service layer driver will be extended to provide
> > > services for QSPI, Crypto and warm reset.
> > > 
> > > Richard Gong (1):
> > >    driver: misc: add Intel Stratix10 service layer driver
> > > 
> > >   drivers/misc/Kconfig                       |   3 +-
> > >   drivers/misc/Makefile                      |   3 +-
> > >   drivers/misc/intel-service/Kconfig         |   9 +
> > >   drivers/misc/intel-service/Makefile        |   2 +
> > >   drivers/misc/intel-service/intel_service.c | 703 +++++++++++++++++++++++++++++
> > >   include/linux/intel-service-client.h       | 227 ++++++++++
> > >   include/linux/intel-service.h              | 122 +++++
> > >   include/linux/intel-smc.h                  | 246 ++++++++++
> > Simple questions first:
> >   - why do you have 3 different .h files for a single .c file?
> This is because service layer driver interface with both the service
> provider and secure monitor SW.
> intel-service-client.h is created to define interface between service
> providers (FPGA manager is one of them) and service layer. Alan Tull's FPGA
> manager .c file includes this header file
> intel-smc.h defines the secure monitor call (SMC) message protocols used for
> service layer driver in normal world (EL1) to communicate with secure
> monitor SW in secure monitor exception level 3 (EL3). Also this header file
> is shared with firmware since both (FW, service layer) utilizes the same SMC
> message protocol.
> intel-sevice.h is created to define service layer's own data structures
> (service controller, channel for communicating with service provider, shared
> memory region, private data etc)

That's very complex for a single patch submission, don't you think?

Please do not add new apis / interfaces for code that is not part of
your patch series, otherwise we don't know what the future is going to
hold :)

This feels like it should be a series of patches, to properly explain
this and hook up all of the new interfaces you are adding, right?

thanks,

greg k-h

Re: [PATCHv1] Add Intel Stratix10 service layer driver

[Alan Tull]

On Tue, Jan 30, 2018 at 1:41 AM, Greg KH <gregkh@linuxfoundation.org> wrote:
> On Mon, Jan 29, 2018 at 08:08:11PM -0600, Richard Gong wrote:
>> Hi Greg,
>>
>> Many thanks for your reviews.
>>
>>
>> On 01/25/2018 10:53 AM, Greg KH wrote:
>> > On Thu, Jan 25, 2018 at 10:39:03AM -0600, richard.gong@linux.intel.com wrote:
>> > > From: Richard Gong <richard.gong@intel.com>
>> > >
>> > > Intel Stratix10 SoC is composed of a 64 bit quad-core ARM Cortex A53 hard
>> > > processor system (HPS) and Secure Device Manager (SDM). SDM is the hardware
>> > > which does the FPGA configuration, QSPI, Crypto and warm reset.
>> > >
>> > > When the FPGA is configured from HPS, there needs to be a way for HPS to
>> > > notify SDM the location and size of the configuration data. Then SDM will
>> > > get the configuration data from that location and perform the FPGA configuration.
>> > >
>> > > To meet the whole system security needs and support virtual machine
>> > > requesting communication with SDM, only the secure world of software (EL3,
>> > > Exception Level 3) can interface with SDM. All software entities running
>> > > on other exception levels must channel through the EL3 software whenever it
>> > > needs service from SDM.
>> > >
>> > > Intel Stratix10 service layer driver is added to provide the service for
>> > > FPGA configuration. Running at privileged exception level (EL1, Exception
>> > > Level 1), Intel Stratix10 service layer driver interfaces with the service
>> > > provider at EL1 (Intel Stratix10 FPGA Manager) and manages secure monitor
>> > > call (SMC) to communicate with secure monitor software at secure monitor
>> > > exception level (EL3).
>> > >
>> > > Later the Intel Stratix10 service layer driver will be extended to provide
>> > > services for QSPI, Crypto and warm reset.
>> > >
>> > > Richard Gong (1):
>> > >    driver: misc: add Intel Stratix10 service layer driver
>> > >
>> > >   drivers/misc/Kconfig                       |   3 +-
>> > >   drivers/misc/Makefile                      |   3 +-
>> > >   drivers/misc/intel-service/Kconfig         |   9 +
>> > >   drivers/misc/intel-service/Makefile        |   2 +
>> > >   drivers/misc/intel-service/intel_service.c | 703 +++++++++++++++++++++++++++++
>> > >   include/linux/intel-service-client.h       | 227 ++++++++++
>> > >   include/linux/intel-service.h              | 122 +++++
>> > >   include/linux/intel-smc.h                  | 246 ++++++++++
>> > Simple questions first:
>> >   - why do you have 3 different .h files for a single .c file?
>> This is because service layer driver interface with both the service
>> provider and secure monitor SW.
>> intel-service-client.h is created to define interface between service
>> providers (FPGA manager is one of them) and service layer. Alan Tull's FPGA
>> manager .c file includes this header file
>> intel-smc.h defines the secure monitor call (SMC) message protocols used for
>> service layer driver in normal world (EL1) to communicate with secure
>> monitor SW in secure monitor exception level 3 (EL3). Also this header file
>> is shared with firmware since both (FW, service layer) utilizes the same SMC
>> message protocol.
>> intel-sevice.h is created to define service layer's own data structures
>> (service controller, channel for communicating with service provider, shared
>> memory region, private data etc)
>
> That's very complex for a single patch submission, don't you think?

Hi Greg,

I'm reviewing this patchset now.  v2 will simplify things down and add
better documentation.  Only 2 headers (one for the client API and one
to define the interface with the out of tree code this talks to).

>
> Please do not add new apis / interfaces for code that is not part of
> your patch series, otherwise we don't know what the future is going to
> hold :)

The v2 patch set will include my FPGA manager driver which is the
first client of this service.  My driver will use all the API
functions exported by this service driver.

>
> This feels like it should be a series of patches, to properly explain
> this and hook up all of the new interfaces you are adding, right?

There will be other clients, so there will be other patchsets on top
of this.  We'll be sure to include new API functions and the client
that needs them in the same patch set in the future.

Thanks for your feedback, helpful as always!

Alan

>
> thanks,
>
> greg k-h