| Message ID | 20250403181907.1947517-2-sean.anderson@linux.dev (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add PCS core support | expand |
On Thu, Apr 03, 2025 at 02:18:55PM GMT, Sean Anderson wrote: > This adds a binding for the Xilinx 1G/2.5G Ethernet PCS/PMA or SGMII Incomplete review, since this is an RFC. Please do not use "This commit/patch/change", but imperative mood. See longer explanation here: https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 A nit, subject: drop second/last, redundant "binding for". The "dt-bindings" prefix is already stating that these are bindings. See also: https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 > LogiCORE IP. This device is a soft device typically used to adapt > between GMII and SGMII or 1000BASE-X (possbilty in combination with a > serdes). pcs-modes reflects the modes available with the as configured > when the device is synthesized. Multiple modes may be specified if > dynamic reconfiguration is supported. > > One PCS may contain "shared logic in core" which can be connected to > other PCSs with "shared logic in example design." This primarily refers > to clocking resources, allowing a reference clock to be shared by a bank > of PCSs. To support this, if #clock-cells is defined then the PCS will > register itself as a clock provider for other PCSs. > > Signed-off-by: Sean Anderson <sean.anderson@linux.dev> > --- > > .../devicetree/bindings/net/xilinx,pcs.yaml | 129 ++++++++++++++++++ > 1 file changed, 129 insertions(+) > create mode 100644 Documentation/devicetree/bindings/net/xilinx,pcs.yaml > > diff --git a/Documentation/devicetree/bindings/net/xilinx,pcs.yaml b/Documentation/devicetree/bindings/net/xilinx,pcs.yaml > new file mode 100644 > index 000000000000..56a3ce0c4ef0 > --- /dev/null > +++ b/Documentation/devicetree/bindings/net/xilinx,pcs.yaml > @@ -0,0 +1,129 @@ > +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) > +%YAML 1.2 > +--- > +$id: http://devicetree.org/schemas/net/xilinx,pcs.yaml# > +$schema: http://devicetree.org/meta-schemas/core.yaml# > + > +title: Xilinx 1G/2.5G Ethernet PCS/PMA or SGMII LogiCORE IP > + > +maintainers: > + - Sean Anderson <sean.anderson@seco.com> > + > +description: > + This is a soft device which implements the PCS and (depending on > + configuration) PMA layers of an IEEE Ethernet PHY. On the MAC side, it > + implements GMII. It may have an attached SERDES (internal or external), or > + may directly use LVDS IO resources. Depending on the configuration, it may > + implement 1000BASE-X, SGMII, 2500BASE-X, or 2.5G SGMII. > + > + This device has a notion of "shared logic" such as reset and clocking > + resources which must be shared between multiple PCSs using the same I/O > + banks. Each PCS can be configured to have the shared logic in the "core" > + (instantiated internally and made available to other PCSs) or in the "example > + design" (provided by another PCS). PCSs with shared logic in the core are > + reset controllers, and generally provide several resets for other PCSs in the > + same bank. > + > +allOf: > + - $ref: ethernet-controller.yaml# > + > +properties: > + compatible: > + contains: From where did you get such syntax? What do you want to express? > + const: xilinx,pcs-16.2 What does the number mean? > + > + reg: > + maxItems: 1 > + > + "#clock-cells": > + const: 0 > + description: > + Register a clock representing the clocking resources shared with other > + PCSs. Drop description, redundant. > + > + clocks: > + items: > + - description: > + The reference clock for the PCS. Depending on your setup, this may be > + the gtrefclk, refclk, clk125m signal, or clocks from another PCS. > + > + clock-names: > + const: refclk > + > + done-gpios: > + maxItems: 1 > + description: > + GPIO connected to the reset-done output, if present. > + > + interrupts: > + items: > + - description: > + The an_interrupt autonegotiation-complete interrupt. > + > + interrupt-names: > + const: an > + > + pcs-modes: > + description: > + The interfaces that the PCS supports. > + oneOf: > + - const: sgmii > + - const: 1000base-x > + - const: 2500base-x > + - items: > + - const: sgmii > + - const: 1000base-x This is confusing. Why fallbacks? Shouldn't this be just enum? And where is the type or constraints about number of items? > + > + reset-gpios: > + maxItems: 1 > + description: > + GPIO connected to the reset input. > + > +required: > + - compatible > + - reg > + - pcs-modes > + > +additionalProperties: false > + > +examples: > + - | > + #include <dt-bindings/gpio/gpio.h> > + #include <dt-bindings/interrupt-controller/arm-gic.h> > + #include <dt-bindings/interrupt-controller/irq.h> > + > + mdio { > + #address-cells = <1>; > + #size-cells = <0>; > + > + pcs0: ethernet-pcs@0 { > + #clock-cells = <0>; Follow DTS coding style. clock-cells are never the first property. > + compatible = "xlnx,pcs-16.2"; > + reg = <0>; > + clocks = <&si570>; > + clock-names = "refclk"; > + interrupts-extended = <&gic GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "an"; > + reset-gpios = <&gpio 5 GPIO_ACTIVE_HIGH>; > + done-gpios = <&gpio 6 GPIO_ACTIVE_HIGH>; > + pcs-modes = "sgmii", "1000base-x"; > + }; > + > + pcs1: ethernet-pcs@1 { > + compatible = "xlnx,pcs-16.2"; > + reg = <1>; > + clocks = <&pcs0>; > + clock-names = "refclk"; > + interrupts-extended = <&gic GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>; > + interrupt-names = "an"; > + reset-gpios = <&gpio 7 GPIO_ACTIVE_HIGH>; > + done-gpios = <&gpio 8 GPIO_ACTIVE_HIGH>; > + pcs-modes = "sgmii", "1000base-x"; Drop example, basically the same as previous. Best regards, Krzysztof
On 04/04/2025 12:37, Krzysztof Kozlowski wrote: >> + pcs-modes: >> + description: >> + The interfaces that the PCS supports. >> + oneOf: >> + - const: sgmii >> + - const: 1000base-x >> + - const: 2500base-x >> + - items: >> + - const: sgmii >> + - const: 1000base-x > > This is confusing. Why fallbacks? Shouldn't this be just enum? And > where is the type or constraints about number of items? > I just double checked now in dtschema and latest next - there is no such property. Best regards, Krzysztof
On 4/4/25 06:39, Krzysztof Kozlowski wrote: > On 04/04/2025 12:37, Krzysztof Kozlowski wrote: >>> + pcs-modes: >>> + description: >>> + The interfaces that the PCS supports. >>> + oneOf: >>> + - const: sgmii >>> + - const: 1000base-x >>> + - const: 2500base-x >>> + - items: >>> + - const: sgmii >>> + - const: 1000base-x >> >> This is confusing. Why fallbacks? Shouldn't this be just enum? And >> where is the type or constraints about number of items? >> > I just double checked now in dtschema and latest next - there is no such > property. OK, so you would prefer xlnx,pcs-modes? --Sean
On 4/4/25 06:37, Krzysztof Kozlowski wrote: > On Thu, Apr 03, 2025 at 02:18:55PM GMT, Sean Anderson wrote: >> This adds a binding for the Xilinx 1G/2.5G Ethernet PCS/PMA or SGMII > > Incomplete review, since this is an RFC. Only an RFC due to netdev's rules. I consider this patchset complete. > Please do not use "This commit/patch/change", but imperative mood. See > longer explanation here: > https://elixir.bootlin.com/linux/v5.17.1/source/Documentation/process/submitting-patches.rst#L95 > > A nit, subject: drop second/last, redundant "binding for". The > "dt-bindings" prefix is already stating that these are bindings. > See also: > https://elixir.bootlin.com/linux/v6.7-rc8/source/Documentation/devicetree/bindings/submitting-patches.rst#L18 > >> LogiCORE IP. This device is a soft device typically used to adapt >> between GMII and SGMII or 1000BASE-X (possbilty in combination with a >> serdes). pcs-modes reflects the modes available with the as configured >> when the device is synthesized. Multiple modes may be specified if >> dynamic reconfiguration is supported. >> >> One PCS may contain "shared logic in core" which can be connected to >> other PCSs with "shared logic in example design." This primarily refers >> to clocking resources, allowing a reference clock to be shared by a bank >> of PCSs. To support this, if #clock-cells is defined then the PCS will >> register itself as a clock provider for other PCSs. >> >> Signed-off-by: Sean Anderson <sean.anderson@linux.dev> >> --- >> >> .../devicetree/bindings/net/xilinx,pcs.yaml | 129 ++++++++++++++++++ >> 1 file changed, 129 insertions(+) >> create mode 100644 Documentation/devicetree/bindings/net/xilinx,pcs.yaml >> >> diff --git a/Documentation/devicetree/bindings/net/xilinx,pcs.yaml b/Documentation/devicetree/bindings/net/xilinx,pcs.yaml >> new file mode 100644 >> index 000000000000..56a3ce0c4ef0 >> --- /dev/null >> +++ b/Documentation/devicetree/bindings/net/xilinx,pcs.yaml >> @@ -0,0 +1,129 @@ >> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) >> +%YAML 1.2 >> +--- >> +$id: http://devicetree.org/schemas/net/xilinx,pcs.yaml# >> +$schema: http://devicetree.org/meta-schemas/core.yaml# >> + >> +title: Xilinx 1G/2.5G Ethernet PCS/PMA or SGMII LogiCORE IP >> + >> +maintainers: >> + - Sean Anderson <sean.anderson@seco.com> >> + >> +description: >> + This is a soft device which implements the PCS and (depending on >> + configuration) PMA layers of an IEEE Ethernet PHY. On the MAC side, it >> + implements GMII. It may have an attached SERDES (internal or external), or >> + may directly use LVDS IO resources. Depending on the configuration, it may >> + implement 1000BASE-X, SGMII, 2500BASE-X, or 2.5G SGMII. >> + >> + This device has a notion of "shared logic" such as reset and clocking >> + resources which must be shared between multiple PCSs using the same I/O >> + banks. Each PCS can be configured to have the shared logic in the "core" >> + (instantiated internally and made available to other PCSs) or in the "example >> + design" (provided by another PCS). PCSs with shared logic in the core are >> + reset controllers, and generally provide several resets for other PCSs in the >> + same bank. >> + >> +allOf: >> + - $ref: ethernet-controller.yaml# >> + >> +properties: >> + compatible: >> + contains: > > From where did you get such syntax? What do you want to express? The compatible should contain this value, but we don't really care what else it contains. This aligns with how the kernel matches drivers to devices. >> + const: xilinx,pcs-16.2 > > What does the number mean? It's the version of the IP. >> + >> + reg: >> + maxItems: 1 >> + >> + "#clock-cells": >> + const: 0 >> + description: >> + Register a clock representing the clocking resources shared with other >> + PCSs. > > Drop description, redundant. > >> + >> + clocks: >> + items: >> + - description: >> + The reference clock for the PCS. Depending on your setup, this may be >> + the gtrefclk, refclk, clk125m signal, or clocks from another PCS. >> + >> + clock-names: >> + const: refclk >> + >> + done-gpios: >> + maxItems: 1 >> + description: >> + GPIO connected to the reset-done output, if present. >> + >> + interrupts: >> + items: >> + - description: >> + The an_interrupt autonegotiation-complete interrupt. >> + >> + interrupt-names: >> + const: an >> + >> + pcs-modes: >> + description: >> + The interfaces that the PCS supports. >> + oneOf: >> + - const: sgmii >> + - const: 1000base-x >> + - const: 2500base-x >> + - items: >> + - const: sgmii >> + - const: 1000base-x > > This is confusing. Why fallbacks? Shouldn't this be just enum? And > where is the type or constraints about number of items? As stated in the commit message, multiple modes may be specified if dynamic reconfiguration is supported. So I want to allow pcs-modes = "sgmii" pcs-modes = "1000base-x" pcs-modes = "2500base-x" pcs-modes = "sgmii", "1000base-x" >> + >> + reset-gpios: >> + maxItems: 1 >> + description: >> + GPIO connected to the reset input. >> + >> +required: >> + - compatible >> + - reg >> + - pcs-modes >> + >> +additionalProperties: false >> + >> +examples: >> + - | >> + #include <dt-bindings/gpio/gpio.h> >> + #include <dt-bindings/interrupt-controller/arm-gic.h> >> + #include <dt-bindings/interrupt-controller/irq.h> >> + >> + mdio { >> + #address-cells = <1>; >> + #size-cells = <0>; >> + >> + pcs0: ethernet-pcs@0 { >> + #clock-cells = <0>; > > Follow DTS coding style. clock-cells are never the first property. Where is this documented? >> + compatible = "xlnx,pcs-16.2"; >> + reg = <0>; >> + clocks = <&si570>; >> + clock-names = "refclk"; >> + interrupts-extended = <&gic GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>; >> + interrupt-names = "an"; >> + reset-gpios = <&gpio 5 GPIO_ACTIVE_HIGH>; >> + done-gpios = <&gpio 6 GPIO_ACTIVE_HIGH>; >> + pcs-modes = "sgmii", "1000base-x"; >> + }; >> + >> + pcs1: ethernet-pcs@1 { >> + compatible = "xlnx,pcs-16.2"; >> + reg = <1>; >> + clocks = <&pcs0>; >> + clock-names = "refclk"; >> + interrupts-extended = <&gic GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>; >> + interrupt-names = "an"; >> + reset-gpios = <&gpio 7 GPIO_ACTIVE_HIGH>; >> + done-gpios = <&gpio 8 GPIO_ACTIVE_HIGH>; >> + pcs-modes = "sgmii", "1000base-x"; > > Drop example, basically the same as previous. > > Best regards, > Krzysztof >
diff --git a/Documentation/devicetree/bindings/net/xilinx,pcs.yaml b/Documentation/devicetree/bindings/net/xilinx,pcs.yaml new file mode 100644 index 000000000000..56a3ce0c4ef0 --- /dev/null +++ b/Documentation/devicetree/bindings/net/xilinx,pcs.yaml @@ -0,0 +1,129 @@ +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause) +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/xilinx,pcs.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Xilinx 1G/2.5G Ethernet PCS/PMA or SGMII LogiCORE IP + +maintainers: + - Sean Anderson <sean.anderson@seco.com> + +description: + This is a soft device which implements the PCS and (depending on + configuration) PMA layers of an IEEE Ethernet PHY. On the MAC side, it + implements GMII. It may have an attached SERDES (internal or external), or + may directly use LVDS IO resources. Depending on the configuration, it may + implement 1000BASE-X, SGMII, 2500BASE-X, or 2.5G SGMII. + + This device has a notion of "shared logic" such as reset and clocking + resources which must be shared between multiple PCSs using the same I/O + banks. Each PCS can be configured to have the shared logic in the "core" + (instantiated internally and made available to other PCSs) or in the "example + design" (provided by another PCS). PCSs with shared logic in the core are + reset controllers, and generally provide several resets for other PCSs in the + same bank. + +allOf: + - $ref: ethernet-controller.yaml# + +properties: + compatible: + contains: + const: xilinx,pcs-16.2 + + reg: + maxItems: 1 + + "#clock-cells": + const: 0 + description: + Register a clock representing the clocking resources shared with other + PCSs. + + clocks: + items: + - description: + The reference clock for the PCS. Depending on your setup, this may be + the gtrefclk, refclk, clk125m signal, or clocks from another PCS. + + clock-names: + const: refclk + + done-gpios: + maxItems: 1 + description: + GPIO connected to the reset-done output, if present. + + interrupts: + items: + - description: + The an_interrupt autonegotiation-complete interrupt. + + interrupt-names: + const: an + + pcs-modes: + description: + The interfaces that the PCS supports. + oneOf: + - const: sgmii + - const: 1000base-x + - const: 2500base-x + - items: + - const: sgmii + - const: 1000base-x + + reset-gpios: + maxItems: 1 + description: + GPIO connected to the reset input. + +required: + - compatible + - reg + - pcs-modes + +additionalProperties: false + +examples: + - | + #include <dt-bindings/gpio/gpio.h> + #include <dt-bindings/interrupt-controller/arm-gic.h> + #include <dt-bindings/interrupt-controller/irq.h> + + mdio { + #address-cells = <1>; + #size-cells = <0>; + + pcs0: ethernet-pcs@0 { + #clock-cells = <0>; + compatible = "xlnx,pcs-16.2"; + reg = <0>; + clocks = <&si570>; + clock-names = "refclk"; + interrupts-extended = <&gic GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "an"; + reset-gpios = <&gpio 5 GPIO_ACTIVE_HIGH>; + done-gpios = <&gpio 6 GPIO_ACTIVE_HIGH>; + pcs-modes = "sgmii", "1000base-x"; + }; + + pcs1: ethernet-pcs@1 { + compatible = "xlnx,pcs-16.2"; + reg = <1>; + clocks = <&pcs0>; + clock-names = "refclk"; + interrupts-extended = <&gic GIC_SPI 106 IRQ_TYPE_LEVEL_HIGH>; + interrupt-names = "an"; + reset-gpios = <&gpio 7 GPIO_ACTIVE_HIGH>; + done-gpios = <&gpio 8 GPIO_ACTIVE_HIGH>; + pcs-modes = "sgmii", "1000base-x"; + }; + + pcs2: ethernet-pcs@2 { + compatible = "xlnx,pcs-16.2"; + reg = <2>; + pcs-modes = "sgmii"; + }; + };
This adds a binding for the Xilinx 1G/2.5G Ethernet PCS/PMA or SGMII LogiCORE IP. This device is a soft device typically used to adapt between GMII and SGMII or 1000BASE-X (possbilty in combination with a serdes). pcs-modes reflects the modes available with the as configured when the device is synthesized. Multiple modes may be specified if dynamic reconfiguration is supported. One PCS may contain "shared logic in core" which can be connected to other PCSs with "shared logic in example design." This primarily refers to clocking resources, allowing a reference clock to be shared by a bank of PCSs. To support this, if #clock-cells is defined then the PCS will register itself as a clock provider for other PCSs. Signed-off-by: Sean Anderson <sean.anderson@linux.dev> --- .../devicetree/bindings/net/xilinx,pcs.yaml | 129 ++++++++++++++++++ 1 file changed, 129 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/xilinx,pcs.yaml