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

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

[Christian Herber]

> 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

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

[Marc Kleine-Budde]

On 3/3/20 1:42 PM, Christian Herber wrote:
> You could also refer the solution implemented here as part of a TJA110x driver:
> https://source.codeaurora.org/external/autoivnsw/tja110x_linux_phydev/about/

Do you mean this?
https://source.codeaurora.org/external/autoivnsw/tja110x_linux_phydev/tree/tja110x.c#n26

| /* load driver for TJA1102p1. It needs to be ensured,
|  * that no other mdio device with phy id 0 is present
|  */
| #define CONFIG_TJA1102_FIX

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

[Oleksij Rempel]

On 03.03.20 13:42, Christian Herber wrote:
>> 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/

OK, thank you!

Suddenly, the solution in this driver is not mainlainable. It may match on ther PHYs with 
PHYID == 0.

See this part of the code:
#define NXP_PHY_ID_TJA1102P1      (0x00000000U)
...
	, {
	.phy_id = NXP_PHY_ID_TJA1102P1,
	.name = "TJA1102_p1",
	.phy_id_mask = NXP_PHY_ID_MASK,


Kind regards,
Oleksij Rempel

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

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

[Andrew Lunn]

> > 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/
> 
> OK, thank you!
> 
> Suddenly, the solution in this driver is not mainlainable. It may match on
> ther PHYs with PHYID == 0.
> 
> See this part of the code:
> #define NXP_PHY_ID_TJA1102P1      (0x00000000U)
> ...
> 	, {
> 	.phy_id = NXP_PHY_ID_TJA1102P1,
> 	.name = "TJA1102_p1",
> 	.phy_id_mask = NXP_PHY_ID_MASK,

Noooo

You cannot assume NXP is the only silicon vendor to manufacture broken
silicon with a PHY ID of 0.

	Andrew

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

[Marc Kleine-Budde]

On 3/3/20 3:09 PM, Andrew Lunn wrote:
>>> 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/
>>
>> OK, thank you!
>>
>> Suddenly, the solution in this driver is not mainlainable. It may match on
>> ther PHYs with PHYID == 0.
>>
>> See this part of the code:
>> #define NXP_PHY_ID_TJA1102P1      (0x00000000U)
>> ...
>> 	, {
>> 	.phy_id = NXP_PHY_ID_TJA1102P1,
>> 	.name = "TJA1102_p1",
>> 	.phy_id_mask = NXP_PHY_ID_MASK,
> 
> Noooo
> 
> You cannot assume NXP is the only silicon vendor to manufacture broken
> silicon with a PHY ID of 0.

ACK, we ruled that soultion out already :)

We'll look into your and Marek's suggestions.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

[Oleksij Rempel]

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;
+	}
+
 	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) },
 	{ }
 };
 
-- 
2.25.0

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

[Heiner Kallweit]

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) },
>  	{ }
>  };
>  
> 

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

[Marc Kleine-Budde]

On 3/3/20 9:46 AM, Heiner Kallweit wrote:
> 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".
    ^^^^^^^^^^^^^^^^^^^^^^^^

> 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?

Yes, Port 0 and Port 1 have seperate MFIO bus addresses, but only Port 0
has a proper phy_id (== PHY_ID_TJA1102), while Port 1 just has 0.

Currently we register Port 1 via the device tree compatible
"ethernet-phy-id0180.dc81".

> 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)

But we "force" it via the DT compatible. Another option would be to make
up a phyid for Port 1 and hope that it doesn't collide with a real phy
id in other or upcoming chips. But that sounds not like a clean solution
either.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

[Heiner Kallweit]

On 03.03.2020 09:56, Marc Kleine-Budde wrote:
> On 3/3/20 9:46 AM, Heiner Kallweit wrote:
>> 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".
>     ^^^^^^^^^^^^^^^^^^^^^^^^
> 
>> 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?
> 
> Yes, Port 0 and Port 1 have seperate MFIO bus addresses, but only Port 0
> has a proper phy_id (== PHY_ID_TJA1102), while Port 1 just has 0.
> 
> Currently we register Port 1 via the device tree compatible
> "ethernet-phy-id0180.dc81".
> 
Thanks for the explanation. Missed that part, then reading the PHY ID
of the chip is skipped. For checking whether we are port 0 or 1 it wouldn't
even be needed to read both PHY ID registers.
One drawback of the solution might be that it works on DT-configured
systems only (not sure how relevant this is). A little bit more detailed
comment may be helpful, to avoid misunderstandings like mine.

