[PATCH linux dev-5.4 0/3] fsi: Disable link when slave init fails

[PATCH linux dev-5.4 0/3] fsi: Disable link when slave init fails

[Eddie James]

This series modifies the link_enable function to accept a boolean parameter to
indicate whether to enable or disable the link. Then, it disables the link when
initialization of the slave fails or if there is no slave on the link.

Eddie James (3):
  fsi: master: Remove link enable read-back
  fsi: master: Add boolean parameter to link_enable function
  fsi: core: Disable link when slave init fails

 drivers/fsi/fsi-core.c          | 15 +++++++++++++--
 drivers/fsi/fsi-master-aspeed.c | 25 +++++++++++--------------
 drivers/fsi/fsi-master-ast-cf.c |  5 +++--
 drivers/fsi/fsi-master-gpio.c   |  5 +++--
 drivers/fsi/fsi-master-hub.c    | 17 ++++++++++++-----
 drivers/fsi/fsi-master.h        |  3 ++-
 6 files changed, 44 insertions(+), 26 deletions(-)

-- 
2.24.0

[PATCH linux dev-5.4 1/3] fsi: master: Remove link enable read-back

[Eddie James]

Both the Aspeed and hub masters read back the link enable register
after enabling the link, but this is unnecessary, so remove it.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/fsi/fsi-master-aspeed.c | 11 +----------
 drivers/fsi/fsi-master-hub.c    |  6 +++---
 2 files changed, 4 insertions(+), 13 deletions(-)

diff --git a/drivers/fsi/fsi-master-aspeed.c b/drivers/fsi/fsi-master-aspeed.c
index fe2da6f90590..0c04656208a6 100644
--- a/drivers/fsi/fsi-master-aspeed.c
+++ b/drivers/fsi/fsi-master-aspeed.c
@@ -306,7 +306,7 @@ static int aspeed_master_link_enable(struct fsi_master *master, int link)
 {
 	struct fsi_master_aspeed *aspeed = to_fsi_master_aspeed(master);
 	int idx, bit, ret;
-	__be32 reg, result;
+	__be32 reg;
 
 	idx = link / 32;
 	bit = link % 32;
@@ -319,15 +319,6 @@ static int aspeed_master_link_enable(struct fsi_master *master, int link)
 
 	mdelay(FSI_LINK_ENABLE_SETUP_TIME);
 
-	ret = opb_readl(aspeed, ctrl_base + FSI_MENP0 + (4 * idx), &result);
-	if (ret)
-		return ret;
-
-	if (result != reg) {
-		dev_err(aspeed->dev, "%s failed: %08x\n", __func__, result);
-		return -EIO;
-	}
-
 	return 0;
 }
 
diff --git a/drivers/fsi/fsi-master-hub.c b/drivers/fsi/fsi-master-hub.c
index def35cf92571..f89c25d68b5a 100644
--- a/drivers/fsi/fsi-master-hub.c
+++ b/drivers/fsi/fsi-master-hub.c
@@ -90,12 +90,12 @@ static int hub_master_link_enable(struct fsi_master *master, int link)
 	reg = cpu_to_be32(0x80000000 >> bit);
 
 	rc = fsi_device_write(hub->upstream, FSI_MSENP0 + (4 * idx), &reg, 4);
+	if (rc)
+		return rc;
 
 	mdelay(FSI_LINK_ENABLE_SETUP_TIME);
 
-	fsi_device_read(hub->upstream, FSI_MENP0 + (4 * idx), &reg, 4);
-
-	return rc;
+	return 0;
 }
 
 static void hub_master_release(struct device *dev)
-- 
2.24.0

[PATCH linux dev-5.4 2/3] fsi: master: Add boolean parameter to link_enable function

[Eddie James]

