netdev - Re: [PATCH] net: phy: Add TJA1100 BroadR-Reach PHY driver.

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20180608155529.GA19702@lunn.ch>
Date:   Fri, 8 Jun 2018 17:55:29 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Kirill Kranke <kranke.kirill@...il.com>
Cc:     f.fainelli@...il.com, davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH] net: phy: Add TJA1100 BroadR-Reach PHY driver.

On Fri, Jun 08, 2018 at 05:45:32PM +0300, Kirill Kranke wrote:
> Current generic PHY driver does not work with TJA1100 BroadR-REACH PHY
> properly. TJA1100 does not have any standard ability enabled at MII_BMSR
> register. Instead it has BroadR-REACH ability at MII_ESTATUS enabled, which
> is not handled by generic driver yet. Therefore generic driver is unable to
> guess required link speed, duplex etc. Device is started up with 10Mbps
> halfduplex which is incorrect.
> 
> BroadR-REACH able flag is not specified in IEEE802.3-2015. Which is why I
> did not add BroadR-REACH able flag support at generic driver. Once
> BroadR-REACH able flag gets into IEEE802.3 it should be reasonable to
> support it in the generic PHY driver.

Hi Kirill

Thank for making the changes.

It is normal to put 'v2' after PATCH in the subject line. Also, make a
brief list of changes since the previous version, after a line with
---. They will get removed when the patch is committed, but help
reviewers see what has changed.

For network patches, you should also include which tree these patches
are for. net-next in this case. See the networking FAQ.

> 
> Signed-off-by: Kirill Kranke <kranke.kirill@...il.com>
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 343989f..7014eb7 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -422,6 +422,14 @@ config TERANETICS_PHY
>  	---help---
>  	  Currently supports the Teranetics TN2020
>  
> +config TJA1100_PHY
> +	tristate "NXP TJA1100 PHY"

Please call this NXP_TJA1100_PHY. Putting the vendor first is the
general pattern. Are are a few TI drivers which ignore this, but other
follow this. This also means moving it up so it comes after
NATIONAL_PHY.

> +	help
> +	  Support of NXP TJA1100 BroadR-REACH ethernet PHY.
> +	  Generic driver is not suitable for TJA1100 PHY while the PHY does not
> +	  advertise any standard IEEE capabilities. It uses BroadR-REACH able
> +	  flag instead. This driver configures capabilities of the PHY properly.
>

Does 100Base-T1/cause 96 define a way to identify a PHY which
implements this? I'm just wondering if we can do this in the generic
code, for devices which correctly implement the standard?

 +
>  config VITESSE_PHY
>  	tristate "Vitesse PHYs"
>  	---help---
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index 5805c0b..4d2a69d 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -83,5 +83,6 @@ obj-$(CONFIG_ROCKCHIP_PHY)	+= rockchip.o
>  obj-$(CONFIG_SMSC_PHY)		+= smsc.o
>  obj-$(CONFIG_STE10XP)		+= ste10Xp.o
>  obj-$(CONFIG_TERANETICS_PHY)	+= teranetics.o
> +obj-$(CONFIG_TJA1100_PHY)	+= tja1100.o
>  obj-$(CONFIG_VITESSE_PHY)	+= vitesse.o
>  obj-$(CONFIG_XILINX_GMII2RGMII) += xilinx_gmii2rgmii.o
> diff --git a/drivers/net/phy/tja1100.c b/drivers/net/phy/tja1100.c
> new file mode 100644
> index 0000000..cddf4d7
> --- /dev/null
> +++ b/drivers/net/phy/tja1100.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* tja1100.c: TJA1100 BoardR-REACH PHY driver.
> + *
> + * Copyright (c) 2017 Kirill Kranke <kirill.kranke@...il.com>
> + * Author: Kirill Kranke <kirill.kranke@...il.com>
> + */
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/phy.h>
> +
> +static int tja1100_phy_config_init(struct phy_device *phydev)
> +{
> +	phydev->autoneg = AUTONEG_DISABLE;
> +	phydev->speed = SPEED_100;
> +	phydev->duplex = DUPLEX_FULL;
> +
> +	return 0;
> +}
> +
> +static int tja1100_phy_config_aneg(struct phy_device *phydev)
> +{
> +	if (phydev->autoneg == AUTONEG_ENABLE) {
> +		phydev_err(phydev, "autonegotiation is not supported\n");
> +		return -EINVAL;
> +	}
> +
> +	if (phydev->speed != SPEED_100 || phydev->duplex != DUPLEX_FULL) {
> +		phydev_err(phydev, "only 100MBps Full Duplex allowed\n");
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static struct phy_driver tja1100_phy_driver[] = {
> +	{
> +		.phy_id = 0x0180dc48,
> +		.phy_id_mask = 0xfffffff0,
> +		.name = "NXP TJA1100",
> +
> +		/* TJA1100 has only 100BASE-BroadR-REACH ability specified
> +		 * at MII_ESTATUS register. Standard modes are not
> +		 * supported. Therefore BroadR-REACH allow only 100Mbps
> +		 * full duplex without autoneg.
> +		 */
> +		.features = SUPPORTED_100baseT_Full | SUPPORTED_MII,

This is the second T1 driver we have had recently. It might make sense to add a
PHY_T1_FEATURES macro the include/linux/phy.h

Don't you also want SUPPORTED_TP?

	Andrew