i2c: npcm: Bug fixes timeout, spurious interrupts

From: Tyrone Ting <[email protected]>

Add nuvoton,sys-mgr property for controlling NPCM gcr register.

Signed-off-by Tyrone Ting <[email protected]>

Signed-off-by Tali Perry <[email protected]>

Quoted 4 lines

From: Tyrone Ting <[email protected]>

Add nuvoton,sys-mgr property for controlling NPCM gcr register.

Quoted 2 lines

Signed-off-by Tyrone Ting <[email protected]>

Signed-off-by Tali Perry <[email protected]>

There are some comments about this series, so I am expecting a v4 somewhen. However, I already want to state that I usually don't take DTS patches. So, I guess the path forward is that Rob needs to ack the patch which is now patch 2. Once he does this and I apply it, you can take this DTS patch via arm-soc. Sounds good?

There are still some discussions for the patch V4 and it might take some time though.

Yes, the dts patch could be submitted via arm-soc.

I really appreciate your comments.

From: Tyrone Ting <[email protected]>

Add compatible and nuvoton,sys-mgr description for NPCM i2c module.

Signed-off-by Tyrone Ting <[email protected]>

Signed-off-by Tali Perry <[email protected]>

---
 .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml     | 26 +++++++++++++++----
 1 file changed, 21 insertions(+), 5 deletions(-)

Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml +10 -5

index 128444942aec..37976ddcf406 100644
@@ -7,17 +7,18 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
 title: nuvoton NPCM7XX I2C Controller Device Tree Bindings
 
 description: |
-  The NPCM750x includes sixteen I2C bus controllers. All Controllers support
-  both master and slave mode. Each controller can switch between master and slave
-  at run time (i.e. IPMB mode). Each controller has two 16 byte HW FIFO for TX and
-  RX.
+  I2C bus controllers of the NPCM series support both master and
+  slave mode. Each controller can switch between master and slave at run time
+  (i.e. IPMB mode). HW FIFO for TX and RX are supported.
 
 maintainers:
   - Tali Perry <[email protected]>
 
 properties:
   compatible:
-    const: nuvoton,npcm750-i2c
+    enum:
+      - nuvoton,npcm750-i2c
+      - nuvoton,npcm845-i2c
 
   reg:
     maxItems: 1

@@ -36,6 +37,10 @@ properties:
     default: 100000
     enum: [100000, 400000, 1000000]
 
+  nuvoton,sys-mgr:
+    $ref: "/schemas/types.yaml#/definitions/phandle"
+    description: The phandle of system manager register node.
+
 required:
   - compatible
   - reg

Quoted 50 lines

From: Tyrone Ting <[email protected]>

Add compatible and nuvoton,sys-mgr description for NPCM i2c module.

Signed-off-by Tyrone Ting <[email protected]>

Signed-off-by Tali Perry <[email protected]>

---
 .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml     | 26 +++++++++++++++----
 1 file changed, 21 insertions(+), 5 deletions(-)

Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml +10 -5

index 128444942aec..37976ddcf406 100644
@@ -7,17 +7,18 @@ $schema: http://devicetree.org/meta-schemas/core.yaml#
 title: nuvoton NPCM7XX I2C Controller Device Tree Bindings
 
 description: |
-  The NPCM750x includes sixteen I2C bus controllers. All Controllers support
-  both master and slave mode. Each controller can switch between master and slave
-  at run time (i.e. IPMB mode). Each controller has two 16 byte HW FIFO for TX and
-  RX.
+  I2C bus controllers of the NPCM series support both master and
+  slave mode. Each controller can switch between master and slave at run time
+  (i.e. IPMB mode). HW FIFO for TX and RX are supported.
 
 maintainers:
   - Tali Perry <[email protected]>
 
 properties:
   compatible:
-    const: nuvoton,npcm750-i2c
+    enum:
+      - nuvoton,npcm750-i2c
+      - nuvoton,npcm845-i2c
 
   reg:
     maxItems: 1

@@ -36,6 +37,10 @@ properties:
     default: 100000
     enum: [100000, 400000, 1000000]
 
+  nuvoton,sys-mgr:
+    $ref: "/schemas/types.yaml#/definitions/phandle"
+    description: The phandle of system manager register node.
+
 required:
   - compatible
   - reg

Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml required: · line 57

Quoted 9 lines

Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml +6 -0

