[PATCH] net: phy: tja11xx: Add TJA11xx PHY driver

[PATCH] net: phy: tja11xx: Add TJA11xx PHY driver

[Marek Vasut]

Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
BroadRReach 100BaseT1 PHYs used in automotive.

Signed-off-by: Marek Vasut <marex@denx.de>
---
 drivers/net/phy/Kconfig       |   5 +
 drivers/net/phy/Makefile      |   1 +
 drivers/net/phy/nxp-tja11xx.c | 348 ++++++++++++++++++++++++++++++++++
 3 files changed, 354 insertions(+)
 create mode 100644 drivers/net/phy/nxp-tja11xx.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index cd931cf9dcc26..030960158a2ba 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -356,6 +356,11 @@ config NATIONAL_PHY
 	---help---
 	  Currently supports the DP83865 PHY.
 
+config NXP_TJA11XX_PHY
+	tristate "NXP TJA11xx PHYs support"
+	---help---
+	  Currently supports the NXP TJA1100 and TJA1101 PHY.
+
 config QSEMI_PHY
 	tristate "Quality Semiconductor PHYs"
 	---help---
diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
index 00f097e18c68a..242cb5ffe676c 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -71,6 +71,7 @@ obj-$(CONFIG_MICREL_PHY)	+= micrel.o
 obj-$(CONFIG_MICROCHIP_PHY)	+= microchip.o
 obj-$(CONFIG_MICROSEMI_PHY)	+= mscc.o
 obj-$(CONFIG_NATIONAL_PHY)	+= national.o
+obj-$(CONFIG_NXP_TJA11XX_PHY)	+= nxp-tja11xx.o
 obj-$(CONFIG_QSEMI_PHY)		+= qsemi.o
 obj-$(CONFIG_REALTEK_PHY)	+= realtek.o
 obj-$(CONFIG_ROCKCHIP_PHY)	+= rockchip.o
diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
new file mode 100644
index 0000000000000..22f9c091befac
--- /dev/null
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -0,0 +1,348 @@
+// SPDX-License-Identifier: GPL-2.0
+/* NXP TJA1100 BroadRReach PHY driver
+ *
+ * Copyright (C) 2018 Marek Vasut <marex@denx.de>
+ */
+#include <linux/delay.h>
+#include <linux/ethtool.h>
+#include <linux/kernel.h>
+#include <linux/mii.h>
+#include <linux/module.h>
+#include <linux/phy.h>
+
+#define PHY_ID_MASK			0xfffffff0
+#define PHY_ID_TJA1100			0x0180dc40
+#define PHY_ID_TJA1101			0x0180dd00
+
+#define MII_ECTRL			17
+#define MII_ECTRL_LINK_CONTROL		BIT(15)
+#define MII_ECTRL_POWER_MODE_MASK	GENMASK(14, 11)
+#define MII_ECTRL_POWER_MODE_NO_CHANGE	(0x0 << 11)
+#define MII_ECTRL_POWER_MODE_NORMAL	(0x3 << 11)
+#define MII_ECTRL_POWER_MODE_STANDBY	(0xc << 11)
+#define MII_ECTRL_CONFIG_EN		BIT(2)
+#define MII_ECTRL_WAKE_REQUEST		BIT(0)
+
+#define MII_CFG1			18
+#define MII_CFG1_AUTO_OP		BIT(14)
+#define MII_CFG1_SLEEP_CONFIRM		BIT(6)
+#define MII_CFG1_LED_MODE_MASK		GENMASK(5, 4)
+#define MII_CFG1_LED_MODE_LINKUP	0
+#define MII_CFG1_LED_ENABLE		BIT(3)
+
+#define MII_CFG2			19
+#define MII_CFG2_SLEEP_REQUEST_TO	GENMASK(1, 0)
+#define MII_CFG2_SLEEP_REQUEST_TO_16MS	0x3
+
+#define MII_INTSRC			21
+
+#define MII_COMMSTAT			23
+#define MII_COMMSTAT_LINK_UP		BIT(15)
+
+#define MII_GENSTAT			24
+#define MII_GENSTAT_PLL_LOCKED		BIT(14)
+
+#define MII_COMMCFG			27
+#define MII_COMMCFG_AUTO_OP		BIT(15)
+
+struct tja11xx_phy_stats {
+	const char	*string;
+	u8		reg;
+	u8		off;
+	u16		mask;
+};
+
+static struct tja11xx_phy_stats tja11xx_hw_stats[] = {
+	{ "phy_symbol_error_count", 20, 0, 0xffff },
+	{ "phy_overtemp_error", 21, 1, BIT(1) },
+	{ "phy_undervolt_error", 21, 3, BIT(3) },
+	{ "phy_polarity_detect", 25, 6, BIT(6) },
+	{ "phy_open_detect", 25, 7, BIT(7) },
+	{ "phy_short_detect", 25, 8, BIT(8) },
+	{ "phy_rem_rcvr_count", 26, 0, 0xff },
+	{ "phy_loc_rcvr_count", 26, 8, 0xff },
+};
+
+static int tja11xx_rmw(struct phy_device *phydev, u8 reg, u16 clr, u16 set)
+{
+	int ret;
+
+	ret = phy_read(phydev, reg);
+	if (ret < 0)
+		return ret;
+
+	ret &= ~clr;
+	ret |= set;
+
+	ret = phy_write(phydev, reg, ret);
+	return ret < 0 ? ret : 0;
+}
+
+static int tja11xx_check(struct phy_device *phydev, u8 reg, u16 clr, u16 set)
+{
+	int i, ret;
+
+	for (i = 0; i < 200; i++) {
+		ret = phy_read(phydev, reg);
+		if (ret < 0)
+			return ret;
+
+		if ((ret & clr) == set)
+			return 0;
+
+		usleep_range(100, 150);
+	}
+
+	return -ETIMEDOUT;
+}
+
+static int tja11xx_rmw_check(struct phy_device *phydev, u8 reg,
+			     u16 clr, u16 set)
+{
+	int ret;
+
+	ret = tja11xx_rmw(phydev, reg, clr, set);
+	if (ret)
+		return ret;
+
+	return tja11xx_check(phydev, reg, clr, set);
+}
+
+static int tja11xx_enable_reg_write(struct phy_device *phydev)
+{
+	return tja11xx_rmw(phydev, MII_ECTRL, MII_ECTRL_CONFIG_EN,
+			   MII_ECTRL_CONFIG_EN);
+}
+
+static int tja11xx_enable_link_control(struct phy_device *phydev, bool en)
+{
+	return tja11xx_rmw(phydev, MII_ECTRL, MII_ECTRL_LINK_CONTROL,
+			   en ? MII_ECTRL_CONFIG_EN : 0);
+}
+
+static int tja11xx_wakeup(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = phy_read(phydev, MII_ECTRL);
+	if (ret < 0)
+		return ret;
+
+	switch (ret & MII_ECTRL_POWER_MODE_MASK) {
+	case MII_ECTRL_POWER_MODE_NO_CHANGE:
+		break;
+	case MII_ECTRL_POWER_MODE_NORMAL:
+		ret = tja11xx_rmw(phydev, MII_ECTRL, MII_ECTRL_WAKE_REQUEST,
+				  MII_ECTRL_WAKE_REQUEST);
+		if (ret)
+			return ret;
+
+		ret = tja11xx_rmw(phydev, MII_ECTRL, MII_ECTRL_WAKE_REQUEST, 0);
+		if (ret)
+			return ret;
+		break;
+	case MII_ECTRL_POWER_MODE_STANDBY:
+		ret = tja11xx_rmw_check(phydev, MII_ECTRL,
+					MII_ECTRL_POWER_MODE_MASK,
+					MII_ECTRL_POWER_MODE_STANDBY);
+		if (ret)
+			return ret;
+
+		ret = tja11xx_rmw(phydev, MII_ECTRL, MII_ECTRL_POWER_MODE_MASK,
+				  MII_ECTRL_POWER_MODE_NORMAL);
+		if (ret)
+			return ret;
+
+		ret = tja11xx_rmw_check(phydev, MII_GENSTAT,
+					MII_GENSTAT_PLL_LOCKED,
+					MII_GENSTAT_PLL_LOCKED);
+		if (ret)
+			return ret;
+
+		return tja11xx_enable_link_control(phydev, true);
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int tja11xx_soft_reset(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = tja11xx_enable_reg_write(phydev);
+	if (ret < 0)
+		return ret;
+
+	ret = phy_write(phydev, 0x0, 0xa100);
+	return ret < 0 ? ret : 0;
+}
+
+static int tja11xx_config_aneg(struct phy_device *phydev)
+{
+	phydev->autoneg = 0;
+	phydev->speed = SPEED_100;
+	phydev->duplex = DUPLEX_FULL;
+	phydev->pause = 0;
+	phydev->asym_pause = 0;
+
+	return 0;
+}
+
+static int tja11xx_config_init(struct phy_device *phydev)
+{
+	u32 features = SUPPORTED_TP | SUPPORTED_MII | SUPPORTED_100baseT_Full;
+	int ret;
+
+	ret = tja11xx_enable_reg_write(phydev);
+	if (ret)
+		return ret;
+
+	phydev->supported &= features;
+	phydev->advertising &= features;
+	phydev->irq = PHY_POLL;
+	tja11xx_config_aneg(phydev);
+
+	switch (phydev->phy_id & PHY_ID_MASK) {
+	case PHY_ID_TJA1100:
+		ret = tja11xx_rmw(phydev, MII_CFG1,
+				  MII_CFG1_AUTO_OP | MII_CFG1_LED_MODE_MASK |
+				  MII_CFG1_LED_ENABLE,
+				  MII_CFG1_AUTO_OP | MII_CFG1_LED_MODE_LINKUP |
+				  MII_CFG1_LED_ENABLE);
+		if (ret)
+			return ret;
+		break;
+	case PHY_ID_TJA1101:
+		ret = tja11xx_rmw(phydev, MII_COMMCFG, MII_COMMCFG_AUTO_OP,
+				  MII_COMMCFG_AUTO_OP);
+		if (ret)
+			return ret;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = tja11xx_rmw(phydev, MII_CFG1, MII_CFG1_SLEEP_CONFIRM, 0);
+	if (ret)
+		return ret;
+
+	ret = tja11xx_rmw(phydev, MII_CFG2, MII_CFG2_SLEEP_REQUEST_TO,
+			  MII_CFG2_SLEEP_REQUEST_TO_16MS);
+	if (ret)
+		return ret;
+
+	ret = tja11xx_wakeup(phydev);
+	if (ret < 0)
+		return ret;
+
+	/* ACK interrupts by reading the status register */
+	ret = phy_read(phydev, MII_INTSRC);
+	if (ret < 0)
+		return ret;
+
+	return 0;
+}
+
+static int tja11xx_read_status(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = genphy_read_status(phydev);
+	if (ret)
+		return ret;
+
+	ret = phy_read(phydev, MII_COMMSTAT);
+	if (ret < 0)
+		return ret;
+
+	phydev->link = phydev->link && !!(ret & MII_COMMSTAT_LINK_UP);
+
+	return 0;
+}
+
+static int tja11xx_get_sset_count(struct phy_device *phydev)
+{
+	return ARRAY_SIZE(tja11xx_hw_stats);
+}
+
+static void tja11xx_get_strings(struct phy_device *phydev, u8 *data)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++) {
+		strncpy(data + i * ETH_GSTRING_LEN,
+			tja11xx_hw_stats[i].string, ETH_GSTRING_LEN);
+	}
+}
+
+static void tja11xx_get_stats(struct phy_device *phydev,
+			      struct ethtool_stats *stats, u64 *data)
+{
+	int i, ret;
+
+	for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++) {
+		ret = phy_read(phydev, tja11xx_hw_stats[i].reg);
+		if (ret < 0) {
+			data[i] = U64_MAX;
+		} else {
+			data[i] = ret & tja11xx_hw_stats[i].mask;
+			data[i] >>= tja11xx_hw_stats[i].off;
+		}
+	}
+}
+
+static struct phy_driver tja11xx_driver[] = {
+	{
+		.phy_id		= PHY_ID_TJA1100,
+		.phy_id_mask	= PHY_ID_MASK,
+		.name		= "NXP TJA1100",
+		.features	= SUPPORTED_MII | SUPPORTED_TP |
+				  SUPPORTED_100baseT_Full,
+		.soft_reset	= tja11xx_soft_reset,
+		.config_init	= tja11xx_config_init,
+		.config_aneg	= tja11xx_config_aneg,
+		.aneg_done	= genphy_aneg_done,
+		.read_status	= tja11xx_read_status,
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
+		.set_loopback   = genphy_loopback,
+		/* Statistics */
+		.get_sset_count = tja11xx_get_sset_count,
+		.get_strings	= tja11xx_get_strings,
+		.get_stats	= tja11xx_get_stats,
+	}, {
+		.phy_id		= PHY_ID_TJA1101,
+		.phy_id_mask	= PHY_ID_MASK,
+		.name		= "NXP TJA1101",
+		.features	= SUPPORTED_MII | SUPPORTED_TP |
+				  SUPPORTED_100baseT_Full,
+		.soft_reset	= tja11xx_soft_reset,
+		.config_init	= tja11xx_config_init,
+		.config_aneg	= tja11xx_config_aneg,
+		.aneg_done	= genphy_aneg_done,
+		.read_status	= tja11xx_read_status,
+		.suspend	= genphy_suspend,
+		.resume		= genphy_resume,
+		.set_loopback   = genphy_loopback,
+		/* Statistics */
+		.get_sset_count = tja11xx_get_sset_count,
+		.get_strings	= tja11xx_get_strings,
+		.get_stats	= tja11xx_get_stats,
+	}
+};
+
+module_phy_driver(tja11xx_driver);
+
+static struct mdio_device_id __maybe_unused tja11xx_tbl[] = {
+	{ PHY_ID_TJA1100, PHY_ID_MASK },
+	{ PHY_ID_TJA1101, PHY_ID_MASK },
+	{ }
+};
+
+MODULE_DEVICE_TABLE(mdio, tja11xx_tbl);
+
+MODULE_AUTHOR("Marek Vasut <marex@denx.de>");
+MODULE_DESCRIPTION("NXP TJA11xx BroadR-Reach PHY driver");
+MODULE_LICENSE("GPL");
-- 
2.18.0

