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:06:54 +0100 Message-ID: <3cb8b832-b1b8-783e-d1c6-0e1859849775@gmail.com> References: <20181214161149.6493-1-marex@denx.de> <13845d07-3958-dd55-5e2f-bb088db057e7@gmail.com> 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-f67.google.com ([209.85.221.67]:40458 "EHLO mail-wr1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726520AbeLOSHB (ORCPT ); Sat, 15 Dec 2018 13:07:01 -0500 Received: by mail-wr1-f67.google.com with SMTP id p4so8396779wrt.7 for ; Sat, 15 Dec 2018 10:06:59 -0800 (PST) In-Reply-To: <13845d07-3958-dd55-5e2f-bb088db057e7@gmail.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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 >> --- >> 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; 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 "); >> +MODULE_DESCRIPTION("NXP TJA11xx BoardR-Reach PHY driver"); >> +MODULE_LICENSE("GPL"); >> >