diff mbox series

[3/5] i2c: designware: Separate timing parameter setting from HW initalization

Message ID 20180529112754.13477-4-jarkko.nikula@linux.intel.com
State Superseded
Headers show
Series i2c: designware: Improve debug prints | expand

Commit Message

Jarkko Nikula May 29, 2018, 11:27 a.m. UTC 
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. Since SDA hold
time setting is common to both master and slave code remove this
duplication and move it to common code.

Signed-off-by: Jarkko Nikula <jarkko.nikula@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-common.c |  34 +++++
 drivers/i2c/busses/i2c-designware-core.h   |   1 +
 drivers/i2c/busses/i2c-designware-master.c | 157 ++++++++++++---------
 drivers/i2c/busses/i2c-designware-slave.c  |  22 +--
 4 files changed, 125 insertions(+), 89 deletions(-)

Comments

Andy Shevchenko May 29, 2018, 7:14 p.m. UTC | #1 
On Tue, 2018-05-29 at 14:27 +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. Since SDA
> hold
> time setting is common to both master and slave code remove this
> duplication and move it to common code.

Just would like you to double check if it possible to split this to
couple or more smaller changes.

> +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);
> +		}

Pewrhaps + blank line here ?

> +		/*
> +		 * 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);
> +}

> +static void i2c_dw_set_timings_master(struct dw_i2c_dev *dev)
>  {
> 

> +	u32 comp_param1;
>  	u32 sda_falling_time, scl_falling_time;
> +	int ret = 0;

Redundant assignment.
 
>  	ret = i2c_dw_acquire_lock(dev);
>  	if (ret)
> +		return;

> +	/* Calculate SCL timing parameters for standard mode if not
> set */
> +	if (!dev->ss_hcnt || !dev->ss_lcnt) {
> +		dev->ss_hcnt =
> +			i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),

You can consider to use

dev->ss_hcnt = dev->ss_hcnt ?: instead of conditional
(rationale: smaller indentation level)

> +		dev->ss_lcnt =
> +			i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),

Ditto here and below.

Moreover the question if in all cases the first argument is
i2c_dw_clk_rate(dev)? It might be useful to factor this into the callee.

> +			dev->fs_hcnt = dev->fp_hcnt;
> +			dev->fs_lcnt = dev->fp_lcnt;

Dunno if it makes sense to introduce something like

struct i2c_dw_cnt {
  ... low;
  ... high;
};

...and convert the driver to use it.

> +	dev_dbg(dev->dev, "Fast Mode%s HCNT:LCNT = %d:%d\n",
> +		dev->clk_freq == 1000000 ? " Plus" : "",
> +		dev->fs_hcnt, dev->fs_lcnt);

I would rather go with switch-case or alike and put full mode name into
string literals. Moreover you may use same literal in the code after
all.

> +	if (dev->sda_hold_time)
>  		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);

Btw, what happens on older hardware that doesn't support SDA_HOLD
register when you try to write 0 there?
Jarkko Nikula May 30, 2018, 6:39 a.m. UTC | #2 
On 05/29/2018 10:14 PM, Andy Shevchenko wrote:
> 
> Just would like you to double check if it possible to split this to
> couple or more smaller changes.
> I was thinking to have i2c_dw_set_sda_hold() as a separate patch but 
decided otherwise. Maybe it makes sense as it is not plain code move and 
I can add the blank line you suggested.

>> +	/* Calculate SCL timing parameters for standard mode if not
>> set */
>> +	if (!dev->ss_hcnt || !dev->ss_lcnt) {
>> +		dev->ss_hcnt =
>> +			i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),
> 
> You can consider to use
> 
> dev->ss_hcnt = dev->ss_hcnt ?: instead of conditional
> (rationale: smaller indentation level)
> 
>> +		dev->ss_lcnt =
>> +			i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
> 
I wouldn't change the semantics now. Now if either timing parameter is 
missing then we calculate both.

> Moreover the question if in all cases the first argument is
> i2c_dw_clk_rate(dev)? It might be useful to factor this into the callee.
> 
>> +			dev->fs_hcnt = dev->fp_hcnt;
>> +			dev->fs_lcnt = dev->fp_lcnt;
> 
> Dunno if it makes sense to introduce something like
> 
> struct i2c_dw_cnt {
>    ... low;
>    ... high;
> };
> 
> ...and convert the driver to use it.

I'll check that.

> 
>> +	dev_dbg(dev->dev, "Fast Mode%s HCNT:LCNT = %d:%d\n",
>> +		dev->clk_freq == 1000000 ? " Plus" : "",
>> +		dev->fs_hcnt, dev->fs_lcnt);
> 
> I would rather go with switch-case or alike and put full mode name into
> string literals. Moreover you may use same literal in the code after
> all.
> 
True.

>> +	if (dev->sda_hold_time)
>>   		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
> 
> Btw, what happens on older hardware that doesn't support SDA_HOLD
> register when you try to write 0 there?
> 
if (dev->sda_hold_time) covers that and dev->sda_hold_time is set to 
zero on that HW.
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-designware-common.c b/drivers/i2c/busses/i2c-designware-common.c
index a9e75b809f38..4f951486f74a 100644
--- a/drivers/i2c/busses/i2c-designware-common.c
+++ b/drivers/i2c/busses/i2c-designware-common.c
@@ -182,6 +182,40 @@  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..b47aa919b377 100644
--- a/drivers/i2c/busses/i2c-designware-master.c
+++ b/drivers/i2c/busses/i2c-designware-master.c
@@ -45,76 +45,78 @@  static void i2c_dw_configure_fifo_master(struct dw_i2c_dev *dev)
 	dw_writel(dev, dev->master_cfg, DW_IC_CON);
 }
 
-/**
- * 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)
+static void i2c_dw_set_timings_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;
+	int ret = 0;
 
 	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 = i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),
+	/* Calculate SCL timing parameters for standard mode if not set */
+	if (!dev->ss_hcnt || !dev->ss_lcnt) {
+		dev->ss_hcnt =
+			i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),
 					4000,	/* tHD;STA = tHIGH = 4.0 us */
 					sda_falling_time,
 					0,	/* 0: DW default, 1: Ideal */
 					0);	/* No offset */
-		lcnt = i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
+		dev->ss_lcnt =
+			i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
 					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 = i2c_dw_scl_hcnt(i2c_dw_clk_rate(dev),
+	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;
+		} else {
+			dev->clk_freq = 400000;
+		}
+	}
+	/*
+	 * 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(i2c_dw_clk_rate(dev),
 					600,	/* tHD;STA = tHIGH = 0.6 us */
 					sda_falling_time,
 					0,	/* 0: DW default, 1: Ideal */
 					0);	/* No offset */
-		lcnt = i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
+		dev->fs_lcnt =
+			i2c_dw_scl_lcnt(i2c_dw_clk_rate(dev),
 					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",
+		dev->clk_freq == 1000000 ? " Plus" : "",
+		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)
@@ -122,38 +124,54 @@  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);
 		}
 	}
 
-	/* 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;
-		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_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);
+
 	i2c_dw_configure_fifo_master(dev);
 	i2c_dw_release_lock(dev);
 
@@ -671,6 +689,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..938e4aaebfec 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);
@@ -62,26 +61,8 @@  static int i2c_dw_init_slave(struct dw_i2c_dev *dev)
 	__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;
+	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;