diff mbox series

[v2,09/11] i2c: nomadik: support Mobileye EyeQ5 I2C controller

Message ID 20240229-mbly-i2c-v2-9-b32ed18c098c@bootlin.com
State New
Headers
Series Add Mobileye EyeQ5 support to the Nomadik I2C controller & use hrtimers for timeouts |

Commit Message

Théo Lebrun Feb. 29, 2024, 6:10 p.m. UTC 
  Add compatible for the integration of the same DB8500 IP block into the
Mobileye EyeQ5 platform. Two quirks are present:

 - The memory bus only supports 32-bit accesses. Avoid writeb() and
   readb() by introducing helper functions that fallback to writel()
   and readl().

 - A register must be configured for the I2C speed mode; it is located
   in a shared register region called OLB. We access that memory region
   using a syscon & regmap that gets passed as a phandle (mobileye,olb).

   A two-bit enum per controller is written into the register; that
   requires us to know the global index of the I2C controller (cell arg
   to the mobileye,olb phandle).

We add #include <linux/mfd/syscon.h> and <linux/regmap.h> and sort
headers.

Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
---
 drivers/i2c/busses/i2c-nomadik.c | 95 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 84 insertions(+), 11 deletions(-)

Comments

Linus Walleij Feb. 29, 2024, 9:08 p.m. UTC | #1 
On Thu, Feb 29, 2024 at 7:10 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote:

