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@xxxxxxxxxxxxxxx>
> ---
> 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);
--
Andy Shevchenko <andriy.shevchenko@xxxxxxxxxxxxxxx>
Intel Finland Oy
