[PATCH v3 00/11] i2c: npcm: Bug fixes timeout, spurious interrupts

[PATCH v3 00/11] i2c: npcm: Bug fixes timeout, spurious interrupts

[Tyrone Ting]

From: Tyrone Ting <kfting@nuvoton.com>

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.

Addressed comments from:
 - Krzysztof Kozlowski : https://lkml.org/lkml/2022/2/21/129
 - Wolfram Sang : https://lore.kernel.org/lkml/Yh536s%2F7bm6Xt6o3@ninjato/
 - Rob Herring : https://lkml.org/lkml/2022/2/20/425
 - Rob Herring : https://lkml.org/lkml/2022/2/22/945
 - Krzysztof Kozlowski : https://www.spinics.net/lists/linux-i2c/
                         msg55903.html
 - Jonathan Neuschäfer : https://lkml.org/lkml/2022/2/21/500
 - Krzysztof Kozlowski : https://lkml.org/lkml/2022/2/20/49

Changes since version 2:
 - Keep old code as fallback, if getting nuvoton,sys-mgr property fails.
 - Fix the error reported by running 'make DT_CHECKER_FLAGS=-m 
   dt_binding_check'.
 - Make nuvoton,sys-mgr required for nuvoton,npcm845-i2c.
 - Correct the patch's subject about changing the way of getting GCR
   regmap and add the description about keeping old code as fallback
   if getting nuvoton,sys-mgr property fails.
 - Correct the patch title and description about removing the unused 
   variable clk_regmap.
 - Use the data field directly instead of the macros since macros are
   not constants anymore in this patch.
 
Changes since version 1:
 - Add nuvoton,sys-mgr property in NPCM devicetree.
 - Describe the commit message in imperative mood.
 - Modify the description in i2c binding document to cover NPCM series.
 - Add new property in i2c binding document.
 - Create a new patch for client address calculation.
 - Create a new patch for updating gcr property name.
 - Create a new patch for removing unused clock node.
 - Explain EOB in the commit description.
 - Create a new patch for correcting NPCM register access width.
 - Remove some comment since the corresponding logic no longer exists.
 - Remove fixes tag while the patch adds an additional feature.
 - Use devicetree data field to support NPCM845.

Tali Perry (7):
  i2c: npcm: Fix client address calculation
  i2c: npcm: Change the way of getting GCR regmap
  i2c: npcm: Remove unused variable clk_regmap
  i2c: npcm: Fix timeout calculation
  i2c: npcm: Add tx complete counter
  i2c: npcm: Handle spurious interrupts
  i2c: npcm: Remove own slave addresses 2:10

Tyrone Ting (4):
  arm: dts: add new property for NPCM i2c module
  dt-bindings: i2c: npcm: support NPCM845
  i2c: npcm: Correct register access width
  i2c: npcm: Support NPCM845

 .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml     |  26 +-
 arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi |  16 +
 drivers/i2c/busses/Kconfig                    |   8 +-
 drivers/i2c/busses/Makefile                   |   2 +-
 drivers/i2c/busses/i2c-npcm7xx.c              | 291 +++++++++++-------
 5 files changed, 221 insertions(+), 122 deletions(-)

-- 
2.17.1

[PATCH v3 01/11] arm: dts: add new property for NPCM i2c module

[Tyrone Ting]

From: Tyrone Ting <kfting@nuvoton.com>

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

Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
Signed-off-by: Tali Perry <tali.perry1@gmail.com>
---
 arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi b/arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi
index 3696980a3da1..0fee5fc67e02 100644
--- a/arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi
+++ b/arch/arm/boot/dts/nuvoton-common-npcm7xx.dtsi
@@ -371,6 +371,7 @@
 				interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb0_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 
@@ -383,6 +384,7 @@
 				interrupts = <GIC_SPI 65 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb1_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 
@@ -395,6 +397,7 @@
 				interrupts = <GIC_SPI 66 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb2_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 
@@ -407,6 +410,7 @@
 				interrupts = <GIC_SPI 67 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb3_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 
@@ -419,6 +423,7 @@
 				interrupts = <GIC_SPI 68 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb4_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 
@@ -431,6 +436,7 @@
 				interrupts = <GIC_SPI 69 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb5_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 
@@ -443,6 +449,7 @@
 				interrupts = <GIC_SPI 70 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb6_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 
@@ -455,6 +462,7 @@
 				interrupts = <GIC_SPI 71 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb7_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 
@@ -467,6 +475,7 @@
 				interrupts = <GIC_SPI 72 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb8_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 
@@ -479,6 +488,7 @@
 				interrupts = <GIC_SPI 73 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb9_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 
@@ -491,6 +501,7 @@
 				interrupts = <GIC_SPI 74 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb10_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 
@@ -503,6 +514,7 @@
 				interrupts = <GIC_SPI 75 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb11_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 
@@ -515,6 +527,7 @@
 				interrupts = <GIC_SPI 76 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb12_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 
@@ -527,6 +540,7 @@
 				interrupts = <GIC_SPI 77 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb13_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 
@@ -539,6 +553,7 @@
 				interrupts = <GIC_SPI 78 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb14_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 
@@ -551,6 +566,7 @@
 				interrupts = <GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>;
 				pinctrl-names = "default";
 				pinctrl-0 = <&smb15_pins>;
+				nuvoton,sys-mgr = <&gcr>;
 				status = "disabled";
 			};
 		};
-- 
2.17.1

Re: [PATCH v3 01/11] arm: dts: add new property for NPCM i2c module

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 614 bytes --]

On Thu, Mar 03, 2022 at 04:31:31PM +0800, Tyrone Ting wrote:
> From: Tyrone Ting <kfting@nuvoton.com>
> 
> Add nuvoton,sys-mgr property for controlling NPCM gcr register.
> 
> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> Signed-off-by: Tali Perry <tali.perry1@gmail.com>

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?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 01/11] arm: dts: add new property for NPCM i2c module

[Tyrone Ting]

Hi Wolfram:

Thank you for your reminder and suggestion.

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.

Wolfram Sang <wsa@kernel.org> 於 2022年3月19日 週六 上午4:29寫道：
>
> On Thu, Mar 03, 2022 at 04:31:31PM +0800, Tyrone Ting wrote:
> > From: Tyrone Ting <kfting@nuvoton.com>
> >
> > Add nuvoton,sys-mgr property for controlling NPCM gcr register.
> >
> > Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> > Signed-off-by: Tali Perry <tali.perry1@gmail.com>
>
> 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?
>

Regards,
Tyrone

Re: [PATCH v3 01/11] arm: dts: add new property for NPCM i2c module

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 370 bytes --]

Hi Tyrone,

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

Take your time, I am not in a hurry. Just wanted to outline the best
process so it will be easier to apply the new version.

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

Great.

> I really appreciate your comments.

Thank you and happy hacking,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 02/11] dt-bindings: i2c: npcm: support NPCM845

[Tyrone Ting]

From: Tyrone Ting <kfting@nuvoton.com>

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

Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
Signed-off-by: Tali Perry <tali.perry1@gmail.com>
---
 .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml     | 26 +++++++++++++++----
 1 file changed, 21 insertions(+), 5 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml b/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
index 128444942aec..37976ddcf406 100644
--- a/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
@@ -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 <tali.perry1@gmail.com>
 
 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
@@ -44,6 +49,16 @@ required:
 
 allOf:
   - $ref: /schemas/i2c/i2c-controller.yaml#
+  - if:
+      properties:
+        compatible:
+          contains:
+            const:
+              - nuvoton,npcm845-i2c
+
+    then:
+      required:
+        - nuvoton,sys-mgr
 
 unevaluatedProperties: false
 
@@ -57,6 +72,7 @@ examples:
         clock-frequency = <100000>;
         interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
         compatible = "nuvoton,npcm750-i2c";
+        nuvoton,sys-mgr = <&gcr>;
     };
 
 ...
-- 
2.17.1

Re: [PATCH v3 02/11] dt-bindings: i2c: npcm: support NPCM845

[Krzysztof Kozlowski]

On 03/03/2022 09:31, Tyrone Ting wrote:
> From: Tyrone Ting <kfting@nuvoton.com>
> 
> Add compatible and nuvoton,sys-mgr description for NPCM i2c module.
> 
> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> Signed-off-by: Tali Perry <tali.perry1@gmail.com>
> ---
>  .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml     | 26 +++++++++++++++----
>  1 file changed, 21 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml b/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
> index 128444942aec..37976ddcf406 100644
> --- a/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
> @@ -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 <tali.perry1@gmail.com>
>  
>  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
> @@ -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

Re: [PATCH v3 02/11] dt-bindings: i2c: npcm: support NPCM845

[Tyrone Ting]

Hi Krzysztof:

