[PATCH v2 0/2] Add i2c ac-timing adjust support

[PATCH v2 0/2] Add i2c ac-timing adjust support

[Qii Wang]

This series are based on 5.7-rc1, we provide two patches to support i2c ac-timing.

Main changes compared to v1:
--add maintainer for mediatek i2c controller driver
--fix warning of self-assignment

Qii Wang (2):
  MAINTAINERS: add maintainer for mediatek i2c controller driver
  i2c: mediatek: Add i2c ac-timing adjust support

 MAINTAINERS                     |   7 +
 drivers/i2c/busses/i2c-mt65xx.c | 328 +++++++++++++++++++++++++++++++++-------
 2 files changed, 284 insertions(+), 51 deletions(-)

--
1.9.1

[PATCH v2 1/2] MAINTAINERS: add maintainer for mediatek i2c controller driver

[Qii Wang]

Add Qii Wang as maintainer for mediatek i2c controller driver.

Signed-off-by: Qii Wang <qii.wang@mediatek.com>
---
 MAINTAINERS | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index e64e5db..c0fdf11 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10658,6 +10658,13 @@ L:	netdev@vger.kernel.org
 S:	Maintained
 F:	drivers/net/ethernet/mediatek/
 
+MEDIATEK I2C CONTROLLER DRIVER
+M:	Qii Wang <qii.wang@mediatek.com>
+L:	linux-i2c@vger.kernel.org
+S:	Maintained
+F:	Documentation/devicetree/bindings/i2c/i2c-mt65xx.txt
+F:	drivers/i2c/busses/i2c-mt65xx.c
+
 MEDIATEK JPEG DRIVER
 M:	Rick Chang <rick.chang@mediatek.com>
 M:	Bin Liu <bin.liu@mediatek.com>
-- 
1.9.1

[PATCH v2 2/2] i2c: mediatek: Add i2c ac-timing adjust support

[Qii Wang]

This patch adds a algorithm to calculate some ac-timing parameters
which can fully meet I2C Spec.

Signed-off-by: Qii Wang <qii.wang@mediatek.com>
---
 drivers/i2c/busses/i2c-mt65xx.c | 328 +++++++++++++++++++++++++++++++++-------
 1 file changed, 277 insertions(+), 51 deletions(-)

diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
index 0ca6c38a..7020618 100644
--- a/drivers/i2c/busses/i2c-mt65xx.c
+++ b/drivers/i2c/busses/i2c-mt65xx.c
@@ -40,12 +40,11 @@
 #define I2C_SOFT_RST			0x0001
 #define I2C_FIFO_ADDR_CLR		0x0001
 #define I2C_DELAY_LEN			0x0002
-#define I2C_ST_START_CON		0x8001
-#define I2C_FS_START_CON		0x1800
 #define I2C_TIME_CLR_VALUE		0x0000
 #define I2C_TIME_DEFAULT_VALUE		0x0003
 #define I2C_WRRD_TRANAC_VALUE		0x0002
 #define I2C_RD_TRANAC_VALUE		0x0001
+#define I2C_SCL_MIS_COMP_VALUE		0x0000
 
 #define I2C_DMA_CON_TX			0x0000
 #define I2C_DMA_CON_RX			0x0001
@@ -55,10 +54,13 @@
 #define I2C_DMA_HARD_RST		0x0002
 #define I2C_DMA_4G_MODE			0x0001
 
-#define I2C_DEFAULT_CLK_DIV		5
 #define MAX_SAMPLE_CNT_DIV		8
 #define MAX_STEP_CNT_DIV		64
+#define MAX_CLOCK_DIV			256
 #define MAX_HS_STEP_CNT_DIV		8
+#define I2C_STANDARD_MODE_BUFFER	(1000 / 2)
+#define I2C_FAST_MODE_BUFFER		(300 / 2)
+#define I2C_FAST_MODE_PLUS_BUFFER	(20 / 2)
 
 #define I2C_CONTROL_RS                  (0x1 << 1)
 #define I2C_CONTROL_DMA_EN              (0x1 << 2)