>> 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)
> 
> But we "force" it via the DT compatible. Another option would be to make
> up a phyid for Port 1 and hope that it doesn't collide with a real phy
> id in other or upcoming chips. But that sounds not like a clean solution
> either.
> 
> Marc
> 
Heiner

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

[Marek Vasut]

On 3/3/20 8:37 AM, 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.

Can't you do some magic with match_phy_device (like in
8b95599c55ed24b36cf44a4720067cfe67edbcb4) to discern the second half of
the PHY ?

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

[Marc Kleine-Budde]

On 3/3/20 1:18 PM, Marek Vasut wrote:
> On 3/3/20 8:37 AM, 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.
> 
> Can't you do some magic with match_phy_device (like in
> 8b95599c55ed24b36cf44a4720067cfe67edbcb4) to discern the second half of
> the PHY ?

Great, that looks better.

Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde           |
Embedded Linux                   | https://www.pengutronix.de  |
Vertretung West/Dortmund         | Phone: +49-231-2826-924     |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-5555 |

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

[Andrew Lunn]

Hi Oleksij

> 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".

Why-o-why do silicon vendors make devices with invalid PHY IDs!?!?!

Did you try avoiding the compatible string. We know PHY 0 will probe
as normal. From its PHY ID we know it is a dual device. Could the
probe of PHY 0 register PHY 1?

No idea if it will work, but could nxp-tja11xx.c register is fixup for
PHY_ID_TJA1102. That fixup would do something like:

void tja1102_fixup(struct phy_device *phydev_phy0)
{
        struct mii_bus *bus = phydev_phy0->mdio.mii;
        struct phy_device *phydev_phy1;

        phydev_phy1 = phy_device_create(bus, phydev_phy0->addr + 1,
                                        PHY_ID_TJA1102, FALSE, NULL);
	if (phydev_phy1)
               phy_device_register(phydev_phy1);
}

I think the issue here is, it will deadlock when scanning for fixup
for phydev_phy1. So this basic idea, but maybe hooked in somewhere
else?

Something like this might also help vsc8584 which is a quad PHY with
some shared registers?

	  Andrew

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

[Oleksij Rempel]

On Tue, Mar 03, 2020 at 02:59:36PM +0100, Andrew Lunn wrote:
> Hi Oleksij
> 
> > 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".
> 
> Why-o-why do silicon vendors make devices with invalid PHY IDs!?!?!
> 
> Did you try avoiding the compatible string. We know PHY 0 will probe
> as normal. From its PHY ID we know it is a dual device. Could the
> probe of PHY 0 register PHY 1?
> 
> No idea if it will work, but could nxp-tja11xx.c register is fixup for
> PHY_ID_TJA1102. That fixup would do something like:
> 
> void tja1102_fixup(struct phy_device *phydev_phy0)
> {
>         struct mii_bus *bus = phydev_phy0->mdio.mii;
>         struct phy_device *phydev_phy1;
> 
>         phydev_phy1 = phy_device_create(bus, phydev_phy0->addr + 1,
>                                         PHY_ID_TJA1102, FALSE, NULL);
> 	if (phydev_phy1)
>                phy_device_register(phydev_phy1);
> }
> 
> I think the issue here is, it will deadlock when scanning for fixup
> for phydev_phy1. So this basic idea, but maybe hooked in somewhere
> else?
> 
> Something like this might also help vsc8584 which is a quad PHY with
> some shared registers?

OK, thx! I'll take a look on it.
Currently there is not solved issues with controlled power on/reset sequence
of this chip. The reset and enable pins will affect both PHYs. So, may be vsc8584
will answer my questions.

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |