| Message ID | 20180601134725.1375-7-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: > Mixed timing parameter validation, calculation and their debug prints > with HW initialization in i2c_dw_init_master() and i2c_dw_init_slave() > has been bothering me some time. > > It makes function a little bit unclear to follow, doesn't show what > steps > are needed to do only once during probe and what are needed whenever > HW > needs to be reinitialized. Also those debug prints show information > that > doesn't change runtime and thus are also needlessly printed multiple > times > whenever HW is reinitialized. > > Thus let the i2c_dw_init_master() and i2c_dw_init_slave() to do only > HW > initialization and move out one time parameter setting and debug > prints > to separate functions which are called only during probe. > > Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> > --- > v2: > - SDA hold time configuration change moved to a new patch > - Indentation change around i2c_dw_scl_hcnt()/i2c_dw_scl_lcnt() and > i2c_dw_clk_rate() cleanup moved to a new patch > - fp_str pointing to empty or " Plus" string for simpler fast > mode/fast > mode plus debug prints in this and another patch in this series > --- > drivers/i2c/busses/i2c-designware-master.c | 128 +++++++++++++------- > - > 1 file changed, 77 insertions(+), 51 deletions(-) > > diff --git a/drivers/i2c/busses/i2c-designware-master.c > b/drivers/i2c/busses/i2c-designware-master.c > index 6557e163065e..e3a56c8e33c7 100644 > --- a/drivers/i2c/busses/i2c-designware-master.c > +++ b/drivers/i2c/busses/i2c-designware-master.c > @@ -46,85 +46,77 @@ static void i2c_dw_configure_fifo_master(struct > dw_i2c_dev *dev) > } > > 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 > - * > - * This functions configures and enables the I2C master. > - * This function is called during I2C init function, and in case of > timeout at > - * run time. > - */ > -static int i2c_dw_init_master(struct dw_i2c_dev *dev) > { > u32 ic_clk = i2c_dw_clk_rate(dev); > - u32 hcnt, lcnt; > + const char *fp_str = ""; > u32 comp_param1; > u32 sda_falling_time, scl_falling_time; > int ret; > > ret = i2c_dw_acquire_lock(dev); > if (ret) > - return ret; > - > + return; Shouldn't we return an error code to the caller? Previously we abort initialization IIUC and now silently skip the timing settings completely. Feels like a regression. > comp_param1 = dw_readl(dev, DW_IC_COMP_PARAM_1); > + i2c_dw_release_lock(dev); > > - /* Disable the adapter */ > - __i2c_dw_disable(dev); > - > - /* Set standard and fast speed deviders for high/low periods > */ > - > + /* Set standard and fast speed dividers for high/low periods > */ > sda_falling_time = dev->sda_falling_time ?: 300; /* ns */ > scl_falling_time = dev->scl_falling_time ?: 300; /* ns */ > > - /* Set SCL timing parameters for standard-mode */ > - if (dev->ss_hcnt && dev->ss_lcnt) { > - hcnt = dev->ss_hcnt; > - lcnt = dev->ss_lcnt; > - } else { > - hcnt = > + /* Calculate SCL timing parameters for standard mode if not > set */ > + if (!dev->ss_hcnt || !dev->ss_lcnt) { > + dev->ss_hcnt = > i2c_dw_scl_hcnt(ic_clk, > 4000, /* tHD;STA = > tHIGH = 4.0 us */ > sda_falling_time, > 0, /* 0: DW default, > 1: Ideal */ > 0); /* No offset */ > - lcnt = > + dev->ss_lcnt = > i2c_dw_scl_lcnt(ic_clk, > 4700, /* tLOW = 4.7 us > */ > scl_falling_time, > 0); /* No offset */ > } > - dw_writel(dev, hcnt, DW_IC_SS_SCL_HCNT); > - dw_writel(dev, lcnt, DW_IC_SS_SCL_LCNT); > - dev_dbg(dev->dev, "Standard-mode HCNT:LCNT = %d:%d\n", hcnt, > lcnt); > - > - /* Set SCL timing parameters for fast-mode or fast-mode plus > */ > - if ((dev->clk_freq == 1000000) && dev->fp_hcnt && dev- > >fp_lcnt) { > - hcnt = dev->fp_hcnt; > - lcnt = dev->fp_lcnt; > - } else if (dev->fs_hcnt && dev->fs_lcnt) { > - hcnt = dev->fs_hcnt; > - lcnt = dev->fs_lcnt; > - } else { > - hcnt = > + dev_dbg(dev->dev, "Standard Mode HCNT:LCNT = %d:%d\n", > + dev->ss_hcnt, dev->ss_lcnt); > + > + /* > + * Set SCL timing parameters for fast mode or fast mode plus. > Only > + * difference is the timing parameter values since the > registers are > + * the same. > + */ > + if (dev->clk_freq == 1000000) { > + /* > + * Check are fast mode plus parameters available and > use > + * fast mode if not. > + */ > + if (dev->fp_hcnt && dev->fp_lcnt) { > + dev->fs_hcnt = dev->fp_hcnt; > + dev->fs_lcnt = dev->fp_lcnt; > + fp_str = " Plus"; > + } > + } > + /* > + * Calculate SCL timing parameters for fast mode if not set. > They are > + * needed also in high speed mode. > + */ > + if (!dev->fs_hcnt || !dev->fs_lcnt) { > + dev->fs_hcnt = > i2c_dw_scl_hcnt(ic_clk, > 600, /* tHD;STA = > tHIGH = 0.6 us */ > sda_falling_time, > 0, /* 0: DW default, > 1: Ideal */ > 0); /* No offset */ > - lcnt = > + dev->fs_lcnt = > i2c_dw_scl_lcnt(ic_clk, > 1300, /* tLOW = 1.3 us > */ > scl_falling_time, > 0); /* No offset */ > } > - dw_writel(dev, hcnt, DW_IC_FS_SCL_HCNT); > - dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT); > - dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, > lcnt); > + dev_dbg(dev->dev, "Fast Mode%s HCNT:LCNT = %d:%d\n", > + fp_str, dev->fs_hcnt, dev->fs_lcnt); > > + /* Check is high speed possible and fall back to fast mode if > not */ > if ((dev->master_cfg & DW_IC_CON_SPEED_MASK) == > DW_IC_CON_SPEED_HIGH) { > if ((comp_param1 & > DW_IC_COMP_PARAM_1_SPEED_MODE_MASK) > @@ -132,16 +124,50 @@ static int i2c_dw_init_master(struct dw_i2c_dev > *dev) > dev_err(dev->dev, "High Speed not > supported!\n"); > dev->master_cfg &= ~DW_IC_CON_SPEED_MASK; > dev->master_cfg |= DW_IC_CON_SPEED_FAST; > + dev->hs_hcnt = 0; > + dev->hs_lcnt = 0; > } else if (dev->hs_hcnt && dev->hs_lcnt) { > - hcnt = dev->hs_hcnt; > - lcnt = dev->hs_lcnt; > - dw_writel(dev, hcnt, DW_IC_HS_SCL_HCNT); > - dw_writel(dev, lcnt, DW_IC_HS_SCL_LCNT); > - dev_dbg(dev->dev, "HighSpeed-mode HCNT:LCNT = > %d:%d\n", > - hcnt, lcnt); > + dev_dbg(dev->dev, "High Speed Mode HCNT:LCNT > = %d:%d\n", > + dev->hs_hcnt, dev->hs_lcnt); > } > } > > + i2c_dw_set_sda_hold(dev); > +} > + > +/** > + * i2c_dw_init() - Initialize the designware I2C master hardware > + * @dev: device private data > + * > + * This functions configures and enables the I2C master. > + * This function is called during I2C init function, and in case of > timeout at > + * run time. > + */ > +static int i2c_dw_init_master(struct dw_i2c_dev *dev) > +{ > + int ret; > + > + ret = i2c_dw_acquire_lock(dev); > + if (ret) > + return ret; > + > + /* Disable the adapter */ > + __i2c_dw_disable(dev); > + > + /* Write standard speed timing parameters */ > + dw_writel(dev, dev->ss_hcnt, DW_IC_SS_SCL_HCNT); > + dw_writel(dev, dev->ss_lcnt, DW_IC_SS_SCL_LCNT); > + > + /* Write fast mode/fast mode plus timing parameters */ > + dw_writel(dev, dev->fs_hcnt, DW_IC_FS_SCL_HCNT); > + dw_writel(dev, dev->fs_lcnt, DW_IC_FS_SCL_LCNT); > + > + /* Write high speed timing parameters if supported */ > + if (dev->hs_hcnt && dev->hs_lcnt) { > + dw_writel(dev, dev->hs_hcnt, DW_IC_HS_SCL_HCNT); > + dw_writel(dev, dev->hs_lcnt, DW_IC_HS_SCL_LCNT); > + } > + > /* Write SDA hold time if supported */ > if (dev->sda_hold_time) > dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
On 06/04/2018 03:48 PM, Andy Shevchenko wrote: > On Fri, 2018-06-01 at 16:47 +0300, Jarkko Nikula wrote: >> Mixed timing parameter validation, calculation and their debug prints >> with HW initialization in i2c_dw_init_master() and i2c_dw_init_slave() >> has been bothering me some time. >> >> It makes function a little bit unclear to follow, doesn't show what >> steps >> are needed to do only once during probe and what are needed whenever >> HW >> needs to be reinitialized. Also those debug prints show information >> that >> doesn't change runtime and thus are also needlessly printed multiple >> times >> whenever HW is reinitialized. >> >> Thus let the i2c_dw_init_master() and i2c_dw_init_slave() to do only >> HW >> initialization and move out one time parameter setting and debug >> prints >> to separate functions which are called only during probe. >> >> Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> >> --- >> v2: >> - SDA hold time configuration change moved to a new patch >> - Indentation change around i2c_dw_scl_hcnt()/i2c_dw_scl_lcnt() and >> i2c_dw_clk_rate() cleanup moved to a new patch >> - fp_str pointing to empty or " Plus" string for simpler fast >> mode/fast >> mode plus debug prints in this and another patch in this series >> --- >> drivers/i2c/busses/i2c-designware-master.c | 128 +++++++++++++------- >> - >> 1 file changed, 77 insertions(+), 51 deletions(-) >> >> diff --git a/drivers/i2c/busses/i2c-designware-master.c >> b/drivers/i2c/busses/i2c-designware-master.c >> index 6557e163065e..e3a56c8e33c7 100644 >> --- a/drivers/i2c/busses/i2c-designware-master.c >> +++ b/drivers/i2c/busses/i2c-designware-master.c >> @@ -46,85 +46,77 @@ static void i2c_dw_configure_fifo_master(struct >> dw_i2c_dev *dev) >> } >> >> 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 >> - * >> - * This functions configures and enables the I2C master. >> - * This function is called during I2C init function, and in case of >> timeout at >> - * run time. >> - */ >> -static int i2c_dw_init_master(struct dw_i2c_dev *dev) >> { >> u32 ic_clk = i2c_dw_clk_rate(dev); >> - u32 hcnt, lcnt; >> + const char *fp_str = ""; >> u32 comp_param1; >> u32 sda_falling_time, scl_falling_time; >> int ret; >> >> ret = i2c_dw_acquire_lock(dev); >> if (ret) >> - return ret; >> - >> + return; > > Shouldn't we return an error code to the caller? > Previously we abort initialization IIUC and now silently skip the timing > settings completely. Feels like a regression. > Yes we should, thanks for spotting. I added locking while cooking the first version but failed to realize this regression in case we are not able to get the lock.
diff --git a/drivers/i2c/busses/i2c-designware-master.c b/drivers/i2c/busses/i2c-designware-master.c index 6557e163065e..e3a56c8e33c7 100644 --- a/drivers/i2c/busses/i2c-designware-master.c +++ b/drivers/i2c/busses/i2c-designware-master.c @@ -46,85 +46,77 @@ static void i2c_dw_configure_fifo_master(struct dw_i2c_dev *dev) } 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 - * - * This functions configures and enables the I2C master. - * This function is called during I2C init function, and in case of timeout at - * run time. - */ -static int i2c_dw_init_master(struct dw_i2c_dev *dev) { u32 ic_clk = i2c_dw_clk_rate(dev); - u32 hcnt, lcnt; + const char *fp_str = ""; u32 comp_param1; u32 sda_falling_time, scl_falling_time; int ret; ret = i2c_dw_acquire_lock(dev); if (ret) - return ret; - + return; comp_param1 = dw_readl(dev, DW_IC_COMP_PARAM_1); + i2c_dw_release_lock(dev); - /* Disable the adapter */ - __i2c_dw_disable(dev); - - /* Set standard and fast speed deviders for high/low periods */ - + /* Set standard and fast speed dividers for high/low periods */ sda_falling_time = dev->sda_falling_time ?: 300; /* ns */ scl_falling_time = dev->scl_falling_time ?: 300; /* ns */ - /* Set SCL timing parameters for standard-mode */ - if (dev->ss_hcnt && dev->ss_lcnt) { - hcnt = dev->ss_hcnt; - lcnt = dev->ss_lcnt; - } else { - hcnt = + /* Calculate SCL timing parameters for standard mode if not set */ + if (!dev->ss_hcnt || !dev->ss_lcnt) { + dev->ss_hcnt = i2c_dw_scl_hcnt(ic_clk, 4000, /* tHD;STA = tHIGH = 4.0 us */ sda_falling_time, 0, /* 0: DW default, 1: Ideal */ 0); /* No offset */ - lcnt = + dev->ss_lcnt = i2c_dw_scl_lcnt(ic_clk, 4700, /* tLOW = 4.7 us */ scl_falling_time, 0); /* No offset */ } - dw_writel(dev, hcnt, DW_IC_SS_SCL_HCNT); - dw_writel(dev, lcnt, DW_IC_SS_SCL_LCNT); - dev_dbg(dev->dev, "Standard-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt); - - /* Set SCL timing parameters for fast-mode or fast-mode plus */ - if ((dev->clk_freq == 1000000) && dev->fp_hcnt && dev->fp_lcnt) { - hcnt = dev->fp_hcnt; - lcnt = dev->fp_lcnt; - } else if (dev->fs_hcnt && dev->fs_lcnt) { - hcnt = dev->fs_hcnt; - lcnt = dev->fs_lcnt; - } else { - hcnt = + dev_dbg(dev->dev, "Standard Mode HCNT:LCNT = %d:%d\n", + dev->ss_hcnt, dev->ss_lcnt); + + /* + * Set SCL timing parameters for fast mode or fast mode plus. Only + * difference is the timing parameter values since the registers are + * the same. + */ + if (dev->clk_freq == 1000000) { + /* + * Check are fast mode plus parameters available and use + * fast mode if not. + */ + if (dev->fp_hcnt && dev->fp_lcnt) { + dev->fs_hcnt = dev->fp_hcnt; + dev->fs_lcnt = dev->fp_lcnt; + fp_str = " Plus"; + } + } + /* + * Calculate SCL timing parameters for fast mode if not set. They are + * needed also in high speed mode. + */ + if (!dev->fs_hcnt || !dev->fs_lcnt) { + dev->fs_hcnt = i2c_dw_scl_hcnt(ic_clk, 600, /* tHD;STA = tHIGH = 0.6 us */ sda_falling_time, 0, /* 0: DW default, 1: Ideal */ 0); /* No offset */ - lcnt = + dev->fs_lcnt = i2c_dw_scl_lcnt(ic_clk, 1300, /* tLOW = 1.3 us */ scl_falling_time, 0); /* No offset */ } - dw_writel(dev, hcnt, DW_IC_FS_SCL_HCNT); - dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT); - dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt); + dev_dbg(dev->dev, "Fast Mode%s HCNT:LCNT = %d:%d\n", + fp_str, dev->fs_hcnt, dev->fs_lcnt); + /* Check is high speed possible and fall back to fast mode if not */ if ((dev->master_cfg & DW_IC_CON_SPEED_MASK) == DW_IC_CON_SPEED_HIGH) { if ((comp_param1 & DW_IC_COMP_PARAM_1_SPEED_MODE_MASK) @@ -132,16 +124,50 @@ static int i2c_dw_init_master(struct dw_i2c_dev *dev) dev_err(dev->dev, "High Speed not supported!\n"); dev->master_cfg &= ~DW_IC_CON_SPEED_MASK; dev->master_cfg |= DW_IC_CON_SPEED_FAST; + dev->hs_hcnt = 0; + dev->hs_lcnt = 0; } else if (dev->hs_hcnt && dev->hs_lcnt) { - hcnt = dev->hs_hcnt; - lcnt = dev->hs_lcnt; - dw_writel(dev, hcnt, DW_IC_HS_SCL_HCNT); - dw_writel(dev, lcnt, DW_IC_HS_SCL_LCNT); - dev_dbg(dev->dev, "HighSpeed-mode HCNT:LCNT = %d:%d\n", - hcnt, lcnt); + dev_dbg(dev->dev, "High Speed Mode HCNT:LCNT = %d:%d\n", + dev->hs_hcnt, dev->hs_lcnt); } } + i2c_dw_set_sda_hold(dev); +} + +/** + * i2c_dw_init() - Initialize the designware I2C master hardware + * @dev: device private data + * + * This functions configures and enables the I2C master. + * This function is called during I2C init function, and in case of timeout at + * run time. + */ +static int i2c_dw_init_master(struct dw_i2c_dev *dev) +{ + int ret; + + ret = i2c_dw_acquire_lock(dev); + if (ret) + return ret; + + /* Disable the adapter */ + __i2c_dw_disable(dev); + + /* Write standard speed timing parameters */ + dw_writel(dev, dev->ss_hcnt, DW_IC_SS_SCL_HCNT); + dw_writel(dev, dev->ss_lcnt, DW_IC_SS_SCL_LCNT); + + /* Write fast mode/fast mode plus timing parameters */ + dw_writel(dev, dev->fs_hcnt, DW_IC_FS_SCL_HCNT); + dw_writel(dev, dev->fs_lcnt, DW_IC_FS_SCL_LCNT); + + /* Write high speed timing parameters if supported */ + if (dev->hs_hcnt && dev->hs_lcnt) { + dw_writel(dev, dev->hs_hcnt, DW_IC_HS_SCL_HCNT); + dw_writel(dev, dev->hs_lcnt, DW_IC_HS_SCL_LCNT); + } + /* Write SDA hold time if supported */ if (dev->sda_hold_time) dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
Mixed timing parameter validation, calculation and their debug prints with HW initialization in i2c_dw_init_master() and i2c_dw_init_slave() has been bothering me some time. It makes function a little bit unclear to follow, doesn't show what steps are needed to do only once during probe and what are needed whenever HW needs to be reinitialized. Also those debug prints show information that doesn't change runtime and thus are also needlessly printed multiple times whenever HW is reinitialized. Thus let the i2c_dw_init_master() and i2c_dw_init_slave() to do only HW initialization and move out one time parameter setting and debug prints to separate functions which are called only during probe. Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com> --- v2: - SDA hold time configuration change moved to a new patch - Indentation change around i2c_dw_scl_hcnt()/i2c_dw_scl_lcnt() and i2c_dw_clk_rate() cleanup moved to a new patch - fp_str pointing to empty or " Plus" string for simpler fast mode/fast mode plus debug prints in this and another patch in this series --- drivers/i2c/busses/i2c-designware-master.c | 128 +++++++++++++-------- 1 file changed, 77 insertions(+), 51 deletions(-)