[PATCH linux v2 00/17] FSI device driver introduction

[PATCH linux v2 00/17] FSI device driver introduction

[christopher.lee.bostic]

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

Patches 01-13 have been provided by Jeremy Kerr as an example of
how FSI function should be phased in. This includes the basic framework
to add FSI extensions to the Linux bus and device models.

Patch 14: Minor changes to text formatting, white space removal.

Patch 15: Recommended changes to patches 01-13 as provided by Jeremy
Kerr.

	V2: Removed suggested renaming of struct fsi_slave to fsi_cfam

Patch 16: Set up CFAM communications.  Send break command and init
target cfam id based on master type.

Patch 17: Define FSI gpio master functionality.


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 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
    *  Interrupt functionality

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.

* Link
    The combination of a serial clock and data line constituting one
    FSI communications element. For each link there is a master on one
    end and a CFAM/slave on the other end.

* Engine
    A self contained hardware function found within a CFAM.  Examples
    include I2C masters, UARTs, GPIOs, etc...

* Client
    A device driver requiring access to its hardware via an FSI bus.
    For example an I2C client would be a device driver requiring
    access to an I2C master engine on a remote CFAM accessible via
    an FSI link.  Clients register with the FSI bus and will receive
    notifications of bus state changes as well as hot plug events related
    to engines of interest to them.

---





Chris Bostic (4):
  drivers/fsi: Fix some text formatting
  drivers/fsi: Revisions to existing patch set
  drivers/fsi: Set up CFAMs for communication
  drivers/fsi: Add GPIO master functionality

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

 .../devicetree/bindings/fsi/fsi-master-gpio.txt    |   8 +
 drivers/Kconfig                                    |   2 +
 drivers/Makefile                                   |   1 +
 drivers/fsi/Kconfig                                |  29 ++
 drivers/fsi/Makefile                               |   4 +
 drivers/fsi/fsi-cfam.h                             |  63 ++++
 drivers/fsi/fsi-core.c                             | 398 +++++++++++++++++++++
 drivers/fsi/fsi-master-fake.c                      | 114 ++++++
 drivers/fsi/fsi-master-gpio.c                      | 259 ++++++++++++++
 drivers/fsi/fsi-master.h                           |  42 +++
 drivers/fsi/fsi-slave.h                            |  62 ++++
 include/linux/fsi.h                                |  60 ++++
 12 files changed, 1042 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-cfam.h
 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 drivers/fsi/fsi-slave.h
 create mode 100644 include/linux/fsi.h

-- 
1.8.2.2

[PATCH linux v2 01/17] fsi: Add empty fsi bus definitions

