| Message ID | 20220920174736.9766-1-asmaa@nvidia.com |
|---|---|
| Headers | show |
| Series | i2c: i2c-mlxbf.c: bug fixes and new feature support | expand |
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.
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
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
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.
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
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
On Tue, Sep 20, 2022 at 01:47:29PM -0400, Asmaa Mnebhi wrote: > The i2c-mlxbf.c driver is currently broken because there is a bug > in the calculation of the frequency. core_f, core_r and core_od > are components read from hardware registers and are used to > compute the frequency used to compute different timing parameters. > The shifting mechanism used to get core_f, core_r and core_od is > wrong. Use FIELD_GET to mask and shift the bitfields properly. > > Fixes: b5b5b32081cd206b (i2c: mlxbf: I2C SMBus driver for Mellanox BlueField SoC) > Reviewed-by: Khalil Blaiech <kblaiech@nvidia.com> > Signed-off-by: Asmaa Mnebhi <asmaa@nvidia.com> Applied to for-current, thanks!
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=*
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=*
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
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.
> 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.
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(-)