Re: [RFC PATCH net-next 2/2] net: phy: micrel: Implement set/get_adj_latency for lan8814

[Andrew Lunn <andrew@lunn.ch>]

On Tue, Apr 19, 2022 at 10:37:04AM +0200, Horatiu Vultur wrote:
> The lan8814 driver supports adjustments of the latency in the silicon
> based on the speed and direction, therefore implement set/get_adj_latency
> to adjust the HW.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  drivers/net/phy/micrel.c | 87 ++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 87 insertions(+)
> 
> diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
> index 96840695debd..099d1ecd6dad 100644
> --- a/drivers/net/phy/micrel.c
> +++ b/drivers/net/phy/micrel.c
> @@ -120,6 +120,15 @@
>  #define PTP_TIMESTAMP_EN_PDREQ_			BIT(2)
>  #define PTP_TIMESTAMP_EN_PDRES_			BIT(3)
>  
> +#define PTP_RX_LATENCY_1000			0x0224
> +#define PTP_TX_LATENCY_1000			0x0225
> +
> +#define PTP_RX_LATENCY_100			0x0222
> +#define PTP_TX_LATENCY_100			0x0223
> +
> +#define PTP_RX_LATENCY_10			0x0220
> +#define PTP_TX_LATENCY_10			0x0221
> +
>  #define PTP_TX_PARSE_L2_ADDR_EN			0x0284
>  #define PTP_RX_PARSE_L2_ADDR_EN			0x0244
>  
> @@ -208,6 +217,16 @@
>  #define PTP_TSU_INT_STS_PTP_RX_TS_OVRFL_INT_	BIT(1)
>  #define PTP_TSU_INT_STS_PTP_RX_TS_EN_		BIT(0)
>  
> +/* Represents the reset value of the latency registers,
> + * The values are express in ns
> + */
> +#define LAN8814_RX_10_LATENCY			8874
> +#define LAN8814_TX_10_LATENCY			11850
> +#define LAN8814_RX_100_LATENCY			2346
> +#define LAN8814_TX_100_LATENCY			705
> +#define LAN8814_RX_1000_LATENCY			429
> +#define LAN8814_TX_1000_LATENCY			201
> +
>  /* PHY Control 1 */
>  #define MII_KSZPHY_CTRL_1			0x1e
>  #define KSZ8081_CTRL1_MDIX_STAT			BIT(4)
> @@ -2657,6 +2676,72 @@ static int lan8804_config_init(struct phy_device *phydev)
>  	return 0;
>  }
>  
> +static int lan8814_set_adj_latency(struct phy_device *phydev,
> +				   enum ethtool_link_mode_bit_indices link_mode,
> +				   s32 rx, s32 tx)
> +{
> +	switch (link_mode) {
> +	case ETHTOOL_LINK_MODE_10baseT_Half_BIT:
> +	case ETHTOOL_LINK_MODE_10baseT_Full_BIT:
> +		rx += LAN8814_RX_10_LATENCY;
> +		tx += LAN8814_TX_10_LATENCY;
> +		lanphy_write_page_reg(phydev, 5, PTP_RX_LATENCY_10, rx);
> +		lanphy_write_page_reg(phydev, 5, PTP_TX_LATENCY_10, tx);

It is not ideal that the user sees an entry for both link modes X and
X+1 in your file, and that writing to link mode X magically also
changes X+1.

I'm not sure there is anything you can do about this in a generic
implementation, so you at least need to document it in sysfs.

What about range checks? I can pass 32764 as an rx delay, which when
added to PTP_RX_LATENCY_10=0x0220 is going to wrap around and be
negative.

		Andrew