Re: [PATCH] net: phy: tja11xx: Add TJA11xx PHY driver

[Yunsheng Lin]

On 2018/12/13 10:01, Marek Vasut wrote:
> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
> BroadRReach 100BaseT1 PHYs used in automotive.

Hi,
a few minor improvement inline.

> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---
>  drivers/net/phy/Kconfig       |   5 +
>  drivers/net/phy/Makefile      |   1 +
>  drivers/net/phy/nxp-tja11xx.c | 348 ++++++++++++++++++++++++++++++++++
>  3 files changed, 354 insertions(+)
>  create mode 100644 drivers/net/phy/nxp-tja11xx.c
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index cd931cf9dcc26..030960158a2ba 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -356,6 +356,11 @@ config NATIONAL_PHY
>  	---help---
>  	  Currently supports the DP83865 PHY.
>  
> +config NXP_TJA11XX_PHY
> +	tristate "NXP TJA11xx PHYs support"
> +	---help---
> +	  Currently supports the NXP TJA1100 and TJA1101 PHY.
> +
>  config QSEMI_PHY
>  	tristate "Quality Semiconductor PHYs"
>  	---help---
> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
> index 00f097e18c68a..242cb5ffe676c 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -71,6 +71,7 @@ obj-$(CONFIG_MICREL_PHY)	+= micrel.o
>  obj-$(CONFIG_MICROCHIP_PHY)	+= microchip.o
>  obj-$(CONFIG_MICROSEMI_PHY)	+= mscc.o
>  obj-$(CONFIG_NATIONAL_PHY)	+= national.o
> +obj-$(CONFIG_NXP_TJA11XX_PHY)	+= nxp-tja11xx.o
>  obj-$(CONFIG_QSEMI_PHY)		+= qsemi.o
>  obj-$(CONFIG_REALTEK_PHY)	+= realtek.o
>  obj-$(CONFIG_ROCKCHIP_PHY)	+= rockchip.o
> diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
> new file mode 100644
> index 0000000000000..22f9c091befac
> --- /dev/null
> +++ b/drivers/net/phy/nxp-tja11xx.c
> @@ -0,0 +1,348 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* NXP TJA1100 BroadRReach PHY driver
> + *
> + * Copyright (C) 2018 Marek Vasut <marex@denx.de>
> + */
> +#include <linux/delay.h>
> +#include <linux/ethtool.h>
> +#include <linux/kernel.h>
> +#include <linux/mii.h>
> +#include <linux/module.h>
> +#include <linux/phy.h>
> +
> +#define PHY_ID_MASK			0xfffffff0
> +#define PHY_ID_TJA1100			0x0180dc40
> +#define PHY_ID_TJA1101			0x0180dd00
> +
> +#define MII_ECTRL			17
> +#define MII_ECTRL_LINK_CONTROL		BIT(15)
> +#define MII_ECTRL_POWER_MODE_MASK	GENMASK(14, 11)
> +#define MII_ECTRL_POWER_MODE_NO_CHANGE	(0x0 << 11)
> +#define MII_ECTRL_POWER_MODE_NORMAL	(0x3 << 11)
> +#define MII_ECTRL_POWER_MODE_STANDBY	(0xc << 11)
> +#define MII_ECTRL_CONFIG_EN		BIT(2)
> +#define MII_ECTRL_WAKE_REQUEST		BIT(0)
> +
> +#define MII_CFG1			18
> +#define MII_CFG1_AUTO_OP		BIT(14)
> +#define MII_CFG1_SLEEP_CONFIRM		BIT(6)
> +#define MII_CFG1_LED_MODE_MASK		GENMASK(5, 4)
> +#define MII_CFG1_LED_MODE_LINKUP	0
> +#define MII_CFG1_LED_ENABLE		BIT(3)
> +
> +#define MII_CFG2			19
> +#define MII_CFG2_SLEEP_REQUEST_TO	GENMASK(1, 0)
> +#define MII_CFG2_SLEEP_REQUEST_TO_16MS	0x3
> +
> +#define MII_INTSRC			21
> +
> +#define MII_COMMSTAT			23
> +#define MII_COMMSTAT_LINK_UP		BIT(15)
> +
> +#define MII_GENSTAT			24
> +#define MII_GENSTAT_PLL_LOCKED		BIT(14)
> +
> +#define MII_COMMCFG			27
> +#define MII_COMMCFG_AUTO_OP		BIT(15)
> +
> +struct tja11xx_phy_stats {
> +	const char	*string;
> +	u8		reg;
> +	u8		off;
> +	u16		mask;
> +};
> +
> +static struct tja11xx_phy_stats tja11xx_hw_stats[] = {
> +	{ "phy_symbol_error_count", 20, 0, 0xffff },
> +	{ "phy_overtemp_error", 21, 1, BIT(1) },
> +	{ "phy_undervolt_error", 21, 3, BIT(3) },
> +	{ "phy_polarity_detect", 25, 6, BIT(6) },
> +	{ "phy_open_detect", 25, 7, BIT(7) },
> +	{ "phy_short_detect", 25, 8, BIT(8) },
> +	{ "phy_rem_rcvr_count", 26, 0, 0xff },
> +	{ "phy_loc_rcvr_count", 26, 8, 0xff },
> +};
> +
> +static int tja11xx_rmw(struct phy_device *phydev, u8 reg, u16 clr, u16 set)
> +{
> +	int ret;
> +
> +	ret = phy_read(phydev, reg);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret &= ~clr;
> +	ret |= set;
> +
> +	ret = phy_write(phydev, reg, ret);
> +	return ret < 0 ? ret : 0;
> +}

It seems tja11xx_rmw is doing the same thing as __phy_modify ?

