[RFC PATCH net-next 0/2] net: phy: Extend sysfs to adjust PHY latency.

[RFC PATCH net-next 0/2] net: phy: Extend sysfs to adjust PHY latency.

[Horatiu Vultur]

The previous try of setting the PHY latency was here[1]. But this approach
could not work for multiple reasons:
- the interface was not generic enough so it would be hard to be extended
  in the future
- if there were multiple time stamper in the system then it was not clear
  to which one should adjust these values.

So the next try is to extend sysfs and configure exactly the desired PHY.
Therefore add a new entry in sysfs for the PHY('adjust_latency') which
will be shown only if the PHY implements the set/get_adj_latency.

[1] https://lore.kernel.org/netdev/20220401093909.3341836-1-horatiu.vultur@microchip.com/

Horatiu Vultur (2):
  net: phy: Add phy latency adjustment support in phy framework.
  net: phy: micrel: Implement set/get_adj_latency for lan8814

 .../ABI/testing/sysfs-class-net-phydev        | 10 +++
 drivers/net/phy/micrel.c                      | 87 +++++++++++++++++++
 drivers/net/phy/phy_device.c                  | 58 +++++++++++++
 include/linux/phy.h                           |  9 ++
 4 files changed, 164 insertions(+)

-- 
2.33.0

[RFC PATCH net-next 1/2] net: phy: Add phy latency adjustment support in phy framework.

[Horatiu Vultur]

Add adjustment support for latency for the phy using sysfs.
This is used to adjust the latency of the phy based on link mode
and direction.

Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
---
 .../ABI/testing/sysfs-class-net-phydev        | 10 ++++
 drivers/net/phy/phy_device.c                  | 58 +++++++++++++++++++
 include/linux/phy.h                           |  9 +++
 3 files changed, 77 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-class-net-phydev b/Documentation/ABI/testing/sysfs-class-net-phydev
index ac722dd5e694..a99bbfeddb6f 100644
--- a/Documentation/ABI/testing/sysfs-class-net-phydev
+++ b/Documentation/ABI/testing/sysfs-class-net-phydev
@@ -63,3 +63,13 @@ Description:
 		only used internally by the kernel and their placement are
 		not meant to be stable across kernel versions. This is intended
 		for facilitating the debugging of PHY drivers.
+
+What:		/sys/class/mdio_bus/<bus>/<device>/adjust_latency
+Date:		April 2022
+KernelVersion:	5.19
+Contact:	netdev@vger.kernel.org
+Description:
+		This file adjusts the latency in the PHY. To set value,
+		write three integers into the file: interface mode, RX latency,
+		TX latency. When the file is read, it returns the supported
+		interface modes and the latency values.
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 8406ac739def..80bf04ca0e02 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -529,6 +529,48 @@ static ssize_t phy_dev_flags_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(phy_dev_flags);
 
