diff mbox series

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

Message ID 20200303073715.32301-1-o.rempel@pengutronix.de
State Changes Requested
Delegated to: David Miller
Headers show
Series [v1] net: phy: tja11xx: add TJA1102 support | expand

Commit Message

Oleksij Rempel March 3, 2020, 7:37 a.m. UTC
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(+)

Comments

Heiner Kallweit March 3, 2020, 8:46 a.m. UTC | #1
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) },
>  	{ }
>  };
>  
>
Marc Kleine-Budde March 3, 2020, 8:56 a.m. UTC | #2
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
Marek Vasut March 3, 2020, 12:18 p.m. UTC | #3
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 ?
Marc Kleine-Budde March 3, 2020, 12:29 p.m. UTC | #4
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
Andrew Lunn March 3, 2020, 1:59 p.m. UTC | #5
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
Oleksij Rempel March 3, 2020, 3:36 p.m. UTC | #6
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
Heiner Kallweit March 3, 2020, 7:50 p.m. UTC | #7
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
diff mbox series

Patch

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