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

* RE: Re: [PATCH v1] net: phy: tja11xx: add TJA1102 support
@ 2020-03-03 12:42 Christian Herber
  2020-03-03 12:53 ` Marc Kleine-Budde
  2020-03-03 12:55 ` Oleksij Rempel
  0 siblings, 2 replies; 5+ messages in thread
From: Christian Herber @ 2020-03-03 12:42 UTC (permalink / raw)
  To: Heiner Kallweit, Oleksij Rempel, Andrew Lunn, Florian Fainelli
  Cc: Pengutronix Kernel Team, linux-kernel, David S. Miller, netdev,
	Marek Vasut, David Jander

> 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@pengutronix.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

^ permalink raw reply	[flat|nested] 5+ messages in thread