@@ -123,6 +125,12 @@ enum I2C_REGS_OFFSET {
 	OFFSET_TRANSFER_LEN_AUX,
 	OFFSET_CLOCK_DIV,
 	OFFSET_LTIMING,
+	OFFSET_SCL_HIGH_LOW_RATIO,
+	OFFSET_HS_SCL_HIGH_LOW_RATIO,
+	OFFSET_SCL_MIS_COMP_POINT,
+	OFFSET_STA_STO_AC_TIMING,
+	OFFSET_HS_STA_STO_AC_TIMING,
+	OFFSET_SDA_TIMING,
 };
 
 static const u16 mt_i2c_regs_v1[] = {
@@ -150,6 +158,12 @@ enum I2C_REGS_OFFSET {
 	[OFFSET_DEBUGCTRL] = 0x68,
 	[OFFSET_TRANSFER_LEN_AUX] = 0x6c,
 	[OFFSET_CLOCK_DIV] = 0x70,
+	[OFFSET_SCL_HIGH_LOW_RATIO] = 0x74,
+	[OFFSET_HS_SCL_HIGH_LOW_RATIO] = 0x78,
+	[OFFSET_SCL_MIS_COMP_POINT] = 0x7C,
+	[OFFSET_STA_STO_AC_TIMING] = 0x80,
+	[OFFSET_HS_STA_STO_AC_TIMING] = 0x84,
+	[OFFSET_SDA_TIMING] = 0x88,
 };
 
 static const u16 mt_i2c_regs_v2[] = {
@@ -168,9 +182,11 @@ enum I2C_REGS_OFFSET {
 	[OFFSET_HS] = 0x30,
 	[OFFSET_IO_CONFIG] = 0x34,
 	[OFFSET_FIFO_ADDR_CLR] = 0x38,
+	[OFFSET_SDA_TIMING] = 0x3c,
 	[OFFSET_TRANSFER_LEN_AUX] = 0x44,
 	[OFFSET_CLOCK_DIV] = 0x48,
 	[OFFSET_SOFTRESET] = 0x50,
+	[OFFSET_SCL_MIS_COMP_POINT] = 0x90,
 	[OFFSET_DEBUGSTAT] = 0xe0,
 	[OFFSET_DEBUGCTRL] = 0xe8,
 	[OFFSET_FIFO_STAT] = 0xf4,
@@ -191,6 +207,19 @@ struct mtk_i2c_compatible {
 	unsigned char ltiming_adjust: 1;
 };
 
+struct mtk_i2c_ac_timing {
+	u16 htiming;
+	u16 ltiming;
+	u16 hs;
+	u16 ext;
+	u16 inter_clk_div;
+	u16 scl_hl_ratio;
+	u16 hs_scl_hl_ratio;
+	u16 sta_stop;
+	u16 hs_sta_stop;
+	u16 sda_timing;
+};
+
 struct mtk_i2c {
 	struct i2c_adapter adap;	/* i2c host adapter */
 	struct device *dev;
@@ -215,9 +244,46 @@ struct mtk_i2c {
 	u16 ltiming_reg;
 	unsigned char auto_restart;
 	bool ignore_restart_irq;
+	struct mtk_i2c_ac_timing ac_timing;
 	const struct mtk_i2c_compatible *dev_comp;
 };
 
+/**
+ * struct i2c_spec_values:
+ * min_low_ns: min LOW period of the SCL clock
+ * min_su_sta_ns: min set-up time for a repeated START condition
+ * max_hd_dat_ns: max data hold time
+ * min_su_dat_ns: min data set-up time
+ */
+struct i2c_spec_values {
+	unsigned int min_low_ns;
+	unsigned int min_high_ns;
+	unsigned int min_su_sta_ns;
+	unsigned int max_hd_dat_ns;
+	unsigned int min_su_dat_ns;
+};
+
+static const struct i2c_spec_values standard_mode_spec = {
+	.min_low_ns = 4700 + I2C_STANDARD_MODE_BUFFER,
+	.min_su_sta_ns = 4700 + I2C_STANDARD_MODE_BUFFER,
+	.max_hd_dat_ns = 3450 - I2C_STANDARD_MODE_BUFFER,
+	.min_su_dat_ns = 250 + I2C_STANDARD_MODE_BUFFER,
+};
+
+static const struct i2c_spec_values fast_mode_spec = {
+	.min_low_ns = 1300 + I2C_FAST_MODE_BUFFER,
+	.min_su_sta_ns = 600 + I2C_FAST_MODE_BUFFER,
+	.max_hd_dat_ns = 900 - I2C_FAST_MODE_BUFFER,
+	.min_su_dat_ns = 100 + I2C_FAST_MODE_BUFFER,
+};
+
+static const struct i2c_spec_values fast_mode_plus_spec = {
+	.min_low_ns = 500 + I2C_FAST_MODE_PLUS_BUFFER,
+	.min_su_sta_ns = 260 + I2C_FAST_MODE_PLUS_BUFFER,
+	.max_hd_dat_ns = 400 - I2C_FAST_MODE_PLUS_BUFFER,
+	.min_su_dat_ns = 50 + I2C_FAST_MODE_PLUS_BUFFER,
+};
+
 static const struct i2c_adapter_quirks mt6577_i2c_quirks = {
 	.flags = I2C_AQ_COMB_WRITE_THEN_READ,
 	.max_num_msgs = 1,
@@ -397,14 +463,38 @@ static void mtk_i2c_init_hw(struct mtk_i2c *i2c)
 	if (i2c->dev_comp->dcm)
 		mtk_i2c_writew(i2c, I2C_DCM_DISABLE, OFFSET_DCM_EN);
 
-	if (i2c->dev_comp->timing_adjust)
-		mtk_i2c_writew(i2c, I2C_DEFAULT_CLK_DIV - 1, OFFSET_CLOCK_DIV);
-
 	mtk_i2c_writew(i2c, i2c->timing_reg, OFFSET_TIMING);
 	mtk_i2c_writew(i2c, i2c->high_speed_reg, OFFSET_HS);
 	if (i2c->dev_comp->ltiming_adjust)
 		mtk_i2c_writew(i2c, i2c->ltiming_reg, OFFSET_LTIMING);
 
+	if (i2c->dev_comp->timing_adjust) {
+		mtk_i2c_writew(i2c, i2c->ac_timing.ext, OFFSET_EXT_CONF);
+		mtk_i2c_writew(i2c, i2c->ac_timing.inter_clk_div,
+			       OFFSET_CLOCK_DIV);
+		mtk_i2c_writew(i2c, I2C_SCL_MIS_COMP_VALUE,
+			       OFFSET_SCL_MIS_COMP_POINT);
+		mtk_i2c_writew(i2c, i2c->ac_timing.sda_timing,
+			       OFFSET_SDA_TIMING);
+
+		if (i2c->dev_comp->ltiming_adjust) {
+			mtk_i2c_writew(i2c, i2c->ac_timing.htiming,
+				       OFFSET_TIMING);
+			mtk_i2c_writew(i2c, i2c->ac_timing.hs, OFFSET_HS);
+			mtk_i2c_writew(i2c, i2c->ac_timing.ltiming,
+				       OFFSET_LTIMING);
+		} else {
+			mtk_i2c_writew(i2c, i2c->ac_timing.scl_hl_ratio,
+				       OFFSET_SCL_HIGH_LOW_RATIO);
+			mtk_i2c_writew(i2c, i2c->ac_timing.hs_scl_hl_ratio,
+				       OFFSET_HS_SCL_HIGH_LOW_RATIO);
+			mtk_i2c_writew(i2c, i2c->ac_timing.sta_stop,
+				       OFFSET_STA_STO_AC_TIMING);
+			mtk_i2c_writew(i2c, i2c->ac_timing.hs_sta_stop,
+				       OFFSET_HS_STA_STO_AC_TIMING);
+		}
+	}
+
 	/* If use i2c pin from PMIC mt6397 side, need set PATH_DIR first */
 	if (i2c->have_pmic)
 		mtk_i2c_writew(i2c, I2C_CONTROL_WRAPPER, OFFSET_PATH_DIR);
@@ -422,6 +512,125 @@ static void mtk_i2c_init_hw(struct mtk_i2c *i2c)
 	writel(I2C_DMA_CLR_FLAG, i2c->pdmabase + OFFSET_RST);
 }
 
+static const struct i2c_spec_values *mtk_i2c_get_spec(unsigned int speed)
+{
+	if (speed <= I2C_MAX_STANDARD_MODE_FREQ)
+		return &standard_mode_spec;
+	else if (speed <= I2C_MAX_FAST_MODE_FREQ)
+		return &fast_mode_spec;
+	else
+		return &fast_mode_plus_spec;
+}
+
+static int mtk_i2c_max_step_cnt(unsigned int target_speed)
+{
+	if (target_speed > I2C_MAX_FAST_MODE_FREQ)
+		return MAX_HS_STEP_CNT_DIV;
+	else
+		return MAX_STEP_CNT_DIV;
+}
+
+/*
+ * Check and Calculate i2c ac-timing
+ *
+ * Hardware design:
+ * sample_ns = (1000000000 * (sample_cnt + 1)) / clk_src
+ * xxx_cnt_div =  spec->min_xxx_ns / sample_ns
+ *
+ * Sample_ns is rounded down for xxx_cnt_div would be greater
+ * than the smallest spec.
+ * The sda_timing is chosen as the middle value between
+ * the largest and smallest.
+ */
+static int mtk_i2c_check_ac_timing(struct mtk_i2c *i2c,
+				   unsigned int clk_src,
+				   unsigned int check_speed,
+				   unsigned int step_cnt,
+				   unsigned int sample_cnt)
+{
+	const struct i2c_spec_values *spec;
+	unsigned int su_sta_cnt, low_cnt, high_cnt, max_step_cnt;
+	unsigned int sda_max, sda_min, clk_ns, max_sta_cnt = 0x3f;
+	long long sample_ns = (1000000000 * (sample_cnt + 1)) / clk_src;
+
+	if (!i2c->dev_comp->timing_adjust)
+		return 0;
+
+	if (i2c->dev_comp->ltiming_adjust)
+		max_sta_cnt = 0x100;
+
+	spec = mtk_i2c_get_spec(check_speed);
+
+	if (i2c->dev_comp->ltiming_adjust)
+		clk_ns = 1000000000 / clk_src;
+	else
+		clk_ns = sample_ns / 2;
+
+	su_sta_cnt = DIV_ROUND_UP(spec->min_su_sta_ns, clk_ns);
+	if (su_sta_cnt > max_sta_cnt)
+		return -1;
+
+	low_cnt = DIV_ROUND_UP(spec->min_low_ns, sample_ns);
+	max_step_cnt = mtk_i2c_max_step_cnt(check_speed);
+	if ((2 * step_cnt) > low_cnt && low_cnt < max_step_cnt) {
+		if (low_cnt > step_cnt) {
+			high_cnt = 2 * step_cnt - low_cnt;
+		} else {
+			high_cnt = step_cnt;
+			low_cnt = step_cnt;
+		}
+	} else {
+		return -2;
+	}
+
+	sda_max = spec->max_hd_dat_ns / sample_ns;
+	if (sda_max > low_cnt)
+		sda_max = 0;
+
+	sda_min = DIV_ROUND_UP(spec->min_su_dat_ns, sample_ns);
+	if (sda_min < low_cnt)
+		sda_min = 0;
+
+	if (sda_min > sda_max)
+		return -3;
+
+	if (check_speed > I2C_MAX_FAST_MODE_FREQ) {
+		if (i2c->dev_comp->ltiming_adjust) {
+			i2c->ac_timing.hs = I2C_TIME_DEFAULT_VALUE |
+				(sample_cnt << 12) | (high_cnt << 8);
+			i2c->ac_timing.ltiming &= ~GENMASK(15, 9);
+			i2c->ac_timing.ltiming |= (sample_cnt << 12) |
+				(low_cnt << 9);
+			i2c->ac_timing.ext &= ~GENMASK(7, 1);
+			i2c->ac_timing.ext |= (su_sta_cnt << 1) | (1 << 0);
+		} else {
+			i2c->ac_timing.hs_scl_hl_ratio = (1 << 12) |
+				(high_cnt << 6) | low_cnt;
+			i2c->ac_timing.hs_sta_stop = (su_sta_cnt << 8) |
+				su_sta_cnt;
+		}
+		i2c->ac_timing.sda_timing &= ~GENMASK(11, 6);
+		i2c->ac_timing.sda_timing |= (1 << 12) |
+			((sda_max + sda_min) / 2) << 6;
+	} else {
+		if (i2c->dev_comp->ltiming_adjust) {
+			i2c->ac_timing.htiming = (sample_cnt << 8) | (high_cnt);
+			i2c->ac_timing.ltiming = (sample_cnt << 6) | (low_cnt);
+			i2c->ac_timing.ext = (su_sta_cnt << 8) | (1 << 0);
+		} else {
+			i2c->ac_timing.scl_hl_ratio = (1 << 12) |
+				(high_cnt << 6) | low_cnt;
+			i2c->ac_timing.sta_stop = (su_sta_cnt << 8) |
+				su_sta_cnt;
+		}
+
+		i2c->ac_timing.sda_timing = (1 << 12) |
+			(sda_max + sda_min) / 2;
+	}
+
+	return 0;
+}
+
 /*
  * Calculate i2c port speed
  *
@@ -446,15 +655,12 @@ static int mtk_i2c_calculate_speed(struct mtk_i2c *i2c, unsigned int clk_src,
 	unsigned int opt_div;
 	unsigned int best_mul;
 	unsigned int cnt_mul;
+	int ret = -EINVAL;
 
 	if (target_speed > I2C_MAX_FAST_MODE_PLUS_FREQ)
 		target_speed = I2C_MAX_FAST_MODE_PLUS_FREQ;
 
-	if (target_speed > I2C_MAX_FAST_MODE_FREQ)
-		max_step_cnt = MAX_HS_STEP_CNT_DIV;
-	else
-		max_step_cnt = MAX_STEP_CNT_DIV;
-
+	max_step_cnt = mtk_i2c_max_step_cnt(target_speed);
 	base_step_cnt = max_step_cnt;
 	/* Find the best combination */
 	opt_div = DIV_ROUND_UP(clk_src >> 1, target_speed);
@@ -473,6 +679,11 @@ static int mtk_i2c_calculate_speed(struct mtk_i2c *i2c, unsigned int clk_src,
 			continue;
 
 		if (cnt_mul < best_mul) {
+			ret = mtk_i2c_check_ac_timing(i2c, clk_src,
+				target_speed, step_cnt - 1, sample_cnt - 1);
+			if (ret)
+				continue;
+
 			best_mul = cnt_mul;
 			base_sample_cnt = sample_cnt;
 			base_step_cnt = step_cnt;
@@ -481,6 +692,9 @@ static int mtk_i2c_calculate_speed(struct mtk_i2c *i2c, unsigned int clk_src,
 		}
 	}
 
+	if (ret)
+		return -EINVAL;
+
 	sample_cnt = base_sample_cnt;
 	step_cnt = base_step_cnt;
 
@@ -506,47 +720,68 @@ static int mtk_i2c_set_speed(struct mtk_i2c *i2c, unsigned int parent_clk)
 	unsigned int l_step_cnt;
 	unsigned int l_sample_cnt;
 	unsigned int target_speed;
+	unsigned int clk_div;
+	unsigned int max_clk_div;
 	int ret;
 
-	clk_src = parent_clk / i2c->clk_src_div;
 	target_speed = i2c->speed_hz;
+	parent_clk /= i2c->clk_src_div;
 
-	if (target_speed > I2C_MAX_FAST_MODE_FREQ) {
-		/* Set master code speed register */
-		ret = mtk_i2c_calculate_speed(i2c, clk_src, I2C_MAX_FAST_MODE_FREQ,
-					      &l_step_cnt, &l_sample_cnt);
-		if (ret < 0)
-			return ret;
-
-		i2c->timing_reg = (l_sample_cnt << 8) | l_step_cnt;
-
-		/* Set the high speed mode register */
-		ret = mtk_i2c_calculate_speed(i2c, clk_src, target_speed,
-					      &step_cnt, &sample_cnt);
-		if (ret < 0)
-			return ret;
-
-		i2c->high_speed_reg = I2C_TIME_DEFAULT_VALUE |
-			(sample_cnt << 12) | (step_cnt << 8);
+	if (i2c->dev_comp->timing_adjust)
+		max_clk_div = MAX_CLOCK_DIV;
+	else
+		max_clk_div = 1;
+
+	for (clk_div = 1; clk_div <= max_clk_div; clk_div++) {
+		clk_src = parent_clk / clk_div;
+
+		if (target_speed > I2C_MAX_FAST_MODE_FREQ) {
+			/* Set master code speed register */
+			ret = mtk_i2c_calculate_speed(i2c, clk_src,
+						      I2C_MAX_FAST_MODE_FREQ,
+						      &l_step_cnt,
+						      &l_sample_cnt);
+			if (ret < 0)
+				continue;
+
+			i2c->timing_reg = (l_sample_cnt << 8) | l_step_cnt;
+
+			/* Set the high speed mode register */
+			ret = mtk_i2c_calculate_speed(i2c, clk_src,
+						      target_speed, &step_cnt,
+						      &sample_cnt);
+			if (ret < 0)
+				continue;
+
+			i2c->high_speed_reg = I2C_TIME_DEFAULT_VALUE |
+					(sample_cnt << 12) | (step_cnt << 8);
+
+			if (i2c->dev_comp->ltiming_adjust)
+				i2c->ltiming_reg =
+					(l_sample_cnt << 6) | l_step_cnt |
+					(sample_cnt << 12) | (step_cnt << 9);
+		} else {
+			ret = mtk_i2c_calculate_speed(i2c, clk_src,
+						      target_speed, &l_step_cnt,
+						      &l_sample_cnt);
+			if (ret < 0)
+				continue;
 
-		if (i2c->dev_comp->ltiming_adjust)
-			i2c->ltiming_reg = (l_sample_cnt << 6) | l_step_cnt |
-					   (sample_cnt << 12) | (step_cnt << 9);
-	} else {
-		ret = mtk_i2c_calculate_speed(i2c, clk_src, target_speed,
-					      &step_cnt, &sample_cnt);
-		if (ret < 0)
-			return ret;
+			i2c->timing_reg = (l_sample_cnt << 8) | l_step_cnt;
 
-		i2c->timing_reg = (sample_cnt << 8) | step_cnt;
+			/* Disable the high speed transaction */
+			i2c->high_speed_reg = I2C_TIME_CLR_VALUE;
 
-		/* Disable the high speed transaction */
-		i2c->high_speed_reg = I2C_TIME_CLR_VALUE;
+			if (i2c->dev_comp->ltiming_adjust)
+				i2c->ltiming_reg =
+					(l_sample_cnt << 6) | l_step_cnt;
+		}
 
-		if (i2c->dev_comp->ltiming_adjust)
-			i2c->ltiming_reg = (sample_cnt << 6) | step_cnt;
+		break;
 	}
 
+	i2c->ac_timing.inter_clk_div = clk_div - 1;
+
 	return 0;
 }
 
@@ -586,12 +821,6 @@ static int mtk_i2c_do_transfer(struct mtk_i2c *i2c, struct i2c_msg *msgs,
 
 	mtk_i2c_writew(i2c, control_reg, OFFSET_CONTROL);
 
-	/* set start condition */
-	if (i2c->speed_hz <= I2C_MAX_STANDARD_MODE_FREQ)
-		mtk_i2c_writew(i2c, I2C_ST_START_CON, OFFSET_EXT_CONF);
-	else
-		mtk_i2c_writew(i2c, I2C_FS_START_CON, OFFSET_EXT_CONF);
-
 	addr_reg = i2c_8bit_addr_from_msg(msgs);
 	mtk_i2c_writew(i2c, addr_reg, OFFSET_SLAVE_ADDR);
 
@@ -948,9 +1177,6 @@ static int mtk_i2c_probe(struct platform_device *pdev)
 	if (ret)
 		return -EINVAL;
 
-	if (i2c->dev_comp->timing_adjust)
-		i2c->clk_src_div *= I2C_DEFAULT_CLK_DIV;
-
 	if (i2c->have_pmic && !i2c->dev_comp->pmic_i2c)
 		return -EINVAL;
 
-- 
1.9.1

Re: [PATCH v2 1/2] MAINTAINERS: add maintainer for mediatek i2c controller driver

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 233 bytes --]

On Thu, May 14, 2020 at 09:09:04PM +0800, Qii Wang wrote:
> Add Qii Wang as maintainer for mediatek i2c controller driver.
> 
> Signed-off-by: Qii Wang <qii.wang@mediatek.com>

Applied to for-current, thanks for stepping up!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/2] i2c: mediatek: Add i2c ac-timing adjust support

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 252 bytes --]

On Thu, May 14, 2020 at 09:09:05PM +0800, Qii Wang wrote:
> This patch adds a algorithm to calculate some ac-timing parameters
> which can fully meet I2C Spec.
> 
> Signed-off-by: Qii Wang <qii.wang@mediatek.com>

Applied to for-next, thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/2] i2c: mediatek: Add i2c ac-timing adjust support

[Geert Uytterhoeven]

On Thu, May 14, 2020 at 3:13 PM Qii Wang <qii.wang@mediatek.com> wrote:
> This patch adds a algorithm to calculate some ac-timing parameters
> which can fully meet I2C Spec.
>
> Signed-off-by: Qii Wang <qii.wang@mediatek.com>
> ---
>  drivers/i2c/busses/i2c-mt65xx.c | 328 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 277 insertions(+), 51 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> index 0ca6c38a..7020618 100644
> --- a/drivers/i2c/busses/i2c-mt65xx.c
> +++ b/drivers/i2c/busses/i2c-mt65xx.c

> +/*
> + * Check and Calculate i2c ac-timing
> + *
> + * Hardware design:
> + * sample_ns = (1000000000 * (sample_cnt + 1)) / clk_src
> + * xxx_cnt_div =  spec->min_xxx_ns / sample_ns
> + *
> + * Sample_ns is rounded down for xxx_cnt_div would be greater
> + * than the smallest spec.
> + * The sda_timing is chosen as the middle value between
> + * the largest and smallest.
> + */
> +static int mtk_i2c_check_ac_timing(struct mtk_i2c *i2c,
> +                                  unsigned int clk_src,
> +                                  unsigned int check_speed,
> +                                  unsigned int step_cnt,
> +                                  unsigned int sample_cnt)
> +{
> +       const struct i2c_spec_values *spec;
> +       unsigned int su_sta_cnt, low_cnt, high_cnt, max_step_cnt;
> +       unsigned int sda_max, sda_min, clk_ns, max_sta_cnt = 0x3f;
> +       long long sample_ns = (1000000000 * (sample_cnt + 1)) / clk_src;

So sample_ns is a 64-bit value. Is that really needed?

> +       if (!i2c->dev_comp->timing_adjust)
> +               return 0;
> +
> +       if (i2c->dev_comp->ltiming_adjust)
> +               max_sta_cnt = 0x100;
> +
> +       spec = mtk_i2c_get_spec(check_speed);
> +
> +       if (i2c->dev_comp->ltiming_adjust)
> +               clk_ns = 1000000000 / clk_src;
> +       else
> +               clk_ns = sample_ns / 2;
> +
> +       su_sta_cnt = DIV_ROUND_UP(spec->min_su_sta_ns, clk_ns);
> +       if (su_sta_cnt > max_sta_cnt)
> +               return -1;
> +
> +       low_cnt = DIV_ROUND_UP(spec->min_low_ns, sample_ns);

So this is a 32-bit by 64-bit division (indeed, not 64-by-32!)

noreply@ellerman.id.au reports:

    ERROR: modpost: "__udivdi3" [drivers/i2c/busses/i2c-mt65xx.ko] undefined!
    ERROR: modpost: "__divdi3" [drivers/i2c/busses/i2c-mt65xx.ko] undefined!

for 32-bit builds.

> +       max_step_cnt = mtk_i2c_max_step_cnt(check_speed);
> +       if ((2 * step_cnt) > low_cnt && low_cnt < max_step_cnt) {
> +               if (low_cnt > step_cnt) {
> +                       high_cnt = 2 * step_cnt - low_cnt;
> +               } else {
> +                       high_cnt = step_cnt;
> +                       low_cnt = step_cnt;
> +               }
> +       } else {
> +               return -2;
> +       }
> +
> +       sda_max = spec->max_hd_dat_ns / sample_ns;
> +       if (sda_max > low_cnt)
> +               sda_max = 0;
> +
> +       sda_min = DIV_ROUND_UP(spec->min_su_dat_ns, sample_ns);

One more.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/2] i2c: mediatek: Add i2c ac-timing adjust support

[Qii Wang]

On Mon, 2020-05-18 at 17:44 +0200, Geert Uytterhoeven wrote:
> On Thu, May 14, 2020 at 3:13 PM Qii Wang <qii.wang@mediatek.com> wrote:
> > This patch adds a algorithm to calculate some ac-timing parameters
> > which can fully meet I2C Spec.
> >
> > Signed-off-by: Qii Wang <qii.wang@mediatek.com>
> > ---
> >  drivers/i2c/busses/i2c-mt65xx.c | 328 +++++++++++++++++++++++++++++++++-------
> >  1 file changed, 277 insertions(+), 51 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> > index 0ca6c38a..7020618 100644
> > --- a/drivers/i2c/busses/i2c-mt65xx.c
> > +++ b/drivers/i2c/busses/i2c-mt65xx.c
> 
> > +/*
> > + * Check and Calculate i2c ac-timing
> > + *
> > + * Hardware design:
> > + * sample_ns = (1000000000 * (sample_cnt + 1)) / clk_src
> > + * xxx_cnt_div =  spec->min_xxx_ns / sample_ns
> > + *
> > + * Sample_ns is rounded down for xxx_cnt_div would be greater
> > + * than the smallest spec.
> > + * The sda_timing is chosen as the middle value between
> > + * the largest and smallest.
> > + */
> > +static int mtk_i2c_check_ac_timing(struct mtk_i2c *i2c,
> > +                                  unsigned int clk_src,
> > +                                  unsigned int check_speed,
> > +                                  unsigned int step_cnt,
> > +                                  unsigned int sample_cnt)
> > +{
> > +       const struct i2c_spec_values *spec;
> > +       unsigned int su_sta_cnt, low_cnt, high_cnt, max_step_cnt;
> > +       unsigned int sda_max, sda_min, clk_ns, max_sta_cnt = 0x3f;
> > +       long long sample_ns = (1000000000 * (sample_cnt + 1)) / clk_src;
> 
> So sample_ns is a 64-bit value. Is that really needed?
> 

(1000000000 * (sample_cnt + 1)) / clk_src value is a 32-bit, (1000000000
* (sample_cnt + 1)) will over 32-bit if sample_cnt is 7.

I think 1000000000 and clk_src is too big, maybe I can reduce then with
be divided all by 1000.
example:

unsigned int sample_ns;
unsigned int clk_src_khz = clk_src / 1000;

if(clk_src_khz)
	sample_ns = (1000000 * (sample_cnt + 1)) / clk_src_khz;
else
	return -EINVAL;

> > +       if (!i2c->dev_comp->timing_adjust)
> > +               return 0;
> > +
> > +       if (i2c->dev_comp->ltiming_adjust)
> > +               max_sta_cnt = 0x100;
> > +
> > +       spec = mtk_i2c_get_spec(check_speed);
> > +
> > +       if (i2c->dev_comp->ltiming_adjust)
> > +               clk_ns = 1000000000 / clk_src;
> > +       else
> > +               clk_ns = sample_ns / 2;
> > +
> > +       su_sta_cnt = DIV_ROUND_UP(spec->min_su_sta_ns, clk_ns);
> > +       if (su_sta_cnt > max_sta_cnt)
> > +               return -1;
> > +
> > +       low_cnt = DIV_ROUND_UP(spec->min_low_ns, sample_ns);
> 
> So this is a 32-bit by 64-bit division (indeed, not 64-by-32!)
> 
> noreply@ellerman.id.au reports:
> 
>     ERROR: modpost: "__udivdi3" [drivers/i2c/busses/i2c-mt65xx.ko] undefined!
>     ERROR: modpost: "__divdi3" [drivers/i2c/busses/i2c-mt65xx.ko] undefined!
> 
> for 32-bit builds.
> 
> > +       max_step_cnt = mtk_i2c_max_step_cnt(check_speed);
> > +       if ((2 * step_cnt) > low_cnt && low_cnt < max_step_cnt) {
> > +               if (low_cnt > step_cnt) {
> > +                       high_cnt = 2 * step_cnt - low_cnt;
> > +               } else {
> > +                       high_cnt = step_cnt;
> > +                       low_cnt = step_cnt;
> > +               }
> > +       } else {
> > +               return -2;
> > +       }
> > +
> > +       sda_max = spec->max_hd_dat_ns / sample_ns;
> > +       if (sda_max > low_cnt)
> > +               sda_max = 0;
> > +
> > +       sda_min = DIV_ROUND_UP(spec->min_su_dat_ns, sample_ns);
> 
> One more.
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds

Re: [PATCH v2 2/2] i2c: mediatek: Add i2c ac-timing adjust support

[Geert Uytterhoeven]

Hi Qii,

On Tue, May 19, 2020 at 4:59 AM Qii Wang <qii.wang@mediatek.com> wrote:
> On Mon, 2020-05-18 at 17:44 +0200, Geert Uytterhoeven wrote:
> > On Thu, May 14, 2020 at 3:13 PM Qii Wang <qii.wang@mediatek.com> wrote:
> > > This patch adds a algorithm to calculate some ac-timing parameters
> > > which can fully meet I2C Spec.
> > >
> > > Signed-off-by: Qii Wang <qii.wang@mediatek.com>
> > > ---
> > >  drivers/i2c/busses/i2c-mt65xx.c | 328 +++++++++++++++++++++++++++++++++-------
> > >  1 file changed, 277 insertions(+), 51 deletions(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> > > index 0ca6c38a..7020618 100644
> > > --- a/drivers/i2c/busses/i2c-mt65xx.c
> > > +++ b/drivers/i2c/busses/i2c-mt65xx.c
> >
> > > +/*
> > > + * Check and Calculate i2c ac-timing
> > > + *
> > > + * Hardware design:
> > > + * sample_ns = (1000000000 * (sample_cnt + 1)) / clk_src
> > > + * xxx_cnt_div =  spec->min_xxx_ns / sample_ns
> > > + *
> > > + * Sample_ns is rounded down for xxx_cnt_div would be greater
> > > + * than the smallest spec.
> > > + * The sda_timing is chosen as the middle value between
> > > + * the largest and smallest.
> > > + */
> > > +static int mtk_i2c_check_ac_timing(struct mtk_i2c *i2c,
> > > +                                  unsigned int clk_src,
> > > +                                  unsigned int check_speed,
> > > +                                  unsigned int step_cnt,
> > > +                                  unsigned int sample_cnt)
> > > +{
> > > +       const struct i2c_spec_values *spec;
> > > +       unsigned int su_sta_cnt, low_cnt, high_cnt, max_step_cnt;
> > > +       unsigned int sda_max, sda_min, clk_ns, max_sta_cnt = 0x3f;
> > > +       long long sample_ns = (1000000000 * (sample_cnt + 1)) / clk_src;
> >
> > So sample_ns is a 64-bit value. Is that really needed?
> >
>
> (1000000000 * (sample_cnt + 1)) / clk_src value is a 32-bit, (1000000000
> * (sample_cnt + 1)) will over 32-bit if sample_cnt is 7.

The intermediate value will indeed not fit in 32-bit.
But that doesn't mean the end result won't fit in 32-bit.
As you divide spec->min_low_ns and spec->min_su_dat_ns (which I assume
are small numbers) by sample_ns below, sample_ns cannot be very large,
or the quotient will be zero anyway.
So just doing the multiplication in 64-bit, followed by a 64-by-32
division is probably fine:

    unsigned int sample_ns = div_u64(1000000000ULL * (sample_cnt + 1), clk_src);

You may want to take precautions for the case where the passed value of
clk_src is a small number (can that happen?).

BTW, clk_get_rate() returns "unsigned long", while mtk_i2c_set_speed()
takes an "unsigned int" parent_clk, which may cause future issues.
You may want to change that to "unsigned long", along the whole
propagation path, and use div64_ul() instead of div_u64() above.

> I think 1000000000 and clk_src is too big, maybe I can reduce then with
> be divided all by 1000.
> example:
>
> unsigned int sample_ns;
> unsigned int clk_src_khz = clk_src / 1000;

That may cause too much loss of precision.

>
> if(clk_src_khz)
>         sample_ns = (1000000 * (sample_cnt + 1)) / clk_src_khz;
> else
>         return -EINVAL;
>
> > > +       if (!i2c->dev_comp->timing_adjust)
> > > +               return 0;
> > > +
> > > +       if (i2c->dev_comp->ltiming_adjust)
> > > +               max_sta_cnt = 0x100;
> > > +
> > > +       spec = mtk_i2c_get_spec(check_speed);
> > > +
> > > +       if (i2c->dev_comp->ltiming_adjust)
> > > +               clk_ns = 1000000000 / clk_src;
> > > +       else
> > > +               clk_ns = sample_ns / 2;
> > > +
> > > +       su_sta_cnt = DIV_ROUND_UP(spec->min_su_sta_ns, clk_ns);
> > > +       if (su_sta_cnt > max_sta_cnt)
> > > +               return -1;
> > > +
> > > +       low_cnt = DIV_ROUND_UP(spec->min_low_ns, sample_ns);
> >
> > So this is a 32-bit by 64-bit division (indeed, not 64-by-32!)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/2] i2c: mediatek: Add i2c ac-timing adjust support

[Valdis Klētnieks]

[-- Attachment #1: Type: text/plain, Size: 599 bytes --]

On Tue, 19 May 2020 10:57:53 +0800, Qii Wang said:

> (1000000000 * (sample_cnt + 1)) / clk_src value is a 32-bit, (1000000000
> * (sample_cnt + 1)) will over 32-bit if sample_cnt is 7.
>
> I think 1000000000 and clk_src is too big, maybe I can reduce then with
> be divided all by 1000.

Yes, it's definitely too big, the 3 DIV_ROUND_UP calls in  mtk_i2c_check_ac_timing()
end up causing a build issue during modpost on a 32-bit RPi4:

ERROR: modpost: "__aeabi_uldivmod" [drivers/i2c/busses/i2c-mt65xx.ko] undefined!
ERROR: modpost: "__aeabi_ldivmod" [drivers/i2c/busses/i2c-mt65xx.ko] undefined!


[-- Attachment #2: Type: application/pgp-signature, Size: 832 bytes --]

Re: [PATCH v2 2/2] i2c: mediatek: Add i2c ac-timing adjust support

[Qii Wang]

Hi Geert,

On Tue, 2020-05-19 at 09:14 +0200, Geert Uytterhoeven wrote:
> Hi Qii,
> 
> On Tue, May 19, 2020 at 4:59 AM Qii Wang <qii.wang@mediatek.com> wrote:
> > On Mon, 2020-05-18 at 17:44 +0200, Geert Uytterhoeven wrote:
> > > On Thu, May 14, 2020 at 3:13 PM Qii Wang <qii.wang@mediatek.com> wrote:
> > > > This patch adds a algorithm to calculate some ac-timing parameters
> > > > which can fully meet I2C Spec.
> > > >
> > > > Signed-off-by: Qii Wang <qii.wang@mediatek.com>
> > > > ---
> > > >  drivers/i2c/busses/i2c-mt65xx.c | 328 +++++++++++++++++++++++++++++++++-------
> > > >  1 file changed, 277 insertions(+), 51 deletions(-)
> > > >
> > > > diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> > > > index 0ca6c38a..7020618 100644
> > > > --- a/drivers/i2c/busses/i2c-mt65xx.c
> > > > +++ b/drivers/i2c/busses/i2c-mt65xx.c
> > >
> > > > +/*
> > > > + * Check and Calculate i2c ac-timing
> > > > + *
> > > > + * Hardware design:
> > > > + * sample_ns = (1000000000 * (sample_cnt + 1)) / clk_src
> > > > + * xxx_cnt_div =  spec->min_xxx_ns / sample_ns
> > > > + *
> > > > + * Sample_ns is rounded down for xxx_cnt_div would be greater
> > > > + * than the smallest spec.
> > > > + * The sda_timing is chosen as the middle value between
> > > > + * the largest and smallest.
> > > > + */
> > > > +static int mtk_i2c_check_ac_timing(struct mtk_i2c *i2c,
> > > > +                                  unsigned int clk_src,
> > > > +                                  unsigned int check_speed,
> > > > +                                  unsigned int step_cnt,
> > > > +                                  unsigned int sample_cnt)
> > > > +{
> > > > +       const struct i2c_spec_values *spec;
> > > > +       unsigned int su_sta_cnt, low_cnt, high_cnt, max_step_cnt;
> > > > +       unsigned int sda_max, sda_min, clk_ns, max_sta_cnt = 0x3f;
> > > > +       long long sample_ns = (1000000000 * (sample_cnt + 1)) / clk_src;
> > >
> > > So sample_ns is a 64-bit value. Is that really needed?
> > >
> >
> > (1000000000 * (sample_cnt + 1)) / clk_src value is a 32-bit, (1000000000
> > * (sample_cnt + 1)) will over 32-bit if sample_cnt is 7.
> 
> The intermediate value will indeed not fit in 32-bit.
> But that doesn't mean the end result won't fit in 32-bit.
> As you divide spec->min_low_ns and spec->min_su_dat_ns (which I assume
> are small numbers) by sample_ns below, sample_ns cannot be very large,
> or the quotient will be zero anyway.
> So just doing the multiplication in 64-bit, followed by a 64-by-32
> division is probably fine:
> 
>     unsigned int sample_ns = div_u64(1000000000ULL * (sample_cnt + 1), clk_src);
> 
> You may want to take precautions for the case where the passed value of
> clk_src is a small number (can that happen?).
> 
> BTW, clk_get_rate() returns "unsigned long", while mtk_i2c_set_speed()
> takes an "unsigned int" parent_clk, which may cause future issues.
> You may want to change that to "unsigned long", along the whole
> propagation path, and use div64_ul() instead of div_u64() above.
> 

The return type of div_u64 is u64(unsigned long long), there is a
compulsory type conversion operator. Do you think it is needed?
BTW, we just need to change the type of sample_ns to unsigned int, no
matter which method is used, what is your opinion?

> > I think 1000000000 and clk_src is too big, maybe I can reduce then with
> > be divided all by 1000.
> > example:
> >
> > unsigned int sample_ns;
> > unsigned int clk_src_khz = clk_src / 1000;
> 
> That may cause too much loss of precision.
> 

clk_src is more than MHz and less than GHZ for MTK i2c controller, so it
wouldn't cause too much loss of precision.

> >
> > if(clk_src_khz)
> >         sample_ns = (1000000 * (sample_cnt + 1)) / clk_src_khz;
> > else
> >         return -EINVAL;
> >
> > > > +       if (!i2c->dev_comp->timing_adjust)
> > > > +               return 0;
> > > > +
> > > > +       if (i2c->dev_comp->ltiming_adjust)
> > > > +               max_sta_cnt = 0x100;
> > > > +
> > > > +       spec = mtk_i2c_get_spec(check_speed);
> > > > +
> > > > +       if (i2c->dev_comp->ltiming_adjust)
> > > > +               clk_ns = 1000000000 / clk_src;
> > > > +       else
> > > > +               clk_ns = sample_ns / 2;
> > > > +
> > > > +       su_sta_cnt = DIV_ROUND_UP(spec->min_su_sta_ns, clk_ns);
> > > > +       if (su_sta_cnt > max_sta_cnt)
> > > > +               return -1;
> > > > +
> > > > +       low_cnt = DIV_ROUND_UP(spec->min_low_ns, sample_ns);
> > >
> > > So this is a 32-bit by 64-bit division (indeed, not 64-by-32!)
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH v2 2/2] i2c: mediatek: Add i2c ac-timing adjust support

[Geert Uytterhoeven]

Hi Qii,

On Wed, May 20, 2020 at 10:44 AM Qii Wang <qii.wang@mediatek.com> wrote:
> On Tue, 2020-05-19 at 09:14 +0200, Geert Uytterhoeven wrote:
> > On Tue, May 19, 2020 at 4:59 AM Qii Wang <qii.wang@mediatek.com> wrote:
> > > On Mon, 2020-05-18 at 17:44 +0200, Geert Uytterhoeven wrote:
> > > > On Thu, May 14, 2020 at 3:13 PM Qii Wang <qii.wang@mediatek.com> wrote:
> > > > > This patch adds a algorithm to calculate some ac-timing parameters
> > > > > which can fully meet I2C Spec.
> > > > >
> > > > > Signed-off-by: Qii Wang <qii.wang@mediatek.com>
> > > > > ---
> > > > >  drivers/i2c/busses/i2c-mt65xx.c | 328 +++++++++++++++++++++++++++++++++-------
> > > > >  1 file changed, 277 insertions(+), 51 deletions(-)
> > > > >
> > > > > diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> > > > > index 0ca6c38a..7020618 100644
> > > > > --- a/drivers/i2c/busses/i2c-mt65xx.c
> > > > > +++ b/drivers/i2c/busses/i2c-mt65xx.c
> > > >
> > > > > +/*
> > > > > + * Check and Calculate i2c ac-timing
> > > > > + *
> > > > > + * Hardware design:
> > > > > + * sample_ns = (1000000000 * (sample_cnt + 1)) / clk_src
> > > > > + * xxx_cnt_div =  spec->min_xxx_ns / sample_ns
> > > > > + *
> > > > > + * Sample_ns is rounded down for xxx_cnt_div would be greater
> > > > > + * than the smallest spec.
> > > > > + * The sda_timing is chosen as the middle value between
> > > > > + * the largest and smallest.
> > > > > + */
> > > > > +static int mtk_i2c_check_ac_timing(struct mtk_i2c *i2c,
> > > > > +                                  unsigned int clk_src,
> > > > > +                                  unsigned int check_speed,
> > > > > +                                  unsigned int step_cnt,
> > > > > +                                  unsigned int sample_cnt)
> > > > > +{
> > > > > +       const struct i2c_spec_values *spec;
> > > > > +       unsigned int su_sta_cnt, low_cnt, high_cnt, max_step_cnt;
> > > > > +       unsigned int sda_max, sda_min, clk_ns, max_sta_cnt = 0x3f;
> > > > > +       long long sample_ns = (1000000000 * (sample_cnt + 1)) / clk_src;
> > > >
> > > > So sample_ns is a 64-bit value. Is that really needed?
> > > >
> > >
> > > (1000000000 * (sample_cnt + 1)) / clk_src value is a 32-bit, (1000000000
> > > * (sample_cnt + 1)) will over 32-bit if sample_cnt is 7.
> >
> > The intermediate value will indeed not fit in 32-bit.
> > But that doesn't mean the end result won't fit in 32-bit.
> > As you divide spec->min_low_ns and spec->min_su_dat_ns (which I assume
> > are small numbers) by sample_ns below, sample_ns cannot be very large,
> > or the quotient will be zero anyway.
> > So just doing the multiplication in 64-bit, followed by a 64-by-32
> > division is probably fine:
> >
> >     unsigned int sample_ns = div_u64(1000000000ULL * (sample_cnt + 1), clk_src);
> >
> > You may want to take precautions for the case where the passed value of
> > clk_src is a small number (can that happen?).
> >
> > BTW, clk_get_rate() returns "unsigned long", while mtk_i2c_set_speed()
> > takes an "unsigned int" parent_clk, which may cause future issues.
> > You may want to change that to "unsigned long", along the whole
> > propagation path, and use div64_ul() instead of div_u64() above.
> >
>
> The return type of div_u64 is u64(unsigned long long), there is a
> compulsory type conversion operator. Do you think it is needed?

The result of a 64-by-32 bit division may indeed not fit in 32-bit, so that's
why it returns u64.
If you know the quotient will always fit, it's fine.

> BTW, we just need to change the type of sample_ns to unsigned int, no
> matter which method is used, what is your opinion?

Indeed.

BTW, I just realize

    long long sample_ns = (1000000000 * (sample_cnt + 1)) / clk_src;

wasn't doing what you wanted anyway, as 1000000000 is (implicit) int,
and sample_cnt is unsigned int, so the multiplication was done in 32-bit,
possible leading to a truncation.  Hence that division was done in 32-bit, too,
that's why I didn't notice a call to __udivdi3() in the assembler output here.

So you have to force the multiplication to be done in 64-bit, e.g.
by changing the constant to 1000000000ULL, and use div_u64() for
the division.

>
> > > I think 1000000000 and clk_src is too big, maybe I can reduce then with
> > > be divided all by 1000.
> > > example:
> > >
> > > unsigned int sample_ns;
> > > unsigned int clk_src_khz = clk_src / 1000;
> >
> > That may cause too much loss of precision.
> >
>
> clk_src is more than MHz and less than GHZ for MTK i2c controller, so it
> wouldn't cause too much loss of precision.

OK, so that would work, too.

> > >
> > > if(clk_src_khz)
> > >         sample_ns = (1000000 * (sample_cnt + 1)) / clk_src_khz;
> > > else
> > >         return -EINVAL;
> > >
> > > > > +       if (!i2c->dev_comp->timing_adjust)
> > > > > +               return 0;
> > > > > +
> > > > > +       if (i2c->dev_comp->ltiming_adjust)
> > > > > +               max_sta_cnt = 0x100;
> > > > > +
> > > > > +       spec = mtk_i2c_get_spec(check_speed);
> > > > > +
> > > > > +       if (i2c->dev_comp->ltiming_adjust)
> > > > > +               clk_ns = 1000000000 / clk_src;
> > > > > +       else
> > > > > +               clk_ns = sample_ns / 2;
> > > > > +
> > > > > +       su_sta_cnt = DIV_ROUND_UP(spec->min_su_sta_ns, clk_ns);
> > > > > +       if (su_sta_cnt > max_sta_cnt)
> > > > > +               return -1;
> > > > > +
> > > > > +       low_cnt = DIV_ROUND_UP(spec->min_low_ns, sample_ns);
> > > >
> > > > So this is a 32-bit by 64-bit division (indeed, not 64-by-32!)

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v2 2/2] i2c: mediatek: Add i2c ac-timing adjust support

[Qii Wang]

Hi Geert,

On Wed, 2020-05-20 at 10:58 +0200, Geert Uytterhoeven wrote:
> Hi Qii,
> 
> On Wed, May 20, 2020 at 10:44 AM Qii Wang <qii.wang@mediatek.com> wrote:
> > On Tue, 2020-05-19 at 09:14 +0200, Geert Uytterhoeven wrote:
> > > On Tue, May 19, 2020 at 4:59 AM Qii Wang <qii.wang@mediatek.com> wrote:
> > > > On Mon, 2020-05-18 at 17:44 +0200, Geert Uytterhoeven wrote:
> > > > > On Thu, May 14, 2020 at 3:13 PM Qii Wang <qii.wang@mediatek.com> wrote:
> > > > > > This patch adds a algorithm to calculate some ac-timing parameters
> > > > > > which can fully meet I2C Spec.
> > > > > >
> > > > > > Signed-off-by: Qii Wang <qii.wang@mediatek.com>
> > > > > > ---
> > > > > >  drivers/i2c/busses/i2c-mt65xx.c | 328 +++++++++++++++++++++++++++++++++-------
> > > > > >  1 file changed, 277 insertions(+), 51 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> > > > > > index 0ca6c38a..7020618 100644
> > > > > > --- a/drivers/i2c/busses/i2c-mt65xx.c
> > > > > > +++ b/drivers/i2c/busses/i2c-mt65xx.c
> > > > >
> > > > > > +/*
> > > > > > + * Check and Calculate i2c ac-timing
> > > > > > + *
> > > > > > + * Hardware design:
> > > > > > + * sample_ns = (1000000000 * (sample_cnt + 1)) / clk_src
> > > > > > + * xxx_cnt_div =  spec->min_xxx_ns / sample_ns
> > > > > > + *
> > > > > > + * Sample_ns is rounded down for xxx_cnt_div would be greater
> > > > > > + * than the smallest spec.
> > > > > > + * The sda_timing is chosen as the middle value between
> > > > > > + * the largest and smallest.
> > > > > > + */
> > > > > > +static int mtk_i2c_check_ac_timing(struct mtk_i2c *i2c,
> > > > > > +                                  unsigned int clk_src,
> > > > > > +                                  unsigned int check_speed,
> > > > > > +                                  unsigned int step_cnt,
> > > > > > +                                  unsigned int sample_cnt)
> > > > > > +{
> > > > > > +       const struct i2c_spec_values *spec;
> > > > > > +       unsigned int su_sta_cnt, low_cnt, high_cnt, max_step_cnt;
> > > > > > +       unsigned int sda_max, sda_min, clk_ns, max_sta_cnt = 0x3f;
> > > > > > +       long long sample_ns = (1000000000 * (sample_cnt + 1)) / clk_src;
> > > > >
> > > > > So sample_ns is a 64-bit value. Is that really needed?
> > > > >
> > > >
> > > > (1000000000 * (sample_cnt + 1)) / clk_src value is a 32-bit, (1000000000
> > > > * (sample_cnt + 1)) will over 32-bit if sample_cnt is 7.
> > >
> > > The intermediate value will indeed not fit in 32-bit.
> > > But that doesn't mean the end result won't fit in 32-bit.
> > > As you divide spec->min_low_ns and spec->min_su_dat_ns (which I assume
> > > are small numbers) by sample_ns below, sample_ns cannot be very large,
> > > or the quotient will be zero anyway.
> > > So just doing the multiplication in 64-bit, followed by a 64-by-32
> > > division is probably fine:
> > >
> > >     unsigned int sample_ns = div_u64(1000000000ULL * (sample_cnt + 1), clk_src);
> > >
> > > You may want to take precautions for the case where the passed value of
> > > clk_src is a small number (can that happen?).
> > >
> > > BTW, clk_get_rate() returns "unsigned long", while mtk_i2c_set_speed()
> > > takes an "unsigned int" parent_clk, which may cause future issues.
> > > You may want to change that to "unsigned long", along the whole
> > > propagation path, and use div64_ul() instead of div_u64() above.
> > >
> >
> > The return type of div_u64 is u64(unsigned long long), there is a
> > compulsory type conversion operator. Do you think it is needed?
> 
> The result of a 64-by-32 bit division may indeed not fit in 32-bit, so that's
> why it returns u64.
> If you know the quotient will always fit, it's fine.
> 
> > BTW, we just need to change the type of sample_ns to unsigned int, no
> > matter which method is used, what is your opinion?
> 
> Indeed.
> 
> BTW, I just realize
> 
>     long long sample_ns = (1000000000 * (sample_cnt + 1)) / clk_src;
> 
> wasn't doing what you wanted anyway, as 1000000000 is (implicit) int,
> and sample_cnt is unsigned int, so the multiplication was done in 32-bit,
> possible leading to a truncation.  Hence that division was done in 32-bit, too,
> that's why I didn't notice a call to __udivdi3() in the assembler output here.
> 
> So you have to force the multiplication to be done in 64-bit, e.g.
> by changing the constant to 1000000000ULL, and use div_u64() for
> the division.
> 

ok, I will give a patch with your way, thanks for your opinion.

> >
> > > > I think 1000000000 and clk_src is too big, maybe I can reduce then with
> > > > be divided all by 1000.
> > > > example:
> > > >
> > > > unsigned int sample_ns;
> > > > unsigned int clk_src_khz = clk_src / 1000;
> > >
> > > That may cause too much loss of precision.
> > >
> >
> > clk_src is more than MHz and less than GHZ for MTK i2c controller, so it
> > wouldn't cause too much loss of precision.
> 
> OK, so that would work, too.
> 
> > > >
> > > > if(clk_src_khz)
> > > >         sample_ns = (1000000 * (sample_cnt + 1)) / clk_src_khz;
> > > > else
> > > >         return -EINVAL;
> > > >
> > > > > > +       if (!i2c->dev_comp->timing_adjust)
> > > > > > +               return 0;
> > > > > > +
> > > > > > +       if (i2c->dev_comp->ltiming_adjust)
> > > > > > +               max_sta_cnt = 0x100;
> > > > > > +
> > > > > > +       spec = mtk_i2c_get_spec(check_speed);
> > > > > > +
> > > > > > +       if (i2c->dev_comp->ltiming_adjust)
> > > > > > +               clk_ns = 1000000000 / clk_src;
> > > > > > +       else
> > > > > > +               clk_ns = sample_ns / 2;
> > > > > > +
> > > > > > +       su_sta_cnt = DIV_ROUND_UP(spec->min_su_sta_ns, clk_ns);
> > > > > > +       if (su_sta_cnt > max_sta_cnt)
> > > > > > +               return -1;
> > > > > > +
> > > > > > +       low_cnt = DIV_ROUND_UP(spec->min_low_ns, sample_ns);
> > > > >
> > > > > So this is a 32-bit by 64-bit division (indeed, not 64-by-32!)
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 

Re: [PATCH v2 2/2] i2c: mediatek: Add i2c ac-timing adjust support

[Yingjoe Chen]

Sorry for late review.


On Thu, 2020-05-14 at 21:09 +0800, Qii Wang wrote:
> This patch adds a algorithm to calculate some ac-timing parameters
> which can fully meet I2C Spec.
> 
> Signed-off-by: Qii Wang <qii.wang@mediatek.com>
> ---
>  drivers/i2c/busses/i2c-mt65xx.c | 328 +++++++++++++++++++++++++++++++++-------
>  1 file changed, 277 insertions(+), 51 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-mt65xx.c b/drivers/i2c/busses/i2c-mt65xx.c
> index 0ca6c38a..7020618 100644
> --- a/drivers/i2c/busses/i2c-mt65xx.c
> +++ b/drivers/i2c/busses/i2c-mt65xx.c

<...>

> @@ -948,9 +1177,6 @@ static int mtk_i2c_probe(struct platform_device *pdev)
>  	if (ret)
>  		return -EINVAL;
>  
> -	if (i2c->dev_comp->timing_adjust)
> -		i2c->clk_src_div *= I2C_DEFAULT_CLK_DIV;
> -

After this patch, the 'clock-div' property in device tree is no longer
used for platform with timing_adjust ability.
Please change the binding, so we don't need to provide 'clock-div' for
these platform.

Joe.C

>  	if (i2c->have_pmic && !i2c->dev_comp->pmic_i2c)
>  		return -EINVAL;
>