[PATCH linux v5 00/18] FSI device driver introduction

[PATCH linux v5 00/18] FSI device driver introduction

[christopher.lee.bostic]

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

Patch Descriptions:

Patches 01-12 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 13 adds function to set up read/write access to a given CFAM.

Patch 14 sets up slave smode during scanning operations to allow
config space access.

Patch 15 defines a FSI master hardware implmentation using I/O.
The base of this was created by Jeremy Kerr and I've added additional
functionality on top of this.

Patch 16 is core additions for client driver registration.

Patch 17 implements a basic SCOM FSI client device driver.

Patch 18 adds some introductory documentation explaining FSI.

Chris Bostic (6):
  drivers/fsi: Set up links for slave communication
  drivers/fsi: Set slave SMODE to init communications
  drivers/fsi: Add GPIO FSI master
  drivers/fsi: Add client driver register utilities
  drivers/fsi: Add SCOM FSI client device driver
  Documenation: Add basic FSI text file

Jeremy Kerr (12):
  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 definition
  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

 .../devicetree/bindings/fsi/fsi-master-gpio.txt    |  21 +
 Documentation/fsi.txt                              |  36 ++
 arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts      |  30 ++
 drivers/Kconfig                                    |   2 +
 drivers/Makefile                                   |   1 +
 drivers/fsi/Kconfig                                |  35 ++
 drivers/fsi/Makefile                               |   5 +
 drivers/fsi/fsi-core.c                             | 493 +++++++++++++++++++++
 drivers/fsi/fsi-master-fake.c                      |  95 ++++
 drivers/fsi/fsi-master-gpio.c                      | 464 +++++++++++++++++++
 drivers/fsi/fsi-master.h                           |  60 +++
 drivers/fsi/fsi-scom.c                             | 217 +++++++++
 include/linux/fsi.h                                |  72 +++
 13 files changed, 1531 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt
 create mode 100644 Documentation/fsi.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 drivers/fsi/fsi-scom.c
 create mode 100644 include/linux/fsi.h

-- 
1.8.2.2

[PATCH linux v5 01/18] 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 v5 02/18] 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 v5 03/18] 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 v5 04/18] 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 v5 05/18] 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.

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..ec1ed5e
--- /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"
+
+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 v5 06/18] 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 v5 07/18] fsi: Add slave definition

[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 v5 08/18] 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 v5 09/18] 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.

We will need this in the FSI core code, as well as any master
implementations that need to calculate CRCs in software.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
---
 drivers/fsi/fsi-core.c   | 25 +++++++++++++++++++++++++
 drivers/fsi/fsi-master.h | 21 +++++++++++++++++++++
 2 files changed, 46 insertions(+)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 9744a55..2d22320 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -34,6 +34,31 @@ 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,
+};
+
+uint8_t fsi_crc4(uint8_t c, uint64_t x, int bits)
+{
+	int i;
+
+	/* Align to 4-bits */
+	bits = (bits + 3) & ~0x3;
+
+	/* Calculate crc4 over four-bit nibbles, starting at the MSbit */
+	for (i = bits; i >= 0; i -= 4)
+		c = crc4_tab[c ^ ((x >> i) & 0xf)];
+
+	return c;
+}
+
+static bool check_crc4_u32(uint32_t x)
+{
+	return fsi_crc4(0, x, 32) == 0;
+}
+
 /* FSI slave support */
 static int fsi_slave_init(struct fsi_master *master,
 		int link, uint8_t slave_id)
