netdev - RE: Re: [PATCH v1] net: phy: tja11xx: add TJA1102 support

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Date:   Tue, 3 Mar 2020 12:42:06 +0000
From:   Christian Herber <christian.herber@....com>
To:     Heiner Kallweit <hkallweit1@...il.com>,
        Oleksij Rempel <o.rempel@...gutronix.de>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>
CC:     Pengutronix Kernel Team <kernel@...gutronix.de>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Marek Vasut <marex@...x.de>, David Jander <david@...tonic.nl>
Subject: RE: Re: [PATCH v1] net: phy: tja11xx: add TJA1102 support

> On 03.03.2020 08:37, Oleksij Rempel wrote:
>> TJA1102 is an dual T1 PHY chip. Both PHYs are separately addressable.
>> PHY 0 can be identified by PHY ID. PHY 1 has no PHY ID and can be
>> configured in device tree by setting compatible =
>> "ethernet-phy-id0180.dc81".
>>
>> PHY 1 has less suported registers and functionality. For current driver
>> it will affect only the HWMON support.
>>
>> Signed-off-by: Oleksij Rempel <o.rempel@...gutronix.de>
>> ---
>> drivers/net/phy/nxp-tja11xx.c | 43 +++++++++++++++++++++++++++++++++++
>> 1 file changed, 43 insertions(+)
>>
>> diff --git a/drivers/net/phy/nxp-tja11xx.c b/drivers/net/phy/nxp-tja11xx.c
>> index b705d0bd798b..52090cfaa54e 100644
>> --- a/drivers/net/phy/nxp-tja11xx.c
>> +++ b/drivers/net/phy/nxp-tja11xx.c
>> @@ -15,6 +15,7 @@
>> #define PHY_ID_MASK                  0xfffffff0
>> #define PHY_ID_TJA1100                       0x0180dc40
>> #define PHY_ID_TJA1101                       0x0180dd00
>> +#define PHY_ID_TJA1102                       0x0180dc80
>>
>> #define MII_ECTRL                    17
>> #define MII_ECTRL_LINK_CONTROL               BIT(15)
>> @@ -190,6 +191,7 @@ static int tja11xx_config_init(struct phy_device *phydev)
>>              return ret;
>>      break;
>> case PHY_ID_TJA1101:
>> +     case PHY_ID_TJA1102:
>>      ret = phy_set_bits(phydev, MII_COMMCFG, MII_COMMCFG_AUTO_OP);
>>      if (ret)
>>              return ret;
>> @@ -337,6 +339,31 @@ static int tja11xx_probe(struct phy_device *phydev)
>> if (!priv)
>>      return -ENOMEM;
>>
>> +     /* Use the phyid to distinguish between port 0 and port 1 of the
>> +      * TJA1102. Port 0 has a proper phyid, while port 1 reads 0.
>> +      */
>> +     if ((phydev->phy_id & PHY_ID_MASK) == PHY_ID_TJA1102) {
>> +             int ret;
>> +             u32 id;
>> +
>> +             ret = phy_read(phydev, MII_PHYSID1);
>> +             if (ret < 0)
>> +                     return ret;
>> +
>> +             id = ret;
>> +             ret = phy_read(phydev, MII_PHYSID2);
>> +             if (ret < 0)
>> +                     return ret;
>> +
>> +             id |= ret << 16;
>> +
>> +             /* TJA1102 Port 1 has phyid 0 and doesn't support temperature
>> +              * and undervoltage alarms.
>> +              */
>> +             if (id == 0)
>> +                     return 0;
>
> I'm not sure I understand what you're doing here. The two ports of the chip
> are separate PHY's on individual MDIO bus addresses?
> Reading the PHY ID registers here seems to repeat what phylib did already
> to populate phydev->phy_id. If port 1 has PHD ID 0 then the driver wouldn't
> bind and tja11xx_probe() would never be called (see phy_bus_match)
>
>> +     }
>> +
>> priv->hwmon_name = devm_kstrdup(dev, dev_name(dev), GFP_KERNEL);
>> if (!priv->hwmon_name)
>>      return -ENOMEM;
>> @@ -385,6 +412,21 @@ static struct phy_driver tja11xx_driver[] = {
>>      .get_sset_count = tja11xx_get_sset_count,
>>      .get_strings    = tja11xx_get_strings,
>>      .get_stats      = tja11xx_get_stats,
>> +     }, {
>> +             PHY_ID_MATCH_MODEL(PHY_ID_TJA1102),
>> +             .name           = "NXP TJA1102",
>> +             .features       = PHY_BASIC_T1_FEATURES,
>> +             .probe          = tja11xx_probe,
>> +             .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,
>> }
>> };
>>
>> @@ -393,6 +435,7 @@ module_phy_driver(tja11xx_driver);
>> static struct mdio_device_id __maybe_unused tja11xx_tbl[] = {
>> { PHY_ID_MATCH_MODEL(PHY_ID_TJA1100) },
>> { PHY_ID_MATCH_MODEL(PHY_ID_TJA1101) },
>> +     { PHY_ID_MATCH_MODEL(PHY_ID_TJA1102) },
>> { }
>> };

Hi Oleksij, Heiner, Marc, 

You could also refer the solution implemented here as part of a TJA110x driver:
https://source.codeaurora.org/external/autoivnsw/tja110x_linux_phydev/about/

Regards, Christian