| 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
| ||
|
Message-ID: <bf03038b-19f3-86fe-6c98-fe576f1f364b@denx.de>
Date: Sat, 22 Dec 2018 00:31:26 +0100
From: Marek Vasut <marex@...x.de>
To: Heiner Kallweit <hkallweit1@...il.com>, netdev@...r.kernel.org
Cc: f.fainelli@...il.com, andrew@...n.ch
Subject: Re: [PATCH V2] net: phy: tja11xx: Add TJA11xx PHY driver
On 12/15/2018 07:01 PM, 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
Fixed
>> +{
>> + 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?
Yes, which is why the check is here. The generic bit seems to be stuck
at times, which is why we check the commstat bit too. It'd be nice to
get a proper explanation from someone from NXP, but I doubt that's ever
gonna happen.
>> + 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.
OK
>
>> + .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");
>>
>
--
Best regards,
Marek Vasut
Powered by blists - more mailing lists