[PATCH 0/2] net: phy: Support managed Cortina phys

[PATCH 0/2] net: phy: Support managed Cortina phys

[Bogdan Purcareata]

So far, the Cortina family phys (CS4340 in this particular case) are only
supported in fixed link mode (via fixed_phy_register). The generic 10G
phy driver does not work well with the phylib state machine, when the phy
is registered via of_phy_connect. This prohibits the user from describing the
phy nodes in the device tree.

In order to support this scenario, and to properly describe the board
device tree, add a minimal Cortina driver that reads the status from the
right register. With the generic 10G C45 driver, the kernel will print
messages like:
[    0.226521] mdio_bus 8b96000: Error while reading PHY16 reg at 1.6
[    0.232780] mdio_bus 8b96000: Error while reading PHY16 reg at 1.5

Bogdan Purcareata (2):
  net: phy: Update get_phy_c45_ids for Cortina PHYs
  drivers: phy: Add Cortina CS4340 driver

 drivers/net/phy/Kconfig        |  5 +++
 drivers/net/phy/Makefile       |  1 +
 drivers/net/phy/mdio-cortina.c | 90 ++++++++++++++++++++++++++++++++++++++++++
 drivers/net/phy/phy_device.c   | 10 +++--
 4 files changed, 102 insertions(+), 4 deletions(-)
 create mode 100644 drivers/net/phy/mdio-cortina.c

-- 
1.9.1

[PATCH 1/2] net: phy: Update get_phy_c45_ids for Cortina PHYs

[Bogdan Purcareata]

Complete the work started by the following patches
5f6c99e0 net: phy: fix a bug in get_phy_c45_ids
da1da284 net/phy: tune get_phy_c45_ids to support more c45 phy

in order to properly get non-standard C45 10G PHY IDs (e.g. Cortina
CS4340).

Signed-off-by: Bogdan Purcareata <bogdan.purcareata@nxp.com>
---
 drivers/net/phy/phy_device.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 1219eea..fef5627 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -335,7 +335,7 @@ static int phy_bus_match(struct device *dev, struct device_driver *drv)
 		return phydrv->match_phy_device(phydev);
 
 	if (phydev->is_c45) {
-		for (i = 1; i < num_ids; i++) {
+		for (i = 0; i < num_ids; i++) {
 			if (!(phydev->c45_ids.devices_in_package & (1 << i)))
 				continue;
 
@@ -488,28 +488,30 @@ static int get_phy_c45_ids(struct mii_bus *bus, int addr, u32 *phy_id,
 				*phy_id = 0xffffffff;
 				return 0;
 			} else {
+				c45_ids->devices_in_package |= 1;
 				break;
 			}
 		}
 	}
 
 	/* Now probe Device Identifiers for each device present. */
-	for (i = 1; i < num_ids; i++) {
+	for (i = 0; i < num_ids; i++) {
 		if (!(c45_ids->devices_in_package & (1 << i)))
 			continue;
 
-		reg_addr = MII_ADDR_C45 | i << 16 | MII_PHYSID1;
+		reg_addr = MII_ADDR_C45 | i << 16 | (i ? MII_PHYSID1 : 0);
 		phy_reg = mdiobus_read(bus, addr, reg_addr);
 		if (phy_reg < 0)
 			return -EIO;
 		c45_ids->device_ids[i] = (phy_reg & 0xffff) << 16;
 
-		reg_addr = MII_ADDR_C45 | i << 16 | MII_PHYSID2;
+		reg_addr = MII_ADDR_C45 | i << 16 | (i ? MII_PHYSID2 : 1);
 		phy_reg = mdiobus_read(bus, addr, reg_addr);
 		if (phy_reg < 0)
 			return -EIO;
 		c45_ids->device_ids[i] |= (phy_reg & 0xffff);
 	}
+
 	*phy_id = 0;
 	return 0;
 }
-- 
1.9.1

Re: [PATCH 1/2] net: phy: Update get_phy_c45_ids for Cortina PHYs

[Andrew Lunn]

On Tue, May 23, 2017 at 03:53:18PM +0000, Bogdan Purcareata wrote:
> Complete the work started by the following patches
> 5f6c99e0 net: phy: fix a bug in get_phy_c45_ids
> da1da284 net/phy: tune get_phy_c45_ids to support more c45 phy
> 
> in order to properly get non-standard C45 10G PHY IDs (e.g. Cortina
> CS4340).

At a first look, this seems to be a hack to work around a broken phy.

Instead of this, can you use the ethernet-phy-id property in device
tree?

	Andrew

RE: [PATCH 1/2] net: phy: Update get_phy_c45_ids for Cortina PHYs

[Bogdan Purcareata]

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Tuesday, May 23, 2017 7:04 PM
> To: Bogdan Purcareata <bogdan.purcareata@nxp.com>
> Cc: f.fainelli@gmail.com; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 1/2] net: phy: Update get_phy_c45_ids for Cortina PHYs
> 
> On Tue, May 23, 2017 at 03:53:18PM +0000, Bogdan Purcareata wrote:
> > Complete the work started by the following patches
> > 5f6c99e0 net: phy: fix a bug in get_phy_c45_ids
> > da1da284 net/phy: tune get_phy_c45_ids to support more c45 phy
> >
> > in order to properly get non-standard C45 10G PHY IDs (e.g. Cortina
> > CS4340).
> 
> At a first look, this seems to be a hack to work around a broken phy.
> 
> Instead of this, can you use the ethernet-phy-id property in device
> tree?

