| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAFXsbZohCG5OScjAszD5vpMacfUEUYK_74FU1tjz4Sm8nbegsg@mail.gmail.com>
Date: Sun, 17 May 2020 13:25:23 -0700
From: Chris Healy <cphealy@...il.com>
To: Andrew Lunn <andrew@...n.ch>
Cc: David Miller <davem@...emloft.net>,
netdev <netdev@...r.kernel.org>,
Florian Fainelli <f.fainelli@...il.com>,
Heiner Kallweit <hkallweit1@...il.com>,
Michal Kubecek <mkubecek@...e.cz>
Subject: Re: [PATCH net-next 4/7] net: phy: marvell: Add support for amplitude graph
On Sun, May 17, 2020 at 12:59 PM Andrew Lunn <andrew@...n.ch> wrote:
>
> The Marvell PHYs can measure the amplitude of the returned signal for
> a given distance. Implement this option of the cable test
> infrastructure.
>
> Signed-off-by: Andrew Lunn <andrew@...n.ch>
> ---
> drivers/net/phy/marvell.c | 227 +++++++++++++++++++++++++++++++++++++-
> 1 file changed, 226 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index 4bc7febf9248..e7994f5b506e 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -42,6 +42,7 @@
> #define MII_MARVELL_FIBER_PAGE 0x01
> #define MII_MARVELL_MSCR_PAGE 0x02
> #define MII_MARVELL_LED_PAGE 0x03
> +#define MII_MARVELL_VCT5_PAGE 0x05
> #define MII_MARVELL_MISC_TEST_PAGE 0x06
> #define MII_MARVELL_VCT7_PAGE 0x07
> #define MII_MARVELL_WOL_PAGE 0x11
> @@ -164,6 +165,54 @@
> #define MII_88E1510_GEN_CTRL_REG_1_MODE_SGMII 0x1 /* SGMII to copper */
> #define MII_88E1510_GEN_CTRL_REG_1_RESET 0x8000 /* Soft reset */
>
> +#define MII_VCT5_TX_RX_MDI0_COUPLING 0x10
> +#define MII_VCT5_TX_RX_MDI1_COUPLING 0x11
> +#define MII_VCT5_TX_RX_MDI2_COUPLING 0x12
> +#define MII_VCT5_TX_RX_MDI3_COUPLING 0x13
> +#define MII_VCT5_TX_RX_AMPLITUDE_MASK 0x7f00
> +#define MII_VCT5_TX_RX_AMPLITUDE_SHIFT 8
> +#define MII_VCT5_TX_RX_COUPLING_POSITIVE_REFLECTION BIT(15)
> +
> +#define MII_VCT5_CTRL 0x17
> +#define MII_VCT5_CTRL_ENABLE BIT(15)
> +#define MII_VCT5_CTRL_COMPLETE BIT(14)
> +#define MII_VCT5_CTRL_TX_SAME_CHANNEL (0x0 << 11)
> +#define MII_VCT5_CTRL_TX0_CHANNEL (0x4 << 11)
> +#define MII_VCT5_CTRL_TX1_CHANNEL (0x5 << 11)
> +#define MII_VCT5_CTRL_TX2_CHANNEL (0x6 << 11)
> +#define MII_VCT5_CTRL_TX3_CHANNEL (0x7 << 11)
> +#define MII_VCT5_CTRL_SAMPLES_2 (0x0 << 8)
> +#define MII_VCT5_CTRL_SAMPLES_4 (0x1 << 8)
> +#define MII_VCT5_CTRL_SAMPLES_8 (0x2 << 8)
> +#define MII_VCT5_CTRL_SAMPLES_16 (0x3 << 8)
> +#define MII_VCT5_CTRL_SAMPLES_32 (0x4 << 8)
> +#define MII_VCT5_CTRL_SAMPLES_64 (0x5 << 8)
> +#define MII_VCT5_CTRL_SAMPLES_128 (0x6 << 8)
> +#define MII_VCT5_CTRL_SAMPLES_DEFAULT (0x6 << 8)
> +#define MII_VCT5_CTRL_SAMPLES_256 (0x7 << 8)
> +#define MII_VCT5_CTRL_SAMPLES_SHIFT 8
> +#define MII_VCT5_CTRL_MODE_MAXIMUM_PEEK (0x0 << 6)
> +#define MII_VCT5_CTRL_MODE_FIRST_LAST_PEEK (0x1 << 6)
> +#define MII_VCT5_CTRL_MODE_OFFSET (0x2 << 6)
> +#define MII_VCT5_CTRL_SAMPLE_POINT (0x3 << 6)
> +#define MII_VCT5_CTRL_PEEK_HYST_DEFAULT 3
> +
> +#define MII_VCT5_SAMPLE_POINT_DISTANCE 0x18
> +#define MII_VCT5_TX_PULSE_CTRL 0x1c
> +#define MII_VCT5_TX_PULSE_CTRL_DONT_WAIT_LINK_DOWN BIT(12)
> +#define MII_VCT5_TX_PULSE_CTRL_PULSE_WIDTH_128nS (0x0 << 10)
> +#define MII_VCT5_TX_PULSE_CTRL_PULSE_WIDTH_96nS (0x1 << 10)
> +#define MII_VCT5_TX_PULSE_CTRL_PULSE_WIDTH_64nS (0x2 << 10)
> +#define MII_VCT5_TX_PULSE_CTRL_PULSE_WIDTH_32nS (0x3 << 10)
> +#define MII_VCT5_TX_PULSE_CTRL_PULSE_WIDTH_SHIFT 10
> +#define MII_VCT5_TX_PULSE_CTRL_PULSE_AMPLITUDE_1000mV (0x0 << 8)
> +#define MII_VCT5_TX_PULSE_CTRL_PULSE_AMPLITUDE_750mV (0x1 << 8)
> +#define MII_VCT5_TX_PULSE_CTRL_PULSE_AMPLITUDE_500mV (0x2 << 8)
> +#define MII_VCT5_TX_PULSE_CTRL_PULSE_AMPLITUDE_250mV (0x3 << 8)
> +#define MII_VCT5_TX_PULSE_CTRL_PULSE_AMPLITUDE_SHIFT 8
> +#define MII_VCT5_TX_PULSE_CTRL_MAX_AMP BIT(7)
> +#define MII_VCT5_TX_PULSE_CTRL_GT_140m_46_86mV (0x6 << 0)
> +
> #define MII_VCT7_PAIR_0_DISTANCE 0x10
> #define MII_VCT7_PAIR_1_DISTANCE 0x11
> #define MII_VCT7_PAIR_2_DISTANCE 0x12
> @@ -220,6 +269,7 @@ struct marvell_priv {
> u64 stats[ARRAY_SIZE(marvell_hw_stats)];
> char *hwmon_name;
> struct device *hwmon_dev;
> + bool cable_test_tdr;
> };
>
> static int marvell_read_page(struct phy_device *phydev)
> @@ -1690,7 +1740,117 @@ static void marvell_get_stats(struct phy_device *phydev,
> data[i] = marvell_get_stat(phydev, i);
> }
>
> -static int marvell_vct7_cable_test_start(struct phy_device *phydev)
> +static int marvell_vct5_wait_complete(struct phy_device *phydev)
> +{
> + int i;
> + int val;
> +
> + for (i = 0; i < 32; i++) {
> + val = phy_read_paged(phydev, MII_MARVELL_VCT5_PAGE,
> + MII_VCT5_CTRL);
> + if (val < 0)
> + return val;
> +
> + if (val & MII_VCT5_CTRL_COMPLETE)
> + return 0;
> +
> + usleep_range(1000, 2000);
> + }
> +
> + phydev_err(phydev, "Timeout while waiting for cable test to finish\n");
> + return -ETIMEDOUT;
> +}
> +
> +static int marvell_vct5_amplitude(struct phy_device *phydev, int pair)
> +{
> + int amplitude;
> + int val;
> + int reg;
> +
> + reg = MII_VCT5_TX_RX_MDI0_COUPLING + pair;
> + val = phy_read_paged(phydev, MII_MARVELL_VCT5_PAGE, reg);
> +
> + if (val < 0)
> + return 0;
> +
> + amplitude = (val & MII_VCT5_TX_RX_AMPLITUDE_MASK) >>
> + MII_VCT5_TX_RX_AMPLITUDE_SHIFT;
> +
> + if (!(val & MII_VCT5_TX_RX_COUPLING_POSITIVE_REFLECTION))
> + amplitude = -amplitude;
> +
> + return 1000 * amplitude / 128;
> +}
> +
> +static int marvell_vct5_amplitude_distance(struct phy_device *phydev,
> + int meters)
> +{
> + int mV_pair0, mV_pair1, mV_pair2, mV_pair3;
> + int distance;
> + u16 reg;
> + int err;
> +
> + distance = meters * 1000 / 805;
With this integer based meters representation, it seems to me that we
are artificially reducing the resolution of the distance sampling.
For a 100 meter cable, the Marvell implementation looks to support 124
sample points. This could result in incorrect data reporting as two
adjacent meter numbers would resolve to the same disatance value
entered into the register. (eg - 2 meters = 2 distance 3 meters = 2
distance)
Is there a better way of doing this which would allow for userspace to
use the full resolution of the hardware?
> +
> + err = phy_write_paged(phydev, MII_MARVELL_VCT5_PAGE,
> + MII_VCT5_SAMPLE_POINT_DISTANCE,
> + distance);
> + if (err)
> + return err;
> +
> + reg = MII_VCT5_CTRL_ENABLE |
> + MII_VCT5_CTRL_TX_SAME_CHANNEL |
> + MII_VCT5_CTRL_SAMPLES_DEFAULT |
> + MII_VCT5_CTRL_SAMPLE_POINT |
> + MII_VCT5_CTRL_PEEK_HYST_DEFAULT;
> + err = phy_write_paged(phydev, MII_MARVELL_VCT5_PAGE,
> + MII_VCT5_CTRL, reg);
> + if (err)
> + return err;
> +
> + err = marvell_vct5_wait_complete(phydev);
> + if (err)
> + return err;
> +
> + mV_pair0 = marvell_vct5_amplitude(phydev, 0);
> + mV_pair1 = marvell_vct5_amplitude(phydev, 1);
> + mV_pair2 = marvell_vct5_amplitude(phydev, 2);
> + mV_pair3 = marvell_vct5_amplitude(phydev, 3);
> +
> + ethnl_cable_test_amplitude(phydev, ETHTOOL_A_CABLE_PAIR_A, mV_pair0);
> + ethnl_cable_test_amplitude(phydev, ETHTOOL_A_CABLE_PAIR_B, mV_pair1);
> + ethnl_cable_test_amplitude(phydev, ETHTOOL_A_CABLE_PAIR_C, mV_pair2);
> + ethnl_cable_test_amplitude(phydev, ETHTOOL_A_CABLE_PAIR_D, mV_pair3);
> +
> + return 0;
> +}
> +
> +static int marvell_vct5_amplitude_graph(struct phy_device *phydev)
> +{
> + int meters;
> + int err;
> + u16 reg;
> +
> + reg = MII_VCT5_TX_PULSE_CTRL_GT_140m_46_86mV |
> + MII_VCT5_TX_PULSE_CTRL_DONT_WAIT_LINK_DOWN |
> + MII_VCT5_TX_PULSE_CTRL_MAX_AMP |
> + MII_VCT5_TX_PULSE_CTRL_PULSE_WIDTH_32nS;
> +
> + err = phy_write_paged(phydev, MII_MARVELL_VCT5_PAGE,
> + MII_VCT5_TX_PULSE_CTRL, reg);
> + if (err)
> + return err;
> +
> + for (meters = 0; meters <= 100; meters++) {
> + err = marvell_vct5_amplitude_distance(phydev, meters);
> + if (err)
> + return err;
> + }
> +
> + return 0;
> +}
> +
> +static int marvell_cable_test_start_common(struct phy_device *phydev)
> {
> int bmcr, bmsr, ret;
>
> @@ -1719,12 +1879,66 @@ static int marvell_vct7_cable_test_start(struct phy_device *phydev)
> if (bmsr & BMSR_LSTATUS)
> msleep(1500);
>
> + return 0;
> +}
> +
> +static int marvell_vct7_cable_test_start(struct phy_device *phydev)
> +{
> + struct marvell_priv *priv = phydev->priv;
> + int ret;
> +
> + ret = marvell_cable_test_start_common(phydev);
> + if (ret)
> + return ret;
> +
> + priv->cable_test_tdr = false;
> +
> + /* Reset the VCT5 API control to defaults, otherwise
> + * VCT7 does not work correctly.
> + */
> + ret = phy_write_paged(phydev, MII_MARVELL_VCT5_PAGE,
> + MII_VCT5_CTRL,
> + MII_VCT5_CTRL_TX_SAME_CHANNEL |
> + MII_VCT5_CTRL_SAMPLES_DEFAULT |
> + MII_VCT5_CTRL_MODE_MAXIMUM_PEEK |
> + MII_VCT5_CTRL_PEEK_HYST_DEFAULT);
> + if (ret)
> + return ret;
> +
> + ret = phy_write_paged(phydev, MII_MARVELL_VCT5_PAGE,
> + MII_VCT5_SAMPLE_POINT_DISTANCE, 0);
> + if (ret)
> + return ret;
> +
> return phy_write_paged(phydev, MII_MARVELL_VCT7_PAGE,
> MII_VCT7_CTRL,
> MII_VCT7_CTRL_RUN_NOW |
> MII_VCT7_CTRL_CENTIMETERS);
> }
>
> +static int marvell_vct5_cable_test_tdr_start(struct phy_device *phydev)
> +{
> + struct marvell_priv *priv = phydev->priv;
> + int ret;
> +
> + /* Disable VCT7 */
> + ret = phy_write_paged(phydev, MII_MARVELL_VCT7_PAGE,
> + MII_VCT7_CTRL, 0);
> + if (ret)
> + return ret;
> +
> + ret = marvell_cable_test_start_common(phydev);
> + if (ret)
> + return ret;
> +
> + priv->cable_test_tdr = true;
> + ret = ethnl_cable_test_pulse(phydev, 1000);
> + if (ret)
> + return ret;
> +
> + return ethnl_cable_test_step(phydev, 0, 100, 1);
> +}
> +
> static int marvell_vct7_distance_to_length(int distance, bool meter)
> {
> if (meter)
> @@ -1828,8 +2042,15 @@ static int marvell_vct7_cable_test_report(struct phy_device *phydev)
> static int marvell_vct7_cable_test_get_status(struct phy_device *phydev,
> bool *finished)
> {
> + struct marvell_priv *priv = phydev->priv;
> int ret;
>
> + if (priv->cable_test_tdr) {
> + ret = marvell_vct5_amplitude_graph(phydev);
> + *finished = true;
> + return ret;
> + }
> +
> *finished = false;
>
> ret = phy_read_paged(phydev, MII_MARVELL_VCT7_PAGE,
> @@ -2563,6 +2784,7 @@ static struct phy_driver marvell_drivers[] = {
> .get_tunable = m88e1011_get_tunable,
> .set_tunable = m88e1011_set_tunable,
> .cable_test_start = marvell_vct7_cable_test_start,
> + .cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
> .cable_test_get_status = marvell_vct7_cable_test_get_status,
> },
> {
> @@ -2588,6 +2810,7 @@ static struct phy_driver marvell_drivers[] = {
> .get_tunable = m88e1540_get_tunable,
> .set_tunable = m88e1540_set_tunable,
> .cable_test_start = marvell_vct7_cable_test_start,
> + .cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
> .cable_test_get_status = marvell_vct7_cable_test_get_status,
> },
> {
> @@ -2613,6 +2836,7 @@ static struct phy_driver marvell_drivers[] = {
> .get_tunable = m88e1540_get_tunable,
> .set_tunable = m88e1540_set_tunable,
> .cable_test_start = marvell_vct7_cable_test_start,
> + .cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
> .cable_test_get_status = marvell_vct7_cable_test_get_status,
> },
> {
> @@ -2658,6 +2882,7 @@ static struct phy_driver marvell_drivers[] = {
> .get_tunable = m88e1540_get_tunable,
> .set_tunable = m88e1540_set_tunable,
> .cable_test_start = marvell_vct7_cable_test_start,
> + .cable_test_tdr_start = marvell_vct5_cable_test_tdr_start,
> .cable_test_get_status = marvell_vct7_cable_test_get_status,
> },
> };
> --
> 2.26.2
>
Powered by blists - more mailing lists