[PATCH v5 0/8] i2c: i2c-mlxbf.c: bug fixes and new feature support

[PATCH v5 0/8] i2c: i2c-mlxbf.c: bug fixes and new feature support

[Asmaa Mnebhi]

This is a series of patches fixing several bugs and implementing
new features.

Bug fixes:
1) Fix the frequency calculation
2) Fix incorrect base address passed during io write
3) prevent stack overflow in mlxbf_i2c_smbus_start_transaction()
4) Support lock mechanism to avoid race condition between entities
   using the i2c bus

Cleanup:
5) remove IRQF_ONESHOT flag as it is no longer needed.

Features:
6) Support multi slave functionality
7) Support BlueField-3 SoC
8) Update binding devicetree

What has changed from v4->v5:
Fix build error in the mellanox i2c device tree documentation

Asmaa Mnebhi (8):
  i2c: i2c-mlxbf.c: Fix frequency calculation
  i2c: i2c-mlxbf.c: remove IRQF_ONESHOT
  i2c: i2c-mlxbf.c: incorrect base address passed during io write
  i2c: i2c-mlxbf: prevent stack overflow in
    mlxbf_i2c_smbus_start_transaction()
  i2c: i2c-mlxbf.c: support lock mechanism
  i2c: i2c-mlxbf: add multi slave functionality
  i2c: i2c-mlxbf.c: support BlueField-3 SoC
  i2c: i2c-mlxbf.c: Update binding devicetree

 .../bindings/i2c/mellanox,i2c-mlxbf.yaml      |  48 +-
 MAINTAINERS                                   |   1 +
 drivers/i2c/busses/i2c-mlxbf.c                | 862 ++++++++++--------
 3 files changed, 521 insertions(+), 390 deletions(-)

-- 
2.30.1

[PATCH v5 8/8] i2c: i2c-mlxbf.c: Update binding devicetree

[Asmaa Mnebhi]

In the latest version of the i2c-mlxbf.c driver, the "Smbus block"
resource was broken down to 3 separate resources "Smbus timer",
"Smbus master" and "Smbus slave" to accommodate for BlueField-3
SoC registers' changes.

Reviewed-by: Khalil Blaiech <kblaiech@nvidia.com>
Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
---
 .../bindings/i2c/mellanox,i2c-mlxbf.yaml      | 48 ++++++++++++++-----
 1 file changed, 36 insertions(+), 12 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/mellanox,i2c-mlxbf.yaml b/Documentation/devicetree/bindings/i2c/mellanox,i2c-mlxbf.yaml
index 93198d5d43a6..24ab70c987fe 100644
--- a/Documentation/devicetree/bindings/i2c/mellanox,i2c-mlxbf.yaml
+++ b/Documentation/devicetree/bindings/i2c/mellanox,i2c-mlxbf.yaml
@@ -8,6 +8,7 @@ title: Mellanox I2C SMBus on BlueField SoCs
 
 maintainers:
   - Khalil Blaiech <kblaiech@nvidia.com>
+  - Asmaa Mnebhi <asmaa@nvidia.com>
 
 allOf:
   - $ref: /schemas/i2c/i2c-controller.yaml#
@@ -17,6 +18,7 @@ properties:
     enum:
       - mellanox,i2c-mlxbf1
       - mellanox,i2c-mlxbf2
+      - mellanox,i2c-mlxbf3
 
   reg:
     minItems: 3
@@ -25,6 +27,9 @@ properties:
       - description: Cause master registers
       - description: Cause slave registers
       - description: Cause coalesce registers
+      - description: Smbus timer registers
+      - description: Smbus master registers
+      - description: Smbus slave registers
 
   interrupts:
     maxItems: 1
@@ -35,6 +40,13 @@ properties:
       bus frequency used to configure timing registers;
       The frequency is expressed in Hz. Default is 100000.
 
+  resource_version:
+    enum: [ 0, 1 ]
+    description:
+      Version of the device tree. resource_version = 0 when the driver uses
+      Smbus block resource. resource_version = 1 when the driver uses Smbus
+      timer, Smbus master and Smbus slave resources.
+
 required:
   - compatible
   - reg
@@ -42,18 +54,6 @@ required:
 
 unevaluatedProperties: false
 
