Date:   Sat, 15 Dec 2018 19:06:54 +0100
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Marek Vasut <marex@...x.de>, netdev@...r.kernel.org
Cc:     f.fainelli@...il.com, andrew@...n.ch
Subject: Re: [PATCH V2] net: phy: tja11xx: Add TJA11xx PHY driver

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@...x.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@...x.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@...x.de>");
>> +MODULE_DESCRIPTION("NXP TJA11xx BoardR-Reach PHY driver");
>> +MODULE_LICENSE("GPL");
>>
> 

Powered by blists - more mailing lists