> Add compatible for the integration of the same DB8500 IP block into the
> Mobileye EyeQ5 platform. Two quirks are present:
>
>  - The memory bus only supports 32-bit accesses. Avoid writeb() and
>    readb() by introducing helper functions that fallback to writel()
>    and readl().
>
>  - A register must be configured for the I2C speed mode; it is located
>    in a shared register region called OLB. We access that memory region
>    using a syscon & regmap that gets passed as a phandle (mobileye,olb).
>
>    A two-bit enum per controller is written into the register; that
>    requires us to know the global index of the I2C controller (cell arg
>    to the mobileye,olb phandle).
>
> We add #include <linux/mfd/syscon.h> and <linux/regmap.h> and sort
> headers.
>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij
Wolfram Sang March 4, 2024, 9:27 a.m. UTC | #2 
> +	if (of_device_is_compatible(np, "mobileye,eyeq5-i2c")) {
> +		ret = nmk_i2c_eyeq5_probe(priv);
> +		if (ret) {
> +			dev_info(dev, "%s: %d: %d\n", __func__, __LINE__, ret);

This is debug code, or? Please remove it. Especially since
nmk_i2c_eyeq5_probe() prints something out on error.
Théo Lebrun March 4, 2024, 10:25 a.m. UTC | #3 
Hello,

On Mon Mar 4, 2024 at 10:27 AM CET, Wolfram Sang wrote:
>
> > +	if (of_device_is_compatible(np, "mobileye,eyeq5-i2c")) {
> > +		ret = nmk_i2c_eyeq5_probe(priv);
> > +		if (ret) {
> > +			dev_info(dev, "%s: %d: %d\n", __func__, __LINE__, ret);
>
> This is debug code, or? Please remove it. Especially since
> nmk_i2c_eyeq5_probe() prints something out on error.

It is. Nice catch, sorry about that.

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Andi Shyti March 4, 2024, 2:08 p.m. UTC | #4 
Hi Theo,

..

> +#include <linux/amba/bus.h>
>  #include <linux/bitfield.h>
> +#include <linux/clk.h>
> +#include <linux/err.h>
> +#include <linux/i2c.h>
>  #include <linux/init.h>
> -#include <linux/module.h>
> -#include <linux/amba/bus.h>
> -#include <linux/slab.h>
>  #include <linux/interrupt.h>
> -#include <linux/i2c.h>
> -#include <linux/err.h>
> -#include <linux/clk.h>
>  #include <linux/io.h>
> -#include <linux/pm_runtime.h>
> +#include <linux/mfd/syscon.h>
> +#include <linux/module.h>
>  #include <linux/of.h>
>  #include <linux/pinctrl/consumer.h>
> +#include <linux/pm_runtime.h>
> +#include <linux/regmap.h>
> +#include <linux/slab.h>

Please reorder the headers in a different patch.

>  #define DRIVER_NAME "nmk-i2c"
>  

..

> +static inline u8 nmk_i2c_readb(const struct nmk_i2c_dev *priv,
> +			       unsigned long reg)
> +{
> +	if (priv->has_32b_bus)
> +		return readl(priv->virtbase + reg);
> +	else
> +		return readb(priv->virtbase + reg);

nit: no need for the else (your choice though, if you want to
have ti coherent with the write counterpart).

> +}
> +
> +static inline void nmk_i2c_writeb(const struct nmk_i2c_dev *priv, u32 val,
> +				unsigned long reg)
> +{
> +	if (priv->has_32b_bus)
> +		writel(val, priv->virtbase + reg);
> +	else
> +		writeb(val, priv->virtbase + reg);
> +}

..

> +static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
> +{
> +	struct device *dev = &priv->adev->dev;
> +	struct device_node *np = dev->of_node;
> +	unsigned int shift, speed_mode;
> +	struct regmap *olb;
> +	unsigned int id;
> +
> +	priv->has_32b_bus = true;
> +
> +	olb = syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 1, &id);
> +	if (IS_ERR_OR_NULL(olb)) {
> +		if (!olb)
> +			olb = ERR_PTR(-ENOENT);
> +		return dev_err_probe(dev, PTR_ERR(olb),
> +				     "failed OLB lookup: %lu\n", PTR_ERR(olb));

just return PTR_ERR(olb) and use dev_err_probe() in the upper
layer probe.

> +	}
> +
> +	if (priv->clk_freq <= 400000)
> +		speed_mode = 0b00;
> +	else if (priv->clk_freq <= 1000000)
> +		speed_mode = 0b01;
> +	else
> +		speed_mode = 0b10;

would be nice to have these as defines.

> +
> +	shift = NMK_I2C_EYEQ5_OLB_IOCR2_SHIFT(id);
> +	regmap_update_bits(olb, NMK_I2C_EYEQ5_OLB_IOCR2,
> +			   0b11 << shift, speed_mode << shift);

please define these values and for hexadecimals use 0x...

> +	return 0;
> +}
> +
>  static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
>  {
>  	int ret = 0;
> @@ -1001,8 +1065,17 @@ static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
>  
>  	priv->vendor = vendor;
>  	priv->adev = adev;
> +	priv->has_32b_bus = false;
>  	nmk_i2c_of_probe(np, priv);
>  
> +	if (of_device_is_compatible(np, "mobileye,eyeq5-i2c")) {
> +		ret = nmk_i2c_eyeq5_probe(priv);
> +		if (ret) {
> +			dev_info(dev, "%s: %d: %d\n", __func__, __LINE__, ret);
> +			return ret;
> +		}

as said above, use dev_err_probe here.

Andi

> +	}
> +
>  	if (priv->tft > max_fifo_threshold) {
>  		dev_warn(dev, "requested TX FIFO threshold %u, adjusted down to %u\n",
>  			 priv->tft, max_fifo_threshold);
> 
> -- 
> 2.44.0
>
Théo Lebrun March 4, 2024, 2:53 p.m. UTC | #5 
Hello,

On Mon Mar 4, 2024 at 3:08 PM CET, Andi Shyti wrote:
> Hi Theo,
>
> ...
>
> > +#include <linux/amba/bus.h>
> >  #include <linux/bitfield.h>
> > +#include <linux/clk.h>
> > +#include <linux/err.h>
> > +#include <linux/i2c.h>
> >  #include <linux/init.h>
> > -#include <linux/module.h>
> > -#include <linux/amba/bus.h>
> > -#include <linux/slab.h>
> >  #include <linux/interrupt.h>
> > -#include <linux/i2c.h>
> > -#include <linux/err.h>
> > -#include <linux/clk.h>
> >  #include <linux/io.h>
> > -#include <linux/pm_runtime.h>
> > +#include <linux/mfd/syscon.h>
> > +#include <linux/module.h>
> >  #include <linux/of.h>
> >  #include <linux/pinctrl/consumer.h>
> > +#include <linux/pm_runtime.h>
> > +#include <linux/regmap.h>
> > +#include <linux/slab.h>
>
> Please reorder the headers in a different patch.

Will do.

>
> >  #define DRIVER_NAME "nmk-i2c"
> >  
>
> ...
>
> > +static inline u8 nmk_i2c_readb(const struct nmk_i2c_dev *priv,
> > +			       unsigned long reg)
> > +{
> > +	if (priv->has_32b_bus)
> > +		return readl(priv->virtbase + reg);
> > +	else
> > +		return readb(priv->virtbase + reg);
>
> nit: no need for the else (your choice though, if you want to
> have ti coherent with the write counterpart).

Indeed, the useless else block can be removed. Will do.

> > +}
> > +
> > +static inline void nmk_i2c_writeb(const struct nmk_i2c_dev *priv, u32 val,
> > +				unsigned long reg)
> > +{
> > +	if (priv->has_32b_bus)
> > +		writel(val, priv->virtbase + reg);
> > +	else
> > +		writeb(val, priv->virtbase + reg);
> > +}
>
> ...
>
> > +static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
> > +{
> > +	struct device *dev = &priv->adev->dev;
> > +	struct device_node *np = dev->of_node;
> > +	unsigned int shift, speed_mode;
> > +	struct regmap *olb;
> > +	unsigned int id;
> > +
> > +	priv->has_32b_bus = true;
> > +
> > +	olb = syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 1, &id);
> > +	if (IS_ERR_OR_NULL(olb)) {
> > +		if (!olb)
> > +			olb = ERR_PTR(-ENOENT);
> > +		return dev_err_probe(dev, PTR_ERR(olb),
> > +				     "failed OLB lookup: %lu\n", PTR_ERR(olb));
>
> just return PTR_ERR(olb) and use dev_err_probe() in the upper
> layer probe.

Good catch. In previous revisions nmk_i2c_eyeq5_probe() had multiple
error cases so it had to be the one doing the logging. Now that there
is a single possible error parent can do it. It should simplify code.

>
> > +	}
> > +
> > +	if (priv->clk_freq <= 400000)
> > +		speed_mode = 0b00;
> > +	else if (priv->clk_freq <= 1000000)
> > +		speed_mode = 0b01;
> > +	else
> > +		speed_mode = 0b10;
>
> would be nice to have these as defines.

Agreed. Will be named based on I2C mode names, eg standard, fast, high
speed, fast plus.

>
> > +
> > +	shift = NMK_I2C_EYEQ5_OLB_IOCR2_SHIFT(id);
> > +	regmap_update_bits(olb, NMK_I2C_EYEQ5_OLB_IOCR2,
> > +			   0b11 << shift, speed_mode << shift);
>
> please define these values and for hexadecimals use 0x...

0b11 is a two-bit mask. What do you mean by "these values"? Something
like:



#define NMK_I2C_EYEQ5_SPEED_MODE_FAST		0b00
#define NMK_I2C_EYEQ5_SPEED_MODE_FAST_PLUS	0b01
#define NMK_I2C_EYEQ5_SPEED_MODE_HIGH_SPEED	0b10

static const u8 nmk_i2c_eyeq5_masks[] = {
	[0] = 0x0030,
	[1] = 0x00C0,
	[2] = 0x0300,
	[3] = 0x0C00,
	[4] = 0x3000,
};

static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
{
	// ...
	unsigned int id, mask, speed_mode;

	priv->has_32b_bus = true;

	olb = syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 1, &id);
	// TODO: olb error checking
	// TODO: check id is valid

	if (priv->clk_freq <= 400000)
		speed_mode = NMK_I2C_EYEQ5_SPEED_MODE_FAST;
	else if (priv->clk_freq <= 1000000)
		speed_mode = NMK_I2C_EYEQ5_SPEED_MODE_FAST_PLUS;
	else
		speed_mode = NMK_I2C_EYEQ5_SPEED_MODE_HIGH_SPEED;

	mask = nmk_i2c_eyeq5_masks[id];
	regmap_update_bits(olb, NMK_I2C_EYEQ5_OLB_IOCR2,
			   mask, speed_mode << __fls(mask));
	return 0;
}

Else I do not see what you are suggesting by "please define these
values".

Have a nice day,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
diff mbox series

Patch

diff --git a/drivers/i2c/busses/i2c-nomadik.c b/drivers/i2c/busses/i2c-nomadik.c
index 2d3247979e45..e9a77377add4 100644
--- a/drivers/i2c/busses/i2c-nomadik.c
+++ b/drivers/i2c/busses/i2c-nomadik.c
@@ -6,22 +6,30 @@ 
  * I2C master mode controller driver, used in Nomadik 8815
  * and Ux500 platforms.
  *
+ * The Mobileye EyeQ5 platform is also supported; it uses
+ * the same Ux500/DB8500 IP block with two quirks:
+ *  - The memory bus only supports 32-bit accesses.
+ *  - A register must be configured for the I2C speed mode;
+ *    it is located in a shared register region called OLB.
+ *
  * Author: Srinidhi Kasagar <srinidhi.kasagar@stericsson.com>
  * Author: Sachin Verma <sachin.verma@st.com>
  */
+#include <linux/amba/bus.h>
 #include <linux/bitfield.h>
+#include <linux/clk.h>
+#include <linux/err.h>
+#include <linux/i2c.h>
 #include <linux/init.h>
-#include <linux/module.h>
-#include <linux/amba/bus.h>
-#include <linux/slab.h>
 #include <linux/interrupt.h>
-#include <linux/i2c.h>
-#include <linux/err.h>
-#include <linux/clk.h>
 #include <linux/io.h>
-#include <linux/pm_runtime.h>
+#include <linux/mfd/syscon.h>
+#include <linux/module.h>
 #include <linux/of.h>
 #include <linux/pinctrl/consumer.h>
+#include <linux/pm_runtime.h>
+#include <linux/regmap.h>
+#include <linux/slab.h>
 
 #define DRIVER_NAME "nmk-i2c"
 
@@ -110,6 +118,10 @@  enum i2c_freq_mode {
 	I2C_FREQ_MODE_FAST_PLUS,	/* up to 1 Mb/s */
 };
 
+/* Mobileye EyeQ5 offset into a shared register region (called OLB) */
+#define NMK_I2C_EYEQ5_OLB_IOCR2			0x0B8
+#define NMK_I2C_EYEQ5_OLB_IOCR2_SHIFT(id)	(4 + 2 * (id))
+
 /**
  * struct i2c_vendor_data - per-vendor variations
  * @has_mtdws: variant has the MTDWS bit
@@ -168,6 +180,7 @@  struct i2c_nmk_client {
  * @xfer_wq: xfer done wait queue.
  * @xfer_done: xfer done boolean.
  * @result: controller propogated result.
+ * @has_32b_bus: controller is on a bus that only supports 32-bit accesses.
  */
 struct nmk_i2c_dev {
 	struct i2c_vendor_data		*vendor;
@@ -186,6 +199,7 @@  struct nmk_i2c_dev {
 	struct wait_queue_head		xfer_wq;
 	bool				xfer_done;
 	int				result;
+	bool				has_32b_bus;
 };
 
 /* controller's abort causes */
@@ -209,6 +223,24 @@  static inline void i2c_clr_bit(void __iomem *reg, u32 mask)
 	writel(readl(reg) & ~mask, reg);
 }
 
+static inline u8 nmk_i2c_readb(const struct nmk_i2c_dev *priv,
+			       unsigned long reg)
+{
+	if (priv->has_32b_bus)
+		return readl(priv->virtbase + reg);
+	else
+		return readb(priv->virtbase + reg);
+}
+
+static inline void nmk_i2c_writeb(const struct nmk_i2c_dev *priv, u32 val,
+				unsigned long reg)
+{
+	if (priv->has_32b_bus)
+		writel(val, priv->virtbase + reg);
+	else
+		writeb(val, priv->virtbase + reg);
+}
+
 /**
  * flush_i2c_fifo() - This function flushes the I2C FIFO
  * @priv: private data of I2C Driver
@@ -514,7 +546,7 @@  static void fill_tx_fifo(struct nmk_i2c_dev *priv, int no_bytes)
 			(priv->cli.count != 0);
 			count--) {
 		/* write to the Tx FIFO */
-		writeb(*priv->cli.buffer, priv->virtbase + I2C_TFR);
+		nmk_i2c_writeb(priv, *priv->cli.buffer, I2C_TFR);
 		priv->cli.buffer++;
 		priv->cli.count--;
 		priv->cli.xfer_bytes++;
@@ -783,7 +815,7 @@  static irqreturn_t i2c_irq_handler(int irq, void *arg)
 	case I2C_IT_RXFNF:
 		for (count = rft; count > 0; count--) {
 			/* Read the Rx FIFO */
-			*priv->cli.buffer = readb(priv->virtbase + I2C_RFR);
+			*priv->cli.buffer = nmk_i2c_readb(priv, I2C_RFR);
 			priv->cli.buffer++;
 		}
 		priv->cli.count -= rft;
@@ -793,7 +825,7 @@  static irqreturn_t i2c_irq_handler(int irq, void *arg)
 	/* Rx FIFO full */
 	case I2C_IT_RXFF:
 		for (count = MAX_I2C_FIFO_THRESHOLD; count > 0; count--) {
-			*priv->cli.buffer = readb(priv->virtbase + I2C_RFR);
+			*priv->cli.buffer = nmk_i2c_readb(priv, I2C_RFR);
 			priv->cli.buffer++;
 		}
 		priv->cli.count -= MAX_I2C_FIFO_THRESHOLD;
@@ -809,7 +841,7 @@  static irqreturn_t i2c_irq_handler(int irq, void *arg)
 				if (priv->cli.count == 0)
 					break;
 				*priv->cli.buffer =
-					readb(priv->virtbase + I2C_RFR);
+					nmk_i2c_readb(priv, I2C_RFR);
 				priv->cli.buffer++;
 				priv->cli.count--;
 				priv->cli.xfer_bytes++;
@@ -985,6 +1017,38 @@  static void nmk_i2c_of_probe(struct device_node *np,
 		priv->timeout_usecs = 200 * USEC_PER_MSEC;
 }
 
+static int nmk_i2c_eyeq5_probe(struct nmk_i2c_dev *priv)
+{
+	struct device *dev = &priv->adev->dev;
+	struct device_node *np = dev->of_node;
+	unsigned int shift, speed_mode;
+	struct regmap *olb;
+	unsigned int id;
+
+	priv->has_32b_bus = true;
+
+	olb = syscon_regmap_lookup_by_phandle_args(np, "mobileye,olb", 1, &id);
+	if (IS_ERR_OR_NULL(olb)) {
+		if (!olb)
+			olb = ERR_PTR(-ENOENT);
+		return dev_err_probe(dev, PTR_ERR(olb),
+				     "failed OLB lookup: %lu\n", PTR_ERR(olb));
+	}
+
+	if (priv->clk_freq <= 400000)
+		speed_mode = 0b00;
+	else if (priv->clk_freq <= 1000000)
+		speed_mode = 0b01;
+	else
+		speed_mode = 0b10;
+
+	shift = NMK_I2C_EYEQ5_OLB_IOCR2_SHIFT(id);
+	regmap_update_bits(olb, NMK_I2C_EYEQ5_OLB_IOCR2,
+			   0b11 << shift, speed_mode << shift);
+
+	return 0;
+}
+
 static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 {
 	int ret = 0;
@@ -1001,8 +1065,17 @@  static int nmk_i2c_probe(struct amba_device *adev, const struct amba_id *id)
 
 	priv->vendor = vendor;
 	priv->adev = adev;
+	priv->has_32b_bus = false;
 	nmk_i2c_of_probe(np, priv);
 
+	if (of_device_is_compatible(np, "mobileye,eyeq5-i2c")) {
+		ret = nmk_i2c_eyeq5_probe(priv);
+		if (ret) {
+			dev_info(dev, "%s: %d: %d\n", __func__, __LINE__, ret);
+			return ret;
+		}
+	}
+
 	if (priv->tft > max_fifo_threshold) {
 		dev_warn(dev, "requested TX FIFO threshold %u, adjusted down to %u\n",
 			 priv->tft, max_fifo_threshold);