[PATCH v6 01/23] drivers/fsi: Add fsi master definition

[PATCH v6 01/23] drivers/fsi: Add fsi master definition

[Christopher Bostic]

From: Jeremy Kerr <jk@ozlabs.org>

Add a `struct fsi_master` to represent a FSI master controller.

FSI master drivers register one of these structs to provide
device-specific of the standard operations: read/write/term/break and
link control.

Includes changes from Edward A. James <eajames@us.ibm.com> & Jeremy Kerr
<jk@ozlabs.org>.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/fsi/fsi-core.c   | 35 +++++++++++++++++++++++++++++++++++
 drivers/fsi/fsi-master.h | 41 +++++++++++++++++++++++++++++++++++++++++
 2 files changed, 76 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..ca02913 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -15,8 +15,43 @@
 
 #include <linux/device.h>
 #include <linux/fsi.h>
+#include <linux/idr.h>
 #include <linux/module.h>
 
+#include "fsi-master.h"
+
+static DEFINE_IDA(master_ida);
+
+/* FSI master support */
+int fsi_master_register(struct fsi_master *master)
+{
+	int rc;
+
+	if (!master)
+		return -EINVAL;
+
+	master->idx = ida_simple_get(&master_ida, 0, INT_MAX, GFP_KERNEL);
+	dev_set_name(&master->dev, "fsi%d", master->idx);
+
+	rc = device_register(&master->dev);
+	if (rc)
+		ida_simple_remove(&master_ida, master->idx);
+
+	return rc;
+}
+EXPORT_SYMBOL_GPL(fsi_master_register);
+
+void fsi_master_unregister(struct fsi_master *master)
+{
+	if (master->idx >= 0) {
+		ida_simple_remove(&master_ida, master->idx);
+		master->idx = -1;
+	}
+
+	device_unregister(&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..7764b00
--- /dev/null
+++ b/drivers/fsi/fsi-master.h
@@ -0,0 +1,41 @@
+/*
+ * 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		flags;
+	int		(*read)(struct fsi_master *, int link, uint8_t id,
+				uint32_t addr, void *val, size_t size);
+	int		(*write)(struct fsi_master *, int link, uint8_t id,
+				uint32_t addr, const void *val, size_t size);
+	int		(*term)(struct fsi_master *, int link, uint8_t id);
+	int		(*send_break)(struct fsi_master *, int link);
+	int		(*link_enable)(struct fsi_master *, int link);
+};
+
+#define dev_to_fsi_master(d) container_of(d, struct fsi_master, dev)
+
+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 v6 02/23] drivers/fsi: Add slave definition

[Christopher Bostic]

From: Jeremy Kerr <jk@ozlabs.org>

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

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/fsi/fsi-core.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index ca02913..2f19509 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -22,6 +22,16 @@
 
 static DEFINE_IDA(master_ida);
 
+struct fsi_slave {
+	struct device		dev;
+	struct fsi_master	*master;
+	int			id;
+	int			link;
+	uint32_t		size;	/* size of slave address space */
+};
+
+#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 v6 03/23] drivers/fsi: Add empty master scan

[Christopher Bostic]

From: Jeremy Kerr <jk@ozlabs.org>

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

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/fsi/fsi-core.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 2f19509..e90d45d 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -32,7 +32,25 @@ 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 id)
+{
+	/* todo: initialise slave device, perform engine scan */
+
+	return -ENODEV;
+}
+
 /* FSI master support */
+static int fsi_master_scan(struct fsi_master *master)
+{
+	int link;
+
+	for (link = 0; link < master->n_links; link++)
+		fsi_slave_init(master, link, 0);
+
+	return 0;
+}
+
 int fsi_master_register(struct fsi_master *master)
 {
 	int rc;
@@ -44,10 +62,13 @@ int fsi_master_register(struct fsi_master *master)
 	dev_set_name(&master->dev, "fsi%d", master->idx);
 
 	rc = device_register(&master->dev);
-	if (rc)
+	if (rc) {
 		ida_simple_remove(&master_ida, master->idx);
+		return rc;
+	}
 
-	return rc;
+	fsi_master_scan(master);
+	return 0;
 }
 EXPORT_SYMBOL_GPL(fsi_master_register);
 
-- 
1.8.2.2

[PATCH v6 04/23] drivers/fsi: Add crc4 helpers

[Christopher 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.

We add this to the fsi code (rather than lib/), as we need a specific
polynomial for FSI CRCs.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/fsi/fsi-core.c   | 24 ++++++++++++++++++++++++
 drivers/fsi/fsi-master.h | 21 +++++++++++++++++++++
 2 files changed, 45 insertions(+)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index e90d45d..4bbca95 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -32,6 +32,30 @@ 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;
+
+	/* mask off anything above the top bit */
+	x &= (1ull << bits) - 1;
+
+	/* Calculate crc4 over four-bit nibbles, starting@the MSbit */
+	for (i = bits - 4; i >= 0; i -= 4)
+		c = crc4_tab[c ^ ((x >> i) & 0xf)];
+
+	return c;
+}
+EXPORT_SYMBOL_GPL(fsi_crc4);
+
 /* FSI slave support */
 static int fsi_slave_init(struct fsi_master *master, int link, uint8_t id)
 {
diff --git a/drivers/fsi/fsi-master.h b/drivers/fsi/fsi-master.h
index 7764b00..d6a4885 100644
--- a/drivers/fsi/fsi-master.h
+++ b/drivers/fsi/fsi-master.h
@@ -38,4 +38,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 v6 05/23] drivers/fsi: Add slave & master read/write APIs

[Christopher Bostic]

From: Jeremy Kerr <jk@ozlabs.org>

Introduce functions to perform reads/writes on the slave address space;
these simply pass the request on the slave's master with the correct
link and slave ID.