diff --git a/drivers/fsi/fsi-master.h b/drivers/fsi/fsi-master.h
index e75a810..cafb433 100644
--- a/drivers/fsi/fsi-master.h
+++ b/drivers/fsi/fsi-master.h
@@ -34,4 +34,25 @@ struct fsi_master {
 extern int fsi_master_register(struct fsi_master *master);
 extern void fsi_master_unregister(struct fsi_master *master);
 
+/**
+ * crc4 helper: Given a starting crc4 state @c, calculate the crc4 vaue of @x,
+ * which is @bits in length. This may be required by master implementations
+ * that do not provide their own hardware checksums.
+ *
+ * The crc4 is performed on 4-bit chunks (which is all we need for FSI
+ * calculations). Typically, we'll want a starting state of 0:
+ *
+ *  c = fsi_crc4(0, msg, len);
+ *
+ * To crc4 a message that includes a single start bit, initialise crc4 state
+ * with:
+ *
+ *  c = fsi_crc4(0, 1, 1);
+ *
+ * Then update with message data:
+ *
+ *  c = fsi_crc4(c, msg, len);
+ */
+uint8_t fsi_crc4(uint8_t c, uint64_t x, int bits);
+
 #endif /* DRIVERS_FSI_MASTER_H */
-- 
1.8.2.2

[PATCH linux v5 10/18] 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 | 51 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 49 insertions(+), 2 deletions(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 2d22320..4282370 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"
 
@@ -60,12 +61,58 @@ static bool check_crc4_u32(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_u32(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 v5 11/18] 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.

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 | 128 ++++++++++++++++++++++++++++++++++++++++++++++++-
 include/linux/fsi.h    |   4 ++
 2 files changed, 131 insertions(+), 1 deletion(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 4282370..50e0616 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,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,
@@ -61,6 +95,97 @@ static bool check_crc4_u32(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_u32(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)
 {
@@ -112,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 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 v5 12/18] 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 50e0616..565c7e3 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)
 {
@@ -102,6 +139,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 v5 13/18] drivers/fsi: Set up links for slave communication

[christopher.lee.bostic]

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

Enable each link and send a break command in preparation
for scanning each link for slaves.

V3 - Remove definition of BREAK command from fsi-master.h

   - Remove definitions of fsi_master_fake for set_smode
     and break

   - Remove master->set_smode master dependent function and
     moved to a generic base master set_smode.

   - Add fsi_master_link_enable with master type dependencies

V4 - Remove all references to set smode

   - Remove file fsi-slave.h

   - Move link break and enable up into master scan

   - Change rc = func(); return rc; coding to return func();
     Note this is still in place in fsi_master_send_break in
     anticipation of changes coming up in next patch of this
     series.

   - Fix comment spelling error

   - Use dev_dbg instead of dev_info when link enable or break
     fails

   - Remove explicit set of master->break and
     master->link_enable to NULL due to kzalloc

Signed-off-by: Chris Bostic <cbostic@us.ibm.com>
---
 drivers/fsi/fsi-core.c   | 42 ++++++++++++++++++++++++++++++++++++++++--
 drivers/fsi/fsi-master.h |  2 ++
 2 files changed, 42 insertions(+), 2 deletions(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 565c7e3..81ed444 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -287,13 +287,51 @@ static int fsi_slave_init(struct fsi_master *master,
 
 /* FSI master support */
 
+static int fsi_master_link_enable(struct fsi_master *master, int link)
+{
+	if (master->link_enable)
+		return master->link_enable(master, link);
+
+	return -EINVAL;
+}
+
+/*
+ * Issue a break command on this link
+ */
+static int fsi_master_break(struct fsi_master *master, int link)
+{
+	int rc;
+
+	if (!master->send_break)
+		return -EINVAL;
+
+	rc = master->send_break(master, link);
+	if (rc)
+		dev_info(master->dev, "break failed with:%d\n", rc);
+
+	return rc;
+}
+
 static int fsi_master_scan(struct fsi_master *master)
 {
-	int link, slave_id;
+	int link, slave_id, rc;
 
-	for (link = 0; link < master->n_links; link++)
+	for (link = 0; link < master->n_links; link++) {
+		rc = fsi_master_link_enable(master, link);
+		if (rc) {
+			dev_dbg(master->dev,
+				"Enable link:%d failed with:%d\n", link, rc);
+			continue;
+		}
+		rc = fsi_master_break(master, link);
+		if (rc) {
+			dev_dbg(master->dev,
+				"Break to link:%d failed with:%d\n", link, rc);
+			continue;
+		}
 		for (slave_id = 0; slave_id < FSI_N_SLAVES; slave_id++)
 			fsi_slave_init(master, link, slave_id);
+	}
 
 	return 0;
 
diff --git a/drivers/fsi/fsi-master.h b/drivers/fsi/fsi-master.h
index cafb433..56aad0e 100644
--- a/drivers/fsi/fsi-master.h
+++ b/drivers/fsi/fsi-master.h
@@ -29,6 +29,8 @@ 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		(*link_enable)(struct fsi_master *, int link);
 };
 
 extern int fsi_master_register(struct fsi_master *master);
-- 
1.8.2.2

[PATCH linux v5 14/18] drivers/fsi: Set slave SMODE to init communications

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

V3 - Remove master->set_smode op and make it generic for
     all masters.

V4 - 'Change rc = func(); return rc;' code to 'return func();'

   - Change set_smode_defaults to return value instead of
     passing back via reference.

   - Rename fsi_master_set_smode to fsi_slave_set_smode

   - Pass in slave id to fsi_slave_set_smode instead of
     making assumptions internal to the function.

   - Only allow slave inits on id 0 - a todo to accommodate
     all slave ID's.

V5 - Move SMODE read at default ID out of break and into
     the caller.

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

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 81ed444..ed0d20f 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -33,6 +33,7 @@
 #define FSI_SLAVE_CONF_TYPE_SHIFT	4
 
 #define FSI_PEEK_BASE			0x410
+#define FSI_SLAVE_BASE			0x800
 
 static const int engine_page_size = 0x400;
 
@@ -52,6 +53,25 @@ 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 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	/* 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		/* 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)
@@ -132,6 +152,31 @@ static bool check_crc4_u32(uint32_t x)
 }
 
 /* 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)
 {
@@ -238,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, 0, FSI_SLAVE_BASE + FSI_SMODE,
+			&smode, sizeof(smode));
+}
+
 static int fsi_slave_init(struct fsi_master *master,
 		int link, uint8_t slave_id)
 {
@@ -245,6 +306,21 @@ static int fsi_slave_init(struct fsi_master *master,
 	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 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",
@@ -300,21 +376,16 @@ static int fsi_master_link_enable(struct fsi_master *master, int link)
  */
 static int fsi_master_break(struct fsi_master *master, int link)
 {
-	int rc;
-
-	if (!master->send_break)
-		return -EINVAL;
-
-	rc = master->send_break(master, link);
-	if (rc)
-		dev_info(master->dev, "break failed with:%d\n", rc);
+	if (master->send_break)
+		return master->send_break(master, link);
 
-	return rc;
+	return -EINVAL;
 }
 
 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);
@@ -329,6 +400,17 @@ static int fsi_master_scan(struct fsi_master *master)
 				"Break to link:%d failed with:%d\n", link, rc);
 			continue;
 		}
+		/* Verify can read slave at default ID location. If fails */
+		/* then 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 linux v5 15/18] drivers/fsi: Add GPIO FSI master

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

V3 - Added remainder of base I/O ops (excluding dev tree and crc calcs).

   - Add encoding of all read/write commands and send data over sda

   - Check for and decode response from slave.

   - Add set enable pin in master->link_enable() master specific function.

V4 - Remove unnecessary comments about delays in various locations

   - Define non clock and data pins as optional and check for gpio
     descriptors == NULL before accessing.  Don't return error if
     optional pins cannot be retrieved via devm.

   - serial_in: rearrange order of msg shift and input bit OR op.

   - serial_out: Fix mirror image issue on shift data out and set
     data out as necessary for bit count to be sent.

   - serial_out: make message parm a const

   - serial_out: fix 3x 'msg & data' repeat

   - poll_for_response: bits remaining was calculated with
     sizeof(size) should be size.

   - fsi_master_gpio_break: remove smode read

   - Replace dev_info with dev_dbg in proper places.

   - Add a bit count parm to serial_in, increment msg->bits
     on every bit received.

   - Remove magic numbers in poll_for_response that indicate
     how many bits to receive

   - Utilize the crc utilities to check input data and set
     crc for output data

   - Remove add_crc stub

V5 - Rename pins from generic 'clk', 'data', etc in dts file to
     'fsi_clk', 'fsi_data', etc...

   - serial_in: invert data bit during message assembly
     instead of inverting whole message after assembly.

   - Remove all instances of clearing out message field prior
     to calling serial_in, redundant.

   - Change name of build_command() to build_abs_ar_command()
     to make it more obvious its creating ABS AR type commands.

   - Remove unlikely( ) checks.

   - Indent dev_info string "master time out waiting for response"

   - poll_for_response: return FSI_GPIO_MAX busy instead of
     busy_count.

   - fsi_master_gpio_break: move 'logic reset' comment to above
     the delay call.

   - Add crc4 calculation initialization since data includes
     a start bit.

Signed-off-by: Jeremy Kerr <jk@0zlabs.org>
---
 .../devicetree/bindings/fsi/fsi-master-gpio.txt    |  21 +
 arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts      |  30 ++
 drivers/fsi/Kconfig                                |   7 +
 drivers/fsi/Makefile                               |   1 +
 drivers/fsi/fsi-master-gpio.c                      | 464 +++++++++++++++++++++
 5 files changed, 523 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..4cb2c81
--- /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-gpio;
+	- data-gpio;
+
+Optional properties:
+	- enable-gpio;
+	- trans-gpio;
+	- mux-gpio;
+
+fsi-master {
+	compatible = "ibm,fsi-master", "ibm,fsi-master-gpio";
+	clk-gpio = <&gpio 0>;
+	data-gpio = <&gpio 1>;
+	enable-gpio = <&gpio 2>;
+	trans-gpio = <&gpio 3>;
+	mux-gpio = <&gpio 4>;
+}
diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
index 21619fd..1875313 100644
--- a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
+++ b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
@@ -167,6 +167,36 @@
 		output-low;
 		line-name = "func_mode2";
 	};
+
+	pin_fsi_clk {
+		gpios = <ASPEED_GPIO(A, 4)  GPIO_ACTIVE_HIGH>;
+		output-low;
+		line-name = "fsi_clk";
+	};
+
+	pin_fsi_data {
+		gpios = <ASPEED_GPIO(A, 5)  GPIO_ACTIVE_HIGH>;
+		output-low;
+		line-name = "fsi_data";
+	};
+
+	pin_fsi_trans {
+		gpios = <ASPEED_GPIO(H, 6)  GPIO_ACTIVE_HIGH>;
+		output-low;
+		line-name = "fsi_trans";
+	};
+
+	pin_fsi_enable {
+		gpios = <ASPEED_GPIO(D, 0)  GPIO_ACTIVE_HIGH>;
+		output-low;
+		line-name = "fsi_enable";
+	};
+
+	pin_fsi_mux {
+		gpios = <ASPEED_GPIO(A, 6)  GPIO_ACTIVE_HIGH>;
+		output-low;
+		line-name = "fsi_mux";
+	};
 };
 
 &vuart {
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..a9ee36c
--- /dev/null
+++ b/drivers/fsi/fsi-master-gpio.c
@@ -0,0 +1,464 @@
+/*
+ * 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 "fsi-master.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	/* 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	0x0000808A
+#define	FSI_GPIO_CMD_DPOLL_SIZE	10
+#define	FSI_GPIO_DPOLL_CLOCKS	24      /* < 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	3
+#define	FSI_GPIO_CMD_SIZE_16	0x0000001000000000ULL
+#define	FSI_GPIO_CMD_SIZE_32	0x0000003000000000ULL
+#define	FSI_GPIO_CMD_DATA_SHIFT	28
+#define	FSI_GPIO_CMD_DFLT_LEN	32
+#define	FSI_GPIO_RESP_BUSY	1
+#define	FSI_GPIO_RESP_ERRA	2
+#define	FSI_GPIO_RESP_ERRC	3
+#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_MTOE		1
+#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_CRC_INVAL		5
+
+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 set_clock(struct fsi_master_gpio *master)
+{
+	ndelay(FSI_GPIO_STD_DELAY);
+	gpiod_set_value(master->gpio_clk, 1);
+}
+
+static void clear_clock(struct fsi_master_gpio *master)
+{
+	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);
+	}
+}
+
+static int sda_in(struct fsi_master_gpio *master)
+{
+	return gpiod_get_value(master->gpio_data);
+}
+
+static void sda_out(struct fsi_master_gpio *master, int value)
+{
+	gpiod_set_value(master->gpio_data, value);
+	ndelay(FSI_GPIO_STD_DELAY);
+}
+
+static void set_sda_input(struct fsi_master_gpio *master)
+{
+	gpiod_direction_input(master->gpio_data);
+	if (master->gpio_trans)
+		gpiod_direction_output(master->gpio_trans, 0);
+}
+static void set_sda_output(struct fsi_master_gpio *master,
+				int value)
+{
+	gpiod_direction_output(master->gpio_data, value);
+	if (master->gpio_trans)
+		gpiod_direction_output(master->gpio_trans, 1);
+}
+
+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);
+	cmd->bits = 0;
+
+	for (bit = 0; bit < num_bits; bit++) {
+		clock_toggle(master, 1);
+		in_bit = sda_in(master);
+		cmd->bits++;
+		msg <<= 1;
+		msg |= ~in_bit & 0x1;	/* Data is negative active */
+	}
+	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;
+	uint64_t last_bit = ~0;
+	int next_bit;
+
+	if (!cmd->bits) {
+		dev_warn(master->master.dev, "trying to output 0 bits\n");
+		return;
+	}
+	sda_mask = BIT(cmd->bits - 1);
+	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++) {
+		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, 0);
+	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;
+	uint64_t resp = 0;
+	uint8_t bits_received = 1 + 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);
+			resp = response.msg;
+			if (response.msg)
+				break;
+		}
+		if (i >= FSI_GPIO_MTOE_COUNT) {
+			dev_info(master->master.dev,
+				"Master time out waiting for response\n");
+			drain_response(master);
+			fsi_master_gpio_error(master, FSI_GPIO_MTOE);
+			return FSI_GPIO_MTOE;
+		}
+
+		/* Response received */
+		serial_in(master, &response, FSI_GPIO_MSG_ID_SIZE);
+		dev_info(master->master.dev, "cfam id:%d\n", (int)response.msg);
+		resp <<= FSI_GPIO_MSG_ID_SIZE;
+		resp |= response.msg;
+
+		serial_in(master, &response, FSI_GPIO_MSG_RESPID_SIZE);
+		dev_info(master->master.dev, "response id:%d\n",
+				(int)response.msg);
+		resp <<= FSI_GPIO_MSG_RESPID_SIZE;
+		resp |= response.msg;
+
+		switch (response.msg) {
+		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, 0);
+			clock_toggle(master, FSI_GPIO_DPOLL_CLOCKS);
+			cmd.msg = FSI_GPIO_CMD_DPOLL;
+			cmd.bits = FSI_GPIO_CMD_DPOLL_SIZE;
+			serial_out(master, &cmd);
+			continue;
+
+		case FSI_GPIO_RESP_ERRA:
+		case FSI_GPIO_RESP_ERRC:
+			dev_info(master->master.dev, "ERR received: %d\n",
+				(int)response.msg);
+			drain_response(master);
+			fsi_master_gpio_error(master, response.msg);
+			return response.msg;
+		}
+
+		/* 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;
+		}
+
+		/* Read in the crc and check it */
+		serial_in(master, &response, FSI_GPIO_CRC_SIZE);
+
+		crc_in = fsi_crc4(0, 1, 1);
+		crc_in = fsi_crc4(crc_in, resp, bits_received);
+		if (crc_in != response.msg) {
+			/* CRC's don't match */
+			dev_info(master->master.dev, "ERR response CRC\n");
+			fsi_master_gpio_error(master, FSI_GPIO_CRC_INVAL);
+			return FSI_GPIO_CRC_INVAL;
+		}
+		return 0;
+
+	} while (busy_count++ < FSI_GPIO_MAX_BUSY);
+
+	return FSI_GPIO_MAX_BUSY;
+}
+
+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;
+	uint64_t msg_with_start;
+
+	cmd->bits = FSI_GPIO_CMD_DFLT_LEN;
+	cmd->msg = FSI_GPIO_CMD_DEFAULT;
+	cmd->msg |= mode;
+
+	/* todo: handle more than just slave id 0 */
+	cmd->msg &= ~FSI_GPIO_CMD_SLAVE_MASK;
+
+	cmd->msg |= (addr << FSI_GPIO_CMD_ADDR_SHIFT);
+	if (size == sizeof(uint8_t)) {
+		if (data)
+			cmd->msg |=
+				*((uint8_t *)data) >> FSI_GPIO_CMD_DATA_SHIFT;
+	} else if (size == sizeof(uint16_t)) {
+		cmd->msg |= FSI_GPIO_CMD_SIZE_16;
+		if (data)
+			cmd->msg |=
+				*((uint16_t *)data) >> FSI_GPIO_CMD_DATA_SHIFT;
+	} else {
+		cmd->msg |= FSI_GPIO_CMD_SIZE_32;
+		if (data)
+			cmd->msg |=
+				*((uint32_t *)data) >> FSI_GPIO_CMD_DATA_SHIFT;
+	}
+
+	if (mode == FSI_GPIO_CMD_WRITE)
+		cmd->bits += (8 * size);
+
+	/* Start bit isn't considered part of command but we need to */
+	/* account for it in crc calcs */
+	msg_with_start = 0x1 << cmd->bits;
+	msg_with_start |= cmd->msg;
+	crc = fsi_crc4(0, 1, 1);
+	crc = fsi_crc4(crc, msg_with_start, cmd->bits);
+	cmd->msg |= crc >> cmd->bits;
+	cmd->bits += FSI_GPIO_CRC_SIZE;
+}
+
+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;
+
+	if (link != 0)
+		return -ENODEV;
+
+	build_abs_ar_command(&cmd, FSI_GPIO_CMD_READ, slave, addr, size, NULL);
+	serial_out(master, &cmd);
+	echo_delay(master);
+
+	return poll_for_response(master, FSI_GPIO_RESP_ACKD, size, val);
+}
+
+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;
+
+	build_abs_ar_command(&cmd, FSI_GPIO_CMD_WRITE, slave, addr, size, val);
+	serial_out(master, &cmd);
+	echo_delay(master);
+
+	return poll_for_response(master, FSI_GPIO_RESP_ACK, size, NULL);
+}
+
+/*
+ * 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, 0);
+	clock_toggle(master, FSI_PRE_BREAK_CLOCKS);
+	sda_out(master, 1);
+	clock_toggle(master, FSI_BREAK_CLOCKS);
+	echo_delay(master);
+	sda_out(master, 0);
+	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);
+	gpiod_direction_output(master->gpio_clk, 1);
+	set_sda_output(master, 1);
+	if (master->gpio_enable)
+		gpiod_direction_output(master->gpio_enable, 0);
+
+	/* 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 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, "fsi_clk", 0);
+	if (IS_ERR(gpio))
+		return PTR_ERR(gpio);
+	master->gpio_clk = gpio;
+
+	gpio = devm_gpiod_get(&pdev->dev, "fsi_data", 0);
+	if (IS_ERR(gpio))
+		return PTR_ERR(gpio);
+	master->gpio_data = gpio;
+
+	/* Optional pins */
+
+	gpio = devm_gpiod_get(&pdev->dev, "fsi_trans", 0);
+	if (!IS_ERR(gpio))
+		master->gpio_trans = gpio;
+
+	gpio = devm_gpiod_get(&pdev->dev, "fsi_enable", 0);
+	if (!IS_ERR(gpio))
+		master->gpio_enable = gpio;
+
+	gpio = devm_gpiod_get(&pdev->dev, "fsi_mux", 0);
+	if (!IS_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.link_enable = fsi_master_gpio_link_enable;
+
+	fsi_master_gpio_init(master);
+
+	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 v5 16/18] drivers/fsi: Add client driver register utilities

