| Message ID | E1j7FqO-0003sv-Ho@rmk-PC.armlinux.org.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add support for configuring Marvell 10G PHY LEDs | expand |
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.
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 >
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.
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.
> > > + 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
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.
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>; > + }; >
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.
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
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>; + };
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