[PATCH 1/2] i2c: designware_i2c: Tidy up use of NULL priv

[PATCH 1/2] i2c: designware_i2c: Tidy up use of NULL priv

[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(-)

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(&regs->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(&regs->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;
 
-- 
2.26.1.301.g55bc3eb7cb9-goog

[PATCH 2/2] i2c: designware_i2c: Check if the device is powered

[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(+)

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);
 }
-- 
2.26.1.301.g55bc3eb7cb9-goog

[PATCH 1/2] i2c: designware_i2c: Tidy up use of NULL priv

[Heiko Schocher]

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
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de

[PATCH 2/2] i2c: designware_i2c: Check if the device is powered

[Heiko Schocher]

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
-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-52   Fax: +49-8142-66989-80   Email: hs at denx.de