[PATCH v1 0/6] i2c: npcm: Bug fixes timeout, spurious interrupts

[PATCH v1 0/6] 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.

Tali Perry (4):
  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 (2):
  dt-bindings: i2c: npcm: support NPCM845
  i2c: npcm: Support NPCM845

 .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml     |  21 +-
 drivers/i2c/busses/Kconfig                    |   8 +-
 drivers/i2c/busses/Makefile                   |   2 +-
 drivers/i2c/busses/i2c-npcm7xx.c              | 191 ++++++++++++------
 4 files changed, 149 insertions(+), 73 deletions(-)

-- 
2.17.1

[PATCH v1 1/6] dt-bindings: i2c: npcm: support NPCM845

[Tyrone Ting]

From: Tyrone Ting <kfting@nuvoton.com>

This commit adds compatible and syscon description for NPCM845 i2c module.

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>
---
 .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml     | 21 ++++++++++++++-----
 1 file changed, 16 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..05e58f44b03a 100644
--- a/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
+++ b/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
@@ -7,17 +7,22 @@ $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.
+  The NPCM7XX includes sixteen I2C bus controllers and the NPCM8XX includes
+  twenty-seven I2C bus controllers. NPCM8XX controllers 24-26 are connected on I2C
+  pins in parallel to controllers 8-10.
+  All controllers support both master and slave mode.
+  Each controller can switch between master and slave at run time (i.e. IPMB mode).
+  NPCM7XX I2C controller has two 16 byte HW FIFO for TX and RX and NPCM8XX I2C
+  controller has two 32 byte HW FIFO for TX and RX.
 
 maintainers:
   - Tali Perry <tali.perry1@gmail.com>
 
 properties:
   compatible:
-    const: nuvoton,npcm750-i2c
+    enum:
+      - nuvoton,npcm750-i2c
+      - nuvoton,npcm845-i2c
 
   reg:
     maxItems: 1
@@ -36,11 +41,16 @@ properties:
     default: 100000
     enum: [100000, 400000, 1000000]
 
+  syscon:
+    $ref: "/schemas/types.yaml#/definitions/phandle"
+    description: The phandle of system manager register node.
+
 required:
   - compatible
   - reg
   - interrupts
   - clocks
+  - syscon
 
 allOf:
   - $ref: /schemas/i2c/i2c-controller.yaml#
@@ -57,6 +67,7 @@ examples:
         clock-frequency = <100000>;
         interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
         compatible = "nuvoton,npcm750-i2c";
+        syscon = <&gcr>;
     };
 
 ...
-- 
2.17.1

Re: [PATCH v1 1/6] dt-bindings: i2c: npcm: support NPCM845

[Jonathan Neuschäfer]

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

Hello,

On Mon, Feb 07, 2022 at 02:33:33PM +0800, Tyrone Ting wrote:
> From: Tyrone Ting <kfting@nuvoton.com>
> 
> This commit adds compatible and syscon description for NPCM845 i2c module.
> 
> 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>
> ---
[...]
>  
> +  syscon:
> +    $ref: "/schemas/types.yaml#/definitions/phandle"
> +    description: The phandle of system manager register node.
> +
>  required:
>    - compatible
>    - reg
>    - interrupts
>    - clocks
> +  - syscon

By making the syscon property required, the existing npcm7xx devicetrees
become wrong (because they don't have the syscon property).
Please, either keep it optional, or adjust the npcm7xx devicetrees in
this series.


Best regards,
Jonathan

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

Re: [PATCH v1 1/6] dt-bindings: i2c: npcm: support NPCM845

[Krzysztof Kozlowski]

On 07/02/2022 07:33, Tyrone Ting wrote:
> From: Tyrone Ting <kfting@nuvoton.com>
> 
> This commit adds compatible and syscon description for NPCM845 i2c module.

Do not use "This commit" but instead:
https://elixir.bootlin.com/linux/v5.13/source/Documentation/process/submitting-patches.rst#L89

> 
> 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>
> ---
>  .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml     | 21 ++++++++++++++-----
>  1 file changed, 16 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..05e58f44b03a 100644
> --- a/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
> @@ -7,17 +7,22 @@ $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.
> +  The NPCM7XX includes sixteen I2C bus controllers and the NPCM8XX includes
> +  twenty-seven I2C bus controllers. NPCM8XX controllers 24-26 are connected on I2C
> +  pins in parallel to controllers 8-10.
> +  All controllers support both master and slave mode.
> +  Each controller can switch between master and slave at run time (i.e. IPMB mode).
> +  NPCM7XX I2C controller has two 16 byte HW FIFO for TX and RX and NPCM8XX I2C
> +  controller has two 32 byte HW FIFO for TX and RX.
>  
>  maintainers:
>    - Tali Perry <tali.perry1@gmail.com>
>  
>  properties:
>    compatible:
> -    const: nuvoton,npcm750-i2c
> +    enum:
> +      - nuvoton,npcm750-i2c
> +      - nuvoton,npcm845-i2c
>  
>    reg:
>      maxItems: 1
> @@ -36,11 +41,16 @@ properties:
>      default: 100000
>      enum: [100000, 400000, 1000000]
>  
> +  syscon:
> +    $ref: "/schemas/types.yaml#/definitions/phandle"
> +    description: The phandle of system manager register node.
> +
>  required:
>    - compatible
>    - reg
>    - interrupts
>    - clocks
> +  - syscon

This breaks ABI of existing DTS. Actually your driver change does it but
here it is explicit...

>  
>  allOf:
>    - $ref: /schemas/i2c/i2c-controller.yaml#
> @@ -57,6 +67,7 @@ examples:
>          clock-frequency = <100000>;
>          interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
>          compatible = "nuvoton,npcm750-i2c";
> +        syscon = <&gcr>;
>      };
>  
>  ...


Best regards,
Krzysztof

Re: [PATCH v1 1/6] dt-bindings: i2c: npcm: support NPCM845

[Rob Herring]

On Mon, 07 Feb 2022 14:33:33 +0800, Tyrone Ting wrote:
> From: Tyrone Ting <kfting@nuvoton.com>
> 
> This commit adds compatible and syscon description for NPCM845 i2c module.
> 
> 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>
> ---
>  .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml     | 21 ++++++++++++++-----
>  1 file changed, 16 insertions(+), 5 deletions(-)
> 

Running 'make dtbs_check' with the schema in this patch gives the
following warnings. Consider if they are expected or the schema is
incorrect. These may not be new warnings.

Note that it is not yet a requirement to have 0 warnings for dtbs_check.
This will change in the future.

Full log is available here: https://patchwork.ozlabs.org/patch/1589171


i2c@80000: 'syscon' is a required property
	arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml

i2c@81000: 'syscon' is a required property
	arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml

i2c@82000: 'syscon' is a required property
	arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml

i2c@83000: 'syscon' is a required property
	arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml

i2c@84000: 'syscon' is a required property
	arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml

i2c@85000: 'syscon' is a required property
	arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml

i2c@86000: 'syscon' is a required property
	arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml

i2c@87000: 'syscon' is a required property
	arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml

i2c@88000: 'syscon' is a required property
	arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml

i2c@89000: 'syscon' is a required property
	arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml

i2c@8a000: 'syscon' is a required property
	arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml

i2c@8b000: 'syscon' is a required property
	arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml

i2c@8c000: 'syscon' is a required property
	arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml

i2c@8d000: 'syscon' is a required property
	arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml

i2c@8e000: 'syscon' is a required property
	arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml

i2c@8f000: 'syscon' is a required property
	arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
	arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml

Re: [PATCH v1 1/6] dt-bindings: i2c: npcm: support NPCM845

[warp5tw]

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

Hi all:

Thank you for your comments and they will be addressed.

Regards,
Tyrone

Rob Herring <robh@kernel.org> 於 2022年2月7日 週一 下午10:22寫道：

> On Mon, 07 Feb 2022 14:33:33 +0800, Tyrone Ting wrote:
> > From: Tyrone Ting <kfting@nuvoton.com>
> >
> > This commit adds compatible and syscon description for NPCM845 i2c
> module.
> >
> > 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>
> > ---
> >  .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml     | 21 ++++++++++++++-----
> >  1 file changed, 16 insertions(+), 5 deletions(-)
> >
>
> Running 'make dtbs_check' with the schema in this patch gives the
> following warnings. Consider if they are expected or the schema is
> incorrect. These may not be new warnings.
>
> Note that it is not yet a requirement to have 0 warnings for dtbs_check.
> This will change in the future.
>
> Full log is available here: https://patchwork.ozlabs.org/patch/1589171
>
>
> i2c@80000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
>
> i2c@81000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@82000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@83000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@84000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@85000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@86000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@87000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@88000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@89000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@8a000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@8b000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@8c000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@8d000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@8e000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
>
> i2c@8f000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>
>

