Re: [PATCH v3 22/23] i2c: designware_i2c: Separate out the speed calculation
Hi Heinrich,
On Wed, 22 Apr 2020 at 00:43, Heinrich Schuchardt wrote:
>
> On 1/23/20 7:48 PM, Simon Glass wrote:
> > We want to be able to calculate the speed separately from actually setting
> > the speed, so we can generate the required ACPI tables. Split out the
> > calculation into its own function.
> >
> > Drop the double underscore on __dw_i2c_set_bus_speed while we are here.
> > That is reserved for compiler internals.
> >
> > Signed-off-by: Simon Glass
> > ---
> >
> > Changes in v3:
> > - Add new patch to separate out the speed calculation
> >
> > Changes in v2: None
> >
> > drivers/i2c/designware_i2c.c | 78 +---
> > drivers/i2c/designware_i2c.h | 3 ++
> > 2 files changed, 48 insertions(+), 33 deletions(-)
> >
> > diff --git a/drivers/i2c/designware_i2c.c b/drivers/i2c/designware_i2c.c
> > index 6be98ee43b..39af25af9a 100644
> > --- a/drivers/i2c/designware_i2c.c
> > +++ b/drivers/i2c/designware_i2c.c
> > @@ -194,22 +194,12 @@ static int dw_i2c_calc_timing(struct dw_i2c *priv,
> > enum i2c_speed_mode mode,
> > return 0;
> > }
> >
> > -/*
> > - * i2c_set_bus_speed - Set the i2c speed
> > - * @speed: required i2c speed
> > - *
> > - * Set the i2c speed.
> > - */
> > -static unsigned int __dw_i2c_set_bus_speed(struct dw_i2c *priv,
> > -struct i2c_regs *i2c_base,
> > -unsigned int speed,
> > -unsigned int bus_clk)
> > +static int calc_bus_speed(struct dw_i2c *priv, int speed, ulong bus_clk,
> > + struct dw_i2c_speed_config *config)
> > {
> > const struct dw_scl_sda_cfg *scl_sda_cfg = NULL;
> > - struct dw_i2c_speed_config config;
> > + struct i2c_regs *regs = priv->regs;
>
> Later in the code you have 'if (priv)'. Please, do not dereference priv
> before the check.
>
> Overall the code is somehow odd:
>
> _dw_i2c_set_bus_speed() is called in multiple places with priv == NULL
> and then calls calc_bus_speed(priv, ...).
>
> Then in calc_bus_speed() you have:
>
> comp_param1 = readl(>comp_param1);
>
> where regs == NULL->regs.
>
> comp_param1 is used later on in the code to determine i2c_spd which is
> returned in config->speed_mode.
Only for non-DM though I think. Still this is horrible, will send a patch.
What board are you testing with?
>
> Could you, please, have a close look at the driver.
>
> Best regards
>
> Heinrich
>
Regards,
Simon
Re: [PATCH v3 22/23] i2c: designware_i2c: Separate out the speed calculation
On 1/23/20 7:48 PM, Simon Glass wrote:
> We want to be able to calculate the speed separately from actually setting
> the speed, so we can generate the required ACPI tables. Split out the
> calculation into its own function.
>
> Drop the double underscore on __dw_i2c_set_bus_speed while we are here.
> That is reserved for compiler internals.
>
> Signed-off-by: Simon Glass
> ---
>
> Changes in v3:
> - Add new patch to separate out the speed calculation
>
> Changes in v2: None
>
> drivers/i2c/designware_i2c.c | 78 +---
> drivers/i2c/designware_i2c.h | 3 ++
> 2 files changed, 48 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/i2c/designware_i2c.c b/drivers/i2c/designware_i2c.c
> index 6be98ee43b..39af25af9a 100644
> --- a/drivers/i2c/designware_i2c.c
> +++ b/drivers/i2c/designware_i2c.c
> @@ -194,22 +194,12 @@ static int dw_i2c_calc_timing(struct dw_i2c *priv, enum
> i2c_speed_mode mode,
> return 0;
> }
>
> -/*
> - * i2c_set_bus_speed - Set the i2c speed
> - * @speed: required i2c speed
> - *
> - * Set the i2c speed.
> - */
> -static unsigned int __dw_i2c_set_bus_speed(struct dw_i2c *priv,
> -struct i2c_regs *i2c_base,
> -unsigned int speed,
> -unsigned int bus_clk)
> +static int calc_bus_speed(struct dw_i2c *priv, int speed, ulong bus_clk,
> + struct dw_i2c_speed_config *config)
> {
> const struct dw_scl_sda_cfg *scl_sda_cfg = NULL;
> - struct dw_i2c_speed_config config;
> + struct i2c_regs *regs = priv->regs;
Later in the code you have 'if (priv)'. Please, do not dereference priv
before the check.
Overall the code is somehow odd:
_dw_i2c_set_bus_speed() is called in multiple places with priv == NULL
and then calls calc_bus_speed(priv, ...).
Then in calc_bus_speed() you have:
comp_param1 = readl(>comp_param1);
where regs == NULL->regs.
comp_param1 is used later on in the code to determine i2c_spd which is
returned in config->speed_mode.
Could you, please, have a close look at the driver.
Best regards
Heinrich
> enum i2c_speed_mode i2c_spd;
> - unsigned int cntl;
> - unsigned int ena;
> int spk_cnt;
> int ret;
>
> @@ -226,38 +216,60 @@ static unsigned int __dw_i2c_set_bus_speed(struct
> dw_i2c *priv,
> else
> i2c_spd = IC_SPEED_MODE_STANDARD;
>
> - /* Get enable setting for restore later */
> - ena = readl(_base->ic_enable) & IC_ENABLE_0B;
> -
> - /* to set speed cltr must be disabled */
> - dw_i2c_enable(i2c_base, false);
> -
> - cntl = (readl(_base->ic_con) & (~IC_CON_SPD_MSK));
> -
> /* Get the proper spike-suppression count based on target speed */
> if (!priv || !priv->has_spk_cnt)
> spk_cnt = 0;
> else if (i2c_spd >= IC_SPEED_MODE_HIGH)
> - spk_cnt = readl(_base->hs_spklen);
> + spk_cnt = readl(>hs_spklen);
> else
> - spk_cnt = readl(_base->fs_spklen);
> + spk_cnt = readl(>fs_spklen);
> if (scl_sda_cfg) {
> - config.sda_hold = scl_sda_cfg->sda_hold;
> + config->sda_hold = scl_sda_cfg->sda_hold;
> if (i2c_spd == IC_SPEED_MODE_STANDARD) {
> - config.scl_hcnt = scl_sda_cfg->ss_hcnt;
> - config.scl_lcnt = scl_sda_cfg->ss_lcnt;
> + config->scl_hcnt = scl_sda_cfg->ss_hcnt;
> + config->scl_lcnt = scl_sda_cfg->ss_lcnt;
> } else {
> - config.scl_hcnt = scl_sda_cfg->fs_hcnt;
> - config.scl_lcnt = scl_sda_cfg->fs_lcnt;
> + config->scl_hcnt = scl_sda_cfg->fs_hcnt;
> + config->scl_lcnt = scl_sda_cfg->fs_lcnt;
> }
> } else {
> ret = dw_i2c_calc_timing(priv, i2c_spd, bus_clk, spk_cnt,
> - );
> + config);
> if (ret)
> return log_msg_ret("gen_confg", ret);
> }
> + config->speed_mode = i2c_spd;
> +
> + return 0;
> +}
> +
> +/*
> + * _dw_i2c_set_bus_speed - Set the i2c speed
> + * @speed: required i2c speed
> + *
> + * Set the i2c speed.
> + */
> +static int _dw_i2c_set_bus_speed(struct dw_i2c *priv, struct i2c_regs
> *i2c_base,
> + unsigned int speed, unsigned int bus_clk)
> +{
> + struct dw_i2c_speed_config config;
> + unsigned int cntl;
> + unsigned int ena;
> + int ret;
> +
> + ret = calc_bus_speed(priv, speed, bus_clk, );
> + if (ret)
> + return ret;
> +
> + /* Get enable setting for restore later */
> + ena = readl(_base->ic_enable) & IC_ENABLE_0B;
> +
> + /* to set speed cltr must be disabled */
> + dw_i2c_enable(i2c_base, false);
> +
> + cntl = (readl(_base->ic_con) & (~IC_CON_SPD_MSK));
>
[PATCH v3 22/23] i2c: designware_i2c: Separate out the speed calculation
We want to be able to calculate the speed separately from actually setting
the speed, so we can generate the required ACPI tables. Split out the
calculation into its own function.
Drop the double underscore on __dw_i2c_set_bus_speed while we are here.
That is reserved for compiler internals.
Signed-off-by: Simon Glass
---
Changes in v3:
- Add new patch to separate out the speed calculation
Changes in v2: None
drivers/i2c/designware_i2c.c | 78 +---
drivers/i2c/designware_i2c.h | 3 ++
2 files changed, 48 insertions(+), 33 deletions(-)
diff --git a/drivers/i2c/designware_i2c.c b/drivers/i2c/designware_i2c.c
index 6be98ee43b..39af25af9a 100644
--- a/drivers/i2c/designware_i2c.c
+++ b/drivers/i2c/designware_i2c.c
@@ -194,22 +194,12 @@ static int dw_i2c_calc_timing(struct dw_i2c *priv, enum
i2c_speed_mode mode,
return 0;
}
-/*
- * i2c_set_bus_speed - Set the i2c speed
- * @speed: required i2c speed
- *
- * Set the i2c speed.
- */
-static unsigned int __dw_i2c_set_bus_speed(struct dw_i2c *priv,
- struct i2c_regs *i2c_base,
- unsigned int speed,
- unsigned int bus_clk)
+static int calc_bus_speed(struct dw_i2c *priv, int speed, ulong bus_clk,
+ struct dw_i2c_speed_config *config)
{
const struct dw_scl_sda_cfg *scl_sda_cfg = NULL;
- struct dw_i2c_speed_config config;
+ struct i2c_regs *regs = priv->regs;
enum i2c_speed_mode i2c_spd;
- unsigned int cntl;
- unsigned int ena;
int spk_cnt;
int ret;
@@ -226,38 +216,60 @@ static unsigned int __dw_i2c_set_bus_speed(struct dw_i2c
*priv,
else
i2c_spd = IC_SPEED_MODE_STANDARD;
- /* Get enable setting for restore later */
- ena = readl(_base->ic_enable) & IC_ENABLE_0B;
-
- /* to set speed cltr must be disabled */
- dw_i2c_enable(i2c_base, false);
-
- cntl = (readl(_base->ic_con) & (~IC_CON_SPD_MSK));
-
/* Get the proper spike-suppression count based on target speed */
if (!priv || !priv->has_spk_cnt)
spk_cnt = 0;
else if (i2c_spd >= IC_SPEED_MODE_HIGH)
- spk_cnt = readl(_base->hs_spklen);
+ spk_cnt = readl(>hs_spklen);
else
- spk_cnt = readl(_base->fs_spklen);
+ spk_cnt = readl(>fs_spklen);
if (scl_sda_cfg) {
- config.sda_hold = scl_sda_cfg->sda_hold;
+ config->sda_hold = scl_sda_cfg->sda_hold;
if (i2c_spd == IC_SPEED_MODE_STANDARD) {
- config.scl_hcnt = scl_sda_cfg->ss_hcnt;
- config.scl_lcnt = scl_sda_cfg->ss_lcnt;
+ config->scl_hcnt = scl_sda_cfg->ss_hcnt;
+ config->scl_lcnt = scl_sda_cfg->ss_lcnt;
} else {
- config.scl_hcnt = scl_sda_cfg->fs_hcnt;
- config.scl_lcnt = scl_sda_cfg->fs_lcnt;
+ config->scl_hcnt = scl_sda_cfg->fs_hcnt;
+ config->scl_lcnt = scl_sda_cfg->fs_lcnt;
}
} else {
ret = dw_i2c_calc_timing(priv, i2c_spd, bus_clk, spk_cnt,
-);
+config);
if (ret)
return log_msg_ret("gen_confg", ret);
}
+ config->speed_mode = i2c_spd;
+
+ return 0;
+}
+
+/*
+ * _dw_i2c_set_bus_speed - Set the i2c speed
+ * @speed: required i2c speed
+ *
+ * Set the i2c speed.
+ */
+static int _dw_i2c_set_bus_speed(struct dw_i2c *priv, struct i2c_regs
*i2c_base,
+unsigned int speed, unsigned int bus_clk)
+{
+ struct dw_i2c_speed_config config;
+ unsigned int cntl;
+ unsigned int ena;
+ int ret;
+
+ ret = calc_bus_speed(priv, speed, bus_clk, );
+ if (ret)
+ return ret;
+
+ /* Get enable setting for restore later */
+ ena = readl(_base->ic_enable) & IC_ENABLE_0B;
+
+ /* to set speed cltr must be disabled */
+ dw_i2c_enable(i2c_base, false);
+
+ cntl = (readl(_base->ic_con) & (~IC_CON_SPD_MSK));
- switch (i2c_spd) {
+ switch (config.speed_mode) {
case IC_SPEED_MODE_HIGH:
cntl |= IC_CON_SPD_SS;
writel(config.scl_hcnt, _base->ic_hs_scl_hcnt);
@@ -526,7 +538,7 @@ static int __dw_i2c_init(struct i2c_regs *i2c_base, int
speed, int slaveaddr)
writel(IC_TX_TL, _base->ic_tx_tl);
writel(IC_STOP_DET, _base->ic_intr_mask);
#ifndef CONFIG_DM_I2C
- __dw_i2c_set_bus_speed(NULL, i2c_base, speed, IC_CLK);
+ _dw_i2c_set_bus_speed(NULL, i2c_base, speed, IC_CLK);
writel(slaveaddr, _base->ic_sar);
#endif
@@ -571,7 +583,7 @@ static unsigned int dw_i2c_set_bus_speed(struct