@@ -44,6 +49,16 @@ required:
 
 allOf:
   - $ref: /schemas/i2c/i2c-controller.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const:
+              - nuvoton,npcm845-i2c

This should be one line in const (not an enum).

Rest looks good to me.

Best regards,
Krzysztof

From: Tali Perry <[email protected]>

Fix i2c client address by left-shifting 1 bit before applying it to the data register.

Quoted 4 lines

From: Tali Perry <[email protected]>

Fix i2c client address by left-shifting 1 bit before applying it to the data register.

...

drivers/i2c/busses/i2c-npcm7xx.c static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs, · line 2134

Quoted 2 lines

drivers/i2c/busses/i2c-npcm7xx.c +1 -1

-     bus->dest_addr = slave_addr;
+     bus->dest_addr = slave_addr << 1;

1. Why this is not using i2c_8bit_addr_from_msg() helper?
2. This is duplication of what npcm_i2c_master_start_xmit() does.

Taking 2 into account, what is this exactly fixing? Sounds like a red herring.

--
With Best Regards,
Andy Shevchenko

From: Tali Perry <[email protected]>

Change the way of getting NPCM system manager reigster (GCR) and still maintain the old mechanism as a fallback if getting nuvoton,sys-mgr fails while working with the legacy devicetree file.

Fixes 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")

Signed-off-by Tali Perry <[email protected]>

Signed-off-by Tyrone Ting <[email protected]>

---
 drivers/i2c/busses/i2c-npcm7xx.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Quoted 7 lines

From: Tali Perry <[email protected]>

Change the way of getting NPCM system manager reigster (GCR) and still maintain the old mechanism as a fallback if getting nuvoton,sys-mgr fails while working with the legacy devicetree file.

Quoted 7 lines

Fixes 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")

Signed-off-by Tali Perry <[email protected]>

Signed-off-by Tyrone Ting <[email protected]>

---
 drivers/i2c/busses/i2c-npcm7xx.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Acked-by Krzysztof Kozlowski <[email protected]>

Best regards,
Krzysztof

From: Tyrone Ting <[email protected]>

Use ioread8 instead of ioread32 to access the SMBnCTL3 register since the register is only 8-bit wide.

Fixes 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")

Signed-off-by Tyrone Ting <[email protected]>

Signed-off-by Tali Perry <[email protected]>

drivers/i2c/busses/i2c-npcm7xx.c +1 -1

-	return !!(I2CCTL3_SCL_LVL & ioread32(bus->reg + NPCM_I2CCTL3));
+	return !!(I2CCTL3_SCL_LVL & ioread8(bus->reg + NPCM_I2CCTL3));

drivers/i2c/busses/i2c-npcm7xx.c +1 -1

-	return !!(I2CCTL3_SDA_LVL & ioread32(bus->reg + NPCM_I2CCTL3));
+	return !!(I2CCTL3_SDA_LVL & ioread8(bus->reg + NPCM_I2CCTL3));

Quoted 6 lines

From: Tyrone Ting <[email protected]>

Use ioread8 instead of ioread32 to access the SMBnCTL3 register since the register is only 8-bit wide.

Fixes 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")

No, this is bad commit message, since you have bitwise masks and there is nothing to fix from functional point of view. So, why is this a fix?

Quoted 2 lines

Signed-off-by Tyrone Ting <[email protected]>

Signed-off-by Tali Perry <[email protected]>

This is wrong SoB chain.

...

drivers/i2c/busses/i2c-npcm7xx.c static int npcm_i2c_get_SCL(struct i2c_adapter *_adap) · line 363

Quoted 2 lines

drivers/i2c/busses/i2c-npcm7xx.c +1 -1

-     return !!(I2CCTL3_SCL_LVL & ioread32(bus->reg + NPCM_I2CCTL3));
+     return !!(I2CCTL3_SCL_LVL & ioread8(bus->reg + NPCM_I2CCTL3));

...

drivers/i2c/busses/i2c-npcm7xx.c static int npcm_i2c_get_SCL(struct i2c_adapter *_adap) · line 370

Quoted 2 lines

drivers/i2c/busses/i2c-npcm7xx.c +1 -1

-     return !!(I2CCTL3_SDA_LVL & ioread32(bus->reg + NPCM_I2CCTL3));
+     return !!(I2CCTL3_SDA_LVL & ioread8(bus->reg + NPCM_I2CCTL3));

--
With Best Regards,
Andy Shevchenko

