* Microsemi VSC 8531/41 PHY Driver @ 2016-07-26 9:49 Raju Lakkaraju 2016-07-26 11:55 ` Andrew Lunn 0 siblings, 1 reply; 15+ messages in thread From: Raju Lakkaraju @ 2016-07-26 9:49 UTC (permalink / raw) To: netdev@vger.kernel.org; +Cc: f.fainelli@gmail.com, Allan Nielsen [-- Attachment #1: Type: text/plain, Size: 2114 bytes --] Hello All, I would like to introduce myself as Nagaraju Lakkaraju (Raju), is working in Microsemi Communications India Pvt. Ltd (Formerly known as Vitesse Semiconductors Limited) - Hyderabad as Sr. Staff Engineer. I do work on Microsemi PHY drivers development. Microsemi is developing the new Physical Layer (PHY) chips for Internet Of Things (IoT) products with 1 Gbps link speed. As part of the development, Microsemi would like to contribute the new PHYs (i.e. VSC 8531 / VSC 8541) chip drivers in Linux Kernel Open source. VSC 8531 / 8541 PHYs will have the following features a part of the basic features like Auto-neg, Speed, Duplex etc. 1. Wake on LAN 2. Auto MDIX/MDI 3. Link Speed Down shift 4. Fast Link Failure-2 5. Loopbacks (FAR-END, NEAR-END and Connector) 6. Ethernet Packet Generator (EPG) 7. Serial Management Interface (SMI) Interrupt 8. Clock Squelch configuration (SyncE) 9. Jumbo Frame Support 10. In-line Power On Ethernet (PoE) status 11. Acti PHY power Management 12. Energy Efficiency Ethenet (EEE) 13. VeriPHY (Cable Diagnostics) 14. LED configuration 15. Ring Resiliency 16. Start Of Frame Detection (SOF) 17. COMA mode As part of Initial submission of the Linux Kernel Open source drivers, I developed the VSC 8531 driver basic features and built the Linux Kernel image for Beaglebone Black hardware. Also developed Ethtool enhancement for VSC 8531 register access functionality to test the VSC 8531 Test Setup: ------------- Hardware Details: Beaglebone Black with VSC 8531 PHY Software Linux Kernel version: 4.6.4 Microsemi VSC 8531 chip is mount on Beaglebone Black hardware (replaced the Microchip PHY) and tested the following features. 1. Auto negotiation 2. Speed change ( 10 Mbps, 100 Mbps and 1 Gbps) 3. Full/Half Duplex 4. Ping 5. Line rate traffic with Test center I would like you to review my code and provide me the valuable comments. Please find the attached git code diff patch. Thanks and regards, Raju (Nagaraju Lakkaraju) Sr. Staff Engg. Microsemi Communications India Pvt Ltd. Ph: +91 040 6686 0132 [-- Attachment #2: 0001-net-phy-Add-drivers-for-Microsemi-PHYs.patch --] [-- Type: application/octet-stream, Size: 6722 bytes --] From d75dce082d229595211e8e7106a19c967e5ce07f Mon Sep 17 00:00:00 2001 From: "Nagaraju Lakkaraju" <raju.lakkaraju@microsemi.com> Date: Thu, 21 Jul 2016 12:47:54 +0200 Subject: [PATCH] net: phy: Add drivers for Microsemi PHYs Adding support for Microsemi PHYs: VSC8531 and VSC8541. Signed-off-by: Nagaraju Lakkaraju <raju.lakkaraju@microsemi.com> --- drivers/net/phy/Kconfig | 5 ++ drivers/net/phy/Makefile | 1 + drivers/net/phy/mscc.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 189 insertions(+) create mode 100644 drivers/net/phy/mscc.c diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index 6dad9a9..9d1bd0e 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -271,6 +271,11 @@ config MDIO_BCM_IPROC This module provides a driver for the MDIO busses found in the Broadcom iProc 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 fcdbb92..f98ac90 100644 --- a/drivers/net/phy/Makefile +++ b/drivers/net/phy/Makefile @@ -10,6 +10,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 0000000..605e91c --- /dev/null +++ b/drivers/net/phy/mscc.c @@ -0,0 +1,183 @@ +/* + * Driver for Microsemi VSC85xx PHYs + * + * Author: Nagaraju Lakkaraju + * License: Dual MIT/GPL + * Copyright (c) 2016 Microsemi Corporation + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/mdio.h> +#include <linux/mii.h> +#include <linux/phy.h> + +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_STATUS 26 + +#define MSCC_EXT_PAGE_ACCESS 31 +#define MSCC_PHY_PAGE_STANDARD 0x0000 /* Standard registers */ +#define MSCC_PHY_PAGE_EXTENDED_2 0x0002 /* Extended registers - page 2 */ + +/* Extended Page 2 Registers */ +#define MSCC_PHY_RGMII_CNTL 20 +#define RGMII_RX_CLK_DELAY_MASK 0x0070 +#define RGMII_RX_CLK_DELAY_POS 4 + +/* Microsemi PHY ID's */ +#define PHY_ID_VSC8531 0x00070570 +#define PHY_ID_VSC8541 0x00070770 + +/* RGMII Rx Clock delay value change with board lay-out */ +static u8 rgmii_rx_clk_delay = RGMII_RX_CLK_DELAY_1_1_NS; + +static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page) +{ + int rc = 0; + + rc = phy_write(phydev, MSCC_EXT_PAGE_ACCESS, page); + return rc; +} + +static int vsc85xx_default_config(struct phy_device *phydev) +{ + int rc = 0; + u16 reg_val = 0; + + 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); + + 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; + + mutex_lock(&phydev->lock); + rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2); + if (rc != 0) { + rc = -EINVAL; + 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 << RGMII_RX_CLK_DELAY_POS); + phy_write(phydev, MSCC_PHY_RGMII_CNTL, reg_val); + rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD); + if (rc != 0) + rc = -EINVAL; + +out_unlock: + mutex_unlock(&phydev->lock); + + return rc; +} + +static int vsc85xx_config_init(struct phy_device *phydev) +{ + int rc = 0; + + rc = vsc85xx_default_config(phydev); + rc = genphy_config_init(phydev); + + return rc; +} + +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; + + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) { + rc = phy_write(phydev, MII_VSC85XX_INT_MASK, + MII_VSC85XX_INT_MASK_MASK); + } else { + rc = phy_read(phydev, MII_VSC85XX_INT_STATUS); + if (rc < 0) + return rc; + rc = phy_write(phydev, MII_VSC85XX_INT_MASK, 0); + } + + return rc; +} + +/* Microsemi VSC85xx PHYs */ +static struct phy_driver vsc85xx_driver[] = { +{ + .phy_id = PHY_ID_VSC8531, + .name = "Microsemi VSC8531", + .phy_id_mask = 0xfffffff0, + .features = PHY_GBIT_FEATURES, + .flags = PHY_HAS_INTERRUPT, + .soft_reset = &genphy_soft_reset, + .config_init = &vsc85xx_config_init, + .config_aneg = &genphy_config_aneg, + .aneg_done = &genphy_aneg_done, + .read_status = &genphy_read_status, + .ack_interrupt = &vsc85xx_ack_interrupt, + .config_intr = &vsc85xx_config_intr, + .suspend = &genphy_suspend, + .resume = &genphy_resume, +}, +{ + .phy_id = PHY_ID_VSC8541, + .name = "Microsemi VSC8541 SyncE", + .phy_id_mask = 0xfffffff0, + .features = PHY_GBIT_FEATURES, + .flags = PHY_HAS_INTERRUPT, + .soft_reset = &genphy_soft_reset, + .config_init = &vsc85xx_config_init, + .config_aneg = &genphy_config_aneg, + .aneg_done = &genphy_aneg_done, + .read_status = &genphy_read_status, + .ack_interrupt = &vsc85xx_ack_interrupt, + .config_intr = &vsc85xx_config_intr, + .suspend = &genphy_suspend, + .resume = &genphy_resume, +} + +}; + +module_phy_driver(vsc85xx_driver); + +static struct mdio_device_id __maybe_unused vsc85xx_tbl[] = { + { PHY_ID_VSC8531, 0xfffffff0, }, + { PHY_ID_VSC8541, 0xfffffff0, }, + { } +}; + +MODULE_DEVICE_TABLE(mdio, vsc85xx_tbl); + +MODULE_DESCRIPTION("Microsemi VSC85xx PHY driver"); +MODULE_AUTHOR("Nagaraju Lakkaraju"); +MODULE_LICENSE("Dual MIT/GPL"); -- 2.7.3 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: Microsemi VSC 8531/41 PHY Driver 2016-07-26 9:49 Microsemi VSC 8531/41 PHY Driver Raju Lakkaraju @ 2016-07-26 11:55 ` Andrew Lunn 2016-07-26 12:20 ` Raju Lakkaraju 0 siblings, 1 reply; 15+ messages in thread From: Andrew Lunn @ 2016-07-26 11:55 UTC (permalink / raw) To: Raju Lakkaraju Cc: netdev@vger.kernel.org, f.fainelli@gmail.com, Allan Nielsen On Tue, Jul 26, 2016 at 09:49:53AM +0000, Raju Lakkaraju wrote: > Hello All, > > I would like to introduce myself as Nagaraju Lakkaraju (Raju), is working in Microsemi Communications India Pvt. Ltd (Formerly known as Vitesse Semiconductors Limited) - Hyderabad as Sr. Staff Engineer. > I do work on Microsemi PHY drivers development. > Microsemi is developing the new Physical Layer (PHY) chips for Internet Of Things (IoT) products with 1 Gbps link speed. > As part of the development, Microsemi would like to contribute the new PHYs (i.e. VSC 8531 / VSC 8541) chip drivers in Linux Kernel Open source. > VSC 8531 / 8541 PHYs will have the following features a part of the basic features like Auto-neg, Speed, Duplex etc. > 1. Wake on LAN > 2. Auto MDIX/MDI > 3. Link Speed Down shift > 4. Fast Link Failure-2 > 5. Loopbacks (FAR-END, NEAR-END and Connector) > 6. Ethernet Packet Generator (EPG) > 7. Serial Management Interface (SMI) Interrupt > 8. Clock Squelch configuration (SyncE) > 9. Jumbo Frame Support > 10. In-line Power On Ethernet (PoE) status > 11. Acti PHY power Management > 12. Energy Efficiency Ethenet (EEE) > 13. VeriPHY (Cable Diagnostics) > 14. LED configuration > 15. Ring Resiliency > 16. Start Of Frame Detection (SOF) > 17. COMA mode Some interesting features. Is the datasheet publicly available? > As part of Initial submission of the Linux Kernel Open source drivers, > I developed the VSC 8531 driver basic features and built the Linux Kernel image for Beaglebone Black hardware. > Also developed Ethtool enhancement for VSC 8531 register access functionality to test the VSC 8531 > > Test Setup: > ------------- > Hardware Details: Beaglebone Black with VSC 8531 PHY > Software Linux Kernel version: 4.6.4 > > Microsemi VSC 8531 chip is mount on Beaglebone Black hardware (replaced the Microchip PHY) and tested the following features. > 1. Auto negotiation > 2. Speed change ( 10 Mbps, 100 Mbps and 1 Gbps) > 3. Full/Half Duplex > 4. Ping > 5. Line rate traffic with Test center > > I would like you to review my code and provide me the valuable comments. > Please find the attached git code diff patch. I would like to review your code, but please read the SubmittingPathes document and in particularly, the bit about attachments. Andrew ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: Microsemi VSC 8531/41 PHY Driver 2016-07-26 11:55 ` Andrew Lunn @ 2016-07-26 12:20 ` Raju Lakkaraju 2016-07-26 12:43 ` Andrew Lunn 0 siblings, 1 reply; 15+ messages in thread From: Raju Lakkaraju @ 2016-07-26 12:20 UTC (permalink / raw) To: Andrew Lunn; +Cc: netdev@vger.kernel.org, f.fainelli@gmail.com, Allan Nielsen Hello Andrew, VSC 8531 / 8541 is not yet release to Public. Now It's in draft version. After commit the VSC 8531 PHY drivers in Linux Kernel Open source, Microsemi will release the VSC 8531/8541 data sheet to Public. My apologies. I missed instructions about attachments in SubmittingPathes document. VSC 8531/8541 PHY Driver code patch inline. >From d75dce082d229595211e8e7106a19c967e5ce07f Mon Sep 17 00:00:00 2001 From: "Nagaraju Lakkaraju" <raju.lakkaraju@microsemi.com> Date: Thu, 21 Jul 2016 12:47:54 +0200 Subject: [PATCH] net: phy: Add drivers for Microsemi PHYs Adding support for Microsemi PHYs: VSC8531 and VSC8541. Signed-off-by: Nagaraju Lakkaraju <raju.lakkaraju@microsemi.com> --- drivers/net/phy/Kconfig | 5 ++ drivers/net/phy/Makefile | 1 + drivers/net/phy/mscc.c | 183 +++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 189 insertions(+) create mode 100644 drivers/net/phy/mscc.c diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index 6dad9a9..9d1bd0e 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -271,6 +271,11 @@ config MDIO_BCM_IPROC This module provides a driver for the MDIO busses found in the Broadcom iProc 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 fcdbb92..f98ac90 100644 --- a/drivers/net/phy/Makefile +++ b/drivers/net/phy/Makefile @@ -10,6 +10,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 0000000..605e91c --- /dev/null +++ b/drivers/net/phy/mscc.c @@ -0,0 +1,183 @@ +/* + * Driver for Microsemi VSC85xx PHYs + * + * Author: Nagaraju Lakkaraju + * License: Dual MIT/GPL + * Copyright (c) 2016 Microsemi Corporation + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/mdio.h> +#include <linux/mii.h> +#include <linux/phy.h> + +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_STATUS 26 + +#define MSCC_EXT_PAGE_ACCESS 31 +#define MSCC_PHY_PAGE_STANDARD 0x0000 /* Standard registers */ +#define MSCC_PHY_PAGE_EXTENDED_2 0x0002 /* Extended registers - page 2 */ + +/* Extended Page 2 Registers */ +#define MSCC_PHY_RGMII_CNTL 20 +#define RGMII_RX_CLK_DELAY_MASK 0x0070 +#define RGMII_RX_CLK_DELAY_POS 4 + +/* Microsemi PHY ID's */ +#define PHY_ID_VSC8531 0x00070570 +#define PHY_ID_VSC8541 0x00070770 + +/* RGMII Rx Clock delay value change with board lay-out */ +static u8 rgmii_rx_clk_delay = RGMII_RX_CLK_DELAY_1_1_NS; + +static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page) +{ + int rc = 0; + + rc = phy_write(phydev, MSCC_EXT_PAGE_ACCESS, page); + return rc; +} + +static int vsc85xx_default_config(struct phy_device *phydev) +{ + int rc = 0; + u16 reg_val = 0; + + 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); + + 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; + + mutex_lock(&phydev->lock); + rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2); + if (rc != 0) { + rc = -EINVAL; + 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 << RGMII_RX_CLK_DELAY_POS); + phy_write(phydev, MSCC_PHY_RGMII_CNTL, reg_val); + rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD); + if (rc != 0) + rc = -EINVAL; + +out_unlock: + mutex_unlock(&phydev->lock); + + return rc; +} + +static int vsc85xx_config_init(struct phy_device *phydev) +{ + int rc = 0; + + rc = vsc85xx_default_config(phydev); + rc = genphy_config_init(phydev); + + return rc; +} + +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; + + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) { + rc = phy_write(phydev, MII_VSC85XX_INT_MASK, + MII_VSC85XX_INT_MASK_MASK); + } else { + rc = phy_read(phydev, MII_VSC85XX_INT_STATUS); + if (rc < 0) + return rc; + rc = phy_write(phydev, MII_VSC85XX_INT_MASK, 0); + } + + return rc; +} + +/* Microsemi VSC85xx PHYs */ +static struct phy_driver vsc85xx_driver[] = { +{ + .phy_id = PHY_ID_VSC8531, + .name = "Microsemi VSC8531", + .phy_id_mask = 0xfffffff0, + .features = PHY_GBIT_FEATURES, + .flags = PHY_HAS_INTERRUPT, + .soft_reset = &genphy_soft_reset, + .config_init = &vsc85xx_config_init, + .config_aneg = &genphy_config_aneg, + .aneg_done = &genphy_aneg_done, + .read_status = &genphy_read_status, + .ack_interrupt = &vsc85xx_ack_interrupt, + .config_intr = &vsc85xx_config_intr, + .suspend = &genphy_suspend, + .resume = &genphy_resume, +}, +{ + .phy_id = PHY_ID_VSC8541, + .name = "Microsemi VSC8541 SyncE", + .phy_id_mask = 0xfffffff0, + .features = PHY_GBIT_FEATURES, + .flags = PHY_HAS_INTERRUPT, + .soft_reset = &genphy_soft_reset, + .config_init = &vsc85xx_config_init, + .config_aneg = &genphy_config_aneg, + .aneg_done = &genphy_aneg_done, + .read_status = &genphy_read_status, + .ack_interrupt = &vsc85xx_ack_interrupt, + .config_intr = &vsc85xx_config_intr, + .suspend = &genphy_suspend, + .resume = &genphy_resume, +} + +}; + +module_phy_driver(vsc85xx_driver); + +static struct mdio_device_id __maybe_unused vsc85xx_tbl[] = { + { PHY_ID_VSC8531, 0xfffffff0, }, + { PHY_ID_VSC8541, 0xfffffff0, }, + { } +}; + +MODULE_DEVICE_TABLE(mdio, vsc85xx_tbl); + +MODULE_DESCRIPTION("Microsemi VSC85xx PHY driver"); +MODULE_AUTHOR("Nagaraju Lakkaraju"); +MODULE_LICENSE("Dual MIT/GPL"); -- 2.7.3 Thanks and regards, Raju (Nagaraju Lakkaraju) Sr. Staff Engg. Microsemi Communications India Pvt Ltd. Ph: +91 040 6686 0132 -----Original Message----- From: Andrew Lunn [mailto:andrew@lunn.ch] Sent: Tuesday, July 26, 2016 5:26 PM To: Raju Lakkaraju Cc: netdev@vger.kernel.org; f.fainelli@gmail.com; Allan Nielsen Subject: Re: Microsemi VSC 8531/41 PHY Driver EXTERNAL EMAIL On Tue, Jul 26, 2016 at 09:49:53AM +0000, Raju Lakkaraju wrote: > Hello All, > > I would like to introduce myself as Nagaraju Lakkaraju (Raju), is working in Microsemi Communications India Pvt. Ltd (Formerly known as Vitesse Semiconductors Limited) - Hyderabad as Sr. Staff Engineer. > I do work on Microsemi PHY drivers development. > Microsemi is developing the new Physical Layer (PHY) chips for Internet Of Things (IoT) products with 1 Gbps link speed. > As part of the development, Microsemi would like to contribute the new PHYs (i.e. VSC 8531 / VSC 8541) chip drivers in Linux Kernel Open source. > VSC 8531 / 8541 PHYs will have the following features a part of the basic features like Auto-neg, Speed, Duplex etc. > 1. Wake on LAN > 2. Auto MDIX/MDI > 3. Link Speed Down shift > 4. Fast Link Failure-2 > 5. Loopbacks (FAR-END, NEAR-END and Connector) 6. Ethernet Packet > Generator (EPG) 7. Serial Management Interface (SMI) Interrupt 8. > Clock Squelch configuration (SyncE) 9. Jumbo Frame Support 10. In-line > Power On Ethernet (PoE) status 11. Acti PHY power Management 12. > Energy Efficiency Ethenet (EEE) 13. VeriPHY (Cable Diagnostics) 14. > LED configuration 15. Ring Resiliency 16. Start Of Frame Detection > (SOF) 17. COMA mode Some interesting features. Is the datasheet publicly available? > As part of Initial submission of the Linux Kernel Open source drivers, > I developed the VSC 8531 driver basic features and built the Linux Kernel image for Beaglebone Black hardware. > Also developed Ethtool enhancement for VSC 8531 register access > functionality to test the VSC 8531 > > Test Setup: > ------------- > Hardware Details: Beaglebone Black with VSC 8531 PHY Software Linux > Kernel version: 4.6.4 > > Microsemi VSC 8531 chip is mount on Beaglebone Black hardware (replaced the Microchip PHY) and tested the following features. > 1. Auto negotiation > 2. Speed change ( 10 Mbps, 100 Mbps and 1 Gbps) 3. Full/Half Duplex 4. > Ping 5. Line rate traffic with Test center > > I would like you to review my code and provide me the valuable comments. > Please find the attached git code diff patch. I would like to review your code, but please read the SubmittingPathes document and in particularly, the bit about attachments. Andrew ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: Microsemi VSC 8531/41 PHY Driver 2016-07-26 12:20 ` Raju Lakkaraju @ 2016-07-26 12:43 ` Andrew Lunn 2016-07-28 6:44 ` Raju Lakkaraju 0 siblings, 1 reply; 15+ messages in thread From: Andrew Lunn @ 2016-07-26 12:43 UTC (permalink / raw) To: Raju Lakkaraju Cc: netdev@vger.kernel.org, f.fainelli@gmail.com, Allan Nielsen > +/* 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. > + 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); > + > + 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. > + > + mutex_lock(&phydev->lock); What are you locking against? > + rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2); > + if (rc != 0) { > + rc = -EINVAL; Why do you overwrite the error code vsc85xx_phy_page_set gives you? > + 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 << RGMII_RX_CLK_DELAY_POS); > + phy_write(phydev, MSCC_PHY_RGMII_CNTL, reg_val); > + rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD); > + if (rc != 0) > + rc = -EINVAL; Same here. > + > +out_unlock: > + mutex_unlock(&phydev->lock); > + > + return rc; > +} > + > +static int vsc85xx_config_init(struct phy_device *phydev) > +{ > + int rc = 0; No need to initialise rc. > + rc = vsc85xx_default_config(phydev); if (rc) return rc; > + rc = genphy_config_init(phydev); > + > + return rc; Or just return genphy_config_init(phydev); > +} > + > +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; > + > + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) { > + rc = phy_write(phydev, MII_VSC85XX_INT_MASK, > + MII_VSC85XX_INT_MASK_MASK); > + } else { > + rc = phy_read(phydev, MII_VSC85XX_INT_STATUS); > + if (rc < 0) > + return rc; And the purpose of this read is? I assume it clears an outstanding interrupt? If so, shouldn't you do it after disabling interrupts, not before? Otherwise you have a race condition. > + rc = phy_write(phydev, MII_VSC85XX_INT_MASK, 0); > + } > + > + return rc; Andrew ^ permalink raw reply [flat|nested] 15+ messages in thread
* RE: Microsemi VSC 8531/41 PHY Driver 2016-07-26 12:43 ` Andrew Lunn @ 2016-07-28 6:44 ` Raju Lakkaraju 2016-07-28 17:35 ` Florian Fainelli ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Raju Lakkaraju @ 2016-07-28 6:44 UTC (permalink / raw) To: Andrew Lunn; +Cc: netdev@vger.kernel.org, f.fainelli@gmail.com, Allan Nielsen Hello Andrew, Thank you for given valuable comments. Please see the my responses inline. Thanks, Raju -----Original Message----- From: Andrew Lunn [mailto:andrew@lunn.ch] Sent: Tuesday, July 26, 2016 6:14 PM To: Raju Lakkaraju Cc: netdev@vger.kernel.org; f.fainelli@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. 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. > + 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); > + > + 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. > + > + mutex_lock(&phydev->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. > + rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2); > + if (rc != 0) { > + rc = -EINVAL; Why do you overwrite the error code vsc85xx_phy_page_set gives you? Raju: initially I would like to create new type of Error code. Then, I decided to use existing one. I accept your comment. I will remove the code. > + 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 << RGMII_RX_CLK_DELAY_POS); > + phy_write(phydev, MSCC_PHY_RGMII_CNTL, reg_val); > + rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD); > + if (rc != 0) > + rc = -EINVAL; Same here. Raju: I accept your comment. I will remove the code. > + > +out_unlock: > + mutex_unlock(&phydev->lock); > + > + return rc; > +} > + > +static int vsc85xx_config_init(struct phy_device *phydev) { > + int rc = 0; No need to initialise rc. Raju: I accept your comment. I will remove the code. > + rc = vsc85xx_default_config(phydev); if (rc) return rc; > + rc = genphy_config_init(phydev); > + > + return rc; Or just return genphy_config_init(phydev); Raju: I accept your comment. I will remove the code. > +} > + > +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; > + > + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) { > + rc = phy_write(phydev, MII_VSC85XX_INT_MASK, > + MII_VSC85XX_INT_MASK_MASK); > + } else { > + rc = phy_read(phydev, MII_VSC85XX_INT_STATUS); > + if (rc < 0) > + return rc; And the purpose of this read is? I assume it clears an outstanding interrupt? If so, shouldn't you do it after disabling interrupts, not before? Otherwise you have a race condition. Raju: The Interrupt status register is read on clean. When, PHY_INTERRUPT_DISABLE case, I should make sure that status should be clear. If I read the Interrupt status registers, it clears all preexisting interrupts. > + rc = phy_write(phydev, MII_VSC85XX_INT_MASK, 0); > + } > + > + return rc; Andrew ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Microsemi VSC 8531/41 PHY Driver 2016-07-28 6:44 ` Raju Lakkaraju @ 2016-07-28 17:35 ` Florian Fainelli 2016-08-04 9:31 ` Nagaraju Lakkaraju 2016-07-29 8:04 ` Andrew Lunn 2016-07-29 8:17 ` Andrew Lunn 2 siblings, 1 reply; 15+ messages in thread From: Florian Fainelli @ 2016-07-28 17:35 UTC (permalink / raw) To: Raju Lakkaraju, Andrew Lunn; +Cc: netdev@vger.kernel.org, Allan Nielsen 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:andrew@lunn.ch] > Sent: Tuesday, July 26, 2016 6:14 PM > To: Raju Lakkaraju > Cc: netdev@vger.kernel.org; f.fainelli@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. > 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. > >> + 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. >> + >> + 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. > >> + >> + mutex_lock(&phydev->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] >> + >> +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; >> + >> + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) { >> + rc = phy_write(phydev, MII_VSC85XX_INT_MASK, >> + MII_VSC85XX_INT_MASK_MASK); >> + } else { >> + rc = phy_read(phydev, MII_VSC85XX_INT_STATUS); >> + if (rc < 0) >> + return rc; > > And the purpose of this read is? I assume it clears an outstanding interrupt? If so, shouldn't you do it after disabling interrupts, not before? Otherwise you have a race condition. > > Raju: The Interrupt status register is read on clean. When, PHY_INTERRUPT_DISABLE case, > I should make sure that status should be clear. If I read the Interrupt status registers, it clears all preexisting interrupts. Should not you clear the interrupt status irrespective of whether PHY interrupts will be enabled or not as a first operation? Then later on, if the PHY needs to enable interrupt or not, this can be based on PHY_INTERRUPT_ENABLED. -- Florian ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Microsemi VSC 8531/41 PHY Driver 2016-07-28 17:35 ` Florian Fainelli @ 2016-08-04 9:31 ` Nagaraju Lakkaraju 0 siblings, 0 replies; 15+ messages in thread From: Nagaraju Lakkaraju @ 2016-08-04 9:31 UTC (permalink / raw) To: Florian Fainelli; +Cc: Andrew Lunn, netdev@vger.kernel.org, Allan Nielsen 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:andrew@lunn.ch] > > Sent: Tuesday, July 26, 2016 6:14 PM > > To: Raju Lakkaraju > > Cc: netdev@vger.kernel.org; f.fainelli@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(&phydev->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; > >> + > >> + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) { > >> + rc = phy_write(phydev, MII_VSC85XX_INT_MASK, > >> + MII_VSC85XX_INT_MASK_MASK); > >> + } else { > >> + rc = phy_read(phydev, MII_VSC85XX_INT_STATUS); > >> + if (rc < 0) > >> + return rc; > > > > And the purpose of this read is? I assume it clears an outstanding interrupt? If so, shouldn't you do it after disabling interrupts, not before? Otherwise you have a race condition. > > > > Raju: The Interrupt status register is read on clean. When, PHY_INTERRUPT_DISABLE case, > > I should make sure that status should be clear. If I read the Interrupt status registers, it clears all preexisting interrupts. > > Should not you clear the interrupt status irrespective of whether PHY > interrupts will be enabled or not as a first operation? Then later on, > if the PHY needs to enable interrupt or not, this can be based on > PHY_INTERRUPT_ENABLED. In PHY_INTERRUPT_DISABLE case, after disable the interrupts, I would like to clear the interrupt status. > -- > Florian ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Microsemi VSC 8531/41 PHY Driver 2016-07-28 6:44 ` Raju Lakkaraju 2016-07-28 17:35 ` Florian Fainelli @ 2016-07-29 8:04 ` Andrew Lunn 2016-07-29 8:17 ` Andrew Lunn 2 siblings, 0 replies; 15+ messages in thread From: Andrew Lunn @ 2016-07-29 8:04 UTC (permalink / raw) To: Raju Lakkaraju Cc: netdev@vger.kernel.org, f.fainelli@gmail.com, Allan Nielsen On Thu, Jul 28, 2016 at 06:44:37AM +0000, 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:andrew@lunn.ch] > Sent: Tuesday, July 26, 2016 6:14 PM > To: Raju Lakkaraju > Cc: netdev@vger.kernel.org; f.fainelli@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. > 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. Hi Raju Please can you use standard email quoting, just like everybody else does. Andrew ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Microsemi VSC 8531/41 PHY Driver 2016-07-28 6:44 ` Raju Lakkaraju 2016-07-28 17:35 ` Florian Fainelli 2016-07-29 8:04 ` Andrew Lunn @ 2016-07-29 8:17 ` Andrew Lunn 2016-08-04 9:17 ` Nagaraju Lakkaraju 2016-08-05 12:24 ` Nagaraju Lakkaraju 2 siblings, 2 replies; 15+ messages in thread From: Andrew Lunn @ 2016-07-29 8:17 UTC (permalink / raw) To: Raju Lakkaraju Cc: netdev@vger.kernel.org, f.fainelli@gmail.com, Allan Nielsen > > +/* 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. > 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. Andrew ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Microsemi VSC 8531/41 PHY Driver 2016-07-29 8:17 ` Andrew Lunn @ 2016-08-04 9:17 ` Nagaraju Lakkaraju 2016-08-05 12:24 ` Nagaraju Lakkaraju 1 sibling, 0 replies; 15+ messages in thread From: Nagaraju Lakkaraju @ 2016-08-04 9:17 UTC (permalink / raw) To: Andrew Lunn; +Cc: netdev@vger.kernel.org, f.fainelli@gmail.com, Allan Nielsen 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Microsemi VSC 8531/41 PHY Driver 2016-07-29 8:17 ` Andrew Lunn 2016-08-04 9:17 ` Nagaraju Lakkaraju @ 2016-08-05 12:24 ` Nagaraju Lakkaraju 2016-08-16 19:41 ` Andrew Lunn 2016-08-16 19:48 ` Andrew Lunn 1 sibling, 2 replies; 15+ messages in thread From: Nagaraju Lakkaraju @ 2016-08-05 12:24 UTC (permalink / raw) To: Andrew Lunn, f.fainelli@gmail.com; +Cc: netdev@vger.kernel.org, Allan Nielsen Hello, I added all review comments and re-sending for review. >From a5017f5878a92d2acec86a6a29b1498c457cb73a Mon Sep 17 00:00:00 2001 From: Nagaraju Lakkaraju <Raju.Lakkaraju@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.Lakkaraju@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 0000000..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 <linux/kernel.h> +#include <linux/module.h> +#include <linux/mdio.h> +#include <linux/mii.h> +#include <linux/phy.h> + +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_STATUS 26 + +#define MSCC_EXT_PAGE_ACCESS 31 +#define MSCC_PHY_PAGE_STANDARD 0x0000 /* 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_POS 4 + +/* Microsemi PHY ID's */ +#define PHY_ID_VSC8531 0x00070570 +#define PHY_ID_VSC8541 0x00070770 + +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(&phydev->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(&phydev->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, + .name = "Microsemi VSC8531", + .phy_id_mask = 0xfffffff0, + .features = PHY_GBIT_FEATURES, + .flags = PHY_HAS_INTERRUPT, + .soft_reset = &genphy_soft_reset, + .config_init = &vsc85xx_config_init, + .config_aneg = &genphy_config_aneg, + .aneg_done = &genphy_aneg_done, + .read_status = &genphy_read_status, + .ack_interrupt = &vsc85xx_ack_interrupt, + .config_intr = &vsc85xx_config_intr, + .suspend = &genphy_suspend, + .resume = &genphy_resume, +}, +{ + .phy_id = PHY_ID_VSC8541, + .name = "Microsemi VSC8541 SyncE", + .phy_id_mask = 0xfffffff0, + .features = PHY_GBIT_FEATURES, + .flags = PHY_HAS_INTERRUPT, + .soft_reset = &genphy_soft_reset, + .config_init = &vsc85xx_config_init, + .config_aneg = &genphy_config_aneg, + .aneg_done = &genphy_aneg_done, + .read_status = &genphy_read_status, + .ack_interrupt = &vsc85xx_ack_interrupt, + .config_intr = &vsc85xx_config_intr, + .suspend = &genphy_suspend, + .resume = &genphy_resume, +} + +}; + +module_phy_driver(vsc85xx_driver); + +static struct mdio_device_id __maybe_unused vsc85xx_tbl[] = { + { PHY_ID_VSC8531, 0xfffffff0, }, + { PHY_ID_VSC8541, 0xfffffff0, }, + { } +}; + +MODULE_DEVICE_TABLE(mdio, vsc85xx_tbl); + +MODULE_DESCRIPTION("Microsemi VSC85xx PHY driver"); +MODULE_AUTHOR("Nagaraju Lakkaraju"); +MODULE_LICENSE("Dual MIT/GPL"); -- 2.1.0 -- Thanks, Raju 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. > > > 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. > > Andrew ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: Microsemi VSC 8531/41 PHY Driver 2016-08-05 12:24 ` Nagaraju Lakkaraju @ 2016-08-16 19:41 ` Andrew Lunn 2016-08-16 19:48 ` Andrew Lunn 1 sibling, 0 replies; 15+ messages in thread From: Andrew Lunn @ 2016-08-16 19:41 UTC (permalink / raw) To: Nagaraju Lakkaraju Cc: f.fainelli@gmail.com, netdev@vger.kernel.org, Allan Nielsen On Fri, Aug 05, 2016 at 05:54:21PM +0530, Nagaraju Lakkaraju wrote: > Hello, > > I added all review comments and re-sending for review. > > >From a5017f5878a92d2acec86a6a29b1498c457cb73a Mon Sep 17 00:00:00 2001 > From: Nagaraju Lakkaraju <Raju.Lakkaraju@microsemi.com> > Date: Wed, 3 Aug 2016 18:28:24 +0530 > Subject: [PATCH v2] net: phy: Add drivers for Microsemi PHYs Please use git send-email to send patches. All comments which should not be committed to the change log should appear after the ---. It is normal to briefly list changes between this version and the previous version. > +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; Please remove rc and just do return phy_write(phydev, MSCC_EXT_PAGE_ACCESS, page); > +} > + > +static int vsc85xx_default_config(struct phy_device *phydev) This function is setting the RGMII delay. So vsc85xx_default_config() is not a particularly good name. Andrew ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: Microsemi VSC 8531/41 PHY Driver 2016-08-05 12:24 ` Nagaraju Lakkaraju 2016-08-16 19:41 ` Andrew Lunn @ 2016-08-16 19:48 ` Andrew Lunn 2016-09-08 8:39 ` [PATCH net-next 1/1] net: phy: Fixed checkpatch errors for Microsemi PHYs Raju Lakkaraju 1 sibling, 1 reply; 15+ messages in thread From: Andrew Lunn @ 2016-08-16 19:48 UTC (permalink / raw) To: Nagaraju Lakkaraju Cc: f.fainelli@gmail.com, netdev@vger.kernel.org, Allan Nielsen On Fri, Aug 05, 2016 at 05:54:21PM +0530, Nagaraju Lakkaraju wrote: > Hello, > > I added all review comments and re-sending for review. You should also take a look at the output of scripts/checkpatch.pl: total: 9 errors, 82 warnings, 1 checks, 179 lines checked Andrew ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH net-next 1/1] net: phy: Fixed checkpatch errors for Microsemi PHYs. 2016-08-16 19:48 ` Andrew Lunn @ 2016-09-08 8:39 ` Raju Lakkaraju 2016-09-10 1:16 ` David Miller 0 siblings, 1 reply; 15+ messages in thread From: Raju Lakkaraju @ 2016-09-08 8:39 UTC (permalink / raw) To: netdev; +Cc: f.fainelli, Allan.Nielsen, andrew, Raju Lakkaraju From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com> The existing VSC85xx PHY driver did not follow the coding style and caused "checkpatch" to complain. This commit fixes this. Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com> --- drivers/net/phy/Kconfig | 6 +- drivers/net/phy/mscc.c | 178 ++++++++++++++++++++++++------------------------ 2 files changed, 92 insertions(+), 92 deletions(-) diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index 1c3e07c..87b566f 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -274,9 +274,9 @@ config MICROCHIP_PHY Supports the LAN88XX PHYs. config MICROSEMI_PHY - tristate "Microsemi PHYs" - ---help--- - Currently supports the VSC8531 and VSC8541 PHYs + tristate "Microsemi PHYs" + ---help--- + Currently supports the VSC8531 and VSC8541 PHYs config NATIONAL_PHY tristate "National Semiconductor PHYs" diff --git a/drivers/net/phy/mscc.c b/drivers/net/phy/mscc.c index ad33390..c09cc4a 100644 --- a/drivers/net/phy/mscc.c +++ b/drivers/net/phy/mscc.c @@ -13,135 +13,135 @@ #include <linux/phy.h> 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 + 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_STATUS 26 +#define MII_VSC85XX_INT_MASK 25 +#define MII_VSC85XX_INT_MASK_MASK 0xa000 +#define MII_VSC85XX_INT_STATUS 26 -#define MSCC_EXT_PAGE_ACCESS 31 -#define MSCC_PHY_PAGE_STANDARD 0x0000 /* Standard registers */ -#define MSCC_PHY_PAGE_EXTENDED_2 0x0002 /* Extended reg - page 2 */ +#define MSCC_EXT_PAGE_ACCESS 31 +#define MSCC_PHY_PAGE_STANDARD 0x0000 /* 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_POS 4 +#define MSCC_PHY_RGMII_CNTL 20 +#define RGMII_RX_CLK_DELAY_MASK 0x0070 +#define RGMII_RX_CLK_DELAY_POS 4 /* Microsemi PHY ID's */ -#define PHY_ID_VSC8531 0x00070570 -#define PHY_ID_VSC8541 0x00070770 +#define PHY_ID_VSC8531 0x00070570 +#define PHY_ID_VSC8541 0x00070770 static int vsc85xx_phy_page_set(struct phy_device *phydev, u8 page) { - int rc; + int rc; - rc = phy_write(phydev, MSCC_EXT_PAGE_ACCESS, page); - return 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; + int rc; + u16 reg_val; - mutex_lock(&phydev->lock); - rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_EXTENDED_2); - if (rc != 0) - goto out_unlock; + mutex_lock(&phydev->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); + 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(&phydev->lock); + mutex_unlock(&phydev->lock); - return rc; + return rc; } static int vsc85xx_config_init(struct phy_device *phydev) { - int rc; + int rc; - rc = vsc85xx_default_config(phydev); - if (rc) - return rc; - rc = genphy_config_init(phydev); + rc = vsc85xx_default_config(phydev); + if (rc) + return rc; + rc = genphy_config_init(phydev); - return rc; + return rc; } static int vsc85xx_ack_interrupt(struct phy_device *phydev) { - int rc = 0; + int rc = 0; - if (phydev->interrupts == PHY_INTERRUPT_ENABLED) - rc = phy_read(phydev, MII_VSC85XX_INT_STATUS); + if (phydev->interrupts == PHY_INTERRUPT_ENABLED) + rc = phy_read(phydev, MII_VSC85XX_INT_STATUS); - return (rc < 0) ? rc : 0; + 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; + 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, - .name = "Microsemi VSC8531", - .phy_id_mask = 0xfffffff0, - .features = PHY_GBIT_FEATURES, - .flags = PHY_HAS_INTERRUPT, - .soft_reset = &genphy_soft_reset, - .config_init = &vsc85xx_config_init, - .config_aneg = &genphy_config_aneg, - .aneg_done = &genphy_aneg_done, - .read_status = &genphy_read_status, - .ack_interrupt = &vsc85xx_ack_interrupt, - .config_intr = &vsc85xx_config_intr, - .suspend = &genphy_suspend, - .resume = &genphy_resume, + .phy_id = PHY_ID_VSC8531, + .name = "Microsemi VSC8531", + .phy_id_mask = 0xfffffff0, + .features = PHY_GBIT_FEATURES, + .flags = PHY_HAS_INTERRUPT, + .soft_reset = &genphy_soft_reset, + .config_init = &vsc85xx_config_init, + .config_aneg = &genphy_config_aneg, + .aneg_done = &genphy_aneg_done, + .read_status = &genphy_read_status, + .ack_interrupt = &vsc85xx_ack_interrupt, + .config_intr = &vsc85xx_config_intr, + .suspend = &genphy_suspend, + .resume = &genphy_resume, }, { - .phy_id = PHY_ID_VSC8541, - .name = "Microsemi VSC8541 SyncE", - .phy_id_mask = 0xfffffff0, - .features = PHY_GBIT_FEATURES, - .flags = PHY_HAS_INTERRUPT, - .soft_reset = &genphy_soft_reset, - .config_init = &vsc85xx_config_init, - .config_aneg = &genphy_config_aneg, - .aneg_done = &genphy_aneg_done, - .read_status = &genphy_read_status, - .ack_interrupt = &vsc85xx_ack_interrupt, - .config_intr = &vsc85xx_config_intr, - .suspend = &genphy_suspend, - .resume = &genphy_resume, + .phy_id = PHY_ID_VSC8541, + .name = "Microsemi VSC8541 SyncE", + .phy_id_mask = 0xfffffff0, + .features = PHY_GBIT_FEATURES, + .flags = PHY_HAS_INTERRUPT, + .soft_reset = &genphy_soft_reset, + .config_init = &vsc85xx_config_init, + .config_aneg = &genphy_config_aneg, + .aneg_done = &genphy_aneg_done, + .read_status = &genphy_read_status, + .ack_interrupt = &vsc85xx_ack_interrupt, + .config_intr = &vsc85xx_config_intr, + .suspend = &genphy_suspend, + .resume = &genphy_resume, } }; @@ -149,9 +149,9 @@ static struct phy_driver vsc85xx_driver[] = { module_phy_driver(vsc85xx_driver); static struct mdio_device_id __maybe_unused vsc85xx_tbl[] = { - { PHY_ID_VSC8531, 0xfffffff0, }, - { PHY_ID_VSC8541, 0xfffffff0, }, - { } + { PHY_ID_VSC8531, 0xfffffff0, }, + { PHY_ID_VSC8541, 0xfffffff0, }, + { } }; MODULE_DEVICE_TABLE(mdio, vsc85xx_tbl); -- 2.7.4 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH net-next 1/1] net: phy: Fixed checkpatch errors for Microsemi PHYs. 2016-09-08 8:39 ` [PATCH net-next 1/1] net: phy: Fixed checkpatch errors for Microsemi PHYs Raju Lakkaraju @ 2016-09-10 1:16 ` David Miller 0 siblings, 0 replies; 15+ messages in thread From: David Miller @ 2016-09-10 1:16 UTC (permalink / raw) To: Raju.Lakkaraju; +Cc: netdev, f.fainelli, Allan.Nielsen, andrew From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com> Date: Thu, 8 Sep 2016 14:09:31 +0530 > From: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com> > > The existing VSC85xx PHY driver did not follow the coding style and caused "checkpatch" to complain. This commit fixes this. > > Signed-off-by: Raju Lakkaraju <Raju.Lakkaraju@microsemi.com> Applied. ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2016-09-10 1:16 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-07-26 9:49 Microsemi VSC 8531/41 PHY Driver Raju Lakkaraju 2016-07-26 11:55 ` Andrew Lunn 2016-07-26 12:20 ` Raju Lakkaraju 2016-07-26 12:43 ` Andrew Lunn 2016-07-28 6:44 ` Raju Lakkaraju 2016-07-28 17:35 ` Florian Fainelli 2016-08-04 9:31 ` Nagaraju Lakkaraju 2016-07-29 8:04 ` Andrew Lunn 2016-07-29 8:17 ` Andrew Lunn 2016-08-04 9:17 ` Nagaraju Lakkaraju 2016-08-05 12:24 ` Nagaraju Lakkaraju 2016-08-16 19:41 ` Andrew Lunn 2016-08-16 19:48 ` Andrew Lunn 2016-09-08 8:39 ` [PATCH net-next 1/1] net: phy: Fixed checkpatch errors for Microsemi PHYs Raju Lakkaraju 2016-09-10 1:16 ` David Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).