Thank you for your comment and it'll be addressed.

Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> 於 2022年3月3日 週四 下午6:38寫道：
>
> On 03/03/2022 09:31, Tyrone Ting wrote:
> > From: Tyrone Ting <kfting@nuvoton.com>
> >
> > Add compatible and nuvoton,sys-mgr description for NPCM i2c module.
> >
> > Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> > Signed-off-by: Tali Perry <tali.perry1@gmail.com>
> > ---
> >  .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml     | 26 +++++++++++++++----
> >  1 file changed, 21 insertions(+), 5 deletions(-)
> >
> > diff --git a/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml b/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
> > index 128444942aec..37976ddcf406 100644
> > --- a/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
> > +++ b/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
> > @@ -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 <tali.perry1@gmail.com>
> >
> >  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
> > @@ -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

Best regards,
Tyrone

[PATCH v3 03/11] i2c: npcm: Fix client address calculation

[Tyrone Ting]

From: Tali Perry <tali.perry1@gmail.com>

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

Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by: Tali Perry <tali.perry1@gmail.com>
Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 2ad166355ec9..4c225e1a058f 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -2131,7 +2131,7 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	}
 
 	npcm_i2c_init_params(bus);
-	bus->dest_addr = slave_addr;
+	bus->dest_addr = slave_addr << 1;
 	bus->msgs = msgs;
 	bus->msgs_num = num;
 	bus->cmd_err = 0;
-- 
2.17.1

Re: [PATCH v3 03/11] i2c: npcm: Fix client address calculation

[Andy Shevchenko]

On Thu, Mar 03, 2022 at 04:31:33PM +0800, Tyrone Ting wrote:
> From: Tali Perry <tali.perry1@gmail.com>
> 
> Fix i2c client address by left-shifting 1 bit before
> applying it to the data register.

...

> -	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

Re: [PATCH v3 03/11] i2c: npcm: Fix client address calculation

[Tyrone Ting]

Hi Andy:

Thank you for your comment and it'll be addressed.

Andy Shevchenko <andriy.shevchenko@linux.intel.com> 於 2022年3月3日 週四 下午6:31寫道：
>
> On Thu, Mar 03, 2022 at 04:31:33PM +0800, Tyrone Ting wrote:
> > From: Tali Perry <tali.perry1@gmail.com>
> >
> > Fix i2c client address by left-shifting 1 bit before
> > applying it to the data register.
>
> ...
>
> > -     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
>
>

Best regards,
Tyrone

[PATCH v3 04/11] i2c: npcm: Change the way of getting GCR regmap

[Tyrone Ting]

From: Tali Perry <tali.perry1@gmail.com>

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 <tali.perry1@gmail.com>
Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 4c225e1a058f..fa12212b2428 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -2236,6 +2236,7 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
 	static struct regmap *clk_regmap;
 	int irq;
 	int ret;
+	struct device_node *np = pdev->dev.of_node;
 
 	bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
 	if (!bus)
@@ -2250,7 +2251,10 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
 		return PTR_ERR(i2c_clk);
 	bus->apb_clk = clk_get_rate(i2c_clk);
 
-	gcr_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-gcr");
+	gcr_regmap = syscon_regmap_lookup_by_phandle(np, "nuvoton,sys-mgr");
+	if (IS_ERR(gcr_regmap))
+		gcr_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-gcr");
+
 	if (IS_ERR(gcr_regmap))
 		return PTR_ERR(gcr_regmap);
 	regmap_write(gcr_regmap, NPCM_I2CSEGCTL, NPCM_I2CSEGCTL_INIT_VAL);
-- 
2.17.1

Re: [PATCH v3 04/11] i2c: npcm: Change the way of getting GCR regmap

[Krzysztof Kozlowski]

On 03/03/2022 09:31, Tyrone Ting wrote:
> From: Tali Perry <tali.perry1@gmail.com>
> 
> 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 <tali.perry1@gmail.com>
> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> ---
>  drivers/i2c/busses/i2c-npcm7xx.c | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 


Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>


Best regards,
Krzysztof

Re: [PATCH v3 04/11] i2c: npcm: Change the way of getting GCR regmap

[Tyrone Ting]

Hi Krzysztof:

Thank you for your review.

Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com> 於 2022年3月3日 週四 下午6:38寫道：
>
> On 03/03/2022 09:31, Tyrone Ting wrote:
> > From: Tali Perry <tali.perry1@gmail.com>
> >
> > 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 <tali.perry1@gmail.com>
> > Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> > ---
> >  drivers/i2c/busses/i2c-npcm7xx.c | 6 +++++-
> >  1 file changed, 5 insertions(+), 1 deletion(-)
> >
>
>
> Acked-by: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
>
>
> Best regards,
> Krzysztof

Best regards,
Tyrone

[PATCH v3 05/11] i2c: npcm: Remove unused variable clk_regmap

[Tyrone Ting]

From: Tali Perry <tali.perry1@gmail.com>

Remove unused variable clk_regmap.

Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by: Tali Perry <tali.perry1@gmail.com>
Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index fa12212b2428..c41de3afcf38 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -2233,7 +2233,6 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
 	struct i2c_adapter *adap;
 	struct clk *i2c_clk;
 	static struct regmap *gcr_regmap;
-	static struct regmap *clk_regmap;
 	int irq;
 	int ret;
 	struct device_node *np = pdev->dev.of_node;
@@ -2259,10 +2258,6 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
 		return PTR_ERR(gcr_regmap);
 	regmap_write(gcr_regmap, NPCM_I2CSEGCTL, NPCM_I2CSEGCTL_INIT_VAL);
 
-	clk_regmap = syscon_regmap_lookup_by_compatible("nuvoton,npcm750-clk");
-	if (IS_ERR(clk_regmap))
-		return PTR_ERR(clk_regmap);
-
 	bus->reg = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(bus->reg))
 		return PTR_ERR(bus->reg);
-- 
2.17.1

[PATCH v3 06/11] i2c: npcm: Fix timeout calculation

[Tyrone Ting]

From: Tali Perry <tali.perry1@gmail.com>

Use adap.timeout for timeout calculation instead of hard-coded
value of 35ms.

Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by: Tali Perry <tali.perry1@gmail.com>
Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
Reported-by: kernel test robot <lkp@intel.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index c41de3afcf38..3fd30c38b3bf 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -2047,7 +2047,7 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	u16 nwrite, nread;
 	u8 *write_data, *read_data;
 	u8 slave_addr;
-	int timeout;
+	unsigned long timeout;
 	int ret = 0;
 	bool read_block = false;
 	bool read_PEC = false;
@@ -2099,13 +2099,13 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	 * 9: bits per transaction (including the ack/nack)
 	 */
 	timeout_usec = (2 * 9 * USEC_PER_SEC / bus->bus_freq) * (2 + nread + nwrite);
-	timeout = max(msecs_to_jiffies(35), usecs_to_jiffies(timeout_usec));
+	timeout = max_t(unsigned long, bus->adap.timeout, usecs_to_jiffies(timeout_usec));
 	if (nwrite >= 32 * 1024 || nread >= 32 * 1024) {
 		dev_err(bus->dev, "i2c%d buffer too big\n", bus->num);
 		return -EINVAL;
 	}
 
