| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 13 Dec 2018 11:49:58 +0800
From: Yunsheng Lin <linyunsheng@...wei.com>
To: Marek Vasut <marex@...x.de>, <netdev@...r.kernel.org>
CC: <f.fainelli@...il.com>, <andrew@...n.ch>
Subject: Re: [PATCH] net: phy: tja11xx: Add TJA11xx PHY driver
On 2018/12/13 10:01, Marek Vasut wrote:
> Add driver for the NXP TJA1100 and TJA1101 PHYs. These PHYs are special
> BroadRReach 100BaseT1 PHYs used in automotive.
Hi,
a few minor improvement inline.
>
> Signed-off-by: Marek Vasut <marex@...x.de>
> ---
> drivers/net/phy/Kconfig | 5 +
> drivers/net/phy/Makefile | 1 +
> drivers/net/phy/nxp-tja11xx.c | 348 ++++++++++++++++++++++++++++++++++
> 3 files changed, 354 insertions(+)
> create mode 100644 drivers/net/phy/nxp-tja11xx.c
>
> diff --git a/drivers/net/phy/Kconfig b/drivers/net/phy/Kconfig
> index cd931cf9dcc26..030960158a2ba 100644
> --- a/drivers/net/phy/Kconfig
> +++ b/drivers/net/phy/Kconfig
> @@ -356,6 +356,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 00f097e18c68a..242cb5ffe676c 100644
> --- a/drivers/net/phy/Makefile
> +++ b/drivers/net/phy/Makefile
> @@ -71,6 +71,7 @@ obj-$(CONFIG_MICREL_PHY) += micrel.o
> obj-$(CONFIG_MICROCHIP_PHY) += microchip.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_ROCKCHIP_PHY) += rockchip.o
> diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
> new file mode 100644
> index 0000000000000..22f9c091befac
> --- /dev/null
> +++ b/drivers/net/phy/nxp-tja11xx.c
> @@ -0,0 +1,348 @@
> +// 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_rmw(struct phy_device *phydev, u8 reg, u16 clr, u16 set)
> +{
> + int ret;
> +
> + ret = phy_read(phydev, reg);
> + if (ret < 0)
> + return ret;
> +
> + ret &= ~clr;
> + ret |= set;
> +
> + ret = phy_write(phydev, reg, ret);
> + return ret < 0 ? ret : 0;
> +}
It seems tja11xx_rmw is doing the same thing as __phy_modify ?
> +
> +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 tja11xx_rmw_check(struct phy_device *phydev, u8 reg,
> + u16 clr, u16 set)
> +{
> + int ret;
> +
> + ret = tja11xx_rmw(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 tja11xx_rmw(phydev, MII_ECTRL, MII_ECTRL_CONFIG_EN,
> + MII_ECTRL_CONFIG_EN);
> +}
> +
> +static int tja11xx_enable_link_control(struct phy_device *phydev, bool en)
The function name suggests that it only do enabling, but it also do disabling,
maybe:
static int tja11xx_enable_link_controll(struct phy_device *phydev)
static int tja11xx_disable_link_controll(struct phy_device *phydev)
> +{
> + return tja11xx_rmw(phydev, MII_ECTRL, MII_ECTRL_LINK_CONTROL,
> + en ? MII_ECTRL_CONFIG_EN : 0);
> +}
> +
> +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 = tja11xx_rmw(phydev, MII_ECTRL, MII_ECTRL_WAKE_REQUEST,
> + MII_ECTRL_WAKE_REQUEST);
> + if (ret)
> + return ret;
> +
> + ret = tja11xx_rmw(phydev, MII_ECTRL, MII_ECTRL_WAKE_REQUEST, 0);
> + if (ret)
> + return ret;
> + break;
> + case MII_ECTRL_POWER_MODE_STANDBY:
> + ret = tja11xx_rmw_check(phydev, MII_ECTRL,
> + MII_ECTRL_POWER_MODE_MASK,
> + MII_ECTRL_POWER_MODE_STANDBY);
> + if (ret)
> + return ret;
> +
> + ret = tja11xx_rmw(phydev, MII_ECTRL, MII_ECTRL_POWER_MODE_MASK,
> + MII_ECTRL_POWER_MODE_NORMAL);
> + if (ret)
> + return ret;
> +
> + ret = tja11xx_rmw_check(phydev, MII_GENSTAT,
> + MII_GENSTAT_PLL_LOCKED,
> + MII_GENSTAT_PLL_LOCKED);
> + if (ret)
> + return ret;
> +
> + return tja11xx_enable_link_control(phydev, true);
> + default:
> + break;
> + }
> +
> + return 0;
> +}
> +
> +static int tja11xx_soft_reset(struct phy_device *phydev)
> +{
> + int ret;
> +
> + ret = tja11xx_enable_reg_write(phydev);
> + if (ret < 0)
Perhaps if (ret) ? because in tja11xx_config_init it only do if (ret).
> + return ret;
> +
> + ret = phy_write(phydev, 0x0, 0xa100);
> + return ret < 0 ? ret : 0;
> +}
> +
> +static int tja11xx_config_aneg(struct phy_device *phydev)
> +{
> + phydev->autoneg = 0;
> + phydev->speed = SPEED_100;
> + phydev->duplex = DUPLEX_FULL;
> + phydev->pause = 0;
> + phydev->asym_pause = 0;
> +
> + return 0;
> +}
> +
> +static int tja11xx_config_init(struct phy_device *phydev)
> +{
> + u32 features = SUPPORTED_TP | SUPPORTED_MII | SUPPORTED_100baseT_Full;
> + int ret;
> +
> + ret = tja11xx_enable_reg_write(phydev);
> + if (ret)
> + return ret;
> +
> + phydev->supported &= features;
> + phydev->advertising &= features;
> + phydev->irq = PHY_POLL;
> + tja11xx_config_aneg(phydev);
> +
> + switch (phydev->phy_id & PHY_ID_MASK) {
> + case PHY_ID_TJA1100:
> + ret = tja11xx_rmw(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 = tja11xx_rmw(phydev, MII_COMMCFG, MII_COMMCFG_AUTO_OP,
> + MII_COMMCFG_AUTO_OP);
> + if (ret)
> + return ret;
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + ret = tja11xx_rmw(phydev, MII_CFG1, MII_CFG1_SLEEP_CONFIRM, 0);
> + if (ret)
> + return ret;
> +
> + ret = tja11xx_rmw(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)
Same here?
> + 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_read_status(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 = SUPPORTED_MII | SUPPORTED_TP |
> + SUPPORTED_100baseT_Full,
> + .soft_reset = tja11xx_soft_reset,
> + .config_init = tja11xx_config_init,
> + .config_aneg = tja11xx_config_aneg,
> + .aneg_done = genphy_aneg_done,
> + .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 = SUPPORTED_MII | SUPPORTED_TP |
> + SUPPORTED_100baseT_Full,
> + .soft_reset = tja11xx_soft_reset,
> + .config_init = tja11xx_config_init,
> + .config_aneg = tja11xx_config_aneg,
> + .aneg_done = genphy_aneg_done,
> + .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 BroadR-Reach PHY driver");
> +MODULE_LICENSE("GPL");
>
Powered by blists - more mailing lists