* [PATCH linux dev-5.4] fsi: master: Add link_disable function
@ 2020-02-17 20:16 Eddie James
2020-02-25 2:01 ` Joel Stanley
0 siblings, 1 reply; 3+ messages in thread
From: Eddie James @ 2020-02-17 20:16 UTC (permalink / raw)
To: openbmc; +Cc: joel, andrew, 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>
---
drivers/fsi/fsi-core.c | 13 ++++++++++++-
drivers/fsi/fsi-master-aspeed.c | 30 ++++++++++++++++++++++++++++++
drivers/fsi/fsi-master-hub.c | 22 ++++++++++++++++++++++
drivers/fsi/fsi-master.h | 1 +
4 files changed, 65 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..7ce5d9eb6c78 100644
--- a/drivers/fsi/fsi-master-aspeed.c
+++ b/drivers/fsi/fsi-master-aspeed.c
@@ -299,6 +299,35 @@ 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, ret;
+ __be32 reg, result;
+
+ idx = link / 32;
+ bit = link % 32;
+
+ reg = cpu_to_be32(0x80000000 >> bit);
+
+ ret = opb_writel(aspeed, ctrl_base + FSI_MCENP0 + (4 * idx), reg);
+ if (ret)
+ return ret;
+
+ 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;
+}
+
static int aspeed_master_link_enable(struct fsi_master *master, int link)
{
struct fsi_master_aspeed *aspeed = to_fsi_master_aspeed(master);
@@ -491,6 +520,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..26617fd5e2de 100644
--- a/drivers/fsi/fsi-master-hub.c
+++ b/drivers/fsi/fsi-master-hub.c
@@ -77,6 +77,27 @@ 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;
+ int rc;
+
+ idx = link / 32;
+ bit = link % 32;
+
+ reg = cpu_to_be32(0x80000000 >> bit);
+
+ rc = fsi_device_write(hub->upstream, FSI_MCENP0 + (4 * idx), ®, 4);
+
+ mdelay(FSI_LINK_ENABLE_SETUP_TIME);
+
+ fsi_device_read(hub->upstream, FSI_MENP0 + (4 * idx), ®, 4);
+
+ return rc;
+}
+
static int hub_master_link_enable(struct fsi_master *master, int link)
{
struct fsi_master_hub *hub = to_fsi_master_hub(master);
@@ -228,6 +249,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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH linux dev-5.4] fsi: master: Add link_disable function
2020-02-17 20:16 [PATCH linux dev-5.4] fsi: master: Add link_disable function Eddie James
@ 2020-02-25 2:01 ` Joel Stanley
2020-02-25 16:24 ` Eddie James
0 siblings, 1 reply; 3+ messages in thread
From: Joel Stanley @ 2020-02-25 2:01 UTC (permalink / raw)
To: Eddie James; +Cc: OpenBMC Maillist, Andrew Jeffery, Jeremy Kerr
On Mon, 17 Feb 2020 at 20:16, Eddie James <eajames@linux.ibm.com> wrote:
>
> 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.
The implementation looks okay, aside from the code duplication.
We should clear up my first question about disabling the link in all
error paths. The other questions don't need to block this going into
dev-5.4, but should be resolved when upstreaming.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
> drivers/fsi/fsi-core.c | 13 ++++++++++++-
> drivers/fsi/fsi-master-aspeed.c | 30 ++++++++++++++++++++++++++++++
> drivers/fsi/fsi-master-hub.c | 22 ++++++++++++++++++++++
> drivers/fsi/fsi-master.h | 1 +
> 4 files changed, 65 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);
Do we want to set the link as disabled when the init fails in all
fsi_slave_init error paths?
> }
>
> return 0;
> diff --git a/drivers/fsi/fsi-master-aspeed.c b/drivers/fsi/fsi-master-aspeed.c
> index f49742b310c2..7ce5d9eb6c78 100644
> --- a/drivers/fsi/fsi-master-aspeed.c
> +++ b/drivers/fsi/fsi-master-aspeed.c
> @@ -299,6 +299,35 @@ 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, ret;
> + __be32 reg, result;
> +
> + idx = link / 32;
> + bit = link % 32;
> +
> + reg = cpu_to_be32(0x80000000 >> bit);
> +
> + ret = opb_writel(aspeed, ctrl_base + FSI_MCENP0 + (4 * idx), reg);
> + if (ret)
> + return ret;
> +
> + mdelay(FSI_LINK_ENABLE_SETUP_TIME);
> +
> + ret = opb_readl(aspeed, ctrl_base + FSI_MENP0 + (4 * idx), &result);
> + if (ret)
> + return ret;
Do we need to have the delay and read when disabling?
> +
> + if (result & reg) {
> + dev_err(aspeed->dev, "%s failed: %08x\n", __func__, result);
> + return -EIO;
> + }
> +
> + return 0;
> +}
> +
> static int aspeed_master_link_enable(struct fsi_master *master, int link)
> {
> struct fsi_master_aspeed *aspeed = to_fsi_master_aspeed(master);
> @@ -491,6 +520,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..26617fd5e2de 100644
> --- a/drivers/fsi/fsi-master-hub.c
> +++ b/drivers/fsi/fsi-master-hub.c
> @@ -77,6 +77,27 @@ 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;
> + int rc;
> +
> + idx = link / 32;
> + bit = link % 32;
> +
> + reg = cpu_to_be32(0x80000000 >> bit);
> +
> + rc = fsi_device_write(hub->upstream, FSI_MCENP0 + (4 * idx), ®, 4);
> +
> + mdelay(FSI_LINK_ENABLE_SETUP_TIME);
> +
> + fsi_device_read(hub->upstream, FSI_MENP0 + (4 * idx), ®, 4);
> +
> + return rc;
> +}
This is the same code, with fsi_device_read instead of obp_read.
We should look to share this code between the aspeed master and the hub master.
> +
> static int hub_master_link_enable(struct fsi_master *master, int link)
> {
> struct fsi_master_hub *hub = to_fsi_master_hub(master);
> @@ -228,6 +249,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
>
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH linux dev-5.4] fsi: master: Add link_disable function
2020-02-25 2:01 ` Joel Stanley
@ 2020-02-25 16:24 ` Eddie James
0 siblings, 0 replies; 3+ messages in thread
From: Eddie James @ 2020-02-25 16:24 UTC (permalink / raw)
To: Joel Stanley; +Cc: OpenBMC Maillist, Andrew Jeffery, Jeremy Kerr
On 2/24/20 8:01 PM, Joel Stanley wrote:
> On Mon, 17 Feb 2020 at 20:16, Eddie James <eajames@linux.ibm.com> wrote:
>> 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.
> The implementation looks okay, aside from the code duplication.
>
> We should clear up my first question about disabling the link in all
> error paths. The other questions don't need to block this going into
> dev-5.4, but should be resolved when upstreaming.
>
>> Signed-off-by: Eddie James <eajames@linux.ibm.com>
>> ---
>> drivers/fsi/fsi-core.c | 13 ++++++++++++-
>> drivers/fsi/fsi-master-aspeed.c | 30 ++++++++++++++++++++++++++++++
>> drivers/fsi/fsi-master-hub.c | 22 ++++++++++++++++++++++
>> drivers/fsi/fsi-master.h | 1 +
>> 4 files changed, 65 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);
> Do we want to set the link as disabled when the init fails in all
> fsi_slave_init error paths?
Yes, I think so. I don't see why that would need to be conditional on
anything? The link won't work so may as well disable it, right?
>
>> }
>>
>> return 0;
>> diff --git a/drivers/fsi/fsi-master-aspeed.c b/drivers/fsi/fsi-master-aspeed.c
>> index f49742b310c2..7ce5d9eb6c78 100644
>> --- a/drivers/fsi/fsi-master-aspeed.c
>> +++ b/drivers/fsi/fsi-master-aspeed.c
>> @@ -299,6 +299,35 @@ 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, ret;
>> + __be32 reg, result;
>> +
>> + idx = link / 32;
>> + bit = link % 32;
>> +
>> + reg = cpu_to_be32(0x80000000 >> bit);
>> +
>> + ret = opb_writel(aspeed, ctrl_base + FSI_MCENP0 + (4 * idx), reg);
>> + if (ret)
>> + return ret;
>> +
>> + mdelay(FSI_LINK_ENABLE_SETUP_TIME);
>> +
>> + ret = opb_readl(aspeed, ctrl_base + FSI_MENP0 + (4 * idx), &result);
>> + if (ret)
>> + return ret;
> Do we need to have the delay and read when disabling?
I can't find any documentation on a delay for disabling, so I guess not
actually.
>
>> +
>> + if (result & reg) {
>> + dev_err(aspeed->dev, "%s failed: %08x\n", __func__, result);
>> + return -EIO;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> static int aspeed_master_link_enable(struct fsi_master *master, int link)
>> {
>> struct fsi_master_aspeed *aspeed = to_fsi_master_aspeed(master);
>> @@ -491,6 +520,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..26617fd5e2de 100644
>> --- a/drivers/fsi/fsi-master-hub.c
>> +++ b/drivers/fsi/fsi-master-hub.c
>> @@ -77,6 +77,27 @@ 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;
>> + int rc;
>> +
>> + idx = link / 32;
>> + bit = link % 32;
>> +
>> + reg = cpu_to_be32(0x80000000 >> bit);
>> +
>> + rc = fsi_device_write(hub->upstream, FSI_MCENP0 + (4 * idx), ®, 4);
>> +
>> + mdelay(FSI_LINK_ENABLE_SETUP_TIME);
>> +
>> + fsi_device_read(hub->upstream, FSI_MENP0 + (4 * idx), ®, 4);
>> +
>> + return rc;
>> +}
> This is the same code, with fsi_device_read instead of obp_read.
>
> We should look to share this code between the aspeed master and the hub master.
So is hub_master_link_enable and aspeed_master_link_enable...
It's quite tricky because the writes to the Aspeed master regs are to a
different offset than like a master.write operation.
>
>> +
>> static int hub_master_link_enable(struct fsi_master *master, int link)
>> {
>> struct fsi_master_hub *hub = to_fsi_master_hub(master);
>> @@ -228,6 +249,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
>>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-02-25 16:24 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-02-17 20:16 [PATCH linux dev-5.4] fsi: master: Add link_disable function Eddie James
2020-02-25 2:01 ` Joel Stanley
2020-02-25 16:24 ` Eddie James
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.