From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiner Kallweit Subject: Re: [PATCH V2] net: phy: tja11xx: Add TJA11xx PHY driver Date: Sat, 15 Dec 2018 19:01:18 +0100 Message-ID: <13845d07-3958-dd55-5e2f-bb088db057e7@gmail.com> References: <20181214161149.6493-1-marex@denx.de> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: f.fainelli@gmail.com, andrew@lunn.ch To: Marek Vasut , netdev@vger.kernel.org Return-path: Received: from mail-wr1-f66.google.com ([209.85.221.66]:35549 "EHLO mail-wr1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1729270AbeLOSB2 (ORCPT ); Sat, 15 Dec 2018 13:01:28 -0500 Received: by mail-wr1-f66.google.com with SMTP id 96so8409599wrb.2 for ; Sat, 15 Dec 2018 10:01:25 -0800 (PST) In-Reply-To: <20181214161149.6493-1-marex@denx.de> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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 > --- > 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 > + */ > +#include > +#include > +#include > +#include > +#include > +#include > + > +#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 "); > +MODULE_DESCRIPTION("NXP TJA11xx BoardR-Reach PHY driver"); > +MODULE_LICENSE("GPL"); >