All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH V5] net: phy: tja11xx: Add TJA11xx PHY driver
@ 2019-05-17 23:51 Marek Vasut
  2019-05-18 14:14 ` Andrew Lunn
                   ` (2 more replies)
  0 siblings, 3 replies; 18+ messages in thread
From: Marek Vasut @ 2019-05-17 23:51 UTC (permalink / raw)
  To: netdev
  Cc: Marek Vasut, Andrew Lunn, Florian Fainelli, Guenter Roeck,
	Heiner Kallweit, Jean Delvare, linux-hwmon

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>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Jean Delvare <jdelvare@suse.com>
Cc: linux-hwmon@vger.kernel.org
---
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
V3: - Replace clr with mask
    - Add hwmon support
    - Check commstat in tja11xx_read_status() only if link is up
    - Use PHY_ID_MATCH_MODEL()
V4: - Use correct bit in tja11xx_hwmon_read() hwmon_temp_crit_alarm
    - Use ENOMEM if devm_kstrdup() fails
    - Check $type in tja11xx_hwmon_read() in addition to $attr
V5: - Drop assignment of phydev->irq,pause,asym_pause
---
 drivers/net/phy/Kconfig       |   6 +
 drivers/net/phy/Makefile      |   1 +
 drivers/net/phy/nxp-tja11xx.c | 423 ++++++++++++++++++++++++++++++++++
 3 files changed, 430 insertions(+)
 create mode 100644 drivers/net/phy/nxp-tja11xx.c

diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
index d6299710d634..31478d8b3c0c 100644
--- a/drivers/net/phy/Kconfig
+++ b/drivers/net/phy/Kconfig
@@ -415,6 +415,12 @@ config NATIONAL_PHY
 	---help---
 	  Currently supports the DP83865 PHY.
 
