[v2,2/2] dt-bindings: i2c: I2C binding for Mellanox BlueField SoC

Message ID 559a79756c9da9a7b188ffa141a2056a83c60de6.1543941145.git.kblaiech@mellanox.com
State Superseded
Headers show
Series
  • i2c: added driver for Mellanox BlueField SoC
Related show

Commit Message

Khalil Blaiech Dec. 4, 2018, 5:40 p.m.
Added device tree bindings documentation for Mellanox BlueField
I2C SMBus controller.

Reviewed-by: David Woods <dwoods@mellanox.com>
Signed-off-by: Khalil Blaiech <kblaiech@mellanox.com>
---
 .../devicetree/bindings/i2c/mellanox,i2c-mlxbf.txt | 67 ++++++++++++++++++++++
 1 file changed, 67 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/i2c/mellanox,i2c-mlxbf.txt

Comments

Rob Herring Dec. 18, 2018, 3:53 p.m. | #1
On Tue, Dec 04, 2018 at 12:40:58PM -0500, Khalil Blaiech wrote:
> Added device tree bindings documentation for Mellanox BlueField
> I2C SMBus controller.
> 
> Reviewed-by: David Woods <dwoods@mellanox.com>
> Signed-off-by: Khalil Blaiech <kblaiech@mellanox.com>
> ---
>  .../devicetree/bindings/i2c/mellanox,i2c-mlxbf.txt | 67 ++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/i2c/mellanox,i2c-mlxbf.txt
> 
> diff --git a/Documentation/devicetree/bindings/i2c/mellanox,i2c-mlxbf.txt b/Documentation/devicetree/bindings/i2c/mellanox,i2c-mlxbf.txt
> new file mode 100644
> index 0000000..61a8037
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/mellanox,i2c-mlxbf.txt
> @@ -0,0 +1,67 @@
> +Device tree configuration for the Mellanox I2C SMBus on BlueField SoCs
> +
> +Required Properties:
> +- reg        :	address offset and length of the device registers. The
> +		registers consists of a set of dedicated and shared
> +	       	resources:

mixed tab and spaces.

> +
> +			1: Smbus block registers (dedicated)
> +			2: Cause master registers (dedicated)
> +			3: Cause slave registers (dedicated)
> +			4: Cause coalesce registers (shared)
> +			5: GPIO 0 registers (shared)
> +			6: CorePLL registers (shared)
> +
> +		The BlueField SoCs includes three I2C bus controllers;
> +		the set of resources <address length> must be defined
> +		as follow:
> +
> +		i2c bus 0:
> +		    <0x02804000 0x800>	/* Smbus[0]        */
> +		    <0x02801200 0x020>	/* Cause Master[0] */
> +		    <0x02801260 0x020>	/* Cause Slave[0]  */
> +		    <0x02801300 0x010>	/* Cause Coalesce  */
> +		    <0x02802000 0x100>	/* GPIO 0          */
> +		    <0x02800358 0x008>	/* CorePll         */
> +
> +		i2c bus 1:
> +		    <0x02804800 0x800>	/* Smbus[1]        */
> +		    <0x02801220 0x020>	/* Cause Master[1] */
> +		    <0x02801280 0x020>	/* Cause Slave[1]  */

> +		    <0x02801300 0x010>	/* Cause Coalesce  */
> +		    <0x02802000 0x100>	/* GPIO 0          */
> +		    <0x02800358 0x008>	/* CorePll         */

You should not have overlapping register addresses in DT. These should 
be separate nodes.

Rob
Khalil Blaiech Jan. 30, 2019, 6:31 p.m. | #2
Rob,

Apologies for the late reply. It wasn't that easy to address your comments
and revisit the design of the driver. Please note that I applied the necessary
changes and submitted a V3.

> On Tue, Dec 04, 2018 at 12:40:58PM -0500, Khalil Blaiech wrote:
> > Added device tree bindings documentation for Mellanox BlueField I2C
> > SMBus controller.
> >
> > Reviewed-by: David Woods <dwoods@mellanox.com>
> > Signed-off-by: Khalil Blaiech <kblaiech@mellanox.com>
> > ---
> >  .../devicetree/bindings/i2c/mellanox,i2c-mlxbf.txt | 67
> > ++++++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/i2c/mellanox,i2c-mlxbf.txt
> >
> > diff --git
> > a/Documentation/devicetree/bindings/i2c/mellanox,i2c-mlxbf.txt
> > b/Documentation/devicetree/bindings/i2c/mellanox,i2c-mlxbf.txt
> > new file mode 100644
> > index 0000000..61a8037
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/i2c/mellanox,i2c-mlxbf.txt
> > @@ -0,0 +1,67 @@
> > +Device tree configuration for the Mellanox I2C SMBus on BlueField
> > +SoCs
> > +
> > +Required Properties:
> > +- reg        :	address offset and length of the device registers. The
> > +		registers consists of a set of dedicated and shared
> > +	       	resources:
> 
> mixed tab and spaces.

Removed the spaces and used tabs instead (sorry about that).