+static ssize_t adjust_latency_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct phy_device *phydev = to_phy_device(dev);
+	ssize_t count = 0;
+	int err, i;
+	s32 rx, tx;
+
+	for (i = 0; i < __ETHTOOL_LINK_MODE_MASK_NBITS; ++i) {
+		err = phydev->drv->get_adj_latency(phydev, i, &rx, &tx);
+		if (err == -EINVAL)
+			continue;
+
+		count += sprintf(&buf[count], "%d rx: %d, tx: %d\n", i, rx, tx);
+	}
+
+	return count;
+}
+
+static ssize_t adjust_latency_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	struct phy_device *phydev = to_phy_device(dev);
+	enum ethtool_link_mode_bit_indices link_mode;
+	int cnt, err = -EINVAL;
+	s32 rx, tx;
+
+	cnt = sscanf(buf, "%u %d %d", &link_mode, &rx, &tx);
+	if (cnt != 3)
+		goto out;
+
+	err = phydev->drv->set_adj_latency(phydev, link_mode, rx, tx);
+	if (err)
+		goto out;
+
+	return count;
+out:
+	return err;
+}
+static DEVICE_ATTR_RW(adjust_latency);
+
 static struct attribute *phy_dev_attrs[] = {
 	&dev_attr_phy_id.attr,
 	&dev_attr_phy_interface.attr,
@@ -3009,6 +3051,16 @@ static int phy_probe(struct device *dev)
 
 	phydev->drv = phydrv;
 
+	if (phydev->drv &&
+	    phydev->drv->set_adj_latency &&
+	    phydev->drv->get_adj_latency) {
+		err = sysfs_create_file(&phydev->mdio.dev.kobj,
+					&dev_attr_adjust_latency.attr);
+		if (err)
+			phydev_err(phydev, "error creating 'adjust_latency' sysfs entry\n");
+		err = 0;
+	}
+
 	/* Disable the interrupt if the PHY doesn't support it
 	 * but the interrupt is still a valid one
 	 */
@@ -3112,6 +3164,12 @@ static int phy_remove(struct device *dev)
 	if (phydev->drv && phydev->drv->remove)
 		phydev->drv->remove(phydev);
 
+	if (phydev->drv &&
+	    phydev->drv->set_adj_latency &&
+	    phydev->drv->get_adj_latency)
+		sysfs_remove_file(&phydev->mdio.dev.kobj,
+				  &dev_attr_adjust_latency.attr);
+
 	/* Assert the reset signal */
 	phy_device_reset(phydev, 1);
 
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 36ca2b5c2253..584e467ff4d1 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -932,6 +932,15 @@ struct phy_driver {
 	int (*get_sqi)(struct phy_device *dev);
 	/** @get_sqi_max: Get the maximum signal quality indication */
 	int (*get_sqi_max)(struct phy_device *dev);
+	/** @set_adj_latency: Set the latency values of the PHY */
+	int (*set_adj_latency)(struct phy_device *dev,
+			       enum ethtool_link_mode_bit_indices link_mode,
+			       s32 rx, s32 tx);
+	/** @get_latency: Get the latency values of the PHY */
+	int (*get_adj_latency)(struct phy_device *dev,
+			       enum ethtool_link_mode_bit_indices link_mode,
+			       s32 *rx, s32 *tx);
+
 };
 #define to_phy_driver(d) container_of(to_mdio_common_driver(d),		\
 				      struct phy_driver, mdiodrv)
-- 
2.33.0

Re: [RFC PATCH net-next 1/2] net: phy: Add phy latency adjustment support in phy framework.

[Andrew Lunn]

On Tue, Apr 19, 2022 at 10:37:03AM +0200, Horatiu Vultur wrote:
> Add adjustment support for latency for the phy using sysfs.
> This is used to adjust the latency of the phy based on link mode
> and direction.
> 
> Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> ---
>  .../ABI/testing/sysfs-class-net-phydev        | 10 ++++
>  drivers/net/phy/phy_device.c                  | 58 +++++++++++++++++++
>  include/linux/phy.h                           |  9 +++
>  3 files changed, 77 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-class-net-phydev b/Documentation/ABI/testing/sysfs-class-net-phydev
> index ac722dd5e694..a99bbfeddb6f 100644
> --- a/Documentation/ABI/testing/sysfs-class-net-phydev
> +++ b/Documentation/ABI/testing/sysfs-class-net-phydev
> @@ -63,3 +63,13 @@ Description:
>  		only used internally by the kernel and their placement are
>  		not meant to be stable across kernel versions. This is intended
>  		for facilitating the debugging of PHY drivers.
> +
> +What:		/sys/class/mdio_bus/<bus>/<device>/adjust_latency
> +Date:		April 2022
> +KernelVersion:	5.19
> +Contact:	netdev@vger.kernel.org
> +Description:
> +		This file adjusts the latency in the PHY. To set value,
> +		write three integers into the file: interface mode, RX latency,
> +		TX latency. When the file is read, it returns the supported
> +		interface modes and the latency values.