+config NXP_TJA11XX_PHY
+	tristate "NXP TJA11xx PHYs support"
+	depends on HWMON
+	---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 27d7f9f3b0de..bac339e09042 100644
--- a/drivers/net/phy/Makefile
+++ b/drivers/net/phy/Makefile
@@ -82,6 +82,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..11b8701e78fd
--- /dev/null
+++ b/drivers/net/phy/nxp-tja11xx.c
@@ -0,0 +1,423 @@
+// 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>
+#include <linux/hwmon.h>
+#include <linux/bitfield.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_INTSRC_TEMP_ERR		BIT(1)
+#define MII_INTSRC_UV_ERR		BIT(3)
+
+#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_priv {
+	char		*hwmon_name;
+	struct device	*hwmon_dev;
+};
+
+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, GENMASK(15, 0) },
+	{ "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, GENMASK(7, 0) },
+	{ "phy_loc_rcvr_count", 26, 8, GENMASK(15, 8) },
+};
+
+static int tja11xx_check(struct phy_device *phydev, u8 reg, u16 mask, u16 set)
+{
+	int i, ret;
+
+	for (i = 0; i < 200; i++) {
+		ret = phy_read(phydev, reg);
+		if (ret < 0)
+			return ret;
+
+		if ((ret & mask) == set)
+			return 0;
+
+		usleep_range(100, 150);
+	}
+
+	return -ETIMEDOUT;
+}
+
+static int phy_modify_check(struct phy_device *phydev, u8 reg,
+			    u16 mask, u16 set)
+{
+	int ret;
+
+	ret = phy_modify(phydev, reg, mask, set);
+	if (ret)
+		return ret;
+
+	return tja11xx_check(phydev, reg, mask, 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->autoneg = AUTONEG_DISABLE;
+	phydev->speed = SPEED_100;
+	phydev->duplex = DUPLEX_FULL;
+
+	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;
+
+	if (phydev->link) {
+		ret = phy_read(phydev, MII_COMMSTAT);
+		if (ret < 0)
+			return ret;
+
+		if (!(ret & MII_COMMSTAT_LINK_UP))
+			phydev->link = 0;
+	}
+
+	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 int tja11xx_hwmon_read(struct device *dev,
+			      enum hwmon_sensor_types type,
+			      u32 attr, int channel, long *value)
+{
+	struct phy_device *phydev = dev_get_drvdata(dev);
+	int ret;
+
+	if (type == hwmon_in && attr == hwmon_in_lcrit_alarm) {
+		ret = phy_read(phydev, MII_INTSRC);
+		if (ret < 0)
+			return ret;
+
+		*value = !!(ret & MII_INTSRC_TEMP_ERR);
+		return 0;
+	}
+
+	if (type == hwmon_temp && attr == hwmon_temp_crit_alarm) {
+		ret = phy_read(phydev, MII_INTSRC);
+		if (ret < 0)
+			return ret;
+
+		*value = !!(ret & MII_INTSRC_UV_ERR);
+		return 0;
+	}
+
+	return -EOPNOTSUPP;
+}
+
+static umode_t tja11xx_hwmon_is_visible(const void *data,
+					enum hwmon_sensor_types type,
+					u32 attr, int channel)
+{
+	if (type == hwmon_in && attr == hwmon_in_lcrit_alarm)
+		return 0444;
+
+	if (type == hwmon_temp && attr == hwmon_temp_crit_alarm)
+		return 0444;
+
+	return 0;
+}
+
+static u32 tja11xx_hwmon_in_config[] = {
+	HWMON_I_LCRIT_ALARM,
+	0
+};
+
+static const struct hwmon_channel_info tja11xx_hwmon_in = {
+	.type		= hwmon_in,
+	.config		= tja11xx_hwmon_in_config,
+};
+
+static u32 tja11xx_hwmon_temp_config[] = {
+	HWMON_T_CRIT_ALARM,
+	0
+};
+
+static const struct hwmon_channel_info tja11xx_hwmon_temp = {
+	.type		= hwmon_temp,
+	.config		= tja11xx_hwmon_temp_config,
+};
+
+static const struct hwmon_channel_info *tja11xx_hwmon_info[] = {
+	&tja11xx_hwmon_in,
+	&tja11xx_hwmon_temp,
+	NULL
+};
+
+static const struct hwmon_ops tja11xx_hwmon_hwmon_ops = {
+	.is_visible	= tja11xx_hwmon_is_visible,
+	.read		= tja11xx_hwmon_read,
+};
+
+static const struct hwmon_chip_info tja11xx_hwmon_chip_info = {
+	.ops		= &tja11xx_hwmon_hwmon_ops,
+	.info		= tja11xx_hwmon_info,
+};
+
+static int tja11xx_probe(struct phy_device *phydev)
+{
+	struct device *dev = &phydev->mdio.dev;
+	struct tja11xx_priv *priv;
+	int i;
+
+	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+
+	priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
+	if (!priv->hwmon_name)
+		return -ENOMEM;
+
+	for (i = 0; priv->hwmon_name[i]; i++)
+		if (hwmon_is_bad_char(priv->hwmon_name[i]))
+			priv->hwmon_name[i] = '_';
+
+	priv->hwmon_dev =
+		devm_hwmon_device_register_with_info(dev, priv->hwmon_name,
+						     phydev,
+						     &tja11xx_hwmon_chip_info,
+						     NULL);
+
+	return PTR_ERR_OR_ZERO(priv->hwmon_dev);
+}
+
+static struct phy_driver tja11xx_driver[] = {
+	{
+		PHY_ID_MATCH_MODEL(PHY_ID_TJA1100),
+		.name		= "NXP TJA1100",
+		.features       = PHY_BASIC_T1_FEATURES,
+		.probe		= tja11xx_probe,
+		.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_MATCH_MODEL(PHY_ID_TJA1101),
+		.name		= "NXP TJA1101",
+		.features       = PHY_BASIC_T1_FEATURES,
+		.probe		= tja11xx_probe,
+		.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_MATCH_MODEL(PHY_ID_TJA1100) },
+	{ PHY_ID_MATCH_MODEL(PHY_ID_TJA1101) },
+	{ }
+};
+
+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.20.1


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH V5] net: phy: tja11xx: Add TJA11xx PHY driver
  2019-05-17 23:51 [PATCH V5] net: phy: tja11xx: Add TJA11xx PHY driver Marek Vasut
@ 2019-05-18 14:14 ` Andrew Lunn
  2019-05-18 16:50   ` Marek Vasut
  2019-05-22 21:48   ` Marek Vasut
  2019-05-23  2:38 ` Florian Fainelli
  2019-05-27 15:44 ` Guenter Roeck
  2 siblings, 2 replies; 18+ messages in thread
From: Andrew Lunn @ 2019-05-18 14:14 UTC (permalink / raw)
  To: Marek Vasut
  Cc: netdev, Florian Fainelli, Guenter Roeck, Heiner Kallweit,
	Jean Delvare, linux-hwmon

On Sat, May 18, 2019 at 01:51:23AM +0200, Marek Vasut wrote:
> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
> BroadRReach 100BaseT1 PHYs used in automotive.

Hi Marek

> +	}, {
> +		PHY_ID_MATCH_MODEL(PHY_ID_TJA1101),
> +		.name		= "NXP TJA1101",
> +		.features       = PHY_BASIC_T1_FEATURES,

One thing i would like to do before this patch goes in is define
ETHTOOL_LINK_MODE_100baseT1_Full_BIT in ethtool.h, and use it here.
We could not do it earlier because were ran out of bits. But with
PHYLIB now using bitmaps, rather than u32, we can.

Once net-next reopens i will submit a patch adding it.

I also see in the data sheet we should be able to correct detect its
features using register 15. So we should extend
genphy_read_abilities(). That will allow us to avoid changing
PHY_BASIC_T1_FEATURES and possibly breaking backwards compatibility of
other T1 PHY which currently say they are plain 100BaseT.

      Andrew

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH V5] net: phy: tja11xx: Add TJA11xx PHY driver
  2019-05-18 14:14 ` Andrew Lunn
@ 2019-05-18 16:50   ` Marek Vasut
  2019-05-18 20:12     ` Andrew Lunn
  2019-05-20 17:21     ` Florian Fainelli
  2019-05-22 21:48   ` Marek Vasut
  1 sibling, 2 replies; 18+ messages in thread
From: Marek Vasut @ 2019-05-18 16:50 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, Florian Fainelli, Guenter Roeck, Heiner Kallweit,
	Jean Delvare, linux-hwmon

On 5/18/19 4:14 PM, Andrew Lunn wrote:
> On Sat, May 18, 2019 at 01:51:23AM +0200, Marek Vasut wrote:
>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>> BroadRReach 100BaseT1 PHYs used in automotive.
> 
> Hi Marek

Hello Andrew,

>> +	}, {
>> +		PHY_ID_MATCH_MODEL(PHY_ID_TJA1101),
>> +		.name		= "NXP TJA1101",
>> +		.features       = PHY_BASIC_T1_FEATURES,
> 
> One thing i would like to do before this patch goes in is define
> ETHTOOL_LINK_MODE_100baseT1_Full_BIT in ethtool.h, and use it here.
> We could not do it earlier because were ran out of bits. But with
> PHYLIB now using bitmaps, rather than u32, we can.
> 
> Once net-next reopens i will submit a patch adding it.

I can understand blocking patches from being applied if they have review
problems or need to be updated on some existing or even posted feature.
But blocking a patch because some future yet-to-be-developed patch is a
bit odd.

Besides, this sounds more like a cleanup which can very well be done
later. It will surely be done for the other PHY drivers too.

> I also see in the data sheet we should be able to correct detect its
> features using register 15. So we should extend
> genphy_read_abilities().

Which bits do you refer to ?

Anyway, this is something which can be done in a subsequent patch, I
don't see a reason for blocking hardware enablement because of this.

> That will allow us to avoid changing
> PHY_BASIC_T1_FEATURES and possibly breaking backwards compatibility of
> other T1 PHY which currently say they are plain 100BaseT.

What sort of backward compatibility ?

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH V5] net: phy: tja11xx: Add TJA11xx PHY driver
  2019-05-18 16:50   ` Marek Vasut
@ 2019-05-18 20:12     ` Andrew Lunn
  2019-05-18 20:46       ` Marek Vasut
  2019-05-20 17:21     ` Florian Fainelli
  1 sibling, 1 reply; 18+ messages in thread
