mbox series

[v2,00/11] Add Mobileye EyeQ5 support to the Nomadik I2C controller & use hrtimers for timeouts

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

Message

Théo Lebrun Feb. 29, 2024, 6:10 p.m. UTC
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,

Comments

Linus Walleij Feb. 29, 2024, 9:04 p.m. UTC | #1
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
Linus Walleij Feb. 29, 2024, 9:08 p.m. UTC | #2
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
Linus Walleij Feb. 29, 2024, 9:09 p.m. UTC | #3
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
Linus Walleij Feb. 29, 2024, 9:09 p.m. UTC | #4
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
Andi Shyti March 2, 2024, 12:16 a.m. UTC | #5
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
Andi Shyti March 2, 2024, 12:39 a.m. UTC | #6
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>
Andi Shyti March 2, 2024, 1:31 a.m. UTC | #7
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
Wolfram Sang March 4, 2024, 9:13 a.m. UTC | #8
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>
Wolfram Sang March 4, 2024, 9:18 a.m. UTC | #9
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?
Wolfram Sang March 4, 2024, 9:23 a.m. UTC | #10
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>
Wolfram Sang March 4, 2024, 9:25 a.m. UTC | #11
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>
Wolfram Sang March 4, 2024, 9:27 a.m. UTC | #12
> +	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, 9:46 a.m. UTC | #13
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
Théo Lebrun March 4, 2024, 10 a.m. UTC | #14
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
Théo Lebrun March 4, 2024, 10:14 a.m. UTC | #15
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
Théo Lebrun March 4, 2024, 10:25 a.m. UTC | #16
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
Wolfram Sang March 4, 2024, 11:37 a.m. UTC | #17
> > > -	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.
Andi Shyti March 4, 2024, 1:54 p.m. UTC | #18
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
Andi Shyti March 4, 2024, 1:55 p.m. UTC | #19
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
Andi Shyti March 4, 2024, 1:57 p.m. UTC | #20
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
Andi Shyti March 4, 2024, 2:08 p.m. UTC | #21
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:32 p.m. UTC | #22
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
Théo Lebrun March 4, 2024, 2:53 p.m. UTC | #23
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
Andi Shyti March 4, 2024, 3:09 p.m. UTC | #24
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
Andi Shyti March 6, 2024, 1:49 a.m. UTC | #25
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
Théo Lebrun March 6, 2024, 9:34 a.m. UTC | #26
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