| Message ID | 20200217201619.16713-1-eajames@linux.ibm.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | [linux,dev-5.4] fsi: master: Add link_disable function | expand |
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 >
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 >>
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);
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(-)