From: Andrew Lunn @ 2019-05-18 20:12 UTC (permalink / raw)
  To: Marek Vasut
  Cc: netdev, Florian Fainelli, Guenter Roeck, Heiner Kallweit,
	Jean Delvare, linux-hwmon

On Sat, May 18, 2019 at 06:50:48PM +0200, Marek Vasut wrote:
> On 5/18/19 4:14 PM, Andrew Lunn wrote:
> > On Sat, May 18, 2019 at 01:51:23AM +0200, Marek Vasut wrote:
> >> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
> >> BroadRReach 100BaseT1 PHYs used in automotive.
> > 
> > Hi Marek
> 
> Hello Andrew,
> 
> >> +	}, {
> >> +		PHY_ID_MATCH_MODEL(PHY_ID_TJA1101),
> >> +		.name		= "NXP TJA1101",
> >> +		.features       = PHY_BASIC_T1_FEATURES,
> > 
> > One thing i would like to do before this patch goes in is define
> > ETHTOOL_LINK_MODE_100baseT1_Full_BIT in ethtool.h, and use it here.
> > We could not do it earlier because were ran out of bits. But with
> > PHYLIB now using bitmaps, rather than u32, we can.
> > 
> > Once net-next reopens i will submit a patch adding it.
> 
> I can understand blocking patches from being applied if they have review
> problems or need to be updated on some existing or even posted feature.
> But blocking a patch because some future yet-to-be-developed patch is a
> bit odd.

Hi Marek

What i'm trying to avoid is an ABI change. By using
PHY_BASIC_T1_FEATURES you are saying the device support 100BaseT. It
does not. It supports 100BaseT1. I want to add 100BaseT1 first, so
your PHY does not change from 100BaseT to 100BaseT1, which could be
considered an ABI change.

I'm not suggesting blocking your patch for a long time. I'm already
2/3 of the way doing the work. At the latest, i expect to have patches
submitted in the next few days. And then your driver can go in, using
this. So by end of next week, your driver can be in.

> > I also see in the data sheet we should be able to correct detect its
> > features using register 15. So we should extend
> > genphy_read_abilities().
> 
> Which bits do you refer to ?

Register 15, bit 7. This indicates the PHY can do 100BaseT1. I want to
double check with the 802.3 standard, but i expect this is part of the
standard.

	Andrew

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH V5] net: phy: tja11xx: Add TJA11xx PHY driver
  2019-05-18 20:12     ` Andrew Lunn
@ 2019-05-18 20:46       ` Marek Vasut
  0 siblings, 0 replies; 18+ messages in thread
From: Marek Vasut @ 2019-05-18 20:46 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, Florian Fainelli, Guenter Roeck, Heiner Kallweit,
	Jean Delvare, linux-hwmon

On 5/18/19 10:12 PM, Andrew Lunn wrote:
> On Sat, May 18, 2019 at 06:50:48PM +0200, Marek Vasut wrote:
>> On 5/18/19 4:14 PM, Andrew Lunn wrote:
>>> On Sat, May 18, 2019 at 01:51:23AM +0200, Marek Vasut wrote:
>>>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>>>> BroadRReach 100BaseT1 PHYs used in automotive.
>>>
>>> Hi Marek
>>
>> Hello Andrew,
>>
>>>> +	}, {
>>>> +		PHY_ID_MATCH_MODEL(PHY_ID_TJA1101),
>>>> +		.name		= "NXP TJA1101",
>>>> +		.features       = PHY_BASIC_T1_FEATURES,
>>>
>>> One thing i would like to do before this patch goes in is define
>>> ETHTOOL_LINK_MODE_100baseT1_Full_BIT in ethtool.h, and use it here.
>>> We could not do it earlier because were ran out of bits. But with
>>> PHYLIB now using bitmaps, rather than u32, we can.
>>>
>>> Once net-next reopens i will submit a patch adding it.
>>
>> I can understand blocking patches from being applied if they have review
>> problems or need to be updated on some existing or even posted feature.
>> But blocking a patch because some future yet-to-be-developed patch is a
>> bit odd.
> 
> Hi Marek
> 
> What i'm trying to avoid is an ABI change. By using
> PHY_BASIC_T1_FEATURES you are saying the device support 100BaseT. It
> does not. It supports 100BaseT1. I want to add 100BaseT1 first, so
> your PHY does not change from 100BaseT to 100BaseT1, which could be
> considered an ABI change.

I would expect PHY_BASIC_T1_FEATURES , with T1 in the middle , would
imply 100baseT1 , but maybe that's a misnomer .

> I'm not suggesting blocking your patch for a long time. I'm already
> 2/3 of the way doing the work. At the latest, i expect to have patches
> submitted in the next few days. And then your driver can go in, using
> this. So by end of next week, your driver can be in.

But someone has to review your patches too, so that would add another
few weeks.

>>> I also see in the data sheet we should be able to correct detect its
>>> features using register 15. So we should extend
>>> genphy_read_abilities().
>>
>> Which bits do you refer to ?
> 
> Register 15, bit 7. This indicates the PHY can do 100BaseT1. I want to
> double check with the 802.3 standard, but i expect this is part of the
> standard.

The PHY does only 100BaseT1 .

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH V5] net: phy: tja11xx: Add TJA11xx PHY driver
  2019-05-18 16:50   ` Marek Vasut
  2019-05-18 20:12     ` Andrew Lunn
