Hello everyone,
This is the V2 of a previous series [1] initially aimed at introducing
inband extensions, with modes like QUSGMII. This mode allows passing
info in the ethernet preamble between the MAC and the PHY, such s
timestamps.
This series has now become a preliminary series, that simply introduces
the new interface mode, without support for inband extensions, that will
come later.
The reasonning is that work will need to be done in the networking
subsystem, but also in the generic phy driver subsystem to allow serdes
configuration for qusgmii.
This series add the mode, the relevant binding changes, adds support for
it in the lan966x driver, and also introduces a small helper to get the
number of links a given phy mode can carry (think 1 for SGMII and 4 for
QSGMII). This allows for better readability and will prove useful
when (if) we support PSGMII (5 links on 1 interface) and OUSGMII (8
links on one interface).
Best regards,
Maxime
[1] : https://lore.kernel.org/netdev/[email protected]/
Maxime Chevallier (4):
net: phy: Introduce QUSGMII PHY mode
dt-bindings: net: ethernet-controller: add QUSGMII mode
net: phy: Add helper to derive the number of ports from a phy mode
net: lan966x: Add QUSGMII support for lan966x
.../bindings/net/ethernet-controller.yaml | 1 +
Documentation/networking/phy.rst | 9 ++++
.../ethernet/microchip/lan966x/lan966x_main.c | 2 +
.../microchip/lan966x/lan966x_phylink.c | 3 +-
.../ethernet/microchip/lan966x/lan966x_port.c | 22 +++++---
.../ethernet/microchip/lan966x/lan966x_regs.h | 6 +++
drivers/net/phy/phy-core.c | 52 +++++++++++++++++++
drivers/net/phy/phylink.c | 3 ++
include/linux/phy.h | 5 ++
9 files changed, 96 insertions(+), 7 deletions(-)
--
2.37.1
The Lan996x controller supports the QUSGMII mode, which is very similar
to QSGMII in the way it's configured and the autonegociation
capababilities it provides.
This commit adds support for that mode, treating it most of the time
like QSGMII, making sure that we do configure the PCS how we should.
Signed-off-by: Maxime Chevallier <[email protected]>
---
V1->V2 : Pass the QUSGMII mode as-is to the generic PHY driver, and use
phy_interface_num_ports, as per Russell's review
.../ethernet/microchip/lan966x/lan966x_main.c | 2 ++
.../microchip/lan966x/lan966x_phylink.c | 3 ++-
.../ethernet/microchip/lan966x/lan966x_port.c | 22 ++++++++++++++-----
.../ethernet/microchip/lan966x/lan966x_regs.h | 6 +++++
4 files changed, 26 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
index 1d6e3b641b2e..1e604e8db20c 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.c
@@ -778,6 +778,8 @@ static int lan966x_probe_port(struct lan966x *lan966x, u32 p,
port->phylink_config.supported_interfaces);
__set_bit(PHY_INTERFACE_MODE_QSGMII,
port->phylink_config.supported_interfaces);
+ __set_bit(PHY_INTERFACE_MODE_QUSGMII,
+ port->phylink_config.supported_interfaces);
__set_bit(PHY_INTERFACE_MODE_1000BASEX,
port->phylink_config.supported_interfaces);
__set_bit(PHY_INTERFACE_MODE_2500BASEX,
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c b/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c
index 38a7e95d69b4..87f3d3a57aed 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_phylink.c
@@ -28,11 +28,12 @@ static int lan966x_phylink_mac_prepare(struct phylink_config *config,
phy_interface_t iface)
{
struct lan966x_port *port = netdev_priv(to_net_dev(config->dev));
+ phy_interface_t serdes_mode = iface;
int err;
if (port->serdes) {
err = phy_set_mode_ext(port->serdes, PHY_MODE_ETHERNET,
- iface);
+ serdes_mode);
if (err) {
netdev_err(to_net_dev(config->dev),
"Could not set mode of SerDes\n");
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_port.c b/drivers/net/ethernet/microchip/lan966x/lan966x_port.c
index f141644e4372..bbf42fc8c8d5 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_port.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_port.c
@@ -168,7 +168,7 @@ static void lan966x_port_link_up(struct lan966x_port *port)
/* Also the GIGA_MODE_ENA(1) needs to be set regardless of the
* port speed for QSGMII ports.
*/
- if (config->portmode == PHY_INTERFACE_MODE_QSGMII)
+ if (phy_interface_num_ports(config->portmode) == 4)
mode = DEV_MAC_MODE_CFG_GIGA_MODE_ENA_SET(1);
lan_wr(config->duplex | mode,
@@ -331,10 +331,14 @@ int lan966x_port_pcs_set(struct lan966x_port *port,
struct lan966x *lan966x = port->lan966x;
bool inband_aneg = false;
bool outband;
+ bool full_preamble = false;
+
+ if (config->portmode == PHY_INTERFACE_MODE_QUSGMII)
+ full_preamble = true;
if (config->inband) {
if (config->portmode == PHY_INTERFACE_MODE_SGMII ||
- config->portmode == PHY_INTERFACE_MODE_QSGMII)
+ phy_interface_num_ports(config->portmode) == 4)
inband_aneg = true; /* Cisco-SGMII in-band-aneg */
else if (config->portmode == PHY_INTERFACE_MODE_1000BASEX &&
config->autoneg)
@@ -345,9 +349,15 @@ int lan966x_port_pcs_set(struct lan966x_port *port,
outband = true;
}
- /* Disable or enable inband */
- lan_rmw(DEV_PCS1G_MODE_CFG_SGMII_MODE_ENA_SET(outband),
- DEV_PCS1G_MODE_CFG_SGMII_MODE_ENA,
+ /* Disable or enable inband.
+ * For QUSGMII, we rely on the preamble to transmit data such as
+ * timestamps, therefore force full preamble transmission, and prevent
+ * premable shortening
+ */
+ lan_rmw(DEV_PCS1G_MODE_CFG_SGMII_MODE_ENA_SET(outband) |
+ DEV_PCS1G_MODE_CFG_SAVE_PREAMBLE_ENA_SET(full_preamble),
+ DEV_PCS1G_MODE_CFG_SGMII_MODE_ENA |
+ DEV_PCS1G_MODE_CFG_SAVE_PREAMBLE_ENA,
lan966x, DEV_PCS1G_MODE_CFG(port->chip_port));
/* Enable PCS */
@@ -396,7 +406,7 @@ void lan966x_port_init(struct lan966x_port *port)
if (lan966x->fdma)
lan966x_fdma_netdev_init(lan966x, port->dev);
- if (config->portmode != PHY_INTERFACE_MODE_QSGMII)
+ if (phy_interface_num_ports(config->portmode) != 4)
return;
lan_rmw(DEV_CLOCK_CFG_PCS_RX_RST_SET(0) |
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h b/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
index 8265ad89f0bc..c53bae5d8dbd 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
@@ -504,6 +504,12 @@ enum lan966x_target {
#define DEV_PCS1G_MODE_CFG_SGMII_MODE_ENA_GET(x)\
FIELD_GET(DEV_PCS1G_MODE_CFG_SGMII_MODE_ENA, x)
+#define DEV_PCS1G_MODE_CFG_SAVE_PREAMBLE_ENA BIT(1)
+#define DEV_PCS1G_MODE_CFG_SAVE_PREAMBLE_ENA_SET(x)\
+ FIELD_PREP(DEV_PCS1G_MODE_CFG_SAVE_PREAMBLE_ENA, x)
+#define DEV_PCS1G_MODE_CFG_SAVE_PREAMBLE_ENA_GET(x)\
+ FIELD_GET(DEV_PCS1G_MODE_CFG_SAVE_PREAMBLE_ENA, x)
+
/* DEV:PCS1G_CFG_STATUS:PCS1G_SD_CFG */
#define DEV_PCS1G_SD_CFG(t) __REG(TARGET_DEV, t, 8, 72, 0, 1, 68, 8, 0, 1, 4)
--
2.37.1
Some phy modes such as QSGMII multiplex several MAC<->PHY links on one
single physical interface. QSGMII used to be the only one supported, but
other modes such as QUSGMII also carry multiple links.
This helper allows getting the number of links that are multiplexed
on a given interface.
Signed-off-by: Maxime Chevallier <[email protected]>
---
V1->V2 : New patch
drivers/net/phy/phy-core.c | 52 ++++++++++++++++++++++++++++++++++++++
include/linux/phy.h | 2 ++
2 files changed, 54 insertions(+)
diff --git a/drivers/net/phy/phy-core.c b/drivers/net/phy/phy-core.c
index 1f2531a1a876..6029583f367d 100644
--- a/drivers/net/phy/phy-core.c
+++ b/drivers/net/phy/phy-core.c
@@ -74,6 +74,58 @@ const char *phy_duplex_to_str(unsigned int duplex)
}
EXPORT_SYMBOL_GPL(phy_duplex_to_str);
+/**
+ * phy_interface_num_ports - Return the number of links that can be carried by
+ * a given MAC-PHY physical link. Returns 0 if this is
+ * unknown, the number of links else.
+ *
+ * @interface: The interface mode we want to get the number of ports
+ */
+int phy_interface_num_ports(phy_interface_t interface)
+{
+ switch (interface) {
+ case PHY_INTERFACE_MODE_NA:
+ case PHY_INTERFACE_MODE_INTERNAL:
+ return 0;
+
+ case PHY_INTERFACE_MODE_MII:
+ case PHY_INTERFACE_MODE_GMII:
+ case PHY_INTERFACE_MODE_TBI:
+ case PHY_INTERFACE_MODE_REVMII:
+ case PHY_INTERFACE_MODE_RMII:
+ case PHY_INTERFACE_MODE_REVRMII:
+ case PHY_INTERFACE_MODE_RGMII:
+ case PHY_INTERFACE_MODE_RGMII_ID:
+ case PHY_INTERFACE_MODE_RGMII_RXID:
+ case PHY_INTERFACE_MODE_RGMII_TXID:
+ case PHY_INTERFACE_MODE_RTBI:
+ case PHY_INTERFACE_MODE_XGMII:
+ case PHY_INTERFACE_MODE_XLGMII:
+ case PHY_INTERFACE_MODE_MOCA:
+ case PHY_INTERFACE_MODE_TRGMII:
+ case PHY_INTERFACE_MODE_USXGMII:
+ case PHY_INTERFACE_MODE_SGMII:
+ case PHY_INTERFACE_MODE_SMII:
+ case PHY_INTERFACE_MODE_1000BASEX:
+ case PHY_INTERFACE_MODE_2500BASEX:
+ case PHY_INTERFACE_MODE_5GBASER:
+ case PHY_INTERFACE_MODE_10GBASER:
+ case PHY_INTERFACE_MODE_25GBASER:
+ case PHY_INTERFACE_MODE_10GKR:
+ case PHY_INTERFACE_MODE_100BASEX:
+ case PHY_INTERFACE_MODE_RXAUI:
+ case PHY_INTERFACE_MODE_XAUI:
+ return 1;
+ case PHY_INTERFACE_MODE_QSGMII:
+ case PHY_INTERFACE_MODE_QUSGMII:
+ return 4;
+
+ default:
+ return 0;
+ }
+}
+EXPORT_SYMBOL_GPL(phy_interface_num_ports);
+
/* A mapping of all SUPPORTED settings to speed/duplex. This table
* must be grouped by speed and sorted in descending match priority
* - iow, descending speed.
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 6b96b810a4d8..77e4b60bb9ab 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -967,6 +967,8 @@ struct phy_fixup {
const char *phy_speed_to_str(int speed);
const char *phy_duplex_to_str(unsigned int duplex);
+int phy_interface_num_ports(phy_interface_t interface);
+
/* A structure for mapping a particular speed and duplex
* combination to a particular SUPPORTED and ADVERTISED value
*/
--
2.37.1
Add a new QUSGMII mode, standing for "Quad Universal Serial Gigabit
Media Independent Interface", a derivative of USGMII which, similarly to
QSGMII, allows to multiplex 4 1Gbps links to a Quad-PHY.
The main difference with QSGMII is that QUSGMII can include an extension
instead of the standard 7bytes ethernet preamble, allowing to convey
arbitrary data such as Timestamps.
Signed-off-by: Maxime Chevallier <[email protected]>
Acked-by: Rob Herring <[email protected]>
---
V1->V2 : Added Rob's acked-by
Documentation/devicetree/bindings/net/ethernet-controller.yaml | 1 +
1 file changed, 1 insertion(+)
diff --git a/Documentation/devicetree/bindings/net/ethernet-controller.yaml b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
index 56d9aca8c954..eb69a720575f 100644
--- a/Documentation/devicetree/bindings/net/ethernet-controller.yaml
+++ b/Documentation/devicetree/bindings/net/ethernet-controller.yaml
@@ -67,6 +67,7 @@ properties:
- gmii
- sgmii
- qsgmii
+ - qusgmii
- tbi
- rev-mii
- rmii
--
2.37.1
The QUSGMII mode is a derivative of Cisco's USXGMII standard. This
standard is pretty similar to SGMII, but allows for faster speeds, and
has the build-in bits for Quad and Octa variants (like QSGMII).
The main difference with SGMII/QSGMII is that USXGMII/QUSGMII re-uses
the preamble to carry various information, named 'Extensions'.
As of today, the USXGMII standard only mentions the "PCH" extension,
which is used to convey timestamps, allowing in-band signaling of PTP
timestamps without having to modify the frame itself.
This commit adds support for that mode. When no extension is in use, it
behaves exactly like QSGMII, although it's not compatible with QSGMII.
Signed-off-by: Maxime Chevallier <[email protected]>
---
V1->V2 : No changes
Documentation/networking/phy.rst | 9 +++++++++
drivers/net/phy/phylink.c | 3 +++
include/linux/phy.h | 3 +++
3 files changed, 15 insertions(+)
diff --git a/Documentation/networking/phy.rst b/Documentation/networking/phy.rst
index 704f31da5167..712e44caebd0 100644
--- a/Documentation/networking/phy.rst
+++ b/Documentation/networking/phy.rst
@@ -308,6 +308,15 @@ Some of the interface modes are described below:
rate of 125Mpbs using a 4B/5B encoding scheme, resulting in an underlying
data rate of 100Mpbs.
+``PHY_INTERFACE_MODE_QUSGMII``
+ This defines the Cisco the Quad USGMII mode, which is the Quad variant of
+ the USGMII (Universal SGMII) link. It's very similar to QSGMII, but uses
+ a Packet Control Header (PCH) instead of the 7 bytes preamble to carry not
+ only the port id, but also so-called "extensions". The only documented
+ extension so-far in the specification is the inclusion of timestamps, for
+ PTP-enabled PHYs. This mode isn't compatible with QSGMII, but offers the
+ same capabilities in terms of link speed and negociation.
+
Pause frames / flow control
===========================
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 9bd69328dc4d..d2455df1d8d2 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -321,6 +321,7 @@ void phylink_get_linkmodes(unsigned long *linkmodes, phy_interface_t interface,
case PHY_INTERFACE_MODE_RGMII_ID:
case PHY_INTERFACE_MODE_RGMII:
case PHY_INTERFACE_MODE_QSGMII:
+ case PHY_INTERFACE_MODE_QUSGMII:
case PHY_INTERFACE_MODE_SGMII:
case PHY_INTERFACE_MODE_GMII:
caps |= MAC_1000HD | MAC_1000FD;
@@ -632,6 +633,7 @@ static int phylink_parse_mode(struct phylink *pl, struct fwnode_handle *fwnode)
switch (pl->link_config.interface) {
case PHY_INTERFACE_MODE_SGMII:
case PHY_INTERFACE_MODE_QSGMII:
+ case PHY_INTERFACE_MODE_QUSGMII:
phylink_set(pl->supported, 10baseT_Half);
phylink_set(pl->supported, 10baseT_Full);
phylink_set(pl->supported, 100baseT_Half);
@@ -2929,6 +2931,7 @@ void phylink_mii_c22_pcs_decode_state(struct phylink_link_state *state,
case PHY_INTERFACE_MODE_SGMII:
case PHY_INTERFACE_MODE_QSGMII:
+ case PHY_INTERFACE_MODE_QUSGMII:
phylink_decode_sgmii_word(state, lpa);
break;
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 87638c55d844..6b96b810a4d8 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -152,6 +152,7 @@ typedef enum {
PHY_INTERFACE_MODE_USXGMII,
/* 10GBASE-KR - with Clause 73 AN */
PHY_INTERFACE_MODE_10GKR,
+ PHY_INTERFACE_MODE_QUSGMII,
PHY_INTERFACE_MODE_MAX,
} phy_interface_t;
@@ -267,6 +268,8 @@ static inline const char *phy_modes(phy_interface_t interface)
return "10gbase-kr";
case PHY_INTERFACE_MODE_100BASEX:
return "100base-x";
+ case PHY_INTERFACE_MODE_QUSGMII:
+ return "qusgmii";
default:
return "unknown";
}
--
2.37.1
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 87638c55d844..6b96b810a4d8 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -152,6 +152,7 @@ typedef enum {
> PHY_INTERFACE_MODE_USXGMII,
> /* 10GBASE-KR - with Clause 73 AN */
> PHY_INTERFACE_MODE_10GKR,
> + PHY_INTERFACE_MODE_QUSGMII,
> PHY_INTERFACE_MODE_MAX,
> } phy_interface_t;
I _think_ this will give you a kerneldoc warning about
PHY_INTERFACE_MODE_QUSGMII not having any documentation?
Andrew
> +int phy_interface_num_ports(phy_interface_t interface)
> +{
> + switch (interface) {
> + case PHY_INTERFACE_MODE_NA:
> + case PHY_INTERFACE_MODE_INTERNAL:
> + return 0;
I've not yet looked at how this is used. Returning 0 could have
interesting effects i guess? INTERNAL clearly does have some sort of
path between the MAC and the PHY, so i think 1 would be a better
value. NA is less clear, it generally means Don't touch. But again,
there still needs to be a path between the MAC and PHY, otherwise
there would not be any to touch.
Why did you pick 0?
> +
> + case PHY_INTERFACE_MODE_MII:
> + case PHY_INTERFACE_MODE_GMII:
> + case PHY_INTERFACE_MODE_TBI:
> + case PHY_INTERFACE_MODE_REVMII:
> + case PHY_INTERFACE_MODE_RMII:
> + case PHY_INTERFACE_MODE_REVRMII:
> + case PHY_INTERFACE_MODE_RGMII:
> + case PHY_INTERFACE_MODE_RGMII_ID:
> + case PHY_INTERFACE_MODE_RGMII_RXID:
> + case PHY_INTERFACE_MODE_RGMII_TXID:
> + case PHY_INTERFACE_MODE_RTBI:
> + case PHY_INTERFACE_MODE_XGMII:
> + case PHY_INTERFACE_MODE_XLGMII:
> + case PHY_INTERFACE_MODE_MOCA:
> + case PHY_INTERFACE_MODE_TRGMII:
> + case PHY_INTERFACE_MODE_USXGMII:
> + case PHY_INTERFACE_MODE_SGMII:
> + case PHY_INTERFACE_MODE_SMII:
> + case PHY_INTERFACE_MODE_1000BASEX:
> + case PHY_INTERFACE_MODE_2500BASEX:
> + case PHY_INTERFACE_MODE_5GBASER:
> + case PHY_INTERFACE_MODE_10GBASER:
> + case PHY_INTERFACE_MODE_25GBASER:
> + case PHY_INTERFACE_MODE_10GKR:
> + case PHY_INTERFACE_MODE_100BASEX:
> + case PHY_INTERFACE_MODE_RXAUI:
> + case PHY_INTERFACE_MODE_XAUI:
> + return 1;
> + case PHY_INTERFACE_MODE_QSGMII:
> + case PHY_INTERFACE_MODE_QUSGMII:
> + return 4;
> +
> + default:
> + return 0;
> + }
> +}
Have you tried without a default: ? I _think_ gcc will then warn about
missing enum values, which will help future developers when they add
further values to the enum.
Andrew
On 7/28/22 14:32, Andrew Lunn wrote:
>> +int phy_interface_num_ports(phy_interface_t interface)
>> +{
>> + switch (interface) {
>> + case PHY_INTERFACE_MODE_NA:
>> + case PHY_INTERFACE_MODE_INTERNAL:
>> + return 0;
>
> I've not yet looked at how this is used. Returning 0 could have
> interesting effects i guess? INTERNAL clearly does have some sort of
> path between the MAC and the PHY, so i think 1 would be a better
> value. NA is less clear, it generally means Don't touch. But again,
> there still needs to be a path between the MAC and PHY, otherwise
> there would not be any to touch.
>
> Why did you pick 0?
I would agree that returning 1 is a more sensible default to avoid breaking users of that function. However this makes me wonder, in what case will we break the following common meaning:
- Q -> quad
- P -> penta
- O -> octal
Is the helper really needed in the sense that the phy_interface_t enumeration is explicit enough thanks to or because of its name?
--
Florian
Hello Florian, Andrew,
On Thu, 28 Jul 2022 14:44:47 -0700
Florian Fainelli <[email protected]> wrote:
> On 7/28/22 14:32, Andrew Lunn wrote:
> >> +int phy_interface_num_ports(phy_interface_t interface)
> >> +{
> >> + switch (interface) {
> >> + case PHY_INTERFACE_MODE_NA:
> >> + case PHY_INTERFACE_MODE_INTERNAL:
> >> + return 0;
> >
> > I've not yet looked at how this is used. Returning 0 could have
> > interesting effects i guess? INTERNAL clearly does have some sort of
> > path between the MAC and the PHY, so i think 1 would be a better
> > value. NA is less clear, it generally means Don't touch. But again,
> > there still needs to be a path between the MAC and PHY, otherwise
> > there would not be any to touch.
> >
> > Why did you pick 0?
My reasonning was that PHY_INTERNAL is likely a custom solution to link
IPs existing on the same die, so nothing prevents vendors from
multiplexing links on that interface. But it's a far-fetched
reasonning, so 1 can be good, as other interfaces that are meant to be
used on-die like XGMII.
> I would agree that returning 1 is a more sensible default to avoid
> breaking users of that function. However this makes me wonder, in
> what case will we break the following common meaning:
>
> - Q -> quad
> - P -> penta
> - O -> octal
>
> Is the helper really needed in the sense that the phy_interface_t
> enumeration is explicit enough thanks to or because of its name? --
> Florian
Good question actually ! It started as a point from Russell proposing a
helper to get the number of serdes lanes for a given interface, but
this sisn't quite fit the use-case, which was simply to simplify
if (interface == PHY_INTERFACE_MODE_QSGMII ||
interface == PHY_INTERFACE_MODE_QUSGMII)
into
if (phy_interface_num_ports(interface) == 4)
But this a slim simplification at the cost of a new helper to maintain,
so I can repove that if you want.
Thanks for the reviews,
Maxime
On Thu, 28 Jul 2022 23:32:36 +0200
Andrew Lunn <[email protected]> wrote:
> > +int phy_interface_num_ports(phy_interface_t interface)
> > +{
> > + switch (interface) {
> > + case PHY_INTERFACE_MODE_NA:
> > + case PHY_INTERFACE_MODE_INTERNAL:
> > + return 0;
>
> I've not yet looked at how this is used. Returning 0 could have
> interesting effects i guess? INTERNAL clearly does have some sort of
> path between the MAC and the PHY, so i think 1 would be a better
> value. NA is less clear, it generally means Don't touch. But again,
> there still needs to be a path between the MAC and PHY, otherwise
> there would not be any to touch.
>
> Why did you pick 0?
>
> > +
> > + case PHY_INTERFACE_MODE_MII:
> > + case PHY_INTERFACE_MODE_GMII:
> > + case PHY_INTERFACE_MODE_TBI:
> > + case PHY_INTERFACE_MODE_REVMII:
> > + case PHY_INTERFACE_MODE_RMII:
> > + case PHY_INTERFACE_MODE_REVRMII:
> > + case PHY_INTERFACE_MODE_RGMII:
> > + case PHY_INTERFACE_MODE_RGMII_ID:
> > + case PHY_INTERFACE_MODE_RGMII_RXID:
> > + case PHY_INTERFACE_MODE_RGMII_TXID:
> > + case PHY_INTERFACE_MODE_RTBI:
> > + case PHY_INTERFACE_MODE_XGMII:
> > + case PHY_INTERFACE_MODE_XLGMII:
> > + case PHY_INTERFACE_MODE_MOCA:
> > + case PHY_INTERFACE_MODE_TRGMII:
> > + case PHY_INTERFACE_MODE_USXGMII:
> > + case PHY_INTERFACE_MODE_SGMII:
> > + case PHY_INTERFACE_MODE_SMII:
> > + case PHY_INTERFACE_MODE_1000BASEX:
> > + case PHY_INTERFACE_MODE_2500BASEX:
> > + case PHY_INTERFACE_MODE_5GBASER:
> > + case PHY_INTERFACE_MODE_10GBASER:
> > + case PHY_INTERFACE_MODE_25GBASER:
> > + case PHY_INTERFACE_MODE_10GKR:
> > + case PHY_INTERFACE_MODE_100BASEX:
> > + case PHY_INTERFACE_MODE_RXAUI:
> > + case PHY_INTERFACE_MODE_XAUI:
> > + return 1;
> > + case PHY_INTERFACE_MODE_QSGMII:
> > + case PHY_INTERFACE_MODE_QUSGMII:
> > + return 4;
> > +
> > + default:
> > + return 0;
> > + }
> > +}
>
> Have you tried without a default: ? I _think_ gcc will then warn about
> missing enum values, which will help future developers when they add
> further values to the enum.
Without the default clause, I get an error about the missing
PHY_INTERFACE_MODE_MAX case, which I don't think belongs here...
Too bad :/
> Andrew
On Fri, Jul 29, 2022 at 09:32:52AM +0200, Maxime Chevallier wrote:
> On Thu, 28 Jul 2022 23:32:36 +0200
> Andrew Lunn <[email protected]> wrote:
>
> > > +int phy_interface_num_ports(phy_interface_t interface)
> > > +{
> > > + switch (interface) {
> > > + case PHY_INTERFACE_MODE_NA:
> > > + case PHY_INTERFACE_MODE_INTERNAL:
> > > + return 0;
> >
> > I've not yet looked at how this is used. Returning 0 could have
> > interesting effects i guess? INTERNAL clearly does have some sort of
> > path between the MAC and the PHY, so i think 1 would be a better
> > value. NA is less clear, it generally means Don't touch. But again,
> > there still needs to be a path between the MAC and PHY, otherwise
> > there would not be any to touch.
> >
> > Why did you pick 0?
> >
> > > +
> > > + case PHY_INTERFACE_MODE_MII:
> > > + case PHY_INTERFACE_MODE_GMII:
> > > + case PHY_INTERFACE_MODE_TBI:
> > > + case PHY_INTERFACE_MODE_REVMII:
> > > + case PHY_INTERFACE_MODE_RMII:
> > > + case PHY_INTERFACE_MODE_REVRMII:
> > > + case PHY_INTERFACE_MODE_RGMII:
> > > + case PHY_INTERFACE_MODE_RGMII_ID:
> > > + case PHY_INTERFACE_MODE_RGMII_RXID:
> > > + case PHY_INTERFACE_MODE_RGMII_TXID:
> > > + case PHY_INTERFACE_MODE_RTBI:
> > > + case PHY_INTERFACE_MODE_XGMII:
> > > + case PHY_INTERFACE_MODE_XLGMII:
> > > + case PHY_INTERFACE_MODE_MOCA:
> > > + case PHY_INTERFACE_MODE_TRGMII:
> > > + case PHY_INTERFACE_MODE_USXGMII:
> > > + case PHY_INTERFACE_MODE_SGMII:
> > > + case PHY_INTERFACE_MODE_SMII:
> > > + case PHY_INTERFACE_MODE_1000BASEX:
> > > + case PHY_INTERFACE_MODE_2500BASEX:
> > > + case PHY_INTERFACE_MODE_5GBASER:
> > > + case PHY_INTERFACE_MODE_10GBASER:
> > > + case PHY_INTERFACE_MODE_25GBASER:
> > > + case PHY_INTERFACE_MODE_10GKR:
> > > + case PHY_INTERFACE_MODE_100BASEX:
> > > + case PHY_INTERFACE_MODE_RXAUI:
> > > + case PHY_INTERFACE_MODE_XAUI:
> > > + return 1;
> > > + case PHY_INTERFACE_MODE_QSGMII:
> > > + case PHY_INTERFACE_MODE_QUSGMII:
> > > + return 4;
> > > +
> > > + default:
> > > + return 0;
> > > + }
> > > +}
> >
> > Have you tried without a default: ? I _think_ gcc will then warn about
> > missing enum values, which will help future developers when they add
> > further values to the enum.
>
> Without the default clause, I get an error about the missing
> PHY_INTERFACE_MODE_MAX case, which I don't think belongs here...
case PHY_INTERFACE_MODE_MAX:
WARN_ONCE()
return 0;
break;
Being passed PHY_INTERFACE_MODE_MAX is a bug in itself, so warning
seems sensible.
Andrew