| Message ID | cover.1521656774.git.richardcochran@gmail.com |
|---|---|
| Headers | show |
| Series | Peer to Peer One-Step time stamping | expand |
On 03/21/2018 11:58 AM, Richard Cochran wrote: > There are different ways of obtaining hardware time stamps on network > packets. The ingress and egress times can be measured in the MAC, in > the PHY, or by a device listening on the MII bus. Up until now, the > kernel has support for MAC and PHY time stamping, but not for other > MII bus devices. > > This patch moves the PHY time stamping interface into the generic > mdio device in order to support MII time stamping hardware. > > Signed-off-by: Richard Cochran <richardcochran@gmail.com> > --- > drivers/net/phy/dp83640.c | 29 ++++++++++++++++++++--------- > drivers/net/phy/phy.c | 4 ++-- > include/linux/mdio.h | 23 +++++++++++++++++++++++ > include/linux/phy.h | 23 ----------------------- > net/core/ethtool.c | 4 ++-- > net/core/timestamping.c | 8 ++++---- > 6 files changed, 51 insertions(+), 40 deletions(-) > > diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c > index 654f42d00092..79aeb5eb471a 100644 > --- a/drivers/net/phy/dp83640.c > +++ b/drivers/net/phy/dp83640.c > @@ -215,6 +215,10 @@ static LIST_HEAD(phyter_clocks); > static DEFINE_MUTEX(phyter_clocks_lock); > > static void rx_timestamp_work(struct work_struct *work); > +static int dp83640_ts_info(struct mdio_device *m, struct ethtool_ts_info *i); > +static int dp83640_hwtstamp(struct mdio_device *m, struct ifreq *i); > +static bool dp83640_rxtstamp(struct mdio_device *m, struct sk_buff *s, int t); > +static void dp83640_txtstamp(struct mdio_device *m, struct sk_buff *s, int t); > > /* extended register access functions */ > > @@ -1162,6 +1166,12 @@ static int dp83640_probe(struct phy_device *phydev) > list_add_tail(&dp83640->list, &clock->phylist); > > dp83640_clock_put(clock); > + > + phydev->mdio.ts_info = dp83640_ts_info; > + phydev->mdio.hwtstamp = dp83640_hwtstamp; > + phydev->mdio.rxtstamp = dp83640_rxtstamp; > + phydev->mdio.txtstamp = dp83640_txtstamp; Why is this implemented a the mdio_device level and not at the mdio_driver level? This looks like the wrong level at which this is done. -- Florian -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 03/21/2018 11:58 AM, Richard Cochran wrote: > This patch adds a new field to the network device structure to reference > a time stamping device on the MII bus. By decoupling the time stamping > function from the PHY device, we pave the way to allowing a non-PHY > device to take this role. > > Signed-off-by: Richard Cochran <richardcochran@gmail.com> > --- > drivers/net/phy/mdio_bus.c | 51 +++++++++++++++++++++++++++++++++++++++++++++- > include/linux/netdevice.h | 1 + > 2 files changed, 51 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/phy/mdio_bus.c b/drivers/net/phy/mdio_bus.c > index 24b5511222c8..fdac8c8ac272 100644 > --- a/drivers/net/phy/mdio_bus.c > +++ b/drivers/net/phy/mdio_bus.c > @@ -717,6 +717,47 @@ static int mdio_uevent(struct device *dev, struct kobj_uevent_env *env) > return 0; > } > > +static bool mdiodev_supports_timestamping(struct mdio_device *mdiodev) > +{ > + if (mdiodev->ts_info && mdiodev->hwtstamp && > + mdiodev->rxtstamp && mdiodev->txtstamp) > + return true; > + else > + return false; > +} > + > +static int mdiobus_netdev_notification(struct notifier_block *nb, > + unsigned long msg, void *ptr) > +{ > + struct net_device *netdev = netdev_notifier_info_to_dev(ptr); > + struct phy_device *phydev = netdev->phydev; > + struct mdio_device *mdev; > + struct mii_bus *bus; > + int i; > + > + if (netdev->mdiots || msg != NETDEV_UP || !phydev) > + return NOTIFY_DONE; You are still assuming that we have a phy_device somehow, whereas you parch series wants to solve that for generic MDIO devices, that is a bit confusing. > + > + /* > + * Examine the MII bus associated with the PHY that is > + * attached to the MAC. If there is a time stamping device > + * on the bus, then connect it to the network device. > + */ > + bus = phydev->mdio.bus; > + > + for (i = 0; i < PHY_MAX_ADDR; i++) { > + mdev = bus->mdio_map[i]; > + if (!mdev) > + continue; > + if (mdiodev_supports_timestamping(mdev)) { > + netdev->mdiots = mdev; > + return NOTIFY_OK; What guarantees that netdev->mdiots gets cleared? Also, why is this done with a notifier instead of through phy_{connect,attach,disconnect}? It looks like we still have this requirement of the mdio TS device being a phy_device somehow, I am confused here... > + } > + } > + > + return NOTIFY_DONE; > +} > + > #ifdef CONFIG_PM > static int mdio_bus_suspend(struct device *dev) > { > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > index 5fbb9f1da7fd..223d691aa0b0 100644 > --- a/include/linux/netdevice.h > +++ b/include/linux/netdevice.h > @@ -1943,6 +1943,7 @@ struct net_device { > struct netprio_map __rcu *priomap; > #endif > struct phy_device *phydev; > + struct mdio_device *mdiots; phy_device embedds a mdio_device, can you find a way to rework the PHY PTP code to utilize the phy_device's mdio instance so do not introduce yet another pointer in that big structure that net_device already is? > struct lock_class_key *qdisc_tx_busylock; > struct lock_class_key *qdisc_running_key; > bool proto_down; >
> -----Original Message----- > From: netdev-owner@vger.kernel.org [mailto:netdev-owner@vger.kernel.org] > On Behalf Of Richard Cochran > Sent: Wednesday, March 21, 2018 11:58 AM > To: netdev@vger.kernel.org > Cc: devicetree@vger.kernel.org; Andrew Lunn <andrew@lunn.ch>; David Miller > <davem@davemloft.net>; Florian Fainelli <f.fainelli@gmail.com>; Mark Rutland > <mark.rutland@arm.com>; Miroslav Lichvar <mlichvar@redhat.com>; Rob > Herring <robh+dt@kernel.org>; Willem de Bruijn <willemb@google.com> > Subject: [PATCH net-next RFC V1 1/5] net: Introduce peer to peer one step PTP > + > + /* > + * Same as HWTSTAMP_TX_ONESTEP_SYNC, but also enables time > + * stamp insertion directly into PDelay_Resp packets. In this > + * case, neither transmitted Sync nor PDelay_Resp packets will > + * receive a time stamp via the socket error queue. > + */ > + HWTSTAMP_TX_ONESTEP_P2P, > }; > I am guessing that we expect all devices which support onestep P2P messages, will always support onestep SYNC as well? Thanks, Jake -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 21, 2018 at 08:05:36PM +0000, Keller, Jacob E wrote:
> I am guessing that we expect all devices which support onestep P2P messages, will always support onestep SYNC as well?
Yes. Anything else doesn't make sense, don't you think?
Also, reading 1588, it isn't clear whether supporting only 1-step Sync
without 1-step P2P is even intended. There is only a "one-step
clock", and it is described as doing both.
Thanks,
Richard
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 21, 2018 at 12:10:07PM -0700, Florian Fainelli wrote: > > + phydev->mdio.ts_info = dp83640_ts_info; > > + phydev->mdio.hwtstamp = dp83640_hwtstamp; > > + phydev->mdio.rxtstamp = dp83640_rxtstamp; > > + phydev->mdio.txtstamp = dp83640_txtstamp; > > Why is this implemented a the mdio_device level and not at the > mdio_driver level? This looks like the wrong level at which this is done. The question could be asked of: struct mdio_device { int (*bus_match)(struct device *dev, struct device_driver *drv); void (*device_free)(struct mdio_device *mdiodev); void (*device_remove)(struct mdio_device *mdiodev); } I saw how this is done for the phy, etc, but I don't see any benefit of doing it that way. It would add an extra layer (or two) of indirection and save the space four pointer functions. Is that trade-off worth it? Thanks, Richard -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 21, 2018 at 12:12:00PM -0700, Florian Fainelli wrote: > > +static int mdiobus_netdev_notification(struct notifier_block *nb, > > + unsigned long msg, void *ptr) > > +{ > > + struct net_device *netdev = netdev_notifier_info_to_dev(ptr); > > + struct phy_device *phydev = netdev->phydev; > > + struct mdio_device *mdev; > > + struct mii_bus *bus; > > + int i; > > + > > + if (netdev->mdiots || msg != NETDEV_UP || !phydev) > > + return NOTIFY_DONE; > > You are still assuming that we have a phy_device somehow, whereas you > parch series wants to solve that for generic MDIO devices, that is a bit > confusing. The phydev is the only thing that associates a netdev with an MII bus. > > + > > + /* > > + * Examine the MII bus associated with the PHY that is > > + * attached to the MAC. If there is a time stamping device > > + * on the bus, then connect it to the network device. > > + */ > > + bus = phydev->mdio.bus; > > + > > + for (i = 0; i < PHY_MAX_ADDR; i++) { > > + mdev = bus->mdio_map[i]; > > + if (!mdev) > > + continue; > > + if (mdiodev_supports_timestamping(mdev)) { > > + netdev->mdiots = mdev; > > + return NOTIFY_OK; > > What guarantees that netdev->mdiots gets cleared? Why would it need to be cleared? > Also, why is this done > with a notifier instead of through phy_{connect,attach,disconnect}? We have no guarantee the mdio device has been probed yet. > It > looks like we still have this requirement of the mdio TS device being a > phy_device somehow, I am confused here... We only need the phydev to get from the netdev to the mii bus. > > + } > > + } > > + > > + return NOTIFY_DONE; > > +} > > + > > #ifdef CONFIG_PM > > static int mdio_bus_suspend(struct device *dev) > > { > > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > > index 5fbb9f1da7fd..223d691aa0b0 100644 > > --- a/include/linux/netdevice.h > > +++ b/include/linux/netdevice.h > > @@ -1943,6 +1943,7 @@ struct net_device { > > struct netprio_map __rcu *priomap; > > #endif > > struct phy_device *phydev; > > + struct mdio_device *mdiots; > > phy_device embedds a mdio_device, can you find a way to rework the PHY > PTP code to utilize the phy_device's mdio instance so do not introduce > yet another pointer in that big structure that net_device already is? It would be strange and wrong to "steal" the phy's mdio struct, IMHO. After all, we just got support for non-PHY mdio devices. The natural solution is to use it. Thanks, Richard -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> -----Original Message----- > From: Richard Cochran [mailto:richardcochran@gmail.com] > Sent: Wednesday, March 21, 2018 2:26 PM > To: Keller, Jacob E <jacob.e.keller@intel.com> > Cc: netdev@vger.kernel.org; devicetree@vger.kernel.org; Andrew Lunn > <andrew@lunn.ch>; David Miller <davem@davemloft.net>; Florian Fainelli > <f.fainelli@gmail.com>; Mark Rutland <mark.rutland@arm.com>; Miroslav > Lichvar <mlichvar@redhat.com>; Rob Herring <robh+dt@kernel.org>; Willem de > Bruijn <willemb@google.com> > Subject: Re: [PATCH net-next RFC V1 1/5] net: Introduce peer to peer one step > PTP time stamping. > > On Wed, Mar 21, 2018 at 08:05:36PM +0000, Keller, Jacob E wrote: > > I am guessing that we expect all devices which support onestep P2P messages, > will always support onestep SYNC as well? > > Yes. Anything else doesn't make sense, don't you think? > > Also, reading 1588, it isn't clear whether supporting only 1-step Sync > without 1-step P2P is even intended. There is only a "one-step > clock", and it is described as doing both. > > Thanks, > Richard This was my understanding as well, but given the limited hardware which can do sync but not pdelay messages, I just wanted to make sure we were on the same page. Thanks, Jake -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 21, 2018 at 02:45:13PM -0700, Richard Cochran wrote: > On Wed, Mar 21, 2018 at 12:10:07PM -0700, Florian Fainelli wrote: > > > + phydev->mdio.ts_info = dp83640_ts_info; > > > + phydev->mdio.hwtstamp = dp83640_hwtstamp; > > > + phydev->mdio.rxtstamp = dp83640_rxtstamp; > > > + phydev->mdio.txtstamp = dp83640_txtstamp; > > > > Why is this implemented a the mdio_device level and not at the > > mdio_driver level? This looks like the wrong level at which this is done. > > The question could be asked of: > > struct mdio_device { > int (*bus_match)(struct device *dev, struct device_driver *drv); > void (*device_free)(struct mdio_device *mdiodev); > void (*device_remove)(struct mdio_device *mdiodev); > } > > I saw how this is done for the phy, etc, but I don't see any benefit > of doing it that way. It would add an extra layer (or two) of > indirection and save the space four pointer functions. Is that > trade-off worth it? Actually, there was another reason not to put these methods into the driver structure. In contrast to phy_device, mdio_device doesn't have a pointer to the driver structure. It looks like there is no way to start from a mdio_device and get to a mdio_driver. Please correct me if I'm wrong. I propose keeping those methods in the mdio_device, wrapping them in #ifdef CONFIG_NETWORK_PHY_TIMESTAMPING. That way, they do not add any burden to the vast majority of users. Thoughts? Thanks, Richard -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Mar 21, 2018 at 12:12:00PM -0700, Florian Fainelli wrote: > > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h > > index 5fbb9f1da7fd..223d691aa0b0 100644 > > --- a/include/linux/netdevice.h > > +++ b/include/linux/netdevice.h > > @@ -1943,6 +1943,7 @@ struct net_device { > > struct netprio_map __rcu *priomap; > > #endif > > struct phy_device *phydev; > > + struct mdio_device *mdiots; > > phy_device embedds a mdio_device, can you find a way to rework the PHY > PTP code to utilize the phy_device's mdio instance so do not introduce > yet another pointer in that big structure that net_device already is? You are right in that this field is wasted space for most users. In V2 this will be inside #ifdef CONFIG_NETWORK_PHY_TIMESTAMPING. Thanks, Richard -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html