@ 2019-05-20 17:21     ` Florian Fainelli
  1 sibling, 0 replies; 18+ messages in thread
From: Florian Fainelli @ 2019-05-20 17:21 UTC (permalink / raw)
  To: Marek Vasut, Andrew Lunn
  Cc: netdev, Guenter Roeck, Heiner Kallweit, Jean Delvare, linux-hwmon

On May 18, 2019 9:50:48 AM PDT, Marek Vasut <marex@denx.de> wrote:
>On 5/18/19 4:14 PM, Andrew Lunn wrote:
>> On Sat, May 18, 2019 at 01:51:23AM +0200, Marek Vasut wrote:
>>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are
>special
>>> BroadRReach 100BaseT1 PHYs used in automotive.
>> 
>> Hi Marek
>
>Hello Andrew,
>
>>> +	}, {
>>> +		PHY_ID_MATCH_MODEL(PHY_ID_TJA1101),
>>> +		.name		= "NXP TJA1101",
>>> +		.features       = PHY_BASIC_T1_FEATURES,
>> 
>> One thing i would like to do before this patch goes in is define
>> ETHTOOL_LINK_MODE_100baseT1_Full_BIT in ethtool.h, and use it here.
>> We could not do it earlier because were ran out of bits. But with
>> PHYLIB now using bitmaps, rather than u32, we can.
>> 
>> Once net-next reopens i will submit a patch adding it.
>
>I can understand blocking patches from being applied if they have
>review
>problems or need to be updated on some existing or even posted feature.
>But blocking a patch because some future yet-to-be-developed patch is a
>bit odd.
>

The net-next tree is currently closed which means there is ample time
for you and Andrew to work offline and have you submit both Andrew's
cleanup patches as well as this very one as a patch series when net-next
opens back up giving both of you your own cookies. Once submitted it
won't take weeks to get merged more like hours given David's typical
review and patch acceptance time.
--
Florian

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH V5] net: phy: tja11xx: Add TJA11xx PHY driver
  2019-05-18 14:14 ` Andrew Lunn
  2019-05-18 16:50   ` Marek Vasut
@ 2019-05-22 21:48   ` Marek Vasut
  2019-05-22 22:14     ` Andrew Lunn
  1 sibling, 1 reply; 18+ messages in thread
From: Marek Vasut @ 2019-05-22 21:48 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, Florian Fainelli, Guenter Roeck, Heiner Kallweit,
	Jean Delvare, linux-hwmon

On 5/18/19 4:14 PM, Andrew Lunn wrote:
> On Sat, May 18, 2019 at 01:51:23AM +0200, Marek Vasut wrote:
>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>> BroadRReach 100BaseT1 PHYs used in automotive.
> 
> Hi Marek
> 
>> +	}, {
>> +		PHY_ID_MATCH_MODEL(PHY_ID_TJA1101),
>> +		.name		= "NXP TJA1101",
>> +		.features       = PHY_BASIC_T1_FEATURES,
> 
> One thing i would like to do before this patch goes in is define
> ETHTOOL_LINK_MODE_100baseT1_Full_BIT in ethtool.h, and use it here.
> We could not do it earlier because were ran out of bits. But with
> PHYLIB now using bitmaps, rather than u32, we can.

So now that "[PATCH net-next 0/2] net: phy: T1 support" is posted, does
this patch require any change ? I don't think it does ?

[...]


-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH V5] net: phy: tja11xx: Add TJA11xx PHY driver
  2019-05-22 21:48   ` Marek Vasut
@ 2019-05-22 22:14     ` Andrew Lunn
  2019-05-22 22:42       ` Marek Vasut
  0 siblings, 1 reply; 18+ messages in thread
From: Andrew Lunn @ 2019-05-22 22:14 UTC (permalink / raw)
  To: Marek Vasut
  Cc: netdev, Florian Fainelli, Guenter Roeck, Heiner Kallweit,
	Jean Delvare, linux-hwmon