Quoted 11 lines · Tyrone Ting

Quoted 6 lines

From: Tyrone Ting <[email protected]>

Use ioread8 instead of ioread32 to access the SMBnCTL3 register since the register is only 8-bit wide.

Fixes 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")

No, this is bad commit message, since you have bitwise masks and there is nothing to fix from functional point of view. So, why is this a fix?

The next gen of this device is a 64 bit cpu.
The module is and was 8 bit.

The ioread32 that seemed to work smoothly on a 32 bit machine was causing a panic on a 64 bit machine.
since the module is 8 bit we changed to ioread8. This is working both for the 32 and 64 CPUs with no issue.

Quoted 15 lines · Tali Perry

Quoted 11 lines · Tyrone Ting

Quoted 6 lines

From: Tyrone Ting <[email protected]>

Use ioread8 instead of ioread32 to access the SMBnCTL3 register since the register is only 8-bit wide.

Fixes 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")

No, this is bad commit message, since you have bitwise masks and there is nothing to fix from functional point of view. So, why is this a fix?

The next gen of this device is a 64 bit cpu.
The module is and was 8 bit.

Quoted 4 lines

The ioread32 that seemed to work smoothly on a 32 bit machine was causing a panic on a 64 bit machine.
since the module is 8 bit we changed to ioread8. This is working both for the 32 and 64 CPUs with no issue.

Then the commit message is completely wrong here.

And provide necessary (no need to have noisy commit messages) bits of the oops to show what's going on

Hello,

Quoted 16 lines · Andy Shevchenko

Quoted 15 lines · Tali Perry

Quoted 11 lines · Tyrone Ting

Quoted 6 lines

From: Tyrone Ting <[email protected]>

Use ioread8 instead of ioread32 to access the SMBnCTL3 register since the register is only 8-bit wide.

Fixes 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")

No, this is bad commit message, since you have bitwise masks and there is nothing to fix from functional point of view. So, why is this a fix?

The next gen of this device is a 64 bit cpu.
The module is and was 8 bit.

Quoted 6 lines

Quoted 4 lines

The ioread32 that seemed to work smoothly on a 32 bit machine was causing a panic on a 64 bit machine.
since the module is 8 bit we changed to ioread8. This is working both for the 32 and 64 CPUs with no issue.

Then the commit message is completely wrong here.

I disagree: The commit message is perhaps incomplete, but not wrong. The SMBnCTL3 register was specified as 8 bits wide in the datasheets of multiple chip generations, as far as I can tell, but the driver wrongly made a 32-bit access, which just happened not to blow up.

So, indeed, "since the register is only 8-bit wide" seems to be a correct claim.

Quoted 2 lines

And provide necessary (no need to have noisy commit messages) bits of the oops to show what's going on

I guess it's blowing up now because SMBnCTL3 isn't 32-bit aligned (being at offset 0x0e in the controller).

Jonathan

Quoted 38 lines · Jonathan Neuschäfer

Hello,

Quoted 16 lines · Andy Shevchenko

Quoted 15 lines · Tali Perry

Quoted 11 lines · Tyrone Ting

Quoted 6 lines

From: Tyrone Ting <[email protected]>

Use ioread8 instead of ioread32 to access the SMBnCTL3 register since the register is only 8-bit wide.

Fixes 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")

No, this is bad commit message, since you have bitwise masks and there is nothing to fix from functional point of view. So, why is this a fix?

The next gen of this device is a 64 bit cpu.
The module is and was 8 bit.

Quoted 6 lines

Quoted 4 lines

The ioread32 that seemed to work smoothly on a 32 bit machine was causing a panic on a 64 bit machine.
since the module is 8 bit we changed to ioread8. This is working both for the 32 and 64 CPUs with no issue.

Then the commit message is completely wrong here.

I disagree: The commit message is perhaps incomplete, but not wrong. The SMBnCTL3 register was specified as 8 bits wide in the datasheets of multiple chip generations, as far as I can tell, but the driver wrongly made a 32-bit access, which just happened not to blow up.

So, indeed, "since the register is only 8-bit wide" seems to be a correct claim.

Quoted 2 lines

And provide necessary (no need to have noisy commit messages) bits of the oops to show what's going on

Quoted 3 lines

I guess it's blowing up now because SMBnCTL3 isn't 32-bit aligned (being at offset 0x0e in the controller).

Hi Andy,
After this clarification can you please acknowledge this specific patch? If you think there is a better way to describe this, can you propose one?

