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

[PATCH V2] 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>
---
V2: - Use phy_modify(), phy_{set,clear}_bits()
    - Drop enable argument of tja11xx_enable_link_control()
    - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
      features in config_init callback
    - Use genphy_soft_reset() instead of opencoding the reset sequence.
    - Drop the aneg parts, since the PHY datasheet claims it does not
      support aneg
---
 drivers/net/phy/Kconfig       |   5 +
 drivers/net/phy/Makefile      |   1 +
 drivers/net/phy/nxp-tja11xx.c | 312 ++++++++++++++++++++++++++++++++++
 3 files changed, 318 insertions(+)
 create mode 100644 drivers/net/phy/nxp-tja11xx.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index 3d187cd50eb0..ad7420c95fa6 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -389,6 +389,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 5805c0b7d60e..023e5db8c7cc 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -76,6 +76,7 @@ obj-$(CONFIG_MICROCHIP_PHY)	+= microchip.o
 obj-$(CONFIG_MICROCHIP_T1_PHY)	+= microchip_t1.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_RENESAS_PHY)	+= uPD60620.o
diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
new file mode 100644
index 000000000000..8a3f4f15d215
--- /dev/null
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -0,0 +1,312 @@
+// 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_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 phy_modify_check(struct phy_device *phydev, u8 reg,
+			     u16 clr, u16 set)
+{
+	int ret;
+
+	ret = phy_modify(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 phy_set_bits(phydev, MII_ECTRL, MII_ECTRL_CONFIG_EN);
+}
+
+static int tja11xx_enable_link_control(struct phy_device *phydev)
+{
+	return phy_set_bits(phydev, MII_ECTRL, MII_ECTRL_LINK_CONTROL);
+}
+
+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 = phy_set_bits(phydev, MII_ECTRL, MII_ECTRL_WAKE_REQUEST);
+		if (ret)
+			return ret;
+
+		ret = phy_clear_bits(phydev, MII_ECTRL, MII_ECTRL_WAKE_REQUEST);
+		if (ret)
+			return ret;
+		break;
+	case MII_ECTRL_POWER_MODE_STANDBY:
+		ret = phy_modify_check(phydev, MII_ECTRL,
+					MII_ECTRL_POWER_MODE_MASK,
+					MII_ECTRL_POWER_MODE_STANDBY);
+		if (ret)
+			return ret;
+
+		ret = phy_modify(phydev, MII_ECTRL, MII_ECTRL_POWER_MODE_MASK,
+				 MII_ECTRL_POWER_MODE_NORMAL);
+		if (ret)
+			return ret;
+
+		ret = phy_modify_check(phydev, MII_GENSTAT,
+					MII_GENSTAT_PLL_LOCKED,
+					MII_GENSTAT_PLL_LOCKED);
+		if (ret)
+			return ret;
+
+		return tja11xx_enable_link_control(phydev);
+	default:
+		break;
+	}
+
+	return 0;
+}
+
+static int tja11xx_soft_reset(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = tja11xx_enable_reg_write(phydev);
+	if (ret)
+		return ret;
+
+	return genphy_soft_reset(phydev);
+}
+
+static int tja11xx_config_init(struct phy_device *phydev)
+{
+	int ret;
+
+	ret = tja11xx_enable_reg_write(phydev);
+	if (ret)
+		return ret;
+
+	phydev->irq = PHY_POLL;
+	phydev->autoneg = AUTONEG_DISABLE;
+	phydev->speed = SPEED_100;
+	phydev->duplex = DUPLEX_FULL;
+	phydev->pause = 0;
+	phydev->asym_pause = 0;
+
+	switch (phydev->phy_id & PHY_ID_MASK) {
+	case PHY_ID_TJA1100:
+		ret = phy_modify(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 = phy_set_bits(phydev, MII_COMMCFG, MII_COMMCFG_AUTO_OP);
+		if (ret)
+			return ret;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	ret = phy_clear_bits(phydev, MII_CFG1, MII_CFG1_SLEEP_CONFIRM);
+	if (ret)
+		return ret;
+
+	ret = phy_modify(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_update_link(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       = PHY_BASIC_T1_FEATURES,
+		.soft_reset	= tja11xx_soft_reset,
+		.config_init	= tja11xx_config_init,
+		.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       = PHY_BASIC_T1_FEATURES,
+		.soft_reset	= tja11xx_soft_reset,
+		.config_init	= tja11xx_config_init,
+		.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 BoardR-Reach PHY driver");
+MODULE_LICENSE("GPL");
-- 
2.18.0

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

[Andrew Lunn]

> +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) },

Hi Marek

You have a number of one bit counters here, which is pretty unusual.
The names also don't really suggest they are counters.

Florian, Heiner, do we want to allow this?

	 Andrew

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

[Florian Fainelli]

Le 12/15/18 à 9:01 AM, Andrew Lunn a écrit :
>> +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) },
> 
> Hi Marek
> 
> You have a number of one bit counters here, which is pretty unusual.
> The names also don't really suggest they are counters.
> 
> Florian, Heiner, do we want to allow this?

Would it make sense to register HWMON attributes for "overtemp",
"polarity", "undervolt"? The open/short detect sounds like something we
should add once we finally get to the cable diagnostics support within
the netlink version of ethtool.
-- 
Florian

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

[Andrew Lunn]

On Sat, Dec 15, 2018 at 09:31:54AM -0800, Florian Fainelli wrote:
> Le 12/15/18 à 9:01 AM, Andrew Lunn a écrit :
> >> +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) },
> > 
> > Hi Marek
> > 
> > You have a number of one bit counters here, which is pretty unusual.
> > The names also don't really suggest they are counters.
> > 
> > Florian, Heiner, do we want to allow this?
> 
> Would it make sense to register HWMON attributes for "overtemp",
> "polarity", "undervolt"?