-if:
-  properties:
-    compatible:
-      contains:
-        enum:
-          - mellanox,i2c-mlxbf1
-
-then:
-  properties:
-    reg:
-      maxItems: 3
-
 examples:
   - |
     i2c@2804000 {
@@ -61,8 +61,13 @@ examples:
         reg = <0x02804000 0x800>,
               <0x02801200 0x020>,
               <0x02801260 0x020>;
+              <0x00000001 0x1>;
+              <0x02804000 0x40>,
+              <0x02804200 0x200>,
+              <0x02804400 0x200>,
         interrupts = <57>;
         clock-frequency = <100000>;
+        resource_version = <1>;
     };
 
   - |
@@ -72,6 +77,25 @@ examples:
               <0x02808e00 0x020>,
               <0x02808e20 0x020>,
               <0x02808e40 0x010>;
+              <0x02808800 0x040>;
+              <0x02808a00 0x200>,
+              <0x02808c00 0x200>,
         interrupts = <57>;
         clock-frequency = <400000>;
+        resource_version = <1>;
+    };
+
+  - |
+    i2c@2808800 {
+        compatible = "mellanox,i2c-mlxbf3";
+        reg = <0x00000001 0x1>,
+              <0x13404400 0x020>,
+              <0x13404420 0x020>,
+              <0x13404440 0x010>;
+              <0x13404480 0x40>,
+              <0x13404200 0x200>,
+              <0x13404000 0x200>,
+        interrupts = <35>;
+        clock-frequency = <400000>;
+        resource_version = <1>;
     };
-- 
2.30.1

Re: [PATCH v5 8/8] i2c: i2c-mlxbf.c: Update binding devicetree

[Krzysztof Kozlowski]

On Tue, 20 Sep 2022 13:47:36 -0400, Asmaa Mnebhi wrote:
> In the latest version of the i2c-mlxbf.c driver, the "Smbus block"
> resource was broken down to 3 separate resources "Smbus timer",
> "Smbus master" and "Smbus slave" to accommodate for BlueField-3
> SoC registers' changes.
> 
> Reviewed-by: Khalil Blaiech <kblaiech@nvidia.com>
> Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
> ---
>  .../bindings/i2c/mellanox,i2c-mlxbf.yaml      | 48 ++++++++++++++-----
>  1 file changed, 36 insertions(+), 12 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/i2c/mellanox,i2c-mlxbf.example.dts:26.19-20 syntax error
FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:384: Documentation/devicetree/bindings/i2c/mellanox,i2c-mlxbf.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1420: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch
series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above
error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v5 8/8] i2c: i2c-mlxbf.c: Update binding devicetree

[Krzysztof Kozlowski]

On 20/09/2022 19:47, Asmaa Mnebhi wrote:
> In the latest version of the i2c-mlxbf.c driver, the "Smbus block"
> resource was broken down to 3 separate resources "Smbus timer",
> "Smbus master" and "Smbus slave" to accommodate for BlueField-3
> SoC registers' changes.
> 
> Reviewed-by: Khalil Blaiech <kblaiech@nvidia.com>
> Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>

Use scripts/get_maintainers.pl to CC all maintainers and relevant
mailing lists.

You keep cc-ing other addresses or you rebased your patch on some old
tree (like 1 year old...). If this is the second case, please be sure it
is rebased on LATEST kernel, maintainer's tree or linux-next.

By not-ccing people, you will not get reviews from maintainers.

Best regards,
Krzysztof

Re: [PATCH v5 8/8] i2c: i2c-mlxbf.c: Update binding devicetree

[Krzysztof Kozlowski]

On 20/09/2022 19:47, Asmaa Mnebhi wrote:
> In the latest version of the i2c-mlxbf.c driver, the "Smbus block"
> resource was broken down to 3 separate resources "Smbus timer",
> "Smbus master" and "Smbus slave" to accommodate for BlueField-3
> SoC registers' changes.
> 
> Reviewed-by: Khalil Blaiech <kblaiech@nvidia.com>
> Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
> ---


>    reg:
>      minItems: 3
> @@ -25,6 +27,9 @@ properties:
>        - description: Cause master registers
>        - description: Cause slave registers
>        - description: Cause coalesce registers
> +      - description: Smbus timer registers
> +      - description: Smbus master registers
> +      - description: Smbus slave registers
>  
>    interrupts:
>      maxItems: 1
> @@ -35,6 +40,13 @@ properties:
>        bus frequency used to configure timing registers;
>        The frequency is expressed in Hz. Default is 100000.
>  
> +  resource_version:

No underscores in names.

