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

Commit Message

Liming Sun Oct. 24, 2018, 5:55 p.m. UTC

Add devicetree bindings for the TmFifo which is found on Mellanox
BlueField SoCs.

Reviewed-by: Rob Herring <[e-mail]>
Reviewed-by: David Woods <[e-mail]>
Signed-off-by: Liming Sun <[e-mail]>
---
 .../devicetree/bindings/soc/mellanox/tmfifo.txt    | 23 ++++++++++++++++++++++
 1 file changed, 23 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/soc/mellanox/tmfifo.txt

Comments

Arnd Bergmann Oct. 25, 2018, 3:32 p.m. UTC | #1

On 10/24/18, Liming Sun <[e-mail]> wrote:
> Add devicetree bindings for the TmFifo which is found on Mellanox
> BlueField SoCs.
>
> Reviewed-by: Rob Herring <[e-mail]>
> Reviewed-by: David Woods <[e-mail]>
> Signed-off-by: Liming Sun <[e-mail]>
> ---
>  .../devicetree/bindings/soc/mellanox/tmfifo.txt    | 23
> ++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
>  create mode 100644
> Documentation/devicetree/bindings/soc/mellanox/tmfifo.txt
>
> diff --git a/Documentation/devicetree/bindings/soc/mellanox/tmfifo.txt
> b/Documentation/devicetree/bindings/soc/mellanox/tmfifo.txt
> new file mode 100644
> index 0000000..8a13fa6
> --- /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?

       Arnd

Liming Sun Oct. 26, 2018, 7:36 p.m. UTC | #2

Thanks Arnd for the comments! Please see the response inline.

- Liming

> -----Original Message-----
> From: [e-mail] [mailto:[e-mail]] On
> Behalf Of Arnd Bergmann
> Sent: Thursday, October 25, 2018 11:33 AM
> To: Liming Sun <[e-mail]>
> Cc: Olof Johansson <[e-mail]>; David Woods
> <[e-mail]>; Robin Murphy <[e-mail]>; arm-
> soc <[e-mail]>; [e-mail]; linux-arm-
> [e-mail]
> Subject: Re: [PATCH v4 3/4] dt-bindings: soc: Add TmFifo binding for Mellanox
> BlueField SoC
> 
> On 10/24/18, Liming Sun <[e-mail]> wrote:
> > Add devicetree bindings for the TmFifo which is found on Mellanox
> > BlueField SoCs.
> >
> > Reviewed-by: Rob Herring <[e-mail]>
> > Reviewed-by: David Woods <[e-mail]>
> > Signed-off-by: Liming Sun <[e-mail]>
> > ---
> >  .../devicetree/bindings/soc/mellanox/tmfifo.txt    | 23
> > ++++++++++++++++++++++
> >  1 file changed, 23 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/soc/mellanox/tmfifo.txt
> >
> > diff --git a/Documentation/devicetree/bindings/soc/mellanox/tmfifo.txt
> > b/Documentation/devicetree/bindings/soc/mellanox/tmfifo.txt
> > new file mode 100644
> > index 0000000..8a13fa6
> > --- /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?

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.
> 
>        Arnd

Arnd Bergmann Oct. 26, 2018, 8:33 p.m. UTC | #3

On Fri, Oct 26, 2018 at 9:36 PM Liming Sun <[e-mail]> wrote:
> > -----Original Message-----
> > From: [e-mail] [mailto:[e-mail]] 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

Liming Sun Oct. 29, 2018, 4:48 p.m. UTC | #4

Thanks for the comments! Please see my response/questions inline.

> -----Original Message-----
> From: Arnd Bergmann [mailto:[e-mail]]
> Sent: Friday, October 26, 2018 4:34 PM
> To: Liming Sun <[e-mail]>
> Cc: Olof Johansson <[e-mail]>; David Woods
> <[e-mail]>; Robin Murphy <[e-mail]>; arm-
> soc <[e-mail]>; DTML <[e-mail]>; Linux ARM
> <[e-mail]>
> 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 <[e-mail]> wrote:
> > > -----Original Message-----
> > > From: [e-mail] [mailto:[e-mail]] 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.

A little explanation for this SoC:
In the PCIe case, it's not just an endpoint. It can work in a PCIe "multi-host" mode
which behaves just like standalone system, such as doing PXE boot and CentOS 
or other OS installation on the eMMC and boot from it. It can run fully isolated 
from the x86 host. Below is a link of brief introduction.
http://www.mellanox.com/related-docs/prod_adapter_cards/PB_BlueField_Smart_NIC.pdf

So for now the same SW with ACPI configuration is used on all the boards for simplicity. 
But I think DT could definitely be used on customized board or when needed.

> 
> > 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.

We actually have more drivers coming for this SoC, such for I2C, GPIO, PKA, 
performance counter, L3 cache profile, etc, which could be found at the link below
https://git.launchpad.net/~dcwoods/ubuntu/+source/linux/+git/cosmic/log/?h=mellanox_bluefield

Since it's not just the FIFO driver, any suggestion which one would be better, 
the "soc" or still the 'platform' ? Thanks!

> 
>        Arnd

Liming Sun Jan. 24, 2019, 3:07 p.m. UTC | #5

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 <[e-mail]>
> Sent: Friday, October 26, 2018 4:34 PM
> To: Liming Sun <[e-mail]>
> Cc: Olof Johansson <[e-mail]>; David Woods <[e-mail]>; Robin Murphy <[e-mail]>; arm-soc
> <[e-mail]>; DTML <[e-mail]>; Linux ARM <[e-mail]>
> 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 <[e-mail]> wrote:
> > > -----Original Message-----
> > > From: [e-mail] [mailto:[e-mail]] 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

Patch

diff --git a/Documentation/devicetree/bindings/soc/mellanox/tmfifo.txt b/Documentation/devicetree/bindings/soc/mellanox/tmfifo.txt
new file mode 100644
index 0000000..8a13fa6
--- /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.
+
+Example:
+
+tmfifo@800a20 {
+	compatible = "mellanox,bf-tmfifo";
+	reg = <0x00800a20 0x00000018
+	       0x00800a40 0x00000018>;
+	interrupts = <41, 42, 43, 44>;
+};