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

[PATCH linux dev-4.7 0/5] 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.

Christopher Bostic (5):
  drivers/fsi: Add MFSI a.k.a. hub master device driver
  drivers/fsi: Initialize hub master engine
  drivers/fsi: Define hub master read write
  drivers/fsi: Add FSI bus error cleanup
  drivers/fsi: Add command line FSI test

 arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 1 +
 1 file changed, 1 insertion(+)

-- 
1.8.2.2

[PATCH linux dev-4.7 1/5] drivers/fsi: Add MFSI a.k.a. hub master device driver

[Christopher Bostic]

Add the basic hub master device driver.  Registers with the
FSI core to be notified of its engine device and its link devices.

Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
---
 drivers/fsi/Kconfig          |  9 ++++++
 drivers/fsi/Makefile         |  1 +
 drivers/fsi/fsi-master-hub.c | 65 ++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 75 insertions(+)
 create mode 100644 drivers/fsi/fsi-master-hub.c

diff --git a/drivers/fsi/Kconfig b/drivers/fsi/Kconfig
index c78b9b6e..bb7c5d5 100644
--- a/drivers/fsi/Kconfig
+++ b/drivers/fsi/Kconfig
@@ -18,6 +18,15 @@ config FSI_MASTER_GPIO
 	---help---
 	This option enables a FSI master driver using GPIO lines.
 
+config FSI_MASTER_HUB
+	tristate "Hub FSI master"
+	depends on FSI
+	---help---
+	This option enables a hub FSI master. Hub masters extend the
+	address space of the primary master and allow chaining of
+	multiple devices. Chaining FSI links from one hub to another
+	allows for very high device fan out.
+
 config FSI_SCOM
 	tristate "SCOM FSI client device driver"
 	depends on FSI
diff --git a/drivers/fsi/Makefile b/drivers/fsi/Makefile
index 3a106ba..37b5f5e 100644
--- a/drivers/fsi/Makefile
+++ b/drivers/fsi/Makefile
@@ -3,3 +3,4 @@ obj-$(CONFIG_FSI) += fsi-core.o
 obj-$(CONFIG_FSI_MASTER_GPIO) += fsi-master-gpio.o
 obj-$(CONFIG_FSI_SCOM) += fsi-scom.o
 obj-$(CONFIG_FSI_I2C) += i2c/
+obj-$(CONFIG_FSI_MASTER_HUB) += fsi-master-hub.o
diff --git a/drivers/fsi/fsi-master-hub.c b/drivers/fsi/fsi-master-hub.c
new file mode 100644
index 0000000..19b7c77
--- /dev/null
+++ b/drivers/fsi/fsi-master-hub.c
@@ -0,0 +1,65 @@
+/*
+ * Hub Master FSI Client device driver
+ *
+ * Copyright (C) IBM Corporation 2017
+ *
+ * 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/fs.h>
+#include <linux/uaccess.h>
+
+#include "fsi-master.h"
+
+#define FSI_ENGID_HUB_MASTER	0x1C
+#define FSI_ENGID_HUB_LINK	0x1D
+
+static int hub_master_probe(struct device *dev)
+{
+	return 0;
+}
+
+static struct fsi_device_id hub_master_ids[] = {
+	{
+		.engine_type = FSI_ENGID_HUB_MASTER,
+		.version = FSI_VERSION_ANY,
+	},
+	{
+		.engine_type = FSI_ENGID_HUB_LINK,
+		.version = FSI_VERSION_ANY,
+	},
+	{ 0 }
+};
+
+static struct fsi_driver hub_master_drv = {
+	.id_table = hub_master_ids,
+	.drv = {
+		.name = "hub_master",
+		.bus = &fsi_bus_type,
+		.probe = hub_master_probe,
+	}
+};
+
+static int hub_master_init(void)
+{
+	return fsi_driver_register(&hub_master_drv);
+}
+
+static void hub_master_exit(void)
+{
+	fsi_driver_unregister(&hub_master_drv);
+}
+
+module_init(hub_master_init);
+module_exit(hub_master_exit);
+MODULE_LICENSE("GPL");
-- 
1.8.2.2

[PATCH linux dev-4.7 2/5] drivers/fsi: Initialize hub master engine

[Christopher Bostic]

Define hub master probe and add definitions for general
master registers and bitfields.

Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
---
 drivers/fsi/fsi-core.c        |  44 ++++++++++--
 drivers/fsi/fsi-master-gpio.c |   7 ++
 drivers/fsi/fsi-master-hub.c  | 154 ++++++++++++++++++++++++++++++++++++++++++
 drivers/fsi/fsi-master.h      |  64 ++++++++++++++++++
 include/linux/fsi.h           |   6 ++
 5 files changed, 269 insertions(+), 6 deletions(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 1e9c5a2..843389e 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -48,13 +48,19 @@ static unsigned int fsi_ipoll_period_ms = 100;
 
 static atomic_t master_idx = ATOMIC_INIT(-1);
 
+static uint32_t set_smode_defaults(struct fsi_master *master);
+static int fsi_slave_set_smode(struct fsi_master *master, int link, int id);
+static int fsi_master_break(struct fsi_master *master, int link);
+
 struct fsi_slave {
 	struct list_head	list_link;	/* Master's list of slaves */
 	struct list_head	my_engines;
 	struct device		dev;
-	struct fsi_master	*master;
+	struct fsi_master	*master;	/* Upstream master */
+	struct fsi_master	*next_master;	/* Downstream master */
 	int			link;
 	uint8_t			id;
+	uint32_t		base;		/* Base address */
 };
 
 #define to_fsi_slave(d) container_of(d, struct fsi_slave, dev)
@@ -112,6 +118,11 @@ int fsi_device_write(struct fsi_device *dev, uint32_t addr, const void *val,
 }
 EXPORT_SYMBOL_GPL(fsi_device_write);
 
+struct fsi_master *fsi_get_master(struct fsi_device *fsi_dev)
+{
+	return fsi_dev->slave->master;
+}
+
 int fsi_device_peek(struct fsi_device *dev, void *val)
 {
 	uint32_t addr = FSI_PEEK_BASE + ((dev->unit - 2) * sizeof(uint32_t));
@@ -192,14 +203,14 @@ static int fsi_slave_read(struct fsi_slave *slave, uint32_t addr,
 			void *val, size_t size)
 {
 	return slave->master->read(slave->master, slave->link,
-			slave->id, addr, val, size);
+			slave->id, slave->base + addr, val, size);
 }
 
 static int fsi_slave_write(struct fsi_slave *slave, uint32_t addr,
 			const void *val, size_t size)
 {
 	return slave->master->write(slave->master, slave->link,
-			slave->id, addr, val, size);
+			slave->id, slave->base + addr, val, size);
 }
 
 static int fsi_slave_scan(struct fsi_slave *slave)
@@ -251,7 +262,7 @@ 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) {
 
 			/* create device */
 			dev = fsi_create_device(slave);
@@ -413,6 +424,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->base = link * master->link_size;
 
 	dev_set_name(&slave->dev, "slave@%02x:%02x", link, slave_id);
 	rc = device_register(&slave->dev);
@@ -519,6 +531,24 @@ static void fsi_master_unscan(struct fsi_master *master)
 	master->slave_list = false;
 }
 
+struct fsi_master *fsi_get_link_master(struct fsi_device *fsi_dev)
+{
+	if (fsi_dev && fsi_dev->slave)
+		return fsi_dev->slave->next_master;
+	return NULL;
+}
+EXPORT_SYMBOL_GPL(fsi_get_link_master);
+
+int fsi_set_next_master(struct fsi_device *fsi_dev, struct fsi_master *master)
+{
+	if (fsi_dev && fsi_dev->slave) {
+		fsi_dev->slave->next_master = master;
+		return 0;
+	}
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(fsi_set_next_master);
+
 static void fsi_master_irq(struct fsi_master *master, int link, uint32_t si1s)
 {
 	struct fsi_slave *slave;
@@ -595,7 +625,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 = atomic_inc_return(&master_idx);
@@ -657,8 +687,10 @@ static int fsi_bus_match(struct device *dev, struct device_driver *drv)
 		if (id->engine_type != fsi_dev->engine_type)
 			continue;
 		if (id->version == FSI_VERSION_ANY ||
-				id->version == fsi_dev->version)
+				id->version == fsi_dev->version) {
+			fsi_dev->id = id;
 			return 1;
+		}
 	}
 
 	return 0;
diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
index 91bdbf2..92af53d 100644
--- a/drivers/fsi/fsi-master-gpio.c
+++ b/drivers/fsi/fsi-master-gpio.c
@@ -13,6 +13,8 @@
 
 #include "fsi-master.h"
 
+#define	FSI_MASTER_GPIO_TYPE	0x1
+
 #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 */
@@ -63,6 +65,8 @@
 #define	FSI_GPIO_MSG_RESPID_SIZE	2
 #define	FSI_GPIO_PRIME_SLAVE_CLOCKS	100
 
