[PATCH 1/2] i2c: designware_i2c: Tidy up use of NULL priv
At present we still have pre-driver-model code in this driver and it makes things a bit confusing. In particular calc_bus_speed() is called with priv as NULL if not using driver model.
This results in spk_cnt and comp_param1 being read from an invalid address if not using driver model. For comp_param1 this may not cause problems if reading from addresses close to 0 happens to be allowed, as high speed is only supported by DM code. But spk_cnt is subsequently used to calculate the bus periods and so this may cause problems (e.g. on spear600 board which has not been migrated yet).
Add a new parameter regs parameter to calc_bus_speed() and add more comments to this function and to _dw_i2c_set_bus_speed(), which calls it.
Signed-off-by: Simon Glass sjg@chromium.org Reported-by: Heinrich Schuchardt xypron.glpk@gmx.de ---
drivers/i2c/designware_i2c.c | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-)
diff --git a/drivers/i2c/designware_i2c.c b/drivers/i2c/designware_i2c.c index 088a6f3efb..ac170769f4 100644 --- a/drivers/i2c/designware_i2c.c +++ b/drivers/i2c/designware_i2c.c @@ -197,18 +197,24 @@ static int dw_i2c_calc_timing(struct dw_i2c *priv, enum i2c_speed_mode mode, return 0; }
-static int calc_bus_speed(struct dw_i2c *priv, int speed, ulong bus_clk, - struct dw_i2c_speed_config *config) +/** + * calc_bus_speed() - Calculate the config to use for a particular i2c speed + * + * @priv: Private information for the driver (NULL if not using driver model) + * @i2c_base: Registers for the I2C controller + * @speed: Required i2c speed in Hz + * @bus_clk: Input clock to the I2C controller in Hz (e.g. IC_CLK) + * @config: Returns the config to use for this speed + * @return 0 if OK, -ve on error + */ +static int calc_bus_speed(struct dw_i2c *priv, struct i2c_regs *regs, int speed, + ulong bus_clk, struct dw_i2c_speed_config *config) { const struct dw_scl_sda_cfg *scl_sda_cfg = NULL; - struct i2c_regs *regs = priv->regs; enum i2c_speed_mode i2c_spd; - u32 comp_param1; int spk_cnt; int ret;
- comp_param1 = readl(®s->comp_param1); - if (priv) scl_sda_cfg = priv->scl_sda_cfg; /* Allow high speed if there is no config, or the config allows it */ @@ -223,6 +229,9 @@ static int calc_bus_speed(struct dw_i2c *priv, int speed, ulong bus_clk,
/* Check is high speed possible and fall back to fast mode if not */ if (i2c_spd == IC_SPEED_MODE_HIGH) { + u32 comp_param1; + + comp_param1 = readl(®s->comp_param1); if ((comp_param1 & DW_IC_COMP_PARAM_1_SPEED_MODE_MASK) != DW_IC_COMP_PARAM_1_SPEED_MODE_HIGH) i2c_spd = IC_SPEED_MODE_FAST; @@ -258,11 +267,14 @@ static int calc_bus_speed(struct dw_i2c *priv, int speed, ulong bus_clk, return 0; }
-/* - * _dw_i2c_set_bus_speed - Set the i2c speed - * @speed: required i2c speed +/** + * _dw_i2c_set_bus_speed() - Set the i2c speed * - * Set the i2c speed. + * @priv: Private information for the driver (NULL if not using driver model) + * @i2c_base: Registers for the I2C controller + * @speed: Required i2c speed in Hz + * @bus_clk: Input clock to the I2C controller in Hz (e.g. IC_CLK) + * @return 0 if OK, -ve on error */ static int _dw_i2c_set_bus_speed(struct dw_i2c *priv, struct i2c_regs *i2c_base, unsigned int speed, unsigned int bus_clk) @@ -272,7 +284,7 @@ static int _dw_i2c_set_bus_speed(struct dw_i2c *priv, struct i2c_regs *i2c_base, unsigned int ena; int ret;
- ret = calc_bus_speed(priv, speed, bus_clk, &config); + ret = calc_bus_speed(priv, i2c_base, speed, bus_clk, &config); if (ret) return ret;
From: Raul E Rangel rrangel@chromium.org
If the device doesn't return a version that means the device is non-functional.
The dw_i2c_regs had invalid offsets for the version field. I got the correct value from the DesignWare databook. It also matches what the Picasso PPR says.
Signed-off-by: Raul E Rangel rrangel@chromium.org Reviewed-by: Simon Glass sjg@chromium.org Reviewed-by: Furquan Shaikh furquan@chromium.org Tested on chromebook_coral: Signed-off-by: Simon Glass sjg@chromium.org ---
drivers/i2c/designware_i2c.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/drivers/i2c/designware_i2c.c b/drivers/i2c/designware_i2c.c index ac170769f4..f7a48f6225 100644 --- a/drivers/i2c/designware_i2c.c +++ b/drivers/i2c/designware_i2c.c @@ -16,6 +16,12 @@ #include <dm/device_compat.h> #include <linux/err.h>
+/* + * This assigned unique hex value is constant and is derived from the two ASCII + * letters 'DW' followed by a 16-bit unsigned number + */ +#define DW_I2C_COMP_TYPE 0x44570140 + #ifdef CONFIG_SYS_I2C_DW_ENABLE_STATUS_UNSUPPORTED static int dw_i2c_enable(struct i2c_regs *i2c_base, bool enable) { @@ -764,6 +770,17 @@ int designware_i2c_ofdata_to_platdata(struct udevice *bus) int designware_i2c_probe(struct udevice *bus) { struct dw_i2c *priv = dev_get_priv(bus); + uint comp_type; + + comp_type = readl(&priv->regs->comp_type); + if (comp_type != DW_I2C_COMP_TYPE) { + log_err("I2C bus %s has unknown type %#x\n", bus->name, + comp_type); + return -ENXIO; + } + + log_info("I2C bus %s version %#x\n", bus->name, + readl(&priv->regs->comp_version));
return __dw_i2c_init(priv->regs, 0, 0); }
Hello Simon,
Am 22.04.2020 um 18:13 schrieb Simon Glass:
From: Raul E Rangel rrangel@chromium.org
If the device doesn't return a version that means the device is non-functional.
The dw_i2c_regs had invalid offsets for the version field. I got the correct value from the DesignWare databook. It also matches what the Picasso PPR says.
Signed-off-by: Raul E Rangel rrangel@chromium.org Reviewed-by: Simon Glass sjg@chromium.org Reviewed-by: Furquan Shaikh furquan@chromium.org Tested on chromebook_coral: Signed-off-by: Simon Glass sjg@chromium.org
drivers/i2c/designware_i2c.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
Applied to u-boot-i2c master
Thanks!
bye, HEiko
Hello Simon,
Am 22.04.2020 um 18:13 schrieb Simon Glass:
At present we still have pre-driver-model code in this driver and it makes things a bit confusing. In particular calc_bus_speed() is called with priv as NULL if not using driver model.
This results in spk_cnt and comp_param1 being read from an invalid address if not using driver model. For comp_param1 this may not cause problems if reading from addresses close to 0 happens to be allowed, as high speed is only supported by DM code. But spk_cnt is subsequently used to calculate the bus periods and so this may cause problems (e.g. on spear600 board which has not been migrated yet).
Add a new parameter regs parameter to calc_bus_speed() and add more comments to this function and to _dw_i2c_set_bus_speed(), which calls it.
Signed-off-by: Simon Glass sjg@chromium.org Reported-by: Heinrich Schuchardt xypron.glpk@gmx.de
drivers/i2c/designware_i2c.c | 34 +++++++++++++++++++++++----------- 1 file changed, 23 insertions(+), 11 deletions(-)
Applied to u-boot-i2c master
Thanks!
bye, Heiko
participants (2)
-
Heiko Schocher -
Simon Glass