Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Subject: Re: [PATCHv1] driver: misc: add Intel Stratix10 service layer driver
To:     richard.gong@linux.intel.com, arnd@arndb.de,
        gregkh@linuxfoundation.org
Cc:     linux-kernel@vger.kernel.org, richard.gong@intel.com
References: <1516898344-20020-1-git-send-email-richard.gong@linux.intel.com>
 <1516898344-20020-2-git-send-email-richard.gong@linux.intel.com>
From:   Randy Dunlap <rdunlap@infradead.org>
Message-ID: <2b2e5eb3-1786-c772-fb60-d1f9c3ad9036@infradead.org>
Date:   Thu, 25 Jan 2018 15:27:20 -0800
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101
 Thunderbird/52.3.0
MIME-Version: 1.0
In-Reply-To: <1516898344-20020-2-git-send-email-richard.gong@linux.intel.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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