| Message ID | 20240215-mbly-i2c-v1-10-19a336e91dca@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add Mobileye EyeQ5 support to the Nomadik I2C controller & use hrtimers for timeouts | expand |
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
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
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 --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);
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(-)