| Message ID | 20220719210621.7895-2-asmaa@nvidia.com |
|---|---|
| State | New |
| Headers | show |
| Series | UBUNTU: SAUCE: i2c-mlxbf.c: support lock mechanism | expand |
On 7/19/22 15:06, Asmaa Mnebhi wrote: > Buglink: https://bugs.launchpad.net/bugs/1981105 > > Support the I2C lock mechanism, otherwise there could be > unexpected behavior when an i2c bus is accessed by > several entities like the linux driver, ATF driver and UEFI driver. > > Make sure to pick up the ATF/UEFI image to accompany this change > because at boot time ATF will ensure that the lock is released. > > Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com> > --- > drivers/i2c/busses/i2c-mlxbf.c | 38 ++++++++++++++++++++++++++++++---- > 1 file changed, 34 insertions(+), 4 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-mlxbf.c b/drivers/i2c/busses/i2c-mlxbf.c > index 0ac9e70244a7..fcb5bb587df1 100644 > --- a/drivers/i2c/busses/i2c-mlxbf.c > +++ b/drivers/i2c/busses/i2c-mlxbf.c > @@ -340,7 +340,8 @@ enum { > * Timeout is given in microsends. Note also that timeout handling is not > * exact. > */ > -#define SMBUS_TIMEOUT (300 * 1000) /* 300ms */ > +#define SMBUS_TIMEOUT (300 * 1000) /* 300ms */ > +#define SMBUS_LOCK_POLL_TIMEOUT (300 * 1000) /* 300ms */ > > /* Encapsulates timing parameters */ > struct mlx_i2c_timings { > @@ -573,6 +574,25 @@ static bool mlx_smbus_master_wait_for_idle(struct mlx_i2c_priv *priv) > return false; > } > > +/* > + * wait for the lock to be released before acquiring it. > + */ > +static bool mlx_smbus_master_lock(struct mlx_i2c_priv *priv) > +{ > + if (mlx_smbus_poll(priv->smbus->io, SMBUS_MASTER_GW, > + 1 << MASTER_LOCK_BIT_OFF, true, > + SMBUS_LOCK_POLL_TIMEOUT)) > + return true; > + > + return false; > +} > + > +static void mlx_smbus_master_unlock(struct mlx_i2c_priv *priv) > +{ > + /* Clear the gw to clear the lock */ > + writel(0, priv->smbus->io + SMBUS_MASTER_GW); > +} > + > /* > * Poll SMBus master status and return transaction status, > * i.e. whether succeeded or failed. I2C and SMBus fault codes > @@ -704,7 +724,7 @@ static int mlx_smbus_enable(struct mlx_i2c_priv *priv, u8 slave, > /* Clear status bits */ > smbus_write(priv->smbus->io, SMBUS_MASTER_STATUS, 0x0); > /* Set the cause data */ > - smbus_write(priv->smbus->io, I2C_CAUSE_OR_CLEAR_BITS, ~0x0); > + smbus_write(priv->mst_cause->io, I2C_CAUSE_OR_CLEAR_BITS, ~0x0); This looks like a bug fix unrelated to the locking changes. > /* Zero PEC byte */ > smbus_write(priv->smbus->io, SMBUS_MASTER_PEC, 0x0); > /* Zero byte count */ > @@ -744,7 +764,13 @@ static int mlx_smbus_start_transaction(struct mlx_i2c_priv *priv, > slave = request->slave & 0x7f; > addr = slave << 1; > > - /* First of all, check whether the HW is idle */ > + /* Try to acquire the smbus gw lock before any reads of the GW register since > + * a read sets the lock. > + */ > + if (WARN_ON(!mlx_smbus_master_lock(priv))) > + return -EBUSY; > + Does this function actually set the master lock bit ? It appears that it only polls the master lock bit until it returns 0 or 300 msec elapses. Does the HW have some unexplained behavior ? If so, then that is worthy of some explanation in the code. > + /* Check whether the HW is idle */ > if (WARN_ON(!mlx_smbus_master_wait_for_idle(priv))) > return -EBUSY; If the master lock bit is set, shouldn't it get released before returning an error ? > > @@ -802,8 +828,10 @@ static int mlx_smbus_start_transaction(struct mlx_i2c_priv *priv, > if (write_en) { > ret = mlx_smbus_enable(priv, slave, write_len, block_en, > pec_en, 0); > - if (ret != 0) > + if (ret) { > + mlx_smbus_master_unlock(priv); > return ret; > + } > } > > if (read_en) { > @@ -830,6 +858,8 @@ static int mlx_smbus_start_transaction(struct mlx_i2c_priv *priv, > SMBUS_MASTER_FSM_PS_STATE_MASK); > } > > + mlx_smbus_master_unlock(priv); > + > return ret; > } >
Hi Tim, PSB -----Original Message----- From: Tim Gardner <tim.gardner@canonical.com> Sent: Wednesday, July 20, 2022 8:49 AM To: Asmaa Mnebhi <asmaa@nvidia.com>; kernel-team@lists.ubuntu.com Subject: NAK/cmnt: [SRU][F:linux-bluefield][PATCH v2 1/1] UBUNTU: SAUCE: i2c-mlxbf.c: support lock mechanism On 7/19/22 15:06, Asmaa Mnebhi wrote: > Buglink: > https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs > .launchpad.net%2Fbugs%2F1981105&data=05%7C01%7Casmaa%40nvidia.com% > 7Ce65fe8237b4641f7092708da6a4e3fd3%7C43083d15727340c1b7db39efd9ccc17a% > 7C0%7C0%7C637939181559012282%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwM > DAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C& > sdata=x4g6zeDfDIoM2ZDVOeAbKqvx6yf6DMa50vc5UvRQhb0%3D&reserved=0 > > Support the I2C lock mechanism, otherwise there could be unexpected > behavior when an i2c bus is accessed by several entities like the > linux driver, ATF driver and UEFI driver. > > Make sure to pick up the ATF/UEFI image to accompany this change > because at boot time ATF will ensure that the lock is released. > > Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com> > --- > drivers/i2c/busses/i2c-mlxbf.c | 38 ++++++++++++++++++++++++++++++---- > 1 file changed, 34 insertions(+), 4 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-mlxbf.c > b/drivers/i2c/busses/i2c-mlxbf.c index 0ac9e70244a7..fcb5bb587df1 > 100644 > --- a/drivers/i2c/busses/i2c-mlxbf.c > +++ b/drivers/i2c/busses/i2c-mlxbf.c > @@ -340,7 +340,8 @@ enum { > * Timeout is given in microsends. Note also that timeout handling is not > * exact. > */ > -#define SMBUS_TIMEOUT (300 * 1000) /* 300ms */ > +#define SMBUS_TIMEOUT (300 * 1000) /* 300ms */ > +#define SMBUS_LOCK_POLL_TIMEOUT (300 * 1000) /* 300ms */ > > /* Encapsulates timing parameters */ > struct mlx_i2c_timings { > @@ -573,6 +574,25 @@ static bool mlx_smbus_master_wait_for_idle(struct mlx_i2c_priv *priv) > return false; > } > > +/* > + * wait for the lock to be released before acquiring it. > + */ > +static bool mlx_smbus_master_lock(struct mlx_i2c_priv *priv) { > + if (mlx_smbus_poll(priv->smbus->io, SMBUS_MASTER_GW, > + 1 << MASTER_LOCK_BIT_OFF, true, > + SMBUS_LOCK_POLL_TIMEOUT)) > + return true; > + > + return false; > +} > + > +static void mlx_smbus_master_unlock(struct mlx_i2c_priv *priv) { > + /* Clear the gw to clear the lock */ > + writel(0, priv->smbus->io + SMBUS_MASTER_GW); } > + > /* > * Poll SMBus master status and return transaction status, > * i.e. whether succeeded or failed. I2C and SMBus fault codes @@ > -704,7 +724,7 @@ static int mlx_smbus_enable(struct mlx_i2c_priv *priv, u8 slave, > /* Clear status bits */ > smbus_write(priv->smbus->io, SMBUS_MASTER_STATUS, 0x0); > /* Set the cause data */ > - smbus_write(priv->smbus->io, I2C_CAUSE_OR_CLEAR_BITS, ~0x0); > + smbus_write(priv->mst_cause->io, I2C_CAUSE_OR_CLEAR_BITS, ~0x0); This looks like a bug fix unrelated to the locking changes. > /* Zero PEC byte */ > smbus_write(priv->smbus->io, SMBUS_MASTER_PEC, 0x0); > /* Zero byte count */ > @@ -744,7 +764,13 @@ static int mlx_smbus_start_transaction(struct mlx_i2c_priv *priv, > slave = request->slave & 0x7f; > addr = slave << 1; > > - /* First of all, check whether the HW is idle */ > + /* Try to acquire the smbus gw lock before any reads of the GW register since > + * a read sets the lock. > + */ > + if (WARN_ON(!mlx_smbus_master_lock(priv))) > + return -EBUSY; > + Does this function actually set the master lock bit ? It appears that it only polls the master lock bit until it returns 0 or 300 msec elapses. Does the HW have some unexplained behavior ? If so, then that is worthy of some explanation in the code. Reading the gw reg sets the bit so yes. The comment is clear in my opinion: " a read sets the lock." > + /* Check whether the HW is idle */ > if (WARN_ON(!mlx_smbus_master_wait_for_idle(priv))) > return -EBUSY; If the master lock bit is set, shouldn't it get released before returning an error ? No. If the lock is set, that means some other software entity is using it. It goes against the sole purpose of this lock. The entity that acquired it should be the one releasing it. > > @@ -802,8 +828,10 @@ static int mlx_smbus_start_transaction(struct mlx_i2c_priv *priv, > if (write_en) { > ret = mlx_smbus_enable(priv, slave, write_len, block_en, > pec_en, 0); > - if (ret != 0) > + if (ret) { > + mlx_smbus_master_unlock(priv); > return ret; > + } > } > > if (read_en) { > @@ -830,6 +858,8 @@ static int mlx_smbus_start_transaction(struct mlx_i2c_priv *priv, > SMBUS_MASTER_FSM_PS_STATE_MASK); > } > > + mlx_smbus_master_unlock(priv); > + > return ret; > } > -- ----------- Tim Gardner Canonical, Inc
On 7/20/22 06:58, Asmaa Mnebhi wrote: > Hi Tim, > > PSB > > -----Original Message----- > From: Tim Gardner <tim.gardner@canonical.com> > Sent: Wednesday, July 20, 2022 8:49 AM > To: Asmaa Mnebhi <asmaa@nvidia.com>; kernel-team@lists.ubuntu.com > Subject: NAK/cmnt: [SRU][F:linux-bluefield][PATCH v2 1/1] UBUNTU: SAUCE: i2c-mlxbf.c: support lock mechanism > > On 7/19/22 15:06, Asmaa Mnebhi wrote: >> Buglink: >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbugs >> .launchpad.net%2Fbugs%2F1981105&data=05%7C01%7Casmaa%40nvidia.com% >> 7Ce65fe8237b4641f7092708da6a4e3fd3%7C43083d15727340c1b7db39efd9ccc17a% >> 7C0%7C0%7C637939181559012282%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwM >> DAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C& >> sdata=x4g6zeDfDIoM2ZDVOeAbKqvx6yf6DMa50vc5UvRQhb0%3D&reserved=0 >> >> Support the I2C lock mechanism, otherwise there could be unexpected >> behavior when an i2c bus is accessed by several entities like the >> linux driver, ATF driver and UEFI driver. >> >> Make sure to pick up the ATF/UEFI image to accompany this change >> because at boot time ATF will ensure that the lock is released. >> >> Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com> >> --- >> drivers/i2c/busses/i2c-mlxbf.c | 38 ++++++++++++++++++++++++++++++---- >> 1 file changed, 34 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-mlxbf.c >> b/drivers/i2c/busses/i2c-mlxbf.c index 0ac9e70244a7..fcb5bb587df1 >> 100644 >> --- a/drivers/i2c/busses/i2c-mlxbf.c >> +++ b/drivers/i2c/busses/i2c-mlxbf.c >> @@ -340,7 +340,8 @@ enum { >> * Timeout is given in microsends. Note also that timeout handling is not >> * exact. >> */ >> -#define SMBUS_TIMEOUT (300 * 1000) /* 300ms */ >> +#define SMBUS_TIMEOUT (300 * 1000) /* 300ms */ >> +#define SMBUS_LOCK_POLL_TIMEOUT (300 * 1000) /* 300ms */ >> >> /* Encapsulates timing parameters */ >> struct mlx_i2c_timings { >> @@ -573,6 +574,25 @@ static bool mlx_smbus_master_wait_for_idle(struct mlx_i2c_priv *priv) >> return false; >> } >> >> +/* >> + * wait for the lock to be released before acquiring it. >> + */ >> +static bool mlx_smbus_master_lock(struct mlx_i2c_priv *priv) { >> + if (mlx_smbus_poll(priv->smbus->io, SMBUS_MASTER_GW, >> + 1 << MASTER_LOCK_BIT_OFF, true, >> + SMBUS_LOCK_POLL_TIMEOUT)) >> + return true; >> + >> + return false; >> +} >> + >> +static void mlx_smbus_master_unlock(struct mlx_i2c_priv *priv) { >> + /* Clear the gw to clear the lock */ >> + writel(0, priv->smbus->io + SMBUS_MASTER_GW); } >> + >> /* >> * Poll SMBus master status and return transaction status, >> * i.e. whether succeeded or failed. I2C and SMBus fault codes @@ >> -704,7 +724,7 @@ static int mlx_smbus_enable(struct mlx_i2c_priv *priv, u8 slave, >> /* Clear status bits */ >> smbus_write(priv->smbus->io, SMBUS_MASTER_STATUS, 0x0); >> /* Set the cause data */ >> - smbus_write(priv->smbus->io, I2C_CAUSE_OR_CLEAR_BITS, ~0x0); >> + smbus_write(priv->mst_cause->io, I2C_CAUSE_OR_CLEAR_BITS, ~0x0); > > This looks like a bug fix unrelated to the locking changes. > >> /* Zero PEC byte */ >> smbus_write(priv->smbus->io, SMBUS_MASTER_PEC, 0x0); >> /* Zero byte count */ >> @@ -744,7 +764,13 @@ static int mlx_smbus_start_transaction(struct mlx_i2c_priv *priv, >> slave = request->slave & 0x7f; >> addr = slave << 1; >> >> - /* First of all, check whether the HW is idle */ >> + /* Try to acquire the smbus gw lock before any reads of the GW register since >> + * a read sets the lock. >> + */ >> + if (WARN_ON(!mlx_smbus_master_lock(priv))) >> + return -EBUSY; >> + > > Does this function actually set the master lock bit ? It appears that it only polls the master lock bit until it returns 0 or 300 msec elapses. > Does the HW have some unexplained behavior ? If so, then that is worthy of some explanation in the code. > > Reading the gw reg sets the bit so yes. The comment is clear in my opinion: " a read sets the lock." Seems like that comment belongs in the mlx_smbus_master_lock() function, but whatever. > >> + /* Check whether the HW is idle */ >> if (WARN_ON(!mlx_smbus_master_wait_for_idle(priv))) >> return -EBUSY; > > If the master lock bit is set, shouldn't it get released before returning an error ? > > No. If the lock is set, that means some other software entity is using it. It goes against the sole purpose of this lock. The entity that acquired it should be the one releasing it. > If the thread has gotten this far, then isn't it the entity that set the lock ? A master_wait_for_idle() failure at this point kind of implies a HW issue. >> >> @@ -802,8 +828,10 @@ static int mlx_smbus_start_transaction(struct mlx_i2c_priv *priv, >> if (write_en) { >> ret = mlx_smbus_enable(priv, slave, write_len, block_en, >> pec_en, 0); >> - if (ret != 0) >> + if (ret) { >> + mlx_smbus_master_unlock(priv); >> return ret; >> + } >> } >> >> if (read_en) { >> @@ -830,6 +858,8 @@ static int mlx_smbus_start_transaction(struct mlx_i2c_priv *priv, >> SMBUS_MASTER_FSM_PS_STATE_MASK); >> } >> >> + mlx_smbus_master_unlock(priv); >> + >> return ret; >> } >> > > > -- > ----------- > Tim Gardner > Canonical, Inc
-----Original Message----- From: Tim Gardner <tim.gardner@canonical.com> Sent: Wednesday, July 20, 2022 9:19 AM To: Asmaa Mnebhi <asmaa@nvidia.com>; kernel-team@lists.ubuntu.com Subject: Re: NAK/cmnt: [SRU][F:linux-bluefield][PATCH v2 1/1] UBUNTU: SAUCE: i2c-mlxbf.c: support lock mechanism On 7/20/22 06:58, Asmaa Mnebhi wrote: > Hi Tim, > > PSB > > -----Original Message----- > From: Tim Gardner <tim.gardner@canonical.com> > Sent: Wednesday, July 20, 2022 8:49 AM > To: Asmaa Mnebhi <asmaa@nvidia.com>; kernel-team@lists.ubuntu.com > Subject: NAK/cmnt: [SRU][F:linux-bluefield][PATCH v2 1/1] UBUNTU: > SAUCE: i2c-mlxbf.c: support lock mechanism > > On 7/19/22 15:06, Asmaa Mnebhi wrote: >> Buglink: >> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Fbug >> s >> .launchpad.net%2Fbugs%2F1981105&data=05%7C01%7Casmaa%40nvidia.com >> % >> 7Ce65fe8237b4641f7092708da6a4e3fd3%7C43083d15727340c1b7db39efd9ccc17a >> % >> 7C0%7C0%7C637939181559012282%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAw >> M >> DAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C& >> ; >> sdata=x4g6zeDfDIoM2ZDVOeAbKqvx6yf6DMa50vc5UvRQhb0%3D&reserved=0 >> >> Support the I2C lock mechanism, otherwise there could be unexpected >> behavior when an i2c bus is accessed by several entities like the >> linux driver, ATF driver and UEFI driver. >> >> Make sure to pick up the ATF/UEFI image to accompany this change >> because at boot time ATF will ensure that the lock is released. >> >> Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com> >> --- >> drivers/i2c/busses/i2c-mlxbf.c | 38 ++++++++++++++++++++++++++++++---- >> 1 file changed, 34 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-mlxbf.c >> b/drivers/i2c/busses/i2c-mlxbf.c index 0ac9e70244a7..fcb5bb587df1 >> 100644 >> --- a/drivers/i2c/busses/i2c-mlxbf.c >> +++ b/drivers/i2c/busses/i2c-mlxbf.c >> @@ -340,7 +340,8 @@ enum { >> * Timeout is given in microsends. Note also that timeout handling is not >> * exact. >> */ >> -#define SMBUS_TIMEOUT (300 * 1000) /* 300ms */ >> +#define SMBUS_TIMEOUT (300 * 1000) /* 300ms */ >> +#define SMBUS_LOCK_POLL_TIMEOUT (300 * 1000) /* 300ms */ >> >> /* Encapsulates timing parameters */ >> struct mlx_i2c_timings { >> @@ -573,6 +574,25 @@ static bool mlx_smbus_master_wait_for_idle(struct mlx_i2c_priv *priv) >> return false; >> } >> >> +/* >> + * wait for the lock to be released before acquiring it. >> + */ >> +static bool mlx_smbus_master_lock(struct mlx_i2c_priv *priv) { >> + if (mlx_smbus_poll(priv->smbus->io, SMBUS_MASTER_GW, >> + 1 << MASTER_LOCK_BIT_OFF, true, >> + SMBUS_LOCK_POLL_TIMEOUT)) >> + return true; >> + >> + return false; >> +} >> + >> +static void mlx_smbus_master_unlock(struct mlx_i2c_priv *priv) { >> + /* Clear the gw to clear the lock */ >> + writel(0, priv->smbus->io + SMBUS_MASTER_GW); } >> + >> /* >> * Poll SMBus master status and return transaction status, >> * i.e. whether succeeded or failed. I2C and SMBus fault codes @@ >> -704,7 +724,7 @@ static int mlx_smbus_enable(struct mlx_i2c_priv *priv, u8 slave, >> /* Clear status bits */ >> smbus_write(priv->smbus->io, SMBUS_MASTER_STATUS, 0x0); >> /* Set the cause data */ >> - smbus_write(priv->smbus->io, I2C_CAUSE_OR_CLEAR_BITS, ~0x0); >> + smbus_write(priv->mst_cause->io, I2C_CAUSE_OR_CLEAR_BITS, ~0x0); > > This looks like a bug fix unrelated to the locking changes. > >> /* Zero PEC byte */ >> smbus_write(priv->smbus->io, SMBUS_MASTER_PEC, 0x0); >> /* Zero byte count */ >> @@ -744,7 +764,13 @@ static int mlx_smbus_start_transaction(struct mlx_i2c_priv *priv, >> slave = request->slave & 0x7f; >> addr = slave << 1; >> >> - /* First of all, check whether the HW is idle */ >> + /* Try to acquire the smbus gw lock before any reads of the GW register since >> + * a read sets the lock. >> + */ >> + if (WARN_ON(!mlx_smbus_master_lock(priv))) >> + return -EBUSY; >> + > > Does this function actually set the master lock bit ? It appears that it only polls the master lock bit until it returns 0 or 300 msec elapses. > Does the HW have some unexplained behavior ? If so, then that is worthy of some explanation in the code. > > Reading the gw reg sets the bit so yes. The comment is clear in my opinion: " a read sets the lock." Seems like that comment belongs in the mlx_smbus_master_lock() function, but whatever. > >> + /* Check whether the HW is idle */ >> if (WARN_ON(!mlx_smbus_master_wait_for_idle(priv))) >> return -EBUSY; > > If the master lock bit is set, shouldn't it get released before returning an error ? > > No. If the lock is set, that means some other software entity is using it. It goes against the sole purpose of this lock. The entity that acquired it should be the one releasing it. > If the thread has gotten this far, then isn't it the entity that set the lock ? A master_wait_for_idle() failure at this point kind of implies a HW issue. Oh apologies, I misunderstood your question. You are right, we should be releasing the lock here. Thanks! >> >> @@ -802,8 +828,10 @@ static int mlx_smbus_start_transaction(struct mlx_i2c_priv *priv, >> if (write_en) { >> ret = mlx_smbus_enable(priv, slave, write_len, block_en, >> pec_en, 0); >> - if (ret != 0) >> + if (ret) { >> + mlx_smbus_master_unlock(priv); >> return ret; >> + } >> } >> >> if (read_en) { >> @@ -830,6 +858,8 @@ static int mlx_smbus_start_transaction(struct mlx_i2c_priv *priv, >> SMBUS_MASTER_FSM_PS_STATE_MASK); >> } >> >> + mlx_smbus_master_unlock(priv); >> + >> return ret; >> } >> > > > -- > ----------- > Tim Gardner > Canonical, Inc -- ----------- Tim Gardner Canonical, Inc
diff --git a/drivers/i2c/busses/i2c-mlxbf.c b/drivers/i2c/busses/i2c-mlxbf.c index 0ac9e70244a7..fcb5bb587df1 100644 --- a/drivers/i2c/busses/i2c-mlxbf.c +++ b/drivers/i2c/busses/i2c-mlxbf.c @@ -340,7 +340,8 @@ enum { * Timeout is given in microsends. Note also that timeout handling is not * exact. */ -#define SMBUS_TIMEOUT (300 * 1000) /* 300ms */ +#define SMBUS_TIMEOUT (300 * 1000) /* 300ms */ +#define SMBUS_LOCK_POLL_TIMEOUT (300 * 1000) /* 300ms */ /* Encapsulates timing parameters */ struct mlx_i2c_timings { @@ -573,6 +574,25 @@ static bool mlx_smbus_master_wait_for_idle(struct mlx_i2c_priv *priv) return false; } +/* + * wait for the lock to be released before acquiring it. + */ +static bool mlx_smbus_master_lock(struct mlx_i2c_priv *priv) +{ + if (mlx_smbus_poll(priv->smbus->io, SMBUS_MASTER_GW, + 1 << MASTER_LOCK_BIT_OFF, true, + SMBUS_LOCK_POLL_TIMEOUT)) + return true; + + return false; +} + +static void mlx_smbus_master_unlock(struct mlx_i2c_priv *priv) +{ + /* Clear the gw to clear the lock */ + writel(0, priv->smbus->io + SMBUS_MASTER_GW); +} + /* * Poll SMBus master status and return transaction status, * i.e. whether succeeded or failed. I2C and SMBus fault codes @@ -704,7 +724,7 @@ static int mlx_smbus_enable(struct mlx_i2c_priv *priv, u8 slave, /* Clear status bits */ smbus_write(priv->smbus->io, SMBUS_MASTER_STATUS, 0x0); /* Set the cause data */ - smbus_write(priv->smbus->io, I2C_CAUSE_OR_CLEAR_BITS, ~0x0); + smbus_write(priv->mst_cause->io, I2C_CAUSE_OR_CLEAR_BITS, ~0x0); /* Zero PEC byte */ smbus_write(priv->smbus->io, SMBUS_MASTER_PEC, 0x0); /* Zero byte count */ @@ -744,7 +764,13 @@ static int mlx_smbus_start_transaction(struct mlx_i2c_priv *priv, slave = request->slave & 0x7f; addr = slave << 1; - /* First of all, check whether the HW is idle */ + /* Try to acquire the smbus gw lock before any reads of the GW register since + * a read sets the lock. + */ + if (WARN_ON(!mlx_smbus_master_lock(priv))) + return -EBUSY; + + /* Check whether the HW is idle */ if (WARN_ON(!mlx_smbus_master_wait_for_idle(priv))) return -EBUSY; @@ -802,8 +828,10 @@ static int mlx_smbus_start_transaction(struct mlx_i2c_priv *priv, if (write_en) { ret = mlx_smbus_enable(priv, slave, write_len, block_en, pec_en, 0); - if (ret != 0) + if (ret) { + mlx_smbus_master_unlock(priv); return ret; + } } if (read_en) { @@ -830,6 +858,8 @@ static int mlx_smbus_start_transaction(struct mlx_i2c_priv *priv, SMBUS_MASTER_FSM_PS_STATE_MASK); } + mlx_smbus_master_unlock(priv); + return ret; }
Buglink: https://bugs.launchpad.net/bugs/1981105 Support the I2C lock mechanism, otherwise there could be unexpected behavior when an i2c bus is accessed by several entities like the linux driver, ATF driver and UEFI driver. Make sure to pick up the ATF/UEFI image to accompany this change because at boot time ATF will ensure that the lock is released. Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com> --- drivers/i2c/busses/i2c-mlxbf.c | 38 ++++++++++++++++++++++++++++++---- 1 file changed, 34 insertions(+), 4 deletions(-)