[PATCH linux dev-5.4] fsi: master: Add link_disable function
Joel Stanley
joel at jms.id.au
Tue Feb 25 13:01:27 AEDT 2020
On Mon, 17 Feb 2020 at 20:16, Eddie James <eajames at 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 at 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
>
More information about the openbmc
mailing list