| Message ID | 1391500242-10554-1-git-send-email-stefan.sorensen@spectralink.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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)
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.
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/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)
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(-)