https://lwn.net/Articles/378884/

	The key design goal relating to attribute files is the
	stipulation - almost a mantra - of "one file, one value" or
	sometimes "one item per file". The idea here is that each
	attribute file should contain precisely one value. If multiple
	values are needed, then multiple files should be used.

You also need to specify units in the documentation.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 8406ac739def..80bf04ca0e02 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -529,6 +529,48 @@ static ssize_t phy_dev_flags_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(phy_dev_flags);
>  
> +static ssize_t adjust_latency_show(struct device *dev,
> +				   struct device_attribute *attr, char *buf)
> +{
> +	struct phy_device *phydev = to_phy_device(dev);
> +	ssize_t count = 0;
> +	int err, i;
> +	s32 rx, tx;
> +
> +	for (i = 0; i < __ETHTOOL_LINK_MODE_MASK_NBITS; ++i) {
> +		err = phydev->drv->get_adj_latency(phydev, i, &rx, &tx);
> +		if (err == -EINVAL)

-EOPNOTSUPP seems more likley than EINVAL.

> +			continue;
> +
> +		count += sprintf(&buf[count], "%d rx: %d, tx: %d\n", i, rx, tx);

Link modes as a number? Can you tell me off the top of you head what
link mode 42 is?

Also, if phydev->drv->get_adj_latency() returns any other error, it is
likely rx and tx contain rubbish, yet you still add them to the
output.

> @@ -932,6 +932,15 @@ struct phy_driver {
>  	int (*get_sqi)(struct phy_device *dev);
>  	/** @get_sqi_max: Get the maximum signal quality indication */
>  	int (*get_sqi_max)(struct phy_device *dev);
> +	/** @set_adj_latency: Set the latency values of the PHY */
> +	int (*set_adj_latency)(struct phy_device *dev,
> +			       enum ethtool_link_mode_bit_indices link_mode,
> +			       s32 rx, s32 tx);
> +	/** @get_latency: Get the latency values of the PHY */
> +	int (*get_adj_latency)(struct phy_device *dev,
> +			       enum ethtool_link_mode_bit_indices link_mode,
> +			       s32 *rx, s32 *tx);

You need to clearly document the return value here, that -EINVAL (or
maybe -EOPNOTUSPP) has special meaning. I would also document the
units, and what is supposed to happen if the stamper already has a
hard coded offset.

     Andrew

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

[Horatiu Vultur]

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);
+		return 0;
+	case ETHTOOL_LINK_MODE_100baseT_Half_BIT:
+	case ETHTOOL_LINK_MODE_100baseT_Full_BIT:
+		rx += LAN8814_RX_100_LATENCY;
+		tx += LAN8814_TX_100_LATENCY;
+		lanphy_write_page_reg(phydev, 5, PTP_RX_LATENCY_100, rx);
+		lanphy_write_page_reg(phydev, 5, PTP_TX_LATENCY_100, tx);
+		return 0;
+	case ETHTOOL_LINK_MODE_1000baseT_Half_BIT:
+	case ETHTOOL_LINK_MODE_1000baseT_Full_BIT:
+		rx += LAN8814_RX_1000_LATENCY;
+		tx += LAN8814_TX_1000_LATENCY;
+		lanphy_write_page_reg(phydev, 5, PTP_RX_LATENCY_1000, rx);
+		lanphy_write_page_reg(phydev, 5, PTP_TX_LATENCY_1000, tx);
+		return 0;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int lan8814_get_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 = lanphy_read_page_reg(phydev, 5, PTP_RX_LATENCY_10);
+		*tx = lanphy_read_page_reg(phydev, 5, PTP_TX_LATENCY_10);
+		*rx -= LAN8814_RX_10_LATENCY;
+		*tx -= LAN8814_TX_10_LATENCY;
+		return 0;
+	case ETHTOOL_LINK_MODE_100baseT_Half_BIT:
+	case ETHTOOL_LINK_MODE_100baseT_Full_BIT:
+		*rx = lanphy_read_page_reg(phydev, 5, PTP_RX_LATENCY_100);
+		*tx = lanphy_read_page_reg(phydev, 5, PTP_TX_LATENCY_100);
+		*rx -= LAN8814_RX_100_LATENCY;
+		*tx -= LAN8814_TX_100_LATENCY;
+		return 0;
+	case ETHTOOL_LINK_MODE_1000baseT_Half_BIT:
+	case ETHTOOL_LINK_MODE_1000baseT_Full_BIT:
+		*rx = lanphy_read_page_reg(phydev, 5, PTP_RX_LATENCY_1000);
+		*tx = lanphy_read_page_reg(phydev, 5, PTP_TX_LATENCY_1000);
+		*rx -= LAN8814_RX_1000_LATENCY;
+		*tx -= LAN8814_TX_1000_LATENCY;
+		return 0;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static irqreturn_t lan8814_handle_interrupt(struct phy_device *phydev)
 {
 	u16 tsu_irq_status;
@@ -3052,6 +3137,8 @@ static struct phy_driver ksphy_driver[] = {
 	.resume		= kszphy_resume,
 	.config_intr	= lan8814_config_intr,
 	.handle_interrupt = lan8814_handle_interrupt,
+	.set_adj_latency = lan8814_set_adj_latency,
+	.get_adj_latency = lan8814_get_adj_latency,
 }, {
 	.phy_id		= PHY_ID_LAN8804,
 	.phy_id_mask	= MICREL_PHY_ID_MASK,
-- 
2.33.0

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

[Andrew Lunn]

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

Re: [RFC PATCH net-next 0/2] net: phy: Extend sysfs to adjust PHY latency.

[Andrew Lunn]

On Tue, Apr 19, 2022 at 10:37:02AM +0200, Horatiu Vultur wrote:
> The previous try of setting the PHY latency was here[1]. But this approach
> could not work for multiple reasons:
> - the interface was not generic enough so it would be hard to be extended
>   in the future
> - if there were multiple time stamper in the system then it was not clear
>   to which one should adjust these values.
> 
> So the next try is to extend sysfs and configure exactly the desired PHY.

What about timestampers which are not PHYs? Ideally you want one
interface which will work for any sort of stamper, be it MAC, PHY, or
a bump in the wire between the MAC and the PHY.

  Andrew

Re: [RFC PATCH net-next 0/2] net: phy: Extend sysfs to adjust PHY latency.

[Horatiu Vultur]

The 04/19/2022 14:17, Andrew Lunn wrote:

Hi Andrew,

> 
> On Tue, Apr 19, 2022 at 10:37:02AM +0200, Horatiu Vultur wrote:
> > The previous try of setting the PHY latency was here[1]. But this approach
> > could not work for multiple reasons:
> > - the interface was not generic enough so it would be hard to be extended
> >   in the future
> > - if there were multiple time stamper in the system then it was not clear
> >   to which one should adjust these values.
> >
> > So the next try is to extend sysfs and configure exactly the desired PHY.
> 
> What about timestampers which are not PHYs? Ideally you want one
> interface which will work for any sort of stamper, be it MAC, PHY, or
> a bump in the wire between the MAC and the PHY.

My initial idea was that each of the timestampers will need to extend
the sysfs to add this file or multiple files. But from what I can see
this approach will not fly.

If we want an interface to be used by any sort of stamper, we could
create a new generic class (eth_tunable/eth_obj). Then each driver will
register a device that will use this generic class.
We will continue to use sysfs to expose all the modes supported by the
driver. But this time create a file for each mode.
The entire approach should be something similar with the ptp clocks.
What do you think, is this the right approach?

> 
>   Andrew

-- 
/Horatiu