Hi Antoine,
Feels weird to review my own patches a year later having written them,
almost nostalgic :)
The review is mostly nitpicks.
On 2020-06-19 14:22, Antoine Tenart wrote:
[...]
@@ -373,6 +374,21 @@ struct vsc8531_private {
unsigned long ingr_flows;
unsigned long egr_flows;
#endif
+
+ bool input_clk_init;
+ struct vsc85xx_ptp *ptp;
+
+ /* For multiple port PHYs; the MDIO address of the base PHY in the
+ * pair of two PHYs that share a 1588 engine. PHY0 and PHY2 are
coupled.
+ * PHY1 and PHY3 as well. PHY0 and PHY1 are base PHYs for their
+ * respective pair.
+ */
+ unsigned int ts_base_addr;
+ u8 ts_base_phy;
+
I hate myself now for this bad naming. After reading the code,
ts_base_addr is the address
of the base PHY (of a pair) on the MDIO bus and ts_base_phy is the
"internal" (package)
address of the base PHy (of a pair). This is not very explicit.
Would ts_base_phy renamed into a ts_base_pkg_addr work better?
+ /* ts_lock: used for per-PHY timestamping operations.
+ */
I don't remember exactly the comment best practices in net anymore, but
one line
comment instead?
[...]
#endif /* _MSCC_PHY_H_ */
diff --git a/drivers/net/phy/mscc/mscc_main.c
b/drivers/net/phy/mscc/mscc_main.c
index 052a0def6e83..87ddae514627 100644
--- a/drivers/net/phy/mscc/mscc_main.c
+++ b/drivers/net/phy/mscc/mscc_main.c
@@ -1299,11 +1299,29 @@ static void vsc8584_get_base_addr(struct
phy_device *phydev)
__phy_write(phydev, MSCC_EXT_PAGE_ACCESS, MSCC_PHY_PAGE_STANDARD);
mutex_unlock(&phydev->mdio.bus->mdio_lock);
- if (val & PHY_ADDR_REVERSED)
+ /* In the package, there are two pairs of PHYs (PHY0 + PHY2 and
+ * PHY1 + PHY3). The first PHY of each pair (PHY0 and PHY1) is
+ * the base PHY for timestamping operations.
+ */
+ if (val & PHY_ADDR_REVERSED) {
vsc8531->base_addr = phydev->mdio.addr + addr;
- else
+ vsc8531->ts_base_addr = phydev->mdio.addr;
+ vsc8531->ts_base_phy = addr;
+ if (addr > 1) {
+ vsc8531->ts_base_addr += 2;
+ vsc8531->ts_base_phy += 2;
+ }
+ } else {
vsc8531->base_addr = phydev->mdio.addr - addr;
+ vsc8531->ts_base_addr = phydev->mdio.addr;
+ vsc8531->ts_base_phy = addr;
The two lines above are identical in both conditions, what about moving
them just before the if (val & PHY_ADDR_REVERSED) line?
[...]
+static const u32 vsc85xx_egr_latency[] = {
+ /* Copper Egress */
+ 1272, /* 1000Mbps */
+ 12516, /* 100Mbps */
+ 125444, /* 10Mbps */
+ /* Fiber Egress */
+ 1277, /* 1000Mbps */
+ 12537, /* 100Mbps */
+ /* Copper Egress when MACsec ON */
+ 3496, /* 1000Mbps */
+ 34760, /* 100Mbps */
+ 347844, /* 10Mbps */
+ /* Fiber Egress when MACsec ON */
+ 3502, /* 1000Mbps */
+ 34780, /* 100Mbps */
+};
+
+static const u32 vsc85xx_ingr_latency[] = {
+ /* Copper Ingress */
+ 208, /* 1000Mbps */
+ 304, /* 100Mbps */
+ 2023, /* 10Mbps */
+ /* Fiber Ingress */
+ 98, /* 1000Mbps */
+ 197, /* 100Mbps */
+ /* Copper Ingress when MACsec ON */
+ 2408, /* 1000Mbps */
+ 22300, /* 100Mbps */
+ 222009, /* 10Mbps */
+ /* Fiber Ingress when MACsec ON */
+ 2299, /* 1000Mbps */
+ 22192, /* 100Mbps */
+};
+
Wouldn't it make more sense to separate the latencies into two different
arrays? One for non-MACsec and one with? No idx "hack" later in the
function
that way.
+static void vsc85xx_ts_set_latencies(struct phy_device *phydev)
+{
+ u32 val;
+ u8 idx;
+
+ /* No need to set latencies of packets if the PHY is not connected */
+ if (!phydev->link)
+ return;
+
+ vsc85xx_ts_write_csr(phydev, PROCESSOR,
MSCC_PHY_PTP_EGR_STALL_LATENCY,
+ STALL_EGR_LATENCY(phydev->speed));
+
+ switch (phydev->speed) {
+ case SPEED_100:
+ idx = 1;
+ break;
+ case SPEED_1000:
+ idx = 0;
+ break;
+ default:
+ idx = 2;
+ break;
+ }
+
+ if (IS_ENABLED(CONFIG_MACSEC))
+ idx += 5;
[...]
+static int vsc85xx_eth1_conf(struct phy_device *phydev, enum ts_blk
blk,
+ bool enable)
+{
+ struct vsc8531_private *vsc8531 = phydev->priv;
+ u32 val = ANA_ETH1_FLOW_ADDR_MATCH2_DEST;
+
+ if (vsc8531->ptp->rx_filter == HWTSTAMP_FILTER_PTP_V2_L2_EVENT) {
+ /* PTP over Ethernet multicast address for SYNC and DELAY msg */
+ u8 ptp_multicast[6] = {0x01, 0x1b, 0x19, 0x00, 0x00, 0x00};
+
I think this is actually part of "the" standard:
https://en.wikipedia.org/wiki/Precision_Time_Protocol#Message_transport
So would it make sense to make it available to all drivers via one of
the
include/linux/ptp_*.h?
[...]
+static bool vsc8584_is_1588_input_clk_configured(struct phy_device
*phydev)
+{
+ struct vsc8531_private *vsc8531 = phydev->priv;
+
+ if (vsc8531->ts_base_addr != phydev->mdio.addr) {
+ struct mdio_device *dev;
+
+ dev = phydev->mdio.bus->mdio_map[vsc8531->ts_base_addr];
+ phydev = container_of(dev, struct phy_device, mdio);
+ vsc8531 = phydev->priv;
+ }
+
+ return vsc8531->input_clk_init;
+}
+
+static void vsc8584_set_input_clk_configured(struct phy_device
*phydev)
+{
+ struct vsc8531_private *vsc8531 = phydev->priv;
+
+ if (vsc8531->ts_base_addr != phydev->mdio.addr) {
+ struct mdio_device *dev;
+
+ dev = phydev->mdio.bus->mdio_map[vsc8531->ts_base_addr];
+ phydev = container_of(dev, struct phy_device, mdio);
+ vsc8531 = phydev->priv;
+ }
+
+ vsc8531->input_clk_init = true;
+}
Duplicated code here.
Maybe:
static struct vsc8531_private * vsc8584_get_ts_base_phydev(struct
phy_device *phydev)
{
struct vsc8531_private *vsc8531 = phydev->priv;
if (vsc8531->ts_base_addr != phydev->mdio.addr) {
struct mdio_device *dev;
dev = phydev->mdio.bus->mdio_map[vsc8531->ts_base_addr];
phydev = container_of(dev, struct phy_device, mdio);
vsc8531 = phydev->priv;
}
return vsc8531;
}
?
[...]
diff --git a/drivers/net/phy/mscc/mscc_ptp.h
b/drivers/net/phy/mscc/mscc_ptp.h
[...]
+
+struct vsc85xx_ptphdr {
+ u8 tsmt; /* transportSpecific | messageType */
+ u8 ver; /* reserved0 | versionPTP */
+ __be16 msglen;
+ u8 domain;
+ u8 rsrvd1;
+ __be16 flags;
+ __be64 correction;
+ __be32 rsrvd2;
+ __be64 clk_identity;
+ __be16 src_port_id;
+ __be16 seq_id;
+ u8 ctrl;
+ u8 log_interval;
+} __attribute__((__packed__));
+
AFAICT, this is also part of "the" standard:
http://wiki.hevs.ch/uit/index.php5/Standards/Ethernet_PTP/frames#PTP_Header
Would maybe be better to have it in one of the header files in include/?
Thanks,
Quentin