Hi Florian

I just had a quick look at the datasheet. There does not appear to be
an actual temperature value, nor a voltage value. Can you have a hwmon
device with just alarms?

> The open/short detect sounds like something we should add once we
> finally get to the cable diagnostics support within the netlink
> version of ethtool.

Yes, that sounds better,

     Andrew

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

[Heiner Kallweit]

On 15.12.2018 18:01, Andrew Lunn wrote:
>> +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) },
> 
> Hi Marek
> 
> You have a number of one bit counters here, which is pretty unusual.
> The names also don't really suggest they are counters.
> 
Apart from few counters the values seem to be flags. I just think
it could be done in a little bit more readable form, e.g. instead of
{ "phy_short_detect", 25, 8, BIT(8) } use
{ "phy_short_detect", 25, BIT(8) } and in tja11xx_get_stats() then
use FIELD_GET (see linux/bitfield.h).

The idea of HWMON attributes sounds good to me because it allows
to use the flags to trigger actions in a structured way. And I
assume in case of e.g. "PHY undervolt" some monitoring system
would like to be informed (especially because we talk about
automotive here).

> Florian, Heiner, do we want to allow this?
> 
> 	 Andrew
> 

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

[Marek Vasut]

On 12/15/2018 06:38 PM, Heiner Kallweit wrote:
> On 15.12.2018 18:01, Andrew Lunn wrote:
>>> +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) },
>>
>> Hi Marek
>>
>> You have a number of one bit counters here, which is pretty unusual.
>> The names also don't really suggest they are counters.
>>
> Apart from few counters the values seem to be flags. I just think
> it could be done in a little bit more readable form, e.g. instead of
> { "phy_short_detect", 25, 8, BIT(8) } use
> { "phy_short_detect", 25, BIT(8) } and in tja11xx_get_stats() then
> use FIELD_GET (see linux/bitfield.h).

This doesn't work with the counters, it only works with flags. The array
contains both.

> The idea of HWMON attributes sounds good to me because it allows
> to use the flags to trigger actions in a structured way. And I
> assume in case of e.g. "PHY undervolt" some monitoring system
> would like to be informed (especially because we talk about
> automotive here).

I added HWMON_T_CRIT_ALARM and HWMON_I_LCRIT_ALARM for
phy_overtemp_error and phy_undervolt_error respectively.
Are there any other hwmon attributes I can use to replace the flags in
the tja11xx_hw_stats array ?

-- 
Best regards,
Marek Vasut

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

[Heiner Kallweit]

