| Message ID | 1585230682-24417-3-git-send-email-florinel.iordache@nxp.com |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | net: ethernet backplane support | expand |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | success | |
| robh/dt-meta-schema | fail | build log |
On Thu, Mar 26, 2020 at 03:51:15PM +0200, Florinel Iordache wrote: > Add ethernet backplane device tree bindings > + - | > + /* Backplane configurations for specific setup */ > + &mdio9 { > + bpphy6: ethernet-phy@0 { > + compatible = "ethernet-phy-ieee802.3-c45"; > + reg = <0x0>; > + lane-handle = <&lane_d>; /* use lane D */ > + eq-algorithm = "bee"; > + /* 10G Short cables setup: up to 30 cm cable */ > + eq-init = <0x2 0x5 0x29>; > + eq-params = <0>; > + }; > + }; So you are modelling this as just another PHY? Does the driver get loaded based on the PHY ID in registers 2 and 3? Does the standard define these IDs or are they vendor specific? Thanks Andrew
On Fri, Mar 27, 2020 at 02:04:11AM +0100, Andrew Lunn wrote: > On Thu, Mar 26, 2020 at 03:51:15PM +0200, Florinel Iordache wrote: > > Add ethernet backplane device tree bindings > > > + - | > > + /* Backplane configurations for specific setup */ > > + &mdio9 { > > + bpphy6: ethernet-phy@0 { > > + compatible = "ethernet-phy-ieee802.3-c45"; > > + reg = <0x0>; > > + lane-handle = <&lane_d>; /* use lane D */ > > + eq-algorithm = "bee"; > > + /* 10G Short cables setup: up to 30 cm cable */ > > + eq-init = <0x2 0x5 0x29>; > > + eq-params = <0>; > > + }; > > + }; > > So you are modelling this as just another PHY? Does the driver get > loaded based on the PHY ID in registers 2 and 3? Does the standard > define these IDs or are they vendor specific? We likely need some mutual coordination here between the patches I've already posted for PCS support and these patches. As I've said, we can't deal with multiple ethernet PHYs connected to one MAC, the patches I've posted cope with that, but likely will need changes for this. Conversely, these patches will need changes for my work.
> On Thu, Mar 26, 2020 at 03:51:15PM +0200, Florinel Iordache wrote: > > Add ethernet backplane device tree bindings > > > + - | > > + /* Backplane configurations for specific setup */ > > + &mdio9 { > > + bpphy6: ethernet-phy@0 { > > + compatible = "ethernet-phy-ieee802.3-c45"; > > + reg = <0x0>; > > + lane-handle = <&lane_d>; /* use lane D */ > > + eq-algorithm = "bee"; > > + /* 10G Short cables setup: up to 30 cm cable */ > > + eq-init = <0x2 0x5 0x29>; > > + eq-params = <0>; > > + }; > > + }; > > So you are modelling this as just another PHY? Does the driver get loaded based > on the PHY ID in registers 2 and 3? Does the standard define these IDs or are > they vendor specific? > > Thanks > Andrew Hi Andrew, Thank you all for the feedback. I am currently working to address the entire feedback received so far for this new Backplane driver. Yes, we are modelling backplane driver as a phy driver. The driver is loaded based on PHY ID in registers 2 and 3 which are specified by the standard but it is a vendor specific value: 32-Bit identifier composed of the 3rd through 24th bits of the Organizationally Unique Identifier (OUI) assigned to the device manufacturer by the IEEE, plus a six-bit model number, plus a four-bit revision number. This is done in the device specific code and not in backplane generic driver. You can check support for QorIQ devices where qoriq_backplane_driver is registered as a phy_driver: @file: qoriq_backplane.c +static struct phy_driver qoriq_backplane_driver[] = { + { + .phy_id = PCS_PHY_DEVICE_ID, + .name = QORIQ_BACKPLANE_DRIVER_NAME, + .phy_id_mask = PCS_PHY_DEVICE_ID_MASK, + .features = BACKPLANE_FEATURES, + .probe = qoriq_backplane_probe, + .remove = backplane_remove, + .config_init = qoriq_backplane_config_init, + .aneg_done = backplane_aneg_done, Here we register the particular phy device ID/mask and driver name specific for qoriq devices. Also we can use generic routines provided by generic backplane driver if they are suitable for particular qoriq device or otherwise we can use more specialized specific routines like: qoriq_backplane_config_init
On Fri, Mar 27, 2020 at 03:00:22PM +0000, Florinel Iordache wrote: > > On Thu, Mar 26, 2020 at 03:51:15PM +0200, Florinel Iordache wrote: > > > Add ethernet backplane device tree bindings > > > > > + - | > > > + /* Backplane configurations for specific setup */ > > > + &mdio9 { > > > + bpphy6: ethernet-phy@0 { > > > + compatible = "ethernet-phy-ieee802.3-c45"; > > > + reg = <0x0>; > > > + lane-handle = <&lane_d>; /* use lane D */ > > > + eq-algorithm = "bee"; > > > + /* 10G Short cables setup: up to 30 cm cable */ > > > + eq-init = <0x2 0x5 0x29>; > > > + eq-params = <0>; > > > + }; > > > + }; > > > > So you are modelling this as just another PHY? Does the driver get loaded based > > on the PHY ID in registers 2 and 3? Does the standard define these IDs or are > > they vendor specific? > > > > Thanks > > Andrew > > Hi Andrew, > Thank you all for the feedback. > I am currently working to address the entire feedback received > so far for this new Backplane driver. > > Yes, we are modelling backplane driver as a phy driver. I think we need to think very carefully about that, and consider whether that really is a good idea. phylib is currently built primarily around copper PHYs, although there are some which also support fiber as well in weird "non-standard" forms. What worries me is the situation which I've been working on, where we want access to the PCS PHYs, and we can't have the PCS PHYs represented as a phylib PHY because we may have a copper PHY behind the PCS PHY, and we want to be talking to the copper PHY in the first instance (the PCS PHY effectivel ybecomes a slave to the copper PHY.) My worry is that we're ending up with conflicting implementations for the same hardware which may only end up causing problems down the line. Please can you look at my DPAA2 PCS series which has been previously posted to netdev - it's rather difficult to work out who in NXP should be copied, because that information is not visible to those of us in the community - we only find that out after someone inside NXP posts patches, and even then the MAINTAINERS file doesn't seem to get updated. It's also worth mentioning that on other SoCs, such as Marvell SoCs, the serdes and "PCS" are entirely separate hardware blocks, and the implementation in the kernel, which works very well, is to use the drivers/phy for the serdes/comphy as they call it, and the ethernet driver binds to the comphy to control the serdes settings, whereas the ethernet driver looks after the PCS. I haven't been able to look at your code enough yet to work out if that would be possible. I also wonder whether we want a separate class of MDIO device for PCS PHYs, just as we have things like DSA switches implemented entirely separately from phylib - they're basically different sub- classes of a mdio device. I think we have around 20 or so weeks to hash this out, since it's clear that the 10gbase-kr (10GKR) phy interface mode can't be used until we've eliminated it from existing dts files. > The driver is loaded based on PHY ID in registers 2 and 3 which > are specified by the standard but it is a vendor specific value: > 32-Bit identifier composed of the 3rd through 24th bits of the > Organizationally Unique Identifier (OUI) assigned to the device > manufacturer by the IEEE, plus a six-bit model number, plus a > four-bit revision number. > This is done in the device specific code and not in backplane > generic driver. > You can check support for QorIQ devices where qoriq_backplane_driver > is registered as a phy_driver: > > @file: qoriq_backplane.c > +static struct phy_driver qoriq_backplane_driver[] = { > + { > + .phy_id = PCS_PHY_DEVICE_ID, > + .name = QORIQ_BACKPLANE_DRIVER_NAME, > + .phy_id_mask = PCS_PHY_DEVICE_ID_MASK, > + .features = BACKPLANE_FEATURES, > + .probe = qoriq_backplane_probe, > + .remove = backplane_remove, > + .config_init = qoriq_backplane_config_init, > + .aneg_done = backplane_aneg_done, > > Here we register the particular phy device ID/mask and driver name > specific for qoriq devices. > Also we can use generic routines provided by generic backplane driver > if they are suitable for particular qoriq device or otherwise we can use > more specialized specific routines like: qoriq_backplane_config_init >
> What worries me is the situation which I've been working on, where > we want access to the PCS PHYs, and we can't have the PCS PHYs > represented as a phylib PHY because we may have a copper PHY behind > the PCS PHY, and we want to be talking to the copper PHY in the > first instance (the PCS PHY effectivel ybecomes a slave to the > copper PHY.) I guess we need to clarify what KR actually means. If we have a backplane with a MAC on each end, i think modelling it as a PHY could work. If however, we have a MAC connected to a backplane, and on the end of the backplane is a traditional PHY, or an SFP cage, we have problems. As your point out, we cannot have two PHYs in a chain for one MAC. But i agree with Russell. We need a general solution of how we deal with PCSs. Andrew
On Fri, Mar 27, 2020 at 04:44:48PM +0100, Andrew Lunn wrote: > > What worries me is the situation which I've been working on, where > > we want access to the PCS PHYs, and we can't have the PCS PHYs > > represented as a phylib PHY because we may have a copper PHY behind > > the PCS PHY, and we want to be talking to the copper PHY in the > > first instance (the PCS PHY effectivel ybecomes a slave to the > > copper PHY.) > > I guess we need to clarify what KR actually means. If we have a > backplane with a MAC on each end, i think modelling it as a PHY could > work. > > If however, we have a MAC connected to a backplane, and on the end of > the backplane is a traditional PHY, or an SFP cage, we have problems. > As your point out, we cannot have two PHYs in a chain for one MAC. > > But i agree with Russell. We need a general solution of how we deal > with PCSs. What really worries me is that we may be driving the same hardware with two different approaches/drivers for two different applications which isn't going to work out very well in the long run.
> -----Original Message----- > From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On > Behalf Of Russell King - ARM Linux admin > Sent: Friday, March 27, 2020 7:35 PM > To: Andrew Lunn <andrew@lunn.ch> > Cc: Florinel Iordache <florinel.iordache@nxp.com>; davem@davemloft.net; > netdev@vger.kernel.org; f.fainelli@gmail.com; hkallweit1@gmail.com; > 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: Re: [EXT] Re: [PATCH net-next 2/9] dt-bindings: net: add > backplane dt bindings > > On Fri, Mar 27, 2020 at 04:44:48PM +0100, Andrew Lunn wrote: > > > What worries me is the situation which I've been working on, where > > > we want access to the PCS PHYs, and we can't have the PCS PHYs > > > represented as a phylib PHY because we may have a copper PHY behind > > > the PCS PHY, and we want to be talking to the copper PHY in the > > > first instance (the PCS PHY effectivel ybecomes a slave to the > > > copper PHY.) > > > > I guess we need to clarify what KR actually means. If we have a > > backplane with a MAC on each end, i think modelling it as a PHY could > > work. > > > > If however, we have a MAC connected to a backplane, and on the end of > > the backplane is a traditional PHY, or an SFP cage, we have problems. > > As your point out, we cannot have two PHYs in a chain for one MAC. > > > > But i agree with Russell. We need a general solution of how we deal > > with PCSs. > > What really worries me is that we may be driving the same hardware > with two different approaches/drivers for two different applications > which isn't going to work out very well in the long run. The same HW can be used in multiple ways here so having different drivers for these modes is not really an issue, you won't be able to use it both in backplane and non-backplane mode at the same time. Besides the (oversimplifying) model used in SW, there is no constraint to have just one independently manageable entity belonging to the PHY layer. Nowadays there are complex configurable PCS/PMA units, retimers, single chip PHYs that can function also in backplane mode, and so on. All these require a rethinking of the one PHY per interface, tied to a MDIO bus model we use today. The DPAA 1 already make use of the MDIO bus infrastructure to manage the PCS devices in the SoC, without an issue related to the PHYlib one PHY assumption. One risk I see here is that we may abandon PHYlib before we give it a chance to adapt to the new complexity of the HW and roll something new just to do away with the required work in understanding its inner workings. This could even be fine but it creates a no return point for drivers that will use a new infrastructure we put in place (i.e. no backporting). Madalin
On Thu, 26 Mar 2020 15:51:15 +0200, Florinel Iordache wrote: > Add ethernet backplane device tree bindings > > Signed-off-by: Florinel Iordache <florinel.iordache@nxp.com> > --- > .../bindings/net/ethernet-controller.yaml | 3 +- > .../devicetree/bindings/net/ethernet-phy.yaml | 53 +++++++++++++ > Documentation/devicetree/bindings/net/serdes.yaml | 90 ++++++++++++++++++++++ > 3 files changed, 145 insertions(+), 1 deletion(-) > create mode 100644 Documentation/devicetree/bindings/net/serdes.yaml > My bot found errors running 'make dt_binding_check' on your patch: Documentation/devicetree/bindings/net/serdes.yaml: mapping values are not allowed in this context in "<unicode string>", line 30, column 62 Documentation/devicetree/bindings/Makefile:12: recipe for target 'Documentation/devicetree/bindings/net/serdes.example.dts' failed make[1]: *** [Documentation/devicetree/bindings/net/serdes.example.dts] Error 1 make[1]: *** Waiting for unfinished jobs.... Documentation/devicetree/bindings/net/ethernet-phy.yaml: mapping values are not allowed in this context in "<unicode string>", line 179, column 20 Documentation/devicetree/bindings/Makefile:12: recipe for target 'Documentation/devicetree/bindings/net/ethernet-phy.example.dts' failed make[1]: *** [Documentation/devicetree/bindings/net/ethernet-phy.example.dts] Error 1 warning: no schema found in file: Documentation/devicetree/bindings/net/serdes.yaml warning: no schema found in file: Documentation/devicetree/bindings/net/ethernet-phy.yaml /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/serdes.yaml: ignoring, error parsing file /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/net/ethernet-phy.yaml: ignoring, error parsing file Makefile:1262: recipe for target 'dt_binding_check' failed make: *** [dt_binding_check] Error 2 See https://patchwork.ozlabs.org/patch/1261991 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 ac471b6..541cee5 100644 --- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml +++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml @@ -93,8 +93,9 @@ properties: - rxaui - xaui - # 10GBASE-KR, XFI, SFI + # 10GBASE-KR, 40GBASE-KR4, XFI, SFI - 10gbase-kr + - 40gbase-kr4 - usxgmii phy-mode: diff --git a/Documentation/devicetree/bindings/net/ethernet-phy.yaml b/Documentation/devicetree/bindings/net/ethernet-phy.yaml index 8927941..2fb377e 100644 --- a/Documentation/devicetree/bindings/net/ethernet-phy.yaml +++ b/Documentation/devicetree/bindings/net/ethernet-phy.yaml @@ -158,6 +158,42 @@ properties: description: Specifies a reference to a node representing a SFP cage. + eq-algorithm: + 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 + description: + Specifies the desired equalization algorithm to be used + by the KR link training + + 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, post-cursor, main-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 @@ -180,3 +216,20 @@ examples: reset-deassert-us = <2000>; }; }; + - | + /* Backplane configurations for specific setup */ + &mdio9 { + bpphy6: ethernet-phy@0 { + compatible = "ethernet-phy-ieee802.3-c45"; + reg = <0x0>; + lane-handle = <&lane_d>; /* use lane D */ + eq-algorithm = "bee"; + /* 10G Short cables setup: up to 30 cm cable */ + eq-init = <0x2 0x5 0x29>; + eq-params = <0>; + }; + }; + &mac9 { + phy-connection-type = "10gbase-kr"; + phy-handle = <&bpphy6>; + }; diff --git a/Documentation/devicetree/bindings/net/serdes.yaml b/Documentation/devicetree/bindings/net/serdes.yaml new file mode 100644 index 0000000..965152f --- /dev/null +++ b/Documentation/devicetree/bindings/net/serdes.yaml @@ -0,0 +1,90 @@ +# SPDX-License-Identifier: GPL-2.0 +%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 + - const: serdes-28g + description: SerDes module type of 28G + + reg: + description: + Registers memory map offset and size for this serdes module + + reg-names: + description: + Names of the register map given in "reg" node. + Should be one of the following according to serdes type: + "serdes", "serdes-10g", "serdes-28g" + + little-endian: + description: + Specifies the endianness of serdes module + For complete definition see: + Documentation/devicetree/bindings/common-properties.txt + + #address-cells: + description: Must be <1> + + #size-cells: + description: Must be <1> + + ranges: + description: + Address range of serdes module. + +properties: + $nodename: + pattern: "^lane(@[a-f0-9]+)?$" + + compatible: + oneOf: + - const: lane-10g + description: Lane part of a 10G SerDes module + - const: lane-28g + description: Lane part of a 28G 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. + Should be one of the following: "lane", "serdes-lane" + +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>; + ranges = <0x0 0x00 0x1ea0000 0x00002000>; + 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"; + }; + };
Add ethernet backplane device tree bindings Signed-off-by: Florinel Iordache <florinel.iordache@nxp.com> --- .../bindings/net/ethernet-controller.yaml | 3 +- .../devicetree/bindings/net/ethernet-phy.yaml | 53 +++++++++++++ Documentation/devicetree/bindings/net/serdes.yaml | 90 ++++++++++++++++++++++ 3 files changed, 145 insertions(+), 1 deletion(-) create mode 100644 Documentation/devicetree/bindings/net/serdes.yaml