Re: Microsemi VSC 8531/41 PHY Driver

[Andrew Lunn]

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

Re: Microsemi VSC 8531/41 PHY Driver

[Andrew Lunn]

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 
> 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

Re: Microsemi VSC 8531/41 PHY Driver

[Nagaraju Lakkaraju]

Hello,

I added all review comments and re-sending for review.

>From a5017f5878a92d2acec86a6a29b1498c457cb73a Mon Sep 17 00:00:00 2001
From: Nagaraju Lakkaraju 
Date: Wed, 3 Aug 2016 18:28:24 +0530
Subject: [PATCH v2] net: phy: Add drivers for Microsemi PHYs

Signed-off-by: Nagaraju Lakkaraju 
---
 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,
+   .name   = "Microsemi VSC8531",
+   

Re: Microsemi VSC 8531/41 PHY Driver

[Nagaraju Lakkaraju]

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

[Nagaraju Lakkaraju]

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 ch

Re: Microsemi VSC 8531/41 PHY Driver

[Andrew Lunn]

> > +/* 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

Re: Microsemi VSC 8531/41 PHY Driver

[Andrew Lunn]

On Thu, Jul 28, 2016 at 06:44:37AM +, 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.
> 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

Re: Microsemi VSC 8531/41 PHY Driver

[Florian Fainelli]

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.

> 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(>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)
>> + 

RE: Microsemi VSC 8531/41 PHY Driver

[Raju Lakkaraju]

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.
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(>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(>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

Re: Microsemi VSC 8531/41 PHY Driver

[Andrew Lunn]

> +/* 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(>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(>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

RE: Microsemi VSC 8531/41 PHY Driver

[Raju Lakkaraju]

al);
+   rc = vsc85xx_phy_page_set(phydev, MSCC_PHY_PAGE_STANDARD);
+   if (rc != 0)
+   rc = -EINVAL;
+
+out_unlock:
+   mutex_unlock(>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= 0xfff0,
+   .features   = PHY_GBIT_FEATURES,
+   .flags  = PHY_HAS_INTERRUPT,
+   .soft_reset = _soft_reset,
+   .config_init= _config_init,
+   .config_aneg= _config_aneg,
+   .aneg_done  = _aneg_done,
+   .read_status= _read_status,
+   .ack_interrupt  = _ack_interrupt,
+   .config_intr= _config_intr,
+   .suspend= _suspend,
+   .resume = _resume,
+},
+{
+   .phy_id = PHY_ID_VSC8541,
+   .name   = "Microsemi VSC8541 SyncE",
+   .phy_id_mask= 0xfff0,
+   .features   = PHY_GBIT_FEATURES,
+   .flags  = PHY_HAS_INTERRUPT,
+   .soft_reset = _soft_reset,
+   .config_init= _config_init,
+   .config_aneg= _config_aneg,
+   .aneg_done  = _aneg_done,
+   .read_status= _read_status,
+   .ack_interrupt  = _ack_interrupt,
+   .config_intr= _config_intr,
+   .suspend= _suspend,
+   .resume = _resume,
+}
+
+};
+
+module_phy_driver(vsc85xx_driver);
+
+static struct mdio_device_id __maybe_unused vsc85xx_tbl[] = {
+   { PHY_ID_VSC8531, 0xfff0, },
+   { PHY_ID_VSC8541, 0xfff0, },
+   { }
+};
+
+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:and...@lunn.ch] 
Sent: Tuesday, July 26, 2016 5:26 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


On Tue, Jul 26, 2016 at 09:49:53AM +, 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 

Re: Microsemi VSC 8531/41 PHY Driver

[Andrew Lunn]

On Tue, Jul 26, 2016 at 09:49:53AM +, 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

Microsemi VSC 8531/41 PHY Driver

[Raju Lakkaraju]

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



0001-net-phy-Add-drivers-for-Microsemi-PHYs.patch
Description: 0001-net-phy-Add-drivers-for-Microsemi-PHYs.patch