[christopher.lee.bostic]

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

Add driver_register and driver_unregister wrappers for FSI.

V4 - Separate out SCOM client driver registration for a follow
     on patch.

V5 - Rename fsidrv_register, fsidrv_unregster to fsi_drv_register,
     fsi_drv_unregister.

   - Add module_fsi_driver() macro for clients that just need
     to do boilerplate registration

Signed-off-by: Chris Bostic <cbostic@us.ibm.com>
---
 drivers/fsi/fsi-core.c | 17 +++++++++++++++++
 include/linux/fsi.h    | 12 ++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index ed0d20f..704c473 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -456,6 +456,23 @@ static int fsi_bus_match(struct device *dev, struct device_driver *drv)
 	return 0;
 }
 
+int fsi_drv_register(struct fsi_driver *fsi_drv)
+{
+	if (!fsi_drv)
+		return -EINVAL;
+	if (!fsi_drv->id_table)
+		return -EINVAL;
+
+	return driver_register(&fsi_drv->drv);
+}
+EXPORT_SYMBOL_GPL(fsi_drv_register);
+
+void fsi_drv_unregister(struct fsi_driver *fsi_drv)
+{
+	driver_unregister(&fsi_drv->drv);
+}
+EXPORT_SYMBOL_GPL(fsi_drv_unregister);
+
 struct bus_type fsi_bus_type = {
 	.name		= "fsi",
 	.match		= fsi_bus_match,
diff --git a/include/linux/fsi.h b/include/linux/fsi.h
index 47af0af..3235dcc 100644
--- a/include/linux/fsi.h
+++ b/include/linux/fsi.h
@@ -55,6 +55,18 @@ struct fsi_driver {
 #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 int fsi_drv_register(struct fsi_driver *);
+extern void fsi_drv_unregister(struct fsi_driver *);
+
+/* module_fsi_driver() - Helper macro for drivers that don't do
+ * anything special in module init/exit.  This eliminates a lot of
+ * boilerplate.   Each module may only use this macro once, and
+ * calling it replaces module_init() and module_exit()
+ */
+#define module_fsi_driver(__fsi_driver) \
+	module_driver(__fsi_driver, fsi_drv_register, \
+			fsi_drv_unregister)
+
 extern struct bus_type fsi_bus_type;
 
 #endif /* LINUX_FSI_H */
-- 
1.8.2.2

[PATCH linux v5 17/18] drivers/fsi: Add SCOM FSI client device driver

[christopher.lee.bostic]

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

Create a simple SCOM engine device driver that reads and writes
across an FSI bus.

V4 - Add put_scom and get_scom operations

V5 - Add character device registration and fill in read/write
     syscalls.

Signed-off-by: Chris Bostic <cbostic@us.ibm.com>
---
 drivers/fsi/Kconfig    |   6 ++
 drivers/fsi/Makefile   |   1 +
 drivers/fsi/fsi-scom.c | 217 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 224 insertions(+)
 create mode 100644 drivers/fsi/fsi-scom.c

diff --git a/drivers/fsi/Kconfig b/drivers/fsi/Kconfig
index 69e7ee8..719a217 100644
--- a/drivers/fsi/Kconfig
+++ b/drivers/fsi/Kconfig
@@ -24,6 +24,12 @@ config FSI_MASTER_GPIO
 	select GPIO_DEVRES
 	---help---
 	This option enables a FSI master driver, using GPIO lines directly.
+
+config FSI_SCOM
+	tristate "SCOM FSI client"
+	depends on FSI
+	---help---
+	This option enables the SCOM FSI client device driver.
 endif
 
 endmenu
diff --git a/drivers/fsi/Makefile b/drivers/fsi/Makefile
index 2021ce5..3e31d9a 100644
--- a/drivers/fsi/Makefile
+++ b/drivers/fsi/Makefile
@@ -2,3 +2,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
+obj-$(CONFIG_FSI_SCOM) += fsi-scom.o
diff --git a/drivers/fsi/fsi-scom.c b/drivers/fsi/fsi-scom.c
new file mode 100644
index 0000000..5ee12a9
--- /dev/null
+++ b/drivers/fsi/fsi-scom.c
@@ -0,0 +1,217 @@
+/*
+ * SCOM FSI Client device 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
+ * MERGCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#include <linux/fsi.h>
+#include <linux/module.h>
+#include <linux/cdev.h>
+#include <linux/delay.h>
+#include <linux/fs.h>
+#include <linux/uaccess.h>
+
+#define FSI_ENGID_SCOM		0x5
+
+#define SCOM_FSI2PIB_DELAY	50
+
+/* SCOM engine register set */
+#define SCOM_DATA0_REG		0x00
+#define SCOM_DATA1_REG		0x04
+#define SCOM_CMD_REG		0x08
+#define SCOM_RESET_REG		0x1C
+
+#define SCOM_RESET_CMD		0x80000000
+#define SCOM_WRITE_CMD		0x80000000
+
+static int scom_probe(struct device *);
+
+struct scom_op {
+	uint64_t	data;
+	uint32_t	addr;
+};
+
+struct scom_device {
+	struct fsi_device *fsi_dev;
+	struct cdev *cdev;
+};
+
+struct scom_device scom_dev;
+
+struct fsi_device_id scom_ids[] = {
+	{
+		.engine_type = FSI_ENGID_SCOM,
+		.version = FSI_VERSION_ANY,
+	},
+	{ 0 }
+};
+
+struct fsi_driver scom_drv = {
+	.id_table = scom_ids,
+	.drv = {
+		.name = "scom",
+		.bus = &fsi_bus_type,
+		.probe = scom_probe,
+	}
+};
+
+static int put_scom(uint64_t value, uint32_t addr)
+{
+	int rc;
+	uint32_t data = SCOM_RESET_CMD;
+	struct scom_device *scom = &scom_dev;
+
+	rc = fsi_device_write(scom->fsi_dev, SCOM_RESET_REG, &data,
+				sizeof(uint32_t));
+	if (rc)
+		return rc;
+
+	data = (value >> 32) & 0xffffffff;
+	rc = fsi_device_write(scom->fsi_dev, SCOM_DATA0_REG, &data,
+				sizeof(uint32_t));
+	if (rc)
+		return rc;
+
+	data = value & 0xffffffff;
+	rc = fsi_device_write(scom->fsi_dev, SCOM_DATA1_REG, &data,
+				sizeof(uint32_t));
+	if (rc)
+		return rc;
+
+	data = SCOM_WRITE_CMD | addr;
+	return fsi_device_write(scom->fsi_dev, SCOM_CMD_REG, &data,
+				sizeof(uint32_t));
+}
+
+
+static int get_scom(uint64_t *value, uint32_t addr)
+{
+	uint32_t result, data;
+	struct scom_device *scom = &scom_dev;
+	int rc;
+
+	udelay(SCOM_FSI2PIB_DELAY);
+
+	data = addr;
+	rc = fsi_device_write(scom->fsi_dev, SCOM_CMD_REG, &data,
+				sizeof(uint32_t));
+	if (rc)
+		return rc;
+
+	rc = fsi_device_read(scom->fsi_dev, SCOM_DATA0_REG, &result,
+				sizeof(uint32_t));
+	if (rc)
+		return rc;
+
+	*value |= (uint64_t) result << 32;
+	rc = fsi_device_read(scom->fsi_dev, SCOM_DATA1_REG, &result,
+				sizeof(uint32_t));
+	if (rc)
+		return rc;
+
+	*value |= result;
+
+	return 0;
+}
+
+static ssize_t scom_read(struct file *filep, char __user *buf, size_t len,
+			loff_t *offset)
+{
+	int rc = 0;
+	struct scom_op op;
+	struct scom_device *scom = &scom_dev;
+	struct device *dev = &scom->fsi_dev->dev;
+
+	/* todo: Retrieve device to operate on from filep->private_data */
+
+	if (len != sizeof(struct scom_op))
+		return -EINVAL;
+
+	rc = copy_from_user(&op, buf, len);
+	if (rc) {
+		dev_dbg(dev, "copy from user failed:%d\n", rc);
+		return -EINVAL;
+	}
+
+	rc = get_scom(&op.data, op.addr);
+	if (rc) {
+		dev_dbg(dev, "get_scom failed with:%d\n", rc);
+		return -EINVAL;
+	}
+
+	rc = copy_to_user(buf, &op, sizeof(struct scom_op));
+	if (rc) {
+		dev_dbg(dev, "copy to user failed:%d\n", rc);
+		return -EIO;
+	}
+
+	return len;
+}
+
+static ssize_t scom_write(struct file *filep, const char __user *buf,
+			size_t len, loff_t *offset)
+{
+	int rc = 0;
+	struct scom_op op;
+	struct scom_device *scom = &scom_dev;
+	struct device *dev = &scom->fsi_dev->dev;
+
+	/* todo: Retrieve device to operate on from filep->private_data */
+
+	if (len != sizeof(struct scom_op))
+		return -EINVAL;
+
+	rc = copy_from_user(&op, buf, sizeof(uint64_t));
+	if (rc) {
+		dev_dbg(dev, "copy from user failed:%d\n", rc);
+		return -EINVAL;
+	}
+
+	rc = put_scom(op.data, op.addr);
+	if (rc) {
+		dev_dbg(dev, "put_scom failed with:%d\n", rc);
+		return -EIO;
+	}
+
+	return len;
+}
+
+static const struct file_operations scom_fops = {
+	.owner	= THIS_MODULE,
+	.read	= scom_read,
+	.write	= scom_write,
+};
+
+static int scom_probe(struct device *dev)
+{
+	struct fsi_device *fsi_dev = to_fsi_dev(dev);
+	struct scom_device *scom = &scom_dev;
+	int rc;
+
+	scom->fsi_dev = fsi_dev;
+	scom->cdev = cdev_alloc();
+	if (!scom->cdev)
+		return -ENOMEM;
+
+	scom->cdev->owner = THIS_MODULE;
+
+	rc = register_chrdev(0, "scom", &scom_fops);
+	if (rc < 0) {
+		dev_dbg(dev, "scom major allocation failed:%d\n", rc);
+		return rc;
+	}
+	dev_dbg(dev, "scom major allocated:%d\n", rc);
+
+	return 0;
+}
+
+module_fsi_driver(scom_drv);
-- 
1.8.2.2

[PATCH linux v5 18/18] Documenation: Add basic FSI text file

[christopher.lee.bostic]

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

Add documentation explaining the basics of FSI and how to find
more information.

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

diff --git a/Documentation/fsi.txt b/Documentation/fsi.txt
new file mode 100644
index 0000000..b8c94a0
--- /dev/null
+++ b/Documentation/fsi.txt
@@ -0,0 +1,36 @@
+The Flexible Support Interface (FSI) is a two wire bus consisting of
+a serial data line and a clock capable of running at up to 166 MHz.
+FSI features strong data integrity with CRC protection and is capable
+of very high fan out.
+
+The complete hardware specification[1] for FSI is publicly available on
+the OpenPower Foundation website:
+
+[1] http://openpowerfoundation.org/wp-content/uploads/resources/OpenFSI-spec/content/ch_preface.html
+
+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 / Configuration 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 contains, as well as its addressing range.
+
+* FSI Client driver
+    A device driver that registers with the FSI core so that it can access
+    devices it owns on an FSI bus.
-- 
1.8.2.2

Re: [PATCH linux v5 13/18] drivers/fsi: Set up links for slave communication

[Jeremy Kerr]

Hi Chris,

> Enable each link and send a break command in preparation
> for scanning each link for slaves.

Looks good!

Acked-by: Jeremy Kerr <jk@ozlabs.org>

> V3 - Remove definition of BREAK command from fsi-master.h

In general, You'll want to move the V3/V4 changelog to after your
Signed-off-by, and the '---' line. So you have:

<comments>

Signed-off-by:
Acked-by/Tested-by/etc.

---

<version changes>

---

<diffstat & patch>

This means that `git am` will do the right thing and remove the version
changes before applying (as we don't need that in the git tree once
merged).

Cheers,


Jeremy

Re: [PATCH linux v5 13/18] drivers/fsi: Set up links for slave communication

[Joel Stanley]

Hey Chris,

On Thu, Oct 20, 2016 at 10:09 AM, Jeremy Kerr <jk@ozlabs.org> wrote:
>> Enable each link and send a break command in preparation
>> for scanning each link for slaves.
>
> Looks good!
>
> Acked-by: Jeremy Kerr <jk@ozlabs.org>

Woot! Your first ack. This is a great milestone. Congratulations to
you and Jeremy for your persistence in getting to this point.

You can add the ack to the commit message of this patch (providing you
don't change the code again).

I was wondering, is this good to go on hardware? If so, I will fire up
a palmetto and give it a spin.

Cheers,

Joel

Re: [PATCH linux v5 14/18] drivers/fsi: Set slave SMODE to init communications

[Jeremy Kerr]

Hi Chris,

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

Looks good, one minor thing:

> @@ -329,6 +400,17 @@ static int fsi_master_scan(struct fsi_master *master)
>  				"Break to link:%d failed with:%d\n", link, rc);
>  			continue;
>  		}
> +		/* Verify can read slave at default ID location. If fails */
> +		/* then there must be nothing on other end of link */

Best to do this in a single comment. However, don't do a v6 just for
this, as it's really insignificant. Only re-roll this patch if there are
other changes required.

Acked-by: Jeremy Kerr <jk@ozlabs.org>

Cheers,


Jeremy

Re: [PATCH linux v5 15/18] drivers/fsi: Add GPIO FSI master

[Jeremy Kerr]

Hi Chris,

> 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..4cb2c81
> --- /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-gpio;
> +	- data-gpio;
> +
> +Optional properties:
> +	- enable-gpio;
> +	- trans-gpio;
> +	- mux-gpio;
> +
> +fsi-master {
> +	compatible = "ibm,fsi-master", "ibm,fsi-master-gpio";
> +	clk-gpio = <&gpio 0>;
> +	data-gpio = <&gpio 1>;
> +	enable-gpio = <&gpio 2>;
> +	trans-gpio = <&gpio 3>;
> +	mux-gpio = <&gpio 4>;
> +}

Looks good. Might be good to include descriptions for the
enable/trans/mux lines here.

We should also think about what we want to do for multiple links. There
are two main options here:

  - list them as separate masters in the DT (ie, two fsi-master nodes,
    completely independent); or

  - enable multiple GPIO descriptors in the gpio properties, eg:

    fsi-master {
	compatible = "ibm,fsi-master", "ibm,fsi-master-gpio";
	clk-gpios = <&gpio 0 &gpio 6>;
	data-gpios = <&gpio 1 &gpio 7>;
    }

    which describes a master with two links

If we use the latter case, we'd want the property name to be plural
(*-gpios) to indicate this.

> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
> index 21619fd..1875313 100644
> --- a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
> @@ -167,6 +167,36 @@
>  		output-low;
>  		line-name = "func_mode2";
>  	};
> +
> +	pin_fsi_clk {
> +		gpios = <ASPEED_GPIO(A, 4)  GPIO_ACTIVE_HIGH>;
> +		output-low;
> +		line-name = "fsi_clk";
> +	};
> +
> +	pin_fsi_data {
> +		gpios = <ASPEED_GPIO(A, 5)  GPIO_ACTIVE_HIGH>;
> +		output-low;
> +		line-name = "fsi_data";
> +	};
> +
> +	pin_fsi_trans {
> +		gpios = <ASPEED_GPIO(H, 6)  GPIO_ACTIVE_HIGH>;
> +		output-low;
> +		line-name = "fsi_trans";
> +	};
> +
> +	pin_fsi_enable {
> +		gpios = <ASPEED_GPIO(D, 0)  GPIO_ACTIVE_HIGH>;
> +		output-low;
> +		line-name = "fsi_enable";
> +	};
> +
> +	pin_fsi_mux {
> +		gpios = <ASPEED_GPIO(A, 6)  GPIO_ACTIVE_HIGH>;
> +		output-low;
> +		line-name = "fsi_mux";
> +	};
>  };

