From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-12-14 14:47:33
Hi,
This series allows phylink to include the PCS in its validation step.
There are two reasons to make this change:
1. Some of the network drivers that are making use of the split PCS
support are already manually calling into their PCS drivers to
perform validation. E.g. stmmac with xpcs.
2. Logically, some network drivers such as mvneta and mvpp2, the
restriction we impose in the validate() callback is a property of
the "PCS" block that we provide rather than the MAC.
This series:
1. Gives phylink a mechanism to query the MAC driver which PCS is
wishes to use for the PHY interface mode. This is necessary to allow
the PCS to be involved in the validation step without making changes
to the configuration.
2. Provide a pcs_validate() method that PCS can implement. This follows
a similar model to the MAC's validate() callback, but with some minor
differences due to observations from the various implementations.
E.g. returning an error code for not-supported and the way the
advertising bitmap is masked.
3. Convert mvpp2 and mvneta to this as examples of its use. Further
Conversions are in the pipeline, including for stmmac+xpcs, as well
as some DSA drivers. Note that DSA conversion to this is conditional
upon all DSA drivers populating their supported_interfaces bitmap,
since this is required before mac_select_pcs() can be used.
Existing drivers that set a PCS in mac_prepare() or mac_config(), or
shortly after phylink_create() will continue to work. However, it should
be noted that mac_select_pcs() will be called during phylink_create(),
and thus any PCS returned by mac_select_pcs() must be available by this
time - or we drop the check in phylink_create().
drivers/net/ethernet/marvell/mvneta.c | 229 ++++++++++++++++--------
drivers/net/ethernet/marvell/mvpp2/mvpp2.h | 3 +-
drivers/net/ethernet/marvell/mvpp2/mvpp2_main.c | 112 ++++++------
drivers/net/phy/phylink.c | 99 +++++++++-
include/linux/phylink.h | 38 ++++
5 files changed, 337 insertions(+), 144 deletions(-)
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
From: Russell King (Oracle) <hidden> Date: 2021-12-14 14:48:10
mac_select_pcs() allows us to have an explicit point to query which
PCS the MAC wishes to use for a particular PHY interface mode, thereby
allowing us to add support to validate the link settings with the PCS.
Phylink will also use this to select the PCS to be used during a major
configuration event without the MAC driver needing to call
phylink_set_pcs().
Note that if mac_select_pcs() is present, the supported_interfaces
bitmap must be filled in; this avoids mac_select_pcs() being called
with PHY_INTERFACE_MODE_NA when we want to get support for all
interface types. Phylink will return an error in phylink_create()
unless this condition is satisfied.
Signed-off-by: Russell King (Oracle) <redacted>
---
drivers/net/phy/phylink.c | 68 +++++++++++++++++++++++++++++++++------
include/linux/phylink.h | 18 +++++++++++
2 files changed, 77 insertions(+), 9 deletions(-)
@@ -764,6 +791,12 @@ static void phylink_major_config(struct phylink *pl, bool restart,}}+/* If we have a new PCS, switch to the new PCS after preparing the MAC+*forthechange.+*/+if(pcs)+phylink_set_pcs(pl,pcs);+phylink_mac_config(pl,state);if(pl->pcs_ops){
From: Russell King (Oracle) <hidden> Date: 2021-12-14 14:48:14
Add a hook for PCS to validate the link parameters. This avoids MAC
drivers having to have knowledge of their PCS in their validate()
method, thereby allowing several MAC drivers to be simplfied.
Signed-off-by: Russell King (Oracle) <redacted>
---
drivers/net/phy/phylink.c | 31 +++++++++++++++++++++++++++++++
include/linux/phylink.h | 20 ++++++++++++++++++++
2 files changed, 51 insertions(+)
@@ -424,13 +424,44 @@ static int phylink_validate_mac_and_pcs(struct phylink *pl,structphylink_link_state*state){structphylink_pcs*pcs;+intret;+/* Get the PCS for this interface mode */if(pl->mac_ops->mac_select_pcs){pcs=pl->mac_ops->mac_select_pcs(pl->config,state->interface);if(IS_ERR(pcs))returnPTR_ERR(pcs);+}else{+pcs=pl->pcs;+}++if(pcs){+/* The PCS, if present, must be setup before phylink_create()+*hasbeencalled.Iftheopsisnotinitialised,printan+*errorandbacktraceratherthanoopsingthekernel.+*/+if(!pcs->ops){+phylink_err(pl,"interface %s: uninitialised PCS\n",+phy_modes(state->interface));+dump_stack();+return-EINVAL;+}++/* Validate the link parameters with the PCS */+if(pcs->ops->pcs_validate){+ret=pcs->ops->pcs_validate(pcs,supported,state);+if(ret<0||phylink_is_empty_linkmode(supported))+return-EINVAL;++/* Ensure the advertising mask is a subset of the+*supportedmask.+*/+linkmode_and(state->advertising,state->advertising,+supported);+}}+/* Then validate the link parameters with the MAC */pl->mac_ops->validate(pl->config,supported,state);returnphylink_is_empty_linkmode(supported)?-EINVAL:0;
From: Russell King (Oracle) <hidden> Date: 2021-12-14 14:48:20
Use the mac_select_pcs() method to choose between the GMAC and XLG
PCS implementations.
Signed-off-by: Russell King (Oracle) <redacted>
---
drivers/net/ethernet/marvell/mvpp2/mvpp2.h | 3 +-
.../net/ethernet/marvell/mvpp2/mvpp2_main.c | 75 ++++++++++---------
2 files changed, 42 insertions(+), 36 deletions(-)
@@ -6201,7 +6206,7 @@ static int mvpp2_gmac_pcs_config(struct phylink_pcs *pcs, unsigned int mode,constunsignedlong*advertising,boolpermit_pause_to_mac){-structmvpp2_port*port=mvpp2_pcs_to_port(pcs);+structmvpp2_port*port=mvpp2_pcs_gmac_to_port(pcs);u32mask,val,an,old_an,changed;mask=MVPP2_GMAC_IN_BAND_AUTONEG_BYPASS|
@@ -6255,7 +6260,7 @@ static int mvpp2_gmac_pcs_config(struct phylink_pcs *pcs, unsigned int mode,staticvoidmvpp2_gmac_pcs_an_restart(structphylink_pcs*pcs){-structmvpp2_port*port=mvpp2_pcs_to_port(pcs);+structmvpp2_port*port=mvpp2_pcs_gmac_to_port(pcs);u32val=readl(port->base+MVPP2_GMAC_AUTONEG_CONFIG);writel(val|MVPP2_GMAC_IN_BAND_RESTART_AN,
@@ -6368,8 +6373,23 @@ static void mvpp2_gmac_config(struct mvpp2_port *port, unsigned int mode,writel(ctrl4,port->base+MVPP22_GMAC_CTRL_4_REG);}-staticintmvpp2__mac_prepare(structphylink_config*config,unsignedintmode,-phy_interface_tinterface)+staticstructphylink_pcs*mvpp2_select_pcs(structphylink_config*config,+phy_interface_tinterface)+{+structmvpp2_port*port=mvpp2_phylink_to_port(config);++/* Select the appropriate PCS operations depending on the+*configuredinterfacemode.Wewillonlyswitchtoamode+*thatthevalidate()checkshavealreadypassed.+*/+if(mvpp2_is_xlg(interface))+return&port->pcs_xlg;+else+return&port->pcs_gmac;+}++staticintmvpp2_mac_prepare(structphylink_config*config,unsignedintmode,+phy_interface_tinterface){structmvpp2_port*port=mvpp2_phylink_to_port(config);
@@ -6418,31 +6438,9 @@ static int mvpp2__mac_prepare(struct phylink_config *config, unsigned int mode,}}-/* Select the appropriate PCS operations depending on the-*configuredinterfacemode.Wewillonlyswitchtoamode-*thatthevalidate()checkshavealreadypassed.-*/-if(mvpp2_is_xlg(interface))-port->phylink_pcs.ops=&mvpp2_phylink_xlg_pcs_ops;-else-port->phylink_pcs.ops=&mvpp2_phylink_gmac_pcs_ops;-return0;}-staticintmvpp2_mac_prepare(structphylink_config*config,unsignedintmode,-phy_interface_tinterface)-{-structmvpp2_port*port=mvpp2_phylink_to_port(config);-intret;--ret=mvpp2__mac_prepare(config,mode,interface);-if(ret==0)-phylink_set_pcs(port->phylink,&port->phylink_pcs);--returnret;-}-staticvoidmvpp2_mac_config(structphylink_config*config,unsignedintmode,conststructphylink_link_state*state){
From: Russell King (Oracle) <hidden> Date: 2021-12-14 14:48:29
Convert mvpp2 to validate the autoneg state for 1000base-X in the
pcs_validate() operation, rather than the MAC validate() operation.
This allows us to switch the MAC validate() to use
phylink_generic_validate().
Signed-off-by: Russell King (Oracle) <redacted>
---
.../net/ethernet/marvell/mvpp2/mvpp2_main.c | 37 +++++++++----------
1 file changed, 17 insertions(+), 20 deletions(-)
@@ -6166,6 +6166,21 @@ static const struct phylink_pcs_ops mvpp2_phylink_xlg_pcs_ops = {.pcs_config=mvpp2_xlg_pcs_config,};+staticintmvpp2_gmac_pcs_validate(structphylink_pcs*pcs,+unsignedlong*supported,+conststructphylink_link_state*state)+{+/* When in 802.3z mode, we must have AN enabled:+*Bit2FieldInBandAnEnIn-bandAuto-Negotiationenable....+*When<PortType>=1(1000BASE-X)thisfieldmustbesetto1.+*/+if(phy_interface_mode_is_8023z(state->interface)&&+!phylink_test(state->advertising,Autoneg))+return-EINVAL;++return0;+}+staticvoidmvpp2_gmac_pcs_get_state(structphylink_pcs*pcs,structphylink_link_state*state){
@@ -6270,30 +6285,12 @@ static void mvpp2_gmac_pcs_an_restart(struct phylink_pcs *pcs)}staticconststructphylink_pcs_opsmvpp2_phylink_gmac_pcs_ops={+.pcs_validate=mvpp2_gmac_pcs_validate,.pcs_get_state=mvpp2_gmac_pcs_get_state,.pcs_config=mvpp2_gmac_pcs_config,.pcs_an_restart=mvpp2_gmac_pcs_an_restart,};-staticvoidmvpp2_phylink_validate(structphylink_config*config,-unsignedlong*supported,-structphylink_link_state*state)-{-/* When in 802.3z mode, we must have AN enabled:-*Bit2FieldInBandAnEnIn-bandAuto-Negotiationenable....-*When<PortType>=1(1000BASE-X)thisfieldmustbesetto1.-*/-if(phy_interface_mode_is_8023z(state->interface)&&-!phylink_test(state->advertising,Autoneg))-gotoempty_set;--phylink_generic_validate(config,supported,state);-return;--empty_set:-linkmode_zero(supported);-}-staticvoidmvpp2_xlg_config(structmvpp2_port*port,unsignedintmode,conststructphylink_link_state*state){
From: Russell King <hidden> Date: 2021-12-14 14:48:30
Convert mvneta to use the mac_prepare() and mac_finish() methods in
preparation to converting mvneta to split-PCS support.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/marvell/mvneta.c | 103 +++++++++++++++++---------
1 file changed, 68 insertions(+), 35 deletions(-)
@@ -3905,6 +3905,40 @@ static void mvneta_mac_an_restart(struct phylink_config *config)gmac_an&~MVNETA_GMAC_INBAND_RESTART_AN);}+staticintmvneta_mac_prepare(structphylink_config*config,unsignedintmode,+phy_interface_tinterface)+{+structnet_device*ndev=to_net_dev(config->dev);+structmvneta_port*pp=netdev_priv(ndev);+u32val;++if(pp->phy_interface!=interface||+phylink_autoneg_inband(mode)){+/* Force the link down when changing the interface or if in+*in-bandmode.AccordingtoArmada370documentation,we+*canonlychangetheportmodeandin-bandenablewhenthe+*linkisdown.+*/+val=mvreg_read(pp,MVNETA_GMAC_AUTONEG_CONFIG);+val&=~MVNETA_GMAC_FORCE_LINK_PASS;+val|=MVNETA_GMAC_FORCE_LINK_DOWN;+mvreg_write(pp,MVNETA_GMAC_AUTONEG_CONFIG,val);+}++if(pp->phy_interface!=interface)+WARN_ON(phy_power_off(pp->comphy));++/* Enable the 1ms clock */+if(phylink_autoneg_inband(mode)){+unsignedlongrate=clk_get_rate(pp->clk);++mvreg_write(pp,MVNETA_GMAC_CLOCK_DIVIDER,+MVNETA_GMAC_1MS_CLOCK_ENABLE|(rate/1000));+}++return0;+}+staticvoidmvneta_mac_config(structphylink_config*config,unsignedintmode,conststructphylink_link_state*state){
@@ -3948,10 +3980,7 @@ static void mvneta_mac_config(struct phylink_config *config, unsigned int mode,}elseif(state->interface==PHY_INTERFACE_MODE_SGMII){/* SGMII mode receives the state from the PHY */new_ctrl2|=MVNETA_GMAC2_INBAND_AN_ENABLE;-new_clk=MVNETA_GMAC_1MS_CLOCK_ENABLE;-new_an=(new_an&~(MVNETA_GMAC_FORCE_LINK_DOWN|-MVNETA_GMAC_FORCE_LINK_PASS|-MVNETA_GMAC_CONFIG_MII_SPEED|+new_an=(new_an&~(MVNETA_GMAC_CONFIG_MII_SPEED|MVNETA_GMAC_CONFIG_GMII_SPEED|MVNETA_GMAC_CONFIG_FULL_DUPLEX))|MVNETA_GMAC_INBAND_AN_ENABLE|
@@ -3960,10 +3989,7 @@ static void mvneta_mac_config(struct phylink_config *config, unsigned int mode,}else{/* 802.3z negotiation - only 1000base-X */new_ctrl0|=MVNETA_GMAC0_PORT_1000BASE_X;-new_clk=MVNETA_GMAC_1MS_CLOCK_ENABLE;-new_an=(new_an&~(MVNETA_GMAC_FORCE_LINK_DOWN|-MVNETA_GMAC_FORCE_LINK_PASS|-MVNETA_GMAC_CONFIG_MII_SPEED))|+new_an=(new_an&~MVNETA_GMAC_CONFIG_MII_SPEED)|MVNETA_GMAC_INBAND_AN_ENABLE|MVNETA_GMAC_CONFIG_GMII_SPEED|/* The MAC only supports FD mode */
@@ -3973,43 +3999,18 @@ static void mvneta_mac_config(struct phylink_config *config, unsigned int mode,new_an|=MVNETA_GMAC_AN_FLOW_CTRL_EN;}-/* Set the 1ms clock divisor */-if(new_clk==MVNETA_GMAC_1MS_CLOCK_ENABLE)-new_clk|=clk_get_rate(pp->clk)/1000;--/* Armada 370 documentation says we can only change the port mode-*andin-bandenablewhenthelinkisdown,soforceitdown-*whilemakingthesechanges.WealsodothisforGMAC_CTRL2-*/-if((new_ctrl0^gmac_ctrl0)&MVNETA_GMAC0_PORT_1000BASE_X||-(new_ctrl2^gmac_ctrl2)&MVNETA_GMAC2_INBAND_AN_ENABLE||-(new_an^gmac_an)&MVNETA_GMAC_INBAND_AN_ENABLE){-mvreg_write(pp,MVNETA_GMAC_AUTONEG_CONFIG,-(gmac_an&~MVNETA_GMAC_FORCE_LINK_PASS)|-MVNETA_GMAC_FORCE_LINK_DOWN);-}--/* When at 2.5G, the link partner can send frames with shortened*preambles.*/if(state->interface==PHY_INTERFACE_MODE_2500BASEX)new_ctrl4|=MVNETA_GMAC4_SHORT_PREAMBLE_ENABLE;-if(pp->phy_interface!=state->interface){-if(pp->comphy)-WARN_ON(phy_power_off(pp->comphy));-WARN_ON(mvneta_config_interface(pp,state->interface));-}-if(new_ctrl0!=gmac_ctrl0)mvreg_write(pp,MVNETA_GMAC_CTRL_0,new_ctrl0);if(new_ctrl2!=gmac_ctrl2)mvreg_write(pp,MVNETA_GMAC_CTRL_2,new_ctrl2);if(new_ctrl4!=gmac_ctrl4)mvreg_write(pp,MVNETA_GMAC_CTRL_4,new_ctrl4);-if(new_clk!=gmac_clk)-mvreg_write(pp,MVNETA_GMAC_CLOCK_DIVIDER,new_clk);if(new_an!=gmac_an)mvreg_write(pp,MVNETA_GMAC_AUTONEG_CONFIG,new_an);
@@ -4020,6 +4021,36 @@ static void mvneta_mac_config(struct phylink_config *config, unsigned int mode,}}+staticintmvneta_mac_finish(structphylink_config*config,unsignedintmode,+phy_interface_tinterface)+{+structnet_device*ndev=to_net_dev(config->dev);+structmvneta_port*pp=netdev_priv(ndev);+u32val,clk;++/* Disable 1ms clock if not in in-band mode */+if(!phylink_autoneg_inband(mode)){+clk=mvreg_read(pp,MVNETA_GMAC_CLOCK_DIVIDER);+clk&=~MVNETA_GMAC_1MS_CLOCK_ENABLE;+mvreg_write(pp,MVNETA_GMAC_CLOCK_DIVIDER,clk);+}++if(pp->phy_interface!=interface)+/* Enable the Serdes PHY */+WARN_ON(mvneta_config_interface(pp,interface));++/* Allow the link to come up if in in-band mode, otherwise the+*linkisforcedviamac_link_down()/mac_link_up()+*/+if(phylink_autoneg_inband(mode)){+val=mvreg_read(pp,MVNETA_GMAC_AUTONEG_CONFIG);+val&=~MVNETA_GMAC_FORCE_LINK_DOWN;+mvreg_write(pp,MVNETA_GMAC_AUTONEG_CONFIG,val);+}++return0;+}+staticvoidmvneta_set_eee(structmvneta_port*pp,boolenable){u32lpi_ctl1;
From: Russell King <hidden> Date: 2021-12-14 14:48:37
An initial stab at converting mvneta to PCS operations. There's a few
FIXMEs to be solved.
Signed-off-by: Russell King <redacted>
---
drivers/net/ethernet/marvell/mvneta.c | 143 ++++++++++++++++----------
1 file changed, 91 insertions(+), 52 deletions(-)
@@ -3846,29 +3847,15 @@ static int mvneta_set_mac_addr(struct net_device *dev, void *addr)return0;}-staticvoidmvneta_validate(structphylink_config*config,-unsignedlong*supported,-structphylink_link_state*state)+staticstructmvneta_port*mvneta_pcs_to_port(structphylink_pcs*pcs){-/* We only support QSGMII, SGMII, 802.3z and RGMII modes.-*Whenin802.3zmode,wemusthaveANenabled:-*"Bit 2 Field InBandAnEn In-band Auto-Negotiation enable. ...-*When<PortType>=1(1000BASE-X)thisfieldmustbesetto1."-*/-if(phy_interface_mode_is_8023z(state->interface)&&-!phylink_test(state->advertising,Autoneg)){-linkmode_zero(supported);-return;-}--phylink_generic_validate(config,supported,state);+returncontainer_of(pcs,structmvneta_port,phylink_pcs);}-staticvoidmvneta_mac_pcs_get_state(structphylink_config*config,-structphylink_link_state*state)+staticvoidmvneta_pcs_get_state(structphylink_pcs*pcs,+structphylink_link_state*state){-structnet_device*ndev=to_net_dev(config->dev);-structmvneta_port*pp=netdev_priv(ndev);+structmvneta_port*pp=mvneta_pcs_to_port(pcs);u32gmac_stat;gmac_stat=mvreg_read(pp,MVNETA_GMAC_STATUS);
@@ -3886,17 +3873,71 @@ static void mvneta_mac_pcs_get_state(struct phylink_config *config,state->link=!!(gmac_stat&MVNETA_GMAC_LINK_UP);state->duplex=!!(gmac_stat&MVNETA_GMAC_FULL_DUPLEX);-state->pause=0;if(gmac_stat&MVNETA_GMAC_RX_FLOW_CTRL_ENABLE)state->pause|=MLO_PAUSE_RX;if(gmac_stat&MVNETA_GMAC_TX_FLOW_CTRL_ENABLE)state->pause|=MLO_PAUSE_TX;}-staticvoidmvneta_mac_an_restart(structphylink_config*config)+staticintmvneta_pcs_config(structphylink_pcs*pcs,+unsignedintmode,phy_interface_tinterface,+constunsignedlong*advertising,+boolpermit_pause_to_mac){-structnet_device*ndev=to_net_dev(config->dev);-structmvneta_port*pp=netdev_priv(ndev);+structmvneta_port*pp=mvneta_pcs_to_port(pcs);+u32mask,val,an,old_an,changed;++mask=MVNETA_GMAC_INBAND_AN_ENABLE|+MVNETA_GMAC_INBAND_RESTART_AN|+MVNETA_GMAC_AN_SPEED_EN|+MVNETA_GMAC_AN_FLOW_CTRL_EN|+MVNETA_GMAC_AN_DUPLEX_EN;++if(phylink_autoneg_inband(mode)){+mask|=MVNETA_GMAC_CONFIG_MII_SPEED|+MVNETA_GMAC_CONFIG_GMII_SPEED|+MVNETA_GMAC_CONFIG_FULL_DUPLEX;+val=MVNETA_GMAC_INBAND_AN_ENABLE;++if(interface==PHY_INTERFACE_MODE_SGMII){+/* SGMII mode receives the speed and duplex from PHY */+val|=MVNETA_GMAC_AN_SPEED_EN|+MVNETA_GMAC_AN_DUPLEX_EN;+}else{+/* 802.3z mode has fixed speed and duplex */+val|=MVNETA_GMAC_CONFIG_GMII_SPEED|+MVNETA_GMAC_CONFIG_FULL_DUPLEX;++/* The FLOW_CTRL_EN bit selects either the hardware+*automaticallyortheCONFIG_FLOW_CTRLmanually+*controlstheGMACpausemode.+*/+if(permit_pause_to_mac)+val|=MVNETA_GMAC_AN_FLOW_CTRL_EN;++/* Update the advertisement bits */+mask|=MVNETA_GMAC_ADVERT_SYM_FLOW_CTRL;+if(phylink_test(advertising,Pause))+val|=MVNETA_GMAC_ADVERT_SYM_FLOW_CTRL;+}+}else{+/* Phy or fixed speed - disable in-band AN modes */+val=0;+}++old_an=an=mvreg_read(pp,MVNETA_GMAC_AUTONEG_CONFIG);+an=(an&~mask)|val;+changed=old_an^an;+if(changed)+mvreg_write(pp,MVNETA_GMAC_AUTONEG_CONFIG,an);++/* We are only interested in the advertisement bits changing */+return!!(changed&MVNETA_GMAC_ADVERT_SYM_FLOW_CTRL);+}++staticvoidmvneta_pcs_an_restart(structphylink_pcs*pcs)+{+structmvneta_port*pp=mvneta_pcs_to_port(pcs);u32gmac_an=mvreg_read(pp,MVNETA_GMAC_AUTONEG_CONFIG);mvreg_write(pp,MVNETA_GMAC_AUTONEG_CONFIG,
@@ -3905,6 +3946,30 @@ static void mvneta_mac_an_restart(struct phylink_config *config)gmac_an&~MVNETA_GMAC_INBAND_RESTART_AN);}+staticconststructphylink_pcs_opsmvneta_phylink_pcs_ops={+.pcs_get_state=mvneta_pcs_get_state,+.pcs_config=mvneta_pcs_config,+.pcs_an_restart=mvneta_pcs_an_restart,+};++staticvoidmvneta_validate(structphylink_config*config,+unsignedlong*supported,+structphylink_link_state*state)+{+/* We only support QSGMII, SGMII, 802.3z and RGMII modes.+*Whenin802.3zmode,wemusthaveANenabled:+*"Bit 2 Field InBandAnEn In-band Auto-Negotiation enable. ...+*When<PortType>=1(1000BASE-X)thisfieldmustbesetto1."+*/+if(phy_interface_mode_is_8023z(state->interface)&&+!phylink_test(state->advertising,Autoneg)){+linkmode_zero(supported);+return;+}++phylink_generic_validate(config,supported,state);+}+staticintmvneta_mac_prepare(structphylink_config*config,unsignedintmode,phy_interface_tinterface){
@@ -3947,18 +4012,11 @@ static void mvneta_mac_config(struct phylink_config *config, unsigned int mode,u32new_ctrl0,gmac_ctrl0=mvreg_read(pp,MVNETA_GMAC_CTRL_0);u32new_ctrl2,gmac_ctrl2=mvreg_read(pp,MVNETA_GMAC_CTRL_2);u32new_ctrl4,gmac_ctrl4=mvreg_read(pp,MVNETA_GMAC_CTRL_4);-u32new_an,gmac_an=mvreg_read(pp,MVNETA_GMAC_AUTONEG_CONFIG);new_ctrl0=gmac_ctrl0&~MVNETA_GMAC0_PORT_1000BASE_X;new_ctrl2=gmac_ctrl2&~(MVNETA_GMAC2_INBAND_AN_ENABLE|MVNETA_GMAC2_PORT_RESET);new_ctrl4=gmac_ctrl4&~(MVNETA_GMAC4_SHORT_PREAMBLE_ENABLE);-new_an=gmac_an&~(MVNETA_GMAC_INBAND_AN_ENABLE|-MVNETA_GMAC_INBAND_RESTART_AN|-MVNETA_GMAC_AN_SPEED_EN|-MVNETA_GMAC_ADVERT_SYM_FLOW_CTRL|-MVNETA_GMAC_AN_FLOW_CTRL_EN|-MVNETA_GMAC_AN_DUPLEX_EN);/* Even though it might look weird, when we're configured in*SGMIIorQSGMIImode,theRGMIIbitneedstobeset.
@@ -3970,9 +4028,6 @@ static void mvneta_mac_config(struct phylink_config *config, unsigned int mode,phy_interface_mode_is_8023z(state->interface))new_ctrl2|=MVNETA_GMAC2_PCS_ENABLE;-if(phylink_test(state->advertising,Pause))-new_an|=MVNETA_GMAC_ADVERT_SYM_FLOW_CTRL;-if(!phylink_autoneg_inband(mode)){/* Phy or fixed speed - nothing to do, leave the*configuredspeed,duplexandflowcontrolas-is.
@@ -3980,23 +4035,9 @@ static void mvneta_mac_config(struct phylink_config *config, unsigned int mode,}elseif(state->interface==PHY_INTERFACE_MODE_SGMII){/* SGMII mode receives the state from the PHY */new_ctrl2|=MVNETA_GMAC2_INBAND_AN_ENABLE;-new_an=(new_an&~(MVNETA_GMAC_CONFIG_MII_SPEED|-MVNETA_GMAC_CONFIG_GMII_SPEED|-MVNETA_GMAC_CONFIG_FULL_DUPLEX))|-MVNETA_GMAC_INBAND_AN_ENABLE|-MVNETA_GMAC_AN_SPEED_EN|-MVNETA_GMAC_AN_DUPLEX_EN;}else{/* 802.3z negotiation - only 1000base-X */new_ctrl0|=MVNETA_GMAC0_PORT_1000BASE_X;-new_an=(new_an&~MVNETA_GMAC_CONFIG_MII_SPEED)|-MVNETA_GMAC_INBAND_AN_ENABLE|-MVNETA_GMAC_CONFIG_GMII_SPEED|-/* The MAC only supports FD mode */-MVNETA_GMAC_CONFIG_FULL_DUPLEX;--if(state->pause&MLO_PAUSE_AN&&state->an_enabled)-new_an|=MVNETA_GMAC_AN_FLOW_CTRL_EN;}/* When at 2.5G, the link partner can send frames with shortened
From: Russell King (Oracle) <hidden> Date: 2021-12-14 14:48:41
Convert mvneta to validate the autoneg state for 1000base-X in the
pcs_validate() operation, rather than the MAC validate() operation.
This allows us to switch the MAC validate() to use
phylink_generic_validate().
Signed-off-by: Russell King (Oracle) <redacted>
---
drivers/net/ethernet/marvell/mvneta.c | 37 +++++++++++++--------------
1 file changed, 18 insertions(+), 19 deletions(-)
@@ -3852,6 +3852,22 @@ static struct mvneta_port *mvneta_pcs_to_port(struct phylink_pcs *pcs)returncontainer_of(pcs,structmvneta_port,phylink_pcs);}+staticintmvneta_pcs_validate(structphylink_pcs*pcs,+unsignedlong*supported,+conststructphylink_link_state*state)+{+/* We only support QSGMII, SGMII, 802.3z and RGMII modes.+*Whenin802.3zmode,wemusthaveANenabled:+*"Bit 2 Field InBandAnEn In-band Auto-Negotiation enable. ...+*When<PortType>=1(1000BASE-X)thisfieldmustbesetto1."+*/+if(phy_interface_mode_is_8023z(state->interface)&&+!phylink_test(state->advertising,Autoneg))+return-EINVAL;++return0;+}+staticvoidmvneta_pcs_get_state(structphylink_pcs*pcs,structphylink_link_state*state){
@@ -3947,29 +3963,12 @@ static void mvneta_pcs_an_restart(struct phylink_pcs *pcs)}staticconststructphylink_pcs_opsmvneta_phylink_pcs_ops={+.pcs_validate=mvneta_pcs_validate,.pcs_get_state=mvneta_pcs_get_state,.pcs_config=mvneta_pcs_config,.pcs_an_restart=mvneta_pcs_an_restart,};-staticvoidmvneta_validate(structphylink_config*config,-unsignedlong*supported,-structphylink_link_state*state)-{-/* We only support QSGMII, SGMII, 802.3z and RGMII modes.-*Whenin802.3zmode,wemusthaveANenabled:-*"Bit 2 Field InBandAnEn In-band Auto-Negotiation enable. ...-*When<PortType>=1(1000BASE-X)thisfieldmustbesetto1."-*/-if(phy_interface_mode_is_8023z(state->interface)&&-!phylink_test(state->advertising,Autoneg)){-linkmode_zero(supported);-return;-}--phylink_generic_validate(config,supported,state);-}-staticintmvneta_mac_prepare(structphylink_config*config,unsignedintmode,phy_interface_tinterface){
From: Sean Anderson <hidden> Date: 2021-12-14 19:49:22
Hi Russell,
On 12/14/21 9:48 AM, Russell King (Oracle) wrote:
quoted hunk
Add a hook for PCS to validate the link parameters. This avoids MAC
drivers having to have knowledge of their PCS in their validate()
method, thereby allowing several MAC drivers to be simplfied.
Signed-off-by: Russell King (Oracle) <redacted>
---
drivers/net/phy/phylink.c | 31 +++++++++++++++++++++++++++++++
include/linux/phylink.h | 20 ++++++++++++++++++++
2 files changed, 51 insertions(+)
@@ -424,13 +424,44 @@ static int phylink_validate_mac_and_pcs(struct phylink *pl,structphylink_link_state*state){structphylink_pcs*pcs;+intret;+/* Get the PCS for this interface mode */if(pl->mac_ops->mac_select_pcs){pcs=pl->mac_ops->mac_select_pcs(pl->config,state->interface);if(IS_ERR(pcs))returnPTR_ERR(pcs);+}else{+pcs=pl->pcs;+}++if(pcs){+/* The PCS, if present, must be setup before phylink_create()+*hasbeencalled.Iftheopsisnotinitialised,printan+*errorandbacktraceratherthanoopsingthekernel.+*/+if(!pcs->ops){+phylink_err(pl,"interface %s: uninitialised PCS\n",+phy_modes(state->interface));+dump_stack();+return-EINVAL;+}++/* Validate the link parameters with the PCS */+if(pcs->ops->pcs_validate){+ret=pcs->ops->pcs_validate(pcs,supported,state);
I wonder if we can add a pcs->supported_interfaces. That would let me
write something like
static int xilinx_pcs_validate(struct phylink_pcs *pcs,
unsigned long *supported,
struct phylink_link_state *state)
{
__ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
phylink_set_port_modes(mask);
phylink_set(mask, Autoneg);
phylink_get_linkmodes(mask, state->interface,
MAC_10FD | MAC_100FD | MAC_1000FD);
linkmode_and(supported, supported, mask);
}
And of course, the above could become phylink_pcs_validate_generic with
the addition of a pcs->pcs_capabilities member.
The only wrinkle is that we need to handle PHY_INTERFACE_MODE_NA,
because of the pcs = pl->pcs assignment above. This would require doing
the phylink_validate_any dance again.
Maybe the best way is to stick
if (state->interface == PHY_INTERFACE_MODE_NA)
return -EINVAL;
at the top of phylink_pcs_validate_generic (perhaps with a warning).
That would catch any MACs who use a PCS which wants the MAC to have
supported_interfaces.
quoted hunk
+ if (ret < 0 || phylink_is_empty_linkmode(supported))+ return -EINVAL;++ /* Ensure the advertising mask is a subset of the+ * supported mask.+ */+ linkmode_and(state->advertising, state->advertising,+ supported);+ } }+ /* Then validate the link parameters with the MAC */ pl->mac_ops->validate(pl->config, supported, state);
Shouldn't the PCS stuff happen here? Later in the series, you do things
like
if (phy_interface_mode_is_8023z(state->interface) &&
!phylink_test(state->advertising, Autoneg))
return -EINVAL;
but there's nothing to stop a mac validate from coming along and saying
"we don't support autonegotiation".
--Sean
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-12-14 23:27:36
On Tue, Dec 14, 2021 at 02:49:13PM -0500, Sean Anderson wrote:
Hi Russell,
On 12/14/21 9:48 AM, Russell King (Oracle) wrote:
quoted
Add a hook for PCS to validate the link parameters. This avoids MAC
drivers having to have knowledge of their PCS in their validate()
method, thereby allowing several MAC drivers to be simplfied.
Signed-off-by: Russell King (Oracle) <redacted>
---
drivers/net/phy/phylink.c | 31 +++++++++++++++++++++++++++++++
include/linux/phylink.h | 20 ++++++++++++++++++++
2 files changed, 51 insertions(+)
@@ -424,13 +424,44 @@ static int phylink_validate_mac_and_pcs(struct phylink *pl,structphylink_link_state*state){structphylink_pcs*pcs;+intret;+/* Get the PCS for this interface mode */if(pl->mac_ops->mac_select_pcs){pcs=pl->mac_ops->mac_select_pcs(pl->config,state->interface);if(IS_ERR(pcs))returnPTR_ERR(pcs);+}else{+pcs=pl->pcs;+}++if(pcs){+/* The PCS, if present, must be setup before phylink_create()+*hasbeencalled.Iftheopsisnotinitialised,printan+*errorandbacktraceratherthanoopsingthekernel.+*/+if(!pcs->ops){+phylink_err(pl,"interface %s: uninitialised PCS\n",+phy_modes(state->interface));+dump_stack();+return-EINVAL;+}++/* Validate the link parameters with the PCS */+if(pcs->ops->pcs_validate){+ret=pcs->ops->pcs_validate(pcs,supported,state);
I wonder if we can add a pcs->supported_interfaces. That would let me
write something like
I have two arguments against that:
1) Given that .mac_select_pcs should not return a PCS that is not
appropriate for the provided state->interface, I don't see what
use having a supported_interfaces member in the PCS would give.
All that phylink would end up doing is validating that the MAC
was giving us a sane PCS.
2) In the case of a static PCS (in other words, one attached just
after phylink_create_pcs()) the PCS is known at creation time,
so limiting phylink_config.supported_interfaces according to the
single attached interface seems sane, rather than phylink having
to repeatedly recalculate the bitwise-and between both
supported_interface masks.
This would be buggy - doesn't the PCS allow pause frames through?
I already have a conversion for axienet in my tree, and it doesn't
need a pcs_validate() implementation. I'll provide it below.
And of course, the above could become phylink_pcs_validate_generic with
the addition of a pcs->pcs_capabilities member.
The only wrinkle is that we need to handle PHY_INTERFACE_MODE_NA,
because of the pcs = pl->pcs assignment above. This would require doing
the phylink_validate_any dance again.
Why do you think PHY_INTERFACE_MODE_NA needs handling? If this is not
set in phylink_config.supported_interfaces (which it should never be)
then none of the validation will be called with this.
The special PHY_INTERFACE_MODE_NA meaning "give us everything you have"
is something I want to get rid of, and is something that I am already
explicitly not supporting for pcs_validate(). It doesn't work with the
mac_select_pcs() model, since that can't return all PCS that may be
used.
if (state->interface == PHY_INTERFACE_MODE_NA)
return -EINVAL;
at the top of phylink_pcs_validate_generic (perhaps with a warning).
That would catch any MACs who use a PCS which wants the MAC to have
supported_interfaces.
... which could be too late.
quoted
+ if (ret < 0 || phylink_is_empty_linkmode(supported))+ return -EINVAL;++ /* Ensure the advertising mask is a subset of the+ * supported mask.+ */+ linkmode_and(state->advertising, state->advertising,+ supported);+ } }+ /* Then validate the link parameters with the MAC */ pl->mac_ops->validate(pl->config, supported, state);
Shouldn't the PCS stuff happen here? Later in the series, you do things
like
if (phy_interface_mode_is_8023z(state->interface) &&
!phylink_test(state->advertising, Autoneg))
return -EINVAL;
but there's nothing to stop a mac validate from coming along and saying
"we don't support autonegotiation".
How is autonegotiation a property of the MAC when there is a PCS?
In what situation is autonegotiation terminated at the MAC when
there is a PCS present?
The only case I can think of is where the PCS is tightly tied to the
MAC, and in that case you end up with a choice whether or not to model
a PCS in software. This is the case with mvneta and mvpp2 - there is
no separation of the MAC and PCS in the hardware register design. There
is one register that controls pause/duplex advertisement and speeds
irrespective of the PHY interface, whether the interface mode to the
external world is 1000BASE-X, SGMII, QSGMII, RGMII etc. mvpp2 is
slightly different in that it re-uses the GMAC design from mvneta for
speeds <= 2.5G, and an entirely separate XLG implementation for 5G
and 10G. Here, we model these as two separate PCS that we choose
between depending on the interface.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-12-14 23:29:12
On Tue, Dec 14, 2021 at 11:27:22PM +0000, Russell King (Oracle) wrote:
I already have a conversion for axienet in my tree, and it doesn't
need a pcs_validate() implementation. I'll provide it below.
Forgot to do so... this is obviously untested on real hardware.
8<===
From: "Russell King (Oracle)" <redacted>
Subject: [PATCH] net: axienet: convert to phylink_pcs
Convert axienet to use the phylink_pcs layer, resulting in it no longer
being a legacy driver.
Signed-off-by: Russell King (Oracle) <redacted>
---
drivers/net/ethernet/xilinx/xilinx_axienet.h | 1 +
.../net/ethernet/xilinx/xilinx_axienet_main.c | 109 +++++++++---------
2 files changed, 53 insertions(+), 57 deletions(-)
From: Sean Anderson <hidden> Date: 2021-12-14 23:54:26
On 12/14/21 6:27 PM, Russell King (Oracle) wrote:
On Tue, Dec 14, 2021 at 02:49:13PM -0500, Sean Anderson wrote:
quoted
Hi Russell,
On 12/14/21 9:48 AM, Russell King (Oracle) wrote:
quoted
Add a hook for PCS to validate the link parameters. This avoids MAC
drivers having to have knowledge of their PCS in their validate()
method, thereby allowing several MAC drivers to be simplfied.
Signed-off-by: Russell King (Oracle) <redacted>
---
drivers/net/phy/phylink.c | 31 +++++++++++++++++++++++++++++++
include/linux/phylink.h | 20 ++++++++++++++++++++
2 files changed, 51 insertions(+)
@@ -424,13 +424,44 @@ static int phylink_validate_mac_and_pcs(struct phylink *pl,structphylink_link_state*state){structphylink_pcs*pcs;+intret;+/* Get the PCS for this interface mode */if(pl->mac_ops->mac_select_pcs){pcs=pl->mac_ops->mac_select_pcs(pl->config,state->interface);if(IS_ERR(pcs))returnPTR_ERR(pcs);+}else{+pcs=pl->pcs;+}++if(pcs){+/* The PCS, if present, must be setup before phylink_create()+*hasbeencalled.Iftheopsisnotinitialised,printan+*errorandbacktraceratherthanoopsingthekernel.+*/+if(!pcs->ops){+phylink_err(pl,"interface %s: uninitialised PCS\n",+phy_modes(state->interface));+dump_stack();+return-EINVAL;+}++/* Validate the link parameters with the PCS */+if(pcs->ops->pcs_validate){+ret=pcs->ops->pcs_validate(pcs,supported,state);
I wonder if we can add a pcs->supported_interfaces. That would let me
write something like
I have two arguments against that:
1) Given that .mac_select_pcs should not return a PCS that is not
appropriate for the provided state->interface, I don't see what
use having a supported_interfaces member in the PCS would give.
All that phylink would end up doing is validating that the MAC
was giving us a sane PCS.
The MAC may not know what the PCS can support. For example, the xilinx
PCS/PMA can be configured to support 1000BASE-X, SGMII, both, or
neither. How else should the mac find out what is supported?
2) In the case of a static PCS (in other words, one attached just
after phylink_create_pcs()) the PCS is known at creation time,
so limiting phylink_config.supported_interfaces according to the
single attached interface seems sane, rather than phylink having
to repeatedly recalculate the bitwise-and between both
supported_interface masks.
This would be buggy - doesn't the PCS allow pause frames through?
Yes. I noticed this after writing my above email :)
I already have a conversion for axienet in my tree, and it doesn't
need a pcs_validate() implementation. I'll provide it below.
quoted
And of course, the above could become phylink_pcs_validate_generic with
the addition of a pcs->pcs_capabilities member.
The only wrinkle is that we need to handle PHY_INTERFACE_MODE_NA,
because of the pcs = pl->pcs assignment above. This would require doing
the phylink_validate_any dance again.
Why do you think PHY_INTERFACE_MODE_NA needs handling? If this is not
set in phylink_config.supported_interfaces (which it should never be)
then none of the validation will be called with this.
If the MAC has no supported_interfaces and calls phylink_set_pcs, but
does not implement mac_select_pcs, then you can have something like
phylink_validate(NA)
phylink_validate_mac_and_pcs(NA)
pcs = pl->pcs;
pcs->ops->pcs_validate(NA)
phylink_get_linkmodes(NA)
/* returns just Pause and Asym_Pause linkmodes */
/* nonzero, so pcs_validate thinks it's fine */
/* phylink_validate returns 0, but there are no valid interfaces */
The special PHY_INTERFACE_MODE_NA meaning "give us everything you have"
is something I want to get rid of, and is something that I am already
explicitly not supporting for pcs_validate(). It doesn't work with the
mac_select_pcs() model, since that can't return all PCS that may be
used.
quoted
if (state->interface == PHY_INTERFACE_MODE_NA)
return -EINVAL;
at the top of phylink_pcs_validate_generic (perhaps with a warning).
That would catch any MACs who use a PCS which wants the MAC to have
supported_interfaces.
... which could be too late.
You can't detect this in advance, since a MAC can choose to attach
whatever PCS it wants at any time. So all you can do is warn about it so
people report it as a bug instead of wondering why their ethernet won't
configure.
quoted
quoted
+ if (ret < 0 || phylink_is_empty_linkmode(supported))+ return -EINVAL;++ /* Ensure the advertising mask is a subset of the+ * supported mask.+ */+ linkmode_and(state->advertising, state->advertising,+ supported);+ } }+ /* Then validate the link parameters with the MAC */ pl->mac_ops->validate(pl->config, supported, state);
Shouldn't the PCS stuff happen here? Later in the series, you do things
like
if (phy_interface_mode_is_8023z(state->interface) &&
!phylink_test(state->advertising, Autoneg))
return -EINVAL;
but there's nothing to stop a mac validate from coming along and saying
"we don't support autonegotiation".
How is autonegotiation a property of the MAC when there is a PCS?
In what situation is autonegotiation terminated at the MAC when
there is a PCS present?
*shrug* it doesn't make a difference really as long as the MAC and PCS
play nice. But validate works by masking out bits, so you can only
really test for a bit after everyone has gotten their chance to veto
things. Which is why I think it is strange that the PCS check comes
first.
--Sean
The only case I can think of is where the PCS is tightly tied to the
MAC, and in that case you end up with a choice whether or not to model
a PCS in software. This is the case with mvneta and mvpp2 - there is
no separation of the MAC and PCS in the hardware register design. There
is one register that controls pause/duplex advertisement and speeds
irrespective of the PHY interface, whether the interface mode to the
external world is 1000BASE-X, SGMII, QSGMII, RGMII etc. mvpp2 is
slightly different in that it re-uses the GMAC design from mvneta for
speeds <= 2.5G, and an entirely separate XLG implementation for 5G
and 10G. Here, we model these as two separate PCS that we choose
between depending on the interface.
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-12-15 00:32:29
On Tue, Dec 14, 2021 at 06:54:16PM -0500, Sean Anderson wrote:
On 12/14/21 6:27 PM, Russell King (Oracle) wrote:
quoted
On Tue, Dec 14, 2021 at 02:49:13PM -0500, Sean Anderson wrote:
quoted
Hi Russell,
On 12/14/21 9:48 AM, Russell King (Oracle) wrote:
quoted
Add a hook for PCS to validate the link parameters. This avoids MAC
drivers having to have knowledge of their PCS in their validate()
method, thereby allowing several MAC drivers to be simplfied.
Signed-off-by: Russell King (Oracle) <redacted>
---
drivers/net/phy/phylink.c | 31 +++++++++++++++++++++++++++++++
include/linux/phylink.h | 20 ++++++++++++++++++++
2 files changed, 51 insertions(+)
@@ -424,13 +424,44 @@ static int phylink_validate_mac_and_pcs(struct phylink *pl,structphylink_link_state*state){structphylink_pcs*pcs;+intret;+/* Get the PCS for this interface mode */if(pl->mac_ops->mac_select_pcs){pcs=pl->mac_ops->mac_select_pcs(pl->config,state->interface);if(IS_ERR(pcs))returnPTR_ERR(pcs);+}else{+pcs=pl->pcs;+}++if(pcs){+/* The PCS, if present, must be setup before phylink_create()+*hasbeencalled.Iftheopsisnotinitialised,printan+*errorandbacktraceratherthanoopsingthekernel.+*/+if(!pcs->ops){+phylink_err(pl,"interface %s: uninitialised PCS\n",+phy_modes(state->interface));+dump_stack();+return-EINVAL;+}++/* Validate the link parameters with the PCS */+if(pcs->ops->pcs_validate){+ret=pcs->ops->pcs_validate(pcs,supported,state);
I wonder if we can add a pcs->supported_interfaces. That would let me
write something like
I have two arguments against that:
1) Given that .mac_select_pcs should not return a PCS that is not
appropriate for the provided state->interface, I don't see what
use having a supported_interfaces member in the PCS would give.
All that phylink would end up doing is validating that the MAC
was giving us a sane PCS.
The MAC may not know what the PCS can support. For example, the xilinx
PCS/PMA can be configured to support 1000BASE-X, SGMII, both, or
neither. How else should the mac find out what is supported?
I'll reply by asking a more relevant question at this point.
If we've asked for a PCS for 1000BASE-X via .mac_select_pcs() and a
PCS is returned that does not support 1000BASE-X, what happens then?
The system level says 1000BASE-X was supported when it isn't...
That to me sounds like bug.
quoted
2) In the case of a static PCS (in other words, one attached just
after phylink_create_pcs()) the PCS is known at creation time,
so limiting phylink_config.supported_interfaces according to the
single attached interface seems sane, rather than phylink having
to repeatedly recalculate the bitwise-and between both
supported_interface masks.
This would be buggy - doesn't the PCS allow pause frames through?
Yes. I noticed this after writing my above email :)
quoted
I already have a conversion for axienet in my tree, and it doesn't
need a pcs_validate() implementation. I'll provide it below.
quoted
And of course, the above could become phylink_pcs_validate_generic with
the addition of a pcs->pcs_capabilities member.
The only wrinkle is that we need to handle PHY_INTERFACE_MODE_NA,
because of the pcs = pl->pcs assignment above. This would require doing
the phylink_validate_any dance again.
Why do you think PHY_INTERFACE_MODE_NA needs handling? If this is not
set in phylink_config.supported_interfaces (which it should never be)
then none of the validation will be called with this.
If the MAC has no supported_interfaces and calls phylink_set_pcs, but
does not implement mac_select_pcs, then you can have something like
phylink_validate(NA)
phylink_validate_mac_and_pcs(NA)
pcs = pl->pcs;
pcs->ops->pcs_validate(NA)
phylink_get_linkmodes(NA)
/* returns just Pause and Asym_Pause linkmodes */
/* nonzero, so pcs_validate thinks it's fine */
/* phylink_validate returns 0, but there are no valid interfaces */
No, you don't end up in that situation, because phylink_validate() will
not return 0. It will return -EINVAL. We are not checking for an empty
supported mask, we are checking for a supported mask that contains no
linkmodes - this is an important difference between linkmode_empty()
and phylink_is_empty_linkmode(). The former checks for the linkmode
bitmap containing all zeros, the latter doesn't care about the media
bits, autoneg, pause or asympause linkmode bits. If all other bits are
zero, it returns true, causing phylink_validate_mac_and_pcs() to return
-EINVAL.
quoted
The special PHY_INTERFACE_MODE_NA meaning "give us everything you have"
is something I want to get rid of, and is something that I am already
explicitly not supporting for pcs_validate(). It doesn't work with the
mac_select_pcs() model, since that can't return all PCS that may be
used.
quoted
if (state->interface == PHY_INTERFACE_MODE_NA)
return -EINVAL;
at the top of phylink_pcs_validate_generic (perhaps with a warning).
That would catch any MACs who use a PCS which wants the MAC to have
supported_interfaces.
... which could be too late.
You can't detect this in advance, since a MAC can choose to attach
whatever PCS it wants at any time. So all you can do is warn about it so
people report it as a bug instead of wondering why their ethernet won't
configure.
As I say above, I don't see there's a problem here - and I think you've
mistaken the behaviour of phylink_is_empty_linkmode().
quoted
quoted
quoted
+ if (ret < 0 || phylink_is_empty_linkmode(supported))+ return -EINVAL;++ /* Ensure the advertising mask is a subset of the+ * supported mask.+ */+ linkmode_and(state->advertising, state->advertising,+ supported);+ } }+ /* Then validate the link parameters with the MAC */ pl->mac_ops->validate(pl->config, supported, state);
Shouldn't the PCS stuff happen here? Later in the series, you do things
like
if (phy_interface_mode_is_8023z(state->interface) &&
!phylink_test(state->advertising, Autoneg))
return -EINVAL;
but there's nothing to stop a mac validate from coming along and saying
"we don't support autonegotiation".
How is autonegotiation a property of the MAC when there is a PCS?
In what situation is autonegotiation terminated at the MAC when
there is a PCS present?
*shrug* it doesn't make a difference really as long as the MAC and PCS
play nice. But validate works by masking out bits, so you can only
really test for a bit after everyone has gotten their chance to veto
things. Which is why I think it is strange that the PCS check comes
first.
For the explicit case you are highlighting (autoneg), please give an
example where both the PCS and MAC would get to "vote" on this bit.
Please explain how the MAC would even be involved in autonegotiation.
I'm not going to say that this model is perfect, because it isn't.
It is adequate for the purposes we need to solve to move the code
forwards, and I believe it will allow us to elimate the mac_ops
.validate method entirely in a release or two.
Once that is done, we will rely on mac_capabilities to tell us what
the MAC supports, which is really all that we need the MAC to be
telling us. This will become important when we e.g. properly model
rate adapting PCS.
We can't get close to a model that allows us to do that right now
because the .validate method prevents that because that deals with
media linkmodes, rather than just the capabilities of the MAC. Our
only option right now would be to completely avoid calling the
.validate method if we know we have a PCS and completely ignore
MAC capabilities.
This is an evolutionary step sorting out some of the issues. I'm
very sure that there will be things it doesn't do very well
identified, and we will need to address those later.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
From: Jakub Kicinski <kuba@kernel.org> Date: 2021-12-15 02:35:12
On Tue, 14 Dec 2021 14:48:05 +0000 Russell King (Oracle) wrote:
+/**
+ * @mac_select_pcs: Select a PCS for the interface mode.
nit: no '@' in front of the function name
quoted hunk
+ * @config: a pointer to a &struct phylink_config.+ * @interface: PHY interface mode for PCS+ *+ * Return the &struct phylink_pcs for the specified interface mode, or+ * NULL if none is required, or an error pointer on error.+ *+ * This must not modify any state. It is used to query which PCS should+ * be used. Phylink will use this during validation to ensure that the+ * configuration is valid, and when setting a configuration to internally+ * set the PCS that will be used.+ */+struct phylink_pcs *mac_select_pcs(struct phylink_config *config,+ phy_interface_t interface);
From: Sean Anderson <hidden> Date: 2021-12-16 15:42:17
On 12/14/21 7:32 PM, Russell King (Oracle) wrote:
On Tue, Dec 14, 2021 at 06:54:16PM -0500, Sean Anderson wrote:
quoted
On 12/14/21 6:27 PM, Russell King (Oracle) wrote:
quoted
On Tue, Dec 14, 2021 at 02:49:13PM -0500, Sean Anderson wrote:
quoted
Hi Russell,
On 12/14/21 9:48 AM, Russell King (Oracle) wrote:
quoted
Add a hook for PCS to validate the link parameters. This avoids MAC
drivers having to have knowledge of their PCS in their validate()
method, thereby allowing several MAC drivers to be simplfied.
Signed-off-by: Russell King (Oracle) <redacted>
---
drivers/net/phy/phylink.c | 31 +++++++++++++++++++++++++++++++
include/linux/phylink.h | 20 ++++++++++++++++++++
2 files changed, 51 insertions(+)
@@ -424,13 +424,44 @@ static int phylink_validate_mac_and_pcs(struct phylink *pl,structphylink_link_state*state){structphylink_pcs*pcs;+intret;+/* Get the PCS for this interface mode */if(pl->mac_ops->mac_select_pcs){pcs=pl->mac_ops->mac_select_pcs(pl->config,state->interface);if(IS_ERR(pcs))returnPTR_ERR(pcs);+}else{+pcs=pl->pcs;+}++if(pcs){+/* The PCS, if present, must be setup before phylink_create()+*hasbeencalled.Iftheopsisnotinitialised,printan+*errorandbacktraceratherthanoopsingthekernel.+*/+if(!pcs->ops){+phylink_err(pl,"interface %s: uninitialised PCS\n",+phy_modes(state->interface));+dump_stack();+return-EINVAL;+}++/* Validate the link parameters with the PCS */+if(pcs->ops->pcs_validate){+ret=pcs->ops->pcs_validate(pcs,supported,state);
I wonder if we can add a pcs->supported_interfaces. That would let me
write something like
I have two arguments against that:
1) Given that .mac_select_pcs should not return a PCS that is not
appropriate for the provided state->interface, I don't see what
use having a supported_interfaces member in the PCS would give.
All that phylink would end up doing is validating that the MAC
was giving us a sane PCS.
The MAC may not know what the PCS can support. For example, the xilinx
PCS/PMA can be configured to support 1000BASE-X, SGMII, both, or
neither. How else should the mac find out what is supported?
I'll reply by asking a more relevant question at this point.
If we've asked for a PCS for 1000BASE-X via .mac_select_pcs() and a
PCS is returned that does not support 1000BASE-X, what happens then?
The system level says 1000BASE-X was supported when it isn't...
That to me sounds like bug.
Well, there are two ways to approach this, IMO, and both involve some
kind of supported_interfaces bitmap. The underlying constraint here is
that the MAC doesn't really know/care at compile-time what the PCS
supports.
- The MAC always returns the external PCS, since that is what the user
configured. In this case, the PCS is responsible for ensuring that the
interface is supported. If phylink does not do this check, then it
must be done in pcs_validate().
- The MAC inspects the PCS's supported_interfaces bitmap, and only
returns it from mac_select_pcs if it matches.
Sure, if the user says
pcs-handle = <&my_1000basex_only_pcs>;
phy-mode = "sgmii";
then this is a misconfiguration, but it is something which we have to
catch, and which the MAC shouldn't detect without additional
information.
quoted
quoted
2) In the case of a static PCS (in other words, one attached just
after phylink_create_pcs()) the PCS is known at creation time,
so limiting phylink_config.supported_interfaces according to the
single attached interface seems sane, rather than phylink having
to repeatedly recalculate the bitwise-and between both
supported_interface masks.
This would be buggy - doesn't the PCS allow pause frames through?
Yes. I noticed this after writing my above email :)
quoted
I already have a conversion for axienet in my tree, and it doesn't
need a pcs_validate() implementation. I'll provide it below.
quoted
And of course, the above could become phylink_pcs_validate_generic with
the addition of a pcs->pcs_capabilities member.
The only wrinkle is that we need to handle PHY_INTERFACE_MODE_NA,
because of the pcs = pl->pcs assignment above. This would require doing
the phylink_validate_any dance again.
Why do you think PHY_INTERFACE_MODE_NA needs handling? If this is not
set in phylink_config.supported_interfaces (which it should never be)
then none of the validation will be called with this.
If the MAC has no supported_interfaces and calls phylink_set_pcs, but
does not implement mac_select_pcs, then you can have something like
phylink_validate(NA)
phylink_validate_mac_and_pcs(NA)
pcs = pl->pcs;
pcs->ops->pcs_validate(NA)
phylink_get_linkmodes(NA)
/* returns just Pause and Asym_Pause linkmodes */
/* nonzero, so pcs_validate thinks it's fine */
/* phylink_validate returns 0, but there are no valid interfaces */
No, you don't end up in that situation, because phylink_validate() will
not return 0. It will return -EINVAL. We are not checking for an empty
supported mask, we are checking for a supported mask that contains no
linkmodes - this is an important difference between linkmode_empty()
and phylink_is_empty_linkmode(). The former checks for the linkmode
bitmap containing all zeros, the latter doesn't care about the media
bits, autoneg, pause or asympause linkmode bits. If all other bits are
zero, it returns true, causing phylink_validate_mac_and_pcs() to return
-EINVAL.
From: "Russell King (Oracle)" <linux@armlinux.org.uk> Date: 2021-12-16 17:10:29
On Thu, Dec 16, 2021 at 10:42:08AM -0500, Sean Anderson wrote:
On 12/14/21 7:32 PM, Russell King (Oracle) wrote:
quoted
On Tue, Dec 14, 2021 at 06:54:16PM -0500, Sean Anderson wrote:
quoted
On 12/14/21 6:27 PM, Russell King (Oracle) wrote:
quoted
On Tue, Dec 14, 2021 at 02:49:13PM -0500, Sean Anderson wrote:
quoted
Hi Russell,
On 12/14/21 9:48 AM, Russell King (Oracle) wrote:
quoted
Add a hook for PCS to validate the link parameters. This avoids MAC
drivers having to have knowledge of their PCS in their validate()
method, thereby allowing several MAC drivers to be simplfied.
Signed-off-by: Russell King (Oracle) <redacted>
---
drivers/net/phy/phylink.c | 31 +++++++++++++++++++++++++++++++
include/linux/phylink.h | 20 ++++++++++++++++++++
2 files changed, 51 insertions(+)
@@ -424,13 +424,44 @@ static int phylink_validate_mac_and_pcs(struct phylink *pl,structphylink_link_state*state){structphylink_pcs*pcs;+intret;+/* Get the PCS for this interface mode */if(pl->mac_ops->mac_select_pcs){pcs=pl->mac_ops->mac_select_pcs(pl->config,state->interface);if(IS_ERR(pcs))returnPTR_ERR(pcs);+}else{+pcs=pl->pcs;+}++if(pcs){+/* The PCS, if present, must be setup before phylink_create()+*hasbeencalled.Iftheopsisnotinitialised,printan+*errorandbacktraceratherthanoopsingthekernel.+*/+if(!pcs->ops){+phylink_err(pl,"interface %s: uninitialised PCS\n",+phy_modes(state->interface));+dump_stack();+return-EINVAL;+}++/* Validate the link parameters with the PCS */+if(pcs->ops->pcs_validate){+ret=pcs->ops->pcs_validate(pcs,supported,state);
I wonder if we can add a pcs->supported_interfaces. That would let me
write something like
I have two arguments against that:
1) Given that .mac_select_pcs should not return a PCS that is not
appropriate for the provided state->interface, I don't see what
use having a supported_interfaces member in the PCS would give.
All that phylink would end up doing is validating that the MAC
was giving us a sane PCS.
The MAC may not know what the PCS can support. For example, the xilinx
PCS/PMA can be configured to support 1000BASE-X, SGMII, both, or
neither. How else should the mac find out what is supported?
I'll reply by asking a more relevant question at this point.
If we've asked for a PCS for 1000BASE-X via .mac_select_pcs() and a
PCS is returned that does not support 1000BASE-X, what happens then?
The system level says 1000BASE-X was supported when it isn't...
That to me sounds like bug.
Well, there are two ways to approach this, IMO, and both involve some
kind of supported_interfaces bitmap. The underlying constraint here is
that the MAC doesn't really know/care at compile-time what the PCS
supports.
- The MAC always returns the external PCS, since that is what the user
configured. In this case, the PCS is responsible for ensuring that the
interface is supported. If phylink does not do this check, then it
must be done in pcs_validate().
- The MAC inspects the PCS's supported_interfaces bitmap, and only
returns it from mac_select_pcs if it matches.
Yes - we can do these sorts of things later if it turns out there is
a requirement to do so. At the moment, having been through all the
drivers recently, I don't see the need for it yet.
The only driver that may come close is xpcs, and the patches I've
proposed there don't need it - I just populate the PCS support
by calling into xpcs.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!