[-- Attachment #2: Type: text/html, Size: 6930 bytes --]

Re: [PATCH v1 1/6] dt-bindings: i2c: npcm: support NPCM845

[warp5tw]

Hi all:

Thank you for your comments and they will be addressed.

Regards,
Tyrone


Rob Herring <robh@kernel.org> 於 2022年2月7日 週一 下午10:22寫道：
>
> On Mon, 07 Feb 2022 14:33:33 +0800, Tyrone Ting wrote:
> > From: Tyrone Ting <kfting@nuvoton.com>
> >
> > This commit adds compatible and syscon description for NPCM845 i2c module.
> >
> > 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>
> > ---
> >  .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml     | 21 ++++++++++++++-----
> >  1 file changed, 16 insertions(+), 5 deletions(-)
> >
>
> Running 'make dtbs_check' with the schema in this patch gives the
> following warnings. Consider if they are expected or the schema is
> incorrect. These may not be new warnings.
>
> Note that it is not yet a requirement to have 0 warnings for dtbs_check.
> This will change in the future.
>
> Full log is available here: https://patchwork.ozlabs.org/patch/1589171
>
>
> i2c@80000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
>
> i2c@81000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@82000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@83000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@84000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@85000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@86000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@87000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@88000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@89000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@8a000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@8b000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@8c000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@8d000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-runbmc-olympus.dt.yaml
>
> i2c@8e000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gbs.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm750-evb.dt.yaml
>
> i2c@8f000: 'syscon' is a required property
>         arch/arm/boot/dts/nuvoton-npcm730-gsj.dt.yaml
>         arch/arm/boot/dts/nuvoton-npcm730-kudo.dt.yaml
>

Re: [PATCH v1 1/6] dt-bindings: i2c: npcm: support NPCM845

[Rob Herring]

On Mon, Feb 07, 2022 at 02:33:33PM +0800, Tyrone Ting wrote:
> From: Tyrone Ting <kfting@nuvoton.com>
> 
> This commit adds compatible and syscon description for NPCM845 i2c module.
> 
> 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>
> ---
>  .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml     | 21 ++++++++++++++-----
>  1 file changed, 16 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..05e58f44b03a 100644
> --- a/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
> +++ b/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
> @@ -7,17 +7,22 @@ $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.
> +  The NPCM7XX includes sixteen I2C bus controllers and the NPCM8XX includes
> +  twenty-seven I2C bus controllers. NPCM8XX controllers 24-26 are connected on I2C
> +  pins in parallel to controllers 8-10.

How many instances is really outside the scope of this binding. And I 
don't want to be updating this for every new SoC. So rework it to cover 
both chips and the next one.

> +  All controllers support both master and slave mode.
> +  Each controller can switch between master and slave at run time (i.e. IPMB mode).
> +  NPCM7XX I2C controller has two 16 byte HW FIFO for TX and RX and NPCM8XX I2C
> +  controller has two 32 byte HW FIFO for TX and RX.
>  
>  maintainers:
>    - Tali Perry <tali.perry1@gmail.com>
>  
>  properties:
>    compatible:
> -    const: nuvoton,npcm750-i2c
> +    enum:
> +      - nuvoton,npcm750-i2c
> +      - nuvoton,npcm845-i2c
>  
>    reg:
>      maxItems: 1
> @@ -36,11 +41,16 @@ properties:
>      default: 100000
>      enum: [100000, 400000, 1000000]
>  
> +  syscon:

nuvoton,sys-mgr or similar.

> +    $ref: "/schemas/types.yaml#/definitions/phandle"
> +    description: The phandle of system manager register node.
> +
>  required:
>    - compatible
>    - reg
>    - interrupts
>    - clocks
> +  - syscon
>  
>  allOf:
>    - $ref: /schemas/i2c/i2c-controller.yaml#
> @@ -57,6 +67,7 @@ examples:
>          clock-frequency = <100000>;
>          interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
>          compatible = "nuvoton,npcm750-i2c";
> +        syscon = <&gcr>;
>      };
>  
>  ...
> -- 
> 2.17.1
> 
> 

Re: [PATCH v1 1/6] dt-bindings: i2c: npcm: support NPCM845

[warp5tw]

Hi Rob:

Thank you for your comments and they will be addressed.

Regards,
Tyrone


Rob Herring <robh@kernel.org> 於 2022年2月12日 週六 上午12:14寫道：
>
> On Mon, Feb 07, 2022 at 02:33:33PM +0800, Tyrone Ting wrote:
> > From: Tyrone Ting <kfting@nuvoton.com>
> >
> > This commit adds compatible and syscon description for NPCM845 i2c module.
> >
> > 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>
> > ---
> >  .../bindings/i2c/nuvoton,npcm7xx-i2c.yaml     | 21 ++++++++++++++-----
> >  1 file changed, 16 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..05e58f44b03a 100644
> > --- a/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
> > +++ b/Documentation/devicetree/bindings/i2c/nuvoton,npcm7xx-i2c.yaml
> > @@ -7,17 +7,22 @@ $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.
> > +  The NPCM7XX includes sixteen I2C bus controllers and the NPCM8XX includes
> > +  twenty-seven I2C bus controllers. NPCM8XX controllers 24-26 are connected on I2C
> > +  pins in parallel to controllers 8-10.
>
> How many instances is really outside the scope of this binding. And I
> don't want to be updating this for every new SoC. So rework it to cover
> both chips and the next one.
>
> > +  All controllers support both master and slave mode.
> > +  Each controller can switch between master and slave at run time (i.e. IPMB mode).
> > +  NPCM7XX I2C controller has two 16 byte HW FIFO for TX and RX and NPCM8XX I2C
> > +  controller has two 32 byte HW FIFO for TX and RX.
> >
> >  maintainers:
> >    - Tali Perry <tali.perry1@gmail.com>
> >
> >  properties:
> >    compatible:
> > -    const: nuvoton,npcm750-i2c
> > +    enum:
> > +      - nuvoton,npcm750-i2c
> > +      - nuvoton,npcm845-i2c
> >
> >    reg:
> >      maxItems: 1
> > @@ -36,11 +41,16 @@ properties:
> >      default: 100000
> >      enum: [100000, 400000, 1000000]
> >
> > +  syscon:
>
> nuvoton,sys-mgr or similar.
>
> > +    $ref: "/schemas/types.yaml#/definitions/phandle"
> > +    description: The phandle of system manager register node.
> > +
> >  required:
> >    - compatible
> >    - reg
> >    - interrupts
> >    - clocks
> > +  - syscon
> >
> >  allOf:
> >    - $ref: /schemas/i2c/i2c-controller.yaml#
> > @@ -57,6 +67,7 @@ examples:
> >          clock-frequency = <100000>;
> >          interrupts = <GIC_SPI 64 IRQ_TYPE_LEVEL_HIGH>;
> >          compatible = "nuvoton,npcm750-i2c";
> > +        syscon = <&gcr>;
> >      };
> >
> >  ...
> > --
> > 2.17.1
> >
> >

[PATCH v1 2/6] 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.
Use syscon to access gcr, instead of "compatible".

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 | 18 +++++++-----------
 1 file changed, 7 insertions(+), 11 deletions(-)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 2ad166355ec9..ddeee6f53621 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(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
@@ -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;
@@ -2233,9 +2233,9 @@ 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;
 
 	bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
 	if (!bus)
@@ -2250,15 +2250,11 @@ 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, "syscon");
 	if (IS_ERR(gcr_regmap))
 		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);
@@ -2269,7 +2265,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

Re: [PATCH v1 2/6] i2c: npcm: Fix timeout calculation

[Krzysztof Kozlowski]

On 07/02/2022 07:33, Tyrone Ting wrote:
> From: Tali Perry <tali.perry1@gmail.com>
> 
> Use adap.timeout for timeout calculation instead of hard-coded
> value of 35ms.
> Use syscon to access gcr, instead of "compatible".

One commit = one fix/feature/logical change, so requirement of syscon
should be a separate commit.


Best regards,
Krzysztof

Re: [PATCH v1 2/6] i2c: npcm: Fix timeout calculation

[Jonathan Neuschäfer]

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

Hello,

On Mon, Feb 07, 2022 at 02:33:34PM +0800, Tyrone Ting wrote:
> From: Tali Perry <tali.perry1@gmail.com>
> 
> Use adap.timeout for timeout calculation instead of hard-coded
> value of 35ms.


> Use syscon to access gcr, instead of "compatible".

Please put the GCR/syscon change into a separate patch, because it is
not obvious from the commit title that such a change would happen in
this patch.

> 
> 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 | 18 +++++++-----------
>  1 file changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
> index 2ad166355ec9..ddeee6f53621 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(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
> @@ -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;

This seems unrelated to timeout calculation.

>  	bus->msgs = msgs;
>  	bus->msgs_num = num;
>  	bus->cmd_err = 0;
> @@ -2233,9 +2233,9 @@ 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;
>  
>  	bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
>  	if (!bus)
> @@ -2250,15 +2250,11 @@ 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, "syscon");
>  	if (IS_ERR(gcr_regmap))
>  		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);

I agree that clk_regmap can be removed, but I'd rather see it in a
separate patch, because it's unrelated to the timeout calculation.

> -
>  	bus->reg = devm_platform_ioremap_resource(pdev, 0);
>  	if (IS_ERR(bus->reg))
>  		return PTR_ERR(bus->reg);
> @@ -2269,7 +2265,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;


Best regards,
Jonathan

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

Re: [PATCH v1 2/6] i2c: npcm: Fix timeout calculation

[warp5tw]

Hi all:

Thank you for your comments and they will be addressed.

Regards,
Tyrone