On Wed, May 22, 2019 at 11:48:06PM +0200, Marek Vasut wrote:
> On 5/18/19 4:14 PM, Andrew Lunn wrote:
> > On Sat, May 18, 2019 at 01:51:23AM +0200, Marek Vasut wrote:
> >> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
> >> BroadRReach 100BaseT1 PHYs used in automotive.
> > 
> > Hi Marek
> > 
> >> +	}, {
> >> +		PHY_ID_MATCH_MODEL(PHY_ID_TJA1101),
> >> +		.name		= "NXP TJA1101",
> >> +		.features       = PHY_BASIC_T1_FEATURES,
> > 
> > One thing i would like to do before this patch goes in is define
> > ETHTOOL_LINK_MODE_100baseT1_Full_BIT in ethtool.h, and use it here.
> > We could not do it earlier because were ran out of bits. But with
> > PHYLIB now using bitmaps, rather than u32, we can.
> 
> So now that "[PATCH net-next 0/2] net: phy: T1 support" is posted, does
> this patch require any change ? I don't think it does ?

Hi Marek

In the end, no it does not need any changes because of my patchset.

   Andrew

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH V5] net: phy: tja11xx: Add TJA11xx PHY driver
  2019-05-22 22:14     ` Andrew Lunn
@ 2019-05-22 22:42       ` Marek Vasut
  0 siblings, 0 replies; 18+ messages in thread
From: Marek Vasut @ 2019-05-22 22:42 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: netdev, Florian Fainelli, Guenter Roeck, Heiner Kallweit,
	Jean Delvare, linux-hwmon

On 5/23/19 12:14 AM, Andrew Lunn wrote:
> On Wed, May 22, 2019 at 11:48:06PM +0200, Marek Vasut wrote:
>> On 5/18/19 4:14 PM, Andrew Lunn wrote:
>>> On Sat, May 18, 2019 at 01:51:23AM +0200, Marek Vasut wrote:
>>>> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
>>>> BroadRReach 100BaseT1 PHYs used in automotive.
>>>
>>> Hi Marek
>>>
>>>> +	}, {
>>>> +		PHY_ID_MATCH_MODEL(PHY_ID_TJA1101),
>>>> +		.name		= "NXP TJA1101",
>>>> +		.features       = PHY_BASIC_T1_FEATURES,
>>>
>>> One thing i would like to do before this patch goes in is define
>>> ETHTOOL_LINK_MODE_100baseT1_Full_BIT in ethtool.h, and use it here.
>>> We could not do it earlier because were ran out of bits. But with
>>> PHYLIB now using bitmaps, rather than u32, we can.
>>
>> So now that "[PATCH net-next 0/2] net: phy: T1 support" is posted, does
>> this patch require any change ? I don't think it does ?
> 
> Hi Marek
> 
> In the end, no it does not need any changes because of my patchset.

OK, thanks.

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH V5] net: phy: tja11xx: Add TJA11xx PHY driver
  2019-05-17 23:51 [PATCH V5] net: phy: tja11xx: Add TJA11xx PHY driver Marek Vasut
  2019-05-18 14:14 ` Andrew Lunn
@ 2019-05-23  2:38 ` Florian Fainelli
  2019-05-23 19:48   ` Vladimir Oltean
  2019-05-27 15:44 ` Guenter Roeck
  2 siblings, 1 reply; 18+ messages in thread
From: Florian Fainelli @ 2019-05-23  2:38 UTC (permalink / raw)
  To: Marek Vasut, netdev, Vladimir Oltean
  Cc: Andrew Lunn, Guenter Roeck, Heiner Kallweit, Jean Delvare, linux-hwmon

+Vladimir,

On 5/17/2019 4:51 PM, 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>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: linux-hwmon@vger.kernel.org

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH V5] net: phy: tja11xx: Add TJA11xx PHY driver
  2019-05-23  2:38 ` Florian Fainelli
@ 2019-05-23 19:48   ` Vladimir Oltean
  2019-05-24 11:41     ` Marek Vasut
  0 siblings, 1 reply; 18+ messages in thread
From: Vladimir Oltean @ 2019-05-23 19:48 UTC (permalink / raw)
  To: Florian Fainelli
  Cc: Marek Vasut, netdev, Andrew Lunn, Guenter Roeck, Heiner Kallweit,
	Jean Delvare, linux-hwmon

On Thu, 23 May 2019 at 05:39, Florian Fainelli <f.fainelli@gmail.com> wrote:
>
> +Vladimir,
>
> On 5/17/2019 4:51 PM, 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>
> > Cc: Andrew Lunn <andrew@lunn.ch>
> > Cc: Florian Fainelli <f.fainelli@gmail.com>
> > Cc: Guenter Roeck <linux@roeck-us.net>
> > Cc: Heiner Kallweit <hkallweit1@gmail.com>
> > Cc: Jean Delvare <jdelvare@suse.com>
> > Cc: linux-hwmon@vger.kernel.org
>
> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> --
> Florian

My only feedback is: keep up the good work!

-Vladimir

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH V5] net: phy: tja11xx: Add TJA11xx PHY driver
  2019-05-23 19:48   ` Vladimir Oltean
@ 2019-05-24 11:41     ` Marek Vasut
  2019-05-24 13:52       ` Andrew Lunn
  0 siblings, 1 reply; 18+ messages in thread
From: Marek Vasut @ 2019-05-24 11:41 UTC (permalink / raw)
  To: Vladimir Oltean, Florian Fainelli
  Cc: netdev, Andrew Lunn, Guenter Roeck, Heiner Kallweit,
	Jean Delvare, linux-hwmon

On 5/23/19 9:48 PM, Vladimir Oltean wrote:
> On Thu, 23 May 2019 at 05:39, Florian Fainelli <f.fainelli@gmail.com> wrote:
>>
>> +Vladimir,
>>
>> On 5/17/2019 4:51 PM, 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>
>>> Cc: Andrew Lunn <andrew@lunn.ch>
>>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>>> Cc: Guenter Roeck <linux@roeck-us.net>
>>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>>> Cc: Jean Delvare <jdelvare@suse.com>
>>> Cc: linux-hwmon@vger.kernel.org
>>
>> Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
>> --
>> Florian
> 
> My only feedback is: keep up the good work!

