[PATCH linux dev-5.4 v2] fsi: master: Add link_disable function

[PATCH linux dev-5.4 v2] fsi: master: Add link_disable function

[Eddie James]

The master driver should disable links that don't have slaves or links
that fail to be accessed. To do this, add a link_disable function and
use it in the failure path for slave break and init.

Signed-off-by: Eddie James <eajames@linux.ibm.com>
---
Changes since v1:
 - Drop the delay and read back MENP0 to check link status.

 drivers/fsi/fsi-core.c          | 13 ++++++++++++-
 drivers/fsi/fsi-master-aspeed.c | 15 +++++++++++++++
 drivers/fsi/fsi-master-hub.c    | 15 +++++++++++++++
 drivers/fsi/fsi-master.h        |  1 +
 4 files changed, 43 insertions(+), 1 deletion(-)

diff --git a/drivers/fsi/fsi-core.c b/drivers/fsi/fsi-core.c
index 8244da8a7241..d81ee9f582a5 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_disable)
+		return master->link_disable(master, link);
+
+	return 0;
+}
+
 static int fsi_master_link_enable(struct fsi_master *master, int link)
 {
 	if (master->link_enable)
@@ -1194,10 +1202,13 @@ static int fsi_master_scan(struct fsi_master *master)
 		if (rc) {
 			dev_dbg(&master->dev,
 				"break to link %d failed: %d\n", link, rc);
+			fsi_master_link_disable(master, link);
 			continue;
 		}
 
-		fsi_slave_init(master, link, 0);
+		rc = fsi_slave_init(master, link, 0);
+		if (rc)
+			fsi_master_link_disable(master, link);
 	}
 
 	return 0;
diff --git a/drivers/fsi/fsi-master-aspeed.c b/drivers/fsi/fsi-master-aspeed.c
index f49742b310c2..12e48f6a0874 100644
--- a/drivers/fsi/fsi-master-aspeed.c
+++ b/drivers/fsi/fsi-master-aspeed.c
@@ -299,6 +299,20 @@ static int aspeed_master_write(struct fsi_master *master, int link,
 	return 0;
 }
 
+static int aspeed_master_link_disable(struct fsi_master *master, int link)
+{
+	struct fsi_master_aspeed *aspeed = to_fsi_master_aspeed(master);
+	int idx, bit;
+	__be32 reg;
+
+	idx = link / 32;
+	bit = link % 32;
+
+	reg = cpu_to_be32(0x80000000 >> bit);
+
+	return opb_writel(aspeed, ctrl_base + FSI_MCENP0 + (4 * idx), reg);
+}
+
 static int aspeed_master_link_enable(struct fsi_master *master, int link)
 {
 	struct fsi_master_aspeed *aspeed = to_fsi_master_aspeed(master);
@@ -491,6 +505,7 @@ static int fsi_master_aspeed_probe(struct platform_device *pdev)
 	aspeed->master.write = aspeed_master_write;
 	aspeed->master.send_break = aspeed_master_break;
 	aspeed->master.term = aspeed_master_term;
+	aspeed->master.link_disable = aspeed_master_link_disable;
 	aspeed->master.link_enable = aspeed_master_link_enable;
 
 	dev_set_drvdata(&pdev->dev, aspeed);
diff --git a/drivers/fsi/fsi-master-hub.c b/drivers/fsi/fsi-master-hub.c
index def35cf92571..6034e8fd8d3b 100644
--- a/drivers/fsi/fsi-master-hub.c
+++ b/drivers/fsi/fsi-master-hub.c
@@ -77,6 +77,20 @@ 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_disable(struct fsi_master *master, int link)
+{
+	struct fsi_master_hub *hub = to_fsi_master_hub(master);
+	int idx, bit;
+	__be32 reg;
+
+	idx = link / 32;
+	bit = link % 32;
+
+	reg = cpu_to_be32(0x80000000 >> bit);
+
+	return fsi_device_write(hub->upstream, FSI_MCENP0 + (4 * idx), &reg, 4);
+}
+
 static int hub_master_link_enable(struct fsi_master *master, int link)
 {
 	struct fsi_master_hub *hub = to_fsi_master_hub(master);
@@ -228,6 +242,7 @@ static int hub_master_probe(struct device *dev)
 	hub->master.read = hub_master_read;
 	hub->master.write = hub_master_write;
 	hub->master.send_break = hub_master_break;
+	hub->master.link_disable = hub_master_link_disable;
 	hub->master.link_enable = hub_master_link_enable;
 
 	dev_set_drvdata(dev, hub);
diff --git a/drivers/fsi/fsi-master.h b/drivers/fsi/fsi-master.h
index 6e8d4d4d5149..7ecb86a678f9 100644
--- a/drivers/fsi/fsi-master.h
+++ b/drivers/fsi/fsi-master.h
@@ -130,6 +130,7 @@ 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_disable)(struct fsi_master *, int link);
 	int		(*link_enable)(struct fsi_master *, int link);
 	int		(*link_config)(struct fsi_master *, int link,
 				       u8 t_send_delay, u8 t_echo_delay);
-- 
2.24.0

Re: [PATCH linux dev-5.4 v2] fsi: master: Add link_disable function

[Jeremy Kerr]

Hi Eddie,

> The master driver should disable links that don't have slaves or links
> that fail to be accessed. To do this, add a link_disable function and
> use it in the failure path for slave break and init.

Concept looks good, but would it work to merge the disable
functionality with the existing link_enable callback?

ie., something like:

--- a/drivers/fsi/fsi-master.h
+++ b/drivers/fsi/fsi-master.h
@@ -130,6 +130,7 @@ 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);

