| Message ID | 20240215-mbly-i2c-v1-0-19a336e91dca@bootlin.com |
|---|---|
| Headers | show |
| Series | Add Mobileye EyeQ5 support to the Nomadik I2C controller & use hrtimers for timeouts | expand |
On 15/02/2024 17:52, Théo Lebrun 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> > --- > arch/mips/boot/dts/mobileye/eyeq5-epm5.dts | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts b/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts > index 6898b2d8267d..1f8549acd40d 100644 > --- a/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts > +++ b/arch/mips/boot/dts/mobileye/eyeq5-epm5.dts > @@ -21,3 +21,11 @@ memory@0 { > <0x8 0x02000000 0x0 0x7E000000>; > }; > }; > + > +&i2c2 { > + tmp112@48 { Node names should be generic. See also an explanation and list of examples (not exhaustive) in DT specification: https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation temperature-sensor, power-sensor etc. Best regards, Krzysztof
On 15/02/2024 17:52, Théo Lebrun wrote: > Add resets properties to each I2C controller. This depends on the > reset-eyeq5 platform reset controller driver. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > --- This should be squashed with previous patch adding i2c controllers. Don't add incomplete nodes just to fix them in next patch. Best regards, Krzysztof
Hello, On Fri Feb 16, 2024 at 8:59 AM CET, Krzysztof Kozlowski wrote: > On 15/02/2024 17:52, Théo Lebrun wrote: > > Add resets properties to each I2C controller. This depends on the > > reset-eyeq5 platform reset controller driver. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > --- > > This should be squashed with previous patch adding i2c controllers. > Don't add incomplete nodes just to fix them in next patch. The goal was to isolate reset phandles to a single patch. The series with this patch dropped works because resets in their default state are deasserted, so this isn't a fix. And it allows testing the series on hardware with only the base platform series, which I found useful. Noted, I'll be squashed for next revision. Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On 16/02/2024 10:05, Théo Lebrun wrote: > Hello, > > On Fri Feb 16, 2024 at 8:59 AM CET, Krzysztof Kozlowski wrote: >> On 15/02/2024 17:52, Théo Lebrun wrote: >>> Add resets properties to each I2C controller. This depends on the >>> reset-eyeq5 platform reset controller driver. >>> >>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> >>> --- >> >> This should be squashed with previous patch adding i2c controllers. >> Don't add incomplete nodes just to fix them in next patch. > > The goal was to isolate reset phandles to a single patch. The series That was what you did, not the goal. If that's the goal, then it is clearly wrong. > with this patch dropped works because resets in their default state are > deasserted, so this isn't a fix. And it allows testing the series on > hardware with only the base platform series, which I found useful. Series or half-of-series? Anyway, commits must be logical chunks, so one chunk is to add I2C controllers, not "part of I2C controllers". DTS is also independent of drivers (and it will go via different trees!), so whatever dependency you think of, it does not exist. Best regards, Krzysztof
Hello, On Fri Feb 16, 2024 at 10:17 AM CET, Krzysztof Kozlowski wrote: > On 16/02/2024 10:05, Théo Lebrun wrote: > > Hello, > > > > On Fri Feb 16, 2024 at 8:59 AM CET, Krzysztof Kozlowski wrote: > >> On 15/02/2024 17:52, Théo Lebrun wrote: > >>> Add resets properties to each I2C controller. This depends on the > >>> reset-eyeq5 platform reset controller driver. > >>> > >>> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > >>> --- > >> > >> This should be squashed with previous patch adding i2c controllers. > >> Don't add incomplete nodes just to fix them in next patch. > > > > The goal was to isolate reset phandles to a single patch. The series > > That was what you did, not the goal. If that's the goal, then it is > clearly wrong. > > > with this patch dropped works because resets in their default state are > > deasserted, so this isn't a fix. And it allows testing the series on > > hardware with only the base platform series, which I found useful. > > Series or half-of-series? Anyway, commits must be logical chunks, so one > chunk is to add I2C controllers, not "part of I2C controllers". DTS is > also independent of drivers (and it will go via different trees!), so > whatever dependency you think of, it does not exist. My reasoning was focused on my point-of-view as a contributor and tester of the series. Your explanation makes sense; I had never thought this through from the maintainer's POV. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > - Add a new compatible to support Mobileye EyeQ5 which uses the same IP > block as Nomadik. Sweet! I'm amazed ST Micro licensed this "ARM PrimeCell" to Mobileye, but it's a well tested IP and used in eg ST automotive SoC:s so it's a solid product. It feels worth it for all the time I have put into maintaining it, finally some real users again! :) Yours, Linus Walleij
On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> 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). > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> Fair enough, it's more readable like this. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > IRQ_MASK and I2C_CLEAR_ALL_INTS are redundant. One masks the top three > 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. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> Yeah, more readable like this definitely. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > Constant register bit fields are declared using hardcoded hex values; > replace them by calls to BIT() and GENMASK(). Replace custom GEN_MASK() > macro by the generic FIELD_PREP(). Replace manual bit manipulations by > the generic FIELD_GET() macro. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> I'm a fan of this style. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> 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. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> Part of me want to go and fix completions to handle hrtimer timeouts for submicrosecond timeouts, BUT I realized that this is a bit thick request for a simple driver, so just a suggestion for something we could do one day. This is fine with me. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> 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. > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> Excellent patch. Thanks. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > Allow overriding the default timeout value (200ms) from devicetree, > using the timeout-usecs 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> Once we agree on the binding name: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
Hello, On Mon Feb 19, 2024 at 3:11 PM CET, Linus Walleij wrote: > On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > > > - Add a new compatible to support Mobileye EyeQ5 which uses the same IP > > block as Nomadik. > > Sweet! I'm amazed ST Micro licensed this "ARM PrimeCell" to Mobileye, but > it's a well tested IP and used in eg ST automotive SoC:s so it's a solid > product. > > It feels worth it for all the time I have put into maintaining it, finally some > real users again! :) Using the existing Nomadik drivers with the AMBA bus infrastructure on a non-ARM platform and having it work as-is made our day here at Bootlin. We are indeed grateful for your work maintaining this platform! Regards, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ------------------------------------------------------------------------
Hello, On Mon Feb 19, 2024 at 3:19 PM CET, Linus Walleij wrote: > On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> 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. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > Part of me want to go and fix completions to handle hrtimer timeouts > for submicrosecond timeouts, BUT I realized that this is a bit thick > request for a simple driver, so just a suggestion for something we could > do one day. This is fine with me. > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Indeed having to switch to another abstraction because we desire another timeout method is nonsensical. Completion supporting hrtimeouts would make sense. As you said though, this is too much for a simple driver. Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com ------------------------------------------------------------------------
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:21 PM CET, Linus Walleij wrote: > On Thu, Feb 15, 2024 at 5:52 PM Théo Lebrun <theo.lebrun@bootlin.com> 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. > > > > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> > > Excellent patch. Thanks. > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Somewhat related to this patch: while writing it, I noticed the total timeout of flush_i2c_fifo() is 10 times the timeout. Without this series, this means 10*200ms of busywaiting! If you have an opinion on a more sensible option for this I could add a patch to my V2. I don't know enough to pick a sensible value. I'm unsure if it makes sense that the timeout of flush_i2c_fifo() is a multiple of the transfer timeout. Does it make sense that those two timeouts are correlated? Big thanks for your review, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
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
On Mon, Feb 19, 2024 at 3:38 PM Théo Lebrun <theo.lebrun@bootlin.com> wrote: > Somewhat related to this patch: while writing it, I noticed the total > timeout of flush_i2c_fifo() is 10 times the timeout. Without this > series, this means 10*200ms of busywaiting! > > If you have an opinion on a more sensible option for this I could add a > patch to my V2. I don't know enough to pick a sensible value. > > I'm unsure if it makes sense that the timeout of flush_i2c_fifo() is a > multiple of the transfer timeout. Does it make sense that those two > timeouts are correlated? I have a *vague* memory of the timeouts for flushing needing to be longer but I might be mistaken. This is probably a Srinidhi or even Sachin question... Sadly I don't have their current mail addresses. Yours, Linus Walleij
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
> + /* Slave response timeout */ > + if (!of_property_read_u32(np, "timeout-usecs", &timeout_usecs)) > + priv->timeout_usecs = timeout_usecs; > + else > + priv->timeout_usecs = 200 * USEC_PER_MSEC; I could imagine to add 'transfer_timeout_us' to struct i2c_timings. Then, you could use 'i2c_parse_fw_timings' to obtain the value. What values/value range do you use here? I can't find them in the DTS additions.
Hello, On Tue Feb 27, 2024 at 1:14 PM CET, Wolfram Sang wrote: > > + /* Slave response timeout */ > > + if (!of_property_read_u32(np, "timeout-usecs", &timeout_usecs)) > > + priv->timeout_usecs = timeout_usecs; > > + else > > + priv->timeout_usecs = 200 * USEC_PER_MSEC; > > I could imagine to add 'transfer_timeout_us' to struct i2c_timings. > Then, you could use 'i2c_parse_fw_timings' to obtain the value. What > values/value range do you use here? I can't find them in the DTS > additions. That sounds good. I have not used this prop in the DTS as it does not make much sense for an eval board. The target is production boards. An order of magnitude is a few transfers every 15ms. It means a timeout of 15ms divided by "a few". I don't have more precise values, but I could if you consider it useful. I've done some testing at 50~100µs timeouts and it works as expected. At those values timerslack is important to consider (default of 50µs). This is at 400kHz clock frequency. Keep in mind the controllers support up to 3.4MHz (not yet upstreamed) so timeouts could in theory go lower if required by the usecase. My upcoming question is how to move forward on this series. I can do the patch to i2c_parse_fw_timings() in the next revision. That way it gets added alongside the first user of this feature. Would it work for you? Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hi Théo, > That sounds good. I have not used this prop in the DTS as it does not > make much sense for an eval board. The target is production boards. ... > My upcoming question is how to move forward on this series. I can do the > patch to i2c_parse_fw_timings() in the next revision. That way it gets > added alongside the first user of this feature. Would it work for you? Hmmm, to be honest I have a bit of an issue with the 'no user' problem. There is a driver which uses this feature, okay. But there is no upstream hardware which uses this driver with this new feature. This makes maintaining harder ("Who uses this feature?" - "Someone" - "How do they use it? Can we modify it?" - "Dunno"). Kind regards, Wolfram
Hello Wolfram, On Wed Feb 28, 2024 at 11:49 AM CET, Wolfram Sang wrote: > > That sounds good. I have not used this prop in the DTS as it does not > > make much sense for an eval board. The target is production boards. > > ... > > > My upcoming question is how to move forward on this series. I can do the > > patch to i2c_parse_fw_timings() in the next revision. That way it gets > > added alongside the first user of this feature. Would it work for you? > > Hmmm, to be honest I have a bit of an issue with the 'no user' problem. > There is a driver which uses this feature, okay. But there is no > upstream hardware which uses this driver with this new feature. This > makes maintaining harder ("Who uses this feature?" - "Someone" - "How do > they use it? Can we modify it?" - "Dunno"). The alternative is that I keep going with a new revision of i2c-nomadik that manually parses the prop. It'll be refactored if/when the I2C core provides a better way to access the value. Is that OK? Thanks, -- Théo Lebrun, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
> The alternative is that I keep going with a new revision of i2c-nomadik > that manually parses the prop. It'll be refactored if/when the I2C core > provides a better way to access the value. Is that OK? That wouldn't have helped because there is still no user in-kernel of that property, i.e. no DTS file with that property. But I just realized that I need to convert i2c-mpc to avoid a deprecated binding, so we have a user there. Lucky you ;) I'll try to get the series done today.
Hello, On Thu Feb 29, 2024 at 10:25 AM CET, Wolfram Sang wrote: > > The alternative is that I keep going with a new revision of i2c-nomadik > > that manually parses the prop. It'll be refactored if/when the I2C core > > provides a better way to access the value. Is that OK? > > That wouldn't have helped because there is still no user in-kernel of > that property, i.e. no DTS file with that property. But I just realized > that I need to convert i2c-mpc to avoid a deprecated binding, so we have > a user there. Lucky you ;) > > I'll try to get the series done today. Lucky me indeed! I just thought about it but I don't mind using the property in the eval board DTS. The default 200ms timeout makes no sense for us. Using a default of a few ms would be more sensible and make us the second user of the prop. Thanks, -- 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. A writeb() is used which we avoid. - We must write a value into a shared register region (OLB, "Other Logic Block") depending on the I2C bus speed. - Allow xfer timeouts below one jiffy by using a workqueue 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 cannot rely on it and therefore we take a value from devicetree as a µs value. 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-rc4. - For testing on EyeQ5 hardware and devicetree patches, we need the base platform series from Grégory [0]. - The last commit (adding DT phandles for resets), we need the syscon series [1] that provides the reset controller node. I think there are discussions to be had about: - The handling of timeouts. Having a non-jiffy value is not driver specific. Should this change be done at the subsystem layer? The subsystem could even fetch the value from devicetree and auto-fill timeout, with a default given by the driver. Not many drivers seem to use the i2c_adapter timeout field from my quick grepping. - The DT prop for timeout. I've picked "timeout-usecs". Some drivers use vendor prefixes, but this is not vendor-specific and only a software implementation detail. - The shape of this series. Initially it was split in two. However I brought them together as they cannot be applied independently. Please tell me if a better approach is to be preferred. Those are thoughts, I'm sure people will have feedback on this. Have a nice day, Théo Lebrun [0]: https://lore.kernel.org/lkml/20240205153503.574468-1-gregory.clement@bootlin.com/ [1]: https://lore.kernel.org/lkml/20240212-mbly-clk-v6-0-c46fa1f93839@bootlin.com/ Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- Théo Lebrun (13): dt-bindings: i2c: nomadik: add timeout-usecs property bindings dt-bindings: i2c: nomadik: add mobileye,eyeq5-i2c bindings and example dt-bindings: hwmon: lm75: add label property 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 timeout-usecs 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 MIPS: mobileye: eyeq5: add resets to I2C controllers Documentation/devicetree/bindings/hwmon/lm75.yaml | 4 + .../devicetree/bindings/i2c/st,nomadik-i2c.yaml | 49 +- arch/mips/boot/dts/mobileye/eyeq5-epm5.dts | 8 + arch/mips/boot/dts/mobileye/eyeq5.dtsi | 75 +++ drivers/i2c/busses/i2c-nomadik.c | 710 ++++++++++++--------- 5 files changed, 534 insertions(+), 312 deletions(-) --- base-commit: d55aa725e32849f709b61eab3b7a50b810a71a84 change-id: 20231023-mbly-i2c-7c2fbbb1299f Best regards,