| Message ID | 20180428135607.22979-3-amonakov@ispras.ru |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [v2,1/2] i2c: designware: fix poll-after-enable regression | expand |
On 04/28/2018 04:56 PM, Alexander Monakov wrote: > Low-level controller enable function __i2c_dw_enable is overloaded to > also handle disabling. What's worse, even though the documentation > requires polling the IC_ENABLE_STATUS register when disabling, this > is not done: polling needs to be requested specifically by calling > __i2c_dw_enable_and_wait, which can also poll on enabling, but that > doesn't work if the IC_ENABLE_STATUS register is not implemented. > This is quite confusing if not in fact backwards. > > Especially since the documentation says that disabling should be > followed by polling, the driver should be using a separate function > where it does one-shot disables to make the optimization stand out. > > This refactors the two functions so that requested status is given > in the name rather than in a boolean argument. Specifically: > > - __i2c_dw_enable: enable without polling (in accordance with docs) > - __i2c_dw_disable: disable and do poll (also as suggested by docs) > - __i2c_dw_disable_nowait: disable without polling (Linux-specific) > > No functional change. > > Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com> > Cc: Wolfram Sang <wsa@the-dreams.de> > Signed-off-by: Alexander Monakov <amonakov@ispras.ru> > --- > drivers/i2c/busses/i2c-designware-common.c | 20 +++++++++----------- > drivers/i2c/busses/i2c-designware-core.h | 13 +++++++++++-- > drivers/i2c/busses/i2c-designware-master.c | 8 ++++---- > drivers/i2c/busses/i2c-designware-slave.c | 6 +++--- > 4 files changed, 27 insertions(+), 20 deletions(-) > I'm neutral with this change. Acked-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
On Sat, Apr 28, 2018 at 04:56:07PM +0300, Alexander Monakov wrote: > Low-level controller enable function __i2c_dw_enable is overloaded to > also handle disabling. What's worse, even though the documentation > requires polling the IC_ENABLE_STATUS register when disabling, this > is not done: polling needs to be requested specifically by calling > __i2c_dw_enable_and_wait, which can also poll on enabling, but that > doesn't work if the IC_ENABLE_STATUS register is not implemented. > This is quite confusing if not in fact backwards. > > Especially since the documentation says that disabling should be > followed by polling, the driver should be using a separate function > where it does one-shot disables to make the optimization stand out. > > This refactors the two functions so that requested status is given > in the name rather than in a boolean argument. Specifically: > > - __i2c_dw_enable: enable without polling (in accordance with docs) > - __i2c_dw_disable: disable and do poll (also as suggested by docs) > - __i2c_dw_disable_nowait: disable without polling (Linux-specific) > > No functional change. > > Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com> > Cc: Wolfram Sang <wsa@the-dreams.de> > Signed-off-by: Alexander Monakov <amonakov@ispras.ru> I like the explicit enable/disable, so applied to for-next, thanks! I improved the blank lines in this section, though (checkpatch --strict warned about it): > +static inline void __i2c_dw_enable(struct dw_i2c_dev *dev) > +{ > + dw_writel(dev, 1, DW_IC_ENABLE); > +} > +static inline void __i2c_dw_disable_nowait(struct dw_i2c_dev *dev) > +{ > + dw_writel(dev, 0, DW_IC_ENABLE); > +} > +void __i2c_dw_disable(struct dw_i2c_dev *dev); > + > +
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c index 27ebd90de43b..48914dfc8ce8 100644 --- a/drivers/i2c/busses/i2c-designware-common.c +++ b/drivers/i2c/busses/i2c-designware-common.c @@ -149,18 +149,17 @@ u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset) return ((ic_clk * (tLOW + tf) + 500000) / 1000000) - 1 + offset; } -void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable) -{ - dw_writel(dev, enable, DW_IC_ENABLE); -} - -void __i2c_dw_enable_and_wait(struct dw_i2c_dev *dev, bool enable) +void __i2c_dw_disable(struct dw_i2c_dev *dev) { int timeout = 100; do { - __i2c_dw_enable(dev, enable); - if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable) + __i2c_dw_disable_nowait(dev); + /* + * The enable status register may be unimplemented, but + * in that case this test reads zero and exits the loop. + */ + if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == 0) return; /* @@ -171,8 +170,7 @@ void __i2c_dw_enable_and_wait(struct dw_i2c_dev *dev, bool enable) usleep_range(25, 250); } while (timeout--); - dev_warn(dev->dev, "timeout in %sabling adapter\n", - enable ? "en" : "dis"); + dev_warn(dev->dev, "timeout in disabling adapter\n"); } unsigned long i2c_dw_clk_rate(struct dw_i2c_dev *dev) @@ -277,7 +275,7 @@ u32 i2c_dw_func(struct i2c_adapter *adap) void i2c_dw_disable(struct dw_i2c_dev *dev) { /* Disable controller */ - __i2c_dw_enable_and_wait(dev, false); + __i2c_dw_disable(dev); /* Disable all interupts */ dw_writel(dev, 0, DW_IC_INTR_MASK); diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index 8707c76b2fee..6c61ac85c936 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -297,8 +297,6 @@ u32 dw_readl(struct dw_i2c_dev *dev, int offset); void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset); u32 i2c_dw_scl_hcnt(u32 ic_clk, u32 tSYMBOL, u32 tf, int cond, int offset); u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset); -void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable); -void __i2c_dw_enable_and_wait(struct dw_i2c_dev *dev, bool enable); unsigned long i2c_dw_clk_rate(struct dw_i2c_dev *dev); int i2c_dw_prepare_clk(struct dw_i2c_dev *dev, bool prepare); int i2c_dw_acquire_lock(struct dw_i2c_dev *dev); @@ -309,6 +307,17 @@ u32 i2c_dw_func(struct i2c_adapter *adap); void i2c_dw_disable(struct dw_i2c_dev *dev); void i2c_dw_disable_int(struct dw_i2c_dev *dev); +static inline void __i2c_dw_enable(struct dw_i2c_dev *dev) +{ + dw_writel(dev, 1, DW_IC_ENABLE); +} +static inline void __i2c_dw_disable_nowait(struct dw_i2c_dev *dev) +{ + dw_writel(dev, 0, DW_IC_ENABLE); +} +void __i2c_dw_disable(struct dw_i2c_dev *dev); + + extern u32 i2c_dw_read_comp_param(struct dw_i2c_dev *dev); extern int i2c_dw_probe(struct dw_i2c_dev *dev); #if IS_ENABLED(CONFIG_I2C_DESIGNWARE_SLAVE) diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c index 0cdba29ae0a9..27436a937492 100644 --- a/drivers/i2c/busses/i2c-designware-master.c +++ b/drivers/i2c/busses/i2c-designware-master.c @@ -81,7 +81,7 @@ static int i2c_dw_init_master(struct dw_i2c_dev *dev) comp_param1 = dw_readl(dev, DW_IC_COMP_PARAM_1); /* Disable the adapter */ - __i2c_dw_enable_and_wait(dev, false); + __i2c_dw_disable(dev); /* Set standard and fast speed deviders for high/low periods */ @@ -180,7 +180,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev) u32 ic_con, ic_tar = 0; /* Disable the adapter */ - __i2c_dw_enable_and_wait(dev, false); + __i2c_dw_disable(dev); /* If the slave address is ten bit address, enable 10BITADDR */ ic_con = dw_readl(dev, DW_IC_CON); @@ -209,7 +209,7 @@ static void i2c_dw_xfer_init(struct dw_i2c_dev *dev) i2c_dw_disable_int(dev); /* Enable the adapter */ - __i2c_dw_enable(dev, true); + __i2c_dw_enable(dev); /* Dummy read to avoid the register getting stuck on Bay Trail */ dw_readl(dev, DW_IC_ENABLE_STATUS); @@ -462,7 +462,7 @@ i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num) * additional interrupts are a hardware bug or this driver doesn't * handle them correctly yet. */ - __i2c_dw_enable(dev, false); + __i2c_dw_disable_nowait(dev); if (dev->msg_err) { ret = dev->msg_err; diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c index d42558d1b002..8ce2cd368477 100644 --- a/drivers/i2c/busses/i2c-designware-slave.c +++ b/drivers/i2c/busses/i2c-designware-slave.c @@ -75,7 +75,7 @@ static int i2c_dw_init_slave(struct dw_i2c_dev *dev) comp_param1 = dw_readl(dev, DW_IC_COMP_PARAM_1); /* Disable the adapter. */ - __i2c_dw_enable_and_wait(dev, false); + __i2c_dw_disable(dev); /* Configure SDA Hold Time if required. */ reg = dw_readl(dev, DW_IC_COMP_VERSION); @@ -119,11 +119,11 @@ static int i2c_dw_reg_slave(struct i2c_client *slave) * Set slave address in the IC_SAR register, * the address to which the DW_apb_i2c responds. */ - __i2c_dw_enable(dev, false); + __i2c_dw_disable_nowait(dev); dw_writel(dev, slave->addr, DW_IC_SAR); dev->slave = slave; - __i2c_dw_enable(dev, true); + __i2c_dw_enable(dev); dev->cmd_err = 0; dev->msg_write_idx = 0;
Low-level controller enable function __i2c_dw_enable is overloaded to also handle disabling. What's worse, even though the documentation requires polling the IC_ENABLE_STATUS register when disabling, this is not done: polling needs to be requested specifically by calling __i2c_dw_enable_and_wait, which can also poll on enabling, but that doesn't work if the IC_ENABLE_STATUS register is not implemented. This is quite confusing if not in fact backwards. Especially since the documentation says that disabling should be followed by polling, the driver should be using a separate function where it does one-shot disables to make the optimization stand out. This refactors the two functions so that requested status is given in the name rather than in a boolean argument. Specifically: - __i2c_dw_enable: enable without polling (in accordance with docs) - __i2c_dw_disable: disable and do poll (also as suggested by docs) - __i2c_dw_disable_nowait: disable without polling (Linux-specific) No functional change. Cc: Jarkko Nikula <jarkko.nikula@linux.intel.com> Cc: Wolfram Sang <wsa@the-dreams.de> Signed-off-by: Alexander Monakov <amonakov@ispras.ru> --- drivers/i2c/busses/i2c-designware-common.c | 20 +++++++++----------- drivers/i2c/busses/i2c-designware-core.h | 13 +++++++++++-- drivers/i2c/busses/i2c-designware-master.c | 8 ++++---- drivers/i2c/busses/i2c-designware-slave.c | 6 +++--- 4 files changed, 27 insertions(+), 20 deletions(-)