On 22.12.2018 00:22, Marek Vasut wrote:
> On 12/15/2018 06:38 PM, Heiner Kallweit wrote:
>> On 15.12.2018 18:01, Andrew Lunn wrote:
>>>> +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) },
>>>
>>> Hi Marek
>>>
>>> You have a number of one bit counters here, which is pretty unusual.
>>> The names also don't really suggest they are counters.
>>>
>> Apart from few counters the values seem to be flags. I just think
>> it could be done in a little bit more readable form, e.g. instead of
>> { "phy_short_detect", 25, 8, BIT(8) } use
>> { "phy_short_detect", 25, BIT(8) } and in tja11xx_get_stats() then
>> use FIELD_GET (see linux/bitfield.h).
> 
> This doesn't work with the counters, it only works with flags. The array
> contains both.
> 
Maybe we have a misunderstanding here, but FIELD_GET would work also
with the counters. FIELD_GET calculates the field offset, so you don't
need to specify it. Let's say you would have an entry in your array like
this: { "phy_xxx_error_count", 20, 4, 0xfff0 }

Then you would do:
#define XXX_ERROR_CNT_MASK GENMASK(15, 4)
{ "phy_xxx_error_count", 20, XXX_ERROR_CNT_MASK }
and access the value by
val = FIELD_GET(XXX_ERROR_CNT_MASK, reg)


>> The idea of HWMON attributes sounds good to me because it allows
>> to use the flags to trigger actions in a structured way. And I
>> assume in case of e.g. "PHY undervolt" some monitoring system
>> would like to be informed (especially because we talk about
>> automotive here).
> 
> I added HWMON_T_CRIT_ALARM and HWMON_I_LCRIT_ALARM for
> phy_overtemp_error and phy_undervolt_error respectively.
> Are there any other hwmon attributes I can use to replace the flags in
> the tja11xx_hw_stats array ?
> 
I think that's it.

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

[Marek Vasut]

On 12/22/18 6:24 PM, Heiner Kallweit wrote:
> On 22.12.2018 00:22, Marek Vasut wrote:
>> On 12/15/2018 06:38 PM, Heiner Kallweit wrote:
>>> On 15.12.2018 18:01, Andrew Lunn wrote:
>>>>> +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) },
>>>>
>>>> Hi Marek
>>>>
>>>> You have a number of one bit counters here, which is pretty unusual.
>>>> The names also don't really suggest they are counters.
>>>>
>>> Apart from few counters the values seem to be flags. I just think
>>> it could be done in a little bit more readable form, e.g. instead of
>>> { "phy_short_detect", 25, 8, BIT(8) } use
>>> { "phy_short_detect", 25, BIT(8) } and in tja11xx_get_stats() then
>>> use FIELD_GET (see linux/bitfield.h).
>>
>> This doesn't work with the counters, it only works with flags. The array
>> contains both.
>>
> Maybe we have a misunderstanding here, but FIELD_GET would work also
> with the counters. FIELD_GET calculates the field offset, so you don't
> need to specify it. Let's say you would have an entry in your array like
> this: { "phy_xxx_error_count", 20, 4, 0xfff0 }
> 
> Then you would do:
> #define XXX_ERROR_CNT_MASK GENMASK(15, 4)
> { "phy_xxx_error_count", 20, XXX_ERROR_CNT_MASK }
> and access the value by
> val = FIELD_GET(XXX_ERROR_CNT_MASK, reg)

Ah, let's try that then.

>>> The idea of HWMON attributes sounds good to me because it allows
>>> to use the flags to trigger actions in a structured way. And I
>>> assume in case of e.g. "PHY undervolt" some monitoring system
>>> would like to be informed (especially because we talk about
>>> automotive here).
>>
>> I added HWMON_T_CRIT_ALARM and HWMON_I_LCRIT_ALARM for
>> phy_overtemp_error and phy_undervolt_error respectively.
>> Are there any other hwmon attributes I can use to replace the flags in
>> the tja11xx_hw_stats array ?
>>
> I think that's it.

OK

-- 
Best regards,
Marek Vasut

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

[Heiner Kallweit]