Well, it seems this patch is flagged in patchwork as "changes requested"
. I don't know what changes are requested though :-(

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH V5] net: phy: tja11xx: Add TJA11xx PHY driver
  2019-05-24 11:41     ` Marek Vasut
@ 2019-05-24 13:52       ` Andrew Lunn
  2019-05-24 14:23         ` Marek Vasut
  0 siblings, 1 reply; 18+ messages in thread
From: Andrew Lunn @ 2019-05-24 13:52 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Vladimir Oltean, Florian Fainelli, netdev, Guenter Roeck,
	Heiner Kallweit, Jean Delvare, linux-hwmon

> Well, it seems this patch is flagged in patchwork as "changes requested"
> . I don't know what changes are requested though :-(

Hi Marek

The patch was submitted while net-next as closed. That is pretty much
an automatic reject.

Please submit it again, and add on the review tags you have received.
It should then get accepted.

   Andrew

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH V5] net: phy: tja11xx: Add TJA11xx PHY driver
  2019-05-24 13:52       ` Andrew Lunn
@ 2019-05-24 14:23         ` Marek Vasut
  2019-05-24 14:28           ` Andrew Lunn
  0 siblings, 1 reply; 18+ messages in thread
From: Marek Vasut @ 2019-05-24 14:23 UTC (permalink / raw)
  To: Andrew Lunn
  Cc: Vladimir Oltean, Florian Fainelli, netdev, Guenter Roeck,
	Heiner Kallweit, Jean Delvare, linux-hwmon

On 5/24/19 3:52 PM, Andrew Lunn wrote:
>> Well, it seems this patch is flagged in patchwork as "changes requested"
>> . I don't know what changes are requested though :-(
> 
> Hi Marek

Hi,

> The patch was submitted while net-next as closed. That is pretty much
> an automatic reject.
> 
> Please submit it again, and add on the review tags you have received.
> It should then get accepted.

Would be nice to get some sort of notification.

Are all these subtleties documented anywhere ?
Is there some calendar when to send / not-send patches ?

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH V5] net: phy: tja11xx: Add TJA11xx PHY driver
  2019-05-24 14:23         ` Marek Vasut
@ 2019-05-24 14:28           ` Andrew Lunn
  0 siblings, 0 replies; 18+ messages in thread
From: Andrew Lunn @ 2019-05-24 14:28 UTC (permalink / raw)
  To: Marek Vasut
  Cc: Vladimir Oltean, Florian Fainelli, netdev, Guenter Roeck,
	Heiner Kallweit, Jean Delvare, linux-hwmon

> Are all these subtleties documented anywhere ?

The netdev FAQ.

    Andrew

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH V5] net: phy: tja11xx: Add TJA11xx PHY driver
  2019-05-17 23:51 [PATCH V5] net: phy: tja11xx: Add TJA11xx PHY driver Marek Vasut
  2019-05-18 14:14 ` Andrew Lunn
  2019-05-23  2:38 ` Florian Fainelli