+#define	FSI_MASTER_GPIO_LINK_SIZE	0x00800000
+
 static DEFINE_SPINLOCK(fsi_gpio_cmd_lock);	/* lock around fsi commands */
 
 struct fsi_master_gpio {
@@ -512,11 +516,14 @@ static int fsi_master_gpio_probe(struct platform_device *pdev)
 	else
 		master->gpio_mux = gpio;
 
+	master->master.type = FSI_MASTER_GPIO_TYPE;
 	master->master.n_links = 1;
+	master->master.link_size = FSI_MASTER_GPIO_LINK_SIZE;
 	master->master.read = fsi_master_gpio_read;
 	master->master.write = fsi_master_gpio_write;
 	master->master.send_break = fsi_master_gpio_break;
 	master->master.link_enable = fsi_master_gpio_link_enable;
+	master->master.dev = &pdev->dev;
 	platform_set_drvdata(pdev, master);
 
 	return device_create_file(&pdev->dev, &dev_attr_scan);
diff --git a/drivers/fsi/fsi-master-hub.c b/drivers/fsi/fsi-master-hub.c
index 19b7c77..e8c0726 100644
--- a/drivers/fsi/fsi-master-hub.c
+++ b/drivers/fsi/fsi-master-hub.c
@@ -18,14 +18,168 @@
 #include <linux/cdev.h>
 #include <linux/fs.h>
 #include <linux/uaccess.h>
+#include <linux/slab.h>
+#include <linux/delay.h>
 
 #include "fsi-master.h"
 
+#define FSI_MASTER_HUB_TYPE             0x02
+
+#define FSI_MASTER_HUB_MAX_LINKS        8
+#define FSI_MASTER_HUB_LINK_SIZE        0x00080000
+
 #define FSI_ENGID_HUB_MASTER	0x1C
 #define FSI_ENGID_HUB_LINK	0x1D
 
+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 link)
+{
+	return 0;
+}
+
 static int hub_master_probe(struct device *dev)
 {
+	struct fsi_device *fsi_dev = to_fsi_dev(dev);
+	struct fsi_master *master;
+	int rc = 0;
+	uint32_t mver;
+
+	master = fsi_get_master(fsi_dev);
+
+	/* Two Hub masters in communication chain not allowed */
+	if (master->type == FSI_MASTER_HUB_TYPE)
+		return 0;
+
+	if (master && (master->n_links >= FSI_MASTER_HUB_MAX_LINKS * 2))
+		return 0;
+
+	if (fsi_dev->id->engine_type == FSI_ENGID_HUB_MASTER) {
+		master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL);
+		if (!master)
+			return -ENOMEM;
+
+		fsi_dev->master = master;
+
+		/* TODO: can use master->dev to get at the engine possibly */
+		master->engine = fsi_dev;
+		master->read = hub_master_read;
+		master->write = hub_master_write;
+		master->send_break = hub_master_break;
+		master->link_enable = hub_master_link_enable;
+		master->link_size = FSI_MASTER_HUB_LINK_SIZE;
+		rc = fsi_set_next_master(fsi_dev, master);
+
+		/* Initialize the MFSI (hub master) engine */
+		rc = fsi_device_read(master->engine, 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_device_write(master->engine, FSI_MRESP0, &mver,
+					sizeof(mver));
+		if (rc)
+			return rc;
+
+		mver = FSI_MECTRL_EOAE | FSI_MECTRL_P8_AUTO_TERM;
+		rc = fsi_device_write(master->engine, 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_device_write(master->engine, FSI_MMODE, &mver,
+					sizeof(mver));
+		if (rc)
+			return rc;
+
+		mver = 0xffff0000;
+		rc = fsi_device_write(master->engine, FSI_MDLYR, &mver,
+					sizeof(mver));
+		if (rc)
+			return rc;
+
+		mver = ~0;
+		rc = fsi_device_write(master->engine, FSI_MSENP0, &mver,
+					sizeof(mver));
+		if (rc)
+			return rc;
+
+		/* Leave enabled long enough for master logic to set up */
+		udelay(1000);
+
+		rc = fsi_device_write(master->engine, FSI_MCENP0, &mver,
+					sizeof(mver));
+		if (rc)
+			return rc;
+
+		rc = fsi_device_read(master->engine, FSI_MAEB, &mver,
+					sizeof(mver));
+		if (rc)
+			return rc;
+
+		mver = FSI_MRESP_RST_ALL_MASTER | FSI_MRESP_RST_ALL_LINK;
+		rc = fsi_device_write(master->engine, FSI_MRESP0, &mver,
+					sizeof(mver));
+		if (rc)
+			return rc;
+
+		rc = fsi_device_read(master->engine, FSI_MLEVP0, &mver,
+					sizeof(mver));
+		if (rc)
+			return rc;
+
+		/* Reset the master bridge */
+		mver = FSI_MRESB_RST_GEN;
+		rc = fsi_device_write(master->engine, FSI_MRESB0, &mver,
+					sizeof(mver));
+		if (rc)
+			return rc;
+
+		mver = FSI_MRESB_RST_ERR;
+		rc = fsi_device_write(master->engine, FSI_MRESB0, &mver,
+					sizeof(mver));
+		if (rc)
+			return rc;
+	} else {
+		/*
+		 * A hub link device.
+		 */
+		master = fsi_get_link_master(fsi_dev);
+		if (!master)
+			return -ENODEV;
+
+		if (master->n_links == 0) {
+			/*
+			 * CFAM Config space doesn't list correct size
+			 * for hub links, fix this.
+			 */
+			fsi_dev->size = fsi_dev->addr *
+					FSI_MASTER_HUB_MAX_LINKS;
+			master->link = fsi_dev;
+		}
+		master->n_links++;
+	}
+
 	return 0;
 }
 
diff --git a/drivers/fsi/fsi-master.h b/drivers/fsi/fsi-master.h
index 3737404..367b8d9 100644
--- a/drivers/fsi/fsi-master.h
+++ b/drivers/fsi/fsi-master.h
@@ -17,8 +17,57 @@
 #ifndef DRIVERS_FSI_MASTER_H
 #define DRIVERS_FSI_MASTER_H
 
+#include <linux/fsi.h>
 #include <linux/device.h>
 
+#define FSI_BREAK			0xc0de0000
+
+/* 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;
@@ -26,6 +75,10 @@ struct fsi_master {
 	int		idx;
 	int		n_links;
 	uint32_t	ipoll;
+	struct fsi_device *engine;
+	struct fsi_device *link;
+	uint8_t		type;
+	uint32_t	link_size;
 	int		(*read)(struct fsi_master *, int link,
 				uint8_t slave, uint32_t addr,
 				void *val, size_t size);
@@ -61,4 +114,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 */
diff --git a/include/linux/fsi.h b/include/linux/fsi.h
index d22d0c5..9bf0b06 100644
--- a/include/linux/fsi.h
+++ b/include/linux/fsi.h
@@ -20,6 +20,8 @@
 struct fsi_device {
 	struct list_head	link;	/* for slave's list */
 	struct device		dev;
+	const struct fsi_device_id *id;
+	struct fsi_master	*master;
 	u8			engine_type;
 	u8			version;
 	u8			unit;
@@ -71,6 +73,10 @@ extern void fsi_driver_unregister(struct fsi_driver *);
 
 extern struct bus_type fsi_bus_type;
 
+extern struct fsi_master *fsi_get_link_master(struct fsi_device *dev);
+extern int fsi_set_next_master(struct fsi_device *dev,
+				struct fsi_master *master);
+extern struct fsi_master *fsi_get_master(struct fsi_device *dev);
 extern int fsi_enable_irq(struct fsi_device *dev);
 extern void fsi_disable_irq(struct fsi_device *dev);
 
-- 
1.8.2.2

[PATCH linux dev-4.7 3/5] drivers/fsi: Define hub master read write

[Christopher Bostic]

Define the master read, write, link enable, and break methods.
Register the hub master with the core.

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

diff --git a/drivers/fsi/fsi-master-hub.c b/drivers/fsi/fsi-master-hub.c
index e8c0726..ea6db17 100644
--- a/drivers/fsi/fsi-master-hub.c
+++ b/drivers/fsi/fsi-master-hub.c
@@ -34,23 +34,38 @@
 int hub_master_read(struct fsi_master *master, int linkno, uint8_t slave,
 			uint32_t addr, void *val, size_t size)
 {
-	return 0;
+	addr += (linkno * FSI_MASTER_HUB_LINK_SIZE);
+	return fsi_device_read(master->link, 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;
+	addr += (linkno * FSI_MASTER_HUB_LINK_SIZE);
+	return fsi_device_write(master->link, addr, val, size);
 }
 
 int hub_master_break(struct fsi_master *master, int linkno)
 {
-	return 0;
+	uint32_t command;
+	uint32_t break_offset = 0x4; /* hw workaround */
+
+	command = FSI_BREAK;
+	return fsi_device_write(master->link, break_offset, &command,
+				sizeof(command));
 }
 
 int hub_master_link_enable(struct fsi_master *master, int link)
 {
-	return 0;
+	uint32_t menp = L_MSB_MASK(link);
+	int rc;
+
+	rc = fsi_device_write(master->engine, FSI_MSENP0, &menp,
+				sizeof(menp));
+	/* Wait for hw to finish enable */
+	mdelay(10);
+
+	return rc;
 }
 
 static int hub_master_probe(struct device *dev)
@@ -178,6 +193,13 @@ static int hub_master_probe(struct device *dev)
 			master->link = fsi_dev;
 		}
 		master->n_links++;
+
+		/* Each hub links is listed twice in CFAM config space */
+		if (master->n_links == (FSI_MASTER_HUB_MAX_LINKS * 2)) {
+			/* Only need hub link 1 for now */
+			master->n_links = 2;
+			return fsi_master_register(master);
+		}
 	}
 
 	return 0;
-- 
1.8.2.2

[PATCH linux dev-4.7 4/5] drivers/fsi: Add FSI bus error cleanup

[Christopher Bostic]

On detect of bus fails attempt to clean them up and try the
command again.

Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
---
 drivers/fsi/fsi-master-gpio.c | 100 +++++++++++++++++++++++++++++++++++-------
 1 file changed, 85 insertions(+), 15 deletions(-)

diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
index 92af53d..bb3983c 100644
--- a/drivers/fsi/fsi-master-gpio.c
+++ b/drivers/fsi/fsi-master-gpio.c
@@ -67,8 +67,16 @@
 
 #define	FSI_MASTER_GPIO_LINK_SIZE	0x00800000
 
+static int fsi_master_gpio_read(struct fsi_master *_master, int link,
+		uint8_t slave, uint32_t addr, void *val, size_t size);
+static int fsi_master_gpio_write(struct fsi_master *_master, int link,
+		uint8_t slave, uint32_t addr, const void *val, size_t size);
+static int fsi_master_gpio_break(struct fsi_master *_master, int link);
+
 static DEFINE_SPINLOCK(fsi_gpio_cmd_lock);	/* lock around fsi commands */
 
+uint8_t in_err_cleanup;
+
 struct fsi_master_gpio {
 	struct fsi_master	master;
 	struct gpio_desc	*gpio_clk;
@@ -358,24 +366,85 @@ static void build_abs_ar_command(struct fsi_gpio_msg *cmd, uint64_t mode,
 	cmd->msg >>= (64 - cmd->bits);
 }
 
+/*
+ * Clean up the FSI bus path on detection of error.
+ * todo: get rid of magic numbers
+ */
+static void fsi_master_gpio_handle_error(struct fsi_master_gpio *master,
+					uint32_t addr)
+{
+	uint32_t smode = 0xa0ff0100;
+
+	/* Avoid nested error handling */
+	if (in_err_cleanup)
+		return;
+
+	in_err_cleanup = 1;
+	fsi_master_gpio_break(&master->master, 0);
+	udelay(200);
+
+	/* Set slave ID in smode */
+	fsi_master_gpio_write(&master->master, 0, 0, 0x800, &smode,
+				sizeof(smode));
+
+	/* Reset the MFSI bridge */
+	smode = 0xc0000000;
+	fsi_master_gpio_write(&master->master, 0, 0, 0x35D0, &smode,
+				sizeof(smode));
+
+	if (addr >= 0x80000) {
+
+		/*
+		 * Break to hub link 1
+		 * todo: make this handle more arbitrary link topologies
+		 */
+		smode = 0xc0de0000;
+		fsi_master_gpio_write(&master->master, 0, 0, 0x100004, &smode,
+					sizeof(smode));
+
+		smode = 0xa0ff0100;
+		fsi_master_gpio_write(&master->master, 0, 0, 0x100800, &smode,
+					sizeof(smode));
+	}
+	in_err_cleanup = 0;
+}
+
+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);
+	rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
+	if (rc) {
+		fsi_master_gpio_handle_error(master, addr);
+
+		/* Try again */
+		rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
+		if (rc)
+			fsi_master_gpio_handle_error(master, addr);
+	}
 
 	return rc;
 }
