Hello everyone,
This series follows (albeit with a lot of delay) the work that was
initiated when USGMII support was added a while ago [1], through the QUSGMII
variant. One of the differences between the QUSGMII mode and QSGMII is
the possibility to pass so-called "Extensions" within the preamble. So
far, the only extension documented in the USGMII standard is to pass
a 32-bits timestamp through the preamble, but the standard mentions
other possible usages in the future, such as a MACSec sectag for
example.
This series aims at adding support for such extensions and proposes a
phylib API to manipulate these.
It includes an example using the lan8814 PHY driver and the LAN966x MAC
driver, where both have to agree on using this mode to convey the
nanoseconds part of a PTP timestamp (the seconds part is still retreived
through MDIO accesses).
Thanks,
Maxime
[1] : https://lore.kernel.org/netdev/[email protected]/
Maxime Chevallier (3):
net: phy: Add support for inband extensions
net: lan966x: Allow using PCH extension for PTP
net: phy: micrel: Add QUSGMII support and PCH extension
Documentation/networking/phy.rst | 70 ++++++++++++++
.../ethernet/microchip/lan966x/lan966x_main.h | 1 +
.../ethernet/microchip/lan966x/lan966x_port.c | 12 +++
.../ethernet/microchip/lan966x/lan966x_ptp.c | 94 +++++++++++++++++--
.../ethernet/microchip/lan966x/lan966x_regs.h | 64 +++++++++++++
drivers/net/phy/micrel.c | 84 ++++++++++++++++-
drivers/net/phy/phy.c | 86 +++++++++++++++++
include/linux/phy.h | 28 ++++++
8 files changed, 425 insertions(+), 14 deletions(-)
--
2.43.0
This commit adds the logic to use the PCH mode for timestamp passing in
ingress traffic for PTP. The tricky part is that we need to configure
both the PHY and the MAC, since the MAC will extract the timetsamp
reported by the PHY from the preamble and set it into the SKB.
Note that with the PCH mode, we only get the RX timestamp that way, the
TX timestamps are still reported OOB by the PHY.
Note that only the nanoseconds part is extracted from the PCH extension,
since there's not enough room in the 4 bytes extension to pass a full
80bits timestamp. The seconds part of the timestamp still needs to get
retrieved from the PHY using an out-of-band mechanism.
Signed-off-by: Maxime Chevallier <[email protected]>
---
.../ethernet/microchip/lan966x/lan966x_main.h | 1 +
.../ethernet/microchip/lan966x/lan966x_port.c | 12 +++
.../ethernet/microchip/lan966x/lan966x_ptp.c | 94 +++++++++++++++++--
.../ethernet/microchip/lan966x/lan966x_regs.h | 64 +++++++++++++
4 files changed, 161 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
index caa9e0533c96..0aca85be02ac 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_main.h
@@ -302,6 +302,7 @@ struct lan966x_phc {
struct kernel_hwtstamp_config hwtstamp_config;
struct lan966x *lan966x;
u8 index;
+ bool pch;
};
struct lan966x_skb_cb {
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_port.c b/drivers/net/ethernet/microchip/lan966x/lan966x_port.c
index 2e83bbb9477e..cf432d797ce6 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_port.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_port.c
@@ -363,6 +363,18 @@ int lan966x_port_pcs_set(struct lan966x_port *port,
DEV_PCS1G_MODE_CFG_SAVE_PREAMBLE_ENA,
lan966x, DEV_PCS1G_MODE_CFG(port->chip_port));
+ if (full_preamble) {
+ lan_rmw(DEV_ENABLE_CONFIG_MM_TX_ENA_SET(1) |
+ DEV_ENABLE_CONFIG_MM_RX_ENA_SET(1),
+ DEV_ENABLE_CONFIG_MM_TX_ENA |
+ DEV_ENABLE_CONFIG_MM_RX_ENA,
+ lan966x, DEV_ENABLE_CONFIG(port->chip_port));
+
+ lan_rmw(SYS_PTP_MODE_CFG_PTP_MODE_VAL_SET(1),
+ SYS_PTP_MODE_CFG_PTP_MODE_VAL,
+ lan966x, SYS_PTP_MODE_CFG(port->chip_port, 0));
+ }
+
/* Enable PCS */
lan_wr(DEV_PCS1G_CFG_PCS_ENA_SET(1),
lan966x, DEV_PCS1G_CFG(port->chip_port));
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
index 63905bb5a63a..4d8c865eda90 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_ptp.c
@@ -257,11 +257,47 @@ int lan966x_ptp_setup_traps(struct lan966x_port *port,
return lan966x_ptp_add_traps(port);
}
+/* Enable or disable PCH timestamp transmission. This uses the USGMII PCH
+ * extensions to transmit the timestamps in the frame preamble.
+ */
+static void lan966x_ptp_pch_configure(struct lan966x_port *port, bool *enable)
+{
+ struct phy_device *phydev = port->dev->phydev;
+ int ret;
+
+ if (!phydev)
+ *enable = false;
+
+ if (*enable) {
+ /* If we cannot enable inband PCH mode, we fallback to classic
+ * timestamping
+ */
+ if (phy_inband_ext_available(phydev, PHY_INBAND_EXT_PCH_TIMESTAMP)) {
+ ret = phy_inband_ext_enable(phydev, PHY_INBAND_EXT_PCH_TIMESTAMP);
+ if (ret)
+ *enable = false;
+ } else {
+ *enable = false;
+ }
+ } else {
+ phy_inband_ext_disable(phydev, PHY_INBAND_EXT_PCH_TIMESTAMP);
+ }
+
+ lan_rmw(SYS_PCH_CFG_PCH_SUB_PORT_ID_SET(port->chip_port % 4) |
+ SYS_PCH_CFG_PCH_TX_MODE_SET(*enable) |
+ SYS_PCH_CFG_PCH_RX_MODE_SET(*enable),
+ SYS_PCH_CFG_PCH_SUB_PORT_ID |
+ SYS_PCH_CFG_PCH_TX_MODE |
+ SYS_PCH_CFG_PCH_RX_MODE,
+ port->lan966x, SYS_PCH_CFG(port->chip_port));
+}
+
int lan966x_ptp_hwtstamp_set(struct lan966x_port *port,
struct kernel_hwtstamp_config *cfg,
struct netlink_ext_ack *extack)
{
struct lan966x *lan966x = port->lan966x;
+ bool timestamp_in_pch = false;
struct lan966x_phc *phc;
switch (cfg->tx_type) {
@@ -303,10 +339,18 @@ int lan966x_ptp_hwtstamp_set(struct lan966x_port *port,
return -ERANGE;
}
+ if (cfg->source == HWTSTAMP_SOURCE_PHYLIB &&
+ cfg->tx_type == HWTSTAMP_TX_ON &&
+ port->config.portmode == PHY_INTERFACE_MODE_QUSGMII)
+ timestamp_in_pch = true;
+
+ lan966x_ptp_pch_configure(port, ×tamp_in_pch);
+
/* Commit back the result & save it */
mutex_lock(&lan966x->ptp_lock);
phc = &lan966x->phc[LAN966X_PHC_PORT];
phc->hwtstamp_config = *cfg;
+ phc->pch = timestamp_in_pch;
mutex_unlock(&lan966x->ptp_lock);
return 0;
@@ -397,6 +441,7 @@ int lan966x_ptp_txtstamp_request(struct lan966x_port *port,
LAN966X_SKB_CB(skb)->jiffies = jiffies;
lan966x->ptp_skbs++;
+
port->ts_id++;
if (port->ts_id == LAN966X_MAX_PTP_ID)
port->ts_id = 0;
@@ -500,6 +545,27 @@ irqreturn_t lan966x_ptp_irq_handler(int irq, void *args)
/* Read RX timestamping to get the ID */
id = lan_rd(lan966x, PTP_TWOSTEP_STAMP);
+ /* If PCH is enabled, there is a "feature" that also the MAC
+ * will generate an interrupt for transmitted frames. This
+ * interrupt should be ignored, so clear the allocated resources
+ * and try to get the next timestamp. Maybe should clean the
+ * resources on the TX side?
+ */
+ if (phy_inband_ext_enabled(port->dev->phydev,
+ PHY_INBAND_EXT_PCH_TIMESTAMP)) {
+ spin_lock(&lan966x->ptp_ts_id_lock);
+ lan966x->ptp_skbs--;
+ spin_unlock(&lan966x->ptp_ts_id_lock);
+
+ dev_kfree_skb_any(skb_match);
+
+ lan_rmw(PTP_TWOSTEP_CTRL_NXT_SET(1),
+ PTP_TWOSTEP_CTRL_NXT,
+ lan966x, PTP_TWOSTEP_CTRL);
+
+ continue;
+ }
+
spin_lock_irqsave(&port->tx_skbs.lock, flags);
skb_queue_walk_safe(&port->tx_skbs, skb, skb_tmp) {
if (LAN966X_SKB_CB(skb)->ts_id != id)
@@ -1088,19 +1154,27 @@ void lan966x_ptp_rxtstamp(struct lan966x *lan966x, struct sk_buff *skb,
struct timespec64 ts;
u64 full_ts_in_ns;
+ phc = &lan966x->phc[LAN966X_PHC_PORT];
+
if (!lan966x->ptp ||
- !lan966x->ports[src_port]->ptp_rx_cmd)
+ !lan966x->ports[src_port]->ptp_rx_cmd ||
+ !phc->pch)
return;
- phc = &lan966x->phc[LAN966X_PHC_PORT];
- lan966x_ptp_gettime64(&phc->info, &ts);
-
- /* Drop the sub-ns precision */
- timestamp = timestamp >> 2;
- if (ts.tv_nsec < timestamp)
- ts.tv_sec--;
- ts.tv_nsec = timestamp;
- full_ts_in_ns = ktime_set(ts.tv_sec, ts.tv_nsec);
+ if (phc->pch) {
+ /* Drop the sub-ns precision */
+ timestamp = timestamp >> 2;
+ full_ts_in_ns = lower_32_bits(timestamp);
+ } else {
+ lan966x_ptp_gettime64(&phc->info, &ts);
+
+ /* Drop the sub-ns precision */
+ timestamp = timestamp >> 2;
+ if (ts.tv_nsec < timestamp)
+ ts.tv_sec--;
+ ts.tv_nsec = timestamp;
+ full_ts_in_ns = ktime_set(ts.tv_sec, ts.tv_nsec);
+ }
shhwtstamps = skb_hwtstamps(skb);
shhwtstamps->hwtstamp = full_ts_in_ns;
diff --git a/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h b/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
index 4b553927d2e0..bf4df025925e 100644
--- a/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
+++ b/drivers/net/ethernet/microchip/lan966x/lan966x_regs.h
@@ -937,6 +937,27 @@ enum lan966x_target {
#define DEV_PCS1G_STICKY_LINK_DOWN_STICKY_GET(x)\
FIELD_GET(DEV_PCS1G_STICKY_LINK_DOWN_STICKY, x)
+/* DEV:MM_CONFIG:ENABLE_CONFIG */
+#define DEV_ENABLE_CONFIG(t) __REG(TARGET_DEV, t, 8, 156, 0, 1, 8, 0, 0, 1, 4)
+
+#define DEV_ENABLE_CONFIG_KEEP_S_AFTER_D BIT(8)
+#define DEV_ENABLE_CONFIG_KEEP_S_AFTER_D_SET(x)\
+ FIELD_PREP(DEV_ENABLE_CONFIG_KEEP_S_AFTER_D, x)
+#define DEV_ENABLE_CONFIG_KEEP_S_AFTER_D_GET(x)\
+ FIELD_GET(DEV_ENABLE_CONFIG_KEEP_S_AFTER_D, x)
+
+#define DEV_ENABLE_CONFIG_MM_TX_ENA BIT(4)
+#define DEV_ENABLE_CONFIG_MM_TX_ENA_SET(x)\
+ FIELD_PREP(DEV_ENABLE_CONFIG_MM_TX_ENA, x)
+#define DEV_ENABLE_CONFIG_MM_TX_ENA_GET(x)\
+ FIELD_GET(DEV_ENABLE_CONFIG_MM_TX_ENA, x)
+
+#define DEV_ENABLE_CONFIG_MM_RX_ENA BIT(0)
+#define DEV_ENABLE_CONFIG_MM_RX_ENA_SET(x)\
+ FIELD_PREP(DEV_ENABLE_CONFIG_MM_RX_ENA, x)
+#define DEV_ENABLE_CONFIG_MM_RX_ENA_GET(x)\
+ FIELD_GET(DEV_ENABLE_CONFIG_MM_RX_ENA, x)
+
/* FDMA:FDMA:FDMA_CH_ACTIVATE */
#define FDMA_CH_ACTIVATE __REG(TARGET_FDMA, 0, 1, 8, 0, 1, 428, 0, 0, 1, 4)
@@ -1184,6 +1205,15 @@ enum lan966x_target {
#define PTP_TWOSTEP_STAMP_STAMP_NSEC_GET(x)\
FIELD_GET(PTP_TWOSTEP_STAMP_STAMP_NSEC, x)
+/* SYS:PTPPORT:PTP_MODE_CFG */
+#define SYS_PTP_MODE_CFG(g, r) __REG(TARGET_SYS, 0, 1, 4452, g, 10, 28, 20, r, 2, 4)
+
+#define SYS_PTP_MODE_CFG_PTP_MODE_VAL GENMASK(1, 0)
+#define SYS_PTP_MODE_CFG_PTP_MODE_VAL_SET(x)\
+ FIELD_PREP(SYS_PTP_MODE_CFG_PTP_MODE_VAL, x)
+#define SYS_PTP_MODE_CFG_PTP_MODE_VAL_GET(x)\
+ FIELD_GET(SYS_PTP_MODE_CFG_PTP_MODE_VAL, x)
+
/* DEVCPU_QS:XTR:XTR_GRP_CFG */
#define QS_XTR_GRP_CFG(r) __REG(TARGET_QS, 0, 1, 0, 0, 1, 36, 0, r, 2, 4)
@@ -1623,6 +1653,20 @@ enum lan966x_target {
FIELD_PREP(REW_STAT_CFG_STAT_MODE, x)
#define REW_STAT_CFG_STAT_MODE_GET(x)\
FIELD_GET(REW_STAT_CFG_STAT_MODE, x)
+/* REW:PORT:PTP_MISC_CFG */
+#define REW_PTP_MISC_CFG(g) __REG(TARGET_REW, 0, 1, 0, g, 10, 128, 80, 0, 1, 4)
+
+#define REW_PTP_MISC_CFG_PTP_RSRV_MOVEBACK BIT(2)
+#define REW_PTP_MISC_CFG_PTP_RSRV_MOVEBACK_SET(x)\
+ FIELD_PREP(REW_PTP_MISC_CFG_PTP_RSRV_MOVEBACK, x)
+#define REW_PTP_MISC_CFG_PTP_RSRV_MOVEBACK_GET(x)\
+ FIELD_GET(REW_PTP_MISC_CFG_PTP_RSRV_MOVEBACK, x)
+
+#define REW_PTP_MISC_CFG_PTP_SIGNATURE_SEL GENMASK(1, 0)
+#define REW_PTP_MISC_CFG_PTP_SIGNATURE_SEL_SET(x)\
+ FIELD_PREP(REW_PTP_MISC_CFG_PTP_SIGNATURE_SEL, x)
+#define REW_PTP_MISC_CFG_PTP_SIGNATURE_SEL_GET(x)\
+ FIELD_GET(REW_PTP_MISC_CFG_PTP_SIGNATURE_SEL, x)
/* SYS:SYSTEM:RESET_CFG */
#define SYS_RESET_CFG __REG(TARGET_SYS, 0, 1, 4128, 0, 1, 168, 0, 0, 1, 4)
@@ -1884,5 +1928,25 @@ enum lan966x_target {
/* VCAP:VCAP_CONST:IF_CNT */
#define VCAP_IF_CNT(t) __REG(TARGET_VCAP, t, 3, 924, 0, 1, 40, 36, 0, 1, 4)
+/* SYS:PTPPORT:PCH_CFG */
+#define SYS_PCH_CFG(g) __REG(TARGET_SYS, 0, 1, 4452, g, 10, 28, 0, 0, 1, 4)
+
+#define SYS_PCH_CFG_PCH_SUB_PORT_ID GENMASK(10, 7)
+#define SYS_PCH_CFG_PCH_SUB_PORT_ID_SET(x)\
+ FIELD_PREP(SYS_PCH_CFG_PCH_SUB_PORT_ID, x)
+#define SYS_PCH_CFG_PCH_SUB_PORT_ID_GET(x)\
+ FIELD_GET(SYS_PCH_CFG_PCH_SUB_PORT_ID, x)
+
+#define SYS_PCH_CFG_PCH_TX_MODE GENMASK(6, 5)
+#define SYS_PCH_CFG_PCH_TX_MODE_SET(x)\
+ FIELD_PREP(SYS_PCH_CFG_PCH_TX_MODE, x)
+#define SYS_PCH_CFG_PCH_TX_MODE_GET(x)\
+ FIELD_GET(SYS_MAC_FC_CFG_PAUSE_VAL_CFG, x)
+
+#define SYS_PCH_CFG_PCH_RX_MODE GENMASK(4, 2)
+#define SYS_PCH_CFG_PCH_RX_MODE_SET(x)\
+ FIELD_PREP(SYS_PCH_CFG_PCH_RX_MODE, x)
+#define SYS_PCH_CFG_PCH_RX_MODE_GET(x)\
+ FIELD_GET(SYS_PCH_CFG_PCH_RX_MODE, x)
#endif /* _LAN966X_REGS_H_ */
--
2.43.0
The USGMII Standard by Cisco introduces the notion of extensions used
in the preamble. The standard proposes a "PCH" extension, which allows
passing timestamps in the preamble. However, other alternatives are
possible, like Microchip's "MCH" mode, that allows passing indication to
a PHY telling whether or not the PHY should timestamp an outgoing packet,
therefore removing the need for the PHY to have an internal classifier.
This commit allows reporting the various extensions a PHY supports,
without tying them to the actual PHY mode. This is done 1) because there
are multiple variants of the USGMII mode, like QUSGMII and OUSGMII, and
2) because other non-cisco standards might one day propose a similar
mechanism.
Signed-off-by: Maxime Chevallier <[email protected]>
---
Documentation/networking/phy.rst | 70 ++++++++++++++++++++++++++
drivers/net/phy/phy.c | 86 ++++++++++++++++++++++++++++++++
include/linux/phy.h | 28 +++++++++++
3 files changed, 184 insertions(+)
diff --git a/Documentation/networking/phy.rst b/Documentation/networking/phy.rst
index 1283240d7620..f10a45ac7053 100644
--- a/Documentation/networking/phy.rst
+++ b/Documentation/networking/phy.rst
@@ -538,6 +538,76 @@ Call one of following function before unloading module::
int phy_unregister_fixup_for_uid(u32 phy_uid, u32 phy_uid_mask);
int phy_register_fixup_for_id(const char *phy_id);
+Inband Extensions
+=================
+
+The USGMII Standard allows the possibility to re-use the full-length 7-bytes
+frame preamble to convey meaningful data. This is already partly used by modes
+like QSGMII, which passes the port number in the preamble.
+
+In USGMII, we have a standardized approach to allow the MAC and PHY to pass
+such data in the preamble, which looks like this :
+
+| 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | Frame data
+| SoP | | Extension | CRC |
+| / \_______________ | |
+| / \ | |
+| | type | subport | ext type | | |
+
+The preamble in that case uses the Packet Control Header (PCH) format, where
+the byte 1 is used as a control field with :
+
+type - 2 bits :
+ - 00 : Packet with PCH
+ - 01 : Packet without PCH
+ - 10 : Idle Packet, without data
+ - 11 : Reserved
+
+subport - 4 bits : The subport identifier. For QUSGMII, this field ranges from
+ 0 to 3, and for OUSGMII, it ranges from 0 to 7.
+
+ext type - 2 bits : Indicated the type of data conveyed in the extension
+ - 00 : Ignore extension
+ - 01 : 8 bits reserved + 32 timestamp
+ - 10 : Reserved
+ - 11 : Reserved
+
+It is possible for vendors to use the extensions mechanism without relying on
+the PCH formatting.
+
+In order to leverage such extensions, both the MAC and the PHY have to agree on
+which extension to use. The current model has the PHY expose the possible
+available extensions with ::
+
+ bool phy_inband_ext_available(struct phy_device *phydev, enum phy_inband_ext ext);
+
+The PHY driver decides which extensions are available to use at any given time,
+as they can only be used if ::
+ - A compatible PHY mode is used, such as USXGMII or QUSGMII
+ - The PHY can use the required mode at that moment
+
+A PHY driver can register available modes with::
+
+ int phy_inband_ext_set_available(struct phy_device *phydev, enum phy_inband_ext ext);
+ int phy_inband_ext_set_unavailable(struct phy_device *phydev, enum phy_inband_ext ext);
+
+It's then up to the MAC driver to enable/disable the extension in the PHY as
+needed. This was designed to fit the timestamping configuration model, as it
+is the only mode supported so far.
+
+Enabling/Disabling an extension is done from the MAC driver through::
+
+ int phy_inband_ext_enable(struct phy_device *phydev, enum phy_inband_ext ext);
+ int phy_inband_ext_disable(struct phy_device *phydev, enum phy_inband_ext ext);
+
+These functions will cause the relevant callback to be called in the PHY driver::
+
+ int (*set_inband_ext)(struct phy_device *dev, enum phy_inband_ext ext, bool enable);
+
+The state of currently enabled extensions can be queried with::
+
+ bool phy_inband_ext_enabled(struct phy_device *phydev, enum phy_inband_ext ext);
+
Standards
=========
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 3b9531143be1..4b6cf94f51d5 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1760,3 +1760,89 @@ int phy_ethtool_nway_reset(struct net_device *ndev)
return ret;
}
EXPORT_SYMBOL(phy_ethtool_nway_reset);
+
+/**
+ * PHY modes in the USXGMII family can have extensions, with data transmitted
+ * in the frame preamble.
+ * For now, only QUSGMII is supported, but other variants like USGMII and
+ * OUSGMII can be added in the future.
+ */
+static inline bool phy_interface_has_inband_ext(phy_interface_t interface)
+{
+ return interface == PHY_INTERFACE_MODE_QUSGMII;
+}
+
+bool phy_inband_ext_available(struct phy_device *phydev, enum phy_inband_ext ext)
+{
+ return !!(phydev->inband_ext.available & ext);
+}
+EXPORT_SYMBOL(phy_inband_ext_available);
+
+bool phy_inband_ext_enabled(struct phy_device *phydev, enum phy_inband_ext ext)
+{
+ return !!(phydev->inband_ext.enabled & ext);
+}
+EXPORT_SYMBOL(phy_inband_ext_enabled);
+
+static int phy_set_inband_ext(struct phy_device *phydev,
+ enum phy_inband_ext ext,
+ bool enable)
+{
+ int ret;
+
+ if (!phy_interface_has_inband_ext(phydev->interface))
+ return -EOPNOTSUPP;
+
+ if (!phydev->drv->set_inband_ext)
+ return -EOPNOTSUPP;
+
+ mutex_lock(&phydev->lock);
+ ret = phydev->drv->set_inband_ext(phydev, ext, enable);
+ mutex_unlock(&phydev->lock);
+ if (ret)
+ return ret;
+
+ if (enable)
+ phydev->inband_ext.enabled |= BIT(ext);
+ else
+ phydev->inband_ext.enabled &= ~BIT(ext);
+
+ return 0;
+}
+
+int phy_inband_ext_enable(struct phy_device *phydev, enum phy_inband_ext ext)
+{
+ if (!phy_inband_ext_available(phydev, ext))
+ return -EOPNOTSUPP;
+
+ return phy_set_inband_ext(phydev, ext, true);
+}
+EXPORT_SYMBOL(phy_inband_ext_enable);
+
+int phy_inband_ext_disable(struct phy_device *phydev, enum phy_inband_ext ext)
+{
+ return phy_set_inband_ext(phydev, ext, false);
+}
+EXPORT_SYMBOL(phy_inband_ext_disable);
+
+int phy_inband_ext_set_available(struct phy_device *phydev, enum phy_inband_ext ext)
+{
+ if (!(BIT(ext) & phydev->drv->inband_ext))
+ return -EOPNOTSUPP;
+
+ phydev->inband_ext.available |= BIT(ext);
+
+ return 0;
+}
+EXPORT_SYMBOL(phy_inband_ext_set_available);
+
+int phy_inband_ext_set_unavailable(struct phy_device *phydev, enum phy_inband_ext ext)
+{
+ if (!(BIT(ext) & phydev->drv->inband_ext))
+ return -EOPNOTSUPP;
+
+ phydev->inband_ext.available &= ~BIT(ext);
+
+ return 0;
+}
+EXPORT_SYMBOL(phy_inband_ext_set_unavailable);
diff --git a/include/linux/phy.h b/include/linux/phy.h
index a66f07d3f5f4..b358a96f71e7 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -202,6 +202,25 @@ static inline void phy_interface_set_rgmii(unsigned long *intf)
__set_bit(PHY_INTERFACE_MODE_RGMII_TXID, intf);
}
+/**
+ * enum phy_inband_ext - Inband extensions
+ *
+ * @PHY_INBAND_EXT_PCH_TIMESTAMP: Transmit the nanoseconds part of a timestamp,
+ * Using the PCH format.
+ *
+ * Describes the inband extensions that can be conveyed in the ethernet preamble
+ */
+enum phy_inband_ext {
+ PHY_INBAND_EXT_PCH_TIMESTAMP = 0,
+};
+
+int phy_inband_ext_enable(struct phy_device *phydev, enum phy_inband_ext ext);
+int phy_inband_ext_disable(struct phy_device *phydev, enum phy_inband_ext ext);
+int phy_inband_ext_set_available(struct phy_device *phydev, enum phy_inband_ext ext);
+int phy_inband_ext_set_unavailable(struct phy_device *phydev, enum phy_inband_ext ext);
+bool phy_inband_ext_available(struct phy_device *phydev, enum phy_inband_ext ext);
+bool phy_inband_ext_enabled(struct phy_device *phydev, enum phy_inband_ext ext);
+
/*
* phy_supported_speeds - return all speeds currently supported by a PHY device
*/
@@ -678,6 +697,11 @@ struct phy_device {
phy_interface_t interface;
DECLARE_PHY_INTERFACE_MASK(possible_interfaces);
+ struct {
+ u32 available;
+ u32 enabled;
+ } inband_ext;
+
/*
* forced speed & duplex (no autoneg)
* partner speed & duplex & pause (autoneg)
@@ -908,6 +932,7 @@ struct phy_driver {
u32 phy_id_mask;
const unsigned long * const features;
u32 flags;
+ u32 inband_ext;
const void *driver_data;
/**
@@ -1167,6 +1192,9 @@ struct phy_driver {
*/
int (*led_polarity_set)(struct phy_device *dev, int index,
unsigned long modes);
+ /** @set_inband_ext: Enable or disable a given extension*/
+ int (*set_inband_ext)(struct phy_device *dev, enum phy_inband_ext ext,
+ bool enable);
};
#define to_phy_driver(d) container_of(to_mdio_common_driver(d), \
struct phy_driver, mdiodrv)
--
2.43.0
This commit adds support for the PCH extension in the Lan8814 PHY,
allowing the PHY to report RX timestamps to the MAC in the ethernet
preamble.
When using the PCH extension, the PHY will only report the nanoseconds
part of the timestamp in-band, and the seconds part out-of-band.
The main goal in the end is to lower the pressure on the MDIO bus, which
may get pushed to its limit on 48 ports switches doing PTP at a high
rate.
Signed-off-by: Maxime Chevallier <[email protected]>
---
drivers/net/phy/micrel.c | 84 ++++++++++++++++++++++++++++++++++++++--
1 file changed, 80 insertions(+), 4 deletions(-)
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 9b6973581989..d8084174c5a7 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -149,6 +149,10 @@
#define LTC_HARD_RESET 0x023F
#define LTC_HARD_RESET_ BIT(0)
+#define TSU_GENERAL_CONFIG 0x2C0
+#define TSU_GENERAL_CONFIG_TSU_ENABLE_ BIT(0)
+#define TSU_GENERAL_CONFIG_TSU_ENABLE_PCH_ BIT(1)
+
#define TSU_HARD_RESET 0x02C1
#define TSU_HARD_RESET_ BIT(0)
@@ -174,6 +178,7 @@
#define PTP_OPERATING_MODE 0x0241
#define PTP_OPERATING_MODE_STANDALONE_ BIT(0)
+#define PTP_OPERATING_MODE_PCH_ BIT(1)
#define PTP_TX_MOD 0x028F
#define PTP_TX_MOD_TX_PTP_SYNC_TS_INSERT_ BIT(12)
@@ -2360,6 +2365,16 @@ static void lan8814_ptp_rx_ts_get(struct phy_device *phydev,
*seq_id = lanphy_read_page_reg(phydev, 5, PTP_RX_MSG_HEADER2);
}
+static void lan8814_ptp_rx_ts_get_partial(struct phy_device *phydev,
+ u32 *seconds, u16 *seq_id)
+{
+ *seconds = lanphy_read_page_reg(phydev, 5, PTP_RX_INGRESS_SEC_HI);
+ *seconds = (*seconds << 16) |
+ lanphy_read_page_reg(phydev, 5, PTP_RX_INGRESS_SEC_LO);
+
+ *seq_id = lanphy_read_page_reg(phydev, 5, PTP_RX_MSG_HEADER2);
+}
+
static void lan8814_ptp_tx_ts_get(struct phy_device *phydev,
u32 *seconds, u32 *nano_seconds, u16 *seq_id)
{
@@ -2504,6 +2519,12 @@ static int lan8814_hwtstamp(struct mii_timestamper *mii_ts,
lan8814_flush_fifo(ptp_priv->phydev, false);
lan8814_flush_fifo(ptp_priv->phydev, true);
+ if (phydev->interface == PHY_INTERFACE_MODE_QUSGMII &&
+ ptp_priv->hwts_tx_type == HWTSTAMP_TX_ON)
+ phy_inband_ext_set_available(phydev, PHY_INBAND_EXT_PCH_TIMESTAMP);
+ else
+ phy_inband_ext_set_unavailable(phydev, PHY_INBAND_EXT_PCH_TIMESTAMP);
+
return 0;
}
@@ -2917,8 +2938,20 @@ static bool lan8814_match_skb(struct kszphy_ptp_priv *ptp_priv,
if (ret) {
shhwtstamps = skb_hwtstamps(skb);
- memset(shhwtstamps, 0, sizeof(*shhwtstamps));
- shhwtstamps->hwtstamp = ktime_set(rx_ts->seconds, rx_ts->nsec);
+
+ if (phy_inband_ext_enabled(ptp_priv->phydev, PHY_INBAND_EXT_PCH_TIMESTAMP)) {
+ /* When using the PCH extension, we get the seconds part
+ * from MDIO accesses, but the seconds part gets
+ * set by the MAC driver according to the PCH data in the
+ * preamble
+ */
+ struct timespec64 ts = ktime_to_timespec64(shhwtstamps->hwtstamp);
+
+ shhwtstamps->hwtstamp = ktime_set(rx_ts->seconds, ts.tv_nsec);
+ } else {
+ memset(shhwtstamps, 0, sizeof(*shhwtstamps));
+ shhwtstamps->hwtstamp = ktime_set(rx_ts->seconds, rx_ts->nsec);
+ }
netif_rx(skb);
}
@@ -2953,8 +2986,18 @@ static void lan8814_get_rx_ts(struct kszphy_ptp_priv *ptp_priv)
if (!rx_ts)
return;
- lan8814_ptp_rx_ts_get(phydev, &rx_ts->seconds, &rx_ts->nsec,
- &rx_ts->seq_id);
+ /* When using PCH mode, the nanoseconds part of the timestamp is
+ * transmitted inband through the ethernet preamble. We only need
+ * to retrieve the seconds part along with the seq_id, the MAC
+ * driver will fill-in the nanoseconds part itself
+ */
+ if (phy_inband_ext_enabled(ptp_priv->phydev, PHY_INBAND_EXT_PCH_TIMESTAMP))
+ lan8814_ptp_rx_ts_get_partial(phydev, &rx_ts->seconds,
+ &rx_ts->seq_id);
+ else
+ lan8814_ptp_rx_ts_get(phydev, &rx_ts->seconds,
+ &rx_ts->nsec, &rx_ts->seq_id);
+
lan8814_match_rx_ts(ptp_priv, rx_ts);
/* If other timestamps are available in the FIFO,
@@ -3240,6 +3283,37 @@ static void lan8814_setup_led(struct phy_device *phydev, int val)
lanphy_write_page_reg(phydev, 5, LAN8814_LED_CTRL_1, temp);
}
+static int lan8814_set_inband_ext(struct phy_device *phydev,
+ enum phy_inband_ext ext, bool enable)
+{
+ u32 tsu_cfg;
+
+ if (ext != PHY_INBAND_EXT_PCH_TIMESTAMP)
+ return -EOPNOTSUPP;
+
+ tsu_cfg = ~TSU_GENERAL_CONFIG_TSU_ENABLE_;
+
+ lanphy_write_page_reg(phydev, 5, TSU_GENERAL_CONFIG, tsu_cfg);
+
+ if (enable) {
+ lanphy_write_page_reg(phydev, 4, PTP_OPERATING_MODE,
+ PTP_OPERATING_MODE_PCH_);
+
+ tsu_cfg |= TSU_GENERAL_CONFIG_TSU_ENABLE_PCH_;
+ } else {
+ lanphy_write_page_reg(phydev, 4, PTP_OPERATING_MODE,
+ PTP_OPERATING_MODE_STANDALONE_);
+
+ tsu_cfg &= ~TSU_GENERAL_CONFIG_TSU_ENABLE_PCH_;
+ }
+
+ tsu_cfg |= TSU_GENERAL_CONFIG_TSU_ENABLE_;
+
+ lanphy_write_page_reg(phydev, 5, TSU_GENERAL_CONFIG, tsu_cfg);
+
+ return 0;
+}
+
static int lan8814_config_init(struct phy_device *phydev)
{
struct kszphy_priv *lan8814 = phydev->priv;
@@ -4805,6 +4879,7 @@ static struct phy_driver ksphy_driver[] = {
.phy_id_mask = MICREL_PHY_ID_MASK,
.name = "Microchip INDY Gigabit Quad PHY",
.flags = PHY_POLL_CABLE_TEST,
+ .inband_ext = PHY_INBAND_EXT_PCH_TIMESTAMP,
.config_init = lan8814_config_init,
.driver_data = &lan8814_type,
.probe = lan8814_probe,
@@ -4819,6 +4894,7 @@ static struct phy_driver ksphy_driver[] = {
.handle_interrupt = lan8814_handle_interrupt,
.cable_test_start = lan8814_cable_test_start,
.cable_test_get_status = ksz886x_cable_test_get_status,
+ .set_inband_ext = lan8814_set_inband_ext,
}, {
.phy_id = PHY_ID_LAN8804,
.phy_id_mask = MICREL_PHY_ID_MASK,
--
2.43.0
On Mon, 12 Feb 2024 18:33:04 +0100 Maxime Chevallier wrote:
> +/**
> + * PHY modes in the USXGMII family can have extensions, with data transmitted
> + * in the frame preamble.
> + * For now, only QUSGMII is supported, but other variants like USGMII and
> + * OUSGMII can be added in the future.
> + */
> +static inline bool phy_interface_has_inband_ext(phy_interface_t interface)
Spurious use of /**, it seems. You either need to use full kdoc format,
or the normal /* comment start.
--
pw-bot: cr
Hello Jakub,
On Mon, 12 Feb 2024 17:25:11 -0800
Jakub Kicinski <[email protected]> wrote:
> On Mon, 12 Feb 2024 18:33:04 +0100 Maxime Chevallier wrote:
> > +/**
> > + * PHY modes in the USXGMII family can have extensions, with data transmitted
> > + * in the frame preamble.
> > + * For now, only QUSGMII is supported, but other variants like USGMII and
> > + * OUSGMII can be added in the future.
> > + */
> > +static inline bool phy_interface_has_inband_ext(phy_interface_t interface)
>
>
> Spurious use of /**, it seems. You either need to use full kdoc format,
> or the normal /* comment start.
Ah sorry about that, I'll address it. Thanks for taking a look !
Maxime
The 02/12/2024 18:33, Maxime Chevallier wrote:
> [Some people who received this message don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
Hi Maxime,
I have tried your patches on pcb8291, which is a lan966x without PHYs
that support timestamping. And on this platform this patch breaks up the
things. Because it should just do the timestamping the MAC in that case,
but with this patch it doesn't get any time.
The same issue can be reproduced on pcb8280 and then disable PHY
timestamping, or change the lan8814 not to support HW timestamping.
Please see bellow the reason why.
>
> +/* Enable or disable PCH timestamp transmission. This uses the USGMII PCH
> + * extensions to transmit the timestamps in the frame preamble.
> + */
> +static void lan966x_ptp_pch_configure(struct lan966x_port *port, bool *enable)
> +{
> + struct phy_device *phydev = port->dev->phydev;
> + int ret;
> +
> + if (!phydev)
> + *enable = false;
> +
> + if (*enable) {
> + /* If we cannot enable inband PCH mode, we fallback to classic
> + * timestamping
> + */
> + if (phy_inband_ext_available(phydev, PHY_INBAND_EXT_PCH_TIMESTAMP)) {
> + ret = phy_inband_ext_enable(phydev, PHY_INBAND_EXT_PCH_TIMESTAMP);
> + if (ret)
> + *enable = false;
> + } else {
> + *enable = false;
> + }
> + } else {
> + phy_inband_ext_disable(phydev, PHY_INBAND_EXT_PCH_TIMESTAMP);
> + }
> +
> + lan_rmw(SYS_PCH_CFG_PCH_SUB_PORT_ID_SET(port->chip_port % 4) |
> + SYS_PCH_CFG_PCH_TX_MODE_SET(*enable) |
> + SYS_PCH_CFG_PCH_RX_MODE_SET(*enable),
> + SYS_PCH_CFG_PCH_SUB_PORT_ID |
> + SYS_PCH_CFG_PCH_TX_MODE |
> + SYS_PCH_CFG_PCH_RX_MODE,
> + port->lan966x, SYS_PCH_CFG(port->chip_port));
> +}
> +
> int lan966x_ptp_hwtstamp_set(struct lan966x_port *port,
> struct kernel_hwtstamp_config *cfg,
> struct netlink_ext_ack *extack)
> {
> struct lan966x *lan966x = port->lan966x;
> + bool timestamp_in_pch = false;
> struct lan966x_phc *phc;
>
> switch (cfg->tx_type) {
> @@ -303,10 +339,18 @@ int lan966x_ptp_hwtstamp_set(struct lan966x_port *port,
> return -ERANGE;
> }
>
> + if (cfg->source == HWTSTAMP_SOURCE_PHYLIB &&
> + cfg->tx_type == HWTSTAMP_TX_ON &&
> + port->config.portmode == PHY_INTERFACE_MODE_QUSGMII)
> + timestamp_in_pch = true;
> +
> + lan966x_ptp_pch_configure(port, ×tamp_in_pch);
> +
> /* Commit back the result & save it */
> mutex_lock(&lan966x->ptp_lock);
> phc = &lan966x->phc[LAN966X_PHC_PORT];
> phc->hwtstamp_config = *cfg;
> + phc->pch = timestamp_in_pch;
Here we figure out if pch is enabled or not. If the cfg->source is not
PHYLIB or the interface is not QUSGMII then timestamp_in_pch will stay
false.
> mutex_unlock(&lan966x->ptp_lock);
>
> return 0;
> @@ -397,6 +441,7 @@ int lan966x_ptp_txtstamp_request(struct lan966x_port *port,
> LAN966X_SKB_CB(skb)->jiffies = jiffies;
>
> lan966x->ptp_skbs++;
> +
I think this is just a small style change. So maybe it shouldn't be in
here.
> port->ts_id++;
> if (port->ts_id == LAN966X_MAX_PTP_ID)
> port->ts_id = 0;
> @@ -500,6 +545,27 @@ irqreturn_t lan966x_ptp_irq_handler(int irq, void *args)
> /* Read RX timestamping to get the ID */
> id = lan_rd(lan966x, PTP_TWOSTEP_STAMP);
>
> + /* If PCH is enabled, there is a "feature" that also the MAC
> + * will generate an interrupt for transmitted frames. This
> + * interrupt should be ignored, so clear the allocated resources
> + * and try to get the next timestamp. Maybe should clean the
> + * resources on the TX side?
> + */
> + if (phy_inband_ext_enabled(port->dev->phydev,
> + PHY_INBAND_EXT_PCH_TIMESTAMP)) {
> + spin_lock(&lan966x->ptp_ts_id_lock);
> + lan966x->ptp_skbs--;
> + spin_unlock(&lan966x->ptp_ts_id_lock);
> +
> + dev_kfree_skb_any(skb_match);
> +
> + lan_rmw(PTP_TWOSTEP_CTRL_NXT_SET(1),
> + PTP_TWOSTEP_CTRL_NXT,
> + lan966x, PTP_TWOSTEP_CTRL);
> +
> + continue;
> + }
> +
> spin_lock_irqsave(&port->tx_skbs.lock, flags);
> skb_queue_walk_safe(&port->tx_skbs, skb, skb_tmp) {
> if (LAN966X_SKB_CB(skb)->ts_id != id)
> @@ -1088,19 +1154,27 @@ void lan966x_ptp_rxtstamp(struct lan966x *lan966x, struct sk_buff *skb,
> struct timespec64 ts;
> u64 full_ts_in_ns;
>
> + phc = &lan966x->phc[LAN966X_PHC_PORT];
> +
> if (!lan966x->ptp ||
> - !lan966x->ports[src_port]->ptp_rx_cmd)
> + !lan966x->ports[src_port]->ptp_rx_cmd ||
> + !phc->pch)
And here because phc->pch is false, it would just return.
Meaning that it would never be able to get the time.
I presume that this check should not be modified.
> return;
>
> - phc = &lan966x->phc[LAN966X_PHC_PORT];
> - lan966x_ptp_gettime64(&phc->info, &ts);
> -
> - /* Drop the sub-ns precision */
> - timestamp = timestamp >> 2;
> - if (ts.tv_nsec < timestamp)
> - ts.tv_sec--;
> - ts.tv_nsec = timestamp;
> - full_ts_in_ns = ktime_set(ts.tv_sec, ts.tv_nsec);
> + if (phc->pch) {
> + /* Drop the sub-ns precision */
> + timestamp = timestamp >> 2;
> + full_ts_in_ns = lower_32_bits(timestamp);
> + } else {
> + lan966x_ptp_gettime64(&phc->info, &ts);
> +
> + /* Drop the sub-ns precision */
> + timestamp = timestamp >> 2;
> + if (ts.tv_nsec < timestamp)
> + ts.tv_sec--;
> + ts.tv_nsec = timestamp;
> + full_ts_in_ns = ktime_set(ts.tv_sec, ts.tv_nsec);
> + }
--
/Horatiu
> +Inband Extensions
> +=================
> +
> +The USGMII Standard allows the possibility to re-use the full-length 7-bytes
> +frame preamble to convey meaningful data. This is already partly used by modes
> +like QSGMII, which passes the port number in the preamble.
> +
> +In USGMII, we have a standardized approach to allow the MAC and PHY to pass
> +such data in the preamble, which looks like this :
> +
> +| 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | Frame data
> +| SoP | | Extension | CRC |
> +| / \_______________ | |
> +| / \ | |
> +| | type | subport | ext type | | |
> +
> +The preamble in that case uses the Packet Control Header (PCH) format, where
> +the byte 1 is used as a control field with :
> +
> +type - 2 bits :
> + - 00 : Packet with PCH
> + - 01 : Packet without PCH
> + - 10 : Idle Packet, without data
> + - 11 : Reserved
> +
> +subport - 4 bits : The subport identifier. For QUSGMII, this field ranges from
> + 0 to 3, and for OUSGMII, it ranges from 0 to 7.
> +
> +ext type - 2 bits : Indicated the type of data conveyed in the extension
> + - 00 : Ignore extension
> + - 01 : 8 bits reserved + 32 timestamp
> + - 10 : Reserved
> + - 11 : Reserved
Somewhat crystal ball...
Those two reserved values could be used in the future to indicate
other extensions. So we could have three in operation at once, but
only one selected per frame.
> +A PHY driver can register available modes with::
> +
> + int phy_inband_ext_set_available(struct phy_device *phydev, enum phy_inband_ext ext);
> + int phy_inband_ext_set_unavailable(struct phy_device *phydev, enum phy_inband_ext ext);
enum phy_inband_ext is just an well defined, but arbitrary number? 0
is this time stamp value mode, 1 could be used MACSEC, 2 could be a
QoS indicator when doing rate adaptation? 3 could be ....
> +It's then up to the MAC driver to enable/disable the extension in the PHY as
> +needed. This was designed to fit the timestamping configuration model, as it
> +is the only mode supported so far.
> +
> +Enabling/Disabling an extension is done from the MAC driver through::
> +
> + int phy_inband_ext_enable(struct phy_device *phydev, enum phy_inband_ext ext);
So maybe this should return the 2 bit ext type value? The MAC can
request QoS marking, and the PHY replies it expects the bits to be 3 ?
I'm just trying to ensure we have an API which is extensible in the
future to make use of those two reserved values.
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 3b9531143be1..4b6cf94f51d5 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -1760,3 +1760,89 @@ int phy_ethtool_nway_reset(struct net_device *ndev)
> return ret;
> }
> EXPORT_SYMBOL(phy_ethtool_nway_reset);
> +
> +/**
> + * PHY modes in the USXGMII family can have extensions, with data transmitted
> + * in the frame preamble.
> + * For now, only QUSGMII is supported, but other variants like USGMII and
> + * OUSGMII can be added in the future.
> + */
> +static inline bool phy_interface_has_inband_ext(phy_interface_t interface)
No inline functions in .c file please. Let the compiler decide.
> +bool phy_inband_ext_available(struct phy_device *phydev, enum phy_inband_ext ext)
> +{
> + return !!(phydev->inband_ext.available & ext);
should this be BIT(ext) ?
> +}
> +EXPORT_SYMBOL(phy_inband_ext_available);
If you don't mind, i would prefer EXPORT_SYMBOL_GPL().
> +static int phy_set_inband_ext(struct phy_device *phydev,
> + enum phy_inband_ext ext,
> + bool enable)
> +{
> + int ret;
> +
> + if (!phy_interface_has_inband_ext(phydev->interface))
> + return -EOPNOTSUPP;
> +
> + if (!phydev->drv->set_inband_ext)
> + return -EOPNOTSUPP;
That is a driver bug. It should not set phydev->inband_ext.available
and then not have drv->set_inband_ext. So we should probably test this
earlier. Maybe define that phydev->inband_ext.available has to be set
during probe, and the core can validate this after probe and reject
the device if it is inconsistent?
> +
> + mutex_lock(&phydev->lock);
> + ret = phydev->drv->set_inband_ext(phydev, ext, enable);
> + mutex_unlock(&phydev->lock);
> + if (ret)
> + return ret;
> +
> + if (enable)
> + phydev->inband_ext.enabled |= BIT(ext);
> + else
> + phydev->inband_ext.enabled &= ~BIT(ext);
Should these be also protected by the mutex?
Andrew
Hello Horatiu,
On Tue, 13 Feb 2024 11:31:56 +0100
Horatiu Vultur <[email protected]> wrote:
> The 02/12/2024 18:33, Maxime Chevallier wrote:
> > [Some people who received this message don't often get email from [email protected]. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Hi Maxime,
>
> I have tried your patches on pcb8291, which is a lan966x without PHYs
> that support timestamping. And on this platform this patch breaks up the
> things. Because it should just do the timestamping the MAC in that case,
> but with this patch it doesn't get any time.
> The same issue can be reproduced on pcb8280 and then disable PHY
> timestamping, or change the lan8814 not to support HW timestamping.
>
> Please see bellow the reason why.
You are entirely correct and I apparently messed-up my series as these
changes were implemented locally and somehow lost in the rebase. Indeed
this codes doesn't work at all... I'll resend that, thanks a lot for
the test and sorry !
>
> >
> > +/* Enable or disable PCH timestamp transmission. This uses the USGMII PCH
> > + * extensions to transmit the timestamps in the frame preamble.
> > + */
> > +static void lan966x_ptp_pch_configure(struct lan966x_port *port, bool *enable)
> > +{
> > + struct phy_device *phydev = port->dev->phydev;
> > + int ret;
> > +
> > + if (!phydev)
> > + *enable = false;
> > +
> > + if (*enable) {
> > + /* If we cannot enable inband PCH mode, we fallback to classic
> > + * timestamping
> > + */
> > + if (phy_inband_ext_available(phydev, PHY_INBAND_EXT_PCH_TIMESTAMP)) {
> > + ret = phy_inband_ext_enable(phydev, PHY_INBAND_EXT_PCH_TIMESTAMP);
> > + if (ret)
> > + *enable = false;
> > + } else {
> > + *enable = false;
> > + }
> > + } else {
> > + phy_inband_ext_disable(phydev, PHY_INBAND_EXT_PCH_TIMESTAMP);
> > + }
> > +
> > + lan_rmw(SYS_PCH_CFG_PCH_SUB_PORT_ID_SET(port->chip_port % 4) |
> > + SYS_PCH_CFG_PCH_TX_MODE_SET(*enable) |
> > + SYS_PCH_CFG_PCH_RX_MODE_SET(*enable),
> > + SYS_PCH_CFG_PCH_SUB_PORT_ID |
> > + SYS_PCH_CFG_PCH_TX_MODE |
> > + SYS_PCH_CFG_PCH_RX_MODE,
> > + port->lan966x, SYS_PCH_CFG(port->chip_port));
> > +}
> > +
> > int lan966x_ptp_hwtstamp_set(struct lan966x_port *port,
> > struct kernel_hwtstamp_config *cfg,
> > struct netlink_ext_ack *extack)
> > {
> > struct lan966x *lan966x = port->lan966x;
> > + bool timestamp_in_pch = false;
> > struct lan966x_phc *phc;
> >
> > switch (cfg->tx_type) {
> > @@ -303,10 +339,18 @@ int lan966x_ptp_hwtstamp_set(struct lan966x_port *port,
> > return -ERANGE;
> > }
> >
> > + if (cfg->source == HWTSTAMP_SOURCE_PHYLIB &&
> > + cfg->tx_type == HWTSTAMP_TX_ON &&
> > + port->config.portmode == PHY_INTERFACE_MODE_QUSGMII)
> > + timestamp_in_pch = true;
> > +
> > + lan966x_ptp_pch_configure(port, ×tamp_in_pch);
> > +
> > /* Commit back the result & save it */
> > mutex_lock(&lan966x->ptp_lock);
> > phc = &lan966x->phc[LAN966X_PHC_PORT];
> > phc->hwtstamp_config = *cfg;
> > + phc->pch = timestamp_in_pch;
>
> Here we figure out if pch is enabled or not. If the cfg->source is not
> PHYLIB or the interface is not QUSGMII then timestamp_in_pch will stay
> false.
>
> > mutex_unlock(&lan966x->ptp_lock);
> >
> > return 0;
> > @@ -397,6 +441,7 @@ int lan966x_ptp_txtstamp_request(struct lan966x_port *port,
> > LAN966X_SKB_CB(skb)->jiffies = jiffies;
> >
> > lan966x->ptp_skbs++;
> > +
>
> I think this is just a small style change. So maybe it shouldn't be in
> here.
>
> > port->ts_id++;
> > if (port->ts_id == LAN966X_MAX_PTP_ID)
> > port->ts_id = 0;
> > @@ -500,6 +545,27 @@ irqreturn_t lan966x_ptp_irq_handler(int irq, void *args)
> > /* Read RX timestamping to get the ID */
> > id = lan_rd(lan966x, PTP_TWOSTEP_STAMP);
> >
> > + /* If PCH is enabled, there is a "feature" that also the MAC
> > + * will generate an interrupt for transmitted frames. This
> > + * interrupt should be ignored, so clear the allocated resources
> > + * and try to get the next timestamp. Maybe should clean the
> > + * resources on the TX side?
> > + */
> > + if (phy_inband_ext_enabled(port->dev->phydev,
> > + PHY_INBAND_EXT_PCH_TIMESTAMP)) {
> > + spin_lock(&lan966x->ptp_ts_id_lock);
> > + lan966x->ptp_skbs--;
> > + spin_unlock(&lan966x->ptp_ts_id_lock);
> > +
> > + dev_kfree_skb_any(skb_match);
> > +
> > + lan_rmw(PTP_TWOSTEP_CTRL_NXT_SET(1),
> > + PTP_TWOSTEP_CTRL_NXT,
> > + lan966x, PTP_TWOSTEP_CTRL);
> > +
> > + continue;
> > + }
> > +
> > spin_lock_irqsave(&port->tx_skbs.lock, flags);
> > skb_queue_walk_safe(&port->tx_skbs, skb, skb_tmp) {
> > if (LAN966X_SKB_CB(skb)->ts_id != id)
> > @@ -1088,19 +1154,27 @@ void lan966x_ptp_rxtstamp(struct lan966x *lan966x, struct sk_buff *skb,
> > struct timespec64 ts;
> > u64 full_ts_in_ns;
> >
> > + phc = &lan966x->phc[LAN966X_PHC_PORT];
> > +
> > if (!lan966x->ptp ||
> > - !lan966x->ports[src_port]->ptp_rx_cmd)
> > + !lan966x->ports[src_port]->ptp_rx_cmd ||
> > + !phc->pch)
>
> And here because phc->pch is false, it would just return.
> Meaning that it would never be able to get the time.
> I presume that this check should not be modified.
Dammit you are right and I had these modifications locally, but
apparently I messed my rebase and lost that...
>
> > return;
> >
> > - phc = &lan966x->phc[LAN966X_PHC_PORT];
> > - lan966x_ptp_gettime64(&phc->info, &ts);
> > -
> > - /* Drop the sub-ns precision */
> > - timestamp = timestamp >> 2;
> > - if (ts.tv_nsec < timestamp)
> > - ts.tv_sec--;
> > - ts.tv_nsec = timestamp;
> > - full_ts_in_ns = ktime_set(ts.tv_sec, ts.tv_nsec);
> > + if (phc->pch) {
> > + /* Drop the sub-ns precision */
> > + timestamp = timestamp >> 2;
> > + full_ts_in_ns = lower_32_bits(timestamp);
> > + } else {
> > + lan966x_ptp_gettime64(&phc->info, &ts);
> > +
> > + /* Drop the sub-ns precision */
> > + timestamp = timestamp >> 2;
> > + if (ts.tv_nsec < timestamp)
> > + ts.tv_sec--;
> > + ts.tv_nsec = timestamp;
> > + full_ts_in_ns = ktime_set(ts.tv_sec, ts.tv_nsec);
> > + }
>
>
Thanks for the review and analysis Horatiu, and sorry for this hiccup !
Maxime
Hello Andrew,
On Tue, 13 Feb 2024 15:03:01 +0100
Andrew Lunn <[email protected]> wrote:
> > +Inband Extensions
> > +=================
> > +
> > +The USGMII Standard allows the possibility to re-use the full-length 7-bytes
> > +frame preamble to convey meaningful data. This is already partly used by modes
> > +like QSGMII, which passes the port number in the preamble.
> > +
> > +In USGMII, we have a standardized approach to allow the MAC and PHY to pass
> > +such data in the preamble, which looks like this :
> > +
> > +| 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | Frame data
> > +| SoP | | Extension | CRC |
> > +| / \_______________ | |
> > +| / \ | |
> > +| | type | subport | ext type | | |
> > +
> > +The preamble in that case uses the Packet Control Header (PCH) format, where
> > +the byte 1 is used as a control field with :
> > +
> > +type - 2 bits :
> > + - 00 : Packet with PCH
> > + - 01 : Packet without PCH
> > + - 10 : Idle Packet, without data
> > + - 11 : Reserved
> > +
> > +subport - 4 bits : The subport identifier. For QUSGMII, this field ranges from
> > + 0 to 3, and for OUSGMII, it ranges from 0 to 7.
> > +
> > +ext type - 2 bits : Indicated the type of data conveyed in the extension
> > + - 00 : Ignore extension
> > + - 01 : 8 bits reserved + 32 timestamp
> > + - 10 : Reserved
> > + - 11 : Reserved
>
> Somewhat crystal ball...
>
> Those two reserved values could be used in the future to indicate
> other extensions. So we could have three in operation at once, but
> only one selected per frame.
>
> > +A PHY driver can register available modes with::
> > +
> > + int phy_inband_ext_set_available(struct phy_device *phydev, enum phy_inband_ext ext);
> > + int phy_inband_ext_set_unavailable(struct phy_device *phydev, enum phy_inband_ext ext);
>
> enum phy_inband_ext is just an well defined, but arbitrary number? 0
> is this time stamp value mode, 1 could be used MACSEC, 2 could be a
> QoS indicator when doing rate adaptation? 3 could be ....
>
> > +It's then up to the MAC driver to enable/disable the extension in the PHY as
> > +needed. This was designed to fit the timestamping configuration model, as it
> > +is the only mode supported so far.
> > +
> > +Enabling/Disabling an extension is done from the MAC driver through::
> > +
> > + int phy_inband_ext_enable(struct phy_device *phydev, enum phy_inband_ext ext);
>
> So maybe this should return the 2 bit ext type value? The MAC can
> request QoS marking, and the PHY replies it expects the bits to be 3 ?
>
> I'm just trying to ensure we have an API which is extensible in the
> future to make use of those two reserved values.
You are right, that's a much better idea !
>
> > diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> > index 3b9531143be1..4b6cf94f51d5 100644
> > --- a/drivers/net/phy/phy.c
> > +++ b/drivers/net/phy/phy.c
> > @@ -1760,3 +1760,89 @@ int phy_ethtool_nway_reset(struct net_device *ndev)
> > return ret;
> > }
> > EXPORT_SYMBOL(phy_ethtool_nway_reset);
> > +
> > +/**
> > + * PHY modes in the USXGMII family can have extensions, with data transmitted
> > + * in the frame preamble.
> > + * For now, only QUSGMII is supported, but other variants like USGMII and
> > + * OUSGMII can be added in the future.
> > + */
> > +static inline bool phy_interface_has_inband_ext(phy_interface_t interface)
>
> No inline functions in .c file please. Let the compiler decide.
My bad this one slept through the cracks...
>
> > +bool phy_inband_ext_available(struct phy_device *phydev, enum phy_inband_ext ext)
> > +{
> > + return !!(phydev->inband_ext.available & ext);
>
> should this be BIT(ext) ?
Correct indeed
>
> > +}
> > +EXPORT_SYMBOL(phy_inband_ext_available);
>
> If you don't mind, i would prefer EXPORT_SYMBOL_GPL().
I don't mind, I'll fix that
>
> > +static int phy_set_inband_ext(struct phy_device *phydev,
> > + enum phy_inband_ext ext,
> > + bool enable)
> > +{
> > + int ret;
> > +
> > + if (!phy_interface_has_inband_ext(phydev->interface))
> > + return -EOPNOTSUPP;
> > +
> > + if (!phydev->drv->set_inband_ext)
> > + return -EOPNOTSUPP;
>
> That is a driver bug. It should not set phydev->inband_ext.available
> and then not have drv->set_inband_ext. So we should probably test this
> earlier. Maybe define that phydev->inband_ext.available has to be set
> during probe, and the core can validate this after probe and reject
> the device if it is inconsistent?
Good point, I'll add that !
>
> > +
> > + mutex_lock(&phydev->lock);
> > + ret = phydev->drv->set_inband_ext(phydev, ext, enable);
> > + mutex_unlock(&phydev->lock);
> > + if (ret)
> > + return ret;
> > +
> > + if (enable)
> > + phydev->inband_ext.enabled |= BIT(ext);
> > + else
> > + phydev->inband_ext.enabled &= ~BIT(ext);
>
> Should these be also protected by the mutex?
I think you are right, it would be better making sure we serialize
accesses to these indeed.
Thanks for the review,
Maxime