> +
> +static int tja11xx_check(struct phy_device *phydev, u8 reg, u16 clr, u16 set)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < 200; i++) {
> +		ret = phy_read(phydev, reg);
> +		if (ret < 0)
> +			return ret;
> +
> +		if ((ret & clr) == set)
> +			return 0;
> +
> +		usleep_range(100, 150);
> +	}
> +
> +	return -ETIMEDOUT;
> +}
> +
> +static int tja11xx_rmw_check(struct phy_device *phydev, u8 reg,
> +			     u16 clr, u16 set)
> +{
> +	int ret;
> +
> +	ret = tja11xx_rmw(phydev, reg, clr, set);
> +	if (ret)
> +		return ret;
> +
> +	return tja11xx_check(phydev, reg, clr, set);
> +}
> +
> +static int tja11xx_enable_reg_write(struct phy_device *phydev)
> +{
> +	return tja11xx_rmw(phydev, MII_ECTRL, MII_ECTRL_CONFIG_EN,
> +			   MII_ECTRL_CONFIG_EN);
> +}
> +
> +static int tja11xx_enable_link_control(struct phy_device *phydev, bool en)

The function name suggests that it only do enabling, but it also do disabling,
maybe:

static int tja11xx_enable_link_controll(struct phy_device *phydev)

static int tja11xx_disable_link_controll(struct phy_device *phydev)

