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 <s...@chromium.org>
> ---
>
> 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(®s->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(&i2c_base->ic_enable) & IC_ENABLE_0B;
> -
> - /* to set speed cltr must be disabled */
> - dw_i2c_enable(i2c_base, false);
> -
> - cntl = (readl(&i2c_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(&i2c_base->hs_spklen);
> + spk_cnt = readl(®s->hs_spklen);
> else
> - spk_cnt = readl(&i2c_base->fs_spklen);
> + spk_cnt = readl(®s->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);
> + 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, &config);
> + if (ret)
> + return ret;
> +
> + /* Get enable setting for restore later */
> + ena = readl(&i2c_base->ic_enable) & IC_ENABLE_0B;
> +
> + /* to set speed cltr must be disabled */
> + dw_i2c_enable(i2c_base, false);
> +
> + cntl = (readl(&i2c_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, &i2c_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, &i2c_base->ic_tx_tl);
> writel(IC_STOP_DET, &i2c_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, &i2c_base->ic_sar);
> #endif
>
> @@ -571,7 +583,7 @@ static unsigned int dw_i2c_set_bus_speed(struct
> i2c_adapter *adap,
> unsigned int speed)
> {
> adap->speed = speed;
> - return __dw_i2c_set_bus_speed(NULL, i2c_get_base(adap), speed, IC_CLK);
> + return _dw_i2c_set_bus_speed(NULL, i2c_get_base(adap), speed, IC_CLK);
> }
>
> static void dw_i2c_init(struct i2c_adapter *adap, int speed, int slaveaddr)
> @@ -670,7 +682,7 @@ static int designware_i2c_set_bus_speed(struct udevice
> *bus, unsigned int speed)
> #else
> rate = IC_CLK;
> #endif
> - return __dw_i2c_set_bus_speed(i2c, i2c->regs, speed, rate);
> + return _dw_i2c_set_bus_speed(i2c, i2c->regs, speed, rate);
> }
>
> static int designware_i2c_probe_chip(struct udevice *bus, uint chip_addr,
> diff --git a/drivers/i2c/designware_i2c.h b/drivers/i2c/designware_i2c.h
> index 2027a91add..61a882cb65 100644
> --- a/drivers/i2c/designware_i2c.h
> +++ b/drivers/i2c/designware_i2c.h
> @@ -8,6 +8,7 @@
> #define __DW_I2C_H_
>
> #include <clk.h>
> +#include <i2c.h>
> #include <reset.h>
>
> struct i2c_regs {
> @@ -165,12 +166,14 @@ struct dw_scl_sda_cfg {
> * @scl_lcnt: Low count value for SCL
> * @scl_hcnt: High count value for SCL
> * @sda_hold: Data hold count
> + * @speed_mode: Speed mode being used
> */
> struct dw_i2c_speed_config {
> /* SCL high and low period count */
> u16 scl_lcnt;
> u16 scl_hcnt;
> u32 sda_hold;
> + enum i2c_speed_mode speed_mode;
> };
>
> /**
>
