On 12/13/2018 04:49 AM, Yunsheng Lin wrote:
> 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.
Hi,
>>
>> Signed-off-by: Marek Vasut <ma...@denx.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 <ma...@denx.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 ?
phy_modify(), yes, let's use that.
>> +
>> +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:
No, I can just drop the $en
> 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).
Yes
Thanks
--
Best regards,
Marek Vasut
