[PATCH linux dev-4.10 0/3] drivers: fsi: Add OCC driver with SBEFIFO in-kernel API

[PATCH linux dev-4.10 0/3] drivers: fsi: Add OCC driver with SBEFIFO in-kernel API

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

This patch series adds a driver to communicate with the POWER processor
On-Chip Controller (OCC), from a service processor. Communication is performed
via the SBEFIFO driver, which required a new in-kernel API.

The patch series includes device-tree updates to instantiate devices for the
OCC driver from the SBEFIFO driver.

An application to test the OCC driver can be found at
https://github.com/eddiejames/occtest

Edward A. James (3):
  drivers: fsi: sbefifo: Add in-kernel API
  drivers: fsi: sbefifo: Add OCC driver
  dts: aspeed: witherspoon: Add FSI devices

 arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts |  46 ++
 drivers/fsi/Kconfig                              |   9 +
 drivers/fsi/Makefile                             |   1 +
 drivers/fsi/fsi-sbefifo.c                        | 161 ++++--
 drivers/fsi/occ.c                                | 625 +++++++++++++++++++++++
 include/linux/fsi-sbefifo.h                      |  30 ++
 6 files changed, 842 insertions(+), 30 deletions(-)
 create mode 100644 drivers/fsi/occ.c
 create mode 100644 include/linux/fsi-sbefifo.h

-- 
1.8.3.1

[PATCH linux dev-4.10 1/3] drivers: fsi: sbefifo: Add in-kernel API

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Add exported functions to the SBEFIFO driver to open/write/read/close
from within the kernel.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/fsi-sbefifo.c   | 161 +++++++++++++++++++++++++++++++++++---------
 include/linux/fsi-sbefifo.h |  30 +++++++++
 2 files changed, 161 insertions(+), 30 deletions(-)
 create mode 100644 include/linux/fsi-sbefifo.h

diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
index b49aec2..56e6331 100644
--- a/drivers/fsi/fsi-sbefifo.c
+++ b/drivers/fsi/fsi-sbefifo.c
@@ -15,9 +15,12 @@
 #include <linux/errno.h>
 #include <linux/idr.h>
 #include <linux/fsi.h>
+#include <linux/fsi-sbefifo.h>
 #include <linux/list.h>
 #include <linux/miscdevice.h>
 #include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
 #include <linux/poll.h>
 #include <linux/sched.h>
 #include <linux/slab.h>
@@ -82,6 +85,7 @@ struct sbefifo_client {
 	struct list_head xfrs;
 	struct sbefifo *dev;
 	struct kref kref;
+	unsigned long f_flags;
 };
 
 static struct list_head sbefifo_fifos;
@@ -506,6 +510,7 @@ static int sbefifo_open(struct inode *inode, struct file *file)
 		return -ENOMEM;
 
 	file->private_data = client;
+	client->f_flags = file->f_flags;
 
 	return 0;
 }
@@ -530,24 +535,18 @@ static unsigned int sbefifo_poll(struct file *file, poll_table *wait)
 	return mask;
 }
 
