[PATCH 00/16] FSI device driver introduction

[PATCH 00/16] FSI device driver introduction

[Chris Bostic]

From: Chris Bostic <cbostic@us.ibm.com>

Introduction of the IBM 'Flexible Support Interface' (FSI) bus device
driver. FSI is a high fan out serial bus consisting of a clock and a serial
data line capable of running at speeds up to 166 MHz.

This set provides the basic framework to add FSI extensions to the
Linux bus and device models. Master specific implementations are
defined to utilize the core FSI function.

In Linux, we have a core FSI "bus type", along with drivers for FSI
masters and engines.

The FSI master drivers expose a read/write interface to the bus address
space. The master drivers are under drivers/fsi/fsi-master-*.c.

The core handles probing and discovery of slaves and slave
engines, using those read/write interfaces. It is responsible for
creating the endpoint Linux devices corresponding to the discovered
engines on each slave.

Slave engines are identified by an 'engine' type, and an optional
version. Engine, a.k.a. client, drivers are matched and bound to these
engines during discovery.

This patch set does not include extended FSI function such as:
    *  Hub master support
    *  Cascaded master support
    *  Application layer hot plug notification
    *  Application layer FSI bus status interface

Common FSI terminology:

* Master
    Controller of the FSI bus.  Only the master is allowed to control the
    clock line and is the initiator of all transactions on a bus.

* Slave
    The receiver or target of a master initiated transaction.  The slave
    cannot initiate communications on a bus and must respond to any
    master requests for data.

* CFAM
    Stands for Common Field replaceable unit Access Macro.  A CFAM is an
    ASIC residing in any device requiring FSI communications. CFAMs
    consist of an array of hardware 'engines' used for various purposes.
    I2C masters, UARTs, General Purpose IO hardware are common types of
    these engines.

* Configuration Space / Table
    A table contained at the beginning of each CFAM address space.
    This table lists information such as the CFAM's ID, which engine types
    and versions it has available, as well as its addressing range.

* FSI Engine driver
    A device driver that registers with the FSI core so that it can access
    devices it owns on an FSI bus.


Chris Bostic (5):
  drivers/fsi: Set up links for slave communication
  drivers/fsi: Set slave SMODE to init communication
  drivers/fsi: Add master unscan
  drivers/fsi: Add documentation for GPIO bindings
  drivers/fsi: Add GPIO based FSI master

Jeremy Kerr (11):
  drivers/fsi: Add empty fsi bus definitions
  drivers/fsi: Add device & driver definitions
  drivers/fsi: add driver to device matches
  drivers/fsi: Add fsi master definition
  drivers/fsi: Add fake master driver
  drivers/fsi: Add slave definition
  drivers/fsi: Add empty master scan
  drivers/fsi: Add crc4 helpers
  drivers/fsi: Implement slave initialisation
  drivers/fsi: scan slaves & register devices
  drivers/fsi: Add device read/write/peek functions

 .../devicetree/bindings/fsi/fsi-master-gpio.txt    |  21 +
 drivers/Kconfig                                    |   2 +
 drivers/Makefile                                   |   1 +
 drivers/fsi/Kconfig                                |  29 ++
 drivers/fsi/Makefile                               |   4 +
 drivers/fsi/fsi-core.c                             | 514 +++++++++++++++++++
 drivers/fsi/fsi-master-fake.c                      |  95 ++++
 drivers/fsi/fsi-master-gpio.c                      | 552 +++++++++++++++++++++
 drivers/fsi/fsi-master.h                           |  62 +++
 include/linux/fsi.h                                |  60 +++
 10 files changed, 1340 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt
 create mode 100644 drivers/fsi/Kconfig
 create mode 100644 drivers/fsi/Makefile
 create mode 100644 drivers/fsi/fsi-core.c
 create mode 100644 drivers/fsi/fsi-master-fake.c
 create mode 100644 drivers/fsi/fsi-master-gpio.c
 create mode 100644 drivers/fsi/fsi-master.h
 create mode 100644 include/linux/fsi.h

-- 
1.8.2.2

[PATCH 01/16] drivers/fsi: Add empty fsi bus definitions

[Chris Bostic]

From: Jeremy Kerr <jk@ozlabs.org>

This change adds the initial (empty) fsi bus definition, and introduces
drivers/fsi/.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Chris Bostic <cbostic@us.ibm.com>
---
 drivers/Kconfig        |  2 ++
 drivers/Makefile       |  1 +
 drivers/fsi/Kconfig    | 12 ++++++++++++
 drivers/fsi/Makefile   |  2 ++
 drivers/fsi/fsi-core.c | 38 ++++++++++++++++++++++++++++++++++++++
 include/linux/fsi.h    | 22 ++++++++++++++++++++++
 6 files changed, 77 insertions(+)
 create mode 100644 drivers/fsi/Kconfig
 create mode 100644 drivers/fsi/Makefile
 create mode 100644 drivers/fsi/fsi-core.c
 create mode 100644 include/linux/fsi.h

diff --git a/drivers/Kconfig b/drivers/Kconfig
index e1e2066..117ca14c 100644
--- a/drivers/Kconfig
+++ b/drivers/Kconfig
@@ -202,4 +202,6 @@ source "drivers/hwtracing/intel_th/Kconfig"
 
 source "drivers/fpga/Kconfig"
 
+source "drivers/fsi/Kconfig"
+
 endmenu
diff --git a/drivers/Makefile b/drivers/Makefile
index f0afdfb..126e109 100644
--- a/drivers/Makefile
+++ b/drivers/Makefile
@@ -173,3 +173,4 @@ obj-$(CONFIG_STM)		+= hwtracing/stm/
 obj-$(CONFIG_ANDROID)		+= android/
 obj-$(CONFIG_NVMEM)		+= nvmem/
 obj-$(CONFIG_FPGA)		+= fpga/
+obj-$(CONFIG_FSI)		+= fsi/
diff --git a/drivers/fsi/Kconfig b/drivers/fsi/Kconfig
new file mode 100644
index 0000000..04c1a0e
--- /dev/null
+++ b/drivers/fsi/Kconfig
@@ -0,0 +1,12 @@
+#
+# FSI subsystem
+#
+
+menu "FSI support"
+
+config FSI
+	tristate "FSI support"
+	---help---
+	  FSI - the FRU Support Interface - is a simple bus for low-level
+	  access to POWER-based hardware.
+endmenu
diff --git a/drivers/fsi/Makefile b/drivers/fsi/Makefile
new file mode 100644
index 0000000..db0e5e7
--- /dev/null
+++ b/drivers/fsi/Makefile
@@ -0,0 +1,2 @@
+
+obj-$(CONFIG_FSI) += fsi-core.o
diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
new file mode 100644
index 0000000..3e45306
--- /dev/null
+++ b/drivers/fsi/fsi-core.c
@@ -0,0 +1,38 @@
+/*
+ * FSI core driver
+ *
+ * Copyright (C) IBM Corporation 2016
+ *
+ * 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
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/device.h>
+#include <linux/fsi.h>
+#include <linux/module.h>
+
+/* FSI core & Linux bus type definitions */
+
+struct bus_type fsi_bus_type = {
+	.name		= "fsi",
+};
+EXPORT_SYMBOL_GPL(fsi_bus_type);
+
+static int fsi_init(void)
+{
+	return bus_register(&fsi_bus_type);
+}
+
+static void fsi_exit(void)
+{
+	bus_unregister(&fsi_bus_type);
+}
+
+module_init(fsi_init);
+module_exit(fsi_exit);
diff --git a/include/linux/fsi.h b/include/linux/fsi.h
new file mode 100644
index 0000000..47aa181
--- /dev/null
+++ b/include/linux/fsi.h
@@ -0,0 +1,22 @@
+/* FSI device & driver interfaces
+ *
+ * Copyright (C) IBM Corporation 2016
+ *
+ * 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
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef LINUX_FSI_H
+#define LINUX_FSI_H
+
+#include <linux/device.h>
+
+extern struct bus_type fsi_bus_type;
+
+#endif /* LINUX_FSI_H */
-- 
1.8.2.2

[PATCH 02/16] drivers/fsi: Add device & driver definitions

[Chris Bostic]

From: Jeremy Kerr <jk@ozlabs.org>

Add structs for fsi devices & drivers, and struct device conversion
functions.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Chris Bostic <cbostic@us.ibm.com>
---
 include/linux/fsi.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/fsi.h b/include/linux/fsi.h
index 47aa181..f73886a 100644
--- a/include/linux/fsi.h
+++ b/include/linux/fsi.h
@@ -17,6 +17,17 @@
 
 #include <linux/device.h>
 
+struct fsi_device {
+	struct device dev;
+};
+
+struct fsi_driver {
+	struct device_driver drv;
+};
+
+#define to_fsi_dev(devp) container_of(devp, struct fsi_device, dev)
+#define to_fsi_drv(drvp) container_of(drvp, struct fsi_driver, drv)
+
 extern struct bus_type fsi_bus_type;
 
 #endif /* LINUX_FSI_H */
-- 
1.8.2.2

[PATCH 03/16] drivers/fsi: add driver to device matches

[Chris Bostic]

From: Jeremy Kerr <jk@ozlabs.org>