@@ -386,18 +455,20 @@ 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);
+	rc = send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
+	if (rc) {
+		fsi_master_gpio_handle_error(master, addr);
+
+		/* Try again */
+		rc = send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
+		if (rc)
+			fsi_master_gpio_handle_error(master, addr);
+	}
 
 	return rc;
 }
@@ -480,7 +551,6 @@ static int fsi_master_gpio_probe(struct platform_device *pdev)
 	master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL);
 	if (!master)
 		return -ENOMEM;
-	master->master.dev = &pdev->dev;
 
 	gpio = devm_gpiod_get(&pdev->dev, "clock", 0);
 	if (IS_ERR(gpio)) {
-- 
1.8.2.2

[PATCH linux dev-4.7 5/5] drivers/fsi: Add command line FSI test

[Christopher Bostic]

Perform FSI reads and writes via the raw sysfs file.  Program
reads and writes to various locations on the main link and
off of hub link 1.

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

diff --git a/drivers/fsi/fsitest.c b/drivers/fsi/fsitest.c
new file mode 100644
index 0000000..7aa3e7f
--- /dev/null
+++ b/drivers/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 1/5] drivers/fsi: Add MFSI a.k.a. hub master device driver

[Eddie James]

Acked-by: Eddie James <eajames@linux.vnet.ibm.com>