Do we want to add a master node too?

> +static void set_clock(struct fsi_master_gpio *master)
> +{
> +	ndelay(FSI_GPIO_STD_DELAY);
> +	gpiod_set_value(master->gpio_clk, 1);
> +}
> +
> +static void clear_clock(struct fsi_master_gpio *master)
> +{
> +	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);
> +	}
> +}

To me, this seems a bit awkward, since the set_clock and clear_clock
have their pre-delays, but are only used in one context (clock_toggle).
Perhaps this would be cleaner just doing the gpiod_set_value()s and
delays in clock_toggle would be cleaner?

> +
> +static int sda_in(struct fsi_master_gpio *master)
> +{
> +	return gpiod_get_value(master->gpio_data);
> +}
> +
> +static void sda_out(struct fsi_master_gpio *master, int value)
> +{
> +	gpiod_set_value(master->gpio_data, value);
> +	ndelay(FSI_GPIO_STD_DELAY);
> +}

So an output bit that changes sda will take twice as long? Won't the
delays in clock_toggle handle this?

> +
> +static void set_sda_input(struct fsi_master_gpio *master)
> +{
> +	gpiod_direction_input(master->gpio_data);
> +	if (master->gpio_trans)
> +		gpiod_direction_output(master->gpio_trans, 0);
> +}
> +static void set_sda_output(struct fsi_master_gpio *master,
> +				int value)
> +{
> +	gpiod_direction_output(master->gpio_data, value);
> +	if (master->gpio_trans)
> +		gpiod_direction_output(master->gpio_trans, 1);
> +}

Isn't trans always an output? Shouldn't we set the direction to output
once during init, and do gpiod_set_value() here?

> +
> +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);
> +	cmd->bits = 0;
> +
> +	for (bit = 0; bit < num_bits; bit++) {
> +		clock_toggle(master, 1);
> +		in_bit = sda_in(master);
> +		cmd->bits++;
> +		msg <<= 1;
> +		msg |= ~in_bit & 0x1;	/* Data is negative active */
> +	}
> +	cmd->msg = msg;
> +}

You can shortcut all of the increments of ->num_bits (and initialisation
to zero) by just setting it to num_bits (probably after the for-loop,
where you set ->msg);

> +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;
> +	uint64_t resp = 0;
> +	uint8_t bits_received = 1 + 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);
> +			resp = response.msg;
> +			if (response.msg)
> +				break;
> +		}
> +		if (i >= FSI_GPIO_MTOE_COUNT) {
> +			dev_info(master->master.dev,
> +				"Master time out waiting for response\n");
> +			drain_response(master);
> +			fsi_master_gpio_error(master, FSI_GPIO_MTOE);
> +			return FSI_GPIO_MTOE;
> +		}
> +
> +		/* Response received */
> +		serial_in(master, &response, FSI_GPIO_MSG_ID_SIZE);
> +		dev_info(master->master.dev, "cfam id:%d\n", (int)response.msg);
> +		resp <<= FSI_GPIO_MSG_ID_SIZE;
> +		resp |= response.msg;
> +
> +		serial_in(master, &response, FSI_GPIO_MSG_RESPID_SIZE);
> +		dev_info(master->master.dev, "response id:%d\n",
> +				(int)response.msg);
> +		resp <<= FSI_GPIO_MSG_RESPID_SIZE;
> +		resp |= response.msg;

Those dev_info() calls are going to make things very noisy. For
printouts in non-error paths, use dev_dbg().

However, you have two serial_in() calls in the single path, can you
consolidate these?

> +
> +		switch (response.msg) {
> +		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, 0);
> +			clock_toggle(master, FSI_GPIO_DPOLL_CLOCKS);
> +			cmd.msg = FSI_GPIO_CMD_DPOLL;
> +			cmd.bits = FSI_GPIO_CMD_DPOLL_SIZE;
> +			serial_out(master, &cmd);
> +			continue;
> +
> +		case FSI_GPIO_RESP_ERRA:
> +		case FSI_GPIO_RESP_ERRC:
> +			dev_info(master->master.dev, "ERR received: %d\n",
> +				(int)response.msg);
> +			drain_response(master);
> +			fsi_master_gpio_error(master, response.msg);
> +			return response.msg;
> +		}
> +
> +		/* 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;
> +		}
> +
> +		/* Read in the crc and check it */
> +		serial_in(master, &response, FSI_GPIO_CRC_SIZE);
> +
> +		crc_in = fsi_crc4(0, 1, 1);
> +		crc_in = fsi_crc4(crc_in, resp, bits_received);
> +		if (crc_in != response.msg) {
> +			/* CRC's don't match */
> +			dev_info(master->master.dev, "ERR response CRC\n");
> +			fsi_master_gpio_error(master, FSI_GPIO_CRC_INVAL);
> +			return FSI_GPIO_CRC_INVAL;
> +		}
> +		return 0;
> +
> +	} while (busy_count++ < FSI_GPIO_MAX_BUSY);
> +
> +	return FSI_GPIO_MAX_BUSY;
> +}

While you've explained the return values to me, it isn't obvious from
the code. From reading the poll_response:

 - we return 0 on success

 - we return 1 if we don't see a start bit (what's MTOE?)

 - we return 100 if we got 100 busy responses (after dpoll)

 - we return 2 or 3 if there was a checksum error on the slave

 - we return 5 if there was a checksum error on the master

Now, this *may* be okay if we were interpreting the return values within
this one file (although we'd want to change busy into something more in
line with the others), but this is returned directly to the fsi core
through the read & write callbacks.

Without any explicit definition of the return values, we'd expect a
standard kernel scheme (0 on success, negative errno (eg -EIO) on
failure), particularly on the master->read/write interface.

I assume you're going to handle the various return values as part
of the future error recovery changes, but we should at least (at this
stage) compress this into -EIO from the read/write callbacks (possibly
in the actual read/write implementations).

In future, when we have conditions that we can potentially recover from,
I'd suggest using an enum to represent the full set of errors, and
define this function to return that type. That allows us to describe the
possible set of errors to future developers, and allows the compiler to
warn us if we're not handling a particular case.

Until the error recovery code is added, we won't know what the best
strategy is - but in the meantime, we should stick to the
zero/negative-errno convention.

> +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, "fsi_clk", 0);

You've changed these unnecessarily; this name string is used to look up
the device tree property, not the pinctl descriptor.

Cheers,


Jeremy

Re: [PATCH linux v5 16/18] drivers/fsi: Add client driver register utilities

[Jeremy Kerr]

Hi Chris,

> +int fsi_drv_register(struct fsi_driver *fsi_drv)

Not fsi_driver_register()?

Cheers,


Jeremy

Re: [PATCH linux v5 18/18] Documenation: Add basic FSI text file

[Jeremy Kerr]

Hi Chris,

> Add documentation explaining the basics of FSI and how to find
> more information.
> +The Flexible Support Interface (FSI) is a two wire bus consisting of
> +a serial data line and a clock capable of running at up to 166 MHz.
> +FSI features strong data integrity with CRC protection and is capable
> +of very high fan out.
> +
> +The complete hardware specification[1] for FSI is publicly available on
> +the OpenPower Foundation website:
> +
> +[1] http://openpowerfoundation.org/wp-content/uploads/resources/OpenFSI-spec/content/ch_preface.html
> +
> +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 / Configuration 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 contains, as well as its addressing range.
> +
> +* FSI Client driver
> +    A device driver that registers with the FSI core so that it can access
> +    devices it owns on an FSI bus.

Is "client" something new, or is this standard FSI terminology? I would
have thought "engine" is the term used elsewhere.

Anyway, here's a little more to tie it into the kernel implementation:

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

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. Client drivers are matched and bound to these engines during
discovery.

------------

