[PATCH linux dev-4.7 v2 0/7] drivers/fsi: Add hub master

[PATCH linux dev-4.7 v2 0/7] drivers/fsi: Add hub master

[Christopher Bostic]

Adds the FSI hub master functionality to the OpenFSI device driver.
Hub master is a FSI client device driver that registers with the
core and is notified when devices it wants, hub master engine and
links, are discovered.

v2: Restructured to move hub into the core instead of a separate
    slave device driver.

Christopher Bostic (7):
  drivers/fsi: Add hub master scan detect
  drivers/fsi: Initialize hub master
  drivers/fsi: Define hub master callbacks
  drivers/fsi: Move common read write ops into single function
  drivers/fsi: Add retry on bus error detect
  drivers/fsi: Cleanup bus errors
  tools/testing/selftests: Add fsi command line tool


-- 
1.8.2.2

[PATCH linux dev-4.7 v2 1/7] drivers/fsi: Add hub master scan detect

[Christopher Bostic]

On detect of hub master engine in config space set up a master
device and register it with the core.

Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>

---

Changes in V2:

- Refactor structures for hub master. Previously hub master engine
  was considered a slave engine.
---
 drivers/fsi/fsi-core.c | 47 +++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 45 insertions(+), 2 deletions(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index d63a892..646bb43 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -43,6 +43,11 @@
 
 #define FSI_IPOLL_PERIOD		msecs_to_jiffies(fsi_ipoll_period_ms)
 
+#define	FSI_ENGID_HUB_MASTER		0x1c
+#define	FSI_ENGID_HUB_LINK		0x1d
+#define	FSI_HUB_LINK_OFFSET		0x80000
+#define	FSI_HUB_MASTER_MAX_LINKS	8
+
 static const int engine_page_size = 0x400;
 static struct task_struct *master_ipoll;
 static unsigned int fsi_ipoll_period_ms = 100;
@@ -58,6 +63,14 @@ struct fsi_slave {
 	uint8_t			id;
 };
 
+struct fsi_master_hub {
+	struct fsi_master	master;
+	struct fsi_slave	*slave;
+	uint32_t		control_regs;	/* slave-relative addr regs */
+	uint32_t		base;		/* slave-relative addr of */
+						/* master address space */
+};
+
 #define to_fsi_slave(d) container_of(d, struct fsi_slave, dev)
 
 static int fsi_slave_read(struct fsi_slave *slave, uint32_t addr,
@@ -203,12 +216,18 @@ static int fsi_slave_write(struct fsi_slave *slave, uint32_t addr,
 			slave->id, addr, val, size);
 }
 
+static int hub_master_init(struct fsi_master_hub *hub)
+{
+	return 0;
+}
+
 static int fsi_slave_scan(struct fsi_slave *slave)
 {
 	uint32_t engine_addr;
 	uint32_t conf;
 	int rc, i;
 	uint8_t si1s_bit = 1;
+	struct fsi_master_hub *hub;
 
 	INIT_LIST_HEAD(&slave->my_engines);
 
@@ -252,7 +271,31 @@ static int fsi_slave_scan(struct fsi_slave *slave)
 		 * Unused address areas are marked by a zero type value; this
 		 * skips the defined address areas
 		 */
-		if (type != 0 && slots != 0) {
+		if (type != 0) {
+
+			if (type == FSI_ENGID_HUB_MASTER) {
+				hub = kzalloc(sizeof(*hub), GFP_KERNEL);
+				if (!hub)
+					return -ENOMEM;
+
+				hub->base = FSI_HUB_LINK_OFFSET;
+				hub->control_regs = engine_addr;
+				hub->slave = slave;
+				rc = hub_master_init(hub);
+
+				continue;
+			}
+			if (type == FSI_ENGID_HUB_LINK) {
+				if (!hub)
+					continue;
+
+				hub->master.n_links++;
+				if (hub->master.n_links ==
+						(FSI_HUB_MASTER_MAX_LINKS * 2))
+					rc = fsi_master_register(&hub->master);
+
+				continue;
+			}
 
 			/* create device */
 			dev = fsi_create_device(slave);
@@ -597,7 +640,7 @@ DEVICE_ATTR(fsi_ipoll_period, S_IRUGO | S_IWUSR, fsi_ipoll_period_show,
 
 int fsi_master_register(struct fsi_master *master)
 {
-	if (!master || !master->dev)
+	if (!master)
 		return -EINVAL;
 
 	master->idx = ida_simple_get(&master_ida, 0, INT_MAX, GFP_KERNEL);
-- 
1.8.2.2

[PATCH linux dev-4.7 v2 2/7] drivers/fsi: Initialize hub master

[Christopher Bostic]

Initialize the hub master control registers.  Set up clock delays,
error handling behavior, toggle link enables.

Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>

---

Changes in V2:

- Add some description of what a hub master is and how it differs
  from a cascaded master.
---
 drivers/fsi/fsi-core.c   | 116 ++++++++++++++++++++++++++++++++++++++++++++++-
 drivers/fsi/fsi-master.h |  57 +++++++++++++++++++++++
 2 files changed, 172 insertions(+), 1 deletion(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 646bb43..4199188 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -20,6 +20,7 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/jiffies.h>
+#include <linux/delay.h>
 
 #include "fsi-master.h"
 
@@ -216,11 +217,124 @@ static int fsi_slave_write(struct fsi_slave *slave, uint32_t addr,
 			slave->id, addr, val, size);
 }
 
-static int hub_master_init(struct fsi_master_hub *hub)
+/*
+ * 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
+ * hub links of its own.  Hubs differ from cascaded masters (cMFSI) in the
+ * total addressable range per link -hubs having address ranges that are much
+ * larger.
+ */
+int hub_master_read(struct fsi_master *master, int linkno, uint8_t slave,
+			uint32_t addr, void *val, size_t size)
+{
+	return 0;
+}
+
+int hub_master_write(struct fsi_master *master, int linkno, uint8_t slave,
+			uint32_t addr, const void *val, size_t size)
 {
 	return 0;
 }
 
+int hub_master_break(struct fsi_master *master, int linkno)
+{
+	return 0;
+}
+
+int hub_master_link_enable(struct fsi_master *master, int linkno)
+{
+	return 0;
+}
+
+static int hub_master_init(struct fsi_master_hub *hub)
+{
+	int rc;
+	uint32_t mver;
+	struct fsi_master *master = &hub->master;
+
+	master->read = hub_master_read;
+	master->write = hub_master_write;
+	master->send_break = hub_master_break;
+	master->link_enable = hub_master_link_enable;
+
+	/* Initialize the MFSI (hub master) engine */
+	rc = fsi_slave_read(hub->slave, hub->control_regs + FSI_MVER, &mver,
+				sizeof(mver));
+	if (rc)
+		return rc;
+
+	mver = FSI_MRESP_RST_ALL_MASTER | FSI_MRESP_RST_ALL_LINK
+			| FSI_MRESP_RST_MCR | FSI_MRESP_RST_PYE;
+	rc = fsi_slave_write(hub->slave, hub->control_regs + FSI_MRESP0, &mver,
+				sizeof(mver));
+	if (rc)
+		return rc;
+
+	mver = FSI_MECTRL_EOAE | FSI_MECTRL_P8_AUTO_TERM;
+	rc = fsi_slave_write(hub->slave, hub->control_regs + FSI_MECTRL, &mver,
+				sizeof(mver));
+	if (rc)
+		return rc;
+
+	mver = FSI_MMODE_EIP | FSI_MMODE_ECRC | FSI_MMODE_EPC
+			| fsi_mmode_crs0(1) | fsi_mmode_crs1(1)
+			| FSI_MMODE_P8_TO_LSB;
+	rc = fsi_slave_write(hub->slave, hub->control_regs + FSI_MMODE, &mver,
+				sizeof(mver));
+	if (rc)
+		return rc;
+
+	mver = 0xffff0000;
+	rc = fsi_slave_write(hub->slave, hub->control_regs + FSI_MDLYR, &mver,
+				sizeof(mver));
+	if (rc)
+		return rc;
+
+	mver = ~0;
+	rc = fsi_slave_write(hub->slave, hub->control_regs + FSI_MSENP0, &mver,
+				sizeof(mver));
+	if (rc)
+		return rc;
+
+	/* Leave enabled long enough for master logic to set up */
+	udelay(1000);
+
+	rc = fsi_slave_write(hub->slave, hub->control_regs + FSI_MCENP0, &mver,
+				sizeof(mver));
+	if (rc)
+		return rc;
+
+	rc = fsi_slave_read(hub->slave, hub->control_regs + FSI_MAEB, &mver,
+				sizeof(mver));
+	if (rc)
+		return rc;
+
+	mver = FSI_MRESP_RST_ALL_MASTER | FSI_MRESP_RST_ALL_LINK;
+	rc = fsi_slave_write(hub->slave, hub->control_regs + FSI_MRESP0, &mver,
+				sizeof(mver));
+	if (rc)
+		return rc;
+
+	rc = fsi_slave_read(hub->slave, hub->control_regs + FSI_MLEVP0, &mver,
+				sizeof(mver));
+	if (rc)
+		return rc;
+
+	/* Reset the master bridge */
+	mver = FSI_MRESB_RST_GEN;
+	rc = fsi_slave_write(hub->slave, hub->control_regs + FSI_MRESB0, &mver,
+				sizeof(mver));
+	if (rc)
+		return rc;
+
+	mver = FSI_MRESB_RST_ERR;
+	return fsi_slave_write(hub->slave, hub->control_regs + FSI_MRESB0,
+				&mver, sizeof(mver));
+}
+
 static int fsi_slave_scan(struct fsi_slave *slave)
 {
 	uint32_t engine_addr;
diff --git a/drivers/fsi/fsi-master.h b/drivers/fsi/fsi-master.h
index 3737404..4a3176b 100644
--- a/drivers/fsi/fsi-master.h
+++ b/drivers/fsi/fsi-master.h
@@ -19,6 +19,52 @@
 
 #include <linux/device.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 */
+						/* Rolf Fritz Nov 20, 2013: */
+						/*   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 L_MSB_MASK(x)		(0x80000000 >> (x))
+
 struct fsi_master {
 	struct list_head my_slaves;
 	bool		slave_list;
@@ -61,4 +107,15 @@ extern int fsi_master_start_ipoll(struct fsi_master *master);
  */
 uint8_t fsi_crc4(uint8_t c, uint64_t x, int bits);
 
+/* 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;
+}
+
 #endif /* DRIVERS_FSI_MASTER_H */
-- 
1.8.2.2

[PATCH linux dev-4.7 v2 3/7] drivers/fsi: Define hub master callbacks

[Christopher Bostic]

Define hub master read, write, link enable, and break ops.

Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
---
 drivers/fsi/fsi-core.c | 36 ++++++++++++++++++++++++++++++++----
 1 file changed, 32 insertions(+), 4 deletions(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 4199188..360c02a 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -27,6 +27,7 @@
 #define DEBUG
 
 #define FSI_N_SLAVES	4
+#define FSI_BREAK	0xc0de0000
 
 #define FSI_SLAVE_CONF_NEXT_MASK	0x80000000
 #define FSI_SLAVE_CONF_SLOTS_MASK	0x00ff0000
@@ -47,6 +48,7 @@
 #define	FSI_ENGID_HUB_MASTER		0x1c
 #define	FSI_ENGID_HUB_LINK		0x1d
 #define	FSI_HUB_LINK_OFFSET		0x80000
+#define	FSI_MASTER_HUB_LINK_SIZE	0x80000
 #define	FSI_HUB_MASTER_MAX_LINKS	8
 
 static const int engine_page_size = 0x400;
@@ -72,6 +74,7 @@ struct fsi_master_hub {
 						/* master address space */
 };
 
+#define to_fsi_master_hub(d) container_of(d, struct fsi_master_hub, master)
 #define to_fsi_slave(d) container_of(d, struct fsi_slave, dev)
 
 static int fsi_slave_read(struct fsi_slave *slave, uint32_t addr,
@@ -230,23 +233,47 @@ static int fsi_slave_write(struct fsi_slave *slave, uint32_t addr,
 int hub_master_read(struct fsi_master *master, int linkno, uint8_t slave,
 			uint32_t addr, void *val, size_t size)
 {
-	return 0;
+	struct fsi_master_hub *hub = to_fsi_master_hub(master);
+
+	addr += (linkno * FSI_MASTER_HUB_LINK_SIZE) + hub->base;
+	return fsi_slave_read(hub->slave, addr, val, size);
 }
 
 int hub_master_write(struct fsi_master *master, int linkno, uint8_t slave,
 			uint32_t addr, const void *val, size_t size)
 {
-	return 0;
+	struct fsi_master_hub *hub = to_fsi_master_hub(master);
+
+	addr += (linkno * FSI_MASTER_HUB_LINK_SIZE) + hub->base;
+	return fsi_slave_write(hub->slave, addr, val, size);
 }
 
 int hub_master_break(struct fsi_master *master, int linkno)
 {
-	return 0;
+	struct fsi_master_hub *hub = to_fsi_master_hub(master);
+	uint32_t command;
+	uint32_t break_offset = 0x4; /* hw workaround */
+	uint32_t addr;
+
+	command = FSI_BREAK;
+	addr = (linkno * FSI_MASTER_HUB_LINK_SIZE) + hub->base;
+	return fsi_slave_write(hub->slave, addr + break_offset, &command,
+			sizeof(command));
 }
 
 int hub_master_link_enable(struct fsi_master *master, int linkno)
 {
-	return 0;
+	struct fsi_master_hub *hub = to_fsi_master_hub(master);
+	uint32_t menp = L_MSB_MASK(linkno);
+	int rc;
+
+	rc = fsi_slave_write(hub->slave, hub->control_regs + FSI_MSENP0, &menp,
+				sizeof(menp));
+
+	/* Wait for hw to finish enable */
+	mdelay(10);
+
+	return rc;
 }
 
 static int hub_master_init(struct fsi_master_hub *hub)
@@ -571,6 +598,7 @@ static int fsi_slave_init(struct fsi_master *master,
 	slave->id = slave_id;
 	slave->dev.parent = master->dev;
 	slave->dev.release = fsi_slave_release;
+	slave->link = link;
 
 	dev_set_name(&slave->dev, "slave@%02x:%02x", link, slave_id);
 	rc = device_register(&slave->dev);
-- 
1.8.2.2

[PATCH linux dev-4.7 v2 4/7] drivers/fsi: Move common read write ops into single function

[Christopher Bostic]

Move common operations between gpio reads/writes into a new
utility send_command.

Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>

---

v2: Separate the break out of common read/write elements into its
    own separate patch.
---
 drivers/fsi/fsi-master-gpio.c | 36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
index 3ed82ea..8aec538 100644
--- a/drivers/fsi/fsi-master-gpio.c
+++ b/drivers/fsi/fsi-master-gpio.c
@@ -354,26 +354,34 @@ static void build_abs_ar_command(struct fsi_gpio_msg *cmd, uint64_t mode,
 	cmd->msg >>= (64 - cmd->bits);
 }
 
+static int send_command(struct fsi_master_gpio *master,
+			struct fsi_gpio_msg *cmd, uint8_t expected,
+			size_t size, void *val)
+{
+	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, expected, size, val);
+	spin_unlock_irqrestore(&fsi_gpio_cmd_lock, flags);
+
+	return rc;
+}
+
 static int fsi_master_gpio_read(struct fsi_master *_master, int link,
 		uint8_t slave, uint32_t addr, void *val, size_t size)
 {
 	struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
 	struct fsi_gpio_msg cmd;
 	int rc;
-	unsigned long flags;
 
 	if (link != 0)
 		return -ENODEV;
 
 	build_abs_ar_command(&cmd, FSI_GPIO_CMD_READ, slave, addr, size, NULL);
-
-	spin_lock_irqsave(&fsi_gpio_cmd_lock, flags);
-	serial_out(master, &cmd);
-	echo_delay(master);
-	rc = poll_for_response(master, FSI_GPIO_RESP_ACKD, size, val);
-	spin_unlock_irqrestore(&fsi_gpio_cmd_lock, flags);
-
-	return rc;
+	return send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
 }
 
 static int fsi_master_gpio_write(struct fsi_master *_master, int link,
@@ -382,20 +390,12 @@ static int fsi_master_gpio_write(struct fsi_master *_master, int link,
 	struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
 	struct fsi_gpio_msg cmd;
 	int rc;
-	unsigned long flags;
 
 	if (link != 0)
 		return -ENODEV;
 
 	build_abs_ar_command(&cmd, FSI_GPIO_CMD_WRITE, slave, addr, size, val);
-
-	spin_lock_irqsave(&fsi_gpio_cmd_lock, flags);
-	serial_out(master, &cmd);
-	echo_delay(master);
-	rc = poll_for_response(master, FSI_GPIO_RESP_ACK, size, NULL);
-	spin_unlock_irqrestore(&fsi_gpio_cmd_lock, flags);
-
-	return rc;
+	return send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
 }
 
 /*
-- 
1.8.2.2

[PATCH linux dev-4.7 v2 5/7] drivers/fsi: Add retry on bus error detect

[Christopher Bostic]

After each gpio master read/write check for bus errors.  If
errors detected then clean it up and retry the original operation
again.  If second attempt succeeds then don't report original
error to caller.

Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
---
 drivers/fsi/fsi-core.c        |  4 ++++
 drivers/fsi/fsi-master-gpio.c | 24 ++++++++++++++++++++++--
 drivers/fsi/fsi-master.h      |  3 +++
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 360c02a..428675f 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -639,6 +639,10 @@ static int fsi_master_break(struct fsi_master *master, int link)
 	return 0;
 }
 
+void fsi_master_handle_error(struct fsi_master *master, uint32_t addr)
+{
+}
+
 static int fsi_master_scan(struct fsi_master *master)
 {
 	int link, slave_id, rc;
diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
index 8aec538..5b502eb 100644
--- a/drivers/fsi/fsi-master-gpio.c
+++ b/drivers/fsi/fsi-master-gpio.c
@@ -381,7 +381,17 @@ static int fsi_master_gpio_read(struct fsi_master *_master, int link,
 		return -ENODEV;
 
 	build_abs_ar_command(&cmd, FSI_GPIO_CMD_READ, slave, addr, size, NULL);
-	return send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
+	rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
+	if (rc) {
+		fsi_master_handle_error(&master->master, addr);
+
+		/* Try again */
+		rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
+		if (rc)
+			fsi_master_handle_error(&master->master, addr);
+	}
+
+	return rc;
 }
 
 static int fsi_master_gpio_write(struct fsi_master *_master, int link,
@@ -395,7 +405,17 @@ static int fsi_master_gpio_write(struct fsi_master *_master, int link,
 		return -ENODEV;
 
 	build_abs_ar_command(&cmd, FSI_GPIO_CMD_WRITE, slave, addr, size, val);
-	return send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
+	rc = send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
+	if (rc) {
+		fsi_master_handle_error(&master->master, addr);
+
+		/* Try again */
+		rc = send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
+		if (rc)
+			fsi_master_handle_error(&master->master, addr);
+	}
+
+	return rc;
 }
 
 /*
diff --git a/drivers/fsi/fsi-master.h b/drivers/fsi/fsi-master.h
index 4a3176b..4ed416c 100644
--- a/drivers/fsi/fsi-master.h
+++ b/drivers/fsi/fsi-master.h
@@ -19,6 +19,8 @@
 
 #include <linux/device.h>
 
+#include "fsi-master.h"
+
 /* Control Registers */
 #define FSI_MMODE		0x0		/* R/W: mode */
 #define FSI_MDLYR		0x4		/* R/W: delay */
@@ -85,6 +87,7 @@ struct fsi_master {
 extern int fsi_master_register(struct fsi_master *master);
 extern void fsi_master_unregister(struct fsi_master *master);
 extern int fsi_master_start_ipoll(struct fsi_master *master);
+extern void fsi_master_handle_error(struct fsi_master *master, uint32_t addr);
 
 /**
  * crc4 helper: Given a starting crc4 state @c, calculate the crc4 vaue of @x,
-- 
1.8.2.2

[PATCH linux dev-4.7 v2 6/7] drivers/fsi: Cleanup bus errors

[Christopher Bostic]

Define the bus error cleanup method for primary and hub links.

Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>

---

v2: Change to atomic operation when checking for nested error
    handling.

    Move slave access details out of gpio master and into the
    core.

    Only allow hub link 1 scans to cut down on scan bus errors.
---
 drivers/fsi/fsi-core.c | 34 ++++++++++++++++++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 428675f..b660d33 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -21,6 +21,7 @@
 #include <linux/slab.h>
 #include <linux/jiffies.h>
 #include <linux/delay.h>
+#include <linux/bitops.h>
 
 #include "fsi-master.h"
 
@@ -42,6 +43,9 @@
 
 #define FSI_PEEK_BASE			0x410
 #define	FSI_SLAVE_BASE			0x800
+#define	FSI_HUB_CONTROL			0x3400
+
+#define	FSI_SLAVE_SMODE_DFLT		0xa0ff0100
 
 #define FSI_IPOLL_PERIOD		msecs_to_jiffies(fsi_ipoll_period_ms)
 
@@ -54,6 +58,10 @@
 static const int engine_page_size = 0x400;
 static struct task_struct *master_ipoll;
 static unsigned int fsi_ipoll_period_ms = 100;
+static unsigned long fsi_state;
+
+/* state flags */
+#define	FSI_IN_ERR_CLEANUP		0
 
 static DEFINE_IDA(master_ida);
 
@@ -255,6 +263,10 @@ int hub_master_break(struct fsi_master *master, int linkno)
 	uint32_t break_offset = 0x4; /* hw workaround */
 	uint32_t addr;
 
+	/* config table lists 2 entries per hub link */
+	if (linkno >= (master->n_links / 2))
+		return -ENODEV;
+
 	command = FSI_BREAK;
 	addr = (linkno * FSI_MASTER_HUB_LINK_SIZE) + hub->base;
 	return fsi_slave_write(hub->slave, addr + break_offset, &command,
@@ -641,6 +653,28 @@ static int fsi_master_break(struct fsi_master *master, int link)
 
 void fsi_master_handle_error(struct fsi_master *master, uint32_t addr)
 {
+	uint32_t smode = FSI_SLAVE_SMODE_DFLT;
+
+	/* Avoid nested error handling */
+	if (test_and_set_bit(FSI_IN_ERR_CLEANUP, &fsi_state))
+		return;
+
+	fsi_master_break(master, 0);
+	udelay(200);
+	master->write(master, 0, 0, FSI_SLAVE_BASE + FSI_SMODE, &smode,
+			sizeof(smode));
+	smode = FSI_MRESB_RST_GEN | FSI_MRESB_RST_ERR;
+	master->write(master, 0, 0, FSI_HUB_CONTROL + FSI_MRESB0, &smode,
+			sizeof(smode));
+
+	if (addr > FSI_HUB_LINK_OFFSET) {
+		smode = FSI_BREAK;
+		master->write(master, 0, 0, 0x100004, &smode, sizeof(smode));
+		smode = FSI_SLAVE_SMODE_DFLT;
+		master->write(master, 0, 0, 0x100800, &smode, sizeof(smode));
+	}
+
+	clear_bit(FSI_IN_ERR_CLEANUP, &fsi_state);
 }
 
 static int fsi_master_scan(struct fsi_master *master)
-- 
1.8.2.2

[PATCH linux dev-4.7 v2 7/7] tools/testing/selftests: Add fsi command line tool

[Christopher Bostic]

Add command line tool to read/write CFAMs via the slave 'raw'
sysfs file.

Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
---
 tools/testing/selftests/fsi/fsitest.c | 163 ++++++++++++++++++++++++++++++++++
 1 file changed, 163 insertions(+)
 create mode 100644 tools/testing/selftests/fsi/fsitest.c

diff --git a/tools/testing/selftests/fsi/fsitest.c b/tools/testing/selftests/fsi/fsitest.c
new file mode 100644
index 0000000..7aa3e7f
--- /dev/null
+++ b/tools/testing/selftests/fsi/fsitest.c
@@ -0,0 +1,163 @@
+#include <stdio.h>
+#include <sys/types.h>
+#include <sys/stat.h>
+#include <sys/ioctl.h>
+#include <fcntl.h>
+#include <unistd.h>
+#include <err.h>
+#include <errno.h>
+#include <stdlib.h>
+#include <stdint.h>
+#include <getopt.h>
+#include <string.h>
+
+int testread(int fd, uint32_t addr, uint32_t *value)
+{
+	int rc;
+
+	lseek(fd, addr, SEEK_SET);
+	rc = read(fd, value, 4);
+	if (rc >= 0)
+		rc = 0;
+
+	return rc;
+}
+
+int testwrite(int fd, uint32_t addr, uint32_t value)
+{
+	int rc;
+
+	lseek(fd, addr, SEEK_SET);
+	rc = write(fd, &value, 4);
+	if (rc >= 0)
+		rc = 0;
+
+	return rc;
+}
+
+#define NUM_WRITES	6
+#define NUM_READS	9
+#define NUM_HUB_READS	9
+
+int main(int argc, char **argv)
+{
+	int rc = 0;
+	int i, fd, option;
+	int skip_write = 0;
+	int verbose = 0;
+	int hub = 0;
+	char path[64] = "/sys/bus/platform/devices/fsi-master/slave@00:00/raw";
+	uint32_t value;
+	const uint32_t write_addresses[NUM_WRITES] = {
+		0x800,
+		0x818,
+		0x35D0,
+		0x100800,
+		0x100818,
+		0x1035D0,
+	};
+	const uint32_t addresses[NUM_READS] = {
+		0x800,	/* smode */
+		0x818,	/* si1m */
+		0x81C,	/* si1s */
+		0x101C,	/* scom status */
+		0x1028,	/* scom chipid */
+		0x1808,	/* i2c mode */
+		0x181C,	/* i2c status */
+		0x3400,	/* hub mmode */
+		0x35D0,	/* hub status */
+	};
+	const uint32_t hub_addresses[NUM_HUB_READS] = {
+		0x100800,	/* smode */
+		0x100818,	/* si1m */
+		0x10081C,	/* si1s */
+		0x10101C,	/* scom status */
+		0x101028,	/* scom chipid */
+		0x101808,	/* i2c mode */
+		0x10181C,	/* i2c status */
+		0x103400,	/* hub mmode */
+		0x1035D0,	/* hub status */
+	};
+
+	while ((option = getopt(argc, argv, "vsb:x")) != -1) {
+		switch (option) {
+		case 'v':
+			verbose = 1;
+			break;
+		case 's':
+			skip_write = 1;
+			break;
+		case 'b':
+			strncpy(path, optarg, 64);
+			break;
+		case 'x':
+			hub = 1;
+			break;
+		default:
+			printf("unknown option\n");
+		}
+	}
+
+	fd = open(path, O_RDWR);
+	if (fd < 0) {
+		printf("FAILED - failed to open %s\n", path);
+		return -ENODEV;
+	}
+
+	for (i = 0; i < NUM_READS; ++i) {
+		rc = testread(fd, addresses[i], &value);
+		if (rc) {
+			printf("FAILED - failed to read %08x\n", addresses[i]);
+			goto exit;
+		}
+
+		if (verbose)
+			printf("read %08x: %08x\n", addresses[i], value);
+	}
+
+	if (!skip_write) {
+		for (i = 0; i < NUM_WRITES; ++i) {
+			value = 0;
+			rc = testread(fd, write_addresses[i], &value);
+			if (rc) {
+				printf("FAILED - failed to read %08x\n",
+				       write_addresses[i]);
+				goto exit;
+			}
+
+			rc = testwrite(fd, write_addresses[i], value);
+			if (rc) {
+				printf("FAILED - failed to write %08x\n",
+				       write_addresses[i]);
+				goto exit;
+			}
+
+			if (verbose)
+				printf("wrote %08x: %08x\n",
+				       write_addresses[i], value);
+		}
+	} else if (verbose)
+		printf("skipping write\n");
+
+	if (hub) {
+		for (i = 0; i < NUM_HUB_READS; ++i) {
+			rc = testread(fd, hub_addresses[i], &value);
+			if (rc) {
+				printf("FAILED - failed to read hub %08x\n",
+				       hub_addresses[i]);
+				goto exit;
+			}
+
+			if (verbose)
+				printf("read %08x: %08x\n", hub_addresses[i],
+				       value);
+		}
+	}
+
+	printf("SUCCESS\n");
+
+exit:
+	close(fd);
+
+	return rc;
+}
-- 
1.8.2.2

Re: [PATCH linux dev-4.7 v2 5/7] drivers/fsi: Add retry on bus error detect

[Joel Stanley]

On Wed, Feb 22, 2017 at 8:20 AM, Christopher Bostic
<cbostic@linux.vnet.ibm.com> wrote:
> After each gpio master read/write check for bus errors.  If
> errors detected then clean it up and retry the original operation
> again.  If second attempt succeeds then don't report original
> error to caller.
>
> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> ---
>  drivers/fsi/fsi-core.c        |  4 ++++
>  drivers/fsi/fsi-master-gpio.c | 24 ++++++++++++++++++++++--
>  drivers/fsi/fsi-master.h      |  3 +++
>  3 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
> index 360c02a..428675f 100644
> --- a/drivers/fsi/fsi-core.c
> +++ b/drivers/fsi/fsi-core.c
> @@ -639,6 +639,10 @@ static int fsi_master_break(struct fsi_master *master, int link)
>         return 0;
>  }
>
> +void fsi_master_handle_error(struct fsi_master *master, uint32_t addr)
> +{
> +}
> +
>  static int fsi_master_scan(struct fsi_master *master)
>  {
>         int link, slave_id, rc;
> diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
> index 8aec538..5b502eb 100644
> --- a/drivers/fsi/fsi-master-gpio.c
> +++ b/drivers/fsi/fsi-master-gpio.c
> @@ -381,7 +381,17 @@ static int fsi_master_gpio_read(struct fsi_master *_master, int link,
>                 return -ENODEV;
>
>         build_abs_ar_command(&cmd, FSI_GPIO_CMD_READ, slave, addr, size, NULL);
> -       return send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
> +       rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
> +       if (rc) {
> +               fsi_master_handle_error(&master->master, addr);
> +
> +               /* Try again */
> +               rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
> +               if (rc)
> +                       fsi_master_handle_error(&master->master, addr);
> +       }
> +
> +       return rc;
>  }
>
>  static int fsi_master_gpio_write(struct fsi_master *_master, int link,
> @@ -395,7 +405,17 @@ static int fsi_master_gpio_write(struct fsi_master *_master, int link,
>                 return -ENODEV;
>
>         build_abs_ar_command(&cmd, FSI_GPIO_CMD_WRITE, slave, addr, size, val);
> -       return send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
> +       rc = send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
> +       if (rc) {
> +               fsi_master_handle_error(&master->master, addr);
> +
> +               /* Try again */

It's obvious from the code that we're trying again. You could change
the comment to indicate why we are trying again.

> +               rc = send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
> +               if (rc)
> +                       fsi_master_handle_error(&master->master, addr);
> +       }

You're repeating yourself here. Perhaps a loop?

You have the same sequence in fsi_master_gpio_read. It would be nice
to not repeat the sequence.

> +
> +       return rc;
>  }
>
>  /*
> diff --git a/drivers/fsi/fsi-master.h b/drivers/fsi/fsi-master.h
> index 4a3176b..4ed416c 100644
> --- a/drivers/fsi/fsi-master.h
> +++ b/drivers/fsi/fsi-master.h
> @@ -19,6 +19,8 @@
>
>  #include <linux/device.h>
>
> +#include "fsi-master.h"

This looks wrong.

> +
>  /* Control Registers */
>  #define FSI_MMODE              0x0             /* R/W: mode */
>  #define FSI_MDLYR              0x4             /* R/W: delay */
> @@ -85,6 +87,7 @@ struct fsi_master {
>  extern int fsi_master_register(struct fsi_master *master);
>  extern void fsi_master_unregister(struct fsi_master *master);
>  extern int fsi_master_start_ipoll(struct fsi_master *master);
> +extern void fsi_master_handle_error(struct fsi_master *master, uint32_t addr);

Does this need to be added to the header?

>
>  /**
>   * crc4 helper: Given a starting crc4 state @c, calculate the crc4 vaue of @x,
> --
> 1.8.2.2
>

Re: [PATCH linux dev-4.7 v2 4/7] drivers/fsi: Move common read write ops into single function

[Joel Stanley]

On Wed, Feb 22, 2017 at 8:20 AM, Christopher Bostic
<cbostic@linux.vnet.ibm.com> wrote:
> Move common operations between gpio reads/writes into a new
> utility send_command.
>
> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
>
> ---
>
> v2: Separate the break out of common read/write elements into its
>     own separate patch.

Nice one.

Reviewed-by: Joel Stanley <joel@jms.id.au>

> ---
>  drivers/fsi/fsi-master-gpio.c | 36 ++++++++++++++++++------------------
>  1 file changed, 18 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
> index 3ed82ea..8aec538 100644
> --- a/drivers/fsi/fsi-master-gpio.c
> +++ b/drivers/fsi/fsi-master-gpio.c
> @@ -354,26 +354,34 @@ static void build_abs_ar_command(struct fsi_gpio_msg *cmd, uint64_t mode,
>         cmd->msg >>= (64 - cmd->bits);
>  }
>
> +static int send_command(struct fsi_master_gpio *master,
> +                       struct fsi_gpio_msg *cmd, uint8_t expected,
> +                       size_t size, void *val)
> +{
> +       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, expected, size, val);
> +       spin_unlock_irqrestore(&fsi_gpio_cmd_lock, flags);
> +
> +       return rc;
> +}
> +
>  static int fsi_master_gpio_read(struct fsi_master *_master, int link,
>                 uint8_t slave, uint32_t addr, void *val, size_t size)
>  {
>         struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
>         struct fsi_gpio_msg cmd;
>         int rc;
> -       unsigned long flags;
>
>         if (link != 0)
>                 return -ENODEV;
>
>         build_abs_ar_command(&cmd, FSI_GPIO_CMD_READ, slave, addr, size, NULL);
> -
> -       spin_lock_irqsave(&fsi_gpio_cmd_lock, flags);
> -       serial_out(master, &cmd);
> -       echo_delay(master);
> -       rc = poll_for_response(master, FSI_GPIO_RESP_ACKD, size, val);
> -       spin_unlock_irqrestore(&fsi_gpio_cmd_lock, flags);
> -
> -       return rc;
> +       return send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
>  }
>
>  static int fsi_master_gpio_write(struct fsi_master *_master, int link,
> @@ -382,20 +390,12 @@ static int fsi_master_gpio_write(struct fsi_master *_master, int link,
>         struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
>         struct fsi_gpio_msg cmd;
>         int rc;
> -       unsigned long flags;
>
>         if (link != 0)
>                 return -ENODEV;
>
>         build_abs_ar_command(&cmd, FSI_GPIO_CMD_WRITE, slave, addr, size, val);
> -
> -       spin_lock_irqsave(&fsi_gpio_cmd_lock, flags);
> -       serial_out(master, &cmd);
> -       echo_delay(master);
> -       rc = poll_for_response(master, FSI_GPIO_RESP_ACK, size, NULL);
> -       spin_unlock_irqrestore(&fsi_gpio_cmd_lock, flags);
> -
> -       return rc;
> +       return send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
>  }
>
>  /*
> --
> 1.8.2.2
>

Re: [PATCH linux dev-4.7 v2 1/7] drivers/fsi: Add hub master scan detect

[Jeremy Kerr]

Hi Chris,

> @@ -252,7 +271,31 @@ static int fsi_slave_scan(struct fsi_slave *slave)
>  		 * Unused address areas are marked by a zero type value; this
>  		 * skips the defined address areas
>  		 */
> -		if (type != 0 && slots != 0) {
> +		if (type != 0) {
> +
> +			if (type == FSI_ENGID_HUB_MASTER) {

I'd split this into a separate part of the above 'if' statement. Or,
convert it to a switch.

> +				hub = kzalloc(sizeof(*hub), GFP_KERNEL);
> +				if (!hub)
> +					return -ENOMEM;
> +
> +				hub->base = FSI_HUB_LINK_OFFSET;
> +				hub->control_regs = engine_addr;
> +				hub->slave = slave;
> +				rc = hub_master_init(hub);
> +
> +				continue;
> +			}
> +			if (type == FSI_ENGID_HUB_LINK) {
> +				if (!hub)
> +					continue;
> +
> +				hub->master.n_links++;
> +				if (hub->master.n_links ==
> +						(FSI_HUB_MASTER_MAX_LINKS * 2))
> +					rc = fsi_master_register(&hub->master);
> +
> +				continue;
> +			}

This logic doesn't make a lot of sense. You're counting the number of
link slots, but then registering the master when that count hits a
predefined number. I'd suggest either:

 - just setting n_links to the predefined number; or

 - keeping a separate variable for n_links, and then once we've finished
   scanning the configuration table (after the for loop)

    if (hub) {
	    hub->n_links = conf_link_count / 2;
	    fsi_master_register(&hub->master);
    }

I'd much prefer the second option, as that actually uses the link count
from the config table.

>  int fsi_master_register(struct fsi_master *master)
>  {
> -	if (!master || !master->dev)
> +	if (!master)
>  		return -EINVAL;

I'm worried if this is needed - we should be creating a device for all
masters.

Cheers,


Jeremy

Re: [PATCH linux dev-4.7 v2 3/7] drivers/fsi: Define hub master callbacks

[Jeremy Kerr]

Hi Chris,

> Define hub master read, write, link enable, and break ops.

Now that we have things more contained, you may want to add the complete
hub functionality in one patch.

However:

> @@ -571,6 +598,7 @@ static int fsi_slave_init(struct fsi_master *master,
>  	slave->id = slave_id;
>  	slave->dev.parent = master->dev;
>  	slave->dev.release = fsi_slave_release;
> +	slave->link = link;
>  
>  	dev_set_name(&slave->dev, "slave@%02x:%02x", link, slave_id);
>  	rc = device_register(&slave->dev);

This looks like a generally-useful fix, for more than just the hub code.

Otherwise, looks good.

Cheers,


Jeremy

Re: [PATCH linux dev-4.7 v2 0/7] drivers/fsi: Add hub master

[Jeremy Kerr]

Hi Chris,

> Adds the FSI hub master functionality to the OpenFSI device driver.
> Hub master is a FSI client device driver that registers with the
> core and is notified when devices it wants, hub master engine and
> links, are discovered.
> 
> v2: Restructured to move hub into the core instead of a separate
>     slave device driver.
> 
> Christopher Bostic (7):
>   drivers/fsi: Add hub master scan detect
>   drivers/fsi: Initialize hub master
>   drivers/fsi: Define hub master callbacks
>   drivers/fsi: Move common read write ops into single function
>   drivers/fsi: Add retry on bus error detect
>   drivers/fsi: Cleanup bus errors
>   tools/testing/selftests: Add fsi command line tool

This changeset seems to add both the hub master, and the generic error
handing. Could this be split into two series?

Cheers,


Jeremy

Re: [PATCH linux dev-4.7 v2 5/7] drivers/fsi: Add retry on bus error detect

[Jeremy Kerr]

Hi Chris,

> --- a/drivers/fsi/fsi-master-gpio.c
> +++ b/drivers/fsi/fsi-master-gpio.c
> @@ -381,7 +381,17 @@ static int fsi_master_gpio_read(struct fsi_master *_master, int link,
>  		return -ENODEV;
>  
>  	build_abs_ar_command(&cmd, FSI_GPIO_CMD_READ, slave, addr, size, NULL);
> -	return send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
> +	rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
> +	if (rc) {
> +		fsi_master_handle_error(&master->master, addr);
> +
> +		/* Try again */
> +		rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
> +		if (rc)
> +			fsi_master_handle_error(&master->master, addr);
> +	}
> +
> +	return rc;

Lets avoid the repeated code:

	const static int master_retries = 2;

	[...]

	for (retries = 0; retries < master_retries; retries++) {
		rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
		if (!rc)
			break;
		fsi_master_handle_error(&master->master, addr);
	}

	return rc;

[or even better, we may want to put this in a helper, for use by both
call sites]

Also, is there any condition where fsi_master_handle_error() knows that
the FSI bus is totally hosed, and there's no point retrying?

Cheers,


Jeremy

Re: [PATCH linux dev-4.7 v2 0/7] drivers/fsi: Add hub master

[Christopher Bostic]

On 2/21/17 7:08 PM, Jeremy Kerr wrote:
> Hi Chris,
>
>> Adds the FSI hub master functionality to the OpenFSI device driver.
>> Hub master is a FSI client device driver that registers with the
>> core and is notified when devices it wants, hub master engine and
>> links, are discovered.
>>
>> v2: Restructured to move hub into the core instead of a separate
>>      slave device driver.
>>
>> Christopher Bostic (7):
>>    drivers/fsi: Add hub master scan detect
>>    drivers/fsi: Initialize hub master
>>    drivers/fsi: Define hub master callbacks
>>    drivers/fsi: Move common read write ops into single function
>>    drivers/fsi: Add retry on bus error detect
>>    drivers/fsi: Cleanup bus errors
>>    tools/testing/selftests: Add fsi command line tool
> This changeset seems to add both the hub master, and the generic error
> handing. Could this be split into two series?
Hi Jeremy,

Sure, will split those up.

Thanks


> Cheers,
>
>
> Jeremy
>

Re: [PATCH linux dev-4.7 v2 1/7] drivers/fsi: Add hub master scan detect

[Christopher Bostic]

On 2/21/17 7:02 PM, Jeremy Kerr wrote:
> Hi Chris,
>
>> @@ -252,7 +271,31 @@ static int fsi_slave_scan(struct fsi_slave *slave)
>>   		 * Unused address areas are marked by a zero type value; this
>>   		 * skips the defined address areas
>>   		 */
>> -		if (type != 0 && slots != 0) {
>> +		if (type != 0) {
>> +
>> +			if (type == FSI_ENGID_HUB_MASTER) {
> I'd split this into a separate part of the above 'if' statement. Or,
> convert it to a switch.

Hi Jeremy,

Will change.

>> +				hub = kzalloc(sizeof(*hub), GFP_KERNEL);
>> +				if (!hub)
>> +					return -ENOMEM;
>> +
>> +				hub->base = FSI_HUB_LINK_OFFSET;
>> +				hub->control_regs = engine_addr;
>> +				hub->slave = slave;
>> +				rc = hub_master_init(hub);
>> +
>> +				continue;
>> +			}
>> +			if (type == FSI_ENGID_HUB_LINK) {
>> +				if (!hub)
>> +					continue;
>> +
>> +				hub->master.n_links++;
>> +				if (hub->master.n_links ==
>> +						(FSI_HUB_MASTER_MAX_LINKS * 2))
>> +					rc = fsi_master_register(&hub->master);
>> +
>> +				continue;
>> +			}
> This logic doesn't make a lot of sense. You're counting the number of
> link slots, but then registering the master when that count hits a
> predefined number. I'd suggest either:
>
>   - just setting n_links to the predefined number; or
>
>   - keeping a separate variable for n_links, and then once we've finished
>     scanning the configuration table (after the for loop)
>
>      if (hub) {
> 	    hub->n_links = conf_link_count / 2;
> 	    fsi_master_register(&hub->master);
>      }
>
> I'd much prefer the second option, as that actually uses the link count
> from the config table.

Will implement the second option

>>   int fsi_master_register(struct fsi_master *master)
>>   {
>> -	if (!master || !master->dev)
>> +	if (!master)
>>   		return -EINVAL;
> I'm worried if this is needed - we should be creating a device for all
> masters.

Will add a dev for struct fsi_master.

Thanks

> Cheers,
>
>
> Jeremy
>

Re: [PATCH linux dev-4.7 v2 0/7] drivers/fsi: Add hub master

[Andrew Geissler]

On Tue, Feb 21, 2017 at 3:50 PM, Christopher Bostic
<cbostic@linux.vnet.ibm.com> wrote:
> Adds the FSI hub master functionality to the OpenFSI device driver.
> Hub master is a FSI client device driver that registers with the
> core and is notified when devices it wants, hub master engine and
> links, are discovered.
>
> v2: Restructured to move hub into the core instead of a separate
>     slave device driver.
>
> Christopher Bostic (7):
>   drivers/fsi: Add hub master scan detect
>   drivers/fsi: Initialize hub master
>   drivers/fsi: Define hub master callbacks
>   drivers/fsi: Move common read write ops into single function
>   drivers/fsi: Add retry on bus error detect
>   drivers/fsi: Cleanup bus errors
>   tools/testing/selftests: Add fsi command line tool
>
>
> --
> 1.8.2.2
>

Do I need to respond that I tested to all commits in this series?
I'll just do this one for now.

I built all patches into an image and we verified all is looking good
on Witherspoon and Zaius.

Tested-by: Andrew Geissler <geissonator@gmail.com>

Re: [PATCH linux dev-4.7 v2 3/7] drivers/fsi: Define hub master callbacks

[Christopher Bostic]

On 2/21/17 7:06 PM, Jeremy Kerr wrote:
> Hi Chris,
>
>> Define hub master read, write, link enable, and break ops.
> Now that we have things more contained, you may want to add the complete
> hub functionality in one patch.
Hi Jeremy,

OK will add all in one patch.

> However:
>
>> @@ -571,6 +598,7 @@ static int fsi_slave_init(struct fsi_master *master,
>>   	slave->id = slave_id;
>>   	slave->dev.parent = master->dev;
>>   	slave->dev.release = fsi_slave_release;
>> +	slave->link = link;
>>   
>>   	dev_set_name(&slave->dev, "slave@%02x:%02x", link, slave_id);
>>   	rc = device_register(&slave->dev);
> This looks like a generally-useful fix, for more than just the hub code.

This was uncovered in hub link testing.  Will treat it as a separate
patch from hub  and error handling.

Thanks


> Otherwise, looks good.
>
> Cheers,
>
>
> Jeremy
>

Re: [PATCH linux dev-4.7 v2 5/7] drivers/fsi: Add retry on bus error detect

[Christopher Bostic]

On 2/21/17 7:00 PM, Joel Stanley wrote:
> On Wed, Feb 22, 2017 at 8:20 AM, Christopher Bostic
> <cbostic@linux.vnet.ibm.com> wrote:
>> After each gpio master read/write check for bus errors.  If
>> errors detected then clean it up and retry the original operation
>> again.  If second attempt succeeds then don't report original
>> error to caller.
>>
>> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
>> ---
>>   drivers/fsi/fsi-core.c        |  4 ++++
>>   drivers/fsi/fsi-master-gpio.c | 24 ++++++++++++++++++++++--
>>   drivers/fsi/fsi-master.h      |  3 +++
>>   3 files changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
>> index 360c02a..428675f 100644
>> --- a/drivers/fsi/fsi-core.c
>> +++ b/drivers/fsi/fsi-core.c
>> @@ -639,6 +639,10 @@ static int fsi_master_break(struct fsi_master *master, int link)
>>          return 0;
>>   }
>>
>> +void fsi_master_handle_error(struct fsi_master *master, uint32_t addr)
>> +{
>> +}
>> +
>>   static int fsi_master_scan(struct fsi_master *master)
>>   {
>>          int link, slave_id, rc;
>> diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
>> index 8aec538..5b502eb 100644
>> --- a/drivers/fsi/fsi-master-gpio.c
>> +++ b/drivers/fsi/fsi-master-gpio.c
>> @@ -381,7 +381,17 @@ static int fsi_master_gpio_read(struct fsi_master *_master, int link,
>>                  return -ENODEV;
>>
>>          build_abs_ar_command(&cmd, FSI_GPIO_CMD_READ, slave, addr, size, NULL);
>> -       return send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
>> +       rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
>> +       if (rc) {
>> +               fsi_master_handle_error(&master->master, addr);
>> +
>> +               /* Try again */
>> +               rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
>> +               if (rc)
>> +                       fsi_master_handle_error(&master->master, addr);
>> +       }
>> +
>> +       return rc;
>>   }
>>
>>   static int fsi_master_gpio_write(struct fsi_master *_master, int link,
>> @@ -395,7 +405,17 @@ static int fsi_master_gpio_write(struct fsi_master *_master, int link,
>>                  return -ENODEV;
>>
>>          build_abs_ar_command(&cmd, FSI_GPIO_CMD_WRITE, slave, addr, size, val);
>> -       return send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
>> +       rc = send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
>> +       if (rc) {
>> +               fsi_master_handle_error(&master->master, addr);
>> +
>> +               /* Try again */
> It's obvious from the code that we're trying again. You could change
> the comment to indicate why we are trying again.

Hi Joel

will update.
>> +               rc = send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
>> +               if (rc)
>> +                       fsi_master_handle_error(&master->master, addr);
>> +       }
> You're repeating yourself here. Perhaps a loop?
>
> You have the same sequence in fsi_master_gpio_read. It would be nice
> to not repeat the sequence.

OK, will add a loop here.

>> +
>> +       return rc;
>>   }
>>
>>   /*
>> diff --git a/drivers/fsi/fsi-master.h b/drivers/fsi/fsi-master.h
>> index 4a3176b..4ed416c 100644
>> --- a/drivers/fsi/fsi-master.h
>> +++ b/drivers/fsi/fsi-master.h
>> @@ -19,6 +19,8 @@
>>
>>   #include <linux/device.h>
>>
>> +#include "fsi-master.h"
> This looks wrong.
>
>> +
>>   /* Control Registers */
>>   #define FSI_MMODE              0x0             /* R/W: mode */
>>   #define FSI_MDLYR              0x4             /* R/W: delay */
>> @@ -85,6 +87,7 @@ struct fsi_master {
>>   extern int fsi_master_register(struct fsi_master *master);
>>   extern void fsi_master_unregister(struct fsi_master *master);
>>   extern int fsi_master_start_ipoll(struct fsi_master *master);
>> +extern void fsi_master_handle_error(struct fsi_master *master, uint32_t addr);
> Does this need to be added to the header?

Probably not, will remove.

Thanks,
Chris

>>   /**
>>    * crc4 helper: Given a starting crc4 state @c, calculate the crc4 vaue of @x,
>> --
>> 1.8.2.2
>>

Re: [PATCH linux dev-4.7 v2 5/7] drivers/fsi: Add retry on bus error detect

[Christopher Bostic]

On 2/21/17 7:13 PM, Jeremy Kerr wrote:
> Hi Chris,
>
>> --- a/drivers/fsi/fsi-master-gpio.c
>> +++ b/drivers/fsi/fsi-master-gpio.c
>> @@ -381,7 +381,17 @@ static int fsi_master_gpio_read(struct fsi_master *_master, int link,
>>   		return -ENODEV;
>>   
>>   	build_abs_ar_command(&cmd, FSI_GPIO_CMD_READ, slave, addr, size, NULL);
>> -	return send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
>> +	rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
>> +	if (rc) {
>> +		fsi_master_handle_error(&master->master, addr);
>> +
>> +		/* Try again */
>> +		rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
>> +		if (rc)
>> +			fsi_master_handle_error(&master->master, addr);
>> +	}
>> +
>> +	return rc;
> Lets avoid the repeated code:
>
> 	const static int master_retries = 2;
>
> 	[...]
>
> 	for (retries = 0; retries < master_retries; retries++) {
> 		rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
> 		if (!rc)
> 			break;
> 		fsi_master_handle_error(&master->master, addr);
> 	}
>
> 	return rc;
>
> [or even better, we may want to put this in a helper, for use by both
> call sites]
Hi Jeremy,

Will change to a loop.


> Also, is there any condition where fsi_master_handle_error() knows that
> the FSI bus is totally hosed, and there's no point retrying?

There's not anything defined to flag a bad bus -will look into it.

Thanks,
Chris

> Cheers,
>
>
> Jeremy
>

Re: [PATCH linux dev-4.7 v2 6/7] drivers/fsi: Cleanup bus errors

[Jeremy Kerr]

Hi Chris,

> @@ -255,6 +263,10 @@ int hub_master_break(struct fsi_master *master, int linkno)
>  	uint32_t break_offset = 0x4; /* hw workaround */
>  	uint32_t addr;
>  
> +	/* config table lists 2 entries per hub link */
> +	if (linkno >= (master->n_links / 2))
> +		return -ENODEV;
> +

master->n_links should represent the actual number of links, rather than
reflect what's in the config table.

[besides, don't we do the / 2 during the config table parse anyway?]

>  	command = FSI_BREAK;
>  	addr = (linkno * FSI_MASTER_HUB_LINK_SIZE) + hub->base;
>  	return fsi_slave_write(hub->slave, addr + break_offset, &command,
> @@ -641,6 +653,28 @@ static int fsi_master_break(struct fsi_master *master, int link)
>  
>  void fsi_master_handle_error(struct fsi_master *master, uint32_t addr)
>  {
> +	uint32_t smode = FSI_SLAVE_SMODE_DFLT;
> +
> +	/* Avoid nested error handling */
> +	if (test_and_set_bit(FSI_IN_ERR_CLEANUP, &fsi_state))
> +		return;

Do we have any other bits of state to represent? If not, just use a
plain atomic_t rather than a set of flags.

Cheers,


Jeremy

Re: [PATCH linux dev-4.7 v2 5/7] drivers/fsi: Add retry on bus error detect

[Christopher Bostic]

On 2/21/17 7:00 PM, Joel Stanley wrote:
> On Wed, Feb 22, 2017 at 8:20 AM, Christopher Bostic
> <cbostic@linux.vnet.ibm.com> wrote:
>> After each gpio master read/write check for bus errors.  If
>> errors detected then clean it up and retry the original operation
>> again.  If second attempt succeeds then don't report original
>> error to caller.
>>
>> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
>> ---
>>   drivers/fsi/fsi-core.c        |  4 ++++
>>   drivers/fsi/fsi-master-gpio.c | 24 ++++++++++++++++++++++--
>>   drivers/fsi/fsi-master.h      |  3 +++
>>   3 files changed, 29 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
>> index 360c02a..428675f 100644
>> --- a/drivers/fsi/fsi-core.c
>> +++ b/drivers/fsi/fsi-core.c
>> @@ -639,6 +639,10 @@ static int fsi_master_break(struct fsi_master *master, int link)
>>          return 0;
>>   }
>>
>> +void fsi_master_handle_error(struct fsi_master *master, uint32_t addr)
>> +{
>> +}
>> +
>>   static int fsi_master_scan(struct fsi_master *master)
>>   {
>>          int link, slave_id, rc;
>> diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
>> index 8aec538..5b502eb 100644
>> --- a/drivers/fsi/fsi-master-gpio.c
>> +++ b/drivers/fsi/fsi-master-gpio.c
>> @@ -381,7 +381,17 @@ static int fsi_master_gpio_read(struct fsi_master *_master, int link,
>>                  return -ENODEV;
>>
>>          build_abs_ar_command(&cmd, FSI_GPIO_CMD_READ, slave, addr, size, NULL);
>> -       return send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
>> +       rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
>> +       if (rc) {
>> +               fsi_master_handle_error(&master->master, addr);
>> +
>> +               /* Try again */
>> +               rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
>> +               if (rc)
>> +                       fsi_master_handle_error(&master->master, addr);
>> +       }
>> +
>> +       return rc;
>>   }
>>
>>   static int fsi_master_gpio_write(struct fsi_master *_master, int link,
>> @@ -395,7 +405,17 @@ static int fsi_master_gpio_write(struct fsi_master *_master, int link,
>>                  return -ENODEV;
>>
>>          build_abs_ar_command(&cmd, FSI_GPIO_CMD_WRITE, slave, addr, size, val);
>> -       return send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
>> +       rc = send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
>> +       if (rc) {
>> +               fsi_master_handle_error(&master->master, addr);
>> +
>> +               /* Try again */
> It's obvious from the code that we're trying again. You could change
> the comment to indicate why we are trying again.
>
>> +               rc = send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
>> +               if (rc)
>> +                       fsi_master_handle_error(&master->master, addr);
>> +       }
> You're repeating yourself here. Perhaps a loop?
>
> You have the same sequence in fsi_master_gpio_read. It would be nice
> to not repeat the sequence.
>
>> +
>> +       return rc;
>>   }
>>
>>   /*
>> diff --git a/drivers/fsi/fsi-master.h b/drivers/fsi/fsi-master.h
>> index 4a3176b..4ed416c 100644
>> --- a/drivers/fsi/fsi-master.h
>> +++ b/drivers/fsi/fsi-master.h
>> @@ -19,6 +19,8 @@
>>
>>   #include <linux/device.h>
>>
>> +#include "fsi-master.h"
> This looks wrong.
>
>> +
>>   /* Control Registers */
>>   #define FSI_MMODE              0x0             /* R/W: mode */
>>   #define FSI_MDLYR              0x4             /* R/W: delay */
>> @@ -85,6 +87,7 @@ struct fsi_master {
>>   extern int fsi_master_register(struct fsi_master *master);
>>   extern void fsi_master_unregister(struct fsi_master *master);
>>   extern int fsi_master_start_ipoll(struct fsi_master *master);
>> +extern void fsi_master_handle_error(struct fsi_master *master, uint32_t addr);
> Does this need to be added to the header?

Hi Joel,

Yes, since the gpio master is requiring a core function,  similar to 
fsi_master_register()/unregister()

Thanks,
Chris
>>   /**
>>    * crc4 helper: Given a starting crc4 state @c, calculate the crc4 vaue of @x,
>> --
>> 1.8.2.2
>>

Re: [PATCH linux dev-4.7 v2 6/7] drivers/fsi: Cleanup bus errors

[Christopher Bostic]

On 2/22/17 3:55 PM, Jeremy Kerr wrote:
> Hi Chris,
>
>> @@ -255,6 +263,10 @@ int hub_master_break(struct fsi_master *master, int linkno)
>>   	uint32_t break_offset = 0x4; /* hw workaround */
>>   	uint32_t addr;
>>   
>> +	/* config table lists 2 entries per hub link */
>> +	if (linkno >= (master->n_links / 2))
>> +		return -ENODEV;
>> +
> master->n_links should represent the actual number of links, rather than
> reflect what's in the config table.
>
> [besides, don't we do the / 2 during the config table parse anyway?]

Hi Jeremy,

Yes that's true, its set in scan so no need for the /2 here - will fix.


>>   	command = FSI_BREAK;
>>   	addr = (linkno * FSI_MASTER_HUB_LINK_SIZE) + hub->base;
>>   	return fsi_slave_write(hub->slave, addr + break_offset, &command,
>> @@ -641,6 +653,28 @@ static int fsi_master_break(struct fsi_master *master, int link)
>>   
>>   void fsi_master_handle_error(struct fsi_master *master, uint32_t addr)
>>   {
>> +	uint32_t smode = FSI_SLAVE_SMODE_DFLT;
>> +
>> +	/* Avoid nested error handling */
>> +	if (test_and_set_bit(FSI_IN_ERR_CLEANUP, &fsi_state))
>> +		return;
> Do we have any other bits of state to represent? If not, just use a
> plain atomic_t rather than a set of flags.
No others at this point,  will change to an atomic_t

Thanks,
Chris

> Cheers,
>
>
> Jeremy
>