On 02/16/2017 04:04 PM, Christopher Bostic wrote:
> Add the basic hub master device driver.  Registers with the
> FSI core to be notified of its engine device and its link devices.
>
> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> ---
>   drivers/fsi/Kconfig          |  9 ++++++
>   drivers/fsi/Makefile         |  1 +
>   drivers/fsi/fsi-master-hub.c | 65 ++++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 75 insertions(+)
>   create mode 100644 drivers/fsi/fsi-master-hub.c
>
> diff --git a/drivers/fsi/Kconfig b/drivers/fsi/Kconfig
> index c78b9b6e..bb7c5d5 100644
> --- a/drivers/fsi/Kconfig
> +++ b/drivers/fsi/Kconfig
> @@ -18,6 +18,15 @@ config FSI_MASTER_GPIO
>   	---help---
>   	This option enables a FSI master driver using GPIO lines.
>
> +config FSI_MASTER_HUB
> +	tristate "Hub FSI master"
> +	depends on FSI
> +	---help---
> +	This option enables a hub FSI master. Hub masters extend the
> +	address space of the primary master and allow chaining of
> +	multiple devices. Chaining FSI links from one hub to another
> +	allows for very high device fan out.
> +
>   config FSI_SCOM
>   	tristate "SCOM FSI client device driver"
>   	depends on FSI
> diff --git a/drivers/fsi/Makefile b/drivers/fsi/Makefile
> index 3a106ba..37b5f5e 100644
> --- a/drivers/fsi/Makefile
> +++ b/drivers/fsi/Makefile
> @@ -3,3 +3,4 @@ obj-$(CONFIG_FSI) += fsi-core.o
>   obj-$(CONFIG_FSI_MASTER_GPIO) += fsi-master-gpio.o
>   obj-$(CONFIG_FSI_SCOM) += fsi-scom.o
>   obj-$(CONFIG_FSI_I2C) += i2c/
> +obj-$(CONFIG_FSI_MASTER_HUB) += fsi-master-hub.o
> diff --git a/drivers/fsi/fsi-master-hub.c b/drivers/fsi/fsi-master-hub.c
> new file mode 100644
> index 0000000..19b7c77
> --- /dev/null
> +++ b/drivers/fsi/fsi-master-hub.c
> @@ -0,0 +1,65 @@
> +/*
> + * Hub Master FSI Client device driver
> + *
> + * Copyright (C) IBM Corporation 2017
> + *
> + * 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/fs.h>
> +#include <linux/uaccess.h>
> +
> +#include "fsi-master.h"
> +
> +#define FSI_ENGID_HUB_MASTER	0x1C
> +#define FSI_ENGID_HUB_LINK	0x1D
> +
> +static int hub_master_probe(struct device *dev)
> +{
> +	return 0;
> +}
> +
> +static struct fsi_device_id hub_master_ids[] = {
> +	{
> +		.engine_type = FSI_ENGID_HUB_MASTER,
> +		.version = FSI_VERSION_ANY,
> +	},
> +	{
> +		.engine_type = FSI_ENGID_HUB_LINK,
> +		.version = FSI_VERSION_ANY,
> +	},
> +	{ 0 }
> +};
> +
> +static struct fsi_driver hub_master_drv = {
> +	.id_table = hub_master_ids,
> +	.drv = {
> +		.name = "hub_master",
> +		.bus = &fsi_bus_type,
> +		.probe = hub_master_probe,
> +	}
> +};
> +
> +static int hub_master_init(void)
> +{
> +	return fsi_driver_register(&hub_master_drv);
> +}
> +
> +static void hub_master_exit(void)
> +{
> +	fsi_driver_unregister(&hub_master_drv);
> +}
> +
> +module_init(hub_master_init);
> +module_exit(hub_master_exit);
> +MODULE_LICENSE("GPL");

Re: [PATCH linux dev-4.7 2/5] drivers/fsi: Initialize hub master engine

[Eddie James]

Acked-by: Eddie James <eajames@linux.vnet.ibm.com>


On 02/16/2017 04:04 PM, Christopher Bostic wrote:
> Define hub master probe and add definitions for general
> master registers and bitfields.
>
> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> ---
>   drivers/fsi/fsi-core.c        |  44 ++++++++++--
>   drivers/fsi/fsi-master-gpio.c |   7 ++
>   drivers/fsi/fsi-master-hub.c  | 154 ++++++++++++++++++++++++++++++++++++++++++
>   drivers/fsi/fsi-master.h      |  64 ++++++++++++++++++
>   include/linux/fsi.h           |   6 ++
>   5 files changed, 269 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
> index 1e9c5a2..843389e 100644
> --- a/drivers/fsi/fsi-core.c
> +++ b/drivers/fsi/fsi-core.c
> @@ -48,13 +48,19 @@ static unsigned int fsi_ipoll_period_ms = 100;
>
>   static atomic_t master_idx = ATOMIC_INIT(-1);
>
> +static uint32_t set_smode_defaults(struct fsi_master *master);
> +static int fsi_slave_set_smode(struct fsi_master *master, int link, int id);
> +static int fsi_master_break(struct fsi_master *master, int link);
> +
>   struct fsi_slave {
>   	struct list_head	list_link;	/* Master's list of slaves */
>   	struct list_head	my_engines;
>   	struct device		dev;
> -	struct fsi_master	*master;
> +	struct fsi_master	*master;	/* Upstream master */
> +	struct fsi_master	*next_master;	/* Downstream master */
>   	int			link;
>   	uint8_t			id;
> +	uint32_t		base;		/* Base address */
>   };
>
>   #define to_fsi_slave(d) container_of(d, struct fsi_slave, dev)
> @@ -112,6 +118,11 @@ int fsi_device_write(struct fsi_device *dev, uint32_t addr, const void *val,
>   }
>   EXPORT_SYMBOL_GPL(fsi_device_write);
>
> +struct fsi_master *fsi_get_master(struct fsi_device *fsi_dev)
> +{
> +	return fsi_dev->slave->master;
> +}
> +
>   int fsi_device_peek(struct fsi_device *dev, void *val)
>   {
>   	uint32_t addr = FSI_PEEK_BASE + ((dev->unit - 2) * sizeof(uint32_t));
> @@ -192,14 +203,14 @@ static int fsi_slave_read(struct fsi_slave *slave, uint32_t addr,
>   			void *val, size_t size)
>   {
>   	return slave->master->read(slave->master, slave->link,
> -			slave->id, addr, val, size);
> +			slave->id, slave->base + addr, val, size);
>   }
>
>   static int fsi_slave_write(struct fsi_slave *slave, uint32_t addr,
>   			const void *val, size_t size)
>   {
>   	return slave->master->write(slave->master, slave->link,
> -			slave->id, addr, val, size);
> +			slave->id, slave->base + addr, val, size);
>   }
>
>   static int fsi_slave_scan(struct fsi_slave *slave)
> @@ -251,7 +262,7 @@ 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) {
>
>   			/* create device */
>   			dev = fsi_create_device(slave);
> @@ -413,6 +424,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->base = link * master->link_size;
>
>   	dev_set_name(&slave->dev, "slave@%02x:%02x", link, slave_id);
>   	rc = device_register(&slave->dev);
> @@ -519,6 +531,24 @@ static void fsi_master_unscan(struct fsi_master *master)
>   	master->slave_list = false;
>   }
>
> +struct fsi_master *fsi_get_link_master(struct fsi_device *fsi_dev)
> +{
> +	if (fsi_dev && fsi_dev->slave)
> +		return fsi_dev->slave->next_master;
> +	return NULL;
> +}
> +EXPORT_SYMBOL_GPL(fsi_get_link_master);
> +
> +int fsi_set_next_master(struct fsi_device *fsi_dev, struct fsi_master *master)
> +{
> +	if (fsi_dev && fsi_dev->slave) {
> +		fsi_dev->slave->next_master = master;
> +		return 0;
> +	}
> +	return -EINVAL;
> +}
> +EXPORT_SYMBOL_GPL(fsi_set_next_master);
> +
>   static void fsi_master_irq(struct fsi_master *master, int link, uint32_t si1s)
>   {
>   	struct fsi_slave *slave;
> @@ -595,7 +625,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 = atomic_inc_return(&master_idx);
> @@ -657,8 +687,10 @@ static int fsi_bus_match(struct device *dev, struct device_driver *drv)
>   		if (id->engine_type != fsi_dev->engine_type)
>   			continue;
>   		if (id->version == FSI_VERSION_ANY ||
> -				id->version == fsi_dev->version)
> +				id->version == fsi_dev->version) {
> +			fsi_dev->id = id;
>   			return 1;
> +		}
>   	}
>
>   	return 0;
> diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
> index 91bdbf2..92af53d 100644
> --- a/drivers/fsi/fsi-master-gpio.c
> +++ b/drivers/fsi/fsi-master-gpio.c
> @@ -13,6 +13,8 @@
>
>   #include "fsi-master.h"
>
> +#define	FSI_MASTER_GPIO_TYPE	0x1
> +
>   #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 */
> @@ -63,6 +65,8 @@
>   #define	FSI_GPIO_MSG_RESPID_SIZE	2
>   #define	FSI_GPIO_PRIME_SLAVE_CLOCKS	100
>
> +#define	FSI_MASTER_GPIO_LINK_SIZE	0x00800000
> +
>   static DEFINE_SPINLOCK(fsi_gpio_cmd_lock);	/* lock around fsi commands */
>
>   struct fsi_master_gpio {
> @@ -512,11 +516,14 @@ static int fsi_master_gpio_probe(struct platform_device *pdev)
>   	else
>   		master->gpio_mux = gpio;
>
> +	master->master.type = FSI_MASTER_GPIO_TYPE;
>   	master->master.n_links = 1;
> +	master->master.link_size = FSI_MASTER_GPIO_LINK_SIZE;
>   	master->master.read = fsi_master_gpio_read;
>   	master->master.write = fsi_master_gpio_write;
>   	master->master.send_break = fsi_master_gpio_break;
>   	master->master.link_enable = fsi_master_gpio_link_enable;
> +	master->master.dev = &pdev->dev;
>   	platform_set_drvdata(pdev, master);
>
>   	return device_create_file(&pdev->dev, &dev_attr_scan);
> diff --git a/drivers/fsi/fsi-master-hub.c b/drivers/fsi/fsi-master-hub.c
> index 19b7c77..e8c0726 100644
> --- a/drivers/fsi/fsi-master-hub.c
> +++ b/drivers/fsi/fsi-master-hub.c
> @@ -18,14 +18,168 @@
>   #include <linux/cdev.h>
>   #include <linux/fs.h>
>   #include <linux/uaccess.h>
> +#include <linux/slab.h>
> +#include <linux/delay.h>
>
>   #include "fsi-master.h"
>
> +#define FSI_MASTER_HUB_TYPE             0x02
> +
> +#define FSI_MASTER_HUB_MAX_LINKS        8
> +#define FSI_MASTER_HUB_LINK_SIZE        0x00080000
> +
>   #define FSI_ENGID_HUB_MASTER	0x1C
>   #define FSI_ENGID_HUB_LINK	0x1D
>
> +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 link)
> +{
> +	return 0;
> +}
> +
>   static int hub_master_probe(struct device *dev)
>   {
> +	struct fsi_device *fsi_dev = to_fsi_dev(dev);
> +	struct fsi_master *master;
> +	int rc = 0;
> +	uint32_t mver;
> +
> +	master = fsi_get_master(fsi_dev);
> +
> +	/* Two Hub masters in communication chain not allowed */
> +	if (master->type == FSI_MASTER_HUB_TYPE)
> +		return 0;
> +
> +	if (master && (master->n_links >= FSI_MASTER_HUB_MAX_LINKS * 2))
> +		return 0;
> +
> +	if (fsi_dev->id->engine_type == FSI_ENGID_HUB_MASTER) {
> +		master = devm_kzalloc(dev, sizeof(*master), GFP_KERNEL);
> +		if (!master)
> +			return -ENOMEM;
> +
> +		fsi_dev->master = master;
> +
> +		/* TODO: can use master->dev to get at the engine possibly */
> +		master->engine = fsi_dev;
> +		master->read = hub_master_read;
> +		master->write = hub_master_write;
> +		master->send_break = hub_master_break;
> +		master->link_enable = hub_master_link_enable;
> +		master->link_size = FSI_MASTER_HUB_LINK_SIZE;
> +		rc = fsi_set_next_master(fsi_dev, master);
> +
> +		/* Initialize the MFSI (hub master) engine */
> +		rc = fsi_device_read(master->engine, 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_device_write(master->engine, FSI_MRESP0, &mver,
> +					sizeof(mver));
> +		if (rc)
> +			return rc;
> +
> +		mver = FSI_MECTRL_EOAE | FSI_MECTRL_P8_AUTO_TERM;
> +		rc = fsi_device_write(master->engine, 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_device_write(master->engine, FSI_MMODE, &mver,
> +					sizeof(mver));
> +		if (rc)
> +			return rc;
> +
> +		mver = 0xffff0000;
> +		rc = fsi_device_write(master->engine, FSI_MDLYR, &mver,
> +					sizeof(mver));
> +		if (rc)
> +			return rc;
> +
> +		mver = ~0;
> +		rc = fsi_device_write(master->engine, FSI_MSENP0, &mver,
> +					sizeof(mver));
> +		if (rc)
> +			return rc;
> +
> +		/* Leave enabled long enough for master logic to set up */
> +		udelay(1000);
> +
> +		rc = fsi_device_write(master->engine, FSI_MCENP0, &mver,
> +					sizeof(mver));
> +		if (rc)
> +			return rc;
> +
> +		rc = fsi_device_read(master->engine, FSI_MAEB, &mver,
> +					sizeof(mver));
> +		if (rc)
> +			return rc;
> +
> +		mver = FSI_MRESP_RST_ALL_MASTER | FSI_MRESP_RST_ALL_LINK;
> +		rc = fsi_device_write(master->engine, FSI_MRESP0, &mver,
> +					sizeof(mver));
> +		if (rc)
> +			return rc;
> +
> +		rc = fsi_device_read(master->engine, FSI_MLEVP0, &mver,
> +					sizeof(mver));
> +		if (rc)
> +			return rc;
> +
> +		/* Reset the master bridge */
> +		mver = FSI_MRESB_RST_GEN;
> +		rc = fsi_device_write(master->engine, FSI_MRESB0, &mver,
> +					sizeof(mver));
> +		if (rc)
> +			return rc;
> +
> +		mver = FSI_MRESB_RST_ERR;
> +		rc = fsi_device_write(master->engine, FSI_MRESB0, &mver,
> +					sizeof(mver));
> +		if (rc)
> +			return rc;
> +	} else {
> +		/*
> +		 * A hub link device.
> +		 */
> +		master = fsi_get_link_master(fsi_dev);
> +		if (!master)
> +			return -ENODEV;
> +
> +		if (master->n_links == 0) {
> +			/*
> +			 * CFAM Config space doesn't list correct size
> +			 * for hub links, fix this.
> +			 */
> +			fsi_dev->size = fsi_dev->addr *
> +					FSI_MASTER_HUB_MAX_LINKS;
> +			master->link = fsi_dev;
> +		}
> +		master->n_links++;
> +	}
> +
>   	return 0;
>   }
>
> diff --git a/drivers/fsi/fsi-master.h b/drivers/fsi/fsi-master.h
> index 3737404..367b8d9 100644
> --- a/drivers/fsi/fsi-master.h
> +++ b/drivers/fsi/fsi-master.h
> @@ -17,8 +17,57 @@
>   #ifndef DRIVERS_FSI_MASTER_H
>   #define DRIVERS_FSI_MASTER_H
>
> +#include <linux/fsi.h>
>   #include <linux/device.h>
>
> +#define FSI_BREAK			0xc0de0000
> +
> +/* 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;
> @@ -26,6 +75,10 @@ struct fsi_master {
>   	int		idx;
>   	int		n_links;
>   	uint32_t	ipoll;
> +	struct fsi_device *engine;
> +	struct fsi_device *link;
> +	uint8_t		type;
> +	uint32_t	link_size;
>   	int		(*read)(struct fsi_master *, int link,
>   				uint8_t slave, uint32_t addr,
>   				void *val, size_t size);
> @@ -61,4 +114,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 */
> diff --git a/include/linux/fsi.h b/include/linux/fsi.h
> index d22d0c5..9bf0b06 100644
> --- a/include/linux/fsi.h
> +++ b/include/linux/fsi.h
> @@ -20,6 +20,8 @@
>   struct fsi_device {
>   	struct list_head	link;	/* for slave's list */
>   	struct device		dev;
> +	const struct fsi_device_id *id;
> +	struct fsi_master	*master;
>   	u8			engine_type;
>   	u8			version;
>   	u8			unit;
> @@ -71,6 +73,10 @@ extern void fsi_driver_unregister(struct fsi_driver *);
>
>   extern struct bus_type fsi_bus_type;
>
> +extern struct fsi_master *fsi_get_link_master(struct fsi_device *dev);
> +extern int fsi_set_next_master(struct fsi_device *dev,
> +				struct fsi_master *master);
> +extern struct fsi_master *fsi_get_master(struct fsi_device *dev);
>   extern int fsi_enable_irq(struct fsi_device *dev);
>   extern void fsi_disable_irq(struct fsi_device *dev);
>

Re: [PATCH linux dev-4.7 3/5] drivers/fsi: Define hub master read write

[Eddie James]

Acked-by: Eddie James <eajames@linux.vnet.ibm.com>


On 02/16/2017 04:04 PM, Christopher Bostic wrote:
> Define the master read, write, link enable, and break methods.
> Register the hub master with the core.
>
> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> ---
>   drivers/fsi/fsi-master-hub.c | 30 ++++++++++++++++++++++++++----
>   1 file changed, 26 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/fsi/fsi-master-hub.c b/drivers/fsi/fsi-master-hub.c
> index e8c0726..ea6db17 100644
> --- a/drivers/fsi/fsi-master-hub.c
> +++ b/drivers/fsi/fsi-master-hub.c
> @@ -34,23 +34,38 @@
>   int hub_master_read(struct fsi_master *master, int linkno, uint8_t slave,
>   			uint32_t addr, void *val, size_t size)
>   {
> -	return 0;
> +	addr += (linkno * FSI_MASTER_HUB_LINK_SIZE);
> +	return fsi_device_read(master->link, 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;
> +	addr += (linkno * FSI_MASTER_HUB_LINK_SIZE);
> +	return fsi_device_write(master->link, addr, val, size);
>   }
>
>   int hub_master_break(struct fsi_master *master, int linkno)
>   {
> -	return 0;
> +	uint32_t command;
> +	uint32_t break_offset = 0x4; /* hw workaround */
> +
> +	command = FSI_BREAK;
> +	return fsi_device_write(master->link, break_offset, &command,
> +				sizeof(command));
>   }
>
>   int hub_master_link_enable(struct fsi_master *master, int link)
>   {
> -	return 0;
> +	uint32_t menp = L_MSB_MASK(link);
> +	int rc;
> +
> +	rc = fsi_device_write(master->engine, FSI_MSENP0, &menp,
> +				sizeof(menp));
> +	/* Wait for hw to finish enable */
> +	mdelay(10);
> +
> +	return rc;
>   }
>
>   static int hub_master_probe(struct device *dev)
> @@ -178,6 +193,13 @@ static int hub_master_probe(struct device *dev)
>   			master->link = fsi_dev;
>   		}
>   		master->n_links++;
> +
> +		/* Each hub links is listed twice in CFAM config space */
> +		if (master->n_links == (FSI_MASTER_HUB_MAX_LINKS * 2)) {
> +			/* Only need hub link 1 for now */
> +			master->n_links = 2;
> +			return fsi_master_register(master);
> +		}
>   	}
>
>   	return 0;

Re: [PATCH linux dev-4.7 4/5] drivers/fsi: Add FSI bus error cleanup

[Eddie James]

Acked-by: Eddie James <eajames@linux.vnet.ibm.com>


On 02/16/2017 04:04 PM, Christopher Bostic wrote:
> On detect of bus fails attempt to clean them up and try the
> command again.
>
> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> ---
>   drivers/fsi/fsi-master-gpio.c | 100 +++++++++++++++++++++++++++++++++++-------
>   1 file changed, 85 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
> index 92af53d..bb3983c 100644
> --- a/drivers/fsi/fsi-master-gpio.c
> +++ b/drivers/fsi/fsi-master-gpio.c
> @@ -67,8 +67,16 @@
>
>   #define	FSI_MASTER_GPIO_LINK_SIZE	0x00800000
>
> +static int fsi_master_gpio_read(struct fsi_master *_master, int link,
> +		uint8_t slave, uint32_t addr, void *val, size_t size);
> +static int fsi_master_gpio_write(struct fsi_master *_master, int link,
> +		uint8_t slave, uint32_t addr, const void *val, size_t size);
> +static int fsi_master_gpio_break(struct fsi_master *_master, int link);
> +
>   static DEFINE_SPINLOCK(fsi_gpio_cmd_lock);	/* lock around fsi commands */
>
> +uint8_t in_err_cleanup;
> +
>   struct fsi_master_gpio {
>   	struct fsi_master	master;
>   	struct gpio_desc	*gpio_clk;
> @@ -358,24 +366,85 @@ static void build_abs_ar_command(struct fsi_gpio_msg *cmd, uint64_t mode,
>   	cmd->msg >>= (64 - cmd->bits);
>   }
>
> +/*
> + * Clean up the FSI bus path on detection of error.
> + * todo: get rid of magic numbers
> + */
> +static void fsi_master_gpio_handle_error(struct fsi_master_gpio *master,
> +					uint32_t addr)
> +{
> +	uint32_t smode = 0xa0ff0100;
> +
> +	/* Avoid nested error handling */
> +	if (in_err_cleanup)
> +		return;
> +
> +	in_err_cleanup = 1;
> +	fsi_master_gpio_break(&master->master, 0);
> +	udelay(200);
> +
> +	/* Set slave ID in smode */
> +	fsi_master_gpio_write(&master->master, 0, 0, 0x800, &smode,
> +				sizeof(smode));
> +
> +	/* Reset the MFSI bridge */
> +	smode = 0xc0000000;
> +	fsi_master_gpio_write(&master->master, 0, 0, 0x35D0, &smode,
> +				sizeof(smode));
> +
> +	if (addr >= 0x80000) {
> +
> +		/*
> +		 * Break to hub link 1
> +		 * todo: make this handle more arbitrary link topologies
> +		 */
> +		smode = 0xc0de0000;
> +		fsi_master_gpio_write(&master->master, 0, 0, 0x100004, &smode,
> +					sizeof(smode));
> +
> +		smode = 0xa0ff0100;
> +		fsi_master_gpio_write(&master->master, 0, 0, 0x100800, &smode,
> +					sizeof(smode));
> +	}
> +	in_err_cleanup = 0;
> +}
> +
> +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);
> +	rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
> +	if (rc) {
> +		fsi_master_gpio_handle_error(master, addr);
> +
> +		/* Try again */
> +		rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
> +		if (rc)
> +			fsi_master_gpio_handle_error(master, addr);
> +	}
>
>   	return rc;
>   }
> @@ -386,18 +455,20 @@ 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);
> +	rc = send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
> +	if (rc) {
> +		fsi_master_gpio_handle_error(master, addr);
> +
> +		/* Try again */
> +		rc = send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
> +		if (rc)
> +			fsi_master_gpio_handle_error(master, addr);
> +	}
>
>   	return rc;
>   }
> @@ -480,7 +551,6 @@ static int fsi_master_gpio_probe(struct platform_device *pdev)
>   	master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL);
>   	if (!master)
>   		return -ENOMEM;
> -	master->master.dev = &pdev->dev;
>
>   	gpio = devm_gpiod_get(&pdev->dev, "clock", 0);
>   	if (IS_ERR(gpio)) {

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

[Eddie James]

I just tested the patch set on a single socket system. Obviously 
couldn't test hub access, but everything else looks good.

Tested-by: Eddie James <eajames@linux.vnet.ibm.com>


On 02/16/2017 04:04 PM, Christopher Bostic 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.
>
> Christopher Bostic (5):
>    drivers/fsi: Add MFSI a.k.a. hub master device driver
>    drivers/fsi: Initialize hub master engine
>    drivers/fsi: Define hub master read write
>    drivers/fsi: Add FSI bus error cleanup
>    drivers/fsi: Add command line FSI test
>
>   arch/arm/boot/dts/aspeed-bmc-opp-witherspoon.dts | 1 +
>   1 file changed, 1 insertion(+)
>

Re: [PATCH linux dev-4.7 2/5] drivers/fsi: Initialize hub master engine

[Jeremy Kerr]

Hi Chris,

> Define hub master probe and add definitions for general
> master registers and bitfields.

I think some background on what a hub master is (and how it differs from
a cascaded master) would be useful - either here or in Documentation/.

>  struct fsi_slave {
>  	struct list_head	list_link;	/* Master's list of slaves */
>  	struct list_head	my_engines;
>  	struct device		dev;
> -	struct fsi_master	*master;
> +	struct fsi_master	*master;	/* Upstream master */
> +	struct fsi_master	*next_master;	/* Downstream master */
>  	int			link;
>  	uint8_t			id;
> +	uint32_t		base;		/* Base address */
>  };

[...]

> @@ -26,6 +75,10 @@ struct fsi_master {
>  	int		idx;
>  	int		n_links;
>  	uint32_t	ipoll;
> +	struct fsi_device *engine;
> +	struct fsi_device *link;
> +	uint8_t		type;
> +	uint32_t	link_size;
>  	int		(*read)(struct fsi_master *, int link,
>  				uint8_t slave, uint32_t addr,
>  				void *val, size_t size);

It seems to me that you've added a lot of hub-specific definitions to
multiple parts of the generic FSI core code. I would much rather see
this contained to code that supports just the hub.

However, the hub functionality doesn't seem to fit within the
slave-engine device model - as a slave-engine driver (binding on the
0x1c/0x1d entries in the configuration table), the hub driver violates
the device model in that it performs accesses to addresses *way* outside
of the allocations described in the configuration table.

[that's why you've altered fsi_dev->size in the master driver, and can
potentially lead to conflicts with other drivers in future]

From what I can glean from the docs, the hub master has a couple of
interfaces:

 - the control register set described in the slave configuration table
   as type 0x1d

 - a description of the links, as entries in the slave configuration
   table as type 0x1c

 - the actual FSI master address space, provided underneath a *slave*
   (and not a *slave engine* - that distinction is important).

To me, it sounds like we should not make this a slave engine driver, but
a part of the core *slave* code, because the hub masters are attached
directly to the slaves. This would involve:

  - struct fsi_master_hub {
        struct fsi_master master;
        struct fsi_slave *slave;
	uint32_t          control_regs;  /* slave-relative addr of regs */
	uint32_t          base;          /* slave-relative addr of
					    master address space */
    }

  - in fsi_slave_scan():

     - suppressing the 0x1c and 0x1d devices from creating slave engines

     - if we find an 0x1d type in the configuration table:
        - struct fsi_master_hub *hub = kzalloc(...);
	  hub->base = /* pre-defined */
	  hub->control_regs = /* parsed from config table */
	  hub->slave = slave;

     - if (hub) {
          use a count of the 0x1c types to populate n_links
	  fsi_master_register(&hub->master);
       }

  - the read() and write() callbacks of fsi_master_hub just pass through
    to the slave (at offset ->base), and the enable/break will access the
    control_regs.

This means that we keep the hub code encapsulated, and the hub
implementation doesn't leak out to the core fsi_master or fsi_slave
structs.

For cascaded masters, we can probably keep this model of using a slave
engine driver, but I just don't think it's suitable for the hubs.

Regards,


Jeremy

Re: [PATCH linux dev-4.7 5/5] drivers/fsi: Add command line FSI test

[Jeremy Kerr]

Hi Chris,

> Perform FSI reads and writes via the raw sysfs file.  Program
> reads and writes to various locations on the main link and
> off of hub link 1.
> 
> Signed-off-by: Edward James <eajames@us.ibm.com>
> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> ---
>  drivers/fsi/fsitest.c | 163 ++++++++++++++++++++++++++++++++++++++++++++++++++

Try to avoid putting non-kernel code in the general kernel directories;
we have tools/testing/selftests/ for this kind of thing.

Cheers,


Jeremy

Re: [PATCH linux dev-4.7 4/5] drivers/fsi: Add FSI bus error cleanup

[Joel Stanley]

On Fri, Feb 17, 2017 at 8:34 AM, Christopher Bostic
<cbostic@linux.vnet.ibm.com> wrote:
> On detect of bus fails attempt to clean them up and try the
> command again.
>
> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
> ---
>  drivers/fsi/fsi-master-gpio.c | 100 +++++++++++++++++++++++++++++++++++-------
>  1 file changed, 85 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
> index 92af53d..bb3983c 100644
> --- a/drivers/fsi/fsi-master-gpio.c
> +++ b/drivers/fsi/fsi-master-gpio.c
> @@ -67,8 +67,16 @@
>
>  #define        FSI_MASTER_GPIO_LINK_SIZE       0x00800000
>
> +static int fsi_master_gpio_read(struct fsi_master *_master, int link,
> +               uint8_t slave, uint32_t addr, void *val, size_t size);
> +static int fsi_master_gpio_write(struct fsi_master *_master, int link,
> +               uint8_t slave, uint32_t addr, const void *val, size_t size);
> +static int fsi_master_gpio_break(struct fsi_master *_master, int link);
> +
>  static DEFINE_SPINLOCK(fsi_gpio_cmd_lock);     /* lock around fsi commands */
>
> +uint8_t in_err_cleanup;
> +
>  struct fsi_master_gpio {
>         struct fsi_master       master;
>         struct gpio_desc        *gpio_clk;
> @@ -358,24 +366,85 @@ static void build_abs_ar_command(struct fsi_gpio_msg *cmd, uint64_t mode,
>         cmd->msg >>= (64 - cmd->bits);
>  }
>
> +/*
> + * Clean up the FSI bus path on detection of error.
> + * todo: get rid of magic numbers
> + */
> +static void fsi_master_gpio_handle_error(struct fsi_master_gpio *master,
> +                                       uint32_t addr)
> +{
> +       uint32_t smode = 0xa0ff0100;

I was wondering what a smode is?

> +
> +       /* Avoid nested error handling */
> +       if (in_err_cleanup)
> +               return;
> +
> +       in_err_cleanup = 1;

This locking is racy. Instead use a atomic type for in_err_cleanup,
declared in the scope of fsi_master_gpio_handle_error.

> +       fsi_master_gpio_break(&master->master, 0);
> +       udelay(200);
> +
> +       /* Set slave ID in smode */
> +       fsi_master_gpio_write(&master->master, 0, 0, 0x800, &smode,
> +                               sizeof(smode));
> +
> +       /* Reset the MFSI bridge */
> +       smode = 0xc0000000;
> +       fsi_master_gpio_write(&master->master, 0, 0, 0x35D0, &smode,
> +                               sizeof(smode));
> +
> +       if (addr >= 0x80000) {
> +
> +               /*
> +                * Break to hub link 1
> +                * todo: make this handle more arbitrary link topologies
> +                */
> +               smode = 0xc0de0000;
> +               fsi_master_gpio_write(&master->master, 0, 0, 0x100004, &smode,
> +                                       sizeof(smode));
> +
> +               smode = 0xa0ff0100;
> +               fsi_master_gpio_write(&master->master, 0, 0, 0x100800, &smode,
> +                                       sizeof(smode));
> +       }
> +       in_err_cleanup = 0;
> +}
> +
> +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);

I see you've pulled the guts of this function out into send_command.
It would have made it easier to follow if that refactoring was in it's
own patch, and the error handling was introduced afterwards.

> +       rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
> +       if (rc) {
> +               fsi_master_gpio_handle_error(master, addr);
> +
> +               /* Try again */
> +               rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
> +               if (rc)
> +                       fsi_master_gpio_handle_error(master, addr);
> +       }
>
>         return rc;
>  }
> @@ -386,18 +455,20 @@ 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);
> +       rc = send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
> +       if (rc) {
> +               fsi_master_gpio_handle_error(master, addr);
> +
> +               /* Try again */
> +               rc = send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
> +               if (rc)
> +                       fsi_master_gpio_handle_error(master, addr);
> +       }
>
>         return rc;
>  }
> @@ -480,7 +551,6 @@ static int fsi_master_gpio_probe(struct platform_device *pdev)
>         master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL);
>         if (!master)
>                 return -ENOMEM;
> -       master->master.dev = &pdev->dev;

