RE: [PATCH v4 3/4] dt-bindings: soc: Add TmFifo binding for Mellanox BlueField SoC

[Liming Sun <lsun@mellanox.com>]

Arnd,

According to the emails and discussions I saw recently, I think that your comments "Finally, drivers/platform/mellanox might be a reasonable choice, and it would let you keep both sides of the driver in one place." does make more sense.

I'll try to resubmit the changes under the drivers/platform/mellanox directory.

Thanks!
Liming

> -----Original Message-----
> From: Arnd Bergmann <arnd@arndb.de>
> Sent: Friday, October 26, 2018 4:34 PM
> To: Liming Sun <lsun@mellanox.com>
> Cc: Olof Johansson <olof@lixom.net>; David Woods <dwoods@mellanox.com>; Robin Murphy <robin.murphy@arm.com>; arm-soc
> <arm@kernel.org>; DTML <devicetree@vger.kernel.org>; Linux ARM <linux-arm-kernel@lists.infradead.org>
> Subject: Re: [PATCH v4 3/4] dt-bindings: soc: Add TmFifo binding for Mellanox BlueField SoC
> 
> On Fri, Oct 26, 2018 at 9:36 PM Liming Sun <lsun@mellanox.com> wrote:
> > > -----Original Message-----
> > > From: arndbergmann@gmail.com [mailto:arndbergmann@gmail.com] On
> > > > --- /dev/null
> > > > +++ b/Documentation/devicetree/bindings/soc/mellanox/tmfifo.txt
> > > > @@ -0,0 +1,23 @@
> > > > +* Mellanox BlueField SoC TmFifo
> > > > +
> > > > +BlueField TmFifo provides a shared FIFO between the target and the
> > > > +external host machine, which can be accessed by external host via
> > > > +USB or PCIe. In the current tmfifo driver, this FIFO has been demuxed
> > > > +to implement virtual console and network interface based on the virtio
> > > > +framework.
> > > > +
> > > > +Required properties:
> > > > +
> > > > +- compatible:      Should be "mellanox,bf-tmfifo"
> > > > +- reg:             Physical base address and length of Rx/Tx block
> > > > +- interrupts:      The interrupt number of Rx low water mark, Rx high water
> > > > mark
> > > > +           Tx low water mark, Tx high water mark respectively.
> > >
> > >
> > > This sounds like it might fit into the mailbox subsystem, and perhaps
> > > it should use the mailbox DT bindings. Have you had a look at that?
> >
> > This commit of dt-bindings is mainly to solve the warning of checkpatch.pl.
> > Like the response to patch 2/4, ACPI is actually used now instead of device tree.
> > The TMFIFO definition in the ACPI DSDT table would be something like below.
> >
> >     // RShim TMFIFO
> >     Device(RSH0) {
> >       Name(_HID, "MLNXBF01")
> >       Name(_UID, Zero)
> >       Name(_CCA, 1)
> >       Name(_CRS, ResourceTemplate() {
> >         Memory32Fixed(ReadWrite, 0x00800a20, 0x00000018)
> >         Memory32Fixed(ReadWrite, 0x00800a40, 0x00000018)
> >         Interrupt(ResourceConsumer, Edge, ActiveHigh, Exclusive)
> >           { BF1_RSH0_TM_HTT_LWM_INT,
> >             BF1_RSH0_TM_HTT_HWM_INT,
> >             BF1_RSH0_TM_TTH_LWM_INT,
> >             BF1_RSH0_TM_TTH_HWM_INT
> >           }
> >       })
> >     }
> >
> > Any suggestion how it should be added into Linux Documentation, or maybe I
> > should just remove this commit from this patch series?
> 
> Maybe the best way here would be to not use ACPI for the case
> where bluefin is integrated into a PCIe endpoint, since ACPI is
> not as flexible here and generally relies on having an SBSA
> compliant hardware that you no longer have if you require
> random platform devices for booting from and for your console.
> 
> For the case where a bluefin SoC is used in a standalone system,
> having ACPI makes more sense, as that lets you install Red Hat
> Linux or other operating systems that rely on SBBR and SBSA.
> 
> > As for the sub-component of this driver, the "soc" might be better fit than the mailbox
> > for some reasons. It's a communication between extern machines and the SoC via
> > USB / PCIe,  like pushing boot stream, console and network mgmt. Some of the features,
> > like pushing boot stream, doesn't communicate with the ARM core. The boot stream
> > is pushed to the SoC HW logic directly. I'll add the host-side virtio-based driver in patch v5.
> 
> Right, the drivers/mailbox subsystem was not the right idea here,
> I noticed that myself after actually reading the driver. Drivers/soc
> may also not be the best fit, since this is not really about it being
> a SoC, but rather a way to encapsulate virtual devices. The
> mic driver I mentioned is in drivers/misc, but I don't like to add stuff
> there if we can avoid it.
> 
> drivers/virtio, drivers/bus or drivers/mfd might also be an option that
> could fit better than drivers/soc, or you could have your own subdir
> below drivers/ as some others do. Finally, drivers/platform/mellanox
> might be a reasonable choice, and it would let you keep both sides
> of the driver in one place.
> 
>        Arnd