Driver bind to devices based on the engine types & (optional) versions.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Chris Bostic <cbostic@us.ibm.com>
---
 drivers/fsi/fsi-core.c | 21 +++++++++++++++++++++
 include/linux/fsi.h    | 21 +++++++++++++++++++--
 2 files changed, 40 insertions(+), 2 deletions(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 3e45306..3d55bd5 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -19,8 +19,29 @@
 
 /* FSI core & Linux bus type definitions */
 
+static int fsi_bus_match(struct device *dev, struct device_driver *drv)
+{
+	struct fsi_device *fsi_dev = to_fsi_dev(dev);
+	struct fsi_driver *fsi_drv = to_fsi_drv(drv);
+	const struct fsi_device_id *id;
+
+	if (!fsi_drv->id_table)
+		return 0;
+
+	for (id = fsi_drv->id_table; id->engine_type; id++) {
+		if (id->engine_type != fsi_dev->engine_type)
+			continue;
+		if (id->version == FSI_VERSION_ANY ||
+				id->version == fsi_dev->version)
+			return 1;
+	}
+
+	return 0;
+}
+
 struct bus_type fsi_bus_type = {
 	.name		= "fsi",
+	.match		= fsi_bus_match,
 };
 EXPORT_SYMBOL_GPL(fsi_bus_type);
 
diff --git a/include/linux/fsi.h b/include/linux/fsi.h
index f73886a..273cbf6 100644
--- a/include/linux/fsi.h
+++ b/include/linux/fsi.h
@@ -18,11 +18,28 @@
 #include <linux/device.h>
 
 struct fsi_device {
-	struct device dev;
+	struct device		dev;
+	u8			engine_type;
+	u8			version;
 };
 
+struct fsi_device_id {
+	u8	engine_type;
+	u8	version;
+};
+
+#define FSI_VERSION_ANY		0
+
+#define FSI_DEVICE(t) \
+	.engine_type = (t), .version = FSI_VERSION_ANY,
+
+#define FSI_DEVICE_VERSIONED(t, v) \
+	.engine_type = (t), .version = (v),
+
+
 struct fsi_driver {
-	struct device_driver drv;
+	struct device_driver		drv;
+	const struct fsi_device_id	*id_table;
 };
 
 #define to_fsi_dev(devp) container_of(devp, struct fsi_device, dev)
-- 
1.8.2.2

[PATCH 05/16] drivers/fsi: Add fake master driver

[Chris Bostic]

From: Jeremy Kerr <jk@ozlabs.org>

For debugging, add a fake master driver, that only supports reads,
returning a fixed set of data.

Includes changes from Chris Bostic <cbostic@us.ibm.com>.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Chris Bostic <cbostic@us.ibm.com>
---
 drivers/fsi/Kconfig           | 10 +++++
 drivers/fsi/Makefile          |  1 +
 drivers/fsi/fsi-master-fake.c | 95 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 106 insertions(+)
 create mode 100644 drivers/fsi/fsi-master-fake.c

diff --git a/drivers/fsi/Kconfig b/drivers/fsi/Kconfig
index 04c1a0e..f065dbe 100644
--- a/drivers/fsi/Kconfig
+++ b/drivers/fsi/Kconfig
@@ -9,4 +9,14 @@ config FSI
 	---help---
 	  FSI - the FRU Support Interface - is a simple bus for low-level
 	  access to POWER-based hardware.
+
+if FSI
+
+config FSI_MASTER_FAKE
+	tristate "Fake FSI master"
+	depends on FSI
+	---help---
+	This option enables a fake FSI master driver for debugging.
+endif
+
 endmenu
diff --git a/drivers/fsi/Makefile b/drivers/fsi/Makefile
index db0e5e7..847c00c 100644
--- a/drivers/fsi/Makefile
+++ b/drivers/fsi/Makefile
@@ -1,2 +1,3 @@
 
 obj-$(CONFIG_FSI) += fsi-core.o
+obj-$(CONFIG_FSI_MASTER_FAKE) += fsi-master-fake.o
diff --git a/drivers/fsi/fsi-master-fake.c b/drivers/fsi/fsi-master-fake.c
new file mode 100644
index 0000000..b42fe5b
--- /dev/null
+++ b/drivers/fsi/fsi-master-fake.c
@@ -0,0 +1,95 @@
+/*
+ * Fake FSI master driver for FSI development
+ *
+ * Copyright (C) IBM Corporation 2016
+ *
+ * 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
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/platform_device.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_platform.h>
+
+#include "fsi-master.h"
+
+static const uint8_t data[] = {
+	0xc0, 0x02, 0x08, 0x03,	/* chip id */
+	0x80, 0x01, 0x11, 0x00,	/* peek */
+	0x80, 0x01, 0x20, 0x3e,	/* slave */
+	0x00, 0x01, 0x10, 0xa5,	/* i2c */
+};
+
+
+static int fsi_master_fake_read(struct fsi_master *_master, int link,
+		uint8_t slave, uint32_t addr, void *val, size_t size)
+{
+	if (link != 0)
+		return -ENODEV;
+
+	if (addr + size > sizeof(data))
+		memset(val, 0, size);
+	else
+		memcpy(val, data + addr, size);
+
+	return 0;
+}
+
+static int fsi_master_fake_write(struct fsi_master *_master, int link,
+		uint8_t slave, uint32_t addr, const void *val, size_t size)
+{
+	if (link != 0)
+		return -ENODEV;
+
+	return -EACCES;
+}
+
+static int fsi_master_fake_probe(struct platform_device *pdev)
+{
+	struct fsi_master *master;
+
+	master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL);
+	if (!master)
+		return -ENOMEM;
+
+	master->dev = &pdev->dev;
+	master->n_links = 1;
+	master->read = fsi_master_fake_read;
+	master->write = fsi_master_fake_write;
+
+	return fsi_master_register(master);
+}
+
+static const struct of_device_id fsi_master_fake_match[] = {
+	{ .compatible = "ibm,fsi-master-fake" },
+	{ },
+};
+
+static struct platform_driver fsi_master_fake_driver = {
+	.driver = {
+		.name		= "fsi-master-fake",
+		.of_match_table	= fsi_master_fake_match,
+	},
+	.probe	= fsi_master_fake_probe,
+};
+
+static int __init fsi_master_fake_init(void)
+{
+	struct device_node *np;
+
+	platform_driver_register(&fsi_master_fake_driver);
+
+	for_each_compatible_node(np, NULL, "ibm,fsi-master-fake")
+		of_platform_device_create(np, NULL, NULL);
+
+	return 0;
+}
+
+module_init(fsi_master_fake_init);
-- 
1.8.2.2

[PATCH 06/16] drivers/fsi: Add slave definition

[Chris Bostic]

From: Jeremy Kerr <jk@ozlabs.org>

Add the initial fsi slave device, which is private to the core code.
This will be a child of the master, and parent to endpoint devices.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Chris Bostic <cbostic@us.ibm.com>
---
 drivers/fsi/fsi-core.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index ce9428d..60a6d91 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -21,6 +21,15 @@
 
 static atomic_t master_idx = ATOMIC_INIT(-1);
 
+struct fsi_slave {
+	struct device		dev;
+	struct fsi_master	*master;
+	int			link;
+	uint8_t			id;
+};
+
+#define to_fsi_slave(d) container_of(d, struct fsi_slave, dev)
+
 /* FSI master support */
 
 int fsi_master_register(struct fsi_master *master)
-- 
1.8.2.2

[PATCH 07/16] drivers/fsi: Add empty master scan

[Chris Bostic]

From: Jeremy Kerr <jk@ozlabs.org>

When a new fsi master is added, we will need to scan its links, and
slaves attached to those links. This change introduces a little shell to
iterate the links, which we will populate with the actual slave scan in
a later change.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Chris Bostic <cbostic@us.ibm.com>
---
 drivers/fsi/fsi-core.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 60a6d91..ceaf536 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -19,6 +19,8 @@
 
 #include "fsi-master.h"
 