The patches mentioned in the commit message add _some_ support for the Cortina PHYs - mainly checking for devices at additional locations. Once they are found, the phy IDs must be read from custom locations. I followed the u-boot model [1].

I was aiming to patch the get_phy_c45_ids to fully handle the Cortina PHY scenario (right now, the support is only partial). 

[1] http://code.metager.de/source/xref/denx/u-boot/drivers/net/phy/cortina.c#288

Thank you!
Bogdan

Re: [PATCH 1/2] net: phy: Update get_phy_c45_ids for Cortina PHYs

[Andrew Lunn]

> The patches mentioned in the commit message add _some_ support for
> the Cortina PHYs - mainly checking for devices at additional
> locations. Once they are found, the phy IDs must be read from custom
> locations.
 
As a general principle, we don't add hacks in generic code to handle
broken devices. We add generic mechanisms to work around the
brokenness.

In this case, by using ethernet-phy-id in the device tree, we are
saying, this PHYs probing is totally borked, but we know it is there,
at this address. Just load the driver.

Please try to make ethernet-phy-id work.

       Andrew

Re: [PATCH 1/2] net: phy: Update get_phy_c45_ids for Cortina PHYs

[Florian Fainelli]

On 05/23/2017 09:55 AM, Andrew Lunn wrote:
>> The patches mentioned in the commit message add _some_ support for
>> the Cortina PHYs - mainly checking for devices at additional
>> locations. Once they are found, the phy IDs must be read from custom
>> locations.
>  
> As a general principle, we don't add hacks in generic code to handle
> broken devices. We add generic mechanisms to work around the
> brokenness.
> 
> In this case, by using ethernet-phy-id in the device tree, we are
> saying, this PHYs probing is totally borked, but we know it is there,
> at this address. Just load the driver.
> 
> Please try to make ethernet-phy-id work.

What Andrew is suggesting is to leverage the code in
drivers/of/of_mdio.c which does the following:

       is_c45 = of_device_is_compatible(child,
                                         "ethernet-phy-ieee802.3-c45");

        if (!is_c45 && !of_get_phy_id(child, &phy_id))
                phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
        else
                phy = get_phy_device(mdio, addr, is_c45);
        if (IS_ERR(phy))
                return;

If you know the PHY ID, and you did put it in the PHY node's compatible
string (in the format that of_get_phy_id() expects it to, and you also
did not add "ethernet-phy-ieee802.3-c45") then the PHY library will
directly create the PHY device, with the designated ID, at the specific
address.

While this works for clause 22 PHYs, I don't know if it also does for
clause 45 PHYs, but as Andrew is suggesting, I would be more inclined
into making this scheme work for all types (22 or 45) PHYs, rather than
hacking the core code that tries to identify devices in packages.

Can you give it a spin?
-- 
Florian

RE: [PATCH 1/2] net: phy: Update get_phy_c45_ids for Cortina PHYs

[Bogdan Purcareata]