I'm not sure why you're making this change?

>
>         gpio = devm_gpiod_get(&pdev->dev, "clock", 0);
>         if (IS_ERR(gpio)) {
> --
> 1.8.2.2
>

Re: [PATCH linux dev-4.7 5/5] drivers/fsi: Add command line FSI test

[Christopher Bostic]

On 2/17/17 2:08 AM, Jeremy Kerr wrote:
> Hi Chris,
>
>> Perform FSI reads and writes via the raw sysfs file.  Program
>> reads and writes to various locations on the main link and
>> off of hub link 1.
>>
>> Signed-off-by: Edward James <eajames@us.ibm.com>
>> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
>> ---
>>   drivers/fsi/fsitest.c | 163 ++++++++++++++++++++++++++++++++++++++++++++++++++
> Try to avoid putting non-kernel code in the general kernel directories;
> we have tools/testing/selftests/ for this kind of thing.
Hi Jeremy,

Ok will move that to tools

Thanks,
> Cheers,
>
>
> Jeremy
>

Re: [PATCH linux dev-4.7 2/5] drivers/fsi: Initialize hub master engine

[Christopher Bostic]

On 2/17/17 1:42 AM, Jeremy Kerr wrote:
> Hi Chris,
>
>> Define hub master probe and add definitions for general
>> master registers and bitfields.
> I think some background on what a hub master is (and how it differs from
> a cascaded master) would be useful - either here or in Documentation/.

Hi Jeremy,

Ok will add some details here.
>>   struct fsi_slave {
>>   	struct list_head	list_link;	/* Master's list of slaves */
>>   	struct list_head	my_engines;
>>   	struct device		dev;
>> -	struct fsi_master	*master;
>> +	struct fsi_master	*master;	/* Upstream master */
>> +	struct fsi_master	*next_master;	/* Downstream master */
>>   	int			link;
>>   	uint8_t			id;
>> +	uint32_t		base;		/* Base address */
>>   };
> [...]
>
>> @@ -26,6 +75,10 @@ struct fsi_master {
>>   	int		idx;
>>   	int		n_links;
>>   	uint32_t	ipoll;
>> +	struct fsi_device *engine;
>> +	struct fsi_device *link;
>> +	uint8_t		type;
>> +	uint32_t	link_size;
>>   	int		(*read)(struct fsi_master *, int link,
>>   				uint8_t slave, uint32_t addr,
>>   				void *val, size_t size);
> It seems to me that you've added a lot of hub-specific definitions to
> multiple parts of the generic FSI core code. I would much rather see
> this contained to code that supports just the hub.
>
> However, the hub functionality doesn't seem to fit within the
> slave-engine device model - as a slave-engine driver (binding on the
> 0x1c/0x1d entries in the configuration table), the hub driver violates
> the device model in that it performs accesses to addresses *way* outside
> of the allocations described in the configuration table.
>
> [that's why you've altered fsi_dev->size in the master driver, and can
> potentially lead to conflicts with other drivers in future]
I altered the dev size due to the incorrect size of hMFSI links/ports in 
the config table.  More on that below.

>  From what I can glean from the docs, the hub master has a couple of
> interfaces:
>
>   - the control register set described in the slave configuration table
>     as type 0x1d
>   - a description of the links, as entries in the slave configuration
>     table as type 0x1c

I think 0x1d and 0x1c types are swapped in the overall discussion.
>
>   - the actual FSI master address space, provided underneath a *slave*
>     (and not a *slave engine* - that distinction is important).
>
> To me, it sounds like we should not make this a slave engine driver, but
> a part of the core *slave* code, because the hub masters are attached
> directly to the slaves. This would involve:
>
>    - struct fsi_master_hub {
>          struct fsi_master master;
>          struct fsi_slave *slave;
> 	uint32_t          control_regs;  /* slave-relative addr of regs */
> 	uint32_t          base;          /* slave-relative addr of
> 					    master address space */
>      }
>
>    - in fsi_slave_scan():
>
>       - suppressing the 0x1c and 0x1d devices from creating slave engines
>
>       - if we find an 0x1d type in the configuration table:
>          - struct fsi_master_hub *hub = kzalloc(...);
> 	  hub->base = /* pre-defined */
> 	  hub->control_regs = /* parsed from config table */
> 	  hub->slave = slave;
>
>       - if (hub) {
>            use a count of the 0x1c types to populate n_links
> 	  fsi_master_register(&hub->master);
>         }
>
>    - the read() and write() callbacks of fsi_master_hub just pass through
>      to the slave (at offset ->base), and the enable/break will access the
>      control_regs.
>
> This means that we keep the hub code encapsulated, and the hub
> implementation doesn't leak out to the core fsi_master or fsi_slave
> structs.

Good idea, will implement that.
> For cascaded masters, we can probably keep this model of using a slave
> engine driver, but I just don't think it's suitable for the hubs.

There isn't much difference conceptually between a cMFSI master and hub 
hMFSI master.   Only things that vary between the two are number of 
links/ports that might be supported off of them and what size address 
space their links/ports span.  The cMFSI master on P9 has a 0x8000 byte 
addressable window per link/port  whereas hMFSI accesses 0x80000 per link.

The P9 CFAM configuration space does not list the hMFSI port size 
correctly.  Each link/port is listed as size *0*.
The cMFSI link/port size is correct, 0x20 1k pages.

Given their similarities it would make sense to me to use the refactored 
approach you describe above for both hMFSI and and in the future, cMFSI.

Thanks for your input,

Chris

>
> Regards,
>
>
> Jeremy
>

Re: [PATCH linux dev-4.7 4/5] drivers/fsi: Add FSI bus error cleanup

[Christopher Bostic]

On 2/17/17 6:55 AM, Joel Stanley wrote:
> On Fri, Feb 17, 2017 at 8:34 AM, Christopher Bostic
> <cbostic@linux.vnet.ibm.com> wrote:
>> On detect of bus fails attempt to clean them up and try the
>> command again.
>>
>> Signed-off-by: Christopher Bostic <cbostic@linux.vnet.ibm.com>
>> ---
>>   drivers/fsi/fsi-master-gpio.c | 100 +++++++++++++++++++++++++++++++++++-------
>>   1 file changed, 85 insertions(+), 15 deletions(-)
>>
>> diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
>> index 92af53d..bb3983c 100644
>> --- a/drivers/fsi/fsi-master-gpio.c
>> +++ b/drivers/fsi/fsi-master-gpio.c
>> @@ -67,8 +67,16 @@
>>
>>   #define        FSI_MASTER_GPIO_LINK_SIZE       0x00800000
>>
>> +static int fsi_master_gpio_read(struct fsi_master *_master, int link,
>> +               uint8_t slave, uint32_t addr, void *val, size_t size);
>> +static int fsi_master_gpio_write(struct fsi_master *_master, int link,
>> +               uint8_t slave, uint32_t addr, const void *val, size_t size);
>> +static int fsi_master_gpio_break(struct fsi_master *_master, int link);
>> +
>>   static DEFINE_SPINLOCK(fsi_gpio_cmd_lock);     /* lock around fsi commands */
>>
>> +uint8_t in_err_cleanup;
>> +
>>   struct fsi_master_gpio {
>>          struct fsi_master       master;
>>          struct gpio_desc        *gpio_clk;
>> @@ -358,24 +366,85 @@ static void build_abs_ar_command(struct fsi_gpio_msg *cmd, uint64_t mode,
>>          cmd->msg >>= (64 - cmd->bits);
>>   }
>>
>> +/*
>> + * Clean up the FSI bus path on detection of error.
>> + * todo: get rid of magic numbers
>> + */
>> +static void fsi_master_gpio_handle_error(struct fsi_master_gpio *master,
>> +                                       uint32_t addr)
>> +{
>> +       uint32_t smode = 0xa0ff0100;
> I was wondering what a smode is?
Hi Joel,

smode is the Slave Mode register.   I know, its a hack to directly 
access the slave registers from the hub code.  The next revision I'll 
put a bit more thought into how to manage this.

>> +
>> +       /* Avoid nested error handling */
>> +       if (in_err_cleanup)
>> +               return;
>> +
>> +       in_err_cleanup = 1;
> This locking is racy. Instead use a atomic type for in_err_cleanup,
> declared in the scope of fsi_master_gpio_handle_error.

Will change.
>> +       fsi_master_gpio_break(&master->master, 0);
>> +       udelay(200);
>> +
>> +       /* Set slave ID in smode */
>> +       fsi_master_gpio_write(&master->master, 0, 0, 0x800, &smode,
>> +                               sizeof(smode));
>> +
>> +       /* Reset the MFSI bridge */
>> +       smode = 0xc0000000;
>> +       fsi_master_gpio_write(&master->master, 0, 0, 0x35D0, &smode,
>> +                               sizeof(smode));
>> +
>> +       if (addr >= 0x80000) {
>> +
>> +               /*
>> +                * Break to hub link 1
>> +                * todo: make this handle more arbitrary link topologies
>> +                */
>> +               smode = 0xc0de0000;
>> +               fsi_master_gpio_write(&master->master, 0, 0, 0x100004, &smode,
>> +                                       sizeof(smode));
>> +
>> +               smode = 0xa0ff0100;
>> +               fsi_master_gpio_write(&master->master, 0, 0, 0x100800, &smode,
>> +                                       sizeof(smode));
>> +       }
>> +       in_err_cleanup = 0;
>> +}
>> +
>> +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);
> I see you've pulled the guts of this function out into send_command.
> It would have made it easier to follow if that refactoring was in it's
> own patch, and the error handling was introduced afterwards.
One reason I left the send_command refactor in with the rest of this 
patch is that if I didn't then I'd have several locations in the file 
that do the same sequence of steps - itself a cause for a review 
comment.  Guess breaking it out in a separate patch is a preferable 
approach in the interest of making the code more review friendly, will 
change.

>> +       rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
>> +       if (rc) {
>> +               fsi_master_gpio_handle_error(master, addr);
>> +
>> +               /* Try again */
>> +               rc = send_command(master, &cmd, FSI_GPIO_RESP_ACKD, size, val);
>> +               if (rc)
>> +                       fsi_master_gpio_handle_error(master, addr);
>> +       }
>>
>>          return rc;
>>   }
>> @@ -386,18 +455,20 @@ 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);
>> +       rc = send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
>> +       if (rc) {
>> +               fsi_master_gpio_handle_error(master, addr);
>> +
>> +               /* Try again */
>> +               rc = send_command(master, &cmd, FSI_GPIO_RESP_ACK, size, NULL);
>> +               if (rc)
>> +                       fsi_master_gpio_handle_error(master, addr);
>> +       }
>>
>>          return rc;
>>   }
>> @@ -480,7 +551,6 @@ static int fsi_master_gpio_probe(struct platform_device *pdev)
>>          master = devm_kzalloc(&pdev->dev, sizeof(*master), GFP_KERNEL);
>>          if (!master)
>>                  return -ENOMEM;
>> -       master->master.dev = &pdev->dev;
> I'm not sure why you're making this change?