We implement these on top of similar helpers for the master.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Joel Stanley <joel@jms.id.au>
Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
---
 drivers/fsi/fsi-core.c | 92 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 92 insertions(+)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 4bbca95..32698ed 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -32,6 +32,11 @@ struct fsi_slave {
 
 #define to_fsi_slave(d) container_of(d, struct fsi_slave, dev)
 
+static int fsi_master_read(struct fsi_master *master, int link,
+		uint8_t slave_id, uint32_t addr, void *val, size_t size);
+static int fsi_master_write(struct fsi_master *master, int link,
+		uint8_t slave_id, uint32_t addr, const void *val, size_t size);
+
 /* crc helpers */
 static const uint8_t crc4_tab[] = {
 	0x0, 0x7, 0xe, 0x9, 0xb, 0xc, 0x5, 0x2,
@@ -57,6 +62,58 @@ uint8_t fsi_crc4(uint8_t c, uint64_t x, int bits)
 EXPORT_SYMBOL_GPL(fsi_crc4);
 
 /* FSI slave support */
+static int fsi_slave_calc_addr(struct fsi_slave *slave, uint32_t *addrp,
+		uint8_t *idp)
+{
+	uint32_t addr = *addrp;
+	uint8_t id = *idp;
+
+	if (addr > slave->size)
+		return -EINVAL;
+
+	/* For 23 bit addressing, we encode the extra two bits in the slave
+	 * id (and the slave's actual ID needs to be 0).
+	 */
+	if (addr > 0x1fffff) {
+		if (slave->id != 0)
+			return -EINVAL;
+		id = (addr >> 21) & 0x3;
+		addr &= 0x1fffff;
+	}
+
+	*addrp = addr;
+	*idp = id;
+	return 0;
+}
+
+static int fsi_slave_read(struct fsi_slave *slave, uint32_t addr,
+			void *val, size_t size)
+{
+	uint8_t id = slave->id;
+	int rc;
+
+	rc = fsi_slave_calc_addr(slave, &addr, &id);
+	if (rc)
+		return rc;
+
+	return fsi_master_read(slave->master, slave->link, id,
+			addr, val, size);
+}
+
+static int fsi_slave_write(struct fsi_slave *slave, uint32_t addr,
+			const void *val, size_t size)
+{
+	uint8_t id = slave->id;
+	int rc;
+
+	rc = fsi_slave_calc_addr(slave, &addr, &id);
+	if (rc)
+		return rc;
+
+	return fsi_master_write(slave->master, slave->link, id,
+			addr, val, size);
+}
+
 static int fsi_slave_init(struct fsi_master *master, int link, uint8_t id)
 {
 	/* todo: initialise slave device, perform engine scan */
@@ -65,6 +122,41 @@ static int fsi_slave_init(struct fsi_master *master, int link, uint8_t id)
 }
 
 /* FSI master support */
+static int fsi_check_access(uint32_t addr, size_t size)
+{
+	if (size != 1 && size != 2 && size != 4)
+		return -EINVAL;
+
+	if ((addr & 0x3) != (size & 0x3))
+		return -EINVAL;
+
+	return 0;
+}
+
+static int fsi_master_read(struct fsi_master *master, int link,
+		uint8_t slave_id, uint32_t addr, void *val, size_t size)
+{
+	int rc;
+
+	rc = fsi_check_access(addr, size);
+	if (rc)
+		return rc;
+
+	return master->read(master, link, slave_id, addr, val, size);
+}
+
+static int fsi_master_write(struct fsi_master *master, int link,
+		uint8_t slave_id, uint32_t addr, const void *val, size_t size)
+{
+	int rc;
+
+	rc = fsi_check_access(addr, size);
+	if (rc)
+		return rc;
+
+	return master->write(master, link, slave_id, addr, val, size);
+}
+
 static int fsi_master_scan(struct fsi_master *master)
 {
 	int link;
-- 
1.8.2.2

[PATCH v6 06/23] drivers/fsi: Set up links for slave communication

[Christopher Bostic]

From: Chris Bostic <cbostic@linux.vnet.ibm.com>

Enable each link and send a break command, and try to detect a slave by
reading from the SMODE register.

Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/fsi/fsi-core.c | 37 +++++++++++++++++++++++++++++++++++--
 1 file changed, 35 insertions(+), 2 deletions(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 32698ed..6e1cfdf 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -157,12 +157,45 @@ static int fsi_master_write(struct fsi_master *master, int link,
 	return master->write(master, link, slave_id, addr, val, size);
 }
 
+static int fsi_master_link_enable(struct fsi_master *master, int link)
+{
+	if (master->link_enable)
+		return master->link_enable(master, link);
+
+	return 0;
+}
+
+/*
+ * Issue a break command on this link
+ */
+static int fsi_master_break(struct fsi_master *master, int link)
+{
+	if (master->send_break)
+		return master->send_break(master, link);
+
+	return 0;
+}
+
 static int fsi_master_scan(struct fsi_master *master)
 {
-	int link;
+	int link, rc;
+
+	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: %d\n", link, rc);
+			continue;
+		}
+		rc = fsi_master_break(master, link);
+		if (rc) {
+			dev_dbg(&master->dev,
+				"break to link %d failed: %d\n", link, rc);
+			continue;
+		}
 
-	for (link = 0; link < master->n_links; link++)
 		fsi_slave_init(master, link, 0);
+	}
 
 	return 0;
 }
-- 
1.8.2.2

[PATCH v6 07/23] drivers/fsi: Implement slave initialisation

[Christopher Bostic]

From: Jeremy Kerr <jk@ozlabs.org>

Implement fsi_slave_init: if we can read a chip ID, create fsi_slave
devices and register with the driver core.

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

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/fsi/fsi-core.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 64 insertions(+), 2 deletions(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 6e1cfdf..c705ca2 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -17,9 +17,12 @@
 #include <linux/fsi.h>
 #include <linux/idr.h>
 #include <linux/module.h>
+#include <linux/slab.h>
 
 #include "fsi-master.h"
 
+#define FSI_SLAVE_SIZE_23b		0x800000
+
 static DEFINE_IDA(master_ida);
 
 struct fsi_slave {
@@ -114,11 +117,70 @@ static int fsi_slave_write(struct fsi_slave *slave, uint32_t addr,
 			addr, val, size);
 }
 
+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 id)
 {
-	/* todo: initialise slave device, perform engine scan */
+	struct fsi_slave *slave;
+	uint32_t chip_id;
+	uint8_t crc;
+	int rc;
+
+	/* Currently, we only support single slaves on a link, and use the
+	 * full 23-bit address range
+	 */
+	if (id != 0)
+		return -EINVAL;
+
+	rc = fsi_master_read(master, link, id, 0, &chip_id, sizeof(chip_id));
+	if (rc) {
+		dev_warn(&master->dev, "can't read slave %02x:%02x %d\n",
+				link, id, rc);
+		return -ENODEV;
+	}
+	chip_id = be32_to_cpu(chip_id);
+
+	crc = fsi_crc4(0, chip_id, 32);
+	if (crc) {
+		dev_warn(&master->dev, "slave %02x:%02x invalid chip id CRC!\n",
+				link, id);
+		return -EIO;
+	}
+
+	dev_info(&master->dev, "fsi: found chip %08x at %02x:%02x:%02x\n",
+			chip_id, master->idx, link, id);
+
+	/* We can communicate with a slave; create the slave device and
+	 * register.
+	 */
+	slave = kzalloc(sizeof(*slave), GFP_KERNEL);
+	if (!slave)
+		return -ENOMEM;
+
+	slave->master = master;
+	slave->dev.parent = &master->dev;
+	slave->dev.release = fsi_slave_release;
+	slave->link = link;
+	slave->id = id;
+	slave->size = FSI_SLAVE_SIZE_23b;
+
+	dev_set_name(&slave->dev, "slave@%02x:%02x", link, 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;
+	}
+
+	/* todo: perform engine scan */
 
-	return -ENODEV;
+	return rc;
 }
 
 /* FSI master support */
-- 
1.8.2.2

[PATCH v6 07/23] drivers/fsi: Implement slave initialisation

[Joel Stanley]

On Tue, Apr 11, 2017 at 5:16 AM, Christopher Bostic
<cbostic@linux.vnet.ibm.com> wrote:
> From: Jeremy Kerr <jk@ozlabs.org>
>
> Implement fsi_slave_init: if we can read a chip ID, create fsi_slave
> devices and register with the driver core.
>
> Includes changes from Chris Bostic <cbostic@linux.vnet.ibm.com>.
>
> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
> Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  drivers/fsi/fsi-core.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 64 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
> index 6e1cfdf..c705ca2 100644
> --- a/drivers/fsi/fsi-core.c
> +++ b/drivers/fsi/fsi-core.c
> @@ -17,9 +17,12 @@
>  #include <linux/fsi.h>
>  #include <linux/idr.h>
>  #include <linux/module.h>
> +#include <linux/slab.h>
>
>  #include "fsi-master.h"
>
> +#define FSI_SLAVE_SIZE_23b             0x800000
> +
>  static DEFINE_IDA(master_ida);
>
>  struct fsi_slave {
> @@ -114,11 +117,70 @@ static int fsi_slave_write(struct fsi_slave *slave, uint32_t addr,
>                         addr, val, size);
>  }
>
> +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 id)
>  {
> -       /* todo: initialise slave device, perform engine scan */
> +       struct fsi_slave *slave;
> +       uint32_t chip_id;
> +       uint8_t crc;
> +       int rc;
> +
> +       /* Currently, we only support single slaves on a link, and use the
> +        * full 23-bit address range
> +        */
> +       if (id != 0)
> +               return -EINVAL;
> +
> +       rc = fsi_master_read(master, link, id, 0, &chip_id, sizeof(chip_id));
> +       if (rc) {
> +               dev_warn(&master->dev, "can't read slave %02x:%02x %d\n",
> +                               link, id, rc);

When I boot a system with this driver loaded, I get his warning:

[    9.740000] usbhid: USB HID core driver
[    9.840000]  fsi0: can't read slave 00:00 -5
[    9.840000] NET: Registered protocol family 10

Two things:

There's a space in front of "fsi0".

This warning isn't useful at that point. The slave is not readable as
the FSI master is not present (the P9 hasn't been turned on). Can we
avoid printing the warning at boot?

Cheers,

Joel

> +               return -ENODEV;
> +       }
> +       chip_id = be32_to_cpu(chip_id);
> +
> +       crc = fsi_crc4(0, chip_id, 32);
> +       if (crc) {
> +               dev_warn(&master->dev, "slave %02x:%02x invalid chip id CRC!\n",
> +                               link, id);
> +               return -EIO;
> +       }
> +
> +       dev_info(&master->dev, "fsi: found chip %08x at %02x:%02x:%02x\n",
> +                       chip_id, master->idx, link, id);
> +
> +       /* We can communicate with a slave; create the slave device and
> +        * register.
> +        */
> +       slave = kzalloc(sizeof(*slave), GFP_KERNEL);
> +       if (!slave)
> +               return -ENOMEM;
> +
> +       slave->master = master;
> +       slave->dev.parent = &master->dev;
> +       slave->dev.release = fsi_slave_release;
> +       slave->link = link;
> +       slave->id = id;
> +       slave->size = FSI_SLAVE_SIZE_23b;
> +
> +       dev_set_name(&slave->dev, "slave@%02x:%02x", link, 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;
> +       }
> +
> +       /* todo: perform engine scan */
>
> -       return -ENODEV;
> +       return rc;
>  }
>
>  /* FSI master support */
> --
> 1.8.2.2
>

[PATCH v6 07/23] drivers/fsi: Implement slave initialisation

[Christopher Bostic]

On 5/2/17 1:24 AM, Joel Stanley wrote:
> On Tue, Apr 11, 2017 at 5:16 AM, Christopher Bostic
> <cbostic@linux.vnet.ibm.com> wrote:
>> From: Jeremy Kerr <jk@ozlabs.org>
>>
>> Implement fsi_slave_init: if we can read a chip ID, create fsi_slave
>> devices and register with the driver core.
>>
>> Includes changes from Chris Bostic <cbostic@linux.vnet.ibm.com>.
>>
>> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
>> Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> ---
>>   drivers/fsi/fsi-core.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++++--
>>   1 file changed, 64 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
>> index 6e1cfdf..c705ca2 100644
>> --- a/drivers/fsi/fsi-core.c
>> +++ b/drivers/fsi/fsi-core.c
>> @@ -17,9 +17,12 @@
>>   #include <linux/fsi.h>
>>   #include <linux/idr.h>
>>   #include <linux/module.h>
>> +#include <linux/slab.h>
>>
>>   #include "fsi-master.h"
>>
>> +#define FSI_SLAVE_SIZE_23b             0x800000
>> +
>>   static DEFINE_IDA(master_ida);
>>
>>   struct fsi_slave {
>> @@ -114,11 +117,70 @@ static int fsi_slave_write(struct fsi_slave *slave, uint32_t addr,
>>                          addr, val, size);
>>   }
>>
>> +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 id)
>>   {
>> -       /* todo: initialise slave device, perform engine scan */
>> +       struct fsi_slave *slave;
>> +       uint32_t chip_id;
>> +       uint8_t crc;
>> +       int rc;
>> +
>> +       /* Currently, we only support single slaves on a link, and use the
>> +        * full 23-bit address range
>> +        */
>> +       if (id != 0)
>> +               return -EINVAL;
>> +
>> +       rc = fsi_master_read(master, link, id, 0, &chip_id, sizeof(chip_id));
>> +       if (rc) {
>> +               dev_warn(&master->dev, "can't read slave %02x:%02x %d\n",
>> +                               link, id, rc);
> When I boot a system with this driver loaded, I get his warning:
>
> [    9.740000] usbhid: USB HID core driver
> [    9.840000]  fsi0: can't read slave 00:00 -5
> [    9.840000] NET: Registered protocol family 10
>
> Two things:
>
> There's a space in front of "fsi0".

Hi Joel,

Will correct.
>
> This warning isn't useful at that point. The slave is not readable as
> the FSI master is not present (the P9 hasn't been turned on). Can we
> avoid printing the warning at boot?

Yes agreed.  Will remove the warning in that case.

Thanks,
Chris
>
> Cheers,
>
> Joel
>
>> +               return -ENODEV;
>> +       }
>> +       chip_id = be32_to_cpu(chip_id);
>> +
>> +       crc = fsi_crc4(0, chip_id, 32);
>> +       if (crc) {
>> +               dev_warn(&master->dev, "slave %02x:%02x invalid chip id CRC!\n",
>> +                               link, id);
>> +               return -EIO;
>> +       }
>> +
>> +       dev_info(&master->dev, "fsi: found chip %08x at %02x:%02x:%02x\n",
>> +                       chip_id, master->idx, link, id);
>> +
>> +       /* We can communicate with a slave; create the slave device and
>> +        * register.
>> +        */
>> +       slave = kzalloc(sizeof(*slave), GFP_KERNEL);
>> +       if (!slave)
>> +               return -ENOMEM;
>> +
>> +       slave->master = master;
>> +       slave->dev.parent = &master->dev;
>> +       slave->dev.release = fsi_slave_release;
>> +       slave->link = link;
>> +       slave->id = id;
>> +       slave->size = FSI_SLAVE_SIZE_23b;
>> +
>> +       dev_set_name(&slave->dev, "slave@%02x:%02x", link, 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;
>> +       }
>> +
>> +       /* todo: perform engine scan */
>>
>> -       return -ENODEV;
>> +       return rc;
>>   }
>>
>>   /* FSI master support */
>> --
>> 1.8.2.2
>>

[PATCH v6 08/23] drivers/fsi: Set slave SMODE to init communication

[Christopher Bostic]

From: Chris Bostic <cbostic@linux.vnet.ibm.com>

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

Includes changes from Jeremy Kerr <jk@ozlabs.org>.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/fsi/fsi-core.c | 75 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 75 insertions(+)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index c705ca2..b7b138b 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -21,6 +21,27 @@
 
 #include "fsi-master.h"
 
+#define FSI_SLAVE_BASE			0x800
+
+/*
+ * FSI slave engine control register offsets
+ */
+#define FSI_SMODE			0x0	/* R/W: Mode register */
+
+/*
+ * SMODE fields
+ */
+#define FSI_SMODE_WSC		0x80000000	/* Warm start done */
+#define FSI_SMODE_ECRC		0x20000000	/* Hw CRC check */
+#define FSI_SMODE_SID_SHIFT	24		/* ID shift */
+#define FSI_SMODE_SID_MASK	3		/* ID Mask */
+#define FSI_SMODE_ED_SHIFT	20		/* Echo delay shift */
+#define FSI_SMODE_ED_MASK	0xf		/* Echo delay mask */
+#define FSI_SMODE_SD_SHIFT	16		/* Send delay shift */
+#define FSI_SMODE_SD_MASK	0xf		/* Send delay mask */
+#define FSI_SMODE_LBCRR_SHIFT	8		/* Clk ratio shift */
+#define FSI_SMODE_LBCRR_MASK	0xf		/* Clk ratio mask */
+
 #define FSI_SLAVE_SIZE_23b		0x800000
 
 static DEFINE_IDA(master_ida);
@@ -117,6 +138,52 @@ static int fsi_slave_write(struct fsi_slave *slave, uint32_t addr,
 			addr, val, size);
 }
 
+/* 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 const uint32_t fsi_slave_smode(int id)
+{
+	return FSI_SMODE_WSC | FSI_SMODE_ECRC
+		| fsi_smode_sid(id)
+		| fsi_smode_echodly(0xf) | fsi_smode_senddly(0xf)
+		| fsi_smode_lbcrr(0x8);
+}
+
+static int fsi_slave_set_smode(struct fsi_master *master, int link, int id)
+{
+	uint32_t smode;
+
+	/* set our smode register with the slave ID field to 0; this enables
+	 * extended slave addressing
+	 */
+	smode = fsi_slave_smode(id);
+	smode = cpu_to_be32(smode);
+
+	return fsi_master_write(master, link, id, FSI_SLAVE_BASE + FSI_SMODE,
+			&smode, sizeof(smode));
+}
+
 static void fsi_slave_release(struct device *dev)
 {
 	struct fsi_slave *slave = to_fsi_slave(dev);
@@ -155,6 +222,14 @@ static int fsi_slave_init(struct fsi_master *master, int link, uint8_t id)
 	dev_info(&master->dev, "fsi: found chip %08x at %02x:%02x:%02x\n",
 			chip_id, master->idx, link, id);
 
+	rc = fsi_slave_set_smode(master, link, id);
+	if (rc) {
+		dev_warn(&master->dev,
+				"can't set smode on slave:%02x:%02x %d\n",
+				link, id, rc);
+		return -ENODEV;
+	}
+
 	/* We can communicate with a slave; create the slave device and
 	 * register.
 	 */
-- 
1.8.2.2

[PATCH v6 09/23] drivers/fsi: scan slaves & register devices

[Christopher 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@linux.vnet.ibm.com>

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/fsi/fsi-core.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++--
 include/linux/fsi.h    |   4 ++
 2 files changed, 128 insertions(+), 3 deletions(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index b7b138b..a8faa89 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -21,6 +21,19 @@
 
 #include "fsi-master.h"
 
+#define FSI_SLAVE_CONF_NEXT_MASK	0x80000000
+#define FSI_SLAVE_CONF_SLOTS_MASK	0x00ff0000
+#define FSI_SLAVE_CONF_SLOTS_SHIFT	16
+#define FSI_SLAVE_CONF_VERSION_MASK	0x0000f000
+#define FSI_SLAVE_CONF_VERSION_SHIFT	12
+#define FSI_SLAVE_CONF_TYPE_MASK	0x00000ff0
+#define FSI_SLAVE_CONF_TYPE_SHIFT	4
+#define FSI_SLAVE_CONF_CRC_SHIFT	4
+#define FSI_SLAVE_CONF_CRC_MASK		0x0000000f
+#define FSI_SLAVE_CONF_DATA_BITS	28
+
+static const int engine_page_size = 0x400;
+
 #define FSI_SLAVE_BASE			0x800
 
 /*
@@ -61,6 +74,30 @@ static int fsi_master_read(struct fsi_master *master, int link,
 static int fsi_master_write(struct fsi_master *master, int link,
 		uint8_t slave_id, uint32_t addr, const void *val, size_t size);
 
+/* 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,
@@ -138,6 +175,91 @@ static int fsi_slave_write(struct fsi_slave *slave, uint32_t addr,
 			addr, val, size);
 }
 
+static int fsi_slave_scan(struct fsi_slave *slave)
+{
+	uint32_t engine_addr;
+	uint32_t conf;
+	int rc, i;
+
+	/*
+	 * scan engines
+	 *
+	 * We keep the peek mode and slave engines for the core; so start
+	 * at the third slot in the configuration table. We also need to
+	 * skip the chip ID entry at the start of the address space.
+	 */
+	engine_addr = engine_page_size * 3;
+	for (i = 2; i < engine_page_size / sizeof(uint32_t); i++) {
+		uint8_t slots, version, type, crc;
+		struct fsi_device *dev;
+
+		rc = fsi_slave_read(slave, (i + 1) * sizeof(conf),
+				&conf, sizeof(conf));
+		if (rc) {
+			dev_warn(&slave->dev,
+				"error reading slave registers\n");
+			return -1;
+		}
+		conf = be32_to_cpu(conf);
+
+		crc = fsi_crc4(0, conf, 32);
+		if (crc) {
+			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 && slots != 0) {
+
+			/* create device */
+			dev = fsi_create_device(slave);
+			if (!dev)
+				return -ENOMEM;
+
+			dev->slave = slave;
+			dev->engine_type = type;
+			dev->version = version;
+			dev->unit = i;
+			dev->addr = engine_addr;
+			dev->size = slots * engine_page_size;
+
+			dev_info(&slave->dev,
+			"engine[%i]: type %x, version %x, addr %x size %x\n",
+					dev->unit, dev->engine_type, version,
+					dev->addr, dev->size);
+
+			dev_set_name(&dev->dev, "%02x:%02x:%02x:%02x",
+					slave->master->idx, slave->link,
+					slave->id, i - 2);
+
+			rc = device_register(&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;
+}
+
 /* Encode slave local bus echo delay */
 static inline uint32_t fsi_smode_echodly(int x)
 {
@@ -253,9 +375,8 @@ static int fsi_slave_init(struct fsi_master *master, int link, uint8_t id)
 		return rc;
 	}
 
-	/* todo: perform engine scan */
-
-	return rc;
+	fsi_slave_scan(slave);
+	return 0;
 }
 
 /* FSI master support */
diff --git a/include/linux/fsi.h b/include/linux/fsi.h
index 273cbf6..efa55ba 100644
--- a/include/linux/fsi.h
+++ b/include/linux/fsi.h
@@ -21,6 +21,10 @@ struct fsi_device {
 	struct device		dev;
 	u8			engine_type;
 	u8			version;
+	u8			unit;
+	struct fsi_slave	*slave;
+	uint32_t		addr;
+	uint32_t		size;
 };
 
 struct fsi_device_id {
-- 
1.8.2.2

[PATCH v6 09/23] drivers/fsi: scan slaves & register devices

[Joel Stanley]

On Tue, Apr 11, 2017 at 5:16 AM, Christopher Bostic
<cbostic@linux.vnet.ibm.com> wrote:
> 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@linux.vnet.ibm.com>
>
> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
> Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  drivers/fsi/fsi-core.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++--
>  include/linux/fsi.h    |   4 ++
>  2 files changed, 128 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
> index b7b138b..a8faa89 100644
> --- a/drivers/fsi/fsi-core.c
> +++ b/drivers/fsi/fsi-core.c
> @@ -21,6 +21,19 @@
>
>  #include "fsi-master.h"
>
> +#define FSI_SLAVE_CONF_NEXT_MASK       0x80000000
> +#define FSI_SLAVE_CONF_SLOTS_MASK      0x00ff0000
> +#define FSI_SLAVE_CONF_SLOTS_SHIFT     16
> +#define FSI_SLAVE_CONF_VERSION_MASK    0x0000f000
> +#define FSI_SLAVE_CONF_VERSION_SHIFT   12
> +#define FSI_SLAVE_CONF_TYPE_MASK       0x00000ff0
> +#define FSI_SLAVE_CONF_TYPE_SHIFT      4
> +#define FSI_SLAVE_CONF_CRC_SHIFT       4
> +#define FSI_SLAVE_CONF_CRC_MASK                0x0000000f
> +#define FSI_SLAVE_CONF_DATA_BITS       28

You could use GENAMSK for these. It would make it easier to check eg.
that 0x00ff0000 needs to be shifted by 16.


> +
> +static const int engine_page_size = 0x400;
> +
>  #define FSI_SLAVE_BASE                 0x800
>
>  /*
> @@ -61,6 +74,30 @@ static int fsi_master_read(struct fsi_master *master, int link,
>  static int fsi_master_write(struct fsi_master *master, int link,
>                 uint8_t slave_id, uint32_t addr, const void *val, size_t size);
>
> +/* 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,
> @@ -138,6 +175,91 @@ static int fsi_slave_write(struct fsi_slave *slave, uint32_t addr,
>                         addr, val, size);
>  }
>
> +static int fsi_slave_scan(struct fsi_slave *slave)
> +{
> +       uint32_t engine_addr;
> +       uint32_t conf;
> +       int rc, i;
> +
> +       /*
> +        * scan engines
> +        *
> +        * We keep the peek mode and slave engines for the core; so start
> +        * at the third slot in the configuration table. We also need to
> +        * skip the chip ID entry at the start of the address space.
> +        */
> +       engine_addr = engine_page_size * 3;
> +       for (i = 2; i < engine_page_size / sizeof(uint32_t); i++) {
> +               uint8_t slots, version, type, crc;
> +               struct fsi_device *dev;
> +
> +               rc = fsi_slave_read(slave, (i + 1) * sizeof(conf),
> +                               &conf, sizeof(conf));
> +               if (rc) {
> +                       dev_warn(&slave->dev,
> +                               "error reading slave registers\n");
> +                       return -1;
> +               }
> +               conf = be32_to_cpu(conf);
> +
> +               crc = fsi_crc4(0, conf, 32);
> +               if (crc) {
> +                       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 && slots != 0) {
> +
> +                       /* create device */
> +                       dev = fsi_create_device(slave);
> +                       if (!dev)
> +                               return -ENOMEM;
> +
> +                       dev->slave = slave;
> +                       dev->engine_type = type;
> +                       dev->version = version;
> +                       dev->unit = i;
> +                       dev->addr = engine_addr;
> +                       dev->size = slots * engine_page_size;
> +
> +                       dev_info(&slave->dev,
> +                       "engine[%i]: type %x, version %x, addr %x size %x\n",
> +                                       dev->unit, dev->engine_type, version,
> +                                       dev->addr, dev->size);

This produces a lot of noise in the kernel log. I suggest making it
drv_dbg if you require it. If you don't then remove it.

> +
> +                       dev_set_name(&dev->dev, "%02x:%02x:%02x:%02x",
> +                                       slave->master->idx, slave->link,
> +                                       slave->id, i - 2);
> +
> +                       rc = device_register(&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;
> +}
> +
>  /* Encode slave local bus echo delay */
>  static inline uint32_t fsi_smode_echodly(int x)
>  {
> @@ -253,9 +375,8 @@ static int fsi_slave_init(struct fsi_master *master, int link, uint8_t id)
>                 return rc;
>         }
>
> -       /* todo: perform engine scan */
> -
> -       return rc;
> +       fsi_slave_scan(slave);
> +       return 0;

You don't check the return value of fsi_slave_scan.

>  }
>
>  /* FSI master support */
> diff --git a/include/linux/fsi.h b/include/linux/fsi.h
> index 273cbf6..efa55ba 100644
> --- a/include/linux/fsi.h
> +++ b/include/linux/fsi.h
> @@ -21,6 +21,10 @@ struct fsi_device {
>         struct device           dev;
>         u8                      engine_type;
>         u8                      version;
> +       u8                      unit;
> +       struct fsi_slave        *slave;
> +       uint32_t                addr;
> +       uint32_t                size;
>  };
>
>  struct fsi_device_id {
> --
> 1.8.2.2
>

[PATCH v6 09/23] drivers/fsi: scan slaves & register devices

[Christopher Bostic]

On 5/10/17 2:30 AM, Joel Stanley wrote:
> On Tue, Apr 11, 2017 at 5:16 AM, Christopher Bostic
> <cbostic@linux.vnet.ibm.com> wrote:
>> 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@linux.vnet.ibm.com>
>>
>> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
>> Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> ---
>>   drivers/fsi/fsi-core.c | 127 +++++++++++++++++++++++++++++++++++++++++++++++--
>>   include/linux/fsi.h    |   4 ++
>>   2 files changed, 128 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
>> index b7b138b..a8faa89 100644
>> --- a/drivers/fsi/fsi-core.c
>> +++ b/drivers/fsi/fsi-core.c
>> @@ -21,6 +21,19 @@
>>
>>   #include "fsi-master.h"
>>
>> +#define FSI_SLAVE_CONF_NEXT_MASK       0x80000000
>> +#define FSI_SLAVE_CONF_SLOTS_MASK      0x00ff0000
>> +#define FSI_SLAVE_CONF_SLOTS_SHIFT     16
>> +#define FSI_SLAVE_CONF_VERSION_MASK    0x0000f000
>> +#define FSI_SLAVE_CONF_VERSION_SHIFT   12
>> +#define FSI_SLAVE_CONF_TYPE_MASK       0x00000ff0
>> +#define FSI_SLAVE_CONF_TYPE_SHIFT      4
>> +#define FSI_SLAVE_CONF_CRC_SHIFT       4
>> +#define FSI_SLAVE_CONF_CRC_MASK                0x0000000f
>> +#define FSI_SLAVE_CONF_DATA_BITS       28
> You could use GENAMSK for these. It would make it easier to check eg.
> that 0x00ff0000 needs to be shifted by 16.

Hi Joel,

OK will update to use GENMASK.
>
>> +
>> +static const int engine_page_size = 0x400;
>> +
>>   #define FSI_SLAVE_BASE                 0x800
>>
>>   /*
>> @@ -61,6 +74,30 @@ static int fsi_master_read(struct fsi_master *master, int link,
>>   static int fsi_master_write(struct fsi_master *master, int link,
>>                  uint8_t slave_id, uint32_t addr, const void *val, size_t size);
>>
>> +/* 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,
>> @@ -138,6 +175,91 @@ static int fsi_slave_write(struct fsi_slave *slave, uint32_t addr,
>>                          addr, val, size);
>>   }
>>
>> +static int fsi_slave_scan(struct fsi_slave *slave)
>> +{
>> +       uint32_t engine_addr;
>> +       uint32_t conf;
>> +       int rc, i;
>> +
>> +       /*
>> +        * scan engines
>> +        *
>> +        * We keep the peek mode and slave engines for the core; so start
>> +        * at the third slot in the configuration table. We also need to
>> +        * skip the chip ID entry at the start of the address space.
>> +        */
>> +       engine_addr = engine_page_size * 3;
>> +       for (i = 2; i < engine_page_size / sizeof(uint32_t); i++) {
>> +               uint8_t slots, version, type, crc;
>> +               struct fsi_device *dev;
>> +
>> +               rc = fsi_slave_read(slave, (i + 1) * sizeof(conf),
>> +                               &conf, sizeof(conf));
>> +               if (rc) {
>> +                       dev_warn(&slave->dev,
>> +                               "error reading slave registers\n");
>> +                       return -1;
>> +               }
>> +               conf = be32_to_cpu(conf);
>> +
>> +               crc = fsi_crc4(0, conf, 32);
>> +               if (crc) {
>> +                       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 && slots != 0) {
>> +
>> +                       /* create device */
>> +                       dev = fsi_create_device(slave);
>> +                       if (!dev)
>> +                               return -ENOMEM;
>> +
>> +                       dev->slave = slave;
>> +                       dev->engine_type = type;
>> +                       dev->version = version;
>> +                       dev->unit = i;
>> +                       dev->addr = engine_addr;
>> +                       dev->size = slots * engine_page_size;
>> +
>> +                       dev_info(&slave->dev,
>> +                       "engine[%i]: type %x, version %x, addr %x size %x\n",
>> +                                       dev->unit, dev->engine_type, version,
>> +                                       dev->addr, dev->size);
> This produces a lot of noise in the kernel log. I suggest making it
> drv_dbg if you require it. If you don't then remove it.

Will need for debug purposes so will change to dev_dbg( ).
>
>> +
>> +                       dev_set_name(&dev->dev, "%02x:%02x:%02x:%02x",
>> +                                       slave->master->idx, slave->link,
>> +                                       slave->id, i - 2);
>> +
>> +                       rc = device_register(&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;
>> +}
>> +
>>   /* Encode slave local bus echo delay */
>>   static inline uint32_t fsi_smode_echodly(int x)
>>   {
>> @@ -253,9 +375,8 @@ static int fsi_slave_init(struct fsi_master *master, int link, uint8_t id)
>>                  return rc;
>>          }
>>
>> -       /* todo: perform engine scan */
>> -
>> -       return rc;
>> +       fsi_slave_scan(slave);
>> +       return 0;
> You don't check the return value of fsi_slave_scan.
Will correct.

Thanks,
-Chris

>>   }
>>
>>   /* FSI master support */
>> diff --git a/include/linux/fsi.h b/include/linux/fsi.h
>> index 273cbf6..efa55ba 100644
>> --- a/include/linux/fsi.h
>> +++ b/include/linux/fsi.h
>> @@ -21,6 +21,10 @@ struct fsi_device {
>>          struct device           dev;
>>          u8                      engine_type;
>>          u8                      version;
>> +       u8                      unit;
>> +       struct fsi_slave        *slave;
>> +       uint32_t                addr;
>> +       uint32_t                size;
>>   };
>>
>>   struct fsi_device_id {
>> --
>> 1.8.2.2
>>

[PATCH v6 10/23] drivers/fsi: Add device read/write/peek API

[Christopher 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.

Includes contributions from Chris Bostic <cbostic@linux.vnet.ibm.com>
and Edward A. James <eajames@us.ibm.com>.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/fsi/fsi-core.c | 33 +++++++++++++++++++++++++++++++++
 include/linux/fsi.h    |  7 ++++++-
 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index a8faa89..4da0b030 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -32,6 +32,8 @@
 #define FSI_SLAVE_CONF_CRC_MASK		0x0000000f
 #define FSI_SLAVE_CONF_DATA_BITS	28
 
+#define FSI_PEEK_BASE			0x410
+
 static const int engine_page_size = 0x400;
 
 #define FSI_SLAVE_BASE			0x800
@@ -73,9 +75,40 @@ static int fsi_master_read(struct fsi_master *master, int link,
 		uint8_t slave_id, uint32_t addr, void *val, size_t size);
 static int fsi_master_write(struct fsi_master *master, int link,
 		uint8_t slave_id, uint32_t addr, const void *val, size_t size);
+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 || size > dev->size || addr > dev->size - size)
+		return -EINVAL;
+
+	return fsi_slave_read(dev->slave, dev->addr + addr, val, size);
+}
+EXPORT_SYMBOL_GPL(fsi_device_read);
+
+int fsi_device_write(struct fsi_device *dev, uint32_t addr, const void *val,
+		size_t size)
+{
+	if (addr > dev->size || size > dev->size || addr > dev->size - size)
+		return -EINVAL;
+
+	return fsi_slave_write(dev->slave, dev->addr + addr, val, size);
+}
+EXPORT_SYMBOL_GPL(fsi_device_write);
+
+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)
 {
 	struct fsi_device *device = to_fsi_dev(_device);
diff --git a/include/linux/fsi.h b/include/linux/fsi.h
index efa55ba..66bce48 100644
--- a/include/linux/fsi.h
+++ b/include/linux/fsi.h
@@ -27,6 +27,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;
@@ -40,7 +46,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

[v6,10/23] drivers/fsi: Add device read/write/peek API

[Brad Bishop]

> On Apr 10, 2017, at 3:46 PM, Christopher Bostic <cbostic@linux.vnet.ibm.com> wrote:
> 
> From: Jeremy Kerr <jk@ozlabs.org>
> 
> This change introduces the fsi device API: simple read, write and peek
> accessors for the devices' address spaces.
> 
> Includes contributions from Chris Bostic <cbostic@linux.vnet.ibm.com>
> and Edward A. James <eajames@us.ibm.com>.
> 
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
> Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> drivers/fsi/fsi-core.c | 33 +++++++++++++++++++++++++++++++++
> include/linux/fsi.h    |  7 ++++++-
> 2 files changed, 39 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
> index a8faa89..4da0b030 100644
> --- a/drivers/fsi/fsi-core.c
> +++ b/drivers/fsi/fsi-core.c
> @@ -32,6 +32,8 @@
> #define FSI_SLAVE_CONF_CRC_MASK		0x0000000f
> #define FSI_SLAVE_CONF_DATA_BITS	28
> 
> +#define FSI_PEEK_BASE			0x410
> +
> static const int engine_page_size = 0x400;
> 
> #define FSI_SLAVE_BASE			0x800
> @@ -73,9 +75,40 @@ static int fsi_master_read(struct fsi_master *master, int link,
> 		uint8_t slave_id, uint32_t addr, void *val, size_t size);
> static int fsi_master_write(struct fsi_master *master, int link,
> 		uint8_t slave_id, uint32_t addr, const void *val, size_t size);
> +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 || size > dev->size || addr > dev->size - size)
> +		return -EINVAL;
> +
> +	return fsi_slave_read(dev->slave, dev->addr + addr, val, size);
> +}
> +EXPORT_SYMBOL_GPL(fsi_device_read);
> +
> +int fsi_device_write(struct fsi_device *dev, uint32_t addr, const void *val,
> +		size_t size)
> +{
> +	if (addr > dev->size || size > dev->size || addr > dev->size - size)
> +		return -EINVAL;
> +
> +	return fsi_slave_write(dev->slave, dev->addr + addr, val, size);
> +}
> +EXPORT_SYMBOL_GPL(fsi_device_write);
> +
> +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)
> {
> 	struct fsi_device *device = to_fsi_dev(_device);
> diff --git a/include/linux/fsi.h b/include/linux/fsi.h
> index efa55ba..66bce48 100644
> --- a/include/linux/fsi.h
> +++ b/include/linux/fsi.h
> @@ -27,6 +27,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;
> @@ -40,7 +46,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;

I wrote a driver using this API.  It seems to be suitable enough.

Acked-by: Brad Bishop <bradleyb@fuzziesquirrel.com>

[PATCH v6 10/23] drivers/fsi: Add device read/write/peek API

[Joel Stanley]

On Tue, Apr 11, 2017 at 5:16 AM, Christopher Bostic
<cbostic@linux.vnet.ibm.com> wrote:
> From: Jeremy Kerr <jk@ozlabs.org>
>
> This change introduces the fsi device API: simple read, write and peek
> accessors for the devices' address spaces.
>
> Includes contributions from Chris Bostic <cbostic@linux.vnet.ibm.com>
> and Edward A. James <eajames@us.ibm.com>.
>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
> Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  drivers/fsi/fsi-core.c | 33 +++++++++++++++++++++++++++++++++
>  include/linux/fsi.h    |  7 ++++++-
>  2 files changed, 39 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
> index a8faa89..4da0b030 100644
> --- a/drivers/fsi/fsi-core.c
> +++ b/drivers/fsi/fsi-core.c
> @@ -32,6 +32,8 @@
>  #define FSI_SLAVE_CONF_CRC_MASK                0x0000000f
>  #define FSI_SLAVE_CONF_DATA_BITS       28
>
> +#define FSI_PEEK_BASE                  0x410
> +
>  static const int engine_page_size = 0x400;
>
>  #define FSI_SLAVE_BASE                 0x800
> @@ -73,9 +75,40 @@ static int fsi_master_read(struct fsi_master *master, int link,
>                 uint8_t slave_id, uint32_t addr, void *val, size_t size);
>  static int fsi_master_write(struct fsi_master *master, int link,
>                 uint8_t slave_id, uint32_t addr, const void *val, size_t size);
> +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);

I don't think these
>
>  /* FSI endpoint-device support */
>
> +int fsi_device_read(struct fsi_device *dev, uint32_t addr, void *val,
> +               size_t size)

I suggest adding some comments about the use of fsi_device_read,
fsi_device_write and fsi_device_peek APIs for driver authors.

One important note is that the data in val must be FSI bus endian (big endian).

It would be nice to have the sparse notation (eg. __be32) as well.
That doesn't make sense for void *, but we could add some helper
functions like:

 int fsi_device_read32(struct fsi_device *dev, uint32_t addr, __be32 *val)

 int fsi_device_write32(struct fsi_device *dev, uint32_t addr, __be32 val)

We probably only need a 32 bit version, as all of the reads/writes
being done in users of this function are 32 bit.

What do you think?

> +{
> +       if (addr > dev->size || size > dev->size || addr > dev->size - size)
> +               return -EINVAL;
> +
> +       return fsi_slave_read(dev->slave, dev->addr + addr, val, size);
> +}
> +EXPORT_SYMBOL_GPL(fsi_device_read);
> +
> +int fsi_device_write(struct fsi_device *dev, uint32_t addr, const void *val,
> +               size_t size)
> +{
> +       if (addr > dev->size || size > dev->size || addr > dev->size - size)
> +               return -EINVAL;
> +
> +       return fsi_slave_write(dev->slave, dev->addr + addr, val, size);
> +}
> +EXPORT_SYMBOL_GPL(fsi_device_write);
> +
> +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)
>  {
>         struct fsi_device *device = to_fsi_dev(_device);
> diff --git a/include/linux/fsi.h b/include/linux/fsi.h
> index efa55ba..66bce48 100644
> --- a/include/linux/fsi.h
> +++ b/include/linux/fsi.h
> @@ -27,6 +27,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;
> @@ -40,7 +46,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 v6 10/23] drivers/fsi: Add device read/write/peek API

[Christopher Bostic]

On 5/10/17 3:13 AM, Joel Stanley wrote:
> On Tue, Apr 11, 2017 at 5:16 AM, Christopher Bostic
> <cbostic@linux.vnet.ibm.com> wrote:
>> From: Jeremy Kerr <jk@ozlabs.org>
>>
>> This change introduces the fsi device API: simple read, write and peek
>> accessors for the devices' address spaces.
>>
>> Includes contributions from Chris Bostic <cbostic@linux.vnet.ibm.com>
>> and Edward A. James <eajames@us.ibm.com>.
>>
>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
>> Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> ---
>>   drivers/fsi/fsi-core.c | 33 +++++++++++++++++++++++++++++++++
>>   include/linux/fsi.h    |  7 ++++++-
>>   2 files changed, 39 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
>> index a8faa89..4da0b030 100644
>> --- a/drivers/fsi/fsi-core.c
>> +++ b/drivers/fsi/fsi-core.c
>> @@ -32,6 +32,8 @@
>>   #define FSI_SLAVE_CONF_CRC_MASK                0x0000000f
>>   #define FSI_SLAVE_CONF_DATA_BITS       28
>>
>> +#define FSI_PEEK_BASE                  0x410
>> +
>>   static const int engine_page_size = 0x400;
>>
>>   #define FSI_SLAVE_BASE                 0x800
>> @@ -73,9 +75,40 @@ static int fsi_master_read(struct fsi_master *master, int link,
>>                  uint8_t slave_id, uint32_t addr, void *val, size_t size);
>>   static int fsi_master_write(struct fsi_master *master, int link,
>>                  uint8_t slave_id, uint32_t addr, const void *val, size_t size);
>> +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);
> I don't think these
>>   /* FSI endpoint-device support */
>>
>> +int fsi_device_read(struct fsi_device *dev, uint32_t addr, void *val,
>> +               size_t size)
> I suggest adding some comments about the use of fsi_device_read,
> fsi_device_write and fsi_device_peek APIs for driver authors.
>
> One important note is that the data in val must be FSI bus endian (big endian).

Hi Joel,

OK will add some comments here.
> It would be nice to have the sparse notation (eg. __be32) as well.
> That doesn't make sense for void *, but we could add some helper
> functions like:
>
>   int fsi_device_read32(struct fsi_device *dev, uint32_t addr, __be32 *val)
>
>   int fsi_device_write32(struct fsi_device *dev, uint32_t addr, __be32 val)
>
> We probably only need a 32 bit version, as all of the reads/writes
> being done in users of this function are 32 bit.
> What do you think?

These functions will need to support reads/writes of 8 bit and 16 bit 
values.

Thanks
-Chris

>> +{
>> +       if (addr > dev->size || size > dev->size || addr > dev->size - size)
>> +               return -EINVAL;
>> +
>> +       return fsi_slave_read(dev->slave, dev->addr + addr, val, size);
>> +}
>> +EXPORT_SYMBOL_GPL(fsi_device_read);
>> +
>> +int fsi_device_write(struct fsi_device *dev, uint32_t addr, const void *val,
>> +               size_t size)
>> +{
>> +       if (addr > dev->size || size > dev->size || addr > dev->size - size)
>> +               return -EINVAL;
>> +
>> +       return fsi_slave_write(dev->slave, dev->addr + addr, val, size);
>> +}
>> +EXPORT_SYMBOL_GPL(fsi_device_write);
>> +
>> +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)
>>   {
>>          struct fsi_device *device = to_fsi_dev(_device);
>> diff --git a/include/linux/fsi.h b/include/linux/fsi.h
>> index efa55ba..66bce48 100644
>> --- a/include/linux/fsi.h
>> +++ b/include/linux/fsi.h
>> @@ -27,6 +27,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;
>> @@ -40,7 +46,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 v6 11/23] drivers/fsi: Add master unscan

[Christopher Bostic]

From: Chris Bostic <cbostic@linux.vnet.ibm.com>

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

Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/fsi/fsi-core.c | 40 ++++++++++++++++++++++++++++++++++++++++
 1 file changed, 40 insertions(+)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 4da0b030..75d2a88 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -69,6 +69,7 @@ struct fsi_slave {
 	uint32_t		size;	/* size of slave address space */
 };
 
+#define to_fsi_master(d) container_of(d, struct fsi_master, dev)
 #define to_fsi_slave(d) container_of(d, struct fsi_slave, dev)
 
 static int fsi_master_read(struct fsi_master *master, int link,
@@ -491,6 +492,37 @@ static int fsi_master_scan(struct fsi_master *master)
 	return 0;
 }
 
+static int __fsi_slave_remove_device(struct device *dev, void *arg)
+{
+	device_unregister(dev);
+	return 0;
+}
+
+static int __fsi_master_remove_slave(struct device *dev, void *arg)
+{
+	device_for_each_child(dev, NULL, __fsi_slave_remove_device);
+	device_unregister(dev);
+	return 0;
+}
+
+static void fsi_master_unscan(struct fsi_master *master)
+{
+	device_for_each_child(&master->dev, NULL, __fsi_master_remove_slave);
+}
+
+static ssize_t master_rescan_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct fsi_master *master = to_fsi_master(dev);
+
+	fsi_master_unscan(master);
+	fsi_master_scan(master);
+
+	return count;
+}
+
+static DEVICE_ATTR(rescan, 0200, NULL, master_rescan_store);
+
 int fsi_master_register(struct fsi_master *master)
 {
 	int rc;
@@ -507,7 +539,15 @@ int fsi_master_register(struct fsi_master *master)
 		return rc;
 	}
 
+	rc = device_create_file(&master->dev, &dev_attr_rescan);
+	if (rc) {
+		device_unregister(&master->dev);
+		ida_simple_remove(&master_ida, master->idx);
+		return rc;
+	}
+
 	fsi_master_scan(master);
+
 	return 0;
 }
 EXPORT_SYMBOL_GPL(fsi_master_register);
