| Message ID | 1559603524-18288-1-git-send-email-hancock@sedsystems.ca |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next,v2] net: phy: xilinx: add Xilinx PHY driver | expand |
On Mon, 3 Jun 2019 17:12:04 -0600 Robert Hancock <hancock@sedsystems.ca> wrote: > This adds a driver for the PHY device implemented in the Xilinx PCS/PMA > Core logic. This is mostly a generic gigabit PHY, except that the > features are explicitly set because the PHY wrongly indicates it has no > extended status register when it actually does. > > This version is a simplified version of the GPL 2+ version from the > Xilinx kernel tree. > > Signed-off-by: Robert Hancock <hancock@sedsystems.ca> > --- > > Differences from v1: > -Removed unnecessary config_init method > -Added comment to explain why features are explicitly set > > drivers/net/phy/Kconfig | 6 ++++++ > drivers/net/phy/Makefile | 1 + > drivers/net/phy/xilinx.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 58 insertions(+) > create mode 100644 drivers/net/phy/xilinx.c > Seems fine. Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
+Heiner, Andrew, On 6/3/2019 4:12 PM, Robert Hancock wrote: > This adds a driver for the PHY device implemented in the Xilinx PCS/PMA > Core logic. This is mostly a generic gigabit PHY, except that the > features are explicitly set because the PHY wrongly indicates it has no > extended status register when it actually does. > > This version is a simplified version of the GPL 2+ version from the > Xilinx kernel tree. For future submission, please use scripts/get_maintainer.pl so the appropriate maintainers can be copied and give a chance to review this. > > Signed-off-by: Robert Hancock <hancock@sedsystems.ca> > --- > [snip] > +/* Mask used for ID comparisons */ > +#define XILINX_PHY_ID_MASK 0xfffffff0 > + > +/* Known PHY IDs */ > +#define XILINX_PHY_ID 0x01740c00 > + > +static struct phy_driver xilinx_drivers[] = { > +{ > + .phy_id = XILINX_PHY_ID, > + .phy_id_mask = XILINX_PHY_ID_MASK, You can use PHY_ID_MATCH_MODEL to declare the first two fields here. > + .name = "Xilinx PCS/PMA PHY", > + /* Xilinx PHY wrongly indicates BMSR_ESTATEN = 0 even though > + * extended status registers are supported. So we force the PHY > + * features to PHY_GBIT_FEATURES in order to allow gigabit support > + * to be detected. > + */ A PHY fixup might have worked too, but I suppose this is equally fine.
On 04.06.2019 01:12, Robert Hancock wrote: > This adds a driver for the PHY device implemented in the Xilinx PCS/PMA > Core logic. This is mostly a generic gigabit PHY, except that the > features are explicitly set because the PHY wrongly indicates it has no > extended status register when it actually does. > > This version is a simplified version of the GPL 2+ version from the > Xilinx kernel tree. > > Signed-off-by: Robert Hancock <hancock@sedsystems.ca> > --- > > Differences from v1: > -Removed unnecessary config_init method > -Added comment to explain why features are explicitly set > > drivers/net/phy/Kconfig | 6 ++++++ > drivers/net/phy/Makefile | 1 + > drivers/net/phy/xilinx.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 58 insertions(+) > create mode 100644 drivers/net/phy/xilinx.c > > diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig > index db5645b..101c794 100644 > --- a/drivers/net/phy/Kconfig > +++ b/drivers/net/phy/Kconfig > @@ -462,6 +462,12 @@ config VITESSE_PHY > ---help--- > Currently supports the vsc8244 > > +config XILINX_PHY > + tristate "Drivers for Xilinx PHYs" > + help > + This module provides a driver for the PHY implemented in the > + Xilinx PCS/PMA Core. > + > config XILINX_GMII2RGMII > tristate "Xilinx GMII2RGMII converter driver" > ---help--- > diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile > index bac339e..3ee9cdb 100644 > --- a/drivers/net/phy/Makefile > +++ b/drivers/net/phy/Makefile > @@ -91,4 +91,5 @@ obj-$(CONFIG_SMSC_PHY) += smsc.o > obj-$(CONFIG_STE10XP) += ste10Xp.o > obj-$(CONFIG_TERANETICS_PHY) += teranetics.o > obj-$(CONFIG_VITESSE_PHY) += vitesse.o > +obj-$(CONFIG_XILINX_PHY) += xilinx.o > obj-$(CONFIG_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o > diff --git a/drivers/net/phy/xilinx.c b/drivers/net/phy/xilinx.c > new file mode 100644 > index 0000000..0e5509b > --- /dev/null > +++ b/drivers/net/phy/xilinx.c > @@ -0,0 +1,51 @@ > +// SPDX-License-Identifier: GPL-2.0+ > +/* Xilinx PCS/PMA Core phy driver > + * > + * Copyright (C) 2019 SED Systems, a division of Calian Ltd. > + * > + * Based upon Xilinx version of this driver: > + * Copyright (C) 2015 Xilinx, Inc. > + * > + * Description: > + * This driver is developed for PCS/PMA Core. > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/mii.h> > +#include <linux/phy.h> > + > +/* Mask used for ID comparisons */ > +#define XILINX_PHY_ID_MASK 0xfffffff0 > + > +/* Known PHY IDs */ > +#define XILINX_PHY_ID 0x01740c00 > + > +static struct phy_driver xilinx_drivers[] = { > +{ > + .phy_id = XILINX_PHY_ID, > + .phy_id_mask = XILINX_PHY_ID_MASK, > + .name = "Xilinx PCS/PMA PHY", Please no slash in the name. This breaks sysfs. > + /* Xilinx PHY wrongly indicates BMSR_ESTATEN = 0 even though > + * extended status registers are supported. So we force the PHY > + * features to PHY_GBIT_FEATURES in order to allow gigabit support > + * to be detected. > + */ > + .features = PHY_GBIT_FEATURES, BMSR_ESTATEN is used by genphy_config_advert() too. Means you would need to implement your own config_aneg callback and basically copy 99% of genphy_config_advert(). I think the better alternative is to implement a quirk flag in phylib similar to PHY_RST_AFTER_CLK_EN. Let me come up with a proposal. Last but not least: Not setting BMSR_ESTATEN for a GBit PHY violates the standard. Any intention from Xilinx to fix this? > + .resume = genphy_resume, > + .suspend = genphy_suspend, > + .set_loopback = genphy_loopback, > +}, > +}; > + > +module_phy_driver(xilinx_drivers); > + > +static struct mdio_device_id __maybe_unused xilinx_tbl[] = { > + { XILINX_PHY_ID, XILINX_PHY_ID_MASK }, > + { } > +}; > + > +MODULE_DEVICE_TABLE(mdio, xilinx_tbl); > +MODULE_DESCRIPTION("Xilinx PCS/PMA PHY driver"); > +MODULE_LICENSE("GPL"); > + >
On 2019-06-03 11:37 p.m., Heiner Kallweit wrote: >> + /* Xilinx PHY wrongly indicates BMSR_ESTATEN = 0 even though >> + * extended status registers are supported. So we force the PHY >> + * features to PHY_GBIT_FEATURES in order to allow gigabit support >> + * to be detected. >> + */ >> + .features = PHY_GBIT_FEATURES, > > BMSR_ESTATEN is used by genphy_config_advert() too. Means you would > need to implement your own config_aneg callback and basically > copy 99% of genphy_config_advert(). > > I think the better alternative is to implement a quirk flag in phylib > similar to PHY_RST_AFTER_CLK_EN. Let me come up with a proposal. > > Last but not least: Not setting BMSR_ESTATEN for a GBit PHY violates > the standard. Any intention from Xilinx to fix this? My apologies, it seems like my initial diagnosis of this was incorrect. ESTATEN is indeed set to 1. The problem is that in the configuration of the PHY that we are using, which is set up for 1000Base-X mode, ESTATUS returns 0x8000 indicating support for 1000Base-X full duplex and not ESTATUS_1000_TFULL or ESTATUS_1000_THALF, which are the only bits that genphy_config_init checks for. Therefore, genphy_config_init comes back with no modes supported for the PHY, and phydev therefore rejects attaching the PHY to the network device. Adding PHY_GBIT_FEATURES in causes 1000Base-T half and full duplex to be added into the mix, which fakes things out enough for it to work, it appears. So it seems like what is missing is the ability of genphy_config_init to detect the bits in the extended status register for 1000Base-X and add the corresponding mode flags. It appears bit 15 for 1000Base-X full duplex is standardized in 802.3 Clause 22, so I would expect Linux should be able to detect that and add it as a supported mode for the PHY. genphy_config_init is dealing with the "legacy" 32-bit mode masks that have no bit for 1000BaseX though.. how is that intended to work?
> So it seems like what is missing is the ability of genphy_config_init to > detect the bits in the extended status register for 1000Base-X and add > the corresponding mode flags. It appears bit 15 for 1000Base-X full > duplex is standardized in 802.3 Clause 22, so I would expect Linux > should be able to detect that and add it as a supported mode for the > PHY. genphy_config_init is dealing with the "legacy" 32-bit mode masks > that have no bit for 1000BaseX though.. how is that intended to work? Hi Robert I think you are looking at an old genphy_config_init(). The u32 has been replaced. Adding: #define ESTATUS_1000_XFULL 0x8000 /* Can do 1000BX Full */ #define ESTATUS_1000_XHALF 0x4000 /* Can do 1000BT Half */ and if (val & ESTATUS_1000_XFULL) linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT, features); should not be a problem. Andrew
On 2019-06-04 10:54 a.m., Andrew Lunn wrote: >> So it seems like what is missing is the ability of genphy_config_init to >> detect the bits in the extended status register for 1000Base-X and add >> the corresponding mode flags. It appears bit 15 for 1000Base-X full >> duplex is standardized in 802.3 Clause 22, so I would expect Linux >> should be able to detect that and add it as a supported mode for the >> PHY. genphy_config_init is dealing with the "legacy" 32-bit mode masks >> that have no bit for 1000BaseX though.. how is that intended to work? > > Hi Robert > > I think you are looking at an old genphy_config_init(). The u32 has > been replaced. Adding: > > #define ESTATUS_1000_XFULL 0x8000 /* Can do 1000BX Full */ > #define ESTATUS_1000_XHALF 0x4000 /* Can do 1000BT Half */ > > and > > if (val & ESTATUS_1000_XFULL) > linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT, > features); > > should not be a problem. Yup, mixing up branches again. I'll need to try adding that in and see if that works with net-next. So I think this patch can be shelved for now - if that change for 1000BaseX works, there's no real need for a specific PHY driver since the generic one should be good enough.
On 04.06.2019 18:54, Andrew Lunn wrote: >> So it seems like what is missing is the ability of genphy_config_init to >> detect the bits in the extended status register for 1000Base-X and add >> the corresponding mode flags. It appears bit 15 for 1000Base-X full >> duplex is standardized in 802.3 Clause 22, so I would expect Linux >> should be able to detect that and add it as a supported mode for the >> PHY. genphy_config_init is dealing with the "legacy" 32-bit mode masks >> that have no bit for 1000BaseX though.. how is that intended to work? > > Hi Robert > > I think you are looking at an old genphy_config_init(). The u32 has > been replaced. Adding: > > #define ESTATUS_1000_XFULL 0x8000 /* Can do 1000BX Full */ > #define ESTATUS_1000_XHALF 0x4000 /* Can do 1000BT Half */ > At least so far phylib knows 1000Base-X/Full only. Not sure whether optical half duplex modes are used in reality. Detecting 1000Base-X capability is one thing, how about 1000Base-X advertisement and link partner capability detection? If I remember the Marvell specs correctly, there was some bit to switch the complete register set to fibre mode. Robert, how is this done for the Xilinx PHY? > and > > if (val & ESTATUS_1000_XFULL) > linkmode_set_bit(ETHTOOL_LINK_MODE_1000baseX_Full_BIT, > features); > > should not be a problem. > > Andrew > >
> If I remember the Marvell specs correctly, there was some bit to switch the > complete register set to fibre mode. Hi Heiner The Marvell PHY has a second page for Fibre. It mostly mirrors the normal registers, and you need to look at both pages to determine if copper or fibre has link, etc. Fibre has no auto-neg, so there is nothing to configure for that. For the Xilinx PHY the auto-neg ability should not be set, so i doubt phylib will offer the option. Andrew
diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig index db5645b..101c794 100644 --- a/drivers/net/phy/Kconfig +++ b/drivers/net/phy/Kconfig @@ -462,6 +462,12 @@ config VITESSE_PHY ---help--- Currently supports the vsc8244 +config XILINX_PHY + tristate "Drivers for Xilinx PHYs" + help + This module provides a driver for the PHY implemented in the + Xilinx PCS/PMA Core. + config XILINX_GMII2RGMII tristate "Xilinx GMII2RGMII converter driver" ---help--- diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile index bac339e..3ee9cdb 100644 --- a/drivers/net/phy/Makefile +++ b/drivers/net/phy/Makefile @@ -91,4 +91,5 @@ obj-$(CONFIG_SMSC_PHY) += smsc.o obj-$(CONFIG_STE10XP) += ste10Xp.o obj-$(CONFIG_TERANETICS_PHY) += teranetics.o obj-$(CONFIG_VITESSE_PHY) += vitesse.o +obj-$(CONFIG_XILINX_PHY) += xilinx.o obj-$(CONFIG_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o diff --git a/drivers/net/phy/xilinx.c b/drivers/net/phy/xilinx.c new file mode 100644 index 0000000..0e5509b --- /dev/null +++ b/drivers/net/phy/xilinx.c @@ -0,0 +1,51 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* Xilinx PCS/PMA Core phy driver + * + * Copyright (C) 2019 SED Systems, a division of Calian Ltd. + * + * Based upon Xilinx version of this driver: + * Copyright (C) 2015 Xilinx, Inc. + * + * Description: + * This driver is developed for PCS/PMA Core. + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/mii.h> +#include <linux/phy.h> + +/* Mask used for ID comparisons */ +#define XILINX_PHY_ID_MASK 0xfffffff0 + +/* Known PHY IDs */ +#define XILINX_PHY_ID 0x01740c00 + +static struct phy_driver xilinx_drivers[] = { +{ + .phy_id = XILINX_PHY_ID, + .phy_id_mask = XILINX_PHY_ID_MASK, + .name = "Xilinx PCS/PMA PHY", + /* Xilinx PHY wrongly indicates BMSR_ESTATEN = 0 even though + * extended status registers are supported. So we force the PHY + * features to PHY_GBIT_FEATURES in order to allow gigabit support + * to be detected. + */ + .features = PHY_GBIT_FEATURES, + .resume = genphy_resume, + .suspend = genphy_suspend, + .set_loopback = genphy_loopback, +}, +}; + +module_phy_driver(xilinx_drivers); + +static struct mdio_device_id __maybe_unused xilinx_tbl[] = { + { XILINX_PHY_ID, XILINX_PHY_ID_MASK }, + { } +}; + +MODULE_DEVICE_TABLE(mdio, xilinx_tbl); +MODULE_DESCRIPTION("Xilinx PCS/PMA PHY driver"); +MODULE_LICENSE("GPL"); +
This adds a driver for the PHY device implemented in the Xilinx PCS/PMA Core logic. This is mostly a generic gigabit PHY, except that the features are explicitly set because the PHY wrongly indicates it has no extended status register when it actually does. This version is a simplified version of the GPL 2+ version from the Xilinx kernel tree. Signed-off-by: Robert Hancock <hancock@sedsystems.ca> --- Differences from v1: -Removed unnecessary config_init method -Added comment to explain why features are explicitly set drivers/net/phy/Kconfig | 6 ++++++ drivers/net/phy/Makefile | 1 + drivers/net/phy/xilinx.c | 51 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 58 insertions(+) create mode 100644 drivers/net/phy/xilinx.c