| Message ID | 20181212123308.20100-1-joakim.tjernlund@infinera.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [PATCHv2] gianfar: Add gfar_change_carrier() for Fixed PHYs | expand |
On Wed, Dec 12, 2018 at 01:33:08PM +0100, Joakim Tjernlund wrote: > This allows to control carrier from /sys/class/net/ethX/carrier for > Fixed PHYs. > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com> > --- > v2 - Only allow carrier changes for Fixed PHYs > > Florian: I have reimpl. this as I think you meant by registering > a Fixed PHY callback. > Andrew: Are happy with this as well? Hi Joakim The basic idea is O.K. > > If this is OK I will sent the other 2 drivers. Rather than replicating the code three times, how about putting all the code in the fixed phy driver, exporting a fixed_phy_change_carrier function which can be assigned to the .ndo. Andrew
>-----Original Message----- >From: Andrew Lunn <andrew@lunn.ch> >Sent: Wednesday, December 12, 2018 2:43 PM >To: jocke@infinera.com <joakim.tjernlund@infinera.com> >Cc: netdev @ vger . kernel . org <netdev@vger.kernel.org>; Claudiu Manoil ><claudiu.manoil@nxp.com>; Florian Fainelli <f.fainelli@gmail.com> >Subject: Re: [PATCHv2] gianfar: Add gfar_change_carrier() for Fixed PHYs > >On Wed, Dec 12, 2018 at 01:33:08PM +0100, Joakim Tjernlund wrote: >> This allows to control carrier from /sys/class/net/ethX/carrier for >> Fixed PHYs. >> >> Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com> >> --- >> v2 - Only allow carrier changes for Fixed PHYs >> >> Florian: I have reimpl. this as I think you meant by registering >> a Fixed PHY callback. >> Andrew: Are happy with this as well? > >Hi Joakim > >The basic idea is O.K. > >> >> If this is OK I will sent the other 2 drivers. > >Rather than replicating the code three times, how about putting all >the code in the fixed phy driver, exporting a fixed_phy_change_carrier >function which can be assigned to the .ndo. > I agree with Andrew on this. This code is not device (eTSEC) specific, it is generic and only uses the gianfar driver to get a reference to its net_device. Thanks, Claudiu
On Wed, 2018-12-12 at 13:10 +0000, Claudiu Manoil wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. > > > > -----Original Message----- > > From: Andrew Lunn <andrew@lunn.ch> > > Sent: Wednesday, December 12, 2018 2:43 PM > > To: jocke@infinera.com <joakim.tjernlund@infinera.com> > > Cc: netdev @ vger . kernel . org <netdev@vger.kernel.org>; Claudiu Manoil > > <claudiu.manoil@nxp.com>; Florian Fainelli <f.fainelli@gmail.com> > > Subject: Re: [PATCHv2] gianfar: Add gfar_change_carrier() for Fixed PHYs > > > > On Wed, Dec 12, 2018 at 01:33:08PM +0100, Joakim Tjernlund wrote: > > > This allows to control carrier from /sys/class/net/ethX/carrier for > > > Fixed PHYs. > > > > > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com> > > > --- > > > v2 - Only allow carrier changes for Fixed PHYs > > > > > > Florian: I have reimpl. this as I think you meant by registering > > > a Fixed PHY callback. > > > Andrew: Are happy with this as well? > > > > Hi Joakim > > > > The basic idea is O.K. > > > > > If this is OK I will sent the other 2 drivers. > > > > Rather than replicating the code three times, how about putting all > > the code in the fixed phy driver, exporting a fixed_phy_change_carrier > > function which can be assigned to the .ndo. > > > > I agree with Andrew on this. This code is not device (eTSEC) specific, it is generic > and only uses the gianfar driver to get a reference to its net_device. > Thanks, > Claudiu Yes, point taken. Looking into moving most stuff into Fixed PHY Jocke
On Wed, 2018-12-12 at 13:10 +0000, Claudiu Manoil wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. > > > > -----Original Message----- > > From: Andrew Lunn <andrew@lunn.ch> > > Sent: Wednesday, December 12, 2018 2:43 PM > > To: jocke@infinera.com <joakim.tjernlund@infinera.com> > > Cc: netdev @ vger . kernel . org <netdev@vger.kernel.org>; Claudiu Manoil > > <claudiu.manoil@nxp.com>; Florian Fainelli <f.fainelli@gmail.com> > > Subject: Re: [PATCHv2] gianfar: Add gfar_change_carrier() for Fixed PHYs > > > > On Wed, Dec 12, 2018 at 01:33:08PM +0100, Joakim Tjernlund wrote: > > > This allows to control carrier from /sys/class/net/ethX/carrier for > > > Fixed PHYs. > > > > > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com> > > > --- > > > v2 - Only allow carrier changes for Fixed PHYs > > > > > > Florian: I have reimpl. this as I think you meant by registering > > > a Fixed PHY callback. > > > Andrew: Are happy with this as well? > > > > Hi Joakim > > > > The basic idea is O.K. > > > > > If this is OK I will sent the other 2 drivers. > > > > Rather than replicating the code three times, how about putting all > > the code in the fixed phy driver, exporting a fixed_phy_change_carrier > > function which can be assigned to the .ndo. > > > > I agree with Andrew on this. This code is not device (eTSEC) specific, it is generic > and only uses the gianfar driver to get a reference to its net_device. > Thanks, > Claudiu fast check, would you be happy with this in fixed PHY: --- a/drivers/net/phy/fixed_phy.c +++ b/drivers/net/phy/fixed_phy.c @@ -25,6 +25,7 @@ #include <linux/gpio.h> #include <linux/seqlock.h> #include <linux/idr.h> +#include <linux/netdevice.h> #include "swphy.h" @@ -38,6 +39,7 @@ struct fixed_phy { struct phy_device *phydev; seqcount_t seqcount; struct fixed_phy_status status; + bool no_carrier; int (*link_update)(struct net_device *, struct fixed_phy_status *); struct list_head node; int link_gpio; @@ -48,6 +50,24 @@ static struct fixed_mdio_bus platform_fmb = { .phys = LIST_HEAD_INIT(platform_fmb.phys), }; +int +fixed_phy_change_carrier(struct net_device *dev, bool new_carrier) +{ + struct fixed_mdio_bus *fmb = &platform_fmb; + struct phy_device *phydev = dev->phydev; + struct fixed_phy *fp; + + if (!phydev || !phydev->mdio.bus) + return -EINVAL; + + list_for_each_entry(fp, &fmb->phys, node) { + if (fp->addr == phydev->mdio.addr) { + fp->no_carrier = !new_carrier; + return 0; + } + } + return -EINVAL; +} static void fixed_phy_update(struct fixed_phy *fp) { if (gpio_is_valid(fp->link_gpio)) @@ -72,6 +92,12 @@ static int fixed_mdio_read(struct mii_bus *bus, int phy_addr, int reg_num) &fp->status); fixed_phy_update(fp); } + if (fp->no_carrier) { + fp->link_update(fp->phydev->attached_dev, + &fp->status); + fp->status.link = 0; + } + state = fp->status; } while (read_seqcount_retry(&fp->seqcount, s)); Then one just set: + .ndo_change_carrier = fixed_phy_change_carrier, in each ethernet driver. OK? Jocke
> fast check, would you be happy with this in fixed PHY: > --- a/drivers/net/phy/fixed_phy.c > +++ b/drivers/net/phy/fixed_phy.c > @@ -25,6 +25,7 @@ > #include <linux/gpio.h> > #include <linux/seqlock.h> > #include <linux/idr.h> > +#include <linux/netdevice.h> > > #include "swphy.h" > > @@ -38,6 +39,7 @@ struct fixed_phy { > struct phy_device *phydev; > seqcount_t seqcount; > struct fixed_phy_status status; > + bool no_carrier; > int (*link_update)(struct net_device *, struct fixed_phy_status *); > struct list_head node; > int link_gpio; > @@ -48,6 +50,24 @@ static struct fixed_mdio_bus platform_fmb = { > .phys = LIST_HEAD_INIT(platform_fmb.phys), > }; > > +int > +fixed_phy_change_carrier(struct net_device *dev, bool new_carrier) > +{ > + struct fixed_mdio_bus *fmb = &platform_fmb; > + struct phy_device *phydev = dev->phydev; > + struct fixed_phy *fp; > + > + if (!phydev || !phydev->mdio.bus) > + return -EINVAL; > + > + list_for_each_entry(fp, &fmb->phys, node) { > + if (fp->addr == phydev->mdio.addr) { > + fp->no_carrier = !new_carrier; Why is no_carrier needed? You can directly change fp->status.link, so long as you take care of locking. > + return 0; > + } > + } > + return -EINVAL; > +} You need to export the function. Andrew
On Wed, 2018-12-12 at 15:51 +0100, Andrew Lunn wrote: > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender and know the content is safe. > > > > fast check, would you be happy with this in fixed PHY: > > --- a/drivers/net/phy/fixed_phy.c > > +++ b/drivers/net/phy/fixed_phy.c > > @@ -25,6 +25,7 @@ > > #include <linux/gpio.h> > > #include <linux/seqlock.h> > > #include <linux/idr.h> > > +#include <linux/netdevice.h> > > > > #include "swphy.h" > > > > @@ -38,6 +39,7 @@ struct fixed_phy { > > struct phy_device *phydev; > > seqcount_t seqcount; > > struct fixed_phy_status status; > > + bool no_carrier; > > int (*link_update)(struct net_device *, struct fixed_phy_status *); > > struct list_head node; > > int link_gpio; > > @@ -48,6 +50,24 @@ static struct fixed_mdio_bus platform_fmb = { > > .phys = LIST_HEAD_INIT(platform_fmb.phys), > > }; > > > > +int > > +fixed_phy_change_carrier(struct net_device *dev, bool new_carrier) > > +{ > > + struct fixed_mdio_bus *fmb = &platform_fmb; > > + struct phy_device *phydev = dev->phydev; > > + struct fixed_phy *fp; > > + > > + if (!phydev || !phydev->mdio.bus) > > + return -EINVAL; > > + > > + list_for_each_entry(fp, &fmb->phys, node) { > > + if (fp->addr == phydev->mdio.addr) { > > + fp->no_carrier = !new_carrier; > > Why is no_carrier needed? You can directly change fp->status.link, so > long as you take care of locking. I to remember it for the gpio control of link state, see below. > > > + return 0; > > + } > > + } > > + return -EINVAL; > > +} > > You need to export the function. Yes, it was not a complete patch. Below is what I have now and is happy with: diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c index eb5167210681..aedebc251997 100644 --- a/drivers/net/phy/fixed_phy.c +++ b/drivers/net/phy/fixed_phy.c @@ -25,6 +25,7 @@ #include <linux/gpio.h> #include <linux/seqlock.h> #include <linux/idr.h> +#include <linux/netdevice.h> #include "swphy.h" @@ -38,6 +39,7 @@ struct fixed_phy { struct phy_device *phydev; seqcount_t seqcount; struct fixed_phy_status status; + bool no_carrier; int (*link_update)(struct net_device *, struct fixed_phy_status *); struct list_head node; int link_gpio; @@ -48,9 +50,27 @@ static struct fixed_mdio_bus platform_fmb = { .phys = LIST_HEAD_INIT(platform_fmb.phys), }; +int +fixed_phy_change_carrier(struct net_device *dev, bool new_carrier) +{ + struct fixed_mdio_bus *fmb = &platform_fmb; + struct phy_device *phydev = dev->phydev; + struct fixed_phy *fp; + + if (!phydev || !phydev->mdio.bus) + return -EINVAL; + + list_for_each_entry(fp, &fmb->phys, node) { + if (fp->addr == phydev->mdio.addr) { + fp->no_carrier = !new_carrier; + return 0; + } + } + return -EINVAL; +} static void fixed_phy_update(struct fixed_phy *fp) { - if (gpio_is_valid(fp->link_gpio)) + if (!fp->no_carrier && gpio_is_valid(fp->link_gpio)) fp->status.link = !!gpio_get_value_cansleep(fp->link_gpio); } @@ -66,6 +86,7 @@ static int fixed_mdio_read(struct mii_bus *bus, int phy_addr, int reg_num) do { s = read_seqcount_begin(&fp->seqcount); + fp->status.link = !fp->no_carrier; /* Issue callback if user registered it. */ if (fp->link_update) { fp->link_update(fp->phydev->attached_dev, diff --git a/include/linux/phy_fixed.h b/include/linux/phy_fixed.h index cf6392de6eb0..09d7e613cffc 100644 --- a/include/linux/phy_fixed.h +++ b/include/linux/phy_fixed.h @@ -27,6 +27,7 @@ extern int fixed_phy_set_link_update(struct phy_device *phydev, extern int fixed_phy_update_state(struct phy_device *phydev, const struct fixed_phy_status *status, const struct fixed_phy_status *changed); +extern int fixed_phy_change_carrier(struct net_device *dev, bool new_carrier); #else static inline int fixed_phy_add(unsigned int irq, int phy_id, struct fixed_phy_status *status, @@ -56,6 +57,10 @@ static inline int fixed_phy_update_state(struct phy_device *phydev, { return -ENODEV; } +static inline int fixed_phy_change_carrier(struct net_device *dev, bool new_carrier) +{ + return -EINVAL; +} #endif /* CONFIG_FIXED_PHY */ #endif /* __PHY_FIXED_H */
On 12/12/18 4:33 AM, Joakim Tjernlund wrote: > This allows to control carrier from /sys/class/net/ethX/carrier for > Fixed PHYs. > > Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com> > --- > v2 - Only allow carrier changes for Fixed PHYs > > Florian: I have reimpl. this as I think you meant by registering > a Fixed PHY callback. > Andrew: Are happy with this as well? Yes that is exactly what I had in mind, thanks for listening and translating that into code :) Looking forward to the generic version being submitted. Thanks! > > If this is OK I will sent the other 2 drivers. > > drivers/net/ethernet/freescale/gianfar.c | 26 ++++++++++++++++++++++++ > drivers/net/ethernet/freescale/gianfar.h | 2 +- > 2 files changed, 27 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c > index 63daae120b2d..49971093e10f 100644 > --- a/drivers/net/ethernet/freescale/gianfar.c > +++ b/drivers/net/ethernet/freescale/gianfar.c > @@ -491,7 +491,28 @@ static int gfar_set_mac_addr(struct net_device *dev, void *p) > > return 0; > } > +static int gfar_fixed_phy_link_update(struct net_device *dev, > + struct fixed_phy_status *status) > +{ > + struct gfar_private *priv; > + > + if (dev && dev->phydev && status) { > + priv = netdev_priv(dev); > + status->link = !priv->no_carrier; > + } > + return 0; > +} > +static int gfar_change_carrier(struct net_device *dev, bool new_carrier) > +{ > + struct phy_device *phydev = dev->phydev; > + struct gfar_private *priv; > > + if (!phy_is_pseudo_fixed_link(phydev)) > + return -EINVAL; > + priv = netdev_priv(dev); > + priv->no_carrier = !new_carrier; > + return 0; > +} > static const struct net_device_ops gfar_netdev_ops = { > .ndo_open = gfar_enet_open, > .ndo_start_xmit = gfar_start_xmit, > @@ -502,6 +523,7 @@ static const struct net_device_ops gfar_netdev_ops = { > .ndo_tx_timeout = gfar_timeout, > .ndo_do_ioctl = gfar_ioctl, > .ndo_get_stats = gfar_get_stats, > + .ndo_change_carrier = gfar_change_carrier, > .ndo_set_mac_address = gfar_set_mac_addr, > .ndo_validate_addr = eth_validate_addr, > #ifdef CONFIG_NET_POLL_CONTROLLER > @@ -1807,6 +1829,10 @@ static int init_phy(struct net_device *dev) > return -ENODEV; > } > > + if (phy_is_pseudo_fixed_link(phydev)) > + fixed_phy_set_link_update(phydev, > + gfar_fixed_phy_link_update); > + > if (interface == PHY_INTERFACE_MODE_SGMII) > gfar_configure_serdes(dev); > > diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h > index 5aa814799d70..94a64d2dcc6f 100644 > --- a/drivers/net/ethernet/freescale/gianfar.h > +++ b/drivers/net/ethernet/freescale/gianfar.h > @@ -1158,7 +1158,7 @@ struct gfar_private { > int oldspeed; > int oldduplex; > int oldlink; > - > + bool no_carrier; > uint32_t msg_enable; > > struct work_struct reset_task; >
diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c index 63daae120b2d..49971093e10f 100644 --- a/drivers/net/ethernet/freescale/gianfar.c +++ b/drivers/net/ethernet/freescale/gianfar.c @@ -491,7 +491,28 @@ static int gfar_set_mac_addr(struct net_device *dev, void *p) return 0; } +static int gfar_fixed_phy_link_update(struct net_device *dev, + struct fixed_phy_status *status) +{ + struct gfar_private *priv; + + if (dev && dev->phydev && status) { + priv = netdev_priv(dev); + status->link = !priv->no_carrier; + } + return 0; +} +static int gfar_change_carrier(struct net_device *dev, bool new_carrier) +{ + struct phy_device *phydev = dev->phydev; + struct gfar_private *priv; + if (!phy_is_pseudo_fixed_link(phydev)) + return -EINVAL; + priv = netdev_priv(dev); + priv->no_carrier = !new_carrier; + return 0; +} static const struct net_device_ops gfar_netdev_ops = { .ndo_open = gfar_enet_open, .ndo_start_xmit = gfar_start_xmit, @@ -502,6 +523,7 @@ static const struct net_device_ops gfar_netdev_ops = { .ndo_tx_timeout = gfar_timeout, .ndo_do_ioctl = gfar_ioctl, .ndo_get_stats = gfar_get_stats, + .ndo_change_carrier = gfar_change_carrier, .ndo_set_mac_address = gfar_set_mac_addr, .ndo_validate_addr = eth_validate_addr, #ifdef CONFIG_NET_POLL_CONTROLLER @@ -1807,6 +1829,10 @@ static int init_phy(struct net_device *dev) return -ENODEV; } + if (phy_is_pseudo_fixed_link(phydev)) + fixed_phy_set_link_update(phydev, + gfar_fixed_phy_link_update); + if (interface == PHY_INTERFACE_MODE_SGMII) gfar_configure_serdes(dev); diff --git a/drivers/net/ethernet/freescale/gianfar.h b/drivers/net/ethernet/freescale/gianfar.h index 5aa814799d70..94a64d2dcc6f 100644 --- a/drivers/net/ethernet/freescale/gianfar.h +++ b/drivers/net/ethernet/freescale/gianfar.h @@ -1158,7 +1158,7 @@ struct gfar_private { int oldspeed; int oldduplex; int oldlink; - + bool no_carrier; uint32_t msg_enable; struct work_struct reset_task;
This allows to control carrier from /sys/class/net/ethX/carrier for Fixed PHYs. Signed-off-by: Joakim Tjernlund <joakim.tjernlund@infinera.com> --- v2 - Only allow carrier changes for Fixed PHYs Florian: I have reimpl. this as I think you meant by registering a Fixed PHY callback. Andrew: Are happy with this as well? If this is OK I will sent the other 2 drivers. drivers/net/ethernet/freescale/gianfar.c | 26 ++++++++++++++++++++++++ drivers/net/ethernet/freescale/gianfar.h | 2 +- 2 files changed, 27 insertions(+), 1 deletion(-)