+#define FSI_N_SLAVES	4
+
 static atomic_t master_idx = ATOMIC_INIT(-1);
 
 struct fsi_slave {
@@ -30,12 +32,34 @@ struct fsi_slave {
 
 #define to_fsi_slave(d) container_of(d, struct fsi_slave, dev)
 
+/* FSI slave support */
+static int fsi_slave_init(struct fsi_master *master,
+		int link, uint8_t slave_id)
+{
+	/* todo: initialise slave device, perform engine scan */
+
+	return -ENODEV;
+}
+
 /* FSI master support */
 
+static int fsi_master_scan(struct fsi_master *master)
+{
+	int link, slave_id;
+
+	for (link = 0; link < master->n_links; link++)
+		for (slave_id = 0; slave_id < FSI_N_SLAVES; slave_id++)
+			fsi_slave_init(master, link, slave_id);
+
+	return 0;
+
+}
+
 int fsi_master_register(struct fsi_master *master)
 {
 	master->idx = atomic_inc_return(&master_idx);
 	get_device(master->dev);
+	fsi_master_scan(master);
 	return 0;
 }
 EXPORT_SYMBOL_GPL(fsi_master_register);
-- 
1.8.2.2

[PATCH 10/16] drivers/fsi: scan slaves & register devices

[Chris Bostic]

From: Jeremy Kerr <jk@ozlabs.org>

Now that we have fsi_slave devices, scan each for endpoints, and
register them on the fsi bus.

Includes contributions from Chris Bostic <cbostic@us.ibm.com>

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Chris Bostic <cbostic@us.ibm.com>
---
 drivers/fsi/fsi-core.c | 136 +++++++++++++++++++++++++++++++++++++++++++++++--
 include/linux/fsi.h    |   4 ++
 2 files changed, 136 insertions(+), 4 deletions(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index aa4330a..b51ea35 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -21,9 +21,19 @@
 #include "fsi-master.h"
 
 #define FSI_N_SLAVES	4
-#define FSI_SLAVE_CONF_CRC_SHIFT        4
-#define FSI_SLAVE_CONF_CRC_MASK         0x0000000f
-#define FSI_SLAVE_CONF_DATA_BITS        28
+
+#define FSI_SLAVE_CONF_NEXT_MASK	0x80000000
+#define FSI_SLAVE_CONF_SLOTS_MASK	0x00ff0000
+#define FSI_SLAVE_CONF_SLOTS_SHIFT	16
+#define FSI_SLAVE_CONF_VERSION_MASK	0x0000f000
+#define FSI_SLAVE_CONF_VERSION_SHIFT	12
+#define FSI_SLAVE_CONF_TYPE_MASK	0x00000ff0
+#define FSI_SLAVE_CONF_TYPE_SHIFT	4
+#define FSI_SLAVE_CONF_CRC_SHIFT	4
+#define FSI_SLAVE_CONF_CRC_MASK		0x0000000f
+#define FSI_SLAVE_CONF_DATA_BITS	28
+
+static const int engine_page_size = 0x400;
 
 static atomic_t master_idx = ATOMIC_INIT(-1);
 
@@ -36,6 +46,30 @@ struct fsi_slave {
 
 #define to_fsi_slave(d) container_of(d, struct fsi_slave, dev)
 
+/* FSI endpoint-device support */
+
+static void fsi_device_release(struct device *_device)
+{
+	struct fsi_device *device = to_fsi_dev(_device);
+
+	kfree(device);
+}
+
+static struct fsi_device *fsi_create_device(struct fsi_slave *slave)
+{
+	struct fsi_device *dev;
+
+	dev = kzalloc(sizeof(*dev), GFP_KERNEL);
+	if (!dev)
+		return NULL;
+
+	dev->dev.parent = &slave->dev;
+	dev->dev.bus = &fsi_bus_type;
+	dev->dev.release = fsi_device_release;
+
+	return dev;
+}
+
 /* crc helpers */
 static const uint8_t crc4_tab[] = {
 	0x0, 0x7, 0xe, 0x9, 0xb, 0xc, 0x5, 0x2,
@@ -59,6 +93,99 @@ uint8_t fsi_crc4(uint8_t c, uint64_t x, int bits)
 
 /* FSI slave support */
 
+static int fsi_slave_read(struct fsi_slave *slave, uint32_t addr,
+			void *val, size_t size)
+{
+	return slave->master->read(slave->master, slave->link,
+			slave->id, addr, val, size);
+}
+
+static int fsi_slave_scan(struct fsi_slave *slave)
+{
+	uint32_t engine_addr;
+	uint32_t conf;
+	int rc, i;
+
+	/*
+	 * scan engines
+	 *
+	 * We keep the peek mode and slave engines for the core; so start
+	 * at the third slot in the configuration table. We also need to
+	 * skip the chip ID entry at the start of the address space.
+	 */
+	engine_addr = engine_page_size * 3;
+	for (i = 2; i < engine_page_size / sizeof(uint32_t); i++) {
+		uint8_t slots, version, type, crc;
+		struct fsi_device *dev;
+
+		rc = fsi_slave_read(slave, (i + 1) * sizeof(conf),
+				&conf, sizeof(conf));
+		if (rc) {
+			dev_warn(&slave->dev,
+				"error reading slave registers\n");
+			return -1;
+		}
+
+		crc = fsi_crc4(0, conf >> FSI_SLAVE_CONF_CRC_SHIFT,
+				FSI_SLAVE_CONF_DATA_BITS);
+		if (crc != (conf & FSI_SLAVE_CONF_CRC_MASK)) {
+			dev_warn(&slave->dev,
+				"crc error in slave register at 0x%04x\n",
+				i);
+			return -1;
+		}
+
+		slots = (conf & FSI_SLAVE_CONF_SLOTS_MASK)
+			>> FSI_SLAVE_CONF_SLOTS_SHIFT;
+		version = (conf & FSI_SLAVE_CONF_VERSION_MASK)
+			>> FSI_SLAVE_CONF_VERSION_SHIFT;
+		type = (conf & FSI_SLAVE_CONF_TYPE_MASK)
+			>> FSI_SLAVE_CONF_TYPE_SHIFT;
+
+		/*
+		 * Unused address areas are marked by a zero type value; this
+		 * skips the defined address areas
+		 */
+		if (type != 0) {
+
+			/* create device */
+			dev = fsi_create_device(slave);
+			if (!dev)
+				return -ENOMEM;
+
+			dev->slave = slave;
+			dev->engine_type = type;
+			dev->version = version;
+			dev->unit = i;
+			dev->addr = engine_addr;
+			dev->size = slots * engine_page_size;
+
+			dev_info(&slave->dev,
+			"engine[%i]: type %x, version %x, addr %x size %x\n",
+					dev->unit, dev->engine_type, version,
+					dev->addr, dev->size);
+
+			device_initialize(&dev->dev);
+			dev_set_name(&dev->dev, "%02x:%02x:%02x:%02x",
+					slave->master->idx, slave->link,
+					slave->id, i - 2);
+
+			rc = device_add(&dev->dev);
+			if (rc) {
+				dev_warn(&slave->dev, "add failed: %d\n", rc);
+				put_device(&dev->dev);
+			}
+		}
+
+		engine_addr += slots * engine_page_size;
+
+		if (!(conf & FSI_SLAVE_CONF_NEXT_MASK))
+			break;
+	}
+
+	return 0;
+}
+
 static void fsi_slave_release(struct device *dev)
 {
 	struct fsi_slave *slave = to_fsi_slave(dev);
@@ -110,7 +237,8 @@ static int fsi_slave_init(struct fsi_master *master,
 		return rc;
 	}
 
-	return rc;
+	fsi_slave_scan(slave);
+	return 0;
 }
 
 /* FSI master support */
diff --git a/include/linux/fsi.h b/include/linux/fsi.h
index 273cbf6..efa55ba 100644
--- a/include/linux/fsi.h
+++ b/include/linux/fsi.h
@@ -21,6 +21,10 @@ struct fsi_device {
 	struct device		dev;
 	u8			engine_type;
 	u8			version;
+	u8			unit;
+	struct fsi_slave	*slave;
+	uint32_t		addr;
+	uint32_t		size;
 };
 
 struct fsi_device_id {
-- 
1.8.2.2

[PATCH 13/16] drivers/fsi: Set slave SMODE to init communication

[Chris Bostic]

From: Chris Bostic <cbostic@us.ibm.com>

Set CFAM to appropriate ID so that the controlling master
can manage link memory ranges.  Add slave engine register
definitions.

Signed-off-by: Chris Bostic <cbostic@us.ibm.com>
---
 drivers/fsi/fsi-core.c | 90 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 89 insertions(+), 1 deletion(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 93de0f1..a28434b 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -34,6 +34,7 @@
 #define FSI_SLAVE_CONF_DATA_BITS	28
 
 #define FSI_PEEK_BASE			0x410
+#define	FSI_SLAVE_BASE			0x800
 
 static const int engine_page_size = 0x400;
 
@@ -53,8 +54,26 @@ static int fsi_slave_read(struct fsi_slave *slave, uint32_t addr,
 static int fsi_slave_write(struct fsi_slave *slave, uint32_t addr,
 		const void *val, size_t size);
 
-/* FSI endpoint-device support */
+/*
+ * FSI slave engine control register offsets
+ */
+#define	FSI_SMODE		0x0	/* R/W: Mode register */
+
+/*
+ * SMODE fields
+ */
+#define	FSI_SMODE_WSC		0x80000000	/* Warm start done */
+#define	FSI_SMODE_ECRC		0x20000000	/* Hw CRC check */
+#define	FSI_SMODE_SID_SHIFT	24		/* ID shift */
+#define	FSI_SMODE_SID_MASK	3		/* ID Mask */
+#define	FSI_SMODE_ED_SHIFT	20		/* Echo delay shift */
+#define	FSI_SMODE_ED_MASK	0xf		/* Echo delay mask */
+#define	FSI_SMODE_SD_SHIFT	16		/* Send delay shift */
+#define	FSI_SMODE_SD_MASK	0xf		/* Send delay mask */
+#define	FSI_SMODE_LBCRR_SHIFT	8		/* Clk ratio shift */
+#define	FSI_SMODE_LBCRR_MASK	0xf		/* Clk ratio mask */
 
+/* FSI endpoint-device support */
 int fsi_device_read(struct fsi_device *dev, uint32_t addr, void *val,
 		size_t size)
 {
@@ -133,6 +152,30 @@ uint8_t fsi_crc4(uint8_t c, uint64_t x, int bits)
 
 /* FSI slave support */
 
+/* Encode slave local bus echo delay */
+static inline uint32_t fsi_smode_echodly(int x)
+{
+	return (x & FSI_SMODE_ED_MASK) << FSI_SMODE_ED_SHIFT;
+}
+
+/* Encode slave local bus send delay */
+static inline uint32_t fsi_smode_senddly(int x)
+{
+	return (x & FSI_SMODE_SD_MASK) << FSI_SMODE_SD_SHIFT;
+}
+
+/* Encode slave local bus clock rate ratio */
+static inline uint32_t fsi_smode_lbcrr(int x)
+{
+	return (x & FSI_SMODE_LBCRR_MASK) << FSI_SMODE_LBCRR_SHIFT;
+}
+
+/* Encode slave ID */
+static inline uint32_t fsi_smode_sid(int x)
+{
+	return (x & FSI_SMODE_SID_MASK) << FSI_SMODE_SID_SHIFT;
+}
+
 static int fsi_slave_read(struct fsi_slave *slave, uint32_t addr,
 			void *val, size_t size)
 {
@@ -240,6 +283,22 @@ static void fsi_slave_release(struct device *dev)
 	kfree(slave);
 }
 
+static uint32_t set_smode_defaults(struct fsi_master *master)
+{
+	return FSI_SMODE_WSC | FSI_SMODE_ECRC
+		| fsi_smode_echodly(0xf) | fsi_smode_senddly(0xf)
+		| fsi_smode_lbcrr(1);
+}
+
+static int fsi_slave_set_smode(struct fsi_master *master, int link, int id)
+{
+	uint32_t smode = set_smode_defaults(master);
+
+	smode |= fsi_smode_sid(id);
+	return master->write(master, link, 3, FSI_SLAVE_BASE + FSI_SMODE,
+				&smode, sizeof(smode));
+}
+
 static int fsi_slave_init(struct fsi_master *master,
 		int link, uint8_t slave_id)
 {
@@ -248,6 +307,21 @@ static int fsi_slave_init(struct fsi_master *master,
 	int rc;
 	uint8_t crc;
 
+	/*
+	 * todo: Due to CFAM hardware issues related to BREAK commands we're
+	 * limited to only one CFAM per link.  Once issues are resolved this
+	 * restriction can be removed.
+	 */
+	if (slave_id > 0)
+		return 0;
+
+	rc = fsi_slave_set_smode(master, link, slave_id);
+	if (rc) {
+		dev_warn(master->dev, "can't set smode on slave:%02x:%02x %d\n",
+				link, slave_id, rc);
+		return -ENODEV;
+	}
+
 	rc = master->read(master, link, slave_id, 0, &chip_id, sizeof(chip_id));
 	if (rc) {
 		dev_warn(master->dev, "can't read slave %02x:%02x: %d\n",
@@ -312,6 +386,7 @@ static int fsi_master_break(struct fsi_master *master, int link)
 static int fsi_master_scan(struct fsi_master *master)
 {
 	int link, slave_id, rc;
+	uint32_t smode;
 
 	for (link = 0; link < master->n_links; link++) {
 		rc = fsi_master_link_enable(master, link);
@@ -327,6 +402,19 @@ static int fsi_master_scan(struct fsi_master *master)
 			continue;
 		}
 
+		/*
+		 * Verify can read slave at default ID location. If fails
+		 * there must be nothing on other end of link
+		 */
+		rc = master->read(master, link, 3, FSI_SLAVE_BASE + FSI_SMODE,
+				&smode, sizeof(smode));
+		if (rc) {
+			dev_dbg(master->dev,
+				"Read link:%d smode default id failed:%d\n",
+				link, rc);
+			continue;
+		}
+
 		for (slave_id = 0; slave_id < FSI_N_SLAVES; slave_id++)
 			fsi_slave_init(master, link, slave_id);
 
-- 
1.8.2.2

[PATCH 14/16] drivers/fsi: Add master unscan

[Chris Bostic]

From: Chris Bostic <cbostic@us.ibm.com>

Allow a master to undo a previous scan.  Should a master scan a bus
twice it will need to ensure it doesn't double register any
previously detected device.

Signed-off-by: Chris Bostic <cbostic@us.ibm.com>
---
 drivers/fsi/fsi-core.c   | 36 +++++++++++++++++++++++++++++++++++-
 drivers/fsi/fsi-master.h |  2 ++
 include/linux/fsi.h      |  1 +
 3 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index a28434b..8ccfe50 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -41,6 +41,8 @@
 static atomic_t master_idx = ATOMIC_INIT(-1);
 
 struct fsi_slave {
+	struct list_head	list_link;	/* Master's list of slaves */
+	struct list_head	my_engines;
 	struct device		dev;
 	struct fsi_master	*master;
 	int			link;
@@ -196,6 +198,8 @@ static int fsi_slave_scan(struct fsi_slave *slave)
 	uint32_t conf;
 	int rc, i;
 
+	INIT_LIST_HEAD(&slave->my_engines);
+
 	/*
 	 * scan engines
 	 *
@@ -264,7 +268,9 @@ static int fsi_slave_scan(struct fsi_slave *slave)
 			if (rc) {
 				dev_warn(&slave->dev, "add failed: %d\n", rc);
 				put_device(&dev->dev);
+				continue;
 			}
+			list_add(&dev->link, &slave->my_engines);
 		}
 
 		engine_addr += slots * engine_page_size;
@@ -357,7 +363,7 @@ static int fsi_slave_init(struct fsi_master *master,
 		put_device(&slave->dev);
 		return rc;
 	}
-
+	list_add(&slave->list_link, &master->my_slaves);
 	fsi_slave_scan(slave);
 	return 0;
 }
@@ -388,6 +394,11 @@ static int fsi_master_scan(struct fsi_master *master)
 	int link, slave_id, rc;
 	uint32_t smode;
 
+	if (!master->slave_list) {
+		INIT_LIST_HEAD(&master->my_slaves);
+		master->slave_list = true;
+	}
+
 	for (link = 0; link < master->n_links; link++) {
 		rc = fsi_master_link_enable(master, link);
 		if (rc) {
@@ -423,9 +434,31 @@ static int fsi_master_scan(struct fsi_master *master)
 	return 0;
 }
 
+static void fsi_master_unscan(struct fsi_master *master)
+{
+	struct fsi_slave *slave, *slave_tmp;
+	struct fsi_device *fsi_dev, *fsi_dev_tmp;
+
+	if (!master->slave_list)
+		return;
+
+	list_for_each_entry_safe(slave, slave_tmp, &master->my_slaves,
+							list_link) {
+		list_del(&slave->list_link);
+		list_for_each_entry_safe(fsi_dev, fsi_dev_tmp,
+					&slave->my_engines, link) {
+			list_del(&fsi_dev->link);
+			put_device(&fsi_dev->dev);
+		}
+		device_unregister(&slave->dev);
+	}
+	master->slave_list = false;
+}
+
 int fsi_master_register(struct fsi_master *master)
 {
 	master->idx = atomic_inc_return(&master_idx);
+	master->slave_list = false;
 	get_device(master->dev);
 	fsi_master_scan(master);
 	return 0;
@@ -434,6 +467,7 @@ int fsi_master_register(struct fsi_master *master)
 
 void fsi_master_unregister(struct fsi_master *master)
 {
+	fsi_master_unscan(master);
 	put_device(master->dev);
 }
 EXPORT_SYMBOL_GPL(fsi_master_unregister);
diff --git a/drivers/fsi/fsi-master.h b/drivers/fsi/fsi-master.h
index 56aad0e..454af2b 100644
--- a/drivers/fsi/fsi-master.h
+++ b/drivers/fsi/fsi-master.h
@@ -20,6 +20,8 @@
 #include <linux/device.h>
 
 struct fsi_master {
+	struct list_head my_slaves;
+	bool		slave_list;
 	struct device	*dev;
 	int		idx;
 	int		n_links;
diff --git a/include/linux/fsi.h b/include/linux/fsi.h
index 66bce48..924502b 100644
--- a/include/linux/fsi.h
+++ b/include/linux/fsi.h
@@ -18,6 +18,7 @@
 #include <linux/device.h>
 
 struct fsi_device {
+	struct list_head	link;	/* for slave's list */
 	struct device		dev;
 	u8			engine_type;
 	u8			version;
-- 
1.8.2.2

[PATCH 15/16] drivers/fsi: Add documentation for GPIO bindings

[Chris Bostic]

From: Chris Bostic <cbostic@us.ibm.com>

Add fsi master gpio device tree binding documentation

Signed-off-by: Chris Bostic <cbostic@us.ibm.com>
---
 .../devicetree/bindings/fsi/fsi-master-gpio.txt     | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt

diff --git a/Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt b/Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt
new file mode 100644
index 0000000..ff3a62e
--- /dev/null
+++ b/Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt
@@ -0,0 +1,21 @@
+Device-tree bindings for gpio-based FSI master driver
+-----------------------------------------------------
+
+Required properties:
+	- compatible = "ibm,fsi-master-gpio";
+	- clk-gpios;
+	- data-gpios;
+
+Optional properties:
+	- enable-gpios;
+	- trans-gpios;
+	- mux-gpios;
+
+fsi-master {
+	compatible = "ibm,fsi-master", "ibm,fsi-master-gpio";
+	clk-gpios = <&gpio 0 &gpio 6>;
+	data-gpios = <&gpio 1 &gpio 7>;
+	enable-gpios = <&gpio 2 &gpio 8>;	/* Enable FSI data in/out */
+	trans-gpios = <&gpio 3 &gpio 9>;	/* Volts translator direction */
+	mux-gpios = <&gpio 4> &gpio 10>;	/* Multiplexer for FSI pins */
+}
-- 
1.8.2.2

[PATCH 16/16] drivers/fsi: Add GPIO based FSI master

[Chris Bostic]

From: Chris Bostic <cbostic@us.ibm.com>

Implement a FSI master using GPIO.  Will generate FSI protocol for
read and write commands to particular addresses.  Sends master command
and waits for and decodes a slave response.

Includes Jeremy Kerr's original GPIO master base commit.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Chris Bostic <cbostic@us.ibm.com>
---
 drivers/fsi/Kconfig           |   7 +
 drivers/fsi/Makefile          |   1 +
 drivers/fsi/fsi-master-gpio.c | 552 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 560 insertions(+)
 create mode 100644 drivers/fsi/fsi-master-gpio.c

diff --git a/drivers/fsi/Kconfig b/drivers/fsi/Kconfig
index f065dbe..9530459 100644
--- a/drivers/fsi/Kconfig
+++ b/drivers/fsi/Kconfig
@@ -17,6 +17,13 @@ config FSI_MASTER_FAKE
 	depends on FSI
 	---help---
 	This option enables a fake FSI master driver for debugging.
+
+config FSI_MASTER_GPIO
+	tristate "GPIO-based FSI master"
+	depends on FSI && GPIOLIB
+	---help---
+	This option enables a FSI master driver using GPIO lines.
+
 endif
 
 endmenu
diff --git a/drivers/fsi/Makefile b/drivers/fsi/Makefile
index 847c00c..2021ce5 100644
--- a/drivers/fsi/Makefile
+++ b/drivers/fsi/Makefile
@@ -1,3 +1,4 @@
 
 obj-$(CONFIG_FSI) += fsi-core.o
 obj-$(CONFIG_FSI_MASTER_FAKE) += fsi-master-fake.o
+obj-$(CONFIG_FSI_MASTER_GPIO) += fsi-master-gpio.o
diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
new file mode 100644
index 0000000..79cb0b1
--- /dev/null
+++ b/drivers/fsi/fsi-master-gpio.c
@@ -0,0 +1,552 @@
+/*
+ * A FSI master controller, using a simple GPIO bit-banging interface
+ */
+
+#include <linux/platform_device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/fsi.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/spinlock.h>
+
+#include "fsi-master.h"
+
+#define	FSI_GPIO_STD_DLY	1	/* Standard pin delay in nS */
+#define	FSI_ECHO_DELAY_CLOCKS	16	/* Number clocks for echo delay */
+#define	FSI_PRE_BREAK_CLOCKS	50	/* Number clocks to prep for break */
+#define	FSI_BREAK_CLOCKS	256	/* Number of clocks to issue break */
+#define	FSI_POST_BREAK_CLOCKS	16000	/* Number clocks to set up cfam */
+#define	FSI_INIT_CLOCKS		5000	/* Clock out any old data */
+#define	FSI_GPIO_STD_DELAY	10	/* Standard GPIO delay in nS */
+					/* todo: adjust down as low as */
+					/* possible or eliminate */
+#define	FSI_GPIO_CMD_DPOLL	0x000000000000002AULL
+#define	FSI_GPIO_CMD_DPOLL_SIZE	9
+#define	FSI_GPIO_DPOLL_CLOCKS	100      /* < 21 will cause slave to hang */
+#define	FSI_GPIO_CMD_DEFAULT	0x2000000000000000ULL
+#define	FSI_GPIO_CMD_WRITE	0
+#define	FSI_GPIO_CMD_READ	0x0400000000000000ULL
+#define	FSI_GPIO_CMD_SLAVE_MASK	0xC000000000000000ULL
+#define	FSI_GPIO_CMD_ADDR_SHIFT	37
+#define	FSI_GPIO_CMD_ADDR_MASK	0x001FFFFF
+#define	FSI_GPIO_CMD_SLV_SHIFT	62
+#define	FSI_GPIO_CMD_SIZE_16	0x0000001000000000ULL
+#define	FSI_GPIO_CMD_SIZE_32	0x0000003000000000ULL
+#define	FSI_GPIO_CMD_DT32_SHIFT	4
+#define	FSI_GPIO_CMD_DT16_SHIFT	20
+#define	FSI_GPIO_CMD_DT8_SHIFT	28
+#define	FSI_GPIO_CMD_DFLT_LEN	28
+#define	FSI_GPIO_CMD_CRC_SHIFT	60
+
+/* Bus errors */
+#define	FSI_GPIO_ERR_BUSY	1	/* Slave stuck in busy state */
+#define	FSI_GPIO_RESP_ERRA	2	/* Any (misc) Error */
+#define	FSI_GPIO_RESP_ERRC	3	/* Slave reports master CRC error */
+#define	FSI_GPIO_MTOE		4	/* Master time out error */
+#define	FSI_GPIO_CRC_INVAL	5	/* Master reports slave CRC error */
+
+/* Normal slave responses */
+#define	FSI_GPIO_RESP_BUSY	1
+#define	FSI_GPIO_RESP_ACK	0
+#define	FSI_GPIO_RESP_ACKD	4
+
+#define	FSI_GPIO_MAX_BUSY	100
+#define	FSI_GPIO_MTOE_COUNT	1000
+#define	FSI_GPIO_DRAIN_BITS	20
+#define	FSI_GPIO_CRC_SIZE	4
+#define	FSI_GPIO_MSG_ID_SIZE		2
+#define	FSI_GPIO_MSG_RESPID_SIZE	2
+#define	FSI_GPIO_PRIME_SLAVE_CLOCKS	100
+
+static DEFINE_SPINLOCK(fsi_gpio_cmd_lock);	/* lock around fsi commands */
+
+struct fsi_master_gpio {
+	struct fsi_master	master;
+	struct gpio_desc	*gpio_clk;
+	struct gpio_desc	*gpio_data;
+	struct gpio_desc	*gpio_trans;	/* Voltage translator */
+	struct gpio_desc	*gpio_enable;	/* FSI enable */
+	struct gpio_desc	*gpio_mux;	/* Mux control */
+};
+
+#define to_fsi_master_gpio(m) container_of(m, struct fsi_master_gpio, master)
+
+struct fsi_gpio_msg {
+	uint64_t	msg;
+	uint8_t		bits;
+};
+
+static void clock_toggle(struct fsi_master_gpio *master, int count)
+{
+	int i;
+
+	for (i = 0; i < count; i++) {
+		ndelay(FSI_GPIO_STD_DLY);
+		gpiod_set_value(master->gpio_clk, 0);
+		ndelay(FSI_GPIO_STD_DLY);
+		gpiod_set_value(master->gpio_clk, 1);
+	}
+}
+
+static int sda_in(struct fsi_master_gpio *master)
+{
+	int in;
+
+	ndelay(FSI_GPIO_STD_DLY);
+	in = gpiod_get_value(master->gpio_data);
+	return in ? 1 : 0;
+}
+
+static void sda_out(struct fsi_master_gpio *master, int value)
+{
+	gpiod_set_value(master->gpio_data, value);
+}
+
+static void set_sda_input(struct fsi_master_gpio *master)
+{
+	gpiod_direction_input(master->gpio_data);
+	if (master->gpio_trans)
+		gpiod_set_value(master->gpio_trans, 0);
+}
+
+static void set_sda_output(struct fsi_master_gpio *master, int value)
+{
+	if (master->gpio_trans)
+		gpiod_set_value(master->gpio_trans, 1);
+	gpiod_direction_output(master->gpio_data, value);
+}
+
+static void serial_in(struct fsi_master_gpio *master, struct fsi_gpio_msg *cmd,
+			uint8_t num_bits)
+{
+	uint8_t bit;
+	uint64_t msg = 0;
+	uint8_t in_bit = 0;
+
+	set_sda_input(master);
+
+	for (bit = 0; bit < num_bits; bit++) {
+		clock_toggle(master, 1);
+		in_bit = sda_in(master);
+		msg <<= 1;
+		msg |= ~in_bit & 0x1;	/* Data is negative active */
+	}
+	cmd->bits = num_bits;
+	cmd->msg = msg;
+}
+
+static void serial_out(struct fsi_master_gpio *master,
+			const struct fsi_gpio_msg *cmd)
+{
+	uint8_t bit;
+	uint64_t msg = ~cmd->msg;	/* Data is negative active */
+	uint64_t sda_mask = 0x1ULL << (cmd->bits - 1);
+	uint64_t last_bit = ~0;
+	int next_bit;
+
+	if (!cmd->bits) {
+		dev_warn(master->master.dev, "trying to output 0 bits\n");
+		return;
+	}
+	set_sda_output(master, 0);
+
+	/* Send the start bit */
+	sda_out(master, 0);
+	clock_toggle(master, 1);
+
+	/* Send the message */
+	for (bit = 0; bit < cmd->bits; bit++) {
+		next_bit = (msg & sda_mask) >> (cmd->bits - 1);
+		if (last_bit ^ next_bit) {
+			sda_out(master, next_bit);
+			last_bit = next_bit;
+		}
+		clock_toggle(master, 1);
+		msg <<= 1;
+	}
+}
+
+/*
+ * Clock out some 0's after every message to ride out line reflections
+ */
+static void echo_delay(struct fsi_master_gpio *master)
+{
+	set_sda_output(master, 1);
+	clock_toggle(master, FSI_ECHO_DELAY_CLOCKS);
+}
+
+/*
+ * Used in bus error cases only.  Clears out any remaining data the slave
+ * is attempting to send
+ */
+static void drain_response(struct fsi_master_gpio *master)
+{
+	struct fsi_gpio_msg msg;
+
+	serial_in(master, &msg, FSI_GPIO_DRAIN_BITS);
+}
+
+/*
+ * Store information on master errors so handler can detect and clean
+ * up the bus
+ */
+static void fsi_master_gpio_error(struct fsi_master_gpio *master, int error)
+{
+
+}
+
+static int poll_for_response(struct fsi_master_gpio *master, uint8_t expected,
+			uint8_t size, void *data)
+{
+	int busy_count = 0, i;
+	struct fsi_gpio_msg response, cmd;
+	int bits_remaining = 0, bit_count, response_id, id;
+	uint64_t resp = 0;
+	uint8_t bits_received = FSI_GPIO_MSG_ID_SIZE +
+				FSI_GPIO_MSG_RESPID_SIZE;
+	uint8_t crc_in;
+
+	do {
+		for (i = 0; i < FSI_GPIO_MTOE_COUNT; i++) {
+			serial_in(master, &response, 1);
+			if (response.msg)
+				break;
+		}
+		if (i >= FSI_GPIO_MTOE_COUNT) {
+			dev_dbg(master->master.dev,
+				"Master time out waiting for response\n");
+			drain_response(master);
+			fsi_master_gpio_error(master, FSI_GPIO_MTOE);
+			return -EIO;
+		}
+
+		/* Response received */
+		bit_count = FSI_GPIO_MSG_ID_SIZE + FSI_GPIO_MSG_RESPID_SIZE;
+		serial_in(master, &response, bit_count);
+
+		response_id = response.msg & 0x3;
+		id = (response.msg >> FSI_GPIO_MSG_RESPID_SIZE) & 0x3;
+		dev_dbg(master->master.dev, "id:%d resp:%d\n", id, response_id);
+
+		resp = response.msg;
+
+		switch (response_id) {
+		case FSI_GPIO_RESP_ACK:
+			if (expected == FSI_GPIO_RESP_ACKD)
+				bits_remaining = 8 * size;
+			break;
+
+		case FSI_GPIO_RESP_BUSY:
+			/*
+			 * Its necessary to clock slave before issuing
+			 * d-poll, not indicated in the hardware protocol
+			 * spec. < 20 clocks causes slave to hang, 21 ok.
+			 */
+			set_sda_output(master, 1);
+			clock_toggle(master, FSI_GPIO_DPOLL_CLOCKS);
+			cmd.msg = FSI_GPIO_CMD_DPOLL;
+			cmd.bits = FSI_GPIO_CMD_DPOLL_SIZE;
+			serial_out(master, &cmd);
+			echo_delay(master);
+			continue;
+
+		case FSI_GPIO_RESP_ERRA:
+		case FSI_GPIO_RESP_ERRC:
+			dev_dbg(master->master.dev, "ERR received: %d\n",
+				(int)response.msg);
+			/*
+			 * todo: Verify crc from slave and in general
+			 * only act on any response if crc is correct
+			 */
+			clock_toggle(master, FSI_GPIO_CRC_SIZE);
+			fsi_master_gpio_error(master, response.msg);
+			return -EIO;
+		}
+
+		/* Read in the data field if applicable */
+		if (bits_remaining) {
+			serial_in(master, &response, bits_remaining);
+			resp <<= bits_remaining;
+			resp |= response.msg;
+			bits_received += bits_remaining;
+			*((uint32_t *)data) = response.msg;
+		}
+
+		crc_in = fsi_crc4(0, resp | (0x1ULL << bits_received),
+					bits_received + 1);
+
+		/* Read in the crc and check it */
+		serial_in(master, &response, FSI_GPIO_CRC_SIZE);
+		if (crc_in != response.msg) {
+			dev_dbg(master->master.dev, "ERR response CRC\n");
+			fsi_master_gpio_error(master, FSI_GPIO_CRC_INVAL);
+			return -EIO;
+		}
+		/* Clock the slave enough to be ready for next operation */
+		clock_toggle(master, FSI_GPIO_PRIME_SLAVE_CLOCKS);
+		return 0;
+
+	} while (busy_count++ < FSI_GPIO_MAX_BUSY);
+
+	dev_dbg(master->master.dev, "ERR slave is stuck in busy state\n");
+	fsi_master_gpio_error(master, FSI_GPIO_ERR_BUSY);
+
+	return -EIO;
+}
+
+static void build_abs_ar_command(struct fsi_gpio_msg *cmd, uint64_t mode,
+		uint8_t slave, uint32_t addr, size_t size,
+		const void *data)
+{
+	uint8_t crc;
+
+	cmd->bits = FSI_GPIO_CMD_DFLT_LEN;
+	cmd->msg = FSI_GPIO_CMD_DEFAULT;
+	cmd->msg |= mode;
+	cmd->msg &= ~FSI_GPIO_CMD_SLAVE_MASK;
+	cmd->msg |= (((uint64_t)slave) << FSI_GPIO_CMD_SLV_SHIFT);
+	addr &= FSI_GPIO_CMD_ADDR_MASK;
+	cmd->msg |= (((uint64_t)addr) << FSI_GPIO_CMD_ADDR_SHIFT);
+	if (size == sizeof(uint8_t)) {
+		if (data) {
+			uint8_t cmd_data = *((uint8_t *)data);
+
+			cmd->msg |=
+				((uint64_t)cmd_data) << FSI_GPIO_CMD_DT8_SHIFT;
+		}
+	} else if (size == sizeof(uint16_t)) {
+		cmd->msg |= FSI_GPIO_CMD_SIZE_16;
+		if (data) {
+			uint16_t cmd_data;
+
+			memcpy(&cmd_data, data, size);
+			cmd->msg |=
+				((uint64_t)cmd_data) << FSI_GPIO_CMD_DT16_SHIFT;
+		}
+	} else {
+		cmd->msg |= FSI_GPIO_CMD_SIZE_32;
+		if (data) {
+			uint32_t cmd_data;
+
+			memcpy(&cmd_data, data, size);
+			cmd->msg |=
+				((uint64_t)cmd_data) << FSI_GPIO_CMD_DT32_SHIFT;
+		}
+	}
+
+	if (mode == FSI_GPIO_CMD_WRITE)
+		cmd->bits += (8 * size);
+
+	/* Include start bit */
+	crc = fsi_crc4(0,
+			(cmd->msg >> (64 - cmd->bits)) | (0x1ULL << cmd->bits),
+			cmd->bits + 1);
+	cmd->msg |= ((uint64_t)crc) << (FSI_GPIO_CMD_CRC_SHIFT - cmd->bits);
+	cmd->bits += FSI_GPIO_CRC_SIZE;
+
+	/* Right align message */
+	cmd->msg >>= (64 - cmd->bits);
+}
+
+static int fsi_master_gpio_read(struct fsi_master *_master, int link,
+		uint8_t slave, uint32_t addr, void *val, size_t size)
+{
+	struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
+	struct fsi_gpio_msg cmd;
+	int rc;
+	unsigned long flags;
+
+	if (link != 0)
+		return -ENODEV;
+
+	build_abs_ar_command(&cmd, FSI_GPIO_CMD_READ, slave, addr, size, NULL);
+
+	spin_lock_irqsave(&fsi_gpio_cmd_lock, flags);
+	serial_out(master, &cmd);
+	echo_delay(master);
+	rc = poll_for_response(master, FSI_GPIO_RESP_ACKD, size, val);
+	spin_unlock_irqrestore(&fsi_gpio_cmd_lock, flags);
+
+	return rc;
+}
+
+static int fsi_master_gpio_write(struct fsi_master *_master, int link,
+		uint8_t slave, uint32_t addr, const void *val, size_t size)
+{
+	struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
+	struct fsi_gpio_msg cmd;
+	int rc;
+	unsigned long flags;
+
+	if (link != 0)
+		return -ENODEV;
+
+	build_abs_ar_command(&cmd, FSI_GPIO_CMD_WRITE, slave, addr, size, val);
+
+	spin_lock_irqsave(&fsi_gpio_cmd_lock, flags);
+	serial_out(master, &cmd);
+	echo_delay(master);
+	rc = poll_for_response(master, FSI_GPIO_RESP_ACK, size, NULL);
+	spin_unlock_irqrestore(&fsi_gpio_cmd_lock, flags);
+
+	return rc;
+}
+
+/*
+ * Issue a break command on link
+ */
+static int fsi_master_gpio_break(struct fsi_master *_master, int link)
+{
+	struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
+
+	if (link != 0)
+		return -ENODEV;
+
+	set_sda_output(master, 1);
+	clock_toggle(master, FSI_PRE_BREAK_CLOCKS);
+	sda_out(master, 0);
+	clock_toggle(master, FSI_BREAK_CLOCKS);
+	echo_delay(master);
+	sda_out(master, 1);
+	clock_toggle(master, FSI_POST_BREAK_CLOCKS);
+
+	/* Wait for logic reset to take effect */
+	udelay(200);
+
+	return 0;
+}
+
+static void fsi_master_gpio_init(struct fsi_master_gpio *master)
+{
+	if (master->gpio_mux)
+		gpiod_direction_output(master->gpio_mux, 1);
+	if (master->gpio_trans)
+		gpiod_direction_output(master->gpio_trans, 1);
+	if (master->gpio_enable)
+		gpiod_direction_output(master->gpio_enable, 1);
+	gpiod_direction_output(master->gpio_clk, 1);
+	gpiod_direction_output(master->gpio_data, 1);
+
+	/* todo: evaluate if clocks can be reduced */
+	clock_toggle(master, FSI_INIT_CLOCKS);
+}
+
+static int fsi_master_gpio_link_enable(struct fsi_master *_master, int link)
+{
+	struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
+
+	if (link != 0)
+		return -ENODEV;
+	if (master->gpio_enable)
+		gpiod_set_value(master->gpio_enable, 1);
+
+	return 0;
+}
+
+static ssize_t store_scan(struct device *dev,
+				struct device_attribute *attr,
+				const char *buf,
+				size_t count)
+{
+	struct fsi_master_gpio *master = dev_get_drvdata(dev);
+
+	fsi_master_gpio_init(master);
+
+	/* clear out any old scan data if present */
+	fsi_master_unregister(&master->master);
+	fsi_master_register(&master->master);
+
+	return count;
+}
+
+static DEVICE_ATTR(scan, 0200, NULL, store_scan);
+
+static int fsi_master_gpio_probe(struct platform_device *pdev)
+{
+	struct fsi_master_gpio *master;
+	struct gpio_desc *gpio;
+
+	master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL);
+	if (!master)
+		return -ENOMEM;
+
+	gpio = devm_gpiod_get(&pdev->dev, "clock", 0);
+	if (IS_ERR(gpio)) {
+		dev_dbg(&pdev->dev, "probe: failed to get clock pin\n");
+		return PTR_ERR(gpio);
+	}
+	master->gpio_clk = gpio;
+
+	gpio = devm_gpiod_get(&pdev->dev, "data", 0);
+	if (IS_ERR(gpio)) {
+		dev_dbg(&pdev->dev, "probe: failed to get data pin\n");
+		return PTR_ERR(gpio);
+	}
+	master->gpio_data = gpio;
+
+	/* Optional pins */
+
+	gpio = devm_gpiod_get(&pdev->dev, "trans", 0);
+	if (IS_ERR(gpio))
+		dev_dbg(&pdev->dev, "probe: failed to get trans pin\n");
+	else
+		master->gpio_trans = gpio;
+
+	gpio = devm_gpiod_get(&pdev->dev, "enable", 0);
+	if (IS_ERR(gpio))
+		dev_dbg(&pdev->dev, "probe: failed to get enable pin\n");
+	else
+		master->gpio_enable = gpio;
+
+	gpio = devm_gpiod_get(&pdev->dev, "mux", 0);
+	if (IS_ERR(gpio))
+		dev_dbg(&pdev->dev, "probe: failed to get mux pin\n");
+	else
+		master->gpio_mux = gpio;
+
+	master->master.n_links = 1;
+	master->master.read = fsi_master_gpio_read;
+	master->master.write = fsi_master_gpio_write;
+	master->master.send_break = fsi_master_gpio_break;
+	master->master.link_enable = fsi_master_gpio_link_enable;
+	platform_set_drvdata(pdev, master);
+
+	return device_create_file(&pdev->dev, &dev_attr_scan);
+}
+
+
+static int fsi_master_gpio_remove(struct platform_device *pdev)
+{
+	struct fsi_master_gpio *master = platform_get_drvdata(pdev);
+
+	devm_gpiod_put(&pdev->dev, master->gpio_clk);
+	devm_gpiod_put(&pdev->dev, master->gpio_data);
+	if (master->gpio_trans)
+		devm_gpiod_put(&pdev->dev, master->gpio_trans);
+	if (master->gpio_enable)
+		devm_gpiod_put(&pdev->dev, master->gpio_enable);
+	if (master->gpio_mux)
+		devm_gpiod_put(&pdev->dev, master->gpio_mux);
+	fsi_master_unregister(&master->master);
+
+	return 0;
+}
+
+static const struct of_device_id fsi_master_gpio_match[] = {
+	{ .compatible = "ibm,fsi-master-gpio" },
+	{ },
+};
+
+static struct platform_driver fsi_master_gpio_driver = {
+	.driver = {
+		.name		= "fsi-master-gpio",
+		.of_match_table	= fsi_master_gpio_match,
+	},
+	.probe	= fsi_master_gpio_probe,
+	.remove = fsi_master_gpio_remove,
+};
+
+module_platform_driver(fsi_master_gpio_driver);
+MODULE_LICENSE("GPL");
-- 
1.8.2.2

Re: [PATCH 00/16] FSI device driver introduction

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 498 bytes --]

Hi.

On Tue, Dec 06, 2016 at 06:14:21PM -0600, Chris Bostic wrote:
> [...]
>
> Introduction of the IBM 'Flexible Support Interface' (FSI) bus device
> driver. FSI is a high fan out serial bus consisting of a clock and a serial
> data line capable of running at speeds up to 166 MHz.
> 
> [...]

I would expect, that this information is added to Documentation/
and there should be Documentation/ABI/<stable or testing>/sysfs-bus-fsi

P.S.: I'm not sure, why I'm Cc'd.

-- Sebastian

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 14/16] drivers/fsi: Add master unscan

[Greg KH]

On Tue, Dec 06, 2016 at 06:14:35PM -0600, Chris Bostic wrote:
> From: Chris Bostic <cbostic@us.ibm.com>
> 
> Allow a master to undo a previous scan.  Should a master scan a bus
> twice it will need to ensure it doesn't double register any
> previously detected device.
> 
> Signed-off-by: Chris Bostic <cbostic@us.ibm.com>
> ---
>  drivers/fsi/fsi-core.c   | 36 +++++++++++++++++++++++++++++++++++-
>  drivers/fsi/fsi-master.h |  2 ++
>  include/linux/fsi.h      |  1 +
>  3 files changed, 38 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
> index a28434b..8ccfe50 100644
> --- a/drivers/fsi/fsi-core.c
> +++ b/drivers/fsi/fsi-core.c
> @@ -41,6 +41,8 @@
>  static atomic_t master_idx = ATOMIC_INIT(-1);
>  
>  struct fsi_slave {
> +	struct list_head	list_link;	/* Master's list of slaves */
> +	struct list_head	my_engines;
>  	struct device		dev;
>  	struct fsi_master	*master;
>  	int			link;
> @@ -196,6 +198,8 @@ static int fsi_slave_scan(struct fsi_slave *slave)
>  	uint32_t conf;
>  	int rc, i;
>  
> +	INIT_LIST_HEAD(&slave->my_engines);
> +
>  	/*
>  	 * scan engines
>  	 *
> @@ -264,7 +268,9 @@ static int fsi_slave_scan(struct fsi_slave *slave)
>  			if (rc) {
>  				dev_warn(&slave->dev, "add failed: %d\n", rc);
>  				put_device(&dev->dev);
> +				continue;
>  			}
> +			list_add(&dev->link, &slave->my_engines);
>  		}
>  
>  		engine_addr += slots * engine_page_size;
> @@ -357,7 +363,7 @@ static int fsi_slave_init(struct fsi_master *master,
>  		put_device(&slave->dev);
>  		return rc;
>  	}
> -
> +	list_add(&slave->list_link, &master->my_slaves);
>  	fsi_slave_scan(slave);
>  	return 0;
>  }
> @@ -388,6 +394,11 @@ static int fsi_master_scan(struct fsi_master *master)
>  	int link, slave_id, rc;
>  	uint32_t smode;
>  
> +	if (!master->slave_list) {
> +		INIT_LIST_HEAD(&master->my_slaves);
> +		master->slave_list = true;
> +	}
> +
>  	for (link = 0; link < master->n_links; link++) {
>  		rc = fsi_master_link_enable(master, link);
>  		if (rc) {
> @@ -423,9 +434,31 @@ static int fsi_master_scan(struct fsi_master *master)
>  	return 0;
>  }
>  
> +static void fsi_master_unscan(struct fsi_master *master)
> +{
> +	struct fsi_slave *slave, *slave_tmp;
> +	struct fsi_device *fsi_dev, *fsi_dev_tmp;
> +
> +	if (!master->slave_list)
> +		return;
> +
> +	list_for_each_entry_safe(slave, slave_tmp, &master->my_slaves,
> +							list_link) {
> +		list_del(&slave->list_link);
> +		list_for_each_entry_safe(fsi_dev, fsi_dev_tmp,
> +					&slave->my_engines, link) {
> +			list_del(&fsi_dev->link);
> +			put_device(&fsi_dev->dev);
> +		}
> +		device_unregister(&slave->dev);
> +	}
> +	master->slave_list = false;
> +}
> +
>  int fsi_master_register(struct fsi_master *master)
>  {
>  	master->idx = atomic_inc_return(&master_idx);
> +	master->slave_list = false;
>  	get_device(master->dev);
>  	fsi_master_scan(master);
>  	return 0;
> @@ -434,6 +467,7 @@ int fsi_master_register(struct fsi_master *master)
>  
>  void fsi_master_unregister(struct fsi_master *master)
>  {
> +	fsi_master_unscan(master);
>  	put_device(master->dev);
>  }
>  EXPORT_SYMBOL_GPL(fsi_master_unregister);
> diff --git a/drivers/fsi/fsi-master.h b/drivers/fsi/fsi-master.h
> index 56aad0e..454af2b 100644
> --- a/drivers/fsi/fsi-master.h
> +++ b/drivers/fsi/fsi-master.h
> @@ -20,6 +20,8 @@
>  #include <linux/device.h>
>  
>  struct fsi_master {
> +	struct list_head my_slaves;
> +	bool		slave_list;
>  	struct device	*dev;
>  	int		idx;
>  	int		n_links;
> diff --git a/include/linux/fsi.h b/include/linux/fsi.h
> index 66bce48..924502b 100644
> --- a/include/linux/fsi.h
> +++ b/include/linux/fsi.h
> @@ -18,6 +18,7 @@
>  #include <linux/device.h>
>  
>  struct fsi_device {
> +	struct list_head	link;	/* for slave's list */

Can't you use the device list on the bus instead?  Putting a device on
multiple lists gets tricky very quickly :(

thanks,

greg k-h

Re: [PATCH 15/16] drivers/fsi: Add documentation for GPIO bindings

[Mark Rutland]

On Tue, Dec 06, 2016 at 06:14:36PM -0600, Chris Bostic wrote:
> From: Chris Bostic <cbostic@us.ibm.com>
> 
> Add fsi master gpio device tree binding documentation

Please see Documentation/devicetree/bindings/submitting-patches.txt.

Specifically:

* Please put binding documents earlier in the series than code
  implementing the binding.

* Please document _all_ compatible strings used in the
  series.

Please also write the binding documents in terms of the hardware, rather
then the driver (e.g. introduce what the hardware is in the document,
don't mention the driver). The bindings are there to describe the former
to the latter, and the latter may change arbitrarily.

> Signed-off-by: Chris Bostic <cbostic@us.ibm.com>
> ---
>  .../devicetree/bindings/fsi/fsi-master-gpio.txt     | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt
> 
> diff --git a/Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt b/Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt

AFAICT, this is the first use of this directory. We should have a
general FSI binding document in there, covering what FSI is, the
"ibm,fsi-master" binding, etc.

> new file mode 100644
> index 0000000..ff3a62e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt
> @@ -0,0 +1,21 @@
> +Device-tree bindings for gpio-based FSI master driver

There's very little information here, so I'm not sure what to make of
this. Can you please elaborate on the above to make it clear what this
means?

IIUC, this is an FSI controller/master that we only communicate with via
GPIOs, right?

Or is this a *virtual* master? i.e. the GPIOs themselves form the master
and are directly connected to slaves?

> +-----------------------------------------------------
> +
> +Required properties:
> +	- compatible = "ibm,fsi-master-gpio";
> +	- clk-gpios;
> +	- data-gpios;

Please give a description of what each of these are used for, how many
are required, and what order elements must come in.

> +Optional properties:
> +	- enable-gpios;
> +	- trans-gpios;
> +	- mux-gpios;

Likewise.

> +
> +fsi-master {
> +	compatible = "ibm,fsi-master", "ibm,fsi-master-gpio";

This is backwards. The most specific string must come first.

> +	clk-gpios = <&gpio 0 &gpio 6>;
> +	data-gpios = <&gpio 1 &gpio 7>;
> +	enable-gpios = <&gpio 2 &gpio 8>;	/* Enable FSI data in/out */
> +	trans-gpios = <&gpio 3 &gpio 9>;	/* Volts translator direction */
> +	mux-gpios = <&gpio 4> &gpio 10>;	/* Multiplexer for FSI pins */

If this were described above, we don't need the comment here.

I note that in the patch, the mux-gpios property has an unmatched '>'
and won't compile.

As a general nit, please bracket elements of a list individually, e.g.

	trans-gpios = <&gpio 3>, <&gpio 9>;
	mux-gpios = <&gpio 4>, <&gpio 10>;

Thanks,
Mark.

Re: [PATCH 05/16] drivers/fsi: Add fake master driver

[Mark Rutland]

On Tue, Dec 06, 2016 at 06:14:26PM -0600, Chris Bostic wrote:
> From: Jeremy Kerr <jk@ozlabs.org>
> 
> For debugging, add a fake master driver, that only supports reads,
> returning a fixed set of data.

> +config FSI_MASTER_FAKE
> +	tristate "Fake FSI master"
> +	depends on FSI
> +	---help---
> +	This option enables a fake FSI master driver for debugging.
> +endif

> +static const struct of_device_id fsi_master_fake_match[] = {
> +	{ .compatible = "ibm,fsi-master-fake" },
> +	{ },
> +};

NAK.

DT should be treated as an ABI, and should describe the HW explicitly.
This makes no sense. This is also missing a binding document.

Have your module take a module parameter allowing you to bind it to
arbitrary devices, or do something like what PCI does where you can
bind/unbind arbitrary drivers to devices using sysfs.

> +
> +static struct platform_driver fsi_master_fake_driver = {
> +	.driver = {
> +		.name		= "fsi-master-fake",
> +		.of_match_table	= fsi_master_fake_match,
> +	},
> +	.probe	= fsi_master_fake_probe,
> +};
> +
> +static int __init fsi_master_fake_init(void)
> +{
> +	struct device_node *np;
> +
> +	platform_driver_register(&fsi_master_fake_driver);
> +
> +	for_each_compatible_node(np, NULL, "ibm,fsi-master-fake")
> +		of_platform_device_create(np, NULL, NULL);

As a general note, please use for_each_matching_node in situations like
this. That way you can reuse your existing of_device_id table, and not
reproduce the string.

That said, this is not necessary. The platform driver has an
of_match_table, so presumes the parent bus registers children, and hence
they should already have platform devices.

Thanks,
Mark.

Re: [PATCH 05/16] drivers/fsi: Add fake master driver

[Jeremy Kerr]

Hi Mark & Chris,

> On Tue, Dec 06, 2016 at 06:14:26PM -0600, Chris Bostic wrote:
>> From: Jeremy Kerr <jk@ozlabs.org>
>>
>> For debugging, add a fake master driver, that only supports reads,
>> returning a fixed set of data.
> 
>> +config FSI_MASTER_FAKE
>> +	tristate "Fake FSI master"
>> +	depends on FSI
>> +	---help---
>> +	This option enables a fake FSI master driver for debugging.
>> +endif
> 
>> +static const struct of_device_id fsi_master_fake_match[] = {
>> +	{ .compatible = "ibm,fsi-master-fake" },
>> +	{ },
>> +};
> 
> NAK.
> 
> DT should be treated as an ABI, and should describe the HW explicitly.
> This makes no sense. This is also missing a binding document.
> 
> Have your module take a module parameter allowing you to bind it to
> arbitrary devices, or do something like what PCI does where you can
> bind/unbind arbitrary drivers to devices using sysfs.

This driver is purely for testing the FSI engine scan code; we could
probably just drop this patch since I suspect that it's no longer useful
(now that we have an actual master driver).

If we do want to keep it though, I'd say we remove the device tree
dependency; all this is doing at the moment is triggering the ->probe,
and there are better ways to do that.

Cheers,


Jeremy

Re: [PATCH 16/16] drivers/fsi: Add GPIO based FSI master

[Jeremy Kerr]

Hi Chris,

> +static ssize_t store_scan(struct device *dev,
> +				struct device_attribute *attr,
> +				const char *buf,
> +				size_t count)
> +{
> +	struct fsi_master_gpio *master = dev_get_drvdata(dev);
> +
> +	fsi_master_gpio_init(master);
> +
> +	/* clear out any old scan data if present */
> +	fsi_master_unregister(&master->master);
> +	fsi_master_register(&master->master);
> +
> +	return count;
> +}
> +
> +static DEVICE_ATTR(scan, 0200, NULL, store_scan);

I think it would make more sense to have the scan attribute populated by
the fsi core; we want this on all masters, not just GPIO.

Currently, the only GPIO-master-specific functionality here is the
fsi_master_gpio_init() - but isn't this something that we can do at
probe time instead?

> +static int fsi_master_gpio_probe(struct platform_device *pdev)
> +{
> +	struct fsi_master_gpio *master;
> +	struct gpio_desc *gpio;
> +
> +	master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL);
> +	if (!master)
> +		return -ENOMEM;

We should be populating master->dev.parent, see

  https://github.com/jk-ozlabs/linux/commit/5225d6c47


> +	/* Optional pins */
> +
> +	gpio = devm_gpiod_get(&pdev->dev, "trans", 0);
> +	if (IS_ERR(gpio))
> +		dev_dbg(&pdev->dev, "probe: failed to get trans pin\n");
> +	else
> +		master->gpio_trans = gpio;

I found devm_gpiod_get_optional(), which might make this a little
neater.

Cheers,


Jeremy

Re: [PATCH 16/16] drivers/fsi: Add GPIO based FSI master

[Christopher Bostic]

On Thu, Dec 8, 2016 at 10:12 PM, Jeremy Kerr <jk@ozlabs.org> wrote:
> Hi Chris,
>
>> +static ssize_t store_scan(struct device *dev,
>> +                             struct device_attribute *attr,
>> +                             const char *buf,
>> +                             size_t count)
>> +{
>> +     struct fsi_master_gpio *master = dev_get_drvdata(dev);
>> +
>> +     fsi_master_gpio_init(master);
>> +
>> +     /* clear out any old scan data if present */
>> +     fsi_master_unregister(&master->master);
>> +     fsi_master_register(&master->master);
>> +
>> +     return count;
>> +}
>> +
>> +static DEVICE_ATTR(scan, 0200, NULL, store_scan);
>
> I think it would make more sense to have the scan attribute populated by
> the fsi core; we want this on all masters, not just GPIO.
>

Hi Jeremy,

Sure, will move that to the core.

> Currently, the only GPIO-master-specific functionality here is the
> fsi_master_gpio_init() - but isn't this something that we can do at
> probe time instead?
>

Yes that can be done at probe time.  Will change.

>> +static int fsi_master_gpio_probe(struct platform_device *pdev)
>> +{
>> +     struct fsi_master_gpio *master;
>> +     struct gpio_desc *gpio;
>> +
>> +     master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL);
>> +     if (!master)
>> +             return -ENOMEM;
>
> We should be populating master->dev.parent, see
>
>   https://github.com/jk-ozlabs/linux/commit/5225d6c47
>
>

Will make the change.

>> +     /* Optional pins */
>> +
>> +     gpio = devm_gpiod_get(&pdev->dev, "trans", 0);
>> +     if (IS_ERR(gpio))
>> +             dev_dbg(&pdev->dev, "probe: failed to get trans pin\n");
>> +     else
>> +             master->gpio_trans = gpio;
>
> I found devm_gpiod_get_optional(), which might make this a little
> neater.

Will make this change.


Thanks,
Chris

>
> Cheers,
>
>
> Jeremy