> -----Original Message-----
> From: Florian Fainelli [mailto:f.fainelli@gmail.com]
> Sent: Tuesday, May 23, 2017 9:12 PM
> To: Andrew Lunn <andrew@lunn.ch>; Bogdan Purcareata
> <bogdan.purcareata@nxp.com>
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH 1/2] net: phy: Update get_phy_c45_ids for Cortina PHYs
> 
> On 05/23/2017 09:55 AM, Andrew Lunn wrote:
> >> The patches mentioned in the commit message add _some_ support for
> >> the Cortina PHYs - mainly checking for devices at additional
> >> locations. Once they are found, the phy IDs must be read from custom
> >> locations.
> >
> > As a general principle, we don't add hacks in generic code to handle
> > broken devices. We add generic mechanisms to work around the
> > brokenness.
> >
> > In this case, by using ethernet-phy-id in the device tree, we are
> > saying, this PHYs probing is totally borked, but we know it is there,
> > at this address. Just load the driver.
> >
> > Please try to make ethernet-phy-id work.
> 
> What Andrew is suggesting is to leverage the code in
> drivers/of/of_mdio.c which does the following:
> 
>        is_c45 = of_device_is_compatible(child,
>                                          "ethernet-phy-ieee802.3-c45");
> 
>         if (!is_c45 && !of_get_phy_id(child, &phy_id))
>                 phy = phy_device_create(mdio, addr, phy_id, 0, NULL);
>         else
>                 phy = get_phy_device(mdio, addr, is_c45);
>         if (IS_ERR(phy))
>                 return;
> 
> If you know the PHY ID, and you did put it in the PHY node's compatible
> string (in the format that of_get_phy_id() expects it to, and you also
> did not add "ethernet-phy-ieee802.3-c45") then the PHY library will
> directly create the PHY device, with the designated ID, at the specific
> address.
> 
> While this works for clause 22 PHYs, I don't know if it also does for
> clause 45 PHYs, but as Andrew is suggesting, I would be more inclined
> into making this scheme work for all types (22 or 45) PHYs, rather than
> hacking the core code that tries to identify devices in packages.
> 
> Can you give it a spin?

Sure. My first thought was to latch onto the codepath that had some mention of Cortina PHYs.

I will check of_get_phy_id() and see if it does the job.

Thank you!
Bogdan

[PATCH 2/2] drivers: phy: Add Cortina CS4340 driver

[Bogdan Purcareata]

Add basic support for Cortina PHY drivers. Support only CS4340 for now.
The phys are not fully compatible with IEEE 802.3 clause 45 registers.
Implement proper read_status support, so that phy polling does not cause
bus register access errors.

Signed-off-by: Bogdan Purcareata <bogdan.purcareata@nxp.com>
---
 drivers/net/phy/Kconfig        |  5 +++
 drivers/net/phy/Makefile       |  1 +
 drivers/net/phy/mdio-cortina.c | 90 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 96 insertions(+)
 create mode 100644 drivers/net/phy/mdio-cortina.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 60ffc9d..1d2aeb4 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -82,6 +82,11 @@ config MDIO_BUS_MUX_MMIOREG
 config MDIO_CAVIUM
 	tristate
 
+config MDIO_CORTINA
+	tristate "Driver for Cortina quad-10G Ethernet PHY"
+	---help---
+	  Currently only supports the Cortina CS4340 PHY.
+
 config MDIO_GPIO
 	tristate "GPIO lib-based bitbanged MDIO buses"
 	depends on MDIO_BITBANG && GPIOLIB
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index e36db9a..c82754f 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -28,6 +28,7 @@ obj-$(CONFIG_MDIO_BUS_MUX_BCM_IPROC)	+= mdio-mux-bcm-iproc.o
 obj-$(CONFIG_MDIO_BUS_MUX_GPIO)	+= mdio-mux-gpio.o
 obj-$(CONFIG_MDIO_BUS_MUX_MMIOREG) += mdio-mux-mmioreg.o
 obj-$(CONFIG_MDIO_CAVIUM)	+= mdio-cavium.o
+obj-$(CONFIG_MDIO_CORTINA)	+= mdio-cortina.o
 obj-$(CONFIG_MDIO_GPIO)		+= mdio-gpio.o
 obj-$(CONFIG_MDIO_HISI_FEMAC)	+= mdio-hisi-femac.o
 obj-$(CONFIG_MDIO_MOXART)	+= mdio-moxart.o
