[PATCH net-next v2] net: phy: xilinx: add Xilinx PHY driver

[PATCH net-next v2] net: phy: xilinx: add Xilinx PHY driver

[Robert Hancock]

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",
+	/* 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");
+
-- 
1.8.3.1

Re: [PATCH net-next v2] net: phy: xilinx: add Xilinx PHY driver

[Jesse Brandeburg]

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>

Re: [PATCH net-next v2] net: phy: xilinx: add Xilinx PHY driver

[Florian Fainelli]

+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.
-- 
Florian

Re: [PATCH net-next v2] net: phy: xilinx: add Xilinx PHY driver

[Heiner Kallweit]

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");
> +
> 

Re: [PATCH net-next v2] net: phy: xilinx: add Xilinx PHY driver

[Robert Hancock]

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?

-- 
Robert Hancock
Senior Software Developer
SED Systems, a division of Calian Ltd.
Email: hancock@sedsystems.ca

Re: [PATCH net-next v2] net: phy: xilinx: add Xilinx PHY driver

[Andrew Lunn]

> 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
  

Re: [PATCH net-next v2] net: phy: xilinx: add Xilinx PHY driver

[Robert Hancock]

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.

-- 
Robert Hancock
Senior Software Developer
SED Systems, a division of Calian Ltd.
Email: hancock@sedsystems.ca

Re: [PATCH net-next v2] net: phy: xilinx: add Xilinx PHY driver

[Heiner Kallweit]

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

Re: [PATCH net-next v2] net: phy: xilinx: add Xilinx PHY driver

[Andrew Lunn]

> 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