Message ID | e9def53dd0f751d7f5e3df47e604d7ccb6020159.1521656774.git.richardcochran@gmail.com |
---|---|
State | Changes Requested, archived |
Headers | show |
Series | Peer to Peer One-Step time stamping | expand |
On Wed, Mar 21, 2018 at 11:58:18AM -0700, Richard Cochran wrote: > The InES at the ZHAW offers a PTP time stamping IP core. The FPGA > logic recognizes and time stamps PTP frames on the MII bus. This > patch adds a driver for the core along with a device tree binding to > allow hooking the driver to MAC devices. Hi Richard Can you point us at some documentation for this. I think Florian and I want to better understand how this device works, in order to understand your other changes. Andrew -- 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:33:15PM +0100, Andrew Lunn wrote: > Can you point us at some documentation for this. The overall one-step functionality is described IEEE 1588. > I think Florian and I want to better understand how this device works, > in order to understand your other changes. The device is from here: https://www.zhaw.ch/en/engineering/institutes-centres/ines/products-and-services/ptp-ieee-1588/ptp-hardware/#c43991 The only other docs that I have is a PDF of the register layout, but I don't think I can redistribute that. Actually, there really isn't any detail in that doc at all. 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
Hi Richard > The only other docs that I have is a PDF of the register layout, but I > don't think I can redistribute that. Actually, there really isn't any > detail in that doc at all. O.K, so lets do the 20 questions approach. As far as i can see, this is not an MDIO device. It is not connected to the MDIO bus, it has no MDIO registers, you don't even pass a valid MDIO address in device tree. It it actually an MII bus snooper? Does it snoop, or is it actually in the MII bus, and can modify packets, i.e. insert time stamps as frames pass over the MII bus? When the driver talks about having three ports, does that mean it can be on three different MII busses? Thanks Andrew -- 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 10:44:36PM +0100, Andrew Lunn wrote: > O.K, so lets do the 20 questions approach. :) > As far as i can see, this is not an MDIO device. It is not connected > to the MDIO bus, it has no MDIO registers, you don't even pass a valid > MDIO address in device tree. Right. There might very well be other products out there that *do* use MDIO commands. I know that there are MII time stamping asics and ip cores on the market, but I don't know all of their creative design details. > It it actually an MII bus snooper? Does it snoop, or is it actually in > the MII bus, and can modify packets, i.e. insert time stamps as frames > pass over the MII bus? It acts like a "snooper" to provide out of band time stamps, but it also can modify packets when for the one-step functionality. > When the driver talks about having three ports, does that mean it can > be on three different MII busses? Yes. HTH, 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 02:57:29PM -0700, Richard Cochran wrote: > On Wed, Mar 21, 2018 at 10:44:36PM +0100, Andrew Lunn wrote: > > O.K, so lets do the 20 questions approach. > > :) > > > As far as i can see, this is not an MDIO device. It is not connected > > to the MDIO bus, it has no MDIO registers, you don't even pass a valid > > MDIO address in device tree. > > Right. O.K, so i suggest we stop trying to model this thing as an MDIO device. It is really an MMIO device. > There might very well be other products out there that *do* > use MDIO commands. I know that there are MII time stamping asics and > ip cores on the market, but I don't know all of their creative design > details. So i suggest we leave the design for those until we actual see one. > > It it actually an MII bus snooper? Does it snoop, or is it actually in > > the MII bus, and can modify packets, i.e. insert time stamps as frames > > pass over the MII bus? > > It acts like a "snooper" to provide out of band time stamps, but it > also can modify packets when for the one-step functionality. > > > When the driver talks about having three ports, does that mean it can > > be on three different MII busses? O.K, so here is how i think it should be done. It is a device which offers services to other devices. It is not that different to an interrupt controller, a GPIO controller, etc. Lets follow how they work in device tree.... The device itself is just another MMIO mapped device in the SoC: timestamper@60000000 { compatible = "ines,ptp-ctrl"; reg = <0x60000000 0x80>; #address-cells = <1>; #size-cells = <0>; }; The MAC drivers are clients of this device. They then use a phandle and specifier: eth0: ethernet-controller@72000 { compatible = "marvell,kirkwood-eth"; #address-cells = <1>; #size-cells = <0>; reg = <0x72000 0x4000>; timerstamper = <×tamper 2> } The 2 indicates this MAC is using port 2. The MAC driver can then do the standard device tree things to follow the phandle to get access to the device and use the API it exports. Andrew -- 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 11:16:52PM +0100, Andrew Lunn wrote: > The MAC drivers are clients of this device. They then use a phandle > and specifier: > > eth0: ethernet-controller@72000 { > compatible = "marvell,kirkwood-eth"; > #address-cells = <1>; > #size-cells = <0>; > reg = <0x72000 0x4000>; > > timerstamper = <×tamper 2> > } > > The 2 indicates this MAC is using port 2. > > The MAC driver can then do the standard device tree things to follow > the phandle to get access to the device and use the API it exports. But that would require hacking every last MAC driver. I happy to improve the modeling, but the solution should be generic and work for every MAC driver. 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 03:47:02PM -0700, Richard Cochran wrote: > On Wed, Mar 21, 2018 at 11:16:52PM +0100, Andrew Lunn wrote: > > The MAC drivers are clients of this device. They then use a phandle > > and specifier: > > > > eth0: ethernet-controller@72000 { > > compatible = "marvell,kirkwood-eth"; > > #address-cells = <1>; > > #size-cells = <0>; > > reg = <0x72000 0x4000>; > > > > timerstamper = <×tamper 2> > > } > > > > The 2 indicates this MAC is using port 2. > > > > The MAC driver can then do the standard device tree things to follow > > the phandle to get access to the device and use the API it exports. > > But that would require hacking every last MAC driver. > > I happy to improve the modeling, but the solution should be generic > and work for every MAC driver. Well, the solution is generic, in that the phandle can point to a device anywhere. It could be MMIO, it could be on an MDIO bus, etc. You just need to make sure you API makes no assumption about how the device driver talks to the hardware. How clever is this device? Can it tell the difference between 1000Base-X and SGMII? Can it figure out that the MAC is repeating every bit 100 times and so has dropped to 10Mbits? Does it understand EEE? Does it need to know if RGMII or RGMII-ID is being used? Can such a device really operation without the MAC being involved? My feeling is it needs to understand how the MII bus is being used. It might also be that the device is less capable than the MAC, so you need to turn off some of the MAC features. I think you are going to need the MAC actively involved in this. Andrew -- 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 03:47:02PM -0700, Richard Cochran wrote: > On Wed, Mar 21, 2018 at 11:16:52PM +0100, Andrew Lunn wrote: > > The MAC drivers are clients of this device. They then use a phandle > > and specifier: > > > > eth0: ethernet-controller@72000 { > > compatible = "marvell,kirkwood-eth"; > > #address-cells = <1>; > > #size-cells = <0>; > > reg = <0x72000 0x4000>; > > > > timerstamper = <×tamper 2> > > } > > > > The 2 indicates this MAC is using port 2. > > > > The MAC driver can then do the standard device tree things to follow > > the phandle to get access to the device and use the API it exports. > > But that would require hacking every last MAC driver. > > I happy to improve the modeling, but the solution should be generic > and work for every MAC driver. Something else to think about. There are a number of MAC drivers which don't use phylib. All the intel drivers for example. They have their own MDIO and PHY code. And recently there have been a number of MAC drivers for hardware capable of > 1GBps which do all the PHY control in firmware. A phydev is optional, the MAC is mandatory. Andrew -- 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 Thu, Mar 22, 2018 at 01:43:49AM +0100, Andrew Lunn wrote: > On Wed, Mar 21, 2018 at 03:47:02PM -0700, Richard Cochran wrote: > > I happy to improve the modeling, but the solution should be generic > > and work for every MAC driver. Let me qualify that a bit... > Something else to think about. There are a number of MAC drivers which > don't use phylib. All the intel drivers for example. They have their > own MDIO and PHY code. And recently there have been a number of MAC > drivers for hardware capable of > 1GBps which do all the PHY control > in firmware. > > A phydev is optional, the MAC is mandatory. So MACs that have a built in PHY won't work, but we don't care because there is no way to hang another MII device in there anyhow. We already require phylib for NETWORK_PHY_TIMESTAMPING, and so we expect that here, too. Many of these IP core things will be targeting arm with device tree, and I want that to "just work" without MAC changes. (This is exactly the same situation with DSA, BTW.) If someone attaches an MII time stamper to a MACs whose driver does their own thing without phylib, then they are going to have to hack the MAC driver in any case. Such hacks will never be acceptable for mainline because they are design specific. We really don't have to worry about this case. 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 Thu, Mar 22, 2018 at 12:50:07AM +0100, Andrew Lunn wrote: > How clever is this device? Can it tell the difference between > 1000Base-X and SGMII? Can it figure out that the MAC is repeating > every bit 100 times and so has dropped to 10Mbits? Does it understand > EEE? Does it need to know if RGMII or RGMII-ID is being used? This device isn't configurable at run time for any of those AFAICT. Those decisions are made when the IP core is synthesized as part of the HW design. > Can such a device really operation without the MAC being involved? My > feeling is it needs to understand how the MII bus is being used. It > might also be that the device is less capable than the MAC, so you > need to turn off some of the MAC features. I think you are going to > need the MAC actively involved in this. You are right in that this particular device *does* need to know the link speed. I have neglected that part for this RFC. I'm looking for a notification based method of informing the device of link speed changes, but without hacking any MAC driver. In general, we might see devices one day that care about things like EEE for example, but let's cross that bridge when we come to it. In the case of EEE, when the user enables it via ethtool we can tell the time stamping device directly without hacking the MAC driver. 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 Sat, Mar 24, 2018 at 10:12:19AM -0700, Richard Cochran wrote: > On Thu, Mar 22, 2018 at 12:50:07AM +0100, Andrew Lunn wrote: > > How clever is this device? Can it tell the difference between > > 1000Base-X and SGMII? Can it figure out that the MAC is repeating > > every bit 100 times and so has dropped to 10Mbits? Does it understand > > EEE? Does it need to know if RGMII or RGMII-ID is being used? > > This device isn't configurable at run time for any of those AFAICT. > Those decisions are made when the IP core is synthesized as part of > the HW design. Hi Richard Should we be designing an API for this specific device, or should we think of the general case? The general case would be a device which passes through MII signals, and has some sort of control interface. The control interface could be MMIO as here, MDIO, I2C, SPI, etc. The MII interfaces could be MII, RMII, GMII, RGMII, SGMII, QSGMII, XGMII, etc. Generally, a device which can do GMII can also do RGMII, etc. The device probably needs to know about the MII bus. If it is synthesized as part of the hardware design, it might be able to get this information directly from the MAC IP core. An external device probably needs to be told. Especially when it comes to RGII, where the clocking is 'interesting'. As you already said, you need to know link speed. Do we want to be able to restrict the MAC to specific link speeds? The Marvell MACs can do a 2.5Gbps SGMII, by speeding up the clock. I can image a PTP device not supporting this, the MII breaks, but PHY looks happy. To limit this cleanly, you at least need to mask out the unsupported auto-neg offered speeds. With EEE there is probably two cases: 1) The PTP device understands it, but needs to be told it has been enabled. 2) The PTP device breaks when EEE is enabled, so EEE needs to be disabled. To me, 1) seems pretty unlikely. But 2) is possible. Disabling EEE again means changing the auto-neg parameters, so that EEE is not offered. As far as i can see, you have three basic problems: 1) How do you associate the PTP device to the netdev? 2) How do you get the information you need to configure the PTP device 3) How do you limit the MAC/PHY to what the PTP device can do. For 1) you need some sort of phandle, either in the MAC, or the PHY section of device tree. There is currently no generic code for handling MAC OF nodes. phylib however does do all the generic work for PHY nodes in DT. 2) you can get some of the information you need via notifiers. e.g, NETDEV_CHANGE tells you something has happen, up/down, etc. But i'm not sure it is 100% reliable. A PHY might be able to do 1G->100M without going down and back up again, so you don't get a NETDEV_CHANGE event. There is no notification of how the MII bus is being used. So i think notifiers is probably not the best bet. phylib does have all this information. It is phylib that calls the MAC with link speed information. When the MAC connects to the PHY, it passes the MII mode, and when the PHY requests the MII mode changes, phylib knows. The MAC has to call phy_init_eee() to see if the PHY is EEE capable. phylib also tells the MAC what speeds the PHY is capable off, so it is in the position to mask out speeds the PTP device does not support, etc. So i really think you need to cleanly integrate into phylib and phylink. Andrew -- 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 Sat, Mar 24, 2018 at 07:48:58PM +0100, Andrew Lunn wrote: > As far as i can see, you have three basic problems: > > 1) How do you associate the PTP device to the netdev? > 2) How do you get the information you need to configure the PTP device Yes, yes. > 3) How do you limit the MAC/PHY to what the PTP device can do. Hm, I don't think this is important. > phylib does have all this information. It is phylib that calls the MAC > with link speed information. When the MAC connects to the PHY, it > passes the MII mode, and when the PHY requests the MII mode changes, > phylib knows. The MAC has to call phy_init_eee() to see if the PHY is > EEE capable. phylib also tells the MAC what speeds the PHY is capable > off, so it is in the position to mask out speeds the PTP device does > not support, etc. Right, so phylib can operate on phydev->attached_dev->mdiots; > So i really think you need to cleanly integrate into phylib and > phylink. So I think I've done that, more or less, but I'd like to hear your ideas on how to make it cleaner... 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 Sat, Mar 24, 2018 at 09:51:52PM -0700, Richard Cochran wrote: > On Sat, Mar 24, 2018 at 07:48:58PM +0100, Andrew Lunn wrote: > > As far as i can see, you have three basic problems: > > > > 1) How do you associate the PTP device to the netdev? > > 2) How do you get the information you need to configure the PTP device > > Yes, yes. > > > 3) How do you limit the MAC/PHY to what the PTP device can do. > > Hm, I don't think this is important. So you are happy that the PTP device will cause the MC/PHY link to break down when it is asked to do something it cannot do? Take for example a Marvell MAC connected to a Marvell PHY doing 2.5Gbps SGMII because it can. But say the PTP device cannot be clocked that fast, and the MII links break.... You as a PTP maintainer might be happy with that, but as a PHY maintainer, i'm not too happy with this. > Right, so phylib can operate on phydev->attached_dev->mdiots; So first off, it is not an MDIO device. You current code is a horrible hack which gets a NACK. Use a phandle, and have of_mdiobus_register_phy() follow the phandle to get the device. To keep lifecycle issues simple, i would also keep it in phydev, not netdev. mdiots as a name is completely wrong. It is not an MDIO device. Maybe in the future some devices will be MDIO, or I2C, or SPI. Just call it ptpdev. This ptpdev needs to be control bus agnostic. You need a ptpdev core API exposing functions like ptpdev_hwtstamp, ptpdev_rxtstamp, ptpdev_txtstamp, ptpdev_link_change, which take a ptpdev. Have phy_link_change call ptpdev_link_change. You have the flexibility in that if in the future you do care that your ptpdev breaks the MAC-PHY link, you can add a ptpdev_validate_advertise, which allows the ptpdev to mask out link modes it does not support. Your ptp device, when probing needs to register with the ptpdev core, passing a generic ptpdev_ops for the operations its support. How it talks to the device, MMIO, SPI, I2C is hidden within the device driver. You can then clean up the code in timestamping.c. Code like: phydev = skb->dev->phydev; if (likely(phydev->drv->txtstamp)) { clone = skb_clone_sk(skb); if (!clone) return; phydev->drv->txtstamp(phydev, clone, type); } violates the layering, and the locking looks broken. Add a phy_txtstamp() call to phylib. It can then either call into the PHY driver, or use the call the ptpdev API, or -EOPNOTSUP. Andrew -- 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 Sun, Mar 25, 2018 at 05:59:37PM +0200, Andrew Lunn wrote: > > > 3) How do you limit the MAC/PHY to what the PTP device can do. > > > > Hm, I don't think this is important. > > So you are happy that the PTP device will cause the MC/PHY link to > break down when it is asked to do something it cannot do? No, but I do expect the system designer to use common sense. No need to over engineer this now just to catch hypothetical future problems. > > Right, so phylib can operate on phydev->attached_dev->mdiots; > > So first off, it is not an MDIO device. ... > To keep lifecycle issues simple, i would also keep it in phydev, not > netdev. > > mdiots as a name is completely wrong. It is not an MDIO device. I am well aware of what terms MDIO and MII represent, but our current code is hopelessly confused. Case in point: static inline struct mii_bus *mdiobus_alloc(void); The kernel's 'struct mii_bus' is really only about MDIO and not about the rest of MII. Clearly MII time stamping devices belong on the MII bus, so that is where I put them. Or maybe mii_bus should first be renamed to mdio_bus? That you pave the way for introducing a representation of the MII bus. > in the future some devices will be MDIO, or I2C, or SPI. Just call it > ptpdev. This ptpdev needs to be control bus agnostic. You need a > ptpdev core API exposing functions like ptpdev_hwtstamp, > ptpdev_rxtstamp, ptpdev_txtstamp, ptpdev_link_change, which take a > ptpdev. Well, this name is not ideal, since time stamping devices in general can time stamp any frame, not just PTP frames. > You can then clean up the code in timestamping.c. Code like: > > phydev = skb->dev->phydev; > if (likely(phydev->drv->txtstamp)) { > clone = skb_clone_sk(skb); > if (!clone) > return; > phydev->drv->txtstamp(phydev, clone, type); > } > > violates the layering, and the locking looks broken. Are you sure the locking is broken? If so, how? (This code path has been reviewed more than once.) Can you please explain the layering and how this code breaks it? (This code goes back to 2.6.36 and perhaps predates the layers that were added later on.) 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 Sun, Mar 25, 2018 at 05:59:37PM +0200, Andrew Lunn wrote: > To keep lifecycle issues simple, i would also keep it in phydev, not > netdev. Okay. Since we don't have any representation for MII anyhow, it seems equally fitting to attach this to the PHY's data structure as to the MAC's. 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 03/25/2018 03:10 PM, Richard Cochran wrote: > On Sun, Mar 25, 2018 at 05:59:37PM +0200, Andrew Lunn wrote: >>>> 3) How do you limit the MAC/PHY to what the PTP device can do. >>> >>> Hm, I don't think this is important. >> >> So you are happy that the PTP device will cause the MC/PHY link to >> break down when it is asked to do something it cannot do? > > No, but I do expect the system designer to use common sense. No need > to over engineer this now just to catch hypothetical future problems. > >>> Right, so phylib can operate on phydev->attached_dev->mdiots; >> >> So first off, it is not an MDIO device. > > ... > >> To keep lifecycle issues simple, i would also keep it in phydev, not >> netdev. >> >> mdiots as a name is completely wrong. It is not an MDIO device. > > I am well aware of what terms MDIO and MII represent, but our current > code is hopelessly confused. Case in point: > > static inline struct mii_bus *mdiobus_alloc(void); > > The kernel's 'struct mii_bus' is really only about MDIO and not about > the rest of MII. Clearly MII time stamping devices belong on the MII > bus, so that is where I put them. They do, if and only if their control path goes through the MDIO bus, this one does not, see below. > > Or maybe mii_bus should first be renamed to mdio_bus? That you pave > the way for introducing a representation of the MII bus. It would have been ideal to name this structure mdio_bus, because that is what it indeed is. An argument could be made this is true about a lot of devices though, e.g: PCIe end point drivers do register a pci_driver/device, not a pcie_driver/device... Andrew still has a valid point though that devices are child of their control bus, not data bus. The data bus here is MII, but the control bus is here through MMIO register, therefore platform device in Linux's device driver model so we would expect the The best that I can think about and it still is a hack in some way, is to you have your time stamping driver create a proxy mii_bus whose purpose is just to hook to mdio/phy_device events (such as link changes) in order to do what is necessary, or at least, this would indicate its transparent nature towards the MDIO/MDC lines... What is not great in your current binding is that you created a notion of "port" which is tied to the timestamper device, whereas it really seems to be a property of the Ethernet controller, where the datapath being timestamped resides. Tangential: the existing PHY time stamping logic should probably be generalized to a mdio_device (which the phy_device is a specialized superset of) instead of to the phy_device. This would still allow existing use cases but it would also allow us to support possible "pure MDIO" devices would that become some thing in the future. > >> in the future some devices will be MDIO, or I2C, or SPI. Just call it >> ptpdev. This ptpdev needs to be control bus agnostic. You need a >> ptpdev core API exposing functions like ptpdev_hwtstamp, >> ptpdev_rxtstamp, ptpdev_txtstamp, ptpdev_link_change, which take a >> ptpdev. > > Well, this name is not ideal, since time stamping devices in general > can time stamp any frame, not just PTP frames. > >> You can then clean up the code in timestamping.c. Code like: >> >> phydev = skb->dev->phydev; >> if (likely(phydev->drv->txtstamp)) { >> clone = skb_clone_sk(skb); >> if (!clone) >> return; >> phydev->drv->txtstamp(phydev, clone, type); >> } >> >> violates the layering, and the locking looks broken. > > Are you sure the locking is broken? If so, how? > > (This code path has been reviewed more than once.) > > Can you please explain the layering and how this code breaks it? I see it both ways, you either invoke an operation to timestamp which goes into an abstract "timestamping device" which invokes a driver to determine the actual operation, or you can do what you did which was to augment struct net_device with a phy_device, under the premise this will be the only type of timestamping capable device we will ever see. This is no longer holding true, your patches are a testament to that, so now you need another member: mdiots, as you can see, there is now overlap between the two, because a phy_device should arguably be encapsulating a mdiots device object... > > (This code goes back to 2.6.36 and perhaps predates the layers that > were added later on.) > > Thanks, > Richard >
> > You can then clean up the code in timestamping.c. Code like: > > > > phydev = skb->dev->phydev; > > if (likely(phydev->drv->txtstamp)) { > > clone = skb_clone_sk(skb); > > if (!clone) > > return; > > phydev->drv->txtstamp(phydev, clone, type); > > } > > > > violates the layering, and the locking looks broken. > > Are you sure the locking is broken? If so, how? As a general rule of thumb, locking is performed in the core when possible. Experience has shown that device driver writers get locking wrong more often than core code writers. So doing it once in the core results in less bugs. The phylib core code will take the phydev lock before calling into the driver. By violating the layering, we are missing on this lock. Maybe the one driver which currently implements these calls does not need locking. But after the Marvell DSA work, we have most of the code needed to implement support for the Marvell PHY PTP. It has pretty similar registers. That would need the phydev lock to be held, because we need to swap to different pages, so any other calls happening in parallel are going to see registers from the wrong page. I don't want to have to put these locks in the driver, that leads to bugs. The core should do it. Andrew -- 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
> > in the future some devices will be MDIO, or I2C, or SPI. Just call it > > ptpdev. This ptpdev needs to be control bus agnostic. You need a > > ptpdev core API exposing functions like ptpdev_hwtstamp, > > ptpdev_rxtstamp, ptpdev_txtstamp, ptpdev_link_change, which take a > > ptpdev. > > Well, this name is not ideal, since time stamping devices in general > can time stamp any frame, not just PTP frames. Hi Richard I don't really care about the name. I care about the semantics of the API. How about ieee1588_rxtstamp, ieee1588_txtstamp, etc. Andrew -- 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 Sun, Mar 25, 2018 at 04:01:49PM -0700, Florian Fainelli wrote: > The best that I can think about and it still is a hack in some way, is > to you have your time stamping driver create a proxy mii_bus whose > purpose is just to hook to mdio/phy_device events (such as link changes) > in order to do what is necessary, or at least, this would indicate its > transparent nature towards the MDIO/MDC lines... That won't work at all, AFAICT. There is only one mii_bus per netdev, that is one that is attached to the phydev. > Tangential: the existing PHY time stamping logic should probably be > generalized to a mdio_device (which the phy_device is a specialized > superset of) instead of to the phy_device. This would still allow > existing use cases but it would also allow us to support possible "pure > MDIO" devices would that become some thing in the future. So this is exactly what I did. The time stamping methods were pushed down into the mdio_device. The active device (mdiots pointer) points either to a non-PHY mdio_device or to the mdio_device embedded in the phydev. 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 Mon, Mar 26, 2018 at 01:01:58AM +0200, Andrew Lunn wrote: > The phylib core code will take the phydev lock before calling into the > driver. By violating the layering, we are missing on this lock. That lock protects the fields within the struct phy_device, like the state field. None of the time stamping methods need to read or write any part of that data structure. Actually it is not true that the core always takes the lock before calling the driver methods. See .ack_interrupt for example. > Maybe the one driver which currently implements these calls does not > need locking. It has locking. If a specific device needs locking to protect the integrity of its registers or other internal data structures, then it can and should implement those locks. 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 Mon, Apr 02, 2018 at 08:55:27PM -0700, Richard Cochran wrote: > On Sun, Mar 25, 2018 at 04:01:49PM -0700, Florian Fainelli wrote: > > The best that I can think about and it still is a hack in some way, is > > to you have your time stamping driver create a proxy mii_bus whose > > purpose is just to hook to mdio/phy_device events (such as link changes) > > in order to do what is necessary, or at least, this would indicate its > > transparent nature towards the MDIO/MDC lines... > > That won't work at all, AFAICT. There is only one mii_bus per netdev, > that is one that is attached to the phydev. Hi Richard Have you tried implementing it using a phandle in the phy node, pointing to the time stamping device? I think it makes a much better architecture. Andrew -- 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 Tue, Apr 03, 2018 at 03:13:19PM +0200, Andrew Lunn wrote: > Have you tried implementing it using a phandle in the phy node, > pointing to the time stamping device? Not yet, but I'll take this up for V2, after the merge window... Thinking about MII, it really is a 1:1 connection between the MAC and the PHY. It has no representation in the current code, at least not yet. It is too bad about the naming of mii_bus, oh well. While hanging this thing off of the PHY isn't really great modeling (it isn't a sub-device of the PHY in any sense), still this will work well enough to enable the new functionality. 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
diff --git a/Documentation/devicetree/bindings/net/ines-ptp.txt b/Documentation/devicetree/bindings/net/ines-ptp.txt new file mode 100644 index 000000000000..ed7b1d773ded --- /dev/null +++ b/Documentation/devicetree/bindings/net/ines-ptp.txt @@ -0,0 +1,42 @@ +ZHAW InES PTP time stamping IP core + +The IP core needs two different kinds of nodes. The control node +lives somewhere in the memory map and specifies the address of the +control registers. There can be up to three port nodes placed on the +mdio bus. They associate a particular MAC with a port index within +the IP core. + +Required properties of the control node: + +- compatible: "ines,ptp-ctrl" +- reg: physical address and size of the register bank +- phandle: globally unique handle for the ports to point to + +Required properties of the port nodes: + +- compatible: "ines,ptp-port" +- ctrl-handle: points to the control node +- port-index: port channel within the IP core +- reg: phy address. This is required even though the + device does not respond to mdio operations + +Example: + + timestamper@60000000 { + compatible = "ines,ptp-ctrl"; + reg = <0x60000000 0x80>; + phandle = <0x10>; + }; + + ethernet@80000000 { + ... + mdio { + ... + timestamper@1f { + compatible = "ines,ptp-port"; + ctrl-handle = <0x10>; + port-index = <0>; + reg = <0x1f>; + }; + }; + }; diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile index 01acbcb2c798..e286bb822295 100644 --- a/drivers/net/phy/Makefile +++ b/drivers/net/phy/Makefile @@ -61,6 +61,7 @@ obj-$(CONFIG_DP83848_PHY) += dp83848.o obj-$(CONFIG_DP83867_PHY) += dp83867.o obj-$(CONFIG_FIXED_PHY) += fixed_phy.o obj-$(CONFIG_ICPLUS_PHY) += icplus.o +obj-$(CONFIG_INES_PTP_TSTAMP) += ines_ptp.o obj-$(CONFIG_INTEL_XWAY_PHY) += intel-xway.o obj-$(CONFIG_LSI_ET1011C_PHY) += et1011c.o obj-$(CONFIG_LXT_PHY) += lxt.o diff --git a/drivers/net/phy/ines_ptp.c b/drivers/net/phy/ines_ptp.c new file mode 100644 index 000000000000..4f66459d4417 --- /dev/null +++ b/drivers/net/phy/ines_ptp.c @@ -0,0 +1,857 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Copyright (C) 2018 MOSER-BAER AG + */ +#define pr_fmt(fmt) "InES_PTP: " fmt + +#include <linux/ethtool.h> +#include <linux/export.h> +#include <linux/if_vlan.h> +#include <linux/mdio.h> +#include <linux/module.h> +#include <linux/net_tstamp.h> +#include <linux/of.h> +#include <linux/of_address.h> +#include <linux/of_irq.h> +#include <linux/platform_device.h> +#include <linux/ptp_classify.h> +#include <linux/ptp_clock_kernel.h> +#include <linux/stddef.h> + +MODULE_DESCRIPTION("Driver for the ZHAW InES PTP time stamping IP core"); +MODULE_AUTHOR("Richard Cochran <richardcochran@gmail.com>"); +MODULE_VERSION("1.0"); +MODULE_LICENSE("GPL"); + +/* GLOBAL register */ +#define MCAST_MAC_SELECT_SHIFT 2 +#define MCAST_MAC_SELECT_MASK 0x3 +#define IO_RESET BIT(1) +#define PTP_RESET BIT(0) + +/* VERSION register */ +#define IF_MAJOR_VER_SHIFT 12 +#define IF_MAJOR_VER_MASK 0xf +#define IF_MINOR_VER_SHIFT 8 +#define IF_MINOR_VER_MASK 0xf +#define FPGA_MAJOR_VER_SHIFT 4 +#define FPGA_MAJOR_VER_MASK 0xf +#define FPGA_MINOR_VER_SHIFT 0 +#define FPGA_MINOR_VER_MASK 0xf + +/* INT_STAT register */ +#define RX_INTR_STATUS_3 BIT(5) +#define RX_INTR_STATUS_2 BIT(4) +#define RX_INTR_STATUS_1 BIT(3) +#define TX_INTR_STATUS_3 BIT(2) +#define TX_INTR_STATUS_2 BIT(1) +#define TX_INTR_STATUS_1 BIT(0) + +/* INT_MSK register */ +#define RX_INTR_MASK_3 BIT(5) +#define RX_INTR_MASK_2 BIT(4) +#define RX_INTR_MASK_1 BIT(3) +#define TX_INTR_MASK_3 BIT(2) +#define TX_INTR_MASK_2 BIT(1) +#define TX_INTR_MASK_1 BIT(0) + +/* BUF_STAT register */ +#define RX_FIFO_NE_3 BIT(5) +#define RX_FIFO_NE_2 BIT(4) +#define RX_FIFO_NE_1 BIT(3) +#define TX_FIFO_NE_3 BIT(2) +#define TX_FIFO_NE_2 BIT(1) +#define TX_FIFO_NE_1 BIT(0) + +/* PORT_CONF register */ +#define CM_ONE_STEP BIT(6) +#define PHY_SPEED_SHIFT 4 +#define PHY_SPEED_MASK 0x3 +#define P2P_DELAY_WR_POS_SHIFT 2 +#define P2P_DELAY_WR_POS_MASK 0x3 +#define PTP_MODE_SHIFT 0 +#define PTP_MODE_MASK 0x3 + +/* TS_STAT_TX register */ +#define TS_ENABLE BIT(15) +#define DATA_READ_POS_SHIFT 8 +#define DATA_READ_POS_MASK 0x1f +#define DISCARDED_EVENTS_SHIFT 4 +#define DISCARDED_EVENTS_MASK 0xf + +#define INES_N_PORTS 3 +#define INES_REGISTER_SIZE 0x80 +#define INES_PORT_OFFSET 0x20 +#define INES_PORT_SIZE 0x20 +#define INES_FIFO_DEPTH 90 +#define INES_MAX_EVENTS 100 + +#define BC_PTP_V1 0 +#define BC_PTP_V2 1 +#define TC_E2E_PTP_V2 2 +#define TC_P2P_PTP_V2 3 + +#define OFF_PTP_CLOCK_ID 20 +#define OFF_PTP_PORT_NUM 28 + +#define PHY_SPEED_10 0 +#define PHY_SPEED_100 1 +#define PHY_SPEED_1000 2 + +#define PORT_CONF \ + ((PHY_SPEED_1000 << PHY_SPEED_SHIFT) | (BC_PTP_V2 << PTP_MODE_SHIFT)) + +#define ines_read32(s, r) __raw_readl(&s->regs->r) +#define ines_write32(s, v, r) __raw_writel(v, &s->regs->r) + +#define MESSAGE_TYPE_SYNC 1 +#define MESSAGE_TYPE_P_DELAY_REQ 2 +#define MESSAGE_TYPE_P_DELAY_RESP 3 +#define MESSAGE_TYPE_DELAY_REQ 4 + +#define SYNC 0x0 +#define DELAY_REQ 0x1 +#define PDELAY_REQ 0x2 +#define PDELAY_RESP 0x3 + +static LIST_HEAD(ines_clocks); +static DEFINE_MUTEX(ines_clocks_lock); + +struct ines_global_registers { + u32 id; + u32 test; + u32 global; + u32 version; + u32 test2; + u32 int_stat; + u32 int_msk; + u32 buf_stat; +}; + +struct ines_port_registers { + u32 port_conf; + u32 p_delay; + u32 ts_stat_tx; + u32 ts_stat_rx; + u32 ts_tx; + u32 ts_rx; +}; + +struct ines_timestamp { + struct list_head list; + unsigned long tmo; + u16 tag; + u64 sec; + u64 nsec; + u64 clkid; + u16 portnum; + u16 seqid; +}; + +struct ines_port { + struct ines_port_registers *regs; + struct ines_clock *clock; + bool rxts_enabled; + bool txts_enabled; + unsigned int index; + struct delayed_work ts_work; + /* lock protects event list and tx_skb */ + spinlock_t lock; + struct sk_buff *tx_skb; + struct list_head events; + struct list_head pool; + struct ines_timestamp pool_data[INES_MAX_EVENTS]; +}; + +struct ines_clock { + struct ines_port port[INES_N_PORTS]; + struct ines_global_registers *regs; + void __iomem *base; + struct device_node *node; + struct list_head list; +}; + +static bool ines_match(struct sk_buff *skb, unsigned int ptp_class, + struct ines_timestamp *ts); +static int ines_rxfifo_read(struct ines_port *port); +static u64 ines_rxts64(struct ines_port *port, unsigned int words); +static bool ines_timestamp_expired(struct ines_timestamp *ts); +static u64 ines_txts64(struct ines_port *port, unsigned int words); +static void ines_txtstamp_work(struct work_struct *work); +static bool is_sync_pdelay_resp(struct sk_buff *skb, int type); +static u8 tag_to_msgtype(u8 tag); + +static void ines_clock_cleanup(struct ines_clock *clock) +{ + struct ines_port *port; + int i; + + for (i = 0; i < INES_N_PORTS; i++) { + port = &clock->port[i]; + cancel_delayed_work_sync(&port->ts_work); + } +} + +static int ines_clock_init(struct ines_clock *clock, struct device_node *node, + void __iomem *addr) +{ + unsigned long port_addr; + struct ines_port *port; + int i, j; + + INIT_LIST_HEAD(&clock->list); + clock->node = node; + clock->base = addr; + clock->regs = clock->base; + + for (i = 0; i < INES_N_PORTS; i++) { + port = &clock->port[i]; + port_addr = (unsigned long) clock->base + + INES_PORT_OFFSET + i * INES_PORT_SIZE; + port->regs = (struct ines_port_registers *) port_addr; + port->clock = clock; + port->index = i; + INIT_DELAYED_WORK(&port->ts_work, ines_txtstamp_work); + spin_lock_init(&port->lock); + INIT_LIST_HEAD(&port->events); + INIT_LIST_HEAD(&port->pool); + for (j = 0; j < INES_MAX_EVENTS; j++) + list_add(&port->pool_data[j].list, &port->pool); + } + + ines_write32(clock, 0xBEEF, test); + ines_write32(clock, 0xBEEF, test2); + + pr_debug("ID 0x%x\n", ines_read32(clock, id)); + pr_debug("TEST 0x%x\n", ines_read32(clock, test)); + pr_debug("VERSION 0x%x\n", ines_read32(clock, version)); + pr_debug("TEST2 0x%x\n", ines_read32(clock, test2)); + + for (i = 0; i < INES_N_PORTS; i++) { + port = &clock->port[i]; + ines_write32(port, PORT_CONF, port_conf); + } + + return 0; +} + +static void ines_dump_ts(char *label, struct ines_timestamp *ts) +{ +#ifdef DEBUG + pr_err("%s timestamp, tag=0x%04hx t=%llu.%9llu c=0x%llx p=%hu s=%hu\n", + label, ts->tag, ts->sec, ts->nsec, + ts->clkid, ts->portnum, ts->seqid); +#endif +} + +static struct ines_port *ines_find_port(struct device_node *node, u32 index) +{ + struct ines_port *port = NULL; + struct ines_clock *clock; + struct list_head *this; + + if (index > INES_N_PORTS - 1) + return NULL; + + mutex_lock(&ines_clocks_lock); + list_for_each(this, &ines_clocks) { + clock = list_entry(this, struct ines_clock, list); + if (clock->node == node) { + port = &clock->port[index]; + break; + } + } + mutex_unlock(&ines_clocks_lock); + return port; +} + +static u64 ines_find_rxts(struct ines_port *port, struct sk_buff *skb, int type) +{ + struct list_head *this, *next; + struct ines_timestamp *ts; + unsigned long flags; + u64 ns = 0; + + if (type == PTP_CLASS_NONE) + return 0; + + spin_lock_irqsave(&port->lock, flags); + ines_rxfifo_read(port); + list_for_each_safe(this, next, &port->events) { + ts = list_entry(this, struct ines_timestamp, list); + if (ines_timestamp_expired(ts)) { + list_del_init(&ts->list); + list_add(&ts->list, &port->pool); + continue; + } + if (ines_match(skb, type, ts)) { + ns = ts->sec * 1000000000ULL + ts->nsec; + list_del_init(&ts->list); + list_add(&ts->list, &port->pool); + break; + } + } + spin_unlock_irqrestore(&port->lock, flags); + + return ns; +} + +static u64 ines_find_txts(struct ines_port *port, struct sk_buff *skb) +{ + unsigned int class = ptp_classify_raw(skb), i; + u32 data_rd_pos, buf_stat, mask, ts_stat_tx; + struct ines_timestamp ts; + unsigned long flags; + u64 ns = 0; + + mask = TX_FIFO_NE_1 << port->index; + + spin_lock_irqsave(&port->lock, flags); + + for (i = 0; i < INES_FIFO_DEPTH; i++) { + + buf_stat = ines_read32(port->clock, buf_stat); + if (!(buf_stat & mask)) { + pr_debug("Tx timestamp FIFO unexpectedly empty\n"); + break; + } + ts_stat_tx = ines_read32(port, ts_stat_tx); + data_rd_pos = (ts_stat_tx >> DATA_READ_POS_SHIFT) & + DATA_READ_POS_MASK; + if (data_rd_pos) { + pr_err("unexpected Tx read pos %u\n", data_rd_pos); + break; + } + + ts.tag = ines_read32(port, ts_tx); + ts.sec = ines_txts64(port, 3); + ts.nsec = ines_txts64(port, 2); + ts.clkid = ines_txts64(port, 4); + ts.portnum = ines_read32(port, ts_tx); + ts.seqid = ines_read32(port, ts_tx); + + ines_dump_ts("Tx", &ts); + + if (ines_match(skb, class, &ts)) { + ns = ts.sec * 1000000000ULL + ts.nsec; + break; + } + } + + spin_unlock_irqrestore(&port->lock, flags); + return ns; +} + +static int ines_hwtstamp(struct mdio_device *m, struct ifreq *ifr) +{ + u32 cm_one_step = 0, port_conf, ts_stat_rx, ts_stat_tx; + struct ines_port *port = dev_get_drvdata(&m->dev); + struct hwtstamp_config cfg; + unsigned long flags; + + if (copy_from_user(&cfg, ifr->ifr_data, sizeof(cfg))) + return -EFAULT; + + /* reserved for future extensions */ + if (cfg.flags) + return -EINVAL; + + switch (cfg.tx_type) { + case HWTSTAMP_TX_OFF: + ts_stat_tx = 0; + break; + case HWTSTAMP_TX_ON: + ts_stat_tx = TS_ENABLE; + break; + case HWTSTAMP_TX_ONESTEP_P2P: + ts_stat_tx = TS_ENABLE; + cm_one_step = CM_ONE_STEP; + break; + default: + return -ERANGE; + } + + switch (cfg.rx_filter) { + case HWTSTAMP_FILTER_NONE: + ts_stat_rx = 0; + break; + case HWTSTAMP_FILTER_ALL: + case HWTSTAMP_FILTER_PTP_V1_L4_EVENT: + case HWTSTAMP_FILTER_PTP_V1_L4_SYNC: + case HWTSTAMP_FILTER_PTP_V1_L4_DELAY_REQ: + return -ERANGE; + case HWTSTAMP_FILTER_PTP_V2_L4_EVENT: + case HWTSTAMP_FILTER_PTP_V2_L4_SYNC: + case HWTSTAMP_FILTER_PTP_V2_L4_DELAY_REQ: + case HWTSTAMP_FILTER_PTP_V2_L2_EVENT: + case HWTSTAMP_FILTER_PTP_V2_L2_SYNC: + case HWTSTAMP_FILTER_PTP_V2_L2_DELAY_REQ: + case HWTSTAMP_FILTER_PTP_V2_EVENT: + case HWTSTAMP_FILTER_PTP_V2_SYNC: + case HWTSTAMP_FILTER_PTP_V2_DELAY_REQ: + ts_stat_rx = TS_ENABLE; + cfg.rx_filter = HWTSTAMP_FILTER_PTP_V2_EVENT; + break; + default: + return -ERANGE; + } + + spin_lock_irqsave(&port->lock, flags); + + port_conf = ines_read32(port, port_conf); + port_conf &= ~CM_ONE_STEP; + port_conf |= cm_one_step; + + ines_write32(port, port_conf, port_conf); + ines_write32(port, ts_stat_rx, ts_stat_rx); + ines_write32(port, ts_stat_tx, ts_stat_tx); + + port->rxts_enabled = ts_stat_rx == TS_ENABLE ? true : false; + port->txts_enabled = ts_stat_tx == TS_ENABLE ? true : false; + + spin_unlock_irqrestore(&port->lock, flags); + + return copy_to_user(ifr->ifr_data, &cfg, sizeof(cfg)) ? -EFAULT : 0; +} + +static bool ines_match(struct sk_buff *skb, unsigned int ptp_class, + struct ines_timestamp *ts) +{ + u8 *msgtype, *data = skb_mac_header(skb); + unsigned int offset = 0; + u16 *portn, *seqid; + u64 *clkid; + + if (unlikely(ptp_class & PTP_CLASS_V1)) + return false; + + if (ptp_class & PTP_CLASS_VLAN) + offset += VLAN_HLEN; + + switch (ptp_class & PTP_CLASS_PMASK) { + case PTP_CLASS_IPV4: + offset += ETH_HLEN + IPV4_HLEN(data + offset) + UDP_HLEN; + break; + case PTP_CLASS_IPV6: + offset += ETH_HLEN + IP6_HLEN + UDP_HLEN; + break; + case PTP_CLASS_L2: + offset += ETH_HLEN; + break; + default: + return false; + } + + if (skb->len + ETH_HLEN < offset + OFF_PTP_SEQUENCE_ID + sizeof(*seqid)) + return false; + + msgtype = data + offset; + clkid = (u64 *)(data + offset + OFF_PTP_CLOCK_ID); + portn = (u16 *)(data + offset + OFF_PTP_PORT_NUM); + seqid = (u16 *)(data + offset + OFF_PTP_SEQUENCE_ID); + + if (tag_to_msgtype(ts->tag & 0x7) != (*msgtype & 0xf)) { + pr_debug("msgtype mismatch ts %hhu != skb %hhu\n", + tag_to_msgtype(ts->tag & 0x7), *msgtype & 0xf); + return false; + } + if (cpu_to_be64(ts->clkid) != *clkid) { + pr_debug("clkid mismatch ts %llx != skb %llx\n", + cpu_to_be64(ts->clkid), *clkid); + return false; + } + if (ts->portnum != ntohs(*portn)) { + pr_debug("portn mismatch ts %hu != skb %hu\n", + ts->portnum, ntohs(*portn)); + return false; + } + if (ts->seqid != ntohs(*seqid)) { + pr_debug("seqid mismatch ts %hu != skb %hu\n", + ts->seqid, ntohs(*seqid)); + return false; + } + + return true; +} + +static bool ines_rxtstamp(struct mdio_device *m, struct sk_buff *skb, int type) +{ + struct ines_port *port = dev_get_drvdata(&m->dev); + struct skb_shared_hwtstamps *ssh; + u64 ns; + + if (!port->rxts_enabled) + return false; + + ns = ines_find_rxts(port, skb, type); + if (!ns) + return false; + + ssh = skb_hwtstamps(skb); + ssh->hwtstamp = ns_to_ktime(ns); + netif_rx(skb); + + return true; +} + +static int ines_rxfifo_read(struct ines_port *port) +{ + u32 data_rd_pos, buf_stat, mask, ts_stat_rx; + struct ines_timestamp *ts; + unsigned int i; + + mask = RX_FIFO_NE_1 << port->index; + + for (i = 0; i < INES_FIFO_DEPTH; i++) { + if (list_empty(&port->pool)) { + pr_err("event pool is empty\n"); + return -1; + } + buf_stat = ines_read32(port->clock, buf_stat); + if (!(buf_stat & mask)) + break; + + ts_stat_rx = ines_read32(port, ts_stat_rx); + data_rd_pos = (ts_stat_rx >> DATA_READ_POS_SHIFT) & + DATA_READ_POS_MASK; + if (data_rd_pos) { + pr_err("unexpected Rx read pos %u\n", data_rd_pos); + break; + } + + ts = list_first_entry(&port->pool, struct ines_timestamp, list); + ts->tmo = jiffies + HZ; + ts->tag = ines_read32(port, ts_rx); + ts->sec = ines_rxts64(port, 3); + ts->nsec = ines_rxts64(port, 2); + ts->clkid = ines_rxts64(port, 4); + ts->portnum = ines_read32(port, ts_rx); + ts->seqid = ines_read32(port, ts_rx); + + ines_dump_ts("Rx", ts); + + list_del_init(&ts->list); + list_add_tail(&ts->list, &port->events); + } + + return 0; +} + +static u64 ines_rxts64(struct ines_port *port, unsigned int words) +{ + unsigned int i; + u64 result; + u16 word; + + word = ines_read32(port, ts_rx); + result = word; + words--; + for (i = 0; i < words; i++) { + word = ines_read32(port, ts_rx); + result <<= 16; + result |= word; + } + return result; +} + +static bool ines_timestamp_expired(struct ines_timestamp *ts) +{ + return time_after(jiffies, ts->tmo); +} + +static int ines_ts_info(struct mdio_device *m, struct ethtool_ts_info *info) +{ + info->so_timestamping = + SOF_TIMESTAMPING_TX_HARDWARE | + SOF_TIMESTAMPING_TX_SOFTWARE | + SOF_TIMESTAMPING_RX_HARDWARE | + SOF_TIMESTAMPING_RX_SOFTWARE | + SOF_TIMESTAMPING_SOFTWARE | + SOF_TIMESTAMPING_RAW_HARDWARE; + + info->phc_index = -1; + + info->tx_types = + (1 << HWTSTAMP_TX_OFF) | + (1 << HWTSTAMP_TX_ON) | + (1 << HWTSTAMP_TX_ONESTEP_P2P); + + info->rx_filters = + (1 << HWTSTAMP_FILTER_NONE) | + (1 << HWTSTAMP_FILTER_PTP_V2_EVENT); + + return 0; +} + +static u64 ines_txts64(struct ines_port *port, unsigned int words) +{ + unsigned int i; + u64 result; + u16 word; + + word = ines_read32(port, ts_tx); + result = word; + words--; + for (i = 0; i < words; i++) { + word = ines_read32(port, ts_tx); + result <<= 16; + result |= word; + } + return result; +} + +static bool ines_txts_onestep(struct ines_port *port, struct sk_buff *skb, int type) +{ + unsigned long flags; + u32 port_conf; + + spin_lock_irqsave(&port->lock, flags); + port_conf = ines_read32(port, port_conf); + spin_unlock_irqrestore(&port->lock, flags); + + if (port_conf & CM_ONE_STEP) + return is_sync_pdelay_resp(skb, type); + + return false; +} + +static void ines_txtstamp(struct mdio_device *m, struct sk_buff *skb, int type) +{ + struct ines_port *port = dev_get_drvdata(&m->dev); + struct sk_buff *old_skb; + unsigned long flags; + + if (!port->txts_enabled || ines_txts_onestep(port, skb, type)) { + kfree_skb(skb); + return; + } + + spin_lock_irqsave(&port->lock, flags); + + if (port->tx_skb) + old_skb = port->tx_skb; + + port->tx_skb = skb; + + spin_unlock_irqrestore(&port->lock, flags); + + if (old_skb) + kfree_skb(old_skb); + + schedule_delayed_work(&port->ts_work, 1); +} + +static void ines_txtstamp_work(struct work_struct *work) +{ + struct ines_port *port = + container_of(work, struct ines_port, ts_work.work); + struct skb_shared_hwtstamps ssh; + struct sk_buff *skb; + unsigned long flags; + u64 ns; + + spin_lock_irqsave(&port->lock, flags); + skb = port->tx_skb; + port->tx_skb = NULL; + spin_unlock_irqrestore(&port->lock, flags); + + ns = ines_find_txts(port, skb); + if (!ns) { + kfree_skb(skb); + return; + } + ssh.hwtstamp = ns_to_ktime(ns); + skb_complete_tx_timestamp(skb, &ssh); +} + +static bool is_sync_pdelay_resp(struct sk_buff *skb, int type) +{ + u8 *data = skb->data, *msgtype; + unsigned int offset = 0; + + if (type & PTP_CLASS_VLAN) + offset += VLAN_HLEN; + + switch (type & PTP_CLASS_PMASK) { + case PTP_CLASS_IPV4: + offset += ETH_HLEN + IPV4_HLEN(data + offset) + UDP_HLEN; + break; + case PTP_CLASS_IPV6: + offset += ETH_HLEN + IP6_HLEN + UDP_HLEN; + break; + case PTP_CLASS_L2: + offset += ETH_HLEN; + break; + default: + return 0; + } + + if (type & PTP_CLASS_V1) + offset += OFF_PTP_CONTROL; + + if (skb->len < offset + 1) + return 0; + + msgtype = data + offset; + + switch ((*msgtype & 0xf)) { + case SYNC: + case PDELAY_RESP: + return true; + default: + return false; + } +} + +static u8 tag_to_msgtype(u8 tag) +{ + switch (tag) { + case MESSAGE_TYPE_SYNC: + return SYNC; + case MESSAGE_TYPE_P_DELAY_REQ: + return PDELAY_REQ; + case MESSAGE_TYPE_P_DELAY_RESP: + return PDELAY_RESP; + case MESSAGE_TYPE_DELAY_REQ: + return DELAY_REQ; + } + return 0xf; +} + +static int ines_ptp_ctrl_probe(struct platform_device *pld) +{ + struct ines_clock *clock; + struct resource *res; + void __iomem *addr; + int err = 0; + + res = platform_get_resource(pld, IORESOURCE_MEM, 0); + if (!res) { + dev_err(&pld->dev, "missing memory resource\n"); + return -EINVAL; + } + addr = devm_ioremap_resource(&pld->dev, res); + if (IS_ERR(addr)) { + err = PTR_ERR(addr); + goto out; + } + clock = kzalloc(sizeof(*clock), GFP_KERNEL); + if (!clock) { + err = -ENOMEM; + goto out; + } + if (ines_clock_init(clock, pld->dev.of_node, addr)) { + kfree(clock); + err = -ENOMEM; + goto out; + } + mutex_lock(&ines_clocks_lock); + list_add_tail(&ines_clocks, &clock->list); + mutex_unlock(&ines_clocks_lock); + + dev_set_drvdata(&pld->dev, clock); +out: + return err; +} + +static int ines_ptp_ctrl_remove(struct platform_device *pld) +{ + struct ines_clock *clock = dev_get_drvdata(&pld->dev); + + mutex_lock(&ines_clocks_lock); + list_del(&clock->list); + mutex_unlock(&ines_clocks_lock); + ines_clock_cleanup(clock); + kfree(clock); + return 0; +} + +static int ines_ptp_port_probe(struct mdio_device *mdiodev) +{ + struct device_node *node; + struct ines_port *port; + int err = 0; + u32 index; + + if (of_property_read_u32(mdiodev->dev.of_node, "port-index", &index)) { + dev_err(&mdiodev->dev, "missing port-index\n"); + return -EINVAL; + } + node = of_parse_phandle(mdiodev->dev.of_node, "ctrl-handle", 0); + if (IS_ERR(node)) { + dev_err(&mdiodev->dev, "missing ctrl-handle\n"); + return PTR_ERR(node); + } + port = ines_find_port(node, index); + if (!port) { + dev_err(&mdiodev->dev, "missing port\n"); + err = -ENODEV; + goto out; + } + mdiodev->ts_info = ines_ts_info; + mdiodev->hwtstamp = ines_hwtstamp; + mdiodev->rxtstamp = ines_rxtstamp; + mdiodev->txtstamp = ines_txtstamp; + dev_set_drvdata(&mdiodev->dev, port); +out: + of_node_put(node); + return err; +} + +static void ines_ptp_port_remove(struct mdio_device *mdiodev) +{ +} + +static const struct of_device_id ines_ptp_ctrl_of_match[] = { + { .compatible = "ines,ptp-ctrl" }, + { } +}; + +MODULE_DEVICE_TABLE(of, ines_ptp_ctrl_of_match); + +static struct platform_driver ines_ptp_ctrl_driver = { + .probe = ines_ptp_ctrl_probe, + .remove = ines_ptp_ctrl_remove, + .driver = { + .name = "ines_ptp_ctrl", + .of_match_table = of_match_ptr(ines_ptp_ctrl_of_match), + }, +}; + +static const struct of_device_id ines_ptp_port_of_match[] = { + { .compatible = "ines,ptp-port" }, + { } +}; + +static struct mdio_driver ines_ptp_port_driver = { + .probe = ines_ptp_port_probe, + .remove = ines_ptp_port_remove, + .mdiodrv.driver = { + .name = "ines_ptp_port", + .of_match_table = ines_ptp_port_of_match, + }, +}; + +static int __init ines_ptp_init(void) +{ + int err; + + err = platform_driver_register(&ines_ptp_ctrl_driver); + if (err) + return err; + + err = mdio_driver_register(&ines_ptp_port_driver); + if (err) + platform_driver_unregister(&ines_ptp_ctrl_driver); + + return err; +} + +static void __exit ines_ptp_cleanup(void) +{ + mdio_driver_unregister(&ines_ptp_port_driver); + platform_driver_unregister(&ines_ptp_ctrl_driver); +} + +module_init(ines_ptp_init); +module_exit(ines_ptp_cleanup); diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig index a21ad10d613c..10ef161dd2a1 100644 --- a/drivers/ptp/Kconfig +++ b/drivers/ptp/Kconfig @@ -88,6 +88,16 @@ config DP83640_PHY In order for this to work, your MAC driver must also implement the skb_tx_timestamp() function. +config INES_PTP_TSTAMP + tristate "ZHAW InES PTP time stamping IP core" + depends on NETWORK_PHY_TIMESTAMPING + depends on PHYLIB + depends on PTP_1588_CLOCK + ---help--- + This driver adds support for using the ZHAW InES 1588 IP + core. This clock is only useful if the MII bus of your MAC + is wired up to the core. + config PTP_1588_CLOCK_PCH tristate "Intel PCH EG20T as PTP clock" depends on X86_32 || COMPILE_TEST
The InES at the ZHAW offers a PTP time stamping IP core. The FPGA logic recognizes and time stamps PTP frames on the MII bus. This patch adds a driver for the core along with a device tree binding to allow hooking the driver to MAC devices. Signed-off-by: Richard Cochran <richardcochran@gmail.com> --- Documentation/devicetree/bindings/net/ines-ptp.txt | 42 + drivers/net/phy/Makefile | 1 + drivers/net/phy/ines_ptp.c | 857 +++++++++++++++++++++ drivers/ptp/Kconfig | 10 + 4 files changed, 910 insertions(+) create mode 100644 Documentation/devicetree/bindings/net/ines-ptp.txt create mode 100644 drivers/net/phy/ines_ptp.c