On 14.12.2018 17:11, Marek Vasut wrote:
> 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>
> ---
> V2: - Use phy_modify(), phy_{set,clear}_bits()
>     - Drop enable argument of tja11xx_enable_link_control()
>     - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
>       features in config_init callback
>     - Use genphy_soft_reset() instead of opencoding the reset sequence.
>     - Drop the aneg parts, since the PHY datasheet claims it does not
>       support aneg
> ---
>  drivers/net/phy/Kconfig       |   5 +
>  drivers/net/phy/Makefile      |   1 +
>  drivers/net/phy/nxp-tja11xx.c | 312 ++++++++++++++++++++++++++++++++++
>  3 files changed, 318 insertions(+)
>  create mode 100644 drivers/net/phy/nxp-tja11xx.c
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index 3d187cd50eb0..ad7420c95fa6 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -389,6 +389,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 5805c0b7d60e..023e5db8c7cc 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -76,6 +76,7 @@ obj-$(CONFIG_MICROCHIP_PHY)	+= microchip.o
>  obj-$(CONFIG_MICROCHIP_T1_PHY)	+= microchip_t1.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_RENESAS_PHY)	+= uPD60620.o
> diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
> new file mode 100644
> index 000000000000..8a3f4f15d215
> --- /dev/null
> +++ b/drivers/net/phy/nxp-tja11xx.c
> @@ -0,0 +1,312 @@
> +// 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_check(struct phy_device *phydev, u8 reg, u16 clr, u16 set)

Based on how you use "clr" it may be better to call this parameter "mask".