From: Tali Perry <[email protected]>

In order to better handle spurious interrupts:
1. Disable incoming interrupts in master only mode.
2. Clear end of busy (EOB) after every interrupt.
3. Return correct status during interrupt.

drivers/i2c/busses/i2c-npcm7xx.c +7 -0

+	 */
+	if (ret == IRQ_NONE) {
+		npcm_i2c_eob_int(bus, false);
+		npcm_i2c_clear_master_status(bus);
+	}
+
+	return IRQ_HANDLED;

Quoted 4 lines · Tyrone Ting

Quoted 2 lines

From: Tali Perry <[email protected]>

Quoted 8 lines

Quoted 4 lines

In order to better handle spurious interrupts:
1. Disable incoming interrupts in master only mode.
2. Clear end of busy (EOB) after every interrupt.
3. Return correct status during interrupt.

This is bad commit message, it doesn't explain "why" you are doing these.

...

Quoted 15 lines

drivers/i2c/busses/i2c-npcm7xx.c static irqreturn_t npcm_i2c_int_slave_handler(struct npcm_i2c *bus) · line 1242

Quoted 4 lines

drivers/i2c/busses/i2c-npcm7xx.c +3 -0

+     /*
+      * if irq is not one of the above, make sure EOB is disabled and all
+      * status bits are cleared.

This does not explain why you hide the spurious interrupt.

Quoted 7 lines

drivers/i2c/busses/i2c-npcm7xx.c +7 -0

+      */
+     if (ret == IRQ_NONE) {
+             npcm_i2c_eob_int(bus, false);
+             npcm_i2c_clear_master_status(bus);
+     }
+
+     return IRQ_HANDLED;

Quoted 5 lines

--
With Best Regards,
Andy Shevchenko

Quoted 11 lines · Andy Shevchenko

Quoted 10 lines · Tyrone Ting

Quoted 6 lines

From: Tali Perry <[email protected]>

In order to better handle spurious interrupts:
1. Disable incoming interrupts in master only mode.
2. Clear end of busy (EOB) after every interrupt.
3. Return correct status during interrupt.

This is bad commit message, it doesn't explain "why" you are doing these.

BMC users connect a huge tree of i2c devices and muxes. This tree suffers from spikes, noise and double clocks. All these may cause spurious interrupts to the BMC.

If the driver gets an IRQ which was not expected and was not handled by the IRQ handler,
there is nothing left to do but to clear the interrupt and move on.

If the transaction failed, driver has a recovery function. After that, user may retry to send the message.

Indeed the commit message doesn't explain all this. We will fix and add to the next patchset.

Quoted 13 lines

Quoted 13 lines

drivers/i2c/busses/i2c-npcm7xx.c static irqreturn_t npcm_i2c_int_slave_handler(struct npcm_i2c *bus) · line 1242

Quoted 3 lines

drivers/i2c/busses/i2c-npcm7xx.c +3 -0

+     /*
+      * if irq is not one of the above, make sure EOB is disabled and all
+      * status bits are cleared.

This does not explain why you hide the spurious interrupt.

Quoted 7 lines

drivers/i2c/busses/i2c-npcm7xx.c +7 -0

+      */
+     if (ret == IRQ_NONE) {
+             npcm_i2c_eob_int(bus, false);
+             npcm_i2c_clear_master_status(bus);
+     }
+
+     return IRQ_HANDLED;

Quoted 11 lines · Tali Perry

Quoted 11 lines · Andy Shevchenko

Quoted 10 lines · Tyrone Ting

Quoted 6 lines

From: Tali Perry <[email protected]>

In order to better handle spurious interrupts:
1. Disable incoming interrupts in master only mode.
2. Clear end of busy (EOB) after every interrupt.
3. Return correct status during interrupt.

This is bad commit message, it doesn't explain "why" you are doing these.

...

Quoted 7 lines

BMC users connect a huge tree of i2c devices and muxes. This tree suffers from spikes, noise and double clocks. All these may cause spurious interrupts to the BMC.

If the driver gets an IRQ which was not expected and was not handled by the IRQ handler,
there is nothing left to do but to clear the interrupt and move on.

Yes, the problem is what "move on" means in your case.
If you get a spurious interrupts there are possibilities what's wrong:
1) HW bug(s)
2) FW bug(s)
3) Missed IRQ mask in the driver
4) Improper IRQ mask in the driver

