diff mbox series

[10/13] i2c: nomadik: support Mobileye EyeQ5 I2C controller

Message ID 20240215-mbly-i2c-v1-10-19a336e91dca@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. 15, 2024, 4:52 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. One writeb() is done to
   fill the Tx FIFO which we replace with a writel().

 - 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 (mobileye,id).

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 | 79 ++++++++++++++++++++++++++++++++++++----
 1 file changed, 71 insertions(+), 8 deletions(-)

Comments

Linus Walleij Feb. 19, 2024, 2:35 p.m. UTC | #1 
On Thu, Feb 15, 2024 at 5:52 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. One writeb() is done to
>    fill the Tx FIFO which we replace with a writel().
>
>  - 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 (mobileye,id).
>
> We add #include <linux/mfd/syscon.h> and <linux/regmap.h> and sort
> headers.
>
> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>

(...)

> -               writeb(*priv->cli.buffer, priv->virtbase + I2C_TFR);
> +               if (priv->has_32b_bus)
> +                       writel(*priv->cli.buffer, priv->virtbase + I2C_TFR);
> +               else
> +                       writeb(*priv->cli.buffer, priv->virtbase + I2C_TFR);

Are the other byte accessors working flawlessly? I get the shivers.
If it's needed in one place I bet the others prefer 32bit access too.

Further the MIPS is big-endian is it not? It feels that this just happens
to work because of byte order access? writel() is little-endian by
definition.

What happens if you replace all writeb():s with something like

static void nmk_write_reg(struct nmk_i2c_dev *priv, u32 reg, u8 val)
{
    if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
        writeb(val, priv->virtbase + reg + 3);
        // if this doesn't work then use writeb((u32)val,
priv->virtbase + reg) I guess
   else
        writeb(val, priv->virtbase + reg);
}

and conversely for readb()?

Other accessors such as iowrite* are perhaps viable in this case, I'm not sure.

Yours,
Linus Walleij
Théo Lebrun Feb. 19, 2024, 2:52 p.m. UTC | #2 
Hello,

On Mon Feb 19, 2024 at 3:35 PM CET, Linus Walleij wrote:
> On Thu, Feb 15, 2024 at 5:52 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. One writeb() is done to
> >    fill the Tx FIFO which we replace with a writel().
> >
> >  - 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 (mobileye,id).
> >
> > We add #include <linux/mfd/syscon.h> and <linux/regmap.h> and sort
> > headers.
> >
> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
>
> (...)
>
> > -               writeb(*priv->cli.buffer, priv->virtbase + I2C_TFR);
> > +               if (priv->has_32b_bus)
> > +                       writel(*priv->cli.buffer, priv->virtbase + I2C_TFR);
> > +               else
> > +                       writeb(*priv->cli.buffer, priv->virtbase + I2C_TFR);
>
> Are the other byte accessors working flawlessly? I get the shivers.
> If it's needed in one place I bet the others prefer 32bit access too.

I see where your shivers come from; I'll investigate as I don't remember
my conclusion from the time when I worked on this driver (a few months
ago).

> Further the MIPS is big-endian is it not? It feels that this just happens
> to work because of byte order access? writel() is little-endian by
> definition.

Actually, no. Our platform is little-endian.

The full story, summarised: the endianness of our cores in kernel and
hypervisor mode is defined by a pin read at reset. User mode can toggle
the endianness at runtime I believe, but that is not of our concern.
Our endianness in kernel mode is little-endian because the pin in
question is hardwired to the value meaning little-endian.

> What happens if you replace all writeb():s with something like
>
> static void nmk_write_reg(struct nmk_i2c_dev *priv, u32 reg, u8 val)
> {
>     if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
>         writeb(val, priv->virtbase + reg + 3);
>         // if this doesn't work then use writeb((u32)val,
> priv->virtbase + reg) I guess
>    else
>         writeb(val, priv->virtbase + reg);
> }
>
> and conversely for readb()?