-- 
1.8.2.2

[PATCH v6 11/23] drivers/fsi: Add master unscan

[Joel Stanley]

On Tue, Apr 11, 2017 at 5:16 AM, Christopher Bostic
<cbostic@linux.vnet.ibm.com> wrote:
> From: Chris Bostic <cbostic@linux.vnet.ibm.com>
>
> Allow a master to undo a previous scan.  Should a master scan a bus
> twice it will need to ensure it doesn't double register any
> previously detected device.
>
> Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  drivers/fsi/fsi-core.c | 40 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 40 insertions(+)
>
> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
> index 4da0b030..75d2a88 100644
> --- a/drivers/fsi/fsi-core.c
> +++ b/drivers/fsi/fsi-core.c
> @@ -69,6 +69,7 @@ struct fsi_slave {
>         uint32_t                size;   /* size of slave address space */
>  };
>
> +#define to_fsi_master(d) container_of(d, struct fsi_master, dev)
>  #define to_fsi_slave(d) container_of(d, struct fsi_slave, dev)
>
>  static int fsi_master_read(struct fsi_master *master, int link,
> @@ -491,6 +492,37 @@ static int fsi_master_scan(struct fsi_master *master)
>         return 0;
>  }
>
> +static int __fsi_slave_remove_device(struct device *dev, void *arg)
> +{
> +       device_unregister(dev);
> +       return 0;
> +}
> +
> +static int __fsi_master_remove_slave(struct device *dev, void *arg)
> +{
> +       device_for_each_child(dev, NULL, __fsi_slave_remove_device);
> +       device_unregister(dev);
> +       return 0;
> +}

I can't see why the two above functions to have the __ prefix.

> +
> +static void fsi_master_unscan(struct fsi_master *master)
> +{
> +       device_for_each_child(&master->dev, NULL, __fsi_master_remove_slave);
> +}
> +
> +static ssize_t master_rescan_store(struct device *dev,
> +               struct device_attribute *attr, const char *buf, size_t count)
> +{
> +       struct fsi_master *master = to_fsi_master(dev);
> +
> +       fsi_master_unscan(master);
> +       fsi_master_scan(master);

These function can return errors. Do you want to return those errors
to userspace?

> +
> +       return count;
> +}
> +
> +static DEVICE_ATTR(rescan, 0200, NULL, master_rescan_store);
> +
>  int fsi_master_register(struct fsi_master *master)
>  {
>         int rc;
> @@ -507,7 +539,15 @@ int fsi_master_register(struct fsi_master *master)
>                 return rc;
>         }
>
> +       rc = device_create_file(&master->dev, &dev_attr_rescan);
> +       if (rc) {
> +               device_unregister(&master->dev);
> +               ida_simple_remove(&master_ida, master->idx);
> +               return rc;
> +       }
> +
>         fsi_master_scan(master);
> +
>         return 0;
>  }
>  EXPORT_SYMBOL_GPL(fsi_master_register);
> --
> 1.8.2.2
>

[PATCH v6 11/23] drivers/fsi: Add master unscan

[Christopher Bostic]

On 5/10/17 2:31 AM, Joel Stanley wrote:
> On Tue, Apr 11, 2017 at 5:16 AM, Christopher Bostic
> <cbostic@linux.vnet.ibm.com> wrote:
>> From: Chris Bostic <cbostic@linux.vnet.ibm.com>
>>
>> Allow a master to undo a previous scan.  Should a master scan a bus
>> twice it will need to ensure it doesn't double register any
>> previously detected device.
>>
>> Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> ---
>>   drivers/fsi/fsi-core.c | 40 ++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 40 insertions(+)
>>
>> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
>> index 4da0b030..75d2a88 100644
>> --- a/drivers/fsi/fsi-core.c
>> +++ b/drivers/fsi/fsi-core.c
>> @@ -69,6 +69,7 @@ struct fsi_slave {
>>          uint32_t                size;   /* size of slave address space */
>>   };
>>
>> +#define to_fsi_master(d) container_of(d, struct fsi_master, dev)
>>   #define to_fsi_slave(d) container_of(d, struct fsi_slave, dev)
>>
>>   static int fsi_master_read(struct fsi_master *master, int link,
>> @@ -491,6 +492,37 @@ static int fsi_master_scan(struct fsi_master *master)
>>          return 0;
>>   }
>>
>> +static int __fsi_slave_remove_device(struct device *dev, void *arg)
>> +{
>> +       device_unregister(dev);
>> +       return 0;
>> +}
>> +
>> +static int __fsi_master_remove_slave(struct device *dev, void *arg)
>> +{
>> +       device_for_each_child(dev, NULL, __fsi_slave_remove_device);
>> +       device_unregister(dev);
>> +       return 0;
>> +}
> I can't see why the two above functions to have the __ prefix.

Jeremy Kerr had introduced this convention.  Jeremy can you comment on 
this?

>
>> +
>> +static void fsi_master_unscan(struct fsi_master *master)
>> +{
>> +       device_for_each_child(&master->dev, NULL, __fsi_master_remove_slave);
>> +}
>> +
>> +static ssize_t master_rescan_store(struct device *dev,
>> +               struct device_attribute *attr, const char *buf, size_t count)
>> +{
>> +       struct fsi_master *master = to_fsi_master(dev);
>> +
>> +       fsi_master_unscan(master);
>> +       fsi_master_scan(master);
> These function can return errors. Do you want to return those errors
> to userspace?

That would be the best approach yes... Will change.

>
>> +
>> +       return count;
>> +}
>> +
>> +static DEVICE_ATTR(rescan, 0200, NULL, master_rescan_store);
>> +
>>   int fsi_master_register(struct fsi_master *master)
>>   {
>>          int rc;
>> @@ -507,7 +539,15 @@ int fsi_master_register(struct fsi_master *master)
>>                  return rc;
>>          }
>>
>> +       rc = device_create_file(&master->dev, &dev_attr_rescan);
>> +       if (rc) {
>> +               device_unregister(&master->dev);
>> +               ida_simple_remove(&master_ida, master->idx);
>> +               return rc;
>> +       }
>> +
>>          fsi_master_scan(master);
>> +
>>          return 0;
>>   }
>>   EXPORT_SYMBOL_GPL(fsi_master_register);
>> --
>> 1.8.2.2
>>

[PATCH v6 12/23] drivers/fsi: Add documentation for GPIO bindings

[Christopher Bostic]

From: Chris Bostic <cbostic@linux.vnet.ibm.com>

Add fsi master gpio device tree binding documentation

Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
Signed-off-by: Joel Stanley <joel@jms.id.au>
Acked-by: Rob Herring <robh@kernel.org>
---
 .../devicetree/bindings/fsi/fsi-master-gpio.txt    | 24 ++++++++++++++++++++++
 1 file changed, 24 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt

diff --git a/Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt b/Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt
new file mode 100644
index 0000000..a767259
--- /dev/null
+++ b/Documentation/devicetree/bindings/fsi/fsi-master-gpio.txt
@@ -0,0 +1,24 @@
+Device-tree bindings for gpio-based FSI master driver
+-----------------------------------------------------
+
+Required properties:
+ - compatible = "fsi-master-gpio";
+ - clock-gpios = <gpio-descriptor>;	: GPIO for FSI clock
+ - data-gpios = <gpio-descriptor>;	: GPIO for FSI data signal
+
+Optional properties:
+ - enable-gpios = <gpio-descriptor>;	: GPIO for enable signal
+ - trans-gpios = <gpio-descriptor>;	: GPIO for voltage translator enable
+ - mux-gpios = <gpio-descriptor>;	: GPIO for pin multiplexing with other
+                                          functions (eg, external FSI masters)
+
+Examples:
+
+    fsi-master {
+        compatible = "fsi-master-gpio", "fsi-master";
+        clock-gpios = <&gpio 0>;
+        data-gpios = <&gpio 1>;
+        enable-gpios = <&gpio 2>;
+        trans-gpios = <&gpio 3>;
+        mux-gpios = <&gpio 4>;
+    }
-- 
1.8.2.2

[PATCH v6 13/23] drivers/fsi: Add client driver register utilities

[Christopher Bostic]

From: Chris Bostic <cbostic@linux.vnet.ibm.com>

Add driver_register and driver_unregister wrappers for FSI.

Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 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 75d2a88..4359e26 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -585,6 +585,23 @@ static int fsi_bus_match(struct device *dev, struct device_driver *drv)
 	return 0;
 }
 
+int fsi_driver_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_driver_register);
+
+void fsi_driver_unregister(struct fsi_driver *fsi_drv)
+{
+	driver_unregister(&fsi_drv->drv);
+}
+EXPORT_SYMBOL_GPL(fsi_driver_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 66bce48..34f1e9a 100644
--- a/include/linux/fsi.h
+++ b/include/linux/fsi.h
@@ -54,6 +54,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_driver_register(struct fsi_driver *fsi_drv);
+extern void fsi_driver_unregister(struct fsi_driver *fsi_drv);
+
+/* 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_driver_register, \
+				fsi_driver_unregister)
+
 extern struct bus_type fsi_bus_type;
 
 #endif /* LINUX_FSI_H */
-- 
1.8.2.2

[PATCH v6 14/23] drivers/fsi: Add sysfs files for FSI master & slave accesses

[Christopher Bostic]

From: Jeremy Kerr <jk@ozlabs.org>

This change adds a 'raw' file for reads & writes, and a 'term' file for
the TERM command, and a 'break' file for issuing a BREAK.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Joel Stanley <joel@jms.id.au>
Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
---
 drivers/fsi/fsi-core.c | 116 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 116 insertions(+)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 4359e26..f0f0556 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -294,6 +294,95 @@ static int fsi_slave_scan(struct fsi_slave *slave)
 	return 0;
 }
 
+static ssize_t fsi_slave_sysfs_raw_read(struct file *file,
+		struct kobject *kobj, struct bin_attribute *attr, char *buf,
+		loff_t off, size_t count)
+{
+	struct fsi_slave *slave = to_fsi_slave(kobj_to_dev(kobj));
+	size_t total_len, read_len;
+	int rc;
+
+	if (off < 0)
+		return -EINVAL;
+
+	if (off > 0xffffffff || count > 0xffffffff || off + count > 0xffffffff)
+		return -EINVAL;
+
+	for (total_len = 0; total_len < count; total_len += read_len) {
+		read_len = min_t(size_t, count, 4);
+		read_len -= off & 0x3;
+
+		rc = fsi_slave_read(slave, off, buf + total_len, read_len);
+		if (rc)
+			return rc;
+
+		off += read_len;
+	}
+
+	return count;
+}
+
+static ssize_t fsi_slave_sysfs_raw_write(struct file *file,
+		struct kobject *kobj, struct bin_attribute *attr,
+		char *buf, loff_t off, size_t count)
+{
+	struct fsi_slave *slave = to_fsi_slave(kobj_to_dev(kobj));
+	size_t total_len, write_len;
+	int rc;
+
+	if (off < 0)
+		return -EINVAL;
+
+	if (off > 0xffffffff || count > 0xffffffff || off + count > 0xffffffff)
+		return -EINVAL;
+
+	for (total_len = 0; total_len < count; total_len += write_len) {
+		write_len = min_t(size_t, count, 4);
+		write_len -= off & 0x3;
+
+		rc = fsi_slave_write(slave, off, buf + total_len, write_len);
+		if (rc)
+			return rc;
+
+		off += write_len;
+	}
+
+	return count;
+}
+
+static struct bin_attribute fsi_slave_raw_attr = {
+	.attr = {
+		.name = "raw",
+		.mode = 0600,
+	},
+	.size = 0,
+	.read = fsi_slave_sysfs_raw_read,
+	.write = fsi_slave_sysfs_raw_write,
+};
+
+static ssize_t fsi_slave_sysfs_term_write(struct file *file,
+		struct kobject *kobj, struct bin_attribute *attr,
+		char *buf, loff_t off, size_t count)
+{
+	struct fsi_slave *slave = to_fsi_slave(kobj_to_dev(kobj));
+	struct fsi_master *master = slave->master;
+
+	if (!master->term)
+		return -ENODEV;
+
+	master->term(master, slave->link, slave->id);
+	return count;
+}
+
+static struct bin_attribute fsi_slave_term_attr = {
+	.attr = {
+		.name = "term",
+		.mode = 0200,
+	},
+	.size = 0,
+	.write = fsi_slave_sysfs_term_write,
+};
+
 /* Encode slave local bus echo delay */
 static inline uint32_t fsi_smode_echodly(int x)
 {
@@ -409,6 +498,14 @@ static int fsi_slave_init(struct fsi_master *master, int link, uint8_t id)
 		return rc;
 	}
 
+	rc = device_create_bin_file(&slave->dev, &fsi_slave_raw_attr);
+	if (rc)
+		dev_warn(&slave->dev, "failed to create raw attr: %d\n", rc);
+
+	rc = device_create_bin_file(&slave->dev, &fsi_slave_term_attr);
+	if (rc)
+		dev_warn(&slave->dev, "failed to create term attr: %d\n", rc);
+
 	fsi_slave_scan(slave);
 	return 0;
 }
