mvneta: SGMII-based in-band link state signaling
Subsystems (6)
cover mvneta: SGMII-based in-band link state signaling
Hello.
Currently the fixed-link DT binding is pre-configured and cannot be changed in run-time. This means the cable unplug events are not being detected, and the link parameters can't be negotiated.
The following patches are needed when mvneta is used in fixed-link mode (without MDIO).
They add an API to of_mdio for updating the fixed_phy status, and use that API in the mvneta driver when parsing the SGMII in-band status.
PATCH 1/6] fixed_phy: pass phy_device instead of net_device to link_update() function
Looking up phy_device pointer from net_device requires looking into private data. This may be problematic if phy is handled separately from net_device.
Pass phy_device pointer to link_update() function because looking up net_device pointer from phy_device is trivial, if needed.
--- drivers/net/phy/fixed_phy.c | 7 +++---- include/linux/phy_fixed.h | 4 ++-- net/dsa/slave.c | 3 ++- 3 files changed, 7 insertions(+), 7 deletions(-)
drivers/net/phy/fixed_phy.c +3 -4
@@ -36,7 +36,7 @@ struct fixed_phy {u16 regs[MII_REGS_NUM ];struct phy_device *phydev;struct fixed_phy_status status; -int (*link_update)(struct net_device *,struct fixed_phy_status *); +int (*link_update)(struct phy_device *,struct fixed_phy_status *);struct list_head node; };
@@ -139,8 +139,7 @@ static int fixed_mdio_read(struct mii_bus *bus, int phy_addr, int reg_num)if (fp->addr == phy_addr) {/* Issue callback if user registered it. */ if (fp->link_update) { - fp->link_update (fp->phydev->attached_dev, - &fp->status); + fp->link_update (fp->phydev, &fp->status);fixed_phy_update_regs (fp); }return fp->regs[reg_num];
@@ -162,7 +161,7 @@ static int fixed_mdio_write(struct mii_bus *bus, int phy_addr, int reg_num,* May be useful for PHY's that need to be software-driven. */ int fixed_phy_set_link_update (struct phy_device *phydev, -int (*link_update)(struct net_device *, +int (*link_update)(struct phy_device *,struct fixed_phy_status *)) {struct fixed_mdio_bus *fmb = &platform_fmb;
include/linux/phy_fixed.h +2 -2
@@ -19,7 +19,7 @@ extern struct phy_device *fixed_phy_register(unsigned int irq, struct device_node *np);extern void fixed_phy_del (int phy_addr);extern int fixed_phy_set_link_update (struct phy_device *phydev, -int (*link_update)(struct net_device *, +int (*link_update)(struct phy_device *,struct fixed_phy_status *));#else static inline int fixed_phy_add(unsigned int irq, int phy_id,
@@ -38,7 +38,7 @@ static inline int fixed_phy_del(int phy_addr)return -ENODEV ; }static inline int fixed_phy_set_link_update (struct phy_device *phydev, -int (*link_update)(struct net_device *, +int (*link_update)(struct phy_device *,struct fixed_phy_status *)) {return -ENODEV ;
net/dsa/slave.c +2 -1
@@ -500,9 +500,10 @@ static void dsa_slave_adjust_link(struct net_device *dev)phy_print_status (p->phy); } -static int dsa_slave_fixed_link_update (struct net_device *dev, +static int dsa_slave_fixed_link_update (struct phy_device *phy,struct fixed_phy_status *status) { +struct net_device *dev = phy->attached_dev;struct dsa_slave_priv *p =netdev_priv (dev);struct dsa_switch *ds = p->parent;
Signature · 1 line
1.7.9.5
PATCH 1/6 fixed_phy: pass phy_device instead of net_device to link_update() function
In reply to Stas SergeevLooking up phy_device pointer from net_device requires looking into private data. This may be problematic if phy is handled separately from net_device.
Pass phy_device pointer to link_update() function because looking up net_device pointer from phy_device is trivial, if needed.
--- drivers/net/phy/fixed_phy.c | 7 +++---- include/linux/phy_fixed.h | 4 ++-- net/dsa/slave.c | 3 ++- 3 files changed, 7 insertions(+), 7 deletions(-)
drivers/net/phy/fixed_phy.c +3 -4
@@ -36,7 +36,7 @@ struct fixed_phy {u16 regs[MII_REGS_NUM ];struct phy_device *phydev;struct fixed_phy_status status; -int (*link_update)(struct net_device *,struct fixed_phy_status *); +int (*link_update)(struct phy_device *,struct fixed_phy_status *);struct list_head node; };
@@ -139,8 +139,7 @@ static int fixed_mdio_read(struct mii_bus *bus, int phy_addr, int reg_num)if (fp->addr == phy_addr) {/* Issue callback if user registered it. */ if (fp->link_update) { - fp->link_update (fp->phydev->attached_dev, - &fp->status); + fp->link_update (fp->phydev, &fp->status);fixed_phy_update_regs (fp); }return fp->regs[reg_num];
@@ -162,7 +161,7 @@ static int fixed_mdio_write(struct mii_bus *bus, int phy_addr, int reg_num,* May be useful for PHY's that need to be software-driven. */ int fixed_phy_set_link_update (struct phy_device *phydev, -int (*link_update)(struct net_device *, +int (*link_update)(struct phy_device *,struct fixed_phy_status *)) {struct fixed_mdio_bus *fmb = &platform_fmb;
include/linux/phy_fixed.h +2 -2
@@ -19,7 +19,7 @@ extern struct phy_device *fixed_phy_register(unsigned int irq, struct device_node *np);extern void fixed_phy_del (int phy_addr);extern int fixed_phy_set_link_update (struct phy_device *phydev, -int (*link_update)(struct net_device *, +int (*link_update)(struct phy_device *,struct fixed_phy_status *));#else static inline int fixed_phy_add(unsigned int irq, int phy_id,
@@ -38,7 +38,7 @@ static inline int fixed_phy_del(int phy_addr)return -ENODEV ; }static inline int fixed_phy_set_link_update (struct phy_device *phydev, -int (*link_update)(struct net_device *, +int (*link_update)(struct phy_device *,struct fixed_phy_status *)) {return -ENODEV ;
net/dsa/slave.c +2 -1
@@ -500,9 +500,10 @@ static void dsa_slave_adjust_link(struct net_device *dev)phy_print_status (p->phy); } -static int dsa_slave_fixed_link_update (struct net_device *dev, +static int dsa_slave_fixed_link_update (struct phy_device *phy,struct fixed_phy_status *status) { +struct net_device *dev = phy->attached_dev;struct dsa_slave_priv *p =netdev_priv (dev);struct dsa_switch *ds = p->parent;
Signature · 1 line
1.7.9.5
PATCH 2/6 fixed_phy: add fixed_phy_unregister()
In reply to Stas SergeevA counterpart of fixed_phy_register().
Can be used on failure path, like below:
phy = fixed_phy_register(...);phy->priv = kmalloc(...);
if (!phy->priv) {
fixed_phy_unregister(phy);
return -ENOMEM;
}--- drivers/net/phy/fixed_phy.c | 11 +++++++++++ drivers/net/phy/phy_device.c | 7 +++++++ include/linux/phy.h | 1 + include/linux/phy_fixed.h | 4 ++++ 4 files changed, 23 insertions(+)
drivers/net/phy/fixed_phy.c +11 -0
@@ -275,6 +275,17 @@ struct phy_device *fixed_phy_register(unsigned int irq, }EXPORT_SYMBOL_GPL (fixed_phy_register); +void fixed_phy_unregister (struct phy_device *phy) +{ +struct device_node *np = phy->dev.of_node; + +fixed_phy_del (phy->addr); +phy_device_unregister (phy); +phy_device_free (phy); +of_node_put (np); +} +EXPORT_SYMBOL_GPL (fixed_phy_unregister); +static int __init fixed_mdio_bus_init (void ) {struct fixed_mdio_bus *fmb = &platform_fmb;
drivers/net/phy/phy_device.c +7 -0
@@ -375,6 +375,13 @@ int phy_device_register(struct phy_device *phydev) }EXPORT_SYMBOL (phy_device_register); +void phy_device_unregister (struct phy_device *phydev) +{ + phydev->bus->phy_map[phydev->addr] =NULL ; +device_del (&phydev->dev); +} +EXPORT_SYMBOL (phy_device_unregister); +/** * phy_find_first - finds the first PHY device on the bus * @bus: the target MII bus
include/linux/phy.h +1 -0
@@ -714,6 +714,7 @@ struct phy_device *phy_device_create(struct mii_bus *bus, int addr, int phy_id, struct phy_c45_device_ids *c45_ids);struct phy_device *get_phy_device (struct mii_bus *bus,int addr,bool is_c45);int phy_device_register (struct phy_device *phy); +void phy_device_unregister (struct phy_device *phydev);int phy_init_hw (struct phy_device *phydev);int phy_suspend (struct phy_device *phydev);int phy_resume (struct phy_device *phydev);
include/linux/phy_fixed.h +4 -0
@@ -17,6 +17,7 @@ extern int fixed_phy_add(unsigned int irq, int phy_id,extern struct phy_device *fixed_phy_register (unsigned int irq,struct fixed_phy_status *status,struct device_node *np); +extern void fixed_phy_unregister (struct phy_device *phy);extern void fixed_phy_del (int phy_addr); extern int fixed_phy_set_link_update(struct phy_device *phydev, int (*link_update)(struct phy_device *,
@@ -33,6 +34,9 @@ static inline struct phy_device *fixed_phy_register(unsigned int irq, {return ERR_PTR (-ENODEV ); } +static inline void fixed_phy_unregister (struct phy_device *phy) +{ +}static inline int fixed_phy_del (int phy_addr) {return -ENODEV ;
Signature · 1 line
1.7.9.5
PATCH 3/6 of_mdio: restructure of_phy_register_fixed_link() for further modifications
In reply to Stas SergeevThis is a no-op patch needed to separate code rearrangements from the actual changes. It allows to easily add code to the function without duplicating it for new and old fixed-phy DT binding. The subsequent patch therefore happily adds the code without duplications.
--- drivers/of/of_mdio.c | 32 ++++++++++++++++++-------------- 1 file changed, 18 insertions(+), 14 deletions(-)
drivers/of/of_mdio.c +18 -14
@@ -301,22 +301,26 @@ int of_phy_register_fixed_link(struct device_node *np) "asym-pause");of_node_put (fixed_link_node); phy =fixed_phy_register (PHY_POLL , &status, np); - return IS_ERR(phy) ? PTR_ERR(phy) : 0; - } - -/* Old binding */ - fixed_link_prop =of_get_property (np,"fixed-link" , &len); -if (fixed_link_prop && len == (5 *sizeof (__be32))) { - status.link =1 ; - status.duplex =be32_to_cpu (fixed_link_prop[1 ]); - status.speed =be32_to_cpu (fixed_link_prop[2 ]); - status.pause =be32_to_cpu (fixed_link_prop[3 ]); - status.asym_pause =be32_to_cpu (fixed_link_prop[4 ]); - phy =fixed_phy_register (PHY_POLL , &status, np); -return IS_ERR (phy) ?PTR_ERR (phy) :0 ; +if (IS_ERR (phy)) + return PTR_ERR(phy); + }else { +/* Old binding */ + fixed_link_prop =of_get_property (np,"fixed-link" , &len); +if (fixed_link_prop && len == (5 *sizeof (__be32))) { + status.link =1 ; + status.duplex =be32_to_cpu (fixed_link_prop[1 ]); + status.speed =be32_to_cpu (fixed_link_prop[2 ]); + status.pause =be32_to_cpu (fixed_link_prop[3 ]); + status.asym_pause =be32_to_cpu (fixed_link_prop[4 ]); + phy =fixed_phy_register (PHY_POLL , &status, np); +if (IS_ERR (phy)) +return PTR_ERR (phy); + }else { +return -ENODEV ; + } } -return -ENODEV ; +return 0 ; }EXPORT_SYMBOL (of_phy_register_fixed_link);#endif
Signature · 1 line
1.7.9.5
PATCH 4/6 of: add API for changing parameters of fixed link
In reply to Stas SergeevThe following API is added:
- of_phy_fixed_link_set_link() allows to set link state (up/down)
- of_phy_fixed_link_set_speed() allows to set link speed
- of_phy_fixed_link_set_duplex() allows to enable/disable duplex
This API is needed when the MDIO-less link have some other means of a status passing to MAC, for example with in-band data (SGMII). MAC driver can then use that API to update the PHY status.
--- drivers/of/of_mdio.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/of_mdio.h | 17 +++++++++++ 2 files changed, 89 insertions(+)
drivers/of/of_mdio.c +72 -0
@@ -251,6 +251,69 @@ struct phy_device *of_phy_attach(struct net_device *dev,EXPORT_SYMBOL (of_phy_attach);#if defined(CONFIG_FIXED_PHY ) +struct fixed_link_data { +struct fixed_phy_status status; +struct fixed_phy_status changed; +}; + +static int of_phy_fixed_link_update (struct phy_device *phy, +struct fixed_phy_status *status) +{ +struct fixed_link_data *priv = phy->priv; + +if (priv->changed.link) { + status->link = priv->status.link; + priv->changed.link =0 ; + } +if (priv->changed.speed) { + status->speed = priv->status.speed; + priv->changed.speed =0 ; + } +if (priv->changed.duplex) { + status->duplex = priv->status.duplex; + priv->changed.duplex =0 ; + } +if (priv->changed.pause) { + status->pause = priv->status.pause; + priv->changed.pause =0 ; + } +if (priv->changed.asym_pause) { + status->asym_pause = priv->status.asym_pause; + priv->changed.asym_pause =0 ; + } +return 0 ; +} + +int of_phy_fixed_link_set_link (struct phy_device *phy,int link) +{ +struct fixed_link_data *priv = phy->priv; + + priv->status.link = link; + priv->changed.link =1 ; +return 0 ; +} +EXPORT_SYMBOL (of_phy_fixed_link_set_link); + +int of_phy_fixed_link_set_speed (struct phy_device *phy,int speed) +{ +struct fixed_link_data *priv = phy->priv; + + priv->status.speed = speed; + priv->changed.speed =1 ; +return 0 ; +} +EXPORT_SYMBOL (of_phy_fixed_link_set_speed); + +int of_phy_fixed_link_set_duplex (struct phy_device *phy,int duplex) +{ +struct fixed_link_data *priv = phy->priv; + + priv->status.duplex = duplex; + priv->changed.duplex =1 ; +return 0 ; +} +EXPORT_SYMBOL (of_phy_fixed_link_set_duplex); +/* * of_phy_is_fixed_link() and of_phy_register_fixed_link() must * support two DT bindings:
@@ -287,6 +350,7 @@ int of_phy_register_fixed_link(struct device_node *np)const __be32 *fixed_link_prop;int len;struct phy_device *phy; +struct fixed_link_data *priv;/* New binding */ fixed_link_node =of_get_child_by_name (np,"fixed-link" );
@@ -320,6 +384,14 @@ int of_phy_register_fixed_link(struct device_node *np) } } + priv =kmalloc (sizeof (*priv),GFP_KERNEL ); +if (!priv) { +fixed_phy_unregister (phy); +return -ENOMEM ; + } +memset (&priv->changed,0 ,sizeof (priv->changed)); + phy->priv = priv; +fixed_phy_set_link_update (phy, of_phy_fixed_link_update);return 0 ; }EXPORT_SYMBOL (of_phy_register_fixed_link);
include/linux/of_mdio.h +17 -0
@@ -65,6 +65,9 @@ static inline struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np)#if defined(CONFIG_OF ) && defined(CONFIG_FIXED_PHY )extern int of_phy_register_fixed_link (struct device_node *np);extern bool of_phy_is_fixed_link (struct device_node *np); +extern int of_phy_fixed_link_set_link (struct phy_device *phy,int link); +extern int of_phy_fixed_link_set_speed (struct phy_device *phy,int speed); +extern int of_phy_fixed_link_set_duplex (struct phy_device *phy,int duplex);#else static inline int of_phy_register_fixed_link (struct device_node *np) {
@@ -74,6 +77,20 @@ static inline bool of_phy_is_fixed_link(struct device_node *np) {return false; } +static inline int of_phy_fixed_link_set_link (struct phy_device *phy,int link) +{ +return -ENOSYS ; +} +static inline int of_phy_fixed_link_set_speed (struct phy_device *phy, +int speed) +{ +return -ENOSYS ; +} +static inline int of_phy_fixed_link_set_duplex (struct phy_device *phy, +int duplex) +{ +return -ENOSYS ; +}#endif
Signature · 1 line
1.7.9.5
Re: [PATCH 4/6] of: add API for changing parameters of fixed link
2015-03-27 6:37 GMT-07:00 Stas Sergeev <[email protected]>:
Quoted 9 lines
The following API is added:
-of_phy_fixed_link_set_link()allows to set link state (up/down)
-of_phy_fixed_link_set_speed()allows to set link speed
-of_phy_fixed_link_set_duplex()allows to enable/disable duplexThis API is needed when the MDIO-less link have some other means of a status passing to MAC, for example with in-band data (SGMII). MAC driver can then use that API to update the PHY status.
I do not think any of these changes are required, if you look at drivers/net/dsa/bcm_sf2.c, there is a fixed_link_update callback to re-act to link up/down interrupts (but this could easily be extended to speed/duplex as well), by directly modifying a fixed_phy_status structure.
Can you try that approach in mvneta?
Quoted 148 lines
CC Florian Fainelli <[email protected]>CC Grant Likely <[email protected]>CC Rob Herring <[email protected]>Signed-off-by Stas Sergeev <[email protected]>--- drivers/of/of_mdio.c | 72 +++++++++++++++++++++++++++++++++++++++++++++++ include/linux/of_mdio.h | 17 +++++++++++ 2 files changed, 89 insertions(+)drivers/of/of_mdio.c +72
-0@@ -251,6 +251,69 @@ struct phy_device *of_phy_attach(struct net_device *dev,EXPORT_SYMBOL (of_phy_attach);#if defined(CONFIG_FIXED_PHY ) +struct fixed_link_data { +struct fixed_phy_status status; +struct fixed_phy_status changed; +}; + +static int of_phy_fixed_link_update (struct phy_device *phy, +struct fixed_phy_status *status) +{ +struct fixed_link_data *priv = phy->priv; + +if (priv->changed.link) { + status->link = priv->status.link; + priv->changed.link =0 ; + } +if (priv->changed.speed) { + status->speed = priv->status.speed; + priv->changed.speed =0 ; + } +if (priv->changed.duplex) { + status->duplex = priv->status.duplex; + priv->changed.duplex =0 ; + } +if (priv->changed.pause) { + status->pause = priv->status.pause; + priv->changed.pause =0 ; + } +if (priv->changed.asym_pause) { + status->asym_pause = priv->status.asym_pause; + priv->changed.asym_pause =0 ; + } +return 0 ; +} + +int of_phy_fixed_link_set_link (struct phy_device *phy,int link) +{ +struct fixed_link_data *priv = phy->priv; + + priv->status.link = link; + priv->changed.link =1 ; +return 0 ; +} +EXPORT_SYMBOL (of_phy_fixed_link_set_link); + +int of_phy_fixed_link_set_speed (struct phy_device *phy,int speed) +{ +struct fixed_link_data *priv = phy->priv; + + priv->status.speed = speed; + priv->changed.speed =1 ; +return 0 ; +} +EXPORT_SYMBOL (of_phy_fixed_link_set_speed); + +int of_phy_fixed_link_set_duplex (struct phy_device *phy,int duplex) +{ +struct fixed_link_data *priv = phy->priv; + + priv->status.duplex = duplex; + priv->changed.duplex =1 ; +return 0 ; +} +EXPORT_SYMBOL (of_phy_fixed_link_set_duplex); +/* * of_phy_is_fixed_link() and of_phy_register_fixed_link() must * support two DT bindings: @@ -287,6 +350,7 @@ int of_phy_register_fixed_link(struct device_node *np)const __be32 *fixed_link_prop;int len;struct phy_device *phy; +struct fixed_link_data *priv;/* New binding */ fixed_link_node =of_get_child_by_name (np,"fixed-link" );@@ -320,6 +384,14 @@ int of_phy_register_fixed_link(struct device_node *np) } } + priv =kmalloc (sizeof (*priv),GFP_KERNEL ); +if (!priv) { +fixed_phy_unregister (phy); +return -ENOMEM ; + } +memset (&priv->changed,0 ,sizeof (priv->changed)); + phy->priv = priv; +fixed_phy_set_link_update (phy, of_phy_fixed_link_update);return 0 ; }EXPORT_SYMBOL (of_phy_register_fixed_link);include/linux/of_mdio.h +17
-1@@ -65,6 +65,9 @@ static inline struct mii_bus *of_mdio_find_bus(struct device_node *mdio_np)#if defined(CONFIG_OF ) && defined(CONFIG_FIXED_PHY )extern int of_phy_register_fixed_link (struct device_node *np);extern bool of_phy_is_fixed_link (struct device_node *np); +extern int of_phy_fixed_link_set_link (struct phy_device *phy,int link); +extern int of_phy_fixed_link_set_speed (struct phy_device *phy,int speed); +extern int of_phy_fixed_link_set_duplex (struct phy_device *phy,int duplex);#else static inline int of_phy_register_fixed_link (struct device_node *np) {@@ -74,6 +77,20 @@ static inline bool of_phy_is_fixed_link(struct device_node *np) {return false; } +static inline int of_phy_fixed_link_set_link (struct phy_device *phy,int link) +{ +return -ENOSYS ; +} +static inline int of_phy_fixed_link_set_speed (struct phy_device *phy, +int speed) +{ +return -ENOSYS ; +} +static inline int of_phy_fixed_link_set_duplex (struct phy_device *phy, +int duplex) +{ +return -ENOSYS ; +}#endif --1.7.9.5
Signature · 5 lines
Florian -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] of: add API for changing parameters of fixed link
27.03.2015 18:41, Florian Fainelli пишет:
Quoted 11 lines
2015-03-27 6:37 GMT-07:00 Stas Sergeev <[email protected]>:
Quoted 9 lines
The following API is added:
-of_phy_fixed_link_set_link()allows to set link state (up/down)
-of_phy_fixed_link_set_speed()allows to set link speed
-of_phy_fixed_link_set_duplex()allows to enable/disable duplexThis API is needed when the MDIO-less link have some other means of a status passing to MAC, for example with in-band data (SGMII). MAC driver can then use that API to update the PHY status.
Quoted 7 lines
I do not think any of these changes are required, if you look at
drivers/net/dsa/bcm_sf2.c, there is afixed_link_updatecallback to re-act to link up/down interrupts (but this could easily be extended to speed/duplex as well), by directly modifying afixed_phy_statusstructure.Can you try that approach in mvneta?
Just to make sure I properly understand what you want. drivers/net/dsa/bcm_sf2.c registers the .fixed_link_update callback of struct dsa_switch_driver, which is later called from net/dsa/slave.c's dsa_slave_fixed_link_update().
Do you want mvneta to register a similar callback in of_mdio, instead of adding an explicit state-updating functions? Something like of_phy_fixed_link_set_update_callback()?
This will remove a few changes indeed, but perhaps not too much. Please confirm if this is exactly what you want, and then I try.
Re: [PATCH 4/6] of: add API for changing parameters of fixed link
2015-03-27 9:07 GMT-07:00 Stas Sergeev <[email protected]>:
Quoted 18 lines
27.03.2015 18:41, Florian Fainelli пишет:
Quoted 17 lines
2015-03-27 6:37 GMT-07:00 Stas Sergeev <[email protected]>:
Quoted 9 lines
The following API is added:
-of_phy_fixed_link_set_link()allows to set link state (up/down)
-of_phy_fixed_link_set_speed()allows to set link speed
-of_phy_fixed_link_set_duplex()allows to enable/disable duplexThis API is needed when the MDIO-less link have some other means of a status passing to MAC, for example with in-band data (SGMII). MAC driver can then use that API to update the PHY status.
I do not think any of these changes are required, if you look at
drivers/net/dsa/bcm_sf2.c, there is afixed_link_updatecallback to re-act to link up/down interrupts (but this could easily be extended to speed/duplex as well), by directly modifying afixed_phy_statusstructure.
Quoted 5 lines
Quoted 1 line
Can you try that approach in mvneta?
Just to make sure I properly understand what you want.
drivers/net/dsa/bcm_sf2.cregisters the .fixed_link_update callback of structdsa_switch_driver, which is later called from net/dsa/slave.c'sdsa_slave_fixed_link_update().
Right, that's because there is a two layer model, first in DSA, then in the switch driver.
Quoted 3 lines
Do you want mvneta to register a similar callback in
of_mdio, instead of adding an explicit state-updating functions? Something likeof_phy_fixed_link_set_update_callback()?
You don't need an of_phy_fixed_link_set_update callback, you just need to provide a fixed_link_update callback in mvneta, that you register, if and only if you are binding to a fixed PHY which is also SGMII (you can use a phy-mode property to tell that). So essentially, do the same thing as net/dsa/slave.c, checking that a PHY is fixed, and if so, register a fixed_link_update callback, within mvneta, and this callback updates fixed_phy_status, ala drivers/net/dsa/bcm_sf2.c to adjust the link parameters.
Quoted 2 lines
This will remove a few changes indeed, but perhaps not too much. Please confirm if this is exactly what you want, and then I try.
Let me know if this is clearer now, if not, I can certainly cook a patch which does what I am suggesting. Thanks!
Signature · 1 line
Florian
Re: [PATCH 4/6] of: add API for changing parameters of fixed link
27.03.2015 19:21, Florian Fainelli пишет:
Quoted 5 lines
Quoted 3 lines
Do you want mvneta to register a similar callback in
of_mdio, instead of adding an explicit state-updating functions? Something likeof_phy_fixed_link_set_update_callback()?You don't need an
of_phy_fixed_link_set_updatecallback, you just need to provide afixed_link_updatecallback in mvneta, that you register,
That approach I in fact considered initially, as the simplest one, and even had a patch. But I disliked the fact that then mvneta will exploit the knowledge of the fact that of_phy_register_fixed_link() uses a fixed_phy driver. What if the implementation will later change? Also what makes me uncomfortable is that since of_phy_register_fixed_link() doesn't even return the struct phy_device pointer, mvneta will have to get around that and use for example of_phy_find_device(), or register the callback later, after of_phy_connect(). dsa/slave.c does of_phy_connect() initially, together with fixed link registration, so it gets around the problem. But mvneta registers the fixed_link in .probe callback, and does of_phy_connect() in .open callback.
This all made me to drop that idea despite the simplicity.
Quoted 4 lines
Quoted 2 lines
This will remove a few changes indeed, but perhaps not too much. Please confirm if this is exactly what you want, and then I try.
Let me know if this is clearer now, if not, I can certainly cook a patch which does what I am suggesting. Thanks!
I can do that too, because I already did.
Let me know if the above concerns are not important, and I'll restore my initial patch.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/6] of: add API for changing parameters of fixed link
Quoted 10 lines · Stas Sergeev
27.03.2015 19:21, Florian Fainelli пишет:
Quoted 5 lines
Quoted 3 lines
Do you want mvneta to register a similar callback in
of_mdio, instead of adding an explicit state-updating functions? Something likeof_phy_fixed_link_set_update_callback()?You don't need an
of_phy_fixed_link_set_updatecallback, you just need to provide afixed_link_updatecallback in mvneta, that you register,That approach I in fact considered initially, as the simplest one, and even had a patch. But I disliked the fact that then mvneta will exploit the knowledge of the fact that
of_phy_register_fixed_link()uses afixed_phydriver. What if the implementation will later change?
There is no reason why it should change later, that's the entire purpose of why we can tell whether it is a fixed PHY or a regular MDIO-managed PHY, and drivers rely on that for their operations.
Quoted 4 lines
Also what makes me uncomfortable is that since
of_phy_register_fixed_link()doesn't even return the structphy_devicepointer, mvneta will have to get around that and use for exampleof_phy_find_device(), or register the callback later, afterof_phy_connect()
Ok, you could either make of_phy_register_fixed_link() return a phy_device, or as you suggest resolve the phy_device from the device_node later, your call.
. dsa/slave.c does of_phy_connect()
Quoted 4 lines
initially, together with fixed link registration, so it gets around the problem. But mvneta registers the
fixed_linkin .probe callback, and doesof_phy_connect()in .open callback.
This all made me to drop that idea despite the simplicity.
Yet that's still the cleanest/less invasive approach imho.
Quoted 9 lines
Quoted 5 lines
Quoted 3 lines
This will remove a few changes indeed, but perhaps not too much. Please confirm if this is exactly what you want, and then I try.
Let me know if this is clearer now, if not, I can certainly cook a patch which does what I am suggesting. Thanks!
I can do that too, because I already did.
Let me know if the above concerns are not important, and I'll restore my initial patch.
I think your concerns are valid, but I don't think there is going to be any problem with the approach I suggested because there is a contract that the fixed PHYs and regular PHYs need to
Signature · 1 line
Florian
Re: [PATCH 4/6] of: add API for changing parameters of fixed link
27.03.2015 20:15, Florian Fainelli пишет:
Quoted 7 lines · Stas Sergeev
Quoted 6 lines
27.03.2015 19:21, Florian Fainelli пишет:
Quoted 5 lines
Quoted 3 lines
Do you want mvneta to register a similar callback in
of_mdio, instead of adding an explicit state-updating functions? Something likeof_phy_fixed_link_set_update_callback()?You don't need an
of_phy_fixed_link_set_updatecallback, you just need to provide afixed_link_updatecallback in mvneta, that you register,
Quoted 8 lines
Quoted 4 lines
That approach I in fact considered initially, as the simplest one, and even had a patch. But I disliked the fact that then mvneta will exploit the knowledge of the fact that
of_phy_register_fixed_link()uses afixed_phydriver. What if the implementation will later change?There is no reason why it should change later, that's the entire purpose of why we can tell whether it is a fixed PHY or a regular MDIO-managed PHY, and drivers rely on that for their operations.
I don't think anything is exported to the outside of of_mdio. It uses fixed_phy driver completely internally. Assuming that we can pass the created phy_device to the fixed_phy API directly, look like a bit of layering violation to me. But it is not that big as to worry about. :)
I don't think many drivers rely on that - perhaps only dsa_slave. So I'll add the second one.
Quoted 2 lines
Ok, you could either make
of_phy_register_fixed_link()return aphy_device,
OK, if you think that's right - I'll do.
I was under an impression that this was intentional to not return the phy.
Quoted 3 lines
I think your concerns are valid, but I don't think there is going to be any problem with the approach I suggested because there is a contract that the fixed PHYs and regular PHYs need to
OK, so I'll do that on Monday then.
Re: [PATCH 4/6] of: add API for changing parameters of fixed link
In reply to Florian Fainelli27.03.2015 20:15, Florian Fainelli пишет:
Quoted 3 lines
I think your concerns are valid, but I don't think there is going to be any problem with the approach I suggested because there is a contract that the fixed PHYs and regular PHYs need to
Hello Florian.
As promised, today I tried to resurrect my first implementation and do things as you suggested: install the link_update callback for mvneta privately.
I feel SGMII setup is very common and deserves the separate API, not the per-driver handling, but in any case, I'd like to show the implementation first, then discuss.
Unfortunately, it didn't work quite right.
The problem is that mvneta calls phy_disconnect() on .ndo_stop callback. After that, phy->attached_dev becomes NULL, and so the link_update callback gets called with net_dev==NULL! And crashs. Of course I can easily work around that, but IMHO its a bug - the one that actually gets fixed by the patches I posted previously. They were changing the callback to receive phy_device instead of net_device, and so NULL will never be passed.
I am attaching the new patch so that you can take a look and decide. If you still think its fine, even despite the NULL passing w/a, then I'll mail it with the proper boilerplate. But if you agree that passing NULL to link_update is a bug, then maybe you'll decide to get the whole surgery thing. :)
-=-=-=-=-=-=-=-=-=# Don't remove this line #=-=-=-=-=-=-=-=-=- When MDIO bus is unavailable (common setup for SGMII), the in-band signaling must be used to correctly track link state.
This patch enables the in-band status delivery for links state changes, namely:
- link up/down
- link speed
- duplex full/halffixed_phy link_update() callback is used to update status.
Note: SGMII setup is so common that I think it deserves a separate API rather than the per-driver handling. There is an alternative implementation that adds such API:
https://lkml.org/lkml/2015/3/27/346
--- drivers/net/ethernet/marvell/mvneta.c | 97 +++++++++++++++++++++++++++++---- 1 file changed, 86 insertions(+), 11 deletions(-)
drivers/net/ethernet/marvell/mvneta.c +86 -11
@@ -100,6 +100,8 @@#define MVNETA_TXQ_CMD 0x2448#define MVNETA_TXQ_DISABLE_SHIFT 8#define MVNETA_TXQ_ENABLE_MASK 0x000000ff +#define MVNETA_GMAC_CLOCK_DIVIDER 0x24f4 +#define MVNETA_GMAC_1MS_CLOCK_ENABLE BIT(31)#define MVNETA_ACC_MODE 0x2500#define MVNETA_CPU_MAP (cpu) (0x2540 + ((cpu) << 2))#define MVNETA_CPU_RXQ_ACCESS_ALL_MASK 0x000000ff
@@ -122,6 +124,7 @@#define MVNETA_TX_INTR_MASK_ALL (0xff << 0)#define MVNETA_RX_INTR_MASK (nr_rxqs) (((1 << nr_rxqs) - 1) << 8)#define MVNETA_RX_INTR_MASK_ALL (0xff << 8) +#define MVNETA_MISCINTR_INTR_MASK BIT(31)#define MVNETA_INTR_OLD_CAUSE 0x25a8#define MVNETA_INTR_OLD_MASK 0x25ac
@@ -165,6 +168,7 @@#define MVNETA_GMAC_MAX_RX_SIZE_MASK 0x7ffc#define MVNETA_GMAC0_PORT_ENABLE BIT(0)#define MVNETA_GMAC_CTRL_2 0x2c08 +#define MVNETA_GMAC2_INBAND_AN_ENABLE BIT(0)#define MVNETA_GMAC2_PCS_ENABLE BIT(3)#define MVNETA_GMAC2_PORT_RGMII BIT(4)#define MVNETA_GMAC2_PORT_RESET BIT(6)
@@ -180,9 +184,11 @@#define MVNETA_GMAC_AUTONEG_CONFIG 0x2c0c#define MVNETA_GMAC_FORCE_LINK_DOWN BIT(0)#define MVNETA_GMAC_FORCE_LINK_PASS BIT(1) +#define MVNETA_GMAC_INBAND_AN_ENABLE BIT(2)#define MVNETA_GMAC_CONFIG_MII_SPEED BIT(5)#define MVNETA_GMAC_CONFIG_GMII_SPEED BIT(6)#define MVNETA_GMAC_AN_SPEED_EN BIT(7) +#define MVNETA_GMAC_AN_FLOW_CTRL_EN BIT(11)#define MVNETA_GMAC_CONFIG_FULL_DUPLEX BIT(12)#define MVNETA_GMAC_AN_DUPLEX_EN BIT(13)#define MVNETA_MIB_COUNTERS_BASE 0x3080
@@ -304,6 +310,7 @@ struct mvneta_port {unsigned int link;unsigned int duplex;unsigned int speed; +int inband_status; };/* The mvneta_tx_desc and mvneta_rx_desc structures describe the
@@ -994,6 +1001,20 @@ static void mvneta_defaults_set(struct mvneta_port *pp) val &= ~MVNETA_PHY_POLLING_ENABLE ;mvreg_write (pp,MVNETA_UNIT_CONTROL , val); +if (pp->inband_status) { + val =mvreg_read (pp,MVNETA_GMAC_AUTONEG_CONFIG ); + val &= ~(MVNETA_GMAC_FORCE_LINK_PASS | +MVNETA_GMAC_FORCE_LINK_DOWN | +MVNETA_GMAC_AN_FLOW_CTRL_EN ); + val |=MVNETA_GMAC_INBAND_AN_ENABLE | +MVNETA_GMAC_AN_SPEED_EN | +MVNETA_GMAC_AN_DUPLEX_EN ; +mvreg_write (pp,MVNETA_GMAC_AUTONEG_CONFIG , val); + val =mvreg_read (pp,MVNETA_GMAC_CLOCK_DIVIDER ); + val |=MVNETA_GMAC_1MS_CLOCK_ENABLE ; +mvreg_write (pp,MVNETA_GMAC_CLOCK_DIVIDER , val); + } +mvneta_set_ucast_table (pp,-1 );mvneta_set_special_mcast_table (pp,-1 );mvneta_set_other_mcast_table (pp,-1 );
@@ -2043,6 +2064,29 @@ static irqreturn_t mvneta_isr(int irq, void *dev_id)return IRQ_HANDLED ; } +static int mvneta_fixed_link_update (struct net_device *dev, +struct fixed_phy_status *status) +{ +struct mvneta_port *pp; +u32 gmac_stat; + +if (!dev) { +/* we have a bug here */ +return -EINVAL ; + } + pp =netdev_priv (dev); + gmac_stat =mvreg_read (pp,MVNETA_GMAC_STATUS ); + status->link = !!(gmac_stat &MVNETA_GMAC_LINK_UP ); +if (gmac_stat &MVNETA_GMAC_SPEED_1000 ) + status->speed =SPEED_1000 ; +else if (gmac_stat &MVNETA_GMAC_SPEED_100 ) + status->speed =SPEED_100 ; +else + status->speed =SPEED_10 ; + status->duplex = !!(gmac_stat &MVNETA_GMAC_FULL_DUPLEX ); +return 0 ; +} +/* NAPI handler * Bits 0 - 7 of the causeRxTx register indicate that are transmitted * packets on the corresponding TXQ (Bit 0 is for TX queue 1).
@@ -2063,8 +2107,12 @@ static int mvneta_poll(struct napi_struct *napi, int budget) }/* Read cause register */ - cause_rx_tx =mvreg_read (pp,MVNETA_INTR_NEW_CAUSE ) & - (MVNETA_RX_INTR_MASK (rxq_number) |MVNETA_TX_INTR_MASK (txq_number)); + cause_rx_tx =mvreg_read (pp,MVNETA_INTR_NEW_CAUSE ); +if (cause_rx_tx &MVNETA_MISCINTR_INTR_MASK ) { +/* since phylib uses polling, not much to do here... +* Any way to tell phylib to do the poll NOW? */ +mvreg_write (pp,MVNETA_INTR_MISC_CAUSE ,0 ); + }/* Release Tx descriptors */ if (cause_rx_tx &MVNETA_TX_INTR_MASK_ALL ) {
@@ -2109,7 +2157,9 @@ static int mvneta_poll(struct napi_struct *napi, int budget)napi_complete (napi);local_irq_save (flags);mvreg_write (pp,MVNETA_INTR_NEW_MASK , -MVNETA_RX_INTR_MASK (rxq_number) |MVNETA_TX_INTR_MASK (txq_number)); +MVNETA_RX_INTR_MASK (rxq_number) | +MVNETA_TX_INTR_MASK (txq_number) | +MVNETA_MISCINTR_INTR_MASK );local_irq_restore (flags); }
@@ -2373,7 +2423,13 @@ static void mvneta_start_dev(struct mvneta_port *pp)/* Unmask interrupts */ mvreg_write (pp,MVNETA_INTR_NEW_MASK , -MVNETA_RX_INTR_MASK (rxq_number) |MVNETA_TX_INTR_MASK (txq_number)); +MVNETA_RX_INTR_MASK (rxq_number) | +MVNETA_TX_INTR_MASK (txq_number) | +MVNETA_MISCINTR_INTR_MASK ); +mvreg_write (pp,MVNETA_INTR_MISC_MASK , +MVNETA_CAUSE_PHY_STATUS_CHANGE | +MVNETA_CAUSE_LINK_CHANGE | +MVNETA_CAUSE_PSC_SYNC_CHANGE );phy_start (pp->phy_dev);netif_tx_start_all_queues (pp->dev);
@@ -2523,9 +2579,7 @@ static void mvneta_adjust_link(struct net_device *ndev) val =mvreg_read (pp,MVNETA_GMAC_AUTONEG_CONFIG ); val &= ~(MVNETA_GMAC_CONFIG_MII_SPEED |MVNETA_GMAC_CONFIG_GMII_SPEED | -MVNETA_GMAC_CONFIG_FULL_DUPLEX | -MVNETA_GMAC_AN_SPEED_EN | -MVNETA_GMAC_AN_DUPLEX_EN ); +MVNETA_GMAC_CONFIG_FULL_DUPLEX );if (phydev->duplex) val |=MVNETA_GMAC_CONFIG_FULL_DUPLEX ;
@@ -2554,12 +2608,24 @@ static void mvneta_adjust_link(struct net_device *ndev)if (status_change) {if (phydev->link) { -u32 val =mvreg_read (pp,MVNETA_GMAC_AUTONEG_CONFIG ); - val |= (MVNETA_GMAC_FORCE_LINK_PASS | -MVNETA_GMAC_FORCE_LINK_DOWN ); -mvreg_write (pp,MVNETA_GMAC_AUTONEG_CONFIG , val); +if (!pp->inband_status) { +u32 val =mvreg_read (pp, +MVNETA_GMAC_AUTONEG_CONFIG ); + val &= ~MVNETA_GMAC_FORCE_LINK_DOWN ; + val |=MVNETA_GMAC_FORCE_LINK_PASS ; +mvreg_write (pp,MVNETA_GMAC_AUTONEG_CONFIG , + val); + }mvneta_port_up (pp); }else { +if (!pp->inband_status) { +u32 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); + }mvneta_port_down (pp); }phy_print_status (phydev);
@@ -2910,6 +2976,9 @@ static int mvneta_port_power_up(struct mvneta_port *pp, int phy_mode)return -EINVAL ; } +if (pp->inband_status) + ctrl |=MVNETA_GMAC2_INBAND_AN_ENABLE ; +/* Cancel Port Reset */ ctrl &= ~MVNETA_GMAC2_PORT_RESET ;mvreg_write (pp,MVNETA_GMAC_CTRL_2 , ctrl);
@@ -2934,6 +3003,7 @@ static int mvneta_probe(struct platform_device *pdev)char hw_mac_addr[ETH_ALEN ];const char *mac_from;int phy_mode; +int fixed_phy =0 ;int err;/* Our multiqueue support is not complete, so for now, only
@@ -2956,6 +3026,7 @@ static int mvneta_probe(struct platform_device *pdev) phy_node =of_parse_phandle (dn,"phy" ,0 );if (!phy_node) { +struct phy_device *phy;if (!of_phy_is_fixed_link (dn)) {dev_err (&pdev->dev,"no PHY specified\n" ); err = -ENODEV ;
@@ -2967,11 +3038,14 @@ static int mvneta_probe(struct platform_device *pdev)dev_err (&pdev->dev,"cannot register fixed PHY\n" ); goto err_free_irq; } + fixed_phy =1 ;/* In the case of a fixed PHY, the DT node associated * to the PHY is the Ethernet MAC DT node. */ phy_node =of_node_get (dn); + phy =of_phy_find_device (dn); +fixed_phy_set_link_update (phy, mvneta_fixed_link_update); } phy_mode =of_get_phy_mode (dn);
@@ -2990,6 +3064,7 @@ static int mvneta_probe(struct platform_device *pdev) pp =netdev_priv (dev); pp->phy_node = phy_node; pp->phy_interface = phy_mode; + pp->inband_status = (phy_mode ==PHY_INTERFACE_MODE_SGMII ) && fixed_phy; pp->clk =devm_clk_get (&pdev->dev,NULL );if (IS_ERR (pp->clk)) {
Signature · 1 line
1.7.9.5
Re: [PATCH 4/6] of: add API for changing parameters of fixed link
2015-03-30 7:39 GMT-07:00 Stas Sergeev <[email protected]>:
Quoted 13 lines
27.03.2015 20:15, Florian Fainelli пишет:
Quoted 3 lines
I think your concerns are valid, but I don't think there is going to be any problem with the approach I suggested because there is a contract that the fixed PHYs and regular PHYs need to
Hello Florian.
As promised, today I tried to resurrect my first implementation and do things as you suggested: install the
link_updatecallback for mvneta privately.
I feel SGMII setup is very common and deserves the separate API, not the per-driver handling, but in any case, I'd like to show the implementation first, then discuss.
Quoted 6 lines
Unfortunately, it didn't work quite right.
The problem is that mvneta callsphy_disconnect()on .ndo_stop callback. After that,phy->attached_devbecomes NULL, and so thelink_updatecallback gets called with net_dev==NULL! And crashs. Of course I can easily work around that, but IMHO its a bug - the one that actually gets fixed by the patches I posted previously.
I actually submitted some patches a while ago that allow you to unregister the fixed_link_update callback before in case you need to, precisely for that. Since this is specific to dealing with a fixed PHY, it is the driver responsibility to know that is has registered a fixed_link_update callback and then unregister it by passing a NULL callback as the new callback.
So yes, it is a bug in the sense that it is not transparently handled, but at the same time, the PHY library has no way to know whether a fixed_link_update callback is being invoked since it is not poking into the fixed PHY driver.
Quoted 2 lines
They were changing the callback to receive
phy_deviceinstead ofnet_device, and so NULL will never be passed.
Ok, but then you may still poll a PHY which has no network device attached to it, so that leads to worse drivers which might not be doing the right thing wrt. power management or other things.
Quoted 5 lines
I am attaching the new patch so that you can take a look and decide. If you still think its fine, even despite the NULL passing w/a, then I'll mail it with the proper boilerplate. But if you agree that passing NULL to
link_updateis a bug, then maybe you'll decide to get the whole surgery thing. :)
[snip]
drivers/net/ethernet/marvell/mvneta.c struct mvneta_port { · line 313
Quoted 5 lines
drivers/net/ethernet/marvell/mvneta.c +1
-0@@ -304,6 +310,7 @@ struct mvneta_port {unsigned int link;unsigned int duplex;unsigned int speed; +int inband_status;
Since you are essentially using this variable as a boolean to indicate whether in-band status should be queried or not, maybe you should name that "needs_inband_status" or "wants_inband_status", inband_status would suggest that this is the data-structure holding all the in-band status information, but that's pure nitpicking.
--
Florian
Re: [PATCH 4/6] of: add API for changing parameters of fixed link
30.03.2015 19:06, Florian Fainelli пишет:
Quoted 4 lines
So yes, it is a bug in the sense that it is not transparently handled, but at the same time, the PHY library has no way to know whether a
fixed_link_updatecallback is being invoked since it is not poking into the fixed PHY driver.
Maybe then it would be better to have an API in fixed_phy.c itself to change the state? No one will then care about the callback at all. For example, currently in my new patch, when I receive the interrupt about phy status change, I can't do anything: I can't change the state or force the phylib to do a callback right now. So, instead of changing the state upon interrupt, I need instead to ask HW the current state on every poll, which is likely not effective in some regards. By having the API that would be solved, as well as the detached device problem.
Quoted 8 lines
drivers/net/ethernet/marvell/mvneta.c struct mvneta_port { · line 313
Quoted 5 lines
drivers/net/ethernet/marvell/mvneta.c +1
-0@@ -304,6 +310,7 @@ struct mvneta_port {unsigned int link;unsigned int duplex;unsigned int speed; +int inband_status;Since you are essentially using this variable as a boolean to indicate whether in-band status should be queried or not, maybe you should name that "
needs_inband_status" or "wants_inband_status",inband_status
That's fine, will go for "use_inband_status:1" tomorrow.
Re: [PATCH 4/6] of: add API for changing parameters of fixed link
In reply to Florian Fainelli30.03.2015 19:06, Florian Fainelli пишет:
Quoted 21 lines
2015-03-30 7:39 GMT-07:00 Stas Sergeev <[email protected]>:
Quoted 13 lines
27.03.2015 20:15, Florian Fainelli пишет:
Quoted 3 lines
I think your concerns are valid, but I don't think there is going to be any problem with the approach I suggested because there is a contract that the fixed PHYs and regular PHYs need to
Hello Florian.
As promised, today I tried to resurrect my first implementation and do things as you suggested: install the
link_updatecallback for mvneta privately.
I feel SGMII setup is very common and deserves the separate API, not the per-driver handling, but in any case, I'd like to show the implementation first, then discuss.Quoted 6 lines
Unfortunately, it didn't work quite right.
The problem is that mvneta callsphy_disconnect()on .ndo_stop callback. After that,phy->attached_devbecomes NULL, and so thelink_updatecallback gets called with net_dev==NULL! And crashs. Of course I can easily work around that, but IMHO its a bug - the one that actually gets fixed by the patches I posted previously.
Quoted 6 lines
I actually submitted some patches a while ago that allow you to unregister the
fixed_link_updatecallback before in case you need to, precisely for that. Since this is specific to dealing with a fixed PHY, it is the driver responsibility to know that is has registered afixed_link_updatecallback and then unregister it by passing a NULL callback as the new callback.
Today I posted the patch that does exactly that.
But I already see the problem:
---
[ 10.535424] mvneta f1030000.ethernet eth1: Link is Up - Unsupported (update phy.c)/Half - flow control off
---
Seems like if I register a callback after of_phy_connect() (and unregister before disconnect), there is a small window between connect and setting the callback. If someone at that time will query phy, he'll get an undefined status (speed). And before of_phy_connect() the phy_device pointer is not known to register callback earlier. Of course I can see the possible work-arounds, but I wonder if something better can be done than using of_phy_find_device() and related magic right before of_phy_connect() just for that.
PATCH 5/6 mvneta: implement SGMII-based in-band link state signaling
In reply to Stas SergeevWhen MDIO bus is unavailable (common setup for SGMII), the in-band signaling must be used to correctly track link state. This patch enables the in-band status delivery and interrupts for links state changes, namely:
- link up/down
- link speed
- duplex full/half
Upon reciving the appropriate interrupt, the driver updates the fixed_phy status to match the received status.
--- drivers/net/ethernet/marvell/mvneta.c | 92 +++++++++++++++++++++++++++++---- 1 file changed, 81 insertions(+), 11 deletions(-)
drivers/net/ethernet/marvell/mvneta.c +81 -11
@@ -122,6 +122,7 @@#define MVNETA_TX_INTR_MASK_ALL (0xff << 0)#define MVNETA_RX_INTR_MASK (nr_rxqs) (((1 << nr_rxqs) - 1) << 8)#define MVNETA_RX_INTR_MASK_ALL (0xff << 8) +#define MVNETA_MISCINTR_INTR_MASK BIT(31)#define MVNETA_INTR_OLD_CAUSE 0x25a8#define MVNETA_INTR_OLD_MASK 0x25ac
@@ -180,6 +181,7 @@#define MVNETA_GMAC_AUTONEG_CONFIG 0x2c0c#define MVNETA_GMAC_FORCE_LINK_DOWN BIT(0)#define MVNETA_GMAC_FORCE_LINK_PASS BIT(1) +#define MVNETA_GMAC_INBAND_AN_ENABLE BIT(2)#define MVNETA_GMAC_CONFIG_MII_SPEED BIT(5)#define MVNETA_GMAC_CONFIG_GMII_SPEED BIT(6)#define MVNETA_GMAC_AN_SPEED_EN BIT(7)
@@ -304,6 +306,7 @@ struct mvneta_port {unsigned int link;unsigned int duplex;unsigned int speed; +int inband_status; };/* The mvneta_tx_desc and mvneta_rx_desc structures describe the
@@ -994,6 +997,16 @@ static void mvneta_defaults_set(struct mvneta_port *pp) val &= ~MVNETA_PHY_POLLING_ENABLE ;mvreg_write (pp,MVNETA_UNIT_CONTROL , val); +if (pp->inband_status) { + val =mvreg_read (pp,MVNETA_GMAC_AUTONEG_CONFIG ); + val &= ~(MVNETA_GMAC_FORCE_LINK_PASS | +MVNETA_GMAC_FORCE_LINK_DOWN ); + val |=MVNETA_GMAC_INBAND_AN_ENABLE | +MVNETA_GMAC_AN_SPEED_EN | +MVNETA_GMAC_AN_DUPLEX_EN ; +mvreg_write (pp,MVNETA_GMAC_AUTONEG_CONFIG , val); + } +mvneta_set_ucast_table (pp,-1 );mvneta_set_special_mcast_table (pp,-1 );mvneta_set_other_mcast_table (pp,-1 );
@@ -2043,6 +2056,21 @@ static irqreturn_t mvneta_isr(int irq, void *dev_id)return IRQ_HANDLED ; } +static void mvneta_get_phy_status (struct mvneta_port *pp, +struct fixed_phy_status *r_stat) +{ +u32 gmac_stat =mvreg_read (pp,MVNETA_GMAC_STATUS ); + + r_stat->link = !!(gmac_stat &MVNETA_GMAC_LINK_UP ); +if (gmac_stat &MVNETA_GMAC_SPEED_1000 ) + r_stat->speed =SPEED_1000 ; +else if (gmac_stat &MVNETA_GMAC_SPEED_100 ) + r_stat->speed =SPEED_100 ; +else + r_stat->speed =SPEED_10 ; + r_stat->duplex = !!(gmac_stat &MVNETA_GMAC_FULL_DUPLEX ); +} +/* NAPI handler * Bits 0 - 7 of the causeRxTx register indicate that are transmitted * packets on the corresponding TXQ (Bit 0 is for TX queue 1).
@@ -2063,8 +2091,29 @@ static int mvneta_poll(struct napi_struct *napi, int budget) }/* Read cause register */ - cause_rx_tx =mvreg_read (pp,MVNETA_INTR_NEW_CAUSE ) & - (MVNETA_RX_INTR_MASK (rxq_number) |MVNETA_TX_INTR_MASK (txq_number)); + cause_rx_tx =mvreg_read (pp,MVNETA_INTR_NEW_CAUSE ); +if (cause_rx_tx &MVNETA_MISCINTR_INTR_MASK ) { +u32 cause_misc =mvreg_read (pp,MVNETA_INTR_MISC_CAUSE ); + +mvreg_write (pp,MVNETA_INTR_MISC_CAUSE ,0 ); +if (pp->inband_status && (cause_misc & + (MVNETA_CAUSE_PHY_STATUS_CHANGE | +MVNETA_CAUSE_LINK_CHANGE | +MVNETA_CAUSE_PSC_SYNC_CHANGE ))) { +struct fixed_phy_status phy_stat; + +mvneta_get_phy_status (pp, &phy_stat); +if (pp->link != phy_stat.link) +of_phy_fixed_link_set_link (pp->phy_dev, + phy_stat.link); +if (pp->speed != phy_stat.speed) +of_phy_fixed_link_set_speed (pp->phy_dev, + phy_stat.speed); +if (pp->duplex != phy_stat.duplex) +of_phy_fixed_link_set_duplex (pp->phy_dev, + phy_stat.duplex); + } + }/* Release Tx descriptors */ if (cause_rx_tx &MVNETA_TX_INTR_MASK_ALL ) {
@@ -2109,7 +2158,9 @@ static int mvneta_poll(struct napi_struct *napi, int budget)napi_complete (napi);local_irq_save (flags);mvreg_write (pp,MVNETA_INTR_NEW_MASK , -MVNETA_RX_INTR_MASK (rxq_number) |MVNETA_TX_INTR_MASK (txq_number)); +MVNETA_RX_INTR_MASK (rxq_number) | +MVNETA_TX_INTR_MASK (txq_number) | +MVNETA_MISCINTR_INTR_MASK );local_irq_restore (flags); }
@@ -2373,7 +2424,13 @@ static void mvneta_start_dev(struct mvneta_port *pp)/* Unmask interrupts */ mvreg_write (pp,MVNETA_INTR_NEW_MASK , -MVNETA_RX_INTR_MASK (rxq_number) |MVNETA_TX_INTR_MASK (txq_number)); +MVNETA_RX_INTR_MASK (rxq_number) | +MVNETA_TX_INTR_MASK (txq_number) | +MVNETA_MISCINTR_INTR_MASK ); +mvreg_write (pp,MVNETA_INTR_MISC_MASK , +MVNETA_CAUSE_PHY_STATUS_CHANGE | +MVNETA_CAUSE_LINK_CHANGE | +MVNETA_CAUSE_PSC_SYNC_CHANGE );phy_start (pp->phy_dev);netif_tx_start_all_queues (pp->dev);
@@ -2523,9 +2580,7 @@ static void mvneta_adjust_link(struct net_device *ndev) val =mvreg_read (pp,MVNETA_GMAC_AUTONEG_CONFIG ); val &= ~(MVNETA_GMAC_CONFIG_MII_SPEED |MVNETA_GMAC_CONFIG_GMII_SPEED | -MVNETA_GMAC_CONFIG_FULL_DUPLEX | -MVNETA_GMAC_AN_SPEED_EN | -MVNETA_GMAC_AN_DUPLEX_EN ); +MVNETA_GMAC_CONFIG_FULL_DUPLEX );if (phydev->duplex) val |=MVNETA_GMAC_CONFIG_FULL_DUPLEX ;
@@ -2554,12 +2609,24 @@ static void mvneta_adjust_link(struct net_device *ndev)if (status_change) {if (phydev->link) { -u32 val =mvreg_read (pp,MVNETA_GMAC_AUTONEG_CONFIG ); - val |= (MVNETA_GMAC_FORCE_LINK_PASS | -MVNETA_GMAC_FORCE_LINK_DOWN ); -mvreg_write (pp,MVNETA_GMAC_AUTONEG_CONFIG , val); +if (!pp->inband_status) { +u32 val =mvreg_read (pp, +MVNETA_GMAC_AUTONEG_CONFIG ); + val &= ~MVNETA_GMAC_FORCE_LINK_DOWN ; + val |=MVNETA_GMAC_FORCE_LINK_PASS ; +mvreg_write (pp,MVNETA_GMAC_AUTONEG_CONFIG , + val); + }mvneta_port_up (pp); }else { +if (!pp->inband_status) { +u32 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); + }mvneta_port_down (pp); }phy_print_status (phydev);
@@ -2934,6 +3001,7 @@ static int mvneta_probe(struct platform_device *pdev)char hw_mac_addr[ETH_ALEN ];const char *mac_from;int phy_mode; +int fixed_phy =0 ;int err;/* Our multiqueue support is not complete, so for now, only
@@ -2967,6 +3035,7 @@ static int mvneta_probe(struct platform_device *pdev)dev_err (&pdev->dev,"cannot register fixed PHY\n" ); goto err_free_irq; } + fixed_phy =1 ;/* In the case of a fixed PHY, the DT node associated * to the PHY is the Ethernet MAC DT node.
@@ -2990,6 +3059,7 @@ static int mvneta_probe(struct platform_device *pdev) pp =netdev_priv (dev); pp->phy_node = phy_node; pp->phy_interface = phy_mode; + pp->inband_status = (phy_mode ==PHY_INTERFACE_MODE_SGMII ) && fixed_phy; pp->clk =devm_clk_get (&pdev->dev,NULL );if (IS_ERR (pp->clk)) {
Signature · 1 line
1.7.9.5
Re: [5/6] mvneta: implement SGMII-based in-band link state signaling
Hi Stas,
Quoted 9 lines · Stas Sergeev
When MDIO bus is unavailable (common setup for SGMII), the in-band signaling must be used to correctly track link state. This patch enables the in-band status delivery and interrupts for links state changes, namely:
- link up/down
- link speed
- duplex full/half
Upon reciving the appropriate interrupt, the driver updates thefixed_phystatus to match the received status.
I'm seeing a regression with this patch when trying to netboot an Armada XP board (by reverting this commit it is fine), the network link stays down:
[ 9.274492] mvneta d0070000.ethernet eth0: Link is Down
(I've added an extra call to phy_print_status() in mvneta_adjust_link() to get this trace).
I've tried to dig a bit in the code, and it seems that the status.link flag never gets set in mvneta_fixed_link_update(). If I try to force the use_inband_status to 0 in mvneta_probe(), it boots properly so I'm not sure that I need the in-band status/delivery in my case ; I'm using a custom DTB with a fixed-link:
eth0: ethernet@70000 {
status = "okay";
fixed-link = <1 1 1000 0 0>;
phy-mode = "sgmii";
};Could there be something missing in the condition that initializes pp->use_inband_status?
PS: I've also tried to apply this patch without success:
https://lkml.org/lkml/2015/6/18/496
Signature · 1 line
Sébastien
Re: [5/6] mvneta: implement SGMII-based in-band link state signaling
08.07.2015 19:30, Sebastien Rannou пишет:
Quoted 28 lines
Hi Stas,
Quoted 9 lines · Stas Sergeev
When MDIO bus is unavailable (common setup for SGMII), the in-band signaling must be used to correctly track link state. This patch enables the in-band status delivery and interrupts for links state changes, namely:
- link up/down
- link speed
- duplex full/half
Upon reciving the appropriate interrupt, the driver updates thefixed_phystatus to match the received status.I'm seeing a regression with this patch when trying to netboot an Armada XP board (by reverting this commit it is fine), the network link stays down:
[ 9.274492] mvneta d0070000.ethernet eth0: Link is Down(I've added an extra call to
phy_print_status()inmvneta_adjust_link()to get this trace).I've tried to dig a bit in the code, and it seems that the status.link flag never gets set in
mvneta_fixed_link_update(). If I try to force theuse_inband_statusto 0 inmvneta_probe(), it boots properly so I'm not sure that I need the in-band status/delivery in my case ; I'm using a custom DTB with a fixed-link:
Quoted 8 lines
eth0: ethernet@70000 { status = "okay"; fixed-link = <1 1 1000 0 0>; phy-mode = "sgmii"; };Could there be something missing in the condition that initializes
pp->use_inband_status?
Hi, use_inband_status is set when fixed-link is used, which is exactly your case. But it seems something on the other end is not generating the inband status. What is there? A phy chip, or something else?
Perhaps some DT property should be added to explicitly enable the use of the inband status... I just thought in sgmii protocol it is a mandatory.
I'll try to come up with the patch tomorrow that adds such property.
Re: [5/6] mvneta: implement SGMII-based in-band link state signaling
Quoted 1 line · Stas Sergeev
What is there? A phy chip, or something else?
It's "something else", there's a phy which aggregates 4xSGMIIs to 1xQSGMII, we are on the media side here, the MAC side is connected to the switch through QSGMII.
Quoted 2 lines
Perhaps some DT property should be added to explicitly enable the use of the inband status...
Yes, that would be fine.
Quoted 2 lines
I'll try to come up with the patch tomorrow that adds such property.
Thanks! I can help for testing.
Signature · 1 line
Sébastien
Re: [5/6] mvneta: implement SGMII-based in-band link state signaling
Sebastien, Stas,
Quoted 4 lines · Sebastien Rannou
Quoted 1 line · Stas Sergeev
What is there? A phy chip, or something else?
Quoted 8 lines
It's "something else", there's a phy which aggregates 4xSGMIIs to 1xQSGMII, we are on the media side here, the MAC side is connected to the switch through QSGMII.
Quoted 2 lines
Perhaps some DT property should be added to explicitly enable the use of the inband status...
Yes, that would be fine.
Isn't it a bit weird to need a new DT property for this? Shouldn't fixed-link always imply this inband status thing?
Thomas
Signature · 3 lines
Thomas Petazzoni, CTO, Free Electrons Embedded Linux, Kernel and Android engineering http://free-electrons.com
Re: [5/6] mvneta: implement SGMII-based in-band link state signaling
09.07.2015 12:19, Thomas Petazzoni пишет:
Quoted 11 lines
Sebastien, Stas,
Quoted 8 lines · Sebastien Rannou
Quoted 1 line · Stas Sergeev
What is there? A phy chip, or something else?
It's "something else", there's a phy which aggregates 4xSGMIIs to 1xQSGMII, we are on the media side here, the MAC side is connected to the switch through QSGMII.
Quoted 7 lines
Quoted 4 lines
Quoted 2 lines
Perhaps some DT property should be added to explicitly enable the use of the inband status...
Yes, that would be fine.
Isn't it a bit weird to need a new DT property for this? Shouldn't fixed-link always imply this inband status thing?
That's how it is currently implemented. I thought its safe. But what if the device on the other end does not generate the inband status? I think this device is doing the wrong thing, but nevertheless we have a regression at hands.
Currently the link status cannot be specified for fixed-link, at all.
What I am going to code up, is the new property, like this:
fixed-link {
link = "up" | "down" | "auto";
};
"auto" will mean the inband status.
Looks like a simple solution.
PATCH 6/6 mvneta: port marvell's official in-band status enabling procedure
In reply to Stas SergeevThis is a back-port of official marvell's patch. It changes nothing visible for me but looks reasonable. Hope people with access to marvell's specs can review and justify this patch better.
--- drivers/net/ethernet/marvell/mvneta.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-)
drivers/net/ethernet/marvell/mvneta.c +12 -1
@@ -100,6 +100,8 @@#define MVNETA_TXQ_CMD 0x2448#define MVNETA_TXQ_DISABLE_SHIFT 8#define MVNETA_TXQ_ENABLE_MASK 0x000000ff +#define MVNETA_GMAC_CLOCK_DIVIDER 0x24f4 +#define MVNETA_GMAC_1MS_CLOCK_ENABLE BIT(31)#define MVNETA_ACC_MODE 0x2500#define MVNETA_CPU_MAP (cpu) (0x2540 + ((cpu) << 2))#define MVNETA_CPU_RXQ_ACCESS_ALL_MASK 0x000000ff
@@ -166,6 +168,7 @@#define MVNETA_GMAC_MAX_RX_SIZE_MASK 0x7ffc#define MVNETA_GMAC0_PORT_ENABLE BIT(0)#define MVNETA_GMAC_CTRL_2 0x2c08 +#define MVNETA_GMAC2_INBAND_AN_ENABLE BIT(0)#define MVNETA_GMAC2_PCS_ENABLE BIT(3)#define MVNETA_GMAC2_PORT_RGMII BIT(4)#define MVNETA_GMAC2_PORT_RESET BIT(6)
@@ -185,6 +188,7 @@#define MVNETA_GMAC_CONFIG_MII_SPEED BIT(5)#define MVNETA_GMAC_CONFIG_GMII_SPEED BIT(6)#define MVNETA_GMAC_AN_SPEED_EN BIT(7) +#define MVNETA_GMAC_AN_FLOW_CTRL_EN BIT(11)#define MVNETA_GMAC_CONFIG_FULL_DUPLEX BIT(12)#define MVNETA_GMAC_AN_DUPLEX_EN BIT(13)#define MVNETA_MIB_COUNTERS_BASE 0x3080
@@ -1000,11 +1004,15 @@ static void mvneta_defaults_set(struct mvneta_port *pp)if (pp->inband_status) { val =mvreg_read (pp,MVNETA_GMAC_AUTONEG_CONFIG ); val &= ~(MVNETA_GMAC_FORCE_LINK_PASS | -MVNETA_GMAC_FORCE_LINK_DOWN ); +MVNETA_GMAC_FORCE_LINK_DOWN | +MVNETA_GMAC_AN_FLOW_CTRL_EN ); val |=MVNETA_GMAC_INBAND_AN_ENABLE |MVNETA_GMAC_AN_SPEED_EN |MVNETA_GMAC_AN_DUPLEX_EN ;mvreg_write (pp,MVNETA_GMAC_AUTONEG_CONFIG , val); + val =mvreg_read (pp,MVNETA_GMAC_CLOCK_DIVIDER ); + val |=MVNETA_GMAC_1MS_CLOCK_ENABLE ; +mvreg_write (pp,MVNETA_GMAC_CLOCK_DIVIDER , val); }mvneta_set_ucast_table (pp,-1 );
@@ -2977,6 +2985,9 @@ static int mvneta_port_power_up(struct mvneta_port *pp, int phy_mode)return -EINVAL ; } +if (pp->inband_status) + ctrl |=MVNETA_GMAC2_INBAND_AN_ENABLE ; +/* Cancel Port Reset */ ctrl &= ~MVNETA_GMAC2_PORT_RESET ;mvreg_write (pp,MVNETA_GMAC_CTRL_2 , ctrl);
Signature · 1 line
1.7.9.5
Andrew Lunn
maintainerQuoted 6 lines · Stas Sergeev
Hello.
Currently the fixed-link DT binding is pre-configured and cannot be changed in run-time. This means the cable unplug events are not being detected, and the link parameters can't be negotiated.
O.K, i will ask the dumb question....
Isn't fixed-link supposed to be used for links which are fixed? Why would a fixed-link change?
The use cases i've seen for this is when you are connecting the MAC to a Switch. The link is configured to its fastest mode and then left alone.
Please could you give some more background information. What do you have on the other end of this fixed link which keeps changing? Obviously not a switch.
Thanks
Andrew
27.03.2015 16:39, Andrew Lunn пишет:
Quoted 8 lines · Stas Sergeev
Quoted 6 lines
Hello.
Currently the fixed-link DT binding is pre-configured and cannot be changed in run-time. This means the cable unplug events are not being detected, and the link parameters can't be negotiated.
Quoted 4 lines
O.K, i will ask the dumb question....
Isn't fixed-link supposed to be used for links which are fixed? Why would a fixed-link change?
Hi, my current understanding of fixed-link DT binding is that it is actually just an MDIO-less link, but not more fixed than that. I made a patch out of that assumption.
Why I think that way is because fixed-link internally uses a fixed_phy driver - the one that actually allows a state updates and emulates MDIO on top of these. So I just exploited that already coded capability. I think any other approach will require much more work.
Quoted 7 lines
The use cases i've seen for this is when you are connecting the MAC to a Switch. The link is configured to its fastest mode and then left alone.
Please could you give some more background information. What do you have on the other end of this fixed link which keeps changing? Obviously not a switch.
Normal PHY, not a switch.
But there is no MDIO, because SGMII AFAIK doesn't need MDIO. SGMII has in-band status, but for some reason it seems currently linux is not ready for such setup - this is what my patch addresses. Or maybe I am missing something?
Andrew Lunn
maintainerQuoted 3 lines
But there is no MDIO, because SGMII AFAIK doesn't need MDIO. SGMII has in-band status, but for some reason it seems currently linux is not ready for such setup - this is what my patch addresses.
Hacking the fixed-link driver feels wrong to me. You probably want to implement an SGMII-link driver. With some refactoring you can probably re-use some of the fixed-link driver code, but it should be a separate driver, with its own DT binding.
But i'm no export here. So lets see what others say.
Andrew
27.03.2015 16:59, Andrew Lunn пишет:
Quoted 8 lines
Quoted 3 lines
But there is no MDIO, because SGMII AFAIK doesn't need MDIO. SGMII has in-band status, but for some reason it seems currently linux is not ready for such setup - this is what my patch addresses.
Hacking the fixed-link driver feels wrong to me. You probably want to implement an SGMII-link driver. With some refactoring you can probably re-use some of the fixed-link driver code, but it should be a separate driver, with its own DT binding.
Well I took the path of "the least resistance" of course... and the one that doesn't look unreasonable to me, but the separate driver is an option too. Though it will really be just the "same fixed-link with 3 added functions to change properties". Do we really need 2 mostly similar drivers? Esp when one includes the functionality of the other, and just adds a bit on top? Maybe having just one, with all the capabilities added?
For example, maybe someone will later want the ability to alter the properties of fixed-link with ethtool or alike? Will this also require "just another fixed link driver"?
Having one fully-featured driver will allow us to add the "sgmii-link" DT binding as an alias to "fixed-link" and some function like of_phy_fixed_link_is_sgmii() that will tell us whether we need an in-band status or not.
Quoted 1 line
But i'm no export here. So lets see what others say.
Right. If people will be negative to an addition to fixed-link driver, I'll take a look into adding a new DT binding - something I'd really like to avoid doing. :) More work, more code, and yet even a new config option - this have to be justified.
2015-03-27 7:20 GMT-07:00 Stas Sergeev <[email protected]>:
Quoted 24 lines
27.03.2015 16:59, Andrew Lunn пишет:
Quoted 8 lines
Quoted 3 lines
But there is no MDIO, because SGMII AFAIK doesn't need MDIO. SGMII has in-band status, but for some reason it seems currently linux is not ready for such setup - this is what my patch addresses.
Hacking the fixed-link driver feels wrong to me. You probably want to implement an SGMII-link driver. With some refactoring you can probably re-use some of the fixed-link driver code, but it should be a separate driver, with its own DT binding.
Well I took the path of "the least resistance" of course... and the one that doesn't look unreasonable to me, but the separate driver is an option too. Though it will really be just the "same fixed-link with 3 added functions to change properties". Do we really need 2 mostly similar drivers? Esp when one includes the functionality of the other, and just adds a bit on top? Maybe having just one, with all the capabilities added?
For example, maybe someone will later want the ability to alter the properties of fixed-link with ethtool or alike? Will this also require "just another fixed link driver"?
Having one fully-featured driver will allow us to add the "sgmii-link" DT binding as an alias to "fixed-link" and some function likeof_phy_fixed_link_is_sgmii()that will tell us whether we need an in-band status or not.
Quoted 5 lines
Quoted 1 line
But i'm no export here. So lets see what others say.
Right. If people will be negative to an addition to fixed-link driver, I'll take a look into adding a new DT binding - something I'd really like to avoid doing. :) More work, more code, and yet even a new config option - this have to be justified.
See my other reply, I think that by register a fixed_link_update callback and adjusting fixed_phy_status based on the SGMII in-band decoded data you would get all you want to work without major surgery to the current code. If we are missing information in fixed_phy_status to support SGMII-specific properties, maybe we could add those.
Signature · 1 line
Florian