On Wed, Nov 14, 2018 at 5:53 PM Khalil Blaiech <kblaiech@xxxxxxxxxxxx> 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@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?
'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
