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@xxxxxxxxxxxx>
> > Signed-off-by: Khalil Blaiech <kblaiech@xxxxxxxxxxxx>
> > ---
> > .../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
