| Message ID | 894b09ec9b4a18c91c3573145c59b8ff41c8ea04.1541174814.git.kblaiech@mellanox.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | i2c: added driver for Mellanox BlueField SoC | expand |
Rob, Thank you, for having taken the time to review this change and providing feedback. > On Fri, Nov 02, 2018 at 12:53:11PM -0400, 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-mlx.txt | 58 > ++++++++++++++++++++++ > > 1 file changed, 58 insertions(+) > > create mode 100644 > > Documentation/devicetree/bindings/i2c/mellanox,i2c-mlx.txt > > > > diff --git > > a/Documentation/devicetree/bindings/i2c/mellanox,i2c-mlx.txt > > b/Documentation/devicetree/bindings/i2c/mellanox,i2c-mlx.txt > > new file mode 100644 > > index 0000000..49c9e2f > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/i2c/mellanox,i2c-mlx.txt > > @@ -0,0 +1,58 @@ > > +Device tree configuration for the Mellanox I2C SMBus on BlueField > > +SoCs > > + > > +Required Properties: > > +- reg : address offset and range of bus device registers > > Need to enumerate the exact list. Okay. > > > +- compatible : should be "mellanox,i2c-mlx" > > Kind of generic. Please note that we do not provide identifier/serial number for our I2C device. I can think of "mellanox,i2c-mlxbf" instead where "bf" refers to our SoC. Is this less generic? > > > +- interrupts : interrupt number > > +- bus : hardware bus identifier. BlueField ARM side has three bus > > + controllers; thus, values might be 0, 1, or 2 > > We don't do bus indexes in DT. Actually, it is critical for us to know what bus we are using and/or exposing to user space; we rely on the bus identifier to match the hardware block. On the other hand, we don't want to put complex logic in the driver because it may require hardcoding memory region addresses. And we don't want to do that, right? So, maybe we should redefine it? And refer to with different property name... > > > +- profile : device profile. This ensures compatibility between actual > > + driver and ACPI/DT. The most recent profile is "mlnx-bf18" > > What? compatible is what that is for. Sorry! Totally agree. I'll get rid of it. > > > + > > +Optional Properties: > > +- bus-freq : bus frequency used to configure timing registers; allowed > > + values are 100, 400 and 1000; those are expressed in > > +KHz > > clock-frequency (in Hz) is the standard property for this. Okay > > > + > > +Examples: > > + > > +i2c0 { > > + compatible = "mellanox,i2c-mlx"; > > + reg = <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 */ > > Are these 2 really part of the I2C block? Our hardware design is quite complex, roughly, we have kind of centralized block which contains multiple sub-blocks. These regs are shared among the I2C controllers (i.e., sub-blocks). These are needed during device initialization. You'll also notice the "Cause Coalesce" above. > > > + interrupts = <57>; > > + bus = <1>; > > + bus-freq = <100>; > > + profile = "mlnx-bf18"; > > +}; > > + > > +i2c1 { > > These 2 examples don't add anything. Remove them. Okay. > > > + compatible = "mellanox,i2c-mlx"; > > + reg = <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 */ > > + interrupts = <57>; > > + bus = <1>; > > + bus-freq = <100>; > > + profile = "mlnx-bf18"; > > +}; > > + > > +i2c2 { > > + compatible = "mellanox,i2c-mlx"; > > + reg = <0x02805000 0x800>, /* Smbus[2] */ > > + <0x02801240 0x020>, /* Cause Master[2] */ > > + <0x028012a0 0x020>, /* Cause Slave[1] */ > > + <0x02801300 0x010>, /* Cause Coalesce */ > > + <0x02802000 0x100>, /* GPIO 0 */ > > + <0x02800358 0x008>; /* CorePll */ > > + interrupts = <57>; > > + bus = <2>; > > + bus-freq = <400>; > > + profile = "mlnx-bf18"; > > +}; > > -- > > 2.1.2 > > Regards, -Khalil
On Wed, Nov 14, 2018 at 5:53 PM Khalil Blaiech <kblaiech@mellanox.com> wrote: > > Rob, > > Thank you, for having taken the time to review this change and providing > feedback. > > > On Fri, Nov 02, 2018 at 12:53:11PM -0400, 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-mlx.txt | 58 > > ++++++++++++++++++++++ > > > 1 file changed, 58 insertions(+) > > > create mode 100644 > > > Documentation/devicetree/bindings/i2c/mellanox,i2c-mlx.txt > > > > > > diff --git > > > a/Documentation/devicetree/bindings/i2c/mellanox,i2c-mlx.txt > > > b/Documentation/devicetree/bindings/i2c/mellanox,i2c-mlx.txt > > > new file mode 100644 > > > index 0000000..49c9e2f > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/i2c/mellanox,i2c-mlx.txt > > > @@ -0,0 +1,58 @@ > > > +Device tree configuration for the Mellanox I2C SMBus on BlueField > > > +SoCs > > > + > > > +Required Properties: > > > +- reg : address offset and range of bus device registers > > > > Need to enumerate the exact list. > > Okay. > > > > > > +- compatible : should be "mellanox,i2c-mlx" > > > > Kind of generic. > > Please note that we do not provide identifier/serial number for our I2C > device. I can think of "mellanox,i2c-mlxbf" instead where "bf" refers to > our SoC. Is this less generic? > > > > > > +- interrupts : interrupt number > > > +- bus : hardware bus identifier. BlueField ARM side has three bus > > > + controllers; thus, values might be 0, 1, or 2 > > > > We don't do bus indexes in DT. > > Actually, it is critical for us to know what bus we are using and/or exposing > to user space; we rely on the bus identifier to match the hardware block. > On the other hand, we don't want to put complex logic in the driver because > it may require hardcoding memory region addresses. And we don't want to > do that, right? 'i2cN' alias should work for you. That's what others do. > > > +i2c0 { > > > + compatible = "mellanox,i2c-mlx"; > > > + reg = <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 */ > > > > Are these 2 really part of the I2C block? > > Our hardware design is quite complex, roughly, we have kind of > centralized block which contains multiple sub-blocks. These regs > are shared among the I2C controllers (i.e., sub-blocks). These > are needed during device initialization. > You'll also notice the "Cause Coalesce" above. Okay, as long as these addresses don't also appear somewhere else in the DT it is fine. It's not something we check currently, but I want to and we have to work-around the kernel's resource tracking because of that problem. Rob
diff --git a/Documentation/devicetree/bindings/i2c/mellanox,i2c-mlx.txt b/Documentation/devicetree/bindings/i2c/mellanox,i2c-mlx.txt new file mode 100644 index 0000000..49c9e2f --- /dev/null +++ b/Documentation/devicetree/bindings/i2c/mellanox,i2c-mlx.txt @@ -0,0 +1,58 @@ +Device tree configuration for the Mellanox I2C SMBus on BlueField SoCs + +Required Properties: +- reg : address offset and range of bus device registers +- compatible : should be "mellanox,i2c-mlx" +- interrupts : interrupt number +- bus : hardware bus identifier. BlueField ARM side has three bus + controllers; thus, values might be 0, 1, or 2 +- profile : device profile. This ensures compatibility between actual + driver and ACPI/DT. The most recent profile is "mlnx-bf18" + +Optional Properties: +- bus-freq : bus frequency used to configure timing registers; allowed + values are 100, 400 and 1000; those are expressed in KHz + +Examples: + +i2c0 { + compatible = "mellanox,i2c-mlx"; + reg = <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 */ + interrupts = <57>; + bus = <1>; + bus-freq = <100>; + profile = "mlnx-bf18"; +}; + +i2c1 { + compatible = "mellanox,i2c-mlx"; + reg = <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 */ + interrupts = <57>; + bus = <1>; + bus-freq = <100>; + profile = "mlnx-bf18"; +}; + +i2c2 { + compatible = "mellanox,i2c-mlx"; + reg = <0x02805000 0x800>, /* Smbus[2] */ + <0x02801240 0x020>, /* Cause Master[2] */ + <0x028012a0 0x020>, /* Cause Slave[1] */ + <0x02801300 0x010>, /* Cause Coalesce */ + <0x02802000 0x100>, /* GPIO 0 */ + <0x02800358 0x008>; /* CorePll */ + interrupts = <57>; + bus = <2>; + bus-freq = <400>; + profile = "mlnx-bf18"; +};