> +    enum: [ 0, 1 ]
> +    description:
> +      Version of the device tree. resource_version = 0 when the driver uses
> +      Smbus block resource. resource_version = 1 when the driver uses Smbus
> +      timer, Smbus master and Smbus slave resources.

No way. That's not a DT property.

> +
>  required:
>    - compatible
>    - reg
> @@ -42,18 +54,6 @@ required:
>  
>  unevaluatedProperties: false
>  
> -if:
> -  properties:
> -    compatible:
> -      contains:
> -        enum:
> -          - mellanox,i2c-mlxbf1
> -
> -then:
> -  properties:
> -    reg:
> -      maxItems: 3

Why?

> -
>  examples:
>    - |
>      i2c@2804000 {
> @@ -61,8 +61,13 @@ examples:
>          reg = <0x02804000 0x800>,
>                <0x02801200 0x020>,
>                <0x02801260 0x020>;
> +              <0x00000001 0x1>;
> +              <0x02804000 0x40>,
> +              <0x02804200 0x200>,
> +              <0x02804400 0x200>,
>          interrupts = <57>;
>          clock-frequency = <100000>;
> +        resource_version = <1>;
>      };
>  
>    - |
> @@ -72,6 +77,25 @@ examples:
>                <0x02808e00 0x020>,
>                <0x02808e20 0x020>,
>                <0x02808e40 0x010>;
> +              <0x02808800 0x040>;
> +              <0x02808a00 0x200>,
> +              <0x02808c00 0x200>,
>          interrupts = <57>;
>          clock-frequency = <400000>;
> +        resource_version = <1>;
> +    };
> +
> +  - |
> +    i2c@2808800 {
> +        compatible = "mellanox,i2c-mlxbf3";
> +        reg = <0x00000001 0x1>,
> +              <0x13404400 0x020>,
> +              <0x13404420 0x020>,
> +              <0x13404440 0x010>;
> +              <0x13404480 0x40>,
> +              <0x13404200 0x200>,
> +              <0x13404000 0x200>,

No need for the same example.

Best regards,
Krzysztof

RE: [PATCH v5 8/8] i2c: i2c-mlxbf.c: Update binding devicetree

[Asmaa Mnebhi]

I have a question for you and Wolfram, we don’t use device trees and are not planning to use device trees; we only use ACPI tables. But I think when Khalil submitted the first version of the i2c-mlxbf.c driver, it was requested from him to add devicetree support. Do you know why? Is it possible to remove the device tree support and so this doc? or is devicetree support a requirement regardless of the actual implementation? 

-----Original Message-----
From: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> 
Sent: Wednesday, September 21, 2022 2:55 AM
To: Asmaa Mnebhi <asmaa@nvidia.com>
Cc: Wolfram Sang <wsa+renesas@sang-engineering.com>; linux-kernel@vger.kernel.org; robh@kernel.org; devicetree@vger.kernel.org; linux-i2c@vger.kernel.org; Khalil Blaiech <kblaiech@nvidia.com>
Subject: Re: [PATCH v5 8/8] i2c: i2c-mlxbf.c: Update binding devicetree

On Tue, 20 Sep 2022 13:47:36 -0400, Asmaa Mnebhi wrote:
> In the latest version of the i2c-mlxbf.c driver, the "Smbus block"
> resource was broken down to 3 separate resources "Smbus timer", "Smbus 
> master" and "Smbus slave" to accommodate for BlueField-3 SoC 
> registers' changes.
> 
> Reviewed-by: Khalil Blaiech <kblaiech@nvidia.com>
> Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com>
> ---
>  .../bindings/i2c/mellanox,i2c-mlxbf.yaml      | 48 ++++++++++++++-----
>  1 file changed, 36 insertions(+), 12 deletions(-)
> 

My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check'
on your patch (DT_CHECKER_FLAGS is new in v5.13):

yamllint warnings/errors:

dtschema/dtc warnings/errors:
Error: Documentation/devicetree/bindings/i2c/mellanox,i2c-mlxbf.example.dts:26.19-20 syntax error FATAL ERROR: Unable to parse input tree
make[1]: *** [scripts/Makefile.lib:384: Documentation/devicetree/bindings/i2c/mellanox,i2c-mlxbf.example.dtb] Error 1
make[1]: *** Waiting for unfinished jobs....
make: *** [Makefile:1420: dt_binding_check] Error 2

doc reference errors (make refcheckdocs):

See https://patchwork.ozlabs.org/patch/

This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1.

If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to
date:

pip3 install dtschema --upgrade

Please check and re-submit.

Re: [PATCH v5 8/8] i2c: i2c-mlxbf.c: Update binding devicetree

[Krzysztof Kozlowski]

On 21/09/2022 15:12, Asmaa Mnebhi wrote:
> I have a question for you and Wolfram, we don’t use device trees and are not planning to use device trees; we only use ACPI tables. But I think when Khalil submitted the first version of the i2c-mlxbf.c driver, it was requested from him to add devicetree support. Do you know why? Is it possible to remove the device tree support and so this doc? or is devicetree support a requirement regardless of the actual implementation? 

I don't think I am the right person to answer your question. I don't
know why you do things and did things in your driver. I also I do not
have any interest in your driver supporting anything. However if you do
support DT, I have interest in its correctness.

Best regards,
Krzysztof

RE: [PATCH v5 0/8] i2c: i2c-mlxbf.c: bug fixes and new feature support

[Asmaa Mnebhi]

Hi Wolfram,

Is there any requirement to support DTs in i2c drivers?
I would like to remove it in the v6 series because we don't support device tree, only ACPI.

Thank you,
Asmaa

-----Original Message-----
From: Asmaa Mnebhi <asmaa@nvidia.com> 
Sent: Tuesday, September 20, 2022 1:47 PM
To: Wolfram Sang <wsa+renesas@sang-engineering.com>; robh@kernel.org; linux-i2c@vger.kernel.org; linux-kernel@vger.kernel.org; devicetree@vger.kernel.org
Cc: Asmaa Mnebhi <asmaa@nvidia.com>
Subject: [PATCH v5 0/8] i2c: i2c-mlxbf.c: bug fixes and new feature support

This is a series of patches fixing several bugs and implementing new features.

Bug fixes:
1) Fix the frequency calculation
2) Fix incorrect base address passed during io write
3) prevent stack overflow in mlxbf_i2c_smbus_start_transaction()
4) Support lock mechanism to avoid race condition between entities
   using the i2c bus

