Re: [PATCH 0/4] net: phy: Register header file for Microsemi PHYs.
Hi Andrew, Thank you for quick response. I did not use "git send-email" I created 5 patches and sent them by using 5 individual mails thru Mutt. Thanks, Raju. On Wed, Aug 24, 2016 at 02:45:26PM +0200, Andrew Lunn wrote: > EXTERNAL EMAIL > > > On Wed, Aug 24, 2016 at 11:58:01AM +, Raju Lakkaraju wrote: > > From: Nagaraju Lakkaraju <raju.lakkar...@microsemi.com> > > > > This is Microsemi's VSC 85xx PHY register definitions header file. > > Hi Raju > > Patch [0/X] is the cover note. It should contain an overview of the > patch series and why this patch series is needed. It should not > contain a patch itself. > > Also, the 5 patches have not been threaded together. Did you send them > with a single git send-email command, or 5 individual commands? By > default, using a single command should cause them to be threaded. > > Andrew > > > > > Signed-off-by: Nagaraju Lakkaraju <raju.lakkar...@microsemi.com> > > --- > > drivers/net/phy/mscc_reg.h | 135 > > + > > 1 file changed, 135 insertions(+) > > create mode 100644 drivers/net/phy/mscc_reg.h > > > > diff --git a/drivers/net/phy/mscc_reg.h b/drivers/net/phy/mscc_reg.h > > new file mode 100644 > > index 000..ddb825c > > --- /dev/null > > +++ b/drivers/net/phy/mscc_reg.h > > @@ -0,0 +1,135 @@ > > +/* > > + * Driver for Microsemi VSC85xx PHYs > > + * > > + * Author: Nagaraju Lakkaraju > > + * License: Dual MIT/GPL > > + * Copyright (c) 2016 Microsemi Corporation > > + */ > > + > > +#ifndef __MSCC_REG_H > > +#define __MSCC_REG_H > > + > > +/* Microsemi VSC85xx PHY registers */ > > +/* IEEE 802. Std Registers */ > > +#define MSCC_PHY_BYPASS_CONTROL18 > > +#define DISABLE_HP_AUTO_MDIX_MASK 0x0080 > > +#define DISABLE_PAIR_SWAP_CORR_MASK0x0020 > > +#define DISABLE_POLARITY_CORR_MASK 0x0010 > > + > > +#define MSCC_PHY_EXT_PHY_CNTL_123 > > +#define MAC_IF_SELECTION_MASK 0x1800 > > +#define MAC_IF_SELECTION_GMII 0 > > +#define MAC_IF_SELECTION_RMII 1 > > +#define MAC_IF_SELECTION_RGMII 2 > > +#define MAC_IF_SELECTION_POS 11 > > +#define FAR_END_LOOPBACK_MODE_MASK 0x0008 > > + > > +#define MSCC_PHY_EXT_PHY_CNTL_224 > > +#define CONNECTOR_LOOPBACK_MASK0x0001 > > +#define JUMBO_PACKET_MODE_MASK 0x0030 > > +#define JUMBO_PACKET_MODE_POS 4 > > + > > +#define MII_VSC85XX_INT_MASK 25 > > +#define MII_VSC85XX_INT_MASK_MDINT 0x8000 > > +#define MII_VSC85XX_INT_MASK_SPEED 0x4000 > > +#define MII_VSC85XX_INT_MASK_LINK 0x2000 > > +#define MII_VSC85XX_INT_MASK_DUPLEX0x1000 > > +#define MII_VSC85XX_INT_MASK_ANEG_ERR 0x0800 > > +#define MII_VSC85XX_INT_MASK_ANEG_COM 0x0400 > > +#define MII_VSC85XX_INT_MASK_POE 0x0200 > > +#define MII_VSC85XX_INT_MASK_SYM 0x0100 > > +#define MII_VSC85XX_INT_MASK_FLF 0x0080 > > +#define MII_VSC85XX_INT_MASK_WOL 0x0040 > > +#define MII_VSC85XX_INT_MASK_EXT 0x0020 > > +#define MII_VSC85XX_INT_MASK_RESV 0x0010 > > +#define MII_VSC85XX_INT_MASK_FCI 0x0008 > > +#define MII_VSC85XX_INT_MASK_LDI 0x0004 > > +#define MII_VSC85XX_INT_MASK_MSE 0x0002 > > +#define MII_VSC85XX_INT_MASK_RX_ER 0x0001 > > +#define MII_VSC85XX_INT_MASK_MASK 0xa000 > > + > > +#define MII_VSC85XX_INT_STATUS 26 > > +#define MSCC_PHY_DEV_AUX_CNTL 28 > > +#define HP_AUTO_MDIX_X_OVER_IND_MASK 0x2000 > > +#define ACTIPHY_TIME_OUT_BIT_7 0x0080 > > +#define ACTIPHY_TIME_OUT_BIT_2 0x0004 > > +#define ACTIPHY_TIME_OUT_MASK 0x0084 > > +#define ACTIPHY_MODE_ENT 0x0040 > > + > > +#define MSCC_EXT_PAGE_ACCESS 31 > > +#define MSCC_PHY_PAGE_STANDARD 0x /* Standard registers */ > > +#define MSCC_PHY_PAGE_EXTENDED 0x0001 /* Extended registers */ > > +#define MSCC_PHY_PAGE_EXTENDED_2 0x0002 /* Extended registers - page > > 2 */ > > +#define MSCC_PHY_PAGE_EXTENDED_3 0x0003 /* Extended registers - page > > 3 */ > > +#define MSCC_PHY_PAGE_EXTENDED_4 0x0004 /* Extended registers - page > > 4 */ > > +#define MSCC_PHY_PAGE_GPIO 0x0010 /* GPIO registers */ > > + > > +/* Extended Page 1 Registers */ > > +#define MSCC_PHY_EXT_MODE_CNTL 19 > > +#define FORCE_MDI_CROSSOVER_MASK 0x000C > > +#define FORCE_M
RE: [PATCH v1 1/1] net: phy: Add edge-rate, mac-if, read, write func to Microsemi PHYs.
Hello,
Can you please review this code?
Thanks,
Raju.
-Original Message-
From: Nagaraju Lakkaraju [mailto:raju.lakkar...@microsemi.com]
Sent: Monday, August 08, 2016 7:13 PM
To: netdev@vger.kernel.org
Cc: f.faine...@gmail.com; Allan Nielsen
Subject: [PATCH v1 1/1] net: phy: Add edge-rate, mac-if, read, write func to
Microsemi PHYs.
Hello,
As part of 2nd patch, Add Edge rate control, MAC Interface, Read and write
driver functions add for Microsemi PHYs.
Please review and send your comments.
Thanks,
Raju.
>From 6303576768b5c5dcc0e35fb46c525337a3845557 Mon Sep 17 00:00:00 2001
From: Nagaraju Lakkaraju <raju.lakkar...@microsemi.com>
Date: Mon, 8 Aug 2016 18:51:36 +0530
Subject: [PATCH v1 1/1] net: phy: Add edge-rate, mac-if, read, write func to
Microsemi PHYs.
Signed-off-by: Nagaraju Lakkaraju <raju.lakkar...@microsemi.com>
---
drivers/net/phy/mscc.c | 234 +++--
drivers/net/phy/mscc_reg.h | 135 ++
include/linux/mscc.h | 45 +
include/linux/phy.h| 2 +
4 files changed, 387 insertions(+), 29 deletions(-) mode change 100644 =>
100755 drivers/net/phy/mscc.c create mode 100644 drivers/net/phy/mscc_reg.h
create mode 100644 include/linux/mscc.h mode change 100644 => 100755
include/linux/phy.h
diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c old mode 100644
new mode 100755 index 49c7506..af7a441
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -11,34 +11,9 @@
#include
#include
#include
+#include
-enum rgmii_rx_clock_delay {
- RGMII_RX_CLK_DELAY_0_2_NS = 0,
- RGMII_RX_CLK_DELAY_0_8_NS = 1,
- RGMII_RX_CLK_DELAY_1_1_NS = 2,
- RGMII_RX_CLK_DELAY_1_7_NS = 3,
- RGMII_RX_CLK_DELAY_2_0_NS = 4,
- RGMII_RX_CLK_DELAY_2_3_NS = 5,
- RGMII_RX_CLK_DELAY_2_6_NS = 6,
- RGMII_RX_CLK_DELAY_3_4_NS = 7
-};
-
-#define MII_VSC85XX_INT_MASK 25
-#define MII_VSC85XX_INT_MASK_MASK 0xa000
-#define MII_VSC85XX_INT_STATUS26
-
-#define MSCC_EXT_PAGE_ACCESS 31
-#define MSCC_PHY_PAGE_STANDARD0x /* Standard registers */
-#define MSCC_PHY_PAGE_EXTENDED_2 0x0002 /* Extended reg - page 2 */
-
-/* Extended Page 2 Registers */
-#define MSCC_PHY_RGMII_CNTL 20
-#define RGMII_RX_CLK_DELAY_MASK 0x0070
-#define RGMII_RX_CLK_DELAY_POS4
-
-/* Microsemi PHY ID's */
-#define PHY_ID_VSC85310x00070570
-#define PHY_ID_VSC85410x00070770
+#include "mscc_reg.h"
static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page) { @@
-84,7 +59,7 @@ static int vsc85xx_config_init(struct phy_device *phydev)
static int vsc85xx_ack_interrupt(struct phy_device *phydev) {
- int rc;
+ int rc = 0;
if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
rc = phy_read(phydev, MII_VSC85XX_INT_STATUS); @@ -98,7 +73,7
@@ static int vsc85xx_config_intr(struct phy_device *phydev)
if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
rc = phy_write(phydev, MII_VSC85XX_INT_MASK,
- MII_VSC85XX_INT_MASK_MASK);
+ MII_VSC85XX_INT_MASK_MASK);
} else {
rc = phy_write(phydev, MII_VSC85XX_INT_MASK, 0);
if (rc < 0)
@@ -109,6 +84,203 @@ static int vsc85xx_config_intr(struct phy_device *phydev)
return rc;
}
+static int vsc85xx_soft_reset(struct phy_device *phydev) {
+ int rc;
+ u16 reg_val;
+
+ reg_val = phy_read(phydev, MII_BMCR);
+ reg_val |= BMCR_RESET;
+ rc = phy_write(phydev, MII_BMCR, reg_val);
+
+ return rc;
+}
+
+static int vsc85xx_edge_rate_cntl_set(struct phy_device *phydev,
+ u8 *rate)
+{
+ int rc;
+ u16 reg_val;
+ u8 edge_rate = *rate;
+
+ mutex_lock(>lock);
+ rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
+ if (rc != 0)
+ 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);
+ phy_write(phydev, MSCC_PHY_WOL_MAC_CONTROL, reg_val);
+ rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
+
+out_unlock:
+ mutex_unlock(>lock);
+
+ return rc;
+}
+
+static int vsc85xx_edge_rate_cntl_get(struct phy_device *phydev,
+ u8 *rate)
+{
+ int rc;
+ u16 reg_val;
+
+ mutex_lock(>lock);
+ rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
+ if (rc != 0)
+ goto out_unlock;
+ reg_val = phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
+ reg_val &= EDGE_RATE_CNTL_MASK;
+ *rate = reg_val
[PATCH v1 1/1] net: phy: Add edge-rate, mac-if, read, write func to Microsemi PHYs.
crosemi PHYsBcc:
Subject: [PATCH v1 1/1] net: phy: Add edge-rate, mac-if, read, write func to
Reply-To: Nagaraju Lakkaraju <raju.lakkar...@microsemi.com>
Hello,
As part of 2nd patch, Add Edge rate control, MAC Interface, Read and write
driver functions add for Microsemi PHYs.
Please review and send your comments.
Thanks,
Raju.
>From 6303576768b5c5dcc0e35fb46c525337a3845557 Mon Sep 17 00:00:00 2001
From: Nagaraju Lakkaraju <raju.lakkar...@microsemi.com>
Date: Mon, 8 Aug 2016 18:51:36 +0530
Subject: [PATCH v1 1/1] net: phy: Add edge-rate, mac-if, read, write func to
Microsemi PHYs.
Signed-off-by: Nagaraju Lakkaraju <raju.lakkar...@microsemi.com>
---
drivers/net/phy/mscc.c | 234 +++--
drivers/net/phy/mscc_reg.h | 135 ++
include/linux/mscc.h | 45 +
include/linux/phy.h| 2 +
4 files changed, 387 insertions(+), 29 deletions(-)
mode change 100644 => 100755 drivers/net/phy/mscc.c
create mode 100644 drivers/net/phy/mscc_reg.h
create mode 100644 include/linux/mscc.h
mode change 100644 => 100755 include/linux/phy.h
diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
old mode 100644
new mode 100755
index 49c7506..af7a441
--- a/drivers/net/phy/mscc.c
+++ b/drivers/net/phy/mscc.c
@@ -11,34 +11,9 @@
#include
#include
#include
+#include
-enum rgmii_rx_clock_delay {
- RGMII_RX_CLK_DELAY_0_2_NS = 0,
- RGMII_RX_CLK_DELAY_0_8_NS = 1,
- RGMII_RX_CLK_DELAY_1_1_NS = 2,
- RGMII_RX_CLK_DELAY_1_7_NS = 3,
- RGMII_RX_CLK_DELAY_2_0_NS = 4,
- RGMII_RX_CLK_DELAY_2_3_NS = 5,
- RGMII_RX_CLK_DELAY_2_6_NS = 6,
- RGMII_RX_CLK_DELAY_3_4_NS = 7
-};
-
-#define MII_VSC85XX_INT_MASK 25
-#define MII_VSC85XX_INT_MASK_MASK 0xa000
-#define MII_VSC85XX_INT_STATUS26
-
-#define MSCC_EXT_PAGE_ACCESS 31
-#define MSCC_PHY_PAGE_STANDARD0x /* Standard registers */
-#define MSCC_PHY_PAGE_EXTENDED_2 0x0002 /* Extended reg - page 2 */
-
-/* Extended Page 2 Registers */
-#define MSCC_PHY_RGMII_CNTL 20
-#define RGMII_RX_CLK_DELAY_MASK 0x0070
-#define RGMII_RX_CLK_DELAY_POS4
-
-/* Microsemi PHY ID's */
-#define PHY_ID_VSC85310x00070570
-#define PHY_ID_VSC85410x00070770
+#include "mscc_reg.h"
static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page)
{
@@ -84,7 +59,7 @@ static int vsc85xx_config_init(struct phy_device *phydev)
static int vsc85xx_ack_interrupt(struct phy_device *phydev)
{
- int rc;
+ int rc = 0;
if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
@@ -98,7 +73,7 @@ static int vsc85xx_config_intr(struct phy_device *phydev)
if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
rc = phy_write(phydev, MII_VSC85XX_INT_MASK,
- MII_VSC85XX_INT_MASK_MASK);
+ MII_VSC85XX_INT_MASK_MASK);
} else {
rc = phy_write(phydev, MII_VSC85XX_INT_MASK, 0);
if (rc < 0)
@@ -109,6 +84,203 @@ static int vsc85xx_config_intr(struct phy_device *phydev)
return rc;
}
+static int vsc85xx_soft_reset(struct phy_device *phydev)
+{
+ int rc;
+ u16 reg_val;
+
+ reg_val = phy_read(phydev, MII_BMCR);
+ reg_val |= BMCR_RESET;
+ rc = phy_write(phydev, MII_BMCR, reg_val);
+
+ return rc;
+}
+
+static int vsc85xx_edge_rate_cntl_set(struct phy_device *phydev,
+ u8 *rate)
+{
+ int rc;
+ u16 reg_val;
+ u8 edge_rate = *rate;
+
+ mutex_lock(>lock);
+ rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
+ if (rc != 0)
+ 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);
+ phy_write(phydev, MSCC_PHY_WOL_MAC_CONTROL, reg_val);
+ rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
+
+out_unlock:
+ mutex_unlock(>lock);
+
+ return rc;
+}
+
+static int vsc85xx_edge_rate_cntl_get(struct phy_device *phydev,
+ u8 *rate)
+{
+ int rc;
+ u16 reg_val;
+
+ mutex_lock(>lock);
+ rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
+ if (rc != 0)
+ goto out_unlock;
+ reg_val = phy_read(phydev, MSCC_PHY_WOL_MAC_CONTROL);
+ reg_val &= EDGE_RATE_CNTL_MASK;
+ *rate = reg_val >> EDGE_RATE_CNTL_POS;
+ rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
+
+out_unlock:
+ mutex_unlock(>lock);
+
+ return rc;
+}
+
+static int vsc85xx_mac_if_set(struct
Re: Microsemi VSC 8531/41 PHY Driver
Hello,
I added all review comments and re-sending for review.
>From a5017f5878a92d2acec86a6a29b1498c457cb73a Mon Sep 17 00:00:00 2001
From: Nagaraju Lakkaraju <raju.lakkar...@microsemi.com>
Date: Wed, 3 Aug 2016 18:28:24 +0530
Subject: [PATCH v2] net: phy: Add drivers for Microsemi PHYs
Signed-off-by: Nagaraju Lakkaraju <raju.lakkar...@microsemi.com>
---
drivers/net/phy/Kconfig | 5 ++
drivers/net/phy/Makefile | 1 +
drivers/net/phy/mscc.c | 161 +++
3 files changed, 167 insertions(+)
create mode 100644 drivers/net/phy/mscc.c
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 47a6434..1b534ea 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -307,6 +307,11 @@ config MDIO_XGENE
This module provides a driver for the MDIO busses found in the
APM X-Gene SoC's.
+config MICROSEMI_PHY
+tristate "Drivers for the Microsemi PHYs"
+---help---
+ Currently supports the VSC8531 and VSC8541 PHYs
+
endif # PHYLIB
config MICREL_KS8995MA
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 534dfa7..a713bd4 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -11,6 +11,7 @@ obj-$(CONFIG_CICADA_PHY) += cicada.o
obj-$(CONFIG_LXT_PHY) += lxt.o
obj-$(CONFIG_QSEMI_PHY)+= qsemi.o
obj-$(CONFIG_SMSC_PHY) += smsc.o
+obj-$(CONFIG_MICROSEMI_PHY) += mscc.o
obj-$(CONFIG_TERANETICS_PHY) += teranetics.o
obj-$(CONFIG_VITESSE_PHY) += vitesse.o
obj-$(CONFIG_BCM_NET_PHYLIB) += bcm-phy-lib.o
diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c
new file mode 100644
index 000..49c7506
--- /dev/null
+++ b/drivers/net/phy/mscc.c
@@ -0,0 +1,161 @@
+/*
+ * Driver for Microsemi VSC85xx PHYs
+ *
+ * Author: Nagaraju Lakkaraju
+ * License: Dual MIT/GPL
+ * Copyright (c) 2016 Microsemi Corporation
+ */
+
+#include
+#include
+#include
+#include
+#include
+
+enum rgmii_rx_clock_delay {
+ RGMII_RX_CLK_DELAY_0_2_NS = 0,
+ RGMII_RX_CLK_DELAY_0_8_NS = 1,
+ RGMII_RX_CLK_DELAY_1_1_NS = 2,
+ RGMII_RX_CLK_DELAY_1_7_NS = 3,
+ RGMII_RX_CLK_DELAY_2_0_NS = 4,
+ RGMII_RX_CLK_DELAY_2_3_NS = 5,
+ RGMII_RX_CLK_DELAY_2_6_NS = 6,
+ RGMII_RX_CLK_DELAY_3_4_NS = 7
+};
+
+#define MII_VSC85XX_INT_MASK 25
+#define MII_VSC85XX_INT_MASK_MASK 0xa000
+#define MII_VSC85XX_INT_STATUS26
+
+#define MSCC_EXT_PAGE_ACCESS 31
+#define MSCC_PHY_PAGE_STANDARD0x /* Standard registers */
+#define MSCC_PHY_PAGE_EXTENDED_2 0x0002 /* Extended reg - page 2 */
+
+/* Extended Page 2 Registers */
+#define MSCC_PHY_RGMII_CNTL 20
+#define RGMII_RX_CLK_DELAY_MASK 0x0070
+#define RGMII_RX_CLK_DELAY_POS4
+
+/* Microsemi PHY ID's */
+#define PHY_ID_VSC85310x00070570
+#define PHY_ID_VSC85410x00070770
+
+static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page)
+{
+ int rc;
+
+ rc = phy_write(phydev, MSCC_EXT_PAGE_ACCESS, page);
+ return rc;
+}
+
+static int vsc85xx_default_config(struct phy_device *phydev)
+{
+ int rc;
+ u16 reg_val;
+
+ mutex_lock(>lock);
+ rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2);
+ if (rc != 0)
+ goto out_unlock;
+
+ reg_val = phy_read(phydev, MSCC_PHY_RGMII_CNTL);
+ reg_val &= ~(RGMII_RX_CLK_DELAY_MASK);
+ reg_val |= (RGMII_RX_CLK_DELAY_1_1_NS << RGMII_RX_CLK_DELAY_POS);
+ phy_write(phydev, MSCC_PHY_RGMII_CNTL, reg_val);
+ rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
+
+out_unlock:
+ mutex_unlock(>lock);
+
+ return rc;
+}
+
+static int vsc85xx_config_init(struct phy_device *phydev)
+{
+ int rc;
+
+ rc = vsc85xx_default_config(phydev);
+ if (rc)
+ return rc;
+ rc = genphy_config_init(phydev);
+
+ return rc;
+}
+
+static int vsc85xx_ack_interrupt(struct phy_device *phydev)
+{
+ int rc;
+
+ if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
+ rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
+
+ return (rc < 0) ? rc : 0;
+}
+
+static int vsc85xx_config_intr(struct phy_device *phydev)
+{
+ int rc;
+
+ if (phydev->interrupts == PHY_INTERRUPT_ENABLED) {
+ rc = phy_write(phydev, MII_VSC85XX_INT_MASK,
+ MII_VSC85XX_INT_MASK_MASK);
+ } else {
+ rc = phy_write(phydev, MII_VSC85XX_INT_MASK, 0);
+ if (rc < 0)
+ return rc;
+ rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
+ }
+
+ return rc;
+}
+
+/* Microsemi VSC85xx PHYs */
+static struct phy_driver vsc85xx_driver[] = {
+{
+ .phy_id = PHY_ID_VSC8531,
Re: Microsemi VSC 8531/41 PHY Driver
On Fri, Jul 29, 2016 at 10:17:36AM +0200, Andrew Lunn wrote: > EXTERNAL EMAIL > > > > > +/* RGMII Rx Clock delay value change with board lay-out */ static u8 > > > +rgmii_rx_clk_delay = RGMII_RX_CLK_DELAY_1_1_NS; > > > > Doesn't this stop you from having a board with two PHYs with different > > layouts? You should be getting this value from the device tree. > > > > Raju: As of now, RGMII Rx clock delay value should be 1.1 nsec as > > optimized/recommended value. > > We tested on Beaglebone Black with VSC 8531 PHY. > > We would like to provide new function to configure correct/require value > > based on PHY layouts > > alone with other RGMII configuration parameters as part of our next > > implementation. > > Please either do it properly now or hard code it as the default, and > then later replace it with device tree, etc. We don't like to see half > finished features. > I accepted your review comment. I do hard code it as the default values. > > What are you locking against? > > > > Raju: VSC 8531 has different PAGEs. Whenever MDC/MDIO access the PHY > > control registers, > > first set the page number then read/write the register address. Default > > page should be Page 0. > > When I want to access not default page register, I have to lock phy device > > access and change > > the page number and register access as atomic operation. > > I understand all that, which is why i asked, "what are you locking > against?", not "why are you locking?" What are the other call paths? I > don't see you taking this lock anywhere else? Should you be? I would > just like to see a comment which suggests you understand when this > lock is needed, and when not. > This is Read Modify Write (RMW) operation on register MSCC_PHY_RGMII_CNTL. This register in different page i.e. EXTENDED-2. I would like to execute all these operations in atomic. I also use the mutex in different functions. But not in this patch. > Andrew
Re: Microsemi VSC 8531/41 PHY Driver
On Thu, Jul 28, 2016 at 10:35:05AM -0700, Florian Fainelli wrote:
> EXTERNAL EMAIL
>
>
> On 07/27/2016 11:44 PM, Raju Lakkaraju wrote:
> > Hello Andrew,
> >
> > Thank you for given valuable comments.
> > Please see the my responses inline.
> >
> > Thanks,
> > Raju
> >
> > -Original Message-
> > From: Andrew Lunn [mailto:and...@lunn.ch]
> > Sent: Tuesday, July 26, 2016 6:14 PM
> > To: Raju Lakkaraju
> > Cc: netdev@vger.kernel.org; f.faine...@gmail.com; Allan Nielsen
> > Subject: Re: Microsemi VSC 8531/41 PHY Driver
> >
> > EXTERNAL EMAIL
> >
> >
> >> +/* RGMII Rx Clock delay value change with board lay-out */ static u8
> >> +rgmii_rx_clk_delay = RGMII_RX_CLK_DELAY_1_1_NS;
> >
> > Doesn't this stop you from having a board with two PHYs with different
> > layouts? You should be getting this value from the device tree.
> >
> > Raju: As of now, RGMII Rx clock delay value should be 1.1 nsec as
> > optimized/recommended value.
> > We tested on Beaglebone Black with VSC 8531 PHY.
>
> That is true, until the next design with a PHY that does not need this
> value and then, it will have to be adjusted.
>
I accepted your review comment. I do hard code it as the default value.
> > We would like to provide new function to configure correct/require value
> > based on PHY layouts
> > alone with other RGMII configuration parameters as part of our next
> > implementation.
>
> You can either introduce a Device Tree property to allow boards to
> specify what the correct delay(s) should be, or if the platform does not
> use Device Tree, using phy_register_fixup_for_id would be acceptable for
> that.
>
I do hard code this value as the default value.
> >
> >> + phydev->supported = (SUPPORTED_1000baseT_Full |
> >> + SUPPORTED_1000baseT_Half |
> >> + SUPPORTED_100baseT_Full |
> >> + SUPPORTED_100baseT_Half |
> >> + SUPPORTED_10baseT_Full |
> >> + SUPPORTED_10baseT_Half |
> >> + SUPPORTED_Autoneg|
> >> + SUPPORTED_Pause |
> >> + SUPPORTED_Asym_Pause |
> >> + SUPPORTED_TP);
>
> This is not necessary, your driver should advertise what the PHY is
> capable of in phy_driver::features. The Ethernet MAC driver later should
> be adjusting phydev->supported with what it actually support, there are
> cases where you connect a 10/100Mbits MAC to a 1Gbits PHY, and you want
> to properly restrict unsupported speeds.
>
I accepted your reveiw comment. I delete initialization.
> >> +
> >> + phydev->speed = SPEED_1000;
> >> + phydev->duplex = DUPLEX_FULL;
> >> + phydev->pause = 0;
> >> + phydev->asym_pause = 0;
> >> + phydev->interface = PHY_INTERFACE_MODE_RGMII;
> >> + phydev->mdix = ETH_TP_MDI_AUTO;
> >
> > Why are you setting all these? This is not normal, if you look at other
> > drivers.
> >
> > Raju: I would like to update the default values in software data structure
> > (phydev).
> > Our PHY is 1G speed support device and RGMII supported device.
>
> Whether RGMII is used as an interface/connection type between the MAC
> and PHY is something that is within the consumer of the PHYLIB API
> (typically Ethernet MAC/Switch driver), your PHY cannot enforce
> anything, but the driver can check that the connection interface is sensble.
>
> All of these default values that you are setting here will need to be
> potentially changed by the state machine (link, duplex, pause) upon
> reaction to link state changes, this change needs to be dropped.
>
I accepted your reveiw comment. I delete initialization.
> >
> >> +
> >> + mutex_lock(>lock);
> >
> > What are you locking against?
> >
> > Raju: VSC 8531 has different PAGEs. Whenever MDC/MDIO access the PHY
> > control registers,
> > first set the page number then read/write the register address. Default
> > page should be Page 0.
> > When I want to access not default page register, I have to lock phy device
> > access and change
> > the page number and register access as atomic operation.
>
> Based on the execution context of this function, acquiring the mutex is
> not necessary, the state machine has not started yet, so there cannot be
> a conflicting PHY read which would end up changing the page selection.
>
> [snip]
>
This is Read Modify Write (RMW) operation on register MSCC_PHY_RGMII_CNTL. This
register in different page i.e. EXTENDED-2. I would like to execute all these
operations in atomic operation.
> >> +
> >> +static int vsc85xx_ack_interrupt(struct phy_device *phydev) {
> >> + int rc = 0;
> >> +
> >> + if (phydev->interrupts == PHY_INTERRUPT_ENABLED)
> >> + rc = phy_read(phydev, MII_VSC85XX_INT_STATUS);
> >> +
> >> + return (rc < 0) ? rc : 0;
> >> +}
> >> +
> >> +static int vsc85xx_config_intr(struct phy_device *phydev) {
> >> + int rc;
> >> +
