[PATCH] net:cpsw: Pass unhandled ioctl's on to generic phy ioctl

[PATCH] net:cpsw: Pass unhandled ioctl's on to generic phy ioctl

[Stefan Sørensen]

This patch allows the use of a generic timestamping phy connected
to the cpsw if CPTS support is not enabled.

Signed-off-by: Stefan Sørensen <stefan.sorensen@spectralink.com>
---
 drivers/net/ethernet/ti/cpsw.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
index bde63e3..a03cfb3 100644
--- a/drivers/net/ethernet/ti/cpsw.c
+++ b/drivers/net/ethernet/ti/cpsw.c
@@ -1486,12 +1486,12 @@ static int cpsw_ndo_ioctl(struct net_device *dev, struct ifreq *req, int cmd)
 #endif
 	case SIOCGMIIPHY:
 		data->phy_id = priv->slaves[slave_no].phy->addr;
-		break;
-	default:
-		return -ENOTSUPP;
+		return 0;
 	}
 
-	return 0;
+	if (!priv->slaves[slave_no].phy)
+		return -EINVAL;
+	return phy_mii_ioctl(priv->slaves[slave_no].phy, req, cmd);
 }
 
 static void cpsw_ndo_tx_timeout(struct net_device *ndev)
-- 
1.8.5.3

Re: [PATCH] net:cpsw: Pass unhandled ioctl's on to generic phy ioctl

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1436 bytes --]

On Tue, 2014-02-04 at 08:50 +0100, Stefan Sørensen wrote:
> This patch allows the use of a generic timestamping phy connected
> to the cpsw if CPTS support is not enabled.

What if CPTS support is enabled in the driver, but this particular
machine doesn't have it and uses a timestamping PHY instead?

> Signed-off-by: Stefan Sørensen <stefan.sorensen@spectralink.com>
> ---
>  drivers/net/ethernet/ti/cpsw.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index bde63e3..a03cfb3 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -1486,12 +1486,12 @@ static int cpsw_ndo_ioctl(struct net_device *dev, struct ifreq *req, int cmd)
>  #endif
>  	case SIOCGMIIPHY:
>  		data->phy_id = priv->slaves[slave_no].phy->addr;

It looks like this existing code is broken, as the phy pointer can be
NULL!

> -		break;
> -	default:
> -		return -ENOTSUPP;
> +		return 0;
>  	}
>  
> -	return 0;
> +	if (!priv->slaves[slave_no].phy)
> +		return -EINVAL;
> +	return phy_mii_ioctl(priv->slaves[slave_no].phy, req, cmd);

This presumably also enables SIOC{G,S}MIIREG, but you didn't mention
that.

Ben.

>  }
>  
>  static void cpsw_ndo_tx_timeout(struct net_device *ndev)

-- 
Ben Hutchings
One of the nice things about standards is that there are so many of them.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH] net:cpsw: Pass unhandled ioctl's on to generic phy ioctl

[Sørensen, Stefan]

On Tue, 2014-02-04 at 10:50 +0000, Ben Hutchings wrote:
> > This patch allows the use of a generic timestamping phy connected
> > to the cpsw if CPTS support is not enabled.
> 
> What if CPTS support is enabled in the driver, but this particular
> machine doesn't have it and uses a timestamping PHY instead?

That would not work, the CPTS will grab the SIOC{G,S}HWTSTAMP. I'm not
sure how that could be configured at runtime, other than a private
ethtool flag.

Also it seem as the situation with a timestamping MAC and a timestamping
PHY could deliver bogus ethtool timestamping info, as it will come from
the PHY if available, but the timestamping will be handled by the MAC.

> >  	case SIOCGMIIPHY:
> >  		data->phy_id = priv->slaves[slave_no].phy->addr;
> 
> It looks like this existing code is broken, as the phy pointer can be
> NULL!

Right, but that code can go away since SIOCGMIIPHY is also handled by
phy_mii_ioctl. I will delete that case in the next version.

> > +	return phy_mii_ioctl(priv->slaves[slave_no].phy, req, cmd);
> 
> This presumably also enables SIOC{G,S}MIIREG, but you didn't mention
> that.

Yes, I will add that to the commit log in the next version.

Stefan

Re: [PATCH] net:cpsw: Pass unhandled ioctl's on to generic phy ioctl

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1210 bytes --]

