Re: [PATCH net-next v3 5/8] net: phy: mscc: 1588 block initialization

[Quentin Schulz <foss@0leil.net>]

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