(and add the bool to the core api too).

- as I suspect the logic between enable and disable will be much the
same, aside from the actual value set.

Along those lines:

> --- a/drivers/fsi/fsi-master-aspeed.c
> +++ b/drivers/fsi/fsi-master-aspeed.c
> @@ -299,6 +299,20 @@ static int aspeed_master_write(struct fsi_master *master, int link,
>  	return 0;
>  }
>  
> +static int aspeed_master_link_disable(struct fsi_master *master, int link)
> +{
> +	struct fsi_master_aspeed *aspeed = to_fsi_master_aspeed(master);
> +	int idx, bit;
> +	__be32 reg;
> +
> +	idx = link / 32;
> +	bit = link % 32;
> +
> +	reg = cpu_to_be32(0x80000000 >> bit);
> +
> +	return opb_writel(aspeed, ctrl_base + FSI_MCENP0 + (4 * idx), reg);
> +}

If we don't need the delay and read-back here, should we drop it from
enable too?

Cheers,


Jeremy

Re: [PATCH linux dev-5.4 v2] fsi: master: Add link_disable function

[Eddie James]

On 3/31/20 12:18 AM, Jeremy Kerr wrote:
> Hi Eddie,
>
>> The master driver should disable links that don't have slaves or links
>> that fail to be accessed. To do this, add a link_disable function and
>> use it in the failure path for slave break and init.
> Concept looks good, but would it work to merge the disable
> functionality with the existing link_enable callback?
>
> ie., something like:
>
> --- a/drivers/fsi/fsi-master.h
> +++ b/drivers/fsi/fsi-master.h
> @@ -130,6 +130,7 @@ 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);
>
> (and add the bool to the core api too).
>
> - as I suspect the logic between enable and disable will be much the
> same, aside from the actual value set.
>
> Along those lines:
>
>> --- a/drivers/fsi/fsi-master-aspeed.c
>> +++ b/drivers/fsi/fsi-master-aspeed.c
>> @@ -299,6 +299,20 @@ static int aspeed_master_write(struct fsi_master *master, int link,
>>   	return 0;
>>   }
>>   
>> +static int aspeed_master_link_disable(struct fsi_master *master, int link)
>> +{
>> +	struct fsi_master_aspeed *aspeed = to_fsi_master_aspeed(master);
>> +	int idx, bit;
>> +	__be32 reg;
>> +
>> +	idx = link / 32;
>> +	bit = link % 32;
>> +
>> +	reg = cpu_to_be32(0x80000000 >> bit);
>> +
>> +	return opb_writel(aspeed, ctrl_base + FSI_MCENP0 + (4 * idx), reg);
>> +}
> If we don't need the delay and read-back here, should we drop it from
> enable too?


No, the delay is required by the FSI spec for enabling the link.


Thanks for the review!

Eddie


>
> Cheers,
>
>
> Jeremy
>