-	time_left = jiffies + msecs_to_jiffies(DEFAULT_STALL_COUNT) + 1;
+	time_left = jiffies + timeout + 1;
 	do {
 		/*
 		 * we must clear slave address immediately when the bus is not
@@ -2268,7 +2268,7 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
 	adap = &bus->adap;
 	adap->owner = THIS_MODULE;
 	adap->retries = 3;
-	adap->timeout = HZ;
+	adap->timeout = msecs_to_jiffies(35);
 	adap->algo = &npcm_i2c_algo;
 	adap->quirks = &npcm_i2c_quirks;
 	adap->algo_data = bus;
-- 
2.17.1

[PATCH v3 07/11] i2c: npcm: Add tx complete counter

[Tyrone Ting]

From: Tali Perry <tali.perry1@gmail.com>

tx_complete counter is used to indicate successful transaction
count.
Similar counters for failed tx were previously added.

Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by: Tali Perry <tali.perry1@gmail.com>
Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 3fd30c38b3bf..237da0ef32ca 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -314,6 +314,7 @@ struct npcm_i2c {
 	u64 rec_fail_cnt;
 	u64 nack_cnt;
 	u64 timeout_cnt;
+	u64 tx_complete_cnt;
 };
 
 static inline void npcm_i2c_select_bank(struct npcm_i2c *bus,
@@ -684,6 +685,8 @@ static void npcm_i2c_callback(struct npcm_i2c *bus,
 	switch (op_status) {
 	case I2C_MASTER_DONE_IND:
 		bus->cmd_err = bus->msgs_num;
+		if (bus->tx_complete_cnt < ULLONG_MAX)
+			bus->tx_complete_cnt++;
 		fallthrough;
 	case I2C_BLOCK_BYTES_ERR_IND:
 		/* Master tx finished and all transmit bytes were sent */
@@ -2223,6 +2226,7 @@ static void npcm_i2c_init_debugfs(struct platform_device *pdev,
 	debugfs_create_u64("rec_succ_cnt", 0444, d, &bus->rec_succ_cnt);
 	debugfs_create_u64("rec_fail_cnt", 0444, d, &bus->rec_fail_cnt);
 	debugfs_create_u64("timeout_cnt", 0444, d, &bus->timeout_cnt);
+	debugfs_create_u64("tx_complete_cnt", 0444, d, &bus->tx_complete_cnt);
 
 	bus->debugfs = d;
 }
-- 
2.17.1

[PATCH v3 08/11] i2c: npcm: Correct register access width

[Tyrone Ting]

From: Tyrone Ting <kfting@nuvoton.com>

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 <kfting@nuvoton.com>
Signed-off-by: Tali Perry <tali.perry1@gmail.com>
Reviewed-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 237da0ef32ca..66532c680338 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -360,14 +360,14 @@ static int npcm_i2c_get_SCL(struct i2c_adapter *_adap)
 {
 	struct npcm_i2c *bus = container_of(_adap, struct npcm_i2c, adap);
 
-	return !!(I2CCTL3_SCL_LVL & ioread32(bus->reg + NPCM_I2CCTL3));
+	return !!(I2CCTL3_SCL_LVL & ioread8(bus->reg + NPCM_I2CCTL3));
 }
 
 static int npcm_i2c_get_SDA(struct i2c_adapter *_adap)
 {
 	struct npcm_i2c *bus = container_of(_adap, struct npcm_i2c, adap);
 
-	return !!(I2CCTL3_SDA_LVL & ioread32(bus->reg + NPCM_I2CCTL3));
+	return !!(I2CCTL3_SDA_LVL & ioread8(bus->reg + NPCM_I2CCTL3));
 }
 
 static inline u16 npcm_i2c_get_index(struct npcm_i2c *bus)
-- 
2.17.1

Re: [PATCH v3 08/11] i2c: npcm: Correct register access width

[Andy Shevchenko]

On Thu, Mar 03, 2022 at 04:31:38PM +0800, Tyrone Ting wrote:
> From: Tyrone Ting <kfting@nuvoton.com>
> 
> 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?

> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> Signed-off-by: Tali Perry <tali.perry1@gmail.com>

This is wrong SoB chain.

...

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

...

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 08/11] i2c: npcm: Correct register access width

[Tali Perry]

> On Thu, Mar 03, 2022 at 04:31:38PM +0800, Tyrone Ting wrote:
> > From: Tyrone Ting <kfting@nuvoton.com>
> >
> > 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.


> > Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> > Signed-off-by: Tali Perry <tali.perry1@gmail.com>
>
> This is wrong SoB chain.
>
> ...
>
> > -     return !!(I2CCTL3_SCL_LVL & ioread32(bus->reg + NPCM_I2CCTL3));
> > +     return !!(I2CCTL3_SCL_LVL & ioread8(bus->reg + NPCM_I2CCTL3));
>
> ...
>
> > -     return !!(I2CCTL3_SDA_LVL & ioread32(bus->reg + NPCM_I2CCTL3));
> > +     return !!(I2CCTL3_SDA_LVL & ioread8(bus->reg + NPCM_I2CCTL3));
>
> --
> With Best Regards,
> Andy Shevchenko

Thanks Andy,

BR,
Tali Perry

Re: [PATCH v3 08/11] i2c: npcm: Correct register access width

[Andy Shevchenko]

On Thu, Mar 03, 2022 at 02:54:27PM +0200, Tali Perry wrote:
> > On Thu, Mar 03, 2022 at 04:31:38PM +0800, Tyrone Ting wrote:
> > > From: Tyrone Ting <kfting@nuvoton.com>
> > >
> > > 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.

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 08/11] i2c: npcm: Correct register access width

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 1632 bytes --]

Hello,

On Thu, Mar 03, 2022 at 04:15:18PM +0200, Andy Shevchenko wrote:
> On Thu, Mar 03, 2022 at 02:54:27PM +0200, Tali Perry wrote:
> > > On Thu, Mar 03, 2022 at 04:31:38PM +0800, Tyrone Ting wrote:
> > > > From: Tyrone Ting <kfting@nuvoton.com>
> > > >
> > > > 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.
> 
> 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.

> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 08/11] i2c: npcm: Correct register access width

[Avi Fishman]

On Fri, Mar 4, 2022 at 10:42 PM Jonathan Neuschäfer
<j.neuschaefer@gmx.net> wrote:
>
> Hello,
>
> On Thu, Mar 03, 2022 at 04:15:18PM +0200, Andy Shevchenko wrote:
> > On Thu, Mar 03, 2022 at 02:54:27PM +0200, Tali Perry wrote:
> > > > On Thu, Mar 03, 2022 at 04:31:38PM +0800, Tyrone Ting wrote:
> > > > > From: Tyrone Ting <kfting@nuvoton.com>
> > > > >
> > > > > 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.
> >
> > 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.
>
> > 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).
>

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?

>
> Jonathan



-- 
Regards,
Avi

Re: [PATCH v3 08/11] i2c: npcm: Correct register access width

[Jonathan Neuschäfer]

[-- Attachment #1: Type: text/plain, Size: 2506 bytes --]

On Tue, Mar 22, 2022 at 07:18:34PM +0200, Avi Fishman wrote:
> On Fri, Mar 4, 2022 at 10:42 PM Jonathan Neuschäfer
> <j.neuschaefer@gmx.net> wrote:
> >
> > Hello,
> >
> > On Thu, Mar 03, 2022 at 04:15:18PM +0200, Andy Shevchenko wrote:
> > > On Thu, Mar 03, 2022 at 02:54:27PM +0200, Tali Perry wrote:
> > > > > On Thu, Mar 03, 2022 at 04:31:38PM +0800, Tyrone Ting wrote:
> > > > > > From: Tyrone Ting <kfting@nuvoton.com>
> > > > > >
> > > > > > 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.
> > >
> > > 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.
> >
> > > 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).
> >
> 
> 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?

To be honest, I think it's probably best to include all the necessary
explanations in the next version of this patch, i.e.:

 - That the register was always defined as 8-bit in the datasheets,
   and so the 32-bit access was always incorrect, but simply didn't
   cause a visible error
 - How the 32-bit access caused an error now, perhaps with a trimmed
   Oops log as Andy suggested


Jonathan

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH v3 09/11] i2c: npcm: Handle spurious interrupts

[Tyrone Ting]

From: Tali Perry <tali.perry1@gmail.com>

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.

Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by: Tali Perry <tali.perry1@gmail.com>
Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 92 ++++++++++++++++++++++----------
 1 file changed, 63 insertions(+), 29 deletions(-)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 66532c680338..73cef76127c9 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -564,6 +564,15 @@ static inline void npcm_i2c_nack(struct npcm_i2c *bus)
 	iowrite8(val, bus->reg + NPCM_I2CCTL1);
 }
 
+static inline void npcm_i2c_clear_master_status(struct npcm_i2c *bus)
+{
+	u8 val;
+
+	/* Clear NEGACK, STASTR and BER bits */
+	val = NPCM_I2CST_BER | NPCM_I2CST_NEGACK | NPCM_I2CST_STASTR;
+	iowrite8(val, bus->reg + NPCM_I2CST);
+}
+
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 static void npcm_i2c_slave_int_enable(struct npcm_i2c *bus, bool enable)
 {
@@ -643,8 +652,8 @@ static void npcm_i2c_reset(struct npcm_i2c *bus)
 	iowrite8(NPCM_I2CCST_BB, bus->reg + NPCM_I2CCST);
 	iowrite8(0xFF, bus->reg + NPCM_I2CST);
 
-	/* Clear EOB bit */
-	iowrite8(NPCM_I2CCST3_EO_BUSY, bus->reg + NPCM_I2CCST3);
+	/* Clear and disable EOB */
+	npcm_i2c_eob_int(bus, false);
 
 	/* Clear all fifo bits: */
 	iowrite8(NPCM_I2CFIF_CTS_CLR_FIFO, bus->reg + NPCM_I2CFIF_CTS);
@@ -656,6 +665,9 @@ static void npcm_i2c_reset(struct npcm_i2c *bus)
 	}
 #endif
 
+	/* clear status bits for spurious interrupts */
+	npcm_i2c_clear_master_status(bus);
+
 	bus->state = I2C_IDLE;
 }
 
@@ -818,15 +830,6 @@ static void npcm_i2c_read_fifo(struct npcm_i2c *bus, u8 bytes_in_fifo)
 	}
 }
 
