diff mbox series

[net-next,RFC,V1,5/5] net: mdio: Add a driver for InES time stamping IP core.

Message ID e9def53dd0f751d7f5e3df47e604d7ccb6020159.1521656774.git.richardcochran@gmail.com
State Changes Requested, archived
Headers show
Series Peer to Peer One-Step time stamping | expand

Commit Message

Richard Cochran March 21, 2018, 6:58 p.m. UTC
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

Comments

Andrew Lunn March 21, 2018, 7:33 p.m. UTC | #1
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
Richard Cochran March 21, 2018, 9:36 p.m. UTC | #2
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
Andrew Lunn March 21, 2018, 9:44 p.m. UTC | #3
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
Richard Cochran March 21, 2018, 9:57 p.m. UTC | #4
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
Andrew Lunn March 21, 2018, 10:16 p.m. UTC | #5
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 = <&timestamper 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
Richard Cochran March 21, 2018, 10:47 p.m. UTC | #6
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 = <&timestamper 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
Andrew Lunn March 21, 2018, 11:50 p.m. UTC | #7
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 = <&timestamper 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
Andrew Lunn March 22, 2018, 12:43 a.m. UTC | #8
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 = <&timestamper 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
Richard Cochran March 22, 2018, 1:57 a.m. UTC | #9
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
Richard Cochran March 24, 2018, 5:12 p.m. UTC | #10
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
Andrew Lunn March 24, 2018, 6:48 p.m. UTC | #11
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
Richard Cochran March 25, 2018, 4:51 a.m. UTC | #12
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
Andrew Lunn March 25, 2018, 3:59 p.m. UTC | #13
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
Richard Cochran March 25, 2018, 10:10 p.m. UTC | #14
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
Richard Cochran March 25, 2018, 10:14 p.m. UTC | #15
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
Florian Fainelli March 25, 2018, 11:01 p.m. UTC | #16
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
>
Andrew Lunn March 25, 2018, 11:01 p.m. UTC | #17
> > 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
Andrew Lunn March 25, 2018, 11:06 p.m. UTC | #18
> > 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
Richard Cochran April 3, 2018, 3:55 a.m. UTC | #19
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
Richard Cochran April 3, 2018, 4:27 a.m. UTC | #20
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
Andrew Lunn April 3, 2018, 1:13 p.m. UTC | #21
> 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
Richard Cochran April 3, 2018, 3:02 p.m. UTC | #22
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 mbox series

Patch

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