[net-next,1/3] dt-bindings: net: add dt bindings for marvell10g driver
diff mbox series

Message ID E1j7FqO-0003sv-Ho@rmk-PC.armlinux.org.uk
State New
Headers show
Series
  • Add support for configuring Marvell 10G PHY LEDs
Related show

Commit Message

Russell King Feb. 27, 2020, 9:52 a.m. UTC
Add a DT bindings document for the Marvell 10G driver, which will
augment the generic ethernet PHY binding by having LED mode
configuration.

Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
 .../devicetree/bindings/net/marvell,10g.yaml  | 31 +++++++++++++++++++
 1 file changed, 31 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/net/marvell,10g.yaml

Comments

Rob Herring Feb. 27, 2020, 5:08 p.m. UTC | #1
On Thu, 27 Feb 2020 09:52:36 +0000, Russell King wrote:
> Add a DT bindings document for the Marvell 10G driver, which will
> augment the generic ethernet PHY binding by having LED mode
> configuration.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  .../devicetree/bindings/net/marvell,10g.yaml  | 31 +++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/marvell,10g.yaml
> 

My bot found errors running 'make dt_binding_check' on your patch:

Documentation/devicetree/bindings/display/simple-framebuffer.example.dts:21.16-37.11: Warning (chosen_node_is_root): /example-0/chosen: chosen node must be at root node
Documentation/devicetree/bindings/net/marvell,10g.example.dts:18.13-23: Warning (reg_format): /example-0/ethernet-phy@0:reg: property has invalid length (4 bytes) (#address-cells == 1, #size-cells == 1)
Documentation/devicetree/bindings/net/marvell,10g.example.dt.yaml: Warning (pci_device_bus_num): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/net/marvell,10g.example.dt.yaml: Warning (i2c_bus_reg): Failed prerequisite 'reg_format'
Documentation/devicetree/bindings/net/marvell,10g.example.dt.yaml: Warning (spi_bus_reg): Failed prerequisite 'reg_format'

See https://patchwork.ozlabs.org/patch/1245687
Please check and re-submit.
Rob Herring Feb. 27, 2020, 5:13 p.m. UTC | #2
On Thu, Feb 27, 2020 at 3:52 AM Russell King <rmk+kernel@armlinux.org.uk> wrote:
>
> Add a DT bindings document for the Marvell 10G driver, which will
> augment the generic ethernet PHY binding by having LED mode
> configuration.
>
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> ---
>  .../devicetree/bindings/net/marvell,10g.yaml  | 31 +++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/marvell,10g.yaml
>
> diff --git a/Documentation/devicetree/bindings/net/marvell,10g.yaml b/Documentation/devicetree/bindings/net/marvell,10g.yaml
> new file mode 100644
> index 000000000000..da597fc5314d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/marvell,10g.yaml
> @@ -0,0 +1,31 @@
> +# SPDX-License-Identifier: GPL-2.0+

Dual license new bindings please:

(GPL-2.0-only OR BSD-2-Clause)

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/marvell,10g.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell Alaska X family Ethernet PHYs
> +
> +maintainers:
> +  - Russell King <rmk+kernel@armlinux.org.uk>
> +
> +allOf:
> +  - $ref: ethernet-phy.yaml#
> +
> +properties:
> +  marvell,led-mode:
> +    description: |
> +      An array of one to four 16-bit integers to write to the PHY LED
> +      configuration registers.

This is for what to blink or turn on for? I thought we had something
generic for configuring PHY LEDs specifically?

> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint16-array
> +      - minItems: 1
> +        maxItems: 4
> +
> +examples:
> +  - |
> +    ethernet-phy@0 {
> +        reg = <0>;

This needs to be under an 'mdio' node with #address-cells and
#size-cells set correctly.

> +        compatible = "ethernet-phy-ieee802.3-c45";
> +        marvell,led-mode = /bits/ 16 <0x0129 0x095d 0x0855>;
> +    };
> --
> 2.20.1
>
Russell King - ARM Linux admin Feb. 27, 2020, 5:14 p.m. UTC | #3
On Thu, Feb 27, 2020 at 11:08:58AM -0600, Rob Herring wrote:
> On Thu, 27 Feb 2020 09:52:36 +0000, Russell King wrote:
> > Add a DT bindings document for the Marvell 10G driver, which will
> > augment the generic ethernet PHY binding by having LED mode
> > configuration.
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >  .../devicetree/bindings/net/marvell,10g.yaml  | 31 +++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/net/marvell,10g.yaml
> > 
> 
> My bot found errors running 'make dt_binding_check' on your patch:
> 
> Documentation/devicetree/bindings/display/simple-framebuffer.example.dts:21.16-37.11: Warning (chosen_node_is_root): /example-0/chosen: chosen node must be at root node
> Documentation/devicetree/bindings/net/marvell,10g.example.dts:18.13-23: Warning (reg_format): /example-0/ethernet-phy@0:reg: property has invalid length (4 bytes) (#address-cells == 1, #size-cells == 1)
> Documentation/devicetree/bindings/net/marvell,10g.example.dt.yaml: Warning (pci_device_bus_num): Failed prerequisite 'reg_format'
> Documentation/devicetree/bindings/net/marvell,10g.example.dt.yaml: Warning (i2c_bus_reg): Failed prerequisite 'reg_format'
> Documentation/devicetree/bindings/net/marvell,10g.example.dt.yaml: Warning (spi_bus_reg): Failed prerequisite 'reg_format'

It looks like your bot has made a mistake, or I don't understand the
error messages. It seems to be trying to treat the example as a PCI
device, but it isn't, it is a PHY device.

I don't think that's something I can fix, sorry.
Russell King - ARM Linux admin Feb. 27, 2020, 5:26 p.m. UTC | #4
On Thu, Feb 27, 2020 at 11:13:41AM -0600, Rob Herring wrote:
> On Thu, Feb 27, 2020 at 3:52 AM Russell King <rmk+kernel@armlinux.org.uk> wrote:
> >
> > Add a DT bindings document for the Marvell 10G driver, which will
> > augment the generic ethernet PHY binding by having LED mode
> > configuration.
> >
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> > ---
> >  .../devicetree/bindings/net/marvell,10g.yaml  | 31 +++++++++++++++++++
> >  1 file changed, 31 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/net/marvell,10g.yaml
> >
> > diff --git a/Documentation/devicetree/bindings/net/marvell,10g.yaml b/Documentation/devicetree/bindings/net/marvell,10g.yaml
> > new file mode 100644
> > index 000000000000..da597fc5314d
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/marvell,10g.yaml
> > @@ -0,0 +1,31 @@
> > +# SPDX-License-Identifier: GPL-2.0+
> 
> Dual license new bindings please:
> 
> (GPL-2.0-only OR BSD-2-Clause)
> 
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/net/marvell,10g.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Marvell Alaska X family Ethernet PHYs
> > +
> > +maintainers:
> > +  - Russell King <rmk+kernel@armlinux.org.uk>
> > +
> > +allOf:
> > +  - $ref: ethernet-phy.yaml#
> > +
> > +properties:
> > +  marvell,led-mode:
> > +    description: |
> > +      An array of one to four 16-bit integers to write to the PHY LED
> > +      configuration registers.
> 
> This is for what to blink or turn on for? I thought we had something
> generic for configuring PHY LEDs specifically?

I see nothing in ethernet-phy.yaml.

Yes, it configures which conditions cause the LED to illuminate and/or
blink, what blink rate and polarity of the pin.

> > +    allOf:
> > +      - $ref: /schemas/types.yaml#/definitions/uint16-array
> > +      - minItems: 1
> > +        maxItems: 4
> > +
> > +examples:
> > +  - |
> > +    ethernet-phy@0 {
> > +        reg = <0>;
> 
> This needs to be under an 'mdio' node with #address-cells and
> #size-cells set correctly.

I wish these things were documented somewhere... I'm pretty sure this
passed validation when I wrote it.
Andrew Lunn Feb. 27, 2020, 5:36 p.m. UTC | #5
> > > +    allOf:
> > > +      - $ref: /schemas/types.yaml#/definitions/uint16-array
> > > +      - minItems: 1
> > > +        maxItems: 4
> > > +
> > > +examples:
> > > +  - |
> > > +    ethernet-phy@0 {
> > > +        reg = <0>;
> > 
> > This needs to be under an 'mdio' node with #address-cells and
> > #size-cells set correctly.
> 
> I wish these things were documented somewhere... I'm pretty sure this
> passed validation when I wrote it.

Documentation/devicetree/bindings/net/mdio.yaml

Rob, is there a way to express the hierarchy between yaml files and
properties? Can we say that a phy, as defined by ethernet-phy.yaml
should always be inside an MDIO bus as defined in mdio.yaml?

Thanks
	Andrew
Russell King - ARM Linux admin Feb. 27, 2020, 5:40 p.m. UTC | #6
On Thu, Feb 27, 2020 at 06:36:36PM +0100, Andrew Lunn wrote:
> > > > +    allOf:
> > > > +      - $ref: /schemas/types.yaml#/definitions/uint16-array
> > > > +      - minItems: 1
> > > > +        maxItems: 4
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    ethernet-phy@0 {
> > > > +        reg = <0>;
> > > 
> > > This needs to be under an 'mdio' node with #address-cells and
> > > #size-cells set correctly.
> > 
> > I wish these things were documented somewhere... I'm pretty sure this
> > passed validation when I wrote it.
> 
> Documentation/devicetree/bindings/net/mdio.yaml

I'm not sure that makes it any more obvious.  Maybe it's obvious to
those who understand yaml, but for the rest of us, it isn't.

> Rob, is there a way to express the hierarchy between yaml files and
> properties? Can we say that a phy, as defined by ethernet-phy.yaml
> should always be inside an MDIO bus as defined in mdio.yaml?

and yes, it isn't even referenced from ethernet-phy.yaml, so how one
would know to even look there.
Florian Fainelli Feb. 27, 2020, 5:44 p.m. UTC | #7
On 2/27/20 1:52 AM, Russell King wrote:
> Add a DT bindings document for the Marvell 10G driver, which will
> augment the generic ethernet PHY binding by having LED mode
> configuration.
> 
> Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>

We have been kicking the ball for way too long but there really ought to
be a standardized binding to configure LED modes for a PHY. Something
that we previously discussed here without making much progress because
the LED maintainer was not involved:

http://patchwork.ozlabs.org/patch/1146609/
http://patchwork.ozlabs.org/patch/1146610/
http://patchwork.ozlabs.org/patch/1146611/
http://patchwork.ozlabs.org/patch/1146612/

What you are proposing here is just a plain configuration interface via
Device Tree, which is really borderline. It gets the job done, and it is
extremely easy to maintain and use because people just stick in their
register value in there, but boy, what a poor abstraction that is.

Maybe you can resume where Matthias left and improve upon his patch
series, if nothing else for the binding and PHY layer integration?

> ---
>  .../devicetree/bindings/net/marvell,10g.yaml  | 31 +++++++++++++++++++
>  1 file changed, 31 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/net/marvell,10g.yaml
> 
> diff --git a/Documentation/devicetree/bindings/net/marvell,10g.yaml b/Documentation/devicetree/bindings/net/marvell,10g.yaml
> new file mode 100644
> index 000000000000..da597fc5314d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/marvell,10g.yaml
> @@ -0,0 +1,31 @@
> +# SPDX-License-Identifier: GPL-2.0+
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/net/marvell,10g.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Marvell Alaska X family Ethernet PHYs
> +
> +maintainers:
> +  - Russell King <rmk+kernel@armlinux.org.uk>
> +
> +allOf:
> +  - $ref: ethernet-phy.yaml#
> +
> +properties:
> +  marvell,led-mode:
> +    description: |
> +      An array of one to four 16-bit integers to write to the PHY LED
> +      configuration registers.
> +    allOf:
> +      - $ref: /schemas/types.yaml#/definitions/uint16-array
> +      - minItems: 1
> +        maxItems: 4
> +
> +examples:
> +  - |
> +    ethernet-phy@0 {
> +        reg = <0>;
> +        compatible = "ethernet-phy-ieee802.3-c45";
> +        marvell,led-mode = /bits/ 16 <0x0129 0x095d 0x0855>;
> +    };
>
Russell King - ARM Linux admin Feb. 27, 2020, 6:59 p.m. UTC | #8
On Thu, Feb 27, 2020 at 09:44:35AM -0800, Florian Fainelli wrote:
> On 2/27/20 1:52 AM, Russell King wrote:
> > Add a DT bindings document for the Marvell 10G driver, which will
> > augment the generic ethernet PHY binding by having LED mode
> > configuration.
> > 
> > Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
> 
> We have been kicking the ball for way too long but there really ought to
> be a standardized binding to configure LED modes for a PHY. Something
> that we previously discussed here without making much progress because
> the LED maintainer was not involved:
> 
> http://patchwork.ozlabs.org/patch/1146609/
> http://patchwork.ozlabs.org/patch/1146610/
> http://patchwork.ozlabs.org/patch/1146611/
> http://patchwork.ozlabs.org/patch/1146612/
> 
> What you are proposing here is just a plain configuration interface via
> Device Tree, which is really borderline. It gets the job done, and it is
> extremely easy to maintain and use because people just stick in their
> register value in there, but boy, what a poor abstraction that is.
> 
> Maybe you can resume where Matthias left and improve upon his patch
> series, if nothing else for the binding and PHY layer integration?

That series is way too simplistic, and would not allow for a
usable configuration for a four-speed PHY such as this one.

The proposed binding in those patches makes the assumption that
the only time that a LED shall blink is when there is traffic.

LED configuration is highly PHY specific.

For the 88x3310, we have around 31 different conditions that the LED
can blink for, or be solid for, the blink rate, and the polarity -
each LED is controlled by 13 bits in total, and then there's the "dual"
modes for bi-color LEDs which cause other of the LED configuration
registers to be ignored.  In other words, it's rather complex.

We could choose to limit the complexity, but then that risks making
it useless for certain boards - such as the Macchiatobin board, where
the dual modes can't be used due to the way the LEDs are wired - see
the last patch, where I describe how the LEDs are configured to
behave, which is the sanest organisation I could come up with which
doesn't result in mixing up various modes.


In any case, I do not wish to add to my patch backlog right now.  Maybe
when the backlog is smaller, I'll consider it, but not before.
Rob Herring Feb. 27, 2020, 8:11 p.m. UTC | #9
On Thu, Feb 27, 2020 at 11:36 AM Andrew Lunn <andrew@lunn.ch> wrote:
>
> > > > +    allOf:
> > > > +      - $ref: /schemas/types.yaml#/definitions/uint16-array
> > > > +      - minItems: 1
> > > > +        maxItems: 4
> > > > +
> > > > +examples:
> > > > +  - |
> > > > +    ethernet-phy@0 {
> > > > +        reg = <0>;
> > >
> > > This needs to be under an 'mdio' node with #address-cells and
> > > #size-cells set correctly.
> >
> > I wish these things were documented somewhere... I'm pretty sure this
> > passed validation when I wrote it.
>
> Documentation/devicetree/bindings/net/mdio.yaml
>
> Rob, is there a way to express the hierarchy between yaml files and
> properties? Can we say that a phy, as defined by ethernet-phy.yaml
> should always be inside an MDIO bus as defined in mdio.yaml?

We can link a child schema into a parent schema, but not the other way
around. So you can do something like this in mdio.yaml:

  "^ethernet-phy@[0-9a-f]+$":
    type: object
    allOf:
      - $ref: ethernet-phy.yaml#

That happens to work in this case since there's a common compatible
string for ethernet phys, but doesn't scale in the general case. Note
that ethernet-phy.yaml would need a couple of changes too. Also, this
should also be expanded to other possible node names like 'switch'.

I've had some thoughts of defining a pseudo property '$parent' or
something to be able to express constraints such as to what bus a
device has to be on. Currently, we rely on the overlap of the bus
schemas checking the bus specific aspects of the bus child nodes. I'm
also not really convinced that putting say an I2C device under a SPI
bus node is a problem we need to check for.


I'm not sure how any of this would help on examples compiling and
validating correctly. In example-schema.yaml, it mentions all the
problems I see: dtc fails, validation fails, bus node requirements,
and include file requirements:

  # Examples are now compiled with dtc and validated against the schemas
  #
  # Examples have a default #address-cells and #size-cells value of 1. This can
  # be overridden or an appropriate parent bus node should be shown (such as on
  # i2c buses).
  #
  # Any includes used have to be explicitly included.

Rob

Patch
diff mbox series

diff --git a/Documentation/devicetree/bindings/net/marvell,10g.yaml b/Documentation/devicetree/bindings/net/marvell,10g.yaml
new file mode 100644
index 000000000000..da597fc5314d
--- /dev/null
+++ b/Documentation/devicetree/bindings/net/marvell,10g.yaml
@@ -0,0 +1,31 @@ 
+# SPDX-License-Identifier: GPL-2.0+
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/net/marvell,10g.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Marvell Alaska X family Ethernet PHYs
+
+maintainers:
+  - Russell King <rmk+kernel@armlinux.org.uk>
+
+allOf:
+  - $ref: ethernet-phy.yaml#
+
+properties:
+  marvell,led-mode:
+    description: |
+      An array of one to four 16-bit integers to write to the PHY LED
+      configuration registers.
+    allOf:
+      - $ref: /schemas/types.yaml#/definitions/uint16-array
+      - minItems: 1
+        maxItems: 4
+
+examples:
+  - |
+    ethernet-phy@0 {
+        reg = <0>;
+        compatible = "ethernet-phy-ieee802.3-c45";
+        marvell,led-mode = /bits/ 16 <0x0129 0x095d 0x0855>;
+    };