@@ -523,6 +620,18 @@ static ssize_t master_rescan_store(struct device *dev,
 
 static DEVICE_ATTR(rescan, 0200, NULL, master_rescan_store);
 
+static ssize_t master_break_store(struct device *dev,
+		struct device_attribute *attr, const char *buf, size_t count)
+{
+	struct fsi_master *master = to_fsi_master(dev);
+
+	fsi_master_break(master, 0);
+
+	return count;
+}
+
+static DEVICE_ATTR(break, 0200, NULL, master_break_store);
+
 int fsi_master_register(struct fsi_master *master)
 {
 	int rc;
@@ -546,6 +655,13 @@ int fsi_master_register(struct fsi_master *master)
 		return rc;
 	}
 
+	rc = device_create_file(&master->dev, &dev_attr_break);
+	if (rc) {
+		device_unregister(&master->dev);
+		ida_simple_remove(&master_ida, master->idx);
+		return rc;
+	}
+
 	fsi_master_scan(master);
 
 	return 0;
-- 
1.8.2.2

[PATCH v6 15/23] drivers/fsi: expose direct-access slave API

[Christopher Bostic]

From: Jeremy Kerr <jk@ozlabs.org>

Allow drivers to access the slave address ranges.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Joel Stanley <joel@jms.id.au>
Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
---
 drivers/fsi/fsi-core.c | 26 ++++++++++++++++++++------
 include/linux/fsi.h    | 12 ++++++++++++
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index f0f0556..f7e55e7 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -76,10 +76,6 @@ static int fsi_master_read(struct fsi_master *master, int link,
 		uint8_t slave_id, uint32_t addr, void *val, size_t size);
 static int fsi_master_write(struct fsi_master *master, int link,
 		uint8_t slave_id, uint32_t addr, const void *val, size_t size);
-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 */
 
@@ -181,7 +177,7 @@ static int fsi_slave_calc_addr(struct fsi_slave *slave, uint32_t *addrp,
 	return 0;
 }
 
-static int fsi_slave_read(struct fsi_slave *slave, uint32_t addr,
+int fsi_slave_read(struct fsi_slave *slave, uint32_t addr,
 			void *val, size_t size)
 {
 	uint8_t id = slave->id;
@@ -195,7 +191,7 @@ static int fsi_slave_read(struct fsi_slave *slave, uint32_t addr,
 			addr, val, size);
 }
 
-static int fsi_slave_write(struct fsi_slave *slave, uint32_t addr,
+int fsi_slave_write(struct fsi_slave *slave, uint32_t addr,
 			const void *val, size_t size)
 {
 	uint8_t id = slave->id;
@@ -209,6 +205,24 @@ static int fsi_slave_write(struct fsi_slave *slave, uint32_t addr,
 			addr, val, size);
 }
 
+extern int fsi_slave_claim_range(struct fsi_slave *slave,
+		uint32_t addr, uint32_t size)
+{
+	if (addr + size < addr)
+		return -EINVAL;
+
+	if (addr + size > slave->size)
+		return -EINVAL;
+
+	/* todo: check for overlapping claims */
+	return 0;
+}
+
+extern void fsi_slave_release_range(struct fsi_slave *slave,
+		uint32_t addr, uint32_t 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 34f1e9a..141fd38 100644
--- a/include/linux/fsi.h
+++ b/include/linux/fsi.h
@@ -66,6 +66,18 @@ struct fsi_driver {
 		module_driver(__fsi_driver, fsi_driver_register, \
 				fsi_driver_unregister)
 
+/* direct slave API */
+extern int fsi_slave_claim_range(struct fsi_slave *slave,
+		uint32_t addr, uint32_t size);
+extern void fsi_slave_release_range(struct fsi_slave *slave,
+		uint32_t addr, uint32_t size);
+extern int fsi_slave_read(struct fsi_slave *slave, uint32_t addr,
+		void *val, size_t size);
+extern int fsi_slave_write(struct fsi_slave *slave, uint32_t addr,
+		const void *val, size_t size);
+
+
+
 extern struct bus_type fsi_bus_type;
 
 #endif /* LINUX_FSI_H */
-- 
1.8.2.2

[PATCH v6 16/23] drivers/fsi: Add tracepoints for low-level operations

[Christopher Bostic]

From: Jeremy Kerr <jk@ozlabs.org>

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Joel Stanley <joel@jms.id.au>
Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
---
 drivers/fsi/fsi-core.c     |  27 +++++++---
 include/trace/events/fsi.h | 127 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 148 insertions(+), 6 deletions(-)
 create mode 100644 include/trace/events/fsi.h

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index f7e55e7..09becec 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -21,6 +21,9 @@
 
 #include "fsi-master.h"
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/fsi.h>
+
 #define FSI_SLAVE_CONF_NEXT_MASK	0x80000000
 #define FSI_SLAVE_CONF_SLOTS_MASK	0x00ff0000
 #define FSI_SLAVE_CONF_SLOTS_SHIFT	16
@@ -541,11 +544,16 @@ static int fsi_master_read(struct fsi_master *master, int link,
 {
 	int rc;
 
+	trace_fsi_master_read(master, link, slave_id, addr, size);
+
 	rc = fsi_check_access(addr, size);
-	if (rc)
-		return rc;
+	if (!rc)
+		rc = master->read(master, link, slave_id, addr, val, size);
+
+	trace_fsi_master_rw_result(master, link, slave_id, addr, size,
+			false, val, rc);
 
-	return master->read(master, link, slave_id, addr, val, size);
+	return rc;
 }
 
 static int fsi_master_write(struct fsi_master *master, int link,
@@ -553,11 +561,16 @@ static int fsi_master_write(struct fsi_master *master, int link,
 {
 	int rc;
 
+	trace_fsi_master_write(master, link, slave_id, addr, size, val);
+
 	rc = fsi_check_access(addr, size);
-	if (rc)
-		return rc;
+	if (!rc)
+		rc = master->write(master, link, slave_id, addr, val, size);
 
-	return master->write(master, link, slave_id, addr, val, size);
+	trace_fsi_master_rw_result(master, link, slave_id, addr, size,
+			true, val, rc);
+
+	return rc;
 }
 
 static int fsi_master_link_enable(struct fsi_master *master, int link)
@@ -573,6 +586,8 @@ static int fsi_master_link_enable(struct fsi_master *master, int link)
  */
 static int fsi_master_break(struct fsi_master *master, int link)
 {
+	trace_fsi_master_break(master, link);
+
 	if (master->send_break)
 		return master->send_break(master, link);
 
diff --git a/include/trace/events/fsi.h b/include/trace/events/fsi.h
new file mode 100644
index 0000000..ac74a30
--- /dev/null
+++ b/include/trace/events/fsi.h
@@ -0,0 +1,127 @@
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM fsi
+
+#if !defined(_TRACE_FSI_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_FSI_H
+
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(fsi_master_read,
+	TP_PROTO(const struct fsi_master *master, int link, int id,
+			uint32_t addr, size_t size),
+	TP_ARGS(master, link, id, addr, size),
+	TP_STRUCT__entry(
+		__field(int,	master_idx)
+		__field(int,	link)
+		__field(int,	id)
+		__field(__u32,	addr)
+		__field(size_t,	size)
+	),
+	TP_fast_assign(
+		__entry->master_idx = master->idx;
+		__entry->link = link;
+		__entry->id = id;
+		__entry->addr = addr;
+		__entry->size = size;
+	),
+	TP_printk("fsi%d:%02d:%02d %08x[%zd]",
+		__entry->master_idx,
+		__entry->link,
+		__entry->id,
+		__entry->addr,
+		__entry->size
+	)
+);
+
+TRACE_EVENT(fsi_master_write,
+	TP_PROTO(const struct fsi_master *master, int link, int id,
+			uint32_t addr, size_t size, const void *data),
+	TP_ARGS(master, link, id, addr, size, data),
+	TP_STRUCT__entry(
+		__field(int,	master_idx)
+		__field(int,	link)
+		__field(int,	id)
+		__field(__u32,	addr)
+		__field(size_t,	size)
+		__field(__u32,	data)
+	),
+	TP_fast_assign(
+		__entry->master_idx = master->idx;
+		__entry->link = link;
+		__entry->id = id;
+		__entry->addr = addr;
+		__entry->size = size;
+		__entry->data = 0;
+		memcpy(&__entry->data, data, size);
+	),
+	TP_printk("fsi%d:%02d:%02d %08x[%zd] <= {%*ph}",
+		__entry->master_idx,
+		__entry->link,
+		__entry->id,
+		__entry->addr,
+		__entry->size,
+		__entry->size, &__entry->data
+	)
+);
+
+TRACE_EVENT(fsi_master_rw_result,
+	TP_PROTO(const struct fsi_master *master, int link, int id,
+			uint32_t addr, size_t size,
+			bool write, const void *data, int ret),
+	TP_ARGS(master, link, id, addr, size, write, data, ret),
+	TP_STRUCT__entry(
+		__field(int,	master_idx)
+		__field(int,	link)
+		__field(int,	id)
+		__field(__u32,	addr)
+		__field(size_t,	size)
+		__field(bool,	write)
+		__field(__u32,	data)
+		__field(int,	ret)
+	),
+	TP_fast_assign(
+		__entry->master_idx = master->idx;
+		__entry->link = link;
+		__entry->id = id;
+		__entry->addr = addr;
+		__entry->size = size;
+		__entry->write = write;
+		__entry->data = 0;
+		__entry->ret = ret;
+		if (__entry->write || !__entry->ret)
+			memcpy(&__entry->data, data, size);
+	),
+	TP_printk("fsi%d:%02d:%02d %08x[%zd] %s {%*ph} ret %d",
+		__entry->master_idx,
+		__entry->link,
+		__entry->id,
+		__entry->addr,
+		__entry->size,
+		__entry->write ? "<=" : "=>",
+		__entry->size, &__entry->data,
+		__entry->ret
+	)
+);
+
+TRACE_EVENT(fsi_master_break,
+	TP_PROTO(const struct fsi_master *master, int link),
+	TP_ARGS(master, link),
+	TP_STRUCT__entry(
+		__field(int,	master_idx)
+		__field(int,	link)
+	),
+	TP_fast_assign(
+		__entry->master_idx = master->idx;
+		__entry->link = link;
+	),
+	TP_printk("fsi%d:%d",
+		__entry->master_idx,
+		__entry->link
+	)
+);
+
+
+#endif /* _TRACE_FSI_H */
+
+#include <trace/define_trace.h>
-- 
1.8.2.2

[PATCH v6 17/23] drivers/fsi: Add error handling for slave communication errors

[Christopher Bostic]

From: Jeremy Kerr <jk@ozlabs.org>

This change implements error handling in the FSI core, by cleaning up
and retrying failed operations, using the SISC, TERM and BREAK
facilities.

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

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 09becec..747d0e3 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -44,7 +44,9 @@
 /*
  * FSI slave engine control register offsets
  */
-#define FSI_SMODE			0x0	/* R/W: Mode register */
+#define FSI_SMODE		0x0	/* R/W: Mode register */
+#define FSI_SISC		0x8	/* R/W: Interrupt condition */
+#define FSI_SSTAT		0x14	/* R  : Slave status */
 
 /*
  * SMODE fields
@@ -75,10 +77,14 @@ struct fsi_slave {
 #define to_fsi_master(d) container_of(d, struct fsi_master, dev)
 #define to_fsi_slave(d) container_of(d, struct fsi_slave, dev)
 
+static const int slave_retries = 2;
+static int discard_errors;
+
 static int fsi_master_read(struct fsi_master *master, int link,
 		uint8_t slave_id, uint32_t addr, void *val, size_t size);
 static int fsi_master_write(struct fsi_master *master, int link,
 		uint8_t slave_id, uint32_t addr, const void *val, size_t size);
+static int fsi_master_break(struct fsi_master *master, int link);
 
 /* FSI endpoint-device support */
 
@@ -180,32 +186,131 @@ static int fsi_slave_calc_addr(struct fsi_slave *slave, uint32_t *addrp,
 	return 0;
 }
 
+int fsi_slave_report_and_clear_errors(struct fsi_slave *slave)
+{
+	struct fsi_master *master = slave->master;
+	uint32_t irq, stat;
+	int rc, link;
+	uint8_t id;
+
+	link = slave->link;
+	id = slave->id;
+
+	rc = fsi_master_read(master, link, id, FSI_SLAVE_BASE + FSI_SISC,
+			&irq, sizeof(irq));
+	if (rc)
+		return rc;
+
+	rc =  fsi_master_read(master, link, id, FSI_SLAVE_BASE + FSI_SSTAT,
+			&stat, sizeof(stat));
+	if (rc)
+		return rc;
+
+	dev_info(&slave->dev, "status: 0x%08x, sisc: 0x%08x\n",
+			be32_to_cpu(stat), be32_to_cpu(irq));
+
+	/* clear interrupts */
+	return fsi_master_write(master, link, id, FSI_SLAVE_BASE + FSI_SISC,
+			&irq, sizeof(irq));
+}
+
+static int fsi_slave_set_smode(struct fsi_master *master, int link, int id);
+
+int fsi_slave_handle_error(struct fsi_slave *slave, bool write, uint32_t addr,
+		size_t size)
+{
+	struct fsi_master *master = slave->master;
+	int rc, link;
+	uint32_t reg;
+	uint8_t id;
+
+	if (discard_errors)
+		return -1;
+
+	link = slave->link;
+	id = slave->id;
+
+	dev_dbg(&slave->dev, "handling error on %s to 0x%08x[%zd]",
+			write ? "write" : "read", addr, size);
+
+	/* try a simple clear of error conditions, which may fail if we've lost
+	 * communication with the slave
+	 */
+	rc = fsi_slave_report_and_clear_errors(slave);
+	if (!rc)
+		return 0;
+
+	/* send a TERM and retry */
+	if (master->term) {
+		rc = master->term(master, link, id);
+		if (!rc) {
+			rc = fsi_master_read(master, link, id, 0,
+					&reg, sizeof(reg));
+			if (!rc)
+				rc = fsi_slave_report_and_clear_errors(slave);
+			if (!rc)
+				return 0;
+		}
+	}
+
+	/* getting serious, reset the slave via BREAK */
+	rc = fsi_master_break(master, link);
+	if (rc)
+		return rc;
+
+	rc = fsi_slave_set_smode(master, link, id);
+	if (rc)
+		return rc;
+
+	return fsi_slave_report_and_clear_errors(slave);
+}
+
 int fsi_slave_read(struct fsi_slave *slave, uint32_t addr,
 			void *val, size_t size)
 {
 	uint8_t id = slave->id;
-	int rc;
+	int rc, err_rc, i;
 
 	rc = fsi_slave_calc_addr(slave, &addr, &id);
 	if (rc)
 		return rc;
 
-	return fsi_master_read(slave->master, slave->link, id,
-			addr, val, size);
+	for (i = 0; i < slave_retries; i++) {
+		rc = fsi_master_read(slave->master, slave->link,
+				id, addr, val, size);
+		if (!rc)
+			break;
+
+		err_rc = fsi_slave_handle_error(slave, false, addr, size);
+		if (err_rc)
+			break;
+	}
+
+	return rc;
 }
 
 int fsi_slave_write(struct fsi_slave *slave, uint32_t addr,
 			const void *val, size_t size)
 {
 	uint8_t id = slave->id;
-	int rc;
+	int rc, err_rc, i;
 
 	rc = fsi_slave_calc_addr(slave, &addr, &id);
 	if (rc)
 		return rc;
 
-	return fsi_master_write(slave->master, slave->link, id,
-			addr, val, size);
+	for (i = 0; i < slave_retries; i++) {
+		rc = fsi_master_write(slave->master, slave->link,
+				id, addr, val, size);
+		if (!rc)
+			break;
+
+		err_rc = fsi_slave_handle_error(slave, true, addr, size);
+		if (err_rc)
+			break;
+	}
+
+	return rc;
 }
 
 extern int fsi_slave_claim_range(struct fsi_slave *slave,
@@ -765,3 +870,5 @@ static void fsi_exit(void)
 
 module_init(fsi_init);
 module_exit(fsi_exit);
+module_param(discard_errors, int, 0664);
+MODULE_PARM_DESC(discard_errors, "Don't invoke error handling on bus accesses");
-- 
1.8.2.2

[PATCH v6 18/23] drivers/fsi: Document FSI master sysfs files in ABI

[Christopher Bostic]

From: Chris Bostic <cbostic@linux.vnet.ibm.com>

Add info for sysfs scan file in Documentaiton ABI/testing

Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
---
 Documentation/ABI/testing/sysfs-bus-fsi | 6 ++++++
 1 file changed, 6 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-fsi

diff --git a/Documentation/ABI/testing/sysfs-bus-fsi b/Documentation/ABI/testing/sysfs-bus-fsi
new file mode 100644
index 0000000..dfcbc1b
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-fsi
@@ -0,0 +1,6 @@
+What:           /sys/bus/platform/devices/fsi-master/scan
+KernelVersion:  4.9
+Contact:        cbostic at us.ibm.com
+Description:
+                Initiates a FSI master scan for all connected
+                slave devices on its links.
-- 
1.8.2.2

[PATCH v6 18/23] drivers/fsi: Document FSI master sysfs files in ABI

[Joel Stanley]

On Tue, Apr 11, 2017 at 5:17 AM, Christopher Bostic
<cbostic@linux.vnet.ibm.com> wrote:
> From: Chris Bostic <cbostic@linux.vnet.ibm.com>
>
> Add info for sysfs scan file in Documentaiton ABI/testing

You are missing documentation for the 'raw', 'term' and 'break' files.

> Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-fsi | 6 ++++++
>  1 file changed, 6 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-fsi
>
> diff --git a/Documentation/ABI/testing/sysfs-bus-fsi b/Documentation/ABI/testing/sysfs-bus-fsi
> new file mode 100644
> index 0000000..dfcbc1b
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-fsi
> @@ -0,0 +1,6 @@
> +What:           /sys/bus/platform/devices/fsi-master/scan

This has been renamed 'rescan' in this series. The path has changed too.

> +KernelVersion:  4.9

This should be 4.12 I guess, assuming we get it merged this cycle.

> +Contact:        cbostic at us.ibm.com
> +Description:
> +                Initiates a FSI master scan for all connected
> +                slave devices on its links.
> --
> 1.8.2.2
>

[PATCH v6 18/23] drivers/fsi: Document FSI master sysfs files in ABI

[Christopher Bostic]

On 5/10/17 2:30 AM, Joel Stanley wrote:
> On Tue, Apr 11, 2017 at 5:17 AM, Christopher Bostic
> <cbostic@linux.vnet.ibm.com> wrote:
>> From: Chris Bostic <cbostic@linux.vnet.ibm.com>
>>
>> Add info for sysfs scan file in Documentaiton ABI/testing
> You are missing documentation for the 'raw', 'term' and 'break' files.

Hi Joel,

Yes, I've made your suggested changes in my pending release version 7 
patch set.

>> Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
>> ---
>>   Documentation/ABI/testing/sysfs-bus-fsi | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>   create mode 100644 Documentation/ABI/testing/sysfs-bus-fsi
>>
>> diff --git a/Documentation/ABI/testing/sysfs-bus-fsi b/Documentation/ABI/testing/sysfs-bus-fsi
>> new file mode 100644
>> index 0000000..dfcbc1b
>> --- /dev/null
>> +++ b/Documentation/ABI/testing/sysfs-bus-fsi
>> @@ -0,0 +1,6 @@
>> +What:           /sys/bus/platform/devices/fsi-master/scan
> This has been renamed 'rescan' in this series. The path has changed too.
>
>> +KernelVersion:  4.9
> This should be 4.12 I guess, assuming we get it merged this cycle.

Will update.

Thanks
-Chris
>
>> +Contact:        cbostic at us.ibm.com
>> +Description:
>> +                Initiates a FSI master scan for all connected
>> +                slave devices on its links.
>> --
>> 1.8.2.2
>>

[PATCH v6 19/23] drivers/fsi: Add GPIO based FSI master

[Christopher Bostic]

From: Chris Bostic <cbostic@linux.vnet.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 changes from Edward A. James <eajames@us.ibm.com> and Jeremy
Kerr <jk@ozlabs.org>.

Signed-off-by: Edward A. James <eajames@us.ibm.com>
Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/fsi/Kconfig           |  11 +
 drivers/fsi/Makefile          |   1 +
 drivers/fsi/fsi-master-gpio.c | 610 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 622 insertions(+)
 create mode 100644 drivers/fsi/fsi-master-gpio.c

diff --git a/drivers/fsi/Kconfig b/drivers/fsi/Kconfig
index 04c1a0e..448bc3b 100644
--- a/drivers/fsi/Kconfig
+++ b/drivers/fsi/Kconfig
@@ -9,4 +9,15 @@ 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_GPIO
+	tristate "GPIO-based FSI master"
+	depends on GPIOLIB
+	---help---
+	This option enables a FSI master driver using GPIO lines.
+
+endif
+
 endmenu
diff --git a/drivers/fsi/Makefile b/drivers/fsi/Makefile
index db0e5e7..ed28ac0 100644
--- a/drivers/fsi/Makefile
+++ b/drivers/fsi/Makefile
@@ -1,2 +1,3 @@
 
 obj-$(CONFIG_FSI) += fsi-core.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..9fedfaf
--- /dev/null
+++ b/drivers/fsi/fsi-master-gpio.c
@@ -0,0 +1,610 @@
+/*
+ * A FSI master controller, using a simple GPIO bit-banging interface
+ */
+
+#include <linux/platform_device.h>
+#include <linux/gpio/consumer.h>
+#include <linux/module.h>
+#include <linux/delay.h>
+#include <linux/fsi.h>
+#include <linux/device.h>
+#include <linux/io.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+
+#include "fsi-master.h"
+
+#define	FSI_GPIO_STD_DLY	1	/* Standard pin delay in nS */
+#define	FSI_ECHO_DELAY_CLOCKS	16	/* Number clocks for echo delay */
+#define	FSI_PRE_BREAK_CLOCKS	50	/* Number clocks to prep for break */
+#define	FSI_BREAK_CLOCKS	256	/* Number of clocks to issue break */
+#define	FSI_POST_BREAK_CLOCKS	16000	/* Number clocks to set up cfam */
+#define	FSI_INIT_CLOCKS		5000	/* Clock out any old data */
+#define	FSI_GPIO_STD_DELAY	10	/* Standard GPIO delay in nS */
+					/* todo: adjust down as low as */
+					/* possible or eliminate */
+#define	FSI_GPIO_CMD_DPOLL      0x2
+#define	FSI_GPIO_CMD_TERM	0x3f
+#define FSI_GPIO_CMD_ABS_AR	0x4
+
+#define	FSI_GPIO_DPOLL_CLOCKS	100      /* < 21 will cause slave to hang */
+
+/* Bus errors */
+#define	FSI_GPIO_ERR_BUSY	1	/* Slave stuck in busy state */
+#define	FSI_GPIO_RESP_ERRA	2	/* Any (misc) Error */
+#define	FSI_GPIO_RESP_ERRC	3	/* Slave reports master CRC error */
+#define	FSI_GPIO_MTOE		4	/* Master time out error */
+#define	FSI_GPIO_CRC_INVAL	5	/* Master reports slave CRC error */
+
+/* Normal slave responses */
+#define	FSI_GPIO_RESP_BUSY	1
+#define	FSI_GPIO_RESP_ACK	0
+#define	FSI_GPIO_RESP_ACKD	4
+
+#define	FSI_GPIO_MAX_BUSY	100
+#define	FSI_GPIO_MTOE_COUNT	1000
+#define	FSI_GPIO_DRAIN_BITS	20
+#define	FSI_GPIO_CRC_SIZE	4
+#define	FSI_GPIO_MSG_ID_SIZE		2
+#define	FSI_GPIO_MSG_RESPID_SIZE	2
+#define	FSI_GPIO_PRIME_SLAVE_CLOCKS	100
+
+static DEFINE_SPINLOCK(fsi_gpio_cmd_lock);	/* lock around fsi commands */
+
+struct fsi_master_gpio {
+	struct fsi_master	master;
+	struct device		*dev;
+	struct gpio_desc	*gpio_clk;
+	struct gpio_desc	*gpio_data;
+	struct gpio_desc	*gpio_trans;	/* Voltage translator */
+	struct gpio_desc	*gpio_enable;	/* FSI enable */
+	struct gpio_desc	*gpio_mux;	/* Mux control */
+};
+
+#define to_fsi_master_gpio(m) container_of(m, struct fsi_master_gpio, master)
+
+struct fsi_gpio_msg {
+	uint64_t	msg;
+	uint8_t		bits;
+};
+
+static void clock_toggle(struct fsi_master_gpio *master, int count)
+{
+	int i;
+
+	for (i = 0; i < count; i++) {
+		ndelay(FSI_GPIO_STD_DLY);
+		gpiod_set_value(master->gpio_clk, 0);
+		ndelay(FSI_GPIO_STD_DLY);
+		gpiod_set_value(master->gpio_clk, 1);
+	}
+}
+
+static int sda_in(struct fsi_master_gpio *master)
+{
+	int in;
+
+	ndelay(FSI_GPIO_STD_DLY);
+	in = gpiod_get_value(master->gpio_data);
+	return in ? 1 : 0;
+}
+
+static void sda_out(struct fsi_master_gpio *master, int value)
+{
+	gpiod_set_value(master->gpio_data, value);
+}
+
+static void set_sda_input(struct fsi_master_gpio *master)
+{
+	gpiod_direction_input(master->gpio_data);
+	if (master->gpio_trans)
+		gpiod_set_value(master->gpio_trans, 0);
+}
+
+static void set_sda_output(struct fsi_master_gpio *master, int value)
+{
+	if (master->gpio_trans)
+		gpiod_set_value(master->gpio_trans, 1);
+	gpiod_direction_output(master->gpio_data, value);
+}
+
+static void clock_zeros(struct fsi_master_gpio *master, int count)
+{
+	set_sda_output(master, 1);
+	clock_toggle(master, count);
+}
+
+static void serial_in(struct fsi_master_gpio *master, struct fsi_gpio_msg *msg,
+			uint8_t num_bits)
+{
+	uint8_t bit, in_bit;
+
+	set_sda_input(master);
+
+	for (bit = 0; bit < num_bits; bit++) {
+		clock_toggle(master, 1);
+		in_bit = sda_in(master);
+		msg->msg <<= 1;
+		msg->msg |= ~in_bit & 0x1;	/* Data is negative active */
+	}
+	msg->bits += num_bits;
+}
+
+static void serial_out(struct fsi_master_gpio *master,
+			const struct fsi_gpio_msg *cmd)
+{
+	uint8_t bit;
+	uint64_t msg = ~cmd->msg;	/* Data is negative active */
+	uint64_t sda_mask = 0x1ULL << (cmd->bits - 1);
+	uint64_t last_bit = ~0;
+	int next_bit;
+
+	if (!cmd->bits) {
+		dev_warn(master->dev, "trying to output 0 bits\n");
+		return;
+	}
+	set_sda_output(master, 0);
+
+	/* Send the start bit */
+	sda_out(master, 0);
+	clock_toggle(master, 1);
+
+	/* Send the message */
+	for (bit = 0; bit < cmd->bits; bit++) {
+		next_bit = (msg & sda_mask) >> (cmd->bits - 1);
+		if (last_bit ^ next_bit) {
+			sda_out(master, next_bit);
+			last_bit = next_bit;
+		}
+		clock_toggle(master, 1);
+		msg <<= 1;
+	}
+}
+
+static void msg_push_bits(struct fsi_gpio_msg *msg, uint64_t data, int bits)
+{
+	msg->msg <<= bits;
+	msg->msg |= data & ((1ull << bits) - 1);
+	msg->bits += bits;
+}
+
+static void msg_push_crc(struct fsi_gpio_msg *msg)
+{
+	uint8_t crc;
+	int top;
+
+	top = msg->bits & 0x3;
+
+	/* start bit, and any non-aligned top bits */
+	crc = fsi_crc4(0,
+			1 << top | msg->msg >> (msg->bits - top),
+			top + 1);
+
+	/* aligned bits */
+	crc = fsi_crc4(crc, msg->msg, msg->bits - top);
+
+	msg_push_bits(msg, crc, 4);
+}
+
+static void build_abs_ar_command(struct fsi_gpio_msg *cmd,
+		uint8_t id, uint32_t addr, size_t size, const void *data)
+{
+	bool write = !!data;
+	uint8_t ds;
+	int i;
+
+	cmd->bits = 0;
+	cmd->msg = 0;
+
+	msg_push_bits(cmd, id, 2);
+	msg_push_bits(cmd, FSI_GPIO_CMD_ABS_AR, 3);
+	msg_push_bits(cmd, write ? 0 : 1, 1);
+
+	/*
+	 * The read/write size is encoded in the lower bits of the address
+	 * (as it must be naturally-aligned), and the following ds bit.
+	 *
+	 *	size	addr:1	addr:0	ds
+	 *	1	x	x	0
+	 *	2	x	0	1
+	 *	4	0	1	1
+	 *
+	 */
+	ds = size > 1 ? 1 : 0;
+	addr &= ~(size - 1);
+	if (size == 4)
+		addr |= 1;
+
+	msg_push_bits(cmd, addr & ((1 << 21) - 1), 21);
+	msg_push_bits(cmd, ds, 1);
+	for (i = 0; write && i < size; i++)
+		msg_push_bits(cmd, ((uint8_t *)data)[i], 8);
+
+	msg_push_crc(cmd);
+}
+
+static void build_dpoll_command(struct fsi_gpio_msg *cmd, uint8_t slave_id)
+{
+	cmd->bits = 0;
+	cmd->msg = 0;
+
+	msg_push_bits(cmd, slave_id, 2);
+	msg_push_bits(cmd, FSI_GPIO_CMD_DPOLL, 3);
+	msg_push_crc(cmd);
+}
+
+static void echo_delay(struct fsi_master_gpio *master)
+{
+	set_sda_output(master, 1);
+	clock_toggle(master, FSI_ECHO_DELAY_CLOCKS);
+}
+
+static void build_term_command(struct fsi_gpio_msg *cmd, uint8_t slave_id)
+{
+	cmd->bits = 0;
+	cmd->msg = 0;
+
+	msg_push_bits(cmd, slave_id, 2);
+	msg_push_bits(cmd, FSI_GPIO_CMD_TERM, 6);
+	msg_push_crc(cmd);
+}
+
+/*
+ * 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 read_one_response(struct fsi_master_gpio *master,
+		uint8_t data_size, struct fsi_gpio_msg *msgp, uint8_t *tagp)
+{
+	struct fsi_gpio_msg msg;
+	uint8_t id, tag;
+	uint32_t crc;
+	int i;
+
+	/* wait for the start bit */
+	for (i = 0; i < FSI_GPIO_MTOE_COUNT; i++) {
+		msg.bits = 0;
+		msg.msg = 0;
+		serial_in(master, &msg, 1);
+		if (msg.msg)
+			break;
+	}
+	if (i >= FSI_GPIO_MTOE_COUNT) {
+		dev_dbg(master->dev,
+			"Master time out waiting for response\n");
+		fsi_master_gpio_error(master, FSI_GPIO_MTOE);
+		return -EIO;
+	}
+
+	msg.bits = 0;
+	msg.msg = 0;
+
+	/* Read slave ID & response tag */
+	serial_in(master, &msg, 4);
+
+	id = (msg.msg >> FSI_GPIO_MSG_RESPID_SIZE) & 0x3;
+	tag = msg.msg & 0x3;
+
+	/* if we have an ACK, and we're expecting data, clock the
+	 * data in too
+	 */
+	if (tag == FSI_GPIO_RESP_ACK && data_size)
+		serial_in(master, &msg, data_size * 8);
+
+	/* read CRC */
+	serial_in(master, &msg, FSI_GPIO_CRC_SIZE);
+
+	/* we have a whole message now; check CRC */
+	crc = fsi_crc4(0, 1, 1);
+	crc = fsi_crc4(crc, msg.msg, msg.bits);
+	if (crc) {
+		dev_dbg(master->dev, "ERR response CRC\n");
+		fsi_master_gpio_error(master, FSI_GPIO_CRC_INVAL);
+		return -EIO;
+	}
+
+	if (msgp)
+		*msgp = msg;
+	if (tagp)
+		*tagp = tag;
+
+	return 0;
+}
+
+static int issue_term(struct fsi_master_gpio *master, uint8_t slave)
+{
+	struct fsi_gpio_msg cmd;
+	uint8_t tag;
+	int rc;
+
+	build_term_command(&cmd, slave);
+	serial_out(master, &cmd);
+	echo_delay(master);
+
+	rc = read_one_response(master, 0, NULL, &tag);
+	if (rc) {
+		dev_err(master->dev,
+				"TERM failed; lost communication with slave\n");
+		return -EIO;
+	} else if (tag != FSI_GPIO_RESP_ACK) {
+		dev_err(master->dev, "TERM failed; response %d\n", tag);
+		return -EIO;
+	}
+
+	return 0;
+}
+
+static int poll_for_response(struct fsi_master_gpio *master,
+		uint8_t slave, uint8_t size, void *data)
+{
+	struct fsi_gpio_msg response, cmd;
+	int busy_count = 0, rc, i;
+	uint8_t tag;
+
+retry:
+	rc = read_one_response(master, size, &response, &tag);
+	if (rc)
+		return rc;
+
+	switch (tag) {
+	case FSI_GPIO_RESP_ACK:
+		if (size && data) {
+			uint64_t val = response.msg;
+			/* clear crc & mask */
+			val >>= 4;
+			val &= (1ull << (size * 8)) - 1;
+
+			for (i = 0; i < size; i++) {
+				((uint8_t *)data)[size-i-1] =
+					val & 0xff;
+				val >>= 8;
+			}
+		}
+		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.
+		 */
+		clock_zeros(master, FSI_GPIO_DPOLL_CLOCKS);
+		if (busy_count++ < FSI_GPIO_MAX_BUSY) {
+			build_dpoll_command(&cmd, slave);
+			serial_out(master, &cmd);
+			echo_delay(master);
+			goto retry;
+		}
+		dev_warn(master->dev,
+			"ERR slave is stuck in busy state, issuing TERM\n");
+		issue_term(master, slave);
+		rc = -EIO;
+		break;
+
+	case FSI_GPIO_RESP_ERRA:
+	case FSI_GPIO_RESP_ERRC:
+		dev_dbg(master->dev, "ERR%c received: 0x%x\n",
+			tag == FSI_GPIO_RESP_ERRA ? 'A' : 'C',
+			(int)response.msg);
+		fsi_master_gpio_error(master, response.msg);
+		rc = -EIO;
+		break;
+	}
+
+	/* Clock the slave enough to be ready for next operation */
+	clock_zeros(master, FSI_GPIO_PRIME_SLAVE_CLOCKS);
+	return rc;
+}
+
+static int fsi_master_gpio_xfer(struct fsi_master_gpio *master, uint8_t slave,
+		struct fsi_gpio_msg *cmd, size_t resp_len, void *resp)
+{
+	unsigned long flags;
+	int rc;
+
+	spin_lock_irqsave(&fsi_gpio_cmd_lock, flags);
+	serial_out(master, cmd);
+	echo_delay(master);
+	rc = poll_for_response(master, slave, resp_len, resp);
+	spin_unlock_irqrestore(&fsi_gpio_cmd_lock, flags);
+
+	return rc;
+}
+
+static int fsi_master_gpio_read(struct fsi_master *_master, int link,
+		uint8_t id, 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, id, addr, size, NULL);
+	return fsi_master_gpio_xfer(master, id, &cmd, size, val);
+}
+
+static int fsi_master_gpio_write(struct fsi_master *_master, int link,
+		uint8_t id, 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, id, addr, size, val);
+	return fsi_master_gpio_xfer(master, id, &cmd, 0, NULL);
+}
+
+static int fsi_master_gpio_term(struct fsi_master *_master,
+		int link, uint8_t id)
+{
+	struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
+	struct fsi_gpio_msg cmd;
+
+	if (link != 0)
+		return -ENODEV;
+
+	build_term_command(&cmd, id);
+	return fsi_master_gpio_xfer(master, id, &cmd, 0, 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, 1);
+	sda_out(master, 1);
+	clock_toggle(master, FSI_PRE_BREAK_CLOCKS);
+	sda_out(master, 0);
+	clock_toggle(master, FSI_BREAK_CLOCKS);
+	echo_delay(master);
+	sda_out(master, 1);
+	clock_toggle(master, FSI_POST_BREAK_CLOCKS);
+
+	/* Wait for logic reset to take effect */
+	udelay(200);
+
+	return 0;
+}
+
+static void fsi_master_gpio_init(struct fsi_master_gpio *master)
+{
+	if (master->gpio_mux)
+		gpiod_direction_output(master->gpio_mux, 1);
+	if (master->gpio_trans)
+		gpiod_direction_output(master->gpio_trans, 1);
+	if (master->gpio_enable)
+		gpiod_direction_output(master->gpio_enable, 1);
+	gpiod_direction_output(master->gpio_clk, 1);
+	gpiod_direction_output(master->gpio_data, 1);
+
+	/* todo: evaluate if clocks can be reduced */
+	clock_zeros(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 void fsi_master_gpio_release(struct device *dev)
+{
+}
+
+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;
+
+	master->dev = &pdev->dev;
+	master->master.dev.parent = master->dev;
+	master->master.dev.release = fsi_master_gpio_release;
+
+	gpio = devm_gpiod_get(&pdev->dev, "clock", 0);
+	if (IS_ERR(gpio)) {
+		dev_err(&pdev->dev, "failed to get clock gpio\n");
+		return PTR_ERR(gpio);
+	}
+	master->gpio_clk = gpio;
+
+	gpio = devm_gpiod_get(&pdev->dev, "data", 0);
+	if (IS_ERR(gpio)) {
+		dev_err(&pdev->dev, "failed to get data gpio\n");
+		return PTR_ERR(gpio);
+	}
+	master->gpio_data = gpio;
+
+	/* Optional GPIOs */
+	gpio = devm_gpiod_get_optional(&pdev->dev, "trans", 0);
+	if (IS_ERR(gpio)) {
+		dev_err(&pdev->dev, "failed to get trans gpio\n");
+		return PTR_ERR(gpio);
+	}
+	master->gpio_trans = gpio;
+
+	gpio = devm_gpiod_get_optional(&pdev->dev, "enable", 0);
+	if (IS_ERR(gpio)) {
+		dev_err(&pdev->dev, "failed to get enable gpio\n");
+		return PTR_ERR(gpio);
+	}
+	master->gpio_enable = gpio;
+
+	gpio = devm_gpiod_get_optional(&pdev->dev, "mux", 0);
+	if (IS_ERR(gpio)) {
+		dev_err(&pdev->dev, "failed to get mux gpio\n");
+		return PTR_ERR(gpio);
+	}
+	master->gpio_mux = gpio;
+
+	master->master.n_links = 1;
+	master->master.read = fsi_master_gpio_read;
+	master->master.write = fsi_master_gpio_write;
+	master->master.term = fsi_master_gpio_term;
+	master->master.send_break = fsi_master_gpio_break;
+	master->master.link_enable = fsi_master_gpio_link_enable;
+	platform_set_drvdata(pdev, master);
+
+	fsi_master_gpio_init(master);
+
+	fsi_master_register(&master->master);
+
+	return 0;
+}
+
+
+static int fsi_master_gpio_remove(struct platform_device *pdev)
+{
+	struct fsi_master_gpio *master = platform_get_drvdata(pdev);
+
+	devm_gpiod_put(&pdev->dev, master->gpio_clk);
+	devm_gpiod_put(&pdev->dev, master->gpio_data);
+	if (master->gpio_trans)
+		devm_gpiod_put(&pdev->dev, master->gpio_trans);
+	if (master->gpio_enable)
+		devm_gpiod_put(&pdev->dev, master->gpio_enable);
+	if (master->gpio_mux)
+		devm_gpiod_put(&pdev->dev, master->gpio_mux);
+	fsi_master_unregister(&master->master);
+
+	return 0;
+}
+
+static const struct of_device_id fsi_master_gpio_match[] = {
+	{ .compatible = "fsi-master-gpio" },
+	{ },
+};
+
+static struct platform_driver fsi_master_gpio_driver = {
+	.driver = {
+		.name		= "fsi-master-gpio",
+		.of_match_table	= fsi_master_gpio_match,
+	},
+	.probe	= fsi_master_gpio_probe,
+	.remove = fsi_master_gpio_remove,
+};
+
+module_platform_driver(fsi_master_gpio_driver);
+MODULE_LICENSE("GPL");
-- 
1.8.2.2

[PATCH v6 19/23] drivers/fsi: Add GPIO based FSI master

[Joel Stanley]

Hi Chris,

On Tue, Apr 11, 2017 at 5:17 AM, Christopher Bostic
<cbostic@linux.vnet.ibm.com> wrote:

> From: Chris Bostic <cbostic@linux.vnet.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 changes from Edward A. James <eajames@us.ibm.com> and Jeremy
> Kerr <jk@ozlabs.org>.

I think the series is looking good. I've done a bunch of testing on
some machines, and it worked for me.

I've got a few comments and things to be clarified below.

>
> Signed-off-by: Edward A. James <eajames@us.ibm.com>
> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
> Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
>  drivers/fsi/Kconfig           |  11 +
>  drivers/fsi/Makefile          |   1 +
>  drivers/fsi/fsi-master-gpio.c | 610 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 622 insertions(+)
>  create mode 100644 drivers/fsi/fsi-master-gpio.c
>
> diff --git a/drivers/fsi/Kconfig b/drivers/fsi/Kconfig
> index 04c1a0e..448bc3b 100644
> --- a/drivers/fsi/Kconfig
> +++ b/drivers/fsi/Kconfig
> @@ -9,4 +9,15 @@ 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_GPIO
> +       tristate "GPIO-based FSI master"
> +       depends on GPIOLIB
> +       ---help---
> +       This option enables a FSI master driver using GPIO lines.
> +
> +endif
> +
>  endmenu
> diff --git a/drivers/fsi/Makefile b/drivers/fsi/Makefile
> index db0e5e7..ed28ac0 100644
> --- a/drivers/fsi/Makefile
> +++ b/drivers/fsi/Makefile
> @@ -1,2 +1,3 @@
>
>  obj-$(CONFIG_FSI) += fsi-core.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..9fedfaf
> --- /dev/null
> +++ b/drivers/fsi/fsi-master-gpio.c
> @@ -0,0 +1,610 @@
> +/*
> + * A FSI master controller, using a simple GPIO bit-banging interface
> + */
> +
> +#include <linux/platform_device.h>
> +#include <linux/gpio/consumer.h>
> +#include <linux/module.h>
> +#include <linux/delay.h>
> +#include <linux/fsi.h>
> +#include <linux/device.h>
> +#include <linux/io.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +
> +#include "fsi-master.h"
> +
> +#define        FSI_GPIO_STD_DLY        1       /* Standard pin delay in nS */
> +#define        FSI_ECHO_DELAY_CLOCKS   16      /* Number clocks for echo delay */
> +#define        FSI_PRE_BREAK_CLOCKS    50      /* Number clocks to prep for break */
> +#define        FSI_BREAK_CLOCKS        256     /* Number of clocks to issue break */
> +#define        FSI_POST_BREAK_CLOCKS   16000   /* Number clocks to set up cfam */
> +#define        FSI_INIT_CLOCKS         5000    /* Clock out any old data */
> +#define        FSI_GPIO_STD_DELAY      10      /* Standard GPIO delay in nS */
> +                                       /* todo: adjust down as low as */
> +                                       /* possible or eliminate */
> +#define        FSI_GPIO_CMD_DPOLL      0x2
> +#define        FSI_GPIO_CMD_TERM       0x3f
> +#define FSI_GPIO_CMD_ABS_AR    0x4
> +
> +#define        FSI_GPIO_DPOLL_CLOCKS   100      /* < 21 will cause slave to hang */
> +
> +/* Bus errors */
> +#define        FSI_GPIO_ERR_BUSY       1       /* Slave stuck in busy state */
> +#define        FSI_GPIO_RESP_ERRA      2       /* Any (misc) Error */
> +#define        FSI_GPIO_RESP_ERRC      3       /* Slave reports master CRC error */
> +#define        FSI_GPIO_MTOE           4       /* Master time out error */
> +#define        FSI_GPIO_CRC_INVAL      5       /* Master reports slave CRC error */
> +
> +/* Normal slave responses */
> +#define        FSI_GPIO_RESP_BUSY      1
> +#define        FSI_GPIO_RESP_ACK       0
> +#define        FSI_GPIO_RESP_ACKD      4
> +
> +#define        FSI_GPIO_MAX_BUSY       100
> +#define        FSI_GPIO_MTOE_COUNT     1000
> +#define        FSI_GPIO_DRAIN_BITS     20
> +#define        FSI_GPIO_CRC_SIZE       4
> +#define        FSI_GPIO_MSG_ID_SIZE            2
> +#define        FSI_GPIO_MSG_RESPID_SIZE        2
> +#define        FSI_GPIO_PRIME_SLAVE_CLOCKS     100
> +
> +static DEFINE_SPINLOCK(fsi_gpio_cmd_lock);     /* lock around fsi commands */

Should this be per-master?

> +
> +struct fsi_master_gpio {
> +       struct fsi_master       master;
> +       struct device           *dev;
> +       struct gpio_desc        *gpio_clk;
> +       struct gpio_desc        *gpio_data;
> +       struct gpio_desc        *gpio_trans;    /* Voltage translator */
> +       struct gpio_desc        *gpio_enable;   /* FSI enable */
> +       struct gpio_desc        *gpio_mux;      /* Mux control */
> +};
> +
> +#define to_fsi_master_gpio(m) container_of(m, struct fsi_master_gpio, master)
> +
> +struct fsi_gpio_msg {
> +       uint64_t        msg;
> +       uint8_t         bits;
> +};
> +
> +static void clock_toggle(struct fsi_master_gpio *master, int count)
> +{
> +       int i;
> +
> +       for (i = 0; i < count; i++) {
> +               ndelay(FSI_GPIO_STD_DLY);
> +               gpiod_set_value(master->gpio_clk, 0);
> +               ndelay(FSI_GPIO_STD_DLY);
> +               gpiod_set_value(master->gpio_clk, 1);
> +       }
> +}
> +
> +static int sda_in(struct fsi_master_gpio *master)
> +{
> +       int in;
> +
> +       ndelay(FSI_GPIO_STD_DLY);
> +       in = gpiod_get_value(master->gpio_data);
> +       return in ? 1 : 0;
> +}
> +
> +static void sda_out(struct fsi_master_gpio *master, int value)
> +{
> +       gpiod_set_value(master->gpio_data, value);
> +}
> +
> +static void set_sda_input(struct fsi_master_gpio *master)
> +{
> +       gpiod_direction_input(master->gpio_data);
> +       if (master->gpio_trans)
> +               gpiod_set_value(master->gpio_trans, 0);

gpiod_set_value checks for the first argument being NULL. You could
drop your check here.

> +}
> +
> +static void set_sda_output(struct fsi_master_gpio *master, int value)
> +{
> +       if (master->gpio_trans)
> +               gpiod_set_value(master->gpio_trans, 1);
> +       gpiod_direction_output(master->gpio_data, value);
> +}
> +
> +static void clock_zeros(struct fsi_master_gpio *master, int count)
> +{
> +       set_sda_output(master, 1);
> +       clock_toggle(master, count);
> +}
> +
> +static void serial_in(struct fsi_master_gpio *master, struct fsi_gpio_msg *msg,
> +                       uint8_t num_bits)
> +{
> +       uint8_t bit, in_bit;
> +
> +       set_sda_input(master);
> +
> +       for (bit = 0; bit < num_bits; bit++) {
> +               clock_toggle(master, 1);
> +               in_bit = sda_in(master);
> +               msg->msg <<= 1;
> +               msg->msg |= ~in_bit & 0x1;      /* Data is negative active */

"Active low" makes more sense than negative active.

> +       }
> +       msg->bits += num_bits;
> +}
> +
> +static void serial_out(struct fsi_master_gpio *master,
> +                       const struct fsi_gpio_msg *cmd)
> +{
> +       uint8_t bit;
> +       uint64_t msg = ~cmd->msg;       /* Data is negative active */
> +       uint64_t sda_mask = 0x1ULL << (cmd->bits - 1);
> +       uint64_t last_bit = ~0;
> +       int next_bit;
> +
> +       if (!cmd->bits) {
> +               dev_warn(master->dev, "trying to output 0 bits\n");
> +               return;
> +       }
> +       set_sda_output(master, 0);
> +
> +       /* Send the start bit */
> +       sda_out(master, 0);
> +       clock_toggle(master, 1);
> +
> +       /* Send the message */
> +       for (bit = 0; bit < cmd->bits; bit++) {
> +               next_bit = (msg & sda_mask) >> (cmd->bits - 1);
> +               if (last_bit ^ next_bit) {
> +                       sda_out(master, next_bit);
> +                       last_bit = next_bit;
> +               }
> +               clock_toggle(master, 1);
> +               msg <<= 1;
> +       }
> +}
> +
> +static void msg_push_bits(struct fsi_gpio_msg *msg, uint64_t data, int bits)
> +{
> +       msg->msg <<= bits;
> +       msg->msg |= data & ((1ull << bits) - 1);
> +       msg->bits += bits;
> +}
> +
> +static void msg_push_crc(struct fsi_gpio_msg *msg)
> +{
> +       uint8_t crc;
> +       int top;
> +
> +       top = msg->bits & 0x3;
> +
> +       /* start bit, and any non-aligned top bits */
> +       crc = fsi_crc4(0,
> +                       1 << top | msg->msg >> (msg->bits - top),
> +                       top + 1);
> +
> +       /* aligned bits */
> +       crc = fsi_crc4(crc, msg->msg, msg->bits - top);
> +
> +       msg_push_bits(msg, crc, 4);
> +}
> +
> +static void build_abs_ar_command(struct fsi_gpio_msg *cmd,
> +               uint8_t id, uint32_t addr, size_t size, const void *data)

What is abs_ar? Perhaps a comment would help.

> +{
> +       bool write = !!data;
> +       uint8_t ds;
> +       int i;
> +
> +       cmd->bits = 0;
> +       cmd->msg = 0;
> +
> +       msg_push_bits(cmd, id, 2);
> +       msg_push_bits(cmd, FSI_GPIO_CMD_ABS_AR, 3);
> +       msg_push_bits(cmd, write ? 0 : 1, 1);
> +
> +       /*
> +        * The read/write size is encoded in the lower bits of the address
> +        * (as it must be naturally-aligned), and the following ds bit.
> +        *
> +        *      size    addr:1  addr:0  ds
> +        *      1       x       x       0
> +        *      2       x       0       1
> +        *      4       0       1       1
> +        *
> +        */
> +       ds = size > 1 ? 1 : 0;
> +       addr &= ~(size - 1);
> +       if (size == 4)
> +               addr |= 1;
> +
> +       msg_push_bits(cmd, addr & ((1 << 21) - 1), 21);
> +       msg_push_bits(cmd, ds, 1);
> +       for (i = 0; write && i < size; i++)
> +               msg_push_bits(cmd, ((uint8_t *)data)[i], 8);
> +
> +       msg_push_crc(cmd);
> +}
> +
> +static void build_dpoll_command(struct fsi_gpio_msg *cmd, uint8_t slave_id)
> +{
> +       cmd->bits = 0;
> +       cmd->msg = 0;
> +
> +       msg_push_bits(cmd, slave_id, 2);
> +       msg_push_bits(cmd, FSI_GPIO_CMD_DPOLL, 3);
> +       msg_push_crc(cmd);
> +}
> +
> +static void echo_delay(struct fsi_master_gpio *master)
> +{
> +       set_sda_output(master, 1);
> +       clock_toggle(master, FSI_ECHO_DELAY_CLOCKS);
> +}
> +
> +static void build_term_command(struct fsi_gpio_msg *cmd, uint8_t slave_id)
> +{
> +       cmd->bits = 0;
> +       cmd->msg = 0;
> +
> +       msg_push_bits(cmd, slave_id, 2);
> +       msg_push_bits(cmd, FSI_GPIO_CMD_TERM, 6);
> +       msg_push_crc(cmd);
> +}
> +
> +/*
> + * 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 read_one_response(struct fsi_master_gpio *master,
> +               uint8_t data_size, struct fsi_gpio_msg *msgp, uint8_t *tagp)
> +{
> +       struct fsi_gpio_msg msg;
> +       uint8_t id, tag;
> +       uint32_t crc;
> +       int i;
> +
> +       /* wait for the start bit */
> +       for (i = 0; i < FSI_GPIO_MTOE_COUNT; i++) {
> +               msg.bits = 0;
> +               msg.msg = 0;
> +               serial_in(master, &msg, 1);
> +               if (msg.msg)
> +                       break;
> +       }
> +       if (i >= FSI_GPIO_MTOE_COUNT) {

Testing for == will do.

> +               dev_dbg(master->dev,
> +                       "Master time out waiting for response\n");
> +               fsi_master_gpio_error(master, FSI_GPIO_MTOE);
> +               return -EIO;
> +       }
> +
> +       msg.bits = 0;
> +       msg.msg = 0;
> +
> +       /* Read slave ID & response tag */
> +       serial_in(master, &msg, 4);
> +
> +       id = (msg.msg >> FSI_GPIO_MSG_RESPID_SIZE) & 0x3;
> +       tag = msg.msg & 0x3;
> +
> +       /* if we have an ACK, and we're expecting data, clock the
> +        * data in too
> +        */

The comment should fit on one line.

> +       if (tag == FSI_GPIO_RESP_ACK && data_size)
> +               serial_in(master, &msg, data_size * 8);
> +
> +       /* read CRC */
> +       serial_in(master, &msg, FSI_GPIO_CRC_SIZE);
> +
> +       /* we have a whole message now; check CRC */
> +       crc = fsi_crc4(0, 1, 1);
> +       crc = fsi_crc4(crc, msg.msg, msg.bits);
> +       if (crc) {
> +               dev_dbg(master->dev, "ERR response CRC\n");
> +               fsi_master_gpio_error(master, FSI_GPIO_CRC_INVAL);
> +               return -EIO;
> +       }
> +
> +       if (msgp)
> +               *msgp = msg;
> +       if (tagp)
> +               *tagp = tag;
> +
> +       return 0;
> +}
> +
> +static int issue_term(struct fsi_master_gpio *master, uint8_t slave)
> +{
> +       struct fsi_gpio_msg cmd;
> +       uint8_t tag;
> +       int rc;
> +
> +       build_term_command(&cmd, slave);
> +       serial_out(master, &cmd);
> +       echo_delay(master);
> +
> +       rc = read_one_response(master, 0, NULL, &tag);
> +       if (rc) {

 read_one_response returns zero on success and a negative error code
on failure. Do you mean if (rc < 0)?

> +               dev_err(master->dev,
> +                               "TERM failed; lost communication with slave\n");
> +               return -EIO;
> +       } else if (tag != FSI_GPIO_RESP_ACK) {
> +               dev_err(master->dev, "TERM failed; response %d\n", tag);
> +               return -EIO;
> +       }
> +
> +       return 0;
> +}
> +
> +static int poll_for_response(struct fsi_master_gpio *master,
> +               uint8_t slave, uint8_t size, void *data)
> +{
> +       struct fsi_gpio_msg response, cmd;
> +       int busy_count = 0, rc, i;
> +       uint8_t tag;
> +
> +retry:
> +       rc = read_one_response(master, size, &response, &tag);
> +       if (rc)
> +               return rc;
> +
> +       switch (tag) {
> +       case FSI_GPIO_RESP_ACK:
> +               if (size && data) {
> +                       uint64_t val = response.msg;
> +                       /* clear crc & mask */
> +                       val >>= 4;
> +                       val &= (1ull << (size * 8)) - 1;

You could write this as a GENMASK. Not sure if it's more readable though.

val &= GENMASK_ULL(size * 8 - 1, 0)

> +
> +                       for (i = 0; i < size; i++) {
> +                               ((uint8_t *)data)[size-i-1] =
> +                                       val & 0xff;
> +                               val >>= 8;

Put this on one line so it's easier to read:

((uint8_t *)data)[size-i-1] = val & 0xff;

You can drop the 0xff; this is happening implicitly when assigning to
the uint_8 array.

I suggest creating a local variable of type uint8_t * so you don't
need the cast.

> +                       }
> +               }
> +               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.
> +                */
> +               clock_zeros(master, FSI_GPIO_DPOLL_CLOCKS);
> +               if (busy_count++ < FSI_GPIO_MAX_BUSY) {
> +                       build_dpoll_command(&cmd, slave);
> +                       serial_out(master, &cmd);
> +                       echo_delay(master);
> +                       goto retry;
> +               }
> +               dev_warn(master->dev,
> +                       "ERR slave is stuck in busy state, issuing TERM\n");
> +               issue_term(master, slave);
> +               rc = -EIO;
> +               break;
> +
> +       case FSI_GPIO_RESP_ERRA:
> +       case FSI_GPIO_RESP_ERRC:
> +               dev_dbg(master->dev, "ERR%c received: 0x%x\n",
> +                       tag == FSI_GPIO_RESP_ERRA ? 'A' : 'C',
> +                       (int)response.msg);
> +               fsi_master_gpio_error(master, response.msg);
> +               rc = -EIO;
> +               break;
> +       }
> +
> +       /* Clock the slave enough to be ready for next operation */
> +       clock_zeros(master, FSI_GPIO_PRIME_SLAVE_CLOCKS);

Do you need to do this after an issue_term?

How about after a fsi_master_gpio_error? I suspect yes, for now, as
this function is empty (?!).

> +       return rc;
> +}
> +
> +static int fsi_master_gpio_xfer(struct fsi_master_gpio *master, uint8_t slave,
> +               struct fsi_gpio_msg *cmd, size_t resp_len, void *resp)
> +{
> +       unsigned long flags;
> +       int rc;
> +
> +       spin_lock_irqsave(&fsi_gpio_cmd_lock, flags);
> +       serial_out(master, cmd);
> +       echo_delay(master);
> +       rc = poll_for_response(master, slave, resp_len, resp);
> +       spin_unlock_irqrestore(&fsi_gpio_cmd_lock, flags);
> +
> +       return rc;
> +}
> +
> +static int fsi_master_gpio_read(struct fsi_master *_master, int link,
> +               uint8_t id, 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;

Is this because we only support one link?

> +
> +       build_abs_ar_command(&cmd, id, addr, size, NULL);
> +       return fsi_master_gpio_xfer(master, id, &cmd, size, val);
> +}
> +
> +static int fsi_master_gpio_write(struct fsi_master *_master, int link,
> +               uint8_t id, 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, id, addr, size, val);
> +       return fsi_master_gpio_xfer(master, id, &cmd, 0, NULL);
> +}
> +
> +static int fsi_master_gpio_term(struct fsi_master *_master,
> +               int link, uint8_t id)
> +{
> +       struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
> +       struct fsi_gpio_msg cmd;
> +
> +       if (link != 0)
> +               return -ENODEV;
> +
> +       build_term_command(&cmd, id);
> +       return fsi_master_gpio_xfer(master, id, &cmd, 0, NULL);
> +}
> +
> +/*
> + * Issue a break command on link
> + */

Redundant comment, remove it.

> +static int fsi_master_gpio_break(struct fsi_master *_master, int link)
> +{
> +       struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
> +
> +       if (link != 0)
> +               return -ENODEV;
> +
> +       set_sda_output(master, 1);
> +       sda_out(master, 1);
> +       clock_toggle(master, FSI_PRE_BREAK_CLOCKS);
> +       sda_out(master, 0);
> +       clock_toggle(master, FSI_BREAK_CLOCKS);
> +       echo_delay(master);
> +       sda_out(master, 1);
> +       clock_toggle(master, FSI_POST_BREAK_CLOCKS);
> +
> +       /* Wait for logic reset to take effect */
> +       udelay(200);
> +
> +       return 0;
> +}
> +
> +static void fsi_master_gpio_init(struct fsi_master_gpio *master)
> +{
> +       if (master->gpio_mux)
> +               gpiod_direction_output(master->gpio_mux, 1);
> +       if (master->gpio_trans)
> +               gpiod_direction_output(master->gpio_trans, 1);
> +       if (master->gpio_enable)
> +               gpiod_direction_output(master->gpio_enable, 1);
> +       gpiod_direction_output(master->gpio_clk, 1);
> +       gpiod_direction_output(master->gpio_data, 1);
> +
> +       /* todo: evaluate if clocks can be reduced */
> +       clock_zeros(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 void fsi_master_gpio_release(struct device *dev)
> +{
> +}

Empty function?

> +
> +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;
> +
> +       master->dev = &pdev->dev;
> +       master->master.dev.parent = master->dev;
> +       master->master.dev.release = fsi_master_gpio_release;
> +
> +       gpio = devm_gpiod_get(&pdev->dev, "clock", 0);
> +       if (IS_ERR(gpio)) {
> +               dev_err(&pdev->dev, "failed to get clock gpio\n");
> +               return PTR_ERR(gpio);
> +       }
> +       master->gpio_clk = gpio;
> +
> +       gpio = devm_gpiod_get(&pdev->dev, "data", 0);
> +       if (IS_ERR(gpio)) {
> +               dev_err(&pdev->dev, "failed to get data gpio\n");
> +               return PTR_ERR(gpio);
> +       }
> +       master->gpio_data = gpio;
> +
> +       /* Optional GPIOs */
> +       gpio = devm_gpiod_get_optional(&pdev->dev, "trans", 0);
> +       if (IS_ERR(gpio)) {
> +               dev_err(&pdev->dev, "failed to get trans gpio\n");
> +               return PTR_ERR(gpio);
> +       }
> +       master->gpio_trans = gpio;
> +
> +       gpio = devm_gpiod_get_optional(&pdev->dev, "enable", 0);
> +       if (IS_ERR(gpio)) {
> +               dev_err(&pdev->dev, "failed to get enable gpio\n");
> +               return PTR_ERR(gpio);
> +       }
> +       master->gpio_enable = gpio;
> +
> +       gpio = devm_gpiod_get_optional(&pdev->dev, "mux", 0);
> +       if (IS_ERR(gpio)) {
> +               dev_err(&pdev->dev, "failed to get mux gpio\n");
> +               return PTR_ERR(gpio);
> +       }
> +       master->gpio_mux = gpio;
> +
> +       master->master.n_links = 1;
> +       master->master.read = fsi_master_gpio_read;
> +       master->master.write = fsi_master_gpio_write;
> +       master->master.term = fsi_master_gpio_term;
> +       master->master.send_break = fsi_master_gpio_break;
> +       master->master.link_enable = fsi_master_gpio_link_enable;
> +       platform_set_drvdata(pdev, master);
> +
> +       fsi_master_gpio_init(master);
> +
> +       fsi_master_register(&master->master);

This function can return an error.

> +
> +       return 0;
> +}
> +
> +
> +static int fsi_master_gpio_remove(struct platform_device *pdev)
> +{
> +       struct fsi_master_gpio *master = platform_get_drvdata(pdev);
> +
> +       devm_gpiod_put(&pdev->dev, master->gpio_clk);
> +       devm_gpiod_put(&pdev->dev, master->gpio_data);
> +       if (master->gpio_trans)
> +               devm_gpiod_put(&pdev->dev, master->gpio_trans);
> +       if (master->gpio_enable)
> +               devm_gpiod_put(&pdev->dev, master->gpio_enable);
> +       if (master->gpio_mux)
> +               devm_gpiod_put(&pdev->dev, master->gpio_mux);

Given you're removing the driver here, can you omit the devm_gpiod_put calls?

> +       fsi_master_unregister(&master->master);
> +
> +       return 0;
> +}
> +
> +static const struct of_device_id fsi_master_gpio_match[] = {
> +       { .compatible = "fsi-master-gpio" },
> +       { },
> +};
> +
> +static struct platform_driver fsi_master_gpio_driver = {
> +       .driver = {
> +               .name           = "fsi-master-gpio",
> +               .of_match_table = fsi_master_gpio_match,
> +       },
> +       .probe  = fsi_master_gpio_probe,
> +       .remove = fsi_master_gpio_remove,
> +};
> +
> +module_platform_driver(fsi_master_gpio_driver);
> +MODULE_LICENSE("GPL");
> --
> 1.8.2.2
>

[PATCH v6 19/23] drivers/fsi: Add GPIO based FSI master

[Christopher Bostic]

On 5/10/17 2:30 AM, Joel Stanley wrote:
> Hi Chris,
>
> On Tue, Apr 11, 2017 at 5:17 AM, Christopher Bostic
> <cbostic@linux.vnet.ibm.com> wrote:
>
>> From: Chris Bostic <cbostic@linux.vnet.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 changes from Edward A. James <eajames@us.ibm.com> and Jeremy
>> Kerr <jk@ozlabs.org>.
> I think the series is looking good. I've done a bunch of testing on
> some machines, and it worked for me.
>
> I've got a few comments and things to be clarified below.
>
>> Signed-off-by: Edward A. James <eajames@us.ibm.com>
>> Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
>> Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
>> Signed-off-by: Joel Stanley <joel@jms.id.au>
>> ---
>>   drivers/fsi/Kconfig           |  11 +
>>   drivers/fsi/Makefile          |   1 +
>>   drivers/fsi/fsi-master-gpio.c | 610 ++++++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 622 insertions(+)
>>   create mode 100644 drivers/fsi/fsi-master-gpio.c
>>
>> diff --git a/drivers/fsi/Kconfig b/drivers/fsi/Kconfig
>> index 04c1a0e..448bc3b 100644
>> --- a/drivers/fsi/Kconfig
>> +++ b/drivers/fsi/Kconfig
>> @@ -9,4 +9,15 @@ 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_GPIO
>> +       tristate "GPIO-based FSI master"
>> +       depends on GPIOLIB
>> +       ---help---
>> +       This option enables a FSI master driver using GPIO lines.
>> +
>> +endif
>> +
>>   endmenu
>> diff --git a/drivers/fsi/Makefile b/drivers/fsi/Makefile
>> index db0e5e7..ed28ac0 100644
>> --- a/drivers/fsi/Makefile
>> +++ b/drivers/fsi/Makefile
>> @@ -1,2 +1,3 @@
>>
>>   obj-$(CONFIG_FSI) += fsi-core.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..9fedfaf
>> --- /dev/null
>> +++ b/drivers/fsi/fsi-master-gpio.c
>> @@ -0,0 +1,610 @@
>> +/*
>> + * A FSI master controller, using a simple GPIO bit-banging interface
>> + */
>> +
>> +#include <linux/platform_device.h>
>> +#include <linux/gpio/consumer.h>
>> +#include <linux/module.h>
>> +#include <linux/delay.h>
>> +#include <linux/fsi.h>
>> +#include <linux/device.h>
>> +#include <linux/io.h>
>> +#include <linux/slab.h>
>> +#include <linux/spinlock.h>
>> +
>> +#include "fsi-master.h"
>> +
>> +#define        FSI_GPIO_STD_DLY        1       /* Standard pin delay in nS */
>> +#define        FSI_ECHO_DELAY_CLOCKS   16      /* Number clocks for echo delay */
>> +#define        FSI_PRE_BREAK_CLOCKS    50      /* Number clocks to prep for break */
>> +#define        FSI_BREAK_CLOCKS        256     /* Number of clocks to issue break */
>> +#define        FSI_POST_BREAK_CLOCKS   16000   /* Number clocks to set up cfam */
>> +#define        FSI_INIT_CLOCKS         5000    /* Clock out any old data */
>> +#define        FSI_GPIO_STD_DELAY      10      /* Standard GPIO delay in nS */
>> +                                       /* todo: adjust down as low as */
>> +                                       /* possible or eliminate */
>> +#define        FSI_GPIO_CMD_DPOLL      0x2
>> +#define        FSI_GPIO_CMD_TERM       0x3f
>> +#define FSI_GPIO_CMD_ABS_AR    0x4
>> +
>> +#define        FSI_GPIO_DPOLL_CLOCKS   100      /* < 21 will cause slave to hang */
>> +
>> +/* Bus errors */
>> +#define        FSI_GPIO_ERR_BUSY       1       /* Slave stuck in busy state */
>> +#define        FSI_GPIO_RESP_ERRA      2       /* Any (misc) Error */
>> +#define        FSI_GPIO_RESP_ERRC      3       /* Slave reports master CRC error */
>> +#define        FSI_GPIO_MTOE           4       /* Master time out error */
>> +#define        FSI_GPIO_CRC_INVAL      5       /* Master reports slave CRC error */
>> +
>> +/* Normal slave responses */
>> +#define        FSI_GPIO_RESP_BUSY      1
>> +#define        FSI_GPIO_RESP_ACK       0
>> +#define        FSI_GPIO_RESP_ACKD      4
>> +
>> +#define        FSI_GPIO_MAX_BUSY       100
>> +#define        FSI_GPIO_MTOE_COUNT     1000
>> +#define        FSI_GPIO_DRAIN_BITS     20
>> +#define        FSI_GPIO_CRC_SIZE       4
>> +#define        FSI_GPIO_MSG_ID_SIZE            2
>> +#define        FSI_GPIO_MSG_RESPID_SIZE        2
>> +#define        FSI_GPIO_PRIME_SLAVE_CLOCKS     100
>> +
>> +static DEFINE_SPINLOCK(fsi_gpio_cmd_lock);     /* lock around fsi commands */
> Should this be per-master?

Hi Joel,

I don't think we'd want this per master.   The lock is for the 'top' 
master issuing commands.  Only the top master can initiate any 
transactions on the bus to any devices connected downstream. Downstream 
masters such as hub masters, etc... cannot initiate a command.
>
>> +
>> +struct fsi_master_gpio {
>> +       struct fsi_master       master;
>> +       struct device           *dev;
>> +       struct gpio_desc        *gpio_clk;
>> +       struct gpio_desc        *gpio_data;
>> +       struct gpio_desc        *gpio_trans;    /* Voltage translator */
>> +       struct gpio_desc        *gpio_enable;   /* FSI enable */
>> +       struct gpio_desc        *gpio_mux;      /* Mux control */
>> +};
>> +
>> +#define to_fsi_master_gpio(m) container_of(m, struct fsi_master_gpio, master)
>> +
>> +struct fsi_gpio_msg {
>> +       uint64_t        msg;
>> +       uint8_t         bits;
>> +};
>> +
>> +static void clock_toggle(struct fsi_master_gpio *master, int count)
>> +{
>> +       int i;
>> +
>> +       for (i = 0; i < count; i++) {
>> +               ndelay(FSI_GPIO_STD_DLY);
>> +               gpiod_set_value(master->gpio_clk, 0);
>> +               ndelay(FSI_GPIO_STD_DLY);
>> +               gpiod_set_value(master->gpio_clk, 1);
>> +       }
>> +}
>> +
>> +static int sda_in(struct fsi_master_gpio *master)
>> +{
>> +       int in;
>> +
>> +       ndelay(FSI_GPIO_STD_DLY);
>> +       in = gpiod_get_value(master->gpio_data);
>> +       return in ? 1 : 0;
>> +}
>> +
>> +static void sda_out(struct fsi_master_gpio *master, int value)
>> +{
>> +       gpiod_set_value(master->gpio_data, value);
>> +}
>> +
>> +static void set_sda_input(struct fsi_master_gpio *master)
>> +{
>> +       gpiod_direction_input(master->gpio_data);
>> +       if (master->gpio_trans)
>> +               gpiod_set_value(master->gpio_trans, 0);
> gpiod_set_value checks for the first argument being NULL. You could
> drop your check here.

Will remove the check.
>> +}
>> +
>> +static void set_sda_output(struct fsi_master_gpio *master, int value)
>> +{
>> +       if (master->gpio_trans)
>> +               gpiod_set_value(master->gpio_trans, 1);
>> +       gpiod_direction_output(master->gpio_data, value);
>> +}
>> +
>> +static void clock_zeros(struct fsi_master_gpio *master, int count)
>> +{
>> +       set_sda_output(master, 1);
>> +       clock_toggle(master, count);
>> +}
>> +
>> +static void serial_in(struct fsi_master_gpio *master, struct fsi_gpio_msg *msg,
>> +                       uint8_t num_bits)
>> +{
>> +       uint8_t bit, in_bit;
>> +
>> +       set_sda_input(master);
>> +
>> +       for (bit = 0; bit < num_bits; bit++) {
>> +               clock_toggle(master, 1);
>> +               in_bit = sda_in(master);
>> +               msg->msg <<= 1;
>> +               msg->msg |= ~in_bit & 0x1;      /* Data is negative active */
> "Active low" makes more sense than negative active.

Will reword.

>> +       }
>> +       msg->bits += num_bits;
>> +}
>> +
>> +static void serial_out(struct fsi_master_gpio *master,
>> +                       const struct fsi_gpio_msg *cmd)
>> +{
>> +       uint8_t bit;
>> +       uint64_t msg = ~cmd->msg;       /* Data is negative active */
>> +       uint64_t sda_mask = 0x1ULL << (cmd->bits - 1);
>> +       uint64_t last_bit = ~0;
>> +       int next_bit;
>> +
>> +       if (!cmd->bits) {
>> +               dev_warn(master->dev, "trying to output 0 bits\n");
>> +               return;
>> +       }
>> +       set_sda_output(master, 0);
>> +
>> +       /* Send the start bit */
>> +       sda_out(master, 0);
>> +       clock_toggle(master, 1);
>> +
>> +       /* Send the message */
>> +       for (bit = 0; bit < cmd->bits; bit++) {
>> +               next_bit = (msg & sda_mask) >> (cmd->bits - 1);
>> +               if (last_bit ^ next_bit) {
>> +                       sda_out(master, next_bit);
>> +                       last_bit = next_bit;
>> +               }
>> +               clock_toggle(master, 1);
>> +               msg <<= 1;
>> +       }
>> +}
>> +
>> +static void msg_push_bits(struct fsi_gpio_msg *msg, uint64_t data, int bits)
>> +{
>> +       msg->msg <<= bits;
>> +       msg->msg |= data & ((1ull << bits) - 1);
>> +       msg->bits += bits;
>> +}
>> +
>> +static void msg_push_crc(struct fsi_gpio_msg *msg)
>> +{
>> +       uint8_t crc;
>> +       int top;
>> +
>> +       top = msg->bits & 0x3;
>> +
>> +       /* start bit, and any non-aligned top bits */
>> +       crc = fsi_crc4(0,
>> +                       1 << top | msg->msg >> (msg->bits - top),
>> +                       top + 1);
>> +
>> +       /* aligned bits */
>> +       crc = fsi_crc4(crc, msg->msg, msg->bits - top);
>> +
>> +       msg_push_bits(msg, crc, 4);
>> +}
>> +
>> +static void build_abs_ar_command(struct fsi_gpio_msg *cmd,
>> +               uint8_t id, uint32_t addr, size_t size, const void *data)
> What is abs_ar? Perhaps a comment would help.

ok, will add some text here.

>> +{
>> +       bool write = !!data;
>> +       uint8_t ds;
>> +       int i;
>> +
>> +       cmd->bits = 0;
>> +       cmd->msg = 0;
>> +
>> +       msg_push_bits(cmd, id, 2);
>> +       msg_push_bits(cmd, FSI_GPIO_CMD_ABS_AR, 3);
>> +       msg_push_bits(cmd, write ? 0 : 1, 1);
>> +
>> +       /*
>> +        * The read/write size is encoded in the lower bits of the address
>> +        * (as it must be naturally-aligned), and the following ds bit.
>> +        *
>> +        *      size    addr:1  addr:0  ds
>> +        *      1       x       x       0
>> +        *      2       x       0       1
>> +        *      4       0       1       1
>> +        *
>> +        */
>> +       ds = size > 1 ? 1 : 0;
>> +       addr &= ~(size - 1);
>> +       if (size == 4)
>> +               addr |= 1;
>> +
>> +       msg_push_bits(cmd, addr & ((1 << 21) - 1), 21);
>> +       msg_push_bits(cmd, ds, 1);
>> +       for (i = 0; write && i < size; i++)
>> +               msg_push_bits(cmd, ((uint8_t *)data)[i], 8);
>> +
>> +       msg_push_crc(cmd);
>> +}
>> +
>> +static void build_dpoll_command(struct fsi_gpio_msg *cmd, uint8_t slave_id)
>> +{
>> +       cmd->bits = 0;
>> +       cmd->msg = 0;
>> +
>> +       msg_push_bits(cmd, slave_id, 2);
>> +       msg_push_bits(cmd, FSI_GPIO_CMD_DPOLL, 3);
>> +       msg_push_crc(cmd);
>> +}
>> +
>> +static void echo_delay(struct fsi_master_gpio *master)
>> +{
>> +       set_sda_output(master, 1);
>> +       clock_toggle(master, FSI_ECHO_DELAY_CLOCKS);
>> +}
>> +
>> +static void build_term_command(struct fsi_gpio_msg *cmd, uint8_t slave_id)
>> +{
>> +       cmd->bits = 0;
>> +       cmd->msg = 0;
>> +
>> +       msg_push_bits(cmd, slave_id, 2);
>> +       msg_push_bits(cmd, FSI_GPIO_CMD_TERM, 6);
>> +       msg_push_crc(cmd);
>> +}
>> +
>> +/*
>> + * 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 read_one_response(struct fsi_master_gpio *master,
>> +               uint8_t data_size, struct fsi_gpio_msg *msgp, uint8_t *tagp)
>> +{
>> +       struct fsi_gpio_msg msg;
>> +       uint8_t id, tag;
>> +       uint32_t crc;
>> +       int i;
>> +
>> +       /* wait for the start bit */
>> +       for (i = 0; i < FSI_GPIO_MTOE_COUNT; i++) {
>> +               msg.bits = 0;
>> +               msg.msg = 0;
>> +               serial_in(master, &msg, 1);
>> +               if (msg.msg)
>> +                       break;
>> +       }
>> +       if (i >= FSI_GPIO_MTOE_COUNT) {
> Testing for == will do.
Will change.

>> +               dev_dbg(master->dev,
>> +                       "Master time out waiting for response\n");
>> +               fsi_master_gpio_error(master, FSI_GPIO_MTOE);
>> +               return -EIO;
>> +       }
>> +
>> +       msg.bits = 0;
>> +       msg.msg = 0;
>> +
>> +       /* Read slave ID & response tag */
>> +       serial_in(master, &msg, 4);
>> +
>> +       id = (msg.msg >> FSI_GPIO_MSG_RESPID_SIZE) & 0x3;
>> +       tag = msg.msg & 0x3;
>> +
>> +       /* if we have an ACK, and we're expecting data, clock the
>> +        * data in too
>> +        */
> The comment should fit on one line.
Will correct.

>> +       if (tag == FSI_GPIO_RESP_ACK && data_size)
>> +               serial_in(master, &msg, data_size * 8);
>> +
>> +       /* read CRC */
>> +       serial_in(master, &msg, FSI_GPIO_CRC_SIZE);
>> +
>> +       /* we have a whole message now; check CRC */
>> +       crc = fsi_crc4(0, 1, 1);
>> +       crc = fsi_crc4(crc, msg.msg, msg.bits);
>> +       if (crc) {
>> +               dev_dbg(master->dev, "ERR response CRC\n");
>> +               fsi_master_gpio_error(master, FSI_GPIO_CRC_INVAL);
>> +               return -EIO;
>> +       }
>> +
>> +       if (msgp)
>> +               *msgp = msg;
>> +       if (tagp)
>> +               *tagp = tag;
>> +
>> +       return 0;
>> +}
>> +
>> +static int issue_term(struct fsi_master_gpio *master, uint8_t slave)
>> +{
>> +       struct fsi_gpio_msg cmd;
>> +       uint8_t tag;
>> +       int rc;
>> +
>> +       build_term_command(&cmd, slave);
>> +       serial_out(master, &cmd);
>> +       echo_delay(master);
>> +
>> +       rc = read_one_response(master, 0, NULL, &tag);
>> +       if (rc) {
>   read_one_response returns zero on success and a negative error code
> on failure. Do you mean if (rc < 0)?

I can make the change.  Makes sense to make that more explicit.

>> +               dev_err(master->dev,
>> +                               "TERM failed; lost communication with slave\n");
>> +               return -EIO;
>> +       } else if (tag != FSI_GPIO_RESP_ACK) {
>> +               dev_err(master->dev, "TERM failed; response %d\n", tag);
>> +               return -EIO;
>> +       }
>> +
>> +       return 0;
>> +}
>> +
>> +static int poll_for_response(struct fsi_master_gpio *master,
>> +               uint8_t slave, uint8_t size, void *data)
>> +{
>> +       struct fsi_gpio_msg response, cmd;
>> +       int busy_count = 0, rc, i;
>> +       uint8_t tag;
>> +
>> +retry:
>> +       rc = read_one_response(master, size, &response, &tag);
>> +       if (rc)
>> +               return rc;
>> +
>> +       switch (tag) {
>> +       case FSI_GPIO_RESP_ACK:
>> +               if (size && data) {
>> +                       uint64_t val = response.msg;
>> +                       /* clear crc & mask */
>> +                       val >>= 4;
>> +                       val &= (1ull << (size * 8)) - 1;
>>
>> You could write this as a GENMASK. Not sure if it's more readable though.
>>
>> val &= GENMASK_ULL(size * 8 - 1, 0)

I could make that change but as you mention I'm not sure its any more 
readable.  For now
I'd prefer to leave as is unless there is a strong preference to update it.

>> +
>> +                       for (i = 0; i < size; i++) {
>> +                               ((uint8_t *)data)[size-i-1] =
>> +                                       val & 0xff;
>> +                               val >>= 8;
> Put this on one line so it's easier to read:
>
> ((uint8_t *)data)[size-i-1] = val & 0xff;
>
> You can drop the 0xff; this is happening implicitly when assigning to
> the uint_8 array.
>
> I suggest creating a local variable of type uint8_t * so you don't
> need the cast.

Ok, will update.
>
>> +                       }
>> +               }
>> +               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.
>> +                */
>> +               clock_zeros(master, FSI_GPIO_DPOLL_CLOCKS);
>> +               if (busy_count++ < FSI_GPIO_MAX_BUSY) {
>> +                       build_dpoll_command(&cmd, slave);
>> +                       serial_out(master, &cmd);
>> +                       echo_delay(master);
>> +                       goto retry;
>> +               }
>> +               dev_warn(master->dev,
>> +                       "ERR slave is stuck in busy state, issuing TERM\n");
>> +               issue_term(master, slave);
>> +               rc = -EIO;
>> +               break;
>> +
>> +       case FSI_GPIO_RESP_ERRA:
>> +       case FSI_GPIO_RESP_ERRC:
>> +               dev_dbg(master->dev, "ERR%c received: 0x%x\n",
>> +                       tag == FSI_GPIO_RESP_ERRA ? 'A' : 'C',
>> +                       (int)response.msg);
>> +               fsi_master_gpio_error(master, response.msg);
>> +               rc = -EIO;
>> +               break;
>> +       }
>> +
>> +       /* Clock the slave enough to be ready for next operation */
>> +       clock_zeros(master, FSI_GPIO_PRIME_SLAVE_CLOCKS);
> Do you need to do this after an issue_term?

Yes, from hardware tests I found it is necessary to clock the slave.  
The slave relies
on the master to provide it clocks so that it can advance its state 
machines (if configured
to rely on external master clocks).

> How about after a fsi_master_gpio_error? I suspect yes, for now, as
> this function is empty (?!).

Not sure I understand.   Are you asking if clock_zeroes() should be done 
after
fsi_master_gpio_error( ) ?
>> +       return rc;
>> +}
>> +
>> +static int fsi_master_gpio_xfer(struct fsi_master_gpio *master, uint8_t slave,
>> +               struct fsi_gpio_msg *cmd, size_t resp_len, void *resp)
>> +{
>> +       unsigned long flags;
>> +       int rc;
>> +
>> +       spin_lock_irqsave(&fsi_gpio_cmd_lock, flags);
>> +       serial_out(master, cmd);
>> +       echo_delay(master);
>> +       rc = poll_for_response(master, slave, resp_len, resp);
>> +       spin_unlock_irqrestore(&fsi_gpio_cmd_lock, flags);
>> +
>> +       return rc;
>> +}
>> +
>> +static int fsi_master_gpio_read(struct fsi_master *_master, int link,
>> +               uint8_t id, 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;
> Is this because we only support one link?

Correct.  Some of the general scan function allows for multiple master 
links but
like here it is stubbed out.   Will be modified once multi link support 
is required.
>
>> +
>> +       build_abs_ar_command(&cmd, id, addr, size, NULL);
>> +       return fsi_master_gpio_xfer(master, id, &cmd, size, val);
>> +}
>> +
>> +static int fsi_master_gpio_write(struct fsi_master *_master, int link,
>> +               uint8_t id, 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, id, addr, size, val);
>> +       return fsi_master_gpio_xfer(master, id, &cmd, 0, NULL);
>> +}
>> +
>> +static int fsi_master_gpio_term(struct fsi_master *_master,
>> +               int link, uint8_t id)
>> +{
>> +       struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
>> +       struct fsi_gpio_msg cmd;
>> +
>> +       if (link != 0)
>> +               return -ENODEV;
>> +
>> +       build_term_command(&cmd, id);
>> +       return fsi_master_gpio_xfer(master, id, &cmd, 0, NULL);
>> +}
>> +
>> +/*
>> + * Issue a break command on link
>> + */
> Redundant comment, remove it.
Removing...

>> +static int fsi_master_gpio_break(struct fsi_master *_master, int link)
>> +{
>> +       struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
>> +
>> +       if (link != 0)
>> +               return -ENODEV;
>> +
>> +       set_sda_output(master, 1);
>> +       sda_out(master, 1);
>> +       clock_toggle(master, FSI_PRE_BREAK_CLOCKS);
>> +       sda_out(master, 0);
>> +       clock_toggle(master, FSI_BREAK_CLOCKS);
>> +       echo_delay(master);
>> +       sda_out(master, 1);
>> +       clock_toggle(master, FSI_POST_BREAK_CLOCKS);
>> +
>> +       /* Wait for logic reset to take effect */
>> +       udelay(200);
>> +
>> +       return 0;
>> +}
>> +
>> +static void fsi_master_gpio_init(struct fsi_master_gpio *master)
>> +{
>> +       if (master->gpio_mux)
>> +               gpiod_direction_output(master->gpio_mux, 1);
>> +       if (master->gpio_trans)
>> +               gpiod_direction_output(master->gpio_trans, 1);
>> +       if (master->gpio_enable)
>> +               gpiod_direction_output(master->gpio_enable, 1);
>> +       gpiod_direction_output(master->gpio_clk, 1);
>> +       gpiod_direction_output(master->gpio_data, 1);
>> +
>> +       /* todo: evaluate if clocks can be reduced */
>> +       clock_zeros(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 void fsi_master_gpio_release(struct device *dev)
>> +{
>> +}
> Empty function?

Will remove.

>> +
>> +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;
>> +
>> +       master->dev = &pdev->dev;
>> +       master->master.dev.parent = master->dev;
>> +       master->master.dev.release = fsi_master_gpio_release;
>> +
>> +       gpio = devm_gpiod_get(&pdev->dev, "clock", 0);
>> +       if (IS_ERR(gpio)) {
>> +               dev_err(&pdev->dev, "failed to get clock gpio\n");
>> +               return PTR_ERR(gpio);
>> +       }
>> +       master->gpio_clk = gpio;
>> +
>> +       gpio = devm_gpiod_get(&pdev->dev, "data", 0);
>> +       if (IS_ERR(gpio)) {
>> +               dev_err(&pdev->dev, "failed to get data gpio\n");
>> +               return PTR_ERR(gpio);
>> +       }
>> +       master->gpio_data = gpio;
>> +
>> +       /* Optional GPIOs */
>> +       gpio = devm_gpiod_get_optional(&pdev->dev, "trans", 0);
>> +       if (IS_ERR(gpio)) {
>> +               dev_err(&pdev->dev, "failed to get trans gpio\n");
>> +               return PTR_ERR(gpio);
>> +       }
>> +       master->gpio_trans = gpio;
>> +
>> +       gpio = devm_gpiod_get_optional(&pdev->dev, "enable", 0);
>> +       if (IS_ERR(gpio)) {
>> +               dev_err(&pdev->dev, "failed to get enable gpio\n");
>> +               return PTR_ERR(gpio);
>> +       }
>> +       master->gpio_enable = gpio;
>> +
>> +       gpio = devm_gpiod_get_optional(&pdev->dev, "mux", 0);
>> +       if (IS_ERR(gpio)) {
>> +               dev_err(&pdev->dev, "failed to get mux gpio\n");
>> +               return PTR_ERR(gpio);
>> +       }
>> +       master->gpio_mux = gpio;
>> +
>> +       master->master.n_links = 1;
>> +       master->master.read = fsi_master_gpio_read;
>> +       master->master.write = fsi_master_gpio_write;
>> +       master->master.term = fsi_master_gpio_term;
>> +       master->master.send_break = fsi_master_gpio_break;
>> +       master->master.link_enable = fsi_master_gpio_link_enable;
>> +       platform_set_drvdata(pdev, master);
>> +
>> +       fsi_master_gpio_init(master);
>> +
>> +       fsi_master_register(&master->master);
> This function can return an error.

Don't understand.   On detect of error it returns immediately.  If makes 
it to end then no
error encountered.
>> +
>> +       return 0;
>> +}
>> +
>> +
>> +static int fsi_master_gpio_remove(struct platform_device *pdev)
>> +{
>> +       struct fsi_master_gpio *master = platform_get_drvdata(pdev);
>> +
>> +       devm_gpiod_put(&pdev->dev, master->gpio_clk);
>> +       devm_gpiod_put(&pdev->dev, master->gpio_data);
>> +       if (master->gpio_trans)
>> +               devm_gpiod_put(&pdev->dev, master->gpio_trans);
>> +       if (master->gpio_enable)
>> +               devm_gpiod_put(&pdev->dev, master->gpio_enable);
>> +       if (master->gpio_mux)
>> +               devm_gpiod_put(&pdev->dev, master->gpio_mux);
> Given you're removing the driver here, can you omit the devm_gpiod_put calls?

This would be called on an unbind op so I'm thinking we'd still want to 
explicitly release
the pins with devm_gpiod_put( ).

Thanks for your input,
Chris.
>> +       fsi_master_unregister(&master->master);
>> +
>> +       return 0;
>> +}
>> +
>> +static const struct of_device_id fsi_master_gpio_match[] = {
>> +       { .compatible = "fsi-master-gpio" },
>> +       { },
>> +};
>> +
>> +static struct platform_driver fsi_master_gpio_driver = {
>> +       .driver = {
>> +               .name           = "fsi-master-gpio",
>> +               .of_match_table = fsi_master_gpio_match,
>> +       },
>> +       .probe  = fsi_master_gpio_probe,
>> +       .remove = fsi_master_gpio_remove,
>> +};
>> +
>> +module_platform_driver(fsi_master_gpio_driver);
>> +MODULE_LICENSE("GPL");
>> --
>> 1.8.2.2
>>

[PATCH v6 19/23] drivers/fsi: Add GPIO based FSI master

[Jeremy Kerr]

Hi Chris,

> I don't think we'd want this per master.   The lock is for the 'top'
> master issuing commands.  Only the top master can initiate any
> transactions on the bus to any devices connected downstream. Downstream
> masters such as hub masters, etc... cannot initiate a command.

I think what Joel meant there was that we have it per *GPIO* master; if
there are two GPIO masters on a system, there's no need to provide
mutual exclusion to each (separate) set of GPIOs.

To implement this, we'd just move the lock into struct fsi_master_gpio.

Cheers,


Jeremy

[PATCH v6 19/23] drivers/fsi: Add GPIO based FSI master

[Christopher Bostic]

On 5/10/17 8:58 PM, Jeremy Kerr wrote:
> Hi Chris,
>
>> I don't think we'd want this per master.   The lock is for the 'top'
>> master issuing commands.  Only the top master can initiate any
>> transactions on the bus to any devices connected downstream. Downstream
>> masters such as hub masters, etc... cannot initiate a command.
> I think what Joel meant there was that we have it per *GPIO* master; if
> there are two GPIO masters on a system, there's no need to provide
> mutual exclusion to each (separate) set of GPIOs.
>
> To implement this, we'd just move the lock into struct fsi_master_gpio.

Hi Jeremy,

Understand now -will make the change.

Thanks
-Chris

> Cheers,
>
>
> Jeremy
>

[PATCH v6 20/23] drivers/fsi/gpio: Add tracepoints for GPIO master

[Christopher Bostic]

From: Jeremy Kerr <jk@ozlabs.org>

Add trace points for key GPIO operations of the GPIO based FSI
master.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
---
 drivers/fsi/fsi-master-gpio.c          |  9 +++++
 include/trace/events/fsi_master_gpio.h | 68 ++++++++++++++++++++++++++++++++++
 2 files changed, 77 insertions(+)
 create mode 100644 include/trace/events/fsi_master_gpio.h

diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
index 9fedfaf..5d9e0b0 100644
--- a/drivers/fsi/fsi-master-gpio.c
+++ b/drivers/fsi/fsi-master-gpio.c
@@ -61,6 +61,9 @@ struct fsi_master_gpio {
 	struct gpio_desc	*gpio_mux;	/* Mux control */
 };
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/fsi_master_gpio.h>
+
 #define to_fsi_master_gpio(m) container_of(m, struct fsi_master_gpio, master)
 
 struct fsi_gpio_msg {
@@ -128,6 +131,8 @@ static void serial_in(struct fsi_master_gpio *master, struct fsi_gpio_msg *msg,
 		msg->msg |= ~in_bit & 0x1;	/* Data is negative active */
 	}
 	msg->bits += num_bits;
+
+	trace_fsi_master_gpio_in(master, num_bits, msg->msg);
 }
 
 static void serial_out(struct fsi_master_gpio *master,
@@ -139,6 +144,8 @@ static void serial_out(struct fsi_master_gpio *master,
 	uint64_t last_bit = ~0;
 	int next_bit;
 
+	trace_fsi_master_gpio_out(master, cmd->bits, cmd->msg);
+
 	if (!cmd->bits) {
 		dev_warn(master->dev, "trying to output 0 bits\n");
 		return;
@@ -464,6 +471,8 @@ static int fsi_master_gpio_break(struct fsi_master *_master, int link)
 	if (link != 0)
 		return -ENODEV;
 
+	trace_fsi_master_gpio_break(master);
+
 	set_sda_output(master, 1);
 	sda_out(master, 1);
 	clock_toggle(master, FSI_PRE_BREAK_CLOCKS);
diff --git a/include/trace/events/fsi_master_gpio.h b/include/trace/events/fsi_master_gpio.h
new file mode 100644
index 0000000..11b36c1
--- /dev/null
+++ b/include/trace/events/fsi_master_gpio.h
@@ -0,0 +1,68 @@
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM fsi_master_gpio
+
+#if !defined(_TRACE_FSI_MASTER_GPIO_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_FSI_MASTER_GPIO_H
+
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(fsi_master_gpio_in,
+	TP_PROTO(const struct fsi_master_gpio *master, int bits, uint64_t msg),
+	TP_ARGS(master, bits, msg),
+	TP_STRUCT__entry(
+		__field(int,	master_idx)
+		__field(int,	bits)
+		__field(uint64_t, msg)
+	),
+	TP_fast_assign(
+		__entry->master_idx = master->master.idx;
+		__entry->bits = bits;
+		__entry->msg  = msg & ((1ull<<bits) - 1);
+	),
+	TP_printk("fsi-gpio%d => %0*llx[%d]",
+		__entry->master_idx,
+		(__entry->bits + 3) / 4,
+		__entry->msg,
+		__entry->bits
+	)
+);
+
+TRACE_EVENT(fsi_master_gpio_out,
+	TP_PROTO(const struct fsi_master_gpio *master, int bits, uint64_t msg),
+	TP_ARGS(master, bits, msg),
+	TP_STRUCT__entry(
+		__field(int,	master_idx)
+		__field(int,	bits)
+		__field(uint64_t, msg)
+	),
+	TP_fast_assign(
+		__entry->master_idx = master->master.idx;
+		__entry->bits = bits;
+		__entry->msg  = msg & ((1ull<<bits) - 1);
+	),
+	TP_printk("fsi-gpio%d <= %0*llx[%d]",
+		__entry->master_idx,
+		(__entry->bits + 3) / 4,
+		__entry->msg,
+		__entry->bits
+	)
+);
+
+TRACE_EVENT(fsi_master_gpio_break,
+	TP_PROTO(const struct fsi_master_gpio *master),
+	TP_ARGS(master),
+	TP_STRUCT__entry(
+		__field(int,	master_idx)
+	),
+	TP_fast_assign(
+		__entry->master_idx = master->master.idx;
+	),
+	TP_printk("fsi-gpio%d ----break---",
+		__entry->master_idx
+	)
+);
+
+#endif /* _TRACE_FSI_MASTER_GPIO_H */
+
+#include <trace/define_trace.h>
-- 
1.8.2.2

[PATCH v6 21/23] drivers/fsi: Add SCOM FSI client device driver

[Christopher Bostic]

From: Chris Bostic <cbostic@linux.vnet.ibm.com>

Create a simple SCOM engine device driver that reads and writes
its control registers via an FSI bus.

Includes changes from Edward A. James <eajames@us.ibm.com>.

Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
Signed-off-by: Joel Stanley <joel@jms.id.au>
Signed-off-by: Edward A. James <eajames@us.ibm.com>
Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
---
 drivers/fsi/Kconfig    |   5 +
 drivers/fsi/Makefile   |   1 +
 drivers/fsi/fsi-scom.c | 263 +++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 269 insertions(+)
 create mode 100644 drivers/fsi/fsi-scom.c

diff --git a/drivers/fsi/Kconfig b/drivers/fsi/Kconfig
index 448bc3b..e8dad25 100644
--- a/drivers/fsi/Kconfig
+++ b/drivers/fsi/Kconfig
@@ -18,6 +18,11 @@ config FSI_MASTER_GPIO
 	---help---
 	This option enables a FSI master driver using GPIO lines.
 
+config FSI_SCOM
+	tristate "SCOM FSI client device driver"
+	---help---
+	This option enables an FSI based SCOM device driver.
+
 endif
 
 endmenu
diff --git a/drivers/fsi/Makefile b/drivers/fsi/Makefile
index ed28ac0..3466f08 100644
--- a/drivers/fsi/Makefile
+++ b/drivers/fsi/Makefile
@@ -1,3 +1,4 @@
 
 obj-$(CONFIG_FSI) += fsi-core.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..98d062f
--- /dev/null
+++ b/drivers/fsi/fsi-scom.c
@@ -0,0 +1,263 @@
+/*
+ * 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>
+#include <linux/slab.h>
+#include <linux/miscdevice.h>
+#include <linux/list.h>
+#include <linux/idr.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
+
+struct scom_device {
+	struct list_head link;
+	struct fsi_device *fsi_dev;
+	struct miscdevice mdev;
+	char	name[32];
+	int idx;
+};
+
+#define to_scom_dev(x)		container_of((x), struct scom_device, mdev)
+
+static struct list_head scom_devices;
+
+static DEFINE_IDA(scom_ida);
+
+static int put_scom(struct scom_device *scom_dev, uint64_t value,
+			uint32_t addr)
+{
+	int rc;
+	uint32_t data;
+
+	data = cpu_to_be32(SCOM_RESET_CMD);
+	rc = fsi_device_write(scom_dev->fsi_dev, SCOM_RESET_REG, &data,
+				sizeof(uint32_t));
+	if (rc)
+		return rc;
+
+	data = cpu_to_be32((value >> 32) & 0xffffffff);
+	rc = fsi_device_write(scom_dev->fsi_dev, SCOM_DATA0_REG, &data,
+				sizeof(uint32_t));
+	if (rc)
+		return rc;
+
+	data = cpu_to_be32(value & 0xffffffff);
+	rc = fsi_device_write(scom_dev->fsi_dev, SCOM_DATA1_REG, &data,
+				sizeof(uint32_t));
+	if (rc)
+		return rc;
+
+	data = cpu_to_be32(SCOM_WRITE_CMD | addr);
+	return fsi_device_write(scom_dev->fsi_dev, SCOM_CMD_REG, &data,
+				sizeof(uint32_t));
+}
+
+static int get_scom(struct scom_device *scom_dev, uint64_t *value,
+			uint32_t addr)
+{
+	uint32_t result, data;
+	int rc;
+
+	*value = 0ULL;
+	data = cpu_to_be32(addr);
+	rc = fsi_device_write(scom_dev->fsi_dev, SCOM_CMD_REG, &data,
+				sizeof(uint32_t));
+	if (rc)
+		return rc;
+
+	rc = fsi_device_read(scom_dev->fsi_dev, SCOM_DATA0_REG, &result,
+				sizeof(uint32_t));
+	if (rc)
+		return rc;
+
+	*value |= (uint64_t)cpu_to_be32(result) << 32;
+	rc = fsi_device_read(scom_dev->fsi_dev, SCOM_DATA1_REG, &result,
+				sizeof(uint32_t));
+	if (rc)
+		return rc;
+
+	*value |= cpu_to_be32(result);
+
+	return 0;
+}
+
+static ssize_t scom_read(struct file *filep, char __user *buf, size_t len,
+			loff_t *offset)
+{
+	int rc;
+	struct miscdevice *mdev =
+				(struct miscdevice *)filep->private_data;
+	struct scom_device *scom = to_scom_dev(mdev);
+	struct device *dev = &scom->fsi_dev->dev;
+	uint64_t val;
+
+	if (len != sizeof(uint64_t))
+		return -EINVAL;
+
+	rc = get_scom(scom, &val, *offset);
+	if (rc) {
+		dev_dbg(dev, "get_scom fail:%d\n", rc);
+		return rc;
+	}
+
+	rc = copy_to_user(buf, &val, len);
+	if (rc)
+		dev_dbg(dev, "copy to user failed:%d\n", rc);
+
+	return rc ? rc : len;
+}
+
+static ssize_t scom_write(struct file *filep, const char __user *buf,
+			size_t len, loff_t *offset)
+{
+	int rc;
+	struct miscdevice *mdev = filep->private_data;
+	struct scom_device *scom = to_scom_dev(mdev);
+	struct device *dev = &scom->fsi_dev->dev;
+	uint64_t val;
+
+	if (len != sizeof(uint64_t))
+		return -EINVAL;
+
+	rc = copy_from_user(&val, buf, len);
+	if (rc) {
+		dev_dbg(dev, "copy from user failed:%d\n", rc);
+		return -EINVAL;
+	}
+
+	rc = put_scom(scom, val, *offset);
+	if (rc) {
+		dev_dbg(dev, "put_scom failed with:%d\n", rc);
+		return rc;
+	}
+
+	return len;
+}
+
+static loff_t scom_llseek(struct file *file, loff_t offset, int whence)
+{
+	switch (whence) {
+	case SEEK_CUR:
+		break;
+	case SEEK_SET:
+		file->f_pos = offset;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return offset;
+}
+
+static const struct file_operations scom_fops = {
+	.owner	= THIS_MODULE,
+	.llseek	= scom_llseek,
+	.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 = devm_kzalloc(dev, sizeof(*scom), GFP_KERNEL);
+	if (!scom)
+		return -ENOMEM;
+
+	scom->idx = ida_simple_get(&scom_ida, 1, INT_MAX, GFP_KERNEL);
+	snprintf(scom->name, sizeof(scom->name), "scom%d", 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;
+	list_add(&scom->link, &scom_devices);
+
+	return misc_register(&scom->mdev);
+}
+
+static int scom_remove(struct device *dev)
+{
+	struct scom_device *scom, *scom_tmp;
+	struct fsi_device *fsi_dev = to_fsi_dev(dev);
+
+	list_for_each_entry_safe(scom, scom_tmp, &scom_devices, link) {
+		if (scom->fsi_dev == fsi_dev) {
+			list_del(&scom->link);
+			ida_simple_remove(&scom_ida, scom->idx);
+			misc_deregister(&scom->mdev);
+		}
+	}
+
+	return 0;
+}
+
+static struct fsi_device_id scom_ids[] = {
+	{
+		.engine_type = FSI_ENGID_SCOM,
+		.version = FSI_VERSION_ANY,
+	},
+	{ 0 }
+};
+
+static struct fsi_driver scom_drv = {
+	.id_table = scom_ids,
+	.drv = {
+		.name = "scom",
+		.bus = &fsi_bus_type,
+		.probe = scom_probe,
+		.remove = scom_remove,
+	}
+};
+
+static int scom_init(void)
+{
+	INIT_LIST_HEAD(&scom_devices);
+	return fsi_driver_register(&scom_drv);
+}
+
+static void scom_exit(void)
+{
+	struct list_head *pos;
+	struct scom_device *scom;
+
+	list_for_each(pos, &scom_devices) {
+		scom = list_entry(pos, struct scom_device, link);
+		misc_deregister(&scom->mdev);
+		devm_kfree(&scom->fsi_dev->dev, scom);
+	}
+	fsi_driver_unregister(&scom_drv);
+}
+
+module_init(scom_init);
+module_exit(scom_exit);
+MODULE_LICENSE("GPL");
-- 
1.8.2.2

[PATCH v6 22/23] drivers/ fsi: Add hub master support

[Christopher Bostic]

From: Chris Bostic <cbostic@linux.vnet.ibm.com>

Add an engine driver to expose a "hub" FSI master - which has a set of
control registers in the engine address space, and uses a chunk of the
slave address space for actual FSI communication.

Additional changes from Jeremy Kerr <jk@ozlabs.org>.

Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Joel Stanley <joel@jms.id.au>
---
 drivers/fsi/Kconfig          |   8 ++
 drivers/fsi/Makefile         |   1 +
 drivers/fsi/fsi-master-hub.c | 327 +++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 336 insertions(+)
 create mode 100644 drivers/fsi/fsi-master-hub.c

diff --git a/drivers/fsi/Kconfig b/drivers/fsi/Kconfig
index e8dad25..fc031ac 100644
--- a/drivers/fsi/Kconfig
+++ b/drivers/fsi/Kconfig
@@ -18,6 +18,14 @@ config FSI_MASTER_GPIO
 	---help---
 	This option enables a FSI master driver using GPIO lines.
 
+config FSI_MASTER_HUB
+	tristate "FSI hub master"
+	---help---
+	This option enables a FSI hub master driver.  Hub is a type of FSI
+	master that is connected to the upstream master via a slave.  Hubs
+	allow chaining of FSI links to an arbitrary depth.  This allows for
+	a high target device fanout.
+
 config FSI_SCOM
 	tristate "SCOM FSI client device driver"
 	---help---
diff --git a/drivers/fsi/Makefile b/drivers/fsi/Makefile
index 3466f08..65eb99d 100644
--- a/drivers/fsi/Makefile
+++ b/drivers/fsi/Makefile
@@ -1,4 +1,5 @@
 
 obj-$(CONFIG_FSI) += fsi-core.o
+obj-$(CONFIG_FSI_MASTER_HUB) += fsi-master-hub.o
 obj-$(CONFIG_FSI_MASTER_GPIO) += fsi-master-gpio.o
 obj-$(CONFIG_FSI_SCOM) += fsi-scom.o
diff --git a/drivers/fsi/fsi-master-hub.c b/drivers/fsi/fsi-master-hub.c
new file mode 100644
index 0000000..133b9bf
--- /dev/null
+++ b/drivers/fsi/fsi-master-hub.c
@@ -0,0 +1,327 @@
+/*
+ * FSI hub master 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/delay.h>
+#include <linux/fsi.h>
+#include <linux/module.h>
+#include <linux/slab.h>
+
+#include "fsi-master.h"
+
+/* Control Registers */
+#define FSI_MMODE		0x0		/* R/W: mode */
+#define FSI_MDLYR		0x4		/* R/W: delay */
+#define FSI_MCRSP		0x8		/* R/W: clock rate */
+#define FSI_MENP0		0x10		/* R/W: enable */
+#define FSI_MLEVP0		0x18		/* R: plug detect */
+#define FSI_MSENP0		0x18		/* S: Set enable */
+#define FSI_MCENP0		0x20		/* C: Clear enable */
+#define FSI_MAEB		0x70		/* R: Error address */
+#define FSI_MVER		0x74		/* R: master version/type */
+#define FSI_MRESP0		0xd0		/* W: Port reset */
+#define FSI_MESRB0		0x1d0		/* R: Master error status */
+#define FSI_MRESB0		0x1d0		/* W: Reset bridge */
+#define FSI_MECTRL		0x2e0		/* W: Error control */
+
+/* MMODE: Mode control */
+#define FSI_MMODE_EIP		0x80000000	/* Enable interrupt polling */
+#define FSI_MMODE_ECRC		0x40000000	/* Enable error recovery */
+#define FSI_MMODE_EPC		0x10000000	/* Enable parity checking */
+#define FSI_MMODE_P8_TO_LSB	0x00000010	/* Timeout value LSB */
+						/*   MSB=1, LSB=0 is 0.8 ms */
+						/*   MSB=0, LSB=1 is 0.9 ms */
+#define FSI_MMODE_CRS0SHFT	18		/* Clk rate selection 0 shift */
+#define FSI_MMODE_CRS0MASK	0x3ff		/* Clk rate selection 0 mask */
+#define FSI_MMODE_CRS1SHFT	8		/* Clk rate selection 1 shift */
+#define FSI_MMODE_CRS1MASK	0x3ff		/* Clk rate selection 1 mask */
+
+/* MRESB: Reset brindge */
+#define FSI_MRESB_RST_GEN	0x80000000	/* General reset */
+#define FSI_MRESB_RST_ERR	0x40000000	/* Error Reset */
+
+/* MRESB: Reset port */
+#define FSI_MRESP_RST_ALL_MASTER 0x20000000	/* Reset all FSI masters */
+#define FSI_MRESP_RST_ALL_LINK	0x10000000	/* Reset all FSI port contr. */
+#define FSI_MRESP_RST_MCR	0x08000000	/* Reset FSI master reg. */
+#define FSI_MRESP_RST_PYE	0x04000000	/* Reset FSI parity error */
+#define FSI_MRESP_RST_ALL	0xfc000000	/* Reset any error */
+
+/* MECTRL: Error control */
+#define FSI_MECTRL_EOAE		0x8000		/* Enable machine check when */
+						/* master 0 in error */
+#define FSI_MECTRL_P8_AUTO_TERM	0x4000		/* Auto terminate */
+
+#define FSI_ENGID_HUB_MASTER		0x1c
+#define FSI_HUB_LINK_OFFSET		0x80000
+#define FSI_HUB_LINK_SIZE		0x80000
+#define FSI_HUB_MASTER_MAX_LINKS	8
+
+#define FSI_LINK_ENABLE_SETUP_TIME	10	/* in mS */
+
+/*
+ * FSI hub master support
+ *
+ * A hub master increases the number of potential target devices that the
+ * primary FSI master can access. For each link a primary master supports,
+ * each of those links can in turn be chained to a hub master with multiple
+ * links of its own.
+ *
+ * The hub is controlled by a set of control registers exposed as a regular fsi
+ * device (the hub->upstream device), and provides access to the downstream FSI
+ * bus as through an address range on the slave itself (->addr and ->size).
+ *
+ * [This differs from "cascaded" masters, which expose the entire downstream
+ * bus entirely through the fsi device address range, and so have a smaller
+ * accessible address space.]
+ */
+struct fsi_master_hub {
+	struct fsi_master	master;
+	struct fsi_device	*upstream;
+	uint32_t		addr, size;	/* slave-relative addr of */
+						/* master address space */
+};
+
+#define to_fsi_master_hub(m) container_of(m, struct fsi_master_hub, master)
+
+static int hub_master_read(struct fsi_master *master, int link,
+			uint8_t id, uint32_t addr, void *val, size_t size)
+{
+	struct fsi_master_hub *hub = to_fsi_master_hub(master);
+
+	if (id != 0)
+		return -EINVAL;
+
+	addr += hub->addr + (link * FSI_HUB_LINK_SIZE);
+	return fsi_slave_read(hub->upstream->slave, addr, val, size);
+}
+
+static int hub_master_write(struct fsi_master *master, int link,
+			uint8_t id, uint32_t addr, const void *val, size_t size)
+{
+	struct fsi_master_hub *hub = to_fsi_master_hub(master);
+
+	if (id != 0)
+		return -EINVAL;
+
+	addr += hub->addr + (link * FSI_HUB_LINK_SIZE);
+	return fsi_slave_write(hub->upstream->slave, addr, val, size);
+}
+
+static int hub_master_break(struct fsi_master *master, int link)
+{
+	uint32_t addr, cmd;
+
+	addr = 0x4;
+	cmd = cpu_to_be32(0xc0de0000);
+
+	return hub_master_write(master, link, 0, addr, &cmd, sizeof(cmd));
+}
+
+static int hub_master_link_enable(struct fsi_master *master, int link)
+{
+	struct fsi_master_hub *hub = to_fsi_master_hub(master);
+	int idx, bit;
+	__be32 reg;
+	int rc;
+
+	idx = link / 32;
+	bit = link % 32;
+
+	reg = cpu_to_be32(0x80000000 >> bit);
+
+	rc = fsi_device_write(hub->upstream, FSI_MSENP0 + (4 * idx), &reg, 4);
+
+	mdelay(FSI_LINK_ENABLE_SETUP_TIME);
+
+	fsi_device_read(hub->upstream, FSI_MENP0 + (4 * idx), &reg, 4);
+
+	return rc;
+}
+
+static void hub_master_release(struct device *dev)
+{
+	struct fsi_master_hub *hub = to_fsi_master_hub(dev_to_fsi_master(dev));
+
+	kfree(hub);
+}
+
+/* mmode encoders */
+static inline u32 fsi_mmode_crs0(u32 x)
+{
+	return (x & FSI_MMODE_CRS0MASK) << FSI_MMODE_CRS0SHFT;
+}
+
+static inline u32 fsi_mmode_crs1(u32 x)
+{
+	return (x & FSI_MMODE_CRS1MASK) << FSI_MMODE_CRS1SHFT;
+}
+
+static int hub_master_init(struct fsi_master_hub *hub)
+{
+	struct fsi_device *dev = hub->upstream;
+	__be32 reg;
+	int rc;
+
+	reg = cpu_to_be32(FSI_MRESP_RST_ALL_MASTER | FSI_MRESP_RST_ALL_LINK
+			| FSI_MRESP_RST_MCR | FSI_MRESP_RST_PYE);
+	rc = fsi_device_write(dev, FSI_MRESP0, &reg, sizeof(reg));
+	if (rc)
+		return rc;
+
+	/* Initialize the MFSI (hub master) engine */
+	reg = cpu_to_be32(FSI_MRESP_RST_ALL_MASTER | FSI_MRESP_RST_ALL_LINK
+			| FSI_MRESP_RST_MCR | FSI_MRESP_RST_PYE);
+	rc = fsi_device_write(dev, FSI_MRESP0, &reg, sizeof(reg));
+	if (rc)
+		return rc;
+
+	reg = cpu_to_be32(FSI_MECTRL_EOAE | FSI_MECTRL_P8_AUTO_TERM);
+	rc = fsi_device_write(dev, FSI_MECTRL, &reg, sizeof(reg));
+	if (rc)
+		return rc;
+
+	reg = cpu_to_be32(FSI_MMODE_EIP | FSI_MMODE_ECRC | FSI_MMODE_EPC
+			| fsi_mmode_crs0(1) | fsi_mmode_crs1(1)
+			| FSI_MMODE_P8_TO_LSB);
+	rc = fsi_device_write(dev, FSI_MMODE, &reg, sizeof(reg));
+	if (rc)
+		return rc;
+
+	reg = cpu_to_be32(0xffff0000);
+	rc = fsi_device_write(dev, FSI_MDLYR, &reg, sizeof(reg));
+	if (rc)
+		return rc;
+
+	reg = ~0;
+	rc = fsi_device_write(dev, FSI_MSENP0, &reg, sizeof(reg));
+	if (rc)
+		return rc;
+
+	/* Leave enabled long enough for master logic to set up */
+	mdelay(FSI_LINK_ENABLE_SETUP_TIME);
+
+	rc = fsi_device_write(dev, FSI_MCENP0, &reg, sizeof(reg));
+	if (rc)
+		return rc;
+
+	rc = fsi_device_read(dev, FSI_MAEB, &reg, sizeof(reg));
+	if (rc)
+		return rc;
+
+	reg = cpu_to_be32(FSI_MRESP_RST_ALL_MASTER | FSI_MRESP_RST_ALL_LINK);
+	rc = fsi_device_write(dev, FSI_MRESP0, &reg, sizeof(reg));
+	if (rc)
+		return rc;
+
+	rc = fsi_device_read(dev, FSI_MLEVP0, &reg, sizeof(reg));
+	if (rc)
+		return rc;
+
+	/* Reset the master bridge */
+	reg = cpu_to_be32(FSI_MRESB_RST_GEN);
+	rc = fsi_device_write(dev, FSI_MRESB0, &reg, sizeof(reg));
+	if (rc)
+		return rc;
+
+	reg = cpu_to_be32(FSI_MRESB_RST_ERR);
+	return fsi_device_write(dev, FSI_MRESB0, &reg, sizeof(reg));
+}
+
+static int hub_master_probe(struct device *dev)
+{
+	struct fsi_device *fsi_dev = to_fsi_dev(dev);
+	struct fsi_master_hub *hub;
+	uint32_t reg, links;
+	__be32 __reg;
+	int rc;
+
+	rc = fsi_device_read(fsi_dev, FSI_MVER, &__reg, sizeof(__reg));
+	if (rc)
+		return rc;
+
+	reg = be32_to_cpu(__reg);
+	links = (reg >> 8) & 0xff;
+	dev_info(dev, "hub version %08x (%d links)\n", reg, links);
+
+	rc = fsi_slave_claim_range(fsi_dev->slave, FSI_HUB_LINK_OFFSET,
+			FSI_HUB_LINK_SIZE * links);
+	if (rc) {
+		dev_err(dev, "can't claim slave address range for links");
+		return rc;
+	}
+
+	hub = kzalloc(sizeof(*hub), GFP_KERNEL);
+	if (!hub) {
+		rc = -ENOMEM;
+		goto err_release;
+	}
+
+	hub->addr = FSI_HUB_LINK_OFFSET;
+	hub->size = FSI_HUB_LINK_SIZE * links;
+	hub->upstream = fsi_dev;
+
+	hub->master.dev.parent = dev;
+	hub->master.dev.release = hub_master_release;
+
+	hub->master.n_links = links;
+	hub->master.read = hub_master_read;
+	hub->master.write = hub_master_write;
+	hub->master.send_break = hub_master_break;
+	hub->master.link_enable = hub_master_link_enable;
+
+	dev_set_drvdata(dev, hub);
+
+	hub_master_init(hub);
+
+	rc = fsi_master_register(&hub->master);
+	if (!rc)
+		return 0;
+
+	kfree(hub);
+err_release:
+	fsi_slave_release_range(fsi_dev->slave, FSI_HUB_LINK_OFFSET,
+			FSI_HUB_LINK_SIZE * links);
+	return rc;
+}
+
+static int hub_master_remove(struct device *dev)
+{
+	struct fsi_master_hub *hub = dev_get_drvdata(dev);
+
+	fsi_master_unregister(&hub->master);
+	fsi_slave_release_range(hub->upstream->slave, hub->addr, hub->size);
+	return 0;
+}
+
+static struct fsi_device_id hub_master_ids[] = {
+	{
+		.engine_type = FSI_ENGID_HUB_MASTER,
+		.version = FSI_VERSION_ANY,
+	},
+	{ 0 }
+};
+
+static struct fsi_driver hub_master_driver = {
+	.id_table = hub_master_ids,
+	.drv = {
+		.name = "fsi-master-hub",
+		.bus = &fsi_bus_type,
+		.probe = hub_master_probe,
+		.remove = hub_master_remove,
+	}
+};
+
+module_fsi_driver(hub_master_driver);
+MODULE_LICENSE("GPL");
-- 
1.8.2.2

[PATCH v6 23/23] drivers/fsi: Use asynchronous slave mode

[Christopher Bostic]

From: Jeremy Kerr <jk@ozlabs.org>

For slaves that are behind a software-clocked master, we want FSI CFAMs
to run asynchronously to the FSI clock, so set up our slaves to be in
async mode.

Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
Signed-off-by: Chris Bostic <cbostic@linux.vnet.ibm.com>
---
 drivers/fsi/fsi-core.c        | 22 +++++++++++++++++++++-
 drivers/fsi/fsi-master-gpio.c |  1 +
 drivers/fsi/fsi-master.h      |  2 ++
 3 files changed, 24 insertions(+), 1 deletion(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 747d0e3..a6ed34f 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -47,6 +47,7 @@
 #define FSI_SMODE		0x0	/* R/W: Mode register */
 #define FSI_SISC		0x8	/* R/W: Interrupt condition */
 #define FSI_SSTAT		0x14	/* R  : Slave status */
+#define FSI_LLMODE		0x100	/* R/W: Link layer mode register */
 
 /*
  * SMODE fields
@@ -62,6 +63,11 @@
 #define FSI_SMODE_LBCRR_SHIFT	8		/* Clk ratio shift */
 #define FSI_SMODE_LBCRR_MASK	0xf		/* Clk ratio mask */
 
+/*
+ * LLMODE fields
+ */
+#define FSI_LLMODE_ASYNC	0x1
+
 #define FSI_SLAVE_SIZE_23b		0x800000
 
 static DEFINE_IDA(master_ida);
@@ -560,8 +566,8 @@ static void fsi_slave_release(struct device *dev)
 
 static int fsi_slave_init(struct fsi_master *master, int link, uint8_t id)
 {
+	uint32_t chip_id, llmode;
 	struct fsi_slave *slave;
-	uint32_t chip_id;
 	uint8_t crc;
 	int rc;
 
@@ -597,6 +603,20 @@ static int fsi_slave_init(struct fsi_master *master, int link, uint8_t id)
 		return -ENODEV;
 	}
 
+	/* If we're behind a master that doesn't provide a self-running bus
+	 * clock, put the slave into async mode
+	 */
+	if (master->flags & FSI_MASTER_FLAG_SWCLOCK) {
+		llmode = cpu_to_be32(FSI_LLMODE_ASYNC);
+		rc = fsi_master_write(master, link, id,
+				FSI_SLAVE_BASE + FSI_LLMODE,
+				&llmode, sizeof(llmode));
+		if (rc)
+			dev_warn(&master->dev,
+				"can't set llmode on slave:%02x:%02x %d\n",
+				link, id, rc);
+	}
+
 	/* We can communicate with a slave; create the slave device and
 	 * register.
 	 */
diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
index 5d9e0b0..d5cce88 100644
--- a/drivers/fsi/fsi-master-gpio.c
+++ b/drivers/fsi/fsi-master-gpio.c
@@ -569,6 +569,7 @@ static int fsi_master_gpio_probe(struct platform_device *pdev)
 	master->gpio_mux = gpio;
 
 	master->master.n_links = 1;
+	master->master.flags = FSI_MASTER_FLAG_SWCLOCK;
 	master->master.read = fsi_master_gpio_read;
 	master->master.write = fsi_master_gpio_write;
 	master->master.term = fsi_master_gpio_term;
diff --git a/drivers/fsi/fsi-master.h b/drivers/fsi/fsi-master.h
index d6a4885..fd39924 100644
--- a/drivers/fsi/fsi-master.h
+++ b/drivers/fsi/fsi-master.h
@@ -19,6 +19,8 @@
 
 #include <linux/device.h>
 
+#define FSI_MASTER_FLAG_SWCLOCK	0x1
+
 struct fsi_master {
 	struct device	dev;
 	int		idx;
-- 
1.8.2.2