-static inline void npcm_i2c_clear_master_status(struct npcm_i2c *bus)
-{
-	u8 val;
-
-	/* Clear NEGACK, STASTR and BER bits */
-	val = NPCM_I2CST_BER | NPCM_I2CST_NEGACK | NPCM_I2CST_STASTR;
-	iowrite8(val, bus->reg + NPCM_I2CST);
-}
-
 static void npcm_i2c_master_abort(struct npcm_i2c *bus)
 {
 	/* Only current master is allowed to issue a stop condition */
@@ -1234,7 +1237,16 @@ static irqreturn_t npcm_i2c_int_slave_handler(struct npcm_i2c *bus)
 		ret = IRQ_HANDLED;
 	} /* SDAST */
 
-	return ret;
+	/*
+	 * if irq is not one of the above, make sure EOB is disabled and all
+	 * status bits are cleared.
+	 */
+	if (ret == IRQ_NONE) {
+		npcm_i2c_eob_int(bus, false);
+		npcm_i2c_clear_master_status(bus);
+	}
+
+	return IRQ_HANDLED;
 }
 
 static int npcm_i2c_reg_slave(struct i2c_client *client)
@@ -1470,6 +1482,9 @@ static void npcm_i2c_irq_handle_nack(struct npcm_i2c *bus)
 		npcm_i2c_eob_int(bus, false);
 		npcm_i2c_master_stop(bus);
 
+		/* Clear SDA Status bit (by reading dummy byte) */
+		npcm_i2c_rd_byte(bus);
+
 		/*
 		 * The bus is released from stall only after the SW clears
 		 * NEGACK bit. Then a Stop condition is sent.
@@ -1477,6 +1492,8 @@ static void npcm_i2c_irq_handle_nack(struct npcm_i2c *bus)
 		npcm_i2c_clear_master_status(bus);
 		readx_poll_timeout_atomic(ioread8, bus->reg + NPCM_I2CCST, val,
 					  !(val & NPCM_I2CCST_BUSY), 10, 200);
+		/* verify no status bits are still set after bus is released */
+		npcm_i2c_clear_master_status(bus);
 	}
 	bus->state = I2C_IDLE;
 
@@ -1675,10 +1692,10 @@ static int npcm_i2c_recovery_tgclk(struct i2c_adapter *_adap)
 	int              iter = 27;
 
 	if ((npcm_i2c_get_SDA(_adap) == 1) && (npcm_i2c_get_SCL(_adap) == 1)) {
-		dev_dbg(bus->dev, "bus%d recovery skipped, bus not stuck",
-			bus->num);
+		dev_dbg(bus->dev, "bus%d-0x%x recovery skipped, bus not stuck",
+			bus->num, bus->dest_addr);
 		npcm_i2c_reset(bus);
-		return status;
+		return 0;
 	}
 
 	npcm_i2c_int_enable(bus, false);
@@ -1912,6 +1929,7 @@ static int npcm_i2c_init_module(struct npcm_i2c *bus, enum i2c_mode mode,
 	    bus_freq_hz < I2C_FREQ_MIN_HZ || bus_freq_hz > I2C_FREQ_MAX_HZ)
 		return -EINVAL;
 
+	npcm_i2c_int_enable(bus, false);
 	npcm_i2c_disable(bus);
 
 	/* Configure FIFO mode : */
@@ -1940,10 +1958,18 @@ static int npcm_i2c_init_module(struct npcm_i2c *bus, enum i2c_mode mode,
 	val = (val | NPCM_I2CCTL1_NMINTE) & ~NPCM_I2CCTL1_RWS;
 	iowrite8(val, bus->reg + NPCM_I2CCTL1);
 
-	npcm_i2c_int_enable(bus, true);
-
 	npcm_i2c_reset(bus);
 
+	/* check HW is OK: SDA and SCL should be high at this point. */
+	if ((npcm_i2c_get_SDA(&bus->adap) == 0) ||
+	    (npcm_i2c_get_SCL(&bus->adap) == 0)) {
+		dev_err(bus->dev, "I2C%d init fail: lines are low", bus->num);
+		dev_err(bus->dev, "SDA=%d SCL=%d", npcm_i2c_get_SDA(&bus->adap),
+			npcm_i2c_get_SCL(&bus->adap));
+		return -ENXIO;
+	}
+
+	npcm_i2c_int_enable(bus, true);
 	return 0;
 }
 
@@ -1991,10 +2017,14 @@ static irqreturn_t npcm_i2c_bus_irq(int irq, void *dev_id)
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 	if (bus->slave) {
 		bus->master_or_slave = I2C_SLAVE;
-		return npcm_i2c_int_slave_handler(bus);
+		if (npcm_i2c_int_slave_handler(bus))
+			return IRQ_HANDLED;
 	}
 #endif
-	return IRQ_NONE;
+	/* clear status bits for spurious interrupts */
+	npcm_i2c_clear_master_status(bus);
+
+	return IRQ_HANDLED;
 }
 
 static bool npcm_i2c_master_start_xmit(struct npcm_i2c *bus,
@@ -2051,7 +2081,6 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	u8 *write_data, *read_data;
 	u8 slave_addr;
 	unsigned long timeout;
-	int ret = 0;
 	bool read_block = false;
 	bool read_PEC = false;
 	u8 bus_busy;
@@ -2141,12 +2170,12 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	bus->read_block_use = read_block;
 
 	reinit_completion(&bus->cmd_complete);
-	if (!npcm_i2c_master_start_xmit(bus, slave_addr, nwrite, nread,
-					write_data, read_data, read_PEC,
-					read_block))
-		ret = -EBUSY;
 
-	if (ret != -EBUSY) {
+	npcm_i2c_int_enable(bus, true);
+
+	if (npcm_i2c_master_start_xmit(bus, slave_addr, nwrite, nread,
+				       write_data, read_data, read_PEC,
+				       read_block)) {
 		time_left = wait_for_completion_timeout(&bus->cmd_complete,
 							timeout);
 
@@ -2160,26 +2189,31 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 			}
 		}
 	}
-	ret = bus->cmd_err;
 
 	/* if there was BER, check if need to recover the bus: */
 	if (bus->cmd_err == -EAGAIN)
-		ret = i2c_recover_bus(adap);
+		bus->cmd_err = i2c_recover_bus(adap);
 
 	/*
 	 * After any type of error, check if LAST bit is still set,
 	 * due to a HW issue.
 	 * It cannot be cleared without resetting the module.
 	 */
-	if (bus->cmd_err &&
-	    (NPCM_I2CRXF_CTL_LAST_PEC & ioread8(bus->reg + NPCM_I2CRXF_CTL)))
+	else if (bus->cmd_err &&
+		 (NPCM_I2CRXF_CTL_LAST_PEC & ioread8(bus->reg + NPCM_I2CRXF_CTL)))
 		npcm_i2c_reset(bus);
 
+	/* after any xfer, successful or not, stall and EOB must be disabled */
+	npcm_i2c_stall_after_start(bus, false);
+	npcm_i2c_eob_int(bus, false);
+
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 	/* reenable slave if it was enabled */
 	if (bus->slave)
 		iowrite8((bus->slave->addr & 0x7F) | NPCM_I2CADDR_SAEN,
 			 bus->reg + NPCM_I2CADDR1);
+#else
+	npcm_i2c_int_enable(bus, false);
 #endif
 	return bus->cmd_err;
 }
-- 
2.17.1

Re: [PATCH v3 09/11] i2c: npcm: Handle spurious interrupts

[Andy Shevchenko]

On Thu, Mar 03, 2022 at 04:31:39PM +0800, Tyrone Ting wrote:
> From: Tali Perry <tali.perry1@gmail.com>
> 
> 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.

...

> +	/*
> +	 * 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.

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

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 09/11] i2c: npcm: Handle spurious interrupts

[Tali Perry]

> On Thu, Mar 3, 2022 at 12:37 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Thu, Mar 03, 2022 at 04:31:39PM +0800, Tyrone Ting wrote:
> > > From: Tali Perry <tali.perry1@gmail.com>
> > >
> > > 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.


> >
> > > +     /*
> > > +      * 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.
> >
> > > +      */
> > > +     if (ret == IRQ_NONE) {
> > > +             npcm_i2c_eob_int(bus, false);
> > > +             npcm_i2c_clear_master_status(bus);
> > > +     }
> > > +
> > > +     return IRQ_HANDLED;
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >

Thanks Andy,

BR,
Tali Perry

Re: [PATCH v3 09/11] i2c: npcm: Handle spurious interrupts

[Andy Shevchenko]