[christopher.lee.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>
---
 drivers/Kconfig        |  2 ++
 drivers/Makefile       |  1 +
 drivers/fsi/Kconfig    | 12 ++++++++++++
 drivers/fsi/Makefile   |  2 ++
 drivers/fsi/fsi-core.c | 38 ++++++++++++++++++++++++++++++++++++++
 include/linux/fsi.h    | 23 +++++++++++++++++++++++
 6 files changed, 78 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 0b6f3d6..7a1c96f 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..8e8bdea
--- /dev/null
+++ b/include/linux/fsi.h
@@ -0,0 +1,23 @@
+/*
+ * 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 linux v2 02/17] fsi: Add device & driver definitions

[christopher.lee.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>
---
 include/linux/fsi.h | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/include/linux/fsi.h b/include/linux/fsi.h
index 8e8bdea..66dcf25 100644
--- a/include/linux/fsi.h
+++ b/include/linux/fsi.h
@@ -18,6 +18,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 linux v2 03/17] fsi: add driver to device matches

[christopher.lee.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>
---
 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 66dcf25..6d843d4 100644
--- a/include/linux/fsi.h
+++ b/include/linux/fsi.h
@@ -19,11 +19,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 linux v2 04/17] fsi: Add fsi master definition

[christopher.lee.bostic]

From: Jeremy Kerr <jk@ozlabs.org>

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
---
 drivers/fsi/fsi-core.c   | 20 ++++++++++++++++++++
 drivers/fsi/fsi-master.h | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+)
 create mode 100644 drivers/fsi/fsi-master.h

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 3d55bd5..ce9428d 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -17,6 +17,26 @@
 #include <linux/fsi.h>
 #include <linux/module.h>
 
+#include "fsi-master.h"
+
+static atomic_t master_idx = ATOMIC_INIT(-1);
+
+/* FSI master support */
+
+int fsi_master_register(struct fsi_master *master)
+{
+	master->idx = atomic_inc_return(&master_idx);
+	get_device(master->dev);
+	return 0;
+}
+EXPORT_SYMBOL_GPL(fsi_master_register);
+
+void fsi_master_unregister(struct fsi_master *master)
+{
+	put_device(master->dev);
+}
+EXPORT_SYMBOL_GPL(fsi_master_unregister);
+
 /* FSI core & Linux bus type definitions */
 
 static int fsi_bus_match(struct device *dev, struct device_driver *drv)
diff --git a/drivers/fsi/fsi-master.h b/drivers/fsi/fsi-master.h
new file mode 100644
index 0000000..e75a810
--- /dev/null
+++ b/drivers/fsi/fsi-master.h
@@ -0,0 +1,37 @@
+/*
+ * FSI master definitions. These comprise the core <--> master interface,
+ * to allow the core to interact with the (hardware-specific) masters.
+ *
+ * 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 DRIVERS_FSI_MASTER_H
+#define DRIVERS_FSI_MASTER_H
+
+#include <linux/device.h>
+
+struct fsi_master {
+	struct device	*dev;
+	int		idx;
+	int		n_links;
+	int		(*read)(struct fsi_master *, int link,
+				uint8_t slave, uint32_t addr,
+				void *val, size_t size);
+	int		(*write)(struct fsi_master *, int link,
+				uint8_t slave, uint32_t addr,
+				const void *val, size_t size);
+};
+
+extern int fsi_master_register(struct fsi_master *master);
+extern void fsi_master_unregister(struct fsi_master *master);
+
+#endif /* DRIVERS_FSI_MASTER_H */
-- 
1.8.2.2

[PATCH linux v2 05/17] fsi: Add fake master driver

[christopher.lee.bostic]

From: Jeremy Kerr <jk@ozlabs.org>

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

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
---
 drivers/fsi/Kconfig           | 10 +++++
 drivers/fsi/Makefile          |  1 +
 drivers/fsi/fsi-master-fake.c | 96 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 107 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..50aac0b
--- /dev/null
+++ b/drivers/fsi/fsi-master-fake.c
@@ -0,0 +1,96 @@
+/*
+ * 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"
+
+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 linux v2 06/17] fsi: enable debug

[christopher.lee.bostic]

From: Jeremy Kerr <jk@ozlabs.org>

Enable debug for the fsi core during development. Remove before
submission.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
---
 drivers/fsi/fsi-core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index ce9428d..db1a1ce 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -13,6 +13,8 @@
  * GNU General Public License for more details.
  */
 
+#define DEBUG
+
 #include <linux/device.h>
 #include <linux/fsi.h>
 #include <linux/module.h>
-- 
1.8.2.2

[PATCH linux v2 07/17] fsi: Add slave defintion

[christopher.lee.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>
---
 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 db1a1ce..7df4291 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -23,6 +23,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 linux v2 08/17] fsi: Add empty master scan

[christopher.lee.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>
---
 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 7df4291..9744a55 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -21,6 +21,8 @@
 
 #include "fsi-master.h"
 
+#define FSI_N_SLAVES	4
+
 static atomic_t master_idx = ATOMIC_INIT(-1);
 
 struct fsi_slave {
@@ -32,12 +34,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 linux v2 09/17] fsi: Add crc4 helpers

[christopher.lee.bostic]

From: Jeremy Kerr <jk@ozlabs.org>

Add some helpers for the crc checks for the slave configuration table.
This works 4-bits-at-a-time, using a simple table approach.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
---
 drivers/fsi/fsi-core.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 9744a55..24538ae 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -34,6 +34,29 @@ struct fsi_slave {
 
 #define to_fsi_slave(d) container_of(d, struct fsi_slave, dev)
 
+/* crc helpers */
+static const uint8_t crc4_tab[] = {
+	0x0, 0x7, 0xe, 0x9, 0xb, 0xc, 0x5, 0x2,
+	0x1, 0x6, 0xf, 0x8, 0xa, 0xd, 0x4, 0x3,
+};
+
+static uint8_t crc4(uint32_t x)
+{
+	uint8_t c = 0;
+	int i;
+
+	/* Calculate crc4 over four-bit nibbles, starting at the MSbit */
+	for (i = 28; i >= 0; i -= 4)
+		c = crc4_tab[c ^ ((x >> i) & 0xf)];
+
+	return c;
+}
+
+static bool check_crc4(uint32_t x)
+{
+	return crc4(x) == 0;
+}
+
 /* FSI slave support */
 static int fsi_slave_init(struct fsi_master *master,
 		int link, uint8_t slave_id)
-- 
1.8.2.2

[PATCH linux v2 10/17] fsi: Implement slave initialisation

[christopher.lee.bostic]

From: Jeremy Kerr <jk@ozlabs.org>

Create fsi_slave devices during the master scan.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
---
 drivers/fsi/fsi-core.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 48 insertions(+), 2 deletions(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 24538ae..49a8e14 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -18,6 +18,7 @@
 #include <linux/device.h>
 #include <linux/fsi.h>
 #include <linux/module.h>
+#include <linux/slab.h>
 
 #include "fsi-master.h"
 
@@ -58,12 +59,57 @@ static bool check_crc4(uint32_t x)
 }
 
 /* FSI slave support */
+
+static void fsi_slave_release(struct device *dev)
+{
+	struct fsi_slave *slave = to_fsi_slave(dev);
+	kfree(slave);
+}
+
 static int fsi_slave_init(struct fsi_master *master,
 		int link, uint8_t slave_id)
 {
-	/* todo: initialise slave device, perform engine scan */
+	struct fsi_slave *slave;
+	uint32_t chip_id;
+	int rc;
+
+	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",
+				link, slave_id, rc);
+		return -ENODEV;
+	}
+
+	chip_id = be32_to_cpu(chip_id);
+	if (!check_crc4(chip_id)) {
+		dev_warn(master->dev, "slave %02x:%02x: invalid chip id CRC!\n",
+				link, slave_id);
+		return -EIO;
+	}
+
+	pr_debug("fsi: found chip %08x at %02x:%02x:%02x\n",
+			master->idx, chip_id, link, slave_id);
+
+	/* we can communicate with a slave; create devices and scan */
+	slave = kzalloc(sizeof(*slave), GFP_KERNEL);
+	if (!slave)
+		return -ENOMEM;
+
+	slave->master = master;
+	slave->id = slave_id;
+	slave->dev.parent = master->dev;
+	slave->dev.release = fsi_slave_release;
+
+	dev_set_name(&slave->dev, "slave@%02x:%02x", link, slave_id);
+	rc = device_register(&slave->dev);
+	if (rc < 0) {
+		dev_warn(master->dev, "failed to create slave device: %d\n",
+				rc);
+		put_device(&slave->dev);
+		return rc;
+	}
 
-	return -ENODEV;
+	return rc;
 }
 
 /* FSI master support */
-- 
1.8.2.2

[PATCH linux v2 11/17] fsi: scan slaves & register devices

[christopher.lee.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.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
---
 drivers/fsi/fsi-core.c | 125 ++++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/fsi.h    |   4 ++
 2 files changed, 128 insertions(+), 1 deletion(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 49a8e14..63d26b5 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -24,6 +24,16 @@
 
 #define FSI_N_SLAVES	4
 
+#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
+
+static const int engine_page_size = 0x400;
+
 static atomic_t master_idx = ATOMIC_INIT(-1);
 
 struct fsi_slave {
@@ -35,6 +45,29 @@ 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 +92,95 @@ static bool check_crc4(uint32_t x)
 }
 
 /* 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;
+		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;
+		}
+
+		conf = be32_to_cpu(conf);
+
+		if (!check_crc4(conf)) {
+			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->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)
 {
@@ -109,7 +231,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 6d843d4..dfd3513 100644
--- a/include/linux/fsi.h
+++ b/include/linux/fsi.h
@@ -22,6 +22,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 linux v2 12/17] fsi: Add device read/write/peek functions

[christopher.lee.bostic]

From: Jeremy Kerr <jk@ozlabs.org>

This change introduces the fsi device API: simple read, write and peek
accessors for the devices' address spaces.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
---
 drivers/fsi/fsi-core.c | 44 ++++++++++++++++++++++++++++++++++++++++++++
 include/linux/fsi.h    |  7 ++++++-
 2 files changed, 50 insertions(+), 1 deletion(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 63d26b5..10bf817 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -32,6 +32,8 @@
 #define FSI_SLAVE_CONF_TYPE_MASK	0x00000ff0
 #define FSI_SLAVE_CONF_TYPE_SHIFT	4
 
+#define FSI_PEEK_BASE			0x410
+
 static const int engine_page_size = 0x400;
 
 static atomic_t master_idx = ATOMIC_INIT(-1);
@@ -45,7 +47,42 @@ struct fsi_slave {
 
 #define to_fsi_slave(d) container_of(d, struct fsi_slave, dev)
 
+static int fsi_slave_read(struct fsi_slave *slave, uint32_t addr,
+		void *val, size_t size);
+static int fsi_slave_write(struct fsi_slave *slave, uint32_t addr,
+		const void *val, size_t size);
+
 /* FSI endpoint-device support */
+int fsi_device_read(struct fsi_device *dev, uint32_t addr, void *val,
+		size_t size)
+{
+	if (addr > dev->size)
+		return -EINVAL;
+
+	if (addr + size > dev->size)
+		return -EINVAL;
+
+	return fsi_slave_read(dev->slave, dev->addr + addr, val, size);
+}
+
+int fsi_device_write(struct fsi_device *dev, uint32_t addr, const void *val,
+		size_t size)
+{
+	if (addr > dev->size)
+		return -EINVAL;
+
+	if (addr + size > dev->size)
+		return -EINVAL;
+
+	return fsi_slave_write(dev->slave, dev->addr + addr, val, size);
+}
+
+int fsi_device_peek(struct fsi_device *dev, void *val)
+{
+	uint32_t addr = FSI_PEEK_BASE + ((dev->unit - 2) * sizeof(uint32_t));
+
+	return fsi_slave_read(dev->slave, addr, val, sizeof(uint32_t));
+}
 
 static void fsi_device_release(struct device *_device)
 {
@@ -99,6 +136,13 @@ static int fsi_slave_read(struct fsi_slave *slave, uint32_t addr,
 			slave->id, addr, val, size);
 }
 
+static int fsi_slave_write(struct fsi_slave *slave, uint32_t addr,
+		const void *val, size_t size)
+{
+	return slave->master->write(slave->master, slave->link,
+			slave->id, addr, val, size);
+}
+
 static int fsi_slave_scan(struct fsi_slave *slave)
 {
 	uint32_t engine_addr;
diff --git a/include/linux/fsi.h b/include/linux/fsi.h
index dfd3513..47af0af 100644
--- a/include/linux/fsi.h
+++ b/include/linux/fsi.h
@@ -28,6 +28,12 @@ struct fsi_device {
 	uint32_t		size;
 };
 
+extern int fsi_device_read(struct fsi_device *dev, uint32_t addr,
+		void *val, size_t size);
+extern int fsi_device_write(struct fsi_device *dev, uint32_t addr,
+		const void *val, size_t size);
+extern int fsi_device_peek(struct fsi_device *dev, void *val);
+
 struct fsi_device_id {
 	u8	engine_type;
 	u8	version;
@@ -41,7 +47,6 @@ struct fsi_device_id {
 #define FSI_DEVICE_VERSIONED(t, v) \
 	.engine_type = (t), .version = (v),
 
-
 struct fsi_driver {
 	struct device_driver		drv;
 	const struct fsi_device_id	*id_table;
-- 
1.8.2.2

[PATCH linux v2 13/17] fsi: Add GPIO master driver

[christopher.lee.bostic]

From: Jeremy Kerr <jk@ozlabs.org>

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
---
 .../devicetree/bindings/fsi/fsi-master-gpio.txt    |  8 ++
 drivers/fsi/Kconfig                                |  7 ++
 drivers/fsi/Makefile                               |  1 +
 drivers/fsi/fsi-master-gpio.c                      | 92 ++++++++++++++++++++++
 4 files changed, 108 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt
 create mode 100644 drivers/fsi/fsi-master-gpio.c

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..44762a3
--- /dev/null
+++ b/Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt
@@ -0,0 +1,8 @@
+Device-tree bindings for gpio-based FSI master driver
+-----------------------------------------------------
+
+fsi-master {
+	compatible = "ibm,fsi-master", "ibm,fsi-master-gpio";
+	clk-gpio = <&gpio 0>;
+	data-gpio = <&gpio 1>;
+}
diff --git a/drivers/fsi/Kconfig b/drivers/fsi/Kconfig
index f065dbe..69e7ee8 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
+	select GPIO_DEVRES
+	---help---
+	This option enables a FSI master driver, using GPIO lines directly.
 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..3855829
--- /dev/null
+++ b/drivers/fsi/fsi-master-gpio.c
@@ -0,0 +1,92 @@
+/*
+ * 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 "fsi-master.h"
+
+struct fsi_master_gpio {
+	struct fsi_master	master;
+	struct gpio_desc	*gpio_clk;
+	struct gpio_desc	*gpio_data;
+};
+
+#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 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);
+
+	if (link != 0)
+		return -ENODEV;
+
+	/* todo: format read into a message, send, poll for response */
+	(void)master;
+
+
+	return 0;
+}
+
+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);
+
+	if (link != 0)
+		return -ENODEV;
+
+	/* todo: format write into a message, send, poll for response */
+	(void)master;
+
+	return 0;
+}
+
+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, "clk", 0);
+	if (IS_ERR(gpio))
+		return PTR_ERR(gpio);
+	master->gpio_clk = gpio;
+
+	gpio = devm_gpiod_get(&pdev->dev, "data", 0);
+	if (IS_ERR(gpio))
+		return PTR_ERR(gpio);
+	master->gpio_data = gpio;
+
+	master->master.read = fsi_master_gpio_read;
+	master->master.write = fsi_master_gpio_write;
+
+	return fsi_master_register(&master->master);
+}
+
+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,
+};
+
+module_platform_driver(fsi_master_gpio_driver);
+
-- 
1.8.2.2

[PATCH linux v2 14/17] drivers/fsi: Fix some text formatting

[christopher.lee.bostic]

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

Cleaned up a few formatting warnings like blank lines at EOF
and block comment style.

Signed-off-by: Chris Bostic <cbostic@us.ibm.com>
---
 drivers/fsi/fsi-core.c        | 8 ++++++--
 drivers/fsi/fsi-master-fake.c | 1 -
 drivers/fsi/fsi-master-gpio.c | 1 -
 3 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 10bf817..a3a860a 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -87,6 +87,7 @@ int fsi_device_peek(struct fsi_device *dev, void *val)
 static void fsi_device_release(struct device *_device)
 {
 	struct fsi_device *device = to_fsi_dev(_device);
+
 	kfree(device);
 }
 
@@ -185,8 +186,10 @@ static int fsi_slave_scan(struct fsi_slave *slave)
 		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 */
+		/*
+		 * Unused address areas are marked by a zero type value; this
+		 * skips the defined address areas
+		 */
 		if (type != 0) {
 
 			/* create device */
@@ -229,6 +232,7 @@ static int fsi_slave_scan(struct fsi_slave *slave)
 static void fsi_slave_release(struct device *dev)
 {
 	struct fsi_slave *slave = to_fsi_slave(dev);
+
 	kfree(slave);
 }
 
diff --git a/drivers/fsi/fsi-master-fake.c b/drivers/fsi/fsi-master-fake.c
index 50aac0b..ec1ed5e 100644
--- a/drivers/fsi/fsi-master-fake.c
+++ b/drivers/fsi/fsi-master-fake.c
@@ -93,4 +93,3 @@ static int __init fsi_master_fake_init(void)
 }
 
 module_init(fsi_master_fake_init);
-
diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
index 3855829..5c3d5cd 100644
--- a/drivers/fsi/fsi-master-gpio.c
+++ b/drivers/fsi/fsi-master-gpio.c
@@ -89,4 +89,3 @@ static struct platform_driver fsi_master_gpio_driver = {
 };
 
 module_platform_driver(fsi_master_gpio_driver);
-
-- 
1.8.2.2

[PATCH linux v2 15/17] drivers/fsi: Revisions to existing patch set

[christopher.lee.bostic]

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

Changes recommended to the patch set so far as supplied by
Jeremy Kerr.   Moved some CFAM specific details to a new cfam header

Signed-off-by: Chris Bostic <cbostic@us.ibm.com>

Changes in V2:

- Removed the renaming of slave to cfam
---
 drivers/fsi/fsi-cfam.h | 63 ++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/fsi/fsi-core.c | 22 ++++--------------
 2 files changed, 68 insertions(+), 17 deletions(-)
 create mode 100644 drivers/fsi/fsi-cfam.h

diff --git a/drivers/fsi/fsi-cfam.h b/drivers/fsi/fsi-cfam.h
new file mode 100644
index 0000000..ddb03cd
--- /dev/null
+++ b/drivers/fsi/fsi-cfam.h
@@ -0,0 +1,63 @@
+/*
+ * FSI CFAM definitions
+ *
+ * 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 DRIVERS_FSI_CFAM_H
+#define DRIVERS_FSI_CFAM_H
+
+#include <linux/device.h>
+
+#include "fsi-master.h"
+
+#define FSI_SLAVE_ENG_ID	2
+
+#define FSI_MAX_CFAMS_PER_LINK	4
+#define FSI_CFAM_SIZE		(FSI_LINK_SIZE / FSI_MAX_CFAMS_PER_LINK)
+#define FSI_PEEK_BASE		0x410
+#define FSI_SLAVE_BASE		0x800
+
+/* Config space decoding */
+#define FSI_CFG_NEXT_MASK	0x80000000
+#define FSI_CFG_SLOT_MASK	0x00ff0000
+#define FSI_CFG_SLOT_SHIFT	16
+#define FSI_CFG_VERS_MASK	0x0000f000
+#define FSI_CFG_VERS_SHIFT	12
+#define FSI_CFG_TYPE_MASK	0x00000ff0
+#define FSI_CFG_TYPE_SHIFT	4
+
+/*
+ * Return number of slots in the configuration word
+ */
+static inline uint8_t fsi_cfg_slot(uint32_t x)
+{
+	return (x & FSI_CFG_SLOT_MASK) >> FSI_CFG_SLOT_SHIFT;
+}
+
+/*
+ * Return version in the configuration word
+ */
+static inline uint8_t fsi_cfg_version(uint32_t x)
+{
+	return (x & FSI_CFG_VERS_MASK) >> FSI_CFG_VERS_SHIFT;
+}
+
+/*
+ * Return type field in the configuration word
+ */
+static inline uint8_t fsi_cfg_type(uint32_t x)
+{
+	return (x & FSI_CFG_TYPE_MASK) >> FSI_CFG_TYPE_SHIFT;
+}
+
+#endif /* DRIVERS_FSI_CFAM_H */
diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index a3a860a..0da8cf0 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -21,19 +21,10 @@
 #include <linux/slab.h>
 
 #include "fsi-master.h"
+#include "fsi-cfam.h"
 
 #define FSI_N_SLAVES	4
 
-#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_PEEK_BASE			0x410
-
 static const int engine_page_size = 0x400;
 
 static atomic_t master_idx = ATOMIC_INIT(-1);
@@ -179,12 +170,9 @@ static int fsi_slave_scan(struct fsi_slave *slave)
 			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;
+		slots = fsi_cfg_slot(conf);
+		version = fsi_cfg_version(conf);
+		type = fsi_cfg_type(conf);
 
 		/*
 		 * Unused address areas are marked by a zero type value; this
@@ -222,7 +210,7 @@ static int fsi_slave_scan(struct fsi_slave *slave)
 
 		engine_addr += slots * engine_page_size;
 
-		if (!(conf & FSI_SLAVE_CONF_NEXT_MASK))
+		if (!(conf & FSI_CFG_NEXT_MASK))
 			break;
 	}
 
-- 
1.8.2.2

[PATCH linux v2 16/17] drivers/fsi: Set up CFAMs for communication

[christopher.lee.bostic]

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

Issue break command to reset CFAM logic and set ID as appropriate
for a particular master type.  Begin definition of the slave
engine register set and accessors.

Signed-off-by: Chris Bostic <cbostic@us.ibm.com>
---
 drivers/fsi/fsi-core.c        | 58 +++++++++++++++++++++++++++++++++++++++-
 drivers/fsi/fsi-master-fake.c | 19 +++++++++++++
 drivers/fsi/fsi-master-gpio.c | 31 ++++++++++++++++++++++
 drivers/fsi/fsi-master.h      |  5 ++++
 drivers/fsi/fsi-slave.h       | 62 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 174 insertions(+), 1 deletion(-)
 create mode 100644 drivers/fsi/fsi-slave.h

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 0da8cf0..ac8c287 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -22,6 +22,7 @@
 
 #include "fsi-master.h"
 #include "fsi-cfam.h"
+#include "fsi-slave.h"
 
 #define FSI_N_SLAVES	4
 
@@ -224,13 +225,68 @@ static void fsi_slave_release(struct device *dev)
 	kfree(slave);
 }
 
+/*
+ * Issue a break commad on this link
+ */
+static int fsi_master_break(struct fsi_master *master, int link)
+{
+	int rc = 0;
+
+	if (master->send_break)
+		rc = master->send_break(master, link);
+
+	return rc;
+}
+
+static void set_smode_defaults(struct fsi_master *master, uint32_t *smode)
+{
+	*smode = FSI_SMODE_WSC | FSI_SMODE_ECRC
+		| fsi_smode_echodly(0xf) | fsi_smode_senddly(0xf)
+		| fsi_smode_lbcrr(1);
+}
+
+static int fsi_master_set_smode(struct fsi_master *master, int link)
+{
+	uint32_t smode;
+	int rc;
+
+	if (!master->set_smode)
+		return -ENODEV;
+
+	set_smode_defaults(master, &smode);
+	rc = master->set_smode(master, link, smode);
+
+	return rc;
+}
+
 static int fsi_slave_init(struct fsi_master *master,
 		int link, uint8_t slave_id)
 {
-	struct fsi_slave *slave;
+	struct fsi_slave *slave = NULL;
 	uint32_t chip_id;
 	int rc;
 
+	/*
+	 * todo: Due to CFAM hardware issues related to BREAK commands we're
+	 * limited to only one CFAM per link. Once hardware fixes are available
+	 * this restriction can be removed.
+	 */
+	if (slave_id > 0)
+		return 0;
+
+	rc = fsi_master_break(master, link);
+	if (rc) {
+		dev_warn(master->dev, "no slave detected at %02x:%02x\n",
+				link, slave_id);
+		return -ENODEV;
+	}
+
+	rc = fsi_master_set_smode(master, link);
+	if (rc) {
+		dev_warn(master->dev, "failed to set smode id, rc:%d\n", 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",
diff --git a/drivers/fsi/fsi-master-fake.c b/drivers/fsi/fsi-master-fake.c
index ec1ed5e..08a083a 100644
--- a/drivers/fsi/fsi-master-fake.c
+++ b/drivers/fsi/fsi-master-fake.c
@@ -51,6 +51,23 @@ static int fsi_master_fake_write(struct fsi_master *_master, int link,
 	return -EACCES;
 }
 
+static int fsi_master_fake_break(struct fsi_master *master, int link)
+{
+	if (link != 0)
+		return -ENODEV;
+
+	return 0;
+}
+
+static int fsi_master_fake_set_smode(struct fsi_master *master, int link,
+					uint32_t smode)
+{
+	if (link != 0)
+		return -ENODEV;
+
+	return 0;
+}
+
 static int fsi_master_fake_probe(struct platform_device *pdev)
 {
 	struct fsi_master *master;
@@ -63,6 +80,8 @@ static int fsi_master_fake_probe(struct platform_device *pdev)
 	master->n_links = 1;
 	master->read = fsi_master_fake_read;
 	master->write = fsi_master_fake_write;
+	master->send_break = fsi_master_fake_break;
+	master->set_smode = fsi_master_fake_set_smode;
 
 	return fsi_master_register(master);
 }
diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
index 5c3d5cd..3b2770d 100644
--- a/drivers/fsi/fsi-master-gpio.c
+++ b/drivers/fsi/fsi-master-gpio.c
@@ -7,6 +7,8 @@
 #include <linux/module.h>
 
 #include "fsi-master.h"
+#include "fsi-cfam.h"
+#include "fsi-slave.h"
 
 struct fsi_master_gpio {
 	struct fsi_master	master;
@@ -50,6 +52,33 @@ static int fsi_master_gpio_write(struct fsi_master *_master, int link,
 	return 0;
 }
 
+/*
+ * 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;
+
+	/* todo: send the break pattern over gpio */
+	(void)master;
+
+	return 0;
+}
+
+static int fsi_master_gpio_set_smode(struct fsi_master *_master, int link,
+						uint32_t smode)
+{
+	int rc;
+
+	rc = _master->write(_master, link, 0, FSI_SLAVE_BASE + FSI_SMODE,
+			&smode, sizeof(smode));
+
+	return rc;
+}
+
 static int fsi_master_gpio_probe(struct platform_device *pdev)
 {
 	struct fsi_master_gpio *master;
@@ -71,6 +100,8 @@ static int fsi_master_gpio_probe(struct platform_device *pdev)
 
 	master->master.read = fsi_master_gpio_read;
 	master->master.write = fsi_master_gpio_write;
+	master->master.send_break = fsi_master_gpio_break;
+	master->master.set_smode = fsi_master_gpio_set_smode;
 
 	return fsi_master_register(&master->master);
 }
diff --git a/drivers/fsi/fsi-master.h b/drivers/fsi/fsi-master.h
index e75a810..73b62e0 100644
--- a/drivers/fsi/fsi-master.h
+++ b/drivers/fsi/fsi-master.h
@@ -19,6 +19,8 @@
 
 #include <linux/device.h>
 
+#define FSI_CMD_BREAK		0xc0de0000	/* Break command */
+
 struct fsi_master {
 	struct device	*dev;
 	int		idx;
@@ -29,6 +31,9 @@ struct fsi_master {
 	int		(*write)(struct fsi_master *, int link,
 				uint8_t slave, uint32_t addr,
 				const void *val, size_t size);
+	int		(*send_break)(struct fsi_master *, int link);
+	int		(*set_smode)(struct fsi_master *, int link,
+				uint32_t smode);
 };
 
 extern int fsi_master_register(struct fsi_master *master);
diff --git a/drivers/fsi/fsi-slave.h b/drivers/fsi/fsi-slave.h
new file mode 100644
index 0000000..ea7a760
--- /dev/null
+++ b/drivers/fsi/fsi-slave.h
@@ -0,0 +1,62 @@
+/*
+ * FSI slave engine definitions.
+ *
+ * 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 DRIVERS_FSI_SLAVE_H
+#define DRIVERS_FSI_SLAVE_H
+
+/*
+ * FSI slave engine control register offsets
+ */
+#define	FSI_SMODE		0x0	/* R/W: Mode register */
+
+/*
+ * SMODE fields
+ */
+#define	FSI_SMODE_WSC		0x80000000	/* Warm start completed */
+#define	FSI_SMODE_ECRC		0x20000000	/* Enable hardware 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		/* Clock rate ratio shift */
+#define	FSI_SMODE_LBCRR_MASK	0xf		/* Clock rate ratio mask */
+
+/* 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;
+}
+
+#endif /* DRIVERS_FSI_SLAVE_H */
-- 
1.8.2.2

[PATCH linux v2 17/17] drivers/fsi: Add GPIO master functionality

[christopher.lee.bostic]

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

Add setup of the GPIO pins for FSI master function.
Set up I/O directions, define all pins needed and
set up their initial values.  Define serial out
operation.

Signed-off-by: Chris Bostic <cbostic@us.ibm.com>
---
 drivers/fsi/fsi-master-gpio.c | 143 +++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 140 insertions(+), 3 deletions(-)

diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
index 3b2770d..7ad1293 100644
--- a/drivers/fsi/fsi-master-gpio.c
+++ b/drivers/fsi/fsi-master-gpio.c
@@ -5,15 +5,29 @@
 #include <linux/platform_device.h>
 #include <linux/gpio/consumer.h>
 #include <linux/module.h>
+#include <linux/delay.h>
 
 #include "fsi-master.h"
 #include "fsi-cfam.h"
 #include "fsi-slave.h"
 
+#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	/* Clear out any old data and states */
+
+#define FSI_GPIO_STD_DELAY	10	/* Standard GPIO delay in nS */
+					/* todo: adjust down as low as */
+					/* possible or eliminate */
+
 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)
@@ -23,6 +37,8 @@ struct fsi_gpio_msg {
 	uint8_t		bits;
 };
 
+static void serial_out(struct fsi_master_gpio *, struct fsi_gpio_msg *);
+
 static int fsi_master_gpio_read(struct fsi_master *_master, int link,
 		uint8_t slave, uint32_t addr, void *val, size_t size)
 {
@@ -42,6 +58,7 @@ 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;
 
 	if (link != 0)
 		return -ENODEV;
@@ -49,23 +66,115 @@ static int fsi_master_gpio_write(struct fsi_master *_master, int link,
 	/* todo: format write into a message, send, poll for response */
 	(void)master;
 
+	/* todo: fill in message */
+	cmd.msg = 0;
+	cmd.bits = 0;
+	serial_out(master, &cmd);
+
 	return 0;
 }
 
+static void sda_out(struct fsi_master_gpio *master, int value)
+{
+	gpiod_set_value(master->gpio_data, value);
+
+	/* Required -do not remove */
+	ndelay(FSI_GPIO_STD_DELAY);
+}
+
+static void set_sda_output(struct fsi_master_gpio *master,
+			   int value)
+{
+	gpiod_direction_output(master->gpio_data, value);
+	gpiod_direction_output(master->gpio_trans, 1);
+}
+
+static void set_clock(struct fsi_master_gpio *master)
+{
+	/* This delay is required - do not remove */
+	ndelay(FSI_GPIO_STD_DELAY);
+	gpiod_set_value(master->gpio_clk, 1);
+}
+
+static void clear_clock(struct fsi_master_gpio *master)
+{
+	/* This delay is required - do not remove */
+	ndelay(FSI_GPIO_STD_DELAY);
+	gpiod_set_value(master->gpio_clk, 0);
+}
+
+static void clock_toggle(struct fsi_master_gpio *master, int count)
+{
+	int i;
+
+	for (i = 0; i < count; i++) {
+		clear_clock(master);
+		set_clock(master);
+	}
+}
+
+/*
+ * 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, 0);
+	clock_toggle(master, FSI_ECHO_DELAY_CLOCKS);
+}
+
+static void serial_out(struct fsi_master_gpio *master, struct fsi_gpio_msg *cmd)
+{
+	uint16_t bit;
+	uint64_t msg = ~cmd->msg;	/* Data is negative active */
+	uint64_t sda_mask = 1;
+	uint64_t last_bit = ~0;
+
+	set_sda_output(master, 0);
+
+	/* Send the start bit */
+	sda_out(master, 1);
+	clock_toggle(master, 1);
+
+	/* Send the message */
+	for (bit = 0; bit < cmd->bits; bit++) {
+		if (last_bit ^ (msg & sda_mask)) {
+			sda_out(master, msg & sda_mask);
+			last_bit = msg & sda_mask;
+		}
+		clock_toggle(master, 1);
+		msg >>= 1;
+	}
+}
+
 /*
  * 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);
+	uint32_t smode;
+	int rc;
 
 	if (link != 0)
 		return -ENODEV;
 
-	/* todo: send the break pattern over gpio */
-	(void)master;
+	set_sda_output(master, 0);
+	clock_toggle(master, FSI_PRE_BREAK_CLOCKS);
 
-	return 0;
+	sda_out(master, 1);
+	clock_toggle(master, FSI_BREAK_CLOCKS);
+
+	echo_delay(master);
+
+	sda_out(master, 0);
+	clock_toggle(master, FSI_POST_BREAK_CLOCKS);
+
+	udelay(200);		/* wait for logic reset to take effect */
+	rc = _master->read(_master, link, 3, FSI_SLAVE_BASE + FSI_SMODE,
+			&smode, sizeof(smode));
+	dev_info(_master->dev, "smode after break:%08x rc:%d\n", smode, rc);
+
+	return rc;
 }
 
 static int fsi_master_gpio_set_smode(struct fsi_master *_master, int link,
@@ -79,6 +188,17 @@ static int fsi_master_gpio_set_smode(struct fsi_master *_master, int link,
 	return rc;
 }
 
+static void fsi_master_gpio_init(struct fsi_master_gpio *master)
+{
+	gpiod_direction_output(master->gpio_mux, 1);
+	gpiod_direction_output(master->gpio_clk, 1);
+	set_sda_output(master, 1);
+	gpiod_direction_output(master->gpio_enable, 1);
+
+	/* todo: evaluate if clocks can be reduced */
+	clock_toggle(master, FSI_INIT_CLOCKS);
+}
+
 static int fsi_master_gpio_probe(struct platform_device *pdev)
 {
 	struct fsi_master_gpio *master;
@@ -98,11 +218,28 @@ static int fsi_master_gpio_probe(struct platform_device *pdev)
 		return PTR_ERR(gpio);
 	master->gpio_data = gpio;
 
+	gpio = devm_gpiod_get(&pdev->dev, "trans", 0);
+	if (IS_ERR(gpio))
+		return PTR_ERR(gpio);
+	master->gpio_trans = gpio;
+
+	gpio = devm_gpiod_get(&pdev->dev, "enable", 0);
+	if (IS_ERR(gpio))
+		return PTR_ERR(gpio);
+	master->gpio_enable = gpio;
+
+	gpio = devm_gpiod_get(&pdev->dev, "mux", 0);
+	if (IS_ERR(gpio))
+		return PTR_ERR(gpio);
+	master->gpio_mux = gpio;
+
 	master->master.read = fsi_master_gpio_read;
 	master->master.write = fsi_master_gpio_write;
 	master->master.send_break = fsi_master_gpio_break;
 	master->master.set_smode = fsi_master_gpio_set_smode;
 
+	fsi_master_gpio_init(master);
+
 	return fsi_master_register(&master->master);
 }
 
-- 
1.8.2.2

Re: [PATCH linux v2 17/17] drivers/fsi: Add GPIO master functionality

[Jeremy Kerr]

Hi Chris,

> Add setup of the GPIO pins for FSI master function.
> Set up I/O directions, define all pins needed and
> set up their initial values.  Define serial out
> operation.
> 
> Signed-off-by: Chris Bostic <cbostic@us.ibm.com>

Awesome! I assume this is still in development though, right?

>  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 */
>  };

OK, you should update the device-tree binding to include these extra
GPIOs (and put their descriptions in there). Are they required for all
implementations? (is it possible that some would only have clock and
data?)

Cheers,


Jeremy

Re: [PATCH linux v2 16/17] drivers/fsi: Set up CFAMs for communication

[Jeremy Kerr]

Hi Chris,

> +/*
> + * Issue a break commad on this link
> + */
> +static int fsi_master_break(struct fsi_master *master, int link)
> +{
> +	int rc = 0;
> +
> +	if (master->send_break)
> +		rc = master->send_break(master, link);
> +
> +	return rc;
> +}

OK, looks good.

> +static int fsi_master_fake_break(struct fsi_master *master, int link)
> +{
> +	if (link != 0)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +
> +static int fsi_master_fake_set_smode(struct fsi_master *master, int link,
> +					uint32_t smode)
> +{
> +	if (link != 0)
> +		return -ENODEV;
> +
> +	return 0;
> +}
> +

Since these are no-ops...

>  static int fsi_master_fake_probe(struct platform_device *pdev)
>  {
>  	struct fsi_master *master;
> @@ -63,6 +80,8 @@ static int fsi_master_fake_probe(struct platform_device *pdev)
>  	master->n_links = 1;
>  	master->read = fsi_master_fake_read;
>  	master->write = fsi_master_fake_write;
> +	master->send_break = fsi_master_fake_break;
> +	master->set_smode = fsi_master_fake_set_smode;

... we can just leave the callbacks NULL (the wrapper functions check
this).

However:

> +static int fsi_master_gpio_set_smode(struct fsi_master *_master, int link,
> +						uint32_t smode)
> +{
> +	int rc;
> +
> +	rc = _master->write(_master, link, 0, FSI_SLAVE_BASE + FSI_SMODE,
> +			&smode, sizeof(smode));
> +
> +	return rc;
> +}
> +

Won't this always be implemented the same way on all masters (ie, a
write to a particular slave register)? Why the need for a
master-specific callback here?

> +#define FSI_CMD_BREAK		0xc0de0000	/* Break command */
> +

This isn't used anywhere; what's it for? It doesn't look like something
that all masters will use, so shouldn't be in the global master header.

> @@ -29,6 +31,9 @@ struct fsi_master {
>  	int		(*write)(struct fsi_master *, int link,
>  				uint8_t slave, uint32_t addr,
>  				const void *val, size_t size);
> +	int		(*send_break)(struct fsi_master *, int link);
> +	int		(*set_smode)(struct fsi_master *, int link,
> +				uint32_t smode);

See above comments - do we really need a set_smode callback?

Cheers,


Jeremy

Re: [PATCH linux v2 14/17] drivers/fsi: Fix some text formatting

[Jeremy Kerr]

Hi Chris,

> Cleaned up a few formatting warnings like blank lines at EOF
> and block comment style.
> 
> Signed-off-by: Chris Bostic <cbostic@us.ibm.com>

This looks good to me. Do you want me to apply these as fixups to my
patches now, or do you want to carry this separately?

Note that we'll need to apply as fixup before sending upstream, so this
is just a matter of when would suit you best.

Cheers,


Jeremy

Re: [PATCH linux v2 17/17] drivers/fsi: Add GPIO master functionality

[Christopher Bostic]

On Sun, Oct 9, 2016 at 7:56 PM, Jeremy Kerr <jk@ozlabs.org> wrote:
> Hi Chris,
>
>> Add setup of the GPIO pins for FSI master function.
>> Set up I/O directions, define all pins needed and
>> set up their initial values.  Define serial out
>> operation.
>>
>> Signed-off-by: Chris Bostic <cbostic@us.ibm.com>
>
> Awesome! I assume this is still in development though, right?
>

Yes its not complete as it stands in this set.  I'll put the remainder
in version 3
of this set

>>  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 */
>>  };
>
> OK, you should update the device-tree binding to include these extra

> GPIOs (and put their descriptions in there). Are they required for all
> implementations? (is it possible that some would only have clock and
> data?)
>

OK Will update the dev-tree binding.

Good question about other implementations.  It is possible that they could have
only clock and data.  I suspect some type of enable should be used but
it wouldn't be strictly necessary.  What would you suggest given the potential
for pin differences between platforms?

Thanks,
Chris

> Cheers,
>
>
> Jeremy

Re: [PATCH linux v2 16/17] drivers/fsi: Set up CFAMs for communication

[Christopher Bostic]

On Sun, Oct 9, 2016 at 7:56 PM, Jeremy Kerr <jk@ozlabs.org> wrote:
> Hi Chris,
>
>> +/*
>> + * Issue a break commad on this link
>> + */
>> +static int fsi_master_break(struct fsi_master *master, int link)
>> +{
>> +     int rc = 0;
>> +
>> +     if (master->send_break)
>> +             rc = master->send_break(master, link);
>> +
>> +     return rc;
>> +}
>
> OK, looks good.
>
>> +static int fsi_master_fake_break(struct fsi_master *master, int link)
>> +{
>> +     if (link != 0)
>> +             return -ENODEV;
>> +
>> +     return 0;
>> +}
>> +
>> +static int fsi_master_fake_set_smode(struct fsi_master *master, int link,
>> +                                     uint32_t smode)
>> +{
>> +     if (link != 0)
>> +             return -ENODEV;
>> +
>> +     return 0;
>> +}
>> +
>
> Since these are no-ops...
>
>>  static int fsi_master_fake_probe(struct platform_device *pdev)
>>  {
>>       struct fsi_master *master;
>> @@ -63,6 +80,8 @@ static int fsi_master_fake_probe(struct platform_device *pdev)
>>       master->n_links = 1;
>>       master->read = fsi_master_fake_read;
>>       master->write = fsi_master_fake_write;
>> +     master->send_break = fsi_master_fake_break;
>> +     master->set_smode = fsi_master_fake_set_smode;
>
> ... we can just leave the callbacks NULL (the wrapper functions check
> this).
>

Hi Jeremy,

For break and smode callbacks I did what you had done for
fsi_master_fake_write( ).
I'm not sure how mine are any different?

> However:
>
>> +static int fsi_master_gpio_set_smode(struct fsi_master *_master, int link,
>> +                                             uint32_t smode)
>> +{
>> +     int rc;
>> +
>> +     rc = _master->write(_master, link, 0, FSI_SLAVE_BASE + FSI_SMODE,
>> +                     &smode, sizeof(smode));
>> +
>> +     return rc;
>> +}
>> +
>
> Won't this always be implemented the same way on all masters (ie, a
> write to a particular slave register)? Why the need for a
> master-specific callback here?


This will be different for various masters based on the hardware
issues mentioned
in previous notes.  For example the cascaded master needs to set a different ID
in smode.

>
>> +#define FSI_CMD_BREAK                0xc0de0000      /* Break command */
>> +
>
> This isn't used anywhere; what's it for? It doesn't look like something
> that all masters will use, so shouldn't be in the global master header.

OK, I'll move this out of the global master header.

>
>> @@ -29,6 +31,9 @@ struct fsi_master {
>>       int             (*write)(struct fsi_master *, int link,
>>                               uint8_t slave, uint32_t addr,
>>                               const void *val, size_t size);
>> +     int             (*send_break)(struct fsi_master *, int link);
>> +     int             (*set_smode)(struct fsi_master *, int link,
>> +                             uint32_t smode);
>
> See above comments - do we really need a set_smode callback?
>
> Cheers,
>
>
> Jeremy

Re: [PATCH linux v2 14/17] drivers/fsi: Fix some text formatting

[Christopher Bostic]

On Sun, Oct 9, 2016 at 9:51 PM, Jeremy Kerr <jk@ozlabs.org> wrote:
> Hi Chris,
>
>> Cleaned up a few formatting warnings like blank lines at EOF
>> and block comment style.
>>
>> Signed-off-by: Chris Bostic <cbostic@us.ibm.com>
>
> This looks good to me. Do you want me to apply these as fixups to my
> patches now, or do you want to carry this separately?
>
> Note that we'll need to apply as fixup before sending upstream, so this
> is just a matter of when would suit you best.
>
Hi Jeremy,

I'd be fine with leaving it as it is here in this patch set.  Not
familiar with what fixups are in
this context.

-Chris

> Cheers,
>
>
> Jeremy

Re: [PATCH linux v2 17/17] drivers/fsi: Add GPIO master functionality

[Jeremy Kerr]

Hi Chris,

> OK Will update the dev-tree binding.

OK, but we should sort out the below first:

> Good question about other implementations.  It is possible that they
> could have only clock and data.  I suspect some type of enable should
> be used but it wouldn't be strictly necessary.  What would you suggest
> given the potential for pin differences between platforms?

Make only the clk and data descriptors mandatory, and the others
optional. Enable extra functionality on the master if the other GPIOs
are present & described.

Cheers,


Jeremy

Re: [PATCH linux v2 14/17] drivers/fsi: Fix some text formatting

[Jeremy Kerr]

Hi Chris,

>> This looks good to me. Do you want me to apply these as fixups to my
>> patches now, or do you want to carry this separately?
>>
>> Note that we'll need to apply as fixup before sending upstream, so this
>> is just a matter of when would suit you best.
>>
> Hi Jeremy,
> 
> I'd be fine with leaving it as it is here in this patch set.  Not
> familiar with what fixups are in
> this context.

This would mean that I rework my initial series (ie, 1-13) to include
this change (but rolled into the changes, rather than a separate patch).
Since this updates my base series, you would re-consume that series, and
would drop this change from your set.

However, I'm happy to leave it as-is for now. We'd still need to do the
above before submitting upstream, but that can come later.

Cheers,


Jeremy

Re: [PATCH linux v2 16/17] drivers/fsi: Set up CFAMs for communication

[Jeremy Kerr]

Hi Chris,

> For break and smode callbacks I did what you had done for
> fsi_master_fake_write( ).
> I'm not sure how mine are any different?

These implementations don't do anything, so no there's no point
populating those callbacks.

>>> +static int fsi_master_gpio_set_smode(struct fsi_master *_master, int link,
>>> +                                             uint32_t smode)
>>> +{
>>> +     int rc;
>>> +
>>> +     rc = _master->write(_master, link, 0, FSI_SLAVE_BASE + FSI_SMODE,
>>> +                     &smode, sizeof(smode));
>>> +
>>> +     return rc;
>>> +}
>>> +
>>
>> Won't this always be implemented the same way on all masters (ie, a
>> write to a particular slave register)? Why the need for a
>> master-specific callback here?
> 
> 
> This will be different for various masters based on the hardware
> issues mentioned in previous notes.  For example the cascaded master
> needs to set a different ID in smode.

Is sounds like you're putting slave logic (is, the actual value of
smode) into the master code then.

Keep the abstraction at the right level. If the smode depends on whether
the master is cascaded, then all the master needs to expose is whether
or not it's cascaded. It shouldn't need an entirely new callback to do a
write to a particular (fixed?) slave register.

However, since this isn't really used, I can't tell whether it's the
right thing to do or not. Can you defer changes like this (which are
used to support specific functionality - in this case adding cascades)
until their corresponding consumers are added too? This allows us to
review the changes with their proper context.

It seems like the priority at the moment should be getting a single
master going, with interrupts, then we can look at cascading.

Cheers,


Jeremy

Re: [PATCH linux v2 16/17] drivers/fsi: Set up CFAMs for communication

[Joel Stanley]

Chris,

On Tue, Oct 11, 2016 at 2:48 AM, Jeremy Kerr <jk@ozlabs.org> wrote:
> It seems like the priority at the moment should be getting a single
> master going, with interrupts, then we can look at cascading.

This is an important point that I think is worth restating. Back when
we met in July, we agreed the goal should be implement only what was
required to perform a putscom on the host.

I believe this helps us work out what is essential for v1, and what
can be deferred to the second set of patches.

Cheers,

Joel

Re: [PATCH linux v2 16/17] drivers/fsi: Set up CFAMs for communication

[Christopher Bostic]

On Mon, Oct 10, 2016 at 7:04 PM, Joel Stanley <joel@jms.id.au> wrote:
> Chris,
>
> On Tue, Oct 11, 2016 at 2:48 AM, Jeremy Kerr <jk@ozlabs.org> wrote:
>> It seems like the priority at the moment should be getting a single
>> master going, with interrupts, then we can look at cascading.
>
> This is an important point that I think is worth restating. Back when
> we met in July, we agreed the goal should be implement only what was
> required to perform a putscom on the host.
>
> I believe this helps us work out what is essential for v1, and what
> can be deferred to the second set of patches.
>

Joel,

Agreed.  I'll hold off on any of the cascaded master considerations for now.

Thanks.

> Cheers,
>
> Joel