Cleanup:
5) remove IRQF_ONESHOT flag as it is no longer needed.

Features:
6) Support multi slave functionality
7) Support BlueField-3 SoC
8) Update binding devicetree

What has changed from v4->v5:
Fix build error in the mellanox i2c device tree documentation

Asmaa Mnebhi (8):
  i2c: i2c-mlxbf.c: Fix frequency calculation
  i2c: i2c-mlxbf.c: remove IRQF_ONESHOT
  i2c: i2c-mlxbf.c: incorrect base address passed during io write
  i2c: i2c-mlxbf: prevent stack overflow in
    mlxbf_i2c_smbus_start_transaction()
  i2c: i2c-mlxbf.c: support lock mechanism
  i2c: i2c-mlxbf: add multi slave functionality
  i2c: i2c-mlxbf.c: support BlueField-3 SoC
  i2c: i2c-mlxbf.c: Update binding devicetree

 .../bindings/i2c/mellanox,i2c-mlxbf.yaml      |  48 +-
 MAINTAINERS                                   |   1 +
 drivers/i2c/busses/i2c-mlxbf.c                | 862 ++++++++++--------
 3 files changed, 521 insertions(+), 390 deletions(-)

--
2.30.1

How to remove DT support from a driver? (was Re: [PATCH v5 8/8] i2c: i2c-mlxbf.c: Update binding devicetree)

[Wolfram Sang]

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

Hi,

> I have a question for you and Wolfram, we don’t use device trees and
> are not planning to use device trees; we only use ACPI tables. But I
> think when Khalil submitted the first version of the i2c-mlxbf.c
> driver, it was requested from him to add devicetree support. Do you
> know why? Is it possible to remove the device tree support and so this
> doc? or is devicetree support a requirement regardless of the actual
> implementation? 

The first version sent from Khalil to the public I2C mailing list already
had DT bindings [1]. I don't see a sign of someone of the public list
requesting DT bindings. Maybe it was company internal?

Technically, there is no requirement to support DT, especially since you
have working ACPI. I don't know the process, though, of removing DT
support. You would basically need to be sure that no user made use of
the DT bindings introduced before. I don't know to what degree you can
assume that.

Maybe the DT list has more to add here?

Happy hacking,

   Wolfram

[1] http://patchwork.ozlabs.org/project/linux-i2c/list/?series=73827&state=*


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

RE: How to remove DT support from a driver? (was Re: [PATCH v5 8/8] i2c: i2c-mlxbf.c: Update binding devicetree)

[Asmaa Mnebhi]

