LinuxLists.cc - [PATCH net-next 0/5] Various improvements to Microsemi PHY driver

[permalink] [raw]

Subject: [PATCH net-next 5/5] net: phy: mscc: remove unneeded temporary variable

Here, the rc variable is either used only for the condition right after
the assignment or right before being used as the return value of the
function it's being used in.

So let's remove this unneeded temporary variable whenever possible.

Signed-off-by: Quentin Schulz <[email protected]>
---
drivers/net/phy/mscc.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index efa9352..24f4754 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -199,10 +199,7 @@ static const struct vsc8531_edge_rate_table edge_table[] = {

static int vsc85xx_phy_page_set(struct phy_device *phydev, u16 page)
{
- int rc;
-
- rc = phy_write(phydev, MSCC_EXT_PAGE_ACCESS, page);
- return rc;
+ return phy_write(phydev, MSCC_EXT_PAGE_ACCESS, page);
}

static int vsc85xx_get_sset_count(struct phy_device *phydev)
@@ -504,7 +501,7 @@ static void vsc85xx_wol_get(struct phy_device *phydev,
static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev)
{
u32 vdd, sd;
- int rc, i, j;
+ int i, j;
struct device *dev = &phydev->mdio.dev;
struct device_node *of_node = dev->of_node;
u8 sd_array_size = ARRAY_SIZE(edge_table[0].slowdown);
@@ -512,12 +509,10 @@ static int vsc85xx_edge_rate_magic_get(struct phy_device *phydev)
if (!of_node)
return -ENODEV;

- rc = of_property_read_u32(of_node, "vsc8531,vddmac", &vdd);
- if (rc != 0)
+ if (of_property_read_u32(of_node, "vsc8531,vddmac", &vdd))
vdd = MSCC_VDDMAC_3300;

- rc = of_property_read_u32(of_node, "vsc8531,edge-slowdown", &sd);
- if (rc != 0)
+ if (of_property_read_u32(of_node, "vsc8531,edge-slowdown", &sd))
sd = 0;

for (i = 0; i < ARRAY_SIZE(edge_table); i++)
@@ -762,9 +757,7 @@ static int vsc85xx_config_init(struct phy_device *phydev)
return rc;
}

- rc = genphy_config_init(phydev);
-
- return rc;
+ return genphy_config_init(phydev);
}

static int vsc85xx_ack_interrupt(struct phy_device *phydev)
--
git-series 0.9.1

2018-09-14 08:35:21

[permalink] [raw]

Subject: [PATCH net-next 4/5] net: phy: mscc: shorten `x != 0` condition to `x`

`if (x != 0)` is basically a more verbose version of `if (x)` so let's
use the latter so it's consistent throughout the whole driver.

Signed-off-by: Quentin Schulz <[email protected]>
---
drivers/net/phy/mscc.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index 734d9fb..efa9352 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -311,11 +311,11 @@ static int vsc85xx_mdix_set(struct phy_device *phydev, u8 mdix)
DISABLE_HP_AUTO_MDIX_MASK);
}
rc = phy_write(phydev, MSCC_PHY_BYPASS_CONTROL, reg_val);
- if (rc != 0)
+ if (rc)
return rc;

rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED);
- if (rc != 0)
+ if (rc)
return rc;

reg_val = phy_read(phydev, MSCC_PHY_EXT_MODE_CNTL);
@@ -325,11 +325,11 @@ static int vsc85xx_mdix_set(struct phy_device *phydev, u8 mdix)
else if (mdix == ETH_TP_MDI_X)
reg_val |= FORCE_MDI_CROSSOVER_MDIX;
rc = phy_write(phydev, MSCC_PHY_EXT_MODE_CNTL, reg_val);
- if (rc != 0)
+ if (rc)
return rc;

rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
- if (rc != 0)
+ if (rc)
return rc;

return genphy_restart_aneg(phydev);
@@ -341,7 +341,7 @@ static int vsc85xx_downshift_get(struct phy_device *phydev, u8 *count)
u16 reg_val;

rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED);
- if (rc != 0)
+ if (rc)
goto out;

reg_val = phy_read(phydev, MSCC_PHY_ACTIPHY_CNTL);
@@ -373,14 +373,14 @@ static int vsc85xx_downshift_set(struct phy_device *phydev, u8 count)
}

rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED);
- if (rc != 0)
+ if (rc)
goto out;

reg_val = phy_read(phydev, MSCC_PHY_ACTIPHY_CNTL);
reg_val &= ~(DOWNSHIFT_CNTL_MASK);
reg_val |= count;
rc = phy_write(phydev, MSCC_PHY_ACTIPHY_CNTL, reg_val);
- if (rc != 0)
+ if (rc)
goto out;

rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
@@ -401,7 +401,7 @@ static int vsc85xx_wol_set(struct phy_device *phydev,

mutex_lock(&phydev->lock);
rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
- if (rc != 0)
+ if (rc)
goto out_unlock;

if (wol->wolopts & WAKE_MAGIC) {
@@ -439,7 +439,7 @@ static int vsc85xx_wol_set(struct phy_device *phydev,
phy_write(phydev, MSCC_PHY_WOL_MAC_CONTROL, reg_val);

rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
- if (rc != 0)
+ if (rc)
goto out_unlock;

if (wol->wolopts & WAKE_MAGIC) {
@@ -447,14 +447,14 @@ static int vsc85xx_wol_set(struct phy_device *phydev,
reg_val = phy_read(phydev, MII_VSC85XX_INT_MASK);
reg_val |= MII_VSC85XX_INT_MASK_WOL;
rc = phy_write(phydev, MII_VSC85XX_INT_MASK, reg_val);
- if (rc != 0)
+ if (rc)
goto out_unlock;
} else {
/* Disable the WOL interrupt */
reg_val = phy_read(phydev, MII_VSC85XX_INT_MASK);
reg_val &= (~MII_VSC85XX_INT_MASK_WOL);
rc = phy_write(phydev, MII_VSC85XX_INT_MASK, reg_val);
- if (rc != 0)
+ if (rc)
goto out_unlock;
}
/* Clear WOL iterrupt status */
@@ -595,13 +595,13 @@ static int vsc85xx_edge_rate_cntl_set(struct phy_device *phydev, u8 edge_rate)

mutex_lock(&phydev->lock);
rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
- if (rc != 0)
+ if (rc)
goto out_unlock;
reg_val = phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
reg_val &= ~(EDGE_RATE_CNTL_MASK);
reg_val |= (edge_rate << EDGE_RATE_CNTL_POS);
rc = phy_write(phydev, MSCC_PHY_WOL_MAC_CONTROL, reg_val);
- if (rc != 0)
+ if (rc)
goto out_unlock;
rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);

@@ -636,7 +636,7 @@ static int vsc85xx_mac_if_set(struct phy_device *phydev,
goto out_unlock;
}
rc = phy_write(phydev, MSCC_PHY_EXT_PHY_CNTL_1, reg_val);
- if (rc != 0)
+ if (rc)
goto out_unlock;

rc = genphy_soft_reset(phydev);
@@ -655,7 +655,7 @@ static int vsc85xx_default_config(struct phy_device *phydev)
phydev->mdix_ctrl = ETH_TP_MDI_AUTO;
mutex_lock(&phydev->lock);
rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
- if (rc != 0)
+ if (rc)
goto out_unlock;

reg_val = phy_read(phydev, MSCC_PHY_RGMII_CNTL);
--
git-series 0.9.1

2018-09-14 08:35:23

[permalink] [raw]

Subject: [PATCH net-next 2/5] net: phy: mscc: Add EEE init sequence

From: Raju Lakkaraju <[email protected]>

Microsemi PHYs (VSC 8530/31/40/41) need to update the Energy Efficient
Ethernet initialization sequence.
In order to avoid certain link state errors that could result in link
drops and packet loss, the physical coding sublayer (PCS) must be
updated with settings related to EEE in order to improve performance.

Signed-off-by: Raju Lakkaraju <[email protected]>
Signed-off-by: Quentin Schulz <[email protected]>
---
drivers/net/phy/mscc.c | 54 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 54 insertions(+)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index 62d6e0a..c0a9ea9 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -67,6 +67,7 @@ enum rgmii_rx_clock_delay {
#define MSCC_PHY_PAGE_STANDARD 0x0000 /* Standard registers */
#define MSCC_PHY_PAGE_EXTENDED 0x0001 /* Extended registers */
#define MSCC_PHY_PAGE_EXTENDED_2 0x0002 /* Extended reg - page 2 */
+#define MSCC_PHY_PAGE_TR 0x52b5 /* Token ring registers */

/* Extended Page 1 Registers */
#define MSCC_PHY_CU_MEDIA_CRC_VALID_CNT 18
@@ -100,6 +101,13 @@ enum rgmii_rx_clock_delay {
#define SECURE_ON_ENABLE 0x8000
#define SECURE_ON_PASSWD_LEN_4 0x4000

+/* Token ring page Registers */
+#define MSCC_PHY_TR_CNTL 16
+#define TR_WRITE 0x8000
+#define TR_ADDR(x) (0x7fff & (x))
+#define MSCC_PHY_TR_LSB 17
+#define MSCC_PHY_TR_MSB 18
+
/* Microsemi PHY ID's */
#define PHY_ID_VSC8530 0x00070560
#define PHY_ID_VSC8531 0x00070570
@@ -685,6 +693,48 @@ static int vsc85xx_set_tunable(struct phy_device *phydev,
}
}

+static void vsc85xx_tr_write(struct phy_device *phydev, u16 addr, u32 val)
+{
+ phy_write(phydev, MSCC_PHY_TR_MSB, val >> 16);
+ phy_write(phydev, MSCC_PHY_TR_LSB, val & GENMASK(15, 0));
+ phy_write(phydev, MSCC_PHY_TR_CNTL, TR_WRITE | TR_ADDR(addr));
+}
+
+static int vsc85xx_eee_init_seq_set(struct phy_device *phydev)
+{
+ int rc;
+
+ mutex_lock(&phydev->lock);
+ rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_TR);
+ if (rc)
+ goto out_unlock;
+
+ vsc85xx_tr_write(phydev, 0x0f82, 0x0012b00a);
+ vsc85xx_tr_write(phydev, 0x1686, 0x00000004);
+ vsc85xx_tr_write(phydev, 0x168c, 0x00d2c46f);
+ vsc85xx_tr_write(phydev, 0x17a2, 0x00000620);
+ vsc85xx_tr_write(phydev, 0x16a0, 0x00eeffdd);
+ vsc85xx_tr_write(phydev, 0x16a6, 0x00071448);
+ vsc85xx_tr_write(phydev, 0x16a4, 0x0013132f);
+ vsc85xx_tr_write(phydev, 0x16a8, 0x00000000);
+ vsc85xx_tr_write(phydev, 0x0ffc, 0x00c0a028);
+ vsc85xx_tr_write(phydev, 0x0fe8, 0x0091b06c);
+ vsc85xx_tr_write(phydev, 0x0fea, 0x00041600);
+ vsc85xx_tr_write(phydev, 0x0f80, 0x00000af4);
+ vsc85xx_tr_write(phydev, 0x0fec, 0x00901809);
+ vsc85xx_tr_write(phydev, 0x0fee, 0x0000a6a1);
+ vsc85xx_tr_write(phydev, 0x0ffe, 0x00b01007);
+ vsc85xx_tr_write(phydev, 0x16b0, 0x00eeff00);
+ vsc85xx_tr_write(phydev, 0x16b2, 0x00007000);
+ vsc85xx_tr_write(phydev, 0x16b4, 0x00000814);
+
+out_unlock:
+ rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
+ mutex_unlock(&phydev->lock);
+
+ return rc;
+}
+
static int vsc85xx_config_init(struct phy_device *phydev)
{
int rc, i;
@@ -702,6 +752,10 @@ static int vsc85xx_config_init(struct phy_device *phydev)
if (rc)
return rc;

+ rc = vsc85xx_eee_init_seq_set(phydev);
+ if (rc)
+ return rc;
+
for (i = 0; i < vsc8531->nleds; i++) {
rc = vsc85xx_led_cntl_set(phydev, i, vsc8531->leds_mode[i]);
if (rc)
--
git-series 0.9.1

2018-09-14 08:35:40

[permalink] [raw]

Subject: [PATCH net-next 1/5] net: phy: mscc: add ethtool statistics counters

From: Raju Lakkaraju <[email protected]>

There are a few counters available in the PHY: receive errors, false
carriers, link disconnects, media CRC errors and valids counters.

So let's expose those in the PHY driver.

Use the priv structure as the next PHY to be supported has a few
additional counters.

Signed-off-by: Raju Lakkaraju <[email protected]>
Signed-off-by: Quentin Schulz <[email protected]>
---
drivers/net/phy/mscc.c | 128 ++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 128 insertions(+)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index 2d9676d..62d6e0a 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -33,6 +33,11 @@ enum rgmii_rx_clock_delay {
#define DISABLE_PAIR_SWAP_CORR_MASK 0x0020
#define DISABLE_POLARITY_CORR_MASK 0x0010

+#define MSCC_PHY_ERR_RX_CNT 19
+#define MSCC_PHY_ERR_FALSE_CARRIER_CNT 20
+#define MSCC_PHY_ERR_LINK_DISCONNECT_CNT 21
+#define ERR_CNT_MASK GENMASK(7, 0)
+
#define MSCC_PHY_EXT_PHY_CNTL_1 23
#define MAC_IF_SELECTION_MASK 0x1800
#define MAC_IF_SELECTION_GMII 0
@@ -64,6 +69,9 @@ enum rgmii_rx_clock_delay {
#define MSCC_PHY_PAGE_EXTENDED_2 0x0002 /* Extended reg - page 2 */

/* Extended Page 1 Registers */
+#define MSCC_PHY_CU_MEDIA_CRC_VALID_CNT 18
+#define VALID_CRC_CNT_CRC_MASK GENMASK(13, 0)
+
#define MSCC_PHY_EXT_MODE_CNTL 19
#define FORCE_MDI_CROSSOVER_MASK 0x000C
#define FORCE_MDI_CROSSOVER_MDIX 0x000C
@@ -74,6 +82,8 @@ enum rgmii_rx_clock_delay {
#define DOWNSHIFT_EN 0x0010
#define DOWNSHIFT_CNTL_POS 2

+#define MSCC_PHY_EXT_PHY_CNTL_4 23
+
/* Extended Page 2 Registers */
#define MSCC_PHY_RGMII_CNTL 20
#define RGMII_RX_CLK_DELAY_MASK 0x0070
@@ -119,11 +129,50 @@ enum rgmii_rx_clock_delay {
BIT(VSC8531_FORCE_LED_OFF) | \
BIT(VSC8531_FORCE_LED_ON))

+struct vsc85xx_hw_stat {
+ const char *string;
+ u8 reg;
+ u16 page;
+ u16 mask;
+};
+
+static struct vsc85xx_hw_stat vsc85xx_hw_stats[] = {
+ {
+ .string = "phy_receive_errors",
+ .reg = MSCC_PHY_ERR_RX_CNT,
+ .page = MSCC_PHY_PAGE_STANDARD,
+ .mask = ERR_CNT_MASK,
+ }, {
+ .string = "phy_false_carrier",
+ .reg = MSCC_PHY_ERR_FALSE_CARRIER_CNT,
+ .page = MSCC_PHY_PAGE_STANDARD,
+ .mask = ERR_CNT_MASK,
+ }, {
+ .string = "phy_cu_media_link_disconnect",
+ .reg = MSCC_PHY_ERR_LINK_DISCONNECT_CNT,
+ .page = MSCC_PHY_PAGE_STANDARD,
+ .mask = ERR_CNT_MASK,
+ }, {
+ .string = "phy_cu_media_crc_good_count",
+ .reg = MSCC_PHY_CU_MEDIA_CRC_VALID_CNT,
+ .page = MSCC_PHY_PAGE_EXTENDED,
+ .mask = VALID_CRC_CNT_CRC_MASK,
+ }, {
+ .string = "phy_cu_media_crc_error_count",
+ .reg = MSCC_PHY_EXT_PHY_CNTL_4,
+ .page = MSCC_PHY_PAGE_EXTENDED,
+ .mask = ERR_CNT_MASK,
+ },
+};
+
struct vsc8531_private {
int rate_magic;
u16 supp_led_modes;
u32 leds_mode[MAX_LEDS];
u8 nleds;
+ struct vsc85xx_hw_stat *hw_stats;
+ u64 *stats;
+ int nstats;
};

#ifdef CONFIG_OF_MDIO
@@ -148,6 +197,66 @@ static int vsc85xx_phy_page_set(struct phy_device *phydev, u16 page)
return rc;
}

+static int vsc85xx_get_sset_count(struct phy_device *phydev)
+{
+ struct vsc8531_private *priv = phydev->priv;
+
+ if (!priv)
+ return 0;
+
+ return priv->nstats;
+}
+
+static void vsc85xx_get_strings(struct phy_device *phydev, u8 *data)
+{
+ struct vsc8531_private *priv = phydev->priv;
+ int i;
+
+ if (!priv)
+ return;
+
+ for (i = 0; i < priv->nstats; i++)
+ strlcpy(data + i * ETH_GSTRING_LEN, priv->hw_stats[i].string,
+ ETH_GSTRING_LEN);
+}
+
+static u64 vsc85xx_get_stat(struct phy_device *phydev, int i)
+{
+ struct vsc8531_private *priv = phydev->priv;
+ int val;
+ u64 ret;
+
+ vsc85xx_phy_page_set(phydev, priv->hw_stats[i].page);
+
+ val = phy_read(phydev, priv->hw_stats[i].reg);
+ if (val < 0) {
+ ret = U64_MAX;
+ goto out;
+ }
+
+ val = val & priv->hw_stats[i].mask;
+ priv->stats[i] += val;
+ ret = priv->stats[i];
+
+out:
+ vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
+
+ return ret;
+}
+
+static void vsc85xx_get_stats(struct phy_device *phydev,
+ struct ethtool_stats *stats, u64 *data)
+{
+ struct vsc8531_private *priv = phydev->priv;
+ int i;
+
+ if (!priv)
+ return;
+
+ for (i = 0; i < priv->nstats; i++)
+ data[i] = vsc85xx_get_stat(phydev, i);
+}
+
static int vsc85xx_led_cntl_set(struct phy_device *phydev,
u8 led_num,
u8 mode)
@@ -673,6 +782,13 @@ static int vsc85xx_probe(struct phy_device *phydev)
vsc8531->rate_magic = rate_magic;
vsc8531->nleds = 2;
vsc8531->supp_led_modes = VSC85XX_SUPP_LED_MODES;
+ vsc8531->hw_stats = vsc85xx_hw_stats;
+ vsc8531->nstats = ARRAY_SIZE(vsc85xx_hw_stats);
+ vsc8531->stats = devm_kzalloc(&phydev->mdio.dev,
+ sizeof(u64) * vsc8531->nstats,
+ GFP_KERNEL);
+ if (!vsc8531->stats)
+ return -ENOMEM;

return vsc85xx_dt_led_modes_get(phydev, default_mode);
}
@@ -699,6 +815,9 @@ static struct phy_driver vsc85xx_driver[] = {
.get_wol = &vsc85xx_wol_get,
.get_tunable = &vsc85xx_get_tunable,
.set_tunable = &vsc85xx_set_tunable,
+ .get_sset_count = &vsc85xx_get_sset_count,
+ .get_strings = &vsc85xx_get_strings,
+ .get_stats = &vsc85xx_get_stats,
},
{
.phy_id = PHY_ID_VSC8531,
@@ -720,6 +839,9 @@ static struct phy_driver vsc85xx_driver[] = {
.get_wol = &vsc85xx_wol_get,
.get_tunable = &vsc85xx_get_tunable,
.set_tunable = &vsc85xx_set_tunable,
+ .get_sset_count = &vsc85xx_get_sset_count,
+ .get_strings = &vsc85xx_get_strings,
+ .get_stats = &vsc85xx_get_stats,
},
{
.phy_id = PHY_ID_VSC8540,
@@ -741,6 +863,9 @@ static struct phy_driver vsc85xx_driver[] = {
.get_wol = &vsc85xx_wol_get,
.get_tunable = &vsc85xx_get_tunable,
.set_tunable = &vsc85xx_set_tunable,
+ .get_sset_count = &vsc85xx_get_sset_count,
+ .get_strings = &vsc85xx_get_strings,
+ .get_stats = &vsc85xx_get_stats,
},
{
.phy_id = PHY_ID_VSC8541,
@@ -762,6 +887,9 @@ static struct phy_driver vsc85xx_driver[] = {
.get_wol = &vsc85xx_wol_get,
.get_tunable = &vsc85xx_get_tunable,
.set_tunable = &vsc85xx_set_tunable,
+ .get_sset_count = &vsc85xx_get_sset_count,
+ .get_strings = &vsc85xx_get_strings,
+ .get_stats = &vsc85xx_get_stats,
}

};
--
git-series 0.9.1

2018-09-14 08:35:53

[permalink] [raw]

Subject: [PATCH net-next 3/5] net: phy: mscc: remove unneeded parenthesis

The == operator precedes the || operator, so we can remove the
parenthesis around (a == b) || (c == d).

The condition is rather explicit and short so removing the parenthesis
definitely does not make it harder to read.

Signed-off-by: Quentin Schulz <[email protected]>
---
drivers/net/phy/mscc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
index c0a9ea9..734d9fb 100644
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -301,7 +301,7 @@ static int vsc85xx_mdix_set(struct phy_device *phydev, u8 mdix)
u16 reg_val;

reg_val = phy_read(phydev, MSCC_PHY_BYPASS_CONTROL);
- if ((mdix == ETH_TP_MDI) || (mdix == ETH_TP_MDI_X)) {
+ if (mdix == ETH_TP_MDI || mdix == ETH_TP_MDI_X) {
reg_val |= (DISABLE_PAIR_SWAP_CORR_MASK |
DISABLE_POLARITY_CORR_MASK |
DISABLE_HP_AUTO_MDIX_MASK);
--
git-series 0.9.1

2018-09-14 13:02:30

[permalink] [raw]

Subject: Re: [PATCH net-next 1/5] net: phy: mscc: add ethtool statistics counters

Hi Quentin

> +static struct vsc85xx_hw_stat vsc85xx_hw_stats[] = {

You could add a const to that.

> + {
> + .string = "phy_receive_errors",
> + .reg = MSCC_PHY_ERR_RX_CNT,
> + .page = MSCC_PHY_PAGE_STANDARD,
> + .mask = ERR_CNT_MASK,
> + }, {
> + .string = "phy_false_carrier",
> + .reg = MSCC_PHY_ERR_FALSE_CARRIER_CNT,
> + .page = MSCC_PHY_PAGE_STANDARD,
> + .mask = ERR_CNT_MASK,
> + }, {
> + .string = "phy_cu_media_link_disconnect",
> + .reg = MSCC_PHY_ERR_LINK_DISCONNECT_CNT,
> + .page = MSCC_PHY_PAGE_STANDARD,
> + .mask = ERR_CNT_MASK,
> + }, {
> + .string = "phy_cu_media_crc_good_count",
> + .reg = MSCC_PHY_CU_MEDIA_CRC_VALID_CNT,
> + .page = MSCC_PHY_PAGE_EXTENDED,
> + .mask = VALID_CRC_CNT_CRC_MASK,
> + }, {
> + .string = "phy_cu_media_crc_error_count",
> + .reg = MSCC_PHY_EXT_PHY_CNTL_4,
> + .page = MSCC_PHY_PAGE_EXTENDED,
> + .mask = ERR_CNT_MASK,
> + },
> +};

> +static u64 vsc85xx_get_stat(struct phy_device *phydev, int i)
> +{
> + struct vsc8531_private *priv = phydev->priv;
> + int val;
> + u64 ret;
> +
> + vsc85xx_phy_page_set(phydev, priv->hw_stats[i].page);

I might of asked this before...

Does changing the page effect registers in the lower range? It is
possible for other operations to happen at the same time, and you
don't want for example a status read to happen from some other
extended page register because a statistics read is happening.

phy_read_page() and phy_write_page() will do the needed locking if
this is an issue.

> @@ -673,6 +782,13 @@ static int vsc85xx_probe(struct phy_device *phydev)
> vsc8531->rate_magic = rate_magic;
> vsc8531->nleds = 2;
> vsc8531->supp_led_modes = VSC85XX_SUPP_LED_MODES;
> + vsc8531->hw_stats = vsc85xx_hw_stats;
> + vsc8531->nstats = ARRAY_SIZE(vsc85xx_hw_stats);
> + vsc8531->stats = devm_kzalloc(&phydev->mdio.dev,
> + sizeof(u64) * vsc8531->nstats,
> + GFP_KERNEL);

devm_kmalloc_array()? The security people prefer that.

> + if (!vsc8531->stats)
> + return -ENOMEM;
>
> return vsc85xx_dt_led_modes_get(phydev, default_mode);
> }

Andrew

2018-09-14 13:05:20

[permalink] [raw]

Subject: Re: [PATCH net-next 3/5] net: phy: mscc: remove unneeded parenthesis

On Fri, Sep 14, 2018 at 10:33:45AM +0200, Quentin Schulz wrote:
> The == operator precedes the || operator, so we can remove the
> parenthesis around (a == b) || (c == d).
>
> The condition is rather explicit and short so removing the parenthesis
> definitely does not make it harder to read.
>
> Signed-off-by: Quentin Schulz <[email protected]>

Reviewed-by: Andrew Lunn <[email protected]>

Andrew

2018-09-14 13:06:33

[permalink] [raw]

Subject: Re: [PATCH net-next 4/5] net: phy: mscc: shorten `x != 0` condition to `x`

On Fri, Sep 14, 2018 at 10:33:46AM +0200, Quentin Schulz wrote:
> `if (x != 0)` is basically a more verbose version of `if (x)` so let's
> use the latter so it's consistent throughout the whole driver.
>
> Signed-off-by: Quentin Schulz <[email protected]>

Reviewed-by: Andrew Lunn <[email protected]>

Andrew

2018-09-14 13:07:08

[permalink] [raw]

Subject: Re: [PATCH net-next 5/5] net: phy: mscc: remove unneeded temporary variable

On Fri, Sep 14, 2018 at 10:33:47AM +0200, Quentin Schulz wrote:
> Here, the rc variable is either used only for the condition right after
> the assignment or right before being used as the return value of the
> function it's being used in.
>
> So let's remove this unneeded temporary variable whenever possible.
>
> Signed-off-by: Quentin Schulz <[email protected]>

Reviewed-by: Andrew Lunn <[email protected]>

Andrew

2018-09-14 13:17:43

[permalink] [raw]

Subject: Re: [PATCH net-next 1/5] net: phy: mscc: add ethtool statistics counters

Hi Andrew,

On Fri, Sep 14, 2018 at 03:01:56PM +0200, Andrew Lunn wrote:
> Hi Quentin
>
> > +static struct vsc85xx_hw_stat vsc85xx_hw_stats[] = {
>
> You could add a const to that.
>

ACK.

> > + {
> > + .string = "phy_receive_errors",
> > + .reg = MSCC_PHY_ERR_RX_CNT,
> > + .page = MSCC_PHY_PAGE_STANDARD,
> > + .mask = ERR_CNT_MASK,
> > + }, {
> > + .string = "phy_false_carrier",
> > + .reg = MSCC_PHY_ERR_FALSE_CARRIER_CNT,
> > + .page = MSCC_PHY_PAGE_STANDARD,
> > + .mask = ERR_CNT_MASK,
> > + }, {
> > + .string = "phy_cu_media_link_disconnect",
> > + .reg = MSCC_PHY_ERR_LINK_DISCONNECT_CNT,
> > + .page = MSCC_PHY_PAGE_STANDARD,
> > + .mask = ERR_CNT_MASK,
> > + }, {
> > + .string = "phy_cu_media_crc_good_count",
> > + .reg = MSCC_PHY_CU_MEDIA_CRC_VALID_CNT,
> > + .page = MSCC_PHY_PAGE_EXTENDED,
> > + .mask = VALID_CRC_CNT_CRC_MASK,
> > + }, {
> > + .string = "phy_cu_media_crc_error_count",
> > + .reg = MSCC_PHY_EXT_PHY_CNTL_4,
> > + .page = MSCC_PHY_PAGE_EXTENDED,
> > + .mask = ERR_CNT_MASK,
> > + },
> > +};
>
> > +static u64 vsc85xx_get_stat(struct phy_device *phydev, int i)
> > +{
> > + struct vsc8531_private *priv = phydev->priv;
> > + int val;
> > + u64 ret;
> > +
> > + vsc85xx_phy_page_set(phydev, priv->hw_stats[i].page);
>
> I might of asked this before...
>
> Does changing the page effect registers in the lower range? It is
> possible for other operations to happen at the same time, and you
> don't want for example a status read to happen from some other
> extended page register because a statistics read is happening.
>

When you change a page, you basically can access only the registers in
this page so if there are two functions requesting different pages at
the same time or registers of different pages, it won't work well
indeed.

> phy_read_page() and phy_write_page() will do the needed locking if
> this is an issue.
>

That's awesome! Didn't know it existed. Thanks a ton!

Well, that means I should migrate the whole driver to use
phy_read/write_paged instead of the phy_read/write that is currently in
use.

That's impacting performance though as per phy_read/write_paged we read
the current page, set the desired page, read/write the register, set the
old page back. That's 4 times more operations. Couldn't we use the
phy_device mutex instead (as it's currently done in the whole driver)?
Or is it worse/comparable in performance to the suggested solution?

> > @@ -673,6 +782,13 @@ static int vsc85xx_probe(struct phy_device *phydev)
> > vsc8531->rate_magic = rate_magic;
> > vsc8531->nleds = 2;
> > vsc8531->supp_led_modes = VSC85XX_SUPP_LED_MODES;
> > + vsc8531->hw_stats = vsc85xx_hw_stats;
> > + vsc8531->nstats = ARRAY_SIZE(vsc85xx_hw_stats);
> > + vsc8531->stats = devm_kzalloc(&phydev->mdio.dev,
> > + sizeof(u64) * vsc8531->nstats,
> > + GFP_KERNEL);
>
> devm_kmalloc_array()? The security people prefer that.
>

ACK.

Thanks,
Quentin

Attachments:

(No filename) (2.97 kB)
signature.asc (849.00 B)
Download all attachments

2018-09-14 13:30:59

[permalink] [raw]

Subject: Re: [PATCH net-next 1/5] net: phy: mscc: add ethtool statistics counters

> When you change a page, you basically can access only the registers in
> this page so if there are two functions requesting different pages at
> the same time or registers of different pages, it won't work well
> indeed.
>
> > phy_read_page() and phy_write_page() will do the needed locking if
> > this is an issue.
> >
>
> That's awesome! Didn't know it existed. Thanks a ton!
>
> Well, that means I should migrate the whole driver to use
> phy_read/write_paged instead of the phy_read/write that is currently in
> use.
>
> That's impacting performance though as per phy_read/write_paged we read
> the current page, set the desired page, read/write the register, set the
> old page back. That's 4 times more operations.

You can use the lower level locking primatives. See m88e1318_set_wol()
for example.

> Couldn't we use the
> phy_device mutex instead (as it's currently done in the whole driver)?
> Or is it worse/comparable in performance to the suggested solution?

Russell King found a race condition where this breaks. You cannot hold
the phy_device mutex everywhere.

Andrew

2018-09-15 20:53:16

[permalink] [raw]

Subject: Re: [PATCH net-next 3/5] net: phy: mscc: remove unneeded parenthesis

On 9/14/2018 1:33 AM, Quentin Schulz wrote:
> The == operator precedes the || operator, so we can remove the
> parenthesis around (a == b) || (c == d).
>
> The condition is rather explicit and short so removing the parenthesis
> definitely does not make it harder to read.
>
> Signed-off-by: Quentin Schulz <[email protected]>

Reviewed-by: Florian Fainelli <[email protected]>
--
Florian

2018-09-15 20:54:28

[permalink] [raw]

Subject: Re: [PATCH net-next 4/5] net: phy: mscc: shorten `x != 0` condition to `x`

On 9/14/2018 1:33 AM, Quentin Schulz wrote:
> `if (x != 0)` is basically a more verbose version of `if (x)` so let's
> use the latter so it's consistent throughout the whole driver.
>
> Signed-off-by: Quentin Schulz <[email protected]>

Reviewed-by: Florian Fainelli <[email protected]>
--
Florian

2018-09-15 21:00:03

[permalink] [raw]

Subject: Re: [PATCH net-next 5/5] net: phy: mscc: remove unneeded temporary variable

On 09/14/18 01:33, Quentin Schulz wrote:
> Here, the rc variable is either used only for the condition right after
> the assignment or right before being used as the return value of the
> function it's being used in.
>
> So let's remove this unneeded temporary variable whenever possible.
>
> Signed-off-by: Quentin Schulz <[email protected]>

Reviewed-by: Florian Fainelli <[email protected]>
--
Florian

2018-09-15 21:01:32

[permalink] [raw]

Subject: Re: [PATCH net-next 2/5] net: phy: mscc: Add EEE init sequence

On 09/14/18 01:33, Quentin Schulz wrote:
> From: Raju Lakkaraju <[email protected]>
>
> Microsemi PHYs (VSC 8530/31/40/41) need to update the Energy Efficient
> Ethernet initialization sequence.
> In order to avoid certain link state errors that could result in link
> drops and packet loss, the physical coding sublayer (PCS) must be
> updated with settings related to EEE in order to improve performance.
>
> Signed-off-by: Raju Lakkaraju <[email protected]>
> Signed-off-by: Quentin Schulz <[email protected]>
> ---

[snip]

> + vsc85xx_tr_write(phydev, 0x0f82, 0x0012b00a);

Can you just make this an array of register + value pair? That would be
less error prone in case you need to update that sequence in the future.

With that:

Reviewed-by: Florian Fainelli <[email protected]>
--
Florian

2018-10-01 08:52:10

[permalink] [raw]

Subject: Re: [PATCH net-next 2/5] net: phy: mscc: Add EEE init sequence

Hi Florian,

On Fri, Sep 14, 2018 at 07:21:09PM -0700, Florian Fainelli wrote:
>
>
> On 09/14/18 01:33, Quentin Schulz wrote:
> > From: Raju Lakkaraju <[email protected]>
> >
> > Microsemi PHYs (VSC 8530/31/40/41) need to update the Energy Efficient
> > Ethernet initialization sequence.
> > In order to avoid certain link state errors that could result in link
> > drops and packet loss, the physical coding sublayer (PCS) must be
> > updated with settings related to EEE in order to improve performance.
> >
> > Signed-off-by: Raju Lakkaraju <[email protected]>
> > Signed-off-by: Quentin Schulz <[email protected]>
> > ---
>
> [snip]
>
> > + vsc85xx_tr_write(phydev, 0x0f82, 0x0012b00a);
>
> Can you just make this an array of register + value pair? That would be

Sure, I'll.

> less error prone in case you need to update that sequence in the future.
>

I'm curious about the kind of errors you're worrying about or have
experienced. Do you have any particular example or thought in mind?

> With that:
>
> Reviewed-by: Florian Fainelli <[email protected]>

Thanks,
Quentin

Attachments:

(No filename) (1.13 kB)
signature.asc (849.00 B)
Download all attachments

2018-10-01 16:27:49

[permalink] [raw]

Subject: Re: [PATCH net-next 2/5] net: phy: mscc: Add EEE init sequence

On 10/01/2018 01:51 AM, Quentin Schulz wrote:
> Hi Florian,
>
> On Fri, Sep 14, 2018 at 07:21:09PM -0700, Florian Fainelli wrote:
>>
>>
>> On 09/14/18 01:33, Quentin Schulz wrote:
>>> From: Raju Lakkaraju <[email protected]>
>>>
>>> Microsemi PHYs (VSC 8530/31/40/41) need to update the Energy Efficient
>>> Ethernet initialization sequence.
>>> In order to avoid certain link state errors that could result in link
>>> drops and packet loss, the physical coding sublayer (PCS) must be
>>> updated with settings related to EEE in order to improve performance.
>>>
>>> Signed-off-by: Raju Lakkaraju <[email protected]>
>>> Signed-off-by: Quentin Schulz <[email protected]>
>>> ---
>>
>> [snip]
>>
>>> + vsc85xx_tr_write(phydev, 0x0f82, 0x0012b00a);
>>
>> Can you just make this an array of register + value pair? That would be
>
> Sure, I'll.
>
>> less error prone in case you need to update that sequence in the future.
>>
>
> I'm curious about the kind of errors you're worrying about or have
> experienced. Do you have any particular example or thought in mind?

Since this is just a completely non documented sequence likely given
as-is by the vendor, there could be in the future an arbitrary number of
changes made to that sequence because reasons. It seems to me that
putting that sequence in an array, instead of having to produce the
right sequence of calls, inlined in the source, is more manageable, and
will lead to an easier process if back porting/forward porting is necessary.
--
Florian

2018-10-02 13:52:27

[permalink] [raw]

Subject: Re: [PATCH net-next 1/5] net: phy: mscc: add ethtool statistics counters

Hi Russel,

Adding you to the discussion as you're the author and commiter of the
patch adding support for all the paged access in the phy core.

On Fri, Sep 14, 2018 at 03:29:59PM +0200, Andrew Lunn wrote:
> > When you change a page, you basically can access only the registers in
> > this page so if there are two functions requesting different pages at
> > the same time or registers of different pages, it won't work well
> > indeed.
> >
> > > phy_read_page() and phy_write_page() will do the needed locking if
> > > this is an issue.
> > >
> >
> > That's awesome! Didn't know it existed. Thanks a ton!
> >
> > Well, that means I should migrate the whole driver to use
> > phy_read/write_paged instead of the phy_read/write that is currently in
> > use.
> >
> > That's impacting performance though as per phy_read/write_paged we read
> > the current page, set the desired page, read/write the register, set the
> > old page back. That's 4 times more operations.
>
> You can use the lower level locking primatives. See m88e1318_set_wol()
> for example.
>

I'm converting the drivers/net/phy/mscc.c driver to make use of the
paged accesses but I'm hitting something confusing to me.

Firstly, just to be sure, I should use paged accesses only for read/write
outside of the standard page, right? I'm guessing that because we need
to be able to use the genphy functions which are using phy_write/read
and not __phy_write/read, thus we assume the mdio lock is not taken
(which is taken by phy_select/restore_page) and those functions
read/write to the standard page.

Secondly, I should refactor the driver to do the following:

oldpage = phy_select_page();
if (oldpage < 0) {
phy_restore_page();
error_path;
}

[...]
/* paged accesses */
__phy_write/read();
[...]

phy_restore_page();

I assume this is the correct way to handle paged accesses. Let me know
if it's not clear enough or wrong. (depending on the function, we could
of course put phy_restore_page in the error_path).

Now, I saw that phy_restore_page takes the phydev, the oldpage and a ret
parameters[1].

The (ret >= 0 && r < 0) condition of [2] seems counterintuitive to me.

ret being the argument passed to the function and r being the return of
__phy_write_page (which is phydev->drv->phy_write_page()).

In my understanding of C best practices, any return value equal to zero
marks a successful call to the function.

That would mean that with:
if (ret >= 0 && r < 0)
ret = r;

If ret is greather than 0, if __phy_write_page is successful (r == 0),
ret will be > 0, which would result in phy_restore_page not returning 0
thus signaling (IMO) an error occured in phy_restore_page.

One example is the following:
oldpage = phy_select_page(phydev, new_page);
[...]
return phy_restore_page(phydev, oldpage, oldpage);

If phy_select_page is successful, return phy_restore_page(phydev,
oldpage, oldpage) would return the value of oldpage which can be
different from 0.

This code could (I think) be working with `if (ret >= 0 && r <= 0)` (or
even `if (ret >= 0)`).

Now to have the same behaviour, I need to do:
oldpage = phy_select_page(phydev, new_page);
[...]
return phy_restore_page(phydev, oldpage, oldpage > 0 ? 0 : oldpage);

Another example is:
oldpage = phy_select_page(phydev, new_page);
ret = `any function returning a value > 0 in case of success and < 0 in
case of failure`().
return phy_restore_page(phydev, oldpage, ret);

Is there any reason for not wanting to overwrite the ret value when
__phy_write_page is successful in phy_restore_page?

I'd say that it could be more readable without the ternary condition in
the argument of phy_restore_page.

Let me know your thoughts on this.

Thanks,
Quentin

[1] https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy-core.c#L444
[2] https://elixir.bootlin.com/linux/latest/source/drivers/net/phy/phy-core.c#L454

Attachments:

(No filename) (3.88 kB)
signature.asc (849.00 B)
Download all attachments

2018-10-04 14:19:27