The below approach seems incorrect to me.

Quoted 19 lines

If the transaction failed, driver has a recovery function. After that, user may retry to send the message.

Indeed the commit message doesn't explain all this. We will fix and add to the next patchset.

Quoted 13 lines

Quoted 13 lines

drivers/i2c/busses/i2c-npcm7xx.c static irqreturn_t npcm_i2c_int_slave_handler(struct npcm_i2c *bus) · line 1242

Quoted 3 lines

drivers/i2c/busses/i2c-npcm7xx.c +3 -0

+     /*
+      * if irq is not one of the above, make sure EOB is disabled and all
+      * status bits are cleared.

This does not explain why you hide the spurious interrupt.

Quoted 7 lines

drivers/i2c/busses/i2c-npcm7xx.c +7 -0

+      */
+     if (ret == IRQ_NONE) {
+             npcm_i2c_eob_int(bus, false);
+             npcm_i2c_clear_master_status(bus);
+     }
+
+     return IRQ_HANDLED;

--
With Best Regards,
Andy Shevchenko

Quoted 10 lines · Andy Shevchenko

Quoted 9 lines · Tali Perry

Quoted 9 lines · Andy Shevchenko

Quoted 8 lines · Tyrone Ting

Quoted 6 lines

From: Tali Perry <[email protected]>

In order to better handle spurious interrupts:
1. Disable incoming interrupts in master only mode.
2. Clear end of busy (EOB) after every interrupt.
3. Return correct status during interrupt.

Quoted 7 lines

Quoted 1 line

Quoted 1 line

Quoted 1 line

This is bad commit message, it doesn't explain "why" you are doing these.

...

Quoted 3 lines

BMC users connect a huge tree of i2c devices and muxes. This tree suffers from spikes, noise and double clocks. All these may cause spurious interrupts to the BMC.

Quoted 4 lines

Quoted 3 lines

If the driver gets an IRQ which was not expected and was not handled by the IRQ handler,
there is nothing left to do but to clear the interrupt and move on.

Quoted 8 lines

Yes, the problem is what "move on" means in your case.
If you get a spurious interrupts there are possibilities what's wrong:
1) HW bug(s)
2) FW bug(s)
3) Missed IRQ mask in the driver
4) Improper IRQ mask in the driver

The below approach seems incorrect to me.

Andy, What about this explanation:
On rare cases the i2c gets a spurious interrupt which means that we enter an interrupt but in
the interrupt handler we don't find any status bit that points to the reason we got this interrupt.
This may be a rare case of HW issue that is still under investigation. In order to overcome this we are doing the following:
1. Disable incoming interrupts in master mode only when slave mode is not enabled.
2. Clear end of busy (EOB) after every interrupt.
3. Clear other status bits (just in case since we found them cleared)
4. Return correct status during the interrupt that will finish the transaction. On next xmit transaction if the bus is still busy the master will issue a recovery process before issuing the new transaction.

Quoted 13 lines

Quoted 13 lines

If the transaction failed, driver has a recovery function. After that, user may retry to send the message.

Indeed the commit message doesn't explain all this. We will fix and add to the next patchset.

Quoted 7 lines

Quoted 7 lines

drivers/i2c/busses/i2c-npcm7xx.c static irqreturn_t npcm_i2c_int_slave_handler(struct npcm_i2c *bus) · line 1242

Quoted 3 lines

drivers/i2c/busses/i2c-npcm7xx.c +3 -0

+     /*
+      * if irq is not one of the above, make sure EOB is disabled and all
+      * status bits are cleared.

This does not explain why you hide the spurious interrupt.

Quoted 1 line

+ */

Quoted 14 lines · Avi Fishman

Quoted 12 lines · Andy Shevchenko

Quoted 10 lines · Tali Perry

Quoted 10 lines · Andy Shevchenko

Quoted 9 lines · Tyrone Ting

Quoted 6 lines

From: Tali Perry <[email protected]>

In order to better handle spurious interrupts:
1. Disable incoming interrupts in master only mode.
2. Clear end of busy (EOB) after every interrupt.
3. Return correct status during interrupt.

This is bad commit message, it doesn't explain "why" you are doing these.

Quoted 5 lines

Quoted 5 lines

...

Quoted 3 lines

BMC users connect a huge tree of i2c devices and muxes. This tree suffers from spikes, noise and double clocks. All these may cause spurious interrupts to the BMC.