> 
> > +
> > +			1: Smbus block registers (dedicated)
> > +			2: Cause master registers (dedicated)
> > +			3: Cause slave registers (dedicated)
> > +			4: Cause coalesce registers (shared)
> > +			5: GPIO 0 registers (shared)
> > +			6: CorePLL registers (shared)
> > +
> > +		The BlueField SoCs includes three I2C bus controllers;
> > +		the set of resources <address length> must be defined
> > +		as follow:
> > +
> > +		i2c bus 0:
> > +		    <0x02804000 0x800>	/* Smbus[0]        */
> > +		    <0x02801200 0x020>	/* Cause Master[0] */
> > +		    <0x02801260 0x020>	/* Cause Slave[0]  */
> > +		    <0x02801300 0x010>	/* Cause Coalesce  */
> > +		    <0x02802000 0x100>	/* GPIO 0          */
> > +		    <0x02800358 0x008>	/* CorePll         */
> > +
> > +		i2c bus 1:
> > +		    <0x02804800 0x800>	/* Smbus[1]        */
> > +		    <0x02801220 0x020>	/* Cause Master[1] */
> > +		    <0x02801280 0x020>	/* Cause Slave[1]  */
> 
> > +		    <0x02801300 0x010>	/* Cause Coalesce  */
> > +		    <0x02802000 0x100>	/* GPIO 0          */
> > +		    <0x02800358 0x008>	/* CorePll         */
> 
> You should not have overlapping register addresses in DT. These should be
> separate nodes.

From our perspective, a separate node would make things more complicated
because this would imply a sophisticated probe routine for such simple device;
this may also require a deferral of the probe of the i2c controllers...
That being said, I believe the intuitive approach would be to implement a logic
within the driver code that maps the device memory and serialize the access to
the common registers. The only convenient of such approach is to keep the
common register definitions consistent across our platforms...

By the way, these definitions were initially based on the design of our hardware.

> 
> Rob

Regards,
Khalil

Patch

diff --git a/Documentation/devicetree/bindings/i2c/mellanox,i2c-mlxbf.txt b/Documentation/devicetree/bindings/i2c/mellanox,i2c-mlxbf.txt
new file mode 100644
index 0000000..61a8037
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/mellanox,i2c-mlxbf.txt
@@ -0,0 +1,67 @@ 
+Device tree configuration for the Mellanox I2C SMBus on BlueField SoCs
+
+Required Properties:
+- reg        :	address offset and length of the device registers. The
+		registers consists of a set of dedicated and shared
+	       	resources:
+
+			1: Smbus block registers (dedicated)
+			2: Cause master registers (dedicated)
+			3: Cause slave registers (dedicated)
+			4: Cause coalesce registers (shared)
+			5: GPIO 0 registers (shared)
+			6: CorePLL registers (shared)
+
+		The BlueField SoCs includes three I2C bus controllers;
+		the set of resources <address length> must be defined
+		as follow:
+
+		i2c bus 0:
+		    <0x02804000 0x800>	/* Smbus[0]        */
+		    <0x02801200 0x020>	/* Cause Master[0] */
+		    <0x02801260 0x020>	/* Cause Slave[0]  */
+		    <0x02801300 0x010>	/* Cause Coalesce  */
+		    <0x02802000 0x100>	/* GPIO 0          */
+		    <0x02800358 0x008>	/* CorePll         */
+
+		i2c bus 1:
+		    <0x02804800 0x800>	/* Smbus[1]        */
+		    <0x02801220 0x020>	/* Cause Master[1] */
+		    <0x02801280 0x020>	/* Cause Slave[1]  */
+		    <0x02801300 0x010>	/* Cause Coalesce  */
+		    <0x02802000 0x100>	/* GPIO 0          */
+		    <0x02800358 0x008>	/* CorePll         */
+
+		i2c bus 2:
+		    <0x02805000 0x800>	/* Smbus[2]        */
+		    <0x02801240 0x020>	/* Cause Master[2] */
+		    <0x028012a0 0x020>	/* Cause Slave[2]  */
+		    <0x02801300 0x010>	/* Cause Coalesce  */
+		    <0x02802000 0x100>	/* GPIO 0          */
+		    <0x02800358 0x008>	/* CorePll         */
+
+- compatible : 	should be "mellanox,i2c-mlxbf".
+- interrupts : 	interrupt number.
+
+Optional Properties:
+- clock-frequency :	bus frequency used to configure timing registers;
+  		    	allowed values are 100000, 400000 and 1000000;
+		    	those are expressed in Hz.
+
+Example:
+
+aliases {
+	i2c0 = &i2c_0
+};
+
+i2c_0: i2c {
+	compatible = "mellanox,i2c-mlxbf";
+	reg = <0x02804000 0x800>,
+	      <0x02801200 0x020>,
+	      <0x02801260 0x020>,
+	      <0x02801300 0x010>,
+	      <0x02802000 0x100>,
+	      <0x02800358 0x008>;
+	interrupts = <57>;
+	clock-frequency = <100000>;
+};