| Message ID | 559a79756c9da9a7b188ffa141a2056a83c60de6.1543941145.git.kblaiech@mellanox.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: added driver for Mellanox BlueField SoC | expand |
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
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
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>; +};