RE: [PATCH v2] drivers: phy: Add Cortina CS4340 driver

[Bogdan Purcareata]

> -----Original Message-----
> From: Andrew Lunn [mailto:andrew@xxxxxxx]
> Sent: Wednesday, May 24, 2017 3:59 PM
> To: Bogdan Purcareata <bogdan.purcareata@xxxxxxx>
> Cc: f.fainelli@xxxxxxxxx; netdev@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2] drivers: phy: Add Cortina CS4340 driver
> 
> On Wed, May 24, 2017 at 09:25:48AM +0000, Bogdan Purcareata wrote:
> > Add basic support for Cortina PHY drivers. Support only CS4340 for now.
> > The phys are not compatible with IEEE 802.3 clause 45 registers.
> Implement
> > proper read_status support, so that phy polling does not cause bus
> > register access errors.
> >
> > The driver should be described using the "ethernet-phy-id" device tree
> > compatible.
> 
> Hi Bogdan
> 
> Thanks for testing that ethernet-phy-id works.
> 
> I suggest you write a
> Documentation/devicetree/binding/net/phy/cortina.txt giving an example
> device tree node.

Yes, I can do that. However, I don't see a "phy" folder under Documentation/devicetree/bindings/net/.
Should I change the location to Documentation/devicetree/bindings/net/cortina.txt instead?

> >
> > Signed-off-by: Bogdan Purcareata <bogdan.purcareata@xxxxxxx>
> > ---
> > v1 -> v2:
> > - Rename "mdio-cortina.c" to "cortina.c" since it's a phy driver.
> > - Test probing based on the "ethernet-phy-id" compatible. In the previous
> >   version, getting the phy_id via get_phy_c45_ids() involved an
> additional
> >   hack. Drop that approach and document probing in the commit message.
> >
> >  drivers/net/phy/Kconfig   |  5 +++
> >  drivers/net/phy/Makefile  |  1 +
> >  drivers/net/phy/cortina.c | 90
> +++++++++++++++++++++++++++++++++++++++++++++++
> >  3 files changed, 96 insertions(+)
> >  create mode 100644 drivers/net/phy/cortina.c
> >
> > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> > index 22dea7f..ad09e2d 100644
> > --- a/drivers/net/phy/Kconfig
> > +++ b/drivers/net/phy/Kconfig
> > @@ -240,6 +240,11 @@ config CICADA_PHY
> >  	---help---
> >  	  Currently supports the cis8204
> >
> > +config CORTINA_PHY
> > +	tristate "Cortina quad-10G Ethernet PHY"
> > +	---help---
> > +	  Currently supports the CS4340 phy.
> > +
> >  config DAVICOM_PHY
> >  	tristate "Davicom PHYs"
> >  	---help---
> > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> > index 79365be..0de3e20 100644
> > --- a/drivers/net/phy/Makefile
> > +++ b/drivers/net/phy/Makefile
> > @@ -47,6 +47,7 @@ obj-$(CONFIG_BCM_CYGNUS_PHY)	+= bcm-cygnus.o
> >  obj-$(CONFIG_BCM_NET_PHYLIB)	+= bcm-phy-lib.o
> >  obj-$(CONFIG_BROADCOM_PHY)	+= broadcom.o
> >  obj-$(CONFIG_CICADA_PHY)	+= cicada.o
> > +obj-$(CONFIG_CORTINA_PHY)	+= cortina.o
> >  obj-$(CONFIG_DAVICOM_PHY)	+= davicom.o
> >  obj-$(CONFIG_DP83640_PHY)	+= dp83640.o
> >  obj-$(CONFIG_DP83848_PHY)	+= dp83848.o
> > diff --git a/drivers/net/phy/cortina.c b/drivers/net/phy/cortina.c
> > new file mode 100644
> > index 0000000..6f054ed
> > --- /dev/null
> > +++ b/drivers/net/phy/cortina.c
> > @@ -0,0 +1,90 @@
> > +/*
> > + *    Based on code from Cortina Systems, Inc.
> > + *
> > + *    Copyright 2011 Cortina Systems, Inc.
> > + *    Copyright 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;
> 
> I think there needs to be some explanation here. What exactly are you
> using to indicate link up? What does CORTINA_GPIO_GPIO_INTS mean?

I can only assume it's the location of an interrupt status register. The Cortina PHYs go mostly undocumented, I've found an implementation in an old thread [1], that also did the job of programming the Cortina PHY microcode. I understand that microcode programming is now part of the bootloader.

The register seems to provide the link status properly, and based on that the phydev is initialized with the only (currently) supported configuration, 10Gbps full duplex.

I can change the register name to something more meaningful, though - e.g. CORTINA_LINK_STATUS.

> > +	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,
> > +},
> > +};
> 
> Having two } at the same indentation level seems odd. Please can you
> fix this.

I've purposely left them on the same indentation level in case more Cortina PHY IDs will be added (similar to the situation in drivers/net/phy/aquantia.c). If it does look odd, I can refactor it to use the proper indentation.

[1] https://www.linux-mips.org/archives/linux-mips/2012-05/msg00314.html

Thank you!
Bogdan