| Message ID | 1495617948-13435-1-git-send-email-bogdan.purcareata@nxp.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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. > > Signed-off-by: Bogdan Purcareata <bogdan.purcareata@nxp.com> > --- > 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? > + 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. Thanks Andrew > + > +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 >
> -----Original Message----- > From: Andrew Lunn [mailto:andrew@lunn.ch] > Sent: Wednesday, May 24, 2017 3:59 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 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@nxp.com> > > --- > > 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
> 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? Ah, sorry, my error. Yes, Documentation/devicetree/bindings/net/cortina.txt. > > 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. No, leave the name as is. The MIPS driver you gave a reference to seems to be using sensible names. My guess is, the boot loader is configuring the PHY to generate an interrupt on link up via one of its GPIO lines. And this code is reading that GPIO status. This is all very fragile, you are making a lot of assumptions. Have you tested pulling the cable out? And plugging it back in again? There is a chance this interrupt is "link change", not "link up". What i do like in that mips code is the probe function verifies the product ID. + /* + * CS4312 keeps its ID values in non-standard registers, make + * sure we are talking to what we think we are. + */ + id_lsb = cs4321_phy_read(phydev, CS4321_GLOBAL_CHIP_ID_LSB); + if (id_lsb < 0) { + ret = id_lsb; + goto err; + } + + id_msb = cs4321_phy_read(phydev, CS4321_GLOBAL_CHIP_ID_MSB); + if (id_msb < 0) { + ret = id_msb; + goto err; + } + + if (id_lsb != 0x23E5 || id_msb != 0x1002) { + ret = -ENODEV; + goto err; + } You should do that as well. Andrew
> -----Original Message----- > From: Andrew Lunn [mailto:andrew@lunn.ch] > Sent: Wednesday, May 24, 2017 5:26 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 v2] drivers: phy: Add Cortina CS4340 driver > > > 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? > > Ah, sorry, my error. Yes, > Documentation/devicetree/bindings/net/cortina.txt. > > > > 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. > > No, leave the name as is. The MIPS driver you gave a reference to > seems to be using sensible names. My guess is, the boot loader is > configuring the PHY to generate an interrupt on link up via one of its > GPIO lines. And this code is reading that GPIO status. This is all > very fragile, you are making a lot of assumptions. > > Have you tested pulling the cable out? And plugging it back in again? > There is a chance this interrupt is "link change", not "link up". No, I have not. I had it planned but it somehow slipped. Will test. > What i do like in that mips code is the probe function verifies the > product ID. > > + /* > + * CS4312 keeps its ID values in non-standard registers, make > + * sure we are talking to what we think we are. > + */ > + id_lsb = cs4321_phy_read(phydev, CS4321_GLOBAL_CHIP_ID_LSB); > + if (id_lsb < 0) { > + ret = id_lsb; > + goto err; > + } > + > + id_msb = cs4321_phy_read(phydev, CS4321_GLOBAL_CHIP_ID_MSB); > + if (id_msb < 0) { > + ret = id_msb; > + goto err; > + } > + > + if (id_lsb != 0x23E5 || id_msb != 0x1002) { > + ret = -ENODEV; > + goto err; > + } > > You should do that as well. Okay, I will include this check in a probe function in v3. Thanks a lot! Bogdan
On 05/24/2017 07:34 AM, Bogdan Purcareata wrote: >> You should do that as well. > > Okay, I will include this check in a probe function in v3. When you do so, can you change the subject to be net: phy: Add Cortina CS4340 driver for the driver, and use dt-bindings: net: for the Device Tree binding patch? Thanks!
> -----Original Message----- > From: Florian Fainelli [mailto:f.fainelli@gmail.com] > Sent: Wednesday, May 24, 2017 8:03 PM > To: Bogdan Purcareata <bogdan.purcareata@nxp.com>; Andrew Lunn > <andrew@lunn.ch> > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v2] drivers: phy: Add Cortina CS4340 driver > > On 05/24/2017 07:34 AM, Bogdan Purcareata wrote: > >> You should do that as well. > > > > Okay, I will include this check in a probe function in v3. > > When you do so, can you change the subject to be net: phy: Add Cortina > CS4340 driver for the driver, and use dt-bindings: net: for the Device > Tree binding patch? Will do, thanks! Bogdan
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; + + 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);
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. Signed-off-by: Bogdan Purcareata <bogdan.purcareata@nxp.com> --- 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