Jonathan Neuschäfer <j.neuschaefer@gmx.net> 於 2022年2月7日 週一 下午7:27寫道：
>
> Hello,
>
> On Mon, Feb 07, 2022 at 02:33:34PM +0800, Tyrone Ting wrote:
> > From: Tali Perry <tali.perry1@gmail.com>
> >
> > Use adap.timeout for timeout calculation instead of hard-coded
> > value of 35ms.
>
>
> > Use syscon to access gcr, instead of "compatible".
>
> Please put the GCR/syscon change into a separate patch, because it is
> not obvious from the commit title that such a change would happen in
> this patch.
>
> >
> > 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 | 18 +++++++-----------
> >  1 file changed, 7 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
> > index 2ad166355ec9..ddeee6f53621 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(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
> > @@ -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;
>
> This seems unrelated to timeout calculation.
>
> >       bus->msgs = msgs;
> >       bus->msgs_num = num;
> >       bus->cmd_err = 0;
> > @@ -2233,9 +2233,9 @@ 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;
> >
> >       bus = devm_kzalloc(&pdev->dev, sizeof(*bus), GFP_KERNEL);
> >       if (!bus)
> > @@ -2250,15 +2250,11 @@ 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, "syscon");
> >       if (IS_ERR(gcr_regmap))
> >               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);
>
> I agree that clk_regmap can be removed, but I'd rather see it in a
> separate patch, because it's unrelated to the timeout calculation.
>
> > -
> >       bus->reg = devm_platform_ioremap_resource(pdev, 0);
> >       if (IS_ERR(bus->reg))
> >               return PTR_ERR(bus->reg);
> > @@ -2269,7 +2265,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;
>
>
> Best regards,
> Jonathan

[PATCH v1 3/6] 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 ddeee6f53621..5c22e69afe34 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 v1 4/6] 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 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 | 96 +++++++++++++++++++++-----------
 1 file changed, 65 insertions(+), 31 deletions(-)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 5c22e69afe34..1ddf309b91a3 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)
@@ -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 v1 4/6] i2c: npcm: Handle spurious interrupts

[Jonathan Neuschäfer]

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

Hello,
some comments below.

On Mon, Feb 07, 2022 at 02:33:36PM +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 EOB after every interrupt.

For those who rarely deal with this particular I2C controller, please
add the meaning of EOB, e.g.: "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 | 96 +++++++++++++++++++++-----------
>  1 file changed, 65 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
> index 5c22e69afe34..1ddf309b91a3 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));
>  }

This change seems unrelated and isn't mentioned in the commit message.
Please remove it or put it into a separate commit, or document the
motivation in the commit message.


>  
>  static inline u16 npcm_i2c_get_index(struct npcm_i2c *bus)
> @@ -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);

Small nitpick: Please keep the order the same between comment and code,
e.g.:

	/* Clear BER, NEGACK and STASTR bits */
	val = NPCM_I2CST_BER | NPCM_I2CST_NEGACK | NPCM_I2CST_STASTR;



Best regards,
Jonathan

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

Re: [PATCH v1 4/6] i2c: npcm: Handle spurious interrupts

[warp5tw]

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

Hi Jonathan:

Thank you for your comments.

Regards,
Tyrone

Jonathan Neuschäfer <j.neuschaefer@gmx.net> 於 2022年2月7日 週一 下午7:40寫道：
>
> Hello,
> some comments below.
>
> On Mon, Feb 07, 2022 at 02:33:36PM +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 EOB after every interrupt.
>
> For those who rarely deal with this particular I2C controller, please
> add the meaning of EOB, e.g.: "2. Clear end of busy (EOB) after every interrupt"
>

Okay, it will be addressed.

> > 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 | 96 +++++++++++++++++++++-----------
> >  1 file changed, 65 insertions(+), 31 deletions(-)
> >
> > diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
> > index 5c22e69afe34..1ddf309b91a3 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));
> >  }
>
> This change seems unrelated and isn't mentioned in the commit message.
> Please remove it or put it into a separate commit, or document the
> motivation in the commit message.
>

Okay, it will be addressed.

>
> >
> >  static inline u16 npcm_i2c_get_index(struct npcm_i2c *bus)
> > @@ -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);
>
> Small nitpick: Please keep the order the same between comment and code,
> e.g.:
>
>         /* Clear BER, NEGACK and STASTR bits */
>         val = NPCM_I2CST_BER | NPCM_I2CST_NEGACK | NPCM_I2CST_STASTR;
>
>
>
> Best regards,
> Jonathan

[-- Attachment #2: signature.asc --]
[-- Type: text/plain, Size: 849 bytes --]

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEvHAHGBBjQPVy+qvDCDBEmo7zX9sFAmIBBP8ACgkQCDBEmo7z
X9srLw/9Fk0Dwv3OutJWngQjBucW7KWzadrpxsMsesyKne0BvaZUN7Dcw5Zsa9lz
vx9/xR8G/+KQNxcPFETqhHzCp9D2DLW/qTniAvocA+mACXPXRK7yA9+L2m8U58PS
QuKD1NVCtXXFziLr+4tT63xI13i28D8H9Jo1fTd+nPZgmnMeIFfNcDOeVOdNBV57
ovfJGZzFGSGR7RosMtknhf/vxIzLuu4lRqECrORFlGUVWi2pOghcElrdgY3g89CO
Fr7urdqkb7OHmGY+J9tvbIXC+fpA5W+LOlLq2C+x+qJf22T2MK9JwB9muY7DJov+
6ywzqUqQDbPVW5R7iXoN0VGXSvVdzwRV7xQm0L3gdQeiUNAkC6UkiCPtSgT9KAlm
sQCkTvSVUhP1Vo3u6jnp2kqnz8r7v8EgEdPQ0QkN2XS8lup1xlBnmXU95flzgJAj
asd1j5yB5sMkIomwugnMVZLpQaUVHEpls5ACaJXk5nmGWb3ymASO486n6d1U+BiW
EdaTohnLRD5NMXuJvtGIuCb6yZV2ggc5l1cHR/6deWp8Zre2CiJ7RELiIr5Blr1k
BfDLoS8ICOrADAfPaoVYVfhtHdjw+dicO67JA8XMIXG179Lzg7CY9yymvtk/HeA8
/hIA6sMn1M+PQLAn0HgKfo/fTaJ01CuljQYz7ud4JdKxkf+LTYY=
=/37B
-----END PGP SIGNATURE-----

[PATCH v1 5/6] 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.

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 | 44 +++++++++++++++++---------------
 1 file changed, 23 insertions(+), 21 deletions(-)

diff --git a/drivers/i2c/busses/i2c-npcm7xx.c b/drivers/i2c/busses/i2c-npcm7xx.c
index 1ddf309b91a3..0af97f179af2 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,
@@ -393,13 +395,11 @@ static void npcm_i2c_disable(struct npcm_i2c *bus)
 	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 +604,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 +613,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 +847,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 +863,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 +923,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 +979,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 v1 6/6] i2c: npcm: Support NPCM845

[Tyrone Ting]

From: Tyrone Ting <kfting@nuvoton.com>

NPCM8XX uses a similar i2c module as NPCM7XX.
The only difference is that the internal HW FIFO
is larger.

Related Makefile and Kconfig files are modified
to support as well.

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>
---
 drivers/i2c/busses/Kconfig       |  8 ++++----
 drivers/i2c/busses/Makefile      |  2 +-
 drivers/i2c/busses/i2c-npcm7xx.c | 29 +++++++++++++++++++++++++++++
 3 files changed, 34 insertions(+), 5 deletions(-)

diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig
index 42da31c1ab70..ab9ee2de5e00 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 0af97f179af2..4e0148ccc215 100644
--- a/drivers/i2c/busses/i2c-npcm7xx.c
+++ b/drivers/i2c/busses/i2c-npcm7xx.c
@@ -91,7 +91,11 @@ enum i2c_addr {
 
 /* init register and default value required to enable module */
 #define NPCM_I2CSEGCTL			0xE4
+#ifdef CONFIG_ARCH_NPCM7XX
 #define NPCM_I2CSEGCTL_INIT_VAL		0x0333F000
+#else
+#define NPCM_I2CSEGCTL_INIT_VAL		0x9333F000
+#endif
 
 /* Common regs */
 #define NPCM_I2CSDA			0x00
@@ -229,7 +233,11 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
 #define NPCM_I2CFIF_CTS_SLVRSTR		BIT(7)
 
 /* NPCM_I2CTXF_CTL reg fields */
+#ifdef CONFIG_ARCH_NPCM7XX
 #define NPCM_I2CTXF_CTL_TX_THR		GENMASK(4, 0)
+#else
+#define NPCM_I2CTXF_CTL_TX_THR		GENMASK(5, 0)
+#endif /*CONFIG_ARCH_NPCM7XX*/
 #define NPCM_I2CTXF_CTL_THR_TXIE	BIT(6)
 
 /* NPCM_I2CT_OUT reg fields */
@@ -238,22 +246,42 @@ static const int npcm_i2caddr[I2C_NUM_OWN_ADDR] = {
 #define NPCM_I2CT_OUT_T_OUTST		BIT(7)
 
 /* NPCM_I2CTXF_STS reg fields */
+#ifdef CONFIG_ARCH_NPCM7XX
 #define NPCM_I2CTXF_STS_TX_BYTES	GENMASK(4, 0)
+#else
+#define NPCM_I2CTXF_STS_TX_BYTES	GENMASK(5, 0)
+#endif /*CONFIG_ARCH_NPCM7XX*/
 #define NPCM_I2CTXF_STS_TX_THST		BIT(6)
 
 /* NPCM_I2CRXF_STS reg fields */
+#ifdef CONFIG_ARCH_NPCM7XX
 #define NPCM_I2CRXF_STS_RX_BYTES	GENMASK(4, 0)
+#else
+#define NPCM_I2CRXF_STS_RX_BYTES	GENMASK(5, 0)
+#endif /*CONFIG_ARCH_NPCM7XX*/
 #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 */
+#ifdef CONFIG_ARCH_NPCM7XX
 #define NPCM_I2CRXF_CTL_RX_THR		GENMASK(4, 0)
+#else
+#define NPCM_I2CRXF_CTL_RX_THR		GENMASK(5, 0)
+#endif /*CONFIG_ARCH_NPCM7XX*/
+#ifdef CONFIG_ARCH_NPCM7XX
 #define NPCM_I2CRXF_CTL_LAST_PEC	BIT(5)
+#else
+#define NPCM_I2CRXF_CTL_LAST_PEC	BIT(7)
+#endif /*CONFIG_ARCH_NPCM7XX*/
 #define NPCM_I2CRXF_CTL_THR_RXIE	BIT(6)
 
+#ifdef CONFIG_ARCH_NPCM7XX
 #define I2C_HW_FIFO_SIZE		16
+#else
+#define I2C_HW_FIFO_SIZE		32
+#endif /* CONFIG_ARCH_NPCM7XX */
 
 /* I2C_VER reg fields */
 #define I2C_VER_VERSION			GENMASK(6, 0)
@@ -2356,6 +2384,7 @@ static int npcm_i2c_remove_bus(struct platform_device *pdev)
 
 static const struct of_device_id npcm_i2c_bus_of_table[] = {
 	{ .compatible = "nuvoton,npcm750-i2c", },
+	{ .compatible = "nuvoton,npcm845-i2c", },
 	{}
 };
 MODULE_DEVICE_TABLE(of, npcm_i2c_bus_of_table);
-- 
2.17.1

Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845

[Jonathan Neuschäfer]

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

Hello,

On Mon, Feb 07, 2022 at 02:33:38PM +0800, Tyrone Ting wrote:
> From: Tyrone Ting <kfting@nuvoton.com>
> 
> NPCM8XX uses a similar i2c module as NPCM7XX.
> The only difference is that the internal HW FIFO
> is larger.
> 
> Related Makefile and Kconfig files are modified
> to support as well.
> 
> Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")

It's not really a bug fix, but rather an additional feature.
Therefore, I suggest removing the Fixes tag from this patch.

> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> Signed-off-by: Tali Perry <tali.perry1@gmail.com>
> ---
[...]
>  /* init register and default value required to enable module */
>  #define NPCM_I2CSEGCTL			0xE4
> +#ifdef CONFIG_ARCH_NPCM7XX
>  #define NPCM_I2CSEGCTL_INIT_VAL		0x0333F000
> +#else
> +#define NPCM_I2CSEGCTL_INIT_VAL		0x9333F000
> +#endif

This is going to cause problems when someone tries to compile a kernel
that runs on both NPCM7xx and NPCM8xx (because the driver will then only
work on NPCM7xx).

And every time another platform is added, this approach will make the
code less readable.

A more future-proof approach is probably to have a struct with chip-
specific data (such as the I2CSECCTL initialization value), which is
then selected via the .data field in of_device_id.


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

e.g.:

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


Best regards,
Jonathan

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

Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845

[Krzysztof Kozlowski]

On 07/02/2022 13:00, Jonathan Neuschäfer wrote:
> Hello,
> 
> On Mon, Feb 07, 2022 at 02:33:38PM +0800, Tyrone Ting wrote:
>> From: Tyrone Ting <kfting@nuvoton.com>
>>
>> NPCM8XX uses a similar i2c module as NPCM7XX.
>> The only difference is that the internal HW FIFO
>> is larger.
>>
>> Related Makefile and Kconfig files are modified
>> to support as well.
>>
>> Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller driver")
> 
> It's not really a bug fix, but rather an additional feature.
> Therefore, I suggest removing the Fixes tag from this patch.
> 
>> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
>> Signed-off-by: Tali Perry <tali.perry1@gmail.com>
>> ---
> [...]
>>  /* init register and default value required to enable module */
>>  #define NPCM_I2CSEGCTL			0xE4
>> +#ifdef CONFIG_ARCH_NPCM7XX
>>  #define NPCM_I2CSEGCTL_INIT_VAL		0x0333F000
>> +#else
>> +#define NPCM_I2CSEGCTL_INIT_VAL		0x9333F000
>> +#endif
> 
> This is going to cause problems when someone tries to compile a kernel
> that runs on both NPCM7xx and NPCM8xx (because the driver will then only
> work on NPCM7xx).

Yes, good catch.

The NPCM7XX is multiplatform, I guess NPCM8xx will be as well, so this
looks like an invalid code. How such code is supposed to work on
multiplatform kernel?

> 
> And every time another platform is added, this approach will make the
> code less readable.
> 
> A more future-proof approach is probably to have a struct with chip-
> specific data (such as the I2CSECCTL initialization value), which is
> then selected via the .data field in of_device_id.


Best regards,
Krzysztof

RE: [PATCH v1 6/6] i2c: npcm: Support NPCM845

[tali.perry]

>-----Original Message-----
>From: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
>Sent: Monday, February 7, 2022 5:27 PM
>To: Jonathan Neuschäfer <j.neuschaefer@gmx.net>; Tyrone Ting <warp5tw@gmail.com>
>Cc: avifishman70@gmail.com; tmaimon77@gmail.com; tali.perry1@gmail.com; venture@google.com; yuenn@google.com; benjaminfair@google.com; robh+dt@kernel.org; semen.protsenko@linaro.org; yangyicong@hisilicon.com; wsa@kernel.org; jie.deng@intel.com; sven@svenpeter.dev; bence98@sch.bme.hu; lukas.bulwahn@gmail.com; arnd@arndb.de; olof@lixom.net; andriy.shevchenko@linux.intel.com; IS20 Tali Perry <tali.perry@nuvoton.com>; IS20 Avi Fishman <Avi.Fishman@nuvoton.com>; IS20 Tomer Maimon <tomer.maimon@nuvoton.com>; CS20 KWLiu <KWLIU@nuvoton.com>; CS20 JJLiu0 <JJLIU0@nuvoton.com>; CS20 KFTing <KFTING@nuvoton.com>; devicetree@vger.kernel.org; openbmc@lists.ozlabs.org; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org
>Subject: Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845
>
>On 07/02/2022 13:00, Jonathan Neuschäfer wrote:
>> Hello,
>>
>> On Mon, Feb 07, 2022 at 02:33:38PM +0800, Tyrone Ting wrote:
>>> From: Tyrone Ting <kfting@nuvoton.com>
>>>
>>> NPCM8XX uses a similar i2c module as NPCM7XX.
>>> The only difference is that the internal HW FIFO is larger.
>>>
>>> Related Makefile and Kconfig files are modified to support as well.
>>>
>>> Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller
>>> driver")
>>
>> It's not really a bug fix, but rather an additional feature.
>> Therefore, I suggest removing the Fixes tag from this patch.
>>
>>> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
>>> Signed-off-by: Tali Perry <tali.perry1@gmail.com>
>>> ---
>> [...]
>>>  /* init register and default value required to enable module */
>>>  #define NPCM_I2CSEGCTL                     0xE4
>>> +#ifdef CONFIG_ARCH_NPCM7XX
>>>  #define NPCM_I2CSEGCTL_INIT_VAL            0x0333F000
>>> +#else
>>> +#define NPCM_I2CSEGCTL_INIT_VAL            0x9333F000
>>> +#endif
>>
>> This is going to cause problems when someone tries to compile a kernel
>> that runs on both NPCM7xx and NPCM8xx (because the driver will then
>> only work on NPCM7xx).
>
>Yes, good catch.
>
>The NPCM7XX is multiplatform, I guess NPCM8xx will be as well, so this looks like an invalid code. How such code is supposed to work on multiplatform kernel?
>


NPCM7xx and NPCM8xx are very different devices.
They share same driver sources for some of the modules but it's not ABI.
Users cannot compile a single kernel with two separate DTS.
In case of the i2c controller, the npcm7xx has a 16 byte HW FIFO,
and the NPCM8xx has 32 bytes HW FIFO.
This also means that registers fields are slightly different.
For init data we can move it to the DTS, but register field sizes
can't be handled with this approach.



>>
>> And every time another platform is added, this approach will make the
>> code less readable.
>>
>> A more future-proof approach is probably to have a struct with chip-
>> specific data (such as the I2CSECCTL initialization value), which is
>> then selected via the .data field in of_device_id.
>
>
>Best regards,
>Krzysztof

Thanks for the review!
Tali Perry, Nuvoton.
________________________________
________________________________
 The privileged confidential information contained in this email is intended for use only by the addressees as indicated by the original sender of this email. If you are not the addressee indicated in this email or are not responsible for delivery of the email to such a person, please kindly reply to the sender indicating this fact and delete all copies of it from your computer and network server immediately. Your cooperation is highly appreciated. It is advised that any unauthorized use of confidential information of Nuvoton is strictly prohibited; and any information in this email irrelevant to the official business of Nuvoton shall be deemed as neither given nor endorsed by Nuvoton.

Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845

[Tali Perry]

>-----Original Message-----
>From: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
>Sent: Monday, February 7, 2022 5:27 PM
>To: Jonathan Neuschäfer <j.neuschaefer@gmx.net>; Tyrone Ting <warp5tw@gmail.com>
>Cc: avifishman70@gmail.com; tmaimon77@gmail.com; tali.perry1@gmail.com; venture@google.com; yuenn@google.com; benjaminfair@google.com; robh+dt@kernel.org; semen.protsenko@linaro.org; yangyicong@hisilicon.com; wsa@kernel.org; jie.deng@intel.com; sven@svenpeter.dev; bence98@sch.bme.hu; lukas.bulwahn@gmail.com; arnd@arndb.de; olof@lixom.net; andriy.shevchenko@linux.intel.com; IS20 Tali Perry <tali.perry@nuvoton.com>; IS20 Avi Fishman <Avi.Fishman@nuvoton.com>; IS20 Tomer Maimon <tomer.maimon@nuvoton.com>; CS20 KWLiu <KWLIU@nuvoton.com>; CS20 JJLiu0 <JJLIU0@nuvoton.com>; CS20 KFTing <KFTING@nuvoton.com>; devicetree@vger.kernel.org; openbmc@lists.ozlabs.org; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org
>Subject: Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845
>
>On 07/02/2022 13:00, Jonathan Neuschäfer wrote:
>> Hello,
>>
>> On Mon, Feb 07, 2022 at 02:33:38PM +0800, Tyrone Ting wrote:
>>> From: Tyrone Ting <kfting@nuvoton.com>
>>>
>>> NPCM8XX uses a similar i2c module as NPCM7XX.
>>> The only difference is that the internal HW FIFO is larger.
>>>
>>> Related Makefile and Kconfig files are modified to support as well.
>>>
>>> Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller
>>> driver")
>>
>> It's not really a bug fix, but rather an additional feature.
>> Therefore, I suggest removing the Fixes tag from this patch.
>>
>>> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
>>> Signed-off-by: Tali Perry <tali.perry1@gmail.com>
>>> ---
>> [...]
>>>  /* init register and default value required to enable module */
>>>  #define NPCM_I2CSEGCTL 0xE4
>>> +#ifdef CONFIG_ARCH_NPCM7XX
>>>  #define NPCM_I2CSEGCTL_INIT_VAL 0x0333F000
>>> +#else
>>> +#define NPCM_I2CSEGCTL_INIT_VAL 0x9333F000
>>> +#endif
>>
>> This is going to cause problems when someone tries to compile a kernel
>> that runs on both NPCM7xx and NPCM8xx (because the driver will then
>> only work on NPCM7xx).
>
>Yes, good catch.
>
>The NPCM7XX is multiplatform, I guess NPCM8xx will be as well, so this looks like an invalid code. How such code is supposed to work on multiplatform kernel?
>

NPCM7xx and NPCM8xx are very different devices.
They share same driver sources for some of the modules but it's not ABI.
Users cannot compile a single kernel with two separate DTS.
In case of the i2c controller, the npcm7xx has a 16 byte HW FIFO,
and the NPCM8xx has 32 bytes HW FIFO.
This also means that registers fields are slightly different.
For init data we can move it to the DTS, but register field sizes
can't be handled with this approach.



>>
>> And every time another platform is added, this approach will make the
>> code less readable.
>>
>> A more future-proof approach is probably to have a struct with chip-
>> specific data (such as the I2CSECCTL initialization value), which is
>> then selected via the .data field in of_device_id.
>
>
>Best regards,
>Krzysztof

Thanks for the review!
Tali Perry, Nuvoton.

Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845

[Krzysztof Kozlowski]

On 08/02/2022 08:14, Tali Perry wrote:
>> -----Original Message-----
>> From: Krzysztof Kozlowski <krzysztof.kozlowski@canonical.com>
>> Sent: Monday, February 7, 2022 5:27 PM
>> To: Jonathan Neuschäfer <j.neuschaefer@gmx.net>; Tyrone Ting <warp5tw@gmail.com>
>> Cc: avifishman70@gmail.com; tmaimon77@gmail.com; tali.perry1@gmail.com; venture@google.com; yuenn@google.com; benjaminfair@google.com; robh+dt@kernel.org; semen.protsenko@linaro.org; yangyicong@hisilicon.com; wsa@kernel.org; jie.deng@intel.com; sven@svenpeter.dev; bence98@sch.bme.hu; lukas.bulwahn@gmail.com; arnd@arndb.de; olof@lixom.net; andriy.shevchenko@linux.intel.com; IS20 Tali Perry <tali.perry@nuvoton.com>; IS20 Avi Fishman <Avi.Fishman@nuvoton.com>; IS20 Tomer Maimon <tomer.maimon@nuvoton.com>; CS20 KWLiu <KWLIU@nuvoton.com>; CS20 JJLiu0 <JJLIU0@nuvoton.com>; CS20 KFTing <KFTING@nuvoton.com>; devicetree@vger.kernel.org; openbmc@lists.ozlabs.org; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845
>>
>> On 07/02/2022 13:00, Jonathan Neuschäfer wrote:
>>> Hello,
>>>
>>> On Mon, Feb 07, 2022 at 02:33:38PM +0800, Tyrone Ting wrote:
>>>> From: Tyrone Ting <kfting@nuvoton.com>
>>>>
>>>> NPCM8XX uses a similar i2c module as NPCM7XX.
>>>> The only difference is that the internal HW FIFO is larger.
>>>>
>>>> Related Makefile and Kconfig files are modified to support as well.
>>>>
>>>> Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller
>>>> driver")
>>>
>>> It's not really a bug fix, but rather an additional feature.
>>> Therefore, I suggest removing the Fixes tag from this patch.
>>>
>>>> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
>>>> Signed-off-by: Tali Perry <tali.perry1@gmail.com>
>>>> ---
>>> [...]
>>>>  /* init register and default value required to enable module */
>>>>  #define NPCM_I2CSEGCTL 0xE4
>>>> +#ifdef CONFIG_ARCH_NPCM7XX
>>>>  #define NPCM_I2CSEGCTL_INIT_VAL 0x0333F000
>>>> +#else
>>>> +#define NPCM_I2CSEGCTL_INIT_VAL 0x9333F000
>>>> +#endif
>>>
>>> This is going to cause problems when someone tries to compile a kernel
>>> that runs on both NPCM7xx and NPCM8xx (because the driver will then
>>> only work on NPCM7xx).
>>
>> Yes, good catch.
>>
>> The NPCM7XX is multiplatform, I guess NPCM8xx will be as well, so this looks like an invalid code. How such code is supposed to work on multiplatform kernel?
>>
> 
> NPCM7xx and NPCM8xx are very different devices.
> They share same driver sources for some of the modules but it's not ABI.
> Users cannot compile a single kernel with two separate DTS.
> In case of the i2c controller, the npcm7xx has a 16 byte HW FIFO,
> and the NPCM8xx has 32 bytes HW FIFO.
> This also means that registers fields are slightly different.
> For init data we can move it to the DTS, but register field sizes
> can't be handled with this approach.
> 

What do you mean they cannot compile a kernel with different DTS? Of
course they can - when we talk about multiplatform sub-architectures!
Maybe there is something specific in NPCMxxx which stops it but then it
should not be marked multiplatform.


Best regards,
Krzysztof

Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845

[Tali Perry]

>On 08/02/2022 08:14, Tali Perry wrote:
>>> Subject: Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845
>>>
>>> On 07/02/2022 13:00, Jonathan Neuschäfer wrote:
>>>> Hello,
>>>>
>>>> On Mon, Feb 07, 2022 at 02:33:38PM +0800, Tyrone Ting wrote:
>>>>> From: Tyrone Ting <kfting@nuvoton.com>
>>>>>
>>>>> NPCM8XX uses a similar i2c module as NPCM7XX.
>>>>> The only difference is that the internal HW FIFO is larger.
>>>>>
>>>>> Related Makefile and Kconfig files are modified to support as well.
>>>>>
>>>>> Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller
>>>>> driver")
>>>>
>>>> It's not really a bug fix, but rather an additional feature.
>>>> Therefore, I suggest removing the Fixes tag from this patch.
>>>>
>>>>> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
>>>>> Signed-off-by: Tali Perry <tali.perry1@gmail.com>
>>>>> ---
>>>> [...]
>>>>>  /* init register and default value required to enable module */
>>>>>  #define NPCM_I2CSEGCTL 0xE4
>>>>> +#ifdef CONFIG_ARCH_NPCM7XX
>>>>>  #define NPCM_I2CSEGCTL_INIT_VAL 0x0333F000
>>>>> +#else
>>>>> +#define NPCM_I2CSEGCTL_INIT_VAL 0x9333F000
>>>>> +#endif
>>>>
>>>> This is going to cause problems when someone tries to compile a kernel
>>>> that runs on both NPCM7xx and NPCM8xx (because the driver will then
>>>> only work on NPCM7xx).
>>>
>>> Yes, good catch.
>>>
>>> The NPCM7XX is multiplatform, I guess NPCM8xx will be as well, so this looks like an invalid code. How such code is supposed to work on multiplatform kernel?
>>>
>>
>> NPCM7xx and NPCM8xx are very different devices.
>> They share same driver sources for some of the modules but it's not ABI.
>> Users cannot compile a single kernel with two separate DTS.
>> In case of the i2c controller, the npcm7xx has a 16 byte HW FIFO,
>> and the NPCM8xx has 32 bytes HW FIFO.
>> This also means that registers fields are slightly different.
>> For init data we can move it to the DTS, but register field sizes
>> can't be handled with this approach.
>>
>
>What do you mean they cannot compile a kernel with different DTS? Of
>course they can - when we talk about multiplatform sub-architectures!
>Maybe there is something specific in NPCMxxx which stops it but then it
>should not be marked multiplatform.
>


NCPM7xx is ARM32 bit (dual core Cortex A9)
NPCM8xx is ARM64 bit (quad core Cortex A35)

They have completely different architecture so not ABI compliant.
I2C module is similar, but the devices are quite different and have
separate architectures.

Sorry for the confusion.
This is the first patch we try to upstream for NPCM8xx.
In the coming weeks we will upstream the architecture of NPCM8xx as well.



>
>Best regards,
>Krzysztof

Thanks!
Tali

Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845

[Krzysztof Kozlowski]

On 08/02/2022 09:51, Tali Perry wrote:
>> On 08/02/2022 08:14, Tali Perry wrote:
>>>> Subject: Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845
>>>>
>>>> On 07/02/2022 13:00, Jonathan Neuschäfer wrote:
>>>>> Hello,
>>>>>
>>>>> On Mon, Feb 07, 2022 at 02:33:38PM +0800, Tyrone Ting wrote:
>>>>>> From: Tyrone Ting <kfting@nuvoton.com>
>>>>>>
>>>>>> NPCM8XX uses a similar i2c module as NPCM7XX.
>>>>>> The only difference is that the internal HW FIFO is larger.
>>>>>>
>>>>>> Related Makefile and Kconfig files are modified to support as well.
>>>>>>
>>>>>> Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller
>>>>>> driver")
>>>>>
>>>>> It's not really a bug fix, but rather an additional feature.
>>>>> Therefore, I suggest removing the Fixes tag from this patch.
>>>>>
>>>>>> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
>>>>>> Signed-off-by: Tali Perry <tali.perry1@gmail.com>
>>>>>> ---
>>>>> [...]
>>>>>>  /* init register and default value required to enable module */
>>>>>>  #define NPCM_I2CSEGCTL 0xE4
>>>>>> +#ifdef CONFIG_ARCH_NPCM7XX
>>>>>>  #define NPCM_I2CSEGCTL_INIT_VAL 0x0333F000
>>>>>> +#else
>>>>>> +#define NPCM_I2CSEGCTL_INIT_VAL 0x9333F000
>>>>>> +#endif
>>>>>
>>>>> This is going to cause problems when someone tries to compile a kernel
>>>>> that runs on both NPCM7xx and NPCM8xx (because the driver will then
>>>>> only work on NPCM7xx).
>>>>
>>>> Yes, good catch.
>>>>
>>>> The NPCM7XX is multiplatform, I guess NPCM8xx will be as well, so this looks like an invalid code. How such code is supposed to work on multiplatform kernel?
>>>>
>>>
>>> NPCM7xx and NPCM8xx are very different devices.
>>> They share same driver sources for some of the modules but it's not ABI.
>>> Users cannot compile a single kernel with two separate DTS.
>>> In case of the i2c controller, the npcm7xx has a 16 byte HW FIFO,
>>> and the NPCM8xx has 32 bytes HW FIFO.
>>> This also means that registers fields are slightly different.
>>> For init data we can move it to the DTS, but register field sizes
>>> can't be handled with this approach.
>>>
>>
>> What do you mean they cannot compile a kernel with different DTS? Of
>> course they can - when we talk about multiplatform sub-architectures!
>> Maybe there is something specific in NPCMxxx which stops it but then it
>> should not be marked multiplatform.
>>
> 
> 
> NCPM7xx is ARM32 bit (dual core Cortex A9)
> NPCM8xx is ARM64 bit (quad core Cortex A35)
> 
> They have completely different architecture so not ABI compliant.
> I2C module is similar, but the devices are quite different and have
> separate architectures.

OK, in such case usually you indeed can't have both. :)

> Sorry for the confusion.
> This is the first patch we try to upstream for NPCM8xx.
> In the coming weeks we will upstream the architecture of NPCM8xx as well.

Still, ARCH_XXX should not be hard-coded in the drivers to change the
driver's behavior, even if driver won't be used simultaneously. It
breaks all design principles and prevents any further re-use if a new
use case appears.

You can use "ifdef ARCH_XXX" to skip building of some parts of the
driver, but it's not the case here.


Best regards,
Krzysztof

Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845

[Tali Perry]

>On 08/02/2022 09:51, Tali Perry wrote:
>>> On 08/02/2022 08:14, Tali Perry wrote:
>>>>> Subject: Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845
>>>>>
>>>>> On 07/02/2022 13:00, Jonathan Neuschäfer wrote:
>>>>>> Hello,
>>>>>>
>>>>>> On Mon, Feb 07, 2022 at 02:33:38PM +0800, Tyrone Ting wrote:
>>>>>>> From: Tyrone Ting <kfting@nuvoton.com>
>>>>>>>
>>>>>>> NPCM8XX uses a similar i2c module as NPCM7XX.
>>>>>>> The only difference is that the internal HW FIFO is larger.
>>>>>>>
>>>>>>> Related Makefile and Kconfig files are modified to support as well.
>>>>>>>
>>>>>>> Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller
>>>>>>> driver")
>>>>>>
>>>>>> It's not really a bug fix, but rather an additional feature.
>>>>>> Therefore, I suggest removing the Fixes tag from this patch.
>>>>>>
>>>>>>> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
>>>>>>> Signed-off-by: Tali Perry <tali.perry1@gmail.com>
>>>>>>> ---
>>>>>> [...]
>>>>>>>  /* init register and default value required to enable module */
>>>>>>>  #define NPCM_I2CSEGCTL 0xE4
>>>>>>> +#ifdef CONFIG_ARCH_NPCM7XX
>>>>>>>  #define NPCM_I2CSEGCTL_INIT_VAL 0x0333F000
>>>>>>> +#else
>>>>>>> +#define NPCM_I2CSEGCTL_INIT_VAL 0x9333F000
>>>>>>> +#endif
>>>>>>
>>>>>> This is going to cause problems when someone tries to compile a kernel
>>>>>> that runs on both NPCM7xx and NPCM8xx (because the driver will then
>>>>>> only work on NPCM7xx).
>>>>>
>>>>> Yes, good catch.
>>>>>
>>>>> The NPCM7XX is multiplatform, I guess NPCM8xx will be as well, so this looks like an invalid code. How such code is supposed to work on multiplatform kernel?
>>>>>
>>>>
>>>> NPCM7xx and NPCM8xx are very different devices.
>>>> They share same driver sources for some of the modules but it's not ABI.
>>>> Users cannot compile a single kernel with two separate DTS.
>>>> In case of the i2c controller, the npcm7xx has a 16 byte HW FIFO,
>>>> and the NPCM8xx has 32 bytes HW FIFO.
>>>> This also means that registers fields are slightly different.
>>>> For init data we can move it to the DTS, but register field sizes
>>>> can't be handled with this approach.
>>>>
>>>
>>> What do you mean they cannot compile a kernel with different DTS? Of
>>> course they can - when we talk about multiplatform sub-architectures!
>>> Maybe there is something specific in NPCMxxx which stops it but then it
>>> should not be marked multiplatform.
>>>
>>
>>
>> NCPM7xx is ARM32 bit (dual core Cortex A9)
>> NPCM8xx is ARM64 bit (quad core Cortex A35)
>>
>> They have completely different architecture so not ABI compliant.
>> I2C module is similar, but the devices are quite different and have
>> separate architectures.
>
>OK, in such case usually you indeed can't have both. :)
>
>> Sorry for the confusion.
>> This is the first patch we try to upstream for NPCM8xx.
>> In the coming weeks we will upstream the architecture of NPCM8xx as well.
>
>Still, ARCH_XXX should not be hard-coded in the drivers to change the
>driver's behavior, even if driver won't be used simultaneously. It
>breaks all design principles and prevents any further re-use if a new
>use case appears.
>
>You can use "ifdef ARCH_XXX" to skip building of some parts of the
>driver, but it's not the case here.
>

Correct, the main change is in FIFO size:
+#ifdef CONFIG_ARCH_NPCM7XX
#define I2C_HW_FIFO_SIZE               16
+#else
+#define I2C_HW_FIFO_SIZE               32
+#endif /* CONFIG_ARCH_NPCM7XX */

NPCM7XX will always have 16 bytes, all the next gens will have 32.

This impact some registers sizes, like this one:

+#ifdef CONFIG_ARCH_NPCM7XX
#define NPCM_I2CRXF_STS_RX_BYTES       GENMASK(4, 0)
+#else
+#define NPCM_I2CRXF_STS_RX_BYTES       GENMASK(5, 0)
+#endif /*CONFIG_ARCH_NPCM7XX*/

For this, the FIFO size should be defined before compilation.
I also don't want to let users select FIFO size per architecture.
NPCM7XX has 16, NPCM8XX has 32. This is not a user selection.
It's part of the arch.



>
>Best regards,
>Krzysztof

Thanks,
Tali

Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845

[Krzysztof Kozlowski]

On 08/02/2022 10:22, Tali Perry wrote:
>> On 08/02/2022 09:51, Tali Perry wrote:
>>>> On 08/02/2022 08:14, Tali Perry wrote:
>>>>>> Subject: Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845
>>>>>>
>>>>>> On 07/02/2022 13:00, Jonathan Neuschäfer wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> On Mon, Feb 07, 2022 at 02:33:38PM +0800, Tyrone Ting wrote:
>>>>>>>> From: Tyrone Ting <kfting@nuvoton.com>
>>>>>>>>
>>>>>>>> NPCM8XX uses a similar i2c module as NPCM7XX.
>>>>>>>> The only difference is that the internal HW FIFO is larger.
>>>>>>>>
>>>>>>>> Related Makefile and Kconfig files are modified to support as well.
>>>>>>>>
>>>>>>>> Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller
>>>>>>>> driver")
>>>>>>>
>>>>>>> It's not really a bug fix, but rather an additional feature.
>>>>>>> Therefore, I suggest removing the Fixes tag from this patch.
>>>>>>>
>>>>>>>> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
>>>>>>>> Signed-off-by: Tali Perry <tali.perry1@gmail.com>
>>>>>>>> ---
>>>>>>> [...]
>>>>>>>>  /* init register and default value required to enable module */
>>>>>>>>  #define NPCM_I2CSEGCTL 0xE4
>>>>>>>> +#ifdef CONFIG_ARCH_NPCM7XX
>>>>>>>>  #define NPCM_I2CSEGCTL_INIT_VAL 0x0333F000
>>>>>>>> +#else
>>>>>>>> +#define NPCM_I2CSEGCTL_INIT_VAL 0x9333F000
>>>>>>>> +#endif
>>>>>>>
>>>>>>> This is going to cause problems when someone tries to compile a kernel
>>>>>>> that runs on both NPCM7xx and NPCM8xx (because the driver will then
>>>>>>> only work on NPCM7xx).
>>>>>>
>>>>>> Yes, good catch.
>>>>>>
>>>>>> The NPCM7XX is multiplatform, I guess NPCM8xx will be as well, so this looks like an invalid code. How such code is supposed to work on multiplatform kernel?
>>>>>>
>>>>>
>>>>> NPCM7xx and NPCM8xx are very different devices.
>>>>> They share same driver sources for some of the modules but it's not ABI.
>>>>> Users cannot compile a single kernel with two separate DTS.
>>>>> In case of the i2c controller, the npcm7xx has a 16 byte HW FIFO,
>>>>> and the NPCM8xx has 32 bytes HW FIFO.
>>>>> This also means that registers fields are slightly different.
>>>>> For init data we can move it to the DTS, but register field sizes
>>>>> can't be handled with this approach.
>>>>>
>>>>
>>>> What do you mean they cannot compile a kernel with different DTS? Of
>>>> course they can - when we talk about multiplatform sub-architectures!
>>>> Maybe there is something specific in NPCMxxx which stops it but then it
>>>> should not be marked multiplatform.
>>>>
>>>
>>>
>>> NCPM7xx is ARM32 bit (dual core Cortex A9)
>>> NPCM8xx is ARM64 bit (quad core Cortex A35)
>>>
>>> They have completely different architecture so not ABI compliant.
>>> I2C module is similar, but the devices are quite different and have
>>> separate architectures.
>>
>> OK, in such case usually you indeed can't have both. :)
>>
>>> Sorry for the confusion.
>>> This is the first patch we try to upstream for NPCM8xx.
>>> In the coming weeks we will upstream the architecture of NPCM8xx as well.
>>
>> Still, ARCH_XXX should not be hard-coded in the drivers to change the
>> driver's behavior, even if driver won't be used simultaneously. It
>> breaks all design principles and prevents any further re-use if a new
>> use case appears.
>>
>> You can use "ifdef ARCH_XXX" to skip building of some parts of the
>> driver, but it's not the case here.
>>
> 
> Correct, the main change is in FIFO size:
> +#ifdef CONFIG_ARCH_NPCM7XX
> #define I2C_HW_FIFO_SIZE               16
> +#else
> +#define I2C_HW_FIFO_SIZE               32
> +#endif /* CONFIG_ARCH_NPCM7XX */
> 
> NPCM7XX will always have 16 bytes, all the next gens will have 32.
> 
> This impact some registers sizes, like this one:
> 
> +#ifdef CONFIG_ARCH_NPCM7XX
> #define NPCM_I2CRXF_STS_RX_BYTES       GENMASK(4, 0)
> +#else
> +#define NPCM_I2CRXF_STS_RX_BYTES       GENMASK(5, 0)
> +#endif /*CONFIG_ARCH_NPCM7XX*/
> 
> For this, the FIFO size should be defined before compilation.

No, it does not have to. We solved it numerous time with quirks or
per-chip-drvdata. It's common.

> I also don't want to let users select FIFO size per architecture.
> NPCM7XX has 16, NPCM8XX has 32. This is not a user selection.
> It's part of the arch.

I understand it is part of the architecture but why Nuvoton is different
than other architectures and requires special treatment here? With most
of the drivers, regardless of possibility of running same build on
different hardware, we strive to make it ifdef-independent.

Please run:
  git grep ARCH -- drivers/i2c/busses/*c
and see how many of such ifdef patterns you see. You also won't find
them if you grep for CONFIG...

The driver should be designed in portable way. The driver should not
have architecture-dependent code.

Best regards,
Krzysztof

Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845

[Avi Fishman]

On Tue, Feb 8, 2022 at 11:22 AM Tali Perry <tali.perry1@gmail.com> wrote:
>
> >On 08/02/2022 09:51, Tali Perry wrote:
> >>> On 08/02/2022 08:14, Tali Perry wrote:
> >>>>> Subject: Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845
> >>>>>
> >>>>> On 07/02/2022 13:00, Jonathan Neuschäfer wrote:
> >>>>>> Hello,
> >>>>>>
> >>>>>> On Mon, Feb 07, 2022 at 02:33:38PM +0800, Tyrone Ting wrote:
> >>>>>>> From: Tyrone Ting <kfting@nuvoton.com>
> >>>>>>>
> >>>>>>> NPCM8XX uses a similar i2c module as NPCM7XX.
> >>>>>>> The only difference is that the internal HW FIFO is larger.
> >>>>>>>
> >>>>>>> Related Makefile and Kconfig files are modified to support as well.
> >>>>>>>
> >>>>>>> Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller
> >>>>>>> driver")
> >>>>>>
> >>>>>> It's not really a bug fix, but rather an additional feature.
> >>>>>> Therefore, I suggest removing the Fixes tag from this patch.
> >>>>>>
> >>>>>>> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
> >>>>>>> Signed-off-by: Tali Perry <tali.perry1@gmail.com>
> >>>>>>> ---
> >>>>>> [...]
> >>>>>>>  /* init register and default value required to enable module */
> >>>>>>>  #define NPCM_I2CSEGCTL 0xE4
> >>>>>>> +#ifdef CONFIG_ARCH_NPCM7XX
> >>>>>>>  #define NPCM_I2CSEGCTL_INIT_VAL 0x0333F000
> >>>>>>> +#else
> >>>>>>> +#define NPCM_I2CSEGCTL_INIT_VAL 0x9333F000
> >>>>>>> +#endif
> >>>>>>
> >>>>>> This is going to cause problems when someone tries to compile a kernel
> >>>>>> that runs on both NPCM7xx and NPCM8xx (because the driver will then
> >>>>>> only work on NPCM7xx).
> >>>>>
> >>>>> Yes, good catch.
> >>>>>
> >>>>> The NPCM7XX is multiplatform, I guess NPCM8xx will be as well, so this looks like an invalid code. How such code is supposed to work on multiplatform kernel?
> >>>>>
> >>>>
> >>>> NPCM7xx and NPCM8xx are very different devices.
> >>>> They share same driver sources for some of the modules but it's not ABI.
> >>>> Users cannot compile a single kernel with two separate DTS.
> >>>> In case of the i2c controller, the npcm7xx has a 16 byte HW FIFO,
> >>>> and the NPCM8xx has 32 bytes HW FIFO.
> >>>> This also means that registers fields are slightly different.
> >>>> For init data we can move it to the DTS, but register field sizes
> >>>> can't be handled with this approach.
> >>>>
> >>>
> >>> What do you mean they cannot compile a kernel with different DTS? Of
> >>> course they can - when we talk about multiplatform sub-architectures!
> >>> Maybe there is something specific in NPCMxxx which stops it but then it
> >>> should not be marked multiplatform.
> >>>
> >>
> >>
> >> NCPM7xx is ARM32 bit (dual core Cortex A9)
> >> NPCM8xx is ARM64 bit (quad core Cortex A35)
> >>
> >> They have completely different architecture so not ABI compliant.
> >> I2C module is similar, but the devices are quite different and have
> >> separate architectures.
> >
> >OK, in such case usually you indeed can't have both. :)
> >
> >> Sorry for the confusion.
> >> This is the first patch we try to upstream for NPCM8xx.
> >> In the coming weeks we will upstream the architecture of NPCM8xx as well.
> >
> >Still, ARCH_XXX should not be hard-coded in the drivers to change the
> >driver's behavior, even if driver won't be used simultaneously. It
> >breaks all design principles and prevents any further re-use if a new
> >use case appears.
> >
> >You can use "ifdef ARCH_XXX" to skip building of some parts of the
> >driver, but it's not the case here.
> >
>
> Correct, the main change is in FIFO size:
> +#ifdef CONFIG_ARCH_NPCM7XX
> #define I2C_HW_FIFO_SIZE               16
> +#else
> +#define I2C_HW_FIFO_SIZE               32
> +#endif /* CONFIG_ARCH_NPCM7XX */
>
> NPCM7XX will always have 16 bytes, all the next gens will have 32.
>
> This impact some registers sizes, like this one:
>
> +#ifdef CONFIG_ARCH_NPCM7XX
> #define NPCM_I2CRXF_STS_RX_BYTES       GENMASK(4, 0)
> +#else
> +#define NPCM_I2CRXF_STS_RX_BYTES       GENMASK(5, 0)
> +#endif /*CONFIG_ARCH_NPCM7XX*/
>
> For this, the FIFO size should be defined before compilation.
> I also don't want to let users select FIFO size per architecture.
> NPCM7XX has 16, NPCM8XX has 32. This is not a user selection.
> It's part of the arch.

Will this be a better approach to add in the Kconfig file a legacy mode:
config I2C_NPCM_VER_89
      bool "support legacy i2c version 0x89 of npcm"
      depends on ARCH_NPCM7XX
      default y
      help
        This limits the support to legacy i2c version 0x89 of npcm
        Will be only 16 bytes fifo and will not include other new features

config I2C_NPCM
      tristate "Nuvoton I2C Controller"
      depends on ARCH_NPCM || COMPILE_TEST
      select I2C_NPCM_VER_89 if ARCH_NPCM7XX
      help
        If you say yes to this option, support will be included for the
        Nuvoton I2C controller, which is available on the NPCM BMC
        controllers.
        Driver can also support slave mode (select I2C_SLAVE).

and in the code replace:
#ifdef CONFIG_ARCH_NPCM7XX
with
#ifdef CONFIG_I2C_NPCM_VER_89

>
>
>
> >
> >Best regards,
> >Krzysztof
>
> Thanks,
> Tali



-- 
Regards,
Avi

Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845

[Krzysztof Kozlowski]

On 08/02/2022 10:31, Avi Fishman wrote:
> On Tue, Feb 8, 2022 at 11:22 AM Tali Perry <tali.perry1@gmail.com> wrote:
>>
>>> On 08/02/2022 09:51, Tali Perry wrote:
>>>>> On 08/02/2022 08:14, Tali Perry wrote:
>>>>>>> Subject: Re: [PATCH v1 6/6] i2c: npcm: Support NPCM845
>>>>>>>
>>>>>>> On 07/02/2022 13:00, Jonathan Neuschäfer wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> On Mon, Feb 07, 2022 at 02:33:38PM +0800, Tyrone Ting wrote:
>>>>>>>>> From: Tyrone Ting <kfting@nuvoton.com>
>>>>>>>>>
>>>>>>>>> NPCM8XX uses a similar i2c module as NPCM7XX.
>>>>>>>>> The only difference is that the internal HW FIFO is larger.
>>>>>>>>>
>>>>>>>>> Related Makefile and Kconfig files are modified to support as well.
>>>>>>>>>
>>>>>>>>> Fixes: 56a1485b102e ("i2c: npcm7xx: Add Nuvoton NPCM I2C controller
>>>>>>>>> driver")
>>>>>>>>
>>>>>>>> It's not really a bug fix, but rather an additional feature.
>>>>>>>> Therefore, I suggest removing the Fixes tag from this patch.
>>>>>>>>
>>>>>>>>> Signed-off-by: Tyrone Ting <kfting@nuvoton.com>
>>>>>>>>> Signed-off-by: Tali Perry <tali.perry1@gmail.com>
>>>>>>>>> ---
>>>>>>>> [...]
>>>>>>>>>  /* init register and default value required to enable module */
>>>>>>>>>  #define NPCM_I2CSEGCTL 0xE4
>>>>>>>>> +#ifdef CONFIG_ARCH_NPCM7XX
>>>>>>>>>  #define NPCM_I2CSEGCTL_INIT_VAL 0x0333F000
>>>>>>>>> +#else
>>>>>>>>> +#define NPCM_I2CSEGCTL_INIT_VAL 0x9333F000
>>>>>>>>> +#endif
>>>>>>>>
>>>>>>>> This is going to cause problems when someone tries to compile a kernel
>>>>>>>> that runs on both NPCM7xx and NPCM8xx (because the driver will then
>>>>>>>> only work on NPCM7xx).
>>>>>>>
>>>>>>> Yes, good catch.
>>>>>>>
>>>>>>> The NPCM7XX is multiplatform, I guess NPCM8xx will be as well, so this looks like an invalid code. How such code is supposed to work on multiplatform kernel?
>>>>>>>
>>>>>>
>>>>>> NPCM7xx and NPCM8xx are very different devices.
>>>>>> They share same driver sources for some of the modules but it's not ABI.
>>>>>> Users cannot compile a single kernel with two separate DTS.
>>>>>> In case of the i2c controller, the npcm7xx has a 16 byte HW FIFO,
>>>>>> and the NPCM8xx has 32 bytes HW FIFO.
>>>>>> This also means that registers fields are slightly different.
>>>>>> For init data we can move it to the DTS, but register field sizes
>>>>>> can't be handled with this approach.
>>>>>>
>>>>>
>>>>> What do you mean they cannot compile a kernel with different DTS? Of
>>>>> course they can - when we talk about multiplatform sub-architectures!
>>>>> Maybe there is something specific in NPCMxxx which stops it but then it
>>>>> should not be marked multiplatform.
>>>>>
>>>>
>>>>
>>>> NCPM7xx is ARM32 bit (dual core Cortex A9)
>>>> NPCM8xx is ARM64 bit (quad core Cortex A35)
>>>>
>>>> They have completely different architecture so not ABI compliant.
>>>> I2C module is similar, but the devices are quite different and have
>>>> separate architectures.
>>>
>>> OK, in such case usually you indeed can't have both. :)
>>>
>>>> Sorry for the confusion.
>>>> This is the first patch we try to upstream for NPCM8xx.
>>>> In the coming weeks we will upstream the architecture of NPCM8xx as well.
>>>
>>> Still, ARCH_XXX should not be hard-coded in the drivers to change the
>>> driver's behavior, even if driver won't be used simultaneously. It
>>> breaks all design principles and prevents any further re-use if a new
>>> use case appears.
>>>
>>> You can use "ifdef ARCH_XXX" to skip building of some parts of the
>>> driver, but it's not the case here.
>>>
>>
>> Correct, the main change is in FIFO size:
>> +#ifdef CONFIG_ARCH_NPCM7XX
>> #define I2C_HW_FIFO_SIZE               16
>> +#else
>> +#define I2C_HW_FIFO_SIZE               32
>> +#endif /* CONFIG_ARCH_NPCM7XX */
>>
>> NPCM7XX will always have 16 bytes, all the next gens will have 32.
>>
>> This impact some registers sizes, like this one:
>>
>> +#ifdef CONFIG_ARCH_NPCM7XX
>> #define NPCM_I2CRXF_STS_RX_BYTES       GENMASK(4, 0)
>> +#else
>> +#define NPCM_I2CRXF_STS_RX_BYTES       GENMASK(5, 0)
>> +#endif /*CONFIG_ARCH_NPCM7XX*/
>>
>> For this, the FIFO size should be defined before compilation.
>> I also don't want to let users select FIFO size per architecture.
>> NPCM7XX has 16, NPCM8XX has 32. This is not a user selection.
>> It's part of the arch.
> 
> Will this be a better approach to add in the Kconfig file a legacy mode:
> config I2C_NPCM_VER_89
>       bool "support legacy i2c version 0x89 of npcm"
>       depends on ARCH_NPCM7XX
>       default y
>       help
>         This limits the support to legacy i2c version 0x89 of npcm
>         Will be only 16 bytes fifo and will not include other new features
> 
> config I2C_NPCM
>       tristate "Nuvoton I2C Controller"
>       depends on ARCH_NPCM || COMPILE_TEST
>       select I2C_NPCM_VER_89 if ARCH_NPCM7XX
>       help
>         If you say yes to this option, support will be included for the
>         Nuvoton I2C controller, which is available on the NPCM BMC
>         controllers.
>         Driver can also support slave mode (select I2C_SLAVE).
> 
> and in the code replace:
> #ifdef CONFIG_ARCH_NPCM7XX
> with
> #ifdef CONFIG_I2C_NPCM_VER_89

This looks better but it is still the same approach - embedding ifdefs
to control the code flow and logic. Only naming is different. Whenever
you come with new I2C block for old or new architecture, you will have
the same problem. We solved such problems this already - quirks or
per-chip-drvdata - so here should be the same.

Even if the code does not end up in the same binary, it should be
designed properly. Because in the future it might end-up in same binary.

Everyone thought there will be no multiplatform ARMv7 so entire
arch/arm/mach-* was written exactly like that. And what? It was a lot of
effort to all of us to fix that mistakes. ARMv8 can work in 32-bit, so
who knows... Just do it properly.

Best regards,
Krzysztof