Add the ability to disable a link with a boolean parameter to the
link_enable function.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/fsi/fsi-core.c          |  2 +-
 drivers/fsi/fsi-master-aspeed.c | 18 ++++++++++++------
 drivers/fsi/fsi-master-ast-cf.c |  5 +++--
 drivers/fsi/fsi-master-gpio.c   |  5 +++--
 drivers/fsi/fsi-master-hub.c    | 19 +++++++++++++------
 drivers/fsi/fsi-master.h        |  3 ++-
 6 files changed, 34 insertions(+), 18 deletions(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 8244da8a7241..0743bba42757 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -1157,7 +1157,7 @@ static int fsi_master_write(struct fsi_master *master, int link,
 static int fsi_master_link_enable(struct fsi_master *master, int link)
 {
 	if (master->link_enable)
-		return master->link_enable(master, link);
+		return master->link_enable(master, link, true);
 
 	return 0;
 }
diff --git a/drivers/fsi/fsi-master-aspeed.c b/drivers/fsi/fsi-master-aspeed.c
index 0c04656208a6..98e3bd233355 100644
--- a/drivers/fsi/fsi-master-aspeed.c
+++ b/drivers/fsi/fsi-master-aspeed.c
@@ -302,7 +302,8 @@ static int aspeed_master_write(struct fsi_master *master, int link,
 	return 0;
 }
 
-static int aspeed_master_link_enable(struct fsi_master *master, int link)
+static int aspeed_master_link_enable(struct fsi_master *master, int link,
+				     bool enable)
 {
 	struct fsi_master_aspeed *aspeed = to_fsi_master_aspeed(master);
 	int idx, bit, ret;
@@ -313,13 +314,18 @@ static int aspeed_master_link_enable(struct fsi_master *master, int link)
 
 	reg = cpu_to_be32(0x80000000 >> bit);
 
-	ret = opb_writel(aspeed, ctrl_base + FSI_MSENP0 + (4 * idx), reg);
-	if (ret)
-		return ret;
+	if (enable) {
+		ret = opb_writel(aspeed, ctrl_base + FSI_MSENP0 + (4 * idx),
+				 reg);
+		if (ret)
+			return ret;
 
-	mdelay(FSI_LINK_ENABLE_SETUP_TIME);
+		mdelay(FSI_LINK_ENABLE_SETUP_TIME);
 
-	return 0;
+		return 0;
+	}
+
+	return opb_writel(aspeed, ctrl_base + FSI_MCENP0 + (4 * idx), reg);
 }
 
 static int aspeed_master_term(struct fsi_master *master, int link, uint8_t id)
diff --git a/drivers/fsi/fsi-master-ast-cf.c b/drivers/fsi/fsi-master-ast-cf.c
index 04d10ea8d343..62dcc71a30e6 100644
--- a/drivers/fsi/fsi-master-ast-cf.c
+++ b/drivers/fsi/fsi-master-ast-cf.c
@@ -1039,7 +1039,8 @@ static void fsi_master_acf_setup_external(struct fsi_master_acf *master)
 	gpiod_direction_input(master->gpio_data);
 }
 
-static int fsi_master_acf_link_enable(struct fsi_master *_master, int link)
+static int fsi_master_acf_link_enable(struct fsi_master *_master, int link,
+				      bool enable)
 {
 	struct fsi_master_acf *master = to_fsi_master_acf(_master);
 	int rc = -EBUSY;
@@ -1049,7 +1050,7 @@ static int fsi_master_acf_link_enable(struct fsi_master *_master, int link)
 
 	mutex_lock(&master->lock);
 	if (!master->external_mode) {
-		gpiod_set_value(master->gpio_enable, 1);
+		gpiod_set_value(master->gpio_enable, enable ? 1 : 0);
 		rc = 0;
 	}
 	mutex_unlock(&master->lock);
diff --git a/drivers/fsi/fsi-master-gpio.c b/drivers/fsi/fsi-master-gpio.c
index 4dcce17f243f..aa97c4a250cb 100644
--- a/drivers/fsi/fsi-master-gpio.c
+++ b/drivers/fsi/fsi-master-gpio.c
@@ -678,7 +678,8 @@ static void fsi_master_gpio_init_external(struct fsi_master_gpio *master)
 	gpiod_direction_input(master->gpio_data);
 }
 
-static int fsi_master_gpio_link_enable(struct fsi_master *_master, int link)
+static int fsi_master_gpio_link_enable(struct fsi_master *_master, int link,
+				       bool enable)
 {
 	struct fsi_master_gpio *master = to_fsi_master_gpio(_master);
 	int rc = -EBUSY;
@@ -688,7 +689,7 @@ static int fsi_master_gpio_link_enable(struct fsi_master *_master, int link)
 
 	mutex_lock(&master->cmd_lock);
 	if (!master->external_mode) {
-		gpiod_set_value(master->gpio_enable, 1);
+		gpiod_set_value(master->gpio_enable, enable ? 1 : 0);
 		rc = 0;
 	}
 	mutex_unlock(&master->cmd_lock);
diff --git a/drivers/fsi/fsi-master-hub.c b/drivers/fsi/fsi-master-hub.c
index f89c25d68b5a..b53240df2f9b 100644
--- a/drivers/fsi/fsi-master-hub.c
+++ b/drivers/fsi/fsi-master-hub.c
@@ -77,7 +77,8 @@ static int hub_master_break(struct fsi_master *master, int link)
 	return hub_master_write(master, link, 0, addr, &cmd, sizeof(cmd));
 }
 
-static int hub_master_link_enable(struct fsi_master *master, int link)
+static int hub_master_link_enable(struct fsi_master *master, int link,
+				  bool enable)
 {
 	struct fsi_master_hub *hub = to_fsi_master_hub(master);
 	int idx, bit;
@@ -89,13 +90,19 @@ static int hub_master_link_enable(struct fsi_master *master, int link)
 
 	reg = cpu_to_be32(0x80000000 >> bit);
 
-	rc = fsi_device_write(hub->upstream, FSI_MSENP0 + (4 * idx), &reg, 4);
-	if (rc)
-		return rc;
+	if (enable) {
+		rc = fsi_device_write(hub->upstream, FSI_MSENP0 + (4 * idx),
+				      &reg, 4);
+		if (rc)
+			return rc;
 
-	mdelay(FSI_LINK_ENABLE_SETUP_TIME);
+		mdelay(FSI_LINK_ENABLE_SETUP_TIME);
 
-	return 0;
+		return 0;
+	}
+
+	return fsi_device_write(hub->upstream, FSI_MCENP0 + (4 * idx), &reg,
+				4);
 }
 
 static void hub_master_release(struct device *dev)
diff --git a/drivers/fsi/fsi-master.h b/drivers/fsi/fsi-master.h
index 6e8d4d4d5149..cd6bee5e12a7 100644
--- a/drivers/fsi/fsi-master.h
+++ b/drivers/fsi/fsi-master.h
@@ -130,7 +130,8 @@ struct fsi_master {
 				uint32_t addr, const void *val, size_t size);
 	int		(*term)(struct fsi_master *, int link, uint8_t id);
 	int		(*send_break)(struct fsi_master *, int link);
-	int		(*link_enable)(struct fsi_master *, int link);
+	int		(*link_enable)(struct fsi_master *, int link,
+				       bool enable);
 	int		(*link_config)(struct fsi_master *, int link,
 				       u8 t_send_delay, u8 t_echo_delay);
 };
