| Message ID | cd75c96640cc7fe306ee355acb1db85adb5b796f.1532954208.git-series.quentin.schulz@bootlin.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | mscc: ocelot: add support for SerDes muxing configuration | expand |
> +Required properties: > + > +- compatible: should be "mscc,vsc7514-serdes" > +- #phy-cells : from the generic phy bindings, must be 3. The first number > + defines the kind of Serdes (1 for SERDES1G_X, 6 for > + SERDES6G_X), the second defines the macros in the specified > + kind of Serdes (X for SERDES1G_X or SERDES6G_X) and the > + last one defines the input port to use for a given SerDes > + macro, It looks like there are some space vs tab issues here. > + > +Example: > + > + serdes: serdes { Maybe this should be serdes-mux? The SERDES itself should have some registers somewhere. If you ever decide to make use of phylink, e.g. to support SFP, you are going to need to know if the SERDES is up. So you might need to add the actual SERDES device, in addition to the mux for the SERDES. > + compatible = "mscc,vsc7514-serdes"; > + #phy-cells = <3>; > + };
> +- compatible: should be "mscc,vsc7514-serdes" > +- #phy-cells : from the generic phy bindings, must be 3. The first number > + defines the kind of Serdes (1 for SERDES1G_X, 6 for > + SERDES6G_X), the second defines the macros in the specified > + kind of Serdes (X for SERDES1G_X or SERDES6G_X) and the > + last one defines the input port to use for a given SerDes > + macro, Hi Quentin Maybe add #defines in an include file to make the binding less magic? Andrew
On 07/30/2018 05:43 AM, Quentin Schulz wrote: > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > --- > Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt | 42 +++++++- > 1 file changed, 42 insertions(+) > create mode 100644 Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt > > diff --git a/Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt b/Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt > new file mode 100644 > index 0000000..25b102d > --- /dev/null > +++ b/Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt > @@ -0,0 +1,42 @@ > +Microsemi Ocelot SerDes muxing driver > +------------------------------------- > + > +On Microsemi Ocelot, there is a handful of registers in HSIO address > +space for setting up the SerDes to switch port muxing. > + > +A SerDes X can be "muxed" to work with switch port Y or Z for example. > +One specific SerDes can also be used as a PCIe interface. > + > +Hence, a SerDes represents an interface, be it an Ethernet or a PCIe one. > + > +There are two kinds of SerDes: SERDES1G supports 10/100Mbps in > +half/full-duplex and 1000Mbps in full-duplex mode while SERDES6G supports > +10/100Mbps in half/full-duplex and 1000/2500Mbps in full-duplex mode. > + > +Also, SERDES6G number (aka "macro") 0 is the only interface supporting > +QSGMII. > + > +Required properties: > + > +- compatible: should be "mscc,vsc7514-serdes" > +- #phy-cells : from the generic phy bindings, must be 3. The first number > + defines the kind of Serdes (1 for SERDES1G_X, 6 for > + SERDES6G_X), the second defines the macros in the specified > + kind of Serdes (X for SERDES1G_X or SERDES6G_X) and the > + last one defines the input port to use for a given SerDes > + macro, It would probably be more natural to reverse some of this and have the 1st cell be the input port, while the 2nd and 3rd cell are the serdes kind and the serdes macro type. Same comment as Andrew, can you please define the 2nd and 3rd cells possible values in a header file that you can include from both the DTS and the driver making use of that?
Hi Florian, On Mon, Jul 30, 2018 at 02:39:35PM -0700, Florian Fainelli wrote: > On 07/30/2018 05:43 AM, Quentin Schulz wrote: > > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > > --- > > Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt | 42 +++++++- > > 1 file changed, 42 insertions(+) > > create mode 100644 Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt > > > > diff --git a/Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt b/Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt > > new file mode 100644 > > index 0000000..25b102d > > --- /dev/null > > +++ b/Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt > > @@ -0,0 +1,42 @@ > > +Microsemi Ocelot SerDes muxing driver > > +------------------------------------- > > + > > +On Microsemi Ocelot, there is a handful of registers in HSIO address > > +space for setting up the SerDes to switch port muxing. > > + > > +A SerDes X can be "muxed" to work with switch port Y or Z for example. > > +One specific SerDes can also be used as a PCIe interface. > > + > > +Hence, a SerDes represents an interface, be it an Ethernet or a PCIe one. > > + > > +There are two kinds of SerDes: SERDES1G supports 10/100Mbps in > > +half/full-duplex and 1000Mbps in full-duplex mode while SERDES6G supports > > +10/100Mbps in half/full-duplex and 1000/2500Mbps in full-duplex mode. > > + > > +Also, SERDES6G number (aka "macro") 0 is the only interface supporting > > +QSGMII. > > + > > +Required properties: > > + > > +- compatible: should be "mscc,vsc7514-serdes" > > +- #phy-cells : from the generic phy bindings, must be 3. The first number > > + defines the kind of Serdes (1 for SERDES1G_X, 6 for > > + SERDES6G_X), the second defines the macros in the specified > > + kind of Serdes (X for SERDES1G_X or SERDES6G_X) and the > > + last one defines the input port to use for a given SerDes > > + macro, > > It would probably be more natural to reverse some of this and have the > 1st cell be the input port, while the 2nd and 3rd cell are the serdes > kind and the serdes macro type. Same comment as Andrew, can you please > define the 2nd and 3rd cells possible values in a header file that you > can include from both the DTS and the driver making use of that? OK for a define for the DeviceTree part. You want one set of defines for the values in the 2nd cell and one other set of defines for the 3rd cell? I'm fine with a define for the second value (which is basically the enum serdes_type I've defined at the beginning of the serdes driver) but I don't see the point of defining the index of the SerDes. What would it look like? enum serdes_type { SERDES1G = 1, SERDES6G = 6, } #define SERDES1G_0 0 #define SERDES1G_1 1 #define SERDES1G_2 2 #define SERDES6G_0 0 #define SERDES6G_1 1 Then, e.g.: &port5 { phys = <&serdes 5 SERDES1G SERDES1G_0> }; If you want a define for the pair (serdes_type, serdes_index), I don't see how I could re-use it on the driver side but it makes more sense on the DeviceTree side: #define SERDES1G_0 1 0 #define SERDES1G_1 1 1 #define SERDES1G_2 1 2 #define SERDES6G_0 6 0 #define SERDES6G_1 6 1 Quentin
Hi Andrew, On Mon, Jul 30, 2018 at 03:34:48PM +0200, Andrew Lunn wrote: > > +Required properties: > > + > > +- compatible: should be "mscc,vsc7514-serdes" > > +- #phy-cells : from the generic phy bindings, must be 3. The first number > > + defines the kind of Serdes (1 for SERDES1G_X, 6 for > > + SERDES6G_X), the second defines the macros in the specified > > + kind of Serdes (X for SERDES1G_X or SERDES6G_X) and the > > + last one defines the input port to use for a given SerDes > > + macro, > > It looks like there are some space vs tab issues here. > Yup. > > + > > +Example: > > + > > + serdes: serdes { > > Maybe this should be serdes-mux? The SERDES itself should have some > registers somewhere. If you ever decide to make use of phylink, > e.g. to support SFP, you are going to need to know if the SERDES is > up. So you might need to add the actual SERDES device, in addition to > the mux for the SERDES. > I'm not sure to follow. To be honest, I might have mislead you. The whole configuration of the serdes is in the hsio register address space. For now, muxing is the only reason there is a driver for the serdes but there are other things that can be configured (though not used yet): de/serializer, input/output buffers, PLL, ... configuration registers for the SerDes. So I think I should remove the mention to muxing and just say in the cover letter and commit log it's for muxing the SerDes among other configurations. So I think we're good with the current driver and DT, just poor wording on my side for the commit log/cover letter. Do you agree? Quentin
> > Maybe this should be serdes-mux? The SERDES itself should have some > > registers somewhere. If you ever decide to make use of phylink, > > e.g. to support SFP, you are going to need to know if the SERDES is > > up. So you might need to add the actual SERDES device, in addition to > > the mux for the SERDES. > > > > I'm not sure to follow. > > To be honest, I might have mislead you. The whole configuration of the > serdes is in the hsio register address space. For now, muxing is the > only reason there is a driver for the serdes but there are other things > that can be configured (though not used yet): de/serializer, input/output > buffers, PLL, ... configuration registers for the SerDes. When you are using the SERDES for networking, you need to know if the SERDES has achieved sync. For example, when the SERDES connects to an optical SFP module, the SERDES bit stream continues unmodified over the optical link to the SERDES in the peer. The optical module can tell you if it is receiving optical power, but it cannot tell you if the optical signal makes any sense. The SERDES however knows how to decode the bitstream, sync to it, etc. So you need some registers in the SERDES to get this status information. Typically, you can also get access to the SGMII/1000Base-X code word, so you can do auto-negotiation, or know if you need to send each bit 10 or 100 times in order to do 100Mbps or 10Mbps. If you are connecting to a PHY which can do > 1Gbps, you need to change the SERDES between SGMII, 1000Base-X, 2500Base-X, etc. Before you can say the link is up, you want the PHY to tell you it has link to its peer PHY, and you want to know the SERDES is ready. Typically the SERDES is last, since you don't know what to configure the SERDES to until the PHY is finished negotiating the link to its peer. If you look at any of the Marvell SERDES interfaces, found in PHYs or switches, there are dozens of registers for controlling the SERDES. Now, it could be we don't have a clear definition of what a SERDES is. The Marvell documents has a lot in its definition of SERDES, where as what you could be purely a 'dumb' parallel to serial convert, and all the rest of the logic is in the Ethernet MAC and the PCIe device? Now, back to my original point. Where are the registers for 'the rest of this logic'? If they are in the MAC address space, we don't have a problem. If they are somewhere else, maybe you will need to add another device. What is this device called? That is why i'm trying to differentiate between the 'SERDES-MUX' and the 'SERDES'. Andrew
Hi Andrew, On Wed, Aug 01, 2018 at 04:31:47PM +0200, Andrew Lunn wrote: > > > Maybe this should be serdes-mux? The SERDES itself should have some > > > registers somewhere. If you ever decide to make use of phylink, > > > e.g. to support SFP, you are going to need to know if the SERDES is > > > up. So you might need to add the actual SERDES device, in addition to > > > the mux for the SERDES. > > > > > > > I'm not sure to follow. > > > > To be honest, I might have mislead you. The whole configuration of the > > serdes is in the hsio register address space. For now, muxing is the > > only reason there is a driver for the serdes but there are other things > > that can be configured (though not used yet): de/serializer, input/output > > buffers, PLL, ... configuration registers for the SerDes. > > When you are using the SERDES for networking, you need to know if the > SERDES has achieved sync. For example, when the SERDES connects to an > optical SFP module, the SERDES bit stream continues unmodified over > the optical link to the SERDES in the peer. The optical module can > tell you if it is receiving optical power, but it cannot tell you if > the optical signal makes any sense. The SERDES however knows how to > decode the bitstream, sync to it, etc. So you need some registers in > the SERDES to get this status information. Typically, you can also get > access to the SGMII/1000Base-X code word, so you can do > auto-negotiation, or know if you need to send each bit 10 or 100 times > in order to do 100Mbps or 10Mbps. If you are connecting to a PHY which > can do > 1Gbps, you need to change the SERDES between SGMII, > 1000Base-X, 2500Base-X, etc. Before you can say the link is up, you > want the PHY to tell you it has link to its peer PHY, and you want to > know the SERDES is ready. Typically the SERDES is last, since you > don't know what to configure the SERDES to until the PHY is finished > negotiating the link to its peer. > > If you look at any of the Marvell SERDES interfaces, found in PHYs or > switches, there are dozens of registers for controlling the SERDES. > > Now, it could be we don't have a clear definition of what a SERDES > is. The Marvell documents has a lot in its definition of SERDES, where > as what you could be purely a 'dumb' parallel to serial convert, and > all the rest of the logic is in the Ethernet MAC and the PCIe device? > > Now, back to my original point. Where are the registers for 'the rest > of this logic'? If they are in the MAC address space, we don't have a > problem. If they are somewhere else, maybe you will need to add > another device. What is this device called? That is why i'm trying to > differentiate between the 'SERDES-MUX' and the 'SERDES'. > If I've correctly read the datasheet of the switch, the sync status bit is in the MAC address space. Same for 1000BASE-X/SGMII, autonegotiation. The point I was trying to make was that this driver isn't only for "muxing". There are also a handful of registers for (electronically-related ?) features of SerDes (e.g. "control of phase regulator logic", "deserializer phase control", a few thresholds/hysteresis/frequencies, etc... I understand that "serdes" isn't also fully matching the work done by this driver as some features are handled within the MAC controller address space. Let me know if something bothers you/does not make sense, Thanks, Quentin
On Wed, Aug 01, 2018 at 10:15:39AM +0200, Quentin Schulz wrote: > Hi Florian, > > On Mon, Jul 30, 2018 at 02:39:35PM -0700, Florian Fainelli wrote: > > On 07/30/2018 05:43 AM, Quentin Schulz wrote: > > > Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> > > > --- > > > Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt | 42 +++++++- > > > 1 file changed, 42 insertions(+) > > > create mode 100644 Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt > > > > > > diff --git a/Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt b/Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt > > > new file mode 100644 > > > index 0000000..25b102d > > > --- /dev/null > > > +++ b/Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt > > > @@ -0,0 +1,42 @@ > > > +Microsemi Ocelot SerDes muxing driver > > > +------------------------------------- > > > + > > > +On Microsemi Ocelot, there is a handful of registers in HSIO address > > > +space for setting up the SerDes to switch port muxing. > > > + > > > +A SerDes X can be "muxed" to work with switch port Y or Z for example. > > > +One specific SerDes can also be used as a PCIe interface. > > > + > > > +Hence, a SerDes represents an interface, be it an Ethernet or a PCIe one. > > > + > > > +There are two kinds of SerDes: SERDES1G supports 10/100Mbps in > > > +half/full-duplex and 1000Mbps in full-duplex mode while SERDES6G supports > > > +10/100Mbps in half/full-duplex and 1000/2500Mbps in full-duplex mode. > > > + > > > +Also, SERDES6G number (aka "macro") 0 is the only interface supporting > > > +QSGMII. > > > + > > > +Required properties: > > > + > > > +- compatible: should be "mscc,vsc7514-serdes" > > > +- #phy-cells : from the generic phy bindings, must be 3. The first number > > > + defines the kind of Serdes (1 for SERDES1G_X, 6 for > > > + SERDES6G_X), the second defines the macros in the specified > > > + kind of Serdes (X for SERDES1G_X or SERDES6G_X) and the > > > + last one defines the input port to use for a given SerDes > > > + macro, > > > > It would probably be more natural to reverse some of this and have the > > 1st cell be the input port, while the 2nd and 3rd cell are the serdes > > kind and the serdes macro type. Same comment as Andrew, can you please > > define the 2nd and 3rd cells possible values in a header file that you > > can include from both the DTS and the driver making use of that? > > OK for a define for the DeviceTree part. > > You want one set of defines for the values in the 2nd cell and one other > set of defines for the 3rd cell? > > I'm fine with a define for the second value (which is basically the enum > serdes_type I've defined at the beginning of the serdes driver) but I > don't see the point of defining the index of the SerDes. What would it > look like? > > enum serdes_type { > SERDES1G = 1, > SERDES6G = 6, > } > > #define SERDES1G_0 0 > #define SERDES1G_1 1 > #define SERDES1G_2 2 > #define SERDES6G_0 0 > #define SERDES6G_1 1 > > Then, e.g.: > > &port5 { > phys = <&serdes 5 SERDES1G SERDES1G_0> > }; > > If you want a define for the pair (serdes_type, serdes_index), I don't > see how I could re-use it on the driver side but it makes more sense on the > DeviceTree side: > > #define SERDES1G_0 1 0 > #define SERDES1G_1 1 1 > #define SERDES1G_2 1 2 > #define SERDES6G_0 6 0 > #define SERDES6G_1 6 1 I prefer #defines which are a single number. Otherwise if you read a dts file when #phy-cells is 3, it will look like an error in that you have what looks like 2 cells. Rob
On 13/08/2018 16:37:48-0600, Rob Herring wrote: > > I'm fine with a define for the second value (which is basically the enum > > serdes_type I've defined at the beginning of the serdes driver) but I > > don't see the point of defining the index of the SerDes. What would it > > look like? > > > > enum serdes_type { > > SERDES1G = 1, > > SERDES6G = 6, > > } > > > > #define SERDES1G_0 0 > > #define SERDES1G_1 1 > > #define SERDES1G_2 2 > > #define SERDES6G_0 0 > > #define SERDES6G_1 1 > > > > Then, e.g.: > > > > &port5 { > > phys = <&serdes 5 SERDES1G SERDES1G_0> > > }; > > > > If you want a define for the pair (serdes_type, serdes_index), I don't > > see how I could re-use it on the driver side but it makes more sense on the > > DeviceTree side: > > > > #define SERDES1G_0 1 0 > > #define SERDES1G_1 1 1 > > #define SERDES1G_2 1 2 > > #define SERDES6G_0 6 0 > > #define SERDES6G_1 6 1 > > I prefer #defines which are a single number. Otherwise if you read a dts > file when #phy-cells is 3, it will look like an error in that you have > what looks like 2 cells. > Maybe we should not have the type in DT and simply have an index. The driver will now what the serdes type is anyway and the defines would be: #define SERDES1G_0 0 #define SERDES1G_1 1 #define SERDES1G_2 2 #define SERDES6G_0 3 #define SERDES6G_1 4 The main drawback is that this requires one include file per soc.
diff --git a/Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt b/Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt new file mode 100644 index 0000000..25b102d --- /dev/null +++ b/Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt @@ -0,0 +1,42 @@ +Microsemi Ocelot SerDes muxing driver +------------------------------------- + +On Microsemi Ocelot, there is a handful of registers in HSIO address +space for setting up the SerDes to switch port muxing. + +A SerDes X can be "muxed" to work with switch port Y or Z for example. +One specific SerDes can also be used as a PCIe interface. + +Hence, a SerDes represents an interface, be it an Ethernet or a PCIe one. + +There are two kinds of SerDes: SERDES1G supports 10/100Mbps in +half/full-duplex and 1000Mbps in full-duplex mode while SERDES6G supports +10/100Mbps in half/full-duplex and 1000/2500Mbps in full-duplex mode. + +Also, SERDES6G number (aka "macro") 0 is the only interface supporting +QSGMII. + +Required properties: + +- compatible: should be "mscc,vsc7514-serdes" +- #phy-cells : from the generic phy bindings, must be 3. The first number + defines the kind of Serdes (1 for SERDES1G_X, 6 for + SERDES6G_X), the second defines the macros in the specified + kind of Serdes (X for SERDES1G_X or SERDES6G_X) and the + last one defines the input port to use for a given SerDes + macro, + +Example: + + serdes: serdes { + compatible = "mscc,vsc7514-serdes"; + #phy-cells = <3>; + }; + + ethernet { + port1 { + phy-handle = <&phy_foo>; + /* Link SERDES1G_5 to port1 */ + phys = <&serdes 1 5 1>; + }; + };
Signed-off-by: Quentin Schulz <quentin.schulz@bootlin.com> --- Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt | 42 +++++++- 1 file changed, 42 insertions(+) create mode 100644 Documentation/devicetree/bindings/phy/phy-ocelot-serdes.txt