| Message ID | 1592832924-31733-3-git-send-email-florinel.iordache@nxp.com |
|---|---|
| State | Changes Requested |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: ethernet backplane support on DPAA1 | expand |
On 6/22/20 6:35 AM, Florinel Iordache wrote: > Add ethernet backplane device tree bindings > > Signed-off-by: Florinel Iordache <florinel.iordache@nxp.com> > --- [snip] > +properties: > + $nodename: > + pattern: "^serdes(@[a-f0-9]+)?$" > + > + compatible: > + oneOf: > + - const: serdes-10g > + description: SerDes module type of 10G Since this appears to be memory mapped in your case, it does not sound like "serdes-10g" alone is going to be sufficient, should not we have a SoC specific compatible string if nothing else? > + > + reg: > + description: > + Registers memory map offset and size for this serdes module > + > + reg-names: > + description: > + Names of the register map given in "reg" node. You would also need to describe how many of these two properties are expected. > + > + little-endian: > + description: > + Specifies the endianness of serdes module > + For complete definition see > + Documentation/devicetree/bindings/common-properties.txt This is redundant with the default binding then, and if it is not already in the common YAML binding, can you please add little-endian and native-endian added there? > + > +examples: > + - | > + serdes1: serdes@1ea0000 { > + compatible = "serdes-10g"; > + reg = <0x0 0x1ea0000 0 0x00002000>; > + reg-names = "serdes", "serdes-10g"; > + little-endian; > + }; >
> -----Original Message----- > From: Florian Fainelli <f.fainelli@gmail.com> > Sent: Tuesday, June 23, 2020 1:21 AM > To: Florinel Iordache <florinel.iordache@nxp.com>; davem@davemloft.net; > netdev@vger.kernel.org; andrew@lunn.ch; hkallweit1@gmail.com; > linux@armlinux.org.uk > Cc: devicetree@vger.kernel.org; linux-doc@vger.kernel.org; > robh+dt@kernel.org; mark.rutland@arm.com; kuba@kernel.org; > corbet@lwn.net; shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; Madalin > Bucur (OSS) <madalin.bucur@oss.nxp.com>; Ioana Ciornei > <ioana.ciornei@nxp.com>; linux-kernel@vger.kernel.org > Subject: [EXT] Re: [PATCH net-next v3 2/7] dt-bindings: net: add backplane dt > bindings > > Caution: EXT Email > > On 6/22/20 6:35 AM, Florinel Iordache wrote: > > Add ethernet backplane device tree bindings > > > > Signed-off-by: Florinel Iordache <florinel.iordache@nxp.com> > > --- > > [snip] > > > +properties: > > + $nodename: > > + pattern: "^serdes(@[a-f0-9]+)?$" > > + > > + compatible: > > + oneOf: > > + - const: serdes-10g > > + description: SerDes module type of 10G > > Since this appears to be memory mapped in your case, it does not sound like > "serdes-10g" alone is going to be sufficient, should not we have a SoC specific > compatible string if nothing else? My intention was to make it generic enough to be used by any SerDes (Serializer/Deserializer) block. So I was thinking that specifying serdes as HW block and the type: 10g (or 28g for example) should be enough. I could add SoC specific (or family of SoC) to the compatible string like for example Freescale/NXP QorIQ Soc: "fsl,ls1046a-serdes-10g" or "fsl,qoriq-serdes-10g" > > > + > > + reg: > > + description: > > + Registers memory map offset and size for this serdes module > > + > > + reg-names: > > + description: > > + Names of the register map given in "reg" node. > > You would also need to describe how many of these two properties are > expected. Only one memory map is required since the memory maps for lanes are individually described (as it is documented in serdes-lane.yaml). I will specify this. > > > + > > + little-endian: > > + description: > > + Specifies the endianness of serdes module > > + For complete definition see > > + Documentation/devicetree/bindings/common-properties.txt > > This is redundant with the default binding then, and if it is not already in the > common YAML binding, can you please add little-endian and native-endian > added there? The endianness of the serdes block must be specified as little-endian or big-endian. The serdes endianness may be different than the cores endianness. This is also the case for QorIQ LS1043/LS1046 platforms with ARM cores which are little endian but serdes block is big endian. So endianness must be specified in device tree in order for the driver to know how to access it. This is the generic binding description (with an example below) but for LS1046 platform for example we should put: big-endian (as it is in the last patch: 0007-arm64-dts-add-serdes-and-mdio-description.patch in file: /arch/arm64/boot/dts/freescale/fsl-ls1046a.dtsi ) > > > + > > +examples: > > + - | > > + serdes1: serdes@1ea0000 { > > + compatible = "serdes-10g"; > > + reg = <0x0 0x1ea0000 0 0x00002000>; > > + reg-names = "serdes", "serdes-10g"; > > + little-endian; > > + }; > > > > > -- > Florian Thank you for feedback Florinel.
On 6/24/20 5:55 AM, Florinel Iordache wrote: >> -----Original Message----- >> From: Florian Fainelli <f.fainelli@gmail.com> >> Sent: Tuesday, June 23, 2020 1:21 AM >> To: Florinel Iordache <florinel.iordache@nxp.com>; davem@davemloft.net; >> netdev@vger.kernel.org; andrew@lunn.ch; hkallweit1@gmail.com; >> linux@armlinux.org.uk >> Cc: devicetree@vger.kernel.org; linux-doc@vger.kernel.org; >> robh+dt@kernel.org; mark.rutland@arm.com; kuba@kernel.org; >> corbet@lwn.net; shawnguo@kernel.org; Leo Li <leoyang.li@nxp.com>; Madalin >> Bucur (OSS) <madalin.bucur@oss.nxp.com>; Ioana Ciornei >> <ioana.ciornei@nxp.com>; linux-kernel@vger.kernel.org >> Subject: [EXT] Re: [PATCH net-next v3 2/7] dt-bindings: net: add backplane dt >> bindings >> >> Caution: EXT Email >> >> On 6/22/20 6:35 AM, Florinel Iordache wrote: >>> Add ethernet backplane device tree bindings >>> >>> Signed-off-by: Florinel Iordache <florinel.iordache@nxp.com> >>> --- >> >> [snip] >> >>> +properties: >>> + $nodename: >>> + pattern: "^serdes(@[a-f0-9]+)?$" >>> + >>> + compatible: >>> + oneOf: >>> + - const: serdes-10g >>> + description: SerDes module type of 10G >> >> Since this appears to be memory mapped in your case, it does not sound like >> "serdes-10g" alone is going to be sufficient, should not we have a SoC specific >> compatible string if nothing else? > > My intention was to make it generic enough to be used by any SerDes (Serializer/Deserializer) block. > So I was thinking that specifying serdes as HW block and the type: 10g (or 28g for example) should be enough. > I could add SoC specific (or family of SoC) to the compatible string > like for example Freescale/NXP QorIQ Soc: "fsl,ls1046a-serdes-10g" or "fsl,qoriq-serdes-10g" It does not seem to me that the register interface is going to be anything but generic, therefore using SoC specific compatible strings would be a safer and forward looking approach. If a generic/fall back compatibility string can be added, it can be added later on, that is much less problematic than the opposite. > >> >>> + >>> + reg: >>> + description: >>> + Registers memory map offset and size for this serdes module >>> + >>> + reg-names: >>> + description: >>> + Names of the register map given in "reg" node. >> >> You would also need to describe how many of these two properties are >> expected. > > Only one memory map is required since the memory maps for lanes are individually described > (as it is documented in serdes-lane.yaml). > I will specify this. Then I believe you need to advertise that with maxItems property to denote how many. > >> >>> + >>> + little-endian: >>> + description: >>> + Specifies the endianness of serdes module >>> + For complete definition see >>> + Documentation/devicetree/bindings/common-properties.txt >> >> This is redundant with the default binding then, and if it is not already in the >> common YAML binding, can you please add little-endian and native-endian >> added there? > > The endianness of the serdes block must be specified as little-endian or big-endian. > The serdes endianness may be different than the cores endianness. > This is also the case for QorIQ LS1043/LS1046 platforms with ARM cores which > are little endian but serdes block is big endian. > So endianness must be specified in device tree in order for the driver to know how to access it. I understand that part, my point was more than these properties are generic properties, therefore it should not be necessary to provide a description, and their definition belongs in the common properties binding. If the common binding does not have a definition for those (that is, in a YAML way), then please add them there.
On Mon, 22 Jun 2020 16:35:19 +0300, Florinel Iordache wrote: > Add ethernet backplane device tree bindings > > Signed-off-by: Florinel Iordache <florinel.iordache@nxp.com> > --- > .../bindings/net/ethernet-controller.yaml | 7 ++- > .../devicetree/bindings/net/ethernet-phy.yaml | 50 ++++++++++++++++++++++ > .../devicetree/bindings/net/serdes-lane.yaml | 49 +++++++++++++++++++++ > Documentation/devicetree/bindings/net/serdes.yaml | 42 ++++++++++++++++++ > 4 files changed, 146 insertions(+), 2 deletions(-) > create mode 100644 Documentation/devicetree/bindings/net/serdes-lane.yaml > create mode 100644 Documentation/devicetree/bindings/net/serdes.yaml > My bot found errors running 'make dt_binding_check' on your patch: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/serdes.example.dt.yaml: example-0: serdes@1ea0000:reg:0: [0, 32112640, 0, 8192] is too long /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/serdes-lane.example.dt.yaml: example-0: serdes@1ea0000:reg:0: [0, 32112640, 0, 8192] is too long See https://patchwork.ozlabs.org/patch/1314386 If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure dt-schema is up to date: pip3 install git+https://github.com/devicetree-org/dt-schema.git@master --upgrade Please check and re-submit.
diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml index 1c44740..6c4c7d8 100644 --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml @@ -91,10 +91,13 @@ properties: - rxaui - xaui - # 10GBASE-KR, XFI, SFI - - 10gbase-kr + # 10GBASE-R, XFI, SFI + - 10gbase-r - usxgmii + # 10GBASE-KR (10G Ethernet Backplane with autonegotiation) + - 10gbase-kr + phy-mode: $ref: "#/properties/phy-connection-type" diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml index 9b1f114..a23a7d6 100644 --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml @@ -162,6 +162,42 @@ properties: description: Specifies a reference to a node representing a SFP cage. + eq-algorithm: + description: + Specifies the desired equalization algorithm to be used + by the KR link training + oneOf: + - const: fixed + description: + Backplane KR using fixed coefficients meaning no + equalization algorithm + - const: bee + description: + Backplane KR using 3-Taps Bit Edge Equalization (BEE) + algorithm + + eq-init: + $ref: /schemas/types.yaml#/definitions/uint32-array + minItems: 3 + maxItems: 3 + description: + Triplet of KR coefficients. Specifies the initialization + values for standard KR equalization coefficients used by + the link training (pre-cursor, main-cursor, post-cursor) + + eq-params: + $ref: /schemas/types.yaml#/definitions/uint32-array + description: + Variable size array of KR parameters. Specifies the HW + specific parameters used by the link training. + + lane-handle: + $ref: /schemas/types.yaml#definitions/phandle + description: + Specifies a reference (or array of references) to a node + representing the desired SERDES lane (or lanes) used in + backplane mode. + required: - reg @@ -184,3 +220,17 @@ examples: reset-deassert-us = <2000>; }; }; + - | + ethernet { + #address-cells = <1>; + #size-cells = <0>; + + ethernet-phy@0 { + compatible = "ethernet-phy-ieee802.3-c45"; + reg = <0x0>; + lane-handle = <&lane_d>; + eq-algorithm = "fixed"; + eq-init = <0x2 0x29 0x5>; + eq-params = <0>; + }; + }; diff --git a/Documentation/devicetree/bindings/net/serdes-lane.yaml b/Documentation/devicetree/bindings/net/serdes-lane.yaml new file mode 100644 index 0000000..d83a6a9 --- /dev/null +++ b/Documentation/devicetree/bindings/net/serdes-lane.yaml @@ -0,0 +1,49 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/serdes-lane.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Serdes Lane Binding + +maintainers: + - Florinel Iordache <florinel.iordache@nxp.com> + +properties: + $nodename: + pattern: "^lane(@[a-f0-9]+)?$" + + compatible: + oneOf: + - const: lane-10g + description: Lane part of a 10G SerDes module + + reg: + description: + Registers memory map offset and size for this lane + + reg-names: + description: + Names of the register map given in "reg" node. + +examples: + - | + serdes1: serdes@1ea0000 { + compatible = "serdes-10g"; + reg = <0x0 0x1ea0000 0 0x00002000>; + reg-names = "serdes", "serdes-10g"; + little-endian; + + #address-cells = <1>; + #size-cells = <1>; + lane_a: lane@800 { + compatible = "lane-10g"; + reg = <0x800 0x40>; + reg-names = "lane", "serdes-lane"; + }; + lane_b: lane@840 { + compatible = "lane-10g"; + reg = <0x840 0x40>; + reg-names = "lane", "serdes-lane"; + }; + }; diff --git a/Documentation/devicetree/bindings/net/serdes.yaml b/Documentation/devicetree/bindings/net/serdes.yaml new file mode 100644 index 0000000..ed77689c --- /dev/null +++ b/Documentation/devicetree/bindings/net/serdes.yaml @@ -0,0 +1,42 @@ +# SPDX-License-Identifier: GPL-2.0-only OR BSD-2-Clause +%YAML 1.2 +--- +$id: http://devicetree.org/schemas/net/serdes.yaml# +$schema: http://devicetree.org/meta-schemas/core.yaml# + +title: Serdes Module Binding + +maintainers: + - Florinel Iordache <florinel.iordache@nxp.com> + +properties: + $nodename: + pattern: "^serdes(@[a-f0-9]+)?$" + + compatible: + oneOf: + - const: serdes-10g + description: SerDes module type of 10G + + reg: + description: + Registers memory map offset and size for this serdes module + + reg-names: + description: + Names of the register map given in "reg" node. + + little-endian: + description: + Specifies the endianness of serdes module + For complete definition see + Documentation/devicetree/bindings/common-properties.txt + +examples: + - | + serdes1: serdes@1ea0000 { + compatible = "serdes-10g"; + reg = <0x0 0x1ea0000 0 0x00002000>; + reg-names = "serdes", "serdes-10g"; + little-endian; + };
Add ethernet backplane device tree bindings Signed-off-by: Florinel Iordache <florinel.iordache@nxp.com> --- .../bindings/net/ethernet-controller.yaml | 7 ++- .../devicetree/bindings/net/ethernet-phy.yaml | 50 ++++++++++++++++++++++ .../devicetree/bindings/net/serdes-lane.yaml | 49 +++++++++++++++++++++ Documentation/devicetree/bindings/net/serdes.yaml | 42 ++++++++++++++++++ 4 files changed, 146 insertions(+), 2 deletions(-) create mode 100644 Documentation/devicetree/bindings/net/serdes-lane.yaml create mode 100644 Documentation/devicetree/bindings/net/serdes.yaml