| Message ID | 20180601134725.1375-5-jarkko.nikula@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: designware: Improve debug prints | expand |
On Fri, 2018-06-01 at 16:47 +0300, Jarkko Nikula wrote: > SDA hold time configuration is common to both master and slave code. > It > is also something that can be done once during probe and do only > register write when HW needs to be reinitialized. > > Remove duplication and move SDA hold time configuration to common > code. > It will be called from slave probe and for master code from a new > i2c_dw_set_timings_master() to where we will populate more probe time > timing parameter setting. > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> (Some minor issues / questions below) > Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> > --- > drivers/i2c/busses/i2c-designware-common.c | 35 > ++++++++++++++++++++++ > drivers/i2c/busses/i2c-designware-core.h | 1 + > drivers/i2c/busses/i2c-designware-master.c | 30 ++++++------------- > drivers/i2c/busses/i2c-designware-slave.c | 24 ++------------- > 4 files changed, 48 insertions(+), 42 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-designware-common.c > b/drivers/i2c/busses/i2c-designware-common.c > index c22654cce1df..1b542d3e5a2e 100644 > --- a/drivers/i2c/busses/i2c-designware-common.c > +++ b/drivers/i2c/busses/i2c-designware-common.c > @@ -183,6 +183,41 @@ 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_set_sda_hold(struct dw_i2c_dev *dev) > +{ > + u32 reg; > + int ret = 0; Redundant assignment. > + > + ret = i2c_dw_acquire_lock(dev); > + if (ret) > + return; > + > + /* Configure SDA Hold Time if required */ > + reg = dw_readl(dev, DW_IC_COMP_VERSION); > + if (reg >= DW_IC_SDA_HOLD_MIN_VERS) { > + if (!dev->sda_hold_time) { > + /* Keep previous hold time setting if no one > set it */ > + dev->sda_hold_time = dw_readl(dev, > DW_IC_SDA_HOLD); > + } > + > + /* > + * Workaround for avoiding TX arbitration lost in > case I2C > + * slave pulls SDA down "too quickly" after falling > egde of > + * SCL by enabling non-zero SDA RX hold. > Specification says it > + * extends incoming SDA low to high transition while > SCL is > + * high but it apprears to help also above issue. > + */ > + if (!(dev->sda_hold_time & DW_IC_SDA_HOLD_RX_MASK)) > + dev->sda_hold_time |= 1 << > DW_IC_SDA_HOLD_RX_SHIFT; > + } else if (dev->sda_hold_time) { > + dev_warn(dev->dev, > + "Hardware too old to adjust SDA hold > time.\n"); > + dev->sda_hold_time = 0; > + } > + > + i2c_dw_release_lock(dev); > +} > + > void __i2c_dw_disable(struct dw_i2c_dev *dev) > { > int timeout = 100; > diff --git a/drivers/i2c/busses/i2c-designware-core.h > b/drivers/i2c/busses/i2c-designware-core.h > index 5d54f70710ad..831b3d62b32c 100644 > --- a/drivers/i2c/busses/i2c-designware-core.h > +++ b/drivers/i2c/busses/i2c-designware-core.h > @@ -298,6 +298,7 @@ void dw_writel(struct dw_i2c_dev *dev, u32 b, int > offset); > int i2c_dw_set_reg_access(struct dw_i2c_dev *dev); > 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_set_sda_hold(struct dw_i2c_dev *dev); > 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); > diff --git a/drivers/i2c/busses/i2c-designware-master.c > b/drivers/i2c/busses/i2c-designware-master.c > index 3a7c184f24c8..b89bc8a5b0b1 100644 > --- a/drivers/i2c/busses/i2c-designware-master.c > +++ b/drivers/i2c/busses/i2c-designware-master.c > @@ -45,6 +45,11 @@ static void i2c_dw_configure_fifo_master(struct > dw_i2c_dev *dev) > dw_writel(dev, dev->master_cfg, DW_IC_CON); > } > > +static void i2c_dw_set_timings_master(struct dw_i2c_dev *dev) > +{ > + i2c_dw_set_sda_hold(dev); > +} > + Looking into patch 6 I'm not sure this split is required here and not there. Have you checked which one looks better? > /** > * i2c_dw_init() - Initialize the designware I2C master hardware > * @dev: device private data > @@ -56,7 +61,7 @@ static void i2c_dw_configure_fifo_master(struct > dw_i2c_dev *dev) > static int i2c_dw_init_master(struct dw_i2c_dev *dev) > { > u32 hcnt, lcnt; > - u32 reg, comp_param1; > + u32 comp_param1; > u32 sda_falling_time, scl_falling_time; > int ret; > > @@ -132,27 +137,9 @@ static int i2c_dw_init_master(struct dw_i2c_dev > *dev) > } > } > > - /* Configure SDA Hold Time if required */ > - reg = dw_readl(dev, DW_IC_COMP_VERSION); > - if (reg >= DW_IC_SDA_HOLD_MIN_VERS) { > - if (!dev->sda_hold_time) { > - /* Keep previous hold time setting if no one > set it */ > - dev->sda_hold_time = dw_readl(dev, > DW_IC_SDA_HOLD); > - } > - /* > - * Workaround for avoiding TX arbitration lost in > case I2C > - * slave pulls SDA down "too quickly" after falling > egde of > - * SCL by enabling non-zero SDA RX hold. > Specification says it > - * extends incoming SDA low to high transition while > SCL is > - * high but it apprears to help also above issue. > - */ > - if (!(dev->sda_hold_time & DW_IC_SDA_HOLD_RX_MASK)) > - dev->sda_hold_time |= 1 << > DW_IC_SDA_HOLD_RX_SHIFT; > + /* Write SDA hold time if supported */ > + if (dev->sda_hold_time) > dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD); > - } else if (dev->sda_hold_time) { > - dev_warn(dev->dev, > - "Hardware too old to adjust SDA hold > time.\n"); > - } > > i2c_dw_configure_fifo_master(dev); > i2c_dw_release_lock(dev); > @@ -671,6 +658,7 @@ int i2c_dw_probe(struct dw_i2c_dev *dev) > if (ret) > return ret; > > + i2c_dw_set_timings_master(dev); > ret = dev->init(dev); > if (ret) > return ret; > diff --git a/drivers/i2c/busses/i2c-designware-slave.c > b/drivers/i2c/busses/i2c-designware-slave.c > index a1f802001e1f..6ba265db4eb0 100644 > --- a/drivers/i2c/busses/i2c-designware-slave.c > +++ b/drivers/i2c/busses/i2c-designware-slave.c > @@ -51,7 +51,6 @@ static void i2c_dw_configure_fifo_slave(struct > dw_i2c_dev *dev) > */ > static int i2c_dw_init_slave(struct dw_i2c_dev *dev) > { > - u32 reg; > int ret; > > ret = i2c_dw_acquire_lock(dev); > @@ -61,27 +60,9 @@ static int i2c_dw_init_slave(struct dw_i2c_dev > *dev) > /* Disable the adapter. */ > __i2c_dw_disable(dev); > > - /* Configure SDA Hold Time if required. */ > - reg = dw_readl(dev, DW_IC_COMP_VERSION); > - if (reg >= DW_IC_SDA_HOLD_MIN_VERS) { > - if (!dev->sda_hold_time) { > - /* Keep previous hold time setting if no one > set it. */ > - dev->sda_hold_time = dw_readl(dev, > DW_IC_SDA_HOLD); > - } > - /* > - * Workaround for avoiding TX arbitration lost in > case I2C > - * slave pulls SDA down "too quickly" after falling > egde of > - * SCL by enabling non-zero SDA RX hold. > Specification says it > - * extends incoming SDA low to high transition while > SCL is > - * high but it apprears to help also above issue. > - */ > - if (!(dev->sda_hold_time & DW_IC_SDA_HOLD_RX_MASK)) > - dev->sda_hold_time |= 1 << > DW_IC_SDA_HOLD_RX_SHIFT; > + /* Write SDA hold time if supported */ > + if (dev->sda_hold_time) > dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD); > - } else { > - dev_warn(dev->dev, > - "Hardware too old to adjust SDA hold > time.\n"); > - } > > i2c_dw_configure_fifo_slave(dev); > i2c_dw_release_lock(dev); > @@ -287,6 +268,7 @@ int i2c_dw_probe_slave(struct dw_i2c_dev *dev) > if (ret) > return ret; > > + i2c_dw_set_sda_hold(dev); > ret = dev->init(dev); > if (ret) > return ret;
On 06/04/2018 03:42 PM, Andy Shevchenko wrote: > On Fri, 2018-06-01 at 16:47 +0300, Jarkko Nikula wrote: >> SDA hold time configuration is common to both master and slave code. >> It >> is also something that can be done once during probe and do only >> register write when HW needs to be reinitialized. >> >> Remove duplication and move SDA hold time configuration to common >> code. >> It will be called from slave probe and for master code from a new >> i2c_dw_set_timings_master() to where we will populate more probe time >> timing parameter setting. >> > > Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com> > > (Some minor issues / questions below) > ... >> --- a/drivers/i2c/busses/i2c-designware-master.c >> +++ b/drivers/i2c/busses/i2c-designware-master.c >> @@ -45,6 +45,11 @@ static void i2c_dw_configure_fifo_master(struct >> dw_i2c_dev *dev) >> dw_writel(dev, dev->master_cfg, DW_IC_CON); >> } >> >> +static void i2c_dw_set_timings_master(struct dw_i2c_dev *dev) >> +{ >> + i2c_dw_set_sda_hold(dev); >> +} >> + > > Looking into patch 6 I'm not sure this split is required here and not > there. > > Have you checked which one looks better? > Originally thought keeping this and patch 6/8 in one patch but started liking more this separation after you asked about it :-) I noticed from your 6/8 comment that I need to return error code also from i2c_dw_set_sda_hold() as we attempt to do locking there too. I'd like to keep this separate from 6/8 for simplicity but move 5/8 before this and 6/8.
diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c index c22654cce1df..1b542d3e5a2e 100644 --- a/drivers/i2c/busses/i2c-designware-common.c +++ b/drivers/i2c/busses/i2c-designware-common.c @@ -183,6 +183,41 @@ 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_set_sda_hold(struct dw_i2c_dev *dev) +{ + u32 reg; + int ret = 0; + + ret = i2c_dw_acquire_lock(dev); + if (ret) + return; + + /* Configure SDA Hold Time if required */ + reg = dw_readl(dev, DW_IC_COMP_VERSION); + if (reg >= DW_IC_SDA_HOLD_MIN_VERS) { + if (!dev->sda_hold_time) { + /* Keep previous hold time setting if no one set it */ + dev->sda_hold_time = dw_readl(dev, DW_IC_SDA_HOLD); + } + + /* + * Workaround for avoiding TX arbitration lost in case I2C + * slave pulls SDA down "too quickly" after falling egde of + * SCL by enabling non-zero SDA RX hold. Specification says it + * extends incoming SDA low to high transition while SCL is + * high but it apprears to help also above issue. + */ + if (!(dev->sda_hold_time & DW_IC_SDA_HOLD_RX_MASK)) + dev->sda_hold_time |= 1 << DW_IC_SDA_HOLD_RX_SHIFT; + } else if (dev->sda_hold_time) { + dev_warn(dev->dev, + "Hardware too old to adjust SDA hold time.\n"); + dev->sda_hold_time = 0; + } + + i2c_dw_release_lock(dev); +} + void __i2c_dw_disable(struct dw_i2c_dev *dev) { int timeout = 100; diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h index 5d54f70710ad..831b3d62b32c 100644 --- a/drivers/i2c/busses/i2c-designware-core.h +++ b/drivers/i2c/busses/i2c-designware-core.h @@ -298,6 +298,7 @@ void dw_writel(struct dw_i2c_dev *dev, u32 b, int offset); int i2c_dw_set_reg_access(struct dw_i2c_dev *dev); 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_set_sda_hold(struct dw_i2c_dev *dev); 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); diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c index 3a7c184f24c8..b89bc8a5b0b1 100644 --- a/drivers/i2c/busses/i2c-designware-master.c +++ b/drivers/i2c/busses/i2c-designware-master.c @@ -45,6 +45,11 @@ static void i2c_dw_configure_fifo_master(struct dw_i2c_dev *dev) dw_writel(dev, dev->master_cfg, DW_IC_CON); } +static void i2c_dw_set_timings_master(struct dw_i2c_dev *dev) +{ + i2c_dw_set_sda_hold(dev); +} + /** * i2c_dw_init() - Initialize the designware I2C master hardware * @dev: device private data @@ -56,7 +61,7 @@ static void i2c_dw_configure_fifo_master(struct dw_i2c_dev *dev) static int i2c_dw_init_master(struct dw_i2c_dev *dev) { u32 hcnt, lcnt; - u32 reg, comp_param1; + u32 comp_param1; u32 sda_falling_time, scl_falling_time; int ret; @@ -132,27 +137,9 @@ static int i2c_dw_init_master(struct dw_i2c_dev *dev) } } - /* Configure SDA Hold Time if required */ - reg = dw_readl(dev, DW_IC_COMP_VERSION); - if (reg >= DW_IC_SDA_HOLD_MIN_VERS) { - if (!dev->sda_hold_time) { - /* Keep previous hold time setting if no one set it */ - dev->sda_hold_time = dw_readl(dev, DW_IC_SDA_HOLD); - } - /* - * Workaround for avoiding TX arbitration lost in case I2C - * slave pulls SDA down "too quickly" after falling egde of - * SCL by enabling non-zero SDA RX hold. Specification says it - * extends incoming SDA low to high transition while SCL is - * high but it apprears to help also above issue. - */ - if (!(dev->sda_hold_time & DW_IC_SDA_HOLD_RX_MASK)) - dev->sda_hold_time |= 1 << DW_IC_SDA_HOLD_RX_SHIFT; + /* Write SDA hold time if supported */ + if (dev->sda_hold_time) dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD); - } else if (dev->sda_hold_time) { - dev_warn(dev->dev, - "Hardware too old to adjust SDA hold time.\n"); - } i2c_dw_configure_fifo_master(dev); i2c_dw_release_lock(dev); @@ -671,6 +658,7 @@ int i2c_dw_probe(struct dw_i2c_dev *dev) if (ret) return ret; + i2c_dw_set_timings_master(dev); ret = dev->init(dev); if (ret) return ret; diff --git a/drivers/i2c/busses/i2c-designware-slave.c b/drivers/i2c/busses/i2c-designware-slave.c index a1f802001e1f..6ba265db4eb0 100644 --- a/drivers/i2c/busses/i2c-designware-slave.c +++ b/drivers/i2c/busses/i2c-designware-slave.c @@ -51,7 +51,6 @@ static void i2c_dw_configure_fifo_slave(struct dw_i2c_dev *dev) */ static int i2c_dw_init_slave(struct dw_i2c_dev *dev) { - u32 reg; int ret; ret = i2c_dw_acquire_lock(dev); @@ -61,27 +60,9 @@ static int i2c_dw_init_slave(struct dw_i2c_dev *dev) /* Disable the adapter. */ __i2c_dw_disable(dev); - /* Configure SDA Hold Time if required. */ - reg = dw_readl(dev, DW_IC_COMP_VERSION); - if (reg >= DW_IC_SDA_HOLD_MIN_VERS) { - if (!dev->sda_hold_time) { - /* Keep previous hold time setting if no one set it. */ - dev->sda_hold_time = dw_readl(dev, DW_IC_SDA_HOLD); - } - /* - * Workaround for avoiding TX arbitration lost in case I2C - * slave pulls SDA down "too quickly" after falling egde of - * SCL by enabling non-zero SDA RX hold. Specification says it - * extends incoming SDA low to high transition while SCL is - * high but it apprears to help also above issue. - */ - if (!(dev->sda_hold_time & DW_IC_SDA_HOLD_RX_MASK)) - dev->sda_hold_time |= 1 << DW_IC_SDA_HOLD_RX_SHIFT; + /* Write SDA hold time if supported */ + if (dev->sda_hold_time) dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD); - } else { - dev_warn(dev->dev, - "Hardware too old to adjust SDA hold time.\n"); - } i2c_dw_configure_fifo_slave(dev); i2c_dw_release_lock(dev); @@ -287,6 +268,7 @@ int i2c_dw_probe_slave(struct dw_i2c_dev *dev) if (ret) return ret; + i2c_dw_set_sda_hold(dev); ret = dev->init(dev); if (ret) return ret;
SDA hold time configuration is common to both master and slave code. It is also something that can be done once during probe and do only register write when HW needs to be reinitialized. Remove duplication and move SDA hold time configuration to common code. It will be called from slave probe and for master code from a new i2c_dw_set_timings_master() to where we will populate more probe time timing parameter setting. Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> --- drivers/i2c/busses/i2c-designware-common.c | 35 ++++++++++++++++++++++ drivers/i2c/busses/i2c-designware-core.h | 1 + drivers/i2c/busses/i2c-designware-master.c | 30 ++++++------------- drivers/i2c/busses/i2c-designware-slave.c | 24 ++------------- 4 files changed, 48 insertions(+), 42 deletions(-)