We may want to add some description of the APIs, but that should do for
now.

Cheers,


Jeremy

Re: [PATCH linux v5 17/18] drivers/fsi: Add SCOM FSI client device driver

[Jeremy Kerr]

Hi Chris,

> diff > +static int scom_probe(struct device *);
> +
> +struct scom_op {
> +	uint64_t	data;
> +	uint32_t	addr;
> +};

You're making this a userspace interface, so this should go into the
uapi headers (and then will form part of the unchangeable Linux ABI).
However, more on the ABI below...

> +struct scom_device {
> +	struct fsi_device *fsi_dev;
> +	struct cdev *cdev;
> +};
> +
> +struct scom_device scom_dev;
> +
> +struct fsi_device_id scom_ids[] = {
> +	{
> +		.engine_type = FSI_ENGID_SCOM,
> +		.version = FSI_VERSION_ANY,
> +	},
> +	{ 0 }
> +};
> +
> +struct fsi_driver scom_drv = {
> +	.id_table = scom_ids,
> +	.drv = {
> +		.name = "scom",
> +		.bus = &fsi_bus_type,
> +		.probe = scom_probe,
> +	}
> +};

Make these static, and put after the code, just before the
module_fsi_driver. Then you won't need the forward definition of
scom_probe.

> > +static ssize_t scom_read(struct file *filep, char __user *buf, size_t len,
> +			loff_t *offset)
> +{
> +	int rc = 0;
> +	struct scom_op op;
> +	struct scom_device *scom = &scom_dev;
> +	struct device *dev = &scom->fsi_dev->dev;
> +
> +	/* todo: Retrieve device to operate on from filep->private_data */

Yep, otherwise you only get one scom interface, and crazy results if you
get probed twice. I'd suggest not leaving this until later.

> +
> +	if (len != sizeof(struct scom_op))
> +		return -EINVAL;
> +
> +	rc = copy_from_user(&op, buf, len);
> +	if (rc) {
> +		dev_dbg(dev, "copy from user failed:%d\n", rc);
> +		return -EINVAL;
> +	}
> +
> +	rc = get_scom(&op.data, op.addr);

I'm not a huge fan of that ABI to userspace. You already have the
address in the offset argument, so you can do this without needing new
uapi headers, using something like:

static ssize_t scom_read(struct file *filep, char __user *buf, size_t len,
			loff_t *offset)
{
	uint64_t val;

	/* todo: can we do 1/2/4/8 byte operations too? */
	if (len != sizeof(uint64_t))
		return -EINVAL;

	rc = get_scom(&val, offset);
	if (rc)
		return rc;

	rc = copy_to_user(&val, buf, len);

	return rc ? rc : len;
}


Also, have you thought about endianness?
	

> +static int scom_probe(struct device *dev)
> +{
> +	struct fsi_device *fsi_dev = to_fsi_dev(dev);
> +	struct scom_device *scom = &scom_dev;
> +	int rc;
> +
> +	scom->fsi_dev = fsi_dev;
> +	scom->cdev = cdev_alloc();
> +	if (!scom->cdev)
> +		return -ENOMEM;
> +
> +	scom->cdev->owner = THIS_MODULE;
> +
> +	rc = register_chrdev(0, "scom", &scom_fops);
> +	if (rc < 0) {
> +		dev_dbg(dev, "scom major allocation failed:%d\n", rc);
> +		return rc;
> +	}
> +	dev_dbg(dev, "scom major allocated:%d\n", rc);

You need to register the actual device too, right? We'll want scom0,
scom1, etc.

Cheers,


Jeremy

Re: [PATCH linux v5 13/18] drivers/fsi: Set up links for slave communication

[Christopher Bostic]

On Wed, Oct 19, 2016 at 7:10 PM, Joel Stanley <joel@jms.id.au> wrote:
> Hey Chris,
>
> On Thu, Oct 20, 2016 at 10:09 AM, Jeremy Kerr <jk@ozlabs.org> wrote:
>>> Enable each link and send a break command in preparation
>>> for scanning each link for slaves.
>>
>> Looks good!
>>
>> Acked-by: Jeremy Kerr <jk@ozlabs.org>
>
> Woot! Your first ack. This is a great milestone. Congratulations to
> you and Jeremy for your persistence in getting to this point.
>
> You can add the ack to the commit message of this patch (providing you
> don't change the code again).
>

Hi Joel,

The only thing I'd need to do is modify the commit message to relocate the
version history log.  Would that prevent me from adding an ack at this
point?

> I was wondering, is this good to go on hardware? If so, I will fire up
> a palmetto and give it a spin.
>

This code as it exists in the patch set hasn't been fully tested.  I'm
in process
of doing that now.

Thanks,
Chris

> Cheers,
>
> Joel

Re: [PATCH linux v5 15/18] drivers/fsi: Add GPIO FSI master

[Christopher Bostic]

On Wed, Oct 19, 2016 at 8:26 PM, Jeremy Kerr <jk@ozlabs.org> wrote:
> Hi Chris,
>
>> 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..4cb2c81
>> --- /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-gpio;
>> +     - data-gpio;
>> +
>> +Optional properties:
>> +     - enable-gpio;
>> +     - trans-gpio;
>> +     - mux-gpio;
>> +
>> +fsi-master {
>> +     compatible = "ibm,fsi-master", "ibm,fsi-master-gpio";
>> +     clk-gpio = <&gpio 0>;
>> +     data-gpio = <&gpio 1>;
>> +     enable-gpio = <&gpio 2>;
>> +     trans-gpio = <&gpio 3>;
>> +     mux-gpio = <&gpio 4>;
>> +}
>
> Looks good. Might be good to include descriptions for the
> enable/trans/mux lines here.
>

Hi Jeremy,

OK, will add that.

> We should also think about what we want to do for multiple links. There
> are two main options here:
>
>   - list them as separate masters in the DT (ie, two fsi-master nodes,
>     completely independent); or
>
>   - enable multiple GPIO descriptors in the gpio properties, eg:
>
>     fsi-master {
>         compatible = "ibm,fsi-master", "ibm,fsi-master-gpio";
>         clk-gpios = <&gpio 0 &gpio 6>;
>         data-gpios = <&gpio 1 &gpio 7>;
>     }
>
>     which describes a master with two links
>
> If we use the latter case, we'd want the property name to be plural
> (*-gpios) to indicate this.

Its difficult to say how many links we'll eventually need a priori.
This assumes
two links but it could eventually be way more than that.  To be extensible
and prevent us from having to modify the device tree later I'd lean towards
the idea of having separate masters where each owns its own single FSI
link.  We're still left with the issue of guessing how many we'll eventually
need.  How problematic is it to define one gpio master now and then add
others to the device tree later?  If we need to do it now we'll have to pick
a number out of a hat.

>
>> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
>> index 21619fd..1875313 100644
>> --- a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
>> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
>> @@ -167,6 +167,36 @@
>>               output-low;
>>               line-name = "func_mode2";
>>       };
>> +
>> +     pin_fsi_clk {
>> +             gpios = <ASPEED_GPIO(A, 4)  GPIO_ACTIVE_HIGH>;
>> +             output-low;
>> +             line-name = "fsi_clk";
>> +     };
>> +
>> +     pin_fsi_data {
>> +             gpios = <ASPEED_GPIO(A, 5)  GPIO_ACTIVE_HIGH>;
>> +             output-low;
>> +             line-name = "fsi_data";
>> +     };
>> +
>> +     pin_fsi_trans {
>> +             gpios = <ASPEED_GPIO(H, 6)  GPIO_ACTIVE_HIGH>;
>> +             output-low;
>> +             line-name = "fsi_trans";
>> +     };
>> +
>> +     pin_fsi_enable {
>> +             gpios = <ASPEED_GPIO(D, 0)  GPIO_ACTIVE_HIGH>;
>> +             output-low;
>> +             line-name = "fsi_enable";
>> +     };
>> +
>> +     pin_fsi_mux {
>> +             gpios = <ASPEED_GPIO(A, 6)  GPIO_ACTIVE_HIGH>;
>> +             output-low;
>> +             line-name = "fsi_mux";
>> +     };
>>  };
>
> Do we want to add a master node too?
>

Don't follow what you mean by a master node.  What does that help with?
Also how would that be defined in the dev tree?   The discussion above
about choosing the number of masters we'll eventually need would
impact this as well I assume.

>> +static void set_clock(struct fsi_master_gpio *master)
>> +{
>> +     ndelay(FSI_GPIO_STD_DELAY);
>> +     gpiod_set_value(master->gpio_clk, 1);
>> +}
>> +
>> +static void clear_clock(struct fsi_master_gpio *master)
>> +{
>> +     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);
>> +     }
>> +}
>
> To me, this seems a bit awkward, since the set_clock and clear_clock
> have their pre-delays, but are only used in one context (clock_toggle).
> Perhaps this would be cleaner just doing the gpiod_set_value()s and
> delays in clock_toggle would be cleaner?

OK, will change.

>
>> +
>> +static int sda_in(struct fsi_master_gpio *master)
>> +{
>> +     return gpiod_get_value(master->gpio_data);
>> +}
>> +
>> +static void sda_out(struct fsi_master_gpio *master, int value)
>> +{
>> +     gpiod_set_value(master->gpio_data, value);
>> +     ndelay(FSI_GPIO_STD_DELAY);
>> +}
>
> So an output bit that changes sda will take twice as long? Won't the
> delays in clock_toggle handle this?

Yes that's true, the delays in clock_toggle would be enough I think.
Will change.

>
>> +
>> +static void set_sda_input(struct fsi_master_gpio *master)
>> +{
>> +     gpiod_direction_input(master->gpio_data);
>> +     if (master->gpio_trans)
>> +             gpiod_direction_output(master->gpio_trans, 0);
>> +}
>> +static void set_sda_output(struct fsi_master_gpio *master,
>> +                             int value)
>> +{
>> +     gpiod_direction_output(master->gpio_data, value);
>> +     if (master->gpio_trans)
>> +             gpiod_direction_output(master->gpio_trans, 1);
>> +}
>
> Isn't trans always an output? Shouldn't we set the direction to output
> once during init, and do gpiod_set_value() here?
>

Will change.

>> +
>> +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);
>> +     cmd->bits = 0;
>> +
>> +     for (bit = 0; bit < num_bits; bit++) {
>> +             clock_toggle(master, 1);
>> +             in_bit = sda_in(master);
>> +             cmd->bits++;
>> +             msg <<= 1;
>> +             msg |= ~in_bit & 0x1;   /* Data is negative active */
>> +     }
>> +     cmd->msg = msg;
>> +}
>
> You can shortcut all of the increments of ->num_bits (and initialisation
> to zero) by just setting it to num_bits (probably after the for-loop,
> where you set ->msg);
>

Will change.

>> +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;
>> +     uint64_t resp = 0;
>> +     uint8_t bits_received = 1 + 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);
>> +                     resp = response.msg;
>> +                     if (response.msg)
>> +                             break;
>> +             }
>> +             if (i >= FSI_GPIO_MTOE_COUNT) {
>> +                     dev_info(master->master.dev,
>> +                             "Master time out waiting for response\n");
>> +                     drain_response(master);
>> +                     fsi_master_gpio_error(master, FSI_GPIO_MTOE);
>> +                     return FSI_GPIO_MTOE;
>> +             }
>> +
>> +             /* Response received */
>> +             serial_in(master, &response, FSI_GPIO_MSG_ID_SIZE);
>> +             dev_info(master->master.dev, "cfam id:%d\n", (int)response.msg);
>> +             resp <<= FSI_GPIO_MSG_ID_SIZE;
>> +             resp |= response.msg;
>> +
>> +             serial_in(master, &response, FSI_GPIO_MSG_RESPID_SIZE);
>> +             dev_info(master->master.dev, "response id:%d\n",
>> +                             (int)response.msg);
>> +             resp <<= FSI_GPIO_MSG_RESPID_SIZE;
>> +             resp |= response.msg;
>
> Those dev_info() calls are going to make things very noisy. For
> printouts in non-error paths, use dev_dbg().
>