-- 
2.24.0

[PATCH linux dev-5.4 3/3] fsi: core: Disable link when slave init fails

[Eddie James]

In the case that links don't have slaves or fail to be accessed, the
master should disable the link during the scan.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
 drivers/fsi/fsi-core.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 0743bba42757..c9c3842f6e62 100644
--- a/drivers/fsi/fsi-core.c
+++ b/drivers/fsi/fsi-core.c
@@ -1154,6 +1154,14 @@ static int fsi_master_write(struct fsi_master *master, int link,
 	return rc;
 }
 
+static int fsi_master_link_disable(struct fsi_master *master, int link)
+{
+	if (master->link_enable)
+		return master->link_enable(master, link, false);
+
+	return 0;
+}
+
 static int fsi_master_link_enable(struct fsi_master *master, int link)
 {
 	if (master->link_enable)
@@ -1192,12 +1200,15 @@ static int fsi_master_scan(struct fsi_master *master)
 		}
 		rc = fsi_master_break(master, link);
 		if (rc) {
+			fsi_master_link_disable(master, link);
 			dev_dbg(&master->dev,
 				"break to link %d failed: %d\n", link, rc);
 			continue;
 		}
 
-		fsi_slave_init(master, link, 0);
+		rc = fsi_slave_init(master, link, 0);
+		if (rc)
+			fsi_master_link_disable(master, link);
 	}
 
 	return 0;
-- 
2.24.0

Re: [PATCH linux dev-5.4 1/3] fsi: master: Remove link enable read-back

[Andrew Jeffery]

On Tue, 7 Apr 2020, at 04:49, Eddie James wrote:
> Both the Aspeed and hub masters read back the link enable register
> after enabling the link, but this is unnecessary, so remove it.

Unfortunately the code isn't commented, but you're making the assertion
that it's not necessary without explaining why. Are we sure it's not
implementing a workaround e.g. to make sure the write is flushed?

Or is this assertion made on the basis that the result is unused?

Andrew

Re: [PATCH linux dev-5.4 2/3] fsi: master: Add boolean parameter to link_enable function

[Andrew Jeffery]

On Tue, 7 Apr 2020, at 04:49, Eddie James wrote:
> Add the ability to disable a link with a boolean parameter to the
> link_enable function.

Right, but when might we want to disable a link? This functionality
didn't exist previously, so why does it need to exist now?

Andrew

Re: [PATCH linux dev-5.4 3/3] fsi: core: Disable link when slave init fails

[Andrew Jeffery]

On Tue, 7 Apr 2020, at 04:49, Eddie James wrote:
> In the case that links don't have slaves or fail to be accessed, the
> master should disable the link during the scan.

What are the side-effects of leaving it enabled?

Andrew