On Thu, Mar 03, 2022 at 02:48:20PM +0200, Tali Perry wrote:
> > On Thu, Mar 3, 2022 at 12:37 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > >
> > > On Thu, Mar 03, 2022 at 04:31:39PM +0800, Tyrone Ting wrote:
> > > > From: Tali Perry <tali.perry1@gmail.com>
> > > >
> > > > 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.

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.

> 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.
> 
> > > > +     /*
> > > > +      * 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.
> > >
> > > > +      */
> > > > +     if (ret == IRQ_NONE) {
> > > > +             npcm_i2c_eob_int(bus, false);
> > > > +             npcm_i2c_clear_master_status(bus);
> > > > +     }
> > > > +
> > > > +     return IRQ_HANDLED;

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 09/11] i2c: npcm: Handle spurious interrupts

[Avi Fishman]

On Thu, Mar 3, 2022 at 4:14 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Mar 03, 2022 at 02:48:20PM +0200, Tali Perry wrote:
> > > On Thu, Mar 3, 2022 at 12:37 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > >
> > > > On Thu, Mar 03, 2022 at 04:31:39PM +0800, Tyrone Ting wrote:
> > > > > From: Tali Perry <tali.perry1@gmail.com>
> > > > >
> > > > > 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.
>
> 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.
> > 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.
> >
> > > > > +     /*
> > > > > +      * 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.
> > > >
> > > > > +      */
> > > > > +     if (ret == IRQ_NONE) {
> > > > > +             npcm_i2c_eob_int(bus, false);
> > > > > +             npcm_i2c_clear_master_status(bus);
> > > > > +     }
> > > > > +
> > > > > +     return IRQ_HANDLED;
>
> --
> With Best Regards,
> Andy Shevchenko
>
>


-- 
Regards,
Avi

Re: [PATCH v3 09/11] i2c: npcm: Handle spurious interrupts

[Andy Shevchenko]

On Mon, Apr 04, 2022 at 08:03:44PM +0300, Avi Fishman wrote:
> On Thu, Mar 3, 2022 at 4:14 PM Andy Shevchenko
> <andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Mar 03, 2022 at 02:48:20PM +0200, Tali Perry wrote:
> > > > On Thu, Mar 3, 2022 at 12:37 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > > > On Thu, Mar 03, 2022 at 04:31:39PM +0800, Tyrone Ting wrote:
> > > > > > From: Tali Perry <tali.perry1@gmail.com>
> > > > > >
> > > > > > 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.

(1)

> > > 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.
> 
> 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.

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%?

> > > 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.
> > >
> > > > > > +     /*
> > > > > > +      * 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.
> > > > >
> > > > > > +      */

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 09/11] i2c: npcm: Handle spurious interrupts

[Avi Fishman]

On Tue, Apr 5, 2022 at 10:13 AM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Mon, Apr 04, 2022 at 08:03:44PM +0300, Avi Fishman wrote:
> > On Thu, Mar 3, 2022 at 4:14 PM Andy Shevchenko
> > <andriy.shevchenko@linux.intel.com> wrote:
> > > On Thu, Mar 03, 2022 at 02:48:20PM +0200, Tali Perry wrote:
> > > > > On Thu, Mar 3, 2022 at 12:37 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > > > > On Thu, Mar 03, 2022 at 04:31:39PM +0800, Tyrone Ting wrote:
> > > > > > > From: Tali Perry <tali.perry1@gmail.com>
> > > > > > >
> > > > > > > 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.
>
> (1)
>
> > > > 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.
> >
> > 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

About 1 to 100,000 transactions

> > 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.
>

The HW issue is not known yet, we see it on few platforms and in other
platforms we don't, so the first assumption was this.
So eventually we don't want to claim this without proving it.

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

I added above the known statistics.

>
> > > > 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.
> > > >
> > > > > > > +     /*
> > > > > > > +      * 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.
> > > > > >
> > > > > > > +      */
>
> --
> With Best Regards,
> Andy Shevchenko
>
>


-- 
Regards,
Avi

[PATCH v3 10/11] i2c: npcm: Remove own slave addresses 2:10

[Tyrone Ting]

From: Tali Perry <tali.perry1@gmail.com>

NPCM can support up to 10 own slave addresses.
In practice, only one address is actually being used.
In order to access addresses 2 and above, need to switch
register banks. The switch needs spinlock.
To avoid using spinlock for this useless feature
removed support of SA >= 2.

Also fix returned slave event enum.

Remove some comment since the bank selection is not
required. The bank selection is not required since
the supported slave addresses are reduced.

Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
Signed-off-by: Tali Perry <tali.perry1@gmail.com>
Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
---
 drivers/i2c/busses/i2c-npcm7xx.c | 46 ++++++++++++++++----------------
 1 file changed, 23 insertions(+), 23 deletions(-)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 73cef76127c9..5c0bbc134f9d 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -124,6 +124,8 @@ enum i2c_addr {
  * use this array to get the address or each register.
  */
 #define I2C_NUM_OWN_ADDR 10
+#define I2C_NUM_OWN_ADDR_SUPPORTED 2
+
 static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
 	NPCM_I2CADDR1, NPCM_I2CADDR2, NPCM_I2CADDR3, NPCM_I2CADDR4,
 	NPCM_I2CADDR5, NPCM_I2CADDR6, NPCM_I2CADDR7, NPCM_I2CADDR8,
@@ -392,14 +394,10 @@ static void npcm_i2c_disable(struct npcm_i2c *bus)
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 	int i;
 
-	/* select bank 0 for I2C addresses */
-	npcm_i2c_select_bank(bus, I2C_BANK_0);
-
 	/* Slave addresses removal */
-	for (i = I2C_SLAVE_ADDR1; i < I2C_NUM_OWN_ADDR; i++)
+	for (i = I2C_SLAVE_ADDR1; i < I2C_NUM_OWN_ADDR_SUPPORTED; i++)
 		iowrite8(0, bus->reg + npcm_i2caddr[i]);
 
-	npcm_i2c_select_bank(bus, I2C_BANK_1);
 #endif
 	/* Disable module */
 	i2cctl2 = ioread8(bus->reg + NPCM_I2CCTL2);
@@ -604,8 +602,7 @@ static int npcm_i2c_slave_enable(struct npcm_i2c *bus, enum i2c_addr addr_type,
 			i2cctl1 &= ~NPCM_I2CCTL1_GCMEN;
 		iowrite8(i2cctl1, bus->reg + NPCM_I2CCTL1);
 		return 0;
-	}
-	if (addr_type == I2C_ARP_ADDR) {
+	} else if (addr_type == I2C_ARP_ADDR) {
 		i2cctl3 = ioread8(bus->reg + NPCM_I2CCTL3);
 		if (enable)
 			i2cctl3 |= I2CCTL3_ARPMEN;
@@ -614,16 +611,18 @@ static int npcm_i2c_slave_enable(struct npcm_i2c *bus, enum i2c_addr addr_type,
 		iowrite8(i2cctl3, bus->reg + NPCM_I2CCTL3);
 		return 0;
 	}
+	if (addr_type > I2C_SLAVE_ADDR2 && addr_type <= I2C_SLAVE_ADDR10) {
+		dev_err(bus->dev,
+			"try to enable more then 2 SA not supported\n");
+	}
 	if (addr_type >= I2C_ARP_ADDR)
 		return -EFAULT;
 	/* select bank 0 for address 3 to 10 */
-	if (addr_type > I2C_SLAVE_ADDR2)
-		npcm_i2c_select_bank(bus, I2C_BANK_0);
+
 	/* Set and enable the address */
 	iowrite8(sa_reg, bus->reg + npcm_i2caddr[addr_type]);
 	npcm_i2c_slave_int_enable(bus, enable);
-	if (addr_type > I2C_SLAVE_ADDR2)
-		npcm_i2c_select_bank(bus, I2C_BANK_1);
+
 	return 0;
 }
 #endif
@@ -846,15 +845,13 @@ static u8 npcm_i2c_get_slave_addr(struct npcm_i2c *bus, enum i2c_addr addr_type)
 {
 	u8 slave_add;
 
-	/* select bank 0 for address 3 to 10 */
-	if (addr_type > I2C_SLAVE_ADDR2)
-		npcm_i2c_select_bank(bus, I2C_BANK_0);
+	if (addr_type > I2C_SLAVE_ADDR2 && addr_type <= I2C_SLAVE_ADDR10) {
+		dev_err(bus->dev,
+			"get slave: try to use more then 2 slave addresses not supported\n");
+	}
 
 	slave_add = ioread8(bus->reg + npcm_i2caddr[(int)addr_type]);
 
-	if (addr_type > I2C_SLAVE_ADDR2)
-		npcm_i2c_select_bank(bus, I2C_BANK_1);
-
 	return slave_add;
 }
 
@@ -864,12 +861,12 @@ static int npcm_i2c_remove_slave_addr(struct npcm_i2c *bus, u8 slave_add)
 
 	/* Set the enable bit */
 	slave_add |= 0x80;
-	npcm_i2c_select_bank(bus, I2C_BANK_0);
-	for (i = I2C_SLAVE_ADDR1; i < I2C_NUM_OWN_ADDR; i++) {
+
+	for (i = I2C_SLAVE_ADDR1; i < I2C_NUM_OWN_ADDR_SUPPORTED; i++) {
 		if (ioread8(bus->reg + npcm_i2caddr[i]) == slave_add)
 			iowrite8(0, bus->reg + npcm_i2caddr[i]);
 	}
-	npcm_i2c_select_bank(bus, I2C_BANK_1);
+
 	return 0;
 }
 
@@ -924,11 +921,15 @@ static int npcm_i2c_slave_get_wr_buf(struct npcm_i2c *bus)
 	for (i = 0; i < I2C_HW_FIFO_SIZE; i++) {
 		if (bus->slv_wr_size >= I2C_HW_FIFO_SIZE)
 			break;
-		i2c_slave_event(bus->slave, I2C_SLAVE_READ_REQUESTED, &value);
+		if (bus->state == I2C_SLAVE_MATCH) {
+			i2c_slave_event(bus->slave, I2C_SLAVE_READ_REQUESTED, &value);
+			bus->state = I2C_OPER_STARTED;
+		} else {
+			i2c_slave_event(bus->slave, I2C_SLAVE_READ_PROCESSED, &value);
+		}
 		ind = (bus->slv_wr_ind + bus->slv_wr_size) % I2C_HW_FIFO_SIZE;
 		bus->slv_wr_buf[ind] = value;
 		bus->slv_wr_size++;
-		i2c_slave_event(bus->slave, I2C_SLAVE_READ_PROCESSED, &value);
 	}
 	return I2C_HW_FIFO_SIZE - ret;
 }