On Tue, 2014-02-04 at 15:08 +0000, Sørensen, Stefan wrote:
> On Tue, 2014-02-04 at 10:50 +0000, Ben Hutchings wrote:
> > > This patch allows the use of a generic timestamping phy connected
> > > to the cpsw if CPTS support is not enabled.
> > 
> > What if CPTS support is enabled in the driver, but this particular
> > machine doesn't have it and uses a timestamping PHY instead?
> 
> That would not work, the CPTS will grab the SIOC{G,S}HWTSTAMP. I'm not
> sure how that could be configured at runtime, other than a private
> ethtool flag.

Do all versions of CPSW include hardware timestamping?  Because the
condition at the top of cpsw_htstamp_ioctl() suggested to me that there
are some that don't.

> Also it seem as the situation with a timestamping MAC and a timestamping
> PHY could deliver bogus ethtool timestamping info, as it will come from
> the PHY if available, but the timestamping will be handled by the MAC.
[...]

Right.  If all versions of CPSW include hardware timestamping then
bother with PHY timestamping at all?  And why make CONFIG_TI_CPTS
configurable?

Ben.

-- 
Ben Hutchings
One of the nice things about standards is that there are so many of them.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH] net:cpsw: Pass unhandled ioctl's on to generic phy ioctl

[Richard Cochran]

On Tue, Feb 04, 2014 at 09:51:59PM +0000, Ben Hutchings wrote:
> 
> Right.  If all versions of CPSW include hardware timestamping then
> bother with PHY timestamping at all?  And why make CONFIG_TI_CPTS
> configurable?

On the one hand, PHY time stamping is more accurate and offers
synchronization performance that is measurably better than MAC time
stamping. On the other hand, when using a MAC the CPU usually has much
more direct access to the clock (for example, direct register access
or PCIe, versus MDIO).

I once worked on a project in which it was planned to have both kinds
of hardware in the design, in order to keep our options open in the
face of fluid requirements. So I think you can expect to see such
combinations in the wild, especially in the embedded area.

We cannot reasonably support both types in the kernel at the same
time, and so it makes sense to have compile time options in MAC
drivers to disable time stamping.

Thanks,
Richard

Re: [PATCH] net:cpsw: Pass unhandled ioctl's on to generic phy ioctl

[Sørensen, Stefan]

On Tue, 2014-02-04 at 21:51 +0000, Ben Hutchings wrote:
> Right.  If all versions of CPSW include hardware timestamping then
> bother with PHY timestamping at all?  And why make CONFIG_TI_CPTS
> configurable?

The CPSW only supports timestamping which is only one part of the PTP
functionality. Some PHYs (like the dp83630 that we use with the CPSW)
also has external event generation and detection.

Stefan

Re: [PATCH] net:cpsw: Pass unhandled ioctl's on to generic phy ioctl

[Mugunthan V N]

On Wednesday 05 February 2014 12:58 PM, Sørensen, Stefan wrote:
> The CPSW only supports timestamping which is only one part of the PTP
> functionality. Some PHYs (like the dp83630 that we use with the CPSW)
> also has external event generation and detection.
CPSW also has External event detection upto 4 events and newer SoCs upto
8 events, currently no boards came with this pins pinned out, so it is
not supported as of now. If we have a board design with this we can
support external event generation.

Regards
Mugunthan V N

Re: [PATCH] net:cpsw: Pass unhandled ioctl's on to generic phy ioctl

[Ben Hutchings]

[-- Attachment #1: Type: text/plain, Size: 1574 bytes --]

On Wed, 2014-02-05 at 08:12 +0100, Richard Cochran wrote:
> On Tue, Feb 04, 2014 at 09:51:59PM +0000, Ben Hutchings wrote:
> > 
> > Right.  If all versions of CPSW include hardware timestamping then
> > bother with PHY timestamping at all?  And why make CONFIG_TI_CPTS
> > configurable?
> 
> On the one hand, PHY time stamping is more accurate and offers
> synchronization performance that is measurably better than MAC time
> stamping.

I suppose that depends on how much jitter there is in the PHY?

With SFC boards and 10GBASE-SR modules or DA cables, I would see
synchronisation to within about 10 nanoseconds using MAC timestamps.

> On the other hand, when using a MAC the CPU usually has much
> more direct access to the clock (for example, direct register access
> or PCIe, versus MDIO).
> 
> I once worked on a project in which it was planned to have both kinds
> of hardware in the design, in order to keep our options open in the
> face of fluid requirements. So I think you can expect to see such
> combinations in the wild, especially in the embedded area.
> 
> We cannot reasonably support both types in the kernel at the same
> time, and so it makes sense to have compile time options in MAC
> drivers to disable time stamping.

I would think that it should be a *run-time* option, as I don't like
forcing people to rebuild their kernel.  But perhaps this is esoteric
enough that the people who care will be doing that anyway.

Ben.

-- 
Ben Hutchings
Once a job is fouled up, anything done to improve it makes it worse.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH] net:cpsw: Pass unhandled ioctl's on to generic phy ioctl