OK will change,

> However, you have two serial_in() calls in the single path, can you
> consolidate these?
>

Ok, will change.

This type of change I would agree offers some minor performance
gain but it does add a few lines of code to decode the slave id that weren't
needed before.  Since I'm making changes to this file already it makes
sense to add this one.   If all that remain though are minor changes like
this one I'd like to be able to address those after the first core patch set
is approved.   Would this be acceptable?  I agree with the desire to get
it all right the first time but I'm also concerned with getting the openfsi
code available for bringup purposes.

>> +
>> +             switch (response.msg) {
>> +             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, 0);
>> +                     clock_toggle(master, FSI_GPIO_DPOLL_CLOCKS);
>> +                     cmd.msg = FSI_GPIO_CMD_DPOLL;
>> +                     cmd.bits = FSI_GPIO_CMD_DPOLL_SIZE;
>> +                     serial_out(master, &cmd);
>> +                     continue;
>> +
>> +             case FSI_GPIO_RESP_ERRA:
>> +             case FSI_GPIO_RESP_ERRC:
>> +                     dev_info(master->master.dev, "ERR received: %d\n",
>> +                             (int)response.msg);
>> +                     drain_response(master);
>> +                     fsi_master_gpio_error(master, response.msg);
>> +                     return response.msg;
>> +             }
>> +
>> +             /* 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;
>> +             }
>> +
>> +             /* Read in the crc and check it */
>> +             serial_in(master, &response, FSI_GPIO_CRC_SIZE);
>> +
>> +             crc_in = fsi_crc4(0, 1, 1);
>> +             crc_in = fsi_crc4(crc_in, resp, bits_received);
>> +             if (crc_in != response.msg) {
>> +                     /* CRC's don't match */
>> +                     dev_info(master->master.dev, "ERR response CRC\n");
>> +                     fsi_master_gpio_error(master, FSI_GPIO_CRC_INVAL);
>> +                     return FSI_GPIO_CRC_INVAL;
>> +             }
>> +             return 0;
>> +
>> +     } while (busy_count++ < FSI_GPIO_MAX_BUSY);
>> +
>> +     return FSI_GPIO_MAX_BUSY;
>> +}
>
> While you've explained the return values to me, it isn't obvious from
> the code. From reading the poll_response:
>
>  - we return 0 on success
>
>  - we return 1 if we don't see a start bit (what's MTOE?)
>
>  - we return 100 if we got 100 busy responses (after dpoll)
>
>  - we return 2 or 3 if there was a checksum error on the slave
>
>  - we return 5 if there was a checksum error on the master
>
> Now, this *may* be okay if we were interpreting the return values within
> this one file (although we'd want to change busy into something more in
> line with the others), but this is returned directly to the fsi core
> through the read & write callbacks.
>
> Without any explicit definition of the return values, we'd expect a
> standard kernel scheme (0 on success, negative errno (eg -EIO) on
> failure), particularly on the master->read/write interface.
>
> I assume you're going to handle the various return values as part
> of the future error recovery changes, but we should at least (at this
> stage) compress this into -EIO from the read/write callbacks (possibly
> in the actual read/write implementations).
>
> In future, when we have conditions that we can potentially recover from,
> I'd suggest using an enum to represent the full set of errors, and
> define this function to return that type. That allows us to describe the
> possible set of errors to future developers, and allows the compiler to
> warn us if we're not handling a particular case.
>
> Until the error recovery code is added, we won't know what the best
> strategy is - but in the meantime, we should stick to the
> zero/negative-errno convention.

I'll compress those return codes to -EIO.  The specific bus error
information is to only consumed by the master error handler anyway.

>
>> +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, "fsi_clk", 0);
>
> You've changed these unnecessarily; this name string is used to look up
> the device tree property, not the pinctl descriptor.
>

Will back that out.

> Cheers,
>
>
> Jeremy

Re: [PATCH linux v5 16/18] drivers/fsi: Add client driver register utilities

[Christopher Bostic]

On Wed, Oct 19, 2016 at 8:29 PM, Jeremy Kerr <jk@ozlabs.org> wrote:
> Hi Chris,
>
>> +int fsi_drv_register(struct fsi_driver *fsi_drv)
>
> Not fsi_driver_register()?
>

Hi Jeremy,

Will change.

Thanks,
Chris

> Cheers,
>
>
> Jeremy

Re: [PATCH linux v5 18/18] Documenation: Add basic FSI text file

[Christopher Bostic]

On Wed, Oct 19, 2016 at 9:17 PM, Jeremy Kerr <jk@ozlabs.org> wrote:
> Hi Chris,
>
>> Add documentation explaining the basics of FSI and how to find
>> more information.
>> +The Flexible Support Interface (FSI) is a two wire bus consisting of
>> +a serial data line and a clock capable of running at up to 166 MHz.
>> +FSI features strong data integrity with CRC protection and is capable
>> +of very high fan out.
>> +
>> +The complete hardware specification[1] for FSI is publicly available on
>> +the OpenPower Foundation website:
>> +
>> +[1] http://openpowerfoundation.org/wp-content/uploads/resources/OpenFSI-spec/content/ch_preface.html
>> +
>> +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 / Configuration 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 contains, as well as its addressing range.
>> +
>> +* FSI Client driver
>> +    A device driver that registers with the FSI core so that it can access
>> +    devices it owns on an FSI bus.
>
> Is "client" something new, or is this standard FSI terminology? I would
> have thought "engine" is the term used elsewhere.
>
> Anyway, here's a little more to tie it into the kernel implementation:

Hi Jeremy,

OK will change client wording to engine and add your text.
If we can defer the description of API's I'd prefer to do that.

Thanks,
Chris

>
> ------------
> In Linux, we have a core FSI "bus type", along with drivers for FSI
> masters and clients.
>
> 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. Client drivers are matched and bound to these engines during
> discovery.
>
> ------------
>
> We may want to add some description of the APIs, but that should do for
> now.
>
> Cheers,
>
>
> Jeremy

Re: [PATCH linux v5 17/18] drivers/fsi: Add SCOM FSI client device driver

[Christopher Bostic]

On Wed, Oct 19, 2016 at 10:27 PM, Jeremy Kerr <jk@ozlabs.org> wrote:
> Hi Chris,
>
>> diff > +static int scom_probe(struct device *);
>> +
>> +struct scom_op {
>> +     uint64_t        data;
>> +     uint32_t        addr;
>> +};
>
> You're making this a userspace interface, so this should go into the
> uapi headers (and then will form part of the unchangeable Linux ABI).
> However, more on the ABI below...
>
>> +struct scom_device {
>> +     struct fsi_device *fsi_dev;
>> +     struct cdev *cdev;
>> +};
>> +
>> +struct scom_device scom_dev;
>> +
>> +struct fsi_device_id scom_ids[] = {
>> +     {
>> +             .engine_type = FSI_ENGID_SCOM,
>> +             .version = FSI_VERSION_ANY,
>> +     },
>> +     { 0 }
>> +};
>> +
>> +struct fsi_driver scom_drv = {
>> +     .id_table = scom_ids,
>> +     .drv = {
>> +             .name = "scom",
>> +             .bus = &fsi_bus_type,
>> +             .probe = scom_probe,
>> +     }
>> +};
>
> Make these static, and put after the code, just before the
> module_fsi_driver. Then you won't need the forward definition of
> scom_probe.
>

Hi Jeremy,

OK will change.