This was related to a change earlier in the patch set.  It doesn't 
belong in this one.  Will fix.

Thanks,

Chris
>
>>          gpio = devm_gpiod_get(&pdev->dev, "clock", 0);
>>          if (IS_ERR(gpio)) {
>> --
>> 1.8.2.2
>>

Re: [PATCH linux dev-4.7 2/5] drivers/fsi: Initialize hub master engine

[Christopher Bostic]

On 2/17/17 11:24 AM, Christopher Bostic wrote:
>
>
> On 2/17/17 1:42 AM, Jeremy Kerr wrote:
>> Hi Chris,
>>
>>> Define hub master probe and add definitions for general
>>> master registers and bitfields.
>> I think some background on what a hub master is (and how it differs from
>> a cascaded master) would be useful - either here or in Documentation/.
>
> Hi Jeremy,
>
> Ok will add some details here.
>>>   struct fsi_slave {
>>>       struct list_head    list_link;    /* Master's list of slaves */
>>>       struct list_head    my_engines;
>>>       struct device        dev;
>>> -    struct fsi_master    *master;
>>> +    struct fsi_master    *master;    /* Upstream master */
>>> +    struct fsi_master    *next_master;    /* Downstream master */
>>>       int            link;
>>>       uint8_t            id;
>>> +    uint32_t        base;        /* Base address */
>>>   };
>> [...]
>>
>>> @@ -26,6 +75,10 @@ struct fsi_master {
>>>       int        idx;
>>>       int        n_links;
>>>       uint32_t    ipoll;
>>> +    struct fsi_device *engine;
>>> +    struct fsi_device *link;
>>> +    uint8_t        type;
>>> +    uint32_t    link_size;
>>>       int        (*read)(struct fsi_master *, int link,
>>>                   uint8_t slave, uint32_t addr,
>>>                   void *val, size_t size);
>> It seems to me that you've added a lot of hub-specific definitions to
>> multiple parts of the generic FSI core code. I would much rather see
>> this contained to code that supports just the hub.
>>
>> However, the hub functionality doesn't seem to fit within the
>> slave-engine device model - as a slave-engine driver (binding on the
>> 0x1c/0x1d entries in the configuration table), the hub driver violates
>> the device model in that it performs accesses to addresses *way* outside
>> of the allocations described in the configuration table.
>>
>> [that's why you've altered fsi_dev->size in the master driver, and can
>> potentially lead to conflicts with other drivers in future]
> I altered the dev size due to the incorrect size of hMFSI links/ports 
> in the config table.  More on that below.
>
>>  From what I can glean from the docs, the hub master has a couple of
>> interfaces:
>>
>>   - the control register set described in the slave configuration table
>>     as type 0x1d
>>   - a description of the links, as entries in the slave configuration
>>     table as type 0x1c
>
> I think 0x1d and 0x1c types are swapped in the overall discussion.
>>
>>   - the actual FSI master address space, provided underneath a *slave*
>>     (and not a *slave engine* - that distinction is important).
>>
>> To me, it sounds like we should not make this a slave engine driver, but
>> a part of the core *slave* code, because the hub masters are attached
>> directly to the slaves. This would involve:
>>
>>    - struct fsi_master_hub {
>>          struct fsi_master master;
>>          struct fsi_slave *slave;
>>     uint32_t          control_regs;  /* slave-relative addr of regs */
>>     uint32_t          base;          /* slave-relative addr of
>>                         master address space */
>>      }
>>
>>    - in fsi_slave_scan():
>>
>>       - suppressing the 0x1c and 0x1d devices from creating slave 
>> engines
>>
>>       - if we find an 0x1d type in the configuration table:
>>          - struct fsi_master_hub *hub = kzalloc(...);
>>       hub->base = /* pre-defined */
>>       hub->control_regs = /* parsed from config table */
>>       hub->slave = slave;
>>
>>       - if (hub) {
>>            use a count of the 0x1c types to populate n_links
>>       fsi_master_register(&hub->master);
>>         }
>>
>>    - the read() and write() callbacks of fsi_master_hub just pass 
>> through
>>      to the slave (at offset ->base), and the enable/break will 
>> access the
>>      control_regs.
>>
>> This means that we keep the hub code encapsulated, and the hub
>> implementation doesn't leak out to the core fsi_master or fsi_slave
>> structs.
>
> Good idea, will implement that.
>> For cascaded masters, we can probably keep this model of using a slave
>> engine driver, but I just don't think it's suitable for the hubs.
>
> There isn't much difference conceptually between a cMFSI master and 
> hub hMFSI master.   Only things that vary between the two are number 
> of links/ports that might be supported off of them and what size 
> address space their links/ports span.  The cMFSI master on P9 has a 
> 0x8000 byte addressable window per link/port  whereas hMFSI accesses 
> 0x80000 per link.
>
> The P9 CFAM configuration space does not list the hMFSI port size 
> correctly.  Each link/port is listed as size *0*.
> The cMFSI link/port size is correct, 0x20 1k pages.
>
> Given their similarities it would make sense to me to use the 
> refactored approach you describe above for both hMFSI and and in the 
> future, cMFSI.

Hi Jeremy,

Since the hMFSI link type does have a number of 1k slots like cMFSI 
would this influence your proposed changes?
Sounded like you were inclined to keep the slave engine driver model for 
cMFSI.

Thanks,
> Thanks for your input,
>
> Chris
>
>>
>> Regards,
>>
>>
>> Jeremy
>>
>

Re: [PATCH linux dev-4.7 2/5] drivers/fsi: Initialize hub master engine

[Jeremy Kerr]

Hi Chris,

>> [that's why you've altered fsi_dev->size in the master driver, and can
>> potentially lead to conflicts with other drivers in future]
> I altered the dev size due to the incorrect size of hMFSI links/ports in 
> the config table.  More on that below.

But I don't think the sizes are incorrect, if the HW appears as is
described in the docs. More on that below too.

>>  From what I can glean from the docs, the hub master has a couple of
>> interfaces:
>>
>>   - the control register set described in the slave configuration table
>>     as type 0x1d
>>   - a description of the links, as entries in the slave configuration
>>     table as type 0x1c
> 
> I think 0x1d and 0x1c types are swapped in the overall discussion.

Yep, I'd realised that after I sent it, sorry!

>> For cascaded masters, we can probably keep this model of using a slave
>> engine driver, but I just don't think it's suitable for the hubs.
> 
> There isn't much difference conceptually between a cMFSI master and hub 
> hMFSI master.

There is one critical difference for a driver implementation:

 - cMFSI: the master's address space is contained entirely within the
   slave engine (ie, the address and size described in the upstream
   configuration table)

 - hMFSI: the master's address space is entirely *outside* the slave
   engine. The slave engine's address range only describes the
   configuration registers.

> Only things that vary between the two are number of 
> links/ports that might be supported off of them and what size address 
> space their links/ports span.  The cMFSI master on P9 has a 0x8000 byte 
> addressable window per link/port  whereas hMFSI accesses 0x80000 per link.
> 
> The P9 CFAM configuration space does not list the hMFSI port size 
> correctly.  Each link/port is listed as size *0*.
> The cMFSI link/port size is correct, 0x20 1k pages.

I disagree that it's incorrect: the link/port entries in the
configuration table *should* have a zero size, as they do not describe
any address range allocated to a *slave engine*.

[from above: the address range for a hMFSI is not within a slave engine]

What they do give us is the count of links, so we should use them for
that.

Because we know that there's a 0x80000-byte range per link, the link
count does tell us how much of the *slave* address space to allocate,
but that's not what the configuration table size field is intended to
describe.

> Given their similarities it would make sense to me to use the refactored 
> approach you describe above for both hMFSI and and in the future, cMFSI.

The difference above means that cMFSI can be implemented as a
slave-engine driver, because its accesses are all within a slave
engine's allocated address space.

We can probably share a lot of code between the two, because of the
shared control register set. But let's focus on one at a time for now :)

Cheers,


Jeremy