[PATCH linux dev-5.4] fsi: master: Add link_disable function
Eddie James
eajames at linux.ibm.com
Wed Feb 26 03:24:07 AEDT 2020
On 2/24/20 8:01 PM, Joel Stanley wrote:
> 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?
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
>>
More information about the openbmc
mailing list