>> > +static ssize_t scom_read(struct file *filep, char __user *buf, size_t len,
>> +                     loff_t *offset)
>> +{
>> +     int rc = 0;
>> +     struct scom_op op;
>> +     struct scom_device *scom = &scom_dev;
>> +     struct device *dev = &scom->fsi_dev->dev;
>> +
>> +     /* todo: Retrieve device to operate on from filep->private_data */
>
> Yep, otherwise you only get one scom interface, and crazy results if you
> get probed twice. I'd suggest not leaving this until later.
>

Will add.

>> +
>> +     if (len != sizeof(struct scom_op))
>> +             return -EINVAL;
>> +
>> +     rc = copy_from_user(&op, buf, len);
>> +     if (rc) {
>> +             dev_dbg(dev, "copy from user failed:%d\n", rc);
>> +             return -EINVAL;
>> +     }
>> +
>> +     rc = get_scom(&op.data, op.addr);
>
> I'm not a huge fan of that ABI to userspace. You already have the
> address in the offset argument, so you can do this without needing new
> uapi headers, using something like:
>
> static ssize_t scom_read(struct file *filep, char __user *buf, size_t len,
>                         loff_t *offset)
> {
>         uint64_t val;
>
>         /* todo: can we do 1/2/4/8 byte operations too? */
>         if (len != sizeof(uint64_t))
>                 return -EINVAL;
>
>         rc = get_scom(&val, offset);
>         if (rc)
>                 return rc;
>
>         rc = copy_to_user(&val, buf, len);
>
>         return rc ? rc : len;
> }
>

OK, will change that.

>
> Also, have you thought about endianness?
>
>

The plan is for endianness is to be dealt with in the fsi-core
so the other engines don't need to worry about it.  Yes there
is likely to be some additions there.  Will need to evaluate.

>> +static int scom_probe(struct device *dev)
>> +{
>> +     struct fsi_device *fsi_dev = to_fsi_dev(dev);
>> +     struct scom_device *scom = &scom_dev;
>> +     int rc;
>> +
>> +     scom->fsi_dev = fsi_dev;
>> +     scom->cdev = cdev_alloc();
>> +     if (!scom->cdev)
>> +             return -ENOMEM;
>> +
>> +     scom->cdev->owner = THIS_MODULE;
>> +
>> +     rc = register_chrdev(0, "scom", &scom_fops);
>> +     if (rc < 0) {
>> +             dev_dbg(dev, "scom major allocation failed:%d\n", rc);
>> +             return rc;
>> +     }
>> +     dev_dbg(dev, "scom major allocated:%d\n", rc);
>
> You need to register the actual device too, right? We'll want scom0,
> scom1, etc.

Don't understand your meaning here.  Also scom0 and scom1 refer to
what exactly?  For a first pass of core code I'd like to be able to deal
with a single engine if possible and expand later.

Thanks,
Chris


>
> Cheers,
>
>
> Jeremy

Re: [PATCH linux v5 17/18] drivers/fsi: Add SCOM FSI client device driver

[Christopher Bostic]

>
>> +static int scom_probe(struct device *dev)
>> +{
>> +     struct fsi_device *fsi_dev = to_fsi_dev(dev);
>> +     struct scom_device *scom = &scom_dev;
>> +     int rc;
>> +
>> +     scom->fsi_dev = fsi_dev;
>> +     scom->cdev = cdev_alloc();
>> +     if (!scom->cdev)
>> +             return -ENOMEM;
>> +
>> +     scom->cdev->owner = THIS_MODULE;
>> +
>> +     rc = register_chrdev(0, "scom", &scom_fops);
>> +     if (rc < 0) {
>> +             dev_dbg(dev, "scom major allocation failed:%d\n", rc);
>> +             return rc;
>> +     }
>> +     dev_dbg(dev, "scom major allocated:%d\n", rc);
>
> You need to register the actual device too, right? We'll want scom0,
> scom1, etc.

Hi Jeremy,

Is there any value in appending an FSI device path to scom?  Like cat'ing the
fsi device name on end.  That's what is done in existing product and it makes
it easy to tell what device is where in the topology.

Thanks,
Chris

>
> Cheers,
>
>
> Jeremy

Re: [PATCH linux v5 15/18] drivers/fsi: Add GPIO FSI master

[Jeremy Kerr]

Hi Chris,

>>> diff --git a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
>>> index 21619fd..1875313 100644
>>> --- a/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
>>> +++ b/arch/arm/boot/dts/aspeed-bmc-opp-palmetto.dts
>>> @@ -167,6 +167,36 @@
>>>               output-low;
>>>               line-name = "func_mode2";
>>>       };
>>> +
>>> +     pin_fsi_clk {
>>> +             gpios = <ASPEED_GPIO(A, 4)  GPIO_ACTIVE_HIGH>;
>>> +             output-low;
>>> +             line-name = "fsi_clk";
>>> +     };
>>> +
>>> +     pin_fsi_data {
>>> +             gpios = <ASPEED_GPIO(A, 5)  GPIO_ACTIVE_HIGH>;
>>> +             output-low;
>>> +             line-name = "fsi_data";
>>> +     };
>>> +
>>> +     pin_fsi_trans {
>>> +             gpios = <ASPEED_GPIO(H, 6)  GPIO_ACTIVE_HIGH>;
>>> +             output-low;
>>> +             line-name = "fsi_trans";
>>> +     };
>>> +
>>> +     pin_fsi_enable {
>>> +             gpios = <ASPEED_GPIO(D, 0)  GPIO_ACTIVE_HIGH>;
>>> +             output-low;
>>> +             line-name = "fsi_enable";
>>> +     };
>>> +
>>> +     pin_fsi_mux {
>>> +             gpios = <ASPEED_GPIO(A, 6)  GPIO_ACTIVE_HIGH>;
>>> +             output-low;
>>> +             line-name = "fsi_mux";
>>> +     };
>>>  };
>>
>> Do we want to add a master node too?
> 
> Don't follow what you mean by a master node.  What does that help with?

The master node is what describes the fsi master "device" (in this case,
the usage of a set of GPIOs as a functional unit).

The properties above just define some pinctl configuration to describe a
bunch of GPIOs. This doesn't actually describe anything fsi-master
related.

To do that, we need the master node too; that's the binding
specification that we're adding to Documentation/device-tree, and is the
thing that looks like:

     fsi-master {
         compatible = "ibm,fsi-master", "ibm,fsi-master-gpio";
         clk-gpios = <&gpio 0>
         data-gpios = <&gpio 1>
     };

or, for one of the fake masters for testing:

     fsi-master {
         compatible = "ibm,fsi-master", "ibm,fsi-master-fake";
     };

That master node is what causes the master drivers to be "bound" to a
device (and multiple masters mean multiple devices instances & probes).

Does this mean you haven't yet tested with either the fake or gpio
masters? If that's the case, that should be your priority. We're at the
stage that any further code review would be redundant, as that code is
fairly likely to need changes as a result of testing.

>> We should also think about what we want to do for multiple links. There
>> are two main options here:
>>
>>   - list them as separate masters in the DT (ie, two fsi-master nodes,
>>     completely independent); or
>>
>>   - enable multiple GPIO descriptors in the gpio properties, eg:
>>
>>     fsi-master {
>>         compatible = "ibm,fsi-master", "ibm,fsi-master-gpio";
>>         clk-gpios = <&gpio 0 &gpio 6>;
>>         data-gpios = <&gpio 1 &gpio 7>;
>>     }
>>
>>     which describes a master with two links
>>
>> If we use the latter case, we'd want the property name to be plural
>> (*-gpios) to indicate this.
> 
> Its difficult to say how many links we'll eventually need a priori.

We don't need to - this isn't prescribed by the binding.

These *-gpios properties contain one or more "descriptors" of individual
GPIOs. While the format of a descriptor depends on how the GPIO
controller describes each GPIO, the notation we've used in that example
is fairly common, and is:

  clk-gpio = <(pointer to gpio controller) (index within controller)>

in this case, each gpio descriptor takes two cells (a cell is a u32),
and so the GPIO controller will have a #gpio-cells = <2> property.

Our vanilla master with one link would just be something like:

     fsi-master {
         compatible = "ibm,fsi-master", "ibm,fsi-master-gpio";
         clk-gpios = <&gpio 0>
         data-gpios = <&gpio 1>
     }

- this describes a single clock and single data line, as each *-gpios
  property is one descriptor.

Say if we have a FSI master with two links, we would have four cells
for all the *-gpios properties:

     fsi-master {
         compatible = "ibm,fsi-master", "ibm,fsi-master-gpio";
         clk-gpios = <&gpio 0 &gpio 6>;
         data-gpios = <&gpio 1 &gpio 7>;
     }

- and the binding would remain as is (just that we should use the plural
of gpios for the property names, to make this a little more sensible).

This could also be represented as two separate master nodes:

     fsi-master {
         compatible = "ibm,fsi-master", "ibm,fsi-master-gpio";
         clk-gpios = <&gpio 0>;
         data-gpios = <&gpio 1>;
     }

     fsi-master {
         compatible = "ibm,fsi-master", "ibm,fsi-master-gpio";
         clk-gpios = <&gpio 6>;
         data-gpios = <&gpio 7>;
     }

Our choice there would depend on the hardware layout. Does is make more
sense to represent this as two independent masters? Or a single one?
Would there ever be any shared state between those two masters that a
driver needs to handle?

Once we've settled that, we will need the device tree binding people to
take a look, which just involves CCing devicetree-discuss@ozlabs.org for
patches that modify Documentation/devicetree/*.

>> However, you have two serial_in() calls in the single path, can you
>> consolidate these?
>>
> 
> Ok, will change.
> 
> This type of change I would agree offers some minor performance
> gain but it does add a few lines of code to decode the slave id that weren't
> needed before.

This isn't for performance gain, it's purely for readability.

> Since I'm making changes to this file already it makes
> sense to add this one.   If all that remain though are minor changes like
> this one I'd like to be able to address those after the first core patch set
> is approved.   Would this be acceptable?

That can be a tough call. If it's a cleanup post-submission, then you've
lost the opportunity to roll that cleanup into the original inclusion,
and will need an extra patch for that cleanup (it's bad form to include
a cleanup change in something unrelated).

However, if there's minor things that don't warrant resubmission, then
that should be fine - just be aware that upstream folks may request that
you change it before submission *anyway*.

Cheers,


Jeremy

Re: [PATCH linux v5 17/18] drivers/fsi: Add SCOM FSI client device driver

[Jeremy Kerr]

Hi Chris,

>> Also, have you thought about endianness?
> 
> The plan is for endianness is to be dealt with in the fsi-core
> so the other engines don't need to worry about it.  Yes there
> is likely to be some additions there.  Will need to evaluate.

OK.

>>> +static int scom_probe(struct device *dev)
>>> +{
>>> +     struct fsi_device *fsi_dev = to_fsi_dev(dev);
>>> +     struct scom_device *scom = &scom_dev;
>>> +     int rc;
>>> +
>>> +     scom->fsi_dev = fsi_dev;
>>> +     scom->cdev = cdev_alloc();
>>> +     if (!scom->cdev)
>>> +             return -ENOMEM;
>>> +
>>> +     scom->cdev->owner = THIS_MODULE;
>>> +
>>> +     rc = register_chrdev(0, "scom", &scom_fops);
>>> +     if (rc < 0) {
>>> +             dev_dbg(dev, "scom major allocation failed:%d\n", rc);
>>> +             return rc;
>>> +     }
>>> +     dev_dbg(dev, "scom major allocated:%d\n", rc);
>>
>> You need to register the actual device too, right? We'll want scom0,
>> scom1, etc.
> 
> Don't understand your meaning here.  Also scom0 and scom1 refer to
> what exactly?

The actual devices. You're only registering a chardev major number
there, not an actual character device, and so this doesn't result in any
/dev/scom node being created.

You'll want to do a device_create there, to get the minor allocated and
have the device appear. However, that means you'll also need to create a
class too.

If you want to skip all that, just create a miscdev instead. That will
take care of the major/minor numbers for you, and will go into the
'misc' device class.

> For a first pass of core code I'd like to be able to deal with a
> single engine if possible and expand later.

There's a right and wrong way to do that though. Currently, it looks
like you're intending to register a /dev/scom. Then, when you want to
add multiple interfaces in future, that will have to be removed to
create /dev/scom0, /dev/scom1, etc. That's a userspace ABI change, which
could break old code.

So, even if your driver only handles one device for now, name that in a
way that doesn't conflict with that future interface.

However, handling multiple devices is pretty trivial. Just allocate your
device in the probe, rather than using one static instance, and name it
accordingly. Here's what I'd suggest for your probe function:

struct scom_device {
	struct fsi_device	*fsi_dev;
	struct miscdevice	mdev;
	char			name[32];
};

static atomic_t scom_idx = ATOMIC_INIT(0);

static int scom_probe(struct device *dev)
{
	struct fsi_device *fsi_dev = to_fsi_dev(dev);
	struct scom_device *scom;
	int rc;

	scom = devm_kzalloc(dev, sizeof(*scom), GFP_KERNEL);
	if (!scom)
		return -ENOMEM;

	snprintf(scom->name, sizeof(scom->name),
			"scom%d", atomic_inc_return(&scom_idx));
	scom->fsi_dev = fsi_dev;
	scom->mdev.minor = MISC_DYNAMIC_MINOR;
	scom->mdev.fops = &scom_fops;
	scom->mdev.name = scom->name;
	scom->mdev.parent = dev;

	return misc_register(&scom->mdev);
}


Then, in your read/write functions, file->private_data is your mdev, and
you can just do a container_of to get the struct scom_device *.

From your next email:

> Is there any value in appending an FSI device path to scom?  Like
> cat'ing the fsi device name on end.  That's what is done in existing
> product and it makes it easy to tell what device is where in the
> topology.

No, that's not normal convention for providing that sort of information.
If we set the device parent (ie, scom->mdev.parent), the device will be
referenced correctly through sysfs, so we have the correct hardware bus
layout available there. For example:

  # cat /sys/bus/fsi/devices/00:00:00:00/misc/scom1/dev
  10:62
  # ls -l /dev/scom1
  crw-------    1 root     root       10,  62 Jan  1 00:00 /dev/scom1

Then, if we want symlinks in dev, we add the appropriate udev rules to
create those according to system policy. The names in dev should be
fairly dynamic (eg, /dev/sdXN, /dev/ipmiN, etc)

[One advantage of using a chardev with a proper class is that 'misc'
class directory in the sysfs path could be named something more
appropriate...]

Cheers,


Jeremy

Re: [PATCH linux v5 13/18] drivers/fsi: Set up links for slave communication

[Jeremy Kerr]

Hi Chris,

> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
> index 565c7e3..81ed444 100644
> --- a/drivers/fsi/fsi-core.c
> +++ b/drivers/fsi/fsi-core.c
> @@ -287,13 +287,51 @@ static int fsi_slave_init(struct fsi_master *master,
>  
>  /* FSI master support */
>  
> +static int fsi_master_link_enable(struct fsi_master *master, int link)
> +{
> +	if (master->link_enable)
> +		return master->link_enable(master, link);
> +
> +	return -EINVAL;
> +}

We don't want to error on no ->link_enable callback; this breaks for any
masters that don't require this (eg, the fake master)

> +/*
> + * Issue a break command on this link
> + */
> +static int fsi_master_break(struct fsi_master *master, int link)
> +{
> +	int rc;
> +
> +	if (!master->send_break)
> +		return -EINVAL;

And do we want to error here? If so, we'll need to implement send_break
for the fake master.

Cheers,


Jeremy

Re: [PATCH linux v5 17/18] drivers/fsi: Add SCOM FSI client device driver

[Christopher Bostic]

On Fri, Oct 21, 2016 at 12:24 AM, Jeremy Kerr <jk@ozlabs.org> wrote:
> Hi Chris,
>
>>> Also, have you thought about endianness?
>>
>> The plan is for endianness is to be dealt with in the fsi-core
>> so the other engines don't need to worry about it.  Yes there
>> is likely to be some additions there.  Will need to evaluate.
>
> OK.
>
>>>> +static int scom_probe(struct device *dev)
>>>> +{
>>>> +     struct fsi_device *fsi_dev = to_fsi_dev(dev);
>>>> +     struct scom_device *scom = &scom_dev;
>>>> +     int rc;
>>>> +
>>>> +     scom->fsi_dev = fsi_dev;
>>>> +     scom->cdev = cdev_alloc();
>>>> +     if (!scom->cdev)
>>>> +             return -ENOMEM;
>>>> +
>>>> +     scom->cdev->owner = THIS_MODULE;
>>>> +
>>>> +     rc = register_chrdev(0, "scom", &scom_fops);
>>>> +     if (rc < 0) {
>>>> +             dev_dbg(dev, "scom major allocation failed:%d\n", rc);
>>>> +             return rc;
>>>> +     }
>>>> +     dev_dbg(dev, "scom major allocated:%d\n", rc);
>>>
>>> You need to register the actual device too, right? We'll want scom0,
>>> scom1, etc.
>>
>> Don't understand your meaning here.  Also scom0 and scom1 refer to
>> what exactly?
>
> The actual devices. You're only registering a chardev major number
> there, not an actual character device, and so this doesn't result in any
> /dev/scom node being created.
>
> You'll want to do a device_create there, to get the minor allocated and
> have the device appear. However, that means you'll also need to create a
> class too.
>
> If you want to skip all that, just create a miscdev instead. That will
> take care of the major/minor numbers for you, and will go into the
> 'misc' device class.
>

Hi Jeremy,

Ok, I've added this change for the next set to review.

>> For a first pass of core code I'd like to be able to deal with a
>> single engine if possible and expand later.
>
> There's a right and wrong way to do that though. Currently, it looks
> like you're intending to register a /dev/scom. Then, when you want to
> add multiple interfaces in future, that will have to be removed to
> create /dev/scom0, /dev/scom1, etc. That's a userspace ABI change, which
> could break old code.
>
> So, even if your driver only handles one device for now, name that in a
> way that doesn't conflict with that future interface.
>
> However, handling multiple devices is pretty trivial. Just allocate your
> device in the probe, rather than using one static instance, and name it
> accordingly. Here's what I'd suggest for your probe function:
>
> struct scom_device {
>         struct fsi_device       *fsi_dev;
>         struct miscdevice       mdev;
>         char                    name[32];
> };
>
> static atomic_t scom_idx = ATOMIC_INIT(0);
>
> static int scom_probe(struct device *dev)
> {
>         struct fsi_device *fsi_dev = to_fsi_dev(dev);
>         struct scom_device *scom;
>         int rc;
>
>         scom = devm_kzalloc(dev, sizeof(*scom), GFP_KERNEL);
>         if (!scom)
>                 return -ENOMEM;
>
>         snprintf(scom->name, sizeof(scom->name),
>                         "scom%d", atomic_inc_return(&scom_idx));
>         scom->fsi_dev = fsi_dev;
>         scom->mdev.minor = MISC_DYNAMIC_MINOR;
>         scom->mdev.fops = &scom_fops;
>         scom->mdev.name = scom->name;
>         scom->mdev.parent = dev;
>
>         return misc_register(&scom->mdev);
> }
>
>
> Then, in your read/write functions, file->private_data is your mdev, and
> you can just do a container_of to get the struct scom_device *.
>

Added these changes as well to the next patch set.


> From your next email:
>
>> Is there any value in appending an FSI device path to scom?  Like
>> cat'ing the fsi device name on end.  That's what is done in existing
>> product and it makes it easy to tell what device is where in the
>> topology.
>
> No, that's not normal convention for providing that sort of information.
> If we set the device parent (ie, scom->mdev.parent), the device will be
> referenced correctly through sysfs, so we have the correct hardware bus
> layout available there. For example:
>
>   # cat /sys/bus/fsi/devices/00:00:00:00/misc/scom1/dev
>   10:62
>   # ls -l /dev/scom1
>   crw-------    1 root     root       10,  62 Jan  1 00:00 /dev/scom1
>
> Then, if we want symlinks in dev, we add the appropriate udev rules to
> create those according to system policy. The names in dev should be
> fairly dynamic (eg, /dev/sdXN, /dev/ipmiN, etc)
>
> [One advantage of using a chardev with a proper class is that 'misc'
> class directory in the sysfs path could be named something more
> appropriate...]
>

Thanks for the clarification.
Chris.


> Cheers,
>
>
> Jeremy

Re: [PATCH linux v5 13/18] drivers/fsi: Set up links for slave communication

[Christopher Bostic]

On Fri, Oct 21, 2016 at 12:28 AM, Jeremy Kerr <jk@ozlabs.org> wrote:
> Hi Chris,
>
>> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
>> index 565c7e3..81ed444 100644
>> --- a/drivers/fsi/fsi-core.c
>> +++ b/drivers/fsi/fsi-core.c
>> @@ -287,13 +287,51 @@ static int fsi_slave_init(struct fsi_master *master,
>>
>>  /* FSI master support */
>>
>> +static int fsi_master_link_enable(struct fsi_master *master, int link)
>> +{
>> +     if (master->link_enable)
>> +             return master->link_enable(master, link);
>> +
>> +     return -EINVAL;
>> +}
>
> We don't want to error on no ->link_enable callback; this breaks for any
> masters that don't require this (eg, the fake master)
>

Hi Jeremy,

OK will change that.

>> +/*
>> + * Issue a break command on this link
>> + */
>> +static int fsi_master_break(struct fsi_master *master, int link)
>> +{
>> +     int rc;
>> +
>> +     if (!master->send_break)
>> +             return -EINVAL;
>
> And do we want to error here? If so, we'll need to implement send_break
> for the fake master.

Will remove this as well.


Thanks,
Chris

>
> Cheers,
>
>
> Jeremy

Re: [PATCH linux v5 15/18] drivers/fsi: Add GPIO FSI master

[Christopher Bostic]

>
> Say if we have a FSI master with two links, we would have four cells
> for all the *-gpios properties:
>
>      fsi-master {
>          compatible = "ibm,fsi-master", "ibm,fsi-master-gpio";
>          clk-gpios = <&gpio 0 &gpio 6>;
>          data-gpios = <&gpio 1 &gpio 7>;
>      }
>

Hi Jeremy,

One thing I don't understand is the numbering conventions.   What does
6 and 7 represent in this case?
There are five pins total 0-4 for the existing gpio master.  I've
followed the numbering as is listed here
in the latest patch set I'm submitting this afternoon.

> - and the binding would remain as is (just that we should use the plural
> of gpios for the property names, to make this a little more sensible).
>
> This could also be represented as two separate master nodes:
>
>      fsi-master {
>          compatible = "ibm,fsi-master", "ibm,fsi-master-gpio";
>          clk-gpios = <&gpio 0>;
>          data-gpios = <&gpio 1>;
>      }
>
>      fsi-master {
>          compatible = "ibm,fsi-master", "ibm,fsi-master-gpio";
>          clk-gpios = <&gpio 6>;
>          data-gpios = <&gpio 7>;
>      }
>
> Our choice there would depend on the hardware layout. Does is make more
> sense to represent this as two independent masters? Or a single one?
> Would there ever be any shared state between those two masters that a
> driver needs to handle?

Up until now, all available FSI masters in hardware,  FSP, P8 "hub", P8 cascade,
P7 cascade, have all had multiple links controlled by one master.   It would
make sense I think to continue this.

There is no direct sharing of information between masters other than what's
required by the firmware to start at the beginning master in the chain and
walk down each link  and checking 'sub' master states to determine where an
interrupt or error was sourced.

General flow:
Identify on which link interrupt / error was reported.
Look at each CFAM on that link to see if it was reported locally or downstream
if downstream,  Look at the cascaded master's owned links to see which
reported.  Etc..

In my opinion that type of master to master information is better kept separated
as it is in the interests of keep a general design common between real hardware
FSI masters such as the FSP's and the gpio type master implementations.

>
> Once we've settled that, we will need the device tree binding people to
> take a look, which just involves CCing devicetree-discuss@ozlabs.org for
> patches that modify Documentation/devicetree/*.
>
Will keep them off the cc list for now until we all agree which way we go.

Thanks,
Chris

Re: [PATCH linux v5 15/18] drivers/fsi: Add GPIO FSI master

[Jeremy Kerr]

Hi Chris,

>> Say if we have a FSI master with two links, we would have four cells
>> for all the *-gpios properties:
>>
>>      fsi-master {
>>          compatible = "ibm,fsi-master", "ibm,fsi-master-gpio";
>>          clk-gpios = <&gpio 0 &gpio 6>;
>>          data-gpios = <&gpio 1 &gpio 7>;
>>      }
>>
> 
> Hi Jeremy,
> 
> One thing I don't understand is the numbering conventions.   What does
> 6 and 7 represent in this case?
> There are five pins total 0-4 for the existing gpio master.

This is only an example node, and the format of those example *-gpios
properties isn't really relevant here - what I'm illustrating is that
we could have multiple GPIOs described by each property.

The actual format of those *-gpios properties depends on the GPIO
controller that they're referring to, and is completely opaque to the
consumer of those nodes (in our case, that's the fsi master driver).

Here's how it works:

The GPIO controller driver has its own way of referencing an
individual GPIO, as a tuple of cells. This could be something as
simple as:

  (gpio-number)

or, typically they may have flags (like active-low, etc) too:

  (gpio-number,flags)

but more complex controllers can use any format, which could better
reference their hardware layout. To make up an example:

  (controller-bank,bank-index,flags)

The device tree node for the GPIO controller will have a #gpio-cells
property. This tells us how many cells are required for a tuple that
describes an individual GPIO.

When we want to reference an individual GPIO from elsewhere in the
device tree, we use one of these *-gpios properties, which contains:

  1) a pointer to the controller node
  2) the tuple describing the GPIO within that controller

(1) is always a phandle (a single cell that references the phandle
property of another node). This is how we know which controller we're
referring to.

Once we have the controller, we can look at its #gpio-cells property -
this tells us how many cells are needed for (2), which allows the GPIO
core code to construct that tuple from a DT property.

We request GPIOs using this tuple - the controller driver (and only that
driver) knows how to interpret the tuple, and returns the selected tuple
to our fsi master.

[The term "tuple" isn't standard here - I'm just using it for the
purposes of this email. The whole construct (ie, (1) and (2)) is called
a descriptor though].

However, the fsi driver doesn't need to care about *any* of that - we
just do a devm_gpiod_get() and out pops the correct GPIO.

We do need to know that format when constructing the device tree though,
and that will vary from platform to platform. Using the palmetto board
as an example, we would have something that looks like:

  fsi-master {
      compatible = "ibm,fsi-master", "ibm,fsi-master-gpio";
      clk-gpios = <&gpio ASPEED_GPIO(A,4) GPIO_ACTIVE_LOW>;
      data-gpios = <&gpio ASPEED_GPIO(A,5) GPIO_ACTIVE_LOW>;
  }

- using GPIOs A4 and A5, Assuming my reading of the schematic is
correct.

> Up until now, all available FSI masters in hardware,  FSP, P8 "hub",
> P8 cascade, P7 cascade, have all had multiple links controlled by one
> master.   It would make sense I think to continue this.

I'm only referring to separate "top-level" masters here (ie, simply the
groups of GPIOs themselves). There are a couple of considerations here:

  a) Using one-link-per-GPIO-master keeps the driver code a little simpler;
     it doesn't need to manage multiple links

  b) There is a little more scope for complexity & error in the *-gpios
      properties, if multiple clk/data/etc lines are described (say, if
      one FSI link of a multi-master doesn't have a separate mux line..)
     
- so I'm slightly preferring the single-link-per-master idea at present
(of course, that could cascade to other masters behind slave engines,
but the master driver doesn't need to care about that).

> There is no direct sharing of information between masters other than
> what's required by the firmware to start at the beginning master in
> the chain and walk down each link  and checking 'sub' master states to
> determine where an interrupt or error was sourced.

Again, this only references how we handle the top-level masters.

Regards,


Jeremy