(1)

Quoted 19 lines

Quoted 4 lines

Quoted 3 lines

If the driver gets an IRQ which was not expected and was not handled by the IRQ handler,
there is nothing left to do but to clear the interrupt and move on.

Quoted 8 lines

Yes, the problem is what "move on" means in your case.
If you get a spurious interrupts there are possibilities what's wrong:
1) HW bug(s)
2) FW bug(s)
3) Missed IRQ mask in the driver
4) Improper IRQ mask in the driver

The below approach seems incorrect to me.

Andy, What about this explanation:
On rare cases the i2c gets a spurious interrupt which means that we enter an interrupt but in
the interrupt handler we don't find any status bit that points to the reason we got this interrupt.
This may be a rare case of HW issue that is still under investigation

Quoted 8 lines

In order to overcome this we are doing the following:
1. Disable incoming interrupts in master mode only when slave mode is not enabled.
2. Clear end of busy (EOB) after every interrupt.
3. Clear other status bits (just in case since we found them cleared)
4. Return correct status during the interrupt that will finish the transaction. On next xmit transaction if the bus is still busy the master will issue a recovery process before issuing the new transaction.

This sounds better, thanks.

One thing to clarify, the (1) states that the HW "issue" is known and becomes a PCB level one, i.e. noisy environment that has not been properly shielded. So, if it is known, please put the reason in the commit message.

Also would be good to see numbers of "rare". Is it 0.1%?

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

If the transaction failed, driver has a recovery function. After that, user may retry to send the message.

Indeed the commit message doesn't explain all this. We will fix and add to the next patchset.

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

Quoted 7 lines

drivers/i2c/busses/i2c-npcm7xx.c static irqreturn_t npcm_i2c_int_slave_handler(struct npcm_i2c *bus) · line 1242

Quoted 3 lines

drivers/i2c/busses/i2c-npcm7xx.c +3 -0

+     /*
+      * if irq is not one of the above, make sure EOB is disabled and all
+      * status bits are cleared.

This does not explain why you hide the spurious interrupt.

Quoted 1 line

+ */

--
With Best Regards,
Andy Shevchenko

From: Tyrone Ting <[email protected]>

Add NPCM8XX I2C support.
The NPCM8XX uses a similar i2c module as NPCM7XX. The internal HW FIFO is larger in NPCM8XX.

Signed-off-by Tyrone Ting <[email protected]>

Signed-off-by Tali Perry <[email protected]>

+};

Quoted 44 lines · Tyrone Ting

Quoted 8 lines

From: Tyrone Ting <[email protected]>

Add NPCM8XX I2C support.
The NPCM8XX uses a similar i2c module as NPCM7XX. The internal HW FIFO is larger in NPCM8XX.

Signed-off-by Tyrone Ting <[email protected]>

Signed-off-by Tali Perry <[email protected]>

Wrong SoB chain.

...