[Christian Riesch]

On 2014-02-05 09:18, Mugunthan V N wrote:
> On Wednesday 05 February 2014 12:58 PM, Sørensen, Stefan wrote:
>> The CPSW only supports timestamping which is only one part of the PTP
>> functionality. Some PHYs (like the dp83630 that we use with the CPSW)
>> also has external event generation and detection.
> CPSW also has External event detection upto 4 events and newer SoCs upto
> 8 events, currently no boards came with this pins pinned out, so it is
> not supported as of now. If we have a board design with this we can
> support external event generation.

But for external event generation it would be useful to adjust the clock 
frequency of the PHC. IIRC this is broken on some silicon like the 
AM335x, right?

Christian

Re: [PATCH] net:cpsw: Pass unhandled ioctl's on to generic phy ioctl

[Christian Riesch]

On 2014-02-05 11:23, Ben Hutchings wrote:
> On Wed, 2014-02-05 at 08:12 +0100, Richard Cochran wrote:
>> On Tue, Feb 04, 2014 at 09:51:59PM +0000, Ben Hutchings wrote:
>>>
>>> Right.  If all versions of CPSW include hardware timestamping then
>>> bother with PHY timestamping at all?  And why make CONFIG_TI_CPTS
>>> configurable?
>>
>> On the one hand, PHY time stamping is more accurate and offers
>> synchronization performance that is measurably better than MAC time
>> stamping.
>
> I suppose that depends on how much jitter there is in the PHY?

Not only jitter, asymmetries of the PHY delays must be well known, too, 
and these data are not always available from datasheets.

If someone is interested in measurement results: We did a few 
experiments last year for 100 Mbit Ethernet and compared MAC 
timestamping of a P2020 with the PHY timestamping in the DP83640 (see 
Table III in [1]).

Christian


[1] C. Riesch, C. Marinescu, and M. Rudigier, "Measurement of egress and 
ingress delays of PTP clocks", in 2013 International IEEE Symposium on 
Precision Clock Synchronization for Measurement Control and 
Communication (ISPCS), Lemgo, Germany, Sep. 22–27, 2013, pp. 113–118.
http://www.riesch.at/christian/ISPCS2013_Measurement_of_egress_and_ingress_delays_of_PTP_clocks.pdf

Re: [PATCH] net:cpsw: Pass unhandled ioctl's on to generic phy ioctl

[Richard Cochran]

On Wed, Feb 05, 2014 at 10:23:40AM +0000, Ben Hutchings wrote:
> 
> I would think that it should be a *run-time* option, as I don't like
> forcing people to rebuild their kernel.  But perhaps this is esoteric
> enough that the people who care will be doing that anyway.

For PHY time stamping, you need CONFIG_NETWORK_PHY_TIMESTAMPING which
adds extra checks into the hot path. I don't think anyone would enable
this by default. People using PHY time stamping are definitely
recompiling for their special use case.

Thanks,
Richard

Re: [PATCH] net:cpsw: Pass unhandled ioctl's on to generic phy ioctl

[Mugunthan V N]

On Wednesday 05 February 2014 04:19 PM, Christian Riesch wrote:
> On 2014-02-05 09:18, Mugunthan V N wrote:
>> On Wednesday 05 February 2014 12:58 PM, Sørensen, Stefan wrote:
>>> The CPSW only supports timestamping which is only one part of the PTP
>>> functionality. Some PHYs (like the dp83630 that we use with the CPSW)
>>> also has external event generation and detection.
>> CPSW also has External event detection upto 4 events and newer SoCs upto
>> 8 events, currently no boards came with this pins pinned out, so it is
>> not supported as of now. If we have a board design with this we can
>> support external event generation.
>
> But for external event generation it would be useful to adjust the
> clock frequency of the PHC. IIRC this is broken on some silicon like
> the AM335x, right?
>

Yes, I agree but it is fixed in future SoCs like DRA7xx and AM43xx

Regards
Mugunthan V N