diff --git a/drivers/net/phy/mdio-cortina.c b/drivers/net/phy/mdio-cortina.c
new file mode 100644
index 0000000..2a5c234
--- /dev/null
+++ b/drivers/net/phy/mdio-cortina.c
@@ -0,0 +1,90 @@
+/*
+ *    Based on code from Cortina Systems, Inc.
+ *
+ *    Copyright (C) 2011 Cortina Systems, Inc.
+ *    Copyright (C) 2017 NXP
+ *
+ *    This program is free software; you can redistribute it and/or modify
+ *    it under the terms of the GNU General Public License as published by
+ *    the Free Software Foundation; either version 2 of the License, or
+ *    (at your option) any later version.
+ *
+ *    This program is distributed in the hope that it will be useful,
+ *    but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *    MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *    GNU General Public License for more details.
+ *
+ */
+#include <linux/module.h>
+#include <linux/phy.h>
+
+#define PHY_ID_CS4340	0x13e51002
+
+#define CORTINA_GPIO_GPIO_INTS				0x16D
+
+static int cortina_read_x(struct phy_device *phydev, int off, u16 regnum)
+{
+	return mdiobus_read(phydev->mdio.bus, phydev->mdio.addr + off,
+			    MII_ADDR_C45 | regnum);
+}
+
+static int cortina_read(struct phy_device *phydev, u16 regnum)
+{
+	return cortina_read_x(phydev, 0, regnum);
+}
+
+static int cortina_config_aneg(struct phy_device *phydev)
+{
+	phydev->supported = SUPPORTED_10000baseT_Full;
+	phydev->advertising = SUPPORTED_10000baseT_Full;
+
+	return 0;
+}
+
+static int cortina_read_status(struct phy_device *phydev)
+{
+	int gpio_int_status;
+	int ret = 0;
+
+	gpio_int_status = cortina_read(phydev, CORTINA_GPIO_GPIO_INTS);
+	if (gpio_int_status < 0) {
+		ret = gpio_int_status;
+		goto err;
+	}
+
+	if (gpio_int_status & 0x8) {
+		phydev->speed = SPEED_10000;
+		phydev->duplex = DUPLEX_FULL;
+		phydev->link = 1;
+	} else {
+		phydev->link = 0;
+	}
+
+err:
+	return ret;
+}
+
+static int cortina_soft_reset(struct phy_device *phydev)
+{
+	return 0;
+}
+
+static struct phy_driver cortina_driver[] = {
+{
+	.phy_id         = PHY_ID_CS4340,
+	.phy_id_mask    = 0xffffffff,
+	.name           = "Cortina CS4340",
+	.config_aneg    = cortina_config_aneg,
+	.read_status    = cortina_read_status,
+	.soft_reset     = cortina_soft_reset,
+},
+};
+
+module_phy_driver(cortina_driver);
+
+static struct mdio_device_id __maybe_unused cortina_tbl[] = {
+	{ PHY_ID_CS4340, 0xffffffff},
+	{},
+};
+
+MODULE_DEVICE_TABLE(mdio, cortina_tbl);
-- 
1.9.1

Re: [PATCH 2/2] drivers: phy: Add Cortina CS4340 driver

[Andrew Lunn]

On Tue, May 23, 2017 at 03:53:19PM +0000, Bogdan Purcareata wrote:
> Add basic support for Cortina PHY drivers. Support only CS4340 for now.
> The phys are not fully compatible with IEEE 802.3 clause 45 registers.
> Implement proper read_status support, so that phy polling does not cause
> bus register access errors.
> 
> Signed-off-by: Bogdan Purcareata <bogdan.purcareata@nxp.com>
> ---
>  drivers/net/phy/Kconfig        |  5 +++
>  drivers/net/phy/Makefile       |  1 +
>  drivers/net/phy/mdio-cortina.c | 90 ++++++++++++++++++++++++++++++++++++++++++

This is a phy driver, not a mdio bus driver. Please use the correct
file name.

     Andrew

RE: [PATCH 2/2] drivers: phy: Add Cortina CS4340 driver

[Bogdan Purcareata]

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@lunn.ch]
> Sent: Tuesday, May 23, 2017 6:57 PM
> To: Bogdan Purcareata <bogdan.purcareata@nxp.com>
> Cc: f.fainelli@gmail.com; netdev@vger.kernel.org; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH 2/2] drivers: phy: Add Cortina CS4340 driver
> 
> On Tue, May 23, 2017 at 03:53:19PM +0000, Bogdan Purcareata wrote:
> > Add basic support for Cortina PHY drivers. Support only CS4340 for now.
> > The phys are not fully compatible with IEEE 802.3 clause 45 registers.
> > Implement proper read_status support, so that phy polling does not cause
> > bus register access errors.
> >
> > Signed-off-by: Bogdan Purcareata <bogdan.purcareata@nxp.com>
> > ---
> >  drivers/net/phy/Kconfig        |  5 +++
> >  drivers/net/phy/Makefile       |  1 +
> >  drivers/net/phy/mdio-cortina.c | 90
> ++++++++++++++++++++++++++++++++++++++++++
> 
> This is a phy driver, not a mdio bus driver. Please use the correct
> file name.

Will fix in v2, thanks!
Bogdan