On Wed, May 13, 2020 at 02:06:48PM +0200, Oleksij Rempel wrote:
> The cable test seems to be support by all of currently support Atherso
> PHYs, so add support for all of them. This patch was tested only on
> AR9331 PHY with following results:
> - No cable is detected as short
> - A 15m long cable connected only on one side is detected as 9m open.
That sounds wrong. What about a shorted 15m cable? Is it also 9m? Do
you have any other long cables you can test with? Is it always 1/2 the
cable length?
> - A cable test with active link partner will provide no usable results.
>
> Signed-off-by: Oleksij Rempel <o.rem...@pengutronix.de>
> ---
> drivers/net/phy/at803x.c | 141 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 141 insertions(+)
>
> diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
> index f4fec5f644e91..03ec500defb34 100644
> --- a/drivers/net/phy/at803x.c
> +++ b/drivers/net/phy/at803x.c
> @@ -7,11 +7,13 @@
> * Author: Matus Ujhelyi <ujhely...@gmail.com>
> */
>
> +#include <linux/bitfield.h>
> #include <linux/phy.h>
> #include <linux/module.h>
> #include <linux/string.h>
> #include <linux/netdevice.h>
> #include <linux/etherdevice.h>
> +#include <linux/ethtool_netlink.h>
> #include <linux/of_gpio.h>
> #include <linux/bitfield.h>
> #include <linux/gpio/consumer.h>
> @@ -48,6 +50,20 @@
> #define AT803X_SMART_SPEED_BYPASS_TIMER BIT(1)
> #define AT803X_LED_CONTROL 0x18
>
> +/* Cable Tester Contol Register */
> +#define AT803X_CABLE_DIAG_CTRL 0x16
> +#define AT803X_CABLE_DIAG_MDI_PAIR GENMASK(9, 8)
> +#define AT803X_CABLE_DIAG_EN BIT(0)
> +
> +/* Cable Tester Status Register */
> +#define AT803X_CABLE_DIAG_STATUS 0x1c
> +#define AT803X_CABLE_DIAG_RESULT GENMASK(9, 8)
> +#define AT803X_CABLE_DIAG_RESULT_OK 0
> +#define AT803X_CABLE_DIAG_RESULT_SHORT 1
> +#define AT803X_CABLE_DIAG_RESULT_OPEN 2
> +#define AT803X_CABLE_DIAG_RESULT_FAIL 3
> +#define AT803X_CABLE_DIAG_DTIME GENMASK(7, 0)
> +
> #define AT803X_DEVICE_ADDR 0x03
> #define AT803X_LOC_MAC_ADDR_0_15_OFFSET 0x804C
> #define AT803X_LOC_MAC_ADDR_16_31_OFFSET 0x804B
> @@ -122,6 +138,7 @@ MODULE_AUTHOR("Matus Ujhelyi");
> MODULE_LICENSE("GPL");
>
> struct at803x_priv {
> + struct phy_device *phydev;
> int flags;
> #define AT803X_KEEP_PLL_ENABLED BIT(0) /* don't turn off internal PLL
> */
> u16 clk_25m_reg;
> @@ -129,6 +146,9 @@ struct at803x_priv {
> struct regulator_dev *vddio_rdev;
> struct regulator_dev *vddh_rdev;
> struct regulator *vddio;
> + struct work_struct cable_test_work;
> + bool cable_test_finished;
> + int cable_test_ret;
> };
>
> struct at803x_context {
> @@ -168,6 +188,113 @@ static int at803x_debug_reg_mask(struct phy_device
> *phydev, u16 reg,
> return phy_write(phydev, AT803X_DEBUG_DATA, val);
> }
> +static void at803x_cable_test_work(struct work_struct *work)
> +{
> + struct at803x_priv *priv = container_of(work, struct at803x_priv,
> + cable_test_work);
> + struct phy_device *phydev = priv->phydev;
> + int i, ret = 0, pairs = 4;
> +
> + if (phydev->phy_id == ATH9331_PHY_ID)
> + pairs = 2;
> +
> + for (i = 0; i < pairs; i++) {
> + ret = at803x_cable_test_pair(phydev, i);
> + if (ret)
> + break;
> + }
> +
> + priv->cable_test_ret = ret;
> + priv->cable_test_finished = true;
> +
> + phy_queue_state_machine(phydev, 0);
> +}
> +
> +static int at803x_cable_test_start(struct phy_device *phydev)
> +{
> + struct at803x_priv *priv = phydev->priv;
> +
> + if (!priv->cable_test_finished) {
> + phydev_err(phydev, "cable test is already running\n");
> + return -EIO;
> + }
> +
> + priv->cable_test_finished = false;
> + schedule_work(&priv->cable_test_work);
You don't need the work queue. You cannot spend a long time in
at803x_cable_test_start(), but you can in
at803x_cable_test_get_status(). So start the first the measurement of
the first pair in at803x_cable_test_start(), and do the rest in
of the work in at803x_cable_test_get_status().
If we see there is a common pattern of wanting
at803x_cable_test_get_status() to run immediately, not one second
later, we can change to core to support that.
Andrew