-static ssize_t sbefifo_read(struct file *file, char __user *buf,
-		size_t len, loff_t *offset)
+static ssize_t sbefifo_read_common(struct sbefifo_client *client,
+				   char __user *ubuf, char *kbuf, size_t len)
 {
-	struct sbefifo_client *client = file->private_data;
 	struct sbefifo *sbefifo = client->dev;
 	struct sbefifo_xfr *xfr;
-	ssize_t ret = 0;
 	size_t n;
-
-	WARN_ON(*offset);
-
-	if (!access_ok(VERIFY_WRITE, buf, len))
-		return -EFAULT;
+	ssize_t ret = 0;
 
 	if ((len >> 2) << 2 != len)
 		return -EINVAL;
 
-	if ((file->f_flags & O_NONBLOCK) && !sbefifo_xfr_rsp_pending(client))
+	if ((client->f_flags & O_NONBLOCK) && !sbefifo_xfr_rsp_pending(client))
 		return -EAGAIN;
 
 	sbefifo_get_client(client);
@@ -566,10 +565,13 @@ static ssize_t sbefifo_read(struct file *file, char __user *buf,
 
 	n = min_t(size_t, n, len);
 
-	if (copy_to_user(buf, READ_ONCE(client->rbuf.rpos), n)) {
-		sbefifo_put_client(client);
-		return -EFAULT;
-	}
+	if (ubuf) {
+		if (copy_to_user(ubuf, READ_ONCE(client->rbuf.rpos), n)) {
+			sbefifo_put_client(client);
+			return -EFAULT;
+		}
+	} else
+		memcpy(kbuf, READ_ONCE(client->rbuf.rpos), n);
 
 	if (sbefifo_buf_readnb(&client->rbuf, n)) {
 		xfr = sbefifo_client_next_xfr(client);
@@ -592,20 +594,28 @@ static ssize_t sbefifo_read(struct file *file, char __user *buf,
 	return n;
 }
 
-static ssize_t sbefifo_write(struct file *file, const char __user *buf,
+static ssize_t sbefifo_read(struct file *file, char __user *buf,
 		size_t len, loff_t *offset)
 {
 	struct sbefifo_client *client = file->private_data;
-	struct sbefifo *sbefifo = client->dev;
-	struct sbefifo_xfr *xfr;
-	ssize_t ret = 0;
-	size_t n;
 
 	WARN_ON(*offset);
 
-	if (!access_ok(VERIFY_READ, buf, len))
+	if (!access_ok(VERIFY_WRITE, buf, len))
 		return -EFAULT;
 
+	return sbefifo_read_common(client, buf, NULL, len);
+}
+
+static ssize_t sbefifo_write_common(struct sbefifo_client *client,
+				    const char __user *ubuf, const char *kbuf,
+				    size_t len)
+{
+	struct sbefifo *sbefifo = client->dev;
+	struct sbefifo_xfr *xfr;
+	ssize_t ret = 0;
+	size_t n;
+
 	if ((len >> 2) << 2 != len)
 		return -EINVAL;
 
@@ -617,7 +627,7 @@ static ssize_t sbefifo_write(struct file *file, const char __user *buf,
 	spin_lock_irq(&sbefifo->lock);
 	xfr = sbefifo_next_xfr(sbefifo);
 
-	if ((file->f_flags & O_NONBLOCK) && xfr && n < len) {
+	if ((client->f_flags & O_NONBLOCK) && xfr && n < len) {
 		spin_unlock_irq(&sbefifo->lock);
 		return -EAGAIN;
 	}
@@ -657,18 +667,25 @@ static ssize_t sbefifo_write(struct file *file, const char __user *buf,
 
 		n = min_t(size_t, n, len);
 
-		if (copy_from_user(READ_ONCE(client->wbuf.wpos), buf, n)) {
-			set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
-			sbefifo_get(sbefifo);
-			if (mod_timer(&sbefifo->poll_timer, jiffies))
-				sbefifo_put(sbefifo);
-			sbefifo_put_client(client);
-			return -EFAULT;
+		if (ubuf) {
+			if (copy_from_user(READ_ONCE(client->wbuf.wpos), ubuf,
+			    n)) {
+				set_bit(SBEFIFO_XFR_CANCEL, &xfr->flags);
+				sbefifo_get(sbefifo);
+				if (mod_timer(&sbefifo->poll_timer, jiffies))
+					sbefifo_put(sbefifo);
+				sbefifo_put_client(client);
+				return -EFAULT;
+			}
+
+			ubuf += n;
+		} else {
+			memcpy(READ_ONCE(client->wbuf.wpos), kbuf, n);
+			kbuf += n;
 		}
 
 		sbefifo_buf_wrotenb(&client->wbuf, n);
 		len -= n;
-		buf += n;
 		ret += n;
 
 		/*
@@ -685,6 +702,19 @@ static ssize_t sbefifo_write(struct file *file, const char __user *buf,
 	return ret;
 }
 
+static ssize_t sbefifo_write(struct file *file, const char __user *buf,
+		size_t len, loff_t *offset)
+{
+	struct sbefifo_client *client = file->private_data;
+
+	WARN_ON(*offset);
+
+	if (!access_ok(VERIFY_READ, buf, len))
+		return -EFAULT;
+
+	return sbefifo_write_common(client, buf, NULL, len);
+}
+
 static int sbefifo_release(struct inode *inode, struct file *file)
 {
 	struct sbefifo_client *client = file->private_data;
@@ -704,12 +734,68 @@ static int sbefifo_release(struct inode *inode, struct file *file)
 	.release	= sbefifo_release,
 };
 
+struct sbefifo_client *sbefifo_drv_open(struct device *dev,
+					unsigned long flags)
+{
+	struct sbefifo_client *client = NULL;
+	struct sbefifo *sbefifo;
+	struct fsi_device *fsi_dev = to_fsi_dev(dev);
+
+	list_for_each_entry(sbefifo, &sbefifo_fifos, link) {
+		if (sbefifo->fsi_dev != fsi_dev)
+			continue;
+
+		client = sbefifo_new_client(sbefifo);
+		if (client)
+			client->f_flags = flags;
+	}
+
+	return client;
+}
+EXPORT_SYMBOL_GPL(sbefifo_drv_open);
+
+int sbefifo_drv_read(struct sbefifo_client *client, char *buf, size_t len)
+{
+	return sbefifo_read_common(client, NULL, buf, len);
+}
+EXPORT_SYMBOL_GPL(sbefifo_drv_read);
+
+int sbefifo_drv_write(struct sbefifo_client *client, const char *buf,
+		      size_t len)
+{
+	return sbefifo_write_common(client, NULL, buf, len);
+}
+EXPORT_SYMBOL_GPL(sbefifo_drv_write);
+
+void sbefifo_drv_release(struct sbefifo_client *client)
+{
+	if (!client)
+		return;
+
+	sbefifo_put_client(client);
+}
+EXPORT_SYMBOL_GPL(sbefifo_drv_release);
+
+static int sbefifo_unregister_child(struct device *dev, void *data)
+{
+	struct platform_device *child = to_platform_device(dev);
+
+	of_device_unregister(child);
+	if (dev->of_node)
+		of_node_clear_flag(dev->of_node, OF_POPULATED);
+
+	return 0;
+}
+
 static int sbefifo_probe(struct device *dev)
 {
 	struct fsi_device *fsi_dev = to_fsi_dev(dev);
 	struct sbefifo *sbefifo;
+	struct device_node *np;
+	struct platform_device *child;
+	char child_name[32];
 	u32 sts;
-	int ret;
+	int ret, child_idx = 0;
 
 	dev_info(dev, "Found sbefifo device\n");
 	sbefifo = kzalloc(sizeof(*sbefifo), GFP_KERNEL);
@@ -750,6 +836,18 @@ static int sbefifo_probe(struct device *dev)
 	init_waitqueue_head(&sbefifo->wait);
 	INIT_LIST_HEAD(&sbefifo->xfrs);
 
+	if (dev->of_node) {
+		/* create platform devs for dts child nodes (occ, etc) */
+		for_each_child_of_node(dev->of_node, np) {
+			snprintf(child_name, sizeof(child_name), "%s-dev%d",
+				 sbefifo->name, child_idx++);
+			child = of_platform_device_create(np, child_name, dev);
+			if (!child)
+				dev_warn(&sbefifo->fsi_dev->dev,
+					 "failed to create child node dev\n");
+		}
+	}
+
 	/* This bit of silicon doesn't offer any interrupts... */
 	setup_timer(&sbefifo->poll_timer, sbefifo_poll_timer,
 			(unsigned long)sbefifo);
@@ -767,6 +865,9 @@ static int sbefifo_remove(struct device *dev)
 	list_for_each_entry_safe(sbefifo, sbefifo_tmp, &sbefifo_fifos, link) {
 		if (sbefifo->fsi_dev != fsi_dev)
 			continue;
+
+		device_for_each_child(dev, NULL, sbefifo_unregister_child);
+
 		misc_deregister(&sbefifo->mdev);
 		list_del(&sbefifo->link);
 		ida_simple_remove(&sbefifo_ida, sbefifo->idx);
diff --git a/include/linux/fsi-sbefifo.h b/include/linux/fsi-sbefifo.h
new file mode 100644
index 0000000..1b46c63
--- /dev/null
+++ b/include/linux/fsi-sbefifo.h
@@ -0,0 +1,30 @@
+/*
+ * SBEFIFO FSI Client device driver
+ *
+ * Copyright (C) IBM Corporation 2017
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERGCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef __FSI_SBEFIFO_H__
+#define __FSI_SBEFIFO_H__
+
+struct device;
+struct sbefifo_client;
+
+extern struct sbefifo_client *sbefifo_drv_open(struct device *dev,
+					       unsigned long flags);
+extern int sbefifo_drv_read(struct sbefifo_client *client, char *buf,
+			    size_t len);
+extern int sbefifo_drv_write(struct sbefifo_client *client, const char *buf,
+			     size_t len);
+extern void sbefifo_drv_release(struct sbefifo_client *client);
+
+#endif /* __FSI_SBEFIFO_H__ */
-- 
1.8.3.1

[PATCH linux dev-4.10 2/3] drivers: fsi: sbefifo: Add OCC driver

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

This driver provides an atomic communications channel between the OCC on
the POWER9 processor and a service processor (a BMC). The driver is
dependent on the FSI SBEIFO driver to get hardware access to the OCC
SRAM.

The format of the communication is a command followed by a response.
Since the command and response must be performed atomically, the driver
will perform this operations asynchronously. In this way, a write
operation starts the command, and a read will gather the response data
once it is complete.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 drivers/fsi/Kconfig  |   9 +
 drivers/fsi/Makefile |   1 +
 drivers/fsi/occ.c    | 625 +++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 635 insertions(+)
 create mode 100644 drivers/fsi/occ.c

diff --git a/drivers/fsi/Kconfig b/drivers/fsi/Kconfig
index 39527fa..f3d8593 100644
--- a/drivers/fsi/Kconfig
+++ b/drivers/fsi/Kconfig
@@ -36,6 +36,15 @@ config FSI_SBEFIFO
 	---help---
 	This option enables an FSI based SBEFIFO device driver.
 
+if FSI_SBEFIFO
+
+config OCC
+	tristate "OCC SBEFIFO client device driver"
+	---help---
+	This option enables an SBEFIFO based OCC device driver.
+
+endif
+
 endif
 
 endmenu
diff --git a/drivers/fsi/Makefile b/drivers/fsi/Makefile
index 851182e..336d9d5 100644
--- a/drivers/fsi/Makefile
+++ b/drivers/fsi/Makefile
@@ -4,3 +4,4 @@ obj-$(CONFIG_FSI_MASTER_HUB) += fsi-master-hub.o
 obj-$(CONFIG_FSI_MASTER_GPIO) += fsi-master-gpio.o
 obj-$(CONFIG_FSI_SCOM) += fsi-scom.o
 obj-$(CONFIG_FSI_SBEFIFO) += fsi-sbefifo.o
+obj-$(CONFIG_OCC) += occ.o
diff --git a/drivers/fsi/occ.c b/drivers/fsi/occ.c
new file mode 100644
index 0000000..74272c8
--- /dev/null
+++ b/drivers/fsi/occ.c
@@ -0,0 +1,625 @@
+/*
+ * Copyright 2017 IBM Corp.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ */
+
+#include <asm/unaligned.h>
+#include <linux/device.h>
+#include <linux/err.h>
+#include <linux/fsi-sbefifo.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/miscdevice.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/uaccess.h>
+#include <linux/wait.h>
+#include <linux/workqueue.h>
+
+#define OCC_SRAM_BYTES		4096
+#define OCC_CMD_DATA_BYTES	4090
+#define OCC_RESP_DATA_BYTES	4089
+
+struct occ {
+	struct device *sbefifo;
+	char name[32];
+	int idx;
+	struct miscdevice mdev;
+	struct list_head xfrs;
+	spinlock_t list_lock;
+	spinlock_t occ_lock;
+	struct work_struct work;
+};
+
+#define to_occ(x)	container_of((x), struct occ, mdev)
+
+struct occ_command {
+	u8 seq_no;
+	u8 cmd_type;
+	u16 data_length;
+	u8 data[OCC_CMD_DATA_BYTES];
+	u16 checksum;
+};
+
+struct occ_response {
+	u8 seq_no;
+	u8 cmd_type;
+	u8 return_status;
+	u16 data_length;
+	u8 data[OCC_RESP_DATA_BYTES];
+	u16 checksum;
+};
+
+struct occ_xfr;
+
+enum {
+	CLIENT_NONBLOCKING,
+};
+
+struct occ_client {
+	struct occ *occ;
+	struct occ_xfr *xfr;
+	spinlock_t lock;
+	wait_queue_head_t wait;
+	size_t read_offset;
+	unsigned long flags;
+};
+
+enum {
+	XFR_IN_PROGRESS,
+	XFR_COMPLETE,
+	XFR_CANCELED,
+	XFR_WAITING,
+};
+
+struct occ_xfr {
+	struct list_head link;
+	struct occ_client *client;
+	int rc;
+	u8 buf[OCC_SRAM_BYTES];
+	size_t cmd_data_length;
+	size_t resp_data_length;
+	unsigned long flags;
+};
+
+static struct workqueue_struct *occ_wq;
+
+static DEFINE_IDA(occ_ida);
+
+static void occ_enqueue_xfr(struct occ_xfr *xfr)
+{
+	int empty;
+	struct occ *occ = xfr->client->occ;
+
+	spin_lock_irq(&occ->list_lock);
+	empty = list_empty(&occ->xfrs);
+	list_add_tail(&xfr->link, &occ->xfrs);
+	spin_unlock(&occ->list_lock);
+
+	if (empty)
+		queue_work(occ_wq, &occ->work);
+}
+
+static int occ_open(struct inode *inode, struct file *file)
+{
+	struct occ_client *client;
+	struct miscdevice *mdev = file->private_data;
+	struct occ *occ = to_occ(mdev);
+
+	client = kzalloc(sizeof(*client), GFP_KERNEL);
+	if (!client)
+		return -ENOMEM;
+
+	client->occ = occ;
+	spin_lock_init(&client->lock);
+	init_waitqueue_head(&client->wait);
+
+	if (file->f_flags & O_NONBLOCK)
+		set_bit(CLIENT_NONBLOCKING, &client->flags);
+
+	file->private_data = client;
+
+	return 0;
+}
+
+static ssize_t occ_read(struct file *file, char __user *buf, size_t len,
+			loff_t *offset)
+{
+	int rc;
+	size_t bytes;
+	struct occ_xfr *xfr;
+	struct occ_client *client = file->private_data;
+
+	if (!access_ok(VERIFY_WRITE, buf, len))
+		return -EFAULT;
+
+	if (len > OCC_SRAM_BYTES)
+		return -EINVAL;
+
+	spin_lock_irq(&client->lock);
+	if (!client->xfr) {
+		/* we just finished reading all data, return 0 */
+		if (client->read_offset) {
+			rc = 0;
+			client->read_offset = 0;
+		} else
+			rc = -ENOMSG;
+
+		goto done;
+	}
+
+	xfr = client->xfr;
+
+	if (!test_bit(XFR_COMPLETE, &xfr->flags)) {
+		if (client->flags & CLIENT_NONBLOCKING) {
+			rc = -ERESTARTSYS;
+			goto done;
+		}
+
+		set_bit(XFR_WAITING, &xfr->flags);
+		spin_unlock(&client->lock);
+
+		rc = wait_event_interruptible(client->wait,
+			test_bit(XFR_COMPLETE, &xfr->flags) ||
+			test_bit(XFR_CANCELED, &xfr->flags));
+
+		spin_lock_irq(&client->lock);
+		if (test_bit(XFR_CANCELED, &xfr->flags)) {
+			kfree(xfr);
+			spin_unlock(&client->lock);
+			kfree(client);
+			return -EBADFD;
+		}
+
+		clear_bit(XFR_WAITING, &xfr->flags);
+		if (!test_bit(XFR_COMPLETE, &xfr->flags)) {
+			rc = -EINTR;
+			goto done;
+		}
+	}
+
+	if (xfr->rc) {
+		rc = xfr->rc;
+		goto done;
+	}
+
+	bytes = min(len, xfr->resp_data_length - client->read_offset);
+	if (copy_to_user(buf, &xfr->buf[client->read_offset], bytes)) {
+		rc = -EFAULT;
+		goto done;
+	}
+
+	client->read_offset += bytes;
+
+	/* xfr done */
+	if (client->read_offset == xfr->resp_data_length) {
+		kfree(xfr);
+		client->xfr = NULL;
+	}
+
+	rc = bytes;
+
+done:
+	spin_unlock(&client->lock);
+	return rc;
+}
+
+static ssize_t occ_write(struct file *file, const char __user *buf,
+			 size_t len, loff_t *offset)
+{
+	int rc;
+	struct occ_xfr *xfr;
+	struct occ_client *client = file->private_data;
+
+	if (!access_ok(VERIFY_READ, buf, len))
+		return -EFAULT;
+
+	if (len > OCC_SRAM_BYTES)
+		return -EINVAL;
+
+	spin_lock_irq(&client->lock);
+	if (client->xfr) {
+		rc = -EDEADLK;
+		goto done;
+	}
+
+	xfr = kzalloc(sizeof(*xfr), GFP_KERNEL);
+	if (!xfr) {
+		rc = -ENOMEM;
+		goto done;
+	}
+
+	if (copy_from_user(xfr->buf, buf, len)) {
+		kfree(xfr);
+		rc = -EFAULT;
+		goto done;
+	}
+
+	xfr->client = client;
+	xfr->cmd_data_length = len;
+	client->xfr = xfr;
+	client->read_offset = 0;
+
+	occ_enqueue_xfr(xfr);
+
+	rc = len;
+
+done:
+	spin_unlock(&client->lock);
+	return rc;
+}
+
+static int occ_release(struct inode *inode, struct file *file)
+{
+	struct occ_xfr *xfr;
+	struct occ_client *client = file->private_data;
+	struct occ *occ = client->occ;
+
+	spin_lock_irq(&client->lock);
+	xfr = client->xfr;
+	if (!xfr) {
+		spin_unlock(&client->lock);
+		kfree(client);
+		return 0;
+	}
+
+	spin_lock_irq(&occ->list_lock);
+	set_bit(XFR_CANCELED, &xfr->flags);
+	if (!test_bit(XFR_IN_PROGRESS, &xfr->flags)) {
+		/* already deleted from list if complete */
+		if (!test_bit(XFR_COMPLETE, &xfr->flags))
+			list_del(&xfr->link);
+
+		spin_unlock(&occ->list_lock);
+
+		if (test_bit(XFR_WAITING, &xfr->flags)) {
+			/* blocking read; let reader clean up */
+			wake_up_interruptible(&client->wait);
+			spin_unlock(&client->lock);
+			return 0;
+		}
+
+		kfree(xfr);
+		spin_unlock(&client->lock);
+		kfree(client);
+		return 0;
+	}
+
+	/* operation is in progress; let worker clean up*/
+	spin_unlock(&occ->list_lock);
+	spin_unlock(&client->lock);
+	return 0;
+}
+
+static const struct file_operations occ_fops = {
+	.owner = THIS_MODULE,
+	.open = occ_open,
+	.read = occ_read,
+	.write = occ_write,
+	.release = occ_release,
+};
+
+static int occ_getscom(struct device *sbefifo, u32 address, u8 *data)
+{
+	int rc;
+	u32 buf[4];
+	struct sbefifo_client *client;
+	const size_t len = sizeof(buf);
+
+	buf[0] = cpu_to_be32(0x4);
+	buf[1] = cpu_to_be32(0xa201);
+	buf[2] = 0;
+	buf[3] = cpu_to_be32(address);
+
+	client = sbefifo_drv_open(sbefifo, 0);
+	if (!client)
+		return -ENODEV;
+
+	rc = sbefifo_drv_write(client, (const char *)buf, len);
+	if (rc < 0)
+		goto done;
+	else if (rc != len) {
+		rc = -EMSGSIZE;
+		goto done;
+	}
+
+	rc = sbefifo_drv_read(client, (char *)buf, len);
+	if (rc < 0)
+		goto done;
+	else if (rc != len) {
+		rc = -EMSGSIZE;
+		goto done;
+	}
+
+	/* check for good response */
+	if ((be32_to_cpu(buf[2]) >> 16) != 0xC0DE) {
+		rc = -EFAULT;
+		goto done;
+	}
+
+	rc = 0;
+
+	memcpy(data, buf, sizeof(u64));
+
+done:
+	sbefifo_drv_release(client);
+	return rc;
+}
+
+static int occ_putscom(struct device *sbefifo, u32 address, u8 *data)
+{
+	int rc;
+	u32 buf[6];
+	struct sbefifo_client *client;
+	const size_t len = sizeof(buf);
+
+	buf[0] = cpu_to_be32(0x6);
+	buf[1] = cpu_to_be32(0xa202);
+	buf[2] = 0;
+	buf[3] = cpu_to_be32(address);
+	memcpy(&buf[4], data, sizeof(u64));
+
+	client = sbefifo_drv_open(sbefifo, 0);
+	if (!client)
+		return -ENODEV;
+
+	rc = sbefifo_drv_write(client, (const char *)buf, len);
+	if (rc < 0)
+		goto done;
+	else if (rc != len) {
+		rc = -EMSGSIZE;
+		goto done;
+	}
+
+	rc = 0;
+
+	/* ignore response */
+	sbefifo_drv_read(client, (char *)buf, len);
+
+done:
+	sbefifo_drv_release(client);
+	return rc;
+}
+
+static int occ_putscom_u32(struct device *sbefifo, u32 address, u32 data0,
+			   u32 data1)
+{
+	u8 buf[8];
+	u32 raw_data0 = cpu_to_be32(data0), raw_data1 = cpu_to_be32(data1);
+
+	memcpy(buf, &raw_data0, 4);
+	memcpy(buf + 4, &raw_data1, 4);
+
+	return occ_putscom(sbefifo, address, buf);
+}
+
+static void occ_worker(struct work_struct *work)
+{
+	int i, empty, canceled, waiting, rc;
+	u16 resp_data_length;
+	struct occ *occ = container_of(work, struct occ, work);
+	struct device *sbefifo = occ->sbefifo;
+	struct occ_client *client;
+	struct occ_xfr *xfr;
+	struct occ_response *resp;
+
+again:
+	spin_lock_irq(&occ->list_lock);
+	xfr = list_first_entry(&occ->xfrs, struct occ_xfr, link);
+	if (!xfr) {
+		spin_unlock(&occ->list_lock);
+		return;
+	}
+
+	set_bit(XFR_IN_PROGRESS, &xfr->flags);
+	spin_unlock(&occ->list_lock);
+
+	resp = (struct occ_response *)xfr->buf;
+
+	spin_lock_irq(&occ->occ_lock);
+
+	/* set address reg to occ sram command buffer */
+	rc = occ_putscom_u32(sbefifo, 0x6D050, 0xFFFBE000, 0);
+	if (rc)
+		goto done;
+
+	/* write cmd data */
+	for (i = 0; i < xfr->cmd_data_length; i += 8) {
+		rc = occ_putscom(sbefifo, 0x6D055, &xfr->buf[i]);
+		if (rc)
+			goto done;
+	}
+
+	/* trigger attention */
+	rc = occ_putscom_u32(sbefifo, 0x6D035, 0x20010000, 0);
+	if (rc)
+		goto done;
+
+	/* set address reg to occ sram response buffer */
+	rc = occ_putscom_u32(sbefifo, 0x6D050, 0xFFFBF000, 0);
+	if (rc)
+		goto done;
+
+	rc = occ_getscom(sbefifo, 0x6D055, xfr->buf);
+	if (rc)
+		goto done;
+
+	xfr->resp_data_length += 8;
+
+	resp_data_length = be16_to_cpu(get_unaligned(&resp->data_length));
+	if (resp_data_length > OCC_RESP_DATA_BYTES) {
+		rc = -EFAULT;
+		goto done;
+	}
+
+	/* already read 3 bytes of resp data, but also need 2 bytes chksum */
+	for (i = 8; i < resp_data_length + 7; i += 8) {
+		rc = occ_getscom(sbefifo, 0x6D055, &xfr->buf[i]);
+		if (rc)
+			goto done;
+
+		xfr->resp_data_length += 8;
+	}
+
+	/* no errors, got all data */
+	xfr->resp_data_length = resp_data_length + 7;
+
+done:
+	spin_unlock(&occ->occ_lock);
+
+	xfr->rc = rc;
+	client = xfr->client;
+
+	/* lock client to prevent race with read() */
+	spin_lock_irq(&client->lock);
+	set_bit(XFR_COMPLETE, &xfr->flags);
+	waiting = test_bit(XFR_WAITING, &xfr->flags);
+	spin_unlock(&client->lock);
+
+	spin_lock_irq(&occ->list_lock);
+	clear_bit(XFR_IN_PROGRESS, &xfr->flags);
+	list_del(&xfr->link);
+	empty = list_empty(&occ->xfrs);
+	canceled = test_bit(XFR_CANCELED, &xfr->flags);
+	spin_unlock(&occ->list_lock);
+
+	if (waiting)
+		wake_up_interruptible(&client->wait);
+	else if (canceled) {
+		kfree(xfr);
+		kfree(xfr->client);
+	}
+
+	if (!empty)
+		goto again;
+}
+
+static int occ_probe(struct platform_device *pdev)
+{
+	int rc;
+	u32 reg;
+	struct occ *occ;
+	struct device *dev = &pdev->dev;
+
+	dev_info(dev, "Found occ device\n");
+	occ = devm_kzalloc(dev, sizeof(*occ), GFP_KERNEL);
+	if (!occ)
+		return -ENOMEM;
+
+	occ->sbefifo = dev->parent;
+	INIT_LIST_HEAD(&occ->xfrs);
+	spin_lock_init(&occ->list_lock);
+	spin_lock_init(&occ->occ_lock);
+	INIT_WORK(&occ->work, occ_worker);
+
+	if (dev->of_node) {
+		rc = of_property_read_u32(dev->of_node, "reg", &reg);
+		if (!rc) {
+			/* make sure we don't have a duplicate from dts */
+			occ->idx = ida_simple_get(&occ_ida, reg, reg + 1,
+						  GFP_KERNEL);
+			if (occ->idx < 0)
+				occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
+							  GFP_KERNEL);
+		} else
+			occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
+						  GFP_KERNEL);
+	} else
+		occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX, GFP_KERNEL);
+
+	snprintf(occ->name, sizeof(occ->name), "occ%d", occ->idx);
+	occ->mdev.fops = &occ_fops;
+	occ->mdev.minor = MISC_DYNAMIC_MINOR;
+	occ->mdev.name = occ->name;
+	occ->mdev.parent = dev;
+
+	rc = misc_register(&occ->mdev);
+	if (rc) {
+		dev_err(dev, "failed to register miscdevice\n");
+		return rc;
+	}
+
+	platform_set_drvdata(pdev, occ);
+
+	return 0;
+}
+
+static int occ_remove(struct platform_device *pdev)
+{
+	struct occ_xfr *xfr, *tmp;
+	struct occ *occ = platform_get_drvdata(pdev);
+	struct occ_client *client;
+
+	misc_deregister(&occ->mdev);
+
+	spin_lock_irq(&occ->list_lock);
+	list_for_each_entry_safe(xfr, tmp, &occ->xfrs, link) {
+		client = xfr->client;
+		set_bit(XFR_CANCELED, &xfr->flags);
+
+		if (!test_bit(XFR_IN_PROGRESS, &xfr->flags)) {
+			list_del(&xfr->link);
+
+			spin_lock_irq(&client->lock);
+			if (test_bit(XFR_WAITING, &xfr->flags)) {
+				wake_up_interruptible(&client->wait);
+				spin_unlock(&client->lock);
+			} else {
+				kfree(xfr);
+				spin_unlock(&client->lock);
+				kfree(client);
+			}
+		}
+	}
+	spin_unlock(&occ->list_lock);
+
+	flush_work(&occ->work);
+
+	ida_simple_remove(&occ_ida, occ->idx);
+
+	return 0;
+}
+
+static const struct of_device_id occ_match[] = {
+	{ .compatible = "ibm,p9-occ" },
+	{ },
+};
+
+static struct platform_driver occ_driver = {
+	.driver = {
+		.name = "occ",
+		.of_match_table	= occ_match,
+	},
+	.probe	= occ_probe,
+	.remove = occ_remove,
+};
+
+static int occ_init(void)
+{
+	occ_wq = create_singlethread_workqueue("occ");
+	if (!occ_wq)
+		return -ENOMEM;
+
+	return platform_driver_register(&occ_driver);
+}
+
+static void occ_exit(void)
+{
+	destroy_workqueue(occ_wq);
+
+	platform_driver_unregister(&occ_driver);
+}
+
+module_init(occ_init);
+module_exit(occ_exit);
+
+MODULE_AUTHOR("Eddie James <eajames@us.ibm.com>");
+MODULE_DESCRIPTION("BMC P9 OCC driver");
+MODULE_LICENSE("GPL");
+
-- 
1.8.3.1

[PATCH linux dev-4.10 3/3] dts: aspeed: witherspoon: Add FSI devices

[Eddie James]

From: "Edward A. James" <eajames@us.ibm.com>

Signed-off-by: Edward A. James <eajames@us.ibm.com>
---
 arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 46 ++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
index f20aaf4..4138c93 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
@@ -65,6 +65,8 @@
 
 	gpio-fsi {
 		compatible = "fsi-master-gpio", "fsi-master";
+		#address-cells = <2>;
+		#size-cells = <0>;
 
 		status = "okay";
 
@@ -73,6 +75,50 @@
 		mux-gpios = <&gpio ASPEED_GPIO(A, 6) GPIO_ACTIVE_HIGH>;
 		enable-gpios = <&gpio ASPEED_GPIO(D, 0) GPIO_ACTIVE_HIGH>;
 		trans-gpios = <&gpio ASPEED_GPIO(R, 2) GPIO_ACTIVE_HIGH>;
+
+		cfam@0,0 {
+			reg = <0 0>;
+			#address-cells = <1>;
+			#size-cells = <1>;
+
+			sbefifo@2400 {
+				compatible = "ibm,p9-sbefifo";
+				reg = <0x2400 0x400>;
+				#address-cells = <1>;
+				#size-cells = <0>;
+
+				occ@1 {
+					compatible = "ibm,p9-occ";
+					reg = <1>;
+				};
+			};
+
+			hub@3400 {
+				compatible = "fsi-master-hub";
+				reg = <0x3400 0x400>;
+				#address-cells = <2>;
+				#size-cells = <0>;
+
+				cfam@1,0 {
+					reg = <1 0>;
+					#address-cells = <1>;
+					#size-cells = <1>;
+
+					sbefifo@2400 {
+						compatible = "ibm,p9-sbefifo";
+						reg = <0x2400 0x400>;
+						#address-cells = <1>;
+						#size-cells = <0>;
+
+						occ@2 {
+							compatible =
+								"ibm,p9-occ";
+							reg = <2>;
+						};
+					};
+				};
+			};
+		};
 	};
 
 	iio-hwmon {
-- 
1.8.3.1

Re: [PATCH linux dev-4.10 2/3] drivers: fsi: sbefifo: Add OCC driver

[Jeremy Kerr]

Hi Eddie,

> From: "Edward A. James" <eajames@us.ibm.com>
> 
> This driver provides an atomic communications channel between the OCC on
> the POWER9 processor and a service processor (a BMC). The driver is
> dependent on the FSI SBEIFO driver to get hardware access to the OCC
> SRAM.
> 
> The format of the communication is a command followed by a response.
> Since the command and response must be performed atomically, the driver
> will perform this operations asynchronously. In this way, a write
> operation starts the command, and a read will gather the response data
> once it is complete.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>

Overall, this looks pretty good. The userspace ABI seems straightforward
(which is the important part to get right early), which is great.

I do have a few comments though, provided inline. However, most of these
are fairly minor. We do need to sort out the DT representation though,
as that's harder to change post-merge.

> --- a/drivers/fsi/Kconfig
> +++ b/drivers/fsi/Kconfig
> @@ -36,6 +36,15 @@ config FSI_SBEFIFO
>  	---help---
>  	This option enables an FSI based SBEFIFO device driver.
>  
> +if FSI_SBEFIFO
> +
> +config OCC
> +	tristate "OCC SBEFIFO client device driver"
> +	---help---
> +	This option enables an SBEFIFO based OCC device driver.
> +
> +endif
> +
>  endif

I don't think we need the `if FSI_SBEFIFO` there, as you've already got
the `depends on FSI_SBEFIFO` (ie., there's no need to hide it from the
menu).

> +#define OCC_SRAM_BYTES		4096
> +#define OCC_CMD_DATA_BYTES	4090
> +#define OCC_RESP_DATA_BYTES	4089
> +
> +struct occ {
> +	struct device *sbefifo;
> +	char name[32];
> +	int idx;
> +	struct miscdevice mdev;
> +	struct list_head xfrs;
> +	spinlock_t list_lock;
> +	spinlock_t occ_lock;
> +	struct work_struct work;
> +};

Since you have two locks there, can you describe what each lock
protects?

Also, does occ_lock need to be a spin_lock? Looks like we're doing quite
a lot with that held - can we change to a mutex instead?

> +struct occ_xfr;
> +
> +enum {
> +	CLIENT_NONBLOCKING,
> +};
> +
> +struct occ_client {
> +	struct occ *occ;
> +	struct occ_xfr *xfr;
> +	spinlock_t lock;
> +	wait_queue_head_t wait;
> +	size_t read_offset;
> +	unsigned long flags;
> +};
> +
> +enum {
> +	XFR_IN_PROGRESS,
> +	XFR_COMPLETE,
> +	XFR_CANCELED,
> +	XFR_WAITING,
> +};

This looks like a set of states; but you're storing them in a bitmask
(so, you can have multiple states asserted at the same time). Is that
necessary, or are these mutually exclusive (which would be much simple
to follow)?

If we do need to support multiple states, could you add a comment here
indicating which combinations are valid, and the transitions between
states?

> +static ssize_t occ_read(struct file *file, char __user *buf, size_t len,
> +			loff_t *offset)
> +{
> +	int rc;
> +	size_t bytes;
> +	struct occ_xfr *xfr;
> +	struct occ_client *client = file->private_data;
> +
> +	if (!access_ok(VERIFY_WRITE, buf, len))
> +		return -EFAULT;

This is probably redundant, as you're (correctly) using copy_to_user
below.

> +
> +	if (len > OCC_SRAM_BYTES)
> +		return -EINVAL;
> +
> +	spin_lock_irq(&client->lock);
> +	if (!client->xfr) {
> +		/* we just finished reading all data, return 0 */
> +		if (client->read_offset) {
> +			rc = 0;
> +			client->read_offset = 0;
> +		} else
> +			rc = -ENOMSG;

Super minor nit: closing curly brace and 'else' go on the same line.

> +static ssize_t occ_write(struct file *file, const char __user *buf,
> +			 size_t len, loff_t *offset)
> +{
> +	int rc;
> +	struct occ_xfr *xfr;
> +	struct occ_client *client = file->private_data;
> +
> +	if (!access_ok(VERIFY_READ, buf, len))
> +		return -EFAULT;
> +
> +	if (len > OCC_SRAM_BYTES)
> +		return -EINVAL;
> +
> +	spin_lock_irq(&client->lock);
> +	if (client->xfr) {
> +		rc = -EDEADLK;
> +		goto done;
> +	}

I think -EBUSY would be better here.

In general, it looks like the code supports two separate processes
performing independent writes. In that case, you already have the
correct sequencing in the interface to the OCC, could we possibly
support multiple queued writes on the one client? Assuming reads()
return the responses in the order that they were written...

Or is there no point in doing that?

> +
> +	xfr = kzalloc(sizeof(*xfr), GFP_KERNEL);
> +	if (!xfr) {
> +		rc = -ENOMEM;
> +		goto done;
> +	}

Do we need to allocate xfr on every read/write? Could we just allocate
this as part of struct occfifo_client, and re-use that?

> +static int occ_getscom(struct device *sbefifo, u32 address, u8 *data)
> +{
> +	int rc;
> +	u32 buf[4];
> +	struct sbefifo_client *client;
> +	const size_t len = sizeof(buf);
> +
> +	buf[0] = cpu_to_be32(0x4);
> +	buf[1] = cpu_to_be32(0xa201);
> +	buf[2] = 0;
> +	buf[3] = cpu_to_be32(address);

No endianness concerns there? I haven't had a thorough look at the
sbefifo_drv_read/_write API, do they define an endianness for the
buffers?

> +
> +	client = sbefifo_drv_open(sbefifo, 0);
> +	if (!client)
> +		return -ENODEV;
> +
> +	rc = sbefifo_drv_write(client, (const char *)buf, len);
> +	if (rc < 0)
> +		goto done;
> +	else if (rc != len) {
> +		rc = -EMSGSIZE;
> +		goto done;
> +	}

Using a compound statements for only one (but not all) branches of an
conditional expression freaks me out a bit, mainly from the bugs that
have arisen from subsequent changes to the code.

Also, you're not passing on negative rc values to the caller?

> +static int occ_putscom_u32(struct device *sbefifo, u32 address, u32 data0,
> +			   u32 data1)

Does that make sense to split into two u32s, instead of a single u64?

I know a some of the existing scom interfaces split it up, but is there
a particular reason we need to carry that over to this code?

> +static int occ_probe(struct platform_device *pdev)
> +{
> +	int rc;
> +	u32 reg;
> +	struct occ *occ;
> +	struct device *dev = &pdev->dev;
> +
> +	dev_info(dev, "Found occ device\n");

This probably isn't necessary; the presence of the device itself is a
pretty good indication that we probed one (and you have a log on the
error case).

> +	occ = devm_kzalloc(dev, sizeof(*occ), GFP_KERNEL);
> +	if (!occ)
> +		return -ENOMEM;
> +
> +	occ->sbefifo = dev->parent;
> +	INIT_LIST_HEAD(&occ->xfrs);
> +	spin_lock_init(&occ->list_lock);
> +	spin_lock_init(&occ->occ_lock);
> +	INIT_WORK(&occ->work, occ_worker);
> +
> +	if (dev->of_node) {
> +		rc = of_property_read_u32(dev->of_node, "reg", &reg);
> +		if (!rc) {
> +			/* make sure we don't have a duplicate from dts */
> +			occ->idx = ida_simple_get(&occ_ida, reg, reg + 1,
> +						  GFP_KERNEL);
> +			if (occ->idx < 0)
> +				occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
> +							  GFP_KERNEL);
> +		} else
> +			occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
> +						  GFP_KERNEL);

We'll need a binding document for this. What is the reg property
representing, hardware-wise?

Traditionally, reg is the address of the device on the parent bus, and
so it'd totally find to have multiple devices in the system to share reg
values (provided they're on different busses). So, I'm not sure that
this is the usage that we want here; perhaps another property name would
be better...

> +static int occ_remove(struct platform_device *pdev)
> +{
> +	struct occ_xfr *xfr, *tmp;
> +	struct occ *occ = platform_get_drvdata(pdev);
> +	struct occ_client *client;
> +
> +	misc_deregister(&occ->mdev);
> +
> +	spin_lock_irq(&occ->list_lock);
> +	list_for_each_entry_safe(xfr, tmp, &occ->xfrs, link) {

Do you ever hit this code? I'd have assumed that ->remove is going to be
called once all open file descriptors have gone, and your ->release
should have emptied the transfer lists at that point...

Cheers,


Jeremy

Re: [PATCH linux dev-4.10 2/3] drivers: fsi: sbefifo: Add OCC driver

[Eddie James]

On 06/01/2017 01:55 AM, Jeremy Kerr wrote:
> Hi Eddie,
>
>> From: "Edward A. James" <eajames@us.ibm.com>
>>
>> This driver provides an atomic communications channel between the OCC on
>> the POWER9 processor and a service processor (a BMC). The driver is
>> dependent on the FSI SBEIFO driver to get hardware access to the OCC
>> SRAM.
>>
>> The format of the communication is a command followed by a response.
>> Since the command and response must be performed atomically, the driver
>> will perform this operations asynchronously. In this way, a write
>> operation starts the command, and a read will gather the response data
>> once it is complete.
>>
>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> Overall, this looks pretty good. The userspace ABI seems straightforward
> (which is the important part to get right early), which is great.
>
> I do have a few comments though, provided inline. However, most of these
> are fairly minor. We do need to sort out the DT representation though,
> as that's harder to change post-merge.

Hi, thanks for the review. I'm switching to a different way to access 
the OCC SRAM (SBE getSram/putSram calls), so some of this will be 
refactored.

>
>> --- a/drivers/fsi/Kconfig
>> +++ b/drivers/fsi/Kconfig
>> @@ -36,6 +36,15 @@ config FSI_SBEFIFO
>>   	---help---
>>   	This option enables an FSI based SBEFIFO device driver.
>>   
>> +if FSI_SBEFIFO
>> +
>> +config OCC
>> +	tristate "OCC SBEFIFO client device driver"
>> +	---help---
>> +	This option enables an SBEFIFO based OCC device driver.
>> +
>> +endif
>> +
>>   endif
> I don't think we need the `if FSI_SBEFIFO` there, as you've already got
> the `depends on FSI_SBEFIFO` (ie., there's no need to hide it from the
> menu).
>
>> +#define OCC_SRAM_BYTES		4096
>> +#define OCC_CMD_DATA_BYTES	4090
>> +#define OCC_RESP_DATA_BYTES	4089
>> +
>> +struct occ {
>> +	struct device *sbefifo;
>> +	char name[32];
>> +	int idx;
>> +	struct miscdevice mdev;
>> +	struct list_head xfrs;
>> +	spinlock_t list_lock;
>> +	spinlock_t occ_lock;
>> +	struct work_struct work;
>> +};
> Since you have two locks there, can you describe what each lock
> protects?
>
> Also, does occ_lock need to be a spin_lock? Looks like we're doing quite
> a lot with that held - can we change to a mutex instead?

Yes, we can probably use a mutex there. occ_lock controls access to the 
hardware, so clients don't step on each other. list_lock controls access 
to the list of xfrs when we're deleting and adding transfers. I'll add a 
comment.

>
>> +struct occ_xfr;
>> +
>> +enum {
>> +	CLIENT_NONBLOCKING,
>> +};
>> +
>> +struct occ_client {
>> +	struct occ *occ;
>> +	struct occ_xfr *xfr;
>> +	spinlock_t lock;
>> +	wait_queue_head_t wait;
>> +	size_t read_offset;
>> +	unsigned long flags;
>> +};
>> +
>> +enum {
>> +	XFR_IN_PROGRESS,
>> +	XFR_COMPLETE,
>> +	XFR_CANCELED,
>> +	XFR_WAITING,
>> +};
> This looks like a set of states; but you're storing them in a bitmask
> (so, you can have multiple states asserted at the same time). Is that
> necessary, or are these mutually exclusive (which would be much simple
> to follow)?
>
> If we do need to support multiple states, could you add a comment here
> indicating which combinations are valid, and the transitions between
> states?

XFR_WAITING can be combined with the others, and I'm planning another 
flag that will be in combination. I'll add a comment.

>
>> +static ssize_t occ_read(struct file *file, char __user *buf, size_t len,
>> +			loff_t *offset)
>> +{
>> +	int rc;
>> +	size_t bytes;
>> +	struct occ_xfr *xfr;
>> +	struct occ_client *client = file->private_data;
>> +
>> +	if (!access_ok(VERIFY_WRITE, buf, len))
>> +		return -EFAULT;
> This is probably redundant, as you're (correctly) using copy_to_user
> below.
>
>> +
>> +	if (len > OCC_SRAM_BYTES)
>> +		return -EINVAL;
>> +
>> +	spin_lock_irq(&client->lock);
>> +	if (!client->xfr) {
>> +		/* we just finished reading all data, return 0 */
>> +		if (client->read_offset) {
>> +			rc = 0;
>> +			client->read_offset = 0;
>> +		} else
>> +			rc = -ENOMSG;
> Super minor nit: closing curly brace and 'else' go on the same line.
>
>> +static ssize_t occ_write(struct file *file, const char __user *buf,
>> +			 size_t len, loff_t *offset)
>> +{
>> +	int rc;
>> +	struct occ_xfr *xfr;
>> +	struct occ_client *client = file->private_data;
>> +
>> +	if (!access_ok(VERIFY_READ, buf, len))
>> +		return -EFAULT;
>> +
>> +	if (len > OCC_SRAM_BYTES)
>> +		return -EINVAL;
>> +
>> +	spin_lock_irq(&client->lock);
>> +	if (client->xfr) {
>> +		rc = -EDEADLK;
>> +		goto done;
>> +	}
> I think -EBUSY would be better here.
>
> In general, it looks like the code supports two separate processes
> performing independent writes. In that case, you already have the
> correct sequencing in the interface to the OCC, could we possibly
> support multiple queued writes on the one client? Assuming reads()
> return the responses in the order that they were written...
>
> Or is there no point in doing that?

The idea was to allow multiple clients on multiple devices to do their 
thing and get their results in the right order. I could try and figure 
out multiple transfers pending on one client, but I couldn't quite see a 
use case for it. I'll think about it a bit more.

>
>> +
>> +	xfr = kzalloc(sizeof(*xfr), GFP_KERNEL);
>> +	if (!xfr) {
>> +		rc = -ENOMEM;
>> +		goto done;
>> +	}
> Do we need to allocate xfr on every read/write? Could we just allocate
> this as part of struct occfifo_client, and re-use that?

Yea if we stick with one xfr per client, that make more sense.

>
>> +static int occ_getscom(struct device *sbefifo, u32 address, u8 *data)
>> +{
>> +	int rc;
>> +	u32 buf[4];
>> +	struct sbefifo_client *client;
>> +	const size_t len = sizeof(buf);
>> +
>> +	buf[0] = cpu_to_be32(0x4);
>> +	buf[1] = cpu_to_be32(0xa201);
>> +	buf[2] = 0;
>> +	buf[3] = cpu_to_be32(address);
> No endianness concerns there? I haven't had a thorough look at the
> sbefifo_drv_read/_write API, do they define an endianness for the
> buffers?

sbefifo driver just passes the data through to the actual SBE, which 
expects data in big endian.

>
>> +
>> +	client = sbefifo_drv_open(sbefifo, 0);
>> +	if (!client)
>> +		return -ENODEV;
>> +
>> +	rc = sbefifo_drv_write(client, (const char *)buf, len);
>> +	if (rc < 0)
>> +		goto done;
>> +	else if (rc != len) {
>> +		rc = -EMSGSIZE;
>> +		goto done;
>> +	}
> Using a compound statements for only one (but not all) branches of an
> conditional expression freaks me out a bit, mainly from the bugs that
> have arisen from subsequent changes to the code.
>
> Also, you're not passing on negative rc values to the caller?
>
>> +static int occ_putscom_u32(struct device *sbefifo, u32 address, u32 data0,
>> +			   u32 data1)
> Does that make sense to split into two u32s, instead of a single u64?
>
> I know a some of the existing scom interfaces split it up, but is there
> a particular reason we need to carry that over to this code?

Mostly due to the endian swapping, SBE expects things in words for some 
reason, so we have to swap them in 32 bit chunks.

>
>> +static int occ_probe(struct platform_device *pdev)
>> +{
>> +	int rc;
>> +	u32 reg;
>> +	struct occ *occ;
>> +	struct device *dev = &pdev->dev;
>> +
>> +	dev_info(dev, "Found occ device\n");
> This probably isn't necessary; the presence of the device itself is a
> pretty good indication that we probed one (and you have a log on the
> error case).
>
>> +	occ = devm_kzalloc(dev, sizeof(*occ), GFP_KERNEL);
>> +	if (!occ)
>> +		return -ENOMEM;
>> +
>> +	occ->sbefifo = dev->parent;
>> +	INIT_LIST_HEAD(&occ->xfrs);
>> +	spin_lock_init(&occ->list_lock);
>> +	spin_lock_init(&occ->occ_lock);
>> +	INIT_WORK(&occ->work, occ_worker);
>> +
>> +	if (dev->of_node) {
>> +		rc = of_property_read_u32(dev->of_node, "reg", &reg);
>> +		if (!rc) {
>> +			/* make sure we don't have a duplicate from dts */
>> +			occ->idx = ida_simple_get(&occ_ida, reg, reg + 1,
>> +						  GFP_KERNEL);
>> +			if (occ->idx < 0)
>> +				occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
>> +							  GFP_KERNEL);
>> +		} else
>> +			occ->idx = ida_simple_get(&occ_ida, 1, INT_MAX,
>> +						  GFP_KERNEL);
> We'll need a binding document for this. What is the reg property
> representing, hardware-wise?
>
> Traditionally, reg is the address of the device on the parent bus, and
> so it'd totally find to have multiple devices in the system to share reg
> values (provided they're on different busses). So, I'm not sure that
> this is the usage that we want here; perhaps another property name would
> be better...

reg just represents the processor index. Maybe "proc"?

>
>> +static int occ_remove(struct platform_device *pdev)
>> +{
>> +	struct occ_xfr *xfr, *tmp;
>> +	struct occ *occ = platform_get_drvdata(pdev);
>> +	struct occ_client *client;
>> +
>> +	misc_deregister(&occ->mdev);
>> +
>> +	spin_lock_irq(&occ->list_lock);
>> +	list_for_each_entry_safe(xfr, tmp, &occ->xfrs, link) {
> Do you ever hit this code? I'd have assumed that ->remove is going to be
> called once all open file descriptors have gone, and your ->release
> should have emptied the transfer lists at that point...

Good to know, I don't think this ever gets used, I was just worried 
about FSI going away and removing everything mid-transfer.

Thanks,
Eddie

>
> Cheers,
>
>
> Jeremy
>

Re: [PATCH linux dev-4.10 3/3] dts: aspeed: witherspoon: Add FSI devices

[Joel Stanley]

On Sat, May 13, 2017 at 5:08 AM, Eddie James <eajames@linux.vnet.ibm.com> wrote:
> From: "Edward A. James" <eajames@us.ibm.com>
>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> ---
>  arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 46 ++++++++++++++++++++++++
>  1 file changed, 46 insertions(+)
>
> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
> index f20aaf4..4138c93 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts
> @@ -65,6 +65,8 @@
>
>         gpio-fsi {
>                 compatible = "fsi-master-gpio", "fsi-master";
> +               #address-cells = <2>;
> +               #size-cells = <0>;
>
>                 status = "okay";
>
> @@ -73,6 +75,50 @@
>                 mux-gpios = <&gpio ASPEED_GPIO(A, 6) GPIO_ACTIVE_HIGH>;
>                 enable-gpios = <&gpio ASPEED_GPIO(D, 0) GPIO_ACTIVE_HIGH>;
>                 trans-gpios = <&gpio ASPEED_GPIO(R, 2) GPIO_ACTIVE_HIGH>;
> +
> +               cfam@0,0 {
> +                       reg = <0 0>;
> +                       #address-cells = <1>;
> +                       #size-cells = <1>;

Since this snippet will be shared by all power9 machines, I suggest we
put it in a dtsi file that is included by them all.

I will resend with my proposal.

Cheers,

Joel

> +
> +                       sbefifo@2400 {
> +                               compatible = "ibm,p9-sbefifo";
> +                               reg = <0x2400 0x400>;
> +                               #address-cells = <1>;
> +                               #size-cells = <0>;
> +
> +                               occ@1 {
> +                                       compatible = "ibm,p9-occ";
> +                                       reg = <1>;
> +                               };
> +                       };
> +
> +                       hub@3400 {
> +                               compatible = "fsi-master-hub";
> +                               reg = <0x3400 0x400>;
> +                               #address-cells = <2>;
> +                               #size-cells = <0>;
> +
> +                               cfam@1,0 {
> +                                       reg = <1 0>;
> +                                       #address-cells = <1>;
> +                                       #size-cells = <1>;
> +
> +                                       sbefifo@2400 {
> +                                               compatible = "ibm,p9-sbefifo";
> +                                               reg = <0x2400 0x400>;
> +                                               #address-cells = <1>;
> +                                               #size-cells = <0>;
> +
> +                                               occ@2 {
> +                                                       compatible =
> +                                                               "ibm,p9-occ";
> +                                                       reg = <2>;
> +                                               };
> +                                       };
> +                               };
> +                       };
> +               };
>         };
>
>         iio-hwmon {
> --
> 1.8.3.1
>