@@ -976,7 +977,6 @@ static void npcm_i2c_slave_xmit(struct npcm_i2c *bus, u16 nwrite,
 	if (nwrite == 0)
 		return;
 
-	bus->state = I2C_OPER_STARTED;
 	bus->operation = I2C_WRITE_OPER;
 
 	/* get the next buffer */
-- 
2.17.1

[PATCH v3 11/11] i2c: npcm: Support NPCM845

[Tyrone Ting]

From: Tyrone Ting <kfting@nuvoton.com>

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 <kfting@nuvoton.com>
Signed-off-by: Tali Perry <tali.perry1@gmail.com>
---
 drivers/i2c/busses/Kconfig       |   8 +-
 drivers/i2c/busses/Makefile      |   2 +-
 drivers/i2c/busses/i2c-npcm7xx.c | 126 +++++++++++++++++++------------
 3 files changed, 83 insertions(+), 53 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 8a6c6ee28556..ea0b63274442 100644
--- a/drivers/i2c/busses/Kconfig
+++ b/drivers/i2c/busses/Kconfig
@@ -817,13 +817,13 @@ config I2C_NOMADIK
 	  I2C interface from ST-Ericsson's Nomadik and Ux500 architectures,
 	  as well as the STA2X11 PCIe I/O HUB.
 
-config I2C_NPCM7XX
+config I2C_NPCM
 	tristate "Nuvoton I2C Controller"
-	depends on ARCH_NPCM7XX || COMPILE_TEST
+	depends on ARCH_NPCM || COMPILE_TEST
 	help
 	  If you say yes to this option, support will be included for the
-	  Nuvoton I2C controller, which is available on the NPCM7xx BMC
-	  controller.
+	  Nuvoton I2C controller, which is available on the NPCM BMC
+	  controllers.
 	  Driver can also support slave mode (select I2C_SLAVE).
 
 config I2C_OCORES
diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile
index 1d00dce77098..01fdf74a5565 100644
--- a/drivers/i2c/busses/Makefile
+++ b/drivers/i2c/busses/Makefile
@@ -80,7 +80,7 @@ obj-$(CONFIG_I2C_MT7621)	+= i2c-mt7621.o
 obj-$(CONFIG_I2C_MV64XXX)	+= i2c-mv64xxx.o
 obj-$(CONFIG_I2C_MXS)		+= i2c-mxs.o
 obj-$(CONFIG_I2C_NOMADIK)	+= i2c-nomadik.o
-obj-$(CONFIG_I2C_NPCM7XX)	+= i2c-npcm7xx.o
+obj-$(CONFIG_I2C_NPCM)		+= i2c-npcm7xx.o
 obj-$(CONFIG_I2C_OCORES)	+= i2c-ocores.o
 obj-$(CONFIG_I2C_OMAP)		+= i2c-omap.o
 obj-$(CONFIG_I2C_OWL)		+= i2c-owl.o
diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 5c0bbc134f9d..a6266ebc6a6f 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -17,6 +17,7 @@
 #include <linux/mfd/syscon.h>
 #include <linux/module.h>
 #include <linux/of.h>
+#include <linux/of_device.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
 
@@ -91,7 +92,6 @@ enum i2c_addr {
 
 /* init register and default value required to enable module */
 #define NPCM_I2CSEGCTL			0xE4
-#define NPCM_I2CSEGCTL_INIT_VAL		0x0333F000
 
 /* Common regs */
 #define NPCM_I2CSDA			0x00
@@ -228,8 +228,7 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
 #define NPCM_I2CFIF_CTS_CLR_FIFO	BIT(6)
 #define NPCM_I2CFIF_CTS_SLVRSTR		BIT(7)
 
-/* NPCM_I2CTXF_CTL reg fields */
-#define NPCM_I2CTXF_CTL_TX_THR		GENMASK(4, 0)
+/* NPCM_I2CTXF_CTL reg field */
 #define NPCM_I2CTXF_CTL_THR_TXIE	BIT(6)
 
 /* NPCM_I2CT_OUT reg fields */
@@ -238,22 +237,18 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
 #define NPCM_I2CT_OUT_T_OUTST		BIT(7)
 
 /* NPCM_I2CTXF_STS reg fields */
-#define NPCM_I2CTXF_STS_TX_BYTES	GENMASK(4, 0)
 #define NPCM_I2CTXF_STS_TX_THST		BIT(6)
 
 /* NPCM_I2CRXF_STS reg fields */
-#define NPCM_I2CRXF_STS_RX_BYTES	GENMASK(4, 0)
 #define NPCM_I2CRXF_STS_RX_THST		BIT(6)
 
 /* NPCM_I2CFIF_CTL reg fields */
 #define NPCM_I2CFIF_CTL_FIFO_EN		BIT(4)
 
 /* NPCM_I2CRXF_CTL reg fields */
-#define NPCM_I2CRXF_CTL_RX_THR		GENMASK(4, 0)
-#define NPCM_I2CRXF_CTL_LAST_PEC	BIT(5)
 #define NPCM_I2CRXF_CTL_THR_RXIE	BIT(6)
 
-#define I2C_HW_FIFO_SIZE		16
+#define MAX_I2C_HW_FIFO_SIZE		32
 
 /* I2C_VER reg fields */
 #define I2C_VER_VERSION			GENMASK(6, 0)
@@ -270,11 +265,36 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
 #define I2C_FREQ_MIN_HZ			10000
 #define I2C_FREQ_MAX_HZ			I2C_MAX_FAST_MODE_PLUS_FREQ
 
+struct npcm_i2c_data {
+	u8 fifo_size;
+	u32 segctl_init_val;
+	u8 txf_sts_tx_bytes;
+	u8 rxf_sts_rx_bytes;
+	u8 rxf_ctl_last_pec;
+};
+
+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)
+};
+
+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)
+};
+
 /* Status of one I2C module */
 struct npcm_i2c {
 	struct i2c_adapter adap;
 	struct device *dev;
 	unsigned char __iomem *reg;
+	const struct npcm_i2c_data *data;
 	spinlock_t lock;   /* IRQ synchronization */
 	struct completion cmd_complete;
 	int cmd_err;
@@ -307,8 +327,8 @@ struct npcm_i2c {
 	int slv_rd_ind;
 	int slv_wr_size;
 	int slv_wr_ind;
-	u8 slv_rd_buf[I2C_HW_FIFO_SIZE];
-	u8 slv_wr_buf[I2C_HW_FIFO_SIZE];
+	u8 slv_rd_buf[MAX_I2C_HW_FIFO_SIZE];
+	u8 slv_wr_buf[MAX_I2C_HW_FIFO_SIZE];
 #endif
 	struct dentry *debugfs; /* debugfs device directory */
 	u64 ber_cnt;
@@ -441,7 +461,7 @@ static inline bool npcm_i2c_tx_fifo_empty(struct npcm_i2c *bus)
 
 	tx_fifo_sts = ioread8(bus->reg + NPCM_I2CTXF_STS);
 	/* check if TX FIFO is not empty */
-	if ((tx_fifo_sts & NPCM_I2CTXF_STS_TX_BYTES) == 0)
+	if ((tx_fifo_sts & bus->data->txf_sts_tx_bytes) == 0)
 		return false;
 
 	/* check if TX FIFO status bit is set: */
@@ -454,7 +474,7 @@ static inline bool npcm_i2c_rx_fifo_full(struct npcm_i2c *bus)
 
 	rx_fifo_sts = ioread8(bus->reg + NPCM_I2CRXF_STS);
 	/* check if RX FIFO is not empty: */
-	if ((rx_fifo_sts & NPCM_I2CRXF_STS_RX_BYTES) == 0)
+	if ((rx_fifo_sts & bus->data->rxf_sts_rx_bytes) == 0)
 		return false;
 
 	/* check if rx fifo full status is set: */
@@ -743,11 +763,11 @@ static void npcm_i2c_callback(struct npcm_i2c *bus,
 static u8 npcm_i2c_fifo_usage(struct npcm_i2c *bus)
 {
 	if (bus->operation == I2C_WRITE_OPER)
-		return FIELD_GET(NPCM_I2CTXF_STS_TX_BYTES,
-				 ioread8(bus->reg + NPCM_I2CTXF_STS));
+		return (bus->data->txf_sts_tx_bytes &
+			ioread8(bus->reg + NPCM_I2CTXF_STS));
 	if (bus->operation == I2C_READ_OPER)
-		return FIELD_GET(NPCM_I2CRXF_STS_RX_BYTES,
-				 ioread8(bus->reg + NPCM_I2CRXF_STS));
+		return (bus->data->rxf_sts_rx_bytes &
+			ioread8(bus->reg + NPCM_I2CRXF_STS));
 	return 0;
 }
 
@@ -759,13 +779,13 @@ static void npcm_i2c_write_to_fifo_master(struct npcm_i2c *bus, u16 max_bytes)
 	 * Fill the FIFO, while the FIFO is not full and there are more bytes
 	 * to write
 	 */
-	size_free_fifo = I2C_HW_FIFO_SIZE - npcm_i2c_fifo_usage(bus);
+	size_free_fifo = bus->data->fifo_size - npcm_i2c_fifo_usage(bus);
 	while (max_bytes-- && size_free_fifo) {
 		if (bus->wr_ind < bus->wr_size)
 			npcm_i2c_wr_byte(bus, bus->wr_buf[bus->wr_ind++]);
 		else
 			npcm_i2c_wr_byte(bus, 0xFF);
-		size_free_fifo = I2C_HW_FIFO_SIZE - npcm_i2c_fifo_usage(bus);
+		size_free_fifo = bus->data->fifo_size - npcm_i2c_fifo_usage(bus);
 	}
 }
 
@@ -786,11 +806,11 @@ static void npcm_i2c_set_fifo(struct npcm_i2c *bus, int nread, int nwrite)
 
 	/* configure RX FIFO */
 	if (nread > 0) {
-		rxf_ctl = min_t(int, nread, I2C_HW_FIFO_SIZE);
+		rxf_ctl = min_t(int, nread, bus->data->fifo_size);
 
 		/* set LAST bit. if LAST is set next FIFO packet is nacked */
-		if (nread <= I2C_HW_FIFO_SIZE)
-			rxf_ctl |= NPCM_I2CRXF_CTL_LAST_PEC;
+		if (nread <= bus->data->fifo_size)
+			rxf_ctl |= bus->data->rxf_ctl_last_pec;
 
 		/*
 		 * if we are about to read the first byte in blk rd mode,
@@ -808,9 +828,9 @@ static void npcm_i2c_set_fifo(struct npcm_i2c *bus, int nread, int nwrite)
 
 	/* configure TX FIFO */
 	if (nwrite > 0) {
-		if (nwrite > I2C_HW_FIFO_SIZE)
+		if (nwrite > bus->data->fifo_size)
 			/* data to send is more then FIFO size. */
-			iowrite8(I2C_HW_FIFO_SIZE, bus->reg + NPCM_I2CTXF_CTL);
+			iowrite8(bus->data->fifo_size, bus->reg + NPCM_I2CTXF_CTL);
 		else
 			iowrite8(nwrite, bus->reg + NPCM_I2CTXF_CTL);
 
@@ -879,13 +899,13 @@ static void npcm_i2c_write_fifo_slave(struct npcm_i2c *bus, u16 max_bytes)
 	npcm_i2c_clear_fifo_int(bus);
 	npcm_i2c_clear_tx_fifo(bus);
 	iowrite8(0, bus->reg + NPCM_I2CTXF_CTL);
-	while (max_bytes-- && I2C_HW_FIFO_SIZE != npcm_i2c_fifo_usage(bus)) {
+	while (max_bytes-- && bus->data->fifo_size != npcm_i2c_fifo_usage(bus)) {
 		if (bus->slv_wr_size <= 0)
 			break;
-		bus->slv_wr_ind = bus->slv_wr_ind % I2C_HW_FIFO_SIZE;
+		bus->slv_wr_ind = bus->slv_wr_ind & (bus->data->fifo_size - 1);
 		npcm_i2c_wr_byte(bus, bus->slv_wr_buf[bus->slv_wr_ind]);
 		bus->slv_wr_ind++;
-		bus->slv_wr_ind = bus->slv_wr_ind % I2C_HW_FIFO_SIZE;
+		bus->slv_wr_ind = bus->slv_wr_ind & (bus->data->fifo_size - 1);
 		bus->slv_wr_size--;
 	}
 }
@@ -900,7 +920,7 @@ static void npcm_i2c_read_fifo_slave(struct npcm_i2c *bus, u8 bytes_in_fifo)
 	while (bytes_in_fifo--) {
 		data = npcm_i2c_rd_byte(bus);
 
-		bus->slv_rd_ind = bus->slv_rd_ind % I2C_HW_FIFO_SIZE;
+		bus->slv_rd_ind = bus->slv_rd_ind & (bus->data->fifo_size - 1);
 		bus->slv_rd_buf[bus->slv_rd_ind] = data;
 		bus->slv_rd_ind++;
 
@@ -918,8 +938,8 @@ static int npcm_i2c_slave_get_wr_buf(struct npcm_i2c *bus)
 	int ret = bus->slv_wr_ind;
 
 	/* fill a cyclic buffer */
-	for (i = 0; i < I2C_HW_FIFO_SIZE; i++) {
-		if (bus->slv_wr_size >= I2C_HW_FIFO_SIZE)
+	for (i = 0; i < bus->data->fifo_size; i++) {
+		if (bus->slv_wr_size >= bus->data->fifo_size)
 			break;
 		if (bus->state == I2C_SLAVE_MATCH) {
 			i2c_slave_event(bus->slave, I2C_SLAVE_READ_REQUESTED, &value);
@@ -927,11 +947,11 @@ static int npcm_i2c_slave_get_wr_buf(struct npcm_i2c *bus)
 		} else {
 			i2c_slave_event(bus->slave, I2C_SLAVE_READ_PROCESSED, &value);
 		}
-		ind = (bus->slv_wr_ind + bus->slv_wr_size) % I2C_HW_FIFO_SIZE;
+		ind = (bus->slv_wr_ind + bus->slv_wr_size) & (bus->data->fifo_size - 1);
 		bus->slv_wr_buf[ind] = value;
 		bus->slv_wr_size++;
 	}
-	return I2C_HW_FIFO_SIZE - ret;
+	return bus->data->fifo_size - ret;
 }
 
 static void npcm_i2c_slave_send_rd_buf(struct npcm_i2c *bus)
@@ -966,7 +986,7 @@ static void npcm_i2c_slave_receive(struct npcm_i2c *bus, u16 nread,
 	bus->slv_rd_ind = 0;
 
 	iowrite8(0, bus->reg + NPCM_I2CTXF_CTL);
-	iowrite8(I2C_HW_FIFO_SIZE, bus->reg + NPCM_I2CRXF_CTL);
+	iowrite8(bus->data->fifo_size, bus->reg + NPCM_I2CRXF_CTL);
 	npcm_i2c_clear_tx_fifo(bus);
 	npcm_i2c_clear_rx_fifo(bus);
 }
@@ -999,12 +1019,12 @@ static void npcm_i2c_slave_wr_buf_sync(struct npcm_i2c *bus)
 {
 	int left_in_fifo;
 
-	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));
 
 	/* fifo already full: */
-	if (left_in_fifo >= I2C_HW_FIFO_SIZE ||
-	    bus->slv_wr_size >= I2C_HW_FIFO_SIZE)
+	if (left_in_fifo >= bus->data->fifo_size ||
+	    bus->slv_wr_size >= bus->data->fifo_size)
 		return;
 
 	/* update the wr fifo index back to the untransmitted bytes: */
@@ -1012,7 +1032,7 @@ static void npcm_i2c_slave_wr_buf_sync(struct npcm_i2c *bus)
 	bus->slv_wr_size = bus->slv_wr_size + left_in_fifo;
 
 	if (bus->slv_wr_ind < 0)
-		bus->slv_wr_ind += I2C_HW_FIFO_SIZE;
+		bus->slv_wr_ind += bus->data->fifo_size;
 }
 
 static void npcm_i2c_slave_rd_wr(struct npcm_i2c *bus)
@@ -1158,7 +1178,7 @@ static irqreturn_t npcm_i2c_int_slave_handler(struct npcm_i2c *bus)
 		npcm_i2c_clear_rx_fifo(bus);
 		npcm_i2c_clear_tx_fifo(bus);
 		iowrite8(0, bus->reg + NPCM_I2CTXF_CTL);
-		iowrite8(I2C_HW_FIFO_SIZE, bus->reg + NPCM_I2CRXF_CTL);
+		iowrite8(bus->data->fifo_size, bus->reg + NPCM_I2CRXF_CTL);
 		if (NPCM_I2CST_XMIT & i2cst) {
 			bus->operation = I2C_WRITE_OPER;
 		} else {
@@ -1319,8 +1339,8 @@ static void npcm_i2c_master_fifo_read(struct npcm_i2c *bus)
 	 * read == FIFO Size + C (where C < FIFO Size)then first read C bytes
 	 * and in the next int we read rest of the data.
 	 */
-	if (rcount < (2 * I2C_HW_FIFO_SIZE) && rcount > I2C_HW_FIFO_SIZE)
-		fifo_bytes = rcount - I2C_HW_FIFO_SIZE;
+	if (rcount < (2 * bus->data->fifo_size) && rcount > bus->data->fifo_size)
+		fifo_bytes = rcount - bus->data->fifo_size;
 
 	if (rcount <= fifo_bytes) {
 		/* last bytes are about to be read - end of tx */
@@ -2200,7 +2220,7 @@ static int npcm_i2c_master_xfer(struct i2c_adapter *adap, struct i2c_msg *msgs,
 	 * It cannot be cleared without resetting the module.
 	 */
 	else if (bus->cmd_err &&
-		 (NPCM_I2CRXF_CTL_LAST_PEC & ioread8(bus->reg + NPCM_I2CRXF_CTL)))
+		 (bus->data->rxf_ctl_last_pec & ioread8(bus->reg + NPCM_I2CRXF_CTL)))
 		npcm_i2c_reset(bus);
 
 	/* after any xfer, successful or not, stall and EOB must be disabled */
@@ -2265,12 +2285,21 @@ static void npcm_i2c_init_debugfs(struct platform_device *pdev,
 	bus->debugfs = d;
 }
 
+static const struct of_device_id npcm_i2c_bus_of_table[] = {
+	{ .compatible = "nuvoton,npcm750-i2c", .data = &npxm7xx_i2c_data },
+	{ .compatible = "nuvoton,npcm845-i2c", .data = &npxm8xx_i2c_data },
+	{}
+};
+MODULE_DEVICE_TABLE(of, npcm_i2c_bus_of_table);
+
 static int npcm_i2c_probe_bus(struct platform_device *pdev)
 {
 	struct npcm_i2c *bus;
 	struct i2c_adapter *adap;
 	struct clk *i2c_clk;
 	static struct regmap *gcr_regmap;
+	struct device *dev = &pdev->dev;
+	const struct of_device_id *match;
 	int irq;
 	int ret;
 	struct device_node *np = pdev->dev.of_node;
@@ -2281,6 +2310,13 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
 
 	bus->dev = &pdev->dev;
 
+	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;
+
 	bus->num = of_alias_get_id(pdev->dev.of_node, "i2c");
 	/* core clk must be acquired to calculate module timing settings */
 	i2c_clk = devm_clk_get(&pdev->dev, NULL);
@@ -2294,7 +2330,7 @@ static int npcm_i2c_probe_bus(struct platform_device *pdev)
 
 	if (IS_ERR(gcr_regmap))
 		return PTR_ERR(gcr_regmap);
-	regmap_write(gcr_regmap, NPCM_I2CSEGCTL, NPCM_I2CSEGCTL_INIT_VAL);
+	regmap_write(gcr_regmap, NPCM_I2CSEGCTL, bus->data->segctl_init_val);
 
 	bus->reg = devm_platform_ioremap_resource(pdev, 0);
 	if (IS_ERR(bus->reg))
@@ -2355,12 +2391,6 @@ static int npcm_i2c_remove_bus(struct platform_device *pdev)
 	return 0;
 }
 
-static const struct of_device_id npcm_i2c_bus_of_table[] = {
-	{ .compatible = "nuvoton,npcm750-i2c", },
-	{}
-};
-MODULE_DEVICE_TABLE(of, npcm_i2c_bus_of_table);
-
 static struct platform_driver npcm_i2c_bus_driver = {
 	.probe = npcm_i2c_probe_bus,
 	.remove = npcm_i2c_remove_bus,
-- 
2.17.1

Re: [PATCH v3 11/11] i2c: npcm: Support NPCM845

[Andy Shevchenko]

On Thu, Mar 03, 2022 at 04:31:41PM +0800, Tyrone Ting wrote:
> From: Tyrone Ting <kfting@nuvoton.com>
> 
> 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 <kfting@nuvoton.com>
> Signed-off-by: Tali Perry <tali.perry1@gmail.com>

Wrong SoB chain.

...

> +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.

> +};
> +
> +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.

> +};

...

> -	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?

...

> +	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().

...

> -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.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 11/11] i2c: npcm: Support NPCM845

[Tali Perry]

> On Thu, Mar 3, 2022 at 12:45 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> >
> > On Thu, Mar 03, 2022 at 04:31:41PM +0800, Tyrone Ting wrote:
> > > From: Tyrone Ting <kfting@nuvoton.com>
> > >
> > > 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 <kfting@nuvoton.com>
> > > Signed-off-by: Tali Perry <tali.perry1@gmail.com>
> >
> > Wrong SoB chain.
> >
> > ...
> >
> > > +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.
> >
> > > +};
> > > +
> > > +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.
> >
> > > +};
> >
> > ...
> >
> > > -     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?


> > ...
> >
> > > +     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().
> >
> > ...
> >
> > > -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.
> >
> > --
> > With Best Regards,
> > Andy Shevchenko
> >
> >

Thanks for the detailed review, Andy!

BR,
Tali Perry
Nuvoton Technologies

Re: [PATCH v3 11/11] i2c: npcm: Support NPCM845

[Andy Shevchenko]

On Thu, Mar 03, 2022 at 02:35:58PM +0200, Tali Perry wrote:
> > On Thu, Mar 3, 2022 at 12:45 PM Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote:
> > > On Thu, Mar 03, 2022 at 04:31:41PM +0800, Tyrone Ting wrote:

...

> > > > -     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?

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

Re: [PATCH v3 11/11] i2c: npcm: Support NPCM845

[Andy Shevchenko]

On Sun, Mar 06, 2022 at 03:33:20PM +0200, Tomer Maimon wrote:
> On Thu, 3 Mar 2022 at 16:11, Andy Shevchenko <
> andriy.shevchenko@linux.intel.com> wrote:
> > On Thu, Mar 03, 2022 at 02:35:58PM +0200, Tali Perry wrote:
> > > > On Thu, Mar 3, 2022 at 12:45 PM Andy Shevchenko <
> > andriy.shevchenko@linux.intel.com> wrote:

...

> > 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?

I meant from 16 bytes to 32 bytes.

> > 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?

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

Re: [PATCH v3 00/11] i2c: npcm: Bug fixes timeout, spurious interrupts

[Andy Shevchenko]

On Thu, Mar 03, 2022 at 04:31:30PM +0800, Tyrone Ting wrote:
> From: Tyrone Ting <kfting@nuvoton.com>
> 
> 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.

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

Re: [PATCH v3 00/11] i2c: npcm: Bug fixes timeout, spurious interrupts

[Tali Perry]

On Thu, Mar 3, 2022 at 12:27 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Thu, Mar 03, 2022 at 04:31:30PM +0800, Tyrone Ting wrote:
> > From: Tyrone Ting <kfting@nuvoton.com>
> >
> > 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.
>
> 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.
>
> --


Actually it was and is being used by multiple users in lots of BMCs.
We haven't submitted patches for this driver for a while
and accumulated them all on Nuvoton Github repo, but now we wanted to
clear the table.

All your comments will be addressed and fixed for the next patchset.
As always, we really appreciate your review and taking the time to go
through all these changes.


> With Best Regards,
> Andy Shevchenko
>
>

Thanks!

Tali Perry