@ 2019-05-27 15:44 ` Guenter Roeck
  2019-05-28 18:17   ` Marek Vasut
  2019-05-28 19:34   ` Andrew Lunn
  2 siblings, 2 replies; 18+ messages in thread
From: Guenter Roeck @ 2019-05-27 15:44 UTC (permalink / raw)
  To: Marek Vasut, netdev
  Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, Jean Delvare,
	linux-hwmon

On 5/17/19 4:51 PM, 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>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Cc: Jean Delvare <jdelvare@suse.com>
> Cc: linux-hwmon@vger.kernel.org
> ---
> 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
> V3: - Replace clr with mask
>      - Add hwmon support
>      - Check commstat in tja11xx_read_status() only if link is up
>      - Use PHY_ID_MATCH_MODEL()
> V4: - Use correct bit in tja11xx_hwmon_read() hwmon_temp_crit_alarm
>      - Use ENOMEM if devm_kstrdup() fails
>      - Check $type in tja11xx_hwmon_read() in addition to $attr
> V5: - Drop assignment of phydev->irq,pause,asym_pause
> ---
>   drivers/net/phy/Kconfig       |   6 +
>   drivers/net/phy/Makefile      |   1 +
>   drivers/net/phy/nxp-tja11xx.c | 423 ++++++++++++++++++++++++++++++++++
>   3 files changed, 430 insertions(+)
>   create mode 100644 drivers/net/phy/nxp-tja11xx.c
> 
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index d6299710d634..31478d8b3c0c 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -415,6 +415,12 @@ config NATIONAL_PHY
>   	---help---
>   	  Currently supports the DP83865 PHY.
>   
> +config NXP_TJA11XX_PHY
> +	tristate "NXP TJA11xx PHYs support"
> +	depends on HWMON
> +	---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 27d7f9f3b0de..bac339e09042 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -82,6 +82,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..11b8701e78fd
> --- /dev/null
> +++ b/drivers/net/phy/nxp-tja11xx.c
> @@ -0,0 +1,423 @@
> +// 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>
> +#include <linux/hwmon.h>
> +#include <linux/bitfield.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_INTSRC_TEMP_ERR		BIT(1)
> +#define MII_INTSRC_UV_ERR		BIT(3)
> +
> +#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_priv {
> +	char		*hwmon_name;
> +	struct device	*hwmon_dev;
> +};
> +
> +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, GENMASK(15, 0) },
> +	{ "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, GENMASK(7, 0) },
> +	{ "phy_loc_rcvr_count", 26, 8, GENMASK(15, 8) },
> +};
> +
> +static int tja11xx_check(struct phy_device *phydev, u8 reg, u16 mask, u16 set)
> +{
> +	int i, ret;
> +
> +	for (i = 0; i < 200; i++) {
> +		ret = phy_read(phydev, reg);
> +		if (ret < 0)
> +			return ret;
> +
> +		if ((ret & mask) == set)
> +			return 0;
> +
> +		usleep_range(100, 150);
> +	}
> +
> +	return -ETIMEDOUT;
> +}
> +
> +static int phy_modify_check(struct phy_device *phydev, u8 reg,
> +			    u16 mask, u16 set)
> +{
> +	int ret;
> +
> +	ret = phy_modify(phydev, reg, mask, set);
> +	if (ret)
> +		return ret;
> +
> +	return tja11xx_check(phydev, reg, mask, 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->autoneg = AUTONEG_DISABLE;
> +	phydev->speed = SPEED_100;
> +	phydev->duplex = DUPLEX_FULL;
> +
> +	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;
> +
> +	if (phydev->link) {
> +		ret = phy_read(phydev, MII_COMMSTAT);
> +		if (ret < 0)
> +			return ret;
> +
> +		if (!(ret & MII_COMMSTAT_LINK_UP))
> +			phydev->link = 0;
> +	}
> +
> +	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 int tja11xx_hwmon_read(struct device *dev,
> +			      enum hwmon_sensor_types type,
> +			      u32 attr, int channel, long *value)
> +{
> +	struct phy_device *phydev = dev_get_drvdata(dev);
> +	int ret;
> +
> +	if (type == hwmon_in && attr == hwmon_in_lcrit_alarm) {
> +		ret = phy_read(phydev, MII_INTSRC);
> +		if (ret < 0)
> +			return ret;
> +
> +		*value = !!(ret & MII_INTSRC_TEMP_ERR);
> +		return 0;
> +	}
> +
> +	if (type == hwmon_temp && attr == hwmon_temp_crit_alarm) {
> +		ret = phy_read(phydev, MII_INTSRC);
> +		if (ret < 0)
> +			return ret;
> +
> +		*value = !!(ret & MII_INTSRC_UV_ERR);
> +		return 0;
> +	}
> +
> +	return -EOPNOTSUPP;
> +}
> +
> +static umode_t tja11xx_hwmon_is_visible(const void *data,
> +					enum hwmon_sensor_types type,
> +					u32 attr, int channel)
> +{
> +	if (type == hwmon_in && attr == hwmon_in_lcrit_alarm)
> +		return 0444;
> +
> +	if (type == hwmon_temp && attr == hwmon_temp_crit_alarm)
> +		return 0444;
> +
> +	return 0;
> +}
> +
> +static u32 tja11xx_hwmon_in_config[] = {
> +	HWMON_I_LCRIT_ALARM,
> +	0
> +};
> +
> +static const struct hwmon_channel_info tja11xx_hwmon_in = {
> +	.type		= hwmon_in,
> +	.config		= tja11xx_hwmon_in_config,
> +};
> +
> +static u32 tja11xx_hwmon_temp_config[] = {
> +	HWMON_T_CRIT_ALARM,
> +	0
> +};
> +
> +static const struct hwmon_channel_info tja11xx_hwmon_temp = {
> +	.type		= hwmon_temp,
> +	.config		= tja11xx_hwmon_temp_config,
> +};
> +
> +static const struct hwmon_channel_info *tja11xx_hwmon_info[] = {
> +	&tja11xx_hwmon_in,
> +	&tja11xx_hwmon_temp,
> +	NULL
> +};
> +
You might want to consider using the new HWMON_CHANNEL_INFO() macro
to simplify above boilerplate code.

Guenter

> +static const struct hwmon_ops tja11xx_hwmon_hwmon_ops = {
> +	.is_visible	= tja11xx_hwmon_is_visible,
> +	.read		= tja11xx_hwmon_read,
> +};
> +
> +static const struct hwmon_chip_info tja11xx_hwmon_chip_info = {
> +	.ops		= &tja11xx_hwmon_hwmon_ops,
> +	.info		= tja11xx_hwmon_info,
> +};
> +
> +static int tja11xx_probe(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	struct tja11xx_priv *priv;
> +	int i;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
> +	if (!priv->hwmon_name)
> +		return -ENOMEM;
> +
> +	for (i = 0; priv->hwmon_name[i]; i++)
> +		if (hwmon_is_bad_char(priv->hwmon_name[i]))
> +			priv->hwmon_name[i] = '_';
> +
> +	priv->hwmon_dev =
> +		devm_hwmon_device_register_with_info(dev, priv->hwmon_name,
> +						     phydev,
> +						     &tja11xx_hwmon_chip_info,
> +						     NULL);
> +
> +	return PTR_ERR_OR_ZERO(priv->hwmon_dev);
> +}
> +
> +static struct phy_driver tja11xx_driver[] = {
> +	{
> +		PHY_ID_MATCH_MODEL(PHY_ID_TJA1100),
> +		.name		= "NXP TJA1100",
> +		.features       = PHY_BASIC_T1_FEATURES,
> +		.probe		= tja11xx_probe,
> +		.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_MATCH_MODEL(PHY_ID_TJA1101),
> +		.name		= "NXP TJA1101",
> +		.features       = PHY_BASIC_T1_FEATURES,
> +		.probe		= tja11xx_probe,
> +		.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_MATCH_MODEL(PHY_ID_TJA1100) },
> +	{ PHY_ID_MATCH_MODEL(PHY_ID_TJA1101) },
> +	{ }
> +};
> +
> +MODULE_DEVICE_TABLE(mdio, tja11xx_tbl);
> +
> +MODULE_AUTHOR("Marek Vasut <marex@denx.de>");
> +MODULE_DESCRIPTION("NXP TJA11xx BoardR-Reach PHY driver");
> +MODULE_LICENSE("GPL");
> 


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH V5] net: phy: tja11xx: Add TJA11xx PHY driver
  2019-05-27 15:44 ` Guenter Roeck
