| Message ID | 20240229-mbly-i2c-v2-0-b32ed18c098c@bootlin.com |
|---|---|
| Headers | show |
| Series | Add Mobileye EyeQ5 support to the Nomadik I2C controller & use hrtimers for timeouts | expand |
On Thu, Feb 29, 2024 at 7:10 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > Allow overriding the default timeout value (200ms) from devicetree, > using the generic i2c-transfer-timeout-us property. > > The i2c_adapter->timeout field is an unaccurate jiffies amount; > i2c-nomadik uses hrtimers for timeouts below one jiffy. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
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
On Thu, Feb 29, 2024 at 7:11 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > Add the SoC I2C controller nodes to the platform devicetree. Use a > default bus frequency of 400kHz. They are AMBA devices that are matched > on PeriphID. > > Set transfer timeout to 10ms instead of Linux's default of 200ms. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Thu, Feb 29, 2024 at 7:11 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > Declare the temperature sensor on I2C bus 2. Its label is the schematics > identifier. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
Hi Theo, On Thu, Feb 29, 2024 at 07:10:51PM +0100, Théo Lebrun wrote: > Disambiguate the usage of dev as a variable name; it is usually best to > keep it reserved for struct device pointers. Avoid having multiple > names for the same struct pointer (previously: dev, nmk, nmk_i2c). just a nitpick here: this patch does also some small style fixes. Could you please mention them in the commit log in your v3? > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> In any case, Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Thanks, Andi
Hi Theo, On Thu, Feb 29, 2024 at 07:10:52PM +0100, Théo Lebrun wrote: > IRQ_MASK and I2C_CLEAR_ALL_INTS are redundant. One masks the top three if I2C_CLEAR_ALL_INTS is redundant why don't you remove it? > bits off as reserved, the other one masks the reserved IRQs inside the > u32. Get rid of IRQ_MASK and only use the most restrictive mask. Why is IRQ_MASK redundant? What happens if you write in the reserved bits? Can you please explain a bit better the change you did? Thanks, Andi > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com>
Hi Theo, ... > @@ -284,7 +290,7 @@ static int init_hw(struct nmk_i2c_dev *priv) > } > > /* enable peripheral, master mode operation */ > -#define DEFAULT_I2C_REG_CR ((1 << 1) | I2C_CR_PE) > +#define DEFAULT_I2C_REG_CR (FIELD_PREP(I2C_CR_OM, 0b01) | I2C_CR_PE) 0b01? > /** > * load_i2c_mcr_reg() - load the MCR register > @@ -296,41 +302,42 @@ static u32 load_i2c_mcr_reg(struct nmk_i2c_dev *priv, u16 flags) > u32 mcr = 0; > unsigned short slave_adr_3msb_bits; > > - mcr |= GEN_MASK(priv->cli.slave_adr, I2C_MCR_A7, 1); > + mcr |= FIELD_PREP(I2C_MCR_A7, priv->cli.slave_adr); > > if (unlikely(flags & I2C_M_TEN)) { > /* 10-bit address transaction */ > - mcr |= GEN_MASK(2, I2C_MCR_AM, 12); > + mcr |= FIELD_PREP(I2C_MCR_AM, 2); > /* > * Get the top 3 bits. > * EA10 represents extended address in MCR. This includes > * the extension (MSB bits) of the 7 bit address loaded > * in A7 > */ > - slave_adr_3msb_bits = (priv->cli.slave_adr >> 7) & 0x7; > + slave_adr_3msb_bits = FIELD_GET(GENMASK(9, 7), > + priv->cli.slave_adr); This looks like the only one not having a define. Shall we give a definition to GENMASK(9, 7)? > > - mcr |= GEN_MASK(slave_adr_3msb_bits, I2C_MCR_EA10, 8); > + mcr |= FIELD_PREP(I2C_MCR_EA10, slave_adr_3msb_bits); ... > @@ -824,15 +827,16 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg) > * during the transaction. > */ > case I2C_IT_BERR: > + { > + u32 sr = readl(priv->virtbase + I2C_SR); please give a blank line after the variable definition. Besides, I'd prefer the assignment, when it is a bit more complex, in a different line, as well. Rest looks OK, didn't see anything off. Andi
On Thu, Feb 29, 2024 at 07:10:51PM +0100, Théo Lebrun wrote: > Disambiguate the usage of dev as a variable name; it is usually best to > keep it reserved for struct device pointers. Avoid having multiple > names for the same struct pointer (previously: dev, nmk, nmk_i2c). > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> I think this improves readability a lot. I didn't really review, but I do like such changes: Acked-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
On Thu, Feb 29, 2024 at 07:10:54PM +0100, Théo Lebrun wrote: > Replace the completion by a waitqueue for synchronization from IRQ > handler to task. For short timeouts, use hrtimers, else use timers. > Usecase: avoid blocking the I2C bus for too long when an issue occurs. > > The threshold picked is one jiffy: if timeout is below that, use > hrtimers. This threshold is NOT configurable. > > Implement behavior but do NOT change fetching of timeout. This means the > timeout is unchanged (200ms) and the hrtimer case will never trigger. > > A waitqueue is used because it supports both desired timeout approaches. > See wait_event_timeout() and wait_event_hrtimeout(). An atomic boolean > serves as synchronization condition. > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> Largely: Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Nit: > - int timeout; > + int timeout_usecs; I think 'unsigned' makes a lot of sense here. Maybe u32 even?
On Thu, Feb 29, 2024 at 07:10:55PM +0100, Théo Lebrun wrote: > The FIFO flush function uses a jiffies amount to detect timeouts as the > flushing is async. Replace with ktime to get more accurate precision > and support short timeouts. > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
On Thu, Feb 29, 2024 at 07:10:56PM +0100, Théo Lebrun wrote: > Allow overriding the default timeout value (200ms) from devicetree, > using the generic i2c-transfer-timeout-us property. > > The i2c_adapter->timeout field is an unaccurate jiffies amount; > i2c-nomadik uses hrtimers for timeouts below one jiffy. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> I agree to get the DT property here in the driver. We may later refactor it to handle it in the I2C core. Syncing this new property with the existing 'adapter->timeout' will be a tad annoying, though. But I guess the shorter timeouts are a desired feature these days... Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> + 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.
Hello, On Sat Mar 2, 2024 at 1:39 AM CET, Andi Shyti wrote: > On Thu, Feb 29, 2024 at 07:10:52PM +0100, Théo Lebrun wrote: > > IRQ_MASK and I2C_CLEAR_ALL_INTS are redundant. One masks the top three > > if I2C_CLEAR_ALL_INTS is redundant why don't you remove it? I understand this is unclear. What I meant by redundant is that they are redundant from one another; one overlaps the other. I'll give a better commit description for v3. Something like: IRQ_MASK and I2C_CLEAR_ALL_INTS both mask available interrupts. IRQ_MASK removes top options (bits 29-31). I2C_CLEAR_ALL_INTS removes reserved options including top bits. Keep the latter. 31 29 27 25 23 21 19 17 15 13 11 09 07 05 03 01 30 28 26 24 22 20 18 16 14 12 10 08 06 04 02 00 --- IRQ_MASK: -------------------------------------------------- 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 0 0 0 --- I2C_CLEAR_ALL_INTS: ---------------------------------------- 1 1 1 1 1 1 1 1 1 1 1 1 1 1 1 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 Notice I2C_CLEAR_ALL_INTS is more restrictive than IRQ_MASK. Is that better? > > bits off as reserved, the other one masks the reserved IRQs inside the > > u32. Get rid of IRQ_MASK and only use the most restrictive mask. > > Why is IRQ_MASK redundant? What happens if you write in the > reserved bits? The wording wasn't correct. Have I answered your question from the above? Thanks Andi, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello, On Sat Mar 2, 2024 at 2:31 AM CET, Andi Shyti wrote: [...] > > @@ -284,7 +290,7 @@ static int init_hw(struct nmk_i2c_dev *priv) > > } > > > > /* enable peripheral, master mode operation */ > > -#define DEFAULT_I2C_REG_CR ((1 << 1) | I2C_CR_PE) > > +#define DEFAULT_I2C_REG_CR (FIELD_PREP(I2C_CR_OM, 0b01) | I2C_CR_PE) > > 0b01? OM is a two-bit field. We want to put 0b01 in that. We do not declare constants for its 4 potential values. Values are: - 0b00 slave mode - 0b01 master mode - 0b10 master/slave mode - 0b11 reserved To me the comment above DEFAULT_I2C_REG_CR is enough to show that we go into master mode. We could declare all values as constants but only 0b01 would get used. > > > /** > > * load_i2c_mcr_reg() - load the MCR register > > @@ -296,41 +302,42 @@ static u32 load_i2c_mcr_reg(struct nmk_i2c_dev *priv, u16 flags) > > u32 mcr = 0; > > unsigned short slave_adr_3msb_bits; > > > > - mcr |= GEN_MASK(priv->cli.slave_adr, I2C_MCR_A7, 1); > > + mcr |= FIELD_PREP(I2C_MCR_A7, priv->cli.slave_adr); > > > > if (unlikely(flags & I2C_M_TEN)) { > > /* 10-bit address transaction */ > > - mcr |= GEN_MASK(2, I2C_MCR_AM, 12); > > + mcr |= FIELD_PREP(I2C_MCR_AM, 2); > > /* > > * Get the top 3 bits. > > * EA10 represents extended address in MCR. This includes > > * the extension (MSB bits) of the 7 bit address loaded > > * in A7 > > */ > > - slave_adr_3msb_bits = (priv->cli.slave_adr >> 7) & 0x7; > > + slave_adr_3msb_bits = FIELD_GET(GENMASK(9, 7), > > + priv->cli.slave_adr); > > This looks like the only one not having a define. Shall we give a > definition to GENMASK(9, 7)? Indeed. What should it be named? It could be generic; this is about getting the upper 3 bits from an extended (10-bit) I2C address? > > - mcr |= GEN_MASK(slave_adr_3msb_bits, I2C_MCR_EA10, 8); > > + mcr |= FIELD_PREP(I2C_MCR_EA10, slave_adr_3msb_bits); > > ... > > > @@ -824,15 +827,16 @@ static irqreturn_t i2c_irq_handler(int irq, void *arg) > > * during the transaction. > > */ > > case I2C_IT_BERR: > > + { > > + u32 sr = readl(priv->virtbase + I2C_SR); > > please give a blank line after the variable definition. > > Besides, I'd prefer the assignment, when it is a bit more > complex, in a different line, as well. Will do. > Rest looks OK, didn't see anything off. Thanks for the review Andi! -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hello, On Mon Mar 4, 2024 at 10:18 AM CET, Wolfram Sang wrote: > On Thu, Feb 29, 2024 at 07:10:54PM +0100, Théo Lebrun wrote: > > Replace the completion by a waitqueue for synchronization from IRQ > > handler to task. For short timeouts, use hrtimers, else use timers. > > Usecase: avoid blocking the I2C bus for too long when an issue occurs. > > > > The threshold picked is one jiffy: if timeout is below that, use > > hrtimers. This threshold is NOT configurable. > > > > Implement behavior but do NOT change fetching of timeout. This means the > > timeout is unchanged (200ms) and the hrtimer case will never trigger. > > > > A waitqueue is used because it supports both desired timeout approaches. > > See wait_event_timeout() and wait_event_hrtimeout(). An atomic boolean > > serves as synchronization condition. > > > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > Largely: > > Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com> Thanks for the reviews Wolfram. > Nit: > > > - int timeout; > > + int timeout_usecs; > > I think 'unsigned' makes a lot of sense here. Maybe u32 even? Yes unsigned would make sense. unsigned int or u32, I wouldn't know which to pick. Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
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
> > > - int timeout; > > > + int timeout_usecs; > > > > I think 'unsigned' makes a lot of sense here. Maybe u32 even? > > Yes unsigned would make sense. unsigned int or u32, I wouldn't know > which to pick. It gets (later) fed by of_property_read_u32(), so I tend to suggest u32. Sounds like least conversions then but please double check.
Hi Theo, ... > +static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv) > +{ > + if (priv->timeout_usecs < jiffies_to_usecs(1)) { > + unsigned long timeout_usecs = priv->timeout_usecs; > + ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC); > + > + wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done, timeout); > + } else { > + unsigned long timeout = usecs_to_jiffies(priv->timeout_usecs); > + > + wait_event_timeout(priv->xfer_wq, priv->xfer_done, timeout); > + } > + > + return priv->xfer_done; You could eventually write this as static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv) { if (priv->timeout_usecs < jiffies_to_usecs(1)) { ... return !wait_event_hrtimeout(...); } ... return wait_event_timeout(...); } It looks a bit cleaner to me... your choice. Rest looks good. Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Andi
Hi Theo, On Thu, Feb 29, 2024 at 07:10:55PM +0100, Théo Lebrun wrote: > The FIFO flush function uses a jiffies amount to detect timeouts as the > flushing is async. Replace with ktime to get more accurate precision > and support short timeouts. > > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Thanks, Andi
Hi Theo, On Thu, Feb 29, 2024 at 07:10:56PM +0100, Théo Lebrun wrote: > Allow overriding the default timeout value (200ms) from devicetree, > using the generic i2c-transfer-timeout-us property. > > The i2c_adapter->timeout field is an unaccurate jiffies amount; > i2c-nomadik uses hrtimers for timeouts below one jiffy. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Thanks, Andi
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 >
Hello, On Mon Mar 4, 2024 at 2:54 PM CET, Andi Shyti wrote: > Hi Theo, > > ... > > > +static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv) > > +{ > > + if (priv->timeout_usecs < jiffies_to_usecs(1)) { > > + unsigned long timeout_usecs = priv->timeout_usecs; > > + ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC); > > + > > + wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done, timeout); > > + } else { > > + unsigned long timeout = usecs_to_jiffies(priv->timeout_usecs); > > + > > + wait_event_timeout(priv->xfer_wq, priv->xfer_done, timeout); > > + } > > + > > + return priv->xfer_done; > > You could eventually write this as > > static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv) > { > if (priv->timeout_usecs < jiffies_to_usecs(1)) { > ... > > return !wait_event_hrtimeout(...); > } > > ... > return wait_event_timeout(...); > } > > It looks a bit cleaner to me... your choice. The full block would become: static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv) { if (priv->timeout_usecs < jiffies_to_usecs(1)) { unsigned long timeout_usecs = priv->timeout_usecs; ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC); return !wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done, timeout); } return wait_event_timeout(priv->xfer_wq, priv->xfer_done, usecs_to_jiffies(priv->timeout_usecs)); } Three things: - Deindenting the jiffy timeout case means no variable declaration after the if-block. This is fine from my point-of-view. - It means we depend on the half-mess that are return values from wait_event_*timeout() macros. I wanted to avoid that because it looks like an error when you read the above code and see one is negated while the other is not. - Also, I'm not confident in casting either return value to bool; what happens if either macro returns an error? This is a theoretical case that shouldn't happen, but behavior might change at some point or bugs could occur. We know priv->xfer_done will give us the right answer. My preference still goes to the original version, but I'm happy we are having a discussion about this code block. > Reviewed-by: Andi Shyti <andi.shyti@kernel.org> Thanks for your review Andi! -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
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
Hi Theo, On Mon, Mar 04, 2024 at 03:32:38PM +0100, Théo Lebrun wrote: > On Mon Mar 4, 2024 at 2:54 PM CET, Andi Shyti wrote: > > > +static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv) > > > +{ > > > + if (priv->timeout_usecs < jiffies_to_usecs(1)) { > > > + unsigned long timeout_usecs = priv->timeout_usecs; > > > + ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC); > > > + > > > + wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done, timeout); > > > + } else { > > > + unsigned long timeout = usecs_to_jiffies(priv->timeout_usecs); > > > + > > > + wait_event_timeout(priv->xfer_wq, priv->xfer_done, timeout); > > > + } > > > + > > > + return priv->xfer_done; > > > > You could eventually write this as > > > > static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv) > > { > > if (priv->timeout_usecs < jiffies_to_usecs(1)) { > > ... > > > > return !wait_event_hrtimeout(...); > > } > > > > ... > > return wait_event_timeout(...); > > } > > > > It looks a bit cleaner to me... your choice. > > The full block would become: > > static bool nmk_i2c_wait_xfer_done(struct nmk_i2c_dev *priv) > { > if (priv->timeout_usecs < jiffies_to_usecs(1)) { > unsigned long timeout_usecs = priv->timeout_usecs; > ktime_t timeout = ktime_set(0, timeout_usecs * NSEC_PER_USEC); > > return !wait_event_hrtimeout(priv->xfer_wq, priv->xfer_done, > timeout); > } > > return wait_event_timeout(priv->xfer_wq, priv->xfer_done, > usecs_to_jiffies(priv->timeout_usecs)); > } > > Three things: > > - Deindenting the jiffy timeout case means no variable declaration > after the if-block. This is fine from my point-of-view. > > - It means we depend on the half-mess that are return values from > wait_event_*timeout() macros. I wanted to avoid that because it > looks like an error when you read the above code and see one is > negated while the other is not. > > - Also, I'm not confident in casting either return value to bool; what > happens if either macro returns an error? This is a theoretical case > that shouldn't happen, but behavior might change at some point or > bugs could occur. We know priv->xfer_done will give us the right > answer. > > My preference still goes to the original version, but I'm happy we are > having a discussion about this code block. sure... it's not a binding comment. Andi
Hi Theo, > Théo Lebrun (11): > dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example > dt-bindings: hwmon: lm75: use common hwmon schema > i2c: nomadik: rename private struct pointers from dev to priv > i2c: nomadik: simplify IRQ masking logic > i2c: nomadik: use bitops helpers > i2c: nomadik: support short xfer timeouts using waitqueue & hrtimer > i2c: nomadik: replace jiffies by ktime for FIFO flushing timeout > i2c: nomadik: fetch i2c-transfer-timeout-us property from devicetree > i2c: nomadik: support Mobileye EyeQ5 I2C controller > MIPS: mobileye: eyeq5: add 5 I2C controller nodes > MIPS: mobileye: eyeq5: add evaluation board I2C temp sensor what's your plan for this series? If you extract into a separate series the refactoring patches that are not dependent on the bindings I could queue them up for the merge window. Andi
Hello Andi, On Wed Mar 6, 2024 at 2:49 AM CET, Andi Shyti wrote: > > Théo Lebrun (11): > > dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example > > dt-bindings: hwmon: lm75: use common hwmon schema > > i2c: nomadik: rename private struct pointers from dev to priv > > i2c: nomadik: simplify IRQ masking logic > > i2c: nomadik: use bitops helpers > > i2c: nomadik: support short xfer timeouts using waitqueue & hrtimer > > i2c: nomadik: replace jiffies by ktime for FIFO flushing timeout > > i2c: nomadik: fetch i2c-transfer-timeout-us property from devicetree > > i2c: nomadik: support Mobileye EyeQ5 I2C controller > > MIPS: mobileye: eyeq5: add 5 I2C controller nodes > > MIPS: mobileye: eyeq5: add evaluation board I2C temp sensor > > what's your plan for this series? If you extract into a separate > series the refactoring patches that are not dependent on the > bindings I could queue them up for the merge window. V3 is ready and will be sent today. I think we can get trailers from dt-bindings maintainers as the discussion has been caried out on this revision. Am I being too optimistic of seeing this series queued before the merge window? Thanks Andi, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hi, This series adds two tangent features to the Nomadik I2C controller: - Add a new compatible to support Mobileye EyeQ5 which uses the same IP block as Nomadik. It has two quirks to be handled: - The memory bus only supports 32-bit accesses. Avoid readb() and writeb() calls that might generate byte load/store instructions. - We must write a value into a shared register region (OLB) depending on the I2C bus speed. - Allow xfer timeouts below one jiffy by using a waitqueue and hrtimers instead of a completion. The situation to be addressed is: - Many devices on the same I2C bus. - One xfer to each device is sent at regular interval. - One device gets stuck and does not answer. - With long timeouts, following devices won't get their message. A shorter timeout ensures we can still talk to the following devices. This clashes a bit with the current i2c_adapter timeout field that stores a jiffies amount. We therefore avoid it and store the value in a private struct field, as a µs amount. If the timeout is less than a jiffy duration, we switch from standard jiffies timeout to hrtimers. There is one patch targeting a hwmon dt-bindings file: Documentation/devicetree/bindings/hwmon/lm75.yaml. The rest is touching the I2C bus driver, its bindings and platform devicetrees. About dependencies: - The series is based upon v6.8-rc6. - For testing on EyeQ5 hardware and devicetree patches, we need the base platform series from Grégory [0] and its dependency [1]. Both in mips-next [2]. - Devicetree commits require the EyeQ5 syscon series [3] that provides the reset controller node. - The LM75 dt-bindings patch depends on the common schema hwmon-common.yaml series from Krzysztof [4]. Found in hwmon-next [5]. Have a nice day, Théo Lebrun [0]: https://lore.kernel.org/lkml/20240216174227.409400-1-gregory.clement@bootlin.com/ [1]: https://lore.kernel.org/linux-mips/20240209-regname-v1-0-2125efa016ef@flygoat.com/ [2]: https://git.kernel.org/pub/scm/linux/kernel/git/mips/linux.git/log/ [3]: https://lore.kernel.org/lkml/20240227-mbly-clk-v8-0-c57fbda7664a@bootlin.com/ [4]: https://lore.kernel.org/lkml/20240224-dt-bindings-hwmon-common-v2-0-b446eecf5480@linaro.org/ [5]: https://git.kernel.org/pub/scm/linux/kernel/git/groeck/linux-staging.git/log/?h=hwmon-next To: Linus Walleij <linus.walleij@linaro.org> To: Andi Shyti <andi.shyti@kernel.org> To: Rob Herring <robh+dt@kernel.org> To: Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org> To: Conor Dooley <conor+dt@kernel.org> To: Thomas Bogendoerfer <tsbogend@alpha.franken.de> Cc: <linux-arm-kernel@lists.infradead.org> Cc: <linux-i2c@vger.kernel.org> Cc: <devicetree@vger.kernel.org> Cc: <linux-kernel@vger.kernel.org> Cc: <linux-mips@vger.kernel.org> Cc: Gregory Clement <gregory.clement@bootlin.com> Cc: Vladimir Kondratiev <vladimir.kondratiev@mobileye.com> Cc: Thomas Petazzoni <thomas.petazzoni@bootlin.com> Cc: Tawfik Bayouk <tawfik.bayouk@mobileye.com> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> Changes in v2: - dt-bindings: i2c: st,nomadic-i2c: - Drop timeout-usecs property, rely on generic i2c-transfer-timeout-us. - Use phandle to syscon with cell args; remove mobileye,id prop; move mobileye,olb from if-statement to top-level. - dt-bindings: hwmon: lm75: - Inherit from hwmon-common.yaml rather than declare generic label property. - i2c: nomadik: (ie driver code) - Parse i2c-transfer-timeout-us rather than custom timeout-usecs property. - Introduce readb/writeb helpers with fallback to readl/writel. - Avoid readb() on Mobileye. - Use mobileye,olb cell args to get controller index rather than mobileye,id. - Take 5 Reviewed-by Linus Walleij. - MIPS: mobileye: (ie devicetrees) - Use mobileye,olb with cell args rather than mobileye,id. - Squash reset commit. - Add i2c-transfer-timeout-us value of 10ms to all controllers. - Rename LM75 instance from tmp112@48 to temperature-sensor@48. - Link to v1: https://lore.kernel.org/r/20240215-mbly-i2c-v1-0-19a336e91dca@bootlin.com --- Théo Lebrun (11): dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example dt-bindings: hwmon: lm75: use common hwmon schema i2c: nomadik: rename private struct pointers from dev to priv i2c: nomadik: simplify IRQ masking logic i2c: nomadik: use bitops helpers i2c: nomadik: support short xfer timeouts using waitqueue & hrtimer i2c: nomadik: replace jiffies by ktime for FIFO flushing timeout i2c: nomadik: fetch i2c-transfer-timeout-us property from devicetree i2c: nomadik: support Mobileye EyeQ5 I2C controller MIPS: mobileye: eyeq5: add 5 I2C controller nodes MIPS: mobileye: eyeq5: add evaluation board I2C temp sensor Documentation/devicetree/bindings/hwmon/lm75.yaml | 3 +- .../devicetree/bindings/i2c/st,nomadik-i2c.yaml | 48 +- arch/mips/boot/dts/mobileye/eyeq5-epm5.dts | 8 + arch/mips/boot/dts/mobileye/eyeq5.dtsi | 75 +++ drivers/i2c/busses/i2c-nomadik.c | 720 ++++++++++++--------- 5 files changed, 541 insertions(+), 313 deletions(-) --- base-commit: a6cc37d1a531e1c99e7989001a0529b443397900 change-id: 20231023-mbly-i2c-7c2fbbb1299f Best regards,