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
Add Qii Wang as maintainer for mediatek i2c controller driver.
Signed-off-by: Qii Wang <[email protected]>
---
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: [email protected]
S: Maintained
F: drivers/net/ethernet/mediatek/
+MEDIATEK I2C CONTROLLER DRIVER
+M: Qii Wang <[email protected]>
+L: [email protected]
+S: Maintained
+F: Documentation/devicetree/bindings/i2c/i2c-mt65xx.txt
+F: drivers/i2c/busses/i2c-mt65xx.c
+
MEDIATEK JPEG DRIVER
M: Rick Chang <[email protected]>
M: Bin Liu <[email protected]>
--
1.9.1
This patch adds a algorithm to calculate some ac-timing parameters
which can fully meet I2C Spec.
Signed-off-by: Qii Wang <[email protected]>
---
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
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 <[email protected]>
Applied to for-current, thanks for stepping up!
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 <[email protected]>
Applied to for-next, thanks!
On Thu, May 14, 2020 at 3:13 PM Qii Wang <[email protected]> wrote:
> This patch adds a algorithm to calculate some ac-timing parameters
> which can fully meet I2C Spec.
>
> Signed-off-by: Qii Wang <[email protected]>
> ---
> 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!)
[email protected] 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 -- [email protected]
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
On Mon, 2020-05-18 at 17:44 +0200, Geert Uytterhoeven wrote:
> On Thu, May 14, 2020 at 3:13 PM Qii Wang <[email protected]> wrote:
> > This patch adds a algorithm to calculate some ac-timing parameters
> > which can fully meet I2C Spec.
> >
> > Signed-off-by: Qii Wang <[email protected]>
> > ---
> > 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!)
>
> [email protected] 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 -- [email protected]
>
> 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
Hi Qii,
On Tue, May 19, 2020 at 4:59 AM Qii Wang <[email protected]> wrote:
> On Mon, 2020-05-18 at 17:44 +0200, Geert Uytterhoeven wrote:
> > On Thu, May 14, 2020 at 3:13 PM Qii Wang <[email protected]> wrote:
> > > This patch adds a algorithm to calculate some ac-timing parameters
> > > which can fully meet I2C Spec.
> > >
> > > Signed-off-by: Qii Wang <[email protected]>
> > > ---
> > > 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 -- [email protected]
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
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!
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 <[email protected]> wrote:
> > On Mon, 2020-05-18 at 17:44 +0200, Geert Uytterhoeven wrote:
> > > On Thu, May 14, 2020 at 3:13 PM Qii Wang <[email protected]> wrote:
> > > > This patch adds a algorithm to calculate some ac-timing parameters
> > > > which can fully meet I2C Spec.
> > > >
> > > > Signed-off-by: Qii Wang <[email protected]>
> > > > ---
> > > > 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
>
Hi Qii,
On Wed, May 20, 2020 at 10:44 AM Qii Wang <[email protected]> wrote:
> On Tue, 2020-05-19 at 09:14 +0200, Geert Uytterhoeven wrote:
> > On Tue, May 19, 2020 at 4:59 AM Qii Wang <[email protected]> wrote:
> > > On Mon, 2020-05-18 at 17:44 +0200, Geert Uytterhoeven wrote:
> > > > On Thu, May 14, 2020 at 3:13 PM Qii Wang <[email protected]> wrote:
> > > > > This patch adds a algorithm to calculate some ac-timing parameters
> > > > > which can fully meet I2C Spec.
> > > > >
> > > > > Signed-off-by: Qii Wang <[email protected]>
> > > > > ---
> > > > > 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 -- [email protected]
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
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 <[email protected]> wrote:
> > On Tue, 2020-05-19 at 09:14 +0200, Geert Uytterhoeven wrote:
> > > On Tue, May 19, 2020 at 4:59 AM Qii Wang <[email protected]> wrote:
> > > > On Mon, 2020-05-18 at 17:44 +0200, Geert Uytterhoeven wrote:
> > > > > On Thu, May 14, 2020 at 3:13 PM Qii Wang <[email protected]> wrote:
> > > > > > This patch adds a algorithm to calculate some ac-timing parameters
> > > > > > which can fully meet I2C Spec.
> > > > > >
> > > > > > Signed-off-by: Qii Wang <[email protected]>
> > > > > > ---
> > > > > > 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
>
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 <[email protected]>
> ---
> 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;
>