As mentionned above, big endian isn't the worry for us. I'll be checking
the readb() calls found in i2c_irq_handler() though.

Thanks,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
Théo Lebrun Feb. 19, 2024, 4:27 p.m. UTC | #3 
Hello,

On Mon Feb 19, 2024 at 3:52 PM CET, Théo Lebrun wrote:
> On Mon Feb 19, 2024 at 3:35 PM CET, Linus Walleij wrote:
> > On Thu, Feb 15, 2024 at 5:52 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. One writeb() is done to
> > >    fill the Tx FIFO which we replace with a writel().
> > >
> > >  - 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 (mobileye,id).
> > >
> > > We add #include <linux/mfd/syscon.h> and <linux/regmap.h> and sort
> > > headers.
> > >
> > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
> >
> > (...)
> >
> > > -               writeb(*priv->cli.buffer, priv->virtbase + I2C_TFR);
> > > +               if (priv->has_32b_bus)
> > > +                       writel(*priv->cli.buffer, priv->virtbase + I2C_TFR);
> > > +               else
> > > +                       writeb(*priv->cli.buffer, priv->virtbase + I2C_TFR);
> >
> > Are the other byte accessors working flawlessly? I get the shivers.
> > If it's needed in one place I bet the others prefer 32bit access too.
>
> I see where your shivers come from; I'll investigate as I don't remember
> my conclusion from the time when I worked on this driver (a few months
> ago).
>
> > Further the MIPS is big-endian is it not? It feels that this just happens
> > to work because of byte order access? writel() is little-endian by
> > definition.
>
> Actually, no. Our platform is little-endian.
>
> The full story, summarised: the endianness of our cores in kernel and
> hypervisor mode is defined by a pin read at reset. User mode can toggle
> the endianness at runtime I believe, but that is not of our concern.
> Our endianness in kernel mode is little-endian because the pin in
> question is hardwired to the value meaning little-endian.
>
> > What happens if you replace all writeb():s with something like
> >
> > static void nmk_write_reg(struct nmk_i2c_dev *priv, u32 reg, u8 val)
> > {
> >     if (IS_ENABLED(CONFIG_CPU_BIG_ENDIAN))
> >         writeb(val, priv->virtbase + reg + 3);
> >         // if this doesn't work then use writeb((u32)val,
> > priv->virtbase + reg) I guess
> >    else
> >         writeb(val, priv->virtbase + reg);
> > }
> >
> > and conversely for readb()?
>
> As mentionned above, big endian isn't the worry for us. I'll be checking
> the readb() calls found in i2c_irq_handler() though.

Follow up on this. It was working by luck.

 - writeb() are generating Store Byte (sb) instructions which are
   unsupported on the memory bus.

 - readb() are generating Load Doubleword (ld) instructions and not the
   expected Load Byte (lb). It explains why readb() are working.

To be safe I'll make sure to use readl() and writel() everywhere for our
compatible. There is one writeb() and three readb(). Only the writeb()
was covered by this V1.

Thanks,

--
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 23e12c570457..eb076419dfa8 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 */
@@ -514,7 +528,10 @@  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);
+		if (priv->has_32b_bus)
+			writel(*priv->cli.buffer, priv->virtbase + I2C_TFR);
+		else
+			writeb(*priv->cli.buffer, priv->virtbase + I2C_TFR);
 		priv->cli.buffer++;
 		priv->cli.count--;
 		priv->cli.xfer_bytes++;
@@ -985,6 +1002,43 @@  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;
+	int ret;
+	u32 id;
+
+	priv->has_32b_bus = true;
+
+	olb = syscon_regmap_lookup_by_phandle(np, "mobileye,olb");
+	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));
+	}
+
+	ret = of_property_read_u32(np, "mobileye,id", &id);
+	if (ret)
+		return dev_err_probe(dev, ret, "failed ID lookup: %d\n", ret);
+
+	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 +1055,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);