Thanks for your reply Wolfram. All customers using BlueField hardware need to install our internal Firmware (proprietary) before booting any customized OS so they will always use ACPI tables. So I think it is safe to remove it. Any feedback from the DT list would be greatly appreciated! 

-----Original Message-----
From: Wolfram Sang <wsa+renesas@sang-engineering.com> 
Sent: Wednesday, September 21, 2022 4:02 PM
To: Asmaa Mnebhi <asmaa@nvidia.com>
Cc: Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>; linux-kernel@vger.kernel.org; robh@kernel.org; devicetree@vger.kernel.org; linux-i2c@vger.kernel.org; Khalil Blaiech <kblaiech@nvidia.com>
Subject: How to remove DT support from a driver? (was Re: [PATCH v5 8/8] i2c: i2c-mlxbf.c: Update binding devicetree)

Hi,

> I have a question for you and Wolfram, we don’t use device trees and 
> are not planning to use device trees; we only use ACPI tables. But I 
> think when Khalil submitted the first version of the i2c-mlxbf.c 
> driver, it was requested from him to add devicetree support. Do you 
> know why? Is it possible to remove the device tree support and so this 
> doc? or is devicetree support a requirement regardless of the actual 
> implementation?

The first version sent from Khalil to the public I2C mailing list already had DT bindings [1]. I don't see a sign of someone of the public list requesting DT bindings. Maybe it was company internal?

Technically, there is no requirement to support DT, especially since you have working ACPI. I don't know the process, though, of removing DT support. You would basically need to be sure that no user made use of the DT bindings introduced before. I don't know to what degree you can assume that.

Maybe the DT list has more to add here?

Happy hacking,

   Wolfram

[1] http://patchwork.ozlabs.org/project/linux-i2c/list/?series=73827&state=*

Re: How to remove DT support from a driver? (was Re: [PATCH v5 8/8] i2c: i2c-mlxbf.c: Update binding devicetree)

[Rob Herring]

On Wed, Sep 21, 2022 at 10:01:59PM +0200, Wolfram Sang wrote:
> Hi,
> 
> > I have a question for you and Wolfram, we don’t use device trees and
> > are not planning to use device trees; we only use ACPI tables. But I
> > think when Khalil submitted the first version of the i2c-mlxbf.c
> > driver, it was requested from him to add devicetree support. Do you
> > know why? Is it possible to remove the device tree support and so this
> > doc? or is devicetree support a requirement regardless of the actual
> > implementation? 
> 
> The first version sent from Khalil to the public I2C mailing list already
> had DT bindings [1]. I don't see a sign of someone of the public list
> requesting DT bindings. Maybe it was company internal?
> 
> Technically, there is no requirement to support DT, especially since you
> have working ACPI. I don't know the process, though, of removing DT
> support. You would basically need to be sure that no user made use of
> the DT bindings introduced before. I don't know to what degree you can
> assume that.

There's the whole using DT bindings in ACPI bindings thing, but I have 
little interest (or time) in supporting that. Maybe that's what's 
happening here? I haven't looked. The whole concept is flawed IMO. It 
may work for simple cases of key/value device properties, but the ACPI 
model is quite different in how resources are described and managed.

Rob

RE: How to remove DT support from a driver? (was Re: [PATCH v5 8/8] i2c: i2c-mlxbf.c: Update binding devicetree)

[Asmaa Mnebhi]

There's the whole using DT bindings in ACPI bindings thing, but I have little interest (or time) in supporting that. Maybe that's what's happening here? I haven't looked. The whole concept is flawed IMO. It may work for simple cases of key/value device properties, but the ACPI model is quite different in how resources are described and managed.

Hi Rob,
I chatted with Khalil and he confirmed that the reason he added the device tree support was to follow the example of existing upstreamed i2c drivers (even if we have no support for it and all our customers have to use our firmware including UEFI ACPI tables). So it is unrelated to DT bindings in ACPI bindings. He agrees that it is better to just remove the device tree support (from the driver itself and from the documentation). So if you don't have any objections, I will remove it.

Re: How to remove DT support from a driver? (was Re: [PATCH v5 8/8] i2c: i2c-mlxbf.c: Update binding devicetree)

[Wolfram Sang]

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


> i2c drivers (even if we have no support for it and all our customers
> have to use our firmware including UEFI ACPI tables). So it is

Because of the "custom firmware is needed and it is only ACPI" fact, I
think this is one of the rare cases where we can actually remove DT
support.


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