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

[Richard Cochran]

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

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

[Andrew Lunn]

> 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

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

[Richard Cochran]

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

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

[Richard Cochran]

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

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

[Andrew Lunn]

> > 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

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

[Florian Fainelli]

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
> 

-- 
Florian

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

[Andrew Lunn]

> > 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

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

[Richard Cochran]

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

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

[Richard Cochran]

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

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

[Andrew Lunn]

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

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

[Richard Cochran]

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

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

[Andrew Lunn]

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

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

[Richard Cochran]

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

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

[Richard Cochran]

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

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

[Andrew Lunn]

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 = < 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

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

[Andrew Lunn]

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 = < 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

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

[Richard Cochran]

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 = < 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

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

[Andrew Lunn]

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@6000 {
compatible = "ines,ptp-ctrl";
reg = <0x6000 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 = < 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

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

[Richard Cochran]

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

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

[Andrew Lunn]

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

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

[Richard Cochran]

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

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

[Andrew Lunn]

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

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

[Richard Cochran]

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 
---
 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

diff --git a/Documentation/devicetree/bindings/net/ines-ptp.txt 
b/Documentation/devicetree/bindings/net/ines-ptp.txt
new file mode 100644
index ..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@6000 {
+   compatible = "ines,ptp-ctrl";
+   reg = <0x6000 0x80>;
+   phandle = <0x10>;
+   };
+
+   ethernet@8000 {
+   ...
+   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 ..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 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+#include 
+
+MODULE_DESCRIPTION("Driver for the ZHAW InES PTP time stamping IP core");
+MODULE_AUTHOR("Richard Cochran ");
+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_MASK0xf
+#define FPGA_MINOR_VER_SHIFT   0
+#define FPGA_MINOR_VER_MASK0xf
+
+/* 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_STEPBIT(6)
+#define