> +{
> +	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 phy_modify_check(struct phy_device *phydev, u8 reg,
> +			     u16 clr, u16 set)

Same here

> +{
> +	int ret;
> +
> +	ret = phy_modify(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 phy_set_bits(phydev, MII_ECTRL, MII_ECTRL_CONFIG_EN);
> +}
> +
> +static int tja11xx_enable_link_control(struct phy_device *phydev)
> +{
> +	return phy_set_bits(phydev, MII_ECTRL, MII_ECTRL_LINK_CONTROL);
> +}
> +
> +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 = phy_set_bits(phydev, MII_ECTRL, MII_ECTRL_WAKE_REQUEST);
> +		if (ret)
> +			return ret;
> +
> +		ret = phy_clear_bits(phydev, MII_ECTRL, MII_ECTRL_WAKE_REQUEST);
> +		if (ret)
> +			return ret;
> +		break;
> +	case MII_ECTRL_POWER_MODE_STANDBY:
> +		ret = phy_modify_check(phydev, MII_ECTRL,
> +					MII_ECTRL_POWER_MODE_MASK,
> +					MII_ECTRL_POWER_MODE_STANDBY);
> +		if (ret)
> +			return ret;
> +
> +		ret = phy_modify(phydev, MII_ECTRL, MII_ECTRL_POWER_MODE_MASK,
> +				 MII_ECTRL_POWER_MODE_NORMAL);
> +		if (ret)
> +			return ret;
> +
> +		ret = phy_modify_check(phydev, MII_GENSTAT,
> +					MII_GENSTAT_PLL_LOCKED,
> +					MII_GENSTAT_PLL_LOCKED);
> +		if (ret)
> +			return ret;
> +
> +		return tja11xx_enable_link_control(phydev);
> +	default:
> +		break;
> +	}
> +
> +	return 0;
> +}
> +
> +static int tja11xx_soft_reset(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = tja11xx_enable_reg_write(phydev);
> +	if (ret)
> +		return ret;
> +
> +	return genphy_soft_reset(phydev);
> +}
> +
> +static int tja11xx_config_init(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = tja11xx_enable_reg_write(phydev);
> +	if (ret)
> +		return ret;
> +
> +	phydev->irq = PHY_POLL;
> +	phydev->autoneg = AUTONEG_DISABLE;
> +	phydev->speed = SPEED_100;
> +	phydev->duplex = DUPLEX_FULL;
> +	phydev->pause = 0;
> +	phydev->asym_pause = 0;
> +
> +	switch (phydev->phy_id & PHY_ID_MASK) {
> +	case PHY_ID_TJA1100:
> +		ret = phy_modify(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 = phy_set_bits(phydev, MII_COMMCFG, MII_COMMCFG_AUTO_OP);
> +		if (ret)
> +			return ret;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	ret = phy_clear_bits(phydev, MII_CFG1, MII_CFG1_SLEEP_CONFIRM);
> +	if (ret)
> +		return ret;
> +
> +	ret = phy_modify(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_update_link(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);
> +

I think better to read would be:

genphy_update_link();
if (link) {
	get_commstat();
	if (!commstat_link_up)
		link = 0
}

This also avoids the extra read of commstat in case link is down.

After having had a brief look at the data sheet it's not clear to
me how this "link up" bit is different from the standard one in BMSR.
Can they ever have different values?

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

Here you may want to use new macro PHY_ID_MATCH_MODEL().
Same for the id table.

> +		.phy_id_mask	= PHY_ID_MASK,
> +		.name		= "NXP TJA1100",
> +		.features       = PHY_BASIC_T1_FEATURES,
> +		.soft_reset	= tja11xx_soft_reset,
> +		.config_init	= tja11xx_config_init,
> +		.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       = PHY_BASIC_T1_FEATURES,
> +		.soft_reset	= tja11xx_soft_reset,
> +		.config_init	= tja11xx_config_init,
> +		.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 BoardR-Reach PHY driver");
> +MODULE_LICENSE("GPL");
> 

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

[Heiner Kallweit]

On 15.12.2018 19:01, Heiner Kallweit wrote:
> On 14.12.2018 17:11, Marek Vasut wrote:
>> 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>
>> ---
>> V2: - Use phy_modify(), phy_{set,clear}_bits()
>>     - Drop enable argument of tja11xx_enable_link_control()
>>     - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
>>       features in config_init callback
>>     - Use genphy_soft_reset() instead of opencoding the reset sequence.
>>     - Drop the aneg parts, since the PHY datasheet claims it does not
>>       support aneg
>> ---
>>  drivers/net/phy/Kconfig       |   5 +
>>  drivers/net/phy/Makefile      |   1 +
>>  drivers/net/phy/nxp-tja11xx.c | 312 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 318 insertions(+)
>>  create mode 100644 drivers/net/phy/nxp-tja11xx.c
>>
>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>> index 3d187cd50eb0..ad7420c95fa6 100644
>> --- a/drivers/net/phy/Kconfig
>> +++ b/drivers/net/phy/Kconfig
>> @@ -389,6 +389,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 5805c0b7d60e..023e5db8c7cc 100644
>> --- a/drivers/net/phy/Makefile
>> +++ b/drivers/net/phy/Makefile
>> @@ -76,6 +76,7 @@ obj-$(CONFIG_MICROCHIP_PHY)	+= microchip.o
>>  obj-$(CONFIG_MICROCHIP_T1_PHY)	+= microchip_t1.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_RENESAS_PHY)	+= uPD60620.o
>> diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
>> new file mode 100644
>> index 000000000000..8a3f4f15d215
>> --- /dev/null
>> +++ b/drivers/net/phy/nxp-tja11xx.c
>> @@ -0,0 +1,312 @@
>> +// 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_check(struct phy_device *phydev, u8 reg, u16 clr, u16 set)
> 
> Based on how you use "clr" it may be better to call this parameter "mask".
> 
>> +{
>> +	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 phy_modify_check(struct phy_device *phydev, u8 reg,
>> +			     u16 clr, u16 set)
> 
> Same here
> 
>> +{
>> +	int ret;
>> +
>> +	ret = phy_modify(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 phy_set_bits(phydev, MII_ECTRL, MII_ECTRL_CONFIG_EN);
>> +}
>> +
>> +static int tja11xx_enable_link_control(struct phy_device *phydev)
>> +{
>> +	return phy_set_bits(phydev, MII_ECTRL, MII_ECTRL_LINK_CONTROL);
>> +}
>> +
>> +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 = phy_set_bits(phydev, MII_ECTRL, MII_ECTRL_WAKE_REQUEST);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = phy_clear_bits(phydev, MII_ECTRL, MII_ECTRL_WAKE_REQUEST);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	case MII_ECTRL_POWER_MODE_STANDBY:
>> +		ret = phy_modify_check(phydev, MII_ECTRL,
>> +					MII_ECTRL_POWER_MODE_MASK,
>> +					MII_ECTRL_POWER_MODE_STANDBY);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = phy_modify(phydev, MII_ECTRL, MII_ECTRL_POWER_MODE_MASK,
>> +				 MII_ECTRL_POWER_MODE_NORMAL);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = phy_modify_check(phydev, MII_GENSTAT,
>> +					MII_GENSTAT_PLL_LOCKED,
>> +					MII_GENSTAT_PLL_LOCKED);
>> +		if (ret)
>> +			return ret;
>> +
>> +		return tja11xx_enable_link_control(phydev);
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int tja11xx_soft_reset(struct phy_device *phydev)
>> +{
>> +	int ret;
>> +
>> +	ret = tja11xx_enable_reg_write(phydev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return genphy_soft_reset(phydev);
>> +}
>> +
>> +static int tja11xx_config_init(struct phy_device *phydev)
>> +{
>> +	int ret;
>> +
>> +	ret = tja11xx_enable_reg_write(phydev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	phydev->irq = PHY_POLL;
>> +	phydev->autoneg = AUTONEG_DISABLE;
>> +	phydev->speed = SPEED_100;

One more thing: In the data sheet there are SPEED_SELECT bits allowing
to set also 10MBit and 1GBit mode. Don't you want to support this?

>> +	phydev->duplex = DUPLEX_FULL;
>> +	phydev->pause = 0;
>> +	phydev->asym_pause = 0;
>> +
>> +	switch (phydev->phy_id & PHY_ID_MASK) {
>> +	case PHY_ID_TJA1100:
>> +		ret = phy_modify(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 = phy_set_bits(phydev, MII_COMMCFG, MII_COMMCFG_AUTO_OP);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = phy_clear_bits(phydev, MII_CFG1, MII_CFG1_SLEEP_CONFIRM);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = phy_modify(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_update_link(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);
>> +
> 
> I think better to read would be:
> 
> genphy_update_link();
> if (link) {
> 	get_commstat();
> 	if (!commstat_link_up)
> 		link = 0
> }
> 
> This also avoids the extra read of commstat in case link is down.
> 
> After having had a brief look at the data sheet it's not clear to
> me how this "link up" bit is different from the standard one in BMSR.
> Can they ever have different values?
> 
>> +	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,
> 
> Here you may want to use new macro PHY_ID_MATCH_MODEL().
> Same for the id table.
> 
>> +		.phy_id_mask	= PHY_ID_MASK,
>> +		.name		= "NXP TJA1100",
>> +		.features       = PHY_BASIC_T1_FEATURES,
>> +		.soft_reset	= tja11xx_soft_reset,
>> +		.config_init	= tja11xx_config_init,
>> +		.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       = PHY_BASIC_T1_FEATURES,
>> +		.soft_reset	= tja11xx_soft_reset,
>> +		.config_init	= tja11xx_config_init,
>> +		.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 BoardR-Reach PHY driver");
>> +MODULE_LICENSE("GPL");
>>
> 

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

[Marek Vasut]

On 12/15/2018 07:06 PM, Heiner Kallweit wrote:

[...]

>>> +static int tja11xx_config_init(struct phy_device *phydev)
>>> +{
>>> +	int ret;
>>> +
>>> +	ret = tja11xx_enable_reg_write(phydev);
>>> +	if (ret)
>>> +		return ret;
>>> +
>>> +	phydev->irq = PHY_POLL;
>>> +	phydev->autoneg = AUTONEG_DISABLE;
>>> +	phydev->speed = SPEED_100;
> 
> One more thing: In the data sheet there are SPEED_SELECT bits allowing
> to set also 10MBit and 1GBit mode. Don't you want to support this?

The bits are there, but they have no influence on the actual symbol rate
of the PHY, that's fixed, so the PHY can only too 100/Full. There are
more bits like that, I presume for "compatibility" reason.

Registering HWMON device sounds good.

I'll deal with the rest of the feedback later (~next week).

-- 
Best regards,
Marek Vasut

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

[Andrew Lunn]

> >> +static int tja11xx_config_init(struct phy_device *phydev)
> >> +{
> >> +	int ret;
> >> +
> >> +	ret = tja11xx_enable_reg_write(phydev);
> >> +	if (ret)
> >> +		return ret;
> >> +
> >> +	phydev->irq = PHY_POLL;
> >> +	phydev->autoneg = AUTONEG_DISABLE;
> >> +	phydev->speed = SPEED_100;
> 
> One more thing: In the data sheet there are SPEED_SELECT bits allowing
> to set also 10MBit and 1GBit mode. Don't you want to support this?

Hi Heiner

Did you read footnote 2?

Speed Select. 00: 10 Mbits; 01: 100Mbit/s, 10: 1000Mbit/s, 11:
reserved. a write access value other than 01 is ignored.

So the bits are there, but not really used. It can only do 100Mbit/s.

   Andrew

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

[Heiner Kallweit]

On 15.12.2018 20:59, Andrew Lunn wrote:
>>>> +static int tja11xx_config_init(struct phy_device *phydev)
>>>> +{
>>>> +	int ret;
>>>> +
>>>> +	ret = tja11xx_enable_reg_write(phydev);
>>>> +	if (ret)
>>>> +		return ret;
>>>> +
>>>> +	phydev->irq = PHY_POLL;
>>>> +	phydev->autoneg = AUTONEG_DISABLE;
>>>> +	phydev->speed = SPEED_100;
>>
>> One more thing: In the data sheet there are SPEED_SELECT bits allowing
>> to set also 10MBit and 1GBit mode. Don't you want to support this?
> 
> Hi Heiner
> 
> Did you read footnote 2?
> 
> Speed Select. 00: 10 Mbits; 01: 100Mbit/s, 10: 1000Mbit/s, 11:
> reserved. a write access value other than 01 is ignored.
> 
Uh, missed that. Thanks for the hint.

> So the bits are there, but not really used. It can only do 100Mbit/s.
> 
>    Andrew
> 
> 

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

[Marek Vasut]

On 12/15/2018 07:01 PM, Heiner Kallweit wrote:
> On 14.12.2018 17:11, Marek Vasut wrote:
>> 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>
>> ---
>> V2: - Use phy_modify(), phy_{set,clear}_bits()
>>     - Drop enable argument of tja11xx_enable_link_control()
>>     - Use PHY_BASIC_T1_FEATURES and dont modify supported/advertised
>>       features in config_init callback
>>     - Use genphy_soft_reset() instead of opencoding the reset sequence.
>>     - Drop the aneg parts, since the PHY datasheet claims it does not
>>       support aneg
>> ---
>>  drivers/net/phy/Kconfig       |   5 +
>>  drivers/net/phy/Makefile      |   1 +
>>  drivers/net/phy/nxp-tja11xx.c | 312 ++++++++++++++++++++++++++++++++++
>>  3 files changed, 318 insertions(+)
>>  create mode 100644 drivers/net/phy/nxp-tja11xx.c
>>
>> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
>> index 3d187cd50eb0..ad7420c95fa6 100644
>> --- a/drivers/net/phy/Kconfig
>> +++ b/drivers/net/phy/Kconfig
>> @@ -389,6 +389,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 5805c0b7d60e..023e5db8c7cc 100644
>> --- a/drivers/net/phy/Makefile
>> +++ b/drivers/net/phy/Makefile
>> @@ -76,6 +76,7 @@ obj-$(CONFIG_MICROCHIP_PHY)	+= microchip.o
>>  obj-$(CONFIG_MICROCHIP_T1_PHY)	+= microchip_t1.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_RENESAS_PHY)	+= uPD60620.o
>> diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
>> new file mode 100644
>> index 000000000000..8a3f4f15d215
>> --- /dev/null
>> +++ b/drivers/net/phy/nxp-tja11xx.c
>> @@ -0,0 +1,312 @@
>> +// 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_check(struct phy_device *phydev, u8 reg, u16 clr, u16 set)
> 
> Based on how you use "clr" it may be better to call this parameter "mask".
> 
>> +{
>> +	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 phy_modify_check(struct phy_device *phydev, u8 reg,
>> +			     u16 clr, u16 set)
> 
> Same here

Fixed

>> +{
>> +	int ret;
>> +
>> +	ret = phy_modify(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 phy_set_bits(phydev, MII_ECTRL, MII_ECTRL_CONFIG_EN);
>> +}
>> +
>> +static int tja11xx_enable_link_control(struct phy_device *phydev)
>> +{
>> +	return phy_set_bits(phydev, MII_ECTRL, MII_ECTRL_LINK_CONTROL);
>> +}
>> +
>> +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 = phy_set_bits(phydev, MII_ECTRL, MII_ECTRL_WAKE_REQUEST);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = phy_clear_bits(phydev, MII_ECTRL, MII_ECTRL_WAKE_REQUEST);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	case MII_ECTRL_POWER_MODE_STANDBY:
>> +		ret = phy_modify_check(phydev, MII_ECTRL,
>> +					MII_ECTRL_POWER_MODE_MASK,
>> +					MII_ECTRL_POWER_MODE_STANDBY);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = phy_modify(phydev, MII_ECTRL, MII_ECTRL_POWER_MODE_MASK,
>> +				 MII_ECTRL_POWER_MODE_NORMAL);
>> +		if (ret)
>> +			return ret;
>> +
>> +		ret = phy_modify_check(phydev, MII_GENSTAT,
>> +					MII_GENSTAT_PLL_LOCKED,
>> +					MII_GENSTAT_PLL_LOCKED);
>> +		if (ret)
>> +			return ret;
>> +
>> +		return tja11xx_enable_link_control(phydev);
>> +	default:
>> +		break;
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int tja11xx_soft_reset(struct phy_device *phydev)
>> +{
>> +	int ret;
>> +
>> +	ret = tja11xx_enable_reg_write(phydev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	return genphy_soft_reset(phydev);
>> +}
>> +
>> +static int tja11xx_config_init(struct phy_device *phydev)
>> +{
>> +	int ret;
>> +
>> +	ret = tja11xx_enable_reg_write(phydev);
>> +	if (ret)
>> +		return ret;
>> +
>> +	phydev->irq = PHY_POLL;
>> +	phydev->autoneg = AUTONEG_DISABLE;
>> +	phydev->speed = SPEED_100;
>> +	phydev->duplex = DUPLEX_FULL;
>> +	phydev->pause = 0;
>> +	phydev->asym_pause = 0;
>> +
>> +	switch (phydev->phy_id & PHY_ID_MASK) {
>> +	case PHY_ID_TJA1100:
>> +		ret = phy_modify(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 = phy_set_bits(phydev, MII_COMMCFG, MII_COMMCFG_AUTO_OP);
>> +		if (ret)
>> +			return ret;
>> +		break;
>> +	default:
>> +		return -EINVAL;
>> +	}
>> +
>> +	ret = phy_clear_bits(phydev, MII_CFG1, MII_CFG1_SLEEP_CONFIRM);
>> +	if (ret)
>> +		return ret;
>> +
>> +	ret = phy_modify(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_update_link(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);
>> +
> 
> I think better to read would be:
> 
> genphy_update_link();
> if (link) {
> 	get_commstat();
> 	if (!commstat_link_up)
> 		link = 0
> }
> 
> This also avoids the extra read of commstat in case link is down.
> 
> After having had a brief look at the data sheet it's not clear to
> me how this "link up" bit is different from the standard one in BMSR.
> Can they ever have different values?

Yes, which is why the check is here. The generic bit seems to be stuck
at times, which is why we check the commstat bit too. It'd be nice to
get a proper explanation from someone from NXP, but I doubt that's ever
gonna happen.

>> +	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,
> 
> Here you may want to use new macro PHY_ID_MATCH_MODEL().
> Same for the id table.

OK

> 
>> +		.phy_id_mask	= PHY_ID_MASK,
>> +		.name		= "NXP TJA1100",
>> +		.features       = PHY_BASIC_T1_FEATURES,
>> +		.soft_reset	= tja11xx_soft_reset,
>> +		.config_init	= tja11xx_config_init,
>> +		.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       = PHY_BASIC_T1_FEATURES,
>> +		.soft_reset	= tja11xx_soft_reset,
>> +		.config_init	= tja11xx_config_init,
>> +		.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 BoardR-Reach PHY driver");
>> +MODULE_LICENSE("GPL");
>>
> 


-- 
Best regards,
Marek Vasut