> +{
> +	return tja11xx_rmw(phydev, MII_ECTRL, MII_ECTRL_LINK_CONTROL,
> +			   en ? MII_ECTRL_CONFIG_EN : 0);
> +}
> +
> +static int tja11xx_wakeup(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = phy_read(phydev, MII_ECTRL);
> +	if (ret < 0)
> +		return ret;
> +
> +	switch (ret & MII_ECTRL_POWER_MODE_MASK) {
> +	case MII_ECTRL_POWER_MODE_NO_CHANGE:
> +		break;
> +	case MII_ECTRL_POWER_MODE_NORMAL:
> +		ret = tja11xx_rmw(phydev, MII_ECTRL, MII_ECTRL_WAKE_REQUEST,
> +				  MII_ECTRL_WAKE_REQUEST);
> +		if (ret)
> +			return ret;
> +
> +		ret = tja11xx_rmw(phydev, MII_ECTRL, MII_ECTRL_WAKE_REQUEST, 0);
> +		if (ret)
> +			return ret;
> +		break;
> +	case MII_ECTRL_POWER_MODE_STANDBY:
> +		ret = tja11xx_rmw_check(phydev, MII_ECTRL,
> +					MII_ECTRL_POWER_MODE_MASK,
> +					MII_ECTRL_POWER_MODE_STANDBY);
> +		if (ret)
> +			return ret;
> +
> +		ret = tja11xx_rmw(phydev, MII_ECTRL, MII_ECTRL_POWER_MODE_MASK,
> +				  MII_ECTRL_POWER_MODE_NORMAL);
> +		if (ret)
> +			return ret;
> +
> +		ret = tja11xx_rmw_check(phydev, MII_GENSTAT,
> +					MII_GENSTAT_PLL_LOCKED,
> +					MII_GENSTAT_PLL_LOCKED);
> +		if (ret)
> +			return ret;
> +
> +		return tja11xx_enable_link_control(phydev, true);
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tja11xx_soft_reset(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = tja11xx_enable_reg_write(phydev);
> +	if (ret < 0)

Perhaps if (ret) ? because in tja11xx_config_init it only do if (ret).

> +		return ret;
> +
> +	ret = phy_write(phydev, 0x0, 0xa100);
> +	return ret < 0 ? ret : 0;
> +}
> +
> +static int tja11xx_config_aneg(struct phy_device *phydev)
> +{
> +	phydev->autoneg = 0;
> +	phydev->speed = SPEED_100;
> +	phydev->duplex = DUPLEX_FULL;
> +	phydev->pause = 0;
> +	phydev->asym_pause = 0;
> +
> +	return 0;
> +}
> +
> +static int tja11xx_config_init(struct phy_device *phydev)
> +{
> +	u32 features = SUPPORTED_TP | SUPPORTED_MII | SUPPORTED_100baseT_Full;
> +	int ret;
> +
> +	ret = tja11xx_enable_reg_write(phydev);
> +	if (ret)
> +		return ret;
> +
> +	phydev->supported &= features;
> +	phydev->advertising &= features;
> +	phydev->irq = PHY_POLL;
> +	tja11xx_config_aneg(phydev);
> +
> +	switch (phydev->phy_id & PHY_ID_MASK) {
> +	case PHY_ID_TJA1100:
> +		ret = tja11xx_rmw(phydev, MII_CFG1,
> +				  MII_CFG1_AUTO_OP | MII_CFG1_LED_MODE_MASK |
> +				  MII_CFG1_LED_ENABLE,
> +				  MII_CFG1_AUTO_OP | MII_CFG1_LED_MODE_LINKUP |
> +				  MII_CFG1_LED_ENABLE);
> +		if (ret)
> +			return ret;
> +		break;
> +	case PHY_ID_TJA1101:
> +		ret = tja11xx_rmw(phydev, MII_COMMCFG, MII_COMMCFG_AUTO_OP,
> +				  MII_COMMCFG_AUTO_OP);
> +		if (ret)
> +			return ret;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = tja11xx_rmw(phydev, MII_CFG1, MII_CFG1_SLEEP_CONFIRM, 0);
> +	if (ret)
> +		return ret;
> +
> +	ret = tja11xx_rmw(phydev, MII_CFG2, MII_CFG2_SLEEP_REQUEST_TO,
> +			  MII_CFG2_SLEEP_REQUEST_TO_16MS);
> +	if (ret)
> +		return ret;
> +
> +	ret = tja11xx_wakeup(phydev);
> +	if (ret < 0)

Same here?

> +		return ret;
> +
> +	/* ACK interrupts by reading the status register */
> +	ret = phy_read(phydev, MII_INTSRC);
> +	if (ret < 0)
> +		return ret;
> +
> +	return 0;
> +}
> +
> +static int tja11xx_read_status(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = genphy_read_status(phydev);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_read(phydev, MII_COMMSTAT);
> +	if (ret < 0)
> +		return ret;
> +
> +	phydev->link = phydev->link && !!(ret & MII_COMMSTAT_LINK_UP);
> +
> +	return 0;
> +}
> +
> +static int tja11xx_get_sset_count(struct phy_device *phydev)
> +{
> +	return ARRAY_SIZE(tja11xx_hw_stats);
> +}
> +
> +static void tja11xx_get_strings(struct phy_device *phydev, u8 *data)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++) {
> +		strncpy(data + i * ETH_GSTRING_LEN,
> +			tja11xx_hw_stats[i].string, ETH_GSTRING_LEN);
> +	}
> +}
> +
> +static void tja11xx_get_stats(struct phy_device *phydev,
> +			      struct ethtool_stats *stats, u64 *data)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < ARRAY_SIZE(tja11xx_hw_stats); i++) {
> +		ret = phy_read(phydev, tja11xx_hw_stats[i].reg);
> +		if (ret < 0) {
> +			data[i] = U64_MAX;
> +		} else {
> +			data[i] = ret & tja11xx_hw_stats[i].mask;
> +			data[i] >>= tja11xx_hw_stats[i].off;
> +		}
> +	}
> +}
> +
> +static struct phy_driver tja11xx_driver[] = {
> +	{
> +		.phy_id		= PHY_ID_TJA1100,
> +		.phy_id_mask	= PHY_ID_MASK,
> +		.name		= "NXP TJA1100",
> +		.features	= SUPPORTED_MII | SUPPORTED_TP |
> +				  SUPPORTED_100baseT_Full,
> +		.soft_reset	= tja11xx_soft_reset,
> +		.config_init	= tja11xx_config_init,
> +		.config_aneg	= tja11xx_config_aneg,
> +		.aneg_done	= genphy_aneg_done,
> +		.read_status	= tja11xx_read_status,
> +		.suspend	= genphy_suspend,
> +		.resume		= genphy_resume,
> +		.set_loopback   = genphy_loopback,
> +		/* Statistics */
> +		.get_sset_count = tja11xx_get_sset_count,
> +		.get_strings	= tja11xx_get_strings,
> +		.get_stats	= tja11xx_get_stats,
> +	}, {
> +		.phy_id		= PHY_ID_TJA1101,
> +		.phy_id_mask	= PHY_ID_MASK,
> +		.name		= "NXP TJA1101",
> +		.features	= SUPPORTED_MII | SUPPORTED_TP |
> +				  SUPPORTED_100baseT_Full,
> +		.soft_reset	= tja11xx_soft_reset,
> +		.config_init	= tja11xx_config_init,
> +		.config_aneg	= tja11xx_config_aneg,
> +		.aneg_done	= genphy_aneg_done,
> +		.read_status	= tja11xx_read_status,
> +		.suspend	= genphy_suspend,
> +		.resume		= genphy_resume,
> +		.set_loopback   = genphy_loopback,
> +		/* Statistics */
> +		.get_sset_count = tja11xx_get_sset_count,
> +		.get_strings	= tja11xx_get_strings,
> +		.get_stats	= tja11xx_get_stats,
> +	}
> +};
> +
> +module_phy_driver(tja11xx_driver);
> +
> +static struct mdio_device_id __maybe_unused tja11xx_tbl[] = {
> +	{ PHY_ID_TJA1100, PHY_ID_MASK },
> +	{ PHY_ID_TJA1101, PHY_ID_MASK },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(mdio, tja11xx_tbl);
> +
> +MODULE_AUTHOR("Marek Vasut <marex@denx.de>");
> +MODULE_DESCRIPTION("NXP TJA11xx BroadR-Reach PHY driver");
> +MODULE_LICENSE("GPL");
> 

Re: [PATCH] net: phy: tja11xx: Add TJA11xx PHY driver

[Marek Vasut]

On 12/13/2018 04:49 AM, Yunsheng Lin wrote:
> On 2018/12/13 10:01, Marek Vasut wrote:
>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>> BroadRReach 100BaseT1 PHYs used in automotive.
> 
> Hi,
> a few minor improvement inline.

Hi,

>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
>>  drivers/net/phy/Kconfig       |   5 +
>>  drivers/net/phy/Makefile      |   1 +
>>  drivers/net/phy/nxp-tja11xx.c | 348 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 354 insertions(+)
>>  create mode 100644 drivers/net/phy/nxp-tja11xx.c
>>
>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>> index cd931cf9dcc26..030960158a2ba 100644
>> --- a/drivers/net/phy/Kconfig
>> +++ b/drivers/net/phy/Kconfig
>> @@ -356,6 +356,11 @@ config NATIONAL_PHY
>>  	---help---
>>  	  Currently supports the DP83865 PHY.
>>  
>> +config NXP_TJA11XX_PHY
>> +	tristate "NXP TJA11xx PHYs support"
>> +	---help---
>> +	  Currently supports the NXP TJA1100 and TJA1101 PHY.
>> +
>>  config QSEMI_PHY
>>  	tristate "Quality Semiconductor PHYs"
>>  	---help---
>> diff --git a/drivers/net/phy/Makefile b/drivers/net/phy/Makefile
>> index 00f097e18c68a..242cb5ffe676c 100644
>> --- a/drivers/net/phy/Makefile
>> +++ b/drivers/net/phy/Makefile
>> @@ -71,6 +71,7 @@ obj-$(CONFIG_MICREL_PHY)	+= micrel.o
>>  obj-$(CONFIG_MICROCHIP_PHY)	+= microchip.o
>>  obj-$(CONFIG_MICROSEMI_PHY)	+= mscc.o
>>  obj-$(CONFIG_NATIONAL_PHY)	+= national.o
>> +obj-$(CONFIG_NXP_TJA11XX_PHY)	+= nxp-tja11xx.o
>>  obj-$(CONFIG_QSEMI_PHY)		+= qsemi.o
>>  obj-$(CONFIG_REALTEK_PHY)	+= realtek.o
>>  obj-$(CONFIG_ROCKCHIP_PHY)	+= rockchip.o
>> diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
>> new file mode 100644
>> index 0000000000000..22f9c091befac
>> --- /dev/null
>> +++ b/drivers/net/phy/nxp-tja11xx.c
>> @@ -0,0 +1,348 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/* NXP TJA1100 BroadRReach PHY driver
>> + *
>> + * Copyright (C) 2018 Marek Vasut <marex@denx.de>
>> + */
>> +#include <linux/delay.h>
>> +#include <linux/ethtool.h>
>> +#include <linux/kernel.h>
>> +#include <linux/mii.h>
>> +#include <linux/module.h>
>> +#include <linux/phy.h>
>> +
>> +#define PHY_ID_MASK			0xfffffff0
>> +#define PHY_ID_TJA1100			0x0180dc40
>> +#define PHY_ID_TJA1101			0x0180dd00
>> +
>> +#define MII_ECTRL			17
>> +#define MII_ECTRL_LINK_CONTROL		BIT(15)
>> +#define MII_ECTRL_POWER_MODE_MASK	GENMASK(14, 11)
>> +#define MII_ECTRL_POWER_MODE_NO_CHANGE	(0x0 << 11)
>> +#define MII_ECTRL_POWER_MODE_NORMAL	(0x3 << 11)
>> +#define MII_ECTRL_POWER_MODE_STANDBY	(0xc << 11)
>> +#define MII_ECTRL_CONFIG_EN		BIT(2)
>> +#define MII_ECTRL_WAKE_REQUEST		BIT(0)
>> +
>> +#define MII_CFG1			18
>> +#define MII_CFG1_AUTO_OP		BIT(14)
>> +#define MII_CFG1_SLEEP_CONFIRM		BIT(6)
>> +#define MII_CFG1_LED_MODE_MASK		GENMASK(5, 4)
>> +#define MII_CFG1_LED_MODE_LINKUP	0
>> +#define MII_CFG1_LED_ENABLE		BIT(3)
>> +
>> +#define MII_CFG2			19
>> +#define MII_CFG2_SLEEP_REQUEST_TO	GENMASK(1, 0)
>> +#define MII_CFG2_SLEEP_REQUEST_TO_16MS	0x3
>> +
>> +#define MII_INTSRC			21
>> +
>> +#define MII_COMMSTAT			23
>> +#define MII_COMMSTAT_LINK_UP		BIT(15)
>> +
>> +#define MII_GENSTAT			24
>> +#define MII_GENSTAT_PLL_LOCKED		BIT(14)
>> +
>> +#define MII_COMMCFG			27
>> +#define MII_COMMCFG_AUTO_OP		BIT(15)
>> +
>> +struct tja11xx_phy_stats {
>> +	const char	*string;
>> +	u8		reg;
>> +	u8		off;
>> +	u16		mask;
>> +};
>> +
>> +static struct tja11xx_phy_stats tja11xx_hw_stats[] = {
>> +	{ "phy_symbol_error_count", 20, 0, 0xffff },
>> +	{ "phy_overtemp_error", 21, 1, BIT(1) },
>> +	{ "phy_undervolt_error", 21, 3, BIT(3) },
>> +	{ "phy_polarity_detect", 25, 6, BIT(6) },
>> +	{ "phy_open_detect", 25, 7, BIT(7) },
>> +	{ "phy_short_detect", 25, 8, BIT(8) },
>> +	{ "phy_rem_rcvr_count", 26, 0, 0xff },
>> +	{ "phy_loc_rcvr_count", 26, 8, 0xff },
>> +};
>> +
>> +static int tja11xx_rmw(struct phy_device *phydev, u8 reg, u16 clr, u16 set)
>> +{
>> +	int ret;
>> +
>> +	ret = phy_read(phydev, reg);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret &= ~clr;
>> +	ret |= set;
>> +
>> +	ret = phy_write(phydev, reg, ret);
>> +	return ret < 0 ? ret : 0;
>> +}
> 
> It seems tja11xx_rmw is doing the same thing as __phy_modify ?

phy_modify(), yes, let's use that.

>> +
>> +static int tja11xx_check(struct phy_device *phydev, u8 reg, u16 clr, u16 set)
>> +{
>> +	int i, ret;
>> +
>> +	for (i = 0; i < 200; i++) {
>> +		ret = phy_read(phydev, reg);
>> +		if (ret < 0)
>> +			return ret;
>> +
>> +		if ((ret & clr) == set)
>> +			return 0;
>> +
>> +		usleep_range(100, 150);
>> +	}
>> +
>> +	return -ETIMEDOUT;
>> +}
>> +
>> +static int tja11xx_rmw_check(struct phy_device *phydev, u8 reg,
>> +			     u16 clr, u16 set)
>> +{
>> +	int ret;
>> +
>> +	ret = tja11xx_rmw(phydev, reg, clr, set);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return tja11xx_check(phydev, reg, clr, set);
>> +}
>> +
>> +static int tja11xx_enable_reg_write(struct phy_device *phydev)
>> +{
>> +	return tja11xx_rmw(phydev, MII_ECTRL, MII_ECTRL_CONFIG_EN,
>> +			   MII_ECTRL_CONFIG_EN);
>> +}
>> +
>> +static int tja11xx_enable_link_control(struct phy_device *phydev, bool en)
> 
> The function name suggests that it only do enabling, but it also do disabling,
> maybe:

No, I can just drop the $en

> static int tja11xx_enable_link_controll(struct phy_device *phydev)
> 
> static int tja11xx_disable_link_controll(struct phy_device *phydev)
> 
>> +{
>> +	return tja11xx_rmw(phydev, MII_ECTRL, MII_ECTRL_LINK_CONTROL,
>> +			   en ? MII_ECTRL_CONFIG_EN : 0);
>> +}
>> +
>> +static int tja11xx_wakeup(struct phy_device *phydev)
>> +{
>> +	int ret;
>> +
>> +	ret = phy_read(phydev, MII_ECTRL);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	switch (ret & MII_ECTRL_POWER_MODE_MASK) {
>> +	case MII_ECTRL_POWER_MODE_NO_CHANGE:
>> +		break;
>> +	case MII_ECTRL_POWER_MODE_NORMAL:
>> +		ret = tja11xx_rmw(phydev, MII_ECTRL, MII_ECTRL_WAKE_REQUEST,
>> +				  MII_ECTRL_WAKE_REQUEST);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = tja11xx_rmw(phydev, MII_ECTRL, MII_ECTRL_WAKE_REQUEST, 0);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	case MII_ECTRL_POWER_MODE_STANDBY:
>> +		ret = tja11xx_rmw_check(phydev, MII_ECTRL,
>> +					MII_ECTRL_POWER_MODE_MASK,
>> +					MII_ECTRL_POWER_MODE_STANDBY);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = tja11xx_rmw(phydev, MII_ECTRL, MII_ECTRL_POWER_MODE_MASK,
>> +				  MII_ECTRL_POWER_MODE_NORMAL);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = tja11xx_rmw_check(phydev, MII_GENSTAT,
>> +					MII_GENSTAT_PLL_LOCKED,
>> +					MII_GENSTAT_PLL_LOCKED);
>> +		if (ret)
>> +			return ret;
>> +
>> +		return tja11xx_enable_link_control(phydev, true);
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int tja11xx_soft_reset(struct phy_device *phydev)
>> +{
>> +	int ret;
>> +
>> +	ret = tja11xx_enable_reg_write(phydev);
>> +	if (ret < 0)
> 
> Perhaps if (ret) ? because in tja11xx_config_init it only do if (ret).

Yes

Thanks

-- 
Best regards,
Marek Vasut

Re: [PATCH] net: phy: tja11xx: Add TJA11xx PHY driver

[Andrew Lunn]

> +static int tja11xx_config_aneg(struct phy_device *phydev)
> +{
> +	phydev->autoneg = 0;
> +	phydev->speed = SPEED_100;
> +	phydev->duplex = DUPLEX_FULL;
> +	phydev->pause = 0;
> +	phydev->asym_pause = 0;
> +
> +	return 0;
> +}

Hi Marek

That is, err, interesting. Are you saying the PHY cannot do auto-neg?

Does it happen to be a 100T1 device for automotive?

> +
> +static int tja11xx_config_init(struct phy_device *phydev)
> +{
> +	u32 features = SUPPORTED_TP | SUPPORTED_MII | SUPPORTED_100baseT_Full;
> +	int ret;
> +
> +	ret = tja11xx_enable_reg_write(phydev);
> +	if (ret)
> +		return ret;
> +
> +	phydev->supported &= features;
> +	phydev->advertising &= features;

What tree is this code based on? I doubt it is net-next. Please rebase
and fix up all the things that then break.

    Thanks
	Andrew

Re: [PATCH] net: phy: tja11xx: Add TJA11xx PHY driver

[Marek Vasut]

On 12/13/2018 09:38 AM, Andrew Lunn wrote:
>> +static int tja11xx_config_aneg(struct phy_device *phydev)
>> +{
>> +	phydev->autoneg = 0;
>> +	phydev->speed = SPEED_100;
>> +	phydev->duplex = DUPLEX_FULL;
>> +	phydev->pause = 0;
>> +	phydev->asym_pause = 0;
>> +
>> +	return 0;
>> +}
> 
> Hi Marek

Hi,

> That is, err, interesting. Are you saying the PHY cannot do auto-neg?

There isn't much to auto-negotiate and some of the bits in the standard
registers behave weirdly.

> Does it happen to be a 100T1 device for automotive?

Yes, that's what the commit message says, why ?

>> +
>> +static int tja11xx_config_init(struct phy_device *phydev)
>> +{
>> +	u32 features = SUPPORTED_TP | SUPPORTED_MII | SUPPORTED_100baseT_Full;
>> +	int ret;
>> +
>> +	ret = tja11xx_enable_reg_write(phydev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	phydev->supported &= features;
>> +	phydev->advertising &= features;
> 
> What tree is this code based on? I doubt it is net-next. Please rebase
> and fix up all the things that then break.

4.19ish . Do you have some other review comments on this ?

-- 
Best regards,
Marek Vasut

Re: [PATCH] net: phy: tja11xx: Add TJA11xx PHY driver

[Andrew Lunn]

On Thu, Dec 13, 2018 at 02:27:51PM +0100, Marek Vasut wrote:
> On 12/13/2018 09:38 AM, Andrew Lunn wrote:
> >> +static int tja11xx_config_aneg(struct phy_device *phydev)
> >> +{
> >> +	phydev->autoneg = 0;
> >> +	phydev->speed = SPEED_100;
> >> +	phydev->duplex = DUPLEX_FULL;
> >> +	phydev->pause = 0;
> >> +	phydev->asym_pause = 0;
> >> +
> >> +	return 0;
> >> +}
> > 
> > Hi Marek
> 
> Hi,
> 
> > That is, err, interesting. Are you saying the PHY cannot do auto-neg?
> 
> There isn't much to auto-negotiate and some of the bits in the standard
> registers behave weirdly.

Hi Marek

Hopefully the config_aneg callback is not called if you don't list
autoneg to the .features. The microchip_t1 driver just uses
genphy_config_aneg, but if a NULL works, i would prefer that.

> > Does it happen to be a 100T1 device for automotive?
> 
> Yes, that's what the commit message says, why ?

Sorry, i missed it in the commit message.  We now have enough bits in
the link_mode that we should explicitly say this is 100BaseT2_Full,
not 100BaseT_Full. So please add a new member to
ethtool_link_mode_bit_indices ...
 
> >> +
> >> +static int tja11xx_config_init(struct phy_device *phydev)
> >> +{
> >> +	u32 features = SUPPORTED_TP | SUPPORTED_MII | SUPPORTED_100baseT_Full;

And make it part of the .features. Well, you need to add a new
PHY_BASIC_T2_FEATURES macro, which uses it, and use that in the
phy_driver structure. There should not be any need to modify it in the
config_init call.

We also need to think about what we do with the PHY_BASIC_T1_FEATURES
macro. Ideally we want to swap that to also make use of a new
ethtool_link_mode_bit_indices, but i've no idea at the moment if that
will break something.

  Andrew

Re: [PATCH] net: phy: tja11xx: Add TJA11xx PHY driver

[Marek Vasut]

On 12/13/2018 03:33 PM, Andrew Lunn wrote:
> On Thu, Dec 13, 2018 at 02:27:51PM +0100, Marek Vasut wrote:
>> On 12/13/2018 09:38 AM, Andrew Lunn wrote:
>>>> +static int tja11xx_config_aneg(struct phy_device *phydev)
>>>> +{
>>>> +	phydev->autoneg = 0;
>>>> +	phydev->speed = SPEED_100;
>>>> +	phydev->duplex = DUPLEX_FULL;
>>>> +	phydev->pause = 0;
>>>> +	phydev->asym_pause = 0;
>>>> +
>>>> +	return 0;
>>>> +}
>>>
>>> Hi Marek
>>
>> Hi,
>>
>>> That is, err, interesting. Are you saying the PHY cannot do auto-neg?
>>
>> There isn't much to auto-negotiate and some of the bits in the standard
>> registers behave weirdly.
> 
> Hi Marek

Hi,

> Hopefully the config_aneg callback is not called if you don't list
> autoneg to the .features. The microchip_t1 driver just uses
> genphy_config_aneg, but if a NULL works, i would prefer that.

Without the custom config_aneg which sets speed and duplex, I get a
report claiming the link is at 10/Half , while the link is at 100/Full.
If I force this in the custom config_aneg, the communication works fine.
Do you have a hint for me ?

>>> Does it happen to be a 100T1 device for automotive?
>>
>> Yes, that's what the commit message says, why ?
> 
> Sorry, i missed it in the commit message.  We now have enough bits in
> the link_mode that we should explicitly say this is 100BaseT2_Full,
> not 100BaseT_Full. So please add a new member to
> ethtool_link_mode_bit_indices ...

Oh, nice. Shouldn't this be basic_t1 , which is already supported ?

>>>> +
>>>> +static int tja11xx_config_init(struct phy_device *phydev)
>>>> +{
>>>> +	u32 features = SUPPORTED_TP | SUPPORTED_MII | SUPPORTED_100baseT_Full;
> 
> And make it part of the .features. Well, you need to add a new
> PHY_BASIC_T2_FEATURES macro, which uses it, and use that in the
> phy_driver structure. There should not be any need to modify it in the
> config_init call.

Right

> We also need to think about what we do with the PHY_BASIC_T1_FEATURES
> macro. Ideally we want to swap that to also make use of a new
> ethtool_link_mode_bit_indices, but i've no idea at the moment if that
> will break something.

Do you mind if I skip this part for now , until I get the driver into
better shape ?

-- 
Best regards,
Marek Vasut

Re: [PATCH] net: phy: tja11xx: Add TJA11xx PHY driver

[Florian Fainelli]

Le 12/12/18 à 6:01 PM, Marek Vasut a écrit :
> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
> BroadRReach 100BaseT1 PHYs used in automotive.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> ---

[snip]

> +#define PHY_ID_MASK			0xfffffff0
> +#define PHY_ID_TJA1100			0x0180dc40
> +#define PHY_ID_TJA1101			0x0180dd00
> +
> +#define MII_ECTRL			17

Stylistic preference, but for register offsets, using hexadecimal may be
more natural than using decimal.

[snip]

> +static int tja11xx_soft_reset(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = tja11xx_enable_reg_write(phydev);
> +	if (ret < 0)
> +		return ret;
> +
> +	ret = phy_write(phydev, 0x0, 0xa100);
> +	return ret < 0 ? ret : 0;

Can you utilize genphy_soft_reset() here or at least not open code the
register offset and the bits you write to (use MII_BMCR and
BMCR_FULLDPLX | BMCR_SPEED100 | BMCR_RESET).
-- 
Florian

Re: [PATCH] net: phy: tja11xx: Add TJA11xx PHY driver

[Andrew Lunn]

> Hi,
> 
> > Hopefully the config_aneg callback is not called if you don't list
> > autoneg to the .features. The microchip_t1 driver just uses
> > genphy_config_aneg, but if a NULL works, i would prefer that.
> 
> Without the custom config_aneg which sets speed and duplex, I get a
> report claiming the link is at 10/Half , while the link is at 100/Full.
> If I force this in the custom config_aneg, the communication works fine.
> Do you have a hint for me ?

I can make some guesses, but i've never used a PHY which does not have
auto-neg.

If the config_aneg function is being called, i wonder if
AUTONEG_DISABLE == phydev->autoneg is true? 

Is the read_status function doing the right thing? Does it set the
speed, duplex and link up? I'm not sure that is needed, it looks like
phy_sanitize_settings() should do that. 

> Oh, nice. Shouldn't this be basic_t1 , which is already supported ?

I'm getting the various T mixed up. Yes, basic_t1.

> > We also need to think about what we do with the PHY_BASIC_T1_FEATURES
> > macro. Ideally we want to swap that to also make use of a new
> > ethtool_link_mode_bit_indices, but i've no idea at the moment if that
> > will break something.
> 
> Do you mind if I skip this part for now , until I get the driver into
> better shape ?

For the moment, we can postpone that. Lets get the basics working.
What i'm slightly worried about is this could be an ABI change, and
break older systems. If things work correctly such that T1 is selected
without userspace being involved, no need to set the link parameters,
then we are probably safe. We can also ask Microchip to test there T1
driver to make sure it still works.

       Andrew

Re: [PATCH] net: phy: tja11xx: Add TJA11xx PHY driver

[Marek Vasut]

On 12/13/2018 07:53 PM, Florian Fainelli wrote:
> Le 12/12/18 à 6:01 PM, Marek Vasut a écrit :
>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>> BroadRReach 100BaseT1 PHYs used in automotive.
>>
>> Signed-off-by: Marek Vasut <marex@denx.de>
>> ---
> 
> [snip]
> 
>> +#define PHY_ID_MASK			0xfffffff0
>> +#define PHY_ID_TJA1100			0x0180dc40
>> +#define PHY_ID_TJA1101			0x0180dd00
>> +
>> +#define MII_ECTRL			17
> 
> Stylistic preference, but for register offsets, using hexadecimal may be
> more natural than using decimal.

This is in-line with the TJA1100 datasheet, where they for whatever
reason use decimal offsets, so I'd like to keep it this way. It saves me
one conversion between bases.

> [snip]
> 
>> +static int tja11xx_soft_reset(struct phy_device *phydev)
>> +{
>> +	int ret;
>> +
>> +	ret = tja11xx_enable_reg_write(phydev);
>> +	if (ret < 0)
>> +		return ret;
>> +
>> +	ret = phy_write(phydev, 0x0, 0xa100);
>> +	return ret < 0 ? ret : 0;
> 
> Can you utilize genphy_soft_reset() here or at least not open code the
> register offset and the bits you write to (use MII_BMCR and
> BMCR_FULLDPLX | BMCR_SPEED100 | BMCR_RESET).

genphy_soft_reset() is fine, changed.

-- 
Best regards,
Marek Vasut

Re: [PATCH] net: phy: tja11xx: Add TJA11xx PHY driver

[Marek Vasut]

On 12/14/2018 09:23 AM, Andrew Lunn wrote:
>> Hi,
>>
>>> Hopefully the config_aneg callback is not called if you don't list
>>> autoneg to the .features. The microchip_t1 driver just uses
>>> genphy_config_aneg, but if a NULL works, i would prefer that.
>>
>> Without the custom config_aneg which sets speed and duplex, I get a
>> report claiming the link is at 10/Half , while the link is at 100/Full.
>> If I force this in the custom config_aneg, the communication works fine.
>> Do you have a hint for me ?
> 
> I can make some guesses, but i've never used a PHY which does not have
> auto-neg.
> 
> If the config_aneg function is being called, i wonder if
> AUTONEG_DISABLE == phydev->autoneg is true? 
> 
> Is the read_status function doing the right thing? Does it set the
> speed, duplex and link up? I'm not sure that is needed, it looks like
> phy_sanitize_settings() should do that. 

The read_status just changes the link, nothing else.

But, I dropped all the aneg stuff and that seems to do the right thing,
so I now have a fixed 100/Full link. I think I'll just submit a V2,
since the patch changed a lot.

>> Oh, nice. Shouldn't this be basic_t1 , which is already supported ?
> 
> I'm getting the various T mixed up. Yes, basic_t1.

Hmmm, is there a 10/Full variant of the T1 ?

>>> We also need to think about what we do with the PHY_BASIC_T1_FEATURES
>>> macro. Ideally we want to swap that to also make use of a new
>>> ethtool_link_mode_bit_indices, but i've no idea at the moment if that
>>> will break something.
>>
>> Do you mind if I skip this part for now , until I get the driver into
>> better shape ?
> 
> For the moment, we can postpone that. Lets get the basics working.
> What i'm slightly worried about is this could be an ABI change, and
> break older systems. If things work correctly such that T1 is selected
> without userspace being involved, no need to set the link parameters,
> then we are probably safe. We can also ask Microchip to test there T1
> driver to make sure it still works.

Fine

-- 
Best regards,
Marek Vasut

Re: [PATCH] net: phy: tja11xx: Add TJA11xx PHY driver

[Andrew Lunn]

> >> Oh, nice. Shouldn't this be basic_t1 , which is already supported ?
> > 
> > I'm getting the various T mixed up. Yes, basic_t1.
> 
> Hmmm, is there a 10/Full variant of the T1 ?

10BaseT1 would be 802.3cg. There is also 100BaseT2, 100BaseT4 which
never really happened, 1000BaseT1 and 1000BaseT2.

	  Andrew

Re: [PATCH] net: phy: tja11xx: Add TJA11xx PHY driver

[Marek Vasut]

On 12/14/2018 09:19 PM, Andrew Lunn wrote:
>>>> Oh, nice. Shouldn't this be basic_t1 , which is already supported ?
>>>
>>> I'm getting the various T mixed up. Yes, basic_t1.
>>
>> Hmmm, is there a 10/Full variant of the T1 ?
> 
> 10BaseT1 would be 802.3cg. There is also 100BaseT2, 100BaseT4 which
> never really happened, 1000BaseT1 and 1000BaseT2.

Ah, I see, thanks.

-- 
Best regards,
Marek Vasut