drivers/i2c/busses/i2c-npcm7xx.c static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = { · line 281

Quoted 6 lines

drivers/i2c/busses/i2c-npcm7xx.c +6 -0

+static const struct npcm_i2c_data npxm7xx_i2c_data = {
+     .fifo_size = 16,
+     .segctl_init_val = 0x0333F000,
+     .txf_sts_tx_bytes = GENMASK(4, 0),
+     .rxf_sts_rx_bytes = GENMASK(4, 0),
+     .rxf_ctl_last_pec = BIT(5)

+ Comma.

drivers/i2c/busses/i2c-npcm7xx.c static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = { · line 289

Quoted 8 lines

drivers/i2c/busses/i2c-npcm7xx.c +8 -0

+};
+
+static const struct npcm_i2c_data npxm8xx_i2c_data = {
+     .fifo_size = 32,
+     .segctl_init_val = 0x9333F000,
+     .txf_sts_tx_bytes = GENMASK(5, 0),
+     .rxf_sts_rx_bytes = GENMASK(5, 0),
+     .rxf_ctl_last_pec = BIT(7)

Ditto.

Quoted 1 line

+};

...

Quoted 4 lines

drivers/i2c/busses/i2c-npcm7xx.c +2 -2

-     left_in_fifo = FIELD_GET(NPCM_I2CTXF_STS_TX_BYTES,
-                              ioread8(bus->reg + NPCM_I2CTXF_STS));
+     left_in_fifo = (bus->data->txf_sts_tx_bytes &
+                     ioread8(bus->reg + NPCM_I2CTXF_STS));

Quoted 5 lines

Besides too many parentheses, this is an interesting change. So, in different versions of IP the field is on different bits? Perhaps it means that you need something like internal ops structure for all these, where you will have been using the statically defined masks?

Quoted 22 lines

...

drivers/i2c/busses/i2c-npcm7xx.c static int npcm_i2c_probe_bus(struct platform_device *pdev) · line 2318

Quoted 6 lines

drivers/i2c/busses/i2c-npcm7xx.c +6 -0

+     match = of_match_device(npcm_i2c_bus_of_table, dev);
+     if (!match) {
+             dev_err(dev, "OF data missing\n");
+             return -EINVAL;
+     }
+     bus->data = match->data;

This is NIH of_device_get_match_data().

...

drivers/i2c/busses/i2c-npcm7xx.c static int npcm_i2c_remove_bus(struct platform_device *pdev) · line 2363

Quoted 6 lines

drivers/i2c/busses/i2c-npcm7xx.c +0 -6

-static const struct of_device_id npcm_i2c_bus_of_table[] = {
-     { .compatible = "nuvoton,npcm750-i2c", },
-     {}
-};
-MODULE_DEVICE_TABLE(of, npcm_i2c_bus_of_table);
-

Redundant change, leave this as is.

Quoted 5 lines

--
With Best Regards,
Andy Shevchenko

Quoted 2 lines · Andy Shevchenko

Quoted 1 line · Tyrone Ting

Quoted 10 lines

Quoted 10 lines

drivers/i2c/busses/i2c-npcm7xx.c static void npcm_i2c_slave_wr_buf_sync(struct npcm_i2c *bus) · line 1023

Quoted 4 lines

drivers/i2c/busses/i2c-npcm7xx.c +2 -2

-     left_in_fifo = FIELD_GET(NPCM_I2CTXF_STS_TX_BYTES,
-                              ioread8(bus->reg + NPCM_I2CTXF_STS));
+     left_in_fifo = (bus->data->txf_sts_tx_bytes &
+                     ioread8(bus->reg + NPCM_I2CTXF_STS));

Besides too many parentheses, this is an interesting change. So, in different versions of IP the field is on different bits? Perhaps it means that you need something like internal ops structure for all these, where you will have been using the statically defined masks?

Those are two very similar modules. The first generation had a 16 bytes HW FIFO and the second generation has 32 bytes.
In V1 of this patchset the masks were defined under
CONFIG but we were asked to change the approach:

the entire discussion can be found here:

https://www.spinics.net/lists/linux-i2c/msg55566.html

Did we understand the request change right?

Quoted 1 line · Tali Perry

Quoted 1 line

On Thu, Mar 3, 2022 at 12:45 PM Andy Shevchenko <

Quoted 1 line · [email protected]> wrote:

Quoted 1 line

Quoted 1 line · Tyrone Ting

...

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Review

Quoted 2 lines

(quoted diff) +0 -2

-     left_in_fifo = FIELD_GET(NPCM_I2CTXF_STS_TX_BYTES,
-                              ioread8(bus->reg +

NPCM_I2CTXF_STS));

Quoted 4 lines

Quoted 4 lines

Quoted 4 lines

drivers/i2c/busses/i2c-npcm7xx.c static void npcm_i2c_slave_wr_buf_sync(struct npcm_i2c *bus) · line 1023

Quoted 2 lines

drivers/i2c/busses/i2c-npcm7xx.c +2 -0

+     left_in_fifo = (bus->data->txf_sts_tx_bytes &
+                     ioread8(bus->reg + NPCM_I2CTXF_STS));

Besides too many parentheses, this is an interesting change. So, in

different

Quoted 1 line

Quoted 1 line

Quoted 1 line

versions of IP the field is on different bits? Perhaps it means that

you need

Quoted 1 line

Quoted 1 line

Quoted 1 line

something like internal ops structure for all these, where you will

have been

Quoted 4 lines

Quoted 2 lines

Quoted 2 lines

using the statically defined masks?

Those are two very similar modules. The first generation had a 16 bytes

HW FIFO

Quoted 9 lines

and the second generation has 32 bytes.
In V1 of this patchset the masks were defined under CONFIG but we were asked to change the approach:

the entire discussion can be found here:

https://www.spinics.net/lists/linux-i2c/msg55566.html

Did we understand the request change right?

Not really. If you have not simply "one (MSB) bit more" for FIFO size, then I proposed to create a specific operations structure and use callbacks (see drivers/dma/dw/ case for iDMA 32-bit vs. DesignWare).

But hold on and read set of questions below.

Previously it was a fixed field with the NPCM_I2CTXF_STS_TX_BYTES mask applied,
right? From above I have got that FIFO is growing twice. Is it correct?

Does the LSB stay at the same offset? What is the meaning of the MSB in 32 byte
case? If it's reserved then why not to always use 32 byte approach?

--
With Best Regards,
Andy Shevchenko

Quoted 3 lines · Andy Shevchenko

Quoted 1 line · Tali Perry

Quoted 1 line

On Thu, Mar 3, 2022 at 12:45 PM Andy Shevchenko <

[email protected]> wrote:

Quoted 5 lines

But hold on and read set of questions below.

Previously it was a fixed field with the NPCM_I2CTXF_STS_TX_BYTES mask applied,
right? From above I have got that FIFO is growing twice. Is it correct?

What do you mean by growing twice? TX and RX?

Quoted 3 lines

Does the LSB stay at the same offset? What is the meaning of the MSB in 32 byte
case? If it's reserved then why not to always use 32 byte approach?

Yes, the LSB stays in the same place, and bit 5 is reserved in the NPCM7XX SoC.
Unfortunately, the I2C test failed when we tried to use the 32 bytes approach at NPCM7XX Soc, this is why we added NPCM_I2CTXF_STS_TX_BYTES and NPCM_I2C_STSRXF_RX_BYTES to the data structure.

The device tree data structure pass data for each specific device, so I don't understand why not use device tree data for supporting the I2C specific device? this is not the case here?

Quoted 5 lines · Tomer Maimon

Quoted 3 lines · Andy Shevchenko

Quoted 1 line · Tali Perry

Quoted 1 line

On Thu, Mar 3, 2022 at 12:45 PM Andy Shevchenko <

[email protected]> wrote:

...

Quoted 2 lines

Quoted 2 lines

But hold on and read set of questions below.

Quoted 5 lines

Quoted 3 lines

Previously it was a fixed field with the NPCM_I2CTXF_STS_TX_BYTES mask applied,
right? From above I have got that FIFO is growing twice. Is it correct?

What do you mean by growing twice? TX and RX?

I meant from 16 bytes to 32 bytes.

Quoted 2 lines

Quoted 2 lines

Does the LSB stay at the same offset? What is the meaning of the MSB

in 32

Quoted 4 lines

Quoted 2 lines

byte
case? If it's reserved then why not to always use 32 byte approach?

Yes, the LSB stays in the same place, and bit 5 is reserved in the

NPCM7XX

Quoted 3 lines

SoC.
Unfortunately, the I2C test failed when we tried to use the 32 bytes approach at NPCM7XX Soc, this is why we added NPCM_I2CTXF_STS_TX_BYTES

and

Quoted 5 lines

NPCM_I2C_STSRXF_RX_BYTES to the data structure.

The device tree data structure pass data for each specific device, so I don't understand why not use device tree data for supporting the I2C specific device? this is not the case here?

Basically we use compatible strings for that, but in any case if something can be autodetected from hardware, it's better to use autodetection.

--
With Best Regards,
Andy Shevchenko

We will send a new version soon, highly appreciate your help!

From: Tyrone Ting <[email protected]>

This patchset includes the following fixes:

- Add dt-bindings description for NPCM845.
- Bug fix for timeout calculation.
- Better handling of spurious interrupts.
- Fix for event type in slave mode.
- Removal of own slave addresses [2:10].
- Support for next gen BMC (NPCM845).

The NPCM I2C driver is tested on NPCM750 and NPCM845 evaluation boards.

Quoted 11 lines · Tyrone Ting

From: Tyrone Ting <[email protected]>

This patchset includes the following fixes:

- Add dt-bindings description for NPCM845.
- Bug fix for timeout calculation.
- Better handling of spurious interrupts.
- Fix for event type in slave mode.
- Removal of own slave addresses [2:10].
- Support for next gen BMC (NPCM845).

Quoted 1 line

The NPCM I2C driver is tested on NPCM750 and NPCM845 evaluation boards.

Overall my impression that the code was never tested for this driver and somehow appears in the upstream and hence this series.

Anyway, I'm going to review the changes here.

--

With Best Regards,
Andy Shevchenko