@ 2019-05-28 18:17   ` Marek Vasut
  2019-05-28 19:34   ` Andrew Lunn
  1 sibling, 0 replies; 18+ messages in thread
From: Marek Vasut @ 2019-05-28 18:17 UTC (permalink / raw)
  To: Guenter Roeck, netdev
  Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, Jean Delvare,
	linux-hwmon

On 5/27/19 5:44 PM, Guenter Roeck wrote:
> On 5/17/19 4:51 PM, 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>
>> Cc: Andrew Lunn <andrew@lunn.ch>
>> Cc: Florian Fainelli <f.fainelli@gmail.com>
>> Cc: Guenter Roeck <linux@roeck-us.net>
>> Cc: Heiner Kallweit <hkallweit1@gmail.com>
>> Cc: Jean Delvare <jdelvare@suse.com>
>> Cc: linux-hwmon@vger.kernel.org

[...]

>> +static u32 tja11xx_hwmon_in_config[] = {
>> +    HWMON_I_LCRIT_ALARM,
>> +    0
>> +};
>> +
>> +static const struct hwmon_channel_info tja11xx_hwmon_in = {
>> +    .type        = hwmon_in,
>> +    .config        = tja11xx_hwmon_in_config,
>> +};
>> +
>> +static u32 tja11xx_hwmon_temp_config[] = {
>> +    HWMON_T_CRIT_ALARM,
>> +    0
>> +};
>> +
>> +static const struct hwmon_channel_info tja11xx_hwmon_temp = {
>> +    .type        = hwmon_temp,
>> +    .config        = tja11xx_hwmon_temp_config,
>> +};
>> +
>> +static const struct hwmon_channel_info *tja11xx_hwmon_info[] = {
>> +    &tja11xx_hwmon_in,
>> +    &tja11xx_hwmon_temp,
>> +    NULL
>> +};
>> +
> You might want to consider using the new HWMON_CHANNEL_INFO() macro
> to simplify above boilerplate code.
Patch posted

[...]

-- 
Best regards,
Marek Vasut

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH V5] net: phy: tja11xx: Add TJA11xx PHY driver
  2019-05-27 15:44 ` Guenter Roeck
  2019-05-28 18:17   ` Marek Vasut
@ 2019-05-28 19:34   ` Andrew Lunn
  1 sibling, 0 replies; 18+ messages in thread
From: Andrew Lunn @ 2019-05-28 19:34 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Marek Vasut, netdev, Florian Fainelli, Heiner Kallweit,
	Jean Delvare, linux-hwmon

On Mon, May 27, 2019 at 08:44:24AM -0700, Guenter Roeck wrote:
> >+static u32 tja11xx_hwmon_in_config[] = {
> >+	HWMON_I_LCRIT_ALARM,
> >+	0
> >+};
> >+
> >+static const struct hwmon_channel_info tja11xx_hwmon_in = {
> >+	.type		= hwmon_in,
> >+	.config		= tja11xx_hwmon_in_config,
> >+};
> >+
> >+static u32 tja11xx_hwmon_temp_config[] = {
> >+	HWMON_T_CRIT_ALARM,
> >+	0
> >+};
> >+
> >+static const struct hwmon_channel_info tja11xx_hwmon_temp = {
> >+	.type		= hwmon_temp,
> >+	.config		= tja11xx_hwmon_temp_config,
> >+};
> >+
> >+static const struct hwmon_channel_info *tja11xx_hwmon_info[] = {
> >+	&tja11xx_hwmon_in,
> >+	&tja11xx_hwmon_temp,
> >+	NULL
> >+};
> >+
> You might want to consider using the new HWMON_CHANNEL_INFO() macro
> to simplify above boilerplate code.

Hi Guenter

That is a nice simplification. Could you run the semantic patch
in drivers/net/phy and submit the results.

Thanks
	Andrew

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2019-05-28 19:35 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-05-17 23:51 [PATCH V5] net: phy: tja11xx: Add TJA11xx PHY driver Marek Vasut
2019-05-18 14:14 ` Andrew Lunn
2019-05-18 16:50   ` Marek Vasut
2019-05-18 20:12     ` Andrew Lunn
2019-05-18 20:46       ` Marek Vasut
2019-05-20 17:21     ` Florian Fainelli
2019-05-22 21:48   ` Marek Vasut
2019-05-22 22:14     ` Andrew Lunn
2019-05-22 22:42       ` Marek Vasut
2019-05-23  2:38 ` Florian Fainelli
2019-05-23 19:48   ` Vladimir Oltean
2019-05-24 11:41     ` Marek Vasut
2019-05-24 13:52       ` Andrew Lunn
2019-05-24 14:23         ` Marek Vasut
2019-05-24 14:28           ` Andrew Lunn
2019-05-27 15:44 ` Guenter Roeck
2019-05-28 18:17   ` Marek Vasut
2019-05-28 19:34   ` Andrew Lunn

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.